Linaro Image Tools

Code review comment for lp:~angus-akkea/linaro-image-tools/Samsung-SMDKV310

Samsung-SMDKV310
Merge into trunk

Revision history for this message

Loïc Minier (lool) wrote on 2011-02-18:

On Thu, Feb 17, 2011, Angus Ainslie wrote:
> The SMDKv310 code now follows the recommended boot (FAT) and rootfs
> (EXT) partition scheme. BL1 , u-boot, the kernel and the initrd all
> load from the raw mmc offsets currently. Once my MMC issues are
> resolved I'll be able to load some of these from the FAT partition.

So overall, the diff is much smaller and it's getting closer to being
mergeable, thanks for your work; some comments below

> === modified file 'linaro_media_create/boards.py'
> --- linaro_media_create/boards.py 2011-02-14 16:26:08 +0000
> +++ linaro_media_create/boards.py 2011-02-17 21:51:59 +0000
> @@ -419,6 +422,67 @@
> cls.load_addr, uboot_parts_dir, cls.kernel_suffix, boot_dir)
> make_uInitrd(uboot_parts_dir, cls.kernel_suffix, boot_dir)
>
> +class SamsungConfig(BoardConfig):
> + boot_env = [
> + 'bootargs=root=/dev/mmcblk0p3 rootwait rw init=/bin/bash console=ttySAC1,115200',
> + 'bootcmd=movi read kernel 40007000; movi read rootfs 41000000 600000;'
> + 'bootm 40007000 41000000',
> + 'ethact=smc911x-0',
> + 'ethaddr=00:40:5c:26:0a:5b',
> + ]
> +

Why do we have root=/dev/mmcblk0p3 here and root=/dev/mmcblk0p2 in
extra_boot_args_options? Both seem incorrect as we use UUID for
booting. Same remark for init=/bin/bash.

console= should be inferred from a serialtty variable.

I don't think we should encode the default u-boot environment into
linaro-image-tools, but rather into u-boot. So things like
ethact=smc911x-0 should go into u-boot's config for the board.

ethaddr= seems broken; this should be read from an EEPROM or should be
generated by u-boot or by the kernel once, and recorded (see what IGEP
or beagle xM do in u-boot + linux)

> + @classmethod
> + def get_sfdisk_cmd(cls, should_align_boot_part=False):
> + # Create a fixed-offset bootloader data at
> + # the beginning of the image (size is 214080 512 byte sectors
> + # with the first sector for MBR).
> + loader_start, loader_end, loader_len = align_partition(
> + 1, 214080, 1, PART_ALIGN_S)
> +
> + boot_start, boot_end, boot_len = align_partition(
> + loader_end + 1, BOOT_MIN_SIZE_S, PART_ALIGN_S, PART_ALIGN_S)
> +
> + # we ignore _root_end / _root_len and return a sfdisk command to
> + # instruct the use of all remaining space; XXX if we had some root size
> + # config, we could do something more sensible
> + root_start, _root_end, _root_len = align_partition(
> + boot_end + 1, ROOT_MIN_SIZE_S, PART_ALIGN_S, PART_ALIGN_S)
> +
> + return '%s,%s,0xDA\n%s,%s,0x0C,*\n%s,,,-' % (
> + loader_start, loader_len, boot_start, boot_len, root_start)

you're creating this partition layout:
           <bootloader>
           <boot>
           <root>
but I understand that the layout of data on the MMC is essentially:
+0 <partition table>
various stuff at fixed offsets:
+1 u-boot
+X kernel
+Y initrd
+4MiB <rootfs>

It would seem more sensible to me to have just a bootloader partition
and a rootfs partition (as long as your u-boot can't read from a vfat):
<bootloader>
<root>

The bootloader partition would have to be as big as the initrd end
offset.

I'm overall still unhappy that this is not using a vfat partition and
loading kernel and initrd from it, as well as boot script, I understand
this is just an u-boot bug. Which bug id is tracking this issue? I'd
like to make sure that it's not swept under the carpet :-)

> + @classmethod
> + def _make_boot_files(cls, uboot_parts_dir, boot_cmd, chroot_dir,
> + boot_dir, boot_script, boot_device_or_file):
> + uboot_file = os.path.join(
> + chroot_dir, 'usr', 'lib', 'u-boot', 'smdkv310', 'u-boot.v310')

I know it's a weird question, but why is the file suffix u-boot.v310?
Is it a special format that only the v310 supports?

> + install_smdkv310_boot_loader(uboot_file, boot_device_or_file)
> +
> + env_file = os.path.join(boot_dir, 'boot_env.flash')
> + # SMDK v310 uses a 16K environment
> + make_flashable_env(cls.boot_env, 16384, env_file)
> + install_smdkv310_boot_env(env_file, boot_device_or_file)

the 16384 KiB are duplicated across:
a) partition size constraints
b) in this make_flashable_env() call
c) in install_smdkv310_boot_env() which has count=32
please use some constants to compute partition offsets, dd args etc.
from the same value

e.g.:
SMDK_V310_ENV_SECTORS = 32
KERNEL_OFFSET_SECTORS = X
KERNEL_SIZE_SECTORS = X2
INITRD_OFFSET_SECTORS = Y
INITRD__SIZE_SECTORS = Y2
assert Y > X+X2

and then make sure you align_partition() above Y+Y2 so that the rootfs
is guaranteed to start at the earliest aligned location after the
initrd

> +class SMDKV310Config(SamsungConfig):
> + extra_serial_opts = 'console=ttySAC1,115200'

Guilherme mentioned that ttySAC1 should be a serialtty var instead,
like on the other boards

> + live_serial_opts = 'serialtty=ttyO2'

Does your board really have a ttyO2?

> + kernel_suffix = 's5pv310'

This kernel suffix is a bit suspicious; will you build a kernel which
only works on s5pv310? Did you consider building one which works on
multiple s5 SoCs? For instance, OMAP kernels can be built for OMAP2,
OMAP3 and OMAP4 in the same binary kernel which is just called -omap.
Linaro builds a -omap for all v7+ OMAPs, so OMAP3 and OMAP4 (and maybe
OMAP5 some day). We should be advocating this for Samsung SoCs as
well.

> + boot_script = 'boot.scr'
> + extra_boot_args_options = (
> + 'root=/dev/mmcblk0p2 rootwait rw init=/bin/bash')

root=/dev/mmcblk0p2 is incorrect; we're using UUID= on all other boards
which is resolved by the UUID

init=/bin/bash is also incorrect; you might have issues booting a
regular userspace, but it seems really broken to encode these in the
image writing tool

also, don't ever hardcode /dev/mmcblk0p2 as we have:
> + mmc_part_offset = 1

> +def make_flashable_env(boot_env, env_size, env_file):

we need some test for this function, and for the other two functions
added by this patch

> + proc = cmd_runner.run([
> + "mv",
> + "%s" % tmpfile,
> + "%s" % env_file], as_root=True)
> + proc.wait()

Why move the file in place instead of generating it directly at the
right location?

> +def install_smdkv310_uImage(uImage_file, boot_device_or_file):
> + # seek offset of 9281 is the MBR + BL1 + u-boot env + u-bbot
> + cmd = [
> + "dd",
> + "if=%s" % uImage_file,
> + "of=%s" % boot_device_or_file,
> + "bs=512",
> + "seek=1089",
> + "conv=notrunc"]

this seek= should be computed via constants

make_flashable_env(), install_smdkv310_boot_env(),
install_smdkv310_uImage() are only called once from _make_boot_files();
maybe it would make more sense to have a single v310 specific functions
which does all these things? It would also ease testing

> +def install_smdkv310_initrd(initrd_file, boot_device_or_file):
> + # seek offset of 9281 is the MBR + BL1 + u-boot env + u-bbot + uImage
> + proc = cmd_runner.run([
> + "dd",
> + "if=%s" % initrd_file,
> + "of=%s" % boot_device_or_file,
> + "bs=512",
> + "seek=9281",

Again, seek= shouldn't be hardcoded

> +def install_smdkv310_boot_env(env_file, boot_device_or_file):
> + # seek offset of 65 is the MBR + BL1 + u-boot env
> + proc = cmd_runner.run([
> + "dd",
> + "if=%s" % env_file,
> + "of=%s" % boot_device_or_file,
> + "bs=512",
> + "seek=33",
> + "count=32",

same remark with seek=, count=...

You could also introduce a dd() function for all these dd cmd runs

> +def install_smdkv310_boot_loader(v310_file, boot_device_or_file):
> + # seek offset of 1 preserves the MBR
> + proc = cmd_runner.run([
> + "dd",
> + "if=%s" % v310_file,
> + "of=%s" % boot_device_or_file,
> + "bs=512",
> + "seek=1",
> + "count=32",

ditto

> + # seek offset of 65 is the MBR + BL2 + u-boot env
> + proc = cmd_runner.run([
> + "dd",
> + "if=%s" % v310_file,
> + "of=%s" % boot_device_or_file,
> + "bs=512",
> + "seek=65",
> + "skip=64",

ditto

> - if should_format_bootfs:
> + if should_format_bootfs and board_config.uses_fat_boot_partition:

I think the flag should really be uses_boot_partition; you don't really
use a boot fs at all, it's just bootloader reserved space (concept of
the bootloader partition for mx5 boards)

Overall, the main things I'd like to see:
* u-boot / linux issues tracked as bugs and fixed in u-boot / linux
* no boot partition at all if you can't support it, just bootloader
   partition
* no duplication of values which are dependent on each other like
   rootfs partition offset, initrd size and offset, kernel size and
   offset, u-boot size and offset, u-boot env size and offset...
* no duplication of root= console= data, should use UUID and mmc
   numbering from linaro-image-tools

--
Loïc Minier

So overall, the diff is much smaller and it's getting closer to being
 mergeable, thanks for your work; some comments below

> === modified file 'linaro_media_create/boards.py'
> --- linaro_media_create/boards.py	2011-02-14 16:26:08 +0000
> +++ linaro_media_create/boards.py	2011-02-17 21:51:59 +0000
> @@ -419,6 +422,67 @@
>              cls.load_addr, uboot_parts_dir, cls.kernel_suffix, boot_dir)
>          make_uInitrd(uboot_parts_dir, cls.kernel_suffix, boot_dir)
>  
> +class SamsungConfig(BoardConfig):
> +    boot_env = [
> +        'bootargs=root=/dev/mmcblk0p3 rootwait rw init=/bin/bash console=ttySAC1,115200',
> +        'bootcmd=movi read kernel 40007000; movi read rootfs 41000000 600000;'
> +        'bootm 40007000 41000000',
> +        'ethact=smc911x-0',
> +        'ethaddr=00:40:5c:26:0a:5b',
> +        ]
> +

Why do we have root=/dev/mmcblk0p3 here and root=/dev/mmcblk0p2 in
 extra_boot_args_options?  Both seem incorrect as we use UUID for
 booting.  Same remark for init=/bin/bash.

console= should be inferred from a serialtty variable.

I don't think we should encode the default u-boot environment into
 linaro-image-tools, but rather into u-boot.  So things like
 ethact=smc911x-0 should go into u-boot's config for the board.

ethaddr= seems broken; this should be read from an EEPROM or should be
 generated by u-boot or by the kernel once, and recorded (see what IGEP
 or beagle xM do in u-boot + linux)

> +    @classmethod
> +    def get_sfdisk_cmd(cls, should_align_boot_part=False):
> +        # Create a fixed-offset bootloader data at
> +        # the beginning of the image (size is 214080 512 byte sectors
> +        # with the first sector for MBR). 
> +        loader_start, loader_end, loader_len = align_partition(
> +            1, 214080, 1, PART_ALIGN_S)
> +
> +        boot_start, boot_end, boot_len = align_partition(
> +            loader_end + 1, BOOT_MIN_SIZE_S, PART_ALIGN_S, PART_ALIGN_S)
> +
> +        # we ignore _root_end / _root_len and return a sfdisk command to
> +        # instruct the use of all remaining space; XXX if we had some root size
> +        # config, we could do something more sensible
> +        root_start, _root_end, _root_len = align_partition(
> +            boot_end + 1, ROOT_MIN_SIZE_S, PART_ALIGN_S, PART_ALIGN_S)
> +
> +        return '%s,%s,0xDA\n%s,%s,0x0C,*\n%s,,,-' % (
> +            loader_start, loader_len, boot_start, boot_len, root_start)

you're creating this partition layout:
           <bootloader>
           <boot>
           <root>
 but I understand that the layout of data on the MMC is essentially:
 +0        <partition table>
 various stuff at fixed offsets:
 +1        u-boot
 +X        kernel
 +Y        initrd
 +4MiB     <rootfs>

It would seem more sensible to me to have just a bootloader partition
 and a rootfs partition (as long as your u-boot can't read from a vfat):
           <bootloader>
           <root>

The bootloader partition would have to be as big as the initrd end
 offset.

I'm overall still unhappy that this is not using a vfat partition and
 loading kernel and initrd from it, as well as boot script, I understand
 this is just an u-boot bug.  Which bug id is tracking this issue?  I'd
 like to make sure that it's not swept under the carpet  :-)

> +    @classmethod
> +    def _make_boot_files(cls, uboot_parts_dir, boot_cmd, chroot_dir,
> +                         boot_dir, boot_script, boot_device_or_file):
> +        uboot_file = os.path.join(
> +            chroot_dir, 'usr', 'lib', 'u-boot', 'smdkv310', 'u-boot.v310')

I know it's a weird question, but why is the file suffix u-boot.v310?
 Is it a special format that only the v310 supports?

> +        install_smdkv310_boot_loader(uboot_file, boot_device_or_file)
> +
> +        env_file = os.path.join(boot_dir, 'boot_env.flash')
> +        # SMDK v310 uses a 16K environment
> +        make_flashable_env(cls.boot_env, 16384, env_file)
> +        install_smdkv310_boot_env(env_file, boot_device_or_file)

the 16384 KiB are duplicated across:
 a) partition size constraints
 b) in this make_flashable_env() call
 c) in install_smdkv310_boot_env() which has count=32
 please use some constants to compute partition offsets, dd args etc.
 from the same value

e.g.:
 SMDK_V310_ENV_SECTORS = 32
 KERNEL_OFFSET_SECTORS = X
 KERNEL_SIZE_SECTORS = X2
 INITRD_OFFSET_SECTORS = Y
 INITRD__SIZE_SECTORS = Y2
 assert Y > X+X2

and then make sure you align_partition() above Y+Y2 so that the rootfs
 is guaranteed to start at the earliest aligned location after the
 initrd

> +class SMDKV310Config(SamsungConfig):
> +    extra_serial_opts = 'console=ttySAC1,115200'

Guilherme mentioned that ttySAC1 should be a serialtty var instead,
 like on the other boards

> +    live_serial_opts = 'serialtty=ttyO2'

Does your board really have a ttyO2?

> +    kernel_suffix = 's5pv310'

This kernel suffix is a bit suspicious; will you build a kernel which
 only works on s5pv310?  Did you consider building one which works on
 multiple s5 SoCs?  For instance, OMAP kernels can be built for OMAP2,
 OMAP3 and OMAP4 in the same binary kernel which is just called -omap.
 Linaro builds a -omap for all v7+ OMAPs, so OMAP3 and OMAP4 (and maybe
 OMAP5 some day).  We should be advocating this for Samsung SoCs as
 well.

> +    boot_script = 'boot.scr'
> +    extra_boot_args_options = (
> +        'root=/dev/mmcblk0p2 rootwait rw init=/bin/bash')

root=/dev/mmcblk0p2 is incorrect; we're using UUID= on all other boards
 which is resolved by the UUID

init=/bin/bash is also incorrect; you might have issues booting a
 regular userspace, but it seems really broken to encode these in the
 image writing tool

also, don't ever hardcode /dev/mmcblk0p2 as we have:
> +    mmc_part_offset = 1

> +def make_flashable_env(boot_env, env_size, env_file):

we need some test for this function, and for the other two functions
 added by this patch

> +    proc = cmd_runner.run([
> +        "mv",
> +        "%s" % tmpfile,
> +        "%s" % env_file], as_root=True)
> +    proc.wait()

Why move the file in place instead of generating it directly at the
 right location?

> +def install_smdkv310_uImage(uImage_file, boot_device_or_file):
> +    # seek offset of 9281 is the MBR + BL1 + u-boot env + u-bbot
> +    cmd = [
> +        "dd",
> +        "if=%s" % uImage_file,
> +        "of=%s" % boot_device_or_file,
> +        "bs=512",
> +        "seek=1089",
> +        "conv=notrunc"]

this seek= should be computed via constants

make_flashable_env(), install_smdkv310_boot_env(),
 install_smdkv310_uImage() are only called once from _make_boot_files();
 maybe it would make more sense to have a single v310 specific functions
 which does all these things?  It would also ease testing

> +def install_smdkv310_initrd(initrd_file, boot_device_or_file):
> +    # seek offset of 9281 is the MBR + BL1 + u-boot env + u-bbot + uImage
> +    proc = cmd_runner.run([
> +        "dd",
> +        "if=%s" % initrd_file,
> +        "of=%s" % boot_device_or_file,
> +        "bs=512",
> +        "seek=9281",

Again, seek= shouldn't be hardcoded

> +def install_smdkv310_boot_env(env_file, boot_device_or_file):
> +    # seek offset of 65 is the MBR + BL1 + u-boot env
> +    proc = cmd_runner.run([
> +        "dd",
> +        "if=%s" % env_file,
> +        "of=%s" % boot_device_or_file,
> +        "bs=512",
> +        "seek=33",
> +        "count=32",

same remark with seek=, count=...

You could also introduce a dd() function for all these dd cmd runs

> +def install_smdkv310_boot_loader(v310_file, boot_device_or_file):
> +    # seek offset of 1 preserves the MBR
> +    proc = cmd_runner.run([
> +        "dd",
> +        "if=%s" % v310_file,
> +        "of=%s" % boot_device_or_file,
> +        "bs=512",
> +        "seek=1",
> +        "count=32",

ditto

> +    # seek offset of 65 is the MBR + BL2 + u-boot env
> +    proc = cmd_runner.run([
> +        "dd",
> +        "if=%s" % v310_file,
> +        "of=%s" % boot_device_or_file,
> +        "bs=512",
> +        "seek=65",
> +        "skip=64",

ditto

> -    if should_format_bootfs:
> +    if should_format_bootfs and board_config.uses_fat_boot_partition:

I think the flag should really be uses_boot_partition; you don't really
 use a boot fs at all, it's just bootloader reserved space (concept of
 the bootloader partition for mx5 boards)

Overall, the main things I'd like to see:
 * u-boot / linux issues tracked as bugs and fixed in u-boot / linux
 * no boot partition at all if you can't support it, just bootloader
   partition
 * no duplication of values which are dependent on each other like
   rootfs partition offset, initrd size and offset, kernel size and
   offset, u-boot size and offset, u-boot env size and offset...
 * no duplication of root= console= data, should use UUID and mmc
   numbering from linaro-image-tools

-- 
Loïc Minier

« Back to merge proposal