| Message ID | 1383093707-10312-1-git-send-email-pdeschrijver@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/29/2013 06:41 PM, Peter De Schrijver wrote: > Use pll_ref instead of pll_re_vco as the pll_e parent on Tegra114 and > Tegra124. Also add a pll_ref table entry for pll_e for Tegra114. Why? What benefit does this give, or what bug does this fix? > diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c > val_aux = pll_readl(pll_params->aux_reg, pll); > > if (val & PLL_BASE_ENABLE) { > - if (!(val_aux & PLLE_AUX_PLLRE_SEL)) > + if ((val_aux & PLLE_AUX_PLLRE_SEL) || (val_aux & val_aux)) Isn't "|| (val_aux & val_aux)" always true, at least if the value is non-zero? Either this should be simply "|| val_aux", or one of those two "val_aux" is the wrong thing. > WARN(1, "pll_e enabled with unsupported parent %s\n", > - (val & PLLE_AUX_PLLP_SEL) ? "pllp_out0" : "pll_ref"); > + (val_aux & PLLE_AUX_PLLP_SEL) ? "pllp_out0" : > + "pll_re_vco"); > } else { > - val_aux |= PLLE_AUX_PLLRE_SEL; > + val_aux &= ~(PLLE_AUX_PLLRE_SEL | PLLE_AUX_PLLP_SEL); > pll_writel(val, pll_params->aux_reg, pll); > } > diff --git a/drivers/clk/tegra/clk-tegra114.c b/drivers/clk/tegra/clk-tegra114.c > @@ -560,6 +560,7 @@ static struct tegra_clk_pll_freq_table pll_e_freq_table[] = { > /* PLLE special case: use cpcon field to store cml divider value */ > {336000000, 100000000, 100, 21, 16, 11}, > {312000000, 100000000, 200, 26, 24, 13}, > + {12000000, 100000000, 200, 1, 24, 13}, Presumably this is because pll_ref is the crystal, which runs at 12MHz. What if it doesn't; Tegra supports a bunch of other crystal rates. Don't we need entries for all the other potential crystal rates too?
Am Mittwoch, den 30.10.2013, 09:41 -0600 schrieb Stephen Warren: > On 10/29/2013 06:41 PM, Peter De Schrijver wrote: > > Use pll_ref instead of pll_re_vco as the pll_e parent on Tegra114 and > > Tegra124. Also add a pll_ref table entry for pll_e for Tegra114. > > Why? What benefit does this give, or what bug does this fix? > > > diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c > > > val_aux = pll_readl(pll_params->aux_reg, pll); > > > > if (val & PLL_BASE_ENABLE) { > > - if (!(val_aux & PLLE_AUX_PLLRE_SEL)) > > + if ((val_aux & PLLE_AUX_PLLRE_SEL) || (val_aux & val_aux)) > > Isn't "|| (val_aux & val_aux)" always true, at least if the value is > non-zero? Either this should be simply "|| val_aux", or one of those two > "val_aux" is the wrong thing. > > > WARN(1, "pll_e enabled with unsupported parent %s\n", > > - (val & PLLE_AUX_PLLP_SEL) ? "pllp_out0" : "pll_ref"); > > + (val_aux & PLLE_AUX_PLLP_SEL) ? "pllp_out0" : > > + "pll_re_vco"); > > } else { > > - val_aux |= PLLE_AUX_PLLRE_SEL; > > + val_aux &= ~(PLLE_AUX_PLLRE_SEL | PLLE_AUX_PLLP_SEL); > > pll_writel(val, pll_params->aux_reg, pll); > > } > > > diff --git a/drivers/clk/tegra/clk-tegra114.c b/drivers/clk/tegra/clk-tegra114.c > > > @@ -560,6 +560,7 @@ static struct tegra_clk_pll_freq_table pll_e_freq_table[] = { > > /* PLLE special case: use cpcon field to store cml divider value */ > > {336000000, 100000000, 100, 21, 16, 11}, > > {312000000, 100000000, 200, 26, 24, 13}, > > + {12000000, 100000000, 200, 1, 24, 13}, > > Presumably this is because pll_ref is the crystal, which runs at 12MHz. > What if it doesn't; Tegra supports a bunch of other crystal rates. Don't > we need entries for all the other potential crystal rates too? The TRM states that PCIe and thus PLLE are only supported with 12MHz external crystal rate.
On Wed, Oct 30, 2013 at 04:41:41PM +0100, Stephen Warren wrote: > On 10/29/2013 06:41 PM, Peter De Schrijver wrote: > > Use pll_ref instead of pll_re_vco as the pll_e parent on Tegra114 and > > Tegra124. Also add a pll_ref table entry for pll_e for Tegra114. > > Why? What benefit does this give, or what bug does this fix? > Otherrwise Tegra114 will crash on boot. > > diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c > > > val_aux = pll_readl(pll_params->aux_reg, pll); > > > > if (val & PLL_BASE_ENABLE) { > > - if (!(val_aux & PLLE_AUX_PLLRE_SEL)) > > + if ((val_aux & PLLE_AUX_PLLRE_SEL) || (val_aux & val_aux)) > > Isn't "|| (val_aux & val_aux)" always true, at least if the value is > non-zero? Either this should be simply "|| val_aux", or one of those two > "val_aux" is the wrong thing. > It should have been val_aux & PLLE_AUX_PLLP_SEL... Cheers, Peter.
On Wed, Oct 30, 2013 at 04:44:10PM +0100, Lucas Stach wrote: > Am Mittwoch, den 30.10.2013, 09:41 -0600 schrieb Stephen Warren: > > On 10/29/2013 06:41 PM, Peter De Schrijver wrote: > > > Use pll_ref instead of pll_re_vco as the pll_e parent on Tegra114 and > > > Tegra124. Also add a pll_ref table entry for pll_e for Tegra114. > > > > Why? What benefit does this give, or what bug does this fix? > > > > > diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c > > > > > val_aux = pll_readl(pll_params->aux_reg, pll); > > > > > > if (val & PLL_BASE_ENABLE) { > > > - if (!(val_aux & PLLE_AUX_PLLRE_SEL)) > > > + if ((val_aux & PLLE_AUX_PLLRE_SEL) || (val_aux & val_aux)) > > > > Isn't "|| (val_aux & val_aux)" always true, at least if the value is > > non-zero? Either this should be simply "|| val_aux", or one of those two > > "val_aux" is the wrong thing. > > > > > WARN(1, "pll_e enabled with unsupported parent %s\n", > > > - (val & PLLE_AUX_PLLP_SEL) ? "pllp_out0" : "pll_ref"); > > > + (val_aux & PLLE_AUX_PLLP_SEL) ? "pllp_out0" : > > > + "pll_re_vco"); > > > } else { > > > - val_aux |= PLLE_AUX_PLLRE_SEL; > > > + val_aux &= ~(PLLE_AUX_PLLRE_SEL | PLLE_AUX_PLLP_SEL); > > > pll_writel(val, pll_params->aux_reg, pll); > > > } > > > > > diff --git a/drivers/clk/tegra/clk-tegra114.c b/drivers/clk/tegra/clk-tegra114.c > > > > > @@ -560,6 +560,7 @@ static struct tegra_clk_pll_freq_table pll_e_freq_table[] = { > > > /* PLLE special case: use cpcon field to store cml divider value */ > > > {336000000, 100000000, 100, 21, 16, 11}, > > > {312000000, 100000000, 200, 26, 24, 13}, > > > + {12000000, 100000000, 200, 1, 24, 13}, > > > > Presumably this is because pll_ref is the crystal, which runs at 12MHz. > > What if it doesn't; Tegra supports a bunch of other crystal rates. Don't > > we need entries for all the other potential crystal rates too? > > The TRM states that PCIe and thus PLLE are only supported with 12MHz > external crystal rate. > This is has been different for Tegra114 at least (where PLLE is used for USB3) Cheers, Peter.
On 10/30/2013 04:18 PM, Peter De Schrijver wrote: > On Wed, Oct 30, 2013 at 04:41:41PM +0100, Stephen Warren wrote: >> On 10/29/2013 06:41 PM, Peter De Schrijver wrote: >>> Use pll_ref instead of pll_re_vco as the pll_e parent on Tegra114 and >>> Tegra124. Also add a pll_ref table entry for pll_e for Tegra114. >> >> Why? What benefit does this give, or what bug does this fix? > > Otherrwise Tegra114 will crash on boot. Sigh. For what reason?
On Wed, Oct 30, 2013 at 11:50:03PM +0100, Stephen Warren wrote: > On 10/30/2013 04:18 PM, Peter De Schrijver wrote: > > On Wed, Oct 30, 2013 at 04:41:41PM +0100, Stephen Warren wrote: > >> On 10/29/2013 06:41 PM, Peter De Schrijver wrote: > >>> Use pll_ref instead of pll_re_vco as the pll_e parent on Tegra114 and > >>> Tegra124. Also add a pll_ref table entry for pll_e for Tegra114. > >> > >> Why? What benefit does this give, or what bug does this fix? > > > > Otherrwise Tegra114 will crash on boot. > > Sigh. For what reason? pll_re_vco having an unsupported rate of 300Mhz. My guess is that it depends on the bootloader if you will see this. I'm fairly sure I verified this on my dalmore in helsinki and it worked, but it failed on Paul's test setup. Cheers, Peter.
On 10/31/2013 09:41 AM, Peter De Schrijver wrote: > On Wed, Oct 30, 2013 at 11:50:03PM +0100, Stephen Warren wrote: >> On 10/30/2013 04:18 PM, Peter De Schrijver wrote: >>> On Wed, Oct 30, 2013 at 04:41:41PM +0100, Stephen Warren wrote: >>>> On 10/29/2013 06:41 PM, Peter De Schrijver wrote: >>>>> Use pll_ref instead of pll_re_vco as the pll_e parent on Tegra114 and >>>>> Tegra124. Also add a pll_ref table entry for pll_e for Tegra114. >>>> >>>> Why? What benefit does this give, or what bug does this fix? >>> >>> Otherrwise Tegra114 will crash on boot. >> >> Sigh. For what reason? > > pll_re_vco having an unsupported rate of 300Mhz. My guess is that it depends > on the bootloader if you will see this. I'm fairly sure I verified this on > my dalmore in helsinki and it worked, but it failed on Paul's test setup. OK, so this is primarily a SW issue, because pll_e's freq_table simply doesn't have an entry for input frequency 300MHz. Can you make sure the commit description explains that?
On Wed, Oct 30, 2013 at 01:41:29AM +0100, Peter De Schrijver wrote: > Use pll_ref instead of pll_re_vco as the pll_e parent on Tegra114 and > Tegra124. Also add a pll_ref table entry for pll_e for Tegra114. > > Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com> I will squash this into the next tegra-clk-next as the previous pull request has never made it. Cheers, Peter.
On Fri, Nov 22, 2013 at 02:40:35PM +0100, Peter De Schrijver wrote: > On Wed, Oct 30, 2013 at 01:41:29AM +0100, Peter De Schrijver wrote: > > Use pll_ref instead of pll_re_vco as the pll_e parent on Tegra114 and > > Tegra124. Also add a pll_ref table entry for pll_e for Tegra114. > > > > Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com> > > I will squash this into the next tegra-clk-next as the previous pull request > has never made it. > Looking again at this, I think the Tegra114 and generic PLL part of the patch better stays as a separate patch. The Tegra124 bit will be squashed into the Tegra124 support patch. Cheers, Peter.
diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c index 7d775a9..193457b 100644 --- a/drivers/clk/tegra/clk-pll.c +++ b/drivers/clk/tegra/clk-pll.c @@ -1712,11 +1712,12 @@ struct clk *tegra_clk_register_plle_tegra114(const char *name, val_aux = pll_readl(pll_params->aux_reg, pll); if (val & PLL_BASE_ENABLE) { - if (!(val_aux & PLLE_AUX_PLLRE_SEL)) + if ((val_aux & PLLE_AUX_PLLRE_SEL) || (val_aux & val_aux)) WARN(1, "pll_e enabled with unsupported parent %s\n", - (val & PLLE_AUX_PLLP_SEL) ? "pllp_out0" : "pll_ref"); + (val_aux & PLLE_AUX_PLLP_SEL) ? "pllp_out0" : + "pll_re_vco"); } else { - val_aux |= PLLE_AUX_PLLRE_SEL; + val_aux &= ~(PLLE_AUX_PLLRE_SEL | PLLE_AUX_PLLP_SEL); pll_writel(val, pll_params->aux_reg, pll); } diff --git a/drivers/clk/tegra/clk-tegra114.c b/drivers/clk/tegra/clk-tegra114.c index e365b35..5cfcd3e 100644 --- a/drivers/clk/tegra/clk-tegra114.c +++ b/drivers/clk/tegra/clk-tegra114.c @@ -560,6 +560,7 @@ static struct tegra_clk_pll_freq_table pll_e_freq_table[] = { /* PLLE special case: use cpcon field to store cml divider value */ {336000000, 100000000, 100, 21, 16, 11}, {312000000, 100000000, 200, 26, 24, 13}, + {12000000, 100000000, 200, 1, 24, 13}, {0, 0, 0, 0, 0, 0}, }; @@ -1178,7 +1179,7 @@ static void __init tegra114_pll_init(void __iomem *clk_base, clks[TEGRA114_CLK_PLL_RE_OUT] = clk; /* PLLE */ - clk = tegra_clk_register_plle_tegra114("pll_e_out0", "pll_re_vco", + clk = tegra_clk_register_plle_tegra114("pll_e_out0", "pll_ref", clk_base, 0, &pll_e_params, NULL); clks[TEGRA114_CLK_PLL_E_OUT0] = clk; }
Use pll_ref instead of pll_re_vco as the pll_e parent on Tegra114 and Tegra124. Also add a pll_ref table entry for pll_e for Tegra114. Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com> --- drivers/clk/tegra/clk-pll.c | 7 ++++--- drivers/clk/tegra/clk-tegra114.c | 3 ++- 2 files changed, 6 insertions(+), 4 deletions(-)