Merge lp:~berolinux/linaro-image-tools/android-iMX53 into lp:linaro-image-tools/11.11

[Bernhard Rosenkraenzer]

Better support for Android on iMX53 -- the boot loader needs its own place at 0x400, and should be installed there.

[James Westby (james-w)]

review: Approve

[James Westby (james-w)]

I'll merge this, but I'll have to rebase on top of trunk to avoid the l-a-m-c fix change.

Thanks,

James

[Guilherme Salgado (salgado)]

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_pa...

[James Westby (james-w)]

On Tue, 26 Jul 2011 15:22:38 -0000, Guilherme Salgado <email address hidden> wrote:
> > - 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.

This was because the branch was based on his fix for that problem, and
it wasn't set as a prerequisite.

I think that we should get answers for the other questions and decide:

  1) Is what I merged unsuitable for this release?
  2) Should anything be changed for next release to have a better
  codebase?

Thanks,

James

[Bernhard Rosenkraenzer (berolinux)]

Hi, here's the answers to your questions:

>> @@ -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.

Yes, it was necessary in Saturday's build because the other changes
broke l-a-m-c completely. Probably obsoleted by the rebase, will check
if it's still needed w/ current trunk

>>      @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?

Probably populate_raw_partition can be adapted to do the right thing instead.

>> --- 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?

Actually it is related to the rest of the changes and is vital to avoid a crash.
The patch removes the mmc_part_offset=0 line, and mmc_part_offset is
set to 1 in Mx5Config (and in turn Mx53Config and Mx53LoCoConfig).

[Guilherme Salgado (salgado)]

On Tue, 2011-07-26 at 19:24 +0000, Bernhard Rosenkraenzer wrote:
[...]
> >> @@ -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?
>
> Probably populate_raw_partition can be adapted to do the right thing instead.

Cool, would you give that a try?

>
> >> --- 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?
>
> Actually it is related to the rest of the changes and is vital to avoid a crash.
> The patch removes the mmc_part_offset=0 line, and mmc_part_offset is
> set to 1 in Mx5Config (and in turn Mx53Config and Mx53LoCoConfig).

Oh, right, I didn't notice that Mx5Config sets mmc_part_offset to 1 so I
thought it'd have the default value from BoardConfig (which is 0) here.

=== modified file 'linaro-android-media-create'
--- linaro-android-media-create	2011-06-30 12:09:46 +0000
+++ linaro-android-media-create	2011-07-25 00:21:08 +0000
@@ -142,6 +142,7 @@
     board_config.populate_raw_partition(args.device, BOOT_DIR)
     populate_partition(BOOT_DIR + "/boot", BOOT_DISK, boot_partition)
     board_config.populate_boot_script(boot_partition, BOOT_DISK, args.consoles)
+    board_config.install_boot_loader(args.device, BOOT_DISK)
     populate_partition(SYSTEM_DIR + "/system", SYSTEM_DISK, system_partition)
     populate_partition(DATA_DIR + "/data", DATA_DISK, data_partition)
     print "Done creating Linaro Android image on %s" % args.device

=== 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
 
     @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
 
 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)
     data_partition = _get_device_file_for_partition_number(
         media.path, 5 + board_config.mmc_part_offset)
     sdcard_partition = _get_device_file_for_partition_number(