Support variable-size SPI chip for dummy programmer.

Support variable-size SPI chip for dummy programmer.

  flashrom -p dummy:emulate=VARIABLE_SIZE,image=image_file,size=4M

This is designed for firmware updater to preserve partitions easily. For example, the use case to keep VPDs when upgrading from H2O to H2C.

Change-Id: I84e1a793639cc33b769f6db41be0a0f0b1eacc78

R=dhendrix@chromium.org,hungte@chromiumg.org,reinauer@chromium.org
BUG=Not a bug
TEST=Tested on Z600 and target.
# a H2C file is ready to write.
% flashrom -r H2O  # read existing content from BIOS, including VPDs.
% flashrom -p dummy:emulate=VARIABLE_SIZE,image=H2C,size=4M \
      -i RO_VPD -i RW_VPD -w H2O  # overwrite VPDs from H2O to H2C.
% flashrom -w H2C  # write it down, with VPDs from H2O.

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=e53fa0f

[Louis, 2011-04-01 16:54:59]

resend for Hungte's reference.

David and Stefan, please kindly review this. Thanks.

[Stefan Reinauer, 2011-04-01 17:47:06]

http://codereview.chromium.org/6791015/diff/1/dummyflasher.c
File dummyflasher.c (right):

http://codereview.chromium.org/6791015/diff/1/dummyflasher.c#newcode54
dummyflasher.c:54: EMULATE_VIRTUAL,
Since this emulates a Winbond, should it be called EMULATE_WINBOND_GENERIC?

http://codereview.chromium.org/6791015/diff/1/dummyflasher.c#newcode174
dummyflasher.c:174: "example: %s-65536\n", VIRTUAL_CHIP_NAME);
Can we use KB instead of bytes here? 64, 128, 256, 512 is much nicer to type
than 65536, 131072, 262144, 524288.

http://codereview.chromium.org/6791015/diff/1/dummyflasher.c#newcode357
dummyflasher.c:357: * TODO: map more chips in future for other sizes.
Should we make sure we only pass sizes supported by this RDID?

[Louis, 2011-04-02 01:48:52]

Thanks, Stefan. Your feedbacks inspire me another better approach. Will refine
my code and send for review again. Stay tuned.

http://codereview.chromium.org/6791015/diff/1/dummyflasher.c
File dummyflasher.c (right):

http://codereview.chromium.org/6791015/diff/1/dummyflasher.c#newcode54
dummyflasher.c:54: EMULATE_VIRTUAL,
My initial attempt is to emulate any size of flash chip. But the flashrom
doesn't allow this because it needs at least an entry in flashchips[].

However, I just think another way to approach for this: add a virtual_spi in
flashchips[] and a probe_virtual(). Then, change 
total_size in probe_virtual().

On 2011/04/01 17:47:06, Stefan Reinauer wrote:
> Since this emulates a Winbond, should it be called EMULATE_WINBOND_GENERIC?

http://codereview.chromium.org/6791015/diff/1/dummyflasher.c#newcode174
dummyflasher.c:174: "example: %s-65536\n", VIRTUAL_CHIP_NAME);
This is intentional for

  -p dummy:VIRTUAL-`stat %s H2C`

On 2011/04/01 17:47:06, Stefan Reinauer wrote:
> Can we use KB instead of bytes here? 64, 128, 256, 512 is much nicer to type
> than 65536, 131072, 262144, 524288.

http://codereview.chromium.org/6791015/diff/1/dummyflasher.c#newcode357
dummyflasher.c:357: * TODO: map more chips in future for other sizes.
Will refine this to support any size.

On 2011/04/01 17:47:06, Stefan Reinauer wrote:
> Should we make sure we only pass sizes supported by this RDID?

[dhendrix, 2011-04-02 23:43:32]

http://codereview.chromium.org/6791015/diff/1/dummyflasher.c
File dummyflasher.c (right):

http://codereview.chromium.org/6791015/diff/1/dummyflasher.c#newcode54
dummyflasher.c:54: EMULATE_VIRTUAL,
On 2011/04/02 01:48:52, Louis wrote:
> My initial attempt is to emulate any size of flash chip. But the flashrom
> doesn't allow this because it needs at least an entry in flashchips[].
> 
> However, I just think another way to approach for this: add a virtual_spi in
> flashchips[] and a probe_virtual(). Then, change 
> total_size in probe_virtual().
> 
> On 2011/04/01 17:47:06, Stefan Reinauer wrote:
> > Since this emulates a Winbond, should it be called EMULATE_WINBOND_GENERIC?
> 

SGTM.

http://codereview.chromium.org/6791015/diff/1/dummyflasher.c#newcode174
dummyflasher.c:174: "example: %s-65536\n", VIRTUAL_CHIP_NAME);
On 2011/04/02 01:48:52, Louis wrote:
> This is intentional for
> 
>   -p dummy:VIRTUAL-`stat %s H2C`

Good point... if desired, maybe we can add something that processes suffixes in
the future, ie "128K" or "1M"

http://codereview.chromium.org/6791015/diff/1/dummyflasher.c#newcode174
dummyflasher.c:174: "example: %s-65536\n", VIRTUAL_CHIP_NAME);
I think it would be better to treat size (and potentially other chip info) as
programmer parameters. e.g. dummy:emulate=VIRTUAL:size=65536

I think that is more consistent with flashrom syntax in general, and also you
will be able to use extract_programmer_param() and related functions for string
parsing.

[Louis, 2011-04-07 02:28:14]

David and Stefan,

I've modified code according to your inspiring feedbacks. I also tested it on
Z600 and target machine. Please take a review. Thanks.

http://codereview.chromium.org/6791015/diff/1/dummyflasher.c
File dummyflasher.c (right):

http://codereview.chromium.org/6791015/diff/1/dummyflasher.c#newcode54
dummyflasher.c:54: EMULATE_VIRTUAL,
On 2011/04/02 23:43:32, dhendrix wrote:
> On 2011/04/02 01:48:52, Louis wrote:
> > My initial attempt is to emulate any size of flash chip. But the flashrom
> > doesn't allow this because it needs at least an entry in flashchips[].
> > 
> > However, I just think another way to approach for this: add a virtual_spi in
> > flashchips[] and a probe_virtual(). Then, change 
> > total_size in probe_virtual().
> > 
> > On 2011/04/01 17:47:06, Stefan Reinauer wrote:
> > > Since this emulates a Winbond, should it be called
EMULATE_WINBOND_GENERIC?
> > 
> 
> SGTM.

Done.

http://codereview.chromium.org/6791015/diff/1/dummyflasher.c#newcode174
dummyflasher.c:174: "example: %s-65536\n", VIRTUAL_CHIP_NAME);
On 2011/04/02 23:43:32, dhendrix wrote:
> On 2011/04/02 01:48:52, Louis wrote:
> > This is intentional for
> > 
> >   -p dummy:VIRTUAL-`stat %s H2C`
> 
> Good point... if desired, maybe we can add something that processes suffixes
in
> the future, ie "128K" or "1M"

Done.

http://codereview.chromium.org/6791015/diff/1/dummyflasher.c#newcode174
dummyflasher.c:174: "example: %s-65536\n", VIRTUAL_CHIP_NAME);
On 2011/04/02 23:43:32, dhendrix wrote:
> I think it would be better to treat size (and potentially other chip info) as
> programmer parameters. e.g. dummy:emulate=VIRTUAL:size=65536
> 
> I think that is more consistent with flashrom syntax in general, and also you
> will be able to use extract_programmer_param() and related functions for
string
> parsing.

Done.

[Stefan Reinauer, 2011-04-07 15:32:42]

... otherwise LGTM

http://codereview.chromium.org/6791015/diff/9001/dummyflasher.c
File dummyflasher.c (right):

http://codereview.chromium.org/6791015/diff/9001/dummyflasher.c#newcode38
dummyflasher.c:38: #endif
There's another #if EMULATE_CHIP right below. Maybe unite them?

http://codereview.chromium.org/6791015/diff/9001/dummyflasher.c#newcode131
dummyflasher.c:131: case 'k': case 'K':
wonderful :-)

http://codereview.chromium.org/6791015/diff/9001/dummyflasher.c#newcode147
dummyflasher.c:147: #if EMULATE_CHIP
Move this before previous code block, and only check for EMULATE_SPI_CHIP there?

http://codereview.chromium.org/6791015/diff/9001/dummyflasher.c#newcode597
dummyflasher.c:597: if (emu_chip != EMULATE_VARIABLE_SIZE) return 0;
put return 0 on next line?

http://codereview.chromium.org/6791015/diff/9001/dummyflasher.c#newcode599
dummyflasher.c:599: flash->total_size = emu_chip_size / 1024;
maybe warn if (emu_chip_size % 1024) != 0 ?

http://codereview.chromium.org/6791015/diff/9001/dummyflasher.c#newcode604
dummyflasher.c:604: for (i = 0; i < NUM_ERASEFUNCTIONS; i++) {
This will break eventually, once upstream turns the flashchip structure to
read-only. Despite plans of that happening, who knows if and when it ever will.
So I don't think it's an issue right now.

[Louis, 2011-04-08 01:40:07]

Stefan, thanks for the review. I've refined my code.

http://codereview.chromium.org/6791015/diff/9001/dummyflasher.c
File dummyflasher.c (right):

http://codereview.chromium.org/6791015/diff/9001/dummyflasher.c#newcode38
dummyflasher.c:38: #endif
On 2011/04/07 15:32:42, Stefan Reinauer wrote:
> There's another #if EMULATE_CHIP right below. Maybe unite them?

Done.

http://codereview.chromium.org/6791015/diff/9001/dummyflasher.c#newcode131
dummyflasher.c:131: case 'k': case 'K':
On 2011/04/07 15:32:42, Stefan Reinauer wrote:
> wonderful :-)

Done.

http://codereview.chromium.org/6791015/diff/9001/dummyflasher.c#newcode147
dummyflasher.c:147: #if EMULATE_CHIP
On 2011/04/07 15:32:42, Stefan Reinauer wrote:
> Move this before previous code block, and only check for EMULATE_SPI_CHIP
there?

Done.

http://codereview.chromium.org/6791015/diff/9001/dummyflasher.c#newcode597
dummyflasher.c:597: if (emu_chip != EMULATE_VARIABLE_SIZE) return 0;
On 2011/04/07 15:32:42, Stefan Reinauer wrote:
> put return 0 on next line?

Done.

http://codereview.chromium.org/6791015/diff/9001/dummyflasher.c#newcode599
dummyflasher.c:599: flash->total_size = emu_chip_size / 1024;
On 2011/04/07 15:32:42, Stefan Reinauer wrote:
> maybe warn if (emu_chip_size % 1024) != 0 ? 

Done.

http://codereview.chromium.org/6791015/diff/9001/dummyflasher.c#newcode604
dummyflasher.c:604: for (i = 0; i < NUM_ERASEFUNCTIONS; i++) {
On 2011/04/07 15:32:42, Stefan Reinauer wrote:
> This will break eventually, once upstream turns the flashchip structure to
> read-only. Despite plans of that happening, who knows if and when it ever
will.
> So I don't think it's an issue right now.

Done.