[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 flashrom at coreboot.org 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 <c-d.hailfinger.devel.2006 at gmx.net>
>>     
>
> 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 <peter at stuge.se>
>
>
> 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 flashrom at coreboot.org 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 <c-d.hailfinger.devel.2006 at gmx.net>


Regards,
Carl-Daniel