ARM: OMAP2+: AM33XX: VDD CORE OPP50 support

Changes since v1:
* Rebased onto new am335x PM branch
* Changed to use 5th param register

Changes since v2:
* Passes I2C bus speed in kHz to M3 firmware

Changes since v3:
* Rebased to 3.11-rc3, moves some functionality to wkup_m3.c
* Additional comments
* Added device-tree binding documentation

This patch adds the ability to pass an I2C sleep sequence and wake sequence to the Cortex-M3. This is useful for adjusting voltages during sleep that cannot be lowered while SDRAM is active. A modified M3 firmware with I2C support is required.

Each sequence is a series of I2C transfers in the form:

u8 length | u8 chip address | u8 byte0/reg address | u8 byte 1 | u8 byte n ...

The length indicates the number of bytes to transfer, including the register address. The length of each transfer is limited by the I2C buffer size of 32 bytes.

The sequences are taken from the i2c1 node in the device tree. The property name for the sleep sequence is "sleep_sequence" and the property name for the wake sequence is "wake_sequence". Each property should be an array of bytes.

No actions are performed if the properties are not present in the device tree.

Signed-off-by Russ Dill <[email protected]>

---
 .../devicetree/bindings/i2c/i2c-suspend-resume.txt | 44 +++++++++++
 arch/arm/mach-omap2/control.c                      |  1 +
 arch/arm/mach-omap2/pm33xx.c                       | 89 ++++++++++++++++++++++
 arch/arm/mach-omap2/pm33xx.h                       |  2 +
 arch/arm/mach-omap2/wkup_m3.c                      | 57 ++++++++++++--
 5 files changed, 186 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-suspend-resume.txt

Documentation/devicetree/bindings/i2c/i2c-suspend-resume.txt new file +21 -0

new file mode 100644
index 0000000..af19372
@@ -0,0 +1,44 @@
+I2C suspend/resume sequences
+
+This provides the ability for a simple I2C sequence to be written at
+suspend time and resume time. This is for sequences that cannot be written
+by the I2C bus driver for reasons such as needing to be run from SRAM
+or needing to be written by firmware.
+
+The sequence is composed of messages. Each message contains a length byte,
+an address byte, and then the message.
+
+Optional properties:
+- sleep_sequence
+	I2C sequence to write during suspend
+
+- wake_sequence
+	I2C sequence to write during wake
+
+Examples :
+
+i2c0: i2c at 0 {
+	/* Set OPP50 (0.95V) for VDD core */

Documentation/devicetree/bindings/i2c/i2c-suspend-resume.txt new file +13 -0

+
+	/* Set OPP100 (1.10V) for VDD core */
+	wake_sequence = /bits/ 8 <
+		0x02 0x24 0x0b 0x6d /* Password unlock 1 */
+		0x02 0x24 0x10 0x08 /* Set DCDC3 to 1.1V */
+		0x02 0x24 0x0b 0x6d /* Password unlock 2 */
+		0x02 0x24 0x10 0x08 /* Set DCDC3 to 1.1V */
+		0x02 0x24 0x0b 0x6c /* Password unlock 1 */
+		0x02 0x24 0x11 0x86 /* Apply DCDC changes */
+		0x02 0x24 0x0b 0x6c /* Password unlock 2 */
+		0x02 0x24 0x11 0x86 /* Apply DCDC changes */
+	>;
+}

arch/arm/mach-omap2/control.c +1 -0

index 934041a..dfbd5f0 100644
@@ -639,6 +639,7 @@ void am33xx_pm_ipc_cmd(struct am33xx_ipc_data *data)
 	omap_ctrl_writel(data->param1, AM33XX_CONTROL_IPC_MSG_REG2);
 	omap_ctrl_writel(data->param2, AM33XX_CONTROL_IPC_MSG_REG3);
 	omap_ctrl_writel(data->param3, AM33XX_CONTROL_IPC_MSG_REG4);
+	omap_ctrl_writel(data->param4, AM33XX_CONTROL_IPC_MSG_REG5);
 }
 
 int am33xx_pm_status(void)

arch/arm/mach-omap2/pm33xx.c +6 -0

index d291c76..8880da3 100644
@@ -29,6 +29,7 @@
 #include <linux/ti_emif.h>
 #include <linux/omap-mailbox.h>
 
+#include <asm/unaligned.h>
 #include <asm/suspend.h>
 #include <asm/proc-fns.h>
 #include <asm/sizes.h>

@@ -50,6 +51,10 @@
 static void __iomem *am33xx_emif_base;
 static struct powerdomain *cefuse_pwrdm, *gfx_pwrdm, *per_pwrdm, *mpu_pwrdm;
 static struct clockdomain *gfx_l4ls_clkdm;
+static char *am33xx_i2c_sleep_sequence;
+static char *am33xx_i2c_wake_sequence;
+static size_t i2c_sleep_sequence_sz;
+static size_t i2c_wake_sequence_sz;
 
 struct wakeup_src wakeups[] = {
 	{.irq_nr = 35,	.src = "USB0_PHY"},

@@ -232,12 +237,34 @@ static void am33xx_m3_state_machine_reset(void)
 static int am33xx_pm_begin(suspend_state_t state)
 {
 	int i;
+	unsigned long param4;

arch/arm/mach-omap2/pm33xx.c +1 -0

 	am33xx_pm->ipc.sleep_mode	= IPC_CMD_DS0;
 	am33xx_pm->ipc.param1		= DS_IPC_DEFAULT;
 	am33xx_pm->ipc.param2		= DS_IPC_DEFAULT;
+	am33xx_pm->ipc.param4		= param4;
 
 	am33xx_pm_ipc_cmd(&am33xx_pm->ipc);
 

arch/arm/mach-omap2/pm33xx.c +29 -0

+		i2c_sleep_sequence_sz = sz + 3;
+	}
+
+	prop = of_get_property(dev->of_node, "wake_sequence", &sz);
+	if (prop) {
+		am33xx_i2c_wake_sequence = kzalloc(sz + 3, GFP_KERNEL);
+		if (!am33xx_i2c_wake_sequence) {
+			ret = -ENOMEM;
+			goto cleanup_sleep;
+		}
+		put_unaligned_le16(freq_khz, am33xx_i2c_sleep_sequence);
+		memcpy(am33xx_i2c_wake_sequence + 2, prop, sz);
+		i2c_wake_sequence_sz = sz + 3;
+	}
+
+	return 0;
+
+cleanup_sleep:
+	kfree(am33xx_i2c_sleep_sequence);
+	am33xx_i2c_sleep_sequence = NULL;
+	return ret;
+}
+
 int __init am33xx_pm_init(void)
 {
 	int ret;

@@ -451,6 +534,12 @@ int __init am33xx_pm_init(void)
 		}
 	}
 
+	ret = am33xx_setup_sleep_sequence();
+	if (ret) {
+		pr_err("Error fetching I2C sleep/wake sequence\n");
+		goto err;
+	}
+
 	(void) clkdm_for_each(omap_pm_clkdms_setup, NULL);
 
 	/* CEFUSE domain can be turned off post bootup */

arch/arm/mach-omap2/pm33xx.h +2 -0

index befdd11..d0f08a5 100644
@@ -52,6 +52,8 @@ struct forced_standby_module {
 };
 
 int wkup_m3_copy_code(const u8 *data, size_t size);
+void wkup_m3_reset_data_pos(void);
+int wkup_m3_copy_data(const u8 *data, size_t size);
 int wkup_m3_prepare(void);
 void wkup_m3_register_txev_handler(void (*txev_handler)(void));
 

arch/arm/mach-omap2/wkup_m3.c +28 -1

index 8eaa7f3..a541de9 100644
@@ -35,6 +35,9 @@
 struct wkup_m3_context {
 	struct device	*dev;
 	void __iomem	*code;
+	void __iomem	*data;
+	void __iomem	*data_end;
+	size_t		data_size;
 	void (*txev_handler)(void);
 };
 

@@ -50,6 +53,30 @@ int wkup_m3_copy_code(const u8 *data, size_t size)
 	return 0;
 }
 
+/*
+ * This pair of functions allows data to be stuffed into the end of the
+ * CM3 data memory. This is currently used for passing the I2C sleep/wake
+ * sequences to the firmware.
+ */
+
+/* Clear out the pointer for data stored at the end of DMEM */
+void wkup_m3_reset_data_pos(void)
+{
+	wkup_m3->data_end = wkup_m3->data + wkup_m3->data_size;
+}
+
+/*
+ * Store a block of data at the end of DMEM, return the offset within DMEM
+ * that the data is stored at, or -ENOMEM if the data did not fit
+ */
+int wkup_m3_copy_data(const u8 *data, size_t size)
+{
+	if (wkup_m3->data + size > wkup_m3->data_end)
+		return -ENOMEM;
+	wkup_m3->data_end -= size;
+	memcpy_toio(wkup_m3->data_end, data, size);
+	return wkup_m3->data_end - wkup_m3->data;
+}
 
 void wkup_m3_register_txev_handler(void (*txev_handler)(void))
 {

@@ -83,7 +110,7 @@ int wkup_m3_prepare(void)
 static int wkup_m3_probe(struct platform_device *pdev)
 {
 	int irq, ret = 0;
-	struct resource *mem;
+	struct resource *umem, *dmem;
 
 	pm_runtime_enable(&pdev->dev);
 

arch/arm/mach-omap2/wkup_m3.c +10 -3

+		ret = -ENXIO;
+		goto err;
+	}
+
+	umem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!umem) {
+		dev_err(&pdev->dev, "no UMEM resource\n");
 		ret = -ENXIO;
 		goto err;
 	}
 
-	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!mem) {
-		dev_err(wkup_m3->dev, "no memory resource\n");
+	dmem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!dmem) {
+		dev_err(&pdev->dev, "no DMEM resource\n");
 		ret = -ENXIO;
 		goto err;
 	}

arch/arm/mach-omap2/wkup_m3.c +2 -1

 		ret = -EADDRNOTAVAIL;
 		goto err;
 	}
+	wkup_m3->data_size = resource_size(dmem);
+	wkup_m3_reset_data_pos();
 
 	ret = devm_request_irq(wkup_m3->dev, irq, wkup_m3_txev_handler,
 		  IRQF_DISABLED, "wkup_m3_txev", NULL);
-- 1.8.3.2 -- To unsubscribe from this list: send the line "unsubscribe

linux-omap" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 11 lines · Russ Dill

Changes since v1:
* Rebased onto new am335x PM branch
* Changed to use 5th param register

Changes since v2:
* Passes I2C bus speed in kHz to M3 firmware

Changes since v3:
* Rebased to 3.11-rc3, moves some functionality to wkup_m3.c
* Additional comments
* Added device-tree binding documentation

AFAIK, Change logs should come below scissors line (---). However there was some discussion to keep it in the commit message. Don't know what happened to it finally. But till date, all revision patch set has the change log under the scissor line.

Quoted 59 lines

This patch adds the ability to pass an I2C sleep sequence and wake sequence to the Cortex-M3. This is useful for adjusting voltages during sleep that cannot be lowered while SDRAM is active. A modified M3 firmware with I2C support is required.

Each sequence is a series of I2C transfers in the form:

u8 length | u8 chip address | u8 byte0/reg address | u8 byte 1 | u8 byte n ...

The length indicates the number of bytes to transfer, including the register address. The length of each transfer is limited by the I2C buffer size of 32 bytes.

The sequences are taken from the i2c1 node in the device tree. The property name for the sleep sequence is "sleep_sequence" and the property name for the wake sequence is "wake_sequence". Each property should be an array of bytes.

No actions are performed if the properties are not present in the device tree.

Signed-off-by Russ Dill <[email protected]>

---
 .../devicetree/bindings/i2c/i2c-suspend-resume.txt | 44 +++++++++++
 arch/arm/mach-omap2/control.c                      |  1 +
 arch/arm/mach-omap2/pm33xx.c                       | 89 ++++++++++++++++++++++
 arch/arm/mach-omap2/pm33xx.h                       |  2 +
 arch/arm/mach-omap2/wkup_m3.c                      | 57 ++++++++++++--
 5 files changed, 186 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-suspend-resume.txt

Documentation/devicetree/bindings/i2c/i2c-suspend-resume.txt new file +21 -0

new file mode 100644
index 0000000..af19372
@@ -0,0 +1,44 @@
+I2C suspend/resume sequences
+
+This provides the ability for a simple I2C sequence to be written at
+suspend time and resume time. This is for sequences that cannot be written
+by the I2C bus driver for reasons such as needing to be run from SRAM
+or needing to be written by firmware.
+
+The sequence is composed of messages. Each message contains a length byte,
+an address byte, and then the message.
+
+Optional properties:
+- sleep_sequence
+     I2C sequence to write during suspend
+
+- wake_sequence
+     I2C sequence to write during wake
+
+Examples :
+
+i2c0: i2c at 0 {
+     /* Set OPP50 (0.95V) for VDD core */

Documentation/devicetree/bindings/i2c/i2c-suspend-resume.txt line 31

Quoted 10 lines

Documentation/devicetree/bindings/i2c/i2c-suspend-resume.txt new file +10 -0

+     sleep_sequence = /bits/ 8 <
+             0x02 0x24 0x0b 0x6d /* Password unlock 1 */
+             0x02 0x24 0x10 0x02 /* Set DCDC3 to 0.95V */
+             0x02 0x24 0x0b 0x6d /* Password unlock 2 */
+             0x02 0x24 0x10 0x02 /* Set DCDC3 to 0.95V */
+             0x02 0x24 0x0b 0x6c /* Password unlock 1 */
+             0x02 0x24 0x11 0x86 /* Apply DCDC changes */
+             0x02 0x24 0x0b 0x6c /* Password unlock 2 */
+             0x02 0x24 0x11 0x86 /* Apply DCDC changes */
+     >;

This data is not related to i2c. Right? Then why is it under i2c dt node?

There is already a wakeup node (wkup_m3) and a pmic node (pmic node for respective boards). Can't these details go under those nodes?

Quoted 57 lines

Documentation/devicetree/bindings/i2c/i2c-suspend-resume.txt new file +13 -0

+
+     /* Set OPP100 (1.10V) for VDD core */
+     wake_sequence = /bits/ 8 <
+             0x02 0x24 0x0b 0x6d /* Password unlock 1 */
+             0x02 0x24 0x10 0x08 /* Set DCDC3 to 1.1V */
+             0x02 0x24 0x0b 0x6d /* Password unlock 2 */
+             0x02 0x24 0x10 0x08 /* Set DCDC3 to 1.1V */
+             0x02 0x24 0x0b 0x6c /* Password unlock 1 */
+             0x02 0x24 0x11 0x86 /* Apply DCDC changes */
+             0x02 0x24 0x0b 0x6c /* Password unlock 2 */
+             0x02 0x24 0x11 0x86 /* Apply DCDC changes */
+     >;
+}

arch/arm/mach-omap2/control.c +1 -0

index 934041a..dfbd5f0 100644
@@ -639,6 +639,7 @@ void am33xx_pm_ipc_cmd(struct am33xx_ipc_data *data)
      omap_ctrl_writel(data->param1, AM33XX_CONTROL_IPC_MSG_REG2);
      omap_ctrl_writel(data->param2, AM33XX_CONTROL_IPC_MSG_REG3);
      omap_ctrl_writel(data->param3, AM33XX_CONTROL_IPC_MSG_REG4);
+     omap_ctrl_writel(data->param4, AM33XX_CONTROL_IPC_MSG_REG5);
 }
 
 int am33xx_pm_status(void)

arch/arm/mach-omap2/pm33xx.c +7 -0

index d291c76..8880da3 100644
@@ -29,6 +29,7 @@
 #include <linux/ti_emif.h>
 #include <linux/omap-mailbox.h>
 
+#include <asm/unaligned.h>
 #include <asm/suspend.h>
 #include <asm/proc-fns.h>
 #include <asm/sizes.h>

@@ -50,6 +51,10 @@
 static void __iomem *am33xx_emif_base;
 static struct powerdomain *cefuse_pwrdm, *gfx_pwrdm, *per_pwrdm, *mpu_pwrdm;
 static struct clockdomain *gfx_l4ls_clkdm;
+static char *am33xx_i2c_sleep_sequence;
+static char *am33xx_i2c_wake_sequence;
+static size_t i2c_sleep_sequence_sz;
+static size_t i2c_wake_sequence_sz;
 
 struct wakeup_src wakeups[] = {
      {.irq_nr = 35,  .src = "USB0_PHY"},

@@ -232,12 +237,34 @@ static void am33xx_m3_state_machine_reset(void)
 static int am33xx_pm_begin(suspend_state_t state)
 {
      int i;
+     unsigned long param4;
+     int pos;
 
      cpu_idle_poll_ctrl(true);
 

arch/arm/mach-omap2/pm33xx.c static void am33xx_m3_state_machine_reset(void) · line 250

Quoted 6 lines

arch/arm/mach-omap2/pm33xx.c +6 -0

+     param4 = DS_IPC_DEFAULT;
+
+     wkup_m3_reset_data_pos();
+     if (am33xx_i2c_sleep_sequence) {
+             pos = wkup_m3_copy_data(am33xx_i2c_sleep_sequence,
+                                             i2c_sleep_sequence_sz);

Why do we need to copy this data (same constant data) on every iteration?

arch/arm/mach-omap2/pm33xx.c static void am33xx_m3_state_machine_reset(void) · line 263

Quoted 13 lines

arch/arm/mach-omap2/pm33xx.c +13 -0

+             /* Lower 16 bits stores offset to sleep sequence */
+             param4 &= ~0xffff;
+             param4 |= pos;
+     }
+
+     if (am33xx_i2c_wake_sequence) {
+             pos = wkup_m3_copy_data(am33xx_i2c_wake_sequence,
+                                             i2c_wake_sequence_sz);
+             /* Upper 16 bits stores offset to wake sequence */
+             param4 &= ~0xffff0000;
+             param4 |= pos << 16;
+     }
+

Seems above entire change can be done only once.

Quoted 11 lines

arch/arm/mach-omap2/pm33xx.c +1 -0

      am33xx_pm->ipc.sleep_mode       = IPC_CMD_DS0;
      am33xx_pm->ipc.param1           = DS_IPC_DEFAULT;
      am33xx_pm->ipc.param2           = DS_IPC_DEFAULT;
+     am33xx_pm->ipc.param4           = param4;
 
      am33xx_pm_ipc_cmd(&am33xx_pm->ipc);
 

@@ -386,6 +413,62 @@ static int __init am33xx_map_emif(void)
      return 0;
 }
 

arch/arm/mach-omap2/pm33xx.c static int __init am33xx_map_emif(void) · line 422

Quoted 7 lines

arch/arm/mach-omap2/pm33xx.c +7 -0

+static int __init am33xx_setup_sleep_sequence(void)
+{
+     int ret;
+     int sz;
+     const void *prop;
+     struct device *dev;
+     u32 freq_hz = 100000;

Magic number?

arch/arm/mach-omap2/pm33xx.c static int __init am33xx_map_emif(void) · line 429

Quoted 7 lines

arch/arm/mach-omap2/pm33xx.c +7 -0

+     unsigned short freq_khz;
+
+     /*
+      * We put the device tree node in the I2C controller that will
+      * be sending the sequence. i2c1 is the only controller that can
+      * be accessed by the firmware as it is the only controller in the
+      * WKUP domain.

and on which the PMIC sits I believe?

arch/arm/mach-omap2/pm33xx.c static int __init am33xx_map_emif(void) · line 436

Quoted 7 lines

arch/arm/mach-omap2/pm33xx.c +7 -0

+      */
+     dev = omap_device_get_by_hwmod_name("i2c1");
+     if (IS_ERR(dev))
+             return PTR_ERR(dev);
+
+     of_property_read_u32(dev->of_node, "clock-frequency", &freq_hz);
+     freq_khz = freq_hz / 1000;

Magic number?

arch/arm/mach-omap2/pm33xx.c static int __init am33xx_map_emif(void) · line 448

Quoted 12 lines

arch/arm/mach-omap2/pm33xx.c +12 -0

+
+     prop = of_get_property(dev->of_node, "sleep_sequence", &sz);
+     if (prop) {
+             /*
+              * Length is sequence length + 2 bytes for freq_khz, and 1
+              * byte for terminator.
+              */
+             am33xx_i2c_sleep_sequence = kzalloc(sz + 3, GFP_KERNEL);
+             if (!am33xx_i2c_sleep_sequence)
+                     return -ENOMEM;
+             put_unaligned_le16(freq_khz, am33xx_i2c_sleep_sequence);
+             memcpy(am33xx_i2c_sleep_sequence + 2, prop, sz);

so, looking at entire code, it seems there is 3 memory space for same data (or 1 original + 2 copy)

1. in DT
2. in am33xx_i2c_[sleep/wake]_sequence
3. in SRAm by call to wkup_m3_copy_data()

why not directly copy to SRAM from DT?

Quoted 106 lines

arch/arm/mach-omap2/pm33xx.c +29 -0

+             i2c_sleep_sequence_sz = sz + 3;
+     }
+
+     prop = of_get_property(dev->of_node, "wake_sequence", &sz);
+     if (prop) {
+             am33xx_i2c_wake_sequence = kzalloc(sz + 3, GFP_KERNEL);
+             if (!am33xx_i2c_wake_sequence) {
+                     ret = -ENOMEM;
+                     goto cleanup_sleep;
+             }
+             put_unaligned_le16(freq_khz, am33xx_i2c_sleep_sequence);
+             memcpy(am33xx_i2c_wake_sequence + 2, prop, sz);
+             i2c_wake_sequence_sz = sz + 3;
+     }
+
+     return 0;
+
+cleanup_sleep:
+     kfree(am33xx_i2c_sleep_sequence);
+     am33xx_i2c_sleep_sequence = NULL;
+     return ret;
+}
+
 int __init am33xx_pm_init(void)
 {
      int ret;

@@ -451,6 +534,12 @@ int __init am33xx_pm_init(void)
              }
      }
 
+     ret = am33xx_setup_sleep_sequence();
+     if (ret) {
+             pr_err("Error fetching I2C sleep/wake sequence\n");
+             goto err;
+     }
+
      (void) clkdm_for_each(omap_pm_clkdms_setup, NULL);
 
      /* CEFUSE domain can be turned off post bootup */

arch/arm/mach-omap2/pm33xx.h +2 -0

index befdd11..d0f08a5 100644
@@ -52,6 +52,8 @@ struct forced_standby_module {
 };
 
 int wkup_m3_copy_code(const u8 *data, size_t size);
+void wkup_m3_reset_data_pos(void);
+int wkup_m3_copy_data(const u8 *data, size_t size);
 int wkup_m3_prepare(void);
 void wkup_m3_register_txev_handler(void (*txev_handler)(void));
 

arch/arm/mach-omap2/wkup_m3.c +28 -1

index 8eaa7f3..a541de9 100644
@@ -35,6 +35,9 @@
 struct wkup_m3_context {
      struct device   *dev;
      void __iomem    *code;
+     void __iomem    *data;
+     void __iomem    *data_end;
+     size_t          data_size;
      void (*txev_handler)(void);
 };
 

@@ -50,6 +53,30 @@ int wkup_m3_copy_code(const u8 *data, size_t size)
      return 0;
 }
 
+/*
+ * This pair of functions allows data to be stuffed into the end of the
+ * CM3 data memory. This is currently used for passing the I2C sleep/wake
+ * sequences to the firmware.
+ */
+
+/* Clear out the pointer for data stored at the end of DMEM */
+void wkup_m3_reset_data_pos(void)
+{
+     wkup_m3->data_end = wkup_m3->data + wkup_m3->data_size;
+}
+
+/*
+ * Store a block of data at the end of DMEM, return the offset within DMEM
+ * that the data is stored at, or -ENOMEM if the data did not fit
+ */
+int wkup_m3_copy_data(const u8 *data, size_t size)
+{
+     if (wkup_m3->data + size > wkup_m3->data_end)
+             return -ENOMEM;
+     wkup_m3->data_end -= size;
+     memcpy_toio(wkup_m3->data_end, data, size);
+     return wkup_m3->data_end - wkup_m3->data;
+}
 
 void wkup_m3_register_txev_handler(void (*txev_handler)(void))
 {

@@ -83,7 +110,7 @@ int wkup_m3_prepare(void)
 static int wkup_m3_probe(struct platform_device *pdev)
 {
      int irq, ret = 0;
-     struct resource *mem;
+     struct resource *umem, *dmem;
 
      pm_runtime_enable(&pdev->dev);
 

arch/arm/mach-omap2/wkup_m3.c static int wkup_m3_probe(struct platform_device *pdev) · line 125

Quoted 6 lines

arch/arm/mach-omap2/wkup_m3.c +1 -1

@@ -95,14 +122,21 @@ static int wkup_m3_probe(struct platform_device *pdev)
 
      irq = platform_get_irq(pdev, 0);
      if (!irq) {
-             dev_err(wkup_m3->dev, "no irq resource\n");
+             dev_err(&pdev->dev, "no irq resource\n");

unrelated change?. Better to mention this as code cleanup in commit.

Quoted 22 lines

arch/arm/mach-omap2/wkup_m3.c +10 -3

+             ret = -ENXIO;
+             goto err;
+     }
+
+     umem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+     if (!umem) {
+             dev_err(&pdev->dev, "no UMEM resource\n");
              ret = -ENXIO;
              goto err;
      }
 
-     mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-     if (!mem) {
-             dev_err(wkup_m3->dev, "no memory resource\n");
+     dmem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+     if (!dmem) {
+             dev_err(&pdev->dev, "no DMEM resource\n");
              ret = -ENXIO;
              goto err;
      }

@@ -116,12 +150,21 @@ static int wkup_m3_probe(struct platform_device *pdev)
 

arch/arm/mach-omap2/wkup_m3.c static int wkup_m3_probe(struct platform_device *pdev) · line 155

Quoted 6 lines

arch/arm/mach-omap2/wkup_m3.c +3 -1

      wkup_m3->dev = &pdev->dev;
 
-     wkup_m3->code = devm_request_and_ioremap(wkup_m3->dev, mem);
+     wkup_m3->code = devm_request_and_ioremap(wkup_m3->dev, umem);
+     if (!wkup_m3->code) {
+             dev_err(wkup_m3->dev, "could not ioremap UMEM\n");

why not use "pdev->dev" here?

arch/arm/mach-omap2/wkup_m3.c static int wkup_m3_probe(struct platform_device *pdev) · line 161

Quoted 6 lines

arch/arm/mach-omap2/wkup_m3.c +5 -0

+             ret = -EADDRNOTAVAIL;
+             goto err;
+     }
+
+     wkup_m3->data = devm_request_and_ioremap(wkup_m3->dev, dmem);
      if (!wkup_m3->code) {

I believe this is just a copy/paste error. s/code/data

arch/arm/mach-omap2/wkup_m3.c static int wkup_m3_probe(struct platform_device *pdev) · line 162

Quoted 2 lines

arch/arm/mach-omap2/wkup_m3.c +1 -1

-             dev_err(wkup_m3->dev, "could not ioremap\n");
+             dev_err(wkup_m3->dev, "could not ioremap DMEM\n");

same as above.

Quoted 9 lines

arch/arm/mach-omap2/wkup_m3.c +2 -1

              ret = -EADDRNOTAVAIL;
              goto err;
      }
+     wkup_m3->data_size = resource_size(dmem);
+     wkup_m3_reset_data_pos();
 
      ret = devm_request_irq(wkup_m3->dev, irq, wkup_m3_txev_handler,
                IRQF_DISABLED, "wkup_m3_txev", NULL);
-- 1.8.3.2 -- To unsubscribe from this list: send the line "unsubscribe

Quoted 3 lines

linux-omap" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 13 lines · Gururaja Hebbar

Quoted 11 lines · Russ Dill

Changes since v1:
* Rebased onto new am335x PM branch
* Changed to use 5th param register

Changes since v2:
* Passes I2C bus speed in kHz to M3 firmware

Changes since v3:
* Rebased to 3.11-rc3, moves some functionality to wkup_m3.c
* Additional comments
* Added device-tree binding documentation

Quoted 4 lines

AFAIK, Change logs should come below scissors line (---). However there was some discussion to keep it in the commit message. Don't know what happened to it finally. But till date, all revision patch set has the change log under the scissor line.

Will fix is v2.

Quoted 72 lines

Quoted 59 lines

This patch adds the ability to pass an I2C sleep sequence and wake sequence to the Cortex-M3. This is useful for adjusting voltages during sleep that cannot be lowered while SDRAM is active. A modified M3 firmware with I2C support is required.

Each sequence is a series of I2C transfers in the form:

u8 length | u8 chip address | u8 byte0/reg address | u8 byte 1 | u8 byte n ...

The length indicates the number of bytes to transfer, including the register address. The length of each transfer is limited by the I2C buffer size of 32 bytes.

The sequences are taken from the i2c1 node in the device tree. The property name for the sleep sequence is "sleep_sequence" and the property name for the wake sequence is "wake_sequence". Each property should be an array of bytes.

No actions are performed if the properties are not present in the device tree.

Signed-off-by Russ Dill <[email protected]>

---
 .../devicetree/bindings/i2c/i2c-suspend-resume.txt | 44 +++++++++++
 arch/arm/mach-omap2/control.c                      |  1 +
 arch/arm/mach-omap2/pm33xx.c                       | 89 ++++++++++++++++++++++
 arch/arm/mach-omap2/pm33xx.h                       |  2 +
 arch/arm/mach-omap2/wkup_m3.c                      | 57 ++++++++++++--
 5 files changed, 186 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-suspend-resume.txt

Documentation/devicetree/bindings/i2c/i2c-suspend-resume.txt new file +21 -0

new file mode 100644
index 0000000..af19372
@@ -0,0 +1,44 @@
+I2C suspend/resume sequences
+
+This provides the ability for a simple I2C sequence to be written at
+suspend time and resume time. This is for sequences that cannot be written
+by the I2C bus driver for reasons such as needing to be run from SRAM
+or needing to be written by firmware.
+
+The sequence is composed of messages. Each message contains a length byte,
+an address byte, and then the message.
+
+Optional properties:
+- sleep_sequence
+     I2C sequence to write during suspend
+
+- wake_sequence
+     I2C sequence to write during wake
+
+Examples :
+
+i2c0: i2c at 0 {
+     /* Set OPP50 (0.95V) for VDD core */

Documentation/devicetree/bindings/i2c/i2c-suspend-resume.txt line 31

Quoted 10 lines

Documentation/devicetree/bindings/i2c/i2c-suspend-resume.txt new file +10 -0

+     sleep_sequence = /bits/ 8 <
+             0x02 0x24 0x0b 0x6d /* Password unlock 1 */
+             0x02 0x24 0x10 0x02 /* Set DCDC3 to 0.95V */
+             0x02 0x24 0x0b 0x6d /* Password unlock 2 */
+             0x02 0x24 0x10 0x02 /* Set DCDC3 to 0.95V */
+             0x02 0x24 0x0b 0x6c /* Password unlock 1 */
+             0x02 0x24 0x11 0x86 /* Apply DCDC changes */
+             0x02 0x24 0x0b 0x6c /* Password unlock 2 */
+             0x02 0x24 0x11 0x86 /* Apply DCDC changes */
+     >;

This data is not related to i2c. Right? Then why is it under i2c dt node?

Quoted 2 lines

There is already a wakeup node (wkup_m3) and a pmic node (pmic node for respective boards). Can't these details go under those nodes?

The i2c node was chosen for several reasons. First that the bus speed is included in the message to the CM3 firmware. The bus speed and sequence can then be read from the same device tree node. Second so it's clear which I2C bus this is being sent out on. Including it under the PMIC node would mean that the code would have to search each child of the I2C bus for a sequence and if multiple children had sequences, it would not know in which order to send those sequences out.

Quoted 58 lines

Quoted 58 lines

Documentation/devicetree/bindings/i2c/i2c-suspend-resume.txt new file +13 -0

+
+     /* Set OPP100 (1.10V) for VDD core */
+     wake_sequence = /bits/ 8 <
+             0x02 0x24 0x0b 0x6d /* Password unlock 1 */
+             0x02 0x24 0x10 0x08 /* Set DCDC3 to 1.1V */
+             0x02 0x24 0x0b 0x6d /* Password unlock 2 */
+             0x02 0x24 0x10 0x08 /* Set DCDC3 to 1.1V */
+             0x02 0x24 0x0b 0x6c /* Password unlock 1 */
+             0x02 0x24 0x11 0x86 /* Apply DCDC changes */
+             0x02 0x24 0x0b 0x6c /* Password unlock 2 */
+             0x02 0x24 0x11 0x86 /* Apply DCDC changes */
+     >;
+}

arch/arm/mach-omap2/control.c +1 -0

index 934041a..dfbd5f0 100644
@@ -639,6 +639,7 @@ void am33xx_pm_ipc_cmd(struct am33xx_ipc_data *data)
      omap_ctrl_writel(data->param1, AM33XX_CONTROL_IPC_MSG_REG2);
      omap_ctrl_writel(data->param2, AM33XX_CONTROL_IPC_MSG_REG3);
      omap_ctrl_writel(data->param3, AM33XX_CONTROL_IPC_MSG_REG4);
+     omap_ctrl_writel(data->param4, AM33XX_CONTROL_IPC_MSG_REG5);
 }
 
 int am33xx_pm_status(void)

arch/arm/mach-omap2/pm33xx.c +7 -0

index d291c76..8880da3 100644
@@ -29,6 +29,7 @@
 #include <linux/ti_emif.h>
 #include <linux/omap-mailbox.h>
 
+#include <asm/unaligned.h>
 #include <asm/suspend.h>
 #include <asm/proc-fns.h>
 #include <asm/sizes.h>

@@ -50,6 +51,10 @@
 static void __iomem *am33xx_emif_base;
 static struct powerdomain *cefuse_pwrdm, *gfx_pwrdm, *per_pwrdm, *mpu_pwrdm;
 static struct clockdomain *gfx_l4ls_clkdm;
+static char *am33xx_i2c_sleep_sequence;
+static char *am33xx_i2c_wake_sequence;
+static size_t i2c_sleep_sequence_sz;
+static size_t i2c_wake_sequence_sz;
 
 struct wakeup_src wakeups[] = {
      {.irq_nr = 35,  .src = "USB0_PHY"},

@@ -232,12 +237,34 @@ static void am33xx_m3_state_machine_reset(void)
 static int am33xx_pm_begin(suspend_state_t state)
 {
      int i;
+     unsigned long param4;
+     int pos;
 
      cpu_idle_poll_ctrl(true);
 

Quoted 8 lines

arch/arm/mach-omap2/pm33xx.c static void am33xx_m3_state_machine_reset(void) · line 250

Quoted 6 lines

arch/arm/mach-omap2/pm33xx.c +6 -0

+     param4 = DS_IPC_DEFAULT;
+
+     wkup_m3_reset_data_pos();
+     if (am33xx_i2c_sleep_sequence) {
+             pos = wkup_m3_copy_data(am33xx_i2c_sleep_sequence,
+                                             i2c_sleep_sequence_sz);

Why do we need to copy this data (same constant data) on every iteration?

Because the CM3 firmware is reset after each suspend/resume cycle. The firmware reset handler reinitializes the DMEM.

Quoted 28 lines

Quoted 4 lines

arch/arm/mach-omap2/pm33xx.c +3 -0

+             /* Lower 16 bits stores offset to sleep sequence */
+             param4 &= ~0xffff;
+             param4 |= pos;

arch/arm/mach-omap2/pm33xx.c static void am33xx_m3_state_machine_reset(void) · line 263

Quoted 10 lines

arch/arm/mach-omap2/pm33xx.c +10 -0

+     }
+
+     if (am33xx_i2c_wake_sequence) {
+             pos = wkup_m3_copy_data(am33xx_i2c_wake_sequence,
+                                             i2c_wake_sequence_sz);
+             /* Upper 16 bits stores offset to wake sequence */
+             param4 &= ~0xffff0000;
+             param4 |= pos << 16;
+     }
+

Seems above entire change can be done only once.

Quoted 11 lines

arch/arm/mach-omap2/pm33xx.c +1 -0

      am33xx_pm->ipc.sleep_mode       = IPC_CMD_DS0;
      am33xx_pm->ipc.param1           = DS_IPC_DEFAULT;
      am33xx_pm->ipc.param2           = DS_IPC_DEFAULT;
+     am33xx_pm->ipc.param4           = param4;
 
      am33xx_pm_ipc_cmd(&am33xx_pm->ipc);
 

@@ -386,6 +413,62 @@ static int __init am33xx_map_emif(void)
      return 0;
 }
 

Quoted 9 lines

arch/arm/mach-omap2/pm33xx.c static int __init am33xx_map_emif(void) · line 422

Quoted 7 lines

arch/arm/mach-omap2/pm33xx.c +7 -0

+static int __init am33xx_setup_sleep_sequence(void)
+{
+     int ret;
+     int sz;
+     const void *prop;
+     struct device *dev;
+     u32 freq_hz = 100000;

Magic number?

It's taken from drivers/i2c/busses/i2c-omap.c

    u32 freq = 100000; /* default to 100000 Hz */

I'll add a comment to that effect.

Quoted 9 lines

arch/arm/mach-omap2/pm33xx.c static int __init am33xx_map_emif(void) · line 429

Quoted 7 lines

arch/arm/mach-omap2/pm33xx.c +7 -0

+     unsigned short freq_khz;
+
+     /*
+      * We put the device tree node in the I2C controller that will
+      * be sending the sequence. i2c1 is the only controller that can
+      * be accessed by the firmware as it is the only controller in the
+      * WKUP domain.

and on which the PMIC sits I believe?

Yes, but this code is designed not to be PMIC specific as one could chose to regulate VDD_CORE with any PMIC, or even with a standalone I2C controlled regulator.

Quoted 8 lines

Quoted 7 lines

arch/arm/mach-omap2/pm33xx.c +7 -0

+      */
+     dev = omap_device_get_by_hwmod_name("i2c1");
+     if (IS_ERR(dev))
+             return PTR_ERR(dev);
+
+     of_property_read_u32(dev->of_node, "clock-frequency", &freq_hz);
+     freq_khz = freq_hz / 1000;

Quoted 1 line

Magic number?

Nah, converting between metric prefixes this way is pretty common in the kernel.

Quoted 17 lines

arch/arm/mach-omap2/pm33xx.c static int __init am33xx_map_emif(void) · line 448

Quoted 13 lines

arch/arm/mach-omap2/pm33xx.c +12 -0

+
+     prop = of_get_property(dev->of_node, "sleep_sequence", &sz);
+     if (prop) {
+             /*
+              * Length is sequence length + 2 bytes for freq_khz, and 1
+              * byte for terminator.
+              */
+             am33xx_i2c_sleep_sequence = kzalloc(sz + 3, GFP_KERNEL);
+             if (!am33xx_i2c_sleep_sequence)
+                     return -ENOMEM;
+             put_unaligned_le16(freq_khz, am33xx_i2c_sleep_sequence);
+             memcpy(am33xx_i2c_sleep_sequence + 2, prop, sz);

so, looking at entire code, it seems there is 3 memory space for same data (or 1 original + 2 copy)

Quoted 5 lines

1. in DT
2. in am33xx_i2c_[sleep/wake]_sequence
3. in SRAm by call to wkup_m3_copy_data()

why not directly copy to SRAM from DT?

As pointed out above, the firmware reset handler would wipe it out.

Quoted 107 lines

Quoted 107 lines

arch/arm/mach-omap2/pm33xx.c +29 -0

+             i2c_sleep_sequence_sz = sz + 3;
+     }
+
+     prop = of_get_property(dev->of_node, "wake_sequence", &sz);
+     if (prop) {
+             am33xx_i2c_wake_sequence = kzalloc(sz + 3, GFP_KERNEL);
+             if (!am33xx_i2c_wake_sequence) {
+                     ret = -ENOMEM;
+                     goto cleanup_sleep;
+             }
+             put_unaligned_le16(freq_khz, am33xx_i2c_sleep_sequence);
+             memcpy(am33xx_i2c_wake_sequence + 2, prop, sz);
+             i2c_wake_sequence_sz = sz + 3;
+     }
+
+     return 0;
+
+cleanup_sleep:
+     kfree(am33xx_i2c_sleep_sequence);
+     am33xx_i2c_sleep_sequence = NULL;
+     return ret;
+}
+
 int __init am33xx_pm_init(void)
 {
      int ret;

@@ -451,6 +534,12 @@ int __init am33xx_pm_init(void)
              }
      }
 
+     ret = am33xx_setup_sleep_sequence();
+     if (ret) {
+             pr_err("Error fetching I2C sleep/wake sequence\n");
+             goto err;
+     }
+
      (void) clkdm_for_each(omap_pm_clkdms_setup, NULL);
 
      /* CEFUSE domain can be turned off post bootup */

arch/arm/mach-omap2/pm33xx.h +2 -0

index befdd11..d0f08a5 100644
@@ -52,6 +52,8 @@ struct forced_standby_module {
 };
 
 int wkup_m3_copy_code(const u8 *data, size_t size);
+void wkup_m3_reset_data_pos(void);
+int wkup_m3_copy_data(const u8 *data, size_t size);
 int wkup_m3_prepare(void);
 void wkup_m3_register_txev_handler(void (*txev_handler)(void));
 

arch/arm/mach-omap2/wkup_m3.c +28 -1

index 8eaa7f3..a541de9 100644
@@ -35,6 +35,9 @@
 struct wkup_m3_context {
      struct device   *dev;
      void __iomem    *code;
+     void __iomem    *data;
+     void __iomem    *data_end;
+     size_t          data_size;
      void (*txev_handler)(void);
 };
 

@@ -50,6 +53,30 @@ int wkup_m3_copy_code(const u8 *data, size_t size)
      return 0;
 }
 
+/*
+ * This pair of functions allows data to be stuffed into the end of the
+ * CM3 data memory. This is currently used for passing the I2C sleep/wake
+ * sequences to the firmware.
+ */
+
+/* Clear out the pointer for data stored at the end of DMEM */
+void wkup_m3_reset_data_pos(void)
+{
+     wkup_m3->data_end = wkup_m3->data + wkup_m3->data_size;
+}
+
+/*
+ * Store a block of data at the end of DMEM, return the offset within DMEM
+ * that the data is stored at, or -ENOMEM if the data did not fit
+ */
+int wkup_m3_copy_data(const u8 *data, size_t size)
+{
+     if (wkup_m3->data + size > wkup_m3->data_end)
+             return -ENOMEM;
+     wkup_m3->data_end -= size;
+     memcpy_toio(wkup_m3->data_end, data, size);
+     return wkup_m3->data_end - wkup_m3->data;
+}
 
 void wkup_m3_register_txev_handler(void (*txev_handler)(void))
 {

@@ -83,7 +110,7 @@ int wkup_m3_prepare(void)
 static int wkup_m3_probe(struct platform_device *pdev)
 {
      int irq, ret = 0;
-     struct resource *mem;
+     struct resource *umem, *dmem;
 
      pm_runtime_enable(&pdev->dev);
 

Quoted 8 lines

arch/arm/mach-omap2/wkup_m3.c static int wkup_m3_probe(struct platform_device *pdev) · line 125

Quoted 6 lines

arch/arm/mach-omap2/wkup_m3.c +1 -1

@@ -95,14 +122,21 @@ static int wkup_m3_probe(struct platform_device *pdev)
 
      irq = platform_get_irq(pdev, 0);
      if (!irq) {
-             dev_err(wkup_m3->dev, "no irq resource\n");
+             dev_err(&pdev->dev, "no irq resource\n");

unrelated change?. Better to mention this as code cleanup in commit.

Will add a comment to that effect, the underlying error should be fixed in the next suspend/resume patch though.

Quoted 24 lines

Quoted 24 lines

arch/arm/mach-omap2/wkup_m3.c +10 -3

+             ret = -ENXIO;
+             goto err;
+     }
+
+     umem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+     if (!umem) {
+             dev_err(&pdev->dev, "no UMEM resource\n");
              ret = -ENXIO;
              goto err;
      }
 
-     mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-     if (!mem) {
-             dev_err(wkup_m3->dev, "no memory resource\n");
+     dmem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+     if (!dmem) {
+             dev_err(&pdev->dev, "no DMEM resource\n");
              ret = -ENXIO;
              goto err;
      }

@@ -116,12 +150,21 @@ static int wkup_m3_probe(struct platform_device *pdev)
 
      wkup_m3->dev = &pdev->dev;
 

Quoted 6 lines

arch/arm/mach-omap2/wkup_m3.c static int wkup_m3_probe(struct platform_device *pdev) · line 155

Quoted 4 lines

arch/arm/mach-omap2/wkup_m3.c +3 -1

-     wkup_m3->code = devm_request_and_ioremap(wkup_m3->dev, mem);
+     wkup_m3->code = devm_request_and_ioremap(wkup_m3->dev, umem);
+     if (!wkup_m3->code) {
+             dev_err(wkup_m3->dev, "could not ioremap UMEM\n");

why not use "pdev->dev" here?

Either one works

Quoted 9 lines

arch/arm/mach-omap2/wkup_m3.c static int wkup_m3_probe(struct platform_device *pdev) · line 161

Quoted 7 lines

arch/arm/mach-omap2/wkup_m3.c +5 -0

+             ret = -EADDRNOTAVAIL;
+             goto err;
+     }
+
+     wkup_m3->data = devm_request_and_ioremap(wkup_m3->dev, dmem);
      if (!wkup_m3->code) {

I believe this is just a copy/paste error. s/code/data

Doh, thanks!

Quoted 23 lines

arch/arm/mach-omap2/wkup_m3.c static int wkup_m3_probe(struct platform_device *pdev) · line 162

Quoted 2 lines

arch/arm/mach-omap2/wkup_m3.c +1 -1

-             dev_err(wkup_m3->dev, "could not ioremap\n");
+             dev_err(wkup_m3->dev, "could not ioremap DMEM\n");

same as above.

Quoted 9 lines

arch/arm/mach-omap2/wkup_m3.c +2 -1

              ret = -EADDRNOTAVAIL;
              goto err;
      }
+     wkup_m3->data_size = resource_size(dmem);
+     wkup_m3_reset_data_pos();
 
      ret = devm_request_irq(wkup_m3->dev, irq, wkup_m3_txev_handler,
                IRQF_DISABLED, "wkup_m3_txev", NULL);
-- 1.8.3.2 -- To unsubscribe from this list: send the line "unsubscribe

Quoted 3 lines

linux-omap" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 6 lines · Gururaja Hebbar

Quoted 3 lines · Russ Dill

Changes since v1:
* Rebased onto new am335x PM branch
* Changed to use 5th param register

Quoted 7 lines

arch/arm/mach-omap2/pm33xx.c static void am33xx_m3_state_machine_reset(void) · line 250

Quoted 5 lines

arch/arm/mach-omap2/pm33xx.c +5 -0

+
+     wkup_m3_reset_data_pos();
+     if (am33xx_i2c_sleep_sequence) {
+             pos = wkup_m3_copy_data(am33xx_i2c_sleep_sequence,
+                                             i2c_sleep_sequence_sz);

Why do we need to copy this data (same constant data) on every iteration?

Because the CM3 firmware is reset after each suspend/resume cycle. The firmware reset handler reinitializes the DMEM.

Quoted 28 lines

Quoted 4 lines

arch/arm/mach-omap2/pm33xx.c +3 -0

+             /* Lower 16 bits stores offset to sleep sequence */
+             param4 &= ~0xffff;
+             param4 |= pos;

arch/arm/mach-omap2/pm33xx.c static void am33xx_m3_state_machine_reset(void) · line 263

Quoted 10 lines

arch/arm/mach-omap2/pm33xx.c +10 -0

+     }
+
+     if (am33xx_i2c_wake_sequence) {
+             pos = wkup_m3_copy_data(am33xx_i2c_wake_sequence,
+                                             i2c_wake_sequence_sz);
+             /* Upper 16 bits stores offset to wake sequence */
+             param4 &= ~0xffff0000;
+             param4 |= pos << 16;
+     }
+

Seems above entire change can be done only once.

Quoted 11 lines

      am33xx_pm->ipc.sleep_mode       = IPC_CMD_DS0;
      am33xx_pm->ipc.param1           = DS_IPC_DEFAULT;
      am33xx_pm->ipc.param2           = DS_IPC_DEFAULT;

arch/arm/mach-omap2/pm33xx.c +1 -0

+     am33xx_pm->ipc.param4           = param4;
 
      am33xx_pm_ipc_cmd(&am33xx_pm->ipc);

@@ -386,6 +413,62 @@ static int __init am33xx_map_emif(void)
      return 0;
 }
 

Quoted 9 lines

arch/arm/mach-omap2/pm33xx.c static int __init am33xx_map_emif(void) · line 422

Quoted 7 lines

arch/arm/mach-omap2/pm33xx.c +7 -0

+static int __init am33xx_setup_sleep_sequence(void)
+{
+     int ret;
+     int sz;
+     const void *prop;
+     struct device *dev;
+     u32 freq_hz = 100000;

Magic number?

It's taken from drivers/i2c/busses/i2c-omap.c

    u32 freq = 100000; /* default to 100000 Hz */

I'll add a comment to that effect.

Quoted 9 lines

arch/arm/mach-omap2/pm33xx.c static int __init am33xx_map_emif(void) · line 429

Quoted 7 lines

arch/arm/mach-omap2/pm33xx.c +7 -0

+     unsigned short freq_khz;
+
+     /*
+      * We put the device tree node in the I2C controller that will
+      * be sending the sequence. i2c1 is the only controller that can
+      * be accessed by the firmware as it is the only controller in the
+      * WKUP domain.

and on which the PMIC sits I believe?

Yes, but this code is designed not to be PMIC specific as one could chose to regulate VDD_CORE with any PMIC, or even with a standalone I2C controlled regulator.

Quoted 9 lines

arch/arm/mach-omap2/pm33xx.c static int __init am33xx_map_emif(void) · line 436

Quoted 7 lines

arch/arm/mach-omap2/pm33xx.c +7 -0

+      */
+     dev = omap_device_get_by_hwmod_name("i2c1");
+     if (IS_ERR(dev))
+             return PTR_ERR(dev);
+
+     of_property_read_u32(dev->of_node, "clock-frequency", &freq_hz);
+     freq_khz = freq_hz / 1000;

Magic number?

Nah, converting between metric prefixes this way is pretty common in the kernel.

Quoted 13 lines

arch/arm/mach-omap2/pm33xx.c static int __init am33xx_map_emif(void) · line 448

Quoted 12 lines

arch/arm/mach-omap2/pm33xx.c +12 -0

+
+     prop = of_get_property(dev->of_node, "sleep_sequence", &sz);
+     if (prop) {
+             /*
+              * Length is sequence length + 2 bytes for freq_khz, and 1
+              * byte for terminator.
+              */
+             am33xx_i2c_sleep_sequence = kzalloc(sz + 3, GFP_KERNEL);
+             if (!am33xx_i2c_sleep_sequence)
+                     return -ENOMEM;
+             put_unaligned_le16(freq_khz, am33xx_i2c_sleep_sequence);
+             memcpy(am33xx_i2c_sleep_sequence + 2, prop, sz);

Quoted 8 lines

so, looking at entire code, it seems there is 3 memory space for same data (or 1 original + 2 copy)

1. in DT
2. in am33xx_i2c_[sleep/wake]_sequence
3. in SRAm by call to wkup_m3_copy_data()

why not directly copy to SRAM from DT?

As pointed out above, the firmware reset handler would wipe it out.

Quoted 106 lines

Quoted 106 lines

arch/arm/mach-omap2/pm33xx.c +29 -0

+             i2c_sleep_sequence_sz = sz + 3;
+     }
+
+     prop = of_get_property(dev->of_node, "wake_sequence", &sz);
+     if (prop) {
+             am33xx_i2c_wake_sequence = kzalloc(sz + 3, GFP_KERNEL);
+             if (!am33xx_i2c_wake_sequence) {
+                     ret = -ENOMEM;
+                     goto cleanup_sleep;
+             }
+             put_unaligned_le16(freq_khz, am33xx_i2c_sleep_sequence);
+             memcpy(am33xx_i2c_wake_sequence + 2, prop, sz);
+             i2c_wake_sequence_sz = sz + 3;
+     }
+
+     return 0;
+
+cleanup_sleep:
+     kfree(am33xx_i2c_sleep_sequence);
+     am33xx_i2c_sleep_sequence = NULL;
+     return ret;
+}
+
 int __init am33xx_pm_init(void)
 {
      int ret;

@@ -451,6 +534,12 @@ int __init am33xx_pm_init(void)
              }
      }
 
+     ret = am33xx_setup_sleep_sequence();
+     if (ret) {
+             pr_err("Error fetching I2C sleep/wake sequence\n");
+             goto err;
+     }
+
      (void) clkdm_for_each(omap_pm_clkdms_setup, NULL);
 
      /* CEFUSE domain can be turned off post bootup */

arch/arm/mach-omap2/pm33xx.h +2 -0

index befdd11..d0f08a5 100644
@@ -52,6 +52,8 @@ struct forced_standby_module {
 };
 
 int wkup_m3_copy_code(const u8 *data, size_t size);
+void wkup_m3_reset_data_pos(void);
+int wkup_m3_copy_data(const u8 *data, size_t size);
 int wkup_m3_prepare(void);
 void wkup_m3_register_txev_handler(void (*txev_handler)(void));
 

arch/arm/mach-omap2/wkup_m3.c +28 -1

index 8eaa7f3..a541de9 100644
@@ -35,6 +35,9 @@
 struct wkup_m3_context {
      struct device   *dev;
      void __iomem    *code;
+     void __iomem    *data;
+     void __iomem    *data_end;
+     size_t          data_size;
      void (*txev_handler)(void);
 };
 

@@ -50,6 +53,30 @@ int wkup_m3_copy_code(const u8 *data, size_t size)
      return 0;
 }
 
+/*
+ * This pair of functions allows data to be stuffed into the end of the
+ * CM3 data memory. This is currently used for passing the I2C sleep/wake
+ * sequences to the firmware.
+ */
+
+/* Clear out the pointer for data stored at the end of DMEM */
+void wkup_m3_reset_data_pos(void)
+{
+     wkup_m3->data_end = wkup_m3->data + wkup_m3->data_size;
+}
+
+/*
+ * Store a block of data at the end of DMEM, return the offset within DMEM
+ * that the data is stored at, or -ENOMEM if the data did not fit
+ */
+int wkup_m3_copy_data(const u8 *data, size_t size)
+{
+     if (wkup_m3->data + size > wkup_m3->data_end)
+             return -ENOMEM;
+     wkup_m3->data_end -= size;
+     memcpy_toio(wkup_m3->data_end, data, size);
+     return wkup_m3->data_end - wkup_m3->data;
+}
 
 void wkup_m3_register_txev_handler(void (*txev_handler)(void))
 {

@@ -83,7 +110,7 @@ int wkup_m3_prepare(void)
 static int wkup_m3_probe(struct platform_device *pdev)
 {
      int irq, ret = 0;
-     struct resource *mem;
+     struct resource *umem, *dmem;
 
      pm_runtime_enable(&pdev->dev);
 

Quoted 8 lines

arch/arm/mach-omap2/wkup_m3.c static int wkup_m3_probe(struct platform_device *pdev) · line 125

Quoted 6 lines

arch/arm/mach-omap2/wkup_m3.c +1 -1

@@ -95,14 +122,21 @@ static int wkup_m3_probe(struct platform_device *pdev)
 
      irq = platform_get_irq(pdev, 0);
      if (!irq) {
-             dev_err(wkup_m3->dev, "no irq resource\n");
+             dev_err(&pdev->dev, "no irq resource\n");

unrelated change?. Better to mention this as code cleanup in commit.

Will add a comment to that effect, the underlying error should be fixed in the next suspend/resume patch though.

Quoted 22 lines

Quoted 22 lines

arch/arm/mach-omap2/wkup_m3.c +10 -3

+             ret = -ENXIO;
+             goto err;
+     }
+
+     umem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+     if (!umem) {
+             dev_err(&pdev->dev, "no UMEM resource\n");
              ret = -ENXIO;
              goto err;
      }
 
-     mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-     if (!mem) {
-             dev_err(wkup_m3->dev, "no memory resource\n");
+     dmem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+     if (!dmem) {
+             dev_err(&pdev->dev, "no DMEM resource\n");
              ret = -ENXIO;
              goto err;
      }

@@ -116,12 +150,21 @@ static int wkup_m3_probe(struct platform_device *pdev)
 

Quoted 8 lines

arch/arm/mach-omap2/wkup_m3.c static int wkup_m3_probe(struct platform_device *pdev) · line 155

Quoted 6 lines

arch/arm/mach-omap2/wkup_m3.c +3 -1

      wkup_m3->dev = &pdev->dev;
 
-     wkup_m3->code = devm_request_and_ioremap(wkup_m3->dev, mem);
+     wkup_m3->code = devm_request_and_ioremap(wkup_m3->dev, umem);
+     if (!wkup_m3->code) {
+             dev_err(wkup_m3->dev, "could not ioremap UMEM\n");

why not use "pdev->dev" here?

Either one works

Quoted 8 lines

arch/arm/mach-omap2/wkup_m3.c static int wkup_m3_probe(struct platform_device *pdev) · line 161

Quoted 6 lines

arch/arm/mach-omap2/wkup_m3.c +5 -0

+             ret = -EADDRNOTAVAIL;
+             goto err;
+     }
+
+     wkup_m3->data = devm_request_and_ioremap(wkup_m3->dev, dmem);
      if (!wkup_m3->code) {

I believe this is just a copy/paste error. s/code/data

Doh, thanks!

Quoted 23 lines

arch/arm/mach-omap2/wkup_m3.c static int wkup_m3_probe(struct platform_device *pdev) · line 162

Quoted 2 lines

arch/arm/mach-omap2/wkup_m3.c +1 -1

-             dev_err(wkup_m3->dev, "could not ioremap\n");
+             dev_err(wkup_m3->dev, "could not ioremap DMEM\n");

same as above.

Quoted 9 lines

arch/arm/mach-omap2/wkup_m3.c +2 -1

              ret = -EADDRNOTAVAIL;
              goto err;
      }
+     wkup_m3->data_size = resource_size(dmem);
+     wkup_m3_reset_data_pos();
 
      ret = devm_request_irq(wkup_m3->dev, irq, wkup_m3_txev_handler,
                IRQF_DISABLED, "wkup_m3_txev", NULL);
-- 1.8.3.2 -- To unsubscribe from this list: send the line "unsubscribe

Quoted 3 lines

linux-omap" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 7 lines · Russ Dill

Quoted 6 lines · Gururaja Hebbar

Quoted 3 lines · Russ Dill

Changes since v1:
* Rebased onto new am335x PM branch
* Changed to use 5th param register

[snip]
[snip]

Quoted 8 lines

Quoted 7 lines

arch/arm/mach-omap2/pm33xx.c static void am33xx_m3_state_machine_reset(void) · line 250

Quoted 5 lines

arch/arm/mach-omap2/pm33xx.c +5 -0

+
+     wkup_m3_reset_data_pos();
+     if (am33xx_i2c_sleep_sequence) {
+             pos = wkup_m3_copy_data(am33xx_i2c_sleep_sequence,
+                                             i2c_sleep_sequence_sz);

Why do we need to copy this data (same constant data) on every iteration?

Quoted 2 lines

Because the CM3 firmware is reset after each suspend/resume cycle. The firmware reset handler reinitializes the DMEM.

Well in that why can't the i2c payload be copied to UMEM?

Thanks & regards
Gururaja

Quoted 24 lines · Gururaja Hebbar

Quoted 7 lines · Russ Dill

Quoted 6 lines · Gururaja Hebbar

Quoted 3 lines · Russ Dill

Changes since v1:
* Rebased onto new am335x PM branch
* Changed to use 5th param register

[snip]
[snip]

Quoted 10 lines

Quoted 7 lines

arch/arm/mach-omap2/pm33xx.c static void am33xx_m3_state_machine_reset(void) · line 250

Quoted 5 lines

arch/arm/mach-omap2/pm33xx.c +5 -0

+
+     wkup_m3_reset_data_pos();
+     if (am33xx_i2c_sleep_sequence) {
+             pos = wkup_m3_copy_data(am33xx_i2c_sleep_sequence,
+                                             i2c_sleep_sequence_sz);

Why do we need to copy this data (same constant data) on every iteration?

Because the CM3 firmware is reset after each suspend/resume cycle. The firmware reset handler reinitializes the DMEM.

Quoted 5 lines

Well in that why can't the i2c payload be copied to UMEM?

Thanks & regards
Gururaja

I've given this some thought, and gone back and forth a bit. UMEM is a bit more complicated because the linker decides what should go where.

Also, it may be that in the future, either the PM for am335x or am43xx will be writing different sequences depending on the suspend mode/options.

Is there a specific aspect of copying the data each suspend cycle that you are trying to fix? Is it just that the data could only be copied once? Or is it the latency of the copy?

Thanks!

Changes since v1:
* Rebased onto new am335x PM branch

This adds a sleep and wake sequence to set the VDD core voltage to the OPP50 level, 0.950V. This saves power during suspend. The sequences are specific to the Beaglebone layout and PMIC, the TPS65217. The sequences are written out by the Cortex-M3.

Signed-off-by Russ Dill <[email protected]>

---
 arch/arm/boot/dts/am335x-bone.dts | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

arch/arm/boot/dts/am335x-bone.dts +0 -0

index 444b4ed..3f6528d 100644

arch/arm/boot/dts/am335x-bone.dts +22 -1

+				0x02 0x24 0x0b 0x6d /* Password unlock 1 */
+				0x02 0x24 0x10 0x02 /* Set DCDC3 to 0.95V */
+				0x02 0x24 0x0b 0x6d /* Password unlock 2 */
+				0x02 0x24 0x10 0x02 /* Set DCDC3 to 0.95V */
+				0x02 0x24 0x0b 0x6c /* Password unlock 1 */
+				0x02 0x24 0x11 0x86 /* Apply DCDC changes */
+				0x02 0x24 0x0b 0x6c /* Password unlock 2 */
+				0x02 0x24 0x11 0x86 /* Apply DCDC changes */
+			>;
+
+			/* Set OPP100 (1.10V) for VDD core */
+			wake_sequence = /bits/ 8 <
+				0x02 0x24 0x0b 0x6d /* Password unlock 1 */
+				0x02 0x24 0x10 0x08 /* Set DCDC3 to 1.1V */
+				0x02 0x24 0x0b 0x6d /* Password unlock 2 */
+				0x02 0x24 0x10 0x08 /* Set DCDC3 to 1.1V */
+				0x02 0x24 0x0b 0x6c /* Password unlock 1 */
+				0x02 0x24 0x11 0x86 /* Apply DCDC changes */
+				0x02 0x24 0x0b 0x6c /* Password unlock 2 */
+				0x02 0x24 0x11 0x86 /* Apply DCDC changes */
+			>;
+
 			tps: tps at 24 {
 				reg = <0x24>;
 			};
-
 		};
 	};
 

Quoted 21 lines · Russ Dill

Changes since v1:
* Rebased onto new am335x PM branch

This adds a sleep and wake sequence to set the VDD core voltage to the OPP50 level, 0.950V. This saves power during suspend. The sequences are specific to the Beaglebone layout and PMIC, the TPS65217. The sequences are written out by the Cortex-M3.

Signed-off-by Russ Dill <[email protected]>

---
 arch/arm/boot/dts/am335x-bone.dts | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

arch/arm/boot/dts/am335x-bone.dts +0 -0

index 444b4ed..3f6528d 100644
@@ -127,10 +127,33 @@
                      status = "okay";
                      clock-frequency = <400000>;
 

arch/arm/boot/dts/am335x-bone.dts line 131

Quoted 2 lines

arch/arm/boot/dts/am335x-bone.dts +2 -0

+                     /* Set OPP50 (0.95V) for VDD core */
+                     sleep_sequence = /bits/ 8 <

For user readability, can you mention the PMIC used here as a comment?

Quoted 20 lines

arch/arm/boot/dts/am335x-bone.dts +19 -0

+                             0x02 0x24 0x0b 0x6d /* Password unlock 1 */
+                             0x02 0x24 0x10 0x02 /* Set DCDC3 to 0.95V */
+                             0x02 0x24 0x0b 0x6d /* Password unlock 2 */
+                             0x02 0x24 0x10 0x02 /* Set DCDC3 to 0.95V */
+                             0x02 0x24 0x0b 0x6c /* Password unlock 1 */
+                             0x02 0x24 0x11 0x86 /* Apply DCDC changes */
+                             0x02 0x24 0x0b 0x6c /* Password unlock 2 */
+                             0x02 0x24 0x11 0x86 /* Apply DCDC changes */
+                     >;
+
+                     /* Set OPP100 (1.10V) for VDD core */
+                     wake_sequence = /bits/ 8 <
+                             0x02 0x24 0x0b 0x6d /* Password unlock 1 */
+                             0x02 0x24 0x10 0x08 /* Set DCDC3 to 1.1V */
+                             0x02 0x24 0x0b 0x6d /* Password unlock 2 */
+                             0x02 0x24 0x10 0x08 /* Set DCDC3 to 1.1V */
+                             0x02 0x24 0x0b 0x6c /* Password unlock 1 */
+                             0x02 0x24 0x11 0x86 /* Apply DCDC changes */
+                             0x02 0x24 0x0b 0x6c /* Password unlock 2 */

arch/arm/boot/dts/am335x-bone.dts line 159

Quoted 10 lines

arch/arm/boot/dts/am335x-bone.dts +3 -1

+                             0x02 0x24 0x11 0x86 /* Apply DCDC changes */
+                     >;
+
                      tps: tps at 24 {
                              reg = <0x24>;
                      };
-
              };
      };
 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

The purpose and method of executing these sequences is left up to each platform. In the case of the am33xx, the CM3 firmware writes out the simple I2C sequences.

Each sequence is a series of I2C write commands. The first byte is the length of the write, the second byte the I2C device to address, and the following bytes are the message.

        /* Set OPP100 (1.10V) for VDD core */
        wake_sequence = /bits/ 8 <
                0x02 0x2d 0x25 0x2b /* Set VDD2 to 1.1V */
        >;

        tps: tps at 2d {
                reg = <0x2d>;
        };

In the above example, the sequence "0x25 0x1f" is written to the I2C device at address 0x2d (the TPS65910 PMIC). The PMIC interprets that as a write to a register at address 0x25.

I'd really like to get some feedback on the devicetree bindings.

Quoted 20 lines · Russ Dill

The purpose and method of executing these sequences is left up to each platform. In the case of the am33xx, the CM3 firmware writes out the simple I2C sequences.

Each sequence is a series of I2C write commands. The first byte is the length of the write, the second byte the I2C device to address, and the following bytes are the message.

        /* Set OPP100 (1.10V) for VDD core */
        wake_sequence = /bits/ 8 <
                0x02 0x2d 0x25 0x2b /* Set VDD2 to 1.1V */
        >;

        tps: tps at 2d {
                reg = <0x2d>;
        };

In the above example, the sequence "0x25 0x1f" is written to the I2C device at address 0x2d (the TPS65910 PMIC). The PMIC interprets that as a write to a register at address 0x25.

Quoted 1 line

I'd really like to get some feedback on the devicetree bindings.

Shouldn't the TPS driver know how to generate this sequence? It seems fragile to do voltage adjustments behind the back of the regulator framework and the TPS driver. The wake-sequence values should match the (in-memory) regulator configuration on resume (which may have been changed by DVFS).

The CM3 driver needs to figure out where the core regulator is connected using using either DT or the regulator framework and ask the TPS (via a new interface) for register writes for sleep/wake sequences. Then those sequences will actually match the correct voltages configured using DT/DVFS.

Regards,
Jan
--

Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 5 lines

The CM3 driver needs to figure out where the core regulator is connected using using either DT or the regulator framework and ask the TPS (via a new interface) for register writes for sleep/wake sequences. Then those sequences will actually match the correct voltages configured using DT/DVFS.

That seems like it'd add a lot of complexity to the regulator subsystem, as well as all the PMIC and other I2C regulators that any users of these device tree properties may end up using for not a lot of gain. There would be two separate code paths for any used I2C regulator or PMIC for setting voltages.

Quoted 18 lines · Jan L?bbe

Quoted 16 lines · Russ Dill

The purpose and method of executing these sequences is left up to each platform. In the case of the am33xx, the CM3 firmware writes out the simple I2C sequences.

Each sequence is a series of I2C write commands. The first byte is the length of the write, the second byte the I2C device to address, and the following bytes are the message.

        /* Set OPP100 (1.10V) for VDD core */
        wake_sequence = /bits/ 8 <
                0x02 0x2d 0x25 0x2b /* Set VDD2 to 1.1V */
        >;

        tps: tps at 2d {
                reg = <0x2d>;
        };

Quoted 5 lines

Quoted 5 lines

In the above example, the sequence "0x25 0x1f" is written to the I2C device at address 0x2d (the TPS65910 PMIC). The PMIC interprets that as a write to a register at address 0x25.

I'd really like to get some feedback on the devicetree bindings.

Quoted 5 lines

Shouldn't the TPS driver know how to generate this sequence? It seems fragile to do voltage adjustments behind the back of the regulator framework and the TPS driver. The wake-sequence values should match the (in-memory) regulator configuration on resume (which may have been changed by DVFS).

The sequence is both PMIC specific and board specific. Additionally, the PMICs used aren't am335x specific. It would be nice to have the regulator framework and the driver write all this out, but the sequence is written out by the Cortex-M3 processor running some PM firmware. Even if the code was changed to run on the A8, it'd have to run from a small piece of SRAM.

As far as DVFS, I'm not aware of any DVFS implementations that muck with VDD CORE.

Quoted 5 lines

The CM3 driver needs to figure out where the core regulator is connected using using either DT or the regulator framework and ask the TPS (via a new interface) for register writes for sleep/wake sequences. Then those sequences will actually match the correct voltages configured using DT/DVFS.

That seems like it'd add a lot of complexity to the regulator subsystem, as well as all the PMIC and other I2C regulators that any users of these device tree properties may end up using for not a lot of gain. There would be two separate code paths for any used I2C regulator or PMIC for setting voltages.

[+Mark Brown for regulator suspend sequence ideas]

Quoted 2 lines · Russ Dill

Quoted 1 line · Jan L?bbe

On Tue, 2013-08-13 at 15:20 -0700, Russ Dill wrote:

Quoted 6 lines

Quoted 5 lines

Shouldn't the TPS driver know how to generate this sequence? It seems fragile to do voltage adjustments behind the back of the regulator framework and the TPS driver. The wake-sequence values should match the (in-memory) regulator configuration on resume (which may have been changed by DVFS).

Quoted 6 lines

The sequence is both PMIC specific and board specific. Additionally, the PMICs used aren't am335x specific. It would be nice to have the regulator framework and the driver write all this out, but the sequence is written out by the Cortex-M3 processor running some PM firmware. Even if the code was changed to run on the A8, it'd have to run from a small piece of SRAM.

So, why/how was the decision made to use the M3 instead of the MPU running from SRAM?

As a firmware minimalist, I obviously prefer to do this from the MPU side. But also, because the M3 is reset every suspend sequence, this becomes rather heavy to do from the M3.

Currently voltage scaling is only being proposed for suspend in this series, but in theory it's possible from idle as well. Doing this from the MPU/SRAM seems much better suited for idle.

Quoted 9 lines

As far as DVFS, I'm not aware of any DVFS implementations that muck with VDD CORE.

Quoted 5 lines

The CM3 driver needs to figure out where the core regulator is connected using using either DT or the regulator framework and ask the TPS (via a new interface) for register writes for sleep/wake sequences. Then those sequences will actually match the correct voltages configured using DT/DVFS.

Quoted 5 lines

That seems like it'd add a lot of complexity to the regulator subsystem, as well as all the PMIC and other I2C regulators that any users of these device tree properties may end up using for not a lot of gain. There would be two separate code paths for any used I2C regulator or PMIC for setting voltages.

Added Mark for his thoughts on this, but seems like complexity that the regulator framework might need to grow anyways.

The framework already has a concept of suspend voltage, suspend mode etc. Maybe it needs some generalizing so low-level platform code could query the framework for the sequence so it can be done late in platform idle/suspend paths. Especially for regmap drivers, this seems feasible.

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 4 lines · Kevin Hilman

The framework already has a concept of suspend voltage, suspend mode etc. Maybe it needs some generalizing so low-level platform code could query the framework for the sequence so it can be done late in platform idle/suspend paths. Especially for regmap drivers, this seems feasible.

Yes, my main hesitation for going down this path is possible lack of support for such an interface upstream.

Quoted 6 lines · Russ Dill

Quoted 4 lines · Kevin Hilman

The framework already has a concept of suspend voltage, suspend mode etc. Maybe it needs some generalizing so low-level platform code could query the framework for the sequence so it can be done late in platform idle/suspend paths. Especially for regmap drivers, this seems feasible.

Quoted 2 lines

Yes, my main hesitation for going down this path is possible lack of support for such an interface upstream.

Someone is going to have to walk me through the context for me to fully understand what this is all about - what's the problem?

Generally for suspend if the sequencing is being done by the processor it's been handled by the drivers for the components in question. For sequencing beyond that you're into hardware features which aren't all that regular sadly and often aren't controllable either. It sounds like this is the latter case but the thing that manages some of the late power down stages does so using a firmware we can rewrite?

Quoted 2 lines · Mark Brown

Someone is going to have to walk me through the context for me to fully understand what this is all about - what's the problem?

The basic problem is how to have low-level platform code (or possibly firmware) send commands to a regulator to scale voltage. This has to happen *very* late in the suspend process, so having the drivers do it themselves is not feasible.

The proposal in this series is to define the i2c commands sequence to be sent to the regulator in the i2c node of the DT. The platform-specific code then reads the sequence from the DT and sends the commands (or in in the case of the current series, passes the sequence to some firmware on a microcontroller which does the sequence.)

Quoted 4 lines · Kevin Hilman

Quoted 2 lines · Mark Brown

Someone is going to have to walk me through the context for me to fully understand what this is all about - what's the problem?

Quoted 4 lines

The basic problem is how to have low-level platform code (or possibly firmware) send commands to a regulator to scale voltage. This has to happen *very* late in the suspend process, so having the drivers do it themselves is not feasible.

Why does it have to happen this late and are the sequences definitely fixed ones not ones that could depend on the system state at the time we suspend? It'd help to know what exactly is being controlled here...

Quoted 6 lines

The proposal in this series is to define the i2c commands sequence to be sent to the regulator in the i2c node of the DT. The platform-specific code then reads the sequence from the DT and sends the commands (or in in the case of the current series, passes the sequence to some firmware on a microcontroller which does the sequence.)

Surely specifying things in terms of the actual sequence would be better than trying to specify the I2C commands if you want this to be done from Linux? If the firmware has to cope directly then this would require the firmware to understand what it's doing of course.

;

;

;

;

Quoted 3 lines · Mark Brown

Why does it have to happen this late and are the sequences definitely fixed ones not ones that could depend on the system state at the time we suspend? It'd help to know what exactly is being controlled here...

On all am335x platforms, the lower operating point for core voltage cannot be reached without first disabling the DDR controller, and programming it into a lower power mode. For DDR3 platforms, no such lower power mode is available and the lower operating point for core voltage can only be reached with the memory controller disabled.

It certainly is possible that some bizarre I2C regulator may mix in regulator voltage and some other state into one I2C register. In the case of such a platform, setting the lower operating point would not be supported.

Quoted 4 lines

Surely specifying things in terms of the actual sequence would be better than trying to specify the I2C commands if you want this to be done from Linux? If the firmware has to cope directly then this would require the firmware to understand what it's doing of course.

I'm not sure what you mean by "actual sequence". Maybe if I show you a couple examples, it will be more clear:

Quoted 5 lines · Russ Dill

Quoted 3 lines · Mark Brown

Why does it have to happen this late and are the sequences definitely fixed ones not ones that could depend on the system state at the time we suspend? It'd help to know what exactly is being controlled here...

Quoted 5 lines

On all am335x platforms, the lower operating point for core voltage cannot be reached without first disabling the DDR controller, and programming it into a lower power mode. For DDR3 platforms, no such lower power mode is available and the lower operating point for core voltage can only be reached with the memory controller disabled.

So this is done from cpuidle rather than system suspend.

Quoted 4 lines

It certainly is possible that some bizarre I2C regulator may mix in regulator voltage and some other state into one I2C register. In the case of such a platform, setting the lower operating point would not be supported.

This isn't particularly bizzare, there is no technical reason why a register in the PMIC has to be given over completely to setting the voltage and indeed there are PMICs in mainline right now which don't do that - if you've got 16 bit registers it's probably not going to take the entire 16 bits to encode the voltage range.

Quoted 4 lines

Quoted 4 lines

Surely specifying things in terms of the actual sequence would be better than trying to specify the I2C commands if you want this to be done from Linux? If the firmware has to cope directly then this would require the firmware to understand what it's doing of course.

Quoted 2 lines

I'm not sure what you mean by "actual sequence". Maybe if I show you a couple examples, it will be more clear:

I mean describe the intended sequence of events in the system rather than the raw register commands to accomplish them.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 1 line · Mark Brown

So this is done from cpuidle rather than system suspend.

It is done for system suspend. It isn't possible to do this in a cpuidle use case as the memory controller would need to come out of idle automatically to service DMA requests.

Quoted 2 lines

I mean describe the intended sequence of events in the system rather than the raw register commands to accomplish them.

I'm still not sure what you mean.

Quoted 3 lines · Russ Dill

Quoted 1 line · Mark Brown

So this is done from cpuidle rather than system suspend.

Quoted 3 lines

It is done for system suspend. It isn't possible to do this in a cpuidle use case as the memory controller would need to come out of idle automatically to service DMA requests.

OK, so it's all part of the core power down sequence for the SoC? I'm not sure that makes a big difference but it at least means that there are fewer runtime interactions to worry about.

Quoted 4 lines

Quoted 2 lines

I mean describe the intended sequence of events in the system rather than the raw register commands to accomplish them.

I'm still not sure what you mean.

Say "set voltage X" or even "regulator X supplies Y" not "send this bytestream to the device".

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 9 lines · Russ Dill

Quoted 4 lines · Kevin Hilman

The framework already has a concept of suspend voltage, suspend mode etc. Maybe it needs some generalizing so low-level platform code could query the framework for the sequence so it can be done late in platform idle/suspend paths. Especially for regmap drivers, this seems feasible.

Yes, my main hesitation for going down this path is possible lack of support for such an interface upstream.

Someone is going to have to walk me through the context for me to fully understand what this is all about - what's the problem?

Generally for suspend if the sequencing is being done by the processor it's been handled by the drivers for the components in question. For sequencing beyond that you're into hardware features which aren't all that regular sadly and often aren't controllable either. It sounds like this is the latter case but the thing that manages some of the late power down stages does so using a firmware we can rewrite?

The path I'm taking in this patchset is to just put the board specific I2C sequences necessary for the CM3 coprocessor to write out into the devicetree. I've made it as generic as I can so that other platforms with similar issues can reuse the bindings. Because of the limitations of the firmware size and the desire not to have some sort of byte code, this is a write only sequence.

Kevin Hilman is suggesting some way to query the regulator framework for this sequence. There would be some call to the regulator framework that would return a series of I2C commands that would be necessary to program a regulator to a specific voltage. It would be called twice, once for the suspend voltage, once for the resume voltage. The regulator framework would then call into the driver that sets that voltage, and by some method query the driver for the necessary I2C sequence, either by some special new call, or some regmap magic.

Quoted 6 lines · Russ Dill

The path I'm taking in this patchset is to just put the board specific I2C sequences necessary for the CM3 coprocessor to write out into the devicetree. I've made it as generic as I can so that other platforms with similar issues can reuse the bindings. Because of the limitations of the firmware size and the desire not to have some sort of byte code, this is a write only sequence.

Making it write only seems to be a mistake - like I said in my other mail I'd expect you'd want bitfield updates here. The read and modify could be done earlier by Linux though.

Quoted 8 lines

Kevin Hilman is suggesting some way to query the regulator framework for this sequence. There would be some call to the regulator framework that would return a series of I2C commands that would be necessary to program a regulator to a specific voltage. It would be called twice, once for the suspend voltage, once for the resume voltage. The regulator framework would then call into the driver that sets that voltage, and by some method query the driver for the necessary I2C sequence, either by some special new call, or some regmap magic.

If it's just setting a single voltage then extracting the bitfield to update shouldn't be too hard for regmap based regulators. Off the top of my head I'd expect either the driver for the M3 or the cpuidle driver that talks to it to be a consumer of the regulator and then go from there.

Just putting the sequence directly into the device tree doesn't feel like the right abstraction.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 3 lines · Mark Brown

Making it write only seems to be a mistake - like I said in my other mail I'd expect you'd want bitfield updates here. The read and modify could be done earlier by Linux though.

Updating bitfields in memory is pretty basic, but with I2C, each device has its own register sizes and methods of accessing registers. The first byte of an I2C sequence being a register address is pretty common though. You'd need a method of defining in the device tree piece how to modify bitfields.

Quoted 5 lines

If it's just setting a single voltage then extracting the bitfield to update shouldn't be too hard for regmap based regulators. Off the top of my head I'd expect either the driver for the M3 or the cpuidle driver that talks to it to be a consumer of the regulator and then go from there.

It'd be a list of bitfields. So are you suggesting a regulator_ops call that would return a list of bitfield updates along with an I2C controller, an I2C device address, I2C register addressing scheme (8 bit, 16 bit, or other) and the info for each bitfield (register size)? And then each regulator driver would be updated as needed with this code. Would this be a way forward? Would regmap actually tie into this at all?

This is VDD core, so its not related to cpuidle or the M3.

Quoted 11 lines · Russ Dill

Quoted 3 lines · Mark Brown

Making it write only seems to be a mistake - like I said in my other mail I'd expect you'd want bitfield updates here. The read and modify could be done earlier by Linux though.

Updating bitfields in memory is pretty basic, but with I2C, each device has its own register sizes and methods of accessing registers. The first byte of an I2C sequence being a register address is pretty common though. You'd need a method of defining in the device tree piece how to modify bitfields.

I'd not assume byte based addressing for a PMIC, it's very common but not universal. The difficulties here assume putting this in the device tree as explicit register I/O - I do tend to agree with Kevin that this doesn't seem like the right abstraction.

Quoted 6 lines

Quoted 5 lines

If it's just setting a single voltage then extracting the bitfield to update shouldn't be too hard for regmap based regulators. Off the top of my head I'd expect either the driver for the M3 or the cpuidle driver that talks to it to be a consumer of the regulator and then go from there.

Quoted 7 lines

It'd be a list of bitfields. So are you suggesting a regulator_ops call that would return a list of bitfield updates along with an I2C controller, an I2C device address, I2C register addressing scheme (8 bit, 16 bit, or other) and the info for each bitfield (register size)? And then each regulator driver would be updated as needed with this code. Would this be a way forward? Would regmap actually tie into this at all?

I was thinking about the majority of regulators where setting a voltage would be a single bitfield update (plus ramp delay, which is going to need to be accounted for in the power on case). If we need to do longer sequences things get a bit more tricky in terms of interface but it seems doable.

regmap would tie in in that it already has a way of describing the format for interactions with the device so we could just reuse the same description, and with some work we can probably reuse the code that generates the bitstreams for sending to the devices too (though I don't immediately see a nice way of doing that).

Not sure if this is worth the effort or not though, but then there's the whole DT as ABI thing to worry about...

Quoted 1 line

This is VDD core, so its not related to cpuidle or the M3.

It's related to the M3 if the M3 is managing it; you can have multiple consumers for a regulator so it doesn't need to be one or the other.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 5 lines · Mark Brown

I was thinking about the majority of regulators where setting a voltage would be a single bitfield update (plus ramp delay, which is going to need to be accounted for in the power on case). If we need to do longer sequences things get a bit more tricky in terms of interface but it seems doable.

The sequence for TPS65217 on beaglebone is rather long, it's 8 register writes. It has to write the new value to the DCDC3 register and the apply bit to the DCDC register, but both registers are password locked registers. The password sequence is to write the password register, then the register, then the password register again, then the register again.

Quoted 6 lines · Kevin Hilman

[+Mark Brown for regulator suspend sequence ideas]

Quoted 2 lines · Russ Dill

Quoted 1 line · Jan L?bbe

On Tue, 2013-08-13 at 15:20 -0700, Russ Dill wrote:

[snip]

Quoted 13 lines

Quoted 6 lines

Quoted 5 lines

Shouldn't the TPS driver know how to generate this sequence? It seems fragile to do voltage adjustments behind the back of the regulator framework and the TPS driver. The wake-sequence values should match the (in-memory) regulator configuration on resume (which may have been changed by DVFS).

Quoted 6 lines

The sequence is both PMIC specific and board specific. Additionally, the PMICs used aren't am335x specific. It would be nice to have the regulator framework and the driver write all this out, but the sequence is written out by the Cortex-M3 processor running some PM firmware. Even if the code was changed to run on the A8, it'd have to run from a small piece of SRAM.

Quoted 6 lines

So, why/how was the decision made to use the M3 instead of the MPU running from SRAM?

As a firmware minimalist, I obviously prefer to do this from the MPU side. But also, because the M3 is reset every suspend sequence, this becomes rather heavy to do from the M3.

After the feedback Vaibhav Bedia received on v2 of his suspend/resume patchset for am335x, he decided to move many of the operations from sleep33xx.S into the M3 firmware.

See the commit message here:
http://arago-project.org/git/projects/?p=am33x-cm3.git;a=commit;h=a972ce2f6

I need to wait until after the PLLs are put into bypass, which is now done in the M3 firmware. It also ends up being a lot easier to write the I2C writer code there in C rather than in assembly in sleep33xx.S.

Quoted 3 lines

Currently voltage scaling is only being proposed for suspend in this series, but in theory it's possible from idle as well. Doing this from the MPU/SRAM seems much better suited for idle.

The M3 firmware will also handle any cpuidle modes deeper than just putting SDRAM into self refresh. This is actually the only way of doing things like turning the MPU domain off on am335x.

Quoted 28 lines · Russ Dill

Quoted 6 lines · Kevin Hilman

[+Mark Brown for regulator suspend sequence ideas]

Quoted 2 lines · Russ Dill

Quoted 1 line · Jan L?bbe

On Tue, 2013-08-13 at 15:20 -0700, Russ Dill wrote:

[snip]

Quoted 13 lines

Quoted 6 lines

Quoted 5 lines

Shouldn't the TPS driver know how to generate this sequence? It seems fragile to do voltage adjustments behind the back of the regulator framework and the TPS driver. The wake-sequence values should match the (in-memory) regulator configuration on resume (which may have been changed by DVFS).

Quoted 6 lines

The sequence is both PMIC specific and board specific. Additionally, the PMICs used aren't am335x specific. It would be nice to have the regulator framework and the driver write all this out, but the sequence is written out by the Cortex-M3 processor running some PM firmware. Even if the code was changed to run on the A8, it'd have to run from a small piece of SRAM.

Quoted 6 lines

So, why/how was the decision made to use the M3 instead of the MPU running from SRAM?

As a firmware minimalist, I obviously prefer to do this from the MPU side. But also, because the M3 is reset every suspend sequence, this becomes rather heavy to do from the M3.

Quoted 6 lines

After the feedback Vaibhav Bedia received on v2 of his suspend/resume patchset for am335x, he decided to move many of the operations from sleep33xx.S into the M3 firmware.

See the commit message here:
http://arago-project.org/git/projects/?p=am33x-cm3.git;a=commit;h=a972ce2f6

Was this feedback on the public lists? That patch has never been posted to linux-omap AFAICT.

Quoted 3 lines

I need to wait until after the PLLs are put into bypass, which is now done in the M3 firmware. It also ends up being a lot easier to write the I2C writer code there in C rather than in assembly in sleep33xx.S.

hmm, (carefully) written functions in C can still be copied to SRAM. I dont' see that as an obstacle.

Quoted 7 lines

Quoted 3 lines

Currently voltage scaling is only being proposed for suspend in this series, but in theory it's possible from idle as well. Doing this from the MPU/SRAM seems much better suited for idle.

The M3 firmware will also handle any cpuidle modes deeper than just putting SDRAM into self refresh. This is actually the only way of doing things like turning the MPU domain off on am335x.

Yes, it will have to handle the MPU/interconnect off parts but other than that, that's the only thing it *has* to do (well, and handle wakeup from the deep state.) The rest of the stuff being piled into the M3 is a result of software/firmware design decisions AFAICT. As I predicted when I first saw this SoC design, the firmware is getting bigger and bigger. Initially it was argued that it would be tiny, and only handle the things it had to do. Now it's growing due to "convenience". IMO, this is a bad trend, and one that will make this code more and more difficult to maintain upstream (assuming that's a goal.)

Quoted 55 lines · Kevin Hilman

Quoted 28 lines · Russ Dill

Quoted 6 lines · Kevin Hilman

[+Mark Brown for regulator suspend sequence ideas]

Quoted 2 lines · Russ Dill

Quoted 1 line · Jan L?bbe

On Tue, 2013-08-13 at 15:20 -0700, Russ Dill wrote:

[snip]

Quoted 13 lines

Quoted 6 lines

Quoted 5 lines

Shouldn't the TPS driver know how to generate this sequence? It seems fragile to do voltage adjustments behind the back of the regulator framework and the TPS driver. The wake-sequence values should match the (in-memory) regulator configuration on resume (which may have been changed by DVFS).

Quoted 6 lines

The sequence is both PMIC specific and board specific. Additionally, the PMICs used aren't am335x specific. It would be nice to have the regulator framework and the driver write all this out, but the sequence is written out by the Cortex-M3 processor running some PM firmware. Even if the code was changed to run on the A8, it'd have to run from a small piece of SRAM.

Quoted 6 lines

So, why/how was the decision made to use the M3 instead of the MPU running from SRAM?

As a firmware minimalist, I obviously prefer to do this from the MPU side. But also, because the M3 is reset every suspend sequence, this becomes rather heavy to do from the M3.

Quoted 6 lines

After the feedback Vaibhav Bedia received on v2 of his suspend/resume patchset for am335x, he decided to move many of the operations from sleep33xx.S into the M3 firmware.

See the commit message here:
http://arago-project.org/git/projects/?p=am33x-cm3.git;a=commit;h=a972ce2f6

Was this feedback on the public lists? That patch has never been posted to linux-omap AFAICT.

Quoted 3 lines

I need to wait until after the PLLs are put into bypass, which is now done in the M3 firmware. It also ends up being a lot easier to write the I2C writer code there in C rather than in assembly in sleep33xx.S.

hmm, (carefully) written functions in C can still be copied to SRAM. I dont' see that as an obstacle.

Quoted 7 lines

Quoted 3 lines

Currently voltage scaling is only being proposed for suspend in this series, but in theory it's possible from idle as well. Doing this from the MPU/SRAM seems much better suited for idle.

The M3 firmware will also handle any cpuidle modes deeper than just putting SDRAM into self refresh. This is actually the only way of doing things like turning the MPU domain off on am335x.

Quoted 9 lines

Yes, it will have to handle the MPU/interconnect off parts but other than that, that's the only thing it *has* to do (well, and handle wakeup from the deep state.) The rest of the stuff being piled into the M3 is a result of software/firmware design decisions AFAICT. As I predicted when I first saw this SoC design, the firmware is getting bigger and bigger. Initially it was argued that it would be tiny, and only handle the things it had to do. Now it's growing due to "convenience". IMO, this is a bad trend, and one that will make this code more and more difficult to maintain upstream (assuming that's a goal.)

Do you mean upstream as in the firmware upstream, or upstream as in the kernel upstream? Upstream kernel is really easy to maintain because sleep33xx.S just puts the SDRAM into self-refresh.

The code to perform these transitions is going to exist, either in kernel or in firmware. If you are looking for this to be maintainable C code that is copied into SRAM, it will need to be built much like a firmware, with it's elf sections being copied into SRAM properly.

I'm not sure how much it complicates things, but the code needs to be able to run with the MMU on and MMU off. The C code would be a minimalized duplication of much of what is already in mach-omap2. Because you are proposing to split this up between A8 and M3, much of that code would then be duplicated again within the M3 firmware.

And don't forget that am335x is just the first platform with such a PM scheme.

Because the M3 firmware already has to manage power domains, hwmods, plls, and clockdomains, adding or removing which ones it handles doesn't really change the size or complexity of the firmware.

In fact, because so much of the code is common code, moving this into kernel would just mean making two copies of the firmware with different steps to be run, one for the A8 SRAM, one for the M3.

Quoted 43 lines

Quoted 33 lines

Quoted 8 lines

Quoted 7 lines

So, why/how was the decision made to use the M3 instead of the MPU running from SRAM?

As a firmware minimalist, I obviously prefer to do this from the MPU side. But also, because the M3 is reset every suspend sequence, this becomes rather heavy to do from the M3.

Quoted 6 lines

After the feedback Vaibhav Bedia received on v2 of his suspend/resume patchset for am335x, he decided to move many of the operations from sleep33xx.S into the M3 firmware.

See the commit message here:
http://arago-project.org/git/projects/?p=am33x-cm3.git;a=commit;h=a972ce2f6

Was this feedback on the public lists? That patch has never been posted to linux-omap AFAICT.

Quoted 3 lines

I need to wait until after the PLLs are put into bypass, which is now done in the M3 firmware. It also ends up being a lot easier to write the I2C writer code there in C rather than in assembly in sleep33xx.S.

hmm, (carefully) written functions in C can still be copied to SRAM. I dont' see that as an obstacle.

Quoted 7 lines

Quoted 3 lines

Currently voltage scaling is only being proposed for suspend in this series, but in theory it's possible from idle as well. Doing this from the MPU/SRAM seems much better suited for idle.

The M3 firmware will also handle any cpuidle modes deeper than just putting SDRAM into self refresh. This is actually the only way of doing things like turning the MPU domain off on am335x.

Quoted 9 lines

Yes, it will have to handle the MPU/interconnect off parts but other than that, that's the only thing it *has* to do (well, and handle wakeup from the deep state.) The rest of the stuff being piled into the M3 is a result of software/firmware design decisions AFAICT. As I predicted when I first saw this SoC design, the firmware is getting bigger and bigger. Initially it was argued that it would be tiny, and only handle the things it had to do. Now it's growing due to "convenience". IMO, this is a bad trend, and one that will make this code more and more difficult to maintain upstream (assuming that's a goal.)

Quoted 3 lines

Do you mean upstream as in the firmware upstream, or upstream as in the kernel upstream? Upstream kernel is really easy to maintain because sleep33xx.S just puts the SDRAM into self-refresh.

I mean making the kernel "simple" at the price of more complicated (and much less reviewed) firmware code is a bad trend. I understand the desire to move this stuff into firmware and bypass the kernel review process, but I predict it will backfire.

Quoted 4 lines

The code to perform these transitions is going to exist, either in kernel or in firmware. If you are looking for this to be maintainable C code that is copied into SRAM, it will need to be built much like a firmware, with it's elf sections being copied into SRAM properly.

You're making it too complicated. No need for ELF, etc.

So the argument for moving much of the stuff from the kernel (in the patch you referenced above) was that the "assembly code is quite big and folks have found it hard to review and fixup issues in this dense piece of code." (interestingly, it was moved to firmware where it will see even less review, but that's off topic.)

My idea was that if reviewing assembly is the issue, why not carefully write a self-contained C function that can be copied to SRAM. It might need a little stub in SRAM to setup the stack etc., but it should be quite doable. That eliminates the problem cited as the reason for the move to M3.

Quoted 8 lines

I'm not sure how much it complicates things, but the code needs to be able to run with the MMU on and MMU off. The C code would be a minimalized duplication of much of what is already in mach-omap2. Because you are proposing to split this up between A8 and M3, much of that code would then be duplicated again within the M3 firmware.

And don't forget that am335x is just the first platform with such a PM scheme.

IMO, all the more reason to handle it in Linux, not in platform-specific firmware.

Quoted 3 lines

Because the M3 firmware already has to manage power domains, hwmods, plls, and clockdomains, adding or removing which ones it handles doesn't really change the size or complexity of the firmware.

No, but it significantly complicates its interaction with Linux, which also has to know about all these things already. IMO, as I've stated from the very beginning of this SoC, there should be a very clean split. M3 should only handle the minimum, what the MPU simply cannot do (MPU/interconnect off, wakeup). MPU/Linux should handle the rest.

Quoted 3 lines

In fact, because so much of the code is common code, moving this into kernel would just mean making two copies of the firmware with different steps to be run, one for the A8 SRAM, one for the M3.

Maybe I'm getting confused, but the more you talk about the linux and the firmware doing the same code, the more I think the firmware is (trying) to do too much. If this is going to be understandable (and maintainable), there needs to be a clean split of roles between the MPU and the M3.

On Thu, Aug 29, 2013 at 3:11 PM, Kevin Hilman <[email protected]> wrote:
[...]

First of all, apologies for jumping in here. Now that i am not actively involved in AM335x/AM437x stuff i wasn't actively following all the discussion here.

Looking at the lists now i just wanted to mention a few things so that we can agree on the approach to be taken here.

Quoted 96 lines

Quoted 43 lines

Quoted 33 lines

Quoted 8 lines

Quoted 7 lines

So, why/how was the decision made to use the M3 instead of the MPU running from SRAM?

As a firmware minimalist, I obviously prefer to do this from the MPU side. But also, because the M3 is reset every suspend sequence, this becomes rather heavy to do from the M3.

Quoted 6 lines

After the feedback Vaibhav Bedia received on v2 of his suspend/resume patchset for am335x, he decided to move many of the operations from sleep33xx.S into the M3 firmware.

See the commit message here:
http://arago-project.org/git/projects/?p=am33x-cm3.git;a=commit;h=a972ce2f6

Was this feedback on the public lists? That patch has never been posted to linux-omap AFAICT.

Quoted 3 lines

I need to wait until after the PLLs are put into bypass, which is now done in the M3 firmware. It also ends up being a lot easier to write the I2C writer code there in C rather than in assembly in sleep33xx.S.

hmm, (carefully) written functions in C can still be copied to SRAM. I dont' see that as an obstacle.

Quoted 7 lines

Quoted 3 lines

Currently voltage scaling is only being proposed for suspend in this series, but in theory it's possible from idle as well. Doing this from the MPU/SRAM seems much better suited for idle.

The M3 firmware will also handle any cpuidle modes deeper than just putting SDRAM into self refresh. This is actually the only way of doing things like turning the MPU domain off on am335x.

Quoted 9 lines

Yes, it will have to handle the MPU/interconnect off parts but other than that, that's the only thing it *has* to do (well, and handle wakeup from the deep state.) The rest of the stuff being piled into the M3 is a result of software/firmware design decisions AFAICT. As I predicted when I first saw this SoC design, the firmware is getting bigger and bigger. Initially it was argued that it would be tiny, and only handle the things it had to do. Now it's growing due to "convenience". IMO, this is a bad trend, and one that will make this code more and more difficult to maintain upstream (assuming that's a goal.)

Quoted 3 lines

Do you mean upstream as in the firmware upstream, or upstream as in the kernel upstream? Upstream kernel is really easy to maintain because sleep33xx.S just puts the SDRAM into self-refresh.

I mean making the kernel "simple" at the price of more complicated (and much less reviewed) firmware code is a bad trend. I understand the desire to move this stuff into firmware and bypass the kernel review process, but I predict it will backfire.

Quoted 4 lines

The code to perform these transitions is going to exist, either in kernel or in firmware. If you are looking for this to be maintainable C code that is copied into SRAM, it will need to be built much like a firmware, with it's elf sections being copied into SRAM properly.

You're making it too complicated. No need for ELF, etc.

So the argument for moving much of the stuff from the kernel (in the patch you referenced above) was that the "assembly code is quite big and folks have found it hard to review and fixup issues in this dense piece of code." (interestingly, it was moved to firmware where it will see even less review, but that's off topic.)

My idea was that if reviewing assembly is the issue, why not carefully write a self-contained C function that can be copied to SRAM. It might need a little stub in SRAM to setup the stack etc., but it should be quite doable. That eliminates the problem cited as the reason for the move to M3.

Quoted 8 lines

I'm not sure how much it complicates things, but the code needs to be able to run with the MMU on and MMU off. The C code would be a minimalized duplication of much of what is already in mach-omap2. Because you are proposing to split this up between A8 and M3, much of that code would then be duplicated again within the M3 firmware.

And don't forget that am335x is just the first platform with such a PM scheme.

IMO, all the more reason to handle it in Linux, not in platform-specific firmware.

Quoted 3 lines

Because the M3 firmware already has to manage power domains, hwmods, plls, and clockdomains, adding or removing which ones it handles doesn't really change the size or complexity of the firmware.

No, but it significantly complicates its interaction with Linux, which also has to know about all these things already. IMO, as I've stated from the very beginning of this SoC, there should be a very clean split. M3 should only handle the minimum, what the MPU simply cannot do (MPU/interconnect off, wakeup). MPU/Linux should handle the rest.

Quoted 3 lines

In fact, because so much of the code is common code, moving this into kernel would just mean making two copies of the firmware with different steps to be run, one for the A8 SRAM, one for the M3.

Quoted 6 lines

Maybe I'm getting confused, but the more you talk about the linux and the firmware doing the same code, the more I think the firmware is (trying) to do too much. If this is going to be understandable (and maintainable), there needs to be a clean split of roles between the MPU and the M3.

In the past i tried to keep the firmware as minimal as possible and pushed back all efforts to push unnecessary stuff in there. Like Kevin, i too was of the opinion that we need to do whatever's possible in the kernel.

Sadly, things just got a point where it made much more sense (technically as well as non-technically) to just put the low level stuff in the M3.

1. In addition to the Linux support, we have non-OS based code for AM335x (and AM437x) which in the past just had to duplicate whatever was done in the Linux. Being a different codebase the non-OS guys have their own set of challenges and we had to at times sit through and debug issues in one codebase while the other was working fine. Ideally these things should never come up but sadly they do and we need to solve them.

We also have people trying to implement this thing on different sort of OS environments and they also end up debugging the same set of issues.

With only a handful of folks able to spend time debugging issues 'do everything in Linux and expect others to copy it' model doesn't scale up. Keeping things in the
firmware with the code available online helps do away with the 'developer scalability'
problem and if one looks at things differently enables code-reuse at the same time.

Moreover, when the code was finally moved to M3, AFAICT we had validated all supported combinations (DDR2, DDR3, DDR3 with VTT, DDR3 without VTT control) that TI hardware guys could throw at us.Yes there could be stupid bugs lurking in the code since it wasn't reviewed as much as i would have liked but we
have something which proves the functionality to be used as the base.

2. As has been already been pointed out by Russ, on AM335x this step needs to be done very late in the suspend process (the last stage actually).

On the next SoC there are other complications related to security which enforce this step must be done from the M3.

So trying to keep it in M3 for both AM335x and AM437x helps avoid the code churn that will happen if we were to do this differently on two SoCs which have similar PM architectures.

Morevoer, all the suggestions on how to keep the code in Linux working around the complications due to the main memory not being accessible will need to be replicated on the non-Linux s/w stacks and that's just make it more difficult for them.

3. Moving this to M3 leaves the option open to try out some crazy stuff for power optimizations wherein the CORE voltage is lowered even below OPP50. This is yet to be proved in h/w so one can ignore this for now but yes it's a possibility.

Vaibhav Bedia <[email protected]> writes:

Quoted 4 lines

Morevoer, all the suggestions on how to keep the code in Linux working around the complications due to the main memory not being accessible will need to be replicated on the non-Linux s/w stacks and that's just make it more difficult for them.

The linux code has to be very self-contained (in assembler or C), so I'm not sure how it's difficult to replicated in any other OS (or non-OS.)

Quoted 1 line · Kevin Hilman

Vaibhav Bedia <[email protected]> writes:

[snip]

Quoted 7 lines

Quoted 4 lines

Morevoer, all the suggestions on how to keep the code in Linux working around the complications due to the main memory not being accessible will need to be replicated on the non-Linux s/w stacks and that's just make it more difficult for them.

The linux code has to be very self-contained (in assembler or C), so I'm not sure how it's difficult to replicated in any other OS (or non-OS.)

Let me pull on this thread a little bit. Would this involve defining a bunch of different sections for each arch so that I could do:

void __sram_am33xx am33xx_some_pm_function(args....) {
}

static struct am33xx_foo_dyn *blargity __sramdata_am33xx = { ...};

static const struct am33xx_foo_const *blarg __sramconst_am33xx = { ... };

The sram push code could then push these sections into sram. There would then be a set of inlines or macros for converting addresses within these sections to SRAM addresses, maybe also inlines or macros for calling code that has been pushed into sram by wrapping the original function pointer. The macros or inlines that call SRAM code could perform the trampoline as well.

Well, I was thinking of something much dumber.

I was thinking about just _carefully_ writing a single, self-contained C function, with all of its data on the stack (and consts as #defines). Think of it is a step up in readability from straight assembly (which was the stated reason for moving the code to the M3 in the first place.)

Kevin

Quoted 7 lines · Kevin Hilman

Well, I was thinking of something much dumber.

I was thinking about just _carefully_ writing a single, self-contained C function, with all of its data on the stack (and consts as #defines). Think of it is a step up in readability from straight assembly (which was the stated reason for moving the code to the M3 in the first place.)

Quoted 1 line

Kevin

That can only give you your suspend function. You need a resume function to undo everything before calling cpu_resume, and the resume function needs access to the data that was in the suspend function.

Quoted 25 lines

Quoted 7 lines

Maybe I'm getting confused, but the more you talk about the linux and the firmware doing the same code, the more I think the firmware is (trying) to do too much. If this is going to be understandable (and maintainable), there needs to be a clean split of roles between the MPU and the M3.

In the past i tried to keep the firmware as minimal as possible and pushed back all efforts to push unnecessary stuff in there. Like Kevin, i too was of the opinion that we need to do whatever's possible in the kernel.

Sadly, things just got a point where it made much more sense (technically as well as non-technically) to just put the low level stuff in the M3.

1. In addition to the Linux support, we have non-OS based code for AM335x (and AM437x) which in the past just had to duplicate whatever was done in the Linux. Being a different codebase the non-OS guys have their own set of challenges and we had to at times sit through and debug issues in one codebase while the other was working fine. Ideally these things should never come up but sadly they do and we need to solve them.

We also have people trying to implement this thing on different sort of OS environments and they also end up debugging the same set of issues.

Quoted 7 lines

With only a handful of folks able to spend time debugging issues 'do everything in Linux and expect others to copy it' model doesn't scale up. Keeping things in the
firmware with the code available online helps do away with the 'developer scalability'
problem and if one looks at things differently enables code-reuse at the same time.

Yes, yes, this is the classic argument for a HAL, and making that case has always gone over well in the kernel. ;)

Quoted 6 lines

Moreover, when the code was finally moved to M3, AFAICT we had validated all supported combinations (DDR2, DDR3, DDR3 with VTT, DDR3 without VTT control) that TI hardware guys could throw at us.Yes there could be stupid bugs lurking in the code since it wasn't reviewed as much as i would have liked but we
have something which proves the functionality to be used as the base.

I don't follow the argument here. You mean it was validated *before* moving it to the M3, or after? Either way, I'm how that makes a case for moving it to the M3. Surely if it was on the MPU, it would also have been validated, no?

Quoted 3 lines

2. As has been already been pointed out by Russ, on AM335x this step needs to be done very late in the suspend process (the last stage actually).

late, yes. But *technically*, it can still be done from the MPU.

Quoted 2 lines

On the next SoC there are other complications related to security which enforce this step must be done from the M3.

Aha! So far, the first technical reason I've seen. Can you elaborate? Exactly which step has to be done from the M3, and why can't the MPU do it? And could it be done more simply by putting *only* the parts needed on the M3 and not letting it snowball into a pile of stuff?