[RFC Patch v2 1/3] clk: add support for temporary parent clock migration

[Chander Kashyap]

On 4 September 2013 03:17, Sylwester Nawrocki
<sylvester.nawrocki at gmail.com> wrote:
> Hi Chander,
>
>
> On 09/03/2013 01:34 PM, Chander Kashyap wrote:
>>
>> Some platforms use to migrate temporarily to another parent during cpu
>> frequency
>> scaling, e.g. Exynos and Tegra. Once the frequency is changed the latch on
>> to
>> original parent.
>>
>> The generic cpufreq-cpu0 driver use clk_set_rate API to scale cpu
>> frequency.
>> This patch is an attempt to address the above mentioned requirement.
>>
>> This is achieved as follows:
>>
>> Add a clk flag "CLK_SET_RATE_TEMP_PARENT" for clocks which need to migrate
>> to
>> another parent during set_rate operation on them.
>>
>> Add "temp_parent_name" and "tmp_parent" fields to clk structure i.e the
>> name of
>> temp_parent_clock and reference to temp_parent_clock.
>>
>> Hence in clk_set_rate API check for the "CLK_SET_RATE_TEMP_PARENT" flag,
>> then
>> latch on to alternate parent clock temporarily. Once the requested rate is
>> set
>> on the clk, re-parent back to original parent clock.
>>
>> Signed-off-by: Chander Kashyap<chander.kashyap at linaro.org>
>> ---
>>   drivers/clk/clk-mux.c        |   13 +++++++------
>>   drivers/clk/clk.c            |   43
>> ++++++++++++++++++++++++++++++++++++++++--
>>   include/linux/clk-private.h  |   19 +++++++++++--------
>>   include/linux/clk-provider.h |   10 ++++++----
>>   4 files changed, 65 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
>> index 4f96ff3..854b3ac 100644
>> --- a/drivers/clk/clk-mux.c
>> +++ b/drivers/clk/clk-mux.c
>> @@ -115,8 +115,8 @@ EXPORT_SYMBOL_GPL(clk_mux_ro_ops);
>>
>>   struct clk *clk_register_mux_table(struct device *dev, const char *name,
>>                 const char **parent_names, u8 num_parents, unsigned long
>> flags,
>> -               void __iomem *reg, u8 shift, u32 mask,
>> -               u8 clk_mux_flags, u32 *table, spinlock_t *lock)
>> +               const char *temp_parent_name, void __iomem *reg, u8 shift,
>> +               u32 mask, u8 clk_mux_flags, u32 *table, spinlock_t *lock)
>
>
> I'm not sure this is a good idea to split the changes like this. Applying
> this patch alone would cause a build break, wouldn't it ?

Yes this will build break if patch 1 is applied only.
>
> The users need to be updated in same patch, so patch 2/3 should be normally
> folded into this one.

ok

>
> [...]
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index 2db08c0..0e29a5b 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -1425,8 +1425,8 @@ static void clk_change_rate(struct clk *clk)
>>    */
>>   int clk_set_rate(struct clk *clk, unsigned long rate)
>>   {
>> -       struct clk *top, *fail_clk;
>> -       int ret = 0;
>> +       struct clk *top, *fail_clk, *parent = NULL;
>> +       int ret = 0, index;
>>
>>         if (!clk)
>>                 return 0;
>> @@ -1450,6 +1450,35 @@ int clk_set_rate(struct clk *clk, unsigned long
>> rate)
>>                 goto out;
>>         }
>>
>> +       /* Latch on to alternate parent temporarily if needed */
>> +       if ((clk->flags&  CLK_SET_RATE_TEMP_PARENT)&&
>> clk->temp_parent_name) {
>>
>> +               /* Save current parent before latching on to alternate
>> parent */
>> +               parent = clk->parent;
>> +
>> +               if (!clk->temp_parent) {
>> +                       for (index = 0; index<  clk->num_parents; index++)
>> {
>> +                               if (!strcmp(clk->parent_names[index],
>> +                                               clk->temp_parent_name))
>> +                                       clk->temp_parent =
>> +
>> __clk_lookup(clk->temp_parent_name);
>> +                       }
>> +
>> +                       if (!clk->temp_parent) {
>> +                               pr_warn("%s: wrong temp_parent_name %s",
>> +                                       __func__, clk->temp_parent_name);
>> +                               ret = -EINVAL;
>> +                               goto out;
>> +                       }
>> +               }
>> +
>> +               ret = clk_set_parent(clk, clk->temp_parent);
>> +               if (ret) {
>> +                       pr_warn("%s: failed to set %s parent\n",
>> +                               __func__, clk->temp_parent->name);
>> +                       goto out;
>> +               }
>> +       }
>> +
>>         /* notify that we are about to change rates */
>>         fail_clk = clk_propagate_rate_change(top, PRE_RATE_CHANGE);
>>         if (fail_clk) {
>> @@ -1464,6 +1493,14 @@ int clk_set_rate(struct clk *clk, unsigned long
>> rate)
>>         clk_change_rate(top);
>>
>>   out:
>> +       /* Reparent back to original parent */
>> +       if (parent) {
>> +               ret = clk_set_parent(clk, parent);
>> +               if (ret)
>> +                       pr_warn("%s: failed to set %s parent\n",
>> +                                       __func__, parent->name);
>> +       }
>> +
>>         clk_prepare_unlock();
>>
>>         return ret;
>
> [...]
>
>> diff --git a/include/linux/clk-private.h b/include/linux/clk-private.h
>> index 8138c94..b70ba4d 100644
>> --- a/include/linux/clk-private.h
>> +++ b/include/linux/clk-private.h
>> @@ -47,6 +47,8 @@ struct clk {
>>   #ifdef CONFIG_COMMON_CLK_DEBUG
>>         struct dentry           *dentry;
>>   #endif
>> +       const char              *temp_parent_name;
>> +       struct clk              *temp_parent;
>
>
> Shouldn't such data rather be on struct clk_mux level ? It's only needed
> for _some_ mux clocks, while it's being added for all the clock types.
>
> Or wouldn't just a single "u8 temp_parent_index" field do ? The order of
> struct clk::parents and struct clk::parent_names is always same, isn't it ?
> Then if, e.g.
>
> parent_names[] = "clk_a", "clk_b", "clk_c";
>
> and "clk_c" is the temporary parent clock name, the corresponding clock
> pointer storage is clk->parents[2] ? It could be used instead of
> clk->temp_parent, couldn't it ?
>

Yes can be.
The mentioned approach provides sanity check, i.e when parent exists
and properly registered.

> --
> Regards,
> Sylwester

thanks,

-- 
with warm regards,
Chander Kashyap