Am Freitag, 1. Mai 2015, 13:52:47 schrieb Stephen Boyd:
> On 05/01/15 12:59, Heiko Stübner wrote:
> > Am Donnerstag, 30. April 2015, 17:19:01 schrieb Stephen Boyd:
> >> On 04/22, Heiko Stuebner wrote:
> >>> Using orphan clocks can introduce strange behaviour as they don't have
> >>> rate information at all and also of course don't track
> >>>
> >>> This v2/v3 takes into account suggestions from Stephen Boyd to not try
> >>> to
> >>> walk the clock tree at runtime but instead keep track of orphan states
> >>> on clock tree changes and making it mandatory for everybody from the
> >>> start as orphaned clocks should not be used at all.
> >>>
> >>>
> >>> This fixes an issue on most rk3288 platforms, where some soc-clocks
> >>> are supplied by a 32khz clock from an external i2c-chip which often
> >>> is only probed later in the boot process and maybe even after the
> >>> drivers using these soc-clocks like the tsadc temperature sensor.
> >>> In this case the driver using the clock should of course defer probing
> >>> until the clock is actually usable.
> >>>
> >>>
> >>> As this changes the behaviour for orphan clocks, it would of course
> >>> benefit from more testing than on my Rockchip boards. To keep the
> >>> recipent-list reasonable and not spam to much I selected one (the
> >>> topmost)
> >>> from the get_maintainer output of each drivers/clk entry.
> >>> Hopefully some will provide Tested-by-tags :-)
> >>
> >> <grumble> I don't see any Tested-by: tags yet </grumble>. I've
> >> put these two patches on a separate branch "defer-orphans" and
> >> pushed it to clk-next so we can give it some more exposure.
> >>
> >> Unfortunately this doesn't solve the orphan problem for non-OF
> >> providers. What if we did the orphan check in __clk_create_clk()
> >> instead and returned an error pointer for orphans? I suspect that
> >> will solve all cases, right?
> >
> > hmm, clk_register also uses __clk_create_clk, which in turn would prevent
> > registering orphan-clocks at all, I'd think.
> > As on my platform I'm dependant on orphan clocks (the soc-level clock gets
> > registerted as part of the big clock controller way before the i2c-based
> > supplying clock), I'd rather not touch this :-) .
>
> Have no fear! We should just change clk_register() to call a
> __clk_create_clk() type function that doesn't check for orphan status.
ok :-D
> > Instead I guess we could hook it less deep into clk_get_sys, like in the
> > following patch?
>
> It looks like it will work at least, but still I'd prefer to keep the
> orphan check contained to clk.c. How about this compile tested only patch?
I gave this a spin on my rk3288-firefly board. It still boots, the clock tree
looks the same and it also still defers nicely in the scenario I needed it
for. The implementation also looks nice - and of course much more compact than
my check in two places :-) . I don't know if you want to put this as follow-up
on top or fold it into the original orphan-check, so in any case
Tested-by: Heiko Stuebner <he...@sntech.de>
Reviewed-by: Heiko Stuebner <he...@sntech.de>
> This also brings up an existing problem with clk_unregister() where
> orphaned clocks are sitting out there useable by drivers when their
> parent is unregistered. That code could use some work to atomically
> switch all the orphaned clocks over to use the nodrv_ops.
Not sure I understand this correctly yet, but when these children get
orphaned, switched to the clk_nodrv_ops, they won't get their original ops
back if the parent reappears.
So I guess we would need to store the original ops in secondary property of
struct clk_core and I guess simply bind the ops-switch to the orphan state
update?
>
> ----8<-----
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 30d45c657a07..1d23daa42dd2 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2221,14 +2221,6 @@ static inline void clk_debug_unregister(struct
> clk_core *core) }
> #endif
>
> -static bool clk_is_orphan(const struct clk *clk)
> -{
> - if (!clk)
> - return false;
> -
> - return clk->core->orphan;
> -}
> -
> /**
> * __clk_init - initialize the data structures in a struct clk
> * @dev: device initializing this clk, placeholder for now
> @@ -2420,15 +2412,11 @@ out:
> return ret;
> }
>
> -struct clk *__clk_create_clk(struct clk_hw *hw, const char *dev_id,
> - const char *con_id)
> +static 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 (!hw || IS_ERR(hw))
> - return (struct clk *) hw;
> -
> clk = kzalloc(sizeof(*clk), GFP_KERNEL);
> if (!clk)
> return ERR_PTR(-ENOMEM);
> @@ -2445,6 +2433,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)
> +{
> + /* This is to allow this function to be chained to others */
> + if (!hw || IS_ERR(hw))
> + return (struct clk *) hw;
> +
> + if (hw->core->orphan)
> + 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();
> @@ -2511,7 +2512,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;
> @@ -2958,10 +2959,6 @@ struct clk *__of_clk_get_from_provider(struct
> of_phandle_args *clkspec, if (provider->node == clkspec->np)
> clk = provider->get(clkspec, provider->data);
> if (!IS_ERR(clk)) {
> - if (clk_is_orphan(clk)) {
> - clk = ERR_PTR(-EPROBE_DEFER);
> - break;
> - }
>
> clk = __clk_create_clk(__clk_get_hw(clk), dev_id,
> con_id);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
