| Message ID | fcb88cd21f31a467d2d49911c2505082837f72ea.1410323179.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
Dear Viresh Kumar, On Wed, 10 Sep 2014 09:59:07 +0530, Viresh Kumar wrote: > Yesterday again I was pinged by Thomas about the status of this work and the > thread isn't going anywhere. He is blocked with just this to get his platform > running on mainline. > > Will it be possible to get this through as a temporary solution? A compile time conditional does not work, as it isn't compatible with multiplatform: what if in the same kernel you want to support two platforms that use cpufreq-dt, one having a single clock for all CPUs, and one having one clock per CPU. I'm still confused as to what is the problem with the DT binding surrounding this driver. Isn't it simply a matter of adding a clocks = <...> property to all CPUs, and that's it? Viresh told me there was an issue related to comparing the clocks in the driver to find out which CPUs share which clocks. But that is not a DT binding issue, it's an implementation detail. Since the driver is purely for DT platforms (as its name suggest), comparing phandles could work as a temporary solution. And since it's just an implementation detail that doesn't affect the DT binding, we could change the implementation later to use a proper clock API like clk_is_same(clk1, clk2) that would tell us if the clocks are the same. But I have probably missed some of the complexity of this. Thanks, Thomas
On 10 September 2014 11:59, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > A compile time conditional does not work, as it isn't compatible with > multiplatform: what if in the same kernel you want to support two > platforms that use cpufreq-dt, one having a single clock for all CPUs, > and one having one clock per CPU. Yeah, that's a known problem. Should have mentioned this in logs. > I'm still confused as to what is the problem with the DT binding > surrounding this driver. Isn't it simply a matter of adding a clocks = > <...> property to all CPUs, and that's it? > > Viresh told me there was an issue related to comparing the clocks in > the driver to find out which CPUs share which clocks. But that is not a > DT binding issue, it's an implementation detail. Since the driver is > purely for DT platforms (as its name suggest), comparing phandles could > work as a temporary solution. And since it's just an implementation > detail that doesn't affect the DT binding, we could change the > implementation later to use a proper clock API like clk_is_same(clk1, > clk2) that would tell us if the clocks are the same. > > But I have probably missed some of the complexity of this. (Couldn't give you these links yesterday as I was done for the day and was replying from my phone (In our private chat)) https://lkml.org/lkml/2014/6/26/577 https://lkml.org/lkml/2014/7/1/565 https://lkml.org/lkml/2014/7/3/813 http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/275520.html So these are few mails I could pick, specially from Mike, where he pointed out why getting a clk-API for this is a bad idea. -- viresh -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, Here is a proposal to solve the cpufreq-dt issue. We don't seem to be anywhere near reaching an agreement on a DT binding to represent the topology of CPU clocks in a system, because it's a complicated matter. Mike doesn't want to add a clock API that would allow the generic cpufreq driver to find out which CPUs share clocks, as he believes this is part of the hardware description and should therefore be described in the DT rather than "guessed" dynamically at boot time by looking at the clocks referenced by each CPU. And with Viresh not accepting any machine specific driver, it results in platforms like Armada XP having no solution to support cpufreq... So this proposal consists in adding a platform_data flag for the cpufreq-dt driver, which allows platform code to tell whether CPU clocks are shared or are independent. If you don't like platform_data, we can also register two different platform_driver for the two different cases, simply with different names. Another approach would be to lift the ban on machine-specific cpufreq drivers, since the generic driver is not capable of handling all situations. Thomas Thomas Petazzoni (4): cpufreq: allow driver-specific flags cpufreq: cpufreq-dt: extend with platform_data ARM: mvebu: use the cpufreq-dt platform_data for independent clocks cpufreq: cpufreq-dt: remove warning about regulators arch/arm/mach-mvebu/pmsu.c | 8 +++++++- drivers/cpufreq/cpufreq-dt.c | 23 ++++++++++++++++++----- drivers/cpufreq/cpufreq.c | 15 +++++++++++++++ include/linux/cpufreq-dt.h | 22 ++++++++++++++++++++++ include/linux/cpufreq.h | 5 ++++- 5 files changed, 66 insertions(+), 7 deletions(-) create mode 100644 include/linux/cpufreq-dt.h
On Wednesday 10 September 2014 11:41:48 Thomas Petazzoni wrote: > We don't seem to be anywhere near reaching an agreement on a DT > binding to represent the topology of CPU clocks in a system, because > it's a complicated matter. Mike doesn't want to add a clock API that > would allow the generic cpufreq driver to find out which CPUs share > clocks, as he believes this is part of the hardware description and > should therefore be described in the DT rather than "guessed" > dynamically at boot time by looking at the clocks referenced by each > CPU. And with Viresh not accepting any machine specific driver, it > results in platforms like Armada XP having no solution to support > cpufreq... > > So this proposal consists in adding a platform_data flag for the > cpufreq-dt driver, which allows platform code to tell whether CPU > clocks are shared or are independent. > > If you don't like platform_data, we can also register two different > platform_driver for the two different cases, simply with different > names. > > Another approach would be to lift the ban on machine-specific cpufreq > drivers, since the generic driver is not capable of handling all > situations. Using platform_data works nicely if you register the cpufreq device from the platform code, but that is a particular thing that has been bugging me for a long time and that I really want to get rid of: We don't want to introduce platform-specific files for ARM64, so we have to solve this anyway by just looking at DT, and whatever solution we end up with should work for both ARM32 and ARM64. Most of us will be at LCU next week, so I'd suggest we solve this problem using the 'lock everyone into one room without beer until we come up with a working approach' method. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Arnd, On Wed, 10 Sep 2014 11:53:27 +0200, Arnd Bergmann wrote: > > Another approach would be to lift the ban on machine-specific cpufreq > > drivers, since the generic driver is not capable of handling all > > situations. > > Using platform_data works nicely if you register the cpufreq device > from the platform code, but that is a particular thing that has > been bugging me for a long time and that I really want to get rid > of: We don't want to introduce platform-specific files for ARM64, > so we have to solve this anyway by just looking at DT, and whatever > solution we end up with should work for both ARM32 and ARM64. Right. Note that there is a similar issue with cpuidle. Maybe each cpufreq/cpuidle driver should simply list of top-level compatible strings it is compatible with, so that at boot time, each cpufreq/cpuidle driver gets a chance to init itself and see whether it is matching the currently running platform. > Most of us will be at LCU next week, so I'd suggest we solve this > problem using the 'lock everyone into one room without beer until > we come up with a working approach' method. That does seem like an interesting idea. However, I initially wanted to push cpufreq support for Armada XP in 3.17, and everything was pushed *except* the cpufreq driver bits. So I was hoping to at least get things working for 3.18. If the discussion takes place next week, and then the time to finally implement something, we'll also miss 3.18. In the mean time, would it be possible to realize that the existing generic cpufreq driver simply isn't generic enough, and that we should accept machine-specific cpufreq drivers for the time being? Thanks, Thomas
On Wednesday 10 September 2014 12:10:02 Thomas Petazzoni wrote: > On Wed, 10 Sep 2014 11:53:27 +0200, Arnd Bergmann wrote: > > > > Another approach would be to lift the ban on machine-specific cpufreq > > > drivers, since the generic driver is not capable of handling all > > > situations. > > > > Using platform_data works nicely if you register the cpufreq device > > from the platform code, but that is a particular thing that has > > been bugging me for a long time and that I really want to get rid > > of: We don't want to introduce platform-specific files for ARM64, > > so we have to solve this anyway by just looking at DT, and whatever > > solution we end up with should work for both ARM32 and ARM64. > > Right. Note that there is a similar issue with cpuidle. > > Maybe each cpufreq/cpuidle driver should simply list of top-level > compatible strings it is compatible with, so that at boot time, each > cpufreq/cpuidle driver gets a chance to init itself and see whether it > is matching the currently running platform. The main problem I see with that is that it requires updating the driver for every new platform we want to support. It would be better to find a way that lets you add new platforms using just new dts files that work with the existing driver. This is the main reason why we try to never match the root compatible property in any driver, and I don't think cpufreq should be an exception to this just because it can't easily be mapped to a particular register set. > > Most of us will be at LCU next week, so I'd suggest we solve this > > problem using the 'lock everyone into one room without beer until > > we come up with a working approach' method. > > That does seem like an interesting idea. However, I initially wanted to > push cpufreq support for Armada XP in 3.17, and everything was pushed > *except* the cpufreq driver bits. So I was hoping to at least get > things working for 3.18. If the discussion takes place next week, and > then the time to finally implement something, we'll also miss 3.18. I see. > In the mean time, would it be possible to realize that the existing > generic cpufreq driver simply isn't generic enough, and that we should > accept machine-specific cpufreq drivers for the time being? That would work for me, to get things going for 3.18, but it's really up to the cpufreq maintainers. Your platform_data based patch set seems like a better solution to me than separate drivers, but I haven't looked at the details. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Arnd, On Wed, 10 Sep 2014 12:19:41 +0200, Arnd Bergmann wrote: > the driver for every new platform we want to support. It would > be better to find a way that lets you add new platforms using just > new dts files that work with the existing driver. What do you call a "platform" ? A board or a SoC ? The top-level compatible string contain both references to the board and the SoC, and my idea was obviously that the cpufreq driver would match against the SoC compatible string, not the board one. This definitely allows to support additional boards by adding more dts without touching the code. Of course, it however means that when a new SoC shows up, you'll need to adjust the cpufreq driver. But you generally anyway need some kernel changes to support a new SoC: .dtsi changes are rarely sufficient to completely support a new chip. > > In the mean time, would it be possible to realize that the existing > > generic cpufreq driver simply isn't generic enough, and that we should > > accept machine-specific cpufreq drivers for the time being? > > That would work for me, to get things going for 3.18, but it's really > up to the cpufreq maintainers. Your platform_data based patch set > seems like a better solution to me than separate drivers, but I haven't > looked at the details. I personally don't really care: using platform_data, adding a machine-specific driver, having two platform_driver for cpufreq-dt with different names. Viresh ? Thomas
On 10 September 2014 15:11, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > The cpufreq-dt driver tries to get a regulator for each CPU. This > regulator is optional, but when not present, a scary message "failed > to get cpuX regulator" is displayed. Since the regulator is really > optional, we cannot sanely display any useful message, so this commit > simply gets rid of this problematic message. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > --- > drivers/cpufreq/cpufreq-dt.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c > index 35d396e..8060022 100644 > --- a/drivers/cpufreq/cpufreq-dt.c > +++ b/drivers/cpufreq/cpufreq-dt.c > @@ -149,9 +149,6 @@ try_again: > reg = reg_cpu; > goto try_again; > } > - > - dev_warn(cpu_dev, "failed to get cpu%d regulator: %ld\n", > - cpu, PTR_ERR(cpu_reg)); What about converting it to dev_dbg() instead? -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10 September 2014 15:11, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > And with Viresh not accepting any machine specific driver, it Its not that we aren't accepting any drivers at all, but if we can reuse existing infrastructure then we better use it for long term maintainability. But there is another view here which Mike objected to, sometime back. Its also not a good idea to push everything onto a virtual-cpu-clock driver.. I had something in mind to get that fixed, but this thread hasn't allowed me to work on that as it should be finished first. Probably we can add some callbacks to the cpufreq-cpu0 driver. For example if somebody wants to do something crazy enough in their ->target() routine then they can just register their callback and we will call it instead of cpufreq-dt's target() routine.. That way we can still reuse the common part.. > results in platforms like Armada XP having no solution to support > cpufreq... > > So this proposal consists in adding a platform_data flag for the > cpufreq-dt driver, which allows platform code to tell whether CPU > clocks are shared or are independent. > > If you don't like platform_data, we can also register two different > platform_driver for the two different cases, simply with different > names. > > Another approach would be to lift the ban on machine-specific cpufreq > drivers, since the generic driver is not capable of handling all > situations. The platform-data solution is better than any other here :), and that will get fixed once we get all this including which driver to probe from DT.. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10 September 2014 15:11, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h > index 138336b..fa35601 100644 > --- a/include/linux/cpufreq.h > +++ b/include/linux/cpufreq.h > @@ -218,7 +218,7 @@ __ATTR(_name, 0644, show_##_name, store_##_name) > > struct cpufreq_driver { > char name[CPUFREQ_NAME_LEN]; > - u8 flags; > + unsigned int flags; > > /* needed by all drivers */ > int (*init) (struct cpufreq_policy *policy); > @@ -308,10 +308,13 @@ struct cpufreq_driver { > */ > #define CPUFREQ_NEED_INITIAL_FREQ_CHECK (1 << 5) > > +#define CPUFREQ_DRIVER_FLAGS_MASK (0xFFFF0000) The flags field is used today to pass on information to cpufreq core and I believe it better stays that way only. Also all these changes might later be reverted and so we better don't change usage of variables.. Instead of this add a "void *driver_data" field in this structure and fill that with your structure. This can be later used for other purposes as well.. Also this will just add few more bytes to the cpufreq-driver structure which wouldn't have many instances in a compiled kernel, and so space isn't a problem.. Otherwise things looked good to me in your complete series.. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dear Viresh Kumar, On Wed, 10 Sep 2014 16:08:51 +0530, Viresh Kumar wrote: > Its not that we aren't accepting any drivers at all, but if we can reuse > existing infrastructure then we better use it for long term maintainability. Sure, but when "existing infrastructure" isn't ready to support a particular use case, and there seems to be no solution on which people are agreeing? > But there is another view here which Mike objected to, sometime back. > Its also not a good idea to push everything onto a virtual-cpu-clock > driver.. > > I had something in mind to get that fixed, but this thread hasn't allowed > me to work on that as it should be finished first. > > Probably we can add some callbacks to the cpufreq-cpu0 driver. For > example if somebody wants to do something crazy enough in their > ->target() routine then they can just register their callback and we > will call it instead of cpufreq-dt's target() routine.. > > That way we can still reuse the common part.. I personally don't think it would be a very good idea: in this case, you'd better do a complete cpufreq driver on its own. Doing subsystems in subsystems just for the sake of factorizing 10 lines of initialization seems a bit silly to me. > The platform-data solution is better than any other here :), and that > will get fixed once we get all this including which driver to probe from > DT.. Ok, I'll respin with a different solution than the flags. Thanks, Thomas
Hello, Here is the v2 of the platform_data based proposal to make cpufreq-dt understand when it has a shared clock by all CPUs vs. independent clock for each CPU. Changes since v1: - Use a 'void *driver_data' in cpufreq_driver instead of extending the existing flags field. Suggested by Viresh. - Keep the regulator related warning in cpufreq-dt, but reword it, and move it to the dev_dbg() level instead of dev_warn(). Thanks, Thomas Thomas Petazzoni (4): cpufreq: allow driver-specific data cpufreq: cpufreq-dt: extend with platform_data ARM: mvebu: use the cpufreq-dt platform_data for independent clocks cpufreq: cpufreq-dt: adjust message related to regulators arch/arm/mach-mvebu/pmsu.c | 8 +++++++- drivers/cpufreq/cpufreq-dt.c | 21 +++++++++++++++++---- drivers/cpufreq/cpufreq.c | 15 +++++++++++++++ include/linux/cpufreq-dt.h | 22 ++++++++++++++++++++++ include/linux/cpufreq.h | 2 ++ 5 files changed, 63 insertions(+), 5 deletions(-) create mode 100644 include/linux/cpufreq-dt.h
On 10 September 2014 17:38, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> + dt_cpufreq_driver.driver_data = dev_get_platdata(&pdev->dev);
I don't think this is right. What if platform device's platform data
is freed later?
That's why its always better to duplicate that structure instead of playing with
pointers.
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10 September 2014 17:38, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > The cpufreq-dt driver tries to get a regulator for each CPU. This > regulator is optional, but when not present, a scary message "failed > to get cpuX regulator" is displayed. To solve this, we reduce the > severity of the message from dev_warn() to dev_dbg() and we reword the > message to not be as scary. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > --- > drivers/cpufreq/cpufreq-dt.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c > index 987bbfa..b14628f 100644 > --- a/drivers/cpufreq/cpufreq-dt.c > +++ b/drivers/cpufreq/cpufreq-dt.c > @@ -147,8 +147,8 @@ try_again: > goto try_again; > } > > - dev_warn(cpu_dev, "failed to get cpu%d regulator: %ld\n", > - cpu, PTR_ERR(cpu_reg)); > + dev_dbg(cpu_dev, "no regulator for cpu%d: %ld\n", > + cpu, PTR_ERR(cpu_reg)); > } > > cpu_clk = clk_get(cpu_dev, NULL); Acked-by: Viresh Kumar <viresh.kumar@linaro.org> -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dear Viresh Kumar, On Wed, 10 Sep 2014 17:52:59 +0530, Viresh Kumar wrote: > On 10 September 2014 17:38, Thomas Petazzoni > <thomas.petazzoni@free-electrons.com> wrote: > > + dt_cpufreq_driver.driver_data = dev_get_platdata(&pdev->dev); > > I don't think this is right. What if platform device's platform data > is freed later? > That's why its always better to duplicate that structure instead of playing with > pointers. Isn't the piece of code registering the platform_device supposed to make sure that platform_data doesn't disappear? At least, in PATCH 3/4, I'm using platform_device_register_data(), which does a kmemdup() of the custom data being passed before assigning the struct device->platform_data field. But if you like, I can add one more memory copy :) Thomas
On 10 September 2014 17:58, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Dear Viresh Kumar, > > On Wed, 10 Sep 2014 17:52:59 +0530, Viresh Kumar wrote: >> On 10 September 2014 17:38, Thomas Petazzoni >> <thomas.petazzoni@free-electrons.com> wrote: >> > + dt_cpufreq_driver.driver_data = dev_get_platdata(&pdev->dev); >> >> I don't think this is right. What if platform device's platform data >> is freed later? >> That's why its always better to duplicate that structure instead of playing with >> pointers. > > Isn't the piece of code registering the platform_device supposed to > make sure that platform_data doesn't disappear? At least, in PATCH 3/4, I don't know. I remember this from the days when I used to write individual drivers for SPEAr platform... Its been some time now that I have seen this :) > I'm using platform_device_register_data(), which does a kmemdup() of > the custom data being passed before assigning the struct > device->platform_data field. Atleast in your case it isn't required to copy anymore but this driver can be used by others which may not guarantee that.. > But if you like, I can add one more memory copy :) Its not what I like, as wasting memory isn't sensible at all.. But about what's the right thing to do to make this code un-breakable.. @Arnd: Any inputs? -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 10 September 2014 18:02:59 Viresh Kumar wrote: > > I'm using platform_device_register_data(), which does a kmemdup() of > > the custom data being passed before assigning the struct > > device->platform_data field. > > Atleast in your case it isn't required to copy anymore but this driver can be > used by others which may not guarantee that.. > > > But if you like, I can add one more memory copy > > Its not what I like, as wasting memory isn't sensible at all.. But about what's > the right thing to do to make this code un-breakable.. > > @Arnd: Any inputs? platform_device_register_data() is the right way to register a platform device if one needs to do that, and it will always copy the data, so that should be used, and the driver should rely on the data not getting freed under it. Statically defining platform_device instances is generally a bad idea, and we should not do that, but if someone does it is up to them to make sure the platform_data is not marked as __init. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10 September 2014 17:38, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Hello, > > Here is the v2 of the platform_data based proposal to make cpufreq-dt > understand when it has a shared clock by all CPUs vs. independent > clock for each CPU. > > Changes since v1: > > - Use a 'void *driver_data' in cpufreq_driver instead of extending > the existing flags field. Suggested by Viresh. > > - Keep the regulator related warning in cpufreq-dt, but reword it, > and move it to the dev_dbg() level instead of dev_warn(). > > Thanks, > > Thomas > > Thomas Petazzoni (4): > cpufreq: allow driver-specific data > cpufreq: cpufreq-dt: extend with platform_data > ARM: mvebu: use the cpufreq-dt platform_data for independent clocks > cpufreq: cpufreq-dt: adjust message related to regulators After Arnd's reply: Acked-by: Viresh Kumar <viresh.kumar@linaro.org> -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/10/14 02:53, Arnd Bergmann wrote: > > Most of us will be at LCU next week, so I'd suggest we solve this > problem using the 'lock everyone into one room without beer until > we come up with a working approach' method. > I'll be at connect Tuesday to mid-Thursday. I'd like to be included if such a discussion happens.
Hello, On Wed, 10 Sep 2014 14:08:24 +0200, Thomas Petazzoni wrote: > Thomas Petazzoni (4): > cpufreq: allow driver-specific data > cpufreq: cpufreq-dt: extend with platform_data > ARM: mvebu: use the cpufreq-dt platform_data for independent clocks > cpufreq: cpufreq-dt: adjust message related to regulators > > arch/arm/mach-mvebu/pmsu.c | 8 +++++++- > drivers/cpufreq/cpufreq-dt.c | 21 +++++++++++++++++---- > drivers/cpufreq/cpufreq.c | 15 +++++++++++++++ > include/linux/cpufreq-dt.h | 22 ++++++++++++++++++++++ > include/linux/cpufreq.h | 2 ++ > 5 files changed, 63 insertions(+), 5 deletions(-) > create mode 100644 include/linux/cpufreq-dt.h Any news on getting this merged for 3.18 ? Thanks, Thomas
Quoting Viresh Kumar (2014-09-10 03:49:25) > On 10 September 2014 15:11, Thomas Petazzoni > <thomas.petazzoni@free-electrons.com> wrote: > > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h > > index 138336b..fa35601 100644 > > --- a/include/linux/cpufreq.h > > +++ b/include/linux/cpufreq.h > > @@ -218,7 +218,7 @@ __ATTR(_name, 0644, show_##_name, store_##_name) > > > > struct cpufreq_driver { > > char name[CPUFREQ_NAME_LEN]; > > - u8 flags; > > + unsigned int flags; > > > > /* needed by all drivers */ > > int (*init) (struct cpufreq_policy *policy); > > @@ -308,10 +308,13 @@ struct cpufreq_driver { > > */ > > #define CPUFREQ_NEED_INITIAL_FREQ_CHECK (1 << 5) > > > > +#define CPUFREQ_DRIVER_FLAGS_MASK (0xFFFF0000) > > The flags field is used today to pass on information to cpufreq core and > I believe it better stays that way only. Also all these changes might later > be reverted and so we better don't change usage of variables.. I have another use case for exposing driver flags and I have created a similar patch to this locally in my git tree. Basically some cpufreq drivers may block or sleep during their .set_target callback, and others will not. E.g. the former case is the common pattern to use the clock framework and the regulator framework, both of which hold mutexes and may have slow operations and the latter case is more like the Intel P-state driver with just some register writes. For the energy-aware scheduling work it is desirable to know if the cpufreq driver can do a "fast" transition of it might sleep, since that affects whether we adjust the cpu frequency from the scheduler context of whether need to put new work on the workqueue (e.g. waking up a kthread dedicated to calling .set_target). Regards, Mike > > Instead of this add a "void *driver_data" field in this structure and fill that > with your structure. This can be later used for other purposes as well.. > > Also this will just add few more bytes to the cpufreq-driver structure which > wouldn't have many instances in a compiled kernel, and so space isn't a > problem.. > > Otherwise things looked good to me in your complete series.. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 27 September 2014 15:44, Mike Turquette <mike.turquette@linaro.org> wrote: > I have another use case for exposing driver flags and I have created a > similar patch to this locally in my git tree. > > Basically some cpufreq drivers may block or sleep during their > .set_target callback, and others will not. E.g. the former case is the > common pattern to use the clock framework and the regulator framework, > both of which hold mutexes and may have slow operations and the latter > case is more like the Intel P-state driver with just some register > writes. > > For the energy-aware scheduling work it is desirable to know if the > cpufreq driver can do a "fast" transition of it might sleep, since that > affects whether we adjust the cpu frequency from the scheduler context > of whether need to put new work on the workqueue (e.g. waking up a > kthread dedicated to calling .set_target). I wasn't against exposing the flags but using the same variable to store driver specific data. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5 October 2014 06:11, Mike Turquette <mike.turquette@linaro.org> wrote: > For reference, here is a patch that I've had locally in my tree for a > couples months. I'm using it while prototyping some ways to integrate > CPUfreq with the scheduler. It is almost identical to Thomas's approach > but it doesn't try to mask off some flags. Additionally it does NOT > change the u8 return type, which is currently how struct > cpufreq_driver->flags is defined today. This is probably not good and > should be an unsigned int or unsigned long so that the api is a bit more > forward-looking. Probably yes. > From 0053dfc6b09fa0c5923b7dcba23b67c1c005cb88 Mon Sep 17 00:00:00 2001 > From: Mike Turquette <mturquette@linaro.org> > Date: Sun, 3 Aug 2014 21:25:38 -0700 > Subject: [PATCH] cpufreq: new function to query driver for flags > > Signed-off-by: Mike Turquette <mturquette@linaro.org> > --- > drivers/cpufreq/cpufreq.c | 9 +++++++++ > include/linux/cpufreq.h | 1 + > 2 files changed, 10 insertions(+) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 1abb44d..515e905 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -2003,6 +2003,15 @@ int cpufreq_driver_target(struct cpufreq_policy *policy, > } > EXPORT_SYMBOL_GPL(cpufreq_driver_target); > > +u8 cpufreq_driver_get_flags(void) > +{ > + if (!cpufreq_driver) > + return 0; Probably you need to return some error here. How would the caller know if there is no cpufreq-driver there.. > + return cpufreq_driver->flags; > +} > +EXPORT_SYMBOL_GPL(cpufreq_driver_get_flags); > + > /* > * when "event" is CPUFREQ_GOV_LIMITS > */ > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h > index 221f0eb..5a97f5f 100644 > --- a/include/linux/cpufreq.h > +++ b/include/linux/cpufreq.h > @@ -417,6 +417,7 @@ int cpufreq_driver_target(struct cpufreq_policy *policy, > int __cpufreq_driver_target(struct cpufreq_policy *policy, > unsigned int target_freq, > unsigned int relation); > +u8 cpufreq_driver_get_flags(void); > int cpufreq_register_governor(struct cpufreq_governor *governor); > void cpufreq_unregister_governor(struct cpufreq_governor *governor); > > -- > 1.8.3.2 > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Wed, 10 Sep 2014 14:08:24 +0200, Thomas Petazzoni wrote: > Thomas Petazzoni (4): > cpufreq: allow driver-specific data > cpufreq: cpufreq-dt: extend with platform_data > ARM: mvebu: use the cpufreq-dt platform_data for independent clocks > cpufreq: cpufreq-dt: adjust message related to regulators > > arch/arm/mach-mvebu/pmsu.c | 8 +++++++- > drivers/cpufreq/cpufreq-dt.c | 21 +++++++++++++++++---- > drivers/cpufreq/cpufreq.c | 15 +++++++++++++++ > include/linux/cpufreq-dt.h | 22 ++++++++++++++++++++++ > include/linux/cpufreq.h | 2 ++ > 5 files changed, 63 insertions(+), 5 deletions(-) > create mode 100644 include/linux/cpufreq-dt.h Since this series was Acked-by Viresh, would it be possible to get it merged for 3.18 ? Thanks, Thomas
Dear Mike Turquette, On Mon, 06 Oct 2014 11:36:24 -0700, Mike Turquette wrote: > Thomas, > > Does such a solution solve your problem? Do you still need to create a > platform_device if you can easily get at the driver flags with this > function? Hum, I'm not sure to get the question: of course to instantiate cpufreq I need to create a platform_device. The only difference between my v1 and v2 is that in v1 I was using those driver flags to pass from the cpufreq ->probe() function to the cpufreq ->init() function that we have independent clocks, while in v2, I'm using a new void *driver_data field. Really, it doesn't change anything and is purely a matter of taste. Best regards, Thomas
On Mon, Oct 6, 2014 at 11:55 AM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Dear Mike Turquette, > > On Mon, 06 Oct 2014 11:36:24 -0700, Mike Turquette wrote: > >> Thomas, >> >> Does such a solution solve your problem? Do you still need to create a >> platform_device if you can easily get at the driver flags with this >> function? > > Hum, I'm not sure to get the question: of course to instantiate cpufreq > I need to create a platform_device. The only difference between my v1 > and v2 is that in v1 I was using those driver flags to pass from the > cpufreq ->probe() function to the cpufreq ->init() function that we > have independent clocks, while in v2, I'm using a new void *driver_data > field. Really, it doesn't change anything and is purely a matter of > taste. Poorly worded question. I was asking if the above patch would work for you and it seems the answer is yes. The reason I care is that I am working on a governor that needs to know a driver flag. It would be painful for the machine-independent governor to understand a machine-specific private data structure from the driver. The flags are listed in cpufreq.h so it makes life easy. Regards, Mike > > Best regards, > > Thomas > -- > Thomas Petazzoni, CTO, Free Electrons > Embedded Linux, Kernel and Android engineering > http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, October 06, 2014 09:19:05 AM Thomas Petazzoni wrote: > Hello, > > On Wed, 10 Sep 2014 14:08:24 +0200, Thomas Petazzoni wrote: > > > Thomas Petazzoni (4): > > cpufreq: allow driver-specific data > > cpufreq: cpufreq-dt: extend with platform_data > > ARM: mvebu: use the cpufreq-dt platform_data for independent clocks > > cpufreq: cpufreq-dt: adjust message related to regulators > > > > arch/arm/mach-mvebu/pmsu.c | 8 +++++++- > > drivers/cpufreq/cpufreq-dt.c | 21 +++++++++++++++++---- > > drivers/cpufreq/cpufreq.c | 15 +++++++++++++++ > > include/linux/cpufreq-dt.h | 22 ++++++++++++++++++++++ > > include/linux/cpufreq.h | 2 ++ > > 5 files changed, 63 insertions(+), 5 deletions(-) > > create mode 100644 include/linux/cpufreq-dt.h > > Since this series was Acked-by Viresh, would it be possible to get it > merged for 3.18 ? I'm still seeing discussion going on about patch [1/4], though. Is that irrelevant? Also I'd prefer Viresh to tell me that this is 3.18 material instead of sending ACKs in random replies and hoping that I'll notice them. I may not.
On 7 October 2014 00:06, Mike Turquette <mike.turquette@linaro.org> wrote: > If I change the return type to int and replace 'return 0' with 'return > -ENODEV', would you accep this patch? Ofcourse :) > Does such a solution solve your problem? Do you still need to create a > platform_device if you can easily get at the driver flags with this > function? Will reply to this on the last mail.. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7 October 2014 03:14, Mike Turquette <mturquette@linaro.org> wrote: > Poorly worded question. I was asking if the above patch would work for > you and it seems the answer is yes. > > The reason I care is that I am working on a governor that needs to > know a driver flag. It would be painful for the machine-independent > governor to understand a machine-specific private data structure from > the driver. The flags are listed in cpufreq.h so it makes life easy. This patch isn't required anymore by thomas. All he needs is a flag local to cpufreq-cpu0 driver and we didn't wanted to abuse cpufreq_driver's flag field for that.. So this patch is just for your work.. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7 October 2014 04:23, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > I'm still seeing discussion going on about patch [1/4], though. That's V1's 1/4 and we finalized V2.. > Is that irrelevant? No. > Also I'd prefer Viresh to tell me that this is 3.18 material instead of > sending ACKs in random replies and hoping that I'll notice them. I may > not. Hmm, so yeah its for 3.18.. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, October 07, 2014 09:00:16 AM Viresh Kumar wrote: > On 7 October 2014 04:23, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > I'm still seeing discussion going on about patch [1/4], though. > > That's V1's 1/4 and we finalized V2.. > > > Is that irrelevant? > > No. > > > Also I'd prefer Viresh to tell me that this is 3.18 material instead of > > sending ACKs in random replies and hoping that I'll notice them. I may > > not. > > Hmm, so yeah its for 3.18.. OK So can you please send me links to the patches? I probably can find them, but honestly I have too little time for other stuff already.
On 8 October 2014 05:29, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > So can you please send me links to the patches? I probably can find them, > but honestly I have too little time for other stuff already. Sure: https://patchwork.kernel.org/patch/4876641/ https://patchwork.kernel.org/patch/4876651/ https://patchwork.kernel.org/patch/4876661/ https://patchwork.kernel.org/patch/4876671/ -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
This series is partially in response to a discussion around DT bindings for CPUfreq drivers [0], but it is also needed for on-going work to integrate CPUfreq with the scheduler. In particular a scheduler-driven cpu frequency scaling policy would be well served to know if the underlying CPUfreq driver .target callback might sleep or block for a long time. [0] http://lkml.kernel.org/r/<fcb88cd21f31a467d2d49911c2505082837f72ea.1410323179.git.viresh.kumar@linaro.org> Mike Turquette (2): cpufreq: add driver flag for sleepable transitions cpufreq: new function to query driver for flags drivers/cpufreq/cpufreq.c | 9 +++++++++ include/linux/cpufreq.h | 13 +++++++++++++ 2 files changed, 22 insertions(+)
On 8 October 2014 13:18, Mike Turquette <mturquette@linaro.org> wrote: > This series is partially in response to a discussion around DT bindings > for CPUfreq drivers [0], but it is also needed for on-going work to > integrate CPUfreq with the scheduler. In particular a scheduler-driven > cpu frequency scaling policy would be well served to know if the > underlying CPUfreq driver .target callback might sleep or block for a > long time. > > [0] http://lkml.kernel.org/r/<fcb88cd21f31a467d2d49911c2505082837f72ea.1410323179.git.viresh.kumar@linaro.org> Firstly this link is broken and then the last comment I gave in Thomas's thread was that he doesn't need this routine anymore. And so your scheduler work is the only user of it.. And so probably we should get this included with the scheduler patches only, isn't it? Just including them without any user wouldn't benefit.. Or am I missing something ? -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dear Viresh Kumar, On Wed, 8 Oct 2014 13:24:30 +0530, Viresh Kumar wrote: > On 8 October 2014 13:18, Mike Turquette <mturquette@linaro.org> wrote: > > This series is partially in response to a discussion around DT bindings > > for CPUfreq drivers [0], but it is also needed for on-going work to > > integrate CPUfreq with the scheduler. In particular a scheduler-driven > > cpu frequency scaling policy would be well served to know if the > > underlying CPUfreq driver .target callback might sleep or block for a > > long time. > > > > [0] http://lkml.kernel.org/r/<fcb88cd21f31a467d2d49911c2505082837f72ea.1410323179.git.viresh.kumar@linaro.org> > > Firstly this link is broken and then the last comment I gave in Thomas's > thread was that he doesn't need this routine anymore. And so your scheduler > work is the only user of it.. And so probably we should get this included with > the scheduler patches only, isn't it? Just including them without any user > wouldn't benefit.. > > Or am I missing something ? Well, when one has to merge a large number of changes, we often recommend to merge them piece by piece, which is what Mike is trying to do here. So we cannot at the same time ask developers to merge things in small pieces that are easy to review and to merge everything together because the users of a given API are not there yet. Thomas
On 8 October 2014 13:41, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > On Wed, 8 Oct 2014 13:24:30 +0530, Viresh Kumar wrote: >> On 8 October 2014 13:18, Mike Turquette <mturquette@linaro.org> wrote: >> > This series is partially in response to a discussion around DT bindings >> > for CPUfreq drivers [0], but it is also needed for on-going work to >> > integrate CPUfreq with the scheduler. In particular a scheduler-driven > Well, when one has to merge a large number of changes, we often > recommend to merge them piece by piece, which is what Mike is trying to > do here. So we cannot at the same time ask developers to merge things > in small pieces that are easy to review and to merge everything > together because the users of a given API are not there yet. From the wording of Mike it looks like: - This is required by cpufreq drivers - today - And this will also be useful for scheduler The first point doesn't stand true anymore. Lets wait for Mike's reply and see his opinion. And then the patches are so small and there are no objections against them. I don't think getting them with the scheduler changes is that bad of an idea. In worst case, what if he has to redesign his idea? The new routines will stay without a caller then :) -- viresh -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Viresh Kumar (2014-10-08 01:19:40) > On 8 October 2014 13:41, Thomas Petazzoni > <thomas.petazzoni@free-electrons.com> wrote: > > On Wed, 8 Oct 2014 13:24:30 +0530, Viresh Kumar wrote: > >> On 8 October 2014 13:18, Mike Turquette <mturquette@linaro.org> wrote: > > >> > This series is partially in response to a discussion around DT bindings > >> > for CPUfreq drivers [0], but it is also needed for on-going work to > >> > integrate CPUfreq with the scheduler. In particular a scheduler-driven > > > Well, when one has to merge a large number of changes, we often > > recommend to merge them piece by piece, which is what Mike is trying to > > do here. So we cannot at the same time ask developers to merge things > > in small pieces that are easy to review and to merge everything > > together because the users of a given API are not there yet. > > From the wording of Mike it looks like: > - This is required by cpufreq drivers - today > - And this will also be useful for scheduler Hi Viresh and Thomas, Apologies if my wording was confusing. Without getting into a grammar war, I did say that these patches were "in response" to this thread (entirely accurate) and only necessary for the "on-going work" I'm doing with the scheduler. Sorry if any of that came across as me stating that these patches were necessary to solve Thomas' problem. > > The first point doesn't stand true anymore. Lets wait for Mike's reply and > see his opinion. > > And then the patches are so small and there are no objections against > them. I don't think getting them with the scheduler changes is that bad > of an idea. In worst case, what if he has to redesign his idea? The new > routines will stay without a caller then :) Whether you merge the patches now or later is fine by me. I prefer to get my patches out early and often so I can avoid any surprises later on. If you have a fundamental objection to these patches then please let me know. Otherwise it would be wonderful to have an Ack. Thanks! Mike > > -- > viresh -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 9 October 2014 05:31, Mike Turquette <mturquette@linaro.org> wrote: > Whether you merge the patches now or later is fine by me. I prefer to > get my patches out early and often so I can avoid any surprises later > on. If you have a fundamental objection to these patches then please let > me know. Otherwise it would be wonderful to have an Ack. Acked-by: Viresh Kumar <viresh.kumar@linaro.org> But I would still ask you to get these merged with scheduler changes. :) -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, October 08, 2014 11:21:42 AM Viresh Kumar wrote: > On 8 October 2014 05:29, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > So can you please send me links to the patches? I probably can find them, > > but honestly I have too little time for other stuff already. > > Sure: > > https://patchwork.kernel.org/patch/4876641/ > https://patchwork.kernel.org/patch/4876651/ > https://patchwork.kernel.org/patch/4876661/ > https://patchwork.kernel.org/patch/4876671/ They didn't apply cleanly for me and I didn't have the time to rebase them myself. Thomas, care to rebase the v2 on top of the current Linus' tree and resend?
Dear Rafael J. Wysocki, On Sun, 12 Oct 2014 22:25:02 +0200, Rafael J. Wysocki wrote: > Thomas, care to rebase the v2 on top of the current Linus' tree and > resend? I've just resent the patches. See: https://patchwork.kernel.org/patch/5100691/ https://patchwork.kernel.org/patch/5100731/ https://patchwork.kernel.org/patch/5100711/ There are only 3 patches in this version, compared to four in the previous version, because one of the patch touches arch/arm/mach-mvebu/, and should therefore go through the mvebu maintainers. Thanks, Thomas
diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig index 3489f8f..a755227 100644 --- a/drivers/cpufreq/Kconfig +++ b/drivers/cpufreq/Kconfig @@ -196,6 +196,15 @@ config CPUFREQ_DT If in doubt, say N. +config CPUFREQ_DT_INDEPENDENT_CLOCKS + bool "CPUs change clocks independently" + depends on CPUFREQ_DT + help + Selecting this will make cpufreq-dt driver believe that all CPUs have + independent clock lines and so all will have separate 'struct + cpufreq_policy' instances for them. This must be replaced by proper DT + bindings later on. + menu "x86 CPU frequency scaling drivers" depends on X86 source "drivers/cpufreq/Kconfig.x86" diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index e002650..dbb6f22 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -266,9 +266,22 @@ static int cpufreq_init(struct cpufreq_policy *policy) policy->driver_data = priv; policy->clk = cpu_clk; - ret = cpufreq_generic_init(policy, freq_table, transition_latency); - if (ret) + 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_cooling_unregister; + } + + policy->cpuinfo.transition_latency = transition_latency; + +#ifndef CONFIG_CPUFREQ_DT_INDEPENDENT_CLOCKS + /* + * The driver only supports the SMP configuartion where all processors + * share the clock and voltage and clock. + */ + cpumask_setall(policy->cpus); +#endif return 0;
There has been lots of discussion over mailing lists about how to get CPU's clock sharing information for platforms as CPUFreq core depends on that. It doesn't look that there will be any immediate solution to that as there are multiple views over how to get that from DT. But there are platforms (already upstreamed) which depend on this requirement and are waiting for a solution to sail through. The common thing about them (Krait and Mvebu) is that all CPUs have independent clock lines and that makes things easier for us. So, this patch creates another Kconfig option to indicate if all CPUs share same clock line or all have independent. The complex cases of multiple clusters having separate clock lines, but having same for all CPUs within a cluster isn't addressed yet. This is a *short-term* solution for getting these platforms up an running and a DT based solution should be upstreamed soon. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- Yesterday again I was pinged by Thomas about the status of this work and the thread isn't going anywhere. He is blocked with just this to get his platform running on mainline. Will it be possible to get this through as a temporary solution? drivers/cpufreq/Kconfig | 9 +++++++++ drivers/cpufreq/cpufreq-dt.c | 17 +++++++++++++++-- 2 files changed, 24 insertions(+), 2 deletions(-)