Code review comment for lp://qastaging/~berolinux/linaro-image-tools/android-iMX53

Hi Bernhard,

I have a few questions about your changes below.

On Mon, 2011-07-25 at 00:21 +0000, Bernhard Rosenkraenzer wrote:
> Bernhard Rosenkraenzer has proposed merging lp:~berolinux/linaro-image-tools/android-iMX53 into lp:linaro-image-tools.
>
> Requested reviews:
> linaro-image-tools maintainers (linaro-image-tools)
>
> For more details, see:
> https://code.launchpad.net/~berolinux/linaro-image-tools/android-iMX53/+merge/69018
>
> Better support for Android on iMX53 -- the boot loader needs its own place at 0x400, and should be installed there.

> === modified file 'linaro_image_tools/media_create/android_boards.py'
> --- linaro_image_tools/media_create/android_boards.py 2011-07-18 18:25:30 +0000
> +++ linaro_image_tools/media_create/android_boards.py 2011-07-25 00:21:08 +0000
> @@ -34,7 +34,8 @@
> from linaro_image_tools.media_create.boards import (
> align_up,
> align_partition,
> - make_boot_script
> + make_boot_script,
> + install_mx5_boot_loader,
> )
>
> from linaro_image_tools import cmd_runner
> @@ -77,10 +78,6 @@
>
> @classmethod
> def populate_boot_script(cls, boot_partition, boot_disk, consoles):
> - cmd_runner.run(['mkdir', '-p', boot_disk]).wait()
> - cmd_runner.run(['mount', boot_partition, boot_disk],
> - as_root=True).wait()
> -
> boot_env = cls._get_boot_env(consoles)
> cmdline_filepath = os.path.join(boot_disk, "cmdline")
> cmdline_file = open(cmdline_filepath, 'r')
> @@ -94,10 +91,6 @@
> make_boot_script(boot_env, boot_script_path)
>
> cmd_runner.run(['sync']).wait()
> - try:
> - cmd_runner.run(['umount', boot_disk], as_root=True).wait()
> - except cmd_runner.SubcommandNonZeroReturnValue:
> - pass

This change is no longer necessary and will actually break l-a-m-c as of
r388. You might want to use the new partition_mounted() context manager
here although that can be done later.

>
> @classmethod
> def get_sfdisk_cmd(cls, should_align_boot_part=False,
> @@ -158,6 +151,9 @@
> def populate_raw_partition(cls, media, boot_dir):
> super(AndroidBoardConfig, cls).populate_raw_partition(boot_dir, media)
>
> + @classmethod
> + def install_boot_loader(cls, boot_partition, boot_device_or_file):
> + pass

Do we need a new classmethod for this? ISTM that
populate_raw_partition(), which is already called in l-a-m-c, should be
the one used to populate a raw partition with the board-specific
bootloader instead, no?

>
> class AndroidOmapConfig(AndroidBoardConfig):
> pass
> @@ -217,7 +213,22 @@
> Mx53LoCoConfig.serial_tty)
> android_specific_args = 'init=/init androidboot.console=%s' % (
> Mx53LoCoConfig.serial_tty)
> - mmc_part_offset = 0
> +
> + @classmethod
> + def get_sfdisk_cmd(cls, should_align_boot_part=False):
> + loader_start, loader_end, loader_len = align_partition(
> + 1, cls.LOADER_MIN_SIZE_S, 1, PART_ALIGN_S)
> +
> + command = super(AndroidMx53LoCoConfig, cls).get_sfdisk_cmd(
> + should_align_boot_part=True, start_addr=loader_end,
> + extra_part=True)
> +
> + return '%s,%s,0xDA\n%s' % (
> + loader_start, loader_len, command)
> +
> + @classmethod
> + def install_boot_loader(cls, boot_partition, boot_device_or_file):
> + install_mx5_boot_loader(os.path.join(boot_device_or_file, "u-boot.bin"), boot_partition, cls.LOADER_MIN_SIZE_S)
>
> android_board_configs = {
> 'beagle': AndroidBeagleConfig,
>
> === modified file 'linaro_image_tools/media_create/partitions.py'
> --- linaro_image_tools/media_create/partitions.py 2011-06-29 09:16:39 +0000
> +++ linaro_image_tools/media_create/partitions.py 2011-07-25 00:21:08 +0000
> @@ -337,8 +337,14 @@
> media.path, 1 + board_config.mmc_part_offset)
> system_partition = _get_device_file_for_partition_number(
> media.path, 2 + board_config.mmc_part_offset)
> - cache_partition = _get_device_file_for_partition_number(
> - media.path, 3 + board_config.mmc_part_offset)
> + if board_config.mmc_part_offset != 1:
> + cache_partition = _get_device_file_for_partition_number(
> + media.path, 3 + board_config.mmc_part_offset)
> + else:
> + # In the current setup, partition 4 is always the
> + # extended partition container, so we need to skip 4
> + cache_partition = _get_device_file_for_partition_number(
> + media.path, 5)

I'm slightly confused by this change as the mmc_part_offset of
Mx53LoCoConfig is still 0, so it won't use the new code (in the else
block). Maybe this is not related to the rest of the changes in this
branch?

> data_partition = _get_device_file_for_partition_number(
> media.path, 5 + board_config.mmc_part_offset)
> sdcard_partition = _get_device_file_for_partition_number(
>

--
Guilherme Salgado <https://launchpad.net/~salgado>