[PATCH] clk: ti: clkctrl: Fix hidden dependency to node name with reg-names

[PATCH] clk: ti: clkctrl: Fix hidden dependency to node name with reg-names

[Tony Lindgren]

We currently have a hidden dependency to the device tree node name for
the clkctrl clocks. Instead of using standard node name like "clock", we
must use "l4-per-clkctrl" naming so the clock driver can find the
associated clock domain. Further, if "clk" is specified for a clock node
name, the driver sets TI_CLK_CLKCTRL_COMPAT flag that uses different
logic with earlier naming for the clock node name.

If the clock node naming dependency is not understood, the related
clockdomain is not found, or a wrong one can get used if a clock manager
instance has multiple domains.

As each clkctrl instance represents a single clock domain with it's
reg property describing the clocks available in that clock domain,
we can simply use "reg-names" property for the clock domain.

This simplifies things and removes the hidden dependency to the node
name. And then later on, we should be able to drop the related code
for parsing the node names.

Let's also update the binding to use standard "clock" node naming
instead of "clk".

Cc: devicetree@vger.kernel.org
Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 Documentation/devicetree/bindings/clock/ti-clkctrl.txt |  6 +++++-
 drivers/clk/ti/clkctrl.c                               | 10 ++++++++--
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/ti-clkctrl.txt b/Documentation/devicetree/bindings/clock/ti-clkctrl.txt
--- a/Documentation/devicetree/bindings/clock/ti-clkctrl.txt
+++ b/Documentation/devicetree/bindings/clock/ti-clkctrl.txt
@@ -20,15 +20,19 @@ Required properties :
 - #clock-cells : shall contain 2 with the first entry being the instance
 		 offset from the clock domain base and the second being the
 		 clock index
+- reg : clock registers
+- reg-names : clock register names for the clock, should be same as the
+	      domain name
 
 Example: Clock controller node on omap 4430:
 
 &cm2 {
 	l4per: cm@1400 {
 		cm_l4per@0 {
-			cm_l4per_clkctrl: clk@20 {
+			cm_l4per_clkctrl: clock@20 {
 				compatible = "ti,clkctrl";
 				reg = <0x20 0x1b0>;
+				reg-names = "l4_per";
 				#clock-cells = <2>;
 			};
 		};
diff --git a/drivers/clk/ti/clkctrl.c b/drivers/clk/ti/clkctrl.c
--- a/drivers/clk/ti/clkctrl.c
+++ b/drivers/clk/ti/clkctrl.c
@@ -446,6 +446,7 @@ static void __init _ti_omap4_clkctrl_setup(struct device_node *node)
 	struct clk_hw_omap *hw;
 	struct clk *clk;
 	struct omap_clkctrl_clk *clkctrl_clk;
+	const char *clkdm_name;
 	const __be32 *addrp;
 	u32 addr;
 	int ret;
@@ -534,7 +535,12 @@ static void __init _ti_omap4_clkctrl_setup(struct device_node *node)
 
 	provider->base = of_iomap(node, 0);
 
-	if (ti_clk_get_features()->flags & TI_CLK_CLKCTRL_COMPAT) {
+	ret = of_property_read_string_index(node, "reg-names", 0, &clkdm_name);
+	if (!ret) {
+		provider->clkdm_name = kasprintf(GFP_KERNEL, "%s_clkdm",
+						 clkdm_name);
+		goto clkdm_found;
+	} else if (ti_clk_get_features()->flags & TI_CLK_CLKCTRL_COMPAT) {
 		provider->clkdm_name = kasprintf(GFP_KERNEL, "%pOFnxxx", node->parent);
 		if (!provider->clkdm_name) {
 			kfree(provider);
@@ -570,7 +576,7 @@ static void __init _ti_omap4_clkctrl_setup(struct device_node *node)
 			*c = '_';
 		c++;
 	}
-
+clkdm_found:
 	INIT_LIST_HEAD(&provider->clocks);
 
 	/* Generate clocks */
-- 
2.23.0

Re: [PATCH] clk: ti: clkctrl: Fix hidden dependency to node name with reg-names

[Stephen Boyd]

Quoting Tony Lindgren (2019-09-05 14:55:32)
> We currently have a hidden dependency to the device tree node name for
> the clkctrl clocks. Instead of using standard node name like "clock", we
> must use "l4-per-clkctrl" naming so the clock driver can find the

The node name is "clk" though.

> associated clock domain. Further, if "clk" is specified for a clock node
> name, the driver sets TI_CLK_CLKCTRL_COMPAT flag that uses different
> logic with earlier naming for the clock node name.
> 
> If the clock node naming dependency is not understood, the related
> clockdomain is not found, or a wrong one can get used if a clock manager
> instance has multiple domains.
> 
> As each clkctrl instance represents a single clock domain with it's
> reg property describing the clocks available in that clock domain,
> we can simply use "reg-names" property for the clock domain.
> 
> This simplifies things and removes the hidden dependency to the node
> name. And then later on, we should be able to drop the related code
> for parsing the node names.
> 
> Let's also update the binding to use standard "clock" node naming
> instead of "clk".
> 
> Cc: devicetree@vger.kernel.org
> Cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  Documentation/devicetree/bindings/clock/ti-clkctrl.txt |  6 +++++-
>  drivers/clk/ti/clkctrl.c                               | 10 ++++++++--
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/ti-clkctrl.txt b/Documentation/devicetree/bindings/clock/ti-clkctrl.txt
> --- a/Documentation/devicetree/bindings/clock/ti-clkctrl.txt
> +++ b/Documentation/devicetree/bindings/clock/ti-clkctrl.txt
> @@ -20,15 +20,19 @@ Required properties :
>  - #clock-cells : shall contain 2 with the first entry being the instance
>                  offset from the clock domain base and the second being the
>                  clock index
> +- reg : clock registers
> +- reg-names : clock register names for the clock, should be same as the
> +             domain name

Is this necessary? I'd rather see that the names of the clks don't
actually matter by means of connecting the clk tree through the "clocks"
property when the parent clks are provided by external nodes and through
direct pointers when they're within a controller. If that works then it
should be possible to ignore this name in general?

>  
>  Example: Clock controller node on omap 4430:
>  
>  &cm2 {
>         l4per: cm@1400 {
>                 cm_l4per@0 {
> -                       cm_l4per_clkctrl: clk@20 {
> +                       cm_l4per_clkctrl: clock@20 {
>                                 compatible = "ti,clkctrl";
>                                 reg = <0x20 0x1b0>;
> +                               reg-names = "l4_per";
>                                 #clock-cells = <2>;
>                         };
>                 };

Re: [PATCH] clk: ti: clkctrl: Fix hidden dependency to node name with reg-names

[Tony Lindgren]

* Stephen Boyd <sboyd@kernel.org> [190907 03:55]:
> Quoting Tony Lindgren (2019-09-05 14:55:32)
> > We currently have a hidden dependency to the device tree node name for
> > the clkctrl clocks. Instead of using standard node name like "clock", we
> > must use "l4-per-clkctrl" naming so the clock driver can find the
> 
> The node name is "clk" though.

Yes for some it's "clk" and for some it's "l4-per-clkctrl".

In general, the clock node name should be "clock@foo", rather than
"clk@foo", right?

> > associated clock domain. Further, if "clk" is specified for a clock node
> > name, the driver sets TI_CLK_CLKCTRL_COMPAT flag that uses different
> > logic with earlier naming for the clock node name.
> > 
> > If the clock node naming dependency is not understood, the related
> > clockdomain is not found, or a wrong one can get used if a clock manager
> > instance has multiple domains.
> > 
> > As each clkctrl instance represents a single clock domain with it's
> > reg property describing the clocks available in that clock domain,
> > we can simply use "reg-names" property for the clock domain.
> > 
> > This simplifies things and removes the hidden dependency to the node
> > name. And then later on, we should be able to drop the related code
> > for parsing the node names.
> > 
> > Let's also update the binding to use standard "clock" node naming
> > instead of "clk".
> > 
> > Cc: devicetree@vger.kernel.org
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > ---
> >  Documentation/devicetree/bindings/clock/ti-clkctrl.txt |  6 +++++-
> >  drivers/clk/ti/clkctrl.c                               | 10 ++++++++--
> >  2 files changed, 13 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/ti-clkctrl.txt b/Documentation/devicetree/bindings/clock/ti-clkctrl.txt
> > --- a/Documentation/devicetree/bindings/clock/ti-clkctrl.txt
> > +++ b/Documentation/devicetree/bindings/clock/ti-clkctrl.txt
> > @@ -20,15 +20,19 @@ Required properties :
> >  - #clock-cells : shall contain 2 with the first entry being the instance
> >                  offset from the clock domain base and the second being the
> >                  clock index
> > +- reg : clock registers
> > +- reg-names : clock register names for the clock, should be same as the
> > +             domain name
> 
> Is this necessary? I'd rather see that the names of the clks don't
> actually matter by means of connecting the clk tree through the "clocks"
> property when the parent clks are provided by external nodes and through
> direct pointers when they're within a controller. If that works then it
> should be possible to ignore this name in general?

This is not for names of the clocks :) This is to name the clock
provider register range. The name of the register range is the
same as the clockdomain that this range of clocks belongs to.
This property is used by the clock provider on init to initialize the
clock provider, not when a clock is requested.

In this case a clkctrl clock provider instance has one to some tens
clocks where they all belong to the same domain. If some similar clock
would have multiple domains, then it would simply just have multiple
reg ranges and multiple reg-names properties.

Or do you have some better ideas on how to name a clock controller
in the device tree?

Regards,

Tony

> >  Example: Clock controller node on omap 4430:
> >  
> >  &cm2 {
> >         l4per: cm@1400 {
> >                 cm_l4per@0 {
> > -                       cm_l4per_clkctrl: clk@20 {
> > +                       cm_l4per_clkctrl: clock@20 {
> >                                 compatible = "ti,clkctrl";
> >                                 reg = <0x20 0x1b0>;
> > +                               reg-names = "l4_per";
> >                                 #clock-cells = <2>;
> >                         };
> >                 };

Re: [PATCH] clk: ti: clkctrl: Fix hidden dependency to node name with reg-names

[Stephen Boyd]

Quoting Tony Lindgren (2019-09-08 12:42:41)
> * Stephen Boyd <sboyd@kernel.org> [190907 03:55]:
> > Quoting Tony Lindgren (2019-09-05 14:55:32)
> > > We currently have a hidden dependency to the device tree node name for
> > > the clkctrl clocks. Instead of using standard node name like "clock", we
> > > must use "l4-per-clkctrl" naming so the clock driver can find the
> > 
> > The node name is "clk" though.
> 
> Yes for some it's "clk" and for some it's "l4-per-clkctrl".
> 
> In general, the clock node name should be "clock@foo", rather than
> "clk@foo", right?

I don't think it really matters but sure, clock is nicer because that's
a more standard node name than the vowel-less one.

> 
> > > associated clock domain. Further, if "clk" is specified for a clock node
> > > name, the driver sets TI_CLK_CLKCTRL_COMPAT flag that uses different
> > > logic with earlier naming for the clock node name.
> > > 
> > > If the clock node naming dependency is not understood, the related
> > > clockdomain is not found, or a wrong one can get used if a clock manager
> > > instance has multiple domains.
> > > 
> > > As each clkctrl instance represents a single clock domain with it's
> > > reg property describing the clocks available in that clock domain,
> > > we can simply use "reg-names" property for the clock domain.
> > > 
> > > This simplifies things and removes the hidden dependency to the node
> > > name. And then later on, we should be able to drop the related code
> > > for parsing the node names.
> > > 
> > > Let's also update the binding to use standard "clock" node naming
> > > instead of "clk".
> > > 
> > > Cc: devicetree@vger.kernel.org
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > > ---
> > >  Documentation/devicetree/bindings/clock/ti-clkctrl.txt |  6 +++++-
> > >  drivers/clk/ti/clkctrl.c                               | 10 ++++++++--
> > >  2 files changed, 13 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/clock/ti-clkctrl.txt b/Documentation/devicetree/bindings/clock/ti-clkctrl.txt
> > > --- a/Documentation/devicetree/bindings/clock/ti-clkctrl.txt
> > > +++ b/Documentation/devicetree/bindings/clock/ti-clkctrl.txt
> > > @@ -20,15 +20,19 @@ Required properties :
> > >  - #clock-cells : shall contain 2 with the first entry being the instance
> > >                  offset from the clock domain base and the second being the
> > >                  clock index
> > > +- reg : clock registers
> > > +- reg-names : clock register names for the clock, should be same as the
> > > +             domain name
> > 
> > Is this necessary? I'd rather see that the names of the clks don't
> > actually matter by means of connecting the clk tree through the "clocks"
> > property when the parent clks are provided by external nodes and through
> > direct pointers when they're within a controller. If that works then it
> > should be possible to ignore this name in general?
> 
> This is not for names of the clocks :) This is to name the clock
> provider register range. The name of the register range is the
> same as the clockdomain that this range of clocks belongs to.
> This property is used by the clock provider on init to initialize the
> clock provider, not when a clock is requested.
> 
> In this case a clkctrl clock provider instance has one to some tens
> clocks where they all belong to the same domain. If some similar clock
> would have multiple domains, then it would simply just have multiple
> reg ranges and multiple reg-names properties.
> 
> Or do you have some better ideas on how to name a clock controller
> in the device tree?
> 

Why does the name of the clock controller or clkdm_name matter? Using a
string from reg-names smells like a workaround to describe some sort of
linkage between things that isn't being described in DT today.

Re: [PATCH] clk: ti: clkctrl: Fix hidden dependency to node name with reg-names

[Tony Lindgren]

Hi,

* Stephen Boyd <sboyd@kernel.org> [190918 18:08]:
> Quoting Tony Lindgren (2019-09-08 12:42:41)
> > Or do you have some better ideas on how to name a clock controller
> > in the device tree?
> > 
> 
> Why does the name of the clock controller or clkdm_name matter? Using a
> string from reg-names smells like a workaround to describe some sort of
> linkage between things that isn't being described in DT today.

Correct. This problem will eventually disappear with genpd
handling the clockdomains.

But currently the clockdomain name is parsed from the dt node
name, which is not standard practise. Using reg-names
is a standard binding, and it's usage follows the standand
here to describe the reg range.

Then eventually with genpd, the reg-names will just become
optinoal. But until that happens the $subject patch fixes
issues as described in the patch.

Regards,

Tony

Re: [PATCH] clk: ti: clkctrl: Fix hidden dependency to node name with reg-names

[Stephen Boyd]

Quoting Tony Lindgren (2019-09-18 13:53:44)
> Hi,
> 
> * Stephen Boyd <sboyd@kernel.org> [190918 18:08]:
> > Quoting Tony Lindgren (2019-09-08 12:42:41)
> > > Or do you have some better ideas on how to name a clock controller
> > > in the device tree?
> > > 
> > 
> > Why does the name of the clock controller or clkdm_name matter? Using a
> > string from reg-names smells like a workaround to describe some sort of
> > linkage between things that isn't being described in DT today.
> 
> Correct. This problem will eventually disappear with genpd
> handling the clockdomains.
> 
> But currently the clockdomain name is parsed from the dt node
> name, which is not standard practise. Using reg-names
> is a standard binding, and it's usage follows the standand
> here to describe the reg range.
> 
> Then eventually with genpd, the reg-names will just become
> optinoal. But until that happens the $subject patch fixes
> issues as described in the patch.
> 

Is anything broken? It looks like the hidden dependency on the node name
is being changed to be a slightly less hidden dependency on reg-names.
reg-names is supposed to be an optional property, so we're trading one
thing for another. I still don't understand the reasoning here, but if
Tero is happy to ack/review this change then I'm not too worried about
it assuming the reg-names property eventually becomes optional. Just
seems like more work and DT churn for no to little gain?

Re: [PATCH] clk: ti: clkctrl: Fix hidden dependency to node name with reg-names

[Tony Lindgren]

* Stephen Boyd <sboyd@kernel.org> [190918 23:29]:
> Quoting Tony Lindgren (2019-09-18 13:53:44)
> > Hi,
> > 
> > * Stephen Boyd <sboyd@kernel.org> [190918 18:08]:
> > > Quoting Tony Lindgren (2019-09-08 12:42:41)
> > > > Or do you have some better ideas on how to name a clock controller
> > > > in the device tree?
> > > > 
> > > 
> > > Why does the name of the clock controller or clkdm_name matter? Using a
> > > string from reg-names smells like a workaround to describe some sort of
> > > linkage between things that isn't being described in DT today.
> > 
> > Correct. This problem will eventually disappear with genpd
> > handling the clockdomains.
> > 
> > But currently the clockdomain name is parsed from the dt node
> > name, which is not standard practise. Using reg-names
> > is a standard binding, and it's usage follows the standand
> > here to describe the reg range.
> > 
> > Then eventually with genpd, the reg-names will just become
> > optinoal. But until that happens the $subject patch fixes
> > issues as described in the patch.
> > 
> 
> Is anything broken? It looks like the hidden dependency on the node name
> is being changed to be a slightly less hidden dependency on reg-names.

Yes I agree. This is still way better than relying on dts node names :)

> reg-names is supposed to be an optional property, so we're trading one
> thing for another. I still don't understand the reasoning here, but if
> Tero is happy to ack/review this change then I'm not too worried about
> it assuming the reg-names property eventually becomes optional. Just
> seems like more work and DT churn for no to little gain?

Yeah with genpd the reg-names becomes optional.

What is currently broken is we can get a wrong clockdomain or no
clockdomain for a clock if the clock controller has multiple
domains.

This happens at least with rng for omap4 and 5 where l4_per has two
clock domains (l4_per and l4_sec). I'll be posting rng dts patches
for all the SoCs after -rc1, but below is what's needed for rng on
omap4 for example after the $subject patch for reference. See the
addition of "l4_secure_clkctrl: clock@1a0" node below.

The other option would be to search and replace the "clk@" domains
with "foo@" type domains with more churn. See for example the
changes from "clk@" to "foo@" for dra7 done in b5f8ffbb6fad:

$ git show b5f8ffbb6fad | grep -C1 "clk@"

-               mpu_clkctrl: clk@20 {
+               mpu_clkctrl: mpu-clkctrl@20 {
--

-               ipu_clkctrl: clk@40 {
+               ipu1_clkctrl: ipu1-clkctrl@20 {
...

And we cannot mix "clk@" naming and "foo@" naming as a flag for all
instances is set by "clk@" naming during clkctrl clock init. So all
the clock domain nodes would need to be renamed just to add l4_sec
domain for omap4 and 5.

Anyways, if acceptable, an immutable branch against v5.3 or v5.4-rc1
with just the $subject patch would be great so I can merge it in too
for the related rng changes for v5.5.

I'm not yet sure if the $subject patch is needed for some SoC as a
fix together with a dts change to add the reg-names, but it is
possible.

Regards,

Tony

8< -----------------------
diff --git a/arch/arm/boot/dts/omap4-droid4-xt894.dts b/arch/arm/boot/dts/omap4-droid4-xt894.dts
--- a/arch/arm/boot/dts/omap4-droid4-xt894.dts
+++ b/arch/arm/boot/dts/omap4-droid4-xt894.dts
@@ -653,6 +653,11 @@
 	};
 };
 
+/* RNG is used by secure mode and not accessible */
+&rng_target {
+	status = "disabled";
+};
+
 /* Configure pwm clock source for timers 8 & 9 */
 &timer8 {
 	assigned-clocks = <&abe_clkctrl OMAP4_TIMER8_CLKCTRL 24>;
diff --git a/arch/arm/boot/dts/omap4-l4.dtsi b/arch/arm/boot/dts/omap4-l4.dtsi
--- a/arch/arm/boot/dts/omap4-l4.dtsi
+++ b/arch/arm/boot/dts/omap4-l4.dtsi
@@ -2010,12 +2010,26 @@
 			};
 		};
 
-		target-module@90000 {			/* 0x48090000, ap 57 2a.0 */
-			compatible = "ti,sysc";
-			status = "disabled";
+		rng_target: target-module@90000 {	/* 0x48090000, ap 57 2a.0 */
+			compatible = "ti,sysc-omap2", "ti,sysc";
+			reg = <0x91fe0 0x4>,
+			      <0x91fe4 0x4>;
+			reg-names = "rev", "sysc";
+			ti,sysc-mask = <(SYSC_OMAP2_AUTOIDLE)>;
+			ti,sysc-sidle = <SYSC_IDLE_FORCE>,
+					<SYSC_IDLE_NO>;
+			/* Domains (P, C): l4per_pwrdm, l4_secure_clkdm */
+			clocks = <&l4_secure_clkctrl OMAP4_RNG_CLKCTRL 0>;
+			clock-names = "fck";
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges = <0x0 0x90000 0x2000>;
+
+			rng: rng@0 {
+				compatible = "ti,omap4-rng";
+				reg = <0x0 0x2000>;
+				interrupts = <GIC_SPI 52 IRQ_TYPE_LEVEL_HIGH>;
+			};
 		};
 
 		target-module@96000 {			/* 0x48096000, ap 37 26.0 */
diff --git a/arch/arm/boot/dts/omap44xx-clocks.dtsi b/arch/arm/boot/dts/omap44xx-clocks.dtsi
--- a/arch/arm/boot/dts/omap44xx-clocks.dtsi
+++ b/arch/arm/boot/dts/omap44xx-clocks.dtsi
@@ -1284,8 +1284,14 @@
 			reg = <0x20 0x144>;
 			#clock-cells = <2>;
 		};
-	};
 
+		l4_secure_clkctrl: clock@1a0 {
+			compatible = "ti,clkctrl";
+			reg = <0x1a0 0x28>;
+			reg-names = "l4_secure";
+			#clock-cells = <2>;
+		};
+	};
 };
 
 &prm {
diff --git a/drivers/clk/ti/clk-44xx.c b/drivers/clk/ti/clk-44xx.c
--- a/drivers/clk/ti/clk-44xx.c
+++ b/drivers/clk/ti/clk-44xx.c
@@ -604,6 +604,12 @@ static const struct omap_clkctrl_reg_data omap4_l4_per_clkctrl_regs[] __initcons
 	{ 0 },
 };
 
+static const struct
+omap_clkctrl_reg_data omap4_l4_secure_clkctrl_regs[] __initconst = {
+	{ OMAP4_RNG_CLKCTRL, NULL, CLKF_HW_SUP | CLKF_SOC_NONSEC, "" },
+	{ 0 },
+};
+
 static const struct omap_clkctrl_bit_data omap4_gpio1_bit_data[] __initconst = {
 	{ 8, TI_CLK_GATE, omap4_gpio2_dbclk_parents, NULL },
 	{ 0 },
@@ -691,6 +697,7 @@ const struct omap_clkctrl_data omap4_clkctrl_data[] __initconst = {
 	{ 0x4a009220, omap4_l3_gfx_clkctrl_regs },
 	{ 0x4a009320, omap4_l3_init_clkctrl_regs },
 	{ 0x4a009420, omap4_l4_per_clkctrl_regs },
+	{ 0x4a0095a0, omap4_l4_secure_clkctrl_regs },
 	{ 0x4a307820, omap4_l4_wkup_clkctrl_regs },
 	{ 0x4a307a20, omap4_emu_sys_clkctrl_regs },
 	{ 0 },
diff --git a/include/dt-bindings/clock/omap4.h b/include/dt-bindings/clock/omap4.h
--- a/include/dt-bindings/clock/omap4.h
+++ b/include/dt-bindings/clock/omap4.h
@@ -124,6 +124,11 @@
 #define OMAP4_UART4_CLKCTRL	OMAP4_CLKCTRL_INDEX(0x158)
 #define OMAP4_MMC5_CLKCTRL	OMAP4_CLKCTRL_INDEX(0x160)
 
+/* l4_secure clocks */
+#define OMAP4_L4_SECURE_CLKCTRL_OFFSET	0x1a0
+#define OMAP4_L4_SECURE_CLKCTRL_INDEX(offset)	((offset) - OMAP4_L4_SECURE_CLKCTRL_OFFSET)
+#define OMAP4_RNG_CLKCTRL	OMAP4_L4_SECURE_CLKCTRL_INDEX(0x1c0)
+
 /* l4_wkup clocks */
 #define OMAP4_L4_WKUP_CLKCTRL	OMAP4_CLKCTRL_INDEX(0x20)
 #define OMAP4_WD_TIMER2_CLKCTRL	OMAP4_CLKCTRL_INDEX(0x30)
-- 
2.23.0

Re: [PATCH] clk: ti: clkctrl: Fix hidden dependency to node name with reg-names

[Tero Kristo]

On 06/09/2019 00:55, Tony Lindgren wrote:
> We currently have a hidden dependency to the device tree node name for
> the clkctrl clocks. Instead of using standard node name like "clock", we
> must use "l4-per-clkctrl" naming so the clock driver can find the
> associated clock domain. Further, if "clk" is specified for a clock node
> name, the driver sets TI_CLK_CLKCTRL_COMPAT flag that uses different
> logic with earlier naming for the clock node name.
> 
> If the clock node naming dependency is not understood, the related
> clockdomain is not found, or a wrong one can get used if a clock manager
> instance has multiple domains.
> 
> As each clkctrl instance represents a single clock domain with it's
> reg property describing the clocks available in that clock domain,
> we can simply use "reg-names" property for the clock domain.
> 
> This simplifies things and removes the hidden dependency to the node
> name. And then later on, we should be able to drop the related code
> for parsing the node names.
> 
> Let's also update the binding to use standard "clock" node naming
> instead of "clk".
> 
> Cc: devicetree@vger.kernel.org
> Cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>   Documentation/devicetree/bindings/clock/ti-clkctrl.txt |  6 +++++-
>   drivers/clk/ti/clkctrl.c                               | 10 ++++++++--
>   2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/ti-clkctrl.txt b/Documentation/devicetree/bindings/clock/ti-clkctrl.txt
> --- a/Documentation/devicetree/bindings/clock/ti-clkctrl.txt
> +++ b/Documentation/devicetree/bindings/clock/ti-clkctrl.txt
> @@ -20,15 +20,19 @@ Required properties :
>   - #clock-cells : shall contain 2 with the first entry being the instance
>   		 offset from the clock domain base and the second being the
>   		 clock index
> +- reg : clock registers
> +- reg-names : clock register names for the clock, should be same as the
> +	      domain name

Hmm, I think using the reg-names property like this is kind of wrong. 
Basically, reg and reg-names have pretty much nothing in common. 
Shouldn't you instead use something like ti,clkdm-name? This also breaks 
with SoCs like am3, which have mutant clkctrl entries like the one here:

                 l4ls_clkctrl: l4ls-clkctrl@38 {
                         compatible = "ti,clkctrl";
                         reg = <0x38 0x2c>, <0x6c 0x28>, <0xac 0xc>, 
<0xc0 0x1c>, <0xec 0xc>, <0x10c 0x8>, <0x130 0x4>;
                         #clock-cells = <2>;
                 };

What would you think single entry in reg-names would mean in this case?

Other than that, I think the approach taken in this patch looks fine.

-Tero

>   
>   Example: Clock controller node on omap 4430:
>   
>   &cm2 {
>   	l4per: cm@1400 {
>   		cm_l4per@0 {
> -			cm_l4per_clkctrl: clk@20 {
> +			cm_l4per_clkctrl: clock@20 {
>   				compatible = "ti,clkctrl";
>   				reg = <0x20 0x1b0>;
> +				reg-names = "l4_per";
>   				#clock-cells = <2>;
>   			};
>   		};
> diff --git a/drivers/clk/ti/clkctrl.c b/drivers/clk/ti/clkctrl.c
> --- a/drivers/clk/ti/clkctrl.c
> +++ b/drivers/clk/ti/clkctrl.c
> @@ -446,6 +446,7 @@ static void __init _ti_omap4_clkctrl_setup(struct device_node *node)
>   	struct clk_hw_omap *hw;
>   	struct clk *clk;
>   	struct omap_clkctrl_clk *clkctrl_clk;
> +	const char *clkdm_name;
>   	const __be32 *addrp;
>   	u32 addr;
>   	int ret;
> @@ -534,7 +535,12 @@ static void __init _ti_omap4_clkctrl_setup(struct device_node *node)
>   
>   	provider->base = of_iomap(node, 0);
>   
> -	if (ti_clk_get_features()->flags & TI_CLK_CLKCTRL_COMPAT) {
> +	ret = of_property_read_string_index(node, "reg-names", 0, &clkdm_name);
> +	if (!ret) {
> +		provider->clkdm_name = kasprintf(GFP_KERNEL, "%s_clkdm",
> +						 clkdm_name);
> +		goto clkdm_found;
> +	} else if (ti_clk_get_features()->flags & TI_CLK_CLKCTRL_COMPAT) {
>   		provider->clkdm_name = kasprintf(GFP_KERNEL, "%pOFnxxx", node->parent);
>   		if (!provider->clkdm_name) {
>   			kfree(provider);
> @@ -570,7 +576,7 @@ static void __init _ti_omap4_clkctrl_setup(struct device_node *node)
>   			*c = '_';
>   		c++;
>   	}
> -
> +clkdm_found:
>   	INIT_LIST_HEAD(&provider->clocks);
>   
>   	/* Generate clocks */
> 

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH] clk: ti: clkctrl: Fix hidden dependency to node name with reg-names

[Tony Lindgren]

Hi,

* Tero Kristo <t-kristo@ti.com> [190919 06:46]:
> On 06/09/2019 00:55, Tony Lindgren wrote:
> > diff --git a/Documentation/devicetree/bindings/clock/ti-clkctrl.txt b/Documentation/devicetree/bindings/clock/ti-clkctrl.txt
> > --- a/Documentation/devicetree/bindings/clock/ti-clkctrl.txt
> > +++ b/Documentation/devicetree/bindings/clock/ti-clkctrl.txt
> > @@ -20,15 +20,19 @@ Required properties :
> >   - #clock-cells : shall contain 2 with the first entry being the instance
> >   		 offset from the clock domain base and the second being the
> >   		 clock index
> > +- reg : clock registers
> > +- reg-names : clock register names for the clock, should be same as the
> > +	      domain name
> 
> Hmm, I think using the reg-names property like this is kind of wrong.
> Basically, reg and reg-names have pretty much nothing in common. Shouldn't
> you instead use something like ti,clkdm-name? This also breaks with SoCs
> like am3, which have mutant clkctrl entries like the one here:
> 
>                 l4ls_clkctrl: l4ls-clkctrl@38 {
>                         compatible = "ti,clkctrl";
>                         reg = <0x38 0x2c>, <0x6c 0x28>, <0xac 0xc>, <0xc0
> 0x1c>, <0xec 0xc>, <0x10c 0x8>, <0x130 0x4>;
>                         #clock-cells = <2>;
>                 };
> 
> What would you think single entry in reg-names would mean in this case?

Oh right, I forgot about the mixed register case again.
These are all in l4ls domain..

So sounds like the best option is just to allow adding more
specific compatible values like this for the omap4 rng case:

	l4_secure_clkctrl: clock@1a0 {
		compatible = "ti,clkctrl-omap4-l4-secure", "ti,clkctrl";
		reg = <0x1a0 0x28>;
		#clock-cells = <2>;
	};

And then use match data to get the domain name on init.

Regards,

Tony

Re: [PATCH] clk: ti: clkctrl: Fix hidden dependency to node name with reg-names

[Stephen Boyd]

Quoting Tony Lindgren (2019-09-19 07:12:24)
> Hi,
> 
> * Tero Kristo <t-kristo@ti.com> [190919 06:46]:
> > On 06/09/2019 00:55, Tony Lindgren wrote:
> > > diff --git a/Documentation/devicetree/bindings/clock/ti-clkctrl.txt b/Documentation/devicetree/bindings/clock/ti-clkctrl.txt
> > > --- a/Documentation/devicetree/bindings/clock/ti-clkctrl.txt
> > > +++ b/Documentation/devicetree/bindings/clock/ti-clkctrl.txt
> > > @@ -20,15 +20,19 @@ Required properties :
> > >   - #clock-cells : shall contain 2 with the first entry being the instance
> > >              offset from the clock domain base and the second being the
> > >              clock index
> > > +- reg : clock registers
> > > +- reg-names : clock register names for the clock, should be same as the
> > > +         domain name
> > 
> > Hmm, I think using the reg-names property like this is kind of wrong.
> > Basically, reg and reg-names have pretty much nothing in common. Shouldn't
> > you instead use something like ti,clkdm-name? This also breaks with SoCs
> > like am3, which have mutant clkctrl entries like the one here:
> > 
> >                 l4ls_clkctrl: l4ls-clkctrl@38 {
> >                         compatible = "ti,clkctrl";
> >                         reg = <0x38 0x2c>, <0x6c 0x28>, <0xac 0xc>, <0xc0
> > 0x1c>, <0xec 0xc>, <0x10c 0x8>, <0x130 0x4>;
> >                         #clock-cells = <2>;
> >                 };
> > 
> > What would you think single entry in reg-names would mean in this case?
> 
> Oh right, I forgot about the mixed register case again.
> These are all in l4ls domain..
> 
> So sounds like the best option is just to allow adding more
> specific compatible values like this for the omap4 rng case:
> 
>         l4_secure_clkctrl: clock@1a0 {
>                 compatible = "ti,clkctrl-omap4-l4-secure", "ti,clkctrl";
>                 reg = <0x1a0 0x28>;
>                 #clock-cells = <2>;
>         };
> 
> And then use match data to get the domain name on init.
> 

The existing ti,clkctrl binding is pretty weird. I still believe that
the CM container node should be the only node and it should be logic in
the driver that describes the clks provided by the CM node. I guess I
have to just ignore this stuff because it's all working! 

Re: [PATCH] clk: ti: clkctrl: Fix hidden dependency to node name with reg-names

[Tony Lindgren]

* Stephen Boyd <sboyd@kernel.org> [190919 16:51]:
> Quoting Tony Lindgren (2019-09-19 07:12:24)
> > Hi,
> > 
> > * Tero Kristo <t-kristo@ti.com> [190919 06:46]:
> > > On 06/09/2019 00:55, Tony Lindgren wrote:
> > > > diff --git a/Documentation/devicetree/bindings/clock/ti-clkctrl.txt b/Documentation/devicetree/bindings/clock/ti-clkctrl.txt
> > > > --- a/Documentation/devicetree/bindings/clock/ti-clkctrl.txt
> > > > +++ b/Documentation/devicetree/bindings/clock/ti-clkctrl.txt
> > > > @@ -20,15 +20,19 @@ Required properties :
> > > >   - #clock-cells : shall contain 2 with the first entry being the instance
> > > >              offset from the clock domain base and the second being the
> > > >              clock index
> > > > +- reg : clock registers
> > > > +- reg-names : clock register names for the clock, should be same as the
> > > > +         domain name
> > > 
> > > Hmm, I think using the reg-names property like this is kind of wrong.
> > > Basically, reg and reg-names have pretty much nothing in common. Shouldn't
> > > you instead use something like ti,clkdm-name? This also breaks with SoCs
> > > like am3, which have mutant clkctrl entries like the one here:
> > > 
> > >                 l4ls_clkctrl: l4ls-clkctrl@38 {
> > >                         compatible = "ti,clkctrl";
> > >                         reg = <0x38 0x2c>, <0x6c 0x28>, <0xac 0xc>, <0xc0
> > > 0x1c>, <0xec 0xc>, <0x10c 0x8>, <0x130 0x4>;
> > >                         #clock-cells = <2>;
> > >                 };
> > > 
> > > What would you think single entry in reg-names would mean in this case?
> > 
> > Oh right, I forgot about the mixed register case again.
> > These are all in l4ls domain..
> > 
> > So sounds like the best option is just to allow adding more
> > specific compatible values like this for the omap4 rng case:
> > 
> >         l4_secure_clkctrl: clock@1a0 {
> >                 compatible = "ti,clkctrl-omap4-l4-secure", "ti,clkctrl";
> >                 reg = <0x1a0 0x28>;
> >                 #clock-cells = <2>;
> >         };
> > 
> > And then use match data to get the domain name on init.
> > 
> 
> The existing ti,clkctrl binding is pretty weird. I still believe that
> the CM container node should be the only node and it should be logic in
> the driver that describes the clks provided by the CM node. I guess I
> have to just ignore this stuff because it's all working! 

There can be multiple clockdomains within a single CM. So again using
the l4_secure_clkctrl as an example, the l4_per CM instance with finer
grained compatible properites becomes:

	l4_per_cm: l4_per_cm@1400 {
		compatible = "ti,omap4-cm";
		reg = <0x1400 0x200>;
		#address-cells = <1>;
		#size-cells = <1>;
		ranges = <0 0x1400 0x200>;

		l4_per_clkctrl: clk@20 {
			compatible = "ti,clkctrl";
			reg = <0x20 0x144>;
			#clock-cells = <2>;
		};

		l4_secure_clkctrl: clock@1a0 {
			compatible = "ti,clkctrl-omap4-l4-secure", "ti,clkctrl";
			reg = <0x1a0 0x28>;
			#clock-cells = <2>;
		};
	};

And then later on as clean-up, we could also update l4_per_clkctrl:

		l4_per_clkctrl: clock@20 {
			compatible = "ti,clkctrl-omap4-l4-per", "ti,clkctrl";
			reg = <0x20 0x144>;
			#clock-cells = <2>;
		};

Regards,

Tony