Re: [PATCH v2 2/3] clk: meson: add support for A1 PLL clock ops

[Jian Hu <jian.hu@amlogic.com>]

Hi, Jerome

On 2019/10/21 19:31, Jerome Brunet wrote:
> 
> On Fri 18 Oct 2019 at 09:14, Jian Hu <jian.hu@amlogic.com> wrote:
> 
>> The A1 PLL design is different with previous SoCs. The PLL
>> internal analog modules Power-on sequence is different
>> with previous, and thus requires a strict register sequence to
>> enable the PLL. Unlike the previous series, the maximum frequency
>> is 6G in G12A, for A1 the maximum is 1536M.
>>
>> Signed-off-by: Jian Hu <jian.hu@amlogic.com>
>> ---
>>   drivers/clk/meson/clk-pll.c | 66 ++++++++++++++++++++++++++++++++++++++++-----
>>   drivers/clk/meson/clk-pll.h |  1 +
>>   2 files changed, 61 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
>> index ddb1e56..b440e62 100644
>> --- a/drivers/clk/meson/clk-pll.c
>> +++ b/drivers/clk/meson/clk-pll.c
>> @@ -349,6 +349,56 @@ static void meson_clk_pll_disable(struct clk_hw *hw)
>>   	meson_parm_write(clk->map, &pll->en, 0);
>>   }
>>   
>> +/*
>> + * The A1 design is different with previous SoCs.The PLL
>> + * internal analog modules Power-on sequence is different with
>> + * previous, different PLL has the different sequence, and
>> + * thus requires a strict register sequence to enable the PLL.
>> + * When set a new target frequency, the sequence should keep
>> + * the same with the initial sequence. Unlike the previous series,
>> + * the maximum frequency is 6G in G12A, for A1 the maximum
>> + * is 1536M.
> 
> The comment about the max frequency belongs in your a1 driver, not in
> the PLL driver
> 
OK, I will remove the max frequency comments
>> + */
>> +static void meson_params_update_with_init_seq(struct clk_regmap *clk,
>> +				       struct meson_clk_pll_data *pll,
>> +				       unsigned int m, unsigned int n,
>> +				       unsigned int frac)
>> +{
>> +	struct parm *pm = &pll->m;
>> +	struct parm *pn = &pll->n;
>> +	struct parm *pfrac = &pll->frac;
>> +	const struct reg_sequence *init_regs = pll->init_regs;
>> +	unsigned int i, val;
>> +
>> +	for (i = 0; i < pll->init_count; i++) {
>> +		if (pn->reg_off == init_regs[i].reg) {
>> +			/* Clear M N bits and Update M N value */
>> +			val = init_regs[i].def;
>> +			val &= CLRPMASK(pn->width, pn->shift);
>> +			val &= CLRPMASK(pm->width, pm->shift);
>> +			val |= n << pn->shift;
>> +			val |= m << pm->shift;
>> +			regmap_write(clk->map, pn->reg_off, val);
>> +		} else if (MESON_PARM_APPLICABLE(&pll->frac) &&
>> +			   (pfrac->reg_off == init_regs[i].reg)) {
>> +			/* Clear Frac bits and Update Frac value */
>> +			val = init_regs[i].def;
>> +			val &= CLRPMASK(pfrac->width, pfrac->shift);
>> +			val |= frac << pfrac->shift;
>> +			regmap_write(clk->map, pfrac->reg_off, val);
>> +		} else {
>> +			/*
>> +			 * According to the PLL hardware constraint,
>> +			 * the left registers should be setted again.
>> +			 */
>> +			val = init_regs[i].def;
>> +			regmap_write(clk->map, init_regs[i].reg, val);
>> +		}
>> +		if (init_regs[i].delay_us)
>> +			udelay(init_regs[i].delay_us);
>> +	}
> 
> So:
> 
> 1) All the code above this there make the PLL lock, IOW enable the
> PLL. It does not belong in the set_rate() callback but in enable() or
> prepare() maybe.
> 
> 2) All the above is works but it is a bit over complicated for what it
> does. From the a1_hifi_init_regs I see, all you really need to do is
>    * toggle BIT(6) in CTRL2
>    * toggle BIT(28) in CTRL0 (enable PARM)
>    * toggle BIT(26) in CTRL0
> 
> You could use PARM 'rst' for one them and introduce another parm for the
> other one. You would not need to repoke the whole sequence this way.
> 
OK, I have realized as you suggested. I will send it in the V3 patch.
>> +}
>> +
>>   static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>>   				  unsigned long parent_rate)
>>   {
>> @@ -366,16 +416,20 @@ static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>>   	if (ret)
>>   		return ret;
>>   
>> +	if (MESON_PARM_APPLICABLE(&pll->frac))
>> +		frac = __pll_params_with_frac(rate, parent_rate, m, n, pll);
>> +
>>   	enabled = meson_parm_read(clk->map, &pll->en);
>>   	if (enabled)
>>   		meson_clk_pll_disable(hw);
>>   
>> -	meson_parm_write(clk->map, &pll->n, n);
>> -	meson_parm_write(clk->map, &pll->m, m);
>> -
>> -	if (MESON_PARM_APPLICABLE(&pll->frac)) {
>> -		frac = __pll_params_with_frac(rate, parent_rate, m, n, pll);
>> -		meson_parm_write(clk->map, &pll->frac, frac);
>> +	if (pll->strict_sequence)
>> +		meson_params_update_with_init_seq(clk, pll, m, n, frac);
>> +	else {
>> +		meson_parm_write(clk->map, &pll->n, n);
>> +		meson_parm_write(clk->map, &pll->m, m);
>> +		if (MESON_PARM_APPLICABLE(&pll->frac))
>> +			meson_parm_write(clk->map, &pll->frac, frac);
>>   	}
>>   
>>   	/* If the pll is stopped, bail out now */
>> diff --git a/drivers/clk/meson/clk-pll.h b/drivers/clk/meson/clk-pll.h
>> index 367efd0..d5789cef 100644
>> --- a/drivers/clk/meson/clk-pll.h
>> +++ b/drivers/clk/meson/clk-pll.h
>> @@ -41,6 +41,7 @@ struct meson_clk_pll_data {
>>   	const struct pll_params_table *table;
>>   	const struct pll_mult_range *range;
>>   	u8 flags;
>> +	bool strict_sequence;
> 
> Don't introduce parameter for this We have ops to tune the behavior of
> the clock driver. Properly refactor the code if some of it is common.
> 
remove the strict_sequence.
>>   };
>>   
>>   extern const struct clk_ops meson_clk_pll_ro_ops;
> 
> .
> 

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic