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