| Message ID | 1453385958-11366-1-git-send-email-emilio.lopez@collabora.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/21, Emilio López wrote: > @@ -3059,7 +3069,25 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec, > */ > struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec) > { > - return __of_clk_get_from_provider(clkspec, NULL, __func__); > + return __of_clk_get_from_provider(clkspec, NULL, __func__, false); > +} > + > +/** > + * of_clk_get_from_provider_with_orphans() - Lookup clock from a clock provider > + * @clkspec: pointer to a clock specifier data structure > + * > + * This function looks up a struct clk from the registered list of clock > + * providers, an input is a clock specifier data structure as returned > + * from the of_parse_phandle_with_args() function call. > + * > + * The difference to of_clk_get_from_provider() is that this function will > + * also successfully lookup orphan-clocks, as it in some cases may be > + * necessary to access such orphan-clocks as well. > + */ > +struct clk * > +of_clk_get_from_provider_with_orphans(struct of_phandle_args *clkspec) Dislike. In fact, the whole clk conf approach is odd here. When we're doing of_clk_init() we do a best effort loop around parent_ready(), waiting for clk providers to register as long as we have a clocks property in our provider node. We should do something similar in the non of_clk_init() case too, because of_clk_init() isn't special. Furthermore, the assigned parents and rates feature doesn't need the clocks that we're assigning parents and rates to to even be provided or consumed by the provider that's probing, so I'm lost why we're checking the provider's node for a clocks property. It would be better to check the assigned-clocks and assigned-parents properties and make sure that those are all non-orphans. If they're orphaned, we should delay until another clk provider is registered. Eventually we'll unstick the orphans and then the tree can be configured. Running the configuration at the end of of_clk_init() even if we still can't get the clocks doesn't make any sense to me. To be really nice, we could build up a set of configuration actions (set this parent, set this rate), and run those actions when we drop the orphan flag. If some clock is orphaned that we're trying to configure, we can attach the action to a list in the clk_core structure. Otherwise we'll run the action immediately. This way, we do a best effort to run as much of the configuration as possible when the provider is registered the first time and skip the overhead of cycling through a potentially long list of provider actions to see if we can run them now. This last part may be over-engineered though. I'm not sure if we really have any such scenario today.
Am Donnerstag, 28. Januar 2016, 00:23:24 schrieb Stephen Boyd: > On 01/21, Emilio López wrote: > > @@ -3059,7 +3069,25 @@ struct clk *__of_clk_get_from_provider(struct > > of_phandle_args *clkspec,> > > */ > > > > struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec) > > { > > > > - return __of_clk_get_from_provider(clkspec, NULL, __func__); > > + return __of_clk_get_from_provider(clkspec, NULL, __func__, false); > > +} > > + > > +/** > > + * of_clk_get_from_provider_with_orphans() - Lookup clock from a clock > > provider + * @clkspec: pointer to a clock specifier data structure > > + * > > + * This function looks up a struct clk from the registered list of clock > > + * providers, an input is a clock specifier data structure as returned > > + * from the of_parse_phandle_with_args() function call. > > + * > > + * The difference to of_clk_get_from_provider() is that this function > > will > > + * also successfully lookup orphan-clocks, as it in some cases may be > > + * necessary to access such orphan-clocks as well. > > + */ > > +struct clk * > > +of_clk_get_from_provider_with_orphans(struct of_phandle_args *clkspec) > > Dislike. In fact, the whole clk conf approach is odd here. When > we're doing of_clk_init() we do a best effort loop around > parent_ready(), waiting for clk providers to register as long as > we have a clocks property in our provider node. We should do > something similar in the non of_clk_init() case too, because > of_clk_init() isn't special. At least to me being able to reparent orphan clocks when knowing that they won't ever get supplied is special. The Rockchip clock controller has quite a number of clocks that can either be supplied by some external source. xin32k supplied by some i2c chip being the most prominent. But while this one will get supplied eventually, there are others who will never get a supply and stay orphans forever. Example: rk3288 sclk_edp_24m can get supplied by either the general 24MHz oscillator or some separate clock input connected some chip pin. Reset-default seems to be the external supply, but on all boards I've seen so far doesn't get connected. So we know on a per-board level if this is connected and want to move away from the non-existent source. Which is why the limit of this new function is limited to be ccf internal and the assigned-clock-parents path. I guess if you really dislike that approach the other option would be reparenting all the time in the clock-controller driver and then let the board-dts reparent back if needed. Which would also work for that clock, but may cause other glitches down the road when it affects some pre-setup things. > Furthermore, the assigned parents and rates feature doesn't need > the clocks that we're assigning parents and rates to to even be > provided or consumed by the provider that's probing, so I'm lost > why we're checking the provider's node for a clocks property. It > would be better to check the assigned-clocks and assigned-parents > properties and make sure that those are all non-orphans. If > they're orphaned, we should delay until another clk provider is > registered. Eventually we'll unstick the orphans and then the > tree can be configured. Running the configuration at the end of > of_clk_init() even if we still can't get the clocks doesn't make > any sense to me. > > To be really nice, we could build up a set of configuration > actions (set this parent, set this rate), and run those actions > when we drop the orphan flag. If some clock is orphaned that > we're trying to configure, we can attach the action to a list in > the clk_core structure. Otherwise we'll run the action > immediately. This way, we do a best effort to run as much of the > configuration as possible when the provider is registered the > first time and skip the overhead of cycling through a potentially > long list of provider actions to see if we can run them now. This > last part may be over-engineered though. I'm not sure if we > really have any such scenario today.
On 01/28, Heiko Stübner wrote: > Am Donnerstag, 28. Januar 2016, 00:23:24 schrieb Stephen Boyd: > > On 01/21, Emilio López wrote: > > > @@ -3059,7 +3069,25 @@ struct clk *__of_clk_get_from_provider(struct > > > of_phandle_args *clkspec,> > > > */ > > > > > > struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec) > > > { > > > > > > - return __of_clk_get_from_provider(clkspec, NULL, __func__); > > > + return __of_clk_get_from_provider(clkspec, NULL, __func__, false); > > > +} > > > + > > > +/** > > > + * of_clk_get_from_provider_with_orphans() - Lookup clock from a clock > > > provider + * @clkspec: pointer to a clock specifier data structure > > > + * > > > + * This function looks up a struct clk from the registered list of clock > > > + * providers, an input is a clock specifier data structure as returned > > > + * from the of_parse_phandle_with_args() function call. > > > + * > > > + * The difference to of_clk_get_from_provider() is that this function > > > will > > > + * also successfully lookup orphan-clocks, as it in some cases may be > > > + * necessary to access such orphan-clocks as well. > > > + */ > > > +struct clk * > > > +of_clk_get_from_provider_with_orphans(struct of_phandle_args *clkspec) > > > > Dislike. In fact, the whole clk conf approach is odd here. When > > we're doing of_clk_init() we do a best effort loop around > > parent_ready(), waiting for clk providers to register as long as > > we have a clocks property in our provider node. We should do > > something similar in the non of_clk_init() case too, because > > of_clk_init() isn't special. > > At least to me being able to reparent orphan clocks when knowing that they > won't ever get supplied is special. > > The Rockchip clock controller has quite a number of clocks that can either be > supplied by some external source. xin32k supplied by some i2c chip being the > most prominent. But while this one will get supplied eventually, there are > others who will never get a supply and stay orphans forever. > > Example: > rk3288 sclk_edp_24m can get supplied by either the general 24MHz oscillator or > some separate clock input connected some chip pin. Reset-default seems to be > the external supply, but on all boards I've seen so far doesn't get connected. > > So we know on a per-board level if this is connected and want to move away > from the non-existent source. Which is why the limit of this new function is > limited to be ccf internal and the assigned-clock-parents path. Right, this problem isn't limited to clk-conf though. If some clock is orphaned by its reset default, then consumers that want to reparent them to something else will never be able to get the clocks without some sort of clk_get_ignore_orphan() API. We can't rely on DTS files telling us to do the reparent. This is a whole other problem to be dealt with before we can defer clk_get() orphans. In this case, I wonder why we haven't specified some sort of "ground" clock that has a rate of 0? In the example above, I would imagine that we have our dts file specify a fixed rate clock with frequency of 0 that's named whatever the external supply clock is named when that pin isn't connected. > > > I guess if you really dislike that approach the other option would be > reparenting all the time in the clock-controller driver and then let the > board-dts reparent back if needed. Which would also work for that clock, but > may cause other glitches down the road when it affects some pre-setup things. > > For all we know that could cause some audio pop on a speaker something. Let's avoid this if we can.
diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c index 43a218f..60ebfd9 100644 --- a/drivers/clk/clk-conf.c +++ b/drivers/clk/clk-conf.c @@ -13,6 +13,7 @@ #include <linux/device.h> #include <linux/of.h> #include <linux/printk.h> +#include "clk.h" static int __set_clk_parents(struct device_node *node, bool clk_supplier) { @@ -38,7 +39,7 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier) } if (clkspec.np == node && !clk_supplier) return 0; - pclk = of_clk_get_from_provider(&clkspec); + pclk = of_clk_get_from_provider_with_orphans(&clkspec); if (IS_ERR(pclk)) { pr_warn("clk: couldn't get parent clock %d for %s\n", index, node->full_name); @@ -53,7 +54,7 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier) rc = 0; goto err; } - clk = of_clk_get_from_provider(&clkspec); + clk = of_clk_get_from_provider_with_orphans(&clkspec); if (IS_ERR(clk)) { pr_warn("clk: couldn't get parent clock %d for %s\n", index, node->full_name); diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index b4db67a..36fe1a0 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -2494,15 +2494,11 @@ out: return ret; } -struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id, - const char *con_id) +struct clk *clk_hw_create_clk(struct clk_hw *hw, const char *dev_id, + const char *con_id) { struct clk *clk; - /* This is to allow this function to be chained to others */ - if (IS_ERR_OR_NULL(hw)) - return (struct clk *) hw; - clk = kzalloc(sizeof(*clk), GFP_KERNEL); if (!clk) return ERR_PTR(-ENOMEM); @@ -2519,6 +2515,19 @@ struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id, return clk; } +struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id, + const char *con_id, bool with_orphans) +{ + /* This is to allow this function to be chained to others */ + if (IS_ERR_OR_NULL(hw)) + return (struct clk *) hw; + + if (hw->core->orphan && !with_orphans) + return ERR_PTR(-EPROBE_DEFER); + + return clk_hw_create_clk(hw, dev_id, con_id); +} + void __clk_free_clk(struct clk *clk) { clk_prepare_lock(); @@ -2587,7 +2596,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw) INIT_HLIST_HEAD(&core->clks); - hw->clk = __clk_create_clk(hw, NULL, NULL); + hw->clk = clk_hw_create_clk(hw, NULL, NULL); if (IS_ERR(hw->clk)) { ret = PTR_ERR(hw->clk); goto fail_parent_names_copy; @@ -3019,7 +3028,8 @@ void of_clk_del_provider(struct device_node *np) EXPORT_SYMBOL_GPL(of_clk_del_provider); struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec, - const char *dev_id, const char *con_id) + const char *dev_id, const char *con_id, + bool with_orphans) { struct of_clk_provider *provider; struct clk *clk = ERR_PTR(-EPROBE_DEFER); @@ -3034,7 +3044,7 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec, clk = provider->get(clkspec, provider->data); if (!IS_ERR(clk)) { clk = __clk_create_clk(__clk_get_hw(clk), dev_id, - con_id); + con_id, with_orphans); if (!IS_ERR(clk) && !__clk_get(clk)) { __clk_free_clk(clk); @@ -3059,7 +3069,25 @@ struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec, */ struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec) { - return __of_clk_get_from_provider(clkspec, NULL, __func__); + return __of_clk_get_from_provider(clkspec, NULL, __func__, false); +} + +/** + * of_clk_get_from_provider_with_orphans() - Lookup clock from a clock provider + * @clkspec: pointer to a clock specifier data structure + * + * This function looks up a struct clk from the registered list of clock + * providers, an input is a clock specifier data structure as returned + * from the of_parse_phandle_with_args() function call. + * + * The difference to of_clk_get_from_provider() is that this function will + * also successfully lookup orphan-clocks, as it in some cases may be + * necessary to access such orphan-clocks as well. + */ +struct clk * +of_clk_get_from_provider_with_orphans(struct of_phandle_args *clkspec) +{ + return __of_clk_get_from_provider(clkspec, NULL, __func__, true); } int of_clk_get_parent_count(struct device_node *np) diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h index 00b35a1..5a70f09 100644 --- a/drivers/clk/clk.h +++ b/drivers/clk/clk.h @@ -13,17 +13,21 @@ struct clk_hw; #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK) struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec, - const char *dev_id, const char *con_id); + const char *dev_id, const char *con_id, + bool with_orphans); +struct clk * +of_clk_get_from_provider_with_orphans(struct of_phandle_args *clkspec); #endif #ifdef CONFIG_COMMON_CLK struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id, - const char *con_id); + const char *con_id, bool with_orphans); void __clk_free_clk(struct clk *clk); #else /* All these casts to avoid ifdefs in clkdev... */ static inline struct clk * -__clk_create_clk(struct clk_hw *hw, const char *dev_id, const char *con_id) +__clk_create_clk(struct clk_hw *hw, const char *dev_id, const char *con_id, + bool with_orphans) { return (struct clk *)hw; } diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c index 779b6ff..eda20c2 100644 --- a/drivers/clk/clkdev.c +++ b/drivers/clk/clkdev.c @@ -43,7 +43,7 @@ static struct clk *__of_clk_get(struct device_node *np, int index, if (rc) return ERR_PTR(rc); - clk = __of_clk_get_from_provider(&clkspec, dev_id, con_id); + clk = __of_clk_get_from_provider(&clkspec, dev_id, con_id, false); of_node_put(clkspec.np); return clk; @@ -177,7 +177,7 @@ struct clk *clk_get_sys(const char *dev_id, const char *con_id) if (!cl) goto out; - clk = __clk_create_clk(cl->clk_hw, dev_id, con_id); + clk = __clk_create_clk(cl->clk_hw, dev_id, con_id, false); if (IS_ERR(clk)) goto out;