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

login
register
mail settings
Submitter Joseph Lo
Date March 18, 2013, 8:09 a.m.
Message ID <1363594199-10974-3-git-send-email-josephl@nvidia.com>
Download mbox | patch
Permalink /patch/228383/
State Superseded, archived
Headers show

Comments

Joseph Lo - March 18, 2013, 8:09 a.m.
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(-)
Stephen Warren - March 19, 2013, 4:40 p.m.
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?
--
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 20, 2013, 10 a.m.
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.


--
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
Stephen Warren - March 20, 2013, 3:51 p.m.
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.
--
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/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)