[PATCH 0/3] clk: qcom: gcc-sm8150: Add gcc_parents_0_ao support

[PATCH 0/3] clk: qcom: gcc-sm8150: Add gcc_parents_0_ao support

[Satya Priya Kakitapalli]

Add support for active_only parent data for gcc_parents_0
as some of the clocks under it are crtical and vote on XO
during suspend.

Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
---
Satya Priya Kakitapalli (3):
      dt-bindings: clock: qcom,gcc-sm8150: Add support for bi_tcxo_ao
      clk: qcom: gcc-sm8150: Add gcc_parents_0_ao support
      arm64: dts: qcom: sm8150: Add bi_tcxo_ao support for gcc

 Documentation/devicetree/bindings/clock/qcom,gcc-sm8150.yaml |  6 ++++--
 arch/arm64/boot/dts/qcom/sm8150.dtsi                         |  3 ++-
 drivers/clk/qcom/gcc-sm8150.c                                | 10 ++++++++--
 3 files changed, 14 insertions(+), 5 deletions(-)
---
base-commit: 774551425799cb5bbac94e1768fd69eec4f78dd4
change-id: 20240123-gcc-ao-support-dbab551ea4f0

Best regards,
-- 
Satya Priya Kakitapalli <quic_skakitap@quicinc.com>

[PATCH 1/3] dt-bindings: clock: qcom,gcc-sm8150: Add support for bi_tcxo_ao

[Satya Priya Kakitapalli]

Add support for bi_tcxo_ao, this allows to put an active only vote on
the critical clocks.

Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
---
 Documentation/devicetree/bindings/clock/qcom,gcc-sm8150.yaml | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/qcom,gcc-sm8150.yaml b/Documentation/devicetree/bindings/clock/qcom,gcc-sm8150.yaml
index 58ccb7df847c..17e29e9bbc11 100644
--- a/Documentation/devicetree/bindings/clock/qcom,gcc-sm8150.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,gcc-sm8150.yaml
@@ -23,11 +23,13 @@ properties:
   clocks:
     items:
       - description: Board XO source
+      - description: Board XO active only source
       - description: Sleep clock source
 
   clock-names:
     items:
       - const: bi_tcxo
+      - const: bi_tcxo_ao
       - const: sleep_clk
 
 required:
@@ -46,9 +48,9 @@ examples:
     clock-controller@100000 {
       compatible = "qcom,gcc-sm8150";
       reg = <0x00100000 0x1f0000>;
-      clocks = <&rpmhcc RPMH_CXO_CLK>,
+      clocks = <&rpmhcc RPMH_CXO_CLK>, <&rpmhcc RPMH_CXO_CLK_A>,
                <&sleep_clk>;
-      clock-names = "bi_tcxo", "sleep_clk";
+      clock-names = "bi_tcxo", "bi_tcxo_ao", "sleep_clk";
       #clock-cells = <1>;
       #reset-cells = <1>;
       #power-domain-cells = <1>;

-- 
2.25.1

[PATCH 2/3] clk: qcom: gcc-sm8150: Add gcc_parents_0_ao support

[Satya Priya Kakitapalli]

Add active_only support for gcc_parents_0, this is needed because
some of the clocks under it are critical which would vote on xo
blocking the suspend.

Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
---
 drivers/clk/qcom/gcc-sm8150.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/gcc-sm8150.c b/drivers/clk/qcom/gcc-sm8150.c
index 05d115c52dfe..2a0608c5a104 100644
--- a/drivers/clk/qcom/gcc-sm8150.c
+++ b/drivers/clk/qcom/gcc-sm8150.c
@@ -123,6 +123,12 @@ static const struct clk_parent_data gcc_parents_0[] = {
 	{ .hw = &gpll0_out_even.clkr.hw },
 };
 
+static const struct clk_parent_data gcc_parents_0_ao[] = {
+	{ .fw_name = "bi_tcxo_ao", .name = "bi_tcxo_ao" },
+	{ .hw = &gpll0.clkr.hw },
+	{ .hw = &gpll0_out_even.clkr.hw },
+};
+
 static const struct parent_map gcc_parent_map_1[] = {
 	{ P_BI_TCXO, 0 },
 	{ P_GPLL0_OUT_MAIN, 1 },
@@ -222,8 +228,8 @@ static struct clk_rcg2 gcc_cpuss_ahb_clk_src = {
 	.freq_tbl = ftbl_gcc_cpuss_ahb_clk_src,
 	.clkr.hw.init = &(struct clk_init_data){
 		.name = "gcc_cpuss_ahb_clk_src",
-		.parent_data = gcc_parents_0,
-		.num_parents = ARRAY_SIZE(gcc_parents_0),
+		.parent_data = gcc_parents_0_ao,
+		.num_parents = ARRAY_SIZE(gcc_parents_0_ao),
 		.flags = CLK_SET_RATE_PARENT,
 		.ops = &clk_rcg2_ops,
 	},

-- 
2.25.1

[PATCH 3/3] arm64: dts: qcom: sm8150: Add bi_tcxo_ao support for gcc

[Satya Priya Kakitapalli]

Add bi_tcxo_ao support to be able to put active only vote
on critical clocks during suspend.

Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sm8150.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi
index 761a6757dc26..b91ef95c0c3a 100644
--- a/arch/arm64/boot/dts/qcom/sm8150.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi
@@ -906,9 +906,10 @@ gcc: clock-controller@100000 {
 			#clock-cells = <1>;
 			#reset-cells = <1>;
 			#power-domain-cells = <1>;
-			clock-names = "bi_tcxo",
+			clock-names = "bi_tcxo", "bi_tcxo_ao",
 				      "sleep_clk";
 			clocks = <&rpmhcc RPMH_CXO_CLK>,
+				 <&rpmhcc RPMH_CXO_CLK_A>,
 				 <&sleep_clk>;
 		};
 

-- 
2.25.1

Re: [PATCH 2/3] clk: qcom: gcc-sm8150: Add gcc_parents_0_ao support

[Dmitry Baryshkov]

On Tue, 23 Jan 2024 at 18:35, Satya Priya Kakitapalli
<quic_skakitap@quicinc.com> wrote:
>
> Add active_only support for gcc_parents_0, this is needed because
> some of the clocks under it are critical which would vote on xo
> blocking the suspend.

The kernel should be able to work with older DT files. Please add
support for using bi_tcxo as a fallback.

>
> Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
> ---
>  drivers/clk/qcom/gcc-sm8150.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/qcom/gcc-sm8150.c b/drivers/clk/qcom/gcc-sm8150.c
> index 05d115c52dfe..2a0608c5a104 100644
> --- a/drivers/clk/qcom/gcc-sm8150.c
> +++ b/drivers/clk/qcom/gcc-sm8150.c
> @@ -123,6 +123,12 @@ static const struct clk_parent_data gcc_parents_0[] = {
>         { .hw = &gpll0_out_even.clkr.hw },
>  };
>
> +static const struct clk_parent_data gcc_parents_0_ao[] = {
> +       { .fw_name = "bi_tcxo_ao", .name = "bi_tcxo_ao" },
> +       { .hw = &gpll0.clkr.hw },
> +       { .hw = &gpll0_out_even.clkr.hw },
> +};
> +
>  static const struct parent_map gcc_parent_map_1[] = {
>         { P_BI_TCXO, 0 },
>         { P_GPLL0_OUT_MAIN, 1 },
> @@ -222,8 +228,8 @@ static struct clk_rcg2 gcc_cpuss_ahb_clk_src = {
>         .freq_tbl = ftbl_gcc_cpuss_ahb_clk_src,
>         .clkr.hw.init = &(struct clk_init_data){
>                 .name = "gcc_cpuss_ahb_clk_src",
> -               .parent_data = gcc_parents_0,
> -               .num_parents = ARRAY_SIZE(gcc_parents_0),
> +               .parent_data = gcc_parents_0_ao,
> +               .num_parents = ARRAY_SIZE(gcc_parents_0_ao),
>                 .flags = CLK_SET_RATE_PARENT,
>                 .ops = &clk_rcg2_ops,
>         },
>
> --
> 2.25.1
>
>


-- 
With best wishes
Dmitry

Re: [PATCH 2/3] clk: qcom: gcc-sm8150: Add gcc_parents_0_ao support

[Konrad Dybcio]

On 1/23/24 17:34, Satya Priya Kakitapalli wrote:
> Add active_only support for gcc_parents_0, this is needed because
> some of the clocks under it are critical which would vote on xo
> blocking the suspend.
> 
> Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
> ---

Is there a need to keep gcc_cpuss_ahb_clk_src around? Do we do any
ratesetting on it? Should we ever turn it off?

Konrad

Re: [PATCH 1/3] dt-bindings: clock: qcom,gcc-sm8150: Add support for bi_tcxo_ao

[Krzysztof Kozlowski]

On 23/01/2024 17:34, Satya Priya Kakitapalli wrote:
> Add support for bi_tcxo_ao, this allows to put an active only vote on
> the critical clocks.
> 
> Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
> ---
>  Documentation/devicetree/bindings/clock/qcom,gcc-sm8150.yaml | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/qcom,gcc-sm8150.yaml b/Documentation/devicetree/bindings/clock/qcom,gcc-sm8150.yaml
> index 58ccb7df847c..17e29e9bbc11 100644
> --- a/Documentation/devicetree/bindings/clock/qcom,gcc-sm8150.yaml
> +++ b/Documentation/devicetree/bindings/clock/qcom,gcc-sm8150.yaml
> @@ -23,11 +23,13 @@ properties:
>    clocks:
>      items:
>        - description: Board XO source
> +      - description: Board XO active only source
>        - description: Sleep clock source
>  
>    clock-names:
>      items:
>        - const: bi_tcxo
> +      - const: bi_tcxo_ao
>        - const: sleep_clk

You cannot add clocks in the middle. The order is fixed.

Best regards,
Krzysztof

Re: [PATCH 2/3] clk: qcom: gcc-sm8150: Add gcc_parents_0_ao support

[Satya Priya Kakitapalli (Temp)]

On 1/23/2024 11:17 PM, Konrad Dybcio wrote:
>
>
> On 1/23/24 17:34, Satya Priya Kakitapalli wrote:
>> Add active_only support for gcc_parents_0, this is needed because
>> some of the clocks under it are critical which would vote on xo
>> blocking the suspend.
>>
>> Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
>> ---
>
> Is there a need to keep gcc_cpuss_ahb_clk_src around? Do we do any
> ratesetting on it? Should we ever turn it off?
>

The branch clocks under gcc_cpuss_ahb_clk_src are critical clocks, which 
are running at 19.2Mhz causing vote on XO during suspend. As of now no 
rate setting is happening but this rcg is useful to get the exact rates 
from debugfs. Hence this change is needed to avoid XO shutdown issues.

Re: [PATCH 2/3] clk: qcom: gcc-sm8150: Add gcc_parents_0_ao support

[Konrad Dybcio]

On 1/25/24 06:49, Satya Priya Kakitapalli (Temp) wrote:
> 
> On 1/23/2024 11:17 PM, Konrad Dybcio wrote:
>>
>>
>> On 1/23/24 17:34, Satya Priya Kakitapalli wrote:
>>> Add active_only support for gcc_parents_0, this is needed because
>>> some of the clocks under it are critical which would vote on xo
>>> blocking the suspend.
>>>
>>> Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
>>> ---
>>
>> Is there a need to keep gcc_cpuss_ahb_clk_src around? Do we do any
>> ratesetting on it? Should we ever turn it off?
>>
> 
> The branch clocks under gcc_cpuss_ahb_clk_src are critical clocks, which are running at 19.2Mhz causing vote on XO during suspend. As of now no rate setting is happening but this rcg is useful to get the exact rates from debugfs. Hence this change is needed to avoid XO shutdown issues.

So, if I underderstood you correctly, this clock serves no purpose other
than getting rate?

In this case, I'd say we should de-register it from the clock driver and
use debugcc [1] (contributions welcome!) for precise measurements.

Konrad

[1] https://github.com/linux-msm/debugcc

Re: [PATCH 2/3] clk: qcom: gcc-sm8150: Add gcc_parents_0_ao support

[Satya Priya Kakitapalli (Temp)]

On 1/25/2024 3:25 PM, Konrad Dybcio wrote:
>
>
> On 1/25/24 06:49, Satya Priya Kakitapalli (Temp) wrote:
>>
>> On 1/23/2024 11:17 PM, Konrad Dybcio wrote:
>>>
>>>
>>> On 1/23/24 17:34, Satya Priya Kakitapalli wrote:
>>>> Add active_only support for gcc_parents_0, this is needed because
>>>> some of the clocks under it are critical which would vote on xo
>>>> blocking the suspend.
>>>>
>>>> Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
>>>> ---
>>>
>>> Is there a need to keep gcc_cpuss_ahb_clk_src around? Do we do any
>>> ratesetting on it? Should we ever turn it off?
>>>
>>
>> The branch clocks under gcc_cpuss_ahb_clk_src are critical clocks, 
>> which are running at 19.2Mhz causing vote on XO during suspend. As of 
>> now no rate setting is happening but this rcg is useful to get the 
>> exact rates from debugfs. Hence this change is needed to avoid XO 
>> shutdown issues.
>
> So, if I underderstood you correctly, this clock serves no purpose other
> than getting rate?
>
> In this case, I'd say we should de-register it from the clock driver and
> use debugcc [1] (contributions welcome!) for precise measurements.
>

Although currently there is no rate-setting happening now, its better to 
keep the rcg modelling as is, considering that it might be needed if 
some use case arises in future.

Re: [PATCH 2/3] clk: qcom: gcc-sm8150: Add gcc_parents_0_ao support

[Konrad Dybcio]

On 5.02.2024 09:37, Satya Priya Kakitapalli (Temp) wrote:
> 
> On 1/25/2024 3:25 PM, Konrad Dybcio wrote:
>>
>>
>> On 1/25/24 06:49, Satya Priya Kakitapalli (Temp) wrote:
>>>
>>> On 1/23/2024 11:17 PM, Konrad Dybcio wrote:
>>>>
>>>>
>>>> On 1/23/24 17:34, Satya Priya Kakitapalli wrote:
>>>>> Add active_only support for gcc_parents_0, this is needed because
>>>>> some of the clocks under it are critical which would vote on xo
>>>>> blocking the suspend.
>>>>>
>>>>> Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
>>>>> ---
>>>>
>>>> Is there a need to keep gcc_cpuss_ahb_clk_src around? Do we do any
>>>> ratesetting on it? Should we ever turn it off?
>>>>
>>>
>>> The branch clocks under gcc_cpuss_ahb_clk_src are critical clocks, which are running at 19.2Mhz causing vote on XO during suspend. As of now no rate setting is happening but this rcg is useful to get the exact rates from debugfs. Hence this change is needed to avoid XO shutdown issues.
>>
>> So, if I underderstood you correctly, this clock serves no purpose other
>> than getting rate?
>>
>> In this case, I'd say we should de-register it from the clock driver and
>> use debugcc [1] (contributions welcome!) for precise measurements.
>>
> 
> Although currently there is no rate-setting happening now, its better to keep the rcg modelling as is, considering that it might be needed if some use case arises in future.

We don't tend to solve imaginary problems, please lay out what it would be
used for, if at all. Then, we can assess whether it's necessary.

Konrad

Re: [PATCH 2/3] clk: qcom: gcc-sm8150: Add gcc_parents_0_ao support

[Satya Priya Kakitapalli (Temp)]

On 2/5/2024 5:03 PM, Konrad Dybcio wrote:
> On 5.02.2024 09:37, Satya Priya Kakitapalli (Temp) wrote:
>> On 1/25/2024 3:25 PM, Konrad Dybcio wrote:
>>>
>>> On 1/25/24 06:49, Satya Priya Kakitapalli (Temp) wrote:
>>>> On 1/23/2024 11:17 PM, Konrad Dybcio wrote:
>>>>>
>>>>> On 1/23/24 17:34, Satya Priya Kakitapalli wrote:
>>>>>> Add active_only support for gcc_parents_0, this is needed because
>>>>>> some of the clocks under it are critical which would vote on xo
>>>>>> blocking the suspend.
>>>>>>
>>>>>> Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
>>>>>> ---
>>>>> Is there a need to keep gcc_cpuss_ahb_clk_src around? Do we do any
>>>>> ratesetting on it? Should we ever turn it off?
>>>>>
>>>> The branch clocks under gcc_cpuss_ahb_clk_src are critical clocks, which are running at 19.2Mhz causing vote on XO during suspend. As of now no rate setting is happening but this rcg is useful to get the exact rates from debugfs. Hence this change is needed to avoid XO shutdown issues.
>>> So, if I underderstood you correctly, this clock serves no purpose other
>>> than getting rate?
>>>
>>> In this case, I'd say we should de-register it from the clock driver and
>>> use debugcc [1] (contributions welcome!) for precise measurements.
>>>
>> Although currently there is no rate-setting happening now, its better to keep the rcg modelling as is, considering that it might be needed if some use case arises in future.
> We don't tend to solve imaginary problems, please lay out what it would be
> used for, if at all. Then, we can assess whether it's necessary.


Okay, I'll de-register this RCG.