Patchwork sb600: new way to load initial verb

login
register
about
Submitter Bao, Zheng
Date 2010-03-08 08:20:27
Message ID <DD1CC71B621B004FA76856E5129D6B170382637F@sbjgexmb1.amd.com>
Download mbox | patch
Permalink /patch/1023/
State New
Headers show

Comments

Bao, Zheng - 2010-03-08 08:20:27
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 <zheng.bao@amd.com>

 	printk_debug("verb loaded!\n");
 }



-----Original Message-----
From: coreboot-bounces@coreboot.org
[mailto:coreboot-bounces@coreboot.org] On Behalf Of Carl-Daniel
Hailfinger
Sent: Friday, March 05, 2010 10:03 PM
To: Stefan Reinauer
Cc: coreboot@coreboot.org
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
Carl-Daniel Hailfinger - 2010-03-08 14:14:57
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 <zheng.bao@amd.com>
>
> 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
Bao, Zheng - 2010-03-09 03:02:06
> -----Original Message-----
> From: Carl-Daniel Hailfinger
[mailto:c-d.hailfinger.devel.2006@gmx.net]
> Sent: Monday, March 08, 2010 10:15 PM
> To: Bao, Zheng
> Cc: Stefan Reinauer; coreboot@coreboot.org
> 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 <zheng.bao@amd.com>
> >
> > 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
> 
> --
> "I do consider assignment statements and pointer variables to be among
> computer science's most valuable treasures."
> -- Donald E. Knuth
>
Bao, Zheng - 2010-03-12 02:23:17
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: coreboot-bounces@coreboot.org
[mailto:coreboot-bounces@coreboot.org]
> On Behalf Of Bao, Zheng
> Sent: Tuesday, March 09, 2010 11:02 AM
> To: coreboot@coreboot.org
> Subject: Re: [coreboot] [PATCH] sb600: new way to load initial verb
> 
> 
> 
> > -----Original Message-----
> > From: Carl-Daniel Hailfinger
> [mailto:c-d.hailfinger.devel.2006@gmx.net]
> > Sent: Monday, March 08, 2010 10:15 PM
> > To: Bao, Zheng
> > Cc: Stefan Reinauer; coreboot@coreboot.org
> > 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 <zheng.bao@amd.com>
> > >
> > > 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
> >
> > --
> > "I do consider assignment statements and pointer variables to be
among
> > computer science's most valuable treasures."
> > -- Donald E. Knuth
> >
> 
> 
> 
> --
> coreboot mailing list: coreboot@coreboot.org
> http://www.coreboot.org/mailman/listinfo/coreboot

Patch

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;
+		}
 	}