On Wed, Sep 29, 2010, Shawn Guo wrote:
> I made a l-m-c patch to add imx51 support. I need your help to review
> and merge the code.
Thanks!
You probably want to use the board name, mx51evk, not the SoC name,
imx51; this also avoids mixing mx51 (kernel flavor) and imx51 (DEVNAME)
in the script.
> install_hwpack() {
> - ensure_command qemu-arm-static qemu-arm-static
> + # Check host architecture
> + arch_is_arm=false
> + arch_str=`uname -m`
> + if [[ "$arch_str" == arm* ]]; then
> + arch_is_arm=true
> + fi
> +
> + if [ ! $arch_is_arm ]; then
These [ ! false ] or [ ! true ] are incorrect; [ ! anything ] is always
false; "!" tests for non-empty.
I suggest you move the ensure_command to the place where qemu is copied
and change it into something less bash-specific for readability:
local arch_is_arm=no
case `uname -m` in
arm*)
arch_is_arm=yes
ensure_command qemu-arm-static qemu-arm-static
sudo cp /usr/bin/qemu-arm-static ${chroot}/usr/bin
;;
esac
[...]
if [ "arch_is_arm" = no ]; then
sudo rm -f ${chroot}/usr/bin/qemu-arm-static
fi
> @@ -371,10 +385,17 @@
>
> # Create a VFAT or FAT16 partition of 9 cylinders which is about 64M
> # and a linux partition of the rest
> + if [ "$DEVIMAGE" = imx51 ]; then
> + sudo sfdisk -D -H $HEADS -S $SECTORS $CYLINDER_ARG $partdev << THEEND
> +1,9,$PARTITION_TYPE,*
> +10,,,-
> +THEEND
> + else
> sudo sfdisk -D -H $HEADS -S $SECTORS $CYLINDER_ARG $partdev << THEEND
> ,9,$PARTITION_TYPE,*
> ,,,-
> THEEND
> + fi
Could we just use the same partition layout for all images (the imx51
one), avoiding the if entirely?
Not your doing, but the sfdisk man page recommends avoiding the c,h,s
specs especially since -H/-S are passed already
If you're using the early data on the image to store stuff, I recommend
you protect it with a dumb partition (e.g. "non-FS data" type, 0xda).
> + imx51)
> + sudo dd if=binary/usr/lib/u-boot/mx51evk/u-boot.imx
> of=/dev/mmcblk0 bs=512 seek=2
bs=1024 seek=1?
I definitely think you want a non-FS data partition protecting this
zone
> + sync
Do you really need a sync in there? There are a bunch of syncs in the
script already
> + sudo mkimage -A arm -O linux -T kernel -C none -a 0x90008000 -e
> 0x90008000 \
> + -n Linux -d "${DIR}/binary/${parts_dir}"/vmlinuz*-mx51
> "${BOOT_DISK}/uImage"
> + sudo mkimage -A arm -O linux -T ramdisk -C none -a 0 \
> + -e 0 -n initramfs \
> + -d "${DIR}/binary/${parts_dir}"/initrd.img-*-linaro-mx51 \
> + "${BOOT_DISK}/uInitrd"
Could you use -linaro-mx51 for the vmlinuz pattern as well? Ideally,
please wrap this at 80 chars like the rest of the code now.
> + cat > ${TMP_DIR}/boot.cmd << BOOTCMD
> +setenv bootcmd 'fatload mmc 0:1 0x90000000 uImage; fatload mmc 0:1
> 0x90800000 uInitrd; bootm 0x90000000 0x90800000'
no mmc init?
> +setenv bootargs '${serial_opts} ${splash_opts} ${lowmem_opt}
> ${boot_snippet} rootwait ro'
> +boot
> +BOOTCMD
would be great if you could merge this with the create_boot_cmd()
logic
> + sudo mkimage -A arm -O linux -T script -C none -a 0 \
> + -e 0 -n "$CODENAME 10.05" -d "${TMP_DIR}/boot.cmd" \
> + "${BOOT_DISK}/boot.scr"
> + ;;
the -n stuff needs to be updated
Otherwise looks good, thanks again!
--
Loïc Minier
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
