mfd: twl4030-power: Fix PM idle pin configuration to not conflict with regulators

PATCH New Tony Lindgren 5 participants 9 messages Aug 19, 2014 – Sep 4, 2014

Subsystems (3)

Device tree bindings Multifunction devices (MFD) OMAP2+ support

lore

Share & tools

lore

Tony Lindgren

author maintainer

2014-08-19 15:24 UTC #0 lore raw

PATCH 1/1 mfd: twl4030-power: Fix PM idle pin configuration to not conflict with regulators

Commit 43fef47f94a1 (mfd: twl4030-power: Add a configuration to turn off oscillator during off-idle) added support for configuring the PMIC to cut off resources during deeper idle states to save power.

This however caused regression for n900 display power that needed the PMIC configuration to be disabled with commit d937678ab625 (ARM: dts:
Revert enabling of twl configuration for n900).

Turns out the root cause of the problem is that we must use TWL4030_RESCONFIG_UNDEF instead of DEV_GRP_NULL to avoid disabling regulators that may have been enabled before the init function for twl4030-power.c runs. With TWL4030_RESCONFIG_UNDEF we let the regulator framework control the regulators like it should. Here we need to only configure the sys_clken and sys_off_mode triggers for the regulators that cannot be done by the regulator framework as it's not running at that point.

This allows us to enable the PMIC configuration for n900.

Fixes 43fef47f94a1 (mfd: twl4030-power: Add a configuration to turn off oscillator during off-idle)

Cc stable at vger.kernel.org # v3.16

Signed-off-by Tony Lindgren <[email protected]>

---

Lee, can you please pick this one for the v3.17-rc series?

arch/arm/boot/dts/omap3-n900.dts +1 -1

@@ -361,7 +361,7 @@
 	};
 
 	twl_power: power {
-		compatible = "ti,twl4030-power-n900";
+		compatible = "ti,twl4030-power-n900", "ti,twl4030-power-idle-osc-off";
 		ti,use_poweroff;
 	};
 };

drivers/mfd/twl4030-power.c +10 ~~-10~~

@@ -724,24 +724,24 @@ static struct twl4030_script *omap3_idle_scripts[] = {
  * above.
  */
 static struct twl4030_resconfig omap3_idle_rconfig[] = {
-	TWL_REMAP_SLEEP(RES_VAUX1, DEV_GRP_NULL, 0, 0),
-	TWL_REMAP_SLEEP(RES_VAUX2, DEV_GRP_NULL, 0, 0),
-	TWL_REMAP_SLEEP(RES_VAUX3, DEV_GRP_NULL, 0, 0),
-	TWL_REMAP_SLEEP(RES_VAUX4, DEV_GRP_NULL, 0, 0),
-	TWL_REMAP_SLEEP(RES_VMMC1, DEV_GRP_NULL, 0, 0),
-	TWL_REMAP_SLEEP(RES_VMMC2, DEV_GRP_NULL, 0, 0),
+	TWL_REMAP_SLEEP(RES_VAUX1, TWL4030_RESCONFIG_UNDEF, 0, 0),
+	TWL_REMAP_SLEEP(RES_VAUX2, TWL4030_RESCONFIG_UNDEF, 0, 0),
+	TWL_REMAP_SLEEP(RES_VAUX3, TWL4030_RESCONFIG_UNDEF, 0, 0),
+	TWL_REMAP_SLEEP(RES_VAUX4, TWL4030_RESCONFIG_UNDEF, 0, 0),
+	TWL_REMAP_SLEEP(RES_VMMC1, TWL4030_RESCONFIG_UNDEF, 0, 0),
+	TWL_REMAP_SLEEP(RES_VMMC2, TWL4030_RESCONFIG_UNDEF, 0, 0),
 	TWL_REMAP_OFF(RES_VPLL1, DEV_GRP_P1, 3, 1),
 	TWL_REMAP_SLEEP(RES_VPLL2, DEV_GRP_P1, 0, 0),
-	TWL_REMAP_SLEEP(RES_VSIM, DEV_GRP_NULL, 0, 0),
-	TWL_REMAP_SLEEP(RES_VDAC, DEV_GRP_NULL, 0, 0),
+	TWL_REMAP_SLEEP(RES_VSIM, TWL4030_RESCONFIG_UNDEF, 0, 0),
+	TWL_REMAP_SLEEP(RES_VDAC, TWL4030_RESCONFIG_UNDEF, 0, 0),
 	TWL_REMAP_SLEEP(RES_VINTANA1, TWL_DEV_GRP_P123, 1, 2),
 	TWL_REMAP_SLEEP(RES_VINTANA2, TWL_DEV_GRP_P123, 0, 2),
 	TWL_REMAP_SLEEP(RES_VINTDIG, TWL_DEV_GRP_P123, 1, 2),
 	TWL_REMAP_SLEEP(RES_VIO, TWL_DEV_GRP_P123, 2, 2),
 	TWL_REMAP_OFF(RES_VDD1, DEV_GRP_P1, 4, 1),
 	TWL_REMAP_OFF(RES_VDD2, DEV_GRP_P1, 3, 1),
-	TWL_REMAP_SLEEP(RES_VUSB_1V5, DEV_GRP_NULL, 0, 0),
-	TWL_REMAP_SLEEP(RES_VUSB_1V8, DEV_GRP_NULL, 0, 0),
+	TWL_REMAP_SLEEP(RES_VUSB_1V5, TWL4030_RESCONFIG_UNDEF, 0, 0),
+	TWL_REMAP_SLEEP(RES_VUSB_1V8, TWL4030_RESCONFIG_UNDEF, 0, 0),
 	TWL_REMAP_SLEEP(RES_VUSB_3V1, TWL_DEV_GRP_P123, 0, 0),
 	/* Resource #20 USB charge pump skipped */
 	TWL_REMAP_SLEEP(RES_REGEN, TWL_DEV_GRP_P123, 2, 1),

Aaro Koskinen

maintainer

2014-08-19 21:33 UTC #1 lore raw

PATCH 1/1 mfd: twl4030-power: Fix PM idle pin configuration to not conflict with regulators

Hi,

Quoted 3 lines · Tony Lindgren

Commit 43fef47f94a1 (mfd: twl4030-power: Add a configuration to turn off oscillator during off-idle) added support for configuring the PMIC to cut off resources during deeper idle states to save power.

[...]

Quoted 3 lines

Fixes 43fef47f94a1 (mfd: twl4030-power: Add a configuration to turn off oscillator during off-idle)
Cc stable at vger.kernel.org # v3.16
Signed-off-by Tony Lindgren <[email protected]>

Tested-by Aaro Koskinen <[email protected]>

Lee Jones

2014-08-20 12:30 UTC #2 lore raw

PATCH 1/1 mfd: twl4030-power: Fix PM idle pin configuration to not conflict with regulators

In reply to Tony Lindgren

Quoted 19 lines · Tony Lindgren

Commit 43fef47f94a1 (mfd: twl4030-power: Add a configuration to turn off oscillator during off-idle) added support for configuring the PMIC to cut off resources during deeper idle states to save power.
This however caused regression for n900 display power that needed the PMIC configuration to be disabled with commit d937678ab625 (ARM: dts:
Revert enabling of twl configuration for n900).
Turns out the root cause of the problem is that we must use TWL4030_RESCONFIG_UNDEF instead of DEV_GRP_NULL to avoid disabling regulators that may have been enabled before the init function for twl4030-power.c runs. With TWL4030_RESCONFIG_UNDEF we let the regulator framework control the regulators like it should. Here we need to only configure the sys_clken and sys_off_mode triggers for the regulators that cannot be done by the regulator framework as it's not running at that point.
This allows us to enable the PMIC configuration for n900.

Quoted 7 lines

Fixes 43fef47f94a1 (mfd: twl4030-power: Add a configuration to turn off oscillator during off-idle)
Cc stable at vger.kernel.org # v3.16
Signed-off-by Tony Lindgren <[email protected]>
---
Lee, can you please pick this one for the v3.17-rc series?

Applied with Aaro's Tested-by.

Signature · 4 lines

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

Sebastian Andrzej Siewior

2014-09-02 08:29 UTC #3 lore raw

PATCH 1/1 mfd: twl4030-power: Fix PM idle pin configuration to not conflict with regulators

In reply to Tony Lindgren

Quoted 4 lines · Tony Lindgren

This allows us to enable the PMIC configuration for n900.
Fixes 43fef47f94a1 (mfd: twl4030-power: Add a configuration to turn off oscillator during off-idle)

My beaglebone-ab does not like this. With this patch applied I end up with:

[    2.437316] Waiting for root device /dev/mmcblk0p2...
[    4.428192] mmc0: card never left busy state
[    4.432647] mmc0: error -110 whilst initialising SD card

I noticed this after I disabled lock debugging and tracing (syscall tracing was enabled). Enabling both again makes the error go away.
With debugging + tracing disabled (so the error pops up) and git bisect I get this commit (daebabd57) reported.
A diff between a good/bad bootlog (with lock-debug + tracing switched off):

| smartreflex smartreflex.0: omap_sr_probe: SmartReflex driver initialized
| smartreflex smartreflex.1: omap_sr_probe: SmartReflex driver initialized
| hsusb2_vbus: disabling
| VPLL2: disabling
| VUSB3V1: disabling
|+VDAC: disabling
|+VAUX3: disabling
| Waiting for root device /dev/mmcblk0p2...
|-mmc0: host does not support reading read-only switch. assuming write-enable.
|-mmc0: new high speed SDHC card at address 1234
|-mmcblk0: mmc0:1234 SA04G 3.63 GiB 
|- mmcblk0: p1 p2 p3
|-BTRFS: device fsid 2e050bbb-1520-425e-a215-1e5bf865c7dc devid 1 transid 496 /dev/root
|-BTRFS info (device mmcblk0p2): disk space caching is enabled
|-BTRFS: detected SSD devices, enabling SSD mode
|-VFS: Mounted root (btrfs filesystem) readonly on device 0:13.
|-devtmpfs: mounted
|-Freeing unused kernel memory: 216K (c051a000 - c0550000)
|-BTRFS info (device mmcblk0p2): disk space caching is enabled
|-
|-Please press Enter to activate this console. 
|+mmc0: card never left busy state
|+mmc0: error -110 whilst initialising SD card

reverting this commit on top of -rc3 makes mmc0 work again.

Sebastian

Tony Lindgren

author maintainer

2014-09-03 00:24 UTC #4 lore raw

PATCH 1/1 mfd: twl4030-power: Fix PM idle pin configuration to not conflict with regulators

Quoted 11 lines · Sebastian Andrzej Siewior

Quoted 4 lines · Tony Lindgren
This allows us to enable the PMIC configuration for n900.
Fixes 43fef47f94a1 (mfd: twl4030-power: Add a configuration to turn off oscillator during off-idle)
My beaglebone-ab does not like this. With this patch applied I end up with:
[    2.437316] Waiting for root device /dev/mmcblk0p2...
[    4.428192] mmc0: card never left busy state
[    4.432647] mmc0: error -110 whilst initialising SD card

I assume you mean beagleboard-ab, not beaglebone-ab :)

Quoted 4 lines

I noticed this after I disabled lock debugging and tracing (syscall tracing was enabled). Enabling both again makes the error go away.
With debugging + tracing disabled (so the error pops up) and git bisect I get this commit (daebabd57) reported.

OK. Sounds like we still have a race between the regulator code and twl4030-power.c for accessing some twl registers.

Quoted 10 lines

A diff between a good/bad bootlog (with lock-debug + tracing switched off):

| smartreflex smartreflex.0: omap_sr_probe: SmartReflex driver initialized
| smartreflex smartreflex.1: omap_sr_probe: SmartReflex driver initialized
| hsusb2_vbus: disabling
| VPLL2: disabling
| VUSB3V1: disabling
|+VDAC: disabling
|+VAUX3: disabling

This seems to be the reason. Seems you're on v3.17-rc3, but with commit daebabd57 we are not even touching the group registers that twl-regulator.c accesses for VDAC and VAUX3 as they are set to TWL4030_RESCONFIG_UNDEF. So I'm a bit baffled what's going on.

Quoted 1 line

reverting this commit on top of -rc3 makes mmc0 work again.

Again, I assume you're talking about reverting daebabd57, not 43fef47f94a1. Anyways, here's a debug hack I used earlier to dump out the twl configuration in late_initcall and via sysfs so maybe try that and see what the values are with working and non-working case?

Regards,

Tony

drivers/mfd/twl4030-power.c +142 -0

@@ -358,6 +358,146 @@ out:
 	return err;
 }
 
+#ifdef CONFIG_DEBUG_FS
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+
+static void twl4030_dump_resource(unsigned index, struct seq_file *s)
+{
+	int addr;
+	int err;
+	u8 type;
+	u8 grp;
+	u8 remap;
+
+	addr = res_config_addrs[index];
+
+	err = twl_i2c_read_u8(TWL_MODULE_PM_RECEIVER, &grp,
+			      addr + DEV_GRP_OFFSET);
+	if (err) {
+		pr_err("TWL4030 Resource group 0x%02x could not be read\n",
+			addr);
+		return;
+	}
+
+	err = twl_i2c_read_u8(TWL_MODULE_PM_RECEIVER, &type,
+				addr + TYPE_OFFSET);
+	if (err < 0) {
+		pr_err("TWL4030 Resource type 0x%02x could not be read\n",
+			addr);
+		return;
+	}
+
+	err = twl_i2c_read_u8(TWL_MODULE_PM_RECEIVER, &remap,
+			      addr + REMAP_OFFSET);
+	if (err < 0) {
+		pr_err("TWL4030 Resource 0x%02x remap could not be read\n",
+			addr);
+		return;
+	}
+
+	if (s)
+		seq_printf(s, "%i: addr: 0x%04x grp: 0x%04x type: 0x%04x remap: 0x%04x\n",
+			   index, addr, grp, type, remap);
+	else
+		printk("%i: addr: 0x%04x grp: 0x%04x type: 0x%04x remap: 0x%04x\n",
+		       index, addr, grp, type, remap);
+}
+
+static void twl4030_dump_resources(void)
+{
+	int i;
+
+	for (i = 1; i <= RES_MAIN_REF; i++)
+		twl4030_dump_resource(i, NULL);
+}
+
+static struct dentry *dbg_root;
+
+static int dbg_show(struct seq_file *s, void *unused)
+{
+	unsigned long offset = (unsigned long)s->private;
+
+	twl4030_dump_resource(offset, s);
+
+	return 0;
+}
+
+static ssize_t dbg_write(struct file *file,
+			 const char __user *user_buf,
+			 size_t count, loff_t *ppos)
+{
+	struct seq_file *seqf;
+	unsigned long offset;
+	u8 val;
+	int res;
+
+	res = kstrtou8_from_user(user_buf, count, 0x10, &val);
+        if (res < 0)
+                return res;
+
+	seqf = file->private_data;
+	offset = (unsigned long)seqf->private;
+	offset = res_config_addrs[offset];
+
+	res = twl_i2c_write_u8(TWL_MODULE_PM_RECEIVER,
+			       val, offset + DEV_GRP_OFFSET);
+	if (res < 0) {
+		pr_err("TWL4030 failed to program devgroup\n");
+		return res;
+	}
+
+	*ppos += count;
+
+	return count;
+}
+
+static int dbg_open(struct inode *inode, struct file *file)
+{
+        return single_open(file, dbg_show, inode->i_private);
+}
+
+static const struct file_operations dbg_fops = {
+        .open           = dbg_open,
+        .read           = seq_read,
+        .write          = dbg_write,
+        .llseek         = seq_lseek,
+        .release        = single_release,
+};
+
+static void __init dbg_init(struct twl4030_resconfig *rconfig)
+{
+	int i;
+
+	dbg_root = debugfs_create_dir("twl4030-power", NULL);
+	if (IS_ERR(dbg_root) || !dbg_root)
+		return;
+
+	for (i = 1; i <= RES_MAIN_REF; i++) {
+		u8 name[16];
+
+		sprintf(name, "0x%02x", res_config_addrs[i]);
+		(void)debugfs_create_file(name, S_IRUGO,
+					  dbg_root,
+					  (void *)i,
+					  &dbg_fops);
+	}
+}
+
+static int __init dbg_initcall(void)
+{
+	twl4030_dump_resources();
+
+	return 0;
+}
+late_initcall(dbg_initcall);
+
+#else
+static inline void dbg_init(struct twl4030_resconfig *rconfig)
+{
+}
+#endif
+
 static int twl4030_configure_resource(struct twl4030_resconfig *rconfig)
 {
 	int rconfig_addr;

@@ -864,6 +1004,8 @@ relock:
 		return err2;
 	}
 
+	dbg_init(pdata->resource_config);
+
 	return err;
 }

Sebastian Andrzej Siewior

2014-09-03 08:38 UTC #5 lore raw

PATCH 1/1 mfd: twl4030-power: Fix PM idle pin configuration to not conflict with regulators

Quoted 1 line · Tony Lindgren

I assume you mean beagleboard-ab, not beaglebone-ab :)

you assume correct.

Quoted 4 lines

Quoted 1 line
reverting this commit on top of -rc3 makes mmc0 work again.
Again, I assume you're talking about reverting daebabd57, not 43fef47f94a1. Anyways, here's a debug hack I used earlier to

Yes, I reverted daebabd57 which was pointed to me by git bisect.

Quoted 3 lines

dump out the twl configuration in late_initcall and via sysfs so maybe try that and see what the values are with working and non-working case?

good.txt and bad.txt are from the late_initcall.

 $ diff -u good.txt bad.txt
--- good.txt    2014-09-03 10:29:58.920317368 +0200
+++ bad.txt     2014-09-03 10:28:57.064313222 +0200
@@ -1,13 +1,13 @@
  1: addr: 0x0017 grp: 0x0000 type: 0x0000 remap: 0x0008
- 2: addr: 0x001b grp: 0x0000 type: 0x0000 remap: 0x0008
- 3: addr: 0x001f grp: 0x0000 type: 0x0000 remap: 0x0008
+ 2: addr: 0x001b grp: 0x002e type: 0x0000 remap: 0x0008
+ 3: addr: 0x001f grp: 0x002e type: 0x0000 remap: 0x0008
  4: addr: 0x0023 grp: 0x0000 type: 0x0000 remap: 0x0008
  5: addr: 0x0027 grp: 0x002e type: 0x0000 remap: 0x0008
  6: addr: 0x002b grp: 0x0000 type: 0x0000 remap: 0x0008
  7: addr: 0x002f grp: 0x002e type: 0x000b remap: 0x0000
  8: addr: 0x0033 grp: 0x002e type: 0x0000 remap: 0x0008
  9: addr: 0x0037 grp: 0x002e type: 0x0000 remap: 0x0008
-10: addr: 0x003b grp: 0x0000 type: 0x0000 remap: 0x0008
+10: addr: 0x003b grp: 0x002e type: 0x0000 remap: 0x0008
 11: addr: 0x003f grp: 0x00ef type: 0x0011 remap: 0x0008
 12: addr: 0x0043 grp: 0x00ef type: 0x0010 remap: 0x0008
 13: addr: 0x0047 grp: 0x00ef type: 0x0011 remap: 0x0008
@@ -17,12 +17,8 @@
 17: addr: 0x0071 grp: 0x0000 type: 0x0000 remap: 0x0008
 18: addr: 0x0074 grp: 0x0000 type: 0x0000 remap: 0x0008
 19: addr: 0x0077 grp: 0x00ef type: 0x0000 remap: 0x0008
-mmc0: host does not support reading read-only switch. assuming
write-enable.
 20: addr: 0x007a grp: 0x0000 type: 0x0000 remap: 0x0000
-mmc0: new high speed SDHC card at address 1234
-mmcblk0: mmc0:1234 SA04G 3.63 GiB
 21: addr: 0x007f grp: 0x00ef type: 0x000a remap: 0x0008
- mmcblk0: p1 p2 p3
 22: addr: 0x0082 grp: 0x00ee type: 0x0008 remap: 0x0008
 23: addr: 0x0085 grp: 0x00af type: 0x0013 remap: 0x0000
 24: addr: 0x0088 grp: 0x00ef type: 0x000e remap: 0x0008

Quoted 4 lines

Regards,
Tony

Sebastian

Tony Lindgren

author maintainer

2014-09-03 18:39 UTC #6 lore raw

PATCH 1/1 mfd: twl4030-power: Fix PM idle pin configuration to not conflict with regulators

bad.txt 2014-09-03 10:28:57.064313222 +0200

Quoted 11 lines · Sebastian Andrzej Siewior

Quoted 3 lines · Tony Lindgren
dump out the twl configuration in late_initcall and via sysfs so maybe try that and see what the values are with working and non-working case?
good.txt and bad.txt are from the late_initcall.
 $ diff -u good.txt bad.txt
bad.txt 2014-09-03 10:28:57.064313222 +0200 renamed from good.txt 2014-09-03 10:29:58.920317368 +0200 +0 -0

Hmm can you check that you have good.txt and bad.txt the right way? I'd assume you need VAUX2 or VAUX3 enabled not disabled for the MMC to work?

bad.txt 2014-09-03 10:28:57.064313222 +0200 line 3

Quoted 6 lines

bad.txt 2014-09-03 10:28:57.064313222 +0200 renamed from good.txt 2014-09-03 10:29:58.920317368 +0200 +2 -2

@@ -1,13 +1,13 @@
  1: addr: 0x0017 grp: 0x0000 type: 0x0000 remap: 0x0008
- 2: addr: 0x001b grp: 0x0000 type: 0x0000 remap: 0x0008
- 3: addr: 0x001f grp: 0x0000 type: 0x0000 remap: 0x0008
+ 2: addr: 0x001b grp: 0x002e type: 0x0000 remap: 0x0008
+ 3: addr: 0x001f grp: 0x002e type: 0x0000 remap: 0x0008

OK so looking at res_config_addrs[], we have RES_VAUX2 at 0x1b and RES_VAUX3 at 0x1f. The value for the group register 0x2e means that bit5 is set and it's used by the p1 device group. So when the group register is 0x2e, the resource is enabled. Those got most likely enabled by twl-regulator.c as twl4030-power.c has TWL4030_RESCONFIG_UNDEF for VAUX2 and VAUX3.

bad.txt 2014-09-03 10:28:57.064313222 +0200 line 10

Quoted 8 lines

bad.txt 2014-09-03 10:28:57.064313222 +0200 renamed from good.txt 2014-09-03 10:29:58.920317368 +0200 +1 -1

  4: addr: 0x0023 grp: 0x0000 type: 0x0000 remap: 0x0008
  5: addr: 0x0027 grp: 0x002e type: 0x0000 remap: 0x0008
  6: addr: 0x002b grp: 0x0000 type: 0x0000 remap: 0x0008
  7: addr: 0x002f grp: 0x002e type: 0x000b remap: 0x0000
  8: addr: 0x0033 grp: 0x002e type: 0x0000 remap: 0x0008
  9: addr: 0x0037 grp: 0x002e type: 0x0000 remap: 0x0008
-10: addr: 0x003b grp: 0x0000 type: 0x0000 remap: 0x0008
+10: addr: 0x003b grp: 0x002e type: 0x0000 remap: 0x0008

And that's RES_VDAC at 0x3b with the same story, it's also marked TWL4030_RESCONFIG_UNDEF and only modified by the twl4030-power.c.

I think the use on beaglboard for these are:

VAUX2 USB_1V8
VAUX3 CAM_CORE
VDAC VDAC_1V8

Not quite seeing the connection.. But I'm assuming you have good.txt and bad.txt the wrong way around, and you need VAUX2, VAUX3 or VDAC _enabled_ to get MMC working :)

So this seems to hint we have issue in one of these:

1. We are not claiming VAUX2, VAUX3 or VDAC for beagleboard, and there's now a timing issue where the regulator framework disables the unused regulators before MMC.

2. We may have a bug that causes register access to DEV_GRP_OFFSET in twl4030-power.c even with those resources marked as TWL4030_RESCONFIG_UNDEF.

3. There's a I2C race somewhere accessing twl registers.

Guessing #1 above, maybe give the following patch a try and see if that helps? If that works, try commenting out vaux3 or vdac and see which one is needed. I'm guessing it's the vdac.

Regards,

Tony

arch/arm/boot/dts/omap3-beagle-xm.dts +12 -0

@@ -331,6 +331,18 @@
 	regulator-always-on;
 };
 
+&vaux3 {
+	regulator-name = "cam_core";
+	regulator-min-microvolt = <1800000>;
+	regulator-max-microvolt = <1800000>;
+	regulator-always-on;
+};
+
+&vdac {
+	regulator-name = "vdac_1v8";
+	regulator-always-on;
+};
+
 &mcbsp2 {
 	status = "okay";
 };

Sebastian Andrzej Siewior

2014-09-04 08:45 UTC #7 lore raw

PATCH 1/1 mfd: twl4030-power: Fix PM idle pin configuration to not conflict with regulators

Quoted 9 lines · Tony Lindgren

bad.txt 2014-09-03 10:28:57.064313222 +0200
Quoted 5 lines
good.txt and bad.txt are from the late_initcall.
 $ diff -u good.txt bad.txt
bad.txt 2014-09-03 10:28:57.064313222 +0200 renamed from good.txt 2014-09-03 10:29:58.920317368 +0200 +0 -0
Hmm can you check that you have good.txt and bad.txt the right way? I'd assume you need VAUX2 or VAUX3 enabled not disabled for the MMC to work?

No, it was correct. If you look at the complete file you will notice the - which removes the mmc detect/mount in the bad case and + which adds the -110 error

Quoted 48 lines

bad.txt 2014-09-03 10:28:57.064313222 +0200 line 3
Quoted 6 lines
bad.txt 2014-09-03 10:28:57.064313222 +0200 renamed from good.txt 2014-09-03 10:29:58.920317368 +0200 +2 -2
@@ -1,13 +1,13 @@
  1: addr: 0x0017 grp: 0x0000 type: 0x0000 remap: 0x0008
- 2: addr: 0x001b grp: 0x0000 type: 0x0000 remap: 0x0008
- 3: addr: 0x001f grp: 0x0000 type: 0x0000 remap: 0x0008
+ 2: addr: 0x001b grp: 0x002e type: 0x0000 remap: 0x0008
+ 3: addr: 0x001f grp: 0x002e type: 0x0000 remap: 0x0008
OK so looking at res_config_addrs[], we have RES_VAUX2 at 0x1b and RES_VAUX3 at 0x1f. The value for the group register 0x2e means that bit5 is set and it's used by the p1 device group. So when the group register is 0x2e, the resource is enabled. Those got most likely enabled by twl-regulator.c as twl4030-power.c has TWL4030_RESCONFIG_UNDEF for VAUX2 and VAUX3.
bad.txt 2014-09-03 10:28:57.064313222 +0200 line 10
Quoted 8 lines
bad.txt 2014-09-03 10:28:57.064313222 +0200 renamed from good.txt 2014-09-03 10:29:58.920317368 +0200 +1 -1
  4: addr: 0x0023 grp: 0x0000 type: 0x0000 remap: 0x0008
  5: addr: 0x0027 grp: 0x002e type: 0x0000 remap: 0x0008
  6: addr: 0x002b grp: 0x0000 type: 0x0000 remap: 0x0008
  7: addr: 0x002f grp: 0x002e type: 0x000b remap: 0x0000
  8: addr: 0x0033 grp: 0x002e type: 0x0000 remap: 0x0008
  9: addr: 0x0037 grp: 0x002e type: 0x0000 remap: 0x0008
-10: addr: 0x003b grp: 0x0000 type: 0x0000 remap: 0x0008
+10: addr: 0x003b grp: 0x002e type: 0x0000 remap: 0x0008
And that's RES_VDAC at 0x3b with the same story, it's also marked TWL4030_RESCONFIG_UNDEF and only modified by the twl4030-power.c.
I think the use on beaglboard for these are:
VAUX2 USB_1V8
VAUX3 CAM_CORE
VDAC VDAC_1V8
Not quite seeing the connection.. But I'm assuming you have good.txt and bad.txt the wrong way around, and you need VAUX2, VAUX3 or VDAC _enabled_ to get MMC working :)
So this seems to hint we have issue in one of these:
1. We are not claiming VAUX2, VAUX3 or VDAC for beagleboard, and there's now a timing issue where the regulator framework disables the unused regulators before MMC.
2. We may have a bug that causes register access to DEV_GRP_OFFSET in twl4030-power.c even with those resources marked as TWL4030_RESCONFIG_UNDEF.

Quoted 6 lines

3. There's a I2C race somewhere accessing twl registers.
Guessing #1 above, maybe give the following patch a try and see if that helps? If that works, try commenting out vaux3 or vdac and see which one is needed. I'm guessing it's the vdac.

With that patch it seems it is a little harder to trigger. It is usually every other boot that fails. Here a diff between two that worked (say good-v1 vs good-v2):

@@ -151,19 +151,15 @@
Btrfs loaded
hsusb2_vbus: 3300 mV
1: addr: 0x0017 grp: 0x0000 type: 0x0000 remap: 0x0008
-2: addr: 0x001b grp: 0x002e type: 0x0000 remap: 0x0008 -3: addr: 0x001f grp: 0x002e type: 0x0000 remap: 0x0008 +2: addr: 0x001b grp: 0x0000 type: 0x0000 remap: 0x0008 +3: addr: 0x001f grp: 0x0000 type: 0x0000 remap: 0x0008
4: addr: 0x0023 grp: 0x0000 type: 0x0000 remap: 0x0008 5: addr: 0x0027 grp: 0x002e type: 0x0000 remap: 0x0008 6: addr: 0x002b grp: 0x0000 type: 0x0000 remap: 0x0008 7: addr: 0x002f grp: 0x002e type: 0x000b remap: 0x0000 8: addr: 0x0033 grp: 0x002e type: 0x0000 remap: 0x0008
-mmc0: host does not support reading read-only switch. assuming write-enable.
9: addr: 0x0037 grp: 0x002e type: 0x0000 remap: 0x0008
-mmc0: new high speed SDHC card at address 1234 -mmcblk0: mmc0:1234 SA04G 3.63 GiB
-10: addr: 0x003b grp: 0x002e type: 0x0000 remap: 0x0008
- mmcblk0: p1 p2 p3
+10: addr: 0x003b grp: 0x0000 type: 0x0000 remap: 0x0008
11: addr: 0x003f grp: 0x00ef type: 0x0011 remap: 0x0008 12: addr: 0x0043 grp: 0x00ef type: 0x0010 remap: 0x0008 13: addr: 0x0047 grp: 0x00ef type: 0x0011 remap: 0x0008
@@ -173,8 +169,12 @@
17: addr: 0x0071 grp: 0x0000 type: 0x0000 remap: 0x0008 18: addr: 0x0074 grp: 0x0000 type: 0x0000 remap: 0x0008 19: addr: 0x0077 grp: 0x00ef type: 0x0000 remap: 0x0008
+mmc0: host does not support reading read-only switch. assuming write-enable.
20: addr: 0x007a grp: 0x0000 type: 0x0000 remap: 0x0000
+mmc0: new high speed SDHC card at address 1234 +mmcblk0: mmc0:1234 SA04G 3.63 GiB
21: addr: 0x007f grp: 0x00ef type: 0x000a remap: 0x0008
+ mmcblk0: p1 p2 p3
22: addr: 0x0082 grp: 0x00ee type: 0x0008 remap: 0x0008 23: addr: 0x0085 grp: 0x00af type: 0x0013 remap: 0x0000 24: addr: 0x0088 grp: 0x00ef type: 0x000e remap: 0x0008

It took mmc a little longer to detect but it worked. And the content of the three registers seem not to matter _or_ it was dumped before MMC got active.

Now a diff of good v1 vs bad:

@@ -158,12 +158,8 @@
6: addr: 0x002b grp: 0x0000 type: 0x0000 remap: 0x0008
7: addr: 0x002f grp: 0x002e type: 0x000b remap: 0x0000
8: addr: 0x0033 grp: 0x002e type: 0x0000 remap: 0x0008
-mmc0: host does not support reading read-only switch. assuming write-enable.
9: addr: 0x0037 grp: 0x002e type: 0x0000 remap: 0x0008
-mmc0: new high speed SDHC card at address 1234
-mmcblk0: mmc0:1234 SA04G 3.63 GiB
10: addr: 0x003b grp: 0x002e type: 0x0000 remap: 0x0008
- mmcblk0: p1 p2 p3
11: addr: 0x003f grp: 0x00ef type: 0x0011 remap: 0x0008
12: addr: 0x0043 grp: 0x00ef type: 0x0010 remap: 0x0008
13: addr: 0x0047 grp: 0x00ef type: 0x0011 remap: 0x0008
@@ -183,19 +179,13 @@
27: addr: 0x0091 grp: 0x00ee type: 0x0006 remap: 0x0008
28: addr: 0x0094 grp: 0x00ef type: 0x0000 remap: 0x0008
input: gpio_keys as /devices/gpio_keys/input/input2
-twl_rtc 48070000.i2c:twl at 48:rtc: setting system clock to 2000-01-01 00:02:39 UTC (946684959)
+twl_rtc 48070000.i2c:twl at 48:rtc: setting system clock to 2000-01-01 00:02:47 UTC (946684967)
sr_init: No PMIC hook to init smartreflex
smartreflex smartreflex.0: omap_sr_probe: SmartReflex driver initialized smartreflex smartreflex.1: omap_sr_probe: SmartReflex driver initialized
hsusb2_vbus: disabling
VPLL2: disabling
VUSB3V1: disabling
-BTRFS: device fsid 2e050bbb-1520-425e-a215-1e5bf865c7dc devid 1 transid 1189 /dev/root
-BTRFS info (device mmcblk0p2): disk space caching is enabled -BTRFS: detected SSD devices, enabling SSD mode
-VFS: Mounted root (btrfs filesystem) readonly on device 0:13. -devtmpfs: mounted
-Freeing unused kernel memory: 316K (c057a000 - c05c9000) -BTRFS info (device mmcblk0p2): disk space caching is enabled -
-(none) login: [ 5.252380] SysRq : Resetting
+Waiting for root device /dev/mmcblk0p2...
+mmc0: card never left busy state
+mmc0: error -110 whilst initialising SD card

I didn't change a thing, I just pressed reset. As you see the content of the TWL registers don't seem to be that important since it was the same at the time of the dump. And the mmc didn't came back.

Quoted 5 lines

Regards,
Tony

Sebastian

Tony Lindgren

author maintainer

2014-09-04 16:39 UTC #8 lore raw

PATCH 1/1 mfd: twl4030-power: Fix PM idle pin configuration to not conflict with regulators

Quoted 14 lines · Sebastian Andrzej Siewior

Quoted 9 lines · Tony Lindgren
bad.txt 2014-09-03 10:28:57.064313222 +0200
Quoted 5 lines
good.txt and bad.txt are from the late_initcall.
 $ diff -u good.txt bad.txt
bad.txt 2014-09-03 10:28:57.064313222 +0200 renamed from good.txt 2014-09-03 10:29:58.920317368 +0200 +0 -0
Hmm can you check that you have good.txt and bad.txt the right way? I'd assume you need VAUX2 or VAUX3 enabled not disabled for the MMC to work?
No, it was correct. If you look at the complete file you will notice the - which removes the mmc detect/mount in the bad case and + which adds the -110 error

...

Quoted 3 lines

With that patch it seems it is a little harder to trigger. It is usually every other boot that fails. Here a diff between two that worked (say good-v1 vs good-v2):

...

Quoted 3 lines

It took mmc a little longer to detect but it worked. And the content of the three registers seem not to matter _or_ it was dumped before MMC got active.

...

Quoted 3 lines

I didn't change a thing, I just pressed reset. As you see the content of the TWL registers don't seem to be that important since it was the same at the time of the dump. And the mmc didn't came back.

OK. I guess that means that it's most likely some timing related issue, or an issue with some regulator not being ready by the time the MMC probes. Or it's a bug somewhere that I'm not figuring out.

Looking at the twl-regulator.c, twl4030reg_enable just sets the group bit and we're not checking any status and don't have startup-delay-us for them.

At least phy-twl4030-usb.c has twl4030_i2c_write_u8_verify(), I wonder if adding a read back of the register to twl-regulator.c would help?

Regards,

Tony