diff mbox

i.MX consolidation patches

Message ID 20110622075615.GJ6069@pengutronix.de (mailing list archive)
State New, archived
Headers show

Commit Message

Sascha Hauer June 22, 2011, 7:56 a.m. UTC 
On Wed, Jun 01, 2011 at 03:24:06PM +0100, Russell King - ARM Linux wrote:
> On Wed, Jun 01, 2011 at 04:18:47PM +0200, Sascha Hauer wrote:
> > diff --git a/arch/arm/boot/Makefile b/arch/arm/boot/Makefile
> > index 9128fdd..e3b6f02 100644
> > --- a/arch/arm/boot/Makefile
> > +++ b/arch/arm/boot/Makefile
> > @@ -59,6 +59,11 @@ $(obj)/zImage:	$(obj)/compressed/vmlinux FORCE
> >  
> >  endif
> >  
> > +ifeq ($(CONFIG_ARM_PATCH_PHYS_VIRT),y)
> > +$(obj)/uImage: $(obj)/zImage FORCE
> > +	@echo 'building uImages is incompatible with CONFIG_ARM_PATCH_PHYS_VIRT'
> > +	@false
> 
> It may be better to drop the zImage dependency too, so that folk don't get
> the message at the end of an otherwise successful kernel build - they may
> decide to turn p2v patching off which would result in a rebuild.
> 
> Or it could suggest the mkimage command which should be run, but with
> <LOAD-ADDRESS> inserted into it at the appropriate point - iow something
> like:
> 
>   mkimage -A arm -O linux -T kernel -C none \
> 	-a <LOAD-ADDRESS> -e <START-ADDRESS> \
> 	-n 'Linux-$(KERNELRELEASE)' -d arch/arm/boot/zImage uImage.<PLATFORM>
> 

Coming back to this topic after a longer time, here is an updated
patch doing exactly this.

8<------------------------------------------------------

[PATCH] ARM: do not allow to build uImages with ARM_PATCH_PHYS_VIRT

U-Boot uImages expect a load address and a entry point in
the image header. With CONFIG_ARM_PATCH_PHYS_VIRT these
become variable and thus can not be compiled into the uImage.
Instead of just failing we give a hint to the user how to
generate an uImage for his hardware.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/boot/Makefile |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

Comments

Russell King - ARM Linux June 22, 2011, 8:11 a.m. UTC | #1 
On Wed, Jun 22, 2011 at 09:56:15AM +0200, Sascha Hauer wrote:
> Coming back to this topic after a longer time, here is an updated
> patch doing exactly this.

I'm not sure that this is actually correct.  Whether ARM_PATCH_PHYS_VIRT
is enabled or not only affects the Image and not the zImage - provided
the mach-* provides the zreladdr in their Makefile.boot, you can build
a uImage with ARM_PATCH_PHYS_VIRT.

Where things become sticky is when AUTO_ZRELADDR is enabled - this is where
the ZRELADDR may be omitted, and therefore there is no load/start address
which can be provided to uboot.

>  $(obj)/uImage: STARTADDR=$(LOADADDR)
>  
> +ifeq ($(CONFIG_ARM_PATCH_PHYS_VIRT),y)

So maybe this wants to be ifeq ($(LOADADDR),) or CONFIG_AUTO_ZRELADDR?

> +$(obj)/uImage:  $(obj)/zImage FORCE
> +	@echo 'CONFIG_ARM_PATCH_PHYS_VIRT is enabled, cannot build an uImage'

and this CONFIG_AUTO_ZRELADDR

> +	@echo 'with multiple start/load addresses. To generate an uImage'
> +	@echo 'suitable for you hardware run:'
> +	@echo '$(MKIMAGE) -A arm -O linux -T kernel -C none -a <LOADADDR> \
> +		-e <STARTADDR> -n 'Linux-$(KERNELRELEASE)' -d $< $@'
> +	@false
> +else
>  $(obj)/uImage:	$(obj)/zImage FORCE
>  	$(call if_changed,uimage)
>  	@echo '  Image $@ is ready'
> +endif
>  
>  $(obj)/bootp/bootp: $(obj)/zImage initrd FORCE
>  	$(Q)$(MAKE) $(build)=$(obj)/bootp $@
> -- 
> 1.7.5.3
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
Sascha Hauer June 22, 2011, 8:32 a.m. UTC | #2 
On Wed, Jun 22, 2011 at 09:11:41AM +0100, Russell King - ARM Linux wrote:
> On Wed, Jun 22, 2011 at 09:56:15AM +0200, Sascha Hauer wrote:
> > Coming back to this topic after a longer time, here is an updated
> > patch doing exactly this.
> 
> I'm not sure that this is actually correct.  Whether ARM_PATCH_PHYS_VIRT
> is enabled or not only affects the Image and not the zImage - provided
> the mach-* provides the zreladdr in their Makefile.boot, you can build
> a uImage with ARM_PATCH_PHYS_VIRT.
> 
> Where things become sticky is when AUTO_ZRELADDR is enabled - this is where
> the ZRELADDR may be omitted, and therefore there is no load/start address
> which can be provided to uboot.
> 
> >  $(obj)/uImage: STARTADDR=$(LOADADDR)
> >  
> > +ifeq ($(CONFIG_ARM_PATCH_PHYS_VIRT),y)
> 
> So maybe this wants to be ifeq ($(LOADADDR),) or CONFIG_AUTO_ZRELADDR?

LOADADDR is always set. Given the following in Makefile.boot:

zreladdr-$(CONFIG_ARCH_MX1)     := 0x08008000
zreladdr-$(CONFIG_MACH_MX21)    := 0xC0008000
zreladdr-$(CONFIG_ARCH_MX25)    := 0x80008000
zreladdr-$(CONFIG_MACH_MX27)    := 0xA0008000

LOADADDR ends up being 0xA0008000 when all of the above are enabled.
Another idea I had was to replace the := with += so that we can
count the words in zreladdr-y and bail out if we have multiple
zreladdrs. I haven't really followed this approach as it means adjusting
all Makefile.boot.

CONFIG_AUTO_ZRELADDR=y also does not necessarily mean that the resulting
uImage will not work. In the above example the image will work on i.MX27


Sascha
Russell King - ARM Linux June 22, 2011, 9:03 a.m. UTC | #3 
On Wed, Jun 22, 2011 at 10:32:57AM +0200, Sascha Hauer wrote:
> LOADADDR ends up being 0xA0008000 when all of the above are enabled.
> Another idea I had was to replace the := with += so that we can
> count the words in zreladdr-y and bail out if we have multiple
> zreladdrs. I haven't really followed this approach as it means adjusting
> all Makefile.boot.
> 
> CONFIG_AUTO_ZRELADDR=y also does not necessarily mean that the resulting
> uImage will not work. In the above example the image will work on i.MX27

But conversely, PATCH_PHYS_TO_VIRT doesn't mean that the uImage won't
work either - I already build several kernels with it enabled for
testing purposes, and the result boots fine.

That's rather my point: PATCH_PHYS_TO_VIRT=y does not mean that the
uImage is unbootable - it's only unbootable if the uImage has a
load/start address which is invalid for the platform.

So, maybe the idea of changing LOADADDR is better.  Or maybe providing
Makefile.boot with a new variable 'nouimage' which can provide the
same functionality - and you may wish to provide LOADADDR on the command
line and detect that, in which case it should override this lockout.

Lastly, as PATCH_PHYS_TO_VIRT is relatively cheap, it's possible that
we may eventually end up with it enabled mostly everywhere, which with
the current approach would mean we might as well rip out the uboot
targets from the kernel entirely.
diff mbox

Patch

diff --git a/arch/arm/boot/Makefile b/arch/arm/boot/Makefile
index 9128fdd..934a67a 100644
--- a/arch/arm/boot/Makefile
+++ b/arch/arm/boot/Makefile
@@ -72,9 +72,19 @@  endif
 
 $(obj)/uImage: STARTADDR=$(LOADADDR)
 
+ifeq ($(CONFIG_ARM_PATCH_PHYS_VIRT),y)
+$(obj)/uImage:  $(obj)/zImage FORCE
+	@echo 'CONFIG_ARM_PATCH_PHYS_VIRT is enabled, cannot build an uImage'
+	@echo 'with multiple start/load addresses. To generate an uImage'
+	@echo 'suitable for you hardware run:'
+	@echo '$(MKIMAGE) -A arm -O linux -T kernel -C none -a <LOADADDR> \
+		-e <STARTADDR> -n 'Linux-$(KERNELRELEASE)' -d $< $@'
+	@false
+else
 $(obj)/uImage:	$(obj)/zImage FORCE
 	$(call if_changed,uimage)
 	@echo '  Image $@ is ready'
+endif
 
 $(obj)/bootp/bootp: $(obj)/zImage initrd FORCE
 	$(Q)$(MAKE) $(build)=$(obj)/bootp $@