Code review comment for lp://qastaging/~linaro-landing-team-freescale/linaro-image-tools/mx53-loco

On Sun, Mar 6, 2011 at 6:06 AM, Loïc Minier <email address hidden> wrote:
>        Hey
>
>  Thanks for the updated branch; currently, we have two i.mx51 boards in
>  linaro-image-tools defined as follow:
> class EfikamxConfig(Mx5Config):
>    uboot_flavor = 'efikamx'
>
> class Mx51evkConfig(Mx5Config):
>    uboot_flavor = 'mx51evk'
>
>  (that's the whole definition!)
>
>  I believe i.MX53 boots similarly to i.MX51, so I was hoping we could
>  achieve something simpler; I've pushed
>    lp:~lool/linaro-image-tools/mx53-loco
>  with some proposed changes on top of your branch and proposed a merge;
>  the board config looks like this now:
> class Mx53LoCoConfig(Mx5Config):
>    uboot_flavor = 'mx53_loco'
>    kernel_addr = '0x70800000'
>    initrd_addr = '0x71800000'
>    load_addr = '0x70008000'
>    kernel_suffix = 'linaro-imx5'
>
>  See below for comments on the diff.
>
>> --- linaro_media_create/boards.py     2011-03-03 13:44:11 +0000
>> +++ linaro_media_create/boards.py     2011-03-05 14:45:31 +0000
>> @@ -411,6 +411,30 @@
>>      uboot_flavor = 'mx51evk'
>>
>>
>> +class Mx53LoCoConfig(Mx5Config):
>> +    uboot_flavor = 'mx53_loco'
>> +    kernel_addr = '0x70800000'
>> +    initrd_addr = '0x71800000'
>> +    load_addr = '0x70008000'
>> +    kernel_suffix = 'linaro-imx5'
>
>  The kernel suffix we use for i.MX51 boards is currently linaro-mx51;
>  for which kernel is the above suffix?  Is it a BSP kernel, and if so
>  why does it have linaro in the name?  If not, could we switch all the
>  i.MX51 boards to use it instead of linaro-mx51?  It would be nice to
>  have a single kernel for all i.MX51 and i.MX53 boards.

That's my intention too. The problem with upstream kernel is that
the RUNTIME PHYS_OFFSET is not yet there, thus preventing a
single kernel image built for i.MX51 and i.MX53, although work is in
smooth progress, and very hopefully we'll see it in 2.6.39.

And for 11.05 release, we are using a kernel based on Freescale's
2.6.35 BSP, which supports a single kernel for both i.MX51/53
(with those relevant patches backported)

>
>  Any reason this is called "imx5" instead of "mx5"?  Is this to use the
>  same name as SOC_IMX50, SOC_IMX51, SOC_IMX53?  SOC_ is used a lot in
>  i.MX mach-* subtrees, but not so much in other trees; maybe we should
>  use the name from the mach-* subdirectories to name our suffixes?
>  (linaro-mx5)

No specific reason for that. mx5 is perfectly fine.

>
>> +    @classmethod
>> +    def _make_boot_files(cls, uboot_parts_dir, boot_cmd, chroot_dir,
>> +                 boot_dir, boot_script, boot_device_or_file):
>> +        uboot_imx_file = os.path.join(chroot_dir, 'usr', 'lib', 'u-boot', 'mx53_loco', 'u-boot.imx')
>> +     uboot_bin_file = os.path.join(chroot_dir, 'usr', 'lib', 'u-boot', 'mx53_loco', 'u-boot.bin')
>
>  Above, you're mixing tabs and spaces; because Python relies on the
>  indentation to decide how to interpret your code, this is particularly
>  dangerous; I've fixed this in my branch.
>
>  Also, it seems 'mx53_loco' should be cls.uboot_flavor; I've fixed this
>  in my branch.

Yes, you are right.

>
>> +     if os.path.exists(uboot_imx_file):
>> +         uboot_file = uboot_imx_file
>> +         uboot_padded = 0
>> +        else:
>> +         uboot_file = uboot_bin_file
>> +         uboot_padded = 1
>
>  Is there a reason why uboot_imx_file would ever be missing from the
>  hwpack?

We're currently using the u-boot from Freescale's BSP since the
support in upstream is not there yet. Until the mx53 loco patches
are upstreamed, we can definitely remove this.

>
>  I looked at u-boot.bin and u-boot.imx for an imx51 board (efikasb)
>  built on upstream u-boot, and u-boot.imx was 147848 while u-boot.bin
>  was 146824, so u-boot.imx is larger than u-boot.bin.  I know u-boot.imx
>  is not padded as it's what we dd for other mx5 (imx51) boards, but
>  clearly that's not the only difference, otherwise u-boot.bin would be
>  exactly 1024 bytes larger than u-boot.imx.  So it seems installing
>  u-boot.bin or u-boot.imx would not have the result (even after
>  adjusting padding); or perhaps your u-boot.bin is not built in the same
>  way as the ones we have for other imx51 boards?

No, the u-boot.bin built from Freescale's BSP is the one padded with
the MBR. It's just painful to explain this. The u-boot.imx is generated
with a script in upstream, while there is no such thing in Freescale's
BSP (cuz it's basing on a earlier u-boot version). So instead, they're
generating a different u-boot.bin.

>
>  My preference would be to always require u-boot.imx; I've changed the
>  logic as such in my proposed branch.
>
>> +     install_mx5_uboot(uboot_file, uboot_padded, boot_device_or_file)
>
>  Ah, so you call this install_mx5_uboot(), but it's only ever used on an
>  imx53 board, and there's already an install_mx5_boot_loader() which is
>  only ever used on imx51 boards now, so it's a bit confusing because we
>  shouldn't name mx5 things which are specific to mx51 or mx53.  I
>  believe we can do with a single function which takes a padding
>  argument, for instance Mx5Config could call install_mx5_uboot with
>  uboot_padded=0.  (I've removed this function in my branch.)

Exactly, and especially because mx53 loco patches in u-boot is ready,
and I suppose it to be merged soon.

>
>> +     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)
>
>  This part is identical to Mx5Config; I've dropped _make_boot_files() in
>  my branch.
>
>> +
>> +
>>  class VexpressConfig(BoardConfig):
>>      uboot_flavor = 'ca9x4_ct_vxp'
>>      uboot_in_boot_part = True
>> @@ -442,6 +466,7 @@
>>      'ux500': Ux500Config,
>>      'efikamx': EfikamxConfig,
>>      'mx51evk': Mx51evkConfig,
>> +    'mx53loco' : Mx53LoCoConfig,
>>      'overo': OveroConfig,
>>      }
>>
>> @@ -520,6 +545,18 @@
>>      proc.wait()
>>
>>
>> +def install_mx5_uboot(uboot_file, padded, boot_device_or_file):
>> +    proc = cmd_runner.run([
>> +     "dd",
>> +     "if=%s" % uboot_file,
>> +     "of=%s" % boot_device_or_file,
>> +     "bs=1024",
>> +     "seek=1",
>> +     "skip=%d" % padded,
>> +     "conv=notrunc"], as_root=True)
>> +    proc.wait()
>
>  This is very similar to install_mx5_boot_loader() and the names would
>  be confusing; I think we should merge these, and I've dropped this
>  function in my branch.
>
>> +
>> +
>>  def _get_mlo_file(chroot_dir):
>>      # XXX bug=702645: This is a temporary solution to make sure l-m-c works
>>      # with any version of x-loader-omap. The proper solution is to have
>
>
>  If we really need to support both padded and unpadded u-boots, we could
>  support this in Mx5Config and install_mx5_boot_loader, but I'd hope we
>  don't need to.

In the end of the day, we don't.

>
>    Thanks!
> --
> Loïc Minier
>