| Message ID | 1410342112-13264-2-git-send-email-thomas.petazzoni@free-electrons.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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-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-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-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 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
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-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 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
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-arm-msm" 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-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 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-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index d9fdedd..e41b971 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1728,6 +1728,21 @@ const char *cpufreq_get_current_driver(void) } EXPORT_SYMBOL_GPL(cpufreq_get_current_driver); +/** + * cpufreq_get_driver_flags - return current driver's flags + * + * Return the flags of the currently loaded cpufreq driver, or 0 + * if none. + */ +unsigned int cpufreq_get_driver_flags(void) +{ + if (cpufreq_driver) + return cpufreq_driver->flags & CPUFREQ_DRIVER_FLAGS_MASK; + + return 0; +} +EXPORT_SYMBOL_GPL(cpufreq_get_driver_flags); + /********************************************************************* * NOTIFIER LISTS INTERFACE * *********************************************************************/ 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) + int cpufreq_register_driver(struct cpufreq_driver *driver_data); int cpufreq_unregister_driver(struct cpufreq_driver *driver_data); const char *cpufreq_get_current_driver(void); +unsigned int cpufreq_get_driver_flags(void); static inline void cpufreq_verify_within_limits(struct cpufreq_policy *policy, unsigned int min, unsigned int max)
Exactly like the cpuidle subsystem separates core cpuidle flags from driver-specific cpuidle flags, this commit extends the cpufreq subsystem with the same idea: the cpufreq_driver->flags field contain the existing core cpufreq flags in the low 16 bits, and driver-specific flags in the high 16 bits. A new function called cpufreq_get_driver_flags() is added to allow a cpufreq driver to retrieve those flags, since they are typically needed from a cpufreq_policy->init() callback, which does not have access to the cpufreq_driver structure. This function call is similar to the existing cpufreq_get_current_driver() function call. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- drivers/cpufreq/cpufreq.c | 15 +++++++++++++++ include/linux/cpufreq.h | 5 ++++- 2 files changed, 19 insertions(+), 1 deletion(-)