Merge lp:~mabac/linaro-image-tools/snowball-support into lp:linaro-image-tools/11.11

[Mattias Backman]

Hi,

This branch is is based on the initial Snowball support written by Tony Månsson and Hans Odeberg @ STE. I have made as non-intrusive changes as possible while taking in the information posted in several previous merge proposals:
    https://code.launchpad.net/~tony-mansson/linaro-image-tools/snowball-support/+merge/60924
    https://code.launchpad.net/~mabac/linaro-image-tools/block-mmc-option-for-snowball/+merge/61121
    https://code.launchpad.net/~mabac/linaro-image-tools/snowball-data-in-file/+merge/61165

This change introduces two board configurations, namely:

    SnowballSdConfig, which is used to create partitions for an mmc. That is the usual boot and rootfs partitions.

    SnowballEmmcConfig, which is used to create the partitions for the internal Snowball EMMC. In addition to boot and rootfs there is a boot loader partition which can only reside on the EMMC.

There is still some work to be done before this should be merged:
    Assert that TOC does not overflow hardcoded boundary of 512 bytes. DONE
    Other checks to secure that files are written in a controlled manner.
    Write unit tests for the new code.

Thanks,

Mattias

[Mattias Backman (mabac)]

There should be a hwpack at https://github.com/Winterland/linaro-hwpack which can be combined with https://github.com/Winterland/startup to build a new .deb to test with. I get only 404 though, possible lacking permissions.

[Hans Odeberg (hans-odeberg)]

"Move functions install_snowball_boot_loader, create_toc and get_file_info into SnowballImageConfig since they are specific to Snowball."
Did you rerun the unit tests after this? I had to move these functions to a global scope when adding unit tests, as otherwise the test framework would not stub them, and they would try to install non-existing files.

[Mattias Backman (mabac)]

On Tue, May 17, 2011 at 10:01 AM, Hans Odeberg
<email address hidden> wrote:
> "Move functions install_snowball_boot_loader, create_toc and get_file_info into SnowballImageConfig since they are specific to Snowball."
> Did you rerun the unit tests after this? I had to move these functions to a global scope when adding unit tests, as otherwise the test framework would not stub them, and they would try to install non-existing files.

Yes, we need to create other ways to test these functions, possibly by
explicitly mocking them. Imo they belong to the SnowballConfig class
and if they do, they shouldn't be global just for the sake of testing.

[Guilherme Salgado (salgado)]

Yeah, we should have some unit tests for get_file_info(), create_toc() and install_snowball_boot_loader() as they're not tested even indirectly. But we also need to change test_snowball_image_steps to mock those 3 functions (now methods) using the same logger double that the testcase class uses to mock the other module-level functions.

[Hans Odeberg (hans-odeberg)]

OK, I will put writing the additional unit tests on my desk. If I cannot figure out how to do the mocking, I will get back to you with the hope you don't mock me...

[Guilherme Salgado (salgado)]

On Wed, 2011-05-18 at 08:48 +0000, Hans Odeberg wrote:
> OK, I will put writing the additional unit tests on my desk. If I cannot figure out how to do the mocking, I will get back to you with the hope you don't mock me...

I certainly wouldn't do that, and I'd be happy to work with you on
writing those tests (maybe using http://pad.ubuntu.com/ to collaborate)
if Mattias doesn't beat us to it :)

[Loïc Minier (lool)]

Looks good to me; the linaro-media-create flag is missing as well.

I think it would be ok to merge with:
- the flag to set the file implemented
- a note in the code to add the sanity checks
- a bug to remind ourselves to add tests
- a change to hwpackv2 spec to support this stuff

[Mattias Backman (mabac)]

> Looks good to me; the linaro-media-create flag is missing as well.
>
> I think it would be ok to merge with:
> - the flag to set the file implemented

Actually I think we realized that the file that describes the "startup files" has to be in the hwpack. This discussion might have been f2f or in STE email, there's been a lot of parallell threads so I forgot about that.

The reason for keeping the description file with the hwpack is that the hwpack contains the startup files and whoever creates the hwpack can make sure that the description file matches the contents.

This would make it possible to skip the flag to linaro-media-create since the description file can have a fixed file name. Of course I need to make it relative to wherever the file would be unpacked from the hwpack.

> - a note in the code to add the sanity checks
> - a bug to remind ourselves to add tests
> - a change to hwpackv2 spec to support this stuff

Sure, that's a good idea.

Note to self: remove snowball_toc.txt from the tree before merging.

[Hans Odeberg (hans-odeberg)]

Looks good to me, so far (with the limitations on tests and sanity checks you mentioned above).

Just a question on SNOWBALL_STARTUP_FILES_CONFIG. Right now, it identifies a file in the lmc install directory, and the patch also contains such a file. Do you want the file to be there for now, removing it when the file is included in a hwpack?

[Mattias Backman (mabac)]

On Thu, May 19, 2011 at 6:11 PM, Hans Odeberg
<email address hidden> wrote:
> Looks good to me, so far (with the limitations on tests and sanity checks you mentioned above).
>
> Just a question on SNOWBALL_STARTUP_FILES_CONFIG. Right now, it identifies a file in the lmc install directory, and the patch also contains such a file. Do you want the file to be there for now, removing it when the file is included in a hwpack?

Yes, I just added that as an example of the format I expect the file
to be in. But it will be removed from the source tree before the
change is ready. I would like to have a complete hwpack with the file
in it for testing before I'm satisfied with this.

[Mattias Backman (mabac)]

> I think it would be ok to merge with:
> - the flag to set the file implemented

It's assumed that the file is packaged with the startup files in the hwpack instead. Is that ok?

> - a note in the code to add the sanity checks

Done.

> - a bug to remind ourselves to add tests
> - a change to hwpackv2 spec to support this stuff

I'll take care of these today.

[Mattias Backman (mabac)]

I have tested this by creating a "raw image" for the board like this:

    ./linaro-media-create --dev snowball_emmc --binary linaro-natty-nano-tar-20110516-0.tar.gz --hwpack hwpack_linaro-snowball_0.3_armel_unsupported.tar.gz --rootfs ext3 --image_file snowball.bin

and writing the image to the board like this:

    sudo riff -f snowball.bin

The board boots to prompt with the nano image. I hope STE (Tony?) will verify this more thoroughly while reviewing.

Note that the riff debs where taken from an internal share at STE and the hwpack was taken from Tony's network share. Can I redistribute these within Linaro? The hwpack does not contain the startfiles.cfg file yet btw.

Thanks,

Mattias

[Loïc Minier (lool)]

On Fri, May 20, 2011, Mattias Backman wrote:
> It's assumed that the file is packaged with the startup files in the
> hwpack instead. Is that ok?

 Yup, that's also ok

--
Loïc Minier

[Loïc Minier (lool)]

Branch looks good to me; since you removed the sample toc file and since I don't think we have support in linaro-hwpack-create for TOCs, I would suggest you add some explanation under doc/ on how the toc file and on repacking hwpacks with such a file; e.g. a doc/snowball-emmc-hwpacks.txt or something like that which would say something like:
===
hwpacks for use with --dev snowball-emmc need a file called toc.cfg at the root of the tarball, but this isn't yet supported by linaro-hwpack-create; repack the hwpack by unpacking the tarball, adding the toc file and the files it references at the root of the tarball and repack the tarball. The TOC file has this format:
<sample TOC file>
===

or file a bug to remind ourselves about this lack of support for toc files in linaro-hwpack-create or lack of documentation on working around it

[Loïc Minier (lool)]

review: Approve

[Hans Odeberg (hans-odeberg)]

> Branch looks good to me; since you removed the sample toc file and since I
> don't think we have support in linaro-hwpack-create for TOCs, I would suggest
> you add some explanation under doc/ on how the toc file and on repacking
> hwpacks with such a file; e.g. a doc/snowball-emmc-hwpacks.txt or something
> like that which would say something like:
> ===
> hwpacks for use with --dev snowball-emmc need a file called toc.cfg at the
> root of the tarball, but this isn't yet supported by linaro-hwpack-create;
> repack the hwpack by unpacking the tarball, adding the toc file and the files
> it references at the root of the tarball and repack the tarball. The TOC file
> has this format:
> <sample TOC file>
> ===
>
> or file a bug to remind ourselves about this lack of support for toc files in
> linaro-hwpack-create or lack of documentation on working around it

Do we need to add support for toc files in linaro-hwpack-create? The fewer Snowball-specific changes, the better. I would prefer if the toc file was simply just another Snowball-specific file packaged inside a deb-package in the hw pack, just the way other files are packaged. Then linaro-media-create just unpacks the hw pack, and the file ends up in the /boot directory. Could this work?

[Mattias Backman (mabac)]

About hwpack-create; I'll add the documentation if needed of course. I can see that the STE binaries are included in the hwpack as a deb package: startupfiles_0.3-0ubuntu1_armel.deb and I guess that's included with the local-debs option. So if the cfg file is in there along with the other files, it would solve the problem?

startupfiles_0.3-0ubuntu1_armel.deb:
    boot/
         boot_image_issw.bin
         boot_image_x-loader.bin
         mem_init.bin
         power_management.bin
         u-boot.bin
         u-boot-env.bin
...

[Hans Odeberg (hans-odeberg)]

review: Approve

[Tony Mansson (tony-mansson)]

I am OK with this as "good enough". For future enhancement I'd like to note:

1) The bootloader environment in 'bootargs' and 'bootcmd' are never used currently, as we have a separate u-boot-env.bin file with these. I am unsure of where LMC puts its hard-coded data and I don't see the advantage of having it here. It does however no harm other than confuse. OK?

2) For the final version we need to have two separate packages containing startup files as the licenses are different (u-boot is GPL and the others are proprietary). as I understand this implementation there is only room for one SNOWBALL_STARTUP_FILES_CONFIG and this will then have to describe the contents of both its own plus an other package which is not pretty. A general solution would e.g loop over all packages and find one cfg file for each package. The current solution is however legally OK and works for now.

review: Approve

[Mattias Backman (mabac)]

> I am OK with this as "good enough". For future enhancement I'd like to note:

Great, thank you.

>
> 1) The bootloader environment in 'bootargs' and 'bootcmd' are never used
> currently, as we have a separate u-boot-env.bin file with these. I am unsure
> of where LMC puts its hard-coded data and I don't see the advantage of having
> it here. It does however no harm other than confuse. OK?

I think we can handle that in the hwpack v2 work which is about taking away the hard coded data and let hwpacks handle it all. I'm going to start expanding this spec next week to take into account the discussions we had at LDS.
    https://wiki.linaro.org/Platform/Specs/11.11/HardwarePacksV2

>
> 2) For the final version we need to have two separate packages containing
> startup files as the licenses are different (u-boot is GPL and the others are
> proprietary). as I understand this implementation there is only room for one
> SNOWBALL_STARTUP_FILES_CONFIG and this will then have to describe the contents
> of both its own plus an other package which is not pretty. A general solution
> would e.g loop over all packages and find one cfg file for each package. The
> current solution is however legally OK and works for now.

Then we'll go with this if it works for you and let's change it as soon as that new packaging scheme is fixed.

[Hans Odeberg (hans-odeberg)]

> 2) For the final version we need to have two separate packages containing
> startup files as the licenses are different (u-boot is GPL and the others are
> proprietary). as I understand this implementation there is only room for one
> SNOWBALL_STARTUP_FILES_CONFIG and this will then have to describe the contents
> of both its own plus an other package which is not pretty. A general solution
> would e.g loop over all packages and find one cfg file for each package. The
> current solution is however legally OK and works for now.

My opinion is that the HW pack should be responsible for placing one configuration file in the /boot directory. This could be done by:
1) Putting the config file in the startup_files package. Simple, but ugly, as the file will then refer to a file in another package.
2) Putting the config file in a separate package, which then has dependencies on a specific version of the startup file and u-boot packages. Sort of like how a devel package depends on a specific version of a binary package.
3) Putting one config file fragment in each of the startup_files and u-boot packages, have the u-boot package depend on the startup_files package, and then include an install script in the u-boot package which combines the two fragments into a config file.

I'm sure more complicated schemes could be invented, too. The end result should however look the same to LMC: a config file in the /boot directory.

=== modified file 'linaro_image_tools/media_create/boards.py'
--- linaro_image_tools/media_create/boards.py	2011-04-26 16:37:19 +0000
+++ linaro_image_tools/media_create/boards.py	2011-05-20 08:27:14 +0000
@@ -495,6 +495,150 @@
         make_boot_script(boot_env, boot_script_path)
 
 
+class SnowballSdConfig(Ux500Config):
+    '''Use only with --mmc option. Creates the standard vfat and ext2
+       partitions for kernel and rootfs on an SD card.
+       Note that the Snowball board needs a loader partition on the
+       internal eMMC flash to boot. That partition is created with
+       the SnowballConfigImage configuration.'''
+
+    @classmethod
+    def _make_boot_files(cls, boot_env, chroot_dir, boot_dir,
+                         boot_device_or_file, k_img_data, i_img_data,
+                         d_img_data):
+        make_uImage(cls.load_addr, k_img_data, boot_dir)
+        boot_script_path = os.path.join(boot_dir, cls.boot_script)
+        make_boot_script(boot_env, boot_script_path)
+
+
+class SnowballEmmcConfig(SnowballSdConfig):
+    '''Use only with --image option. Creates a raw image which contains an
+       additional (raw) loader partition, containing some boot stages
+       and u-boot.'''
+    # Boot ROM looks for a boot table of contents (TOC) at 0x20000
+    # Actually, it first looks at address 0, but that's where l-m-c
+    # puts the MBR, so the boot loader skips that address. 
+    SNOWBALL_LOADER_START_S = (128 * 1024) / SECTOR_SIZE
+    SNOWBALL_STARTUP_FILES_CONFIG = 'startfiles.cfg'
+    TOC_SIZE = 512
+
+    @classmethod
+    def get_sfdisk_cmd(cls, should_align_boot_part=None):
+        """Return the sfdisk command to partition the media.
+
+        :param should_align_boot_part: Ignored.
+
+        The Snowball partitioning scheme depends on whether the target is
+        a raw image or an SD card. Both targets have the normal
+        FAT 32 boot partition and EXT? root partition.
+        The raw image prepends these two partitions with a raw loader partition,
+        containing HW-dependent boot stages up to and including u-boot.
+        This is done since the boot rom always boots off the internal memory;
+        there simply is no point to having a loader partition on SD card.
+        """
+        # boot ROM expects bootloader at 0x20000, which is sector 0x100 
+        # with the usual SECTOR_SIZE of 0x200.
+        # (sector 0 is MBR / partition table)
+        loader_start, loader_end, loader_len = align_partition(
+            SnowballEmmcConfig.SNOWBALL_LOADER_START_S, 
+            LOADER_MIN_SIZE_S, 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 an 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)
+
+    @classmethod
+    def _make_boot_files(cls, boot_env, chroot_dir, boot_dir,
+                         boot_device_or_file, k_img_data, i_img_data,
+                         d_img_data):
+        make_uImage(cls.load_addr, k_img_data, boot_dir)
+        boot_script_path = os.path.join(boot_dir, cls.boot_script)
+        make_boot_script(boot_env, boot_script_path)
+        _, toc_filename = tempfile.mkstemp()
+        atexit.register(os.unlink, toc_filename)
+        config_files_path = os.path.join(chroot_dir, 'boot')
+        new_files = cls.get_file_info(config_files_path)
+        with open(toc_filename, 'wb') as toc:
+            cls.create_toc(toc, new_files)
+        cls.install_snowball_boot_loader(toc_filename, new_files,
+                                     boot_device_or_file,
+                                     cls.SNOWBALL_LOADER_START_S)
+
+    @classmethod
+    def install_snowball_boot_loader(cls, toc_file_name, files,
+                                     boot_device_or_file, start_sector):
+        ''' Copies TOC and boot files into the boot partition.
+        A sector size of 1 is used for some files, as they do not
+        necessarily start on an even address. '''
+        assert os.path.getsize(toc_file_name) <= cls.TOC_SIZE
+        _dd(toc_file_name, boot_device_or_file, seek=start_sector)
+
+        for file in files:
+            # XXX We need checks that these files do not overwrite each
+            # other. This code assumes that offset and file sizes are ok.
+            if (file['offset'] % SECTOR_SIZE) != 0:
+                seek_bytes = start_sector * SECTOR_SIZE + file['offset']
+                _dd(file['filename'], boot_device_or_file, block_size=1,
+                    seek=seek_bytes)
+            else:
+                seek_sectors = start_sector + file['offset']/SECTOR_SIZE
+                _dd(file['filename'], boot_device_or_file, seek=seek_sectors)
+
+    @classmethod
+    def create_toc(cls, f, files):
+        ''' Writes a table of contents of the boot binaries.
+        Boot rom searches this table to find the binaries.'''
+        for file in files:
+            # Format string means: < little endian,
+            # I; unsigned int; offset,
+            # I; unsigned int; size,
+            # I; unsigned int; flags,
+            # i; int; align,
+            # i; int; load_address,
+            # 12s; string of char; name
+            # http://igloocommunity.org/support/index.php/ConfigPartitionOverview
+            flags = 0
+            load_adress = file['align']
+            data = struct.pack('<IIIii12s', file['offset'], file['size'],
+                               flags, file['align'], load_adress,
+                               file['section_name'])
+            f.write(data)
+
+    @classmethod
+    def get_file_info(cls, bin_dir):
+        ''' Fills in the offsets of files that are located in
+        non-absolute memory locations depending on their sizes.'
+        Also fills in file sizes'''
+        ofs = cls.TOC_SIZE
+        files = []
+        with open(os.path.join(bin_dir, cls.SNOWBALL_STARTUP_FILES_CONFIG),
+                  'r') as info_file:
+            for line in info_file:
+                file_data = line.split()
+                if file_data[0][0] == '#':
+                    continue
+                filename = os.path.join(bin_dir, file_data[1])
+                address = long(file_data[3], 16)
+                if address != 0:
+                    ofs = address
+                size = os.path.getsize(filename)
+                files.append({'section_name': file_data[0],
+                              'filename': filename,
+                              'align': int(file_data[2]),
+                              'offset': ofs,
+                              'size': size,
+                              'load_adress': file_data[4]})
+                ofs += size
+        return files
+
+
 class Mx5Config(BoardConfig):
     serial_tty = 'ttymxc0'
     extra_serial_opts = 'console=tty0 console=%s,115200n8' % serial_tty
@@ -687,6 +831,8 @@
     'panda': PandaConfig,
     'vexpress': VexpressConfig,
     'ux500': Ux500Config,
+    'snowball_sd': SnowballSdConfig,
+    'snowball_emmc': SnowballEmmcConfig,
     'efikamx': EfikamxConfig,
     'efikasb': EfikasbConfig,
     'mx51evk': Mx51evkConfig,

=== modified file 'linaro_image_tools/media_create/tests/test_media_create.py'
--- linaro_image_tools/media_create/tests/test_media_create.py	2011-05-10 15:05:57 +0000
+++ linaro_image_tools/media_create/tests/test_media_create.py	2011-05-20 08:27:14 +0000
@@ -213,6 +213,11 @@
         expected = ['make_uImage', 'make_uInitrd', 'make_boot_script']
         self.assertEqual(expected, self.funcs_calls)
 
+    def test_snowball_sd_steps(self):
+        self.make_boot_files(boards.SnowballSdConfig)
+        expected = ['make_uImage', 'make_boot_script']
+        self.assertEqual(expected, self.funcs_calls)
+
     def test_panda_steps(self):
         self.mock_set_appropriate_serial_tty(boards.PandaConfig)
         self.make_boot_files(boards.PandaConfig)
@@ -330,6 +335,16 @@
             '1,8191,0xDA\n8192,106496,0x0C,*\n114688,,,-',
             boards.Mx5Config.get_sfdisk_cmd())
 
+    def test_snowball_sd(self):
+        self.assertEqual(
+            '63,106432,0x0C,*\n106496,,,-',
+            boards.SnowballSdConfig.get_sfdisk_cmd())
+
+    def test_snowball_emmc(self):
+        self.assertEqual(
+            '256,7936,0xDA\n8192,106496,0x0C,*\n114688,,,-',
+            boards.SnowballEmmcConfig.get_sfdisk_cmd())
+
     def test_smdkv310(self):
         self.assertEquals(
             '1,8191,0xDA\n8192,106496,0x0C,*\n114688,,,-',
@@ -393,6 +408,23 @@
                        'bootm 0x00100000 0x08000000'}
         self.assertEqual(expected, boot_commands)
 
+    def test_snowball_emmc(self):
+        boot_commands = board_configs['snowball_emmc']._get_boot_env(
+            is_live=False, is_lowmem=False, consoles=[],
+            rootfs_uuid="deadbeef", d_img_data=None)
+        expected = {
+            'bootargs': 'console=tty0 console=ttyAMA2,115200n8  '
+                        'root=UUID=deadbeef rootwait ro earlyprintk '
+                        'rootdelay=1 fixrtc nocompcache mem=96M@0 '
+                        'mem_modem=32M@96M mem=44M@128M pmem=22M@172M '
+                        'mem=30M@194M mem_mali=32M@224M pmem_hwb=54M@256M '
+                        'hwmem=48M@302M mem=152M@360M',
+            'bootcmd': 'fatload mmc 1:1 0x00100000 uImage; '
+                       'fatload mmc 1:1 0x08000000 uInitrd; '
+                       'bootm 0x00100000 0x08000000'}
+        self.assertEqual(expected, boot_commands)
+
+
     def test_panda(self):
         # XXX: To fix bug 697824 we have to change class attributes of our
         # OMAP board configs, and some tests do that so to make sure they