[PATCH v2 0/2] mfd: dt: add power off support for Tegra20/Tegra30

[PATCH v2 0/2] mfd: dt: add power off support for Tegra20/Tegra30

[Bill Huang]

This patch series add new property into regulator DT for telling whether or not
to hook pmic's power off routine to system call "pm_power_off".

Patch 1 add power off support for Tegra20 boards using TPS6586x
Patch 2 add power off support for Tegra30 boards using TPS65910

Verified on Seaboard (Tegra20) and Cardhu (Tegra30)

V2:
* Take multiple pmic instances into consideration while assigning global variables
  as per suggestion from Thierry Reding <thierry.reding@avionic-design.de>

V1:
* Based on master branch of sameo/mfd-2.6.git

Bill Huang (2):
  mfd: dt: tps6586x: Add power off control
  mfd: dt: tps65910: add power off control

 Documentation/devicetree/bindings/mfd/tps65910.txt |    4 +++
 .../devicetree/bindings/regulator/tps6586x.txt     |    6 +++++
 drivers/mfd/tps6586x.c                             |   19 +++++++++++++++++
 drivers/mfd/tps65910.c                             |   22 ++++++++++++++++++++
 include/linux/mfd/tps6586x.h                       |    1 +
 include/linux/mfd/tps65910.h                       |    3 ++
 6 files changed, 55 insertions(+), 0 deletions(-)

-- 
1.7.4.1

[PATCH v2 1/2] mfd: dt: tps6586x: Add power off control

[Bill Huang]

Add DT property "ti,system-power-controller" telling whether or not this
pmic is in charge of controlling the system power, so the power off
routine can be hooked up to system call "pm_power_off".

Based on the work by:
Dan Willemsen <dwillemsen@nvidia.com>

Signed-off-by: Bill Huang <bilhuang@nvidia.com>
Tested-by: Stephen Warren <swarren@wwwdotorg.org>
---
 .../devicetree/bindings/regulator/tps6586x.txt     |    6 ++++++
 drivers/mfd/tps6586x.c                             |   19 +++++++++++++++++++
 include/linux/mfd/tps6586x.h                       |    1 +
 3 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/regulator/tps6586x.txt b/Documentation/devicetree/bindings/regulator/tps6586x.txt
index d156e1b..03dfa4e 100644
--- a/Documentation/devicetree/bindings/regulator/tps6586x.txt
+++ b/Documentation/devicetree/bindings/regulator/tps6586x.txt
@@ -18,6 +18,10 @@ Required properties:
 - vinldo678-supply: The input supply for the LDO6, LDO7 and LDO8
 - vinldo9-supply: The input supply for the LDO9
 
+Optional properties:
+- ti,system-power-controller: Telling whether or not this pmic is controlling
+  the system power.
+
 Each regulator is defined using the standard binding for regulators.
 
 Example:
@@ -30,6 +34,8 @@ Example:
 		#gpio-cells = <2>;
 		gpio-controller;
 
+		ti,system-power-controller;
+
 		sm0-supply = <&some_reg>;
 		sm1-supply = <&some_reg>;
 		sm2-supply = <&some_reg>;
diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
index 353c348..d08f59c 100644
--- a/drivers/mfd/tps6586x.c
+++ b/drivers/mfd/tps6586x.c
@@ -29,6 +29,10 @@
 #include <linux/mfd/core.h>
 #include <linux/mfd/tps6586x.h>
 
+#define TPS6586X_SUPPLYENE	0x14
+#define EXITSLREQ_BIT		BIT(1)
+#define SLEEP_MODE_BIT		BIT(3)
+
 /* interrupt control registers */
 #define TPS6586X_INT_ACK1	0xb5
 #define TPS6586X_INT_ACK2	0xb6
@@ -409,6 +413,7 @@ static struct tps6586x_platform_data *tps6586x_parse_dt(struct i2c_client *clien
 	pdata->subdevs = devs;
 	pdata->gpio_base = -1;
 	pdata->irq_base = -1;
+	pdata->pm_off = of_property_read_bool(np, "ti,system-power-controller");
 
 	return pdata;
 }
@@ -441,6 +446,15 @@ static const struct regmap_config tps6586x_regmap_config = {
 	.cache_type = REGCACHE_RBTREE,
 };
 
+static struct device *tps6586x_dev;
+static void tps6586x_power_off(void)
+{
+	if (tps6586x_clr_bits(tps6586x_dev, TPS6586X_SUPPLYENE, EXITSLREQ_BIT))
+		return;
+
+	tps6586x_set_bits(tps6586x_dev, TPS6586X_SUPPLYENE, SLEEP_MODE_BIT);
+}
+
 static int __devinit tps6586x_i2c_probe(struct i2c_client *client,
 					const struct i2c_device_id *id)
 {
@@ -505,6 +519,11 @@ static int __devinit tps6586x_i2c_probe(struct i2c_client *client,
 		goto err_add_devs;
 	}
 
+	if (pdata->pm_off && !pm_power_off) {
+		tps6586x_dev = &client->dev;
+		pm_power_off = tps6586x_power_off;
+	}
+
 	return 0;
 
 err_add_devs:
diff --git a/include/linux/mfd/tps6586x.h b/include/linux/mfd/tps6586x.h
index f350fd0..08f109f 100644
--- a/include/linux/mfd/tps6586x.h
+++ b/include/linux/mfd/tps6586x.h
@@ -77,6 +77,7 @@ struct tps6586x_platform_data {
 
 	int gpio_base;
 	int irq_base;
+	bool pm_off;
 };
 
 /*
-- 
1.7.4.1

[PATCH v2 2/2] mfd: dt: tps65910: add power off control

[Bill Huang]

Add DT property "ti,system-power-controller" telling whether or not this
pmic is in charge of controlling the system power, so the power off
routine can be hooked up to system call "pm_power_off".

Based on the work by:
Dan Willemsen <dwillemsen@nvidia.com>

Signed-off-by: Bill Huang <bilhuang@nvidia.com>
Tested-by: Stephen Warren <swarren@wwwdotorg.org>
---
 Documentation/devicetree/bindings/mfd/tps65910.txt |    4 +++
 drivers/mfd/tps65910.c                             |   22 ++++++++++++++++++++
 include/linux/mfd/tps65910.h                       |    3 ++
 3 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/tps65910.txt b/Documentation/devicetree/bindings/mfd/tps65910.txt
index db03599..2e33048 100644
--- a/Documentation/devicetree/bindings/mfd/tps65910.txt
+++ b/Documentation/devicetree/bindings/mfd/tps65910.txt
@@ -59,6 +59,8 @@ Optional properties:
   in TPS6591X datasheet)
 - ti,en-gpio-sleep: enable sleep control for gpios
   There should be 9 entries here, one for each gpio.
+- ti,system-power-controller: Telling whether or not this pmic is controlling
+  the system power.
 
 Regulator Optional properties:
 - ti,regulator-ext-sleep-control: enable external sleep
@@ -79,6 +81,8 @@ Example:
 		#interrupt-cells = <2>;
 		interrupt-controller;
 
+		ti,system-power-controller;
+
 		ti,vmbch-threshold = 0;
 		ti,vmbch2-threshold = 0;
 		ti,en-ck32k-xtal;
diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
index 1c56379..574ae2b 100644
--- a/drivers/mfd/tps65910.c
+++ b/drivers/mfd/tps65910.c
@@ -198,6 +198,8 @@ static struct tps65910_board *tps65910_parse_dt(struct i2c_client *client,
 
 	board_info->irq = client->irq;
 	board_info->irq_base = -1;
+	board_info->pm_off = of_property_read_bool(np,
+			"ti,system-power-controller");
 
 	return board_info;
 }
@@ -210,6 +212,21 @@ struct tps65910_board *tps65910_parse_dt(struct i2c_client *client,
 }
 #endif
 
+static struct i2c_client *tps65910_i2c_client;
+static void tps65910_power_off(void)
+{
+	struct tps65910 *tps65910;
+
+	tps65910 = dev_get_drvdata(&tps65910_i2c_client->dev);
+
+	if (tps65910_reg_set_bits(tps65910, TPS65910_DEVCTRL,
+			DEVCTRL_PWR_OFF_MASK) < 0)
+		return;
+
+	tps65910_reg_clear_bits(tps65910, TPS65910_DEVCTRL,
+			DEVCTRL_DEV_ON_MASK);
+}
+
 static __devinit int tps65910_i2c_probe(struct i2c_client *i2c,
 					const struct i2c_device_id *id)
 {
@@ -267,6 +284,11 @@ static __devinit int tps65910_i2c_probe(struct i2c_client *i2c,
 	tps65910_ck32k_init(tps65910, pmic_plat_data);
 	tps65910_sleepinit(tps65910, pmic_plat_data);
 
+	if (pmic_plat_data->pm_off && !pm_power_off) {
+		tps65910_i2c_client = i2c;
+		pm_power_off = tps65910_power_off;
+	}
+
 	return ret;
 }
 
diff --git a/include/linux/mfd/tps65910.h b/include/linux/mfd/tps65910.h
index 9bf8767..ac772b3 100644
--- a/include/linux/mfd/tps65910.h
+++ b/include/linux/mfd/tps65910.h
@@ -366,6 +366,8 @@
 
 
 /*Register DEVCTRL  (0x80) register.RegisterDescription */
+#define DEVCTRL_PWR_OFF_MASK				0x80
+#define DEVCTRL_PWR_OFF_SHIFT				7
 #define DEVCTRL_RTC_PWDN_MASK				0x40
 #define DEVCTRL_RTC_PWDN_SHIFT				6
 #define DEVCTRL_CK32K_CTRL_MASK				0x20
@@ -809,6 +811,7 @@ struct tps65910_board {
 	int vmbch2_threshold;
 	bool en_ck32k_xtal;
 	bool en_dev_slp;
+	bool pm_off;
 	struct tps65910_sleep_keepon_data *slp_keepon;
 	bool en_gpio_sleep[TPS6591X_MAX_NUM_GPIO];
 	unsigned long regulator_ext_sleep_control[TPS65910_NUM_REGS];
-- 
1.7.4.1

Re: [PATCH v2 1/2] mfd: dt: tps6586x: Add power off control

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3144 bytes --]

On Sun, Aug 19, 2012 at 06:07:55PM -0700, Bill Huang wrote:
> Add DT property "ti,system-power-controller" telling whether or not this
> pmic is in charge of controlling the system power, so the power off
> routine can be hooked up to system call "pm_power_off".
> 
> Based on the work by:
> Dan Willemsen <dwillemsen@nvidia.com>
> 
> Signed-off-by: Bill Huang <bilhuang@nvidia.com>
> Tested-by: Stephen Warren <swarren@wwwdotorg.org>
> ---
>  .../devicetree/bindings/regulator/tps6586x.txt     |    6 ++++++
>  drivers/mfd/tps6586x.c                             |   19 +++++++++++++++++++
>  include/linux/mfd/tps6586x.h                       |    1 +
>  3 files changed, 26 insertions(+), 0 deletions(-)

Hi,

I've seen the following while trying this patch applied on top of next-20120817:

[   40.581151] Power down.
[   41.583160] ------------[ cut here ]------------
[   41.587784] WARNING: at /home/thierry.reding/src/kernel/linux-ipmp.git/drivers/i2c/busses/i2c-tegra.c:525 tegra_i2c_xfer+0x21c/0x29c()
[   41.599850] Modules linked in:
[   41.602927] [<c0014074>] (unwind_backtrace+0x0/0xf8) from [<c00265a8>] (warn_slowpath_common+0x4c/0x64)
[   41.612304] [<c00265a8>] (warn_slowpath_common+0x4c/0x64) from [<c00265dc>] (warn_slowpath_null+0x1c/0x24)
[   41.621947] [<c00265dc>] (warn_slowpath_null+0x1c/0x24) from [<c02a2944>] (tegra_i2c_xfer+0x21c/0x29c)
[   41.631244] [<c02a2944>] (tegra_i2c_xfer+0x21c/0x29c) from [<c029f5e8>] (__i2c_transfer+0x44/0x80)
[   41.640192] [<c029f5e8>] (__i2c_transfer+0x44/0x80) from [<c02a0698>] (i2c_transfer+0x7c/0xb8)
[   41.648796] [<c02a0698>] (i2c_transfer+0x7c/0xb8) from [<c0232a60>] (regmap_i2c_read+0x48/0x64)
[   41.657485] [<c0232a60>] (regmap_i2c_read+0x48/0x64) from [<c0230304>] (_regmap_raw_read+0x90/0x98)
[   41.666518] [<c0230304>] (_regmap_raw_read+0x90/0x98) from [<c023035c>] (_regmap_read+0x50/0xa8)
[   41.675290] [<c023035c>] (_regmap_read+0x50/0xa8) from [<c02300c4>] (_regmap_update_bits+0x24/0x64)
[   41.684322] [<c02300c4>] (_regmap_update_bits+0x24/0x64) from [<c0230b88>] (regmap_update_bits+0x3c/0x58)
[   41.693885] [<c0230b88>] (regmap_update_bits+0x3c/0x58) from [<c0237c30>] (tps6586x_power_off+0x18/0x38)
[   41.703362] [<c0237c30>] (tps6586x_power_off+0x18/0x38) from [<c000edf4>] (machine_power_off+0x1c/0x24)
[   41.712749] [<c000edf4>] (machine_power_off+0x1c/0x24) from [<c0037ca4>] (sys_reboot+0x138/0x1b0)
[   41.721612] [<c0037ca4>] (sys_reboot+0x138/0x1b0) from [<c000e000>] (ret_fast_syscall+0x0/0x30)
[   41.730293] ---[ end trace 9af366974fefa459 ]---
[   41.734906] tegra-i2c tegra-i2c.3: i2c transfer timed out
[   41.740689] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000
[   41.740689] 
[   41.749823] [<c0014074>] (unwind_backtrace+0x0/0xf8) from [<c03e7090>] (panic+0x8c/0x1d8)
[   41.757993] [<c03e7090>] (panic+0x8c/0x1d8) from [<c002b7b8>] (do_exit+0x694/0x750)
[   41.765636] [<c002b7b8>] (do_exit+0x694/0x750) from [<c002bad0>] (do_group_exit+0x3c/0xb0)
[   41.773884] [<c002bad0>] (do_group_exit+0x3c/0xb0) from [<c002bb54>] (__wake_up_parent+0x0/0x18)

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH v2 1/2] mfd: dt: tps6586x: Add power off control

[Bill Huang]

nvpublic
> On Sun, Aug 19, 2012 at 06:07:55PM -0700, Bill Huang wrote:
> > Add DT property "ti,system-power-controller" telling whether or not
> > this pmic is in charge of controlling the system power, so the power
> > off routine can be hooked up to system call "pm_power_off".
> >
> > Based on the work by:
> > Dan Willemsen <dwillemsen@nvidia.com>
> >
> > Signed-off-by: Bill Huang <bilhuang@nvidia.com>
> > Tested-by: Stephen Warren <swarren@wwwdotorg.org>
> > ---
> >  .../devicetree/bindings/regulator/tps6586x.txt     |    6 ++++++
> >  drivers/mfd/tps6586x.c                             |   19 +++++++++++++++++++
> >  include/linux/mfd/tps6586x.h                       |    1 +
> >  3 files changed, 26 insertions(+), 0 deletions(-)
> 
> Hi,
> 
> I've seen the following while trying this patch applied on top of next-20120817:
> 
> [   40.581151] Power down.
> [   41.583160] ------------[ cut here ]------------
> [   41.587784] WARNING: at /home/thierry.reding/src/kernel/linux-ipmp.git/drivers/i2c/busses/i2c-
> tegra.c:525 tegra_i2c_xfer+0x21c/0x29c()
> [   41.599850] Modules linked in:
> [   41.602927] [<c0014074>] (unwind_backtrace+0x0/0xf8) from [<c00265a8>]
> (warn_slowpath_common+0x4c/0x64)
> [   41.612304] [<c00265a8>] (warn_slowpath_common+0x4c/0x64) from [<c00265dc>]
> (warn_slowpath_null+0x1c/0x24)
> [   41.621947] [<c00265dc>] (warn_slowpath_null+0x1c/0x24) from [<c02a2944>]
> (tegra_i2c_xfer+0x21c/0x29c)
> [   41.631244] [<c02a2944>] (tegra_i2c_xfer+0x21c/0x29c) from [<c029f5e8>] (__i2c_transfer+0x44/0x80)
> [   41.640192] [<c029f5e8>] (__i2c_transfer+0x44/0x80) from [<c02a0698>] (i2c_transfer+0x7c/0xb8)
> [   41.648796] [<c02a0698>] (i2c_transfer+0x7c/0xb8) from [<c0232a60>] (regmap_i2c_read+0x48/0x64)
> [   41.657485] [<c0232a60>] (regmap_i2c_read+0x48/0x64) from [<c0230304>] (_regmap_raw_read+0x90/0x98)
> [   41.666518] [<c0230304>] (_regmap_raw_read+0x90/0x98) from [<c023035c>] (_regmap_read+0x50/0xa8)
> [   41.675290] [<c023035c>] (_regmap_read+0x50/0xa8) from [<c02300c4>] (_regmap_update_bits+0x24/0x64)
> [   41.684322] [<c02300c4>] (_regmap_update_bits+0x24/0x64) from [<c0230b88>]
> (regmap_update_bits+0x3c/0x58)
> [   41.693885] [<c0230b88>] (regmap_update_bits+0x3c/0x58) from [<c0237c30>]
> (tps6586x_power_off+0x18/0x38)
> [   41.703362] [<c0237c30>] (tps6586x_power_off+0x18/0x38) from [<c000edf4>]
> (machine_power_off+0x1c/0x24)
> [   41.712749] [<c000edf4>] (machine_power_off+0x1c/0x24) from [<c0037ca4>] (sys_reboot+0x138/0x1b0)
> [   41.721612] [<c0037ca4>] (sys_reboot+0x138/0x1b0) from [<c000e000>] (ret_fast_syscall+0x0/0x30)
> [   41.730293] ---[ end trace 9af366974fefa459 ]---
> [   41.734906] tegra-i2c tegra-i2c.3: i2c transfer timed out
> [   41.740689] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000
> [   41.740689]
> [   41.749823] [<c0014074>] (unwind_backtrace+0x0/0xf8) from [<c03e7090>] (panic+0x8c/0x1d8)
> [   41.757993] [<c03e7090>] (panic+0x8c/0x1d8) from [<c002b7b8>] (do_exit+0x694/0x750)
> [   41.765636] [<c002b7b8>] (do_exit+0x694/0x750) from [<c002bad0>] (do_group_exit+0x3c/0xb0)
> [   41.773884] [<c002bad0>] (do_group_exit+0x3c/0xb0) from [<c002bb54>] (__wake_up_parent+0x0/0x18)

Thanks Thierry, I can repro this on Tegra20 inconsistently and found, if current cpu is not cpu0 when doing "machine_shutdown" (it will call "smp_send_stop"), i2c controller will failed to do any transaction (looks like gic interrupt will be disabled), I'll debug further to find out the root cause.

By the way, Tegra30 is good since it will always be cpu0 when doing "machine_shutdown", I still don't know why it makes the difference against Tegra20 since I'm not familiar with those cpu stuffs and what make it behave differently, I'll study a bit, thanks.

> 
> Thierry
> 
> * Unknown Key
> * 0x7F3EB3A1

RE: [PATCH v2 1/2] mfd: dt: tps6586x: Add power off control

[Bill Huang]

nvpublic
> > On Sun, Aug 19, 2012 at 06:07:55PM -0700, Bill Huang wrote:
> > > Add DT property "ti,system-power-controller" telling whether or not
> > > this pmic is in charge of controlling the system power, so the power
> > > off routine can be hooked up to system call "pm_power_off".
> > >
> > > Based on the work by:
> > > Dan Willemsen <dwillemsen@nvidia.com>
> > >
> > > Signed-off-by: Bill Huang <bilhuang@nvidia.com>
> > > Tested-by: Stephen Warren <swarren@wwwdotorg.org>
> > > ---
> > >  .../devicetree/bindings/regulator/tps6586x.txt     |    6 ++++++
> > >  drivers/mfd/tps6586x.c                             |   19 +++++++++++++++++++
> > >  include/linux/mfd/tps6586x.h                       |    1 +
> > >  3 files changed, 26 insertions(+), 0 deletions(-)
> >
> > Hi,
> >
> > I've seen the following while trying this patch applied on top of next-20120817:
> >
> > [   40.581151] Power down.
> > [   41.583160] ------------[ cut here ]------------
> > [   41.587784] WARNING: at /home/thierry.reding/src/kernel/linux-ipmp.git/drivers/i2c/busses/i2c-
> > tegra.c:525 tegra_i2c_xfer+0x21c/0x29c()
> > [   41.599850] Modules linked in:
> > [   41.602927] [<c0014074>] (unwind_backtrace+0x0/0xf8) from [<c00265a8>]
> > (warn_slowpath_common+0x4c/0x64)
> > [   41.612304] [<c00265a8>] (warn_slowpath_common+0x4c/0x64) from [<c00265dc>]
> > (warn_slowpath_null+0x1c/0x24)
> > [   41.621947] [<c00265dc>] (warn_slowpath_null+0x1c/0x24) from [<c02a2944>]
> > (tegra_i2c_xfer+0x21c/0x29c)
> > [   41.631244] [<c02a2944>] (tegra_i2c_xfer+0x21c/0x29c) from [<c029f5e8>] (__i2c_transfer+0x44/0x80)
> > [   41.640192] [<c029f5e8>] (__i2c_transfer+0x44/0x80) from [<c02a0698>] (i2c_transfer+0x7c/0xb8)
> > [   41.648796] [<c02a0698>] (i2c_transfer+0x7c/0xb8) from [<c0232a60>] (regmap_i2c_read+0x48/0x64)
> > [   41.657485] [<c0232a60>] (regmap_i2c_read+0x48/0x64) from [<c0230304>]
> (_regmap_raw_read+0x90/0x98)
> > [   41.666518] [<c0230304>] (_regmap_raw_read+0x90/0x98) from [<c023035c>] (_regmap_read+0x50/0xa8)
> > [   41.675290] [<c023035c>] (_regmap_read+0x50/0xa8) from [<c02300c4>]
> (_regmap_update_bits+0x24/0x64)
> > [   41.684322] [<c02300c4>] (_regmap_update_bits+0x24/0x64) from [<c0230b88>]
> > (regmap_update_bits+0x3c/0x58)
> > [   41.693885] [<c0230b88>] (regmap_update_bits+0x3c/0x58) from [<c0237c30>]
> > (tps6586x_power_off+0x18/0x38)
> > [   41.703362] [<c0237c30>] (tps6586x_power_off+0x18/0x38) from [<c000edf4>]
> > (machine_power_off+0x1c/0x24)
> > [   41.712749] [<c000edf4>] (machine_power_off+0x1c/0x24) from [<c0037ca4>] (sys_reboot+0x138/0x1b0)
> > [   41.721612] [<c0037ca4>] (sys_reboot+0x138/0x1b0) from [<c000e000>] (ret_fast_syscall+0x0/0x30)
> > [   41.730293] ---[ end trace 9af366974fefa459 ]---
> > [   41.734906] tegra-i2c tegra-i2c.3: i2c transfer timed out
> > [   41.740689] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000000
> > [   41.740689]
> > [   41.749823] [<c0014074>] (unwind_backtrace+0x0/0xf8) from [<c03e7090>] (panic+0x8c/0x1d8)
> > [   41.757993] [<c03e7090>] (panic+0x8c/0x1d8) from [<c002b7b8>] (do_exit+0x694/0x750)
> > [   41.765636] [<c002b7b8>] (do_exit+0x694/0x750) from [<c002bad0>] (do_group_exit+0x3c/0xb0)
> > [   41.773884] [<c002bad0>] (do_group_exit+0x3c/0xb0) from [<c002bb54>] (__wake_up_parent+0x0/0x18)
> 
> Thanks Thierry, I can repro this on Tegra20 inconsistently and found, if current cpu is not cpu0 when
> doing "machine_shutdown" (it will call "smp_send_stop"), i2c controller will failed to do any
> transaction (looks like gic interrupt will be disabled), I'll debug further to find out the root cause.
> 
> By the way, Tegra30 is good since it will always be cpu0 when doing "machine_shutdown", I still don't
> know why it makes the difference against Tegra20 since I'm not familiar with those cpu stuffs and what
> make it behave differently, I'll study a bit, thanks.
> 
I've sent the shutdown issue for discussion in ARM list: Shutdown problem in SMP system happened on Tegra20.
The cause of the i2c timeout is pretty clear now and it is not directly related to this patch, so is this
patch series acceptable? Any thoughts or comment? Thanks.

> >
> > Thierry
> >
> > * Unknown Key
> > * 0x7F3EB3A1

Re: [PATCH v2 1/2] mfd: dt: tps6586x: Add power off control

[Stephen Warren]

On 08/24/2012 06:36 PM, Bill Huang wrote:
>>> On Sun, Aug 19, 2012 at 06:07:55PM -0700, Bill Huang wrote:
>>>> Add DT property "ti,system-power-controller" telling whether or not
>>>> this pmic is in charge of controlling the system power, so the power
>>>> off routine can be hooked up to system call "pm_power_off".
...
>>> I've seen the following while trying this patch applied on top of next-20120817:
>>>
>>> [   40.581151] Power down.
>>> [   41.583160] ------------[ cut here ]------------
>>> [   41.587784] WARNING: at /home/thierry.reding/src/kernel/linux-ipmp.git/drivers/i2c/busses/i2c-
>>> tegra.c:525 tegra_i2c_xfer+0x21c/0x29c()
...
>> Thanks Thierry, I can repro this on Tegra20 inconsistently and found, if current cpu is not cpu0 when
>> doing "machine_shutdown" (it will call "smp_send_stop"), i2c controller will failed to do any
>> transaction (looks like gic interrupt will be disabled), I'll debug further to find out the root cause.
>>
>> By the way, Tegra30 is good since it will always be cpu0 when doing "machine_shutdown", I still don't
>> know why it makes the difference against Tegra20 since I'm not familiar with those cpu stuffs and what
>> make it behave differently, I'll study a bit, thanks.
>
> I've sent the shutdown issue for discussion in ARM list: Shutdown problem in SMP system happened on Tegra20.
> The cause of the i2c timeout is pretty clear now and it is not directly related to this patch, so is this
> patch series acceptable? Any thoughts or comment? Thanks.

I tend to agree; power off never worked without this patch, and
sometimes does with the patch, due to nothing wrong with this patch.

Bill, please do follow up on getting the underlying Tegra issue solved
somehow though. IIRC, Joseph Lo or Prashant has a patch which enabled
the config option that Russell mentioned, so the fix may just be to wait
for that patch to get finalized, but please double-check that solves it.
Thanks!

RE: [PATCH v2 1/2] mfd: dt: tps6586x: Add power off control

[Bill Huang]

nvpublic
> On 08/24/2012 06:36 PM, Bill Huang wrote:
> >>> On Sun, Aug 19, 2012 at 06:07:55PM -0700, Bill Huang wrote:
> >>>> Add DT property "ti,system-power-controller" telling whether or not
> >>>> this pmic is in charge of controlling the system power, so the
> >>>> power off routine can be hooked up to system call "pm_power_off".
> ...
> >>> I've seen the following while trying this patch applied on top of next-20120817:
> >>>
> >>> [   40.581151] Power down.
> >>> [   41.583160] ------------[ cut here ]------------
> >>> [   41.587784] WARNING: at /home/thierry.reding/src/kernel/linux-ipmp.git/drivers/i2c/busses/i2c-
> >>> tegra.c:525 tegra_i2c_xfer+0x21c/0x29c()
> ...
> >> Thanks Thierry, I can repro this on Tegra20 inconsistently and found,
> >> if current cpu is not cpu0 when doing "machine_shutdown" (it will
> >> call "smp_send_stop"), i2c controller will failed to do any transaction (looks like gic interrupt
> will be disabled), I'll debug further to find out the root cause.
> >>
> >> By the way, Tegra30 is good since it will always be cpu0 when doing
> >> "machine_shutdown", I still don't know why it makes the difference
> >> against Tegra20 since I'm not familiar with those cpu stuffs and what make it behave differently,
> I'll study a bit, thanks.
> >
> > I've sent the shutdown issue for discussion in ARM list: Shutdown problem in SMP system happened on
> Tegra20.
> > The cause of the i2c timeout is pretty clear now and it is not
> > directly related to this patch, so is this patch series acceptable? Any thoughts or comment? Thanks.
> 
> I tend to agree; power off never worked without this patch, and sometimes does with the patch, due to
> nothing wrong with this patch.
> 
> Bill, please do follow up on getting the underlying Tegra issue solved somehow though. IIRC, Joseph Lo
> or Prashant has a patch which enabled the config option that Russell mentioned, so the fix may just be
> to wait for that patch to get finalized, but please double-check that solves it.
> Thanks!

As per the shutdown issue discussion, enabling CONFIG_PM_SLEEP_SMP is the only solution and I've confirmed that fix the issue, thanks. 

RE: [PATCH v2 1/2] mfd: dt: tps6586x: Add power off control

[Bill Huang]

> nvpublic
> > On 08/24/2012 06:36 PM, Bill Huang wrote:
> > >>> On Sun, Aug 19, 2012 at 06:07:55PM -0700, Bill Huang wrote:
> > >>>> Add DT property "ti,system-power-controller" telling whether or
> > >>>> not this pmic is in charge of controlling the system power, so
> > >>>> the power off routine can be hooked up to system call "pm_power_off".
> > ...
> > >>> I've seen the following while trying this patch applied on top of next-20120817:
> > >>>
> > >>> [   40.581151] Power down.
> > >>> [   41.583160] ------------[ cut here ]------------
> > >>> [   41.587784] WARNING: at /home/thierry.reding/src/kernel/linux-
> ipmp.git/drivers/i2c/busses/i2c-
> > >>> tegra.c:525 tegra_i2c_xfer+0x21c/0x29c()
> > ...
> > >> Thanks Thierry, I can repro this on Tegra20 inconsistently and
> > >> found, if current cpu is not cpu0 when doing "machine_shutdown" (it
> > >> will call "smp_send_stop"), i2c controller will failed to do any
> > >> transaction (looks like gic interrupt
> > will be disabled), I'll debug further to find out the root cause.
> > >>
> > >> By the way, Tegra30 is good since it will always be cpu0 when doing
> > >> "machine_shutdown", I still don't know why it makes the difference
> > >> against Tegra20 since I'm not familiar with those cpu stuffs and
> > >> what make it behave differently,
> > I'll study a bit, thanks.
> > >
> > > I've sent the shutdown issue for discussion in ARM list: Shutdown
> > > problem in SMP system happened on
> > Tegra20.
> > > The cause of the i2c timeout is pretty clear now and it is not
> > > directly related to this patch, so is this patch series acceptable? Any thoughts or comment?
> Thanks.
> >
> > I tend to agree; power off never worked without this patch, and
> > sometimes does with the patch, due to nothing wrong with this patch.
> >
> > Bill, please do follow up on getting the underlying Tegra issue solved
> > somehow though. IIRC, Joseph Lo or Prashant has a patch which enabled
> > the config option that Russell mentioned, so the fix may just be to wait for that patch to get
> finalized, but please double-check that solves it.
> > Thanks!
> 
> As per the shutdown issue discussion, enabling CONFIG_PM_SLEEP_SMP is the only solution and I've
> confirmed that fix the issue, thanks.

Hi sameo,

Is there any concern on this patch series?

Thanks,
Bill

RE: [PATCH v2 0/2] mfd: dt: add power off support for Tegra20/Tegra30

[Bill Huang]

Hi all, 

Could somebody review this?

Thanks,
Bill

> 
> This patch series add new property into regulator DT for telling whether or not to hook pmic's power off routine to system call
> "pm_power_off".
> 
> Patch 1 add power off support for Tegra20 boards using TPS6586x Patch 2 add power off support for Tegra30 boards using TPS65910
> 
> Verified on Seaboard (Tegra20) and Cardhu (Tegra30)
> 
> V2:
> * Take multiple pmic instances into consideration while assigning global variables
>   as per suggestion from Thierry Reding <thierry.reding@avionic-design.de>
> 
> V1:
> * Based on master branch of sameo/mfd-2.6.git
> 
> Bill Huang (2):
>   mfd: dt: tps6586x: Add power off control
>   mfd: dt: tps65910: add power off control
> 
>  Documentation/devicetree/bindings/mfd/tps65910.txt |    4 +++
>  .../devicetree/bindings/regulator/tps6586x.txt     |    6 +++++
>  drivers/mfd/tps6586x.c                             |   19 +++++++++++++++++
>  drivers/mfd/tps65910.c                             |   22 ++++++++++++++++++++
>  include/linux/mfd/tps6586x.h                       |    1 +
>  include/linux/mfd/tps65910.h                       |    3 ++
>  6 files changed, 55 insertions(+), 0 deletions(-)
> 
> --
> 1.7.4.1
nvpublic

Re: [PATCH v2 0/2] mfd: dt: add power off support for Tegra20/Tegra30

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 365 bytes --]

On Tue, Sep 11, 2012 at 06:25:14PM +0800, Bill Huang wrote:
> Hi all, 
> 
> Could somebody review this?

Given that I haven't been able to test yet (due to time constraints)
with PM_SLEEP_SMP enabled, I don't want to give you a Tested-by, but the
code looks okay to me, so for both patches:

Reviewed-by: Thierry Reding <thierry.reding@avionic-design.de>

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 0/2] mfd: dt: add power off support for Tegra20/Tegra30

[Stephen Warren]

On 09/11/2012 04:46 AM, Thierry Reding wrote:
> On Tue, Sep 11, 2012 at 06:25:14PM +0800, Bill Huang wrote:
>> Hi all,
>> 
>> Could somebody review this?
> 
> Given that I haven't been able to test yet (due to time
> constraints) with PM_SLEEP_SMP enabled, I don't want to give you a
> Tested-by, but the code looks okay to me, so for both patches:
> 
> Reviewed-by: Thierry Reding <thierry.reding@avionic-design.de>

I have tested this with PM_SLEEP_SMP enabled, and it solved the
problem. I think I already gave my Tested-by, but if not:

Tested-by: Stephen Warren <swarren@wwwdotorg.org>

I hope that this gets applied to the MFD tree early enough (i.e.
within the next 2-3 days or so) that I can rely on the binding be
accepted, and hence apply patches to Tegra's device tree to enable
this functionality for 3.7.

Bill, given this was posted about 3 weeks ago, perhaps repost the
series in case Samuel doesn't have it any more, and hence can't apply it.

Re: [PATCH v2 0/2] mfd: dt: add power off support for Tegra20/Tegra30

[Samuel Ortiz]

Hi Bill,

On Sun, Aug 19, 2012 at 06:07:54PM -0700, Bill Huang wrote:
> This patch series add new property into regulator DT for telling whether or not
> to hook pmic's power off routine to system call "pm_power_off".
> 
> Patch 1 add power off support for Tegra20 boards using TPS6586x
> Patch 2 add power off support for Tegra30 boards using TPS65910
> 
> Verified on Seaboard (Tegra20) and Cardhu (Tegra30)
> 
> V2:
> * Take multiple pmic instances into consideration while assigning global variables
>   as per suggestion from Thierry Reding <thierry.reding@avionic-design.de>
> 
> V1:
> * Based on master branch of sameo/mfd-2.6.git
> 
> Bill Huang (2):
>   mfd: dt: tps6586x: Add power off control
>   mfd: dt: tps65910: add power off control
> 
>  Documentation/devicetree/bindings/mfd/tps65910.txt |    4 +++
>  .../devicetree/bindings/regulator/tps6586x.txt     |    6 +++++
>  drivers/mfd/tps6586x.c                             |   19 +++++++++++++++++
>  drivers/mfd/tps65910.c                             |   22 ++++++++++++++++++++
>  include/linux/mfd/tps6586x.h                       |    1 +
>  include/linux/mfd/tps65910.h                       |    3 ++
>  6 files changed, 55 insertions(+), 0 deletions(-)
I applied those 2 patches to my for-next branch, thanks.

Cheers,
Samuel.

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

Re: [PATCH v2 0/2] mfd: dt: add power off support for Tegra20/Tegra30

[Samuel Ortiz]

Hi Stephen,

On Tue, Sep 11, 2012 at 09:15:07AM -0600, Stephen Warren wrote:
> On 09/11/2012 04:46 AM, Thierry Reding wrote:
> > On Tue, Sep 11, 2012 at 06:25:14PM +0800, Bill Huang wrote:
> >> Hi all,
> >> 
> >> Could somebody review this?
> > 
> > Given that I haven't been able to test yet (due to time
> > constraints) with PM_SLEEP_SMP enabled, I don't want to give you a
> > Tested-by, but the code looks okay to me, so for both patches:
> > 
> > Reviewed-by: Thierry Reding <thierry.reding@avionic-design.de>
> 
> I have tested this with PM_SLEEP_SMP enabled, and it solved the
> problem. I think I already gave my Tested-by, but if not:
> 
> Tested-by: Stephen Warren <swarren@wwwdotorg.org>
> 
> I hope that this gets applied to the MFD tree early enough (i.e.
> within the next 2-3 days or so) that I can rely on the binding be
> accepted, and hence apply patches to Tegra's device tree to enable
> this functionality for 3.7.
> 
> Bill, given this was posted about 3 weeks ago, perhaps repost the
> series in case Samuel doesn't have it any more, and hence can't apply it.
No need for that. Sorry for the delay, I just applied and pushed those 2
patches.

Cheers,
Samuel.

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

Re: [PATCH v2 0/2] mfd: dt: add power off support for Tegra20/Tegra30

[Stephen Warren]

On 09/11/2012 10:08 AM, Samuel Ortiz wrote:
> Hi Stephen,
> 
> On Tue, Sep 11, 2012 at 09:15:07AM -0600, Stephen Warren wrote:
>> On 09/11/2012 04:46 AM, Thierry Reding wrote:
>>> On Tue, Sep 11, 2012 at 06:25:14PM +0800, Bill Huang wrote:
>>>> Hi all,
>>>>
>>>> Could somebody review this?
>>>
>>> Given that I haven't been able to test yet (due to time
>>> constraints) with PM_SLEEP_SMP enabled, I don't want to give you a
>>> Tested-by, but the code looks okay to me, so for both patches:
>>>
>>> Reviewed-by: Thierry Reding <thierry.reding@avionic-design.de>
>>
>> I have tested this with PM_SLEEP_SMP enabled, and it solved the
>> problem. I think I already gave my Tested-by, but if not:
>>
>> Tested-by: Stephen Warren <swarren@wwwdotorg.org>
>>
>> I hope that this gets applied to the MFD tree early enough (i.e.
>> within the next 2-3 days or so) that I can rely on the binding be
>> accepted, and hence apply patches to Tegra's device tree to enable
>> this functionality for 3.7.
>>
>> Bill, given this was posted about 3 weeks ago, perhaps repost the
>> series in case Samuel doesn't have it any more, and hence can't apply it.
>
> No need for that. Sorry for the delay, I just applied and pushed those 2
> patches.

Great! Thanks.

Do you have "[PATCH V4 REPOST] mfd: add MAX8907 core driver" on your
list too?

Re: [PATCH v2 0/2] mfd: dt: add power off support for Tegra20/Tegra30

[Samuel Ortiz]

On Tue, Sep 11, 2012 at 10:26:55AM -0600, Stephen Warren wrote:
> On 09/11/2012 10:08 AM, Samuel Ortiz wrote:
> > Hi Stephen,
> > 
> > On Tue, Sep 11, 2012 at 09:15:07AM -0600, Stephen Warren wrote:
> >> On 09/11/2012 04:46 AM, Thierry Reding wrote:
> >>> On Tue, Sep 11, 2012 at 06:25:14PM +0800, Bill Huang wrote:
> >>>> Hi all,
> >>>>
> >>>> Could somebody review this?
> >>>
> >>> Given that I haven't been able to test yet (due to time
> >>> constraints) with PM_SLEEP_SMP enabled, I don't want to give you a
> >>> Tested-by, but the code looks okay to me, so for both patches:
> >>>
> >>> Reviewed-by: Thierry Reding <thierry.reding@avionic-design.de>
> >>
> >> I have tested this with PM_SLEEP_SMP enabled, and it solved the
> >> problem. I think I already gave my Tested-by, but if not:
> >>
> >> Tested-by: Stephen Warren <swarren@wwwdotorg.org>
> >>
> >> I hope that this gets applied to the MFD tree early enough (i.e.
> >> within the next 2-3 days or so) that I can rely on the binding be
> >> accepted, and hence apply patches to Tegra's device tree to enable
> >> this functionality for 3.7.
> >>
> >> Bill, given this was posted about 3 weeks ago, perhaps repost the
> >> series in case Samuel doesn't have it any more, and hence can't apply it.
> >
> > No need for that. Sorry for the delay, I just applied and pushed those 2
> > patches.
> 
> Great! Thanks.
> 
> Do you have "[PATCH V4 REPOST] mfd: add MAX8907 core driver" on your
> list too?
Yes, I'll be chewing my MFD pending patches list in the next days, and this
one is on the list.

Cheers,
Samuel.

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