[PATCH V2 0/3] ARM: tegra: add clock source for PMC

[PATCH V2 0/3] ARM: tegra: add clock source for PMC

[Joseph Lo]

This series was original introduced to clean up the PM related code after
the system suspending support series. The information about the origianl
patch set is below.

[1/3] ARM: tegra: moving the CPU power timer function to PMC driver
[2/3] ARM: tegra: refactor the pmc_pm_set function
[3/3] ARM: tegra: cpuidle: remove redundant parameters for powered-down mode

I split them up in V2. The [1/3] is integrated into this series. And should
be merged before the system suspending support series.

The [2/3] is squashed into the system suspending support series.

The [3/3] become a stand alone clean up patah right after the system suspending
support series.

Joseph Lo (3):
  ARM: dts: tegra: add clock source for PMC
  ARM: tegra: get PMC clock source from DT
  ARM: tegra: moving the CPU power timer function to PMC driver

 arch/arm/boot/dts/tegra20.dtsi |  1 +
 arch/arm/boot/dts/tegra30.dtsi |  1 +
 arch/arm/mach-tegra/common.c   |  2 +-
 arch/arm/mach-tegra/pm.c       | 37 +------------------------------------
 arch/arm/mach-tegra/pmc.c      | 36 ++++++++++++++++++++++++++++++++++++
 arch/arm/mach-tegra/pmc.h      |  4 ++++
 6 files changed, 44 insertions(+), 37 deletions(-)

-- 
1.8.1.5

[PATCH V2 1/3] ARM: dts: tegra: add clock source for PMC

[Joseph Lo]

The clock source of PMC is PCLK. Adding it into DTS for Tegra20 and Tegra30.

Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
V2:
* new in this change
---
 arch/arm/boot/dts/tegra20.dtsi | 1 +
 arch/arm/boot/dts/tegra30.dtsi | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 3183581..4ba9c7b 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -419,6 +419,7 @@
 	pmc {
 		compatible = "nvidia,tegra20-pmc";
 		reg = <0x7000e400 0x400>;
+		clocks = <&tegra_car 110>;
 	};
 
 	memory-controller at 7000f000 {
diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index 11d04fe..7299014 100644
--- a/arch/arm/boot/dts/tegra30.dtsi
+++ b/arch/arm/boot/dts/tegra30.dtsi
@@ -428,6 +428,7 @@
 	pmc {
 		compatible = "nvidia,tegra30-pmc";
 		reg = <0x7000e400 0x400>;
+		clocks = <&tegra_car 218>;
 	};
 
 	memory-controller {
-- 
1.8.1.5

[PATCH V2 1/3] ARM: dts: tegra: add clock source for PMC

[Stephen Warren]

On 03/18/2013 02:09 AM, Joseph Lo wrote:
> The clock source of PMC is PCLK. Adding it into DTS for Tegra20 and Tegra30.

> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi

>  	pmc {
>  		compatible = "nvidia,tegra20-pmc";
>  		reg = <0x7000e400 0x400>;
> +		clocks = <&tegra_car 110>;
>  	};

The DT binding documentation needs to list the set of clocks that must
be present.

Doesn't the PMC also receive a "clk32k_in" from the PMIC, or is that
routed into the CAR, and then into the PMC? Either way, the PMC module
receives that clock somehow. Since there are multiple clocks, that also
means that a clock-names property is required.

[PATCH V2 1/3] ARM: dts: tegra: add clock source for PMC

[Joseph Lo]

On Wed, 2013-03-20 at 00:42 +0800, Stephen Warren wrote:
> On 03/18/2013 02:09 AM, Joseph Lo wrote:
> > The clock source of PMC is PCLK. Adding it into DTS for Tegra20 and Tegra30.
> 
> > diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
> 
> >  	pmc {
> >  		compatible = "nvidia,tegra20-pmc";
> >  		reg = <0x7000e400 0x400>;
> > +		clocks = <&tegra_car 110>;
> >  	};
> 
> The DT binding documentation needs to list the set of clocks that must
> be present.
> 
> Doesn't the PMC also receive a "clk32k_in" from the PMIC, or is that
> routed into the CAR, and then into the PMC? Either way, the PMC module
> receives that clock somehow. Since there are multiple clocks, that also
> means that a clock-names property is required.

Do you mean the DTS below and add it into binding document?

/ SoC dts including file
pmc {
	compatible = "nvidia,tegra20-pmc";
	reg = <0x7000e400 0x400>;
	clocks = <&tegra_car 110>, <&clk32k_in>;
	clock-names= "pclk", "clk32k_in";
};

/ Tegra board dts file

pmic {
	...
	clocks {
		compatible = "simple-bus";
		#address-cells = <1>;
		#size-cells = <0>;

		clk32k_in: clock at 0 {
			compatible = "fixed-clock";
			reg=<0>;
			#clock-cells = <0>;
			clock-frequency = <32768>;
		};
	};
};

[PATCH V2 1/3] ARM: dts: tegra: add clock source for PMC

[Stephen Warren]

On 03/20/2013 04:12 AM, Joseph Lo wrote:
> On Wed, 2013-03-20 at 00:42 +0800, Stephen Warren wrote:
>> On 03/18/2013 02:09 AM, Joseph Lo wrote:
>>> The clock source of PMC is PCLK. Adding it into DTS for Tegra20 and Tegra30.
>>
>>> diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
>>
>>>  	pmc {
>>>  		compatible = "nvidia,tegra20-pmc";
>>>  		reg = <0x7000e400 0x400>;
>>> +		clocks = <&tegra_car 110>;
>>>  	};
>>
>> The DT binding documentation needs to list the set of clocks that must
>> be present.
>>
>> Doesn't the PMC also receive a "clk32k_in" from the PMIC, or is that
>> routed into the CAR, and then into the PMC? Either way, the PMC module
>> receives that clock somehow. Since there are multiple clocks, that also
>> means that a clock-names property is required.
> 
> Do you mean the DTS below and add it into binding document?
> 
> / SoC dts including file
> pmc {
> 	compatible = "nvidia,tegra20-pmc";
> 	reg = <0x7000e400 0x400>;
> 	clocks = <&tegra_car 110>, <&clk32k_in>;
> 	clock-names= "pclk", "clk32k_in";
> };

Yes, that's what should technically be present.

> / Tegra board dts file
> 
> pmic {
> 	...
> 	clocks {
> 		compatible = "simple-bus";
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 
> 		clk32k_in: clock at 0 {
> 			compatible = "fixed-clock";
> 			reg=<0>;
> 			#clock-cells = <0>;
> 			clock-frequency = <32768>;
> 		};
> 	};
> };

That won't work; the PMIC drivers don't enumerate sub-nodes as busss, so
 that clocks node won't ever be processed. Also, the PMIC drivers aren't
clock providers in most cases. It's not quite a correct representation
of the HW, but I would suggest simply creating a top-level "clock" node
for that fixed clock. If we ever have more top-level clocks, we can move
them into a top-level "clocks" node at that time.

Note: If there aren't already any other top-level clock nodes (which I
think is the case), the node can be named just "clock" rather than
"clock at 0", since there are no naming conflicts.

[PATCH V2 2/3] ARM: tegra: get PMC clock source from DT

[Joseph Lo]

The clock source of PMC should be PCLK and gotten from DT.

Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
V2:
* new in this change
---
 arch/arm/mach-tegra/common.c | 2 +-
 arch/arm/mach-tegra/pmc.c    | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-tegra/common.c b/arch/arm/mach-tegra/common.c
index f0315c9..b02ebe7 100644
--- a/arch/arm/mach-tegra/common.c
+++ b/arch/arm/mach-tegra/common.c
@@ -61,6 +61,7 @@ u32 tegra_uart_config[4] = {
 void __init tegra_dt_init_irq(void)
 {
 	tegra_clocks_init();
+	tegra_pmc_init();
 	tegra_init_irq();
 	irqchip_init();
 }
@@ -100,7 +101,6 @@ void __init tegra_init_early(void)
 	tegra_apb_io_init();
 	tegra_init_fuse();
 	tegra_init_cache();
-	tegra_pmc_init();
 	tegra_powergate_init();
 	tegra_hotplug_init();
 }
diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
index b30e921..05259fd 100644
--- a/arch/arm/mach-tegra/pmc.c
+++ b/arch/arm/mach-tegra/pmc.c
@@ -19,6 +19,7 @@
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/clk.h>
 
 #define PMC_CTRL			0x0
 #define PMC_CTRL_INTR_LOW		(1 << 17)
@@ -43,6 +44,7 @@ static DEFINE_SPINLOCK(tegra_powergate_lock);
 
 static void __iomem *tegra_pmc_base;
 static bool tegra_pmc_invert_interrupt;
+static struct clk *tegra_pclk;
 
 static inline u32 tegra_pmc_readl(u32 reg)
 {
@@ -151,6 +153,8 @@ static void tegra_pmc_parse_dt(void)
 
 	tegra_pmc_invert_interrupt = of_property_read_bool(np,
 				     "nvidia,invert-interrupt");
+	tegra_pclk = of_clk_get(np, 0);
+	WARN_ON_ONCE(IS_ERR(tegra_pclk));
 }
 
 void __init tegra_pmc_init(void)
-- 
1.8.1.5

[PATCH V2 2/3] ARM: tegra: get PMC clock source from DT

[Stephen Warren]

On 03/18/2013 02:09 AM, Joseph Lo wrote:
> The clock source of PMC should be PCLK and gotten from DT.
> 
> Signed-off-by: Joseph Lo <josephl@nvidia.com>
> ---
> V2:
> * new in this change

s/change/series/ ???

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

>  void __init tegra_dt_init_irq(void)
>  {
>  	tegra_clocks_init();
> +	tegra_pmc_init();
>  	tegra_init_irq();
>  	irqchip_init();
>  }
> @@ -100,7 +101,6 @@ void __init tegra_init_early(void)
>  	tegra_apb_io_init();
>  	tegra_init_fuse();
>  	tegra_init_cache();
> -	tegra_pmc_init();

This change isn't mentioned in the commit description.

Why is this change needed? We should minimize the amount of code in
tegra_dt_init_irq(), not add more code there.

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

> @@ -151,6 +153,8 @@ static void tegra_pmc_parse_dt(void)
>  
>  	tegra_pmc_invert_interrupt = of_property_read_bool(np,
>  				     "nvidia,invert-interrupt");
> +	tegra_pclk = of_clk_get(np, 0);
> +	WARN_ON_ONCE(IS_ERR(tegra_pclk));

Why WARN_ON_ONCE(); is tegra_pmc_parse_dt() called more than once?

Can the code continue if IS_ERR(tegra_pclk), or is that fatal?

[PATCH V2 2/3] ARM: tegra: get PMC clock source from DT

[Joseph Lo]

On Wed, 2013-03-20 at 00:40 +0800, Stephen Warren wrote:
> On 03/18/2013 02:09 AM, Joseph Lo wrote:
> > The clock source of PMC should be PCLK and gotten from DT.
> > 
> > Signed-off-by: Joseph Lo <josephl@nvidia.com>
> > ---
> > V2:
> > * new in this change
> 
> s/change/series/ ???
> 
> > diff --git a/arch/arm/mach-tegra/common.c b/arch/arm/mach-tegra/common.c
> 
> >  void __init tegra_dt_init_irq(void)
> >  {
> >  	tegra_clocks_init();
> > +	tegra_pmc_init();
> >  	tegra_init_irq();
> >  	irqchip_init();
> >  }
> > @@ -100,7 +101,6 @@ void __init tegra_init_early(void)
> >  	tegra_apb_io_init();
> >  	tegra_init_fuse();
> >  	tegra_init_cache();
> > -	tegra_pmc_init();
> 
> This change isn't mentioned in the commit description.
> 
> Why is this change needed? We should minimize the amount of code in
> tegra_dt_init_irq(), not add more code there.
> 
The clocks only available after tegra_clocks_init(). If I can keep the
PMC dt node somewhere in PMC driver, then I can get the clock later.

> > diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
> 
> > @@ -151,6 +153,8 @@ static void tegra_pmc_parse_dt(void)
> >  
> >  	tegra_pmc_invert_interrupt = of_property_read_bool(np,
> >  				     "nvidia,invert-interrupt");
> > +	tegra_pclk = of_clk_get(np, 0);
> > +	WARN_ON_ONCE(IS_ERR(tegra_pclk));
> 
> Why WARN_ON_ONCE(); is tegra_pmc_parse_dt() called more than once?
> 
> Can the code continue if IS_ERR(tegra_pclk), or is that fatal?
> 

The code is still OK just want to warn people that the PMC DT node
needs PCLK clock info.

[PATCH V2 2/3] ARM: tegra: get PMC clock source from DT

[Stephen Warren]

On 03/20/2013 04:00 AM, Joseph Lo wrote:
> On Wed, 2013-03-20 at 00:40 +0800, Stephen Warren wrote:
>> On 03/18/2013 02:09 AM, Joseph Lo wrote:
>>> The clock source of PMC should be PCLK and gotten from DT.
>>>
>>> Signed-off-by: Joseph Lo <josephl@nvidia.com>
>>> ---
>>> V2:
>>> * new in this change
>>
>> s/change/series/ ???
>>
>>> diff --git a/arch/arm/mach-tegra/common.c b/arch/arm/mach-tegra/common.c
>>
>>>  void __init tegra_dt_init_irq(void)
>>>  {
>>>  	tegra_clocks_init();
>>> +	tegra_pmc_init();
>>>  	tegra_init_irq();
>>>  	irqchip_init();
>>>  }
>>> @@ -100,7 +101,6 @@ void __init tegra_init_early(void)
>>>  	tegra_apb_io_init();
>>>  	tegra_init_fuse();
>>>  	tegra_init_cache();
>>> -	tegra_pmc_init();
>>
>> This change isn't mentioned in the commit description.
>>
>> Why is this change needed? We should minimize the amount of code in
>> tegra_dt_init_irq(), not add more code there.
>
> The clocks only available after tegra_clocks_init(). If I can keep the
> PMC dt node somewhere in PMC driver, then I can get the clock later.

Oh right. For some reason I thought that init_irq was executed before
init_early.

I guess this is OK for now, but I wonder if we shouldn't create a
tegra_init_machine() that tegra.c:tegra_dt_init() can call before
creating all the devices, and have that call both tegra_clocks_init()
and tegra_pmc_init(). But, we can leave that until a later cleanup patch
once we've thought it through a bit more.

>>> diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
>>
>>> @@ -151,6 +153,8 @@ static void tegra_pmc_parse_dt(void)
>>>  
>>>  	tegra_pmc_invert_interrupt = of_property_read_bool(np,
>>>  				     "nvidia,invert-interrupt");
>>> +	tegra_pclk = of_clk_get(np, 0);
>>> +	WARN_ON_ONCE(IS_ERR(tegra_pclk));
>>
>> Why WARN_ON_ONCE(); is tegra_pmc_parse_dt() called more than once?
>>
>> Can the code continue if IS_ERR(tegra_pclk), or is that fatal?
> 
> The code is still OK just want to warn people that the PMC DT node
> needs PCLK clock info.

So perhaps s/WARN_ON_ONCE/WARN_ON/? No need to explicitly use a macro
that only emits the warning once if the code path is only executed once.

[PATCH V2 3/3] ARM: tegra: moving the CPU power timer function to PMC driver

[Joseph Lo]

The CPU power timer set up fucntion was related to PMC register. Now moving
it to PMC driver. And it also help to clean up the PM related code later.

Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
V2:
* the PCLK is from DT not hard code
---
 arch/arm/mach-tegra/pm.c  | 37 +------------------------------------
 arch/arm/mach-tegra/pmc.c | 32 ++++++++++++++++++++++++++++++++
 arch/arm/mach-tegra/pmc.h |  4 ++++
 3 files changed, 37 insertions(+), 36 deletions(-)

diff --git a/arch/arm/mach-tegra/pm.c b/arch/arm/mach-tegra/pm.c
index 0494f73..5f5611f 100644
--- a/arch/arm/mach-tegra/pm.c
+++ b/arch/arm/mach-tegra/pm.c
@@ -22,7 +22,6 @@
 #include <linux/cpumask.h>
 #include <linux/delay.h>
 #include <linux/cpu_pm.h>
-#include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/clk/tegra.h>
 
@@ -37,52 +36,18 @@
 #include "reset.h"
 #include "flowctrl.h"
 #include "fuse.h"
+#include "pmc.h"
 #include "sleep.h"
 
 #define TEGRA_POWER_CPU_PWRREQ_OE	(1 << 16)  /* CPU pwr req enable */
 
 #define PMC_CTRL		0x0
-#define PMC_CPUPWRGOOD_TIMER	0xc8
-#define PMC_CPUPWROFF_TIMER	0xcc
 
 #ifdef CONFIG_PM_SLEEP
 static DEFINE_SPINLOCK(tegra_lp2_lock);
 static void __iomem *pmc = IO_ADDRESS(TEGRA_PMC_BASE);
-static struct clk *tegra_pclk;
 void (*tegra_tear_down_cpu)(void);
 
-static void set_power_timers(unsigned long us_on, unsigned long us_off)
-{
-	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
-		pclk = rate;
-
-	if ((rate != tegra_last_pclk)) {
-		ticks = (us_on * pclk) + 999999ull;
-		do_div(ticks, 1000000);
-		writel((unsigned long)ticks, pmc + PMC_CPUPWRGOOD_TIMER);
-
-		ticks = (us_off * pclk) + 999999ull;
-		do_div(ticks, 1000000);
-		writel((unsigned long)ticks, pmc + PMC_CPUPWROFF_TIMER);
-		wmb();
-	}
-	tegra_last_pclk = pclk;
-}
-
 /*
  * restore_cpu_complex
  *
diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
index 05259fd..d90279c 100644
--- a/arch/arm/mach-tegra/pmc.c
+++ b/arch/arm/mach-tegra/pmc.c
@@ -28,6 +28,9 @@
 #define PMC_REMOVE_CLAMPING		0x34
 #define PMC_PWRGATE_STATUS		0x38
 
+#define PMC_CPUPWRGOOD_TIMER	0xc8
+#define PMC_CPUPWROFF_TIMER	0xcc
+
 #define TEGRA_POWERGATE_PCIE	3
 #define TEGRA_POWERGATE_VDEC	4
 #define TEGRA_POWERGATE_CPU1	9
@@ -135,6 +138,35 @@ int tegra_pmc_cpu_remove_clamping(int cpuid)
 	return tegra_pmc_powergate_remove_clamping(id);
 }
 
+#ifdef CONFIG_PM_SLEEP
+void set_power_timers(unsigned long us_on, unsigned long us_off)
+{
+	unsigned long long ticks;
+	unsigned long long pclk;
+	unsigned long rate;
+	static unsigned long tegra_last_pclk;
+
+	rate = clk_get_rate(tegra_pclk);
+
+	if (WARN_ON_ONCE(rate <= 0))
+		pclk = 100000000;
+	else
+		pclk = rate;
+
+	if ((rate != tegra_last_pclk)) {
+		ticks = (us_on * pclk) + 999999ull;
+		do_div(ticks, 1000000);
+		tegra_pmc_writel((unsigned long)ticks, PMC_CPUPWRGOOD_TIMER);
+
+		ticks = (us_off * pclk) + 999999ull;
+		do_div(ticks, 1000000);
+		tegra_pmc_writel((unsigned long)ticks, PMC_CPUPWROFF_TIMER);
+		wmb();
+	}
+	tegra_last_pclk = pclk;
+}
+#endif
+
 static const struct of_device_id matches[] __initconst = {
 	{ .compatible = "nvidia,tegra114-pmc" },
 	{ .compatible = "nvidia,tegra30-pmc" },
diff --git a/arch/arm/mach-tegra/pmc.h b/arch/arm/mach-tegra/pmc.h
index 7d44710..22f16c9 100644
--- a/arch/arm/mach-tegra/pmc.h
+++ b/arch/arm/mach-tegra/pmc.h
@@ -18,6 +18,10 @@
 #ifndef __MACH_TEGRA_PMC_H
 #define __MACH_TEGRA_PMC_H
 
+#ifdef CONFIG_PM_SLEEP
+void set_power_timers(unsigned long us_on, unsigned long us_off);
+#endif
+
 bool tegra_pmc_cpu_is_powered(int cpuid);
 int tegra_pmc_cpu_power_on(int cpuid);
 int tegra_pmc_cpu_remove_clamping(int cpuid);
-- 
1.8.1.5