[PATCH 0/3] Fixes for SPI-NAND issues on LS1088A
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(-)
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__ */
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
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
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@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,
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, }, { } };
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(-)
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);
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
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@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,
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, }, { } };
participants (2)
-
Mathew McBride -
Pratyush Yadav