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

[Mahapatra, Amit Kumar]

Hello Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
> Sent: Thursday, October 3, 2024 12:31 PM
> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@xxxxxxx>; Conor Dooley
> <conor@xxxxxxxxxx>
> Cc: broonie@xxxxxxxxxx; robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx;
> conor+dt@xxxxxxxxxx; Simek, Michal <michal.simek@xxxxxxx>; linux-
> spi@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; git (AMD-Xilinx)
> <git@xxxxxxx>; amitrkcian2002@xxxxxxxxx
> Subject: Re: [PATCH] dt-bindings: spi: xilinx: Add clocks & clock-names properties
> 
> On 03/10/2024 08:23, Mahapatra, Amit Kumar wrote:
> > Hello Conor,
> >
> >> -----Original Message-----
> >> From: Conor Dooley <conor@xxxxxxxxxx>
> >> Sent: Monday, September 30, 2024 10:10 PM
> >> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@xxxxxxx>
> >> Cc: Krzysztof Kozlowski <krzk@xxxxxxxxxx>; broonie@xxxxxxxxxx;
> >> robh@xxxxxxxxxx;
> >> krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx; Simek, Michal
> >> <michal.simek@xxxxxxx>; linux-spi@xxxxxxxxxxxxxxx;
> >> devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> >> linux-kernel@xxxxxxxxxxxxxxx; git (AMD-Xilinx) <git@xxxxxxx>;
> >> amitrkcian2002@xxxxxxxxx
> >> Subject: Re: [PATCH] dt-bindings: spi: xilinx: Add clocks &
> >> clock-names properties
> >>
> >> On Mon, Sep 30, 2024 at 03:44:47PM +0000, Mahapatra, Amit Kumar wrote:
> >>> 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'.
> >>>>
> >>>>>>>>>> +      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_qu
> >>>>>> ad_s
> >>>>>> pi/v3_2/pg153-axi-quad-spi.pdf
> >>>>>
> >>>>> So if hardware engineers call them "pink_pony_clk_aclk_really_clk"
> >>>>> we should follow...
> >>>>>
> >>>>>  - bus or axi
> >>>>>  - ext_spi or spi
> >>>>>
> >>>>> You have descriptions of each item to reference real signals.
> >>>>> Conor's comment is valid - do no make it verilog file.
> >>>>>
> >>>>>>
> >>>>>>>
> >>>>>>>>>> +
> >>>>>>>>>> +    else:
> >>>>>>>>>> +      properties:
> >>>>>>>>>> +        clock-names:
> >>>>>>>>>> +          items:
> >>>>>>>>>> +            - const: s_axi4_aclk
> >>>>>>>>>> +            - const: ext_spi_clk
> >>>>>
> >>>>> Nah, these are the same.
> >>>>
> >>>> They may be different, depending on whether or not the driver has
> >>>> to handle "axi4- lite" versus "axi" differently. That said, I find
> >>>> the commit message kinda odd in that it states that axi4-lite goes
> >>>> with the s_axi_aclk
> >> clock and axi goes with s_axi4_aclk.
> >>>
> >>> Apologies for the typo. When the AXI4 interface is enabled, it uses
> >>> s_axi4_aclk, and when the AXI4-Lite interface is enabled, it uses s_axi_aclk.
> >>>
> >>> In my next series I will update my commit message & change the
> >>> clock-names 's_axi4_aclk', 's_axi_aclk' & 'ext_spi_clk' to 'axi4',
> >>> 'axi' & 'ref' respectively
> >>
> >> There's no driver here, so it is hard to know (why isn't there?) -
> >> are you using the axi
> >
> > We are working on the driver. Once it is ready we will send it to upstream.
> 
> Why would you send separate binding from driver? That's only making everything
> more difficult...

Alright, I will send the next version along with the driver changes.

> 
> >
> >> v axi4 to do some sort of differentiation in the driver?
> > In the driver we don't do any different operations based on the clocks
> > , we simply enable the available clocks in the driver.
> 
> So it is the same clock?

These are two different clocks and depending on the mode—Legacy or 
Enhanced—either clock will be enabled, but the purpose of both the 
clocks are the same. We can have the name 'axi' for both the 
clocks (axi & axi4). Please let me know if you are fine with this.

Regards,
Amit
> 
> Best regards,
> Krzysztof