>>-----Original Message-----
>>From: Kevin Hilman [mailto:[email protected]]
>>Sent: Wednesday, August 04, 2010 6:03 AM
>>To: Gopinath, Thara
>>Cc: [email protected]; [email protected]; Cousson, Benoit; Sripathy,
>>Vishwanath; Sawant, Anand;
>>Basak, Partha
>>Subject: Re: [RFC 3/7] OMAP: Introduce voltage domain pointer and device
>>specific set rate and get
>>rate in device opp structures.
>>
>>Thara Gopinath <[email protected]> writes:
>>
>>> This patch extends the device opp structure to contain
>>> info about the voltage domain to which the device belongs to
>>> and to contain pointers to scale the operating rate of the
>>> device. This patch also adds an API in the opp layer that
>>> can be used by the voltage layer to get a list of all the
>>> scalable devices belonging to a particular voltage domain.
>>> This API is to be typically called only once by the voltage
>>> layer per voltage domain instance and the device list should
>>> be stored. This approach makes it easy during dvfs to scale
>>> all the devices associated with a voltage domain and then
>>> scale the voltage domain.
>>>
>>> Signed-off-by: Thara Gopinath <[email protected]>
>>> ---
>>> arch/arm/plat-omap/include/plat/opp.h | 37 +++++++++++++++++++++++++-
>>> arch/arm/plat-omap/opp.c | 47
>>> +++++++++++++++++++++++++-------
>>> 2 files changed, 72 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/arch/arm/plat-omap/include/plat/opp.h
>>> b/arch/arm/plat-omap/include/plat/opp.h
>>> index 893731f..15e1e70 100644
>>> --- a/arch/arm/plat-omap/include/plat/opp.h
>>> +++ b/arch/arm/plat-omap/include/plat/opp.h
>>> @@ -16,6 +16,7 @@
>>>
>>> #include <linux/err.h>
>>> #include <linux/cpufreq.h>
>>> +#include <linux/clk.h>
>>>
>>> #include <plat/common.h>
>>>
>>> @@ -38,21 +39,45 @@
>>> */
>>> struct omap_opp_def {
>>> char *hwmod_name;
>>> + char *vdd_name;
>>>
>>> unsigned long freq;
>>> unsigned long u_volt;
>>>
>>> + int (*set_rate)(struct device *dev, unsigned long rate);
>>> + unsigned long (*get_rate) (struct device *dev);
>>> +
>>> bool enabled;
>>> };
>>>
>>> +struct device_opp {
>>> + struct list_head node;
>>> + char *vdd_name;
>>> +
>>> + struct omap_hwmod *oh;
>>> + struct device *dev;
>>> + struct omap_volt_domain *volt_domain;
>>> +
>>> + struct list_head opp_list;
>>> + u32 opp_count;
>>> + u32 enabled_opp_count;
>>> +
>>> + int (*set_rate)(struct device *dev, unsigned long rate);
>>> + unsigned long (*get_rate) (struct device *dev);
>>> +};
>>
>>I don't like moving the definition of this struct out. This exposes the
>>implmentation details of the OPP layer that are subject to change.
>>
>>A quick glance shows you need to access the volt_domain field and the
>>new function pointers.
>>
>>The voltage domain should be part of the hwmod as suggested by Benoit,
>>and an API created to get the voltage domain from the hwmod.
>>
>>For the get/set rate functions, maybe new OPP layer API should be
>>created access those functions. opp_[get|set]_rate()?
Yes I could do this.
>>
>>
>>> /*
>>> * Initialization wrapper used to define an OPP.
>>> * To point at the end of a terminator of a list of OPPs,
>>> * use OMAP_OPP_DEF(0, 0, 0)
>>> */
>>> -#define OMAP_OPP_DEF(_hwmod_name, _enabled, _freq, _uv) \
>>> +#define OMAP_OPP_DEF(_hwmod_name, _vdd_name, _set_rate, _get_rate, \
>>> + _enabled, _freq, _uv) \
>>> { \
>>> .hwmod_name = _hwmod_name, \
>>> + .vdd_name = _vdd_name, \
>>> + .set_rate = _set_rate, \
>>> + .get_rate = _get_rate, \
>>> .enabled = _enabled, \
>>> .freq = _freq, \
>>> .u_volt = _uv, \
>>> @@ -77,6 +102,8 @@ struct omap_opp *opp_find_freq_ceil(struct device *dev,
>>> unsigned long *freq);
>>>
>>> struct omap_opp *opp_find_voltage(struct device *dev, unsigned long volt);
>>>
>>> +struct device_opp *opp_find_dev_opp(struct device *dev);
>>> +
>>> int opp_add(const struct omap_opp_def *opp_def);
>>>
>>> int opp_enable(struct omap_opp *opp);
>>> @@ -89,6 +116,9 @@ u8 __deprecated opp_get_opp_id(struct omap_opp *opp);
>>>
>>> void opp_init_cpufreq_table(struct device *dev,
>>> struct cpufreq_frequency_table **table);
>>> +
>>> +struct device **opp_init_voltage_params(struct omap_volt_domain
>>> *volt_domain,
>>> + int *dev_count);
>>> #else
>>> static inline unsigned long opp_get_voltage(const struct omap_opp *opp)
>>> {
>>> @@ -124,6 +154,11 @@ static inline struct omap_opp
>>> *opp_find_freq_ceil(struct omap_opp *oppl,
>>> return ERR_PTR(-EINVAL);
>>> }
>>>
>>> +static inline struct device_opp *opp_find_dev_opp(struct device *dev)
>>> +{
>>> + return ERR_PTR(-EINVAL);
>>> +}
>>> +
>>> static inline struct omap_opp *opp_add(struct omap_opp *oppl,
>>> const struct omap_opp_def *opp_def)
>>> {
>>> diff --git a/arch/arm/plat-omap/opp.c b/arch/arm/plat-omap/opp.c
>>> index 070ff5b..9bc53e8 100644
>>> --- a/arch/arm/plat-omap/opp.c
>>> +++ b/arch/arm/plat-omap/opp.c
>>> @@ -22,6 +22,7 @@
>>> #include <plat/opp_twl_tps.h>
>>> #include <plat/opp.h>
>>> #include <plat/omap_device.h>
>>> +#include <plat/voltage.h>
>>>
>>> /**
>>> * struct omap_opp - OMAP OPP description structure
>>> @@ -43,17 +44,6 @@ struct omap_opp {
>>> struct device_opp *dev_opp; /* containing device_opp struct */
>>> };
>>>
>>> -struct device_opp {
>>> - struct list_head node;
>>> -
>>> - struct omap_hwmod *oh;
>>> - struct device *dev;
>>> -
>>> - struct list_head opp_list;
>>> - u32 opp_count;
>>> - u32 enabled_opp_count;
>>> -};
>>> -
>>> static LIST_HEAD(dev_opp_list);
>>>
>>> /**
>>> @@ -330,6 +320,11 @@ struct omap_opp *opp_find_voltage(struct device *dev,
>>> unsigned long volt)
>>> return opp;
>>> }
>>>
>>> +struct device_opp *opp_find_dev_opp(struct device *dev)
>>> +{
>>> + return find_device_opp(dev);
>>> +}
>>> +
>>> /* wrapper to reuse converting opp_def to opp struct */
>>> static void omap_opp_populate(struct omap_opp *opp,
>>> const struct omap_opp_def *opp_def)
>>> @@ -385,6 +380,11 @@ int opp_add(const struct omap_opp_def *opp_def)
>>>
>>> dev_opp->oh = oh;
>>> dev_opp->dev = &oh->od->pdev.dev;
>>> + dev_opp->vdd_name = kzalloc(strlen(opp_def->vdd_name) + 1,
>>> + GFP_KERNEL);
>>> + strcpy(dev_opp->vdd_name, opp_def->vdd_name);
>>
>>Since this is user-defined data/string, should probably have a max
>>strlen and use strncpy.
I did not get this. What is the problem with the strlen here ?
Regards
Thara
>>
>>Kevin
>>
>>> + dev_opp->set_rate = opp_def->set_rate;
>>> + dev_opp->get_rate = opp_def->get_rate;
>>> INIT_LIST_HEAD(&dev_opp->opp_list);
>>>
>>> list_add(&dev_opp->node, &dev_opp_list);
>>> @@ -511,3 +511,28 @@ void opp_init_cpufreq_table(struct device *dev,
>>>
>>> *table = &freq_table[0];
>>> }
>>> +
>>> +struct device **opp_init_voltage_params(struct omap_volt_domain
>>> *volt_domain,
>>> + int *dev_count)
>>> +{
>>> + struct device_opp *dev_opp;
>>> + struct device **dev_list;
>>> + int count = 0, i = 0;
>>> +
>>> + list_for_each_entry(dev_opp, &dev_opp_list, node) {
>>> + if (!strcmp(dev_opp->vdd_name, volt_domain->name)) {
>>> + dev_opp->volt_domain = volt_domain;
>>> + count++;
>>> + }
>>> + }
>>> +
>>> + dev_list = kzalloc(sizeof(struct device *) * count, GFP_KERNEL);
>>> +
>>> + list_for_each_entry(dev_opp, &dev_opp_list, node) {
>>> + if (dev_opp->volt_domain == volt_domain)
>>> + dev_list[i++] = dev_opp->dev;
>>> + }
>>> +
>>> + *dev_count = count;
>>> + return dev_list;
>>> +}
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
