From: Michal Simek <michal.simek@amd.com>
To: Ronald Wahl <ronald.wahl@legrand.com>, E Shattow <e@freeshell.de>,
u-boot@lists.denx.de, git@amd.com
Cc: Boon Khai Ng <boon.khai.ng@altera.com>,
Jagan Teki <jagan@amarulasolutions.com>,
Naman Trivedi <naman.trivedimanojbhai@amd.com>,
Naresh Kumar Ravulapalli <nareshkumar.ravulapalli@altera.com>,
Neha Malcom Francis <n-francis@ti.com>,
Prasad Kummari <prasad.kummari@amd.com>,
Prasanth Babu Mantena <p-mantena@ti.com>,
Tejas Bhumkar <tejas.arvind.bhumkar@amd.com>,
Tien Fong Chee <tien.fong.chee@altera.com>,
Tom Rini <trini@konsulko.com>,
Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
Subject: Re: [PATCH v2] spi: cadence-qspi: Remove cdns,is-dma property handling
Date: Mon, 20 Oct 2025 11:38:37 +0200 [thread overview]
Message-ID: <df818160-3ffa-49ad-959d-3df22fae9100@amd.com> (raw)
In-Reply-To: <2b3d068d-4888-4594-a75e-6c2729bf28c4@legrand.com>
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
next prev parent reply other threads:[~2025-10-20 9:38 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2025-10-20 9:53 ` Ronald Wahl
2025-10-23 17:51 ` Markus Elfring
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=df818160-3ffa-49ad-959d-3df22fae9100@amd.com \
--to=michal.simek@amd.com \
--cc=boon.khai.ng@altera.com \
--cc=e@freeshell.de \
--cc=git@amd.com \
--cc=jagan@amarulasolutions.com \
--cc=n-francis@ti.com \
--cc=naman.trivedimanojbhai@amd.com \
--cc=nareshkumar.ravulapalli@altera.com \
--cc=p-mantena@ti.com \
--cc=prasad.kummari@amd.com \
--cc=ronald.wahl@legrand.com \
--cc=tejas.arvind.bhumkar@amd.com \
--cc=tien.fong.chee@altera.com \
--cc=trini@konsulko.com \
--cc=u-boot@lists.denx.de \
--cc=venkatesh.abbarapu@amd.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox