| Message ID | 20240923123242.2101562-1-amit.kumar-mahapatra@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | dt-bindings: spi: xilinx: Add clocks & clock-names properties | expand |
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 >
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).
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
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
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..
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..
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?
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
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
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
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
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>; };
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(+)