[PATCH-V2 0/2] arm:omap:am33xx: Add voltage and power domain data

[PATCH-V2 0/2] arm:omap:am33xx: Add voltage and power domain data

[Vaibhav Hiremath]

Last time I had submitted a big patch-series, almost changing >7.5K lines
for AM33XX voltage, power, clock and HWMOD data support (as a RFC);
which I believe is very hard to review and difficult to manage also.
So I decided to split the patches as and when they are clean
and ready for review/merge/acceptance,
    - Voltage and Power data (ready)
    - Clock (common-clk migration) and HWMOD

This patch-series is created on top of my last patch submitted
for OMAP4 PRM cleanup for PWRSTCTRL & PWRSTST -
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg60468.html

As always, I maintain a seperate git repository where you can find
these patches + some patches to get AM335xEVM/BeagleBone to boot cleanly.

http://arago-project.org/git/people/?p=vaibhav/ti-psp-omap-video.git;a=summary
Branch - am335x-staging

Link to first version (RFC version) -
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg58742.html

Changes form V1:
	- Splitted patchs based on dependencies.
	- Incorporated all Kevin Hilman's review comments
	  * Have only one patch for data and Makefile/Kconfig changes
	  * Voltage Data: Add ".scalable = false" entry, to indicate
	    clearly that scaling is not supported yet.
	- Reuse all OMAP4 PRM implementation
	  * Thanks to Kevin and Rajendra for helping me out in this.

Vaibhav Hiremath (2):
  arm:omap:am33xx: Add voltage domain data
  arm:omap:am33xx: Add power domain data

 arch/arm/mach-omap2/Makefile                  |    5 +
 arch/arm/mach-omap2/io.c                      |    2 +
 arch/arm/mach-omap2/omap_hwmod.c              |    1 +
 arch/arm/mach-omap2/powerdomain.h             |    5 +-
 arch/arm/mach-omap2/powerdomains33xx_data.c   |  134 +++++++++++++++++++++++++
 arch/arm/mach-omap2/prm33xx.h                 |  118 ++++++++++++++++++++++
 arch/arm/mach-omap2/voltage.h                 |    1 +
 arch/arm/mach-omap2/voltagedomains33xx_data.c |   46 +++++++++
 8 files changed, 310 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm/mach-omap2/powerdomains33xx_data.c
 create mode 100644 arch/arm/mach-omap2/prm33xx.h
 create mode 100644 arch/arm/mach-omap2/voltagedomains33xx_data.c

[PATCH-V2 1/2] arm:omap:am33xx: Add voltage domain data

[Vaibhav Hiremath]

Currently dummy voltage domain data is being created
in order to succeed boot process, nothing has been done
w.r.t actual hardware (voltage control).

Also, hook up the AM33XX voltage domain to OMAP framework.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
---
 arch/arm/mach-omap2/Makefile                  |    2 +
 arch/arm/mach-omap2/io.c                      |    1 +
 arch/arm/mach-omap2/voltage.h                 |    1 +
 arch/arm/mach-omap2/voltagedomains33xx_data.c |   46 +++++++++++++++++++++++++
 4 files changed, 50 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-omap2/voltagedomains33xx_data.c

diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
index 74ae499..8d90f9f 100644
--- a/arch/arm/mach-omap2/Makefile
+++ b/arch/arm/mach-omap2/Makefile
@@ -99,6 +99,8 @@ obj-$(CONFIG_ARCH_OMAP2)		+= $(voltagedomain-common) \
 					   voltagedomains2xxx_data.o
 obj-$(CONFIG_ARCH_OMAP3)		+= $(voltagedomain-common) \
 					   voltagedomains3xxx_data.o
+obj-$(CONFIG_SOC_OMAPAM33XX)		+= $(voltagedomain-common) \
+					   voltagedomains33xx_data.o
 obj-$(CONFIG_ARCH_OMAP4)		+= $(voltagedomain-common) \
 					   voltagedomains44xx_data.o

diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c
index 5c180de..d50cbd7 100644
--- a/arch/arm/mach-omap2/io.c
+++ b/arch/arm/mach-omap2/io.c
@@ -472,6 +472,7 @@ void __init am33xx_init_early(void)
 {
 	omap2_set_globals_am33xx();
 	omap_common_init_early();
+	am33xx_voltagedomains_init();
 	omap3xxx_clk_init();
 }
 #endif
diff --git a/arch/arm/mach-omap2/voltage.h b/arch/arm/mach-omap2/voltage.h
index 16a1b09..a7c43c1 100644
--- a/arch/arm/mach-omap2/voltage.h
+++ b/arch/arm/mach-omap2/voltage.h
@@ -156,6 +156,7 @@ int omap_voltage_late_init(void);

 extern void omap2xxx_voltagedomains_init(void);
 extern void omap3xxx_voltagedomains_init(void);
+extern void am33xx_voltagedomains_init(void);
 extern void omap44xx_voltagedomains_init(void);

 struct voltagedomain *voltdm_lookup(const char *name);
diff --git a/arch/arm/mach-omap2/voltagedomains33xx_data.c b/arch/arm/mach-omap2/voltagedomains33xx_data.c
new file mode 100644
index 0000000..78fbf39
--- /dev/null
+++ b/arch/arm/mach-omap2/voltagedomains33xx_data.c
@@ -0,0 +1,46 @@
+/*
+ * AM33XX voltage domain data
+ *
+ * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+
+#include "voltage.h"
+
+static struct voltagedomain am33xx_voltdm_mpu = {
+	.name		= "mpu",
+	.scalable	= false,
+};
+
+static struct voltagedomain am33xx_voltdm_core = {
+	.name		= "core",
+	.scalable	= false,
+};
+
+static struct voltagedomain am33xx_voltdm_rtc = {
+	.name		= "rtc",
+	.scalable	= false,
+};
+
+static struct voltagedomain *voltagedomains_am33xx[] __initdata = {
+	&am33xx_voltdm_mpu,
+	&am33xx_voltdm_core,
+	&am33xx_voltdm_rtc,
+	NULL,
+};
+
+void __init am33xx_voltagedomains_init(void)
+{
+	voltdm_init(voltagedomains_am33xx);
+}
--
1.7.0.4

Re: [PATCH-V2 1/2] arm:omap:am33xx: Add voltage domain data

[Kevin Hilman]

Vaibhav Hiremath <hvaibhav@ti.com> writes:

> Currently dummy voltage domain data is being created
> in order to succeed boot process, nothing has been done
> w.r.t actual hardware (voltage control).
>
> Also, hook up the AM33XX voltage domain to OMAP framework.
>
> Signed-off-by: Afzal Mohammed <afzal@ti.com>
> Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>

Data looks OK, but why the dummy voltage domains?   Does this device not
support voltage scaling? 

Also, I think it was my idea in an earlier review to add the 
'.scalable = false', but it's not really necessary since the default is
false.  Sorry 'bout that.

Kevin

RE: [PATCH-V2 1/2] arm:omap:am33xx: Add voltage domain data

[Hiremath, Vaibhav]

On Tue, Jan 10, 2012 at 23:59:59, Hilman, Kevin wrote:
> Vaibhav Hiremath <hvaibhav@ti.com> writes:
> 
> > Currently dummy voltage domain data is being created
> > in order to succeed boot process, nothing has been done
> > w.r.t actual hardware (voltage control).
> >
> > Also, hook up the AM33XX voltage domain to OMAP framework.
> >
> > Signed-off-by: Afzal Mohammed <afzal@ti.com>
> > Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
> 
> Data looks OK, but why the dummy voltage domains?   Does this device not
> support voltage scaling? 
> 
It does support voltage scaling.

Kevin,
This is part of initial baseport patches and complete PM support 
along with voltage scaling will follow slowly as-n-when it is ready for submission.
We still haven't got full power management working internally which we can submit immediately.


> Also, I think it was my idea in an earlier review to add the 
> '.scalable = false', but it's not really necessary since the default is
> false.  Sorry 'bout that.
> 

Sorry it's my miss as well, I should have conformed it before submission.
I will correct it in next version.

Thanks,
Vaibhav

> Kevin
> 

[PATCH-V2 2/2] arm:omap:am33xx: Add power domain data

[Vaibhav Hiremath]

This patch adds AM33XX power domain data (powerdomains33xx_data.c)
and header file with respective PRM register offsets.
PRM module in AM33XX family of device is close to that of OMAP4 PRM
with respect to register bit-fields and offsets, so reusing all
existing OMAP4 API's to access AM33XX PRM module.

Also, hook up the AM33XX power domain to OMAP framework.

This patch is based on OMAP4 cleanup patch of hardcoded reg-offs for
PWRSTCTRL & PWRSTST.
Patch -
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg60468.html

Signed-off-by: Afzal Mohammed <afzal@ti.com>
Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
---
 arch/arm/mach-omap2/Makefile                |    3 +
 arch/arm/mach-omap2/io.c                    |    1 +
 arch/arm/mach-omap2/omap_hwmod.c            |    1 +
 arch/arm/mach-omap2/powerdomain.h           |    5 +-
 arch/arm/mach-omap2/powerdomains33xx_data.c |  134 +++++++++++++++++++++++++++
 arch/arm/mach-omap2/prm33xx.h               |  118 +++++++++++++++++++++++
 6 files changed, 260 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm/mach-omap2/powerdomains33xx_data.c
 create mode 100644 arch/arm/mach-omap2/prm33xx.h

diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
index 8d90f9f..d30c50c 100644
--- a/arch/arm/mach-omap2/Makefile
+++ b/arch/arm/mach-omap2/Makefile
@@ -114,6 +114,9 @@ obj-$(CONFIG_ARCH_OMAP3)		+= $(powerdomain-common) \
 					   powerdomain2xxx_3xxx.o \
 					   powerdomains3xxx_data.o \
 					   powerdomains2xxx_3xxx_data.o
+obj-$(CONFIG_SOC_OMAPAM33XX)		+= prminst44xx.o \
+					   powerdomain44xx.o \
+					   powerdomains33xx_data.o
 obj-$(CONFIG_ARCH_OMAP4)		+= $(powerdomain-common) \
 					   powerdomain44xx.o \
 					   powerdomains44xx_data.o
diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c
index d50cbd7..5f96aa5 100644
--- a/arch/arm/mach-omap2/io.c
+++ b/arch/arm/mach-omap2/io.c
@@ -473,6 +473,7 @@ void __init am33xx_init_early(void)
 	omap2_set_globals_am33xx();
 	omap_common_init_early();
 	am33xx_voltagedomains_init();
+	am33xx_powerdomains_init();
 	omap3xxx_clk_init();
 }
 #endif
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 4fd53d4..34dc29b 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -150,6 +150,7 @@
 #include "cminst44xx.h"
 #include "prm2xxx_3xxx.h"
 #include "prm44xx.h"
+#include "prm33xx.h"
 #include "prminst44xx.h"
 #include "mux.h"

diff --git a/arch/arm/mach-omap2/powerdomain.h b/arch/arm/mach-omap2/powerdomain.h
index 9ebb872..284eb22 100644
--- a/arch/arm/mach-omap2/powerdomain.h
+++ b/arch/arm/mach-omap2/powerdomain.h
@@ -67,9 +67,9 @@

 /*
  * Maximum number of clockdomains that can be associated with a powerdomain.
- * CORE powerdomain on OMAP4 is the worst case
+ * CORE powerdomain on AM33XX is the worst case
  */
-#define PWRDM_MAX_CLKDMS	9
+#define PWRDM_MAX_CLKDMS	11

 /* XXX A completely arbitrary number. What is reasonable here? */
 #define PWRDM_TRANSITION_BAILOUT 100000
@@ -227,6 +227,7 @@ bool pwrdm_can_ever_lose_context(struct powerdomain *pwrdm);
 extern void omap242x_powerdomains_init(void);
 extern void omap243x_powerdomains_init(void);
 extern void omap3xxx_powerdomains_init(void);
+extern void am33xx_powerdomains_init(void);
 extern void omap44xx_powerdomains_init(void);

 extern struct pwrdm_ops omap2_pwrdm_operations;
diff --git a/arch/arm/mach-omap2/powerdomains33xx_data.c b/arch/arm/mach-omap2/powerdomains33xx_data.c
new file mode 100644
index 0000000..a8b06d1
--- /dev/null
+++ b/arch/arm/mach-omap2/powerdomains33xx_data.c
@@ -0,0 +1,134 @@
+/*
+ * AM33XX Power domains framework
+ *
+ * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+
+#include "powerdomain.h"
+#include "prcm-common.h"
+#include "prm33xx.h"
+#include "prcm44xx.h"
+
+static struct powerdomain gfx_33xx_pwrdm = {
+	.name			= "gfx_pwrdm",
+	.voltdm			= { .name = "core" },
+	.prcm_partition		= AM33XX_PRM_PARTITION,
+	.prcm_offs		= AM33XX_PRM_GFX_MOD,
+	.pwrsts			= PWRSTS_OFF_RET_ON,
+	.pwrsts_logic_ret	= PWRSTS_OFF_RET,
+	.pwrstctrl_offs		= AM33XX_PM_GFX_PWRSTCTRL_OFFSET,
+	.pwrstst_offs		= AM33XX_PM_GFX_PWRSTST_OFFSET,
+	.flags			= PWRDM_HAS_LOWPOWERSTATECHANGE,
+	.banks			= 1,
+	.pwrsts_mem_ret		= {
+		[0]	= PWRSTS_OFF_RET,	/* gfx_mem */
+	},
+	.pwrsts_mem_on		= {
+		[0]	= PWRSTS_ON,		/* gfx_mem */
+	},
+};
+
+static struct powerdomain rtc_33xx_pwrdm = {
+	.name			= "rtc_pwrdm",
+	.voltdm			= { .name = "rtc" },
+	.prcm_partition		= AM33XX_PRM_PARTITION,
+	.prcm_offs		= AM33XX_PRM_RTC_MOD,
+	.pwrsts			= PWRSTS_ON,
+	.pwrstctrl_offs		= AM33XX_PM_RTC_PWRSTCTRL_OFFSET,
+	.pwrstst_offs		= AM33XX_PM_RTC_PWRSTST_OFFSET,
+};
+
+static struct powerdomain wkup_33xx_pwrdm = {
+	.name			= "wkup_pwrdm",
+	.voltdm			= { .name = "core" },
+	.prcm_partition		= AM33XX_PRM_PARTITION,
+	.prcm_offs		= AM33XX_PRM_WKUP_MOD,
+	.pwrsts			= PWRSTS_ON,
+	.pwrstctrl_offs		= AM33XX_PM_WKUP_PWRSTCTRL_OFFSET,
+	.pwrstst_offs		= AM33XX_PM_WKUP_PWRSTST_OFFSET,
+};
+
+static struct powerdomain per_33xx_pwrdm = {
+	.name			= "per_pwrdm",
+	.voltdm			= { .name = "core" },
+	.prcm_partition		= AM33XX_PRM_PARTITION,
+	.prcm_offs		= AM33XX_PRM_PER_MOD,
+	.pwrsts			= PWRSTS_OFF_RET_ON,
+	.pwrsts_logic_ret	= PWRSTS_OFF_RET,
+	.pwrstctrl_offs		= AM33XX_PM_PER_PWRSTCTRL_OFFSET,
+	.pwrstst_offs		= AM33XX_PM_PER_PWRSTST_OFFSET,
+	.flags			= PWRDM_HAS_LOWPOWERSTATECHANGE,
+	.banks			= 3,
+	.pwrsts_mem_ret		= {
+		[0]	= PWRSTS_OFF_RET,	/* icss_mem */
+		[1]	= PWRSTS_OFF_RET,	/* per_mem */
+		[2]	= PWRSTS_OFF_RET,	/* ram_mem */
+	},
+	.pwrsts_mem_on		= {
+		[0]	= PWRSTS_ON,		/* icss_mem */
+		[1]	= PWRSTS_ON,		/* per_mem */
+		[2]	= PWRSTS_ON,		/* ram_mem */
+	},
+};
+
+static struct powerdomain mpu_33xx_pwrdm = {
+	.name			= "mpu_pwrdm",
+	.voltdm			= { .name = "mpu" },
+	.prcm_partition		= AM33XX_PRM_PARTITION,
+	.prcm_offs		= AM33XX_PRM_MPU_MOD,
+	.pwrsts			= PWRSTS_OFF_RET_ON,
+	.pwrsts_logic_ret	= PWRSTS_OFF_RET,
+	.pwrstctrl_offs		= AM33XX_PM_MPU_PWRSTCTRL_OFFSET,
+	.pwrstst_offs		= AM33XX_PM_MPU_PWRSTST_OFFSET,
+	.flags			= PWRDM_HAS_LOWPOWERSTATECHANGE,
+	.banks			= 3,
+	.pwrsts_mem_ret		= {
+		[0]	= PWRSTS_OFF_RET,	/* mpu_l1 */
+		[1]	= PWRSTS_OFF_RET,	/* mpu_l2 */
+		[2]	= PWRSTS_OFF_RET,	/* mpu_ram */
+	},
+	.pwrsts_mem_on		= {
+		[0]	= PWRSTS_ON,		/* mpu_l1 */
+		[1]	= PWRSTS_ON,		/* mpu_l2 */
+		[2]	= PWRSTS_ON,		/* mpu_ram */
+	},
+};
+
+static struct powerdomain cefuse_33xx_pwrdm = {
+	.name			= "cefuse_pwrdm",
+	.voltdm			= { .name = "core" },
+	.prcm_partition		= AM33XX_PRM_PARTITION,
+	.prcm_offs		= AM33XX_PRM_CEFUSE_MOD,
+	.pwrsts			= PWRSTS_OFF_ON,
+	.pwrstctrl_offs		= AM33XX_PM_CEFUSE_PWRSTCTRL_OFFSET,
+	.pwrstst_offs		= AM33XX_PM_CEFUSE_PWRSTST_OFFSET,
+};
+
+static struct powerdomain *powerdomains_am33xx[] __initdata = {
+	&gfx_33xx_pwrdm,
+	&rtc_33xx_pwrdm,
+	&wkup_33xx_pwrdm,
+	&per_33xx_pwrdm,
+	&mpu_33xx_pwrdm,
+	&cefuse_33xx_pwrdm,
+	NULL,
+};
+
+void __init am33xx_powerdomains_init(void)
+{
+	pwrdm_register_platform_funcs(&omap4_pwrdm_operations);
+	pwrdm_register_pwrdms(powerdomains_am33xx);
+	pwrdm_complete_init();
+}
diff --git a/arch/arm/mach-omap2/prm33xx.h b/arch/arm/mach-omap2/prm33xx.h
new file mode 100644
index 0000000..aa1e8c7
--- /dev/null
+++ b/arch/arm/mach-omap2/prm33xx.h
@@ -0,0 +1,118 @@
+/*
+ * AM33XX PRM instance offset macros
+ *
+ * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __ARCH_ARM_MACH_OMAP2_PRM33XX_H
+#define __ARCH_ARM_MACH_OMAP2_PRM33XX_H
+
+#include "prcm-common.h"
+#include "prm.h"
+
+#define AM33XX_PRM_BASE               0x44E00000
+
+#define AM33XX_PRM_REGADDR(inst, reg)                         \
+	AM33XX_L4_WK_IO_ADDRESS(AM33XX_PRM_BASE + (inst) + (reg))
+
+/* PRM instances */
+#define AM33XX_PRM_OCP_SOCKET_MOD	0x0B00
+#define AM33XX_PRM_PER_MOD		0x0C00
+#define AM33XX_PRM_WKUP_MOD		0x0D00
+#define AM33XX_PRM_MPU_MOD		0x0E00
+#define AM33XX_PRM_DEVICE_MOD		0x0F00
+#define AM33XX_PRM_RTC_MOD		0x1000
+#define AM33XX_PRM_GFX_MOD		0x1100
+#define AM33XX_PRM_CEFUSE_MOD		0x1200
+
+/* PRM */
+
+/* PRM.OCP_SOCKET_PRM register offsets */
+#define AM33XX_REVISION_PRM_OFFSET		0x0000
+#define AM33XX_REVISION_PRM			AM33XX_PRM_REGADDR(AM33XX_PRM_OCP_SOCKET_MOD, 0x0000)
+#define AM33XX_PRM_IRQSTATUS_MPU_OFFSET		0x0004
+#define AM33XX_PRM_IRQSTATUS_MPU		AM33XX_PRM_REGADDR(AM33XX_PRM_OCP_SOCKET_MOD, 0x0004)
+#define AM33XX_PRM_IRQENABLE_MPU_OFFSET		0x0008
+#define AM33XX_PRM_IRQENABLE_MPU		AM33XX_PRM_REGADDR(AM33XX_PRM_OCP_SOCKET_MOD, 0x0008)
+#define AM33XX_PRM_IRQSTATUS_M3_OFFSET		0x000c
+#define AM33XX_PRM_IRQSTATUS_M3			AM33XX_PRM_REGADDR(AM33XX_PRM_OCP_SOCKET_MOD, 0x000c)
+#define AM33XX_PRM_IRQENABLE_M3_OFFSET		0x0010
+#define AM33XX_PRM_IRQENABLE_M3			AM33XX_PRM_REGADDR(AM33XX_PRM_OCP_SOCKET_MOD, 0x0010)
+
+/* PRM.PER_PRM register offsets */
+#define AM33XX_RM_PER_RSTCTRL_OFFSET		0x0000
+#define AM33XX_RM_PER_RSTCTRL			AM33XX_PRM_REGADDR(AM33XX_PRM_PER_MOD, 0x0000)
+#define AM33XX_RM_PER_RSTST_OFFSET		0x0004
+#define AM33XX_RM_PER_RSTST			AM33XX_PRM_REGADDR(AM33XX_PRM_PER_MOD, 0x0004)
+#define AM33XX_PM_PER_PWRSTST_OFFSET		0x0008
+#define AM33XX_PM_PER_PWRSTST			AM33XX_PRM_REGADDR(AM33XX_PRM_PER_MOD, 0x0008)
+#define AM33XX_PM_PER_PWRSTCTRL_OFFSET		0x000c
+#define AM33XX_PM_PER_PWRSTCTRL			AM33XX_PRM_REGADDR(AM33XX_PRM_PER_MOD, 0x000c)
+
+/* PRM.WKUP_PRM register offsets */
+#define AM33XX_RM_WKUP_RSTCTRL_OFFSET		0x0000
+#define AM33XX_RM_WKUP_RSTCTRL			AM33XX_PRM_REGADDR(AM33XX_PRM_WKUP_MOD, 0x0000)
+#define AM33XX_PM_WKUP_PWRSTCTRL_OFFSET		0x0004
+#define AM33XX_PM_WKUP_PWRSTCTRL		AM33XX_PRM_REGADDR(AM33XX_PRM_WKUP_MOD, 0x0004)
+#define AM33XX_PM_WKUP_PWRSTST_OFFSET		0x0008
+#define AM33XX_PM_WKUP_PWRSTST			AM33XX_PRM_REGADDR(AM33XX_PRM_WKUP_MOD, 0x0008)
+#define AM33XX_RM_WKUP_RSTST_OFFSET		0x000c
+#define AM33XX_RM_WKUP_RSTST			AM33XX_PRM_REGADDR(AM33XX_PRM_WKUP_MOD, 0x000c)
+
+/* PRM.MPU_PRM register offsets */
+#define AM33XX_PM_MPU_PWRSTCTRL_OFFSET		0x0000
+#define AM33XX_PM_MPU_PWRSTCTRL			AM33XX_PRM_REGADDR(AM33XX_PRM_MPU_MOD, 0x0000)
+#define AM33XX_PM_MPU_PWRSTST_OFFSET		0x0004
+#define AM33XX_PM_MPU_PWRSTST			AM33XX_PRM_REGADDR(AM33XX_PRM_MPU_MOD, 0x0004)
+#define AM33XX_RM_MPU_RSTST_OFFSET		0x0008
+#define AM33XX_RM_MPU_RSTST			AM33XX_PRM_REGADDR(AM33XX_PRM_MPU_MOD, 0x0008)
+
+/* PRM.DEVICE_PRM register offsets */
+#define AM33XX_PRM_RSTCTRL_OFFSET		0x0000
+#define AM33XX_PRM_RSTCTRL			AM33XX_PRM_REGADDR(AM33XX_PRM_DEVICE_MOD, 0x0000)
+#define AM33XX_PRM_RSTTIME_OFFSET		0x0004
+#define AM33XX_PRM_RSTTIME			AM33XX_PRM_REGADDR(AM33XX_PRM_DEVICE_MOD, 0x0004)
+#define AM33XX_PRM_RSTST_OFFSET			0x0008
+#define AM33XX_PRM_RSTST			AM33XX_PRM_REGADDR(AM33XX_PRM_DEVICE_MOD, 0x0008)
+#define AM33XX_PRM_SRAM_COUNT_OFFSET		0x000c
+#define AM33XX_PRM_SRAM_COUNT			AM33XX_PRM_REGADDR(AM33XX_PRM_DEVICE_MOD, 0x000c)
+#define AM33XX_PRM_LDO_SRAM_CORE_SETUP_OFFSET	0x0010
+#define AM33XX_PRM_LDO_SRAM_CORE_SETUP		AM33XX_PRM_REGADDR(AM33XX_PRM_DEVICE_MOD, 0x0010)
+#define AM33XX_PRM_LDO_SRAM_CORE_CTRL_OFFSET	0x0014
+#define AM33XX_PRM_LDO_SRAM_CORE_CTRL		AM33XX_PRM_REGADDR(AM33XX_PRM_DEVICE_MOD, 0x0014)
+#define AM33XX_PRM_LDO_SRAM_MPU_SETUP_OFFSET	0x0018
+#define AM33XX_PRM_LDO_SRAM_MPU_SETUP		AM33XX_PRM_REGADDR(AM33XX_PRM_DEVICE_MOD, 0x0018)
+#define AM33XX_PRM_LDO_SRAM_MPU_CTRL_OFFSET	0x001c
+#define AM33XX_PRM_LDO_SRAM_MPU_CTRL		AM33XX_PRM_REGADDR(AM33XX_PRM_DEVICE_MOD, 0x001c)
+
+/* PRM.RTC_PRM register offsets */
+#define AM33XX_PM_RTC_PWRSTCTRL_OFFSET		0x0000
+#define AM33XX_PM_RTC_PWRSTCTRL			AM33XX_PRM_REGADDR(AM33XX_PRM_RTC_MOD, 0x0000)
+#define AM33XX_PM_RTC_PWRSTST_OFFSET		0x0004
+#define AM33XX_PM_RTC_PWRSTST			AM33XX_PRM_REGADDR(AM33XX_PRM_RTC_MOD, 0x0004)
+
+/* PRM.GFX_PRM register offsets */
+#define AM33XX_PM_GFX_PWRSTCTRL_OFFSET		0x0000
+#define AM33XX_PM_GFX_PWRSTCTRL			AM33XX_PRM_REGADDR(AM33XX_PRM_GFX_MOD, 0x0000)
+#define AM33XX_RM_GFX_RSTCTRL_OFFSET		0x0004
+#define AM33XX_RM_GFX_RSTCTRL			AM33XX_PRM_REGADDR(AM33XX_PRM_GFX_MOD, 0x0004)
+#define AM33XX_PM_GFX_PWRSTST_OFFSET		0x0010
+#define AM33XX_PM_GFX_PWRSTST			AM33XX_PRM_REGADDR(AM33XX_PRM_GFX_MOD, 0x0010)
+#define AM33XX_RM_GFX_RSTST_OFFSET		0x0014
+#define AM33XX_RM_GFX_RSTST			AM33XX_PRM_REGADDR(AM33XX_PRM_GFX_MOD, 0x0014)
+
+/* PRM.CEFUSE_PRM register offsets */
+#define AM33XX_PM_CEFUSE_PWRSTCTRL_OFFSET	0x0000
+#define AM33XX_PM_CEFUSE_PWRSTCTRL		AM33XX_PRM_REGADDR(AM33XX_PRM_CEFUSE_MOD, 0x0000)
+#define AM33XX_PM_CEFUSE_PWRSTST_OFFSET		0x0004
+#define AM33XX_PM_CEFUSE_PWRSTST		AM33XX_PRM_REGADDR(AM33XX_PRM_CEFUSE_MOD, 0x0004)
+#endif
--
1.7.0.4

Re: [PATCH-V2 2/2] arm:omap:am33xx: Add power domain data

[Paul Walmsley]

Hi

a question -

On Sun, 25 Dec 2011, Vaibhav Hiremath wrote:

> +static struct powerdomain mpu_33xx_pwrdm = {
> +	.name			= "mpu_pwrdm",
> +	.voltdm			= { .name = "mpu" },
> +	.prcm_partition		= AM33XX_PRM_PARTITION,
> +	.prcm_offs		= AM33XX_PRM_MPU_MOD,
> +	.pwrsts			= PWRSTS_OFF_RET_ON,
> +	.pwrsts_logic_ret	= PWRSTS_OFF_RET,
> +	.pwrstctrl_offs		= AM33XX_PM_MPU_PWRSTCTRL_OFFSET,
> +	.pwrstst_offs		= AM33XX_PM_MPU_PWRSTST_OFFSET,
> +	.flags			= PWRDM_HAS_LOWPOWERSTATECHANGE,
> +	.banks			= 3,
> +	.pwrsts_mem_ret		= {
> +		[0]	= PWRSTS_OFF_RET,	/* mpu_l1 */
> +		[1]	= PWRSTS_OFF_RET,	/* mpu_l2 */
> +		[2]	= PWRSTS_OFF_RET,	/* mpu_ram */
> +	},
> +	.pwrsts_mem_on		= {
> +		[0]	= PWRSTS_ON,		/* mpu_l1 */
> +		[1]	= PWRSTS_ON,		/* mpu_l2 */
> +		[2]	= PWRSTS_ON,		/* mpu_ram */
> +	},
> +};

Comparing this with Table 8-191 "PM_MPU_PWRSTCTRL Register Field 
Descriptions" of the AM335x TRM Rev C [SPRUH73C], it seems that something 
is really wrong here.

On the OMAP4430's *_PWRSTCTRL registers,

- the memory bank RETSTATE fields always start at bit 8;

- the RETSTATE fields are contiguous;

- the bank ONSTATE fields always start at bit 16; and 

- the ONSTATE fields are contiguous;

- the order of the RETSTATE and ONSTATE fields always matches.

The OMAP4430 TRM Rev O [SWPU231O] Table 3-449 "PM_CORE_PWRSTCTRL" table 
is a good illustration of this.

But, looking at SPRUH73C, none of these criteria seem to apply 
consistently on AM335x :-(

Table 8-191 "PM_MPU_PWRSTCTRL Register Field Descriptions" starts its 
RETSTATE bitfields at bit 22, not bit 8.  The ONSTATE bitfields are at bit 
16 (at least this part is normal).  And both sets of bitfields are 
internally contiguous.  But then the order of the bitfields does not match 
between the RETSTATE bitfields and ONSTATE bitfields - the RETSTATE order 
is (L1, L2, RAM), but the ONSTATE order is (RAM, L1, L2)!

Then looking at Table 8-184 "PM_PER_PWRSTCTRL Register Field Descriptions" 
for the same chip, the layout is completely different.  The PRUSS memory 
ONSTATE starts at bit 5, and its RETSTATE field is at bit 7.  But then 
PER_MEM's ONSTATE field starts at bit 25, and is followed immediately by 
*RAM*_MEM's RETSTATE field at bit 27!  And then PER_MEM's RETSTATE field 
shows up at bit 29, followed by RAM_MEM's ONSTATE field at bit 30.

Is the TRM accurate in these examples?  Or is this a documentation bug?

Anyway, if this is really accurate, this per-powerdomain bitfield 
reordering is definitely not expected by the existing powerdomain code.  
Looks to me that someone will have to add bitshift fields into the struct 
powerdomain for every single bank RETSTATE and ONSTATE field.

Does this match your understanding?


- Paul

RE: [PATCH-V2 2/2] arm:omap:am33xx: Add power domain data

[Hiremath, Vaibhav]

On Thu, Mar 01, 2012 at 07:07:01, Paul Walmsley wrote:
> Hi
> 
> a question -
> 
> On Sun, 25 Dec 2011, Vaibhav Hiremath wrote:
> 
> > +static struct powerdomain mpu_33xx_pwrdm = {
> > +	.name			= "mpu_pwrdm",
> > +	.voltdm			= { .name = "mpu" },
> > +	.prcm_partition		= AM33XX_PRM_PARTITION,
> > +	.prcm_offs		= AM33XX_PRM_MPU_MOD,
> > +	.pwrsts			= PWRSTS_OFF_RET_ON,
> > +	.pwrsts_logic_ret	= PWRSTS_OFF_RET,
> > +	.pwrstctrl_offs		= AM33XX_PM_MPU_PWRSTCTRL_OFFSET,
> > +	.pwrstst_offs		= AM33XX_PM_MPU_PWRSTST_OFFSET,
> > +	.flags			= PWRDM_HAS_LOWPOWERSTATECHANGE,
> > +	.banks			= 3,
> > +	.pwrsts_mem_ret		= {
> > +		[0]	= PWRSTS_OFF_RET,	/* mpu_l1 */
> > +		[1]	= PWRSTS_OFF_RET,	/* mpu_l2 */
> > +		[2]	= PWRSTS_OFF_RET,	/* mpu_ram */
> > +	},
> > +	.pwrsts_mem_on		= {
> > +		[0]	= PWRSTS_ON,		/* mpu_l1 */
> > +		[1]	= PWRSTS_ON,		/* mpu_l2 */
> > +		[2]	= PWRSTS_ON,		/* mpu_ram */
> > +	},
> > +};
> 
> Comparing this with Table 8-191 "PM_MPU_PWRSTCTRL Register Field 
> Descriptions" of the AM335x TRM Rev C [SPRUH73C], it seems that something 
> is really wrong here.
> 
> On the OMAP4430's *_PWRSTCTRL registers,
> 
> - the memory bank RETSTATE fields always start at bit 8;
> 
> - the RETSTATE fields are contiguous;
> 
> - the bank ONSTATE fields always start at bit 16; and 
> 
> - the ONSTATE fields are contiguous;
> 
> - the order of the RETSTATE and ONSTATE fields always matches.
> 
> The OMAP4430 TRM Rev O [SWPU231O] Table 3-449 "PM_CORE_PWRSTCTRL" table 
> is a good illustration of this.
> 
> But, looking at SPRUH73C, none of these criteria seem to apply 
> consistently on AM335x :-(
> 
> Table 8-191 "PM_MPU_PWRSTCTRL Register Field Descriptions" starts its 
> RETSTATE bitfields at bit 22, not bit 8.  The ONSTATE bitfields are at bit 
> 16 (at least this part is normal).  And both sets of bitfields are 
> internally contiguous.  But then the order of the bitfields does not match 
> between the RETSTATE bitfields and ONSTATE bitfields - the RETSTATE order 
> is (L1, L2, RAM), but the ONSTATE order is (RAM, L1, L2)!
> 
> Then looking at Table 8-184 "PM_PER_PWRSTCTRL Register Field Descriptions" 
> for the same chip, the layout is completely different.  The PRUSS memory 
> ONSTATE starts at bit 5, and its RETSTATE field is at bit 7.  But then 
> PER_MEM's ONSTATE field starts at bit 25, and is followed immediately by 
> *RAM*_MEM's RETSTATE field at bit 27!  And then PER_MEM's RETSTATE field 
> shows up at bit 29, followed by RAM_MEM's ONSTATE field at bit 30.
> 
> Is the TRM accurate in these examples?  Or is this a documentation bug?
> 

Unfortunately, the spec is correct in this context.

> Anyway, if this is really accurate, this per-powerdomain bitfield 
> reordering is definitely not expected by the existing powerdomain code.  
> Looks to me that someone will have to add bitshift fields into the struct 
> powerdomain for every single bank RETSTATE and ONSTATE field.
> 

That would be me... :)
I have started looking at it and will soon submit the patches for this.

Side note,

I am not sure whether we access these bits explicitly. If I understand correctly, API's which access these bits are not being called from anywhere, 
For example,

	omap4_pwrdm_read_mem_retst
	omap4_pwrdm_set_mem_retst
	omap4_pwrdm_set_mem_onst

Currently the major issue would be, for PM_PER_PWRSTCTRL, where 
LogicRETState bit has been moved to offset 3. This is being used in 
omap4_pwrdm_set_logic_retst() api.

And in order to support weird/non-standard bit-offsets, we have to clean omap2_pwrdm_get_mem_bank_onstate/retst/_mask() api and have bitshift field in struct powerdomain, as you have rightly pointed out.

Is my all understanding correct?

If yes, do you think this patch should get blocked for above cleanup? I feel
Cleanup can still follow, since this won't impact the AM33xx boot.

What's your opinion on this??

> Does this match your understanding?
> 

Yes, certainly.


Thanks,
Vaibhav

> 
> - Paul
> 

RE: [PATCH-V2 2/2] arm:omap:am33xx: Add power domain data

[Paul Walmsley]

Hi

On Thu, 1 Mar 2012, Hiremath, Vaibhav wrote:

> I am not sure whether we access these bits explicitly. If I understand 
> correctly, API's which access these bits are not being called from 
> anywhere, For example,
> 
> 	omap4_pwrdm_read_mem_retst
> 	omap4_pwrdm_set_mem_retst
> 	omap4_pwrdm_set_mem_onst

They'll be needed for the functional powerstate code, device PM QoS, and 
OSWR.

> Currently the major issue would be, for PM_PER_PWRSTCTRL, where 
> LogicRETState bit has been moved to offset 3. This is being used in 
> omap4_pwrdm_set_logic_retst() api.
> 
> And in order to support weird/non-standard bit-offsets, we have to clean 
> omap2_pwrdm_get_mem_bank_onstate/retst/_mask() api and have bitshift 
> field in struct powerdomain, as you have rightly pointed out.

As you work on this, I have a request.  Please place the AM33xx 
powerdomain implementation functions into a separate file, 
powerdomain33xx.c perhaps, rather than trying to merge them with the OMAP4 
powerdomain implementation functions.

Some functions can be shared -- maybe place those into a separate file, 
powerdomain33xx_44xx.c perhaps?

> Is my all understanding correct?
> 
> If yes, do you think this patch should get blocked for above cleanup? I feel
> Cleanup can still follow, since this won't impact the AM33xx boot.
> 
> What's your opinion on this??

We can't merge patches which are known to be broken.  That's a rule from 
Linus (and a good one).  Better to try to get it right the first time it 
hits mainline.


- Paul

RE: [PATCH-V2 2/2] arm:omap:am33xx: Add power domain data

[Hiremath, Vaibhav]

On Fri, Mar 02, 2012 at 11:19:40, Paul Walmsley wrote:
> Hi
> 
> On Thu, 1 Mar 2012, Hiremath, Vaibhav wrote:
> 
> > I am not sure whether we access these bits explicitly. If I understand 
> > correctly, API's which access these bits are not being called from 
> > anywhere, For example,
> > 
> > 	omap4_pwrdm_read_mem_retst
> > 	omap4_pwrdm_set_mem_retst
> > 	omap4_pwrdm_set_mem_onst
> 
> They'll be needed for the functional powerstate code, device PM QoS, and 
> OSWR.
> 
> > Currently the major issue would be, for PM_PER_PWRSTCTRL, where 
> > LogicRETState bit has been moved to offset 3. This is being used in 
> > omap4_pwrdm_set_logic_retst() api.
> > 
> > And in order to support weird/non-standard bit-offsets, we have to clean 
> > omap2_pwrdm_get_mem_bank_onstate/retst/_mask() api and have bitshift 
> > field in struct powerdomain, as you have rightly pointed out.
> 
> As you work on this, I have a request.  Please place the AM33xx 
> powerdomain implementation functions into a separate file, 
> powerdomain33xx.c perhaps, rather than trying to merge them with the OMAP4 
> powerdomain implementation functions.
> 

That was my initial code and I had submitted same to the list as an RFC, 
Request you to refer to the link - 
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg58746.html


And the reason behind merging am33xx with omap4 thing was triggered
from the discussion on the above patch (between me, KevinH and RajendraNaik), 
and we all believed that, with some cleanup in OMAP4 code we can reuse it 
for am33xx.

Thanks,
Vaibhav

> Some functions can be shared -- maybe place those into a separate file, 
> powerdomain33xx_44xx.c perhaps?
> 
> > Is my all understanding correct?
> > 
> > If yes, do you think this patch should get blocked for above cleanup? I feel
> > Cleanup can still follow, since this won't impact the AM33xx boot.
> > 
> > What's your opinion on this??
> 
> We can't merge patches which are known to be broken.  That's a rule from 
> Linus (and a good one).  Better to try to get it right the first time it 
> hits mainline.
> 
> 
> - Paul
> 

Re: [PATCH-V2 2/2] arm:omap:am33xx: Add power domain data

[Paul Walmsley]

Hi

some other observations:

On Sun, 25 Dec 2011, Vaibhav Hiremath wrote:

> +static struct powerdomain per_33xx_pwrdm = {
> +	.name			= "per_pwrdm",
> +	.voltdm			= { .name = "core" },
> +	.prcm_partition		= AM33XX_PRM_PARTITION,
> +	.prcm_offs		= AM33XX_PRM_PER_MOD,
> +	.pwrsts			= PWRSTS_OFF_RET_ON,
> +	.pwrsts_logic_ret	= PWRSTS_OFF_RET,
> +	.pwrstctrl_offs		= AM33XX_PM_PER_PWRSTCTRL_OFFSET,
> +	.pwrstst_offs		= AM33XX_PM_PER_PWRSTST_OFFSET,
> +	.flags			= PWRDM_HAS_LOWPOWERSTATECHANGE,
> +	.banks			= 3,
> +	.pwrsts_mem_ret		= {
> +		[0]	= PWRSTS_OFF_RET,	/* icss_mem */
> +		[1]	= PWRSTS_OFF_RET,	/* per_mem */
> +		[2]	= PWRSTS_OFF_RET,	/* ram_mem */
> +	},
> +	.pwrsts_mem_on		= {
> +		[0]	= PWRSTS_ON,		/* icss_mem */
> +		[1]	= PWRSTS_ON,		/* per_mem */
> +		[2]	= PWRSTS_ON,		/* ram_mem */
> +	},
> +};

According to SPRUH73C Table 8-184 "PM_PER_PWRSTCTRL Register Field 
Descriptions" the pwrsts_mem_on field for RAM_MEM should be 
PWRSTS_OFF_RET_ON.

> +static struct powerdomain mpu_33xx_pwrdm = {
> +     .name                   = "mpu_pwrdm",
> +     .voltdm                 = { .name = "mpu" },
> +     .prcm_partition         = AM33XX_PRM_PARTITION,
> +     .prcm_offs              = AM33XX_PRM_MPU_MOD,
> +     .pwrsts                 = PWRSTS_OFF_RET_ON,
> +     .pwrsts_logic_ret       = PWRSTS_OFF_RET,
> +     .pwrstctrl_offs         = AM33XX_PM_MPU_PWRSTCTRL_OFFSET,
> +     .pwrstst_offs           = AM33XX_PM_MPU_PWRSTST_OFFSET,
> +     .flags                  = PWRDM_HAS_LOWPOWERSTATECHANGE,
> +     .banks                  = 3,
> +     .pwrsts_mem_ret         = {
> +             [0]     = PWRSTS_OFF_RET,       /* mpu_l1 */
> +             [1]     = PWRSTS_OFF_RET,       /* mpu_l2 */
> +             [2]     = PWRSTS_OFF_RET,       /* mpu_ram */
> +     },

Again SPRUH73C Table 8-191 "PM_MPU_PWRSTCTRL Register Field Descriptions" 
claims these should simply by PWRSTS_RET.

> +     .pwrsts_mem_on          = {
> +             [0]     = PWRSTS_ON,            /* mpu_l1 */
> +             [1]     = PWRSTS_ON,            /* mpu_l2 */
> +             [2]     = PWRSTS_ON,            /* mpu_ram */
> +     },
> +};

And the same table claims the MPU_RAM pwrsts_mem_on field should be 
PWRSTS_OFF_ON.

Can you please reconcile these differences and let us know which values 
should be correct?

thanks


- Paul

RE: [PATCH-V2 2/2] arm:omap:am33xx: Add power domain data

[Hiremath, Vaibhav]

On Thu, Mar 01, 2012 at 07:14:24, Paul Walmsley wrote:
> Hi
> 
> some other observations:
> 
> On Sun, 25 Dec 2011, Vaibhav Hiremath wrote:
> 
> > +static struct powerdomain per_33xx_pwrdm = {
> > +	.name			= "per_pwrdm",
> > +	.voltdm			= { .name = "core" },
> > +	.prcm_partition		= AM33XX_PRM_PARTITION,
> > +	.prcm_offs		= AM33XX_PRM_PER_MOD,
> > +	.pwrsts			= PWRSTS_OFF_RET_ON,
> > +	.pwrsts_logic_ret	= PWRSTS_OFF_RET,
> > +	.pwrstctrl_offs		= AM33XX_PM_PER_PWRSTCTRL_OFFSET,
> > +	.pwrstst_offs		= AM33XX_PM_PER_PWRSTST_OFFSET,
> > +	.flags			= PWRDM_HAS_LOWPOWERSTATECHANGE,
> > +	.banks			= 3,
> > +	.pwrsts_mem_ret		= {
> > +		[0]	= PWRSTS_OFF_RET,	/* icss_mem */
> > +		[1]	= PWRSTS_OFF_RET,	/* per_mem */
> > +		[2]	= PWRSTS_OFF_RET,	/* ram_mem */
> > +	},
> > +	.pwrsts_mem_on		= {
> > +		[0]	= PWRSTS_ON,		/* icss_mem */
> > +		[1]	= PWRSTS_ON,		/* per_mem */
> > +		[2]	= PWRSTS_ON,		/* ram_mem */
> > +	},
> > +};
> 
> According to SPRUH73C Table 8-184 "PM_PER_PWRSTCTRL Register Field 
> Descriptions" the pwrsts_mem_on field for RAM_MEM should be 
> PWRSTS_OFF_RET_ON.
> 
> > +static struct powerdomain mpu_33xx_pwrdm = {
> > +     .name                   = "mpu_pwrdm",
> > +     .voltdm                 = { .name = "mpu" },
> > +     .prcm_partition         = AM33XX_PRM_PARTITION,
> > +     .prcm_offs              = AM33XX_PRM_MPU_MOD,
> > +     .pwrsts                 = PWRSTS_OFF_RET_ON,
> > +     .pwrsts_logic_ret       = PWRSTS_OFF_RET,
> > +     .pwrstctrl_offs         = AM33XX_PM_MPU_PWRSTCTRL_OFFSET,
> > +     .pwrstst_offs           = AM33XX_PM_MPU_PWRSTST_OFFSET,
> > +     .flags                  = PWRDM_HAS_LOWPOWERSTATECHANGE,
> > +     .banks                  = 3,
> > +     .pwrsts_mem_ret         = {
> > +             [0]     = PWRSTS_OFF_RET,       /* mpu_l1 */
> > +             [1]     = PWRSTS_OFF_RET,       /* mpu_l2 */
> > +             [2]     = PWRSTS_OFF_RET,       /* mpu_ram */
> > +     },
> 
> Again SPRUH73C Table 8-191 "PM_MPU_PWRSTCTRL Register Field Descriptions" 
> claims these should simply by PWRSTS_RET.
> 
> > +     .pwrsts_mem_on          = {
> > +             [0]     = PWRSTS_ON,            /* mpu_l1 */
> > +             [1]     = PWRSTS_ON,            /* mpu_l2 */
> > +             [2]     = PWRSTS_ON,            /* mpu_ram */
> > +     },
> > +};
> 
> And the same table claims the MPU_RAM pwrsts_mem_on field should be 
> PWRSTS_OFF_ON.
> 
> Can you please reconcile these differences and let us know which values 
> should be correct?
> 

Paul,

The initialization used in the patch is correct. Unfortunately, the TRM is 
incorrect here. I have already raised this issue with respective folks and soon it will get corrected.

Thanks,
Vaibhav

> thanks
> 
> 
> - Paul
> 

RE: [PATCH-V2 2/2] arm:omap:am33xx: Add power domain data

[Paul Walmsley]

Hi,

On Thu, 1 Mar 2012, Hiremath, Vaibhav wrote:

> The initialization used in the patch is correct. Unfortunately, the TRM 
> is incorrect here. I have already raised this issue with respective 
> folks and soon it will get corrected.

thanks for looking into this.  Could you add a brief comment or comments 
in this file to indicate that the TRM Rev C is incorrect about those?  
That will help others who may be reviewing this code as they track down 
bugs.


thanks


- Paul

RE: [PATCH-V2 2/2] arm:omap:am33xx: Add power domain data

[Hiremath, Vaibhav]

On Fri, Mar 02, 2012 at 12:02:31, Paul Walmsley wrote:
> Hi,
> 
> On Thu, 1 Mar 2012, Hiremath, Vaibhav wrote:
> 
> > The initialization used in the patch is correct. Unfortunately, the TRM 
> > is incorrect here. I have already raised this issue with respective 
> > folks and soon it will get corrected.
> 
> thanks for looking into this.  Could you add a brief comment or comments 
> in this file to indicate that the TRM Rev C is incorrect about those?  
> That will help others who may be reviewing this code as they track down 
> bugs.
> 

Good point. I will certainly add it in the next version of patch-sets.

Paul,
Thanks for your review comments, can we also align on the approach, 
Whether to merge am33xx powerdomain with omap4 (same direction we are now)
OR
Have separate implementation (my original approach).


Thanks,
Vaibhav
> 
> thanks
> 
> 
> - Paul
> 

RE: [PATCH-V2 2/2] arm:omap:am33xx: Add power domain data

[Paul Walmsley]

Hi

On Fri, 2 Mar 2012, Hiremath, Vaibhav wrote:

> Paul,
> Thanks for your review comments, can we also align on the approach, 
> Whether to merge am33xx powerdomain with omap4 (same direction we are now)
> OR
> Have separate implementation (my original approach).

Could you please take a look at the "am335x_prcm_devel_3.4" branch on 
git://git.pwsan.com/linux-2.6 and let me know what you think?

It is still rough, incomplete, and compile-tested only; and the patch 
commit messages have to be updated and revised; but I think it is a 
slightly better approach.

A significant change is that the prminst code changes have been removed.  
That is only needed when PRM registers are spread across multiple PRCM IP 
blocks.  This does not appear to be the case with AM33xx?  So the 
implementation has been modified to create a prm33xx.c file instead.  
This avoids an extra, unnecessary layer of indirection for PRM accesses.

This, along with the other differences between OMAP4 and this chip, means 
that a separate powerdomain33xx.c file had to be created.

The powerdomains33xx_data.c file is still missing the bitmask position 
data, but the structure members are there.

Feel free to combine, rework, or merge this series with what you are 
working on if you feel that it is a good approach.  Otherwise, let's 
discuss.


- Paul

RE: [PATCH-V2 2/2] arm:omap:am33xx: Add power domain data

[Hiremath, Vaibhav]

On Fri, Mar 02, 2012 at 16:16:47, Paul Walmsley wrote:
> Hi
> 
> On Fri, 2 Mar 2012, Hiremath, Vaibhav wrote:
> 
> > Paul,
> > Thanks for your review comments, can we also align on the approach, 
> > Whether to merge am33xx powerdomain with omap4 (same direction we are now)
> > OR
> > Have separate implementation (my original approach).
> 
> Could you please take a look at the "am335x_prcm_devel_3.4" branch on 
> git://git.pwsan.com/linux-2.6 and let me know what you think?
> 
> It is still rough, incomplete, and compile-tested only; and the patch 
> commit messages have to be updated and revised; but I think it is a 
> slightly better approach.
> 

Paul,

Sorry for delayed response, was bit stuck with some baseport thing.


I have reviewed the changes from your above branch. And as I mentioned 
before, it looks closer to my earlier approach + some good and important 
changes/cleanups.

> A significant change is that the prminst code changes have been removed.  
> That is only needed when PRM registers are spread across multiple PRCM IP 
> blocks.  

Make sense actually...I don't know, why didn't thought about this before...

> This does not appear to be the case with AM33xx?  So the 
> implementation has been modified to create a prm33xx.c file instead.  
> This avoids an extra, unnecessary layer of indirection for PRM accesses.
> 
> This, along with the other differences between OMAP4 and this chip, means 
> that a separate powerdomain33xx.c file had to be created.
> 
> The powerdomains33xx_data.c file is still missing the bitmask position 
> data, but the structure members are there.
> 

Yeah, I understand, I think without this it won't be usefule. I will also get this done in next (aligned) changes.

> Feel free to combine, rework, or merge this series with what you are 
> working on if you feel that it is a good approach.  Otherwise, let's 
> discuss.
> 

Thanks, Thanks a lot for your time/support/review, as always its helpful.


Thanks,
Vaibhav

> 
> - Paul
> 

Re: [PATCH-V2 0/2] arm:omap:am33xx: Add voltage and power domain data

[Kevin Hilman]

Vaibhav Hiremath <hvaibhav@ti.com> writes:

> Last time I had submitted a big patch-series, almost changing >7.5K lines
> for AM33XX voltage, power, clock and HWMOD data support (as a RFC);
> which I believe is very hard to review and difficult to manage also.
> So I decided to split the patches as and when they are clean
> and ready for review/merge/acceptance,

FYI... 

In usually (but not always) helps to specifically Cc the maintainers of
the code you're changin.  In this case, Paul & Benoit are maintaining
most of this core code/data, so you should make sure they are Cc'd as
well.

Thanks,

Kevin

RE: [PATCH-V2 0/2] arm:omap:am33xx: Add voltage and power domain data

[Hiremath, Vaibhav]

On Wed, Jan 11, 2012 at 05:09:48, Hilman, Kevin wrote:
> Vaibhav Hiremath <hvaibhav@ti.com> writes:
> 
> > Last time I had submitted a big patch-series, almost changing >7.5K lines
> > for AM33XX voltage, power, clock and HWMOD data support (as a RFC);
> > which I believe is very hard to review and difficult to manage also.
> > So I decided to split the patches as and when they are clean
> > and ready for review/merge/acceptance,
> 
> FYI... 
> 
> In usually (but not always) helps to specifically Cc the maintainers of
> the code you're changin.  In this case, Paul & Benoit are maintaining
> most of this core code/data, so you should make sure they are Cc'd as
> well.
> 

My bad. I fully agree with you and will make sure that I will follow this.

Thanks,
Vaibhav

> Thanks,
> 
> Kevin
>