flashrom: Add tested field to flashchips table.

[Peter Stuge]

Patch attached.

If/when this is acked and committed I think it would be nice to mark
lots of chips tested before the release. I suggest that we self-ack
any patches that only change chip test status from TEST_UNTESTED.

//Peter

[Carl-Daniel Hailfinger]

On 02.05.2008 18:29, Peter Stuge wrote:
> Patch attached.
>
> If/when this is acked and committed I think it would be nice to mark
> lots of chips tested before the release. I suggest that we self-ack
> any patches that only change chip test status from TEST_UNTESTED.
>
> //Peter

Two minor comments:
- People should have a separate address to turn to for any flashrom
success reports. Subscribing to a list just for that purpose is
user-hostile.
- We need separate test status for probe/read/erase/verify.

If the issues above are addressed, the patch is
Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel

[Joe]

On Fri, 02 May 2008 18:34:34 +0200, Carl-Daniel Hailfinger
 wrote:
> On 02.05.2008 18:29, Peter Stuge wrote:
> > Patch attached.
> >
> > If/when this is acked and committed I think it would be nice to mark
> > lots of chips tested before the release. I suggest that we self-ack
> > any patches that only change chip test status from TEST_UNTESTED.
> >
> > //Peter
>
> Two minor comments:
> People should have a separate address to turn to for any flashrom
>
> success reports. Subscribing to a list just for that purpose is
> user-hostile.
> We need separate test status for probe/read/erase/verify.
>
> If the issues above are addressed, the patch is
> Acked-by: Carl-Daniel Hailfinger

Why not just put a link in the manpage pointing to
http://www.coreboot.org/Flashrom#Supported_devices
to show which chips have been tested? This why it is not redundent, right?

flashrom: Add tested bitmap to flashchips table.

[Peter Stuge]

On Fri, May 02, 2008 at 06:34:34PM +0200, Carl-Daniel Hailfinger wrote:
> Two minor comments:
> People should have a separate address to turn to for any flashrom
>
> success reports. Subscribing to a list just for that purpose is
> user-hostile.

Changed to [e-mail] now. How we handle that adress I
don't know.
> We need separate test status for probe/read/erase/verify.

Good idea. I've changed the patch.
> If the issues above are addressed, the patch is
> Acked-by: Carl-Daniel Hailfinger

Thanks! I'll hold off committing the attached patch for one more
round of comments though.

On Fri, May 02, 2008 at 01:23:52PM -0400, Joe wrote:
> Why not just put a link in the manpage pointing to
> http://www.coreboot.org/Flashrom#Supported_devices
> to show which chips have been tested?

A good idea, but unfortunately the wiki does not know what version of
the program the user is running. If the user has an old version, bugs
may have been fixed since the release to make more chips work, which
is what the wiki would say, but they will still not work in the
user's version. :\

//Peter

flashrom: Add tested bitmap to flashchips table.

[Carl-Daniel Hailfinger]

On 02.05.2008 19:50, Peter Stuge wrote:
> On Fri, May 02, 2008 at 06:34:34PM +0200, Carl-Daniel Hailfinger wrote:
> > Two minor comments:
> > People should have a separate address to turn to for any flashrom
> >
> > success reports. Subscribing to a list just for that purpose is
> > user-hostile.
>
> Changed to [e-mail] now. How we handle that adress I
> don't know.

Thanks. As long as Stefan can create a mailbox with that name, we don't
have to decide immediately how to handle this.
> > We need separate test status for probe/read/erase/verify.
>
> Good idea. I've changed the patch.

Great.
> > If the issues above are addressed, the patch is
> > Acked-by: Carl-Daniel Hailfinger
>
> Thanks! I'll hold off committing the attached patch for one more
> round of comments though.
>
> On Fri, May 02, 2008 at 01:23:52PM -0400, Joe wrote:
> > Why not just put a link in the manpage pointing to
> > http://www.coreboot.org/Flashrom#Supported_devices
> > to show which chips have been tested?
>
> A good idea, but unfortunately the wiki does not know what version of
> the program the user is running. If the user has an old version, bugs
> may have been fixed since the release to make more chips work, which
> is what the wiki would say, but they will still not work in the
> user's version. :\

Indeed.
> flashrom: Add a tested bitmap field to the flash chip table.
>
> Two bits indicate OK and BAD for each operation PROBE READ ERASE WRITE.
> All 8 bits are in use now. No bits set means nothing has been tested.
> For chips with at least one operation that is not tested or not working, the
> user is asked to email a report to a special email adress so that the table
> can be updated.
>
> All chips are TEST_UNTESTED for now.
>
> Signed-off-by: Peter Stuge
> Index: flashrom.2.chiptested/flash.h
>
> --- flashrom.2.chiptested/flash.h	(revision 3276)
> +++ flashrom.2.chiptested/flash.h	(working copy)
> @@ -45,6 +45,11 @@
>    int total_size;
>    int page_size;
> /* Indicate if flashrom has been tested with this flash chip and if
> * everything worked correctly.
> */
> uint8_t tested;

Due to alignment of the subsequent struct member, we'll waste 24 bits
here. We might as well make this a 32bit variable. It's not needed right
now, though.
> int (*probe) (struct flashchip *flash);
>  int (*erase) (struct flashchip *flash);
>  int (*write) (struct flashchip *flash, uint8_t *buf);
>
> @@ -55,6 +60,20 @@
>    volatile uint8_t *virtual_registers;
>  };
>
> +#define TEST_UNTESTED	0
>
> +#define TEST_OK_PROBE	(1<<0)
> +#define TEST_OK_READ	(1<<1)
> +#define TEST_OK_ERASE	(1<<2)
> +#define TEST_OK_WRITE	(1<<3)
> +#define TEST_OK_MASK	0x0f
>
> +#define TEST_BAD_PROBE	(1<<4)
> +#define TEST_BAD_READ	(1<<5)
> +#define TEST_BAD_ERASE	(1<<6)
> +#define TEST_BAD_WRITE	(1<<7)
> +#define TEST_BAD_MASK	0xf0
>
> extern struct flashchip flashchips[];
>
> /*
> Index: flashrom.2.chiptested/flashrom.c
> ===================================================================
> --- flashrom.2.chiptested/flashrom.c	(revision 3276)
> +++ flashrom.2.chiptested/flashrom.c	(working copy)
> @@ -412,6 +412,36 @@
>    }
>
> printf("Flash part is %s (%d KB).\n", flash->name, flash->total_size);
> if (TEST_OK_MASK != (flash->tested & TEST_OK_MASK)) {
> printf("--\n");

The printf above may be dangerous if it is interpreted as a signature
separator by some e-mail program...
> if (flash->tested & TEST_BAD_MASK) {
> 	printf("This flash part has status NOT WORKING for operations:");
> 	if (flash->tested & TEST_BAD_PROBE)
> 		printf(" PROBE");
> 	if (flash->tested & TEST_BAD_READ)
> 		printf(" READ");
> 	if (flash->tested & TEST_BAD_ERASE)
> 		printf(" ERASE");
> 	if (flash->tested & TEST_BAD_WRITE)
> 		printf(" WRITE");
> 	printf("\n");
> } else {
> 	printf("This flash part has status UNTESTED for operations:");
> 	if (!(flash->tested & TEST_OK_PROBE))
> 		printf(" PROBE");
> 	if (!(flash->tested & TEST_OK_READ))
> 		printf(" READ");
> 	if (!(flash->tested & TEST_OK_ERASE))
> 		printf(" ERASE");
> 	if (!(flash->tested & TEST_OK_WRITE))
> 		printf(" WRITE");
> 	printf("\n");
> }
> printf("Please email a report to [e-mail] if any of the above operations\n");
> printf("work correctly for you with this flash part. Please also mention which chipset\n");
> printf("the program found. Thank you for your help!\n");

Maybe add: "In doubt, mail the whole output of flashrom."
That also begs the question whether we want to echo the flashrom
parameters by default.
> printf("--\n");
>
> }
>
> if (!(read_it | write_it | verify_it | erase_it)) {
> printf("No operations were specified.\n");

This is an almost ackable form. As long as you answer the questions
above (you don't have to follow the suggestions), this is
Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel

flashrom: Add tested bitmap to flashchips table.

[Peter Stuge]

Thanks for your comments!

On Sat, May 03, 2008 at 03:40:17AM +0200, Carl-Daniel Hailfinger wrote:
> > /* Indicate if flashrom has been tested with this flash chip and if
> > * everything worked correctly.
> > */
> > uint8_t tested;
>
> Due to alignment of the subsequent struct member, we'll waste 24 bits
> here. We might as well make this a 32bit variable. It's not needed
> right now, though.

It felt a bit cramped with no free bits so I changed it to uint32_t.
> > if (TEST_OK_MASK != (flash->tested & TEST_OK_MASK)) {
> > printf("--\n");
>
> The printf above may be dangerous if it is interpreted as a
> signature separator by some e-mail program...

Changed to ===
> Maybe add: "In doubt, mail the whole output of flashrom."

I changed the wording a bit.
> That also begs the question whether we want to echo the flashrom
> parameters by default.

Maybe, but I don't know if that would be useful?
> Acked-by: Carl-Daniel Hailfinger

Thanks! r3277.

//Peter

flashrom: Add tested bitmap to flashchips table.

[Carl-Daniel Hailfinger]

On 03.05.2008 06:36, Peter Stuge wrote:
> Thanks for your comments!

Thanks for taking them into account!
> On Sat, May 03, 2008 at 03:40:17AM +0200, Carl-Daniel Hailfinger wrote:
> > > /* Indicate if flashrom has been tested with this flash chip and if
> > > * everything worked correctly.
> > > */
> > > uint8_t tested;
> >
> > Due to alignment of the subsequent struct member, we'll waste 24 bits
> > here. We might as well make this a 32bit variable. It's not needed
> > right now, though.
>
> It felt a bit cramped with no free bits so I changed it to uint32_t.
> > > if (TEST_OK_MASK != (flash->tested & TEST_OK_MASK)) {
> > > printf("--\n");
> >
> > The printf above may be dangerous if it is interpreted as a
> > signature separator by some e-mail program...
>
> Changed to ===
> > Maybe add: "In doubt, mail the whole output of flashrom."
>
> I changed the wording a bit.
> > That also begs the question whether we want to echo the flashrom
> > parameters by default.
>
> Maybe, but I don't know if that would be useful?

Just that if the user reports "It works" without further info, we know
immediately what works.
> > Acked-by: Carl-Daniel Hailfinger
>
> Thanks! r3277.

Thanks!

Regards,
Carl-Daniel