[RFC PATCH 0/4] cpufreq: qcom-hw: Move clocks to CPU node

[RFC PATCH 0/4] cpufreq: qcom-hw: Move clocks to CPU node

[Viresh Kumar]

Hi,

A recent patch series, targeting enhancements in the OPP core, ended up breaking
cpufreq on some of the Qualcomm platforms [1]. Necessary adjustments are made in
the OPP core, a bit hacky though, to get it working for now but it would be
better to solve the problem at hand in a cleaner way. And this patchset is an
attempt towards the same.

cpufreq-hw is a hardware engine, which takes care of frequency
management for CPUs. The engine manages the clocks for CPU devices, but
it isn't the end consumer of the clocks, which are the CPUs in this
case.

For this reason, it looks incorrect to keep the clock related properties
in the cpufreq-hw node. They should really be present at the end user,
i.e. the CPUs.

The case was simple currently as all the devices, i.e. the CPUs, that
the engine manages share the same clock names. What if the clock names
are different for different CPUs or clusters ? How will keeping the
clock properties in the cpufreq-hw node work in that case ?

This design creates further problems for frameworks like OPP, which
expects all such details (clocks) to be present in the end device node
itself, instead of another related node.

This patchset moves the clock properties to the node that uses them instead,
i.e. the CPU nodes and makes necessary adjustments at other places.

After this is applied, I can drop the unnecessary change from the OPP core, but
I wanted to discuss if this is a step in the right direction or not first and so
the RFC.

--
Viresh

[1] https://lore.kernel.org/lkml/YsxSkswzsqgMOc0l@hovoldconsulting.com/

Viresh Kumar (4):
  dt-bindings: cpufreq-qcom-hw: Move clocks to CPU nodes
  arm64: dts: qcom: Move clocks to CPU nodes
  cpufreq: qcom-cpufreq-hw: Clocks are moved to CPU nodes
  cpufreq: qcom-cpufreq-hw: Register config_clks helper

 .../bindings/cpufreq/cpufreq-qcom-hw.yaml     | 31 ++++----
 arch/arm64/boot/dts/qcom/sc7180.dtsi          | 19 ++++-
 arch/arm64/boot/dts/qcom/sc7280.dtsi          | 18 ++++-
 arch/arm64/boot/dts/qcom/sdm845.dtsi          | 19 ++++-
 arch/arm64/boot/dts/qcom/sm6350.dtsi          | 18 ++++-
 arch/arm64/boot/dts/qcom/sm8150.dtsi          | 19 ++++-
 arch/arm64/boot/dts/qcom/sm8250.dtsi          | 18 ++++-
 arch/arm64/boot/dts/qcom/sm8350.dtsi          | 19 ++++-
 arch/arm64/boot/dts/qcom/sm8450.dtsi          | 18 ++++-
 drivers/cpufreq/qcom-cpufreq-hw.c             | 75 ++++++++++++++-----
 10 files changed, 199 insertions(+), 55 deletions(-)

-- 
2.31.1.272.g89b43f80a514

[RFC PATCH 1/4] dt-bindings: cpufreq-qcom-hw: Move clocks to CPU nodes

[Viresh Kumar]

cpufreq-hw is a hardware engine, which takes care of frequency
management for CPUs. The engine manages the clocks for CPU devices, but
it isn't the end consumer of the clocks, which are the CPUs in this
case.

For this reason, it looks incorrect to keep the clock related properties
in the cpufreq-hw node. They should really be present at the end user,
i.e. the CPUs.

The case was simple currently as all the devices, i.e. the CPUs, that
the engine manages share the same clock names. What if the clock names
are different for different CPUs or clusters ? How will keeping the
clock properties in the cpufreq-hw node work in that case ?

This design creates further problems for frameworks like OPP, which
expects all such details (clocks) to be present in the end device node
itself, instead of another related node.

Move the clocks properties to the node that uses them instead.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 .../bindings/cpufreq/cpufreq-qcom-hw.yaml     | 31 ++++++++++---------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.yaml b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.yaml
index 2f1b8b6852a0..2ef4eeeca9b9 100644
--- a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.yaml
+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.yaml
@@ -42,24 +42,12 @@ description: |
       - const: freq-domain1
       - const: freq-domain2
 
-  clocks:
-    items:
-      - description: XO Clock
-      - description: GPLL0 Clock
-
-  clock-names:
-    items:
-      - const: xo
-      - const: alternate
-
   '#freq-domain-cells':
     const: 1
 
 required:
   - compatible
   - reg
-  - clocks
-  - clock-names
   - '#freq-domain-cells'
 
 additionalProperties: false
@@ -81,6 +69,8 @@ additionalProperties: false
         reg = <0x0 0x0>;
         enable-method = "psci";
         next-level-cache = <&L2_0>;
+        clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GPLL0>;
+        clock-names = "xo", "alternate";
         qcom,freq-domain = <&cpufreq_hw 0>;
         L2_0: l2-cache {
           compatible = "cache";
@@ -97,6 +87,8 @@ additionalProperties: false
         reg = <0x0 0x100>;
         enable-method = "psci";
         next-level-cache = <&L2_100>;
+        clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GPLL0>;
+        clock-names = "xo", "alternate";
         qcom,freq-domain = <&cpufreq_hw 0>;
         L2_100: l2-cache {
           compatible = "cache";
@@ -110,6 +102,8 @@ additionalProperties: false
         reg = <0x0 0x200>;
         enable-method = "psci";
         next-level-cache = <&L2_200>;
+        clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GPLL0>;
+        clock-names = "xo", "alternate";
         qcom,freq-domain = <&cpufreq_hw 0>;
         L2_200: l2-cache {
           compatible = "cache";
@@ -123,6 +117,8 @@ additionalProperties: false
         reg = <0x0 0x300>;
         enable-method = "psci";
         next-level-cache = <&L2_300>;
+        clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GPLL0>;
+        clock-names = "xo", "alternate";
         qcom,freq-domain = <&cpufreq_hw 0>;
         L2_300: l2-cache {
           compatible = "cache";
@@ -136,6 +132,8 @@ additionalProperties: false
         reg = <0x0 0x400>;
         enable-method = "psci";
         next-level-cache = <&L2_400>;
+        clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GPLL0>;
+        clock-names = "xo", "alternate";
         qcom,freq-domain = <&cpufreq_hw 1>;
         L2_400: l2-cache {
           compatible = "cache";
@@ -149,6 +147,8 @@ additionalProperties: false
         reg = <0x0 0x500>;
         enable-method = "psci";
         next-level-cache = <&L2_500>;
+        clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GPLL0>;
+        clock-names = "xo", "alternate";
         qcom,freq-domain = <&cpufreq_hw 1>;
         L2_500: l2-cache {
           compatible = "cache";
@@ -162,6 +162,8 @@ additionalProperties: false
         reg = <0x0 0x600>;
         enable-method = "psci";
         next-level-cache = <&L2_600>;
+        clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GPLL0>;
+        clock-names = "xo", "alternate";
         qcom,freq-domain = <&cpufreq_hw 1>;
         L2_600: l2-cache {
           compatible = "cache";
@@ -175,6 +177,8 @@ additionalProperties: false
         reg = <0x0 0x700>;
         enable-method = "psci";
         next-level-cache = <&L2_700>;
+        clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GPLL0>;
+        clock-names = "xo", "alternate";
         qcom,freq-domain = <&cpufreq_hw 1>;
         L2_700: l2-cache {
           compatible = "cache";
@@ -192,9 +196,6 @@ additionalProperties: false
         reg = <0x17d43000 0x1400>, <0x17d45800 0x1400>;
         reg-names = "freq-domain0", "freq-domain1";
 
-        clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GPLL0>;
-        clock-names = "xo", "alternate";
-
         #freq-domain-cells = <1>;
       };
     };
-- 
2.31.1.272.g89b43f80a514

[RFC PATCH 3/4] cpufreq: qcom-cpufreq-hw: Clocks are moved to CPU nodes

[Viresh Kumar]

The clocks are not in the cpufreq-hw node anymore, and are moved to the
respective CPU nodes. Make changes accordingly here.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/qcom-cpufreq-hw.c | 43 +++++++++++++++++--------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index 0253731d6d25..05fce4a559ca 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -57,9 +57,10 @@ struct qcom_cpufreq_data {
 	struct cpufreq_policy *policy;
 
 	bool per_core_dcvs;
+	unsigned long cpu_hw_rate;
+	unsigned long xo_rate;
 };
 
-static unsigned long cpu_hw_rate, xo_rate;
 static bool icc_scaling_enabled;
 
 static int qcom_cpufreq_set_bw(struct cpufreq_policy *policy,
@@ -209,9 +210,9 @@ static int qcom_cpufreq_hw_read_lut(struct device *cpu_dev,
 		volt = FIELD_GET(LUT_VOLT, data) * 1000;
 
 		if (src)
-			freq = xo_rate * lval / 1000;
+			freq = drv_data->xo_rate * lval / 1000;
 		else
-			freq = cpu_hw_rate / 1000;
+			freq = drv_data->cpu_hw_rate / 1000;
 
 		if (freq != prev_freq && core_count != LUT_TURBO_IND) {
 			if (!qcom_cpufreq_update_opp(cpu_dev, freq, volt)) {
@@ -293,7 +294,7 @@ static unsigned long qcom_lmh_get_throttle_freq(struct qcom_cpufreq_data *data)
 	else
 		lval = readl_relaxed(data->base + data->soc_data->reg_domain_state) & 0xff;
 
-	return lval * xo_rate;
+	return lval * data->xo_rate;
 }
 
 static void qcom_lmh_dcvs_notify(struct qcom_cpufreq_data *data)
@@ -480,6 +481,7 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 	struct device_node *cpu_np;
 	struct device *cpu_dev;
 	struct resource *res;
+	struct clk *clk;
 	void __iomem *base;
 	struct qcom_cpufreq_data *data;
 	int ret, index;
@@ -527,6 +529,24 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 		goto unmap_base;
 	}
 
+	clk = clk_get(cpu_dev, "xo");
+	if (IS_ERR(clk)) {
+		ret = PTR_ERR(clk);
+		goto error;
+	}
+
+	data->xo_rate = clk_get_rate(clk);
+	clk_put(clk);
+
+	clk = clk_get(cpu_dev, "alternate");
+	if (IS_ERR(clk)) {
+		ret = PTR_ERR(clk);
+		goto error;
+	}
+
+	data->cpu_hw_rate = clk_get_rate(clk) / CLK_HW_DIV;
+	clk_put(clk);
+
 	data->soc_data = of_device_get_match_data(&pdev->dev);
 	data->base = base;
 	data->res = res;
@@ -637,23 +657,8 @@ static struct cpufreq_driver cpufreq_qcom_hw_driver = {
 static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
 {
 	struct device *cpu_dev;
-	struct clk *clk;
 	int ret;
 
-	clk = clk_get(&pdev->dev, "xo");
-	if (IS_ERR(clk))
-		return PTR_ERR(clk);
-
-	xo_rate = clk_get_rate(clk);
-	clk_put(clk);
-
-	clk = clk_get(&pdev->dev, "alternate");
-	if (IS_ERR(clk))
-		return PTR_ERR(clk);
-
-	cpu_hw_rate = clk_get_rate(clk) / CLK_HW_DIV;
-	clk_put(clk);
-
 	cpufreq_qcom_hw_driver.driver_data = pdev;
 
 	/* Check for optional interconnect paths on CPU0 */
-- 
2.31.1.272.g89b43f80a514

[RFC PATCH 4/4] cpufreq: qcom-cpufreq-hw: Register config_clks helper

[Viresh Kumar]

There is a corner case with Qcom, where we want to skip clk
configuration that happens via dev_pm_opp_set_opp(), but still want the
OPP core to read the "opp-hz" property so we can find the right OPP via
freq finding helpers.

The OPP core provides support for the platforms to provide config_clks
helpers now, lets use that to provide an empty callback to skip clock
configuration.

The "table" wasn't getting freed properly on error, fix it as well which
we are updating the code.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/qcom-cpufreq-hw.c | 32 ++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index 05fce4a559ca..8d055a5f6575 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -59,6 +59,7 @@ struct qcom_cpufreq_data {
 	bool per_core_dcvs;
 	unsigned long cpu_hw_rate;
 	unsigned long xo_rate;
+	int opp_token;
 };
 
 static bool icc_scaling_enabled;
@@ -162,6 +163,15 @@ static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
 	return policy->freq_table[index].frequency;
 }
 
+static int qcom_cpufreq_hw_config_clks_nop(struct device *dev,
+					   struct opp_table *opp_table,
+					   struct dev_pm_opp *opp, void *data,
+					   bool scaling_down)
+{
+	/* We want to skip clk configuration via dev_pm_opp_set_opp() */
+	return 0;
+}
+
 static int qcom_cpufreq_hw_read_lut(struct device *cpu_dev,
 				    struct cpufreq_policy *policy)
 {
@@ -173,11 +183,23 @@ static int qcom_cpufreq_hw_read_lut(struct device *cpu_dev,
 	int ret;
 	struct qcom_cpufreq_data *drv_data = policy->driver_data;
 	const struct qcom_cpufreq_soc_data *soc_data = drv_data->soc_data;
+	const char * const clk_names[] = { "xo", NULL };
+	struct dev_pm_opp_config config = {
+		.clk_names = clk_names,
+		.config_clks = qcom_cpufreq_hw_config_clks_nop,
+	};
 
 	table = kcalloc(LUT_MAX_ENTRIES + 1, sizeof(*table), GFP_KERNEL);
 	if (!table)
 		return -ENOMEM;
 
+	ret = dev_pm_opp_set_config(cpu_dev, &config);
+	if (ret < 0) {
+		dev_err(cpu_dev, "Failed to set OPP config: %d\n", ret);
+		goto free_table;
+	}
+	drv_data->opp_token = ret;
+
 	ret = dev_pm_opp_of_add_table(cpu_dev);
 	if (!ret) {
 		/* Disable all opps and cross-validate against LUT later */
@@ -192,7 +214,7 @@ static int qcom_cpufreq_hw_read_lut(struct device *cpu_dev,
 		}
 	} else if (ret != -ENODEV) {
 		dev_err(cpu_dev, "Invalid opp table in device tree\n");
-		return ret;
+		goto clear_config;
 	} else {
 		policy->fast_switch_possible = true;
 		icc_scaling_enabled = false;
@@ -260,6 +282,13 @@ static int qcom_cpufreq_hw_read_lut(struct device *cpu_dev,
 	dev_pm_opp_set_sharing_cpus(cpu_dev, policy->cpus);
 
 	return 0;
+
+clear_config:
+	dev_pm_opp_clear_config(drv_data->opp_token);
+
+free_table:
+	kfree(table);
+	return ret;
 }
 
 static void qcom_get_related_cpus(int index, struct cpumask *m)
@@ -614,6 +643,7 @@ static int qcom_cpufreq_hw_cpu_exit(struct cpufreq_policy *policy)
 	dev_pm_opp_remove_all_dynamic(cpu_dev);
 	dev_pm_opp_of_cpumask_remove_table(policy->related_cpus);
 	qcom_cpufreq_hw_lmh_exit(data);
+	dev_pm_opp_clear_config(data->opp_token);
 	kfree(policy->freq_table);
 	kfree(data);
 	iounmap(base);
-- 
2.31.1.272.g89b43f80a514

Re: [RFC PATCH 0/4] cpufreq: qcom-hw: Move clocks to CPU node

[Manivannan Sadhasivam]

On Wed, Jul 13, 2022 at 12:22:55PM +0530, Viresh Kumar wrote:
> Hi,
> 
> A recent patch series, targeting enhancements in the OPP core, ended up breaking
> cpufreq on some of the Qualcomm platforms [1]. Necessary adjustments are made in
> the OPP core, a bit hacky though, to get it working for now but it would be
> better to solve the problem at hand in a cleaner way. And this patchset is an
> attempt towards the same.
> 
> cpufreq-hw is a hardware engine, which takes care of frequency
> management for CPUs. The engine manages the clocks for CPU devices, but
> it isn't the end consumer of the clocks, which are the CPUs in this
> case.
> 
> For this reason, it looks incorrect to keep the clock related properties
> in the cpufreq-hw node. They should really be present at the end user,
> i.e. the CPUs.
> 
> The case was simple currently as all the devices, i.e. the CPUs, that
> the engine manages share the same clock names. What if the clock names
> are different for different CPUs or clusters ? How will keeping the
> clock properties in the cpufreq-hw node work in that case ?
> 
> This design creates further problems for frameworks like OPP, which
> expects all such details (clocks) to be present in the end device node
> itself, instead of another related node.
> 
> This patchset moves the clock properties to the node that uses them instead,
> i.e. the CPU nodes and makes necessary adjustments at other places.
> 
> After this is applied, I can drop the unnecessary change from the OPP core, but
> I wanted to discuss if this is a step in the right direction or not first and so
> the RFC.
> 

The clocks defined in the devicetree currently (CXO, GPLL) are the source
clocks of the EPSS block (cpufreq-hw). And EPSS will supply clock and
voltage through other blocks to the CPU domains. Even though the end
consumer of the source clocks are the CPUs, those clocks are not
directly reachign the CPUs but instead through some other blocks in EPSS.

Initially I was temped to add cpufreq-hw as the clock provider and have
it source clocks to the individual CPUs. This somehow models the clock
topology also, but after having a discussion with Bjorn we concluded that
it is best to leave it as it is.

The main issue that Bjorn pointed out was the fact that the clocks coming
out of EPSS are not exactly of the same frequency that was requested.
EPSS will do its own logic to generate the clocks to the CPUs based on
the input frequency vote and limits.

Thanks,
Mani

> --
> Viresh
> 
> [1] https://lore.kernel.org/lkml/YsxSkswzsqgMOc0l@hovoldconsulting.com/
> 
> Viresh Kumar (4):
>   dt-bindings: cpufreq-qcom-hw: Move clocks to CPU nodes
>   arm64: dts: qcom: Move clocks to CPU nodes
>   cpufreq: qcom-cpufreq-hw: Clocks are moved to CPU nodes
>   cpufreq: qcom-cpufreq-hw: Register config_clks helper
> 
>  .../bindings/cpufreq/cpufreq-qcom-hw.yaml     | 31 ++++----
>  arch/arm64/boot/dts/qcom/sc7180.dtsi          | 19 ++++-
>  arch/arm64/boot/dts/qcom/sc7280.dtsi          | 18 ++++-
>  arch/arm64/boot/dts/qcom/sdm845.dtsi          | 19 ++++-
>  arch/arm64/boot/dts/qcom/sm6350.dtsi          | 18 ++++-
>  arch/arm64/boot/dts/qcom/sm8150.dtsi          | 19 ++++-
>  arch/arm64/boot/dts/qcom/sm8250.dtsi          | 18 ++++-
>  arch/arm64/boot/dts/qcom/sm8350.dtsi          | 19 ++++-
>  arch/arm64/boot/dts/qcom/sm8450.dtsi          | 18 ++++-
>  drivers/cpufreq/qcom-cpufreq-hw.c             | 75 ++++++++++++++-----
>  10 files changed, 199 insertions(+), 55 deletions(-)
> 
> -- 
> 2.31.1.272.g89b43f80a514
> 

Re: [RFC PATCH 0/4] cpufreq: qcom-hw: Move clocks to CPU node

[Viresh Kumar]

On 15-07-22, 21:39, Manivannan Sadhasivam wrote:
> The clocks defined in the devicetree currently (CXO, GPLL) are the source
> clocks of the EPSS block (cpufreq-hw). And EPSS will supply clock and
> voltage through other blocks to the CPU domains. Even though the end
> consumer of the source clocks are the CPUs, those clocks are not
> directly reachign the CPUs but instead through some other blocks in EPSS.

Fair enough, o these clocks should be present in the cpufreq-hw node,
as there were.

> Initially I was temped to add cpufreq-hw as the clock provider and have
> it source clocks to the individual CPUs. This somehow models the clock
> topology also

Right.

> , but after having a discussion with Bjorn we concluded that
> it is best to leave it as it is.
> 
> The main issue that Bjorn pointed out was the fact that the clocks coming
> out of EPSS are not exactly of the same frequency that was requested.
> EPSS will do its own logic to generate the clocks to the CPUs based on
> the input frequency vote and limits.

The OPP tables, which are part of the CPU nodes, mentions clock rates.
Are these values for the cxo/gpll clocks or the clock that reaches the
CPUs? I believe the latter. The DT is not really complete if the CPU
node mentions the frequency, but not the source clock. It works for
you because you don't want to do clk_set_rate() in this case, but then
it leaves other frameworks, like OPP, confused and rightly so.

Normally, there is always a difference in what the OPP table contains
as frequency value and what the hardware programs, mostly it is small
though. It shouldn't prevent us from having the hierarchy clearly
defined in the DT.

Based on your description, I think it would be better to make
cpufreq-hw a clock provider and CPUs the consumer of it. It would then
allow the OPP core to not carry the hack to make it all work.

-- 
viresh

Re: [RFC PATCH 1/4] dt-bindings: cpufreq-qcom-hw: Move clocks to CPU nodes

[Rob Herring]

On Wed, Jul 13, 2022 at 12:22:56PM +0530, Viresh Kumar wrote:
> cpufreq-hw is a hardware engine, which takes care of frequency
> management for CPUs. The engine manages the clocks for CPU devices, but
> it isn't the end consumer of the clocks, which are the CPUs in this
> case.

The question is really where does the clock mux live?

> For this reason, it looks incorrect to keep the clock related properties
> in the cpufreq-hw node. They should really be present at the end user,
> i.e. the CPUs.

The issue is that the CPU itself probably only has 1 clock input (at 
least for its core frequency). Listing out all possible clock sources in 
CPU node 'clocks' is wrong too.

> The case was simple currently as all the devices, i.e. the CPUs, that
> the engine manages share the same clock names. What if the clock names
> are different for different CPUs or clusters ? How will keeping the
> clock properties in the cpufreq-hw node work in that case ?
> 
> This design creates further problems for frameworks like OPP, which
> expects all such details (clocks) to be present in the end device node
> itself, instead of another related node.
> 
> Move the clocks properties to the node that uses them instead.

What's the purpose of freq-domain binding now? I thought the idea was to 
use that instead of clocks directly.

Rob

Re: [RFC PATCH 1/4] dt-bindings: cpufreq-qcom-hw: Move clocks to CPU nodes

[Viresh Kumar]

On 18-07-22, 14:46, Rob Herring wrote:
> On Wed, Jul 13, 2022 at 12:22:56PM +0530, Viresh Kumar wrote:
> > cpufreq-hw is a hardware engine, which takes care of frequency
> > management for CPUs. The engine manages the clocks for CPU devices, but
> > it isn't the end consumer of the clocks, which are the CPUs in this
> > case.
> 
> The question is really where does the clock mux live?

As Manivannan clarified in another email, these clocks are actually consumed by
the cpufreq-hw node, so existing code was fine.

> > For this reason, it looks incorrect to keep the clock related properties
> > in the cpufreq-hw node. They should really be present at the end user,
> > i.e. the CPUs.
> 
> The issue is that the CPU itself probably only has 1 clock input (at 
> least for its core frequency).

Right, and they (Qcom) have skipped adding that in DT currently. I have
suggested to him to add it there, which will solve the issue as well.

> Listing out all possible clock sources in CPU node 'clocks' is wrong too.

Yes, we need to mention only the clocks which are consumed directly by the CPU,
maybe just one of them which comes out of cpufreq-hw node.

> > The case was simple currently as all the devices, i.e. the CPUs, that
> > the engine manages share the same clock names. What if the clock names
> > are different for different CPUs or clusters ? How will keeping the
> > clock properties in the cpufreq-hw node work in that case ?
> > 
> > This design creates further problems for frameworks like OPP, which
> > expects all such details (clocks) to be present in the end device node
> > itself, instead of another related node.
> > 
> > Move the clocks properties to the node that uses them instead.
> 
> What's the purpose of freq-domain binding now? I thought the idea was to 
> use that instead of clocks directly.

Not always I think. It provides register access for programming or voting for
the clock, etc. Yes, the code won't do clk_set_rate() but the DT should still
mention the clock in the CPU node if it mentions an OPP table with frequencies
in it.

-- 
viresh

Re: [RFC PATCH 0/4] cpufreq: qcom-hw: Move clocks to CPU node

[Viresh Kumar]

On 18-07-22, 07:27, Viresh Kumar wrote:
> The OPP tables, which are part of the CPU nodes, mentions clock rates.
> Are these values for the cxo/gpll clocks or the clock that reaches the
> CPUs? I believe the latter. The DT is not really complete if the CPU
> node mentions the frequency, but not the source clock. It works for
> you because you don't want to do clk_set_rate() in this case, but then
> it leaves other frameworks, like OPP, confused and rightly so.
> 
> Normally, there is always a difference in what the OPP table contains
> as frequency value and what the hardware programs, mostly it is small
> though. It shouldn't prevent us from having the hierarchy clearly
> defined in the DT.
> 
> Based on your description, I think it would be better to make
> cpufreq-hw a clock provider and CPUs the consumer of it. It would then
> allow the OPP core to not carry the hack to make it all work.

Bjorn / Mani,

Can we please get this sorted out ? I don't want to carry an unnecessary hack in
the OPP core for this.

-- 
viresh

Re: [RFC PATCH 0/4] cpufreq: qcom-hw: Move clocks to CPU node

[Manivannan Sadhasivam]

On Mon, Aug 01, 2022 at 08:07:56AM +0530, Viresh Kumar wrote:
> On 18-07-22, 07:27, Viresh Kumar wrote:
> > The OPP tables, which are part of the CPU nodes, mentions clock rates.
> > Are these values for the cxo/gpll clocks or the clock that reaches the
> > CPUs? I believe the latter. The DT is not really complete if the CPU
> > node mentions the frequency, but not the source clock. It works for
> > you because you don't want to do clk_set_rate() in this case, but then
> > it leaves other frameworks, like OPP, confused and rightly so.
> > 
> > Normally, there is always a difference in what the OPP table contains
> > as frequency value and what the hardware programs, mostly it is small
> > though. It shouldn't prevent us from having the hierarchy clearly
> > defined in the DT.
> > 
> > Based on your description, I think it would be better to make
> > cpufreq-hw a clock provider and CPUs the consumer of it. It would then
> > allow the OPP core to not carry the hack to make it all work.
> 
> Bjorn / Mani,
> 
> Can we please get this sorted out ? I don't want to carry an unnecessary hack in
> the OPP core for this.
> 

I'm waiting for inputs from Bjorn.

@Bjorn: What do you think of the proposal to add qcom-cpufreq-hw as the clk
provider for CPUs?

Thanks,
Mani

> -- 
> viresh

-- 
மணிவண்ணன் சதாசிவம்

Re: [RFC PATCH 0/4] cpufreq: qcom-hw: Move clocks to CPU node

[Bjorn Andersson]

On Mon, Aug 01, 2022 at 08:07:56AM +0530, Viresh Kumar wrote:
> On 18-07-22, 07:27, Viresh Kumar wrote:
> > The OPP tables, which are part of the CPU nodes, mentions clock rates.
> > Are these values for the cxo/gpll clocks or the clock that reaches the
> > CPUs? I believe the latter. The DT is not really complete if the CPU
> > node mentions the frequency, but not the source clock. It works for
> > you because you don't want to do clk_set_rate() in this case, but then
> > it leaves other frameworks, like OPP, confused and rightly so.
> > 
> > Normally, there is always a difference in what the OPP table contains
> > as frequency value and what the hardware programs, mostly it is small
> > though. It shouldn't prevent us from having the hierarchy clearly
> > defined in the DT.
> > 
> > Based on your description, I think it would be better to make
> > cpufreq-hw a clock provider and CPUs the consumer of it. It would then
> > allow the OPP core to not carry the hack to make it all work.
> 
> Bjorn / Mani,
> 
> Can we please get this sorted out ? I don't want to carry an unnecessary hack in
> the OPP core for this.
> 

Conceptually, it sounds like a good idea to express the clock feeding
the CPU clusters, which is controlled by the OSM/EPSS.  But do you
expect the OPP framework to actually do something with the clock, or
just to ensure that the relationship is properly described?


FWIW, the possible discrepancy between the requested frequency and the
actual frequency comes from the fact that OSM/EPSS throttles the cluster
frequency based on a number of different factors (thermal, voltages
...).
This is reported back to the kernel using the thermal pressure
interface. It would be quite interesting to see some investigation in
how efficient the kernel is at making use of this feedback.

Regards,
Bjorn

Re: [RFC PATCH 0/4] cpufreq: qcom-hw: Move clocks to CPU node

[Viresh Kumar]

On 29-08-22, 22:24, Bjorn Andersson wrote:
> Conceptually, it sounds like a good idea to express the clock feeding
> the CPU clusters, which is controlled by the OSM/EPSS.  But do you
> expect the OPP framework to actually do something with the clock, or
> just to ensure that the relationship is properly described?

No, the OPP core will never try to set the clock rate in your case,
though it will do clk_get().

> FWIW, the possible discrepancy between the requested frequency and the
> actual frequency comes from the fact that OSM/EPSS throttles the cluster
> frequency based on a number of different factors (thermal, voltages
> ...).
> This is reported back to the kernel using the thermal pressure
> interface. It would be quite interesting to see some investigation in
> how efficient the kernel is at making use of this feedback.

-- 
viresh

Re: [RFC PATCH 0/4] cpufreq: qcom-hw: Move clocks to CPU node

[Manivannan Sadhasivam]

On Tue, Aug 30, 2022 at 11:10:42AM +0530, Viresh Kumar wrote:
> On 29-08-22, 22:24, Bjorn Andersson wrote:
> > Conceptually, it sounds like a good idea to express the clock feeding
> > the CPU clusters, which is controlled by the OSM/EPSS.  But do you
> > expect the OPP framework to actually do something with the clock, or
> > just to ensure that the relationship is properly described?
> 
> No, the OPP core will never try to set the clock rate in your case,
> though it will do clk_get().
> 

Okay. Then I think it is a fair argument to make qcom-cpufreq-hw as the
clock provider for CPUs.

I will send the RFC soon.

Thanks,
Mani

> > FWIW, the possible discrepancy between the requested frequency and the
> > actual frequency comes from the fact that OSM/EPSS throttles the cluster
> > frequency based on a number of different factors (thermal, voltages
> > ...).
> > This is reported back to the kernel using the thermal pressure
> > interface. It would be quite interesting to see some investigation in
> > how efficient the kernel is at making use of this feedback.
> 
> -- 
> viresh

Re: [RFC PATCH 0/4] cpufreq: qcom-hw: Move clocks to CPU node

[Viresh Kumar]

On 30-08-22, 11:50, Manivannan Sadhasivam wrote:
> On Tue, Aug 30, 2022 at 11:10:42AM +0530, Viresh Kumar wrote:
> > On 29-08-22, 22:24, Bjorn Andersson wrote:
> > > Conceptually, it sounds like a good idea to express the clock feeding
> > > the CPU clusters, which is controlled by the OSM/EPSS.  But do you
> > > expect the OPP framework to actually do something with the clock, or
> > > just to ensure that the relationship is properly described?
> > 
> > No, the OPP core will never try to set the clock rate in your case,
> > though it will do clk_get().
> > 
> 
> Okay. Then I think it is a fair argument to make qcom-cpufreq-hw as the
> clock provider for CPUs.
> 
> I will send the RFC soon.

Ping.

-- 
viresh

Re: [RFC PATCH 0/4] cpufreq: qcom-hw: Move clocks to CPU node

[Manivannan Sadhasivam]

On Tue, Sep 20, 2022 at 03:58:03PM +0530, Viresh Kumar wrote:
> On 30-08-22, 11:50, Manivannan Sadhasivam wrote:
> > On Tue, Aug 30, 2022 at 11:10:42AM +0530, Viresh Kumar wrote:
> > > On 29-08-22, 22:24, Bjorn Andersson wrote:
> > > > Conceptually, it sounds like a good idea to express the clock feeding
> > > > the CPU clusters, which is controlled by the OSM/EPSS.  But do you
> > > > expect the OPP framework to actually do something with the clock, or
> > > > just to ensure that the relationship is properly described?
> > > 
> > > No, the OPP core will never try to set the clock rate in your case,
> > > though it will do clk_get().
> > > 
> > 
> > Okay. Then I think it is a fair argument to make qcom-cpufreq-hw as the
> > clock provider for CPUs.
> > 
> > I will send the RFC soon.
> 
> Ping.
> 

Didn't get time so far. Will get to this once I'm back from vacation.

Thanks,
Mani

> -- 
> viresh

-- 
மணிவண்ணன் சதாசிவம்