Skip to contentSkip to conversation

spi: cadence-xspi: support 4bytes sdma-io-width

PATCH In review Jisheng Zhang 4 participants 16 messages May 11 – May 30

Subsystems (2)
Device tree bindings SPI
lore
Share & tools
lore

Jisheng Zhang

author
#0 lore raw

cover spi: cadence-xspi: support 4bytes sdma-io-width

The cdns xspi controller SDMA data port may support wider I/O width. Wider I/O width can benefit performance. A simple test with QSPI nor flash on one arm64 platform:

1 byte io width (default):

 # dd if=/dev/mtdblock0 of=/dev/null bs=8192 count=1000
 1000+0 records in
 1000+0 records out
 8192000 bytes (7.8MB) copied, 1.368735 seconds, 5.7MB/s

4 bytes io width:

 # dd if=/dev/mtdblock0 of=/dev/null bs=8192 count=1000
 1000+0 records in
 1000+0 records out
 8192000 bytes (7.8MB) copied, 1.088787 seconds, 7.2MB/s

Improved by 26.3%!

Jisheng Zhang (2):
spi: dt-bindings: cdns,xspi: add sdma-io-width
spi: cadence-xspi: support 4bytes sdma-io-width

 .../devicetree/bindings/spi/cdns,xspi.yaml    |  6 +++
 drivers/spi/spi-cadence-xspi.c                | 43 +++++++++++++++++--
 2 files changed, 45 insertions(+), 4 deletions(-)
Signature · 1 line
2.53.0
author
#1 lore raw

PATCH 1/2 spi: dt-bindings: cdns,xspi: add sdma-io-width

The cdns xspi controller SDMA data port may support wider I/O width. Wider I/O width can benefit performance. A simple test with QSPI nor flash on one arm64 platform:

1 byte io width (default):

 # dd if=/dev/mtdblock0 of=/dev/null bs=8192 count=1000
 1000+0 records in
 1000+0 records out
 8192000 bytes (7.8MB) copied, 1.368735 seconds, 5.7MB/s

4 bytes io width:

 # dd if=/dev/mtdblock0 of=/dev/null bs=8192 count=1000
 1000+0 records in
 1000+0 records out
 8192000 bytes (7.8MB) copied, 1.088787 seconds, 7.2MB/s

Improved by 26.3%!

Add dt binding for sdma-io-width.

Signed-off-by Jisheng Zhang <[email protected]>
---
 Documentation/devicetree/bindings/spi/cdns,xspi.yaml | 6 ++++++
 1 file changed, 6 insertions(+)
Documentation/devicetree/bindings/spi/cdns,xspi.yaml +6 -0
index 38a5795589de..1fd4b6dfd417 100644
@@ -40,6 +40,12 @@ properties:
   interrupts:
     maxItems: 1
 
+  sdma-io-width:
+    description: Slave DMA data port I/O width (in bytes)
+    $ref: /schemas/types.yaml#/definitions/uint32
+    default: 1
+    enum: [ 1, 4 ]
+
 required:
   - compatible
   - reg
Signature · 1 line
2.53.0
AI review
#2 lore raw

Re: [PATCH 1/2] spi: dt-bindings: cdns,xspi: add sdma-io-width

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] The newly added device tree property `sdma-io-width` lacks a vendor prefix. --

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The newly added device tree property sdma-io-width lacks a vendor prefix. --

commit 90424bd22513430cb3aefe4494640a5d950b2a77
Author: Jisheng Zhang <[email protected]>

spi: dt-bindings: cdns,xspi: add sdma-io-width

This commit adds the sdma-io-width property to the Cadence XSPI controller device tree binding to configure the Slave DMA data port I/O width.

Documentation/devicetree/bindings/spi/cdns,xspi.yaml properties: · line 43

Quoted 9 lines
Documentation/devicetree/bindings/spi/cdns,xspi.yaml +1 -0
index 38a5795589de2..1fd4b6dfd4178 100644
@@ -40,6 +40,12 @@ properties:
   interrupts:
     maxItems: 1
 
+  sdma-io-width:

This isn't a bug, but should this property include a vendor prefix? Custom hardware properties typically use a vendor prefix like cdns,sdma-io-width.

Quoted 8 lines
Documentation/devicetree/bindings/spi/cdns,xspi.yaml +5 -0
+    description: Slave DMA data port I/O width (in bytes)
+    $ref: /schemas/types.yaml#/definitions/uint32
+    default: 1
+    enum: [ 1, 4 ]
+
 required:
   - compatible
   - reg
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
#3 lore raw

Re: [PATCH 1/2] spi: dt-bindings: cdns,xspi: add sdma-io-width

In reply to Jisheng Zhang
Quoted 30 lines · Jisheng Zhang

The cdns xspi controller SDMA data port may support wider I/O width. Wider I/O width can benefit performance. A simple test with QSPI nor flash on one arm64 platform:

1 byte io width (default):

 # dd if=/dev/mtdblock0 of=/dev/null bs=8192 count=1000
 1000+0 records in
 1000+0 records out
 8192000 bytes (7.8MB) copied, 1.368735 seconds, 5.7MB/s

4 bytes io width:

 # dd if=/dev/mtdblock0 of=/dev/null bs=8192 count=1000
 1000+0 records in
 1000+0 records out
 8192000 bytes (7.8MB) copied, 1.088787 seconds, 7.2MB/s

Improved by 26.3%!

Add dt binding for sdma-io-width.

Signed-off-by Jisheng Zhang <[email protected]>
---
 Documentation/devicetree/bindings/spi/cdns,xspi.yaml | 6 ++++++
 1 file changed, 6 insertions(+)
Documentation/devicetree/bindings/spi/cdns,xspi.yaml +0 -0
index 38a5795589de..1fd4b6dfd417 100644

Documentation/devicetree/bindings/spi/cdns,xspi.yaml properties: · line 44

Quoted 5 lines
Documentation/devicetree/bindings/spi/cdns,xspi.yaml +2 -0
@@ -40,6 +40,12 @@ properties:
   interrupts:
     maxItems: 1
 
+  sdma-io-width:
+    description: Slave DMA data port I/O width (in bytes)

Waht is SDMA? What is Slave DMA? The same?

Why this cannot be deduced from the compatible? DMA is fixed per platform and SDMA do not sound like SPI device (so slave in older terminology) property.

Anyway, there is generic property reg-io-width if this is for MMIO, bus-width and spi-tx/rx-bus-width.

Best regards,
Krzysztof

author
#4 lore raw

Re: [PATCH 1/2] spi: dt-bindings: cdns,xspi: add sdma-io-width

Quoted 34 lines · Krzysztof Kozlowski
Quoted 33 lines · Jisheng Zhang

The cdns xspi controller SDMA data port may support wider I/O width. Wider I/O width can benefit performance. A simple test with QSPI nor flash on one arm64 platform:

1 byte io width (default):

 # dd if=/dev/mtdblock0 of=/dev/null bs=8192 count=1000
 1000+0 records in
 1000+0 records out
 8192000 bytes (7.8MB) copied, 1.368735 seconds, 5.7MB/s

4 bytes io width:

 # dd if=/dev/mtdblock0 of=/dev/null bs=8192 count=1000
 1000+0 records in
 1000+0 records out
 8192000 bytes (7.8MB) copied, 1.088787 seconds, 7.2MB/s

Improved by 26.3%!

Add dt binding for sdma-io-width.

Signed-off-by Jisheng Zhang <[email protected]>
---
 Documentation/devicetree/bindings/spi/cdns,xspi.yaml | 6 ++++++
 1 file changed, 6 insertions(+)
Documentation/devicetree/bindings/spi/cdns,xspi.yaml +0 -0
index 38a5795589de..1fd4b6dfd417 100644
@@ -40,6 +40,12 @@ properties:
   interrupts:
     maxItems: 1
Quoted 8 lines

Documentation/devicetree/bindings/spi/cdns,xspi.yaml properties: · line 44

Quoted 2 lines
Documentation/devicetree/bindings/spi/cdns,xspi.yaml +2 -0
+  sdma-io-width:
+    description: Slave DMA data port I/O width (in bytes)

Waht is SDMA? What is Slave DMA? The same?

Why this cannot be deduced from the compatible? DMA is fixed per platform and SDMA do not sound like SPI device (so slave in older terminology) property.

in cdns xspi, sdma == slave dma. It can be read by any master, such as DMA engine or CPU. Its purpose and usage is similar as the data reg in other spi controllers, e.g the DW_SPI_DR in dw spi.

Quoted 3 lines

Anyway, there is generic property reg-io-width if this is for MMIO, bus-width and spi-tx/rx-bus-width.

Indeed, spi dw driver uses reg-io-width to specify the io width which is for accessing DW_SPI_DR. But I have one question: the "sdma" name is from cdns xspi HW, I'm not sure whether reg-io-width is better. If you prefer reg-io-width, I will do it in v2

Thanks

#5 lore raw

Re: [PATCH 1/2] spi: dt-bindings: cdns,xspi: add sdma-io-width

Quoted 40 lines · Jisheng Zhang
Quoted 39 lines · Krzysztof Kozlowski
Quoted 30 lines · Jisheng Zhang

The cdns xspi controller SDMA data port may support wider I/O width. Wider I/O width can benefit performance. A simple test with QSPI nor flash on one arm64 platform:

1 byte io width (default):

 # dd if=/dev/mtdblock0 of=/dev/null bs=8192 count=1000
 1000+0 records in
 1000+0 records out
 8192000 bytes (7.8MB) copied, 1.368735 seconds, 5.7MB/s

4 bytes io width:

 # dd if=/dev/mtdblock0 of=/dev/null bs=8192 count=1000
 1000+0 records in
 1000+0 records out
 8192000 bytes (7.8MB) copied, 1.088787 seconds, 7.2MB/s

Improved by 26.3%!

Add dt binding for sdma-io-width.

Signed-off-by Jisheng Zhang <[email protected]>
---
 Documentation/devicetree/bindings/spi/cdns,xspi.yaml | 6 ++++++
 1 file changed, 6 insertions(+)
Documentation/devicetree/bindings/spi/cdns,xspi.yaml +0 -0
index 38a5795589de..1fd4b6dfd417 100644

Documentation/devicetree/bindings/spi/cdns,xspi.yaml properties: · line 44

Quoted 5 lines
Documentation/devicetree/bindings/spi/cdns,xspi.yaml +2 -0
@@ -40,6 +40,12 @@ properties:
   interrupts:
     maxItems: 1
 
+  sdma-io-width:
+    description: Slave DMA data port I/O width (in bytes)

Waht is SDMA? What is Slave DMA? The same?

Quoted 7 lines
Quoted 3 lines

Why this cannot be deduced from the compatible? DMA is fixed per platform and SDMA do not sound like SPI device (so slave in older terminology) property.

in cdns xspi, sdma == slave dma. It can be read by any master, such as DMA engine or CPU. Its purpose and usage is similar as the data reg in other spi controllers, e.g the DW_SPI_DR in dw spi.

You did not really answer the questions.

Best regards,
Krzysztof

author
#6 lore raw

Re: [PATCH 1/2] spi: dt-bindings: cdns,xspi: add sdma-io-width

Quoted 45 lines · Krzysztof Kozlowski
Quoted 44 lines · Jisheng Zhang
Quoted 42 lines · Krzysztof Kozlowski
Quoted 30 lines · Jisheng Zhang

The cdns xspi controller SDMA data port may support wider I/O width. Wider I/O width can benefit performance. A simple test with QSPI nor flash on one arm64 platform:

1 byte io width (default):

 # dd if=/dev/mtdblock0 of=/dev/null bs=8192 count=1000
 1000+0 records in
 1000+0 records out
 8192000 bytes (7.8MB) copied, 1.368735 seconds, 5.7MB/s

4 bytes io width:

 # dd if=/dev/mtdblock0 of=/dev/null bs=8192 count=1000
 1000+0 records in
 1000+0 records out
 8192000 bytes (7.8MB) copied, 1.088787 seconds, 7.2MB/s

Improved by 26.3%!

Add dt binding for sdma-io-width.

Signed-off-by Jisheng Zhang <[email protected]>
---
 Documentation/devicetree/bindings/spi/cdns,xspi.yaml | 6 ++++++
 1 file changed, 6 insertions(+)
Documentation/devicetree/bindings/spi/cdns,xspi.yaml +0 -0
index 38a5795589de..1fd4b6dfd417 100644

Documentation/devicetree/bindings/spi/cdns,xspi.yaml properties: · line 44

Quoted 5 lines
Documentation/devicetree/bindings/spi/cdns,xspi.yaml +2 -0
@@ -40,6 +40,12 @@ properties:
   interrupts:
     maxItems: 1
 
+  sdma-io-width:
+    description: Slave DMA data port I/O width (in bytes)

Waht is SDMA? What is Slave DMA? The same?

Why this cannot be deduced from the compatible? DMA is fixed per platform and SDMA do not sound like SPI device (so slave in older terminology) property.

Quoted 6 lines
Quoted 3 lines

in cdns xspi, sdma == slave dma. It can be read by any master, such as DMA engine or CPU. Its purpose and usage is similar as the data reg in other spi controllers, e.g the DW_SPI_DR in dw spi.

You did not really answer the questions.

If you mean "Why this cannot be deduced from the compatible?", I think the slave dma port is part of the cdns xspi, so its io width belongs to xSPI device property.
From another side, we have seen such property in other drivers such as the reg-io-width for the dw spi DW_SPI_DR port io width.

Thanks

#7 lore raw

Re: [PATCH 1/2] spi: dt-bindings: cdns,xspi: add sdma-io-width

Quoted 50 lines · Jisheng Zhang
Quoted 49 lines · Krzysztof Kozlowski
Quoted 47 lines · Jisheng Zhang
Quoted 34 lines · Krzysztof Kozlowski
Quoted 33 lines · Jisheng Zhang

The cdns xspi controller SDMA data port may support wider I/O width. Wider I/O width can benefit performance. A simple test with QSPI nor flash on one arm64 platform:

1 byte io width (default):

 # dd if=/dev/mtdblock0 of=/dev/null bs=8192 count=1000
 1000+0 records in
 1000+0 records out
 8192000 bytes (7.8MB) copied, 1.368735 seconds, 5.7MB/s

4 bytes io width:

 # dd if=/dev/mtdblock0 of=/dev/null bs=8192 count=1000
 1000+0 records in
 1000+0 records out
 8192000 bytes (7.8MB) copied, 1.088787 seconds, 7.2MB/s

Improved by 26.3%!

Add dt binding for sdma-io-width.

Signed-off-by Jisheng Zhang <[email protected]>
---
 Documentation/devicetree/bindings/spi/cdns,xspi.yaml | 6 ++++++
 1 file changed, 6 insertions(+)
Documentation/devicetree/bindings/spi/cdns,xspi.yaml +0 -0
index 38a5795589de..1fd4b6dfd417 100644
@@ -40,6 +40,12 @@ properties:
   interrupts:
     maxItems: 1
Quoted 8 lines

Documentation/devicetree/bindings/spi/cdns,xspi.yaml properties: · line 44

Quoted 2 lines
Documentation/devicetree/bindings/spi/cdns,xspi.yaml +2 -0
+  sdma-io-width:
+    description: Slave DMA data port I/O width (in bytes)

Waht is SDMA? What is Slave DMA? The same?

Why this cannot be deduced from the compatible? DMA is fixed per platform and SDMA do not sound like SPI device (so slave in older terminology) property.

in cdns xspi, sdma == slave dma. It can be read by any master, such as DMA engine or CPU. Its purpose and usage is similar as the data reg in other spi controllers, e.g the DW_SPI_DR in dw spi.

Quoted 8 lines
Quoted 2 lines

You did not really answer the questions.

If you mean "Why this cannot be deduced from the compatible?", I think the slave dma port is part of the cdns xspi, so its io width belongs to xSPI device property.
From another side, we have seen such property in other drivers such as the reg-io-width for the dw spi DW_SPI_DR port io width.

So you mean it depends on SPI device? Then why spi-peripheral-props is not applicable here?

If this is not bus width, but DMA-something, is not really then SPI device dependent, but rather DMA controller limitation, so either deducible from compatible or something else is missing here.

Argument that some drivers do that is obviously wrong, so I am jumping there.

Best regards,
Krzysztof

maintainer
#8 lore raw

Re: [PATCH 1/2] spi: dt-bindings: cdns,xspi: add sdma-io-width

Quoted 10 lines · Krzysztof Kozlowski
Quoted 5 lines · Jisheng Zhang

If you mean "Why this cannot be deduced from the compatible?", I think the slave dma port is part of the cdns xspi, so its io width belongs to xSPI device property.
From another side, we have seen such property in other drivers such as the reg-io-width for the dw spi DW_SPI_DR port io width.

So you mean it depends on SPI device? Then why spi-peripheral-props is not applicable here?

That will be controller side, not device side.

Quoted 3 lines

If this is not bus width, but DMA-something, is not really then SPI device dependent, but rather DMA controller limitation, so either deducible from compatible or something else is missing here.

My understanding is that this is a connection between the SPI and DMA controllers so it's not as obvious as it could be which side of that link should have the property, eg:

https://www.cadence.com/content/dam/cadence-www/global/en_US/documents/tools/silicon-solutions/design-ip/controller-ip-qspi.pdf

shows a separate direct connection between the DMA controller and the xSPI controller, the DMA controller isn't interacting with registers on the CPU visible buses. The width is probably a design time configurable option on both sides of the link.

#9 lore raw

Re: [PATCH 1/2] spi: dt-bindings: cdns,xspi: add sdma-io-width

Quoted 22 lines · Mark Brown
Quoted 10 lines · Krzysztof Kozlowski
Quoted 5 lines · Jisheng Zhang

If you mean "Why this cannot be deduced from the compatible?", I think the slave dma port is part of the cdns xspi, so its io width belongs to xSPI device property.
From another side, we have seen such property in other drivers such as the reg-io-width for the dw spi DW_SPI_DR port io width.

So you mean it depends on SPI device? Then why spi-peripheral-props is not applicable here?

That will be controller side, not device side.

Quoted 3 lines

If this is not bus width, but DMA-something, is not really then SPI device dependent, but rather DMA controller limitation, so either deducible from compatible or something else is missing here.

My understanding is that this is a connection between the SPI and DMA controllers so it's not as obvious as it could be which side of that link should have the property, eg:

Quoted 6 lines

https://www.cadence.com/content/dam/cadence-www/global/en_US/documents/tools/silicon-solutions/design-ip/controller-ip-qspi.pdf

shows a separate direct connection between the DMA controller and the xSPI controller, the DMA controller isn't interacting with registers on the CPU visible buses. The width is probably a design time configurable option on both sides of the link.

Yes and that sounds a lot specific to particular controller, thus should be implied by / deducible from the compatible.

Best regards,
Krzysztof

author
#10 lore raw

Re: [PATCH 1/2] spi: dt-bindings: cdns,xspi: add sdma-io-width

Quoted 25 lines · Krzysztof Kozlowski
Quoted 24 lines · Mark Brown
Quoted 10 lines · Krzysztof Kozlowski
Quoted 5 lines · Jisheng Zhang

If you mean "Why this cannot be deduced from the compatible?", I think the slave dma port is part of the cdns xspi, so its io width belongs to xSPI device property.
From another side, we have seen such property in other drivers such as the reg-io-width for the dw spi DW_SPI_DR port io width.

So you mean it depends on SPI device? Then why spi-peripheral-props is not applicable here?

That will be controller side, not device side.

Quoted 3 lines

If this is not bus width, but DMA-something, is not really then SPI device dependent, but rather DMA controller limitation, so either deducible from compatible or something else is missing here.

My understanding is that this is a connection between the SPI and DMA controllers so it's not as obvious as it could be which side of that link should have the property, eg:

https://www.cadence.com/content/dam/cadence-www/global/en_US/documents/tools/silicon-solutions/design-ip/controller-ip-qspi.pdf

Quoted 7 lines
Quoted 4 lines

shows a separate direct connection between the DMA controller and the xSPI controller, the DMA controller isn't interacting with registers on the CPU visible buses. The width is probably a design time configurable option on both sides of the link.

Yes and that sounds a lot specific to particular controller, thus should be implied by / deducible from the compatible.

This is IP feature, so if we couple the IP's feature with platform compatible, I would see some unnecessary LoCs. For example, Let's assume the IP has 10 users, they all support 4 bytes io width, other features are the same.

If implied by the compatible string, we need to add 10 compatible string support both in code and dt-bindings.

vs

If supported by "sdma-io-width", nothing is needed after this patch

IMHO, the 2nd sounds better, what do you think?

Thanks

#11 lore raw

Re: [PATCH 1/2] spi: dt-bindings: cdns,xspi: add sdma-io-width

Quoted 34 lines · Jisheng Zhang
Quoted 25 lines · Krzysztof Kozlowski
Quoted 24 lines · Mark Brown
Quoted 10 lines · Krzysztof Kozlowski
Quoted 5 lines · Jisheng Zhang

If you mean "Why this cannot be deduced from the compatible?", I think the slave dma port is part of the cdns xspi, so its io width belongs to xSPI device property.
From another side, we have seen such property in other drivers such as the reg-io-width for the dw spi DW_SPI_DR port io width.

So you mean it depends on SPI device? Then why spi-peripheral-props is not applicable here?

That will be controller side, not device side.

Quoted 3 lines

If this is not bus width, but DMA-something, is not really then SPI device dependent, but rather DMA controller limitation, so either deducible from compatible or something else is missing here.

My understanding is that this is a connection between the SPI and DMA controllers so it's not as obvious as it could be which side of that link should have the property, eg:

https://www.cadence.com/content/dam/cadence-www/global/en_US/documents/tools/silicon-solutions/design-ip/controller-ip-qspi.pdf

Quoted 7 lines
Quoted 4 lines

shows a separate direct connection between the DMA controller and the xSPI controller, the DMA controller isn't interacting with registers on the CPU visible buses. The width is probably a design time configurable option on both sides of the link.

Yes and that sounds a lot specific to particular controller, thus should be implied by / deducible from the compatible.

Quoted 7 lines

This is IP feature, so if we couple the IP's feature with platform compatible, I would see some unnecessary LoCs. For example, Let's assume the IP has 10 users, they all support 4 bytes io width, other features are the same.

If implied by the compatible string, we need to add 10 compatible string support both in code and dt-bindings.

You always need 10 compatible strings in bindings.

But driver would need only one, since devices are compatible as you described.

Quoted 6 lines

vs

If supported by "sdma-io-width", nothing is needed after this patch

IMHO, the 2nd sounds better, what do you think?

We answered this in writing bindings. Properties are not replacement for specific compatible. I don't know how to write that sentence in bindings clearer - it's exactly this case.

Best regards,
Krzysztof

author
#12 lore raw

Re: [PATCH 1/2] spi: dt-bindings: cdns,xspi: add sdma-io-width

Quoted 40 lines · Krzysztof Kozlowski
Quoted 39 lines · Jisheng Zhang
Quoted 25 lines · Krzysztof Kozlowski
Quoted 24 lines · Mark Brown
Quoted 10 lines · Krzysztof Kozlowski
Quoted 5 lines · Jisheng Zhang

If you mean "Why this cannot be deduced from the compatible?", I think the slave dma port is part of the cdns xspi, so its io width belongs to xSPI device property.
From another side, we have seen such property in other drivers such as the reg-io-width for the dw spi DW_SPI_DR port io width.

So you mean it depends on SPI device? Then why spi-peripheral-props is not applicable here?

That will be controller side, not device side.

Quoted 3 lines

If this is not bus width, but DMA-something, is not really then SPI device dependent, but rather DMA controller limitation, so either deducible from compatible or something else is missing here.

My understanding is that this is a connection between the SPI and DMA controllers so it's not as obvious as it could be which side of that link should have the property, eg:

https://www.cadence.com/content/dam/cadence-www/global/en_US/documents/tools/silicon-solutions/design-ip/controller-ip-qspi.pdf

Quoted 7 lines
Quoted 4 lines

shows a separate direct connection between the DMA controller and the xSPI controller, the DMA controller isn't interacting with registers on the CPU visible buses. The width is probably a design time configurable option on both sides of the link.

Yes and that sounds a lot specific to particular controller, thus should be implied by / deducible from the compatible.

This is IP feature, so if we couple the IP's feature with platform compatible, I would see some unnecessary LoCs. For example, Let's assume the IP has 10 users, they all support 4 bytes io width, other features are the same.

Quoted 7 lines
Quoted 2 lines

If implied by the compatible string, we need to add 10 compatible string support both in code and dt-bindings.

You always need 10 compatible strings in bindings.

But driver would need only one, since devices are compatible as you described.

Hi all,

Update: after carefully reading the registers' document, I found the slave dma data width can be known by checking CTRL_FEATURES_REG's DMA_DATA_WIDTH bit, we can don't need the DT property any more. I'm cooking v2.

Thanks for all the review comments

Quoted 14 lines
Quoted 7 lines

vs

If supported by "sdma-io-width", nothing is needed after this patch

IMHO, the 2nd sounds better, what do you think?

We answered this in writing bindings. Properties are not replacement for specific compatible. I don't know how to write that sentence in bindings clearer - it's exactly this case.

Best regards,
Krzysztof

author
#13 lore raw

Re: [PATCH 1/2] spi: dt-bindings: cdns,xspi: add sdma-io-width

In reply to Krzysztof Kozlowski
Quoted 54 lines · Krzysztof Kozlowski
Quoted 53 lines · Jisheng Zhang
Quoted 51 lines · Krzysztof Kozlowski
Quoted 40 lines · Jisheng Zhang
Quoted 39 lines · Krzysztof Kozlowski
Quoted 30 lines · Jisheng Zhang

The cdns xspi controller SDMA data port may support wider I/O width. Wider I/O width can benefit performance. A simple test with QSPI nor flash on one arm64 platform:

1 byte io width (default):

 # dd if=/dev/mtdblock0 of=/dev/null bs=8192 count=1000
 1000+0 records in
 1000+0 records out
 8192000 bytes (7.8MB) copied, 1.368735 seconds, 5.7MB/s

4 bytes io width:

 # dd if=/dev/mtdblock0 of=/dev/null bs=8192 count=1000
 1000+0 records in
 1000+0 records out
 8192000 bytes (7.8MB) copied, 1.088787 seconds, 7.2MB/s

Improved by 26.3%!

Add dt binding for sdma-io-width.

Signed-off-by Jisheng Zhang <[email protected]>
---
 Documentation/devicetree/bindings/spi/cdns,xspi.yaml | 6 ++++++
 1 file changed, 6 insertions(+)
Documentation/devicetree/bindings/spi/cdns,xspi.yaml +0 -0
index 38a5795589de..1fd4b6dfd417 100644

Documentation/devicetree/bindings/spi/cdns,xspi.yaml properties: · line 44

Quoted 5 lines
Documentation/devicetree/bindings/spi/cdns,xspi.yaml +2 -0
@@ -40,6 +40,12 @@ properties:
   interrupts:
     maxItems: 1
 
+  sdma-io-width:
+    description: Slave DMA data port I/O width (in bytes)

Waht is SDMA? What is Slave DMA? The same?

Quoted 7 lines
Quoted 3 lines

Why this cannot be deduced from the compatible? DMA is fixed per platform and SDMA do not sound like SPI device (so slave in older terminology) property.

in cdns xspi, sdma == slave dma. It can be read by any master, such as DMA engine or CPU. Its purpose and usage is similar as the data reg in other spi controllers, e.g the DW_SPI_DR in dw spi.

You did not really answer the questions.

Quoted 7 lines
Quoted 5 lines

If you mean "Why this cannot be deduced from the compatible?", I think the slave dma port is part of the cdns xspi, so its io width belongs to xSPI device property.
From another side, we have seen such property in other drivers such as the reg-io-width for the dw spi DW_SPI_DR port io width.

So you mean it depends on SPI device? Then why spi-peripheral-props is

Nope, it depends on the spi controller itself.

Quoted 4 lines

not applicable here?

If this is not bus width, but DMA-something, is not really then SPI device dependent, but rather DMA controller limitation, so either

The "sdma" name is a bit misleading but that's from the hw. It's a data io port in fact. Any SPI controller has this data io reg or port. It's not DMA controller at all. IMHO, what io width may be supported is determined by the setting during IP configuration.

So from this point of view, is it SPI controller property?

Quoted 7 lines

deducible from compatible or something else is missing here.

Argument that some drivers do that is obviously wrong, so I am jumping there.

Best regards,
Krzysztof

author
#14 lore raw

PATCH 2/2 spi: cadence-xspi: support 4bytes sdma-io-width

In reply to Jisheng Zhang

The cdns xspi controller SDMA data port may support wider I/O width. Wider I/O width can benefit performance. A simple test with QSPI nor flash on one arm64 platform:

1 byte io width (default):

 # dd if=/dev/mtdblock0 of=/dev/null bs=8192 count=1000
 1000+0 records in
 1000+0 records out
 8192000 bytes (7.8MB) copied, 1.368735 seconds, 5.7MB/s

4 bytes io width:

 # dd if=/dev/mtdblock0 of=/dev/null bs=8192 count=1000
 1000+0 records in
 1000+0 records out
 8192000 bytes (7.8MB) copied, 1.088787 seconds, 7.2MB/s

Improved by 26.3%!

Signed-off-by Jisheng Zhang <[email protected]>
---
 drivers/spi/spi-cadence-xspi.c | 43 ++++++++++++++++++++++++++++++----
 1 file changed, 39 insertions(+), 4 deletions(-)
drivers/spi/spi-cadence-xspi.c +39 -4
index 895b4b3276a5..c1fb749540e7 100644
@@ -369,6 +369,7 @@ struct cdns_xspi_dev {
 
 	void *in_buffer;
 	const void *out_buffer;
+	u32 sdma_io_width;
 
 	u8 hw_num_banks;
 

@@ -578,6 +579,38 @@ static int cdns_xspi_controller_init(struct cdns_xspi_dev *cdns_xspi)
 	return 0;
 }
 
+static inline void cdns_xspi_sdma_read(struct cdns_xspi_dev *cdns_xspi, size_t len)
+{
+	void __iomem *src = cdns_xspi->sdmabase;
+	void *buf = cdns_xspi->in_buffer;
+	size_t offset = 0;
+
+	if (cdns_xspi->sdma_io_width == 4) {
+		if (IS_ALIGNED((uintptr_t)src, 4) && IS_ALIGNED((uintptr_t)buf, 4)) {
+			ioread32_rep(src, buf, len >> 2);
+			offset = len & ~0x3;
+			len -= offset;
+		}
+	}
+	ioread8_rep(src, (u8 *)buf + offset, len);
+}
+
+static inline void cdns_xspi_sdma_write(struct cdns_xspi_dev *cdns_xspi, size_t len)
+{
+	void __iomem *dst = cdns_xspi->sdmabase;
+	const void *buf = cdns_xspi->out_buffer;
+	size_t offset = 0;
+
+	if (cdns_xspi->sdma_io_width == 4) {
+		if (IS_ALIGNED((uintptr_t)dst, 4) && IS_ALIGNED((uintptr_t)buf, 4)) {
+			iowrite32_rep(dst, buf, len >> 2);
+			offset = len & ~0x3;
+			len -= offset;
+		}
+	}
+	iowrite8_rep(dst, (const u8 *)buf + offset, len);
+}
+
 static void cdns_xspi_sdma_handle(struct cdns_xspi_dev *cdns_xspi)
 {
 	u32 sdma_size, sdma_trd_info;

@@ -589,13 +622,11 @@ static void cdns_xspi_sdma_handle(struct cdns_xspi_dev *cdns_xspi)
 
 	switch (sdma_dir) {
 	case CDNS_XSPI_SDMA_DIR_READ:
-		ioread8_rep(cdns_xspi->sdmabase,
-			    cdns_xspi->in_buffer, sdma_size);
+		cdns_xspi_sdma_read(cdns_xspi, sdma_size);
 		break;
 
 	case CDNS_XSPI_SDMA_DIR_WRITE:
-		iowrite8_rep(cdns_xspi->sdmabase,
-			     cdns_xspi->out_buffer, sdma_size);
+		cdns_xspi_sdma_write(cdns_xspi, sdma_size);
 		break;
 	}
 }

@@ -1215,6 +1246,10 @@ static int cdns_xspi_probe(struct platform_device *pdev)
 		}
 	}
 
+	if (device_property_read_u32(&pdev->dev, "sdma-io-width",
+				     &cdns_xspi->sdma_io_width))
+		cdns_xspi->sdma_io_width = 1;
+
 	cdns_xspi->irq = platform_get_irq(pdev, 0);
 	if (cdns_xspi->irq < 0)
 		return -ENXIO;
Signature · 1 line
2.53.0
AI review
#15 lore raw
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Mixing 32-bit and 8-bit stream accessors on the same SDMA port causes data corruption on…

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Mixing 32-bit and 8-bit stream accessors on the same SDMA port causes data corruption on big-endian systems and potential bus faults.
- [Low] Casting an __iomem pointer directly to uintptr_t drops the __iomem annotation and the alignment check on sdmabase is redundant. --

commit e78d668225027044d0694ef2be9788b6fddb0ef3
Author: Jisheng Zhang <[email protected]>

spi: cadence-xspi: support 4bytes sdma-io-width

This commit adds support for 4-byte SDMA I/O width to the Cadence XSPI controller. It introduces new read and write functions that utilize 32-bit I/O accessors when the buffer is aligned and the hardware width is configured to 4 bytes.

Quoted 4 lines
drivers/spi/spi-cadence-xspi.c +0 -0
index 895b4b3276a56..c1fb749540e7a 100644

[ ... ]

drivers/spi/spi-cadence-xspi.c static int cdns_xspi_controller_init(struct cdns_xspi_dev *cdns_xspi) · line 589

Quoted 12 lines
drivers/spi/spi-cadence-xspi.c +8 -0
@@ -578,6 +579,38 @@ static int cdns_xspi_controller_init(struct cdns_xspi_dev *cdns_xspi)
 	return 0;
 }
 
+static inline void cdns_xspi_sdma_read(struct cdns_xspi_dev *cdns_xspi, size_t len)
+{
+	void __iomem *src = cdns_xspi->sdmabase;
+	void *buf = cdns_xspi->in_buffer;
+	size_t offset = 0;
+
+	if (cdns_xspi->sdma_io_width == 4) {
+		if (IS_ALIGNED((uintptr_t)src, 4) && IS_ALIGNED((uintptr_t)buf, 4)) {

This isn't a bug, but does casting the __iomem pointer to uintptr_t drop the address space annotation and trigger a sparse warning?

Since sdmabase is mapped via devm_ioremap_resource, is the alignment check on src redundant?

drivers/spi/spi-cadence-xspi.c static int cdns_xspi_controller_init(struct cdns_xspi_dev *cdns_xspi) · line 595

Quoted 6 lines
drivers/spi/spi-cadence-xspi.c +6 -0
+			ioread32_rep(src, buf, len >> 2);
+			offset = len & ~0x3;
+			len -= offset;
+		}
+	}
+	ioread8_rep(src, (u8 *)buf + offset, len);

Can mixing 32-bit and 8-bit stream accessors on the same SDMA port cause data corruption on big-endian architectures?

On big-endian systems, reading memory in 32-bit native words places bytes in a different order than processing them sequentially in 8-bit chunks.

Additionally, if the hardware port is explicitly configured for 32-bit I/O, could falling back to 8-bit accessors for remainders or unaligned buffers trigger bus faults?

Using 32-bit stream accessors exclusively with a temporary 32-bit variable and memcpy for unaligned buffers or remainders would resolve this.

Quoted 6 lines
drivers/spi/spi-cadence-xspi.c +6 -0
+}
+
+static inline void cdns_xspi_sdma_write(struct cdns_xspi_dev *cdns_xspi, size_t len)
+{
+	void __iomem *dst = cdns_xspi->sdmabase;
+	const void *buf = cdns_xspi->out_buffer;

drivers/spi/spi-cadence-xspi.c static int cdns_xspi_controller_init(struct cdns_xspi_dev *cdns_xspi) · line 611

Quoted 10 lines
drivers/spi/spi-cadence-xspi.c +10 -0
+	size_t offset = 0;
+
+	if (cdns_xspi->sdma_io_width == 4) {
+		if (IS_ALIGNED((uintptr_t)dst, 4) && IS_ALIGNED((uintptr_t)buf, 4)) {
+			iowrite32_rep(dst, buf, len >> 2);
+			offset = len & ~0x3;
+			len -= offset;
+		}
+	}
+	iowrite8_rep(dst, (const u8 *)buf + offset, len);

Does the same accessor mixing issue apply here when writing to the SDMA port?

Quoted 1 line
drivers/spi/spi-cadence-xspi.c +1 -0
+}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2