| Message ID | 1382716658-6964-7-git-send-email-t-kristo@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/25/2013 10:57 AM, Tero Kristo wrote: > ti_dt_clk_init_provider() can now be used to initialize the contents of > a single clock IP block. This parses all the clocks under the IP block > and calls the corresponding init function for them. > > Signed-off-by: Tero Kristo <t-kristo@ti.com> > --- > drivers/clk/ti/clk.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/clk/ti.h | 1 + > 2 files changed, 60 insertions(+) > > diff --git a/drivers/clk/ti/clk.c b/drivers/clk/ti/clk.c > index ad58b01..7f030d7 100644 > --- a/drivers/clk/ti/clk.c > +++ b/drivers/clk/ti/clk.c > @@ -19,6 +19,9 @@ > #include <linux/clkdev.h> > #include <linux/clk/ti.h> > #include <linux/of.h> > +#include <linux/list.h> > + > +extern struct of_device_id __clk_of_table[]; > > /** > * ti_dt_clocks_register - register DT duplicate clocks during boot > @@ -50,3 +53,59 @@ void __init ti_dt_clocks_register(struct ti_dt_clk oclks[]) > } > } > } > + > +typedef int (*ti_of_clk_init_cb_t)(struct device_node *, struct regmap *); > + > +struct clk_init_item { > + struct regmap *regmap; > + struct device_node *np; > + ti_of_clk_init_cb_t init_cb; > + struct list_head node; > +}; > + > +static LIST_HEAD(retry_list); > + > +void __init ti_dt_clk_init_provider(struct device_node *parent, > + struct regmap *regmap) > +{ > + const struct of_device_id *match; > + struct device_node *np; > + ti_of_clk_init_cb_t clk_init_cb; > + struct clk_init_item *retry; > + struct clk_init_item *tmp; > + int ret; > + > + for_each_child_of_node(parent, np) { > + match = of_match_node(__clk_of_table, np); > + if (!match) > + continue; > + clk_init_cb = match->data; I must admit I am confused here. a) of_clk_init (in the generic clk.c) uses of_clk_init_cb_t as match data. The prototype of the generic of_clk_init_cb_t is typedef void (*of_clk_init_cb_t)(struct device_node *); b) both of_clk_init and ti_dt_clk_init_provider looks up clock nodes from __clk_of_table I assume we need ti_dt_clk_init_provider to be always called with clock list, as a result of_clk_init(NULL); will never be invoked. This is counter productive as you have have generic non SoC clock providers as well who would have been invoked with of_clk_init(NULL); I do strongly encourage using of_clk_init(NULL) and not having to switch the clk_init call back with SoC specific one as it forces un-intended dependency. Further such as SoC specific callback should have been in a header. > + pr_debug("%s: initializing: %s\n", __func__, np->name); one further comment - using pr_fmt will save us from using __func__ for every pr_* message :( > + ret = clk_init_cb(np, regmap); > + if (ret == -EAGAIN) { > + pr_debug("%s: adding to again list...\n", np->name); > + retry = kzalloc(sizeof(*retry), GFP_KERNEL); > + retry->np = np; > + retry->regmap = regmap; > + retry->init_cb = clk_init_cb; > + list_add(&retry->node, &retry_list); > + } else if (ret) { > + pr_err("%s: clock init failed for %s (%d)!\n", __func__, > + np->name, ret); > + } > + } > + > + list_for_each_entry_safe(retry, tmp, &retry_list, node) { > + pr_debug("%s: retry-init: %s\n", __func__, retry->np->name); > + ret = retry->init_cb(retry->np, retry->regmap); > + if (ret == -EAGAIN) { > + pr_debug("%s failed again?\n", retry->np->name); > + } else { > + if (ret) > + pr_err("%s: clock init failed for %s (%d)!\n", > + __func__, retry->np->name, ret); > + list_del(&retry->node); > + kfree(retry); > + } > + } > +} > diff --git a/include/linux/clk/ti.h b/include/linux/clk/ti.h > index 9786752..7ab02fa 100644 > --- a/include/linux/clk/ti.h > +++ b/include/linux/clk/ti.h > @@ -177,6 +177,7 @@ int omap3_dpll4_set_rate(struct clk_hw *clk, unsigned long rate, > unsigned long parent_rate); > > void ti_dt_clocks_register(struct ti_dt_clk *oclks); > +void ti_dt_clk_init_provider(struct device_node *np, struct regmap *regmap); > > extern const struct clk_hw_omap_ops clkhwops_omap3_dpll; > extern const struct clk_hw_omap_ops clkhwops_omap4_dpllmx; >
On 10/31/2013 05:42 PM, Nishanth Menon wrote: > On 10/25/2013 10:57 AM, Tero Kristo wrote: >> ti_dt_clk_init_provider() can now be used to initialize the contents of >> a single clock IP block. This parses all the clocks under the IP block >> and calls the corresponding init function for them. >> >> Signed-off-by: Tero Kristo <t-kristo@ti.com> >> --- >> drivers/clk/ti/clk.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++ >> include/linux/clk/ti.h | 1 + >> 2 files changed, 60 insertions(+) >> >> diff --git a/drivers/clk/ti/clk.c b/drivers/clk/ti/clk.c >> index ad58b01..7f030d7 100644 >> --- a/drivers/clk/ti/clk.c >> +++ b/drivers/clk/ti/clk.c >> @@ -19,6 +19,9 @@ >> #include <linux/clkdev.h> >> #include <linux/clk/ti.h> >> #include <linux/of.h> >> +#include <linux/list.h> >> + >> +extern struct of_device_id __clk_of_table[]; >> >> /** >> * ti_dt_clocks_register - register DT duplicate clocks during boot >> @@ -50,3 +53,59 @@ void __init ti_dt_clocks_register(struct ti_dt_clk oclks[]) >> } >> } >> } >> + >> +typedef int (*ti_of_clk_init_cb_t)(struct device_node *, struct regmap *); >> + >> +struct clk_init_item { >> + struct regmap *regmap; >> + struct device_node *np; >> + ti_of_clk_init_cb_t init_cb; >> + struct list_head node; >> +}; >> + >> +static LIST_HEAD(retry_list); >> + >> +void __init ti_dt_clk_init_provider(struct device_node *parent, >> + struct regmap *regmap) >> +{ >> + const struct of_device_id *match; >> + struct device_node *np; >> + ti_of_clk_init_cb_t clk_init_cb; >> + struct clk_init_item *retry; >> + struct clk_init_item *tmp; >> + int ret; >> + >> + for_each_child_of_node(parent, np) { >> + match = of_match_node(__clk_of_table, np); >> + if (!match) >> + continue; >> + clk_init_cb = match->data; > > I must admit I am confused here. Yea this patch is something I am not quite comfortable myself yet and would like improvement ideas.... > a) of_clk_init (in the generic clk.c) uses of_clk_init_cb_t as match > data. The prototype of the generic of_clk_init_cb_t is typedef void > (*of_clk_init_cb_t)(struct device_node *); > b) both of_clk_init and ti_dt_clk_init_provider looks up clock nodes > from __clk_of_table __clk_of_table contains the function pointers for the clock init functions, not clock nodes. > > I assume we need ti_dt_clk_init_provider to be always called with > clock list, as a result of_clk_init(NULL); will never be invoked. This > is counter productive as you have have generic non SoC clock providers > as well who would have been invoked with of_clk_init(NULL); Can't call of_clk_init(NULL) anymore, as Paul wants to map the clock nodes under respective IP blocks. Two reasons here: a) This requires me to pass some info from the IP block (CM1/CM2/PRM) to the clock init functions, basically the pointer to the memory map region (regmap.) b) of_clk_init(NULL) will initialize all the clock nodes in the system, irrespective of the hierarchy considerations. Only thing that can be done, is to make the API introduced in this patch a generic API and call it something like of_clk_init_children(). > > I do strongly encourage using of_clk_init(NULL) and not having to > switch the clk_init call back with SoC specific one as it forces > un-intended dependency. Can't do this as mentioned above. > > > Further such as SoC specific callback should have been in a header. >> + pr_debug("%s: initializing: %s\n", __func__, np->name); > > > one further comment - using pr_fmt will save us from using __func__ > for every pr_* message :( Hmm good comment, will fix that for next rev. > >> + ret = clk_init_cb(np, regmap); >> + if (ret == -EAGAIN) { >> + pr_debug("%s: adding to again list...\n", np->name); >> + retry = kzalloc(sizeof(*retry), GFP_KERNEL); >> + retry->np = np; >> + retry->regmap = regmap; >> + retry->init_cb = clk_init_cb; >> + list_add(&retry->node, &retry_list); >> + } else if (ret) { >> + pr_err("%s: clock init failed for %s (%d)!\n", __func__, >> + np->name, ret); >> + } >> + } >> + >> + list_for_each_entry_safe(retry, tmp, &retry_list, node) { >> + pr_debug("%s: retry-init: %s\n", __func__, retry->np->name); >> + ret = retry->init_cb(retry->np, retry->regmap); >> + if (ret == -EAGAIN) { >> + pr_debug("%s failed again?\n", retry->np->name); >> + } else { >> + if (ret) >> + pr_err("%s: clock init failed for %s (%d)!\n", >> + __func__, retry->np->name, ret); >> + list_del(&retry->node); >> + kfree(retry); >> + } >> + } >> +} >> diff --git a/include/linux/clk/ti.h b/include/linux/clk/ti.h >> index 9786752..7ab02fa 100644 >> --- a/include/linux/clk/ti.h >> +++ b/include/linux/clk/ti.h >> @@ -177,6 +177,7 @@ int omap3_dpll4_set_rate(struct clk_hw *clk, unsigned long rate, >> unsigned long parent_rate); >> >> void ti_dt_clocks_register(struct ti_dt_clk *oclks); >> +void ti_dt_clk_init_provider(struct device_node *np, struct regmap *regmap); >> >> extern const struct clk_hw_omap_ops clkhwops_omap3_dpll; >> extern const struct clk_hw_omap_ops clkhwops_omap4_dpllmx; >> > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/01/2013 04:12 AM, Tero Kristo wrote: > On 10/31/2013 05:42 PM, Nishanth Menon wrote: >> On 10/25/2013 10:57 AM, Tero Kristo wrote: >>> ti_dt_clk_init_provider() can now be used to initialize the contents of >>> a single clock IP block. This parses all the clocks under the IP block >>> and calls the corresponding init function for them. >>> >>> Signed-off-by: Tero Kristo <t-kristo@ti.com> >>> --- >>> drivers/clk/ti/clk.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++ >>> include/linux/clk/ti.h | 1 + >>> 2 files changed, 60 insertions(+) >>> >>> diff --git a/drivers/clk/ti/clk.c b/drivers/clk/ti/clk.c >>> index ad58b01..7f030d7 100644 >>> --- a/drivers/clk/ti/clk.c >>> +++ b/drivers/clk/ti/clk.c >>> @@ -19,6 +19,9 @@ >>> #include <linux/clkdev.h> >>> #include <linux/clk/ti.h> >>> #include <linux/of.h> >>> +#include <linux/list.h> >>> + >>> +extern struct of_device_id __clk_of_table[]; >>> >>> /** >>> * ti_dt_clocks_register - register DT duplicate clocks during boot >>> @@ -50,3 +53,59 @@ void __init ti_dt_clocks_register(struct ti_dt_clk oclks[]) >>> } >>> } >>> } >>> + >>> +typedef int (*ti_of_clk_init_cb_t)(struct device_node *, struct regmap *); >>> + >>> +struct clk_init_item { >>> + struct regmap *regmap; >>> + struct device_node *np; >>> + ti_of_clk_init_cb_t init_cb; >>> + struct list_head node; >>> +}; >>> + >>> +static LIST_HEAD(retry_list); >>> + >>> +void __init ti_dt_clk_init_provider(struct device_node *parent, >>> + struct regmap *regmap) >>> +{ >>> + const struct of_device_id *match; >>> + struct device_node *np; >>> + ti_of_clk_init_cb_t clk_init_cb; >>> + struct clk_init_item *retry; >>> + struct clk_init_item *tmp; >>> + int ret; >>> + >>> + for_each_child_of_node(parent, np) { >>> + match = of_match_node(__clk_of_table, np); >>> + if (!match) >>> + continue; >>> + clk_init_cb = match->data; >> >> I must admit I am confused here. > > Yea this patch is something I am not quite comfortable myself yet and > would like improvement ideas.... > >> a) of_clk_init (in the generic clk.c) uses of_clk_init_cb_t as match >> data. The prototype of the generic of_clk_init_cb_t is typedef void >> (*of_clk_init_cb_t)(struct device_node *); >> b) both of_clk_init and ti_dt_clk_init_provider looks up clock nodes >> from __clk_of_table > > __clk_of_table contains the function pointers for the clock init > functions, not clock nodes. yes, apologies, should have stated the prototypes of functions in a single __clk_of_table should not be two different argument list. The reason is as follows: CLK_OF_DECLARE fills up that list. there can be generic drivers such as [1] which might be registered, OR in the case of MULTI_ARCH - multiple other SoC drivers would have registered there. There is a bunch of stuff we could mess up here. If we do not have a choice, if we would like to maintain a different prototype based init list, we should probably do it in a different structure - example __clk_of_table_regmap or something similar.. > >> >> I assume we need ti_dt_clk_init_provider to be always called with >> clock list, as a result of_clk_init(NULL); will never be invoked. This >> is counter productive as you have have generic non SoC clock providers >> as well who would have been invoked with of_clk_init(NULL); > > Can't call of_clk_init(NULL) anymore, as Paul wants to map the clock > nodes under respective IP blocks. Two reasons here: > > a) This requires me to pass some info from the IP block (CM1/CM2/PRM) to > the clock init functions, basically the pointer to the memory map region > (regmap.) > b) of_clk_init(NULL) will initialize all the clock nodes in the system, > irrespective of the hierarchy considerations. > > Only thing that can be done, is to make the API introduced in this patch > a generic API and call it something like of_clk_init_children(). you still can do that: of_prcm_init->ti_dt_clk_init_provider-> instead of matching nodes from __clk_of_table, you can make it __clk_of_regmap_table -> the CLK_OF_DECLARE will not be good enough here ofcourse, but this will probably belong to a generic regmap enabled clock support framework - i dont see much that is TI specific in drivers/clk/ti/clk.c and it makes sense to be part of generic clock framework handling as generic clock framework will receive regmap support based on previous patches. that allows for other platforms to use regmap based clk drivers as well. [1] http://marc.info/?l=linux-omap&m=138331451210184&w=2
On 11/01/2013 09:13 PM, Nishanth Menon wrote: > On 11/01/2013 04:12 AM, Tero Kristo wrote: >> On 10/31/2013 05:42 PM, Nishanth Menon wrote: >>> On 10/25/2013 10:57 AM, Tero Kristo wrote: >>>> ti_dt_clk_init_provider() can now be used to initialize the contents of >>>> a single clock IP block. This parses all the clocks under the IP block >>>> and calls the corresponding init function for them. >>>> >>>> Signed-off-by: Tero Kristo <t-kristo@ti.com> >>>> --- >>>> drivers/clk/ti/clk.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++ >>>> include/linux/clk/ti.h | 1 + >>>> 2 files changed, 60 insertions(+) >>>> >>>> diff --git a/drivers/clk/ti/clk.c b/drivers/clk/ti/clk.c >>>> index ad58b01..7f030d7 100644 >>>> --- a/drivers/clk/ti/clk.c >>>> +++ b/drivers/clk/ti/clk.c >>>> @@ -19,6 +19,9 @@ >>>> #include <linux/clkdev.h> >>>> #include <linux/clk/ti.h> >>>> #include <linux/of.h> >>>> +#include <linux/list.h> >>>> + >>>> +extern struct of_device_id __clk_of_table[]; >>>> >>>> /** >>>> * ti_dt_clocks_register - register DT duplicate clocks during boot >>>> @@ -50,3 +53,59 @@ void __init ti_dt_clocks_register(struct ti_dt_clk oclks[]) >>>> } >>>> } >>>> } >>>> + >>>> +typedef int (*ti_of_clk_init_cb_t)(struct device_node *, struct regmap *); >>>> + >>>> +struct clk_init_item { >>>> + struct regmap *regmap; >>>> + struct device_node *np; >>>> + ti_of_clk_init_cb_t init_cb; >>>> + struct list_head node; >>>> +}; >>>> + >>>> +static LIST_HEAD(retry_list); >>>> + >>>> +void __init ti_dt_clk_init_provider(struct device_node *parent, >>>> + struct regmap *regmap) >>>> +{ >>>> + const struct of_device_id *match; >>>> + struct device_node *np; >>>> + ti_of_clk_init_cb_t clk_init_cb; >>>> + struct clk_init_item *retry; >>>> + struct clk_init_item *tmp; >>>> + int ret; >>>> + >>>> + for_each_child_of_node(parent, np) { >>>> + match = of_match_node(__clk_of_table, np); >>>> + if (!match) >>>> + continue; >>>> + clk_init_cb = match->data; >>> >>> I must admit I am confused here. >> >> Yea this patch is something I am not quite comfortable myself yet and >> would like improvement ideas.... >> >>> a) of_clk_init (in the generic clk.c) uses of_clk_init_cb_t as match >>> data. The prototype of the generic of_clk_init_cb_t is typedef void >>> (*of_clk_init_cb_t)(struct device_node *); >>> b) both of_clk_init and ti_dt_clk_init_provider looks up clock nodes >>> from __clk_of_table >> >> __clk_of_table contains the function pointers for the clock init >> functions, not clock nodes. > > yes, apologies, should have stated the prototypes of functions in a > single __clk_of_table should not be two different argument list. The > reason is as follows: CLK_OF_DECLARE fills up that list. there can be > generic drivers such as [1] which might be registered, OR in the case > of MULTI_ARCH - multiple other SoC drivers would have registered > there. There is a bunch of stuff we could mess up here. > > If we do not have a choice, if we would like to maintain a different > prototype based init list, we should probably do it in a different > structure - example __clk_of_table_regmap or something similar.. > > >> >>> >>> I assume we need ti_dt_clk_init_provider to be always called with >>> clock list, as a result of_clk_init(NULL); will never be invoked. This >>> is counter productive as you have have generic non SoC clock providers >>> as well who would have been invoked with of_clk_init(NULL); >> >> Can't call of_clk_init(NULL) anymore, as Paul wants to map the clock >> nodes under respective IP blocks. Two reasons here: >> >> a) This requires me to pass some info from the IP block (CM1/CM2/PRM) to >> the clock init functions, basically the pointer to the memory map region >> (regmap.) >> b) of_clk_init(NULL) will initialize all the clock nodes in the system, >> irrespective of the hierarchy considerations. >> >> Only thing that can be done, is to make the API introduced in this patch >> a generic API and call it something like of_clk_init_children(). > > you still can do that: > > of_prcm_init->ti_dt_clk_init_provider-> instead of matching nodes from > __clk_of_table, you can make it __clk_of_regmap_table -> the > CLK_OF_DECLARE will not be good enough here ofcourse, but this will > probably belong to a generic regmap enabled clock support framework - > i dont see much that is TI specific in drivers/clk/ti/clk.c and it > makes sense to be part of generic clock framework handling as generic > clock framework will receive regmap support based on previous patches. > that allows for other platforms to use regmap based clk drivers as well. Yea different init tables can be introduced, however I still need to split the support between hierarchical version and the blunt version that registers everything. Looking at the work from Jyri, its possible I need to add calls to both. -Tero > > > [1] http://marc.info/?l=linux-omap&m=138331451210184&w=2 > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/clk/ti/clk.c b/drivers/clk/ti/clk.c index ad58b01..7f030d7 100644 --- a/drivers/clk/ti/clk.c +++ b/drivers/clk/ti/clk.c @@ -19,6 +19,9 @@ #include <linux/clkdev.h> #include <linux/clk/ti.h> #include <linux/of.h> +#include <linux/list.h> + +extern struct of_device_id __clk_of_table[]; /** * ti_dt_clocks_register - register DT duplicate clocks during boot @@ -50,3 +53,59 @@ void __init ti_dt_clocks_register(struct ti_dt_clk oclks[]) } } } + +typedef int (*ti_of_clk_init_cb_t)(struct device_node *, struct regmap *); + +struct clk_init_item { + struct regmap *regmap; + struct device_node *np; + ti_of_clk_init_cb_t init_cb; + struct list_head node; +}; + +static LIST_HEAD(retry_list); + +void __init ti_dt_clk_init_provider(struct device_node *parent, + struct regmap *regmap) +{ + const struct of_device_id *match; + struct device_node *np; + ti_of_clk_init_cb_t clk_init_cb; + struct clk_init_item *retry; + struct clk_init_item *tmp; + int ret; + + for_each_child_of_node(parent, np) { + match = of_match_node(__clk_of_table, np); + if (!match) + continue; + clk_init_cb = match->data; + pr_debug("%s: initializing: %s\n", __func__, np->name); + ret = clk_init_cb(np, regmap); + if (ret == -EAGAIN) { + pr_debug("%s: adding to again list...\n", np->name); + retry = kzalloc(sizeof(*retry), GFP_KERNEL); + retry->np = np; + retry->regmap = regmap; + retry->init_cb = clk_init_cb; + list_add(&retry->node, &retry_list); + } else if (ret) { + pr_err("%s: clock init failed for %s (%d)!\n", __func__, + np->name, ret); + } + } + + list_for_each_entry_safe(retry, tmp, &retry_list, node) { + pr_debug("%s: retry-init: %s\n", __func__, retry->np->name); + ret = retry->init_cb(retry->np, retry->regmap); + if (ret == -EAGAIN) { + pr_debug("%s failed again?\n", retry->np->name); + } else { + if (ret) + pr_err("%s: clock init failed for %s (%d)!\n", + __func__, retry->np->name, ret); + list_del(&retry->node); + kfree(retry); + } + } +} diff --git a/include/linux/clk/ti.h b/include/linux/clk/ti.h index 9786752..7ab02fa 100644 --- a/include/linux/clk/ti.h +++ b/include/linux/clk/ti.h @@ -177,6 +177,7 @@ int omap3_dpll4_set_rate(struct clk_hw *clk, unsigned long rate, unsigned long parent_rate); void ti_dt_clocks_register(struct ti_dt_clk *oclks); +void ti_dt_clk_init_provider(struct device_node *np, struct regmap *regmap); extern const struct clk_hw_omap_ops clkhwops_omap3_dpll; extern const struct clk_hw_omap_ops clkhwops_omap4_dpllmx;
ti_dt_clk_init_provider() can now be used to initialize the contents of a single clock IP block. This parses all the clocks under the IP block and calls the corresponding init function for them. Signed-off-by: Tero Kristo <t-kristo@ti.com> --- drivers/clk/ti/clk.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/clk/ti.h | 1 + 2 files changed, 60 insertions(+)