Re: [PATCH 04/30] mtd: spi-nor: winbond: Make the RDCR fixup Winbond wide

["Michael Walle" <mwalle@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 4498 bytes --]

On Fri May 29, 2026 at 5:22 PM CEST, Miquel Raynal wrote:
> The top level paragraph of the QER field in the JESD216B mentions:
>
> "In this standard, [...]  Status register 2 refers to the byte read
> using instruction 35h. Status register 2 is the second byte transferred
> in a Write Status (01h) command. [...]"
>
> Value 100b, named in Linux BFPT_DWORD15_QER_SR2_BIT1_NO_RD, does not
> mention anything about reads and only brings details about writes.
>
> This has been interpreted in the spi-nor core by the absence of read
> capability, but there is no explicit reason for that, except that there
> were probably some very old chips which didn't support command 35h.

I agree. But making it a vendor default we always have to have an
entry. So if this is really for all winbond chips, we shall do a
catch all entry (like in macronix.c).

> All quad capable Winbond chips carry a CMP SWP bit in SR2. SR2 is
> readable with command 35h. In practice, all Winbond families but the
> W25X family have support for this feature, so re-enable it Winbond-wide
> in a late vendor fixup, except for the {EF, 30, xx} family.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

> ---
>  drivers/mtd/spi-nor/winbond.c | 37 ++++++++++++++-----------------------
>  1 file changed, 14 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
> index b4088fc5fde9..4300f0419f13 100644
> --- a/drivers/mtd/spi-nor/winbond.c
> +++ b/drivers/mtd/spi-nor/winbond.c
> @@ -73,26 +73,6 @@ static const struct spi_nor_fixups w25q256_fixups = {
>  	.post_bfpt = w25q256_post_bfpt_fixups,
>  };
>  
> -static int
> -winbond_rdcr_post_bfpt_fixup(struct spi_nor *nor,
> -			     const struct sfdp_parameter_header *bfpt_header,
> -			     const struct sfdp_bfpt *bfpt)
> -{
> -	/*
> -	 * W25H02NW, unlike its W25H512NW nor W25H01NW cousins, improperly sets
> -	 * the QE BFPT configuration bits, indicating a non readable CR. This is
> -	 * both incorrect and impractical, as the chip features a CMP bit for its
> -	 * locking scheme that lays in the Control Register, and needs to be read.
> -	 */
> -	nor->flags &= ~SNOR_F_NO_READ_CR;
> -
> -	return 0;
> -}
> -
> -static const struct spi_nor_fixups winbond_rdcr_fixup = {
> -	.post_bfpt = winbond_rdcr_post_bfpt_fixup,
> -};
> -
>  /**
>   * winbond_nor_select_die() - Set active die.
>   * @nor:	pointer to 'struct spi_nor'.
> @@ -305,7 +285,6 @@ static const struct flash_info winbond_nor_parts[] = {
>  		.id = SNOR_ID(0xef, 0x60, 0x21),
>  		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_TB_SR_BIT6 |
>  			 SPI_NOR_4BIT_BP | SPI_NOR_HAS_CMP,
> -		.fixups = &winbond_rdcr_fixup,
>  	}, {
>  		/* W25Q16JV-M */
>  		.id = SNOR_ID(0xef, 0x70, 0x15),
> @@ -381,7 +360,6 @@ static const struct flash_info winbond_nor_parts[] = {
>  		.id = SNOR_ID(0xef, 0x80, 0x22),
>  		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_TB_SR_BIT6 |
>  			 SPI_NOR_4BIT_BP | SPI_NOR_HAS_CMP,
> -		.fixups = &winbond_rdcr_fixup,
>  	}, {
>  		/* W25H512NW-M */
>  		.id = SNOR_ID(0xef, 0xa0, 0x20),
> @@ -397,7 +375,6 @@ static const struct flash_info winbond_nor_parts[] = {
>  		.id = SNOR_ID(0xef, 0xa0, 0x22),
>  		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_TB_SR_BIT6 |
>  			 SPI_NOR_4BIT_BP | SPI_NOR_HAS_CMP,
> -		.fixups = &winbond_rdcr_fixup,
>  	},
>  };
>  
> @@ -490,6 +467,20 @@ static int winbond_nor_late_init(struct spi_nor *nor)
>  	 */
>  	params->set_4byte_addr_mode = winbond_nor_set_4byte_addr_mode;
>  
> +	/*
> +	 * All W25Q/W25H chips do set the BFPT_DWORD15_QER_SR2_BIT1_NO_RD bit in
> +	 * their SFDP tables. The historical spi-nor assumption in this case has
> +	 * been to declare CR reads as unsupported, whereas the Jedec
> +	 * specification doesn't clearly state that. In practice, all these
> +	 * chips do support reading back the CR, which is needed for SWP support,
> +	 * so make sure that capability remains enabled (needed for SWP).
> +	 * In practice, only exclude the old W25X family (JEDEC ID: EF 30 xx)
> +	 * which actually does not support this feature.
> +	 */
> +	if (nor->info->id->bytes[0] == 0xef &&
> +	    nor->info->id->bytes[1] > 0x30)
> +		nor->flags &= ~SNOR_F_NO_READ_CR;

This is somewhat hard to read and the byte[0] is redundant here.
Maybe you can come up with some macro magic sugar :)

-michael

> +
>  	return 0;
>  }
>  


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

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