Message-ID:
 <SI6PR06MB710489F8394133B846744D4BFFE4A@SI6PR06MB7104.apcprd06.prod.outlook.com>
Date: Fri, 3 Oct 2025 14:58:45 +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 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@...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.

> 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@...nel.org>
发送时间: 2025年10月2日 2:04
收件人: Jun Guo
抄送: 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
主题: Re: 回复: [PATCH 1/3] dt-bindings: spi: spi-cadence: document optional fifo-width DT property

EXTERNAL EMAIL

Powered by blists - more mailing lists