[PATCH v2 00/11] Add GPLL0 as clock provider for the Qualcomm's IPQ mailbox controller

[PATCH v2 00/11] Add GPLL0 as clock provider for the Qualcomm's IPQ mailbox controller

[Kathiravan Thirumoorthy]

Currently mailbox controller takes the XO and APSS PLL as the input. It
can take the GPLL0 also as an input. This patch series adds the same and
fixes the issue caused by this.

Once the cpufreq driver is up, it tries to bump up the cpu frequency
above 800MHz, while doing so system is going to unusable state. Reason
being, with the GPLL0 included as clock source, clock framework tries to
achieve the required rate with the possible parent and since GPLL0
carries the CLK_SET_RATE_PARENT flag, clock rate of the GPLL0 is getting
changed, causing the issue.

First half of the series, removes the CLK_SET_RATE_PARENT flag from the
PLL clocks since the PLL clock rates shouldn't be changed. Another
half, add the necessary support to include the GPLL0 as clock provider
for mailbox and accomodate the changes in APSS clock driver.

This is also the preparatory series to enable the CPUFreq on IPQ5332
SoC. Dynamic scaling of CPUFreq is not supported on IPQ5332, so to
switch between the frequencies we need to park the APSS PLL in safe
source, here it is GPLL0 and then shutdown and bring up the APSS PLL in
the desired rate.

For IPQ5332 SoC, this series depends on the below patch
https://lore.kernel.org/linux-arm-msm/1693474133-10467-1-git-send-email-quic_varada@quicinc.com/

Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
---
Changes in v2:
- included the patch to drop the CLK_SET_RATE_PARENT from IPQ5018 GCC driver
- Splitted the DTS changes per target
- For IPQ8074 and IPQ6018 keep the CLK_SET_RATE_PARENT for UBI32 PLL
  since the PLL clock rates can be changed
- Pick up the tags in the relevant patches
- Link to v1: https://lore.kernel.org/r/20230904-gpll_cleanup-v1-0-de2c448f1188@quicinc.com

---
Kathiravan Thirumoorthy (11):
      clk: qcom: ipq8074: drop the CLK_SET_RATE_PARENT flag from PLL clocks
      clk: qcom: ipq6018: drop the CLK_SET_RATE_PARENT flag from PLL clocks
      clk: qcom: ipq5018: drop the CLK_SET_RATE_PARENT flag from GPLL clocks
      clk: qcom: ipq9574: drop the CLK_SET_RATE_PARENT flag from GPLL clocks
      clk: qcom: ipq5332: drop the CLK_SET_RATE_PARENT flag from GPLL clocks
      dt-bindings: mailbox: qcom: add one more clock provider for IPQ mailbox
      clk: qcom: apss-ipq6018: add the GPLL0 clock also as clock provider
      arm64: dts: qcom: ipq8074: include the GPLL0 as clock provider for mailbox
      arm64: dts: qcom: ipq6018: include the GPLL0 as clock provider for mailbox
      arm64: dts: qcom: ipq9574: include the GPLL0 as clock provider for mailbox
      arm64: dts: qcom: ipq5332: include the GPLL0 as clock provider for mailbox

 .../devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml          | 2 ++
 arch/arm64/boot/dts/qcom/ipq5332.dtsi                               | 4 ++--
 arch/arm64/boot/dts/qcom/ipq6018.dtsi                               | 4 ++--
 arch/arm64/boot/dts/qcom/ipq8074.dtsi                               | 4 ++--
 arch/arm64/boot/dts/qcom/ipq9574.dtsi                               | 4 ++--
 drivers/clk/qcom/apss-ipq6018.c                                     | 3 +++
 drivers/clk/qcom/gcc-ipq5018.c                                      | 3 ---
 drivers/clk/qcom/gcc-ipq5332.c                                      | 2 --
 drivers/clk/qcom/gcc-ipq6018.c                                      | 6 ------
 drivers/clk/qcom/gcc-ipq8074.c                                      | 6 ------
 drivers/clk/qcom/gcc-ipq9574.c                                      | 4 ----
 11 files changed, 13 insertions(+), 29 deletions(-)
---
base-commit: e143016b56ecb0fcda5bb6026b0a25fe55274f56
change-id: 20230913-gpll_cleanup-5d0a339ebd17

Best regards,
-- 
Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>

[PATCH v2 01/11] clk: qcom: ipq8074: drop the CLK_SET_RATE_PARENT flag from PLL clocks

[Kathiravan Thirumoorthy]

GPLL, NSS crypto PLL clock rates are fixed and shouldn't be scaled based
on the request from dependent clocks. Doing so will result in the
unexpected behaviour. So drop the CLK_SET_RATE_PARENT flag from the PLL
clocks.

Cc: stable@vger.kernel.org
Fixes: b8e7e519625f ("clk: qcom: ipq8074: add remaining PLL’s")
Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
---
Changes in V2:
	- Include the stable mailing list
	- Keep the CLK_SET_RATE_PARENT in UBI32 PLL, looks like these
	  PLL rates can be changed. So don't drop the flag.
---
 drivers/clk/qcom/gcc-ipq8074.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/clk/qcom/gcc-ipq8074.c b/drivers/clk/qcom/gcc-ipq8074.c
index 63ac2ced76bb..b7faf12a511a 100644
--- a/drivers/clk/qcom/gcc-ipq8074.c
+++ b/drivers/clk/qcom/gcc-ipq8074.c
@@ -75,7 +75,6 @@ static struct clk_fixed_factor gpll0_out_main_div2 = {
 				&gpll0_main.clkr.hw },
 		.num_parents = 1,
 		.ops = &clk_fixed_factor_ops,
-		.flags = CLK_SET_RATE_PARENT,
 	},
 };
 
@@ -121,7 +120,6 @@ static struct clk_alpha_pll_postdiv gpll2 = {
 				&gpll2_main.clkr.hw },
 		.num_parents = 1,
 		.ops = &clk_alpha_pll_postdiv_ro_ops,
-		.flags = CLK_SET_RATE_PARENT,
 	},
 };
 
@@ -154,7 +152,6 @@ static struct clk_alpha_pll_postdiv gpll4 = {
 				&gpll4_main.clkr.hw },
 		.num_parents = 1,
 		.ops = &clk_alpha_pll_postdiv_ro_ops,
-		.flags = CLK_SET_RATE_PARENT,
 	},
 };
 
@@ -188,7 +185,6 @@ static struct clk_alpha_pll_postdiv gpll6 = {
 				&gpll6_main.clkr.hw },
 		.num_parents = 1,
 		.ops = &clk_alpha_pll_postdiv_ro_ops,
-		.flags = CLK_SET_RATE_PARENT,
 	},
 };
 
@@ -201,7 +197,6 @@ static struct clk_fixed_factor gpll6_out_main_div2 = {
 				&gpll6_main.clkr.hw },
 		.num_parents = 1,
 		.ops = &clk_fixed_factor_ops,
-		.flags = CLK_SET_RATE_PARENT,
 	},
 };
 
@@ -266,7 +261,6 @@ static struct clk_alpha_pll_postdiv nss_crypto_pll = {
 				&nss_crypto_pll_main.clkr.hw },
 		.num_parents = 1,
 		.ops = &clk_alpha_pll_postdiv_ro_ops,
-		.flags = CLK_SET_RATE_PARENT,
 	},
 };
 

-- 
2.34.1

Re: [PATCH v2 01/11] clk: qcom: ipq8074: drop the CLK_SET_RATE_PARENT flag from PLL clocks

[Konrad Dybcio]

On 14.09.2023 08:59, Kathiravan Thirumoorthy wrote:
> GPLL, NSS crypto PLL clock rates are fixed and shouldn't be scaled based
> on the request from dependent clocks. Doing so will result in the
> unexpected behaviour. So drop the CLK_SET_RATE_PARENT flag from the PLL
> clocks.
> 
> Cc: stable@vger.kernel.org
> Fixes: b8e7e519625f ("clk: qcom: ipq8074: add remaining PLL’s")
> Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
> ---
Stephen, do you think there should be some sort of error
or at least warning thrown when SET_RATE_PARENT is used with
RO ops?

Konrad

Re: [PATCH v2 01/11] clk: qcom: ipq8074: drop the CLK_SET_RATE_PARENT flag from PLL clocks

[Stephen Boyd]

Quoting Konrad Dybcio (2023-09-15 05:19:56)
> On 14.09.2023 08:59, Kathiravan Thirumoorthy wrote:
> > GPLL, NSS crypto PLL clock rates are fixed and shouldn't be scaled based
> > on the request from dependent clocks. Doing so will result in the
> > unexpected behaviour. So drop the CLK_SET_RATE_PARENT flag from the PLL
> > clocks.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: b8e7e519625f ("clk: qcom: ipq8074: add remaining PLL’s")
> > Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
> > ---
> Stephen, do you think there should be some sort of error
> or at least warning thrown when SET_RATE_PARENT is used with
> RO ops?
> 

Sure? How would that be implemented?

Re: [PATCH v2 01/11] clk: qcom: ipq8074: drop the CLK_SET_RATE_PARENT flag from PLL clocks

[Konrad Dybcio]

On 10/19/23 02:16, Stephen Boyd wrote:
> Quoting Konrad Dybcio (2023-09-15 05:19:56)
>> On 14.09.2023 08:59, Kathiravan Thirumoorthy wrote:
>>> GPLL, NSS crypto PLL clock rates are fixed and shouldn't be scaled based
>>> on the request from dependent clocks. Doing so will result in the
>>> unexpected behaviour. So drop the CLK_SET_RATE_PARENT flag from the PLL
>>> clocks.
>>>
>>> Cc: stable@vger.kernel.org
>>> Fixes: b8e7e519625f ("clk: qcom: ipq8074: add remaining PLL’s")
>>> Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
>>> ---
>> Stephen, do you think there should be some sort of error
>> or at least warning thrown when SET_RATE_PARENT is used with
>> RO ops?
>>
> 
> Sure? How would that be implemented?
drivers/clk/clk.c : static void clk_change_rate()

if (!skip_set_rate && core->ops->set_rate)
	core->ops->set_rate(core->hw, core->new_rate, best_parent_rate);

->

if (!skip_set_rate) {
	if (core->ops->set_rate)
		core->ops->set_rate(core->hw, core->new_rate,
				    best_parent_rate);
	else
		pr_err("bad idea");
}

Konrad

Re: [PATCH v2 01/11] clk: qcom: ipq8074: drop the CLK_SET_RATE_PARENT flag from PLL clocks

[Stephen Boyd]

Quoting Konrad Dybcio (2023-10-19 04:22:33)
> 
> 
> On 10/19/23 02:16, Stephen Boyd wrote:
> > Quoting Konrad Dybcio (2023-09-15 05:19:56)
> >> On 14.09.2023 08:59, Kathiravan Thirumoorthy wrote:
> >>> GPLL, NSS crypto PLL clock rates are fixed and shouldn't be scaled based
> >>> on the request from dependent clocks. Doing so will result in the
> >>> unexpected behaviour. So drop the CLK_SET_RATE_PARENT flag from the PLL
> >>> clocks.
> >>>
> >>> Cc: stable@vger.kernel.org
> >>> Fixes: b8e7e519625f ("clk: qcom: ipq8074: add remaining PLL’s")
> >>> Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
> >>> ---
> >> Stephen, do you think there should be some sort of error
> >> or at least warning thrown when SET_RATE_PARENT is used with
> >> RO ops?
> >>
> > 
> > Sure? How would that be implemented?
> drivers/clk/clk.c : static void clk_change_rate()
> 
> if (!skip_set_rate && core->ops->set_rate)
>         core->ops->set_rate(core->hw, core->new_rate, best_parent_rate);
> 
> ->
> 
> if (!skip_set_rate) {
>         if (core->ops->set_rate)
>                 core->ops->set_rate(core->hw, core->new_rate,
>                                     best_parent_rate);
>         else
>                 pr_err("bad idea");
> }
> 

CLK_SET_RATE_PARENT means that "calling clk_set_rate() on this clk will
propagate up to the parent". Changing the rate of the parent could
change the rate of this clk to be the same frequency as the parent if
this clk doesn't have a set_rate clk op, or it could be that this clk
has a fixed divider so during the determine_rate() callback it
calculated what rate the parent should be to achieve the requested rate
in clk_set_rate().

It really matters what determine_rate() returns for a clk and if after
changing rates that rate is actually achieved. I suppose if the
determine_rate() callback returns some rate, and then we recalc rates
and notice that the rate determined earlier doesn't match we're
concerned. So far in the last decade we've never cared about this though
and I'm hesitant to start adding that check. I believe some qcom clk
drivers take a shortcut and round the rate in frequency tables so
whatever is returned in determine_rate() doesn't match what
recalc_rate() calculates.

It would be interesting to get rid of the CLK_SET_RATE_PARENT check in
clk_calc_new_rates() and simply always call clk_calc_new_rates() on the
parent if the parent->rate doesn't match what determine_rate thought it
should be. The framework currently calls the rounding clk op for a clk
and gets back the parent rate that the clk requires to achieve that rate
and then it blindly trusts that the parent rate is going to be achieved.
If the CLK_SET_RATE_PARENT flag is set it calls clk_calc_new_rates()
recursively on the parent, but then it doesn't check that the parent
rate is what was requested. That's mostly there to figure out if the
parent also needs to change rate, i.e. calculating the 'top' clk in a
rate change. Note that this also calls determine_rate again on the
parent, once from the child clk's determine_rate clk op and once from
the framework.

I wouldn't be surprised if some driver is relying on this behavior where
the rate isn't checked after being set. Maybe when we extend struct
clk_rate_request to have a linked list that allows a clk to build up a
chain of rate requests we can also enforce more things like matching
rates on recalc. Then any drivers that are relying on this behavior will
have to opt in to a different method of changing rates and notice that
things aren't working.

[PATCH v2 02/11] clk: qcom: ipq6018: drop the CLK_SET_RATE_PARENT flag from PLL clocks

[Kathiravan Thirumoorthy]

GPLL, NSS crypto PLL clock rates are fixed and shouldn't be scaled based
on the request from dependent clocks. Doing so will result in the
unexpected behaviour. So drop the CLK_SET_RATE_PARENT flag from the PLL
clocks.

Cc: stable@vger.kernel.org
Fixes: d9db07f088af ("clk: qcom: Add ipq6018 Global Clock Controller support")
Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
---
Changes in V2:
	- Include the stable mailing list
	- Keep the CLK_SET_RATE_PARENT in UBI32 PLL, looks like these
	  PLL rates can be changed. So don't drop the flag.
---
 drivers/clk/qcom/gcc-ipq6018.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/clk/qcom/gcc-ipq6018.c b/drivers/clk/qcom/gcc-ipq6018.c
index 6120fbbc5de0..f9494fa1b871 100644
--- a/drivers/clk/qcom/gcc-ipq6018.c
+++ b/drivers/clk/qcom/gcc-ipq6018.c
@@ -72,7 +72,6 @@ static struct clk_fixed_factor gpll0_out_main_div2 = {
 				&gpll0_main.clkr.hw },
 		.num_parents = 1,
 		.ops = &clk_fixed_factor_ops,
-		.flags = CLK_SET_RATE_PARENT,
 	},
 };
 
@@ -86,7 +85,6 @@ static struct clk_alpha_pll_postdiv gpll0 = {
 				&gpll0_main.clkr.hw },
 		.num_parents = 1,
 		.ops = &clk_alpha_pll_postdiv_ro_ops,
-		.flags = CLK_SET_RATE_PARENT,
 	},
 };
 
@@ -161,7 +159,6 @@ static struct clk_alpha_pll_postdiv gpll6 = {
 				&gpll6_main.clkr.hw },
 		.num_parents = 1,
 		.ops = &clk_alpha_pll_postdiv_ro_ops,
-		.flags = CLK_SET_RATE_PARENT,
 	},
 };
 
@@ -192,7 +189,6 @@ static struct clk_alpha_pll_postdiv gpll4 = {
 				&gpll4_main.clkr.hw },
 		.num_parents = 1,
 		.ops = &clk_alpha_pll_postdiv_ro_ops,
-		.flags = CLK_SET_RATE_PARENT,
 	},
 };
 
@@ -243,7 +239,6 @@ static struct clk_alpha_pll_postdiv gpll2 = {
 				&gpll2_main.clkr.hw },
 		.num_parents = 1,
 		.ops = &clk_alpha_pll_postdiv_ro_ops,
-		.flags = CLK_SET_RATE_PARENT,
 	},
 };
 
@@ -274,7 +269,6 @@ static struct clk_alpha_pll_postdiv nss_crypto_pll = {
 				&nss_crypto_pll_main.clkr.hw },
 		.num_parents = 1,
 		.ops = &clk_alpha_pll_postdiv_ro_ops,
-		.flags = CLK_SET_RATE_PARENT,
 	},
 };
 

-- 
2.34.1

Re: [PATCH v2 02/11] clk: qcom: ipq6018: drop the CLK_SET_RATE_PARENT flag from PLL clocks

[Konrad Dybcio]

On 14.09.2023 08:59, Kathiravan Thirumoorthy wrote:
> GPLL, NSS crypto PLL clock rates are fixed and shouldn't be scaled based
> on the request from dependent clocks. Doing so will result in the
> unexpected behaviour. So drop the CLK_SET_RATE_PARENT flag from the PLL
> clocks.
> 
> Cc: stable@vger.kernel.org
> Fixes: d9db07f088af ("clk: qcom: Add ipq6018 Global Clock Controller support")
> Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

[PATCH v2 03/11] clk: qcom: ipq5018: drop the CLK_SET_RATE_PARENT flag from GPLL clocks

[Kathiravan Thirumoorthy]

GPLL clock rates are fixed and shouldn't be scaled based on the
request from dependent clocks. Doing so will result in the unexpected
behaviour. So drop the CLK_SET_RATE_PARENT flag from the GPLL clocks.

Fixes: e3fdbef1bab8 ("clk: qcom: Add Global Clock controller (GCC) driver for IPQ5018")
Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
----
Changes in V2:
	- New patch
---
 drivers/clk/qcom/gcc-ipq5018.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/clk/qcom/gcc-ipq5018.c b/drivers/clk/qcom/gcc-ipq5018.c
index 19dc2b71cacf..2a3c0659b700 100644
--- a/drivers/clk/qcom/gcc-ipq5018.c
+++ b/drivers/clk/qcom/gcc-ipq5018.c
@@ -128,7 +128,6 @@ static struct clk_alpha_pll_postdiv gpll0 = {
 		},
 		.num_parents = 1,
 		.ops = &clk_alpha_pll_postdiv_ro_ops,
-		.flags = CLK_SET_RATE_PARENT,
 	},
 };
 
@@ -143,7 +142,6 @@ static struct clk_alpha_pll_postdiv gpll2 = {
 		},
 		.num_parents = 1,
 		.ops = &clk_alpha_pll_postdiv_ro_ops,
-		.flags = CLK_SET_RATE_PARENT,
 	},
 };
 
@@ -158,7 +156,6 @@ static struct clk_alpha_pll_postdiv gpll4 = {
 		},
 		.num_parents = 1,
 		.ops = &clk_alpha_pll_postdiv_ro_ops,
-		.flags = CLK_SET_RATE_PARENT,
 	},
 };
 

-- 
2.34.1

Re: [PATCH v2 03/11] clk: qcom: ipq5018: drop the CLK_SET_RATE_PARENT flag from GPLL clocks

[Konrad Dybcio]

On 14.09.2023 08:59, Kathiravan Thirumoorthy wrote:
> GPLL clock rates are fixed and shouldn't be scaled based on the
> request from dependent clocks. Doing so will result in the unexpected
> behaviour. So drop the CLK_SET_RATE_PARENT flag from the GPLL clocks.
> 
> Fixes: e3fdbef1bab8 ("clk: qcom: Add Global Clock controller (GCC) driver for IPQ5018")
> Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
> ----
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

[PATCH v2 04/11] clk: qcom: ipq9574: drop the CLK_SET_RATE_PARENT flag from GPLL clocks

[Kathiravan Thirumoorthy]

GPLL clock rates are fixed and shouldn't be scaled based on the request
from dependent clocks. Doing so will result in the unexpected behaviour.
So drop the CLK_SET_RATE_PARENT flag from the GPLL clocks.

Fixes: d75b82cff488 ("clk: qcom: Add Global Clock Controller driver for IPQ9574")
Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
----
Changes in V2:
	- No changes
---
 drivers/clk/qcom/gcc-ipq9574.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/clk/qcom/gcc-ipq9574.c b/drivers/clk/qcom/gcc-ipq9574.c
index 8f430367299e..e8190108e1ae 100644
--- a/drivers/clk/qcom/gcc-ipq9574.c
+++ b/drivers/clk/qcom/gcc-ipq9574.c
@@ -87,7 +87,6 @@ static struct clk_fixed_factor gpll0_out_main_div2 = {
 			&gpll0_main.clkr.hw
 		},
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT,
 		.ops = &clk_fixed_factor_ops,
 	},
 };
@@ -102,7 +101,6 @@ static struct clk_alpha_pll_postdiv gpll0 = {
 			&gpll0_main.clkr.hw
 		},
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT,
 		.ops = &clk_alpha_pll_postdiv_ro_ops,
 	},
 };
@@ -132,7 +130,6 @@ static struct clk_alpha_pll_postdiv gpll4 = {
 			&gpll4_main.clkr.hw
 		},
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT,
 		.ops = &clk_alpha_pll_postdiv_ro_ops,
 	},
 };
@@ -162,7 +159,6 @@ static struct clk_alpha_pll_postdiv gpll2 = {
 			&gpll2_main.clkr.hw
 		},
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT,
 		.ops = &clk_alpha_pll_postdiv_ro_ops,
 	},
 };

-- 
2.34.1

Re: [PATCH v2 04/11] clk: qcom: ipq9574: drop the CLK_SET_RATE_PARENT flag from GPLL clocks

[Konrad Dybcio]

On 14.09.2023 08:59, Kathiravan Thirumoorthy wrote:
> GPLL clock rates are fixed and shouldn't be scaled based on the request
> from dependent clocks. Doing so will result in the unexpected behaviour.
> So drop the CLK_SET_RATE_PARENT flag from the GPLL clocks.
> 
> Fixes: d75b82cff488 ("clk: qcom: Add Global Clock Controller driver for IPQ9574")
> Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
> ----
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

[PATCH v2 05/11] clk: qcom: ipq5332: drop the CLK_SET_RATE_PARENT flag from GPLL clocks

[Kathiravan Thirumoorthy]

GPLL clock rates are fixed and shouldn't be scaled based on the
request from dependent clocks. Doing so will result in the unexpected
behaviour. So drop the CLK_SET_RATE_PARENT flag from the GPLL clocks.

Fixes: 3d89d52970fd ("clk: qcom: add Global Clock controller (GCC) driver for IPQ5332 SoC")
Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
---
Changes in V2:
	- No change
---
 drivers/clk/qcom/gcc-ipq5332.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/clk/qcom/gcc-ipq5332.c b/drivers/clk/qcom/gcc-ipq5332.c
index b02026f8549b..6c74a117ef0b 100644
--- a/drivers/clk/qcom/gcc-ipq5332.c
+++ b/drivers/clk/qcom/gcc-ipq5332.c
@@ -114,7 +114,6 @@ static struct clk_alpha_pll_postdiv gpll2 = {
 				&gpll2_main.clkr.hw },
 		.num_parents = 1,
 		.ops = &clk_alpha_pll_postdiv_ro_ops,
-		.flags = CLK_SET_RATE_PARENT,
 	},
 };
 
@@ -154,7 +153,6 @@ static struct clk_alpha_pll_postdiv gpll4 = {
 				&gpll4_main.clkr.hw },
 		.num_parents = 1,
 		.ops = &clk_alpha_pll_postdiv_ro_ops,
-		.flags = CLK_SET_RATE_PARENT,
 	},
 };
 

-- 
2.34.1

Re: [PATCH v2 05/11] clk: qcom: ipq5332: drop the CLK_SET_RATE_PARENT flag from GPLL clocks

[Konrad Dybcio]

On 14.09.2023 08:59, Kathiravan Thirumoorthy wrote:
> GPLL clock rates are fixed and shouldn't be scaled based on the
> request from dependent clocks. Doing so will result in the unexpected
> behaviour. So drop the CLK_SET_RATE_PARENT flag from the GPLL clocks.
> 
> Fixes: 3d89d52970fd ("clk: qcom: add Global Clock controller (GCC) driver for IPQ5332 SoC")
> Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

[PATCH v2 06/11] dt-bindings: mailbox: qcom: add one more clock provider for IPQ mailbox

[Kathiravan Thirumoorthy]

Mailbox controller present in the IPQ SoCs takes the GPLL0 clock also as
an input. Document the same.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
---
Changes in V2:
	- Pick up the R-b tag
---
 Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
index d2e25ff6db7f..a38413f8d132 100644
--- a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
+++ b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
@@ -125,10 +125,12 @@ allOf:
           items:
             - description: primary pll parent of the clock driver
             - description: XO clock
+            - description: GCC GPLL0 clock source
         clock-names:
           items:
             - const: pll
             - const: xo
+            - const: gpll0
 
   - if:
       properties:

-- 
2.34.1

[PATCH v2 07/11] clk: qcom: apss-ipq6018: add the GPLL0 clock also as clock provider

[Kathiravan Thirumoorthy]

While the kernel is booting up, APSS PLL will be running at 800MHz with
GPLL0 as source. Once the cpufreq driver is available, APSS PLL will be
configured and select the rate based on the opp table and the source will
be changed to APSS_PLL_EARLY.

Without this patch, CPU Freq driver reports that CPU is running at 24MHz
instead of the 800MHz.

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Tested-by: Robert Marko <robimarko@gmail.com>
Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
---
Changes in V2:
	- Pick up te R-b and T-b tags. Thanks Robert!
---
 drivers/clk/qcom/apss-ipq6018.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/clk/qcom/apss-ipq6018.c b/drivers/clk/qcom/apss-ipq6018.c
index f2f502e2d5a4..4e13a085a857 100644
--- a/drivers/clk/qcom/apss-ipq6018.c
+++ b/drivers/clk/qcom/apss-ipq6018.c
@@ -20,16 +20,19 @@
 
 enum {
 	P_XO,
+	P_GPLL0,
 	P_APSS_PLL_EARLY,
 };
 
 static const struct clk_parent_data parents_apcs_alias0_clk_src[] = {
 	{ .fw_name = "xo" },
+	{ .fw_name = "gpll0" },
 	{ .fw_name = "pll" },
 };
 
 static const struct parent_map parents_apcs_alias0_clk_src_map[] = {
 	{ P_XO, 0 },
+	{ P_GPLL0, 4 },
 	{ P_APSS_PLL_EARLY, 5 },
 };
 

-- 
2.34.1

[PATCH v2 08/11] arm64: dts: qcom: ipq8074: include the GPLL0 as clock provider for mailbox

[Kathiravan Thirumoorthy]

While the kernel is booting up, APSS PLL will be running at 800MHz with
GPLL0 as source. Once the cpufreq driver is available, APSS PLL will be
configured to the rate based on the opp table and the source also will
be changed to APSS_PLL_EARLY. So allow the mailbox to consume the GPLL0,
with this inclusion, CPU Freq correctly reports that CPU is running at
800MHz rather than 24MHz.

Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
---
Changes in V2:
	- Splitted the change into target specific file
---
 arch/arm64/boot/dts/qcom/ipq8074.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/ipq8074.dtsi b/arch/arm64/boot/dts/qcom/ipq8074.dtsi
index 00ed71936b47..0be19267bdcf 100644
--- a/arch/arm64/boot/dts/qcom/ipq8074.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq8074.dtsi
@@ -719,8 +719,8 @@ apcs_glb: mailbox@b111000 {
 			compatible = "qcom,ipq8074-apcs-apps-global",
 				     "qcom,ipq6018-apcs-apps-global";
 			reg = <0x0b111000 0x1000>;
-			clocks = <&a53pll>, <&xo>;
-			clock-names = "pll", "xo";
+			clocks = <&a53pll>, <&xo>, <&gcc GPLL0>;
+			clock-names = "pll", "xo", "gpll0";
 
 			#clock-cells = <1>;
 			#mbox-cells = <1>;

-- 
2.34.1

Re: [PATCH v2 08/11] arm64: dts: qcom: ipq8074: include the GPLL0 as clock provider for mailbox

[Konrad Dybcio]

On 14.09.2023 08:59, Kathiravan Thirumoorthy wrote:
> While the kernel is booting up, APSS PLL will be running at 800MHz with
> GPLL0 as source. Once the cpufreq driver is available, APSS PLL will be
> configured to the rate based on the opp table and the source also will
> be changed to APSS_PLL_EARLY. So allow the mailbox to consume the GPLL0,
> with this inclusion, CPU Freq correctly reports that CPU is running at
> 800MHz rather than 24MHz.
> 
> Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

Re: [PATCH v2 08/11] arm64: dts: qcom: ipq8074: include the GPLL0 as clock provider for mailbox

[Kathiravan Thirumoorthy]

On 9/27/2023 5:03 PM, Konrad Dybcio wrote:
> On 14.09.2023 08:59, Kathiravan Thirumoorthy wrote:
>> While the kernel is booting up, APSS PLL will be running at 800MHz with
>> GPLL0 as source. Once the cpufreq driver is available, APSS PLL will be
>> configured to the rate based on the opp table and the source also will
>> be changed to APSS_PLL_EARLY. So allow the mailbox to consume the GPLL0,
>> with this inclusion, CPU Freq correctly reports that CPU is running at
>> 800MHz rather than 24MHz.
>>
>> Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
>> ---
> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>
> Konrad

Thanks Konrad.

I just realized that, in commit message, the statement "APSS PLL will be 
running at 800MHz" should be "APSS clock / CPU clock will be running at 
800MHz".

Bjorn, will you be able to fix it up while applying (all 4 DTS changes 
needs update) or shall I respin it?

Thanks,

Kathiravan T.

[PATCH v2 09/11] arm64: dts: qcom: ipq6018: include the GPLL0 as clock provider for mailbox

[Kathiravan Thirumoorthy]

While the kernel is booting up, APSS PLL will be running at 800MHz with
GPLL0 as source. Once the cpufreq driver is available, APSS PLL will be
configured to the rate based on the opp table and the source also will
be changed to APSS_PLL_EARLY. So allow the mailbox to consume the GPLL0,
with this inclusion, CPU Freq correctly reports that CPU is running at
800MHz rather than 24MHz.

Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
---
Changes in V2:
	- Splitted the change into target specific file
---
 arch/arm64/boot/dts/qcom/ipq6018.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/ipq6018.dtsi b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
index 47b8b1d6730a..a30a5b893762 100644
--- a/arch/arm64/boot/dts/qcom/ipq6018.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
@@ -619,8 +619,8 @@ apcs_glb: mailbox@b111000 {
 			compatible = "qcom,ipq6018-apcs-apps-global";
 			reg = <0x0 0x0b111000 0x0 0x1000>;
 			#clock-cells = <1>;
-			clocks = <&a53pll>, <&xo>;
-			clock-names = "pll", "xo";
+			clocks = <&a53pll>, <&xo>, <&gcc GPLL0>;
+			clock-names = "pll", "xo", "gpll0";
 			#mbox-cells = <1>;
 		};
 

-- 
2.34.1

Re: [PATCH v2 09/11] arm64: dts: qcom: ipq6018: include the GPLL0 as clock provider for mailbox

[Konrad Dybcio]

On 14.09.2023 08:59, Kathiravan Thirumoorthy wrote:
> While the kernel is booting up, APSS PLL will be running at 800MHz with
> GPLL0 as source. Once the cpufreq driver is available, APSS PLL will be
> configured to the rate based on the opp table and the source also will
> be changed to APSS_PLL_EARLY. So allow the mailbox to consume the GPLL0,
> with this inclusion, CPU Freq correctly reports that CPU is running at
> 800MHz rather than 24MHz.
> 
> Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

[PATCH v2 10/11] arm64: dts: qcom: ipq9574: include the GPLL0 as clock provider for mailbox

[Kathiravan Thirumoorthy]

While the kernel is booting up, APSS PLL will be running at 800MHz with
GPLL0 as source. Once the cpufreq driver is available, APSS PLL will be
configured to the rate based on the opp table and the source also will
be changed to APSS_PLL_EARLY. So allow the mailbox to consume the GPLL0,
with this inclusion, CPU Freq correctly reports that CPU is running at
800MHz rather than 24MHz.

Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
---
Changes in V2:
	- Splitted the change into target specific file
---
 arch/arm64/boot/dts/qcom/ipq9574.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
index 51aba071c1eb..89edb4b852df 100644
--- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
@@ -652,8 +652,8 @@ apcs_glb: mailbox@b111000 {
 				     "qcom,ipq6018-apcs-apps-global";
 			reg = <0x0b111000 0x1000>;
 			#clock-cells = <1>;
-			clocks = <&a73pll>, <&xo_board_clk>;
-			clock-names = "pll", "xo";
+			clocks = <&a73pll>, <&xo_board_clk>, <&gcc GPLL0>;
+			clock-names = "pll", "xo", "gpll0";
 			#mbox-cells = <1>;
 		};
 

-- 
2.34.1

Re: [PATCH v2 10/11] arm64: dts: qcom: ipq9574: include the GPLL0 as clock provider for mailbox

[Konrad Dybcio]

On 14.09.2023 09:00, Kathiravan Thirumoorthy wrote:
> While the kernel is booting up, APSS PLL will be running at 800MHz with
> GPLL0 as source. Once the cpufreq driver is available, APSS PLL will be
> configured to the rate based on the opp table and the source also will
> be changed to APSS_PLL_EARLY. So allow the mailbox to consume the GPLL0,
> with this inclusion, CPU Freq correctly reports that CPU is running at
> 800MHz rather than 24MHz.
> 
> Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

[PATCH v2 11/11] arm64: dts: qcom: ipq5332: include the GPLL0 as clock provider for mailbox

[Kathiravan Thirumoorthy]

While the kernel is booting up, APSS PLL will be running at 800MHz with
GPLL0 as source. Once the cpufreq driver is available, APSS PLL will be
configured to the rate based on the opp table and the source also will
be changed to APSS_PLL_EARLY. So allow the mailbox to consume the GPLL0,
with this inclusion, CPU Freq correctly reports that CPU is running at
800MHz rather than 24MHz.

Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
---
Changes in V2:
	- Splitted the change into target specific file
---
 arch/arm64/boot/dts/qcom/ipq5332.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
index 8bfc2db44624..82761ae199a9 100644
--- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
@@ -335,8 +335,8 @@ apcs_glb: mailbox@b111000 {
 				     "qcom,ipq6018-apcs-apps-global";
 			reg = <0x0b111000 0x1000>;
 			#clock-cells = <1>;
-			clocks = <&a53pll>, <&xo_board>;
-			clock-names = "pll", "xo";
+			clocks = <&a53pll>, <&xo_board>, <&gcc GPLL0>;
+			clock-names = "pll", "xo", "gpll0";
 			#mbox-cells = <1>;
 		};
 

-- 
2.34.1

Re: [PATCH v2 11/11] arm64: dts: qcom: ipq5332: include the GPLL0 as clock provider for mailbox

[Konrad Dybcio]

On 14.09.2023 09:00, Kathiravan Thirumoorthy wrote:
> While the kernel is booting up, APSS PLL will be running at 800MHz with
> GPLL0 as source. Once the cpufreq driver is available, APSS PLL will be
> configured to the rate based on the opp table and the source also will
> be changed to APSS_PLL_EARLY. So allow the mailbox to consume the GPLL0,
> with this inclusion, CPU Freq correctly reports that CPU is running at
> 800MHz rather than 24MHz.
> 
> Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

Re: [PATCH v2 00/11] Add GPLL0 as clock provider for the Qualcomm's IPQ mailbox controller

[Kathiravan Thirumoorthy]

On 9/14/2023 12:29 PM, Kathiravan Thirumoorthy wrote:
> Currently mailbox controller takes the XO and APSS PLL as the input. It
> can take the GPLL0 also as an input. This patch series adds the same and
> fixes the issue caused by this.
>
> Once the cpufreq driver is up, it tries to bump up the cpu frequency
> above 800MHz, while doing so system is going to unusable state. Reason
> being, with the GPLL0 included as clock source, clock framework tries to
> achieve the required rate with the possible parent and since GPLL0
> carries the CLK_SET_RATE_PARENT flag, clock rate of the GPLL0 is getting
> changed, causing the issue.
>
> First half of the series, removes the CLK_SET_RATE_PARENT flag from the
> PLL clocks since the PLL clock rates shouldn't be changed. Another
> half, add the necessary support to include the GPLL0 as clock provider
> for mailbox and accomodate the changes in APSS clock driver.
>
> This is also the preparatory series to enable the CPUFreq on IPQ5332
> SoC. Dynamic scaling of CPUFreq is not supported on IPQ5332, so to
> switch between the frequencies we need to park the APSS PLL in safe
> source, here it is GPLL0 and then shutdown and bring up the APSS PLL in
> the desired rate.
>
> For IPQ5332 SoC, this series depends on the below patch
> https://lore.kernel.org/linux-arm-msm/1693474133-10467-1-git-send-email-quic_varada@quicinc.com/


Bjorn, can this series picked up for v6.7?

There is a minor nit the commit message. The statement "APSS PLL will be 
running at 800MHz" should be "APSS clock / CPU clock will be running at 
800MHz" and this should be taken care for clk and the dts patches. Do 
let me know if I need to re-spin the address to address this.

Thanks,


>
> Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
> ---
> Changes in v2:
> - included the patch to drop the CLK_SET_RATE_PARENT from IPQ5018 GCC driver
> - Splitted the DTS changes per target
> - For IPQ8074 and IPQ6018 keep the CLK_SET_RATE_PARENT for UBI32 PLL
>    since the PLL clock rates can be changed
> - Pick up the tags in the relevant patches
> - Link to v1: https://lore.kernel.org/r/20230904-gpll_cleanup-v1-0-de2c448f1188@quicinc.com
>
> ---
> Kathiravan Thirumoorthy (11):
>        clk: qcom: ipq8074: drop the CLK_SET_RATE_PARENT flag from PLL clocks
>        clk: qcom: ipq6018: drop the CLK_SET_RATE_PARENT flag from PLL clocks
>        clk: qcom: ipq5018: drop the CLK_SET_RATE_PARENT flag from GPLL clocks
>        clk: qcom: ipq9574: drop the CLK_SET_RATE_PARENT flag from GPLL clocks
>        clk: qcom: ipq5332: drop the CLK_SET_RATE_PARENT flag from GPLL clocks
>        dt-bindings: mailbox: qcom: add one more clock provider for IPQ mailbox
>        clk: qcom: apss-ipq6018: add the GPLL0 clock also as clock provider
>        arm64: dts: qcom: ipq8074: include the GPLL0 as clock provider for mailbox
>        arm64: dts: qcom: ipq6018: include the GPLL0 as clock provider for mailbox
>        arm64: dts: qcom: ipq9574: include the GPLL0 as clock provider for mailbox
>        arm64: dts: qcom: ipq5332: include the GPLL0 as clock provider for mailbox
>
>   .../devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml          | 2 ++
>   arch/arm64/boot/dts/qcom/ipq5332.dtsi                               | 4 ++--
>   arch/arm64/boot/dts/qcom/ipq6018.dtsi                               | 4 ++--
>   arch/arm64/boot/dts/qcom/ipq8074.dtsi                               | 4 ++--
>   arch/arm64/boot/dts/qcom/ipq9574.dtsi                               | 4 ++--
>   drivers/clk/qcom/apss-ipq6018.c                                     | 3 +++
>   drivers/clk/qcom/gcc-ipq5018.c                                      | 3 ---
>   drivers/clk/qcom/gcc-ipq5332.c                                      | 2 --
>   drivers/clk/qcom/gcc-ipq6018.c                                      | 6 ------
>   drivers/clk/qcom/gcc-ipq8074.c                                      | 6 ------
>   drivers/clk/qcom/gcc-ipq9574.c                                      | 4 ----
>   11 files changed, 13 insertions(+), 29 deletions(-)
> ---
> base-commit: e143016b56ecb0fcda5bb6026b0a25fe55274f56
> change-id: 20230913-gpll_cleanup-5d0a339ebd17
>
> Best regards,

Re: (subset) [PATCH v2 00/11] Add GPLL0 as clock provider for the Qualcomm's IPQ mailbox controller

[Bjorn Andersson]

On Thu, 14 Sep 2023 12:29:50 +0530, Kathiravan Thirumoorthy wrote:
> Currently mailbox controller takes the XO and APSS PLL as the input. It
> can take the GPLL0 also as an input. This patch series adds the same and
> fixes the issue caused by this.
> 
> Once the cpufreq driver is up, it tries to bump up the cpu frequency
> above 800MHz, while doing so system is going to unusable state. Reason
> being, with the GPLL0 included as clock source, clock framework tries to
> achieve the required rate with the possible parent and since GPLL0
> carries the CLK_SET_RATE_PARENT flag, clock rate of the GPLL0 is getting
> changed, causing the issue.
> 
> [...]

Applied, thanks!

[01/11] clk: qcom: ipq8074: drop the CLK_SET_RATE_PARENT flag from PLL clocks
        commit: e641a070137dd959932c7c222e000d9d941167a2
[02/11] clk: qcom: ipq6018: drop the CLK_SET_RATE_PARENT flag from PLL clocks
        commit: 99cd4935cb972d0aafb16838bb2aeadbcaf196ce
[03/11] clk: qcom: ipq5018: drop the CLK_SET_RATE_PARENT flag from GPLL clocks
        commit: 01a5e4c6731ab6b4b74822661d296f8893fc1230
[04/11] clk: qcom: ipq9574: drop the CLK_SET_RATE_PARENT flag from GPLL clocks
        commit: 99a8f8764b70158a712992640a6be46a8fd79d15
[05/11] clk: qcom: ipq5332: drop the CLK_SET_RATE_PARENT flag from GPLL clocks
        commit: 5635ef0bd1052420bc659a00be6fd0c60cec5cb9
[07/11] clk: qcom: apss-ipq6018: add the GPLL0 clock also as clock provider
        commit: e0e6373d653b7707bf042ecf1538884597c5d0da
[08/11] arm64: dts: qcom: ipq8074: include the GPLL0 as clock provider for mailbox
        commit: 80ebe63329909531afc87335f1d95c7bf8414438
[09/11] arm64: dts: qcom: ipq6018: include the GPLL0 as clock provider for mailbox
        commit: 0133c7af3aa0420778d106cb90db708cfa45f2c6
[10/11] arm64: dts: qcom: ipq9574: include the GPLL0 as clock provider for mailbox
        commit: 77c726a4f3b124903db5ced7d597976d5b80dcfb
[11/11] arm64: dts: qcom: ipq5332: include the GPLL0 as clock provider for mailbox
        commit: da528016952bf93ca810c43fafe518c699db7fa0

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>