[PATCH 0/3] spi-cadence: support transmission with bits_per_word

[PATCH 0/3] spi-cadence: support transmission with bits_per_word

[Jun Guo]

The Cadence SPI IP supports configurable FIFO data widths during
integration. On some SoCs, the FIFO data width is designed to be 16 or
32 bits at the chip design stage. However, the current driver only
supports communication with an 8-bit FIFO data width. Therefore, these
patches are added to enable the driver to support communication with
16-bit and 32-bit FIFO data widths.

This series introduces the following enhancements for Cadence SPI
controller support on arm64 platforms:

Patch 1: Document the 'fifo-width' property as optional in
spi-cadence device tree bindings.
Patch 2: Enhance the SPI Cadence driver to support data transmission
with bits_per_word values of 16 and 32.
Patch 3: Add a new 'fifo-width' configuration property to Cadence
SPI node in the CIX device tree.

The CIX Sky1 SPI supported patch is added:
https://lore.kernel.org/all/20250919013118.853078-1-jun.guo@cixtech.com/

This series:
- Documents the new property usage.
- Enables 16/32 bits per word in the driver for broader hardware
  compatibility.
- Makes fifo-width configurable via DT.

The patches have been tested on CIX SKY1 platform.

Jun Guo (3):
  dt-bindings: spi: spi-cadence: document optional fifo-width DT
    property
  spi: spi-cadence: supports transmission with bits_per_word of 16 and
    32
  arm64: dts: cix: add the fifo-width configuration field for cadence
    SPI

 .../devicetree/bindings/spi/spi-cadence.yaml  |  11 ++
 arch/arm64/boot/dts/cix/sky1.dtsi             |   2 +
 drivers/spi/spi-cadence.c                     | 125 ++++++++++++++++--
 3 files changed, 125 insertions(+), 13 deletions(-)

-- 
2.34.1

[PATCH 1/3] dt-bindings: spi: spi-cadence: document optional fifo-width DT property

[Jun Guo]

Add documentation for the optional 'fifo-width' device tree property
for the Cadence SPI controller.

Signed-off-by: Jun Guo <jun.guo@cixtech.com>
---
 .../devicetree/bindings/spi/spi-cadence.yaml          | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/spi-cadence.yaml b/Documentation/devicetree/bindings/spi/spi-cadence.yaml
index 8de96abe9da1..b2e3f217473b 100644
--- a/Documentation/devicetree/bindings/spi/spi-cadence.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-cadence.yaml
@@ -62,6 +62,17 @@ properties:
     items:
       - const: spi
 
+  fifo-width:
+    description: |
+      This property specifies the FIFO data width (in bits) of the hardware.
+      It must be configured according to the actual FIFO width set during
+      the IP design. For instance, if the hardware FIFO is 32 bits wide,
+      this property should be set to 32.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 8
+    maximum: 32
+    default: 8
+
 required:
   - compatible
   - reg
-- 
2.34.1

Re: [PATCH 1/3] dt-bindings: spi: spi-cadence: document optional fifo-width DT property

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1765 bytes --]

On Tue, Sep 30, 2025 at 03:56:42PM +0800, Jun Guo wrote:
> Add documentation for the optional 'fifo-width' device tree property
> for the Cadence SPI controller.
> 
> Signed-off-by: Jun Guo <jun.guo@cixtech.com>
> ---
>  .../devicetree/bindings/spi/spi-cadence.yaml          | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-cadence.yaml b/Documentation/devicetree/bindings/spi/spi-cadence.yaml
> index 8de96abe9da1..b2e3f217473b 100644
> --- a/Documentation/devicetree/bindings/spi/spi-cadence.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-cadence.yaml
> @@ -62,6 +62,17 @@ properties:
>      items:
>        - const: spi
>  
> +  fifo-width:
> +    description: |
> +      This property specifies the FIFO data width (in bits) of the hardware.
> +      It must be configured according to the actual FIFO width set during
> +      the IP design. For instance, if the hardware FIFO is 32 bits wide,
> +      this property should be set to 32.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 8
> +    maximum: 32
> +    default: 8

I assume this differs from fifo-depth because this is the actual width
of the registers rather than the number of elements of that width the
FIFO can contain?

However, this isn't something defined as common in spi-controller.yaml
so you'll need a vendor prefix for the property if the property stays.
This does, however, seem like something that can just be determined by
the compatible and that your omission of a soc-specific one is what's
lead you to introduce this property. Why not just use a sky1-specific
compatible here?

> +
>  required:
>    - compatible
>    - reg
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

回复: [PATCH 1/3] dt-bindings: spi: spi-cadence: document optional fifo-width DT property

[Jun Guo]

On Tue, Oct 01, 2025 at 02:52:00AM +0800, Conor Dooley wrote:
> On Tue, Sep 30, 2025 at 03:56:42PM +0800, Jun Guo wrote:
> > Add documentation for the optional 'fifo-width' device tree property
> > for the Cadence SPI controller.
> >
> > Signed-off-by: Jun Guo <jun.guo@cixtech.com>
> > ---
> >  .../devicetree/bindings/spi/spi-cadence.yaml          | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/spi/spi-cadence.yaml b/Documentation/devicetree/bindings/spi/spi-cadence.yaml
> > index 8de96abe9da1..b2e3f217473b 100644
> > --- a/Documentation/devicetree/bindings/spi/spi-cadence.yaml
> > +++ b/Documentation/devicetree/bindings/spi/spi-cadence.yaml
> > @@ -62,6 +62,17 @@ properties:
> >      items:
> >        - const: spi
> >
> > +  fifo-width:
> > +    description: |
> > +      This property specifies the FIFO data width (in bits) of the hardware.
> > +      It must be configured according to the actual FIFO width set during
> > +      the IP design. For instance, if the hardware FIFO is 32 bits wide,
> > +      this property should be set to 32.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    minimum: 8
> > +    maximum: 32
> > +    default: 8
>
> I assume this differs from fifo-depth because this is the actual width
> of the registers rather than the number of elements of that width the
> FIFO can contain?

Thank you for your review. You are absolutely correct. The `fifo-width`
indeed refers to the physical width of the FIFO data registers (e.g., 8,
16, or 32 bits), whereas `fifo-depth` describes how many elements of
 that width the FIFO can store.

> However, this isn't something defined as common in spi-controller.yaml
> so you'll need a vendor prefix for the property if the property stays.
> This does, however, seem like something that can just be determined by
> the compatible and that your omission of a soc-specific one is what's
> lead you to introduce this property. Why not just use a sky1-specific
> compatible here?

You raise an excellent point, and I initially had the same thought. However,
after further consideration, I realized that the IP of Cadence SPI actually
supports configurable FIFO width as a feature. The choice of using 8-bit,
16-bit, or 32-bit FIFO width can be made by the SoC integrator based on
their specific requirements. This is therefore a feature of the Cadence IP
itself, rather than a chip vendor-specific design constraint.

For this reason, I believe defining a common `fifo-width` property for
Cadence SPI controllers is more appropriate, as it allows any SoC using
this IP with different FIFO width configurations to utilize this property,
without needing to create a specific compatible string for each SoC variant.

Thank you for your valuable time and insightful suggestions. I look forward to
your further feedback on this approach.

>> +
>>  required:
>>    - compatible
>>    - reg
>> --
>> 2.34.1
>>

Best regards,
Jun Guo

________________________________________
发件人: Jun Guo <Jun.Guo@cixtech.com>
发送时间: 2025年10月1日 22:24
收件人: Conor Dooley
抄送: Peter Chen; Fugang Duan; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; broonie@kernel.org; linux-spi@vger.kernel.org; michal.simek@amd.com; cix-kernel-upstream; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
主题: 回复: [PATCH 1/3] dt-bindings: spi: spi-cadence: document optional fifo-width DT property

On Tue, Oct 01, 2025 at 02:52:00AM +0800, Conor Dooley wrote:
> On Tue, Sep 30, 2025 at 03:56:42PM +0800, Jun Guo wrote:
> > Add documentation for the optional 'fifo-width' device tree property
> > for the Cadence SPI controller.
> >
> > Signed-off-by: Jun Guo <jun.guo@cixtech.com>
> > ---
> >  .../devicetree/bindings/spi/spi-cadence.yaml          | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/spi/spi-cadence.yaml b/Documentation/devicetree/bindings/spi/spi-cadence.yaml
> > index 8de96abe9da1..b2e3f217473b 100644
> > --- a/Documentation/devicetree/bindings/spi/spi-cadence.yaml
> > +++ b/Documentation/devicetree/bindings/spi/spi-cadence.yaml
> > @@ -62,6 +62,17 @@ properties:
> >      items:
> >        - const: spi
> >
> > +  fifo-width:
> > +    description: |
> > +      This property specifies the FIFO data width (in bits) of the hardware.
> > +      It must be configured according to the actual FIFO width set during
> > +      the IP design. For instance, if the hardware FIFO is 32 bits wide,
> > +      this property should be set to 32.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    minimum: 8
> > +    maximum: 32
> > +    default: 8
>
> I assume this differs from fifo-depth because this is the actual width
> of the registers rather than the number of elements of that width the
> FIFO can contain?

Thank you for your review. You are absolutely correct. The `fifo-width`
indeed refers to the physical width of the FIFO data registers (e.g., 8,
16, or 32 bits), whereas `fifo-depth` describes how many elements of
 that width the FIFO can store.

> However, this isn't something defined as common in spi-controller.yaml
> so you'll need a vendor prefix for the property if the property stays.
> This does, however, seem like something that can just be determined by
> the compatible and that your omission of a soc-specific one is what's
> lead you to introduce this property. Why not just use a sky1-specific
> compatible here?

You raise an excellent point, and I initially had the same thought. However,
after further consideration, I realized that the IP of Cadence SPI actually
supports configurable FIFO width as a feature. The choice of using 8-bit,
16-bit, or 32-bit FIFO width can be made by the SoC integrator based on
their specific requirements. This is therefore a feature of the Cadence IP
itself, rather than a chip vendor-specific design constraint.

For this reason, I believe defining a common `fifo-width` property for
Cadence SPI controllers is more appropriate, as it allows any SoC using
this IP with different FIFO width configurations to utilize this property,
without needing to create a specific compatible string for each SoC variant.

Thank you for your valuable time and insightful suggestions. I look forward to
your further feedback on this approach.

>> +
>>  required:
>>    - compatible
>>    - reg
>> --
>> 2.34.1
>>

Best regards,
Jun Guo
________________________________
发件人: Conor Dooley <conor@kernel.org>
发送时间: 2025年10月1日 2:51
收件人: Jun Guo <jun.guo@cixtech.com>
抄送: Peter Chen <peter.chen@cixtech.com>; Fugang Duan <fugang.duan@cixtech.com>; robh@kernel.org <robh@kernel.org>; krzk+dt@kernel.org <krzk+dt@kernel.org>; conor+dt@kernel.org <conor+dt@kernel.org>; broonie@kernel.org <broonie@kernel.org>; linux-spi@vger.kernel.org <linux-spi@vger.kernel.org>; michal.simek@amd.com <michal.simek@amd.com>; cix-kernel-upstream <cix-kernel-upstream@cixtech.com>; linux-arm-kernel@lists.infradead.org <linux-arm-kernel@lists.infradead.org>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
主题: Re: [PATCH 1/3] dt-bindings: spi: spi-cadence: document optional fifo-width DT property

EXTERNAL EMAIL

Re: 回复: [PATCH 1/3] dt-bindings: spi: spi-cadence: document optional fifo-width DT property

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3828 bytes --]

On Wed, Oct 01, 2025 at 02:36:44PM +0000, Jun Guo wrote:
> On Tue, Oct 01, 2025 at 02:52:00AM +0800, Conor Dooley wrote:
> > On Tue, Sep 30, 2025 at 03:56:42PM +0800, Jun Guo wrote:
> > > Add documentation for the optional 'fifo-width' device tree property
> > > for the Cadence SPI controller.
> > >
> > > Signed-off-by: Jun Guo <jun.guo@cixtech.com>
> > > ---
> > >  .../devicetree/bindings/spi/spi-cadence.yaml          | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/spi/spi-cadence.yaml b/Documentation/devicetree/bindings/spi/spi-cadence.yaml
> > > index 8de96abe9da1..b2e3f217473b 100644
> > > --- a/Documentation/devicetree/bindings/spi/spi-cadence.yaml
> > > +++ b/Documentation/devicetree/bindings/spi/spi-cadence.yaml
> > > @@ -62,6 +62,17 @@ properties:
> > >      items:
> > >        - const: spi
> > >
> > > +  fifo-width:
> > > +    description: |
> > > +      This property specifies the FIFO data width (in bits) of the hardware.
> > > +      It must be configured according to the actual FIFO width set during
> > > +      the IP design. For instance, if the hardware FIFO is 32 bits wide,
> > > +      this property should be set to 32.
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    minimum: 8
> > > +    maximum: 32
> > > +    default: 8
> >
> > I assume this differs from fifo-depth because this is the actual width
> > of the registers rather than the number of elements of that width the
> > FIFO can contain?
> 
> Thank you for your review. You are absolutely correct. The `fifo-width`
> indeed refers to the physical width of the FIFO data registers (e.g., 8,
> 16, or 32 bits), whereas `fifo-depth` describes how many elements of
>  that width the FIFO can store.
> 
> > However, this isn't something defined as common in spi-controller.yaml
> > so you'll need a vendor prefix for the property if the property stays.
> > This does, however, seem like something that can just be determined by
> > the compatible and that your omission of a soc-specific one is what's
> > lead you to introduce this property. Why not just use a sky1-specific
> > compatible here?
> 
> You raise an excellent point, and I initially had the same thought. However,
> after further consideration, I realized that the IP of Cadence SPI actually
> supports configurable FIFO width as a feature. The choice of using 8-bit,
> 16-bit, or 32-bit FIFO width can be made by the SoC integrator based on
> their specific requirements. This is therefore a feature of the Cadence IP
> itself, rather than a chip vendor-specific design constraint.
> 
> For this reason, I believe defining a common `fifo-width` property for
> Cadence SPI controllers is more appropriate, as it allows any SoC using
> this IP with different FIFO width configurations to utilize this property,
> without needing to create a specific compatible string for each SoC variant.

Except, you do need to create a soc-specific compatible string for every
device, the fact that you didn't add one for your sky1 SoC was a mistake
that you should fix. SoC-specific compatibles are a requirement.
The "cnds,spi-r1p6" compatible seems to be used on Xilinx platforms,
including a zynq platform that should probably be using the zynq
soc-specific compatible. r1p6 sounds like some sort of version info, is
that the version you are even using?

Once you have added a compatible for the sky1, this property is not
needed, since the depth can be determined from that. Any other user that
wants to use non-default depths can also use their soc-specific
compatibles for that purpose.

> 
> Thank you for your valuable time and insightful suggestions. I look forward to
> your further feedback on this approach.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

回复: 回复: [PATCH 1/3] dt-bindings: spi: spi-cadence: document optional fifo-width DT property

[Jun Guo]

On Tue, Oct 02, 2025 at 02:04:00AM +0000, Conor Dooley wrote:
>On Wed, Oct 01, 2025 at 02:36:44PM +0000, Jun Guo wrote:
> > On Tue, Oct 01, 2025 at 02:52:00AM +0800, Conor Dooley wrote:
> > > On Tue, Sep 30, 2025 at 03:56:42PM +0800, Jun Guo wrote:
> > > > Add documentation for the optional 'fifo-width' device tree property
> > > > for the Cadence SPI controller.
> > > >
> > > > Signed-off-by: Jun Guo <jun.guo@cixtech.com>
> > > > ---
> > > >  .../devicetree/bindings/spi/spi-cadence.yaml          | 11 +++++++++++
> > > >  1 file changed, 11 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/spi/spi-cadence.yaml b/Documentation/devicetree/bindings/spi/spi-cadence.yaml
> > > > index 8de96abe9da1..b2e3f217473b 100644
> > > > --- a/Documentation/devicetree/bindings/spi/spi-cadence.yaml
> > > > +++ b/Documentation/devicetree/bindings/spi/spi-cadence.yaml
> > > > @@ -62,6 +62,17 @@ properties:
> > > >      items:
> > > >        - const: spi
> > > >
> > > > +  fifo-width:
> > > > +    description: |
> > > > +      This property specifies the FIFO data width (in bits) of the hardware.
> > > > +      It must be configured according to the actual FIFO width set during
> > > > +      the IP design. For instance, if the hardware FIFO is 32 bits wide,
> > > > +      this property should be set to 32.
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > +    minimum: 8
> > > > +    maximum: 32
> > > > +    default: 8
> > >
> > > I assume this differs from fifo-depth because this is the actual width
> > > of the registers rather than the number of elements of that width the
> > > FIFO can contain?
> >
> > Thank you for your review. You are absolutely correct. The `fifo-width`
> > indeed refers to the physical width of the FIFO data registers (e.g., 8,
> > 16, or 32 bits), whereas `fifo-depth` describes how many elements of
> >  that width the FIFO can store.
> >
> > > However, this isn't something defined as common in spi-controller.yaml
> > > so you'll need a vendor prefix for the property if the property stays.
> > > This does, however, seem like something that can just be determined by
> > > the compatible and that your omission of a soc-specific one is what's
> > > lead you to introduce this property. Why not just use a sky1-specific
> > > compatible here?
> >
> > You raise an excellent point, and I initially had the same thought. However,
> > after further consideration, I realized that the IP of Cadence SPI actually
> > supports configurable FIFO width as a feature. The choice of using 8-bit,
> > 16-bit, or 32-bit FIFO width can be made by the SoC integrator based on
> > their specific requirements. This is therefore a feature of the Cadence IP
> > itself, rather than a chip vendor-specific design constraint.
> >
> > For this reason, I believe defining a common `fifo-width` property for
> > Cadence SPI controllers is more appropriate, as it allows any SoC using
> > this IP with different FIFO width configurations to utilize this property,
> > without needing to create a specific compatible string for each SoC variant.

>Except, you do need to create a soc-specific compatible string for every
> device, the fact that you didn't add one for your sky1 SoC was a mistake
> that you should fix. SoC-specific compatibles are a requirement.
> The "cnds,spi-r1p6" compatible seems to be used on Xilinx platforms,
> including a zynq platform that should probably be using the zynq
> soc-specific compatible. r1p6 sounds like some sort of version info, is
> that the version you are even using?

> Once you have added a compatible for the sky1, this property is not
> needed, since the depth can be determined from that. Any other user that
> wants to use non-default depths can also use their soc-specific
> compatibles for that purpose.

Thank you for the further explanation and suggestions. I see your
point now, and I will rethink the approach for this patch.

> >
> > Thank you for your valuable time and insightful suggestions. I look forward to
> > your further feedback on this approach.

Best wishes,
Jun Guo



________________________________________
发件人: Conor Dooley <conor@kernel.org>
发送时间: 2025年10月2日 2:04
收件人: Jun Guo
抄送: Peter Chen; Fugang Duan; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; broonie@kernel.org; linux-spi@vger.kernel.org; michal.simek@amd.com; cix-kernel-upstream; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
主题: Re: 回复: [PATCH 1/3] dt-bindings: spi: spi-cadence: document optional fifo-width DT property

EXTERNAL EMAIL

回复: 回复: [PATCH 1/3] dt-bindings: spi: spi-cadence: document optional fifo-width DT property

[Jun Guo]

On Tue, Oct 02, 2025 at 02:04:00AM +0000, Conor Dooley wrote:
> On Wed, Oct 01, 2025 at 02:36:44PM +0000, Jun Guo wrote:
> > On Tue, Oct 01, 2025 at 02:52:00AM +0800, Conor Dooley wrote:
> > > On Tue, Sep 30, 2025 at 03:56:42PM +0800, Jun Guo wrote:
> > > > Add documentation for the optional 'fifo-width' device tree property
> > > > for the Cadence SPI controller.
> > > >
> > > > Signed-off-by: Jun Guo <jun.guo@cixtech.com>
> > > > ---
> > > >  .../devicetree/bindings/spi/spi-cadence.yaml          | 11 +++++++++++
> > > >  1 file changed, 11 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/spi/spi-cadence.yaml b/Documentation/devicetree/bindings/spi/spi-cadence.yaml
> > > > index 8de96abe9da1..b2e3f217473b 100644
> > > > --- a/Documentation/devicetree/bindings/spi/spi-cadence.yaml
> > > > +++ b/Documentation/devicetree/bindings/spi/spi-cadence.yaml
> > > > @@ -62,6 +62,17 @@ properties:
> > > >      items:
> > > >        - const: spi
> > > >
> > > > +  fifo-width:
> > > > +    description: |
> > > > +      This property specifies the FIFO data width (in bits) of the hardware.
> > > > +      It must be configured according to the actual FIFO width set during
> > > > +      the IP design. For instance, if the hardware FIFO is 32 bits wide,
> > > > +      this property should be set to 32.
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > +    minimum: 8
> > > > +    maximum: 32
> > > > +    default: 8
> > >
> > > I assume this differs from fifo-depth because this is the actual width
> > > of the registers rather than the number of elements of that width the
> > > FIFO can contain?
> >
> > Thank you for your review. You are absolutely correct. The `fifo-width`
> > indeed refers to the physical width of the FIFO data registers (e.g., 8,
> > 16, or 32 bits), whereas `fifo-depth` describes how many elements of
> >  that width the FIFO can store.
> >
> > > However, this isn't something defined as common in spi-controller.yaml
> > > so you'll need a vendor prefix for the property if the property stays.
> > > This does, however, seem like something that can just be determined by
> > > the compatible and that your omission of a soc-specific one is what's
> > > lead you to introduce this property. Why not just use a sky1-specific
> > > compatible here?
> >
> > You raise an excellent point, and I initially had the same thought. However,
> > after further consideration, I realized that the IP of Cadence SPI actually
> > supports configurable FIFO width as a feature. The choice of using 8-bit,
> > 16-bit, or 32-bit FIFO width can be made by the SoC integrator based on
> > their specific requirements. This is therefore a feature of the Cadence IP
> > itself, rather than a chip vendor-specific design constraint.
> >
> > For this reason, I believe defining a common `fifo-width` property for
> > Cadence SPI controllers is more appropriate, as it allows any SoC using
> > this IP with different FIFO width configurations to utilize this property,
> > without needing to create a specific compatible string for each SoC variant.

> Except, you do need to create a soc-specific compatible string for every
> device, the fact that you didn't add one for your sky1 SoC was a mistake
> that you should fix. SoC-specific compatibles are a requirement.
> The "cnds,spi-r1p6" compatible seems to be used on Xilinx platforms,
> including a zynq platform that should probably be using the zynq
> soc-specific compatible. r1p6 sounds like some sort of version info, is
> that the version you are even using?

> Once you have added a compatible for the sky1, this property is not
> needed, since the depth can be determined from that. Any other user that
> wants to use non-default depths can also use their soc-specific
> compatibles for that purpose.

After further consideration and reviewing how similar cases were resolved
for other IPs, I now believe your approach is correct, We should indeed add
a cix,sky1-xxx compatible string to this YAML file to indicate that our SoC
supports this IP. However, the "fifo-width" is indeed a configurable property
of the IP itself. By using the DTSI and the binding document, one can understand
which "fifo-width" the SoC platform supports without needing to delve into the
code. This implementation is similar to existing binding documentation examples
like reg-io-width
(Documentation/devicetree/bindings/serial/pl011.yaml)
and
snps,incr-burst-type-adjustment
(Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml).

> >
> > Thank you for your valuable time and insightful suggestions. I look forward to
> > your further feedback on this approach.

Best wishes,
Jun Guo



________________________________________
发件人: Conor Dooley <conor@kernel.org>
发送时间: 2025年10月2日 2:04
收件人: Jun Guo
抄送: Peter Chen; Fugang Duan; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; broonie@kernel.org; linux-spi@vger.kernel.org; michal.simek@amd.com; cix-kernel-upstream; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
主题: Re: 回复: [PATCH 1/3] dt-bindings: spi: spi-cadence: document optional fifo-width DT property

EXTERNAL EMAIL

回复: 回复: [PATCH 1/3] dt-bindings: spi: spi-cadence: document optional fifo-width DT property

[Jun Guo]

On 03/10/25 22:58, Jun Guo wrote:
> On Tue, Oct 02, 2025 at 02:04:00AM +0000, Conor Dooley wrote:
>> On Wed, Oct 01, 2025 at 02:36:44PM +0000, Jun Guo wrote:
>>> On Tue, Oct 01, 2025 at 02:52:00AM +0800, Conor Dooley wrote:
>>>> On Tue, Sep 30, 2025 at 03:56:42PM +0800, Jun Guo wrote:
>>>>> Add documentation for the optional 'fifo-width' device tree property
>>>>> for the Cadence SPI controller.
>>>>>
>>>>> Signed-off-by: Jun Guo <jun.guo@cixtech.com>
>>>>> ---
>>>>>   .../devicetree/bindings/spi/spi-cadence.yaml          | 11 +++++++++++
>>>>>   1 file changed, 11 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/spi/spi-cadence.yaml b/Documentation/devicetree/bindings/spi/spi-cadence.yaml
>>>>> index 8de96abe9da1..b2e3f217473b 100644
>>>>> --- a/Documentation/devicetree/bindings/spi/spi-cadence.yaml
>>>>> +++ b/Documentation/devicetree/bindings/spi/spi-cadence.yaml
>>>>> @@ -62,6 +62,17 @@ properties:
>>>>>       items:
>>>>>         - const: spi
>>>>>
>>>>> +  fifo-width:
>>>>> +    description: |
>>>>> +      This property specifies the FIFO data width (in bits) of the hardware.
>>>>> +      It must be configured according to the actual FIFO width set during
>>>>> +      the IP design. For instance, if the hardware FIFO is 32 bits wide,
>>>>> +      this property should be set to 32.
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>> +    minimum: 8
>>>>> +    maximum: 32
>>>>> +    default: 8
>>>>
>>>> I assume this differs from fifo-depth because this is the actual width
>>>> of the registers rather than the number of elements of that width the
>>>> FIFO can contain?
>>>
>>> Thank you for your review. You are absolutely correct. The `fifo-width`
>>> indeed refers to the physical width of the FIFO data registers (e.g., 8,
>>> 16, or 32 bits), whereas `fifo-depth` describes how many elements of
>>>   that width the FIFO can store.
>>>
>>>> However, this isn't something defined as common in spi-controller.yaml
>>>> so you'll need a vendor prefix for the property if the property stays.
>>>> This does, however, seem like something that can just be determined by
>>>> the compatible and that your omission of a soc-specific one is what's
>>>> lead you to introduce this property. Why not just use a sky1-specific
>>>> compatible here?
>>>
>>> You raise an excellent point, and I initially had the same thought. However,
>>> after further consideration, I realized that the IP of Cadence SPI actually
>>> supports configurable FIFO width as a feature. The choice of using 8-bit,
>>> 16-bit, or 32-bit FIFO width can be made by the SoC integrator based on
>>> their specific requirements. This is therefore a feature of the Cadence IP
>>> itself, rather than a chip vendor-specific design constraint.
>>>
>>> For this reason, I believe defining a common `fifo-width` property for
>>> Cadence SPI controllers is more appropriate, as it allows any SoC using
>>> this IP with different FIFO width configurations to utilize this property,
>>> without needing to create a specific compatible string for each SoC variant.
>
>> Except, you do need to create a soc-specific compatible string for every
>> device, the fact that you didn't add one for your sky1 SoC was a mistake
>> that you should fix. SoC-specific compatibles are a requirement.
>> The "cnds,spi-r1p6" compatible seems to be used on Xilinx platforms,
>> including a zynq platform that should probably be using the zynq
>> soc-specific compatible. r1p6 sounds like some sort of version info, is
>> that the version you are even using?
>
>> Once you have added a compatible for the sky1, this property is not
>> needed, since the depth can be determined from that. Any other user that
>> wants to use non-default depths can also use their soc-specific
>> compatibles for that purpose.
>
> After further consideration and reviewing how similar cases were resolved
> for other IPs, I now believe your approach is correct, We should indeed add
> a cix,sky1-xxx compatible string to this YAML file to indicate that our SoC
> supports this IP. However, the "fifo-width" is indeed a configurable property
> of the IP itself. By using the DTSI and the binding document, one can understand
> which "fifo-width" the SoC platform supports without needing to delve into the
> code. This implementation is similar to existing binding documentation examples
> like reg-io-width
> (Documentation/devicetree/bindings/serial/pl011.yaml)
> and
> snps,incr-burst-type-adjustment
> (Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml).
>
Hi Conor. Just a gentle ping. Would you mind sharing your further thoughts on
this idea when you have a moment? Looking forward to your feedback so we can
move this patch forward.
>>>
>>> Thank you for your valuable time and insightful suggestions. I look forward to
>>> your further feedback on this approach.

Best wishes,
Jun Guo



________________________________________
发件人: Jun Guo <Jun.Guo@cixtech.com>
发送时间: 2025年10月3日 22:58
收件人: Conor Dooley
抄送: Peter Chen; Fugang Duan; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; broonie@kernel.org; linux-spi@vger.kernel.org; michal.simek@amd.com; cix-kernel-upstream; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
主题: 回复: 回复: [PATCH 1/3] dt-bindings: spi: spi-cadence: document optional fifo-width DT property

On Tue, Oct 02, 2025 at 02:04:00AM +0000, Conor Dooley wrote:
> On Wed, Oct 01, 2025 at 02:36:44PM +0000, Jun Guo wrote:
> > On Tue, Oct 01, 2025 at 02:52:00AM +0800, Conor Dooley wrote:
> > > On Tue, Sep 30, 2025 at 03:56:42PM +0800, Jun Guo wrote:
> > > > Add documentation for the optional 'fifo-width' device tree property
> > > > for the Cadence SPI controller.
> > > >
> > > > Signed-off-by: Jun Guo <jun.guo@cixtech.com>
> > > > ---
> > > >  .../devicetree/bindings/spi/spi-cadence.yaml          | 11 +++++++++++
> > > >  1 file changed, 11 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/spi/spi-cadence.yaml b/Documentation/devicetree/bindings/spi/spi-cadence.yaml
> > > > index 8de96abe9da1..b2e3f217473b 100644
> > > > --- a/Documentation/devicetree/bindings/spi/spi-cadence.yaml
> > > > +++ b/Documentation/devicetree/bindings/spi/spi-cadence.yaml
> > > > @@ -62,6 +62,17 @@ properties:
> > > >      items:
> > > >        - const: spi
> > > >
> > > > +  fifo-width:
> > > > +    description: |
> > > > +      This property specifies the FIFO data width (in bits) of the hardware.
> > > > +      It must be configured according to the actual FIFO width set during
> > > > +      the IP design. For instance, if the hardware FIFO is 32 bits wide,
> > > > +      this property should be set to 32.
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > +    minimum: 8
> > > > +    maximum: 32
> > > > +    default: 8
> > >
> > > I assume this differs from fifo-depth because this is the actual width
> > > of the registers rather than the number of elements of that width the
> > > FIFO can contain?
> >
> > Thank you for your review. You are absolutely correct. The `fifo-width`
> > indeed refers to the physical width of the FIFO data registers (e.g., 8,
> > 16, or 32 bits), whereas `fifo-depth` describes how many elements of
> >  that width the FIFO can store.
> >
> > > However, this isn't something defined as common in spi-controller.yaml
> > > so you'll need a vendor prefix for the property if the property stays.
> > > This does, however, seem like something that can just be determined by
> > > the compatible and that your omission of a soc-specific one is what's
> > > lead you to introduce this property. Why not just use a sky1-specific
> > > compatible here?
> >
> > You raise an excellent point, and I initially had the same thought. However,
> > after further consideration, I realized that the IP of Cadence SPI actually
> > supports configurable FIFO width as a feature. The choice of using 8-bit,
> > 16-bit, or 32-bit FIFO width can be made by the SoC integrator based on
> > their specific requirements. This is therefore a feature of the Cadence IP
> > itself, rather than a chip vendor-specific design constraint.
> >
> > For this reason, I believe defining a common `fifo-width` property for
> > Cadence SPI controllers is more appropriate, as it allows any SoC using
> > this IP with different FIFO width configurations to utilize this property,
> > without needing to create a specific compatible string for each SoC variant.

> Except, you do need to create a soc-specific compatible string for every
> device, the fact that you didn't add one for your sky1 SoC was a mistake
> that you should fix. SoC-specific compatibles are a requirement.
> The "cnds,spi-r1p6" compatible seems to be used on Xilinx platforms,
> including a zynq platform that should probably be using the zynq
> soc-specific compatible. r1p6 sounds like some sort of version info, is
> that the version you are even using?

> Once you have added a compatible for the sky1, this property is not
> needed, since the depth can be determined from that. Any other user that
> wants to use non-default depths can also use their soc-specific
> compatibles for that purpose.

After further consideration and reviewing how similar cases were resolved
for other IPs, I now believe your approach is correct, We should indeed add
a cix,sky1-xxx compatible string to this YAML file to indicate that our SoC
supports this IP. However, the "fifo-width" is indeed a configurable property
of the IP itself. By using the DTSI and the binding document, one can understand
which "fifo-width" the SoC platform supports without needing to delve into the
code. This implementation is similar to existing binding documentation examples
like reg-io-width
(Documentation/devicetree/bindings/serial/pl011.yaml)
and
snps,incr-burst-type-adjustment
(Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml).

> >
> > Thank you for your valuable time and insightful suggestions. I look forward to
> > your further feedback on this approach.

Best wishes,
Jun Guo



________________________________________
发件人: Conor Dooley <conor@kernel.org>
发送时间: 2025年10月2日 2:04
收件人: Jun Guo
抄送: Peter Chen; Fugang Duan; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; broonie@kernel.org; linux-spi@vger.kernel.org; michal.simek@amd.com; cix-kernel-upstream; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
主题: Re: 回复: [PATCH 1/3] dt-bindings: spi: spi-cadence: document optional fifo-width DT property

EXTERNAL EMAIL

Re: 回复: 回复: [PATCH 1/3] dt-bindings: spi: spi-cadence: document optional fifo-width DT property

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 6222 bytes --]

On Thu, Oct 09, 2025 at 09:51:08AM +0000, Jun Guo wrote:
> On 03/10/25 22:58, Jun Guo wrote:
> > On Tue, Oct 02, 2025 at 02:04:00AM +0000, Conor Dooley wrote:
> >> On Wed, Oct 01, 2025 at 02:36:44PM +0000, Jun Guo wrote:
> >>> On Tue, Oct 01, 2025 at 02:52:00AM +0800, Conor Dooley wrote:
> >>>> On Tue, Sep 30, 2025 at 03:56:42PM +0800, Jun Guo wrote:
> >>>>> Add documentation for the optional 'fifo-width' device tree property
> >>>>> for the Cadence SPI controller.
> >>>>>
> >>>>> Signed-off-by: Jun Guo <jun.guo@cixtech.com>
> >>>>> ---
> >>>>>   .../devicetree/bindings/spi/spi-cadence.yaml          | 11 +++++++++++
> >>>>>   1 file changed, 11 insertions(+)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/spi/spi-cadence.yaml b/Documentation/devicetree/bindings/spi/spi-cadence.yaml
> >>>>> index 8de96abe9da1..b2e3f217473b 100644
> >>>>> --- a/Documentation/devicetree/bindings/spi/spi-cadence.yaml
> >>>>> +++ b/Documentation/devicetree/bindings/spi/spi-cadence.yaml
> >>>>> @@ -62,6 +62,17 @@ properties:
> >>>>>       items:
> >>>>>         - const: spi
> >>>>>
> >>>>> +  fifo-width:
> >>>>> +    description: |
> >>>>> +      This property specifies the FIFO data width (in bits) of the hardware.
> >>>>> +      It must be configured according to the actual FIFO width set during
> >>>>> +      the IP design. For instance, if the hardware FIFO is 32 bits wide,
> >>>>> +      this property should be set to 32.
> >>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
> >>>>> +    minimum: 8
> >>>>> +    maximum: 32
> >>>>> +    default: 8
> >>>>
> >>>> I assume this differs from fifo-depth because this is the actual width
> >>>> of the registers rather than the number of elements of that width the
> >>>> FIFO can contain?
> >>>
> >>> Thank you for your review. You are absolutely correct. The `fifo-width`
> >>> indeed refers to the physical width of the FIFO data registers (e.g., 8,
> >>> 16, or 32 bits), whereas `fifo-depth` describes how many elements of
> >>>   that width the FIFO can store.
> >>>
> >>>> However, this isn't something defined as common in spi-controller.yaml
> >>>> so you'll need a vendor prefix for the property if the property stays.
> >>>> This does, however, seem like something that can just be determined by
> >>>> the compatible and that your omission of a soc-specific one is what's
> >>>> lead you to introduce this property. Why not just use a sky1-specific
> >>>> compatible here?
> >>>
> >>> You raise an excellent point, and I initially had the same thought. However,
> >>> after further consideration, I realized that the IP of Cadence SPI actually
> >>> supports configurable FIFO width as a feature. The choice of using 8-bit,
> >>> 16-bit, or 32-bit FIFO width can be made by the SoC integrator based on
> >>> their specific requirements. This is therefore a feature of the Cadence IP
> >>> itself, rather than a chip vendor-specific design constraint.
> >>>
> >>> For this reason, I believe defining a common `fifo-width` property for
> >>> Cadence SPI controllers is more appropriate, as it allows any SoC using
> >>> this IP with different FIFO width configurations to utilize this property,
> >>> without needing to create a specific compatible string for each SoC variant.
> >
> >> Except, you do need to create a soc-specific compatible string for every
> >> device, the fact that you didn't add one for your sky1 SoC was a mistake
> >> that you should fix. SoC-specific compatibles are a requirement.
> >> The "cnds,spi-r1p6" compatible seems to be used on Xilinx platforms,
> >> including a zynq platform that should probably be using the zynq
> >> soc-specific compatible. r1p6 sounds like some sort of version info, is
> >> that the version you are even using?
> >
> >> Once you have added a compatible for the sky1, this property is not
> >> needed, since the depth can be determined from that. Any other user that
> >> wants to use non-default depths can also use their soc-specific
> >> compatibles for that purpose.
> >
> > After further consideration and reviewing how similar cases were resolved
> > for other IPs, I now believe your approach is correct, We should indeed add
> > a cix,sky1-xxx compatible string to this YAML file to indicate that our SoC
> > supports this IP. However, the "fifo-width" is indeed a configurable property
> > of the IP itself. By using the DTSI and the binding document, one can understand
> > which "fifo-width" the SoC platform supports without needing to delve into the

Who actually is going to read the devicetree to figure out what the
fifo-wdith for the platform is? I don't see how it is relevant outside
of the specific driver implementation, as it just controls how many
bits the loop in cdns_spi_process_fifo() deals with per iteration.

Devicetree is not about publishing information to the world about your
device, it doesn't contain the complete programming model information
etc. It is there to communicate non-discoverable information about the
device to the operating system. The fifo-width can be determined from
the compatible and is therefore not needed to be put in the devicetree
explicitly.

> > code. This implementation is similar to existing binding documentation examples
> > like reg-io-width
> > (Documentation/devicetree/bindings/serial/pl011.yaml)
> > and
> > snps,incr-burst-type-adjustment
> > (Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml).

To be honest, a bunch of what is in this dwc3 binding is crap and should
not have been accepted. There's dozens of properties in that file, and
lots of them should have been inferred from a device-specific compatible.

For pl011, I don't really know why the property is there, the provided
explanations are pretty lacking. Maybe a compatible didn't work because
Xilinx needed it for FPGA fabric implementations where the io width was
not set in stone? If I were reviewing that patch today, I would question
their need for it too.

> Hi Conor. Just a gentle ping. Would you mind sharing your further thoughts on
> this idea when you have a moment? Looking forward to your feedback so we can
> move this patch forward.

I didn't reply to the last mail because I had nothing new to say.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 2/3] spi: spi-cadence: supports transmission with bits_per_word of 16 and 32

[Jun Guo]

The Cadence IP supports configurable FIFO data widths of 16 bits or
32 bits during integration. The default FIFO data width is 8 bits.
If the chip design modifies the FIFO data width to 16 bits or 32 bits,
the fifo-width property can be configured in the firmware (DT/ACPI)
to enable the driver to support data transfers at the corresponding width.

Signed-off-by: Jun Guo <jun.guo@cixtech.com>
---
 drivers/spi/spi-cadence.c | 125 ++++++++++++++++++++++++++++++++++----
 1 file changed, 112 insertions(+), 13 deletions(-)

diff --git a/drivers/spi/spi-cadence.c b/drivers/spi/spi-cadence.c
index 5ae09b21d23a..e91c6afbece9 100644
--- a/drivers/spi/spi-cadence.c
+++ b/drivers/spi/spi-cadence.c
@@ -109,9 +109,11 @@
  * @rxbuf:		Pointer to the RX buffer
  * @tx_bytes:		Number of bytes left to transfer
  * @rx_bytes:		Number of bytes requested
+ * @n_bytes:		Number of bytes per word
  * @dev_busy:		Device busy flag
  * @is_decoded_cs:	Flag for decoder property set or not
  * @tx_fifo_depth:	Depth of the TX FIFO
+ * @fifo_width:		Width of the FIFO
  * @rstc:		Optional reset control for SPI controller
  */
 struct cdns_spi {
@@ -120,16 +122,25 @@ struct cdns_spi {
 	struct clk *pclk;
 	unsigned int clk_rate;
 	u32 speed_hz;
-	const u8 *txbuf;
-	u8 *rxbuf;
+	const void *txbuf;
+	void *rxbuf;
 	int tx_bytes;
 	int rx_bytes;
+	u8 n_bytes;
 	u8 dev_busy;
 	u32 is_decoded_cs;
 	unsigned int tx_fifo_depth;
+	unsigned int fifo_width;
 	struct reset_control *rstc;
 };
 
+enum cdns_spi_frame_n_bytes {
+	CDNS_SPI_N_BYTES_NULL = 0,
+	CDNS_SPI_N_BYTES_U8 = 1,
+	CDNS_SPI_N_BYTES_U16 = 2,
+	CDNS_SPI_N_BYTES_U32 = 4
+};
+
 /* Macros for the SPI controller read/write */
 static inline u32 cdns_spi_read(struct cdns_spi *xspi, u32 offset)
 {
@@ -305,6 +316,78 @@ static int cdns_spi_setup_transfer(struct spi_device *spi,
 	return 0;
 }
 
+static u8 cdns_spi_n_bytes(struct spi_transfer *transfer)
+{
+	if (transfer->bits_per_word <= 8)
+		return CDNS_SPI_N_BYTES_U8;
+	else if (transfer->bits_per_word <= 16)
+		return CDNS_SPI_N_BYTES_U16;
+	else
+		return CDNS_SPI_N_BYTES_U32;
+}
+
+static inline void cdns_spi_reader(struct cdns_spi *xspi)
+{
+	u32 rxw = 0;
+
+	if (xspi->rxbuf && !IS_ALIGNED((uintptr_t)xspi->rxbuf, xspi->n_bytes)) {
+		pr_err("%s: rxbuf address is not aligned for %d bytes\n",
+		       __func__, xspi->n_bytes);
+		return;
+	}
+
+	rxw = cdns_spi_read(xspi, CDNS_SPI_RXD);
+	if (xspi->rxbuf) {
+		switch (xspi->n_bytes) {
+		case CDNS_SPI_N_BYTES_U8:
+			*(u8 *)xspi->rxbuf = rxw;
+			break;
+		case CDNS_SPI_N_BYTES_U16:
+			*(u16 *)xspi->rxbuf = rxw;
+			break;
+		case CDNS_SPI_N_BYTES_U32:
+			*(u32 *)xspi->rxbuf = rxw;
+			break;
+		default:
+			pr_err("%s invalid n_bytes %d\n", __func__,
+			       xspi->n_bytes);
+			return;
+		}
+		xspi->rxbuf = (u8 *)xspi->rxbuf + xspi->n_bytes;
+	}
+}
+
+static inline void cdns_spi_writer(struct cdns_spi *xspi)
+{
+	u32 txw = 0;
+
+	if (xspi->txbuf && !IS_ALIGNED((uintptr_t)xspi->txbuf, xspi->n_bytes)) {
+		pr_err("%s: txbuf address is not aligned for %d bytes\n",
+		       __func__, xspi->n_bytes);
+		return;
+	}
+
+	if (xspi->txbuf) {
+		switch (xspi->n_bytes) {
+		case CDNS_SPI_N_BYTES_U8:
+			txw = *(u8 *)xspi->txbuf;
+			break;
+		case CDNS_SPI_N_BYTES_U16:
+			txw = *(u16 *)xspi->txbuf;
+			break;
+		case CDNS_SPI_N_BYTES_U32:
+			txw = *(u32 *)xspi->txbuf;
+			break;
+		default:
+			pr_err("%s invalid n_bytes %d\n", __func__,
+			       xspi->n_bytes);
+			return;
+		}
+		cdns_spi_write(xspi, CDNS_SPI_TXD, txw);
+		xspi->txbuf = (u8 *)xspi->txbuf + xspi->n_bytes;
+	}
+}
+
 /**
  * cdns_spi_process_fifo - Fills the TX FIFO, and drain the RX FIFO
  * @xspi:	Pointer to the cdns_spi structure
@@ -321,23 +404,14 @@ static void cdns_spi_process_fifo(struct cdns_spi *xspi, int ntx, int nrx)
 
 	while (ntx || nrx) {
 		if (nrx) {
-			u8 data = cdns_spi_read(xspi, CDNS_SPI_RXD);
-
-			if (xspi->rxbuf)
-				*xspi->rxbuf++ = data;
-
+			cdns_spi_reader(xspi);
 			nrx--;
 		}
 
 		if (ntx) {
-			if (xspi->txbuf)
-				cdns_spi_write(xspi, CDNS_SPI_TXD, *xspi->txbuf++);
-			else
-				cdns_spi_write(xspi, CDNS_SPI_TXD, 0);
-
+			cdns_spi_writer(xspi);
 			ntx--;
 		}
-
 	}
 }
 
@@ -454,6 +528,10 @@ static int cdns_transfer_one(struct spi_controller *ctlr,
 	if (cdns_spi_read(xspi, CDNS_SPI_ISR) & CDNS_SPI_IXR_TXFULL)
 		udelay(10);
 
+	xspi->n_bytes = cdns_spi_n_bytes(transfer);
+	xspi->tx_bytes = DIV_ROUND_UP(xspi->tx_bytes, xspi->n_bytes);
+	xspi->rx_bytes = DIV_ROUND_UP(xspi->rx_bytes, xspi->n_bytes);
+
 	cdns_spi_process_fifo(xspi, xspi->tx_fifo_depth, 0);
 
 	cdns_spi_write(xspi, CDNS_SPI_IER, CDNS_SPI_IXR_DEFAULT);
@@ -654,6 +732,27 @@ static int cdns_spi_probe(struct platform_device *pdev)
 	ctlr->mode_bits = SPI_CPOL | SPI_CPHA;
 	ctlr->bits_per_word_mask = SPI_BPW_MASK(8);
 
+	if (!device_property_read_u32(&pdev->dev, "fifo-width",
+				      &xspi->fifo_width)) {
+		switch (xspi->fifo_width) {
+		case 8:
+			break;
+		case 16:
+			ctlr->bits_per_word_mask |= SPI_BPW_MASK(16);
+			break;
+		case 32:
+			ctlr->bits_per_word_mask |=
+				(SPI_BPW_MASK(16) | SPI_BPW_MASK(32));
+			break;
+		default:
+			dev_warn(
+				&pdev->dev,
+				"Unsupported FIFO width: %u. Using default bit-width mask.\n",
+				xspi->fifo_width);
+			break;
+		}
+	}
+
 	if (!spi_controller_is_target(ctlr)) {
 		ctlr->mode_bits |=  SPI_CS_HIGH;
 		ctlr->set_cs = cdns_spi_chipselect;
-- 
2.34.1

回复: [PATCH 2/3] spi: spi-cadence: supports transmission with bits_per_word of 16 and 32

[Jun Guo]

On 9/30/2025 3:56 PM, Jun Guo wrote:
> The Cadence IP supports configurable FIFO data widths of 16 bits or
> 32 bits during integration. The default FIFO data width is 8 bits.
> If the chip design modifies the FIFO data width to 16 bits or 32 bits,
> the fifo-width property can be configured in the firmware (DT/ACPI)
> to enable the driver to support data transfers at the corresponding width.
>
> Signed-off-by: Jun Guo <jun.guo@cixtech.com>
> ---
>   drivers/spi/spi-cadence.c | 125 ++++++++++++++++++++++++++++++++++----
>   1 file changed, 112 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/spi/spi-cadence.c b/drivers/spi/spi-cadence.c
> index 5ae09b21d23a..e91c6afbece9 100644
> --- a/drivers/spi/spi-cadence.c
> +++ b/drivers/spi/spi-cadence.c
> @@ -109,9 +109,11 @@
>    * @rxbuf:          Pointer to the RX buffer
>    * @tx_bytes:               Number of bytes left to transfer
>    * @rx_bytes:               Number of bytes requested
> + * @n_bytes:         Number of bytes per word
>    * @dev_busy:               Device busy flag
>    * @is_decoded_cs:  Flag for decoder property set or not
>    * @tx_fifo_depth:  Depth of the TX FIFO
> + * @fifo_width:              Width of the FIFO
>    * @rstc:           Optional reset control for SPI controller
>    */
>   struct cdns_spi {
> @@ -120,16 +122,25 @@ struct cdns_spi {
>       struct clk *pclk;
>       unsigned int clk_rate;
>       u32 speed_hz;
> -     const u8 *txbuf;
> -     u8 *rxbuf;
> +     const void *txbuf;
> +     void *rxbuf;
>       int tx_bytes;
>       int rx_bytes;
> +     u8 n_bytes;
>       u8 dev_busy;
>       u32 is_decoded_cs;
>       unsigned int tx_fifo_depth;
> +     unsigned int fifo_width;
>       struct reset_control *rstc;
>   };
>
> +enum cdns_spi_frame_n_bytes {
> +     CDNS_SPI_N_BYTES_NULL = 0,
> +     CDNS_SPI_N_BYTES_U8 = 1,
> +     CDNS_SPI_N_BYTES_U16 = 2,
> +     CDNS_SPI_N_BYTES_U32 = 4
> +};
> +
>   /* Macros for the SPI controller read/write */
>   static inline u32 cdns_spi_read(struct cdns_spi *xspi, u32 offset)
>   {
> @@ -305,6 +316,78 @@ static int cdns_spi_setup_transfer(struct spi_device *spi,
>       return 0;
>   }
>
> +static u8 cdns_spi_n_bytes(struct spi_transfer *transfer)
> +{
> +     if (transfer->bits_per_word <= 8)
> +             return CDNS_SPI_N_BYTES_U8;
> +     else if (transfer->bits_per_word <= 16)
> +             return CDNS_SPI_N_BYTES_U16;
> +     else
> +             return CDNS_SPI_N_BYTES_U32;
> +}
> +
> +static inline void cdns_spi_reader(struct cdns_spi *xspi)
> +{
> +     u32 rxw = 0;
> +
> +     if (xspi->rxbuf && !IS_ALIGNED((uintptr_t)xspi->rxbuf, xspi->n_bytes)) {
> +             pr_err("%s: rxbuf address is not aligned for %d bytes\n",
> +                    __func__, xspi->n_bytes);
> +             return;
> +     }
> +
> +     rxw = cdns_spi_read(xspi, CDNS_SPI_RXD);
> +     if (xspi->rxbuf) {
> +             switch (xspi->n_bytes) {
> +             case CDNS_SPI_N_BYTES_U8:
> +                     *(u8 *)xspi->rxbuf = rxw;
> +                     break;
> +             case CDNS_SPI_N_BYTES_U16:
> +                     *(u16 *)xspi->rxbuf = rxw;
> +                     break;
> +             case CDNS_SPI_N_BYTES_U32:
> +                     *(u32 *)xspi->rxbuf = rxw;
> +                     break;
> +             default:
> +                     pr_err("%s invalid n_bytes %d\n", __func__,
> +                            xspi->n_bytes);
> +                     return;
> +             }
> +             xspi->rxbuf = (u8 *)xspi->rxbuf + xspi->n_bytes;
> +     }
> +}
> +
> +static inline void cdns_spi_writer(struct cdns_spi *xspi)
> +{
> +     u32 txw = 0;
> +
> +     if (xspi->txbuf && !IS_ALIGNED((uintptr_t)xspi->txbuf, xspi->n_bytes)) {
> +             pr_err("%s: txbuf address is not aligned for %d bytes\n",
> +                    __func__, xspi->n_bytes);
> +             return;
> +     }
> +
> +     if (xspi->txbuf) {
> +             switch (xspi->n_bytes) {
> +             case CDNS_SPI_N_BYTES_U8:
> +                     txw = *(u8 *)xspi->txbuf;
> +                     break;
> +             case CDNS_SPI_N_BYTES_U16:
> +                     txw = *(u16 *)xspi->txbuf;
> +                     break;
> +             case CDNS_SPI_N_BYTES_U32:
> +                     txw = *(u32 *)xspi->txbuf;
> +                     break;
> +             default:
> +                     pr_err("%s invalid n_bytes %d\n", __func__,
> +                            xspi->n_bytes);
> +                     return;
> +             }
> +             cdns_spi_write(xspi, CDNS_SPI_TXD, txw);
> +             xspi->txbuf = (u8 *)xspi->txbuf + xspi->n_bytes;
> +     }
> +}
> +
>   /**
>    * cdns_spi_process_fifo - Fills the TX FIFO, and drain the RX FIFO
>    * @xspi:   Pointer to the cdns_spi structure
> @@ -321,23 +404,14 @@ static void cdns_spi_process_fifo(struct cdns_spi *xspi, int ntx, int nrx)
>
>       while (ntx || nrx) {
>               if (nrx) {
> -                     u8 data = cdns_spi_read(xspi, CDNS_SPI_RXD);
> -
> -                     if (xspi->rxbuf)
> -                             *xspi->rxbuf++ = data;
> -
> +                     cdns_spi_reader(xspi);
>                       nrx--;
>               }
>
>               if (ntx) {
> -                     if (xspi->txbuf)
> -                             cdns_spi_write(xspi, CDNS_SPI_TXD, *xspi->txbuf++);
> -                     else
> -                             cdns_spi_write(xspi, CDNS_SPI_TXD, 0);
> -
> +                     cdns_spi_writer(xspi);
>                       ntx--;
>               }
> -
>       }
>   }
>
> @@ -454,6 +528,10 @@ static int cdns_transfer_one(struct spi_controller *ctlr,
>       if (cdns_spi_read(xspi, CDNS_SPI_ISR) & CDNS_SPI_IXR_TXFULL)
>               udelay(10);
>
> +     xspi->n_bytes = cdns_spi_n_bytes(transfer);
> +     xspi->tx_bytes = DIV_ROUND_UP(xspi->tx_bytes, xspi->n_bytes);
> +     xspi->rx_bytes = DIV_ROUND_UP(xspi->rx_bytes, xspi->n_bytes);
> +
>       cdns_spi_process_fifo(xspi, xspi->tx_fifo_depth, 0);
>
>       cdns_spi_write(xspi, CDNS_SPI_IER, CDNS_SPI_IXR_DEFAULT);
> @@ -654,6 +732,27 @@ static int cdns_spi_probe(struct platform_device *pdev)
>       ctlr->mode_bits = SPI_CPOL | SPI_CPHA;
>       ctlr->bits_per_word_mask = SPI_BPW_MASK(8);
>
> +     if (!device_property_read_u32(&pdev->dev, "fifo-width",
> +                                   &xspi->fifo_width)) {
> +             switch (xspi->fifo_width) {
> +             case 8:
> +                     break;
> +             case 16:
> +                     ctlr->bits_per_word_mask |= SPI_BPW_MASK(16);
> +                     break;
> +             case 32:
> +                     ctlr->bits_per_word_mask |=
> +                             (SPI_BPW_MASK(16) | SPI_BPW_MASK(32));
> +                     break;
> +             default:
> +                     dev_warn(
> +                             &pdev->dev,
> +                             "Unsupported FIFO width: %u. Using default bit-width mask.\n",
> +                             xspi->fifo_width);
> +                     break;
> +             }
> +     }
> +
>       if (!spi_controller_is_target(ctlr)) {
>               ctlr->mode_bits |=  SPI_CS_HIGH;
>               ctlr->set_cs = cdns_spi_chipselect;
hi Mark, could you please take some time to review the current patch?
I look forward to your comments at your earliest convenience.

Best wishes,
Jun Guo



________________________________________
发件人: Jun Guo <jun.guo@cixtech.com>
发送时间: 2025年9月30日 15:56
收件人: Peter Chen; Fugang Duan; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; broonie@kernel.org
抄送: linux-spi@vger.kernel.org; michal.simek@amd.com; cix-kernel-upstream; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Jun Guo
主题: [PATCH 2/3] spi: spi-cadence: supports transmission with bits_per_word of 16 and 32

The Cadence IP supports configurable FIFO data widths of 16 bits or
32 bits during integration. The default FIFO data width is 8 bits.
If the chip design modifies the FIFO data width to 16 bits or 32 bits,
the fifo-width property can be configured in the firmware (DT/ACPI)
to enable the driver to support data transfers at the corresponding width.

Signed-off-by: Jun Guo <jun.guo@cixtech.com>
---
 drivers/spi/spi-cadence.c | 125 ++++++++++++++++++++++++++++++++++----
 1 file changed, 112 insertions(+), 13 deletions(-)

diff --git a/drivers/spi/spi-cadence.c b/drivers/spi/spi-cadence.c
index 5ae09b21d23a..e91c6afbece9 100644
--- a/drivers/spi/spi-cadence.c
+++ b/drivers/spi/spi-cadence.c
@@ -109,9 +109,11 @@
  * @rxbuf:             Pointer to the RX buffer
  * @tx_bytes:          Number of bytes left to transfer
  * @rx_bytes:          Number of bytes requested
+ * @n_bytes:           Number of bytes per word
  * @dev_busy:          Device busy flag
  * @is_decoded_cs:     Flag for decoder property set or not
  * @tx_fifo_depth:     Depth of the TX FIFO
+ * @fifo_width:                Width of the FIFO
  * @rstc:              Optional reset control for SPI controller
  */
 struct cdns_spi {
@@ -120,16 +122,25 @@ struct cdns_spi {
        struct clk *pclk;
        unsigned int clk_rate;
        u32 speed_hz;
-       const u8 *txbuf;
-       u8 *rxbuf;
+       const void *txbuf;
+       void *rxbuf;
        int tx_bytes;
        int rx_bytes;
+       u8 n_bytes;
        u8 dev_busy;
        u32 is_decoded_cs;
        unsigned int tx_fifo_depth;
+       unsigned int fifo_width;
        struct reset_control *rstc;
 };

+enum cdns_spi_frame_n_bytes {
+       CDNS_SPI_N_BYTES_NULL = 0,
+       CDNS_SPI_N_BYTES_U8 = 1,
+       CDNS_SPI_N_BYTES_U16 = 2,
+       CDNS_SPI_N_BYTES_U32 = 4
+};
+
 /* Macros for the SPI controller read/write */
 static inline u32 cdns_spi_read(struct cdns_spi *xspi, u32 offset)
 {
@@ -305,6 +316,78 @@ static int cdns_spi_setup_transfer(struct spi_device *spi,
        return 0;
 }

+static u8 cdns_spi_n_bytes(struct spi_transfer *transfer)
+{
+       if (transfer->bits_per_word <= 8)
+               return CDNS_SPI_N_BYTES_U8;
+       else if (transfer->bits_per_word <= 16)
+               return CDNS_SPI_N_BYTES_U16;
+       else
+               return CDNS_SPI_N_BYTES_U32;
+}
+
+static inline void cdns_spi_reader(struct cdns_spi *xspi)
+{
+       u32 rxw = 0;
+
+       if (xspi->rxbuf && !IS_ALIGNED((uintptr_t)xspi->rxbuf, xspi->n_bytes)) {
+               pr_err("%s: rxbuf address is not aligned for %d bytes\n",
+                      __func__, xspi->n_bytes);
+               return;
+       }
+
+       rxw = cdns_spi_read(xspi, CDNS_SPI_RXD);
+       if (xspi->rxbuf) {
+               switch (xspi->n_bytes) {
+               case CDNS_SPI_N_BYTES_U8:
+                       *(u8 *)xspi->rxbuf = rxw;
+                       break;
+               case CDNS_SPI_N_BYTES_U16:
+                       *(u16 *)xspi->rxbuf = rxw;
+                       break;
+               case CDNS_SPI_N_BYTES_U32:
+                       *(u32 *)xspi->rxbuf = rxw;
+                       break;
+               default:
+                       pr_err("%s invalid n_bytes %d\n", __func__,
+                              xspi->n_bytes);
+                       return;
+               }
+               xspi->rxbuf = (u8 *)xspi->rxbuf + xspi->n_bytes;
+       }
+}
+
+static inline void cdns_spi_writer(struct cdns_spi *xspi)
+{
+       u32 txw = 0;
+
+       if (xspi->txbuf && !IS_ALIGNED((uintptr_t)xspi->txbuf, xspi->n_bytes)) {
+               pr_err("%s: txbuf address is not aligned for %d bytes\n",
+                      __func__, xspi->n_bytes);
+               return;
+       }
+
+       if (xspi->txbuf) {
+               switch (xspi->n_bytes) {
+               case CDNS_SPI_N_BYTES_U8:
+                       txw = *(u8 *)xspi->txbuf;
+                       break;
+               case CDNS_SPI_N_BYTES_U16:
+                       txw = *(u16 *)xspi->txbuf;
+                       break;
+               case CDNS_SPI_N_BYTES_U32:
+                       txw = *(u32 *)xspi->txbuf;
+                       break;
+               default:
+                       pr_err("%s invalid n_bytes %d\n", __func__,
+                              xspi->n_bytes);
+                       return;
+               }
+               cdns_spi_write(xspi, CDNS_SPI_TXD, txw);
+               xspi->txbuf = (u8 *)xspi->txbuf + xspi->n_bytes;
+       }
+}
+
 /**
  * cdns_spi_process_fifo - Fills the TX FIFO, and drain the RX FIFO
  * @xspi:      Pointer to the cdns_spi structure
@@ -321,23 +404,14 @@ static void cdns_spi_process_fifo(struct cdns_spi *xspi, int ntx, int nrx)

        while (ntx || nrx) {
                if (nrx) {
-                       u8 data = cdns_spi_read(xspi, CDNS_SPI_RXD);
-
-                       if (xspi->rxbuf)
-                               *xspi->rxbuf++ = data;
-
+                       cdns_spi_reader(xspi);
                        nrx--;
                }

                if (ntx) {
-                       if (xspi->txbuf)
-                               cdns_spi_write(xspi, CDNS_SPI_TXD, *xspi->txbuf++);
-                       else
-                               cdns_spi_write(xspi, CDNS_SPI_TXD, 0);
-
+                       cdns_spi_writer(xspi);
                        ntx--;
                }
-
        }
 }

@@ -454,6 +528,10 @@ static int cdns_transfer_one(struct spi_controller *ctlr,
        if (cdns_spi_read(xspi, CDNS_SPI_ISR) & CDNS_SPI_IXR_TXFULL)
                udelay(10);

+       xspi->n_bytes = cdns_spi_n_bytes(transfer);
+       xspi->tx_bytes = DIV_ROUND_UP(xspi->tx_bytes, xspi->n_bytes);
+       xspi->rx_bytes = DIV_ROUND_UP(xspi->rx_bytes, xspi->n_bytes);
+
        cdns_spi_process_fifo(xspi, xspi->tx_fifo_depth, 0);

        cdns_spi_write(xspi, CDNS_SPI_IER, CDNS_SPI_IXR_DEFAULT);
@@ -654,6 +732,27 @@ static int cdns_spi_probe(struct platform_device *pdev)
        ctlr->mode_bits = SPI_CPOL | SPI_CPHA;
        ctlr->bits_per_word_mask = SPI_BPW_MASK(8);

+       if (!device_property_read_u32(&pdev->dev, "fifo-width",
+                                     &xspi->fifo_width)) {
+               switch (xspi->fifo_width) {
+               case 8:
+                       break;
+               case 16:
+                       ctlr->bits_per_word_mask |= SPI_BPW_MASK(16);
+                       break;
+               case 32:
+                       ctlr->bits_per_word_mask |=
+                               (SPI_BPW_MASK(16) | SPI_BPW_MASK(32));
+                       break;
+               default:
+                       dev_warn(
+                               &pdev->dev,
+                               "Unsupported FIFO width: %u. Using default bit-width mask.\n",
+                               xspi->fifo_width);
+                       break;
+               }
+       }
+
        if (!spi_controller_is_target(ctlr)) {
                ctlr->mode_bits |=  SPI_CS_HIGH;
                ctlr->set_cs = cdns_spi_chipselect;
--
2.34.1

Re: 回复: [PATCH 2/3] spi: spi-cadence: supports transmission with bits_per_word of 16 and 32

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1233 bytes --]

On Fri, Oct 10, 2025 at 07:50:24AM +0000, Jun Guo wrote:
> On 9/30/2025 3:56 PM, Jun Guo wrote:
> > The Cadence IP supports configurable FIFO data widths of 16 bits or
> > 32 bits during integration. The default FIFO data width is 8 bits.

Please delete unneeded context from mails when replying.  Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.

Please don't send content free pings and please allow a reasonable time
for review.  People get busy, go on holiday, attend conferences and so 
on so unless there is some reason for urgency (like critical bug fixes)
please allow at least a couple of weeks for review.  If there have been
review comments then people may be waiting for those to be addressed.

Sending content free pings adds to the mail volume (if they are seen at
all) which is often the problem and since they can't be reviewed
directly if something has gone wrong you'll have to resend the patches
anyway, so sending again is generally a better approach though there are
some other maintainers who like them - if in doubt look at how patches
for the subsystem are normally handled.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 3/3] arm64: dts: cix: add the fifo-width configuration field for cadence SPI

[Jun Guo]

The fifo-width is the FIFO data width (in bits) for the Cadence IP.
Configure it according to the FIFO data width set during the chip design.
The current design of sky1 has a FIFO data width of 32 bits.

Signed-off-by: Jun Guo <jun.guo@cixtech.com>
---
 arch/arm64/boot/dts/cix/sky1.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/cix/sky1.dtsi b/arch/arm64/boot/dts/cix/sky1.dtsi
index ea324336bf34..c526b92d62ff 100644
--- a/arch/arm64/boot/dts/cix/sky1.dtsi
+++ b/arch/arm64/boot/dts/cix/sky1.dtsi
@@ -271,6 +271,7 @@ spi0: spi@4090000 {
 				 <&scmi_clk CLK_TREE_FCH_SPI0_APB>;
 			clock-names = "ref_clk", "pclk";
 			interrupts = <GIC_SPI 294 IRQ_TYPE_LEVEL_HIGH 0>;
+			fifo-width = <32>;
 			status = "disabled";
 		};
 
@@ -281,6 +282,7 @@ spi1: spi@40a0000 {
 				 <&scmi_clk CLK_TREE_FCH_SPI1_APB>;
 			clock-names = "ref_clk", "pclk";
 			interrupts = <GIC_SPI 295 IRQ_TYPE_LEVEL_HIGH 0>;
+			fifo-width = <32>;
 			status = "disabled";
 		};
 
-- 
2.34.1