[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: [PATCH 1/2] Add support for the CuBox-i.



On Sat, 2014-05-31 at 00:18 +0200, Steve Langasek wrote:
> On Fri, May 30, 2014 at 07:59:29PM +0100, Ian Campbell wrote:
> > > diff --git a/bootscript/bootscr.cubox b/bootscript/bootscr.cubox
> > > new file mode 100644
> > > index 0000000..4aa27d4
> > > --- /dev/null
> > > +++ b/bootscript/bootscr.cubox
> > > @@ -0,0 +1,22 @@
> > > +setenv ramdiskaddr 0x11800000;
> > > +setenv ramdisk uInitrd;
> > > +setenv file_prefix;
> > > +for prefix in ${boot_prefixes}; do
> > > +  echo Loading ${prefix}${ramdisk} to ${ramdiskaddr};
> > > +  if load mmc ${mmcdev}:${mmcpart} ${ramdiskaddr} ${prefix}${ramdisk}; then
> > > +    setenv file_prefix ${prefix};
> > > +  fi;
> > > +done;
> > > +if test -z ${file_prefix}; then
> > > +  echo No ramdisk found;
> > > +  setenv ramdiskdaddr -;
> > > +fi;
> > > +setenv autobootfdt echo\ Booting\ \${boot_file}\;\ if\ test\ \${boot_file}\ =\ zImage\;\ then\ bootz\ \${loadaddr}\ \${ramdiskaddr}\ \${fdt_addr}\;\ else\ bootm\ \${loadaddr}\ \${ramdiskaddr}\ \${fdt_addr}\;\ fi;

You setenv this and then immediately run it, does something else later
also call this function? If not then just writing the code directly
would avoid all the escaping and allow you to use newlines etc -- it'd
be a lot clearer then what was going on.

> > Is all of this fallback/checking etc stuff really needed? With a
> > packaged kernel we can make certain simplifying assumptions, and for a
> > non-packaged kernel does f-k even work at all? I would half expect
> > someone installing a kernel manually to also manually write the boot
> > script.
> 
> I intended this to be a minimal modification of the (quite elaborate)
> upstream default environment, extending it to support ramdisks in a way that
> can potentially be integrated into upstream (eventually).  We could
> certainly make a simpler boot script that would be generic, but I would
> prefer something that would continue to boot correctly if the user manually
> replaces the kernel on disk with an unpackaged one.

I'm not sure someone doing that can reasonably expect that the
flash-kernel installed boot.scr will DTRT for them unless they are using
a packaged kernel.

People who want to build their own custom kernel can use the upstream
make deb-pkg and it ought to integrate fine with flash-kernel (it
supports all the correct hooks etc).

People stepping outside of that integrated world really ought to not use
flash-kernel IMHO (and if they are setting up kernels etc themselves
already I think flash-kernel is no great loss to them).

> > > diff --git a/db/all.db b/db/all.db
> > > index 68f85b4..dce775d 100644
> > > --- a/db/all.db
> > > +++ b/db/all.db
> > > @@ -436,6 +436,23 @@ Boot-DTB-Path: /boot/dtb
> > >  Required-Packages: u-boot-tools
> > >  Bootloader-Sets-Incorrect-Root: no
> > >  
> > > +Machine: SolidRun i.MX 6Quad/Dual/DualLite/Solo CuBox-i Board
> > > +Machine: SolidRun Cubox-i Dual/Quad
> 
> > Why are there two? Are there two possible DTBs for this board or is
> > there some old board file style support too?
> 
> There is old-style support as well; SolidRun currently still makes available
> a 3.0 BSP kernel,

Which is non-device tree enabled I presume (uses board.c style stuff)?

>  and the machine identifier is different on that kernel
> than on a more recent one.

OK

>   Note that the first machine identifier lists all
> four flavors of cubox-i, but there are two different dtbs - one for
> DualLite+Solo, one for Quad+Dual.  So we can't obviously detect which of the
> two dtbs is right for all of the machines when running an old kernel.

We have the same problem for the QNAP TS-xxx transition to device tree,
where one machine name leads to two possible DTBs. I've got some WIP
patches locally which allow the DTB-Id field to reference a script which
probes for the appropriate one. Would that be useful for you too here?
If so I can post -- I'd been holding off waiting for the armel buildd to
catch up with the latest kernel in experimental that is needed for
people to actually test the qnap side.

>   I
> think it's still useful to default to installing the right dtb on the
> duals and quads when running an old kernel, than to do nothing for any of
> them.

It's certainly better than nothing. Do these systems come with serial
access to the u-boot prompt by default? IOW, are we risking breaking
anything in a non-trivial (i.e. requires a soldering iron or additional
h/w etc) to recover way?

> > How does it work for you without? Does the firmware also embed a DTB? I
> > notice the script uses fdt_addr but doesn't load anything to it so I
> > guess something like that must be happening?
> 
> The firmware does not embed a DTB; I was previously using one that was
> locally built and installed, not from a kernel package, for testing
> purposes.  The fdt_addr variable is set, by the default cubox-i u-boot env.

I think you mean that the DTB is loaded to fdt_addr by the default uboot
env? And from the snippet you show later it seems that it will
automagically pick the correct fdt name? In which case perhaps we could
extend DTB-Id to support a list of all the DTBs to install, which would
then work for all these systems?

I suppose eventually
https://wiki.linaro.org/Platform/DeviceTreeConsolidation might also help
with this case eventually (in which case bodging it for now wouldn't be
so bad)

> > > +#Boot-DTB-Path: /boot/imx6q-cubox-i.dtb
> > > +Boot-Kernel-Path: /boot/zImage
> > > +Boot-Initrd-Path: /boot/uInitrd
> > > +Boot-Script-Path: /boot/boot.scr
> > > +U-Boot-Initrd-Address: 0x0
> > > +#FIXME: we really want initramfs support in the u-boot scripting, so we don't
> > > +#have to use a script to get initramfs support
> > > +U-Boot-Script-Address: 0x0
> > > +U-Boot-Script-Name: bootscr.cubox
> 
> > If the u-boot supports bootz and we are happy to use it in the boot.scr
> > (I think if we should when we can) then Boot-{DTB,Kernel,Initrd}-Path
> > and U-Boot-Initrd-Address aren't needed.
> 
> It does use bootz.  The Boot-Kernel-Path is made use of by my code changes
> in the second patch, to create a symlink under a well-known location so the
> bootloader can find it automatically at boot.

I'm a bit confused -- isn't it the boot.scr which you have added which
should be finding the kernel at a location which the script and the
flash-kernel db have agreed on (an internal detail)?

All the bootloader needs to find is the boot.scr, which doesn't require
symlinks since it can use vmlinuz-@@KERNEL_VERSION@@ etc (see the new
sunxi boot.scr for example)

I guess you are trying to make this work both via boot.scr and via the
existing u-boot env support (or some hybrid of the two?), perhaps with
the aim of supporting unpackaged kernels you mention earlier? Wouldn't
picking one or the other simplify everything?

IOW either boot.scr loads vmlinuz+initrd+fdt from the paths which it and
flash-kernel have agreed among themselves and boots things itself -or-
flash-kernel installs uImage/uInitrd/dtb in whatever place the stock
u-boot env will load them from and in the format which the stock u-boot
env wants and can boot with no boot.scr (I've half inferred that maybe
the second approach doesn't apply because the stock u-boot doesn't do
initrd?)

I'm not mortally opposed to the hybrid approach or anything, but it does
make it hard for someone looking at flash-kernel to get the whole
picture of what is going on and seems more complicated than either of
the other approaches (but maybe it just seems that way when I can only
see half of what is going on).

>   The Boot-Initrd-Path and
> U-Boot-Initrd-Address are used because I'm still creating a uInitrd, for use
> with the zImage, because I didn't know about the "${filesize}" trick for the
> initrd.  I'll try to integrate that properly in the script and see what I
> get.

Thanks.

>   The Boot-DTB-Path is needed to match the default path used by cubox-i
> U-Boot to locate the dtb; the only other way to load this automatically
> seems to be to append it to the kernel image, which we don't otherwise need
> to modify on this platform, and furthermore makes it impossible to prepare a
> single image which can boot on all the cubox-i flavors.

Yes, avoiding DTB append is certainly preferable it's really only for
the case of non-upgradeable factory u-boot which lacks FDT support
entirely.

> > I'm not sure I understand why U-Boot-Script-Address exists at all, I
> > never use it or pass any of the relevant options to mkimage -- I suspect
> > you can just drop it.
> 
> Hmm?  It seems to be used in the version of flash-kernel functions I see in
> trunk:

About half the db entries include it and those are all set to 0 which
isn't ram at all on many ARM systems and I suspect at least for -T
script ends up meaning "disabled".

> I can certainly try dropping it, but this was consistent with existing
> U-Boot Script handling at the time I wrote this.

yeah, I think the existing stuff here is bogus, or at least I'm not
aware of the reason for it :-/ I'm not brave enough to remove the
existing ones without access to the h/w, but if a new thing works
without I think we should omit it.

Ian.


Reply to: