Re: [PATCH 2/3] PM / OPP: Initialize OPP table from device tree
On Fri, Jul 20, 2012 at 3:46 AM, Shawn Guo 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 .
>> > +*/
>> > + 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
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 2/3] PM / OPP: Initialize OPP table from device tree
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 = <
> > + /* kHzuVen */
> > + 120 1275000 0
> > + 996000 1225000 1
> > + 792000 110 1
> > + 672000 110 0
> > + 396000 95 1
> > + 198000 85 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
> > #include
> > #include
> > +#include
> >
> > /*
> > * 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 .
> > +*/
> > + 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
Re: [PATCH 2/3] PM / OPP: Initialize OPP table from device tree
On Thu, Jul 19, 2012 at 10:54 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.
nice to see this happen, a quick feedback:
>
> Signed-off-by: Shawn Guo
> ---
> 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 000..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: 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 = <
> + /* kHzuVen */
> + 120 1275000 0
> + 996000 1225000 1
> + 792000 110 1
> + 672000 110 0
> + 396000 95 1
> + 198000 85 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
> #include
> #include
> +#include
>
> /*
> * 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 .
> +*/
> + 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]) {
> +
[PATCH 2/3] PM / OPP: Initialize OPP table from device tree
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
---
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 000..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: 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 = <
+ /* kHzuVen */
+ 120 1275000 0
+ 996000 1225000 1
+ 792000 110 1
+ 672000 110 0
+ 396000 95 1
+ 198000 85 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
#include
#include
+#include
/*
* 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 .
+*/
+ 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)
{
--
1.7.5.4
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
