[03/13] OMAP: Implement Basic DVFS

This patch introduces an API to perform DVFS for a given voltage domain.
It takes omap_vdd_dvfs_info pointer as input parameter, computes the highest
requested voltage for that vdd and scales all the devices in that vdd to the
requested frequency along with voltage scaling.

Based on original patch from Thara.

Signed-off-by: Vishwanath BS <vishwanath.bs@ti.com>
Cc: Thara Gopinath <thara@ti.com>
---
 arch/arm/mach-omap2/dvfs.c |   87 +++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 86 insertions(+), 1 deletions(-)

Vishwanath BS <vishwanath.bs@ti.com> writes:

> This patch introduces an API to perform DVFS for a given voltage domain.
> It takes omap_vdd_dvfs_info pointer as input parameter, computes the highest
> requested voltage for that vdd and scales all the devices in that vdd to the
> requested frequency along with voltage scaling.
>
> Based on original patch from Thara.
>
> Signed-off-by: Vishwanath BS <vishwanath.bs@ti.com>
> Cc: Thara Gopinath <thara@ti.com>
> ---
>  arch/arm/mach-omap2/dvfs.c |   87 +++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 86 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/dvfs.c b/arch/arm/mach-omap2/dvfs.c
> index 8832e4a..cefc2be 100755
> --- a/arch/arm/mach-omap2/dvfs.c
> +++ b/arch/arm/mach-omap2/dvfs.c
> @@ -21,7 +21,7 @@
>  #include <plat/omap_device.h>
>  
>  /**
> - * struct omap_dev_user_list - Structure maitain userlist per devide
> + * struct omap_dev_user_list - Structure maitain userlist per device

this typo should be done in the original patch, not here.

>   *
>   * @dev:       The device requesting for a particular frequency
>   * @node:      The list head entry
> @@ -413,6 +413,91 @@ static int omap_dvfs_remove_freq_request(struct omap_vdd_dvfs_info *dvfs_info,
>  }
>  
>  /**
> + * omap_dvfs_voltage_scale() : API to scale the devices associated with a
> + *						voltage domain vdd voltage.

This function scales both voltage and frequency, so the name
voltage_scale() is a bit misleading.

> + * @dvfs_info: omap_vdd_dvfs_info pointer for the required vdd
> + *
> + * This API runs through the list of devices associated with the
> + * voltage domain and scales the device rates to the one requested
> + * by the user or those corresponding to the new voltage of the
> + * voltage domain. Target voltage is the highest voltage in the vdd_user_list.
> + *
> + * Returns 0 on success
> + * else the error value.
> + */
> +static int omap_dvfs_voltage_scale(struct omap_vdd_dvfs_info *dvfs_info)
> +{
> +	unsigned long curr_volt;
> +	int is_volt_scaled = 0;

should be a bool

> +	struct omap_vdd_dev_list *temp_dev;
> +	struct plist_node *node;
> +	int ret = 0;
> +	struct voltagedomain *voltdm;
> +	unsigned long volt;
> +
> +	if (!dvfs_info || IS_ERR(dvfs_info)) {
> +		pr_warning("%s: VDD specified does not exist!\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	voltdm = dvfs_info->voltdm;
> +
> +	mutex_lock(&dvfs_info->scaling_mutex);
> +
> +	/* Find the highest voltage being requested */
> +	node = plist_last(&dvfs_info->user_list);
> +	volt = node->prio;
> +
> +	curr_volt = omap_voltage_get_nom_volt(voltdm);
> +
> +	if (curr_volt == volt) {
> +		is_volt_scaled = 1;
> +	} else if (curr_volt < volt) {
> +		ret = omap_voltage_scale_vdd(voltdm, volt);
> +		if (ret) {
> +			pr_warning("%s: Unable to scale the %s to %ld volt\n",
> +						__func__, voltdm->name, volt);
> +			mutex_unlock(&dvfs_info->scaling_mutex);
> +			return ret;
> +		}
> +		is_volt_scaled = 1;
> +	}
> +
> +	list_for_each_entry(temp_dev, &dvfs_info->dev_list, node) {
> +		struct device *dev;
> +		struct opp *opp;
> +		unsigned long freq;
> +
> +		dev = temp_dev->dev;

if you're doing this assignment here, might as well make 'dev' the
iterator instead of temp_dev.

This section would benefit with some comments.  If I understand the code
correctly, something like:

If a frequency has been requested, use the highest requested frequency.

> +		if (!plist_head_empty(&temp_dev->user_list)) {
> +			node = plist_last(&temp_dev->user_list);
> +			freq = node->prio;

otherwise check if device has OPP for this voltage

> +		} else {
> +			opp = omap_dvfs_find_voltage(dev, volt);
> +			if (IS_ERR(opp))
> +				continue;

This needs a comment to, but I'm not sure I understand what's going on
here.  What it seems like:

if this device has no OPP for this voltage, just silently move on to the
next device?   doesn't seem quite right, but not sure I fully grok the
failure modes of omap_dvfs_find_voltage()

Kevin

> +			freq = opp_get_freq(opp);
> +		}
> +
> +		if (freq == omap_device_get_rate(dev)) {
> +			dev_dbg(dev, "%s: Already at the requested"
> +				"rate %ld\n", __func__, freq);
> +			continue;
> +		}
> +
> +		ret |= omap_device_set_rate(dev, freq);
> +	}
> +
> +	if (!is_volt_scaled && !ret)
> +		omap_voltage_scale_vdd(voltdm, volt);
> +
> +	mutex_unlock(&dvfs_info->scaling_mutex);
> +
> +	return 0;
> +}
> +
> +/**
>   * omap_dvfs_init() - Initialize omap dvfs layer
>   *
>   * Initalizes omap dvfs layer. It basically allocates memory for

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@ti.com]
> Sent: Friday, February 04, 2011 6:44 AM
> To: Vishwanath BS
> Cc: linux-omap@vger.kernel.org; patches@linaro.org; Thara Gopinath
> Subject: Re: [PATCH 03/13] OMAP: Implement Basic DVFS
>
> Vishwanath BS <vishwanath.bs@ti.com> writes:
>
> > This patch introduces an API to perform DVFS for a given voltage
> domain.
> > It takes omap_vdd_dvfs_info pointer as input parameter, computes
> the highest
> > requested voltage for that vdd and scales all the devices in that vdd
to
> the
> > requested frequency along with voltage scaling.
> >
> > Based on original patch from Thara.
> >
> > Signed-off-by: Vishwanath BS <vishwanath.bs@ti.com>
> > Cc: Thara Gopinath <thara@ti.com>
> > ---
> >  arch/arm/mach-omap2/dvfs.c |   87
> +++++++++++++++++++++++++++++++++++++++++++-
> >  1 files changed, 86 insertions(+), 1 deletions(-)
> >
> > diff --git a/arch/arm/mach-omap2/dvfs.c b/arch/arm/mach-
> omap2/dvfs.c
> > index 8832e4a..cefc2be 100755
> > --- a/arch/arm/mach-omap2/dvfs.c
> > +++ b/arch/arm/mach-omap2/dvfs.c
> > @@ -21,7 +21,7 @@
> >  #include <plat/omap_device.h>
> >
> >  /**
> > - * struct omap_dev_user_list - Structure maitain userlist per devide
> > + * struct omap_dev_user_list - Structure maitain userlist per device
>
> this typo should be done in the original patch, not here.
OK
>
> >   *
> >   * @dev:       The device requesting for a particular frequency
> >   * @node:      The list head entry
> > @@ -413,6 +413,91 @@ static int
> omap_dvfs_remove_freq_request(struct omap_vdd_dvfs_info *dvfs_info,
> >  }
> >
> >  /**
> > + * omap_dvfs_voltage_scale() : API to scale the devices associated
> with a
> > + *						voltage domain vdd
voltage.
>
> This function scales both voltage and frequency, so the name
> voltage_scale() is a bit misleading.
Does omap_dvfs_do_dvfs look good?
>
> > + * @dvfs_info: omap_vdd_dvfs_info pointer for the required vdd
> > + *
> > + * This API runs through the list of devices associated with the
> > + * voltage domain and scales the device rates to the one requested
> > + * by the user or those corresponding to the new voltage of the
> > + * voltage domain. Target voltage is the highest voltage in the
> vdd_user_list.
> > + *
> > + * Returns 0 on success
> > + * else the error value.
> > + */
> > +static int omap_dvfs_voltage_scale(struct omap_vdd_dvfs_info
> *dvfs_info)
> > +{
> > +	unsigned long curr_volt;
> > +	int is_volt_scaled = 0;
>
> should be a bool
ok
>
> > +	struct omap_vdd_dev_list *temp_dev;
> > +	struct plist_node *node;
> > +	int ret = 0;
> > +	struct voltagedomain *voltdm;
> > +	unsigned long volt;
> > +
> > +	if (!dvfs_info || IS_ERR(dvfs_info)) {
> > +		pr_warning("%s: VDD specified does not exist!\n",
> __func__);
> > +		return -EINVAL;
> > +	}
> > +
> > +	voltdm = dvfs_info->voltdm;
> > +
> > +	mutex_lock(&dvfs_info->scaling_mutex);
> > +
> > +	/* Find the highest voltage being requested */
> > +	node = plist_last(&dvfs_info->user_list);
> > +	volt = node->prio;
> > +
> > +	curr_volt = omap_voltage_get_nom_volt(voltdm);
> > +
> > +	if (curr_volt == volt) {
> > +		is_volt_scaled = 1;
> > +	} else if (curr_volt < volt) {
> > +		ret = omap_voltage_scale_vdd(voltdm, volt);
> > +		if (ret) {
> > +			pr_warning("%s: Unable to scale the %s to %ld
> volt\n",
> > +						__func__, voltdm->name,
> volt);
> > +			mutex_unlock(&dvfs_info->scaling_mutex);
> > +			return ret;
> > +		}
> > +		is_volt_scaled = 1;
> > +	}
> > +
> > +	list_for_each_entry(temp_dev, &dvfs_info->dev_list, node) {
> > +		struct device *dev;
> > +		struct opp *opp;
> > +		unsigned long freq;
> > +
> > +		dev = temp_dev->dev;
>
> if you're doing this assignment here, might as well make 'dev' the
> iterator instead of temp_dev.
temp_dev holds pointer to omap_vdd_dev_list where as dev points to actual
device pointer. Hence this assignment.
>
> This section would benefit with some comments.  If I understand the
> code
> correctly, something like:
>
> If a frequency has been requested, use the highest requested frequency.
>
> > +		if (!plist_head_empty(&temp_dev->user_list)) {
> > +			node = plist_last(&temp_dev->user_list);
> > +			freq = node->prio;
>
> otherwise check if device has OPP for this voltage
>
> > +		} else {
> > +			opp = omap_dvfs_find_voltage(dev, volt);
> > +			if (IS_ERR(opp))
> > +				continue;
>
> This needs a comment to, but I'm not sure I understand what's going on
> here.  What it seems like:
>
> if this device has no OPP for this voltage, just silently move on to the
> next device?   doesn't seem quite right, but not sure I fully grok the
> failure modes of omap_dvfs_find_voltage()
Yes, your understanding is right. omap_dvfs_find_voltage will return error
if the device does not have an OPP table.
Typically devices should not register with a vdd (using
omap_dvfs_register_device) if it has no OPP table associated with it. So
ideally we should not hit this error case. But only exception so far is SR
driver. SR hwmod has vdd_name field set so as to get voltagedomain
pointers. But SR does not have any opp table. So there is no harm in
ignoring the error and moving to next device.

Vishwa
>
> Kevin
>
> > +			freq = opp_get_freq(opp);
> > +		}
> > +
> > +		if (freq == omap_device_get_rate(dev)) {
> > +			dev_dbg(dev, "%s: Already at the requested"
> > +				"rate %ld\n", __func__, freq);
> > +			continue;
> > +		}
> > +
> > +		ret |= omap_device_set_rate(dev, freq);
> > +	}
> > +
> > +	if (!is_volt_scaled && !ret)
> > +		omap_voltage_scale_vdd(voltdm, volt);
> > +
> > +	mutex_unlock(&dvfs_info->scaling_mutex);
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> >   * omap_dvfs_init() - Initialize omap dvfs layer
> >   *
> >   * Initalizes omap dvfs layer. It basically allocates memory for

Vishwanath Sripathy <vishwanath.bs@ti.com> writes:

>> This needs a comment to, but I'm not sure I understand what's going on
>> here.  What it seems like:
>>
>> if this device has no OPP for this voltage, just silently move on to the
>> next device?   doesn't seem quite right, but not sure I fully grok the
>> failure modes of omap_dvfs_find_voltage()
>
> Yes, your understanding is right. omap_dvfs_find_voltage will return error
> if the device does not have an OPP table.
> Typically devices should not register with a vdd (using
> omap_dvfs_register_device) if it has no OPP table associated with it. So
> ideally we should not hit this error case. But only exception so far is SR
> driver. SR hwmod has vdd_name field set so as to get voltagedomain
> pointers. But SR does not have any opp table. So there is no harm in
> ignoring the error and moving to next device.

And what happens when other devices add voltage domains but don't have
OPP tables?

The point is that this error handling is 1) difficult to understand upon
first (or fifth) read and 2) very fragile with other changes.

Kevin