Re: [PATCH] mtd: spi-nor: use 16-bit WRR command when QE is set on spansion flashes

[Geert Uytterhoeven]

Hi Tudor,

On Mon, Jun 10, 2019 at 8:24 AM <Tudor.Ambarus@xxxxxxxxxxxxx> wrote:
> From: Tudor Ambarus <tudor.ambarus@xxxxxxxxxxxxx>
>
> SPI memory devices from different manufacturers have widely
> different configurations for Status, Control and Configuration
> registers. JEDEC 216C defines a new map for these common register
> bits and their functions, and describes how the individual bits may
> be accessed for a specific device. For the JEDEC 216B compliant
> flashes, we can partially deduce Status and Configuration registers
> functions by inspecting the 16th DWORD of BFPT. Older flashes that
> don't declare the SFDP tables (SPANSION FL512SAIFG1 311QQ063 A Â11
> SPANSION) let the software decide how to interact with these registers.
>
> The commit dcb4b22eeaf4 ("spi-nor: s25fl512s supports region locking")
> uncovered a probe error for s25fl512s, when the QUAD bit CR[1] was set
> in the bootloader. When this bit is set, only the Write Register
> WRR command format with 16 data bits may be used, WRR with 8 bits
> is not recognized and hence the error when trying to clear the block
> protection bits.
>
> Fix the above by using 16-bits WRR command when Quad bit is set.
>
> Backward compatibility should be fine. The newly introduced
> spi_nor_spansion_clear_sr_bp() is tightly coupled with the
> spansion_quad_enable() function. Both assume that the Write Register
> with 16 bits, together with the Read Configuration Register (35h)
> instructions are supported.
>
> Reported-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@xxxxxxxxxxxxx>
> ---
> Geert, Jonas,
>
> This patch is compile-tested only. I don't have the flash, I need your
> help for testing this.

Thanks, this revives access to the s25fl512s on Koelsch.

Fixes: dcb4b22eeaf44f91 ("spi-nor: s25fl512s supports region locking")
Tested-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>Hi Tudor,

Two questions below...

> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c

> +static int spi_nor_spansion_clear_sr_bp(struct spi_nor *nor)
> +{

[...]

> +        * When the configuration register QUAD bit CR[1] is 1, only
> +        * the WRR command format with 16 data bits may be used.

s/WRR/WRSR/?

> +        */
> +       if (ret & CR_QUAD_EN_SPAN) {
> +               sr_cr[1] = ret;
> +
> +               ret = read_sr(nor);
> +               if (ret < 0) {
> +                       dev_err(nor->dev,
> +                               "error while reading status register\n");
> +                       return ret;
> +               }
> +               sr_cr[0] = ret & ~mask;
> +
> +               ret = write_sr_cr(nor, sr_cr);
> +               if (ret)
> +                       dev_err(nor->dev, "16-bit write register failed\n");
> +               return ret;
> +       }
> +
> +       /* If quad bit is not set, use 8-bit WRR command. */

Likewise.

> +       return spi_nor_clear_sr_bp(nor);
> +}

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds