[PATCH 0/2] clk: clk-axi-clkgen: make sure to enable the AXI bus clock

[PATCH 0/2] clk: clk-axi-clkgen: make sure to enable the AXI bus clock

[Nuno Sa]

In order to access the registers of the HW, we need to make sure that
the AXI bus clock is enabled.

In order to keep backward compatibility, the new axi clock must be the
last phandle in clocks. On top of that, it's not an actual parent of
the axi-clkgen that needs to be set on struct clk_init_data.

---
Nuno Sa (2):
      dt-bindings: clock: axi-clkgen: include AXI clk
      clk: clk-axi-clkgen: make sure to enable the AXI bus clock

 .../devicetree/bindings/clock/adi,axi-clkgen.yaml  | 21 ++++++++++---
 drivers/clk/clk-axi-clkgen.c                       | 35 +++++++++++++++++++++-
 2 files changed, 51 insertions(+), 5 deletions(-)
---
base-commit: 94be1620fb60ea542170779915917443cda9bba7
change-id: 20241021-axi-clkgen-fix-axiclk-d1d80bcb9ee1
--

Thanks!
- Nuno Sá

[PATCH 1/2] dt-bindings: clock: axi-clkgen: include AXI clk

[Nuno Sa]

In order to access the registers of the HW, we need to make sure that
the AXI bus clock is enabled. Hence let's increase the number of clocks
by one.

In order to keep backward compatibility, the new axi clock must be the
last phandle in the array. To make the intent clear, a non mandatory
clock-names property is also being added.

Fixes: 0e646c52cf0e ("clk: Add axi-clkgen driver")
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 .../devicetree/bindings/clock/adi,axi-clkgen.yaml   | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/adi,axi-clkgen.yaml b/Documentation/devicetree/bindings/clock/adi,axi-clkgen.yaml
index 5e942bccf27787d7029f76fc1a284232fb7f279d..f5f80e61c119b8a68cb6e7a26ed275764f8d200f 100644
--- a/Documentation/devicetree/bindings/clock/adi,axi-clkgen.yaml
+++ b/Documentation/devicetree/bindings/clock/adi,axi-clkgen.yaml
@@ -26,9 +26,21 @@ properties:
     description:
       Specifies the reference clock(s) from which the output frequency is
       derived. This must either reference one clock if only the first clock
-      input is connected or two if both clock inputs are connected.
-    minItems: 1
-    maxItems: 2
+      input is connected or two if both clock inputs are connected. The last
+      clock is the AXI bus clock that needs to be enabled so we can access the
+      core registers.
+    minItems: 2
+    maxItems: 3
+
+  clock-names:
+    oneOf:
+      - items:
+          - const: clkin1
+          - const: s_axi_aclk
+      - items:
+          - const: clkin1
+          - const: clkin2
+          - const: s_axi_aclk
 
   '#clock-cells':
     const: 0
@@ -50,5 +62,6 @@ examples:
       compatible = "adi,axi-clkgen-2.00.a";
       #clock-cells = <0>;
       reg = <0xff000000 0x1000>;
-      clocks = <&osc 1>;
+      clocks = <&osc 1>, <&clkc 15>;
+      clock-names = "clkin1", "s_axi_aclk";
     };

-- 
2.47.0

Re: [PATCH 1/2] dt-bindings: clock: axi-clkgen: include AXI clk

[Conor Dooley]

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

On Wed, Oct 23, 2024 at 04:56:54PM +0200, Nuno Sa wrote:
> In order to access the registers of the HW, we need to make sure that
> the AXI bus clock is enabled. Hence let's increase the number of clocks
> by one.
> 
> In order to keep backward compatibility, the new axi clock must be the
> last phandle in the array. To make the intent clear, a non mandatory
> clock-names property is also being added.

Hmm, I'm not sure. I think clock-names actually may need to be mandatory
here, as otherwise you'll not what the second clock is. The driver would
have to interpret no clock-names meaning clock 2 was clkin2.

> 
> Fixes: 0e646c52cf0e ("clk: Add axi-clkgen driver")
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  .../devicetree/bindings/clock/adi,axi-clkgen.yaml   | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/adi,axi-clkgen.yaml b/Documentation/devicetree/bindings/clock/adi,axi-clkgen.yaml
> index 5e942bccf27787d7029f76fc1a284232fb7f279d..f5f80e61c119b8a68cb6e7a26ed275764f8d200f 100644
> --- a/Documentation/devicetree/bindings/clock/adi,axi-clkgen.yaml
> +++ b/Documentation/devicetree/bindings/clock/adi,axi-clkgen.yaml
> @@ -26,9 +26,21 @@ properties:
>      description:
>        Specifies the reference clock(s) from which the output frequency is
>        derived. This must either reference one clock if only the first clock
> -      input is connected or two if both clock inputs are connected.
> -    minItems: 1
> -    maxItems: 2
> +      input is connected or two if both clock inputs are connected. The last
> +      clock is the AXI bus clock that needs to be enabled so we can access the
> +      core registers.
> +    minItems: 2
> +    maxItems: 3
> +
> +  clock-names:
> +    oneOf:
> +      - items:
> +          - const: clkin1
> +          - const: s_axi_aclk
> +      - items:
> +          - const: clkin1
> +          - const: clkin2
> +          - const: s_axi_aclk
>  
>    '#clock-cells':
>      const: 0
> @@ -50,5 +62,6 @@ examples:
>        compatible = "adi,axi-clkgen-2.00.a";
>        #clock-cells = <0>;
>        reg = <0xff000000 0x1000>;
> -      clocks = <&osc 1>;
> +      clocks = <&osc 1>, <&clkc 15>;
> +      clock-names = "clkin1", "s_axi_aclk";
>      };
> 
> -- 
> 2.47.0
> 

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

Re: [PATCH 1/2] dt-bindings: clock: axi-clkgen: include AXI clk

[Nuno Sá]

On Wed, 2024-10-23 at 17:30 +0100, Conor Dooley wrote:
> On Wed, Oct 23, 2024 at 04:56:54PM +0200, Nuno Sa wrote:
> > In order to access the registers of the HW, we need to make sure that
> > the AXI bus clock is enabled. Hence let's increase the number of clocks
> > by one.
> > 
> > In order to keep backward compatibility, the new axi clock must be the
> > last phandle in the array. To make the intent clear, a non mandatory
> > clock-names property is also being added.
> 
> Hmm, I'm not sure. I think clock-names actually may need to be mandatory
> here, as otherwise you'll not what the second clock is. The driver would
> have to interpret no clock-names meaning clock 2 was clkin2.
> 
> 

So the way things are now is that we just get the parents count with
of_clk_get_parent_count() and then get the names with of_clk_get_parent_name() and
this is given into 'struct clk_init_data'. So they are effectively clk_parents of the
clock we're registering and as you can see clock-names does not really matter. What
I'm trying to do is to keep this and still allow to get the AXI bus clock which is
something we should get and enable and not rely on others to do it. The idea is then
to add the axi bus clock as the last one in the clocks property and I will get it by
index with of_clk_get(). The rest pretty much remains the same and we just need to
decrement by one the number of parent clocks as the axi clock is not really a parent
of our output clock.

All that said, and FWIW, clock-names are not even being used in the driver. I just
added it to the bindings to make the intent clear. I could have it in the driver but
I'm not sure the extra complexity would be worth it...

- Nuno Sá

Re: [PATCH 1/2] dt-bindings: clock: axi-clkgen: include AXI clk

[Conor Dooley]

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

On Thu, Oct 24, 2024 at 02:35:37PM +0200, Nuno Sá wrote:
> On Wed, 2024-10-23 at 17:30 +0100, Conor Dooley wrote:
> > On Wed, Oct 23, 2024 at 04:56:54PM +0200, Nuno Sa wrote:
> > > In order to access the registers of the HW, we need to make sure that
> > > the AXI bus clock is enabled. Hence let's increase the number of clocks
> > > by one.
> > > 
> > > In order to keep backward compatibility, the new axi clock must be the
> > > last phandle in the array. To make the intent clear, a non mandatory
> > > clock-names property is also being added.
> > 
> > Hmm, I'm not sure. I think clock-names actually may need to be mandatory
> > here, as otherwise you'll not what the second clock is. The driver would
> > have to interpret no clock-names meaning clock 2 was clkin2.
> > 
> > 
> 
> So the way things are now is that we just get the parents count with
> of_clk_get_parent_count() and then get the names with of_clk_get_parent_name() and
> this is given into 'struct clk_init_data'. So they are effectively clk_parents of the
> clock we're registering and as you can see clock-names does not really matter. What
> I'm trying to do is to keep this and still allow to get the AXI bus clock which is
> something we should get and enable and not rely on others to do it. The idea is then
> to add the axi bus clock as the last one in the clocks property and I will get it by
> index with of_clk_get(). The rest pretty much remains the same and we just need to
> decrement by one the number of parent clocks as the axi clock is not really a parent
> of our output clock.

I mean, if it works, and you can always disambiguate between whether or
not someone has two clkins or one clkin and the axi clock, then
Acked-by: Conor Dooley <conor.dooley@microchip.com>

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

Re: [PATCH 1/2] dt-bindings: clock: axi-clkgen: include AXI clk

[Nuno Sá]

On Thu, 2024-10-24 at 17:13 +0100, Conor Dooley wrote:
> On Thu, Oct 24, 2024 at 02:35:37PM +0200, Nuno Sá wrote:
> > On Wed, 2024-10-23 at 17:30 +0100, Conor Dooley wrote:
> > > On Wed, Oct 23, 2024 at 04:56:54PM +0200, Nuno Sa wrote:
> > > > In order to access the registers of the HW, we need to make sure that
> > > > the AXI bus clock is enabled. Hence let's increase the number of clocks
> > > > by one.
> > > > 
> > > > In order to keep backward compatibility, the new axi clock must be the
> > > > last phandle in the array. To make the intent clear, a non mandatory
> > > > clock-names property is also being added.
> > > 
> > > Hmm, I'm not sure. I think clock-names actually may need to be mandatory
> > > here, as otherwise you'll not what the second clock is. The driver would
> > > have to interpret no clock-names meaning clock 2 was clkin2.
> > > 
> > > 
> > 
> > So the way things are now is that we just get the parents count with
> > of_clk_get_parent_count() and then get the names with of_clk_get_parent_name()
> > and
> > this is given into 'struct clk_init_data'. So they are effectively clk_parents of
> > the
> > clock we're registering and as you can see clock-names does not really matter.
> > What
> > I'm trying to do is to keep this and still allow to get the AXI bus clock which
> > is
> > something we should get and enable and not rely on others to do it. The idea is
> > then
> > to add the axi bus clock as the last one in the clocks property and I will get it
> > by
> > index with of_clk_get(). The rest pretty much remains the same and we just need
> > to
> > decrement by one the number of parent clocks as the axi clock is not really a
> > parent
> > of our output clock.
> 
> I mean, if it works, and you can always disambiguate between whether or
> not someone has two clkins or one clkin and the axi clock, then
> Acked-by: Conor Dooley <conor.dooley@microchip.com>

The assumption is that the axi clock is the last one in the phandle array. But your
comment made me think a bit more about this and I do see a possible problem if we run
old DTs against a kernel with this patch. We have two possibilities:

1) DT only with one parent clock;
2) DT with two parent clocks;

1) is "fine" as it would now fail to probe. 2) is more problematic as we would assume
the second parent to be the axi_bus clock so effectively not fixing anything and
silently probing with a broken setup.

So yeah, I think I overthinked the backward compatibility thing. I mean, in theory,
all old DTs are not correct and should be fixed by including the axi_clk. And if we
now enforce clock-names we at least get probe errors right away making it clear
(which is far better from silently breaking after probe).

Given the above, it should be fine to just enforce clock-names now, right?

- Nuno Sá

Re: [PATCH 1/2] dt-bindings: clock: axi-clkgen: include AXI clk

[Conor Dooley]

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

On Fri, Oct 25, 2024 at 08:56:34AM +0200, Nuno Sá wrote:
> On Thu, 2024-10-24 at 17:13 +0100, Conor Dooley wrote:
> > On Thu, Oct 24, 2024 at 02:35:37PM +0200, Nuno Sá wrote:
> > > On Wed, 2024-10-23 at 17:30 +0100, Conor Dooley wrote:
> > > > On Wed, Oct 23, 2024 at 04:56:54PM +0200, Nuno Sa wrote:
> > > > > In order to access the registers of the HW, we need to make sure that
> > > > > the AXI bus clock is enabled. Hence let's increase the number of clocks
> > > > > by one.
> > > > > 
> > > > > In order to keep backward compatibility, the new axi clock must be the
> > > > > last phandle in the array. To make the intent clear, a non mandatory
> > > > > clock-names property is also being added.
> > > > 
> > > > Hmm, I'm not sure. I think clock-names actually may need to be mandatory
> > > > here, as otherwise you'll not what the second clock is. The driver would
> > > > have to interpret no clock-names meaning clock 2 was clkin2.
> > > > 
> > > > 
> > > 
> > > So the way things are now is that we just get the parents count with
> > > of_clk_get_parent_count() and then get the names with of_clk_get_parent_name()
> > > and
> > > this is given into 'struct clk_init_data'. So they are effectively clk_parents of
> > > the
> > > clock we're registering and as you can see clock-names does not really matter.
> > > What
> > > I'm trying to do is to keep this and still allow to get the AXI bus clock which
> > > is
> > > something we should get and enable and not rely on others to do it. The idea is
> > > then
> > > to add the axi bus clock as the last one in the clocks property and I will get it
> > > by
> > > index with of_clk_get(). The rest pretty much remains the same and we just need
> > > to
> > > decrement by one the number of parent clocks as the axi clock is not really a
> > > parent
> > > of our output clock.
> > 
> > I mean, if it works, and you can always disambiguate between whether or
> > not someone has two clkins or one clkin and the axi clock, then
> > Acked-by: Conor Dooley <conor.dooley@microchip.com>
> 
> The assumption is that the axi clock is the last one in the phandle array. But your
> comment made me think a bit more about this and I do see a possible problem if we run
> old DTs against a kernel with this patch. We have two possibilities:
> 
> 1) DT only with one parent clock;
> 2) DT with two parent clocks;
> 
> 1) is "fine" as it would now fail to probe. 2) is more problematic as we would assume
> the second parent to be the axi_bus clock so effectively not fixing anything and
> silently probing with a broken setup.
> 
> So yeah, I think I overthinked the backward compatibility thing. I mean, in theory,
> all old DTs are not correct and should be fixed by including the axi_clk. And if we
> now enforce clock-names we at least get probe errors right away making it clear
> (which is far better from silently breaking after probe).
> 
> Given the above, it should be fine to just enforce clock-names now, right?

I think you need to enforce clock-names in the binding and take
!clock-names and 2 clocks to mean that the second one is a clkin. I
think that's a better solution than failing to probe for all extant
devicestrees.

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

Re: [PATCH 1/2] dt-bindings: clock: axi-clkgen: include AXI clk

[Nuno Sá]

On Fri, 2024-10-25 at 17:47 +0100, Conor Dooley wrote:
> On Fri, Oct 25, 2024 at 08:56:34AM +0200, Nuno Sá wrote:
> > On Thu, 2024-10-24 at 17:13 +0100, Conor Dooley wrote:
> > > On Thu, Oct 24, 2024 at 02:35:37PM +0200, Nuno Sá wrote:
> > > > On Wed, 2024-10-23 at 17:30 +0100, Conor Dooley wrote:
> > > > > On Wed, Oct 23, 2024 at 04:56:54PM +0200, Nuno Sa wrote:
> > > > > > In order to access the registers of the HW, we need to make sure that
> > > > > > the AXI bus clock is enabled. Hence let's increase the number of clocks
> > > > > > by one.
> > > > > > 
> > > > > > In order to keep backward compatibility, the new axi clock must be the
> > > > > > last phandle in the array. To make the intent clear, a non mandatory
> > > > > > clock-names property is also being added.
> > > > > 
> > > > > Hmm, I'm not sure. I think clock-names actually may need to be mandatory
> > > > > here, as otherwise you'll not what the second clock is. The driver would
> > > > > have to interpret no clock-names meaning clock 2 was clkin2.
> > > > > 
> > > > > 
> > > > 
> > > > So the way things are now is that we just get the parents count with
> > > > of_clk_get_parent_count() and then get the names with
> > > > of_clk_get_parent_name()
> > > > and
> > > > this is given into 'struct clk_init_data'. So they are effectively
> > > > clk_parents of
> > > > the
> > > > clock we're registering and as you can see clock-names does not really
> > > > matter.
> > > > What
> > > > I'm trying to do is to keep this and still allow to get the AXI bus clock
> > > > which
> > > > is
> > > > something we should get and enable and not rely on others to do it. The idea
> > > > is
> > > > then
> > > > to add the axi bus clock as the last one in the clocks property and I will
> > > > get it
> > > > by
> > > > index with of_clk_get(). The rest pretty much remains the same and we just
> > > > need
> > > > to
> > > > decrement by one the number of parent clocks as the axi clock is not really a
> > > > parent
> > > > of our output clock.
> > > 
> > > I mean, if it works, and you can always disambiguate between whether or
> > > not someone has two clkins or one clkin and the axi clock, then
> > > Acked-by: Conor Dooley <conor.dooley@microchip.com>
> > 
> > The assumption is that the axi clock is the last one in the phandle array. But
> > your
> > comment made me think a bit more about this and I do see a possible problem if we
> > run
> > old DTs against a kernel with this patch. We have two possibilities:
> > 
> > 1) DT only with one parent clock;
> > 2) DT with two parent clocks;
> > 
> > 1) is "fine" as it would now fail to probe. 2) is more problematic as we would
> > assume
> > the second parent to be the axi_bus clock so effectively not fixing anything and
> > silently probing with a broken setup.
> > 
> > So yeah, I think I overthinked the backward compatibility thing. I mean, in
> > theory,
> > all old DTs are not correct and should be fixed by including the axi_clk. And if
> > we
> > now enforce clock-names we at least get probe errors right away making it clear
> > (which is far better from silently breaking after probe).
> > 
> > Given the above, it should be fine to just enforce clock-names now, right?
> 
> I think you need to enforce clock-names in the binding and take
> !clock-names and 2 clocks to mean that the second one is a clkin. I
> think that's a better solution than failing to probe for all extant
> devicestrees.

Ok, so IIUC, you mean leaving old DTs as of today and relying on someone else to
enable the axi clock (if it was not enabled they would have noticed by now). And only
take care of the bus clock when clock-names is provided?

- Nuno Sá

Re: [PATCH 1/2] dt-bindings: clock: axi-clkgen: include AXI clk

[Conor Dooley]

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

On Mon, Oct 28, 2024 at 03:01:44PM +0100, Nuno Sá wrote:

> > > The assumption is that the axi clock is the last one in the phandle array. But
> > > your
> > > comment made me think a bit more about this and I do see a possible problem if we
> > > run
> > > old DTs against a kernel with this patch. We have two possibilities:
> > > 
> > > 1) DT only with one parent clock;
> > > 2) DT with two parent clocks;
> > > 
> > > 1) is "fine" as it would now fail to probe. 2) is more problematic as we would
> > > assume
> > > the second parent to be the axi_bus clock so effectively not fixing anything and
> > > silently probing with a broken setup.
> > > 
> > > So yeah, I think I overthinked the backward compatibility thing. I mean, in
> > > theory,
> > > all old DTs are not correct and should be fixed by including the axi_clk. And if
> > > we
> > > now enforce clock-names we at least get probe errors right away making it clear
> > > (which is far better from silently breaking after probe).
> > > 
> > > Given the above, it should be fine to just enforce clock-names now, right?
> > 
> > I think you need to enforce clock-names in the binding and take
> > !clock-names and 2 clocks to mean that the second one is a clkin. I
> > think that's a better solution than failing to probe for all extant
> > devicestrees.
> 
> Ok, so IIUC, you mean leaving old DTs as of today and relying on someone else to
> enable the axi clock (if it was not enabled they would have noticed by now).

Yea. Obviously any old dts within reach (so those in-tree) should be
updated to have the missing third clock, but as we can't be sure we have
changed all users, so the driver must retain its current functionality
with the old devicetrees.

> And only
> take care of the bus clock when clock-names is provided?

Yup.


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

[PATCH 2/2] clk: clk-axi-clkgen: make sure to enable the AXI bus clock

[Nuno Sa]

In order to access the registers of the HW, we need to make sure that
the AXI bus clock is enabled. Hence let's increase the number of clocks
by one.

In order to keep backward compatibility, the new axi clock must be the
last phandle in the array. On top of that, it's not an actual parent of
the axi-clkgen.

Fixes: 0e646c52cf0e ("clk: Add axi-clkgen driver")
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/clk/clk-axi-clkgen.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-axi-clkgen.c b/drivers/clk/clk-axi-clkgen.c
index bf4d8ddc93aea1d4509a9ae50fa69fdb3f707a29..6403f0e321b4f8fb74852cae2d13507d8cad82df 100644
--- a/drivers/clk/clk-axi-clkgen.c
+++ b/drivers/clk/clk-axi-clkgen.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/platform_device.h>
+#include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/slab.h>
 #include <linux/io.h>
@@ -505,6 +506,16 @@ static const struct clk_ops axi_clkgen_ops = {
 	.get_parent = axi_clkgen_get_parent,
 };
 
+static void axi_clk_put(void *clk)
+{
+	clk_put(clk);
+}
+
+static void axi_clk_disable(void *clk)
+{
+	clk_disable_unprepare(clk);
+}
+
 static int axi_clkgen_probe(struct platform_device *pdev)
 {
 	const struct axi_clkgen_limits *dflt_limits;
@@ -512,6 +523,7 @@ static int axi_clkgen_probe(struct platform_device *pdev)
 	struct clk_init_data init;
 	const char *parent_names[2];
 	const char *clk_name;
+	struct clk *axi_clk;
 	unsigned int i;
 	int ret;
 
@@ -528,9 +540,30 @@ static int axi_clkgen_probe(struct platform_device *pdev)
 		return PTR_ERR(axi_clkgen->base);
 
 	init.num_parents = of_clk_get_parent_count(pdev->dev.of_node);
-	if (init.num_parents < 1 || init.num_parents > 2)
+	if (init.num_parents < 2 || init.num_parents > 3)
 		return -EINVAL;
 
+	/*
+	 * The last clock is the axi bus clock that needs to be enabled so we
+	 * can access the IP core registers.
+	 */
+	axi_clk = of_clk_get(pdev->dev.of_node, init.num_parents - 1);
+	if (IS_ERR(axi_clk))
+		return PTR_ERR(axi_clk);
+
+	ret = devm_add_action_or_reset(&pdev->dev, axi_clk_put, axi_clk);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(axi_clk);
+	if (ret)
+		return ret;
+
+	ret = devm_add_action_or_reset(&pdev->dev, axi_clk_disable, axi_clk);
+	if (ret)
+		return ret;
+
+	init.num_parents -= 1;
 	for (i = 0; i < init.num_parents; i++) {
 		parent_names[i] = of_clk_get_parent_name(pdev->dev.of_node, i);
 		if (!parent_names[i])

-- 
2.47.0