[PATCH 0/2] ARM: OMAP4: Cleanup in cminst code

[PATCH 0/2] ARM: OMAP4: Cleanup in cminst code

[Vaibhav Hiremath]

Similar to PRM code cleanup done recently to reuse prminst api's
for AM33xx device, so in order to reuse the existing OMAP4
cminst code for AM33xx this patch adds,

  - Boot time __init function, to initialize _cm_bases
    based on cpu_is_xxx
  - Instead of maintaining phy addr for CM partition
    in _cm_bases[] table and then changing it to virt addr,
    directly maintain respective virt addr.

And also hook-up support for AM33xx device to existing cminst
code.

Patch series is boot tested on AM335x EVM and AM37xEVM.

NOTE: This patch series is dependent on earlier submitted PRM
      cleanup patch-series -
      http://www.spinics.net/lists/linux-omap/msg62351.html


Vaibhav Hiremath (2):
  ARM: OMAP4: cminst: Add boot time __init function for cminst
  ARM: OMAP: am33xx: Hook-up am33xx support to existing cm code

 arch/arm/mach-omap2/cminst44xx.c |   38 +++++++++++++++++++++++++++++---------
 arch/arm/mach-omap2/cminst44xx.h |    1 +
 arch/arm/mach-omap2/io.c         |    3 +++
 arch/arm/mach-omap2/omap_hwmod.c |   23 +++++++++++++++--------
 4 files changed, 48 insertions(+), 17 deletions(-)

[PATCH 1/2] ARM: OMAP4: cminst: Add boot time __init function for cminst

[Vaibhav Hiremath]

AM33xx CM module is closer to OMAP4 CM module, and
in order to reuse cminst api's we have to address
some of the differences like, base addresses and partitions.
Unlike OMAP4 CM, AM33xx doesn't have any partitions and
maintains only single partition.

So, in order to reuse the existing OMAP4 cminst code
for AM33xx this patch adds,

  - Boot time __init function, to initialize _cm_bases
    based on cpu_is_xxx
  - Instead of maintaining phy addr for CM partition
    in _cm_bases[] table and then changing it to virt addr,
    directly maintain respective virt addr.

Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
Cc: Kevin Hilman <khilman@ti.com>
Cc: Rajendra Nayak <rnayak@ti.com>
CC: Tony Lindgren <tony@atomide.com>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: Benoit Cousson <b-cousson@ti.com>
---
 arch/arm/mach-omap2/cminst44xx.c |   29 ++++++++++++++++++++---------
 arch/arm/mach-omap2/cminst44xx.h |    1 +
 arch/arm/mach-omap2/io.c         |    2 ++
 3 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mach-omap2/cminst44xx.c b/arch/arm/mach-omap2/cminst44xx.c
index 6204dea..f709708 100644
--- a/arch/arm/mach-omap2/cminst44xx.c
+++ b/arch/arm/mach-omap2/cminst44xx.c
@@ -49,13 +49,16 @@
 #define CLKCTRL_IDLEST_INTERFACE_IDLE		0x2
 #define CLKCTRL_IDLEST_DISABLED			0x3

-static u32 _cm_bases[OMAP4_MAX_PRCM_PARTITIONS] = {
+static u32 **_cm_bases;
+static u32 max_cm_partitions;
+
+static u32 *omap44xx_cm_bases[] = {
 	[OMAP4430_INVALID_PRCM_PARTITION]	= 0,
-	[OMAP4430_PRM_PARTITION]		= OMAP4430_PRM_BASE,
-	[OMAP4430_CM1_PARTITION]		= OMAP4430_CM1_BASE,
-	[OMAP4430_CM2_PARTITION]		= OMAP4430_CM2_BASE,
+	[OMAP4430_PRM_PARTITION]		= OMAP2_L4_IO_ADDRESS(OMAP4430_PRM_BASE),
+	[OMAP4430_CM1_PARTITION]		= OMAP2_L4_IO_ADDRESS(OMAP4430_CM1_BASE),
+	[OMAP4430_CM2_PARTITION]		= OMAP2_L4_IO_ADDRESS(OMAP4430_CM2_BASE),
 	[OMAP4430_SCRM_PARTITION]		= 0,
-	[OMAP4430_PRCM_MPU_PARTITION]		= OMAP4430_PRCM_MPU_BASE,
+	[OMAP4430_PRCM_MPU_PARTITION]		= OMAP2_L4_IO_ADDRESS(OMAP4430_PRCM_MPU_BASE),
 };

 /* Private functions */
@@ -103,19 +106,19 @@ static bool _is_module_ready(u8 part, u16 inst, s16 cdoffs, u16 clkctrl_offs)
 /* Read a register in a CM instance */
 u32 omap4_cminst_read_inst_reg(u8 part, s16 inst, u16 idx)
 {
-	BUG_ON(part >= OMAP4_MAX_PRCM_PARTITIONS ||
+	BUG_ON(part >= max_cm_partitions ||
 	       part == OMAP4430_INVALID_PRCM_PARTITION ||
 	       !_cm_bases[part]);
-	return __raw_readl(OMAP2_L4_IO_ADDRESS(_cm_bases[part] + inst + idx));
+	return __raw_readl(_cm_bases[part] + ((inst + idx)/sizeof(u32)));
 }

 /* Write into a register in a CM instance */
 void omap4_cminst_write_inst_reg(u32 val, u8 part, s16 inst, u16 idx)
 {
-	BUG_ON(part >= OMAP4_MAX_PRCM_PARTITIONS ||
+	BUG_ON(part >= max_cm_partitions ||
 	       part == OMAP4430_INVALID_PRCM_PARTITION ||
 	       !_cm_bases[part]);
-	__raw_writel(val, OMAP2_L4_IO_ADDRESS(_cm_bases[part] + inst + idx));
+	__raw_writel(val, _cm_bases[part] + ((inst + idx)/sizeof(u32)));
 }

 /* Read-modify-write a register in CM1. Caller must lock */
@@ -349,3 +352,11 @@ void omap4_cminst_module_disable(u8 part, u16 inst, s16 cdoffs,
 	v &= ~OMAP4430_MODULEMODE_MASK;
 	omap4_cminst_write_inst_reg(v, part, inst, clkctrl_offs);
 }
+
+void __init omap44xx_cminst_init(void)
+{
+	if (cpu_is_omap44xx()) {
+		_cm_bases = omap44xx_cm_bases;
+		max_cm_partitions = ARRAY_SIZE(omap44xx_cm_bases);
+	}
+}
diff --git a/arch/arm/mach-omap2/cminst44xx.h b/arch/arm/mach-omap2/cminst44xx.h
index a018a73..3d1bde1 100644
--- a/arch/arm/mach-omap2/cminst44xx.h
+++ b/arch/arm/mach-omap2/cminst44xx.h
@@ -63,4 +63,5 @@ extern u32 omap4_cminst_clear_inst_reg_bits(u32 bits, u8 part, s16 inst,
 extern u32 omap4_cminst_read_inst_reg_bits(u8 part, u16 inst, s16 idx,
 					   u32 mask);

+extern void __init omap44xx_cminst_init(void);
 #endif
diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c
index 50cf914..ebcc242 100644
--- a/arch/arm/mach-omap2/io.c
+++ b/arch/arm/mach-omap2/io.c
@@ -40,6 +40,7 @@
 #include "voltage.h"
 #include "powerdomain.h"
 #include "prminst44xx.h"
+#include "cminst44xx.h"

 #include "clockdomain.h"
 #include <plat/omap_hwmod.h>
@@ -492,6 +493,7 @@ void __init omap4430_init_early(void)
 	omap44xx_voltagedomains_init();
 	omap44xx_prminst_init();
 	omap44xx_powerdomains_init();
+	omap44xx_cminst_init();
 	omap44xx_clockdomains_init();
 	omap44xx_hwmod_init();
 	omap_hwmod_init_postsetup();
--
1.7.0.4

Re: [PATCH 1/2] ARM: OMAP4: cminst: Add boot time __init function for cminst

[Peter Korsgaard]

>>>>> "Vaibhav" == Vaibhav Hiremath <hvaibhav@ti.com> writes:

 Vaibhav> AM33xx CM module is closer to OMAP4 CM module, and
 Vaibhav> in order to reuse cminst api's we have to address
 Vaibhav> some of the differences like, base addresses and partitions.
 Vaibhav> Unlike OMAP4 CM, AM33xx doesn't have any partitions and
 Vaibhav> maintains only single partition.

 Vaibhav> So, in order to reuse the existing OMAP4 cminst code
 Vaibhav> for AM33xx this patch adds,

 Vaibhav>   - Boot time __init function, to initialize _cm_bases
 Vaibhav>     based on cpu_is_xxx
 Vaibhav>   - Instead of maintaining phy addr for CM partition
 Vaibhav>     in _cm_bases[] table and then changing it to virt addr,
 Vaibhav>     directly maintain respective virt addr.

 Vaibhav> @@ -103,19 +106,19 @@ static bool _is_module_ready(u8 part, u16 inst, s16 cdoffs, u16 clkctrl_offs)
 Vaibhav>  /* Read a register in a CM instance */
 Vaibhav>  u32 omap4_cminst_read_inst_reg(u8 part, s16 inst, u16 idx)
 Vaibhav>  {
 Vaibhav> -	BUG_ON(part >= OMAP4_MAX_PRCM_PARTITIONS ||
 Vaibhav> +	BUG_ON(part >= max_cm_partitions ||
 Vaibhav>  	       part == OMAP4430_INVALID_PRCM_PARTITION ||

How about dropping the OMAP4430_ prefix here as it's no longer 4430 specific?

-- 
Bye, Peter Korsgaard

RE: [PATCH 1/2] ARM: OMAP4: cminst: Add boot time __init function for cminst

[Hiremath, Vaibhav]

On Wed, Feb 15, 2012 at 02:41:32, Peter Korsgaard wrote:
> >>>>> "Vaibhav" == Vaibhav Hiremath <hvaibhav@ti.com> writes:
> 
>  Vaibhav> AM33xx CM module is closer to OMAP4 CM module, and
>  Vaibhav> in order to reuse cminst api's we have to address
>  Vaibhav> some of the differences like, base addresses and partitions.
>  Vaibhav> Unlike OMAP4 CM, AM33xx doesn't have any partitions and
>  Vaibhav> maintains only single partition.
> 
>  Vaibhav> So, in order to reuse the existing OMAP4 cminst code
>  Vaibhav> for AM33xx this patch adds,
> 
>  Vaibhav>   - Boot time __init function, to initialize _cm_bases
>  Vaibhav>     based on cpu_is_xxx
>  Vaibhav>   - Instead of maintaining phy addr for CM partition
>  Vaibhav>     in _cm_bases[] table and then changing it to virt addr,
>  Vaibhav>     directly maintain respective virt addr.
> 
>  Vaibhav> @@ -103,19 +106,19 @@ static bool _is_module_ready(u8 part, u16 inst, s16 cdoffs, u16 clkctrl_offs)
>  Vaibhav>  /* Read a register in a CM instance */
>  Vaibhav>  u32 omap4_cminst_read_inst_reg(u8 part, s16 inst, u16 idx)
>  Vaibhav>  {
>  Vaibhav> -	BUG_ON(part >= OMAP4_MAX_PRCM_PARTITIONS ||
>  Vaibhav> +	BUG_ON(part >= max_cm_partitions ||
>  Vaibhav>  	       part == OMAP4430_INVALID_PRCM_PARTITION ||
> 
> How about dropping the OMAP4430_ prefix here as it's no longer 4430 specific?
> 
Peter,

You could be right here, from this patch perspective, this change doesn't make sense.

But at the first place I am quite not sure, why this has been named as
OMAP4430_? Vs some macros are named as OMAP4_.

So first we need to understand why it has been named this way,
may be "Paul Walmsley" can comment on this. He is original author of this 
file.

Thanks,
Vaibhav

> -- 
> Bye, Peter Korsgaard
> 

Re: [PATCH 1/2] ARM: OMAP4: cminst: Add boot time __init function for cminst

[Peter Korsgaard]

>>>>> "Vaibhav" == Hiremath, Vaibhav <hvaibhav@ti.com> writes:

Hi,

 Vaibhav> -	BUG_ON(part >= OMAP4_MAX_PRCM_PARTITIONS ||
 Vaibhav> +	BUG_ON(part >= max_cm_partitions ||
 Vaibhav> part == OMAP4430_INVALID_PRCM_PARTITION ||
 >> 
 >> How about dropping the OMAP4430_ prefix here as it's no longer 4430 specific?
 >> 
 Vaibhav> Peter,

 Vaibhav> You could be right here, from this patch perspective, this
 Vaibhav> change doesn't make sense.

 Vaibhav> But at the first place I am quite not sure, why this has been
 Vaibhav> named as OMAP4430_? Vs some macros are named as OMAP4_.

 Vaibhav> So first we need to understand why it has been named this way,
 Vaibhav> may be "Paul Walmsley" can comment on this. He is original
 Vaibhav> author of this file.

As I read it, it doesn't have anything to do with any specific hardware,
it's just to catch bugs when people forget to initialize the structure
field:

arch/arm/mach-omap2/prcm44xx.h:
/*
 * OMAP4 PRCM partition IDs
 *
 * The numbers and order are arbitrary, but 0 is reserved for the
 * 'invalid' partition in case someone forgets to add a
 * .prcm_partition field.
 */
#define OMAP4430_INVALID_PRCM_PARTITION         0

-- 
Bye, Peter Korsgaard

[PATCH 2/2] ARM: OMAP: am33xx: Hook-up am33xx support to existing cm code

[Vaibhav Hiremath]

Reuse existing omap4 cminst code for am33xx device,
add separate cm base table for am33xx device and initialize
it during __init for future use.

Also, since cpu_is_omap34xx() check is true for am33xx family of
devices, we must change the order of cpu_is_xxxx check, so first
check for cpu_is_am33xx() to follow right execution path.

Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
Cc: Kevin Hilman <khilman@ti.com>
Cc: Rajendra Nayak <rnayak@ti.com>
CC: Tony Lindgren <tony@atomide.com>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: Benoit Cousson <b-cousson@ti.com>
---
 arch/arm/mach-omap2/cminst44xx.c |    9 +++++++++
 arch/arm/mach-omap2/io.c         |    1 +
 arch/arm/mach-omap2/omap_hwmod.c |   23 +++++++++++++++--------
 3 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-omap2/cminst44xx.c b/arch/arm/mach-omap2/cminst44xx.c
index f709708..402d35a 100644
--- a/arch/arm/mach-omap2/cminst44xx.c
+++ b/arch/arm/mach-omap2/cminst44xx.c
@@ -31,6 +31,7 @@
 #include "cm-regbits-44xx.h"
 #include "prcm44xx.h"
 #include "prm44xx.h"
+#include "prm33xx.h"
 #include "prcm_mpu44xx.h"
 
 /*
@@ -61,6 +62,11 @@ static u32 *omap44xx_cm_bases[] = {
 	[OMAP4430_PRCM_MPU_PARTITION]		= OMAP2_L4_IO_ADDRESS(OMAP4430_PRCM_MPU_BASE),
 };
 
+static u32 *am33xx_cm_bases[] = {
+	[OMAP4430_INVALID_PRCM_PARTITION]	= 0,
+	[AM33XX_PRM_PARTITION]			= AM33XX_L4_WK_IO_ADDRESS(AM33XX_PRM_BASE),
+};
+
 /* Private functions */
 
 /**
@@ -358,5 +364,8 @@ void __init omap44xx_cminst_init(void)
 	if (cpu_is_omap44xx()) {
 		_cm_bases = omap44xx_cm_bases;
 		max_cm_partitions = ARRAY_SIZE(omap44xx_cm_bases);
+	} else if (cpu_is_am33xx()) {
+		_cm_bases =am33xx_cm_bases;
+		max_cm_partitions = ARRAY_SIZE(am33xx_cm_bases);
 	}
 }
diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c
index ebcc242..b318b44 100644
--- a/arch/arm/mach-omap2/io.c
+++ b/arch/arm/mach-omap2/io.c
@@ -479,6 +479,7 @@ void __init am33xx_init_early(void)
 	am33xx_voltagedomains_init();
 	omap44xx_prminst_init();
 	am33xx_powerdomains_init();
+	omap44xx_cminst_init();
 	omap3xxx_clk_init();
 }
 #endif
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 612701e..e4ae811 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -737,7 +737,7 @@ static void _disable_optional_clocks(struct omap_hwmod *oh)
 static void _enable_module(struct omap_hwmod *oh)
 {
 	/* The module mode does not exist prior OMAP4 */
-	if (cpu_is_omap24xx() || cpu_is_omap34xx())
+	if (!cpu_is_omap44xx() && !cpu_is_am33xx())
 		return;
 
 	if (!oh->clkdm || !oh->prcm.omap4.modulemode)
@@ -764,7 +764,7 @@ static void _enable_module(struct omap_hwmod *oh)
  */
 static int _omap4_wait_target_disable(struct omap_hwmod *oh)
 {
-	if (!cpu_is_omap44xx())
+	if (!cpu_is_omap44xx() && !cpu_is_am33xx())
 		return 0;
 
 	if (!oh)
@@ -794,7 +794,7 @@ static int _omap4_disable_module(struct omap_hwmod *oh)
 	int v;
 
 	/* The module mode does not exist prior OMAP4 */
-	if (!cpu_is_omap44xx())
+	if (!cpu_is_omap44xx() && !cpu_is_am33xx())
 		return -EINVAL;
 
 	if (!oh->clkdm || !oh->prcm.omap4.modulemode)
@@ -1219,11 +1219,13 @@ static int _wait_target_ready(struct omap_hwmod *oh)
 
 	/* XXX check clock enable states */
 
-	if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
-		ret = omap2_cm_wait_module_ready(oh->prcm.omap2.module_offs,
-						 oh->prcm.omap2.idlest_reg_id,
-						 oh->prcm.omap2.idlest_idle_bit);
-	} else if (cpu_is_omap44xx()) {
+	/*
+	 * In order to use omap4 cminst code for am33xx family of devices,
+	 * first check cpu_is_am33xx here.
+	 *
+	 * Note: cpu_is_omap34xx is true for am33xx device as well.
+	 */
+	if (cpu_is_omap44xx() || cpu_is_am33xx()) {
 		if (!oh->clkdm)
 			return -EINVAL;
 
@@ -1231,6 +1233,11 @@ static int _wait_target_ready(struct omap_hwmod *oh)
 						     oh->clkdm->cm_inst,
 						     oh->clkdm->clkdm_offs,
 						     oh->prcm.omap4.clkctrl_offs);
+	} else if (cpu_is_omap24xx() || cpu_is_omap34xx()) {
+			ret = omap2_cm_wait_module_ready(
+						oh->prcm.omap2.module_offs,
+						oh->prcm.omap2.idlest_reg_id,
+						oh->prcm.omap2.idlest_idle_bit);
 	} else {
 		BUG();
 	};
-- 
1.7.0.4

Re: [PATCH 2/2] ARM: OMAP: am33xx: Hook-up am33xx support to existing cm code

[Peter Korsgaard]

>>>>> "Vaibhav" == Vaibhav Hiremath <hvaibhav@ti.com> writes:

 Vaibhav> Reuse existing omap4 cminst code for am33xx device,
 Vaibhav> add separate cm base table for am33xx device and initialize
 Vaibhav> it during __init for future use.

 Vaibhav> Also, since cpu_is_omap34xx() check is true for am33xx family of
 Vaibhav> devices, we must change the order of cpu_is_xxxx check, so first
 Vaibhav> check for cpu_is_am33xx() to follow right execution path.

 Vaibhav>  	if (cpu_is_omap44xx()) {
 Vaibhav>  		_cm_bases = omap44xx_cm_bases;
 Vaibhav>  		max_cm_partitions = ARRAY_SIZE(omap44xx_cm_bases);
 Vaibhav> +	} else if (cpu_is_am33xx()) {
 Vaibhav> +		_cm_bases =am33xx_cm_bases;

Space after '='.

 Vaibhav> +++ b/arch/arm/mach-omap2/omap_hwmod.c
 Vaibhav> @@ -737,7 +737,7 @@ static void _disable_optional_clocks(struct omap_hwmod *oh)
 Vaibhav>  static void _enable_module(struct omap_hwmod *oh)
 Vaibhav>  {
 Vaibhav>  	/* The module mode does not exist prior OMAP4 */
 Vaibhav> -	if (cpu_is_omap24xx() || cpu_is_omap34xx())
 Vaibhav> +	if (!cpu_is_omap44xx() && !cpu_is_am33xx())

Maybe update the comment as well?
 
 Vaibhav>  	/* The module mode does not exist prior OMAP4 */
 Vaibhav> -	if (!cpu_is_omap44xx())
 Vaibhav> +	if (!cpu_is_omap44xx() && !cpu_is_am33xx())
 Vaibhav>  		return -EINVAL;

Here as well.

Otherwise it looks good.

Acked-by: Peter Korsgaard <jacmet@sunsite.dk>

-- 
Bye, Peter Korsgaard

RE: [PATCH 2/2] ARM: OMAP: am33xx: Hook-up am33xx support to existing cm code

[Hiremath, Vaibhav]

On Wed, Feb 15, 2012 at 02:25:34, Peter Korsgaard wrote:
> >>>>> "Vaibhav" == Vaibhav Hiremath <hvaibhav@ti.com> writes:
> 
>  Vaibhav> Reuse existing omap4 cminst code for am33xx device,
>  Vaibhav> add separate cm base table for am33xx device and initialize
>  Vaibhav> it during __init for future use.
> 
>  Vaibhav> Also, since cpu_is_omap34xx() check is true for am33xx family of
>  Vaibhav> devices, we must change the order of cpu_is_xxxx check, so first
>  Vaibhav> check for cpu_is_am33xx() to follow right execution path.
> 
>  Vaibhav>  	if (cpu_is_omap44xx()) {
>  Vaibhav>  		_cm_bases = omap44xx_cm_bases;
>  Vaibhav>  		max_cm_partitions = ARRAY_SIZE(omap44xx_cm_bases);
>  Vaibhav> +	} else if (cpu_is_am33xx()) {
>  Vaibhav> +		_cm_bases =am33xx_cm_bases;
> 
> Space after '='.

Oops...Will fix it.

> 
>  Vaibhav> +++ b/arch/arm/mach-omap2/omap_hwmod.c
>  Vaibhav> @@ -737,7 +737,7 @@ static void _disable_optional_clocks(struct omap_hwmod *oh)
>  Vaibhav>  static void _enable_module(struct omap_hwmod *oh)
>  Vaibhav>  {
>  Vaibhav>  	/* The module mode does not exist prior OMAP4 */
>  Vaibhav> -	if (cpu_is_omap24xx() || cpu_is_omap34xx())
>  Vaibhav> +	if (!cpu_is_omap44xx() && !cpu_is_am33xx())
> 
> Maybe update the comment as well?
>  
Will update accordingly and submit the next version.

>  Vaibhav>  	/* The module mode does not exist prior OMAP4 */
>  Vaibhav> -	if (!cpu_is_omap44xx())
>  Vaibhav> +	if (!cpu_is_omap44xx() && !cpu_is_am33xx())
>  Vaibhav>  		return -EINVAL;
> 
> Here as well.
> 
> Otherwise it looks good.
> 
> Acked-by: Peter Korsgaard <jacmet@sunsite.dk>
Thanks
Vaibhav

> -- 
> Bye, Peter Korsgaard
>