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
--
To UNSUBSCRIBE, email to debian-boot-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Archive: http://lists.debian.org/20100919153807.ga13...@bee.dooz.org
