Hi Martin (and Loïc),
I re-factored this patch to incorporate the suggested changes and
so it will apply cleanly to the latest svn repository.
See comments in-line:
On 09/19/2010 09:38 AM, Loïc Minier wrote:
> 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.
Yep - this was a copy paste error. The extra 8 bytes are not required.
>
>> + 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.
Removed the $tmp file creation.
>
>> + 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.
Good catch - removing this step makes the upgrade go a _lot_ faster.
>
>> + 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")
Done.
>
> 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?
Martin - I am sending this patch in 2 sections. One to fix the
problems found in the code review and one to add the d-i parts.
However, like Loïc pointed out the d-i sections are not tested.
I think those changes are pretty straight forward, but without
testing it is risky to add them. I'm ok if you decide not to.
>
>> 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",
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?
Isn't the subarch variable is just the last value of the kernel
name? So in this case "vmlinuz-2.6.35-1006-linaro-vexpress" =>
vexpress. If there are other "tiles" that can connect to the
Versatile Express motherboard won't they have similar kernel
naming constructs? Wouldn't this mean we could have multiple
kernels per subarch?
>
>> This shouldn't say "Linaro". I wonder if it would make sense to drop
>> "Debian" from all kernel/ramdisk references.
I made this change by just removing "Debian" from "Debian kernel" and
"Debian ramdisk". Is that what you were looking for? Or do you
want me to add my own $kernel and $intrd variables?
>
> +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)
Right - padding isn't necessary on NOR writes anyway. So this whole
problem is moot.
>
> 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).
Even if it's not zero padded it doesn't hurt u-boot. U-boot reads
the size of the initrd from the header and only copies the correct
size. So no calculations are required. This is the output from
u-boot when the ramdisk is loaded:
...
## Loading init Ramdisk from Legacy Image at 61000000 ...
Image Name: Ramdisk 2.6.35-1006-linaro-vexpr
Image Type: ARM Linux RAMDisk Image (uncompressed)
Data Size: 3058308 Bytes = 2.9 MiB
Load Address: 81000000
Entry Point: 81000000
Verifying Checksum ... OK
Loading Kernel Image ... OK
Thanks for spending time on this.
--Matt
>
> Thanks for the review!
>
> Cheers,
Index: flash-kernel
===================================================================
--- flash-kernel (revision 64840)
+++ flash-kernel (working copy)
@@ -85,6 +85,7 @@
if [ "x$1" = "x--supported" ]; then
case "$machine" in
+ "ARM-Versatile Express CA9x4") exit 0 ;;
"Buffalo Linkstation Pro/Live") exit 0 ;;
"Buffalo/Revogear Kurobox Pro") exit 0 ;;
"D-Link DNS-323") exit 0 ;;
@@ -112,8 +113,8 @@
kvers="$1"
kfile="/boot/vmlinuz-$kvers"
ifile="/boot/initrd.img-$kvers"
- desc="Debian kernel $1"
- idesc="Debian ramdisk $1"
+ desc="Kernel $1"
+ idesc="Ramdisk $1"
else
if [ -e /vmlinuz ]; then
kfile=/vmlinuz
@@ -124,8 +125,8 @@
else
error "Cannot find a default kernel in /vmlinuz or /boot/vmlinuz"
fi
- desc="Debian kernel"
- idesc="Debian ramdisk"
+ desc="Kernel"
+ idesc="Ramdisk"
fi
if [ ! -e $kfile ] || [ ! -e $ifile ]; then
@@ -141,6 +142,42 @@
subarch=$(echo "$kfile" | sed -e 's/.*-//')
case "$machine" in
+ "ARM-Versatile Express CA9x4")
+ check_subarch "vexpress"
+ check_mtd
+
+ kmtd=$(mtdblock "kernel")
+ if [ -z "$kmtd" ]; then
+ error "Cannot find mtd partition 'kernel'"
+ fi
+ check_dev_mtdblock "$kmtd"
+ kmtdsize=$(mtdsize "kernel")
+ check_size "kernel" $(($kfilesize + 64)) $kmtdsize
+ printf "Generating a u-boot compatible kernel image... " >&2
+ mkimage -A arm -O linux -T kernel -C none -a 0x60008000 \
+ -e 0x60008000 -n "$desc" -d $kfile $kfile.uboot \
+ >&2 1>/dev/null
+ printf "Writing kernel to flash... " >&2
+ cat $kfile.uboot > $kmtd || error "failed."
+ echo "done." >&2
+ rm -f $kfile.uboot
+
+ imtd=$(mtdblock "initrd")
+ if [ -z "$imtd" ]; then
+ error "Cannot find mtd partition for initrd"
+ fi
+ check_dev_mtdblock "$imtd"
+ imtdsize=$(mtdsize "initrd")
+ check_size "initrd" $ifilesize $imtdsize
+ printf "Generating u-boot compatible initrd image... " >&2
+ mkimage -A arm -O linux -T ramdisk -C none -a 0x81000000 \
+ -e 0x81000000 -n "$idesc" -d $ifile $ifile.uboot \
+ >&2 1>/dev/null
+ printf "Writing initrd to flash... " >&2
+ cat $ifile.uboot > $imtd || error "failed."
+ echo "done." >&2
+ rm -f $ifile.uboot
+ ;;
"Buffalo Linkstation Pro/Live" | "Buffalo/Revogear Kurobox Pro")
check_subarch "orion5x"
tmp="$(tempfile)"
Index: README
===================================================================
--- README (revision 64840)
+++ README (working copy)
@@ -12,6 +12,7 @@
The following systems are supported:
+ - ARM-Versatile Express CA9x4
- Buffalo Linkstation Live
- Buffalo Linkstation Pro
- Buffalo/Revogear Kurobox Pro
Index: debian/flash-kernel-installer.postinst
===================================================================
--- debian/flash-kernel-installer.postinst (revision 64840)
+++ debian/flash-kernel-installer.postinst (working copy)
@@ -23,6 +23,9 @@
# Are we writing to flash or constructing an image on disk?
write_to_flash() {
case "$machine" in
+ "ARM-Versatile Express CA9x4")
+ return 1
+ ;;
"Buffalo Linkstation Pro/Live")
return 1
;;
@@ -91,6 +94,12 @@
db_progress STEP 1
case "$machine" in
+ "ARM-Versatile Express CA9x4")
+ in-target update-initramfs -u || true
+ if ! apt-install uboot-mkimage; then
+ error "apt-install uboot-mkimage failed"
+ fi
+ ;;
"Buffalo Linkstation Pro/Live" | "Buffalo/Revogear Kurobox Pro")
in-target update-initramfs -u || true
if ! apt-install uboot-mkimage; then
Index: debian/flash-kernel-installer.isinstallable
===================================================================
--- debian/flash-kernel-installer.isinstallable (revision 64840)
+++ debian/flash-kernel-installer.isinstallable (working copy)
@@ -14,6 +14,9 @@
arm*/orion5x)
exit 0
;;
+ arm*/vexpress)
+ exit 0
+ ;;
# Don't activate it by default
*)
exit 1
Index: initramfs-tools/hooks/flash_kernel_set_root
===================================================================
--- initramfs-tools/hooks/flash_kernel_set_root (revision 64840)
+++ initramfs-tools/hooks/flash_kernel_set_root (working copy)
@@ -23,6 +23,9 @@
# device?
root_type() {
case "$1" in
+ "ARM-Versatile Express CA9x4")
+ echo "override"
+ ;;
"Buffalo Linkstation Pro/Live")
echo "override"
;;
