Re: [flashrom] [PATCH, v2] Intel 28F004/28F400 support

[Carl-Daniel Hailfinger]

On 01.04.2010 20:12, Michael Karcher wrote:
> Remove blockwise write for i82802ab chips. It will be reintroduced
> in post-0.9.2 in a generic way. This is needed to fix
> FWH-like chips with non-uniform sectors.
>
> These are:
>   Intel 28F001
>   Sharp LHF00L04
>   ST M50FW002
>   ST M50LPW116
>
> Add IDs for Intel 28F004/28F400

I have modified the changelog a bit.
> Signed-off-by: Michael Karcher

With the comments below addressed, this is
Acked-by: Carl-Daniel Hailfinger
> 82802ab.c    |   54 +++++++++-------------------
>  flash.h      |    2 +-
>  flashchips.c |  110 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  flashchips.h |    4 ++
>  4 files changed, 133 insertions(+), 37 deletions(-)
>  create mode 100755 dmidecode
>
> diff --git a/82802ab.c b/82802ab.c
> index aa7e45e..0d1bd1a 100644
> --- a/82802ab.c
> +++ b/82802ab.c
> @@ -48,6 +48,7 @@ int probe_82802ab(struct flashchip *flash)
>    chipaddr bios = flash->virtual_memory;
>    uint8_t id1, id2;
>    uint8_t flashcontent1, flashcontent2;
> int shifted = (flash->feature_bits & FEATURE_ADDR_SHIFTED) != 0;

Can we rely on the compiler to set shifted=1 if the feature bit is set?
We assign a boolean value to int shifted, and AFAIK the compiler is free
to use pretty much any value here as long as that value gives reliable
results (could be 0x2, could be 0xffffffff). What about the following
code instead?

+	int shifted = (flash->feature_bits & FEATURE_ADDR_SHIFTED) ? 1 : 0;
> /* Reset to get a clean state */
>    chip_writeb(0xFF, bios);
> @@ -178,44 +179,25 @@ void write_page_82802ab(chipaddr bios, uint8_t *src,
>  int write_82802ab(struct flashchip *flash, uint8_t *buf)
>  {
>    int i;
>
> int total_size = flash->total_size * 1024;
>
> int page_size = flash->page_size;
> chipaddr bios = flash->virtual_memory;
>
> uint8_t *tmpbuf = malloc(page_size);
>
> if (!tmpbuf) {
> msg_cerr("Could not allocate memory!\n");
> exit(1);
> if (erase_flash(flash)) {
> msg_cerr("ERASE FAILED!\n");
> return -1;
> }
> msg_cinfo("Programming page: \n");
> for (i = 0; i < total_size / page_size; i++) {
>
> [...]
> msg_cinfo("Programming page: ");
> for (i = 0; i < flash->total_size; i++) {

You change the write chunk size from page_size to 1024. Does
"programming page" still make sense, or should it be "programming chunk at"?
> diff --git a/dmidecode b/dmidecode
> new file mode 100755
> index 0000000..e69de29

Uhhh. Please make sure you don't commit dmidecode by accident.
> diff --git a/flashchips.c b/flashchips.c
> index 2dbc1e0..a9b6551 100644
> --- a/flashchips.c
> +++ b/flashchips.c
> @@ -2385,6 +2385,116 @@ struct flashchip flashchips[] = {
> {
> .vendor		= "Intel",
> .name		= "28F400BV/CV/CE-B",
> .bustype	= CHIP_BUSTYPE_PARALLEL,
> .manufacture_id	= INTEL_ID,
> .model_id	= P28F400BB,
> },
> {
> .vendor		= "Intel",
> .name		= "28F400BV/CV/CE-B",

Should be ...CE-T unless I'm mistaken.
> .bustype	= CHIP_BUSTYPE_PARALLEL,
> .manufacture_id	= INTEL_ID,
> .model_id	= P28F400BT,

A compile test and run test with -p dummy (which triggers the automatic
chipdefinition selftest) would be appreciated before committing just to
make sure I didn't overlook anything problematic.

Regards,
Carl-Daniel

[PATCH, v2] Intel 28F004/28F400 support

[Michael Karcher]

Am Samstag, den 03.04.2010, 12:08 +0200 schrieb Carl-Daniel Hailfinger:
> > Remove blockwise write for i82802ab chips. It will be reintroduced
> > in post-0.9.2 in a generic way. This is needed to fix
> > FWH-like chips with non-uniform sectors.
>
> I have modified the changelog a bit.

Fine, adjusted.
> > Signed-off-by: Michael Karcher
>
> Acked-by: Carl-Daniel Hailfinger

Thanks. Committed in r991
> > int shifted = (flash->feature_bits & FEATURE_ADDR_SHIFTED) != 0;
>
> Can we rely on the compiler to set shifted=1 if the feature bit is set?

As discussed on IRC, we can.
> > msg_cinfo("Programming page: ");
> > for (i = 0; i < flash->total_size; i++) {
>
> You change the write chunk size from page_size to 1024. Does
> "programming page" still make sense, or should it be "programming chunk at"?

I made it just "programming at".
> > diff --git a/dmidecode b/dmidecode
> > new file mode 100755
> > index 0000000..e69de29
>
> Uhhh. Please make sure you don't commit dmidecode by accident.

Thanks for catching that. I swear to never use "add all" again.
> > .vendor		= "Intel",
> > .name		= "28F400BV/CV/CE-B",
>
> Should be ...CE-T unless I'm mistaken.

It should. I fixed it.
> A compile test and run test with -p dummy (which triggers the automatic
> chipdefinition selftest) would be appreciated before committing just to
> make sure I didn't overlook anything problematic.

Done. Selftest passed.

Regards,
  Michael Karcher