Messages in this thread | | | From | Jerome Brunet <> | Subject | Re: [PATCH] clk: meson: g12a: fix gp0 and hifi ranges | Date | Thu, 29 Apr 2021 14:16:49 +0200 |
| |
On Thu 29 Apr 2021 at 14:15, Neil Armstrong <narmstrong@baylibre.com> wrote:
> On 29/04/2021 11:45, Jerome Brunet wrote: >> >> 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. > > For hifi pll right, not for GP0 ? >
Both have the same spec.
>>> Neil >>
| |