[PATCH] Revert "OMAP: mach-omap2: Fix incorrect assignment warnings"

[PATCH] Revert "OMAP: mach-omap2: Fix incorrect assignment warnings"

[Jean Pihet]

Hi,

This patch reverts commit 914bab936fe0388a529079679e2f137aa4ff548d, which
breaks the OFF mode on the OMAP3 platforms.
The details are here below.

The intent behind the original patch was to fix some compiler
warnings, which I do not have on my side. Is the problem dependent on
the setup and config used?

Jean

---
>From ec85bc90978cf0f257e73eaad593ffb774595863 Mon Sep 17 00:00:00 2001
From: Jean Pihet <jean.pihet@newoldbits.com>
Date: Fri, 8 Oct 2010 18:36:48 +0200
Subject: [PATCH] Revert "OMAP: mach-omap2: Fix incorrect assignment warnings"

This reverts commit 914bab936fe0388a529079679e2f137aa4ff548d, which
breaks the OFF mode on the OMAP3 platforms.

The use of a void* pointer for scratchpad_address confuses the
compiler which generates wrong offset for the access to the L4
address space. In that case an alignement fault is generated
during the wake-up from OFF mode.

The code that causes problem is:
__raw_readl(scratchpad_address + OMAP343X_TABLE_ADDRESS_OFFSET);

Signed-off-by: Jean Pihet <j-pihet@ti.com>
---
 arch/arm/mach-omap2/control.c |    4 ++--
 arch/arm/mach-omap2/pm34xx.c  |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-omap2/control.c b/arch/arm/mach-omap2/control.c
index 1fa3294..049bfb1 100644
--- a/arch/arm/mach-omap2/control.c
+++ b/arch/arm/mach-omap2/control.c
@@ -217,7 +217,7 @@ void omap4_ctrl_pad_writel(u32 val, u16 offset)
 void omap3_clear_scratchpad_contents(void)
 {
 	u32 max_offset = OMAP343X_SCRATCHPAD_ROM_OFFSET;
-	void __iomem *v_addr;
+	u32 *v_addr;
 	u32 offset = 0;
 	v_addr = OMAP2_L4_IO_ADDRESS(OMAP343X_SCRATCHPAD_ROM);
 	if (prm_read_mod_reg(OMAP3430_GR_MOD, OMAP3_PRM_RSTST_OFFSET) &
@@ -233,7 +233,7 @@ void omap3_clear_scratchpad_contents(void)
 /* Populate the scratchpad structure with restore structure */
 void omap3_save_scratchpad_contents(void)
 {
-	void  __iomem *scratchpad_address;
+	void * __iomem scratchpad_address;
 	u32 arm_context_addr;
 	struct omap3_scratchpad scratchpad_contents;
 	struct omap3_scratchpad_prcm_block prcm_block_contents;
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 8c8f1ac..af752ae 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -310,7 +310,7 @@ static void restore_control_register(u32 val)
 /* Function to restore the table entry that was modified for enabling MMU */
 static void restore_table_entry(void)
 {
-	void __iomem *scratchpad_address;
+	u32 *scratchpad_address;
 	u32 previous_value, control_reg_value;
 	u32 *address;

-- 
1.7.1

Re: [PATCH] Revert "OMAP: mach-omap2: Fix incorrect assignment warnings"

[Paul Walmsley]

On Fri, 8 Oct 2010, Jean Pihet wrote:

> The intent behind the original patch was to fix some compiler
> warnings, which I do not have on my side. Is the problem dependent on
> the setup and config used?

It's probably a warning from sparse --

https://sparse.wiki.kernel.org/index.php/Main_Page

Maybe it will show up if you

make C=2

?


- Paul

Re: [PATCH] Revert "OMAP: mach-omap2: Fix incorrect assignment warnings"

[Kevin Hilman]

Jean Pihet <jean.pihet@newoldbits.com> writes:

> This patch reverts commit 914bab936fe0388a529079679e2f137aa4ff548d, which
> breaks the OFF mode on the OMAP3 platforms.
> The details are here below.
>
> The intent behind the original patch was to fix some compiler
> warnings, which I do not have on my side. Is the problem dependent on
> the setup and config used?

> From ec85bc90978cf0f257e73eaad593ffb774595863 Mon Sep 17 00:00:00 2001
> From: Jean Pihet <jean.pihet@newoldbits.com>
> Date: Fri, 8 Oct 2010 18:36:48 +0200
> Subject: [PATCH] Revert "OMAP: mach-omap2: Fix incorrect assignment warnings"
>
> This reverts commit 914bab936fe0388a529079679e2f137aa4ff548d, which
> breaks the OFF mode on the OMAP3 platforms.
>
> The use of a void* pointer for scratchpad_address confuses the
> compiler which generates wrong offset for the access to the L4
> address space. In that case an alignement fault is generated
> during the wake-up from OFF mode.
>
> The code that causes problem is:
> __raw_readl(scratchpad_address + OMAP343X_TABLE_ADDRESS_OFFSET);

Thanks Jean for tracking down why off-mode was broken on the master
branch.  

I completely agree this patch should be reverted.  However, the
description here could be a litle more descriptive.

Specifically, the compiler is not confused and generating the wrong
offset.  The compiler is doing what it was told told.

The problem is that the original patch rather blindly replaced a u32
pointer with a void pointer to fix a sparse warning.  However, the code
using that pointer was doing pointer math which has different results
for a void pointer than for a u32 pointer.   

I'll reply in more detail to the original patch.

Kevin

Re: [PATCH] Revert "OMAP: mach-omap2: Fix incorrect assignment warnings"

[Kevin Hilman]

Kevin Hilman <khilman@deeprootsystems.com> writes:

> Jean Pihet <jean.pihet@newoldbits.com> writes:
>
>> This patch reverts commit 914bab936fe0388a529079679e2f137aa4ff548d, which
>> breaks the OFF mode on the OMAP3 platforms.
>> The details are here below.
>>
>> The intent behind the original patch was to fix some compiler
>> warnings, which I do not have on my side. Is the problem dependent on
>> the setup and config used?
>
>> From ec85bc90978cf0f257e73eaad593ffb774595863 Mon Sep 17 00:00:00 2001
>> From: Jean Pihet <jean.pihet@newoldbits.com>
>> Date: Fri, 8 Oct 2010 18:36:48 +0200
>> Subject: [PATCH] Revert "OMAP: mach-omap2: Fix incorrect assignment warnings"
>>
>> This reverts commit 914bab936fe0388a529079679e2f137aa4ff548d, which
>> breaks the OFF mode on the OMAP3 platforms.
>>
>> The use of a void* pointer for scratchpad_address confuses the
>> compiler which generates wrong offset for the access to the L4
>> address space. In that case an alignement fault is generated
>> during the wake-up from OFF mode.
>>
>> The code that causes problem is:
>> __raw_readl(scratchpad_address + OMAP343X_TABLE_ADDRESS_OFFSET);
>
> Thanks Jean for tracking down why off-mode was broken on the master
> branch.  
>
> I completely agree this patch should be reverted.  However, the
> description here could be a litle more descriptive.

I changed my mind...

Since only one part of the original patch introduced a bug, I decided to
just fix that bug and keep the fixes for the sparse warnings.  I just
posted a patch[1] for the fix.   Please test.

Thanks,

Kevin

[1] https://patchwork.kernel.org/patch/242661/

Re: [PATCH] Revert "OMAP: mach-omap2: Fix incorrect assignment warnings"

[Jean Pihet]

Kevin,

On Sat, Oct 9, 2010 at 12:47 AM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> Kevin Hilman <khilman@deeprootsystems.com> writes:
>
>> Jean Pihet <jean.pihet@newoldbits.com> writes:
>>
>>> This patch reverts commit 914bab936fe0388a529079679e2f137aa4ff548d, which
>>> breaks the OFF mode on the OMAP3 platforms.
>>> The details are here below.
>>>
>>> The intent behind the original patch was to fix some compiler
>>> warnings, which I do not have on my side. Is the problem dependent on
>>> the setup and config used?
>>
>>> From ec85bc90978cf0f257e73eaad593ffb774595863 Mon Sep 17 00:00:00 2001
>>> From: Jean Pihet <jean.pihet@newoldbits.com>
>>> Date: Fri, 8 Oct 2010 18:36:48 +0200
>>> Subject: [PATCH] Revert "OMAP: mach-omap2: Fix incorrect assignment warnings"
>>>
>>> This reverts commit 914bab936fe0388a529079679e2f137aa4ff548d, which
>>> breaks the OFF mode on the OMAP3 platforms.
>>>
>>> The use of a void* pointer for scratchpad_address confuses the
>>> compiler which generates wrong offset for the access to the L4
>>> address space. In that case an alignement fault is generated
>>> during the wake-up from OFF mode.
>>>
>>> The code that causes problem is:
>>> __raw_readl(scratchpad_address + OMAP343X_TABLE_ADDRESS_OFFSET);
>>
>> Thanks Jean for tracking down why off-mode was broken on the master
>> branch.
>>
>> I completely agree this patch should be reverted.  However, the
>> description here could be a litle more descriptive.
>
> I changed my mind...
>
> Since only one part of the original patch introduced a bug, I decided to
> just fix that bug and keep the fixes for the sparse warnings.  I just
> posted a patch[1] for the fix.   Please test.
>
> Thanks,

Thanks for the follow-up on the patch. I will test asap.
>
> Kevin
>
> [1] https://patchwork.kernel.org/patch/242661/
>
Jean

>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Revert "OMAP: mach-omap2: Fix incorrect assignment warnings"

[Jean Pihet]

On Sat, Oct 9, 2010 at 8:55 AM, Jean Pihet <jean.pihet@newoldbits.com> wrote:
> Kevin,
>
> On Sat, Oct 9, 2010 at 12:47 AM, Kevin Hilman
> <khilman@deeprootsystems.com> wrote:
>> Kevin Hilman <khilman@deeprootsystems.com> writes:
...
>>
>> Since only one part of the original patch introduced a bug, I decided to
>> just fix that bug and keep the fixes for the sparse warnings.  I just
>> posted a patch[1] for the fix.   Please test.
>>
>> Thanks,
>
> Thanks for the follow-up on the patch. I will test asap.
Tested OK on OMAP3EVM with RET & OFF mode in idle path.

Tested-by: Jean Pihet <jean.pihet@newoldbits.com>

Jean
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html