| Message ID | 20110622145828.GL6069@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wednesday 22 June 2011, Sascha Hauer wrote: > --- a/arch/arm/boot/Makefile > +++ b/arch/arm/boot/Makefile > @@ -59,6 +59,11 @@ $(obj)/zImage: $(obj)/compressed/vmlinux FORCE > > endif > > +ifneq ($(words $(ZRELADDR)), 1) > +$(obj)/uImage: FORCE > + @echo 'Multiple zreladdrs, cannot build uImage' > + exit 1 > +else Failing the build for a possible configuration is not nice, I would be happier if this could be prevented in Kconfig. I have recently worked on getting all 'randconfig' build working for a bunch of platforms (patches will follow), so it would be a shame to have another guaranteed failure built into the kernel. Your patch is ok by itself, in order to detect the case when Kconfig logic has failed, but we should also have a way to prevent enabling uImage building in Kconfig for configurations that are known to not work. Arnd
On Wed, Jun 22, 2011 at 05:10:39PM +0200, Arnd Bergmann wrote: > On Wednesday 22 June 2011, Sascha Hauer wrote: > > --- a/arch/arm/boot/Makefile > > +++ b/arch/arm/boot/Makefile > > @@ -59,6 +59,11 @@ $(obj)/zImage: $(obj)/compressed/vmlinux FORCE > > > > endif > > > > +ifneq ($(words $(ZRELADDR)), 1) > > +$(obj)/uImage: FORCE > > + @echo 'Multiple zreladdrs, cannot build uImage' > > + exit 1 > > +else > > Failing the build for a possible configuration is not nice, I would > be happier if this could be prevented in Kconfig. I have recently > worked on getting all 'randconfig' build working for a bunch of platforms > (patches will follow), so it would be a shame to have another > guaranteed failure built into the kernel. Arnd, I don't think you're _thinking_. If you issue 'make uImage', how is the kernel configuration stuff to know that it should not enable multiple platforms which would make building a uImage technically impossible (and lead to the created uImage being non-bootable on some of those platforms.) The answer is: you can't. The best we can do is if the configuration encounters this problem, then fail to build a uImage. If that concerns you, then don't ask the kernel to build an inherently fragile boot loader format in the first place.
On Wed, Jun 22, 2011 at 04:58:28PM +0200, Sascha Hauer wrote: > On Wed, Jun 22, 2011 at 10:03:21AM +0100, Russell King - ARM Linux wrote: > > 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. > > Then I have the same problem in my platform because I have no clear > condition on which to set this variable. Currently there is no "more > than one zreladdr" indicator. Maybe a variant of the following patch > is the least of all evils. A mechanism to specify the load address on > the command line could be added. I think your patch looks good so far, but with one exception - we probably want to detect off LOADADDR so that we can override it by passing 'make uImage LOADADDR=blah'. That would allow uImage's to be generated without having to learn all the silly the mkimage command line arguments. The other issue here is that having multiple words in ZRELADDR without AUTO_ZRELADDR enabled in the decompressor is going to lead to decompressor link time errors - so that's something else which needs thinking about. (Do you ensure that it's already set somehow?)
On Wednesday 22 June 2011, Russell King - ARM Linux wrote: > I don't think you're _thinking_. If you issue 'make uImage', how is the > kernel configuration stuff to know that it should not enable multiple > platforms which would make building a uImage technically impossible > (and lead to the created uImage being non-bootable on some of those > platforms.) > > The answer is: you can't. > > The best we can do is if the configuration encounters this problem, then > fail to build a uImage. If that concerns you, then don't ask the kernel > to build an inherently fragile boot loader format in the first place. Ok, I see. I still need to learn about a lot of ways in which ARM differs from other architectures I've worked with. I was thinking of CONFIG_DEFAULT_UIMAGE on powerpc, which causes the uimage to be built as a default make target. Since ARM doesn't have that option, this obviously isn't a problem for randconfig builds. Sorry for the noise. Arnd
On Wed, Jun 22, 2011 at 04:22:51PM +0100, Russell King - ARM Linux wrote: > On Wed, Jun 22, 2011 at 04:58:28PM +0200, Sascha Hauer wrote: > > On Wed, Jun 22, 2011 at 10:03:21AM +0100, Russell King - ARM Linux wrote: > > > 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. > > > > Then I have the same problem in my platform because I have no clear > > condition on which to set this variable. Currently there is no "more > > than one zreladdr" indicator. Maybe a variant of the following patch > > is the least of all evils. A mechanism to specify the load address on > > the command line could be added. > > I think your patch looks good so far, but with one exception - we probably > want to detect off LOADADDR so that we can override it by passing > 'make uImage LOADADDR=blah'. That would allow uImage's to be generated > without having to learn all the silly the mkimage command line arguments. > > The other issue here is that having multiple words in ZRELADDR without > AUTO_ZRELADDR enabled in the decompressor is going to lead to decompressor > link time errors - so that's something else which needs thinking about. Having multiple ZRELADDR without AUTO_ZRELADDR is broken anyway, so all we have to do here is to provide a hint to the user instead of failing in the linker, right?
diff --git a/arch/arm/boot/Makefile b/arch/arm/boot/Makefile index 9128fdd..40c9bbf 100644 --- a/arch/arm/boot/Makefile +++ b/arch/arm/boot/Makefile @@ -59,6 +59,11 @@ $(obj)/zImage: $(obj)/compressed/vmlinux FORCE endif +ifneq ($(words $(ZRELADDR)), 1) +$(obj)/uImage: FORCE + @echo 'Multiple zreladdrs, cannot build uImage' + exit 1 +else quiet_cmd_uimage = UIMAGE $@ cmd_uimage = $(CONFIG_SHELL) $(MKIMAGE) -A arm -O linux -T kernel \ -C none -a $(LOADADDR) -e $(STARTADDR) \ @@ -75,6 +80,7 @@ $(obj)/uImage: STARTADDR=$(LOADADDR) $(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 $@ diff --git a/arch/arm/mach-imx/Makefile.boot b/arch/arm/mach-imx/Makefile.boot index ebee18b..dbe6120 100644 --- a/arch/arm/mach-imx/Makefile.boot +++ b/arch/arm/mach-imx/Makefile.boot @@ -1,19 +1,19 @@ -zreladdr-$(CONFIG_ARCH_MX1) := 0x08008000 +zreladdr-$(CONFIG_ARCH_MX1) += 0x08008000 params_phys-$(CONFIG_ARCH_MX1) := 0x08000100 initrd_phys-$(CONFIG_ARCH_MX1) := 0x08800000 -zreladdr-$(CONFIG_MACH_MX21) := 0xC0008000 +zreladdr-$(CONFIG_MACH_MX21) += 0xC0008000 params_phys-$(CONFIG_MACH_MX21) := 0xC0000100 initrd_phys-$(CONFIG_MACH_MX21) := 0xC0800000 -zreladdr-$(CONFIG_ARCH_MX25) := 0x80008000 +zreladdr-$(CONFIG_ARCH_MX25) += 0x80008000 params_phys-$(CONFIG_ARCH_MX25) := 0x80000100 initrd_phys-$(CONFIG_ARCH_MX25) := 0x80800000 -zreladdr-$(CONFIG_MACH_MX27) := 0xA0008000 +zreladdr-$(CONFIG_MACH_MX27) += 0xA0008000 params_phys-$(CONFIG_MACH_MX27) := 0xA0000100 initrd_phys-$(CONFIG_MACH_MX27) := 0xA0800000 -zreladdr-$(CONFIG_ARCH_MX3) := 0x80008000 +zreladdr-$(CONFIG_ARCH_MX3) += 0x80008000 params_phys-$(CONFIG_ARCH_MX3) := 0x80000100 initrd_phys-$(CONFIG_ARCH_MX3) := 0x80800000