| Message ID | 20210429090325.60970-1-jbrunet@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Neil Armstrong |
| Headers | show |
| Series | clk: meson: g12a: fix gp0 and hifi ranges | expand |
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;*/ 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. 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. 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. Neil
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
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 >
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 >>
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, > }; > > Sorry for bothering with the DSI stuff, I'll fix this when we are ready to upstream DSI support for G12A. I had this patch for a while in my integration branches, so: Acked-by: Neil Armstrong <narmstrong@baylibre.com> Neil
On Thu 20 May 2021 at 16:02, Neil Armstrong <narmstrong@baylibre.com> wrote: > On 29/04/2021 11:03, Jerome Brunet wrote: > Sorry for bothering with the DSI stuff, I'll fix this when we are ready to upstream DSI support for G12A. > > I had this patch for a while in my integration branches, so: > Acked-by: Neil Armstrong <narmstrong@baylibre.com> > > > Neil Applied
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, };
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(-)