[U-Boot] [PATCH 1/2] drivers/mtd/spi: Enable 4B opcodes for SPANSION s25fl512s

[U-Boot] [PATCH 1/2] drivers/mtd/spi: Enable 4B opcodes for SPANSION s25fl512s

[Rajat Srivastava]

From: Ashish Kumar <Ashish.Kumar@nxp.com>

Signed-off-by: Ashish Kumar <Ashish.Kumar@nxp.com>
---
 drivers/mtd/spi/spi-nor-ids.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/spi/spi-nor-ids.c b/drivers/mtd/spi/spi-nor-ids.c
index ec929760ee..a89c1910d9 100644
--- a/drivers/mtd/spi/spi-nor-ids.c
+++ b/drivers/mtd/spi/spi-nor-ids.c
@@ -177,7 +177,7 @@ const struct flash_info spi_nor_ids[] = {
 	{ INFO("s25sl064p",  0x010216, 0x4d00,  64 * 1024, 128, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
 	{ INFO("s25fl256s0", 0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) },
 	{ INFO("s25fl256s1", 0x010219, 0x4d01,  64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
-	{ INFO6("s25fl512s",  0x010220, 0x4d0081, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
+	{ INFO6("s25fl512s",  0x010220, 0x4d0081, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR | SPI_NOR_4B_OPCODES) },
 	{ INFO("s25fl512s_256k",  0x010220, 0x4d00, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
 	{ INFO("s25fl512s_64k",  0x010220, 0x4d01, 64 * 1024, 1024, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
 	{ INFO("s25fl512s_512k", 0x010220, 0x4f00, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
-- 
2.17.1

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

[Rajat Srivastava]

Signed-off-by: Ashish Kumar <ashish.kumar@nxp.com>
Signed-off-by: Rajat Srivastava <rajat.srivastava@nxp.com>
---
 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)) {
-- 
2.17.1

[U-Boot] [PATCH 1/2] drivers/mtd/spi: Enable 4B opcodes for SPANSION s25fl512s

[Vignesh Raghavendra]

Hi.

On 24-Apr-19 6:10 PM, Rajat Srivastava wrote:
> From: Ashish Kumar <Ashish.Kumar@nxp.com>
> 
> Signed-off-by: Ashish Kumar <Ashish.Kumar@nxp.com>
> ---
>  drivers/mtd/spi/spi-nor-ids.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi/spi-nor-ids.c b/drivers/mtd/spi/spi-nor-ids.c
> index ec929760ee..a89c1910d9 100644
> --- a/drivers/mtd/spi/spi-nor-ids.c
> +++ b/drivers/mtd/spi/spi-nor-ids.c
> @@ -177,7 +177,7 @@ const struct flash_info spi_nor_ids[] = {
>  	{ INFO("s25sl064p",  0x010216, 0x4d00,  64 * 1024, 128, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
>  	{ INFO("s25fl256s0", 0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) },
>  	{ INFO("s25fl256s1", 0x010219, 0x4d01,  64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
> -	{ INFO6("s25fl512s",  0x010220, 0x4d0081, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
> +	{ INFO6("s25fl512s",  0x010220, 0x4d0081, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR | SPI_NOR_4B_OPCODES) },

You should not be needing this change to enable 4 Byte opcodes. From
spi-nor-core.c:


        if (nor->addr_width) {
                /* already configured from SFDP */
        } else if (info->addr_width) {
                nor->addr_width = info->addr_width;
        } else if (mtd->size > SZ_16M) {
#ifndef CONFIG_SPI_FLASH_BAR
                /* enable 4-byte addressing if the device exceeds 16MiB */
                nor->addr_width = 4;
                if (JEDEC_MFR(info) == SNOR_MFR_SPANSION ||
                    info->flags & SPI_NOR_4B_OPCODES)
                        spi_nor_set_4byte_opcodes(nor, info);
#else
        /* Configure the BAR - discover bank cmds and read current bank */
        nor->addr_width = 3;
        ret = read_bar(nor, info);
        if (ret < 0)
                return ret;
#endif
	}
        [...]


So as long as SPI_FLASH_BAR is not set, 4 Byte opcodes are used by
default with Spansion flashes >16M size. If that's not the case, then we
need to root cause the actual bug instead of adding SPI_NOR_4B_OPCODES
to s25fl512s

Regards
Vignesh

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

[Vignesh Raghavendra]

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. But from $patch subject, I infer that $patch
is adding new feature and not actually fixing something broken?
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)) {
> 

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

[Rajat Srivastava]

> -----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 at lists.denx.de;
> jagan at 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)) {
> >

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

[Vignesh Raghavendra]

On 25/04/19 5:20 PM, Rajat Srivastava wrote:
> 
> 
>> -----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 at lists.denx.de;
>> jagan at 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.
> 

With SPL_FLASH_BAR set fsl_qspi driver should never see 4 byte opcodes
and should work like it did before. I guess you are disabling SPI_FLASH_BAR?

Please add all details to the commit message and I recommend you to move
the driver over to spi-mem as soon as possible. Else you would have to
keep handling new opcodes in your driver as and when spi-nor-core changes.

Regards
Vignesh

> 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)) {
>>>

-- 
Regards
Vignesh

[U-Boot] [EXT] Re: [PATCH 1/2] drivers/mtd/spi: Enable 4B opcodes for SPANSION s25fl512s

[Rajat Srivastava]

> -----Original Message-----
> From: Vignesh Raghavendra <vigneshr@ti.com>
> Sent: Wednesday, April 24, 2019 10:08 PM
> To: Rajat Srivastava <rajat.srivastava@nxp.com>; u-boot at lists.denx.de;
> jagan at openedev.com
> Cc: Ashish Kumar <ashish.kumar@nxp.com>
> Subject: [EXT] Re: [PATCH 1/2] drivers/mtd/spi: Enable 4B opcodes for
> SPANSION s25fl512s
> 
> Caution: EXT Email
> 
> Hi.
> 
> On 24-Apr-19 6:10 PM, Rajat Srivastava wrote:
> > From: Ashish Kumar <Ashish.Kumar@nxp.com>
> >
> > Signed-off-by: Ashish Kumar <Ashish.Kumar@nxp.com>
> > ---
> >  drivers/mtd/spi/spi-nor-ids.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/mtd/spi/spi-nor-ids.c
> > b/drivers/mtd/spi/spi-nor-ids.c index ec929760ee..a89c1910d9 100644
> > --- a/drivers/mtd/spi/spi-nor-ids.c
> > +++ b/drivers/mtd/spi/spi-nor-ids.c
> > @@ -177,7 +177,7 @@ const struct flash_info spi_nor_ids[] = {
> >       { INFO("s25sl064p",  0x010216, 0x4d00,  64 * 1024, 128,
> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> >       { INFO("s25fl256s0", 0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) },
> >       { INFO("s25fl256s1", 0x010219, 0x4d01,  64 * 1024, 512,
> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
> > -     { INFO6("s25fl512s",  0x010220, 0x4d0081, 256 * 1024, 256,
> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
> > +     { INFO6("s25fl512s",  0x010220, 0x4d0081, 256 * 1024, 256,
> > + SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR |
> > + SPI_NOR_4B_OPCODES) },
> 
> You should not be needing this change to enable 4 Byte opcodes. From
> spi-nor-core.c:
> 
> 
>         if (nor->addr_width) {
>                 /* already configured from SFDP */
>         } else if (info->addr_width) {
>                 nor->addr_width = info->addr_width;
>         } else if (mtd->size > SZ_16M) { #ifndef CONFIG_SPI_FLASH_BAR
>                 /* enable 4-byte addressing if the device exceeds 16MiB */
>                 nor->addr_width = 4;
>                 if (JEDEC_MFR(info) == SNOR_MFR_SPANSION ||
>                     info->flags & SPI_NOR_4B_OPCODES)
>                         spi_nor_set_4byte_opcodes(nor, info); #else
>         /* Configure the BAR - discover bank cmds and read current bank */
>         nor->addr_width = 3;
>         ret = read_bar(nor, info);
>         if (ret < 0)
>                 return ret;
> #endif
>         }
>         [...]
> 
> 
> So as long as SPI_FLASH_BAR is not set, 4 Byte opcodes are used by default
> with Spansion flashes >16M size. If that's not the case, then we need to root
> cause the actual bug instead of adding SPI_NOR_4B_OPCODES to s25fl512s

You are right. I've tested without using SPI_NOR_4B_OPCODES flag and it is working now. Deprecating this patch now.
 
Regards
Rajat

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

[Schrempf Frieder]

Hi,

On 26.04.19 06:58, Vignesh Raghavendra wrote:
> 
> 
> On 25/04/19 5:20 PM, Rajat Srivastava wrote:
>>
>>
>>> -----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 at lists.denx.de;
>>> jagan at 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.
>>
> 
> With SPL_FLASH_BAR set fsl_qspi driver should never see 4 byte opcodes
> and should work like it did before. I guess you are disabling SPI_FLASH_BAR?
> 
> Please add all details to the commit message and I recommend you to move
> the driver over to spi-mem as soon as possible. Else you would have to
> keep handling new opcodes in your driver as and when spi-nor-core changes.
> 
> Regards
> Vignesh
> 
>> 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.

I started moving the driver to spi-mem, by porting the kernel driver 
over to U-Boot. I still have some Kconfig dependency problem for the 
LS2081 platform (CONFIG_SPI_MEM is not enabled implicitly), that needs 
to be solved, otherwise it should be more or less ready.

For anyone who wants to check/test the current state, look here: [1]

Regards,
Frieder

[1]: https://github.com/fschrempf/u-boot/commits/fsl_qspi_spimem_port

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

[Vignesh Raghavendra]

On 30/04/19 1:13 PM, Schrempf Frieder wrote:
> Hi,
> 
> On 26.04.19 06:58, Vignesh Raghavendra wrote:
>>
>>
>> On 25/04/19 5:20 PM, Rajat Srivastava wrote:
>>>
>>>
>>>> -----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 at lists.denx.de;
>>>> jagan at 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.
>>>
>>
>> With SPL_FLASH_BAR set fsl_qspi driver should never see 4 byte opcodes
>> and should work like it did before. I guess you are disabling SPI_FLASH_BAR?
>>
>> Please add all details to the commit message and I recommend you to move
>> the driver over to spi-mem as soon as possible. Else you would have to
>> keep handling new opcodes in your driver as and when spi-nor-core changes.
>>
>> Regards
>> Vignesh
>>
>>> 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.
> 
> I started moving the driver to spi-mem, by porting the kernel driver 
> over to U-Boot. I still have some Kconfig dependency problem for the 
> LS2081 platform (CONFIG_SPI_MEM is not enabled implicitly), that needs 
> to be solved, otherwise it should be more or less ready.
> 

I am not familiar with ls2081 defconfig, but it seems like
CONFIG_SPI_FLASH is not enabled in ls2081ardb_defconfig due to which
SPI_MEM does not be selected. Jagan had a patch to make DM_SPI_FLASH
enable SPI_FLASH[2] to fix such configs.

> For anyone who wants to check/test the current state, look here: [1]
> 

That's great!

> Regards,
> Frieder
> 
> [1]: https://github.com/fschrempf/u-boot/commits/fsl_qspi_spimem_port
> 
[2] https://patchwork.ozlabs.org/patch/1039176/

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

[Ashish Kumar]

> -----Original Message-----
> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Schrempf Frieder
> Sent: Tuesday, April 30, 2019 1:14 PM
> To: Vignesh Raghavendra <vigneshr@ti.com>; Rajat Srivastava
> <rajat.srivastava@nxp.com>; u-boot at lists.denx.de; jagan at openedev.com
> Subject: [EXT] Re: [U-Boot] [PATCH 2/2] fsl_qspi: Improve QSPI driver to
> incorporate 4 byte commands
> 
> Caution: EXT Email
> 
> Hi,
> 
> On 26.04.19 06:58, Vignesh Raghavendra wrote:
> >
> >
> > On 25/04/19 5:20 PM, Rajat Srivastava wrote:
> >>
> >>
> >>> -----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 at lists.denx.de; jagan at 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.
> >>
> >
> > With SPL_FLASH_BAR set fsl_qspi driver should never see 4 byte opcodes
> > and should work like it did before. I guess you are disabling SPI_FLASH_BAR?
> >
> > Please add all details to the commit message and I recommend you to
> > move the driver over to spi-mem as soon as possible. Else you would
> > have to keep handling new opcodes in your driver as and when spi-nor-core
> changes.
> >
> > Regards
> > Vignesh
> >
> >> 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.
> 
> I started moving the driver to spi-mem, by porting the kernel driver over to U-
> Boot. I still have some Kconfig dependency problem for the
> LS2081 platform (CONFIG_SPI_MEM is not enabled implicitly), that needs to
> be solved, otherwise it should be more or less ready.
Hi Frieder,

What is the change, it seems it is moving the driver from Linux as it is to uboot ?
I am not sure how stable is the current Linux driver, since it is not yet tested by FSL/NXP system test team. Last time I tested the current Linux driver(QSPI) it was functional in only 1-1-1 mode. Moving to u-boot may not be good idea at this point of time.

How do you plan to cater  CONFIG_SPI_BAR change in uboot now. Some old board still uses flash that supports CONFIG_SPI_BAR.

Me and Rajat have recently posted patches to fix current u-boot driver to be functional with new frame work pushed by Vignesh, and it serves all the current supported board with and without CONFIG_SPI_BAR.

May be spi-nxp-fspi.c can be a good starting point, but then again I not sure how booting from serial-nand will be taken care in that case.

Regards
Ashish 
 
> 
> For anyone who wants to check/test the current state, look here: [1]
> 
> Regards,
> Frieder
> 
> [1]:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
> com%2Ffschrempf%2Fu-
> boot%2Fcommits%2Ffsl_qspi_spimem_port&amp;data=02%7C01%7CAshish.K
> umar%40nxp.com%7C459c6d89b8b748c928ca08d6cd3fa6cd%7C686ea1d3bc
> 2b4c6fa92cd99c5c301635%7C0%7C0%7C636922070665346047&amp;sdata
> =8HplTAZYeEaBrXZd38h4hgT7hLRixjobx%2ByCjL%2FjJjc%3D&amp;reserved=0
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.de
> nx.de%2Flistinfo%2Fu-
> boot&amp;data=02%7C01%7CAshish.Kumar%40nxp.com%7C459c6d89b8b74
> 8c928ca08d6cd3fa6cd%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> %7C636922070665346047&amp;sdata=sk6GB%2BMeEKjqR5BR5K9PX6yYayC
> nyDUG69LTWl05xlM%3D&amp;reserved=0

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

[Schrempf Frieder]

Hi Ashish,

On 01.05.19 07:32, Ashish Kumar wrote:
> 
> 
>> -----Original Message-----
>> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Schrempf Frieder
>> Sent: Tuesday, April 30, 2019 1:14 PM
>> To: Vignesh Raghavendra <vigneshr@ti.com>; Rajat Srivastava
>> <rajat.srivastava@nxp.com>; u-boot at lists.denx.de; jagan at openedev.com
>> Subject: [EXT] Re: [U-Boot] [PATCH 2/2] fsl_qspi: Improve QSPI driver to
>> incorporate 4 byte commands
>>
>> Caution: EXT Email
>>
>> Hi,
>>
>> On 26.04.19 06:58, Vignesh Raghavendra wrote:
>>>
>>>
>>> On 25/04/19 5:20 PM, Rajat Srivastava wrote:
>>>>
>>>>
>>>>> -----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 at lists.denx.de; jagan at 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.
>>>>
>>>
>>> With SPL_FLASH_BAR set fsl_qspi driver should never see 4 byte opcodes
>>> and should work like it did before. I guess you are disabling SPI_FLASH_BAR?
>>>
>>> Please add all details to the commit message and I recommend you to
>>> move the driver over to spi-mem as soon as possible. Else you would
>>> have to keep handling new opcodes in your driver as and when spi-nor-core
>> changes.
>>>
>>> Regards
>>> Vignesh
>>>
>>>> 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.
>>
>> I started moving the driver to spi-mem, by porting the kernel driver over to U-
>> Boot. I still have some Kconfig dependency problem for the
>> LS2081 platform (CONFIG_SPI_MEM is not enabled implicitly), that needs to
>> be solved, otherwise it should be more or less ready.
> Hi Frieder,
> 
> What is the change, it seems it is moving the driver from Linux as it is to uboot ?

Yes, it's moving the current Linux driver to U-Boot with some small 
compatibility changes and simplifications.

> I am not sure how stable is the current Linux driver, since it is not yet tested by FSL/NXP system test team. Last time I tested the current Linux driver(QSPI) it was functional in only 1-1-1 mode. Moving to u-boot may not be good idea at this point of time.

The current mainline Linux driver is a SPI controller driver using the 
SPI MEM API and it replaced the old SPI NOR driver in Linux 5.1. If 
nothing went wrong in the upstreaming process, the driver should be just 
as good as the old one, while bringing all the advantages of the new SPI 
MEM interface. The driver does support 1-1-1 mode, just like all other 
modes supported by the QSPI controller.

While upstreaming to the kernel, the driver was tested by Han Xu and 
Yogesh Gaur.

> How do you plan to cater  CONFIG_SPI_BAR change in uboot now. Some old board still uses flash that supports CONFIG_SPI_BAR.

The new driver is not a SPI NOR driver, so it does not care about 
CONFIG_SPI_BAR. It just handels SPI MEM operations created by the upper 
layer.

> Me and Rajat have recently posted patches to fix current u-boot driver to be functional with new frame work pushed by Vignesh, and it serves all the current supported board with and without CONFIG_SPI_BAR.
> 
> May be spi-nxp-fspi.c can be a good starting point, but then again I not sure how booting from serial-nand will be taken care in that case.

Please have a look at the SPI MEM and SPI NAND APIs to understand why 
porting the Linux driver is the right thing to do instead of patching 
the old driver. By supporting the SPI MEM API, the driver will 
implicitly allow to interface SPI NAND chips.

Thanks,
Frieder

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

[Ashish Kumar]

> -----Original Message-----
> From: Schrempf Frieder <frieder.schrempf@kontron.de>
> Sent: Thursday, May 2, 2019 12:38 PM
> To: Ashish Kumar <ashish.kumar@nxp.com>; Vignesh Raghavendra
> <vigneshr@ti.com>; Rajat Srivastava <rajat.srivastava@nxp.com>; u-
> boot at lists.denx.de; jagan at openedev.com
> Subject: Re: [EXT] Re: [U-Boot] [PATCH 2/2] fsl_qspi: Improve QSPI driver to
> incorporate 4 byte commands
> 
> Caution: EXT Email
> 
> Hi Ashish,
> 
> On 01.05.19 07:32, Ashish Kumar wrote:
> >
> >
> >> -----Original Message-----
> >> From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Schrempf
> >> Frieder
> >> Sent: Tuesday, April 30, 2019 1:14 PM
> >> To: Vignesh Raghavendra <vigneshr@ti.com>; Rajat Srivastava
> >> <rajat.srivastava@nxp.com>; u-boot at lists.denx.de;
> jagan at openedev.com
> >> Subject: [EXT] Re: [U-Boot] [PATCH 2/2] fsl_qspi: Improve QSPI driver
> >> to incorporate 4 byte commands
> >>
> >> Caution: EXT Email
> >>
> >> Hi,
> >>
> >> On 26.04.19 06:58, Vignesh Raghavendra wrote:
> >>>
> >>>
> >>> On 25/04/19 5:20 PM, Rajat Srivastava wrote:
> >>>>
> >>>>
> >>>>> -----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 at lists.denx.de; jagan at 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.
> >>>>
> >>>
> >>> With SPL_FLASH_BAR set fsl_qspi driver should never see 4 byte
> >>> opcodes and should work like it did before. I guess you are disabling
> SPI_FLASH_BAR?
> >>>
> >>> Please add all details to the commit message and I recommend you to
> >>> move the driver over to spi-mem as soon as possible. Else you would
> >>> have to keep handling new opcodes in your driver as and when
> >>> spi-nor-core
> >> changes.
> >>>
> >>> Regards
> >>> Vignesh
> >>>
> >>>> 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.
> >>
> >> I started moving the driver to spi-mem, by porting the kernel driver
> >> over to U- Boot. I still have some Kconfig dependency problem for the
> >> LS2081 platform (CONFIG_SPI_MEM is not enabled implicitly), that
> >> needs to be solved, otherwise it should be more or less ready.
> > Hi Frieder,
> >
> > What is the change, it seems it is moving the driver from Linux as it is to
> uboot ?
> 
> Yes, it's moving the current Linux driver to U-Boot with some small
> compatibility changes and simplifications.
> 
> > I am not sure how stable is the current Linux driver, since it is not yet tested
> by FSL/NXP system test team. Last time I tested the current Linux
> driver(QSPI) it was functional in only 1-1-1 mode. Moving to u-boot may not
> be good idea at this point of time.
> 
> The current mainline Linux driver is a SPI controller driver using the SPI MEM
> API and it replaced the old SPI NOR driver in Linux 5.1. If nothing went wrong
> in the upstreaming process, the driver should be just as good as the old one,
> while bringing all the advantages of the new SPI MEM interface. The driver
> does support 1-1-1 mode, just like all other modes supported by the QSPI
> controller.
> 
> While upstreaming to the kernel, the driver was tested by Han Xu and Yogesh
> Gaur.
Hi Frieder,

I had a discussion Han Xu, He was also able to find some defects with current linux QSPI driver on i.mx7 platform.

I had also tried testing this with QUAD/DUAL on LS1088 and It was not functional.
"s25fl512s",  INFO(0x010220, 0x4d00, 256 * 1024, 256,  SPI_NOR_DUAL_READ |SPI_NOR_QUAD_READ | USE_CLSR| SPI_NOR_4B_OPCODES),

I will be debugging further and update with findings in sometime.

Regards 
Ashish 
> 
> > How do you plan to cater  CONFIG_SPI_BAR change in uboot now. Some
> old board still uses flash that supports CONFIG_SPI_BAR.
> 
> The new driver is not a SPI NOR driver, so it does not care about
> CONFIG_SPI_BAR. It just handels SPI MEM operations created by the upper
> layer.
> 
> > Me and Rajat have recently posted patches to fix current u-boot driver to
> be functional with new frame work pushed by Vignesh, and it serves all the
> current supported board with and without CONFIG_SPI_BAR.
> >
> > May be spi-nxp-fspi.c can be a good starting point, but then again I not sure
> how booting from serial-nand will be taken care in that case.
> 
> Please have a look at the SPI MEM and SPI NAND APIs to understand why
> porting the Linux driver is the right thing to do instead of patching the old
> driver. By supporting the SPI MEM API, the driver will implicitly allow to
> interface SPI NAND chips.
> 
> Thanks,
> Frieder