RE: [PATCH] dt-bindings: spi: xilinx: Add clocks & clock-names properties

[Mahapatra, Amit Kumar]

Hello Conor,


> > > Subject: Re: [PATCH] dt-bindings: spi: xilinx: Add clocks &
> > > clock-names properties
> > >
> > > On Mon, Sep 23, 2024 at 06:02:42PM +0530, Amit Kumar Mahapatra wrote:
> > > > Include the 'clocks' and 'clock-names' properties in the AXI
> > > > Quad-SPI bindings. When the AXI4-Lite interface is enabled, the
> > > > core operates in legacy mode, maintaining backward compatibility
> > > > with version 1.00, and uses 's_axi_aclk' and 'ext_spi_clk'. For
> > > > the AXI interface, it uses 's_axi4_aclk' and 'ext_spi_clk'.
> > > >
> > > > Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@xxxxxxx>
> > > > ---
> > > > BRANCH: for-next
> > > > ---
> > > >  .../devicetree/bindings/spi/spi-xilinx.yaml   | 29 +++++++++++++++++++
> > > >  1 file changed, 29 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/spi/spi-xilinx.yaml
> > > > b/Documentation/devicetree/bindings/spi/spi-xilinx.yaml
> > > > index 4beb3af0416d..9dfec195ecd4 100644
> > > > --- a/Documentation/devicetree/bindings/spi/spi-xilinx.yaml
> > > > +++ b/Documentation/devicetree/bindings/spi/spi-xilinx.yaml
> > > > @@ -12,6 +12,25 @@ maintainers:
> > > >  allOf:
> > > >    - $ref: spi-controller.yaml#
> > >
> > > Please move the allOf block down to the end of the binding, after
> > > the property definitions.
> >
> > Sure, I'll take care of it in the next series
> > >
> > > > +  - if:
> > > > +      properties:
> > > > +        compatible:
> > > > +          contains:
> > > > +            const: xlnx,axi-quad-spi-1.00.a
> > > > +    then:
> > > > +      properties:
> > > > +        clock-names:
> > > > +          items:
> > > > +            - const: s_axi_aclk
> > > > +            - const: ext_spi_clk
> > >
> > > These are all clocks, there should be no need to have "clk" in the names.
> >
> > These are the names exported by the IP and used by the DTG.
> 
> So? This is a binding, not a verilog file.

Axi Quad SPI is an FPGA-based IP, and the clock names are derived from the 
IP signal names as specified in the IP documentation [1]. 
We chose these names to ensure alignment with the I/O signal names listed 
in Table 2-2 on page 19 of [1].

[1] chrome-extension://efaidnbmnnnibpcajpcglclefindmkaj/https://www.amd.com/content/dam/xilinx/support/documents/ip_documentation/axi_quad_spi/v3_2/pg153-axi-quad-spi.pdf

> 
> > > > +
> > > > +    else:
> > > > +      properties:
> > > > +        clock-names:
> > > > +          items:
> > > > +            - const: s_axi4_aclk
> > > > +            - const: ext_spi_clk
> > > > +
> > > >  properties:
> > > >    compatible:
> > > >      enum:
> > > > @@ -25,6 +44,12 @@ properties:
> > > >    interrupts:
> > > >      maxItems: 1
> > > >
> > > > +  clocks:
> > > > +    maxItems: 2
> > > > +
> > > > +  clock-names:
> > > > +    maxItems: 2
> > > > +
> > > >    xlnx,num-ss-bits:
> > > >      description: Number of chip selects used.
> > > >      minimum: 1
> > > > @@ -39,6 +64,8 @@ required:
> > > >    - compatible
> > > >    - reg
> > > >    - interrupts
> > > > +  - clocks
> > > > +  - clock-names
> > >
> > > New required properties are an ABI break, where is the driver patch
> > > that makes use of these clocks?
> >
> > Alright, I will remove these from the required properties to avoid
> > breaking the ABI. We're working on the driver patch and will send it
> > once it's ready.
> 
> What changed to make the clocks needed now? It's possible that making them
> required is the correct thing to do, so breaking the ABI would be justified (provided
> the driver can still handle there being no clocks).

Axi Quad SPI is an FPGA-based IP that was previously tested on MicroBlaze 
soft-core systems, where the driver didn't need to enable the clock, as it 
would already be enabled before the PL is loaded. However, when used 
with ARM hard-core SoCs, the driver must explicitly enable the clocks, 
making it necessary to provide the clock information.

Regards,
Amit