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>
From:   Neil Armstrong <narmstrong@baylibre.com>
Organization: Baylibre
Message-ID: <979eb290-6fc2-38df-0596-867b82d22226@baylibre.com>
Date:   Thu, 29 Apr 2021 11:20: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: <20210429090325.60970-1-jbrunet@baylibre.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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