This is looking awesome, good job!  Some questions below

On Mon, Apr 11, 2011, Shawn Guo wrote:
> * I chose not to add any argument to lmc for differentiating the
>   boards which do not support dt yet from those which already do.
>   Instead, I use board properties 'dtb_addr' and 'dtb_name'.  With
>   this approach, all the changes will be limited in boards.py.  And
>   also considering that the current solution may be temporary and we
>   will ultimately support dtb by appending it to kernel image, it may
>   be reasonable to save one lmc argument for temporary dtb support.

 Does it make sense to make DT support optional for boards which support
 it?  I mean, if some thing are known not to work in a DT scenario, then
 we should allow people to disable DT even if it's present.  So are
 there any known regressions for boards with DT support?

> * Board 'igep' is being split into 'igepv2' and 'igepv3', as they get
>   different .dtb files.

 This is the right way to implement it, and I don't mind too much
 renaming "--dev igep" to "--dev igepv2", but do you actually have an
 IGEPv3 to test?  We will eventually add support for using u-boot-linaro
 with IGEPv2 (bug #716627), and we don't have an IGEPv3 build of
 u-boot-linaro as we have no way to test it.

> * Boards 'vexpress' and 'ux500' do not get supported, as there is no
>   dt support for them yet in kernel tree (no .dts for the boards).

 Probably something to discuss in some other place, but ARM has a DT
 support tree for their boards, so I'm pretty sure we can find a .dts
 for vexpress /somewhere/.  :-)  Lorenzo from ARM would know.

> * Though dt support for 'smdkv310' has been available in kernel tree,
>   I did not add the dtb support in lmc for it, because bug 727978
>   makes the board so unique in terms of installing and loading boot
>   files.  And I'm seeing some recent activity of addressing the bug,
>   so I would hold this board for a while.

 Ok; we should file a bug to track this as to not forget about it; there
 is indeed a code revamp of smdkv310 pending, and it's going to look
 very similar to mx51/mx53 boards.

> * Boards 'tegra-harmony', 'versatile-ab' and 'versatile-pb' have
>   already got dt support in kernel tree, but lmc does not support
>   them at all.

 Yup; versatile-ab/-pb wont be supported any time soon as we don't have
 any armv5 userspace to run with them, and tegra-harmony isn't really a
 priority because we don't use Nvidia hardware in Linaro ATM.

> * This branch can not be merged into trunk for working with daily
>   build hwpack before John Rigby gets .dtb files be there.

 Actually that's a bug: we still need to support images from the 10.11
 release, at least until the 11.05 release, so it would be good to
 support the absence of the DTB file.  It's a bit more fragile, but we
 don't have a good way to track whether a .dtb file is present in a
 hwpack or not, this will be the case with HardwarePackV2 when that's
 implemented though.

> -        replacements = dict(
> -            mmc_option=cls.mmc_option, kernel_addr=cls.kernel_addr,
> -            initrd_addr=cls.initrd_addr)
> -        return (
> -            "fatload mmc %(mmc_option)s %(kernel_addr)s "
> -                "uImage; fatload mmc %(mmc_option)s %(initrd_addr)s uInitrd; "
> -                "bootm %(kernel_addr)s %(initrd_addr)s"
> -                % replacements)
> +        if cls.dtb_addr is not None:
> +            replacements = dict(
> +                mmc_option=cls.mmc_option, kernel_addr=cls.kernel_addr,
> +                initrd_addr=cls.initrd_addr, dtb_addr=cls.dtb_addr)
> +            return (
> +                "fatload mmc %(mmc_option)s %(kernel_addr)s uImage; "
> +                    "fatload mmc %(mmc_option)s %(initrd_addr)s uInitrd; "
> +                    "fatload mmc %(mmc_option)s %(dtb_addr)s board.dtb; "
> +                    "bootm %(kernel_addr)s %(initrd_addr)s %(dtb_addr)s"
> +                    % replacements)
> +        else:
> +            replacements = dict(
> +                mmc_option=cls.mmc_option, kernel_addr=cls.kernel_addr,
> +                initrd_addr=cls.initrd_addr)
> +            return (
> +                "fatload mmc %(mmc_option)s %(kernel_addr)s uImage; "
> +                    "fatload mmc %(mmc_option)s %(initrd_addr)s uInitrd; "
> +                    "bootm %(kernel_addr)s %(initrd_addr)s"
> +                    % replacements)

 Not sure if this is nicer:

 maybe_dtb_addr = ""
 maybe_fatload_dtb = ""
 if cls.dtb_addr is not None:
     maybe_dtb_addr = " %s" % cls.dtb_addr
     maybe_fatload_dtb = "fatload mmc %(mmc_option)s %(dtb_addr)s board.dtb; "
 replacements = dict(
     mmc_option=cls.mmc_option, kernel_addr=cls.kernel_addr,
     initrd_addr=cls.initrd_addr, maybe_dtb_addr=maybe_dtb_addr,
     maybe_fatload_dtb=maybe_fatload_dtb)
 return (
     "fatload mmc %(mmc_option)s %(kernel_addr)s uImage; "
     "fatload mmc %(mmc_option)s %(initrd_addr)s uInitrd; "
     "%(maybe_fatload_dtb)s"
     "bootm %(kernel_addr)s %(initrd_addr)s%(maybe_dtb_addr)s"
     % replacements)

 Perhaps it would be simpler to just:

 replacements = dict(
     mmc_option=cls.mmc_option, kernel_addr=cls.kernel_addr,
     initrd_addr=cls.initrd_addr, dtb_addr=cls.dtb_addr)
 boot_script = (
     "fatload mmc %(mmc_option)s %(kernel_addr)s uImage; "
     "fatload mmc %(mmc_option)s %(initrd_addr)s uInitrd; "
     % replacements)
 if cls.dtb_addr is not None:
     boot_script += (
         "fatload mmc %(mmc_option)s %(dtb_addr)s board.dtb; "
         "bootm %(kernel_addr)s %(initrd_addr)s %(dtb_addr)s" % replacements)
 else:
     boot_script += "bootm %(kernel_addr)s %(initrd_addr)s" % replacements
 return boot_script

>  class Mx51Config(Mx5Config):
> -    kernel_addr = '0x90000000'
> -    initrd_addr = '0x92000000'
> +    kernel_addr = '0x90800000'
> +    dtb_addr = '0x917f0000'
> +    initrd_addr = '0x91800000'

 Mind giving some details about why these changes to kernel_addr and
 initrd_addr are required?

> +def make_dtb(img_data, boot_disk):
> +    img = None
> +    if img_data is not None:
> +        img = '%s/board.dtb' % boot_disk
> +        cmd_runner.run(['cp', img_data, img], as_root=True).wait()
> +    return img

 Would you add a (trivial) test for this function in
 test_media_create.py?

> @@ -622,12 +627,17 @@
>          flavorxy = 'flavorXY'
>          class config(boards.BoardConfig):
>              kernel_flavors = [flavorx, flavorxy]
> +            dtb_name = 'board_name.dtb'
>          for f in reversed(config.kernel_flavors):
>              kfile = os.path.join(tempdir, 'vmlinuz-1-%s' % f)
>              ifile = os.path.join(tempdir, 'initrd.img-1-%s' % f)
> +            dt = os.path.join(tempdir, 'dt-1-%s' % f)
> +            os.mkdir(dt)
> +            dfile = os.path.join(dt, config.dtb_name)
>              open(kfile, "w").close()
>              open(ifile, "w").close()
> -        self.assertEqual((kfile, ifile), config._get_kflavor_files(tempdir))
> +            open(dfile, "w").close()
> +        self.assertEqual((kfile, ifile, dfile), config._get_kflavor_files(tempdir))
>  
>      def test_get_kflavor_files_later_in_flavors(self):
>          tempdir = self.useFixture(CreateTempDirFixture()).tempdir
> @@ -635,11 +645,16 @@
>          flavor2 = 'flavorAA'
>          class config(boards.BoardConfig):
>              kernel_flavors = [flavor1, flavor2]
> +            dtb_name = 'board_name.dtb'
>          kfile = os.path.join(tempdir, 'vmlinuz-1-%s' % flavor1)
>          ifile = os.path.join(tempdir, 'initrd.img-1-%s' % flavor1)
> +        dt = os.path.join(tempdir, 'dt-1-%s' % flavor1)
> +        os.mkdir(dt)
> +        dfile = os.path.join(dt, config.dtb_name)
>          open(kfile, "w").close()
>          open(ifile, "w").close()
> -        self.assertEqual((kfile, ifile), config._get_kflavor_files(tempdir))
> +        open(dfile, "w").close()
> +        self.assertEqual((kfile, ifile, dfile), config._get_kflavor_files(tempdir))
>  
>      def test_get_kflavor_files_raises_when_no_match(self):
>          tempdir = self.useFixture(CreateTempDirFixture()).tempdir

 Please add a test when there is no .dtb once you make it optional  :-)

-- 
Loïc Minier