| Message ID | 1431525001-5747-1-git-send-email-robertcnelson@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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.)
* 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
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 <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
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 <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
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
* 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
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
* 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
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
* 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
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@0 { regulator-name = "vdds_dpr";