Re: [PATCH v3 0/2] clk: improve handling of orphan clocks

["Heiko Stübner" <heiko@sntech.de>]

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 <heiko@sntech.de>
Reviewed-by: Heiko Stuebner <heiko@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);