[PATCH 0/3] Fixes for SPI-NAND issues on LS1088A

[PATCH 0/3] Fixes for SPI-NAND issues on LS1088A

[Mathew McBride]

This patchset resolves issues seen when using SPI-NAND
with the LS1088A's qspi controller.

The main issue seen is data corruption when reading SPI-NAND,
due to a controller quirk not being applied. Using the same
settings as the LS2088A (the bigger brother of LS1088A) solves
this issue.

In the course of debugging the above issue, it was found
that the fsl_qspi driver was not correctly reporting the operation
width (single/dual/quad) configured for the device in DTS.
(e.g I configured my device for single-lane reads only
but quad operations were being issued)

A fix for this issue is already present in Linux and can be
adapted for U-Boot, providing a missing export for
spi_mem_default_supports_op is added.

Mathew McBride (3):
  mem: spi-mem: define spi_mem_default_supports_op
  spi: fsl_qspi: Ensure width is respected in spi-mem operations
  spi: fsl_qspi: apply the same settings for LS1088 as LS208x

 drivers/spi/fsl_qspi.c | 12 ++----------
 include/spi-mem.h      | 10 ++++++++++
 2 files changed, 12 insertions(+), 10 deletions(-)

-- 
2.30.0

[PATCH 1/3] mem: spi-mem: define spi_mem_default_supports_op

[Mathew McBride]

spi_mem_default_supports_op is used internally by controller
drivers to verify operation semantics are correct.

Signed-off-by: Mathew McBride <matt@traverse.com.au>
---
 include/spi-mem.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/spi-mem.h b/include/spi-mem.h
index ca0f55c8fd..92c640dabe 100644
--- a/include/spi-mem.h
+++ b/include/spi-mem.h
@@ -216,6 +216,10 @@ int spi_controller_dma_map_mem_op_data(struct spi_controller *ctlr,
 void spi_controller_dma_unmap_mem_op_data(struct spi_controller *ctlr,
 					  const struct spi_mem_op *op,
 					  struct sg_table *sg);
+
+bool spi_mem_default_supports_op(struct spi_mem *mem,
+				 const struct spi_mem_op *op);
+
 #else
 static inline int
 spi_controller_dma_map_mem_op_data(struct spi_controller *ctlr,
@@ -231,6 +235,12 @@ spi_controller_dma_unmap_mem_op_data(struct spi_controller *ctlr,
 				     struct sg_table *sg)
 {
 }
+
+bool spi_mem_default_supports_op(struct spi_mem *mem,
+				 const struct spi_mem_op *op)
+{
+	return false;
+}
 #endif /* CONFIG_SPI_MEM */
 #endif /* __UBOOT__ */
 
-- 
2.30.0

[PATCH 1/3] mem: spi-mem: define spi_mem_default_supports_op

[Pratyush Yadav]

Hi Matthew,

> Subject: [PATCH 1/3] mem: spi-mem: define spi_mem_default_supports_op

Nitpick: You are declaring spi_mem_default_supports_op() here. It is 
already defined.

On 18/01/21 11:52PM, Mathew McBride wrote:
> spi_mem_default_supports_op is used internally by controller
> drivers to verify operation semantics are correct.
> 
> Signed-off-by: Mathew McBride <matt@traverse.com.au>
> ---
>  include/spi-mem.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/spi-mem.h b/include/spi-mem.h
> index ca0f55c8fd..92c640dabe 100644
> --- a/include/spi-mem.h
> +++ b/include/spi-mem.h
> @@ -216,6 +216,10 @@ int spi_controller_dma_map_mem_op_data(struct spi_controller *ctlr,
>  void spi_controller_dma_unmap_mem_op_data(struct spi_controller *ctlr,
>  					  const struct spi_mem_op *op,
>  					  struct sg_table *sg);
> +
> +bool spi_mem_default_supports_op(struct spi_mem *mem,
> +				 const struct spi_mem_op *op);
> +

This block of code was imported verbatim from the Linux driver and then 
wrapped around with a #ifndef __UBOOT__ to avoid compilation errors. So 
it will never get "enabled" in U-Boot ever. No driver can use the 
prototypes you have added.

And I tested this by applying your patch series and building the 
fsl_qspi driver using ls1012aqds_qspi_defconfig. Sure enough, the 
compiler reported "implicit declaration of function 
spi_mem_default_supports_op". Strangely, the linker did not complain and 
went through without errors. Not sure which function it would end up 
linking there.

Move the declaration outside this ifdef, right beside where
spi_mem_supports_op() is declared. No need to have the variant below. It 
is safe to assume CONFIG_SPI_MEM is enabled if spi-mem.h is included.

>  #else
>  static inline int
>  spi_controller_dma_map_mem_op_data(struct spi_controller *ctlr,
> @@ -231,6 +235,12 @@ spi_controller_dma_unmap_mem_op_data(struct spi_controller *ctlr,
>  				     struct sg_table *sg)
>  {
>  }
> +
> +bool spi_mem_default_supports_op(struct spi_mem *mem,
> +				 const struct spi_mem_op *op)
> +{
> +	return false;
> +}
>  #endif /* CONFIG_SPI_MEM */
>  #endif /* __UBOOT__ */
>  
> -- 
> 2.30.0
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments India

[PATCH 1/3] mem: spi-mem: define spi_mem_default_supports_op

[Mathew McBride]

Hello Pratyush,
On Tue, Jan 19, 2021, at 11:06 PM, Pratyush Yadav wrote:
> Hi Matthew,
> 
> > Subject: [PATCH 1/3] mem: spi-mem: define spi_mem_default_supports_op
> 
> Nitpick: You are declaring spi_mem_default_supports_op() here. It is 
> already defined.
> [snip]
> 
> This block of code was imported verbatim from the Linux driver and then 
> wrapped around with a #ifndef __UBOOT__ to avoid compilation errors. So 
> it will never get "enabled" in U-Boot ever. No driver can use the 
> prototypes you have added.
> 
> And I tested this by applying your patch series and building the 
> fsl_qspi driver using ls1012aqds_qspi_defconfig. Sure enough, the 
> compiler reported "implicit declaration of function 
> spi_mem_default_supports_op". Strangely, the linker did not complain and 
> went through without errors. Not sure which function it would end up 
> linking there.
> 
> Move the declaration outside this ifdef, right beside where
> spi_mem_supports_op() is declared. No need to have the variant below. It 
> is safe to assume CONFIG_SPI_MEM is enabled if spi-mem.h is included.
> 
Many thanks for your feedback - I did not account for the differences in the kernel and U-Boot here. 

My revised patch should handle this correctly.

Best Regards,
Mathew

[PATCH 2/3] spi: fsl_qspi: Ensure width is respected in spi-mem operations

[Mathew McBride]

Adapted from kernel commit b0177aca7aea
From: Michael Walle <michael@walle.cc>

Make use of a core helper to ensure the desired width is respected
when calling spi-mem operators.

Otherwise only the SPI controller will be matched with the flash chip,
which might lead to wrong widths. Also consider the width specified by
the user in the device tree.

Fixes: 91afd36f38 ("spi: Add a driver for the Freescale/NXP QuadSPI controller")
Signed-off-by: Michael Walle <michael@walle.cc>
Link: https://lore.kernel.org/r/20200114154613.8195-1-michael at walle.cc
Signed-off-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Mathew McBride <matt@traverse.com.au> [adapt for U-Boot]
---
 drivers/spi/fsl_qspi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c
index 8bc7038a82..2a1f3a0c44 100644
--- a/drivers/spi/fsl_qspi.c
+++ b/drivers/spi/fsl_qspi.c
@@ -409,7 +409,7 @@ static bool fsl_qspi_supports_op(struct spi_slave *slave,
 	    op->data.nbytes > q->devtype_data->txfifo)
 		return false;
 
-	return true;
+	return spi_mem_default_supports_op(slave, op);
 }
 
 static void fsl_qspi_prepare_lut(struct fsl_qspi *q,
-- 
2.30.0

[PATCH 3/3] spi: fsl_qspi: apply the same settings for LS1088 as LS208x

[Mathew McBride]

The LS1088 requires the same QUADSPI_QURIK_BASE_INTERNAL
workaround as the LS208x and also has a 64 byte TX buffer.

With the previous settings SPI-NAND reads over AHB were
corrupted.

Fixes: 91afd36f3802 ("spi: Transform the FSL QuadSPI driver to use the SPI MEM API")
Signed-off-by: Mathew McBride <matt@traverse.com.au>
---
 drivers/spi/fsl_qspi.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c
index 2a1f3a0c44..f965301d6a 100644
--- a/drivers/spi/fsl_qspi.c
+++ b/drivers/spi/fsl_qspi.c
@@ -259,14 +259,6 @@ static const struct fsl_qspi_devtype_data ls1021a_data = {
 	.little_endian = false,
 };
 
-static const struct fsl_qspi_devtype_data ls1088a_data = {
-	.rxfifo = SZ_128,
-	.txfifo = SZ_128,
-	.ahb_buf_size = SZ_1K,
-	.quirks = QUADSPI_QUIRK_TKT253890,
-	.little_endian = true,
-};
-
 static const struct fsl_qspi_devtype_data ls2080a_data = {
 	.rxfifo = SZ_128,
 	.txfifo = SZ_64,
@@ -877,7 +869,7 @@ static const struct udevice_id fsl_qspi_ids[] = {
 	{ .compatible = "fsl,imx7d-qspi", .data = (ulong)&imx7d_data, },
 	{ .compatible = "fsl,imx7ulp-qspi", .data = (ulong)&imx7ulp_data, },
 	{ .compatible = "fsl,ls1021a-qspi", .data = (ulong)&ls1021a_data, },
-	{ .compatible = "fsl,ls1088a-qspi", .data = (ulong)&ls1088a_data, },
+	{ .compatible = "fsl,ls1088a-qspi", .data = (ulong)&ls2080a_data, },
 	{ .compatible = "fsl,ls2080a-qspi", .data = (ulong)&ls2080a_data, },
 	{ }
 };
-- 
2.30.0

[PATCH v2 0/3] Fixes for SPI-NAND issues on LS1088A

[Mathew McBride]

This patchset resolves issues seen when using SPI-NAND with the
LS1088A's qspi controller.

The main issue seen is data corruption when reading SPI-NAND, due to a
controller quirk not being applied.
Using the same settings as the LS2088A (the bigger brother of LS1088A) solves this issue.

In the course of debugging the above issue, it was found that the fsl_qspi
driver was not correctly reporting the operation width (single/dual/quad)
configured for the device in DTS.
(e.g I configured my device for single-lane reads only but quad operations were being issued)

A fix for this issue is already present in Linux and can be adapted for U-Boot,
providing a missing declaration for spi_mem_default_supports_op is added.

Changes in v2:
* Correct addition of declration for spi_mem_default_supports_op
The previous patch attempted to mirror how it was added to the Linux
kernel, which is not relevant to U-Boot

Mathew McBride (3):
  mem: spi-mem: add declaration for spi_mem_default_supports_op
  spi: fsl_qspi: Ensure width is respected in spi-mem operations
  spi: fsl_qspi: apply the same settings for LS1088 as LS208x

 drivers/spi/fsl_qspi.c | 12 ++----------
 include/spi-mem.h      |  3 +++
 2 files changed, 5 insertions(+), 10 deletions(-)

-- 
2.30.0

[PATCH v2 1/3] mem: spi-mem: add declaration for spi_mem_default_supports_op

[Mathew McBride]

spi_mem_default_supports_op is used internally by controller
drivers to verify operation semantics are correct.

It is used internally inside spi-mem but has not (in U-Boot)
been declared in spi-mem.h for external use.

Signed-off-by: Mathew McBride <matt@traverse.com.au>
---
 include/spi-mem.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/spi-mem.h b/include/spi-mem.h
index ca0f55c8fd..8be3e2bf6b 100644
--- a/include/spi-mem.h
+++ b/include/spi-mem.h
@@ -240,6 +240,9 @@ bool spi_mem_supports_op(struct spi_slave *slave, const struct spi_mem_op *op);
 
 int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op);
 
+bool spi_mem_default_supports_op(struct spi_slave *mem,
+				 const struct spi_mem_op *op);
+
 #ifndef __UBOOT__
 int spi_mem_driver_register_with_owner(struct spi_mem_driver *drv,
 				       struct module *owner);
-- 
2.30.0

[PATCH v2 1/3] mem: spi-mem: add declaration for spi_mem_default_supports_op

[Pratyush Yadav]

On 25/01/21 03:55AM, Mathew McBride wrote:
> spi_mem_default_supports_op is used internally by controller
> drivers to verify operation semantics are correct.
> 
> It is used internally inside spi-mem but has not (in U-Boot)
> been declared in spi-mem.h for external use.
> 
> Signed-off-by: Mathew McBride <matt@traverse.com.au>

Reviewed-by: Pratyush Yadav <p.yadav@ti.com>

> ---
>  include/spi-mem.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/spi-mem.h b/include/spi-mem.h
> index ca0f55c8fd..8be3e2bf6b 100644
> --- a/include/spi-mem.h
> +++ b/include/spi-mem.h
> @@ -240,6 +240,9 @@ bool spi_mem_supports_op(struct spi_slave *slave, const struct spi_mem_op *op);
>  
>  int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op);
>  
> +bool spi_mem_default_supports_op(struct spi_slave *mem,
> +				 const struct spi_mem_op *op);
> +
>  #ifndef __UBOOT__
>  int spi_mem_driver_register_with_owner(struct spi_mem_driver *drv,
>  				       struct module *owner);
> -- 
> 2.30.0
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments India

[PATCH v2 2/3] spi: fsl_qspi: Ensure width is respected in spi-mem operations

[Mathew McBride]

Adapted from kernel commit b0177aca7aea
From: Michael Walle <michael@walle.cc>

Make use of a core helper to ensure the desired width is respected
when calling spi-mem operators.

Otherwise only the SPI controller will be matched with the flash chip,
which might lead to wrong widths. Also consider the width specified by
the user in the device tree.

Fixes: 91afd36f38 ("spi: Add a driver for the Freescale/NXP QuadSPI controller")
Signed-off-by: Michael Walle <michael@walle.cc>
Link: https://lore.kernel.org/r/20200114154613.8195-1-michael at walle.cc
Signed-off-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Mathew McBride <matt@traverse.com.au> [adapt for U-Boot]
---
 drivers/spi/fsl_qspi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c
index 8bc7038a82..2a1f3a0c44 100644
--- a/drivers/spi/fsl_qspi.c
+++ b/drivers/spi/fsl_qspi.c
@@ -409,7 +409,7 @@ static bool fsl_qspi_supports_op(struct spi_slave *slave,
 	    op->data.nbytes > q->devtype_data->txfifo)
 		return false;
 
-	return true;
+	return spi_mem_default_supports_op(slave, op);
 }
 
 static void fsl_qspi_prepare_lut(struct fsl_qspi *q,
-- 
2.30.0

[PATCH v2 3/3] spi: fsl_qspi: apply the same settings for LS1088 as LS208x

[Mathew McBride]

The LS1088 requires the same QUADSPI_QURIK_BASE_INTERNAL
workaround as the LS208x and also has a 64 byte TX buffer.

With the previous settings SPI-NAND reads over AHB were
corrupted.

Fixes: 91afd36f3802 ("spi: Transform the FSL QuadSPI driver to use the SPI MEM API")
Signed-off-by: Mathew McBride <matt@traverse.com.au>
---
 drivers/spi/fsl_qspi.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c
index 2a1f3a0c44..f965301d6a 100644
--- a/drivers/spi/fsl_qspi.c
+++ b/drivers/spi/fsl_qspi.c
@@ -259,14 +259,6 @@ static const struct fsl_qspi_devtype_data ls1021a_data = {
 	.little_endian = false,
 };
 
-static const struct fsl_qspi_devtype_data ls1088a_data = {
-	.rxfifo = SZ_128,
-	.txfifo = SZ_128,
-	.ahb_buf_size = SZ_1K,
-	.quirks = QUADSPI_QUIRK_TKT253890,
-	.little_endian = true,
-};
-
 static const struct fsl_qspi_devtype_data ls2080a_data = {
 	.rxfifo = SZ_128,
 	.txfifo = SZ_64,
@@ -877,7 +869,7 @@ static const struct udevice_id fsl_qspi_ids[] = {
 	{ .compatible = "fsl,imx7d-qspi", .data = (ulong)&imx7d_data, },
 	{ .compatible = "fsl,imx7ulp-qspi", .data = (ulong)&imx7ulp_data, },
 	{ .compatible = "fsl,ls1021a-qspi", .data = (ulong)&ls1021a_data, },
-	{ .compatible = "fsl,ls1088a-qspi", .data = (ulong)&ls1088a_data, },
+	{ .compatible = "fsl,ls1088a-qspi", .data = (ulong)&ls2080a_data, },
 	{ .compatible = "fsl,ls2080a-qspi", .data = (ulong)&ls2080a_data, },
 	{ }
 };
-- 
2.30.0