[RFC PATCH v2] clk: move check of CLK_SET_RATE_GATE flag to clk_propagate_rate_change()

[RFC PATCH v2] clk: move check of CLK_SET_RATE_GATE flag to clk_propagate_rate_change()

[jiada_wang]

From: Jiada Wang <jiada_wang@mentor.com>

Previously CLK_SET_RATE_GATE flag is only checked in clk_set_rate()
which only ensures the clock being called by clk_set_rate() won't
change rate when it has been prepared if CLK_SET_RATE_GATE flag is set.
But a clk_set_rate() request may propagate rate change to these clocks
from the requested clock's topmost parent clock to all its offsprings,
when any one of these clocks has CLK_SET_RATE_GATE flag set
and it has been prepared, the clk_set_rate() request should fail.

This patch moves check of CLK_SET_RATE_GATE flag to
clk_propagate_rate_change() to ensure all affected clocks will
be checked if their rate will be changed after clk_set_rate().

Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
---
 drivers/clk/clk.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 820a939..2f930c8 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1437,6 +1437,9 @@ static struct clk_core *clk_propagate_rate_change(struct clk_core *core,
 	if (core->rate == core->new_rate)
 		return NULL;
 
+	if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
+		return core;
+
 	if (core->notifier_count) {
 		ret = __clk_notify(core, event, core->rate, core->new_rate);
 		if (ret & NOTIFY_STOP_MASK)
@@ -1571,9 +1574,6 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 	if (rate == clk_core_get_rate_nolock(core))
 		return 0;
 
-	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, rate);
 	if (!top)
-- 
1.7.9.5

Re: [RFC PATCH v2] clk: move check of CLK_SET_RATE_GATE flag to clk_propagate_rate_change()

[Michael Turquette]

Quoting jiada_wang@mentor.com (2016-07-10 22:33:28)
> From: Jiada Wang <jiada_wang@mentor.com>
> =

> Previously CLK_SET_RATE_GATE flag is only checked in clk_set_rate()
> which only ensures the clock being called by clk_set_rate() won't
> change rate when it has been prepared if CLK_SET_RATE_GATE flag is set.
> But a clk_set_rate() request may propagate rate change to these clocks
> from the requested clock's topmost parent clock to all its offsprings,
> when any one of these clocks has CLK_SET_RATE_GATE flag set
> and it has been prepared, the clk_set_rate() request should fail.
> =

> This patch moves check of CLK_SET_RATE_GATE flag to
> clk_propagate_rate_change() to ensure all affected clocks will
> be checked if their rate will be changed after clk_set_rate().
> =

> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
> ---

What's different in version 2? It's tradition to put a little version
changelog here, below the "---" line and above the "diff --git a/..."
line.

Regards,
Mike

>  drivers/clk/clk.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> =

> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 820a939..2f930c8 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1437,6 +1437,9 @@ static struct clk_core *clk_propagate_rate_change(s=
truct clk_core *core,
>         if (core->rate =3D=3D core->new_rate)
>                 return NULL;
>  =

> +       if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
> +               return core;
> +
>         if (core->notifier_count) {
>                 ret =3D __clk_notify(core, event, core->rate, core->new_r=
ate);
>                 if (ret & NOTIFY_STOP_MASK)
> @@ -1571,9 +1574,6 @@ static int clk_core_set_rate_nolock(struct clk_core=
 *core,
>         if (rate =3D=3D clk_core_get_rate_nolock(core))
>                 return 0;
>  =

> -       if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
> -               return -EBUSY;
> -
>         /* calculate new rates and get the topmost changed clock */
>         top =3D clk_calc_new_rates(core, rate);
>         if (!top)
> -- =

> 1.7.9.5
> =

>=20

Re: [RFC PATCH v2] clk: move check of CLK_SET_RATE_GATE flag to clk_propagate_rate_change()

[Jiada Wang]

Hello Michael

On 07/13/2016 07:29 AM, Michael Turquette wrote:
> Quoting jiada_wang@mentor.com (2016-07-10 22:33:28)
>> From: Jiada Wang <jiada_wang@mentor.com>
>>
>> Previously CLK_SET_RATE_GATE flag is only checked in clk_set_rate()
>> which only ensures the clock being called by clk_set_rate() won't
>> change rate when it has been prepared if CLK_SET_RATE_GATE flag is set.
>> But a clk_set_rate() request may propagate rate change to these clocks
>> from the requested clock's topmost parent clock to all its offsprings,
>> when any one of these clocks has CLK_SET_RATE_GATE flag set
>> and it has been prepared, the clk_set_rate() request should fail.
>>
>> This patch moves check of CLK_SET_RATE_GATE flag to
>> clk_propagate_rate_change() to ensure all affected clocks will
>> be checked if their rate will be changed after clk_set_rate().
>>
>> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
>> ---
>
> What's different in version 2? It's tradition to put a little version
> changelog here, below the "---" line and above the "diff --git a/..."
> line.
>
version 2 resolves the following kernel warning
"
    drivers/clk/clk.c: In function 'clk_propagate_rate_change':
 >> drivers/clk/clk.c:1441:3: warning: return makes pointer from integer 
without a cast
       return -EBUSY;
       ^
"
I forgot to add a changelog in v2 patch, sorry for the confusion caused.

Thanks,
Jiada
> Regards,
> Mike
>
>>   drivers/clk/clk.c |    6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index 820a939..2f930c8 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -1437,6 +1437,9 @@ static struct clk_core *clk_propagate_rate_change(struct clk_core *core,
>>          if (core->rate == core->new_rate)
>>                  return NULL;
>>
>> +       if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
>> +               return core;
>> +
>>          if (core->notifier_count) {
>>                  ret = __clk_notify(core, event, core->rate, core->new_rate);
>>                  if (ret & NOTIFY_STOP_MASK)
>> @@ -1571,9 +1574,6 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
>>          if (rate == clk_core_get_rate_nolock(core))
>>                  return 0;
>>
>> -       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, rate);
>>          if (!top)
>> --
>> 1.7.9.5
>>
>>

Re: [RFC PATCH v2] clk: move check of CLK_SET_RATE_GATE flag to clk_propagate_rate_change()

[Stephen Boyd]

On 07/10, jiada_wang@mentor.com wrote:
> From: Jiada Wang <jiada_wang@mentor.com>
> 
> Previously CLK_SET_RATE_GATE flag is only checked in clk_set_rate()
> which only ensures the clock being called by clk_set_rate() won't
> change rate when it has been prepared if CLK_SET_RATE_GATE flag is set.
> But a clk_set_rate() request may propagate rate change to these clocks
> from the requested clock's topmost parent clock to all its offsprings,

s/offsprings/children/ please

> when any one of these clocks has CLK_SET_RATE_GATE flag set
> and it has been prepared, the clk_set_rate() request should fail.
> 
> This patch moves check of CLK_SET_RATE_GATE flag to
> clk_propagate_rate_change() to ensure all affected clocks will
> be checked if their rate will be changed after clk_set_rate().
> 
> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>

I'm slightly worried that this will break providers that were
relying on the previous (mis)behavior of this flag. For example,
I think I have this flag set on clks in the qcom/gcc-msm8960.c
driver that have so far not triggered but will trigger now with
this patch. I suppose we should just delete the flag from those
clks because things are working fine so far anyway.

This also brings up the question about what drivers should do if
this flag is set and clk_set_rate() fails. Should drivers need to
know if they're on a platform where clk_set_rate() is going to
fail because the clk is not gated and take appropriate action?
How would they know this? Or should the framework forcibly gate
the clk and all the children, change the rate, and then ungate?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC PATCH v2] clk: move check of CLK_SET_RATE_GATE flag to clk_propagate_rate_change()

[Jiada Wang]

Hello Stephen

On 08/11/2016 07:19 AM, Stephen Boyd wrote:
> On 07/10, jiada_wang@mentor.com wrote:
>> From: Jiada Wang <jiada_wang@mentor.com>
>>
>> Previously CLK_SET_RATE_GATE flag is only checked in clk_set_rate()
>> which only ensures the clock being called by clk_set_rate() won't
>> change rate when it has been prepared if CLK_SET_RATE_GATE flag is set.
>> But a clk_set_rate() request may propagate rate change to these clocks
>> from the requested clock's topmost parent clock to all its offsprings,
>
> s/offsprings/children/ please
>
will update in next version.

>> when any one of these clocks has CLK_SET_RATE_GATE flag set
>> and it has been prepared, the clk_set_rate() request should fail.
>>
>> This patch moves check of CLK_SET_RATE_GATE flag to
>> clk_propagate_rate_change() to ensure all affected clocks will
>> be checked if their rate will be changed after clk_set_rate().
>>
>> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
>
> I'm slightly worried that this will break providers that were
> relying on the previous (mis)behavior of this flag. For example,
> I think I have this flag set on clks in the qcom/gcc-msm8960.c
> driver that have so far not triggered but will trigger now with
> this patch. I suppose we should just delete the flag from those
> clks because things are working fine so far anyway.
>
I am also worrying about this, that was why I added RFC tag in my patch.
I am not sure if remove all existing CLK_SET_RATE_GATE flags will cause
any issue, for example CLK_SET_RATE_GATE flag still works for these
clocks directly called by clk_set_rate(). if remove all
CLK_SET_RATE_GATE flags, will cause functional change for these clocks.

> This also brings up the question about what drivers should do if
> this flag is set and clk_set_rate() fails. Should drivers need to
> know if they're on a platform where clk_set_rate() is going to
> fail because the clk is not gated and take appropriate action?
> How would they know this? Or should the framework forcibly gate
> the clk and all the children, change the rate, and then ungate?
>
IMO, an error message with the error'ing clock to notify user that
clk_set_rate() is necessary. but clock framework don't need to forcibly
gate the clock (as the clock with CLK_SET_RATE_GATE flag maybe owned by
some other module)

Thanks,
Jiada