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, },
> },
>
