Re: [RFC PATCH 1/2] clk: add property for force to update clock setting

[Doug Anderson <dianders@chromium.org>]

Hi,

On Thu, Nov 13, 2014 at 6:53 AM, Heiko Stübner <heiko@sntech.de> wrote:
> Am Donnerstag, 13. November 2014, 21:20:25 schrieb Kever Yang:
>> Usually we assigned a clock to a default rate in dts,
>> there is a situation that the clock already initialized to the rate
>> we intend to set before kernel(hardware default or init in uboot etc).
>> For the PLLs we can get a rate from different PLL parameter configure,
>> we can't change the PLL parameter if the rate is not changed by now.
>>
>> This patch adds a option property 'assigned-clock-force-rates'
>> to make sure we update all the setting even if we don't need to
>> update the clock rate.
>>
>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>> ---
>>
>>  drivers/clk/clk-conf.c | 33 ++++++++++++++++++++++++++++++++-
>>  1 file changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
>> index aad4796..0c9df48 100644
>> --- a/drivers/clk/clk-conf.c
>> +++ b/drivers/clk/clk-conf.c
>> @@ -84,7 +84,7 @@ static int __set_clk_rates(struct device_node *node, bool
>> clk_supplier) struct clk *clk;
>>       u32 rate;
>>
>> -     of_property_for_each_u32(node, "assigned-clock-rates", prop, cur, rate) {
>> +     of_property_for_each_u32(node, "assigned-force-rates", prop, cur, rate) {
>>               if (rate) {
>>                       rc = of_parse_phandle_with_args(node, "assigned-clocks",
>>                                       "#clock-cells", index, &clkspec);
>> @@ -104,7 +104,38 @@ static int __set_clk_rates(struct device_node *node,
>> bool clk_supplier) index, node->full_name);
>>                               return PTR_ERR(clk);
>>                       }
>> +                     /* change the old rate to 0 to make sure we can get into
>> +                      * clk_change_rate */
>> +                     clk->rate = 0;
>> +                     rc = clk_set_rate(clk, rate);
>> +                     if (rc < 0)
>> +                             pr_err("clk: couldn't set %s clock rate: %d\n",
>> +                                    __clk_get_name(clk), rc);
>> +                     clk_put(clk);
>
> Forcing clocks to 0 at first will probably create issues on some platfoms.
> I think what Doug meant was something like [0], which would then enable
> the clk_conf part to force the rate change. I haven't tested this yet, but it
> seems the check in clk_set_rate is the only one checking for identical new
> and old rates.

Hrm, I was actually not thinking of adding a new device tree property.
I was thinking that we'd _always_ call "force" for
"assigned-clock-rates".  Really the check in clk_set_rate() is an
optimization (right?), not for correctness.  Thus it should be OK to
bypass it at bootup.

Actually, maybe even better: for all clocks you should always skip the
"clk_get_rate()" check the first time through.  Then you'd ensure that
you aren't using some default or firmware-assigned clock settings.
AKA, something like this untested patch:

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 59d853d..56db138 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1618,9 +1618,10 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
        /* prevent racing with updates to the clock topology */
        clk_prepare_lock();

-       /* bail early if nothing to do */
-       if (rate == clk_get_rate(clk))
+       /* bail early if nothing to do; linux should always set the rate once */
+       if (rate == clk_get_rate(clk) && clk->did_set_rate)
                goto out;
+       clk->did_set_rate = true;

        if ((clk->flags & CLK_SET_RATE_GATE) && clk->prepare_count) {
                ret = -EBUSY;

I'm ducking now in case Mike decides to throw a tomato at me.

-Doug