Messages in this thread | ![/](/images/icornerl.gif) | | Date | Thu, 30 Mar 2023 11:53:07 +0200 | From | Maxime Chevallier <> | Subject | Re: [RFC 4/7] mfd: ocelot-spi: Change the regmap stride to reflect the real one |
| |
On Fri, 24 Mar 2023 10:56:05 -0700 Colin Foster <colin.foster@in-advantage.com> 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@lunn.ch> 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
| ![\](/images/icornerr.gif) |