| Message ID | 1465381201-11537-2-git-send-email-ricardo.ribalda@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Stephen Boyd |
| Headers | show |
On 06/08, Ricardo Ribalda Delgado wrote: > of_clk_is_provider() checks if a device_node has already been added to > the clk provider list. This can be used to avoid adding the same clock > provider twice. > > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> While I don't disagree with the concept, I'd like to do this outside of the clk framework checking for nodes, because the problem doesn't seem clk specific. From digging in the OF platform layer I see of_node_test_and_set_flag(OF_POPULATED) may be what we should be using. It looks like this can be used to make sure that any clk provider nodes aren't populated as platform devices when we've initialized them early. The only problem now is that we have drivers using a hybrid approach with of_clk_init(). Sometimes drivers need to get clks up early for timers, so they have CLK_OF_DECLARE() in their driver, but then they also use a platform driver to handle the non-timer related clks. If we mark all nodes as populated in of_clk_init() we'll preclude these drivers from working. The solution there is to make those drivers specifically clear the populated flag in the clk init callback. Or we can automatically do that with some new CLK_OF_DECLARE_EARLY() macro that hides this clearing from them. Either way, the drivers will need to indicate they're using this hybrid style so that we still populate platform devices.
Hi Stephen When the device tree is populated or when an overlay is added, all its nodes have the flag OF_POPULATED set. The flag is enabled recursively in of_platform_bus_create->of_platform_device_create_pdata() So we cannot use that flag to mark what is enabled and what is not. The other issue that I see is of_clk_mutex. Whatever final implementation that we decide to do, it should take into consideration that mutex, otherwise it will not be thread-safe. of_clk_is_provider() is already taking care of it. Another advantage of of_clk_is_provider() is that it opens the door to implement something like: CLK_OF_DECLARE_EARLY_PLATFORM(probe,remove) That allows a driver to implement early clk and platform clk at the same time and automatically, following a logic similar to what I have done in fixed-clk. I will send a v2 with the changes you proposed to fixed-clk Thanks and best regards! On Thu, Jun 16, 2016 at 2:44 AM, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 06/08, Ricardo Ribalda Delgado wrote: >> of_clk_is_provider() checks if a device_node has already been added to >> the clk provider list. This can be used to avoid adding the same clock >> provider twice. >> >> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> > > While I don't disagree with the concept, I'd like to do this > outside of the clk framework checking for nodes, because the > problem doesn't seem clk specific. From digging in the OF > platform layer I see of_node_test_and_set_flag(OF_POPULATED) may > be what we should be using. It looks like this can be used to > make sure that any clk provider nodes aren't populated as > platform devices when we've initialized them early. > > The only problem now is that we have drivers using a hybrid > approach with of_clk_init(). Sometimes drivers need to get clks > up early for timers, so they have CLK_OF_DECLARE() in their > driver, but then they also use a platform driver to handle the > non-timer related clks. If we mark all nodes as populated in > of_clk_init() we'll preclude these drivers from working. The > solution there is to make those drivers specifically clear the > populated flag in the clk init callback. Or we can automatically > do that with some new CLK_OF_DECLARE_EARLY() macro that hides > this clearing from them. Either way, the drivers will need to > indicate they're using this hybrid style so that we still > populate platform devices. > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project
(Please don't top post) On 06/20, Ricardo Ribalda Delgado wrote: > Hi Stephen > > When the device tree is populated or when an overlay is added, all its > nodes have the flag OF_POPULATED set. The flag is enabled recursively > in > of_platform_bus_create->of_platform_device_create_pdata() > So we cannot use that flag to mark what is enabled and what is not. Sorry I don't follow the reasoning here. I'm not asking to test that flag in an of_clk_is_provider() API. The goal is to not have an of_clk_is_provider() API. I was thinking that of_clk_init() would mark any nodes that matched and provided clk providers as OF_POPULATED. That way, when of_platform_populate() ran, it would *not* add platform devices for clk providers that we registered during the of_clk_init() phase. Then we could have platform drivers and CLK_OF_DECLARE drivers for the same compatible strings, but we wouldn't probe random platform drivers for the nodes that we handled early on and we wouldn't need to litter of_clk_is_provider() in driver probe routines. > > The other issue that I see is of_clk_mutex. Whatever final > implementation that we decide to do, it should take into consideration > that mutex, otherwise it will not be thread-safe. > of_clk_is_provider() is already taking care of it. > > Another advantage of of_clk_is_provider() is that it opens the door to > implement something like: CLK_OF_DECLARE_EARLY_PLATFORM(probe,remove) > That allows a driver to implement early clk and platform clk at the > same time and automatically, following a logic similar to what I have > done in fixed-clk. Something like CLK_OF_DECLARE_EARLY_PLATFORM() sounds like it may be good (I haven't looked at the patch). We'll need something to say that there's CLK_OF_DECLARE and a platform driver for the same node.
Hi Stephen On Tue, Jun 21, 2016 at 3:30 AM, Stephen Boyd <sboyd@codeaurora.org> wrote: > (Please don't top post) Sorry about that > > I was thinking that of_clk_init() would mark any nodes that > matched and provided clk providers as OF_POPULATED. That way, > when of_platform_populate() ran, it would *not* add platform > devices for clk providers that we registered during the > of_clk_init() phase. Then we could have platform drivers and > CLK_OF_DECLARE drivers for the same compatible strings, but we > wouldn't probe random platform drivers for the nodes that we > handled early on and we wouldn't need to litter > of_clk_is_provider() in driver probe routines. > Ok, now I get it. I didn't realised that you wanted to set the flag. I have prepared two new patches. now setup does something like: void __init of_fixed_factor_clk_setup(struct device_node *node) { if (!_of_fixed_factor_clk_setup(node)) of_node_set_flag(node, OF_POPULATED); } If we probe this method to be valid, on a future stage I can make a MACRO like: CLK_OF_DECLARE_PLATFORM(fixed_factor_clk, "fixed-factor-clock", _of_fixed_factor_clk_setup, clk_unregister_fixed_factor); That sets the flag, instantiate MODULE_DEVICE_TABLE, builtin_platform_driver, and does the platform_set_drvdata..... Driver developers can choose to use CLK_OF_DECLARE_PLATFORM or CLK_OF_DECLARE Thanks!
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index d584004f7af7..2423c6373906 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -3120,6 +3120,26 @@ __of_clk_get_hw_from_provider(struct of_clk_provider *provider, return hw; } +/** + * of_clk_is_provider() - Reports if a device node is already a clk provider + * @np: Device node pointer under test + */ +bool of_clk_is_provider(struct device_node *np) +{ + struct of_clk_provider *cp; + + mutex_lock(&of_clk_mutex); + list_for_each_entry(cp, &of_clk_providers, link) { + if (cp->node == np) { + mutex_unlock(&of_clk_mutex); + return true; + } + } + mutex_unlock(&of_clk_mutex); + return false; +} +EXPORT_SYMBOL_GPL(of_clk_is_provider); + struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec, const char *dev_id, const char *con_id) { diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index fb39d5add173..a01b18797418 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -787,6 +787,7 @@ int of_clk_add_hw_provider(struct device_node *np, void *data), void *data); void of_clk_del_provider(struct device_node *np); +bool of_clk_is_provider(struct device_node *np); struct clk *of_clk_src_simple_get(struct of_phandle_args *clkspec, void *data); struct clk_hw *of_clk_hw_simple_get(struct of_phandle_args *clkspec, @@ -819,6 +820,10 @@ static inline int of_clk_add_hw_provider(struct device_node *np, return 0; } static inline void of_clk_del_provider(struct device_node *np) {} +static inline bool of_clk_is_provider(struct device_node *np) +{ + return false; +} static inline struct clk *of_clk_src_simple_get( struct of_phandle_args *clkspec, void *data) {
of_clk_is_provider() checks if a device_node has already been added to the clk provider list. This can be used to avoid adding the same clock provider twice. Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> --- drivers/clk/clk.c | 20 ++++++++++++++++++++ include/linux/clk-provider.h | 5 +++++ 2 files changed, 25 insertions(+)