Messages in this thread | | | Subject | Re: [PATCH] clk: meson: g12a: fix gp0 and hifi ranges | From | Neil Armstrong <> | Date | Thu, 29 Apr 2021 14:15:00 +0200 |
| |
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 ?
>> Neil >
| |