Message-ID:
 <SI6PR06MB7104AE0345763471E67CD3C0FFE6A@SI6PR06MB7104.apcprd06.prod.outlook.com>
Date: Wed, 1 Oct 2025 14:36:44 +0000
From: Jun Guo <Jun.Guo@...tech.com>
To: Conor Dooley <conor@...nel.org>
CC: Peter Chen <peter.chen@...tech.com>, Fugang Duan
	<fugang.duan@...tech.com>, "robh@...nel.org" <robh@...nel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "conor+dt@...nel.org"
	<conor+dt@...nel.org>, "broonie@...nel.org" <broonie@...nel.org>,
	"linux-spi@...r.kernel.org" <linux-spi@...r.kernel.org>,
	"michal.simek@....com" <michal.simek@....com>, cix-kernel-upstream
	<cix-kernel-upstream@...tech.com>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject:
 回复: [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@...tech.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@...tech.com>
发送时间: 2025年10月1日 22:24
收件人: Conor Dooley
抄送: Peter Chen; Fugang Duan; robh@...nel.org; krzk+dt@...nel.org; conor+dt@...nel.org; broonie@...nel.org; linux-spi@...r.kernel.org; michal.simek@....com; cix-kernel-upstream; linux-arm-kernel@...ts.infradead.org; devicetree@...r.kernel.org; linux-kernel@...r.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@...tech.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@...nel.org>
发送时间: 2025年10月1日 2:51
收件人: Jun Guo <jun.guo@...tech.com>
抄送: Peter Chen <peter.chen@...tech.com>; Fugang Duan <fugang.duan@...tech.com>; robh@...nel.org <robh@...nel.org>; krzk+dt@...nel.org <krzk+dt@...nel.org>; conor+dt@...nel.org <conor+dt@...nel.org>; broonie@...nel.org <broonie@...nel.org>; linux-spi@...r.kernel.org <linux-spi@...r.kernel.org>; michal.simek@....com <michal.simek@....com>; cix-kernel-upstream <cix-kernel-upstream@...tech.com>; linux-arm-kernel@...ts.infradead.org <linux-arm-kernel@...ts.infradead.org>; devicetree@...r.kernel.org <devicetree@...r.kernel.org>; linux-kernel@...r.kernel.org <linux-kernel@...r.kernel.org>
主题: Re: [PATCH 1/3] dt-bindings: spi: spi-cadence: document optional fifo-width DT property

EXTERNAL EMAIL

Powered by blists - more mailing lists