[RFC PATCH] mtd: spi-nor-core: Handle SPI_RX_SLOW in spi_nor_adjust_hwcaps()

[Bin Meng]

On Thu, Jul 29, 2021 at 1:42 AM Pratyush Yadav <p.yadav at ti.com> wrote:
>
> On 28/07/21 11:56PM, Bin Meng wrote:
> > When CONFIG_SPI_FLASH_SMART_HWCAPS is on, SPI_RX_SLOW flag of the
> > SPI controller is not honored. This adds the missing logic there.
> >
> > With this patch, SPI flash read works again with ICH SPI controller
> > on Intel Crown Bay board.
> >
> > Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
> >
> > ---
> > The quirky stuff of ICH SPI controller is that it only supports normal
> > read (opcode 3) but not fast read (opcode 11), so that's why SPI_RX_SLOW
> > was introduced before.
> >
> > At present the ICH SPI driver does not implement the supports_op() hook.
> > I can add a check against opcode 11 there, however I see a comment in
> > spi_nor_check_op() saying that SPI controller implementation should not
> > check the opcode.
> >
> > /*
> >  * First test with 4 address bytes. The opcode itself might be a 3B
> >  * addressing opcode but we don't care, because SPI controller
> >  * implementation should not check the opcode, but just the sequence.
> >  */
> >
> >  drivers/mtd/spi/spi-nor-core.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> > index 99e2f16349..e9b46c39b5 100644
> > --- a/drivers/mtd/spi/spi-nor-core.c
> > +++ b/drivers/mtd/spi/spi-nor-core.c
> > @@ -2855,6 +2855,7 @@ spi_nor_adjust_hwcaps(struct spi_nor *nor,
> >                     const struct spi_nor_flash_parameter *params,
> >                     u32 *hwcaps)
> >  {
> > +     struct spi_slave *spi = nor->spi;
> >       unsigned int cap;
> >
> >       /*
> > @@ -2879,6 +2880,10 @@ spi_nor_adjust_hwcaps(struct spi_nor *nor,
> >               if (!(*hwcaps & BIT(cap)))
> >                       continue;
> >
> > +             if ((spi->mode & SPI_RX_SLOW) &&
> > +                 (BIT(cap) == SNOR_HWCAPS_READ_FAST))
> > +                     *hwcaps &= ~BIT(cap);
> > +
>
> NACK.
>
> We already check for SPI_RX_SLOW in spi_nor_scan():
>
>   /* Some devices cannot do fast-read, no matter what DT tells us */
>   if ((info->flags & SPI_NOR_NO_FR) || (spi->mode & SPI_RX_SLOW))
>         params.hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;
>
> I think the real bug here is that the smart spi_nor_adjust_hwcaps() does
> not respect the flash's hwcaps, and only looks to the controller on what
> can be supported. Doing the below should fix this:
>
>   diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>   index 8dd44c0f1e..4323b49651 100644
>   --- a/drivers/mtd/spi/spi-nor-core.c
>   +++ b/drivers/mtd/spi/spi-nor-core.c
>   @@ -2741,7 +2741,7 @@ spi_nor_adjust_hwcaps(struct spi_nor *nor,
>          * Enable all caps by default. We will mask them after checking what's
>          * really supported using spi_mem_supports_op().
>          */
>   -     *hwcaps = SNOR_HWCAPS_ALL;
>   +     *hwcaps = SNOR_HWCAPS_ALL & params->hwcaps.mask;
>
>         /* X-X-X modes are not supported yet, mask them all. */
>         *hwcaps &= ~SNOR_HWCAPS_X_X_X;
>
> Can you please test and confirm if it does indeed fix the issue for you?

Yes, this also fixed the issue.

In fact, we should update spi-nor to honor the DT property
"m25p,fast-read" instead of the SPI_RX_SLOW flag which was set by the
ICH SPI driver.

I will prepare patches soon.

>
> >               rdidx = spi_nor_hwcaps_read2cmd(BIT(cap));
> >               if (rdidx >= 0 &&
> >                   spi_nor_check_readop(nor, &params->reads[rdidx]))

Regards,
Bin