[PATCH v4 0/4] CPUFreq: Add support for opp-sharing cpus

[PATCH v4 0/4] CPUFreq: Add support for opp-sharing cpus

[Nicola Mazzucato]

Hi All,

In this V4 posting I have fixed suggestions on opp/of and have added the
implementation for scmi-cpufreq driver.

This is to support systems where exposed cpu performance controls are more
fine-grained that the platform's ability to scale cpus independently.

[v4]
  * Remove unconditional set of opp_table->shared_opp to exclusive
  * Add implementation for scmi-cpufreq
  * Change subject

These patches are on top of:
next-20201201 + Lukasz Luba's patches (waiting for Rafael) [1]

[v3]
  * Remove proposal for new 'cpu-performance-dependencies' as we instead
    can reuse the opp table.
  * Update documentation for devicetree/bindings/opp
  * Minor changes within opp to support empty opp table
  * Rework the RFC by adding a second proposal

[v2]
  * Fix errors when running make dt_binding_check
  * Improve commit message description for the dt-binding
  * Add RFC for implementation in cpufreq-core and one of its
    drivers.

Nicola Mazzucato (3):
  dt-bindings/opp: Update documentation for opp-shared
  opp/of: Allow empty opp-table with opp-shared
  scmi-cpufreq: get opp_shared_cpus from opp-v2 for EM

Sudeep Holla (1):
  cpufreq: blacklist Arm Vexpress platforms in cpufreq-dt-platdev

 Documentation/devicetree/bindings/opp/opp.txt | 53 +++++++++++++++++++
 drivers/cpufreq/cpufreq-dt-platdev.c          |  2 +
 drivers/cpufreq/scmi-cpufreq.c                | 51 ++++++++++++------
 drivers/opp/of.c                              |  7 ++-
 4 files changed, 95 insertions(+), 18 deletions(-)

[1] https://lore.kernel.org/linux-pm/20201124104346.27167-1-lukasz.luba@arm.com/

-- 
2.27.0

[PATCH v4 1/4] dt-bindings/opp: Update documentation for opp-shared

[Nicola Mazzucato]

Currently the optional property opp-shared is used within an opp table
to tell that a set of devices share their clock/voltage lines (and the
opp points).
It is therefore possible to use an empty opp table to convey only that
information, useful in situations where the opp points are provided via
other means (hardware. firmware, etc).

Update the documentation to remark this additional case and provide an
example.

Signed-off-by: Nicola Mazzucato <nicola.mazzucato@arm.com>
---
 Documentation/devicetree/bindings/opp/opp.txt | 53 +++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
index 9847dfeeffcb..a5f1f993eab9 100644
--- a/Documentation/devicetree/bindings/opp/opp.txt
+++ b/Documentation/devicetree/bindings/opp/opp.txt
@@ -72,6 +72,9 @@ Optional properties:
   switch their DVFS state together, i.e. they share clock/voltage/current lines.
   Missing property means devices have independent clock/voltage/current lines,
   but they share OPP tables.
+  This optional property can be used without any OPP nodes when its only purpose
+  is to describe a dependency of clock/voltage/current lines among a set of
+  devices.
 
 - status: Marks the OPP table enabled/disabled.
 
@@ -568,3 +571,53 @@ Example 6: opp-microvolt-<name>, opp-microamp-<name>:
 		};
 	};
 };
+
+Example 7: Single cluster Quad-core ARM cortex A53, OPP points from firmware,
+distinct clock controls but two sets of clock/voltage/current lines.
+
+/ {
+	cpus {
+		#address-cells = <2>;
+		#size-cells = <0>;
+
+		cpu@0 {
+			compatible = "arm,cortex-a53";
+			reg = <0x0 0x100>;
+			next-level-cache = <&A53_L2>;
+			clocks = <&dvfs_controller 0>;
+			operating-points-v2 = <&cpu_opp0_table>;
+		};
+		cpu@1 {
+			compatible = "arm,cortex-a53";
+			reg = <0x0 0x101>;
+			next-level-cache = <&A53_L2>;
+			clocks = <&dvfs_controller 1>;
+			operating-points-v2 = <&cpu_opp0_table>;
+		};
+		cpu@2 {
+			compatible = "arm,cortex-a53";
+			reg = <0x0 0x102>;
+			next-level-cache = <&A53_L2>;
+			clocks = <&dvfs_controller 2>;
+			operating-points-v2 = <&cpu_opp1_table>;
+		};
+		cpu@3 {
+			compatible = "arm,cortex-a53";
+			reg = <0x0 0x103>;
+			next-level-cache = <&A53_L2>;
+			clocks = <&dvfs_controller 3>;
+			operating-points-v2 = <&cpu_opp1_table>;
+		};
+
+	};
+
+	cpu_opp0_table: opp0_table {
+		compatible = "operating-points-v2";
+		opp-shared;
+	};
+
+	cpu_opp1_table: opp1_table {
+		compatible = "operating-points-v2";
+		opp-shared;
+	};
+};
-- 
2.27.0

[PATCH v4 2/4] opp/of: Allow empty opp-table with opp-shared

[Nicola Mazzucato]

The opp binding now allows to have an empty opp table and shared-opp to
still describe that devices share v/f lines.

When initialising an empty opp table, allow such case by:
- treating such conditions with warnings in place of errors
- don't fail on empty table

Signed-off-by: Nicola Mazzucato <nicola.mazzucato@arm.com>
---
 drivers/opp/of.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index ffd937af8089..03cb387236c4 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -170,7 +170,8 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
 	/* Traversing the first OPP node is all we need */
 	np = of_get_next_available_child(opp_np, NULL);
 	if (!np) {
-		dev_err(dev, "Empty OPP table\n");
+		dev_warn(dev, "Empty OPP table\n");
+
 		return;
 	}
 
@@ -378,7 +379,9 @@ int dev_pm_opp_of_find_icc_paths(struct device *dev,
 	struct icc_path **paths;
 
 	ret = _bandwidth_supported(dev, opp_table);
-	if (ret <= 0)
+	if (ret == -EINVAL)
+		return 0; /* Empty OPP table is a valid corner-case, let's not fail */
+	else if (ret <= 0)
 		return ret;
 
 	ret = 0;
-- 
2.27.0

[PATCH v4 3/4] scmi-cpufreq: get opp_shared_cpus from opp-v2 for EM

[Nicola Mazzucato]

By design, SCMI performance domains define the granularity of
performance controls, they do not describe any underlying hardware
dependencies (although they may match in many cases).

It is therefore possible to have some platforms where hardware may have
the ability to control CPU performance at different granularity and choose
to describe fine-grained performance control through SCMI.

In such situations, the energy model would be provided with inaccurate
information based on controls, while it still needs to know the
performance boundaries.

To restore correct functionality, retrieve information of CPUs under the
same v/f domain from operating-points-v2 in DT, and pass it on to EM.

Signed-off-by: Nicola Mazzucato <nicola.mazzucato@arm.com>
---
 drivers/cpufreq/scmi-cpufreq.c | 51 +++++++++++++++++++++++-----------
 1 file changed, 35 insertions(+), 16 deletions(-)

diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
index 491a0a24fb1e..f505efcc62b1 100644
--- a/drivers/cpufreq/scmi-cpufreq.c
+++ b/drivers/cpufreq/scmi-cpufreq.c
@@ -127,6 +127,7 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
 	struct cpufreq_frequency_table *freq_table;
 	struct em_data_callback em_cb = EM_DATA_CB(scmi_get_cpu_power);
 	bool power_scale_mw;
+	cpumask_var_t opp_shared_cpus;
 
 	cpu_dev = get_cpu_device(policy->cpu);
 	if (!cpu_dev) {
@@ -134,30 +135,45 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
 		return -ENODEV;
 	}
 
-	ret = handle->perf_ops->device_opps_add(handle, cpu_dev);
-	if (ret) {
-		dev_warn(cpu_dev, "failed to add opps to the device\n");
-		return ret;
-	}
+	if (!zalloc_cpumask_var(&opp_shared_cpus, GFP_KERNEL))
+		return -ENOMEM;
 
 	ret = scmi_get_sharing_cpus(cpu_dev, policy->cpus);
 	if (ret) {
 		dev_warn(cpu_dev, "failed to get sharing cpumask\n");
-		return ret;
+		goto out_free_cpumask;
 	}
 
-	ret = dev_pm_opp_set_sharing_cpus(cpu_dev, policy->cpus);
-	if (ret) {
-		dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n",
-			__func__, ret);
-		return ret;
+	/*
+	 * The OPP 'sharing cpus' info may come from dt through an empty opp
+	 * table and opp-shared. If found, it takes precedence over the SCMI
+	 * domain IDs info.
+	 */
+	ret = dev_pm_opp_of_get_sharing_cpus(cpu_dev, opp_shared_cpus);
+	if (ret || !cpumask_weight(opp_shared_cpus)) {
+		/*
+		 * Either opp-table is not set or no opp-shared was found,
+		 * use the information from SCMI domain IDs.
+		 */
+		cpumask_copy(opp_shared_cpus, policy->cpus);
 	}
 
 	nr_opp = dev_pm_opp_get_opp_count(cpu_dev);
 	if (nr_opp <= 0) {
-		dev_dbg(cpu_dev, "OPP table is not ready, deferring probe\n");
-		ret = -EPROBE_DEFER;
-		goto out_free_opp;
+		ret = handle->perf_ops->device_opps_add(handle, cpu_dev);
+		if (ret) {
+			dev_warn(cpu_dev, "failed to add opps to the device\n");
+			goto out_free_cpumask;
+		}
+
+		ret = dev_pm_opp_set_sharing_cpus(cpu_dev, opp_shared_cpus);
+		if (ret) {
+			dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n",
+				__func__, ret);
+			goto out_free_cpumask;
+		}
+
+		nr_opp = dev_pm_opp_get_opp_count(cpu_dev);
 	}
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
@@ -191,15 +207,18 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
 		handle->perf_ops->fast_switch_possible(handle, cpu_dev);
 
 	power_scale_mw = handle->perf_ops->power_scale_mw_get(handle);
-	em_dev_register_perf_domain(cpu_dev, nr_opp, &em_cb, policy->cpus,
+	em_dev_register_perf_domain(cpu_dev, nr_opp, &em_cb, opp_shared_cpus,
 				    power_scale_mw);
 
-	return 0;
+	ret = 0;
+	goto out_free_cpumask;
 
 out_free_priv:
 	kfree(priv);
 out_free_opp:
 	dev_pm_opp_remove_all_dynamic(cpu_dev);
+out_free_cpumask:
+	free_cpumask_var(opp_shared_cpus);
 
 	return ret;
 }
-- 
2.27.0

[PATCH v4 4/4] cpufreq: blacklist Arm Vexpress platforms in cpufreq-dt-platdev

[Nicola Mazzucato]

From: Sudeep Holla <sudeep.holla@arm.com>

Add "arm,vexpress" to cpufreq-dt-platdev blacklist since the actual
scaling is handled by the firmware cpufreq drivers(scpi, scmi and
vexpress-spc).

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/cpufreq/cpufreq-dt-platdev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
index bd2db0188cbb..91e6a0c10dbf 100644
--- a/drivers/cpufreq/cpufreq-dt-platdev.c
+++ b/drivers/cpufreq/cpufreq-dt-platdev.c
@@ -103,6 +103,8 @@ static const struct of_device_id whitelist[] __initconst = {
 static const struct of_device_id blacklist[] __initconst = {
 	{ .compatible = "allwinner,sun50i-h6", },
 
+	{ .compatible = "arm,vexpress", },
+
 	{ .compatible = "calxeda,highbank", },
 	{ .compatible = "calxeda,ecx-2000", },
 
-- 
2.27.0

Re: [PATCH v4 1/4] dt-bindings/opp: Update documentation for opp-shared

[Viresh Kumar]

Subject should rather be:

dt-bindings: opp: Allow empty OPP tables

On 02-12-20, 17:23, Nicola Mazzucato wrote:
> Currently the optional property opp-shared is used within an opp table
> to tell that a set of devices share their clock/voltage lines (and the
> opp points).
> It is therefore possible to use an empty opp table to convey only that
> information, useful in situations where the opp points are provided via
> other means (hardware. firmware, etc).
> 
> Update the documentation to remark this additional case and provide an
> example.
> 
> Signed-off-by: Nicola Mazzucato <nicola.mazzucato@arm.com>
> ---
>  Documentation/devicetree/bindings/opp/opp.txt | 53 +++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
> index 9847dfeeffcb..a5f1f993eab9 100644
> --- a/Documentation/devicetree/bindings/opp/opp.txt
> +++ b/Documentation/devicetree/bindings/opp/opp.txt
> @@ -72,6 +72,9 @@ Optional properties:
>    switch their DVFS state together, i.e. they share clock/voltage/current lines.
>    Missing property means devices have independent clock/voltage/current lines,
>    but they share OPP tables.
> +  This optional property can be used without any OPP nodes when its only purpose
> +  is to describe a dependency of clock/voltage/current lines among a set of
> +  devices.

And instead of this, we should rather update "OPP nodes:" section like
this:

diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
index 9847dfeeffcb..28077ce3a845 100644
--- a/Documentation/devicetree/bindings/opp/opp.txt
+++ b/Documentation/devicetree/bindings/opp/opp.txt
@@ -63,11 +63,13 @@ This describes the OPPs belonging to a device. This node can have following
 - compatible: Allow OPPs to express their compatibility. It should be:
   "operating-points-v2".
 
+Optional properties:
 - OPP nodes: One or more OPP nodes describing voltage-current-frequency
   combinations. Their name isn't significant but their phandle can be used to
-  reference an OPP.
+  reference an OPP. These are mandatory except for the case where the OPP table
+  is present only to indicate dependency between devices using the opp-shared
+  property.
 
-Optional properties:
 - opp-shared: Indicates that device nodes using this OPP Table Node's phandle
   switch their DVFS state together, i.e. they share clock/voltage/current lines.
   Missing property means devices have independent clock/voltage/current lines,

-- 
viresh

Re: [PATCH v4 3/4] scmi-cpufreq: get opp_shared_cpus from opp-v2 for EM

[Viresh Kumar]

On 02-12-20, 17:23, Nicola Mazzucato wrote:
> By design, SCMI performance domains define the granularity of
> performance controls, they do not describe any underlying hardware
> dependencies (although they may match in many cases).
> 
> It is therefore possible to have some platforms where hardware may have
> the ability to control CPU performance at different granularity and choose
> to describe fine-grained performance control through SCMI.
> 
> In such situations, the energy model would be provided with inaccurate
> information based on controls, while it still needs to know the
> performance boundaries.
> 
> To restore correct functionality, retrieve information of CPUs under the
> same v/f domain from operating-points-v2 in DT, and pass it on to EM.
> 
> Signed-off-by: Nicola Mazzucato <nicola.mazzucato@arm.com>
> ---
>  drivers/cpufreq/scmi-cpufreq.c | 51 +++++++++++++++++++++++-----------
>  1 file changed, 35 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
> index 491a0a24fb1e..f505efcc62b1 100644
> --- a/drivers/cpufreq/scmi-cpufreq.c
> +++ b/drivers/cpufreq/scmi-cpufreq.c
> @@ -127,6 +127,7 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
>  	struct cpufreq_frequency_table *freq_table;
>  	struct em_data_callback em_cb = EM_DATA_CB(scmi_get_cpu_power);
>  	bool power_scale_mw;
> +	cpumask_var_t opp_shared_cpus;
>  
>  	cpu_dev = get_cpu_device(policy->cpu);
>  	if (!cpu_dev) {
> @@ -134,30 +135,45 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
>  		return -ENODEV;
>  	}
>  
> -	ret = handle->perf_ops->device_opps_add(handle, cpu_dev);
> -	if (ret) {
> -		dev_warn(cpu_dev, "failed to add opps to the device\n");
> -		return ret;
> -	}
> +	if (!zalloc_cpumask_var(&opp_shared_cpus, GFP_KERNEL))
> +		return -ENOMEM;
>  
>  	ret = scmi_get_sharing_cpus(cpu_dev, policy->cpus);
>  	if (ret) {
>  		dev_warn(cpu_dev, "failed to get sharing cpumask\n");
> -		return ret;
> +		goto out_free_cpumask;
>  	}
>  
> -	ret = dev_pm_opp_set_sharing_cpus(cpu_dev, policy->cpus);
> -	if (ret) {
> -		dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n",
> -			__func__, ret);
> -		return ret;
> +	/*
> +	 * The OPP 'sharing cpus' info may come from dt through an empty opp
> +	 * table and opp-shared. If found, it takes precedence over the SCMI
> +	 * domain IDs info.
> +	 */
> +	ret = dev_pm_opp_of_get_sharing_cpus(cpu_dev, opp_shared_cpus);
> +	if (ret || !cpumask_weight(opp_shared_cpus)) {
> +		/*
> +		 * Either opp-table is not set or no opp-shared was found,
> +		 * use the information from SCMI domain IDs.
> +		 */
> +		cpumask_copy(opp_shared_cpus, policy->cpus);
>  	}
>  
>  	nr_opp = dev_pm_opp_get_opp_count(cpu_dev);
>  	if (nr_opp <= 0) {
> -		dev_dbg(cpu_dev, "OPP table is not ready, deferring probe\n");
> -		ret = -EPROBE_DEFER;
> -		goto out_free_opp;
> +		ret = handle->perf_ops->device_opps_add(handle, cpu_dev);
> +		if (ret) {
> +			dev_warn(cpu_dev, "failed to add opps to the device\n");
> +			goto out_free_cpumask;
> +		}
> +
> +		ret = dev_pm_opp_set_sharing_cpus(cpu_dev, opp_shared_cpus);
> +		if (ret) {
> +			dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n",
> +				__func__, ret);
> +			goto out_free_cpumask;
> +		}
> +

Why do we need to call above two after calling
dev_pm_opp_get_opp_count() ? And we don't check the return value of
the below call anymore, moreover we have to call it twice now.

> +		nr_opp = dev_pm_opp_get_opp_count(cpu_dev);
>  	}
>  
>  	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> @@ -191,15 +207,18 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
>  		handle->perf_ops->fast_switch_possible(handle, cpu_dev);
>  
>  	power_scale_mw = handle->perf_ops->power_scale_mw_get(handle);
> -	em_dev_register_perf_domain(cpu_dev, nr_opp, &em_cb, policy->cpus,
> +	em_dev_register_perf_domain(cpu_dev, nr_opp, &em_cb, opp_shared_cpus,
>  				    power_scale_mw);
>  
> -	return 0;
> +	ret = 0;

ret is already 0 here.

> +	goto out_free_cpumask;
>  
>  out_free_priv:
>  	kfree(priv);
>  out_free_opp:
>  	dev_pm_opp_remove_all_dynamic(cpu_dev);
> +out_free_cpumask:
> +	free_cpumask_var(opp_shared_cpus);
>  
>  	return ret;
>  }
> -- 
> 2.27.0

-- 
viresh

Re: [PATCH v4 1/4] dt-bindings/opp: Update documentation for opp-shared

[Nicola Mazzucato]

Hi Viresh,

thanks for your comments. I will update subject and content as you suggested.

Regards,
Nicola

On 12/8/20 4:29 AM, Viresh Kumar wrote:
> Subject should rather be:
> 
> dt-bindings: opp: Allow empty OPP tables
> 
> On 02-12-20, 17:23, Nicola Mazzucato wrote:
>> Currently the optional property opp-shared is used within an opp table
>> to tell that a set of devices share their clock/voltage lines (and the
>> opp points).
>> It is therefore possible to use an empty opp table to convey only that
>> information, useful in situations where the opp points are provided via
>> other means (hardware. firmware, etc).
>>
>> Update the documentation to remark this additional case and provide an
>> example.
>>
>> Signed-off-by: Nicola Mazzucato <nicola.mazzucato@arm.com>
>> ---
>>  Documentation/devicetree/bindings/opp/opp.txt | 53 +++++++++++++++++++
>>  1 file changed, 53 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
>> index 9847dfeeffcb..a5f1f993eab9 100644
>> --- a/Documentation/devicetree/bindings/opp/opp.txt
>> +++ b/Documentation/devicetree/bindings/opp/opp.txt
>> @@ -72,6 +72,9 @@ Optional properties:
>>    switch their DVFS state together, i.e. they share clock/voltage/current lines.
>>    Missing property means devices have independent clock/voltage/current lines,
>>    but they share OPP tables.
>> +  This optional property can be used without any OPP nodes when its only purpose
>> +  is to describe a dependency of clock/voltage/current lines among a set of
>> +  devices.
> 
> And instead of this, we should rather update "OPP nodes:" section like
> this:
> 
> diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
> index 9847dfeeffcb..28077ce3a845 100644
> --- a/Documentation/devicetree/bindings/opp/opp.txt
> +++ b/Documentation/devicetree/bindings/opp/opp.txt
> @@ -63,11 +63,13 @@ This describes the OPPs belonging to a device. This node can have following
>  - compatible: Allow OPPs to express their compatibility. It should be:
>    "operating-points-v2".
>  
> +Optional properties:
>  - OPP nodes: One or more OPP nodes describing voltage-current-frequency
>    combinations. Their name isn't significant but their phandle can be used to
> -  reference an OPP.
> +  reference an OPP. These are mandatory except for the case where the OPP table
> +  is present only to indicate dependency between devices using the opp-shared
> +  property.
>  
> -Optional properties:
>  - opp-shared: Indicates that device nodes using this OPP Table Node's phandle
>    switch their DVFS state together, i.e. they share clock/voltage/current lines.
>    Missing property means devices have independent clock/voltage/current lines,
> 

Re: [PATCH v4 3/4] scmi-cpufreq: get opp_shared_cpus from opp-v2 for EM

[Nicola Mazzucato]

Hi Viresh,

thanks for looking into this. Please find below

On 12/8/20 5:50 AM, Viresh Kumar wrote:
> On 02-12-20, 17:23, Nicola Mazzucato wrote:
>> By design, SCMI performance domains define the granularity of
>> performance controls, they do not describe any underlying hardware
>> dependencies (although they may match in many cases).
>>
>> It is therefore possible to have some platforms where hardware may have
>> the ability to control CPU performance at different granularity and choose
>> to describe fine-grained performance control through SCMI.
>>
>> In such situations, the energy model would be provided with inaccurate
>> information based on controls, while it still needs to know the
>> performance boundaries.
>>
>> To restore correct functionality, retrieve information of CPUs under the
>> same v/f domain from operating-points-v2 in DT, and pass it on to EM.
>>
>> Signed-off-by: Nicola Mazzucato <nicola.mazzucato@arm.com>
>> ---
>>  drivers/cpufreq/scmi-cpufreq.c | 51 +++++++++++++++++++++++-----------
>>  1 file changed, 35 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
>> index 491a0a24fb1e..f505efcc62b1 100644
>> --- a/drivers/cpufreq/scmi-cpufreq.c
>> +++ b/drivers/cpufreq/scmi-cpufreq.c
>> @@ -127,6 +127,7 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
>>  	struct cpufreq_frequency_table *freq_table;
>>  	struct em_data_callback em_cb = EM_DATA_CB(scmi_get_cpu_power);
>>  	bool power_scale_mw;
>> +	cpumask_var_t opp_shared_cpus;
>>  
>>  	cpu_dev = get_cpu_device(policy->cpu);
>>  	if (!cpu_dev) {
>> @@ -134,30 +135,45 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
>>  		return -ENODEV;
>>  	}
>>  
>> -	ret = handle->perf_ops->device_opps_add(handle, cpu_dev);
>> -	if (ret) {
>> -		dev_warn(cpu_dev, "failed to add opps to the device\n");
>> -		return ret;
>> -	}
>> +	if (!zalloc_cpumask_var(&opp_shared_cpus, GFP_KERNEL))
>> +		return -ENOMEM;
>>  
>>  	ret = scmi_get_sharing_cpus(cpu_dev, policy->cpus);
>>  	if (ret) {
>>  		dev_warn(cpu_dev, "failed to get sharing cpumask\n");
>> -		return ret;
>> +		goto out_free_cpumask;
>>  	}
>>  
>> -	ret = dev_pm_opp_set_sharing_cpus(cpu_dev, policy->cpus);
>> -	if (ret) {
>> -		dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n",
>> -			__func__, ret);
>> -		return ret;
>> +	/*
>> +	 * The OPP 'sharing cpus' info may come from dt through an empty opp
>> +	 * table and opp-shared. If found, it takes precedence over the SCMI
>> +	 * domain IDs info.
>> +	 */
>> +	ret = dev_pm_opp_of_get_sharing_cpus(cpu_dev, opp_shared_cpus);
>> +	if (ret || !cpumask_weight(opp_shared_cpus)) {
>> +		/*
>> +		 * Either opp-table is not set or no opp-shared was found,
>> +		 * use the information from SCMI domain IDs.
>> +		 */
>> +		cpumask_copy(opp_shared_cpus, policy->cpus);
>>  	}
>>  
>>  	nr_opp = dev_pm_opp_get_opp_count(cpu_dev);
>>  	if (nr_opp <= 0) {
>> -		dev_dbg(cpu_dev, "OPP table is not ready, deferring probe\n");
>> -		ret = -EPROBE_DEFER;
>> -		goto out_free_opp;
>> +		ret = handle->perf_ops->device_opps_add(handle, cpu_dev);
>> +		if (ret) {
>> +			dev_warn(cpu_dev, "failed to add opps to the device\n");
>> +			goto out_free_cpumask;
>> +		}
>> +
>> +		ret = dev_pm_opp_set_sharing_cpus(cpu_dev, opp_shared_cpus);
>> +		if (ret) {
>> +			dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n",
>> +				__func__, ret);
>> +			goto out_free_cpumask;
>> +		}
>> +
> 
> Why do we need to call above two after calling
> dev_pm_opp_get_opp_count() ?

Sorry, I am not sure to understand your question here. If there are no opps for
a device we want to add them to it, otherwise no need as they would be duplicated.

And we don't check the return value of
> the below call anymore, moreover we have to call it twice now.

This second get_opp_count is required such that we register em with the correct
opp number after having added them. Without this the opp_count would not be correct.

Hope I have answered your questions.
> 
>> +		nr_opp = dev_pm_opp_get_opp_count(cpu_dev);
>>  	}
>>  
>>  	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>> @@ -191,15 +207,18 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
>>  		handle->perf_ops->fast_switch_possible(handle, cpu_dev);
>>  
>>  	power_scale_mw = handle->perf_ops->power_scale_mw_get(handle);
>> -	em_dev_register_perf_domain(cpu_dev, nr_opp, &em_cb, policy->cpus,
>> +	em_dev_register_perf_domain(cpu_dev, nr_opp, &em_cb, opp_shared_cpus,
>>  				    power_scale_mw);
>>  
>> -	return 0;
>> +	ret = 0;
> 
> ret is already 0 here.

true, nice spot, thanks

> 
>> +	goto out_free_cpumask;
>>  
>>  out_free_priv:
>>  	kfree(priv);
>>  out_free_opp:
>>  	dev_pm_opp_remove_all_dynamic(cpu_dev);
>> +out_free_cpumask:
>> +	free_cpumask_var(opp_shared_cpus);
>>  
>>  	return ret;
>>  }
>> -- 
>> 2.27.0
> 

Re: [PATCH v4 3/4] scmi-cpufreq: get opp_shared_cpus from opp-v2 for EM

[Viresh Kumar]

On 08-12-20, 07:22, Nicola Mazzucato wrote:
> On 12/8/20 5:50 AM, Viresh Kumar wrote:
> > On 02-12-20, 17:23, Nicola Mazzucato wrote:
> >>  	nr_opp = dev_pm_opp_get_opp_count(cpu_dev);
> >>  	if (nr_opp <= 0) {
> >> -		dev_dbg(cpu_dev, "OPP table is not ready, deferring probe\n");
> >> -		ret = -EPROBE_DEFER;
> >> -		goto out_free_opp;
> >> +		ret = handle->perf_ops->device_opps_add(handle, cpu_dev);
> >> +		if (ret) {
> >> +			dev_warn(cpu_dev, "failed to add opps to the device\n");
> >> +			goto out_free_cpumask;
> >> +		}
> >> +
> >> +		ret = dev_pm_opp_set_sharing_cpus(cpu_dev, opp_shared_cpus);
> >> +		if (ret) {
> >> +			dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n",
> >> +				__func__, ret);
> >> +			goto out_free_cpumask;
> >> +		}
> >> +
> > 
> > Why do we need to call above two after calling
> > dev_pm_opp_get_opp_count() ?
> 
> Sorry, I am not sure to understand your question here. If there are no opps for
> a device we want to add them to it

Earlier we used to call handle->perf_ops->device_opps_add() and
dev_pm_opp_set_sharing_cpus() before calling dev_pm_opp_get_opp_count(), why is
the order changed now ?

> otherwise no need as they would be duplicated.

I am not sure why they would be duplicated in your case. I though
device_opps_add() is responsible for dynamically adding the OPPs here.

> > And we don't check the return value of
> > the below call anymore, moreover we have to call it twice now.
> 
> This second get_opp_count is required such that we register em with the correct
> opp number after having added them. Without this the opp_count would not be correct.

What if the count is still 0 ? What about deferred probe we were doing earlier ?

-- 
viresh

Re: [PATCH v4 3/4] scmi-cpufreq: get opp_shared_cpus from opp-v2 for EM

[Nicola Mazzucato]

On 12/8/20 7:26 AM, Viresh Kumar wrote:
> On 08-12-20, 07:22, Nicola Mazzucato wrote:
>> On 12/8/20 5:50 AM, Viresh Kumar wrote:
>>> On 02-12-20, 17:23, Nicola Mazzucato wrote:
>>>>  	nr_opp = dev_pm_opp_get_opp_count(cpu_dev);
>>>>  	if (nr_opp <= 0) {
>>>> -		dev_dbg(cpu_dev, "OPP table is not ready, deferring probe\n");
>>>> -		ret = -EPROBE_DEFER;
>>>> -		goto out_free_opp;
>>>> +		ret = handle->perf_ops->device_opps_add(handle, cpu_dev);
>>>> +		if (ret) {
>>>> +			dev_warn(cpu_dev, "failed to add opps to the device\n");
>>>> +			goto out_free_cpumask;
>>>> +		}
>>>> +
>>>> +		ret = dev_pm_opp_set_sharing_cpus(cpu_dev, opp_shared_cpus);
>>>> +		if (ret) {
>>>> +			dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n",
>>>> +				__func__, ret);
>>>> +			goto out_free_cpumask;
>>>> +		}
>>>> +
>>>
>>> Why do we need to call above two after calling
>>> dev_pm_opp_get_opp_count() ?
>>
>> Sorry, I am not sure to understand your question here. If there are no opps for
>> a device we want to add them to it
> 
> Earlier we used to call handle->perf_ops->device_opps_add() and
> dev_pm_opp_set_sharing_cpus() before calling dev_pm_opp_get_opp_count(), why is
> the order changed now ?

True. The order has changed to take into account the fact that when we have
per-cpu + opp-shared, we don't need to add opps for devices which already have them.

> 
>> otherwise no need as they would be duplicated.
> 
> I am not sure why they would be duplicated in your case. I though
> device_opps_add() is responsible for dynamically adding the OPPs here.

In case of per-cpu + opp-shared, with the "previous order" we would try to add
opps to a device which already has them, in fact attempting to add duplicates.
Nothing wrong with it, but a lot of warnings are thrown.

> 
>>> And we don't check the return value of
>>> the below call anymore, moreover we have to call it twice now.
>>
>> This second get_opp_count is required such that we register em with the correct
>> opp number after having added them. Without this the opp_count would not be correct.
> 
> What if the count is still 0 ? What about deferred probe we were doing earlier ?

My assumption is to rely on the two above to fail if there was something wrong.
For the deferred probe, I am not sure it is still a useful case to have, but I
will let Sudeep have his view also on this.

> 

Thanks Viresh, hope it's a bit more clear now.
Nicola

Re: [PATCH v4 3/4] scmi-cpufreq: get opp_shared_cpus from opp-v2 for EM

[Viresh Kumar]

On 08-12-20, 10:58, Nicola Mazzucato wrote:
> 
> 
> On 12/8/20 7:26 AM, Viresh Kumar wrote:
> > On 08-12-20, 07:22, Nicola Mazzucato wrote:
> >> On 12/8/20 5:50 AM, Viresh Kumar wrote:
> >>> On 02-12-20, 17:23, Nicola Mazzucato wrote:
> >>>>  	nr_opp = dev_pm_opp_get_opp_count(cpu_dev);
> >>>>  	if (nr_opp <= 0) {
> >>>> -		dev_dbg(cpu_dev, "OPP table is not ready, deferring probe\n");
> >>>> -		ret = -EPROBE_DEFER;
> >>>> -		goto out_free_opp;
> >>>> +		ret = handle->perf_ops->device_opps_add(handle, cpu_dev);
> >>>> +		if (ret) {
> >>>> +			dev_warn(cpu_dev, "failed to add opps to the device\n");
> >>>> +			goto out_free_cpumask;
> >>>> +		}
> >>>> +
> >>>> +		ret = dev_pm_opp_set_sharing_cpus(cpu_dev, opp_shared_cpus);
> >>>> +		if (ret) {
> >>>> +			dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n",
> >>>> +				__func__, ret);
> >>>> +			goto out_free_cpumask;
> >>>> +		}
> >>>> +
> >>>
> >>> Why do we need to call above two after calling
> >>> dev_pm_opp_get_opp_count() ?
> >>
> >> Sorry, I am not sure to understand your question here. If there are no opps for
> >> a device we want to add them to it
> > 
> > Earlier we used to call handle->perf_ops->device_opps_add() and
> > dev_pm_opp_set_sharing_cpus() before calling dev_pm_opp_get_opp_count(), why is
> > the order changed now ?
> 
> True. The order has changed to take into account the fact that when we have
> per-cpu + opp-shared, we don't need to add opps for devices which already have them.

The opp-shared thing is mostly a dummy thing to get you some information here.
What else has changed here ? I still don't understand why the OPPs would get
added and so the duplicate OPPs messages. Does this already happen ?

-- 
viresh

Re: [PATCH v4 3/4] scmi-cpufreq: get opp_shared_cpus from opp-v2 for EM

[Sudeep Holla]

On Tue, Dec 08, 2020 at 12:56:11PM +0530, Viresh Kumar wrote:
> On 08-12-20, 07:22, Nicola Mazzucato wrote:
> > On 12/8/20 5:50 AM, Viresh Kumar wrote:
> > > On 02-12-20, 17:23, Nicola Mazzucato wrote:
> > >>  	nr_opp = dev_pm_opp_get_opp_count(cpu_dev);
> > >>  	if (nr_opp <= 0) {
> > >> -		dev_dbg(cpu_dev, "OPP table is not ready, deferring probe\n");
> > >> -		ret = -EPROBE_DEFER;
> > >> -		goto out_free_opp;
> > >> +		ret = handle->perf_ops->device_opps_add(handle, cpu_dev);
> > >> +		if (ret) {
> > >> +			dev_warn(cpu_dev, "failed to add opps to the device\n");
> > >> +			goto out_free_cpumask;
> > >> +		}
> > >> +
> > >> +		ret = dev_pm_opp_set_sharing_cpus(cpu_dev, opp_shared_cpus);
> > >> +		if (ret) {
> > >> +			dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n",
> > >> +				__func__, ret);
> > >> +			goto out_free_cpumask;
> > >> +		}
> > >> +
> > >
> > > Why do we need to call above two after calling
> > > dev_pm_opp_get_opp_count() ?
> >
> > Sorry, I am not sure to understand your question here. If there are no opps for
> > a device we want to add them to it
>
> Earlier we used to call handle->perf_ops->device_opps_add() and
> dev_pm_opp_set_sharing_cpus() before calling dev_pm_opp_get_opp_count(), why is
> the order changed now ?
>
> 
> I am not sure why they would be duplicated in your case. I though
> device_opps_add() is responsible for dynamically adding the OPPs here.
> 

It is because of per-CPU vs per domain drama here. Imagine a system with
4 CPUs which the firmware puts in individual domains while they all are
in the same perf domain and hence OPP is marked shared in DT.

Since this probe gets called for all the cpus, we need to skip adding
OPPs for the last 3(add only for 1st one and mark others as shared).
If we attempt to add OPPs on second cpu probe, it *will* shout as duplicate
OPP as we would have already marked it as shared table with the first cpu.
Am I missing anything ? I suggested this as Nicola saw OPP duplicate
warnings when he was hacking up this patch.

> > otherwise no need as they would be duplicated.
> > > And we don't check the return value of
> > > the below call anymore, moreover we have to call it twice now.

Yes, that looks wrong, we need to add the check for non zero values, but ....

> > 
> > This second get_opp_count is required such that we register em with the correct
> > opp number after having added them. Without this the opp_count would not be correct.
>

... I have a question here. Why do you need to call

em_dev_register_perf_domain(cpu_dev, nr_opp, &em_cb, opp_shared_cpus..)

on each CPU ? Why can't that be done once for unique opp_shared_cpus ?

The whole drama of per-CPU vs perf domain is to have energy model and
if feeding it opp_shared_cpus once is not sufficient, then something is
wrong or simply duplicated or just not necessary IMO.

> What if the count is still 0 ? What about deferred probe we were doing earlier ?

OK, you made me think with that question. I think the check was original
added for deferred probe but then scmi core was changed to add the cpufreq
device only after everything needed is ready. So the condition must never
occur now.

-- 
Regards,
Sudeep

Re: [PATCH v4 3/4] scmi-cpufreq: get opp_shared_cpus from opp-v2 for EM

[Sudeep Holla]

On Tue, Dec 08, 2020 at 04:31:48PM +0530, Viresh Kumar wrote:
> On 08-12-20, 10:58, Nicola Mazzucato wrote:
> > 
> > 
> > On 12/8/20 7:26 AM, Viresh Kumar wrote:
> > > On 08-12-20, 07:22, Nicola Mazzucato wrote:
> > >> On 12/8/20 5:50 AM, Viresh Kumar wrote:
> > >>> On 02-12-20, 17:23, Nicola Mazzucato wrote:
> > >>>>  	nr_opp = dev_pm_opp_get_opp_count(cpu_dev);
> > >>>>  	if (nr_opp <= 0) {
> > >>>> -		dev_dbg(cpu_dev, "OPP table is not ready, deferring probe\n");
> > >>>> -		ret = -EPROBE_DEFER;
> > >>>> -		goto out_free_opp;
> > >>>> +		ret = handle->perf_ops->device_opps_add(handle, cpu_dev);
> > >>>> +		if (ret) {
> > >>>> +			dev_warn(cpu_dev, "failed to add opps to the device\n");
> > >>>> +			goto out_free_cpumask;
> > >>>> +		}
> > >>>> +
> > >>>> +		ret = dev_pm_opp_set_sharing_cpus(cpu_dev, opp_shared_cpus);
> > >>>> +		if (ret) {
> > >>>> +			dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n",
> > >>>> +				__func__, ret);
> > >>>> +			goto out_free_cpumask;
> > >>>> +		}
> > >>>> +
> > >>>
> > >>> Why do we need to call above two after calling
> > >>> dev_pm_opp_get_opp_count() ?
> > >>
> > >> Sorry, I am not sure to understand your question here. If there are no opps for
> > >> a device we want to add them to it
> > > 
> > > Earlier we used to call handle->perf_ops->device_opps_add() and
> > > dev_pm_opp_set_sharing_cpus() before calling dev_pm_opp_get_opp_count(), why is
> > > the order changed now ?
> > 
> > True. The order has changed to take into account the fact that when we have
> > per-cpu + opp-shared, we don't need to add opps for devices which already have them.
> 
> The opp-shared thing is mostly a dummy thing to get you some information here.
> What else has changed here ? I still don't understand why the OPPs would get
> added and so the duplicate OPPs messages. Does this already happen ?
> 

Yes, details in my earlier response.

-- 
Regards,
Sudeep

Re: [PATCH v4 3/4] scmi-cpufreq: get opp_shared_cpus from opp-v2 for EM

[Lukasz Luba]

On 12/8/20 11:20 AM, Sudeep Holla wrote:
> On Tue, Dec 08, 2020 at 12:56:11PM +0530, Viresh Kumar wrote:
>> On 08-12-20, 07:22, Nicola Mazzucato wrote:
>>> On 12/8/20 5:50 AM, Viresh Kumar wrote:
>>>> On 02-12-20, 17:23, Nicola Mazzucato wrote:
>>>>>   	nr_opp = dev_pm_opp_get_opp_count(cpu_dev);
>>>>>   	if (nr_opp <= 0) {
>>>>> -		dev_dbg(cpu_dev, "OPP table is not ready, deferring probe\n");
>>>>> -		ret = -EPROBE_DEFER;
>>>>> -		goto out_free_opp;
>>>>> +		ret = handle->perf_ops->device_opps_add(handle, cpu_dev);
>>>>> +		if (ret) {
>>>>> +			dev_warn(cpu_dev, "failed to add opps to the device\n");
>>>>> +			goto out_free_cpumask;
>>>>> +		}
>>>>> +
>>>>> +		ret = dev_pm_opp_set_sharing_cpus(cpu_dev, opp_shared_cpus);
>>>>> +		if (ret) {
>>>>> +			dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n",
>>>>> +				__func__, ret);
>>>>> +			goto out_free_cpumask;
>>>>> +		}
>>>>> +
>>>>
>>>> Why do we need to call above two after calling
>>>> dev_pm_opp_get_opp_count() ?
>>>
>>> Sorry, I am not sure to understand your question here. If there are no opps for
>>> a device we want to add them to it
>>
>> Earlier we used to call handle->perf_ops->device_opps_add() and
>> dev_pm_opp_set_sharing_cpus() before calling dev_pm_opp_get_opp_count(), why is
>> the order changed now ?
>>
>>
>> I am not sure why they would be duplicated in your case. I though
>> device_opps_add() is responsible for dynamically adding the OPPs here.
>>
> 
> It is because of per-CPU vs per domain drama here. Imagine a system with
> 4 CPUs which the firmware puts in individual domains while they all are
> in the same perf domain and hence OPP is marked shared in DT.
> 
> Since this probe gets called for all the cpus, we need to skip adding
> OPPs for the last 3(add only for 1st one and mark others as shared).
> If we attempt to add OPPs on second cpu probe, it *will* shout as duplicate
> OPP as we would have already marked it as shared table with the first cpu.
> Am I missing anything ? I suggested this as Nicola saw OPP duplicate
> warnings when he was hacking up this patch.
> 
>>> otherwise no need as they would be duplicated.
>>>> And we don't check the return value of
>>>> the below call anymore, moreover we have to call it twice now.
> 
> Yes, that looks wrong, we need to add the check for non zero values, but ....
> 
>>>
>>> This second get_opp_count is required such that we register em with the correct
>>> opp number after having added them. Without this the opp_count would not be correct.
>>
> 
> ... I have a question here. Why do you need to call
> 
> em_dev_register_perf_domain(cpu_dev, nr_opp, &em_cb, opp_shared_cpus..)
> 
> on each CPU ? Why can't that be done once for unique opp_shared_cpus ?

It just have to be called once, for one CPU from the mask. Otherwise for
the next CPUs you should see error:
"EM: exists for CPU%d"
It can happen that this print is not seen when the get_cpu_device(cpu)
failed, but that would lead to investigation why CPU devices are not
there yet.

Nicola: have you seen that print?

Re: [PATCH v4 3/4] scmi-cpufreq: get opp_shared_cpus from opp-v2 for EM

[Sudeep Holla]

On Tue, Dec 08, 2020 at 11:34:36AM +0000, Lukasz Luba wrote:
> 
> 
> On 12/8/20 11:20 AM, Sudeep Holla wrote:
> > On Tue, Dec 08, 2020 at 12:56:11PM +0530, Viresh Kumar wrote:
> > > On 08-12-20, 07:22, Nicola Mazzucato wrote:
> > > > On 12/8/20 5:50 AM, Viresh Kumar wrote:
> > > > > On 02-12-20, 17:23, Nicola Mazzucato wrote:
> > > > > >   	nr_opp = dev_pm_opp_get_opp_count(cpu_dev);
> > > > > >   	if (nr_opp <= 0) {
> > > > > > -		dev_dbg(cpu_dev, "OPP table is not ready, deferring probe\n");
> > > > > > -		ret = -EPROBE_DEFER;
> > > > > > -		goto out_free_opp;
> > > > > > +		ret = handle->perf_ops->device_opps_add(handle, cpu_dev);
> > > > > > +		if (ret) {
> > > > > > +			dev_warn(cpu_dev, "failed to add opps to the device\n");
> > > > > > +			goto out_free_cpumask;
> > > > > > +		}
> > > > > > +
> > > > > > +		ret = dev_pm_opp_set_sharing_cpus(cpu_dev, opp_shared_cpus);
> > > > > > +		if (ret) {
> > > > > > +			dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n",
> > > > > > +				__func__, ret);
> > > > > > +			goto out_free_cpumask;
> > > > > > +		}
> > > > > > +
> > > > > 
> > > > > Why do we need to call above two after calling
> > > > > dev_pm_opp_get_opp_count() ?
> > > > 
> > > > Sorry, I am not sure to understand your question here. If there are no opps for
> > > > a device we want to add them to it
> > > 
> > > Earlier we used to call handle->perf_ops->device_opps_add() and
> > > dev_pm_opp_set_sharing_cpus() before calling dev_pm_opp_get_opp_count(), why is
> > > the order changed now ?
> > > 
> > > 
> > > I am not sure why they would be duplicated in your case. I though
> > > device_opps_add() is responsible for dynamically adding the OPPs here.
> > > 
> > 
> > It is because of per-CPU vs per domain drama here. Imagine a system with
> > 4 CPUs which the firmware puts in individual domains while they all are
> > in the same perf domain and hence OPP is marked shared in DT.
> > 
> > Since this probe gets called for all the cpus, we need to skip adding
> > OPPs for the last 3(add only for 1st one and mark others as shared).
> > If we attempt to add OPPs on second cpu probe, it *will* shout as duplicate
> > OPP as we would have already marked it as shared table with the first cpu.
> > Am I missing anything ? I suggested this as Nicola saw OPP duplicate
> > warnings when he was hacking up this patch.
> > 
> > > > otherwise no need as they would be duplicated.
> > > > > And we don't check the return value of
> > > > > the below call anymore, moreover we have to call it twice now.
> > 
> > Yes, that looks wrong, we need to add the check for non zero values, but ....
> > 
> > > > 
> > > > This second get_opp_count is required such that we register em with the correct
> > > > opp number after having added them. Without this the opp_count would not be correct.
> > > 
> > 
> > ... I have a question here. Why do you need to call
> > 
> > em_dev_register_perf_domain(cpu_dev, nr_opp, &em_cb, opp_shared_cpus..)
> > 
> > on each CPU ? Why can't that be done once for unique opp_shared_cpus ?
> 
> It just have to be called once, for one CPU from the mask. Otherwise for
> the next CPUs you should see error:
> "EM: exists for CPU%d"

OK cool, at least it is designed and expected to be used like I thought.
Ah, I might have seen those, but never thought it was error message 😄 

> It can happen that this print is not seen when the get_cpu_device(cpu)
> failed, but that would lead to investigation why CPU devices are not
> there yet.
>
> Nicola: have you seen that print?
>

I assume you must see that and you need to pull this inside if condition
to do this once for each performance domain.

--
Regards,
Sudeep

Re: [PATCH v4 3/4] scmi-cpufreq: get opp_shared_cpus from opp-v2 for EM

[Nicola Mazzucato]

Hi All, thanks for your feedback, please see below

On 12/8/20 12:22 PM, Sudeep Holla wrote:
> On Tue, Dec 08, 2020 at 11:34:36AM +0000, Lukasz Luba wrote:
>>
>>
>> On 12/8/20 11:20 AM, Sudeep Holla wrote:
>>> On Tue, Dec 08, 2020 at 12:56:11PM +0530, Viresh Kumar wrote:
>>>> On 08-12-20, 07:22, Nicola Mazzucato wrote:
>>>>> On 12/8/20 5:50 AM, Viresh Kumar wrote:
>>>>>> On 02-12-20, 17:23, Nicola Mazzucato wrote:
>>>>>>>   	nr_opp = dev_pm_opp_get_opp_count(cpu_dev);
>>>>>>>   	if (nr_opp <= 0) {
>>>>>>> -		dev_dbg(cpu_dev, "OPP table is not ready, deferring probe\n");
>>>>>>> -		ret = -EPROBE_DEFER;
>>>>>>> -		goto out_free_opp;
>>>>>>> +		ret = handle->perf_ops->device_opps_add(handle, cpu_dev);
>>>>>>> +		if (ret) {
>>>>>>> +			dev_warn(cpu_dev, "failed to add opps to the device\n");
>>>>>>> +			goto out_free_cpumask;
>>>>>>> +		}
>>>>>>> +
>>>>>>> +		ret = dev_pm_opp_set_sharing_cpus(cpu_dev, opp_shared_cpus);
>>>>>>> +		if (ret) {
>>>>>>> +			dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n",
>>>>>>> +				__func__, ret);
>>>>>>> +			goto out_free_cpumask;
>>>>>>> +		}
>>>>>>> +
>>>>>>
>>>>>> Why do we need to call above two after calling
>>>>>> dev_pm_opp_get_opp_count() ?
>>>>>
>>>>> Sorry, I am not sure to understand your question here. If there are no opps for
>>>>> a device we want to add them to it
>>>>
>>>> Earlier we used to call handle->perf_ops->device_opps_add() and
>>>> dev_pm_opp_set_sharing_cpus() before calling dev_pm_opp_get_opp_count(), why is
>>>> the order changed now ?
>>>>
>>>>
>>>> I am not sure why they would be duplicated in your case. I though
>>>> device_opps_add() is responsible for dynamically adding the OPPs here.
>>>>
>>>
>>> It is because of per-CPU vs per domain drama here. Imagine a system with
>>> 4 CPUs which the firmware puts in individual domains while they all are
>>> in the same perf domain and hence OPP is marked shared in DT.
>>>
>>> Since this probe gets called for all the cpus, we need to skip adding
>>> OPPs for the last 3(add only for 1st one and mark others as shared).
>>> If we attempt to add OPPs on second cpu probe, it *will* shout as duplicate
>>> OPP as we would have already marked it as shared table with the first cpu.
>>> Am I missing anything ? I suggested this as Nicola saw OPP duplicate
>>> warnings when he was hacking up this patch.
>>>
>>>>> otherwise no need as they would be duplicated.
>>>>>> And we don't check the return value of
>>>>>> the below call anymore, moreover we have to call it twice now.
>>>
>>> Yes, that looks wrong, we need to add the check for non zero values, but ....

will add the check, thanks

>>>
>>>>>
>>>>> This second get_opp_count is required such that we register em with the correct
>>>>> opp number after having added them. Without this the opp_count would not be correct.
>>>>
>>>
>>> ... I have a question here. Why do you need to call
>>>
>>> em_dev_register_perf_domain(cpu_dev, nr_opp, &em_cb, opp_shared_cpus..)
>>>
>>> on each CPU ? Why can't that be done once for unique opp_shared_cpus ?

I left it untouched to reduce changes, but I see your point.

>>
>> It just have to be called once, for one CPU from the mask. Otherwise for
>> the next CPUs you should see error:
>> "EM: exists for CPU%d"
> 
> OK cool, at least it is designed and expected to be used like I thought.
> Ah, I might have seen those, but never thought it was error message 😄 
> 
>> It can happen that this print is not seen when the get_cpu_device(cpu)
>> failed, but that would lead to investigation why CPU devices are not
>> there yet.
>>
>> Nicola: have you seen that print?
>>
> 
> I assume you must see that and you need to pull this inside if condition
> to do this once for each performance domain.

I don't see that error, and that's also why I left it there. If there's already
and em_pd for a device, EM just returns with an error that we don't check.

I agree that it makes more sense to register em for opp_shared_cpus.

> 
> --
> Regards,
> Sudeep
> 

Re: [PATCH v4 3/4] scmi-cpufreq: get opp_shared_cpus from opp-v2 for EM

[Viresh Kumar]

On 08-12-20, 11:20, Sudeep Holla wrote:
> It is because of per-CPU vs per domain drama here. Imagine a system with
> 4 CPUs which the firmware puts in individual domains while they all are
> in the same perf domain and hence OPP is marked shared in DT.
> 
> Since this probe gets called for all the cpus, we need to skip adding
> OPPs for the last 3(add only for 1st one and mark others as shared).

Okay and this wasn't happening before this series because the firmware
was only returning the current CPU from scmi_get_sharing_cpus() ?

Is this driver also used for the cases where we have multiple CPUs in
a policy ? Otherwise we won't be required to call
dev_pm_opp_set_sharing_cpus().

So I assume that we want to support both the cases here ?

> If we attempt to add OPPs on second cpu probe, it *will* shout as duplicate
> OPP as we would have already marked it as shared table with the first cpu.
> Am I missing anything ? I suggested this as Nicola saw OPP duplicate
> warnings when he was hacking up this patch.

The common stuff (for all the CPUs) is better moved to probe() in this
case, instead of the ->init() callback. Otherwise it will always be
messy. You can initialize the OPP and cpufreq tables in probe()
itself, save the pointer somewhere and then just use it here in
->init().

Also do EM registration from there.

> > > otherwise no need as they would be duplicated.
> > > > And we don't check the return value of
> > > > the below call anymore, moreover we have to call it twice now.
> 
> Yes, that looks wrong, we need to add the check for non zero values, but ....
> 
> > > 
> > > This second get_opp_count is required such that we register em with the correct
> > > opp number after having added them. Without this the opp_count would not be correct.
> >
> 
> ... I have a question here. Why do you need to call
> 
> em_dev_register_perf_domain(cpu_dev, nr_opp, &em_cb, opp_shared_cpus..)
> 
> on each CPU ? Why can't that be done once for unique opp_shared_cpus ?
> 
> The whole drama of per-CPU vs perf domain is to have energy model and
> if feeding it opp_shared_cpus once is not sufficient, then something is
> wrong or simply duplicated or just not necessary IMO.
> 
> > What if the count is still 0 ? What about deferred probe we were doing earlier ?
> 
> OK, you made me think with that question. I think the check was original
> added for deferred probe but then scmi core was changed to add the cpufreq
> device only after everything needed is ready. So the condition must never
> occur now.

The deferred probe shall be handled in a different patch in that case.

Nicola, please break the patch into multiple patches, with one patch
dealing only with one task.

-- 
viresh

Re: [PATCH v4 3/4] scmi-cpufreq: get opp_shared_cpus from opp-v2 for EM

[Nicola Mazzucato]

Hi both,

thanks for looking into this.

On 12/9/20 5:45 AM, Viresh Kumar wrote:
> On 08-12-20, 11:20, Sudeep Holla wrote:
>> It is because of per-CPU vs per domain drama here. Imagine a system with
>> 4 CPUs which the firmware puts in individual domains while they all are
>> in the same perf domain and hence OPP is marked shared in DT.
>>
>> Since this probe gets called for all the cpus, we need to skip adding
>> OPPs for the last 3(add only for 1st one and mark others as shared).
> 
> Okay and this wasn't happening before this series because the firmware
> was only returning the current CPU from scmi_get_sharing_cpus() ?

yes

> 
> Is this driver also used for the cases where we have multiple CPUs in
> a policy ? Otherwise we won't be required to call
> dev_pm_opp_set_sharing_cpus().
> 
> So I assume that we want to support both the cases here ?

yes, we want to support existing platforms (n cpus in a policy) + the per-cpu case.

> 
>> If we attempt to add OPPs on second cpu probe, it *will* shout as duplicate
>> OPP as we would have already marked it as shared table with the first cpu.
>> Am I missing anything ? I suggested this as Nicola saw OPP duplicate
>> warnings when he was hacking up this patch.
> 
> The common stuff (for all the CPUs) is better moved to probe() in this
> case, instead of the ->init() callback. Otherwise it will always be
> messy. You can initialize the OPP and cpufreq tables in probe()
> itself, save the pointer somewhere and then just use it here in
> ->init().
> 
> Also do EM registration from there.
>

ok, will rework

>>>> otherwise no need as they would be duplicated.
>>>>> And we don't check the return value of
>>>>> the below call anymore, moreover we have to call it twice now.
>>
>> Yes, that looks wrong, we need to add the check for non zero values, but ....
>>
>>>>
>>>> This second get_opp_count is required such that we register em with the correct
>>>> opp number after having added them. Without this the opp_count would not be correct.
>>>
>>
>> ... I have a question here. Why do you need to call
>>
>> em_dev_register_perf_domain(cpu_dev, nr_opp, &em_cb, opp_shared_cpus..)
>>
>> on each CPU ? Why can't that be done once for unique opp_shared_cpus ?
>>
>> The whole drama of per-CPU vs perf domain is to have energy model and
>> if feeding it opp_shared_cpus once is not sufficient, then something is
>> wrong or simply duplicated or just not necessary IMO.
>>
>>> What if the count is still 0 ? What about deferred probe we were doing earlier ?
>>
>> OK, you made me think with that question. I think the check was original
>> added for deferred probe but then scmi core was changed to add the cpufreq
>> device only after everything needed is ready. So the condition must never
>> occur now.
> 
> The deferred probe shall be handled in a different patch in that case.
> 
> Nicola, please break the patch into multiple patches, with one patch
> dealing only with one task.

Sure, I had the doubt and thanks for confirming. will do, thanks

> 

Cheers,
Nicola

Re: [PATCH v4 3/4] scmi-cpufreq: get opp_shared_cpus from opp-v2 for EM

[Sudeep Holla]

On Wed, Dec 09, 2020 at 11:15:02AM +0530, Viresh Kumar wrote:
> On 08-12-20, 11:20, Sudeep Holla wrote:
> > It is because of per-CPU vs per domain drama here. Imagine a system with
> > 4 CPUs which the firmware puts in individual domains while they all are
> > in the same perf domain and hence OPP is marked shared in DT.
> >
> > Since this probe gets called for all the cpus, we need to skip adding
> > OPPs for the last 3(add only for 1st one and mark others as shared).
>
> Okay and this wasn't happening before this series because the firmware
> was only returning the current CPU from scmi_get_sharing_cpus() ?
>
> Is this driver also used for the cases where we have multiple CPUs in
> a policy ? Otherwise we won't be required to call
> dev_pm_opp_set_sharing_cpus().
>
> So I assume that we want to support both the cases here ?
>

Yes indeed, completely depends on what granularity firmware provides the
performance control. It could be individual CPUs, could be pair of CPUs
(or all the threads in the core) or subset of CPUs in the performance
domain. The subset could be full set.

> > If we attempt to add OPPs on second cpu probe, it *will* shout as duplicate
> > OPP as we would have already marked it as shared table with the first cpu.
> > Am I missing anything ? I suggested this as Nicola saw OPP duplicate
> > warnings when he was hacking up this patch.
>
> The common stuff (for all the CPUs) is better moved to probe() in this
> case, instead of the ->init() callback. Otherwise it will always be
> messy. You can initialize the OPP and cpufreq tables in probe()
> itself, save the pointer somewhere and then just use it here in
> ->init().
>
> Also do EM registration from there.
>

Makes sense.

--
Regards,
Sudeep