Re: [PATCH 3/8] mfd: ocelot: rework SPI (re-)initialization after chip reset

[Colin Foster <colin.foster@in-advantage.com>]

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