[PATCH] sb600: don't load verb for codec

[Bao, Zheng]

Don't load verb in BIOS stage. It is not BIOS's responsibility.
And we can not have verb for every single codec.

Signed-off-by: Zheng Bao

Index: src/southbridge/amd/sb600/sb600_hda.c
===================================================================
--- src/southbridge/amd/sb600/sb600_hda.c	(revision 5184)
+++ src/southbridge/amd/sb600/sb600_hda.c	(working copy)
@@ -90,88 +90,10 @@
    return 0;
 }

-static u32 cim_verb_data[] = {
-	0x01471c10,
-	0x01471d40,
-	0x01471e01,
-	0x01471f01,
-/* 1 */
-	0x01571c12,
-	0x01571d10,
-	0x01571e01,
-	0x01571f01,
-/* 2 */
-	0x01671c11,
-	0x01671d60,
-	0x01671e01,
-	0x01671f01,
-/* 3 */
-	0x01771c14,
-	0x01771d20,
-	0x01771e01,
-	0x01771f01,
-/* 4 */
-	0x01871c30,
-	0x01871d90,
-	0x01871ea1,
-	0x01871f01,
-/* 5 */
-	0x01971cf0,
-	0x01971d11,
-	0x01971e11,
-	0x01971f41,
-/* 6 */
-	0x01a71c80,
-	0x01a71d30,
-	0x01a71e81,
-	0x01a71f01,
-/* 7 */
-	0x01b71cf0,
-	0x01b71d11,
-	0x01b71e11,
-	0x01b71f41,
-/* 8 */
-	0x01c71cf0,
-	0x01c71d11,
-	0x01c71e11,
-	0x01c71f41,
-/* 9 */
-	0x01d71cf0,
-	0x01d71d11,
-	0x01d71e11,
-	0x01d71f41,
-/* 10 */
-	0x01e71c50,
-	0x01e71d11,
-	0x01e71e44,
-	0x01e71f01,
-/* 11 */
-	0x01f71c60,
-	0x01f71d61,
-	0x01f71ec4,
-	0x01f71f01,
-};
-static u32 find_verb(u32 viddid, u32 ** verb)
-{
-	device_t azalia_dev = dev_find_slot(0, PCI_DEVFN(0x14, 2));
-	struct southbridge_amd_sb600_config *cfg =
-	    (struct southbridge_amd_sb600_config
*)azalia_dev->chip_info;
-	printk_debug("Dev=%s\n", dev_path(azalia_dev));
-	printk_debug("Default viddid=%x\n", cfg->hda_viddid);
-	printk_debug("Reading viddid=%x\n", viddid);
-	if (!cfg)
-		return 0;
-	if (viddid != cfg->hda_viddid)
-		return 0;
-	*verb = (u32 *) cim_verb_data;
-	return sizeof(cim_verb_data) / sizeof(u32);
-}
-
 /**
  *  Wait 50usec for for the codec to indicate it is ready
  *  no response would imply that the codec is non-operative
  */
-
 static int wait_for_ready(u8 *base)
 {
    /* Use a 50 usec timeout - the Linux kernel uses the
@@ -194,7 +116,6 @@
  *  the previous command.  No response would imply that the code
  *  is non-operative
  */
-
 static int wait_for_valid(u8 *base)
 {
    /* Use a 50 usec timeout - the Linux kernel uses the
@@ -215,9 +136,6 @@
 static void codec_init(u8 * base, int addr)
 {
    u32 dword;
-	u32 *verb;
-	u32 verb_size;
-	int i;

/* 1 */
    if (wait_for_ready(base) == -1)
@@ -232,26 +150,7 @@
    dword = read32(base + 0x64);

/* 2 */
-	printk_debug("codec viddid: %08x\n", dword);
-	verb_size = find_verb(dword, &verb);
-
-	if (!verb_size) {
-		printk_debug("No verb!\n");
-		return;
-	}
-
-	printk_debug("verb_size: %d\n", verb_size);
-	/* 3 */
-	for (i = 0; i < verb_size; i++) {
-		if (wait_for_ready(base) == -1)
-			return;
-
-		write32(base + 0x60, verb[i]);
-
-		if (wait_for_valid(base) == -1)
-			return;
-	}
-	printk_debug("verb loaded!\n");
+	printk_debug("%x(th) codec viddid: %08x\n", addr, dword);
 }

static void codecs_init(u8 * base, u32 codec_mask)

[Stefan Reinauer]

On 3/5/10 10:29 AM, Bao, Zheng wrote:
> Don't load verb in BIOS stage. It is not BIOS's responsibility.

Who is responsible for that, then?

On i945 I definitely don't get sound on most systems if I don't load the
verb in coreboot. I also don't think the Linux drivers do it.
> And we can not have verb for every single codec.

Have a look at the i82801gx southbridge; I think it was based on the
sb600 code, but I changed it so I can have a per mainboard verb table.
In mainboard_enable() in mainboard.c, call verb_setup() which does:

#include "hda_verb.h"

static void verb_setup(void)
{
        cim_verb_data = mainboard_cim_verb_data;
        cim_verb_data_size = sizeof(mainboard_cim_verb_data);
}

and hda_verb.c looks like this:

static u32 mainboard_cim_verb_data[] = {
        /* coreboot specific header */
        0x10ec0262,     // Codec Vendor ID / Device ID
        0x10714700,     // Subsystem ID
        0x0000000d,     // Number of jacks

/* HDA Codec Subsystem ID Verb Table: 0x10ec0000 */
        0x00172000,
        0x00172100,
        0x001722EC,
        0x00172310,

/* Pin Widget Verb Table */

/* Pin Complex (NID 0x12) */
        0x01271CF0,
        0x01271D11,
        0x01271E11,
...
};

extern u32 * cim_verb_data;
extern u32 cim_verb_data_size;

[Andrew Goodbody]

Stefan Reinauer wrote:
> On 3/5/10 10:29 AM, Bao, Zheng wrote:
> > Don't load verb in BIOS stage. It is not BIOS's responsibility.
>
> Who is responsible for that, then?

Those verbs are motherboard specific as they describe the actual 
external connections so they should not be in southbridge code. They 
cannot be inferred by the driver so they absolutely should be done by 
coreboot. So they should be moved to motherboard specific code not 
dropped entirely.

Andrew

[Carl-Daniel Hailfinger]

On 05.03.2010 11:13, Stefan Reinauer wrote:
> On 3/5/10 10:29 AM, Bao, Zheng wrote:
> > Don't load verb in BIOS stage. It is not BIOS's responsibility.
>
> Who is responsible for that, then?
>
> On i945 I definitely don't get sound on most systems if I don't load the
> verb in coreboot. I also don't think the Linux drivers do it.
> > And we can not have verb for every single codec.
>
> Have a look at the i82801gx southbridge; I think it was based on the
> sb600 code, but I changed it so I can have a per mainboard verb table.

I think we should try to mirror the behaviour of the vendor BIOS. The
Linux kernel source has some interesting info about HD Audio:
http://www.mjmwired.net/kernel/Documentation/sound/alsa/HD-Audio.txt

Regards,
Carl-Daniel

[PATCH] sb600: new way to load initial verb

[Bao, Zheng]

The original way to load initial verb is quite inflexible.
The format of the command is:
CAd 31:28 - codec address
NID 27:20 - Nid
Verb 19:0 - Verb data
Each Nid has 4 byte data to write. It has to write one at a time.
    0x01471c10,      //1st byte 10
    0x01471d40,      //2nd byte 40
    0x01471e01,      //3rd byte 01
    0x01471f01,      //4th byte 01

The new code in sb600_hda.c and the code structure are quite
preliminary.  When it is almost done and can satisfy everybody, We can
define the pin configuration code at mainboard.
 #define CODEC_PIN_CONFIG_FILE "codec/alc_882.h"
or
 config CODEC_PIN_CONFIG_FILE
    string
    default "codec/alc_882.h"
    depends on BOARD_AMD_DBM690T

Now I am wondering if it is legal provide the intial pin configuration
code in coreboot. If it is legal, how can we get it? The 882 code is
got from CIM.

Signed-off-by: Zheng Bao

Index: src/codec/alc_882.h
===================================================================
--- src/codec/alc_882.h	(revision 0)
+++ src/codec/alc_882.h	(revision 0)
@@ -0,0 +1,12 @@
+	{0x14, 0x01014010},
+	{0x15, 0x01011012},
+	{0x16, 0x01016011},
+	{0x17, 0x01012014},
+	{0x18, 0x01A19030},
+	{0x19, 0x411111F0},
+	{0x1a, 0x01813080},
+	{0x1b, 0x411111F0},
+	{0x1C, 0x411111F0},
+	{0x1d, 0x411111F0},
+	{0x1e, 0x01441150},
+	{0x1f, 0x01C46160},
Index: src/southbridge/amd/sb600/sb600_hda.c
===================================================================
--- src/southbridge/amd/sb600/sb600_hda.c	(revision 5195)
+++ src/southbridge/amd/sb600/sb600_hda.c	(working copy)
@@ -90,68 +90,19 @@
    return 0;
 }

-static u32 cim_verb_data[] = {
-	0x01471c10,
-	0x01471d40,
-	0x01471e01,
-	0x01471f01,
-/* 1 */
-	0x01571c12,
-	0x01571d10,
-	0x01571e01,
-	0x01571f01,
-/* 2 */
-	0x01671c11,
-	0x01671d60,
-	0x01671e01,
-	0x01671f01,
-/* 3 */
-	0x01771c14,
-	0x01771d20,
-	0x01771e01,
-	0x01771f01,
-/* 4 */
-	0x01871c30,
-	0x01871d90,
-	0x01871ea1,
-	0x01871f01,
-/* 5 */
-	0x01971cf0,
-	0x01971d11,
-	0x01971e11,
-	0x01971f41,
-/* 6 */
-	0x01a71c80,
-	0x01a71d30,
-	0x01a71e81,
-	0x01a71f01,
-/* 7 */
-	0x01b71cf0,
-	0x01b71d11,
-	0x01b71e11,
-	0x01b71f41,
-/* 8 */
-	0x01c71cf0,
-	0x01c71d11,
-	0x01c71e11,
-	0x01c71f41,
-/* 9 */
-	0x01d71cf0,
-	0x01d71d11,
-	0x01d71e11,
-	0x01d71f41,
-/* 10 */
-	0x01e71c50,
-	0x01e71d11,
-	0x01e71e44,
-	0x01e71f01,
-/* 11 */
-	0x01f71c60,
-	0x01f71d61,
-	0x01f71ec4,
-	0x01f71f01,
+typedef struct _pin_config_codec_entry {
+	u8	nid;
+	u32	pin_config;
+} pin_config_codec_entry;
+
+static pin_config_codec_entry pin_config_file[] = {
+#ifdef	CODEC_PIN_CONFIG_FILE
+	#include CODEC_PIN_CONFIG_FILE
+#endif
+	{-1, -1}
 };
-static u32 find_verb(u32 viddid, u32 ** verb)
+
+static void find_verb(u32 viddid, pin_config_codec_entry ** verb)
 {
    device_t azalia_dev = dev_find_slot(0, PCI_DEVFN(0x14, 2));
    struct southbridge_amd_sb600_config *cfg =
@@ -159,12 +110,13 @@
    printk_debug("Dev=%s\n", dev_path(azalia_dev));
    printk_debug("Default viddid=%x\n", cfg->hda_viddid);
    printk_debug("Reading viddid=%x\n", viddid);
+
+	*verb = NULL;
    if (!cfg)
-		return 0;
+		return;
    if (viddid != cfg->hda_viddid)
-		return 0;
-	*verb = (u32 *) cim_verb_data;
-	return sizeof(cim_verb_data) / sizeof(u32);
+		return;
+	*verb = pin_config_file;
 }

/**
@@ -214,9 +166,8 @@

static void codec_init(u8 * base, int addr)
 {
-	u32 dword;
-	u32 *verb;
-	u32 verb_size;
+	u32 dword, dword1, pin_config;
+	pin_config_codec_entry *verb=NULL;
    int i;

/* 1 */
@@ -233,23 +184,32 @@

/* 2 */
    printk_debug("codec viddid: %08x\n", dword);
-	verb_size = find_verb(dword, &verb);
+	find_verb(dword, &verb);

-	if (!verb_size) {
+	if (verb == NULL) {
    	printk_debug("No verb!\n");
    	return;
    }

-	printk_debug("verb_size: %d\n", verb_size);
    /* 3 */
-	for (i = 0; i < verb_size; i++) {
-		if (wait_for_ready(base) == -1)
-			return;
+	for (verb; verb->nid != 0xFF; verb++) {
+		dword = addr << 28 | verb->nid << 20 | 7 << 16;
+		pin_config = verb->pin_config;

-		write32(base + 0x60, verb[i]);
+		for (i = 4; i > 0; i--, pin_config >>= 8) {
+			if (wait_for_ready(base) == -1)
+				return;

-		if (wait_for_valid(base) == -1)
-			return;
+			if (verb->nid != 1)
+				dword1 = dword | ((0x20 - i) & 0xFF) <<
8;
+			else
+				dword1 = dword | ((0x24 - i) & 0xFF) <<
8;
+			dword1 |= (pin_config & 0xFF);
+			write32(base + 0x60, dword1);
+
+			if (wait_for_valid(base) == -1)
+				return;
+		}
    }
    printk_debug("verb loaded!\n");
 }

-----Original Message-----
From: [e-mail]
[mailto:[e-mail]] On Behalf Of Carl-Daniel
Hailfinger
Sent: Friday, March 05, 2010 10:03 PM
To: Stefan Reinauer
Cc: [e-mail]
Subject: Re: [coreboot] [PATCH] sb600: don't load verb for codec

On 05.03.2010 11:13, Stefan Reinauer wrote:
> On 3/5/10 10:29 AM, Bao, Zheng wrote:
> > Don't load verb in BIOS stage. It is not BIOS's responsibility.
>
> Who is responsible for that, then?
>
> On i945 I definitely don't get sound on most systems if I don't load

the
> verb in coreboot. I also don't think the Linux drivers do it.
> > And we can not have verb for every single codec.
>
> Have a look at the i82801gx southbridge; I think it was based on the
> sb600 code, but I changed it so I can have a per mainboard verb table.

I think we should try to mirror the behaviour of the vendor BIOS. The
Linux kernel source has some interesting info about HD Audio:
http://www.mjmwired.net/kernel/Documentation/sound/alsa/HD-Audio.txt

Regards,
Carl-Daniel

[PATCH] sb600: new way to load initial verb

[Carl-Daniel Hailfinger]

On 08.03.2010 09:20, Bao, Zheng wrote:
> The original way to load initial verb is quite inflexible.
> The format of the command is:
> CAd 31:28 - codec address
> NID 27:20 - Nid
> Verb 19:0 - Verb data
> Each Nid has 4 byte data to write. It has to write one at a time.
>    0x01471c10,      //1st byte 10
>    0x01471d40,      //2nd byte 40
>    0x01471e01,      //3rd byte 01
>    0x01471f01,      //4th byte 01
>
> The new code in sb600_hda.c and the code structure are quite
> preliminary.  When it is almost done and can satisfy everybody, We can
> define the pin configuration code at mainboard.

It would be cool if all chipsets could use the new verb implementation.
> #define CODEC_PIN_CONFIG_FILE "codec/alc_882.h"
> or
>  config CODEC_PIN_CONFIG_FILE
>    string
>    default "codec/alc_882.h"
>    depends on BOARD_AMD_DBM690T

Or we define that file as separate uncompressed CBFS file.
> Now I am wondering if it is legal provide the intial pin configuration
> code in coreboot. If it is legal, how can we get it? The 882 code is
> got from CIM.

Sorry, what is CIM?
In theory, the pin configuration is mainboard specific. If that is true,
the pin configuration is similar to IRQ routing: There is usually only
one correct and working solution. So it is possible that the mainboard
vendor and the codec vendor own parts of it. Not sure. We could tell
users to dump verbs from their mainboards (is that possible?) and add
the resulting file to CBFS.
> Signed-off-by: Zheng Bao
> Index: src/codec/alc_882.h
>
> --- src/codec/alc_882.h	(revision 0)
> +++ src/codec/alc_882.h	(revision 0)
> @@ -0,0 +1,12 @@

struct _pin_config_codec_entry pin_config_file[] = {
> {0x14, 0x01014010},
> {0x15, 0x01011012},
> {0x16, 0x01016011},
> {0x17, 0x01012014},
> {0x18, 0x01A19030},
> {0x19, 0x411111F0},
> {0x1a, 0x01813080},
> {0x1b, 0x411111F0},
> {0x1C, 0x411111F0},
> {0x1d, 0x411111F0},
> {0x1e, 0x01441150},
> {0x1f, 0x01C46160},

{-1, -1}
};
> Index: src/southbridge/amd/sb600/sb600_hda.c
>
> --- src/southbridge/amd/sb600/sb600_hda.c	(revision 5195)
> +++ src/southbridge/amd/sb600/sb600_hda.c	(working copy)
> @@ -90,68 +90,19 @@
>    return 0;
>  }
>
> -static u32 cim_verb_data[] = {
> 0x01471c10,
> 0x01471d40,
> 0x01471e01,
> 0x01471f01,
>
> -/* 1 */
> 0x01571c12,
> 0x01571d10,
> 0x01571e01,
> 0x01571f01,
>
> -/* 2 */
> 0x01671c11,
> 0x01671d60,
> 0x01671e01,
> 0x01671f01,
>
> -/* 3 */
> 0x01771c14,
> 0x01771d20,
> 0x01771e01,
> 0x01771f01,
>
> -/* 4 */
> 0x01871c30,
> 0x01871d90,
> 0x01871ea1,
> 0x01871f01,
>
> -/* 5 */
> 0x01971cf0,
> 0x01971d11,
> 0x01971e11,
> 0x01971f41,
>
> -/* 6 */
> 0x01a71c80,
> 0x01a71d30,
> 0x01a71e81,
> 0x01a71f01,
>
> -/* 7 */
> 0x01b71cf0,
> 0x01b71d11,
> 0x01b71e11,
> 0x01b71f41,
>
> -/* 8 */
> 0x01c71cf0,
> 0x01c71d11,
> 0x01c71e11,
> 0x01c71f41,
>
> -/* 9 */
> 0x01d71cf0,
> 0x01d71d11,
> 0x01d71e11,
> 0x01d71f41,
>
> -/* 10 */
> 0x01e71c50,
> 0x01e71d11,
> 0x01e71e44,
> 0x01e71f01,
>
> -/* 11 */
> 0x01f71c60,
> 0x01f71d61,
> 0x01f71ec4,
> 0x01f71f01,
>
> +typedef struct _pin_config_codec_entry {

Can you please kill the typedef and use "struct _pin_config_codec_entry"
instead?
> u8	nid;
> u32	pin_config;
>
> +} pin_config_codec_entry;
>
> +static pin_config_codec_entry pin_config_file[] = {
> +#ifdef	CODEC_PIN_CONFIG_FILE
> #include CODEC_PIN_CONFIG_FILE

Including C source makes code difficult to read. Can you move the
complete struct to alc_882.h?

Hm. Could we simply store the whole verb stuff uncompressed inside CBFS?
> +#endif
> {-1, -1}
>
> };
> -static u32 find_verb(u32 viddid, u32 ** verb)
>
> +static void find_verb(u32 viddid, pin_config_codec_entry ** verb)
>  {
>    device_t azalia_dev = dev_find_slot(0, PCI_DEVFN(0x14, 2));
>    struct southbridge_amd_sb600_config *cfg =
> @@ -159,12 +110,13 @@
>    printk_debug("Dev=%s\n", dev_path(azalia_dev));
>    printk_debug("Default viddid=%x\n", cfg->hda_viddid);
>    printk_debug("Reading viddid=%x\n", viddid);
> *verb = NULL;
> if (!cfg)
> return 0;
> return;
> if (viddid != cfg->hda_viddid)
> return 0;
> *verb = (u32 *) cim_verb_data;
> return sizeof(cim_verb_data) / sizeof(u32);
> return;
> *verb = pin_config_file;
>
> }
>
> /**
> @@ -214,9 +166,8 @@
>
> static void codec_init(u8 * base, int addr)
>  {
> u32 dword;
> u32 *verb;
> u32 verb_size;
>
> u32 dword, dword1, pin_config;
>
> pin_config_codec_entry *verb=NULL;
> int i;
>
> /* 1 */
>
> @@ -233,23 +184,32 @@
>
> /* 2 */
>    printk_debug("codec viddid: %08x\n", dword);
> verb_size = find_verb(dword, &verb);
> find_verb(dword, &verb);
> if (!verb_size) {
> if (verb == NULL) {
> printk_debug("No verb!\n");
> return;
> }
> printk_debug("verb_size: %d\n", verb_size);
> /* 3 */
> for (i = 0; i < verb_size; i++) {
> if (wait_for_ready(base) == -1)
> 	return;
> for (verb; verb->nid != 0xFF; verb++) {
> dword = addr << 28 | verb->nid << 20 | 7 << 16;
> pin_config = verb->pin_config;
>
> write32(base + 0x60, verb[i]);
> for (i = 4; i > 0; i--, pin_config >>= 8) {
> 	if (wait_for_ready(base) == -1)
> 		return;
>
> if (wait_for_valid(base) == -1)
> 	return;
> 	if (verb->nid != 1)
> 		dword1 = dword | ((0x20 - i) & 0xFF) << 8;
> 	else
> 		dword1 = dword | ((0x24 - i) & 0xFF) << 8;
> 	dword1 |= (pin_config & 0xFF);
> 	write32(base + 0x60, dword1);
> 	if (wait_for_valid(base) == -1)
> 		return;
> }
> }
> printk_debug("verb loaded!\n");
>
> }

Looks good.

Regards,
Carl-Daniel

[PATCH] sb600: new way to load initial verb

[Bao, Zheng]

> -----Original Message-----
> From: Carl-Daniel Hailfinger

[mailto:[e-mail]]
> Sent: Monday, March 08, 2010 10:15 PM
> To: Bao, Zheng
> Cc: Stefan Reinauer; [e-mail]
> Subject: Re: [coreboot] [PATCH] sb600: new way to load initial verb
>
> On 08.03.2010 09:20, Bao, Zheng wrote:
> > The original way to load initial verb is quite inflexible.
> > The format of the command is:
> > CAd 31:28 - codec address
> > NID 27:20 - Nid
> > Verb 19:0 - Verb data
> > Each Nid has 4 byte data to write. It has to write one at a time.
> >    0x01471c10,      //1st byte 10
> >    0x01471d40,      //2nd byte 40
> >    0x01471e01,      //3rd byte 01
> >    0x01471f01,      //4th byte 01
> >
> > The new code in sb600_hda.c and the code structure are quite
> > preliminary.  When it is almost done and can satisfy everybody, We

can
> > define the pin configuration code at mainboard.
>
> It would be cool if all chipsets could use the new verb

implementation.
> > #define CODEC_PIN_CONFIG_FILE "codec/alc_882.h"
> > or
> >  config CODEC_PIN_CONFIG_FILE
> >    string
> >    default "codec/alc_882.h"
> >    depends on BOARD_AMD_DBM690T
>
> Or we define that file as separate uncompressed CBFS file.
> > Now I am wondering if it is legal provide the intial pin

configuration
> > code in coreboot. If it is legal, how can we get it? The 882 code is
> > got from CIM.
>
> Sorry, what is CIM?
> In theory, the pin configuration is mainboard specific. If that is

true,
> the pin configuration is similar to IRQ routing: There is usually only
> one correct and working solution. So it is possible that the mainboard
> vendor and the codec vendor own parts of it. Not sure. We could tell
> users to dump verbs from their mainboards (is that possible?) and add
> the resulting file to CBFS.

It is mainboard specific. Each codec also has its own code. The
mainboard code is based on the codec code, I think. Dumping verbs is
possible. It needs a lot of work from the ground up. There are some
similar tools like http://helllabs.org/codecgraph/. Integrating them to
coreboot/util needs permission, doesn't it?
> > Signed-off-by: Zheng Bao
> > Index: src/codec/alc_882.h
> >
> > --- src/codec/alc_882.h	(revision 0)
> > +++ src/codec/alc_882.h	(revision 0)
> > @@ -0,0 +1,12 @@
>
> struct _pin_config_codec_entry pin_config_file[] = {
> > {0x14, 0x01014010},
> > {0x15, 0x01011012},
> > {0x16, 0x01016011},
> > {0x17, 0x01012014},
> > {0x18, 0x01A19030},
> > {0x19, 0x411111F0},
> > {0x1a, 0x01813080},
> > {0x1b, 0x411111F0},
> > {0x1C, 0x411111F0},
> > {0x1d, 0x411111F0},
> > {0x1e, 0x01441150},
> > {0x1f, 0x01C46160},
>
> {-1, -1}
> };
> > Index: src/southbridge/amd/sb600/sb600_hda.c
> >
> > --- src/southbridge/amd/sb600/sb600_hda.c	(revision 5195)
> > +++ src/southbridge/amd/sb600/sb600_hda.c	(working copy)
> > @@ -90,68 +90,19 @@
> >    return 0;
> >  }
> >
> > -static u32 cim_verb_data[] = {
> > 0x01471c10,
> > 0x01471d40,
> > 0x01471e01,
> > 0x01471f01,
> >
> > -/* 1 */
> > 0x01571c12,
> > 0x01571d10,
> > 0x01571e01,
> > 0x01571f01,
> >
> > -/* 2 */
> > 0x01671c11,
> > 0x01671d60,
> > 0x01671e01,
> > 0x01671f01,
> >
> > -/* 3 */
> > 0x01771c14,
> > 0x01771d20,
> > 0x01771e01,
> > 0x01771f01,
> >
> > -/* 4 */
> > 0x01871c30,
> > 0x01871d90,
> > 0x01871ea1,
> > 0x01871f01,
> >
> > -/* 5 */
> > 0x01971cf0,
> > 0x01971d11,
> > 0x01971e11,
> > 0x01971f41,
> >
> > -/* 6 */
> > 0x01a71c80,
> > 0x01a71d30,
> > 0x01a71e81,
> > 0x01a71f01,
> >
> > -/* 7 */
> > 0x01b71cf0,
> > 0x01b71d11,
> > 0x01b71e11,
> > 0x01b71f41,
> >
> > -/* 8 */
> > 0x01c71cf0,
> > 0x01c71d11,
> > 0x01c71e11,
> > 0x01c71f41,
> >
> > -/* 9 */
> > 0x01d71cf0,
> > 0x01d71d11,
> > 0x01d71e11,
> > 0x01d71f41,
> >
> > -/* 10 */
> > 0x01e71c50,
> > 0x01e71d11,
> > 0x01e71e44,
> > 0x01e71f01,
> >
> > -/* 11 */
> > 0x01f71c60,
> > 0x01f71d61,
> > 0x01f71ec4,
> > 0x01f71f01,
> >
> > +typedef struct _pin_config_codec_entry {
>
> Can you please kill the typedef and use "struct

_pin_config_codec_entry"
> instead?
> > u8	nid;
> > u32	pin_config;
> >
> > +} pin_config_codec_entry;
> >
> > +static pin_config_codec_entry pin_config_file[] = {
> > +#ifdef	CODEC_PIN_CONFIG_FILE
> > #include CODEC_PIN_CONFIG_FILE
>
> Including C source makes code difficult to read. Can you move the
> complete struct to alc_882.h?
>
> Hm. Could we simply store the whole verb stuff uncompressed inside

CBFS?
>

I think the purpose of the CBFS is to integrate some binary files which
are built at other place. The verb data we got is in text mode. We need
to build it into binary and store it into CBFS. Is it wasting? Why don't
we do it as fam10 patch code?
> > +#endif
> > {-1, -1}
> >
> > };
> > -static u32 find_verb(u32 viddid, u32 ** verb)
> >
> > +static void find_verb(u32 viddid, pin_config_codec_entry ** verb)
> >  {
> >    device_t azalia_dev = dev_find_slot(0, PCI_DEVFN(0x14, 2));
> >    struct southbridge_amd_sb600_config *cfg =
> > @@ -159,12 +110,13 @@
> >    printk_debug("Dev=%s\n", dev_path(azalia_dev));
> >    printk_debug("Default viddid=%x\n", cfg->hda_viddid);
> >    printk_debug("Reading viddid=%x\n", viddid);
> > *verb = NULL;
> > if (!cfg)
> > return 0;
> > return;
> > if (viddid != cfg->hda_viddid)
> > return 0;
> > *verb = (u32 *) cim_verb_data;
> > return sizeof(cim_verb_data) / sizeof(u32);
> > return;
> > *verb = pin_config_file;
> >
> > }
> >
> > /**
> > @@ -214,9 +166,8 @@
> >
> > static void codec_init(u8 * base, int addr)
> >  {
> > u32 dword;
> > u32 *verb;
> > u32 verb_size;
> >
> > u32 dword, dword1, pin_config;
> >
> > pin_config_codec_entry *verb=NULL;
> > int i;
> >
> > /* 1 */
> >
> > @@ -233,23 +184,32 @@
> >
> > /* 2 */
> >    printk_debug("codec viddid: %08x\n", dword);
> > verb_size = find_verb(dword, &verb);
> > find_verb(dword, &verb);
> > if (!verb_size) {
> > if (verb == NULL) {
> > printk_debug("No verb!\n");
> > return;
> > }
> > printk_debug("verb_size: %d\n", verb_size);
> > /* 3 */
> > for (i = 0; i < verb_size; i++) {
> > if (wait_for_ready(base) == -1)
> > 	return;
> > for (verb; verb->nid != 0xFF; verb++) {
> > dword = addr << 28 | verb->nid << 20 | 7 << 16;
> > pin_config = verb->pin_config;
> >
> > write32(base + 0x60, verb[i]);
> > for (i = 4; i > 0; i--, pin_config >>= 8) {
> > 	if (wait_for_ready(base) == -1)
> > 		return;
> >
> > if (wait_for_valid(base) == -1)
> > 	return;
> > 	if (verb->nid != 1)
> > 		dword1 = dword | ((0x20 - i) & 0xFF) <<

8;
> > 	else
> > 		dword1 = dword | ((0x24 - i) & 0xFF) <<

8;
> > 	dword1 |= (pin_config & 0xFF);
> > 	write32(base + 0x60, dword1);
> > 	if (wait_for_valid(base) == -1)
> > 		return;
> > }
> > }
> > printk_debug("verb loaded!\n");
> >
> > }
>
> Looks good.
>
> Regards,
> Carl-Daniel

[PATCH] sb600: new way to load initial verb

[Bao, Zheng]

Could it be a GSOC project?
1. Make an application to dump the codec structure and initial pin
configuration.
2. In Coreboot repository, add a public code to load initial pin
configuration as a CBFS modules.

Is it too small? I kinda don't have much time to do this.

Zheng
> -----Original Message-----
> From: [e-mail]

[mailto:[e-mail]]
> On Behalf Of Bao, Zheng
> Sent: Tuesday, March 09, 2010 11:02 AM
> To: [e-mail]
> Subject: Re: [coreboot] [PATCH] sb600: new way to load initial verb
> > -----Original Message-----
> > From: Carl-Daniel Hailfinger
>
> [mailto:[e-mail]]
> > Sent: Monday, March 08, 2010 10:15 PM
> > To: Bao, Zheng
> > Cc: Stefan Reinauer; [e-mail]
> > Subject: Re: [coreboot] [PATCH] sb600: new way to load initial verb
> >
> > On 08.03.2010 09:20, Bao, Zheng wrote:
> > > The original way to load initial verb is quite inflexible.
> > > The format of the command is:
> > > CAd 31:28 - codec address
> > > NID 27:20 - Nid
> > > Verb 19:0 - Verb data
> > > Each Nid has 4 byte data to write. It has to write one at a time.
> > >    0x01471c10,      //1st byte 10
> > >    0x01471d40,      //2nd byte 40
> > >    0x01471e01,      //3rd byte 01
> > >    0x01471f01,      //4th byte 01
> > >
> > > The new code in sb600_hda.c and the code structure are quite
> > > preliminary.  When it is almost done and can satisfy everybody, We
>
> can
> > > define the pin configuration code at mainboard.
> >
> > It would be cool if all chipsets could use the new verb
>
> implementation.
> > > #define CODEC_PIN_CONFIG_FILE "codec/alc_882.h"
> > > or
> > >  config CODEC_PIN_CONFIG_FILE
> > >    string
> > >    default "codec/alc_882.h"
> > >    depends on BOARD_AMD_DBM690T
> >
> > Or we define that file as separate uncompressed CBFS file.
> > > Now I am wondering if it is legal provide the intial pin
>
> configuration
> > > code in coreboot. If it is legal, how can we get it? The 882 code

is
> > > got from CIM.
> >
> > Sorry, what is CIM?
> > In theory, the pin configuration is mainboard specific. If that is
>
> true,
> > the pin configuration is similar to IRQ routing: There is usually

only
> > one correct and working solution. So it is possible that the

mainboard
> > vendor and the codec vendor own parts of it. Not sure. We could tell
> > users to dump verbs from their mainboards (is that possible?) and

add
> > the resulting file to CBFS.
>
> It is mainboard specific. Each codec also has its own code. The
> mainboard code is based on the codec code, I think. Dumping verbs is
> possible. It needs a lot of work from the ground up. There are some
> similar tools like http://helllabs.org/codecgraph/. Integrating them

to
> coreboot/util needs permission, doesn't it?
> > > Signed-off-by: Zheng Bao
> > >
> > > Index: src/codec/alc_882.h

===================================================================
> > > --- src/codec/alc_882.h	(revision 0)
> > > +++ src/codec/alc_882.h	(revision 0)
> > > @@ -0,0 +1,12 @@
> >
> > struct _pin_config_codec_entry pin_config_file[] = {
> > > {0x14, 0x01014010},
> > > {0x15, 0x01011012},
> > > {0x16, 0x01016011},
> > > {0x17, 0x01012014},
> > > {0x18, 0x01A19030},
> > > {0x19, 0x411111F0},
> > > {0x1a, 0x01813080},
> > > {0x1b, 0x411111F0},
> > > {0x1C, 0x411111F0},
> > > {0x1d, 0x411111F0},
> > > {0x1e, 0x01441150},
> > > {0x1f, 0x01C46160},
> >
> > {-1, -1}
> > };
> > > Index: src/southbridge/amd/sb600/sb600_hda.c

===================================================================
> > > --- src/southbridge/amd/sb600/sb600_hda.c	(revision 5195)
> > > +++ src/southbridge/amd/sb600/sb600_hda.c	(working copy)
> > > @@ -90,68 +90,19 @@
> > >    return 0;
> > >  }
> > >
> > > -static u32 cim_verb_data[] = {
> > > 0x01471c10,
> > > 0x01471d40,
> > > 0x01471e01,
> > > 0x01471f01,
> > >
> > > -/* 1 */
> > > 0x01571c12,
> > > 0x01571d10,
> > > 0x01571e01,
> > > 0x01571f01,
> > >
> > > -/* 2 */
> > > 0x01671c11,
> > > 0x01671d60,
> > > 0x01671e01,
> > > 0x01671f01,
> > >
> > > -/* 3 */
> > > 0x01771c14,
> > > 0x01771d20,
> > > 0x01771e01,
> > > 0x01771f01,
> > >
> > > -/* 4 */
> > > 0x01871c30,
> > > 0x01871d90,
> > > 0x01871ea1,
> > > 0x01871f01,
> > >
> > > -/* 5 */
> > > 0x01971cf0,
> > > 0x01971d11,
> > > 0x01971e11,
> > > 0x01971f41,
> > >
> > > -/* 6 */
> > > 0x01a71c80,
> > > 0x01a71d30,
> > > 0x01a71e81,
> > > 0x01a71f01,
> > >
> > > -/* 7 */
> > > 0x01b71cf0,
> > > 0x01b71d11,
> > > 0x01b71e11,
> > > 0x01b71f41,
> > >
> > > -/* 8 */
> > > 0x01c71cf0,
> > > 0x01c71d11,
> > > 0x01c71e11,
> > > 0x01c71f41,
> > >
> > > -/* 9 */
> > > 0x01d71cf0,
> > > 0x01d71d11,
> > > 0x01d71e11,
> > > 0x01d71f41,
> > >
> > > -/* 10 */
> > > 0x01e71c50,
> > > 0x01e71d11,
> > > 0x01e71e44,
> > > 0x01e71f01,
> > >
> > > -/* 11 */
> > > 0x01f71c60,
> > > 0x01f71d61,
> > > 0x01f71ec4,
> > > 0x01f71f01,
> > >
> > > +typedef struct _pin_config_codec_entry {
> >
> > Can you please kill the typedef and use "struct
>
> _pin_config_codec_entry"
> > instead?
> > > u8	nid;
> > > u32	pin_config;
> > >
> > > +} pin_config_codec_entry;
> > >
> > > +static pin_config_codec_entry pin_config_file[] = {
> > > +#ifdef	CODEC_PIN_CONFIG_FILE
> > > #include CODEC_PIN_CONFIG_FILE
> >
> > Including C source makes code difficult to read. Can you move the
> > complete struct to alc_882.h?
> >
> > Hm. Could we simply store the whole verb stuff uncompressed inside
>
> CBFS?
> >
>
> I think the purpose of the CBFS is to integrate some binary files

which
> are built at other place. The verb data we got is in text mode. We

need
> to build it into binary and store it into CBFS. Is it wasting? Why

don't
> we do it as fam10 patch code?
> > > +#endif
> > > {-1, -1}
> > >
> > > };
> > > -static u32 find_verb(u32 viddid, u32 ** verb)
> > >
> > > +static void find_verb(u32 viddid, pin_config_codec_entry ** verb)
> > >  {
> > >    device_t azalia_dev = dev_find_slot(0, PCI_DEVFN(0x14, 2));
> > >    struct southbridge_amd_sb600_config *cfg =
> > > @@ -159,12 +110,13 @@
> > >    printk_debug("Dev=%s\n", dev_path(azalia_dev));
> > >    printk_debug("Default viddid=%x\n", cfg->hda_viddid);
> > >    printk_debug("Reading viddid=%x\n", viddid);
> > > *verb = NULL;
> > > if (!cfg)
> > > return 0;
> > > return;
> > > if (viddid != cfg->hda_viddid)
> > > return 0;
> > > *verb = (u32 *) cim_verb_data;
> > > return sizeof(cim_verb_data) / sizeof(u32);
> > > return;
> > > *verb = pin_config_file;
> > >
> > > }
> > >
> > > /**
> > > @@ -214,9 +166,8 @@
> > >
> > > static void codec_init(u8 * base, int addr)
> > >  {
> > > u32 dword;
> > > u32 *verb;
> > > u32 verb_size;
> > >
> > > u32 dword, dword1, pin_config;
> > >
> > > pin_config_codec_entry *verb=NULL;
> > > int i;
> > >
> > > /* 1 */
> > >
> > > @@ -233,23 +184,32 @@
> > >
> > > /* 2 */
> > >    printk_debug("codec viddid: %08x\n", dword);
> > > verb_size = find_verb(dword, &verb);
> > > find_verb(dword, &verb);
> > > if (!verb_size) {
> > > if (verb == NULL) {
> > > printk_debug("No verb!\n");
> > > return;
> > > }
> > > printk_debug("verb_size: %d\n", verb_size);
> > > /* 3 */
> > > for (i = 0; i < verb_size; i++) {
> > > if (wait_for_ready(base) == -1)
> > > 	return;
> > > for (verb; verb->nid != 0xFF; verb++) {
> > > dword = addr << 28 | verb->nid << 20 | 7 << 16;
> > > pin_config = verb->pin_config;
> > >
> > > write32(base + 0x60, verb[i]);
> > > for (i = 4; i > 0; i--, pin_config >>= 8) {
> > > 	if (wait_for_ready(base) == -1)
> > > 		return;
> > >
> > > if (wait_for_valid(base) == -1)
> > > 	return;
> > > 	if (verb->nid != 1)
> > > 		dword1 = dword | ((0x20 - i) & 0xFF) <<
>
> 8;
> > > 	else
> > > 		dword1 = dword | ((0x24 - i) & 0xFF) <<
>
> 8;
> > > 	dword1 |= (pin_config & 0xFF);
> > > 	write32(base + 0x60, dword1);
> > > 	if (wait_for_valid(base) == -1)
> > > 		return;
> > > }
> > > }
> > > printk_debug("verb loaded!\n");
> > >
> > > }
> >
> > Looks good.
> >
> > Regards,
> > Carl-Daniel

among
> > computer science's most valuable treasures."
> > -- Donald E. Knuth