On Thu, Jul 29, 2021 at 1:42 AM Pratyush Yadav p.yadav@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@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