diff mbox series

clk: qcom: dispcc-x1e80100: Drop the reconfiguring of PLL0 on probe

Message ID 20240418-x1e80100-dispcc-drop-pll0-reconfigure-v1-1-453e4e70e940@linaro.org (mailing list archive)
State Changes Requested
Headers show
Series clk: qcom: dispcc-x1e80100: Drop the reconfiguring of PLL0 on probe | expand

Commit Message

Abel Vesa April 18, 2024, 10:51 a.m. UTC 
Currently, PLL0 is configured by the bootloader is the parent of the
mdp_clk_src. Reconfiguring it on probe leaves the PLL0 in "stand-by"
state (unlocked), which will trigger RCG child clocks to not update
their config, which then breaks eDP on all x1e80100 boards. So rely
on the bootloader for now. Drop the config values as well. Also add
a comment to explain why the PLL0 is not configured alongside PLL1.

Fixes: ee3f0739035f ("clk: qcom: Add dispcc clock driver for x1e80100")
Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
 drivers/clk/qcom/dispcc-x1e80100.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)


---
base-commit: 4eab358930711bbeb85bf5ee267d0d42d3394c2c
change-id: 20240418-x1e80100-dispcc-drop-pll0-reconfigure-0111b338c838

Best regards,

Comments

Konrad Dybcio April 18, 2024, 11:33 a.m. UTC | #1 
On 18.04.2024 12:51 PM, Abel Vesa wrote:
> Currently, PLL0 is configured by the bootloader is the parent of the
> mdp_clk_src. Reconfiguring it on probe leaves the PLL0 in "stand-by"
> state (unlocked), which will trigger RCG child clocks to not update
> their config,

Sounds like this is the problem that should be fixed instead.

 which then breaks eDP on all x1e80100 boards. So rely
> on the bootloader for now. Drop the config values as well. Also add
> a comment to explain why the PLL0 is not configured alongside PLL1.
> 
> Fixes: ee3f0739035f ("clk: qcom: Add dispcc clock driver for x1e80100")
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---

This works, because you have (at least) partially configured hardware, but
we shouldn't assume this to be the case.

Konrad
Abel Vesa April 18, 2024, 12:08 p.m. UTC | #2 
On 24-04-18 13:33:19, Konrad Dybcio wrote:
> On 18.04.2024 12:51 PM, Abel Vesa wrote:
> > Currently, PLL0 is configured by the bootloader is the parent of the
> > mdp_clk_src. Reconfiguring it on probe leaves the PLL0 in "stand-by"
> > state (unlocked), which will trigger RCG child clocks to not update
> > their config,
> 
> Sounds like this is the problem that should be fixed instead.
> 
>  which then breaks eDP on all x1e80100 boards. So rely
> > on the bootloader for now. Drop the config values as well. Also add
> > a comment to explain why the PLL0 is not configured alongside PLL1.
> > 
> > Fixes: ee3f0739035f ("clk: qcom: Add dispcc clock driver for x1e80100")
> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > ---
> 
> This works, because you have (at least) partially configured hardware, but
> we shouldn't assume this to be the case.

OK, I think we should be safe to follow trion's approach instead then.

https://lore.kernel.org/all/20211123162508.153711-1-bjorn.andersson@linaro.org/

> 
> Konrad
Abel Vesa April 18, 2024, 1:52 p.m. UTC | #3 
On 24-04-18 13:51:22, Abel Vesa wrote:
> Currently, PLL0 is configured by the bootloader is the parent of the
> mdp_clk_src. Reconfiguring it on probe leaves the PLL0 in "stand-by"
> state (unlocked), which will trigger RCG child clocks to not update
> their config, which then breaks eDP on all x1e80100 boards. So rely
> on the bootloader for now. Drop the config values as well. Also add
> a comment to explain why the PLL0 is not configured alongside PLL1.
> 
> Fixes: ee3f0739035f ("clk: qcom: Add dispcc clock driver for x1e80100")
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---

Ignore this one. Sent a separate patch that handles this in another way:
https://lore.kernel.org/all/20240418-clk-qcom-lucid-evo-skip-configuring-enabled-v1-1-caede5f1c7a3@linaro.org/

>  drivers/clk/qcom/dispcc-x1e80100.c | 16 +---------------
>  1 file changed, 1 insertion(+), 15 deletions(-)
> 
> diff --git a/drivers/clk/qcom/dispcc-x1e80100.c b/drivers/clk/qcom/dispcc-x1e80100.c
> index 0b2ee6456762..525f645094a8 100644
> --- a/drivers/clk/qcom/dispcc-x1e80100.c
> +++ b/drivers/clk/qcom/dispcc-x1e80100.c
> @@ -73,20 +73,6 @@ static const struct pll_vco lucid_ole_vco[] = {
>  	{ 249600000, 2300000000, 0 },
>  };
>  
> -static const struct alpha_pll_config disp_cc_pll0_config = {
> -	.l = 0xd,
> -	.alpha = 0x6492,
> -	.config_ctl_val = 0x20485699,
> -	.config_ctl_hi_val = 0x00182261,
> -	.config_ctl_hi1_val = 0x82aa299c,
> -	.test_ctl_val = 0x00000000,
> -	.test_ctl_hi_val = 0x00000003,
> -	.test_ctl_hi1_val = 0x00009000,
> -	.test_ctl_hi2_val = 0x00000034,
> -	.user_ctl_val = 0x00000000,
> -	.user_ctl_hi_val = 0x00000005,
> -};
> -
>  static struct clk_alpha_pll disp_cc_pll0 = {
>  	.offset = 0x0,
>  	.vco_table = lucid_ole_vco,
> @@ -1670,7 +1656,7 @@ static int disp_cc_x1e80100_probe(struct platform_device *pdev)
>  		goto err_put_rpm;
>  	}
>  
> -	clk_lucid_evo_pll_configure(&disp_cc_pll0, regmap, &disp_cc_pll0_config);
> +	/* Configure only PLL1. PLL0 is already configured by bootloader */
>  	clk_lucid_evo_pll_configure(&disp_cc_pll1, regmap, &disp_cc_pll1_config);
>  
>  	/* Enable clock gating for MDP clocks */
> 
> ---
> base-commit: 4eab358930711bbeb85bf5ee267d0d42d3394c2c
> change-id: 20240418-x1e80100-dispcc-drop-pll0-reconfigure-0111b338c838
> 
> Best regards,
> -- 
> Abel Vesa <abel.vesa@linaro.org>
>
diff mbox series

Patch

diff --git a/drivers/clk/qcom/dispcc-x1e80100.c b/drivers/clk/qcom/dispcc-x1e80100.c
index 0b2ee6456762..525f645094a8 100644
--- a/drivers/clk/qcom/dispcc-x1e80100.c
+++ b/drivers/clk/qcom/dispcc-x1e80100.c
@@ -73,20 +73,6 @@  static const struct pll_vco lucid_ole_vco[] = {
 	{ 249600000, 2300000000, 0 },
 };
 
-static const struct alpha_pll_config disp_cc_pll0_config = {
-	.l = 0xd,
-	.alpha = 0x6492,
-	.config_ctl_val = 0x20485699,
-	.config_ctl_hi_val = 0x00182261,
-	.config_ctl_hi1_val = 0x82aa299c,
-	.test_ctl_val = 0x00000000,
-	.test_ctl_hi_val = 0x00000003,
-	.test_ctl_hi1_val = 0x00009000,
-	.test_ctl_hi2_val = 0x00000034,
-	.user_ctl_val = 0x00000000,
-	.user_ctl_hi_val = 0x00000005,
-};
-
 static struct clk_alpha_pll disp_cc_pll0 = {
 	.offset = 0x0,
 	.vco_table = lucid_ole_vco,
@@ -1670,7 +1656,7 @@  static int disp_cc_x1e80100_probe(struct platform_device *pdev)
 		goto err_put_rpm;
 	}
 
-	clk_lucid_evo_pll_configure(&disp_cc_pll0, regmap, &disp_cc_pll0_config);
+	/* Configure only PLL1. PLL0 is already configured by bootloader */
 	clk_lucid_evo_pll_configure(&disp_cc_pll1, regmap, &disp_cc_pll1_config);
 
 	/* Enable clock gating for MDP clocks */