[PATCH 0/2] drivers: remove incorrect usage/dependency on topology_physical_package_id

[PATCH 0/2] drivers: remove incorrect usage/dependency on topology_physical_package_id

[Sudeep Holla]

Hi all,

There has been ongoing attempt to add ACPI topology support on ARM/ARM64
platform using PPTT. However it was discovered that the physical package
id is mapped to the so called clusters on ARM platforms and they don't
map to the physical socket as in other architectures.

In order to add the correct support for the well defined physical sockets,
we need to find remove the incorrect usage or any dependency on
topology_physical_package_id which currently maps to the clusters.

There's still one pending user of topology_physical_package_id, which is
arm_big_little driver which is used by bL switcher and by TC2 platform.
I would like to get some feedback on how to remove that dependency ?
Can we hard-code the cluster values ? Any other suggestions ?

Sudeep Holla (2):
  drivers: psci: remove cluster terminology and dependency on
    physical_package_id
  cpufreq: scpi: remove arm_big_little dependency

 drivers/cpufreq/scpi-cpufreq.c  | 193 ++++++++++++++++++++++++++++++++++++----
 drivers/firmware/psci_checker.c |  46 +++++-----
 2 files changed, 200 insertions(+), 39 deletions(-)

--
2.7.4

[PATCH 1/2] drivers: psci: remove cluster terminology and dependency on physical_package_id

[Sudeep Holla]

Since the definition of the term "cluster" is not well defined in the
architecture, we should avoid using it. Also the physical package id
is currently mapped to so called "clusters" in ARM/ARM64 platforms which
is already argumentative.

This patch removes the dependency on physical_package_id from the topology
in this PSCI checker. Also it replaces all the occurences of clusters to
cpu_groups which is derived from core_sibling_mask and may not directly
map to physical "cluster".

Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/psci_checker.c | 46 ++++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 24 deletions(-)

diff --git a/drivers/firmware/psci_checker.c b/drivers/firmware/psci_checker.c
index f3f4f810e5df..bb1c068bff19 100644
--- a/drivers/firmware/psci_checker.c
+++ b/drivers/firmware/psci_checker.c
@@ -77,8 +77,8 @@ static int psci_ops_check(void)
 	return 0;
 }
 
-static int find_clusters(const struct cpumask *cpus,
-			 const struct cpumask **clusters)
+static int find_cpu_groups(const struct cpumask *cpus,
+			   const struct cpumask **cpu_groups)
 {
 	unsigned int nb = 0;
 	cpumask_var_t tmp;
@@ -88,11 +88,11 @@ static int find_clusters(const struct cpumask *cpus,
 	cpumask_copy(tmp, cpus);
 
 	while (!cpumask_empty(tmp)) {
-		const struct cpumask *cluster =
+		const struct cpumask *cpu_group =
 			topology_core_cpumask(cpumask_any(tmp));
 
-		clusters[nb++] = cluster;
-		cpumask_andnot(tmp, tmp, cluster);
+		cpu_groups[nb++] = cpu_group;
+		cpumask_andnot(tmp, tmp, cpu_group);
 	}
 
 	free_cpumask_var(tmp);
@@ -170,24 +170,24 @@ static int hotplug_tests(void)
 {
 	int err;
 	cpumask_var_t offlined_cpus;
-	int i, nb_cluster;
-	const struct cpumask **clusters;
+	int i, nb_cpu_group;
+	const struct cpumask **cpu_groups;
 	char *page_buf;
 
 	err = -ENOMEM;
 	if (!alloc_cpumask_var(&offlined_cpus, GFP_KERNEL))
 		return err;
-	/* We may have up to nb_available_cpus clusters. */
-	clusters = kmalloc_array(nb_available_cpus, sizeof(*clusters),
-				 GFP_KERNEL);
-	if (!clusters)
+	/* We may have up to nb_available_cpus cpu_groups. */
+	cpu_groups = kmalloc_array(nb_available_cpus, sizeof(*cpu_groups),
+				   GFP_KERNEL);
+	if (!cpu_groups)
 		goto out_free_cpus;
 	page_buf = (char *)__get_free_page(GFP_KERNEL);
 	if (!page_buf)
-		goto out_free_clusters;
+		goto out_free_cpu_groups;
 
 	err = 0;
-	nb_cluster = find_clusters(cpu_online_mask, clusters);
+	nb_cpu_group = find_cpu_groups(cpu_online_mask, cpu_groups);
 
 	/*
 	 * Of course the last CPU cannot be powered down and cpu_down() should
@@ -197,24 +197,22 @@ static int hotplug_tests(void)
 	err += down_and_up_cpus(cpu_online_mask, offlined_cpus);
 
 	/*
-	 * Take down CPUs by cluster this time. When the last CPU is turned
-	 * off, the cluster itself should shut down.
+	 * Take down CPUs by cpu group this time. When the last CPU is turned
+	 * off, the cpu group itself should shut down.
 	 */
-	for (i = 0; i < nb_cluster; ++i) {
-		int cluster_id =
-			topology_physical_package_id(cpumask_any(clusters[i]));
+	for (i = 0; i < nb_cpu_group; ++i) {
 		ssize_t len = cpumap_print_to_pagebuf(true, page_buf,
-						      clusters[i]);
+						      cpu_groups[i]);
 		/* Remove trailing newline. */
 		page_buf[len - 1] = '\0';
-		pr_info("Trying to turn off and on again cluster %d "
-			"(CPUs %s)\n", cluster_id, page_buf);
-		err += down_and_up_cpus(clusters[i], offlined_cpus);
+		pr_info("Trying to turn off and on again group %d (CPUs %s)\n",
+			i, page_buf);
+		err += down_and_up_cpus(cpu_groups[i], offlined_cpus);
 	}
 
 	free_page((unsigned long)page_buf);
-out_free_clusters:
-	kfree(clusters);
+out_free_cpu_groups:
+	kfree(cpu_groups);
 out_free_cpus:
 	free_cpumask_var(offlined_cpus);
 	return err;
-- 
2.7.4

[PATCH 1/2] drivers: psci: remove cluster terminology and dependency on physical_package_id

[Lorenzo Pieralisi]

On Tue, Jan 09, 2018 at 04:49:27PM +0000, Sudeep Holla wrote:
> Since the definition of the term "cluster" is not well defined in the
> architecture, we should avoid using it. Also the physical package id
> is currently mapped to so called "clusters" in ARM/ARM64 platforms which
> is already argumentative.

I think we should describe why the PSCI checker uses the physical
package id (ie because it is likely that power domains map to "clusters"
- so physical package id *current* boundaries, it is trying to test
"cluster" idle states) but I can easily rework the log before sending it
upstream.

I will send it upstream for the next cycle along with patch (2), or
if you prefer to send it yourself:

Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

Thanks for putting it together,
Lorenzo

> This patch removes the dependency on physical_package_id from the topology
> in this PSCI checker. Also it replaces all the occurences of clusters to
> cpu_groups which is derived from core_sibling_mask and may not directly
> map to physical "cluster".
> 
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/psci_checker.c | 46 ++++++++++++++++++++---------------------
>  1 file changed, 22 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/firmware/psci_checker.c b/drivers/firmware/psci_checker.c
> index f3f4f810e5df..bb1c068bff19 100644
> --- a/drivers/firmware/psci_checker.c
> +++ b/drivers/firmware/psci_checker.c
> @@ -77,8 +77,8 @@ static int psci_ops_check(void)
>  	return 0;
>  }
>  
> -static int find_clusters(const struct cpumask *cpus,
> -			 const struct cpumask **clusters)
> +static int find_cpu_groups(const struct cpumask *cpus,
> +			   const struct cpumask **cpu_groups)
>  {
>  	unsigned int nb = 0;
>  	cpumask_var_t tmp;
> @@ -88,11 +88,11 @@ static int find_clusters(const struct cpumask *cpus,
>  	cpumask_copy(tmp, cpus);
>  
>  	while (!cpumask_empty(tmp)) {
> -		const struct cpumask *cluster =
> +		const struct cpumask *cpu_group =
>  			topology_core_cpumask(cpumask_any(tmp));
>  
> -		clusters[nb++] = cluster;
> -		cpumask_andnot(tmp, tmp, cluster);
> +		cpu_groups[nb++] = cpu_group;
> +		cpumask_andnot(tmp, tmp, cpu_group);
>  	}
>  
>  	free_cpumask_var(tmp);
> @@ -170,24 +170,24 @@ static int hotplug_tests(void)
>  {
>  	int err;
>  	cpumask_var_t offlined_cpus;
> -	int i, nb_cluster;
> -	const struct cpumask **clusters;
> +	int i, nb_cpu_group;
> +	const struct cpumask **cpu_groups;
>  	char *page_buf;
>  
>  	err = -ENOMEM;
>  	if (!alloc_cpumask_var(&offlined_cpus, GFP_KERNEL))
>  		return err;
> -	/* We may have up to nb_available_cpus clusters. */
> -	clusters = kmalloc_array(nb_available_cpus, sizeof(*clusters),
> -				 GFP_KERNEL);
> -	if (!clusters)
> +	/* We may have up to nb_available_cpus cpu_groups. */
> +	cpu_groups = kmalloc_array(nb_available_cpus, sizeof(*cpu_groups),
> +				   GFP_KERNEL);
> +	if (!cpu_groups)
>  		goto out_free_cpus;
>  	page_buf = (char *)__get_free_page(GFP_KERNEL);
>  	if (!page_buf)
> -		goto out_free_clusters;
> +		goto out_free_cpu_groups;
>  
>  	err = 0;
> -	nb_cluster = find_clusters(cpu_online_mask, clusters);
> +	nb_cpu_group = find_cpu_groups(cpu_online_mask, cpu_groups);
>  
>  	/*
>  	 * Of course the last CPU cannot be powered down and cpu_down() should
> @@ -197,24 +197,22 @@ static int hotplug_tests(void)
>  	err += down_and_up_cpus(cpu_online_mask, offlined_cpus);
>  
>  	/*
> -	 * Take down CPUs by cluster this time. When the last CPU is turned
> -	 * off, the cluster itself should shut down.
> +	 * Take down CPUs by cpu group this time. When the last CPU is turned
> +	 * off, the cpu group itself should shut down.
>  	 */
> -	for (i = 0; i < nb_cluster; ++i) {
> -		int cluster_id =
> -			topology_physical_package_id(cpumask_any(clusters[i]));
> +	for (i = 0; i < nb_cpu_group; ++i) {
>  		ssize_t len = cpumap_print_to_pagebuf(true, page_buf,
> -						      clusters[i]);
> +						      cpu_groups[i]);
>  		/* Remove trailing newline. */
>  		page_buf[len - 1] = '\0';
> -		pr_info("Trying to turn off and on again cluster %d "
> -			"(CPUs %s)\n", cluster_id, page_buf);
> -		err += down_and_up_cpus(clusters[i], offlined_cpus);
> +		pr_info("Trying to turn off and on again group %d (CPUs %s)\n",
> +			i, page_buf);
> +		err += down_and_up_cpus(cpu_groups[i], offlined_cpus);
>  	}
>  
>  	free_page((unsigned long)page_buf);
> -out_free_clusters:
> -	kfree(clusters);
> +out_free_cpu_groups:
> +	kfree(cpu_groups);
>  out_free_cpus:
>  	free_cpumask_var(offlined_cpus);
>  	return err;
> -- 
> 2.7.4
> 

[PATCH 1/2] drivers: psci: remove cluster terminology and dependency on physical_package_id

[Sudeep Holla]

Hi Lorenzo,

On 09/01/18 17:34, Lorenzo Pieralisi wrote:
> On Tue, Jan 09, 2018 at 04:49:27PM +0000, Sudeep Holla wrote:
>> Since the definition of the term "cluster" is not well defined in the
>> architecture, we should avoid using it. Also the physical package id
>> is currently mapped to so called "clusters" in ARM/ARM64 platforms which
>> is already argumentative.
> 
> I think we should describe why the PSCI checker uses the physical
> package id (ie because it is likely that power domains map to "clusters"
> - so physical package id *current* boundaries, it is trying to test
> "cluster" idle states) but I can easily rework the log before sending it
> upstream.
> 

I agree. If I need to resend 2/2 based on review I will update the log
accordingly with acked-by tag. Otherwise, you can pick it up and update
the log.

> I will send it upstream for the next cycle along with patch (2), or
> if you prefer to send it yourself:
> 
> Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> 
> Thanks for putting it together,
> Lorenzo
> 
>> This patch removes the dependency on physical_package_id from the topology
>> in this PSCI checker. Also it replaces all the occurences of clusters to
>> cpu_groups which is derived from core_sibling_mask and may not directly
>> map to physical "cluster".
>>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>  drivers/firmware/psci_checker.c | 46 ++++++++++++++++++++---------------------
>>  1 file changed, 22 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/firmware/psci_checker.c b/drivers/firmware/psci_checker.c
>> index f3f4f810e5df..bb1c068bff19 100644
>> --- a/drivers/firmware/psci_checker.c
>> +++ b/drivers/firmware/psci_checker.c
>> @@ -77,8 +77,8 @@ static int psci_ops_check(void)
>>  	return 0;
>>  }
>>  
>> -static int find_clusters(const struct cpumask *cpus,
>> -			 const struct cpumask **clusters)
>> +static int find_cpu_groups(const struct cpumask *cpus,
>> +			   const struct cpumask **cpu_groups)
>>  {
>>  	unsigned int nb = 0;
>>  	cpumask_var_t tmp;
>> @@ -88,11 +88,11 @@ static int find_clusters(const struct cpumask *cpus,
>>  	cpumask_copy(tmp, cpus);
>>  
>>  	while (!cpumask_empty(tmp)) {
>> -		const struct cpumask *cluster =
>> +		const struct cpumask *cpu_group =
>>  			topology_core_cpumask(cpumask_any(tmp));
>>  
>> -		clusters[nb++] = cluster;
>> -		cpumask_andnot(tmp, tmp, cluster);
>> +		cpu_groups[nb++] = cpu_group;
>> +		cpumask_andnot(tmp, tmp, cpu_group);
>>  	}
>>  
>>  	free_cpumask_var(tmp);
>> @@ -170,24 +170,24 @@ static int hotplug_tests(void)
>>  {
>>  	int err;
>>  	cpumask_var_t offlined_cpus;
>> -	int i, nb_cluster;
>> -	const struct cpumask **clusters;
>> +	int i, nb_cpu_group;
>> +	const struct cpumask **cpu_groups;
>>  	char *page_buf;
>>  
>>  	err = -ENOMEM;
>>  	if (!alloc_cpumask_var(&offlined_cpus, GFP_KERNEL))
>>  		return err;
>> -	/* We may have up to nb_available_cpus clusters. */
>> -	clusters = kmalloc_array(nb_available_cpus, sizeof(*clusters),
>> -				 GFP_KERNEL);
>> -	if (!clusters)
>> +	/* We may have up to nb_available_cpus cpu_groups. */
>> +	cpu_groups = kmalloc_array(nb_available_cpus, sizeof(*cpu_groups),
>> +				   GFP_KERNEL);
>> +	if (!cpu_groups)
>>  		goto out_free_cpus;
>>  	page_buf = (char *)__get_free_page(GFP_KERNEL);
>>  	if (!page_buf)
>> -		goto out_free_clusters;
>> +		goto out_free_cpu_groups;
>>  
>>  	err = 0;
>> -	nb_cluster = find_clusters(cpu_online_mask, clusters);
>> +	nb_cpu_group = find_cpu_groups(cpu_online_mask, cpu_groups);
>>  
>>  	/*
>>  	 * Of course the last CPU cannot be powered down and cpu_down() should
>> @@ -197,24 +197,22 @@ static int hotplug_tests(void)
>>  	err += down_and_up_cpus(cpu_online_mask, offlined_cpus);
>>  
>>  	/*
>> -	 * Take down CPUs by cluster this time. When the last CPU is turned
>> -	 * off, the cluster itself should shut down.
>> +	 * Take down CPUs by cpu group this time. When the last CPU is turned
>> +	 * off, the cpu group itself should shut down.
>>  	 */
>> -	for (i = 0; i < nb_cluster; ++i) {
>> -		int cluster_id =
>> -			topology_physical_package_id(cpumask_any(clusters[i]));
>> +	for (i = 0; i < nb_cpu_group; ++i) {
>>  		ssize_t len = cpumap_print_to_pagebuf(true, page_buf,
>> -						      clusters[i]);
>> +						      cpu_groups[i]);
>>  		/* Remove trailing newline. */
>>  		page_buf[len - 1] = '\0';
>> -		pr_info("Trying to turn off and on again cluster %d "
>> -			"(CPUs %s)\n", cluster_id, page_buf);
>> -		err += down_and_up_cpus(clusters[i], offlined_cpus);
>> +		pr_info("Trying to turn off and on again group %d (CPUs %s)\n",
>> +			i, page_buf);
>> +		err += down_and_up_cpus(cpu_groups[i], offlined_cpus);
>>  	}
>>  
>>  	free_page((unsigned long)page_buf);
>> -out_free_clusters:
>> -	kfree(clusters);
>> +out_free_cpu_groups:
>> +	kfree(cpu_groups);
>>  out_free_cpus:
>>  	free_cpumask_var(offlined_cpus);
>>  	return err;
>> -- 
>> 2.7.4
>>

-- 
Regards,
Sudeep

-- 
Regards,
Sudeep

[PATCH 2/2] cpufreq: scpi: remove arm_big_little dependency

[Sudeep Holla]

The dependency on physical_package_id from the topology to get the
cluster identifier is wrong. The concept of cluster used in ARM topology
is unfortunately not well defined in the architecture, we should avoid
using it. Further the frequency domain need not be mapped to so called
"clusters" one to one.

SCPI already provides means to obtain the frequency domain id from the
device tree. In order to support some new topologies(e.g. DSU which
contains 2 frequency domains within the physical cluster), pseudo
clusters are created to make this driver work which is wrong again.

In order to solve those issues and also remove dependency of topological
physical id for frequency domain, this patch removes the arm_big_little
dependency from scpi driver.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/cpufreq/scpi-cpufreq.c | 193 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 178 insertions(+), 15 deletions(-)

diff --git a/drivers/cpufreq/scpi-cpufreq.c b/drivers/cpufreq/scpi-cpufreq.c
index 05d299052c5c..247fcbfa4cb5 100644
--- a/drivers/cpufreq/scpi-cpufreq.c
+++ b/drivers/cpufreq/scpi-cpufreq.c
@@ -18,27 +18,89 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/clk.h>
 #include <linux/cpu.h>
 #include <linux/cpufreq.h>
+#include <linux/cpumask.h>
+#include <linux/cpu_cooling.h>
+#include <linux/export.h>
 #include <linux/module.h>
-#include <linux/platform_device.h>
+#include <linux/of_platform.h>
 #include <linux/pm_opp.h>
 #include <linux/scpi_protocol.h>
+#include <linux/slab.h>
 #include <linux/types.h>
 
-#include "arm_big_little.h"
+struct scpi_data {
+	struct clk *clk;
+	struct device *cpu_dev;
+	struct thermal_cooling_device *cdev;
+};
 
 static struct scpi_ops *scpi_ops;
 
-static int scpi_get_transition_latency(struct device *cpu_dev)
+static unsigned int scpi_cpufreq_get_rate(unsigned int cpu)
+{
+	struct cpufreq_policy *policy = cpufreq_cpu_get_raw(cpu);
+	struct scpi_data *priv = policy->driver_data;
+	unsigned long rate = clk_get_rate(priv->clk);
+
+	return rate / 1000;
+}
+
+static int
+scpi_cpufreq_set_target(struct cpufreq_policy *policy, unsigned int index)
+{
+	struct scpi_data *priv = policy->driver_data;
+	u64 rate = policy->freq_table[index].frequency * 1000;
+	int ret;
+
+	ret = clk_set_rate(priv->clk, rate);
+	if (!ret && (clk_get_rate(priv->clk) != rate))
+		ret = -EIO;
+
+	return ret;
+}
+
+static int
+scpi_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask)
 {
-	return scpi_ops->get_transition_latency(cpu_dev);
+	int cpu, domain, tdomain;
+	struct device *tcpu_dev;
+
+	domain = scpi_ops->device_domain_id(cpu_dev);
+	if (domain < 0)
+		return domain;
+
+	for_each_possible_cpu(cpu) {
+		if (cpu == cpu_dev->id)
+			continue;
+
+		tcpu_dev = get_cpu_device(cpu);
+		if (!tcpu_dev)
+			continue;
+
+		tdomain = scpi_ops->device_domain_id(tcpu_dev);
+		if (tdomain == domain)
+			cpumask_set_cpu(cpu, cpumask);
+	}
+
+	return 0;
 }
 
-static int scpi_init_opp_table(const struct cpumask *cpumask)
+static int scpi_cpufreq_init(struct cpufreq_policy *policy)
 {
 	int ret;
-	struct device *cpu_dev = get_cpu_device(cpumask_first(cpumask));
+	unsigned int latency;
+	struct device *cpu_dev;
+	struct scpi_data *priv;
+	struct cpufreq_frequency_table *freq_table;
+
+	cpu_dev = get_cpu_device(policy->cpu);
+	if (!cpu_dev) {
+		pr_err("failed to get cpu%d device\n", policy->cpu);
+		return -ENODEV;
+	}
 
 	ret = scpi_ops->add_opps_to_device(cpu_dev);
 	if (ret) {
@@ -46,32 +108,133 @@ static int scpi_init_opp_table(const struct cpumask *cpumask)
 		return ret;
 	}
 
-	ret = dev_pm_opp_set_sharing_cpus(cpu_dev, cpumask);
-	if (ret)
+	ret = scpi_get_sharing_cpus(cpu_dev, policy->cpus);
+	if (ret) {
+		dev_warn(cpu_dev, "failed to get sharing cpumask\n");
+		return ret;
+	}
+
+	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;
+	}
+
+	ret = dev_pm_opp_get_opp_count(cpu_dev);
+	if (ret <= 0) {
+		dev_dbg(cpu_dev, "OPP table is not ready, deferring probe\n");
+		ret = -EPROBE_DEFER;
+		goto out_free_opp;
+	}
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv) {
+		ret = -ENOMEM;
+		goto out_free_opp;
+	}
+
+	ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
+	if (ret) {
+		dev_err(cpu_dev, "failed to init cpufreq table: %d\n", ret);
+		goto out_free_priv;
+	}
+
+	priv->cpu_dev = cpu_dev;
+	priv->clk = clk_get(cpu_dev, NULL);
+	if (IS_ERR(priv->clk)) {
+		dev_err(cpu_dev, "%s: Failed to get clk for cpu: %d\n",
+			__func__, cpu_dev->id);
+		goto out_free_cpufreq_table;
+	}
+
+	policy->driver_data = priv;
+
+	ret = cpufreq_table_validate_and_show(policy, freq_table);
+	if (ret) {
+		dev_err(cpu_dev, "%s: invalid frequency table: %d\n", __func__,
+			ret);
+		goto out_put_clk;
+	}
+
+	/* scpi allows DVFS request for any domain from any CPU */
+	policy->dvfs_possible_from_any_cpu = true;
+
+	latency = scpi_ops->get_transition_latency(cpu_dev);
+	if (!latency)
+		latency = CPUFREQ_ETERNAL;
+
+	policy->cpuinfo.transition_latency = latency;
+
+	policy->fast_switch_possible = false;
+	return 0;
+
+out_put_clk:
+	clk_put(priv->clk);
+out_free_cpufreq_table:
+	dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
+out_free_priv:
+	kfree(priv);
+out_free_opp:
+	dev_pm_opp_cpumask_remove_table(policy->cpus);
+
 	return ret;
 }
 
-static const struct cpufreq_arm_bL_ops scpi_cpufreq_ops = {
-	.name	= "scpi",
-	.get_transition_latency = scpi_get_transition_latency,
-	.init_opp_table = scpi_init_opp_table,
-	.free_opp_table = dev_pm_opp_cpumask_remove_table,
+static int scpi_cpufreq_exit(struct cpufreq_policy *policy)
+{
+	struct scpi_data *priv = policy->driver_data;
+
+	cpufreq_cooling_unregister(priv->cdev);
+	clk_put(priv->clk);
+	dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table);
+	kfree(priv);
+	dev_pm_opp_cpumask_remove_table(policy->related_cpus);
+
+	return 0;
+}
+
+static void scpi_cpufreq_ready(struct cpufreq_policy *policy)
+{
+	struct scpi_data *priv = policy->driver_data;
+	struct thermal_cooling_device *cdev;
+
+	cdev = of_cpufreq_cooling_register(policy);
+	if (!IS_ERR(cdev))
+		priv->cdev = cdev;
+}
+
+static struct cpufreq_driver scpi_cpufreq_driver = {
+	.name	= "scpi-cpufreq",
+	.flags	= CPUFREQ_STICKY | CPUFREQ_HAVE_GOVERNOR_PER_POLICY |
+		  CPUFREQ_NEED_INITIAL_FREQ_CHECK,
+	.verify	= cpufreq_generic_frequency_table_verify,
+	.attr	= cpufreq_generic_attr,
+	.get	= scpi_cpufreq_get_rate,
+	.init	= scpi_cpufreq_init,
+	.exit	= scpi_cpufreq_exit,
+	.ready	= scpi_cpufreq_ready,
+	.target_index	= scpi_cpufreq_set_target,
 };
 
 static int scpi_cpufreq_probe(struct platform_device *pdev)
 {
+	int ret;
+
 	scpi_ops = get_scpi_ops();
 	if (!scpi_ops)
 		return -EIO;
 
-	return bL_cpufreq_register(&scpi_cpufreq_ops);
+	ret = cpufreq_register_driver(&scpi_cpufreq_driver);
+	if (ret)
+		dev_err(&pdev->dev, "%s: registering cpufreq failed, err: %d\n",
+			__func__, ret);
+	return ret;
 }
 
 static int scpi_cpufreq_remove(struct platform_device *pdev)
 {
-	bL_cpufreq_unregister(&scpi_cpufreq_ops);
+	cpufreq_unregister_driver(&scpi_cpufreq_driver);
 	scpi_ops = NULL;
 	return 0;
 }
-- 
2.7.4

[PATCH 2/2] cpufreq: scpi: remove arm_big_little dependency

[Viresh Kumar]

On 09-01-18, 16:49, Sudeep Holla wrote:
> +static int
> +scpi_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask)
>  {
> -	return scpi_ops->get_transition_latency(cpu_dev);
> +	int cpu, domain, tdomain;
> +	struct device *tcpu_dev;
> +
> +	domain = scpi_ops->device_domain_id(cpu_dev);
> +	if (domain < 0)
> +		return domain;
> +
> +	for_each_possible_cpu(cpu) {
> +		if (cpu == cpu_dev->id)
> +			continue;
> +
> +		tcpu_dev = get_cpu_device(cpu);
> +		if (!tcpu_dev)
> +			continue;
> +
> +		tdomain = scpi_ops->device_domain_id(tcpu_dev);
> +		if (tdomain == domain)
> +			cpumask_set_cpu(cpu, cpumask);
> +	}
> +
> +	return 0;
>  }

So this is the main thing you want to achieve ? i.e. get the
policy->cpus from scpi_ops, right ?

Why can't we update .init_opp_table() to include policy as a parameter
and let individual stub drivers update policy->cpus then ?

-- 
viresh

[PATCH 2/2] cpufreq: scpi: remove arm_big_little dependency

[Sudeep Holla]

On 10/01/18 04:19, Viresh Kumar wrote:
> On 09-01-18, 16:49, Sudeep Holla wrote:
>> +static int
>> +scpi_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask)
>>  {
>> -	return scpi_ops->get_transition_latency(cpu_dev);
>> +	int cpu, domain, tdomain;
>> +	struct device *tcpu_dev;
>> +
>> +	domain = scpi_ops->device_domain_id(cpu_dev);
>> +	if (domain < 0)
>> +		return domain;
>> +
>> +	for_each_possible_cpu(cpu) {
>> +		if (cpu == cpu_dev->id)
>> +			continue;
>> +
>> +		tcpu_dev = get_cpu_device(cpu);
>> +		if (!tcpu_dev)
>> +			continue;
>> +
>> +		tdomain = scpi_ops->device_domain_id(tcpu_dev);
>> +		if (tdomain == domain)
>> +			cpumask_set_cpu(cpu, cpumask);
>> +	}
>> +
>> +	return 0;
>>  }
> 
> So this is the main thing you want to achieve ? i.e. get the
> policy->cpus from scpi_ops, right ?
> 

>From the looks of it yes, but...

> Why can't we update .init_opp_table() to include policy as a parameter
> and let individual stub drivers update policy->cpus then ?
> 

Possible, again but ...

There are few reasons why I would like remove scpi dependency on bL driver:

1. It has a notion of big and little which may not be true but that not
   much of a problem.

2. MAX_CLUSTER = 2 and scpi is getting used on multi-cluster systems
   though it was first tested on Juno which was bL system. There are
   quite a few internal FVP platforms that can manage to run with just
   proper DT with this change.

3. raw_cpu_to_cluster still calls topology_physical_package_id which
   breaks on these platforms and also with introduction of ACPI PPTT it
   will break on all ARM64 platforms.

4. We can still leave usage of topology_physical_package_id as is in
   arm_big_little.c worst case if it's going to be used only on ARM32
   For ARM64, topology_physical_package_id will be changed to give
   actual physical socket number which will be 0 for all multi-cluster
   (including bL) systems as along as they are single chip.

-- 
Regards,
Sudeep

[PATCH 2/2] cpufreq: scpi: remove arm_big_little dependency

[Viresh Kumar]

On 09-01-18, 16:49, Sudeep Holla wrote:
> The dependency on physical_package_id from the topology to get the
> cluster identifier is wrong. The concept of cluster used in ARM topology
> is unfortunately not well defined in the architecture, we should avoid
> using it. Further the frequency domain need not be mapped to so called
> "clusters" one to one.
> 
> SCPI already provides means to obtain the frequency domain id from the
> device tree. In order to support some new topologies(e.g. DSU which
> contains 2 frequency domains within the physical cluster), pseudo
> clusters are created to make this driver work which is wrong again.
> 
> In order to solve those issues and also remove dependency of topological
> physical id for frequency domain, this patch removes the arm_big_little
> dependency from scpi driver.
> 
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/cpufreq/scpi-cpufreq.c | 193 +++++++++++++++++++++++++++++++++++++----
>  1 file changed, 178 insertions(+), 15 deletions(-)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

[PATCH 2/2] cpufreq: scpi: remove arm_big_little dependency

[Sudeep Holla]

On 10/01/18 14:46, Viresh Kumar wrote:
> On 09-01-18, 16:49, Sudeep Holla wrote:
>> The dependency on physical_package_id from the topology to get the
>> cluster identifier is wrong. The concept of cluster used in ARM topology
>> is unfortunately not well defined in the architecture, we should avoid
>> using it. Further the frequency domain need not be mapped to so called
>> "clusters" one to one.
>>
>> SCPI already provides means to obtain the frequency domain id from the
>> device tree. In order to support some new topologies(e.g. DSU which
>> contains 2 frequency domains within the physical cluster), pseudo
>> clusters are created to make this driver work which is wrong again.
>>
>> In order to solve those issues and also remove dependency of topological
>> physical id for frequency domain, this patch removes the arm_big_little
>> dependency from scpi driver.
>>
>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>> Cc: Viresh Kumar <viresh.kumar@linaro.org>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>  drivers/cpufreq/scpi-cpufreq.c | 193 +++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 178 insertions(+), 15 deletions(-)
> 
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> 

Thanks :)

-- 
Regards,
Sudeep