Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <c57fe97b-ad4a-874e-663f-7f3a737824c9@microchip.com> <20190610062351.24405-1-tudor.ambarus@microchip.com>
In-Reply-To: <20190610062351.24405-1-tudor.ambarus@microchip.com>
From:   Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Tue, 11 Jun 2019 10:35:59 +0200
Message-ID: <CAMuHMdW3=fzFvt+ZmC2B6qf0zEwfvV--HVEoxa06Tk=a=Q1cWA@mail.gmail.com>
Subject: Re: [PATCH] mtd: spi-nor: use 16-bit WRR command when QE is set on
 spansion flashes
To:     Tudor Ambarus <Tudor.Ambarus@microchip.com>
Cc:     Marek Vasut <marek.vasut+renesas@gmail.com>,
        Marek Vasut <marek.vasut@gmail.com>,
        "R, Vignesh" <vigneshr@ti.com>, Jonas Bonn <jonas@norrbonn.se>,
        David Woodhouse <dwmw2@infradead.org>,
        Brian Norris <computersforpeace@gmail.com>,
        Miquel Raynal <miquel.raynal@bootlin.com>,
        Richard Weinberger <richard@nod.at>,
        MTD Maling List <linux-mtd@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux-Renesas <linux-renesas-soc@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Tudor,

On Mon, Jun 10, 2019 at 8:24 AM <Tudor.Ambarus@microchip.com> wrote:
> From: Tudor Ambarus <tudor.ambarus@microchip.com>
>
> 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@linux-m68k.org>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
> 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@glider.be>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@linux-m68k.org

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