| Message ID | 1344179121-17738-3-git-send-email-shawn.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/05/2012 10:05 AM, Shawn Guo wrote: > With a lot of devices booting from device tree nowadays, it requires > that OPP table can be initialized from device tree. The patch adds > a helper function of_init_opp_table together with a binding doc for > that purpose. > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > --- > Documentation/devicetree/bindings/power/opp.txt | 25 +++++++++ > drivers/base/power/opp.c | 65 +++++++++++++++++++++++ > include/linux/opp.h | 8 +++ > 3 files changed, 98 insertions(+), 0 deletions(-) > create mode 100644 Documentation/devicetree/bindings/power/opp.txt > > diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt > new file mode 100644 > index 0000000..2238520 > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/opp.txt > @@ -0,0 +1,25 @@ > +* Generic OPP Interface > + > +SoCs have a standard set of tuples consisting of frequency and > +voltage pairs that the device will support per voltage domain. These > +are called Operating Performance Points or OPPs. > + > +Properties: > +- operating-points: An array of 3-tuples items, and each item consists 3 tuples? > + of frequency and voltage like <freq-kHz vol-uV>. > + freq: clock frequency in kHz > + vol: voltage in microvolt Although maybe 3 fields would be good for a flags field? I'm concerned it's a pretty generic name and not very future proof. What about transition times? Not sure how you would represent that as it probably depends on which points you are changing between rather than a property of the opp. I'm not saying we have to address that now, but just have some path in the future. > + > +Examples: > + > +cpu@0 { > + compatible = "arm,cortex-a9"; > + reg = <0>; > + next-level-cache = <&L2>; > + operating-points = < > + /* kHz uV */ > + 792000 1100000 > + 396000 950000 > + 198000 850000 > + >; > +}; > diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c > index ac993ea..1bf1be8 100644 > --- a/drivers/base/power/opp.c > +++ b/drivers/base/power/opp.c > @@ -22,6 +22,7 @@ > #include <linux/rculist.h> > #include <linux/rcupdate.h> > #include <linux/opp.h> > +#include <linux/of.h> > > /* > * Internal data structure organization with the OPP layer library is as > @@ -674,3 +675,67 @@ struct srcu_notifier_head *opp_get_notifier(struct device *dev) > > return &dev_opp->head; > } > + > +#ifdef CONFIG_OF > +/** > + * of_init_opp_table() - Initialize opp table from device tree > + * @dev: device pointer used to lookup device OPPs. > + * > + * Register the initial OPP table with the OPP library for given device. > + */ > +int of_init_opp_table(struct device *dev) > +{ > + struct device_node *np = dev->of_node; > + const char *propname = "operating-points"; > + const struct property *pp; > + u32 *opp; > + int ret, i, nr; > + > + pp = of_find_property(np, propname, NULL); > + if (!pp) { > + dev_err(dev, "%s: Unable to find property", __func__); > + return -ENODEV; > + } > + > + opp = kzalloc(pp->length, GFP_KERNEL); > + if (!opp) { > + dev_err(dev, "%s: Unable to allocate array\n", __func__); > + return -ENOMEM; > + } > + > + nr = pp->length / sizeof(u32); > + ret = of_property_read_u32_array(np, propname, opp, nr); > + if (ret) { > + dev_err(dev, "%s: Unable to read OPPs\n", __func__); > + goto out; > + } > + > + if (nr % 2) { > + dev_err(dev, "%s: Invalid OPP list\n", __func__); > + ret = -EINVAL; > + goto out; > + } > + > + nr /= 2; > + for (i = 0; i < nr; i++) { > + /* > + * Each OPP is a set of tuples consisting of frequency and > + * voltage like <freq-kHz vol-uV>. > + */ > + u32 *val = opp + i * 2; > + > + val[0] *= 1000; > + ret = opp_add(dev, val[0], val[1]); > + if (ret) { > + dev_warn(dev, "%s: Failed to add OPP %d: %d\n", > + __func__, val[0], ret); > + continue; > + } > + } I think this whole function can be written more concisely. Just iterate over the property and avoid the intermediate array allocation. Rob
On Sun, Aug 05, 2012 at 09:50:32PM -0500, Rob Herring wrote: > > +Properties: > > +- operating-points: An array of 3-tuples items, and each item consists > > 3 tuples? > It's the case of v1, and I forgot updating it. Thanks for spotting it. > > + of frequency and voltage like <freq-kHz vol-uV>. > > + freq: clock frequency in kHz > > + vol: voltage in microvolt > > Although maybe 3 fields would be good for a flags field? I'm concerned > it's a pretty generic name and not very future proof. What about > transition times? Not sure how you would represent that as it probably > depends on which points you are changing between rather than a property > of the opp. > This is a binding for OPP, which does not define transition times. As for cpufreq, we only need to represent a possible maximum transition latency. The driver will ask regulator subsystem for voltage latency, while the clock latency is defined in DT. > I think this whole function can be written more concisely. Just iterate > over the property and avoid the intermediate array allocation. > I'm not sure about that, since directly iterating over the property means we have to take care of all these sanity checks done in API of_property_read_u32_array().
On 08/05/2012 10:19 PM, Shawn Guo wrote: > On Sun, Aug 05, 2012 at 09:50:32PM -0500, Rob Herring wrote: >>> +Properties: >>> +- operating-points: An array of 3-tuples items, and each item consists >> >> 3 tuples? >> > It's the case of v1, and I forgot updating it. Thanks for spotting it. > >>> + of frequency and voltage like <freq-kHz vol-uV>. >>> + freq: clock frequency in kHz >>> + vol: voltage in microvolt >> >> Although maybe 3 fields would be good for a flags field? I'm concerned >> it's a pretty generic name and not very future proof. What about >> transition times? Not sure how you would represent that as it probably >> depends on which points you are changing between rather than a property >> of the opp. >> > This is a binding for OPP, which does not define transition times. > > As for cpufreq, we only need to represent a possible maximum transition > latency. The driver will ask regulator subsystem for voltage latency, > while the clock latency is defined in DT. > >> I think this whole function can be written more concisely. Just iterate >> over the property and avoid the intermediate array allocation. >> > I'm not sure about that, since directly iterating over the property > means we have to take care of all these sanity checks done in API > of_property_read_u32_array(). > This won't work?: of_property_for_each_u32(...) { if (i & 0x1) { volt = val; ret = opp_add(dev, freq, volt); if (ret) ... } else freq = val * 1000; i++; } Rob
On Sun, Aug 05, 2012 at 11:43:04PM -0500, Rob Herring wrote: > On 08/05/2012 10:19 PM, Shawn Guo wrote: > > On Sun, Aug 05, 2012 at 09:50:32PM -0500, Rob Herring wrote: > >> I think this whole function can be written more concisely. Just iterate > >> over the property and avoid the intermediate array allocation. > >> > > I'm not sure about that, since directly iterating over the property > > means we have to take care of all these sanity checks done in API > > of_property_read_u32_array(). > > > > This won't work?: > It should work. But I rewrote the function like below after I find the sanity check is simple enough to take care of it on our own. The code does look more concise and still easy to read. Regards, Shawn int of_init_opp_table(struct device *dev) { const struct property *prop; const __be32 *val; int nr; prop = of_find_property(dev->of_node, "operating-points", NULL); if (!prop) return -ENODEV; if (!prop->value) return -ENODATA; /* * Each OPP is a set of tuples consisting of frequency and * voltage like <freq-kHz vol-uV>. */ nr = prop->length / sizeof(u32); if (nr % 2) { dev_err(dev, "%s: Invalid OPP list\n", __func__); return -EINVAL; } val = prop->value; while (nr) { unsigned long freq = be32_to_cpup(val++) * 1000; unsigned long volt = be32_to_cpup(val++); if (opp_add(dev, freq, volt)) { dev_warn(dev, "%s: Failed to add OPP %ld\n", __func__, freq); continue; } nr -= 2; } return 0; }
diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt new file mode 100644 index 0000000..2238520 --- /dev/null +++ b/Documentation/devicetree/bindings/power/opp.txt @@ -0,0 +1,25 @@ +* Generic OPP Interface + +SoCs have a standard set of tuples consisting of frequency and +voltage pairs that the device will support per voltage domain. These +are called Operating Performance Points or OPPs. + +Properties: +- operating-points: An array of 3-tuples items, and each item consists + of frequency and voltage like <freq-kHz vol-uV>. + freq: clock frequency in kHz + vol: voltage in microvolt + +Examples: + +cpu@0 { + compatible = "arm,cortex-a9"; + reg = <0>; + next-level-cache = <&L2>; + operating-points = < + /* kHz uV */ + 792000 1100000 + 396000 950000 + 198000 850000 + >; +}; diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index ac993ea..1bf1be8 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -22,6 +22,7 @@ #include <linux/rculist.h> #include <linux/rcupdate.h> #include <linux/opp.h> +#include <linux/of.h> /* * Internal data structure organization with the OPP layer library is as @@ -674,3 +675,67 @@ struct srcu_notifier_head *opp_get_notifier(struct device *dev) return &dev_opp->head; } + +#ifdef CONFIG_OF +/** + * of_init_opp_table() - Initialize opp table from device tree + * @dev: device pointer used to lookup device OPPs. + * + * Register the initial OPP table with the OPP library for given device. + */ +int of_init_opp_table(struct device *dev) +{ + struct device_node *np = dev->of_node; + const char *propname = "operating-points"; + const struct property *pp; + u32 *opp; + int ret, i, nr; + + pp = of_find_property(np, propname, NULL); + if (!pp) { + dev_err(dev, "%s: Unable to find property", __func__); + return -ENODEV; + } + + opp = kzalloc(pp->length, GFP_KERNEL); + if (!opp) { + dev_err(dev, "%s: Unable to allocate array\n", __func__); + return -ENOMEM; + } + + nr = pp->length / sizeof(u32); + ret = of_property_read_u32_array(np, propname, opp, nr); + if (ret) { + dev_err(dev, "%s: Unable to read OPPs\n", __func__); + goto out; + } + + if (nr % 2) { + dev_err(dev, "%s: Invalid OPP list\n", __func__); + ret = -EINVAL; + goto out; + } + + nr /= 2; + for (i = 0; i < nr; i++) { + /* + * Each OPP is a set of tuples consisting of frequency and + * voltage like <freq-kHz vol-uV>. + */ + u32 *val = opp + i * 2; + + val[0] *= 1000; + ret = opp_add(dev, val[0], val[1]); + if (ret) { + dev_warn(dev, "%s: Failed to add OPP %d: %d\n", + __func__, val[0], ret); + continue; + } + } + + ret = 0; +out: + kfree(opp); + return ret; +} +#endif diff --git a/include/linux/opp.h b/include/linux/opp.h index 2a4e5fa..214e0eb 100644 --- a/include/linux/opp.h +++ b/include/linux/opp.h @@ -48,6 +48,14 @@ int opp_disable(struct device *dev, unsigned long freq); struct srcu_notifier_head *opp_get_notifier(struct device *dev); +#ifdef CONFIG_OF +int of_init_opp_table(struct device *dev); +#else +static inline int of_init_opp_table(struct device *dev) +{ + return -EINVAL; +} +#endif /* CONFIG_OF */ #else static inline unsigned long opp_get_voltage(struct opp *opp) {
With a lot of devices booting from device tree nowadays, it requires that OPP table can be initialized from device tree. The patch adds a helper function of_init_opp_table together with a binding doc for that purpose. Signed-off-by: Shawn Guo <shawn.guo@linaro.org> --- Documentation/devicetree/bindings/power/opp.txt | 25 +++++++++ drivers/base/power/opp.c | 65 +++++++++++++++++++++++ include/linux/opp.h | 8 +++ 3 files changed, 98 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/power/opp.txt