Re: [PATCH 05/10] opp: Add support to parse "operating-points-v2" bindings

Viresh Kumar
2015-07-02T06:38:28Z

On 01-07-15, 18:13, Stephen Boyd wrote:
> On 06/15/2015 04:57 AM, Viresh Kumar wrote:
> > ---
> > drivers/base/power/opp.c | 238 ++++++++++++++++++++++++++++++++++++++++++-----
> > 1 file changed, 213 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
> > index 2ac48ff9c1ef..3198c3e77224 100644
> > --- a/drivers/base/power/opp.c
> > +++ b/drivers/base/power/opp.c
> > @@ -49,12 +49,17 @@
> > * are protected by the dev_opp_list_lock for integrity.
> > * IMPORTANT: the opp nodes should be maintained in increasing
> > * order.
> > - * @dynamic: not-created from static DT entries.
> > * @available: true/false - marks if this OPP as available or not
> > + * @dynamic: not-created from static DT entries.
>
> Why move dynamic?

To match its position, as it is present in the struct below. Yes it
could have been done in a separate patch, but its also fine to fix
such silly mistakes in another patch :)

> > + * @turbo: true if turbo (boost) OPP
> > * @rate: Frequency in hertz
> > - * @u_volt: Nominal voltage in microvolts corresponding to this OPP
> > + * @u_volt: Target voltage in microvolts corresponding to this OPP
> > + * @u_volt_min: Minimum voltage in microvolts corresponding to this OPP
> > + * @u_volt_max: Maximum voltage in microvolts corresponding to this OPP
> > + * @u_amp: Maximum current drawn by the device in microamperes
> > * @dev_opp: points back to the device_opp struct this opp belongs to
> > * @rcu_head: RCU callback head used for deferred freeing
> > + * @np: OPP's device node.
> > *
> > * This structure stores the OPP information for a given device.
> > */
> > @@ -63,11 +68,22 @@ struct dev_pm_opp {
> >
> > bool available;
> > bool dynamic;
> > + bool turbo;
> > unsigned long rate;
> > +
> > + /*
> > + * Order in which u_volt{_min|_max} are present in this structure
> > + * shouldn't be changed.
> > + */
> > unsigned long u_volt;
> > + unsigned long u_volt_min;
> > + unsigned long u_volt_max;
> > + unsigned long u_amp;
> >
> > struct device_opp *dev_opp;
> > struct rcu_head rcu_head;
> > +
> > + struct device_node *np;
> > };
> >
> > /**
> > @@ -501,6 +517,7 @@ static void _opp_remove(struct device_opp *dev_opp,
> > */
> > if (notify)
> > srcu_notifier_call_chain(&dev_opp->srcu_head, OPP_EVENT_REMOVE, opp);
> > +
> > list_del_rcu(&opp->node);
> > call_srcu(&dev_opp->srcu_head.srcu, &opp->rcu_head, _kfree_opp_rcu);
> >
>
> Please remove this hunk of noise.

Sigh

> > @@ -675,6 +692,100 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq,
> > }
> >
> > /**
> > + * _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings)
> > + * @dev: device for which we do this operation
> > + * @np: device node
> > + *
> > + * This function adds an opp definition to the opp list and returns status. The
> > + * opp can be controlled using dev_pm_opp_enable/disable functions and may be
> > + * removed by dev_pm_opp_remove.
> > + *
> > + * Locking: The internal device_opp and opp structures are RCU protected.
> > + * Hence this function internally uses RCU updater strategy with mutex locks
> > + * to keep the integrity of the internal data structures. Callers should ensure
> > + * that this function is *NOT* called under RCU protection or in contexts where
> > + * mutex cannot be locked.
> > + *
> > + * Return:
> > + * 0 On success OR
> > + * Duplicate OPPs (both freq and volt are same) and opp->available
> > + * -EEXIST Freq are same and volt are different OR
> > + * Duplicate OPPs (both freq and volt are same) and !opp->available
> > + * -ENOMEM Memory allocation failure
> > + * -EINVAL Failed parsing the OPP node
> > + */
> > +static int _opp_add_static_v2(struct device *dev, struct device_node *np)
> > +{
> > + struct device_opp *dev_opp;
> > + struct dev_pm_opp *new_opp;
> > + int ret;
> > +
> > + /* Hold our list modification lock here */
> > + mutex_lock(&dev_opp_list_lock);
> > +
> > + new_opp = _allocate_opp(dev, &dev_opp);
> > + if (!new_opp) {
> > + ret = -ENOMEM;
> > + goto unlock;
> > + }
> > +
> > + ret = of_property_read_u32(np, "opp-hz", (u32 *)&new_opp->rate);
> > + if (ret < 0) {
> > + dev_err(dev, "%s: opp-hz not found\n", __func__);
> > + goto free_opp;
> > + }
> > +
> > + if (of_get_property(np, "turbo-mode", NULL))
> > + new_opp->turbo = true;
>
> new_opp->turbo = of_property_read_bool(np, "turbo-mode");

Sure.

> > +
> > + new_opp->np = np;
> > + new_opp->dynamic = false;
> > + new_opp->available = true;
> > +
> > + /*
> > + * TODO: Support multiple regulators
> > + *
> > + * read opp-microvolt array
> > + */
> > + ret = of_property_count_u32_elems(np, "opp-microvolt");
> > + if (ret == 1 || ret == 3) {
> > + /* There can be one or three elements here */
> > + ret = of_property_read_u32_array(np, "opp-microvolt",
> > + (u32 *)&new_opp->u_volt, ret);
>
> It seems fragile to rely on the struct packing here. Maybe the same
> comment in the struct should be copied here, and possibly some better
> way of doing this so the code can't be subtly broken?

Any example of how things will break? Aren't these guaranteed to be
present at 3 consecutive 32 bit positions ?

> > +
> > + pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu\n",
> > + __func__, new_opp->turbo, new_opp->rate, new_opp->u_volt,
> > + new_opp->u_volt_min, new_opp->u_volt_max);
> > +
> > + mutex_unlock(&dev_opp_list_lock);
>
> We can pr_debug after the unlock?

Okay

--
viresh