Re: [flashrom] [PATCH] Add support for AMD Bolton chipset

[Stefan Tauner]

Thanks for your patch!
Although this is a small patch, I have to nag about some stuff...

On Mon, 09 Jun 2014 14:48:59 -0600
Martin Roth  wrote:
> Index: sb600spi.c
>
> --- sb600spi.c	(revision 1818)
> +++ sb600spi.c	(working copy)
> @@ -137,7 +138,10 @@
>    	if (rev >= 0x11 && rev <= 0x15) {
>    		amd_gen = CHIPSET_HUDSON234;
>    		msg_pdbg("Hudson-2/3/4 detected.\n");
> } else if (rev >= 0x39 && rev <= 0x3A) {
> }  else if (rev == 0x16) {

^^
Move one space from here...
> 	amd_gen = CHIPSET_BOLTON;
> 	msg_pdbg("Bolton detected.\n");
> }else if (rev >= 0x39 && rev <= 0x3A) {

to here.        ^^
> 	amd_gen = CHIPSET_YANGTZE;
> 	msg_pdbg("Yangtze detected.\n");
> } else {
> @@ -453,6 +457,29 @@
>    	msg_pdbg("FastSpeedNew is %s\n", spispeeds[(tmp >> 8) & 0xf].name);
>    	msg_pdbg("AltSpeedNew is %s\n", spispeeds[(tmp >> 4) & 0xf].name);
>    	msg_pdbg("TpmSpeedNew is %s\n", spispeeds[(tmp >> 0) & 0xf].name);
> } else if (amd_gen == CHIPSET_BOLTON) {
> static const char *spireadmodes[] = {
> 	"Normal (up to 33 MHz)", /* 0 */
> 	"Reserved",		 /* 1 */
> 	"Dual IO (1-1-2)",	 /* 2 */
> 	"Quad IO (1-1-4)",	 /* 3 */
> 	"Dual IO (1-2-2)",	 /* 4 */
> 	"Quad IO (1-4-4)",	 /* 5 */
> 	"Normal (up to 66 MHz)", /* 6 */
> 	"Fast Read",		 /* 7 (I'm Assuming...  This value isn't in the bolton RRG) */
> };
> tmp = mmio_readl(sb600_spibar + 0x00);
> uint8_t read_mode = ((tmp >> 28) & 0x6) | ((tmp >> 18) & 0x1);
> msg_pdbg("SpiReadMode=%s (%i)\n", spireadmodes[read_mode], read_mode);
> if (read_mode != 6) {
> 	read_mode = 6; /* Default to "Normal (up to 66 MHz)" */
> 	if (set_mode(dev, read_mode) != 0) {
> 		msg_perr("Setting read mode to \"%s\" failed.\n", spireadmodes[read_mode]);
> 		return 1;
> 	}
> 	msg_pdbg("Setting read mode to \"%s\" succeeded.\n", spireadmodes[read_mode]);
> }

Apart from the FastSpeedNew etc. bits yangtze's and bolton's branches
are equal here. If FastSpeedNew etc. is really only in yangtze (quite
probable because that's an SPI 100 thing), then why not guard that with
an additional "amd_gen >= CHIPSET_YANGTZE" but combine the rest with
bolton by using(amd_gen >= CHIPSET_BOLTON)?
> } else {
>    	if (amd_gen >= CHIPSET_SB89XX && amd_gen <= CHIPSET_HUDSON234) {
>    		bool fast_read = (mmio_readl(sb600_spibar + 0x00) >> 18) & 0x1;