i.MX consolidation patches

On Wed, Jun 22, 2011 at 10:03:21AM +0100, Russell King - ARM Linux wrote:
> 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.

Yes.

> 
> 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.

> 
> 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.

I don't want to go this way. Personally I prefer zImages but often
enough U-Boot forces me to use uImages and generating uImages by hand is
no fun.

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

[PATCH] ARM i.MX: Do not allow building uImage with different
 zreladdrs

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/boot/Makefile          |    6 ++++++
 arch/arm/mach-imx/Makefile.boot |   10 +++++-----
 2 files changed, 11 insertions(+), 5 deletions(-)

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?