| Message ID | 20150909050554.27129.68718.sendpatchset@little-apple (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Magnus, Thank you for the patch. On Wednesday 09 September 2015 14:05:54 Magnus Damm wrote: > From: Magnus Damm <damm+renesas@opensource.se> > > This patch hacks the CCF core to take clock-indices into > consideration when making a default clock name in case > clock-output-names is not provided. > > Without this patch of_clk_get_parent_name() does not work > for clocks with multiple indices associated with one node. > > Proof of concept only. Leaks memory. Not for upstream merge. > > Not-Yet-Signed-off-by: Magnus Damm <damm+renesas@opensource.se> > --- > > Written on top of "renesas-drivers-2015-09-08-v4.2" > Needed to propagate clock frequencies from CPG to MSTP. > > drivers/clk/clk.c | 46 +++++++++++++++++++------------ > drivers/clk/shmobile/clk-mstp.c | 3 -- > drivers/clk/shmobile/clk-rcar-gen3.c | 13 +-------- > include/linux/clk-provider.h | 1 > 4 files changed, 35 insertions(+), 28 deletions(-) > > --- 0001/drivers/clk/clk.c > +++ work/drivers/clk/clk.c 2015-09-09 13:48:21.992366518 +0900 > @@ -3045,31 +3045,25 @@ int of_clk_get_parent_count(struct devic > } > EXPORT_SYMBOL_GPL(of_clk_get_parent_count); > > -const char *of_clk_get_parent_name(struct device_node *np, int index) > +const char *of_clk_get_name(struct device_node *np, int index) > { > - struct of_phandle_args clkspec; > struct property *prop; > const char *clk_name; > const __be32 *vp; > u32 pv; > - int rc; > int count; > + bool has_indices = false; > > if (index < 0) > return NULL; > > - rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index, > - &clkspec); > - if (rc) > - return NULL; > - > - index = clkspec.args_count ? clkspec.args[0] : 0; > count = 0; > > /* if there is an indices property, use it to transfer the index > * specified into an array offset for the clock-output-names property. > */ > - of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) { > + of_property_for_each_u32(np, "clock-indices", prop, vp, pv) { > + has_indices = true; > if (index == pv) { > index = count; > break; > @@ -3077,12 +3071,34 @@ const char *of_clk_get_parent_name(struc > count++; > } > > - if (of_property_read_string_index(clkspec.np, "clock-output-names", > - index, > - &clk_name) < 0) > - clk_name = clkspec.np->name; > + if (of_property_read_string_index(np, "clock-output-names", index, > + &clk_name) < 0) { > + if (has_indices) > + return kasprintf(GFP_KERNEL, "%s.%u", np->name, index); What if the clock provider has a #clock-cells larger than one ? Another issue is that this won't guarantee that the names are unique as multiple DT nodes can have the same name. Instead of trying to generate unique names, would it be possible to handle clock registration and lookup without relying on names for DT-based platforms ? > + else > + return kstrdup_const(np->name, GFP_KERNEL); > + } > > - of_node_put(clkspec.np); > + return kstrdup_const(clk_name, GFP_KERNEL); > +} > +EXPORT_SYMBOL_GPL(of_clk_get_name); > + > +const char *of_clk_get_parent_name(struct device_node *np, int index) > +{ > + struct of_phandle_args clkspec; > + const char *clk_name = NULL; > + int rc; > + > + if (index < 0) > + return NULL; > + > + rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index, > + &clkspec); > + if (!rc) { > + clk_name = of_clk_get_name(clkspec.np, clkspec.args_count ? > + clkspec.args[0] : 0); > + of_node_put(clkspec.np); > + } > return clk_name; > } > EXPORT_SYMBOL_GPL(of_clk_get_parent_name); > --- 0001/drivers/clk/shmobile/clk-mstp.c > +++ work/drivers/clk/shmobile/clk-mstp.c 2015-09-09 13:45:51.652366518 +0900 > @@ -216,8 +216,7 @@ static void __init cpg_mstp_clocks_init( > > if (of_property_read_string_index(np, "clock-output-names", > i, &name) < 0) > - allocated_name = name = kasprintf(GFP_KERNEL, "%s.%u", > - np->name, clkidx); > + allocated_name = name = of_clk_get_name(np, clkidx); > > clks[clkidx] = cpg_mstp_clock_register(name, parent_name, > clkidx, group); > --- 0001/drivers/clk/shmobile/clk-rcar-gen3.c > +++ work/drivers/clk/shmobile/clk-rcar-gen3.c 2015-09-09 13:45:51.652366518 > +0900 @@ -28,15 +28,6 @@ > #define RCAR_GEN3_CLK_PLL4 5 > #define RCAR_GEN3_CLK_NR 6 > > -static const char * const rcar_gen3_clk_names[RCAR_GEN3_CLK_NR] = { > - [RCAR_GEN3_CLK_MAIN] = "main", > - [RCAR_GEN3_CLK_PLL0] = "pll0", > - [RCAR_GEN3_CLK_PLL1] = "pll1", > - [RCAR_GEN3_CLK_PLL2] = "pll2", > - [RCAR_GEN3_CLK_PLL3] = "pll3", > - [RCAR_GEN3_CLK_PLL4] = "pll4", > -}; > - > struct rcar_gen3_cpg { > struct clk_onecell_data data; > void __iomem *reg; > @@ -116,7 +107,7 @@ rcar_gen3_cpg_register_clk(struct device > const struct cpg_pll_config *config, > unsigned int gen3_clk) > { > - const char *parent_name = rcar_gen3_clk_names[RCAR_GEN3_CLK_MAIN]; > + const char *parent_name = of_clk_get_name(np, RCAR_GEN3_CLK_MAIN); > unsigned int mult = 1; > unsigned int div = 1; > u32 value; > @@ -157,7 +148,7 @@ rcar_gen3_cpg_register_clk(struct device > return ERR_PTR(-EINVAL); > } > > - return clk_register_fixed_factor(NULL, rcar_gen3_clk_names[gen3_clk], > + return clk_register_fixed_factor(NULL, of_clk_get_name(np, gen3_clk), > parent_name, 0, mult, div); > } > > --- 0001/include/linux/clk-provider.h > +++ work/include/linux/clk-provider.h 2015-09-09 13:46:21.052366518 +0900 > @@ -665,6 +665,7 @@ struct clk *of_clk_src_onecell_get(struc > int of_clk_get_parent_count(struct device_node *np); > int of_clk_parent_fill(struct device_node *np, const char **parents, > unsigned int size); > +const char *of_clk_get_name(struct device_node *np, int index); > const char *of_clk_get_parent_name(struct device_node *np, int index); > > void of_clk_init(const struct of_device_id *matches);
HI Laurent, On Wed, Sep 9, 2015 at 2:14 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Magnus, > > Thank you for the patch. > > On Wednesday 09 September 2015 14:05:54 Magnus Damm wrote: >> From: Magnus Damm <damm+renesas@opensource.se> >> >> This patch hacks the CCF core to take clock-indices into >> consideration when making a default clock name in case >> clock-output-names is not provided. >> >> Without this patch of_clk_get_parent_name() does not work >> for clocks with multiple indices associated with one node. >> >> Proof of concept only. Leaks memory. Not for upstream merge. >> >> Not-Yet-Signed-off-by: Magnus Damm <damm+renesas@opensource.se> >> --- >> >> Written on top of "renesas-drivers-2015-09-08-v4.2" >> Needed to propagate clock frequencies from CPG to MSTP. >> >> drivers/clk/clk.c | 46 +++++++++++++++++++------------ >> drivers/clk/shmobile/clk-mstp.c | 3 -- >> drivers/clk/shmobile/clk-rcar-gen3.c | 13 +-------- >> include/linux/clk-provider.h | 1 >> 4 files changed, 35 insertions(+), 28 deletions(-) >> >> --- 0001/drivers/clk/clk.c >> +++ work/drivers/clk/clk.c 2015-09-09 13:48:21.992366518 +0900 >> @@ -3045,31 +3045,25 @@ int of_clk_get_parent_count(struct devic >> } >> EXPORT_SYMBOL_GPL(of_clk_get_parent_count); >> >> -const char *of_clk_get_parent_name(struct device_node *np, int index) >> +const char *of_clk_get_name(struct device_node *np, int index) >> { >> - struct of_phandle_args clkspec; >> struct property *prop; >> const char *clk_name; >> const __be32 *vp; >> u32 pv; >> - int rc; >> int count; >> + bool has_indices = false; >> >> if (index < 0) >> return NULL; >> >> - rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index, >> - &clkspec); >> - if (rc) >> - return NULL; >> - >> - index = clkspec.args_count ? clkspec.args[0] : 0; >> count = 0; >> >> /* if there is an indices property, use it to transfer the index >> * specified into an array offset for the clock-output-names property. >> */ >> - of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) { >> + of_property_for_each_u32(np, "clock-indices", prop, vp, pv) { >> + has_indices = true; >> if (index == pv) { >> index = count; >> break; >> @@ -3077,12 +3071,34 @@ const char *of_clk_get_parent_name(struc >> count++; >> } >> >> - if (of_property_read_string_index(clkspec.np, "clock-output-names", >> - index, >> - &clk_name) < 0) >> - clk_name = clkspec.np->name; >> + if (of_property_read_string_index(np, "clock-output-names", index, >> + &clk_name) < 0) { >> + if (has_indices) >> + return kasprintf(GFP_KERNEL, "%s.%u", np->name, index); > > What if the clock provider has a #clock-cells larger than one ? Right that is of course one issue. But according to the DT documentation file clock-bindings.txt #clock-cells is typically set to 0 or 1. > Another issue is that this won't guarantee that the names are unique as > multiple DT nodes can have the same name. Instead of trying to generate unique > names, would it be possible to handle clock registration and lookup without > relying on names for DT-based platforms ? It would of course make sense to do that for the long run, but at the same time that sounds like major internal API rework since most functions operate on string clock names today. So for short term is the correct approach to use clock-output-names? Cheers, / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Magnus, On Wednesday 09 September 2015 14:27:58 Magnus Damm wrote: > On Wed, Sep 9, 2015 at 2:14 PM, Laurent Pinchart wrote: > > On Wednesday 09 September 2015 14:05:54 Magnus Damm wrote: > >> From: Magnus Damm <damm+renesas@opensource.se> > >> > >> This patch hacks the CCF core to take clock-indices into > >> consideration when making a default clock name in case > >> clock-output-names is not provided. > >> > >> Without this patch of_clk_get_parent_name() does not work > >> for clocks with multiple indices associated with one node. > >> > >> Proof of concept only. Leaks memory. Not for upstream merge. > >> > >> Not-Yet-Signed-off-by: Magnus Damm <damm+renesas@opensource.se> > >> --- > >> > >> Written on top of "renesas-drivers-2015-09-08-v4.2" > >> Needed to propagate clock frequencies from CPG to MSTP. > >> > >> drivers/clk/clk.c | 46 +++++++++++++++++----------- > >> drivers/clk/shmobile/clk-mstp.c | 3 -- > >> drivers/clk/shmobile/clk-rcar-gen3.c | 13 +-------- > >> include/linux/clk-provider.h | 1 > >> 4 files changed, 35 insertions(+), 28 deletions(-) > >> > >> --- 0001/drivers/clk/clk.c > >> +++ work/drivers/clk/clk.c 2015-09-09 13:48:21.992366518 +0900 > >> @@ -3045,31 +3045,25 @@ int of_clk_get_parent_count(struct devic > >> } > >> EXPORT_SYMBOL_GPL(of_clk_get_parent_count); > >> > >> -const char *of_clk_get_parent_name(struct device_node *np, int index) > >> +const char *of_clk_get_name(struct device_node *np, int index) > >> { > >> - struct of_phandle_args clkspec; > >> struct property *prop; > >> const char *clk_name; > >> const __be32 *vp; > >> u32 pv; > >> - int rc; > >> int count; > >> + bool has_indices = false; > >> > >> if (index < 0) > >> return NULL; > >> > >> - rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", > >> index, > >> - &clkspec); > >> - if (rc) > >> - return NULL; > >> - > >> - index = clkspec.args_count ? clkspec.args[0] : 0; > >> count = 0; > >> > >> /* if there is an indices property, use it to transfer the index > >> * specified into an array offset for the clock-output-names > >> property. > >> */ > >> - of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) > >> { > >> + of_property_for_each_u32(np, "clock-indices", prop, vp, pv) { > >> + has_indices = true; > >> if (index == pv) { > >> index = count; > >> break; > >> @@ -3077,12 +3071,34 @@ const char *of_clk_get_parent_name(struc > >> count++; > >> } > >> > >> - if (of_property_read_string_index(clkspec.np, "clock-output-names", > >> - index, > >> - &clk_name) < 0) > >> - clk_name = clkspec.np->name; > >> + if (of_property_read_string_index(np, "clock-output-names", index, > >> + &clk_name) < 0) { > >> + if (has_indices) > >> + return kasprintf(GFP_KERNEL, "%s.%u", np->name, > >> index); > > > > What if the clock provider has a #clock-cells larger than one ? > > Right that is of course one issue. But according to the DT > documentation file clock-bindings.txt #clock-cells is typically set to > 0 or 1. "Typically" :-) We've discussed recently the possibility of using two cells for MSTP clocks. > > Another issue is that this won't guarantee that the names are unique as > > multiple DT nodes can have the same name. Instead of trying to generate > > unique names, would it be possible to handle clock registration and > > lookup without relying on names for DT-based platforms ? > > It would of course make sense to do that for the long run, but at the > same time that sounds like major internal API rework since most > functions operate on string clock names today. So for short term is > the correct approach to use clock-output-names? I think Stephen and Mike should comment on that.
Hi Magnus, On Wed, Sep 9, 2015 at 7:05 AM, Magnus Damm <magnus.damm@gmail.com> wrote: > From: Magnus Damm <damm+renesas@opensource.se> > > This patch hacks the CCF core to take clock-indices into > consideration when making a default clock name in case > clock-output-names is not provided. > > Without this patch of_clk_get_parent_name() does not work > for clocks with multiple indices associated with one node. > > Proof of concept only. Leaks memory. Not for upstream merge. > > Not-Yet-Signed-off-by: Magnus Damm <damm+renesas@opensource.se> While this (probably) solves the issue, is this something we want to do? What does we gain? E.g. we still need to allocate memory to store the clock names, so no memory is saved by not providing clock-output-names. But instead of useful names, the clocks now have autogenerated names, and all look the same (I don't know e.g. all 384 MSTP clock numbers by heart ;-) Clocks are too critical to make mistakes, so having useful names matters a lot. I'm still in favor of keeping clock-output-names for device nodes that provide multiple clock outputs. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/09, Laurent Pinchart wrote: > On Wednesday 09 September 2015 14:27:58 Magnus Damm wrote: > > On Wed, Sep 9, 2015 at 2:14 PM, Laurent Pinchart wrote: > > > Another issue is that this won't guarantee that the names are unique as > > > multiple DT nodes can have the same name. Instead of trying to generate > > > unique names, would it be possible to handle clock registration and > > > lookup without relying on names for DT-based platforms ? > > > > It would of course make sense to do that for the long run, but at the > > same time that sounds like major internal API rework since most > > functions operate on string clock names today. So for short term is > > the correct approach to use clock-output-names? > > I think Stephen and Mike should comment on that. > We've been murmuring about moving away from string based parent child relationship descriptions for some time now. Nothing very concrete has come out though and I haven't thought about it in too much detail. Why can't we call clk_get() for the clocks that we need to find the name of, and then call __clk_get_name() on them? Doing clk_get() has the nice side-effect of ordering probe for different clock controller drivers so that things like suspend/resume are done in the correct order.
Hi Magnus, On Wed, Sep 9, 2015 at 11:21 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Wed, Sep 9, 2015 at 7:05 AM, Magnus Damm <magnus.damm@gmail.com> wrote: >> From: Magnus Damm <damm+renesas@opensource.se> >> >> This patch hacks the CCF core to take clock-indices into >> consideration when making a default clock name in case >> clock-output-names is not provided. >> >> Without this patch of_clk_get_parent_name() does not work >> for clocks with multiple indices associated with one node. >> >> Proof of concept only. Leaks memory. Not for upstream merge. >> >> Not-Yet-Signed-off-by: Magnus Damm <damm+renesas@opensource.se> > > While this (probably) solves the issue, is this something we want to do? > What does we gain? E.g. we still need to allocate memory to store the clock > names, so no memory is saved by not providing clock-output-names. > But instead of useful names, the clocks now have autogenerated names, and all > look the same (I don't know e.g. all 384 MSTP clock numbers by heart ;-) Actually it's even worse: the autogenerated number is not the MSTP bit number, which I can relate to the actual clock using include/dt-bindings/clock/*.h, but the index in the sparse array of used bits, which depends on how many MSTP clocks are defined in DT. So we moved from e.g. "hscif1" through "mstp3.10" ("bit 10 in MSTP set 3", aka "MSTP310") to "mstp3.0" ("the first clock defined in DT for MSTP set 3"). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Stephen, On Wed, Sep 9, 2015 at 11:39 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 09/09, Laurent Pinchart wrote: >> On Wednesday 09 September 2015 14:27:58 Magnus Damm wrote: >> > On Wed, Sep 9, 2015 at 2:14 PM, Laurent Pinchart wrote: >> > > Another issue is that this won't guarantee that the names are unique as >> > > multiple DT nodes can have the same name. Instead of trying to generate >> > > unique names, would it be possible to handle clock registration and >> > > lookup without relying on names for DT-based platforms ? >> > >> > It would of course make sense to do that for the long run, but at the >> > same time that sounds like major internal API rework since most >> > functions operate on string clock names today. So for short term is >> > the correct approach to use clock-output-names? >> >> I think Stephen and Mike should comment on that. > > We've been murmuring about moving away from string based parent > child relationship descriptions for some time now. Nothing very > concrete has come out though and I haven't thought about it in > too much detail. If you want to get rid of the names, clocks could just be numbers, cfr. interrupts. Unfortunately debugging interrupt problems became more difficult with the advent of IRQ domains and the loss of fixed interrupt numbers. as I can no longer predict which is the right interrupt number for a specific device. > Why can't we call clk_get() for the clocks that we need to find > the name of, and then call __clk_get_name() on them? Doing Who defines the name of the clock? For e.g. clk-rcar-gen3.c that can be C code. For e.g. clk-mstp.c we used to take them from "clock-output-names" (old), or auto-generate them (new?). > clk_get() has the nice side-effect of ordering probe for > different clock controller drivers so that things like > suspend/resume are done in the correct order. That's true. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On Mon, Sep 14, 2015 at 9:02 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Magnus, > > On Wed, Sep 9, 2015 at 11:21 AM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: >> On Wed, Sep 9, 2015 at 7:05 AM, Magnus Damm <magnus.damm@gmail.com> wrote: >>> From: Magnus Damm <damm+renesas@opensource.se> >>> >>> This patch hacks the CCF core to take clock-indices into >>> consideration when making a default clock name in case >>> clock-output-names is not provided. >>> >>> Without this patch of_clk_get_parent_name() does not work >>> for clocks with multiple indices associated with one node. >>> >>> Proof of concept only. Leaks memory. Not for upstream merge. >>> >>> Not-Yet-Signed-off-by: Magnus Damm <damm+renesas@opensource.se> >> >> While this (probably) solves the issue, is this something we want to do? >> What does we gain? E.g. we still need to allocate memory to store the clock >> names, so no memory is saved by not providing clock-output-names. >> But instead of useful names, the clocks now have autogenerated names, and all >> look the same (I don't know e.g. all 384 MSTP clock numbers by heart ;-) > > Actually it's even worse: the autogenerated number is not the MSTP bit > number, which I can relate to the actual clock using > include/dt-bindings/clock/*.h, but the index in the sparse array of used bits, > which depends on how many MSTP clocks are defined in DT. > > So we moved from e.g. "hscif1" through "mstp3.10" ("bit 10 in MSTP set 3", > aka "MSTP310") to "mstp3.0" ("the first clock defined in DT for MSTP set 3"). Thanks for letting me know. It needs to be fixed for sure. Cheers, / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Stephen, Thanks for your feedback! On Thu, Sep 10, 2015 at 6:39 AM, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 09/09, Laurent Pinchart wrote: >> On Wednesday 09 September 2015 14:27:58 Magnus Damm wrote: >> > On Wed, Sep 9, 2015 at 2:14 PM, Laurent Pinchart wrote: >> > > Another issue is that this won't guarantee that the names are unique as >> > > multiple DT nodes can have the same name. Instead of trying to generate >> > > unique names, would it be possible to handle clock registration and >> > > lookup without relying on names for DT-based platforms ? >> > >> > It would of course make sense to do that for the long run, but at the >> > same time that sounds like major internal API rework since most >> > functions operate on string clock names today. So for short term is >> > the correct approach to use clock-output-names? >> >> I think Stephen and Mike should comment on that. >> > > We've been murmuring about moving away from string based parent > child relationship descriptions for some time now. Nothing very > concrete has come out though and I haven't thought about it in > too much detail. My half-cooked attempt to fix this was posted yesterday: [PATCH 00/05][RFC] Clock registration without parent name http://www.spinics.net/lists/linux-clk/msg02960.html If you have any ideas how that series can be beaten into something you like then please let me know. > Why can't we call clk_get() for the clocks that we need to find > the name of, and then call __clk_get_name() on them? Doing > clk_get() has the nice side-effect of ordering probe for > different clock controller drivers so that things like > suspend/resume are done in the correct order. I think __get_clk_name() makes sense in general as long as we can produce some sane names for the clocks while registering. In my mind the choices we have for naming clocks for registration are: a) Based on the DT clock-output-names property b) Hard coded string defined in C code c) Built on DT node name and DT clock-indices property (if present - and no clock-output-names in DT) Maybe there are other options? I don't mind so much in general which one to go with, but at the same time I can see the beauty of not making the DT clock-output-names property mandatory from a DT perspective. So I like the approach of defaulting to c) but allow DT to override using a). In my mind all of a) -> c) above should work with the core clock code. Please let me know if my understanding is wrong. Like mentioned in earlier email, the case c) above does not work when using of_clk_get_parent_name(). Especially the function of_fixed_factor_clk_setup() tries to point out the parent using of_clk_get_parent_name() which prevents it from working if the parent clock is named following the style in c). So we cannot use fixed factor clocks without local patches if we go with c).. As for clk_get() vs of_clk_get() (that my patch series above is using), this seems to be tightly related to the driver model and how to register clocks. clk_get() takes a struct device pointer while of_clk_get() takes a struct device_node pointer. I've always been a fan of using the standard driver model (initially with platform devices with platform data, now DT). Over the years various DT-specific init methods have creeped into the kernel - I clearly recall such ones for clocksources, irqchip and clocks, but gpio and pinctrl may also be affected. I've never really understood the reason behind them - shouldn't using the regular driver model with initcall order, linking order and deferred probing be enough? Anyway, if you have time please share your ideas about naming a) - >c) above and what your expectation is for new drivers. Same goes for driver model usage - do you want us to move away from of_clk_init() to regular driver model? of_clk_init() seems to manage probe order correctly what I can tell, not sure about suspend/resume though. From my side I mainly want to make our DT interface stable. So either we use clock-output-names in DT to begin with and fix things incrementally, or we omit clock-output-names from DT from the beginning but if so we need to fix the clock core code so we can use c) (or any other options). Thanks for your help, / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- 0001/drivers/clk/clk.c +++ work/drivers/clk/clk.c 2015-09-09 13:48:21.992366518 +0900 @@ -3045,31 +3045,25 @@ int of_clk_get_parent_count(struct devic } EXPORT_SYMBOL_GPL(of_clk_get_parent_count); -const char *of_clk_get_parent_name(struct device_node *np, int index) +const char *of_clk_get_name(struct device_node *np, int index) { - struct of_phandle_args clkspec; struct property *prop; const char *clk_name; const __be32 *vp; u32 pv; - int rc; int count; + bool has_indices = false; if (index < 0) return NULL; - rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index, - &clkspec); - if (rc) - return NULL; - - index = clkspec.args_count ? clkspec.args[0] : 0; count = 0; /* if there is an indices property, use it to transfer the index * specified into an array offset for the clock-output-names property. */ - of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) { + of_property_for_each_u32(np, "clock-indices", prop, vp, pv) { + has_indices = true; if (index == pv) { index = count; break; @@ -3077,12 +3071,34 @@ const char *of_clk_get_parent_name(struc count++; } - if (of_property_read_string_index(clkspec.np, "clock-output-names", - index, - &clk_name) < 0) - clk_name = clkspec.np->name; + if (of_property_read_string_index(np, "clock-output-names", index, + &clk_name) < 0) { + if (has_indices) + return kasprintf(GFP_KERNEL, "%s.%u", np->name, index); + else + return kstrdup_const(np->name, GFP_KERNEL); + } - of_node_put(clkspec.np); + return kstrdup_const(clk_name, GFP_KERNEL); +} +EXPORT_SYMBOL_GPL(of_clk_get_name); + +const char *of_clk_get_parent_name(struct device_node *np, int index) +{ + struct of_phandle_args clkspec; + const char *clk_name = NULL; + int rc; + + if (index < 0) + return NULL; + + rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index, + &clkspec); + if (!rc) { + clk_name = of_clk_get_name(clkspec.np, clkspec.args_count ? + clkspec.args[0] : 0); + of_node_put(clkspec.np); + } return clk_name; } EXPORT_SYMBOL_GPL(of_clk_get_parent_name); --- 0001/drivers/clk/shmobile/clk-mstp.c +++ work/drivers/clk/shmobile/clk-mstp.c 2015-09-09 13:45:51.652366518 +0900 @@ -216,8 +216,7 @@ static void __init cpg_mstp_clocks_init( if (of_property_read_string_index(np, "clock-output-names", i, &name) < 0) - allocated_name = name = kasprintf(GFP_KERNEL, "%s.%u", - np->name, clkidx); + allocated_name = name = of_clk_get_name(np, clkidx); clks[clkidx] = cpg_mstp_clock_register(name, parent_name, clkidx, group); --- 0001/drivers/clk/shmobile/clk-rcar-gen3.c +++ work/drivers/clk/shmobile/clk-rcar-gen3.c 2015-09-09 13:45:51.652366518 +0900 @@ -28,15 +28,6 @@ #define RCAR_GEN3_CLK_PLL4 5 #define RCAR_GEN3_CLK_NR 6 -static const char * const rcar_gen3_clk_names[RCAR_GEN3_CLK_NR] = { - [RCAR_GEN3_CLK_MAIN] = "main", - [RCAR_GEN3_CLK_PLL0] = "pll0", - [RCAR_GEN3_CLK_PLL1] = "pll1", - [RCAR_GEN3_CLK_PLL2] = "pll2", - [RCAR_GEN3_CLK_PLL3] = "pll3", - [RCAR_GEN3_CLK_PLL4] = "pll4", -}; - struct rcar_gen3_cpg { struct clk_onecell_data data; void __iomem *reg; @@ -116,7 +107,7 @@ rcar_gen3_cpg_register_clk(struct device const struct cpg_pll_config *config, unsigned int gen3_clk) { - const char *parent_name = rcar_gen3_clk_names[RCAR_GEN3_CLK_MAIN]; + const char *parent_name = of_clk_get_name(np, RCAR_GEN3_CLK_MAIN); unsigned int mult = 1; unsigned int div = 1; u32 value; @@ -157,7 +148,7 @@ rcar_gen3_cpg_register_clk(struct device return ERR_PTR(-EINVAL); } - return clk_register_fixed_factor(NULL, rcar_gen3_clk_names[gen3_clk], + return clk_register_fixed_factor(NULL, of_clk_get_name(np, gen3_clk), parent_name, 0, mult, div); } --- 0001/include/linux/clk-provider.h +++ work/include/linux/clk-provider.h 2015-09-09 13:46:21.052366518 +0900 @@ -665,6 +665,7 @@ struct clk *of_clk_src_onecell_get(struc int of_clk_get_parent_count(struct device_node *np); int of_clk_parent_fill(struct device_node *np, const char **parents, unsigned int size); +const char *of_clk_get_name(struct device_node *np, int index); const char *of_clk_get_parent_name(struct device_node *np, int index); void of_clk_init(const struct of_device_id *matches);
From: Magnus Damm <damm+renesas@opensource.se> This patch hacks the CCF core to take clock-indices into consideration when making a default clock name in case clock-output-names is not provided. Without this patch of_clk_get_parent_name() does not work for clocks with multiple indices associated with one node. Proof of concept only. Leaks memory. Not for upstream merge. Not-Yet-Signed-off-by: Magnus Damm <damm+renesas@opensource.se> --- Written on top of "renesas-drivers-2015-09-08-v4.2" Needed to propagate clock frequencies from CPG to MSTP. drivers/clk/clk.c | 46 ++++++++++++++++++++++------------ drivers/clk/shmobile/clk-mstp.c | 3 -- drivers/clk/shmobile/clk-rcar-gen3.c | 13 +-------- include/linux/clk-provider.h | 1 4 files changed, 35 insertions(+), 28 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html