Re: [PATCH 2/5] cpufreq:boost: Add support for software based CPU frequency boost
On 7 June 2013 20:04, Lukasz Majewski wrote: > After refactoring the code. I admit, that we shall embed the struct > cpu_boost fields directly to the coufreq_driver. There is no point to > create structure with 2 fields. Great!! I will wait for your next version. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/5] cpufreq:boost: Add support for software based CPU frequency boost
Hi Viresh,
> On 6 June 2013 17:19, Lukasz Majewski wrote:
> >> On 6 June 2013 12:37, Lukasz Majewski
> >> wrote:
>
> >> > cpufreq_driver->boost->attr))
> >>
> >> Why is this required? Why do we want platforms to add some files
> >> in sysfs?
> >
> > There are two kinds of attributes, which are exported by boost:
> >
> > 1. global boost (/sys/devices/system/cpu/cpufreq/boost)
> >
> > 2. attributes describing cpufreq abilities when boost is available
> > (/sys/devices/syste/cpu/cpu0/cpufreq/):
> > - scaling_boost_frequencies - which will show over clocked
> > frequencies
> > - the scaling_available_frequencies will also display
> > boosted frequency (when boost enabled)
> >
> > Information from 2. is cpufreq_driver dependent. And that
> > information shouldn't been displayed when boost is not available
>
> This is not how I wanted this to be coded. Lets keep things simple:
> - Implement it in the way cpufreq_freq_attr_scaling_available_freqs
> is implemented. And then drivers which need to see boost freqs
> can add it in their attr.
I've added scaling_boost_frequencies to cpufreq_driver attr.
However, I would keep the boost attributes definition in the freq_table
file (as I've proposed in my patch).
>
> >> > + policy->boost = cpufreq_boost = cpufreq_driver->boost;
> >>
> >> Why are we copying same pointer to policy->boost? Driver is
> >> passing pointer to a single memory location, just save it globally.
> >
> > This needs some explanation.
> >
> > The sysfs entry presented at [1] doesn't bring any useful
> > information to reuse (like *policy). For this reason the global
> > cpufreq_boost pointer is needed.
> >
> > However to efficiently manage the boost, it is necessary to keep per
> > policy pointer to the only struct cpufreq_boost instance (defined at
> > cpufreq_driver code).
>
> No we don't need to screw struct cpufreq_policy with it.
> Just need two variables here:
> - cpufreq_driver->boost: If driver supports boost or not.
This will be done as above.
> - If above is true then a global bool variable that will say boost is
> enabled from sysfs or not.
One global flag will be defined at cpufreq.c to indicate if global
boost sysfs attr has been created.
>
> >> > + /* disable boost for newly created policy - as we e.g.
> >> > change
> >> > + governor */
> >> > + policy->boost->status = CPUFREQ_BOOST_DIS;
> >>
> >> Drivers supporting boost may want boost to be enabled by default,
> >> maybe without any sysfs calls.
> >
> > This can be done by setting the struct cpufreq_boost status field to
> > CPUFREQ_BOOST_EN at cpufreq driver code (when boost structure is
> > defined)
>
> This really isn't driver dependent.. But user dependent. Maybe lets
> keep it disabled and people can enable it from sysfs.
The cpufreq_driver struct will have boost_en field. This will allow
keep boost state (it is similar to global boost_enable at
acpi-cpufreq.c).
>
> >> Why do we need to maintain a list of boost here? notifiers?
> >> complex :(
> >
> > Notifier is needed to disable boost when policy is changed (for
> > example when we change from ondemand/lab to performance governor).
> >
> > I wanted to avoid the situation when boost stays enabled across
> > different governors.
> >
> > The list of in system available policies is defined to allow boost
> > enable/disable for all policies available (by changing for example
> > policy->max field).
> >
> > If we decide, that we will support only one policy (as it is now at
> > e.g. Exynos), the list is unnecessary here.
>
> What we discussed last in your earlier patchset was:
> - Keep boost feature separate from governors.
Ok.
> - If it is enabled, then any governor can use it (if they want).
Ok, lets do it in this way
> - Lets not disable it if governor is changed. user must do it
> explicitly.
Ok, agree (notifier removed).
>
> >> > +static int cpufreq_frequency_table_skip_boost(struct
> >> > cpufreq_policy *policy,
> >> > + unsigned int index)
> >> > +{
> >> > + if (index == CPUFREQ_BOOST)
> >> > + if (!policy->boost ||
> >>
> >> This shouldn't be true. If index has got CPUFREQ_BOOST, then driver
> >> has to support boost.
> >
> > Correct me if I'm wrong here, but in my understanding the boost
> > shall be only supported when both CPUFREQ_BOOST index is defined in
> > a freq_table and boost.state = CPUFREQ_BOOST_EN is set.
> >
> > Setting of CPUFREQ_BOOST shouldn't by default allow to use over
> > clocking frequency.
>
> For cpufreq core boost is enabled as soon as cpufreq_driver->boost is
> true. We can have additional checks to see if there is atleast one
> boost frequency but can skip this too.
Checks are needed to read max_normal frequency and max boost frequency
from frequency table.
In exynos cpufreq_driver->init() I will disable boost.
>
> >> why do we need this?
> >
> > To evaluate the maximal boost frequency from the
Re: [PATCH 2/5] cpufreq:boost: Add support for software based CPU frequency boost
Hi Viresh,
> Hi Lukasz,
>
> On 7 June 2013 18:57, Lukasz Majewski wrote:
>
> I hope you agreed to all the other comments I gave as I don't see an
> explicit reply below each of these. I have seen people missing these
> in past, so what would be better to do is:
> - either reply below each one of them and say yes or no..
> - Or write once below many comments and say: All above comments
> are accepted.
>
> So, that Reviewer is assured that you haven't missed anything.
>
Thanks for reminding :-).
I've read through all the comments. I'm redesigning now the driver to
remove redundant code.
> > I would prefer to have following fields in the cpufreq_boost
> > structure: struct cpufreq_boost {
> > unsigned intmax_boost_freq; /*boost max freq*/
> > unsigned intmax_normal_freq; /*max normal freq
> > int (*low_level_boost) (int state);
> > bool boost_en; /* indicate if boost is enabled */
> > }
> >
> > The max_{boost|normal}_freq fields will be filed at
> > ret = cpufreq_driver->init(policy);
> >
> > Thanks to them I will avoid calling many times routine, which
> > extracts from freq_table maximal boost and normal frequencies.
> >
> > I could define those variables in the exynos-cpufreq.c driver, but I
> > think, that they are more suitable to be embedded at cpufreq_boost
> > structure.
>
> I understand that you need these variables (I will still look how you
> are using them in next version). But they are per policy and driver
> isn't responsible for maintaining them. If they are required then
> cpufreq core must find them out and keep in struct cpufreq_policy (as
> they are policy dependent)..
>
> So, remove this structure from cpufreq_driver and embed variables
> directly.
After refactoring the code. I admit, that we shall embed the struct
cpu_boost fields directly to the coufreq_driver. There is no point to
create structure with 2 fields.
--
Best regards,
Lukasz Majewski
Samsung R Institute Poland (SRPOL) | Linux Platform Group
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/5] cpufreq:boost: Add support for software based CPU frequency boost
Hi Lukasz,
On 7 June 2013 18:57, Lukasz Majewski wrote:
I hope you agreed to all the other comments I gave as I don't see an
explicit reply below each of these. I have seen people missing these
in past, so what would be better to do is:
- either reply below each one of them and say yes or no..
- Or write once below many comments and say: All above comments
are accepted.
So, that Reviewer is assured that you haven't missed anything.
> I would prefer to have following fields in the cpufreq_boost structure:
> struct cpufreq_boost {
> unsigned intmax_boost_freq; /*boost max freq*/
> unsigned intmax_normal_freq; /*max normal freq
> int (*low_level_boost) (int state);
> bool boost_en; /* indicate if boost is enabled */
> }
>
> The max_{boost|normal}_freq fields will be filed at
> ret = cpufreq_driver->init(policy);
>
> Thanks to them I will avoid calling many times routine, which extracts
> from freq_table maximal boost and normal frequencies.
>
> I could define those variables in the exynos-cpufreq.c driver, but I
> think, that they are more suitable to be embedded at cpufreq_boost
> structure.
I understand that you need these variables (I will still look how you are
using them in next version). But they are per policy and driver isn't
responsible for maintaining them. If they are required then cpufreq
core must find them out and keep in struct cpufreq_policy (as they
are policy dependent)..
So, remove this structure from cpufreq_driver and embed variables
directly.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/5] cpufreq:boost: Add support for software based CPU frequency boost
Hi Viresh,
> On 6 June 2013 17:19, Lukasz Majewski wrote:
> >> On 6 June 2013 12:37, Lukasz Majewski
> >> wrote:
>
> >> > cpufreq_driver->boost->attr))
> >>
> >> Why is this required? Why do we want platforms to add some files
> >> in sysfs?
> >
> > There are two kinds of attributes, which are exported by boost:
> >
> > 1. global boost (/sys/devices/system/cpu/cpufreq/boost)
> >
> > 2. attributes describing cpufreq abilities when boost is available
> > (/sys/devices/syste/cpu/cpu0/cpufreq/):
> > - scaling_boost_frequencies - which will show over clocked
> > frequencies
> > - the scaling_available_frequencies will also display
> > boosted frequency (when boost enabled)
> >
> > Information from 2. is cpufreq_driver dependent. And that
> > information shouldn't been displayed when boost is not available
>
> This is not how I wanted this to be coded. Lets keep things simple:
> - Implement it in the way cpufreq_freq_attr_scaling_available_freqs
> is implemented. And then drivers which need to see boost freqs
> can add it in their attr.
>
> >> > + policy->boost = cpufreq_boost = cpufreq_driver->boost;
> >>
> >> Why are we copying same pointer to policy->boost? Driver is
> >> passing pointer to a single memory location, just save it globally.
> >
> > This needs some explanation.
> >
> > The sysfs entry presented at [1] doesn't bring any useful
> > information to reuse (like *policy). For this reason the global
> > cpufreq_boost pointer is needed.
> >
> > However to efficiently manage the boost, it is necessary to keep per
> > policy pointer to the only struct cpufreq_boost instance (defined at
> > cpufreq_driver code).
>
> No we don't need to screw struct cpufreq_policy with it.
> Just need two variables here:
> - cpufreq_driver->boost: If driver supports boost or not.
> - If above is true then a global bool variable that will say boost is
> enabled from sysfs or not.
>
> >> > + /* disable boost for newly created policy - as we e.g.
> >> > change
> >> > + governor */
> >> > + policy->boost->status = CPUFREQ_BOOST_DIS;
> >>
> >> Drivers supporting boost may want boost to be enabled by default,
> >> maybe without any sysfs calls.
> >
> > This can be done by setting the struct cpufreq_boost status field to
> > CPUFREQ_BOOST_EN at cpufreq driver code (when boost structure is
> > defined)
>
> This really isn't driver dependent.. But user dependent. Maybe lets
> keep it disabled and people can enable it from sysfs.
>
> >> Why do we need to maintain a list of boost here? notifiers?
> >> complex :(
> >
> > Notifier is needed to disable boost when policy is changed (for
> > example when we change from ondemand/lab to performance governor).
> >
> > I wanted to avoid the situation when boost stays enabled across
> > different governors.
> >
> > The list of in system available policies is defined to allow boost
> > enable/disable for all policies available (by changing for example
> > policy->max field).
> >
> > If we decide, that we will support only one policy (as it is now at
> > e.g. Exynos), the list is unnecessary here.
>
> What we discussed last in your earlier patchset was:
> - Keep boost feature separate from governors.
> - If it is enabled, then any governor can use it (if they want).
> - Lets not disable it if governor is changed. user must do it
> explicitly.
>
> >> > +static int cpufreq_frequency_table_skip_boost(struct
> >> > cpufreq_policy *policy,
> >> > + unsigned int index)
> >> > +{
> >> > + if (index == CPUFREQ_BOOST)
> >> > + if (!policy->boost ||
> >>
> >> This shouldn't be true. If index has got CPUFREQ_BOOST, then driver
> >> has to support boost.
> >
> > Correct me if I'm wrong here, but in my understanding the boost
> > shall be only supported when both CPUFREQ_BOOST index is defined in
> > a freq_table and boost.state = CPUFREQ_BOOST_EN is set.
> >
> > Setting of CPUFREQ_BOOST shouldn't by default allow to use over
> > clocking frequency.
>
> For cpufreq core boost is enabled as soon as cpufreq_driver->boost is
> true. We can have additional checks to see if there is atleast one
> boost frequency but can skip this too.
>
> >> why do we need this?
> >
> > To evaluate the maximal boost frequency from the frequency table.
> > It is then used as a delimiter (when LAB cooperates with thermal
> > framework).
>
> Introduce this with LAB then.. Lets keep it as simple as possible for
> now. One step at a time.
>
> >> > +struct cpufreq_boost {
> >> > + unsigned intmax_boost_freq; /* maximum value
> >> > of
> >> > +* boosted freq
> >> > */
> >> > + unsigned intmax_normal_freq; /* non boost max
> >> > freq */
> >> > + int status; /* status of boost */
> >> > +
> >> > + /* boost sysfs attributies */
> >> > + struct freq_attr**attr;
> >> > +
> >>
Re: [PATCH 2/5] cpufreq:boost: Add support for software based CPU frequency boost
Hi Lukasz,
On 7 June 2013 18:57, Lukasz Majewski l.majew...@samsung.com wrote:
I hope you agreed to all the other comments I gave as I don't see an
explicit reply below each of these. I have seen people missing these
in past, so what would be better to do is:
- either reply below each one of them and say yes or no..
- Or write once below many comments and say: All above comments
are accepted.
So, that Reviewer is assured that you haven't missed anything.
I would prefer to have following fields in the cpufreq_boost structure:
struct cpufreq_boost {
unsigned intmax_boost_freq; /*boost max freq*/
unsigned intmax_normal_freq; /*max normal freq
int (*low_level_boost) (int state);
bool boost_en; /* indicate if boost is enabled */
}
The max_{boost|normal}_freq fields will be filed at
ret = cpufreq_driver-init(policy);
Thanks to them I will avoid calling many times routine, which extracts
from freq_table maximal boost and normal frequencies.
I could define those variables in the exynos-cpufreq.c driver, but I
think, that they are more suitable to be embedded at cpufreq_boost
structure.
I understand that you need these variables (I will still look how you are
using them in next version). But they are per policy and driver isn't
responsible for maintaining them. If they are required then cpufreq
core must find them out and keep in struct cpufreq_policy (as they
are policy dependent)..
So, remove this structure from cpufreq_driver and embed variables
directly.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/5] cpufreq:boost: Add support for software based CPU frequency boost
Hi Viresh,
Hi Lukasz,
On 7 June 2013 18:57, Lukasz Majewski l.majew...@samsung.com wrote:
I hope you agreed to all the other comments I gave as I don't see an
explicit reply below each of these. I have seen people missing these
in past, so what would be better to do is:
- either reply below each one of them and say yes or no..
- Or write once below many comments and say: All above comments
are accepted.
So, that Reviewer is assured that you haven't missed anything.
Thanks for reminding :-).
I've read through all the comments. I'm redesigning now the driver to
remove redundant code.
I would prefer to have following fields in the cpufreq_boost
structure: struct cpufreq_boost {
unsigned intmax_boost_freq; /*boost max freq*/
unsigned intmax_normal_freq; /*max normal freq
int (*low_level_boost) (int state);
bool boost_en; /* indicate if boost is enabled */
}
The max_{boost|normal}_freq fields will be filed at
ret = cpufreq_driver-init(policy);
Thanks to them I will avoid calling many times routine, which
extracts from freq_table maximal boost and normal frequencies.
I could define those variables in the exynos-cpufreq.c driver, but I
think, that they are more suitable to be embedded at cpufreq_boost
structure.
I understand that you need these variables (I will still look how you
are using them in next version). But they are per policy and driver
isn't responsible for maintaining them. If they are required then
cpufreq core must find them out and keep in struct cpufreq_policy (as
they are policy dependent)..
So, remove this structure from cpufreq_driver and embed variables
directly.
After refactoring the code. I admit, that we shall embed the struct
cpu_boost fields directly to the coufreq_driver. There is no point to
create structure with 2 fields.
--
Best regards,
Lukasz Majewski
Samsung RD Institute Poland (SRPOL) | Linux Platform Group
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/5] cpufreq:boost: Add support for software based CPU frequency boost
Hi Viresh,
On 6 June 2013 17:19, Lukasz Majewski l.majew...@samsung.com wrote:
On 6 June 2013 12:37, Lukasz Majewski l.majew...@samsung.com
wrote:
cpufreq_driver-boost-attr))
Why is this required? Why do we want platforms to add some files
in sysfs?
There are two kinds of attributes, which are exported by boost:
1. global boost (/sys/devices/system/cpu/cpufreq/boost)
2. attributes describing cpufreq abilities when boost is available
(/sys/devices/syste/cpu/cpu0/cpufreq/):
- scaling_boost_frequencies - which will show over clocked
frequencies
- the scaling_available_frequencies will also display
boosted frequency (when boost enabled)
Information from 2. is cpufreq_driver dependent. And that
information shouldn't been displayed when boost is not available
This is not how I wanted this to be coded. Lets keep things simple:
- Implement it in the way cpufreq_freq_attr_scaling_available_freqs
is implemented. And then drivers which need to see boost freqs
can add it in their attr.
I've added scaling_boost_frequencies to cpufreq_driver attr.
However, I would keep the boost attributes definition in the freq_table
file (as I've proposed in my patch).
+ policy-boost = cpufreq_boost = cpufreq_driver-boost;
Why are we copying same pointer to policy-boost? Driver is
passing pointer to a single memory location, just save it globally.
This needs some explanation.
The sysfs entry presented at [1] doesn't bring any useful
information to reuse (like *policy). For this reason the global
cpufreq_boost pointer is needed.
However to efficiently manage the boost, it is necessary to keep per
policy pointer to the only struct cpufreq_boost instance (defined at
cpufreq_driver code).
No we don't need to screw struct cpufreq_policy with it.
Just need two variables here:
- cpufreq_driver-boost: If driver supports boost or not.
This will be done as above.
- If above is true then a global bool variable that will say boost is
enabled from sysfs or not.
One global flag will be defined at cpufreq.c to indicate if global
boost sysfs attr has been created.
+ /* disable boost for newly created policy - as we e.g.
change
+ governor */
+ policy-boost-status = CPUFREQ_BOOST_DIS;
Drivers supporting boost may want boost to be enabled by default,
maybe without any sysfs calls.
This can be done by setting the struct cpufreq_boost status field to
CPUFREQ_BOOST_EN at cpufreq driver code (when boost structure is
defined)
This really isn't driver dependent.. But user dependent. Maybe lets
keep it disabled and people can enable it from sysfs.
The cpufreq_driver struct will have boost_en field. This will allow
keep boost state (it is similar to global boost_enable at
acpi-cpufreq.c).
Why do we need to maintain a list of boost here? notifiers?
complex :(
Notifier is needed to disable boost when policy is changed (for
example when we change from ondemand/lab to performance governor).
I wanted to avoid the situation when boost stays enabled across
different governors.
The list of in system available policies is defined to allow boost
enable/disable for all policies available (by changing for example
policy-max field).
If we decide, that we will support only one policy (as it is now at
e.g. Exynos), the list is unnecessary here.
What we discussed last in your earlier patchset was:
- Keep boost feature separate from governors.
Ok.
- If it is enabled, then any governor can use it (if they want).
Ok, lets do it in this way
- Lets not disable it if governor is changed. user must do it
explicitly.
Ok, agree (notifier removed).
+static int cpufreq_frequency_table_skip_boost(struct
cpufreq_policy *policy,
+ unsigned int index)
+{
+ if (index == CPUFREQ_BOOST)
+ if (!policy-boost ||
This shouldn't be true. If index has got CPUFREQ_BOOST, then driver
has to support boost.
Correct me if I'm wrong here, but in my understanding the boost
shall be only supported when both CPUFREQ_BOOST index is defined in
a freq_table and boost.state = CPUFREQ_BOOST_EN is set.
Setting of CPUFREQ_BOOST shouldn't by default allow to use over
clocking frequency.
For cpufreq core boost is enabled as soon as cpufreq_driver-boost is
true. We can have additional checks to see if there is atleast one
boost frequency but can skip this too.
Checks are needed to read max_normal frequency and max boost frequency
from frequency table.
In exynos cpufreq_driver-init() I will disable boost.
why do we need this?
To evaluate the maximal boost frequency from the frequency table.
It is then used as a delimiter (when LAB cooperates with thermal
framework).
Introduce this with LAB then.. Lets keep it as simple as possible for
now. One step at a time.
Re: [PATCH 2/5] cpufreq:boost: Add support for software based CPU frequency boost
On 7 June 2013 20:04, Lukasz Majewski l.majew...@samsung.com wrote: After refactoring the code. I admit, that we shall embed the struct cpu_boost fields directly to the coufreq_driver. There is no point to create structure with 2 fields. Great!! I will wait for your next version. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/5] cpufreq:boost: Add support for software based CPU frequency boost
Hi Viresh,
On 6 June 2013 17:19, Lukasz Majewski l.majew...@samsung.com wrote:
On 6 June 2013 12:37, Lukasz Majewski l.majew...@samsung.com
wrote:
cpufreq_driver-boost-attr))
Why is this required? Why do we want platforms to add some files
in sysfs?
There are two kinds of attributes, which are exported by boost:
1. global boost (/sys/devices/system/cpu/cpufreq/boost)
2. attributes describing cpufreq abilities when boost is available
(/sys/devices/syste/cpu/cpu0/cpufreq/):
- scaling_boost_frequencies - which will show over clocked
frequencies
- the scaling_available_frequencies will also display
boosted frequency (when boost enabled)
Information from 2. is cpufreq_driver dependent. And that
information shouldn't been displayed when boost is not available
This is not how I wanted this to be coded. Lets keep things simple:
- Implement it in the way cpufreq_freq_attr_scaling_available_freqs
is implemented. And then drivers which need to see boost freqs
can add it in their attr.
+ policy-boost = cpufreq_boost = cpufreq_driver-boost;
Why are we copying same pointer to policy-boost? Driver is
passing pointer to a single memory location, just save it globally.
This needs some explanation.
The sysfs entry presented at [1] doesn't bring any useful
information to reuse (like *policy). For this reason the global
cpufreq_boost pointer is needed.
However to efficiently manage the boost, it is necessary to keep per
policy pointer to the only struct cpufreq_boost instance (defined at
cpufreq_driver code).
No we don't need to screw struct cpufreq_policy with it.
Just need two variables here:
- cpufreq_driver-boost: If driver supports boost or not.
- If above is true then a global bool variable that will say boost is
enabled from sysfs or not.
+ /* disable boost for newly created policy - as we e.g.
change
+ governor */
+ policy-boost-status = CPUFREQ_BOOST_DIS;
Drivers supporting boost may want boost to be enabled by default,
maybe without any sysfs calls.
This can be done by setting the struct cpufreq_boost status field to
CPUFREQ_BOOST_EN at cpufreq driver code (when boost structure is
defined)
This really isn't driver dependent.. But user dependent. Maybe lets
keep it disabled and people can enable it from sysfs.
Why do we need to maintain a list of boost here? notifiers?
complex :(
Notifier is needed to disable boost when policy is changed (for
example when we change from ondemand/lab to performance governor).
I wanted to avoid the situation when boost stays enabled across
different governors.
The list of in system available policies is defined to allow boost
enable/disable for all policies available (by changing for example
policy-max field).
If we decide, that we will support only one policy (as it is now at
e.g. Exynos), the list is unnecessary here.
What we discussed last in your earlier patchset was:
- Keep boost feature separate from governors.
- If it is enabled, then any governor can use it (if they want).
- Lets not disable it if governor is changed. user must do it
explicitly.
+static int cpufreq_frequency_table_skip_boost(struct
cpufreq_policy *policy,
+ unsigned int index)
+{
+ if (index == CPUFREQ_BOOST)
+ if (!policy-boost ||
This shouldn't be true. If index has got CPUFREQ_BOOST, then driver
has to support boost.
Correct me if I'm wrong here, but in my understanding the boost
shall be only supported when both CPUFREQ_BOOST index is defined in
a freq_table and boost.state = CPUFREQ_BOOST_EN is set.
Setting of CPUFREQ_BOOST shouldn't by default allow to use over
clocking frequency.
For cpufreq core boost is enabled as soon as cpufreq_driver-boost is
true. We can have additional checks to see if there is atleast one
boost frequency but can skip this too.
why do we need this?
To evaluate the maximal boost frequency from the frequency table.
It is then used as a delimiter (when LAB cooperates with thermal
framework).
Introduce this with LAB then.. Lets keep it as simple as possible for
now. One step at a time.
+struct cpufreq_boost {
+ unsigned intmax_boost_freq; /* maximum value
of
+* boosted freq
*/
+ unsigned intmax_normal_freq; /* non boost max
freq */
+ int status; /* status of boost */
+
+ /* boost sysfs attributies */
+ struct freq_attr**attr;
+
+ /* low-level trigger for boost */
+ int (*low_level_boost) (struct cpufreq_policy *policy,
int state); +
+ struct list_headpolicies;
+};
We don't need it. Just add two more fields to cpufreq_driver:
-
Re: [PATCH 2/5] cpufreq:boost: Add support for software based CPU frequency boost
On 6 June 2013 17:19, Lukasz Majewski wrote:
>> On 6 June 2013 12:37, Lukasz Majewski wrote:
>> > cpufreq_driver->boost->attr))
>>
>> Why is this required? Why do we want platforms to add some files
>> in sysfs?
>
> There are two kinds of attributes, which are exported by boost:
>
> 1. global boost (/sys/devices/system/cpu/cpufreq/boost)
>
> 2. attributes describing cpufreq abilities when boost is available
> (/sys/devices/syste/cpu/cpu0/cpufreq/):
> - scaling_boost_frequencies - which will show over clocked
> frequencies
> - the scaling_available_frequencies will also display boosted
> frequency (when boost enabled)
>
> Information from 2. is cpufreq_driver dependent. And that information
> shouldn't been displayed when boost is not available
This is not how I wanted this to be coded. Lets keep things simple:
- Implement it in the way cpufreq_freq_attr_scaling_available_freqs
is implemented. And then drivers which need to see boost freqs
can add it in their attr.
>> > + policy->boost = cpufreq_boost = cpufreq_driver->boost;
>>
>> Why are we copying same pointer to policy->boost? Driver is
>> passing pointer to a single memory location, just save it globally.
>
> This needs some explanation.
>
> The sysfs entry presented at [1] doesn't bring any useful information
> to reuse (like *policy). For this reason the global cpufreq_boost
> pointer is needed.
>
> However to efficiently manage the boost, it is necessary to keep per
> policy pointer to the only struct cpufreq_boost instance (defined at
> cpufreq_driver code).
No we don't need to screw struct cpufreq_policy with it.
Just need two variables here:
- cpufreq_driver->boost: If driver supports boost or not.
- If above is true then a global bool variable that will say boost is
enabled from sysfs or not.
>> > + /* disable boost for newly created policy - as we e.g.
>> > change
>> > + governor */
>> > + policy->boost->status = CPUFREQ_BOOST_DIS;
>>
>> Drivers supporting boost may want boost to be enabled by default,
>> maybe without any sysfs calls.
>
> This can be done by setting the struct cpufreq_boost status field to
> CPUFREQ_BOOST_EN at cpufreq driver code (when boost structure is
> defined)
This really isn't driver dependent.. But user dependent. Maybe lets
keep it disabled and people can enable it from sysfs.
>> Why do we need to maintain a list of boost here? notifiers? complex :(
>
> Notifier is needed to disable boost when policy is changed (for
> example when we change from ondemand/lab to performance governor).
>
> I wanted to avoid the situation when boost stays enabled across
> different governors.
>
> The list of in system available policies is defined to allow boost
> enable/disable for all policies available (by changing for example
> policy->max field).
>
> If we decide, that we will support only one policy (as it is now at
> e.g. Exynos), the list is unnecessary here.
What we discussed last in your earlier patchset was:
- Keep boost feature separate from governors.
- If it is enabled, then any governor can use it (if they want).
- Lets not disable it if governor is changed. user must do it explicitly.
>> > +static int cpufreq_frequency_table_skip_boost(struct
>> > cpufreq_policy *policy,
>> > + unsigned int index)
>> > +{
>> > + if (index == CPUFREQ_BOOST)
>> > + if (!policy->boost ||
>>
>> This shouldn't be true. If index has got CPUFREQ_BOOST, then driver
>> has to support boost.
>
> Correct me if I'm wrong here, but in my understanding the boost shall be
> only supported when both CPUFREQ_BOOST index is defined in a freq_table
> and boost.state = CPUFREQ_BOOST_EN is set.
>
> Setting of CPUFREQ_BOOST shouldn't by default allow to use over
> clocking frequency.
For cpufreq core boost is enabled as soon as cpufreq_driver->boost is
true. We can have additional checks to see if there is atleast one
boost frequency but can skip this too.
>> why do we need this?
>
> To evaluate the maximal boost frequency from the frequency table. It is
> then used as a delimiter (when LAB cooperates with thermal framework).
Introduce this with LAB then.. Lets keep it as simple as possible for now.
One step at a time.
>> > +struct cpufreq_boost {
>> > + unsigned intmax_boost_freq; /* maximum value of
>> > +* boosted freq */
>> > + unsigned intmax_normal_freq; /* non boost max
>> > freq */
>> > + int status; /* status of boost */
>> > +
>> > + /* boost sysfs attributies */
>> > + struct freq_attr**attr;
>> > +
>> > + /* low-level trigger for boost */
>> > + int (*low_level_boost) (struct cpufreq_policy *policy, int
>> > state); +
>> > + struct list_headpolicies;
>> > +};
>>
>> We don't need it. Just add two more fields to cpufreq_driver:
>> - have_boost_freqs
Re: [PATCH 2/5] cpufreq:boost: Add support for software based CPU frequency boost
Hi Viresh,
> Hi,
>
> On 6 June 2013 12:37, Lukasz Majewski wrote:
> > This commit adds support for software based frequency boosting.
> > Exynos4 SoCs (e.g. 4x12) allow setting of frequency above its normal
> > condition limits. This can be done for some short time.
> >
> > Overclocking (boost) support came from cpufreq driver (which is
> > platform dependent). Hence the data structure describing it is
> > defined at its file.
> >
> > To allow support for either SW and HW (Intel) based boosting, the
> > cpufreq core code has been extended to support both solutions.
> >
> > The main boost switch has been put
> > at /sys/devices/system/cpu/cpufreq/boost.
>
> Log requires some better paragraphs but I am not concerned about it
> for now.
>
> > Signed-off-by: Lukasz Majewski
> > Signed-off-by: Myungjoo Ham
> > ---
> > drivers/cpufreq/cpufreq.c| 156
> > ++
> > drivers/cpufreq/freq_table.c | 87 ++-
> > include/linux/cpufreq.h | 35 +- 3 files changed, 275
> > insertions(+), 3 deletions(-)
>
> My initial view on this patch is: "It is overly engineered and needs
> to get simplified"
>
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index ca74e27..8cf9a92 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -133,6 +133,11 @@ bool have_governor_per_policy(void)
> > return cpufreq_driver->have_governor_per_policy;
> > }
> >
> > +/**
> > + * Global definition of cpufreq_boost structure
> > + */
> > +struct cpufreq_boost *cpufreq_boost;
>
> Probably just a 'static bool' here cpufreq_boost_enabled. Which takes
> care of selection from sysfs.
The pointer to struct cpufreq_boost is needed for further reference
(as it is now done with struct cpufreq_driver pointer - *cpufreq_driver
- defined at cpufreq.c file).
>
> > static struct cpufreq_governor *__find_governor(const char
> > *str_governor) {
> > @@ -761,6 +805,18 @@ static int cpufreq_add_dev_interface(unsigned
> > int cpu, if (cpufreq_set_drv_attr_files(policy,
> > cpufreq_driver->attr)) goto err_out_kobj_put;
> >
> > + if (cpufreq_driver->boost) {
> > + if (sysfs_create_file(cpufreq_global_kobject,
> > + &(global_boost.attr)))
>
> This will report error for systems where we have two policy
> structures. As we are creating something already present.
Good point, thanks.
>
> > + pr_warn("could not register global boost
> > sysfs file\n");
> > + else
> > + pr_debug("registered global boost sysfs
> > file\n");
>
> Please make all your prints to print function name too:
>
> pr_debug("%s: foo\n", __func__, foo);
OK.
>
> > + if (cpufreq_set_drv_attr_files(policy,
> > +
> > cpufreq_driver->boost->attr))
>
> Why is this required? Why do we want platforms to add some files
> in sysfs?
There are two kinds of attributes, which are exported by boost:
1. global boost (/sys/devices/system/cpu/cpufreq/boost)
2. attributes describing cpufreq abilities when boost is available
(/sys/devices/syste/cpu/cpu0/cpufreq/):
- scaling_boost_frequencies - which will show over clocked
frequencies
- the scaling_available_frequencies will also display boosted
frequency (when boost enabled)
Information from 2. is cpufreq_driver dependent. And that information
shouldn't been displayed when boost is not available
>
> > /*
> > + *
> > BOOST *
> > +
> > */
> > +int cpufreq_boost_trigger_state(struct cpufreq_policy *policy, int
> > state) +{
> > + struct cpufreq_boost *boost;
> > + unsigned long flags;
> > + int ret = 0;
> > +
> > + if (!policy || !policy->boost
> > || !policy->boost->low_level_boost)
> > + return -ENODEV;
> > +
> > + boost = policy->boost;
> > + write_lock_irqsave(_driver_lock, flags);
> > +
> > + if (boost->status != state) {
> > + policy->boost->status = state;
> > + ret = boost->low_level_boost(policy, state);
> > + if (ret) {
> > + pr_err("BOOST cannot %sable low level code
> > (%d)\n",
> > + state ? "en" : "dis", ret);
> > + return ret;
> > + }
> > + }
> > +
> > + write_unlock_irqrestore(_driver_lock, flags);
> > +
> > + pr_debug("cpufreq BOOST %sabled\n", state ? "en" : "dis");
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * cpufreq_boost_notifier - notifier callback for cpufreq policy
> > change.
> > + * nb: struct notifier_block * with callback info.
> > + * event: value showing cpufreq event for which this
> > function invoked.
> > + * data:
Re: [PATCH 2/5] cpufreq:boost: Add support for software based CPU frequency boost
Hi,
On 6 June 2013 12:37, Lukasz Majewski wrote:
> This commit adds support for software based frequency boosting.
> Exynos4 SoCs (e.g. 4x12) allow setting of frequency above its normal
> condition limits. This can be done for some short time.
>
> Overclocking (boost) support came from cpufreq driver (which is platform
> dependent). Hence the data structure describing it is defined at its file.
>
> To allow support for either SW and HW (Intel) based boosting, the cpufreq
> core code has been extended to support both solutions.
>
> The main boost switch has been put at /sys/devices/system/cpu/cpufreq/boost.
Log requires some better paragraphs but I am not concerned about it for now.
> Signed-off-by: Lukasz Majewski
> Signed-off-by: Myungjoo Ham
> ---
> drivers/cpufreq/cpufreq.c| 156
> ++
> drivers/cpufreq/freq_table.c | 87 ++-
> include/linux/cpufreq.h | 35 +-
> 3 files changed, 275 insertions(+), 3 deletions(-)
My initial view on this patch is: "It is overly engineered and needs
to get simplified"
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index ca74e27..8cf9a92 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -133,6 +133,11 @@ bool have_governor_per_policy(void)
> return cpufreq_driver->have_governor_per_policy;
> }
>
> +/**
> + * Global definition of cpufreq_boost structure
> + */
> +struct cpufreq_boost *cpufreq_boost;
Probably just a 'static bool' here cpufreq_boost_enabled. Which takes
care of selection from sysfs.
> static struct cpufreq_governor *__find_governor(const char *str_governor)
> {
> @@ -761,6 +805,18 @@ static int cpufreq_add_dev_interface(unsigned int cpu,
> if (cpufreq_set_drv_attr_files(policy, cpufreq_driver->attr))
> goto err_out_kobj_put;
>
> + if (cpufreq_driver->boost) {
> + if (sysfs_create_file(cpufreq_global_kobject,
> + &(global_boost.attr)))
This will report error for systems where we have two policy structures.
As we are creating something already present.
> + pr_warn("could not register global boost sysfs
> file\n");
> + else
> + pr_debug("registered global boost sysfs file\n");
Please make all your prints to print function name too:
pr_debug("%s: foo\n", __func__, foo);
> + if (cpufreq_set_drv_attr_files(policy,
> + cpufreq_driver->boost->attr))
Why is this required? Why do we want platforms to add some files
in sysfs?
> /*
> + * BOOST *
> + */
> +int cpufreq_boost_trigger_state(struct cpufreq_policy *policy, int state)
> +{
> + struct cpufreq_boost *boost;
> + unsigned long flags;
> + int ret = 0;
> +
> + if (!policy || !policy->boost || !policy->boost->low_level_boost)
> + return -ENODEV;
> +
> + boost = policy->boost;
> + write_lock_irqsave(_driver_lock, flags);
> +
> + if (boost->status != state) {
> + policy->boost->status = state;
> + ret = boost->low_level_boost(policy, state);
> + if (ret) {
> + pr_err("BOOST cannot %sable low level code (%d)\n",
> + state ? "en" : "dis", ret);
> + return ret;
> + }
> + }
> +
> + write_unlock_irqrestore(_driver_lock, flags);
> +
> + pr_debug("cpufreq BOOST %sabled\n", state ? "en" : "dis");
> +
> + return 0;
> +}
> +
> +/**
> + * cpufreq_boost_notifier - notifier callback for cpufreq policy change.
> + * nb: struct notifier_block * with callback info.
> + * event: value showing cpufreq event for which this function invoked.
> + * data: callback-specific data
> + */
> +static int cpufreq_boost_notifier(struct notifier_block *nb,
> + unsigned long event, void *data)
> +{
> + struct cpufreq_policy *policy = data;
> +
> + if (event == CPUFREQ_INCOMPATIBLE) {
> + if (!policy || !policy->boost)
> + return -ENODEV;
> +
> + if (policy->boost->status == CPUFREQ_BOOST_EN) {
> + pr_info("NOTIFIER BOOST: MAX: %d e:%lu cpu: %d\n",
> + policy->max, event, policy->cpu);
> + cpufreq_boost_trigger_state(policy, 0);
> + }
> + }
> +
> + return 0;
> +}
> +
> +/* Notifier for cpufreq policy change */
> +static struct notifier_block cpufreq_boost_notifier_block = {
> + .notifier_call = cpufreq_boost_notifier,
> +};
> +
> +int cpufreq_boost_init(struct cpufreq_policy *policy)
> +{
> +
[PATCH 2/5] cpufreq:boost: Add support for software based CPU frequency boost
This commit adds support for software based frequency boosting.
Exynos4 SoCs (e.g. 4x12) allow setting of frequency above its normal
condition limits. This can be done for some short time.
Overclocking (boost) support came from cpufreq driver (which is platform
dependent). Hence the data structure describing it is defined at its file.
To allow support for either SW and HW (Intel) based boosting, the cpufreq
core code has been extended to support both solutions.
The main boost switch has been put at /sys/devices/system/cpu/cpufreq/boost.
Signed-off-by: Lukasz Majewski
Signed-off-by: Myungjoo Ham
---
drivers/cpufreq/cpufreq.c| 156 ++
drivers/cpufreq/freq_table.c | 87 ++-
include/linux/cpufreq.h | 35 +-
3 files changed, 275 insertions(+), 3 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index ca74e27..8cf9a92 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -133,6 +133,11 @@ bool have_governor_per_policy(void)
return cpufreq_driver->have_governor_per_policy;
}
+/**
+ * Global definition of cpufreq_boost structure
+ */
+struct cpufreq_boost *cpufreq_boost;
+
static struct cpufreq_policy *__cpufreq_cpu_get(unsigned int cpu, bool sysfs)
{
struct cpufreq_policy *data;
@@ -315,6 +320,45 @@ EXPORT_SYMBOL_GPL(cpufreq_notify_transition);
/*
* SYSFS INTERFACE *
*/
+ssize_t show_boost_status(struct kobject *kobj,
+struct attribute *attr, char *buf)
+{
+ struct cpufreq_boost *boost = cpufreq_boost;
+
+ if (!boost)
+ return -ENODEV;
+
+ return sprintf(buf, "%d\n", boost->status);
+}
+
+static ssize_t store_boost_status(struct kobject *kobj, struct attribute *attr,
+ const char *buf, size_t count)
+{
+ struct cpufreq_boost *boost = cpufreq_boost;
+ struct cpufreq_policy *p;
+ int ret, enable;
+
+ if (!boost)
+ return -ENODEV;
+
+ ret = sscanf(buf, "%d", );
+ if (ret != 1 || enable < 0 || enable > 1)
+ return -EINVAL;
+
+ /* Adjust all policies to support boost */
+ list_for_each_entry(p, >policies, boost_list)
+ if (cpufreq_boost_trigger_state(p, enable)) {
+ pr_err("Cannot %sable boost (policy 0x%p)!\n",
+ enable ? "en" : "dis", p);
+ return -EINVAL;
+ }
+
+ return count;
+}
+
+static struct global_attr global_boost = __ATTR(boost, 0644,
+ show_boost_status,
+ store_boost_status);
static struct cpufreq_governor *__find_governor(const char *str_governor)
{
@@ -761,6 +805,18 @@ static int cpufreq_add_dev_interface(unsigned int cpu,
if (cpufreq_set_drv_attr_files(policy, cpufreq_driver->attr))
goto err_out_kobj_put;
+ if (cpufreq_driver->boost) {
+ if (sysfs_create_file(cpufreq_global_kobject,
+ &(global_boost.attr)))
+ pr_warn("could not register global boost sysfs file\n");
+ else
+ pr_debug("registered global boost sysfs file\n");
+
+ if (cpufreq_set_drv_attr_files(policy,
+ cpufreq_driver->boost->attr))
+ goto err_out_kobj_put;
+ }
+
if (cpufreq_driver->get) {
ret = sysfs_create_file(>kobj, _cur_freq.attr);
if (ret)
@@ -923,6 +979,8 @@ static int cpufreq_add_dev(struct device *dev, struct
subsys_interface *sif)
init_completion(>kobj_unregister);
INIT_WORK(>update, handle_update);
+ cpufreq_boost_init(policy);
+
/* call driver. From then on the cpufreq must be able
* to accept all calls to ->verify and ->setpolicy for this CPU
*/
@@ -1860,6 +1918,100 @@ static struct notifier_block __refdata
cpufreq_cpu_notifier = {
};
/*
+ * BOOST *
+ */
+int cpufreq_boost_trigger_state(struct cpufreq_policy *policy, int state)
+{
+ struct cpufreq_boost *boost;
+ unsigned long flags;
+ int ret = 0;
+
+ if (!policy || !policy->boost || !policy->boost->low_level_boost)
+ return -ENODEV;
+
+ boost = policy->boost;
+ write_lock_irqsave(_driver_lock, flags);
+
+ if (boost->status != state) {
+ policy->boost->status = state;
+
[PATCH 2/5] cpufreq:boost: Add support for software based CPU frequency boost
This commit adds support for software based frequency boosting.
Exynos4 SoCs (e.g. 4x12) allow setting of frequency above its normal
condition limits. This can be done for some short time.
Overclocking (boost) support came from cpufreq driver (which is platform
dependent). Hence the data structure describing it is defined at its file.
To allow support for either SW and HW (Intel) based boosting, the cpufreq
core code has been extended to support both solutions.
The main boost switch has been put at /sys/devices/system/cpu/cpufreq/boost.
Signed-off-by: Lukasz Majewski l.majew...@samsung.com
Signed-off-by: Myungjoo Ham myungjoo@samsung.com
---
drivers/cpufreq/cpufreq.c| 156 ++
drivers/cpufreq/freq_table.c | 87 ++-
include/linux/cpufreq.h | 35 +-
3 files changed, 275 insertions(+), 3 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index ca74e27..8cf9a92 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -133,6 +133,11 @@ bool have_governor_per_policy(void)
return cpufreq_driver-have_governor_per_policy;
}
+/**
+ * Global definition of cpufreq_boost structure
+ */
+struct cpufreq_boost *cpufreq_boost;
+
static struct cpufreq_policy *__cpufreq_cpu_get(unsigned int cpu, bool sysfs)
{
struct cpufreq_policy *data;
@@ -315,6 +320,45 @@ EXPORT_SYMBOL_GPL(cpufreq_notify_transition);
/*
* SYSFS INTERFACE *
*/
+ssize_t show_boost_status(struct kobject *kobj,
+struct attribute *attr, char *buf)
+{
+ struct cpufreq_boost *boost = cpufreq_boost;
+
+ if (!boost)
+ return -ENODEV;
+
+ return sprintf(buf, %d\n, boost-status);
+}
+
+static ssize_t store_boost_status(struct kobject *kobj, struct attribute *attr,
+ const char *buf, size_t count)
+{
+ struct cpufreq_boost *boost = cpufreq_boost;
+ struct cpufreq_policy *p;
+ int ret, enable;
+
+ if (!boost)
+ return -ENODEV;
+
+ ret = sscanf(buf, %d, enable);
+ if (ret != 1 || enable 0 || enable 1)
+ return -EINVAL;
+
+ /* Adjust all policies to support boost */
+ list_for_each_entry(p, boost-policies, boost_list)
+ if (cpufreq_boost_trigger_state(p, enable)) {
+ pr_err(Cannot %sable boost (policy 0x%p)!\n,
+ enable ? en : dis, p);
+ return -EINVAL;
+ }
+
+ return count;
+}
+
+static struct global_attr global_boost = __ATTR(boost, 0644,
+ show_boost_status,
+ store_boost_status);
static struct cpufreq_governor *__find_governor(const char *str_governor)
{
@@ -761,6 +805,18 @@ static int cpufreq_add_dev_interface(unsigned int cpu,
if (cpufreq_set_drv_attr_files(policy, cpufreq_driver-attr))
goto err_out_kobj_put;
+ if (cpufreq_driver-boost) {
+ if (sysfs_create_file(cpufreq_global_kobject,
+ (global_boost.attr)))
+ pr_warn(could not register global boost sysfs file\n);
+ else
+ pr_debug(registered global boost sysfs file\n);
+
+ if (cpufreq_set_drv_attr_files(policy,
+ cpufreq_driver-boost-attr))
+ goto err_out_kobj_put;
+ }
+
if (cpufreq_driver-get) {
ret = sysfs_create_file(policy-kobj, cpuinfo_cur_freq.attr);
if (ret)
@@ -923,6 +979,8 @@ static int cpufreq_add_dev(struct device *dev, struct
subsys_interface *sif)
init_completion(policy-kobj_unregister);
INIT_WORK(policy-update, handle_update);
+ cpufreq_boost_init(policy);
+
/* call driver. From then on the cpufreq must be able
* to accept all calls to -verify and -setpolicy for this CPU
*/
@@ -1860,6 +1918,100 @@ static struct notifier_block __refdata
cpufreq_cpu_notifier = {
};
/*
+ * BOOST *
+ */
+int cpufreq_boost_trigger_state(struct cpufreq_policy *policy, int state)
+{
+ struct cpufreq_boost *boost;
+ unsigned long flags;
+ int ret = 0;
+
+ if (!policy || !policy-boost || !policy-boost-low_level_boost)
+ return -ENODEV;
+
+ boost = policy-boost;
+ write_lock_irqsave(cpufreq_driver_lock, flags);
+
+ if (boost-status != state) {
+
Re: [PATCH 2/5] cpufreq:boost: Add support for software based CPU frequency boost
Hi,
On 6 June 2013 12:37, Lukasz Majewski l.majew...@samsung.com wrote:
This commit adds support for software based frequency boosting.
Exynos4 SoCs (e.g. 4x12) allow setting of frequency above its normal
condition limits. This can be done for some short time.
Overclocking (boost) support came from cpufreq driver (which is platform
dependent). Hence the data structure describing it is defined at its file.
To allow support for either SW and HW (Intel) based boosting, the cpufreq
core code has been extended to support both solutions.
The main boost switch has been put at /sys/devices/system/cpu/cpufreq/boost.
Log requires some better paragraphs but I am not concerned about it for now.
Signed-off-by: Lukasz Majewski l.majew...@samsung.com
Signed-off-by: Myungjoo Ham myungjoo@samsung.com
---
drivers/cpufreq/cpufreq.c| 156
++
drivers/cpufreq/freq_table.c | 87 ++-
include/linux/cpufreq.h | 35 +-
3 files changed, 275 insertions(+), 3 deletions(-)
My initial view on this patch is: It is overly engineered and needs
to get simplified
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index ca74e27..8cf9a92 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -133,6 +133,11 @@ bool have_governor_per_policy(void)
return cpufreq_driver-have_governor_per_policy;
}
+/**
+ * Global definition of cpufreq_boost structure
+ */
+struct cpufreq_boost *cpufreq_boost;
Probably just a 'static bool' here cpufreq_boost_enabled. Which takes
care of selection from sysfs.
static struct cpufreq_governor *__find_governor(const char *str_governor)
{
@@ -761,6 +805,18 @@ static int cpufreq_add_dev_interface(unsigned int cpu,
if (cpufreq_set_drv_attr_files(policy, cpufreq_driver-attr))
goto err_out_kobj_put;
+ if (cpufreq_driver-boost) {
+ if (sysfs_create_file(cpufreq_global_kobject,
+ (global_boost.attr)))
This will report error for systems where we have two policy structures.
As we are creating something already present.
+ pr_warn(could not register global boost sysfs
file\n);
+ else
+ pr_debug(registered global boost sysfs file\n);
Please make all your prints to print function name too:
pr_debug(%s: foo\n, __func__, foo);
+ if (cpufreq_set_drv_attr_files(policy,
+ cpufreq_driver-boost-attr))
Why is this required? Why do we want platforms to add some files
in sysfs?
/*
+ * BOOST *
+ */
+int cpufreq_boost_trigger_state(struct cpufreq_policy *policy, int state)
+{
+ struct cpufreq_boost *boost;
+ unsigned long flags;
+ int ret = 0;
+
+ if (!policy || !policy-boost || !policy-boost-low_level_boost)
+ return -ENODEV;
+
+ boost = policy-boost;
+ write_lock_irqsave(cpufreq_driver_lock, flags);
+
+ if (boost-status != state) {
+ policy-boost-status = state;
+ ret = boost-low_level_boost(policy, state);
+ if (ret) {
+ pr_err(BOOST cannot %sable low level code (%d)\n,
+ state ? en : dis, ret);
+ return ret;
+ }
+ }
+
+ write_unlock_irqrestore(cpufreq_driver_lock, flags);
+
+ pr_debug(cpufreq BOOST %sabled\n, state ? en : dis);
+
+ return 0;
+}
+
+/**
+ * cpufreq_boost_notifier - notifier callback for cpufreq policy change.
+ * at nb: struct notifier_block * with callback info.
+ * at event: value showing cpufreq event for which this function invoked.
+ * at data: callback-specific data
+ */
+static int cpufreq_boost_notifier(struct notifier_block *nb,
+ unsigned long event, void *data)
+{
+ struct cpufreq_policy *policy = data;
+
+ if (event == CPUFREQ_INCOMPATIBLE) {
+ if (!policy || !policy-boost)
+ return -ENODEV;
+
+ if (policy-boost-status == CPUFREQ_BOOST_EN) {
+ pr_info(NOTIFIER BOOST: MAX: %d e:%lu cpu: %d\n,
+ policy-max, event, policy-cpu);
+ cpufreq_boost_trigger_state(policy, 0);
+ }
+ }
+
+ return 0;
+}
+
+/* Notifier for cpufreq policy change */
+static struct notifier_block cpufreq_boost_notifier_block = {
+ .notifier_call = cpufreq_boost_notifier,
+};
+
+int cpufreq_boost_init(struct cpufreq_policy *policy)
+{
+ int ret = 0;
+
+ if (!policy)
+
Re: [PATCH 2/5] cpufreq:boost: Add support for software based CPU frequency boost
Hi Viresh,
Hi,
On 6 June 2013 12:37, Lukasz Majewski l.majew...@samsung.com wrote:
This commit adds support for software based frequency boosting.
Exynos4 SoCs (e.g. 4x12) allow setting of frequency above its normal
condition limits. This can be done for some short time.
Overclocking (boost) support came from cpufreq driver (which is
platform dependent). Hence the data structure describing it is
defined at its file.
To allow support for either SW and HW (Intel) based boosting, the
cpufreq core code has been extended to support both solutions.
The main boost switch has been put
at /sys/devices/system/cpu/cpufreq/boost.
Log requires some better paragraphs but I am not concerned about it
for now.
Signed-off-by: Lukasz Majewski l.majew...@samsung.com
Signed-off-by: Myungjoo Ham myungjoo@samsung.com
---
drivers/cpufreq/cpufreq.c| 156
++
drivers/cpufreq/freq_table.c | 87 ++-
include/linux/cpufreq.h | 35 +- 3 files changed, 275
insertions(+), 3 deletions(-)
My initial view on this patch is: It is overly engineered and needs
to get simplified
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index ca74e27..8cf9a92 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -133,6 +133,11 @@ bool have_governor_per_policy(void)
return cpufreq_driver-have_governor_per_policy;
}
+/**
+ * Global definition of cpufreq_boost structure
+ */
+struct cpufreq_boost *cpufreq_boost;
Probably just a 'static bool' here cpufreq_boost_enabled. Which takes
care of selection from sysfs.
The pointer to struct cpufreq_boost is needed for further reference
(as it is now done with struct cpufreq_driver pointer - *cpufreq_driver
- defined at cpufreq.c file).
static struct cpufreq_governor *__find_governor(const char
*str_governor) {
@@ -761,6 +805,18 @@ static int cpufreq_add_dev_interface(unsigned
int cpu, if (cpufreq_set_drv_attr_files(policy,
cpufreq_driver-attr)) goto err_out_kobj_put;
+ if (cpufreq_driver-boost) {
+ if (sysfs_create_file(cpufreq_global_kobject,
+ (global_boost.attr)))
This will report error for systems where we have two policy
structures. As we are creating something already present.
Good point, thanks.
+ pr_warn(could not register global boost
sysfs file\n);
+ else
+ pr_debug(registered global boost sysfs
file\n);
Please make all your prints to print function name too:
pr_debug(%s: foo\n, __func__, foo);
OK.
+ if (cpufreq_set_drv_attr_files(policy,
+
cpufreq_driver-boost-attr))
Why is this required? Why do we want platforms to add some files
in sysfs?
There are two kinds of attributes, which are exported by boost:
1. global boost (/sys/devices/system/cpu/cpufreq/boost)
2. attributes describing cpufreq abilities when boost is available
(/sys/devices/syste/cpu/cpu0/cpufreq/):
- scaling_boost_frequencies - which will show over clocked
frequencies
- the scaling_available_frequencies will also display boosted
frequency (when boost enabled)
Information from 2. is cpufreq_driver dependent. And that information
shouldn't been displayed when boost is not available
/*
+ *
BOOST *
+
*/
+int cpufreq_boost_trigger_state(struct cpufreq_policy *policy, int
state) +{
+ struct cpufreq_boost *boost;
+ unsigned long flags;
+ int ret = 0;
+
+ if (!policy || !policy-boost
|| !policy-boost-low_level_boost)
+ return -ENODEV;
+
+ boost = policy-boost;
+ write_lock_irqsave(cpufreq_driver_lock, flags);
+
+ if (boost-status != state) {
+ policy-boost-status = state;
+ ret = boost-low_level_boost(policy, state);
+ if (ret) {
+ pr_err(BOOST cannot %sable low level code
(%d)\n,
+ state ? en : dis, ret);
+ return ret;
+ }
+ }
+
+ write_unlock_irqrestore(cpufreq_driver_lock, flags);
+
+ pr_debug(cpufreq BOOST %sabled\n, state ? en : dis);
+
+ return 0;
+}
+
+/**
+ * cpufreq_boost_notifier - notifier callback for cpufreq policy
change.
+ * at nb: struct notifier_block * with callback info.
+ * at event: value showing cpufreq event for which this
function invoked.
+ * at data: callback-specific data
+ */
+static int cpufreq_boost_notifier(struct notifier_block *nb,
+ unsigned long event, void
Re: [PATCH 2/5] cpufreq:boost: Add support for software based CPU frequency boost
On 6 June 2013 17:19, Lukasz Majewski l.majew...@samsung.com wrote:
On 6 June 2013 12:37, Lukasz Majewski l.majew...@samsung.com wrote:
cpufreq_driver-boost-attr))
Why is this required? Why do we want platforms to add some files
in sysfs?
There are two kinds of attributes, which are exported by boost:
1. global boost (/sys/devices/system/cpu/cpufreq/boost)
2. attributes describing cpufreq abilities when boost is available
(/sys/devices/syste/cpu/cpu0/cpufreq/):
- scaling_boost_frequencies - which will show over clocked
frequencies
- the scaling_available_frequencies will also display boosted
frequency (when boost enabled)
Information from 2. is cpufreq_driver dependent. And that information
shouldn't been displayed when boost is not available
This is not how I wanted this to be coded. Lets keep things simple:
- Implement it in the way cpufreq_freq_attr_scaling_available_freqs
is implemented. And then drivers which need to see boost freqs
can add it in their attr.
+ policy-boost = cpufreq_boost = cpufreq_driver-boost;
Why are we copying same pointer to policy-boost? Driver is
passing pointer to a single memory location, just save it globally.
This needs some explanation.
The sysfs entry presented at [1] doesn't bring any useful information
to reuse (like *policy). For this reason the global cpufreq_boost
pointer is needed.
However to efficiently manage the boost, it is necessary to keep per
policy pointer to the only struct cpufreq_boost instance (defined at
cpufreq_driver code).
No we don't need to screw struct cpufreq_policy with it.
Just need two variables here:
- cpufreq_driver-boost: If driver supports boost or not.
- If above is true then a global bool variable that will say boost is
enabled from sysfs or not.
+ /* disable boost for newly created policy - as we e.g.
change
+ governor */
+ policy-boost-status = CPUFREQ_BOOST_DIS;
Drivers supporting boost may want boost to be enabled by default,
maybe without any sysfs calls.
This can be done by setting the struct cpufreq_boost status field to
CPUFREQ_BOOST_EN at cpufreq driver code (when boost structure is
defined)
This really isn't driver dependent.. But user dependent. Maybe lets
keep it disabled and people can enable it from sysfs.
Why do we need to maintain a list of boost here? notifiers? complex :(
Notifier is needed to disable boost when policy is changed (for
example when we change from ondemand/lab to performance governor).
I wanted to avoid the situation when boost stays enabled across
different governors.
The list of in system available policies is defined to allow boost
enable/disable for all policies available (by changing for example
policy-max field).
If we decide, that we will support only one policy (as it is now at
e.g. Exynos), the list is unnecessary here.
What we discussed last in your earlier patchset was:
- Keep boost feature separate from governors.
- If it is enabled, then any governor can use it (if they want).
- Lets not disable it if governor is changed. user must do it explicitly.
+static int cpufreq_frequency_table_skip_boost(struct
cpufreq_policy *policy,
+ unsigned int index)
+{
+ if (index == CPUFREQ_BOOST)
+ if (!policy-boost ||
This shouldn't be true. If index has got CPUFREQ_BOOST, then driver
has to support boost.
Correct me if I'm wrong here, but in my understanding the boost shall be
only supported when both CPUFREQ_BOOST index is defined in a freq_table
and boost.state = CPUFREQ_BOOST_EN is set.
Setting of CPUFREQ_BOOST shouldn't by default allow to use over
clocking frequency.
For cpufreq core boost is enabled as soon as cpufreq_driver-boost is
true. We can have additional checks to see if there is atleast one
boost frequency but can skip this too.
why do we need this?
To evaluate the maximal boost frequency from the frequency table. It is
then used as a delimiter (when LAB cooperates with thermal framework).
Introduce this with LAB then.. Lets keep it as simple as possible for now.
One step at a time.
+struct cpufreq_boost {
+ unsigned intmax_boost_freq; /* maximum value of
+* boosted freq */
+ unsigned intmax_normal_freq; /* non boost max
freq */
+ int status; /* status of boost */
+
+ /* boost sysfs attributies */
+ struct freq_attr**attr;
+
+ /* low-level trigger for boost */
+ int (*low_level_boost) (struct cpufreq_policy *policy, int
state); +
+ struct list_headpolicies;
+};
We don't need it. Just add two more fields to cpufreq_driver:
- have_boost_freqs and low_level_boost (maybe a better name.
What's its use?)
The separate struct cpufreq_boost was created to explicitly separate
