[LinuxBIOS] GA-M57SLI SPI support

[Carl-Daniel Hailfinger]

On 25.09.2007 01:17, Uwe Hermann wrote:
> On Mon, Sep 24, 2007 at 12:10:15PM +0200, Carl-Daniel Hailfinger wrote:
>> +uint8_t regval(uint16_t port, uint8_t reg)
> 
> Why not regread() btw? Just a minor issue.

Straight copy from flashrom. Besides that, val is shorter and sounds nicer.

>> +	if (id == 0x8716) {
>> +		/* LDN 0x0, reg 0x24, mask out lowest bit (suspend) */
>> +		regwrite(port, 0x07, 0x0);
>> +		tmp = regval(port, 0x24) & 0xFE;
> 
> Why LDN 0? Isn't that for floppy stuff? 0x24 can be manipulated
> regardless of any LDN, IIRC.

superiotool output was a bit misleading. LDN 0 isn't needed.

>> +static void it8716_serial_rdid(uint16_t port)
>> +{
>> +	uint8_t busy, data0, data1, data2;
>> +	do {
>> +		busy = inb(0x0820) & 0x80;
>> +	} while (busy);
>> +	/* RDID */
>> +	outb(0x9f, 0x0820 + 1);
> 
> Should this be a #define?

No. Old code version. Will send new patch.

> 
> 
>> +	/* Start IO, 33MHz, 3 input bytes, 0 output bytes*/
>> +	outb((0x5<<4)|(0x3<<2)|(0x0), 0x0820);
>> +	do {
>> +		busy = inb(0x0820) & 0x80;
>> +	} while (busy);
>> +	data0 = inb(0x0820 + 5);
>> +	data1 = inb(0x0820 + 6);
>> +	data2 = inb(0x0820 + 7);
> 
> #defines? SPI_IDATA0 etc.
> 
> 
>> +	printf("RDID returned %02x %02x %02x\n", data0, data1, data2);
> 
>> +	return;
> 
> Not needed, the function returns void.

Explicit return statements are nice and this will return a value in the
future.

> 
>> +static int gam57sli_probe_serial_flash(const char *name)
> 
> Let's use the full/correct name for the board. In this case:
> ga_m57sli_s4_probe_serial_flash or gam57slis4_probe_serial_flash
> (I think the first version is more readable)

This works for all boards with SPI hanging off IT8716F, so more generic
name needed.

>> +{
>> +	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);
>> +	return 0;
> 
> If the function always returns the same value, maybe we should make
> it return void?

Function must return int unless you change all other probe functions.

>>  struct board_pciid_enable board_pciid_enables[] = {
>> +	{0x10de, 0x0360, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000,
>> +	 "gigabyte", "gam57sli", "Gigabyte GA-M57SLI", gam57sli_probe_serial_flash},
> 
> "gigabyte", "ga-m57sli-s4", "GIGABYTE GA-M57SLI-S4", ga_m57sli_s4_probe_serial_flash},
> 
> (or similar, depending on the above function name)
> 
> GIGABYTE is all-caps as they use that form on their website.

ok, done.

Updated patch attached. This one was unfortunately an old version of the
code.

Carl-Daniel
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: linuxbios_flashrom_ite_spi2.diff
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20070925/d030dc8d/attachment.ksh>