Re: [RFC 4/7] mfd: ocelot-spi: Change the regmap stride to reflect the real one

[Maxime Chevallier]

On Fri, 24 Mar 2023 10:56:05 -0700
Colin Foster <colin.foster@xxxxxxxxxxxxxxxx> wrote:

> On Fri, Mar 24, 2023 at 08:48:18AM -0700, Colin Foster wrote:
> > Hi Maxime,
> > 
> > On Fri, Mar 24, 2023 at 01:48:17PM +0100, Maxime Chevallier wrote:  
> > > Hello Andrew,
> > > 
> > > On Fri, 24 Mar 2023 13:11:07 +0100
> > > Andrew Lunn <andrew@xxxxxxx> wrote:
> > >   
> > > > >  	.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.

Well here I guess it's also about the semantic of reg_stride. Should it
represent the alignment constraints of the register address we feed as
an input to a regmap_read/regmap_write operation, or the alignment
constraints of the underlying bus ? This is kind of a new concern, as
we are now translating register addresses.

I asked myself the same question, so I'm very open for discussion, but
my gut feeling is that the reg_stride is there to make sure we don't
perform an access whose alignment won't work with the bus we are using,
so using a stride of 1 on a memory-mapped device with 2 or 4 byte
register alignment is a bit counter-intuitive.

Thanks a lot for the review, suggestions and tests !

Best regards,

Maxime

> 
> (Aside: I'm now curious how the compiler will optimize
> regmap_reg_addr())
> 
> 
> Colin