ARM: OMAP2+: Remove suspend_set_ops from common pm late init

PATCH New Dave Gerlach 2 participants 4 messages May 9, 2014

Subsystems (4)

Arm (32-bit) Arm sub-architectures OMAP2+ support OMAP power management support

lore

Share & tools

lore

Dave Gerlach

author

2014-05-09 18:53 UTC #0 lore raw

PATCH 1/1 ARM: OMAP2+: Remove suspend_set_ops from common pm late init

In omap2_common_pm_late_init suspend_set_ops was called to set common suspend handling functions for all omap platforms. This created two problems. First, these suspend ops were being set for all platforms, regardless of whether or not suspend support has been integrated so in the case of AM33XX, suspend to mem was presented as available but failed every time. Second, some platforms will need to define a completely separate set of suspend ops, such as AM33XX, due to differences from previous omap platforms so there is no need to always set the common omap ops.

This patch moves the suspend_set_ops call from omap2_common_pm_late_init into a separate function that then gets called in the omap*_pm_init functions for each platform.

Signed-off-by Dave Gerlach <[email protected]>

---
 arch/arm/mach-omap2/common.h | 8 ++++++++
 arch/arm/mach-omap2/pm.c     | 8 ++++----
 arch/arm/mach-omap2/pm24xx.c | 1 +
 arch/arm/mach-omap2/pm34xx.c | 1 +
 arch/arm/mach-omap2/pm44xx.c | 1 +
 5 files changed, 15 insertions(+), 4 deletions(-)

arch/arm/mach-omap2/common.h +8 -0

index d88aff7..091e1c7 100644
@@ -162,6 +162,14 @@ static inline void omap44xx_restart(enum reboot_mode mode, const char *cmd)
 }
 #endif
 
+#ifdef CONFIG_SUSPEND
+void omap_common_suspend_init(void);
+#else
+static inline void omap_common_suspend_init(void)
+{
+}
+#endif
+
 /* This gets called from mach-omap2/io.c, do not call this */
 void __init omap2_set_globals_tap(u32 class, void __iomem *tap);

arch/arm/mach-omap2/pm.c +4 -4

index e1b4141..c9b36fb 100644
@@ -243,6 +243,10 @@ static const struct platform_suspend_ops omap_pm_ops = {
 	.valid		= suspend_valid_only_mem,
 };
 
+void __init omap_common_suspend_init(void)
+{
+	suspend_set_ops(&omap_pm_ops);
+}
 #endif /* CONFIG_SUSPEND */
 
 static void __init omap3_init_voltages(void)

@@ -310,9 +314,5 @@ int __init omap2_common_pm_late_init(void)
 	/* cpufreq dummy device instantiation */
 	omap_init_cpufreq();
 
-#ifdef CONFIG_SUSPEND
-	suspend_set_ops(&omap_pm_ops);
-#endif
-
 	return 0;
 }

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

index 8c07594..8cd2408 100644
@@ -231,6 +231,7 @@ static void __init prcm_setup_regs(void)
 
 #ifdef CONFIG_SUSPEND
 	omap_pm_suspend = omap2_enter_full_retention;
+	omap_common_suspend_init();
 #endif
 
 	/* REVISIT: Configure number of 32 kHz clock cycles for sys_clk

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

index 87099bb..32b2597 100644
@@ -707,6 +707,7 @@ int __init omap3_pm_init(void)
 
 #ifdef CONFIG_SUSPEND
 	omap_pm_suspend = omap3_pm_suspend;
+	omap_common_suspend_init();
 #endif
 
 	arm_pm_idle = omap3_pm_idle;

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

index eefb30c..58d7cd7 100644
@@ -253,6 +253,7 @@ int __init omap4_pm_init(void)
 
 #ifdef CONFIG_SUSPEND
 	omap_pm_suspend = omap4_pm_suspend;
+	omap_common_suspend_init();
 #endif
 
 	/* Overwrite the default cpu_do_idle() */

Signature · 1 line

1.9.0

Nishanth Menon

2014-05-09 19:04 UTC #1 lore raw

PATCH 1/1 ARM: OMAP2+: Remove suspend_set_ops from common pm late init

Quoted 29 lines · Dave Gerlach

In omap2_common_pm_late_init suspend_set_ops was called to set common suspend handling functions for all omap platforms. This created two problems. First, these suspend ops were being set for all platforms, regardless of whether or not suspend support has been integrated so in the case of AM33XX, suspend to mem was presented as available but failed every time. Second, some platforms will need to define a completely separate set of suspend ops, such as AM33XX, due to differences from previous omap platforms so there is no need to always set the common omap ops.
This patch moves the suspend_set_ops call from omap2_common_pm_late_init into a separate function that then gets called in the omap*_pm_init functions for each platform.
Signed-off-by Dave Gerlach <[email protected]>
---
 arch/arm/mach-omap2/common.h | 8 ++++++++
 arch/arm/mach-omap2/pm.c     | 8 ++++----
 arch/arm/mach-omap2/pm24xx.c | 1 +
 arch/arm/mach-omap2/pm34xx.c | 1 +
 arch/arm/mach-omap2/pm44xx.c | 1 +
 5 files changed, 15 insertions(+), 4 deletions(-)
arch/arm/mach-omap2/common.h +0 -0
index d88aff7..091e1c7 100644
@@ -162,6 +162,14 @@ static inline void omap44xx_restart(enum reboot_mode mode, const char *cmd)
 }

arch/arm/mach-omap2/common.h static inline void omap44xx_restart(enum reboot_mode mode, const char *cmd) · line 172

Quoted 10 lines

arch/arm/mach-omap2/common.h +8 -0

 #endif
 
+#ifdef CONFIG_SUSPEND
+void omap_common_suspend_init(void);
+#else
+static inline void omap_common_suspend_init(void)
+{
+}
+#endif
+

push this off to this pm.h?

arch/arm/mach-omap2/pm.c static const struct platform_suspend_ops omap_pm_ops = { · line 246

Quoted 12 lines

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

 /* This gets called from mach-omap2/io.c, do not call this */
 void __init omap2_set_globals_tap(u32 class, void __iomem *tap);

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

index e1b4141..c9b36fb 100644
@@ -243,6 +243,10 @@ static const struct platform_suspend_ops omap_pm_ops = {
 	.valid		= suspend_valid_only_mem,
 };
 
+void __init omap_common_suspend_init(void)

provide kernel doc please? and do we need __init?

Quoted 45 lines

arch/arm/mach-omap2/pm.c +3 -4

+{
+	suspend_set_ops(&omap_pm_ops);
+}
 #endif /* CONFIG_SUSPEND */
 
 static void __init omap3_init_voltages(void)

@@ -310,9 +314,5 @@ int __init omap2_common_pm_late_init(void)
 	/* cpufreq dummy device instantiation */
 	omap_init_cpufreq();
 
-#ifdef CONFIG_SUSPEND
-	suspend_set_ops(&omap_pm_ops);
-#endif
-
 	return 0;
 }

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

index 8c07594..8cd2408 100644
@@ -231,6 +231,7 @@ static void __init prcm_setup_regs(void)
 
 #ifdef CONFIG_SUSPEND
 	omap_pm_suspend = omap2_enter_full_retention;
+	omap_common_suspend_init();
 #endif
 
 	/* REVISIT: Configure number of 32 kHz clock cycles for sys_clk

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

index 87099bb..32b2597 100644
@@ -707,6 +707,7 @@ int __init omap3_pm_init(void)
 
 #ifdef CONFIG_SUSPEND
 	omap_pm_suspend = omap3_pm_suspend;
+	omap_common_suspend_init();
 #endif
 
 	arm_pm_idle = omap3_pm_idle;

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

index eefb30c..58d7cd7 100644

arch/arm/mach-omap2/pm44xx.c int __init omap4_pm_init(void) · line 256

Quoted 4 lines

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

@@ -253,6 +253,7 @@ int __init omap4_pm_init(void)
 
 #ifdef CONFIG_SUSPEND
 	omap_pm_suspend = omap4_pm_suspend;
+	omap_common_suspend_init();

do we need to expose omap_pm_suspend anymore with this since the original purpose was to make pm.c know about the suspend function? How about:
omap_common_suspend_init(omap4_pm_suspend);
should do the job, right? and with that, you dont need the #ifdef CONFIG_SUSPEND either in each of the pmxxx.c files since you already deal with it in header.

Quoted 4 lines

arch/arm/mach-omap2/pm44xx.c +0 -0
 #endif
 
 	/* Overwrite the default cpu_do_idle() */

Signature · 2 lines

Regards,
Nishanth Menon

Dave Gerlach

author

2014-05-09 20:17 UTC #2 lore raw

PATCH 1/1 ARM: OMAP2+: Remove suspend_set_ops from common pm late init

Quoted 32 lines · Nishanth Menon

Quoted 31 lines · Dave Gerlach
In omap2_common_pm_late_init suspend_set_ops was called to set common suspend handling functions for all omap platforms. This created two problems. First, these suspend ops were being set for all platforms, regardless of whether or not suspend support has been integrated so in the case of AM33XX, suspend to mem was presented as available but failed every time. Second, some platforms will need to define a completely separate set of suspend ops, such as AM33XX, due to differences from previous omap platforms so there is no need to always set the common omap ops.
This patch moves the suspend_set_ops call from omap2_common_pm_late_init into a separate function that then gets called in the omap*_pm_init functions for each platform.
Signed-off-by Dave Gerlach <[email protected]>
---
  arch/arm/mach-omap2/common.h | 8 ++++++++
  arch/arm/mach-omap2/pm.c     | 8 ++++----
  arch/arm/mach-omap2/pm24xx.c | 1 +
  arch/arm/mach-omap2/pm34xx.c | 1 +
  arch/arm/mach-omap2/pm44xx.c | 1 +
  5 files changed, 15 insertions(+), 4 deletions(-)
arch/arm/mach-omap2/common.h +0 -0
index d88aff7..091e1c7 100644
@@ -162,6 +162,14 @@ static inline void omap44xx_restart(enum reboot_mode mode, const char *cmd)
  }
  #endif
 

Quoted 9 lines

arch/arm/mach-omap2/common.h static inline void omap44xx_restart(enum reboot_mode mode, const char *cmd) · line 172
Quoted 8 lines
arch/arm/mach-omap2/common.h +8 -0
+#ifdef CONFIG_SUSPEND
+void omap_common_suspend_init(void);
+#else
+static inline void omap_common_suspend_init(void)
+{
+}
+#endif
+
push this off to this pm.h?

I could, I had just put it in the same spot as omap2_common_pm_late_init. I agree it makes sense to move it though.

Quoted 12 lines

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

  /* This gets called from mach-omap2/io.c, do not call this */
  void __init omap2_set_globals_tap(u32 class, void __iomem *tap);

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

index e1b4141..c9b36fb 100644
@@ -243,6 +243,10 @@ static const struct platform_suspend_ops omap_pm_ops = {
  	.valid		= suspend_valid_only_mem,
  };

Quoted 2 lines

arch/arm/mach-omap2/pm.c static const struct platform_suspend_ops omap_pm_ops = { · line 246
Quoted 1 line
arch/arm/mach-omap2/pm.c +1 -0
+void __init omap_common_suspend_init(void)
provide kernel doc please? and do we need __init?

Added and no.

Quoted 47 lines

arch/arm/mach-omap2/pm.c +3 -4

+{
+	suspend_set_ops(&omap_pm_ops);
+}
  #endif /* CONFIG_SUSPEND */
 
  static void __init omap3_init_voltages(void)

@@ -310,9 +314,5 @@ int __init omap2_common_pm_late_init(void)
  	/* cpufreq dummy device instantiation */
  	omap_init_cpufreq();
 
-#ifdef CONFIG_SUSPEND
-	suspend_set_ops(&omap_pm_ops);
-#endif
-
  	return 0;
  }

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

index 8c07594..8cd2408 100644
@@ -231,6 +231,7 @@ static void __init prcm_setup_regs(void)
 
  #ifdef CONFIG_SUSPEND
  	omap_pm_suspend = omap2_enter_full_retention;
+	omap_common_suspend_init();
  #endif
 
  	/* REVISIT: Configure number of 32 kHz clock cycles for sys_clk

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

index 87099bb..32b2597 100644
@@ -707,6 +707,7 @@ int __init omap3_pm_init(void)
 
  #ifdef CONFIG_SUSPEND
  	omap_pm_suspend = omap3_pm_suspend;
+	omap_common_suspend_init();
  #endif
 
  	arm_pm_idle = omap3_pm_idle;

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

index eefb30c..58d7cd7 100644
@@ -253,6 +253,7 @@ int __init omap4_pm_init(void)

Quoted 11 lines

arch/arm/mach-omap2/pm44xx.c int __init omap4_pm_init(void) · line 256
Quoted 3 lines
arch/arm/mach-omap2/pm44xx.c +1 -0
  #ifdef CONFIG_SUSPEND
  	omap_pm_suspend = omap4_pm_suspend;
+	omap_common_suspend_init();
do we need to expose omap_pm_suspend anymore with this since the original purpose was to make pm.c know about the suspend function? How about:
omap_common_suspend_init(omap4_pm_suspend);
should do the job, right? and with that, you dont need the #ifdef CONFIG_SUSPEND either in each of the pmxxx.c files since you already deal with it in header.

Yes, I like this idea myself.

Thanks for the comments,
Dave

Quoted 7 lines

Quoted 5 lines
arch/arm/mach-omap2/pm44xx.c +0 -0
  #endif
 
  	/* Overwrite the default cpu_do_idle() */

Nishanth Menon

2014-05-09 20:38 UTC #3 lore raw

PATCH 1/1 ARM: OMAP2+: Remove suspend_set_ops from common pm late init

Quoted 16 lines · Dave Gerlach

Quoted 13 lines · Nishanth Menon
arch/arm/mach-omap2/common.h static inline void omap44xx_restart(enum reboot_mode mode, const char *cmd) · line 172
Quoted 8 lines · Dave Gerlach
arch/arm/mach-omap2/common.h +8 -0
+#ifdef CONFIG_SUSPEND
+void omap_common_suspend_init(void);
+#else
+static inline void omap_common_suspend_init(void)
+{
+}
+#endif
+
push this off to this pm.h?

Quoted 3 lines

I could, I had just put it in the same spot as omap2_common_pm_late_init. I agree it makes sense to move it though.

yeah - the usage in the case of suspend_init is restricted solely to pm code.. generic code does not have to invoke it.

Regards,
Nishanth Menon