Merge lp://qastaging/~lool/linaro-image-tools/efikamx into lp://qastaging/linaro-image-tools/11.11

On Thu, 2011-02-17 at 13:58 +0000, Loïc Minier wrote:
> Loïc Minier has proposed merging lp:~lool/linaro-image-tools/efikamx into lp:linaro-image-tools.
>
> Requested reviews:
> Linaro Maintainers (linaro-maintainers)
>
> For more details, see:
> https://code.launchpad.net/~lool/linaro-image-tools/efikamx/+merge/50151
>
> Relatively simple support for EfikaMX smarttops.
>
> After setting the board up to boot from MMC via DIP switches, it currently doesn't autoboot due to bug #720714, but can be booted with these commands at the u-boot boot prompt:
> mmc rescan
> mmc part 0
> fatload mmc 0:2 0x90000000 boot.scr
> source 0x90000000

IIUC this is a workaround for the bug you mentioned above and once the
bug is fixed we'll be able to boot without this?

>
>
> While working on this, I realized that install_omap_boot_loader() and install_mx5_boot_loader() aren't currently tested, nor are the concrete calls to these functions tested; I figured that this wasn't a regression and that we might drop most of these while moving to hardware pack v2, so I'm sending this in now. If you have suggestions on testing these, they are welcome :-)
>

We could have some simple tests that mock Popen() and check that it was
called with the correct arguments, similar to the make_uImage test in
TestPopulateBoot. The calls to these functions are already tested in
TestBootSteps, though.

> I've also came across a conflict with usage of "uboot_flavor" which has an omap-specific meaning due to being used in partitions.py; I wonder whether this should be moved to boards.py instead.

I'm not sure what you mean here, but uboot_flavor is not used in
partitions.py (I think you mean that it's used in populate_boot.py?).
Also, I've filed a bug recently about refactoring populate_boot.py and
moving it all to boards.py.

> differences between files attachment (review-diff.txt)
> === 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 13:58:01 +0000
> @@ -346,7 +346,7 @@
> make_boot_script(boot_cmd, boot_script)
>
>
> -class Mx51evkConfig(BoardConfig):
> +class Mx5Config(BoardConfig):
> serial_tty = 'ttymxc0'
> extra_serial_opts = 'console=tty0 console=%s,115200n8' % serial_tty
> live_serial_opts = 'serialtty=%s' % serial_tty
> @@ -390,14 +390,21 @@
> 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', 'mx51evk', 'u-boot.imx')
> - install_mx51evk_boot_loader(uboot_file, boot_device_or_file)
> + chroot_dir, 'usr', 'lib', 'u-boot', cls.uboot_name, 'u-boot.imx')
> + install_mx5_boot_loader(uboot_file, boot_device_or_file)
> make_uImage(
> cls.load_addr, uboot_parts_dir, cls.kernel_suffix, boot_dir)
> make_uInitrd(uboot_parts_dir, cls.kernel_suffix, boot_dir)
> make_boot_script(boot_cmd, boot_script)
>
>
> +class EfikamxConfig(Mx5Config):
> + uboot_name = 'efikamx'

Oh, I think I see what you mean now...

On Thu, Feb 17, 2011, Guilherme Salgado wrote:
> IIUC this is a workaround for the bug you mentioned above and once the
> bug is fixed we'll be able to boot without this?

 Correct

> We could have some simple tests that mock Popen() and check that it was
> called with the correct arguments, similar to the make_uImage test in
> TestPopulateBoot. The calls to these functions are already tested in
> TestBootSteps, though.

 For some reason, the cp of u-boot doesn't appear though

> > I've also came across a conflict with usage of "uboot_flavor" which
> > has an omap-specific meaning due to being used in partitions.py; I
> > wonder whether this should be moved to boards.py instead.
>
> I'm not sure what you mean here, but uboot_flavor is not used in
> partitions.py (I think you mean that it's used in populate_boot.py?).
> Also, I've filed a bug recently about refactoring populate_boot.py and
> moving it all to boards.py.

 Err sorry, I meant populate_boot.py, yes

> Oh, I think I see what you mean now. We use uboot_flavor in
> populate_boot to decide whether or not to copy u-boot.bin to the boot
> partition, and you don't want that to happen for mx5 boards?

 Correct

> I think
> having uboot_name in some configs and uboot_flavor in others is very
> confusing so I'd rather see if we can stick with just uboot_flavor and
> come out with another way of identifying the boards that should have
> u-boot.bin copied to the boot partition; maybe a new attribute on
> BoardConfig?

 Yeah; that's also how I feel

> > + # this is really about testing Mx5Config rather than Mx51evkConfig
> > create_partitions(
> > board_configs['mx51evk'], self.media, 255, 63, '')
> >
>
> In this case I think it'd make more sense to create a subclass of
> Mx5Config (as that's what we really want to test) here and define
> uboot_name there instead of using mx51evkconfig.

 Ok

--
Loïc Minier

On Thu, Feb 17, 2011, Loïc Minier wrote:
> > We could have some simple tests that mock Popen() and check that it was
> > called with the correct arguments, similar to the make_uImage test in
> > TestPopulateBoot. The calls to these functions are already tested in
> > TestBootSteps, though.
> For some reason, the cp of u-boot doesn't appear though

 The reason was that populate_boot assumed u-boot.bin when mx5 uses
 u-boot.imx

 I've push an updated branch with new tests for populate_boot() and
 addressing your comments; see r298

 I don't test install_omap_boot_loader() nor install_mx5_boot_loader();
 shouldn't be too hard

--
Loïc Minier

On Thu, Feb 24, 2011, Loïc Minier wrote:
> I don't test install_omap_boot_loader() nor install_mx5_boot_loader();
> shouldn't be too hard

 Pushed with r300

--
Loïc Minier

101 uboot_flavor = board_config.uboot_flavor
102 - if uboot_flavor is not None:
103 + if board_config.uboot_in_boot_part and uboot_flavor is not None:

I think it would make sense to assert that uboot_flavor is not None when uboot_in_boot_part is True.

Also, there's way too much duplication in TestPopulateBoot's methods. You could save the expected_calls list as a class variable and reuse it in all methods, and maybe have a populate_boot wrapper which takes just the config and is_live argument as these seem to be the only ones that vary.

Oh, and I was expecting test_populate_boot_mx5() to show the u-boot.imx being copied but that doesn't happen because you use boards.Mx5Config, which has uboot_flavor==None. I think you should create a new test config (subclassing Mx5Config) with uboot_flavor and use that instead.

On Fri, Feb 25, 2011, Guilherme Salgado wrote:
> Oh, and I was expecting test_populate_boot_mx5() to show the
> u-boot.imx being copied but that doesn't happen because you use
> boards.Mx5Config, which has uboot_flavor==None. I think you should
> create a new test config (subclassing Mx5Config) with uboot_flavor and
> use that instead.

 BTW test_populate_boot_mx5() is *not* supposed to copy u-boot; Mx5 only
 dd-s u-boot in place, but does not need it in the boot partition.

--
Loïc Minier

This email rejected by Launchpad, copy-pasting here:

On Fri, Feb 25, 2011, Guilherme Salgado wrote:
> Review: Approve
> 101 uboot_flavor = board_config.uboot_flavor
> 102 - if uboot_flavor is not None:
> 103 + if board_config.uboot_in_boot_part and uboot_flavor is not None:
>
> I think it would make sense to assert that uboot_flavor is not None
> when uboot_in_boot_part is True.

 Ok; IGEP was actually abusing this situation, but I agree it's cleaner
 to just enforce sanity.

> Also, there's way too much duplication in TestPopulateBoot's methods.
> You could save the expected_calls list as a class variable and reuse
> it in all methods, and maybe have a populate_boot wrapper which takes
> just the config and is_live argument as these seem to be the only ones
> that vary.

 Ack; I wasn't happy with this duplication and wanted some advice on
 avoiding it, thanks; I'm not sure the duplication can be avoided in
 more elegant ways?

> Oh, and I was expecting test_populate_boot_mx5() to show the
> u-boot.imx being copied but that doesn't happen because you use
> boards.Mx5Config, which has uboot_flavor==None. I think you should
> create a new test config (subclassing Mx5Config) with uboot_flavor and
> use that instead.

 I realized that I should probably not use specific board configs to
 exercize the various combinations, as these might change over time and
 not represent everything we want to test. I revamped the tests to
 construct specific test configs instead.

 This does mean that we miss testing of populate_boot() for all our
 board configs; I'm not sure whether we want this or not. I suspect we
 might want to group all the tests specific to a board in a class, and
 never use specific boards in the other tests, but maybe it's ok to just
 assume that boards.py tests can be board-specific?
   But these are thoughts for future improvements, I think we can safely
 merge efikamx support before we do this.