| Message ID | 1400029380-5372-2-git-send-email-thomas.ab@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 14 May 2014 06:32, Thomas Abraham <ta.omasab@gmail.com> wrote: > From: Thomas Abraham <thomas.ab@samsung.com> > > Commit 6f19efc0 ("cpufreq: Add boost frequency support in core") adds > support for CPU boost mode. This patch adds support for finding available > boost frequencies from device tree and marking them as usable in boost mode. > > Cc: Nishanth Menon <nm@ti.com> > Cc: Lukasz Majewski <l.majewski@samsung.com> > Signed-off-by: Thomas Abraham <thomas.ab@samsung.com> > --- > drivers/cpufreq/cpufreq_opp.c | 39 +++++++++++++++++++++++++++++++++++++++ > 1 files changed, 39 insertions(+), 0 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq_opp.c b/drivers/cpufreq/cpufreq_opp.c > index c0c6f4a..e3c97f3 100644 > --- a/drivers/cpufreq/cpufreq_opp.c > +++ b/drivers/cpufreq/cpufreq_opp.c > @@ -19,6 +19,7 @@ > #include <linux/pm_opp.h> > #include <linux/rcupdate.h> > #include <linux/slab.h> > +#include <linux/of.h> > > /** > * dev_pm_opp_init_cpufreq_table() - create a cpufreq table for a device > @@ -51,6 +52,10 @@ int dev_pm_opp_init_cpufreq_table(struct device *dev, > struct cpufreq_frequency_table *freq_table = NULL; > int i, max_opps, ret = 0; > unsigned long rate; > +#ifdef CONFIG_CPU_FREQ_BOOST_SW > + int j, len; > + u32 *boost_freqs = NULL; > +#endif > > rcu_read_lock(); > > @@ -82,6 +87,40 @@ int dev_pm_opp_init_cpufreq_table(struct device *dev, > > *table = &freq_table[0]; > > +#ifdef CONFIG_CPU_FREQ_BOOST_SW > + if (of_find_property(dev->of_node, "boost-frequency", &len)) { Does this mean another block inside the cpu node ? Like this: ? cpu@0 { compatible = "arm,cortex-a9"; reg = <0>; next-level-cache = <&L2>; operating-points = < /* kHz uV */ 792000 1100000 396000 950000 198000 850000 >; boost-frequency = < 792000 198000 >; }; I think it we might better add another field in the opp block as these OPPs are rather boost one.. @Rob/Rafael: Opinion please .. > + if (len == 0 || (len & (sizeof(u32) - 1)) != 0) { > + dev_err(dev, "%s: invalid boost frequency\n", __func__); > + ret = -EINVAL; > + goto out; > + } > + > + boost_freqs = kzalloc(len, GFP_KERNEL); > + if (!boost_freqs) { > + dev_warn(dev, "%s: no memory for boost freq table\n", > + __func__); > + ret = -ENOMEM; > + goto out; > + } > + of_property_read_u32_array(dev->of_node, "boost-frequency", > + boost_freqs, len / sizeof(u32)); > + } > + > + for (j = 0; j < len / sizeof(u32) && boost_freqs; j++) { Why is this present outside of above if {} ? as boost_freqs is guaranteed to be NULL without that. > + for (i = 0; freq_table[i].frequency != CPUFREQ_TABLE_END; i++) { > + if (boost_freqs[j] == freq_table[i].frequency) { Use cpufreq_frequency_table_get_index() instead. > + freq_table[i].flags |= CPUFREQ_BOOST_FREQ; > + break; > + } > + } > + if (freq_table[i].frequency == CPUFREQ_TABLE_END) > + pr_err("%s: invalid boost frequency %d\n", > + __func__, boost_freqs[j]); > + } > + > + kfree(boost_freqs); > +#endif > + > out: > rcu_read_unlock(); > if (ret) > -- > 1.7.4.4 >
On Tue, May 13, 2014 at 10:40 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 14 May 2014 06:32, Thomas Abraham <ta.omasab@gmail.com> wrote: [...] >> +#ifdef CONFIG_CPU_FREQ_BOOST_SW >> + if (of_find_property(dev->of_node, "boost-frequency", &len)) { > > Does this mean another block inside the cpu node ? Like this: ? > > cpu@0 { > compatible = "arm,cortex-a9"; > reg = <0>; > next-level-cache = <&L2>; > operating-points = < > /* kHz uV */ > 792000 1100000 > 396000 950000 > 198000 850000 > >; > boost-frequency = < > 792000 > 198000 > >; > }; > > I think it we might better add another field in the opp block as these > OPPs are rather boost one.. If we change the meaning to be: operating-points = < /* kHz uV boost? */ 792000 1100000 1 396000 950000 0 We are adding a modifier to the OPP definition here and does imply every platform which may or maynot require "boost" need to indicate that - basically fails at least my "least common" description for a generic definition. As I had indicated in other threads -> we are back to the discussion of "additional properties" to an OPP.. Option 1: - describe modifiers separately (as in boost-frequencies) - that operate based on data from opp table. Option 2: - keep adding to the description of opp as time goes by - every SoC has it's own set of quirks that are needed for an OPP - I know that at TI, we have certain OPPs that can only be enabled *if* "custom hardware driver" is enabled, and similar stories. - yet another example is enable the OPP if efuse X, bit Y is set. Personally, looking at the various descriptions and bL, cpu-idle states dependencies on OPPs, etc.. Option 2 is a non-scalable approach. [...]
Hi Nishanth, Viresh If I may add my 2 cents. > On Tue, May 13, 2014 at 10:40 PM, Viresh Kumar > <viresh.kumar@linaro.org> wrote: > > On 14 May 2014 06:32, Thomas Abraham <ta.omasab@gmail.com> wrote: > [...] > >> +#ifdef CONFIG_CPU_FREQ_BOOST_SW > >> + if (of_find_property(dev->of_node, "boost-frequency", > >> &len)) { > > > > Does this mean another block inside the cpu node ? Like this: ? > > > > cpu@0 { > > compatible = "arm,cortex-a9"; > > reg = <0>; > > next-level-cache = <&L2>; > > operating-points = < > > /* kHz uV */ > > 792000 1100000 > > 396000 950000 > > 198000 850000 > > >; > > boost-frequency = < > > 792000 > > 198000 > > >; I think that the above approach is more appealing [*]. > > }; > > > > I think it we might better add another field in the opp block as > > these OPPs are rather boost one.. > > If we change the meaning to be: > operating-points = < > /* kHz uV boost? */ > 792000 1100000 1 > 396000 950000 0 > > We are adding a modifier to the OPP definition here and does imply > every platform which may or maynot require "boost" need to indicate > that - basically fails at least my "least common" description for a > generic definition. As I had indicated in other threads -> we are back > to the discussion of "additional properties" to an OPP.. > Option 1: > - describe modifiers separately (as in boost-frequencies) - that > operate based on data from opp table. > Option 2: > - keep adding to the description of opp as time goes by - every SoC > has it's own set of quirks that are needed for an OPP - I know that at > TI, we have certain OPPs that can only be enabled *if* "custom > hardware driver" is enabled, and similar stories. - yet another > example is enable the OPP if efuse X, bit Y is set. > > Personally, looking at the various descriptions and bL, cpu-idle > states dependencies on OPPs, etc.. Option 2 is a non-scalable > approach. I agree with Nishanth here, that point 1 (as described by Viresh at [*]) is a more scalable approach. > > [...]
On 14 May 2014 11:39, Lukasz Majewski <l.majewski@samsung.com> wrote: > I agree with Nishanth here, that point 1 (as described by Viresh at > [*]) is a more scalable approach. The only reason why I wanted all that to be done at OPP level was to ensure if somebody else also needs it apart from cpufreq, they don't have to duplicate code and find it.. As it is present at a central place.. But if no other code is going to look for that, it may just be fine as is..
On Wed, May 14, 2014 at 9:10 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 14 May 2014 06:32, Thomas Abraham <ta.omasab@gmail.com> wrote: >> From: Thomas Abraham <thomas.ab@samsung.com> >> >> Commit 6f19efc0 ("cpufreq: Add boost frequency support in core") adds >> support for CPU boost mode. This patch adds support for finding available >> boost frequencies from device tree and marking them as usable in boost mode. >> >> Cc: Nishanth Menon <nm@ti.com> >> Cc: Lukasz Majewski <l.majewski@samsung.com> >> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com> >> --- >> drivers/cpufreq/cpufreq_opp.c | 39 +++++++++++++++++++++++++++++++++++++++ >> 1 files changed, 39 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/cpufreq/cpufreq_opp.c b/drivers/cpufreq/cpufreq_opp.c >> index c0c6f4a..e3c97f3 100644 >> --- a/drivers/cpufreq/cpufreq_opp.c >> +++ b/drivers/cpufreq/cpufreq_opp.c >> @@ -19,6 +19,7 @@ >> #include <linux/pm_opp.h> >> #include <linux/rcupdate.h> >> #include <linux/slab.h> >> +#include <linux/of.h> >> >> /** >> * dev_pm_opp_init_cpufreq_table() - create a cpufreq table for a device >> @@ -51,6 +52,10 @@ int dev_pm_opp_init_cpufreq_table(struct device *dev, >> struct cpufreq_frequency_table *freq_table = NULL; >> int i, max_opps, ret = 0; >> unsigned long rate; >> +#ifdef CONFIG_CPU_FREQ_BOOST_SW >> + int j, len; >> + u32 *boost_freqs = NULL; >> +#endif >> >> rcu_read_lock(); >> >> @@ -82,6 +87,40 @@ int dev_pm_opp_init_cpufreq_table(struct device *dev, >> >> *table = &freq_table[0]; >> >> +#ifdef CONFIG_CPU_FREQ_BOOST_SW >> + if (of_find_property(dev->of_node, "boost-frequency", &len)) { > > Does this mean another block inside the cpu node ? Like this: ? > > cpu@0 { > compatible = "arm,cortex-a9"; > reg = <0>; > next-level-cache = <&L2>; > operating-points = < > /* kHz uV */ > 792000 1100000 > 396000 950000 > 198000 850000 > >; > boost-frequency = < > 792000 > 198000 > >; > }; > > I think it we might better add another field in the opp block as these > OPPs are rather boost one.. > > @Rob/Rafael: Opinion please .. > >> + if (len == 0 || (len & (sizeof(u32) - 1)) != 0) { >> + dev_err(dev, "%s: invalid boost frequency\n", __func__); >> + ret = -EINVAL; >> + goto out; >> + } >> + >> + boost_freqs = kzalloc(len, GFP_KERNEL); >> + if (!boost_freqs) { >> + dev_warn(dev, "%s: no memory for boost freq table\n", >> + __func__); >> + ret = -ENOMEM; >> + goto out; >> + } >> + of_property_read_u32_array(dev->of_node, "boost-frequency", >> + boost_freqs, len / sizeof(u32)); >> + } >> + >> + for (j = 0; j < len / sizeof(u32) && boost_freqs; j++) { > > Why is this present outside of above if {} ? as boost_freqs is guaranteed to > be NULL without that. Just to reduce indentation by one tab. No technical reasons. The code had to wrap at 80 column was becoming unreadable. > >> + for (i = 0; freq_table[i].frequency != CPUFREQ_TABLE_END; i++) { >> + if (boost_freqs[j] == freq_table[i].frequency) { > > Use cpufreq_frequency_table_get_index() instead. Okay. Thanks for pointing out. > >> + freq_table[i].flags |= CPUFREQ_BOOST_FREQ; >> + break; >> + } >> + } >> + if (freq_table[i].frequency == CPUFREQ_TABLE_END) >> + pr_err("%s: invalid boost frequency %d\n", >> + __func__, boost_freqs[j]); >> + } >> + >> + kfree(boost_freqs); >> +#endif >> + >> out: >> rcu_read_unlock(); >> if (ret) >> -- >> 1.7.4.4 >>
On Wed, May 14, 2014 at 11:54 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 14 May 2014 11:39, Lukasz Majewski <l.majewski@samsung.com> wrote: >> I agree with Nishanth here, that point 1 (as described by Viresh at >> [*]) is a more scalable approach. > > The only reason why I wanted all that to be done at OPP level was to > ensure if somebody else also needs it apart from cpufreq, they don't have > to duplicate code and find it.. As it is present at a central place.. > > But if no other code is going to look for that, it may just be fine as is.. Yes, I agree with Nishanth and Lukasz for a separate binding.
On 14 May 2014 19:16, Thomas Abraham <ta.omasab@gmail.com> wrote: > On Wed, May 14, 2014 at 9:10 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote: >> Why is this present outside of above if {} ? as boost_freqs is guaranteed to >> be NULL without that. > > Just to reduce indentation by one tab. No technical reasons. The code > had to wrap at 80 column was becoming unreadable. That's bad :), you have added an extra comparison just for that :( Instead kill the below indentation by doing a "goto out" with a ! of below expression: >> + if (of_find_property(dev->of_node, "boost-frequency", &len)) {
On 05/14/2014 01:24 AM, Viresh Kumar wrote: > On 14 May 2014 11:39, Lukasz Majewski <l.majewski@samsung.com> wrote: >> I agree with Nishanth here, that point 1 (as described by Viresh at >> [*]) is a more scalable approach. > > The only reason why I wanted all that to be done at OPP level was to > ensure if somebody else also needs it apart from cpufreq, they don't have > to duplicate code and find it.. As it is present at a central place.. > > But if no other code is going to look for that, it may just be fine as is.. > If we eventually have a need beyond cpufreq (say devfreq) with similar instances, then it makes sense to move it out to a generic place. Either way, code implementation/duplication is a OS problem - and should be looked at independent of the description in dts. If we feel the description is valid hardware description (which, personally, I do), then lets go to the next discussion point of where to put it - generic or cpufreq specific (here, I have no preference), and finally decide the implementation as necessary as a result of the description.
diff --git a/drivers/cpufreq/cpufreq_opp.c b/drivers/cpufreq/cpufreq_opp.c index c0c6f4a..e3c97f3 100644 --- a/drivers/cpufreq/cpufreq_opp.c +++ b/drivers/cpufreq/cpufreq_opp.c @@ -19,6 +19,7 @@ #include <linux/pm_opp.h> #include <linux/rcupdate.h> #include <linux/slab.h> +#include <linux/of.h> /** * dev_pm_opp_init_cpufreq_table() - create a cpufreq table for a device @@ -51,6 +52,10 @@ int dev_pm_opp_init_cpufreq_table(struct device *dev, struct cpufreq_frequency_table *freq_table = NULL; int i, max_opps, ret = 0; unsigned long rate; +#ifdef CONFIG_CPU_FREQ_BOOST_SW + int j, len; + u32 *boost_freqs = NULL; +#endif rcu_read_lock(); @@ -82,6 +87,40 @@ int dev_pm_opp_init_cpufreq_table(struct device *dev, *table = &freq_table[0]; +#ifdef CONFIG_CPU_FREQ_BOOST_SW + if (of_find_property(dev->of_node, "boost-frequency", &len)) { + if (len == 0 || (len & (sizeof(u32) - 1)) != 0) { + dev_err(dev, "%s: invalid boost frequency\n", __func__); + ret = -EINVAL; + goto out; + } + + boost_freqs = kzalloc(len, GFP_KERNEL); + if (!boost_freqs) { + dev_warn(dev, "%s: no memory for boost freq table\n", + __func__); + ret = -ENOMEM; + goto out; + } + of_property_read_u32_array(dev->of_node, "boost-frequency", + boost_freqs, len / sizeof(u32)); + } + + for (j = 0; j < len / sizeof(u32) && boost_freqs; j++) { + for (i = 0; freq_table[i].frequency != CPUFREQ_TABLE_END; i++) { + if (boost_freqs[j] == freq_table[i].frequency) { + freq_table[i].flags |= CPUFREQ_BOOST_FREQ; + break; + } + } + if (freq_table[i].frequency == CPUFREQ_TABLE_END) + pr_err("%s: invalid boost frequency %d\n", + __func__, boost_freqs[j]); + } + + kfree(boost_freqs); +#endif + out: rcu_read_unlock(); if (ret)