[PATCH] clk: qcom: msm8916: Fix the address location of pll->config_reg

[PATCH] clk: qcom: msm8916: Fix the address location of pll->config_reg

[Bryan O'Donoghue]

During the process of debugging a processor derived from the msm8916 which
we found the new processor was not starting one of its PLLs.

After tracing the addresses and writes that downstream was doing and
comparing to upstream it became obvious that we were writing to a different
register location than downstream when trying to configure the PLL.

This error is also present in upstream msm8916.

As an example clk-pll.c::clk_pll_recalc_rate wants to write to
pll->config_reg updating the bit-field POST_DIV_RATIO. That bit-field is
defined in PLL_USER_CTL not in PLL_CONFIG_CTL. Taking the BIMC PLL as an
example

lm80-p0436-13_c_qc_snapdragon_410_processor_hrd.pdf

0x01823010 GCC_BIMC_PLL_USER_CTL
0x01823014 GCC_BIMC_PLL_CONFIG_CTL

This pattern is repeated for gpll0, gpll1, gpll2 and bimc_pll.

This error is likely not apparent since the bootloader will already have
initialized these PLLs.

This patch corrects the location of config_reg from PLL_CONFIG_CTL to
PLL_USER_CTL for all relevant PLLs on msm8916.

Fixes commit 3966fab8b6ab ("clk: qcom: Add MSM8916 Global Clock Controller support")

Cc: Georgi Djakov <georgi.djakov@linaro.org>
Cc: Andy Gross <agross@kernel.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/clk/qcom/gcc-msm8916.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/qcom/gcc-msm8916.c b/drivers/clk/qcom/gcc-msm8916.c
index 4e329a7baf2b..17e4a5a2a9fd 100644
--- a/drivers/clk/qcom/gcc-msm8916.c
+++ b/drivers/clk/qcom/gcc-msm8916.c
@@ -260,7 +260,7 @@ static struct clk_pll gpll0 = {
 	.l_reg = 0x21004,
 	.m_reg = 0x21008,
 	.n_reg = 0x2100c,
-	.config_reg = 0x21014,
+	.config_reg = 0x21010,
 	.mode_reg = 0x21000,
 	.status_reg = 0x2101c,
 	.status_bit = 17,
@@ -287,7 +287,7 @@ static struct clk_pll gpll1 = {
 	.l_reg = 0x20004,
 	.m_reg = 0x20008,
 	.n_reg = 0x2000c,
-	.config_reg = 0x20014,
+	.config_reg = 0x20010,
 	.mode_reg = 0x20000,
 	.status_reg = 0x2001c,
 	.status_bit = 17,
@@ -314,7 +314,7 @@ static struct clk_pll gpll2 = {
 	.l_reg = 0x4a004,
 	.m_reg = 0x4a008,
 	.n_reg = 0x4a00c,
-	.config_reg = 0x4a014,
+	.config_reg = 0x4a010,
 	.mode_reg = 0x4a000,
 	.status_reg = 0x4a01c,
 	.status_bit = 17,
@@ -341,7 +341,7 @@ static struct clk_pll bimc_pll = {
 	.l_reg = 0x23004,
 	.m_reg = 0x23008,
 	.n_reg = 0x2300c,
-	.config_reg = 0x23014,
+	.config_reg = 0x23010,
 	.mode_reg = 0x23000,
 	.status_reg = 0x2301c,
 	.status_bit = 17,
-- 
2.25.1

Re: [PATCH] clk: qcom: msm8916: Fix the address location of pll->config_reg

[Bjorn Andersson]

On Sun 29 Mar 05:41 PDT 2020, Bryan O'Donoghue wrote:

> During the process of debugging a processor derived from the msm8916 which
> we found the new processor was not starting one of its PLLs.
> 
> After tracing the addresses and writes that downstream was doing and
> comparing to upstream it became obvious that we were writing to a different
> register location than downstream when trying to configure the PLL.
> 

Good catch.

> This error is also present in upstream msm8916.
> 
> As an example clk-pll.c::clk_pll_recalc_rate wants to write to
> pll->config_reg updating the bit-field POST_DIV_RATIO. That bit-field is
> defined in PLL_USER_CTL not in PLL_CONFIG_CTL. Taking the BIMC PLL as an
> example
> 

For some reason we don't specify pll->post_div_width for anything but
ipq806x, so the post_div is not considered for other platforms. This
might be a bug, but in addition to updating the config_reg address
post_div_width would have to be specified for the change to affect
clk_pll_recalc_rate().

More disturbing though is the clk_pll_set_rate() implementation, which
just writes ibits to the entire config_reg. But given that we don't have
a freq_tbl for any of these plls the function will return -EINVAL
earlier.

Lastly is clk_pll_configure() which would need this, but we don't call
it from msm8916 at this point.


So while your change is correct, afaict it's a nop unless you fill out
the other fields as well.

Regards,
Bjorn

> lm80-p0436-13_c_qc_snapdragon_410_processor_hrd.pdf
> 
> 0x01823010 GCC_BIMC_PLL_USER_CTL
> 0x01823014 GCC_BIMC_PLL_CONFIG_CTL
> 
> This pattern is repeated for gpll0, gpll1, gpll2 and bimc_pll.
> 
> This error is likely not apparent since the bootloader will already have
> initialized these PLLs.
> 
> This patch corrects the location of config_reg from PLL_CONFIG_CTL to
> PLL_USER_CTL for all relevant PLLs on msm8916.
> 
> Fixes commit 3966fab8b6ab ("clk: qcom: Add MSM8916 Global Clock Controller support")
> 
> Cc: Georgi Djakov <georgi.djakov@linaro.org>
> Cc: Andy Gross <agross@kernel.org>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/clk/qcom/gcc-msm8916.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gcc-msm8916.c b/drivers/clk/qcom/gcc-msm8916.c
> index 4e329a7baf2b..17e4a5a2a9fd 100644
> --- a/drivers/clk/qcom/gcc-msm8916.c
> +++ b/drivers/clk/qcom/gcc-msm8916.c
> @@ -260,7 +260,7 @@ static struct clk_pll gpll0 = {
>  	.l_reg = 0x21004,
>  	.m_reg = 0x21008,
>  	.n_reg = 0x2100c,
> -	.config_reg = 0x21014,
> +	.config_reg = 0x21010,
>  	.mode_reg = 0x21000,
>  	.status_reg = 0x2101c,
>  	.status_bit = 17,
> @@ -287,7 +287,7 @@ static struct clk_pll gpll1 = {
>  	.l_reg = 0x20004,
>  	.m_reg = 0x20008,
>  	.n_reg = 0x2000c,
> -	.config_reg = 0x20014,
> +	.config_reg = 0x20010,
>  	.mode_reg = 0x20000,
>  	.status_reg = 0x2001c,
>  	.status_bit = 17,
> @@ -314,7 +314,7 @@ static struct clk_pll gpll2 = {
>  	.l_reg = 0x4a004,
>  	.m_reg = 0x4a008,
>  	.n_reg = 0x4a00c,
> -	.config_reg = 0x4a014,
> +	.config_reg = 0x4a010,
>  	.mode_reg = 0x4a000,
>  	.status_reg = 0x4a01c,
>  	.status_bit = 17,
> @@ -341,7 +341,7 @@ static struct clk_pll bimc_pll = {
>  	.l_reg = 0x23004,
>  	.m_reg = 0x23008,
>  	.n_reg = 0x2300c,
> -	.config_reg = 0x23014,
> +	.config_reg = 0x23010,
>  	.mode_reg = 0x23000,
>  	.status_reg = 0x2301c,
>  	.status_bit = 17,
> -- 
> 2.25.1
> 

Re: [PATCH] clk: qcom: msm8916: Fix the address location of pll->config_reg

[Bryan O'Donoghue]

On 30/03/2020 21:41, Bjorn Andersson wrote:
> So while your change is correct, afaict it's a nop unless you fill out
> the other fields as well.

Yes it shouldn't be a problem.

We cloned the 8196 driver and then started adding bits. In our case we 
need to start PLL3 and PLL4 from Linux, similar to line 3452 here

https://source.codeaurora.org/quic/la/kernel/msm-3.10/tree/drivers/clk/qcom/clock-gcc-8936.c?h=APSS.FSM.3.0

which does end up calling into the PLL recalc logic.

---
bod

Re: [PATCH] clk: qcom: msm8916: Fix the address location of pll->config_reg

[Stephen Boyd]

Quoting Bryan O'Donoghue (2020-03-29 05:41:16)
> During the process of debugging a processor derived from the msm8916 which
> we found the new processor was not starting one of its PLLs.
> 
> After tracing the addresses and writes that downstream was doing and
> comparing to upstream it became obvious that we were writing to a different
> register location than downstream when trying to configure the PLL.
> 
> This error is also present in upstream msm8916.
> 
> As an example clk-pll.c::clk_pll_recalc_rate wants to write to
> pll->config_reg updating the bit-field POST_DIV_RATIO. That bit-field is
> defined in PLL_USER_CTL not in PLL_CONFIG_CTL. Taking the BIMC PLL as an
> example
> 
> lm80-p0436-13_c_qc_snapdragon_410_processor_hrd.pdf
> 
> 0x01823010 GCC_BIMC_PLL_USER_CTL
> 0x01823014 GCC_BIMC_PLL_CONFIG_CTL
> 
> This pattern is repeated for gpll0, gpll1, gpll2 and bimc_pll.
> 
> This error is likely not apparent since the bootloader will already have
> initialized these PLLs.
> 
> This patch corrects the location of config_reg from PLL_CONFIG_CTL to
> PLL_USER_CTL for all relevant PLLs on msm8916.
> 
> Fixes commit 3966fab8b6ab ("clk: qcom: Add MSM8916 Global Clock Controller support")
> 
> Cc: Georgi Djakov <georgi.djakov@linaro.org>
> Cc: Andy Gross <agross@kernel.org>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---

Applied to clk-next