Hi Viresh,
On 6 June 2013 17:19, Lukasz Majewski l.majewski@samsung.com wrote:
On 6 June 2013 12:37, Lukasz Majewski l.majewski@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:
global boost (/sys/devices/system/cpu/cpufreq/boost)
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.
Sorry, I have LAB in back of my head. For now I'm forgetting about it :-) [*]
+struct cpufreq_boost {
unsigned int max_boost_freq; /* maximum valueof
* boosted freq*/
unsigned int max_normal_freq; /* non boost maxfreq */
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_head policies;+};
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 boost fields from cpufreq_driver structure.
If in your opinion this structure is redundant, I can remove it and extend cpufreq_driver structure.
I am not against a structure (as putting related stuff in a struct is always better), but against so many fields in it to make things complicated.
I will only keep have_boost_freqs and low_level_boost. Remove everything else.
As I've written at other mail. This struct will have only two fields, so I will embed those fields at cpufreq_driver.
*boost pointer is necessary when one wants to enable/disable boost from e.g governor code (which operates mostly on struct cpufreq_policy *policy pointers).
We don't need to do this. boost can only be disabled from userspace by user. No intervention from governor.
Let's got for that option (as I've promissed at [*] :-) ).