RE: [PATCHv7 6/7] regulator: twl: add support for external controller

RE: [PATCHv7 6/7] regulator: twl: add support for external controller

[Bedia, Vaibhav]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Kristo, Tero
> Sent: Monday, November 28, 2011 8:23 PM
> To: linux-omap@vger.kernel.org
> Cc: sameo@linux.intel.com; broonie@opensource.wolfsonmicro.com;
> Girdwood, Liam; Hilman, Kevin; Cousson, Benoit; Nayak, Rajendra;
> gg@slimlogic.co.uk
> Subject: [PATCHv7 6/7] regulator: twl: add support for external
> controller
> 
> TWL regulator now has two alternative control paths: the default
> I2C path or an optional voltage processor path for OMAP chips.
> If the voltage processor path should be used, this can be
> indicated within the platform data by adding flag TWL_VP_SMPS_MODE
> to regulator_init_data->driver_data.
>

Other TI PMICs like TPS65910 also have two I2C interfaces. Since 
the datasheets refers to these as Control (CTL) and SmartReflex (SR) interfaces, 
instead of the OMAP specific TWL_VP_SMPS_MODE, how about renaming the flag to 
TWL_SR_SMPS_MODE?

AFAIK the second I2C interface in all these PMICs is meant only
for the SMPS outputs. Moreover, which of the two interfaces to use
would be known during the init phase.

Is there a scenario where someone would want to change this at runtime? 

At least for TPS65910, it is possible to use the SR-I2C interface 
just like the CTL interface. In other words, even SoCs which do not 
have VP/VC can choose to change the SMPS voltage using the SR-I2C 
interface. The only catch is that only the SMPS registers can be 
accessed from this interface.

If this is the case with TWL series also, we could have non VP/VC 
specific smps_get/set_voltage APIs for the SR-I2C interface which 
SoCs with VP/VC override at init time. Note that the driver will have
two smps_get/set_voltage implementations, one corresponding to CTL-I2C
and the other corresponding to SR-I2C. During the init phase, which set
of APIs to use is indicated via a flag like TWL_SR_SMPS_MODE.

With such a change, the voltage change during DVFS would also boil down
to normal regulator calls for changing the voltage. SoCs which do not
have the VP/VC functionality can use the default implementations 
via the SR-I2C or CTL-I2C (decided at init time) and SoCs with VP/VC 
use their specific implementations.

Regards,
Vaibhav

P.S. - Somehow this patch series didn't appear in my mailbox. Reply from Gmane
seems to have gotten lost. So I replying to a forwared version. Hope this appears ok.

RE: [PATCHv7 6/7] regulator: twl: add support for external controller

[Tero Kristo]

On Tue, 2011-11-29 at 08:10 +0100, Bedia, Vaibhav wrote:
> > -----Original Message-----
> > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> > owner@vger.kernel.org] On Behalf Of Kristo, Tero
> > Sent: Monday, November 28, 2011 8:23 PM
> > To: linux-omap@vger.kernel.org
> > Cc: sameo@linux.intel.com; broonie@opensource.wolfsonmicro.com;
> > Girdwood, Liam; Hilman, Kevin; Cousson, Benoit; Nayak, Rajendra;
> > gg@slimlogic.co.uk
> > Subject: [PATCHv7 6/7] regulator: twl: add support for external
> > controller
> > 
> > TWL regulator now has two alternative control paths: the default
> > I2C path or an optional voltage processor path for OMAP chips.
> > If the voltage processor path should be used, this can be
> > indicated within the platform data by adding flag TWL_VP_SMPS_MODE
> > to regulator_init_data->driver_data.
> >
> 
> Other TI PMICs like TPS65910 also have two I2C interfaces. Since 
> the datasheets refers to these as Control (CTL) and SmartReflex (SR) interfaces, 
> instead of the OMAP specific TWL_VP_SMPS_MODE, how about renaming the flag to 
> TWL_SR_SMPS_MODE?

I was actually thinking about naming it something like SR mode, however
I think I will drop the flag away as unnecessary once I add the function
pointers for the voltage_get / voltage_scale in to the platform data.

> 
> AFAIK the second I2C interface in all these PMICs is meant only
> for the SMPS outputs. Moreover, which of the two interfaces to use
> would be known during the init phase.

Thats my understanding also.

> 
> Is there a scenario where someone would want to change this at runtime? 

Only testing purposes come to my mind, it is easier to try out some
interesting stuff if you can change it runtime, other than that, not
really. This set is also setting the used mode during init time also,
and there is no runtime configuration possibility.

> 
> At least for TPS65910, it is possible to use the SR-I2C interface 
> just like the CTL interface. In other words, even SoCs which do not 
> have VP/VC can choose to change the SMPS voltage using the SR-I2C 
> interface. The only catch is that only the SMPS registers can be 
> accessed from this interface.

Yea, the SR-I2C should be possible to use with non VP/VC setup, if
someone really wanted to do this. If anybody would actually want to do
it is another question, as you can control everything from the normal
I2C control interface. If you don't have dedicated VP hardware, the use
for the secondary I2C channel is not that obvious.

>
> If this is the case with TWL series also, we could have non VP/VC 
> specific smps_get/set_voltage APIs for the SR-I2C interface which 
> SoCs with VP/VC override at init time. Note that the driver will have
> two smps_get/set_voltage implementations, one corresponding to CTL-I2C
> and the other corresponding to SR-I2C. During the init phase, which set
> of APIs to use is indicated via a flag like TWL_SR_SMPS_MODE.
> 
> With such a change, the voltage change during DVFS would also boil down
> to normal regulator calls for changing the voltage. SoCs which do not
> have the VP/VC functionality can use the default implementations 
> via the SR-I2C or CTL-I2C (decided at init time) and SoCs with VP/VC 
> use their specific implementations.

Yea, for next version I am planning to add voltage_get / voltage_set and
the voltagedomain pointer as platform data for the regulator, these can
be defined if the user wants to use them, otherwise twl-regulator will
use the default implementation.

-Tero

> 
> Regards,
> Vaibhav
> 
> P.S. - Somehow this patch series didn't appear in my mailbox. Reply from Gmane
> seems to have gotten lost. So I replying to a forwared version. Hope this appears ok.

RE: [PATCHv7 6/7] regulator: twl: add support for external controller

[Bedia, Vaibhav]

On Tue, Nov 29, 2011 at 16:49:49, Kristo, Tero wrote:
[...]
> > 
> > Other TI PMICs like TPS65910 also have two I2C interfaces. Since 
> > the datasheets refers to these as Control (CTL) and SmartReflex (SR) interfaces, 
> > instead of the OMAP specific TWL_VP_SMPS_MODE, how about renaming the flag to 
> > TWL_SR_SMPS_MODE?
> 
> I was actually thinking about naming it something like SR mode, however
> I think I will drop the flag away as unnecessary once I add the function
> pointers for the voltage_get / voltage_scale in to the platform data.
> 

Sounds ok.

> > 
> > AFAIK the second I2C interface in all these PMICs is meant only
> > for the SMPS outputs. Moreover, which of the two interfaces to use
> > would be known during the init phase.
> 
> Thats my understanding also.
> 
> > 
> > Is there a scenario where someone would want to change this at runtime? 
> 
> Only testing purposes come to my mind, it is easier to try out some
> interesting stuff if you can change it runtime, other than that, not
> really. This set is also setting the used mode during init time also,
> and there is no runtime configuration possibility.
> 

So there's not much value is trying to implement runtime switch between 
the interfaces in the driver.

> > 
> > At least for TPS65910, it is possible to use the SR-I2C interface 
> > just like the CTL interface. In other words, even SoCs which do not 
> > have VP/VC can choose to change the SMPS voltage using the SR-I2C 
> > interface. The only catch is that only the SMPS registers can be 
> > accessed from this interface.
> 
> Yea, the SR-I2C should be possible to use with non VP/VC setup, if
> someone really wanted to do this. If anybody would actually want to do
> it is another question, as you can control everything from the normal
> I2C control interface. If you don't have dedicated VP hardware, the use
> for the secondary I2C channel is not that obvious.
> 

Well multi-processor scenario is something that come to mind. If there's 
a different core which can also talk to the PMIC, it could use one and the
host processor can use another. There are obvious issues like race conditions
etc which have to be taken into account but it's very much possible. 

> >
> > If this is the case with TWL series also, we could have non VP/VC 
> > specific smps_get/set_voltage APIs for the SR-I2C interface which 
> > SoCs with VP/VC override at init time. Note that the driver will have
> > two smps_get/set_voltage implementations, one corresponding to CTL-I2C
> > and the other corresponding to SR-I2C. During the init phase, which set
> > of APIs to use is indicated via a flag like TWL_SR_SMPS_MODE.
> > 
> > With such a change, the voltage change during DVFS would also boil down
> > to normal regulator calls for changing the voltage. SoCs which do not
> > have the VP/VC functionality can use the default implementations 
> > via the SR-I2C or CTL-I2C (decided at init time) and SoCs with VP/VC 
> > use their specific implementations.
> 
> Yea, for next version I am planning to add voltage_get / voltage_set and
> the voltagedomain pointer as platform data for the regulator, these can
> be defined if the user wants to use them, otherwise twl-regulator will
> use the default implementation.
> 

Ok.

Regards,
Vaibhav

[PATCHv7 0/7] External controller support for TWLxxxx

[Tero Kristo]

Hi,

Changes compared to previous version:
- External controller support is now built in to twl-regulator.c as
  an option that gets configured during probe
- Selected controller is based on option passed in platform data,
  which also needs to be passed through twl-core.c (thus the change
  to drivers/mfd/twl-core.c)
- Dropped a couple of silly debug prints

-Tero

[PATCHv7 6/7] regulator: twl: add support for external controller

[Tero Kristo]

TWL regulator now has two alternative control paths: the default
I2C path or an optional voltage processor path for OMAP chips.
If the voltage processor path should be used, this can be
indicated within the platform data by adding flag TWL_VP_SMPS_MODE
to regulator_init_data->driver_data.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Liam Girdwood <lrg@ti.com>
---
 drivers/regulator/twl-regulator.c |   39 ++++++++++++++++++++++++++++++------
 include/linux/i2c/twl.h           |    1 +
 2 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
index 11cc308..b674ae4 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -18,6 +18,7 @@
 #include <linux/regulator/machine.h>
 #include <linux/i2c/twl.h>
 
+#include <plat/voltage.h>
 
 /*
  * The TWL4030/TW5030/TPS659x0/TWL6030 family chips include power management, a
@@ -37,6 +38,12 @@ struct twlreg_info {
 	/* twl resource ID, for resource control state machine */
 	u8			id;
 
+	/* voltagedomain, only used for VP controlled smps regulators */
+	union {
+		const char		*name;
+		struct voltagedomain	*ptr;
+	} voltdm;
+
 	/* voltage in mV = table[VSEL]; table_len must be a power-of-two */
 	u8			table_len;
 	const u16		*table;
@@ -520,17 +527,28 @@ twl4030smps_set_voltage(struct regulator_dev *rdev, int min_uV, int max_uV,
 			unsigned *selector)
 {
 	struct twlreg_info *info = rdev_get_drvdata(rdev);
-	int vsel = DIV_ROUND_UP(min_uV - 600000, 12500);
 
-	twlreg_write(info, TWL_MODULE_PM_RECEIVER, VREG_VOLTAGE_SMPS_4030,
-		vsel);
+	if (info->voltdm.ptr)
+		voltdm_scale(info->voltdm.ptr, min_uV);
+	else {
+		int vsel = DIV_ROUND_UP(min_uV - 600000, 12500);
+
+		twlreg_write(info, TWL_MODULE_PM_RECEIVER,
+			VREG_VOLTAGE_SMPS_4030, vsel);
+	}
+
 	return 0;
 }
 
 static int twl4030smps_get_voltage(struct regulator_dev *rdev)
 {
 	struct twlreg_info *info = rdev_get_drvdata(rdev);
-	int vsel = twlreg_read(info, TWL_MODULE_PM_RECEIVER,
+	int vsel;
+
+	if (info->voltdm.ptr)
+		return voltdm_get_voltage(info->voltdm.ptr);
+
+	vsel = twlreg_read(info, TWL_MODULE_PM_RECEIVER,
 		VREG_VOLTAGE_SMPS_4030);
 
 	return vsel * 12500 + 600000;
@@ -883,9 +901,13 @@ static struct regulator_ops twlsmps_ops = {
 		}, \
 	}
 
-#define TWL4030_ADJUSTABLE_SMPS(label, offset, num, turnon_delay, remap_conf) \
+#define TWL4030_ADJUSTABLE_SMPS(label, offset, num, voltdm_name, turnon_delay, \
+	remap_conf) \
 	{ \
 	.base = offset, \
+	.voltdm = { \
+		.name = #voltdm_name, \
+	}, \
 	.id = num, \
 	.delay = turnon_delay, \
 	.remap = remap_conf, \
@@ -989,8 +1011,8 @@ static struct twlreg_info twl_regs[] = {
 	TWL4030_ADJUSTABLE_LDO(VINTANA2, 0x43, 12, 100, 0x08),
 	TWL4030_FIXED_LDO(VINTDIG, 0x47, 1500, 13, 100, 0x08),
 	TWL4030_ADJUSTABLE_LDO(VIO, 0x4b, 14, 1000, 0x08),
-	TWL4030_ADJUSTABLE_SMPS(VDD1, 0x55, 15, 1000, 0x08),
-	TWL4030_ADJUSTABLE_SMPS(VDD2, 0x63, 16, 1000, 0x08),
+	TWL4030_ADJUSTABLE_SMPS(VDD1, 0x55, 15, mpu_iva, 1000, 0x08),
+	TWL4030_ADJUSTABLE_SMPS(VDD2, 0x63, 16, core, 1000, 0x08),
 	TWL4030_FIXED_LDO(VUSB1V5, 0x71, 1500, 17, 100, 0x08),
 	TWL4030_FIXED_LDO(VUSB1V8, 0x74, 1800, 18, 100, 0x08),
 	TWL4030_FIXED_LDO(VUSB3V1, 0x77, 3100, 19, 150, 0x08),
@@ -1091,6 +1113,9 @@ static int __devinit twlreg_probe(struct platform_device *pdev)
 		break;
 	}
 
+	if (info->voltdm.name && info->features & TWL_VP_SMPS_MODE)
+		info->voltdm.ptr = voltdm_lookup(info->voltdm.name);
+
 	switch (pdev->id) {
 	case TWL6025_REG_SMPS3:
 		if (twl_get_smps_mult() & SMPS_MULTOFFSET_SMPS3)
diff --git a/include/linux/i2c/twl.h b/include/linux/i2c/twl.h
index 114c0f6..17000e2 100644
--- a/include/linux/i2c/twl.h
+++ b/include/linux/i2c/twl.h
@@ -172,6 +172,7 @@ TWL_CLASS_IS(4030, TWL4030_CLASS_ID)
 TWL_CLASS_IS(6030, TWL6030_CLASS_ID)
 
 #define TWL6025_SUBCLASS	BIT(4)  /* TWL6025 has changed registers */
+#define TWL_VP_SMPS_MODE	BIT(5)	/* Use VP control path for SMPS regs */
 
 /*
  * Read and write single 8-bit registers
-- 
1.7.4.1

Re: [PATCHv7 6/7] regulator: twl: add support for external controller

[Mark Brown]

On Mon, Nov 28, 2011 at 04:53:24PM +0200, Tero Kristo wrote:

> +++ b/drivers/regulator/twl-regulator.c
> @@ -18,6 +18,7 @@
>  #include <linux/regulator/machine.h>
>  #include <linux/i2c/twl.h>
>  
> +#include <plat/voltage.h>

You shouldn't be including platform specific headers in generic code.

> +	/* voltagedomain, only used for VP controlled smps regulators */
> +	union {
> +		const char		*name;
> +		struct voltagedomain	*ptr;
> +	} voltdm;
> +

This looks pretty icky...  Why are you using a union of a pointer to a
struct and a name?  How do things know which to use?

> -	twlreg_write(info, TWL_MODULE_PM_RECEIVER, VREG_VOLTAGE_SMPS_4030,
> -		vsel);
> +	if (info->voltdm.ptr)
> +		voltdm_scale(info->voltdm.ptr, min_uV);
> +	else {


Use braces on both branches.

Re: [PATCHv7 6/7] regulator: twl: add support for external controller

[Tero Kristo]

On Mon, 2011-11-28 at 14:58 +0000, Mark Brown wrote:
> On Mon, Nov 28, 2011 at 04:53:24PM +0200, Tero Kristo wrote:
> 
> > +++ b/drivers/regulator/twl-regulator.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/regulator/machine.h>
> >  #include <linux/i2c/twl.h>
> >  
> > +#include <plat/voltage.h>
> 
> You shouldn't be including platform specific headers in generic code.

Hmm, should I pass the function pointers through platform data also?
Currently this does not seem to be too easy seeing we have a limited
number of parameters that can be passed from board and these are already
in use. regulator_init_data->driver_data is already used as a bitmask
passing feature flags around.

I could change driver_data to be a struct pointer and add the func
pointers + feature flags inside it. However it will end up changing more
code around.

> 
> > +	/* voltagedomain, only used for VP controlled smps regulators */
> > +	union {
> > +		const char		*name;
> > +		struct voltagedomain	*ptr;
> > +	} voltdm;
> > +
> 
> This looks pretty icky...  Why are you using a union of a pointer to a
> struct and a name?  How do things know which to use?

Name is only used during probe, after that we always use the ptr. If
name is not defined, ptr ends up as NULL and we use default mode.

I can change this and add func pointers for both get/set voltages and
also separate pointer for the voltagedomain itself, if I am going to
pass all of these via platform data.

> 
> > -	twlreg_write(info, TWL_MODULE_PM_RECEIVER, VREG_VOLTAGE_SMPS_4030,
> > -		vsel);
> > +	if (info->voltdm.ptr)
> > +		voltdm_scale(info->voltdm.ptr, min_uV);
> > +	else {
> 
> 
> Use braces on both branches.

Okay.

-Tero

Re: [PATCHv7 6/7] regulator: twl: add support for external controller

[Mark Brown]

On Mon, Nov 28, 2011 at 05:43:41PM +0200, Tero Kristo wrote:
> On Mon, 2011-11-28 at 14:58 +0000, Mark Brown wrote:

> > You shouldn't be including platform specific headers in generic code.

> Hmm, should I pass the function pointers through platform data also?
> Currently this does not seem to be too easy seeing we have a limited
> number of parameters that can be passed from board and these are already
> in use. regulator_init_data->driver_data is already used as a bitmask
> passing feature flags around.

> I could change driver_data to be a struct pointer and add the func
> pointers + feature flags inside it. However it will end up changing more
> code around.

Whatever you do you should preserve the ability of the driver to build
on non-OMAP platforms.

> > > +	/* voltagedomain, only used for VP controlled smps regulators */
> > > +	union {
> > > +		const char		*name;
> > > +		struct voltagedomain	*ptr;
> > > +	} voltdm;

> > This looks pretty icky...  Why are you using a union of a pointer to a
> > struct and a name?  How do things know which to use?

> Name is only used during probe, after that we always use the ptr. If
> name is not defined, ptr ends up as NULL and we use default mode.

That's fairly nasty, just use two variables or something.  The above is
just asking for breakage.