[PATCHv8 0/5] SMPS regulator driver

[PATCHv8 0/5] SMPS regulator driver

[Tero Kristo]

Hi,

Sorry I've been busy with other stuff lately, thus the update took
some time. Anyway, changes compared the previous version:

- changed driver_data format that is being passed from board to
  twl-regulator, this also required tweaking twl-core so that the
  data gets passed through correctly
- dropped one patch from the set as it was pushed for integration
  already:
  [PATCHv7 1/7] regulator: twl: fix twl4030 support for smps regulators
- some other minor tweaks

-Tero

[PATCHv8 1/5] TEMP: OMAP3: beagle rev-c4: enable OPP6

[Tero Kristo]

Beagleboard rev-c4 has a speed sorted OMAP3530 chip which can run at 720MHz.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 arch/arm/mach-omap2/board-omap3beagle.c |   29 +++++++++++++++++++++++++++++
 arch/arm/mach-omap2/opp3xxx_data.c      |    4 ++++
 2 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/board-omap3beagle.c b/arch/arm/mach-omap2/board-omap3beagle.c
index 3ae16b4..a7c3d60 100644
--- a/arch/arm/mach-omap2/board-omap3beagle.c
+++ b/arch/arm/mach-omap2/board-omap3beagle.c
@@ -489,6 +489,35 @@ static void __init beagle_opp_init(void)
 		return;
 	}
 
+	if (omap3_beagle_version == OMAP3BEAGLE_BOARD_C4) {
+		struct device *mpu_dev, *iva_dev;
+
+		mpu_dev = omap2_get_mpuss_device();
+		iva_dev = omap2_get_iva_device();
+
+		if (!mpu_dev || !iva_dev) {
+			pr_err("%s: Aiee.. no mpu/dsp devices? %p %p\n",
+				__func__, mpu_dev, iva_dev);
+			return;
+		}
+		/* Enable MPU 720MHz opp */
+		r = opp_enable(mpu_dev, 720000000);
+
+		/* Enable IVA 520MHz opp */
+		r |= opp_enable(iva_dev, 520000000);
+
+		if (r) {
+			pr_err("%s: failed to enable higher opp %d\n",
+				__func__, r);
+			/*
+			 * Cleanup - disable the higher freqs - we dont care
+			 * about the results
+			 */
+			opp_disable(mpu_dev, 720000000);
+			opp_disable(iva_dev, 520000000);
+		}
+	}
+
 	/* Custom OPP enabled for all xM versions */
 	if (cpu_is_omap3630()) {
 		struct device *mpu_dev, *iva_dev;
diff --git a/arch/arm/mach-omap2/opp3xxx_data.c b/arch/arm/mach-omap2/opp3xxx_data.c
index d95f3f9..a0f5fe1 100644
--- a/arch/arm/mach-omap2/opp3xxx_data.c
+++ b/arch/arm/mach-omap2/opp3xxx_data.c
@@ -98,6 +98,8 @@ static struct omap_opp_def __initdata omap34xx_opp_def_list[] = {
 	OPP_INITIALIZER("mpu", true, 550000000, OMAP3430_VDD_MPU_OPP4_UV),
 	/* MPU OPP5 */
 	OPP_INITIALIZER("mpu", true, 600000000, OMAP3430_VDD_MPU_OPP5_UV),
+	/* MPU OPP6 : omap3530 high speed grade only */
+	OPP_INITIALIZER("mpu", false, 720000000, OMAP3430_VDD_MPU_OPP5_UV),
 
 	/*
 	 * L3 OPP1 - 41.5 MHz is disabled because: The voltage for that OPP is
@@ -123,6 +125,8 @@ static struct omap_opp_def __initdata omap34xx_opp_def_list[] = {
 	OPP_INITIALIZER("iva", true, 400000000, OMAP3430_VDD_MPU_OPP4_UV),
 	/* DSP OPP5 */
 	OPP_INITIALIZER("iva", true, 430000000, OMAP3430_VDD_MPU_OPP5_UV),
+	/* DSP OPP6 : omap3530 high speed grade only */
+	OPP_INITIALIZER("iva", false, 520000000, OMAP3430_VDD_MPU_OPP5_UV),
 };
 
 static struct omap_opp_def __initdata omap36xx_opp_def_list[] = {
-- 
1.7.4.1

[PATCHv8 2/5] omap3: voltage: fix channel configuration

[Tero Kristo]

OMAP3 uses the default settings for VDD1 channel, otherwise the settings will
overlap with VDD2 and attempting to modify VDD1 voltage will actually change
VDD2 voltage.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 arch/arm/mach-omap2/vc3xxx_data.c |    1 +
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/vc3xxx_data.c b/arch/arm/mach-omap2/vc3xxx_data.c
index cfe348e..0136ad5 100644
--- a/arch/arm/mach-omap2/vc3xxx_data.c
+++ b/arch/arm/mach-omap2/vc3xxx_data.c
@@ -46,6 +46,7 @@ static struct omap_vc_common omap3_vc_common = {
 };
 
 struct omap_vc_channel omap3_vc_mpu = {
+	.flags = OMAP_VC_CHANNEL_DEFAULT,
 	.common = &omap3_vc_common,
 	.smps_sa_reg	 = OMAP3_PRM_VC_SMPS_SA_OFFSET,
 	.smps_volra_reg	 = OMAP3_PRM_VC_SMPS_VOL_RA_OFFSET,
-- 
1.7.4.1

[PATCHv8 3/5] omap3: add common twl configurations for vdd1 and vdd2

[Tero Kristo]

VDD1 and VDD2 are the core voltage regulators on OMAP3. VDD1 is used
to control MPU/IVA voltage, and VDD2 is used for CORE. These regulators
are needed by DVFS.

Voltage ranges for VDD1 and VDD2 are taken from twl4030/twl5030 data manual.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 arch/arm/mach-omap2/twl-common.c |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/twl-common.c b/arch/arm/mach-omap2/twl-common.c
index daa056e..7cf5347 100644
--- a/arch/arm/mach-omap2/twl-common.c
+++ b/arch/arm/mach-omap2/twl-common.c
@@ -115,6 +115,38 @@ static struct regulator_init_data omap3_vpll2_idata = {
 	.consumer_supplies		= omap3_vpll2_supplies,
 };
 
+static struct regulator_consumer_supply omap3_vdd1_supply[] = {
+	REGULATOR_SUPPLY("vcc", "mpu.0"),
+};
+
+static struct regulator_consumer_supply omap3_vdd2_supply[] = {
+	REGULATOR_SUPPLY("vcc", "l3_main.0"),
+};
+
+static struct regulator_init_data omap3_vdd1 = {
+	.constraints = {
+		.name			= "VDD1",
+		.min_uV			= 600000,
+		.max_uV			= 1450000,
+		.valid_modes_mask	= REGULATOR_MODE_NORMAL,
+		.valid_ops_mask		= REGULATOR_CHANGE_VOLTAGE,
+	},
+	.num_consumer_supplies		= ARRAY_SIZE(omap3_vdd1_supply),
+	.consumer_supplies		= omap3_vdd1_supply,
+};
+
+static struct regulator_init_data omap3_vdd2 = {
+	.constraints = {
+		.name			= "VDD2",
+		.min_uV			= 600000,
+		.max_uV			= 1450000,
+		.valid_modes_mask	= REGULATOR_MODE_NORMAL,
+		.valid_ops_mask		= REGULATOR_CHANGE_VOLTAGE,
+	},
+	.num_consumer_supplies		= ARRAY_SIZE(omap3_vdd2_supply),
+	.consumer_supplies		= omap3_vdd2_supply,
+};
+
 void __init omap3_pmic_get_config(struct twl4030_platform_data *pmic_data,
 				  u32 pdata_flags, u32 regulators_flags)
 {
@@ -122,6 +154,10 @@ void __init omap3_pmic_get_config(struct twl4030_platform_data *pmic_data,
 		pmic_data->irq_base = TWL4030_IRQ_BASE;
 	if (!pmic_data->irq_end)
 		pmic_data->irq_end = TWL4030_IRQ_END;
+	if (!pmic_data->vdd1)
+		pmic_data->vdd1 = &omap3_vdd1;
+	if (!pmic_data->vdd2)
+		pmic_data->vdd2 = &omap3_vdd2;
 
 	/* Common platform data configurations */
 	if (pdata_flags & TWL_COMMON_PDATA_USB && !pmic_data->usb)
-- 
1.7.4.1

[PATCHv8 4/5] twl4030: add support for external voltage get/set

[Tero Kristo]

This is needed for SMPS regulators, which use the OMAP voltage
processor for voltage get/set functions instead of the normal I2C
channel. For this purpose, regulator_init_data->driver_data contents
are expanded, it is now a struct which contains function pointers
for the set/get voltage operations, a data pointer for these, and
the previously used features bitmask.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Liam Girdwood <lrg@ti.com>
Cc: Samuel Ortiz <sameo@linux.intel.com>
---
 drivers/mfd/twl-core.c            |   16 ++++++++++++++-
 drivers/regulator/twl-regulator.c |   39 ++++++++++++++++++++++++++++++++----
 include/linux/i2c/twl.h           |    7 ++++++
 3 files changed, 56 insertions(+), 6 deletions(-)

diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
index 01ecfee..009f62b 100644
--- a/drivers/mfd/twl-core.c
+++ b/drivers/mfd/twl-core.c
@@ -609,6 +609,8 @@ add_regulator_linked(int num, struct regulator_init_data *pdata,
 		unsigned num_consumers, unsigned long features)
 {
 	unsigned sub_chip_id;
+	struct twl_regulator_driver_data drv_data;
+
 	/* regulator framework demands init_data ... */
 	if (!pdata)
 		return NULL;
@@ -618,7 +620,19 @@ add_regulator_linked(int num, struct regulator_init_data *pdata,
 		pdata->num_consumer_supplies = num_consumers;
 	}
 
-	pdata->driver_data = (void *)features;
+	if (pdata->driver_data) {
+		/* If we have existing drv_data, just add the flags */
+		struct twl_regulator_driver_data *tmp;
+		tmp = pdata->driver_data;
+		tmp->features |= features;
+	} else {
+		/* add new driver data struct, used only during init */
+		drv_data.features = features;
+		drv_data.set_voltage = NULL;
+		drv_data.get_voltage = NULL;
+		drv_data.data = NULL;
+		pdata->driver_data = &drv_data;
+	}
 
 	/* NOTE:  we currently ignore regulator IRQs, e.g. for short circuits */
 	sub_chip_id = twl_map[TWL_MODULE_PM_MASTER].sid;
diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
index 11cc308..29eda40 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -58,6 +58,16 @@ struct twlreg_info {
 
 	/* chip specific features */
 	unsigned long 		features;
+
+	/*
+	 * optional override functions for voltage set/get
+	 * these are currently only used for SMPS regulators
+	 */
+	int			(*get_voltage)(void *data);
+	int			(*set_voltage)(void *data, int min_uV);
+
+	/* data passed from board for external get/set voltage */
+	void			*data;
 };
 
 
@@ -522,15 +532,25 @@ twl4030smps_set_voltage(struct regulator_dev *rdev, int min_uV, int max_uV,
 	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->set_voltage) {
+		return info->set_voltage(info->data, min_uV);
+	} else {
+		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->get_voltage)
+		return info->get_voltage(info->data);
+
+	vsel = twlreg_read(info, TWL_MODULE_PM_RECEIVER,
 		VREG_VOLTAGE_SMPS_4030);
 
 	return vsel * 12500 + 600000;
@@ -1052,6 +1072,7 @@ static int __devinit twlreg_probe(struct platform_device *pdev)
 	struct regulator_init_data	*initdata;
 	struct regulation_constraints	*c;
 	struct regulator_dev		*rdev;
+	struct twl_regulator_driver_data	*drvdata;
 
 	for (i = 0, info = NULL; i < ARRAY_SIZE(twl_regs); i++) {
 		if (twl_regs[i].desc.id != pdev->id)
@@ -1066,8 +1087,16 @@ static int __devinit twlreg_probe(struct platform_device *pdev)
 	if (!initdata)
 		return -EINVAL;
 
-	/* copy the features into regulator data */
-	info->features = (unsigned long)initdata->driver_data;
+	drvdata = initdata->driver_data;
+
+	if (!drvdata)
+		return -EINVAL;
+
+	/* copy the driver data into regulator data */
+	info->features = drvdata->features;
+	info->data = drvdata->data;
+	info->set_voltage = drvdata->set_voltage;
+	info->get_voltage = drvdata->get_voltage;
 
 	/* Constrain board-specific capabilities according to what
 	 * this driver and the chip itself can actually do.
diff --git a/include/linux/i2c/twl.h b/include/linux/i2c/twl.h
index 114c0f6..7157e88 100644
--- a/include/linux/i2c/twl.h
+++ b/include/linux/i2c/twl.h
@@ -747,6 +747,13 @@ struct twl4030_platform_data {
 	struct regulator_init_data		*vio6025;
 };
 
+struct twl_regulator_driver_data {
+	int		(*set_voltage)(void *data, int min_uV);
+	int		(*get_voltage)(void *data);
+	void		*data;
+	unsigned long	features;
+};
+
 /*----------------------------------------------------------------------*/
 
 int twl4030_sih_setup(int module);
-- 
1.7.4.1

[PATCHv8 5/5] omap3: twl: add external controllers for core voltage regulators

[Tero Kristo]

VDD1 and VDD2 now use voltage processor for controlling the regulators.
This is done by passing additional voltdm data during the regulator init.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 arch/arm/mach-omap2/twl-common.c |   33 +++++++++++++++++++++++++++++++--
 1 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/twl-common.c b/arch/arm/mach-omap2/twl-common.c
index 7cf5347..d2557d1 100644
--- a/arch/arm/mach-omap2/twl-common.c
+++ b/arch/arm/mach-omap2/twl-common.c
@@ -30,6 +30,19 @@
 #include <plat/usb.h>
 
 #include "twl-common.h"
+#include "voltage.h"
+
+static int twl_set_voltage(void *data, int min_uV)
+{
+	struct voltagedomain *voltdm = (struct voltagedomain *)data;
+	return voltdm_scale(voltdm, min_uV);
+}
+
+static int twl_get_voltage(void *data)
+{
+	struct voltagedomain *voltdm = (struct voltagedomain *)data;
+	return voltdm_get_voltage(voltdm);
+}
 
 static struct i2c_board_info __initdata pmic_i2c_board_info = {
 	.addr		= 0x48,
@@ -147,6 +160,16 @@ static struct regulator_init_data omap3_vdd2 = {
 	.consumer_supplies		= omap3_vdd2_supply,
 };
 
+static struct twl_regulator_driver_data omap3_vdd1_drvdata = {
+	.get_voltage = twl_get_voltage,
+	.set_voltage = twl_set_voltage,
+};
+
+static struct twl_regulator_driver_data omap3_vdd2_drvdata = {
+	.get_voltage = twl_get_voltage,
+	.set_voltage = twl_set_voltage,
+};
+
 void __init omap3_pmic_get_config(struct twl4030_platform_data *pmic_data,
 				  u32 pdata_flags, u32 regulators_flags)
 {
@@ -154,10 +177,16 @@ void __init omap3_pmic_get_config(struct twl4030_platform_data *pmic_data,
 		pmic_data->irq_base = TWL4030_IRQ_BASE;
 	if (!pmic_data->irq_end)
 		pmic_data->irq_end = TWL4030_IRQ_END;
-	if (!pmic_data->vdd1)
+	if (!pmic_data->vdd1) {
+		omap3_vdd1.driver_data = &omap3_vdd1_drvdata;
+		omap3_vdd1_drvdata.data = voltdm_lookup("mpu_iva");
 		pmic_data->vdd1 = &omap3_vdd1;
-	if (!pmic_data->vdd2)
+	}
+	if (!pmic_data->vdd2) {
+		omap3_vdd2.driver_data = &omap3_vdd2_drvdata;
+		omap3_vdd2_drvdata.data = voltdm_lookup("core");
 		pmic_data->vdd2 = &omap3_vdd2;
+	}
 
 	/* Common platform data configurations */
 	if (pdata_flags & TWL_COMMON_PDATA_USB && !pmic_data->usb)
-- 
1.7.4.1

Re: [PATCHv8 2/5] omap3: voltage: fix channel configuration

[Kevin Hilman]

Tero Kristo <t-kristo@ti.com> writes:

> OMAP3 uses the default settings for VDD1 channel, otherwise the settings will
> overlap with VDD2 and attempting to modify VDD1 voltage will actually change
> VDD2 voltage.
>
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---
>  arch/arm/mach-omap2/vc3xxx_data.c |    1 +
>  2 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/vc3xxx_data.c b/arch/arm/mach-omap2/vc3xxx_data.c
> index cfe348e..0136ad5 100644
> --- a/arch/arm/mach-omap2/vc3xxx_data.c
> +++ b/arch/arm/mach-omap2/vc3xxx_data.c
> @@ -46,6 +46,7 @@ static struct omap_vc_common omap3_vc_common = {
>  };
>  
>  struct omap_vc_channel omap3_vc_mpu = {
> +	.flags = OMAP_VC_CHANNEL_DEFAULT,
>  	.common = &omap3_vc_common,
>  	.smps_sa_reg	 = OMAP3_PRM_VC_SMPS_SA_OFFSET,
>  	.smps_volra_reg	 = OMAP3_PRM_VC_SMPS_VOL_RA_OFFSET,

Looking more at the flow diagram you mentioned in the OMAP3 TRM, I don't
think this is right for OMAP3.

Setting the USE_DEFAULTS flags means that VDD1 will only ever be able to
use [slave address | voltage reg | command reg] zero.  While this is
quite likely in most scenarios, the HW doesn't limit this like it does
on OMAP4.

On OMAP3, it's very possible to configure VDD1 to use 
[slave address | voltage reg | command reg] one if you want (even though
I'm not sure why you would.)

In any case, my point is that setting the USE_DEFAULTS flag forces an
OMAP4 restriction onto OMAP3 which the hardware doesn't have.

Kevin

Re: [PATCHv8 2/5] omap3: voltage: fix channel configuration

[Tero Kristo]

On Fri, 2011-12-09 at 16:21 -0800, Kevin Hilman wrote:
> Tero Kristo <t-kristo@ti.com> writes:
> 
> > OMAP3 uses the default settings for VDD1 channel, otherwise the settings will
> > overlap with VDD2 and attempting to modify VDD1 voltage will actually change
> > VDD2 voltage.
> >
> > Signed-off-by: Tero Kristo <t-kristo@ti.com>
> > ---
> >  arch/arm/mach-omap2/vc3xxx_data.c |    1 +
> >  2 files changed, 5 insertions(+), 1 deletions(-)
> >
> > diff --git a/arch/arm/mach-omap2/vc3xxx_data.c b/arch/arm/mach-omap2/vc3xxx_data.c
> > index cfe348e..0136ad5 100644
> > --- a/arch/arm/mach-omap2/vc3xxx_data.c
> > +++ b/arch/arm/mach-omap2/vc3xxx_data.c
> > @@ -46,6 +46,7 @@ static struct omap_vc_common omap3_vc_common = {
> >  };
> >  
> >  struct omap_vc_channel omap3_vc_mpu = {
> > +	.flags = OMAP_VC_CHANNEL_DEFAULT,
> >  	.common = &omap3_vc_common,
> >  	.smps_sa_reg	 = OMAP3_PRM_VC_SMPS_SA_OFFSET,
> >  	.smps_volra_reg	 = OMAP3_PRM_VC_SMPS_VOL_RA_OFFSET,
> 
> Looking more at the flow diagram you mentioned in the OMAP3 TRM, I don't
> think this is right for OMAP3.
> 
> Setting the USE_DEFAULTS flags means that VDD1 will only ever be able to
> use [slave address | voltage reg | command reg] zero.  While this is
> quite likely in most scenarios, the HW doesn't limit this like it does
> on OMAP4.
> 
> On OMAP3, it's very possible to configure VDD1 to use 
> [slave address | voltage reg | command reg] one if you want (even though
> I'm not sure why you would.)
> 
> In any case, my point is that setting the USE_DEFAULTS flag forces an
> OMAP4 restriction onto OMAP3 which the hardware doesn't have.
> 

Well, the voltage control does not work at all if this is not done. I am
not sure what is the root cause for this, as I don't have currently
oscilloscope available so that I could take a look at the bus traffic.
The voltage rail behavior is incorrect unless this change is in, this is
easily seen by measuring the voltage levels and trying to change the
voltages by sw.

-Tero

Re: [PATCHv8 2/5] omap3: voltage: fix channel configuration

[Kevin Hilman]

Tero Kristo <t-kristo@ti.com> writes:

> On Fri, 2011-12-09 at 16:21 -0800, Kevin Hilman wrote:
>> Tero Kristo <t-kristo@ti.com> writes:
>> 
>> > OMAP3 uses the default settings for VDD1 channel, otherwise the settings will
>> > overlap with VDD2 and attempting to modify VDD1 voltage will actually change
>> > VDD2 voltage.
>> >
>> > Signed-off-by: Tero Kristo <t-kristo@ti.com>
>> > ---
>> >  arch/arm/mach-omap2/vc3xxx_data.c |    1 +
>> >  2 files changed, 5 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/arch/arm/mach-omap2/vc3xxx_data.c b/arch/arm/mach-omap2/vc3xxx_data.c
>> > index cfe348e..0136ad5 100644
>> > --- a/arch/arm/mach-omap2/vc3xxx_data.c
>> > +++ b/arch/arm/mach-omap2/vc3xxx_data.c
>> > @@ -46,6 +46,7 @@ static struct omap_vc_common omap3_vc_common = {
>> >  };
>> >  
>> >  struct omap_vc_channel omap3_vc_mpu = {
>> > +	.flags = OMAP_VC_CHANNEL_DEFAULT,
>> >  	.common = &omap3_vc_common,
>> >  	.smps_sa_reg	 = OMAP3_PRM_VC_SMPS_SA_OFFSET,
>> >  	.smps_volra_reg	 = OMAP3_PRM_VC_SMPS_VOL_RA_OFFSET,
>> 
>> Looking more at the flow diagram you mentioned in the OMAP3 TRM, I don't
>> think this is right for OMAP3.
>> 
>> Setting the USE_DEFAULTS flags means that VDD1 will only ever be able to
>> use [slave address | voltage reg | command reg] zero.  While this is
>> quite likely in most scenarios, the HW doesn't limit this like it does
>> on OMAP4.
>> 
>> On OMAP3, it's very possible to configure VDD1 to use 
>> [slave address | voltage reg | command reg] one if you want (even though
>> I'm not sure why you would.)
>> 
>> In any case, my point is that setting the USE_DEFAULTS flag forces an
>> OMAP4 restriction onto OMAP3 which the hardware doesn't have.
>> 
>
> Well, the voltage control does not work at all if this is not done. I am
> not sure what is the root cause for this, as I don't have currently
> oscilloscope available so that I could take a look at the bus traffic.
> The voltage rail behavior is incorrect unless this change is in, this is
> easily seen by measuring the voltage levels and trying to change the
> voltages by sw.

Yeah, I understand why it's broken now and the question is only whether
to make the same "default channel" restriction exist for OMAP3 as exists
for OMAP4.  For now, I'm OK with this restriction, so this patch is
fine.

Kevin

Re: [PATCHv8 4/5] twl4030: add support for external voltage get/set

[Samuel Ortiz]

Hi Tero,

On Fri, Dec 09, 2011 at 05:29:48PM +0200, Tero Kristo wrote:
> This is needed for SMPS regulators, which use the OMAP voltage
> processor for voltage get/set functions instead of the normal I2C
> channel. For this purpose, regulator_init_data->driver_data contents
> are expanded, it is now a struct which contains function pointers
> for the set/get voltage operations, a data pointer for these, and
> the previously used features bitmask.
Looks better than the previous versions. For the MFD part:
Acked-by: Samuel Ortiz <sameo@linux.intel.com>

Liam, I assume this is going through your tree ?

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCHv8 4/5] twl4030: add support for external voltage get/set

[Girdwood, Liam]

On 19 December 2011 08:33, Samuel Ortiz <sameo@linux.intel.com> wrote:
> Hi Tero,
>
> On Fri, Dec 09, 2011 at 05:29:48PM +0200, Tero Kristo wrote:
>> This is needed for SMPS regulators, which use the OMAP voltage
>> processor for voltage get/set functions instead of the normal I2C
>> channel. For this purpose, regulator_init_data->driver_data contents
>> are expanded, it is now a struct which contains function pointers
>> for the set/get voltage operations, a data pointer for these, and
>> the previously used features bitmask.
> Looks better than the previous versions. For the MFD part:
> Acked-by: Samuel Ortiz <sameo@linux.intel.com>
>
> Liam, I assume this is going through your tree ?
>
> Cheers,
> Samuel.
>

Mark will take it for this kernel release (if he's OK with it).

Thanks

Liam

Re: [PATCHv8 4/5] twl4030: add support for external voltage get/set

[Mark Brown]

On Mon, Dec 19, 2011 at 09:56:50AM +0000, Girdwood, Liam wrote:

> Mark will take it for this kernel release (if he's OK with it).

Someone would need to send the change to me, prefereably with a subject
line which makes it look relevant.

Re: [PATCHv8 4/5] twl4030: add support for external voltage get/set

[Kevin Hilman]

Tero Kristo <t-kristo@ti.com> writes:

> This is needed for SMPS regulators, which use the OMAP voltage
> processor for voltage get/set functions instead of the normal I2C
> channel. For this purpose, regulator_init_data->driver_data contents
> are expanded, it is now a struct which contains function pointers
> for the set/get voltage operations, a data pointer for these, and
> the previously used features bitmask.
>
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Cc: Liam Girdwood <lrg@ti.com>
> Cc: Samuel Ortiz <sameo@linux.intel.com>

Seems to me like the get/set override should be more generic (part of
regulator core) instead of TWL specific?

Otherwise, whenever someone hooks up a non-TWL regulator to an OMAP and
is using HW control (via VC/VP), they'll have to duplicate all of this
stuff in their regulator driver?

Kevin


> ---
>  drivers/mfd/twl-core.c            |   16 ++++++++++++++-
>  drivers/regulator/twl-regulator.c |   39 ++++++++++++++++++++++++++++++++----
>  include/linux/i2c/twl.h           |    7 ++++++
>  3 files changed, 56 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
> index 01ecfee..009f62b 100644
> --- a/drivers/mfd/twl-core.c
> +++ b/drivers/mfd/twl-core.c
> @@ -609,6 +609,8 @@ add_regulator_linked(int num, struct regulator_init_data *pdata,
>  		unsigned num_consumers, unsigned long features)
>  {
>  	unsigned sub_chip_id;
> +	struct twl_regulator_driver_data drv_data;
> +
>  	/* regulator framework demands init_data ... */
>  	if (!pdata)
>  		return NULL;
> @@ -618,7 +620,19 @@ add_regulator_linked(int num, struct regulator_init_data *pdata,
>  		pdata->num_consumer_supplies = num_consumers;
>  	}
>  
> -	pdata->driver_data = (void *)features;
> +	if (pdata->driver_data) {
> +		/* If we have existing drv_data, just add the flags */
> +		struct twl_regulator_driver_data *tmp;
> +		tmp = pdata->driver_data;
> +		tmp->features |= features;
> +	} else {
> +		/* add new driver data struct, used only during init */
> +		drv_data.features = features;
> +		drv_data.set_voltage = NULL;
> +		drv_data.get_voltage = NULL;
> +		drv_data.data = NULL;
> +		pdata->driver_data = &drv_data;
> +	}
>  
>  	/* NOTE:  we currently ignore regulator IRQs, e.g. for short circuits */
>  	sub_chip_id = twl_map[TWL_MODULE_PM_MASTER].sid;
> diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
> index 11cc308..29eda40 100644
> --- a/drivers/regulator/twl-regulator.c
> +++ b/drivers/regulator/twl-regulator.c
> @@ -58,6 +58,16 @@ struct twlreg_info {
>  
>  	/* chip specific features */
>  	unsigned long 		features;
> +
> +	/*
> +	 * optional override functions for voltage set/get
> +	 * these are currently only used for SMPS regulators
> +	 */
> +	int			(*get_voltage)(void *data);
> +	int			(*set_voltage)(void *data, int min_uV);
> +
> +	/* data passed from board for external get/set voltage */
> +	void			*data;
>  };
>  
>  
> @@ -522,15 +532,25 @@ twl4030smps_set_voltage(struct regulator_dev *rdev, int min_uV, int max_uV,
>  	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->set_voltage) {
> +		return info->set_voltage(info->data, min_uV);
> +	} else {
> +		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->get_voltage)
> +		return info->get_voltage(info->data);
> +
> +	vsel = twlreg_read(info, TWL_MODULE_PM_RECEIVER,
>  		VREG_VOLTAGE_SMPS_4030);
>  
>  	return vsel * 12500 + 600000;
> @@ -1052,6 +1072,7 @@ static int __devinit twlreg_probe(struct platform_device *pdev)
>  	struct regulator_init_data	*initdata;
>  	struct regulation_constraints	*c;
>  	struct regulator_dev		*rdev;
> +	struct twl_regulator_driver_data	*drvdata;
>  
>  	for (i = 0, info = NULL; i < ARRAY_SIZE(twl_regs); i++) {
>  		if (twl_regs[i].desc.id != pdev->id)
> @@ -1066,8 +1087,16 @@ static int __devinit twlreg_probe(struct platform_device *pdev)
>  	if (!initdata)
>  		return -EINVAL;
>  
> -	/* copy the features into regulator data */
> -	info->features = (unsigned long)initdata->driver_data;
> +	drvdata = initdata->driver_data;
> +
> +	if (!drvdata)
> +		return -EINVAL;
> +
> +	/* copy the driver data into regulator data */
> +	info->features = drvdata->features;
> +	info->data = drvdata->data;
> +	info->set_voltage = drvdata->set_voltage;
> +	info->get_voltage = drvdata->get_voltage;
>  
>  	/* Constrain board-specific capabilities according to what
>  	 * this driver and the chip itself can actually do.
> diff --git a/include/linux/i2c/twl.h b/include/linux/i2c/twl.h
> index 114c0f6..7157e88 100644
> --- a/include/linux/i2c/twl.h
> +++ b/include/linux/i2c/twl.h
> @@ -747,6 +747,13 @@ struct twl4030_platform_data {
>  	struct regulator_init_data		*vio6025;
>  };
>  
> +struct twl_regulator_driver_data {
> +	int		(*set_voltage)(void *data, int min_uV);
> +	int		(*get_voltage)(void *data);
> +	void		*data;
> +	unsigned long	features;
> +};
> +
>  /*----------------------------------------------------------------------*/
>  
>  int twl4030_sih_setup(int module);

Re: [PATCHv8 4/5] twl4030: add support for external voltage get/set

[Mark Brown]

On Mon, Jan 09, 2012 at 05:11:11PM -0800, Kevin Hilman wrote:

> Seems to me like the get/set override should be more generic (part of
> regulator core) instead of TWL specific?

> Otherwise, whenever someone hooks up a non-TWL regulator to an OMAP and
> is using HW control (via VC/VP), they'll have to duplicate all of this
> stuff in their regulator driver?

Frankly I'm not sure I understand how the hardware is supposed to work
here - originally the plan was to just add a new regulator driver for
the hardware control block which is I guess clean enough but it seems
like some of the other control still goes via the normal path.

Re: [PATCHv8 4/5] twl4030: add support for external voltage get/set

[Tero Kristo]

On Mon, 2012-01-09 at 17:02 -0800, Kevin Hilman wrote:
> Tero,
> 
> Mark Brown <broonie@opensource.wolfsonmicro.com> writes:
> 
> > On Mon, Dec 19, 2011 at 09:56:50AM +0000, Girdwood, Liam wrote:
> >
> >> Mark will take it for this kernel release (if he's OK with it).
> >
> > Someone would need to send the change to me, prefereably with a subject
> > line which makes it look relevant.
> 
> Are you taking care of this?

Yea, the patch was lost due to misleading subject line. I re-sent the
file and there are some pending comments for it, so new version will be
written. I've been busy with other work during the last few weeks though
so haven't been able to do it.

I will hopefully send a new version during the next couple weeks, if the
issue you just raised about the generic external controller support can
be resolved by then.

-Tero

Re: [PATCHv8 4/5] twl4030: add support for external voltage get/set

[Kevin Hilman]

Mark Brown <broonie@opensource.wolfsonmicro.com> writes:

> On Mon, Jan 09, 2012 at 05:11:11PM -0800, Kevin Hilman wrote:
>
>> Seems to me like the get/set override should be more generic (part of
>> regulator core) instead of TWL specific?
>
>> Otherwise, whenever someone hooks up a non-TWL regulator to an OMAP and
>> is using HW control (via VC/VP), they'll have to duplicate all of this
>> stuff in their regulator driver?
>
> Frankly I'm not sure I understand how the hardware is supposed to work
> here - originally the plan was to just add a new regulator driver for
> the hardware control block which is I guess clean enough but it seems
> like some of the other control still goes via the normal path.

Yes, some of the control still goes via the normal path (although I
forget which, maybe Benoit can remind us), so I think it's best to add
the HW control part to each regulator that might uses it.

Ideally this could be facilitated by adding the extentions to the
regulator core so the amount of code needed for each regulator driver
would be minimal.

Kevin

Re: [PATCHv8 4/5] twl4030: add support for external voltage get/set

[Mark Brown]

On Tue, Jan 10, 2012 at 07:19:18AM -0800, Kevin Hilman wrote:

> Yes, some of the control still goes via the normal path (although I
> forget which, maybe Benoit can remind us), so I think it's best to add
> the HW control part to each regulator that might uses it.

> Ideally this could be facilitated by adding the extentions to the
> regulator core so the amount of code needed for each regulator driver
> would be minimal.

I think the original version of the patch was something along those
lines but it was just a general facility which ignored the regulator
driver entirely which didn't feel well integrated.  The discussion
suggested that this wasn't something that'd work with other regulators
so a per-driver solution seemed OK.

Re: [PATCHv8 4/5] twl4030: add support for external voltage get/set

[Tero Kristo]

On Tue, 2012-01-10 at 19:10 +0000, Mark Brown wrote:
> On Tue, Jan 10, 2012 at 07:19:18AM -0800, Kevin Hilman wrote:
> 
> > Yes, some of the control still goes via the normal path (although I
> > forget which, maybe Benoit can remind us), so I think it's best to add
> > the HW control part to each regulator that might uses it.
> 
> > Ideally this could be facilitated by adding the extentions to the
> > regulator core so the amount of code needed for each regulator driver
> > would be minimal.
> 
> I think the original version of the patch was something along those
> lines but it was just a general facility which ignored the regulator
> driver entirely which didn't feel well integrated.  The discussion
> suggested that this wasn't something that'd work with other regulators
> so a per-driver solution seemed OK.

Coming back to this patch now as I have time to look at it, what is the
general opinion, is it acceptable to patch the regulator core to add
support for the external controller or should I just resend the latest
version with changes Mark suggested? This will probably mean that once
we add new regulator drivers (e.g. pmics) we may need to duplicate the
external controller support here.

-Tero

Re: [PATCHv8 4/5] twl4030: add support for external voltage get/set

[Kevin Hilman]

Tero Kristo <t-kristo@ti.com> writes:

> On Tue, 2012-01-10 at 19:10 +0000, Mark Brown wrote:
>> On Tue, Jan 10, 2012 at 07:19:18AM -0800, Kevin Hilman wrote:
>> 
>> > Yes, some of the control still goes via the normal path (although I
>> > forget which, maybe Benoit can remind us), so I think it's best to add
>> > the HW control part to each regulator that might uses it.
>> 
>> > Ideally this could be facilitated by adding the extentions to the
>> > regulator core so the amount of code needed for each regulator driver
>> > would be minimal.
>> 
>> I think the original version of the patch was something along those
>> lines but it was just a general facility which ignored the regulator
>> driver entirely which didn't feel well integrated.  The discussion
>> suggested that this wasn't something that'd work with other regulators
>> so a per-driver solution seemed OK.
>
> Coming back to this patch now as I have time to look at it, what is the
> general opinion, is it acceptable to patch the regulator core to add
> support for the external controller or should I just resend the latest
> version with changes Mark suggested? This will probably mean that once
> we add new regulator drivers (e.g. pmics) we may need to duplicate the
> external controller support here.

How about you keep it in the regulator driver for now, and when we need
to abstract it out, we make the case for it then.

Kevin

Re: [PATCHv8 4/5] twl4030: add support for external voltage get/set

[Tero Kristo]

On Tue, 2012-02-14 at 11:16 -0800, Kevin Hilman wrote:
> Tero Kristo <t-kristo@ti.com> writes:
> 
> > On Tue, 2012-01-10 at 19:10 +0000, Mark Brown wrote:
> >> On Tue, Jan 10, 2012 at 07:19:18AM -0800, Kevin Hilman wrote:
> >> 
> >> > Yes, some of the control still goes via the normal path (although I
> >> > forget which, maybe Benoit can remind us), so I think it's best to add
> >> > the HW control part to each regulator that might uses it.
> >> 
> >> > Ideally this could be facilitated by adding the extentions to the
> >> > regulator core so the amount of code needed for each regulator driver
> >> > would be minimal.
> >> 
> >> I think the original version of the patch was something along those
> >> lines but it was just a general facility which ignored the regulator
> >> driver entirely which didn't feel well integrated.  The discussion
> >> suggested that this wasn't something that'd work with other regulators
> >> so a per-driver solution seemed OK.
> >
> > Coming back to this patch now as I have time to look at it, what is the
> > general opinion, is it acceptable to patch the regulator core to add
> > support for the external controller or should I just resend the latest
> > version with changes Mark suggested? This will probably mean that once
> > we add new regulator drivers (e.g. pmics) we may need to duplicate the
> > external controller support here.
> 
> How about you keep it in the regulator driver for now, and when we need
> to abstract it out, we make the case for it then.

Okay, sounds good to me, once we have a second driver that actually
requires this, we have more backing for the request. Currently the
regulator core change is difficult to justify. I'll rework the set based
on last comments and send it out.

-Tero