On Thu, Mar 20, 2025 at 12:17:37PM +0100, Rasmus Villemoes wrote:
> Hi Colin
>
> On Wed, Mar 19 2025, Colin Foster <colin.foster@xxxxxxxxxxxxxxxx> 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
