Merge lp:~shawnguo/linaro-image-tools/add-dtb into lp:linaro-image-tools/11.11

 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 %(kerne...

257 - 'igep': IgepConfig,
258 + 'igepv2': Igepv2Config,

Should we break --dev igep?

We could add an alias facility so that it keeps working, but it would
be a bunch more work.

(We can just put Igepv2Config in the dict twice, but I think it's better
to make it hidden so as not to confuse someone new with an igepv3 board.)

Thanks,

James

On Mon, Apr 11, 2011, James Westby wrote:
> (We can just put Igepv2Config in the dict twice, but I think it's better
> to make it hidden so as not to confuse someone new with an igepv3 board.)

 I think it's ok to break it; the new name is more correct and we can
 include this in the release notes; people who run the command by hand
 should notice the new name in the help output

--
Loïc Minier

On 11 April 2011 18:29, Loïc Minier <email address hidden> wrote:
>  This is looking awesome, good job!  Some questions below
>
Thanks. And I'm still a Python beginner, so really need your expertise here.

> 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?
>
For this cycle, the DT support is very basic and limited, and I do not
see any possible regression for any board 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.
>
All I have for testing is mx51evk :) I have never seen either IGEPv2
or IGEPv3. I'm seeing dts files 'isee-igep-v2.dts' and
'isee-igep-v3.dts' under linux-linaro-2.6.38/arch/arm/boot/dts (Nico's
tree), so I would say it's a direct reflecting of kernel tree.

>> * 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.
>
I only keep my eyes on folder linux-linaro-2.6.38/arch/arm/boot/dts,
as I suppose lmc dt support only works with .deb built from John
Rigby's tree which is a downstream of Nico's tree.

>> * 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.
>
https://bugs.launchpad.net/linaro-image-tools/+bug/757694

>> * 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 armv...

On Tue, Apr 12, 2011, Shawn Guo wrote:
> >  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.
> >
> All I have for testing is mx51evk :) I have never seen either IGEPv2
> or IGEPv3. I'm seeing dts files 'isee-igep-v2.dts' and
> 'isee-igep-v3.dts' under linux-linaro-2.6.38/arch/arm/boot/dts (Nico's
> tree), so I would say it's a direct reflecting of kernel tree.

 Ok, so we only support IGEPv2; you could just keep the current "IGEP"
 name and add IGEPv2 specific things to it. I don't mind if you rename
 it to IGEPv2, but it's also counter-intuitive for people who were using
 the "igep" name, so I wouldn't bother and just let it be "igep". Don't
 add an IGEPv3 though, 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.
> >
> I only keep my eyes on folder linux-linaro-2.6.38/arch/arm/boot/dts,
> as I suppose lmc dt support only works with .deb built from John
> Rigby's tree which is a downstream of Nico's tree.

 Yup; not your duty to chase Vexpress support if it's not in the tree;
 just wanted to mention it for completeness.

> >> * 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.
> Will HardwarePackV2 be implemented for 11.05 release cycle? If yes, I
> will keep my eyes on it and start address the problem when it gets
> ready. Otherwise, please let me know what I need to do.

 No, it wont be done in 11.05. What needs to be done is to check
 whether the .dtb file is present; if it is, use it, otherwise default
 to non-dtb mode.

 You can test with os.path.exists().

> >>  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?
> >
> I think it's nice if they get aligned with mx53 address offset,
> because I do not want to get people wondering why mx51 needs to be
> different from mx53 in terms of the address offset or versus. But the
> changes are not required indeed for dtb support. That's why I got t...

Hi Loic,

I just updated the branch with your comments addressed. Can you please start anther round of review and let me know if you have further comments.

The updates include:
* make the present of .dtb file optional
* keep 'igep' as the original and add isee-igep-v2.dtb support for 'igep' board
* take your suggestion to make bootcmd(cls) a little simpler
* keep mx51 addresses as the original and align mx53 with it to keep initrd image ~32M away from kernel
* add tests for dtb related functions per your suggestion

Regards,
Shawn

 review: approve

On Tue, Apr 12, 2011, Shawn Guo wrote:
> I just updated the branch with your comments addressed. Can you
> please start anther round of review and let me know if you have
> further comments.

 Yup; in the future, please try to commit separate changes separately.

 In _get_kflavor_files, I'd use two different names for cls.dtb_name and
 the result of _get_file_matching; the latter is a pathname while the
 former is just the last part of the pathname.

 But there is an actually even nicer change you could do: include the
 dtb_name in the regexp:
 DT_GLOB = 'dt-*-%(kernel_flavor)s/%(dtb_name)'

 dregex = DT_GLOB % {'kernel_flavor': flavor, 'dtb_name': cls.dtb_name}

 You could add a comment in the function to state that the dtb is
 optional, or better: expand the inline doc to state that dtb might be
 None in the output:
- """Search for kernel, initrd and dtb in path."""
+ """Search for kernel, initrd and optional dtb in path."""

 Don't set cls.dtb_name = cls.dtb_addr in this function; we don't change
 class variables down there, and the values you set should already be
 set if the definition is correct.

 I don't understand the layout though: is it supposed to be
 /boot/dt-something-2.6.38-1/mx51-babbage.dtb?

 What are the possible values for "something" above?

 Are these files provided directly by the kernel package?

 In make_dtb(), I don't really like the name of the file, but I can't
 think of a better filename. :-/

 There are a couple gratuitous changes in the diff, but they seem
 harmless:
 * make_boot_files(): you move the _get_kflavor_files call one line up,
   doesn't seem to matter
 * in test_igep, switching from board_configs['igep'] to
   boards.IgepConfig
 these don't matter to me either way.

--
Loïc Minier

On 13 April 2011 01:12, Loïc Minier <email address hidden> wrote:
> Review: Approve
>  review: approve
>
> On Tue, Apr 12, 2011, Shawn Guo wrote:
>> I just updated the branch with your comments addressed.  Can you
>> please start anther round of review and let me know if you have
>> further comments.
>
>  Yup; in the future, please try to commit separate changes separately.
>
Ok. I actually intended to do so, but the Preview Diff on review page
squash all the commits, which led me thought that the separate is not
important in Bzr. Clearly, I'm wrong here. So what is the benefit of
doing this? Will all those separate commits show up on trunk with
merge?

>  In _get_kflavor_files, I'd use two different names for cls.dtb_name and
>  the result of _get_file_matching; the latter is a pathname while the
>  former is just the last part of the pathname.
>
>  But there is an actually even nicer change you could do: include the
>  dtb_name in the regexp:
>  DT_GLOB = 'dt-*-%(kernel_flavor)s/%(dtb_name)'
>
>  dregex = DT_GLOB % {'kernel_flavor': flavor, 'dtb_name': cls.dtb_name}
>
It does look nice to me.

>  You could add a comment in the function to state that the dtb is
>  optional, or better: expand the inline doc to state that dtb might be
>  None in the output:
> -        """Search for kernel, initrd and dtb in path."""
> +        """Search for kernel, initrd and optional dtb in path."""
>
Ok.

>  Don't set cls.dtb_name = cls.dtb_addr in this function; we don't change
>  class variables down there, and the values you set should already be
>  set if the definition is correct.
>
I need the setting of 'cls.dtb_name = cls.dtb_addr = None' to make
.dtb file optional. If _get_file_matching returns None, that means
the .dtb file is absent, and we need to set cls.dtb_name and
cls.dtb_addr None to let bootcmd(cls) and make_dtb bypass the .dtb
handling.

I do not think it's good to set class variables here either. But I
did not come up with any better idea to do that in the simple manner
as it is currently.

>  I don't understand the layout though: is it supposed to be
>  /boot/dt-something-2.6.38-1/mx51-babbage.dtb?
>
Yes, it's something like /boot/dt-2.6.38-1001-linaro-mx51/mx51-babbage.dtb

>  What are the possible values for "something" above?
>
The 'something' is the exactly same one in
vmlinuz-2.6.38-1001-linaro-mx51 and initrd.img-2.6.38-1001-linaro-mx51

>  Are these files provided directly by the kernel package?
>
Yes.

>  In make_dtb(), I don't really like the name of the file, but I can't
>  think of a better filename.  :-/
>
If you remember, I asked for suggestion on linaro-dev, but no one gave.

>  There are a couple gratuitous changes in the diff, but they seem
>  harmless:
>  * make_boot_files(): you move the _get_kflavor_files call one line up,
>   doesn't seem to matter

It actually matters. We have to set cls.dtb_name and cls.dtb_addr
None in case that .dtb is absent before bootcmd(cls) and make-dtb()
functions are called. These two functions will actually bypass the
.dtb handling when cls.dtb_name and cls.dtb_addr are set None in
_get_kflavor_files.

>  * in test_igep, switching from board_configs['igep'] to
>   boards.IgepConfig

Ah, r...

On Wed, Apr 13, 2011, Shawn Guo wrote:
> Ok. I actually intended to do so, but the Preview Diff on review page
> squash all the commits, which led me thought that the separate is not
> important in Bzr. Clearly, I'm wrong here. So what is the benefit of
> doing this? Will all those separate commits show up on trunk with
> merge?

 I can still open the individual revisions from the review page and see
 each diff individually, or download the branch locally and diff the
 revisions myself or browse them with "bzr vis" or other GUIs.

 They stay as separate commits in the merged tree in the end too.

> >  I don't understand the layout though: is it supposed to be
> >  /boot/dt-something-2.6.38-1/mx51-babbage.dtb?
> >
> Yes, it's something like /boot/dt-2.6.38-1001-linaro-mx51/mx51-babbage.dtb
>
> >  What are the possible values for "something" above?
> >
> The 'something' is the exactly same one in
> vmlinuz-2.6.38-1001-linaro-mx51 and initrd.img-2.6.38-1001-linaro-mx51

 Oh sorry, I'm just being confused by myself; I had forgotten that
 kernel_flavor was lacking the kernel version; that's the reason for the
 first *-.

> >  In make_dtb(), I don't really like the name of the file, but I can't
> >  think of a better filename.  :-/
> If you remember, I asked for suggestion on linaro-dev, but no one gave.

 By any chance, did you find out how ppc does this?

> I need the setting of 'cls.dtb_name = cls.dtb_addr = None' to make
> .dtb file optional. If _get_file_matching returns None, that means
> the .dtb file is absent, and we need to set cls.dtb_name and
> cls.dtb_addr None to let bootcmd(cls) and make_dtb bypass the .dtb
> handling.
>
> I do not think it's good to set class variables here either. But I
> did not come up with any better idea to do that in the simple manner
> as it is currently.
[...]
> >  There are a couple gratuitous changes in the diff, but they seem
> >  harmless:
> >  * make_boot_files(): you move the _get_kflavor_files call one line up,
> >   doesn't seem to matter
>
> It actually matters. We have to set cls.dtb_name and cls.dtb_addr
> None in case that .dtb is absent before bootcmd(cls) and make-dtb()
> functions are called. These two functions will actually bypass the
> .dtb handling when cls.dtb_name and cls.dtb_addr are set None in
> _get_kflavor_files.

 Ok; I guess this means that optional dtb is a bit more work than I had
 hoped: it seems we need to pass the real dtb pathname down to bootcmd
 computation, so that cls.bootcmd can't be a @classproperty anymore but
 needs to be changed to a _get_boot_cmd() function. Fortunately, it's
 only used in one spot which already takes optional parameters like
 "is_live".

 For make_dtb, it should be a bit simpler; you can:
    if d_img_data is not None:
        make_dtb()
 or handle it in the make_dtb function itself (less occurrences, but a
 bit inconsistent with other make_*() functions' interfaces) -- I'm fine
 either way.

--
Loïc Minier

On 13 April 2011 15:36, Loïc Minier <email address hidden> wrote:
> On Wed, Apr 13, 2011, Shawn Guo wrote:
[...]
>> >  In make_dtb(), I don't really like the name of the file, but I can't
>> >  think of a better filename.  :-/
>> If you remember, I asked for suggestion on linaro-dev, but no one gave.
>
>  By any chance, did you find out how ppc does this?
>

This is what I got from Grant.

--- quote begins ---
On Tue, Mar 29, 2011 at 11:03:25PM +0800, Shawn Guo wrote:
> Hi Grant,
>
> Do you happen to know where powerpc releases put dtb? If we could
> possibly align with existing approach, I would do.

They don't. All of the powerpc systems supported by packaged kernels
in the major distros use real OpenFirmware which provides the device
tree. For the embedded boards, most users are building and deploying
the kernel+dtb with their own solutions. We're very much in new
territory here.
--- quote ends ---

>> I need the setting of 'cls.dtb_name = cls.dtb_addr = None'  to make
>> .dtb file optional.  If _get_file_matching returns None, that means
>> the .dtb file is absent, and we need to set cls.dtb_name and
>> cls.dtb_addr None to let bootcmd(cls) and make_dtb bypass the .dtb
>> handling.
>>
>> I do not think it's good to set class variables here either.  But I
>> did not come up with any better idea to do that in the simple manner
>> as it is currently.
> [...]
>> >  There are a couple gratuitous changes in the diff, but they seem
>> >  harmless:
>> >  * make_boot_files(): you move the _get_kflavor_files call one line up,
>> >   doesn't seem to matter
>>
>> It actually matters.  We have to set cls.dtb_name and cls.dtb_addr
>> None in case that .dtb is absent before bootcmd(cls) and make-dtb()
>> functions are called.  These two functions will actually bypass the
>> .dtb handling when cls.dtb_name and cls.dtb_addr are set None in
>> _get_kflavor_files.
>
>  Ok; I guess this means that optional dtb is a bit more work than I had
>  hoped: it seems we need to pass the real dtb pathname down to bootcmd
>  computation, so that cls.bootcmd can't be a @classproperty anymore but
>  needs to be changed to a _get_boot_cmd() function.  Fortunately, it's
>  only used in one spot which already takes optional parameters like
>  "is_live".
>
>  For make_dtb, it should be a bit simpler; you can:
>    if d_img_data is not None:
>        make_dtb()
>  or handle it in the make_dtb function itself (less occurrences, but a
>  bit inconsistent with other make_*() functions' interfaces) -- I'm fine
>  either way.
>
Ok, so you think the current implementation is quick but dirty, and I
will try to implement what you suggest here.

--
Regards,
Shawn

Hi Loic,

I just pushed revisions 321 ~ 324 to address your last round comments. Please review them. Thanks.

Regards,
Shawn

Looks good!