[PATCH] ARM: pxa: fix pxa3xx DFI lockup hack

[PATCH] ARM: pxa: fix pxa3xx DFI lockup hack

[Arnd Bergmann]

Some recently added code to avoid a bug introduced a build error
when CONFIG_PM is disabled and a macro is hidden:

arch/arm/mach-pxa/pxa3xx.c: In function 'pxa3xx_init':
arch/arm/mach-pxa/pxa3xx.c:439:3: error: 'NDCR' undeclared (first use in this function)
   NDCR = (NDCR & ~NDCR_ND_ARB_EN) | NDCR_ND_ARB_CNTL;
   ^

This moves the macro outside of the #ifdef so it can be
referenced correctly.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: adf3442cc890 ("ARM: pxa: fix DFI bus lockups on startup")
---
We merged the patch that introduced this as a fix for 4.3, so we should
probably add this one too.

diff --git a/arch/arm/mach-pxa/pxa3xx.c b/arch/arm/mach-pxa/pxa3xx.c
index 06005d3f2ba3..20ce2d386f17 100644
--- a/arch/arm/mach-pxa/pxa3xx.c
+++ b/arch/arm/mach-pxa/pxa3xx.c
@@ -42,10 +42,6 @@
 #define PECR_IS(n)	((1 << ((n) * 2)) << 29)
 
 extern void __init pxa_dt_irq_init(int (*fn)(struct irq_data *, unsigned int));
-#ifdef CONFIG_PM
-
-#define ISRAM_START	0x5c000000
-#define ISRAM_SIZE	SZ_256K
 
 /*
  * NAND NFC: DFI bus arbitration subset
@@ -54,6 +50,11 @@ extern void __init pxa_dt_irq_init(int (*fn)(struct irq_data *, unsigned int));
 #define NDCR_ND_ARB_EN		(1 << 12)
 #define NDCR_ND_ARB_CNTL	(1 << 19)
 
+#ifdef CONFIG_PM
+
+#define ISRAM_START	0x5c000000
+#define ISRAM_SIZE	SZ_256K
+
 static void __iomem *sram;
 static unsigned long wakeup_src;
 

[PATCH] pxa: remove incorrect __init annotation on pxa27x_set_pwrmode

[Arnd Bergmann]

The z2 machine calls pxa27x_set_pwrmode() in order to power off
the machine, but this function gets discarded early at boot because
it is marked __init, as pointed out by kbuild:

WARNING: vmlinux.o(.text+0x145c4): Section mismatch in reference from the function z2_power_off() to the function .init.text:pxa27x_set_pwrmode()
The function z2_power_off() references
the function __init pxa27x_set_pwrmode().
This is often because z2_power_off lacks a __init
annotation or the annotation of pxa27x_set_pwrmode is wrong.

This removes the __init section modifier to fix rebooting and the
build error.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: ba4a90a6d86a ("ARM: pxa/z2: fix building error of pxa27x_cpu_suspend() no longer available")
---
This is a fix for an old bug, I'd just put it into 4.4

It showed up now because section mismatches now produce errors instead
of warnings.

diff --git a/arch/arm/mach-pxa/pxa27x.c b/arch/arm/mach-pxa/pxa27x.c
index 885d83cfa522..3d5335f2bbe0 100644
--- a/arch/arm/mach-pxa/pxa27x.c
+++ b/arch/arm/mach-pxa/pxa27x.c
@@ -84,7 +84,7 @@ EXPORT_SYMBOL_GPL(pxa27x_configure_ac97reset);
  */
 static unsigned int pwrmode = PWRMODE_SLEEP;
 
-int __init pxa27x_set_pwrmode(unsigned int mode)
+int pxa27x_set_pwrmode(unsigned int mode)
 {
 	switch (mode) {
 	case PWRMODE_SLEEP:
diff --git a/arch/arm/mach-pxa/pxa27x.h b/arch/arm/mach-pxa/pxa27x.h
index d064e0a9f2ce..075131d83eab 100644
--- a/arch/arm/mach-pxa/pxa27x.h
+++ b/arch/arm/mach-pxa/pxa27x.h
@@ -19,7 +19,7 @@
 #define ARB_CORE_PARK		(1<<24)	   /* Be parked with core when idle */
 #define ARB_LOCK_FLAG		(1<<23)	   /* Only Locking masters gain access to the bus */
 
-extern int __init pxa27x_set_pwrmode(unsigned int mode);
+extern int pxa27x_set_pwrmode(unsigned int mode);
 extern void pxa27x_cpu_pm_enter(suspend_state_t state);
 
 #endif /* __MACH_PXA27x_H */

Re: [PATCH] ARM: pxa: fix pxa3xx DFI lockup hack

[Robert Jarzmik]

Arnd Bergmann <arnd@arndb.de> writes:

> Some recently added code to avoid a bug introduced a build error
> when CONFIG_PM is disabled and a macro is hidden:
>
> arch/arm/mach-pxa/pxa3xx.c: In function 'pxa3xx_init':
> arch/arm/mach-pxa/pxa3xx.c:439:3: error: 'NDCR' undeclared (first use in this function)
>    NDCR = (NDCR & ~NDCR_ND_ARB_EN) | NDCR_ND_ARB_CNTL;
>    ^
>
> This moves the macro outside of the #ifdef so it can be
> referenced correctly.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: adf3442cc890 ("ARM: pxa: fix DFI bus lockups on startup")
> ---
> We merged the patch that introduced this as a fix for 4.3, so we should
> probably add this one too.
Oh yes, didn't see that ifdef, and all my non-regression defconfigs have
CONFIG_PM ...

Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>

So do you want to take it directly (my preferred solution) or do you want a
proper pull request for in the -rc5 timeframe ?

Cheers.

-- 
Robert

Re: [PATCH] pxa: remove incorrect __init annotation on pxa27x_set_pwrmode

[Robert Jarzmik]

Arnd Bergmann <arnd@arndb.de> writes:

> The z2 machine calls pxa27x_set_pwrmode() in order to power off
> the machine, but this function gets discarded early at boot because
> it is marked __init, as pointed out by kbuild:
>
> WARNING: vmlinux.o(.text+0x145c4): Section mismatch in reference from the function z2_power_off() to the function .init.text:pxa27x_set_pwrmode()
> The function z2_power_off() references
> the function __init pxa27x_set_pwrmode().
> This is often because z2_power_off lacks a __init
> annotation or the annotation of pxa27x_set_pwrmode is wrong.
>
> This removes the __init section modifier to fix rebooting and the
> build error.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: ba4a90a6d86a ("ARM: pxa/z2: fix building error of pxa27x_cpu_suspend() no longer available")
> ---
> This is a fix for an old bug, I'd just put it into 4.4
>
> It showed up now because section mismatches now produce errors instead
> of warnings.

Hi Arnd,

I already have this queued for 4.4 from Thierry's patch in :
  http://www.spinics.net/lists/arm-kernel/msg449414.html

Yet your patch is more complete, as it deals also with the header in pxa27x.h
and has the fixes tag.

Now I'm wondering if I should enhance Thierry's patch with yours and keeping
your signoff with your permission, or drop Thierry's one to replace by yours (I
don't know if it's sane behavior to drop an already queued patch ...)

Cheers.

--
Robert

Re: [PATCH] pxa: remove incorrect __init annotation on pxa27x_set_pwrmode

[Robert Jarzmik]

Robert Jarzmik <robert.jarzmik@free.fr> writes:

> Arnd Bergmann <arnd@arndb.de> writes:
>
>> The z2 machine calls pxa27x_set_pwrmode() in order to power off
>> the machine, but this function gets discarded early at boot because
>> it is marked __init, as pointed out by kbuild:
>>
>> WARNING: vmlinux.o(.text+0x145c4): Section mismatch in reference from the function z2_power_off() to the function .init.text:pxa27x_set_pwrmode()
>> The function z2_power_off() references
>> the function __init pxa27x_set_pwrmode().
>> This is often because z2_power_off lacks a __init
>> annotation or the annotation of pxa27x_set_pwrmode is wrong.
>>
>> This removes the __init section modifier to fix rebooting and the
>> build error.
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> Fixes: ba4a90a6d86a ("ARM: pxa/z2: fix building error of pxa27x_cpu_suspend() no longer available")
>> ---
>> This is a fix for an old bug, I'd just put it into 4.4
>>
>> It showed up now because section mismatches now produce errors instead
>> of warnings.
>
> Hi Arnd,
>
> I already have this queued for 4.4 from Thierry's patch in :
>   http://www.spinics.net/lists/arm-kernel/msg449414.html
>
> Yet your patch is more complete, as it deals also with the header in pxa27x.h
> and has the fixes tag.
>
> Now I'm wondering if I should enhance Thierry's patch with yours and keeping
> your signoff with your permission, or drop Thierry's one to replace by yours (I
> don't know if it's sane behavior to drop an already queued patch ...)

Actually, I've rethought it over and Thierry's patch approach looks somehow more
appealing to me. What Thierry did is that he modified the _use_ of
pxa27x_set_pwrmode() to be called from a __init annonated function. While your
patch makes pxa27x_set_pwrmode() non __init.

It looks to me the powermode should be initialized once and for all in the
machine init code. So unless I've overseen something, I'll keep Thierry's patch.

Cheers.

--
Robert

Re: [PATCH] pxa: remove incorrect __init annotation on pxa27x_set_pwrmode

[Arnd Bergmann]

On Monday 12 October 2015 19:10:10 Robert Jarzmik wrote:
> Arnd Bergmann <arnd@arndb.de> writes:
> 
> > The z2 machine calls pxa27x_set_pwrmode() in order to power off
> > the machine, but this function gets discarded early at boot because
> > it is marked __init, as pointed out by kbuild:
> >
> > WARNING: vmlinux.o(.text+0x145c4): Section mismatch in reference from the function z2_power_off() to the function .init.text:pxa27x_set_pwrmode()
> > The function z2_power_off() references
> > the function __init pxa27x_set_pwrmode().
> > This is often because z2_power_off lacks a __init
> > annotation or the annotation of pxa27x_set_pwrmode is wrong.
> >
> > This removes the __init section modifier to fix rebooting and the
> > build error.
> >
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > Fixes: ba4a90a6d86a ("ARM: pxa/z2: fix building error of pxa27x_cpu_suspend() no longer available")
> > ---
> > This is a fix for an old bug, I'd just put it into 4.4
> >
> > It showed up now because section mismatches now produce errors instead
> > of warnings.
> 
> Hi Arnd,
> 
> I already have this queued for 4.4 from Thierry's patch in :
>   http://www.spinics.net/lists/arm-kernel/msg449414.html
> 
> Yet your patch is more complete, as it deals also with the header in pxa27x.h
> and has the fixes tag.
> 
> Now I'm wondering if I should enhance Thierry's patch with yours and keeping
> your signoff with your permission, or drop Thierry's one to replace by yours (I
> don't know if it's sane behavior to drop an already queued patch ...)

Thierry's patch has no effect unless both the declaration and the use are changed.
Feel free to fix it any way you see fit by rebasing/replacing or adding
the rest of my patch on top.

As long as nobody has picked up your branch into another tree, you can
rebase it, otherwise it has to be done on top. Some people like to never
rebase, others do it all the time and if I see something very wrong in
a branch that I get as a pull request, I will ask for it to be rebased.

	Arnd

Re: [PATCH] ARM: pxa: fix pxa3xx DFI lockup hack

[Arnd Bergmann]

On Monday 12 October 2015 19:03:44 Robert Jarzmik wrote:
> Arnd Bergmann <arnd@arndb.de> writes:
> 
> > Some recently added code to avoid a bug introduced a build error
> > when CONFIG_PM is disabled and a macro is hidden:
> >
> > arch/arm/mach-pxa/pxa3xx.c: In function 'pxa3xx_init':
> > arch/arm/mach-pxa/pxa3xx.c:439:3: error: 'NDCR' undeclared (first use in this function)
> >    NDCR = (NDCR & ~NDCR_ND_ARB_EN) | NDCR_ND_ARB_CNTL;
> >    ^
> >
> > This moves the macro outside of the #ifdef so it can be
> > referenced correctly.
> >
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > Fixes: adf3442cc890 ("ARM: pxa: fix DFI bus lockups on startup")
> > ---
> > We merged the patch that introduced this as a fix for 4.3, so we should
> > probably add this one too.
> Oh yes, didn't see that ifdef, and all my non-regression defconfigs have
> CONFIG_PM ...
> 
> Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>
> 
> So do you want to take it directly (my preferred solution) or do you want a
> proper pull request for in the -rc5 timeframe ?
> 

I've put it into my todo list, will apply it tomorrow.

	Arnd

Re: [PATCH] pxa: remove incorrect __init annotation on pxa27x_set_pwrmode

[Arnd Bergmann]

On Monday 12 October 2015 20:53:50 Robert Jarzmik wrote:
> Robert Jarzmik <robert.jarzmik@free.fr> writes:
> 
> > Arnd Bergmann <arnd@arndb.de> writes:
> >
> >> The z2 machine calls pxa27x_set_pwrmode() in order to power off
> >> the machine, but this function gets discarded early at boot because
> >> it is marked __init, as pointed out by kbuild:
> >>
> >> WARNING: vmlinux.o(.text+0x145c4): Section mismatch in reference from the function z2_power_off() to the function .init.text:pxa27x_set_pwrmode()
> >> The function z2_power_off() references
> >> the function __init pxa27x_set_pwrmode().
> >> This is often because z2_power_off lacks a __init
> >> annotation or the annotation of pxa27x_set_pwrmode is wrong.
> >>
> >> This removes the __init section modifier to fix rebooting and the
> >> build error.
> >>
> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >> Fixes: ba4a90a6d86a ("ARM: pxa/z2: fix building error of pxa27x_cpu_suspend() no longer available")
> >> ---
> >> This is a fix for an old bug, I'd just put it into 4.4
> >>
> >> It showed up now because section mismatches now produce errors instead
> >> of warnings.
> >
> > Hi Arnd,
> >
> > I already have this queued for 4.4 from Thierry's patch in :
> >   http://www.spinics.net/lists/arm-kernel/msg449414.html
> >
> > Yet your patch is more complete, as it deals also with the header in pxa27x.h
> > and has the fixes tag.
> >
> > Now I'm wondering if I should enhance Thierry's patch with yours and keeping
> > your signoff with your permission, or drop Thierry's one to replace by yours (I
> > don't know if it's sane behavior to drop an already queued patch ...)
> 
> Actually, I've rethought it over and Thierry's patch approach looks somehow more
> appealing to me. What Thierry did is that he modified the _use_ of
> pxa27x_set_pwrmode() to be called from a __init annonated function. While your
> patch makes pxa27x_set_pwrmode() non __init.
> 
> It looks to me the powermode should be initialized once and for all in the
> machine init code. So unless I've overseen something, I'll keep Thierry's patch.

Ah, sorry. I should have looked at the link you sent.

Thierry's patch indeed looks much nicer if that works, but I'm not entirely
sure if it is safe or not. Only two other pxa27x machines set the PWRMODE_DEEPSLEEP
flag, so it looks like there is a reason for some machines not to set it.
The z2 in particular never did, except in the powerdown handler. So if it
is unable to wake up reliably from DEEPSLEEP, that may be a reason to use
that flag only in the poweroff function but not during normal operation.

	Arnd

Re: [PATCH] pxa: remove incorrect __init annotation on pxa27x_set_pwrmode

[Robert Jarzmik]

Arnd Bergmann <arnd@arndb.de> writes:

>> It looks to me the powermode should be initialized once and for all in the
>> machine init code. So unless I've overseen something, I'll keep Thierry's patch.
>
> Ah, sorry. I should have looked at the link you sent.
>
> Thierry's patch indeed looks much nicer if that works, but I'm not entirely
> sure if it is safe or not. Only two other pxa27x machines set the PWRMODE_DEEPSLEEP
> flag, so it looks like there is a reason for some machines not to set it.
Yes, the reason is that the default is set PWRMODE_SLEEP as set in pxa27x.c
This is the only choice for suspend to RAM, as it :
 - disables a lot of power islands
 - and yet keeps Vcc_Mem powered for suspend to RAM

> The z2 in particular never did, except in the powerdown handler. So if it
> is unable to wake up reliably from DEEPSLEEP, that may be a reason to use
> that flag only in the poweroff function but not during normal operation.
My first thought was :
	This flag's only use is for Suspend to RAM.  There might be special
	wirings where the voltage to the RAM is provided out of SYS_EN pin
	control, and therefore PWRMODE_DEEPSLEEP is possible. But the most
	common pattern would be a PMIC, providing voltage to the DRAM through
	Vcc_Mem.

But having a closer look, you're right, I have misunderstood the way z2 poweroff
was designed. The setting of poweroff to PWRMODE_DEEPSLEEP +
pxa27x_cpu_pm_enter() is a whole, it is the closest to a platform halt where no
external chip handles the poweroff phase, and memory is shutdown in deep sleep.

Therefore I'll drop Thierry's patch (which would actually break suspend to RAM I
think) and take yours.

Cheers.

-- 
Robert

Re: [PATCH] pxa: remove incorrect __init annotation on pxa27x_set_pwrmode

[Arnd Bergmann]

On Monday 12 October 2015 23:07:28 Robert Jarzmik wrote:
> Arnd Bergmann <arnd@arndb.de> writes:

> > The z2 in particular never did, except in the powerdown handler. So if it
> > is unable to wake up reliably from DEEPSLEEP, that may be a reason to use
> > that flag only in the poweroff function but not during normal operation.
> My first thought was :
>         This flag's only use is for Suspend to RAM.  There might be special
>         wirings where the voltage to the RAM is provided out of SYS_EN pin
>         control, and therefore PWRMODE_DEEPSLEEP is possible. But the most
>         common pattern would be a PMIC, providing voltage to the DRAM through
>         Vcc_Mem.
> 
> But having a closer look, you're right, I have misunderstood the way z2 poweroff
> was designed. The setting of poweroff to PWRMODE_DEEPSLEEP +
> pxa27x_cpu_pm_enter() is a whole, it is the closest to a platform halt where no
> external chip handles the poweroff phase, and memory is shutdown in deep sleep.
> 
> Therefore I'll drop Thierry's patch (which would actually break suspend to RAM I
> think) and take yours.

Ok, sounds good. thanks!

	Arnd

Re: [PATCH] ARM: pxa: fix pxa3xx DFI lockup hack

[Arnd Bergmann]

On Monday 12 October 2015 19:03:44 Robert Jarzmik wrote:
> Arnd Bergmann <arnd@arndb.de> writes:
> 
> > Some recently added code to avoid a bug introduced a build error
> > when CONFIG_PM is disabled and a macro is hidden:
> >
> > arch/arm/mach-pxa/pxa3xx.c: In function 'pxa3xx_init':
> > arch/arm/mach-pxa/pxa3xx.c:439:3: error: 'NDCR' undeclared (first use in this function)
> >    NDCR = (NDCR & ~NDCR_ND_ARB_EN) | NDCR_ND_ARB_CNTL;
> >    ^
> >
> > This moves the macro outside of the #ifdef so it can be
> > referenced correctly.
> >
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > Fixes: adf3442cc890 ("ARM: pxa: fix DFI bus lockups on startup")
> > ---
> > We merged the patch that introduced this as a fix for 4.3, so we should
> > probably add this one too.
> Oh yes, didn't see that ifdef, and all my non-regression defconfigs have
> CONFIG_PM ...
> 
> Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>
> 
> So do you want to take it directly (my preferred solution) or do you want a
> proper pull request for in the -rc5 timeframe ?

Applied now.

	Arnd

Re: [PATCH] ARM: pxa: fix pxa3xx DFI lockup hack

[Robert Jarzmik]

Arnd Bergmann <arnd@arndb.de> writes:

> On Monday 12 October 2015 19:03:44 Robert Jarzmik wrote:
>> Arnd Bergmann <arnd@arndb.de> writes:
>> 
>> > Some recently added code to avoid a bug introduced a build error
>> > when CONFIG_PM is disabled and a macro is hidden:
>> >
>> > arch/arm/mach-pxa/pxa3xx.c: In function 'pxa3xx_init':
>> > arch/arm/mach-pxa/pxa3xx.c:439:3: error: 'NDCR' undeclared (first use in this function)
>> >    NDCR = (NDCR & ~NDCR_ND_ARB_EN) | NDCR_ND_ARB_CNTL;
>> >    ^
>> >
>> > This moves the macro outside of the #ifdef so it can be
>> > referenced correctly.
>> >
>> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> > Fixes: adf3442cc890 ("ARM: pxa: fix DFI bus lockups on startup")
>> > ---
>> > We merged the patch that introduced this as a fix for 4.3, so we should
>> > probably add this one too.
>> Oh yes, didn't see that ifdef, and all my non-regression defconfigs have
>> CONFIG_PM ...
>> 
>> Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>
>> 
>> So do you want to take it directly (my preferred solution) or do you want a
>> proper pull request for in the -rc5 timeframe ?
>
> Applied now.
>
> 	Arnd
Thanks.

Cheers.

-- 
Robert