RE: [RFC][PATCH 1/1] OMAP: voltage: add a hook for normal regulator calls

RE: [RFC][PATCH 1/1] OMAP: voltage: add a hook for normal regulator calls

[Bedia, Vaibhav]

Hi,

On Thu, Nov 17, 2011 at 21:58:23, Bedia, Vaibhav wrote:
> TI processors in TI81x and AM33x family work with PMICs like
> TPS65910/1 which are not part of the TWL series. These processors
> also do not have a voltage controller/processor module.
> 
> In order to invoke the normal regulator calls from the voltage
> layer the following changes are done to struct voltagedomain
> 	- Add a flag use_regulator for the SoC voltagedomain
> 	  code to indicate its intention of using a PMIC which
> 	  is not controlled by VC/VP
> 	- Add a regulator_init callback which the platform code
> 	  can utilise for any custom init sequence before making
> 	  use of the regulator. Platform code is also expected
> 	  to set the voltdm->scale function in the init callback
> 
> Signed-off-by: Ravikumar Kattekola <rk@ti.com>
> Signed-off-by: Vaibhav Bedia <vaibhav.bedia@ti.com>
> ---
>  arch/arm/mach-omap2/voltage.c |    5 +++++
>  arch/arm/mach-omap2/voltage.h |    4 ++++
>  2 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
> index 1f8fdf7..ca4ef9e 100644
> --- a/arch/arm/mach-omap2/voltage.c
> +++ b/arch/arm/mach-omap2/voltage.c
> @@ -280,6 +280,11 @@ int __init omap_voltage_late_init(void)
>  			voltdm->scale = omap_vp_forceupdate_scale;
>  			omap_vp_init(voltdm);
>  		}
> +
> +		if (voltdm->use_regulator) {
> +			if(voltdm->regulator_init)
> +				voltdm->regulator_init(voltdm);
> +		}
>  	}
>  
>  	return 0;
> diff --git a/arch/arm/mach-omap2/voltage.h b/arch/arm/mach-omap2/voltage.h
> index 16a1b09..21c810f 100644
> --- a/arch/arm/mach-omap2/voltage.h
> +++ b/arch/arm/mach-omap2/voltage.h
> @@ -88,6 +88,10 @@ struct voltagedomain {
>  
>  	u32 nominal_volt;
>  	struct omap_volt_data *volt_data;
> +
> +	bool use_regulator;
> +	struct regulator *regulator;
> +	int (*regulator_init) (struct voltagedomain *voltdm);
>  };
>  

Any comments on this approach? This enables us to make use of generic regulators calls
from the voltage layer. Since TI81xx and AM33xx do not have VC/VP we need something like this 
for implementing DVFS.

Regards,
Vaibhav

Re: [RFC][PATCH 1/1] OMAP: voltage: add a hook for normal regulator calls

[Kevin Hilman]

"Bedia, Vaibhav" <vaibhav.bedia@ti.com> writes:

> Hi,
>
> On Thu, Nov 17, 2011 at 21:58:23, Bedia, Vaibhav wrote:
>> TI processors in TI81x and AM33x family work with PMICs like
>> TPS65910/1 which are not part of the TWL series. These processors
>> also do not have a voltage controller/processor module.
>> 
>> In order to invoke the normal regulator calls from the voltage
>> layer the following changes are done to struct voltagedomain
>> 	- Add a flag use_regulator for the SoC voltagedomain
>> 	  code to indicate its intention of using a PMIC which
>> 	  is not controlled by VC/VP
>> 	- Add a regulator_init callback which the platform code
>> 	  can utilise for any custom init sequence before making
>> 	  use of the regulator. Platform code is also expected
>> 	  to set the voltdm->scale function in the init callback
>> 
>> Signed-off-by: Ravikumar Kattekola <rk@ti.com>
>> Signed-off-by: Vaibhav Bedia <vaibhav.bedia@ti.com>
>> ---
>>  arch/arm/mach-omap2/voltage.c |    5 +++++
>>  arch/arm/mach-omap2/voltage.h |    4 ++++
>>  2 files changed, 9 insertions(+), 0 deletions(-)
>> 
>> diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
>> index 1f8fdf7..ca4ef9e 100644
>> --- a/arch/arm/mach-omap2/voltage.c
>> +++ b/arch/arm/mach-omap2/voltage.c
>> @@ -280,6 +280,11 @@ int __init omap_voltage_late_init(void)
>>  			voltdm->scale = omap_vp_forceupdate_scale;
>>  			omap_vp_init(voltdm);
>>  		}
>> +
>> +		if (voltdm->use_regulator) {
>> +			if(voltdm->regulator_init)
>> +				voltdm->regulator_init(voltdm);
>> +		}
>>  	}
>>  
>>  	return 0;
>> diff --git a/arch/arm/mach-omap2/voltage.h b/arch/arm/mach-omap2/voltage.h
>> index 16a1b09..21c810f 100644
>> --- a/arch/arm/mach-omap2/voltage.h
>> +++ b/arch/arm/mach-omap2/voltage.h
>> @@ -88,6 +88,10 @@ struct voltagedomain {
>>  
>>  	u32 nominal_volt;
>>  	struct omap_volt_data *volt_data;
>> +
>> +	bool use_regulator;
>> +	struct regulator *regulator;
>> +	int (*regulator_init) (struct voltagedomain *voltdm);
>>  };
>>  
>
> Any comments on this approach? 

Sorry, I didn't see this patch before, and I don't see it in the
linux-omap archives either.  Not sure what happened there...

> This enables us to make use of generic regulators calls from the
> voltage layer. 

>From this patch, I don't see how the regulator API is being used from
the voltage layer, so I don't fully understand what you're trying to
achieve.

IOW, why should the voltagedomain code be calling the regulator API?

The voltage domain code was designed with the opposite goal: namely,
that a regulator driver would be calling the voltage domain layer, not
vice versa.

> Since TI81xx and AM33xx do not have VC/VP we need something like this
> for implementing DVFS.

This patch (and changelog) does not make that very clear, since it only
adds an init-time hook.

Kevin

RE: [RFC][PATCH 1/1] OMAP: voltage: add a hook for normal regulator calls

[Bedia, Vaibhav]

Hi Kevin,

On Sat, Dec 03, 2011 at 03:57:57, Hilman, Kevin wrote:
[...]
> >
> > Any comments on this approach? 
> 
> Sorry, I didn't see this patch before, and I don't see it in the
> linux-omap archives either.  Not sure what happened there...
> 
> > This enables us to make use of generic regulators calls from the
> > voltage layer. 
> 
> From this patch, I don't see how the regulator API is being used from
> the voltage layer, so I don't fully understand what you're trying to
> achieve.
> 
> IOW, why should the voltagedomain code be calling the regulator API?
> 
> The voltage domain code was designed with the opposite goal: namely,
> that a regulator driver would be calling the voltage domain layer, not
> vice versa.

Sorry, I should have posted more detail on how we are making use of the 
change proposed here and how it helps in getting cpufreq and DVFS working.

We want to use the existing OMAP implementation of cpufreq (and DVFS) on 
the devices which do not have VC/VP.

The current OMAP cpufreq code under drivers/ invokes clk_set_rate().

We had a look at the future DVFS implementation for OMAP[1] and merged in 
the relevant patches (hope this is close to what's planned).

After this change, cpufreq invokes omap_device_scale(). The DVFS code
makes use of the OPP layer and adds a request for voltage and frequency
change. However, the voltage change request expects the scaling to be done
in scaling functions defined in the voltage layer (vc->scale or vp->scale)

On devices which do not have VC/VP, instead of just hacking the current
implementation to get the voltage scaling done, we thought of adding
a separate path.

Taking the example of AM335x, the regulator used on the EVM is TPS65910 
the driver for which is already present in the mainline kernel.

So, when omap_device_scale() is invoked from cpufreq, we want the voltage
scaling to finally trickle down to a regulator_set_voltage() for the
MPU supply.

The way we have currently done this is-

===

int am33x_mpu_voltdm_scale(struct voltagedomain *voltdm,
				unsigned long target_volt)
{
	int ret = -EINVAL;

	if (!voltdm->regulator)
		return ret;

	ret = regulator_set_voltage(voltdm->regulator, target_volt,
		target_volt + TOLERANCE);

	if (ret)
		pr_debug("Voltage change failed, ret = %d\n", ret);
	else
		pr_debug("Voltage scaled to %d\n",
			regulator_get_voltage(voltdm->regulator));

	return ret;
}

struct omap_vdd_dep_info am33xx_vddmpu_dep_info[] = {
	{.name = NULL, .dep_table = NULL, .nr_dep_entries = 0},
};

static struct omap_vdd_info am33xx_vdd1_info;

int am33x_mpu_voltdm_init(struct voltagedomain *voltdm)
{
	struct regulator *mpu_regulator;
	struct device *mpu_dev;

	mpu_dev = omap_device_get_by_hwmod_name("mpu");
	if (!mpu_dev) {
		pr_warning("%s: unable to get the mpu device\n", __func__);
		return -EINVAL;
	}

	mpu_regulator = regulator_get(mpu_dev, voltdm->name);

	if (IS_ERR(mpu_regulator)) {
		pr_err("%s: Could not get regulator for %s\n",
			__func__, voltdm->name);
		return -ENODEV;
	} else {
		voltdm->regulator = mpu_regulator;
		voltdm->scale = &am33x_mpu_voltdm_scale;
	}

	return 0;
}

static struct voltagedomain am33xx_voltdm_mpu = {
	.name = "mpu",
	.scalable = true,
	.use_regulator = 1,
	.regulator_init = &am33x_mpu_voltdm_init,
	.vdd	= &am33xx_vdd1_info,
};

===

In the init hook we set the voltdm->scale to a function which
invokes regulator_set_voltage(). All the other pieces are already taken
care of by the OPP layer and the DVFS implementation. We just had to add
the OPP data and enable cpufreq for AM335x.

> 
> > Since TI81xx and AM33xx do not have VC/VP we need something like this
> > for implementing DVFS.
> 
> This patch (and changelog) does not make that very clear, since it only
> adds an init-time hook.
> 

Does this approach sound reasonable? If you want to check the implementation,
I could push the cpufreq implementation that we currently have to Arago.

Regards,
Vaibhav

[1] git://gitorious.org/~kristo/omap-pm/omap-pm-work.git:linaro_omap_dvfs

Re: [RFC][PATCH 1/1] OMAP: voltage: add a hook for normal regulator calls

[Kevin Hilman]

"Bedia, Vaibhav" <vaibhav.bedia@ti.com> writes:

> Hi Kevin,
>
> On Sat, Dec 03, 2011 at 03:57:57, Hilman, Kevin wrote:
> [...]
>> >
>> > Any comments on this approach? 
>> 
>> Sorry, I didn't see this patch before, and I don't see it in the
>> linux-omap archives either.  Not sure what happened there...
>> 
>> > This enables us to make use of generic regulators calls from the
>> > voltage layer. 
>> 
>> From this patch, I don't see how the regulator API is being used from
>> the voltage layer, so I don't fully understand what you're trying to
>> achieve.
>> 
>> IOW, why should the voltagedomain code be calling the regulator API?
>> 
>> The voltage domain code was designed with the opposite goal: namely,
>> that a regulator driver would be calling the voltage domain layer, not
>> vice versa.
>
> Sorry, I should have posted more detail on how we are making use of the 
> change proposed here and how it helps in getting cpufreq and DVFS working.
>
> We want to use the existing OMAP implementation of cpufreq (and DVFS) on 
> the devices which do not have VC/VP.
>
> The current OMAP cpufreq code under drivers/ invokes clk_set_rate().
>
> We had a look at the future DVFS implementation for OMAP[1] and merged in 
> the relevant patches (hope this is close to what's planned).

Unfortunately, that implementation is not what's planned, and in fact
was rejected some time ago.

> After this change, cpufreq invokes omap_device_scale(). The DVFS code
> makes use of the OPP layer and adds a request for voltage and frequency
> change. However, the voltage change request expects the scaling to be done
> in scaling functions defined in the voltage layer (vc->scale or vp->scale)
>
> On devices which do not have VC/VP, instead of just hacking the current
> implementation to get the voltage scaling done, we thought of adding
> a separate path.

A much better path for SoCs without VC/VP is to simply modify the
CPUfreq driver to use the regulator API to scale voltage before calling
clk_set_rate() (if scaling up) and after scaling the frequency (if
scaling down.)

In fact, that is the direction we're going for DVFS, even for VC/VP
platforms.  There will be a regulator driver which will call the
voltagedomain/VC/VP calls to scale the voltage when needed, so from a
DVFS perspective, you use the clock API to change frequency, and the
regulator API to change voltages.

This should be a much simpler approach for you, and much easier to
understand.

Kevin

RE: [RFC][PATCH 1/1] OMAP: voltage: add a hook for normal regulator calls

[Bedia, Vaibhav]

On Tue, Dec 06, 2011 at 00:37:38, Hilman, Kevin wrote:
[...]
> >
> > We want to use the existing OMAP implementation of cpufreq (and DVFS) on 
> > the devices which do not have VC/VP.
> >
> > The current OMAP cpufreq code under drivers/ invokes clk_set_rate().
> >
> > We had a look at the future DVFS implementation for OMAP[1] and merged in 
> > the relevant patches (hope this is close to what's planned).
> 
> Unfortunately, that implementation is not what's planned, and in fact
> was rejected some time ago.

That's really unfortunate :(

> 
> > After this change, cpufreq invokes omap_device_scale(). The DVFS code
> > makes use of the OPP layer and adds a request for voltage and frequency
> > change. However, the voltage change request expects the scaling to be done
> > in scaling functions defined in the voltage layer (vc->scale or vp->scale)
> >
> > On devices which do not have VC/VP, instead of just hacking the current
> > implementation to get the voltage scaling done, we thought of adding
> > a separate path.
> 
> A much better path for SoCs without VC/VP is to simply modify the
> CPUfreq driver to use the regulator API to scale voltage before calling
> clk_set_rate() (if scaling up) and after scaling the frequency (if
> scaling down.)

I assume this needs to be done using the OPP library?

> 
> In fact, that is the direction we're going for DVFS, even for VC/VP
> platforms.  There will be a regulator driver which will call the
> voltagedomain/VC/VP calls to scale the voltage when needed, so from a
> DVFS perspective, you use the clock API to change frequency, and the
> regulator API to change voltages.

Wouldn't this end up adding OMAP specific code into the various regulator
drivers? If TPS65910 gets used on a variant that has VC/VP, won't this
require making changes in the regulator drivers to being with?

> 
> This should be a much simpler approach for you, and much easier to
> understand.

Yes, making calls to the regulator and clock frameworks is much simpler
and desirable.

Regards,
Vaibhav 

Re: [RFC][PATCH 1/1] OMAP: voltage: add a hook for normal regulator calls

[Kevin Hilman]

"Bedia, Vaibhav" <vaibhav.bedia@ti.com> writes:

> On Tue, Dec 06, 2011 at 00:37:38, Hilman, Kevin wrote:
> [...]
>> >
>> > We want to use the existing OMAP implementation of cpufreq (and DVFS) on 
>> > the devices which do not have VC/VP.
>> >
>> > The current OMAP cpufreq code under drivers/ invokes clk_set_rate().
>> >
>> > We had a look at the future DVFS implementation for OMAP[1] and merged in 
>> > the relevant patches (hope this is close to what's planned).
>> 
>> Unfortunately, that implementation is not what's planned, and in fact
>> was rejected some time ago.
>
> That's really unfortunate :(
>
>> 
>> > After this change, cpufreq invokes omap_device_scale(). The DVFS code
>> > makes use of the OPP layer and adds a request for voltage and frequency
>> > change. However, the voltage change request expects the scaling to be done
>> > in scaling functions defined in the voltage layer (vc->scale or vp->scale)
>> >
>> > On devices which do not have VC/VP, instead of just hacking the current
>> > implementation to get the voltage scaling done, we thought of adding
>> > a separate path.
>> 
>> A much better path for SoCs without VC/VP is to simply modify the
>> CPUfreq driver to use the regulator API to scale voltage before calling
>> clk_set_rate() (if scaling up) and after scaling the frequency (if
>> scaling down.)
>
> I assume this needs to be done using the OPP library?

Yes, the mainline cpufreq driver (queued and merging for v3.3, see my
for_3.3/omap-cpufreq branch) already uses the OPP library to determine
frequencies.  From there, it's a small step to get the voltage
associated with the OPP and use the regulator framework to scale the
voltage.

>> 
>> In fact, that is the direction we're going for DVFS, even for VC/VP
>> platforms.  There will be a regulator driver which will call the
>> voltagedomain/VC/VP calls to scale the voltage when needed, so from a
>> DVFS perspective, you use the clock API to change frequency, and the
>> regulator API to change voltages.
>
> Wouldn't this end up adding OMAP specific code into the various regulator
> drivers? If TPS65910 gets used on a variant that has VC/VP, won't this
> require making changes in the regulator drivers to being with?

Yes.  Tero is working on this now for the TWL PMICs, and the additional
code is rather small.

>> 
>> This should be a much simpler approach for you, and much easier to
>> understand.
>
> Yes, making calls to the regulator and clock frameworks is much simpler
> and desirable.

Great, I'm glad you agree.

Kevin