| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <77eb8a68-f5b5-357b-f71c-e22337d88a39@amlogic.com>
Date: Fri, 25 Oct 2019 14:47:36 +0800
From: Jian Hu <jian.hu@...ogic.com>
To: Jerome Brunet <jbrunet@...libre.com>,
Neil Armstrong <narmstrong@...libre.com>
CC: Kevin Hilman <khilman@...libre.com>, Rob Herring <robh@...nel.org>,
Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
Michael Turquette <mturquette@...libre.com>,
Stephen Boyd <sboyd@...nel.org>,
Qiufang Dai <qiufang.dai@...ogic.com>,
Jianxin Pan <jianxin.pan@...ogic.com>,
Victor Wan <victor.wan@...ogic.com>,
Chandle Zou <chandle.zou@...ogic.com>,
<linux-clk@...r.kernel.org>, <linux-amlogic@...ts.infradead.org>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>
Subject: Re: [PATCH v2 2/3] clk: meson: add support for A1 PLL clock ops
Hi, Jerome
On 2019/10/21 19:31, Jerome Brunet wrote:
>
> On Fri 18 Oct 2019 at 09:14, Jian Hu <jian.hu@...ogic.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@...ogic.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;
>
> .
>
Powered by blists - more mailing lists