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

[Jordan Crouse]

One of the good things about SimNow is that it does a good job of 
simulating "bad hardware" and can flush out bad infinite loops, such
as the one fixed in this patch.

Jordan

[ron minnich]

Acked for v2:
Acked-by: Ronald G. Minnich

This applied cleanly to v3 and built just fine.

Committed revision 904. (v3)

ron

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

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

[Peter Stuge]

Uwe Hermann wrote:
> Make the hardcoded "1" into a nice, descriptive #define?

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

I don't know. While they are good for documentation purposes they
cause a sometimes annyoing indirection when reading the code..

//Peter

coreboot: Don't loop forever waiting for HDA codecs

[Jordan Crouse]

On 07/10/08 02:45 +0200, Peter Stuge wrote:
> Uwe Hermann wrote:
> > Make the hardcoded "1" into a nice, descriptive #define?
>
> ..
> > Make the hardcoded "2" and "3" into nice, descriptive #defines?
> > Also the 0x68 register could be a #define.
>
> I don't know. While they are good for documentation purposes they
> cause a sometimes annyoing indirection when reading the code..

I'll add them - one one hand, nobody is going to learn how to 
program a HDA codec from reading the firwmare code - the firmware
developers need to know that you read 0x68 and wait for bit 1 to clear
and thats that.  On the other hand, I admit that I had to look up
the values here to see what is going on, so a little in code
documentation by way of #define may be nicer to the next guy.

I'll add the defines in the functions I hacked - I don't know nearly
enough about how it all works to document the whole file.

Jordan