* [PATCH v2] spi: cadence-qspi: Remove cdns,is-dma property handling
@ 2025-10-09 10:34 Michal Simek
2025-10-19 22:03 ` E Shattow
0 siblings, 1 reply; 8+ messages in thread
From: Michal Simek @ 2025-10-09 10:34 UTC (permalink / raw)
To: u-boot, git
Cc: Boon Khai Ng, Jagan Teki, Naman Trivedi, Naresh Kumar Ravulapalli,
Neha Malcom Francis, Prasad Kummari, Prasanth Babu Mantena,
Ronald Wahl, Tejas Bhumkar, Tien Fong Chee, Tom Rini,
Venkatesh Yadav Abbarapu
Remove cdns,is-dma DT property handling. Property is not the part of DT
binding and it is also hardcoded to value 1 in all DTs that's why remove it
because none is also testing value 0.
If there is any use case when this configuration should be supported this
patch can be reverted.
Signed-off-by: Michal Simek <michal.simek@amd.com>
---
Changes in v2:
- Remove other functions which are reported as unused by gitlab CI.
v1: https://lore.kernel.org/r/c6a5b4bfe565668eb3299d5c5a5548a2163b129a.1758786568.git.michal.simek@amd.com
---
arch/arm/dts/versal-mini-ospi.dtsi | 1 -
arch/arm/dts/versal-net-mini-ospi.dtsi | 1 -
drivers/spi/cadence_qspi.c | 8 +-
drivers/spi/cadence_qspi.h | 5 --
drivers/spi/cadence_qspi_apb.c | 119 -------------------------
5 files changed, 1 insertion(+), 133 deletions(-)
diff --git a/arch/arm/dts/versal-mini-ospi.dtsi b/arch/arm/dts/versal-mini-ospi.dtsi
index eec2a08e7c70..6991f6a51db0 100644
--- a/arch/arm/dts/versal-mini-ospi.dtsi
+++ b/arch/arm/dts/versal-mini-ospi.dtsi
@@ -38,7 +38,6 @@
num-cs = <1>;
cdns,fifo-depth = <256>;
cdns,fifo-width = <4>;
- cdns,is-dma = <1>;
cdns,trigger-address = <0xc0000000>;
#address-cells = <1>;
#size-cells = <0>;
diff --git a/arch/arm/dts/versal-net-mini-ospi.dtsi b/arch/arm/dts/versal-net-mini-ospi.dtsi
index 1c94b352dc97..d2d5ec8e5cbf 100644
--- a/arch/arm/dts/versal-net-mini-ospi.dtsi
+++ b/arch/arm/dts/versal-net-mini-ospi.dtsi
@@ -52,7 +52,6 @@
num-cs = <1>;
cdns,fifo-depth = <256>;
cdns,fifo-width = <4>;
- cdns,is-dma = <1>;
cdns,is-stig-pgm = <1>;
cdns,trigger-address = <0xc0000000>;
#address-cells = <1>;
diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
index 599596f9f087..849bd930edf0 100644
--- a/drivers/spi/cadence_qspi.c
+++ b/drivers/spi/cadence_qspi.c
@@ -210,7 +210,6 @@ static int cadence_spi_probe(struct udevice *bus)
priv->regbase = plat->regbase;
priv->ahbbase = plat->ahbbase;
- priv->is_dma = plat->is_dma;
priv->is_decoded_cs = plat->is_decoded_cs;
priv->fifo_depth = plat->fifo_depth;
priv->fifo_width = plat->fifo_width;
@@ -348,10 +347,7 @@ static int cadence_spi_mem_exec_op(struct spi_slave *spi,
case CQSPI_READ:
err = cadence_qspi_apb_read_setup(priv, op);
if (!err) {
- if (priv->is_dma)
- err = cadence_qspi_apb_dma_read(priv, op);
- else
- err = cadence_qspi_apb_read_execute(priv, op);
+ err = cadence_qspi_apb_dma_read(priv, op);
}
break;
case CQSPI_WRITE:
@@ -412,8 +408,6 @@ static int cadence_spi_of_to_plat(struct udevice *bus)
if (plat->ahbsize >= SZ_8M)
priv->use_dac_mode = true;
- plat->is_dma = dev_read_bool(bus, "cdns,is-dma");
-
/* All other parameters are embedded in the child node */
subnode = cadence_qspi_get_subnode(bus);
if (!ofnode_valid(subnode)) {
diff --git a/drivers/spi/cadence_qspi.h b/drivers/spi/cadence_qspi.h
index 879e7f8dbfb8..10c4ad14cc05 100644
--- a/drivers/spi/cadence_qspi.h
+++ b/drivers/spi/cadence_qspi.h
@@ -223,8 +223,6 @@ struct cadence_spi_plat {
u32 tchsh_ns;
u32 tslch_ns;
u32 quirks;
-
- bool is_dma;
};
struct cadence_spi_priv {
@@ -261,7 +259,6 @@ struct cadence_spi_priv {
bool ddr_init;
bool is_decoded_cs;
bool use_dac_mode;
- bool is_dma;
/* Transaction protocol parameters. */
u8 inst_width;
@@ -292,8 +289,6 @@ int cadence_qspi_apb_command_write(struct cadence_spi_priv *priv,
int cadence_qspi_apb_read_setup(struct cadence_spi_priv *priv,
const struct spi_mem_op *op);
-int cadence_qspi_apb_read_execute(struct cadence_spi_priv *priv,
- const struct spi_mem_op *op);
int cadence_qspi_apb_write_setup(struct cadence_spi_priv *priv,
const struct spi_mem_op *op);
int cadence_qspi_apb_write_execute(struct cadence_spi_priv *priv,
diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
index 4696c09f754b..58be017720bc 100644
--- a/drivers/spi/cadence_qspi_apb.c
+++ b/drivers/spi/cadence_qspi_apb.c
@@ -667,125 +667,6 @@ int cadence_qspi_apb_read_setup(struct cadence_spi_priv *priv,
return 0;
}
-static u32 cadence_qspi_get_rd_sram_level(struct cadence_spi_priv *priv)
-{
- u32 reg = readl(priv->regbase + CQSPI_REG_SDRAMLEVEL);
- reg >>= CQSPI_REG_SDRAMLEVEL_RD_LSB;
- return reg & CQSPI_REG_SDRAMLEVEL_RD_MASK;
-}
-
-static int cadence_qspi_wait_for_data(struct cadence_spi_priv *priv)
-{
- unsigned int timeout = 10000;
- u32 reg;
-
- while (timeout--) {
- reg = cadence_qspi_get_rd_sram_level(priv);
- if (reg)
- return reg;
- udelay(1);
- }
-
- return -ETIMEDOUT;
-}
-
-static int
-cadence_qspi_apb_indirect_read_execute(struct cadence_spi_priv *priv,
- unsigned int n_rx, u8 *rxbuf)
-{
- unsigned int remaining = n_rx;
- unsigned int bytes_to_read = 0;
- int ret;
-
- writel(n_rx, priv->regbase + CQSPI_REG_INDIRECTRDBYTES);
-
- /* Start the indirect read transfer */
- writel(CQSPI_REG_INDIRECTRD_START,
- priv->regbase + CQSPI_REG_INDIRECTRD);
-
- while (remaining > 0) {
- ret = cadence_qspi_wait_for_data(priv);
- if (ret < 0) {
- printf("Indirect write timed out (%i)\n", ret);
- goto failrd;
- }
-
- bytes_to_read = ret;
-
- while (bytes_to_read != 0) {
- bytes_to_read *= priv->fifo_width;
- bytes_to_read = bytes_to_read > remaining ?
- remaining : bytes_to_read;
- /*
- * Handle non-4-byte aligned access to avoid
- * data abort.
- */
- if (((uintptr_t)rxbuf % 4) || (bytes_to_read % 4))
- readsb(priv->ahbbase, rxbuf, bytes_to_read);
- else
- readsl(priv->ahbbase, rxbuf,
- bytes_to_read >> 2);
- rxbuf += bytes_to_read;
- remaining -= bytes_to_read;
- bytes_to_read = cadence_qspi_get_rd_sram_level(priv);
- }
- }
-
- /* Check indirect done status */
- ret = wait_for_bit_le32(priv->regbase + CQSPI_REG_INDIRECTRD,
- CQSPI_REG_INDIRECTRD_DONE, 1, 10, 0);
- if (ret) {
- printf("Indirect read completion error (%i)\n", ret);
- goto failrd;
- }
-
- /* Clear indirect completion status */
- writel(CQSPI_REG_INDIRECTRD_DONE,
- priv->regbase + CQSPI_REG_INDIRECTRD);
-
- /* Check indirect done status */
- ret = wait_for_bit_le32(priv->regbase + CQSPI_REG_INDIRECTRD,
- CQSPI_REG_INDIRECTRD_DONE, 0, 10, 0);
- if (ret) {
- printf("Indirect read clear completion error (%i)\n", ret);
- goto failrd;
- }
-
- /* Wait til QSPI is idle */
- if (!cadence_qspi_wait_idle(priv->regbase))
- return -EIO;
-
- return 0;
-
-failrd:
- /* Cancel the indirect read */
- writel(CQSPI_REG_INDIRECTRD_CANCEL,
- priv->regbase + CQSPI_REG_INDIRECTRD);
- return ret;
-}
-
-int cadence_qspi_apb_read_execute(struct cadence_spi_priv *priv,
- const struct spi_mem_op *op)
-{
- u64 from = op->addr.val;
- void *buf = op->data.buf.in;
- size_t len = op->data.nbytes;
-
- cadence_qspi_apb_enable_linear_mode(true);
-
- if (priv->use_dac_mode && (from + len < priv->ahbsize)) {
- if (len < 256 ||
- dma_memcpy(buf, priv->ahbbase + from, len) < 0) {
- memcpy_fromio(buf, priv->ahbbase + from, len);
- }
- if (!cadence_qspi_wait_idle(priv->regbase))
- return -EIO;
- return 0;
- }
-
- return cadence_qspi_apb_indirect_read_execute(priv, len, buf);
-}
-
/* Opcode + Address (3/4 bytes) */
int cadence_qspi_apb_write_setup(struct cadence_spi_priv *priv,
const struct spi_mem_op *op)
--
2.43.0
base-commit: 0cb6970639ee9d3fcff23372d3adfb77e1c5f4b9
branch: debian-sent4444
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] spi: cadence-qspi: Remove cdns,is-dma property handling
2025-10-09 10:34 [PATCH v2] spi: cadence-qspi: Remove cdns,is-dma property handling Michal Simek
@ 2025-10-19 22:03 ` E Shattow
2025-10-19 22:36 ` Ronald Wahl
0 siblings, 1 reply; 8+ messages in thread
From: E Shattow @ 2025-10-19 22:03 UTC (permalink / raw)
To: Michal Simek, u-boot, git
Cc: Boon Khai Ng, Jagan Teki, Naman Trivedi, Naresh Kumar Ravulapalli,
Neha Malcom Francis, Prasad Kummari, Prasanth Babu Mantena,
Ronald Wahl, Tejas Bhumkar, Tien Fong Chee, Tom Rini,
Venkatesh Yadav Abbarapu
Hi Michal,
On 10/9/25 03:34, Michal Simek wrote:
> Remove cdns,is-dma DT property handling. Property is not the part of DT
> binding and it is also hardcoded to value 1 in all DTs that's why remove it
> because none is also testing value 0.
> If there is any use case when this configuration should be supported this
> patch can be reverted.
>
> Signed-off-by: Michal Simek <michal.simek@amd.com>
> ---
>
> Changes in v2:
> - Remove other functions which are reported as unused by gitlab CI.
>
> v1: https://lore.kernel.org/r/c6a5b4bfe565668eb3299d5c5a5548a2163b129a.1758786568.git.michal.simek@amd.com
> ---
> arch/arm/dts/versal-mini-ospi.dtsi | 1 -
> arch/arm/dts/versal-net-mini-ospi.dtsi | 1 -
> drivers/spi/cadence_qspi.c | 8 +-
> drivers/spi/cadence_qspi.h | 5 --
> drivers/spi/cadence_qspi_apb.c | 119 -------------------------
> 5 files changed, 1 insertion(+), 133 deletions(-)
>
> diff --git a/arch/arm/dts/versal-mini-ospi.dtsi b/arch/arm/dts/versal-mini-ospi.dtsi
> index eec2a08e7c70..6991f6a51db0 100644
> --- a/arch/arm/dts/versal-mini-ospi.dtsi
> +++ b/arch/arm/dts/versal-mini-ospi.dtsi
> @@ -38,7 +38,6 @@
> num-cs = <1>;
> cdns,fifo-depth = <256>;
> cdns,fifo-width = <4>;
> - cdns,is-dma = <1>;
> cdns,trigger-address = <0xc0000000>;
> #address-cells = <1>;
> #size-cells = <0>;
> diff --git a/arch/arm/dts/versal-net-mini-ospi.dtsi b/arch/arm/dts/versal-net-mini-ospi.dtsi
> index 1c94b352dc97..d2d5ec8e5cbf 100644
> --- a/arch/arm/dts/versal-net-mini-ospi.dtsi
> +++ b/arch/arm/dts/versal-net-mini-ospi.dtsi
> @@ -52,7 +52,6 @@
> num-cs = <1>;
> cdns,fifo-depth = <256>;
> cdns,fifo-width = <4>;
> - cdns,is-dma = <1>;
> cdns,is-stig-pgm = <1>;
> cdns,trigger-address = <0xc0000000>;
> #address-cells = <1>;
> diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
> index 599596f9f087..849bd930edf0 100644
> --- a/drivers/spi/cadence_qspi.c
> +++ b/drivers/spi/cadence_qspi.c
> @@ -210,7 +210,6 @@ static int cadence_spi_probe(struct udevice *bus)
>
> priv->regbase = plat->regbase;
> priv->ahbbase = plat->ahbbase;
> - priv->is_dma = plat->is_dma;
> priv->is_decoded_cs = plat->is_decoded_cs;
> priv->fifo_depth = plat->fifo_depth;
> priv->fifo_width = plat->fifo_width;
> @@ -348,10 +347,7 @@ static int cadence_spi_mem_exec_op(struct spi_slave *spi,
> case CQSPI_READ:
> err = cadence_qspi_apb_read_setup(priv, op);
> if (!err) {
> - if (priv->is_dma)
> - err = cadence_qspi_apb_dma_read(priv, op);
> - else
> - err = cadence_qspi_apb_read_execute(priv, op);
> + err = cadence_qspi_apb_dma_read(priv, op);
> }
> break;
> case CQSPI_WRITE:
> @@ -412,8 +408,6 @@ static int cadence_spi_of_to_plat(struct udevice *bus)
> if (plat->ahbsize >= SZ_8M)
> priv->use_dac_mode = true;
>
> - plat->is_dma = dev_read_bool(bus, "cdns,is-dma");
> -
> /* All other parameters are embedded in the child node */
> subnode = cadence_qspi_get_subnode(bus);
> if (!ofnode_valid(subnode)) {
> diff --git a/drivers/spi/cadence_qspi.h b/drivers/spi/cadence_qspi.h
> index 879e7f8dbfb8..10c4ad14cc05 100644
> --- a/drivers/spi/cadence_qspi.h
> +++ b/drivers/spi/cadence_qspi.h
> @@ -223,8 +223,6 @@ struct cadence_spi_plat {
> u32 tchsh_ns;
> u32 tslch_ns;
> u32 quirks;
> -
> - bool is_dma;
> };
>
> struct cadence_spi_priv {
> @@ -261,7 +259,6 @@ struct cadence_spi_priv {
> bool ddr_init;
> bool is_decoded_cs;
> bool use_dac_mode;
> - bool is_dma;
>
> /* Transaction protocol parameters. */
> u8 inst_width;
> @@ -292,8 +289,6 @@ int cadence_qspi_apb_command_write(struct cadence_spi_priv *priv,
>
> int cadence_qspi_apb_read_setup(struct cadence_spi_priv *priv,
> const struct spi_mem_op *op);
> -int cadence_qspi_apb_read_execute(struct cadence_spi_priv *priv,
> - const struct spi_mem_op *op);
> int cadence_qspi_apb_write_setup(struct cadence_spi_priv *priv,
> const struct spi_mem_op *op);
> int cadence_qspi_apb_write_execute(struct cadence_spi_priv *priv,
> diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
> index 4696c09f754b..58be017720bc 100644
> --- a/drivers/spi/cadence_qspi_apb.c
> +++ b/drivers/spi/cadence_qspi_apb.c
> @@ -667,125 +667,6 @@ int cadence_qspi_apb_read_setup(struct cadence_spi_priv *priv,
> return 0;
> }
>
> -static u32 cadence_qspi_get_rd_sram_level(struct cadence_spi_priv *priv)
> -{
> - u32 reg = readl(priv->regbase + CQSPI_REG_SDRAMLEVEL);
> - reg >>= CQSPI_REG_SDRAMLEVEL_RD_LSB;
> - return reg & CQSPI_REG_SDRAMLEVEL_RD_MASK;
> -}
> -
> -static int cadence_qspi_wait_for_data(struct cadence_spi_priv *priv)
> -{
> - unsigned int timeout = 10000;
> - u32 reg;
> -
> - while (timeout--) {
> - reg = cadence_qspi_get_rd_sram_level(priv);
> - if (reg)
> - return reg;
> - udelay(1);
> - }
> -
> - return -ETIMEDOUT;
> -}
> -
> -static int
> -cadence_qspi_apb_indirect_read_execute(struct cadence_spi_priv *priv,
> - unsigned int n_rx, u8 *rxbuf)
> -{
> - unsigned int remaining = n_rx;
> - unsigned int bytes_to_read = 0;
> - int ret;
> -
> - writel(n_rx, priv->regbase + CQSPI_REG_INDIRECTRDBYTES);
> -
> - /* Start the indirect read transfer */
> - writel(CQSPI_REG_INDIRECTRD_START,
> - priv->regbase + CQSPI_REG_INDIRECTRD);
> -
> - while (remaining > 0) {
> - ret = cadence_qspi_wait_for_data(priv);
> - if (ret < 0) {
> - printf("Indirect write timed out (%i)\n", ret);
> - goto failrd;
> - }
> -
> - bytes_to_read = ret;
> -
> - while (bytes_to_read != 0) {
> - bytes_to_read *= priv->fifo_width;
> - bytes_to_read = bytes_to_read > remaining ?
> - remaining : bytes_to_read;
> - /*
> - * Handle non-4-byte aligned access to avoid
> - * data abort.
> - */
> - if (((uintptr_t)rxbuf % 4) || (bytes_to_read % 4))
> - readsb(priv->ahbbase, rxbuf, bytes_to_read);
> - else
> - readsl(priv->ahbbase, rxbuf,
> - bytes_to_read >> 2);
> - rxbuf += bytes_to_read;
> - remaining -= bytes_to_read;
> - bytes_to_read = cadence_qspi_get_rd_sram_level(priv);
> - }
> - }
> -
> - /* Check indirect done status */
> - ret = wait_for_bit_le32(priv->regbase + CQSPI_REG_INDIRECTRD,
> - CQSPI_REG_INDIRECTRD_DONE, 1, 10, 0);
> - if (ret) {
> - printf("Indirect read completion error (%i)\n", ret);
> - goto failrd;
> - }
> -
> - /* Clear indirect completion status */
> - writel(CQSPI_REG_INDIRECTRD_DONE,
> - priv->regbase + CQSPI_REG_INDIRECTRD);
> -
> - /* Check indirect done status */
> - ret = wait_for_bit_le32(priv->regbase + CQSPI_REG_INDIRECTRD,
> - CQSPI_REG_INDIRECTRD_DONE, 0, 10, 0);
> - if (ret) {
> - printf("Indirect read clear completion error (%i)\n", ret);
> - goto failrd;
> - }
> -
> - /* Wait til QSPI is idle */
> - if (!cadence_qspi_wait_idle(priv->regbase))
> - return -EIO;
> -
> - return 0;
> -
> -failrd:
> - /* Cancel the indirect read */
> - writel(CQSPI_REG_INDIRECTRD_CANCEL,
> - priv->regbase + CQSPI_REG_INDIRECTRD);
> - return ret;
> -}
> -
> -int cadence_qspi_apb_read_execute(struct cadence_spi_priv *priv,
> - const struct spi_mem_op *op)
> -{
> - u64 from = op->addr.val;
> - void *buf = op->data.buf.in;
> - size_t len = op->data.nbytes;
> -
> - cadence_qspi_apb_enable_linear_mode(true);
> -
> - if (priv->use_dac_mode && (from + len < priv->ahbsize)) {
> - if (len < 256 ||
> - dma_memcpy(buf, priv->ahbbase + from, len) < 0) {
> - memcpy_fromio(buf, priv->ahbbase + from, len);
> - }
> - if (!cadence_qspi_wait_idle(priv->regbase))
> - return -EIO;
> - return 0;
> - }
> -
> - return cadence_qspi_apb_indirect_read_execute(priv, len, buf);
> -}
> -
> /* Opcode + Address (3/4 bytes) */
> int cadence_qspi_apb_write_setup(struct cadence_spi_priv *priv,
> const struct spi_mem_op *op)
Your patch results in corrupted SPI NOR flash writing from U-Boot on
starfive_visionfive2_defconfig target board Pine64 Star64:
a040578d8270ed8788d7663808ea63ce5ffd7840 is the first bad commit
commit a040578d8270ed8788d7663808ea63ce5ffd7840
Author: Michal Simek <michal.simek@amd.com>
Date: Thu Oct 9 12:34:48 2025 +0200
spi: cadence-qspi: Remove cdns,is-dma property handling
Steps to reproduce with Star64:
1). Set JH-7110 SoC RGPIO configured for UART BootROM loader mode
2). Power on and Transfer U-Boot SPL by XMODEM as prompted by BootROM
3). Transfer U-Boot main by YMODEM from U-Boot SPL prompt
4). Having `(cd $HOME/build/u-boot && python3 -m http.server 6789)` or
similar for serving U-Boot SPL and U-Boot Main to save some time with
UART transfer:
4a). wget $loadaddr http://host:6789/spl/u-boot-spl.bin.normal.out && sf
update $loadaddr 0 $filesize
4b). wget $loadaddr http://host:6789/u-boot.itb && sf update $loadaddr
100000 $filesize
Repeat steps 4a and 4b, note that each time all bytes are written and no
bytes are skipped. Written data is wrong and device unsuccessfully boots
in SPI NOR flash BootROM loader configuration showing wrong information
to output.
Please follow up with your analysis of why these changes have this effect.
-E Shattow
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] spi: cadence-qspi: Remove cdns,is-dma property handling
2025-10-19 22:03 ` E Shattow
@ 2025-10-19 22:36 ` Ronald Wahl
2025-10-20 5:51 ` Michal Simek
2025-10-23 17:51 ` Markus Elfring
0 siblings, 2 replies; 8+ messages in thread
From: Ronald Wahl @ 2025-10-19 22:36 UTC (permalink / raw)
To: E Shattow, Michal Simek, u-boot, git
Cc: Boon Khai Ng, Jagan Teki, Naman Trivedi, Naresh Kumar Ravulapalli,
Neha Malcom Francis, Prasad Kummari, Prasanth Babu Mantena,
Tejas Bhumkar, Tien Fong Chee, Tom Rini, Venkatesh Yadav Abbarapu
On 20.10.25 00:03, E Shattow wrote:
> On 10/9/25 03:34, Michal Simek wrote:
>> Remove cdns,is-dma DT property handling. Property is not the part of DT
>> binding and it is also hardcoded to value 1 in all DTs that's why remove it
>> because none is also testing value 0.
>> If there is any use case when this configuration should be supported this
>> patch can be reverted.
>>
>> Signed-off-by: Michal Simek <michal.simek@amd.com>
>> ---
>>
>> Changes in v2:
>> - Remove other functions which are reported as unused by gitlab CI.
>>
>> v1: https://lore.kernel.org/r/c6a5b4bfe565668eb3299d5c5a5548a2163b129a.1758786568.git.michal.simek@amd.com>>> ---
>> arch/arm/dts/versal-mini-ospi.dtsi | 1 -
>> arch/arm/dts/versal-net-mini-ospi.dtsi | 1 -
>> drivers/spi/cadence_qspi.c | 8 +-
>> drivers/spi/cadence_qspi.h | 5 --
>> drivers/spi/cadence_qspi_apb.c | 119 -------------------------
>> 5 files changed, 1 insertion(+), 133 deletions(-)
>>
>> diff --git a/arch/arm/dts/versal-mini-ospi.dtsi b/arch/arm/dts/versal-mini-ospi.dtsi
>> index eec2a08e7c70..6991f6a51db0 100644
>> --- a/arch/arm/dts/versal-mini-ospi.dtsi
>> +++ b/arch/arm/dts/versal-mini-ospi.dtsi
>> @@ -38,7 +38,6 @@
>> num-cs = <1>;
>> cdns,fifo-depth = <256>;
>> cdns,fifo-width = <4>;
>> - cdns,is-dma = <1>;
>> cdns,trigger-address = <0xc0000000>;
>> #address-cells = <1>;
>> #size-cells = <0>;
>> diff --git a/arch/arm/dts/versal-net-mini-ospi.dtsi b/arch/arm/dts/versal-net-mini-ospi.dtsi
>> index 1c94b352dc97..d2d5ec8e5cbf 100644
>> --- a/arch/arm/dts/versal-net-mini-ospi.dtsi
>> +++ b/arch/arm/dts/versal-net-mini-ospi.dtsi
>> @@ -52,7 +52,6 @@
>> num-cs = <1>;
>> cdns,fifo-depth = <256>;
>> cdns,fifo-width = <4>;
>> - cdns,is-dma = <1>;
>> cdns,is-stig-pgm = <1>;
>> cdns,trigger-address = <0xc0000000>;
>> #address-cells = <1>;
>> diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
>> index 599596f9f087..849bd930edf0 100644
>> --- a/drivers/spi/cadence_qspi.c
>> +++ b/drivers/spi/cadence_qspi.c
>> @@ -210,7 +210,6 @@ static int cadence_spi_probe(struct udevice *bus)
>>
>> priv->regbase = plat->regbase;
>> priv->ahbbase = plat->ahbbase;
>> - priv->is_dma = plat->is_dma;
>> priv->is_decoded_cs = plat->is_decoded_cs;
>> priv->fifo_depth = plat->fifo_depth;
>> priv->fifo_width = plat->fifo_width;
>> @@ -348,10 +347,7 @@ static int cadence_spi_mem_exec_op(struct spi_slave *spi,
>> case CQSPI_READ:
>> err = cadence_qspi_apb_read_setup(priv, op);
>> if (!err) {
>> - if (priv->is_dma)
>> - err = cadence_qspi_apb_dma_read(priv, op);
>> - else
>> - err = cadence_qspi_apb_read_execute(priv, op);
>> + err = cadence_qspi_apb_dma_read(priv, op);
>> }
>> break;
>> case CQSPI_WRITE:
>> @@ -412,8 +408,6 @@ static int cadence_spi_of_to_plat(struct udevice *bus)
>> if (plat->ahbsize >= SZ_8M)
>> priv->use_dac_mode = true;
>>
>> - plat->is_dma = dev_read_bool(bus, "cdns,is-dma");
>> -
>> /* All other parameters are embedded in the child node */
>> subnode = cadence_qspi_get_subnode(bus);
>> if (!ofnode_valid(subnode)) {
>> diff --git a/drivers/spi/cadence_qspi.h b/drivers/spi/cadence_qspi.h
>> index 879e7f8dbfb8..10c4ad14cc05 100644
>> --- a/drivers/spi/cadence_qspi.h
>> +++ b/drivers/spi/cadence_qspi.h
>> @@ -223,8 +223,6 @@ struct cadence_spi_plat {
>> u32 tchsh_ns;
>> u32 tslch_ns;
>> u32 quirks;
>> -
>> - bool is_dma;
>> };
>>
>> struct cadence_spi_priv {
>> @@ -261,7 +259,6 @@ struct cadence_spi_priv {
>> bool ddr_init;
>> bool is_decoded_cs;
>> bool use_dac_mode;
>> - bool is_dma;
>>
>> /* Transaction protocol parameters. */
>> u8 inst_width;
>> @@ -292,8 +289,6 @@ int cadence_qspi_apb_command_write(struct cadence_spi_priv *priv,
>>
>> int cadence_qspi_apb_read_setup(struct cadence_spi_priv *priv,
>> const struct spi_mem_op *op);
>> -int cadence_qspi_apb_read_execute(struct cadence_spi_priv *priv,
>> - const struct spi_mem_op *op);
>> int cadence_qspi_apb_write_setup(struct cadence_spi_priv *priv,
>> const struct spi_mem_op *op);
>> int cadence_qspi_apb_write_execute(struct cadence_spi_priv *priv,
>> diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
>> index 4696c09f754b..58be017720bc 100644
>> --- a/drivers/spi/cadence_qspi_apb.c
>> +++ b/drivers/spi/cadence_qspi_apb.c
>> @@ -667,125 +667,6 @@ int cadence_qspi_apb_read_setup(struct cadence_spi_priv *priv,
>> return 0;
>> }
>>
>> -static u32 cadence_qspi_get_rd_sram_level(struct cadence_spi_priv *priv)
>> -{
>> - u32 reg = readl(priv->regbase + CQSPI_REG_SDRAMLEVEL);
>> - reg >>= CQSPI_REG_SDRAMLEVEL_RD_LSB;
>> - return reg & CQSPI_REG_SDRAMLEVEL_RD_MASK;
>> -}
>> -
>> -static int cadence_qspi_wait_for_data(struct cadence_spi_priv *priv)
>> -{
>> - unsigned int timeout = 10000;
>> - u32 reg;
>> -
>> - while (timeout--) {
>> - reg = cadence_qspi_get_rd_sram_level(priv);
>> - if (reg)
>> - return reg;
>> - udelay(1);
>> - }
>> -
>> - return -ETIMEDOUT;
>> -}
>> -
>> -static int
>> -cadence_qspi_apb_indirect_read_execute(struct cadence_spi_priv *priv,
>> - unsigned int n_rx, u8 *rxbuf)
>> -{
>> - unsigned int remaining = n_rx;
>> - unsigned int bytes_to_read = 0;
>> - int ret;
>> -
>> - writel(n_rx, priv->regbase + CQSPI_REG_INDIRECTRDBYTES);
>> -
>> - /* Start the indirect read transfer */
>> - writel(CQSPI_REG_INDIRECTRD_START,
>> - priv->regbase + CQSPI_REG_INDIRECTRD);
>> -
>> - while (remaining > 0) {
>> - ret = cadence_qspi_wait_for_data(priv);
>> - if (ret < 0) {
>> - printf("Indirect write timed out (%i)\n", ret);
>> - goto failrd;
>> - }
>> -
>> - bytes_to_read = ret;
>> -
>> - while (bytes_to_read != 0) {
>> - bytes_to_read *= priv->fifo_width;
>> - bytes_to_read = bytes_to_read > remaining ?
>> - remaining : bytes_to_read;
>> - /*
>> - * Handle non-4-byte aligned access to avoid
>> - * data abort.
>> - */
>> - if (((uintptr_t)rxbuf % 4) || (bytes_to_read % 4))
>> - readsb(priv->ahbbase, rxbuf, bytes_to_read);
>> - else
>> - readsl(priv->ahbbase, rxbuf,
>> - bytes_to_read >> 2);
>> - rxbuf += bytes_to_read;
>> - remaining -= bytes_to_read;
>> - bytes_to_read = cadence_qspi_get_rd_sram_level(priv);
>> - }
>> - }
>> -
>> - /* Check indirect done status */
>> - ret = wait_for_bit_le32(priv->regbase + CQSPI_REG_INDIRECTRD,
>> - CQSPI_REG_INDIRECTRD_DONE, 1, 10, 0);
>> - if (ret) {
>> - printf("Indirect read completion error (%i)\n", ret);
>> - goto failrd;
>> - }
>> -
>> - /* Clear indirect completion status */
>> - writel(CQSPI_REG_INDIRECTRD_DONE,
>> - priv->regbase + CQSPI_REG_INDIRECTRD);
>> -
>> - /* Check indirect done status */
>> - ret = wait_for_bit_le32(priv->regbase + CQSPI_REG_INDIRECTRD,
>> - CQSPI_REG_INDIRECTRD_DONE, 0, 10, 0);
>> - if (ret) {
>> - printf("Indirect read clear completion error (%i)\n", ret);
>> - goto failrd;
>> - }
>> -
>> - /* Wait til QSPI is idle */
>> - if (!cadence_qspi_wait_idle(priv->regbase))
>> - return -EIO;
>> -
>> - return 0;
>> -
>> -failrd:
>> - /* Cancel the indirect read */
>> - writel(CQSPI_REG_INDIRECTRD_CANCEL,
>> - priv->regbase + CQSPI_REG_INDIRECTRD);
>> - return ret;
>> -}
>> -
>> -int cadence_qspi_apb_read_execute(struct cadence_spi_priv *priv,
>> - const struct spi_mem_op *op)
>> -{
>> - u64 from = op->addr.val;
>> - void *buf = op->data.buf.in;
>> - size_t len = op->data.nbytes;
>> -
>> - cadence_qspi_apb_enable_linear_mode(true);
>> -
>> - if (priv->use_dac_mode && (from + len < priv->ahbsize)) {
>> - if (len < 256 ||
>> - dma_memcpy(buf, priv->ahbbase + from, len) < 0) {
>> - memcpy_fromio(buf, priv->ahbbase + from, len);
>> - }
>> - if (!cadence_qspi_wait_idle(priv->regbase))
>> - return -EIO;
>> - return 0;
>> - }
>> -
>> - return cadence_qspi_apb_indirect_read_execute(priv, len, buf);
>> -}
>> -
>> /* Opcode + Address (3/4 bytes) */
>> int cadence_qspi_apb_write_setup(struct cadence_spi_priv *priv,
>> const struct spi_mem_op *op)
>
> Your patch results in corrupted SPI NOR flash writing from U-Boot on
> starfive_visionfive2_defconfig target board Pine64 Star64:
>
> a040578d8270ed8788d7663808ea63ce5ffd7840 is the first bad commit
> commit a040578d8270ed8788d7663808ea63ce5ffd7840
> Author: Michal Simek <michal.simek@amd.com>
> Date: Thu Oct 9 12:34:48 2025 +0200
>
> spi: cadence-qspi: Remove cdns,is-dma property handling
>
> Steps to reproduce with Star64:
>
> 1). Set JH-7110 SoC RGPIO configured for UART BootROM loader mode
> 2). Power on and Transfer U-Boot SPL by XMODEM as prompted by BootROM
> 3). Transfer U-Boot main by YMODEM from U-Boot SPL prompt
> 4). Having `(cd $HOME/build/u-boot && python3 -m http.server 6789)` or
> similar for serving U-Boot SPL and U-Boot Main to save some time with
> UART transfer:
> 4a). wget $loadaddr http://host:6789/spl/u-boot-spl.bin.normal.out && sf
> update $loadaddr 0 $filesize
> 4b). wget $loadaddr http://host:6789/u-boot.itb && sf update $loadaddr
> 100000 $filesize
>
> Repeat steps 4a and 4b, note that each time all bytes are written and no
> bytes are skipped. Written data is wrong and device unsuccessfully boots
> in SPI NOR flash BootROM loader configuration showing wrong information
> to output.
>
> Please follow up with your analysis of why these changes have this effect.
I guess this is because the committer didn't take into account that when
a DT did not mention the property then it is 0 (false).
I assume TI K3 hardware is affected as well. So I think the patch must
be reverted.
- ron
________________________________
Ce message, ainsi que tous les fichiers joints à ce message, peuvent contenir des informations sensibles et/ ou confidentielles ne devant pas être divulguées. Si vous n'êtes pas le destinataire de ce message (ou que vous recevez ce message par erreur), nous vous remercions de le notifier immédiatement à son expéditeur, et de détruire ce message. Toute copie, divulgation, modification, utilisation ou diffusion, non autorisée, directe ou indirecte, de tout ou partie de ce message, est strictement interdite.
This e-mail, and any document attached hereby, may contain confidential and/or privileged information. If you are not the intended recipient (or have received this e-mail in error) please notify the sender immediately and destroy this e-mail. Any unauthorized, direct or indirect, copying, disclosure, distribution or other use of the material or parts thereof is strictly forbidden.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] spi: cadence-qspi: Remove cdns,is-dma property handling
2025-10-19 22:36 ` Ronald Wahl
@ 2025-10-20 5:51 ` Michal Simek
2025-10-20 8:30 ` Ronald Wahl
2025-10-23 17:51 ` Markus Elfring
1 sibling, 1 reply; 8+ messages in thread
From: Michal Simek @ 2025-10-20 5:51 UTC (permalink / raw)
To: Ronald Wahl, E Shattow, u-boot, git
Cc: Boon Khai Ng, Jagan Teki, Naman Trivedi, Naresh Kumar Ravulapalli,
Neha Malcom Francis, Prasad Kummari, Prasanth Babu Mantena,
Tejas Bhumkar, Tien Fong Chee, Tom Rini, Venkatesh Yadav Abbarapu
Hi,
On 10/20/25 00:36, Ronald Wahl wrote:
> On 20.10.25 00:03, E Shattow wrote:
>> On 10/9/25 03:34, Michal Simek wrote:
>>> Remove cdns,is-dma DT property handling. Property is not the part of DT
>>> binding and it is also hardcoded to value 1 in all DTs that's why remove it
>>> because none is also testing value 0.
>>> If there is any use case when this configuration should be supported this
>>> patch can be reverted.
>>>
>>> Signed-off-by: Michal Simek <michal.simek@amd.com>
>>> ---
>>>
>>> Changes in v2:
>>> - Remove other functions which are reported as unused by gitlab CI.
>>>
>>> v1: https://lore.kernel.org/r/
>>> c6a5b4bfe565668eb3299d5c5a5548a2163b129a.1758786568.git.michal.simek@amd.com>>> ---
>>> arch/arm/dts/versal-mini-ospi.dtsi | 1 -
>>> arch/arm/dts/versal-net-mini-ospi.dtsi | 1 -
>>> drivers/spi/cadence_qspi.c | 8 +-
>>> drivers/spi/cadence_qspi.h | 5 --
>>> drivers/spi/cadence_qspi_apb.c | 119 -------------------------
>>> 5 files changed, 1 insertion(+), 133 deletions(-)
>>>
>>> diff --git a/arch/arm/dts/versal-mini-ospi.dtsi b/arch/arm/dts/versal-mini-
>>> ospi.dtsi
>>> index eec2a08e7c70..6991f6a51db0 100644
>>> --- a/arch/arm/dts/versal-mini-ospi.dtsi
>>> +++ b/arch/arm/dts/versal-mini-ospi.dtsi
>>> @@ -38,7 +38,6 @@
>>> num-cs = <1>;
>>> cdns,fifo-depth = <256>;
>>> cdns,fifo-width = <4>;
>>> - cdns,is-dma = <1>;
>>> cdns,trigger-address = <0xc0000000>;
>>> #address-cells = <1>;
>>> #size-cells = <0>;
>>> diff --git a/arch/arm/dts/versal-net-mini-ospi.dtsi b/arch/arm/dts/versal-
>>> net-mini-ospi.dtsi
>>> index 1c94b352dc97..d2d5ec8e5cbf 100644
>>> --- a/arch/arm/dts/versal-net-mini-ospi.dtsi
>>> +++ b/arch/arm/dts/versal-net-mini-ospi.dtsi
>>> @@ -52,7 +52,6 @@
>>> num-cs = <1>;
>>> cdns,fifo-depth = <256>;
>>> cdns,fifo-width = <4>;
>>> - cdns,is-dma = <1>;
>>> cdns,is-stig-pgm = <1>;
>>> cdns,trigger-address = <0xc0000000>;
>>> #address-cells = <1>;
>>> diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
>>> index 599596f9f087..849bd930edf0 100644
>>> --- a/drivers/spi/cadence_qspi.c
>>> +++ b/drivers/spi/cadence_qspi.c
>>> @@ -210,7 +210,6 @@ static int cadence_spi_probe(struct udevice *bus)
>>>
>>> priv->regbase = plat->regbase;
>>> priv->ahbbase = plat->ahbbase;
>>> - priv->is_dma = plat->is_dma;
>>> priv->is_decoded_cs = plat->is_decoded_cs;
>>> priv->fifo_depth = plat->fifo_depth;
>>> priv->fifo_width = plat->fifo_width;
>>> @@ -348,10 +347,7 @@ static int cadence_spi_mem_exec_op(struct spi_slave *spi,
>>> case CQSPI_READ:
>>> err = cadence_qspi_apb_read_setup(priv, op);
>>> if (!err) {
>>> - if (priv->is_dma)
>>> - err = cadence_qspi_apb_dma_read(priv, op);
>>> - else
>>> - err = cadence_qspi_apb_read_execute(priv, op);
>>> + err = cadence_qspi_apb_dma_read(priv, op);
>>> }
>>> break;
>>> case CQSPI_WRITE:
>>> @@ -412,8 +408,6 @@ static int cadence_spi_of_to_plat(struct udevice *bus)
>>> if (plat->ahbsize >= SZ_8M)
>>> priv->use_dac_mode = true;
>>>
>>> - plat->is_dma = dev_read_bool(bus, "cdns,is-dma");
>>> -
>>> /* All other parameters are embedded in the child node */
>>> subnode = cadence_qspi_get_subnode(bus);
>>> if (!ofnode_valid(subnode)) {
>>> diff --git a/drivers/spi/cadence_qspi.h b/drivers/spi/cadence_qspi.h
>>> index 879e7f8dbfb8..10c4ad14cc05 100644
>>> --- a/drivers/spi/cadence_qspi.h
>>> +++ b/drivers/spi/cadence_qspi.h
>>> @@ -223,8 +223,6 @@ struct cadence_spi_plat {
>>> u32 tchsh_ns;
>>> u32 tslch_ns;
>>> u32 quirks;
>>> -
>>> - bool is_dma;
>>> };
>>>
>>> struct cadence_spi_priv {
>>> @@ -261,7 +259,6 @@ struct cadence_spi_priv {
>>> bool ddr_init;
>>> bool is_decoded_cs;
>>> bool use_dac_mode;
>>> - bool is_dma;
>>>
>>> /* Transaction protocol parameters. */
>>> u8 inst_width;
>>> @@ -292,8 +289,6 @@ int cadence_qspi_apb_command_write(struct
>>> cadence_spi_priv *priv,
>>>
>>> int cadence_qspi_apb_read_setup(struct cadence_spi_priv *priv,
>>> const struct spi_mem_op *op);
>>> -int cadence_qspi_apb_read_execute(struct cadence_spi_priv *priv,
>>> - const struct spi_mem_op *op);
>>> int cadence_qspi_apb_write_setup(struct cadence_spi_priv *priv,
>>> const struct spi_mem_op *op);
>>> int cadence_qspi_apb_write_execute(struct cadence_spi_priv *priv,
>>> diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
>>> index 4696c09f754b..58be017720bc 100644
>>> --- a/drivers/spi/cadence_qspi_apb.c
>>> +++ b/drivers/spi/cadence_qspi_apb.c
>>> @@ -667,125 +667,6 @@ int cadence_qspi_apb_read_setup(struct cadence_spi_priv
>>> *priv,
>>> return 0;
>>> }
>>>
>>> -static u32 cadence_qspi_get_rd_sram_level(struct cadence_spi_priv *priv)
>>> -{
>>> - u32 reg = readl(priv->regbase + CQSPI_REG_SDRAMLEVEL);
>>> - reg >>= CQSPI_REG_SDRAMLEVEL_RD_LSB;
>>> - return reg & CQSPI_REG_SDRAMLEVEL_RD_MASK;
>>> -}
>>> -
>>> -static int cadence_qspi_wait_for_data(struct cadence_spi_priv *priv)
>>> -{
>>> - unsigned int timeout = 10000;
>>> - u32 reg;
>>> -
>>> - while (timeout--) {
>>> - reg = cadence_qspi_get_rd_sram_level(priv);
>>> - if (reg)
>>> - return reg;
>>> - udelay(1);
>>> - }
>>> -
>>> - return -ETIMEDOUT;
>>> -}
>>> -
>>> -static int
>>> -cadence_qspi_apb_indirect_read_execute(struct cadence_spi_priv *priv,
>>> - unsigned int n_rx, u8 *rxbuf)
>>> -{
>>> - unsigned int remaining = n_rx;
>>> - unsigned int bytes_to_read = 0;
>>> - int ret;
>>> -
>>> - writel(n_rx, priv->regbase + CQSPI_REG_INDIRECTRDBYTES);
>>> -
>>> - /* Start the indirect read transfer */
>>> - writel(CQSPI_REG_INDIRECTRD_START,
>>> - priv->regbase + CQSPI_REG_INDIRECTRD);
>>> -
>>> - while (remaining > 0) {
>>> - ret = cadence_qspi_wait_for_data(priv);
>>> - if (ret < 0) {
>>> - printf("Indirect write timed out (%i)\n", ret);
>>> - goto failrd;
>>> - }
>>> -
>>> - bytes_to_read = ret;
>>> -
>>> - while (bytes_to_read != 0) {
>>> - bytes_to_read *= priv->fifo_width;
>>> - bytes_to_read = bytes_to_read > remaining ?
>>> - remaining : bytes_to_read;
>>> - /*
>>> - * Handle non-4-byte aligned access to avoid
>>> - * data abort.
>>> - */
>>> - if (((uintptr_t)rxbuf % 4) || (bytes_to_read % 4))
>>> - readsb(priv->ahbbase, rxbuf, bytes_to_read);
>>> - else
>>> - readsl(priv->ahbbase, rxbuf,
>>> - bytes_to_read >> 2);
>>> - rxbuf += bytes_to_read;
>>> - remaining -= bytes_to_read;
>>> - bytes_to_read = cadence_qspi_get_rd_sram_level(priv);
>>> - }
>>> - }
>>> -
>>> - /* Check indirect done status */
>>> - ret = wait_for_bit_le32(priv->regbase + CQSPI_REG_INDIRECTRD,
>>> - CQSPI_REG_INDIRECTRD_DONE, 1, 10, 0);
>>> - if (ret) {
>>> - printf("Indirect read completion error (%i)\n", ret);
>>> - goto failrd;
>>> - }
>>> -
>>> - /* Clear indirect completion status */
>>> - writel(CQSPI_REG_INDIRECTRD_DONE,
>>> - priv->regbase + CQSPI_REG_INDIRECTRD);
>>> -
>>> - /* Check indirect done status */
>>> - ret = wait_for_bit_le32(priv->regbase + CQSPI_REG_INDIRECTRD,
>>> - CQSPI_REG_INDIRECTRD_DONE, 0, 10, 0);
>>> - if (ret) {
>>> - printf("Indirect read clear completion error (%i)\n", ret);
>>> - goto failrd;
>>> - }
>>> -
>>> - /* Wait til QSPI is idle */
>>> - if (!cadence_qspi_wait_idle(priv->regbase))
>>> - return -EIO;
>>> -
>>> - return 0;
>>> -
>>> -failrd:
>>> - /* Cancel the indirect read */
>>> - writel(CQSPI_REG_INDIRECTRD_CANCEL,
>>> - priv->regbase + CQSPI_REG_INDIRECTRD);
>>> - return ret;
>>> -}
>>> -
>>> -int cadence_qspi_apb_read_execute(struct cadence_spi_priv *priv,
>>> - const struct spi_mem_op *op)
>>> -{
>>> - u64 from = op->addr.val;
>>> - void *buf = op->data.buf.in;
>>> - size_t len = op->data.nbytes;
>>> -
>>> - cadence_qspi_apb_enable_linear_mode(true);
>>> -
>>> - if (priv->use_dac_mode && (from + len < priv->ahbsize)) {
>>> - if (len < 256 ||
>>> - dma_memcpy(buf, priv->ahbbase + from, len) < 0) {
>>> - memcpy_fromio(buf, priv->ahbbase + from, len);
>>> - }
>>> - if (!cadence_qspi_wait_idle(priv->regbase))
>>> - return -EIO;
>>> - return 0;
>>> - }
>>> -
>>> - return cadence_qspi_apb_indirect_read_execute(priv, len, buf);
>>> -}
>>> -
>>> /* Opcode + Address (3/4 bytes) */
>>> int cadence_qspi_apb_write_setup(struct cadence_spi_priv *priv,
>>> const struct spi_mem_op *op)
>>
>> Your patch results in corrupted SPI NOR flash writing from U-Boot on
>> starfive_visionfive2_defconfig target board Pine64 Star64:
>>
>> a040578d8270ed8788d7663808ea63ce5ffd7840 is the first bad commit
>> commit a040578d8270ed8788d7663808ea63ce5ffd7840
>> Author: Michal Simek <michal.simek@amd.com>
>> Date: Thu Oct 9 12:34:48 2025 +0200
>>
>> spi: cadence-qspi: Remove cdns,is-dma property handling
>>
>> Steps to reproduce with Star64:
>>
>> 1). Set JH-7110 SoC RGPIO configured for UART BootROM loader mode
>> 2). Power on and Transfer U-Boot SPL by XMODEM as prompted by BootROM
>> 3). Transfer U-Boot main by YMODEM from U-Boot SPL prompt
>> 4). Having `(cd $HOME/build/u-boot && python3 -m http.server 6789)` or
>> similar for serving U-Boot SPL and U-Boot Main to save some time with
>> UART transfer:
>> 4a). wget $loadaddr http://host:6789/spl/u-boot-spl.bin.normal.out && sf
>> update $loadaddr 0 $filesize
>> 4b). wget $loadaddr http://host:6789/u-boot.itb && sf update $loadaddr
>> 100000 $filesize
>>
>> Repeat steps 4a and 4b, note that each time all bytes are written and no
>> bytes are skipped. Written data is wrong and device unsuccessfully boots
>> in SPI NOR flash BootROM loader configuration showing wrong information
>> to output.
>>
>> Please follow up with your analysis of why these changes have this effect.
>
> I guess this is because the committer didn't take into account that when
> a DT did not mention the property then it is 0 (false).
>
> I assume TI K3 hardware is affected as well. So I think the patch must
> be reverted.
Are you using cadence-qspi in non DMA mode? DT property shouldn't be introduced
in the first place that's why it was removed.
Thanks,
Michal
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] spi: cadence-qspi: Remove cdns,is-dma property handling
2025-10-20 5:51 ` Michal Simek
@ 2025-10-20 8:30 ` Ronald Wahl
2025-10-20 9:38 ` Michal Simek
0 siblings, 1 reply; 8+ messages in thread
From: Ronald Wahl @ 2025-10-20 8:30 UTC (permalink / raw)
To: Michal Simek, E Shattow, u-boot, git
Cc: Boon Khai Ng, Jagan Teki, Naman Trivedi, Naresh Kumar Ravulapalli,
Neha Malcom Francis, Prasad Kummari, Prasanth Babu Mantena,
Tejas Bhumkar, Tien Fong Chee, Tom Rini, Venkatesh Yadav Abbarapu
On 20.10.25 07:51, Michal Simek wrote:
> [You don't often get email from michal.simek@amd.com. Learn why this is
> important at https://aka.ms/LearnAboutSenderIdentification ]
> On 10/20/25 00:36, Ronald Wahl wrote:
>> On 20.10.25 00:03, E Shattow wrote:
>>> On 10/9/25 03:34, Michal Simek wrote:
>>>> Remove cdns,is-dma DT property handling. Property is not the part of DT
>>>> binding and it is also hardcoded to value 1 in all DTs that's why
>>>> remove it
>>>> because none is also testing value 0.
>>>> If there is any use case when this configuration should be supported
>>>> this
>>>> patch can be reverted.
>>>>
>>>> Signed-off-by: Michal Simek <michal.simek@amd.com>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - Remove other functions which are reported as unused by gitlab CI.
>>>>
>>>> v1: https://eur01.safelinks.protection.outlook.com/?
>>>> url=https%3A%2F%2Flore.kernel.org%2Fr%2F&data=05%7C02%7Cronald.wahl%40legrand.com%7Cedf1107fa8c143a9738c08de0f9cc246%7C199686b5bef4496087867a6b1888fee3%7C0%7C0%7C638965363122564250%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=wloMzuMKR6AGQHJ%2FMurQdgiZrPsQkHvsewR5%2F48TsZM%3D&reserved=0
>>>> c6a5b4bfe565668eb3299d5c5a5548a2163b129a.1758786568.git.michal.simek@amd.com>>> ---
>>>> arch/arm/dts/versal-mini-ospi.dtsi | 1 -
>>>> arch/arm/dts/versal-net-mini-ospi.dtsi | 1 -
>>>> drivers/spi/cadence_qspi.c | 8 +-
>>>> drivers/spi/cadence_qspi.h | 5 --
>>>> drivers/spi/cadence_qspi_apb.c | 119
>>>> -------------------------
>>>> 5 files changed, 1 insertion(+), 133 deletions(-)
>>>>
>>>> diff --git a/arch/arm/dts/versal-mini-ospi.dtsi b/arch/arm/dts/
>>>> versal-mini-
>>>> ospi.dtsi
>>>> index eec2a08e7c70..6991f6a51db0 100644
>>>> --- a/arch/arm/dts/versal-mini-ospi.dtsi
>>>> +++ b/arch/arm/dts/versal-mini-ospi.dtsi
>>>> @@ -38,7 +38,6 @@
>>>> num-cs = <1>;
>>>> cdns,fifo-depth = <256>;
>>>> cdns,fifo-width = <4>;
>>>> - cdns,is-dma = <1>;
>>>> cdns,trigger-address = <0xc0000000>;
>>>> #address-cells = <1>;
>>>> #size-cells = <0>;
>>>> diff --git a/arch/arm/dts/versal-net-mini-ospi.dtsi b/arch/arm/dts/
>>>> versal-
>>>> net-mini-ospi.dtsi
>>>> index 1c94b352dc97..d2d5ec8e5cbf 100644
>>>> --- a/arch/arm/dts/versal-net-mini-ospi.dtsi
>>>> +++ b/arch/arm/dts/versal-net-mini-ospi.dtsi
>>>> @@ -52,7 +52,6 @@
>>>> num-cs = <1>;
>>>> cdns,fifo-depth = <256>;
>>>> cdns,fifo-width = <4>;
>>>> - cdns,is-dma = <1>;
>>>> cdns,is-stig-pgm = <1>;
>>>> cdns,trigger-address = <0xc0000000>;
>>>> #address-cells = <1>;
>>>> diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
>>>> index 599596f9f087..849bd930edf0 100644
>>>> --- a/drivers/spi/cadence_qspi.c
>>>> +++ b/drivers/spi/cadence_qspi.c
>>>> @@ -210,7 +210,6 @@ static int cadence_spi_probe(struct udevice *bus)
>>>>
>>>> priv->regbase = plat->regbase;
>>>> priv->ahbbase = plat->ahbbase;
>>>> - priv->is_dma = plat->is_dma;
>>>> priv->is_decoded_cs = plat->is_decoded_cs;
>>>> priv->fifo_depth = plat->fifo_depth;
>>>> priv->fifo_width = plat->fifo_width;
>>>> @@ -348,10 +347,7 @@ static int cadence_spi_mem_exec_op(struct
>>>> spi_slave *spi,
>>>> case CQSPI_READ:
>>>> err = cadence_qspi_apb_read_setup(priv, op);
>>>> if (!err) {
>>>> - if (priv->is_dma)
>>>> - err = cadence_qspi_apb_dma_read(priv,
>>>> op);
>>>> - else
>>>> - err =
>>>> cadence_qspi_apb_read_execute(priv, op);
>>>> + err = cadence_qspi_apb_dma_read(priv, op);
>>>> }
>>>> break;
>>>> case CQSPI_WRITE:
>>>> @@ -412,8 +408,6 @@ static int cadence_spi_of_to_plat(struct udevice
>>>> *bus)
>>>> if (plat->ahbsize >= SZ_8M)
>>>> priv->use_dac_mode = true;
>>>>
>>>> - plat->is_dma = dev_read_bool(bus, "cdns,is-dma");
>>>> -
>>>> /* All other parameters are embedded in the child node */
>>>> subnode = cadence_qspi_get_subnode(bus);
>>>> if (!ofnode_valid(subnode)) {
>>>> diff --git a/drivers/spi/cadence_qspi.h b/drivers/spi/cadence_qspi.h
>>>> index 879e7f8dbfb8..10c4ad14cc05 100644
>>>> --- a/drivers/spi/cadence_qspi.h
>>>> +++ b/drivers/spi/cadence_qspi.h
>>>> @@ -223,8 +223,6 @@ struct cadence_spi_plat {
>>>> u32 tchsh_ns;
>>>> u32 tslch_ns;
>>>> u32 quirks;
>>>> -
>>>> - bool is_dma;
>>>> };
>>>>
>>>> struct cadence_spi_priv {
>>>> @@ -261,7 +259,6 @@ struct cadence_spi_priv {
>>>> bool ddr_init;
>>>> bool is_decoded_cs;
>>>> bool use_dac_mode;
>>>> - bool is_dma;
>>>>
>>>> /* Transaction protocol parameters. */
>>>> u8 inst_width;
>>>> @@ -292,8 +289,6 @@ int cadence_qspi_apb_command_write(struct
>>>> cadence_spi_priv *priv,
>>>>
>>>> int cadence_qspi_apb_read_setup(struct cadence_spi_priv *priv,
>>>> const struct spi_mem_op *op);
>>>> -int cadence_qspi_apb_read_execute(struct cadence_spi_priv *priv,
>>>> - const struct spi_mem_op *op);
>>>> int cadence_qspi_apb_write_setup(struct cadence_spi_priv *priv,
>>>> const struct spi_mem_op *op);
>>>> int cadence_qspi_apb_write_execute(struct cadence_spi_priv *priv,
>>>> diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/
>>>> cadence_qspi_apb.c
>>>> index 4696c09f754b..58be017720bc 100644
>>>> --- a/drivers/spi/cadence_qspi_apb.c
>>>> +++ b/drivers/spi/cadence_qspi_apb.c
>>>> @@ -667,125 +667,6 @@ int cadence_qspi_apb_read_setup(struct
>>>> cadence_spi_priv
>>>> *priv,
>>>> return 0;
>>>> }
>>>>
>>>> -static u32 cadence_qspi_get_rd_sram_level(struct cadence_spi_priv
>>>> *priv)
>>>> -{
>>>> - u32 reg = readl(priv->regbase + CQSPI_REG_SDRAMLEVEL);
>>>> - reg >>= CQSPI_REG_SDRAMLEVEL_RD_LSB;
>>>> - return reg & CQSPI_REG_SDRAMLEVEL_RD_MASK;
>>>> -}
>>>> -
>>>> -static int cadence_qspi_wait_for_data(struct cadence_spi_priv *priv)
>>>> -{
>>>> - unsigned int timeout = 10000;
>>>> - u32 reg;
>>>> -
>>>> - while (timeout--) {
>>>> - reg = cadence_qspi_get_rd_sram_level(priv);
>>>> - if (reg)
>>>> - return reg;
>>>> - udelay(1);
>>>> - }
>>>> -
>>>> - return -ETIMEDOUT;
>>>> -}
>>>> -
>>>> -static int
>>>> -cadence_qspi_apb_indirect_read_execute(struct cadence_spi_priv *priv,
>>>> - unsigned int n_rx, u8 *rxbuf)
>>>> -{
>>>> - unsigned int remaining = n_rx;
>>>> - unsigned int bytes_to_read = 0;
>>>> - int ret;
>>>> -
>>>> - writel(n_rx, priv->regbase + CQSPI_REG_INDIRECTRDBYTES);
>>>> -
>>>> - /* Start the indirect read transfer */
>>>> - writel(CQSPI_REG_INDIRECTRD_START,
>>>> - priv->regbase + CQSPI_REG_INDIRECTRD);
>>>> -
>>>> - while (remaining > 0) {
>>>> - ret = cadence_qspi_wait_for_data(priv);
>>>> - if (ret < 0) {
>>>> - printf("Indirect write timed out (%i)\n", ret);
>>>> - goto failrd;
>>>> - }
>>>> -
>>>> - bytes_to_read = ret;
>>>> -
>>>> - while (bytes_to_read != 0) {
>>>> - bytes_to_read *= priv->fifo_width;
>>>> - bytes_to_read = bytes_to_read > remaining ?
>>>> - remaining : bytes_to_read;
>>>> - /*
>>>> - * Handle non-4-byte aligned access to avoid
>>>> - * data abort.
>>>> - */
>>>> - if (((uintptr_t)rxbuf % 4) || (bytes_to_read %
>>>> 4))
>>>> - readsb(priv->ahbbase, rxbuf,
>>>> bytes_to_read);
>>>> - else
>>>> - readsl(priv->ahbbase, rxbuf,
>>>> - bytes_to_read >> 2);
>>>> - rxbuf += bytes_to_read;
>>>> - remaining -= bytes_to_read;
>>>> - bytes_to_read =
>>>> cadence_qspi_get_rd_sram_level(priv);
>>>> - }
>>>> - }
>>>> -
>>>> - /* Check indirect done status */
>>>> - ret = wait_for_bit_le32(priv->regbase + CQSPI_REG_INDIRECTRD,
>>>> - CQSPI_REG_INDIRECTRD_DONE, 1, 10, 0);
>>>> - if (ret) {
>>>> - printf("Indirect read completion error (%i)\n", ret);
>>>> - goto failrd;
>>>> - }
>>>> -
>>>> - /* Clear indirect completion status */
>>>> - writel(CQSPI_REG_INDIRECTRD_DONE,
>>>> - priv->regbase + CQSPI_REG_INDIRECTRD);
>>>> -
>>>> - /* Check indirect done status */
>>>> - ret = wait_for_bit_le32(priv->regbase + CQSPI_REG_INDIRECTRD,
>>>> - CQSPI_REG_INDIRECTRD_DONE, 0, 10, 0);
>>>> - if (ret) {
>>>> - printf("Indirect read clear completion error (%i)\n",
>>>> ret);
>>>> - goto failrd;
>>>> - }
>>>> -
>>>> - /* Wait til QSPI is idle */
>>>> - if (!cadence_qspi_wait_idle(priv->regbase))
>>>> - return -EIO;
>>>> -
>>>> - return 0;
>>>> -
>>>> -failrd:
>>>> - /* Cancel the indirect read */
>>>> - writel(CQSPI_REG_INDIRECTRD_CANCEL,
>>>> - priv->regbase + CQSPI_REG_INDIRECTRD);
>>>> - return ret;
>>>> -}
>>>> -
>>>> -int cadence_qspi_apb_read_execute(struct cadence_spi_priv *priv,
>>>> - const struct spi_mem_op *op)
>>>> -{
>>>> - u64 from = op->addr.val;
>>>> - void *buf = op->data.buf.in;
>>>> - size_t len = op->data.nbytes;
>>>> -
>>>> - cadence_qspi_apb_enable_linear_mode(true);
>>>> -
>>>> - if (priv->use_dac_mode && (from + len < priv->ahbsize)) {
>>>> - if (len < 256 ||
>>>> - dma_memcpy(buf, priv->ahbbase + from, len) < 0) {
>>>> - memcpy_fromio(buf, priv->ahbbase + from, len);
>>>> - }
>>>> - if (!cadence_qspi_wait_idle(priv->regbase))
>>>> - return -EIO;
>>>> - return 0;
>>>> - }
>>>> -
>>>> - return cadence_qspi_apb_indirect_read_execute(priv, len, buf);
>>>> -}
>>>> -
>>>> /* Opcode + Address (3/4 bytes) */
>>>> int cadence_qspi_apb_write_setup(struct cadence_spi_priv *priv,
>>>> const struct spi_mem_op *op)
>>>
>>> Your patch results in corrupted SPI NOR flash writing from U-Boot on
>>> starfive_visionfive2_defconfig target board Pine64 Star64:
>>>
>>> a040578d8270ed8788d7663808ea63ce5ffd7840 is the first bad commit
>>> commit a040578d8270ed8788d7663808ea63ce5ffd7840
>>> Author: Michal Simek <michal.simek@amd.com>
>>> Date: Thu Oct 9 12:34:48 2025 +0200
>>>
>>> spi: cadence-qspi: Remove cdns,is-dma property handling
>>>
>>> Steps to reproduce with Star64:
>>>
>>> 1). Set JH-7110 SoC RGPIO configured for UART BootROM loader mode
>>> 2). Power on and Transfer U-Boot SPL by XMODEM as prompted by BootROM
>>> 3). Transfer U-Boot main by YMODEM from U-Boot SPL prompt
>>> 4). Having `(cd $HOME/build/u-boot && python3 -m http.server 6789)` or
>>> similar for serving U-Boot SPL and U-Boot Main to save some time with
>>> UART transfer:
>>> 4a). wget $loadaddr http://host:6789/spl/u-boot-spl.bin.normal.out && sf
>>> update $loadaddr 0 $filesize
>>> 4b). wget $loadaddr http://host:6789/u-boot.itb && sf update $loadaddr
>>> 100000 $filesize
>>>
>>> Repeat steps 4a and 4b, note that each time all bytes are written and no
>>> bytes are skipped. Written data is wrong and device unsuccessfully boots
>>> in SPI NOR flash BootROM loader configuration showing wrong information
>>> to output.
>>>
>>> Please follow up with your analysis of why these changes have this
>>> effect.
>>
>> I guess this is because the committer didn't take into account that when
>> a DT did not mention the property then it is 0 (false).
>>
>> I assume TI K3 hardware is affected as well. So I think the patch must
>> be reverted.
>
> Are you using cadence-qspi in non DMA mode? DT property shouldn't be
> introduced
> in the first place that's why it was removed.
cadence_qspi_apb_dma_read is only implemented for versal HW and otherwise
a NOP. This does not mean that DMA is not used on non-versal HW, it is just
implemented differently.
- ron
________________________________
Ce message, ainsi que tous les fichiers joints à ce message, peuvent contenir des informations sensibles et/ ou confidentielles ne devant pas être divulguées. Si vous n'êtes pas le destinataire de ce message (ou que vous recevez ce message par erreur), nous vous remercions de le notifier immédiatement à son expéditeur, et de détruire ce message. Toute copie, divulgation, modification, utilisation ou diffusion, non autorisée, directe ou indirecte, de tout ou partie de ce message, est strictement interdite.
This e-mail, and any document attached hereby, may contain confidential and/or privileged information. If you are not the intended recipient (or have received this e-mail in error) please notify the sender immediately and destroy this e-mail. Any unauthorized, direct or indirect, copying, disclosure, distribution or other use of the material or parts thereof is strictly forbidden.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] spi: cadence-qspi: Remove cdns,is-dma property handling
2025-10-20 8:30 ` Ronald Wahl
@ 2025-10-20 9:38 ` Michal Simek
2025-10-20 9:53 ` Ronald Wahl
0 siblings, 1 reply; 8+ messages in thread
From: Michal Simek @ 2025-10-20 9:38 UTC (permalink / raw)
To: Ronald Wahl, E Shattow, u-boot, git
Cc: Boon Khai Ng, Jagan Teki, Naman Trivedi, Naresh Kumar Ravulapalli,
Neha Malcom Francis, Prasad Kummari, Prasanth Babu Mantena,
Tejas Bhumkar, Tien Fong Chee, Tom Rini, Venkatesh Yadav Abbarapu
On 10/20/25 10:30, Ronald Wahl wrote:
> On 20.10.25 07:51, Michal Simek wrote:
>> [You don't often get email from michal.simek@amd.com. Learn why this is
>> important at https://aka.ms/LearnAboutSenderIdentification ]
>> On 10/20/25 00:36, Ronald Wahl wrote:
>>> On 20.10.25 00:03, E Shattow wrote:
>>>> On 10/9/25 03:34, Michal Simek wrote:
>>>>> Remove cdns,is-dma DT property handling. Property is not the part of DT
>>>>> binding and it is also hardcoded to value 1 in all DTs that's why
>>>>> remove it
>>>>> because none is also testing value 0.
>>>>> If there is any use case when this configuration should be supported
>>>>> this
>>>>> patch can be reverted.
>>>>>
>>>>> Signed-off-by: Michal Simek <michal.simek@amd.com>
>>>>> ---
>>>>>
>>>>> Changes in v2:
>>>>> - Remove other functions which are reported as unused by gitlab CI.
>>>>>
>>>>> v1: https://eur01.safelinks.protection.outlook.com/?
>>>>> url=https%3A%2F%2Flore.kernel.org%2Fr%2F&data=05%7C02%7Cronald.wahl%40legrand.com%7Cedf1107fa8c143a9738c08de0f9cc246%7C199686b5bef4496087867a6b1888fee3%7C0%7C0%7C638965363122564250%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=wloMzuMKR6AGQHJ%2FMurQdgiZrPsQkHvsewR5%2F48TsZM%3D&reserved=0
>>>>> c6a5b4bfe565668eb3299d5c5a5548a2163b129a.1758786568.git.michal.simek@amd.com>>> ---
>>>>> arch/arm/dts/versal-mini-ospi.dtsi | 1 -
>>>>> arch/arm/dts/versal-net-mini-ospi.dtsi | 1 -
>>>>> drivers/spi/cadence_qspi.c | 8 +-
>>>>> drivers/spi/cadence_qspi.h | 5 --
>>>>> drivers/spi/cadence_qspi_apb.c | 119
>>>>> -------------------------
>>>>> 5 files changed, 1 insertion(+), 133 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/dts/versal-mini-ospi.dtsi b/arch/arm/dts/
>>>>> versal-mini-
>>>>> ospi.dtsi
>>>>> index eec2a08e7c70..6991f6a51db0 100644
>>>>> --- a/arch/arm/dts/versal-mini-ospi.dtsi
>>>>> +++ b/arch/arm/dts/versal-mini-ospi.dtsi
>>>>> @@ -38,7 +38,6 @@
>>>>> num-cs = <1>;
>>>>> cdns,fifo-depth = <256>;
>>>>> cdns,fifo-width = <4>;
>>>>> - cdns,is-dma = <1>;
>>>>> cdns,trigger-address = <0xc0000000>;
>>>>> #address-cells = <1>;
>>>>> #size-cells = <0>;
>>>>> diff --git a/arch/arm/dts/versal-net-mini-ospi.dtsi b/arch/arm/dts/
>>>>> versal-
>>>>> net-mini-ospi.dtsi
>>>>> index 1c94b352dc97..d2d5ec8e5cbf 100644
>>>>> --- a/arch/arm/dts/versal-net-mini-ospi.dtsi
>>>>> +++ b/arch/arm/dts/versal-net-mini-ospi.dtsi
>>>>> @@ -52,7 +52,6 @@
>>>>> num-cs = <1>;
>>>>> cdns,fifo-depth = <256>;
>>>>> cdns,fifo-width = <4>;
>>>>> - cdns,is-dma = <1>;
>>>>> cdns,is-stig-pgm = <1>;
>>>>> cdns,trigger-address = <0xc0000000>;
>>>>> #address-cells = <1>;
>>>>> diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
>>>>> index 599596f9f087..849bd930edf0 100644
>>>>> --- a/drivers/spi/cadence_qspi.c
>>>>> +++ b/drivers/spi/cadence_qspi.c
>>>>> @@ -210,7 +210,6 @@ static int cadence_spi_probe(struct udevice *bus)
>>>>>
>>>>> priv->regbase = plat->regbase;
>>>>> priv->ahbbase = plat->ahbbase;
>>>>> - priv->is_dma = plat->is_dma;
>>>>> priv->is_decoded_cs = plat->is_decoded_cs;
>>>>> priv->fifo_depth = plat->fifo_depth;
>>>>> priv->fifo_width = plat->fifo_width;
>>>>> @@ -348,10 +347,7 @@ static int cadence_spi_mem_exec_op(struct
>>>>> spi_slave *spi,
>>>>> case CQSPI_READ:
>>>>> err = cadence_qspi_apb_read_setup(priv, op);
>>>>> if (!err) {
>>>>> - if (priv->is_dma)
>>>>> - err = cadence_qspi_apb_dma_read(priv,
>>>>> op);
>>>>> - else
>>>>> - err =
>>>>> cadence_qspi_apb_read_execute(priv, op);
>>>>> + err = cadence_qspi_apb_dma_read(priv, op);
>>>>> }
>>>>> break;
>>>>> case CQSPI_WRITE:
>>>>> @@ -412,8 +408,6 @@ static int cadence_spi_of_to_plat(struct udevice
>>>>> *bus)
>>>>> if (plat->ahbsize >= SZ_8M)
>>>>> priv->use_dac_mode = true;
>>>>>
>>>>> - plat->is_dma = dev_read_bool(bus, "cdns,is-dma");
>>>>> -
>>>>> /* All other parameters are embedded in the child node */
>>>>> subnode = cadence_qspi_get_subnode(bus);
>>>>> if (!ofnode_valid(subnode)) {
>>>>> diff --git a/drivers/spi/cadence_qspi.h b/drivers/spi/cadence_qspi.h
>>>>> index 879e7f8dbfb8..10c4ad14cc05 100644
>>>>> --- a/drivers/spi/cadence_qspi.h
>>>>> +++ b/drivers/spi/cadence_qspi.h
>>>>> @@ -223,8 +223,6 @@ struct cadence_spi_plat {
>>>>> u32 tchsh_ns;
>>>>> u32 tslch_ns;
>>>>> u32 quirks;
>>>>> -
>>>>> - bool is_dma;
>>>>> };
>>>>>
>>>>> struct cadence_spi_priv {
>>>>> @@ -261,7 +259,6 @@ struct cadence_spi_priv {
>>>>> bool ddr_init;
>>>>> bool is_decoded_cs;
>>>>> bool use_dac_mode;
>>>>> - bool is_dma;
>>>>>
>>>>> /* Transaction protocol parameters. */
>>>>> u8 inst_width;
>>>>> @@ -292,8 +289,6 @@ int cadence_qspi_apb_command_write(struct
>>>>> cadence_spi_priv *priv,
>>>>>
>>>>> int cadence_qspi_apb_read_setup(struct cadence_spi_priv *priv,
>>>>> const struct spi_mem_op *op);
>>>>> -int cadence_qspi_apb_read_execute(struct cadence_spi_priv *priv,
>>>>> - const struct spi_mem_op *op);
>>>>> int cadence_qspi_apb_write_setup(struct cadence_spi_priv *priv,
>>>>> const struct spi_mem_op *op);
>>>>> int cadence_qspi_apb_write_execute(struct cadence_spi_priv *priv,
>>>>> diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/
>>>>> cadence_qspi_apb.c
>>>>> index 4696c09f754b..58be017720bc 100644
>>>>> --- a/drivers/spi/cadence_qspi_apb.c
>>>>> +++ b/drivers/spi/cadence_qspi_apb.c
>>>>> @@ -667,125 +667,6 @@ int cadence_qspi_apb_read_setup(struct
>>>>> cadence_spi_priv
>>>>> *priv,
>>>>> return 0;
>>>>> }
>>>>>
>>>>> -static u32 cadence_qspi_get_rd_sram_level(struct cadence_spi_priv
>>>>> *priv)
>>>>> -{
>>>>> - u32 reg = readl(priv->regbase + CQSPI_REG_SDRAMLEVEL);
>>>>> - reg >>= CQSPI_REG_SDRAMLEVEL_RD_LSB;
>>>>> - return reg & CQSPI_REG_SDRAMLEVEL_RD_MASK;
>>>>> -}
>>>>> -
>>>>> -static int cadence_qspi_wait_for_data(struct cadence_spi_priv *priv)
>>>>> -{
>>>>> - unsigned int timeout = 10000;
>>>>> - u32 reg;
>>>>> -
>>>>> - while (timeout--) {
>>>>> - reg = cadence_qspi_get_rd_sram_level(priv);
>>>>> - if (reg)
>>>>> - return reg;
>>>>> - udelay(1);
>>>>> - }
>>>>> -
>>>>> - return -ETIMEDOUT;
>>>>> -}
>>>>> -
>>>>> -static int
>>>>> -cadence_qspi_apb_indirect_read_execute(struct cadence_spi_priv *priv,
>>>>> - unsigned int n_rx, u8 *rxbuf)
>>>>> -{
>>>>> - unsigned int remaining = n_rx;
>>>>> - unsigned int bytes_to_read = 0;
>>>>> - int ret;
>>>>> -
>>>>> - writel(n_rx, priv->regbase + CQSPI_REG_INDIRECTRDBYTES);
>>>>> -
>>>>> - /* Start the indirect read transfer */
>>>>> - writel(CQSPI_REG_INDIRECTRD_START,
>>>>> - priv->regbase + CQSPI_REG_INDIRECTRD);
>>>>> -
>>>>> - while (remaining > 0) {
>>>>> - ret = cadence_qspi_wait_for_data(priv);
>>>>> - if (ret < 0) {
>>>>> - printf("Indirect write timed out (%i)\n", ret);
>>>>> - goto failrd;
>>>>> - }
>>>>> -
>>>>> - bytes_to_read = ret;
>>>>> -
>>>>> - while (bytes_to_read != 0) {
>>>>> - bytes_to_read *= priv->fifo_width;
>>>>> - bytes_to_read = bytes_to_read > remaining ?
>>>>> - remaining : bytes_to_read;
>>>>> - /*
>>>>> - * Handle non-4-byte aligned access to avoid
>>>>> - * data abort.
>>>>> - */
>>>>> - if (((uintptr_t)rxbuf % 4) || (bytes_to_read %
>>>>> 4))
>>>>> - readsb(priv->ahbbase, rxbuf,
>>>>> bytes_to_read);
>>>>> - else
>>>>> - readsl(priv->ahbbase, rxbuf,
>>>>> - bytes_to_read >> 2);
>>>>> - rxbuf += bytes_to_read;
>>>>> - remaining -= bytes_to_read;
>>>>> - bytes_to_read =
>>>>> cadence_qspi_get_rd_sram_level(priv);
>>>>> - }
>>>>> - }
>>>>> -
>>>>> - /* Check indirect done status */
>>>>> - ret = wait_for_bit_le32(priv->regbase + CQSPI_REG_INDIRECTRD,
>>>>> - CQSPI_REG_INDIRECTRD_DONE, 1, 10, 0);
>>>>> - if (ret) {
>>>>> - printf("Indirect read completion error (%i)\n", ret);
>>>>> - goto failrd;
>>>>> - }
>>>>> -
>>>>> - /* Clear indirect completion status */
>>>>> - writel(CQSPI_REG_INDIRECTRD_DONE,
>>>>> - priv->regbase + CQSPI_REG_INDIRECTRD);
>>>>> -
>>>>> - /* Check indirect done status */
>>>>> - ret = wait_for_bit_le32(priv->regbase + CQSPI_REG_INDIRECTRD,
>>>>> - CQSPI_REG_INDIRECTRD_DONE, 0, 10, 0);
>>>>> - if (ret) {
>>>>> - printf("Indirect read clear completion error (%i)\n",
>>>>> ret);
>>>>> - goto failrd;
>>>>> - }
>>>>> -
>>>>> - /* Wait til QSPI is idle */
>>>>> - if (!cadence_qspi_wait_idle(priv->regbase))
>>>>> - return -EIO;
>>>>> -
>>>>> - return 0;
>>>>> -
>>>>> -failrd:
>>>>> - /* Cancel the indirect read */
>>>>> - writel(CQSPI_REG_INDIRECTRD_CANCEL,
>>>>> - priv->regbase + CQSPI_REG_INDIRECTRD);
>>>>> - return ret;
>>>>> -}
>>>>> -
>>>>> -int cadence_qspi_apb_read_execute(struct cadence_spi_priv *priv,
>>>>> - const struct spi_mem_op *op)
>>>>> -{
>>>>> - u64 from = op->addr.val;
>>>>> - void *buf = op->data.buf.in;
>>>>> - size_t len = op->data.nbytes;
>>>>> -
>>>>> - cadence_qspi_apb_enable_linear_mode(true);
>>>>> -
>>>>> - if (priv->use_dac_mode && (from + len < priv->ahbsize)) {
>>>>> - if (len < 256 ||
>>>>> - dma_memcpy(buf, priv->ahbbase + from, len) < 0) {
>>>>> - memcpy_fromio(buf, priv->ahbbase + from, len);
>>>>> - }
>>>>> - if (!cadence_qspi_wait_idle(priv->regbase))
>>>>> - return -EIO;
>>>>> - return 0;
>>>>> - }
>>>>> -
>>>>> - return cadence_qspi_apb_indirect_read_execute(priv, len, buf);
>>>>> -}
>>>>> -
>>>>> /* Opcode + Address (3/4 bytes) */
>>>>> int cadence_qspi_apb_write_setup(struct cadence_spi_priv *priv,
>>>>> const struct spi_mem_op *op)
>>>>
>>>> Your patch results in corrupted SPI NOR flash writing from U-Boot on
>>>> starfive_visionfive2_defconfig target board Pine64 Star64:
>>>>
>>>> a040578d8270ed8788d7663808ea63ce5ffd7840 is the first bad commit
>>>> commit a040578d8270ed8788d7663808ea63ce5ffd7840
>>>> Author: Michal Simek <michal.simek@amd.com>
>>>> Date: Thu Oct 9 12:34:48 2025 +0200
>>>>
>>>> spi: cadence-qspi: Remove cdns,is-dma property handling
>>>>
>>>> Steps to reproduce with Star64:
>>>>
>>>> 1). Set JH-7110 SoC RGPIO configured for UART BootROM loader mode
>>>> 2). Power on and Transfer U-Boot SPL by XMODEM as prompted by BootROM
>>>> 3). Transfer U-Boot main by YMODEM from U-Boot SPL prompt
>>>> 4). Having `(cd $HOME/build/u-boot && python3 -m http.server 6789)` or
>>>> similar for serving U-Boot SPL and U-Boot Main to save some time with
>>>> UART transfer:
>>>> 4a). wget $loadaddr http://host:6789/spl/u-boot-spl.bin.normal.out && sf
>>>> update $loadaddr 0 $filesize
>>>> 4b). wget $loadaddr http://host:6789/u-boot.itb && sf update $loadaddr
>>>> 100000 $filesize
>>>>
>>>> Repeat steps 4a and 4b, note that each time all bytes are written and no
>>>> bytes are skipped. Written data is wrong and device unsuccessfully boots
>>>> in SPI NOR flash BootROM loader configuration showing wrong information
>>>> to output.
>>>>
>>>> Please follow up with your analysis of why these changes have this
>>>> effect.
>>>
>>> I guess this is because the committer didn't take into account that when
>>> a DT did not mention the property then it is 0 (false).
>>>
>>> I assume TI K3 hardware is affected as well. So I think the patch must
>>> be reverted.
>>
>> Are you using cadence-qspi in non DMA mode? DT property shouldn't be
>> introduced
>> in the first place that's why it was removed.
>
> cadence_qspi_apb_dma_read is only implemented for versal HW and otherwise
> a NOP. This does not mean that DMA is not used on non-versal HW, it is just
> implemented differently.
ah ok. I see. Can you please send revert patch or do you want me to send it?
Thanks,
Michal
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] spi: cadence-qspi: Remove cdns,is-dma property handling
2025-10-20 9:38 ` Michal Simek
@ 2025-10-20 9:53 ` Ronald Wahl
0 siblings, 0 replies; 8+ messages in thread
From: Ronald Wahl @ 2025-10-20 9:53 UTC (permalink / raw)
To: Michal Simek, E Shattow, u-boot, git
Cc: Boon Khai Ng, Jagan Teki, Naman Trivedi, Naresh Kumar Ravulapalli,
Neha Malcom Francis, Prasad Kummari, Prasanth Babu Mantena,
Tejas Bhumkar, Tien Fong Chee, Tom Rini, Venkatesh Yadav Abbarapu
On 20.10.25 11:38, Michal Simek wrote:
> On 10/20/25 10:30, Ronald Wahl wrote:
>> On 20.10.25 07:51, Michal Simek wrote:
>>> [You don't often get email from michal.simek@amd.com. Learn why this is
>>> important at https://aka.ms/LearnAboutSenderIdentification ]
>>> On 10/20/25 00:36, Ronald Wahl wrote:
>>>> On 20.10.25 00:03, E Shattow wrote:
>>>>> On 10/9/25 03:34, Michal Simek wrote:
>>>>>> Remove cdns,is-dma DT property handling. Property is not the part
>>>>>> of DT
>>>>>> binding and it is also hardcoded to value 1 in all DTs that's why
>>>>>> remove it
>>>>>> because none is also testing value 0.
>>>>>> If there is any use case when this configuration should be supported
>>>>>> this
>>>>>> patch can be reverted.
>>>>>>
>>>>>> Signed-off-by: Michal Simek <michal.simek@amd.com>
>>>>>> ---
>>>>>>
>>>>>> Changes in v2:
>>>>>> - Remove other functions which are reported as unused by gitlab CI.
>>>>>>
>>>>>> v1: https://eur01.safelinks.protection.outlook.com/?
>>>>>> url=https%3A%2F%2Flore.kernel.org%2Fr%2F&data=05%7C02%7Cronald.wahl%40legrand.com%7Cedf1107fa8c143a9738c08de0f9cc246%7C199686b5bef4496087867a6b1888fee3%7C0%7C0%7C638965363122564250%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=wloMzuMKR6AGQHJ%2FMurQdgiZrPsQkHvsewR5%2F48TsZM%3D&reserved=0
>>>>>> c6a5b4bfe565668eb3299d5c5a5548a2163b129a.1758786568.git.michal.simek@amd.com>>> ---
>>>>>> arch/arm/dts/versal-mini-ospi.dtsi | 1 -
>>>>>> arch/arm/dts/versal-net-mini-ospi.dtsi | 1 -
>>>>>> drivers/spi/cadence_qspi.c | 8 +-
>>>>>> drivers/spi/cadence_qspi.h | 5 --
>>>>>> drivers/spi/cadence_qspi_apb.c | 119
>>>>>> -------------------------
>>>>>> 5 files changed, 1 insertion(+), 133 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/arm/dts/versal-mini-ospi.dtsi b/arch/arm/dts/
>>>>>> versal-mini-
>>>>>> ospi.dtsi
>>>>>> index eec2a08e7c70..6991f6a51db0 100644
>>>>>> --- a/arch/arm/dts/versal-mini-ospi.dtsi
>>>>>> +++ b/arch/arm/dts/versal-mini-ospi.dtsi
>>>>>> @@ -38,7 +38,6 @@
>>>>>> num-cs = <1>;
>>>>>> cdns,fifo-depth = <256>;
>>>>>> cdns,fifo-width = <4>;
>>>>>> - cdns,is-dma = <1>;
>>>>>> cdns,trigger-address = <0xc0000000>;
>>>>>> #address-cells = <1>;
>>>>>> #size-cells = <0>;
>>>>>> diff --git a/arch/arm/dts/versal-net-mini-ospi.dtsi b/arch/arm/dts/
>>>>>> versal-
>>>>>> net-mini-ospi.dtsi
>>>>>> index 1c94b352dc97..d2d5ec8e5cbf 100644
>>>>>> --- a/arch/arm/dts/versal-net-mini-ospi.dtsi
>>>>>> +++ b/arch/arm/dts/versal-net-mini-ospi.dtsi
>>>>>> @@ -52,7 +52,6 @@
>>>>>> num-cs = <1>;
>>>>>> cdns,fifo-depth = <256>;
>>>>>> cdns,fifo-width = <4>;
>>>>>> - cdns,is-dma = <1>;
>>>>>> cdns,is-stig-pgm = <1>;
>>>>>> cdns,trigger-address = <0xc0000000>;
>>>>>> #address-cells = <1>;
>>>>>> diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
>>>>>> index 599596f9f087..849bd930edf0 100644
>>>>>> --- a/drivers/spi/cadence_qspi.c
>>>>>> +++ b/drivers/spi/cadence_qspi.c
>>>>>> @@ -210,7 +210,6 @@ static int cadence_spi_probe(struct udevice *bus)
>>>>>>
>>>>>> priv->regbase = plat->regbase;
>>>>>> priv->ahbbase = plat->ahbbase;
>>>>>> - priv->is_dma = plat->is_dma;
>>>>>> priv->is_decoded_cs = plat->is_decoded_cs;
>>>>>> priv->fifo_depth = plat->fifo_depth;
>>>>>> priv->fifo_width = plat->fifo_width;
>>>>>> @@ -348,10 +347,7 @@ static int cadence_spi_mem_exec_op(struct
>>>>>> spi_slave *spi,
>>>>>> case CQSPI_READ:
>>>>>> err = cadence_qspi_apb_read_setup(priv, op);
>>>>>> if (!err) {
>>>>>> - if (priv->is_dma)
>>>>>> - err = cadence_qspi_apb_dma_read(priv,
>>>>>> op);
>>>>>> - else
>>>>>> - err =
>>>>>> cadence_qspi_apb_read_execute(priv, op);
>>>>>> + err = cadence_qspi_apb_dma_read(priv, op);
>>>>>> }
>>>>>> break;
>>>>>> case CQSPI_WRITE:
>>>>>> @@ -412,8 +408,6 @@ static int cadence_spi_of_to_plat(struct udevice
>>>>>> *bus)
>>>>>> if (plat->ahbsize >= SZ_8M)
>>>>>> priv->use_dac_mode = true;
>>>>>>
>>>>>> - plat->is_dma = dev_read_bool(bus, "cdns,is-dma");
>>>>>> -
>>>>>> /* All other parameters are embedded in the child node */
>>>>>> subnode = cadence_qspi_get_subnode(bus);
>>>>>> if (!ofnode_valid(subnode)) {
>>>>>> diff --git a/drivers/spi/cadence_qspi.h b/drivers/spi/cadence_qspi.h
>>>>>> index 879e7f8dbfb8..10c4ad14cc05 100644
>>>>>> --- a/drivers/spi/cadence_qspi.h
>>>>>> +++ b/drivers/spi/cadence_qspi.h
>>>>>> @@ -223,8 +223,6 @@ struct cadence_spi_plat {
>>>>>> u32 tchsh_ns;
>>>>>> u32 tslch_ns;
>>>>>> u32 quirks;
>>>>>> -
>>>>>> - bool is_dma;
>>>>>> };
>>>>>>
>>>>>> struct cadence_spi_priv {
>>>>>> @@ -261,7 +259,6 @@ struct cadence_spi_priv {
>>>>>> bool ddr_init;
>>>>>> bool is_decoded_cs;
>>>>>> bool use_dac_mode;
>>>>>> - bool is_dma;
>>>>>>
>>>>>> /* Transaction protocol parameters. */
>>>>>> u8 inst_width;
>>>>>> @@ -292,8 +289,6 @@ int cadence_qspi_apb_command_write(struct
>>>>>> cadence_spi_priv *priv,
>>>>>>
>>>>>> int cadence_qspi_apb_read_setup(struct cadence_spi_priv *priv,
>>>>>> const struct spi_mem_op *op);
>>>>>> -int cadence_qspi_apb_read_execute(struct cadence_spi_priv *priv,
>>>>>> - const struct spi_mem_op *op);
>>>>>> int cadence_qspi_apb_write_setup(struct cadence_spi_priv *priv,
>>>>>> const struct spi_mem_op *op);
>>>>>> int cadence_qspi_apb_write_execute(struct cadence_spi_priv *priv,
>>>>>> diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/
>>>>>> cadence_qspi_apb.c
>>>>>> index 4696c09f754b..58be017720bc 100644
>>>>>> --- a/drivers/spi/cadence_qspi_apb.c
>>>>>> +++ b/drivers/spi/cadence_qspi_apb.c
>>>>>> @@ -667,125 +667,6 @@ int cadence_qspi_apb_read_setup(struct
>>>>>> cadence_spi_priv
>>>>>> *priv,
>>>>>> return 0;
>>>>>> }
>>>>>>
>>>>>> -static u32 cadence_qspi_get_rd_sram_level(struct cadence_spi_priv
>>>>>> *priv)
>>>>>> -{
>>>>>> - u32 reg = readl(priv->regbase + CQSPI_REG_SDRAMLEVEL);
>>>>>> - reg >>= CQSPI_REG_SDRAMLEVEL_RD_LSB;
>>>>>> - return reg & CQSPI_REG_SDRAMLEVEL_RD_MASK;
>>>>>> -}
>>>>>> -
>>>>>> -static int cadence_qspi_wait_for_data(struct cadence_spi_priv *priv)
>>>>>> -{
>>>>>> - unsigned int timeout = 10000;
>>>>>> - u32 reg;
>>>>>> -
>>>>>> - while (timeout--) {
>>>>>> - reg = cadence_qspi_get_rd_sram_level(priv);
>>>>>> - if (reg)
>>>>>> - return reg;
>>>>>> - udelay(1);
>>>>>> - }
>>>>>> -
>>>>>> - return -ETIMEDOUT;
>>>>>> -}
>>>>>> -
>>>>>> -static int
>>>>>> -cadence_qspi_apb_indirect_read_execute(struct cadence_spi_priv
>>>>>> *priv,
>>>>>> - unsigned int n_rx, u8 *rxbuf)
>>>>>> -{
>>>>>> - unsigned int remaining = n_rx;
>>>>>> - unsigned int bytes_to_read = 0;
>>>>>> - int ret;
>>>>>> -
>>>>>> - writel(n_rx, priv->regbase + CQSPI_REG_INDIRECTRDBYTES);
>>>>>> -
>>>>>> - /* Start the indirect read transfer */
>>>>>> - writel(CQSPI_REG_INDIRECTRD_START,
>>>>>> - priv->regbase + CQSPI_REG_INDIRECTRD);
>>>>>> -
>>>>>> - while (remaining > 0) {
>>>>>> - ret = cadence_qspi_wait_for_data(priv);
>>>>>> - if (ret < 0) {
>>>>>> - printf("Indirect write timed out (%i)\n", ret);
>>>>>> - goto failrd;
>>>>>> - }
>>>>>> -
>>>>>> - bytes_to_read = ret;
>>>>>> -
>>>>>> - while (bytes_to_read != 0) {
>>>>>> - bytes_to_read *= priv->fifo_width;
>>>>>> - bytes_to_read = bytes_to_read > remaining ?
>>>>>> - remaining : bytes_to_read;
>>>>>> - /*
>>>>>> - * Handle non-4-byte aligned access to avoid
>>>>>> - * data abort.
>>>>>> - */
>>>>>> - if (((uintptr_t)rxbuf % 4) || (bytes_to_read %
>>>>>> 4))
>>>>>> - readsb(priv->ahbbase, rxbuf,
>>>>>> bytes_to_read);
>>>>>> - else
>>>>>> - readsl(priv->ahbbase, rxbuf,
>>>>>> - bytes_to_read >> 2);
>>>>>> - rxbuf += bytes_to_read;
>>>>>> - remaining -= bytes_to_read;
>>>>>> - bytes_to_read =
>>>>>> cadence_qspi_get_rd_sram_level(priv);
>>>>>> - }
>>>>>> - }
>>>>>> -
>>>>>> - /* Check indirect done status */
>>>>>> - ret = wait_for_bit_le32(priv->regbase + CQSPI_REG_INDIRECTRD,
>>>>>> - CQSPI_REG_INDIRECTRD_DONE, 1, 10, 0);
>>>>>> - if (ret) {
>>>>>> - printf("Indirect read completion error (%i)\n", ret);
>>>>>> - goto failrd;
>>>>>> - }
>>>>>> -
>>>>>> - /* Clear indirect completion status */
>>>>>> - writel(CQSPI_REG_INDIRECTRD_DONE,
>>>>>> - priv->regbase + CQSPI_REG_INDIRECTRD);
>>>>>> -
>>>>>> - /* Check indirect done status */
>>>>>> - ret = wait_for_bit_le32(priv->regbase + CQSPI_REG_INDIRECTRD,
>>>>>> - CQSPI_REG_INDIRECTRD_DONE, 0, 10, 0);
>>>>>> - if (ret) {
>>>>>> - printf("Indirect read clear completion error (%i)\n",
>>>>>> ret);
>>>>>> - goto failrd;
>>>>>> - }
>>>>>> -
>>>>>> - /* Wait til QSPI is idle */
>>>>>> - if (!cadence_qspi_wait_idle(priv->regbase))
>>>>>> - return -EIO;
>>>>>> -
>>>>>> - return 0;
>>>>>> -
>>>>>> -failrd:
>>>>>> - /* Cancel the indirect read */
>>>>>> - writel(CQSPI_REG_INDIRECTRD_CANCEL,
>>>>>> - priv->regbase + CQSPI_REG_INDIRECTRD);
>>>>>> - return ret;
>>>>>> -}
>>>>>> -
>>>>>> -int cadence_qspi_apb_read_execute(struct cadence_spi_priv *priv,
>>>>>> - const struct spi_mem_op *op)
>>>>>> -{
>>>>>> - u64 from = op->addr.val;
>>>>>> - void *buf = op->data.buf.in;
>>>>>> - size_t len = op->data.nbytes;
>>>>>> -
>>>>>> - cadence_qspi_apb_enable_linear_mode(true);
>>>>>> -
>>>>>> - if (priv->use_dac_mode && (from + len < priv->ahbsize)) {
>>>>>> - if (len < 256 ||
>>>>>> - dma_memcpy(buf, priv->ahbbase + from, len) < 0) {
>>>>>> - memcpy_fromio(buf, priv->ahbbase + from, len);
>>>>>> - }
>>>>>> - if (!cadence_qspi_wait_idle(priv->regbase))
>>>>>> - return -EIO;
>>>>>> - return 0;
>>>>>> - }
>>>>>> -
>>>>>> - return cadence_qspi_apb_indirect_read_execute(priv, len, buf);
>>>>>> -}
>>>>>> -
>>>>>> /* Opcode + Address (3/4 bytes) */
>>>>>> int cadence_qspi_apb_write_setup(struct cadence_spi_priv *priv,
>>>>>> const struct spi_mem_op *op)
>>>>>
>>>>> Your patch results in corrupted SPI NOR flash writing from U-Boot on
>>>>> starfive_visionfive2_defconfig target board Pine64 Star64:
>>>>>
>>>>> a040578d8270ed8788d7663808ea63ce5ffd7840 is the first bad commit
>>>>> commit a040578d8270ed8788d7663808ea63ce5ffd7840
>>>>> Author: Michal Simek <michal.simek@amd.com>
>>>>> Date: Thu Oct 9 12:34:48 2025 +0200
>>>>>
>>>>> spi: cadence-qspi: Remove cdns,is-dma property handling
>>>>>
>>>>> Steps to reproduce with Star64:
>>>>>
>>>>> 1). Set JH-7110 SoC RGPIO configured for UART BootROM loader mode
>>>>> 2). Power on and Transfer U-Boot SPL by XMODEM as prompted by BootROM
>>>>> 3). Transfer U-Boot main by YMODEM from U-Boot SPL prompt
>>>>> 4). Having `(cd $HOME/build/u-boot && python3 -m http.server 6789)` or
>>>>> similar for serving U-Boot SPL and U-Boot Main to save some time with
>>>>> UART transfer:
>>>>> 4a). wget $loadaddr http://host:6789/spl/u-boot-spl.bin.normal.out
>>>>> && sf
>>>>> update $loadaddr 0 $filesize
>>>>> 4b). wget $loadaddr http://host:6789/u-boot.itb && sf update $loadaddr
>>>>> 100000 $filesize
>>>>>
>>>>> Repeat steps 4a and 4b, note that each time all bytes are written
>>>>> and no
>>>>> bytes are skipped. Written data is wrong and device unsuccessfully
>>>>> boots
>>>>> in SPI NOR flash BootROM loader configuration showing wrong
>>>>> information
>>>>> to output.
>>>>>
>>>>> Please follow up with your analysis of why these changes have this
>>>>> effect.
>>>>
>>>> I guess this is because the committer didn't take into account that
>>>> when
>>>> a DT did not mention the property then it is 0 (false).
>>>>
>>>> I assume TI K3 hardware is affected as well. So I think the patch must
>>>> be reverted.
>>>
>>> Are you using cadence-qspi in non DMA mode? DT property shouldn't be
>>> introduced
>>> in the first place that's why it was removed.
>>
>> cadence_qspi_apb_dma_read is only implemented for versal HW and otherwise
>> a NOP. This does not mean that DMA is not used on non-versal HW, it is
>> just
>> implemented differently.
>
> ah ok. I see. Can you please send revert patch or do you want me to send
> it?
It would be good when you could do it, thanks!
- ron
________________________________
Ce message, ainsi que tous les fichiers joints à ce message, peuvent contenir des informations sensibles et/ ou confidentielles ne devant pas être divulguées. Si vous n'êtes pas le destinataire de ce message (ou que vous recevez ce message par erreur), nous vous remercions de le notifier immédiatement à son expéditeur, et de détruire ce message. Toute copie, divulgation, modification, utilisation ou diffusion, non autorisée, directe ou indirecte, de tout ou partie de ce message, est strictement interdite.
This e-mail, and any document attached hereby, may contain confidential and/or privileged information. If you are not the intended recipient (or have received this e-mail in error) please notify the sender immediately and destroy this e-mail. Any unauthorized, direct or indirect, copying, disclosure, distribution or other use of the material or parts thereof is strictly forbidden.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] spi: cadence-qspi: Remove cdns,is-dma property handling
2025-10-19 22:36 ` Ronald Wahl
2025-10-20 5:51 ` Michal Simek
@ 2025-10-23 17:51 ` Markus Elfring
1 sibling, 0 replies; 8+ messages in thread
From: Markus Elfring @ 2025-10-23 17:51 UTC (permalink / raw)
To: Ronald Wahl
Cc: git, u-boot, Boon Khai Ng, Jagan Teki, Naman Trivedi,
Naresh Kumar Ravulapalli, Neha Malcom Francis, Prasad Kummari,
Prasanth Babu Mantena, Tejas Bhumkar, Tien Fong Chee, Tom Rini,
Venkatesh Yadav Abbarapu
…
> This e-mail, and any document attached hereby, may contain confidential and/or privileged information.
See also once more:
https://subspace.kernel.org/etiquette.html#do-not-include-confidentiality-disclaimers
> If you are not the intended recipient (or have received this e-mail in error) please notify the sender immediately and destroy this e-mail.
Do you expect this action to happen more often with your information?
Regards,
Markus
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-10-23 21:42 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-09 10:34 [PATCH v2] spi: cadence-qspi: Remove cdns,is-dma property handling Michal Simek
2025-10-19 22:03 ` E Shattow
2025-10-19 22:36 ` Ronald Wahl
2025-10-20 5:51 ` Michal Simek
2025-10-20 8:30 ` Ronald Wahl
2025-10-20 9:38 ` Michal Simek
2025-10-20 9:53 ` Ronald Wahl
2025-10-23 17:51 ` Markus Elfring
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox