Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] clk: meson: g12a: fix gp0 and hifi ranges
To:     Jerome Brunet <jbrunet@baylibre.com>
Cc:     Kevin Hilman <khilman@baylibre.com>,
        linux-amlogic@lists.infradead.org, linux-clk@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20210429090325.60970-1-jbrunet@baylibre.com>
 <979eb290-6fc2-38df-0596-867b82d22226@baylibre.com>
 <1jim45juf1.fsf@starbuckisacylon.baylibre.com>
From:   Neil Armstrong <narmstrong@baylibre.com>
Organization: Baylibre
Message-ID: <0cfc7286-7be6-57ec-8040-a4370d0b4204@baylibre.com>
Date:   Thu, 29 Apr 2021 14:15:00 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.7.1
MIME-Version: 1.0
In-Reply-To: <1jim45juf1.fsf@starbuckisacylon.baylibre.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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
>