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

[Conor Dooley <conor@kernel.org>]

[-- 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 --]