As the comments in ocelot-spi.c explain, after a chip reset, the
CFGSTAT register must be written again setting the appropriate number
of padding bytes; otherwise reads are not reliable.
However, the way the code is currently structured violates that: After
the BIT_SOFT_CHIP_RST is written, ocelot_chip_reset() immediately
enters a readx_poll_timeout().
Since the code in ocelot-spi.c implementing those reads still insert
the padding bytes in the spi transfer, any read done after the reset
is effected, including the "final" read that should confirm the
self-clearing has happened, is most likely garbage:
Say we are using 1 padding byte; after the
reset is completed, the device doesn't wait for 8 cycles before
sending data (because CFGSTAT.IF_CFG has been reset to 0), so the
result is going to be something like 24 bits from the actual register,
shifted 8 bits, followed by 8 bits of whatever state the MISO line is
in when not being driven explicitly.
Now, some of the registers blocks (DEVCPU_QS and DEVCPU_ORG) are
documented to have access times of 0.1us, but the reset register is
located in DEVCPU_GCB, thus has access time 1us, so it seems there is
no way around doing the reads of the SOFT_RST register with the
correct value set in CFGSTAT.IF_CFG.
So rework the code to let the underlying bus define an ->init_bus
callback that the core can call after setting the soft reset bit, and
before polling that bit for being clear.
Note that we do have somewhat of a catch-22: We cannot read back the
REG_GCB_SOFT_RST register to know if reset is completed until
CFGSTAT.IF_CFG is reinitialized, but even if we are always allowed to
write to that register, if it is possible to write it "too soon",
before reset is completed, that re-initialization might be in
vain. The data sheet is unfortunately silent on how much time a soft
reset might take, and I simply assume that the re-initialization can
be done after 100us.
This also serves as preparation for implementing mdio management,
lifting some of the initialization steps from ocelot-spi.c to
ocelot-core.c.
Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
---
drivers/mfd/ocelot-core.c | 8 ++++++++
drivers/mfd/ocelot-spi.c | 9 +--------
drivers/mfd/ocelot.h | 2 ++
3 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
index 78b5fe15efdd2..9caab83138e59 100644
--- a/drivers/mfd/ocelot-core.c
+++ b/drivers/mfd/ocelot-core.c
@@ -110,6 +110,14 @@ int ocelot_chip_reset(struct device *dev)
if (ret)
return ret;
+ if (ddata->init_bus) {
+ fsleep(VSC7512_GCB_RST_SLEEP_US);
+ ret = ddata->init_bus(dev);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Error initializing bus after reset\n");
+ }
+
return readx_poll_timeout(ocelot_gcb_chip_rst_status, ddata, val, !val,
VSC7512_GCB_RST_SLEEP_US, VSC7512_GCB_RST_TIMEOUT_US);
}
diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c
index 97e4061e3dff7..37828dd3ee95e 100644
--- a/drivers/mfd/ocelot-spi.c
+++ b/drivers/mfd/ocelot-spi.c
@@ -215,6 +215,7 @@ static int ocelot_spi_probe(struct spi_device *spi)
return -ENOMEM;
spi_set_drvdata(spi, ddata);
+ ddata->init_bus = ocelot_spi_initialize;
ddata->init_regmap = ocelot_spi_init_regmap;
if (spi->max_speed_hz <= 500000) {
@@ -264,14 +265,6 @@ static int ocelot_spi_probe(struct spi_device *spi)
if (err)
return dev_err_probe(dev, err, "Error resetting device\n");
- /*
- * A chip reset will clear the SPI configuration, so it needs to be done
- * again before we can access any registers.
- */
- err = ocelot_spi_initialize(dev);
- if (err)
- return dev_err_probe(dev, err, "Error initializing SPI bus after reset\n");
-
err = ocelot_core_init(dev);
if (err)
return dev_err_probe(dev, err, "Error initializing Ocelot core\n");
diff --git a/drivers/mfd/ocelot.h b/drivers/mfd/ocelot.h
index 4f602611a5a86..5aa6589b9038e 100644
--- a/drivers/mfd/ocelot.h
+++ b/drivers/mfd/ocelot.h
@@ -12,6 +12,7 @@ struct resource;
/**
* struct ocelot_ddata - Private data for an external Ocelot chip
+ * @init_bus: Bus-specific initialization callback (optional).
* @init_regmap: Bus-specific callback for initializing regmap.
* @gcb_regmap: General Configuration Block regmap. Used for
* operations like chip reset.
@@ -25,6 +26,7 @@ struct resource;
* data of a SPI read operation.
*/
struct ocelot_ddata {
+ int (*init_bus)(struct device *dev);
struct regmap * (*init_regmap)(struct device *dev, const struct resource *res);
struct regmap *gcb_regmap;
struct regmap *cpuorg_regmap;
--
2.49.0Hi Rasmus, On Wed, Mar 19, 2025 at 01:30:53PM +0100, Rasmus Villemoes wrote: > As the comments in ocelot-spi.c explain, after a chip reset, the > CFGSTAT register must be written again setting the appropriate number > of padding bytes; otherwise reads are not reliable. > > However, the way the code is currently structured violates that: After > the BIT_SOFT_CHIP_RST is written, ocelot_chip_reset() immediately > enters a readx_poll_timeout(). I ran this new version and everything worked - and I've not seen an issue in previous versions. I'm looking for guidance as to whether this should include a Fixes tag and be backported. Great find, by the way! Is there any information you would like from my setup? I'm happy to add a reviewed tag, and my "tested" would be that I ran it. Colin Foster
Hi Colin On Wed, Mar 19 2025, Colin Foster <colin.foster@in-advantage.com> wrote: > On Wed, Mar 19, 2025 at 01:30:53PM +0100, Rasmus Villemoes wrote: >> As the comments in ocelot-spi.c explain, after a chip reset, the >> CFGSTAT register must be written again setting the appropriate number >> of padding bytes; otherwise reads are not reliable. >> >> However, the way the code is currently structured violates that: After >> the BIT_SOFT_CHIP_RST is written, ocelot_chip_reset() immediately >> enters a readx_poll_timeout(). > > I ran this new version and everything worked - and I've not seen an > issue in previous versions. I'm looking for guidance as to whether this > should include a Fixes tag and be backported. Thanks a lot for testing and reviewing! As for backporting, IDK, I think we'd at least first have to know that it really fixes a bug for somebody. > Great find, by the way! Is there any information you would like from my > setup? Certainly I'd like to know if you do in fact use a SPI clock > 500 kHz? And if so, could you try inserting a read and printk of e.g. CHIP_REGS.CHIP_ID immediately after the fsleep(), but before the re-initialization, just so we can see if my theory that the values are off-by-8-bits plus 8 bits of MISO "garbage" is correct? Because that register should have a fairly easily recognizable value. Thanks, Rasmus
On Thu, Mar 20, 2025 at 12:17:37PM +0100, Rasmus Villemoes wrote:
> Hi Colin
>
> On Wed, Mar 19 2025, Colin Foster <colin.foster@in-advantage.com> wrote:
>
> > On Wed, Mar 19, 2025 at 01:30:53PM +0100, Rasmus Villemoes wrote:
> >> As the comments in ocelot-spi.c explain, after a chip reset, the
> >> CFGSTAT register must be written again setting the appropriate number
> >> of padding bytes; otherwise reads are not reliable.
> >>
> >> However, the way the code is currently structured violates that: After
> >> the BIT_SOFT_CHIP_RST is written, ocelot_chip_reset() immediately
> >> enters a readx_poll_timeout().
> >
> > I ran this new version and everything worked - and I've not seen an
> > issue in previous versions. I'm looking for guidance as to whether this
> > should include a Fixes tag and be backported.
>
> Thanks a lot for testing and reviewing! As for backporting, IDK, I think
> we'd at least first have to know that it really fixes a bug for somebody.
>
> > Great find, by the way! Is there any information you would like from my
> > setup?
>
> Certainly I'd like to know if you do in fact use a SPI clock > 500 kHz?
Yep, looks like 2.5MHz
&spi0 {
#address-cells = <1>;
#size-cells = <0>;
status = "okay";
soc@0 {
compatible = "mscc,vsc7512";
spi-max-frequency = <2500000>;
>
> And if so, could you try inserting a read and printk of e.g. CHIP_REGS.CHIP_ID
> immediately after the fsleep(), but before the re-initialization, just
> so we can see if my theory that the values are off-by-8-bits plus 8 bits
> of MISO "garbage" is correct? Because that register should have a fairly
> easily recognizable value.
diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
index c00d30dbfca8..5a2762b6ecac 100644
--- a/drivers/mfd/ocelot-core.c
+++ b/drivers/mfd/ocelot-core.c
@@ -115,6 +115,8 @@ static int ocelot_chip_reset(struct device *dev)
if (ddata->init_bus) {
fsleep(VSC7512_GCB_RST_SLEEP_US);
+ regmap_read(ddata->gcb_regmap, 0, &val);
+ printk("7512 Chip ID after sleep: 0x%08x\n", val);
ret = ddata->init_bus(dev);
if (ret)
return dev_err_probe(dev, ret,
Prints out this:
[ 3.360986] 7512 Chip ID after sleep: 0xf0e94051
That doesn't seem right. I added a print after init and it makes more sense.
[ 3.351656] 7512 Chip ID after sleep: 0xf0e94051
[ 3.356828] 7512 Chip ID after init: 0x175140e9
That looks better. Good find!
Colin Foster
On Sat, Mar 22 2025, Colin Foster <colin.foster@in-advantage.com> wrote:
> On Thu, Mar 20, 2025 at 12:17:37PM +0100, Rasmus Villemoes wrote:
>> Hi Colin
>>
>> On Wed, Mar 19 2025, Colin Foster <colin.foster@in-advantage.com> wrote:
>>
>> > On Wed, Mar 19, 2025 at 01:30:53PM +0100, Rasmus Villemoes wrote:
>> >> As the comments in ocelot-spi.c explain, after a chip reset, the
>> >> CFGSTAT register must be written again setting the appropriate number
>> >> of padding bytes; otherwise reads are not reliable.
>> >>
>> >> However, the way the code is currently structured violates that: After
>> >> the BIT_SOFT_CHIP_RST is written, ocelot_chip_reset() immediately
>> >> enters a readx_poll_timeout().
>> >
>> > I ran this new version and everything worked - and I've not seen an
>> > issue in previous versions. I'm looking for guidance as to whether this
>> > should include a Fixes tag and be backported.
>>
>> Thanks a lot for testing and reviewing! As for backporting, IDK, I think
>> we'd at least first have to know that it really fixes a bug for somebody.
>>
>> > Great find, by the way! Is there any information you would like from my
>> > setup?
>>
>> Certainly I'd like to know if you do in fact use a SPI clock > 500 kHz?
>
> Yep, looks like 2.5MHz
>
> &spi0 {
> #address-cells = <1>;
> #size-cells = <0>;
> status = "okay";
>
> soc@0 {
> compatible = "mscc,vsc7512";
> spi-max-frequency = <2500000>;
>
>>
>> And if so, could you try inserting a read and printk of e.g. CHIP_REGS.CHIP_ID
>> immediately after the fsleep(), but before the re-initialization, just
>> so we can see if my theory that the values are off-by-8-bits plus 8 bits
>> of MISO "garbage" is correct? Because that register should have a fairly
>> easily recognizable value.
>
> diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
> index c00d30dbfca8..5a2762b6ecac 100644
> --- a/drivers/mfd/ocelot-core.c
> +++ b/drivers/mfd/ocelot-core.c
> @@ -115,6 +115,8 @@ static int ocelot_chip_reset(struct device *dev)
>
> if (ddata->init_bus) {
> fsleep(VSC7512_GCB_RST_SLEEP_US);
> + regmap_read(ddata->gcb_regmap, 0, &val);
> + printk("7512 Chip ID after sleep: 0x%08x\n", val);
> ret = ddata->init_bus(dev);
> if (ret)
> return dev_err_probe(dev, ret,
>
>
> Prints out this:
>
> [ 3.360986] 7512 Chip ID after sleep: 0xf0e94051
>
> That doesn't seem right. I added a print after init and it makes more sense.
>
> [ 3.351656] 7512 Chip ID after sleep: 0xf0e94051
> [ 3.356828] 7512 Chip ID after init: 0x175140e9
Thanks for testing. I hadn't realized that another thing the spi bus init
does is setting the endianness, but this clearly shows both the
off-by-one-byte and that the bytes are sent in the wrong order.
It's hard to know how you end up with that f0 garbage byte, I'd assume
either all-1s or all-0s when MISO is no longer driven explicitly. A wild
guess could be that it's leftover capacitance (the last actually-driven
bit is 1), which could explain why you haven't had a problem when
reading the reset register and expected all zeroes, because in that case
the device only sends 0s, and thus the garbage byte ends up also being a
0x00.
So yes, it does seem like this warrants a backport. I'll add a Fixes tag
for the next iteration, plus a link to this thread which demonstrates
the problem. I suppose this goes back to f3e89362.
Thanks,
Rasmus
© 2016 - 2025 Red Hat, Inc.