Merge lp:~angus-akkea/linaro-image-tools/Samsung-SMDKV310 into lp:linaro-image-tools/11.11

[Angus Ainslie]

Added back in the temp_dir logic I pulled out during the last merge

Fixed conflicts

Remove duplicates added during merge
Fix tests for me partition sizes

Merged in lp:~lool/linaro-image-tools/samsung-v310-v2

Adds a bunch of constants for the Samsung board
More comments
Adds a _dd method for all to use
Creates a boot_env dict for all to use

Added Loic's constant changes and comments

Changed bootargs and bootcmd methods to be used to create bootable_env
All all possible filesystem, bootagrs and bootcmd changes that are possible without functional VFAT in u-boot. Commented lines that should be used once FAT is functional.

More Loic recommendations

The SMDKv310 code now follows the recommended boot (FAT) and rootfs (EXT) partition scheme. BL1 , u-boot, the kernel and the initrd all load from the raw mmc offsets currently. Once my MMC issues are resolved I'll be able to load some of these from the FAT partition.

Update code based on Loic comments and fixed test cases
More changes to fix merge with sfdisk partition changes

Add support for Samsung SMDK V310 board
Use uses_fat_boot_partition = False as an indicator to not create the fat partition
Add make_flashable_env that will create a binary suitable to burn directly to flash for a u-boot environment.
add tests
One more minor change to set the bootm line correctly to mount the initrd

[James Westby (james-w)]

59 + sfdisk_cmd = super(SamsungConfig, cls).get_sfdisk_cmd()
60 + return ',14,0xDA\n,,,-'

You don't want to use the sfdisk_cmd that you get from the upcall?

149 +def install_mx51evk_boot_loader(imx_file, boot_device_or_file):

I don't think you wanted to add this?

166 + "seek=1089",

Could we get some of these magic numbers turned in to local variables, so that
we have a name that means something for them. A comment may be useful too if the
variable name can't express what the number is.

Overall this looks pretty clean, thanks. I'll ask Guilherme for a review
as well, and we'll want the tests before this lands.

Thanks,

James

review: Needs Fixing

[Guilherme Salgado (salgado)]

Hi Angus,

I see some room for improvement here, but overall it looks good.

 review needsfixing

On Thu, 2011-02-10 at 01:13 +0000, Angus Ainslie wrote:
> differences between files attachment (review-diff.txt)
> === modified file 'linaro_media_create/boards.py'
> --- linaro_media_create/boards.py 2011-02-01 13:16:43 +0000
> +++ linaro_media_create/boards.py 2011-02-10 01:13:31 +0000
> @@ -29,9 +29,11 @@
> import os
> import re
> import tempfile
> +import struct
>
> from linaro_media_create import cmd_runner
> -
> +from binascii import hexlify
> +from binascii import crc32

Please combine these imports into just one line:

    from binascii import (
        crc32,
        hexlify
        )

>
> class BoardConfig(object):
> """The configuration used when building an image for a board."""
> @@ -55,6 +57,7 @@
> @classmethod
> def get_sfdisk_cmd(cls):
> """Return the sfdisk command to partition the media."""
> +

Why did you add a blank line here?

> if cls.fat_size == 32:
> partition_type = '0x0C'
> else:
> @@ -201,6 +204,7 @@
> make_boot_ini(boot_script, boot_dir)
>
>
> +

And this one here?

> class BeagleConfig(OmapConfig):
> uboot_flavor = 'omap3_beagle'
> _serial_tty = 'ttyO2'
> @@ -335,6 +339,68 @@
> cls.load_addr, uboot_parts_dir, cls.kernel_suffix, boot_dir)
> make_uInitrd(uboot_parts_dir, cls.kernel_suffix, boot_dir)
>
> +class SamsungConfig(BoardConfig):
> + boot_env = [
> + 'baudrate=115200',
> + 'bootargs=root=/dev/mmcblk0p2 rootdelay=1 rw init=/bin/bash console=ttySAC1,115200',
> + 'bootcmd=movi read kernel 40007000; movi read rootfs 41000000 600000; bootm 40007000',
> + 'bootdelay=3',
> + 'ethact=smc911x-0',
> + 'ethaddr=00:40:5c:26:0a:5b',
> + 'gatewayip=192.168.0.1',
> + 'ipaddr=192.168.0.20',
> + 'netmask=255.255.255.0',
> + 'serverip=192.168.0.10',

We don't configure the network interface statically for any other board.
Is there any reason why we need to do so here?

> + 'stderr=serial',
> + 'stdin=serial',
> + 'stdout=serial'
> + ]
> +
> + @classmethod
> + def get_sfdisk_cmd(cls):
> + # Create a one cylinder partition for fixed-offset bootloader data at
> + # the beginning of the image (size is 14 cylinder, so 115,146,752 bytes
> + # with the first sector for MBR).

You create two partitions here and not just the one for the bootloader
as the comment implies, right?

> + sfdisk_cmd = super(SamsungConfig, cls).get_sfdisk_cmd()

As James pointed out, since you don't use the return value of the upcall
there's no need to upcall at all.

> + return ',14,0xDA\n,,,-'
> +
> + @classmethod
> + def _make_boot_files(cls, uboot_parts_dir, boot_cmd, chroot_dir,
> + boot_dir, boot_script, boot_device_or_file):
> +
> + uboot_file = os.path.join(
> + chroot_dir, 'usr', 'lib', 'u-boot', 'smdkv310', 'u-boot.v310')
> + install_smdkv310_boot_loader(uboot_file, boot_device_or_file)
> + make_uInitrd(uboot_parts_dir, cls.kernel_suffix, bo...

review: Needs Fixing

[Angus Ainslie (angus-akkea)]

Thanks for looking at it, some comments inline.

On Thu, Feb 10, 2011 at 8:52 AM, Guilherme Salgado
<email address hidden> wrote:
> Review: Needs Fixing
> Hi Angus,
>
> I see some room for improvement here, but overall it looks good.
>
>  review needsfixing
>
>>
>>  class BoardConfig(object):
>>      """The configuration used when building an image for a board."""
>> @@ -55,6 +57,7 @@
>>      @classmethod
>>      def get_sfdisk_cmd(cls):
>>          """Return the sfdisk command to partition the media."""
>> +
>
> Why did you add a blank line here?
>
>>          if cls.fat_size == 32:
>>              partition_type = '0x0C'
>>          else:
>> @@ -201,6 +204,7 @@
>>          make_boot_ini(boot_script, boot_dir)
>>
>>
>> +
>
> And this one here?
>

Adding the blank linaes wasn't intentional, I'll fix them.

>>  class BeagleConfig(OmapConfig):
>>      uboot_flavor = 'omap3_beagle'
>>      _serial_tty = 'ttyO2'
>> @@ -335,6 +339,68 @@
>>              cls.load_addr, uboot_parts_dir, cls.kernel_suffix, boot_dir)
>>          make_uInitrd(uboot_parts_dir, cls.kernel_suffix, boot_dir)
>>
>> +class SamsungConfig(BoardConfig):
>> +    boot_env = [
>> +        'baudrate=115200',
>> +        'bootargs=root=/dev/mmcblk0p2 rootdelay=1 rw init=/bin/bash console=ttySAC1,115200',
>> +        'bootcmd=movi read kernel 40007000; movi read rootfs 41000000 600000; bootm 40007000',
>> +        'bootdelay=3',
>> +        'ethact=smc911x-0',
>> +        'ethaddr=00:40:5c:26:0a:5b',
>> +        'gatewayip=192.168.0.1',
>> +        'ipaddr=192.168.0.20',
>> +        'netmask=255.255.255.0',
>> +        'serverip=192.168.0.10',
>
> We don't configure the network interface statically for any other board.
> Is there any reason why we need to do so here?
>

That was just a verbatim copy of the u-boot env I had on my board so I
could get the CRC calc in make_flashable_env correct. I'll remove the
network lines

>> +        install_smdkv310_initrd(uInitrd_file, boot_device_or_file)
>> +
>> +
>> +class SMDKV310Config(SamsungConfig):
>> +    fat_size = 0
>> +    extra_serial_opts = 'console=ttySAC1,115200'
>> +    live_serial_opts = 'serialtty=ttyO2'
>> +    kernel_addr = '0x40008000'
>> +    initrd_addr = '0x40800000'
>> +    load_addr = '0x40008000'
>> +    kernel_suffix = 's5pv310'
>> +    boot_script = 'boot.scr'
>> +    extra_boot_args_options = (
>> +        'root=/dev/mmcblk0p1 rootdelay=1 rw init=/bin/bash')
>>
>>  board_configs = {
>>      'beagle': BeagleConfig,
>> @@ -344,6 +410,7 @@
>>      'ux500': Ux500Config,
>>      'mx51evk': Mx51evkConfig,
>>      'overo': OveroConfig,
>> +    'smdkv310': SMDKV310Config,
>>      }
>>
>>
>> @@ -359,6 +426,7 @@
>>             '-n', name,
>>             '-d', img_data,
>>             img]
>> +
>>      proc = cmd_runner.run(cmd, as_root=as_root, stdout=stdout)
>>      proc.wait()
>>      return proc.returncode
>> @@ -407,6 +475,25 @@
>>          'script', '0', '0', 'boot script', tmpfile, boot_script)
>>
>>
>> +def make_flashable_env( boot_env, env_file ) :
>
> This could be a @classmethod on SamsungConfig, no?  That way you can get
> rid of the boot_env argument as you can get it from the class.
>

It could be but I thought th...

[Loïc Minier (lool)]

You might need a:
from __future__ import with_statement

(we hope to complete a backport to Ubuntu 10.04 and hence we'd like to keep support for python2.5)

perhaps it's best to avoid "with" entirely

[Guilherme Salgado (salgado)]

On Thu, 2011-02-10 at 16:34 +0000, Loïc Minier wrote:
> You might need a:
> from __future__ import with_statement
>
> (we hope to complete a backport to Ubuntu 10.04 and hence we'd like to keep support for python2.5)

We want to support python2.5 even though 2.6 is the default in Lucid?
If so, we'll have to add that import in a few other places because with
is already used in l-i-t.

>
> perhaps it's best to avoid "with" entirely

It's so cheap to make with work on 2.5 that I don't see why we'd want to
avoid it

[Guilherme Salgado (salgado)]

On Thu, 2011-02-10 at 16:22 +0000, Angus Ainslie wrote:
[...]
> >> +class SamsungConfig(BoardConfig):
> >> + boot_env = [
> >> + 'baudrate=115200',
> >> + 'bootargs=root=/dev/mmcblk0p2 rootdelay=1 rw init=/bin/bash console=ttySAC1,115200',
> >> + 'bootcmd=movi read kernel 40007000; movi read rootfs 41000000 600000; bootm 40007000',
> >> + 'bootdelay=3',
> >> + 'ethact=smc911x-0',
> >> + 'ethaddr=00:40:5c:26:0a:5b',
> >> + 'gatewayip=192.168.0.1',
> >> + 'ipaddr=192.168.0.20',
> >> + 'netmask=255.255.255.0',
> >> + 'serverip=192.168.0.10',
> >
> > We don't configure the network interface statically for any other board

[Angus Ainslie (angus-akkea)]

A couple of other comments

On Thu, Feb 10, 2011 at 8:52 AM, Guilherme Salgado
<email address hidden> wrote:
>> +        make_flashable_env(cls.boot_env, env_file)
>> +        install_smdkv310_boot_env(env_file, boot_device_or_file)
>> +
>> +        make_uImage(
>> +            cls.load_addr, uboot_parts_dir, cls.kernel_suffix, boot_dir)
>> +        uImage_file = os.path.join(
>> +            boot_dir, 'uImage')
>
> Would you mind changing make_uImage() to return the path of the file the
> image was written to?  That way we don't have to guess its path here and
> risk getting it wrong.
>

make_uImage already returns the the result of _run_mkimage. There's
probable code that already relies on that return code. Should I make
it return a tuple or would you like it managed in some other way ?

>> +        uInitrd_file = os.path.join(
>> +            boot_dir, 'uInitrd')
>
> Would you mind changing make_uInitrd() to return the path of the file
> the initrd was written to?  That way we don't have to guess its path
> here and risk getting it wrong.
>

Same as above

[Guilherme Salgado (salgado)]

(previous message hit a bug in LP so my actual comments were not included. resending...)

On Thu, 2011-02-10 at 16:22 +0000, Angus Ainslie wrote:
[...]
> >> +class SamsungConfig(BoardConfig):
> >> + boot_env = [
> >> + 'baudrate=115200',
> >> + 'bootargs=root=/dev/mmcblk0p2 rootdelay=1 rw init=/bin/bash console=ttySAC1,115200',
> >> + 'bootcmd=movi read kernel 40007000; movi read rootfs 41000000 600000; bootm 40007000',
> >> + 'bootdelay=3',
> >> + 'ethact=smc911x-0',
> >> + 'ethaddr=00:40:5c:26:0a:5b',
> >> + 'gatewayip=192.168.0.1',
> >> + 'ipaddr=192.168.0.20',
> >> + 'netmask=255.255.255.0',
> >> + 'serverip=192.168.0.10',
> >
> > We don't configure the network interface statically for any other board.
> > Is there any reason why we need to do so here?
> >
>
> That was just a verbatim copy of the u-boot env I had on my board so I
> could get the CRC calc in make_flashable_env correct. I'll remove the
> network lines

Oh, ok, is there anything else that would make sense to omit from the
default env?

>
> >> + install_smdkv310_initrd(uInitrd_file, boot_device_or_file)
> >> +
> >> +
> >> +class SMDKV310Config(SamsungConfig):
> >> + fat_size = 0
> >> + extra_serial_opts = 'console=ttySAC1,115200'
> >> + live_serial_opts = 'serialtty=ttyO2'
> >> + kernel_addr = '0x40008000'
> >> + initrd_addr = '0x40800000'
> >> + load_addr = '0x40008000'
> >> + kernel_suffix = 's5pv310'
> >> + boot_script = 'boot.scr'
> >> + extra_boot_args_options = (
> >> + 'root=/dev/mmcblk0p1 rootdelay=1 rw init=/bin/bash')
> >>
> >> board_configs = {
> >> 'beagle': BeagleConfig,
> >> @@ -344,6 +410,7 @@
> >> 'ux500': Ux500Config,
> >> 'mx51evk': Mx51evkConfig,
> >> 'overo': OveroConfig,
> >> + 'smdkv310': SMDKV310Config,
> >> }
> >>
> >>
> >> @@ -359,6 +426,7 @@
> >> '-n', name,
> >> '-d', img_data,
> >> img]
> >> +
> >> proc = cmd_runner.run(cmd, as_root=as_root, stdout=stdout)
> >> proc.wait()
> >> return proc.returncode
> >> @@ -407,6 +475,25 @@
> >> 'script', '0', '0', 'boot script', tmpfile, boot_script)
> >>
> >>
> >> +def make_flashable_env( boot_env, env_file ) :
> >
> > This could be a @classmethod on SamsungConfig, no? That way you can get
> > rid of the boot_env argument as you can get it from the class.
> >
>
> It could be but I thought there might be other board that might want
> to create a flashable env so that's why I had it as a separate
> function

Fair enough.

>
> >>
> >> === modified file 'linaro_media_create/partitions.py'
> >> --- linaro_media_create/partitions.py 2011-01-28 19:50:48 +0000
> >> +++ linaro_media_create/partitions.py 2011-02-10 01:13:31 +0000
> >> @@ -81,7 +81,7 @@
> >> else:
> >> bootfs, rootfs = get_boot_and_root_loopback_devices(media.path)
> >>
> >> - if should_format_bootfs:
> >> + if should_format_bootfs and board_config.fat_size != 0:
> >
> > I don't quite like the idea of abusing the fat_size to specify whether
> > or not to setup a vfat partition. I think a separate BoardConfig
> > attribute (uses_vfa...

[Angus Ainslie (angus-akkea)]

Some python challenges now

On Thu, Feb 10, 2011 at 8:52 AM, Guilherme Salgado
<email address hidden> wrote:
>> +    env = ''
>> +
>> +    for s in boot_env :
>> +        for c in s :
>> +            env += struct.pack( 'c', c  )
>> +        env += struct.pack( 'B', 0 )
>
> I think this could be written more concisely as
>
>  struct.pack('B', 0).join(boot_env)
>
> ?
>

I changed this section to look like this

def make_flashable_env( boot_env, env_size, env_file ) :
    struct.pack( 'B', 0 ).join( boot_env )

    # pad the rest of the env for the CRC calc
    while len( boot_env ) < (env_size - 4):
        boot_env += struct.pack( 'B', 0)

    crc = crc32(boot_env)
    boot_env = struct.pack('<i', crc) + boot_env

    with open(env_file, 'w') as fd:
        fd.write( boot_env )

I now get and error while running it :

  File "/usr/src/Samsung/linaro-image-tools/linaro_media_create/boards.py",
line 483, in make_flashable_env
    crc = crc32(boot_env)
TypeError: crc32() argument 1 must be string or read-only buffer, not list

[Guilherme Salgado (salgado)]

On Thu, 2011-02-10 at 17:49 +0000, Angus Ainslie wrote:
> A couple of other comments
>
> On Thu, Feb 10, 2011 at 8:52 AM, Guilherme Salgado
> <email address hidden> wrote:
> >> + make_flashable_env(cls.boot_env, env_file)
> >> + install_smdkv310_boot_env(env_file, boot_device_or_file)
> >> +
> >> + make_uImage(
> >> + cls.load_addr, uboot_parts_dir, cls.kernel_suffix, boot_dir)
> >> + uImage_file = os.path.join(
> >> + boot_dir, 'uImage')
> >
> > Would you mind changing make_uImage() to return the path of the file the
> > image was written to? That way we don't have to guess its path here and
> > risk getting it wrong.
> >
>
> make_uImage already returns the the result of _run_mkimage. There's
> probable code that already relies on that return code. Should I make
> it return a tuple or would you like it managed in some other way ?

I don't think there's any code currently using the return value of
make_uImage, but this is a very small and young codebase so if you don't
find anything using it, just change it.

>
> >> + uInitrd_file = os.path.join(
> >> + boot_dir, 'uInitrd')
> >
> > Would you mind changing make_uInitrd() to return the path of the file
> > the initrd was written to? That way we don't have to guess its path
> > here and risk getting it wrong.
> >
>
> Same as above

Ditto. ;)

--
Guilherme Salgado <https://launchpad.net/~salgado>

[Guilherme Salgado (salgado)]

On Thu, 2011-02-10 at 17:57 +0000, Angus Ainslie wrote:
> Some python challenges now
>
> On Thu, Feb 10, 2011 at 8:52 AM, Guilherme Salgado
> <email address hidden> wrote:
> >> + env = ''
> >> +
> >> + for s in boot_env :
> >> + for c in s :
> >> + env += struct.pack( 'c', c )
> >> + env += struct.pack( 'B', 0 )
> >
> > I think this could be written more concisely as
> >
> > struct.pack('B', 0).join(boot_env)
> >
> > ?
> >
>
> I changed this section to look like this
>
> def make_flashable_env( boot_env, env_size, env_file ) :
> struct.pack( 'B', 0 ).join( boot_env )

Sorry, I wasn't clear. The above code will return a string but won't
change boot_env, so you need to assign the result of that to a variable
and use it instead of boot_env.

>
> # pad the rest of the env for the CRC calc
> while len( boot_env ) < (env_size - 4):
> boot_env += struct.pack( 'B', 0)
>
> crc = crc32(boot_env)
> boot_env = struct.pack('<i', crc) + boot_env
>
> with open(env_file, 'w') as fd:
> fd.write( boot_env )
>
> I now get and error while running it :
>
> File "/usr/src/Samsung/linaro-image-tools/linaro_media_create/boards.py",
> line 483, in make_flashable_env
> crc = crc32(boot_env)
> TypeError: crc32() argument 1 must be string or read-only buffer, not list
>

--
Guilherme Salgado <https://launchpad.net/~salgado>

[Loïc Minier (lool)]

Wow, for some reason I thought lucid was 2.5, but it's 2.6; just ignore my previous comment

[Loïc Minier (lool)]

Forwarding a chat on this:

21:53 < lool> nytowl: I find the ethaddr worrying; init=/bin/bash is clearly bad too; console=ttySAC1 sounds like it should be handled like serialtty in the other boards
21:54 < lool> nytowl: do you actually need baudrate, bootdelay etc. in there or would the u-boot default be good enough?
21:55 < nytowl> lool, I don't like the ethaddr and bash either but I'd like network to work and init cause a kernel panic
21:55 < lool> nytowl: comment on "size is 14 cylinder" doesn't make sense anymore
21:55 < nytowl> I could probably get rid of baudrate and bootdelay
21:56 < lool> nytowl: I think we need a way to amend the cmdline; if we had this, you could just use this feature to override the default init while the kernel is broken
21:56 < lool> nytowl: (if you're tempted to provide this, submit it separately so that it can be merged separately)
21:56 < lool> nytowl: seek=1089 sounds like magic, where does it come from?
21:57 < lool> same question for seek=9281
21:57 < lool> or seek=33
21:57 < lool> I understand the seek=1 one though ;-)
21:57 < lool> and then there's this seek=65
21:57 < nytowl> Those are magic offsets that irom looks on the SD card for u-boot , uImage and initrd
21:57 < nytowl> I guess those should be commented :)

21:58 < lool> nytowl: uses_fat_boot_partition sounds like your board needs a different scheme
21:59 < nytowl> the boot_env is more than just the command line it is that flashable env I was talking to you about last week
21:59 < lool> nytowl: I think this board is substantially different and would warrant some deeper thoughts on how we should boot it and support it; where could I read about how it boots, which software
              starts from where etc.?
21:59 < nytowl> lool there's a little info in the wiki about those offsets
22:00 < nytowl> https://wiki.linaro.org/Boards/SMDKV310/Setup
22:00 < lool> nytowl: My ideal documentation would be one which walks me through the various stages of the boot up to linux
22:01 < nytowl> lool that document doesn't exsist

22:01 < lool> nytowl: Is there some kind of technical reference manual for the SoC and some kind of user guide for the board?
22:02 < nytowl> lool, I have a data sheet for the SoC but no user guids
22:02 < lool> nytowl: test_smdkv310() says ,14,0xDA and then rootfs, but https://wiki.linaro.org/Boards/SMDKV310/Setup says rootfs starts at 204001
22:03 < nytowl> lool I fixed the sfdisk get command but forgot about the test
22:05 < lool> nytowl: It seems you have u-boot running and that it's loading kernel and initrd from mmc, so I presume it has mmc support; why wouldn't it be able to load uimage and uinitrd from a MMC
              partition instead of loading them from hardcoded MMC offsets?
22:05 < lool> nytowl: Is the datasheet NDA? would I be able to get access to it?
22:06 < nytowl> lool, that was going to be step 2
22:06 < lool> nytowl: Is it from the datasheet that you found about these values?
22:06 < lool> nytowl: I think the l-i-t diff might be much smaller if you were using the same layout with vfat partition etc. as other boards
22:06 < nytowl> lool, now those values can from the BSP u-boot and discussions with Samsung
22:0...

[Angus Ainslie (angus-akkea)]

Hi Loic

some comments inline

On Mon, Feb 14, 2011 at 2:36 PM, Loïc Minier <email address hidden> wrote:
> Forwarding a chat on this:
>
> 21:53 < lool> nytowl: I find the ethaddr worrying; init=/bin/bash is clearly bad too; console=ttySAC1 sounds like it should be handled like serialtty in the other boards
> 21:54 < lool> nytowl: do you actually need baudrate, bootdelay etc. in there or would the u-boot default be good enough?

I removed the un-necessary varibales

> 21:55 < nytowl> lool, I don't like the ethaddr and bash either but I'd like network to work and init cause a kernel panic
> 21:55 < lool> nytowl: comment on "size is 14 cylinder" doesn't make sense anymore
> 21:55 < nytowl> I could probably get rid of baudrate and bootdelay
> 21:56 < lool> nytowl: I think we need a way to amend the cmdline; if we had this, you could just use this feature to override the default init while the kernel is broken
> 21:56 < lool> nytowl: (if you're tempted to provide this, submit it separately so that it can be merged separately)

The code I've added is a different way of creating a u-boot env. As
the Samsung v310 SMDK does not have any flash on board this is the
only way for me to store the env.

Are you saying you'd like me to override _get_boot_cmd or do you want
me to re-write it so it can deal with systems that don't use a FAT
partition.

> 21:56 < lool> nytowl: seek=1089 sounds like magic, where does it come from?
> 21:57 < lool> same question for seek=9281
> 21:57 < lool> or seek=33
> 21:57 < lool> I understand the seek=1 one though   ;-)
> 21:57 < lool> and then there's this seek=65
> 21:57 < nytowl> Those are magic offsets that irom looks on the SD card for u-boot , uImage and initrd
> 21:57 < nytowl> I guess those should be commented :)

I've fixed these references

>
> 21:58 < lool> nytowl: uses_fat_boot_partition sounds like your board needs a different scheme

I needed some of the functionality provided by the partitioning
mechanism but I don't need a FAT partition. Once the MMC diver for the
Samsung kernel is fixed I should still be able to boot using just a
single EXT2/3 partition.

Is having a boot partition a requirement for using linaro-media-create ?

> 21:59 < nytowl> the boot_env is more than just the command line it is that flashable env I was talking to you about last week
> 21:59 < lool> nytowl: I think this board is substantially different and would warrant some deeper thoughts on how we should boot it and support it; where could I read about how it boots, which software
>              starts from where etc.?

I've changed the way the board boots to closer follow the l-m-c way
but because of the MMC issues I can't finish the job. Are you
suggesting that I don't resubmit this for merge until those issues are
resolved ?

> 21:59 < nytowl> lool there's a little info in the wiki about those offsets
> 22:00 < nytowl> https://wiki.linaro.org/Boards/SMDKV310/Setup
> 22:00 < lool> nytowl: My ideal documentation would be one which walks me through the various stages of the boot up to linux
> 22:01 < nytowl> lool that document doesn't exsist
>

I'll get the boot process documented on the wiki.

[Guilherme Salgado (salgado)]

On Mon, 2011-02-14 at 21:36 +0000, Loïc Minier wrote:
[...]
> 22:05 < lool> nytowl: It seems you have u-boot running and that it's loading kernel and initrd from mmc, so I presume it has mmc support; why wouldn't it be able to load uimage and uinitrd from a MMC
> partition instead of loading them from hardcoded MMC offsets?
> 22:05 < lool> nytowl: Is the datasheet NDA? would I be able to get access to it?
> 22:06 < nytowl> lool, that was going to be step 2
> 22:06 < lool> nytowl: Is it from the datasheet that you found about these values?
> 22:06 < lool> nytowl: I think the l-i-t diff might be much smaller if you were using the same layout with vfat partition etc. as other boards
> 22:06 < nytowl> lool, now those values can from the BSP u-boot and discussions with Samsung
> 22:07 < nytowl> lool, it would be but i wanted to get the board booting first and then start fixing things

Oh, I was assuming we couldn't use the same partition layout on this
board for some reason, but if we can then I really think we should aim
towards that instead of adding complexity to l-m-c just to remove it
later.

[Angus Ainslie (angus-akkea)]

On Wed, Feb 16, 2011 at 5:16 AM, Guilherme Salgado
<email address hidden> wrote:
> On Mon, 2011-02-14 at 21:36 +0000, Loïc Minier wrote:
> [...]
>> 22:05 < lool> nytowl: It seems you have u-boot running and that it's loading kernel and initrd from mmc, so I presume it has mmc support; why wouldn't it be able to load uimage and uinitrd from a MMC
>>               partition instead of loading them from hardcoded MMC offsets?
>> 22:05 < lool> nytowl: Is the datasheet NDA?  would I be able to get access to it?
>> 22:06 < nytowl> lool, that was going to be step 2
>> 22:06 < lool> nytowl: Is it from the datasheet that you found about these values?
>> 22:06 < lool> nytowl: I think the l-i-t diff might be much smaller if you were using the same layout with vfat partition etc. as other boards
>> 22:06 < nytowl> lool, now those values can from the BSP u-boot and discussions with Samsung
>> 22:07 < nytowl> lool, it would be but i wanted to get the board booting first and then start fixing things
>
> Oh, I was assuming we couldn't use the same partition layout on this
> board for some reason, but if we can then I really think we should aim
> towards that instead of adding complexity to l-m-c just to remove it
> later.
>

We can't use the same partition scheme on the Samsung board. It needs
some raw sectors for it's BL1 and u-boot env.

Is there some kind of requirement for having a boot/fat partition ?

Shouldn't having the fat partition be the exception instead of the rule ?

[Loïc Minier (lool)]

On Wed, Feb 16, 2011, Angus Ainslie wrote:
> We can't use the same partition scheme on the Samsung board. It needs
> some raw sectors for it's BL1 and u-boot env.

 We already create a non-FS data partition for mx51 which contains
 u-boot (not the environment, but it's the same thing).

> Is there some kind of requirement for having a boot/fat partition ?

 No; we're trying to use as much common layout across all boards as we
 can as to avoid making the code more complex

> Shouldn't having the fat partition be the exception instead of the rule ?

 I don't see why; it's more convenient to manipulate files with cp/mv
 than dd-ing at specific offsets; also, managing the free space is
 easier. I can also see this being useful for boot scripts, editing the
 environment and more.

--
Loïc Minier

[Angus Ainslie (angus-akkea)]

On Wed, Feb 16, 2011 at 7:52 AM, Loïc Minier <email address hidden> wrote:
> On Wed, Feb 16, 2011, Angus Ainslie wrote:
>> We can't use the same partition scheme on the Samsung board. It needs
>> some raw sectors for it's BL1 and u-boot env.
>
>  We already create a non-FS data partition for mx51 which contains
>  u-boot (not the environment, but it's the same thing).
>

So if I load BL1, env, and u-boot for the non-fs partiton that will be
acceptable ? I can then load the kernel from an ext partition.

>> Shouldn't having the fat partition be the exception instead of the rule ?
>
>  I don't see why; it's more convenient to manipulate files with cp/mv
>  than dd-ing at specific offsets; also, managing the free space is
>  easier.  I can also see this being useful for boot scripts, editing the
>  environment and more.
>

I wasn't thinking use the raw device I was thinking a single rootfs
and boot partition, probably EXT. The question is more why are 2
partitions the default ?

[Loïc Minier (lool)]

On Wed, Feb 16, 2011, Angus Ainslie wrote:
> So if I load BL1, env, and u-boot for the non-fs partiton that will be
> acceptable ?

 That seems about right; ideally, if you could have u-boot on the first
 partition that would be nicer.

 Consider that it will be impossible for people to upgrade from this
 boot layout to another one, so you're bound to support it "forever".
 Linaro doesn't usually prioritize support for upgrading, but it's still
 a good idea to consider it, especially if you take into account that
 Linaro might be used as a base to build real products.

> I can then load the kernel from an ext partition.

 Why ext and not vfat like the other ones?

> I wasn't thinking use the raw device I was thinking a single rootfs
> and boot partition, probably EXT. The question is more why are 2
> partitions the default ?

 Because u-boot doesn't might not support the fancy rootfs scheme which
 you need (e.g. btrfs, LVM...) and can't fsck; so it's easier to read
 the initrd from a simple+solid fs (like vfat) and then have the initrd
 do the heavy lifting, or have linux start the rootfs.

 I bet you're going to ask about initrd next: I'd prefer if we keep
 using initrd for all boards for pretty much the same reasons :-)

--
Loïc Minier

[Angus Ainslie (angus-akkea)]

On Wed, Feb 16, 2011 at 8:26 AM, Loïc Minier <email address hidden> wrote:
> On Wed, Feb 16, 2011, Angus Ainslie wrote:
>> So if I load BL1, env, and u-boot for the non-fs partiton that will be
>> acceptable ?
>
>  That seems about right; ideally, if you could have u-boot on the first
>  partition that would be nicer.
>

Currently Bl1 doesn't support FAT. Samsungs u-boot doesn't either but
that's an easy fix.

>  Consider that it will be impossible for people to upgrade from this
>  boot layout to another one, so you're bound to support it "forever".
>  Linaro doesn't usually prioritize support for upgrading, but it's still
>  a good idea to consider it, especially if you take into account that
>  Linaro might be used as a base to build real products.
>

Why would an upgrade be impossible ? If/when a FAT capable BL1 became
available it could be written to the MMC card, from u-boot, linux or
even a host computer.

>> I can then load the kernel from an ext partition.
>
>  Why ext and not vfat like the other ones?
>
>> I wasn't thinking use the raw device I was thinking a single rootfs
>> and boot partition, probably EXT. The question is more why are 2
>> partitions the default ?
>
>  Because u-boot doesn't might not support the fancy rootfs scheme which
>  you need (e.g. btrfs, LVM...) and can't fsck; so it's easier to read
>  the initrd from a simple+solid fs (like vfat) and then have the initrd
>  do the heavy lifting, or have linux start the rootfs.
>

Well you got me on the fsck :)

>
>  I bet you're going to ask about initrd next: I'd prefer if we keep
>  using initrd for all boards for pretty much the same reasons  :-)
>

No, enough religious debates for one day.

[Loïc Minier (lool)]

On Wed, Feb 16, 2011, Angus Ainslie wrote:
> >  Consider that it will be impossible for people to upgrade from this
> >  boot layout to another one, so you're bound to support it "forever".
> >  Linaro doesn't usually prioritize support for upgrading, but it's still
> >  a good idea to consider it, especially if you take into account that
> >  Linaro might be used as a base to build real products.
>
> Why would an upgrade be impossible ? If/when a FAT capable BL1 became
> available it could be written to the MMC card, from u-boot, linux or
> even a host computer.

 I said it's impossible to upgrade the boot layout, as in to change to a
 different boot layout (moving to having u-boot in vfat for instance).

 Also, consider that your offsets are hardcoded forever and if you ever
 need a larger BL1, u-boot or environment, you're screwed. Obviously
 vfat has some limitations of its own.

--
Loïc Minier

[Angus Ainslie (angus-akkea)]

On Wed, Feb 16, 2011 at 9:07 AM, Loïc Minier <email address hidden> wrote:
> On Wed, Feb 16, 2011, Angus Ainslie wrote:
>> >  Consider that it will be impossible for people to upgrade from this
>> >  boot layout to another one, so you're bound to support it "forever"

[Guilherme Salgado (salgado)]

Hi Angus,

I think this looks quite good; I only have some minor changes to
suggest. I'd just like Loïc to have another look as he reviewed a
previous version of this branch and he may spot things that I've missed.

 review needsfixing

On Thu, 2011-02-17 at 21:52 +0000, Angus Ainslie wrote:
> Angus Ainslie has proposed merging lp:~angus-akkea/linaro-image-tools/Samsung-SMDKV310 into lp:linaro-image-tools.
>
> Requested reviews:
> James Westby (james-w)
> Guilherme Salgado (salgado)
>
> For more details, see:
> https://code.launchpad.net/~angus-akkea/linaro-image-tools/Samsung-SMDKV310/+merge/50237
>
> More Loic recommendations
>
> The SMDKv310 code now follows the recommended boot (FAT) and rootfs (EXT) partition scheme. BL1 , u-boot, the kernel and the initrd all load from the raw mmc offsets currently. Once my MMC issues are resolved I'll be able to load some of these from the FAT partition.
>
> Update code based on Loic comments and fixed test cases
> More changes to fix merge with sfdisk partition changes
>
>
> Add support for Samsung SMDK V310 board
> Use uses_fat_boot_partition = False as an indicator to not create the fat partition
> Add make_flashable_env that will create a binary suitable to burn directly to flash for a u-boot environment.
> add tests
> One more minor change to set the bootm line correctly to mount the initrd
> differences between files attachment (review-diff.txt)
> === modified file 'linaro_media_create/boards.py'
> --- linaro_media_create/boards.py 2011-02-14 16:26:08 +0000
> +++ linaro_media_create/boards.py 2011-02-17 21:51:59 +0000
[...]
> @@ -419,6 +422,67 @@
> cls.load_addr, uboot_parts_dir, cls.kernel_suffix, boot_dir)
> make_uInitrd(uboot_parts_dir, cls.kernel_suffix, boot_dir)
>
> +class SamsungConfig(BoardConfig):
> + boot_env = [
> + 'bootargs=root=/dev/mmcblk0p3 rootwait rw init=/bin/bash console=ttySAC1,115200',

You could at least define a serial_tty attribute here (like in other
config classes) and use that here instead of hard-coding ttySAC1.

> + 'bootcmd=movi read kernel 40007000; movi read rootfs 41000000 600000;'
> + 'bootm 40007000 41000000',
> + 'ethact=smc911x-0',
> + 'ethaddr=00:40:5c:26:0a:5b',

I think some of the lines above are longer than 79 chars?

> + ]
> +
> + @classmethod
> + def get_sfdisk_cmd(cls, should_align_boot_part=False):
> + # Create a fixed-offset bootloader data at
> + # the beginning of the image (size is 214080 512 byte sectors
> + # with the first sector for MBR).
> + loader_start, loader_end, loader_len = align_partition(
> + 1, 214080, 1, PART_ALIGN_S)
> +
> + boot_start, boot_end, boot_len = align_partition(
> + loader_end + 1, BOOT_MIN_SIZE_S, PART_ALIGN_S, PART_ALIGN_S)
> +
> + # we ignore _root_end / _root_len and return a sfdisk command to
> + # instruct the use of all remaining space; XXX if we had some root size
> + # config, we could do something more sensible
> + root_start, _root_end, _root_len = align_partition(
> + boot_end + 1, ROOT_MIN_SIZE_S, PART_ALIGN_S, PART_ALIGN_S...

review: Needs Fixing

[Guilherme Salgado (salgado)]

One thing I've missed...

On Thu, 2011-02-17 at 21:52 +0000, Angus Ainslie wrote:
> === modified file 'linaro_media_create/boards.py'
> --- linaro_media_create/boards.py 2011-02-14 16:26:08 +0000
> +++ linaro_media_create/boards.py 2011-02-17 21:51:59 +0000
> @@ -29,9 +29,11 @@
> import os
> import re
> import tempfile
> +import struct
>
> from linaro_media_create import cmd_runner
> from linaro_media_create.partitions import SECTOR_SIZE
> +from binascii import crc32
>
> # Notes:
> # * geometry is currently always 255 heads and 63 sectors due to limitations of
> @@ -88,6 +90,7 @@
> mmc_option = '0:1'
> mmc_part_offset = 0
> fat_size = 32
> + uses_fat_boot_partition = True

We don't need this any longer.

> === modified file 'linaro_media_create/partitions.py'
> --- linaro_media_create/partitions.py 2011-02-11 15:49:23 +0000
> +++ linaro_media_create/partitions.py 2011-02-17 21:51:59 +0000
> @@ -92,7 +92,7 @@
> else:
> bootfs, rootfs = get_boot_and_root_loopback_devices(media.path)
>
> - if should_format_bootfs:
> + if should_format_bootfs and board_config.uses_fat_boot_partition:

Nor this

> print "\nFormating boot partition\n"
> proc = cmd_runner.run(
> ['mkfs.vfat', '-F', str(board_config.fat_size), bootfs, '-n',

[Loïc Minier (lool)]

On Thu, Feb 17, 2011, Angus Ainslie wrote:
> The SMDKv310 code now follows the recommended boot (FAT) and rootfs
> (EXT) partition scheme. BL1 , u-boot, the kernel and the initrd all
> load from the raw mmc offsets currently. Once my MMC issues are
> resolved I'll be able to load some of these from the FAT partition.

 So overall, the diff is much smaller and it's getting closer to being
 mergeable, thanks for your work; some comments below

> === modified file 'linaro_media_create/boards.py'
> --- linaro_media_create/boards.py 2011-02-14 16:26:08 +0000
> +++ linaro_media_create/boards.py 2011-02-17 21:51:59 +0000
> @@ -419,6 +422,67 @@
> cls.load_addr, uboot_parts_dir, cls.kernel_suffix, boot_dir)
> make_uInitrd(uboot_parts_dir, cls.kernel_suffix, boot_dir)
>
> +class SamsungConfig(BoardConfig):
> + boot_env = [
> + 'bootargs=root=/dev/mmcblk0p3 rootwait rw init=/bin/bash console=ttySAC1,115200',
> + 'bootcmd=movi read kernel 40007000; movi read rootfs 41000000 600000;'
> + 'bootm 40007000 41000000',
> + 'ethact=smc911x-0',
> + 'ethaddr=00:40:5c:26:0a:5b',
> + ]
> +

 Why do we have root=/dev/mmcblk0p3 here and root=/dev/mmcblk0p2 in
 extra_boot_args_options? Both seem incorrect as we use UUID for
 booting. Same remark for init=/bin/bash.

 console= should be inferred from a serialtty variable.

 I don't think we should encode the default u-boot environment into
 linaro-image-tools, but rather into u-boot. So things like
 ethact=smc911x-0 should go into u-boot's config for the board.

 ethaddr= seems broken; this should be read from an EEPROM or should be
 generated by u-boot or by the kernel once, and recorded (see what IGEP
 or beagle xM do in u-boot + linux)

> + @classmethod
> + def get_sfdisk_cmd(cls, should_align_boot_part=False):
> + # Create a fixed-offset bootloader data at
> + # the beginning of the image (size is 214080 512 byte sectors
> + # with the first sector for MBR).
> + loader_start, loader_end, loader_len = align_partition(
> + 1, 214080, 1, PART_ALIGN_S)
> +
> + boot_start, boot_end, boot_len = align_partition(
> + loader_end + 1, BOOT_MIN_SIZE_S, PART_ALIGN_S, PART_ALIGN_S)
> +
> + # we ignore _root_end / _root_len and return a sfdisk command to
> + # instruct the use of all remaining space; XXX if we had some root size
> + # config, we could do something more sensible
> + root_start, _root_end, _root_len = align_partition(
> + boot_end + 1, ROOT_MIN_SIZE_S, PART_ALIGN_S, PART_ALIGN_S)
> +
> + return '%s,%s,0xDA\n%s,%s,0x0C,*\n%s,,,-' % (
> + loader_start, loader_len, boot_start, boot_len, root_start)

 you're creating this partition layout:
           <bootloader>
           <boot>
           <root>
 but I understand that the layout of data on the MMC is essentially:
 +0 <partition table>
 various stuff at fixed offsets:
 +1 u-boot
 +X kernel
 +Y initrd
 +4MiB <rootfs>

 It would seem more sensible to me to have just a bootloader partition
 and a rootfs partition (as long as your u-boot can't read ...

[Loïc Minier (lool)]

On Fri, Feb 18, 2011, Loïc Minier wrote:
> the 16384 KiB are duplicated across:
> a) partition size constraints
> b) in this make_flashable_env() call
> c) in install_smdkv310_boot_env() which has count=32
> please use some constants to compute partition offsets, dd args etc.
> from the same value

 Also, some of these values are duplicated in the u-boot movi bootcmds

--
Loïc Minier

[Angus Ainslie (angus-akkea)]

On Fri, Feb 18, 2011 at 7:58 AM, Loïc Minier <email address hidden> wrote:
> On Thu, Feb 17, 2011, Angus Ainslie wrote:
>> === modified file 'linaro_media_create/boards.py'
>> --- linaro_media_create/boards.py     2011-02-14 16:26:08 +0000
>> +++ linaro_media_create/boards.py     2011-02-17 21:51:59 +0000
>> @@ -419,6 +422,67 @@
>>              cls.load_addr, uboot_parts_dir, cls.kernel_suffix, boot_dir)
>>          make_uInitrd(uboot_parts_dir, cls.kernel_suffix, boot_dir)
>>
>> +class SamsungConfig(BoardConfig):
>> +    boot_env = [
>> +        'bootargs=root=/dev/mmcblk0p3 rootwait rw init=/bin/bash console=ttySAC1,115200',
>> +        'bootcmd=movi read kernel 40007000; movi read rootfs 41000000 600000;'
>> +        'bootm 40007000 41000000',
>> +        'ethact=smc911x-0',
>> +        'ethaddr=00:40:5c:26:0a:5b',
>> +        ]
>> +
>
>  Why do we have root=/dev/mmcblk0p3 here and root=/dev/mmcblk0p2 in
>  extra_boot_args_options?  Both seem incorrect as we use UUID for
>  booting.  Same remark for init=/bin/bash.
>
>  console= should be inferred from a serialtty variable.
>
>  I don't think we should encode the default u-boot environment into
>  linaro-image-tools, but rather into u-boot.  So things like
>  ethact=smc911x-0 should go into u-boot's config for the board.
>

u-boot does not have the facility for creating a flashable environment

>  ethaddr= seems broken; this should be read from an EEPROM or should be
>  generated by u-boot or by the kernel once, and recorded (see what IGEP
>  or beagle xM do in u-boot + linux)
>
>> +    @classmethod
>> +    def get_sfdisk_cmd(cls, should_align_boot_part=False):
>> +        # Create a fixed-offset bootloader data at
>> +        # the beginning of the image (size is 214080 512 byte sectors
>> +        # with the first sector for MBR).
>> +        loader_start, loader_end, loader_len = align_partition(
>> +            1, 214080, 1, PART_ALIGN_S)
>> +
>> +        boot_start, boot_end, boot_len = align_partition(
>> +            loader_end + 1, BOOT_MIN_SIZE_S, PART_ALIGN_S, PART_ALIGN_S)
>> +
>> +        # we ignore _root_end / _root_len and return a sfdisk command to
>> +        # instruct the use of all remaining space; XXX if we had some root size
>> +        # config, we could do something more sensible
>> +        root_start, _root_end, _root_len = align_partition(
>> +            boot_end + 1, ROOT_MIN_SIZE_S, PART_ALIGN_S, PART_ALIGN_S)
>> +
>> +        return '%s,%s,0xDA\n%s,%s,0x0C,*\n%s,,,-' % (
>> +            loader_start, loader_len, boot_start, boot_len, root_start)
>
>  you're creating this partition layout:
>           <bootloader>
>           <boot>
>           <root>
>  but I understand that the layout of data on the MMC is essentially:
>  +0        <partition table>
>  various stuff at fixed offsets:
>  +1        u-boot
>  +X        kernel
>  +Y        initrd
>  +4MiB     <rootfs>
>
>  It would seem more sensible to me to have just a bootloader partition
>  and a rootfs partition (as long as your u-boot can't read from a vfat):
>           <bootloader>
>           <root>
>

During our previous email/irc exchange I suggested just such a layout.
You told me that a vfat boot partition...

[Loïc Minier (lool)]

On Fri, Feb 18, 2011, Angus Ainslie wrote:
> >> +    boot_env = [
> >> +        'bootargs=root=/dev/mmcblk0p3 rootwait rw init=/bin/bash console=ttySAC1,115200',
> >> +        'bootcmd=movi read kernel 40007000; movi read rootfs 41000000 600000;'
> >> +        'bootm 40007000 41000000',
> >> +        'ethact=smc911x-0',
> >> +        'ethaddr=00:40:5c:26:0a:5b',
> >> +        ]
> >  I don't think we should encode the default u-boot environment into
> >  linaro-image-tools, but rather into u-boot.  So things like
> >  ethact=smc911x-0 should go into u-boot's config for the board.
> u-boot does not have the facility for creating a flashable environment

 I'm not sure what you mean; I mean the default u-boot config for these
 values should be in u-boot itself

> During our previous email/irc exchange I suggested just such a layout.
> You told me that a vfat boot partition was not a requirement but then
> went on give quite a number of good reasons why it should be there. So
> now that I've implemented why is the recommendation changing.

 The recommendation is to create /and use/ a vfat partition layout, that
 is actually put kernel and initrd in vfat and load them from there.
 This should be possible, and in fact very very similar to mx5 boards,
 except it's not done due to a but in u-boot. Creating a vfat partition
 and not using it is not what I was suggesting; I was suggesting writing
 only u-boot and u-boot environment to the bootloader partition and
 writing kernel, initrd and boot script on the vfat boot partition -- as
 done on mx5 boards.

> Currently I think this is a bug with my hardware and my hardware only.
> I hope to be receiving replacement hardware sometime next week. If the
> bug turns out to affect more than one board I will file a bug for it

 Good news; we certainly want to add code to support a single broken
 board though, so I guess the code should use a vfat partition and
 u-boot should fatload kernel and initrd from it, if the non-broken
 boards allow this.

> >> +    @classmethod
> >> +    def _make_boot_files(cls, uboot_parts_dir, boot_cmd, chroot_dir,
> >> +                         boot_dir, boot_script, boot_device_or_file):
> >> +        uboot_file = os.path.join(
> >> +            chroot_dir, 'usr', 'lib', 'u-boot', 'smdkv310', 'u-boot.v310')
> >
> >  I know it's a weird question, but why is the file suffix u-boot.v310?
> >  Is it a special format that only the v310 supports?
> Yes this is a monolithic u-boot plus the BL1 I've been telling you
> about. The landing team is in the process of splitting BL1 and u-boot.
> Once this is done we'll be in a similar situation to the x-loader +
> u-boot boards.

 Hmm ok

> >> +    proc = cmd_runner.run([
> >> +        "mv",
> >> +        "%s" % tmpfile,
> >> +        "%s" % env_file], as_root=True)
> >> +    proc.wait()
> >
> >  Why move the file in place instead of generating it directly at the
> >  right location?
>
> If I try to generate it directly to the bootfs I get a permission error

 Do you want to keep boot_env.flash on disk? It might have incorrect
 ownership as a result of mv (might be owned by the user id of the
 person running l-i-t); it's not actually a prob...

[Angus Ainslie (angus-akkea)]

On Fri, Feb 18, 2011 at 9:33 AM, Loïc Minier <email address hidden> wrote:
> On Fri, Feb 18, 2011, Angus Ainslie wrote:
>> >> +    boot_env = [
>> >> +        'bootargs=root=/dev/mmcblk0p3 rootwait rw init=/bin/bash console=ttySAC1,115200',
>> >> +        'bootcmd=movi read kernel 40007000; movi read rootfs 41000000 600000;'
>> >> +        'bootm 40007000 41000000',
>> >> +        'ethact=smc911x-0',
>> >> +        'ethaddr=00:40:5c:26:0a:5b',
>> >> +        ]
>> >  I don't think we should encode the default u-boot environment into
>> >  linaro-image-tools, but rather into u-boot.  So things like
>> >  ethact=smc911x-0 should go into u-boot's config for the board.
>> u-boot does not have the facility for creating a flashable environment
>
>  I'm not sure what you mean; I mean the default u-boot config for these
>  values should be in u-boot itself
>

The u-boot code is probably going to b shard with the c210 u-boot code
so I thought encoding some of these board specific values into the env
was less broken that hard coding them into u-boot.

[Loïc Minier (lool)]

On Fri, Feb 18, 2011, Angus Ainslie wrote:
> The u-boot code is probably going to b shard with the c210 u-boot code
> so I thought encoding some of these board specific values into the env
> was less broken that hard coding them into u-boot.

 I'm not saying that the env should not be able to override them, but
 the default should be correct in u-boot to start with -- that is, if
 the env isn't read, u-boot still does the right thing. I'm thinking of
 it like booting a kernel with an empty cmdline.

--
Loïc Minier

[Angus Ainslie (angus-akkea)]

On Fri, Feb 18, 2011 at 10:34 AM, Loïc Minier <email address hidden> wrote:
>  I'm not saying that the env should not be able to override them, but
>  the default should be correct in u-boot to start with -- that is, if
>  the env isn't read, u-boot still does the right thing.  I'm thinking of
>  it like booting a kernel with an empty cmdline

As FAT support is currently not working for my u-boot/hardware this is
my way of overriding the env.

Is my merge going to be denied until FAT support is working ?

[Loïc Minier (lool)]

On Fri, Feb 18, 2011, Angus Ainslie wrote:
> As FAT support is currently not working for my u-boot/hardware this is
> my way of overriding the env.

 So with all other boards, we don't have any u-boot environment at all;
 we just set bootcmd via a boot script. u-boot only uses its defaults
 from the build-time config, which includes a bootcmd which loads a
 boot.scr, and that boot.scr is generated by linaro-image-tools just to
 set the kernel cmdline.

 I understand that you can't have a boot script because you can't load
 it from vfat, but I don't understand why we'd have to set all these
 things in the u-boot environment instead of just having the u-boot
 defaults be correct?

> Is my merge going to be denied until FAT support is working ?

 Eh, I realize this is frustrating; I don't mind that we carry an
 intermediate version which doesn't use FAT, but only if the root issues
 are tracked and being worked on; if you say it's just your board which
 has this issue, then certainly we should use FAT; right?

--
Loïc Minier

[Loïc Minier (lool)]

        Hi,

 @Angus: In lp:~lool/linaro-image-tools/samsung-v310, I've prepared some
 changes on top of lp:~angus-akkea/linaro-image-tools/Samsung-SMDKV310.
 This mainly reworks the partitioning to use just a bootloader partition
 and compute offsets/lengths from a set of constants.

 Could you take a look and consider merging it in your own branch? I
 obviously can't test it, but it passes tests.

 I have a new question with the current implementation: why dd the first
 and last sectors of your u-boot file separately instead of dd-ing the
 whole file and then replacing the environment?

   Bye,
--
Loïc Minier

[Angus Ainslie (angus-akkea)]

On Mon, Feb 21, 2011 at 4:55 PM, Loïc Minier <email address hidden> wrote:
>        Hi,
>
>  @Angus: In lp:~lool/linaro-image-tools/samsung-v310, I've prepared some
>  changes on top of lp:~angus-akkea/linaro-image-tools/Samsung-SMDKV310.
>  This mainly reworks the partitioning to use just a bootloader partition
>  and compute offsets/lengths from a set of constants.
>
>  Could you take a look and consider merging it in your own branch?  I
>  obviously can't test it, but it passes tests.
>

Sure, once I get my new board I'll be looking at the l-m-c code again.

>  I have a new question with the current implementation: why dd the first
>  and last sectors of your u-boot file separately instead of dd-ing the
>  whole file and then replacing the environment?
>

Before I wrote the make_flashable_env method I chose not to overwrite
the env portion of the MMC card.

Angus

[Loïc Minier (lool)]

        Hey Angus

 I've started from your branch and:
 * did a fake merge with mine as to have the history from my "use
   constants for offsets" commits appear
 * merged it with lp:linaro-image-tools as it was based on a slightly
   older version and had some conflicts
 * reworked it a bit, mostly to have less common code specifically for
   Samsung; the biggest change is that I'm passing a boot_env dict
   around _make_boot_files calls instead of the text of boot commands;
   also, I've dropped the 4 MiB alignment of the bootloader partition as
   to have it start at the beginning of the SD card

 The current layout does create a boot partition (a vfat) but it's not
 actually used for anything except to store uImage and uInitrd. I've
 kept the boot.scr generation commented out as the contents of it were
 completely unused, but it's easy to enable again when the FAT partition
 is actually in use.

 If you're ok with the changes, I'm ok to merge this the above new
 branch(if other reviewers are ok as well).

   Cheers,

 Note for self: there are a couple of things I would like to do before
 doing the final merge:
 * replace "movi read kernel 40007000" with %(kernel_addr), but I don't
   know whether movi accepts addresses prefixed with 0x
 * add tests to some new functions like make_flashable_env(),
   install_smdkv310_uImage() etc.

--
Loïc Minier

[Angus Ainslie (angus-akkea)]

Hi Loic,

On Mon, Feb 28, 2011 at 6:59 PM, Loïc Minier <email address hidden> wrote:
>  Note for self: there are a couple of things I would like to do before
>  doing the final merge:
>  * replace "movi read kernel 40007000" with %(kernel_addr), but I don't
>   know whether movi accepts addresses prefixed with 0x

The board still boots fine if those constants are prefixed with '0x'

>  * add tests to some new functions like make_flashable_env(),
>   install_smdkv310_uImage() etc.
>

How would those be tested ? Write them out to a file and then do an md5sum ?

Angus

[Loïc Minier (lool)]

On Tue, Mar 01, 2011, Angus Ainslie wrote:
> The board still boots fine if those constants are prefixed with '0x'

 Ok; in r317 of my branch the code is now using cls.kernel_addr and
 initrd_addr. What's the 600000 value after rootfs?

> >  * add tests to some new functions like make_flashable_env(),
> >   install_smdkv310_uImage() etc.
> How would those be tested ? Write them out to a file and then do an md5sum ?

 Functions like install_smdkv310_uImage() could be tested with a mock of
 the _dd function, or more simply with a Popen mock.

 For make_flashable_env() I would probably write the reverse function
 creating a dict from a binary environment, then checking with
 unpack_env(make_flashable_env(d)) is the same as d.

--
Loïc Minier

[Angus Ainslie (angus-akkea)]

On Tue, Mar 1, 2011 at 8:02 AM, Loïc Minier <email address hidden> wrote:
> On Tue, Mar 01, 2011, Angus Ainslie wrote:
>> The board still boots fine if those constants are prefixed with '0x'
>
>  Ok; in r317 of my branch the code is now using cls.kernel_addr and
>  initrd_addr.  What's the 600000 value after rootfs?
>

It's the size of the rootfs. I see another constant coming

--
Angus Ainslie <email address hidden>
Team Lead, Samsung Landing Team

[Loïc Minier (lool)]

On Tue, Mar 01, 2011, Angus Ainslie wrote:
> It's the size of the rootfs. I see another constant coming

 The thing is that it doesn't match the size of the initrd in the
 partition layout: I had computed SAMSUNG_V310_UINITRD_LEN = 204800s by
 looking at the existing offsets from you older branch (which is exactly
 100 MiB).

 If 0x600000 bytes are enough, I just need to update
 SAMSUNG_V310_UINITRD_LEN to 0x600000/512 and use that in the movi
 instructions as well; does that make sense?

--
Loïc Minier

[Loïc Minier (lool)]

On Tue, Mar 01, 2011, Loïc Minier wrote:
> If 0x600000 bytes are enough, I just need to update
> SAMSUNG_V310_UINITRD_LEN to 0x600000/512 and use that in the movi
> instructions as well; does that make sense?

 Angus, could you check out r318 in
     lp:~lool/linaro-image-tools/samsung-v310-v2 ?
 it moves to 6 MiB instead of 100 MiB; this reduces the image size
 significantly. (It also prefixes the length with 0x)

 I'm pretty sure our initrd are more than 6 MiB, so I suspect that this
 will break, but that seems to point out a problem with the current movi
 commands which are only 6 MiB when the initrd is actually longer, so
 presumably initrd unpacking was failing?

--
Loïc Minier

[Angus Ainslie (angus-akkea)]

On Tue, Mar 1, 2011 at 9:02 AM, Loïc Minier <email address hidden> wrote:
> On Tue, Mar 01, 2011, Angus Ainslie wrote:
>> It's the size of the rootfs. I see another constant coming
>
>  The thing is that it doesn't match the size of the initrd in the
>  partition layout: I had computed SAMSUNG_V310_UINITRD_LEN = 204800s by
>  looking at the existing offsets from you older branch (which is exactly
>  100 MiB).
>
>  If 0x600000 bytes are enough, I just need to update
>  SAMSUNG_V310_UINITRD_LEN to 0x600000/512 and use that in the movi
>  instructions as well; does that make sense?
>

The 100MiB is hardcoded into BL0 ( the ROM loader ) so I think that we
may nee 2 constants. One for the amount of space reserved for uinitrd
and one for how much we want to copy from there.

> --
> Loïc Minier
>
> https://code.launchpad.net/~angus-akkea/linaro-image-tools/Samsung-SMDKV310/+merge/51646
> You are the owner of lp:~angus-akkea/linaro-image-tools/Samsung-SMDKV310.
>

--
Angus Ainslie <email address hidden>
Team Lead, Samsung Landing Team

[Loïc Minier (lool)]

On Tue, Mar 01, 2011, Angus Ainslie wrote:
> The 100MiB is hardcoded into BL0 ( the ROM loader ) so I think that we
> may nee 2 constants. One for the amount of space reserved for uinitrd
> and one for how much we want to copy from there.

 Would it be too painful to copy the whole 100 MiB?

 Or could we come to an agreement to use e.g. 30 MiB and set that in
 both BL0 and linaro-image-tools movi commands?

--
Loïc Minier

[Angus Ainslie (angus-akkea)]

On Tue, Mar 1, 2011 at 9:56 AM, Loïc Minier <email address hidden> wrote:
> On Tue, Mar 01, 2011, Angus Ainslie wrote:
>> The 100MiB is hardcoded into BL0 ( the ROM loader ) so I think that we
>> may nee 2 constants. One for the amount of space reserved for uinitrd
>> and one for how much we want to copy from there.
>
>  Would it be too painful to copy the whole 100 MiB?
>

IIRC that took the boot from seconds to minutes.

>  Or could we come to an agreement to use e.g. 30 MiB and set that in
>  both BL0 and linaro-image-tools movi commands?
>

If I understand correctly BL0 is burned into the SoC somehow so we
can't get source and it won't be changed.

I'll create 2 constants SAMSUNG_V310_UINITRD_RESERVED_LEN and
SAMSUNG_V310_UINITRD_COPY_LEN.

--
Angus Ainslie <email address hidden>
Team Lead, Samsung Landing Team

[Loïc Minier (lool)]

 review approve

 I'm fine merging this; the two things I'd fix at merge time are:

On Tue, Mar 01, 2011, Angus Ainslie wrote:
> def _get_mlo_file(chroot_dir):
> @@ -549,3 +730,41 @@
> proc = cmd_runner.run(
> ["cp", "-v", boot_script, "%s/boot.ini" % boot_disk], as_root=True)
> proc.wait()
> +
> + return "%s/boot.ini" % boot_disk

 this (which I understand was requested by another reviewer in a prior
 review); don't think the return value is used anywhere, so I'd rather
 not have it

 and the other thing is that I'd fix the relatively trivial conflicts
 with tip

--
Loïc Minier

review: Approve

[Guilherme Salgado (salgado)]

Hi Angus,

This looks good in general, but there are some things which are not
clear to me and others which need changing. Once you're done with the
changes, just reply here and I'll have another look; there's no need to
re-submit the merge-proposal.

 review needsfixing

On Wed, 2011-03-02 at 14:59 +0000, Angus Ainslie wrote:
[...]
> === modified file 'linaro_media_create/boards.py'
> --- linaro_media_create/boards.py 2011-03-01 16:23:52 +0000
> +++ linaro_media_create/boards.py 2011-03-02 14:59:10 +0000
> @@ -27,9 +27,12 @@
> import glob
> import os
> import re
> +import tempfile
> +import struct
>
> from linaro_media_create import cmd_runner
> from linaro_media_create.partitions import SECTOR_SIZE
> +from binascii import crc32

This import should be together with the other ones from the standard
library.

>
> # Notes:
> # * geometry is currently always 255 heads and 63 sectors due to limitations of
> @@ -61,6 +64,44 @@
> # root partition; at least 50 MiB; XXX this shouldn't be hardcoded
> ROOT_MIN_SIZE_S = align_up(50 * 1024 * 1024, SECTOR_SIZE) / SECTOR_SIZE
>
> +# Samsung v310 implementation notes
> +# * BL1 (SPL) is expected at offset +1s and is 32s long
> +# * BL2 (u-boot) is loaded at a raw MMC offset of +65s by BL1 which currently
> +# doesn't support FAT
> +# * the u-boot environment is at +33s and is 32s long (16 KiB)
> +# * currently, some hardware issues on certain boards causes u-boot to not be
> +# able to use FAT to load uImage and uInitrd (or boot.scr); as a temporary
> +# workaround, these are loaded from +1089s and +9281s respectively
> +# * hence we hardcode all offsets, make sure that the files aren't larger than
> +# their reserved spot, and create a bootloader partition from the first
> +# sector after MBR up to end of initrd
> +SAMSUNG_V310_BL1_START = 1
> +SAMSUNG_V310_BL1_LEN = 32
> +SAMSUNG_V310_ENV_START = SAMSUNG_V310_BL1_START + SAMSUNG_V310_BL1_LEN
> +SAMSUNG_V310_ENV_LEN = 32
> +assert SAMSUNG_V310_ENV_START == 33, "BL1 expects u-boot environment at +33s"
> +assert SAMSUNG_V310_ENV_LEN * SECTOR_SIZE == 16 * 1024, (
> + "BL1 expects u-boot environment to be 16 KiB")
> +SAMSUNG_V310_BL2_START = SAMSUNG_V310_ENV_START + SAMSUNG_V310_ENV_LEN
> +SAMSUNG_V310_BL2_LEN = 1024
> +assert SAMSUNG_V310_BL2_LEN * SECTOR_SIZE == 512 * 1024, (
> + "BL1 expects BL2 (u-boot) to be 512 KiB")
> +SAMSUNG_V310_UIMAGE_START = SAMSUNG_V310_BL2_START + SAMSUNG_V310_BL2_LEN
> +SAMSUNG_V310_UIMAGE_LEN = 8192
> +assert SAMSUNG_V310_UIMAGE_START == 1089, (
> + "BL2 (u-boot) expects uImage at +1089s")
> +assert SAMSUNG_V310_UIMAGE_LEN * SECTOR_SIZE == 4 * 1024 * 1024, (
> + "BL2 (u-boot) expects uImage to be 4 MiB")
> +SAMSUNG_V310_UINITRD_START = (
> + SAMSUNG_V310_UIMAGE_START + SAMSUNG_V310_UIMAGE_LEN)
> +SAMSUNG_V310_UINITRD_RESERVED_LEN = 204800
> +SAMSUNG_V310_UINITRD_COPY_LEN = 32768
> +assert SAMSUNG_V310_UINITRD_START == 9281, (
> + "BL2 (u-boot) expects uInitrd at +9281s")
> +assert SAMSUNG_V310_UINITRD_RESERVED_LEN * SECTOR_SIZE == 100 * 1024 * 1024, (
> + "BL2 (u-boot) expects uInitrd to be 100 MiB")
> +assert SAMSUNG_V310_UINITRD_COPY_LEN * SECTOR_SIZE == 16 * 1024 * 1024, (
> + "Only c...

review: Needs Fixing

[Loïc Minier (lool)]

        Angus, Guilherme,

 For some of the comments which I quote from Guilherme below, I'm the
 original author of the change and am happy to provide fixes:

On Wed, Mar 02, 2011, Guilherme Salgado wrote:
> > - def _get_boot_cmd(cls, is_live, is_lowmem, consoles, rootfs_uuid):
> > - """Get the boot command for this board.
> > + def _get_bootcmd(cls):
> Since this doesn't take any arguments it'd be nicer if it was a property
> (you just have to decorate it with @classproperty instead of
> @classmethod). Would you mind doing that?

 Makes sense to me

> > +def _dd(input_file, output_file, block_size=SECTOR_SIZE, count=None, seek=None,
> > + skip=None):
> Care to add a docstring to this function?

 Ack

> > + boot_script_data = (
> > + "setenv bootcmd '%(bootcmd)s'\n"
> > + "setenv bootargs '%(bootargs)s'\n"
> > + "boot"
> > + % boot_env)
> The indentation here is one level off.

 Ack

> > + # XXX need to check that the length of imx_file is smaller than
> > + # LOADER_MIN_SIZE_S
> Is it hard to do this or can we do it now? If there's any reason for
> not doing it now, would you mind filing a bug and putting a link to the
> bug here?

 No; this wasn't properly done for other boards and when I realized this
 I filed LP #722650; it was just distracting for me to do it
 immediately.

> > +def install_smdkv310_boot_env(env_file, boot_device_or_file):
> > + # XXX need to check that the length of env_file is smaller than
> > + # SAMSUNG_V310_ENV_LEN
> > + _dd(env_file, boot_device_or_file, count=SAMSUNG_V310_ENV_LEN,
> > + seek=SAMSUNG_V310_ENV_START)
>
> Again, if there's any reason why we can't do the 3 checks above now,
> there should be bugs filed and a link to them here.

 Same

> > +def install_smdkv310_boot_loader(v310_file, boot_device_or_file):
> > + # v310_file is a binary with the same layout as BL1 + u-boot environment +
> > + # BL2; write BL1 (SPL) piece first (SAMSUNG_V310_BL1_LEN sectors at +0s in
> > + # the file and +SAMSUNG_V310_BL1_START on disk), then write BL2 (u-boot)
> > + # piece (rest of the file starting at +(SAMSUNG_V310_BL1_LEN +
> > + # SAMSUNG_V310_ENV_LEN)s in the file which is the same as
> > + # +(SAMSUNG_V310_BL2_START - SAMSUNG_V310_BL1_START)s)
>
> Care to convert this into a docstring? It'd also be nice if you could
> expand abbreviations like BL* and SPL the first time they're used.

 Ok; BL0 is Samsung specific terminology for the first stage bootloader,
 BL1 is the SPL (secondary program loader)

> > - 'sudo', 'dd', 'if=imx_file', 'of=boot_device_or_file', 'bs=1024',
> > - 'seek=1', 'conv=notrunc']
> > + 'sudo', 'dd', 'if=imx_file', 'of=boot_device_or_file', 'bs=512',
> > + 'conv=notrunc', 'seek=2']
>
> Why are we using bs=512 and seek=2 in install_mx5_boot_loader() now?
> That surely isn't related to support for your board, is it?

 Hmm I'm not sure where my other commit went, but this was preparation
 to use the _dd() function which defaults to SECTOR_SIZE (512); this
 makes install_mx5_boot_loader() much nicer afterwards.

--
Loïc Minier

[Angus Ainslie (angus-akkea)]

James,

Do you have any more comments before I resubmit ?

Angus

On Wed, Mar 2, 2011 at 12:28 PM, Loïc Minier <email address hidden> wrote:
>        Angus, Guilherme,
>
>  For some of the comments which I quote from Guilherme below, I'm the
>  original author of the change and am happy to provide fixes:
>
> On Wed, Mar 02, 2011, Guilherme Salgado wrote:
>> > -    def _get_boot_cmd(cls, is_live, is_lowmem, consoles, rootfs_uuid):
>> > -        """Get the boot command for this board.
>> > +    def _get_bootcmd(cls):
>> Since this doesn't take any arguments it'd be nicer if it was a property
>> (you just have to decorate it with @classproperty instead of
>> @classmethod).  Would you mind doing that?
>
>  Makes sense to me
>
>> > +def _dd(input_file, output_file, block_size=SECTOR_SIZE, count=None, seek=None,
>> > +        skip=None):
>> Care to add a docstring to this function?
>
>  Ack
>
>> > +    boot_script_data = (
>> > +            "setenv bootcmd '%(bootcmd)s'\n"
>> > +            "setenv bootargs '%(bootargs)s'\n"
>> > +            "boot"
>> > +            % boot_env)
>> The indentation here is one level off.
>
>  Ack
>
>> > +    # XXX need to check that the length of imx_file is smaller than
>> > +    # LOADER_MIN_SIZE_S
>> Is it hard to do this or can we do it now?  If there's any reason for
>> not doing it now, would you mind filing a bug and putting a link to the
>> bug here?
>
>  No; this wasn't properly done for other boards and when I realized this
>  I filed LP #722650; it was just distracting for me to do it
>  immediately.
>
>> > +def install_smdkv310_boot_env(env_file, boot_device_or_file):
>> > +    # XXX need to check that the length of env_file is smaller than
>> > +    # SAMSUNG_V310_ENV_LEN
>> > +    _dd(env_file, boot_device_or_file, count=SAMSUNG_V310_ENV_LEN,
>> > +        seek=SAMSUNG_V310_ENV_START)
>>
>> Again, if there's any reason why we can't do the 3 checks above now,
>> there should be bugs filed and a link to them here.
>
>  Same
>
>> > +def install_smdkv310_boot_loader(v310_file, boot_device_or_file):
>> > +    # v310_file is a binary with the same layout as BL1 + u-boot environment +
>> > +    # BL2; write BL1 (SPL) piece first (SAMSUNG_V310_BL1_LEN sectors at +0s in
>> > +    # the file and +SAMSUNG_V310_BL1_START on disk), then write BL2 (u-boot)
>> > +    # piece (rest of the file starting at +(SAMSUNG_V310_BL1_LEN +
>> > +    # SAMSUNG_V310_ENV_LEN)s in the file which is the same as
>> > +    # +(SAMSUNG_V310_BL2_START - SAMSUNG_V310_BL1_START)s)
>>
>> Care to convert this into a docstring?  It'd also be nice if you could
>> expand abbreviations like BL* and SPL the first time they're used.
>
>  Ok; BL0 is Samsung specific terminology for the first stage bootloader,
>  BL1 is the SPL (secondary program loader)
>
>> > -            'sudo', 'dd', 'if=imx_file', 'of=boot_device_or_file', 'bs=1024',
>> > -            'seek=1', 'conv=notrunc']
>> > +            'sudo', 'dd', 'if=imx_file', 'of=boot_device_or_file', 'bs=512',
>> > +            'conv=notrunc', 'seek=2']
>>
>> Why are we using bs=512 and seek=2 in install_mx5_boot_loader() now?
>> That surely isn't related to support for your board, is it?
>
>  Hmm I'm ...

[Guilherme Salgado (salgado)]

On Wed, 2011-03-02 at 19:28 +0000, Loïc Minier wrote:
> Angus, Guilherme,
>
> For some of the comments which I quote from Guilherme below, I'm the
> original author of the change and am happy to provide fixes:

Cool, thanks Loïc.

> > > + # XXX need to check that the length of imx_file is smaller than
> > > + # LOADER_MIN_SIZE_S
> > Is it hard to do this or can we do it now? If there's any reason for
> > not doing it now, would you mind filing a bug and putting a link to the
> > bug here?
[...]
> > > - 'sudo', 'dd', 'if=imx_file', 'of=boot_device_or_file', 'bs=1024',
> > > - 'seek=1', 'conv=notrunc']
> > > + 'sudo', 'dd', 'if=imx_file', 'of=boot_device_or_file', 'bs=512',
> > > + 'conv=notrunc', 'seek=2']
> >
> > Why are we using bs=512 and seek=2 in install_mx5_boot_loader() now?
> > That surely isn't related to support for your board, is it?
>
> Hmm I'm not sure where my other commit went, but this was preparation
> to use the _dd() function which defaults to SECTOR_SIZE (512); this
> makes install_mx5_boot_loader() much nicer afterwards.

Well, you could just pass bs=1024 and seek=1 to the _dd() call in
install_mx5_boot_loader() but I guess passing seek=2 and using the
default bs (512) has the same effect?

--
Guilherme Salgado <https://launchpad.net/~salgado>

[lp:~angus-akkea/linaro-image-tools/Samsung-SMDKV310]

308. By Angus Ainslie on 2011-03-02

fix review comments from salgado

[Loïc Minier (lool)]

On Wed, Mar 02, 2011, Guilherme Salgado wrote:
> Well, you could just pass bs=1024 and seek=1 to the _dd() call in
> install_mx5_boot_loader() but I guess passing seek=2 and using the
> default bs (512) has the same effect?

 Yes; it's exactly the same thing; it was just a bit nicer to express
 the offset in sectors (2) instead of absolute (1024) since dd works on
 blocks which are by default of block_size=CLUSTER_SIZE.

--
Loïc Minier

[James Westby (james-w)]

Nope, I'm happy with Loïc and Guilherme reviewing, and think we should
get this landed :-)

Thanks,

James

On Wed, 2 Mar 2011 12:47:17 -0700, Angus Ainslie <email address hidden> wrote:
> James,
>
> Do you have any more comments before I resubmit ?
>
> Angus
>
> On Wed, Mar 2, 2011 at 12:28 PM, Loïc Minier <email address hidden> wrote:
> >        Angus, Guilherme,
> >
> >  For some of the comments which I quote from Guilherme below, I'm the
> >  original author of the change and am happy to provide fixes:
> >
> > On Wed, Mar 02, 2011, Guilherme Salgado wrote:
> >> > -    def _get_boot_cmd(cls, is_live, is_lowmem, consoles, rootfs_uuid):
> >> > -        """Get the boot command for this board.
> >> > +    def _get_bootcmd(cls):
> >> Since this doesn't take any arguments it'd be nicer if it was a property
> >> (you just have to decorate it with @classproperty instead of
> >> @classmethod).  Would you mind doing that?
> >
> >  Makes sense to me
> >
> >> > +def _dd(input_file, output_file, block_size=SECTOR_SIZE, count=None, seek=None,
> >> > +        skip=None):
> >> Care to add a docstring to this function?
> >
> >  Ack
> >
> >> > +    boot_script_data = (
> >> > +            "setenv bootcmd '%(bootcmd)s'\n"
> >> > +            "setenv bootargs '%(bootargs)s'\n"
> >> > +            "boot"
> >> > +            % boot_env)
> >> The indentation here is one level off.
> >
> >  Ack
> >
> >> > +    # XXX need to check that the length of imx_file is smaller than
> >> > +    # LOADER_MIN_SIZE_S
> >> Is it hard to do this or can we do it now?  If there's any reason for
> >> not doing it now, would you mind filing a bug and putting a link to the
> >> bug here?
> >
> >  No; this wasn't properly done for other boards and when I realized this
> >  I filed LP #722650; it was just distracting for me to do it
> >  immediately.
> >
> >> > +def install_smdkv310_boot_env(env_file, boot_device_or_file):
> >> > +    # XXX need to check that the length of env_file is smaller than
> >> > +    # SAMSUNG_V310_ENV_LEN
> >> > +    _dd(env_file, boot_device_or_file, count=SAMSUNG_V310_ENV_LEN,
> >> > +        seek=SAMSUNG_V310_ENV_START)
> >>
> >> Again, if there's any reason why we can't do the 3 checks above now,
> >> there should be bugs filed and a link to them here.
> >
> >  Same
> >
> >> > +def install_smdkv310_boot_loader(v310_file, boot_device_or_file):
> >> > +    # v310_file is a binary with the same layout as BL1 + u-boot environment +
> >> > +    # BL2; write BL1 (SPL) piece first (SAMSUNG_V310_BL1_LEN sectors at +0s in
> >> > +    # the file and +SAMSUNG_V310_BL1_START on disk), then write BL2 (u-boot)
> >> > +    # piece (rest of the file starting at +(SAMSUNG_V310_BL1_LEN +
> >> > +    # SAMSUNG_V310_ENV_LEN)s in the file which is the same as
> >> > +    # +(SAMSUNG_V310_BL2_START - SAMSUNG_V310_BL1_START)s)
> >>
> >> Care to convert this into a docstring?  It'd also be nice if you could
> >> expand abbreviations like BL* and SPL the first time they're used.
> >
> >  Ok; BL0 is Samsung specific terminology for the first stage bootloader,
> >  BL1 is the SPL (secondary program loader)
> >
> >> > -            'sudo', 'dd', 'if=imx_file', 'of=boot_device_or_fi...

[Loïc Minier (lool)]

 I've proposed merging lp:~lool/linaro-image-tools/samsung-v310-v3 into
 lp:~angus-akkea/linaro-image-tools/Samsung-SMDKV310; see:
https://code.launchpad.net/~lool/linaro-image-tools/samsung-v310-v3/+merge/52274

 I think it addresses all issues; not all Samsung install() functions
 are tested, but they are trivially identical to the mx5 one.

--
Loïc Minier

[lp:~angus-akkea/linaro-image-tools/Samsung-SMDKV310]

309. By Angus Ainslie on 2011-03-05

Merge in Guilherme suggestions for Loic's brnach

[Loïc Minier (lool)]

Looks good; thanks

review: Approve

[Guilherme Salgado (salgado)]

Looks good to me, but there's a new conflict that needs to be resolved

review: Approve

=== modified file 'linaro_media_create/boards.py'
--- linaro_media_create/boards.py	2011-03-03 13:44:11 +0000
+++ linaro_media_create/boards.py	2011-03-05 19:28:12 +0000
@@ -28,7 +28,13 @@
 import glob
 import os
 import re
-import tempfile
+<<<<<<< TREE
+import tempfile
+=======
+import tempfile
+import struct
+from binascii import crc32
+>>>>>>> MERGE-SOURCE
 
 from linaro_media_create import cmd_runner
 from linaro_media_create.partitions import SECTOR_SIZE
@@ -63,6 +69,44 @@
 # root partition; at least 50 MiB; XXX this shouldn't be hardcoded
 ROOT_MIN_SIZE_S = align_up(50 * 1024 * 1024, SECTOR_SIZE) / SECTOR_SIZE
 
+# Samsung v310 implementation notes
+# * BL1 (SPL) is expected at offset +1s and is 32s long
+# * BL2 (u-boot) is loaded at a raw MMC offset of +65s by BL1 which currently
+# doesn't support FAT
+# * the u-boot environment is at +33s and is 32s long (16 KiB)
+# * currently, some hardware issues on certain boards causes u-boot to not be
+# able to use FAT to load uImage and uInitrd (or boot.scr); as a temporary
+# workaround, these are loaded from +1089s and +9281s respectively
+# * hence we hardcode all offsets, make sure that the files aren't larger than
+# their reserved spot, and create a bootloader partition from the first
+# sector after MBR up to end of initrd
+SAMSUNG_V310_BL1_START = 1
+SAMSUNG_V310_BL1_LEN = 32
+SAMSUNG_V310_ENV_START = SAMSUNG_V310_BL1_START + SAMSUNG_V310_BL1_LEN
+SAMSUNG_V310_ENV_LEN = 32
+assert SAMSUNG_V310_ENV_START == 33, "BL1 expects u-boot environment at +33s"
+assert SAMSUNG_V310_ENV_LEN * SECTOR_SIZE == 16 * 1024, (
+    "BL1 expects u-boot environment to be 16 KiB")
+SAMSUNG_V310_BL2_START = SAMSUNG_V310_ENV_START + SAMSUNG_V310_ENV_LEN
+SAMSUNG_V310_BL2_LEN = 1024
+assert SAMSUNG_V310_BL2_LEN * SECTOR_SIZE == 512 * 1024, (
+    "BL1 expects BL2 (u-boot) to be 512 KiB")
+SAMSUNG_V310_UIMAGE_START = SAMSUNG_V310_BL2_START + SAMSUNG_V310_BL2_LEN
+SAMSUNG_V310_UIMAGE_LEN = 8192
+assert SAMSUNG_V310_UIMAGE_START == 1089, (
+    "BL2 (u-boot) expects uImage at +1089s")
+assert SAMSUNG_V310_UIMAGE_LEN * SECTOR_SIZE == 4 * 1024 * 1024, (
+    "BL2 (u-boot) expects uImage to be 4 MiB")
+SAMSUNG_V310_UINITRD_START = (
+    SAMSUNG_V310_UIMAGE_START + SAMSUNG_V310_UIMAGE_LEN)
+SAMSUNG_V310_UINITRD_RESERVED_LEN = 204800
+SAMSUNG_V310_UINITRD_COPY_LEN = 32768
+assert SAMSUNG_V310_UINITRD_START == 9281, (
+    "BL2 (u-boot) expects uInitrd at +9281s")
+assert SAMSUNG_V310_UINITRD_RESERVED_LEN * SECTOR_SIZE == 100 * 1024 * 1024, (
+    "BL2 (u-boot) expects uInitrd to be 100 MiB")
+assert SAMSUNG_V310_UINITRD_COPY_LEN * SECTOR_SIZE == 16 * 1024 * 1024, (
+    "Only copy 16MiB for a faster boot")
 
 def align_partition(min_start, min_length, start_alignment, end_alignment):
     """Compute partition start and end offsets based on specified constraints.
@@ -81,6 +125,14 @@
     return start, end, length
 
 
+class classproperty(object):
+    """A descriptor that provides @property behavior on class methods."""
+    def __init__(self, getter):
+        self.getter = getter
+    def __get__(self, instance, cls):
+        return self.getter(cls)
+
+
 class BoardConfig(object):
     """The configuration used when building an image for a board."""
     # These attributes may not need to be redefined on some subclasses.
@@ -139,9 +191,24 @@
         return '%s,%s,%s,*\n%s,,,-' % (
             boot_start, boot_len, partition_type, root_start)
 
+    @classproperty
+    def bootcmd(cls):
+        """Get the bootcmd for this board.
+
+        In general subclasses should not have to override this.
+        """
+        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)
+
     @classmethod
-    def _get_boot_cmd(cls, is_live, is_lowmem, consoles, rootfs_uuid):
-        """Get the boot command for this board.
+    def _get_bootargs(cls, is_live, is_lowmem, consoles, rootfs_uuid):
+        """Get the bootargs for this board.
 
         In general subclasses should not have to override this.
         """
@@ -161,30 +228,37 @@
                 lowmem_opt = 'only-ubiquity'
 
         replacements = dict(
-            mmc_option=cls.mmc_option, kernel_addr=cls.kernel_addr,
-            initrd_addr=cls.initrd_addr, serial_opts=serial_opts,
+            serial_opts=serial_opts,
             lowmem_opt=lowmem_opt, boot_snippet=boot_snippet,
             boot_args_options=boot_args_options)
         return (
-            "setenv bootcmd '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'\n"
-            "setenv bootargs '%(serial_opts)s %(lowmem_opt)s "
-                "%(boot_snippet)s %(boot_args_options)s'\n"
-            "boot" % replacements)
+            "%(serial_opts)s %(lowmem_opt)s "
+                "%(boot_snippet)s %(boot_args_options)s"
+             % replacements)
+
+    @classmethod
+    def _get_boot_env(cls, is_live, is_lowmem, consoles, rootfs_uuid):
+        """Get the boot environment for this board.
+
+        In general subclasses should not have to override this.
+        """
+        boot_env = {}
+        boot_env["bootargs"] = cls._get_bootargs(
+            is_live, is_lowmem, consoles, rootfs_uuid)
+        boot_env["bootcmd"] = cls.bootcmd
+        return boot_env
 
     @classmethod
     def make_boot_files(cls, uboot_parts_dir, is_live, is_lowmem, consoles,
-                        root_dir, rootfs_uuid, boot_dir, boot_script,
+                        chroot_dir, rootfs_uuid, boot_dir, boot_script,
                         boot_device_or_file):
-        boot_cmd = cls._get_boot_cmd(
-            is_live, is_lowmem, consoles, rootfs_uuid)
+        boot_env = cls._get_boot_env(is_live, is_lowmem, consoles, rootfs_uuid)
         cls._make_boot_files(
-            uboot_parts_dir, boot_cmd, root_dir, boot_dir, boot_script,
+            uboot_parts_dir, boot_env, chroot_dir, boot_dir, boot_script,
             boot_device_or_file)
 
     @classmethod
-    def _make_boot_files(cls, uboot_parts_dir, boot_cmd, root_dir, boot_dir,
+    def _make_boot_files(cls, uboot_parts_dir, boot_env, chroot_dir, boot_dir,
                          boot_script, boot_device_or_file):
         """Make the necessary boot files for this board.
 
@@ -194,14 +268,6 @@
         raise NotImplementedError()
 
 
-class classproperty(object):
-    """A descriptor that provides @property behavior on class methods."""
-    def __init__(self, getter):
-        self.getter = getter
-    def __get__(self, instance, cls):
-        return self.getter(cls)
-
-
 class OmapConfig(BoardConfig):
     uboot_in_boot_part = True
 
@@ -247,24 +313,24 @@
 
     @classmethod
     def make_boot_files(cls, uboot_parts_dir, is_live, is_lowmem, consoles,
-                        root_dir, rootfs_uuid, boot_dir, boot_script,
+                        chroot_dir, rootfs_uuid, boot_dir, boot_script,
                         boot_device_or_file):
         # XXX: This is also part of our temporary hack to fix bug 697824; we
         # need to call set_appropriate_serial_tty() before doing anything that
         # may use cls.serial_tty.
-        cls.set_appropriate_serial_tty(root_dir)
+        cls.set_appropriate_serial_tty(chroot_dir)
         super(OmapConfig, cls).make_boot_files(
-            uboot_parts_dir, is_live, is_lowmem, consoles, root_dir,
+            uboot_parts_dir, is_live, is_lowmem, consoles, chroot_dir,
             rootfs_uuid, boot_dir, boot_script, boot_device_or_file)
 
     @classmethod
-    def _make_boot_files(cls, uboot_parts_dir, boot_cmd, chroot_dir,
-                         boot_dir, boot_script, boot_device_or_file):
+    def _make_boot_files(cls, uboot_parts_dir, boot_env, chroot_dir, boot_dir,
+                         boot_script, boot_device_or_file):
         install_omap_boot_loader(chroot_dir, boot_dir)
         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)
+        make_boot_script(boot_env, boot_script)
         make_boot_ini(boot_script, boot_dir)
 
 
@@ -317,12 +383,12 @@
     uboot_flavor = None
 
     @classmethod
-    def _make_boot_files(cls, uboot_parts_dir, boot_cmd, chroot_dir,
-                         boot_dir, boot_script, boot_device_or_file):
+    def _make_boot_files(cls, uboot_parts_dir, boot_env, chroot_dir, boot_dir,
+                         boot_script, boot_device_or_file):
         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)
+        make_boot_script(boot_env, boot_script)
         make_boot_ini(boot_script, boot_dir)
 
 
@@ -343,12 +409,12 @@
     mmc_option = '1:1'
 
     @classmethod
-    def _make_boot_files(cls, uboot_parts_dir, boot_cmd, chroot_dir,
-                         boot_dir, boot_script, boot_device_or_file):
+    def _make_boot_files(cls, uboot_parts_dir, boot_env, chroot_dir, boot_dir,
+                         boot_script, boot_device_or_file):
         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)
+        make_boot_script(boot_env, boot_script)
 
 
 class Mx5Config(BoardConfig):
@@ -392,15 +458,15 @@
             loader_start, loader_len, boot_start, boot_len, root_start)
 
     @classmethod
-    def _make_boot_files(cls, uboot_parts_dir, boot_cmd, chroot_dir,
-                         boot_dir, boot_script, boot_device_or_file):
+    def _make_boot_files(cls, uboot_parts_dir, boot_env, chroot_dir, boot_dir,
+                         boot_script, boot_device_or_file):
         uboot_file = os.path.join(
             chroot_dir, 'usr', 'lib', 'u-boot', cls.uboot_flavor, 'u-boot.imx')
         install_mx5_boot_loader(uboot_file, boot_device_or_file)
         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)
+        make_boot_script(boot_env, boot_script)
 
 
 class EfikamxConfig(Mx5Config):
@@ -427,12 +493,90 @@
     fat_size = 16
 
     @classmethod
-    def _make_boot_files(cls, uboot_parts_dir, boot_cmd, chroot_dir,
-                         boot_dir, boot_script, boot_device_or_file):
+    def _make_boot_files(cls, uboot_parts_dir, boot_env, chroot_dir, boot_dir,
+                         boot_script, boot_device_or_file):
         make_uImage(
             cls.load_addr, uboot_parts_dir, cls.kernel_suffix, boot_dir)
         make_uInitrd(uboot_parts_dir, cls.kernel_suffix, boot_dir)
 
+class SMDKV310Config(BoardConfig):
+    serial_tty = 'ttySAC1'
+    extra_serial_opts = 'console=%s,115200n8' % serial_tty
+    kernel_addr = '0x40007000'
+    initrd_addr = '0x41000000'
+    load_addr = '0x40008000'
+    kernel_suffix = 's5pv310'
+    boot_script = 'boot.scr'
+    mmc_part_offset = 1
+    mmc_option = '0:2'
+
+    @classmethod
+    def get_sfdisk_cmd(cls, should_align_boot_part=False):
+        # bootloader partition needs to hold everything from BL1 to uInitrd
+        # inclusive
+        min_len = (
+            SAMSUNG_V310_UINITRD_START + SAMSUNG_V310_UINITRD_RESERVED_LEN -
+            SAMSUNG_V310_BL1_START)
+
+        # bootloader partition
+        loader_start, loader_end, loader_len = align_partition(
+            1, min_len, 1, PART_ALIGN_S)
+
+        # FAT boot partition
+        boot_start, boot_end, boot_len = align_partition(
+            loader_end + 1, BOOT_MIN_SIZE_S, PART_ALIGN_S, PART_ALIGN_S)
+
+        # root partition
+        # we ignore _root_end / _root_len and return a sfdisk command to
+        # instruct the use of all remaining space; XXX if we had some root size
+        # config, we could do something more sensible
+        root_start, _root_end, _root_len = align_partition(
+            boot_end + 1, ROOT_MIN_SIZE_S, PART_ALIGN_S, PART_ALIGN_S)
+
+        return '%s,%s,0xDA\n%s,%s,0x0C,*\n%s,,,-' % (
+            loader_start, loader_len, boot_start, boot_len, root_start)
+
+    @classmethod
+    def _get_boot_env(cls, is_live, is_lowmem, consoles, rootfs_uuid):
+        boot_env = super(SMDKV310Config, cls)._get_boot_env(
+            is_live, is_lowmem, consoles, rootfs_uuid)
+
+        boot_env["ethact"] = "smc911x-0"
+        boot_env["ethaddr"] = "00:40:5c:26:0a:5b"
+        # XXX fixme once FAT support is fixed in u-boot bug 727978
+        boot_env["bootcmd"] = (
+            "movi read kernel %(kernel_addr)s; "
+            "movi read rootfs %(initrd_addr)s %(rootfs_size)s; "
+            "bootm %(kernel_addr)s %(initrd_addr)s" % {
+                'kernel_addr': cls.kernel_addr,
+                'initrd_addr': cls.initrd_addr,
+                'rootfs_size': hex(SAMSUNG_V310_UINITRD_COPY_LEN * SECTOR_SIZE)})
+
+        return boot_env
+
+    @classmethod
+    def _make_boot_files(cls, uboot_parts_dir, boot_env, chroot_dir, boot_dir,
+                         boot_script, boot_device_or_file):
+        uboot_file = os.path.join(
+            chroot_dir, 'usr', 'lib', 'u-boot', 'smdkv310', 'u-boot.v310')
+        install_smdkv310_boot_loader(uboot_file, boot_device_or_file)
+
+        env_size = SAMSUNG_V310_ENV_LEN * SECTOR_SIZE
+        env_file = make_flashable_env(boot_env, env_size)
+        install_smdkv310_boot_env(env_file, boot_device_or_file)
+
+        uImage_file = make_uImage(
+            cls.load_addr, uboot_parts_dir, cls.kernel_suffix, boot_dir)
+        install_smdkv310_uImage(uImage_file, boot_device_or_file)
+
+        uInitrd_file = make_uInitrd(
+            uboot_parts_dir, cls.kernel_suffix, boot_dir)
+        install_smdkv310_initrd(uInitrd_file, boot_device_or_file)
+
+        # unused at the moment once FAT support enabled for the 
+        # Samsung u-boot this can be used bug 727978
+        #make_boot_script(boot_env, boot_script)
+
 
 board_configs = {
     'beagle': BeagleConfig,
@@ -443,9 +587,26 @@
     'efikamx': EfikamxConfig,
     'mx51evk': Mx51evkConfig,
     'overo': OveroConfig,
+    'smdkv310': SMDKV310Config,
     }
 
 
+def _dd(input_file, output_file, block_size=SECTOR_SIZE, count=None, seek=None,
+        skip=None):
+    """Wrapper around the dd command"""
+    cmd = [
+        "dd", "if=%s" % input_file, "of=%s" % output_file,
+        "bs=%s" % block_size, "conv=notrunc"]
+    if count is not None:
+        cmd.append("count=%s" % count)
+    if seek is not None:
+        cmd.append("seek=%s" % seek)
+    if skip is not None:
+        cmd.append("skip=%s" % skip)
+    proc = cmd_runner.run(cmd, as_root=True)
+    proc.wait()
+
+
 def _run_mkimage(img_type, load_addr, entry_point, name, img_data, img,
                  stdout=None, as_root=True):
     cmd = ['mkimage',
@@ -484,18 +645,26 @@
     img_data = _get_file_matching(
         '%s/vmlinuz-*-%s' % (uboot_parts_dir, suffix))
     img = '%s/uImage' % boot_disk
-    return _run_mkimage(
+    _run_mkimage(
         'kernel', load_addr, load_addr, 'Linux', img_data, img)
+    return img
 
 
 def make_uInitrd(uboot_parts_dir, suffix, boot_disk):
     img_data = _get_file_matching(
         '%s/initrd.img-*-%s' % (uboot_parts_dir, suffix))
     img = '%s/uInitrd' % boot_disk
-    return _run_mkimage('ramdisk', '0', '0', 'initramfs', img_data, img)
-
-
-def make_boot_script(boot_script_data, boot_script):
+    _run_mkimage('ramdisk', '0', '0', 'initramfs', img_data, img)
+    return img
+
+
+def make_boot_script(boot_env, boot_script):
+    boot_script_data = (
+        "setenv bootcmd '%(bootcmd)s'\n"
+        "setenv bootargs '%(bootargs)s'\n"
+        "boot"
+        % boot_env)
+
     # Need to save the boot script data into a file that will be passed to
     # mkimage.
     _, tmpfile = tempfile.mkstemp()
@@ -509,15 +678,40 @@
         'script', '0', '0', 'boot script', plain_boot_script, boot_script)
 
 
+def make_flashable_env(boot_env, env_size):
+    env_strings = ["%s=%s" % (k, v) for k, v in boot_env.items()]
+    env_strings.sort()
+    env = struct.pack('B', 0).join(env_strings)
+
+    # we still need to zero-terminate the last string, and 4 bytes for crc
+    assert len(env) + 1 + 4 <= env_size, (
+        "environment doesn't fit in %s bytes" % env_size)
+
+    # pad the rest of the env for the CRC calc; the "s" format will write zero
+    # bytes to pad the (empty) string to repeat count
+    env += struct.pack('%ss' % (env_size - len(env) - 4), '')
+
+    crc = crc32(env)
+    env = struct.pack('<i', crc) + env
+
+    _, tmpfile = tempfile.mkstemp()
+
+    with open(tmpfile, 'w') as fd:
+        fd.write(env)
+
+    return tmpfile
+
+
 def install_mx5_boot_loader(imx_file, boot_device_or_file):
-    proc = cmd_runner.run([
-        "dd",
-        "if=%s" % imx_file,
-        "of=%s" % boot_device_or_file,
-        "bs=1024",
-        "seek=1",
-        "conv=notrunc"], as_root=True)
-    proc.wait()
+    # bootloader partition starts at +1s but we write the file at +2s, so we
+    # need to check that the bootloader partition minus 1s is at least as large
+    # as the u-boot binary; note that the real bootloader partition might be
+    # larger than LOADER_MIN_SIZE_S, but if u-boot is larger it's a sign we
+    # need to bump LOADER_MIN_SIZE_S
+    max_size = (LOADER_MIN_SIZE_S - 1) * SECTOR_SIZE
+    assert os.path.getsize(imx_file) <= max_size, (
+        "%s is larger than guaranteed bootloader partition size" % imx_file)
+    _dd(imx_file, boot_device_or_file, seek=2)
 
 
 def _get_mlo_file(chroot_dir):
@@ -554,3 +748,54 @@
     proc = cmd_runner.run(
         ["cp", "-v", boot_script, "%s/boot.ini" % boot_disk], as_root=True)
     proc.wait()
+
+
+def install_smdkv310_uImage(uImage_file, boot_device_or_file):
+    # the layout keeps SAMSUNG_V310_UIMAGE_LEN sectors for uImage; make sure
+    # uImage isn't actually larger or it would be truncated
+    max_size = SAMSUNG_V310_UIMAGE_LEN * SECTOR_SIZE
+    assert os.path.getsize(uImage_file) <= max_size, (
+        "%s is larger than the allocated v310 uImage length" % uImage_file)
+    _dd(uImage_file, boot_device_or_file, count=SAMSUNG_V310_UIMAGE_LEN,
+        seek=SAMSUNG_V310_UIMAGE_START)
+
+
+def install_smdkv310_initrd(initrd_file, boot_device_or_file):
+    # the layout keeps SAMSUNG_V310_UINITRD_RESERVED_LEN sectors for uInitrd
+    # but only SAMSUNG_V310_UINITRD_COPY_LEN sectors are loaded into memory;
+    # make sure uInitrd isn't actually larger or it would be truncated in
+    # memory
+    max_size = SAMSUNG_V310_UINITRD_COPY_LEN * SECTOR_SIZE
+    assert os.path.getsize(initrd_file) <= max_size, (
+        "%s is larger than the v310 uInitrd length as used by u-boot"
+            % initrd_file)
+    _dd(initrd_file, boot_device_or_file,
+        count=SAMSUNG_V310_UINITRD_COPY_LEN,
+        seek=SAMSUNG_V310_UINITRD_START)
+
+
+def install_smdkv310_boot_env(env_file, boot_device_or_file):
+    # the environment file is exactly SAMSUNG_V310_ENV_LEN as created by
+    # make_flashable_env(), so we don't need to check the size of env_file
+    _dd(env_file, boot_device_or_file, count=SAMSUNG_V310_ENV_LEN,
+        seek=SAMSUNG_V310_ENV_START)
+
+
+def install_smdkv310_boot_loader(v310_file, boot_device_or_file):
+    """Samsung specific terminology
+    BL0 is the first stage bootloader,
+    BL1 is the SPL (secondary program loader)
+    v310_file is a binary with the same layout as BL1 + u-boot environment +
+    BL2; write BL1 (SPL) piece first (SAMSUNG_V310_BL1_LEN sectors at +0s in
+    the file and +SAMSUNG_V310_BL1_START on disk), then write BL2 (u-boot)
+    piece (rest of the file starting at +(SAMSUNG_V310_BL1_LEN +
+    SAMSUNG_V310_ENV_LEN)s in the file which is the same as
+    +(SAMSUNG_V310_BL2_START - SAMSUNG_V310_BL1_START)s)
+    """
+    _dd(v310_file, boot_device_or_file, count=SAMSUNG_V310_BL1_LEN,
+        seek=SAMSUNG_V310_BL1_START)
+    # XXX need to check that the length of v310_file - 64s is smaller than
+    # SAMSUNG_V310_BL2_LEN
+    _dd(v310_file, boot_device_or_file, seek=SAMSUNG_V310_BL2_START,
+        skip=(SAMSUNG_V310_BL2_START - SAMSUNG_V310_BL1_START))
+

=== modified file 'linaro_media_create/tests/test_media_create.py'
--- linaro_media_create/tests/test_media_create.py	2011-03-03 13:44:11 +0000
+++ linaro_media_create/tests/test_media_create.py	2011-03-05 19:28:12 +0000
@@ -43,9 +43,12 @@
     )
 import linaro_media_create
 from linaro_media_create.boards import (
+    LOADER_MIN_SIZE_S,
+    SECTOR_SIZE,
     align_up,
     align_partition,
     board_configs,
+    make_flashable_env,
     install_mx5_boot_loader,
     install_omap_boot_loader,
     make_boot_script,
@@ -258,6 +261,15 @@
             'make_boot_script']
         self.assertEqual(expected, self.funcs_calls)
 
+    def test_smdkv310_steps(self):
+        self.make_boot_files(boards.SMDKV310Config)
+        expected = [
+            'install_smdkv310_boot_loader', 'make_flashable_env',
+            'install_smdkv310_boot_env', 'make_uImage',
+            'install_smdkv310_uImage', 'make_uInitrd',
+            'install_smdkv310_initrd',]
+        self.assertEqual(expected, self.funcs_calls)
+
     def test_ux500_steps(self):
         self.make_boot_files(boards.Ux500Config)
         expected = ['make_uImage', 'make_uInitrd', 'make_boot_script']
@@ -372,44 +384,67 @@
             '1,8191,0xDA\n8192,106496,0x0C,*\n114688,,,-',
             boards.Mx5Config.get_sfdisk_cmd())
 
+    def test_smdkv310(self):
+        self.assertEquals(
+            '1,221183,0xDA\n221184,106496,0x0C,*\n327680,,,-',
+            board_configs['smdkv310'].get_sfdisk_cmd())
+
 
 class TestGetBootCmd(TestCase):
 
     def test_vexpress(self):
-        boot_cmd = board_configs['vexpress']._get_boot_cmd(
+        boot_commands = board_configs['vexpress']._get_boot_env(
             is_live=False, is_lowmem=False, consoles=['ttyXXX'],
             rootfs_uuid="deadbeef")
-        expected = (
-            "setenv bootcmd 'fatload mmc 0:1 0x60008000 uImage; fatload mmc "
-            "0:1 0x81000000 uInitrd; bootm 0x60008000 0x81000000'\nsetenv "
-            "bootargs 'console=tty0 console=ttyAMA0,38400n8 console=ttyXXX  "
-            "root=UUID=deadbeef rootwait ro'\nboot")
-        self.assertEqual(expected, boot_cmd)
+        expected = {
+            'bootargs': 'console=tty0 console=ttyAMA0,38400n8 console=ttyXXX  '
+                        'root=UUID=deadbeef rootwait ro',
+            'bootcmd': 'fatload mmc 0:1 0x60008000 uImage; '
+                       'fatload mmc 0:1 0x81000000 uInitrd; '
+                       'bootm 0x60008000 0x81000000'}
+        self.assertEqual(expected, boot_commands)
 
     def test_mx5(self):
-        boot_cmd = boards.Mx5Config._get_boot_cmd(
-            is_live=False, is_lowmem=False, consoles=[],
-            rootfs_uuid="deadbeef")
-        expected = (
-            "setenv bootcmd 'fatload mmc 0:2 0x90000000 uImage; fatload mmc "
-            "0:2 0x90800000 uInitrd; bootm 0x90000000 0x90800000'\nsetenv "
-            "bootargs 'console=tty0 console=ttymxc0,115200n8  "
-            "root=UUID=deadbeef rootwait ro'\nboot")
-        self.assertEqual(expected, boot_cmd)
+        boot_commands = boards.Mx5Config._get_boot_env(
+            is_live=False, is_lowmem=False, consoles=[],
+            rootfs_uuid="deadbeef")
+        expected = {
+            'bootargs': 'console=tty0 console=ttymxc0,115200n8  '
+                        'root=UUID=deadbeef rootwait ro',
+            'bootcmd': 'fatload mmc 0:2 0x90000000 uImage; '
+                       'fatload mmc 0:2 0x90800000 uInitrd; '
+                       'bootm 0x90000000 0x90800000'}
+        self.assertEqual(expected, boot_commands)
+
+    def test_smdkv310(self):
+        boot_commands = board_configs['smdkv310']._get_boot_env(
+            is_live=False, is_lowmem=False, consoles=[],
+            rootfs_uuid="deadbeef")
+        expected = {
+            'bootargs': 'console=ttySAC1,115200n8  root=UUID=deadbeef '
+                        'rootwait ro',
+             'bootcmd': 'movi read kernel 0x40007000; '
+                        'movi read rootfs 0x41000000 0x1000000; '
+                        'bootm 0x40007000 0x41000000',
+             'ethact': 'smc911x-0',
+             'ethaddr': '00:40:5c:26:0a:5b'}
+        self.assertEqual(expected, boot_commands)
 
     def test_ux500(self):
-        boot_cmd = board_configs['ux500']._get_boot_cmd(
+        boot_commands = board_configs['ux500']._get_boot_env(
             is_live=False, is_lowmem=False, consoles=[],
             rootfs_uuid="deadbeef")
-        expected = (
-            "setenv bootcmd 'fatload mmc 1:1 0x00100000 uImage; fatload mmc "
-            "1:1 0x08000000 uInitrd; bootm 0x00100000 0x08000000'\nsetenv "
-            "bootargs 'console=tty0 console=ttyAMA2,115200n8  "
-            "root=UUID=deadbeef rootwait ro earlyprintk rootdelay=1 fixrtc "
-            "nocompcache mem=96M@0 mem_modem=32M@96M mem=44M@128M "
-            "pmem=22M@172M mem=30M@194M mem_mali=32M@224M "
-            "pmem_hwb=54M@256M hwmem=48M@302M mem=152M@360M'\nboot")
-        self.assertEqual(expected, boot_cmd)
+        expected = {
+            'bootargs': 'console=tty0 console=ttyAMA2,115200n8  '
+                        'root=UUID=deadbeef rootwait ro earlyprintk '
+                        'rootdelay=1 fixrtc nocompcache mem=96M@0 '
+                        'mem_modem=32M@96M mem=44M@128M pmem=22M@172M '
+                        'mem=30M@194M mem_mali=32M@224M pmem_hwb=54M@256M '
+                        'hwmem=48M@302M mem=152M@360M',
+            'bootcmd': 'fatload mmc 1:1 0x00100000 uImage; '
+                       'fatload mmc 1:1 0x08000000 uInitrd; '
+                       'bootm 0x00100000 0x08000000'}
+        self.assertEqual(expected, boot_commands)
 
     def test_panda(self):
         # XXX: To fix bug 697824 we have to change class attributes of our
@@ -417,17 +452,18 @@
         # don't interfere with us we'll reset that before doing anything.
         config = board_configs['panda']
         config.serial_tty = config._serial_tty
-        boot_cmd = config._get_boot_cmd(
+        boot_commands = config._get_boot_env(
             is_live=False, is_lowmem=False, consoles=[],
             rootfs_uuid="deadbeef")
-        expected = (
-            "setenv bootcmd 'fatload mmc 0:1 0x80200000 uImage; fatload mmc "
-            "0:1 0x81600000 uInitrd; bootm 0x80200000 0x81600000'\nsetenv "
-            "bootargs 'console=tty0 console=ttyO2,115200n8  "
-            "root=UUID=deadbeef rootwait ro earlyprintk fixrtc nocompcache "
-            "vram=32M omapfb.vram=0:8M mem=463M "
-            "ip=none'\nboot")
-        self.assertEqual(expected, boot_cmd)
+        expected = {
+            'bootargs': 'console=tty0 console=ttyO2,115200n8  '
+                        'root=UUID=deadbeef rootwait ro earlyprintk fixrtc '
+                        'nocompcache vram=32M omapfb.vram=0:8M mem=463M '
+                        'ip=none',
+            'bootcmd': 'fatload mmc 0:1 0x80200000 uImage; '
+                       'fatload mmc 0:1 0x81600000 uInitrd; '
+                       'bootm 0x80200000 0x81600000'}
+        self.assertEqual(expected, boot_commands)
 
     def test_beagle(self):
         # XXX: To fix bug 697824 we have to change class attributes of our
@@ -435,17 +471,17 @@
         # don't interfere with us we'll reset that before doing anything.
         config = board_configs['beagle']
         config.serial_tty = config._serial_tty
-        boot_cmd = config._get_boot_cmd(
+        boot_commands = config._get_boot_env(
             is_live=False, is_lowmem=False, consoles=[],
             rootfs_uuid="deadbeef")
-        expected = (
-            "setenv bootcmd 'fatload mmc 0:1 0x80000000 uImage; "
-            "fatload mmc 0:1 0x81600000 uInitrd; bootm 0x80000000 "
-            "0x81600000'\nsetenv bootargs 'console=tty0 "
-            "console=ttyO2,115200n8  root=UUID=deadbeef rootwait ro "
-            "earlyprintk fixrtc nocompcache vram=12M "
-            "omapfb.mode=dvi:1280x720MR-16@60'\nboot")
-        self.assertEqual(expected, boot_cmd)
+        expected = {
+            'bootargs': 'console=tty0 console=ttyO2,115200n8  '
+                        'root=UUID=deadbeef rootwait ro earlyprintk fixrtc '
+                        'nocompcache vram=12M omapfb.mode=dvi:1280x720MR-16@60',
+            'bootcmd': 'fatload mmc 0:1 0x80000000 uImage; '
+                       'fatload mmc 0:1 0x81600000 uInitrd; '
+                       'bootm 0x80000000 0x81600000'}
+        self.assertEqual(expected, boot_commands)
 
     def test_overo(self):
         # XXX: To fix bug 697824 we have to change class attributes of our
@@ -453,17 +489,19 @@
         # don't interfere with us we'll reset that before doing anything.
         config = board_configs['overo']
         config.serial_tty = config._serial_tty
-        boot_cmd = config._get_boot_cmd(
+        boot_commands = config._get_boot_env(
             is_live=False, is_lowmem=False, consoles=[],
             rootfs_uuid="deadbeef")
-        expected = (
-            "setenv bootcmd 'fatload mmc 0:1 0x80000000 uImage; "
-            "fatload mmc 0:1 0x81600000 uInitrd; bootm 0x80000000 "
-            "0x81600000'\nsetenv bootargs 'console=tty0 "
-            "console=ttyO2,115200n8  root=UUID=deadbeef rootwait ro "
-            "earlyprintk mpurate=500 vram=12M "
-            "omapfb.mode=dvi:1024x768MR-16@60 omapdss.def_disp=dvi'\nboot")
-        self.assertEqual(expected, boot_cmd)
+        expected = {
+            'bootargs': 'console=tty0 console=ttyO2,115200n8  '
+                        'root=UUID=deadbeef rootwait ro earlyprintk '
+                        'mpurate=500 vram=12M '
+                        'omapfb.mode=dvi:1024x768MR-16@60 '
+                        'omapdss.def_disp=dvi',
+            'bootcmd': 'fatload mmc 0:1 0x80000000 uImage; '
+                       'fatload mmc 0:1 0x81600000 uInitrd; '
+                       'bootm 0x80000000 0x81600000'}
+        self.assertEqual(expected, boot_commands)
 
 
 class TestUnpackBinaryTarball(TestCaseWithFixtures):
@@ -586,14 +624,35 @@
             '-d', 'parts_dir/initrd.img-*-sub_arch', 'boot_disk/uInitrd']
         self.assertEqual([expected], fixture.mock.calls)
 
+    def test_make_flashable_env_too_small_env(self):
+        env = {'verylong': 'evenlonger'}
+        self.assertRaises(AssertionError, make_flashable_env, env, 8)
+
+    def test_make_flashable_env(self):
+        env_file = self.createTempFileAsFixture()
+        self.useFixture(MockSomethingFixture(
+            tempfile, "mkstemp", lambda: (None, env_file)))
+        env = {'a': 'b', 'x': 'y'}
+        make_flashable_env(env, 12)
+        with open(env_file, "r") as fd:
+            self.assertEqual("\x80\x29\x2E\x89a=b\x00x=y\x00", fd.read())
+
     def test_install_mx5_boot_loader(self):
         fixture = self._mock_Popen()
-        install_mx5_boot_loader("imx_file", "boot_device_or_file")
+        imx_file = self.createTempFileAsFixture()
+        install_mx5_boot_loader(imx_file, "boot_device_or_file")
         expected = [
-            'sudo', 'dd', 'if=imx_file', 'of=boot_device_or_file', 'bs=1024',
-            'seek=1', 'conv=notrunc']
+            'sudo', 'dd', 'if=%s' % imx_file, 'of=boot_device_or_file',
+            'bs=512', 'conv=notrunc', 'seek=2']
         self.assertEqual([expected], fixture.mock.calls)
 
+    def test_install_mx5_boot_loader_too_large(self):
+        self.useFixture(MockSomethingFixture(
+            os.path, "getsize",
+            lambda s: (LOADER_MIN_SIZE_S - 1) * SECTOR_SIZE + 1))
+        self.assertRaises(AssertionError,
+            install_mx5_boot_loader, "imx_file", "boot_device_or_file")
+
     def test_install_omap_boot_loader(self):
         fixture = self._mock_Popen()
         self.useFixture(MockSomethingFixture(
@@ -613,7 +672,8 @@
         fixture = self._mock_Popen()
         boot_script_path = os.path.join(tempdir, 'boot.scr')
         plain_boot_script_path = os.path.join(tempdir, 'boot.txt')
-        make_boot_script('boot script data', boot_script_path)
+        boot_env = {'bootargs': 'mybootargs', 'bootcmd': 'mybootcmd'}
+        make_boot_script(boot_env, boot_script_path)
         expected = [
             ['sudo', 'cp', '/tmp/random-abxzr', plain_boot_script_path],
             ['sudo', 'mkimage', '-A', 'arm', '-O', 'linux', '-T', 'script',
@@ -692,6 +752,25 @@
             [('1,8191,0xDA\n8192,106496,0x0C,*\n114688,,,-', 255, 63, '', self.media.path)],
             sfdisk_fixture.mock.calls)
 
+    def test_create_partitions_for_smdkv310(self):
+        # For this board we create a one cylinder partition at the beginning.
+        popen_fixture = self.useFixture(MockCmdRunnerPopenFixture())
+        sfdisk_fixture = self.useFixture(MockRunSfdiskCommandsFixture())
+
+        create_partitions(
+            board_configs['smdkv310'], self.media, 255, 63, '')
+
+        self.assertEqual(
+            [['sudo', 'parted', '-s', self.media.path, 'mklabel', 'msdos'],
+             ['sync']],
+            popen_fixture.mock.calls)
+        # Notice that we create all partitions in a single sfdisk run because
+        # every time we run sfdisk it actually repartitions the device,
+        # erasing any partitions created previously.
+        self.assertEqual(
+            [('1,221183,0xDA\n221184,106496,0x0C,*\n327680,,,-', 255, 63, '',
+              self.media.path)], sfdisk_fixture.mock.calls)
+
     def test_create_partitions_for_beagle(self):
         popen_fixture = self.useFixture(MockCmdRunnerPopenFixture())
         sfdisk_fixture = self.useFixture(MockRunSfdiskCommandsFixture())
@@ -711,9 +790,9 @@
         popen_fixture = self.useFixture(MockCmdRunnerPopenFixture())
         sfdisk_fixture = self.useFixture(MockRunSfdiskCommandsFixture())
 
-        tempfile = self.createTempFileAsFixture()
+        tmpfile = self.createTempFileAsFixture()
         create_partitions(
-            board_configs['beagle'], Media(tempfile), 255, 63, '')
+            board_configs['beagle'], Media(tmpfile), 255, 63, '')
 
         # Unlike the test for partitioning of a regular block device, in this
         # case parted was not called as there's no existing partition table
@@ -721,26 +800,26 @@
         self.assertEqual([['sync']], popen_fixture.mock.calls)
 
         self.assertEqual(
-            [('63,106432,0x0C,*\n106496,,,-', 255, 63, '', tempfile)],
+            [('63,106432,0x0C,*\n106496,,,-', 255, 63, '', tmpfile)],
             sfdisk_fixture.mock.calls)
 
     def test_run_sfdisk_commands(self):
-        tempfile = self.createTempFileAsFixture()
+        tmpfile = self.createTempFileAsFixture()
         proc = cmd_runner.run(
-            ['qemu-img', 'create', '-f', 'raw', tempfile, '10M'],
+            ['qemu-img', 'create', '-f', 'raw', tmpfile, '10M'],
             stdout=subprocess.PIPE)
         proc.communicate()
         stdout, stderr = run_sfdisk_commands(
-            '2,16063,0xDA', 255, 63, '', tempfile, as_root=False,
+            '2,16063,0xDA', 255, 63, '', tmpfile, as_root=False,
             stderr=subprocess.PIPE)
         self.assertIn('Successfully wrote the new partition table', stdout)
 
     def test_run_sfdisk_commands_raises_on_non_zero_returncode(self):
-        tempfile = self.createTempFileAsFixture()
+        tmpfile = self.createTempFileAsFixture()
         self.assertRaises(
             cmd_runner.SubcommandNonZeroReturnValue,
             run_sfdisk_commands,
-            ',1,0xDA', 255, 63, '', tempfile, as_root=False,
+            ',1,0xDA', 255, 63, '', tmpfile, as_root=False,
             stderr=subprocess.PIPE)
 
 
@@ -756,7 +835,7 @@
         super(TestPartitionSetup, self).tearDown()
         time.sleep = self.orig_sleep
 
-    def _create_tempfile(self):
+    def _create_tmpfile(self):
         # boot part at +8 MiB, root part at +16 MiB
         return self._create_qemu_img_with_partitions(
             '16384,15746,0x0C,*\n32768,,,-')
@@ -771,19 +850,19 @@
         self.assertEqual(12 * 1024**3, convert_size_to_bytes('12G'))
 
     def test_calculate_partition_size_and_offset(self):
-        tempfile = self._create_tempfile()
+        tmpfile = self._create_tmpfile()
         vfat_size, vfat_offset, linux_size, linux_offset = (
-            calculate_partition_size_and_offset(tempfile))
+            calculate_partition_size_and_offset(tmpfile))
         self.assertEqual(
             [8061952L, 8388608L, 14680064L, 16777216L],
             [vfat_size, vfat_offset, linux_size, linux_offset])
 
     def test_partition_numbering(self):
         # another Linux partition at +24 MiB after the boot/root parts
-        tempfile = self._create_qemu_img_with_partitions(
+        tmpfile = self._create_qemu_img_with_partitions(
             '16384,15746,0x0C,*\n32768,15427,,-\n49152,,,-')
         vfat_size, vfat_offset, linux_size, linux_offset = (
-            calculate_partition_size_and_offset(tempfile))
+            calculate_partition_size_and_offset(tmpfile))
         # check that the linux partition offset starts at +16 MiB so that it's
         # the partition immediately following the vfat one
         self.assertEqual(linux_offset, 32768 * 512)
@@ -791,39 +870,39 @@
     def test_get_boot_and_root_partitions_for_media_beagle(self):
         self.useFixture(MockSomethingFixture(
             partitions, '_get_device_file_for_partition_number',
-            lambda dev, partition: '%s%d' % (tempfile, partition)))
-        tempfile = self.createTempFileAsFixture()
-        media = Media(tempfile)
+            lambda dev, partition: '%s%d' % (tmpfile, partition)))
+        tmpfile = self.createTempFileAsFixture()
+        media = Media(tmpfile)
         media.is_block_device = True
         self.assertEqual(
-            ("%s%d" % (tempfile, 1), "%s%d" % (tempfile, 2)),
+            ("%s%d" % (tmpfile, 1), "%s%d" % (tmpfile, 2)),
             get_boot_and_root_partitions_for_media(
                 media, board_configs['beagle']))
 
     def test_get_boot_and_root_partitions_for_media_mx5(self):
         self.useFixture(MockSomethingFixture(
             partitions, '_get_device_file_for_partition_number',
-            lambda dev, partition: '%s%d' % (tempfile, partition)))
-        tempfile = self.createTempFileAsFixture()
-        media = Media(tempfile)
+            lambda dev, partition: '%s%d' % (tmpfile, partition)))
+        tmpfile = self.createTempFileAsFixture()
+        media = Media(tmpfile)
         media.is_block_device = True
         self.assertEqual(
-            ("%s%d" % (tempfile, 2), "%s%d" % (tempfile, 3)),
+            ("%s%d" % (tmpfile, 2), "%s%d" % (tmpfile, 3)),
             get_boot_and_root_partitions_for_media(media, boards.Mx5Config))
 
     def _create_qemu_img_with_partitions(self, sfdisk_commands):
-        tempfile = self.createTempFileAsFixture()
+        tmpfile = self.createTempFileAsFixture()
         proc = cmd_runner.run(
-            ['qemu-img', 'create', '-f', 'raw', tempfile, '30M'],
+            ['qemu-img', 'create', '-f', 'raw', tmpfile, '30M'],
             stdout=subprocess.PIPE)
         proc.communicate()
         stdout, stderr = run_sfdisk_commands(
-            sfdisk_commands, 255, 63, '', tempfile, as_root=False,
+            sfdisk_commands, 255, 63, '', tmpfile, as_root=False,
             # Throw away stderr as sfdisk complains a lot when operating on a
             # qemu image.
             stderr=subprocess.PIPE)
         self.assertIn('Successfully wrote the new partition table', stdout)
-        return tempfile
+        return tmpfile
 
     def test_ensure_partition_is_not_mounted_for_mounted_partition(self):
         self.useFixture(MockSomethingFixture(
@@ -841,18 +920,18 @@
         self.assertEqual(None, popen_fixture.mock.calls)
 
     def test_get_boot_and_root_loopback_devices(self):
-        tempfile = self._create_tempfile()
+        tmpfile = self._create_tmpfile()
         atexit_fixture = self.useFixture(MockSomethingFixture(
             atexit, 'register', AtExitRegister()))
         popen_fixture = self.useFixture(MockCmdRunnerPopenFixture())
         # We can't test the return value of get_boot_and_root_loopback_devices
         # because it'd require running losetup as root, so we just make sure
         # it calls losetup correctly.
-        get_boot_and_root_loopback_devices(tempfile)
+        get_boot_and_root_loopback_devices(tmpfile)
         self.assertEqual(
-            [['sudo', 'losetup', '-f', '--show', tempfile, '--offset',
+            [['sudo', 'losetup', '-f', '--show', tmpfile, '--offset',
               '8388608', '--sizelimit', '8061952'],
-             ['sudo', 'losetup', '-f', '--show', tempfile, '--offset',
+             ['sudo', 'losetup', '-f', '--show', tmpfile, '--offset',
               '16777216', '--sizelimit', '14680064']],
             popen_fixture.mock.calls)
 
@@ -873,7 +952,7 @@
         # but here we mock Popen() and thanks to that the image is not setup
         # (via qemu-img) inside setup_partitions.  That's why we pass an
         # already setup image file.
-        tempfile = self._create_tempfile()
+        tmpfile = self._create_tmpfile()
         popen_fixture = self.useFixture(MockCmdRunnerPopenFixture())
         self.useFixture(MockSomethingFixture(
             sys, 'stdout', open('/dev/null', 'w')))
@@ -890,14 +969,14 @@
             partitions, 'get_boot_and_root_loopback_devices',
             lambda image: ('/dev/loop99', '/dev/loop98')))
         bootfs_dev, rootfs_dev = setup_partitions(
-            board_configs['beagle'], Media(tempfile), '2G', 'boot',
+            board_configs['beagle'], Media(tmpfile), '2G', 'boot',
             'root', 'ext3', True, True, True)
         self.assertEqual(
              # This is the call that would create a 2 GiB image file.
-            [['qemu-img', 'create', '-f', 'raw', tempfile, '2147483648'],
+            [['qemu-img', 'create', '-f', 'raw', tmpfile, '2147483648'],
              # This call would partition the image file.
              ['sudo', 'sfdisk', '--force', '-D', '-uS', '-H', '255', '-S',
-              '63', '-C', '261', tempfile],
+              '63', '-C', '261', tmpfile],
              # Make sure changes are written to disk.
              ['sync'],
              ['sudo', 'mkfs.vfat', '-F', '32', bootfs_dev, '-n', 'boot'],
@@ -910,21 +989,21 @@
         # Pretend the partitions are mounted.
         self.useFixture(MockSomethingFixture(
             partitions, 'is_partition_mounted', lambda part: True))
-        tempfile = self._create_tempfile()
+        tmpfile = self._create_tmpfile()
         self.useFixture(MockSomethingFixture(
             partitions, '_get_device_file_for_partition_number',
-            lambda dev, partition: '%s%d' % (tempfile, partition)))
-        media = Media(tempfile)
-        # Pretend our tempfile is a block device.
+            lambda dev, partition: '%s%d' % (tmpfile, partition)))
+        media = Media(tmpfile)
+        # Pretend our tmpfile is a block device.
         media.is_block_device = True
         popen_fixture = self.useFixture(MockCmdRunnerPopenFixture())
         bootfs_dev, rootfs_dev = setup_partitions(
             board_configs['beagle'], media, '2G', 'boot', 'root', 'ext3',
             True, True, True)
         self.assertEqual(
-            [['sudo', 'parted', '-s', tempfile, 'mklabel', 'msdos'],
+            [['sudo', 'parted', '-s', tmpfile, 'mklabel', 'msdos'],
              ['sudo', 'sfdisk', '--force', '-D', '-uS', '-H', '255', '-S',
-              '63', tempfile],
+              '63', tmpfile],
              ['sync'],
              # Since the partitions are mounted, setup_partitions will umount
              # them before running mkfs.