lkml.org 
[lkml]   [2021]   [Nov]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v3 09/16] reset: starfive-jh7100: Add StarFive JH7100 reset driver
    On Tue, Nov 2, 2021 at 1:55 PM Yury Norov <yury.norov@gmail.com> wrote:
    >
    > On Tue, Nov 2, 2021 at 12:59 PM Emil Renner Berthing <kernel@esmil.dk> wrote:
    > >
    > > On Tue, 2 Nov 2021 at 20:43, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
    > > > +Cc: Yury (bitmap expert)
    > > >
    > > > On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <kernel@esmil.dk> wrote:
    > > > >
    > > > > Add a driver for the StarFive JH7100 reset controller.
    > > >
    > > > ...
    > > >
    > > > > +#define BIT_MASK32(x) BIT((x) % 32)
    > > >
    > > > Possible namespace collision.
    > > >
    > > > ...
    > > >
    > > > > +/*
    > > > > + * the registers work like a 32bit bitmap, so writing a 1 to the m'th bit of
    > > > > + * the n'th ASSERT register asserts line 32n + m, and writing a 0 deasserts the
    > > > > + * same line.
    >
    > We don't have 32-bit bitmaps. Bitmaps are always arrays of unsigned longs. On a
    > 64-bit system this '32-bit bitmap' may be broken due to endianness issues.
    >
    > > > > + * most reset lines have their status inverted so a 0 in the STATUS register
    > > > > + * means the line is asserted and a 1 means it's deasserted. a few lines don't
    > > > > + * though, so store the expected value of the status registers when all lines
    > > > > + * are asserted.
    > > > > + */
    > > >
    > > > Besides missing capitalization,
    > >
    > > I'm confused. it was you who wanted all comments to capitalized the same..
    > > 64bi
    > > if it sounds like bitmap, use bitmap.
    > > > I have checked DT definitions and it seems you don't even need the
    > > > BIT_MASK() macro,
    > > >
    > > > > +static const u32 jh7100_reset_asserted[4] = {
    > > > > + /* STATUS0 register */
    > > > > + BIT_MASK32(JH7100_RST_U74) |
    >
    > I think we have no BIT_MASK32() for a good reason. Natural alignment is
    > always preferable.
    >
    > > > > + BIT_MASK32(JH7100_RST_VP6_DRESET) |
    > > > > + BIT_MASK32(JH7100_RST_VP6_BRESET),
    > > > > + /* STATUS1 register */
    > > > > + BIT_MASK32(JH7100_RST_HIFI4_DRESET) |
    > > > > + BIT_MASK32(JH7100_RST_HIFI4_BRESET),
    > > > > + /* STATUS2 register */
    > > > > + BIT_MASK32(JH7100_RST_E24),
    > > > > + /* STATUS3 register */
    > > > > + 0,
    > > > > +};
    > > >
    > > > Yury, do we have any clever (clean) way to initialize a bitmap with
    > > > particular bits so that it will be a constant from the beginning? If
    > > > no, any suggestion what we can provide to such users?
    >
    > If you want your array to be a true bitmap, ie, all bitmap functions should
    > work with it correctly, you'd initialize it like this:
    >
    > static const unsigned long jh7100_reset_asserted[] = {
    > BITMAP_FROM_U64(BIT_MASK(JH7100_RST_VP6_DRESET) |
    > BIT_MASK(JH7100_RST_VP6_BRESET) |
    > BIT_MASK(JH7100_RST_HIFI4_DRESET) |
    > BIT_MASK(JH7100_RST_HIFI4_BRESET)),
    > BITMAP_FROM_U64(BIT_MASK(JH7100_RST_E24)),
    > }

    My bad, it should be BIT_ULL_MASK.

    Thanks,
    Yury

    \
     
     \ /
      Last update: 2021-11-10 17:36    [W:4.205 / U:0.320 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site