| Message ID | 20230324093644.464704-5-maxime.chevallier@bootlin.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | Introduce a generic regmap-based MDIO driver | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply, async |
On Fri, Mar 24, 2023 at 10:36:41AM +0100, Maxime Chevallier wrote: > When used over SPI, the register addresses needs to be translated, > compared to when used over MMIO. The translation consists in applying an > offset with reg_base, then downshifting the registers by 2. This > actually changes the register stride from 4 to 1. > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> > --- > drivers/mfd/ocelot-spi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c > index 2d1349a10ca9..107cda0544aa 100644 > --- a/drivers/mfd/ocelot-spi.c > +++ b/drivers/mfd/ocelot-spi.c > @@ -124,7 +124,7 @@ static int ocelot_spi_initialize(struct device *dev) > > static const struct regmap_config ocelot_spi_regmap_config = { > .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? Andrew
Hello Andrew, On Fri, 24 Mar 2023 13:11:07 +0100 Andrew Lunn <andrew@lunn.ch> wrote: > On Fri, Mar 24, 2023 at 10:36:41AM +0100, Maxime Chevallier wrote: > > When used over SPI, the register addresses needs to be translated, > > compared to when used over MMIO. The translation consists in > > applying an offset with reg_base, then downshifting the registers > > by 2. This actually changes the register stride from 4 to 1. > > > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> > > --- > > drivers/mfd/ocelot-spi.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c > > index 2d1349a10ca9..107cda0544aa 100644 > > --- a/drivers/mfd/ocelot-spi.c > > +++ b/drivers/mfd/ocelot-spi.c > > @@ -124,7 +124,7 @@ static int ocelot_spi_initialize(struct device > > *dev) > > static const struct regmap_config ocelot_spi_regmap_config = { > > .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 :( Thanks, Maxime > Andrew
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: > > > On Fri, Mar 24, 2023 at 10:36:41AM +0100, Maxime Chevallier wrote: > > > When used over SPI, the register addresses needs to be translated, > > > compared to when used over MMIO. The translation consists in > > > applying an offset with reg_base, then downshifting the registers > > > by 2. This actually changes the register stride from 4 to 1. > > > > > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> > > > --- > > > drivers/mfd/ocelot-spi.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c > > > index 2d1349a10ca9..107cda0544aa 100644 > > > --- a/drivers/mfd/ocelot-spi.c > > > +++ b/drivers/mfd/ocelot-spi.c > > > @@ -124,7 +124,7 @@ static int ocelot_spi_initialize(struct device > > > *dev) > > > static const struct regmap_config ocelot_spi_regmap_config = { > > > .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. Sorry for the bug. Thanks for the fix. And I'm glad I'm not the only one taking advantage of the "reg_shift" regmap operation! I thought I'd be the only one. Let me know if you want me to take any action on this fix. Colin
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. (Aside: I'm now curious how the compiler will optimize regmap_reg_addr()) Colin
> > > static const struct regmap_config ocelot_spi_regmap_config = { > > > .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. This is the sort of think which is good to explain in the commit message. That is the place to answer questions reviewers are likely to ask for things which are not obvious from just the patch. Andrew
Hello Andrew, On Mon, 27 Mar 2023 02:02:55 +0200 Andrew Lunn <andrew@lunn.ch> wrote: > > > > static const struct regmap_config ocelot_spi_regmap_config = { > > > > .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. > > This is the sort of think which is good to explain in the commit > message. That is the place to answer questions reviewers are likely to > ask for things which are not obvious from just the patch. That's right, I will include this explanation in the next iteration. Thanks for the review, Maxime > Andrew
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
diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c index 2d1349a10ca9..107cda0544aa 100644 --- a/drivers/mfd/ocelot-spi.c +++ b/drivers/mfd/ocelot-spi.c @@ -124,7 +124,7 @@ static int ocelot_spi_initialize(struct device *dev) static const struct regmap_config ocelot_spi_regmap_config = { .reg_bits = 24, - .reg_stride = 4, + .reg_stride = 1, .reg_shift = REGMAP_DOWNSHIFT(2), .val_bits = 32,
When used over SPI, the register addresses needs to be translated, compared to when used over MMIO. The translation consists in applying an offset with reg_base, then downshifting the registers by 2. This actually changes the register stride from 4 to 1. Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> --- drivers/mfd/ocelot-spi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)