Re: [PATCH v1] mtd: spi-nor: unset quad_enable if SFDP doesn't specify it

[Tudor.Ambarus]

On 3/4/22 20:51, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> While the first version of JESD216 specify the opcode for 4 bit I/O
> accesses, it lacks information on how to actually enable this mode.
> 
> For now, the one set in spi_nor_init_default_params() will be used.
> But this one is likely wrong for some flashes, in particular the
> Macronix MX25L12835F. Thus we need to clear the enable method when
> parsing the SFDP. Flashes with such an SFDP revision will have to use a
> flash (and SFDP revision) specific fixup.
> 
> This might break quad I/O for some flashes which relied on the
> spi_nor_sr2_bit1_quad_enable() that was formerly set. If your bisect
> turns up this commit, you'll probably have to set the proper
> quad_enable method in a post_bfpt() fixup for your flash.
> 

Right, I meant adding a paragraph such as the one from above.

> Signed-off-by: Michael Walle <michael@xxxxxxxx>
> Tested-by: Heiko Thiery <heiko.thiery@xxxxxxxxx>
> ---
> changes since RFC:
>  - reworded commit message
>  - added comment about post_bfpt hook
> 
> Tudor, I'm not sure what you meant with
>   Maybe you can update the commit message and explain why would some
>   flashes fail to enable quad mode, similar to what I did.
> 
> It doesn't work because the wrong method is chosen? ;)
> 
>  drivers/mtd/spi-nor/sfdp.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index a5211543d30d..6bba9b601846 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -549,6 +549,16 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>         map->uniform_erase_type = map->uniform_region.offset &
>                                   SNOR_ERASE_TYPE_MASK;
> 
> +       /*
> +        * The first JESD216 revision doesn't specify a method to enable
> +        * quad mode. spi_nor_init_default_params() will set a legacy
> +        * default method to enable quad mode. We have to disable it
> +        * again.
> +        * Flashes with this JESD216 revision need to set the quad_enable
> +        * method in their post_bfpt() fixup if they want to use quad I/O.
> +        */

Great. Looks good to me. I'll change the subject to "mtd: spi-nor: sfdp:"
when applying.

Cheers,
ta
> +       params->quad_enable = NULL;
> +
>         /* Stop here if not JESD216 rev A or later. */
>         if (bfpt_header->length == BFPT_DWORD_MAX_JESD216)
>                 return spi_nor_post_bfpt_fixups(nor, bfpt_header, &bfpt);
> @@ -562,7 +572,6 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>         /* Quad Enable Requirements. */
>         switch (bfpt.dwords[BFPT_DWORD(15)] & BFPT_DWORD15_QER_MASK) {
>         case BFPT_DWORD15_QER_NONE:
> -               params->quad_enable = NULL;
>                 break;
> 
>         case BFPT_DWORD15_QER_SR2_BIT1_BUGGY:
> --
> 2.30.2
>