Re: [PATCH v4 06/11] clk: at91: clk-sam9x60-pll: allow runtime changes for pll

[<Claudiu.Beznea@microchip.com>]

On 14.11.2020 23:14, Stephen Boyd wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Quoting Claudiu Beznea (2020-11-06 01:46:23)
>> diff --git a/drivers/clk/at91/clk-sam9x60-pll.c b/drivers/clk/at91/clk-sam9x60-pll.c
>> index 78f458a7b2ef..6fe5d8530a0c 100644
>> --- a/drivers/clk/at91/clk-sam9x60-pll.c
>> +++ b/drivers/clk/at91/clk-sam9x60-pll.c
>> @@ -225,8 +225,51 @@ static int sam9x60_frac_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>>                                      unsigned long parent_rate)
>>  {
>>         struct sam9x60_pll_core *core = to_sam9x60_pll_core(hw);
>> +       struct sam9x60_frac *frac = to_sam9x60_frac(core);
>> +       struct regmap *regmap = core->regmap;
>> +       unsigned long irqflags, clkflags = clk_hw_get_flags(hw);
>> +       unsigned int val, cfrac, cmul;
>> +       long ret;
>> +
>> +       ret = sam9x60_frac_pll_compute_mul_frac(core, rate, parent_rate, true);
>> +       if (ret <= 0 || (clkflags & CLK_SET_RATE_GATE))
> 
> Is this function being called when the clk is enabled and it has the
> CLK_SET_RATE_GATE flag set?

Yes, this function could be called when CLK_SET_RATE_GATE is set.
On SAMA7G5 there are multiple PLL blocks of the same type. All these PLLs
are controlled by clk-sam9x60-pll.c driver. One of this PLL block fed the
CPU who's frequency could be changed at run time. At the same time there
are PLLs that fed hardware block not glitch free aware or that we don't
want to allow the rate change (this is the case of SAM9X60's CPU PLL, or
the DDR PLL on SAMA7G5).

I'm confused why this driver needs to check
> this flag.

Because we have multiple PLLs of the same type, some of them feed hardware
blocks that are glitch free aware of these PLLs' frequencies changes, some
feed hardware blocks that are not glitch free aware of PLLs' frequencies
changes or for some of them we don't want the frequency changes at all.

> 
>> +               return ret;
>> +
>> +       spin_lock_irqsave(core->lock, irqflags);
>> +
>> +       regmap_update_bits(regmap, AT91_PMC_PLL_UPDT, AT91_PMC_PLL_UPDT_ID_MSK,
>> +                          core->id);
>> +       regmap_read(regmap, AT91_PMC_PLL_CTRL1, &val);
>> +       cmul = (val & core->layout->mul_mask) >> core->layout->mul_shift;
>> +       cfrac = (val & core->layout->frac_mask) >> core->layout->frac_shift;
>> +
>> +       if (cmul == frac->mul && cfrac == frac->frac)
>> +               goto unlock;
>> +
>> +       regmap_write(regmap, AT91_PMC_PLL_CTRL1,
>> +                    (frac->mul << core->layout->mul_shift) |
>> +                    (frac->frac << core->layout->frac_shift));
>> +
>> +       regmap_update_bits(regmap, AT91_PMC_PLL_UPDT,
>> +                          AT91_PMC_PLL_UPDT_UPDATE | AT91_PMC_PLL_UPDT_ID_MSK,
>> +                          AT91_PMC_PLL_UPDT_UPDATE | core->id);
>> +
>> +       regmap_update_bits(regmap, AT91_PMC_PLL_CTRL0,
>> +                          AT91_PMC_PLL_CTRL0_ENLOCK | AT91_PMC_PLL_CTRL0_ENPLL,
>> +                          AT91_PMC_PLL_CTRL0_ENLOCK |
>> +                          AT91_PMC_PLL_CTRL0_ENPLL);
>> +
>> +       regmap_update_bits(regmap, AT91_PMC_PLL_UPDT,
>> +                          AT91_PMC_PLL_UPDT_UPDATE | AT91_PMC_PLL_UPDT_ID_MSK,
>> +                          AT91_PMC_PLL_UPDT_UPDATE | core->id);
>>
>> -       return sam9x60_frac_pll_compute_mul_frac(core, rate, parent_rate, true);
>> +       while (!sam9x60_pll_ready(regmap, core->id))
>> +               cpu_relax();
>> +
>> +unlock:
>> +       spin_unlock_irqrestore(core->lock, irqflags);
>> +
>> +       return ret;
>>  }
>>
>>  static const struct clk_ops sam9x60_frac_pll_ops = {
>> @@ -378,9 +421,39 @@ static int sam9x60_div_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>>  {
>>         struct sam9x60_pll_core *core = to_sam9x60_pll_core(hw);
>>         struct sam9x60_div *div = to_sam9x60_div(core);
>> +       struct regmap *regmap = core->regmap;
>> +       unsigned long irqflags, clkflags = clk_hw_get_flags(hw);
>> +       unsigned int val, cdiv;
>>
>>         div->div = DIV_ROUND_CLOSEST(parent_rate, rate) - 1;
>>
>> +       if (clkflags & CLK_SET_RATE_GATE)
> 
> Same comment.
> 
>> diff --git a/drivers/clk/at91/sama7g5.c b/drivers/clk/at91/sama7g5.c
>> index d685e22b2014..33faf7c6d9fb 100644
>> --- a/drivers/clk/at91/sama7g5.c
>> +++ b/drivers/clk/at91/sama7g5.c
>> @@ -95,15 +95,15 @@ static const struct clk_pll_layout pll_layout_divio = {
>>   * @p:         clock parent
>>   * @l:         clock layout
>>   * @t:         clock type
>> - * @f:         true if clock is critical and cannot be disabled
>> + * @f:         clock flags
>>   * @eid:       export index in sama7g5->chws[] array
>>   */
>>  static const struct {
>>         const char *n;
>>         const char *p;
>>         const struct clk_pll_layout *l;
>> +       u32 f;
> 
> Why not unsigned long?

I'll switch to unsigned long.

> 
>>         u8 t;
>> -       u8 c;
>>         u8 eid;
>>  } sama7g5_plls[][PLL_ID_MAX] = {
>>         [PLL_ID_CPU] = {
>> @@ -111,13 +111,13 @@ static const struct {
>>                   .p = "mainck",
>>                   .l = &pll_layout_frac,
>>                   .t = PLL_TYPE_FRAC,
>> -                 .c = 1, },
>> +                 .f = CLK_IS_CRITICAL, },
>>
>>                 { .n = "cpupll_divpmcck",
>>                   .p = "cpupll_fracck",
>>                   .l = &pll_layout_divpmc,
>>                   .t = PLL_TYPE_DIV,
>> -                 .c = 1,
>> +                 .f = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT,
>>                   .eid = PMC_CPUPLL, },
>>         },
>>
>> @@ -126,13 +126,13 @@ static const struct {
>>                   .p = "mainck",
>>                   .l = &pll_layout_frac,
>>                   .t = PLL_TYPE_FRAC,
>> -                 .c = 1, },
>> +                 .f = CLK_IS_CRITICAL | CLK_SET_RATE_GATE, },
>>
>>                 { .n = "syspll_divpmcck",
>>                   .p = "syspll_fracck",
>>                   .l = &pll_layout_divpmc,
>>                   .t = PLL_TYPE_DIV,
>> -                 .c = 1,
>> +                 .f = CLK_IS_CRITICAL | CLK_SET_RATE_GATE,
> 
> Please indicate why clks are critical.

Sure! I'll do it in next version. I chose it like this because they are
feeding critical parts of the system like CPU or memory.

> Whenever the CLK_IS_CRITICAL flag
> is used we should have a comment indicating why.

I was not aware of this rule. I'll update the code accordingly.

Thank you,
Claudiu Beznea

> 
>>                   .eid = PMC_SYSPLL, },
>>         },
>>