spi-nor: maxronix MX25L12835F support

[Heiko Thiery]

Hi Tudor and all,

Am Di., 16. Feb. 2021 um 12:15 Uhr schrieb <Tudor.Ambarus at microchip.com>:
>
> Hi, all,
>
> +zhengxunli, juliensu & ycllin
>
> On 2/16/21 11:48 AM, Michael Walle wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > Am 2021-02-16 10:27, schrieb Pratyush Yadav:
> >> On 15/02/21 10:53PM, Heiko Thiery wrote:
> >>> Hi all,
> >>>
> >>> I faced an issue with a SPI flash on our board. We use a macronix
> >>> MX25L12835F [1]. Unfortunately this flash has the same JEDEC ID like
> >>> the MX25L12805D [2].
> >>>
> >>> The newer MX25L12835F has support for dual/quad read mode and RDSFDP
> >>> while the older doesn't.
> >>>
> >>> I thought that I could do a fixup with a device specific
> >>> post_bfpt_fixups() call but by now this seems not possible. The older
> >>> MX25L12805D has no flags set that allows a call to
> >>> spi_nor_sfdp_init_params() and implements the fixup.
> >>>
> >>> Has anyone an idea how to solve this?
>
> Maybe macronix can help with some suggestions on how to differentiate
> between flashes at runtime.
>
> My first thought is to introduce a SPI_NOR_HAS_SFDP flag. For the flash
> that doesn't support SFDP tables, there should be no functional change,
> for the one that support SFDP it should fill the properties from the
> SFDP tables.
>
> >>
> >> The post_sfdp fixup is always run regardless of whether the flash has
> >> SFDP or not. You can try putting your flash-specific fixups there.
> >
> > Well the problem here is, that the SFDP setup is skipped though the
> > flash would support SFDP. If the jedec id wasn't already in the table,
> > there would be the flag SPI_NOR_QUAD_READ and the SFDP would be
> > parsed. But because there is already the legacy device (which likely
> > doesn't support SFDP) it really doesn't fit.
> >
> > Its unclear to me, why the SFDP is only parsed if one of the
> > SPI_NOR_*_READ flags are set.
>
> My guess is that a new SFDP flag was not necessary. SFDP defines multiple
> tables, but there is just one that is mandatory, BFPT. BFPT defines DUAL
> and QUAD parameters. From the spi-nor code, a BFPT without DUAL or QUAD
> support doesn't make sense, even though DUAL or QUAD are not mandatory
> in BFPT as I see in the standard. So probably it was just a way to avoid
> adding a extra flag. We have to check the git history for a more accurate
> description, this was just a guess.
>
> Thinking loud, now we do a static initialization of flash params, that
> can be overwritten dynamically by SFDP. How about doing the params init
> the other way around. Try first to dynamically discover the params via
> SFDP, and if SFDP fails or if it is not defined, do the static init via
> flags. That would spare some code. And new flash IDs will have less flags
> declared, and we'll better track faulty SFDP flashes.

I am a newbie but it sounds reasonable. I made a first attempt and
reversed the order. But this is not enough. After a few debug cycles I
found a few settings that are probably not automatically set by the
SFDP detection and were set in spi_nor_info_init_params() before.


I was able to find the following lines as the cause.


---- 8< ----
 /**
  * spi_nor_info_init_params() - Initialize the flash's parameters and settings
@@ -3094,14 +3095,18 @@ static int spi_nor_init_params(struct spi_nor *nor)
        if (!nor->params)
                return -ENOMEM;

-       spi_nor_info_init_params(nor);
+       nor->params->setup = spi_nor_default_setup;
+       nor->params->writesize = 1;

-       spi_nor_manufacturer_init_params(nor);
+       /* Page Program settings. */
+       nor->params->hwcaps.mask |= SNOR_HWCAPS_PP;
+       spi_nor_set_pp_settings(&nor->params->page_programs[SNOR_CMD_PP],
+                               SPINOR_OP_PP, SNOR_PROTO_1_1_1);
+
+       if (spi_nor_parse_sfdp(nor, nor->params))
+               spi_nor_info_init_params(nor);

-       if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
-                                SPI_NOR_OCTAL_READ |
SPI_NOR_OCTAL_DTR_READ)) &&
-           !(nor->info->flags & SPI_NOR_SKIP_SFDP))
-               spi_nor_sfdp_init_params(nor);
+       spi_nor_manufacturer_init_params(nor);

        spi_nor_post_sfdp_fixups(nor);
---- 8< ----

Now I am trying to understand if this makes sense or what is going
wrong. Does anyone here have a hint at which point this would be
correct or if it should also be detected by the SFDP?


--
Heiko