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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251001-simple-conclude-d9cd153cee3c@spud>
Date: Wed, 1 Oct 2025 19:04:17 +0100
From: Conor Dooley <conor@...nel.org>
To: Jun Guo <Jun.Guo@...tech.com>
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: Re: 回复: [PATCH 1/3] dt-bindings: spi: spi-cadence: document optional fifo-width DT property

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.

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


Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)