Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(401)

Issue 6594134: Create new cros_write_firmware tool based on write_tegra_bios. (Closed)

Created:
9 years, 9 months ago by robotboy
Modified:
9 years, 7 months ago
Reviewers:
ggg, sjg, vb, Che-Liang Chiou
CC:
chromium-os-reviews_chromium.org
Visibility:
Public.

Description

Create new cros_write_firmware tool based on write_tegra_bios. This tool uses U-Boot instead of the binary bootloader.bin to do the actual flashing of the boot device. This gives us more control over the firmware flashing process. In particular, U-Boot has SPI support that we can customize, which allows us to write a chromium firmware image directly to SPI. BUG=chromium-os:11981 TEST=Build and boot from SPI U-Boot/kernel/ChromiumOS on Kaen Change-Id: I37f6b3d00b23bec0e79da2282b2f0e75fce76900 This CL depends on: http://codereview.chromium.org/6606011/ http://codereview.chromium.org/6609003/ http://codereview.chromium.org/6609004/ Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=06565e3

Patch Set 1 #

Total comments: 6

Patch Set 2 : Quote commands in U-Boot script. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+211 lines, -0 lines) Patch
A host/cros_write_firmware View 1 chunk +173 lines, -0 lines 0 comments Download
A host/share/cros_write_firmware/nvflash-spi.cfg View 1 chunk +22 lines, -0 lines 0 comments Download
A host/share/cros_write_firmware/spi.script View 1 1 chunk +16 lines, -0 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
robotboy
9 years, 9 months ago (2011-03-02 23:26:25 UTC) #1
vb
Please see a few comments below, also, please add actual description of the test procedure. ...
9 years, 9 months ago (2011-03-04 00:58:31 UTC) #2
robotboy
Thanks, comments addressed and/or done. PTAL, Anton http://codereview.chromium.org/6594134/diff/1/host/cros_write_firmware File host/cros_write_firmware (right): http://codereview.chromium.org/6594134/diff/1/host/cros_write_firmware#newcode71 host/cros_write_firmware:71: echo "setenv ...
9 years, 9 months ago (2011-03-04 16:58:56 UTC) #3
ggg
9 years, 9 months ago (2011-03-04 23:17:29 UTC) #4
LGTM.

On 2011/03/04 16:58:56, robotboy wrote:
> Thanks, comments addressed and/or done.

Tested on Seaboard. I've looked through the files and don't see anything
patently offensive. In fact, good comments embedded in the file in general.

cheers,
grant


> 
> PTAL,
>     Anton
> 
> http://codereview.chromium.org/6594134/diff/1/host/cros_write_firmware
> File host/cros_write_firmware (right):
> 
>
http://codereview.chromium.org/6594134/diff/1/host/cros_write_firmware#newcode71
> host/cros_write_firmware:71: echo "setenv address       0xe49000"
> On 2011/03/04 00:58:31, vb wrote:
> > where does this number come from? Can it be derived from some common source?
> 
> The TODO in the description of the function above explains in pretty good
detail
> where this number comes from.
> 
> > Also, I looked at host/share/cros_write_firmware/spi.script
> > before this file, it seems to me now that the whole thing
> > would be better readable if the spi.script was incorporated
> > here, it's pretty short.
> 
> While it is short, it is much less readable as a script when inlined because
of
> shell escapes required to prevent bash from interpreting the U-Boot variable
> dereferences as shell dereferences.  Also, there will soon be a nand.script
and
> an emmc.script.  So including those here will end up bloating things more.
> 
> > and since you generate it all here you could forgo defining the variables in
> > lines 71...73 and use the values in the
> > script directly instead.
> 
> I explicitly didn't do this so that you can break into the boot delay sequence
> once the flasher image has booted and change these values/run the _commands
> manually.  If I substituted the values directly it would be much more
difficult
> to reuse the _commands.  Also, the above reasoning (nand and emmc scripts
coming
> soon) holds.
> 
>
http://codereview.chromium.org/6594134/diff/1/host/cros_write_firmware#newcod...
> host/cros_write_firmware:173: info "your flash was probably successful."
> On 2011/03/04 00:58:31, vb wrote:
> > are you being too conservative here when you say 'probably'?
> 
> Because there could have been a transmission failure when nvflash wrote the
> combined flasher + script + payload to the device.  Thus even if the values
> written match the values that were initially in RAM when the flasher started,
> they may not match the values that are on the host file system.  The solution
as
> I point out in my TODO is to make a tool that can generate the same CRCs on
the
> host so that that CRC can be compared to the after CRC on the target.
> 
>
http://codereview.chromium.org/6594134/diff/1/host/share/cros_write_firmware/...
> File host/share/cros_write_firmware/spi.script (right):
> 
>
http://codereview.chromium.org/6594134/diff/1/host/share/cros_write_firmware/...
> host/share/cros_write_firmware/spi.script:1: setenv _crc    crc32 ${address}
> ${firmware_size}
> On 2011/03/04 00:58:31, vb wrote:
> > This is a u-boot script. Can you please put the command sequences in double
> > quotes.  This way semicolon escapes won't be needed.
> 
> Done.

Powered by Google App Engine
This is Rietveld 408576698