[PATCH] clk: meson: g12a: fix gp0 and hifi ranges

[PATCH] clk: meson: g12a: fix gp0 and hifi ranges

[Jerome Brunet]

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,
 };
 
-- 
2.31.1

Re: [PATCH] clk: meson: g12a: fix gp0 and hifi ranges

[Neil Armstrong]

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

Re: [PATCH] clk: meson: g12a: fix gp0 and hifi ranges

[Jerome Brunet]

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

Re: [PATCH] clk: meson: g12a: fix gp0 and hifi ranges

[Neil Armstrong]

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
> 

Re: [PATCH] clk: meson: g12a: fix gp0 and hifi ranges

[Jerome Brunet]

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
>> 

Re: [PATCH] clk: meson: g12a: fix gp0 and hifi ranges

[Neil Armstrong]

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

Re: [PATCH] clk: meson: g12a: fix gp0 and hifi ranges

[Jerome Brunet]

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

[PATCH] clk: meson: g12a: fix gp0 and hifi ranges

[Jerome Brunet]

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, so lets use this value until we have a good reason to put
something else.

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 0e1ce8c03259..d3f53a9b97dc 100644
--- a/drivers/clk/meson/g12a.c
+++ b/drivers/clk/meson/g12a.c
@@ -151,7 +151,7 @@ static struct clk_regmap g12a_sys_pll = {
 };
 
 static const struct pll_mult_range g12a_gp0_pll_mult_range = {
-	.min = 55,
+	.min = 60,
 	.max = 255,
 };
 
-- 
2.20.1

Re: [PATCH] clk: meson: g12a: fix gp0 and hifi ranges

[Neil Armstrong]

On 25/03/2019 11:42, 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, so lets use this value until we have a good reason to put
> something else.
> 
> 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 0e1ce8c03259..d3f53a9b97dc 100644
> --- a/drivers/clk/meson/g12a.c
> +++ b/drivers/clk/meson/g12a.c
> @@ -151,7 +151,7 @@ static struct clk_regmap g12a_sys_pll = {
>  };
>  
>  static const struct pll_mult_range g12a_gp0_pll_mult_range = {
> -	.min = 55,
> +	.min = 60,
>  	.max = 255,
>  };
>  
> 

Acked-by: Neil Armstrong <narmstrong@baylibre.com>

And applied to fixes/drivers !

Neil

Re: [PATCH] clk: meson: g12a: fix gp0 and hifi ranges

[Neil Armstrong]

On 25/03/2019 13:18, Neil Armstrong wrote:
> On 25/03/2019 11:42, 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, so lets use this value until we have a good reason to put
>> something else.
>>
>> 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 0e1ce8c03259..d3f53a9b97dc 100644
>> --- a/drivers/clk/meson/g12a.c
>> +++ b/drivers/clk/meson/g12a.c
>> @@ -151,7 +151,7 @@ static struct clk_regmap g12a_sys_pll = {
>>  };
>>  
>>  static const struct pll_mult_range g12a_gp0_pll_mult_range = {
>> -	.min = 55,
>> +	.min = 60,
>>  	.max = 255,
>>  };
>>  
>>
> 
> Acked-by: Neil Armstrong <narmstrong@baylibre.com>
> 
> And applied to fixes/drivers !
> 
> Neil
> 

Seems this will need more work,
unapplied from fixes/drivers

Neil