Re: [PATCH v2] mtd: spi-nor: clear the default RDCR opcode when entering Octal DTR

[Bough Chen <haibo.chen@oss.nxp.com>]

On Tue, Sep 29, 2026 at 09:10:37AM +0000, Bough Chen (OSS) wrote:
> > -----Original Message-----
> > From: Miquel Raynal <miquel.raynal@bootlin.com>
> > Sent: Tuesday, September 29, 2026 4:23 PM
> > To: Bough Chen (OSS) <haibo.chen@oss.nxp.com>
> > Cc: Pratyush Yadav <pratyush@kernel.org>; Michael Walle
> > <mwalle@kernel.org>; Takahiro Kuwano <takahiro.kuwano@infineon.com>;
> > Richard Weinberger <richard@nod.at>; Vignesh Raghavendra
> > <vigneshr@ti.com>; linux-mtd@lists.infradead.org; linux-
> > kernel@vger.kernel.org; michael@walle.cc; Bough Chen
> > <haibo.chen@nxp.com>
> > Subject: Re: [PATCH v2] mtd: spi-nor: clear the default RDCR opcode when
> > entering Octal DTR
> > 
> > On 29/09/2026 at 14:53:07 +08, haibo.chen@oss.nxp.com wrote:
> > 
> > > From: Haibo Chen <haibo.chen@nxp.com>
> > >
> > > The core defaults opcodes.read_sr2 to the legacy RDCR opcode (0x35).
> > > In 8D-8D-8D mode the opcode is extended to two bytes per cmd_ext_type,
> > > but the resulting command is not a valid SR2 read for these flashes,
> > > which access their status/config registers through a vendor-specific
> > > indirect register space. Since spi_nor_cache_sr_lock_bits() now reads
> > > SR2 during init, i.e. after the switch to Octal DTR, the extended RDCR
> > > gets no data back and the read times out (on i.MX FlexSPI: -ETIMEDOUT
> > > and a controller
> > > WARN() during probe with Micron MT35xU and Macronix octal parts).
> > >
> > > Clear read_sr2 when the switch to Octal DTR actually succeeds, so the
> > > SR2 read is skipped (a cleared opcode already means "unsupported").
> > > Doing it in spi_nor_set_octal_dtr() keys off the real runtime
> > > protocol: a flash that advertises Octal DTR but runs in (x)STR because
> > > the host lacks support keeps its usable RDCR.
> > >
> > > Assisted-by: LLM
> > > Fixes: b7b63475903c ("mtd: spi-nor: Create a local SR cache")
> > > Fixes: 63489002d397 ("mtd: spi-nor: Refactor Read Status/Write Status
> > > support")
> > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > > ---
> > > Changes in v2:
> > > - Rework the fix following review: instead of guarding the read at runtime
> > >   in spi_nor_read_sr2(), clear the default RDCR opcode.read_sr2 once, at the
> > >   point the switch to Octal DTR succeeds (spi_nor_set_octal_dtr()).
> > 
> > I don't get that choice. Why switching when Octal DTR succeeds only? SR2 is
> > either supported or not supported, I don't think it is anyway different when
> > entering octal DTR mode, is it? So I would expect SR2 to be cleared earlier than
> > that, once we know the chip is octal DTR capable. And this must be early
> > enough so that manufacturer drivers can still set their own value.
> > 
> > I am wondering whether we should simply drop sr2 opcode in the QER SFDP
> > parsing entirely for octal DTR devices (Michael?).
> 
> The reason I clear it at the point Octal DTR actually succeeds rather than
>  "once the chip is octal-DTR-capable" is that capability != running mode: 
> a chip can advertise Octal DTR while still running in (x)STR because the host 
> controller does not support 8D (see reply below). In that (x)STR case RDCR is
>  valid and needed, so keying off capability alone would wrongly drop a usable
>  SR2 read.
> 
> That said, I agree the current spot is awkward and does not let manufacturer 
> drivers override the value. I'm happy to move it earlier. Two options:
> 1, Clear it during SFDP QER parsing (as you suggest) for octal-DTR-capable devices.
>      This is the natural place next to the existing read_sr2 = 0 QER cases, and it runs 
>       before the manufacturer late_init/fixups, so a driver can still install its own SR2
>       opcode afterwards. 
>      My only concern is the capability-vs-running-mode point above — we'd be 
>      clearing based on advertised 8D capability, not on whether 8D is actually entered.
> 
> 2, Clear it in spi_nor_late_init_params() after the manufacturer hooks, gated on 
>      octal-DTR capability, so vendor values set in their late_init are preserved.
> 
> If the capability-based approach is acceptable (i.e. we accept that a chip advertising
>  8D but forced to STR by the host loses its RDCR), I'll go with dropping it in the QER SFDP
>  parsing. Michael, do you have a preference?

Hi Miquel,

Any suggestion?

Regards
Haibo Chen

> > 
> > 
> > > - Key the clear off the real runtime protocol so a flash that advertises
> > >   Octal DTR but runs in (x)STR (host without 8D support) keeps its
> > >   RDCR.
> > 
> > Are you sure this is a valid case?
> 
> Yes. spi_nor_setup() computes shared_mask = hwcaps->mask & params->hwcaps.mask, 
> i.e. the intersection of flash and controller capabilities. If the host does not support 8D, 
> SNOR_HWCAPS_READ_8_8_8_DTR is not in shared_mask, spi_nor_select_read() picks a 
> non-8D read_proto/write_proto, and spi_nor_set_octal_dtr() bails out early at:
> 
> if (!(nor->read_proto == SNOR_PROTO_8_8_8_DTR &&
>       nor->write_proto == SNOR_PROTO_8_8_8_DTR))
> 	return 0;
> 
> So the chip stays in (x)STR with reg_proto == SNOR_PROTO_1_1_1, and RDCR is still a valid 
> single-byte SR2 read there. That is the case I wanted to avoid breaking by keying the clear
>  off the negotiated runtime protocol rather than the advertised capability. If we move the 
> clear to capability-based SFDP/late_init as discussed, this is exactly the scenario we need to 
> be comfortable regressing (an 8D-capable part on a non-8D host would lose its RDCR read of SR2).
> 
> Regards
> Haibo Chen
> 
> > 
> > Thanks,
> > Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/