[flashrom] [PATCH] Convert chips to block erasers: SST

[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