Messages in this thread | | | From | Jerome Brunet <> | Subject | Re: [PATCH] clk: meson: g12a: fix gp0 and hifi ranges | Date | Thu, 29 Apr 2021 11:45:38 +0200 |
| |
On Thu 29 Apr 2021 at 11:20, Neil Armstrong <narmstrong@baylibre.com> wrote:
> On 29/04/2021 11:03, Jerome Brunet wrote: >> While some SoC samples are able to lock with a PLL factor of 55, others >> samples can't. ATM, a minimum of 60 appears to work on all the samples >> I have tried. >> >> Even with 60, it sometimes takes a long time for the PLL to eventually >> lock. The documentation says that the minimum rate of these PLLs DCO >> should be 3GHz, a factor of 125. Let's use that to be on the safe side. >> >> With factor range changed, the PLL seems to lock quickly (enough) so far. >> It is still unclear if the range was the only reason for the delay. >> >> Fixes: 085a4ea93d54 ("clk: meson: g12a: add peripheral clock controller") >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> >> --- >> drivers/clk/meson/g12a.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c >> index b080359b4645..a805bac93c11 100644 >> --- a/drivers/clk/meson/g12a.c >> +++ b/drivers/clk/meson/g12a.c >> @@ -1603,7 +1603,7 @@ static struct clk_regmap g12b_cpub_clk_trace = { >> }; >> >> static const struct pll_mult_range g12a_gp0_pll_mult_range = { >> - .min = 55, >> + .min = 125, >> .max = 255, >> }; >> >> > > I got other issues with GP0 when trying to use it for DSI on VIM3 & VIM3L. > > I had to do change the following to have it lock correctly and achieve rates usable for MIPI-DSI requested bandwidth: > > diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c > index cde07f7ebad6..897cd6db5c0f 100644 > --- a/drivers/clk/meson/clk-pll.c > +++ b/drivers/clk/meson/clk-pll.c > @@ -391,9 +391,9 @@ static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate, > meson_parm_write(clk->map, &pll->frac, frac); > } > > - /* If the pll is stopped, bail out now */ > + /* If the pll is stopped, bail out now * / > if (!enabled) > - return 0; > + return 0;*/
This enables the PLL everytime set_rate() is called :/
> > if (meson_clk_pll_enable(hw)) { > pr_warn("%s: pll did not lock, trying to restore old rate %lu\n", > > This one is tricky, for DSI the clock rate is set with assigned-clock-rates in DT, but > then the GP0 is seen as stopped and then the rate is never set.
Audio does the same - PLL is set and enabled afterward. This has been working so far.
> > When afterwards we enable the PLL, the rate set in the registers is invalid and never locks, > this permits setting the rate in the registers even if the PLL is > stopped.
There something to be explained here cause the register have been set before bailing out. What is happening ? The pokes before have no effect or are the value being reset at another point ?
I understand this need to address this concern but it does not seems related to this particular patch.
> > diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c > index 1b0167b8de3b..08174724a115 100644 > --- a/drivers/clk/meson/g12a.c > +++ b/drivers/clk/meson/g12a.c > @@ -1602,8 +1602,8 @@ static struct clk_regmap g12b_cpub_clk_trace = { > }; > > static const struct pll_mult_range g12a_gp0_pll_mult_range = { > - .min = 55, > - .max = 255, > + .min = 120, > + .max = 168, > }; > > I had to change the min/max to achieve a stable and functional rate of 720MHz after the ODs. >
How about the range provided in here ? This is range documented by AML (3GHz < DCO < 6GHz). 168 limits would limit the rate to ~4GHz which is way below spec and would negatively impact audio clocks.
> Neil
| |