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
