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