[U-Boot] [PATCH 1/2] spi: cadence_qspi: Move to spi-mem framework

[Simon Goldschmidt]

On Thu, Oct 17, 2019 at 3:48 PM <Tudor.Ambarus at microchip.com> wrote:
>
> Hi, Simon, Vignesh,
>
> On 10/17/2019 02:20 PM, Simon Goldschmidt wrote:
> > On Mon, Oct 14, 2019 at 3:27 PM Vignesh Raghavendra <vigneshr at ti.com> wrote:
> >> Current Cadence QSPI driver has few limitations. It assumes all read
> >> operations to be in Quad mode and thus does not support SFDP parsing.
> >> Also, adding support for new mode such as Octal mode would not be
> >> possible with current configuration. Therefore move the driver over to spi-mem
> >> framework. This has added advantage that driver can be used to support
> >> SPI NAND memories too.
> >> Hence, move driver over to new spi-mem APIs.
> >>
> >> Please note that this gets rid of mode bit setting done when
> >> CONFIG_SPL_SPI_XIP is defined as there does not seem to be any user to
> >> that config option.
> > I just have tried this on an socfgpa cylone5 board with an mt25ql256a, but
> > it does not seem to work: when leaving spi-rx-bus-width and spi-tx-bus-width
> > at 4 in my devicetree, SFDP parsing works, but reading data afterwards
> > produces invalid results (I haven't tested what's wrong there).
> >
> > It works as expected when not parsing SFDP or setting the bus-width to 1.
> > So the change itself probably works, but SFDP parsing is broken?
>
> This can happen if the quad enable method is not correctly set/called. Would you
> try the patch form below?

I tried, but of course that didn't help. And I fail to see why it would, unless
you do something wrong in the flash device manufacturer selection.

Doing the patch like below would only make sense if enabling SFDP parsing would
at the same time completely remove the manufacturer selection (e.g. manufacturer
selection in Kconfig would depend on SFDP not being set).

Regards,
Simon

>
> I don't see much benefit in having those guards, especially that we have
> SPI_FLASH_SFDP_SUPPORT defined - it trims most of the SFDP logic.
>
> More, these #ifdef guards are not scalable and with the addition of flashes that
> support SFDP the #ifdefs will look uglier and uglier.
>
> Cheers,
> ta
>
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index e5b9899c64b2..3002f97a7342 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -188,7 +188,6 @@ static int read_fsr(struct spi_nor *nor)
>   * location. Return the configuration register value.
>   * Returns negative if error occurred.
>   */
> -#if defined(CONFIG_SPI_FLASH_SPANSION) || defined(CONFIG_SPI_FLASH_WINBOND)
>  static int read_cr(struct spi_nor *nor)
>  {
>         int ret;
> @@ -202,7 +201,6 @@ static int read_cr(struct spi_nor *nor)
>
>         return val;
>  }
> -#endif
>
>  /*
>   * Write status register 1 byte
> @@ -591,7 +589,6 @@ erase_err:
>         return ret;
>  }
>
> -#if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST)
>  /* Write status register and ensure bits in mask match written values */
>  static int write_sr_and_check(struct spi_nor *nor, u8 status_new, u8 mask)
>  {
> @@ -877,7 +874,6 @@ static int stm_is_locked(struct spi_nor *nor, loff_t ofs,
> uint64_t len)
>
>         return stm_is_locked_sr(nor, ofs, len, status);
>  }
> -#endif /* CONFIG_SPI_FLASH_STMICRO */
>
>  static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
>  {
> @@ -1116,7 +1112,6 @@ write_err:
>         return ret;
>  }
>
> -#ifdef CONFIG_SPI_FLASH_MACRONIX
>  /**
>   * macronix_quad_enable() - set QE bit in Status Register.
>   * @nor:       pointer to a 'struct spi_nor'
> @@ -1153,9 +1148,7 @@ static int macronix_quad_enable(struct spi_nor *nor)
>
>         return 0;
>  }
> -#endif
>
> -#if defined(CONFIG_SPI_FLASH_SPANSION) || defined(CONFIG_SPI_FLASH_WINBOND)
>  /*
>   * Write status Register and configuration register with 2 bytes
>   * The first byte will be written to the status register, while the
> @@ -1269,7 +1262,6 @@ static int spansion_no_read_cr_quad_enable(struct spi_nor
> *nor)
>  }
>
>  #endif /* CONFIG_SPI_FLASH_SFDP_SUPPORT */
> -#endif /* CONFIG_SPI_FLASH_SPANSION */
>
>  struct spi_nor_read_command {
>         u8                      num_mode_clocks;
> @@ -1787,22 +1779,16 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>         case BFPT_DWORD15_QER_NONE:
>                 params->quad_enable = NULL;
>                 break;
> -#if defined(CONFIG_SPI_FLASH_SPANSION) || defined(CONFIG_SPI_FLASH_WINBOND)
>         case BFPT_DWORD15_QER_SR2_BIT1_BUGGY:
>         case BFPT_DWORD15_QER_SR2_BIT1_NO_RD:
>                 params->quad_enable = spansion_no_read_cr_quad_enable;
>                 break;
> -#endif
> -#ifdef CONFIG_SPI_FLASH_MACRONIX
>         case BFPT_DWORD15_QER_SR1_BIT6:
>                 params->quad_enable = macronix_quad_enable;
>                 break;
> -#endif
> -#if defined(CONFIG_SPI_FLASH_SPANSION) || defined(CONFIG_SPI_FLASH_WINBOND)
>         case BFPT_DWORD15_QER_SR2_BIT1:
>                 params->quad_enable = spansion_read_cr_quad_enable;
>                 break;
> -#endif
>         default:
>                 return -EINVAL;
>         }
> @@ -2013,20 +1999,16 @@ static int spi_nor_init_params(struct spi_nor *nor,
>         if (params->hwcaps.mask & (SNOR_HWCAPS_READ_QUAD |
>                                    SNOR_HWCAPS_PP_QUAD)) {
>                 switch (JEDEC_MFR(info)) {
> -#ifdef CONFIG_SPI_FLASH_MACRONIX
>                 case SNOR_MFR_MACRONIX:
>                         params->quad_enable = macronix_quad_enable;
>                         break;
> -#endif
>                 case SNOR_MFR_ST:
>                 case SNOR_MFR_MICRON:
>                         break;
>
>                 default:
> -#if defined(CONFIG_SPI_FLASH_SPANSION) || defined(CONFIG_SPI_FLASH_WINBOND)
>                         /* Kept only for backward compatibility purpose. */
>                         params->quad_enable = spansion_read_cr_quad_enable;
> -#endif
>                         break;
>                 }
>         }
> @@ -2337,7 +2319,6 @@ int spi_nor_scan(struct spi_nor *nor)
>         mtd->_erase = spi_nor_erase;
>         mtd->_read = spi_nor_read;
>
> -#if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST)
>         /* NOR protection support for STmicro/Micron chips and similar */
>         if (JEDEC_MFR(info) == SNOR_MFR_ST ||
>             JEDEC_MFR(info) == SNOR_MFR_MICRON ||
> @@ -2347,7 +2328,6 @@ int spi_nor_scan(struct spi_nor *nor)
>                 nor->flash_unlock = stm_unlock;
>                 nor->flash_is_locked = stm_is_locked;
>         }
> -#endif
>
>  #ifdef CONFIG_SPI_FLASH_SST
>         /* sst nor chips use AAI word program */