Code review comment for lp:~shawnguo/linaro-image-tools/add-dtb

 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