[U-Boot] [PATCH] tegra: Correct logic for reading pll_misc in clock_start_pll()

[Simon Glass]

Hi Tom,

On 11 August 2015 at 11:41, Tom Warren <TWarren at nvidia.com> wrote:
> Simon,
>
>> -----Original Message-----
>> From: Simon Glass [mailto:sjg at google.com] On Behalf Of Simon Glass
>> Sent: Monday, August 10, 2015 6:15 AM
>> To: U-Boot Mailing List
>> Cc: Simon Glass; Tom Warren; Thierry Reding; Masahiro Yamada; Stephen
>> Warren; Tom Rini; Albert Aribaud; Marcel Ziswiler; Stephen Warren
>> Subject: [PATCH] tegra: Correct logic for reading pll_misc in clock_start_pll()
>>
>> The logic for simple PLLs on T124 was broken by this commit:
>>
>>   722e000c Tegra: PLL: use per-SoC pllinfo table instead of PLL_DIVM/N/P, etc.
>>
>> Correct it by reading from the same pll_misc register that it writes to and
>> adding an entry for the DP PLL.
>>
>> Signed-off-by: Simon Glass <sjg at chromium.org>
>> ---
>>
>>  arch/arm/mach-tegra/clock.c          | 33 +++++++++++++++++++++------------
>>  arch/arm/mach-tegra/tegra124/clock.c |  2 ++
>>  2 files changed, 23 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/arm/mach-tegra/clock.c b/arch/arm/mach-tegra/clock.c index
>> 3b2b4ff..f014434 100644
>> --- a/arch/arm/mach-tegra/clock.c
>> +++ b/arch/arm/mach-tegra/clock.c
>> @@ -126,19 +126,34 @@ unsigned long clock_start_pll(enum clock_id clkid, u32
>> divm, u32 divn,  {
>>       struct clk_pll *pll = NULL;
>>       struct clk_pll_info *pllinfo = &tegra_pll_info_table[clkid];
>> +     struct clk_pll_simple *simple_pll = NULL;
>>       u32 misc_data, data;
>>
>> -     if (clkid < (enum clock_id)TEGRA_CLK_PLLS)
>> +     if (clkid < (enum clock_id)TEGRA_CLK_PLLS) {
>>               pll = get_pll(clkid);
>> +     } else {
>> +             simple_pll = clock_get_simple_pll(clkid);
>> +             if (!simple_pll) {
>> +                     debug("%s: Uknown simple PLL %d\n", __func__,
>> clkid);
>> +                     return 0;
>> +             }
>> +     }
>>
>>       /*
>>        * pllinfo has the m/n/p and kcp/kvco mask and shift
>>        * values for all of the PLLs used in U-Boot, with any
>>        * SoC differences accounted for.
>> +      *
>> +      * Preserve EN_LOCKDET, etc.
>>        */
>> -     misc_data = readl(&pll->pll_misc);      /* preserve EN_LOCKDET, etc.
>> */
>> -     misc_data &= ~(pllinfo->kcp_mask << pllinfo->kcp_shift) | (cpcon <<
>> pllinfo->kcp_shift);
>> -     misc_data &= ~(pllinfo->kvco_mask << pllinfo->kvco_shift) | (lfcon <<
>> pllinfo->kvco_shift);
>> +     if (pll)
>> +             misc_data = readl(&pll->pll_misc);
>> +     else
>> +             misc_data = readl(&simple_pll->pll_misc);
>> +     misc_data &= ~(pllinfo->kcp_mask << pllinfo->kcp_shift);
>> +     misc_data |= cpcon << pllinfo->kcp_shift;
>> +     misc_data &= ~(pllinfo->kvco_mask << pllinfo->kvco_shift);
>> +     misc_data |= lfcon << pllinfo->kvco_shift;
>>
>>       data = (divm << pllinfo->m_shift) | (divn << pllinfo->n_shift);
>>       data |= divp << pllinfo->p_shift;
>> @@ -148,14 +163,8 @@ unsigned long clock_start_pll(enum clock_id clkid, u32
>> divm, u32 divn,
>>               writel(misc_data, &pll->pll_misc);
>>               writel(data, &pll->pll_base);
>>       } else {
>> -             struct clk_pll_simple *pll = clock_get_simple_pll(clkid);
>> -
>> -             if (!pll) {
>> -                     debug("%s: Uknown simple PLL %d\n", __func__,
>> clkid);
>> -                     return 0;
>> -             }
>> -             writel(misc_data, &pll->pll_misc);
>> -             writel(data, &pll->pll_base);
>> +             writel(misc_data, &simple_pll->pll_misc);
>> +             writel(data, &simple_pll->pll_base);
>>       }
>>
>>       /* calculate the stable time */
>> diff --git a/arch/arm/mach-tegra/tegra124/clock.c b/arch/arm/mach-
>> tegra/tegra124/clock.c
>> index 9126218..42000ae 100644
>> --- a/arch/arm/mach-tegra/tegra124/clock.c
>> +++ b/arch/arm/mach-tegra/tegra124/clock.c
>> @@ -593,6 +593,8 @@ struct clk_pll_info
>> tegra_pll_info_table[CLOCK_ID_PLL_COUNT] = {
>>         .lock_ena = 9,  .lock_det = 11, .kcp_shift = 6, .kcp_mask = 3,
>> .kvco_shift = 0, .kvco_mask = 1 },    /* PLLE */
>>       { .m_shift = 0, .m_mask = 0x0F, .n_shift = 8, .n_mask = 0x3FF, .p_shift =
>> 20, .p_mask = 0x07,
>>         .lock_ena = 18, .lock_det = 27, .kcp_shift = 8, .kcp_mask = 0xF,
>> .kvco_shift = 4, .kvco_mask = 0xF },  /* PLLS (RESERVED) */
>> +     { .m_shift = 0, .m_mask = 0x1F, .n_shift = 8, .n_mask = 0x3FF,  .p_shift
>> = 20,  .p_mask = 7,
>> +       .lock_ena = 18, .lock_det = 27, .kcp_shift = 8, .kcp_mask = 0xF,
>> .kvco_shift = 4, .kvco_mask = 0xF },  /* PLLDP */
>>  };
> I was looking at adding a PLLDP table entry for T210 based on your T124 table, and your settings don't match what I have in my T124 TRM.
>
> PLLDP_MDIV is 8 bits wide, starting at bit 0, so .m_shift = 0 but .m_mask s/b 0xFF.
> PLLDP_NDIV is 8 bits wide, starting at bit 8, so .n_shift = 8, but .n_mask s/b 0xFF.
> .lock_det is OK at bit 27, but .lock_ena s/b bit 30 (PLLDP_LOCK_ENABLE).
> PLLDP_KCP is 2 bits wide, starting at bit 25 with a mask of 0x3, so .kcp_shift s/b 25 and .kcp_mask s/b 3.
> PLLDP_KVCO is 1 bit wide, starting at bit 24 with a mask of 1, so .kvco_shift s/b 24 and .kvco_mask s/b 1.
>
> Not sure where your values came from - maybe a cut&paste error?

Actually I just copied the pre-existing defines as I was trying to
revert the behaviour. But please go ahead and update the change, since
what you have comes from the datasheet.

>
> Please take a look. I've got this patch in u-boot-tegra/next right now, but I can update it when you've confirmed my findings.
>
> Tom
> --
> nvpublic

Regards,
Simon