Re: [PATCH] clk: meson: g12a: fix gp0 and hifi ranges
From: Jerome Brunet
Date: Thu Apr 29 2021 - 08:16:55 EST
On Thu 29 Apr 2021 at 14:15, Neil Armstrong <narmstrong@xxxxxxxxxxxx> wrote:
> On 29/04/2021 11:45, Jerome Brunet wrote:
>>
>> On Thu 29 Apr 2021 at 11:20, Neil Armstrong <narmstrong@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
>>>> ---
>>>> 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
>>