diff mbox series

[v2] clk: qcom: apss-ipq-pll: use stromer ops for IPQ5018 to fix boot failure

Message ID 20240315-apss-ipq-pll-ipq5018-hang-v2-1-6fe30ada2009@gmail.com (mailing list archive)
State Awaiting Upstream, archived
Headers show
Series [v2] clk: qcom: apss-ipq-pll: use stromer ops for IPQ5018 to fix boot failure | expand

Commit Message

Gabor Juhos March 15, 2024, 4:16 p.m. UTC 
Booting v6.8 results in a hang on various IPQ5018 based boards.
Investigating the problem showed that the hang happens when the
clk_alpha_pll_stromer_plus_set_rate() function tries to write
into the PLL_MODE register of the APSS PLL.

Checking the downstream code revealed that it uses [1] stromer
specific operations for IPQ5018, whereas in the current code
the stromer plus specific operations are used.

The ops in the 'ipq_pll_stromer_plus' clock definition can't be
changed since that is needed for IPQ5332, so add a new alpha pll
clock declaration which uses the correct stromer ops and use this
new clock for IPQ5018 to avoid the boot failure.

Also, change pll_type in 'ipq5018_pll_data' to
CLK_ALPHA_PLL_TYPE_STROMER to better reflect that it is a Stromer
PLL and change the apss_ipq_pll_probe() function accordingly.

1. https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4/drivers/clk/qcom/apss-ipq5018.c#L67

Cc: stable@vger.kernel.org
Fixes: 50492f929486 ("clk: qcom: apss-ipq-pll: add support for IPQ5018")
Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
Changes in v2:
  - extend commit description due to the changes
  - add a comment about why CLK_ALPHA_PLL_TYPE_STROMER_PLUS register offsets
    are used
  - constify hw clock init data (Stephen)
  - change pll_type in ipq5018_pll_data to CLK_ALPHA_PLL_TYPE_STROMER (Konrad)
  - Link to v1: https://lore.kernel.org/r/20240311-apss-ipq-pll-ipq5018-hang-v1-1-8ed42b7a904d@gmail.com
---
Based on v6.8.
---
 drivers/clk/qcom/apss-ipq-pll.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)


---
base-commit: e8f897f4afef0031fe618a8e94127a0934896aba
change-id: 20240311-apss-ipq-pll-ipq5018-hang-74d9a8f47136

Best regards,

Comments

Kathiravan Thirumoorthy March 18, 2024, 5:39 a.m. UTC | #1 
On 3/15/2024 9:46 PM, Gabor Juhos wrote:
> Booting v6.8 results in a hang on various IPQ5018 based boards.
> Investigating the problem showed that the hang happens when the
> clk_alpha_pll_stromer_plus_set_rate() function tries to write
> into the PLL_MODE register of the APSS PLL.
> 
> Checking the downstream code revealed that it uses [1] stromer
> specific operations for IPQ5018, whereas in the current code
> the stromer plus specific operations are used.
> 
> The ops in the 'ipq_pll_stromer_plus' clock definition can't be
> changed since that is needed for IPQ5332, so add a new alpha pll
> clock declaration which uses the correct stromer ops and use this
> new clock for IPQ5018 to avoid the boot failure.
> 
> Also, change pll_type in 'ipq5018_pll_data' to
> CLK_ALPHA_PLL_TYPE_STROMER to better reflect that it is a Stromer
> PLL and change the apss_ipq_pll_probe() function accordingly.
> 
> 1. https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.4/drivers/clk/qcom/apss-ipq5018.c#L67
> 
> Cc: stable@vger.kernel.org
> Fixes: 50492f929486 ("clk: qcom: apss-ipq-pll: add support for IPQ5018")
> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> ---
> Changes in v2:
>    - extend commit description due to the changes
>    - add a comment about why CLK_ALPHA_PLL_TYPE_STROMER_PLUS register offsets
>      are used
>    - constify hw clock init data (Stephen)
>    - change pll_type in ipq5018_pll_data to CLK_ALPHA_PLL_TYPE_STROMER (Konrad)
>    - Link to v1: https://lore.kernel.org/r/20240311-apss-ipq-pll-ipq5018-hang-v1-1-8ed42b7a904d@gmail.com


I don't see a reason why my tags are dropped, nevertheless

Tested-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
Reviewed-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>


> ---
> Based on v6.8.
> ---
>   drivers/clk/qcom/apss-ipq-pll.c | 30 +++++++++++++++++++++++++++---
>   1 file changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c
> index 678b805f13d45..dfffec2f06ae7 100644
> --- a/drivers/clk/qcom/apss-ipq-pll.c
> +++ b/drivers/clk/qcom/apss-ipq-pll.c
> @@ -55,6 +55,29 @@ static struct clk_alpha_pll ipq_pll_huayra = {
>   	},
>   };
>   
> +static struct clk_alpha_pll ipq_pll_stromer = {
> +	.offset = 0x0,
> +	/*
> +	 * Reuse CLK_ALPHA_PLL_TYPE_STROMER_PLUS register offsets.
> +	 * Although this is a bit confusing, but the offset values
> +	 * are correct nevertheless.
> +	 */
> +	.regs = ipq_pll_offsets[CLK_ALPHA_PLL_TYPE_STROMER_PLUS],
> +	.flags = SUPPORTS_DYNAMIC_UPDATE,
> +	.clkr = {
> +		.enable_reg = 0x0,
> +		.enable_mask = BIT(0),
> +		.hw.init = &(const struct clk_init_data) {
> +			.name = "a53pll",
> +			.parent_data = &(const struct clk_parent_data) {
> +				.fw_name = "xo",
> +			},
> +			.num_parents = 1,
> +			.ops = &clk_alpha_pll_stromer_ops,
> +		},
> +	},
> +};
> +
>   static struct clk_alpha_pll ipq_pll_stromer_plus = {
>   	.offset = 0x0,
>   	.regs = ipq_pll_offsets[CLK_ALPHA_PLL_TYPE_STROMER_PLUS],
> @@ -144,8 +167,8 @@ struct apss_pll_data {
>   };
>   
>   static const struct apss_pll_data ipq5018_pll_data = {
> -	.pll_type = CLK_ALPHA_PLL_TYPE_STROMER_PLUS,
> -	.pll = &ipq_pll_stromer_plus,
> +	.pll_type = CLK_ALPHA_PLL_TYPE_STROMER,
> +	.pll = &ipq_pll_stromer,
>   	.pll_config = &ipq5018_pll_config,
>   };
>   
> @@ -203,7 +226,8 @@ static int apss_ipq_pll_probe(struct platform_device *pdev)
>   
>   	if (data->pll_type == CLK_ALPHA_PLL_TYPE_HUAYRA)
>   		clk_alpha_pll_configure(data->pll, regmap, data->pll_config);
> -	else if (data->pll_type == CLK_ALPHA_PLL_TYPE_STROMER_PLUS)
> +	else if (data->pll_type == CLK_ALPHA_PLL_TYPE_STROMER ||
> +		 data->pll_type == CLK_ALPHA_PLL_TYPE_STROMER_PLUS)
>   		clk_stromer_pll_configure(data->pll, regmap, data->pll_config);
>   
>   	ret = devm_clk_register_regmap(dev, &data->pll->clkr);
> 
> ---
> base-commit: e8f897f4afef0031fe618a8e94127a0934896aba
> change-id: 20240311-apss-ipq-pll-ipq5018-hang-74d9a8f47136
> 
> Best regards,
Gabor Juhos March 18, 2024, 8:03 a.m. UTC | #2 
2024. 03. 18. 6:39 keltezéssel, Kathiravan Thirumoorthy írta:

...

>> Changes in v2:
>>    - extend commit description due to the changes
>>    - add a comment about why CLK_ALPHA_PLL_TYPE_STROMER_PLUS register offsets
>>      are used
>>    - constify hw clock init data (Stephen)
>>    - change pll_type in ipq5018_pll_data to CLK_ALPHA_PLL_TYPE_STROMER (Konrad)
>>    - Link to v1:
>> https://lore.kernel.org/r/20240311-apss-ipq-pll-ipq5018-hang-v1-1-8ed42b7a904d@gmail.com
> 
> 
> I don't see a reason why my tags are dropped, nevertheless

Sorry, that is my fault. Since I have modified the patch, I was not sure that
the tags are applicable to the new version or not.

> Tested-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
> Reviewed-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>

Thanks!

-Gabor
Bjorn Andersson April 27, 2024, 7:34 p.m. UTC | #3 
On Fri, 15 Mar 2024 17:16:41 +0100, Gabor Juhos wrote:
> Booting v6.8 results in a hang on various IPQ5018 based boards.
> Investigating the problem showed that the hang happens when the
> clk_alpha_pll_stromer_plus_set_rate() function tries to write
> into the PLL_MODE register of the APSS PLL.
> 
> Checking the downstream code revealed that it uses [1] stromer
> specific operations for IPQ5018, whereas in the current code
> the stromer plus specific operations are used.
> 
> [...]

Applied, thanks!

[1/1] clk: qcom: apss-ipq-pll: use stromer ops for IPQ5018 to fix boot failure
      commit: 5fce38e2a1a97900989d9fedebcf5a4dacdaee30

Best regards,
diff mbox series

Patch

diff --git a/drivers/clk/qcom/apss-ipq-pll.c b/drivers/clk/qcom/apss-ipq-pll.c
index 678b805f13d45..dfffec2f06ae7 100644
--- a/drivers/clk/qcom/apss-ipq-pll.c
+++ b/drivers/clk/qcom/apss-ipq-pll.c
@@ -55,6 +55,29 @@  static struct clk_alpha_pll ipq_pll_huayra = {
 	},
 };
 
+static struct clk_alpha_pll ipq_pll_stromer = {
+	.offset = 0x0,
+	/*
+	 * Reuse CLK_ALPHA_PLL_TYPE_STROMER_PLUS register offsets.
+	 * Although this is a bit confusing, but the offset values
+	 * are correct nevertheless.
+	 */
+	.regs = ipq_pll_offsets[CLK_ALPHA_PLL_TYPE_STROMER_PLUS],
+	.flags = SUPPORTS_DYNAMIC_UPDATE,
+	.clkr = {
+		.enable_reg = 0x0,
+		.enable_mask = BIT(0),
+		.hw.init = &(const struct clk_init_data) {
+			.name = "a53pll",
+			.parent_data = &(const struct clk_parent_data) {
+				.fw_name = "xo",
+			},
+			.num_parents = 1,
+			.ops = &clk_alpha_pll_stromer_ops,
+		},
+	},
+};
+
 static struct clk_alpha_pll ipq_pll_stromer_plus = {
 	.offset = 0x0,
 	.regs = ipq_pll_offsets[CLK_ALPHA_PLL_TYPE_STROMER_PLUS],
@@ -144,8 +167,8 @@  struct apss_pll_data {
 };
 
 static const struct apss_pll_data ipq5018_pll_data = {
-	.pll_type = CLK_ALPHA_PLL_TYPE_STROMER_PLUS,
-	.pll = &ipq_pll_stromer_plus,
+	.pll_type = CLK_ALPHA_PLL_TYPE_STROMER,
+	.pll = &ipq_pll_stromer,
 	.pll_config = &ipq5018_pll_config,
 };
 
@@ -203,7 +226,8 @@  static int apss_ipq_pll_probe(struct platform_device *pdev)
 
 	if (data->pll_type == CLK_ALPHA_PLL_TYPE_HUAYRA)
 		clk_alpha_pll_configure(data->pll, regmap, data->pll_config);
-	else if (data->pll_type == CLK_ALPHA_PLL_TYPE_STROMER_PLUS)
+	else if (data->pll_type == CLK_ALPHA_PLL_TYPE_STROMER ||
+		 data->pll_type == CLK_ALPHA_PLL_TYPE_STROMER_PLUS)
 		clk_stromer_pll_configure(data->pll, regmap, data->pll_config);
 
 	ret = devm_clk_register_regmap(dev, &data->pll->clkr);