[coreboot] [PATCH] sb600: new way to load initial verb
Bao, Zheng
Zheng.Bao at amd.com
Tue Mar 9 04:02:06 CET 2010
> -----Original Message-----
> From: Carl-Daniel Hailfinger
[mailto:c-d.hailfinger.devel.2006 at gmx.net]
> Sent: Monday, March 08, 2010 10:15 PM
> To: Bao, Zheng
> Cc: Stefan Reinauer; coreboot at 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 at 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
>
More information about the coreboot
mailing list