[LinuxBIOS] GA-M57SLI SPI support

[Carl-Daniel Hailfinger]

On 28.09.2007 16:55, Uwe Hermann wrote:
> On Fri, Sep 28, 2007 at 03:35:54PM +0200, Carl-Daniel Hailfinger wrote:
>> On 27.09.2007 16:30, Ward Vandewege wrote:
>>> On Thu, Sep 27, 2007 at 11:31:14AM +0200, Carl-Daniel Hailfinger wrote:
>>>> No problem. Can you commit with your changes? Changelog follows:
>>>>
>>>> Add preliminary SPI flash identification support for SPI chips attached
>>>> to ITE IT8716F Super I/O. Right now this is hardcoded to the Gigabyte
>>>> M57SLI board. It works only with rev 2.0 of the board, but it will bail
>>>> out on earlier versions, so no damage can occur.
>>>>
>>>> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
>>> Acked-by: Ward Vandewege <ward at gnu.org>
>>>
>>> and committed in r2811
>> Thanks for committing!
>>
>> Here is a patch against current svn which aims to restructure SPI flash
>> support in a more reasonable way. Please note that the patch is not
>> ready to be committed, but it should work much better than the existing
> 
> Yep, Signed-off-by missing for this one (yeah, there's one for the older
> version, but still)...

Intentional. Was not ready to be applied.

>> one, namely without the dirty "do everything in board_enable functions"
>> hack. Functions still have to be moved to another file, but for initial
>> testing, this should be fine.
>>
>> Regards,
>> Carl-Daniel
>>
>> Index: util/flashrom/flash.h
>> ===================================================================
>> --- util/flashrom/flash.h	(Revision 2811)
>> +++ util/flashrom/flash.h	(Arbeitskopie)
>> @@ -68,8 +68,31 @@
>>  #define AT_29C040A		0xA4
>>  #define AT_29C020		0xDA
>>  
>> +#define EON_ID			0x1C
>> +/* EN25 chips are SPI, first byte of device id is memory type,
>> +   second byte of device id is log(bitsize)-9 */
>> +#define EN_25B05		0x2010	/* 2^19 kbit or 2^16 kByte */
>> +#define EN_25B10		0x2011
>> +#define EN_25B20		0x2012
>> +#define EN_25B40		0x2013
>> +#define EN_25B80		0x2014
>> +#define EN_25B16		0x2015
>> +#define EN_25B32		0x2016
>> +
>>  #define MX_ID			0xC2	/* Macronix (MX) */
>>  #define MX_29F002		0xB0
>> +/* MX25L chips are SPI, first byte of device id is memory type,
>> +   second byte of device id is log(bitsize)-9 */
>> +#define MX_25L512		0x2010	/* 2^19 kbit or 2^16 kByte */
>> +#define MX_25L1005		0x2011
>> +#define MX_25L2005		0x2012
>> +#define MX_25L4005		0x2013	/* MX25L4005{,A} */
>> +#define MX_25L8005		0x2014
>> +#define MX_25L1605		0x2015	/* MX25L1605{,A,D} */
>> +#define MX_25L3205		0x2016	/* MX25L3205{,A} */
>> +#define MX_25L6405		0x2017	/* MX25L3205{,D} */
>> +#define MX_25L1635D		0x2415
>> +#define MX_25L3235D		0x2416
> 
> Maybe we should separate out the IDs for SPI chips in a different
> section of the header file (to not confuse people into assuing they're
> LPC parts or so)?

All SPI parts have 16-bit device IDs, I'll add a comment about that.

>>  #define SHARP_ID		0xB0	/* Sharp */
>>  #define SHARP_LHF00L04		0xCF
>> @@ -182,6 +205,8 @@
>>  int linuxbios_init(void);
>>  extern char *lb_part, *lb_vendor;
>>  
>> +int probe_spi(struct flashchip *flash);
> 
> Can be
>   int probe_spi(const struct flashchip *flash);
> I think.

Maybe, but that's a separate cleanup. This function prototype matches
the others.

>> Index: util/flashrom/board_enable.c
>> ===================================================================
>> --- util/flashrom/board_enable.c	(Revision 2811)
>> +++ util/flashrom/board_enable.c	(Arbeitskopie)
>> @@ -96,35 +96,100 @@
>>  	return flashport;
>>  }
>>  
>> -static void it8716_serial_rdid(uint16_t port)
>> +/* The IT8716 only supports commands with length 1,2,4,5 bytes including
> 
> IT8716F please, as per datasheet. AFAICS all ITEs should have the F at the
> end of the name. Applies to several places in the code and comments.

OK.

>> +   command byte and can not read more than 3 bytes from the device.
>> +   This function expects writearr[0] to be the first byte sent to the device,
>> +   whereas the IT8716 splits commands internally into address and non-address
>> +   commands with the address in inverse wire order. That's why the register
>> +   ordering in case 4 and 5 may seem strange. */
>> +static int it8716_spi_command(uint16_t port, unsigned char writecnt, unsigned char readcnt, const unsigned char *writearr, unsigned char *readarr)
> 
> If the size of writecnt/readcnt/writearr/readarr etc. is fixed/specified in
> the datasheet then please make it uint16_t etc. instead of the "generic"
> unsigned char and friends.

{read,write}cnt are between 0 and 5, so they could as well be int. No
datasheet specification for their variable size.
{read,write}arr are just helper constructs to abstract the horrible
datasheet variable types. Or do you honestly want a 24-bit type?


>> +static int it8716_serial_rdid(uint16_t port, unsigned char *readarr)
>> +{
>> +	/* RDID is 0x9f */
>> +	const unsigned char cmd[] = {0x9f};
> 
> #define RDID 0x9f
> 
> at the top of the file?

Can do, but it will get messy very soon because there is no way to
sensibly specify command value, input size, output size in one #define.
Furthermore, depending on the state of the chip, the commands change
values/input/output size. So I'd prefer to keep it that way right now
until we figure out a better abstraction which doesn't complicate stuff.

>> +
>> +	if (it8716_spi_command(port, 1, 3, cmd, readarr))
>> +		return 1;
>> +	printf("RDID returned %02x %02x %02x\n", readarr[0], readarr[1], readarr[2]);
>> +	return 0;
>> +}
>> +
>> +static uint16_t it8716_flashport = 0;
> 
> Move this to the top of the file, please.

OK.

>>  static int it87xx_probe_serial_flash(const char *name)
>>  {
>> -	uint16_t flashport;
>> -	flashport = find_ite_serial_flash_port(0x2e);
>> -	if (flashport)
>> -		it8716_serial_rdid(flashport);
>> -	flashport = find_ite_serial_flash_port(0x4e);
>> -	if (flashport)
>> -		it8716_serial_rdid(flashport);
>> +	it8716_flashport = find_ite_serial_flash_port(0x2e);
>> +	if (!it8716_flashport)
>> +		it8716_flashport = find_ite_serial_flash_port(0x4e);
> 
> #defines for 0x2e and 0x4e.

OK.

>> +	return (!it8716_flashport);
>> +}
>> +
>> +int probe_spi(struct flashchip *flash)
>> +{
>> +	unsigned char readarr[3];
>> +	uint8_t manuf_id;
>> +	uint16_t model_id;
>> +	if (it8716_flashport) {
>> +		it8716_serial_rdid(it8716_flashport, readarr);
>> +		manuf_id = readarr[0];
>> +		model_id = (readarr[1] << 8) | readarr[2];
>> +		printf_debug("%s: id1 0x%x, id2 0x%x\n", __FUNCTION__, manuf_id, model_id);
>> +		if (manuf_id == flash->manufacture_id && model_id == flash->model_id)
>> +			return 1;
>> +	}
>> +
>>  	return 0;
>>  }
> 
> 
> Looks good otherwise, but should be tested of course.

Yes, looking for testers.

Carl-Daniel