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

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

[Amit Kumar Mahapatra]

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@amd.com>
---
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#
 
+  - 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
+
+    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
 
 unevaluatedProperties: false
 
@@ -49,6 +76,8 @@ examples:
       interrupt-parent = <&intc>;
       interrupts = <0 31 1>;
       reg = <0x41e00000 0x10000>;
+      clocks = <&clkc 72>, <&clkc 73>;
+      clock-names = "s_axi4_aclk", "ext_spi_clk";
       xlnx,num-ss-bits = <0x1>;
       xlnx,num-transfer-bits = <32>;
     };
-- 
2.34.1

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2560 bytes --]

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@amd.com>
> ---
> 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.

> +  - 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.

> +
> +    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?

Cheers,
Conor.

>  
>  unevaluatedProperties: false
>  
> @@ -49,6 +76,8 @@ examples:
>        interrupt-parent = <&intc>;
>        interrupts = <0 31 1>;
>        reg = <0x41e00000 0x10000>;
> +      clocks = <&clkc 72>, <&clkc 73>;
> +      clock-names = "s_axi4_aclk", "ext_spi_clk";
>        xlnx,num-ss-bits = <0x1>;
>        xlnx,num-transfer-bits = <32>;
>      };
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Mahapatra, Amit Kumar]

Hello Conor,


> -----Original Message-----
> From: Conor Dooley <conor@kernel.org>
> Sent: Tuesday, September 24, 2024 10:07 PM
> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>
> Cc: broonie@kernel.org; robh@kernel.org; krzk+dt@kernel.org;
> conor+dt@kernel.org; Simek, Michal <michal.simek@amd.com>; linux-
> spi@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git (AMD-Xilinx)
> <git@amd.com>; amitrkcian2002@gmail.com
> 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@amd.com>
> > ---
> > 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.

> 
> > +
> > +    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.

Regards,
Amit
> 
> Cheers,
> Conor.
> 
> >
> >  unevaluatedProperties: false
> >
> > @@ -49,6 +76,8 @@ examples:
> >        interrupt-parent = <&intc>;
> >        interrupts = <0 31 1>;
> >        reg = <0x41e00000 0x10000>;
> > +      clocks = <&clkc 72>, <&clkc 73>;
> > +      clock-names = "s_axi4_aclk", "ext_spi_clk";
> >        xlnx,num-ss-bits = <0x1>;
> >        xlnx,num-transfer-bits = <32>;
> >      };
> > --
> > 2.34.1
> >

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3691 bytes --]

On Wed, Sep 25, 2024 at 11:35:56AM +0000, Mahapatra, Amit Kumar wrote:
> Hello Conor,
> 
> 
> > -----Original Message-----
> > From: Conor Dooley <conor@kernel.org>
> > Sent: Tuesday, September 24, 2024 10:07 PM
> > To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>
> > Cc: broonie@kernel.org; robh@kernel.org; krzk+dt@kernel.org;
> > conor+dt@kernel.org; Simek, Michal <michal.simek@amd.com>; linux-
> > spi@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git (AMD-Xilinx)
> > <git@amd.com>; amitrkcian2002@gmail.com
> > 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@amd.com>
> > > ---
> > > 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.

> > > +
> > > +    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).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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@amd.com>
> > > > ---
> > > > 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

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

[Krzysztof Kozlowski]

On 27/09/2024 11:30, 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'.
>>>>>
>>>>> Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com>
>>>>> ---
>>>>> 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

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.

>>>>> +
>>>>>  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, 


Commit msg should explain this. Including ABI break impact.

Best regards,
Krzysztof

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2336 bytes --]

On Sat, Sep 28, 2024 at 10:19:01AM +0200, Krzysztof Kozlowski wrote:
> On 27/09/2024 11:30, 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_quad_spi/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. Seems backwards..

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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'.
> 
> > >>>>> +      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_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

Regards,
Amit

> Seems backwards..

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2962 bytes --]

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_quad_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 v axi4 to do some sort of differentiation in the driver?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Mahapatra, Amit Kumar]

Hello Conor,

> -----Original Message-----
> From: Conor Dooley <conor@kernel.org>
> Sent: Monday, September 30, 2024 10:10 PM
> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>
> Cc: Krzysztof Kozlowski <krzk@kernel.org>; broonie@kernel.org; robh@kernel.org;
> krzk+dt@kernel.org; conor+dt@kernel.org; Simek, Michal
> <michal.simek@amd.com>; linux-spi@vger.kernel.org; devicetree@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git (AMD-Xilinx)
> <git@amd.com>; amitrkcian2002@gmail.com
> 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.

> 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.

Regards,
Amit

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

[Krzysztof Kozlowski]

On 03/10/2024 08:23, Mahapatra, Amit Kumar wrote:
> Hello Conor,
> 
>> -----Original Message-----
>> From: Conor Dooley <conor@kernel.org>
>> Sent: Monday, September 30, 2024 10:10 PM
>> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>
>> Cc: Krzysztof Kozlowski <krzk@kernel.org>; broonie@kernel.org; robh@kernel.org;
>> krzk+dt@kernel.org; conor+dt@kernel.org; Simek, Michal
>> <michal.simek@amd.com>; linux-spi@vger.kernel.org; devicetree@vger.kernel.org;
>> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git (AMD-Xilinx)
>> <git@amd.com>; amitrkcian2002@gmail.com
>> 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...

> 
>> 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?

Best regards,
Krzysztof

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

[Mahapatra, Amit Kumar]

Hello Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Thursday, October 3, 2024 12:31 PM
> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; Conor Dooley
> <conor@kernel.org>
> Cc: broonie@kernel.org; robh@kernel.org; krzk+dt@kernel.org;
> conor+dt@kernel.org; Simek, Michal <michal.simek@amd.com>; linux-
> spi@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git (AMD-Xilinx)
> <git@amd.com>; amitrkcian2002@gmail.com
> 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@kernel.org>
> >> Sent: Monday, September 30, 2024 10:10 PM
> >> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>
> >> Cc: Krzysztof Kozlowski <krzk@kernel.org>; broonie@kernel.org;
> >> robh@kernel.org;
> >> krzk+dt@kernel.org; conor+dt@kernel.org; Simek, Michal
> >> <michal.simek@amd.com>; linux-spi@vger.kernel.org;
> >> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> >> linux-kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>;
> >> amitrkcian2002@gmail.com
> >> 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

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

[Krzysztof Kozlowski]

On 03/10/2024 09:42, Mahapatra, Amit Kumar wrote:
> Hello Krzysztof,
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzk@kernel.org>
>> Sent: Thursday, October 3, 2024 12:31 PM
>> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>; Conor Dooley
>> <conor@kernel.org>
>> Cc: broonie@kernel.org; robh@kernel.org; krzk+dt@kernel.org;
>> conor+dt@kernel.org; Simek, Michal <michal.simek@amd.com>; linux-
>> spi@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
>> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; git (AMD-Xilinx)
>> <git@amd.com>; amitrkcian2002@gmail.com
>> 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@kernel.org>
>>>> Sent: Monday, September 30, 2024 10:10 PM
>>>> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>
>>>> Cc: Krzysztof Kozlowski <krzk@kernel.org>; broonie@kernel.org;
>>>> robh@kernel.org;
>>>> krzk+dt@kernel.org; conor+dt@kernel.org; Simek, Michal
>>>> <michal.simek@amd.com>; linux-spi@vger.kernel.org;
>>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>>>> linux-kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>;
>>>> amitrkcian2002@gmail.com
>>>> 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.

Please read my first comment in this thread. We are dragging this way
too much...

Best regards,
Krzysztof