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

[Claudiu.Beznea]

On 18.11.2020 03:49, Stephen Boyd wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Quoting Claudiu.Beznea@xxxxxxxxxxxxx (2020-11-16 03:24:54)
>>
>>
>> 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.
> 
> Can we have different clk_ops for the different types of PLLs?

Sure! I'll switch to this way.

Thank you for your feedback,
Claudiu Beznea

> It looks
> like the flag is being used to overload this function to do different
> things depending on how the flags are set. What happens if we decide to
> change the semantics of this clk flag? How does it map to this driver?
> Ideally this driver shouldn't need to worry about this flag, at least
> not in this function, except to figure out if it should do something
> different like not write the value to the hardware or something like
> that.
> 
> The flag indicates to the clk framework that this clk should be gated
> when clk_set_rate() is called on it. The driver should be able to figure
> out that the clk is disabled by reading the hardware here and checking
> the enable state, or it could just have different clk_ops for the
> different type of PLL and do something different without checking the
> flag. Either way, checking the flag looks wrong.
> 
>>>> -                 .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.
> 
> Sorry. I should put a document comment next to the flag.
>