[PATCH 0/4] qcom-cpufreq-hw: Add CPU clock provider support

[PATCH 0/4] qcom-cpufreq-hw: Add CPU clock provider support

[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 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/

Manivannan Sadhasivam (4):
  cpufreq: qcom-hw: Remove un-necessary cpumask_empty() check
  dt-bindings: cpufreq: cpufreq-qcom-hw: Add cpufreq clock provider
  cpufreq: qcom-hw: Add CPU clock provider support
  arm64: dts: qcom: sm8450: Supply clock from cpufreq node to CPUs

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

-- 
2.25.1

[PATCH 1/4] cpufreq: qcom-hw: Remove un-necessary cpumask_empty() check

[Manivannan Sadhasivam]

CPUFreq core will always set the "policy->cpus" bitmask with the bitfield
of the CPU that goes first per domain/policy. So there is no way the
"policy->cpus" bitmask will be empty during qcom_cpufreq_hw_cpu_init().

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

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index d5ef3c66c762..a5b3b8d0e164 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -552,11 +552,6 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 		data->per_core_dcvs = true;
 
 	qcom_get_related_cpus(index, policy->cpus);
-	if (cpumask_empty(policy->cpus)) {
-		dev_err(dev, "Domain-%d failed to get related CPUs\n", index);
-		ret = -ENOENT;
-		goto error;
-	}
 
 	policy->driver_data = data;
 	policy->dvfs_possible_from_any_cpu = true;
-- 
2.25.1

[PATCH 2/4] dt-bindings: cpufreq: cpufreq-qcom-hw: Add cpufreq clock provider

[Manivannan Sadhasivam]

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>
---
 .../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 cbba8979fe0e..2e0336163ffb 100644
--- a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.yaml
+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.yaml
@@ -57,6 +57,9 @@ properties:
   '#freq-domain-cells':
     const: 1
 
+  '#clock-cells':
+    const: 1
+
 required:
   - compatible
   - reg
@@ -84,6 +87,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";
           next-level-cache = <&L3_0>;
@@ -100,6 +104,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";
           next-level-cache = <&L3_0>;
@@ -113,6 +118,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";
           next-level-cache = <&L3_0>;
@@ -126,6 +132,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";
           next-level-cache = <&L3_0>;
@@ -139,6 +146,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";
           next-level-cache = <&L3_0>;
@@ -152,6 +160,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";
           next-level-cache = <&L3_0>;
@@ -165,6 +174,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";
           next-level-cache = <&L3_0>;
@@ -178,6 +188,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";
           next-level-cache = <&L3_0>;
@@ -198,6 +209,7 @@ examples:
         clock-names = "xo", "alternate";
 
         #freq-domain-cells = <1>;
+        #clock-cells = <1>;
       };
     };
 ...
-- 
2.25.1

[PATCH 3/4] cpufreq: qcom-hw: Add CPU clock provider support

[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 CPU policy. 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 | 67 +++++++++++++++++++++++++++++--
 1 file changed, 63 insertions(+), 4 deletions(-)

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index a5b3b8d0e164..4dd710f9fb69 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;
 };
@@ -482,6 +484,54 @@ static void qcom_cpufreq_hw_lmh_exit(struct qcom_cpufreq_data *data)
 	free_irq(data->throttle_irq, data);
 }
 
+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) / HZ_PER_KHZ;
+}
+
+static const struct clk_ops qcom_cpufreq_hw_clk_ops = {
+	.recalc_rate = qcom_cpufreq_hw_recalc_rate,
+};
+
+static int qcom_cpufreq_hw_clk_add(struct qcom_cpufreq_data *data, u32 index)
+{
+	struct platform_device *pdev = cpufreq_get_driver_data();
+	struct device *dev = &pdev->dev;
+	char *clk_name = devm_kasprintf(dev, GFP_KERNEL, "qcom_cpufreq%d", index);
+	static struct clk_init_data init = {};
+	int ret;
+
+	init.name = clk_name;
+	init.flags = CLK_GET_RATE_NOCACHE;
+	init.ops = &qcom_cpufreq_hw_clk_ops;
+	data->cpu_clk.init = &init;
+
+	ret = clk_hw_register(dev, &data->cpu_clk);
+	if (ret < 0) {
+		dev_err(dev, "Failed to register Qcom CPUFreq clock\n");
+		return ret;
+	}
+
+	ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_simple_get, &data->cpu_clk);
+	if (ret < 0) {
+		dev_err(dev, "Failed to add Qcom CPUFreq clock provider\n");
+		clk_hw_unregister(&data->cpu_clk);
+	}
+
+	return ret;
+}
+
+static void qcom_cpufreq_hw_clk_remove(struct qcom_cpufreq_data *data)
+{
+	struct platform_device *pdev = cpufreq_get_driver_data();
+	struct device *dev = &pdev->dev;
+
+	of_clk_del_provider(dev->of_node);
+	clk_hw_unregister(&data->cpu_clk);
+}
+
 static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 {
 	struct platform_device *pdev = cpufreq_get_driver_data();
@@ -556,19 +606,24 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 	policy->driver_data = data;
 	policy->dvfs_possible_from_any_cpu = true;
 
+	ret = qcom_cpufreq_hw_clk_add(data, index);
+	if (ret) {
+		dev_err(dev, "Domain-%d failed to add CPU clock\n", index);
+		goto error;
+	}
+
 	ret = qcom_cpufreq_hw_read_lut(cpu_dev, policy);
 	if (ret) {
 		dev_err(dev, "Domain-%d failed to read LUT\n", index);
-		goto error;
+		goto clk_remove;
 	}
 
 	ret = dev_pm_opp_get_opp_count(cpu_dev);
 	if (ret <= 0) {
 		dev_err(cpu_dev, "Failed to add OPPs\n");
 		ret = -ENODEV;
-		goto error;
+		goto clk_remove;
 	}
-
 	if (policy_has_boost_freq(policy)) {
 		ret = cpufreq_enable_boost_support();
 		if (ret)
@@ -577,9 +632,12 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 
 	ret = qcom_cpufreq_hw_lmh_init(policy, index);
 	if (ret)
-		goto error;
+		goto clk_remove;
 
 	return 0;
+
+clk_remove:
+	qcom_cpufreq_hw_clk_remove(data);
 error:
 	kfree(data);
 unmap_base:
@@ -599,6 +657,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);
+	qcom_cpufreq_hw_clk_remove(data);
 	kfree(policy->freq_table);
 	kfree(data);
 	iounmap(base);
-- 
2.25.1

[PATCH 4/4] arm64: dts: qcom: sm8450: Supply clock from cpufreq node to CPUs

[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 6c18cfca9a34..8f26cf9aad01 100644
--- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
@@ -52,6 +52,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>;
@@ -71,6 +72,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>;
@@ -87,6 +89,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>;
@@ -103,6 +106,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>;
@@ -119,6 +123,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>;
@@ -135,6 +140,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>;
@@ -152,6 +158,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>;
@@ -168,6 +175,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>;
@@ -3804,6 +3812,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

Re: [PATCH 0/4] qcom-cpufreq-hw: Add CPU clock provider support

[Viresh Kumar]

+ Johan,

On 19-10-22, 19:29, 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 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 for working on this Mani.

Can you also test the below code over your series ? This shouldn't
result in issues that Johan reported earlier [1][2]. Below is the hack I
am carrying in the OPP core for Qcom SoCs at the moment.

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index e87567dbe99f..b7158d33c13d 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1384,20 +1384,6 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,
        }

        if (ret == -ENOENT) {
-               /*
-                * There are few platforms which don't want the OPP core to
-                * manage device's clock settings. In such cases neither the
-                * platform provides the clks explicitly to us, nor the DT
-                * contains a valid clk entry. The OPP nodes in DT may still
-                * contain "opp-hz" property though, which we need to parse and
-                * allow the platform to find an OPP based on freq later on.
-                *
-                * This is a simple solution to take care of such corner cases,
-                * i.e. make the clk_count 1, which lets us allocate space for
-                * frequency in opp->rates and also parse the entries in DT.
-                */
-               opp_table->clk_count = 1;
-
                dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, ret);
                return opp_table;
        }
diff --git a/drivers/opp/debugfs.c b/drivers/opp/debugfs.c
index 96a30a032c5f..402c507edac7 100644
--- a/drivers/opp/debugfs.c
+++ b/drivers/opp/debugfs.c
@@ -138,7 +138,7 @@ void opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
         * - For some devices rate isn't available or there are multiple, use
         *   index instead for them.
         */
-       if (likely(opp_table->clk_count == 1 && opp->rates[0]))
+       if (likely(opp_table->clk_count == 1))
                id = opp->rates[0];
        else
                id = _get_opp_count(opp_table);

-- 
viresh

[1] https://lore.kernel.org/all/YsxSkswzsqgMOc0l@hovoldconsulting.com/
[2] https://lore.kernel.org/all/Ys2FZa6YDwt7d%2FZc@hovoldconsulting.com/

Re: [PATCH 3/4] cpufreq: qcom-hw: Add CPU clock provider support

[Dmitry Baryshkov]

On 19/10/2022 16:59, Manivannan Sadhasivam wrote:
> 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 CPU policy. 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 | 67 +++++++++++++++++++++++++++++--
>   1 file changed, 63 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index a5b3b8d0e164..4dd710f9fb69 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;
>   };
> @@ -482,6 +484,54 @@ static void qcom_cpufreq_hw_lmh_exit(struct qcom_cpufreq_data *data)
>   	free_irq(data->throttle_irq, data);
>   }
>   
> +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) / HZ_PER_KHZ;
> +}
> +
> +static const struct clk_ops qcom_cpufreq_hw_clk_ops = {
> +	.recalc_rate = qcom_cpufreq_hw_recalc_rate,
> +};
> +
> +static int qcom_cpufreq_hw_clk_add(struct qcom_cpufreq_data *data, u32 index)
> +{
> +	struct platform_device *pdev = cpufreq_get_driver_data();
> +	struct device *dev = &pdev->dev;
> +	char *clk_name = devm_kasprintf(dev, GFP_KERNEL, "qcom_cpufreq%d", index);
> +	static struct clk_init_data init = {};
> +	int ret;
> +
> +	init.name = clk_name;
> +	init.flags = CLK_GET_RATE_NOCACHE;
> +	init.ops = &qcom_cpufreq_hw_clk_ops;
> +	data->cpu_clk.init = &init;
> +
> +	ret = clk_hw_register(dev, &data->cpu_clk);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to register Qcom CPUFreq clock\n");
> +		return ret;
> +	}
> +
> +	ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_simple_get, &data->cpu_clk);

This doesn't look corresponding to the DT bindings you are adding. 
of_clk_hw_simple_get() would return a single clock per dt node, 
whichever arguments were passed, while you are adding clocks 
correspoding to CPU clusters.

 From what I see according to the bindings, you should register a single 
provider using the of_clk_hw_onecell_get() function.

> +	if (ret < 0) {
> +		dev_err(dev, "Failed to add Qcom CPUFreq clock provider\n");
> +		clk_hw_unregister(&data->cpu_clk);
> +	}
> +
> +	return ret;
> +}
> +
> +static void qcom_cpufreq_hw_clk_remove(struct qcom_cpufreq_data *data)
> +{
> +	struct platform_device *pdev = cpufreq_get_driver_data();
> +	struct device *dev = &pdev->dev;
> +
> +	of_clk_del_provider(dev->of_node);
> +	clk_hw_unregister(&data->cpu_clk);
> +}
> +
>   static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>   {
>   	struct platform_device *pdev = cpufreq_get_driver_data();
> @@ -556,19 +606,24 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>   	policy->driver_data = data;
>   	policy->dvfs_possible_from_any_cpu = true;
>   
> +	ret = qcom_cpufreq_hw_clk_add(data, index);
> +	if (ret) {
> +		dev_err(dev, "Domain-%d failed to add CPU clock\n", index);
> +		goto error;
> +	}
> +
>   	ret = qcom_cpufreq_hw_read_lut(cpu_dev, policy);
>   	if (ret) {
>   		dev_err(dev, "Domain-%d failed to read LUT\n", index);
> -		goto error;
> +		goto clk_remove;
>   	}
>   
>   	ret = dev_pm_opp_get_opp_count(cpu_dev);
>   	if (ret <= 0) {
>   		dev_err(cpu_dev, "Failed to add OPPs\n");
>   		ret = -ENODEV;
> -		goto error;
> +		goto clk_remove;
>   	}
> -
>   	if (policy_has_boost_freq(policy)) {
>   		ret = cpufreq_enable_boost_support();
>   		if (ret)
> @@ -577,9 +632,12 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>   
>   	ret = qcom_cpufreq_hw_lmh_init(policy, index);
>   	if (ret)
> -		goto error;
> +		goto clk_remove;
>   
>   	return 0;
> +
> +clk_remove:
> +	qcom_cpufreq_hw_clk_remove(data);
>   error:
>   	kfree(data);
>   unmap_base:
> @@ -599,6 +657,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);
> +	qcom_cpufreq_hw_clk_remove(data);
>   	kfree(policy->freq_table);
>   	kfree(data);
>   	iounmap(base);

-- 
With best wishes
Dmitry

Re: [PATCH 2/4] dt-bindings: cpufreq: cpufreq-qcom-hw: Add cpufreq clock provider

[Rob Herring]

On Wed, 19 Oct 2022 19:29:23 +0530, Manivannan Sadhasivam wrote:
> 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>
> ---
>  .../devicetree/bindings/cpufreq/cpufreq-qcom-hw.yaml | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH 0/4] qcom-cpufreq-hw: Add CPU clock provider support

[Manivannan Sadhasivam]

On Thu, Oct 20, 2022 at 10:52:30AM +0530, Viresh Kumar wrote:
> + Johan,
> 
> On 19-10-22, 19:29, 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 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 for working on this Mani.
> 
> Can you also test the below code over your series ? This shouldn't
> result in issues that Johan reported earlier [1][2]. Below is the hack I
> am carrying in the OPP core for Qcom SoCs at the moment.
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index e87567dbe99f..b7158d33c13d 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1384,20 +1384,6 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,
>         }
> 
>         if (ret == -ENOENT) {
> -               /*
> -                * There are few platforms which don't want the OPP core to
> -                * manage device's clock settings. In such cases neither the
> -                * platform provides the clks explicitly to us, nor the DT
> -                * contains a valid clk entry. The OPP nodes in DT may still
> -                * contain "opp-hz" property though, which we need to parse and
> -                * allow the platform to find an OPP based on freq later on.
> -                *
> -                * This is a simple solution to take care of such corner cases,
> -                * i.e. make the clk_count 1, which lets us allocate space for
> -                * frequency in opp->rates and also parse the entries in DT.
> -                */
> -               opp_table->clk_count = 1;
> -
>                 dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, ret);
>                 return opp_table;
>         }
> diff --git a/drivers/opp/debugfs.c b/drivers/opp/debugfs.c
> index 96a30a032c5f..402c507edac7 100644
> --- a/drivers/opp/debugfs.c
> +++ b/drivers/opp/debugfs.c
> @@ -138,7 +138,7 @@ void opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
>          * - For some devices rate isn't available or there are multiple, use
>          *   index instead for them.
>          */
> -       if (likely(opp_table->clk_count == 1 && opp->rates[0]))
> +       if (likely(opp_table->clk_count == 1))
>                 id = opp->rates[0];
>         else
>                 id = _get_opp_count(opp_table);
> 

With the above diffs applied, I no longer see the issues reported by Johan on
SM8450 dev board.

Thanks,
Mani

> -- 
> viresh
> 
> [1] https://lore.kernel.org/all/YsxSkswzsqgMOc0l@hovoldconsulting.com/
> [2] https://lore.kernel.org/all/Ys2FZa6YDwt7d%2FZc@hovoldconsulting.com/

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

Re: [PATCH 3/4] cpufreq: qcom-hw: Add CPU clock provider support

[Manivannan Sadhasivam]

On Thu, Oct 20, 2022 at 08:39:50AM +0300, Dmitry Baryshkov wrote:
> On 19/10/2022 16:59, Manivannan Sadhasivam wrote:
> > 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 CPU policy. 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 | 67 +++++++++++++++++++++++++++++--
> >   1 file changed, 63 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> > index a5b3b8d0e164..4dd710f9fb69 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;
> >   };
> > @@ -482,6 +484,54 @@ static void qcom_cpufreq_hw_lmh_exit(struct qcom_cpufreq_data *data)
> >   	free_irq(data->throttle_irq, data);
> >   }
> > +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) / HZ_PER_KHZ;
> > +}
> > +
> > +static const struct clk_ops qcom_cpufreq_hw_clk_ops = {
> > +	.recalc_rate = qcom_cpufreq_hw_recalc_rate,
> > +};
> > +
> > +static int qcom_cpufreq_hw_clk_add(struct qcom_cpufreq_data *data, u32 index)
> > +{
> > +	struct platform_device *pdev = cpufreq_get_driver_data();
> > +	struct device *dev = &pdev->dev;
> > +	char *clk_name = devm_kasprintf(dev, GFP_KERNEL, "qcom_cpufreq%d", index);
> > +	static struct clk_init_data init = {};
> > +	int ret;
> > +
> > +	init.name = clk_name;
> > +	init.flags = CLK_GET_RATE_NOCACHE;
> > +	init.ops = &qcom_cpufreq_hw_clk_ops;
> > +	data->cpu_clk.init = &init;
> > +
> > +	ret = clk_hw_register(dev, &data->cpu_clk);
> > +	if (ret < 0) {
> > +		dev_err(dev, "Failed to register Qcom CPUFreq clock\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_simple_get, &data->cpu_clk);
> 
> This doesn't look corresponding to the DT bindings you are adding.
> of_clk_hw_simple_get() would return a single clock per dt node, whichever
> arguments were passed, while you are adding clocks correspoding to CPU
> clusters.
> 
> From what I see according to the bindings, you should register a single
> provider using the of_clk_hw_onecell_get() function.
> 

Well, that won't work either :( The detail that I missed in first place is
that the clock providers are added for the same DT node for each policy. So
there is a single clock under the clock provider for a policy but they all
belong to the same DT node.

This works when a clk provider gets added and then followed by "clk_get()"
(that's what happening during the ->init() callback). But each time a new
provider gets added, it is replacing the old for the same DT node.

The problem here is, we do not know how many policys are going to be there
during the probe time. I'll think about a proper solution and update.

Thanks,
Mani

> > +	if (ret < 0) {
> > +		dev_err(dev, "Failed to add Qcom CPUFreq clock provider\n");
> > +		clk_hw_unregister(&data->cpu_clk);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static void qcom_cpufreq_hw_clk_remove(struct qcom_cpufreq_data *data)
> > +{
> > +	struct platform_device *pdev = cpufreq_get_driver_data();
> > +	struct device *dev = &pdev->dev;
> > +
> > +	of_clk_del_provider(dev->of_node);
> > +	clk_hw_unregister(&data->cpu_clk);
> > +}
> > +
> >   static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> >   {
> >   	struct platform_device *pdev = cpufreq_get_driver_data();
> > @@ -556,19 +606,24 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> >   	policy->driver_data = data;
> >   	policy->dvfs_possible_from_any_cpu = true;
> > +	ret = qcom_cpufreq_hw_clk_add(data, index);
> > +	if (ret) {
> > +		dev_err(dev, "Domain-%d failed to add CPU clock\n", index);
> > +		goto error;
> > +	}
> > +
> >   	ret = qcom_cpufreq_hw_read_lut(cpu_dev, policy);
> >   	if (ret) {
> >   		dev_err(dev, "Domain-%d failed to read LUT\n", index);
> > -		goto error;
> > +		goto clk_remove;
> >   	}
> >   	ret = dev_pm_opp_get_opp_count(cpu_dev);
> >   	if (ret <= 0) {
> >   		dev_err(cpu_dev, "Failed to add OPPs\n");
> >   		ret = -ENODEV;
> > -		goto error;
> > +		goto clk_remove;
> >   	}
> > -
> >   	if (policy_has_boost_freq(policy)) {
> >   		ret = cpufreq_enable_boost_support();
> >   		if (ret)
> > @@ -577,9 +632,12 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> >   	ret = qcom_cpufreq_hw_lmh_init(policy, index);
> >   	if (ret)
> > -		goto error;
> > +		goto clk_remove;
> >   	return 0;
> > +
> > +clk_remove:
> > +	qcom_cpufreq_hw_clk_remove(data);
> >   error:
> >   	kfree(data);
> >   unmap_base:
> > @@ -599,6 +657,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);
> > +	qcom_cpufreq_hw_clk_remove(data);
> >   	kfree(policy->freq_table);
> >   	kfree(data);
> >   	iounmap(base);
> 
> -- 
> With best wishes
> Dmitry
> 

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

Re: [PATCH 3/4] cpufreq: qcom-hw: Add CPU clock provider support

[Bjorn Andersson]

On Fri, Oct 21, 2022 at 03:01:40PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Oct 20, 2022 at 08:39:50AM +0300, Dmitry Baryshkov wrote:
> > On 19/10/2022 16:59, Manivannan Sadhasivam wrote:
> > > 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 CPU policy. 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 | 67 +++++++++++++++++++++++++++++--
> > >   1 file changed, 63 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> > > index a5b3b8d0e164..4dd710f9fb69 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;
> > >   };
> > > @@ -482,6 +484,54 @@ static void qcom_cpufreq_hw_lmh_exit(struct qcom_cpufreq_data *data)
> > >   	free_irq(data->throttle_irq, data);
> > >   }
> > > +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) / HZ_PER_KHZ;
> > > +}
> > > +
> > > +static const struct clk_ops qcom_cpufreq_hw_clk_ops = {
> > > +	.recalc_rate = qcom_cpufreq_hw_recalc_rate,
> > > +};
> > > +
> > > +static int qcom_cpufreq_hw_clk_add(struct qcom_cpufreq_data *data, u32 index)
> > > +{
> > > +	struct platform_device *pdev = cpufreq_get_driver_data();
> > > +	struct device *dev = &pdev->dev;
> > > +	char *clk_name = devm_kasprintf(dev, GFP_KERNEL, "qcom_cpufreq%d", index);
> > > +	static struct clk_init_data init = {};
> > > +	int ret;
> > > +
> > > +	init.name = clk_name;
> > > +	init.flags = CLK_GET_RATE_NOCACHE;
> > > +	init.ops = &qcom_cpufreq_hw_clk_ops;
> > > +	data->cpu_clk.init = &init;
> > > +
> > > +	ret = clk_hw_register(dev, &data->cpu_clk);
> > > +	if (ret < 0) {
> > > +		dev_err(dev, "Failed to register Qcom CPUFreq clock\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_simple_get, &data->cpu_clk);
> > 
> > This doesn't look corresponding to the DT bindings you are adding.
> > of_clk_hw_simple_get() would return a single clock per dt node, whichever
> > arguments were passed, while you are adding clocks correspoding to CPU
> > clusters.
> > 
> > From what I see according to the bindings, you should register a single
> > provider using the of_clk_hw_onecell_get() function.
> > 
> 
> Well, that won't work either :( The detail that I missed in first place is
> that the clock providers are added for the same DT node for each policy. So
> there is a single clock under the clock provider for a policy but they all
> belong to the same DT node.
> 
> This works when a clk provider gets added and then followed by "clk_get()"
> (that's what happening during the ->init() callback). But each time a new
> provider gets added, it is replacing the old for the same DT node.
> 
> The problem here is, we do not know how many policys are going to be there
> during the probe time. I'll think about a proper solution and update.
> 

You could get this by looping over all the cpus and count how many
unique qcom,freq-domains you have.

But it seems like a bigger problem is that you need to register your
clock "provider" at a device-level, rather than a policy level. I did
some experiments with moving most of the resource management to probe
and it did look quite promising, but in the end I figured out a shorter
path to per-core frequency voting and threw that code out again.

It seems however that this would be a good idea to pick up.


Beyond resolving Viresh request though, we have the problem that on
SM8350 and SC8280XP (at least), the L3 cache is controlled by per-core
registers residing in the register blocks hogged by the cpufreq driver,
and is configured in unit of Hz. So we can't directly use the osm-l3
model - without hacking up the drivers to allow for overlapping ioremap.

We could probably extend the cpufreq driver to express this as a path
between each core and the L3 cache and just ignore the unit (kBps vs Hz)
(i.e.  duplicate osm-l3 in the cpufreq driver).
But it doesn't seem unreasonable to me to express this as a clock per
CPU and just add another opp-hz value to the opp-table, now that this is
supported.

This design would also allow for profiling based mechanisms to pick
these clocks up and issue clk_set_rate(), if such mechanisms would be
desirable.

Regards,
Bjorn

Re: [PATCH 3/4] cpufreq: qcom-hw: Add CPU clock provider support

[Manivannan Sadhasivam]

On Sun, Oct 23, 2022 at 10:06:48PM -0500, Bjorn Andersson wrote:
> On Fri, Oct 21, 2022 at 03:01:40PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Oct 20, 2022 at 08:39:50AM +0300, Dmitry Baryshkov wrote:
> > > On 19/10/2022 16:59, Manivannan Sadhasivam wrote:
> > > > 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 CPU policy. 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 | 67 +++++++++++++++++++++++++++++--
> > > >   1 file changed, 63 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> > > > index a5b3b8d0e164..4dd710f9fb69 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;
> > > >   };
> > > > @@ -482,6 +484,54 @@ static void qcom_cpufreq_hw_lmh_exit(struct qcom_cpufreq_data *data)
> > > >   	free_irq(data->throttle_irq, data);
> > > >   }
> > > > +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) / HZ_PER_KHZ;
> > > > +}
> > > > +
> > > > +static const struct clk_ops qcom_cpufreq_hw_clk_ops = {
> > > > +	.recalc_rate = qcom_cpufreq_hw_recalc_rate,
> > > > +};
> > > > +
> > > > +static int qcom_cpufreq_hw_clk_add(struct qcom_cpufreq_data *data, u32 index)
> > > > +{
> > > > +	struct platform_device *pdev = cpufreq_get_driver_data();
> > > > +	struct device *dev = &pdev->dev;
> > > > +	char *clk_name = devm_kasprintf(dev, GFP_KERNEL, "qcom_cpufreq%d", index);
> > > > +	static struct clk_init_data init = {};
> > > > +	int ret;
> > > > +
> > > > +	init.name = clk_name;
> > > > +	init.flags = CLK_GET_RATE_NOCACHE;
> > > > +	init.ops = &qcom_cpufreq_hw_clk_ops;
> > > > +	data->cpu_clk.init = &init;
> > > > +
> > > > +	ret = clk_hw_register(dev, &data->cpu_clk);
> > > > +	if (ret < 0) {
> > > > +		dev_err(dev, "Failed to register Qcom CPUFreq clock\n");
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_simple_get, &data->cpu_clk);
> > > 
> > > This doesn't look corresponding to the DT bindings you are adding.
> > > of_clk_hw_simple_get() would return a single clock per dt node, whichever
> > > arguments were passed, while you are adding clocks correspoding to CPU
> > > clusters.
> > > 
> > > From what I see according to the bindings, you should register a single
> > > provider using the of_clk_hw_onecell_get() function.
> > > 
> > 
> > Well, that won't work either :( The detail that I missed in first place is
> > that the clock providers are added for the same DT node for each policy. So
> > there is a single clock under the clock provider for a policy but they all
> > belong to the same DT node.
> > 
> > This works when a clk provider gets added and then followed by "clk_get()"
> > (that's what happening during the ->init() callback). But each time a new
> > provider gets added, it is replacing the old for the same DT node.
> > 
> > The problem here is, we do not know how many policys are going to be there
> > during the probe time. I'll think about a proper solution and update.
> > 
> 
> You could get this by looping over all the cpus and count how many
> unique qcom,freq-domains you have.
> 

I just counted the number of "freq-domainX" register spaces defined in cpufreq
node and used that as the domain count.

> But it seems like a bigger problem is that you need to register your
> clock "provider" at a device-level, rather than a policy level. I did
> some experiments with moving most of the resource management to probe
> and it did look quite promising, but in the end I figured out a shorter
> path to per-core frequency voting and threw that code out again.
> 
> It seems however that this would be a good idea to pick up.
> 

This is what exactly I've done now (not posted yet). Moving the resource
management is indeed the correct way since the resources are static and not
tied to the CPUs. Plus it allows us to use devm_ helpers for tieing all the
resources to the device.

> 
> Beyond resolving Viresh request though, we have the problem that on
> SM8350 and SC8280XP (at least), the L3 cache is controlled by per-core
> registers residing in the register blocks hogged by the cpufreq driver,
> and is configured in unit of Hz. So we can't directly use the osm-l3
> model - without hacking up the drivers to allow for overlapping ioremap.
> 
> We could probably extend the cpufreq driver to express this as a path
> between each core and the L3 cache and just ignore the unit (kBps vs Hz)
> (i.e.  duplicate osm-l3 in the cpufreq driver).
> But it doesn't seem unreasonable to me to express this as a clock per
> CPU and just add another opp-hz value to the opp-table, now that this is
> supported.
> 
> This design would also allow for profiling based mechanisms to pick
> these clocks up and issue clk_set_rate(), if such mechanisms would be
> desirable.
> 

This sounds reasonable to me. Let's discuss this offline and come up with a
design.

Thanks,
Mani

> Regards,
> Bjorn

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