| Message ID | 52F506E4.8070900@elopez.com.ar (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/02/2014 17:16, Emilio López wrote: > Hi Gregory, > > El 07/02/14 12:12, Gregory CLEMENT escribió: >> On 07/02/2014 16:00, Emilio López wrote: >>> El 07/02/14 11:49, Gregory CLEMENT escribió: >>>> On 07/02/2014 15:43, Ezequiel Garcia wrote: >>>>> On Fri, Feb 07, 2014 at 09:24:30AM -0500, Jason Cooper wrote: >>>>>> On Fri, Feb 07, 2014 at 10:06:08AM -0300, Emilio López wrote: >>>>>> >>>>>> [snip a great explanation] >>>>>> >>>>>> Guys, can I get some Tested-by's on this? >>>>>> >>>>> >>>>> In case someone missed Emilio's comment about it, I gave his oneliner >>>>> a test on A370 Reference Design. It worked just as well as Sebastian's. >>>> >>>> Well ok it's working but this patch is not better than Sebastian, it is >>>> even worth. I don't think it is a good idea at all to totally ignore the >>>> information given by the device tree. >>> >>> With a bit more work, you can replace the clk_get magic with a call to >>> of_clk_get_parent_name() or similar to be able to keep overriding stuff >>> from DT. This way it would completely match the behaviour on >>> mvebu_coreclk_setup (default to "tclk", allow overriding with DT). >>> >> >> I think you didn't have a look on our implementation: > > I did, several times in fact. > >> the name of the clock >> are created by the driver during the initialization. > > The name of the clock is always "tclk", as hardcoded on the driver, > unless overridden via clock-output-names from DT (see > mvebu_coreclk_setup). Currently none of your DT that I can see does > this, so in practice it's always "tclk". It's not because currently we always use "tclk" that we should not use the information given by the device tree. Else it will be very misleading to have a information written in the dts and doing something else in the code. > >> That's why we need that >> the parent clock are initialized before the gating clock. > > You don't really *need* that. The driver just does that because it may We need it, see my comment in your code. > have been convenient at the time and it worked. Defaulting to "tclk" on > mvebu_clk_gating_setup and overriding it if it turns out it has some > other name on the DT (just like on mvebu_coreclk_setup!) should work as > well, and doesn't require complex, bloaty, dependency management. > > Rough, untested patch below, so you get the idea. > > Cheers, > > Emilio > > > > --- > drivers/clk/mvebu/common.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/clk/mvebu/common.c b/drivers/clk/mvebu/common.c > index 25ceccf..730625b 100644 > --- a/drivers/clk/mvebu/common.c > +++ b/drivers/clk/mvebu/common.c > @@ -119,19 +119,20 @@ void __init mvebu_clk_gating_setup(struct > device_node *np, > const struct clk_gating_soc_desc *desc) > { > struct clk_gating_ctrl *ctrl; > - struct clk *clk; > void __iomem *base; > - const char *default_parent = NULL; > + struct of_phandle_args clkspec; > + const char *default_parent = "tclk"; > int n; > > base = of_iomap(np, 0); > if (WARN_ON(!base)) > return; > > - clk = of_clk_get(np, 0); > - if (!IS_ERR(clk)) { > - default_parent = __clk_get_name(clk); > - clk_put(clk); > + if (!of_parse_phandle_with_args(np, "clocks", "#clock-cells", 0, > &clkspec)) { > + of_property_read_string_index(clkspec.np, "clock-output-names", > + clkspec.args_count ? clkspec.args[0] : 0, > + &default_parent); > + of_node_put(clkspec.np); OK here you duplicate the code from of_clk_get_parent_name, I wonder why you didn't use the function.But whatever. Here you will get default_parent = "mvebu-sar" which is the name of the node, you can't have "tclk" because this name is not in the device tree, but it will be created by the initialization of the core clocks. > } > > ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL); >
Hi, El 07/02/14 15:10, Gregory CLEMENT escribió: (snip) >> --- >> drivers/clk/mvebu/common.c | 13 +++++++------ >> 1 file changed, 7 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/clk/mvebu/common.c b/drivers/clk/mvebu/common.c >> index 25ceccf..730625b 100644 >> --- a/drivers/clk/mvebu/common.c >> +++ b/drivers/clk/mvebu/common.c >> @@ -119,19 +119,20 @@ void __init mvebu_clk_gating_setup(struct >> device_node *np, >> const struct clk_gating_soc_desc *desc) >> { >> struct clk_gating_ctrl *ctrl; >> - struct clk *clk; >> void __iomem *base; >> - const char *default_parent = NULL; >> + struct of_phandle_args clkspec; >> + const char *default_parent = "tclk"; >> int n; >> >> base = of_iomap(np, 0); >> if (WARN_ON(!base)) >> return; >> >> - clk = of_clk_get(np, 0); >> - if (!IS_ERR(clk)) { >> - default_parent = __clk_get_name(clk); >> - clk_put(clk); >> + if (!of_parse_phandle_with_args(np, "clocks", "#clock-cells", 0, >> &clkspec)) { >> + of_property_read_string_index(clkspec.np, "clock-output-names", >> + clkspec.args_count ? clkspec.args[0] : 0, >> + &default_parent); >> + of_node_put(clkspec.np); > > OK here you duplicate the code from of_clk_get_parent_name, I wonder why > you didn't use the function.But whatever. I specifically duplicated it because of_clk_get_parent_name returns the node name if there's no clock-output-names. That wouldn't be useful in this case. > Here you will get default_parent = "mvebu-sar" which is the name of the node, you > can't have "tclk" because this name is not in the device tree, but it will be > created by the initialization of the core clocks. No, default_parent would be "tclk" still because clock-output-names property doesn't exist. If you ever add such a property, it will override "tclk". Cheers, Emilio
diff --git a/drivers/clk/mvebu/common.c b/drivers/clk/mvebu/common.c index 25ceccf..730625b 100644 --- a/drivers/clk/mvebu/common.c +++ b/drivers/clk/mvebu/common.c @@ -119,19 +119,20 @@ void __init mvebu_clk_gating_setup(struct device_node *np, const struct clk_gating_soc_desc *desc) { struct clk_gating_ctrl *ctrl; - struct clk *clk; void __iomem *base; - const char *default_parent = NULL; + struct of_phandle_args clkspec; + const char *default_parent = "tclk"; int n; base = of_iomap(np, 0); if (WARN_ON(!base)) return; - clk = of_clk_get(np, 0); - if (!IS_ERR(clk)) { - default_parent = __clk_get_name(clk); - clk_put(clk); + if (!of_parse_phandle_with_args(np, "clocks", "#clock-cells", 0, &clkspec)) { + of_property_read_string_index(clkspec.np, "clock-output-names", + clkspec.args_count ? clkspec.args[0] : 0, + &default_parent); + of_node_put(clkspec.np); }