Hi Viresh,
> On 6 June 2013 17:19, Lukasz Majewski <l.majewski@xxxxxxxxxxx> wrote:
> >> On 6 June 2013 12:37, Lukasz Majewski <l.majewski@xxxxxxxxxxx>
> >> 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 int max_boost_freq; /* maximum value
> >> > of
> >> > + * boosted freq
> >> > */
> >> > + unsigned int max_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_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.
I would prefer to have following fields in the cpufreq_boost structure:
struct cpufreq_boost {
unsigned int max_boost_freq; /*boost max freq*/
unsigned int max_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.
>
> > *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.
--
Best regards,
Lukasz Majewski
Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
--
To unsubscribe from this list: send the line "unsubscribe cpufreq" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
