arm:omap:am33xx: Add voltage and power domain data

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 <[email protected]>

Signed-off-by Vaibhav Hiremath <[email protected]>

Quoted 4 lines · 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).

Quoted 4 lines

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

Signed-off-by Afzal Mohammed <[email protected]>

Signed-off-by Vaibhav Hiremath <[email protected]>

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

arch/arm/mach-omap2/powerdomains33xx_data.c new file +12 -0

+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		= {

Hi

a question -

Quoted 12 lines · Vaibhav Hiremath

arch/arm/mach-omap2/powerdomains33xx_data.c new file +12 -0

+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		= {

arch/arm/mach-omap2/powerdomains33xx_data.c line 107

Quoted 10 lines

arch/arm/mach-omap2/powerdomains33xx_data.c new file +10 -0

+		[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

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??

Hi

Quoted 7 lines · Vaibhav

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.

Quoted 4 lines

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.

Quoted 3 lines

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?

Quoted 6 lines

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

arch/arm/mach-omap2/powerdomains33xx_data.c new file +12 -0

+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		= {

arch/arm/mach-omap2/powerdomains33xx_data.c new file +6 -0

+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,

Hi

some other observations:

Quoted 12 lines · Vaibhav Hiremath

arch/arm/mach-omap2/powerdomains33xx_data.c new file +12 -0

+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		= {

arch/arm/mach-omap2/powerdomains33xx_data.c line 84

Quoted 10 lines

arch/arm/mach-omap2/powerdomains33xx_data.c new file +10 -0

+		[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.

Quoted 6 lines

arch/arm/mach-omap2/powerdomains33xx_data.c new file +6 -0

+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,

arch/arm/mach-omap2/powerdomains33xx_data.c line 101

Quoted 10 lines

arch/arm/mach-omap2/powerdomains33xx_data.c new file +10 -0

+     .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.

arch/arm/mach-omap2/powerdomains33xx_data.c line 107

Quoted 6 lines

arch/arm/mach-omap2/powerdomains33xx_data.c new file +6 -0

+     .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

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.

Hi,

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

Quoted 3 lines

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

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).

Hi

Quoted 5 lines · Vaibhav

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

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,

Quoted 5 lines · 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,

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