diff mbox

[1/4] ARM: tegra: pmc: convert PMC driver to support DT only

Message ID 1361515491-16199-2-git-send-email-josephl@nvidia.com (mailing list archive)
State New, archived
Headers show

Commit Message

Joseph Lo Feb. 22, 2013, 6:44 a.m. UTC
The Tegra kernel only support boot from DT now. Clean up the PMC driver
to support DT only, that includes:

* remove the ifdef of CONFIG_OF
* replace the static mapping of PMC addr to map from DT

Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
 arch/arm/mach-tegra/pmc.c | 56 +++++++++++++++++++++++------------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

Comments

Peter De Schrijver Feb. 22, 2013, 1:05 p.m. UTC | #1
On Fri, Feb 22, 2013 at 07:44:48AM +0100, Joseph Lo wrote:
> The Tegra kernel only support boot from DT now. Clean up the PMC driver
> to support DT only, that includes:
> 
> * remove the ifdef of CONFIG_OF
> * replace the static mapping of PMC addr to map from DT
> 
> Signed-off-by: Joseph Lo <josephl@nvidia.com>
> ---
>  arch/arm/mach-tegra/pmc.c | 56 +++++++++++++++++++++++------------------------
>  1 file changed, 28 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
> index d4fdb5f..2315e25 100644
> --- a/arch/arm/mach-tegra/pmc.c
> +++ b/arch/arm/mach-tegra/pmc.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (C) 2012 NVIDIA CORPORATION. All rights reserved.
> + * Copyright (C) 2012,2013 NVIDIA CORPORATION. All rights reserved.
>   *
>   * This program is free software; you can redistribute it and/or modify it
>   * under the terms and conditions of the GNU General Public License,
> @@ -16,59 +16,59 @@
>   */
>  
>  #include <linux/kernel.h>
> +#include <linux/err.h>
>  #include <linux/io.h>
>  #include <linux/of.h>
> -
> -#include "iomap.h"
> +#include <linux/of_address.h>
>  
>  #define PMC_CTRL		0x0
>  #define PMC_CTRL_INTR_LOW	(1 << 17)
>  
> +static void __iomem *tegra_pmc_base;
> +static bool tegra_pmc_invert_interrupt;
> +
>  static inline u32 tegra_pmc_readl(u32 reg)
>  {
> -	return readl(IO_ADDRESS(TEGRA_PMC_BASE + reg));
> +	return readl_relaxed(tegra_pmc_base + reg);
>  }
>  
>  static inline void tegra_pmc_writel(u32 val, u32 reg)
>  {
> -	writel(val, IO_ADDRESS(TEGRA_PMC_BASE + reg));
> +	writel_relaxed(val, (tegra_pmc_base + reg));
>  }
>  
> -#ifdef CONFIG_OF
>  static const struct of_device_id matches[] __initconst = {
>  	{ .compatible = "nvidia,tegra20-pmc" },
>  	{ }

At least an extra entry for tegra114-pmc is necessary here. tegra114.dtsi only
has:

pmc {
	compatible = "nvidia,tegra114-pmc", "nvidia,tegra30-pmc";
	reg = <0x7000e400 0x400>;
};

otherwise the system will crash early during boot.

Cheers,

Peter.
Stephen Warren Feb. 22, 2013, 6:32 p.m. UTC | #2
On 02/21/2013 11:44 PM, Joseph Lo wrote:
> The Tegra kernel only support boot from DT now. Clean up the PMC driver
> to support DT only, that includes:
> 
> * remove the ifdef of CONFIG_OF
> * replace the static mapping of PMC addr to map from DT

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

>  static inline u32 tegra_pmc_readl(u32 reg)
>  {
> -	return readl(IO_ADDRESS(TEGRA_PMC_BASE + reg));
> +	return readl_relaxed(tegra_pmc_base + reg);

The change from readl to readl_relaxed is definitely unrelated to
cleaning up DT support. It needs to be a separate patch, and needs to be
mentioned in the commit description.

> +static void tegra_pmc_parse_dt(void)
> +{
> +	struct device_node *np;
> +
> +	np = of_find_matching_node(NULL, matches);
> +	if (np) {

Here, if (!np) BUG(); would be simpler, and allow the rest of the
function not to be indented.

>  void __init tegra_pmc_init(void)
>  {
...
> +	if (!of_have_populated_dt())
> +		return;

That won't ever be true.
Joseph Lo Feb. 23, 2013, 2:03 a.m. UTC | #3
On Fri, 2013-02-22 at 21:05 +0800, Peter De Schrijver wrote:
> On Fri, Feb 22, 2013 at 07:44:48AM +0100, Joseph Lo wrote:
> > The Tegra kernel only support boot from DT now. Clean up the PMC driver
> > to support DT only, that includes:
> > 
> > * remove the ifdef of CONFIG_OF
> > * replace the static mapping of PMC addr to map from DT
> > 
> > -#ifdef CONFIG_OF
> >  static const struct of_device_id matches[] __initconst = {
> >  	{ .compatible = "nvidia,tegra20-pmc" },
> >  	{ }
> 
> At least an extra entry for tegra114-pmc is necessary here. tegra114.dtsi only
> has:
> 
> pmc {
> 	compatible = "nvidia,tegra114-pmc", "nvidia,tegra30-pmc";
> 	reg = <0x7000e400 0x400>;
> };
> 
I think it should be something like below, isn't it?

compatible = "nvidia,tegra114-pmc", "nvidia,tegra30-pmc",
		"nvidia,tegra20-pmc";

or should we add tegra114 and tegra30 in the DT match table?

Thanks,
Joseph
Stephen Warren Feb. 23, 2013, 4:31 a.m. UTC | #4
On 02/22/2013 07:03 PM, Joseph Lo wrote:
> On Fri, 2013-02-22 at 21:05 +0800, Peter De Schrijver wrote:
>> On Fri, Feb 22, 2013 at 07:44:48AM +0100, Joseph Lo wrote:
>>> The Tegra kernel only support boot from DT now. Clean up the PMC driver
>>> to support DT only, that includes:
>>>
>>> * remove the ifdef of CONFIG_OF
>>> * replace the static mapping of PMC addr to map from DT
>>>
>>> -#ifdef CONFIG_OF
>>>  static const struct of_device_id matches[] __initconst = {
>>>  	{ .compatible = "nvidia,tegra20-pmc" },
>>>  	{ }
>>
>> At least an extra entry for tegra114-pmc is necessary here. tegra114.dtsi only
>> has:
>>
>> pmc {
>> 	compatible = "nvidia,tegra114-pmc", "nvidia,tegra30-pmc";
>> 	reg = <0x7000e400 0x400>;
>> };
>>
> I think it should be something like below, isn't it?
> 
> compatible = "nvidia,tegra114-pmc", "nvidia,tegra30-pmc",
> 		"nvidia,tegra20-pmc";
> 
> or should we add tegra114 and tegra30 in the DT match table?

The Tegra114 PMC HW is probably not 100% backwards-compatible with
previous SoCs' PMC, so the DT file should probably only list the
specific SoC, and the driver should probably include all the compatible
values it supports.

Peter, can you confirm exactly which HW versions, if any, are 100%
backwards-compatible?
Peter De Schrijver Feb. 25, 2013, 2:28 p.m. UTC | #5
On Sat, Feb 23, 2013 at 05:31:17AM +0100, Stephen Warren wrote:
> On 02/22/2013 07:03 PM, Joseph Lo wrote:
> > On Fri, 2013-02-22 at 21:05 +0800, Peter De Schrijver wrote:
> >> On Fri, Feb 22, 2013 at 07:44:48AM +0100, Joseph Lo wrote:
> >>> The Tegra kernel only support boot from DT now. Clean up the PMC driver
> >>> to support DT only, that includes:
> >>>
> >>> * remove the ifdef of CONFIG_OF
> >>> * replace the static mapping of PMC addr to map from DT
> >>>
> >>> -#ifdef CONFIG_OF
> >>>  static const struct of_device_id matches[] __initconst = {
> >>>  	{ .compatible = "nvidia,tegra20-pmc" },
> >>>  	{ }
> >>
> >> At least an extra entry for tegra114-pmc is necessary here. tegra114.dtsi only
> >> has:
> >>
> >> pmc {
> >> 	compatible = "nvidia,tegra114-pmc", "nvidia,tegra30-pmc";
> >> 	reg = <0x7000e400 0x400>;
> >> };
> >>
> > I think it should be something like below, isn't it?
> > 
> > compatible = "nvidia,tegra114-pmc", "nvidia,tegra30-pmc",
> > 		"nvidia,tegra20-pmc";
> > 
> > or should we add tegra114 and tegra30 in the DT match table?
> 
> The Tegra114 PMC HW is probably not 100% backwards-compatible with
> previous SoCs' PMC, so the DT file should probably only list the
> specific SoC, and the driver should probably include all the compatible
> values it supports.
> 
> Peter, can you confirm exactly which HW versions, if any, are 100%
> backwards-compatible?
> 

The major difference between the PMCs are the powerdomain IDs. The Tegra30 IDs
are a strict superset of the Tegra20 IDs, but the Tegra114 IDs are not. There
are differences in the CPU domains and 3D. The CPU domains aren't a problem I
think because we won't powergate those domains directly using the PMC. We
always program the flowcontroller to do the powergating on WFI. 3D is a
different story however. We do powergate 3D under software control and the
sequencing is different between Tegra30 and Tegra114. Also Tegra30 has 2
domains for 3D while Tegra114 has only 1. Then ofcourse Tegra114 lacks the
SATA and PCIe domains because those features are missing compared to Tegra30.
But the IDs haven't been reused. All in all I think it's best to explicitly
require the driver to support the various SoCs.

Cheers,

Peter.
Stephen Warren Feb. 25, 2013, 3:43 p.m. UTC | #6
On 02/25/2013 07:28 AM, Peter De Schrijver wrote:
> On Sat, Feb 23, 2013 at 05:31:17AM +0100, Stephen Warren wrote:
...
>> Peter, can you confirm exactly which [PMC] HW versions, if any, are 100%
>> backwards-compatible?
>>
> 
> The major difference between the PMCs are the powerdomain IDs. The Tegra30 IDs
> are a strict superset of the Tegra20 IDs, but the Tegra114 IDs are not. There
> are differences in the CPU domains and 3D. The CPU domains aren't a problem I
> think because we won't powergate those domains directly using the PMC. We
> always program the flowcontroller to do the powergating on WFI. 3D is a
> different story however. We do powergate 3D under software control and the
> sequencing is different between Tegra30 and Tegra114. Also Tegra30 has 2
> domains for 3D while Tegra114 has only 1. Then of course Tegra114 lacks the
> SATA and PCIe domains because those features are missing compared to Tegra30.
> But the IDs haven't been reused. All in all I think it's best to explicitly
> require the driver to support the various SoCs.

That does sound like a good idea; a separate compatible value for each SoC.
Joseph Lo Feb. 26, 2013, 2:27 a.m. UTC | #7
On Mon, 2013-02-25 at 23:43 +0800, Stephen Warren wrote:
> On 02/25/2013 07:28 AM, Peter De Schrijver wrote:
> > On Sat, Feb 23, 2013 at 05:31:17AM +0100, Stephen Warren wrote:
> ...
> >> Peter, can you confirm exactly which [PMC] HW versions, if any, are 100%
> >> backwards-compatible?
> >>
> > 
> > The major difference between the PMCs are the powerdomain IDs. The Tegra30 IDs
> > are a strict superset of the Tegra20 IDs, but the Tegra114 IDs are not. There
> > are differences in the CPU domains and 3D. The CPU domains aren't a problem I
> > think because we won't powergate those domains directly using the PMC. We
> > always program the flowcontroller to do the powergating on WFI. 3D is a
> > different story however. We do powergate 3D under software control and the
> > sequencing is different between Tegra30 and Tegra114. Also Tegra30 has 2
> > domains for 3D while Tegra114 has only 1. Then of course Tegra114 lacks the
> > SATA and PCIe domains because those features are missing compared to Tegra30.
> > But the IDs haven't been reused. All in all I think it's best to explicitly
> > require the driver to support the various SoCs.
> 
> That does sound like a good idea; a separate compatible value for each SoC.

OK. Will add another patch to fix the PMC compatibility in Tegra30 and
Tegra114 dts file. And also in the DT match table of PMC driver.

Thanks,
Joseph
diff mbox

Patch

diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
index d4fdb5f..2315e25 100644
--- a/arch/arm/mach-tegra/pmc.c
+++ b/arch/arm/mach-tegra/pmc.c
@@ -1,5 +1,5 @@ 
 /*
- * Copyright (C) 2012 NVIDIA CORPORATION. All rights reserved.
+ * Copyright (C) 2012,2013 NVIDIA CORPORATION. All rights reserved.
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,
@@ -16,59 +16,59 @@ 
  */
 
 #include <linux/kernel.h>
+#include <linux/err.h>
 #include <linux/io.h>
 #include <linux/of.h>
-
-#include "iomap.h"
+#include <linux/of_address.h>
 
 #define PMC_CTRL		0x0
 #define PMC_CTRL_INTR_LOW	(1 << 17)
 
+static void __iomem *tegra_pmc_base;
+static bool tegra_pmc_invert_interrupt;
+
 static inline u32 tegra_pmc_readl(u32 reg)
 {
-	return readl(IO_ADDRESS(TEGRA_PMC_BASE + reg));
+	return readl_relaxed(tegra_pmc_base + reg);
 }
 
 static inline void tegra_pmc_writel(u32 val, u32 reg)
 {
-	writel(val, IO_ADDRESS(TEGRA_PMC_BASE + reg));
+	writel_relaxed(val, (tegra_pmc_base + reg));
 }
 
-#ifdef CONFIG_OF
 static const struct of_device_id matches[] __initconst = {
 	{ .compatible = "nvidia,tegra20-pmc" },
 	{ }
 };
-#endif
+
+static void tegra_pmc_parse_dt(void)
+{
+	struct device_node *np;
+
+	np = of_find_matching_node(NULL, matches);
+	if (np) {
+		tegra_pmc_base = of_iomap(np, 0);
+
+		tegra_pmc_invert_interrupt = of_property_read_bool(np,
+					     "nvidia,invert-interrupt");
+	} else {
+		/* Should not be here, the PMC DT node should always exist. */
+		BUG();
+	}
+}
 
 void __init tegra_pmc_init(void)
 {
-	/*
-	 * For now, Harmony is the only board that uses the PMC, and it wants
-	 * the signal inverted. Seaboard would too if it used the PMC.
-	 * Hopefully by the time other boards want to use the PMC, everything
-	 * will be device-tree, or they also want it inverted.
-	 */
-	bool invert_interrupt = true;
 	u32 val;
 
-#ifdef CONFIG_OF
-	if (of_have_populated_dt()) {
-		struct device_node *np;
+	if (!of_have_populated_dt())
+		return;
 
-		invert_interrupt = false;
-
-		np = of_find_matching_node(NULL, matches);
-		if (np) {
-			if (of_find_property(np, "nvidia,invert-interrupt",
-						NULL))
-				invert_interrupt = true;
-		}
-	}
-#endif
+	tegra_pmc_parse_dt();
 
 	val = tegra_pmc_readl(PMC_CTRL);
-	if (invert_interrupt)
+	if (tegra_pmc_invert_interrupt)
 		val |= PMC_CTRL_INTR_LOW;
 	else
 		val &= ~PMC_CTRL_INTR_LOW;