On Tue, Nov 2, 2021 at 12:59 PM Emil Renner Berthing [off-list ref] wrote:

On Tue, 2 Nov 2021 at 20:43, Andy Shevchenko [off-list ref] wrote:

quoted

+Cc: Yury (bitmap expert)

On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing [off-list ref] wrote:

quoted

Add a driver for the StarFive JH7100 reset controller.

...

quoted

+#define BIT_MASK32(x) BIT((x) % 32)

Possible namespace collision.

...

quoted

+/*
+ * 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.

quoted

quoted

+ * 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.

quoted

I have checked DT definitions and it seems you don't even need the
BIT_MASK() macro,

quoted

+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.

quoted

quoted

+       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)),
}

Look at lib/test_bitmap.c for example, and comment to BITMAP_FROM_U64()
for internal details.

On the other hand, if you hardware has tricky requirements for bit
positions, and they should depend on endianness and/or size of
long in a way not compatible with bitmaps, you probably know better
how to handle this. Just don't refer to your structure as a bitmap.

Thanks,
Yury

The problem is, that even if we could initialize this without the
monstrosity in our last conversation a 64bit bitmap would still
produce worse code. As it is now it's simply a 32bit load and mask
with index and mask already calculated for the registers. In the
status callback the mask can even be folded into the register read
mask. With a 64bit bitmap you'd need to calculate new 64bit index and
masks, and then conditionally shift the bits into position.

If this reflection of the 32bit registers bothers you that much we
could alternatively do

static bool jh7100_reset_inverted(unsigned int idx)
{
  switch (idx) {
  case JH7100_RST_U74:
  case JH7100_RST_VP6_DRESET:
  ..
    return false;
  }
  return true;
}

It'd still produce worse code, but at least it would be readable.

/Emil

quoted

...

quoted

+       dev_dbg(rcdev->dev, "reset(%lu)\n", id);

These debug messages are useless since one should use ftrace facility instead,

--
With Best Regards,
Andy Shevchenko