2012-08-20 01:08:20

by Bill Huang

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

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 <[email protected]>

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


2012-08-20 01:08:35

by Bill Huang

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

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 <[email protected]>

Signed-off-by: Bill Huang <[email protected]>
Tested-by: Stephen Warren <[email protected]>
---
.../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

2012-08-20 01:08:33

by Bill Huang

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

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 <[email protected]>

Signed-off-by: Bill Huang <[email protected]>
Tested-by: Stephen Warren <[email protected]>
---
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

2012-08-20 11:34:47

by Thierry Reding

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

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 <[email protected]>
>
> Signed-off-by: Bill Huang <[email protected]>
> Tested-by: Stephen Warren <[email protected]>
> ---
> .../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


Attachments:
(No filename) (3.02 kB)
(No filename) (836.00 B)
Download all attachments

2012-08-22 12:07:29

by Bill Huang

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

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 <[email protected]>
> >
> > Signed-off-by: Bill Huang <[email protected]>
> > Tested-by: Stephen Warren <[email protected]>
> > ---
> > .../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

2012-08-25 00:36:37

by Bill Huang

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

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 <[email protected]>
> > >
> > > Signed-off-by: Bill Huang <[email protected]>
> > > Tested-by: Stephen Warren <[email protected]>
> > > ---
> > > .../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

2012-08-25 03:34:32

by Stephen Warren

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

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!

2012-08-27 05:41:05

by Bill Huang

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

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.