Re: [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