Optimizing AM335x IO Power in DeepSleep0

Jonathan Cormier said:

Error during b.:

Error reading memory: Errors during memory.readData(): Address: 0x4c000008 Error: 0x200000 (am335x-ddr-analysis2.dss#38)

Connecting to DAP doesn't work during this stage.

I added some notes on the page that you can execute "devmem2 0x44e00414 w 0x12500002" from the console to enable the DEBUGSS clock per your earlier comments.  This particular issue is actually because the EMIF clock is disabled while you're trying to read the EMIF registers.  I just updated the am335x-ddr-analysis.dss script with some extra try/catch statements so that I can limp past these errors and still read the rest of the registers.  Can you please update your script and try it again?

Jonathan Cormier said:

< CONTROL: VTP_CTRL = 0x00010167 --- > CONTROL: VTP_CTRL = 0x00000067

My very first power up showed the values you initially reported, but even after subsequent boots I saw the second value.  I think that might just be due to the board heating up or something.

Can you give another update as to the current status?  I see some conflicting data points as to whether suspend is broken or not.

Brad Griffis said:

One other note... Regarding "validation" of the IO spreadsheet, I recommend actually using a multimeter to look at each pin during DS0 to see if the measured state agrees with your spreadsheet.  This exercise will help you find mistakes from your original entries in the spreadsheet.  If for example you see a level that doesn't match the spreadsheet (including intermediate levels), then this indicates you have something to fix within the spreadsheet and your corresponding software.  I'm going to add this info to the wiki as well.

This advice has proven to be very helpful so far.  Just finished probing all the pins and there are definitely some that are still in conflict.  One noteworthy conflict is the TLV320aic26 which is still driving several clocks into the ARM even during standby which is likely fighting some of the pulldowns.

Still need to go through all the voltages measured and compare them to the expected levels. And then change pinmuxes.  One note, is that this process would have been a quicker if the PadConf spreadsheet had been either editable or copyable.  If I had been able to write the voltages directly into the spreadsheet in column U it would make it easier to compare.  As it stands I had to manually copy Pin Count, Pad State, and Pull Settings into another spreadsheet and then keep cross referencing Terminal Names as I went through measuring points.

If we aren't putting GPIO0 into sleep, does that mean its deep sleep state doesn't apply?  For example GPIO0[20] isn't connected to anything and the spreadsheet says it should be output low but I measured about 0.1V which seems likely that its floating.  This should be mentioned in your guide.

Brad Griffis said:

Can you give another update as to the current status?  I see some conflicting data points as to whether suspend is broken or not.

I have not seen any problems resuming from Standby with your CM3 patch when VTT is disabled.  The only negative was the ~6mW power increase.

Jonathan Cormier said:

One noteworthy conflict is the TLV320aic26 which is still driving several clocks into the ARM even during standby which is likely fighting some of the pulldowns.

I just had a quick look at the AIC26 data manual.  There's a /PWD pin for power-down (though that pin can be repurposed as something else).  Do you know if your design keeps that as a power-down pin?  If so, you should set the state of the corresponding I/O to pull that low.  I expect the AIC26 should not be clocking anything in its low power mode.  I imagine the power of the AIC26 (i.e. doing conversions, etc.) is much more power than even the contention on the pins.

Jonathan Cormier said:

One note, is that this process would have been a quicker if the PadConf spreadsheet had been either editable or copyable.

I agree with you and have requested that the spreadsheet maintainer add a column where you can record voltages.

Jonathan Cormier said:

If we aren't putting GPIO0 into sleep, does that mean its deep sleep state doesn't apply?

Correct. This is another thing I requested the maintainer to try and address though it might be too difficult.  Ideally we can fix it in the spreadsheet itself.  If not, I'll add some notes.

Jonathan Cormier said:

I have not seen any problems resuming from Standby with your CM3 patch when VTT is disabled.  The only negative was the ~6mW power increase.

Are you saying:

• Leaving VTT enabled all the time works fine, but results in an additional 6mW of power consumption during standby.
• Dynamically disabling/re-enabling VTT does not work for standby (but it works for DS0)?

Brad Griffis said:

Jonathan Cormier

One noteworthy conflict is the TLV320aic26 which is still driving several clocks into the ARM even during standby which is likely fighting some of the pulldowns.

I just had a quick look at the AIC26 data manual.  There's a /PWD pin for power-down (though that pin can be repurposed as something else).  Do you know if your design keeps that as a power-down pin?  If so, you should set the state of the corresponding I/O to pull that low.  I expect the AIC26 should not be clocking anything in its low power mode.  I imagine the power of the AIC26 (i.e. doing conversions, etc.) is much more power than even the contention on the pins.

Sorry this is where things get a little complicated.  I'm using a development kit in order to investigate the power usage of our SOM.  The devkit was never intended for low power.  We are going to be doing a custom baseboard design for lower power.  All of my power measurements have been intending to ignore the power usage of the devkit and only include power used by the SOM.  So the fact that the AIC isn't sleeping shouldn't be directly showing up in my measurements but the clocks its driving could be affecting my power draw.  For the purpose of this experiment I may just have them pull the AIC26 off the devkit since it won't be on the custom design.

Brad Griffis said:

Jonathan Cormier

One note, is that this process would have been a quicker if the PadConf spreadsheet had been either editable or copyable.

I agree with you and have requested that the spreadsheet maintainer add a column where you can record voltages.

Perhaps just don't lock all U and up columns?

Brad Griffis said:

Jonathan Cormier

I have not seen any problems resuming from Standby with your CM3 patch when VTT is disabled.  The only negative was the ~6mW power increase.

Are you saying:

• Leaving VTT enabled all the time works fine, but results in an additional 6mW of power consumption during standby.
• Dynamically disabling/re-enabling VTT does not work for standby (but it works for DS0)?

What your describing above fits the state before your patch, except having VTT enabled all the time was closer to an additional 12mw.

After the patch, I am able to disable the VTT during standby and resume without issue.  So the patch is helping.

The power increase was refering to the following measurements i posted.

Jonathan Cormier said:

2. Power usage  (_vtt means VTT disabled)

Before Patch:

• Idle: 809mW
• standby: 136mW
• standby_vtt: 133mW  (Broken resume)
• mem: 113mW
• mem_vtt: 103mW

After patch:

• Idle: 815mW (+6mW)
• standby: 142mW (+6mW)
• standby_vtt: 132mW (-1mW)
• mem: 117mW (+4mW)
• mem_vtt: 106mW (+3mW)

Brad Griffis said:

Jonathan Cormier

If we aren't putting GPIO0 into sleep, does that mean its deep sleep state doesn't apply?

Correct. This is another thing I requested the maintainer to try and address though it might be too difficult.  Ideally we can fix it in the spreadsheet itself.  If not, I'll add some notes.

Is the deep sleep state for gpio0 pins disabled even if there pinmux isn't set to gpio or does that only apply when pinmuxed to gpio?

I updated the pinmuxes and verified the pins that were at odd voltages now have better voltage levels.  Unfortunately there wasn't a change in power usage.

I then had the AIC26 PWD pulled low in hardware as well as disabling the 24Mhz Audio sysclk.  Power usage dropped by 15mW. So currently at ~92mW.

Brad Griffis said:

Can you run another quick test for me? You'll need your special "jtag stop" firmware in order to poke a register just before you hit DS0. Here's the procedure:

1. Initiate DS0, e.g. "echo mem > /sys/power/state"
2. Connect to the M3.
3. In the memory window go to 0x44E00C0C. (I see a value of 0x0E000061 in mine.)
4. Write the value 0x06000070 into there instead. (The register before it at 0x44E00C08 should hopefully become zero when you do this.)
5. Type in "SP" for the memory address and change the second 32-bit value from 1->0 to make stop=0.
6. Disconnect.

What's the power consumption while you're asleep in this scenario?

Does this create any problems, i.e. it doesn't crash the board or anything right?

Power usage during mem was ~75.7mW.  Not marginally different to power usage without change ~76.2mW.

Was unable to resume after this register change.  Power levels came back up but console didn't respond.

Tested this twice with same results.

Registers before

Registers after

Well that's disappointing on both fronts!  I was hoping for a more substantial power reduction, and I was not expecting it to cause any stability issues.  (Though this might be why it's the way it is in the first place!)

Before I completely give up on this idea, would you please humor and try this one more time.  Here are a couple binaries I just made.  Both of them have the ioctrl changes from yesterday that fixed your standby issue.  (Though we're testing "mem" now, just so we're on the same page.)  Additionally, I updated the code so that it should transition the PER domain to OFF (rather than RET) without the need for you to poke registers through the JTAG window.  I have two versions: one with the spin loop and one without: <files-removed: bad idea!>

Would you mind trying the version without the spin loop and testing it out?  In particular, I'm wondering if you don't ever connect with JTAG does it work any better.

I tested this code on both a BeagleBone Black as well as a AM335x EVM.  It worked fine on both.  So I'm a bit surprised it's not working on your board.  I through the version with the spin loop in there too in case you want to take a look at things close to suspend.
 

Oops! I'm testing on the 4.1 kernel (which you cannot get booted per your other thread). The firmware are not compatible. :-(

If you're interested in trying it out, here's what I did:

diff --git a/src/pm_services/pm_state_data.c b/src/pm_services/pm_state_data.c
index c04f4d8..3b91f6a 100644
--- a/src/pm_services/pm_state_data.c
+++ b/src/pm_services/pm_state_data.c
@@ -31,10 +31,10 @@ union state_data ds0_data = {
.pd_mpu_l1_ret_state = MEM_BANK_RET_ST_OFF,
.pd_mpu_l2_ret_state = MEM_BANK_RET_ST_OFF,

- .pd_per_state = PD_RET,
+ .pd_per_state = PD_OFF,
.pd_per_icss_mem_ret_state = MEM_BANK_RET_ST_OFF,
.pd_per_mem_ret_state = MEM_BANK_RET_ST_OFF,
- .pd_per_ocmc_ret_state = MEM_BANK_RET_ST_RET,
+ .pd_per_ocmc_ret_state = MEM_BANK_RET_ST_OFF,
.pd_per_ocmc2_ret_state = MEM_BANK_RET_ST_OFF,

.wake_sources = WAKE_ALL,

EDIT: Don't bother with this for the reasons outlined in my next post!

Jonathan Cormier said:

I created some patches to enable sleep mode for the tps65910in the kernel. Does TI have a mailing list for their 3.14/4.1 kernel?

As a general matter new patches generally should be sent upstream and applied against mainline.  Though if you would like to make a quick and easy contribution you might start a brand new thread and just attach the patches, specifying what SDK they apply against.

Is that something you found reduced power even further?  My goal for the wiki page was to attach random patches that were known to improve power.  I haven't really gotten around to it yet though, so I have so far only attached the one patch I mentioned earlier for SDK 6.00.  Perhaps post them here or in a new thread and we can take it from there.

It's way too late on Friday so I'm rambling.  Time to get out of here!  :-)

Brad Griffis said:

Jonathan Cormier

I created some patches to enable sleep mode for the tps65910in the kernel. Does TI have a mailing list for their 3.14/4.1 kernel?

As a general matter new patches generally should be sent upstream and applied against mainline.  Though if you would like to make a quick and easy contribution you might start a brand new thread and just attach the patches, specifying what SDK they apply against.

Is that something you found reduced power even further?  My goal for the wiki page was to attach random patches that were known to improve power.  I haven't really gotten around to it yet though, so I have so far only attached the one patch I mentioned earlier for SDK 6.00.  Perhaps post them here or in a new thread and we can take it from there.

It's way too late on Friday so I'm rambling.  Time to get out of here!  :-)

The TPS65910 has a pin called SLEEP which when enabled and pulled low puts the PMIC into a lower powered state.  Since I'm measuring power usage at the input of the PMIC this means its power usage contributes to my readings.  I see ~40mW power savings by putting the PMIC into its sleep state.  On top of this the PMIC can be configured to power off power rails when it enters sleep. So I have it configured to disable VDIG1 which is wired to the VTT enable.

I was setting this all up using i2cget and i2cset.  So the patches allow this configuration to be done through device tree.  Most of the support was already there but had never been updated to allow access via device tree.

Attaching patches but if I get some chance I'll likely also submit to mailing lists.  TPS code hasn't changed much between 3.14 and 4.1 so should apply to both.

From dafd326f0b844454accf52fe85858dbb394d47e7 Mon Sep 17 00:00:00 2001
From: "Cormier, Jonathan" <jcormier@criticallink.com>
Date: Fri, 13 Nov 2015 15:51:47 -0500
Subject: [PATCH 1/3] mfd: tps65910: Add device tree support for SLEEP state

---
 Documentation/devicetree/bindings/mfd/tps65910.txt |  4 ++++
 drivers/mfd/tps65910.c                             | 22 ++++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/tps65910.txt b/Documentation/devicetree/bindings/mfd/tps65910.txt
index b4bd98af1cc7..5a80a90b7d7e 100644
--- a/Documentation/devicetree/bindings/mfd/tps65910.txt
+++ b/Documentation/devicetree/bindings/mfd/tps65910.txt
@@ -61,6 +61,10 @@ Optional properties:
   There should be 9 entries here, one for each gpio.
 - ti,system-power-controller: Telling whether or not this pmic is controlling
   the system power.
+- ti,en-dev-sleep: allows SLEEP device state. To enter SLEEP state this must be enabled, DEV_OFF and DEV_OFF_RST must be 0, interrupt registers must be cleared, and SLEEP pin must be active.
+- ti,sleep-therm-keepon: thermal monitoring is maintained during device SLEEP state
+- ti,sleep-clkout32k-keepon: CLK32KOUT output is maintained during device SLEEP state
+- ti,sleep-i2chs-keepon: high speed internal clock is maintained during device SLEEP state
 
 Regulator Optional properties:
 - ti,regulator-ext-sleep-control: enable external sleep
diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
index d6573318977f..05a6a62643ee 100644
--- a/drivers/mfd/tps65910.c
+++ b/drivers/mfd/tps65910.c
@@ -421,6 +421,28 @@ static struct tps65910_board *tps65910_parse_dt(struct i2c_client *client,
 	prop = of_property_read_bool(np, "ti,en-ck32k-xtal");
 	board_info->en_ck32k_xtal = prop;
 
+	prop = of_property_read_bool(np, "ti,en-dev-sleep");
+	board_info->en_dev_slp = prop;
+
+	if(board_info->en_dev_slp)
+	{
+		board_info->slp_keepon = devm_kzalloc(&client->dev, sizeof(*board_info->slp_keepon),
+				GFP_KERNEL);
+		if (!board_info->slp_keepon) {
+			dev_err(&client->dev, "Failed to allocate slp_keepon\n");
+			return NULL;
+		}
+
+		prop = of_property_read_bool(np, "ti,sleep-therm-keepon");
+		board_info->slp_keepon->therm_keepon = prop;
+
+		prop = of_property_read_bool(np, "ti,sleep-clkout32k-keepon");
+		board_info->slp_keepon->clkout32k_keepon = prop;
+
+		prop = of_property_read_bool(np, "ti,sleep-i2chs-keepon");
+		board_info->slp_keepon->i2chs_keepon = prop;
+	}
+
 	board_info->irq = client->irq;
 	board_info->irq_base = -1;
 	board_info->pm_off = of_property_read_bool(np,
-- 
1.9.1

From 2371ef9bbcffc6b2b27fdf754b55c8072a105a59 Mon Sep 17 00:00:00 2001
From: "Cormier, Jonathan" <jcormier@criticallink.com>
Date: Fri, 13 Nov 2015 15:39:12 -0500
Subject: [PATCH 2/3] mtd: tps65910: Add sleep mode fix to clear interrupts
 when irq not used.

The PMIC won't enter sleep mode if an interrupt is pending. In the case where
the interrupt isn't connected and thus isn't configured, the IRQ registration
will fail and the interrupt registers will not get acknowledged. The solution
is to clear the interrupts and then mask the interrupts.
---
 drivers/mfd/tps65910.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
index 05a6a62643ee..7bd041ba14d2 100644
--- a/drivers/mfd/tps65910.c
+++ b/drivers/mfd/tps65910.c
@@ -518,10 +518,42 @@ static int tps65910_i2c_probe(struct i2c_client *i2c,
 	init_data->irq = pmic_plat_data->irq;
 	init_data->irq_base = pmic_plat_data->irq_base;
 
-	tps65910_irq_init(tps65910, init_data->irq, init_data);
+	ret = tps65910_irq_init(tps65910, init_data->irq, init_data);
 	tps65910_ck32k_init(tps65910, pmic_plat_data);
 	tps65910_sleepinit(tps65910, pmic_plat_data);
 
+	/* If irqs aren't enabled and sleep mode is enabled we need to clear/mask interrupt registers */
+	/* Sleep mode won't be entered if an interrupt is pending */
+	if (ret < 0 && pmic_plat_data->en_dev_slp)
+		ret = tps65910_reg_set_bits(tps65910,
+				TPS65910_INT_STS,
+				0xFF); {
+		if (ret < 0) {
+			dev_err(&i2c->dev, "clear interrupt regs failed: %d\n", ret);
+		}
+
+		ret = tps65910_reg_set_bits(tps65910,
+				TPS65910_INT_MSK,
+				0xFF);
+		if (ret < 0) {
+			dev_err(&i2c->dev, "mask interrupt regs failed: %d\n", ret);
+		}
+
+		ret = tps65910_reg_set_bits(tps65910,
+				TPS65910_INT_STS2,
+				0xFF);
+		if (ret < 0) {
+			dev_err(&i2c->dev, "clear interrupt 2 regs failed: %d\n", ret);
+		}
+
+		ret = tps65910_reg_set_bits(tps65910,
+				TPS65910_INT_MSK2,
+				0xFF);
+		if (ret < 0) {
+			dev_err(&i2c->dev, "mask interrupt 2 regs failed: %d\n", ret);
+		}
+	}
+
 	if (pmic_plat_data->pm_off && !pm_power_off) {
 		tps65910_i2c_client = i2c;
 		pm_power_off = tps65910_power_off;
-- 
1.9.1

Apparently .patch isn't a accepted filetype...

Brad Griffis said:

FYI, I have spent many hours trying to make my scripts more robust. In particular, I was able to manage to put in some logic such that the nagging issue related to SDK 7.00 and later is ancient history. Specifically, I noticed that even with SDK 7.00 I was always able to connect to the M3 (and DAP_M3). And from the DAP_M3 I was able to adjust the register where the issue existed. So now my scripts always connect to the DAP_M3 first and then enable the DEBUGSS clock if it is disabled. I restore the state of the register at the end so as to not change the state of the system. Also, in the case where the EMIF is disabled (e.g. when you're nearly in DS0), I have a more graceful message that prints out. So please download the updated scripts!

Cool.  Any idea if there is any benefit to leaving the DEBUGSS clock disabled.  The patch seemed to indicate that having the DEBUGSS clock disabled was a invalid state.

Brad Griffis said:

As a next step, if you're able to get any further refinements with regard to your power consumption, that would still likely help further track down where you're burning power. I think I'm about at the end of what I can do over a forum. I have very much appreciated your feedback regarding the scripts, the wiki page, etc.

Yes thanks for all the help.  We are currently within our power budget and are planning on putting the power investigation on hold until we get the custom baseboard for this project.

Jonathan Cormier said:

Cool.  Any idea if there is any benefit to leaving the DEBUGSS clock disabled.  The patch seemed to indicate that having the DEBUGSS clock disabled was a invalid state.

Seems like you have a pretty good power measurement setup. (Better than mine!)  A simple test would be:

1. Take a measurement.
2. Execute "devmem2 0x44e00414 w 0x12500002" and then take another measurement.

Jonathan Cormier said:

We are currently within our power budget and are planning on putting the power investigation on hold until we get the custom baseboard for this project.

Ok, thanks for the update.

Jonathan Cormier said:

The TPS65910 has a pin called SLEEP which when enabled and pulled low puts the PMIC into a lower powered state.  Since I'm measuring power usage at the input of the PMIC this means its power usage contributes to my readings.  I see ~40mW power savings by putting the PMIC into its sleep state.  On top of this the PMIC can be configured to power off power rails when it enters sleep. So I have it configured to disable VDIG1 which is wired to the VTT enable.

This is a great data point.  That's a huge reduction.  Are you sure that all the necessary rails are still up?

Brad Griffis said:

Jonathan Cormier

Cool.  Any idea if there is any benefit to leaving the DEBUGSS clock disabled.  The patch seemed to indicate that having the DEBUGSS clock disabled was a invalid state.

Seems like you have a pretty good power measurement setup. (Better than mine!)  A simple test would be:

1. Take a measurement.
2. Execute "devmem2 0x44e00414 w 0x12500002" and then take another measurement.

Could not detect any difference in power usage during idle or mem sleep when this clock is enabled or disabled.

Brad Griffis said:

This is a great data point.  That's a huge reduction.  Are you sure that all the necessary rails are still up?

Yeah I believe so.  By default the SLEEP mode doesn't turn off any rails instead switches them to a low power state via some hardware magic :P.  There are fields that can be set to keep certain rails in high power mode and fields to disable rails entirely. Its a shame there’s no field to reduce the core voltage when in sleep but you can't have everything :P.

Brad Griffis said:

Make sure you're also setting DCDCCTRL_REG [1:0] = 00 in the TPS65910A per its errata. It's not something you see all the time, but once you get a large number of units doing a large number of suspend/resume sequences you'll start seeing a very small number of occurrences (IF you don't implement the simple workaround!).

Thanks I didn't understand the relevance of the errata when I last read it however long ago.  Created a ticket to update driver when I get back to sleep mode stuff.

Brad Griffis said:

Make sure you're also setting DCDCCTRL_REG [1:0] = 00 in the TPS65910A per its errata. It's not something you see all the time, but once you get a large number of units doing a large number of suspend/resume sequences you'll start seeing a very small number of occurrences (IF you don't implement the simple workaround!).

What are the effects of setting these bits to 0?  If I create a kernel patch for this errata would it only apply if someone enables the SLEEP pin or could they always be set to 0?

Jonathan Cormier said:

One note, is that this process would have been a quicker if the PadConf spreadsheet had been either editable or copyable.  If I had been able to write the voltages directly into the spreadsheet in column U it would make it easier to compare. 

FYI, we updated the spreadsheet yesterday to allow you to enter your measured voltages directly.  Thanks for that feedback.