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

[Stephen Boyd]

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? I'm confused why this driver needs to check
this flag.
 
> +               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?

>         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. Whenever the CLK_IS_CRITICAL flag
is used we should have a comment indicating why.

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