| Message ID | 1400029876-5830-8-git-send-email-thomas.ab@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wednesday 14 May 2014 06:41:15 Thomas Abraham wrote: > From: Thomas Abraham <thomas.ab@samsung.com> > > Remove the platform device instantiation for Exynos specific cpufreq > driver and add the platform device for cpufreq-cpu0 driver. > > Signed-off-by: Thomas Abraham <thomas.ab@samsung.com> > --- > arch/arm/mach-exynos/exynos.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c > index b32a907..489a495 100644 > --- a/arch/arm/mach-exynos/exynos.c > +++ b/arch/arm/mach-exynos/exynos.c > @@ -232,7 +232,9 @@ void __init exynos_cpuidle_init(void) > > void __init exynos_cpufreq_init(void) > { > - platform_device_register_simple("exynos-cpufreq", -1, NULL, 0); > + if (!(of_machine_is_compatible("samsung,exynos5420")) && > + !(of_machine_is_compatible("samsung,exynos5440"))) > + platform_device_register_simple("cpufreq-cpu0", -1, NULL, 0); > } > Could we please come up with a way to probe this from DT in the cpufreq-cpu0 driver itself, so we don't have to add a device in every platform using it? I realize you copied it from the other platforms using this driver, but it still seems really wrong. Arnd
On 14 May 2014 18:20, Arnd Bergmann <arnd@arndb.de> wrote: > Could we please come up with a way to probe this from DT in the cpufreq-cpu0 > driver itself, so we don't have to add a device in every platform using it? Its followed that way because DT Maintainers had strong objections to creating virtual device nodes and haven't allowed creation of nodes for cpufreq drivers.. For which there is no physical device, as CPU already has a separate node..
Am Mittwoch, 14. Mai 2014, 18:35:29 schrieb Viresh Kumar: > On 14 May 2014 18:20, Arnd Bergmann <arnd@arndb.de> wrote: > > Could we please come up with a way to probe this from DT in the > > cpufreq-cpu0 driver itself, so we don't have to add a device in every > > platform using it? > Its followed that way because DT Maintainers had strong objections > to creating virtual device nodes and haven't allowed creation of nodes > for cpufreq drivers.. For which there is no physical device, as CPU already > has a separate node.. as we already have the "enable-method" property for enabling/disabling cpus, would something like a "scaling-method" be feasible?
On 14 May 2014 18:41, Heiko Stübner <heiko@sntech.de> wrote: > Am Mittwoch, 14. Mai 2014, 18:35:29 schrieb Viresh Kumar: >> On 14 May 2014 18:20, Arnd Bergmann <arnd@arndb.de> wrote: >> > Could we please come up with a way to probe this from DT in the >> > cpufreq-cpu0 driver itself, so we don't have to add a device in every >> > platform using it? >> Its followed that way because DT Maintainers had strong objections >> to creating virtual device nodes and haven't allowed creation of nodes >> for cpufreq drivers.. For which there is no physical device, as CPU already >> has a separate node.. > > as we already have the "enable-method" property for enabling/disabling cpus, > would something like a "scaling-method" be feasible? Lets see what DT maintainers have to say on this, I would rather go for a more straight forward name: "scaling-driver" :) ..
On Wednesday 14 May 2014 18:44:46 Viresh Kumar wrote: > On 14 May 2014 18:41, Heiko Stübner <heiko@sntech.de> wrote: > > Am Mittwoch, 14. Mai 2014, 18:35:29 schrieb Viresh Kumar: > >> On 14 May 2014 18:20, Arnd Bergmann <arnd@arndb.de> wrote: > >> > Could we please come up with a way to probe this from DT in the > >> > cpufreq-cpu0 driver itself, so we don't have to add a device in every > >> > platform using it? > > >> Its followed that way because DT Maintainers had strong objections > >> to creating virtual device nodes and haven't allowed creation of nodes > >> for cpufreq drivers.. For which there is no physical device, as CPU already > >> has a separate node.. > > > > as we already have the "enable-method" property for enabling/disabling cpus, > > would something like a "scaling-method" be feasible? Good idea to put it as a property into the CPU node. > Lets see what DT maintainers have to say on this, I would rather go for a > more straight forward name: "scaling-driver" .. Both sound fine to me. Arnd
On Wed, May 14, 2014 at 8:18 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Wednesday 14 May 2014 18:44:46 Viresh Kumar wrote: >> On 14 May 2014 18:41, Heiko Stübner <heiko@sntech.de> wrote: >> > Am Mittwoch, 14. Mai 2014, 18:35:29 schrieb Viresh Kumar: >> >> On 14 May 2014 18:20, Arnd Bergmann <arnd@arndb.de> wrote: >> >> > Could we please come up with a way to probe this from DT in the >> >> > cpufreq-cpu0 driver itself, so we don't have to add a device in every >> >> > platform using it? >> >> >> Its followed that way because DT Maintainers had strong objections >> >> to creating virtual device nodes and haven't allowed creation of nodes >> >> for cpufreq drivers.. For which there is no physical device, as CPU already >> >> has a separate node.. >> > >> > as we already have the "enable-method" property for enabling/disabling cpus, >> > would something like a "scaling-method" be feasible? > > Good idea to put it as a property into the CPU node. We already have properties which indicate this driver can be used by a platform: opp table and a clock for the cpu. If this information is not sufficient to determine whether you can use this driver or not, then you simply need to match against the platform. Perhaps the match list should be a blacklist rather than a whitelist, so new platforms work without a kernel change. Alternatively, create a new OPP binding that addresses this and all the other limitations in the current OPP binding. >> Lets see what DT maintainers have to say on this, I would rather go for a >> more straight forward name: "scaling-driver" .. > > Both sound fine to me. The fact that linux needs a way to create a platform device to enable a certain driver is not a DT problem. I proposed a solution for how to get this out of the platform code [1], but evidently we want people to open code the exceptions and adding boilerplate helpers will just encourage the exceptions. Rob [1] https://lkml.org/lkml/2013/10/30/30
On Wed, May 14, 2014 at 6:41 PM, Heiko Stübner <heiko@sntech.de> wrote: > Am Mittwoch, 14. Mai 2014, 18:35:29 schrieb Viresh Kumar: >> On 14 May 2014 18:20, Arnd Bergmann <arnd@arndb.de> wrote: >> > Could we please come up with a way to probe this from DT in the >> > cpufreq-cpu0 driver itself, so we don't have to add a device in every >> > platform using it? >> Its followed that way because DT Maintainers had strong objections >> to creating virtual device nodes and haven't allowed creation of nodes >> for cpufreq drivers.. For which there is no physical device, as CPU already >> has a separate node.. > > as we already have the "enable-method" property for enabling/disabling cpus, > would something like a "scaling-method" be feasible? > "scaling-method" also sounds like a software specific property. Would that be something that will be acceptable in dt?
On 14/05/14 15:03, Thomas Abraham wrote: > On Wed, May 14, 2014 at 6:41 PM, Heiko Stübner <heiko@sntech.de> wrote: >> Am Mittwoch, 14. Mai 2014, 18:35:29 schrieb Viresh Kumar: >>> On 14 May 2014 18:20, Arnd Bergmann <arnd@arndb.de> wrote: >>>> Could we please come up with a way to probe this from DT in the >>>> cpufreq-cpu0 driver itself, so we don't have to add a device in every >>>> platform using it? >>> Its followed that way because DT Maintainers had strong objections >>> to creating virtual device nodes and haven't allowed creation of nodes >>> for cpufreq drivers.. For which there is no physical device, as CPU already >>> has a separate node.. >> >> as we already have the "enable-method" property for enabling/disabling cpus, >> would something like a "scaling-method" be feasible? >> > > "scaling-method" also sounds like a software specific property. Would > that be something that will be acceptable in dt? How about dvfs-method ? But the value should not be based on the driver they use, but something more generic. Regards, Sudeep
On Wed, May 14, 2014 at 6:20 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Wednesday 14 May 2014 06:41:15 Thomas Abraham wrote: >> From: Thomas Abraham <thomas.ab@samsung.com> >> >> Remove the platform device instantiation for Exynos specific cpufreq >> driver and add the platform device for cpufreq-cpu0 driver. >> >> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com> >> --- >> arch/arm/mach-exynos/exynos.c | 4 +++- >> 1 files changed, 3 insertions(+), 1 deletions(-) >> >> diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c >> index b32a907..489a495 100644 >> --- a/arch/arm/mach-exynos/exynos.c >> +++ b/arch/arm/mach-exynos/exynos.c >> @@ -232,7 +232,9 @@ void __init exynos_cpuidle_init(void) >> >> void __init exynos_cpufreq_init(void) >> { >> - platform_device_register_simple("exynos-cpufreq", -1, NULL, 0); >> + if (!(of_machine_is_compatible("samsung,exynos5420")) && >> + !(of_machine_is_compatible("samsung,exynos5440"))) >> + platform_device_register_simple("cpufreq-cpu0", -1, NULL, 0); >> } >> > > Could we please come up with a way to probe this from DT in the cpufreq-cpu0 > driver itself, so we don't have to add a device in every platform using it? Okay, I don't have a solution for this as of now. Would this be considered as a blocker for this series? I hope we could just live with this for now. Thanks, Thomas. > > I realize you copied it from the other platforms using this driver, but > it still seems really wrong. > > Arnd
On Wednesday 14 May 2014 08:45:23 Rob Herring wrote: > On Wed, May 14, 2014 at 8:18 AM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Wednesday 14 May 2014 18:44:46 Viresh Kumar wrote: > >> On 14 May 2014 18:41, Heiko Stübner <heiko@sntech.de> wrote: > >> > Am Mittwoch, 14. Mai 2014, 18:35:29 schrieb Viresh Kumar: > >> >> On 14 May 2014 18:20, Arnd Bergmann <arnd@arndb.de> wrote: > >> >> > Could we please come up with a way to probe this from DT in the > >> >> > cpufreq-cpu0 driver itself, so we don't have to add a device in every > >> >> > platform using it? > >> > >> >> Its followed that way because DT Maintainers had strong objections > >> >> to creating virtual device nodes and haven't allowed creation of nodes > >> >> for cpufreq drivers.. For which there is no physical device, as CPU already > >> >> has a separate node.. > >> > > >> > as we already have the "enable-method" property for enabling/disabling cpus, > >> > would something like a "scaling-method" be feasible? > > > > Good idea to put it as a property into the CPU node. > > We already have properties which indicate this driver can be used by a > platform: opp table and a clock for the cpu. If this information is > not sufficient to determine whether you can use this driver or not, > then you simply need to match against the platform. Perhaps the match > list should be a blacklist rather than a whitelist, so new platforms > work without a kernel change. We'd not only need a blacklist, but also a way to tell whether we want to use the cpu0 or the big/little implementation, which currently have indistinguishable bindings. > Alternatively, create a new OPP binding that addresses this and all > the other limitations in the current OPP binding. Yes. > >> Lets see what DT maintainers have to say on this, I would rather go for a > >> more straight forward name: "scaling-driver" .. > > > > Both sound fine to me. > > The fact that linux needs a way to create a platform device to enable > a certain driver is not a DT problem. I proposed a solution for how to > get this out of the platform code [1], but evidently we want people to > open code the exceptions and adding boilerplate helpers will just > encourage the exceptions. I think the only benefit we have from using platform devices at all for cpufreq (not for cpuidle, which has a similar problem) is module autoloading. I think your patch doesn't actually help with that. Arnd
On 14.05.2014 03:11, Thomas Abraham wrote: > From: Thomas Abraham <thomas.ab@samsung.com> > > Remove the platform device instantiation for Exynos specific cpufreq > driver and add the platform device for cpufreq-cpu0 driver. > > Signed-off-by: Thomas Abraham <thomas.ab@samsung.com> > --- > arch/arm/mach-exynos/exynos.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c > index b32a907..489a495 100644 > --- a/arch/arm/mach-exynos/exynos.c > +++ b/arch/arm/mach-exynos/exynos.c > @@ -232,7 +232,9 @@ void __init exynos_cpuidle_init(void) > > void __init exynos_cpufreq_init(void) > { > - platform_device_register_simple("exynos-cpufreq", -1, NULL, 0); > + if (!(of_machine_is_compatible("samsung,exynos5420")) && > + !(of_machine_is_compatible("samsung,exynos5440"))) > + platform_device_register_simple("cpufreq-cpu0", -1, NULL, 0); > } > > void __init exynos_init_late(void) > As a good intermediate step that can be completely replaced in future, if necessary (as opposed to stable DT bindings...): Acked-by: Tomasz Figa <t.figa@samsung.com> Best regards, Tomasz
Hi Arnd/Rob/Mike et al, We didn't conclude anything out of this thread and so kicking it again as we need to close bindings to support cpufreq-cpu0 better for platforms not sharing clock lines across all CPUs. https://lkml.org/lkml/2014/7/1/358 On 14 May 2014 20:03, Arnd Bergmann <arnd@arndb.de> wrote: > On Wednesday 14 May 2014 08:45:23 Rob Herring wrote: >> We already have properties which indicate this driver can be used by a >> platform: opp table and a clock for the cpu. If this information is There can be platform drivers which also depend on these properties and picking cpufreq-cpu0 on this basis doesn't look correct. >> not sufficient to determine whether you can use this driver or not, >> then you simply need to match against the platform. Perhaps the match >> list should be a blacklist rather than a whitelist, so new platforms >> work without a kernel change. > > We'd not only need a blacklist, but also a way to tell whether we > want to use the cpu0 or the big/little implementation, which currently > have indistinguishable bindings. Correct and there can be other platform drivers which cannot use cpufreq-cpu0 (though I am trying to force people to use cpufreq-cpu0 instead of a new driver). Is something terribly wrong with having a property at 'cpus' node which can point to the driver we want to use? Like: cpus { #address-cells = <1>; #size-cells = <0>; scaling-method = "cpufreq-cpu0" cpu@0 { .... }; .... }; Or if we can reuse compatibility string some way. [Copying mail from Mike] On 15 May 2014 02:46, Mike Turquette <mturquette@linaro.org> wrote: > The hardware property that matters for cpufreq-cpu0 users is that a > multi-core CPU uses a single clock input to scale frequency across all > of the cores in that cluster. So an accurate description is: > > scaling-method = "clock-ganged"; //hardware-people-speak > > Or, > > scaling-method = "clock-shared"; //software-people-speak > > Versus independently scalable CPUs in an SMP cluster: > > scaling-method = "independent"; //x86, Krait, etc. > > Or perhaps instead of "independent" at the parent "cpus" node we would > put the following in each cpu@N node: > > scaling-method = "clock"; > > Or "psci" or "acpi" or whatever. > > Thought exercise: for Hyperthreaded(tm) CPUs with 2 virtual cores for > every hard CPU (and multiple CPUs in a cluster): > > scaling-method = "paired"; > > Or more simply, "hyperthreaded". Probably we have mixed both the problems. We have two problems to solve: - Identifying which driver to probe for a platform, earlier explanation I tried to gave were around that.. - Identifying if clocks are shared between CPUs? If yes which ones? Probably Mike's suggestions were around this second problem, but I still couldn't make out which CPUs share clock line from his examples. Please see if we can close this thread soon... Few platforms are waiting to reuse cpufreq-cpu0 :) -- viresh
diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c index b32a907..489a495 100644 --- a/arch/arm/mach-exynos/exynos.c +++ b/arch/arm/mach-exynos/exynos.c @@ -232,7 +232,9 @@ void __init exynos_cpuidle_init(void) void __init exynos_cpufreq_init(void) { - platform_device_register_simple("exynos-cpufreq", -1, NULL, 0); + if (!(of_machine_is_compatible("samsung,exynos5420")) && + !(of_machine_is_compatible("samsung,exynos5440"))) + platform_device_register_simple("cpufreq-cpu0", -1, NULL, 0); } void __init exynos_init_late(void)