Re: [U-Boot] [EXT] Re: [PATCH 2/2] fsl_qspi: Improve QSPI driver to incorporate 4 byte commands

-----Original Message----- From: Vignesh Raghavendra vigneshr@ti.com Sent: Wednesday, April 24, 2019 10:17 PM To: Rajat Srivastava rajat.srivastava@nxp.com; u-boot@lists.denx.de; jagan@openedev.com Cc: Ashish Kumar ashish.kumar@nxp.com Subject: [EXT] Re: [PATCH 2/2] fsl_qspi: Improve QSPI driver to incorporate 4 byte commands

Caution: EXT Email

On 24-Apr-19 6:10 PM, Rajat Srivastava wrote:

...

Signed-off-by: Ashish Kumar ashish.kumar@nxp.com Signed-off-by: Rajat Srivastava rajat.srivastava@nxp.com

Commit message is missing.

I'll add proper commit message in the next patch version.

But from $patch subject, I infer that $patch is adding new feature and not actually fixing something broken?

Earlier the framework was designed to work for only 3-byte opcodes but our controller supports flashes of size greater than 16 MB. As a workaround, FSL QSPI driver used to explicitly send 4-byte opcodes for 3-byte opcodes sent by framework to the flash. Also there used to exist a temporary patch for framework which never got accepted In upstream. Now the new framework supports 4-byte opcodes and FSL QSPI driver needs correction. I am not introducing any new feature. I am just fixing the driver to suit the current framework.

Please let me know your feedback.

Regards Rajat

If so, you should move the driver over to use spi-mem APIs instead of adding more features and hard coding more flash specific commands in the driver. This makes driver duplicate more of spi-nor core code. I discourage adding new features w/o moving driver over to spi-mem. IMHO, converting the driver would not be a huge effort. And I believe its already done in kernel.

Regards Vignesh

...

---

drivers/spi/fsl_qspi.c | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index 1598c4f698..1d26c6344b 100644 --- a/drivers/spi/fsl_qspi.c +++ b/drivers/spi/fsl_qspi.c @@ -26,7 +26,7 @@ DECLARE_GLOBAL_DATA_PTR; #define TX_BUFFER_SIZE 0x40 #endif

-#define OFFSET_BITS_MASK GENMASK(23, 0) +#define OFFSET_BITS_MASK GENMASK(27, 0)

#define FLASH_STATUS_WEL 0x02

@@ -754,7 +754,8 @@ static void qspi_op_erase(struct fsl_qspi_priv *priv) while (qspi_read32(priv->flags, &regs->sr) & QSPI_SR_BUSY_MASK) ;

• if (priv->cur_seqid == QSPI_CMD_SE) {
  

• if ((priv->cur_seqid == QSPI_CMD_SE_4B) ||
  

•     (priv->cur_seqid == QSPI_CMD_SE)) {
          qspi_write32(priv->flags, &regs->ipcr,
                       (SEQID_SE << QSPI_IPCR_SEQID_SHIFT) | 0);
  } else if (priv->cur_seqid == QSPI_CMD_BE_4K) { @@ -775,31
  

+776,40 @@ int qspi_xfer(struct fsl_qspi_priv *priv, unsigned int bitlen, u32 txbuf;

  WATCHDOG_RESET();

• if (dout) {
          if (flags & SPI_XFER_BEGIN) {
                  priv->cur_seqid = *(u8 *)dout;
  

•                 memcpy(&txbuf, dout, 4);
  

•                 if (FSL_QSPI_FLASH_SIZE  > SZ_16M && bytes > 4)
  

•                         memcpy(&txbuf, dout + 1, 4);
  

•                 else
  

•                         memcpy(&txbuf, dout, 4);
          }
  
          if (flags == SPI_XFER_END) {
                  priv->sf_addr = wr_sfaddr;
  

•                 qspi_op_write(priv, (u8 *)dout, bytes);
  

•                 return 0;
  

•                 if (priv->cur_seqid == QSPI_CMD_PP ||
  

•                     priv->cur_seqid == QSPI_CMD_PP_4B ||
  

•                     priv->cur_seqid == QSPI_CMD_WRAR) {
  

•                         qspi_op_write(priv, (u8 *)dout, bytes);
  

•                         return 0;
  

•                 }
          }
  
  

•         if (priv->cur_seqid == QSPI_CMD_FAST_READ ||
  

•             priv->cur_seqid == QSPI_CMD_RDAR) {
  

•         if ((priv->cur_seqid == QSPI_CMD_FAST_READ) ||
  

•             (priv->cur_seqid == QSPI_CMD_FAST_READ_4B) ||
  

•             (priv->cur_seqid == QSPI_CMD_RDAR)) {
                  priv->sf_addr = swab32(txbuf) & OFFSET_BITS_MASK;
          } else if ((priv->cur_seqid == QSPI_CMD_SE) ||
  

•                    (priv->cur_seqid == QSPI_CMD_BE_4K)) {
  

•                    priv->cur_seqid == QSPI_CMD_SE_4B ||
  

•                    priv->cur_seqid == QSPI_CMD_BE_4K) {
                  priv->sf_addr = swab32(txbuf) & OFFSET_BITS_MASK;
                  qspi_op_erase(priv);
          } else if (priv->cur_seqid == QSPI_CMD_PP ||
  

•                    priv->cur_seqid == QSPI_CMD_PP_4B ||
                     priv->cur_seqid == QSPI_CMD_WRAR) {
                  wr_sfaddr = swab32(txbuf) & OFFSET_BITS_MASK;
          } else if ((priv->cur_seqid == QSPI_CMD_BRWR) ||
  

•                  (priv->cur_seqid == QSPI_CMD_WREAR)) {
  

•                    (priv->cur_seqid == QSPI_CMD_WREAR)) {
  

#ifdef CONFIG_SPI_FLASH_BAR wr_sfaddr = 0; #endif @@ -807,7 +817,8 @@ int qspi_xfer(struct fsl_qspi_priv *priv, unsigned int

bitlen,

...

  }

  if (din) {

•         if (priv->cur_seqid == QSPI_CMD_FAST_READ) {
  

•         if ((priv->cur_seqid == QSPI_CMD_FAST_READ) ||
  

•             (priv->cur_seqid == QSPI_CMD_FAST_READ_4B)) {
  

#ifdef CONFIG_SYS_FSL_QSPI_AHB qspi_ahb_read(priv, din, bytes); #else @@ -815,10 +826,11 @@ int qspi_xfer(struct fsl_qspi_priv *priv, unsigned int bitlen, #endif } else if (priv->cur_seqid == QSPI_CMD_RDAR) { qspi_op_read(priv, din, bytes);

•         } else if (priv->cur_seqid == QSPI_CMD_RDID)
  

•         } else if (priv->cur_seqid == QSPI_CMD_RDID) {
                  qspi_op_rdid(priv, din, bytes);
  

•         else if (priv->cur_seqid == QSPI_CMD_RDSR)
  

•         } else if (priv->cur_seqid == QSPI_CMD_RDSR) {
                  qspi_op_rdsr(priv, din, bytes);
  

•         }
  

#ifdef CONFIG_SPI_FLASH_BAR else if ((priv->cur_seqid == QSPI_CMD_BRRD) || (priv->cur_seqid == QSPI_CMD_RDEAR)) {