On Fri, Mar 24, 2023 at 08:48:18AM -0700, Colin Foster wrote:

quoted

Hi Maxime,

On Fri, Mar 24, 2023 at 01:48:17PM +0100, Maxime Chevallier wrote:  

quoted

Hello Andrew,

On Fri, 24 Mar 2023 13:11:07 +0100
Andrew Lunn [off-list ref] wrote:
  

quoted

quoted

 	.reg_bits = 24,
-	.reg_stride = 4,
+	.reg_stride = 1,
 	.reg_shift = REGMAP_DOWNSHIFT(2),
 	.val_bits = 32,    

This does not look like a bisectable change? Or did it never
work before?  

Actually this works in all cases because of "regmap: check for
alignment on translated register addresses" in this series.
Before this series, I think using a stride of 1 would have worked
too, as any 4-byte-aligned accesses are also 1-byte aligned.

But that's also why I need review on this, my understanding is
that reg_stride is used just as a check for alignment, and I
couldn't test this ocelot-related patch on the real HW, so please
take it with a grain of salt :(  

You're exactly right. reg_stride wasn't used anywhere in the
ocelot-spi path before this patch series. When I build against
patch 3 ("regmap: allow upshifting register addresses before
performing operations") ocelot-spi breaks.

[    3.207711] ocelot-soc spi0.0: error -EINVAL: Error initializing
SPI bus

When I build against the whole series, or even just up to patch 4
("mfd: ocelot-spi: Change the regmap stride to reflect the real
one") functionality returns.

If you keep patch 4 and apply it before patch 2, everything should
work.  

I replied too soon, before looking more into patch 2.

Some context from that patch:

--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c

@@ -2016,7 +2016,7 @@ int regmap_write(struct regmap *map, unsigned

int reg, unsigned int val) {
        int ret;

-       if (!IS_ALIGNED(reg, map->reg_stride))
+       if (!IS_ALIGNED(regmap_reg_addr(map, reg), map->reg_stride))
                return -EINVAL;

        map->lock(map->lock_arg);


I don't know whether checking IS_ALIGNED before or after the shift is
the right thing to do. My initial intention was to perform the shift
at the last possible moment before calling into the read / write
routines. That way it wouldn't interfere with any underlying regcache
mechanisms (which aren't used by ocelot-spi)

But to me it seems like patch 2 changes this expected behavior, so the
two patches should be squashed.


... Thinking more about it ...


In ocelot-spi, at the driver layer, we're accessing two registers.
They'd be at address 0x71070000 and 0x71070004. The driver uses those
addresses, so there's a stride of 4. I can't access 0x71070001.

The fact that the translation from "address" to "bits that go out the
SPI bus" shifts out the last two bits and hacks off a couple of the
MSBs doesn't seem like it should affect the 'reg_stride'.


So maybe patches 2 and 4 should be dropped, and your patch 6
alterra_tse_main should use a reg_stride of 1? That has a subtle
benefit of not needing an additional operation or two from
regmap_reg_addr().

Would that cause any issues? Hopefully there isn't something I'm
missing.