| Message ID | 20250520-pwm-axi-pwmgen-add-external-clock-v1-2-6cd63cc001c8@baylibre.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pwm: axi-pwmgen: add external clock | expand |
On Tue, May 20, 2025 at 04:00:45PM GMT, David Lechner wrote: > Add external clock to the schema. > > The AXI PWMGEN IP block has a compile option ASYNC_CLK_EN that allows > the use of an external clock for the PWM output separate from the AXI > clock that runs the peripheral. > > In these cases, we should specify both clocks in the device tree. The > intention here is that if you specify both clocks, then you include the > clock-names property and if you don't have an external clock, then you > omit the clock-names property. > > There can't be more than one allOf: in the top level of the schema, so > it is stolen from $ref since it isn't needed there and used for the > more typical case of the if statement (even though technically it isn't > needed there either at this time). > > Signed-off-by: David Lechner <dlechner@baylibre.com> > --- > .../devicetree/bindings/pwm/adi,axi-pwmgen.yaml | 26 ++++++++++++++++++---- > 1 file changed, 22 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml b/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml > index bc44381692054f647a160a6573dae4cff2ee3f31..90f702a5cd80bd7d62e2436b2eed44314ab4fd53 100644 > --- a/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml > +++ b/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml > @@ -16,8 +16,7 @@ description: > > https://analogdevicesinc.github.io/hdl/library/axi_pwm_gen/index.html > > -allOf: > - - $ref: pwm.yaml# > +$ref: pwm.yaml# > > properties: > compatible: > @@ -30,7 +29,13 @@ properties: > const: 3 > > clocks: > - maxItems: 1 > + minItems: 1 > + maxItems: 2 > + > + clock-names: > + items: > + - const: axi > + - const: ext > > required: > - reg > @@ -38,11 +43,24 @@ required: > > unevaluatedProperties: false > > +allOf: > + - if: > + required: [clock-names] No, don't do that. If you want clock-names, just add them for both cases. Otherwise, just describe items in clocks and no need for clock-names. > + then: > + properties: > + clocks: > + minItems: 2 > + else: > + properties: > + clocks: > + maxItems: 1 > + > examples: > - | > pwm@44b00000 { > compatible = "adi,axi-pwmgen-2.00.a"; > reg = <0x44b00000 0x1000>; > - clocks = <&spi_clk>; > + clocks = <&fpga_clk>, <&spi_clk>; What was the clock[0] before? Axi, right, so SPI_CLK. Now FPGA is the AXI_CLK? This feels like clock order reversed. Best regards, Krzysztof
On 5/21/25 5:09 AM, Krzysztof Kozlowski wrote: > On Tue, May 20, 2025 at 04:00:45PM GMT, David Lechner wrote: >> Add external clock to the schema. >> >> The AXI PWMGEN IP block has a compile option ASYNC_CLK_EN that allows >> the use of an external clock for the PWM output separate from the AXI >> clock that runs the peripheral. >> >> In these cases, we should specify both clocks in the device tree. The >> intention here is that if you specify both clocks, then you include the >> clock-names property and if you don't have an external clock, then you >> omit the clock-names property. >> >> There can't be more than one allOf: in the top level of the schema, so >> it is stolen from $ref since it isn't needed there and used for the >> more typical case of the if statement (even though technically it isn't >> needed there either at this time). >> >> Signed-off-by: David Lechner <dlechner@baylibre.com> >> --- >> .../devicetree/bindings/pwm/adi,axi-pwmgen.yaml | 26 ++++++++++++++++++---- >> 1 file changed, 22 insertions(+), 4 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml b/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml >> index bc44381692054f647a160a6573dae4cff2ee3f31..90f702a5cd80bd7d62e2436b2eed44314ab4fd53 100644 >> --- a/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml >> +++ b/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml >> @@ -16,8 +16,7 @@ description: >> >> https://analogdevicesinc.github.io/hdl/library/axi_pwm_gen/index.html >> >> -allOf: >> - - $ref: pwm.yaml# >> +$ref: pwm.yaml# >> >> properties: >> compatible: >> @@ -30,7 +29,13 @@ properties: >> const: 3 >> >> clocks: >> - maxItems: 1 >> + minItems: 1 >> + maxItems: 2 >> + >> + clock-names: >> + items: >> + - const: axi >> + - const: ext >> >> required: >> - reg >> @@ -38,11 +43,24 @@ required: >> >> unevaluatedProperties: false >> >> +allOf: >> + - if: >> + required: [clock-names] > > > No, don't do that. If you want clock-names, just add them for both > cases. Otherwise, just describe items in clocks and no need for > clock-names. Would it be OK then to make clock-names required and just let the driver still handle one clocks, no clock-names for backwards compatibility? > > > >> + then: >> + properties: >> + clocks: >> + minItems: 2 >> + else: >> + properties: >> + clocks: >> + maxItems: 1 >> + >> examples: >> - | >> pwm@44b00000 { >> compatible = "adi,axi-pwmgen-2.00.a"; >> reg = <0x44b00000 0x1000>; >> - clocks = <&spi_clk>; >> + clocks = <&fpga_clk>, <&spi_clk>; > > What was the clock[0] before? Axi, right, so SPI_CLK. Now FPGA is the > AXI_CLK? This feels like clock order reversed. The problem being fixed here is that since there was only one clock in the binding, existing .dts files have either have the spi_clock or the FPGA/AXI clock. So the one clock could be either and there are existing .dtbs out in the world with both cases. But we could consider reversing this so that if someone uses the new bindings with an old kernel, then it would still work. > > Best regards, > Krzysztof >
On 21/05/2025 15:14, David Lechner wrote: > On 5/21/25 5:09 AM, Krzysztof Kozlowski wrote: >> On Tue, May 20, 2025 at 04:00:45PM GMT, David Lechner wrote: >>> Add external clock to the schema. >>> >>> The AXI PWMGEN IP block has a compile option ASYNC_CLK_EN that allows >>> the use of an external clock for the PWM output separate from the AXI >>> clock that runs the peripheral. >>> >>> In these cases, we should specify both clocks in the device tree. The >>> intention here is that if you specify both clocks, then you include the >>> clock-names property and if you don't have an external clock, then you >>> omit the clock-names property. >>> >>> There can't be more than one allOf: in the top level of the schema, so >>> it is stolen from $ref since it isn't needed there and used for the >>> more typical case of the if statement (even though technically it isn't >>> needed there either at this time). >>> >>> Signed-off-by: David Lechner <dlechner@baylibre.com> >>> --- >>> .../devicetree/bindings/pwm/adi,axi-pwmgen.yaml | 26 ++++++++++++++++++---- >>> 1 file changed, 22 insertions(+), 4 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml b/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml >>> index bc44381692054f647a160a6573dae4cff2ee3f31..90f702a5cd80bd7d62e2436b2eed44314ab4fd53 100644 >>> --- a/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml >>> +++ b/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml >>> @@ -16,8 +16,7 @@ description: >>> >>> https://analogdevicesinc.github.io/hdl/library/axi_pwm_gen/index.html >>> >>> -allOf: >>> - - $ref: pwm.yaml# >>> +$ref: pwm.yaml# >>> >>> properties: >>> compatible: >>> @@ -30,7 +29,13 @@ properties: >>> const: 3 >>> >>> clocks: >>> - maxItems: 1 >>> + minItems: 1 >>> + maxItems: 2 >>> + >>> + clock-names: >>> + items: >>> + - const: axi >>> + - const: ext >>> >>> required: >>> - reg >>> @@ -38,11 +43,24 @@ required: >>> >>> unevaluatedProperties: false >>> >>> +allOf: >>> + - if: >>> + required: [clock-names] >> >> >> No, don't do that. If you want clock-names, just add them for both >> cases. Otherwise, just describe items in clocks and no need for >> clock-names. > > Would it be OK then to make clock-names required and just let the > driver still handle one clocks, no clock-names for backwards compatibility? So just don't make it required. > >> >> >> >>> + then: >>> + properties: >>> + clocks: >>> + minItems: 2 >>> + else: >>> + properties: >>> + clocks: >>> + maxItems: 1 >>> + >>> examples: >>> - | >>> pwm@44b00000 { >>> compatible = "adi,axi-pwmgen-2.00.a"; >>> reg = <0x44b00000 0x1000>; >>> - clocks = <&spi_clk>; >>> + clocks = <&fpga_clk>, <&spi_clk>; >> >> What was the clock[0] before? Axi, right, so SPI_CLK. Now FPGA is the >> AXI_CLK? This feels like clock order reversed. > > The problem being fixed here is that since there was only one clock in > the binding, existing .dts files have either have the spi_clock or > the FPGA/AXI clock. So the one clock could be either and there are > existing .dtbs out in the world with both cases. No problem like that was explained in commit msg. Nevertheless driver assumed the first clock is the SPI, didn't it? So that's your ABI, even if binding was not conclusive here. > > But we could consider reversing this so that if someone uses the new > bindings with an old kernel, then it would still work. You cannot use new bindings with old kernel. How would that work? Put YAML file there? Nothing would change. Binding is supposed to be complete for exactly this reason. You cannot change it afterwards without breaking users. Best regards, Krzysztof
On 5/21/25 8:28 AM, Krzysztof Kozlowski wrote: > On 21/05/2025 15:14, David Lechner wrote: >> On 5/21/25 5:09 AM, Krzysztof Kozlowski wrote: >>> On Tue, May 20, 2025 at 04:00:45PM GMT, David Lechner wrote: >>>> Add external clock to the schema. >>>> >>>> The AXI PWMGEN IP block has a compile option ASYNC_CLK_EN that allows >>>> the use of an external clock for the PWM output separate from the AXI >>>> clock that runs the peripheral. >>>> >>>> In these cases, we should specify both clocks in the device tree. The >>>> intention here is that if you specify both clocks, then you include the >>>> clock-names property and if you don't have an external clock, then you >>>> omit the clock-names property. >>>> >>>> There can't be more than one allOf: in the top level of the schema, so >>>> it is stolen from $ref since it isn't needed there and used for the >>>> more typical case of the if statement (even though technically it isn't >>>> needed there either at this time). >>>> >>>> Signed-off-by: David Lechner <dlechner@baylibre.com> >>>> --- >>>> .../devicetree/bindings/pwm/adi,axi-pwmgen.yaml | 26 ++++++++++++++++++---- >>>> 1 file changed, 22 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml b/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml >>>> index bc44381692054f647a160a6573dae4cff2ee3f31..90f702a5cd80bd7d62e2436b2eed44314ab4fd53 100644 >>>> --- a/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml >>>> +++ b/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml >>>> @@ -16,8 +16,7 @@ description: >>>> >>>> https://analogdevicesinc.github.io/hdl/library/axi_pwm_gen/index.html >>>> >>>> -allOf: >>>> - - $ref: pwm.yaml# >>>> +$ref: pwm.yaml# >>>> >>>> properties: >>>> compatible: >>>> @@ -30,7 +29,13 @@ properties: >>>> const: 3 >>>> >>>> clocks: >>>> - maxItems: 1 >>>> + minItems: 1 >>>> + maxItems: 2 >>>> + >>>> + clock-names: >>>> + items: >>>> + - const: axi >>>> + - const: ext >>>> >>>> required: >>>> - reg >>>> @@ -38,11 +43,24 @@ required: >>>> >>>> unevaluatedProperties: false >>>> >>>> +allOf: >>>> + - if: >>>> + required: [clock-names] >>> >>> >>> No, don't do that. If you want clock-names, just add them for both >>> cases. Otherwise, just describe items in clocks and no need for >>> clock-names. >> >> Would it be OK then to make clock-names required and just let the >> driver still handle one clocks, no clock-names for backwards compatibility? > > So just don't make it required. > >> >>> >>> >>> >>>> + then: >>>> + properties: >>>> + clocks: >>>> + minItems: 2 >>>> + else: >>>> + properties: >>>> + clocks: >>>> + maxItems: 1 >>>> + >>>> examples: >>>> - | >>>> pwm@44b00000 { >>>> compatible = "adi,axi-pwmgen-2.00.a"; >>>> reg = <0x44b00000 0x1000>; >>>> - clocks = <&spi_clk>; >>>> + clocks = <&fpga_clk>, <&spi_clk>; >>> >>> What was the clock[0] before? Axi, right, so SPI_CLK. Now FPGA is the >>> AXI_CLK? This feels like clock order reversed. >> >> The problem being fixed here is that since there was only one clock in >> the binding, existing .dts files have either have the spi_clock or >> the FPGA/AXI clock. So the one clock could be either and there are >> existing .dtbs out in the world with both cases. > > No problem like that was explained in commit msg. Nevertheless driver You are right. I explained it in the cover letter, but failed to repeat that in this commit message. > assumed the first clock is the SPI, didn't it? So that's your ABI, even > if binding was not conclusive here. Not quite. The driver (before this series) assumes that the clock is the SPI clock ("ext") if the HDL was compiled with ASYNC_CLK_EN=1 or the AXI clock if the HDL was compiled with ASYNC_CLK_EN=0 (in this case, there is no "ext"/SPI clock). >> >> But we could consider reversing this so that if someone uses the new >> bindings with an old kernel, then it would still work. > > You cannot use new bindings with old kernel. How would that work? Put > YAML file there? Nothing would change. > > Binding is supposed to be complete for exactly this reason. You cannot > change it afterwards without breaking users. > > Best regards, > Krzysztof
diff --git a/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml b/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml index bc44381692054f647a160a6573dae4cff2ee3f31..90f702a5cd80bd7d62e2436b2eed44314ab4fd53 100644 --- a/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml +++ b/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml @@ -16,8 +16,7 @@ description: https://analogdevicesinc.github.io/hdl/library/axi_pwm_gen/index.html -allOf: - - $ref: pwm.yaml# +$ref: pwm.yaml# properties: compatible: @@ -30,7 +29,13 @@ properties: const: 3 clocks: - maxItems: 1 + minItems: 1 + maxItems: 2 + + clock-names: + items: + - const: axi + - const: ext required: - reg @@ -38,11 +43,24 @@ required: unevaluatedProperties: false +allOf: + - if: + required: [clock-names] + then: + properties: + clocks: + minItems: 2 + else: + properties: + clocks: + maxItems: 1 + examples: - | pwm@44b00000 { compatible = "adi,axi-pwmgen-2.00.a"; reg = <0x44b00000 0x1000>; - clocks = <&spi_clk>; + clocks = <&fpga_clk>, <&spi_clk>; + clock-names = "axi", "ext"; #pwm-cells = <3>; };
Add external clock to the schema. The AXI PWMGEN IP block has a compile option ASYNC_CLK_EN that allows the use of an external clock for the PWM output separate from the AXI clock that runs the peripheral. In these cases, we should specify both clocks in the device tree. The intention here is that if you specify both clocks, then you include the clock-names property and if you don't have an external clock, then you omit the clock-names property. There can't be more than one allOf: in the top level of the schema, so it is stolen from $ref since it isn't needed there and used for the more typical case of the if statement (even though technically it isn't needed there either at this time). Signed-off-by: David Lechner <dlechner@baylibre.com> --- .../devicetree/bindings/pwm/adi,axi-pwmgen.yaml | 26 ++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)