All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support
@ 2022-11-17  5:31 Manivannan Sadhasivam
  2022-11-17  5:31 ` [PATCH v7 1/4] dt-bindings: cpufreq: cpufreq-qcom-hw: Add cpufreq clock provider Manivannan Sadhasivam
                   ` (6 more replies)
  0 siblings, 7 replies; 21+ messages in thread
From: Manivannan Sadhasivam @ 2022-11-17  5:31 UTC (permalink / raw)
  To: andersson, viresh.kumar, krzysztof.kozlowski+dt, rafael, robh+dt
  Cc: johan, devicetree, linux-arm-msm, linux-kernel, linux-pm,
	Manivannan Sadhasivam

Hello,

This series adds clock provider support to the Qcom CPUFreq driver for
supplying the clocks to the CPU cores in Qcom SoCs.

The Qualcomm platforms making use of CPUFreq HW Engine (EPSS/OSM) supply
clocks to the CPU cores. But this is not represented clearly in devicetree.
There is no clock coming out of the CPUFreq HW node to the CPU. This created
an issue [1] with the OPP core when a recent enhancement series was submitted.
Eventhough the issue got fixed in the OPP framework in the meantime, that's
not a proper solution and this series aims to fix it properly.

There was also an attempt made by Viresh [2] to fix the issue by moving the
clocks supplied to the CPUFreq HW node to the CPU. But that was not accepted
since those clocks belong to the CPUFreq HW node only.

The proposal here is to add clock provider support to the Qcom CPUFreq HW
driver to supply clocks to the CPUs that comes out of the EPSS/OSM block.
This correctly reflects the hardware implementation.

The clock provider is a simple one that just provides the frequency of the
clocks supplied to each frequency domain in the SoC using .recalc_rate()
callback. The frequency supplied by the driver will be the actual frequency
that comes out of the EPSS/OSM block after the DCVS operation. This frequency
is not same as what the CPUFreq framework has set but it is the one that gets
supplied to the CPUs after throttling by LMh.

This series has been tested on SM8450 based dev board with the OPP hack removed
and hence there is a DTS change only for that platform. Once this series gets
accepted, rest of the platform DTS can also be modified and finally the hack on
the OPP core can be dropped.

Thanks,
Mani

[1] https://lore.kernel.org/lkml/YsxSkswzsqgMOc0l@hovoldconsulting.com/
[2] https://lore.kernel.org/lkml/20220801054255.GA12039@thinkpad/t/

Changes in v7:

* Added a patch that returns the throttled frequency for cpufreq_driver->get()
  callback (Sudeep & Viresh)
* Added error check for kasprintf and allocated the clk name locally

Changes in v6:

* Removed the local variable clk_name (Matthias)
* Added the clock id to the error message of devm_clk_hw_register()

Changes in v5:

* Switched to Hz unit for the CPU clocks

Changes in v4:

* Rebased on top of cpufreq/arm/linux-next branch

Changes in v3:

* Submitted the cpufreq driver cleanup patches as a separate series as
  suggested by Viresh
* Removed static keyword from clk_init_data declaration

Changes in v2:

* Moved the qcom_cpufreq_data allocation to probe
* Added single clock provider with multiple clks for each freq domain
* Moved soc_data to qcom_cpufreq struct
* Added Rob's review for binding

Manivannan Sadhasivam (4):
  dt-bindings: cpufreq: cpufreq-qcom-hw: Add cpufreq clock provider
  arm64: dts: qcom: sm8450: Supply clock from cpufreq node to CPUs
  cpufreq: qcom-hw: Add CPU clock provider support
  cpufreq: qcom-hw: Fix the frequency returned by cpufreq_driver->get()

 .../bindings/cpufreq/cpufreq-qcom-hw.yaml     | 12 +++
 arch/arm64/boot/dts/qcom/sm8450.dtsi          |  9 ++
 drivers/cpufreq/qcom-cpufreq-hw.c             | 87 ++++++++++++++++---
 3 files changed, 95 insertions(+), 13 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v7 1/4] dt-bindings: cpufreq: cpufreq-qcom-hw: Add cpufreq clock provider
  2022-11-17  5:31 [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support Manivannan Sadhasivam
@ 2022-11-17  5:31 ` Manivannan Sadhasivam
  2022-11-17  5:31 ` [PATCH v7 2/4] arm64: dts: qcom: sm8450: Supply clock from cpufreq node to CPUs Manivannan Sadhasivam
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 21+ messages in thread
From: Manivannan Sadhasivam @ 2022-11-17  5:31 UTC (permalink / raw)
  To: andersson, viresh.kumar, krzysztof.kozlowski+dt, rafael, robh+dt
  Cc: johan, devicetree, linux-arm-msm, linux-kernel, linux-pm,
	Manivannan Sadhasivam, Rob Herring

Qualcomm platforms making use of CPUFreq HW Engine (EPSS/OSM) supply clocks
to the CPU cores. Document the same in the binding to reflect the actual
implementation.

CPUFreq HW will become the clock provider and CPU cores will become the
clock consumers.

The clock index for each CPU core is based on the frequency domain index.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/cpufreq/cpufreq-qcom-hw.yaml | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.yaml b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.yaml
index e58c55f78aaa..676d369a6fdd 100644
--- a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.yaml
+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.yaml
@@ -56,6 +56,9 @@ properties:
   '#freq-domain-cells':
     const: 1
 
+  '#clock-cells':
+    const: 1
+
 required:
   - compatible
   - reg
@@ -83,6 +86,7 @@ examples:
         enable-method = "psci";
         next-level-cache = <&L2_0>;
         qcom,freq-domain = <&cpufreq_hw 0>;
+        clocks = <&cpufreq_hw 0>;
         L2_0: l2-cache {
           compatible = "cache";
           cache-unified;
@@ -103,6 +107,7 @@ examples:
         enable-method = "psci";
         next-level-cache = <&L2_100>;
         qcom,freq-domain = <&cpufreq_hw 0>;
+        clocks = <&cpufreq_hw 0>;
         L2_100: l2-cache {
           compatible = "cache";
           cache-unified;
@@ -118,6 +123,7 @@ examples:
         enable-method = "psci";
         next-level-cache = <&L2_200>;
         qcom,freq-domain = <&cpufreq_hw 0>;
+        clocks = <&cpufreq_hw 0>;
         L2_200: l2-cache {
           compatible = "cache";
           cache-unified;
@@ -133,6 +139,7 @@ examples:
         enable-method = "psci";
         next-level-cache = <&L2_300>;
         qcom,freq-domain = <&cpufreq_hw 0>;
+        clocks = <&cpufreq_hw 0>;
         L2_300: l2-cache {
           compatible = "cache";
           cache-unified;
@@ -148,6 +155,7 @@ examples:
         enable-method = "psci";
         next-level-cache = <&L2_400>;
         qcom,freq-domain = <&cpufreq_hw 1>;
+        clocks = <&cpufreq_hw 1>;
         L2_400: l2-cache {
           compatible = "cache";
           cache-unified;
@@ -163,6 +171,7 @@ examples:
         enable-method = "psci";
         next-level-cache = <&L2_500>;
         qcom,freq-domain = <&cpufreq_hw 1>;
+        clocks = <&cpufreq_hw 1>;
         L2_500: l2-cache {
           compatible = "cache";
           cache-unified;
@@ -178,6 +187,7 @@ examples:
         enable-method = "psci";
         next-level-cache = <&L2_600>;
         qcom,freq-domain = <&cpufreq_hw 1>;
+        clocks = <&cpufreq_hw 1>;
         L2_600: l2-cache {
           compatible = "cache";
           cache-unified;
@@ -193,6 +203,7 @@ examples:
         enable-method = "psci";
         next-level-cache = <&L2_700>;
         qcom,freq-domain = <&cpufreq_hw 1>;
+        clocks = <&cpufreq_hw 1>;
         L2_700: l2-cache {
           compatible = "cache";
           cache-unified;
@@ -215,6 +226,7 @@ examples:
         clock-names = "xo", "alternate";
 
         #freq-domain-cells = <1>;
+        #clock-cells = <1>;
       };
     };
 ...
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v7 2/4] arm64: dts: qcom: sm8450: Supply clock from cpufreq node to CPUs
  2022-11-17  5:31 [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support Manivannan Sadhasivam
  2022-11-17  5:31 ` [PATCH v7 1/4] dt-bindings: cpufreq: cpufreq-qcom-hw: Add cpufreq clock provider Manivannan Sadhasivam
@ 2022-11-17  5:31 ` Manivannan Sadhasivam
  2022-11-17  5:31 ` [PATCH v7 3/4] cpufreq: qcom-hw: Add CPU clock provider support Manivannan Sadhasivam
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 21+ messages in thread
From: Manivannan Sadhasivam @ 2022-11-17  5:31 UTC (permalink / raw)
  To: andersson, viresh.kumar, krzysztof.kozlowski+dt, rafael, robh+dt
  Cc: johan, devicetree, linux-arm-msm, linux-kernel, linux-pm,
	Manivannan Sadhasivam

Qualcomm platforms making use of CPUFreq HW Engine (EPSS/OSM) supply clocks
to the CPU cores. But this relationship is not represented in DTS so far.

So let's make cpufreq node as the clock provider and CPU nodes as the
consumers. The clock index for each CPU node is based on the frequency
domain index.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8450.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
index d32f08df743d..234d2722a4fa 100644
--- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
@@ -51,6 +51,7 @@ CPU0: cpu@0 {
 			power-domain-names = "psci";
 			qcom,freq-domain = <&cpufreq_hw 0>;
 			#cooling-cells = <2>;
+			clocks = <&cpufreq_hw 0>;
 			L2_0: l2-cache {
 			      compatible = "cache";
 			      next-level-cache = <&L3_0>;
@@ -70,6 +71,7 @@ CPU1: cpu@100 {
 			power-domain-names = "psci";
 			qcom,freq-domain = <&cpufreq_hw 0>;
 			#cooling-cells = <2>;
+			clocks = <&cpufreq_hw 0>;
 			L2_100: l2-cache {
 			      compatible = "cache";
 			      next-level-cache = <&L3_0>;
@@ -86,6 +88,7 @@ CPU2: cpu@200 {
 			power-domain-names = "psci";
 			qcom,freq-domain = <&cpufreq_hw 0>;
 			#cooling-cells = <2>;
+			clocks = <&cpufreq_hw 0>;
 			L2_200: l2-cache {
 			      compatible = "cache";
 			      next-level-cache = <&L3_0>;
@@ -102,6 +105,7 @@ CPU3: cpu@300 {
 			power-domain-names = "psci";
 			qcom,freq-domain = <&cpufreq_hw 0>;
 			#cooling-cells = <2>;
+			clocks = <&cpufreq_hw 0>;
 			L2_300: l2-cache {
 			      compatible = "cache";
 			      next-level-cache = <&L3_0>;
@@ -118,6 +122,7 @@ CPU4: cpu@400 {
 			power-domain-names = "psci";
 			qcom,freq-domain = <&cpufreq_hw 1>;
 			#cooling-cells = <2>;
+			clocks = <&cpufreq_hw 1>;
 			L2_400: l2-cache {
 			      compatible = "cache";
 			      next-level-cache = <&L3_0>;
@@ -134,6 +139,7 @@ CPU5: cpu@500 {
 			power-domain-names = "psci";
 			qcom,freq-domain = <&cpufreq_hw 1>;
 			#cooling-cells = <2>;
+			clocks = <&cpufreq_hw 1>;
 			L2_500: l2-cache {
 			      compatible = "cache";
 			      next-level-cache = <&L3_0>;
@@ -151,6 +157,7 @@ CPU6: cpu@600 {
 			power-domain-names = "psci";
 			qcom,freq-domain = <&cpufreq_hw 1>;
 			#cooling-cells = <2>;
+			clocks = <&cpufreq_hw 1>;
 			L2_600: l2-cache {
 			      compatible = "cache";
 			      next-level-cache = <&L3_0>;
@@ -167,6 +174,7 @@ CPU7: cpu@700 {
 			power-domain-names = "psci";
 			qcom,freq-domain = <&cpufreq_hw 2>;
 			#cooling-cells = <2>;
+			clocks = <&cpufreq_hw 2>;
 			L2_700: l2-cache {
 			      compatible = "cache";
 			      next-level-cache = <&L3_0>;
@@ -3075,6 +3083,7 @@ cpufreq_hw: cpufreq@17d91000 {
 				     <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>;
 			interrupt-names = "dcvsh-irq-0", "dcvsh-irq-1", "dcvsh-irq-2";
 			#freq-domain-cells = <1>;
+			#clock-cells = <1>;
 		};
 
 		gem_noc: interconnect@19100000 {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v7 3/4] cpufreq: qcom-hw: Add CPU clock provider support
  2022-11-17  5:31 [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support Manivannan Sadhasivam
  2022-11-17  5:31 ` [PATCH v7 1/4] dt-bindings: cpufreq: cpufreq-qcom-hw: Add cpufreq clock provider Manivannan Sadhasivam
  2022-11-17  5:31 ` [PATCH v7 2/4] arm64: dts: qcom: sm8450: Supply clock from cpufreq node to CPUs Manivannan Sadhasivam
@ 2022-11-17  5:31 ` Manivannan Sadhasivam
  2022-11-17  5:31 ` [PATCH v7 4/4] cpufreq: qcom-hw: Fix the frequency returned by cpufreq_driver->get() Manivannan Sadhasivam
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 21+ messages in thread
From: Manivannan Sadhasivam @ 2022-11-17  5:31 UTC (permalink / raw)
  To: andersson, viresh.kumar, krzysztof.kozlowski+dt, rafael, robh+dt
  Cc: johan, devicetree, linux-arm-msm, linux-kernel, linux-pm,
	Manivannan Sadhasivam

Qcom CPUFreq hardware (EPSS/OSM) controls clock and voltage to the CPU
cores. But this relationship is not represented with the clk framework
so far.

So, let's make the qcom-cpufreq-hw driver a clock provider. This makes the
clock producer/consumer relationship cleaner and is also useful for CPU
related frameworks like OPP to know the frequency at which the CPUs are
running.

The clock frequency provided by the driver is for each frequency domain.
We cannot get the frequency of each CPU core because, not all platforms
support per-core DCVS feature.

Also the frequency supplied by the driver is the actual frequency that
comes out of the EPSS/OSM block after the DCVS operation. This frequency is
not same as what the CPUFreq framework has set but it is the one that gets
supplied to the CPUs after throttling by LMh.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/cpufreq/qcom-cpufreq-hw.c | 45 +++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index 5e0598730a04..1faa325d3e52 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/clk-provider.h>
 #include <linux/cpufreq.h>
 #include <linux/init.h>
 #include <linux/interconnect.h>
@@ -54,6 +55,7 @@ struct qcom_cpufreq_data {
 	bool cancel_throttle;
 	struct delayed_work throttle_work;
 	struct cpufreq_policy *policy;
+	struct clk_hw cpu_clk;
 
 	bool per_core_dcvs;
 
@@ -615,8 +617,20 @@ static struct cpufreq_driver cpufreq_qcom_hw_driver = {
 	.ready		= qcom_cpufreq_ready,
 };
 
+static unsigned long qcom_cpufreq_hw_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
+{
+	struct qcom_cpufreq_data *data = container_of(hw, struct qcom_cpufreq_data, cpu_clk);
+
+	return qcom_lmh_get_throttle_freq(data);
+}
+
+static const struct clk_ops qcom_cpufreq_hw_clk_ops = {
+	.recalc_rate = qcom_cpufreq_hw_recalc_rate,
+};
+
 static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
 {
+	struct clk_hw_onecell_data *clk_data;
 	struct device *dev = &pdev->dev;
 	struct device *cpu_dev;
 	struct clk *clk;
@@ -659,8 +673,15 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
 
 	qcom_cpufreq.soc_data = of_device_get_match_data(dev);
 
+	clk_data = devm_kzalloc(dev, struct_size(clk_data, hws, num_domains), GFP_KERNEL);
+	if (!clk_data)
+		return -ENOMEM;
+
+	clk_data->num = num_domains;
+
 	for (i = 0; i < num_domains; i++) {
 		struct qcom_cpufreq_data *data = &qcom_cpufreq.data[i];
+		struct clk_init_data clk_init = {};
 		struct resource *res;
 		void __iomem *base;
 
@@ -672,6 +693,30 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
 
 		data->base = base;
 		data->res = res;
+
+		/* Register CPU clock for each frequency domain */
+		clk_init.name = kasprintf(GFP_KERNEL, "qcom_cpufreq%d", i);
+		if (!clk_init.name)
+			return -ENOMEM;
+
+		clk_init.flags = CLK_GET_RATE_NOCACHE;
+		clk_init.ops = &qcom_cpufreq_hw_clk_ops;
+		data->cpu_clk.init = &clk_init;
+
+		ret = devm_clk_hw_register(dev, &data->cpu_clk);
+		if (ret < 0) {
+			dev_err(dev, "Failed to register clock %d: %d\n", i, ret);
+			return ret;
+		}
+
+		clk_data->hws[i] = &data->cpu_clk;
+		kfree(clk_init.name);
+	}
+
+	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data);
+	if (ret < 0) {
+		dev_err(dev, "Failed to add clock provider\n");
+		return ret;
 	}
 
 	ret = cpufreq_register_driver(&cpufreq_qcom_hw_driver);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH v7 4/4] cpufreq: qcom-hw: Fix the frequency returned by cpufreq_driver->get()
  2022-11-17  5:31 [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support Manivannan Sadhasivam
                   ` (2 preceding siblings ...)
  2022-11-17  5:31 ` [PATCH v7 3/4] cpufreq: qcom-hw: Add CPU clock provider support Manivannan Sadhasivam
@ 2022-11-17  5:31 ` Manivannan Sadhasivam
  2022-11-21  5:04   ` Viresh Kumar
  2022-11-17 10:19 ` [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support Sudeep Holla
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 21+ messages in thread
From: Manivannan Sadhasivam @ 2022-11-17  5:31 UTC (permalink / raw)
  To: andersson, viresh.kumar, krzysztof.kozlowski+dt, rafael, robh+dt
  Cc: johan, devicetree, linux-arm-msm, linux-kernel, linux-pm,
	Manivannan Sadhasivam, stable, Sudeep Holla

The cpufreq_driver->get() callback is supposed to return the current
frequency of the CPU and not the one requested by the CPUFreq core.
Fix it by returning the frequency that gets supplied to the CPU after
the DCVS operation of EPSS/OSM.

Cc: stable@vger.kernel.org # v5.15
Fixes: 2849dd8bc72b ("cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver")
Reported-by: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/cpufreq/qcom-cpufreq-hw.c | 42 +++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 13 deletions(-)

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index 1faa325d3e52..62f36c76e26c 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -131,7 +131,35 @@ static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
 	return 0;
 }
 
+static unsigned long qcom_lmh_get_throttle_freq(struct qcom_cpufreq_data *data)
+{
+	unsigned int lval;
+
+	if (qcom_cpufreq.soc_data->reg_current_vote)
+		lval = readl_relaxed(data->base + qcom_cpufreq.soc_data->reg_current_vote) & 0x3ff;
+	else
+		lval = readl_relaxed(data->base + qcom_cpufreq.soc_data->reg_domain_state) & 0xff;
+
+	return lval * xo_rate;
+}
+
+/* Get the current frequency of the CPU (after throttling) */
 static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
+{
+	struct qcom_cpufreq_data *data;
+	struct cpufreq_policy *policy;
+
+	policy = cpufreq_cpu_get_raw(cpu);
+	if (!policy)
+		return 0;
+
+	data = policy->driver_data;
+
+	return qcom_lmh_get_throttle_freq(data) / HZ_PER_KHZ;
+}
+
+/* Get the frequency requested by the cpufreq core for the CPU */
+static unsigned int qcom_cpufreq_get_freq(unsigned int cpu)
 {
 	struct qcom_cpufreq_data *data;
 	const struct qcom_cpufreq_soc_data *soc_data;
@@ -292,18 +320,6 @@ static void qcom_get_related_cpus(int index, struct cpumask *m)
 	}
 }
 
-static unsigned long qcom_lmh_get_throttle_freq(struct qcom_cpufreq_data *data)
-{
-	unsigned int lval;
-
-	if (qcom_cpufreq.soc_data->reg_current_vote)
-		lval = readl_relaxed(data->base + qcom_cpufreq.soc_data->reg_current_vote) & 0x3ff;
-	else
-		lval = readl_relaxed(data->base + qcom_cpufreq.soc_data->reg_domain_state) & 0xff;
-
-	return lval * xo_rate;
-}
-
 static void qcom_lmh_dcvs_notify(struct qcom_cpufreq_data *data)
 {
 	struct cpufreq_policy *policy = data->policy;
@@ -347,7 +363,7 @@ static void qcom_lmh_dcvs_notify(struct qcom_cpufreq_data *data)
 	 * If h/w throttled frequency is higher than what cpufreq has requested
 	 * for, then stop polling and switch back to interrupt mechanism.
 	 */
-	if (throttled_freq >= qcom_cpufreq_hw_get(cpu))
+	if (throttled_freq >= qcom_cpufreq_get_freq(cpu))
 		enable_irq(data->throttle_irq);
 	else
 		mod_delayed_work(system_highpri_wq, &data->throttle_work,
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support
  2022-11-17  5:31 [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support Manivannan Sadhasivam
                   ` (3 preceding siblings ...)
  2022-11-17  5:31 ` [PATCH v7 4/4] cpufreq: qcom-hw: Fix the frequency returned by cpufreq_driver->get() Manivannan Sadhasivam
@ 2022-11-17 10:19 ` Sudeep Holla
  2022-11-17 11:12   ` Manivannan Sadhasivam
  2022-11-17 11:24   ` Viresh Kumar
  2022-11-21  5:19 ` Viresh Kumar
  2022-12-06 18:19 ` (subset) " Bjorn Andersson
  6 siblings, 2 replies; 21+ messages in thread
From: Sudeep Holla @ 2022-11-17 10:19 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: andersson, viresh.kumar, krzysztof.kozlowski+dt, rafael,
	Sudeep Holla, robh+dt, johan, devicetree, linux-arm-msm,
	linux-kernel, linux-pm

On Thu, Nov 17, 2022 at 11:01:41AM +0530, Manivannan Sadhasivam wrote:
> Hello,
> 
> This series adds clock provider support to the Qcom CPUFreq driver for
> supplying the clocks to the CPU cores in Qcom SoCs.
> 
> The Qualcomm platforms making use of CPUFreq HW Engine (EPSS/OSM) supply
> clocks to the CPU cores. But this is not represented clearly in devicetree.
> There is no clock coming out of the CPUFreq HW node to the CPU. This created
> an issue [1] with the OPP core when a recent enhancement series was submitted.
> Eventhough the issue got fixed in the OPP framework in the meantime, that's
> not a proper solution and this series aims to fix it properly.
> 
> There was also an attempt made by Viresh [2] to fix the issue by moving the
> clocks supplied to the CPUFreq HW node to the CPU. But that was not accepted
> since those clocks belong to the CPUFreq HW node only.
> 
> The proposal here is to add clock provider support to the Qcom CPUFreq HW
> driver to supply clocks to the CPUs that comes out of the EPSS/OSM block.
> This correctly reflects the hardware implementation.
> 
> The clock provider is a simple one that just provides the frequency of the
> clocks supplied to each frequency domain in the SoC using .recalc_rate()
> callback. The frequency supplied by the driver will be the actual frequency
> that comes out of the EPSS/OSM block after the DCVS operation. This frequency
> is not same as what the CPUFreq framework has set but it is the one that gets
> supplied to the CPUs after throttling by LMh.
> 
> This series has been tested on SM8450 based dev board with the OPP hack removed
> and hence there is a DTS change only for that platform. Once this series gets
> accepted, rest of the platform DTS can also be modified and finally the hack on
> the OPP core can be dropped.
> 
> Thanks,
> Mani
> 
> [1] https://lore.kernel.org/lkml/YsxSkswzsqgMOc0l@hovoldconsulting.com/
> [2] https://lore.kernel.org/lkml/20220801054255.GA12039@thinkpad/t/
> 
> Changes in v7:
> 
> * Added a patch that returns the throttled frequency for cpufreq_driver->get()
>   callback (Sudeep & Viresh)
> * Added error check for kasprintf and allocated the clk name locally
> 
> Changes in v6:
> 
> * Removed the local variable clk_name (Matthias)
> * Added the clock id to the error message of devm_clk_hw_register()
> 
> Changes in v5:
> 
> * Switched to Hz unit for the CPU clocks
> 
> Changes in v4:
> 
> * Rebased on top of cpufreq/arm/linux-next branch
> 
> Changes in v3:
> 
> * Submitted the cpufreq driver cleanup patches as a separate series as
>   suggested by Viresh
> * Removed static keyword from clk_init_data declaration
> 
> Changes in v2:
> 
> * Moved the qcom_cpufreq_data allocation to probe
> * Added single clock provider with multiple clks for each freq domain
> * Moved soc_data to qcom_cpufreq struct
> * Added Rob's review for binding
> 
> Manivannan Sadhasivam (4):
>   dt-bindings: cpufreq: cpufreq-qcom-hw: Add cpufreq clock provider
>   arm64: dts: qcom: sm8450: Supply clock from cpufreq node to CPUs
>   cpufreq: qcom-hw: Add CPU clock provider support

Why do you need the above 3 changes if the below(4/4) will ensure
cpufreq_get(cpu) returns the clock frequency. I was expecting to drop the
whole "confusing" clock bindings and the unnecessary clock provider.

Can't we just use cpufreq_get(cpu) ?

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support
  2022-11-17 10:19 ` [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support Sudeep Holla
@ 2022-11-17 11:12   ` Manivannan Sadhasivam
  2022-11-17 11:52     ` Sudeep Holla
  2022-11-17 11:24   ` Viresh Kumar
  1 sibling, 1 reply; 21+ messages in thread
From: Manivannan Sadhasivam @ 2022-11-17 11:12 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: andersson, viresh.kumar, krzysztof.kozlowski+dt, rafael, robh+dt,
	johan, devicetree, linux-arm-msm, linux-kernel, linux-pm

On Thu, Nov 17, 2022 at 10:19:03AM +0000, Sudeep Holla wrote:
> On Thu, Nov 17, 2022 at 11:01:41AM +0530, Manivannan Sadhasivam wrote:
> > Hello,
> > 
> > This series adds clock provider support to the Qcom CPUFreq driver for
> > supplying the clocks to the CPU cores in Qcom SoCs.
> > 
> > The Qualcomm platforms making use of CPUFreq HW Engine (EPSS/OSM) supply
> > clocks to the CPU cores. But this is not represented clearly in devicetree.
> > There is no clock coming out of the CPUFreq HW node to the CPU. This created
> > an issue [1] with the OPP core when a recent enhancement series was submitted.
> > Eventhough the issue got fixed in the OPP framework in the meantime, that's
> > not a proper solution and this series aims to fix it properly.
> > 
> > There was also an attempt made by Viresh [2] to fix the issue by moving the
> > clocks supplied to the CPUFreq HW node to the CPU. But that was not accepted
> > since those clocks belong to the CPUFreq HW node only.
> > 
> > The proposal here is to add clock provider support to the Qcom CPUFreq HW
> > driver to supply clocks to the CPUs that comes out of the EPSS/OSM block.
> > This correctly reflects the hardware implementation.
> > 
> > The clock provider is a simple one that just provides the frequency of the
> > clocks supplied to each frequency domain in the SoC using .recalc_rate()
> > callback. The frequency supplied by the driver will be the actual frequency
> > that comes out of the EPSS/OSM block after the DCVS operation. This frequency
> > is not same as what the CPUFreq framework has set but it is the one that gets
> > supplied to the CPUs after throttling by LMh.
> > 
> > This series has been tested on SM8450 based dev board with the OPP hack removed
> > and hence there is a DTS change only for that platform. Once this series gets
> > accepted, rest of the platform DTS can also be modified and finally the hack on
> > the OPP core can be dropped.
> > 
> > Thanks,
> > Mani
> > 
> > [1] https://lore.kernel.org/lkml/YsxSkswzsqgMOc0l@hovoldconsulting.com/
> > [2] https://lore.kernel.org/lkml/20220801054255.GA12039@thinkpad/t/
> > 
> > Changes in v7:
> > 
> > * Added a patch that returns the throttled frequency for cpufreq_driver->get()
> >   callback (Sudeep & Viresh)
> > * Added error check for kasprintf and allocated the clk name locally
> > 
> > Changes in v6:
> > 
> > * Removed the local variable clk_name (Matthias)
> > * Added the clock id to the error message of devm_clk_hw_register()
> > 
> > Changes in v5:
> > 
> > * Switched to Hz unit for the CPU clocks
> > 
> > Changes in v4:
> > 
> > * Rebased on top of cpufreq/arm/linux-next branch
> > 
> > Changes in v3:
> > 
> > * Submitted the cpufreq driver cleanup patches as a separate series as
> >   suggested by Viresh
> > * Removed static keyword from clk_init_data declaration
> > 
> > Changes in v2:
> > 
> > * Moved the qcom_cpufreq_data allocation to probe
> > * Added single clock provider with multiple clks for each freq domain
> > * Moved soc_data to qcom_cpufreq struct
> > * Added Rob's review for binding
> > 
> > Manivannan Sadhasivam (4):
> >   dt-bindings: cpufreq: cpufreq-qcom-hw: Add cpufreq clock provider
> >   arm64: dts: qcom: sm8450: Supply clock from cpufreq node to CPUs
> >   cpufreq: qcom-hw: Add CPU clock provider support
> 
> Why do you need the above 3 changes if the below(4/4) will ensure
> cpufreq_get(cpu) returns the clock frequency. I was expecting to drop the
> whole "confusing" clock bindings and the unnecessary clock provider.
> 
> Can't we just use cpufreq_get(cpu) ?
> 

This can be possible for OPP implementations for the CPUs but not for other
peripherals making use of OPP framework like GPU etc... Moreover this may end
up with different code path for CPUs and other peripherals inside OPP framework.

So I don't think it is applicable. But I'll defer it to Viresh.

Thanks,
Mani

> -- 
> Regards,
> Sudeep

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support
  2022-11-17 10:19 ` [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support Sudeep Holla
  2022-11-17 11:12   ` Manivannan Sadhasivam
@ 2022-11-17 11:24   ` Viresh Kumar
  2022-11-17 12:01     ` Sudeep Holla
  1 sibling, 1 reply; 21+ messages in thread
From: Viresh Kumar @ 2022-11-17 11:24 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: Manivannan Sadhasivam, andersson, krzysztof.kozlowski+dt, rafael,
	robh+dt, johan, devicetree, linux-arm-msm, linux-kernel,
	linux-pm

On 17-11-22, 10:19, Sudeep Holla wrote:
> Why do you need the above 3 changes if the below(4/4) will ensure
> cpufreq_get(cpu) returns the clock frequency. I was expecting to drop the
> whole "confusing" clock bindings and the unnecessary clock provider.
> 
> Can't we just use cpufreq_get(cpu) ?

https://lore.kernel.org/lkml/cover.1657695140.git.viresh.kumar@linaro.org/

The basic idea (need) here was to fix the DT and let the CPU nodes have clock
related properties, which are missing currently.

The context can be seen in the above thread.

-- 
viresh

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support
  2022-11-17 11:12   ` Manivannan Sadhasivam
@ 2022-11-17 11:52     ` Sudeep Holla
  2022-11-17 11:58       ` Manivannan Sadhasivam
  0 siblings, 1 reply; 21+ messages in thread
From: Sudeep Holla @ 2022-11-17 11:52 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: andersson, viresh.kumar, Sudeep Holla, krzysztof.kozlowski+dt,
	rafael, robh+dt, johan, devicetree, linux-arm-msm, linux-kernel,
	linux-pm

On Thu, Nov 17, 2022 at 04:42:07PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Nov 17, 2022 at 10:19:03AM +0000, Sudeep Holla wrote:
> > 
> > Why do you need the above 3 changes if the below(4/4) will ensure
> > cpufreq_get(cpu) returns the clock frequency. I was expecting to drop the
> > whole "confusing" clock bindings and the unnecessary clock provider.
> > 
> > Can't we just use cpufreq_get(cpu) ?
> > 
> 
> This can be possible for OPP implementations for the CPUs but not for other
> peripherals making use of OPP framework like GPU etc... Moreover this may end
> up with different code path for CPUs and other peripherals inside OPP framework.
> 

Fair enough, you can use this for non-CPU devices. But you are adding this for
CPUs here. Is the consumer unaware that this is a CPU or non-CPU device ?
If so, make sense. Otherwise, it is unnecessary to go through the clk
framework to get CPU frequency.

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support
  2022-11-17 11:52     ` Sudeep Holla
@ 2022-11-17 11:58       ` Manivannan Sadhasivam
  2022-11-17 12:08         ` Sudeep Holla
  0 siblings, 1 reply; 21+ messages in thread
From: Manivannan Sadhasivam @ 2022-11-17 11:58 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: andersson, viresh.kumar, krzysztof.kozlowski+dt, rafael, robh+dt,
	johan, devicetree, linux-arm-msm, linux-kernel, linux-pm

On Thu, Nov 17, 2022 at 11:52:03AM +0000, Sudeep Holla wrote:
> On Thu, Nov 17, 2022 at 04:42:07PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Nov 17, 2022 at 10:19:03AM +0000, Sudeep Holla wrote:
> > > 
> > > Why do you need the above 3 changes if the below(4/4) will ensure
> > > cpufreq_get(cpu) returns the clock frequency. I was expecting to drop the
> > > whole "confusing" clock bindings and the unnecessary clock provider.
> > > 
> > > Can't we just use cpufreq_get(cpu) ?
> > > 
> > 
> > This can be possible for OPP implementations for the CPUs but not for other
> > peripherals making use of OPP framework like GPU etc... Moreover this may end
> > up with different code path for CPUs and other peripherals inside OPP framework.
> > 
> 
> Fair enough, you can use this for non-CPU devices. But you are adding this for
> CPUs here. Is the consumer unaware that this is a CPU or non-CPU device ?
> If so, make sense. Otherwise, it is unnecessary to go through the clk
> framework to get CPU frequency.
> 

The consumer here is the OPP framework and yes it doesn't have the knowledge of
the device it is dealing with (for this context).

Thanks,
Mani

> -- 
> Regards,
> Sudeep

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support
  2022-11-17 11:24   ` Viresh Kumar
@ 2022-11-17 12:01     ` Sudeep Holla
  2022-11-18  5:57       ` Viresh Kumar
  0 siblings, 1 reply; 21+ messages in thread
From: Sudeep Holla @ 2022-11-17 12:01 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Manivannan Sadhasivam, Sudeep Holla, andersson,
	krzysztof.kozlowski+dt, rafael, robh+dt, johan, devicetree,
	linux-arm-msm, linux-kernel, linux-pm

On Thu, Nov 17, 2022 at 04:54:03PM +0530, Viresh Kumar wrote:
> On 17-11-22, 10:19, Sudeep Holla wrote:
> > Why do you need the above 3 changes if the below(4/4) will ensure
> > cpufreq_get(cpu) returns the clock frequency. I was expecting to drop the
> > whole "confusing" clock bindings and the unnecessary clock provider.
> > 
> > Can't we just use cpufreq_get(cpu) ?
> 
> https://lore.kernel.org/lkml/cover.1657695140.git.viresh.kumar@linaro.org/
> 
> The basic idea (need) here was to fix the DT and let the CPU nodes have clock
> related properties, which are missing currently.
> 
> The context can be seen in the above thread.

Thanks for the link. Sorry I still don't get the complete picture. Who are
the consumers of these clock nodes if not cpufreq itself.

I am going to guess, so other device(like inter-connect) with phandle into
CPU device perhaps ? Also I assume it will have phandle to non-CPU device
and hence we need generic device clock solution. Sorry for the noise, but
I still find having both clocks and qcom,freq-domain property is quite
confusing but I am fine as I understand it bit better now.

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support
  2022-11-17 11:58       ` Manivannan Sadhasivam
@ 2022-11-17 12:08         ` Sudeep Holla
  2022-11-17 12:38           ` Manivannan Sadhasivam
  0 siblings, 1 reply; 21+ messages in thread
From: Sudeep Holla @ 2022-11-17 12:08 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: andersson, viresh.kumar, Sudeep Holla, krzysztof.kozlowski+dt,
	rafael, robh+dt, johan, devicetree, linux-arm-msm, linux-kernel,
	linux-pm

On Thu, Nov 17, 2022 at 05:28:07PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Nov 17, 2022 at 11:52:03AM +0000, Sudeep Holla wrote:
> > On Thu, Nov 17, 2022 at 04:42:07PM +0530, Manivannan Sadhasivam wrote:
> > > On Thu, Nov 17, 2022 at 10:19:03AM +0000, Sudeep Holla wrote:
> > > > 
> > > > Why do you need the above 3 changes if the below(4/4) will ensure
> > > > cpufreq_get(cpu) returns the clock frequency. I was expecting to drop the
> > > > whole "confusing" clock bindings and the unnecessary clock provider.
> > > > 
> > > > Can't we just use cpufreq_get(cpu) ?
> > > > 
> > > 
> > > This can be possible for OPP implementations for the CPUs but not for other
> > > peripherals making use of OPP framework like GPU etc... Moreover this may end
> > > up with different code path for CPUs and other peripherals inside OPP framework.
> > > 
> > 
> > Fair enough, you can use this for non-CPU devices. But you are adding this for
> > CPUs here. Is the consumer unaware that this is a CPU or non-CPU device ?
> > If so, make sense. Otherwise, it is unnecessary to go through the clk
> > framework to get CPU frequency.
> > 
> 
> The consumer here is the OPP framework and yes it doesn't have the knowledge of
> the device it is dealing with (for this context).

Ah OK, I thought it is something else. Does this mean OPP is tied with clk
framework or clock bindings ? Is this for some specific feature ? Or is it
compulsory for all the devices using OPP ? Just wondering how this affects
SCMI which doesn't use or provide clocks yet.

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support
  2022-11-17 12:08         ` Sudeep Holla
@ 2022-11-17 12:38           ` Manivannan Sadhasivam
  2022-11-17 13:23             ` Sudeep Holla
  0 siblings, 1 reply; 21+ messages in thread
From: Manivannan Sadhasivam @ 2022-11-17 12:38 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: andersson, viresh.kumar, krzysztof.kozlowski+dt, rafael, robh+dt,
	johan, devicetree, linux-arm-msm, linux-kernel, linux-pm

On Thu, Nov 17, 2022 at 12:08:46PM +0000, Sudeep Holla wrote:
> On Thu, Nov 17, 2022 at 05:28:07PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Nov 17, 2022 at 11:52:03AM +0000, Sudeep Holla wrote:
> > > On Thu, Nov 17, 2022 at 04:42:07PM +0530, Manivannan Sadhasivam wrote:
> > > > On Thu, Nov 17, 2022 at 10:19:03AM +0000, Sudeep Holla wrote:
> > > > > 
> > > > > Why do you need the above 3 changes if the below(4/4) will ensure
> > > > > cpufreq_get(cpu) returns the clock frequency. I was expecting to drop the
> > > > > whole "confusing" clock bindings and the unnecessary clock provider.
> > > > > 
> > > > > Can't we just use cpufreq_get(cpu) ?
> > > > > 
> > > > 
> > > > This can be possible for OPP implementations for the CPUs but not for other
> > > > peripherals making use of OPP framework like GPU etc... Moreover this may end
> > > > up with different code path for CPUs and other peripherals inside OPP framework.
> > > > 
> > > 
> > > Fair enough, you can use this for non-CPU devices. But you are adding this for
> > > CPUs here. Is the consumer unaware that this is a CPU or non-CPU device ?
> > > If so, make sense. Otherwise, it is unnecessary to go through the clk
> > > framework to get CPU frequency.
> > > 
> > 
> > The consumer here is the OPP framework and yes it doesn't have the knowledge of
> > the device it is dealing with (for this context).
> 
> Ah OK, I thought it is something else. Does this mean OPP is tied with clk
> framework or clock bindings ? Is this for some specific feature ?

AFAIK, OPP framework needs to know the current frequency of the device it is
dealing with for setting the device's OPP. So it uses clk_get_rate() of the
first clock of the device. If the clock is not available, then it uses the
frequency in the first entry of the OPP table (since it is going to be the
minimum freq of the device).

As you can see, the clk_get_rate() is eminent for switching the OPPs and since
OPP framework doesn't know what device it is dealing with, it cannot use
cpufreq_get().

> Or is it
> compulsory for all the devices using OPP ? Just wondering how this affects
> SCMI which doesn't use or provide clocks yet.

Is SCMI node itself has the OPP tables? Or the consumer nodes of the SCMI?

TLDR; If you tell OPP framework to set a new OPP for a device, it needs to the
know the current frequency of the device. And it is not manadatory now, but in
the future maybe.

Thanks,
Mani

> -- 
> Regards,
> Sudeep

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support
  2022-11-17 12:38           ` Manivannan Sadhasivam
@ 2022-11-17 13:23             ` Sudeep Holla
  0 siblings, 0 replies; 21+ messages in thread
From: Sudeep Holla @ 2022-11-17 13:23 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: andersson, viresh.kumar, Sudeep Holla, krzysztof.kozlowski+dt,
	rafael, robh+dt, johan, devicetree, linux-arm-msm, linux-kernel,
	linux-pm

On Thu, Nov 17, 2022 at 06:08:41PM +0530, Manivannan Sadhasivam wrote:

[...]
>
> AFAIK, OPP framework needs to know the current frequency of the device it is
> dealing with for setting the device's OPP. So it uses clk_get_rate() of the
> first clock of the device. If the clock is not available, then it uses the
> frequency in the first entry of the OPP table (since it is going to be the
> minimum freq of the device).
>

It has been a while since I followed OPP. Thanks for all the info, helped me
get updated without looking at the code in detail.

> As you can see, the clk_get_rate() is eminent for switching the OPPs and since
> OPP framework doesn't know what device it is dealing with, it cannot use
> cpufreq_get().
>

Agreed. I had assumed the qcom-cpufreq-hw as setting cpufreq directly pocking
the hardware but now I see it is using opp library to set some additional
policy.

> Is SCMI node itself has the OPP tables? Or the consumer nodes of the SCMI?
>

No, OPPs are read from the f/w and we just use OPP APIs to register them.
But we don't use OPP library to set the performance.

> TLDR; If you tell OPP framework to set a new OPP for a device, it needs to the
> know the current frequency of the device. And it is not manadatory now, but in
> the future maybe.
>

Hmm, good to know. I prefer it is not coupled with clocks and have some
alternative mechanism that is suitable for performance domains and don't
enforce the use of clock bindings and clk framework unnecessarily.

--
Regards,
Sudeep`

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support
  2022-11-17 12:01     ` Sudeep Holla
@ 2022-11-18  5:57       ` Viresh Kumar
  2022-11-18 11:54         ` Sudeep Holla
  0 siblings, 1 reply; 21+ messages in thread
From: Viresh Kumar @ 2022-11-18  5:57 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: Manivannan Sadhasivam, andersson, krzysztof.kozlowski+dt, rafael,
	robh+dt, johan, devicetree, linux-arm-msm, linux-kernel,
	linux-pm

On 17-11-22, 12:01, Sudeep Holla wrote:
> Thanks for the link. Sorry I still don't get the complete picture. Who are
> the consumers of these clock nodes if not cpufreq itself.
> 
> I am going to guess, so other device(like inter-connect) with phandle into
> CPU device perhaps ? Also I assume it will have phandle to non-CPU device
> and hence we need generic device clock solution. Sorry for the noise, but
> I still find having both clocks and qcom,freq-domain property is quite
> confusing but I am fine as I understand it bit better now.

Lemme try to explain what the initial problem was, because of which I suggested
the DT to be fixed, even if no one is going to use it as a client.

The OPP core provides two features:

- Parsing of the OPP table and provide the data to the client.
- Ability to switch the OPPs, i.e. configuring all resources.

qcom-cpufreq-hw driver uses both of these, but in a tricky way (like Tegra30).
It used the OPP core to parse the data, along with "opp-hz" property and switch
the OPPs by calling dev_pm_opp_set_opp(). But it doesn't want
dev_pm_opp_set_opp() to change the clock rate, but configure everything else.

Now the OPP core needs to distinguish platforms for valid and invalid
configurations, to make sure something isn't broken. For example a developer
wants to change the OPP along with frequency and passes a valid OPP table. But
forgets to set the clock entry in device's node. This is an error and the OPP
core needs to report it. There can be more of such issues with different
configurations.

Also, as Mani explained, if the OPP core is required to switch the OPPs, then it
needs to know the initial frequency of the device to see if we are going up or
down the frequency graph. And so it will do a clk_get_rate() if there is
"opp-hz" available.


What we did in case of Tegra30 (commit 1b195626) is provide a .config_clks
helper, which does nothing. So the OPP core doesn't need to know if frequency is
programmed or not.

The same can not be done for Qcom right now as the CPU node doesn't have the clk
property though it has "opp-hz".

Weather we have a user in kernel (OS) or not, shouldn't decide how the DT looks
like. The DT should clearly define what the hardware looks like, irrespective of
the users. The CPU has a clock and it should be mentioned. If the OPP core
chooses to use that information, then it is a fine expectation to have.

And so we are here. Most likely no one will ever do clk_set_rate() on this new
clock, which is fine, though OPP core will likely do clk_get_rate() here.

Hope I was able to clarify few things here.

-- 
viresh

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support
  2022-11-18  5:57       ` Viresh Kumar
@ 2022-11-18 11:54         ` Sudeep Holla
  0 siblings, 0 replies; 21+ messages in thread
From: Sudeep Holla @ 2022-11-18 11:54 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Manivannan Sadhasivam, Sudeep Holla, andersson,
	krzysztof.kozlowski+dt, rafael, robh+dt, johan, devicetree,
	linux-arm-msm, linux-kernel, linux-pm

On Fri, Nov 18, 2022 at 11:27:30AM +0530, Viresh Kumar wrote:
> On 17-11-22, 12:01, Sudeep Holla wrote:
> > Thanks for the link. Sorry I still don't get the complete picture. Who are
> > the consumers of these clock nodes if not cpufreq itself.
> > 
> > I am going to guess, so other device(like inter-connect) with phandle into
> > CPU device perhaps ? Also I assume it will have phandle to non-CPU device
> > and hence we need generic device clock solution. Sorry for the noise, but
> > I still find having both clocks and qcom,freq-domain property is quite
> > confusing but I am fine as I understand it bit better now.
> 
> Lemme try to explain what the initial problem was, because of which I suggested
> the DT to be fixed, even if no one is going to use it as a client.
> 
> The OPP core provides two features:
> 
> - Parsing of the OPP table and provide the data to the client.
> - Ability to switch the OPPs, i.e. configuring all resources.
> 
> qcom-cpufreq-hw driver uses both of these, but in a tricky way (like Tegra30).
> It used the OPP core to parse the data, along with "opp-hz" property and switch
> the OPPs by calling dev_pm_opp_set_opp(). But it doesn't want
> dev_pm_opp_set_opp() to change the clock rate, but configure everything else.
> 
> Now the OPP core needs to distinguish platforms for valid and invalid
> configurations, to make sure something isn't broken. For example a developer
> wants to change the OPP along with frequency and passes a valid OPP table. But
> forgets to set the clock entry in device's node. This is an error and the OPP
> core needs to report it. There can be more of such issues with different
> configurations.
> 
> Also, as Mani explained, if the OPP core is required to switch the OPPs, then it
> needs to know the initial frequency of the device to see if we are going up or
> down the frequency graph. And so it will do a clk_get_rate() if there is
> "opp-hz" available.
> 
> 
> What we did in case of Tegra30 (commit 1b195626) is provide a .config_clks
> helper, which does nothing. So the OPP core doesn't need to know if frequency is
> programmed or not.
> 
> The same can not be done for Qcom right now as the CPU node doesn't have the clk
> property though it has "opp-hz".
> 
> Weather we have a user in kernel (OS) or not, shouldn't decide how the DT looks
> like. The DT should clearly define what the hardware looks like, irrespective of
> the users. The CPU has a clock and it should be mentioned. If the OPP core
> chooses to use that information, then it is a fine expectation to have.
> 
> And so we are here. Most likely no one will ever do clk_set_rate() on this new
> clock, which is fine, though OPP core will likely do clk_get_rate() here.
> 
> Hope I was able to clarify few things here.
> 

Thanks a ton for such detailed explanation. Ulf was trying to integrate
genpd + performance domains with SCMI. That is the reason for my interest
in this topic. Sorry for all the trouble.

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 4/4] cpufreq: qcom-hw: Fix the frequency returned by cpufreq_driver->get()
  2022-11-17  5:31 ` [PATCH v7 4/4] cpufreq: qcom-hw: Fix the frequency returned by cpufreq_driver->get() Manivannan Sadhasivam
@ 2022-11-21  5:04   ` Viresh Kumar
  0 siblings, 0 replies; 21+ messages in thread
From: Viresh Kumar @ 2022-11-21  5:04 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: andersson, krzysztof.kozlowski+dt, rafael, robh+dt, johan,
	devicetree, linux-arm-msm, linux-kernel, linux-pm, stable,
	Sudeep Holla

On 17-11-22, 11:01, Manivannan Sadhasivam wrote:
> The cpufreq_driver->get() callback is supposed to return the current
> frequency of the CPU and not the one requested by the CPUFreq core.
> Fix it by returning the frequency that gets supplied to the CPU after
> the DCVS operation of EPSS/OSM.
> 
> Cc: stable@vger.kernel.org # v5.15
> Fixes: 2849dd8bc72b ("cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver")
> Reported-by: Sudeep Holla <sudeep.holla@arm.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/cpufreq/qcom-cpufreq-hw.c | 42 +++++++++++++++++++++----------
>  1 file changed, 29 insertions(+), 13 deletions(-)

A fix should always be the first patch in the series as it needs to be
backported.

I can reorder this if it won't break anything for this time.

-- 
viresh

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support
  2022-11-17  5:31 [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support Manivannan Sadhasivam
                   ` (4 preceding siblings ...)
  2022-11-17 10:19 ` [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support Sudeep Holla
@ 2022-11-21  5:19 ` Viresh Kumar
  2022-11-21  6:45   ` Manivannan Sadhasivam
  2022-11-21  7:48   ` Viresh Kumar
  2022-12-06 18:19 ` (subset) " Bjorn Andersson
  6 siblings, 2 replies; 21+ messages in thread
From: Viresh Kumar @ 2022-11-21  5:19 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: andersson, krzysztof.kozlowski+dt, rafael, robh+dt, johan,
	devicetree, linux-arm-msm, linux-kernel, linux-pm

On 17-11-22, 11:01, Manivannan Sadhasivam wrote:
> Hello,
> 
> This series adds clock provider support to the Qcom CPUFreq driver for
> supplying the clocks to the CPU cores in Qcom SoCs.
> 
> The Qualcomm platforms making use of CPUFreq HW Engine (EPSS/OSM) supply
> clocks to the CPU cores. But this is not represented clearly in devicetree.
> There is no clock coming out of the CPUFreq HW node to the CPU. This created
> an issue [1] with the OPP core when a recent enhancement series was submitted.
> Eventhough the issue got fixed in the OPP framework in the meantime, that's
> not a proper solution and this series aims to fix it properly.
> 
> There was also an attempt made by Viresh [2] to fix the issue by moving the
> clocks supplied to the CPUFreq HW node to the CPU. But that was not accepted
> since those clocks belong to the CPUFreq HW node only.
> 
> The proposal here is to add clock provider support to the Qcom CPUFreq HW
> driver to supply clocks to the CPUs that comes out of the EPSS/OSM block.
> This correctly reflects the hardware implementation.
> 
> The clock provider is a simple one that just provides the frequency of the
> clocks supplied to each frequency domain in the SoC using .recalc_rate()
> callback. The frequency supplied by the driver will be the actual frequency
> that comes out of the EPSS/OSM block after the DCVS operation. This frequency
> is not same as what the CPUFreq framework has set but it is the one that gets
> supplied to the CPUs after throttling by LMh.
> 
> This series has been tested on SM8450 based dev board with the OPP hack removed
> and hence there is a DTS change only for that platform. Once this series gets
> accepted, rest of the platform DTS can also be modified and finally the hack on
> the OPP core can be dropped.

Applied. Thanks.

If you get review comments later on, please send incremental patches
for that.

-- 
viresh

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support
  2022-11-21  5:19 ` Viresh Kumar
@ 2022-11-21  6:45   ` Manivannan Sadhasivam
  2022-11-21  7:48   ` Viresh Kumar
  1 sibling, 0 replies; 21+ messages in thread
From: Manivannan Sadhasivam @ 2022-11-21  6:45 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: andersson, krzysztof.kozlowski+dt, rafael, robh+dt, johan,
	devicetree, linux-arm-msm, linux-kernel, linux-pm

On Mon, Nov 21, 2022 at 10:49:59AM +0530, Viresh Kumar wrote:
> On 17-11-22, 11:01, Manivannan Sadhasivam wrote:
> > Hello,
> > 
> > This series adds clock provider support to the Qcom CPUFreq driver for
> > supplying the clocks to the CPU cores in Qcom SoCs.
> > 
> > The Qualcomm platforms making use of CPUFreq HW Engine (EPSS/OSM) supply
> > clocks to the CPU cores. But this is not represented clearly in devicetree.
> > There is no clock coming out of the CPUFreq HW node to the CPU. This created
> > an issue [1] with the OPP core when a recent enhancement series was submitted.
> > Eventhough the issue got fixed in the OPP framework in the meantime, that's
> > not a proper solution and this series aims to fix it properly.
> > 
> > There was also an attempt made by Viresh [2] to fix the issue by moving the
> > clocks supplied to the CPUFreq HW node to the CPU. But that was not accepted
> > since those clocks belong to the CPUFreq HW node only.
> > 
> > The proposal here is to add clock provider support to the Qcom CPUFreq HW
> > driver to supply clocks to the CPUs that comes out of the EPSS/OSM block.
> > This correctly reflects the hardware implementation.
> > 
> > The clock provider is a simple one that just provides the frequency of the
> > clocks supplied to each frequency domain in the SoC using .recalc_rate()
> > callback. The frequency supplied by the driver will be the actual frequency
> > that comes out of the EPSS/OSM block after the DCVS operation. This frequency
> > is not same as what the CPUFreq framework has set but it is the one that gets
> > supplied to the CPUs after throttling by LMh.
> > 
> > This series has been tested on SM8450 based dev board with the OPP hack removed
> > and hence there is a DTS change only for that platform. Once this series gets
> > accepted, rest of the platform DTS can also be modified and finally the hack on
> > the OPP core can be dropped.
> 
> Applied. Thanks.
> 
> If you get review comments later on, please send incremental patches
> for that.
> 

Sure thing.

Thanks,
Mani

> -- 
> viresh

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support
  2022-11-21  5:19 ` Viresh Kumar
  2022-11-21  6:45   ` Manivannan Sadhasivam
@ 2022-11-21  7:48   ` Viresh Kumar
  1 sibling, 0 replies; 21+ messages in thread
From: Viresh Kumar @ 2022-11-21  7:48 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: andersson, krzysztof.kozlowski+dt, rafael, robh+dt, johan,
	devicetree, linux-arm-msm, linux-kernel, linux-pm

On 21-11-22, 10:49, Viresh Kumar wrote:
> On 17-11-22, 11:01, Manivannan Sadhasivam wrote:
> > Hello,
> > 
> > This series adds clock provider support to the Qcom CPUFreq driver for
> > supplying the clocks to the CPU cores in Qcom SoCs.
> > 
> > The Qualcomm platforms making use of CPUFreq HW Engine (EPSS/OSM) supply
> > clocks to the CPU cores. But this is not represented clearly in devicetree.
> > There is no clock coming out of the CPUFreq HW node to the CPU. This created
> > an issue [1] with the OPP core when a recent enhancement series was submitted.
> > Eventhough the issue got fixed in the OPP framework in the meantime, that's
> > not a proper solution and this series aims to fix it properly.
> > 
> > There was also an attempt made by Viresh [2] to fix the issue by moving the
> > clocks supplied to the CPUFreq HW node to the CPU. But that was not accepted
> > since those clocks belong to the CPUFreq HW node only.
> > 
> > The proposal here is to add clock provider support to the Qcom CPUFreq HW
> > driver to supply clocks to the CPUs that comes out of the EPSS/OSM block.
> > This correctly reflects the hardware implementation.
> > 
> > The clock provider is a simple one that just provides the frequency of the
> > clocks supplied to each frequency domain in the SoC using .recalc_rate()
> > callback. The frequency supplied by the driver will be the actual frequency
> > that comes out of the EPSS/OSM block after the DCVS operation. This frequency
> > is not same as what the CPUFreq framework has set but it is the one that gets
> > supplied to the CPUs after throttling by LMh.
> > 
> > This series has been tested on SM8450 based dev board with the OPP hack removed
> > and hence there is a DTS change only for that platform. Once this series gets
> > accepted, rest of the platform DTS can also be modified and finally the hack on
> > the OPP core can be dropped.
> 
> Applied. Thanks.

I have dropped patch 2/4 now, since Mani wants to get it merged via
SoC tree.

-- 
viresh

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: (subset) [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support
  2022-11-17  5:31 [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support Manivannan Sadhasivam
                   ` (5 preceding siblings ...)
  2022-11-21  5:19 ` Viresh Kumar
@ 2022-12-06 18:19 ` Bjorn Andersson
  6 siblings, 0 replies; 21+ messages in thread
From: Bjorn Andersson @ 2022-12-06 18:19 UTC (permalink / raw)
  To: robh+dt, rafael, Viresh Kumar, krzysztof.kozlowski+dt,
	Manivannan Sadhasivam
  Cc: johan, linux-arm-msm, linux-kernel, devicetree, linux-pm

On Thu, 17 Nov 2022 11:01:41 +0530, Manivannan Sadhasivam wrote:
> This series adds clock provider support to the Qcom CPUFreq driver for
> supplying the clocks to the CPU cores in Qcom SoCs.
> 
> The Qualcomm platforms making use of CPUFreq HW Engine (EPSS/OSM) supply
> clocks to the CPU cores. But this is not represented clearly in devicetree.
> There is no clock coming out of the CPUFreq HW node to the CPU. This created
> an issue [1] with the OPP core when a recent enhancement series was submitted.
> Eventhough the issue got fixed in the OPP framework in the meantime, that's
> not a proper solution and this series aims to fix it properly.
> 
> [...]

Applied, thanks!

[2/4] arm64: dts: qcom: sm8450: Supply clock from cpufreq node to CPUs
      commit: 8a8845e07b1164792a4dd5ad4d333d793828b366

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2022-12-06 18:22 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-17  5:31 [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support Manivannan Sadhasivam
2022-11-17  5:31 ` [PATCH v7 1/4] dt-bindings: cpufreq: cpufreq-qcom-hw: Add cpufreq clock provider Manivannan Sadhasivam
2022-11-17  5:31 ` [PATCH v7 2/4] arm64: dts: qcom: sm8450: Supply clock from cpufreq node to CPUs Manivannan Sadhasivam
2022-11-17  5:31 ` [PATCH v7 3/4] cpufreq: qcom-hw: Add CPU clock provider support Manivannan Sadhasivam
2022-11-17  5:31 ` [PATCH v7 4/4] cpufreq: qcom-hw: Fix the frequency returned by cpufreq_driver->get() Manivannan Sadhasivam
2022-11-21  5:04   ` Viresh Kumar
2022-11-17 10:19 ` [PATCH v7 0/4] qcom-cpufreq-hw: Add CPU clock provider support Sudeep Holla
2022-11-17 11:12   ` Manivannan Sadhasivam
2022-11-17 11:52     ` Sudeep Holla
2022-11-17 11:58       ` Manivannan Sadhasivam
2022-11-17 12:08         ` Sudeep Holla
2022-11-17 12:38           ` Manivannan Sadhasivam
2022-11-17 13:23             ` Sudeep Holla
2022-11-17 11:24   ` Viresh Kumar
2022-11-17 12:01     ` Sudeep Holla
2022-11-18  5:57       ` Viresh Kumar
2022-11-18 11:54         ` Sudeep Holla
2022-11-21  5:19 ` Viresh Kumar
2022-11-21  6:45   ` Manivannan Sadhasivam
2022-11-21  7:48   ` Viresh Kumar
2022-12-06 18:19 ` (subset) " Bjorn Andersson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.