clk: ti: clkctrl: Fix hidden dependency to node name with reg-names
Subsystems (3)
PATCH 1/1 clk: ti: clkctrl: Fix hidden dependency to node name with reg-names
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".
--- Documentation/devicetree/bindings/clock/ti-clkctrl.txt | 6 +++++- drivers/clk/ti/clkctrl.c | 10 ++++++++-- 2 files changed, 13 insertions(+), 3 deletions(-)
Documentation/devicetree/bindings/clock/ti-clkctrl.txt +5 -1
@@ -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>; }; };
drivers/clk/ti/clkctrl.c +8 -2
@@ -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 */
Signature · 1 line
2.23.0
Stephen Boyd
maintainerQuoting Tony Lindgren (2019-09-05 14:55:32)
Quoted 3 lines
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.
Quoted 31 lines
associated clock domain. Further, if "clk" is specified for a clock node name, the driver sets
TI_CLK_CLKCTRL_COMPATflag 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 Rob Herring <[email protected]>Signed-off-by Tony Lindgren <[email protected]>--- Documentation/devicetree/bindings/clock/ti-clkctrl.txt | 6 +++++- drivers/clk/ti/clkctrl.c | 10 ++++++++-- 2 files changed, 13 insertions(+), 3 deletions(-)
Documentation/devicetree/bindings/clock/ti-clkctrl.txt Required properties : · line 25
Quoted 6 lines
Documentation/devicetree/bindings/clock/ti-clkctrl.txt +3
-0@@ -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?
Quoted 14 lines
Documentation/devicetree/bindings/clock/ti-clkctrl.txt +2
-1Example: 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>; }; };
Quoted 6 lines · Stephen Boyd
Quoting Tony Lindgren (2019-09-05 14:55:32)
Quoted 3 lines
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?
Quoted 27 lines
Quoted 27 lines
associated clock domain. Further, if "clk" is specified for a clock node name, the driver sets
TI_CLK_CLKCTRL_COMPATflag 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 Rob Herring <[email protected]>Signed-off-by Tony Lindgren <[email protected]>--- Documentation/devicetree/bindings/clock/ti-clkctrl.txt | 6 +++++- drivers/clk/ti/clkctrl.c | 10 ++++++++-- 2 files changed, 13 insertions(+), 3 deletions(-)
Quoted 16 lines
Documentation/devicetree/bindings/clock/ti-clkctrl.txt Required properties : · line 25
Quoted 10 lines
Documentation/devicetree/bindings/clock/ti-clkctrl.txt +3
-0@@ -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 nameIs 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
Quoted 13 lines
Quoted 13 lines
Documentation/devicetree/bindings/clock/ti-clkctrl.txt +2
-1Example: 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>; }; };
Stephen Boyd
maintainerQuoting Tony Lindgren (2019-09-08 12:42:41)
Quoted 6 lines · Stephen Boyd
Quoted 5 lines
Quoting Tony Lindgren (2019-09-05 14:55:32)
Quoted 3 lines
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
Quoted 6 lines
Quoted 1 line
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.
Quoted 51 lines
Quoted 28 lines
Quoted 28 lines
associated clock domain. Further, if "clk" is specified for a clock node name, the driver sets
TI_CLK_CLKCTRL_COMPATflag 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 Rob Herring <[email protected]>Signed-off-by Tony Lindgren <[email protected]>--- Documentation/devicetree/bindings/clock/ti-clkctrl.txt | 6 +++++- drivers/clk/ti/clkctrl.c | 10 ++++++++-- 2 files changed, 13 insertions(+), 3 deletions(-)Quoted 16 lines
Documentation/devicetree/bindings/clock/ti-clkctrl.txt Required properties : · line 25
Quoted 10 lines
Documentation/devicetree/bindings/clock/ti-clkctrl.txt +3
-0@@ -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 nameIs 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.
Quoted 8 lines
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.
Hi,
Quoted 8 lines · Stephen Boyd
Quoting Tony Lindgren (2019-09-08 12:42:41)
Quoted 3 lines
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_namematter? 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
Stephen Boyd
maintainerQuoting Tony Lindgren (2019-09-18 13:53:44)
Quoted 15 lines
Hi,
Quoted 8 lines · Stephen Boyd
Quoting Tony Lindgren (2019-09-08 12:42:41)
Quoted 3 lines
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_namematter? 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.
Quoted 9 lines
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?
Quoted 21 lines · Stephen Boyd
Quoting Tony Lindgren (2019-09-18 13:53:44)
Quoted 20 lines
Hi,
Quoted 8 lines · Stephen Boyd
Quoting Tony Lindgren (2019-09-08 12:42:41)
Quoted 3 lines
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_namematter? 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.
Quoted 7 lines
Quoted 4 lines
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 :)
Quoted 5 lines
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< -----------------------
arch/arm/boot/dts/omap4-droid4-xt894.dts +5 -0
@@ -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>;
arch/arm/boot/dts/omap4-l4.dtsi +17 -3
@@ -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 */
arch/arm/boot/dts/omap44xx-clocks.dtsi +7 -1
@@ -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 {
drivers/clk/ti/clk-44xx.c +7 -0
@@ -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 },
include/dt-bindings/clock/omap4.h +5 -0
@@ -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)
Signature · 1 line
2.23.0
Quoted 34 lines · 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_COMPATflag 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 Rob Herring <[email protected]>Signed-off-by Tony Lindgren <[email protected]>--- Documentation/devicetree/bindings/clock/ti-clkctrl.txt | 6 +++++- drivers/clk/ti/clkctrl.c | 10 ++++++++-- 2 files changed, 13 insertions(+), 3 deletions(-)
Documentation/devicetree/bindings/clock/ti-clkctrl.txt Required properties : · line 25
Quoted 6 lines
Documentation/devicetree/bindings/clock/ti-clkctrl.txt +3
-0@@ -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
Quoted 41 lines
Documentation/devicetree/bindings/clock/ti-clkctrl.txt +2
-1Example: 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>; }; };drivers/clk/ti/clkctrl.c +7
-1@@ -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);
drivers/clk/ti/clkctrl.c static void __init _ti_omap4_clkctrl_setup(struct device_node *node) · line 582
Quoted 8 lines
drivers/clk/ti/clkctrl.c +1
-1@@ -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
Hi,
Quoted 17 lines · Tero Kristo
Documentation/devicetree/bindings/clock/ti-clkctrl.txt Required properties : · line 25
Quoted 10 lines · Tony Lindgren
Documentation/devicetree/bindings/clock/ti-clkctrl.txt +3
-0@@ -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 nameHmm, 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:
Quoted 8 lines
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
Stephen Boyd
maintainerQuoting Tony Lindgren (2019-09-19 07:12:24)
Quoted 35 lines
Hi,
Quoted 17 lines · Tero Kristo
Documentation/devicetree/bindings/clock/ti-clkctrl.txt Required properties : · line 25
Quoted 10 lines · Tony Lindgren
Documentation/devicetree/bindings/clock/ti-clkctrl.txt +3
-0@@ -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 nameHmm, 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:
Quoted 8 lines
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:
Quoted 8 lines
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!
Quoted 42 lines · Stephen Boyd
Quoting Tony Lindgren (2019-09-19 07:12:24)
Quoted 41 lines
Hi,
Quoted 17 lines · Tero Kristo
Documentation/devicetree/bindings/clock/ti-clkctrl.txt Required properties : · line 25
Quoted 10 lines · Tony Lindgren
Documentation/devicetree/bindings/clock/ti-clkctrl.txt +3
-0@@ -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 nameHmm, 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:
Quoted 8 lines
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>; };
Quoted 7 lines
Quoted 2 lines
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