[PATCH] ARM: dts: am335x-bone* enable pmic-shutdown-controller

[PATCH] ARM: dts: am335x-bone* enable pmic-shutdown-controller

[Robert Nelson]

Fixes: http://bugs.elinux.org/issues/143

Commit 672e2b147 which introduced proper shutdown chose to let the PMIC go into sleep-state (aka "RTC-only mode") rather than off-state. This was fine for rev A5 on which that patch was tested, but since then two changes have been made to the power supply scheme:

(rev A6) enable of the 3v3b regulator moved from LDO2 to LDO4 (3v3a)
side-effect: 3v3b rail remains on in sleep-mode (also in off-mode when battery-powered)

(rev A6A) am335x vdds supply moved from LDO3 to LDO1
side-effect: vdds remains supplied in sleep-mode

As a result heavy leakage occurs from vdds (directly) and 3v3b (via i/o pins) to the 3v3a. The amount of current depends on external connections, but for example leaving a serial console cable connected in this state resulted in serious violation of am335x absolute maximum ratings, with potential for hardware damage.

Reported-by: Matthijs van Duin <matthijsvanduin@gmail.com>
Tested-by: Matthijs van Duin <matthijsvanduin@gmail.com>
Signed-off-by: Robert Nelson <robertcnelson@gmail.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Johan Hovold <johan@kernel.org>
---
 arch/arm/boot/dts/am335x-bone-common.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/am335x-bone-common.dtsi b/arch/arm/boot/dts/am335x-bone-common.dtsi
index c3255e0..7198b81 100644
--- a/arch/arm/boot/dts/am335x-bone-common.dtsi
+++ b/arch/arm/boot/dts/am335x-bone-common.dtsi
@@ -223,6 +223,8 @@
 /include/ "tps65217.dtsi"
 
 &tps {
+	ti,pmic-shutdown-controller;
+
 	regulators {
 		dcdc1_reg: regulator at 0 {
 			regulator-name = "vdds_dpr";
-- 
2.1.4

[PATCH] ARM: dts: am335x-bone* enable pmic-shutdown-controller

[Johan Hovold]

On Wed, May 13, 2015 at 08:50:01AM -0500, Robert Nelson wrote:
> Fixes: http://bugs.elinux.org/issues/143
> 
> Commit 672e2b147 which introduced proper shutdown chose to let the PMIC go into sleep-state (aka "RTC-only mode") rather than off-state. This was fine for rev A5 on which that patch was tested, but since then two changes have been made to the power supply scheme:
> 
> (rev A6) enable of the 3v3b regulator moved from LDO2 to LDO4 (3v3a)
> side-effect: 3v3b rail remains on in sleep-mode (also in off-mode when battery-powered)
> 
> (rev A6A) am335x vdds supply moved from LDO3 to LDO1
> side-effect: vdds remains supplied in sleep-mode
> 
> As a result heavy leakage occurs from vdds (directly) and 3v3b (via i/o pins) to the 3v3a. The amount of current depends on external connections, but for example leaving a serial console cable connected in this state resulted in serious violation of am335x absolute maximum ratings, with potential for hardware damage.
> 
> Reported-by: Matthijs van Duin <matthijsvanduin@gmail.com>
> Tested-by: Matthijs van Duin <matthijsvanduin@gmail.com>
> Signed-off-by: Robert Nelson <robertcnelson@gmail.com>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Johan Hovold <johan@kernel.org>

This should go into stable as well:

Fixes: 672e2b147413 ("ARM: dts: am335x-boneblack: enable power off and
rtc wake up")
Cc: stable <stable@vger.kernel.org>	# 3.19
Acked-by: Johan Hovold <johan@kernel.org>

Thanks,
Johan

[PATCH] ARM: dts: am335x-bone* enable pmic-shutdown-controller

[Johan Hovold]

On Wed, May 13, 2015 at 04:16:58PM +0200, Johan Hovold wrote:
> On Wed, May 13, 2015 at 08:50:01AM -0500, Robert Nelson wrote:
> > Fixes: http://bugs.elinux.org/issues/143
> > 
> > Commit 672e2b147 which introduced proper shutdown chose to let the PMIC go into sleep-state (aka "RTC-only mode") rather than off-state. This was fine for rev A5 on which that patch was tested, but since then two changes have been made to the power supply scheme:
> > 
> > (rev A6) enable of the 3v3b regulator moved from LDO2 to LDO4 (3v3a)
> > side-effect: 3v3b rail remains on in sleep-mode (also in off-mode when battery-powered)
> > 
> > (rev A6A) am335x vdds supply moved from LDO3 to LDO1
> > side-effect: vdds remains supplied in sleep-mode
> > 
> > As a result heavy leakage occurs from vdds (directly) and 3v3b (via i/o pins) to the 3v3a. The amount of current depends on external connections, but for example leaving a serial console cable connected in this state resulted in serious violation of am335x absolute maximum ratings, with potential for hardware damage.
> > 
> > Reported-by: Matthijs van Duin <matthijsvanduin@gmail.com>
> > Tested-by: Matthijs van Duin <matthijsvanduin@gmail.com>
> > Signed-off-by: Robert Nelson <robertcnelson@gmail.com>
> > Cc: Tony Lindgren <tony@atomide.com>
> > Cc: Felipe Balbi <balbi@ti.com>
> > Cc: Johan Hovold <johan@kernel.org>
> 
> This should go into stable as well:
> 
> Fixes: 672e2b147413 ("ARM: dts: am335x-boneblack: enable power off and
> rtc wake up")
> Cc: stable <stable@vger.kernel.org>	# 3.19
> Acked-by: Johan Hovold <johan@kernel.org>

By the way, perhaps you should add a comment in the tps node explaining
why ti,pmic-shutdown-controller must not be disabled (on what revisions
of hardware) as well?

Johan

[PATCH] ARM: dts: am335x-bone* enable pmic-shutdown-controller

[Matthijs van Duin]

On 13 May 2015 at 16:48, Johan Hovold <johan@kernel.org> wrote:
> By the way, perhaps you should add a comment in the tps node explaining
> why ti,pmic-shutdown-controller must not be disabled (on what revisions
> of hardware) as well?

There are actually three separate obstacles for RTC-only sleep (only
one of which actually BeagleBone-specific), so the "why" is a bit of a
long story. The end result can however be stated briefly:

Only BBBs prior to rev A6 properly support RTC-only mode. The rare BBW
with a 2.x processor also supports it, provided nothing is connected
to its expansion ports that leaks significant current from 3v3exp to
processor I/Os while in reset.

Any BeagleBone with an AM335x 2.x can be patched to properly support
RTC-only mode. (Though reconnecting vdds to LDO3 is really no fun, and
not a patch I could have done myself.)



For reference, the details on the three problems:


1. Most BeagleBone Whites use AM335x 1.0 which freezes the RTC when
vdd_core is gone or power-on reset is asserted, rendering RTC-only
sleep mostly useless.  The BeagleBone Black, and a few BBWs, use
AM335x 2.x which fixed this.

Of all problems this is the most benign one since it merely results in
loss of functionality. The other two risk hardware damage.


2. TI issued a notice that the AM335x "vdds" supply must be among the
first to come up, and changed the official connection diagram for the
TPS65217C/D (used for boards with DDR3/3L memory) by moving vdds from
LDO3 to LDO1, and the BBB implemented this change in rev A6A.

This change is incompatible with RTC-only sleep (a fact mentioned by
TI), so this rules out RTC-only sleep for any AM335x board with
DDR3/3L memory unless it predates (or ignored) TI's advice or uses a
different power supply scheme altogether. Boards with DDR2 memory
(TPS65217A/B) such as the BBW are not affected.

Curiously, based on preliminary measurements, I have not observed any
leakage to vdds when it is powered up "late" (LDO3). In contrast,
during shutdown vdds begins to leak heavily (far in excess of rated
max current) to other rails once they drop low enough, and moving vdds
to LDO1 only makes this situation persist even longer (and
indefinitely in RTC-only sleep). I've asked TI support for some
clarification on this, but not yet received any response.


3. All BeagleBones have a mismanaged external regulator for the
"3v3exp" (BBW) / "3v3b" (BBB) rail, which remains enabled longer than
the AM335x's 3.3V supplies ("3v3a").

On the BBW, external hardware may end up sourcing current into
processor I/Os, which feeds the 3v3a through protection diodes. Since
the 3v3a is used as enable-signal for the regulator, if non-negligible
current is leaked via this path, 3v3a remains "logic high" and the
situation will persist indefinitely. If the currents remain modest
this won't necessarily violate any absolute maximum ratings, but I'd
still worry about the processor's long-term health.

On BBB rev A6 and later the situation is the same, but without
dependency on external hardware: the current from on-board pull-up
resistors already suffices (3v3a remains at ~1.4V), and since rev A6A
the leakage from vdds would also get the job done. Worse still, since
the buffer for the console port is powered by the 3v3b, having a
serial cable attached causes a dangerous amount of current to be
driven into UART0_RXD (~45 mA) and I captured an event which I fear
may have been a brief latch-up.

On BBBs prior to rev A6 the regulator is enabled by LDO2 (whose only
other use is the power led), which prevents it from staying on
indefinitely. It is however enabled 1ms before and disabled 1ms after
the processor's 3.3V supplies, so external hardware must still avoid
driving much current into I/O pins during those windows, e.g. by
keeping drivers disabled while reset is asserted. As mentioned above,
the console port buffer violates this rule.

Technically this problem is also present when performing a full
shutdown ("OFF-mode"), but then the main supply is cut at the start of
the power-down sequence, which proceeds while running off rapidly
draining capacitors. Heavy current draw from 3v3b will drain them even
faster, thus making the issue self-limiting. (This however does not
apply when running on battery power, in which case even a full
shutdown is hazardous on an unpatched BBB rev A6 or later.)

[PATCH] ARM: dts: am335x-bone* enable pmic-shutdown-controller

[Tony Lindgren]

* Matthijs van Duin <matthijsvanduin@gmail.com> [150515 19:50]:
> On 13 May 2015 at 16:48, Johan Hovold <johan@kernel.org> wrote:
> > By the way, perhaps you should add a comment in the tps node explaining
> > why ti,pmic-shutdown-controller must not be disabled (on what revisions
> > of hardware) as well?
> 
> There are actually three separate obstacles for RTC-only sleep (only
> one of which actually BeagleBone-specific), so the "why" is a bit of a
> long story. The end result can however be stated briefly:
> 
> Only BBBs prior to rev A6 properly support RTC-only mode. The rare BBW
> with a 2.x processor also supports it, provided nothing is connected
> to its expansion ports that leaks significant current from 3v3exp to
> processor I/Os while in reset.
> 
> Any BeagleBone with an AM335x 2.x can be patched to properly support
> RTC-only mode. (Though reconnecting vdds to LDO3 is really no fun, and
> not a patch I could have done myself.)
> 
> 
> 
> For reference, the details on the three problems:
> 
> 
> 1. Most BeagleBone Whites use AM335x 1.0 which freezes the RTC when
> vdd_core is gone or power-on reset is asserted, rendering RTC-only
> sleep mostly useless.  The BeagleBone Black, and a few BBWs, use
> AM335x 2.x which fixed this.
> 
> Of all problems this is the most benign one since it merely results in
> loss of functionality. The other two risk hardware damage.

If some of these depend on the SoC revision and cannot be detected
based on the RTC driver revision register, that information should
be be passed to the RTC driver in platform data. This can be done
with the rev == AM35XX_REV_ES* comparison that can be added to the
mach-omap2/pdata-quirks.c for am335x to initialize platform_data
for the RTC driver.
 
> 2. TI issued a notice that the AM335x "vdds" supply must be among the
> first to come up, and changed the official connection diagram for the
> TPS65217C/D (used for boards with DDR3/3L memory) by moving vdds from
> LDO3 to LDO1, and the BBB implemented this change in rev A6A.
> 
> This change is incompatible with RTC-only sleep (a fact mentioned by
> TI), so this rules out RTC-only sleep for any AM335x board with
> DDR3/3L memory unless it predates (or ignored) TI's advice or uses a
> different power supply scheme altogether. Boards with DDR2 memory
> (TPS65217A/B) such as the BBW are not affected.
> 
> Curiously, based on preliminary measurements, I have not observed any
> leakage to vdds when it is powered up "late" (LDO3). In contrast,
> during shutdown vdds begins to leak heavily (far in excess of rated
> max current) to other rails once they drop low enough, and moving vdds
> to LDO1 only makes this situation persist even longer (and
> indefinitely in RTC-only sleep). I've asked TI support for some
> clarification on this, but not yet received any response.

This information should be passed based on the board revision in
platform_data to the RTC driver assuming we can detect the board
revision during the early boot.. Otherwise we should have revision
specific dts files.

> 3. All BeagleBones have a mismanaged external regulator for the
> "3v3exp" (BBW) / "3v3b" (BBB) rail, which remains enabled longer than
> the AM335x's 3.3V supplies ("3v3a").
> 
> On the BBW, external hardware may end up sourcing current into
> processor I/Os, which feeds the 3v3a through protection diodes. Since
> the 3v3a is used as enable-signal for the regulator, if non-negligible
> current is leaked via this path, 3v3a remains "logic high" and the
> situation will persist indefinitely. If the currents remain modest
> this won't necessarily violate any absolute maximum ratings, but I'd
> still worry about the processor's long-term health.
> 
> On BBB rev A6 and later the situation is the same, but without
> dependency on external hardware: the current from on-board pull-up
> resistors already suffices (3v3a remains at ~1.4V), and since rev A6A
> the leakage from vdds would also get the job done. Worse still, since
> the buffer for the console port is powered by the 3v3b, having a
> serial cable attached causes a dangerous amount of current to be
> driven into UART0_RXD (~45 mA) and I captured an event which I fear
> may have been a brief latch-up.
> 
> On BBBs prior to rev A6 the regulator is enabled by LDO2 (whose only
> other use is the power led), which prevents it from staying on
> indefinitely. It is however enabled 1ms before and disabled 1ms after
> the processor's 3.3V supplies, so external hardware must still avoid
> driving much current into I/O pins during those windows, e.g. by
> keeping drivers disabled while reset is asserted. As mentioned above,
> the console port buffer violates this rule.
> 
> Technically this problem is also present when performing a full
> shutdown ("OFF-mode"), but then the main supply is cut at the start of
> the power-down sequence, which proceeds while running off rapidly
> draining capacitors. Heavy current draw from 3v3b will drain them even
> faster, thus making the issue self-limiting. (This however does not
> apply when running on battery power, in which case even a full
> shutdown is hazardous on an unpatched BBB rev A6 or later.)

Sounds like this should be configured based on the board revision
too.

Regards,

Tony
 

[PATCH] ARM: dts: am335x-bone* enable pmic-shutdown-controller

[Robert Nelson]

On Mon, May 18, 2015 at 10:21 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Matthijs van Duin <matthijsvanduin@gmail.com> [150515 19:50]:
>> On 13 May 2015 at 16:48, Johan Hovold <johan@kernel.org> wrote:
>> > By the way, perhaps you should add a comment in the tps node explaining
>> > why ti,pmic-shutdown-controller must not be disabled (on what revisions
>> > of hardware) as well?
>>
>> There are actually three separate obstacles for RTC-only sleep (only
>> one of which actually BeagleBone-specific), so the "why" is a bit of a
>> long story. The end result can however be stated briefly:
>>
>> Only BBBs prior to rev A6 properly support RTC-only mode. The rare BBW
>> with a 2.x processor also supports it, provided nothing is connected
>> to its expansion ports that leaks significant current from 3v3exp to
>> processor I/Os while in reset.
>>
>> Any BeagleBone with an AM335x 2.x can be patched to properly support
>> RTC-only mode. (Though reconnecting vdds to LDO3 is really no fun, and
>> not a patch I could have done myself.)
>>
>>
>>
>> For reference, the details on the three problems:
>>
>>
>> 1. Most BeagleBone Whites use AM335x 1.0 which freezes the RTC when
>> vdd_core is gone or power-on reset is asserted, rendering RTC-only
>> sleep mostly useless.  The BeagleBone Black, and a few BBWs, use
>> AM335x 2.x which fixed this.
>>
>> Of all problems this is the most benign one since it merely results in
>> loss of functionality. The other two risk hardware damage.
>
> If some of these depend on the SoC revision and cannot be detected
> based on the RTC driver revision register, that information should
> be be passed to the RTC driver in platform data. This can be done
> with the rev == AM35XX_REV_ES* comparison that can be added to the
> mach-omap2/pdata-quirks.c for am335x to initialize platform_data
> for the RTC driver.
>
>> 2. TI issued a notice that the AM335x "vdds" supply must be among the
>> first to come up, and changed the official connection diagram for the
>> TPS65217C/D (used for boards with DDR3/3L memory) by moving vdds from
>> LDO3 to LDO1, and the BBB implemented this change in rev A6A.
>>
>> This change is incompatible with RTC-only sleep (a fact mentioned by
>> TI), so this rules out RTC-only sleep for any AM335x board with
>> DDR3/3L memory unless it predates (or ignored) TI's advice or uses a
>> different power supply scheme altogether. Boards with DDR2 memory
>> (TPS65217A/B) such as the BBW are not affected.
>>
>> Curiously, based on preliminary measurements, I have not observed any
>> leakage to vdds when it is powered up "late" (LDO3). In contrast,
>> during shutdown vdds begins to leak heavily (far in excess of rated
>> max current) to other rails once they drop low enough, and moving vdds
>> to LDO1 only makes this situation persist even longer (and
>> indefinitely in RTC-only sleep). I've asked TI support for some
>> clarification on this, but not yet received any response.
>
> This information should be passed based on the board revision in
> platform_data to the RTC driver assuming we can detect the board
> revision during the early boot.. Otherwise we should have revision
> specific dts files.
>
>> 3. All BeagleBones have a mismanaged external regulator for the
>> "3v3exp" (BBW) / "3v3b" (BBB) rail, which remains enabled longer than
>> the AM335x's 3.3V supplies ("3v3a").
>>
>> On the BBW, external hardware may end up sourcing current into
>> processor I/Os, which feeds the 3v3a through protection diodes. Since
>> the 3v3a is used as enable-signal for the regulator, if non-negligible
>> current is leaked via this path, 3v3a remains "logic high" and the
>> situation will persist indefinitely. If the currents remain modest
>> this won't necessarily violate any absolute maximum ratings, but I'd
>> still worry about the processor's long-term health.
>>
>> On BBB rev A6 and later the situation is the same, but without
>> dependency on external hardware: the current from on-board pull-up
>> resistors already suffices (3v3a remains at ~1.4V), and since rev A6A
>> the leakage from vdds would also get the job done. Worse still, since
>> the buffer for the console port is powered by the 3v3b, having a
>> serial cable attached causes a dangerous amount of current to be
>> driven into UART0_RXD (~45 mA) and I captured an event which I fear
>> may have been a brief latch-up.
>>
>> On BBBs prior to rev A6 the regulator is enabled by LDO2 (whose only
>> other use is the power led), which prevents it from staying on
>> indefinitely. It is however enabled 1ms before and disabled 1ms after
>> the processor's 3.3V supplies, so external hardware must still avoid
>> driving much current into I/O pins during those windows, e.g. by
>> keeping drivers disabled while reset is asserted. As mentioned above,
>> the console port buffer violates this rule.
>>
>> Technically this problem is also present when performing a full
>> shutdown ("OFF-mode"), but then the main supply is cut at the start of
>> the power-down sequence, which proceeds while running off rapidly
>> draining capacitors. Heavy current draw from 3v3b will drain them even
>> faster, thus making the issue self-limiting. (This however does not
>> apply when running on battery power, in which case even a full
>> shutdown is hazardous on an unpatched BBB rev A6 or later.)
>
> Sounds like this should be configured based on the board revision
> too.

All the rev information is in the board's eeprom:

hexdump -e '8/1 "%c"' /sys/bus/i2c/devices/0-0050/eeprom -s 12 -n 4

Rev A5B
0A5B

Rev C
000C

Just another default qwerk to add to Pantelis' bone_capemgr. ;)

Regards,

-- 
Robert Nelson
https://rcn-ee.com/

[PATCH] ARM: dts: am335x-bone* enable pmic-shutdown-controller

[Tony Lindgren]

* Robert Nelson <robertcnelson@gmail.com> [150518 09:15]:
> On Mon, May 18, 2015 at 10:21 AM, Tony Lindgren <tony@atomide.com> wrote:
> 
> All the rev information is in the board's eeprom:
> 
> hexdump -e '8/1 "%c"' /sys/bus/i2c/devices/0-0050/eeprom -s 12 -n 4
> 
> Rev A5B
> 0A5B
> 
> Rev C
> 000C
> 
> Just another default qwerk to add to Pantelis' bone_capemgr. ;)

It seems we should not even instantiate some devices on BBB
until the EEPROM is parsed.. So maybe something like this:

1. The problem devices are initially set with status = "disabled"
   in the dts

2. We set up drivers/*/bbb-eeprom.c that parses the board
   revision at module_init time, and then flips the selected
   devices to have status = "enabled" and populates the revision
   info based on the eeprom and SoC revision passed in pdata.
   Then those devices get their struct device created and
   probed, but at a much later time.

So rather than trying to init all that early, let's just
init them much later when we have the proper I2C driver
running?

Regards,

Tony

[PATCH] ARM: dts: am335x-bone* enable pmic-shutdown-controller

[Robert Nelson]

On Mon, May 18, 2015 at 11:29 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Robert Nelson <robertcnelson@gmail.com> [150518 09:15]:
>> On Mon, May 18, 2015 at 10:21 AM, Tony Lindgren <tony@atomide.com> wrote:
>>
>> All the rev information is in the board's eeprom:
>>
>> hexdump -e '8/1 "%c"' /sys/bus/i2c/devices/0-0050/eeprom -s 12 -n 4
>>
>> Rev A5B
>> 0A5B
>>
>> Rev C
>> 000C
>>
>> Just another default qwerk to add to Pantelis' bone_capemgr. ;)
>
> It seems we should not even instantiate some devices on BBB
> until the EEPROM is parsed.. So maybe something like this:
>
> 1. The problem devices are initially set with status = "disabled"
>    in the dts
>
> 2. We set up drivers/*/bbb-eeprom.c that parses the board
>    revision at module_init time, and then flips the selected
>    devices to have status = "enabled" and populates the revision
>    info based on the eeprom and SoC revision passed in pdata.
>    Then those devices get their struct device created and
>    probed, but at a much later time.
>
> So rather than trying to init all that early, let's just
> init them much later when we have the proper I2C driver
> running?

I see that working just fine.  We (beagleboard.org) enforce the eeprom
data, as all the official images require a proper baseboard eeprom.

We just have to be very careful to limit the scope, otherwise we will
end up with Pantelis' rejected capebus from the v3.2.x days...

Regards,

-- 
Robert Nelson
https://rcn-ee.com/

[PATCH] ARM: dts: am335x-bone* enable pmic-shutdown-controller

[Tony Lindgren]

* Robert Nelson <robertcnelson@gmail.com> [150518 09:51]:
> On Mon, May 18, 2015 at 11:29 AM, Tony Lindgren <tony@atomide.com> wrote:
> > * Robert Nelson <robertcnelson@gmail.com> [150518 09:15]:
> >> On Mon, May 18, 2015 at 10:21 AM, Tony Lindgren <tony@atomide.com> wrote:
> >>
> >> All the rev information is in the board's eeprom:
> >>
> >> hexdump -e '8/1 "%c"' /sys/bus/i2c/devices/0-0050/eeprom -s 12 -n 4
> >>
> >> Rev A5B
> >> 0A5B
> >>
> >> Rev C
> >> 000C
> >>
> >> Just another default qwerk to add to Pantelis' bone_capemgr. ;)
> >
> > It seems we should not even instantiate some devices on BBB
> > until the EEPROM is parsed.. So maybe something like this:
> >
> > 1. The problem devices are initially set with status = "disabled"
> >    in the dts
> >
> > 2. We set up drivers/*/bbb-eeprom.c that parses the board
> >    revision at module_init time, and then flips the selected
> >    devices to have status = "enabled" and populates the revision
> >    info based on the eeprom and SoC revision passed in pdata.
> >    Then those devices get their struct device created and
> >    probed, but at a much later time.
> >
> > So rather than trying to init all that early, let's just
> > init them much later when we have the proper I2C driver
> > running?
> 
> I see that working just fine.  We (beagleboard.org) enforce the eeprom
> data, as all the official images require a proper baseboard eeprom.

OK
 
> We just have to be very careful to limit the scope, otherwise we will
> end up with Pantelis' rejected capebus from the v3.2.x days...

Naturally I was thinking #2 above would use Pantelis' code for
CONFIG_OF_OVERLAY in mainline. But instead of the earlier patches,
we can make things happen much later on to avoid the detect of
EEPROM early on.

Regards,

Tony

[PATCH] ARM: dts: am335x-bone* enable pmic-shutdown-controller

[Pantelis Antoniou]

Hi Tony,

> On May 18, 2015, at 20:03 , Tony Lindgren <tony@atomide.com> wrote:
> 
> * Robert Nelson <robertcnelson@gmail.com> [150518 09:51]:
>> On Mon, May 18, 2015 at 11:29 AM, Tony Lindgren <tony@atomide.com> wrote:
>>> * Robert Nelson <robertcnelson@gmail.com> [150518 09:15]:
>>>> On Mon, May 18, 2015 at 10:21 AM, Tony Lindgren <tony@atomide.com> wrote:
>>>> 
>>>> All the rev information is in the board's eeprom:
>>>> 
>>>> hexdump -e '8/1 "%c"' /sys/bus/i2c/devices/0-0050/eeprom -s 12 -n 4
>>>> 
>>>> Rev A5B
>>>> 0A5B
>>>> 
>>>> Rev C
>>>> 000C
>>>> 
>>>> Just another default qwerk to add to Pantelis' bone_capemgr. ;)
>>> 
>>> It seems we should not even instantiate some devices on BBB
>>> until the EEPROM is parsed.. So maybe something like this:
>>> 
>>> 1. The problem devices are initially set with status = "disabled"
>>>   in the dts
>>> 
>>> 2. We set up drivers/*/bbb-eeprom.c that parses the board
>>>   revision at module_init time, and then flips the selected
>>>   devices to have status = "enabled" and populates the revision
>>>   info based on the eeprom and SoC revision passed in pdata.
>>>   Then those devices get their struct device created and
>>>   probed, but at a much later time.
>>> 
>>> So rather than trying to init all that early, let's just
>>> init them much later when we have the proper I2C driver
>>> running?
>> 
>> I see that working just fine.  We (beagleboard.org) enforce the eeprom
>> data, as all the official images require a proper baseboard eeprom.
> 
> OK
> 
>> We just have to be very careful to limit the scope, otherwise we will
>> end up with Pantelis' rejected capebus from the v3.2.x days...
> 
> Naturally I was thinking #2 above would use Pantelis' code for
> CONFIG_OF_OVERLAY in mainline. But instead of the earlier patches,
> we can make things happen much later on to avoid the detect of
> EEPROM early on.
> 


Already been taken care of:

https://lkml.org/lkml/2015/2/18/258

The patchset works, but it still needs some review eyeballs.
There might be a rename to variants or something.

You were part of that thread too :)

I think it?s best if we go this path instead of twiddling with the
status properties manually. Conceptually the idea is similar to
the detection of the white/black, with the added joy of revision
detection.

Ain?t hardware designers a fun bunch or what?

> Regards,
> 
> Tony

Regards

? Pantelis

[PATCH] ARM: dts: am335x-bone* enable pmic-shutdown-controller

[Tony Lindgren]

* Pantelis Antoniou <pantelis.antoniou@konsulko.com> [150518 11:02]:
> Hi Tony,
> 
> > On May 18, 2015, at 20:03 , Tony Lindgren <tony@atomide.com> wrote:
> > 
> > * Robert Nelson <robertcnelson@gmail.com> [150518 09:51]:
> >> On Mon, May 18, 2015 at 11:29 AM, Tony Lindgren <tony@atomide.com> wrote:
> >>> * Robert Nelson <robertcnelson@gmail.com> [150518 09:15]:
> >>>> On Mon, May 18, 2015 at 10:21 AM, Tony Lindgren <tony@atomide.com> wrote:
> >>>> 
> >>>> All the rev information is in the board's eeprom:
> >>>> 
> >>>> hexdump -e '8/1 "%c"' /sys/bus/i2c/devices/0-0050/eeprom -s 12 -n 4
> >>>> 
> >>>> Rev A5B
> >>>> 0A5B
> >>>> 
> >>>> Rev C
> >>>> 000C
> >>>> 
> >>>> Just another default qwerk to add to Pantelis' bone_capemgr. ;)
> >>> 
> >>> It seems we should not even instantiate some devices on BBB
> >>> until the EEPROM is parsed.. So maybe something like this:
> >>> 
> >>> 1. The problem devices are initially set with status = "disabled"
> >>>   in the dts
> >>> 
> >>> 2. We set up drivers/*/bbb-eeprom.c that parses the board
> >>>   revision at module_init time, and then flips the selected
> >>>   devices to have status = "enabled" and populates the revision
> >>>   info based on the eeprom and SoC revision passed in pdata.
> >>>   Then those devices get their struct device created and
> >>>   probed, but at a much later time.
> >>> 
> >>> So rather than trying to init all that early, let's just
> >>> init them much later when we have the proper I2C driver
> >>> running?
> >> 
> >> I see that working just fine.  We (beagleboard.org) enforce the eeprom
> >> data, as all the official images require a proper baseboard eeprom.
> > 
> > OK
> > 
> >> We just have to be very careful to limit the scope, otherwise we will
> >> end up with Pantelis' rejected capebus from the v3.2.x days...
> > 
> > Naturally I was thinking #2 above would use Pantelis' code for
> > CONFIG_OF_OVERLAY in mainline. But instead of the earlier patches,
> > we can make things happen much later on to avoid the detect of
> > EEPROM early on.
> 
> Already been taken care of:
> 
> https://lkml.org/lkml/2015/2/18/258
> 
> The patchset works, but it still needs some review eyeballs.
> There might be a rename to variants or something.
> 
> You were part of that thread too :)

Right, and what I mean with #2 above is that we can make this all
into a regular drivers/* device driver with no need for the
early hacks in your series in arch/arm/mach-omap2/am33xx-dt-quirks.c.
 
> I think it?s best if we go this path instead of twiddling with the
> status properties manually. Conceptually the idea is similar to
> the detection of the white/black, with the added joy of revision
> detection.

If some device drivers depend on the I2C EEPROM, we should not
create the struct device entry for those until the I2C EEPROM
driver has parsed the EEPROM. And there's no need to do that
early, we want to do everything as late as possible. That way
we have real debug console available in most cases.
 
> Ain?t hardware designers a fun bunch or what?

We need something equal to the MS DOS boot floppy to limit their
ideas :)

Regards,

Tony

[PATCH] ARM: dts: am335x-bone* enable pmic-shutdown-controller

[Pantelis Antoniou]

Hi Tony,

> On May 18, 2015, at 21:14 , Tony Lindgren <tony@atomide.com> wrote:
> 
> * Pantelis Antoniou <pantelis.antoniou@konsulko.com> [150518 11:02]:
>> Hi Tony,
>> 
>>> On May 18, 2015, at 20:03 , Tony Lindgren <tony@atomide.com> wrote:
>>> 
>>> * Robert Nelson <robertcnelson@gmail.com> [150518 09:51]:
>>>> On Mon, May 18, 2015 at 11:29 AM, Tony Lindgren <tony@atomide.com> wrote:
>>>>> * Robert Nelson <robertcnelson@gmail.com> [150518 09:15]:
>>>>>> On Mon, May 18, 2015 at 10:21 AM, Tony Lindgren <tony@atomide.com> wrote:
>>>>>> 
>>>>>> All the rev information is in the board's eeprom:
>>>>>> 
>>>>>> hexdump -e '8/1 "%c"' /sys/bus/i2c/devices/0-0050/eeprom -s 12 -n 4
>>>>>> 
>>>>>> Rev A5B
>>>>>> 0A5B
>>>>>> 
>>>>>> Rev C
>>>>>> 000C
>>>>>> 
>>>>>> Just another default qwerk to add to Pantelis' bone_capemgr. ;)
>>>>> 
>>>>> It seems we should not even instantiate some devices on BBB
>>>>> until the EEPROM is parsed.. So maybe something like this:
>>>>> 
>>>>> 1. The problem devices are initially set with status = "disabled"
>>>>>  in the dts
>>>>> 
>>>>> 2. We set up drivers/*/bbb-eeprom.c that parses the board
>>>>>  revision at module_init time, and then flips the selected
>>>>>  devices to have status = "enabled" and populates the revision
>>>>>  info based on the eeprom and SoC revision passed in pdata.
>>>>>  Then those devices get their struct device created and
>>>>>  probed, but at a much later time.
>>>>> 
>>>>> So rather than trying to init all that early, let's just
>>>>> init them much later when we have the proper I2C driver
>>>>> running?
>>>> 
>>>> I see that working just fine.  We (beagleboard.org) enforce the eeprom
>>>> data, as all the official images require a proper baseboard eeprom.
>>> 
>>> OK
>>> 
>>>> We just have to be very careful to limit the scope, otherwise we will
>>>> end up with Pantelis' rejected capebus from the v3.2.x days...
>>> 
>>> Naturally I was thinking #2 above would use Pantelis' code for
>>> CONFIG_OF_OVERLAY in mainline. But instead of the earlier patches,
>>> we can make things happen much later on to avoid the detect of
>>> EEPROM early on.
>> 
>> Already been taken care of:
>> 
>> https://lkml.org/lkml/2015/2/18/258
>> 
>> The patchset works, but it still needs some review eyeballs.
>> There might be a rename to variants or something.
>> 
>> You were part of that thread too :)
> 
> Right, and what I mean with #2 above is that we can make this all
> into a regular drivers/* device driver with no need for the
> early hacks in your series in arch/arm/mach-omap2/am33xx-dt-quirks.c.
> 

It?s going to be tough. This is touching the pmic that needs to be 
initialized early since a whole bunch of drivers depend on it.

>> I think it?s best if we go this path instead of twiddling with the
>> status properties manually. Conceptually the idea is similar to
>> the detection of the white/black, with the added joy of revision
>> detection.
> 
> If some device drivers depend on the I2C EEPROM, we should not
> create the struct device entry for those until the I2C EEPROM
> driver has parsed the EEPROM. And there's no need to do that
> early, we want to do everything as late as possible. That way
> we have real debug console available in most cases.
> 

FWIW the first patch used an early platform device, but that made things
even more complicated.

>> Ain?t hardware designers a fun bunch or what?
> 
> We need something equal to the MS DOS boot floppy to limit their
> ideas :)
> 

I think hardware people still clink to the idea that this whole OS business
is a fad and MSDOS will make a comeback any minute now :) 

> Regards,
> 
> Tony

Regards

? Pantelis

[PATCH] ARM: dts: am335x-bone* enable pmic-shutdown-controller

[Tony Lindgren]

* Pantelis Antoniou <pantelis.antoniou@konsulko.com> [150518 11:19]:
> Hi Tony,
> 
> > On May 18, 2015, at 21:14 , Tony Lindgren <tony@atomide.com> wrote:
> > 
> > * Pantelis Antoniou <pantelis.antoniou@konsulko.com> [150518 11:02]:
> >> Hi Tony,
> >> 
> >>> On May 18, 2015, at 20:03 , Tony Lindgren <tony@atomide.com> wrote:
> >>> 
> >>> * Robert Nelson <robertcnelson@gmail.com> [150518 09:51]:
> >>>> On Mon, May 18, 2015 at 11:29 AM, Tony Lindgren <tony@atomide.com> wrote:
> >>>>> * Robert Nelson <robertcnelson@gmail.com> [150518 09:15]:
> >>>>>> On Mon, May 18, 2015 at 10:21 AM, Tony Lindgren <tony@atomide.com> wrote:
> >>>>>> 
> >>>>>> All the rev information is in the board's eeprom:
> >>>>>> 
> >>>>>> hexdump -e '8/1 "%c"' /sys/bus/i2c/devices/0-0050/eeprom -s 12 -n 4
> >>>>>> 
> >>>>>> Rev A5B
> >>>>>> 0A5B
> >>>>>> 
> >>>>>> Rev C
> >>>>>> 000C
> >>>>>> 
> >>>>>> Just another default qwerk to add to Pantelis' bone_capemgr. ;)
> >>>>> 
> >>>>> It seems we should not even instantiate some devices on BBB
> >>>>> until the EEPROM is parsed.. So maybe something like this:
> >>>>> 
> >>>>> 1. The problem devices are initially set with status = "disabled"
> >>>>>  in the dts
> >>>>> 
> >>>>> 2. We set up drivers/*/bbb-eeprom.c that parses the board
> >>>>>  revision at module_init time, and then flips the selected
> >>>>>  devices to have status = "enabled" and populates the revision
> >>>>>  info based on the eeprom and SoC revision passed in pdata.
> >>>>>  Then those devices get their struct device created and
> >>>>>  probed, but at a much later time.
> >>>>> 
> >>>>> So rather than trying to init all that early, let's just
> >>>>> init them much later when we have the proper I2C driver
> >>>>> running?
> >>>> 
> >>>> I see that working just fine.  We (beagleboard.org) enforce the eeprom
> >>>> data, as all the official images require a proper baseboard eeprom.
> >>> 
> >>> OK
> >>> 
> >>>> We just have to be very careful to limit the scope, otherwise we will
> >>>> end up with Pantelis' rejected capebus from the v3.2.x days...
> >>> 
> >>> Naturally I was thinking #2 above would use Pantelis' code for
> >>> CONFIG_OF_OVERLAY in mainline. But instead of the earlier patches,
> >>> we can make things happen much later on to avoid the detect of
> >>> EEPROM early on.
> >> 
> >> Already been taken care of:
> >> 
> >> https://lkml.org/lkml/2015/2/18/258
> >> 
> >> The patchset works, but it still needs some review eyeballs.
> >> There might be a rename to variants or something.
> >> 
> >> You were part of that thread too :)
> > 
> > Right, and what I mean with #2 above is that we can make this all
> > into a regular drivers/* device driver with no need for the
> > early hacks in your series in arch/arm/mach-omap2/am33xx-dt-quirks.c.
> > 
> 
> It?s going to be tough. This is touching the pmic that needs to be 
> initialized early since a whole bunch of drivers depend on it.

With the $subject driver we just need to have have RTC driver not
probe until the the EEPROM is parsed in the drivers/foo/bbb-eeprom.c
driver and the RTC overlay is initialized. Or what's the issue
you're talking about?

But in any case, let's not merge a copy of the I2C driver into
arch/arm/mach-omap2/am33xx-dt-quirks.c. We can do all this with
drivers/foo/bbb-eeprom.c that's just a regular device driver.
 
> >> I think it?s best if we go this path instead of twiddling with the
> >> status properties manually. Conceptually the idea is similar to
> >> the detection of the white/black, with the added joy of revision
> >> detection.
> > 
> > If some device drivers depend on the I2C EEPROM, we should not
> > create the struct device entry for those until the I2C EEPROM
> > driver has parsed the EEPROM. And there's no need to do that
> > early, we want to do everything as late as possible. That way
> > we have real debug console available in most cases.
> > 
> 
> FWIW the first patch used an early platform device, but that made things
> even more complicated.

No need to do any early platform devices, we just need to delay
the creation of struct device for the problem drivers.

Regards,

Tony

[PATCH] ARM: dts: am335x-bone* enable pmic-shutdown-controller

[Matthijs van Duin]

On 18 May 2015 at 17:21, Tony Lindgren <tony@atomide.com> wrote:
> If some of these depend on the SoC revision and cannot be detected
> based on the RTC driver revision register, that information should
> be be passed to the RTC driver in platform data.

The SoC revision is easy enough to check via the control module as
usual, but is not really the biggest issue. Even on SoC rev 1.0
RTC-only sleep still has some functionality (the RTC freezes if PMIC
enters sleep-state, but that may still be preferred compared to
complete loss of date and time), so I'm not even sure I'd bother with
this check.

The serious problems however depend on the PCB: Entering RTC-only
sleep is only properly supported on some early prototype series
(pre-A6) of the BeagleBone Black. Since rev A6A (which includes all
production versions) it is not supported at all.

On rev A6 of the Black, as well as (afaik) all versions of the White
(if anyone cares, since they normally have SoC rev 1.0), the impact of
entering RTC-only sleep is heavily dependent on external connections
and hardware, most of which not really detectable. It is possible to
go from "hmm, annoying standby current" to a recipe for stir-fried I/O
pin simply by connecting a console cable while the device is in sleep.
This is not something the kernel can foresee or prevent.

I'm pretty sure the sane thing to do here is disable RTC-only sleep by
default. People who care about it can still enable it after they
verified the particular combination of pcb revision, external
hardware, and usage scenario is safe (or patched the hardware to make
it safe, or decided they're willing to take the risk.)

It may be useful to leave some references for those who seek guidance:

1. The issues with AM335x rev 1.0 SoCs are documented in the errata.

2. Unfortunately I still have no idea why TI altered the connection
diagram w.r.t. "VDDS" (and as a consequence officially defeatured
RTC-only sleep) when the TPS65217C/D PMIC for AM335x + DDR3/3L memory
is used. Maybe I'll still get an answer from TI on E2E, but I'm also
planning to do more thorough testing this week especially w.r.t.
current consumption patterns during boot and shutdown.

3. The BeagleBone regulator problem is particularly notorious among
people who (try to) use the PMIC's battery power support, since the
problem then also surfaces during full shutdown. It has a long
discussion thread here:
https://groups.google.com/d/topic/beagleboard/7sxPePT7wkM/discussion
which includes two posts by me with fairly detailed analysis along
with scope pics:
https://groups.google.com/d/msg/beagleboard/7sxPePT7wkM/3vFMPydR20IJ
https://groups.google.com/d/msg/beagleboard/7sxPePT7wkM/V1Ft-xxh0agJ

[PATCH] ARM: dts: am335x-bone* enable pmic-shutdown-controller

[Tony Lindgren]

* Matthijs van Duin <matthijsvanduin@gmail.com> [150519 00:27]:
> On 18 May 2015 at 17:21, Tony Lindgren <tony@atomide.com> wrote:
> > If some of these depend on the SoC revision and cannot be detected
> > based on the RTC driver revision register, that information should
> > be be passed to the RTC driver in platform data.
> 
> The SoC revision is easy enough to check via the control module as
> usual, but is not really the biggest issue. Even on SoC rev 1.0
> RTC-only sleep still has some functionality (the RTC freezes if PMIC
> enters sleep-state, but that may still be preferred compared to
> complete loss of date and time), so I'm not even sure I'd bother with
> this check.

OK
 
> The serious problems however depend on the PCB: Entering RTC-only
> sleep is only properly supported on some early prototype series
> (pre-A6) of the BeagleBone Black. Since rev A6A (which includes all
> production versions) it is not supported at all.
> 
> On rev A6 of the Black, as well as (afaik) all versions of the White
> (if anyone cares, since they normally have SoC rev 1.0), the impact of
> entering RTC-only sleep is heavily dependent on external connections
> and hardware, most of which not really detectable. It is possible to
> go from "hmm, annoying standby current" to a recipe for stir-fried I/O
> pin simply by connecting a console cable while the device is in sleep.
> This is not something the kernel can foresee or prevent.
> 
> I'm pretty sure the sane thing to do here is disable RTC-only sleep by
> default. People who care about it can still enable it after they
> verified the particular combination of pcb revision, external
> hardware, and usage scenario is safe (or patched the hardware to make
> it safe, or decided they're willing to take the risk.)

Makes sense to me. Robert, care to update your patch to summarize
some of the "why" parts?

> It may be useful to leave some references for those who seek guidance:
> 
> 1. The issues with AM335x rev 1.0 SoCs are documented in the errata.
> 
> 2. Unfortunately I still have no idea why TI altered the connection
> diagram w.r.t. "VDDS" (and as a consequence officially defeatured
> RTC-only sleep) when the TPS65217C/D PMIC for AM335x + DDR3/3L memory
> is used. Maybe I'll still get an answer from TI on E2E, but I'm also
> planning to do more thorough testing this week especially w.r.t.
> current consumption patterns during boot and shutdown.
> 
> 3. The BeagleBone regulator problem is particularly notorious among
> people who (try to) use the PMIC's battery power support, since the
> problem then also surfaces during full shutdown. It has a long
> discussion thread here:
> https://groups.google.com/d/topic/beagleboard/7sxPePT7wkM/discussion
> which includes two posts by me with fairly detailed analysis along
> with scope pics:
> https://groups.google.com/d/msg/beagleboard/7sxPePT7wkM/3vFMPydR20IJ
> https://groups.google.com/d/msg/beagleboard/7sxPePT7wkM/V1Ft-xxh0agJ

OK thanks for the good summary.

Regards,

Tony