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