[coreboot] [PATCH] coreboot: Don't loop forever waiting for HDA codecs

[Uwe Hermann]

On Mon, Oct 06, 2008 at 06:11:15PM -0600, Jordan Crouse wrote:
> [PATCH] coreboot:  Don't loop forever waiting for HDA codecs
> 
> We shouldn't assume the presence of a working HDA codec, so put in
> a reasonable timeout of 50usecs (timeout value borrowed from the kernel).
> This makes SimNow work, since apparently though the codec is 
> present in Simnow, it is non functional.
> 
> Signed-off-by: Jordan Crouse <jordan.crouse at amd.com>

Acked-by: Uwe Hermann <uwe at hermann-uwe.de>

(assuming this is build-tested). But see also below...


> Index: coreboot-v2/src/southbridge/amd/sb600/sb600_hda.c
> ===================================================================
> --- coreboot-v2.orig/src/southbridge/amd/sb600/sb600_hda.c	2008-10-06 18:00:06.000000000 -0600
> +++ coreboot-v2/src/southbridge/amd/sb600/sb600_hda.c	2008-10-06 18:10:32.000000000 -0600
> @@ -160,6 +160,32 @@
>  	return sizeof(cim_verb_data) / sizeof(u32);
>  }
>  

I'd add some (Doxygen) comment, at least saying "Don't loop forever
waiting for HDA codecs" or something more elaborate...


> +static int wait_for_ready(u8 *base)
> +{
> +	int timeout = 50;

A comment here why it's exactly 50 would be nice.


> +	while(timeout--) {
             ^
           space

> +		u32 dword=readl(base + 0x68);
                         ^^
                        spaces


> +		if (!(dword & 1))

Make the hardcoded "1" into a nice, descriptive #define?


> +			return 0;
> +		udelay(1);
> +	}
> +
> +	return -1;
> +}
> +
> +static int wait_for_valid(u8 *base)
> +{
> +	int timeout = 50;
> +	while(timeout--) {
             ^
           space

> +		u32 dword = readl(base + 0x68);
> +		if ((dword & 3) == 2)

Make the hardcoded "2" and "3" into nice, descriptive #defines?
Also the 0x68 register could be a #define.


> +			return 0;
> +		udelay(1);
> +	}
> +
> +	return 1;
> +}
> +
>  static void codec_init(u8 * base, int addr)
>  {
>  	u32 dword;
> @@ -168,16 +194,14 @@
>  	int i;
>  
>  	/* 1 */
> -	do {
> -		dword = readl(base + 0x68);
> -	} while (dword & 1);
> +	if (wait_for_ready(base) == -1)
> +		return;
>  
>  	dword = (addr << 28) | 0x000f0000;
>  	writel(dword, base + 0x60);
>  
> -	do {
> -		dword = readl(base + 0x68);
> -	} while ((dword & 3) != 2);
> +	if (wait_for_valid(base) == -1)
> +		return;
>  
>  	dword = readl(base + 0x64);
>  
> @@ -193,15 +217,13 @@
>  	printk_debug("verb_size: %d\n", verb_size);
>  	/* 3 */
>  	for (i = 0; i < verb_size; i++) {
> -		do {
> -			dword = readl(base + 0x68);
> -		} while (dword & 1);
> +		if (wait_for_ready(base) == -1)
> +			return;
>  
>  		writel(verb[i], base + 0x60);
>  
> -		do {
> -			dword = readl(base + 0x68);
> -		} while ((dword & 3) != 2);
> +		if (wait_for_valid(base) == -1)
> +			return;
>  	}
>  	printk_debug("verb loaded!\n");
>  }


Uwe.
-- 
http://www.hermann-uwe.de  | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org