ARM: OMAP2+: AM33XX: Add suspend-resume support

Mailbox IP on AM33XX is the same as that present in OMAP4. The single instance of Mailbox IP on AM33XX contains 8 sub-modules and facilitates communication between MPU, PRUs and WKUP_M3.

The first mailbox sub-module is assigned for communication between MPU and WKUP-M3.

Signed-off-by Vaibhav Bedia <[email protected]>

Cc Russ Dill <[email protected]>

Cc Santosh Shilimkar <[email protected]>

---
v1->v2:
Address the comment on operator usage from Russ Dill

 drivers/mailbox/mailbox-omap2.c |   35 ++++++++++++++++++++++++++++++++++-
 1 files changed, 34 insertions(+), 1 deletions(-)

drivers/mailbox/mailbox-omap2.c +0 -0

index 7c26bed..6d61159 100644

drivers/mailbox/mailbox-omap2.c +0 -0

@@ -386,6 +412,13 @@ static int __devinit omap2_mbox_probe(struct platform_device *pdev)
 		list[0]->irq = list[1]->irq = platform_get_irq(pdev, 0);
 	}
 #endif

Quoted 2 lines

mbox_write_reg(bit, p->irqdisable);

The cpu_is/soc_is macros are no longer available to drivers with 8d91a42e (Merge tag 'omap-late-cleanups'...). So you'll have to pass whatever flags the driver needs in platform_data or as device tree properties.

Mailbox IP on AM33XX is the same as that present in OMAP4. The single instance of Mailbox IP on AM33XX contains 8 sub-modules and facilitates communication between MPU, PRUs and WKUP_M3.

The first mailbox sub-module is assigned for communication between MPU and WKUP-M3.

Signed-off-by Vaibhav Bedia <[email protected]>

Cc Russ Dill <[email protected]>

Cc Santosh Shilimkar <[email protected]>

---
v1->v2:
Address the comment on operator usage from Russ Dill

  drivers/mailbox/mailbox-omap2.c |   35 ++++++++++++++++++++++++++++++++++-
  1 files changed, 34 insertions(+), 1 deletions(-)

drivers/mailbox/mailbox-omap2.c +27 -1

index 7c26bed..6d61159 100644
@@ -151,7 +151,7 @@ static void omap2_mbox_disable_irq(struct mailbox *mbox,
  	struct omap_mbox2_priv *p = mbox->priv;
  	u32 bit = (irq == IRQ_TX) ? p->notfull_bit : p->newmsg_bit;
 
-	if (!cpu_is_omap44xx())
+	if (!cpu_is_omap44xx() && !soc_is_am33xx())
  		bit = mbox_read_reg(p->irqdisable) & ~bit;
 
  	mbox_write_reg(bit, p->irqdisable);

@@ -352,6 +352,32 @@ struct mailbox mbox_2_info = {
  struct mailbox *omap4_mboxes[] = { &mbox_1_info, &mbox_2_info, NULL };
  #endif
 
+#if defined(CONFIG_SOC_AM33XX)
+static struct omap_mbox2_priv omap2_mbox_wkup_m3_priv = {
+	.tx_fifo = {
+		.msg		= MAILBOX_MESSAGE(0),
+		.fifo_stat	= MAILBOX_FIFOSTATUS(0),
+		.msg_stat	= MAILBOX_MSGSTATUS(0),
+	},
+	.rx_fifo = {
+		.msg		= MAILBOX_MESSAGE(1),
+		.msg_stat	= MAILBOX_MSGSTATUS(1),
+	},
+	.irqenable	= OMAP4_MAILBOX_IRQENABLE(3),
+	.irqstatus	= OMAP4_MAILBOX_IRQSTATUS(3),
+	.irqdisable	= OMAP4_MAILBOX_IRQENABLE_CLR(3),
+	.notfull_bit	= MAILBOX_IRQ_NOTFULL(0),
+	.newmsg_bit	= MAILBOX_IRQ_NEWMSG(0),
+};
+
+struct mailbox mbox_wkup_m3_info = {
+	.name	= "wkup_m3",
+	.ops	= &omap2_mbox_ops,
+	.priv	= &omap2_mbox_wkup_m3_priv,
+};
+struct mailbox *am33xx_mboxes[] = { &mbox_wkup_m3_info, NULL };
+#endif
+
  static int __devinit omap2_mbox_probe(struct platform_device *pdev)
  {
  	struct resource *mem;

Quoted 5 lines · Vaibhav Bedia

Mailbox IP on AM33XX is the same as that present in OMAP4. The single instance of Mailbox IP on AM33XX contains 8 sub-modules and facilitates communication between MPU, PRUs and WKUP_M3.

PRUS?

Quoted 61 lines

The first mailbox sub-module is assigned for communication between MPU and WKUP-M3.

Signed-off-by Vaibhav Bedia <[email protected]>

Cc Russ Dill <[email protected]>

Cc Santosh Shilimkar <[email protected]>

---
v1->v2:
Address the comment on operator usage from Russ Dill

  drivers/mailbox/mailbox-omap2.c |   35 ++++++++++++++++++++++++++++++++++-
  1 files changed, 34 insertions(+), 1 deletions(-)

drivers/mailbox/mailbox-omap2.c +27 -1

index 7c26bed..6d61159 100644
@@ -151,7 +151,7 @@ static void omap2_mbox_disable_irq(struct mailbox *mbox,
  	struct omap_mbox2_priv *p = mbox->priv;
  	u32 bit = (irq == IRQ_TX) ? p->notfull_bit : p->newmsg_bit;
 
-	if (!cpu_is_omap44xx())
+	if (!cpu_is_omap44xx() && !soc_is_am33xx())
  		bit = mbox_read_reg(p->irqdisable) & ~bit;
 
  	mbox_write_reg(bit, p->irqdisable);

@@ -352,6 +352,32 @@ struct mailbox mbox_2_info = {
  struct mailbox *omap4_mboxes[] = { &mbox_1_info, &mbox_2_info, NULL };
  #endif
 
+#if defined(CONFIG_SOC_AM33XX)
+static struct omap_mbox2_priv omap2_mbox_wkup_m3_priv = {
+	.tx_fifo = {
+		.msg		= MAILBOX_MESSAGE(0),
+		.fifo_stat	= MAILBOX_FIFOSTATUS(0),
+		.msg_stat	= MAILBOX_MSGSTATUS(0),
+	},
+	.rx_fifo = {
+		.msg		= MAILBOX_MESSAGE(1),
+		.msg_stat	= MAILBOX_MSGSTATUS(1),
+	},
+	.irqenable	= OMAP4_MAILBOX_IRQENABLE(3),
+	.irqstatus	= OMAP4_MAILBOX_IRQSTATUS(3),
+	.irqdisable	= OMAP4_MAILBOX_IRQENABLE_CLR(3),
+	.notfull_bit	= MAILBOX_IRQ_NOTFULL(0),
+	.newmsg_bit	= MAILBOX_IRQ_NEWMSG(0),
+};
+
+struct mailbox mbox_wkup_m3_info = {
+	.name	= "wkup_m3",
+	.ops	= &omap2_mbox_ops,
+	.priv	= &omap2_mbox_wkup_m3_priv,
+};
+struct mailbox *am33xx_mboxes[] = { &mbox_wkup_m3_info, NULL };
+#endif
+
  static int __devinit omap2_mbox_probe(struct platform_device *pdev)
  {
  	struct resource *mem;

drivers/mailbox/mailbox-omap2.c static int __devinit omap2_mbox_probe(struct platform_device *pdev) · line 415

Quoted 4 lines

drivers/mailbox/mailbox-omap2.c +1 -0

@@ -386,6 +412,13 @@ static int __devinit omap2_mbox_probe(struct platform_device *pdev)
  		list[0]->irq = list[1]->irq = platform_get_irq(pdev, 0);
  	}
  #endif
+#if defined(CONFIG_SOC_AM33XX)

ifdef in middle of the code. I know you are just
following what is already there.

drivers/mailbox/mailbox-omap2.c static int __devinit omap2_mbox_probe(struct platform_device *pdev) · line 418

Quoted 3 lines

drivers/mailbox/mailbox-omap2.c +3 -0

+	else if (soc_is_am33xx()) {
+		list = am33xx_mboxes;
+

UN-necessary extra line here.

drivers/mailbox/mailbox-omap2.c static int __devinit omap2_mbox_probe(struct platform_device *pdev) · line 421

Quoted 3 lines

drivers/mailbox/mailbox-omap2.c +3 -0

+		list[0]->irq = platform_get_irq(pdev, 0);
+	}
+#endif

Hopefully mailbox clean-up will kill that #ifdeffery. Apart from those comments, patch looks fine to me.

On AM33XX, the mailbox module between the MPU and the WKUP-M3 co-processor facilitates a one-way communication. MPU uses the assigned mailbox sub-module to issue the interrupt to the WKUP-M3 co-processor which then goes and reads the the IPC data from registers in the control module.

WKUP-M3 is in the L4_WKUP and does not have any access to the mailbox module. Due to this limitation, the MPU is completely responsible for FIFO maintenance and interrupt generation. MPU needs to ensure that the FIFO does not overflow by reading back the assigned mailbox sub-module.

This patch adds an API in the mailbox code which the MPU can use to empty the FIFO by issuing a readback command.

Signed-off-by Vaibhav Bedia <[email protected]>

Cc Santosh Shilimkar <[email protected]>

---
Note: This patch which will be slightly reworked once the mailbox driver changes are finalized.

  drivers/mailbox/mailbox-omap2.c |   19 +++++++++++++++++++
  drivers/mailbox/mailbox.c       |   36 ++++++++++++++++++++++++++++++++++++
  drivers/mailbox/mailbox.h       |    3 +++
  include/linux/mailbox.h         |    1 +
  4 files changed, 59 insertions(+), 0 deletions(-)

drivers/mailbox/mailbox-omap2.c +19 -0

index 6d61159..c732be1 100644
@@ -125,6 +125,23 @@ static int omap2_mbox_fifo_full(struct mailbox *mbox)
  	return mbox_read_reg(fifo->fifo_stat);
  }
 
+static int omap2_mbox_fifo_needs_flush(struct mailbox *mbox)
+{
+	struct omap_mbox2_priv *p = mbox->priv;
+	struct omap_mbox2_fifo *fifo = &p->tx_fifo;
+
+	return mbox_read_reg(fifo->msg_stat);
+}
+
+static void omap2_mbox_fifo_readback(struct mailbox *mbox,
+			struct mailbox_msg *msg)
+{
+	struct omap_mbox2_priv *p = mbox->priv;
+	struct omap_mbox2_fifo *fifo = &p->tx_fifo;
+
+	msg->header = mbox_read_reg(fifo->msg);
+}
+
  static int ompa2_mbox_poll_for_space(struct mailbox *mbox)
  {
  	if (omap2_mbox_fifo_full(mbox))

@@ -221,6 +238,8 @@ static struct mailbox_ops omap2_mbox_ops = {
  	.read           = omap2_mbox_fifo_read,
  	.write          = omap2_mbox_fifo_write,
  	.empty          = omap2_mbox_fifo_empty,
+	.fifo_needs_flush	= omap2_mbox_fifo_needs_flush,
+	.fifo_readback		= omap2_mbox_fifo_readback,
  	.poll_for_space = ompa2_mbox_poll_for_space,
  	.enable_irq     = omap2_mbox_enable_irq,
  	.disable_irq    = omap2_mbox_disable_irq,

drivers/mailbox/mailbox.c +9 -0

index 2f50226..92c9f68 100644
@@ -57,6 +57,15 @@ static inline int mbox_empty(struct mailbox *mbox)
  {
  	return mbox->ops->empty(mbox);
  }
+static inline int mbox_fifo_needs_flush(struct mailbox *mbox)
+{
+	return mbox->ops->fifo_needs_flush(mbox);
+}
+static inline void mbox_fifo_readback(struct mailbox *mbox,
+				struct mailbox_msg *msg)
+{
+	mbox->ops->fifo_readback(mbox, msg);
+}
 
  /* Mailbox IRQ handle functions */
  static inline void ack_mbox_irq(struct mailbox *mbox, mailbox_irq_t irq)

Quoted 16 lines · Vaibhav Bedia

On AM33XX, the mailbox module between the MPU and the WKUP-M3 co-processor facilitates a one-way communication. MPU uses the assigned mailbox sub-module to issue the interrupt to the WKUP-M3 co-processor which then goes and reads the the IPC data from registers in the control module.

WKUP-M3 is in the L4_WKUP and does not have any access to the mailbox module. Due to this limitation, the MPU is completely responsible for FIFO maintenance and interrupt generation. MPU needs to ensure that the FIFO does not overflow by reading back the assigned mailbox sub-module.

This patch adds an API in the mailbox code which the MPU can use to empty the FIFO by issuing a readback command.

Quoted 6 lines

Signed-off-by Vaibhav Bedia <[email protected]>

Cc Santosh Shilimkar <[email protected]>

---
Note: This patch which will be slightly reworked once the mailbox driver changes are finalized.

Can you expand a bit please ?

Quoted 62 lines

  drivers/mailbox/mailbox-omap2.c |   19 +++++++++++++++++++
  drivers/mailbox/mailbox.c       |   36 ++++++++++++++++++++++++++++++++++++
  drivers/mailbox/mailbox.h       |    3 +++
  include/linux/mailbox.h         |    1 +
  4 files changed, 59 insertions(+), 0 deletions(-)

drivers/mailbox/mailbox-omap2.c +19 -0

index 6d61159..c732be1 100644
@@ -125,6 +125,23 @@ static int omap2_mbox_fifo_full(struct mailbox *mbox)
  	return mbox_read_reg(fifo->fifo_stat);
  }
 
+static int omap2_mbox_fifo_needs_flush(struct mailbox *mbox)
+{
+	struct omap_mbox2_priv *p = mbox->priv;
+	struct omap_mbox2_fifo *fifo = &p->tx_fifo;
+
+	return mbox_read_reg(fifo->msg_stat);
+}
+
+static void omap2_mbox_fifo_readback(struct mailbox *mbox,
+			struct mailbox_msg *msg)
+{
+	struct omap_mbox2_priv *p = mbox->priv;
+	struct omap_mbox2_fifo *fifo = &p->tx_fifo;
+
+	msg->header = mbox_read_reg(fifo->msg);
+}
+
  static int ompa2_mbox_poll_for_space(struct mailbox *mbox)
  {
  	if (omap2_mbox_fifo_full(mbox))

@@ -221,6 +238,8 @@ static struct mailbox_ops omap2_mbox_ops = {
  	.read           = omap2_mbox_fifo_read,
  	.write          = omap2_mbox_fifo_write,
  	.empty          = omap2_mbox_fifo_empty,
+	.fifo_needs_flush	= omap2_mbox_fifo_needs_flush,
+	.fifo_readback		= omap2_mbox_fifo_readback,
  	.poll_for_space = ompa2_mbox_poll_for_space,
  	.enable_irq     = omap2_mbox_enable_irq,
  	.disable_irq    = omap2_mbox_disable_irq,

drivers/mailbox/mailbox.c +9 -0

index 2f50226..92c9f68 100644
@@ -57,6 +57,15 @@ static inline int mbox_empty(struct mailbox *mbox)
  {
  	return mbox->ops->empty(mbox);
  }
+static inline int mbox_fifo_needs_flush(struct mailbox *mbox)
+{
+	return mbox->ops->fifo_needs_flush(mbox);
+}
+static inline void mbox_fifo_readback(struct mailbox *mbox,
+				struct mailbox_msg *msg)
+{
+	mbox->ops->fifo_readback(mbox, msg);
+}
 

drivers/mailbox/mailbox.c out: · line 122

Quoted 7 lines

drivers/mailbox/mailbox.c +1 -0

  /* Mailbox IRQ handle functions */
  static inline void ack_mbox_irq(struct mailbox *mbox, mailbox_irq_t irq)

@@ -110,6 +119,33 @@ out:
  }
  EXPORT_SYMBOL(mailbox_msg_send);
 
+/*

s/*/**

drivers/mailbox/mailbox.c out: · line 128

Quoted 6 lines

drivers/mailbox/mailbox.c +6 -0

+ * Flush the Rx FIFO by reading back the messages
+ * Since the normal expectation is that the Rx will do the
+ * reading, add a debug message to indicate if we really flush
+ *
+ * Returns the no. of messages read back
+ */

Just look at the kernel doc style for above

Rest looks fine.

OMAP4 and AM33XX share the same EMIF controller IP. Although there are significant differences in the IP integration due to which AM33XX can't reuse the EMIF driver DVFS similar to OMAP4, it can definitely benefit by reusing the EMIF related macros defined in drivers/memory/emif.h.

In the current OMAP PM framework the PM code resides under arch/arm/mach-omap2/. To enable reuse of the register defines move the emif header file to include/linux so that both the EMIF driver and the AM33XX PM code can benefit.

Signed-off-by Vaibhav Bedia <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Benoit Cousson <[email protected]>

Cc Aneesh V <[email protected]>

---
v1->v2:
This is a new patch in the series to enable code reuse between the EMIF driver and AM33XX PM code

  drivers/memory/emif.c   |    2 +-
  drivers/memory/emif.h   |  589 -----------------------------------------------
  include/linux/ti_emif.h |  589 +++++++++++++++++++++++++++++++++++++++++++++++

include/linux/ti_emif.h new file +9 -0

+/* DDR_PHY_CTRL_1 - EMIF4D5 */
+#define DLL_HALF_DELAY_SHIFT_4D5			21
+#define DLL_HALF_DELAY_MASK_4D5				(1 << 21)
+#define READ_LATENCY_SHIFT_4D5				0
+#define READ_LATENCY_MASK_4D5				(0x1f << 0)
+
+/* DDR_PHY_CTRL_1_SHDW */
+#define DDR_PHY_CTRL_1_SHDW_SHIFT			5
+#define DDR_PHY_CTRL_1_SHDW_MASK			(0x7ffffff << 5)

Quoted 4 lines

  drivers/memory/emif.c   |    2 +-
  drivers/memory/emif.h   |  589 -----------------------------------------------
  include/linux/ti_emif.h |  589 +++++++++++++++++++++++++++++++++++++++++++++++

You are just moving the file. So "git mv file1 flie2; and the git format-patch -C"
on committed patch should just generate few lines of patch.

Quoted 13 lines

include/linux/ti_emif.h new file +13 -0

+/* DDR_PHY_CTRL_1 - EMIF4D5 */
+#define DLL_HALF_DELAY_SHIFT_4D5			21
+#define DLL_HALF_DELAY_MASK_4D5				(1 << 21)
+#define READ_LATENCY_SHIFT_4D5				0
+#define READ_LATENCY_MASK_4D5				(0x1f << 0)
+
+/* DDR_PHY_CTRL_1_SHDW */
+#define DDR_PHY_CTRL_1_SHDW_SHIFT			5
+#define DDR_PHY_CTRL_1_SHDW_MASK			(0x7ffffff << 5)
+#define READ_LATENCY_SHDW_SHIFT				0
+#define READ_LATENCY_SHDW_MASK				(0x1f << 0)
+
+#ifndef __ASSEMBLY__

include/linux/ti_emif.h line 564

Quoted 6 lines

include/linux/ti_emif.h new file +6 -0

+/*
+ * Structure containing shadow of important registers in EMIF
+ * The calculation function fills in this structure to be later used for
+ * initialisation and DVFS
+ */
+struct emif_regs {

Are you using above struct. If not we can leave it in same place and just move the register defines.

Some of the included header files are not needed so remove them.

Signed-off-by Vaibhav Bedia <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Benoit Cousson <[email protected]>

Cc Paul Walmsley <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

---

This is necessary to ensure that macros declared here can be reused from assembly files.

Signed-off-by Vaibhav Bedia <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Paul Walmsley <[email protected]>

Cc Benoit Cousson <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

---
v1->v2:
Split out the header file changes in a separate patch based on the feedback from Santosh

  arch/arm/mach-omap2/cm33xx.h  |    3 +++
  arch/arm/mach-omap2/prm33xx.h |    3 +++
  2 files changed, 6 insertions(+), 0 deletions(-)

arch/arm/mach-omap2/cm33xx.h +1 -0

index 8009e13..2f215cd 100644
@@ -376,6 +376,7 @@
  #define AM33XX_CM_CEFUSE_CEFUSE_CLKCTRL			AM33XX_CM_REGADDR(AM33XX_CM_CEFUSE_MOD, 0x0020)
 
 
+#ifndef __ASSEMBLER__
  extern bool am33xx_cm_is_clkdm_in_hwsup(s16 inst, u16 cdoffs);
  extern void am33xx_cm_clkdm_enable_hwsup(s16 inst, u16 cdoffs);
  extern void am33xx_cm_clkdm_disable_hwsup(s16 inst, u16 cdoffs);

arch/arm/mach-omap2/cm33xx.h +0 -0

  #endif

arch/arm/mach-omap2/prm33xx.h +1 -0

index 3f25c56..2f2eaa0 100644
@@ -117,6 +117,7 @@
  #define AM33XX_PM_CEFUSE_PWRSTST_OFFSET		0x0004
  #define AM33XX_PM_CEFUSE_PWRSTST		AM33XX_PRM_REGADDR(AM33XX_PRM_CEFUSE_MOD, 0x0004)
 
+#ifndef __ASSEMBLER__
  extern u32 am33xx_prm_read_reg(s16 inst, u16 idx);
  extern void am33xx_prm_write_reg(u32 val, s16 inst, u16 idx);
  extern u32 am33xx_prm_rmw_reg_bits(u32 mask, u32 bits, s16 inst, s16 idx);

Review

Quoted 2 lines

(quoted diff) +2 -0

+#endif /* ASSEMBLER */
+

Drop that extra line.

Quoted 9 lines

  #endif

arch/arm/mach-omap2/prm33xx.h +0 -0

index 3f25c56..2f2eaa0 100644
@@ -117,6 +117,7 @@
  #define AM33XX_PM_CEFUSE_PWRSTST_OFFSET		0x0004
  #define AM33XX_PM_CEFUSE_PWRSTST		AM33XX_PRM_REGADDR(AM33XX_PRM_CEFUSE_MOD, 0x0004)
 

arch/arm/mach-omap2/prm33xx.h extern int am33xx_prm_is_hardreset_asserted(u8 shift, s16 inst, · line 131

Quoted 10 lines

arch/arm/mach-omap2/prm33xx.h +3 -0

+#ifndef __ASSEMBLER__
  extern u32 am33xx_prm_read_reg(s16 inst, u16 idx);
  extern void am33xx_prm_write_reg(u32 val, s16 inst, u16 idx);
  extern u32 am33xx_prm_rmw_reg_bits(u32 mask, u32 bits, s16 inst, s16 idx);

@@ -126,4 +127,6 @@ extern int am33xx_prm_is_hardreset_asserted(u8 shift, s16 inst,
  extern int am33xx_prm_assert_hardreset(u8 shift, s16 inst, u16 rstctrl_offs);
  extern int am33xx_prm_deassert_hardreset(u8 shift, s16 inst,
  		u16 rstctrl_offs, u16 rstst_offs);
+#endif /* ASSEMBLER */
+

ditto

Otherwise looks fine.

Acked-by Santosh Shilimkar <[email protected]>

OCMC RAM lies in the PER power domain and this memory support retention.

Signed-off-by Vaibhav Bedia <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Benoit Cousson <[email protected]>

Cc Paul Walmsley <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

---

TPTC0 needs to be idled and put to standby under SW control. Add the appropriate flags in the TPTC0 hwmod entry.

Signed-off-by Vaibhav Bedia <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Benoit Cousson <[email protected]>

Cc Paul Walmsley <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

---

Quoted 3 lines · Vaibhav Bedia

TPTC0 needs to be idled and put to standby under SW control. Add the appropriate flags in the TPTC0 hwmod entry.

Can you please expand TPTC0 in chane log.

Quoted 7 lines

Signed-off-by Vaibhav Bedia <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Benoit Cousson <[email protected]>

Cc Paul Walmsley <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

---

Patch is fine otherwise.

Acked-by Santosh Shilimkar <[email protected]>

TPTC0 needs to be idled and put to standby under SW control.

Add the appropriate flags in the TPTC0 hwmod entry.

Signed-off-by Vaibhav Bedia <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Benoit Cousson <[email protected]>

Cc Paul Walmsley <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

---
v1->v2:
No change

 arch/arm/mach-omap2/omap_hwmod_33xx_data.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

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

index 5e50e68..5c34d4b 100644
@@ -1823,6 +1823,7 @@ static struct omap_hwmod am33xx_tptc0_hwmod = {
 	.class		= &am33xx_tptc_hwmod_class,
 	.clkdm_name	= "l3_clkdm",
 	.mpu_irqs	= am33xx_tptc0_irqs,
+	.flags		= (HWMOD_SWSUP_SIDLE | HWMOD_SWSUP_MSTANDBY),
 	.main_clk	= "l3_gclk",
 	.prcm		= {
 		.omap4	= {

Quoted 1 line · Vaibhav Bedia

TPTC0 needs to be idled and put to standby under SW control.

Please elaborate about why (e.g. HW support not available, HW support broken/buggy, etc.) since these blocks are not well documented in the docs that I have (spruh73f.)

The current HWMOD code expects the memory region with the IP's SYSCONFIG register to be marked with ADDR_TYPE_RT flag.

CPGMAC0 hwmod entry specifies two memory regions and marks both with the flag ADDR_TYPE_RT although only the 2nd region has the SYSCONFIG register. This leads to the HWMOD code accessing the wrong memory address for idle and standby operations. Fix this by removing the ADDR_TYPE_RT flag from the 1st memory region in CPGMAC0 hwmod entry.

Signed-off-by Vaibhav Bedia <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Benoit Cousson <[email protected]>

Cc Paul Walmsley <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

---

WKUP-M3 has a reset status bit (RM_WKUP_STST.WKUP_M3_LRST) Update the WKUP-M3 hwmod data to reflect the same.

Signed-off-by Vaibhav Bedia <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Benoit Cousson <[email protected]>

Cc Paul Walmsley <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

---

WKUP-M3 has a reset status bit (RM_WKUP_STST.WKUP_M3_LRST) Update the hardreset API to ensure that the reset line properly deasserted.

Signed-off-by Vaibhav Bedia <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Benoit Cousson <[email protected]>

Cc Paul Walmsley <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

---

Since AM33XX supports only DT-boot, this is needed for the appropriate device nodes to be created.

Note: OCMC RAM is part of the PER power domain and supports retention. The assembly code for low power entry/exit will run from OCMC RAM. To ensure that the OMAP PM code does not attempt to disable the clock to OCMC RAM as part of the suspend process add the no_idle_on_suspend flag.

Signed-off-by Vaibhav Bedia <[email protected]>

Cc Tony Lindgren <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Benoit Cousson <[email protected]>

Cc Paul Walmsley <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

---

The current OMAP timer code registers two timers - one as clocksource and one as clockevent. AM33XX has only one usable timer in the WKUP domain so one of the timers needs suspend-resume support to restore the configuration to pre-suspend state.

commit adc78e6 (timekeeping: Add suspend and resume of clock event devices) introduced .suspend and .resume callbacks for clock event devices. Leverages these callbacks to have AM33XX clockevent timer which is in not in WKUP domain to behave properly across system suspend.

Signed-off-by Vaibhav Bedia <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Benoit Cousson <[email protected]>

Cc Paul Walmsley <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

Cc Jon Hunter <[email protected]>

---
v1->v2:
Get rid of harcoded timer id.
Note: since a platform device is not created for these timer instances and because there's very minimal change needed for restarting the timer a full blown context save and restore has been skipped.

  arch/arm/mach-omap2/timer.c |   33 +++++++++++++++++++++++++++++++++
  1 files changed, 33 insertions(+), 0 deletions(-)

arch/arm/mach-omap2/timer.c +20 -0

index 691aa67..38f9cbc 100644
@@ -128,6 +128,36 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode,
  	}
  }
 
+static void omap_clkevt_suspend(struct clock_event_device *unused)
+{
+	char name[10];
+	struct omap_hwmod *oh;
+
+	sprintf(name, "timer%d", clkev.id);
+	oh = omap_hwmod_lookup(name);
+	if (!oh)
+		return;
+
+	__omap_dm_timer_stop(&clkev, 1, clkev.rate);
+	omap_hwmod_idle(oh);
+}
+
+static void omap_clkevt_resume(struct clock_event_device *unused)
+{
+	char name[10];
+	struct omap_hwmod *oh;
+
+	sprintf(name, "timer%d", clkev.id);

Quoted 59 lines · Vaibhav Bedia

The current OMAP timer code registers two timers - one as clocksource and one as clockevent. AM33XX has only one usable timer in the WKUP domain so one of the timers needs suspend-resume support to restore the configuration to pre-suspend state.

commit adc78e6 (timekeeping: Add suspend and resume of clock event devices) introduced .suspend and .resume callbacks for clock event devices. Leverages these callbacks to have AM33XX clockevent timer which is in not in WKUP domain to behave properly across system suspend.

Signed-off-by Vaibhav Bedia <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Benoit Cousson <[email protected]>

Cc Paul Walmsley <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

Cc Jon Hunter <[email protected]>

---
v1->v2:
Get rid of harcoded timer id.
Note: since a platform device is not created for these timer instances and because there's very minimal change needed for restarting the timer a full blown context save and restore has been skipped.

  arch/arm/mach-omap2/timer.c |   33 +++++++++++++++++++++++++++++++++
  1 files changed, 33 insertions(+), 0 deletions(-)

arch/arm/mach-omap2/timer.c +20 -0

index 691aa67..38f9cbc 100644
@@ -128,6 +128,36 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode,
  	}
  }
 
+static void omap_clkevt_suspend(struct clock_event_device *unused)
+{
+	char name[10];
+	struct omap_hwmod *oh;
+
+	sprintf(name, "timer%d", clkev.id);
+	oh = omap_hwmod_lookup(name);
+	if (!oh)
+		return;
+
+	__omap_dm_timer_stop(&clkev, 1, clkev.rate);
+	omap_hwmod_idle(oh);
+}
+
+static void omap_clkevt_resume(struct clock_event_device *unused)
+{
+	char name[10];
+	struct omap_hwmod *oh;
+
+	sprintf(name, "timer%d", clkev.id);

arch/arm/mach-omap2/timer.c static void omap2_gp_timer_set_mode(enum clock_event_mode mode, · line 160

Quoted 10 lines

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

+	oh = omap_hwmod_lookup(name);
+	if (!oh)
+		return;
+
+	omap_hwmod_enable(oh);
+	__omap_dm_timer_load_start(&clkev,
+			OMAP_TIMER_CTRL_ST | OMAP_TIMER_CTRL_AR, 0, 1);
+	__omap_dm_timer_int_enable(&clkev, OMAP_TIMER_INT_OVERFLOW);
+}
+

Am still bit uncomfortable with direct hwmod usage in the suspend/resmue hooks.

Jon, Any alternatives you can think of ?

Quoted 73 lines · Santosh

Quoted 59 lines · Vaibhav Bedia

The current OMAP timer code registers two timers - one as clocksource and one as clockevent. AM33XX has only one usable timer in the WKUP domain so one of the timers needs suspend-resume support to restore the configuration to pre-suspend state.

commit adc78e6 (timekeeping: Add suspend and resume of clock event devices) introduced .suspend and .resume callbacks for clock event devices. Leverages these callbacks to have AM33XX clockevent timer which is in not in WKUP domain to behave properly across system suspend.

Signed-off-by Vaibhav Bedia <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Benoit Cousson <[email protected]>

Cc Paul Walmsley <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

Cc Jon Hunter <[email protected]>

---
v1->v2:
Get rid of harcoded timer id.
Note: since a platform device is not created for these timer instances and because there's very minimal change needed for restarting the timer a full blown context save and restore has been skipped.

  arch/arm/mach-omap2/timer.c |   33 +++++++++++++++++++++++++++++++++
  1 files changed, 33 insertions(+), 0 deletions(-)

arch/arm/mach-omap2/timer.c +20 -0

index 691aa67..38f9cbc 100644
@@ -128,6 +128,36 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode,
  	}
  }
 
+static void omap_clkevt_suspend(struct clock_event_device *unused)
+{
+	char name[10];
+	struct omap_hwmod *oh;
+
+	sprintf(name, "timer%d", clkev.id);
+	oh = omap_hwmod_lookup(name);
+	if (!oh)
+		return;
+
+	__omap_dm_timer_stop(&clkev, 1, clkev.rate);
+	omap_hwmod_idle(oh);
+}
+
+static void omap_clkevt_resume(struct clock_event_device *unused)
+{
+	char name[10];
+	struct omap_hwmod *oh;
+
+	sprintf(name, "timer%d", clkev.id);

arch/arm/mach-omap2/timer.c static void omap2_gp_timer_set_mode(enum clock_event_mode mode, · line 160

Quoted 10 lines

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

+	oh = omap_hwmod_lookup(name);
+	if (!oh)
+		return;
+
+	omap_hwmod_enable(oh);
+	__omap_dm_timer_load_start(&clkev,
+			OMAP_TIMER_CTRL_ST | OMAP_TIMER_CTRL_AR, 0, 1);
+	__omap_dm_timer_int_enable(&clkev, OMAP_TIMER_INT_OVERFLOW);
+}
+

Am still bit uncomfortable with direct hwmod usage in the suspend/resmue hooks.

Quoted 2 lines

Jon, Any alternatives you can think of ?

Jon,

Any suggestions here?

Quoted 74 lines · Vaibhav

Quoted 73 lines · Santosh

Quoted 59 lines · Vaibhav Bedia

The current OMAP timer code registers two timers - one as clocksource and one as clockevent. AM33XX has only one usable timer in the WKUP domain so one of the timers needs suspend-resume support to restore the configuration to pre-suspend state.

commit adc78e6 (timekeeping: Add suspend and resume of clock event devices) introduced .suspend and .resume callbacks for clock event devices. Leverages these callbacks to have AM33XX clockevent timer which is in not in WKUP domain to behave properly across system suspend.

Signed-off-by Vaibhav Bedia <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Benoit Cousson <[email protected]>

Cc Paul Walmsley <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

Cc Jon Hunter <[email protected]>

---
v1->v2:
Get rid of harcoded timer id.
Note: since a platform device is not created for these timer instances and because there's very minimal change needed for restarting the timer a full blown context save and restore has been skipped.

   arch/arm/mach-omap2/timer.c |   33 +++++++++++++++++++++++++++++++++
   1 files changed, 33 insertions(+), 0 deletions(-)

arch/arm/mach-omap2/timer.c +20 -0

index 691aa67..38f9cbc 100644
@@ -128,6 +128,36 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode,
   	}
   }
 
+static void omap_clkevt_suspend(struct clock_event_device *unused)
+{
+	char name[10];
+	struct omap_hwmod *oh;
+
+	sprintf(name, "timer%d", clkev.id);
+	oh = omap_hwmod_lookup(name);
+	if (!oh)
+		return;
+
+	__omap_dm_timer_stop(&clkev, 1, clkev.rate);
+	omap_hwmod_idle(oh);
+}
+
+static void omap_clkevt_resume(struct clock_event_device *unused)
+{
+	char name[10];
+	struct omap_hwmod *oh;
+
+	sprintf(name, "timer%d", clkev.id);

arch/arm/mach-omap2/timer.c static void omap2_gp_timer_set_mode(enum clock_event_mode mode, · line 160

Quoted 10 lines

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

+	oh = omap_hwmod_lookup(name);
+	if (!oh)
+		return;
+
+	omap_hwmod_enable(oh);
+	__omap_dm_timer_load_start(&clkev,
+			OMAP_TIMER_CTRL_ST | OMAP_TIMER_CTRL_AR, 0, 1);
+	__omap_dm_timer_int_enable(&clkev, OMAP_TIMER_INT_OVERFLOW);
+}
+

Am still bit uncomfortable with direct hwmod usage in the suspend/resmue hooks.

Quoted 6 lines

Quoted 2 lines

Jon, Any alternatives you can think of ?

Jon,

Any suggestions here?

Sorry completed this missed this!

Unfortunately, I don't have any good suggestions here. However, I am wondering if the suspend/resume handlers can just be calls to omap_hwmod_idle/enable and we can remove these __omap_dm_timer_xxxx calls (I don't think they are needed). Furthermore, my understanding is this is only needed for AM335x (per the changelog), and so we should not add suspend/resume handlers for all OMAP2+ based devices.

Quoted 85 lines · Jon Hunter

Quoted 74 lines · Vaibhav

Quoted 73 lines · Santosh

Quoted 59 lines · Vaibhav Bedia

The current OMAP timer code registers two timers - one as clocksource and one as clockevent. AM33XX has only one usable timer in the WKUP domain so one of the timers needs suspend-resume support to restore the configuration to pre-suspend state.

commit adc78e6 (timekeeping: Add suspend and resume of clock event devices) introduced .suspend and .resume callbacks for clock event devices. Leverages these callbacks to have AM33XX clockevent timer which is in not in WKUP domain to behave properly across system suspend.

Signed-off-by Vaibhav Bedia <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Benoit Cousson <[email protected]>

Cc Paul Walmsley <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

Cc Jon Hunter <[email protected]>

---
v1->v2:
Get rid of harcoded timer id.
Note: since a platform device is not created for these timer instances and because there's very minimal change needed for restarting the timer a full blown context save and restore has been skipped.

   arch/arm/mach-omap2/timer.c |   33 +++++++++++++++++++++++++++++++++
   1 files changed, 33 insertions(+), 0 deletions(-)

arch/arm/mach-omap2/timer.c +20 -0

index 691aa67..38f9cbc 100644
@@ -128,6 +128,36 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode,
   	}
   }
 
+static void omap_clkevt_suspend(struct clock_event_device *unused)
+{
+	char name[10];
+	struct omap_hwmod *oh;
+
+	sprintf(name, "timer%d", clkev.id);
+	oh = omap_hwmod_lookup(name);
+	if (!oh)
+		return;
+
+	__omap_dm_timer_stop(&clkev, 1, clkev.rate);
+	omap_hwmod_idle(oh);
+}
+
+static void omap_clkevt_resume(struct clock_event_device *unused)
+{
+	char name[10];
+	struct omap_hwmod *oh;
+
+	sprintf(name, "timer%d", clkev.id);

arch/arm/mach-omap2/timer.c static void omap2_gp_timer_set_mode(enum clock_event_mode mode, · line 160

Quoted 10 lines

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

+	oh = omap_hwmod_lookup(name);
+	if (!oh)
+		return;
+
+	omap_hwmod_enable(oh);
+	__omap_dm_timer_load_start(&clkev,
+			OMAP_TIMER_CTRL_ST | OMAP_TIMER_CTRL_AR, 0, 1);
+	__omap_dm_timer_int_enable(&clkev, OMAP_TIMER_INT_OVERFLOW);
+}
+

Am still bit uncomfortable with direct hwmod usage in the suspend/resmue hooks.

Quoted 6 lines

Quoted 2 lines

Jon, Any alternatives you can think of ?

Jon,

Any suggestions here?

Sorry completed this missed this!

Quoted 7 lines

Unfortunately, I don't have any good suggestions here. However, I am wondering if the suspend/resume handlers can just be calls to omap_hwmod_idle/enable and we can remove these __omap_dm_timer_xxxx calls (I don't think they are needed). Furthermore, my understanding is this is only needed for AM335x (per the changelog), and so we should not add suspend/resume handlers for all OMAP2+ based devices.

I agree with the direction.

Quoted 93 lines · Santosh

Quoted 92 lines · Jon Hunter

Quoted 74 lines · Vaibhav

Quoted 73 lines · Santosh

Quoted 59 lines · Vaibhav Bedia

The current OMAP timer code registers two timers - one as clocksource and one as clockevent. AM33XX has only one usable timer in the WKUP domain so one of the timers needs suspend-resume support to restore the configuration to pre-suspend state.

commit adc78e6 (timekeeping: Add suspend and resume of clock event devices) introduced .suspend and .resume callbacks for clock event devices. Leverages these callbacks to have AM33XX clockevent timer which is in not in WKUP domain to behave properly across system suspend.

Signed-off-by Vaibhav Bedia <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Benoit Cousson <[email protected]>

Cc Paul Walmsley <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

Cc Jon Hunter <[email protected]>

---
v1->v2:
Get rid of harcoded timer id.
Note: since a platform device is not created for these timer instances and because there's very minimal change needed for restarting the timer a full blown context save and restore has been skipped.

   arch/arm/mach-omap2/timer.c |   33 +++++++++++++++++++++++++++++++++
   1 files changed, 33 insertions(+), 0 deletions(-)

arch/arm/mach-omap2/timer.c +20 -0

index 691aa67..38f9cbc 100644
@@ -128,6 +128,36 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode,
   	}
   }
 
+static void omap_clkevt_suspend(struct clock_event_device *unused)
+{
+	char name[10];
+	struct omap_hwmod *oh;
+
+	sprintf(name, "timer%d", clkev.id);
+	oh = omap_hwmod_lookup(name);
+	if (!oh)
+		return;
+
+	__omap_dm_timer_stop(&clkev, 1, clkev.rate);
+	omap_hwmod_idle(oh);
+}
+
+static void omap_clkevt_resume(struct clock_event_device *unused)
+{
+	char name[10];
+	struct omap_hwmod *oh;
+
+	sprintf(name, "timer%d", clkev.id);

arch/arm/mach-omap2/timer.c static void omap2_gp_timer_set_mode(enum clock_event_mode mode, · line 160

Quoted 10 lines

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

+	oh = omap_hwmod_lookup(name);
+	if (!oh)
+		return;
+
+	omap_hwmod_enable(oh);
+	__omap_dm_timer_load_start(&clkev,
+			OMAP_TIMER_CTRL_ST | OMAP_TIMER_CTRL_AR, 0, 1);
+	__omap_dm_timer_int_enable(&clkev, OMAP_TIMER_INT_OVERFLOW);
+}
+

Am still bit uncomfortable with direct hwmod usage in the suspend/resmue hooks.

Quoted 6 lines

Quoted 2 lines

Jon, Any alternatives you can think of ?

Jon,

Any suggestions here?

Sorry completed this missed this!

Unfortunately, I don't have any good suggestions here. However, I am wondering if the suspend/resume handlers can just be calls to omap_hwmod_idle/enable and we can remove these __omap_dm_timer_xxxx calls (I don't think they are needed). Furthermore, my understanding is this is only needed for AM335x (per the changelog), and so we should not add suspend/resume handlers for all OMAP2+ based devices.

Quoted 2 lines

I agree with the direction.

I need to retain the call to enable the interrupt but the others can be dropped. Will take care of this and the comment on invoking the suspend/resume handlers only for AM335x in the next version.

Ok fair enough. By the way, I posted a patch today [1] that will use the hwmod name as the clockevent timer name. Care to try on top of that patch and then we can eliminate the sprintf.

The current OMAP timer code registers two timers - one as clocksource and one as clockevent. AM33XX has only one usable timer in the WKUP domain so one of the timers needs suspend-resume support to restore the configuration to pre-suspend state.

commit adc78e6 (timekeeping: Add suspend and resume of clock event devices) introduced .suspend and .resume callbacks for clock event devices. Leverages these callbacks to have AM33XX clockevent timer which is in not in WKUP domain to behave properly across system suspend.

Signed-off-by Vaibhav Bedia <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Benoit Cousson <[email protected]>

Cc Paul Walmsley <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

Cc Jon Hunter <[email protected]>

---
v1->v2:
Get rid of harcoded timer id.
Note: since a platform device is not created for these timer instances and because there's very minimal change needed for restarting the timer a full blown context save and restore has been skipped.

 arch/arm/mach-omap2/timer.c |   33 +++++++++++++++++++++++++++++++++
 1 files changed, 33 insertions(+), 0 deletions(-)

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

index 691aa67..38f9cbc 100644
@@ -128,6 +128,36 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode,
 	}
 }
 
+static void omap_clkevt_suspend(struct clock_event_device *unused)

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

+	omap_hwmod_idle(oh);
+}
+
+static void omap_clkevt_resume(struct clock_event_device *unused)
+{
+	char name[10];
+	struct omap_hwmod *oh;

Quoted 39 lines · Vaibhav Bedia

The current OMAP timer code registers two timers - one as clocksource and one as clockevent. AM33XX has only one usable timer in the WKUP domain so one of the timers needs suspend-resume support to restore the configuration to pre-suspend state.

commit adc78e6 (timekeeping: Add suspend and resume of clock event devices) introduced .suspend and .resume callbacks for clock event devices. Leverages these callbacks to have AM33XX clockevent timer which is in not in WKUP domain to behave properly across system suspend.

Signed-off-by Vaibhav Bedia <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Benoit Cousson <[email protected]>

Cc Paul Walmsley <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

Cc Jon Hunter <[email protected]>

---
v1->v2:
Get rid of harcoded timer id.
Note: since a platform device is not created for these timer instances and because there's very minimal change needed for restarting the timer a full blown context save and restore has been skipped.

 arch/arm/mach-omap2/timer.c |   33 +++++++++++++++++++++++++++++++++
 1 files changed, 33 insertions(+), 0 deletions(-)

arch/arm/mach-omap2/timer.c +0 -0

index 691aa67..38f9cbc 100644
@@ -128,6 +128,36 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode,
 	}
 }
 

arch/arm/mach-omap2/timer.c static void omap2_gp_timer_set_mode(enum clock_event_mode mode, · line 141

Quoted 11 lines

arch/arm/mach-omap2/timer.c +11 -0

+static void omap_clkevt_suspend(struct clock_event_device *unused)
+{
+	char name[10];
+	struct omap_hwmod *oh;
+
+	sprintf(name, "timer%d", clkev.id);
+	oh = omap_hwmod_lookup(name);
+	if (!oh)
+		return;
+
+	__omap_dm_timer_stop(&clkev, 1, clkev.rate);

I am not sure you need to call __omap_dm_timer_stop() here. This should have already been called as timekeeping_suspend() will call omap2_gp_timer_set_mode() to shutdown the timer.

Quoted 12 lines

arch/arm/mach-omap2/timer.c +11 -0

+	omap_hwmod_idle(oh);
+}
+
+static void omap_clkevt_resume(struct clock_event_device *unused)
+{
+	char name[10];
+	struct omap_hwmod *oh;
+
+	sprintf(name, "timer%d", clkev.id);
+	oh = omap_hwmod_lookup(name);
+	if (!oh)

arch/arm/mach-omap2/timer.c static void omap2_gp_timer_set_mode(enum clock_event_mode mode, · line 158

Quoted 6 lines

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

+		return;
+
+	omap_hwmod_enable(oh);
+	__omap_dm_timer_load_start(&clkev,
+			OMAP_TIMER_CTRL_ST | OMAP_TIMER_CTRL_AR, 0, 1);
+	__omap_dm_timer_int_enable(&clkev, OMAP_TIMER_INT_OVERFLOW);

Similarly here, I am not sure these __omap_dm_timer_xxxx calls are needed.

arch/arm/mach-omap2/timer.c static struct clock_event_device clockevent_gpt = { · line 169

Quoted 12 lines

arch/arm/mach-omap2/timer.c +4 -0

+}
+
 static struct clock_event_device clockevent_gpt = {
 	.name		= "gp_timer",
 	.features       = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,

@@ -135,6 +165,8 @@ static struct clock_event_device clockevent_gpt = {
 	.rating		= 300,
 	.set_next_event	= omap2_gp_timer_set_next_event,
 	.set_mode	= omap2_gp_timer_set_mode,
+	.suspend	= omap_clkevt_suspend,
+	.resume		= omap_clkevt_resume,

AFAIK, this is only applicable for AM335x devices and so should not be added for all.

Quoted 6 lines

arch/arm/mach-omap2/timer.c +0 -0

 };
 
 static struct property device_disabled = {

@@ -323,6 +355,7 @@ static void __init omap2_gp_clockevent_init(int gptimer_id,
 	int res;
 

arch/arm/mach-omap2/timer.c static void __init omap2_gp_clockevent_init(int gptimer_id, · line 358

Quoted 2 lines

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

 	clkev.errata = omap_dm_timer_get_errata();
+	clkev.id = gptimer_id;

We should not use gptimer_id anymore. This will go away once the migration to dev-tree is completed. You may be better off storing the oh_name in the clock_event_device name field and passing to the suspend/resume handlers.

AM33XX has two timers (DTIMER0/1) in the WKUP domain. On GP devices the source of DMTIMER0 is fixed to an inaccurate internal 32k RC oscillator and this makes the DMTIMER0 practically either as a clocksource or as clockevent.

Currently the timer instance in WKUP domain is used as the clockevent and the timer in non-WKUP domain as the clocksource. DMTIMER1 in WKUP domain can keep running in suspend from a 32K clock fed from external OSC and can serve as the persistent clock for the kernel. To enable this, interchange the timers used as clocksource and clockevent for AM33XX.

For now a new DT property has been added to allow the timer code to select the timer with the right property.

It has been pointed out by Santosh Shilimkar and Kevin Hilman that such a change will result in soc-idle never being achieved on AM33XX. There are other reasons why soc-idle does not look feasible on AM33XX so for now we go ahead with the interchange of the the timers. If at a later point of time we do come up with an approach which makes soc-idle possible on AM33XX, this can be revisited.

Quoted 17 lines · Vaibhav Bedia

AM33XX has two timers (DTIMER0/1) in the WKUP domain. On GP devices the source of DMTIMER0 is fixed to an inaccurate internal 32k RC oscillator and this makes the DMTIMER0 practically either as a clocksource or as clockevent.

Currently the timer instance in WKUP domain is used as the clockevent and the timer in non-WKUP domain as the clocksource. DMTIMER1 in WKUP domain can keep running in suspend from a 32K clock fed from external OSC and can serve as the persistent clock for the kernel. To enable this, interchange the timers used as clocksource and clockevent for AM33XX.

For now a new DT property has been added to allow the timer code to select the timer with the right property.

Quoted 8 lines

It has been pointed out by Santosh Shilimkar and Kevin Hilman that such a change will result in soc-idle never being achieved on AM33XX. There are other reasons why soc-idle does not look feasible on AM33XX so for now we go ahead with the interchange of the the timers. If at a later point of time we do come up with an approach which makes soc-idle possible on AM33XX, this can be revisited.

Can you please explain other reasons as well for clarity ?

AM33XX has two timers (DTIMER0/1) in the WKUP domain. On GP devices the source of DMTIMER0 is fixed to an inaccurate internal 32k RC oscillator and this makes the DMTIMER0 practically either as a clocksource or as clockevent.

Currently the timer instance in WKUP domain is used as the clockevent and the timer in non-WKUP domain as the clocksource. DMTIMER1 in WKUP domain can keep running in suspend from a 32K clock fed from external OSC and can serve as the persistent clock for the kernel. To enable this, interchange the timers used as clocksource and clockevent for AM33XX.

For now a new DT property has been added to allow the timer code to select the timer with the right property.

It has been pointed out by Santosh Shilimkar and Kevin Hilman that such a change will result in soc-idle never being achieved on AM33XX. There are other reasons why soc-idle does not look feasible on AM33XX so for now we go ahead with the interchange of the the timers. If at a later point of time we do come up with an approach which makes soc-idle possible on AM33XX, this can be revisited.

Signed-off-by Vaibhav Bedia <[email protected]>

Signed-off-by Vaibhav Hiremath <[email protected]>

Cc Tony Lingren <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Benoit Cousson <[email protected]>

Cc Paul Walmsley <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Jon Hunter <[email protected]>

---
v1->v2:
Use DT properties for changing the timers

 .../devicetree/bindings/arm/omap/timer.txt         |    2 ++
 arch/arm/boot/dts/am33xx.dtsi                      |    1 +
 arch/arm/mach-omap2/timer.c                        |    6 +++---
 3 files changed, 6 insertions(+), 3 deletions(-)

Documentation/devicetree/bindings/arm/omap/timer.txt +0 -0

index 8732d4d..62d4f2c 100644

arch/arm/boot/dts/am33xx.dtsi +0 -0

 		};
 
 		timer3: timer at 48042000 {

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

index 38f9cbc..cfb3413 100644
@@ -264,7 +264,7 @@ static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer,
 	int r = 0;
 
 	if (of_have_populated_dt()) {
-		np = omap_get_timer_dt(omap_timer_match, NULL);
+		np = omap_get_timer_dt(omap_timer_match, property);
 		if (!np)
 			return -ENODEV;
 

AM33XX has two timers (DTIMER0/1) in the WKUP domain. On GP devices the source of DMTIMER0 is fixed to an inaccurate internal 32k RC oscillator and this makes the DMTIMER0 practically either as a clocksource or as clockevent.

Currently the timer instance in WKUP domain is used as the clockevent and the timer in non-WKUP domain as the clocksource. DMTIMER1 in WKUP domain can keep running in suspend from a 32K clock fed from external OSC and can serve as the persistent clock for the kernel. To enable this, interchange the timers used as clocksource and clockevent for AM33XX.

For now a new DT property has been added to allow the timer code to select the timer with the right property.

It has been pointed out by Santosh Shilimkar and Kevin Hilman that such a change will result in soc-idle never being achieved on AM33XX. There are other reasons why soc-idle does not look feasible on AM33XX so for now we go ahead with the interchange of the the timers. If at a later point of time we do come up with an approach which makes soc-idle possible on AM33XX, this can be revisited.

Hi Vaibhav,

Quoted 6 lines · Vaibhav Bedia

AM33XX has two timers (DTIMER0/1) in the WKUP domain. On GP devices the source of DMTIMER0 is fixed to an inaccurate internal 32k RC oscillator and this makes the DMTIMER0 practically either as a clocksource or as clockevent.

Quoted 7 lines

Currently the timer instance in WKUP domain is used as the clockevent and the timer in non-WKUP domain as the clocksource. DMTIMER1 in WKUP domain can keep running in suspend from a 32K clock fed from external OSC and can serve as the persistent clock for the kernel. To enable this, interchange the timers used as clocksource and clockevent for AM33XX.

I have been thinking about this some more. In the case where we are using gptimers for both clock-events and clock-source (on both AM33xx and OMAP) and I am wondering if it makes sense to switch the timers so that we use the always-on timer for clock-source and a different one from clock-events.

For OMAP, if we are not using the 32k-sync for clock-source, then we are never going to achieve low power states during idle as we will always have one gptimer running. And in this case, to your point below, it would be better to use the always-on for clock-source so that in suspend we can at least hit low power states and maintain time.

Quoted 23 lines · Jon Hunter

Hi Vaibhav,

Quoted 6 lines · Vaibhav Bedia

AM33XX has two timers (DTIMER0/1) in the WKUP domain. On GP devices the source of DMTIMER0 is fixed to an inaccurate internal 32k RC oscillator and this makes the DMTIMER0 practically either as a clocksource or as clockevent.

Quoted 7 lines

Currently the timer instance in WKUP domain is used as the clockevent and the timer in non-WKUP domain as the clocksource. DMTIMER1 in WKUP domain can keep running in suspend from a 32K clock fed from external OSC and can serve as the persistent clock for the kernel. To enable this, interchange the timers used as clocksource and clockevent for AM33XX.

I have been thinking about this some more. In the case where we are using gptimers for both clock-events and clock-source (on both AM33xx and OMAP) and I am wondering if it makes sense to switch the timers so that we use the always-on timer for clock-source and a different one from clock-events.

Quoted 5 lines

For OMAP, if we are not using the 32k-sync for clock-source, then we are never going to achieve low power states during idle as we will always have one gptimer running. And in this case, to your point below, it would be better to use the always-on for clock-source so that in suspend we can at least hit low power states and maintain time.

I have posted a patch today [1] that I hope will address this issue for you. Can you give that a try?

By the way, this need to be applied on top of the fix I sent yesterday to pass the property.

Add minimal APIs for writing to the IPC and the M3_TXEV registers in the Control module. These will be used in a subsequent patch which adds suspend-resume support for AM33XX.

Signed-off-by Vaibhav Bedia <[email protected]>

Cc Tony Lingren <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Benoit Cousson <[email protected]>

Cc Paul Walmsley <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

---

Quoted 4 lines · Vaibhav Bedia

Add minimal APIs for writing to the IPC and the M3_TXEV registers in the Control module. These will be used in a subsequent patch which adds suspend-resume support for AM33XX.

Quoted 8 lines

Signed-off-by Vaibhav Bedia <[email protected]>

Cc Tony Lingren <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Benoit Cousson <[email protected]>

Cc Paul Walmsley <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

---

On Control module, we are trying to move driver/module specific code to respective drivers. Can you not add below code to ipc related driver component.

If not, then patch as such is fine with me.

Quoted 17 lines · Santosh

Quoted 4 lines · Vaibhav Bedia

Add minimal APIs for writing to the IPC and the M3_TXEV registers in the Control module. These will be used in a subsequent patch which adds suspend-resume support for AM33XX.

Quoted 8 lines

Signed-off-by Vaibhav Bedia <[email protected]>

Cc Tony Lingren <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Benoit Cousson <[email protected]>

Cc Paul Walmsley <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

---

On Control module, we are trying to move driver/module specific code to respective drivers. Can you not add below code to ipc related driver component.

Quoted 2 lines

If not, then patch as such is fine with me.

I had it in the pm33xx.c in the previous version. Kevin had asked me to move it to control.c. Should I revert move it back there?

Quoted 18 lines · Vaibhav

Quoted 17 lines · Santosh

Quoted 4 lines · Vaibhav Bedia

Add minimal APIs for writing to the IPC and the M3_TXEV registers in the Control module. These will be used in a subsequent patch which adds suspend-resume support for AM33XX.

Quoted 8 lines

Signed-off-by Vaibhav Bedia <[email protected]>

Cc Tony Lingren <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Benoit Cousson <[email protected]>

Cc Paul Walmsley <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

---

On Control module, we are trying to move driver/module specific code to respective drivers. Can you not add below code to ipc related driver component.

Quoted 6 lines

Quoted 2 lines

If not, then patch as such is fine with me.

I had it in the pm33xx.c in the previous version. Kevin had asked me to move it to control.c. Should I revert move it back there?

pm33xx.c is not the right place. I was hoping to move to some driver. If that is not possible then leave it in control module.

Quoted 22 lines · Santosh

Quoted 21 lines · Vaibhav

Quoted 19 lines · Santosh

Quoted 4 lines · Vaibhav Bedia

Add minimal APIs for writing to the IPC and the M3_TXEV registers in the Control module. These will be used in a subsequent patch which adds suspend-resume support for AM33XX.

Quoted 8 lines

Signed-off-by Vaibhav Bedia <[email protected]>

Cc Tony Lingren <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Benoit Cousson <[email protected]>

Cc Paul Walmsley <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Vaibhav Hiremath <[email protected]>

---

On Control module, we are trying to move driver/module specific code to respective drivers. Can you not add below code to ipc related driver component.

If not, then patch as such is fine with me.

Quoted 6 lines

Quoted 3 lines

I had it in the pm33xx.c in the previous version. Kevin had asked me to move it to control.c. Should I revert move it back there?

pm33xx.c is not the right place. I was hoping to move to some driver. If that is not possible then leave it in control module.

I think I'll leave it here for now. Could I have you ack in that case?