Linaro Image Tools

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

add-dtb
Merge into trunk

Revision history for this message

Loïc Minier (lool) wrote on 2011-04-11:

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 the
> changes as a separate commit.

The offsets are smaller now though, see
https://launchpad.net/bugs/737321

Maybe change it the other way around? (Bumping the mx53 offsets)

> >> 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 :-)
> >
> I do not plan to make it optional until HardwarePackV2 becomes
> available. But will add a test when we get there.

It's too far away sadly; it's our next big project, but it's going to
take some time, and we can't lock DT support on this (will take time to
implement).

> How should I publish the new patch version? Is it possible to rewrite
> the branch published already, or I have to push a new branch? I'm
> also new on Launchpad and Bzr :)

So bzr encourages /not/ rebasing; just keep committing on top of your
branch and push to the same location, then ping this merge request to
say it's ready for review with this and that change :-)

--
Loïc Minier

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 the
> changes as a separate commit.

The offsets are smaller now though, see
 https://launchpad.net/bugs/737321

Maybe change it the other way around?  (Bumping the mx53 offsets)

> >>      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  :-)
> >
> I do not plan to make it optional until HardwarePackV2 becomes
> available.  But will add a test when we get there.

It's too far away sadly; it's our next big project, but it's going to
 take some time, and we can't lock DT support on this (will take time to
 implement).

> How should I publish the new patch version?  Is it possible to rewrite
> the branch published already, or I have to push a new branch?  I'm
> also new on Launchpad and Bzr :)

So bzr encourages /not/ rebasing; just keep committing on top of your
 branch and push to the same location, then ping this merge request to
 say it's ready for review with this and that change   :-)

-- 
Loïc Minier

« Back to merge proposal