Module + offset calculations have to be signed in arch/arm/mach-omap2/pm-debug.c

PATCH Peter Barada 2 participants 8 messages May 5, 2011 – May 6, 2011

Subsystems (4)

Arm (32-bit) Arm sub-architectures OMAP2+ support OMAP power management support

lore

Share & tools

lore

Peter Barada

author

2011-05-05 16:22 UTC #0 lore raw

I've been working on getting the TI OMAPPSP-03.00.01.06 kernel to properly suspend/resume on my DM37x board and all was going well until I added OTG support to the kernel and on suspend, the IVA2 and CORE pwrdms would not properly go into suspend. When comparing output from /debug/pm-debug/registers/current to the TRM, I noticed the following:

MOD: CM_IVA2 (48014000)

  04 => 00000017  20 => 00000001  24 => 00000001  34 => 00000001
  40 => 00080a00  44 => 00000001  48 => 00000003

MOD: PRM_IVA2 (48316000)
50 => 00000007 e0 => 00ff0f05 e4 => 00000ff7 e8 => 00000ff7 f8 => 00000001

Looking at the TRM, the PRM_IVA2 registers are at 0x48306000, not 0x48316000. OMAP3430_IVA2_MOD is defined in prcm-common.h as -0x800 which means any module + reg_offset address calculation has to be signed. Once I corrected the "unsigned short offset" declaration in pm_module_def, rebuilt and tested again, IVA2/core pwrdms go into suspend correctly (and addresses look correct):

Signed-off-by Peter Barada <[email protected]>

arch/arm/mach-omap2/pm-debug.c +1 -1

index 125f565..b731ef3 100644
@@ -199,7 +199,7 @@ enum {
 struct pm_module_def {
     char name[8]; /* Name of the module */
     short type; /* CM or PRM */
-    unsigned short offset;
+    short offset;
     int low; /* First register address on this module */
     int high; /* Last register address on this module */
 };

Signature · 2 lines

Peter Barada
[email protected]

Kevin Hilman

2011-05-05 17:11 UTC #1 lore raw

Quoted 13 lines · Peter Barada

I've been working on getting the TI OMAPPSP-03.00.01.06 kernel to properly suspend/resume on my DM37x board and all was going well until I added OTG support to the kernel and on suspend, the IVA2 and CORE pwrdms would not properly go into suspend. When comparing output from /debug/pm-debug/registers/current to the TRM, I noticed the following:
MOD: CM_IVA2 (48014000)
  04 => 00000017  20 => 00000001  24 => 00000001  34 => 00000001
  40 => 00080a00  44 => 00000001  48 => 00000003
MOD: PRM_IVA2 (48316000)
50 => 00000007 e0 => 00ff0f05 e4 => 00000ff7 e8 => 00000ff7 f8 => 00000001

Quoted 6 lines

Looking at the TRM, the PRM_IVA2 registers are at 0x48306000, not 0x48316000. OMAP3430_IVA2_MOD is defined in prcm-common.h as -0x800 which means any module + reg_offset address calculation has to be signed. Once I corrected the "unsigned short offset" declaration in pm_module_def, rebuilt and tested again, IVA2/core pwrdms go into suspend correctly (and addresses look correct):

Just to be clear, this should only affect the display of the registers, not whether or not the power domains actually suspend correctly, right?

Or, did you actually notice via some other method that this powerdomain was not hitting retention/off?

Kevin

Quoted 17 lines

Signed-off-by Peter Barada <[email protected]>

arch/arm/mach-omap2/pm-debug.c +1 -1

index 125f565..b731ef3 100644
@@ -199,7 +199,7 @@ enum {
 struct pm_module_def {
     char name[8]; /* Name of the module */
     short type; /* CM or PRM */
-    unsigned short offset;
+    short offset;
     int low; /* First register address on this module */
     int high; /* Last register address on this module */
 };

Peter Barada

author

2011-05-05 17:18 UTC #2 lore raw

Quoted 21 lines · Kevin Hilman

Quoted 13 lines · Peter Barada
I've been working on getting the TI OMAPPSP-03.00.01.06 kernel to properly suspend/resume on my DM37x board and all was going well until I added OTG support to the kernel and on suspend, the IVA2 and CORE pwrdms would not properly go into suspend. When comparing output from /debug/pm-debug/registers/current to the TRM, I noticed the following:
MOD: CM_IVA2 (48014000)
   04 =>  00000017  20 =>  00000001  24 =>  00000001  34 =>  00000001
   40 =>  00080a00  44 =>  00000001  48 =>  00000003
MOD: PRM_IVA2 (48316000)
50 => 00000007 e0 => 00ff0f05 e4 => 00000ff7 e8 => 00000ff7 f8 => 00000001
Quoted 6 lines
Looking at the TRM, the PRM_IVA2 registers are at 0x48306000, not 0x48316000. OMAP3430_IVA2_MOD is defined in prcm-common.h as -0x800 which means any module + reg_offset address calculation has to be signed. Once I corrected the "unsigned short offset" declaration in pm_module_def, rebuilt and tested again, IVA2/core pwrdms go into suspend correctly (and addresses look correct):

Quoted 5 lines

Just to be clear, this should only affect the display of the registers, not whether or not the power domains actually suspend correctly, right?
Or, did you actually notice via some other method that this powerdomain was not hitting retention/off?

Yes the change affects the registers displayed, but since the address calculation is incorrect with CONFIG_PM_DEBUG enabled, the PM_DEBUG code is reading the CM_IVA2/PRM_IVA2 registers at incorrect addresses. On my DM37x board when I suspended with OTG enabled (and without the patch), I see the following on resume:

[  496.168151] Powerdomain (iva2_pwrdm) didn't enter target state 1 (last power state 3)
[  496.176025] Powerdomain (core_pwrdm) didn't enter target state 1 (last power state 3)
[  496.183898] Could not enter target state in pm_suspend

With the patch (and same kernel config) I see the following on resume:

[  234.867889] Successfully put all powerdomains to target state

Before enabling the OTG code all powerdomains went into suspend.

It could be that accessing registers at the incorrect addresses is causing DM37x to keep those domains out of retention...

Quoted 22 lines

Kevin
Quoted 8 lines
Signed-off-by Peter Barada<[email protected]>
arch/arm/mach-omap2/pm-debug.c +0 -0
index 125f565..b731ef3 100644
arch/arm/mach-omap2/pm-debug.c enum { · line 205
Quoted 8 lines
arch/arm/mach-omap2/pm-debug.c +1 -1
@@ -199,7 +199,7 @@ enum {
  struct pm_module_def {
      char name[8]; /* Name of the module */
      short type; /* CM or PRM */
-    unsigned short offset;
+    short offset;
      int low; /* First register address on this module */
      int high; /* Last register address on this module */
  };
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines

Peter Barada
[email protected]

Kevin Hilman

2011-05-05 18:17 UTC #3 lore raw

Quoted 33 lines · Peter Barada

Quoted 21 lines · Kevin Hilman
Quoted 13 lines · Peter Barada
I've been working on getting the TI OMAPPSP-03.00.01.06 kernel to properly suspend/resume on my DM37x board and all was going well until I added OTG support to the kernel and on suspend, the IVA2 and CORE pwrdms would not properly go into suspend. When comparing output from /debug/pm-debug/registers/current to the TRM, I noticed the following:
MOD: CM_IVA2 (48014000)
   04 =>  00000017  20 =>  00000001  24 =>  00000001  34 =>  00000001
   40 =>  00080a00  44 =>  00000001  48 =>  00000003
MOD: PRM_IVA2 (48316000)
50 => 00000007 e0 => 00ff0f05 e4 => 00000ff7 e8 => 00000ff7 f8 => 00000001
Quoted 6 lines
Looking at the TRM, the PRM_IVA2 registers are at 0x48306000, not 0x48316000. OMAP3430_IVA2_MOD is defined in prcm-common.h as -0x800 which means any module + reg_offset address calculation has to be signed. Once I corrected the "unsigned short offset" declaration in pm_module_def, rebuilt and tested again, IVA2/core pwrdms go into suspend correctly (and addresses look correct):
Quoted 5 lines
Just to be clear, this should only affect the display of the registers, not whether or not the power domains actually suspend correctly, right?
Or, did you actually notice via some other method that this powerdomain was not hitting retention/off?
Yes the change affects the registers displayed, but since the address calculation is incorrect with CONFIG_PM_DEBUG enabled, the PM_DEBUG code is reading the CM_IVA2/PRM_IVA2 registers at incorrect addresses. On my DM37x board when I suspended with OTG enabled (and without the patch), I see the following on resume:

Quoted 4 lines

[  496.168151] Powerdomain (iva2_pwrdm) didn't enter target state 1
(last power state 3)
[  496.176025] Powerdomain (core_pwrdm) didn't enter target state 1

(last power state 3)

Hmm, this 'last power state' printk isn't in mainline, so it appears there are some other local modifications to this code.

Quoted 10 lines

[  496.183898] Could not enter target state in pm_suspend
With the patch (and same kernel config) I see the following on resume:
[  234.867889] Successfully put all powerdomains to target state
Before enabling the OTG code all powerdomains went into suspend.
It could be that accessing registers at the incorrect addresses is causing DM37x to keep those domains out of retention...

Hmm, very strange. The PM debug code only ever reads registers, never writes, so I can't imagine how that should prevent those power domains from hitting retention. Are there any other out-of-tree patches to the pm-debug code in your kernel?

In any case, you've found a real bug, and your patch is valid. However, it doesn't currently apply to mainline, and it looks like it's because your patch is using spaces and the code being patched is using tabs.

Also, you can simplify the changelog to to just describe the fix needed due to using negative offsets in some PRM modules.

Kevin

Quoted 23 lines

Quoted 23 lines
Kevin
Quoted 8 lines
Signed-off-by Peter Barada<[email protected]>
arch/arm/mach-omap2/pm-debug.c +0 -0
index 125f565..b731ef3 100644
arch/arm/mach-omap2/pm-debug.c enum { · line 205
Quoted 8 lines
arch/arm/mach-omap2/pm-debug.c +1 -1
@@ -199,7 +199,7 @@ enum {
  struct pm_module_def {
      char name[8]; /* Name of the module */
      short type; /* CM or PRM */
-    unsigned short offset;
+    short offset;
      int low; /* First register address on this module */
      int high; /* Last register address on this module */
  };
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Peter Barada

author

2011-05-05 19:30 UTC #4 lore raw

Quoted 35 lines · Kevin Hilman

Quoted 33 lines · Peter Barada
Quoted 21 lines · Kevin Hilman
Quoted 13 lines · Peter Barada
I've been working on getting the TI OMAPPSP-03.00.01.06 kernel to properly suspend/resume on my DM37x board and all was going well until I added OTG support to the kernel and on suspend, the IVA2 and CORE pwrdms would not properly go into suspend. When comparing output from /debug/pm-debug/registers/current to the TRM, I noticed the following:
MOD: CM_IVA2 (48014000)
    04 =>   00000017  20 =>   00000001  24 =>   00000001  34 =>   00000001
    40 =>   00080a00  44 =>   00000001  48 =>   00000003
MOD: PRM_IVA2 (48316000)
    50 =>   00000007  e0 =>   00ff0f05  e4 =>   00000ff7  e8 =>   00000ff7
    f8 =>   00000001
Quoted 6 lines
Looking at the TRM, the PRM_IVA2 registers are at 0x48306000, not 0x48316000. OMAP3430_IVA2_MOD is defined in prcm-common.h as -0x800 which means any module + reg_offset address calculation has to be signed. Once I corrected the "unsigned short offset" declaration in pm_module_def, rebuilt and tested again, IVA2/core pwrdms go into suspend correctly (and addresses look correct):
Quoted 5 lines
Just to be clear, this should only affect the display of the registers, not whether or not the power domains actually suspend correctly, right?
Or, did you actually notice via some other method that this powerdomain was not hitting retention/off?
Yes the change affects the registers displayed, but since the address calculation is incorrect with CONFIG_PM_DEBUG enabled, the PM_DEBUG code is reading the CM_IVA2/PRM_IVA2 registers at incorrect addresses. On my DM37x board when I suspended with OTG enabled (and without the patch), I see the following on resume:

Quoted 6 lines

Quoted 4 lines
[  496.168151] Powerdomain (iva2_pwrdm) didn't enter target state 1
(last power state 3)
[  496.176025] Powerdomain (core_pwrdm) didn't enter target state 1
(last power state 3)
Hmm, this 'last power state' printk isn't in mainline, so it appears there are some other local modifications to this code.

Yes, I added it to the printk to help figure out what state the domain was in:

    list_for_each_entry(pwrst, &pwrst_list, node) {
        state = pwrdm_read_prev_pwrst(pwrst->pwrdm);
        if (state > pwrst->next_state) {
            printk(KERN_INFO "Powerdomain (%s) didn't enter "
                   "target state %d (last power state %d)\n",
                pwrst->pwrdm->name, pwrst->next_state, state);
            ret = -1;
        }
        set_pwrdm_state(pwrst->pwrdm, pwrst->saved_state);
    }

Quoted 6 lines

Quoted 6 lines
[  496.183898] Could not enter target state in pm_suspend
With the patch (and same kernel config) I see the following on resume:
[  234.867889] Successfully put all powerdomains to target state

Quoted 8 lines

Quoted 4 lines
Before enabling the OTG code all powerdomains went into suspend.
It could be that accessing registers at the incorrect addresses is causing DM37x to keep those domains out of retention...
Hmm, very strange. The PM debug code only ever reads registers, never writes, so I can't imagine how that should prevent those power domains from hitting retention. Are there any other out-of-tree patches to the pm-debug code in your kernel?

Yes there are, but I've kept it to just reading PRM/CM registers (and store them into SRAM to print out on resume). Still, I don't understand how just changing the address where to read the IVA2 registers can make it break/work, unless some side-effect of hte read make's life happy. I'll have to retry w/o PM_DEBUG enabled to see if the effect changes.

Quoted 3 lines

In any case, you've found a real bug, and your patch is valid. However, it doesn't currently apply to mainline, and it looks like it's because your patch is using spaces and the code being patched is using tabs.

Hmm, could be Thunderbird munching it...

Quoted 2 lines

Also, you can simplify the changelog to to just describe the fix needed due to using negative offsets in some PRM modules.

I'll rework the patch and send it back. I'm also comparing the list of registers you read with what the TRM lists (for the AM3730), and I see that you read entire ranges even if the range is sparsely populated, eg:

    { "IVA2", MOD_CM, OMAP3430_IVA2_MOD, 0x00, 0x04 },
    { "IVA2", MOD_CM, OMAP3430_IVA2_MOD, 0x20, 0x24 },
    { "IVA2", MOD_CM, OMAP3430_IVA2_MOD, 0x34, 0x34 },
    { "IVA2", MOD_CM, OMAP3430_IVA2_MOD, 0x40, 0x4c },

and code to detect that its still looking at a pm_module_def entry that has the same type/offset as the previous to prevent the headers from being repeatedly printed.

Quoted 24 lines

Kevin
Quoted 22 lines
Quoted 22 lines
Kevin
Quoted 8 lines
Signed-off-by Peter Barada<[email protected]>
arch/arm/mach-omap2/pm-debug.c +0 -0
index 125f565..b731ef3 100644
arch/arm/mach-omap2/pm-debug.c enum { · line 205
Quoted 8 lines
arch/arm/mach-omap2/pm-debug.c +1 -1
@@ -199,7 +199,7 @@ enum {
   struct pm_module_def {
       char name[8]; /* Name of the module */
       short type; /* CM or PRM */
-    unsigned short offset;
+    short offset;
       int low; /* First register address on this module */
       int high; /* Last register address on this module */
   };
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines

Peter Barada
[email protected]

Kevin Hilman

2011-05-05 20:11 UTC #5 lore raw

Quoted 70 lines · Peter Barada

Quoted 35 lines · Kevin Hilman
Quoted 33 lines · Peter Barada
Quoted 21 lines · Kevin Hilman
Quoted 13 lines · Peter Barada
I've been working on getting the TI OMAPPSP-03.00.01.06 kernel to properly suspend/resume on my DM37x board and all was going well until I added OTG support to the kernel and on suspend, the IVA2 and CORE pwrdms would not properly go into suspend. When comparing output from /debug/pm-debug/registers/current to the TRM, I noticed the following:
MOD: CM_IVA2 (48014000)
    04 =>   00000017  20 =>   00000001  24 =>   00000001  34 =>   00000001
    40 =>   00080a00  44 =>   00000001  48 =>   00000003
MOD: PRM_IVA2 (48316000)
    50 =>   00000007  e0 =>   00ff0f05  e4 =>   00000ff7  e8 =>   00000ff7
    f8 =>   00000001
Quoted 6 lines
Looking at the TRM, the PRM_IVA2 registers are at 0x48306000, not 0x48316000. OMAP3430_IVA2_MOD is defined in prcm-common.h as -0x800 which means any module + reg_offset address calculation has to be signed. Once I corrected the "unsigned short offset" declaration in pm_module_def, rebuilt and tested again, IVA2/core pwrdms go into suspend correctly (and addresses look correct):
Quoted 5 lines
Just to be clear, this should only affect the display of the registers, not whether or not the power domains actually suspend correctly, right?
Or, did you actually notice via some other method that this powerdomain was not hitting retention/off?
Yes the change affects the registers displayed, but since the address calculation is incorrect with CONFIG_PM_DEBUG enabled, the PM_DEBUG code is reading the CM_IVA2/PRM_IVA2 registers at incorrect addresses. On my DM37x board when I suspended with OTG enabled (and without the patch), I see the following on resume:
Quoted 6 lines
Quoted 4 lines
[  496.168151] Powerdomain (iva2_pwrdm) didn't enter target state 1
(last power state 3)
[  496.176025] Powerdomain (core_pwrdm) didn't enter target state 1
(last power state 3)
Hmm, this 'last power state' printk isn't in mainline, so it appears there are some other local modifications to this code.
Yes, I added it to the printk to help figure out what state the domain was in:
    list_for_each_entry(pwrst, &pwrst_list, node) {
        state = pwrdm_read_prev_pwrst(pwrst->pwrdm);
        if (state > pwrst->next_state) {
            printk(KERN_INFO "Powerdomain (%s) didn't enter "
                   "target state %d (last power state %d)\n",
                pwrst->pwrdm->name, pwrst->next_state, state);
            ret = -1;
        }
        set_pwrdm_state(pwrst->pwrdm, pwrst->saved_state);
    }
Quoted 6 lines
Quoted 6 lines
[  496.183898] Could not enter target state in pm_suspend
With the patch (and same kernel config) I see the following on resume:
[  234.867889] Successfully put all powerdomains to target state
Quoted 8 lines
Quoted 4 lines
Before enabling the OTG code all powerdomains went into suspend.
It could be that accessing registers at the incorrect addresses is causing DM37x to keep those domains out of retention...
Hmm, very strange. The PM debug code only ever reads registers, never writes, so I can't imagine how that should prevent those power domains from hitting retention. Are there any other out-of-tree patches to the pm-debug code in your kernel?

Quoted 6 lines

Yes there are, but I've kept it to just reading PRM/CM registers (and store them into SRAM to print out on resume). Still, I don't understand how just changing the address where to read the IVA2 registers can make it break/work, unless some side-effect of hte read make's life happy. I'll have to retry w/o PM_DEBUG enabled to see if the effect changes.

Rather than disable PM_DEBUG...

Does your kernel do any pm_dbg_regset_save() calls in the suspend/idle path? If so, it would suffice to comment those out and see if that makes it work again (without your patch.) Would be interesting.

Quoted 11 lines

Quoted 3 lines
In any case, you've found a real bug, and your patch is valid. However, it doesn't currently apply to mainline, and it looks like it's because your patch is using spaces and the code being patched is using tabs.
Hmm, could be Thunderbird munching it...
Quoted 2 lines
Also, you can simplify the changelog to to just describe the fix needed due to using negative offsets in some PRM modules.
I'll rework the patch and send it back. I'm also comparing the list of registers you read with what the TRM lists (for the AM3730), and I see that you read entire ranges even if the range is sparsely populated, eg:

Quoted 8 lines

    { "IVA2", MOD_CM, OMAP3430_IVA2_MOD, 0x00, 0x04 },
    { "IVA2", MOD_CM, OMAP3430_IVA2_MOD, 0x20, 0x24 },
    { "IVA2", MOD_CM, OMAP3430_IVA2_MOD, 0x34, 0x34 },
    { "IVA2", MOD_CM, OMAP3430_IVA2_MOD, 0x40, 0x4c },
and code to detect that its still looking at a pm_module_def entry that has the same type/offset as the previous to prevent the headers from being repeatedly printed.

Yes, admittedly this code is really awful and doesn't even belong in the kernel. It also doesn't scale well for OMAP4. Don't be surprised if this register snapshot/display code is dropped all together in the future.

Kevin

Peter Barada

author

2011-05-06 15:25 UTC #6 lore raw

In reply to Kevin Hilman

Quoted 7 lines · Kevin Hilman

In any case, you've found a real bug, and your patch is valid. However, it doesn't currently apply to mainline, and it looks like it's because your patch is using spaces and the code being patched is using tabs.
Also, you can simplify the changelog to to just describe the fix needed due to using negative offsets in some PRM modules.

I've attached a new patch to fix the tabs/changelog.

Signature · 2 lines

Peter Barada
[email protected]

Quoted 1 line

From 6857bb52af1fcd90e64d3e6c45de3ff0f071be7a Mon Sep 17 00:00:00 2001

From: Peter Barada <[email protected]>
Date: Fri, 6 May 2011 11:07:50 -0400
Subject: [PATCH] Make offset in pm_module_def signed to allow negative offsets.

Since PRM module offsets can be negative (IVA2 is -0x800), offset in pm_module_def struct has to be signed.

---
 arch/arm/mach-omap2/pm-debug.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

arch/arm/mach-omap2/pm-debug.c +1 -1

index b731ef3..125f565 100644
@@ -199,7 +199,7 @@ enum {
 struct pm_module_def {
 	char name[8]; /* Name of the module */
 	short type; /* CM or PRM */
-	short offset;
+	unsigned short offset;
 	int low; /* First register address on this module */
 	int high; /* Last register address on this module */
 };

Signature · 1 line

1.7.1

Kevin Hilman

2011-05-06 19:55 UTC #7 lore raw

Hi Peter,

Quoted 9 lines · Peter Barada

Quoted 7 lines · Kevin Hilman
In any case, you've found a real bug, and your patch is valid. However, it doesn't currently apply to mainline, and it looks like it's because your patch is using spaces and the code being patched is using tabs.
Also, you can simplify the changelog to to just describe the fix needed due to using negative offsets in some PRM modules.
I've attached a new patch to fix the tabs/changelog.

Thanks for the updated patch.

[...]

Quoted 4 lines

From 6857bb52af1fcd90e64d3e6c45de3ff0f071be7a Mon Sep 17 00:00:00 2001
From: Peter Barada <[email protected]>
Date: Fri, 6 May 2011 11:07:50 -0400
Subject: [PATCH] Make offset in pm_module_def signed to allow negative offsets.

Please post in separate mail, and give subject a prefix like:

OMAP: PM debug: use signed offset in pm_module_def

Also, please Cc the linux-arm-kernel mailing list:
[email protected] for patches intended for mainline.

Quoted 2 lines

Since PRM module offsets can be negative (IVA2 is -0x800), offset in pm_module_def struct has to be signed.

OK, but...

Quoted 9 lines

---
 arch/arm/mach-omap2/pm-debug.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

arch/arm/mach-omap2/pm-debug.c +0 -0

index b731ef3..125f565 100644

arch/arm/mach-omap2/pm-debug.c enum { · line 202

Quoted 5 lines

arch/arm/mach-omap2/pm-debug.c +1 -1

@@ -199,7 +199,7 @@ enum {
 struct pm_module_def {
 	char name[8]; /* Name of the module */
 	short type; /* CM or PRM */
-	short offset;
+	unsigned short offset;

...this changes it from signed to unsigned.

arch/arm/mach-omap2/pm-debug.c enum { · line 205

Quoted 3 lines

arch/arm/mach-omap2/pm-debug.c +0 -0

 	int low; /* First register address on this module */
 	int high; /* Last register address on this module */
 };

Kevin