Bug#596889: flash-kernel: please add ARM-Versatile Express CA9x4 support

[Loïc Minier <loic.minier@linaro.org>]

        Hey

 Sorry for the delay

On Tue, Sep 14, 2010, Matt Waddel wrote:
> +             check_size "kernel" $(($kfilesize + 8 + 64)) $kmtdsize

 The 8+64 seems to be copied from other platforms which prepend 8 bytes
 of ARM assembly to set the machine id (the "devio" calls in the
 script) and wrap the resulting prefixed kernel in an uImage which
 presunably adds 64 bytes of header -- I checked the latter with a
 simple mkimage call on a vmlinuz file, and it showed exactly 64 bytes
 of increment, but I have no idea whether any padding / rounding is
 going on, or whether your specific mkimage args might require more than
 64 bytes.

 So I think this should be:
    check_size "kernel" $(($kfilesize + 64)) $kmtdsize
 but am not sure.

> +             tmp=$(tempfile) 
> +             cat $kfile >> $tmp
> +             mkimage -A arm -O linux -T kernel -C none -a 0x60008000 \
> +                     -e 0x60008000 -n "Linaro Kernel" -d $tmp $tmp.uboot \
> +                     >&2 1>/dev/null

 Don't think you actually need a tmp file since you don't prepend
 anything to the kernel; just mkimage $kfile directly.

> +             printf "Erasing Kernel NOR space... " >&2 
> +             dd if=/dev/zero of=$kmtd bs=$kmtdsize count=1 2>/dev/null

 Does one really need to erase the NOR space?  Given that access are
 done with mtdblock devices, I expected the kenrel would just do it.  In
 some cases, it's useful to zero-pad, but I'm not sure I see why here.

> +             initrd_nor_size=25165824

 Hardcoding the value sucks; could you please read it from /proc/mtd?
 Apparently, there's information since you do:
> +             imtd=$(mtdblock "initrd")

 you can get the size of the mtd just like for the kernel with:
    imtdsize=$(mtdsize "initrd")

On Tue, Sep 14, 2010, Martin Michlmayr wrote:
> That's fine but you should also add support for the udeb, i.e.
> debian/flash-kernel-installer.isinstallable,
> debian/flash-kernel-installer.postinst

 Perhaps not unless we add d-i support?

 Right now, this board is only used in Linaro images and we don't use
 d-i or udebs at all, only flash-kernel for upgrades.  This might change
 in the future though.

 I didn't think about this when I suggested to Matt he could send the
 Vexpress support to Debian.  Would you rather have us add d-i support
 as well, or can you live with just the flash-kernel.deb changes for
 now?

> Would it also make sense to add the device to
> initramfs-tools/hooks/flash_kernel_set_root?

 Ah good catch; Matt, this allows either setting a default root= in the
 initrd for cases where the bootloader might not pass one, or to always
 override the root= passed by the bootloader.  Depending on what the
 default bootloader config is, pick one!  :-)

 If you allow to install to anywhere and the bootloader has a hardcoded
 root=, "override" is the best choice; if the bootloader does not pass
 any root= by default, "default" is the best choice.  If you tell people
 to upgrade the bootloader config, it doesn't matter.


 I didn't know or I didn't remember about this set_root infrastructure,
 but I suppose another option is to arrange to update the bootloader
 config to set root=, in which case one doesn't need the initramfs bits.

> Colin, Loïc: can you comment on the patch since I don't know anything
> about Versatile Express?

 Eh, I don't know much either; good idea to have copied Colin as I
 understand he knows the hardware quite well.

> >  case "$machine" in
> > +	"ARM-Versatile Express CA9x4")
> > +		check_mtd
> 
> Should there be a call to check_subarch?  Or there an agreen subarch
> name for this platform?

 I think the subarch is "vexpress", but I need to note that I find this
 test quite bad for the mid-term.  It means we're stuck with one kernel
 per-subarch.

 Perhaps we should check whether the corresponding kernel's config has
 support for this subarch instead?

> This shouldn't say "Linaro".  I wonder if it would make sense to drop
> "Debian" from all kernel/ramdisk references.

 +1 and +1

> > +			>&2 1>/dev/null
> > +		printf "Erasing Kernel NOR space... " >&2 
> > +		dd if=/dev/zero of=$kmtd bs=$kmtdsize count=1 2>/dev/null
> 
> How large is this partition?  The reason I ask is because I wonder why
> you don't pad the file on disk before writing it, but maybe that's a
> bad idea when the mtd partition is very large. (Same goes for the
> initrd partition)

 I don't see why this would be needed for the kernel myself.

 It might make sense for the initrd though, unless u-boot parses the
 uInitrd headers to copy just the right size (preferred).

 Thanks for the review!

   Cheers,
-- 
Loïc Minier