| Message ID | 20190905215532.8357-1-tony@atomide.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: ti: clkctrl: Fix hidden dependency to node name with reg-names | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | "total: 0 errors, 1 warnings, 48 lines checked" |
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>; > }; > };
* 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>; > > }; > > };
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.
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
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?
* 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)
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
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
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!
* 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
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 */
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(-)