Re: [PATCH v3] mtd: spi-nor: clear the default read_sr2 opcode for xSPI Profile 1.0 flashes

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hello,

On 22/10/2026 at 08:56:03 GMT, "Bough Chen (OSS)" <haibo.chen@oss.nxp.com> wrote:

>> -----Original Message-----
>> From: sashiko-bot@kernel.org <sashiko-bot@kernel.org>
>> Sent: Thursday, October 22, 2026 4:21 PM
>> To: Bough Chen (OSS) <haibo.chen@oss.nxp.com>; Pratyush Yadav
>> <pratyush@kernel.org>; Michael Walle <mwalle@kernel.org>; Takahiro
>> Kuwano <takahiro.kuwano@infineon.com>; Miquel Raynal
>> <miquel.raynal@bootlin.com>; Richard Weinberger <richard@nod.at>;
>> Vignesh Raghavendra <vigneshr@ti.com>
>> Cc: linux-mtd@lists.infradead.org; imx@lists.linux.dev; michael@walle.cc;
>> Bough Chen <haibo.chen@nxp.com>; Frank.Li@kernel.org
>> Subject: Re: [PATCH v3] mtd: spi-nor: clear the default read_sr2 opcode for
>> xSPI Profile 1.0 flashes
>> 
>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
>> to consider:
>> - [Medium] mtd: spi-nor: read_sr2 left set for 8D-8D-8D flashes without
>> Profile 1.0
>> --
>> 
>> commit ef4079e3aea2014890e08df31ea4d8775a392f81
>> Author: Haibo Chen <haibo.chen@nxp.com>
>> 
>> mtd: spi-nor: clear the default read_sr2 opcode for xSPI Profile 1.0 flashes
>> 
>> This commit clears params->opcodes.read_sr2 in spi_nor_parse_profile1() so
>> that flashes with an xspi profile 1.0 table do not issue an unsupported
>> 0x35 register read when running in octal dtr mode.
>> 
>> > diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>> > index 4a3f82e97cef8..722eeff71d0df 100644
>> > --- a/drivers/mtd/spi-nor/sfdp.c
>> > +++ b/drivers/mtd/spi-nor/sfdp.c
>> > @@ -1344,6 +1344,17 @@ static int spi_nor_parse_profile1(struct spi_nor
>> *nor,
>> >  	 */
>> >  	nor->params->hwcaps.mask |= SNOR_HWCAPS_PP_8_8_8_DTR;
>> >
>> > +	/*
>> > +	 * The xSPI Profile 1.0 table does not describe a Read Configuration
>> > +	 * Register (RDCR, 35h) instruction, and the single-byte RDCR default
>> > +	 * is not usable in 8D-8D-8D mode. Drop the SR2 read opcode for
>> these
>> > +	 * octal-capable flashes so spi_nor_read_sr1_and_sr2() degrades to
>> an
>> > +	 * SR1-only read and spi_nor_init() does not issue an unsupported
>> > +	 * command that the flash never answers. A manufacturer driver that
>> > +	 * actually needs SR2 can install its own opcode in a later fixup.
>> > +	 */
>> > +	nor->params->opcodes.read_sr2 = 0;
>> 
>> [Severity: Medium]
>> What happens on octal dtr flashes that do not publish an xspi profile 1.0
>> table, so spi_nor_parse_profile1() is never called?
>> 
>> Specifically, some batches of the Micron mt35xu512aba lack the profile 1.0
>> table and instead configure SNOR_PROTO_8_8_8_DTR in
>> drivers/mtd/spi-nor/micron-st.c:mt35xu512aba_post_sfdp_fixup():
>> 
>> 	nor->params->hwcaps.mask |= SNOR_HWCAPS_READ_8_8_8_DTR;
>> 	spi_nor_set_read_settings(&nor->params-
>> >reads[SNOR_CMD_READ_8_8_8_DTR],
>> 				  0, 20, SPINOR_OP_MT_DTR_RD,
>> 				  SNOR_PROTO_8_8_8_DTR);
>> 	...
>> 	nor->params->hwcaps.mask |= SNOR_HWCAPS_PP_8_8_8_DTR;
>> 	spi_nor_set_pp_settings(&nor->params-
>> >page_programs[SNOR_CMD_PP_8_8_8_DTR],
>> 				SPINOR_OP_PP_4B,
>> SNOR_PROTO_8_8_8_DTR);
>> 
>> In drivers/mtd/spi-nor/sfdp.c:spi_nor_parse_bfpt(), params-
>> >opcodes.read_sr2 is initialized to SPINOR_OP_RDCR (0x35) and remains set
>> when the quad enable requirements field contains a reserved value (which
>> mt35xu512aba_post_sfdp_fixup() notes is the case on this part) or
>> BFPT_DWORD15_QER_NONE:
>> 
>> 	params->opcodes.read_sr1 = SPINOR_OP_RDSR;
>> 	params->opcodes.read_sr2 = SPINOR_OP_RDCR;
>> 	...
>> 	default:
>> 		dev_dbg(nor->dev, "BFPT QER reserved value used\n");
>> 		break;
>> 
>> Similarly, non-sfdp flashes initialized with SPI_NOR_OCTAL_DTR_READ in
>> drivers/mtd/spi-nor/core.c:spi_nor_no_sfdp_init_params() enable
>> SNOR_HWCAPS_READ_8_8_8_DTR while leaving params->opcodes.read_sr2
>> set to SPINOR_OP_RDCR from spi_nor_init_default_params().
>> 
>> When spi_nor_init() switches these flashes to SNOR_PROTO_8_8_8_DTR via
>> spi_nor_set_octal_dtr() and then runs:
>> 
>>   spi_nor_init()
>>     -> spi_nor_cache_sr_lock_bits()
>>       -> spi_nor_read_sr1_and_sr2_careful()
>>         -> spi_nor_read_sr2_careful()
>>           -> spi_nor_read_sr_ll()
>> 
>> wouldn't this still issue the unsupported 0x3535 register read in
>> SNOR_PROTO_8_8_8_DTR mode and fail probe or resume with a controller
>> timeout?
>
> Hi Miquel,
>
> The Sashiko AI review raised a valid point on v3.

Is it a valid point? Only Winbond flashes have something in SR2. It is
mentioning non SFDP compliant Micron devices. So this case never
actually happens.

> I see three ways to go, which do you prefer?
>
> 1, Keep the v3 approach (clear in spi_nor_parse_profile1()) and fix the remaining
>     cases one by one, clearing read_sr2 in each relevant fixup 
>     (e.g. mt35xu512aba_post_sfdp_fixup) / no-SFDP path.

This is my favorite option, as you can guess.

> 2, Move the clear into spi_nor_late_init_params(), gated on the 8D read capability
>     (hwcaps & SNOR_HWCAPS_READ_8_8_8_DTR) and read_sr2 == RDCR, so all 8D 
>     paths are covered in one place. 
>     The downside: late_init runs after the manufacturer fixups, so if a vendor later
>     wants to set its own SR2 opcode for 8D it would be clobbered here and overriding
>      it cleanly becomes awkward.
>
> 3, Go back to the v2 approach (clear in spi_nor_set_octal_dtr() when the switch to 8D
>     actually succeeds). This covers every path that ends up in 8D regardless of how the 
>     capability was discovered, but it is runtime-protocol based rather than capability 
>     based, which you didn't prefer earlier. And this method has the same downside as
>     method 2.

Thanks,
Miquèl