[PATCH] Convert chips to block erasers: SST

[Sean Nelson]

Convert the following chips to block_erasers:
SST28SF040A
SST29EE010
SST29LE010
SST29EE020A
SST29LE020
SST39SF010A
SST39SF020A
SST39SF040
SST39VF512
SST39VF010
SST39VF020
SST39VF040
SST39VF080
SST49LF002A/B
SST49LF003A/B
SST49LF004C
SST49LF008A
SST49LF008C
SST49LF016C
SST49LF020
SST49LF020A
SST49LF040
SST49LF040B
SST49LF080A
SST49LF160C

Extend sst28sf040 to include chip and sector functions for block_eraser.
Extend sst49lfxxxc to include chip, sector, block erasers functions for 
block_erasers.
Extend sst_fwhub to include chip and sector functions for block_erasers.
Add copyrights to changed files.

Signed-off-by: Sean Nelson

[Michael Karcher]

Hello Sean,

thanks for your patch, which looks mostly OK. I didn't check the numbers
very thoroughly, as you will for sure run them through the self-check
code anyways. The layout for chips with main/param/boot is OK. But I
have some remarks nevertheless:
>     {
>             .vendor         = "SST",
>             .name           = "SST49LF002A/B",

[...]
>           .block_erasers  =
>           {
>                   {
>                           .eraseblocks = { {4 * 1024, 64} },
>                           .block_erase = erase_sst_fwhub_sector,
>                   }, {
>                           .eraseblocks = { {16 * 1024, 16} },
>                           .block_erase = erase_sst_fwhub_block,
>                   }, {
>                           .eraseblocks = { {256 * 1024, 1} },
>                           .block_erase = erase_sst_fwhub_chip,
>                   }
>           },

The chip erase function just walks the block_erase_function through the
chip. I don't think it should be listed (again) here. This also applies
to:
>             .name           = "SST49LF003A/B",
>             .name           = "SST49LF008A",
>     {
>             .vendor         = "SST",
>             .name           = "SST49LF020",

[...]
>           .block_erasers  =
>           {
>                   {
>                           .eraseblocks = { {4 * 1024, 64} },
>                           .block_erase = erase_sector_jedec,
>                   }, {
>                           .eraseblocks = { {16 * 1024, 16} },
>                           .block_erase = erase_block_jedec,
>                   }, {
>                           .eraseblocks = { {256 * 1024, 1} },
>                           .block_erase = erase_chip_block_jedec,
>                   }
>           },

This chip supports erase_chip only in A/A mux mode (although the
datasheet calls it "parallel". Do we really want to have that listed?
This also applies to:
>             .name           = "SST49LF040",
>             .name           = "SST49LF040B",
>             .name           = "SST49LF080A",
>     {
>             .vendor         = "SST",
>             .name           = "SST49LF160C",
>             .bustype        = CHIP_BUSTYPE_LPC,

[...]
>           .block_erasers  =
>           {
>                   {
>                           .eraseblocks = { {4 * 1024, 512} },
>                           .block_erase = erase_sector_49lfxxxc,
>                   }, {
>                           .eraseblocks = { 
>                                   {64 * 1024, 31},
>                                   {32 * 1024, 1},
>                                   {8 * 1024, 2},
>                                   {16 * 1024, 1},
>                           },
>                           .block_erase = erase_block_49lfxxxc,
>                   }, {
>                           .eraseblocks = { {2 * 1024 * 1024, 1} },
>                           .block_erase = erase_chip_49lfxxxc,
>                   }
>           },

erase_chip_49lfxxxc is just calling erase_sector_49lfxxxc. No need to
add it as chip erase function.
> +int erase_chip_49lfxxxc(struct flashchip *flash, unsigned int addr, unsigned int blocksize)
> +{
>   if ((addr != 0) || (blocksize != flash->total_size * 1024)) {
>           fprintf(stderr, "%s called with incorrect arguments\n",
>                   __func__);
>           return -1;
>   }
>   return erase_49lfxxxc(flash);
>
> +}

See above, I consider this function useless.
> +int erase_sst_fwhub_chip(struct flashchip *flash, unsigned int addr, unsigned int blocksize)
> +{
>   if ((addr != 0) || (blocksize != flash->total_size * 1024)) {
>           fprintf(stderr, "%s called with incorrect arguments\n",
>                   __func__);
>           return -1;
>   }
>   return erase_sst_fwhub(flash);
>
> +}

This function is useless for the same point. erase_sst_fwhub just walks
all the blocks.

Regards,
  Michael Karcher

[Sean Nelson]

On 1/20/10 2:16 AM, Michael Karcher wrote:
> > .block_erasers =
> > {
> > {
> > .eraseblocks = { {4 * 1024, 64} },
> > .block_erase = erase_sector_jedec,
> > }, {
> > .eraseblocks = { {16 * 1024, 16} },
> > .block_erase = erase_block_jedec,
> > }, {
> > .eraseblocks = { {256 * 1024, 1} },
> > .block_erase = erase_chip_block_jedec,
> > }
> > },
>
> This chip supports erase_chip only in A/A mux mode (although the
> datasheet calls it "parallel". Do we really want to have that listed?
> This also applies to:

I don't think we can determine if the chip is in LPC or A/A mux mode.
> > .block_erasers =
> > {
> > {
> > .eraseblocks = { {4 * 1024, 512} },
> > .block_erase = erase_sector_49lfxxxc,
> > }, {
> > .eraseblocks = {
> > {64 * 1024, 31},
> > {32 * 1024, 1},
> > {8 * 1024, 2},
> > {16 * 1024, 1},
> > },
> > .block_erase = erase_block_49lfxxxc,
> > }, {
> > .eraseblocks = { {2 * 1024 * 1024, 1} },
> > .block_erase = erase_chip_49lfxxxc,
> > }
> > },
>
> erase_chip_49lfxxxc is just calling erase_sector_49lfxxxc. No need to
> add it as chip erase function.

erase_chip_49lfxxc is needed to convert to block_eraser(struct flashrom 
*flash, unsigned int addr, unsigned int size),
its also needed because of unlocking.

[Michael Karcher]

Am Mittwoch, den 20.01.2010, 03:55 -0800 schrieb Sean Nelson:
> On 1/20/10 2:16 AM, Michael Karcher wrote:
> > > .block_erasers =
> > > {
> > > {
> > > .eraseblocks = { {4 * 1024, 64} },
> > > .block_erase = erase_sector_jedec,
> > > }, {
> > > .eraseblocks = { {16 * 1024, 16} },
> > > .block_erase = erase_block_jedec,
> > > }, {
> > > .eraseblocks = { {256 * 1024, 1} },
> > > .block_erase = erase_chip_block_jedec,
> > > }
> > > },
> >
> > This chip supports erase_chip only in A/A mux mode (although the
> > datasheet calls it "parallel". Do we really want to have that listed?
> > This also applies to:
>
> I don't think we can determine if the chip is in LPC or A/A mux mode.

Is there *any* available chipset that uses A/A mux? I think this mode is
only usable by external programmers, and I also don't think that
flashrom is supporting any external programmer that does A/A mux.
> > > .block_erasers =
> > > {
> > > {
> > > .eraseblocks = { {4 * 1024, 512} },
> > > .block_erase = erase_sector_49lfxxxc,
> > > }, {
> > > .eraseblocks = {
> > > {64 * 1024, 31},
> > > {32 * 1024, 1},
> > > {8 * 1024, 2},
> > > {16 * 1024, 1},
> > > },
> > > .block_erase = erase_block_49lfxxxc,
> > > }, {
> > > .eraseblocks = { {2 * 1024 * 1024, 1} },
> > > .block_erase = erase_chip_49lfxxxc,
> > > }
> > > },
> >
> > erase_chip_49lfxxxc is just calling erase_sector_49lfxxxc. No need to
> > add it as chip erase function.
>
> erase_chip_49lfxxc is needed to convert to block_eraser(struct flashrom 
> *flash, unsigned int addr, unsigned int size),
> its also needed because of unlocking.

Right. So erase_sector_49lfxxxc and your new erase_block_49lfxxxc don't
work if you don't unlock beforehands. You might try sector erase anyway
(the chip could be unlocked already), but I would add unlocking stuff
into erase_block_49lfxxxc, as unlocking is done on block level. As the
chip-erase has to be emulated by walking either sectors or blocks, I
don't see the point of doing the walk yourself if the generic eraseblock
walker we are trying to use does exactly that. And after adding
blockwise unlock to erase_block_49lfxxxc, you don't need the unlocking
part of erase_49lfxxxc anymore.

And then you can also kill erase_49lfxxxc completely.

Regards,
  Michael Karcher

[Sean Nelson]

On 1/20/10 4:46 AM, Michael Karcher wrote:
> Am Mittwoch, den 20.01.2010, 03:55 -0800 schrieb Sean Nelson:
> > On 1/20/10 2:16 AM, Michael Karcher wrote:
> > > > .block_erasers =
> > > > {
> > > > {
> > > > .eraseblocks = { {4 * 1024, 64} },
> > > > .block_erase = erase_sector_jedec,
> > > > }, {
> > > > .eraseblocks = { {16 * 1024, 16} },
> > > > .block_erase = erase_block_jedec,
> > > > }, {
> > > > .eraseblocks = { {256 * 1024, 1} },
> > > > .block_erase = erase_chip_block_jedec,
> > > > }
> > > > },
> > >
> > > This chip supports erase_chip only in A/A mux mode (although the
> > > datasheet calls it "parallel". Do we really want to have that listed?
> > > This also applies to:
> >
> > I don't think we can determine if the chip is in LPC or A/A mux mode.
>
> Is there *any* available chipset that uses A/A mux? I think this mode is
> only usable by external programmers, and I also don't think that
> flashrom is supporting any external programmer that does A/A mux.

I think we should just keep it, in case that day comes....

Convert the following chips to block_erasers:
SST28SF040A
SST29EE010
SST29LE010
SST29EE020A
SST29LE020
SST39SF010A
SST39SF020A
SST39SF040
SST39VF512
SST39VF010
SST39VF020
SST39VF040
SST39VF080
SST49LF002A/B
SST49LF003A/B
SST49LF004C
SST49LF008A
SST49LF008C
SST49LF016C
SST49LF020
SST49LF020A
SST49LF040
SST49LF040B
SST49LF080A
SST49LF160C

Extend sst28sf040 to include chip and sector functions for block_eraser.
Extend sst49lfxxxc to include chip, sector, block erasers functions for 
block_erasers.
Extend sst_fwhub to include chip and sector functions for block_erasers.
Add copyrights to changed files.
Killed erase_sst_fwhub.
Killed erase_49lfxxxc.

Signed-off-by: Sean Nelson

[Michael Karcher]

Am Mittwoch, den 20.01.2010, 05:19 -0800 schrieb Sean Nelson:
> -static int erase_sector_49lfxxxc(struct flashchip *flash, unsigned long address, int sector_size)
> +int erase_sector_49lfxxxc(struct flashchip *flash, unsigned int address, unsigned int sector_size)
>  {
>         unsigned char status;
>         chipaddr bios = flash->virtual_memory;
>   unlock_block_49lfxxxc(flash, (address & 0xffff0000), 0);

because of non-uniform block size, this is not going to work in the top
block.

Regards,
  Michael Karcher.

[Sean Nelson]

On Wed, Jan 20, 2010 at 5:53 AM, Michael Karcher
 wrote:
> because of non-uniform block size, this is not going to work in the top
> block.

If you take a look at the Addresses in Table 14 on page 19 of
SST49LF016C; it should (according to the datasheet) work that way.

[Carl-Daniel Hailfinger]

On 20.01.2010 13:46, Michael Karcher wrote:
> Am Mittwoch, den 20.01.2010, 03:55 -0800 schrieb Sean Nelson:
> > On 1/20/10 2:16 AM, Michael Karcher wrote:
> > > This chip supports erase_chip only in A/A mux mode (although the
> > > datasheet calls it "parallel". Do we really want to have that listed?
> > > This also applies to:
> >
> > I don't think we can determine if the chip is in LPC or A/A mux mode.
>
> Is there *any* available chipset that uses A/A mux? I think this mode is
> only usable by external programmers, and I also don't think that
> flashrom is supporting any external programmer that does A/A mux.

AFAIK no chipset uses A/A mux mode. It won't be listed in
.buses_supported (see the comments about A/A mux in flashchips.c), so it
is a safe assumption that commands for A/A mux mode are unavailable for
all practical purposes. My suggestion is to set .block_erase=NULL until
we handle this differently.
> > > > .block_erasers =
> > > > {
> > > > {
> > > > .eraseblocks = { {4 * 1024, 512} },
> > > > .block_erase = erase_sector_49lfxxxc,
> > > > }, {
> > > > .eraseblocks = {
> > > > {64 * 1024, 31},
> > > > {32 * 1024, 1},
> > > > {8 * 1024, 2},
> > > > {16 * 1024, 1},
> > > > },
> > > > .block_erase = erase_block_49lfxxxc,
> > > > }, {
> > > > .eraseblocks = { {2 * 1024 * 1024, 1} },
> > > > .block_erase = erase_chip_49lfxxxc,
> > > > }
> > > > },
> > >
> > > erase_chip_49lfxxxc is just calling erase_sector_49lfxxxc. No need to
> > > add it as chip erase function.
> >
> > erase_chip_49lfxxc is needed to convert to block_eraser(struct flashrom 
> > *flash, unsigned int addr, unsigned int size),
> > its also needed because of unlocking.
>
> Right. So erase_sector_49lfxxxc and your new erase_block_49lfxxxc don't
> work if you don't unlock beforehands. You might try sector erase anyway
> (the chip could be unlocked already), but I would add unlocking stuff
> into erase_block_49lfxxxc, as unlocking is done on block level. As the
> chip-erase has to be emulated by walking either sectors or blocks, I
> don't see the point of doing the walk yourself if the generic eraseblock
> walker we are trying to use does exactly that. And after adding
> blockwise unlock to erase_block_49lfxxxc, you don't need the unlocking
> part of erase_49lfxxxc anymore.
>
> And then you can also kill erase_49lfxxxc completely.

The problem with locking is that locking will always have boundaries
independent of eraseblock structure. My patch at
http://patchwork.coreboot.org/patch/581/ tries to decouple locking from
all other functions, and AFAICS it would help here a lot.

Regards,
Carl-Daniel