[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: apex-1.4.5



Martin Michlmayr wrote:
> Below is the patch used to create this image.  Rod, can you please
> review?

Comments inline.

> --- slugimage~	2006-08-04 22:52:24.000000000 +0200
> +++ slugimage	2006-08-18 19:17:37.000000000 +0200
> @@ -39,12 +39,13 @@
>  use strict;
>  use warnings;
>  
> -use Getopt::Long;
> +use Getopt::Long qw(:config no_ignore_case);
>  use POSIX qw(tmpnam);
>  
>  my($debug) = 1;
>  my($quiet) = 0;
>  my($block_size) = 0x00020000;
> +my(@cleanup);
>  
>  # The last 70 bytes of the SercommRedBootTrailer (i.e. excluding MAC
>  # address).  Needed to create an image with an empty RedBoot partition
> @@ -582,6 +583,16 @@
>  	    # Pack and append the binary table entry for this partition.
>  	    $partition_data .= createPartitionEntry($_->{'name'}, $_->{'offset'} + $flash_start, $_->{'size'});

Let's add the data_size entry in here now too.

> +	    # Optionally put a skip header into the padding area.
> +	    if (defined $_->{'skip'}) {
> +		my $i = 1;

Can we initialise $i to the number of skips (use "scalar @...") and then
decrement?  Then the skip entries end up in the same order as they are
defined in slugimage.

> +		foreach my $skip (@{$_->{'skip'}}) {
> +		    substr($partition_data, -8 - 12*$i, 12) = pack("a4N2", "skip",
> +			$skip->{'offset'}, $skip->{'size'});
> +		    $i++;
> +		}
> +	    }
> +
>  	    # If this is the FIS directory, then write the partition table data into it.
>  	    if ($_->{'name'} eq "FIS directory") {
>  		# Explicitly terminate the partition data.
> @@ -725,9 +736,11 @@
>      } @partitions;
>  }
>  
> -sub defaultPartitions {
> +sub defaultPartitions($) {
> +    my($loader) = @_;
>  
> -    return ({'name'=>'RedBoot',           'file'=>'RedBoot',
> +    my @partitions =
> +	    ({'name'=>'RedBoot',          'file'=>'RedBoot',
>  	     'offset'=>0x00000000,        'size'=>0x00040000,
>  	     'variable'=>0, 'header'=>0,  'pseudo'=>0, 'data'=>undef, 'byteswap'=>0},
>  	    {'name'=>'EthAddr',           'file'=>undef,
> @@ -735,13 +748,44 @@
>  	     'variable'=>0, 'header'=>0,  'pseudo'=>1, 'data'=>undef, 'byteswap'=>0},
>  	    {'name'=>'SysConf',           'file'=>'SysConf',
>  	     'offset'=>0x00040000,        'size'=>0x00020000,
> -	     'variable'=>0, 'header'=>0,  'pseudo'=>0, 'data'=>undef, 'byteswap'=>0},
> -	    {'name'=>'Kernel',            'file'=>'vmlinuz',
> -	     'offset'=>0x00060000,        'size'=>0x00100000,
> +	     'variable'=>0, 'header'=>0,  'pseudo'=>0, 'data'=>undef, 'byteswap'=>0});
> +
> +    my $loader_size = 0;
> +    if ($loader) {
> +	$loader_size = 0x00020000;

Can we read the size of the loader file provided, and then round it up
to the next erase block size?  Then we don't need to change slugimage
again if someone uses a different loader from apex which is larger.

> +	push @partitions,
> +	   ({'name'=>'Loader',            'file'=>'loader',
> +	     'offset'=>0x00060000,        'size'=>$loader_size,
> +	     'variable'=>0, 'header'=>16, 'pseudo'=>0, 'data'=>undef, 'byteswap'=>0});
> +    }
> +
> +    push @partitions,
> +	    ({'name'=>'Kernel',           'file'=>'vmlinuz',
> +	     'offset'=>(0x00060000+$loader_size),
> +	     'variable'=>0, 'header'=>16, 'pseudo'=>0, 'data'=>undef, 'byteswap'=>0});
> +
> +    if ($loader) {
> +	# If a second stage boot loader is defined (APEX), we create two
> +	# kernel partitions, one beginning right after APEX and one
> +	# beginning where RedBoot expects the ramdisk.  Additionally, we
> +	# define a pseudo kernel partition (Kernel) which spans both kernel
> +	# partitions (Kernel1 and Kernel2) and which has some information
> +	# in the padding area of the FIS partition informing APEX which part
> +	# to skip in order to get a good kernel image (i.e. the Sercomm
> +	# header that sits between the two parts of the kernel image).
> +	push @partitions,
> +	   ({'name'=>'Kernel1',           'file'=>'vmlinuz1',
> +	     'offset'=>(0x00060000+$loader_size), 'size'=>(0x00160000-0x00060000-$loader_size),
>  	     'variable'=>0, 'header'=>16, 'pseudo'=>0, 'data'=>undef, 'byteswap'=>0},
> -	    {'name'=>'Ramdisk',           'file'=>'ramdisk.gz',
> -	     'offset'=>0x00160000,        'size'=>0x006a0000,
> -	     'variable'=>1, 'header'=>16, 'pseudo'=>0, 'data'=>undef, 'byteswap'=>0},
> +	    {'name'=>'Kernel2',           'file'=>'vmlinuz2',
> +	     'offset'=>0x00160000,
> +	     'variable'=>1, 'header'=>16, 'pseudo'=>0, 'data'=>undef, 'byteswap'=>0});
> +    }
> +
> +    push @partitions,
> +	   ({'name'=>'Ramdisk',           'file'=>'ramdisk.gz',
> +	     'variable'=>1,               'pseudo'=>0, 'data'=>undef, 'byteswap'=>0,
> +	     'header'=>($loader ? 0 : 16)},

I'd prefer to *always* put a header on the Ramdisk partition.  The stock
Ramdisk partition has a header, so we should keep it for compatibility
(to ease upstream and downstream tools that need to deal with Ramdisks
presented to either RedBoot-based or APEX-bsaed systems.

>  	    {'name'=>'FIS directory',     'file'=>undef,
>  	     'offset'=>0x007e0000,        'size'=>0x00020000,
>  	     'variable'=>0, 'header'=>0,  'pseudo'=>0, 'data'=>undef, 'byteswap'=>0},
> @@ -750,17 +794,48 @@
>  	     'variable'=>1, 'header'=>16, 'pseudo'=>1, 'data'=>undef, 'byteswap'=>0},
>  	    {'name'=>'Trailer',           'file'=>'Trailer',
>  	     'offset'=>0x007ffff0,        'size'=>0x00000010,
> -	     'variable'=>0, 'header'=>0,  'pseudo'=>1, 'data'=>undef, 'byteswap'=>0},
> -	    );
> +	     'variable'=>0, 'header'=>0,  'pseudo'=>1, 'data'=>undef, 'byteswap'=>0});
>  
> +    if (!$loader) {
> +	map { ($_->{'name'} eq 'Kernel') && ($_->{'offset'} = 0x00060000); } @partitions;
> +	map { ($_->{'name'} eq 'Kernel') && ($_->{'size'} = 0x00100000); } @partitions;
> +	map { ($_->{'name'} eq 'Ramdisk') && ($_->{'offset'} = 0x00160000); } @partitions;
> +	map { ($_->{'name'} eq 'Ramdisk') && ($_->{'size'} = 0x006a0000); } @partitions;
> +    }
> +    return @partitions;
>  }
>  
> -# Main routine starts here ...
> +# Split a file into two.  Takes the name of the input file and the size
> +# wanted for the first output file.
> +sub splitFile($$) {
> +    my($file, $size) = @_;
> +    local $/ = undef;
> +    open TMP, $file or die "Can't find file \"$file\": $!\n";
> +    binmode(TMP);
> +    my $data = <TMP>;
> +    close TMP;
> +    # Write the first part
> +    my $file1 = tmpnam();
> +    open TMP, ">$file1" or die "Cannot open file $file1: $!";
> +    binmode(TMP);
> +    push @cleanup, $file1;
> +    print TMP substr($data, 0, $size);
> +    close TMP;
> +    # ... and the second one
> +    my $file2 = tmpnam();
> +    open TMP, ">$file2" or die "Cannot open file $file2: $!";
> +    binmode(TMP);
> +    push @cleanup, $file2;
> +    print TMP substr($data, $size);
> +    close TMP;
> +    return ($file1, $file2);
> +}
>  
> -my(@partitions) = defaultPartitions();
> +# Main routine starts here ...
>  
> -my($unpack, $pack, $little, $input, $output, $redboot, $kernel, $sysconf, $ramdisk, $fisdir, $payload, $trailer, $ethaddr);
> -my (@cleanup);
> +my($unpack, $pack, $little, $input, $output, $redboot);
> +my($kernel, $kernel1, $kernel2, $sysconf, $ramdisk, $fisdir);
> +my($payload, $trailer, $ethaddr, $loader);
>  
>  END {
>      # Remove temporary files
> @@ -784,6 +859,7 @@
>  		"y|payload=s"  => \$payload,
>  		"t|trailer=s"  => \$trailer,
>  		"e|ethaddr=s"  => \$ethaddr,
> +		"L|loader=s"   => \$loader,
>  		) or (not defined $pack and not defined $unpack)) {
>      print "Usage: slugimage <options>\n";
>      print "\n";
> @@ -801,6 +877,7 @@
>      print "  [-f|--fisdir]   <file>		Input/Output FIS directory filename\n";
>      print "  [-y|--payload]  <file>		Input/Output Payload filename\n";
>      print "  [-t|--trailer]  <file>		Input/Output Trailer filename\n";
> +    print "  [-L|--loader]   <file>		Second stage boot loader filename\n";
>      print "  [-e|--ethaddr]  <AABBCCDDEEFF>	Set the Ethernet address\n";
>  
>      # TODO: document --ramdisk syntax
> @@ -808,6 +885,8 @@
>      exit 1;
>  }
>  
> +my(@partitions) = defaultPartitions($loader);
> +
>  if ($pack) {
>      die "Output filename must be specified\n" unless defined $output;
>  
> @@ -849,7 +928,25 @@
>  
>  # Go through the partition options, and set the names and files in @partitions
>  if (defined $redboot) { map { ($_->{'name'} eq 'RedBoot') 	&& ($_->{'file'} = $redboot); } @partitions; }
> -if (defined $kernel)  { map { ($_->{'name'} eq 'Kernel')  	&& ($_->{'file'} = $kernel);  } @partitions; }
> +if (defined $loader)  { map { ($_->{'name'} eq 'Loader')  	&& ($_->{'file'} = $loader);  } @partitions; }
> +if (defined $kernel)  { map { ($_->{'name'} eq 'Kernel') && ($_->{'file'} = $kernel); } @partitions; }
> +if ($loader && $pack) {
> +    # Split the kernel into two, define files for kernel1 and kernel2 und unset
> +    # the original kernel so the pseudo partition doesn't contain any data.
> +    map { ($_->{'name'} eq 'Kernel') && ($kernel = $_->{'file'}); } @partitions;
> +    map { ($_->{'name'} eq 'Kernel') && ($_->{'file'} = undef); } @partitions;
> +    my $size;
> +    map { ($_->{'name'} eq 'Kernel1') && ($size = $_->{'size'}); } @partitions;
> +    ($kernel1, $kernel2) = splitFile($kernel, $size-16);
> +    map { ($_->{'name'} eq 'Kernel1') && ($_->{'file'} = $kernel1); } @partitions;
> +    map { ($_->{'name'} eq 'Kernel2') && ($_->{'file'} = $kernel2); } @partitions;
> +    # Define skip information which the 2nd stage boot loader can use to
> +    # load the kernel.
> +    my @skip;
> +    push @skip, { 'offset' => 0, 'size' => 16 };

Could add:

map { ($_->{'name'} eq 'Ramdisk') && ($_->{'skip'} = \@skip); } @partitions;

here to put the correct skip on the Ramdisk for the header which I would
like to be there.

> +    push @skip, { 'offset' => $size, 'size' => 16 }; # offset is the size of Kernel1
> +    map { ($_->{'name'} eq 'Kernel') && ($_->{'skip'} = \@skip); } @partitions;
> +}
>  if (defined $sysconf) { map { ($_->{'name'} eq 'SysConf') 	&& ($_->{'file'} = $sysconf); } @partitions; }
>  if (defined $fisdir)  { map { ($_->{'name'} eq 'FIS directory') && ($_->{'file'} = $fisdir);  } @partitions; }
>  if (defined $payload) { map { ($_->{'name'} eq 'Payload')       && ($_->{'file'} = $payload); } @partitions; }
> @@ -857,7 +954,10 @@
>  
>  if (defined $little)  {
>      map {
> -	if (($_->{'name'} eq 'Kernel') or
> +	if (($_->{'name'} eq 'Loader') or
> +	    ($_->{'name'} eq 'Kernel') or
> +	    ($_->{'name'} eq 'Kernel1') or
> +	    ($_->{'name'} eq 'Kernel2') or
>  	    ($_->{'name'} eq 'Ramdisk')) {
>  	    $_->{'byteswap'} = 1;
>  	}
> @@ -946,6 +1046,7 @@
>  
>  # Read in the firmware image
>  if ($input) {
> +    map { ($_->{'name'} eq 'Kernel') && ($_->{'variable'} = 1); } @partitions;
>      my $result = readInFirmware($input, \@partitions);
>  }
>  
> @@ -981,6 +1082,15 @@
>      
>      layoutPartitions(@partitions);
>  
> +    if ($loader) {
> +	# The pseudo kernel partitions spans both the kernel1 and kernel2
> +	# partition; set its size accordingly.
> +	my $size = 0;
> +	map { ($_->{'name'} eq 'Kernel1') && ($size += $_->{'size'}); } @partitions;
> +	map { ($_->{'name'} eq 'Kernel2') && ($size += $_->{'size'}); } @partitions;
> +	map { ($_->{'name'} eq 'Kernel') && ($_->{'size'} = $size); } @partitions;
> +    }
> +
>       if ($debug) {
>   	print "after layoutPartitions():\n";
>   	printPartitions(@partitions);
> 

All else looks fine.  Thanks!

-- Rod



Reply to: