[5/6] ARM: Tegra: start using cpufreq-cpu0 driver

Message ID 6610c86618b781b00eba446ca19035e077d99691.1375886595.git.viresh.kumar@linaro.org
State New
Headers show

Commit Message

Viresh Kumar Aug. 7, 2013, 2:46 p.m.
cpufreq-cpu0 driver can be probed over DT only if a corresponding device node is
created for the SoC which wants to use it. Lets create a platform device for
cpufreq-cpu0 driver for Tegra.

Also it removes the Kconfig entry responsible to compiling tegra-cpufreq driver
and hence there will not be any conflicts between two cpufreq drivers.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 arch/arm/mach-tegra/tegra.c | 2 ++
 drivers/cpufreq/Kconfig.arm | 8 --------
 2 files changed, 2 insertions(+), 8 deletions(-)

Comments

Stephen Warren Aug. 7, 2013, 5:46 p.m. | #1
On 08/07/2013 08:46 AM, Viresh Kumar wrote:
> cpufreq-cpu0 driver can be probed over DT only if a corresponding device node is
> created for the SoC which wants to use it. Lets create a platform device for
> cpufreq-cpu0 driver for Tegra.
> 
> Also it removes the Kconfig entry responsible to compiling tegra-cpufreq driver
> and hence there will not be any conflicts between two cpufreq drivers.

> diff --git a/arch/arm/mach-tegra/tegra.c b/arch/arm/mach-tegra/tegra.c

>  static void __init tegra_dt_init(void)
>  {
> +	struct platform_device_info devinfo = { .name = "cpufreq-cpu0", };

static? const?

>  	struct soc_device_attribute *soc_dev_attr;
>  	struct soc_device *soc_dev;
>  	struct device *parent = NULL;
>  
>  	tegra_clocks_apply_init_table();
> +	platform_device_register_full(&devinfo);

This seems awfully like going back to board files. Shouldn't something
that binds to the CPU nodes register the cpufreq device automatically,
based on the CPU's compatible value?
Viresh Kumar Aug. 7, 2013, 5:49 p.m. | #2
On 7 August 2013 23:16, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 08/07/2013 08:46 AM, Viresh Kumar wrote:
>> cpufreq-cpu0 driver can be probed over DT only if a corresponding device node is
>> created for the SoC which wants to use it. Lets create a platform device for
>> cpufreq-cpu0 driver for Tegra.
>>
>> Also it removes the Kconfig entry responsible to compiling tegra-cpufreq driver
>> and hence there will not be any conflicts between two cpufreq drivers.
>
>> diff --git a/arch/arm/mach-tegra/tegra.c b/arch/arm/mach-tegra/tegra.c
>
>>  static void __init tegra_dt_init(void)
>>  {
>> +     struct platform_device_info devinfo = { .name = "cpufreq-cpu0", };
>
> static? const?

static: yes
const: no, as it might be modified by platform_device_register_full()

>>       struct soc_device_attribute *soc_dev_attr;
>>       struct soc_device *soc_dev;
>>       struct device *parent = NULL;
>>
>>       tegra_clocks_apply_init_table();
>> +     platform_device_register_full(&devinfo);
>
> This seems awfully like going back to board files. Shouldn't something
> that binds to the CPU nodes register the cpufreq device automatically,
> based on the CPU's compatible value?

This link has got some information why we can't have a node for cpufreq
or a compatibility value..

http://permalink.gmane.org/gmane.linux.kernel.cpufreq/9018
Stephen Warren Aug. 7, 2013, 5:53 p.m. | #3
On 08/07/2013 11:49 AM, Viresh Kumar wrote:
> On 7 August 2013 23:16, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 08/07/2013 08:46 AM, Viresh Kumar wrote:
>>> cpufreq-cpu0 driver can be probed over DT only if a corresponding device node is
>>> created for the SoC which wants to use it. Lets create a platform device for
>>> cpufreq-cpu0 driver for Tegra.
>>>
>>> Also it removes the Kconfig entry responsible to compiling tegra-cpufreq driver
>>> and hence there will not be any conflicts between two cpufreq drivers.
>>
>>> diff --git a/arch/arm/mach-tegra/tegra.c b/arch/arm/mach-tegra/tegra.c
>>
>>>  static void __init tegra_dt_init(void)
>>>  {
>>> +     struct platform_device_info devinfo = { .name = "cpufreq-cpu0", };
>>>       struct soc_device_attribute *soc_dev_attr;
>>>       struct soc_device *soc_dev;
>>>       struct device *parent = NULL;
>>>
>>>       tegra_clocks_apply_init_table();
>>> +     platform_device_register_full(&devinfo);
>>
>> This seems awfully like going back to board files. Shouldn't something
>> that binds to the CPU nodes register the cpufreq device automatically,
>> based on the CPU's compatible value?
> 
> This link has got some information why we can't have a node for cpufreq
> or a compatibility value..
> 
> http://permalink.gmane.org/gmane.linux.kernel.cpufreq/9018

That link only describes why we shouldn't have a dedicated compatible
value for cpufreq. I certainly agree with that. However, I think it's
reasonable that whatever code binds to:

	compatible = "arm,cortex-a9";

... should instantiate any virtual devices that relate to the CPU.

Doing so would be similar to how the Tegra I2S driver instantiates the
internal struct device that ASoC needs for the PCM/DMA device, rather
than having board-dt-tegra20.c do it, like it would have done in
board-file days.
Viresh Kumar Aug. 7, 2013, 5:59 p.m. | #4
On 7 August 2013 23:23, Stephen Warren <swarren@wwwdotorg.org> wrote:
> That link only describes why we shouldn't have a dedicated compatible
> value for cpufreq. I certainly agree with that. However, I think it's
> reasonable that whatever code binds to:
>
>         compatible = "arm,cortex-a9";
>
> ... should instantiate any virtual devices that relate to the CPU.

But how would we know here if platform really wants us to probe
cpufreq-cpu0 driver? On multiplatform kernel there can be multiple
cpufreq drivers available and there has to be some sort of code
in DT or platform code that reflects which driver we want to use.

We never required a device node for cpufreq, platform device was
added just to solve this issue.

> Doing so would be similar to how the Tegra I2S driver instantiates the
> internal struct device that ASoC needs for the PCM/DMA device, rather
> than having board-dt-tegra20.c do it, like it would have done in
> board-file days.

I haven't gone through it yet though :)
Stephen Warren Aug. 7, 2013, 6:51 p.m. | #5
On 08/07/2013 11:59 AM, Viresh Kumar wrote:
> On 7 August 2013 23:23, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> That link only describes why we shouldn't have a dedicated compatible
>> value for cpufreq. I certainly agree with that. However, I think it's
>> reasonable that whatever code binds to:
>>
>>         compatible = "arm,cortex-a9";
>>
>> ... should instantiate any virtual devices that relate to the CPU.
> 
> But how would we know here if platform really wants us to probe
> cpufreq-cpu0 driver? On multiplatform kernel there can be multiple
> cpufreq drivers available and there has to be some sort of code
> in DT or platform code that reflects which driver we want to use.

Presumably the code would look at the top-level DT node's compatible
value (e.g. "nvidia,tegra20").
Viresh Kumar Aug. 8, 2013, 2:48 a.m. | #6
On 8 August 2013 00:21, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 08/07/2013 11:59 AM, Viresh Kumar wrote:
>> On 7 August 2013 23:23, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> That link only describes why we shouldn't have a dedicated compatible
>>> value for cpufreq. I certainly agree with that. However, I think it's
>>> reasonable that whatever code binds to:
>>>
>>>         compatible = "arm,cortex-a9";
>>>
>>> ... should instantiate any virtual devices that relate to the CPU.
>>
>> But how would we know here if platform really wants us to probe
>> cpufreq-cpu0 driver? On multiplatform kernel there can be multiple
>> cpufreq drivers available and there has to be some sort of code
>> in DT or platform code that reflects which driver we want to use.
>
> Presumably the code would look at the top-level DT node's compatible
> value (e.g. "nvidia,tegra20").

So you are actually asking us to get a compatibility list inside
cpufreq-cpu0 driver which will list all the platforms for which this driver
would work?

Honestly speaking I wasn't in favor of getting a platform-device
registered for cpufreq-cpu0 earlier and had few discussion on the
thread I passed to you.

The problem with the new solution you just proposed is, for every
new platform that comes in we need to update this file.. And that's
it probably..

Don't know how others would see it...
@Rafael/Rob/Shawn: Any suggestions here?

Patch

diff --git a/arch/arm/mach-tegra/tegra.c b/arch/arm/mach-tegra/tegra.c
index 0d1e412..6ab3f69 100644
--- a/arch/arm/mach-tegra/tegra.c
+++ b/arch/arm/mach-tegra/tegra.c
@@ -82,11 +82,13 @@  static struct of_dev_auxdata tegra20_auxdata_lookup[] __initdata = {
 
 static void __init tegra_dt_init(void)
 {
+	struct platform_device_info devinfo = { .name = "cpufreq-cpu0", };
 	struct soc_device_attribute *soc_dev_attr;
 	struct soc_device *soc_dev;
 	struct device *parent = NULL;
 
 	tegra_clocks_apply_init_table();
+	platform_device_register_full(&devinfo);
 
 	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
 	if (!soc_dev_attr)
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index de4d5d9..9472160 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -215,11 +215,3 @@  config ARM_SPEAR_CPUFREQ
 	default y
 	help
 	  This adds the CPUFreq driver support for SPEAr SOCs.
-
-config ARM_TEGRA_CPUFREQ
-	bool "TEGRA CPUFreq support"
-	depends on ARCH_TEGRA
-	select CPU_FREQ_TABLE
-	default y
-	help
-	  This adds the CPUFreq driver support for TEGRA SOCs.