[PATCH] mfd: twl4030-power: Fix poweroff with PM configuration enabled

[PATCH] mfd: twl4030-power: Fix poweroff with PM configuration enabled

[Tony Lindgren]

Commit e7cd1d1eb16f ("mfd: twl4030-power: Add generic reset
configuration") enabled configuring the PM features for twl4030.

This caused poweroff command to fail on devices that have the
BCI charger on twl4030 wired, or have power wired for VBUS.
Instead of powering off, the device reboots. This is because
voltage is detected on charger or VBUS with the default bits
enabled for the power transition registers.

To fix the issue, let's just clear VBUS and CHG bits as we want
poweroff command to keep the system powered off.

Fixes: e7cd1d1eb16f ("mfd: twl4030-power: Add generic reset configuration")
Cc: stable@vger.kernel.org # v3.16+
Reported-by: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Tony Lindgren <tony@atomide.com>

--- a/drivers/mfd/twl4030-power.c
+++ b/drivers/mfd/twl4030-power.c
@@ -44,6 +44,15 @@ static u8 twl4030_start_script_address = 0x2b;
 #define PWR_DEVSLP		BIT(1)
 #define PWR_DEVOFF		BIT(0)
 
+/* Register bits for CFG_P1_TRANSITION (also for P2 and P3) */
+#define STARTON_SWBUG		BIT(7)	/* Start on watchdog */
+#define STARTON_VBUS		BIT(5)	/* Start on VBUS */
+#define STARTON_VBAT		BIT(4)	/* Start on battery insert */
+#define STARTON_RTC		BIT(3)	/* Start on RTC */
+#define STARTON_USB		BIT(2)	/* Start on USB host */
+#define STARTON_CHG		BIT(1)	/* Start on charger */
+#define STARTON_PWON		BIT(0)	/* Start on PWRON button */
+
 #define SEQ_OFFSYNC		(1 << 0)
 
 #define PHY_TO_OFF_PM_MASTER(p)		(p - 0x36)
@@ -606,6 +615,44 @@ twl4030_power_configure_resources(const struct twl4030_power_data *pdata)
 	return 0;
 }
 
+static int twl4030_starton_mask_and_set(u8 bitmask, u8 bitvalues)
+{
+	u8 regs[3] = { TWL4030_PM_MASTER_CFG_P1_TRANSITION,
+		       TWL4030_PM_MASTER_CFG_P2_TRANSITION,
+		       TWL4030_PM_MASTER_CFG_P3_TRANSITION, };
+	u8 val;
+	int i, err;
+
+	err = twl_i2c_write_u8(TWL_MODULE_PM_MASTER, TWL4030_PM_MASTER_KEY_CFG1,
+			       TWL4030_PM_MASTER_PROTECT_KEY);
+	if (err)
+		goto relock;
+	err = twl_i2c_write_u8(TWL_MODULE_PM_MASTER,
+			       TWL4030_PM_MASTER_KEY_CFG2,
+			       TWL4030_PM_MASTER_PROTECT_KEY);
+	if (err)
+		goto relock;
+
+	for (i = 0; i < sizeof(regs); i++) {
+		err = twl_i2c_read_u8(TWL_MODULE_PM_MASTER,
+				      &val, regs[i]);
+		if (err)
+			break;
+		val = (~bitmask & val) | (bitmask & bitvalues);
+		err = twl_i2c_write_u8(TWL_MODULE_PM_MASTER,
+				       val, regs[i]);
+		if (err)
+			break;
+	}
+
+	if (err)
+		pr_err("TWL4030 Register access failed: %i\n", err);
+
+relock:
+	return twl_i2c_write_u8(TWL_MODULE_PM_MASTER, 0,
+				TWL4030_PM_MASTER_PROTECT_KEY);
+}
+
 /*
  * In master mode, start the power off sequence.
  * After a successful execution, TWL shuts down the power to the SoC
@@ -615,6 +662,11 @@ void twl4030_power_off(void)
 {
 	int err;
 
+	/* Disable start on charger or VBUS as it can break poweroff */
+	err = twl4030_starton_mask_and_set(STARTON_VBUS | STARTON_CHG, 0);
+	if (err)
+		pr_err("TWL4030 Unable to configure start-up\n");
+
 	err = twl_i2c_write_u8(TWL_MODULE_PM_MASTER, PWR_DEVOFF,
 			       TWL4030_PM_MASTER_P1_SW_EVENTS);
 	if (err)

Re: [PATCH] mfd: twl4030-power: Fix poweroff with PM configuration enabled

[Lee Jones]

On Sun, 02 Nov 2014, Tony Lindgren wrote:

> Commit e7cd1d1eb16f ("mfd: twl4030-power: Add generic reset
> configuration") enabled configuring the PM features for twl4030.
> 
> This caused poweroff command to fail on devices that have the
> BCI charger on twl4030 wired, or have power wired for VBUS.
> Instead of powering off, the device reboots. This is because
> voltage is detected on charger or VBUS with the default bits
> enabled for the power transition registers.
> 
> To fix the issue, let's just clear VBUS and CHG bits as we want
> poweroff command to keep the system powered off.
> 
> Fixes: e7cd1d1eb16f ("mfd: twl4030-power: Add generic reset configuration")
> Cc: stable@vger.kernel.org # v3.16+
> Reported-by: Russell King <rmk+kernel@arm.linux.org.uk>

It would be good to have Russell to review this to ensure he is happy
with the implementation.

> Signed-off-by: Tony Lindgren <tony@atomide.com>

[...]

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] mfd: twl4030-power: Fix poweroff with PM configuration enabled

[Igor Grinberg]

Hi Tony,

On 11/02/14 20:07, Tony Lindgren wrote:
> Commit e7cd1d1eb16f ("mfd: twl4030-power: Add generic reset
> configuration") enabled configuring the PM features for twl4030.
> 
> This caused poweroff command to fail on devices that have the
> BCI charger on twl4030 wired, or have power wired for VBUS.
> Instead of powering off, the device reboots. This is because
> voltage is detected on charger or VBUS with the default bits
> enabled for the power transition registers.
> 
> To fix the issue, let's just clear VBUS and CHG bits as we want
> poweroff command to keep the system powered off.

What about devices that really need to start once VBUS or CHG is connected?
It seems to me that forcing these bits on power off can break that kind of
devices and these settings should really be board specific.
What do you think?

> 
> Fixes: e7cd1d1eb16f ("mfd: twl4030-power: Add generic reset configuration")
> Cc: stable@vger.kernel.org # v3.16+
> Reported-by: Russell King <rmk+kernel@arm.linux.org.uk>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> 
> --- a/drivers/mfd/twl4030-power.c
> +++ b/drivers/mfd/twl4030-power.c
> @@ -44,6 +44,15 @@ static u8 twl4030_start_script_address = 0x2b;
>  #define PWR_DEVSLP		BIT(1)
>  #define PWR_DEVOFF		BIT(0)
>  
> +/* Register bits for CFG_P1_TRANSITION (also for P2 and P3) */
> +#define STARTON_SWBUG		BIT(7)	/* Start on watchdog */
> +#define STARTON_VBUS		BIT(5)	/* Start on VBUS */
> +#define STARTON_VBAT		BIT(4)	/* Start on battery insert */
> +#define STARTON_RTC		BIT(3)	/* Start on RTC */
> +#define STARTON_USB		BIT(2)	/* Start on USB host */
> +#define STARTON_CHG		BIT(1)	/* Start on charger */
> +#define STARTON_PWON		BIT(0)	/* Start on PWRON button */
> +
>  #define SEQ_OFFSYNC		(1 << 0)
>  
>  #define PHY_TO_OFF_PM_MASTER(p)		(p - 0x36)
> @@ -606,6 +615,44 @@ twl4030_power_configure_resources(const struct twl4030_power_data *pdata)
>  	return 0;
>  }
>  
> +static int twl4030_starton_mask_and_set(u8 bitmask, u8 bitvalues)
> +{
> +	u8 regs[3] = { TWL4030_PM_MASTER_CFG_P1_TRANSITION,
> +		       TWL4030_PM_MASTER_CFG_P2_TRANSITION,
> +		       TWL4030_PM_MASTER_CFG_P3_TRANSITION, };
> +	u8 val;
> +	int i, err;
> +
> +	err = twl_i2c_write_u8(TWL_MODULE_PM_MASTER, TWL4030_PM_MASTER_KEY_CFG1,
> +			       TWL4030_PM_MASTER_PROTECT_KEY);
> +	if (err)
> +		goto relock;
> +	err = twl_i2c_write_u8(TWL_MODULE_PM_MASTER,
> +			       TWL4030_PM_MASTER_KEY_CFG2,
> +			       TWL4030_PM_MASTER_PROTECT_KEY);
> +	if (err)
> +		goto relock;
> +
> +	for (i = 0; i < sizeof(regs); i++) {
> +		err = twl_i2c_read_u8(TWL_MODULE_PM_MASTER,
> +				      &val, regs[i]);
> +		if (err)
> +			break;
> +		val = (~bitmask & val) | (bitmask & bitvalues);
> +		err = twl_i2c_write_u8(TWL_MODULE_PM_MASTER,
> +				       val, regs[i]);
> +		if (err)
> +			break;
> +	}
> +
> +	if (err)
> +		pr_err("TWL4030 Register access failed: %i\n", err);
> +
> +relock:
> +	return twl_i2c_write_u8(TWL_MODULE_PM_MASTER, 0,
> +				TWL4030_PM_MASTER_PROTECT_KEY);
> +}
> +
>  /*
>   * In master mode, start the power off sequence.
>   * After a successful execution, TWL shuts down the power to the SoC
> @@ -615,6 +662,11 @@ void twl4030_power_off(void)
>  {
>  	int err;
>  
> +	/* Disable start on charger or VBUS as it can break poweroff */
> +	err = twl4030_starton_mask_and_set(STARTON_VBUS | STARTON_CHG, 0);
> +	if (err)
> +		pr_err("TWL4030 Unable to configure start-up\n");
> +
>  	err = twl_i2c_write_u8(TWL_MODULE_PM_MASTER, PWR_DEVOFF,
>  			       TWL4030_PM_MASTER_P1_SW_EVENTS);
>  	if (err)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Regards,
Igor.

Re: [PATCH] mfd: twl4030-power: Fix poweroff with PM configuration enabled

[Tony Lindgren]

* Igor Grinberg <grinberg@compulab.co.il> [141104 05:22]:
> Hi Tony,
> 
> On 11/02/14 20:07, Tony Lindgren wrote:
> > Commit e7cd1d1eb16f ("mfd: twl4030-power: Add generic reset
> > configuration") enabled configuring the PM features for twl4030.
> > 
> > This caused poweroff command to fail on devices that have the
> > BCI charger on twl4030 wired, or have power wired for VBUS.
> > Instead of powering off, the device reboots. This is because
> > voltage is detected on charger or VBUS with the default bits
> > enabled for the power transition registers.
> > 
> > To fix the issue, let's just clear VBUS and CHG bits as we want
> > poweroff command to keep the system powered off.
> 
> What about devices that really need to start once VBUS or CHG is connected?

More handling can be added for some cases. With this patch the
poweron bits will clear to defaults if power is completely removed.
So start-up with VBUS and CHG works in that case.

However, if you have a battery connected, and you poweroff, with
this patch the device won't power up with VBUS or CHG connected.

Note that most battery operated devices are not using the charger
on twl4030 because it has issues charging a completely empty
battery AFAIK. So most battery powered devices have been using an
external USB charger chip that's not affected by this patch.

We could consider exporting a function for the charger driver to
configure the poweron mask. And we could also consider passing a
mask in ti,use_poweroff = 0xff.

> It seems to me that forcing these bits on power off can break that kind of
> devices and these settings should really be board specific.
> What do you think?

There's a patch series for "[RFC,01/16] kernel: Add support for
poweroff handler call chain" that should help with that. For sure
the poweroff handling needs to be board specific as some systems
may need to use a GPIO to shut off a regulator powering something
before powering off the SoC.

Regards,

Tony

Re: [PATCH] mfd: twl4030-power: Fix poweroff with PM configuration enabled

[Igor Grinberg]

On 11/04/14 17:42, Tony Lindgren wrote:
> * Igor Grinberg <grinberg@compulab.co.il> [141104 05:22]:
>> Hi Tony,
>>
>> On 11/02/14 20:07, Tony Lindgren wrote:
>>> Commit e7cd1d1eb16f ("mfd: twl4030-power: Add generic reset
>>> configuration") enabled configuring the PM features for twl4030.
>>>
>>> This caused poweroff command to fail on devices that have the
>>> BCI charger on twl4030 wired, or have power wired for VBUS.
>>> Instead of powering off, the device reboots. This is because
>>> voltage is detected on charger or VBUS with the default bits
>>> enabled for the power transition registers.
>>>
>>> To fix the issue, let's just clear VBUS and CHG bits as we want
>>> poweroff command to keep the system powered off.
>>
>> What about devices that really need to start once VBUS or CHG is connected?
> 
> More handling can be added for some cases. With this patch the
> poweron bits will clear to defaults if power is completely removed.
> So start-up with VBUS and CHG works in that case.
> 
> However, if you have a battery connected, and you poweroff, with
> this patch the device won't power up with VBUS or CHG connected.
> 
> Note that most battery operated devices are not using the charger
> on twl4030 because it has issues charging a completely empty
> battery AFAIK. So most battery powered devices have been using an
> external USB charger chip that's not affected by this patch.
> 
> We could consider exporting a function for the charger driver to
> configure the poweron mask. And we could also consider passing a
> mask in ti,use_poweroff = 0xff.

Ok. That sounds better to me.
Yet, if you say there are no such devices in practice,
IMHO, we can merge this.

> 
>> It seems to me that forcing these bits on power off can break that kind of
>> devices and these settings should really be board specific.
>> What do you think?
> 
> There's a patch series for "[RFC,01/16] kernel: Add support for
> poweroff handler call chain" that should help with that. For sure
> the poweroff handling needs to be board specific as some systems
> may need to use a GPIO to shut off a regulator powering something
> before powering off the SoC.

Yes, I've seen this series.
I'm not sure though that I understand how is this supposed
to be used with DT...
Through the regulator bindings?
Which will tell it to hook up on the call chain?


-- 
Regards,
Igor.

Re: [PATCH] mfd: twl4030-power: Fix poweroff with PM configuration enabled

[Tony Lindgren]

* Igor Grinberg <grinberg@compulab.co.il> [141104 09:54]:
> On 11/04/14 17:42, Tony Lindgren wrote:
> > * Igor Grinberg <grinberg@compulab.co.il> [141104 05:22]:
> >> Hi Tony,
> >>
> >> On 11/02/14 20:07, Tony Lindgren wrote:
> >>> Commit e7cd1d1eb16f ("mfd: twl4030-power: Add generic reset
> >>> configuration") enabled configuring the PM features for twl4030.
> >>>
> >>> This caused poweroff command to fail on devices that have the
> >>> BCI charger on twl4030 wired, or have power wired for VBUS.
> >>> Instead of powering off, the device reboots. This is because
> >>> voltage is detected on charger or VBUS with the default bits
> >>> enabled for the power transition registers.
> >>>
> >>> To fix the issue, let's just clear VBUS and CHG bits as we want
> >>> poweroff command to keep the system powered off.
> >>
> >> What about devices that really need to start once VBUS or CHG is connected?
> > 
> > More handling can be added for some cases. With this patch the
> > poweron bits will clear to defaults if power is completely removed.
> > So start-up with VBUS and CHG works in that case.
> > 
> > However, if you have a battery connected, and you poweroff, with
> > this patch the device won't power up with VBUS or CHG connected.
> > 
> > Note that most battery operated devices are not using the charger
> > on twl4030 because it has issues charging a completely empty
> > battery AFAIK. So most battery powered devices have been using an
> > external USB charger chip that's not affected by this patch.
> > 
> > We could consider exporting a function for the charger driver to
> > configure the poweron mask. And we could also consider passing a
> > mask in ti,use_poweroff = 0xff.
> 
> Ok. That sounds better to me.
> Yet, if you say there are no such devices in practice,
> IMHO, we can merge this.
> 
> >> It seems to me that forcing these bits on power off can break that kind of
> >> devices and these settings should really be board specific.
> >> What do you think?
> > 
> > There's a patch series for "[RFC,01/16] kernel: Add support for
> > poweroff handler call chain" that should help with that. For sure
> > the poweroff handling needs to be board specific as some systems
> > may need to use a GPIO to shut off a regulator powering something
> > before powering off the SoC.
> 
> Yes, I've seen this series.
> I'm not sure though that I understand how is this supposed
> to be used with DT...
> Through the regulator bindings?
> Which will tell it to hook up on the call chain?

Yes that should be doable with regulator bindings for board specifc
configuration. For the SoC specific functions those can be registered
by the platform code.

Regards,

Tony

Re: [PATCH] mfd: twl4030-power: Fix poweroff with PM configuration enabled

[Russell King - ARM Linux]

On Sun, Nov 02, 2014 at 10:07:56AM -0800, Tony Lindgren wrote:
> Commit e7cd1d1eb16f ("mfd: twl4030-power: Add generic reset
> configuration") enabled configuring the PM features for twl4030.
> 
> This caused poweroff command to fail on devices that have the
> BCI charger on twl4030 wired, or have power wired for VBUS.
> Instead of powering off, the device reboots. This is because
> voltage is detected on charger or VBUS with the default bits
> enabled for the power transition registers.
> 
> To fix the issue, let's just clear VBUS and CHG bits as we want
> poweroff command to keep the system powered off.
> 
> Fixes: e7cd1d1eb16f ("mfd: twl4030-power: Add generic reset configuration")
> Cc: stable@vger.kernel.org # v3.16+
> Reported-by: Russell King <rmk+kernel@arm.linux.org.uk>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

This fixes the problem for me, thanks.  This needs to go into -rc as
it's a regression between booting non-DT and DT, and stands in the way
of me switching from non-DT booting on OMAP3430.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] mfd: twl4030-power: Fix poweroff with PM configuration enabled

[Lee Jones]

On Sun, 02 Nov 2014, Tony Lindgren wrote:

> Commit e7cd1d1eb16f ("mfd: twl4030-power: Add generic reset
> configuration") enabled configuring the PM features for twl4030.
> 
> This caused poweroff command to fail on devices that have the
> BCI charger on twl4030 wired, or have power wired for VBUS.
> Instead of powering off, the device reboots. This is because
> voltage is detected on charger or VBUS with the default bits
> enabled for the power transition registers.
> 
> To fix the issue, let's just clear VBUS and CHG bits as we want
> poweroff command to keep the system powered off.
> 
> Fixes: e7cd1d1eb16f ("mfd: twl4030-power: Add generic reset configuration")
> Cc: stable@vger.kernel.org # v3.16+
> Reported-by: Russell King <rmk+kernel@arm.linux.org.uk>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Applied to -fixes.

Not sure whether that was an Ack from Russell or not?

> --- a/drivers/mfd/twl4030-power.c
> +++ b/drivers/mfd/twl4030-power.c
> @@ -44,6 +44,15 @@ static u8 twl4030_start_script_address = 0x2b;
>  #define PWR_DEVSLP		BIT(1)
>  #define PWR_DEVOFF		BIT(0)
>  
> +/* Register bits for CFG_P1_TRANSITION (also for P2 and P3) */
> +#define STARTON_SWBUG		BIT(7)	/* Start on watchdog */
> +#define STARTON_VBUS		BIT(5)	/* Start on VBUS */
> +#define STARTON_VBAT		BIT(4)	/* Start on battery insert */
> +#define STARTON_RTC		BIT(3)	/* Start on RTC */
> +#define STARTON_USB		BIT(2)	/* Start on USB host */
> +#define STARTON_CHG		BIT(1)	/* Start on charger */
> +#define STARTON_PWON		BIT(0)	/* Start on PWRON button */
> +
>  #define SEQ_OFFSYNC		(1 << 0)
>  
>  #define PHY_TO_OFF_PM_MASTER(p)		(p - 0x36)
> @@ -606,6 +615,44 @@ twl4030_power_configure_resources(const struct twl4030_power_data *pdata)
>  	return 0;
>  }
>  
> +static int twl4030_starton_mask_and_set(u8 bitmask, u8 bitvalues)
> +{
> +	u8 regs[3] = { TWL4030_PM_MASTER_CFG_P1_TRANSITION,
> +		       TWL4030_PM_MASTER_CFG_P2_TRANSITION,
> +		       TWL4030_PM_MASTER_CFG_P3_TRANSITION, };
> +	u8 val;
> +	int i, err;
> +
> +	err = twl_i2c_write_u8(TWL_MODULE_PM_MASTER, TWL4030_PM_MASTER_KEY_CFG1,
> +			       TWL4030_PM_MASTER_PROTECT_KEY);
> +	if (err)
> +		goto relock;
> +	err = twl_i2c_write_u8(TWL_MODULE_PM_MASTER,
> +			       TWL4030_PM_MASTER_KEY_CFG2,
> +			       TWL4030_PM_MASTER_PROTECT_KEY);
> +	if (err)
> +		goto relock;
> +
> +	for (i = 0; i < sizeof(regs); i++) {
> +		err = twl_i2c_read_u8(TWL_MODULE_PM_MASTER,
> +				      &val, regs[i]);
> +		if (err)
> +			break;
> +		val = (~bitmask & val) | (bitmask & bitvalues);
> +		err = twl_i2c_write_u8(TWL_MODULE_PM_MASTER,
> +				       val, regs[i]);
> +		if (err)
> +			break;
> +	}
> +
> +	if (err)
> +		pr_err("TWL4030 Register access failed: %i\n", err);
> +
> +relock:
> +	return twl_i2c_write_u8(TWL_MODULE_PM_MASTER, 0,
> +				TWL4030_PM_MASTER_PROTECT_KEY);
> +}
> +
>  /*
>   * In master mode, start the power off sequence.
>   * After a successful execution, TWL shuts down the power to the SoC
> @@ -615,6 +662,11 @@ void twl4030_power_off(void)
>  {
>  	int err;
>  
> +	/* Disable start on charger or VBUS as it can break poweroff */
> +	err = twl4030_starton_mask_and_set(STARTON_VBUS | STARTON_CHG, 0);
> +	if (err)
> +		pr_err("TWL4030 Unable to configure start-up\n");
> +
>  	err = twl_i2c_write_u8(TWL_MODULE_PM_MASTER, PWR_DEVOFF,
>  			       TWL4030_PM_MASTER_P1_SW_EVENTS);
>  	if (err)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] mfd: twl4030-power: Fix poweroff with PM configuration enabled

[Russell King - ARM Linux]

On Mon, Nov 10, 2014 at 12:40:19PM +0000, Lee Jones wrote:
> On Sun, 02 Nov 2014, Tony Lindgren wrote:
> 
> > Commit e7cd1d1eb16f ("mfd: twl4030-power: Add generic reset
> > configuration") enabled configuring the PM features for twl4030.
> > 
> > This caused poweroff command to fail on devices that have the
> > BCI charger on twl4030 wired, or have power wired for VBUS.
> > Instead of powering off, the device reboots. This is because
> > voltage is detected on charger or VBUS with the default bits
> > enabled for the power transition registers.
> > 
> > To fix the issue, let's just clear VBUS and CHG bits as we want
> > poweroff command to keep the system powered off.
> > 
> > Fixes: e7cd1d1eb16f ("mfd: twl4030-power: Add generic reset configuration")
> > Cc: stable@vger.kernel.org # v3.16+
> > Reported-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> 
> Applied to -fixes.
> 
> Not sure whether that was an Ack from Russell or not?

A Tested-by would've been more appropriate than an ack.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] mfd: twl4030-power: Fix poweroff with PM configuration enabled

[Lee Jones]

On Mon, 10 Nov 2014, Russell King - ARM Linux wrote:

> On Mon, Nov 10, 2014 at 12:40:19PM +0000, Lee Jones wrote:
> > On Sun, 02 Nov 2014, Tony Lindgren wrote:
> > 
> > > Commit e7cd1d1eb16f ("mfd: twl4030-power: Add generic reset
> > > configuration") enabled configuring the PM features for twl4030.
> > > 
> > > This caused poweroff command to fail on devices that have the
> > > BCI charger on twl4030 wired, or have power wired for VBUS.
> > > Instead of powering off, the device reboots. This is because
> > > voltage is detected on charger or VBUS with the default bits
> > > enabled for the power transition registers.
> > > 
> > > To fix the issue, let's just clear VBUS and CHG bits as we want
> > > poweroff command to keep the system powered off.
> > > 
> > > Fixes: e7cd1d1eb16f ("mfd: twl4030-power: Add generic reset configuration")
> > > Cc: stable@vger.kernel.org # v3.16+
> > > Reported-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > 
> > Applied to -fixes.
> > 
> > Not sure whether that was an Ack from Russell or not?
> 
> A Tested-by would've been more appropriate than an ack.

Applied.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] mfd: twl4030-power: Fix poweroff with PM configuration enabled

[Grazvydas Ignotas]

On Tue, Nov 4, 2014 at 5:42 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Igor Grinberg <grinberg@compulab.co.il> [141104 05:22]:
>> Hi Tony,
>>
>> On 11/02/14 20:07, Tony Lindgren wrote:
>> > Commit e7cd1d1eb16f ("mfd: twl4030-power: Add generic reset
>> > configuration") enabled configuring the PM features for twl4030.
>> >
>> > This caused poweroff command to fail on devices that have the
>> > BCI charger on twl4030 wired, or have power wired for VBUS.
>> > Instead of powering off, the device reboots. This is because
>> > voltage is detected on charger or VBUS with the default bits
>> > enabled for the power transition registers.
>> >
>> > To fix the issue, let's just clear VBUS and CHG bits as we want
>> > poweroff command to keep the system powered off.
>>
>> What about devices that really need to start once VBUS or CHG is connected?
>
> More handling can be added for some cases. With this patch the
> poweron bits will clear to defaults if power is completely removed.
> So start-up with VBUS and CHG works in that case.
>
> However, if you have a battery connected, and you poweroff, with
> this patch the device won't power up with VBUS or CHG connected.
>
> Note that most battery operated devices are not using the charger
> on twl4030 because it has issues charging a completely empty
> battery AFAIK. So most battery powered devices have been using an
> external USB charger chip that's not affected by this patch.

Pandora does, as well as GTA04 AFAIK, but that's not "most devices" I
guess.. At least pandora was booting up on charger connect up until
now. I don't know why shutdown used to work for Russell in legacy boot
and it changed for DT, the device would always start up when there was
AC power here.


Grazvydas

>
> We could consider exporting a function for the charger driver to
> configure the poweron mask. And we could also consider passing a
> mask in ti,use_poweroff = 0xff.
>
>> It seems to me that forcing these bits on power off can break that kind of
>> devices and these settings should really be board specific.
>> What do you think?
>
> There's a patch series for "[RFC,01/16] kernel: Add support for
> poweroff handler call chain" that should help with that. For sure
> the poweroff handling needs to be board specific as some systems
> may need to use a GPIO to shut off a regulator powering something
> before powering off the SoC.
>
> Regards,
>
> Tony
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mfd: twl4030-power: Fix poweroff with PM configuration enabled

[Tony Lindgren]

* Grazvydas Ignotas <notasas@gmail.com> [141112 07:46]:
> On Tue, Nov 4, 2014 at 5:42 PM, Tony Lindgren <tony@atomide.com> wrote:
> > * Igor Grinberg <grinberg@compulab.co.il> [141104 05:22]:
> >> Hi Tony,
> >>
> >> On 11/02/14 20:07, Tony Lindgren wrote:
> >> > Commit e7cd1d1eb16f ("mfd: twl4030-power: Add generic reset
> >> > configuration") enabled configuring the PM features for twl4030.
> >> >
> >> > This caused poweroff command to fail on devices that have the
> >> > BCI charger on twl4030 wired, or have power wired for VBUS.
> >> > Instead of powering off, the device reboots. This is because
> >> > voltage is detected on charger or VBUS with the default bits
> >> > enabled for the power transition registers.
> >> >
> >> > To fix the issue, let's just clear VBUS and CHG bits as we want
> >> > poweroff command to keep the system powered off.
> >>
> >> What about devices that really need to start once VBUS or CHG is connected?
> >
> > More handling can be added for some cases. With this patch the
> > poweron bits will clear to defaults if power is completely removed.
> > So start-up with VBUS and CHG works in that case.
> >
> > However, if you have a battery connected, and you poweroff, with
> > this patch the device won't power up with VBUS or CHG connected.
> >
> > Note that most battery operated devices are not using the charger
> > on twl4030 because it has issues charging a completely empty
> > battery AFAIK. So most battery powered devices have been using an
> > external USB charger chip that's not affected by this patch.
> 
> Pandora does, as well as GTA04 AFAIK, but that's not "most devices" I
> guess.. At least pandora was booting up on charger connect up until
> now. I don't know why shutdown used to work for Russell in legacy boot
> and it changed for DT, the device would always start up when there was
> AC power here.

I think Pandora should still start up fine on charger connect except
after you do a poweroff first?

And my guess is Pandora would not stay powered off earlier after you
did poweroff but would start up again immediately instead.

If something has changed with the default start up events, then
it must be some separate issue. This is because we configure the start
up events only in twl4030_power_off() when poweroff is called, so those
are not touched during the init time.

BTW, the reason why things changed for device tree based booting is
because we're now configuring the PMIC for PM features while for
legacy booting that was only done for n900.

Regards,

Tony

Re: [PATCH] mfd: twl4030-power: Fix poweroff with PM configuration enabled

[Dr. H. Nikolaus Schaller]

Am 12.11.2014 um 16:45 schrieb Grazvydas Ignotas <notasas@gmail.com>:

> On Tue, Nov 4, 2014 at 5:42 PM, Tony Lindgren <tony@atomide.com> wrote:
>> * Igor Grinberg <grinberg@compulab.co.il> [141104 05:22]:
>>> Hi Tony,
>>> 
>>> On 11/02/14 20:07, Tony Lindgren wrote:
>>>> Commit e7cd1d1eb16f ("mfd: twl4030-power: Add generic reset
>>>> configuration") enabled configuring the PM features for twl4030.
>>>> 
>>>> This caused poweroff command to fail on devices that have the
>>>> BCI charger on twl4030 wired, or have power wired for VBUS.
>>>> Instead of powering off, the device reboots. This is because
>>>> voltage is detected on charger or VBUS with the default bits
>>>> enabled for the power transition registers.
>>>> 
>>>> To fix the issue, let's just clear VBUS and CHG bits as we want
>>>> poweroff command to keep the system powered off.
>>> 
>>> What about devices that really need to start once VBUS or CHG is connected?
>> 
>> More handling can be added for some cases. With this patch the
>> poweron bits will clear to defaults if power is completely removed.
>> So start-up with VBUS and CHG works in that case.
>> 
>> However, if you have a battery connected, and you poweroff, with
>> this patch the device won't power up with VBUS or CHG connected.
>> 
>> Note that most battery operated devices are not using the charger
>> on twl4030 because it has issues charging a completely empty
>> battery AFAIK. So most battery powered devices have been using an
>> external USB charger chip that's not affected by this patch.
> 
> Pandora does, as well as GTA04 AFAIK,

Yes. The trick is that the power level that turns on the device is a little higher
and the battery provides enough energy for approx. 30 seconds until it drained
to a level where it turns off. Depending on general setup (WIFI must remain
powered off after boot) this is enough to boot into Linux and start full charging.
Another trick is to modify MLO and U-Boot to raise the charging current.

> but that's not "most devices“ I

Indeed.

> guess.. At least pandora was booting up on charger connect up until
> now.

Same for GTA04.

> I don't know why shutdown used to work for Russell in legacy boot
> and it changed for DT, the device would always start up when there was
> AC power here.
> 
> 
> Grazvydas
> 
>> 
>> We could consider exporting a function for the charger driver to
>> configure the poweron mask. And we could also consider passing a
>> mask in ti,use_poweroff = 0xff.

Yes, exporting these masks would be fine - if a board needs a non-default
setup.

>> 
>>> It seems to me that forcing these bits on power off can break that kind of
>>> devices and these settings should really be board specific.
>>> What do you think?
>> 
>> There's a patch series for "[RFC,01/16] kernel: Add support for
>> poweroff handler call chain" that should help with that. For sure
>> the poweroff handling needs to be board specific as some systems
>> may need to use a GPIO to shut off a regulator powering something
>> before powering off the SoC.
>> 
>> Regards,
>> 
>> Tony
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

BR,
Nikolaus

Re: [PATCH] mfd: twl4030-power: Fix poweroff with PM configuration enabled

[Tony Lindgren]

* Dr. H. Nikolaus Schaller <hns@goldelico.com> [141112 12:28]:
> Am 12.11.2014 um 16:45 schrieb Grazvydas Ignotas <notasas@gmail.com>:
> > On Tue, Nov 4, 2014 at 5:42 PM, Tony Lindgren <tony@atomide.com> wrote:
> > 
> > Pandora does, as well as GTA04 AFAIK,
> 
> Yes. The trick is that the power level that turns on the device is a little higher
> and the battery provides enough energy for approx. 30 seconds until it drained
> to a level where it turns off. Depending on general setup (WIFI must remain
> powered off after boot) this is enough to boot into Linux and start full charging.
> Another trick is to modify MLO and U-Boot to raise the charging current.

Depending on the device and the boot-up speed this seems to work only when
connected to a Linux PC though. AFAIK Windows PCs enforce the 100mA USB power
limit until the device is enumerated which can keep the device in an eternal
reboot loop not being able to ever get the charging going :)
 
> > guess.. At least pandora was booting up on charger connect up until
> > now.
> 
> Same for GTA04.

See the comments I posted earlier today. Have you guys tried the $subject
patch with v3.18-rc series?

It sounds like these comments are based on just reading the patch. Or
else I'm missing something related to some other regression that's
probably unrelated to the $subject patch?

> >> We could consider exporting a function for the charger driver to
> >> configure the poweron mask. And we could also consider passing a
> >> mask in ti,use_poweroff = 0xff.
> 
> Yes, exporting these masks would be fine - if a board needs a non-default
> setup.
 
It seems we've been missing all kind of things. We probably should
first power off any the board specific GPIO regulators. Then configure
the power-up sequence for the board. And then finally call the
twl4030_power_off().

Regards,

Tony

Re: [PATCH] mfd: twl4030-power: Fix poweroff with PM configuration enabled

[Felipe Balbi]

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

On Wed, Nov 12, 2014 at 01:20:27PM -0800, Tony Lindgren wrote:
> * Dr. H. Nikolaus Schaller <hns@goldelico.com> [141112 12:28]:
> > Am 12.11.2014 um 16:45 schrieb Grazvydas Ignotas <notasas@gmail.com>:
> > > On Tue, Nov 4, 2014 at 5:42 PM, Tony Lindgren <tony@atomide.com> wrote:
> > > 
> > > Pandora does, as well as GTA04 AFAIK,
> > 
> > Yes. The trick is that the power level that turns on the device is a little higher
> > and the battery provides enough energy for approx. 30 seconds until it drained
> > to a level where it turns off. Depending on general setup (WIFI must remain
> > powered off after boot) this is enough to boot into Linux and start full charging.
> > Another trick is to modify MLO and U-Boot to raise the charging current.
> 
> Depending on the device and the boot-up speed this seems to work only when
> connected to a Linux PC though. AFAIK Windows PCs enforce the 100mA USB power
> limit until the device is enumerated which can keep the device in an eternal
> reboot loop not being able to ever get the charging going :)

this is actually what the USB Battery Charging spec requires us to
implement. If Linux is doing differently, it's a bug on Linux which
should be fixed :-)

No host is allowed to source more then one unit load (100mA in LS/FS/HS,
150mA in SS) until the device is fully enumerated. Host are also
required to drop max current budget to 8mA (IIRC) if the device doesn't
enumerate for however many minutes (I guess it was a pretty long
threshold, something like half an hour or so. My memory fails me right
now).

-- 
balbi

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

Re: [PATCH] mfd: twl4030-power: Fix poweroff with PM configuration enabled

[NeilBrown]

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

On Wed, 12 Nov 2014 16:31:54 -0600 Felipe Balbi <balbi@ti.com> wrote:

> On Wed, Nov 12, 2014 at 01:20:27PM -0800, Tony Lindgren wrote:
> > * Dr. H. Nikolaus Schaller <hns@goldelico.com> [141112 12:28]:
> > > Am 12.11.2014 um 16:45 schrieb Grazvydas Ignotas <notasas@gmail.com>:
> > > > On Tue, Nov 4, 2014 at 5:42 PM, Tony Lindgren <tony@atomide.com> wrote:
> > > > 
> > > > Pandora does, as well as GTA04 AFAIK,
> > > 
> > > Yes. The trick is that the power level that turns on the device is a little higher
> > > and the battery provides enough energy for approx. 30 seconds until it drained
> > > to a level where it turns off. Depending on general setup (WIFI must remain
> > > powered off after boot) this is enough to boot into Linux and start full charging.
> > > Another trick is to modify MLO and U-Boot to raise the charging current.
> > 
> > Depending on the device and the boot-up speed this seems to work only when
> > connected to a Linux PC though. AFAIK Windows PCs enforce the 100mA USB power
> > limit until the device is enumerated which can keep the device in an eternal
> > reboot loop not being able to ever get the charging going :)
> 
> this is actually what the USB Battery Charging spec requires us to
> implement. If Linux is doing differently, it's a bug on Linux which
> should be fixed :-)
> 
> No host is allowed to source more then one unit load (100mA in LS/FS/HS,
> 150mA in SS) until the device is fully enumerated. Host are also
> required to drop max current budget to 8mA (IIRC) if the device doesn't
> enumerate for however many minutes (I guess it was a pretty long
> threshold, something like half an hour or so. My memory fails me right
> now).
> 

I think the twl4030 driver does do the "right" thing unless the "allow_usb"
module parameter is set, in which case it enables charging at a higher rate
which is 600mA (default value of BCIIREF1).

It would be nice if the driver could check if a charger was plugged in and
act accordingly.
The charger I have for my openmoko is identified by a 47K resistor between ID
and ground.  The twl4030 can detect that easily enough, but it isn't very
standard.

The standard is of course to have D+ and D- shorted, but I don't know if the
twl4030 can detect that?  If it can, then getting some very early code to
check for the short (or the 47k resistor) and quickly enabling charging might
be a sufficient solution.

NeilBrown

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH] mfd: twl4030-power: Fix poweroff with PM configuration enabled

[Tony Lindgren]

* NeilBrown <neilb@suse.de> [141118 19:45]:
> On Wed, 12 Nov 2014 16:31:54 -0600 Felipe Balbi <balbi@ti.com> wrote:
> > 
> > this is actually what the USB Battery Charging spec requires us to
> > implement. If Linux is doing differently, it's a bug on Linux which
> > should be fixed :-)
> > 
> > No host is allowed to source more then one unit load (100mA in LS/FS/HS,
> > 150mA in SS) until the device is fully enumerated. Host are also
> > required to drop max current budget to 8mA (IIRC) if the device doesn't
> > enumerate for however many minutes (I guess it was a pretty long
> > threshold, something like half an hour or so. My memory fails me right
> > now).
> > 
> 
> I think the twl4030 driver does do the "right" thing unless the "allow_usb"
> module parameter is set, in which case it enables charging at a higher rate
> which is 600mA (default value of BCIIREF1).
> 
> It would be nice if the driver could check if a charger was plugged in and
> act accordingly.
> The charger I have for my openmoko is identified by a 47K resistor between ID
> and ground.  The twl4030 can detect that easily enough, but it isn't very
> standard.

Sounds doable to me, feel free to patch it up since you guys are using
the twl4030 charger :)
 
> The standard is of course to have D+ and D- shorted, but I don't know if the
> twl4030 can detect that?  If it can, then getting some very early code to
> check for the short (or the 47k resistor) and quickly enabling charging might
> be a sufficient solution.

I guess. Note that there's also the USB BC1.2 spec that is more
complicated than having the data lines shorted.

Regards,

Tony