[PATCH v2 0/3] pwm: axi-pwmgen: add external clock

[PATCH v2 0/3] pwm: axi-pwmgen: add external clock

[David Lechner]

When we created the driver for the AXI PWMGEN IP block, we overlooked
the fact that it can optionally be configured to use an external clock
in addition to the AXI bus clock. This is easy to miss in testing
because the bus clock is always on because it is driving other
peripherals as well.

Up to now, users were specifying the external clock if there was one and
the AXI bus clock otherwise. But the proper way to do this is to would
be to always specify the bus clock and only specify the external clock
if the IP block has been configured to use it.

To fix this, we add clock-names to the devicetree bindings and change
clocks to allow 1 or 2 clocks.

---
Changes in v2:
- Consider this a fix rather than a new feature.
- Make clock-names required.
- Simplify the logic in the pwm driver to avoid needing to test if
  clock-names is present in old dtbs that used the broken binding.
- Link to v1: https://lore.kernel.org/r/20250520-pwm-axi-pwmgen-add-external-clock-v1-0-6cd63cc001c8@baylibre.com

---
David Lechner (3):
      dt-bindings: pwm: adi,axi-pwmgen: update documentation link
      dt-bindings: pwm: adi,axi-pwmgen: fix clocks
      pwm: axi-pwmgen: fix missing separate external clock

 .../devicetree/bindings/pwm/adi,axi-pwmgen.yaml    | 16 ++++++++++++---
 drivers/pwm/pwm-axi-pwmgen.c                       | 23 +++++++++++++++++++---
 2 files changed, 33 insertions(+), 6 deletions(-)
---
base-commit: 484803582c77061b470ac64a634f25f89715be3f
change-id: 20250515-pwm-axi-pwmgen-add-external-clock-0364fbdf809b

Best regards,
-- 
David Lechner <dlechner@baylibre.com>

[PATCH v2 1/3] dt-bindings: pwm: adi,axi-pwmgen: update documentation link

[David Lechner]

Change the documentation link to point to the location with the most
up-to-date information.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml b/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml
index 45e112d0efb4663bc7fbb3a25a12d66aa8b7492d..bc44381692054f647a160a6573dae4cff2ee3f31 100644
--- a/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml
+++ b/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml
@@ -14,7 +14,7 @@ description:
   The Analog Devices AXI PWM generator can generate PWM signals
   with variable pulse width and period.
 
-  https://wiki.analog.com/resources/fpga/docs/axi_pwm_gen
+  https://analogdevicesinc.github.io/hdl/library/axi_pwm_gen/index.html
 
 allOf:
   - $ref: pwm.yaml#

-- 
2.43.0

[PATCH v2 2/3] dt-bindings: pwm: adi,axi-pwmgen: fix clocks

[David Lechner]

Fix a shortcoming in the bindings that doesn't allow for a separate
external clock.

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.

This was missed in the original bindings and so users were writing dts
files where the one and only clock specified would be the external
clock, if there was one, incorrectly missing the separate AXI clock.

The correct bindings are that the AXI clock is always required and the
external clock is optional (must be given only when HDL compile option
ASYNC_CLK_EN=1).

Fixes: 1edf2c2a2841 ("dt-bindings: pwm: Add AXI PWM generator")
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml b/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml
index bc44381692054f647a160a6573dae4cff2ee3f31..887185ba89daf6eb18ffa2cba164ba546bfbcf2c 100644
--- a/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml
+++ b/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml
@@ -30,11 +30,20 @@ properties:
     const: 3
 
   clocks:
-    maxItems: 1
+    minItems: 1
+    maxItems: 2
+
+  clock-names:
+    enum:
+      - const: axi
+      - items:
+        - const: axi
+        - const: ext
 
 required:
   - reg
   - clocks
+  - clock-names
 
 unevaluatedProperties: false
 
@@ -43,6 +52,7 @@ 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>;
     };

-- 
2.43.0

[PATCH v2 3/3] pwm: axi-pwmgen: fix missing separate external clock

[David Lechner]

Add proper support for external clock to the AXI PWM generator driver.

In most cases, the HDL for this IP block is compiled with the default
ASYNC_CLK_EN=1. With this option, there is a separate external clock
that drives the PWM output separate from the peripheral clock. So the
driver should be enabling the "axi" clock to power the peripheral and
the "ext" clock to drive the PWM output.

When ASYNC_CLK_EN=0, the "axi" clock is also used to drive the PWM
output and there is no "ext" clock.

Previously, if there was a separate external clock, users had to specify
only the external clock and (incorrectly) omit the AXI clock in order
to get the correct operating frequency for the PWM output.

The devicetree bindings are updated to fix this shortcoming and this
patch changes the driver to match the new bindings. To preserve
compatibility with any existing dtbs that specify only one clock, we
don't require the clock name on the first clock.

Fixes: 41814fe5c782 ("pwm: Add driver for AXI PWM generator")
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/pwm/pwm-axi-pwmgen.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-axi-pwmgen.c b/drivers/pwm/pwm-axi-pwmgen.c
index 4337c8f5acf055fc87dc134f2a70b99b0cb5ede6..60dcd354237316bced2d951b7f0b116c8291bb0d 100644
--- a/drivers/pwm/pwm-axi-pwmgen.c
+++ b/drivers/pwm/pwm-axi-pwmgen.c
@@ -257,7 +257,7 @@ static int axi_pwmgen_probe(struct platform_device *pdev)
 	struct regmap *regmap;
 	struct pwm_chip *chip;
 	struct axi_pwmgen_ddata *ddata;
-	struct clk *clk;
+	struct clk *axi_clk, *clk;
 	void __iomem *io_base;
 	int ret;
 
@@ -280,9 +280,26 @@ static int axi_pwmgen_probe(struct platform_device *pdev)
 	ddata = pwmchip_get_drvdata(chip);
 	ddata->regmap = regmap;
 
-	clk = devm_clk_get_enabled(dev, NULL);
+	/*
+	 * Using NULL here instead of "axi" for backwards compatibility. There
+	 * are some dtbs that don't give clock-names and have the "ext" clock
+	 * as the one and only clock (due to mistake in the original bindings).
+	 */
+	axi_clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(axi_clk))
+		return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to get axi clock\n");
+
+	clk = devm_clk_get_optional_enabled(dev, "ext");
 	if (IS_ERR(clk))
-		return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n");
+		return dev_err_probe(dev, PTR_ERR(clk), "failed to get ext clock\n");
+
+	/*
+	 * If there is no "ext" clock, it means the HDL was compiled with
+	 * ASYNC_CLK_EN=0. In this case, the AXI clock is also used for the
+	 * PWM output clock.
+	 */
+	if (!clk)
+		clk = axi_clk;
 
 	ret = devm_clk_rate_exclusive_get(dev, clk);
 	if (ret)

-- 
2.43.0

Re: [PATCH v2 2/3] dt-bindings: pwm: adi,axi-pwmgen: fix clocks

[Krzysztof Kozlowski]

On 22/05/2025 16:49, David Lechner wrote:
> Fix a shortcoming in the bindings that doesn't allow for a separate
> external clock.
> 
> 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.
> 
> This was missed in the original bindings and so users were writing dts
> files where the one and only clock specified would be the external
> clock, if there was one, incorrectly missing the separate AXI clock.
> 
> The correct bindings are that the AXI clock is always required and the
> external clock is optional (must be given only when HDL compile option
> ASYNC_CLK_EN=1).
> 
> Fixes: 1edf2c2a2841 ("dt-bindings: pwm: Add AXI PWM generator")
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>  Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)

Please test your patches. This does not pass build (binding_check). :/

Best regards,
Krzysztof

Re: [PATCH v2 2/3] dt-bindings: pwm: adi,axi-pwmgen: fix clocks

[David Lechner]

On 5/22/25 10:00 AM, Krzysztof Kozlowski wrote:
> On 22/05/2025 16:49, David Lechner wrote:
>> Fix a shortcoming in the bindings that doesn't allow for a separate
>> external clock.
>>
>> 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.
>>
>> This was missed in the original bindings and so users were writing dts
>> files where the one and only clock specified would be the external
>> clock, if there was one, incorrectly missing the separate AXI clock.
>>
>> The correct bindings are that the AXI clock is always required and the
>> external clock is optional (must be given only when HDL compile option
>> ASYNC_CLK_EN=1).
>>
>> Fixes: 1edf2c2a2841 ("dt-bindings: pwm: Add AXI PWM generator")
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>> ---
>>  Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml | 14 ++++++++++++--
>>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> Please test your patches. This does not pass build (binding_check). :/
> 

Oh no, I knew I was forgetting something. :-(

Maybe time to see about building this into b4 prep --check so it won't
be so easy to forget.

The correct will be:


  clock-names:
    oneOf:
      - const: axi
      - items:
          - const: axi
          - const: ext

Re: [PATCH v2 2/3] dt-bindings: pwm: adi,axi-pwmgen: fix clocks

[Krzysztof Kozlowski]

On 22/05/2025 17:17, David Lechner wrote:
> On 5/22/25 10:00 AM, Krzysztof Kozlowski wrote:
>> On 22/05/2025 16:49, David Lechner wrote:
>>> Fix a shortcoming in the bindings that doesn't allow for a separate
>>> external clock.
>>>
>>> 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.
>>>
>>> This was missed in the original bindings and so users were writing dts
>>> files where the one and only clock specified would be the external
>>> clock, if there was one, incorrectly missing the separate AXI clock.
>>>
>>> The correct bindings are that the AXI clock is always required and the
>>> external clock is optional (must be given only when HDL compile option
>>> ASYNC_CLK_EN=1).
>>>
>>> Fixes: 1edf2c2a2841 ("dt-bindings: pwm: Add AXI PWM generator")
>>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>>> ---
>>>  Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml | 14 ++++++++++++--
>>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> Please test your patches. This does not pass build (binding_check). :/
>>
> 
> Oh no, I knew I was forgetting something. :-(
> 
> Maybe time to see about building this into b4 prep --check so it won't
> be so easy to forget.
> 
> The correct will be:
> 
> 
>   clock-names:
>     oneOf:
>       - const: axi
>       - items:
>           - const: axi
>           - const: ext


Then just
minItems: 1
items:
  - foo
  - bar

Best regards,
Krzysztof

Re: [PATCH v2 2/3] dt-bindings: pwm: adi,axi-pwmgen: fix clocks

[Rob Herring (Arm)]

On Thu, 22 May 2025 09:49:40 -0500, David Lechner wrote:
> Fix a shortcoming in the bindings that doesn't allow for a separate
> external clock.
> 
> 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.
> 
> This was missed in the original bindings and so users were writing dts
> files where the one and only clock specified would be the external
> clock, if there was one, incorrectly missing the separate AXI clock.
> 
> The correct bindings are that the AXI clock is always required and the
> external clock is optional (must be given only when HDL compile option
> ASYNC_CLK_EN=1).
> 
> Fixes: 1edf2c2a2841 ("dt-bindings: pwm: Add AXI PWM generator")
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>  Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml:40:9: [warning] wrong indentation: expected 10 but found 8 (indentation)

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml: properties:clock-names:enum: 'oneOf' conditional failed, one must be fixed:
	{'const': 'axi'} is not of type 'integer'
	{'const': 'axi'} is not of type 'string'
	{'items': [{'const': 'axi'}, {'const': 'ext'}]} is not of type 'integer'
	{'items': [{'const': 'axi'}, {'const': 'ext'}]} is not of type 'string'
	hint: "enum" must be an array of either integers or strings
	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml: properties:clock-names:enum:0: {'const': 'axi'} is not of type 'string'
	from schema $id: http://devicetree.org/meta-schemas/string-array.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.yaml: properties:clock-names:enum:1: {'items': [{'const': 'axi'}, {'const': 'ext'}]} is not of type 'string'
	from schema $id: http://devicetree.org/meta-schemas/string-array.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.example.dtb: pwm@44b00000 (adi,axi-pwmgen-2.00.a): clock-names: ['axi', 'ext'] is not one of [{'const': 'axi'}, {'items': [{'const': 'axi'}, {'const': 'ext'}]}]
	from schema $id: http://devicetree.org/schemas/pwm/adi,axi-pwmgen.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pwm/adi,axi-pwmgen.example.dtb: pwm@44b00000 (adi,axi-pwmgen-2.00.a): Unevaluated properties are not allowed ('clock-names' was unexpected)
	from schema $id: http://devicetree.org/schemas/pwm/adi,axi-pwmgen.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250522-pwm-axi-pwmgen-add-external-clock-v2-2-086ea9e6ecf0@baylibre.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v2 3/3] pwm: axi-pwmgen: fix missing separate external clock

[Nuno Sá]

On Thu, 2025-05-22 at 09:49 -0500, David Lechner wrote:
> Add proper support for external clock to the AXI PWM generator driver.
> 
> In most cases, the HDL for this IP block is compiled with the default
> ASYNC_CLK_EN=1. With this option, there is a separate external clock
> that drives the PWM output separate from the peripheral clock. So the
> driver should be enabling the "axi" clock to power the peripheral and
> the "ext" clock to drive the PWM output.
> 
> When ASYNC_CLK_EN=0, the "axi" clock is also used to drive the PWM
> output and there is no "ext" clock.
> 
> Previously, if there was a separate external clock, users had to specify
> only the external clock and (incorrectly) omit the AXI clock in order
> to get the correct operating frequency for the PWM output.
> 
> The devicetree bindings are updated to fix this shortcoming and this
> patch changes the driver to match the new bindings. To preserve
> compatibility with any existing dtbs that specify only one clock, we
> don't require the clock name on the first clock.
> 
> Fixes: 41814fe5c782 ("pwm: Add driver for AXI PWM generator")
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---

Acked-by: Nuno Sá <nuno.sa@analog.com>

>  drivers/pwm/pwm-axi-pwmgen.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-axi-pwmgen.c b/drivers/pwm/pwm-axi-pwmgen.c
> index
> 4337c8f5acf055fc87dc134f2a70b99b0cb5ede6..60dcd354237316bced2d951b7f0b116c8291
> bb0d 100644
> --- a/drivers/pwm/pwm-axi-pwmgen.c
> +++ b/drivers/pwm/pwm-axi-pwmgen.c
> @@ -257,7 +257,7 @@ static int axi_pwmgen_probe(struct platform_device *pdev)
>  	struct regmap *regmap;
>  	struct pwm_chip *chip;
>  	struct axi_pwmgen_ddata *ddata;
> -	struct clk *clk;
> +	struct clk *axi_clk, *clk;
>  	void __iomem *io_base;
>  	int ret;
>  
> @@ -280,9 +280,26 @@ static int axi_pwmgen_probe(struct platform_device *pdev)
>  	ddata = pwmchip_get_drvdata(chip);
>  	ddata->regmap = regmap;
>  
> -	clk = devm_clk_get_enabled(dev, NULL);
> +	/*
> +	 * Using NULL here instead of "axi" for backwards compatibility.
> There
> +	 * are some dtbs that don't give clock-names and have the "ext" clock
> +	 * as the one and only clock (due to mistake in the original
> bindings).
> +	 */
> +	axi_clk = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(axi_clk))
> +		return dev_err_probe(dev, PTR_ERR(axi_clk), "failed to get
> axi clock\n");
> +
> +	clk = devm_clk_get_optional_enabled(dev, "ext");
>  	if (IS_ERR(clk))
> -		return dev_err_probe(dev, PTR_ERR(clk), "failed to get
> clock\n");
> +		return dev_err_probe(dev, PTR_ERR(clk), "failed to get ext
> clock\n");
> +
> +	/*
> +	 * If there is no "ext" clock, it means the HDL was compiled with
> +	 * ASYNC_CLK_EN=0. In this case, the AXI clock is also used for the
> +	 * PWM output clock.
> +	 */
> +	if (!clk)
> +		clk = axi_clk;
>  
>  	ret = devm_clk_rate_exclusive_get(dev, clk);
>  	if (ret)

Re: [PATCH v2 3/3] pwm: axi-pwmgen: fix missing separate external clock

[Trevor Gamblin]

On 2025-05-22 10:49, David Lechner wrote:
> Add proper support for external clock to the AXI PWM generator driver.
>
> In most cases, the HDL for this IP block is compiled with the default
> ASYNC_CLK_EN=1. With this option, there is a separate external clock
> that drives the PWM output separate from the peripheral clock. So the
> driver should be enabling the "axi" clock to power the peripheral and
> the "ext" clock to drive the PWM output.
>
> When ASYNC_CLK_EN=0, the "axi" clock is also used to drive the PWM
> output and there is no "ext" clock.
>
> Previously, if there was a separate external clock, users had to specify
> only the external clock and (incorrectly) omit the AXI clock in order
> to get the correct operating frequency for the PWM output.
>
> The devicetree bindings are updated to fix this shortcoming and this
> patch changes the driver to match the new bindings. To preserve
> compatibility with any existing dtbs that specify only one clock, we
> don't require the clock name on the first clock.
>
> Fixes: 41814fe5c782 ("pwm: Add driver for AXI PWM generator")
> Signed-off-by: David Lechner <dlechner@baylibre.com>

Thanks for fixing this!

Reviewed-by: Trevor Gamblin <tgamblin@baylibre.com>