[PATCH 3/5] mach-u300: patch physoffset by default

[PATCH 3/5] mach-u300: patch physoffset by default

[Linus Walleij]

From: Linus Walleij <linus.walleij@linaro.org>

This works like a charm so I'll just default-select it.

Cc: Nicolas Pitre <nico@fluxnic.net>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 arch/arm/Kconfig |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 2c71a8f..14bfd8b 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -831,6 +831,7 @@ config ARCH_U300
 	select HAVE_SCHED_CLOCK
 	select HAVE_TCM
 	select ARM_AMBA
+	select ARM_PATCH_PHYS_VIRT
 	select ARM_VIC
 	select GENERIC_CLOCKEVENTS
 	select CLKDEV_LOOKUP
-- 
1.7.3.2

Enabling runtime P2V by default (Re: [PATCH 3/5] mach-u300: patch physoffset by default)

[Russell King - ARM Linux]

On Tue, Aug 09, 2011 at 09:41:38PM +0200, Linus Walleij wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> This works like a charm so I'll just default-select it.

Well, we can remove the EXPERIMENTAL status of this option now.  This
raises the question is whether we should now default it to 'y' - I
think we should.  Anyone have any objections?

Enabling runtime P2V by default (Re: [PATCH 3/5] mach-u300: patch physoffset by default)

[Will Deacon]

On Wed, Aug 10, 2011 at 10:16:35AM +0100, Russell King - ARM Linux wrote:
> On Tue, Aug 09, 2011 at 09:41:38PM +0200, Linus Walleij wrote:
> > From: Linus Walleij <linus.walleij@linaro.org>
> > 
> > This works like a charm so I'll just default-select it.
> 
> Well, we can remove the EXPERIMENTAL status of this option now.  This
> raises the question is whether we should now default it to 'y' - I
> think we should.  Anyone have any objections?

I've been running with this option enabled for the collection of ARM boards
I have and the only problem I have encountered was related to u-boot loading
at the wrong address.

So I'm all for enabling it by default, especially since it will force out
any remaining issues for boards where this hasn't been used extensively.

Will

Enabling runtime P2V by default (Re: [PATCH 3/5] mach-u300: patch physoffset by default)

[Russell King - ARM Linux]

On Wed, Aug 10, 2011 at 10:22:06AM +0100, Will Deacon wrote:
> On Wed, Aug 10, 2011 at 10:16:35AM +0100, Russell King - ARM Linux wrote:
> > On Tue, Aug 09, 2011 at 09:41:38PM +0200, Linus Walleij wrote:
> > > From: Linus Walleij <linus.walleij@linaro.org>
> > > 
> > > This works like a charm so I'll just default-select it.
> > 
> > Well, we can remove the EXPERIMENTAL status of this option now.  This
> > raises the question is whether we should now default it to 'y' - I
> > think we should.  Anyone have any objections?
> 
> I've been running with this option enabled for the collection of ARM boards
> I have and the only problem I have encountered was related to u-boot loading
> at the wrong address.
> 
> So I'm all for enabling it by default, especially since it will force out
> any remaining issues for boards where this hasn't been used extensively.

Maybe also making the option hidden depending on EXPERT, or even EMBEDDED
would be a good idea too.  I think it falls into at least the same class
as UID16, sysctl, hotplug, printk, etc. which are all EXPERT options.

Enabling runtime P2V by default (Re: [PATCH 3/5] mach-u300: patch physoffset by default)

[Linus Walleij]

On Wed, Aug 10, 2011 at 11:16 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Aug 09, 2011 at 09:41:38PM +0200, Linus Walleij wrote:
>> From: Linus Walleij <linus.walleij@linaro.org>
>>
>> This works like a charm so I'll just default-select it.
>
> Well, we can remove the EXPERIMENTAL status of this option now. ?This
> raises the question is whether we should now default it to 'y' - I
> think we should. ?Anyone have any objections?

I agree, no objections. Works flawlessly on all my systems.

Thanks,
Linus Walleij

Enabling runtime P2V by default (Re: [PATCH 3/5] mach-u300: patch physoffset by default)

[Russell King - ARM Linux]

On Wed, Aug 10, 2011 at 10:29:55AM +0100, Russell King - ARM Linux wrote:
> On Wed, Aug 10, 2011 at 10:22:06AM +0100, Will Deacon wrote:
> > On Wed, Aug 10, 2011 at 10:16:35AM +0100, Russell King - ARM Linux wrote:
> > > On Tue, Aug 09, 2011 at 09:41:38PM +0200, Linus Walleij wrote:
> > > > From: Linus Walleij <linus.walleij@linaro.org>
> > > > 
> > > > This works like a charm so I'll just default-select it.
> > > 
> > > Well, we can remove the EXPERIMENTAL status of this option now.  This
> > > raises the question is whether we should now default it to 'y' - I
> > > think we should.  Anyone have any objections?
> > 
> > I've been running with this option enabled for the collection of ARM boards
> > I have and the only problem I have encountered was related to u-boot loading
> > at the wrong address.
> > 
> > So I'm all for enabling it by default, especially since it will force out
> > any remaining issues for boards where this hasn't been used extensively.
> 
> Maybe also making the option hidden depending on EXPERT, or even EMBEDDED
> would be a good idea too.  I think it falls into at least the same class
> as UID16, sysctl, hotplug, printk, etc. which are all EXPERT options.

Right, I'm now committing a patch to hide the option unless EMBEDDED
is enabled.  I think this means we should get rid of the 'select
ARM_PATCH_PHYS_VIRT' statements from the various platforms, so that
folk can optimize away that code if they know what they're doing.

Note: this patch will conflict with the removal of the 16-bit P2V
patching, so we need to sort that out.

8<---------
From: Russell King - ARM Linux <linux@arm.linux.org.uk>
ARM: enable ARM_PATCH_PHYS_VIRT by default

Enable virtual to physical translation patching by default in all
kernels.  Hide the option behind EMBEDDED.

This can still be turned off if people desire, and they know what
they're doing, to shrink the size of the kernel to a minimum.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/Kconfig |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 5ebc5d9..6085a6c 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -195,7 +195,8 @@ config VECTORS_BASE
 	  The base address of exception vectors.
 
 config ARM_PATCH_PHYS_VIRT
-	bool "Patch physical to virtual translations at runtime"
+	bool "Patch physical to virtual translations at runtime" if EMBEDDED
+	default y
 	depends on !XIP_KERNEL && MMU
 	depends on !ARCH_REALVIEW || !SPARSEMEM
 	help
@@ -207,6 +208,10 @@ config ARM_PATCH_PHYS_VIRT
 	  of physical memory is at a 16MB boundary, or theoretically 64K
 	  for the MSM machine class.
 
+	  Only disable this option if you know what you do not require
+	  this feature (eg, building a kernel for a single machine) and
+	  you need to shrink the kernel to the minimal size.
+
 config ARM_PATCH_PHYS_VIRT_16BIT
 	def_bool y
 	depends on ARM_PATCH_PHYS_VIRT && ARCH_MSM
@@ -301,7 +306,6 @@ config ARCH_AT91
 	select ARCH_REQUIRE_GPIOLIB
 	select HAVE_CLK
 	select CLKDEV_LOOKUP
-	select ARM_PATCH_PHYS_VIRT if MMU
 	help
 	  This enables support for systems based on the Atmel AT91RM9200,
 	  AT91SAM9 and AT91CAP9 processors.
-- 
1.7.4.4

Enabling runtime P2V by default (Re: [PATCH 3/5] mach-u300: patch physoffset by default)

[Baruch Siach]

Hi Russell,

On Thu, Aug 11, 2011 at 09:24:13AM +0100, Russell King - ARM Linux wrote:
> On Wed, Aug 10, 2011 at 10:29:55AM +0100, Russell King - ARM Linux wrote:
> > On Wed, Aug 10, 2011 at 10:22:06AM +0100, Will Deacon wrote:
> > > On Wed, Aug 10, 2011 at 10:16:35AM +0100, Russell King - ARM Linux wrote:
> > > > On Tue, Aug 09, 2011 at 09:41:38PM +0200, Linus Walleij wrote:
> > > > > From: Linus Walleij <linus.walleij@linaro.org>
> > > > > 
> > > > > This works like a charm so I'll just default-select it.
> > > > 
> > > > Well, we can remove the EXPERIMENTAL status of this option now.  This
> > > > raises the question is whether we should now default it to 'y' - I
> > > > think we should.  Anyone have any objections?
> > > 
> > > I've been running with this option enabled for the collection of ARM boards
> > > I have and the only problem I have encountered was related to u-boot loading
> > > at the wrong address.
> > > 
> > > So I'm all for enabling it by default, especially since it will force out
> > > any remaining issues for boards where this hasn't been used extensively.
> > 
> > Maybe also making the option hidden depending on EXPERT, or even EMBEDDED
> > would be a good idea too.  I think it falls into at least the same class
> > as UID16, sysctl, hotplug, printk, etc. which are all EXPERT options.
> 
> Right, I'm now committing a patch to hide the option unless EMBEDDED
> is enabled.  I think this means we should get rid of the 'select
> ARM_PATCH_PHYS_VIRT' statements from the various platforms, so that
> folk can optimize away that code if they know what they're doing.
> 
> Note: this patch will conflict with the removal of the 16-bit P2V
> patching, so we need to sort that out.
> 
> 8<---------
> From: Russell King - ARM Linux <linux@arm.linux.org.uk>
> ARM: enable ARM_PATCH_PHYS_VIRT by default
> 
> Enable virtual to physical translation patching by default in all
> kernels.  Hide the option behind EMBEDDED.
> 
> This can still be turned off if people desire, and they know what
> they're doing, to shrink the size of the kernel to a minimum.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

[snip]

> +	  Only disable this option if you know what you do not require
> +	  this feature (eg, building a kernel for a single machine) and
> +	  you need to shrink the kernel to the minimal size.

The word 'what' in this statement looks redundant.

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Enabling runtime P2V by default (Re: [PATCH 3/5] mach-u300: patch physoffset by default)

[Russell King - ARM Linux]

On Thu, Aug 11, 2011 at 11:32:44AM +0300, Baruch Siach wrote:
> Hi Russell,
> 
> On Thu, Aug 11, 2011 at 09:24:13AM +0100, Russell King - ARM Linux wrote:
> > On Wed, Aug 10, 2011 at 10:29:55AM +0100, Russell King - ARM Linux wrote:
> > > On Wed, Aug 10, 2011 at 10:22:06AM +0100, Will Deacon wrote:
> > > > On Wed, Aug 10, 2011 at 10:16:35AM +0100, Russell King - ARM Linux wrote:
> > > > > On Tue, Aug 09, 2011 at 09:41:38PM +0200, Linus Walleij wrote:
> > > > > > From: Linus Walleij <linus.walleij@linaro.org>
> > > > > > 
> > > > > > This works like a charm so I'll just default-select it.
> > > > > 
> > > > > Well, we can remove the EXPERIMENTAL status of this option now.  This
> > > > > raises the question is whether we should now default it to 'y' - I
> > > > > think we should.  Anyone have any objections?
> > > > 
> > > > I've been running with this option enabled for the collection of ARM boards
> > > > I have and the only problem I have encountered was related to u-boot loading
> > > > at the wrong address.
> > > > 
> > > > So I'm all for enabling it by default, especially since it will force out
> > > > any remaining issues for boards where this hasn't been used extensively.
> > > 
> > > Maybe also making the option hidden depending on EXPERT, or even EMBEDDED
> > > would be a good idea too.  I think it falls into at least the same class
> > > as UID16, sysctl, hotplug, printk, etc. which are all EXPERT options.
> > 
> > Right, I'm now committing a patch to hide the option unless EMBEDDED
> > is enabled.  I think this means we should get rid of the 'select
> > ARM_PATCH_PHYS_VIRT' statements from the various platforms, so that
> > folk can optimize away that code if they know what they're doing.
> > 
> > Note: this patch will conflict with the removal of the 16-bit P2V
> > patching, so we need to sort that out.
> > 
> > 8<---------
> > From: Russell King - ARM Linux <linux@arm.linux.org.uk>
> > ARM: enable ARM_PATCH_PHYS_VIRT by default
> > 
> > Enable virtual to physical translation patching by default in all
> > kernels.  Hide the option behind EMBEDDED.
> > 
> > This can still be turned off if people desire, and they know what
> > they're doing, to shrink the size of the kernel to a minimum.
> > 
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> 
> [snip]
> 
> > +	  Only disable this option if you know what you do not require
> > +	  this feature (eg, building a kernel for a single machine) and
> > +	  you need to shrink the kernel to the minimal size.
> 
> The word 'what' in this statement looks redundant.

Should be 'that'.

Enabling runtime P2V by default (Re: [PATCH 3/5] mach-u300: patch physoffset by default)

[Will Deacon]

Hi Russell,

On Thu, Aug 11, 2011 at 09:24:13AM +0100, Russell King - ARM Linux wrote:
> Right, I'm now committing a patch to hide the option unless EMBEDDED
> is enabled.  I think this means we should get rid of the 'select
> ARM_PATCH_PHYS_VIRT' statements from the various platforms, so that
> folk can optimize away that code if they know what they're doing.
> 
> Note: this patch will conflict with the removal of the 16-bit P2V
> patching, so we need to sort that out.
> 
> 8<---------
> From: Russell King - ARM Linux <linux@arm.linux.org.uk>
> ARM: enable ARM_PATCH_PHYS_VIRT by default
> 
> Enable virtual to physical translation patching by default in all
> kernels.  Hide the option behind EMBEDDED.
> 
> This can still be turned off if people desire, and they know what
> they're doing, to shrink the size of the kernel to a minimum.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  arch/arm/Kconfig |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)

With the typo fix, you can have my ack if you like:

Acked-by: Will Deacon <will.deacon@arm.com>

Cheers,

Will

Enabling runtime P2V by default (Re: [PATCH 3/5] mach-u300: patch physoffset by default)

[Jean-Christophe PLAGNIOL-VILLARD]

On 09:24 Thu 11 Aug     , Russell King - ARM Linux wrote:
> On Wed, Aug 10, 2011 at 10:29:55AM +0100, Russell King - ARM Linux wrote:
> > On Wed, Aug 10, 2011 at 10:22:06AM +0100, Will Deacon wrote:
> > > On Wed, Aug 10, 2011 at 10:16:35AM +0100, Russell King - ARM Linux wrote:
> > > > On Tue, Aug 09, 2011 at 09:41:38PM +0200, Linus Walleij wrote:
> > > > > From: Linus Walleij <linus.walleij@linaro.org>
> > > > > 
> > > > > This works like a charm so I'll just default-select it.
> > > > 
> > > > Well, we can remove the EXPERIMENTAL status of this option now.  This
> > > > raises the question is whether we should now default it to 'y' - I
> > > > think we should.  Anyone have any objections?
> > > 
> > > I've been running with this option enabled for the collection of ARM boards
> > > I have and the only problem I have encountered was related to u-boot loading
> > > at the wrong address.
> > > 
> > > So I'm all for enabling it by default, especially since it will force out
> > > any remaining issues for boards where this hasn't been used extensively.
> > 
> > Maybe also making the option hidden depending on EXPERT, or even EMBEDDED
> > would be a good idea too.  I think it falls into at least the same class
> > as UID16, sysctl, hotplug, printk, etc. which are all EXPERT options.
> 
> Right, I'm now committing a patch to hide the option unless EMBEDDED
> is enabled.  I think this means we should get rid of the 'select
> ARM_PATCH_PHYS_VIRT' statements from the various platforms, so that
> folk can optimize away that code if they know what they're doing.
> 
> Note: this patch will conflict with the removal of the 16-bit P2V
> patching, so we need to sort that out.
> 
> 8<---------
> From: Russell King - ARM Linux <linux@arm.linux.org.uk>
> ARM: enable ARM_PATCH_PHYS_VIRT by default
> 
> Enable virtual to physical translation patching by default in all
> kernels.  Hide the option behind EMBEDDED.
> 
> This can still be turned off if people desire, and they know what
> they're doing, to shrink the size of the kernel to a minimum.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>

Best Regards,
J.

Enabling runtime P2V by default (Re: [PATCH 3/5] mach-u300: patch physoffset by default)

[Nicolas Pitre]

On Thu, 11 Aug 2011, Russell King - ARM Linux wrote:

> Enable virtual to physical translation patching by default in all
> kernels.  Hide the option behind EMBEDDED.
> 
> This can still be turned off if people desire, and they know what
> they're doing, to shrink the size of the kernel to a minimum.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

Acked-by: Nicolas Pitre <nicolas.pitre@linaro.org>

> ---
>  arch/arm/Kconfig |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 5ebc5d9..6085a6c 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -195,7 +195,8 @@ config VECTORS_BASE
>  	  The base address of exception vectors.
>  
>  config ARM_PATCH_PHYS_VIRT
> -	bool "Patch physical to virtual translations at runtime"
> +	bool "Patch physical to virtual translations at runtime" if EMBEDDED
> +	default y
>  	depends on !XIP_KERNEL && MMU
>  	depends on !ARCH_REALVIEW || !SPARSEMEM
>  	help
> @@ -207,6 +208,10 @@ config ARM_PATCH_PHYS_VIRT
>  	  of physical memory is at a 16MB boundary, or theoretically 64K
>  	  for the MSM machine class.
>  
> +	  Only disable this option if you know what you do not require
> +	  this feature (eg, building a kernel for a single machine) and
> +	  you need to shrink the kernel to the minimal size.
> +
>  config ARM_PATCH_PHYS_VIRT_16BIT
>  	def_bool y
>  	depends on ARM_PATCH_PHYS_VIRT && ARCH_MSM
> @@ -301,7 +306,6 @@ config ARCH_AT91
>  	select ARCH_REQUIRE_GPIOLIB
>  	select HAVE_CLK
>  	select CLKDEV_LOOKUP
> -	select ARM_PATCH_PHYS_VIRT if MMU
>  	help
>  	  This enables support for systems based on the Atmel AT91RM9200,
>  	  AT91SAM9 and AT91CAP9 processors.
> -- 
> 1.7.4.4
> 
> 

[regression] Re: Enabling runtime P2V by default

[Jonathan Nieder]

Will Deacon wrote:
> On Wed, Aug 10, 2011 at 10:16:35AM +0100, Russell King - ARM Linux wrote:
>> On Tue, Aug 09, 2011 at 09:41:38PM +0200, Linus Walleij wrote:

>>> From: Linus Walleij <linus.walleij@linaro.org>
>>>
>>> This works like a charm so I'll just default-select it.
>>
>> Well, we can remove the EXPERIMENTAL status of this option now.  This
>> raises the question is whether we should now default it to 'y' - I
>> think we should.  Anyone have any objections?
>
> I've been running with this option enabled for the collection of ARM boards
> I have and the only problem I have encountered was related to u-boot loading
> at the wrong address.

Hm, another u-boot problem was found[1].  On the affected machines it
prevents booting a compressed kernel (but uncompressed kernels are
fine) unless CONFIG_ARM_PATCH_PHYS_VIRT is disabled or u-boot is
patched.

> So I'm all for enabling it by default, especially since it will force out
> any remaining issues for boards where this hasn't been used extensively.

Looks like it worked. ;-)

So, what now?  Is there anything the kernel can do to work around
this, or should we just live with the regression until people patch
their u-boot?

Thanks,
Jonathan

[1] http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/124051/focus=124062
https://bugzilla.kernel.org/show_bug.cgi?id=42680

[regression] Re: Enabling runtime P2V by default

[Nicolas Pitre]

On Mon, 6 Feb 2012, Jonathan Nieder wrote:

> Will Deacon wrote:
> > On Wed, Aug 10, 2011 at 10:16:35AM +0100, Russell King - ARM Linux wrote:
> >> On Tue, Aug 09, 2011 at 09:41:38PM +0200, Linus Walleij wrote:
> 
> >>> From: Linus Walleij <linus.walleij@linaro.org>
> >>>
> >>> This works like a charm so I'll just default-select it.
> >>
> >> Well, we can remove the EXPERIMENTAL status of this option now.  This
> >> raises the question is whether we should now default it to 'y' - I
> >> think we should.  Anyone have any objections?
> >
> > I've been running with this option enabled for the collection of ARM boards
> > I have and the only problem I have encountered was related to u-boot loading
> > at the wrong address.
> 
> Hm, another u-boot problem was found[1].  On the affected machines it
> prevents booting a compressed kernel (but uncompressed kernels are
> fine) unless CONFIG_ARM_PATCH_PHYS_VIRT is disabled or u-boot is
> patched.
> 
> > So I'm all for enabling it by default, especially since it will force out
> > any remaining issues for boards where this hasn't been used extensively.
> 
> Looks like it worked. ;-)
> 
> So, what now?  Is there anything the kernel can do to work around
> this, or should we just live with the regression until people patch
> their u-boot?

You really do want to have uboot patched.  Who knows what other latent 
issues are there that you don't know about.


Nicolas

[regression] Re: Enabling runtime P2V by default

[Jonathan Nieder]

Nicolas Pitre wrote:

> You really do want to have uboot patched.  Who knows what other latent 
> issues are there that you don't know about.

Ah, sorry for the lack of clarity.  I don't believe this u-boot patch[1]
has been committed anywhere, so even people with the latest u-boot are
affected.  It's just available from the mailing list archive.

"It's not worth working around this bootloader bug" may still be the
right answer.  After all, in a couple of months presumably most people
will have the patch applied.  And for all I know, it might not even be
possible to work around this bootloader bug, short of flipping the P2V
default again (which seems like a bad idea).  I just wanted to make
sure you were aware of it.

Cheers,
Jonathan

[1] http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/124051/focus=124062

[regression] Re: Enabling runtime P2V by default

[Ian Campbell]

On Mon, 2012-02-06 at 17:23 -0600, Jonathan Nieder wrote:
> Nicolas Pitre wrote:
> 
> > You really do want to have uboot patched.  Who knows what other latent 
> > issues are there that you don't know about.
> 
> Ah, sorry for the lack of clarity.  I don't believe this u-boot patch[1]
> has been committed anywhere, so even people with the latest u-boot are
> affected.  It's just available from the mailing list archive.

FYI it has now been applied to the Marvell u-boot custodian's tree:
git://git.denx.de/u-boot-marvell.git

Ian.

-- 
Ian Campbell


An evil mind is a great comfort.