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

[Pratyush Yadav]

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...@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?

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

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.