Patchwork [2/3] ARM: tegra: refactor the pmc_pm_set function

login
register
mail settings
Submitter Joseph Lo
Date March 13, 2013, 8:27 a.m.
Message ID <1363163246-26368-2-git-send-email-josephl@nvidia.com>
Download mbox | patch
Permalink /patch/227188/
State Superseded, archived
Headers show

Comments

Joseph Lo - March 13, 2013, 8:27 a.m.
The pmc_pm_set function was designed for SoC to configure the related
settings when system going to some low power modes (e.g. platform
suspend or CPU idle powered-down mode). We refactor the function to make
it work on the usage.

Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
This series was based on the patch set of suspending support.
---
 arch/arm/mach-tegra/pm.c  | 18 ++----------------
 arch/arm/mach-tegra/pmc.c | 38 ++++++++++++++++++--------------------
 arch/arm/mach-tegra/pmc.h |  5 +----
 3 files changed, 21 insertions(+), 40 deletions(-)
Stephen Warren - March 13, 2013, 5:52 p.m.
On 03/13/2013 02:27 AM, Joseph Lo wrote:
> The pmc_pm_set function was designed for SoC to configure the related
> settings when system going to some low power modes (e.g. platform
> suspend or CPU idle powered-down mode). We refactor the function to make
> it work on the usage.

> diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c

> +#include <linux/clk-provider.h>

This code isn't a clock-provider, and hence shouldn't include that
header file.

> +void tegra_pmc_pm_set(enum tegra_suspend_mode mode)

> +	switch (mode) {
> +	case TEGRA_SUSPEND_LP2:
> +		rate = __clk_get_rate(tegra_pclk);

Doesn't regular clk_get_rate() work here?

> @@ -234,6 +229,9 @@ void tegra_pmc_suspend_init(void)
>  {
>  	u32 reg;
>  
> +	tegra_pclk = clk_get_sys(NULL, "pclk");
> +	WARN_ON(IS_ERR(tegra_pclk));

Shouldn't this instead error out and/or disable any system suspend
modes? Otherwise, tegra_pmc_pm_set() is going to call
clk_get_rate(tegra_pclk), and tegra_pclk won't be a valid clock object.

Also, can you use regular clk_get() rather than clk_get_sys()? That'd
need a clocks property in the PMC DT node.

I guess I see now that this series does actually depend on the suspend
series. However, stuff like:

> -u32 tegra_pmc_get_cpu_good_time(void)
> -{
> -	return pmc_pm_data.cpu_good_time;
> -}
> -
> -u32 tegra_pmc_get_cpu_off_time(void)
> -{
> -	return pmc_pm_data.cpu_off_time;
> -}

... was actually added in that series, so if you put this series first,
or merged the two series with these patches first, you could end up
avoiding some churn.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joseph Lo - March 14, 2013, 1:44 a.m.
On Thu, 2013-03-14 at 01:52 +0800, Stephen Warren wrote:
> On 03/13/2013 02:27 AM, Joseph Lo wrote:
> > The pmc_pm_set function was designed for SoC to configure the related
> > settings when system going to some low power modes (e.g. platform
> > suspend or CPU idle powered-down mode). We refactor the function to make
> > it work on the usage.
> 
> > diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
> 
> > +#include <linux/clk-provider.h>
> 
> This code isn't a clock-provider, and hence shouldn't include that
> header file.
> 
> > +void tegra_pmc_pm_set(enum tegra_suspend_mode mode)
> 
> > +	switch (mode) {
> > +	case TEGRA_SUSPEND_LP2:
> > +		rate = __clk_get_rate(tegra_pclk);
> 
> Doesn't regular clk_get_rate() work here?
> 
Actually it works. So I will use clk_get_rate() here and remove
clk-provider.

> > @@ -234,6 +229,9 @@ void tegra_pmc_suspend_init(void)
> >  {
> >  	u32 reg;
> >  
> > +	tegra_pclk = clk_get_sys(NULL, "pclk");
> > +	WARN_ON(IS_ERR(tegra_pclk));
> 
> Shouldn't this instead error out and/or disable any system suspend
> modes? Otherwise, tegra_pmc_pm_set() is going to call
> clk_get_rate(tegra_pclk), and tegra_pclk won't be a valid clock object.
OK.
> 
> Also, can you use regular clk_get() rather than clk_get_sys()? That'd
> need a clocks property in the PMC DT node.
OK.
> I guess I see now that this series does actually depend on the suspend
> series. However, stuff like:
> 
> > -u32 tegra_pmc_get_cpu_good_time(void)
> > -{
> > -	return pmc_pm_data.cpu_good_time;
> > -}
> > -
> > -u32 tegra_pmc_get_cpu_off_time(void)
> > -{
> > -	return pmc_pm_data.cpu_off_time;
> > -}
> 
> ... was actually added in that series, so if you put this series first,
> or merged the two series with these patches first, you could end up
> avoiding some churn.
Let me check how to reorganize them.

Thanks,
Joseph

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

Patch

diff --git a/arch/arm/mach-tegra/pm.c b/arch/arm/mach-tegra/pm.c
index b7cc637..2a04dfc 100644
--- a/arch/arm/mach-tegra/pm.c
+++ b/arch/arm/mach-tegra/pm.c
@@ -40,13 +40,8 @@ 
 #include "sleep.h"
 #include "pmc.h"
 
-#define TEGRA_POWER_CPU_PWRREQ_OE	(1 << 16)  /* CPU pwr req enable */
-
-#define PMC_CTRL		0x0
-
 #ifdef CONFIG_PM_SLEEP
 static DEFINE_SPINLOCK(tegra_lp2_lock);
-static void __iomem *pmc = IO_ADDRESS(TEGRA_PMC_BASE);
 void (*tegra_tear_down_cpu)(void);
 
 /*
@@ -146,14 +141,7 @@  static int tegra_sleep_cpu(unsigned long v2p)
 
 void tegra_idle_lp2_last(u32 cpu_on_time, u32 cpu_off_time)
 {
-	u32 mode;
-
-	/* Only the last cpu down does the final suspend steps */
-	mode = readl(pmc + PMC_CTRL);
-	mode |= TEGRA_POWER_CPU_PWRREQ_OE;
-	writel(mode, pmc + PMC_CTRL);
-
-	set_power_timers(cpu_on_time, cpu_off_time);
+	tegra_pmc_pm_set(TEGRA_SUSPEND_LP2);
 
 	cpu_cluster_pm_enter();
 	suspend_cpu_complex();
@@ -181,9 +169,7 @@  static int __cpuinit tegra_suspend_enter(suspend_state_t state)
 
 	pr_info("Entering suspend state %s\n", lp_state[mode]);
 
-	tegra_pmc_pm_set();
-	set_power_timers(tegra_pmc_get_cpu_good_time(),
-			 tegra_pmc_get_cpu_off_time());
+	tegra_pmc_pm_set(mode);
 
 	local_fiq_disable();
 
diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
index bb1fe8e..118a465 100644
--- a/arch/arm/mach-tegra/pmc.c
+++ b/arch/arm/mach-tegra/pmc.c
@@ -20,6 +20,7 @@ 
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 
 #include "fuse.h"
 #include "pmc.h"
@@ -164,20 +165,12 @@  int tegra_pmc_cpu_remove_clamping(int cpuid)
 #ifdef CONFIG_PM_SLEEP
 static struct clk *tegra_pclk;
 
-void set_power_timers(unsigned long us_on, unsigned long us_off)
+static void set_power_timers(u32 us_on, u32 us_off, unsigned long rate)
 {
 	unsigned long long ticks;
 	unsigned long long pclk;
-	unsigned long rate;
 	static unsigned long tegra_last_pclk;
 
-	if (tegra_pclk == NULL) {
-		tegra_pclk = clk_get_sys(NULL, "pclk");
-		WARN_ON(IS_ERR(tegra_pclk));
-	}
-
-	rate = clk_get_rate(tegra_pclk);
-
 	if (WARN_ON_ONCE(rate <= 0))
 		pclk = 100000000;
 	else
@@ -209,24 +202,26 @@  void tegra_pmc_set_suspend_mode(enum tegra_suspend_mode mode)
 	pmc_pm_data.suspend_mode = mode;
 }
 
-u32 tegra_pmc_get_cpu_good_time(void)
-{
-	return pmc_pm_data.cpu_good_time;
-}
-
-u32 tegra_pmc_get_cpu_off_time(void)
-{
-	return pmc_pm_data.cpu_off_time;
-}
-
-void tegra_pmc_pm_set(void)
+void tegra_pmc_pm_set(enum tegra_suspend_mode mode)
 {
 	u32 reg;
+	unsigned long rate = 0;
 
 	reg = tegra_pmc_readl(PMC_CTRL);
 	reg |= TEGRA_POWER_CPU_PWRREQ_OE;
 	reg &= ~TEGRA_POWER_EFFECT_LP0;
 
+	switch (mode) {
+	case TEGRA_SUSPEND_LP2:
+		rate = __clk_get_rate(tegra_pclk);
+		break;
+	default:
+		break;
+	}
+
+	set_power_timers(pmc_pm_data.cpu_good_time, pmc_pm_data.cpu_off_time,
+			 rate);
+
 	tegra_pmc_writel(reg, PMC_CTRL);
 }
 
@@ -234,6 +229,9 @@  void tegra_pmc_suspend_init(void)
 {
 	u32 reg;
 
+	tegra_pclk = clk_get_sys(NULL, "pclk");
+	WARN_ON(IS_ERR(tegra_pclk));
+
 	/* Always enable CPU power request; just normal polarity is supported */
 	reg = tegra_pmc_readl(PMC_CTRL);
 	BUG_ON(reg & TEGRA_POWER_CPU_PWRREQ_POLARITY);
diff --git a/arch/arm/mach-tegra/pmc.h b/arch/arm/mach-tegra/pmc.h
index 9413a44..0ee22d9 100644
--- a/arch/arm/mach-tegra/pmc.h
+++ b/arch/arm/mach-tegra/pmc.h
@@ -27,12 +27,9 @@  enum tegra_suspend_mode {
 };
 
 #ifdef CONFIG_PM_SLEEP
-void set_power_timers(unsigned long us_on, unsigned long us_off);
 enum tegra_suspend_mode tegra_pmc_get_suspend_mode(void);
 void tegra_pmc_set_suspend_mode(enum tegra_suspend_mode mode);
-u32 tegra_pmc_get_cpu_good_time(void);
-u32 tegra_pmc_get_cpu_off_time(void);
-void tegra_pmc_pm_set(void);
+void tegra_pmc_pm_set(enum tegra_suspend_mode mode);
 void tegra_pmc_suspend_init(void);
 #endif