Re: [PATCH v5 08/10] clk: fix CLK_SET_RATE_GATE with clock rate protection

[Jerome Brunet]

On Fri, 2017-12-01 at 22:51 +0100, Jerome Brunet wrote:
> Using clock rate protection, we can now enforce CLK_SET_RATE_GATE along the
> clock tree

Hi Mike, Stephen,

Gentle ping.
This patch has been waiting for while now.
As far as I know, the only blocking point to merging this patch is the qcom mmc
driver. The clocks used in this driver rely on the broken implementation of
CLK_SET_RATE_GATE, effectively ignoring the flag.

Since the flag is ignored, removing it from this particular clock won't make any
difference. Can we just do that until a better fix is available for this qcom
mmc clock ?

--- A bit of history ----

I managed to have a run on kci - based on v4.12-rc6:
https://kernelci.org/boot/all/job/khilman/branch/to-build/kernel/v4.12-rc6-10-ge
a373ddef830/

There was no build regression but kci did find one boot regression on qcom
platforms:
* qcom-apq8064-cm-qs600
* qcom-apq8064-ifc6410

it seems the problem is coming from the clock used by the mmc driver
(drivers/mmc/host/mmci.c)

the driver does following sequence:
* get_clk
* prepare_enable
* get_rate
* set_rate
* ...

with clock SDCx_clk (qcom_apq8064.dtsi:1037). This clock has CLK_SET_RATE_PARENT
flag so it will transmit the request to its parent.
The parent of this clock is SDCx_src which has the CLK_SET_RATE_GATE flag.

So obviously, is CLK_SET_RATE_GATE was enforced, the sequence used by mmci
driver would never had worked.

This particular driver rely on the fact that the clock can while the clock is
on. Either it actually works and we can remove CLK_SET_RATE_GATE until a better
solution comes.
OR, it does not work which means nobody has been using this driver for a long
time.

> 
> Acked-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> Tested-by: Quentin Schulz <quentin.schulz@xxxxxxxxxxxxxxxxxx>
> Tested-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx>
> Acked-by: Michael Turquette <mturquette@xxxxxxxxxxxx>
> Signed-off-by: Jerome Brunet <jbrunet@xxxxxxxxxxxx>
> ---
>  drivers/clk/clk.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index f6fe5e5595ca..1af843ae20ff 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -605,6 +605,9 @@ static void clk_core_unprepare(struct clk_core *core)
>  	if (WARN_ON(core->prepare_count == 1 && core->flags & CLK_IS_CRITICAL))
>  		return;
>  
> +	if (core->flags & CLK_SET_RATE_GATE)
> +		clk_core_rate_unprotect(core);
> +
>  	if (--core->prepare_count > 0)
>  		return;
>  
> @@ -679,6 +682,16 @@ static int clk_core_prepare(struct clk_core *core)
>  
>  	core->prepare_count++;
>  
> +	/*
> +	 * CLK_SET_RATE_GATE is a special case of clock protection
> +	 * Instead of a consumer claiming exclusive rate control, it is
> +	 * actually the provider which prevents any consumer from making any
> +	 * operation which could result in a rate change or rate glitch while
> +	 * the clock is prepared.
> +	 */
> +	if (core->flags & CLK_SET_RATE_GATE)
> +		clk_core_rate_protect(core);
> +
>  	return 0;
>  unprepare:
>  	clk_core_unprepare(core->parent);
> @@ -1780,9 +1793,6 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
>  	if (clk_core_rate_is_protected(core))
>  		return -EBUSY;
>  
> -	if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
> -		return -EBUSY;
> -
>  	/* calculate new rates and get the topmost changed clock */
>  	top = clk_calc_new_rates(core, req_rate);
>  	if (!top)