[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZB3kNXpNm9DTRxHH@euler>
Date: Fri, 24 Mar 2023 10:56:05 -0700
From: Colin Foster <colin.foster@...advantage.com>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>
Cc: Andrew Lunn <andrew@...n.ch>, Mark Brown <broonie@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
rafael@...nel.org, Vladimir Oltean <vladimir.oltean@....com>,
Lee Jones <lee@...nel.org>, davem@...emloft.net,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
Heiner Kallweit <hkallweit1@...il.com>,
Russell King <linux@...linux.org.uk>,
linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
thomas.petazzoni@...tlin.com
Subject: Re: [RFC 4/7] mfd: ocelot-spi: Change the regmap stride to reflect
the real one
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@...n.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
Powered by blists - more mailing lists