Re: [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

Acked-by: Uwe Hermann

(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.