Re: [PATCH v2] clk: respect the clock dependencies in of_clk_init

[Tomasz Figa]

Hi Gregory,

On 10.02.2014 18:42, Gregory CLEMENT wrote:
> Until now the clock providers were initialized in the order found in
> the device tree. This led to have the dependencies between the clocks
> not respected: children clocks could be initialized before their
> parent clocks.
>
> Instead of forcing each platform to manage its own initialization order,
> this patch adds this work inside the framework itself.
>
> Using the data of the device tree the of_clk_init function now delayed
> the initialization of a clock provider if its parent provider was not
> ready yet.

In general this is really great. It's a first step towards sorting out 
dependencies between clock providers correctly. I have some comments 
inline, though.

> The strict dependency check (all parents of a given clk must be
> initialized) was added by Boris BREZILLON

Shouldn't this be reflected by a tag of this patch? If you squash a 
patch signed off by someone then I believe their sign-off tag should be 
added to the base patch. Correct me if I'm wrong, though.

> Signed-off-by: Gregory CLEMENT <gregory.clement@xxxxxxxxxxxxxxxxxx>
> ---
>
> Since the v1, I have merged the  strict dependency check from Boris.
> And of course tested on my Armada 370 and Armada XP based board
>
>   drivers/clk/clk.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 106 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 5517944495d8..684976993297 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2526,24 +2526,127 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
>   }
>   EXPORT_SYMBOL_GPL(of_clk_get_parent_name);
>
> +struct clock_provider {

The name is a bit too generic and slightly misleading. IMHO struct 
deferred_clk_provider (and deferred_clk_providers for the list) would be 
better.

> +	of_clk_init_cb_t clk_init_cb;
> +	struct device_node *np;
> +	struct list_head node;
> +};
> +
> +static LIST_HEAD(clk_provider_list);
> +
> +/*
> + * This function looks for a parent clock. If there is one, then it
> + * checks that the provider for this parent clock was initialized, in
> + * this case the parent clock will be ready.
> + */
> +static int parent_ready(struct device_node *np)
> +{
> +	struct of_phandle_args clkspec;
> +	struct of_clk_provider *provider;
> +	int num_parents;
> +	bool found;
> +	int i;
> +
> +	/*
> +	 * If there is no clock parent, no need to wait for them, then
> +	 * we can consider their absence as being ready
> +	 */
> +	num_parents = of_count_phandle_with_args(np, "clocks", "#clock-cells");
> +	if (num_parents <= 0)
> +		return 1;

of_clk_get_parent_count() can be used here...

> +
> +	for (i = 0; i < num_parents; i++) {
> +		if (of_parse_phandle_with_args(np, "clocks", "#clock-cells", i,
> +					       &clkspec))
> +			return 1;
> +
> +		/* Check if we have such a provider in our array */
> +		found = false;
> +		list_for_each_entry(provider, &of_clk_providers, link) {
> +			if (provider->node == clkspec.np) {
> +				found = true;
> +				break;
> +			}
> +		}
> +
> +		if (!found)
> +			return 0;
> +	}

...or even better, __of_clk_get_from_provider() could modified to return 
-EPROBE_DEFER if requested provider is not registered and you could 
simply call of_clk_get(np, i) and handle its return value appropriately:

- on !IS_ERR(clk) call clk_put() and continue with iterations,
- on IS_ERR(clk) && PTR_ERR(clk) == -EPROBE_DEFER return 0 immediately,
- in any other case end the loop (end of clock specifiers).

This would make CCF even closer to proper handling of provider ordering, 
with a nice side effect of handling deferred probe for platform devices.

> +
> +	return 1;
> +}
> +
>   /**
>    * of_clk_init() - Scan and init clock providers from the DT
>    * @matches: array of compatible values and init functions for providers.
>    *
> - * This function scans the device tree for matching clock providers and
> - * calls their initialization functions
> + * This function scans the device tree for matching clock providers
> + * and calls their initialization functions. It also do it by trying
> + * to follow the dependencies.
>    */
>   void __init of_clk_init(const struct of_device_id *matches)
>   {
>   	const struct of_device_id *match;
>   	struct device_node *np;
> +	struct clock_provider *clk_provider, *next;
> +	bool is_init_done;
>
>   	if (!matches)
>   		matches = &__clk_of_table;
>
>   	for_each_matching_node_and_match(np, matches, &match) {
>   		of_clk_init_cb_t clk_init_cb = match->data;
> -		clk_init_cb(np);
> +
> +
> +		if (parent_ready(np)) {
> +			/*
> +			 * The parent clock is ready or there is no
> +			 * clock parent at all, in this case the
> +			 * provider can be initialize immediately.
> +			 */
> +			clk_init_cb(np);
> +		} else {
> +			/*
> +			 * The parent clock is not ready, this
> +			 * provider is moved to a list to be
> +			 * initialized later
> +			 */
> +			struct clock_provider *parent = kzalloc(sizeof(struct clock_provider),
> +							GFP_KERNEL);
> +
> +			parent->clk_init_cb = match->data;
> +			parent->np = np;
> +			list_add(&parent->node, &clk_provider_list);
> +		}
> +	}

I wonder if this couldn't be replaced with simply adding all the 
providers to the list first and then proceeding with the loop below to 
handle the registrations.

> +
> +	while (!list_empty(&clk_provider_list)) {
> +		is_init_done = false;
> +		list_for_each_entry_safe(clk_provider, next,
> +					&clk_provider_list, node) {
> +			if (parent_ready(clk_provider->np)) {
> +				clk_provider->clk_init_cb(clk_provider->np);
> +				list_del(&clk_provider->node);
> +				kfree(clk_provider);
> +				is_init_done = true;
> +			}
> +		}
> +
> +		if (!is_init_done) {
> +			/*
> +			 * We didn't managed to initialize any of the
> +			 * remaining providers during the last loop,
> +			 * so now we initialize all the remaining ones
> +			 * unconditionally in case the clock parent
> +			 * was not mandatory
> +			 */
> +			list_for_each_entry_safe(clk_provider, next,
> +						&clk_provider_list, node) {
> +				clk_provider->clk_init_cb(clk_provider->np);
> +				list_del(&clk_provider->node);
> +				kfree(clk_provider);

Hmm, this is basically the code above repeated without the if. What 
about something like the code snippet below?

bool force = false;
while (!list_empty(&clk_provider_list)) {
	is_init_done = false;
	list_for_each_entry_safe(clk_provider, next,
				&clk_provider_list, node) {
		if (force || parent_ready(clk_provider->np)) {
			clk_provider->clk_init_cb(clk_provider->np);
			list_del(&clk_provider->node);
			kfree(clk_provider);
			is_init_done = true;
		}
	}

	if (!is_init_done)
		force = true;
}

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/