| Message ID | 1342713281-31114-3-git-send-email-shawn.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 19, 2012 at 10:54 AM, Shawn Guo <shawn.guo@linaro.org> 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. nice to see this happen, a quick feedback: > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > --- > Documentation/devicetree/bindings/power/opp.txt | 29 ++++++++++ > drivers/base/power/opp.c | 66 +++++++++++++++++++++++ > include/linux/opp.h | 4 ++ > 3 files changed, 99 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..1dd0db2 > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/opp.txt > @@ -0,0 +1,29 @@ > +* 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, voltage and enabling like <freq vol en>. > + freq: clock frequency in kHz > + vol: voltage in microvolt > + en: initially enabled (1) or not (0) > + > +Examples: > + > +cpu@0 { > + compatible = "arm,cortex-a9"; I am not sure how this works - would an example of OMAP4430, 60, 70 help? they have completely different OPP entries. > + reg = <0>; > + next-level-cache = <&L2>; > + operating-points = < > + /* kHz uV en */ > + 1200000 1275000 0 > + 996000 1225000 1 > + 792000 1100000 1 > + 672000 1100000 0 > + 396000 950000 1 > + 198000 850000 1 Just my 2cents, If we change en to be a flag: 0 - add, but disable 1 - add (enabled) we could extend this further if the definition is a flag, for example: 2 - add and disable IF any of notifier chain return failure 3- add but dont call notifier chain (e.g. OPPs that are present for All SoC) in addition, SoC might have additional properties associated with each OPP such a flag could be split up to mean lower 16 bits as OPP library flag and higher 16 bit to mean SoC custom flag Example - On certain SoC a specific type of power technique is supposed to be used per OPP, such a flag passed on via notifiers to SoC handler might be capable of centralizing the OPP information into the DT. > + >; > +}; > diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c > index ac993ea..2d750f9 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,68 @@ 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); error warn if the pp->length is not multiple of u32? we also expect later on to be a multiple of 3(f,v,disable > + ret = of_property_read_u32_array(np, propname, opp, nr); > + if (ret) { > + dev_err(dev, "%s: Unable to read OPPs\n", __func__); > + goto out; > + } > + > + nr /= 3; > + for (i = 0; i < nr; i++) { > + /* > + * Each OPP is a set of tuples consisting of frequency, > + * voltage and availability like <freq-kHz vol-uV enable>. > + */ > + u32 *val = opp + i * 3; > + > + 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; > + } > + > + if (!val[2]) { > + ret = opp_disable(dev, val[0]); Since we are updating the table out of context of the SoC users, consider what might happen if someone where to operate on the OPP after opp_add, but before opp_disable? instead of having the pain of adding an OPP and then disabling it, since the code will now move to core OPP library itself, wont it be better to hold dev_opp_list_lock and update the full table with a proper list walk - yes the current opp_add and opp_disable apis would need refactoring to be reusable. It will also save on synchronize_rcu calls on multiple iterations of the list. > + if (ret) > + dev_warn(dev, "%s: Failed to disable OPP %d: %d\n", > + __func__, val[0], ret); umm... but we override the return with 0? OPP add failure might indicate the table is invalid/corrupted - or no memory. What is the point in populating a bad table up? having a singular function with direct access to internal structures might save us these un-necessary dilemma. > + } > + } > + > + ret = 0; > +out: > + kfree(opp); > + return ret; > +} > +#endif > diff --git a/include/linux/opp.h b/include/linux/opp.h > index 2a4e5fa..fd165ad 100644 > --- a/include/linux/opp.h > +++ b/include/linux/opp.h > @@ -48,6 +48,10 @@ 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 > + > #else > static inline unsigned long opp_get_voltage(struct opp *opp) > { Regards, Nishanth Menon
Thanks for reviewing it, Nishanth. On Fri, Jul 20, 2012 at 01:00:26AM -0500, Menon, Nishanth wrote: > > +cpu@0 { > > + compatible = "arm,cortex-a9"; > I am not sure how this works - would an example of OMAP4430, 60, 70 > help? they have completely different OPP entries. > Basically, patch #3 is a user of this and shows how this works. > > + reg = <0>; > > + next-level-cache = <&L2>; > > + operating-points = < > > + /* kHz uV en */ > > + 1200000 1275000 0 > > + 996000 1225000 1 > > + 792000 1100000 1 > > + 672000 1100000 0 > > + 396000 950000 1 > > + 198000 850000 1 > > Just my 2cents, If we change en to be a flag: > 0 - add, but disable > 1 - add (enabled) > we could extend this further if the definition is a flag, for example: > 2 - add and disable IF any of notifier chain return failure > 3- add but dont call notifier chain (e.g. OPPs that are present for All SoC) > > in addition, SoC might have additional properties associated with each > OPP such a flag > could be split up to mean lower 16 bits as OPP library flag and higher > 16 bit to mean SoC custom flag > > Example - On certain SoC a specific type of power technique is > supposed to be used per OPP, such a flag > passed on via notifiers to SoC handler might be capable of > centralizing the OPP information into the DT. > I do not follow on the idea of having the third tuple being a flag. The binding is only meant to represent the aspects of operating-points, while operating-points are all about frequency and voltage, nothing else. No Linux implementation details should be encoded here. I'm even a little unhappy about having enabling/availability in the third tuple. If anyone complains about that, I will remove it from the binding without a hesitation. > > + >; > > +}; > > diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c > > index ac993ea..2d750f9 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,68 @@ 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); > error warn if the pp->length is not multiple of u32? The DT core ensures that any integer encoded there will an u32. > we also expect > later on to be a multiple of 3(f,v,disable > Yes, I thought about checking that, but I chose to simply ignore those suspicious tuples at the end of the list. I will add the check for that, since you commented here. > > + ret = of_property_read_u32_array(np, propname, opp, nr); > > + if (ret) { > > + dev_err(dev, "%s: Unable to read OPPs\n", __func__); > > + goto out; > > + } > > + > > + nr /= 3; > > + for (i = 0; i < nr; i++) { > > + /* > > + * Each OPP is a set of tuples consisting of frequency, > > + * voltage and availability like <freq-kHz vol-uV enable>. > > + */ > > + u32 *val = opp + i * 3; > > + > > + 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; > > + } > > + > > + if (!val[2]) { > > + ret = opp_disable(dev, val[0]); > Since we are updating the table out of context of the SoC users, > consider what might happen if someone where to operate on the OPP > after opp_add, but before opp_disable? I would take this as another comment reminding me to remove the enabling/availability tuple from the binding. Will do it in the next version. > instead of having the pain of adding an OPP and then disabling it, > since the code will now move to core OPP > library itself, wont it be better to hold dev_opp_list_lock and update > the full table with a proper list walk - yes > the current opp_add and opp_disable apis would need refactoring to be > reusable. It will also save on > synchronize_rcu calls on multiple iterations of the list. > > > > + if (ret) > > + dev_warn(dev, "%s: Failed to disable OPP %d: %d\n", > > + __func__, val[0], ret); > > umm... but we override the return with 0? OPP add failure might > indicate the table is invalid/corrupted - or no memory. > What is the point in populating a bad table up? having a singular > function with direct access to internal structures > might save us these un-necessary dilemma. > The return overriding only happens when opp_add or opp_disable call fails on particular entry. That does not necessarily mean the whole OPP table is completely invalid/corrupted. A warn message is enough, IMO. > > > + } > > + } > > + > > + ret = 0; > > +out: > > + kfree(opp); > > + return ret; > > +} > > +#endif > > diff --git a/include/linux/opp.h b/include/linux/opp.h > > index 2a4e5fa..fd165ad 100644 > > --- a/include/linux/opp.h > > +++ b/include/linux/opp.h > > @@ -48,6 +48,10 @@ 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; } > ? > Sounds good. Regards, Shawn > > +#endif > > + > > #else > > static inline unsigned long opp_get_voltage(struct opp *opp) > > { > > > Regards, > Nishanth Menon
On Fri, Jul 20, 2012 at 3:46 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > >> > + ret = of_property_read_u32_array(np, propname, opp, nr); >> > + if (ret) { >> > + dev_err(dev, "%s: Unable to read OPPs\n", __func__); >> > + goto out; >> > + } >> > + >> > + nr /= 3; >> > + for (i = 0; i < nr; i++) { >> > + /* >> > + * Each OPP is a set of tuples consisting of frequency, >> > + * voltage and availability like <freq-kHz vol-uV enable>. >> > + */ >> > + u32 *val = opp + i * 3; >> > + >> > + 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; >> > + } >> > + >> > + if (!val[2]) { >> > + ret = opp_disable(dev, val[0]); >> Since we are updating the table out of context of the SoC users, >> consider what might happen if someone where to operate on the OPP >> after opp_add, but before opp_disable? > > I would take this as another comment reminding me to remove the > enabling/availability tuple from the binding. Will do it in the > next version. I am not completely convinced that dropping the flag would be the best approach on a long run, but might be a good starting point while we meet current reqs and as a need arises, could increase the scope. Thanks once again for doing this. Regards, Nishanth Menon
diff --git a/Documentation/devicetree/bindings/power/opp.txt b/Documentation/devicetree/bindings/power/opp.txt new file mode 100644 index 0000000..1dd0db2 --- /dev/null +++ b/Documentation/devicetree/bindings/power/opp.txt @@ -0,0 +1,29 @@ +* 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, voltage and enabling like <freq vol en>. + freq: clock frequency in kHz + vol: voltage in microvolt + en: initially enabled (1) or not (0) + +Examples: + +cpu@0 { + compatible = "arm,cortex-a9"; + reg = <0>; + next-level-cache = <&L2>; + operating-points = < + /* kHz uV en */ + 1200000 1275000 0 + 996000 1225000 1 + 792000 1100000 1 + 672000 1100000 0 + 396000 950000 1 + 198000 850000 1 + >; +}; diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index ac993ea..2d750f9 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,68 @@ 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; + } + + nr /= 3; + for (i = 0; i < nr; i++) { + /* + * Each OPP is a set of tuples consisting of frequency, + * voltage and availability like <freq-kHz vol-uV enable>. + */ + u32 *val = opp + i * 3; + + 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; + } + + if (!val[2]) { + ret = opp_disable(dev, val[0]); + if (ret) + dev_warn(dev, "%s: Failed to disable OPP %d: %d\n", + __func__, val[0], ret); + } + } + + ret = 0; +out: + kfree(opp); + return ret; +} +#endif diff --git a/include/linux/opp.h b/include/linux/opp.h index 2a4e5fa..fd165ad 100644 --- a/include/linux/opp.h +++ b/include/linux/opp.h @@ -48,6 +48,10 @@ 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); +#endif + #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 | 29 ++++++++++ drivers/base/power/opp.c | 66 +++++++++++++++++++++++ include/linux/opp.h | 4 ++ 3 files changed, 99 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/power/opp.txt