Hi Rudolf,
that's an impressive piece of code!
On 01.02.2009 17:02, Rudolf Marek wrote:
> Hello,
>
> Following patch adds dynamic ACPI AML code generator which can be used to
> generate run-time ACPI ASL code like this:
>
> Name (XXX, 0xXX)
>
> Or:
>
> Scope (\_SB.PCI0)
> {
> Name (BUSN, Package (0x04)
> {
> 0x11111111,
> 0x22222222,
> 0x33333333,
> 0x44444444
> })
>
> Moreover it demonstrates its use on Asus M2V-MX SE where the SSDT table is
> generated by new function k8acpi_write_vars (technically similar to
> update_ssdt). But lot of nicer ;)
>
> The ACPI binary code generator will be also useful for P-states
> generation,
> without ugly run-time patching of DSDT.
>
> The old SSDT of K8 will be removed and rest of boards converted in
> next patch.
>
> Tested on that board. It generates same table as the static SSDT
> patched by
> amdk8_acpi. Windows still boots too ;)
>
> Signed-off-by: Rudolf Marek <[email protected]>
The following review is rather short and to-the-point. No offense intended.
A few comments about the general design:
The global variable gencurrent is a bit irritating. How do you handle
running on multiple cores?
Same for len_stack. Besides that, both variables are in the global
namespace and could use some acpi_ prefix or suffix.
> Index: coreboot-v2/src/arch/i386/boot/acpi.c
> ===================================================================
> --- coreboot-v2.orig/src/arch/i386/boot/acpi.c 2008-12-23
> 17:31:37.000000000 +0100
> +++ coreboot-v2/src/arch/i386/boot/acpi.c 2009-02-01 11:05:03.171807310
> +0100
> @@ -24,6 +24,7 @@
> #include <console/console.h>
> #include <string.h>
> #include <arch/acpi.h>
> +#include <arch/acpigen.h>
> #include <device/pci.h>
>
> u8 acpi_checksum(u8 *table, u32 length)
> @@ -180,6 +181,34 @@
> header->checksum = acpi_checksum((void *)mcfg, header->length);
> }
>
> +/* this can be overriden by platform ACPI setup code,
> + if it calls acpi_create_ssdt_generator */
> +unsigned long __attribute__((weak)) acpi_fill_ssdt_generator(unsigned long
> current,
>
I think weak symbols are something that makes code harder to follow.
> + char *oem_table_id) {
>
The { belongs on another line.
> + return current;
> +}
> +
> +void acpi_create_ssdt_generator(acpi_header_t *ssdt, char *oem_table_id)
>
Strange function name.
> +{
> + unsigned long current=(unsigned long)ssdt+sizeof(acpi_header_t);
> + memset((void *)ssdt, 0, sizeof(acpi_header_t));
> + memcpy(&ssdt->signature, SSDT_NAME, 4);
>
Maybe strncpy instead?
> + ssdt->revision = 2;
> + memcpy(&ssdt->oem_id, OEM_ID, 6);
> + memcpy(&ssdt->oem_table_id, oem_table_id, 8);
> + ssdt->oem_revision = 42;
> + memcpy(&ssdt->asl_compiler_id, "GENAML", 4);
> + ssdt->asl_compiler_revision = 42;
>
Heh.
> + ssdt->length = sizeof(acpi_header_t);
> +
> + acpigen_set_current((unsigned char *) current);
> + current = acpi_fill_ssdt_generator(current, oem_table_id);
> +
> + /* recalculate length */
> + ssdt->length = current - (unsigned long)ssdt;
> + ssdt->checksum = acpi_checksum((void *)ssdt, ssdt->length);
> +}
> +
> int acpi_create_srat_lapic(acpi_srat_lapic_t *lapic, u8 node, u8 apic)
> {
> lapic->type=0;
> Index: coreboot-v2/src/arch/i386/boot/acpigen.c
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ coreboot-v2/src/arch/i386/boot/acpigen.c 2009-02-01 11:04:09.927808334
> +0100
> @@ -0,0 +1,138 @@
> +/*
> + * This file is part of the coreboot project.
> + *
> + * Copyright (C) 2009 Rudolf Marek <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License v2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +/* how many nesting we support */
>
"how many nesting levels do we support"
> +#define ACPIGEN_LENSTACK_SIZE 10
> +
> +/* if you need to change this, change the acpigen_write_f and
> + acpigen_patch_len */
> +
>
Can you remove the empty line above?
> +#define ACPIGEN_MAXLEN 0xfff
> +
> +#include <string.h>
> +#include <arch/acpigen.h>
> +#include <console/console.h>
> +
> +static char *gencurrent;
> +
> +char *len_stack[ACPIGEN_LENSTACK_SIZE];
> +int ltop = 0;
>
These global variables need better names and some comments explaining
what they do.
> +
> +static int acpigen_write_len_f()
>
Needs a doxygen comment.
> +{
> + ASSERT(ltop < (ACPIGEN_LENSTACK_SIZE - 1))
> + len_stack[ltop++] = gencurrent;
> + acpigen_emit_byte(0);
> + acpigen_emit_byte(0);
> + return 2;
> +}
> +
> +void acpigen_patch_len(int len)
>
Needs a doxygen comment.
> +{
> + ASSERT(len <= ACPIGEN_MAXLEN)
> + ASSERT(ltop > 0)
> + char *p = len_stack[--ltop];
> + /* generate store length for 0xfff max */
> + p[0] = (0x40 | (len & 0xf));
> + p[1] = (len >> 4 & 0xff);
> +
> +}
> +
> +void acpigen_set_current(char *curr) {
>
Needs a doxygen comment.
> + gencurrent = curr;
> +}
> +
> +char *acpigen_get_current(void) {
>
Needs a doxygen comment.
> + return gencurrent;
> +}
> +
> +int acpigen_emit_byte(unsigned char b)
> +{
> + (*gencurrent++) = b;
> + return 1;
> +}
> +
> +int acpigen_write_package(int nr_el)
> +{
> + int len;
> + /* package op */
> + acpigen_emit_byte(0x12);
> + len = acpigen_write_len_f();
> + acpigen_emit_byte(nr_el);
> + return len + 2;
> +}
> +
> +int acpigen_write_byte(unsigned int data)
> +{
> + /* byte op */
> + acpigen_emit_byte(0xa);
> + acpigen_emit_byte(data & 0xff);
> + return 2;
> +}
> +
> +int acpigen_write_dword(unsigned int data)
> +{
> + /* dword op */
> + acpigen_emit_byte(0xc);
> + acpigen_emit_byte(data & 0xff);
> + acpigen_emit_byte((data >> 8) & 0xff);
> + acpigen_emit_byte((data >> 16) & 0xff);
> + acpigen_emit_byte((data >> 24) & 0xff);
> + return 5;
> +}
> +
> +int acpigen_write_name_byte(char *name, uint8_t val) {
> + int len;
> + len = acpigen_write_name(name);
> + len += acpigen_write_byte(val);
> + return len;
> +}
> +
> +int acpigen_write_name_dword(char *name, uint32_t val) {
> + int len;
> + len = acpigen_write_name(name);
> + len += acpigen_write_dword(val);
> + return len;
> +}
> +
> +int acpigen_emit_stream(char *data, int size) {
> + int i;
> + for (i = 0; i < size; i++) {
> + acpigen_emit_byte(data[i]);
> + }
> + return size;
> +}
> +
> +int acpigen_write_name(char *name)
> +{
> + int len = strlen(name);
> + /* name op */
> + acpigen_emit_byte(0x8);
> + acpigen_emit_stream(name, len);
> + return len + 1;
> +}
> +
> +int acpigen_write_scope(char *name)
> +{
> + int len;
> + /* scope op */
> + acpigen_emit_byte(0x10);
> + len = acpigen_write_len_f();
> + return len + acpigen_emit_stream(name, strlen(name)) + 1;
> +}
> Index: coreboot-v2/src/arch/i386/boot/Config.lb
> ===================================================================
> --- coreboot-v2.orig/src/arch/i386/boot/Config.lb 2009-02-01
> 10:25:41.491806925 +0100
> +++ coreboot-v2/src/arch/i386/boot/Config.lb 2009-02-01 10:30:56.756876765
> +0100
> @@ -13,5 +13,5 @@
> end
> if HAVE_ACPI_TABLES
> object acpi.o
> +object acpigen.o
> end
> -
> Index: coreboot-v2/src/arch/i386/include/arch/acpigen.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ coreboot-v2/src/arch/i386/include/arch/acpigen.h 2009-02-01
> 10:30:56.758807179 +0100
> @@ -0,0 +1,37 @@
> +/*
> + * This file is part of the coreboot project.
> + *
> + * Copyright (C) 2009 Rudolf Marek <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License v2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +#ifndef LIBACPI_H
> +#define LIBACPI_H
> +#include <assert.h>
> +#include <stdlib.h>
> +#include <stdint.h>
> +
> +void acpigen_patch_len(int len);
> +void acpigen_set_current(char *curr);
> +char *acpigen_get_current(void);
> +int acpigen_write_package(int nr_el);
> +int acpigen_write_byte(unsigned int data);
> +int acpigen_emit_byte(unsigned char data);
> +int acpigen_emit_stream(char *data, int size);
> +int acpigen_write_dword(unsigned int data);
> +int acpigen_write_name(char *name);
> +int acpigen_write_name_dword(char *name, uint32_t val);
> +int acpigen_write_name_byte(char *name, uint8_t val);
> +int acpigen_write_scope(char *name);
> +#endif
> Index: coreboot-v2/src/arch/i386/include/arch/acpi.h
> ===================================================================
> --- coreboot-v2.orig/src/arch/i386/include/arch/acpi.h 2008-12-23
> 17:38:41.000000000 +0100
> +++ coreboot-v2/src/arch/i386/include/arch/acpi.h 2009-02-01
> 10:52:25.478808600 +0100
> @@ -29,6 +29,7 @@
> #define MCFG_NAME "MCFG"
> #define SRAT_NAME "SRAT"
> #define SLIT_NAME "SLIT"
> +#define SSDT_NAME "SSDT"
>
> #define RSDT_TABLE "RSDT "
> #define HPET_TABLE "AMD64 "
> @@ -293,6 +294,8 @@
> unsigned long acpi_fill_madt(unsigned long current);
> unsigned long acpi_fill_mcfg(unsigned long current);
> unsigned long acpi_fill_srat(unsigned long current);
> +unsigned long acpi_fill_ssdt_generator(unsigned long current, char
> *oem_table_id);
> +void acpi_create_ssdt_generator(acpi_header_t *ssdt, char *oem_table_id);
>
The two functions above have rather similar names and it's not
immedately ofvious how they differ.
> void acpi_create_fadt(acpi_fadt_t *fadt,acpi_facs_t *facs,void *dsdt);
>
> /* These can be used by the target port */
> Index: coreboot-v2/src/mainboard/asus/m2v-mx_se/acpi_tables.c
> ===================================================================
> --- coreboot-v2.orig/src/mainboard/asus/m2v-mx_se/acpi_tables.c
> 2008-12-23 17:46:56.000000000 +0100
> +++ coreboot-v2/src/mainboard/asus/m2v-mx_se/acpi_tables.c 2009-02-01
> 11:23:05.142807423 +0100
> @@ -30,9 +30,9 @@
> #include <device/pci_ids.h>
> #include <../../../southbridge/via/vt8237r/vt8237r.h>
> #include <../../../southbridge/via/k8t890/k8t890.h>
> +#include <../../../northbridge/amd/amdk8/amdk8_acpi.h>
>
> extern unsigned char AmlCode[];
> -extern unsigned char AmlCode_ssdt[];
>
> unsigned long acpi_fill_mcfg(unsigned long current)
> {
> @@ -81,6 +81,12 @@
> return current;
> }
>
> +unsigned long acpi_fill_ssdt_generator(unsigned long current, char
> *oem_table_id) {
> + k8acpi_write_vars();
> + /* put PSTATES generator call here */
> + return (unsigned long) (acpigen_get_current());
>
acpigen_get_current is a really confusingly named function.
> +}
> +
> unsigned long write_acpi_tables(unsigned long start)
> {
> unsigned long current;
> @@ -175,13 +181,10 @@
> /* SSDT */
> printk_debug("ACPI: * SSDT\n");
> ssdt = (acpi_header_t *)current;
> - current += ((acpi_header_t *)AmlCode_ssdt)->length;
> - memcpy((void *)ssdt, (void *)AmlCode_ssdt, ((acpi_header_t
> *)AmlCode_ssdt)->length);
> - update_ssdt((void*)ssdt);
> - /* recalculate checksum */
> - ssdt->checksum = 0;
> - ssdt->checksum = acpi_checksum((unsigned char *)ssdt,ssdt->length);
> - acpi_add_table(rsdt,ssdt);
> +
> + acpi_create_ssdt_generator(ssdt, "DYNADATA");
>
"COREBOOT" please. That way, people can actually find out where the
table came from. The fact that the data are generated dynamically is
already visible from the compiler ID.
> + current += ssdt->length;
> + acpi_add_table(rsdt, ssdt);
>
> printk_info("ACPI: done.\n");
> return current;
> Index: coreboot-v2/src/northbridge/amd/amdk8/amdk8_acpi.c
> ===================================================================
> --- coreboot-v2.orig/src/northbridge/amd/amdk8/amdk8_acpi.c 2008-12-23
> 17:31:36.000000000 +0100
> +++ coreboot-v2/src/northbridge/amd/amdk8/amdk8_acpi.c 2009-02-01
> 11:26:00.102959065 +0100
> @@ -259,6 +259,80 @@
> }
> }
>
> +static int k8acpi_write_HT(void) {
> + device_t dev;
> + uint32_t dword;
> + int len, lenp, i;
> +
> + len = acpigen_write_name("HCLK");
> + lenp = acpigen_write_package(HC_POSSIBLE_NUM);
> +
> + for(i=0;i<sysconf.hc_possible_num;i++) {
> + lenp += acpigen_write_dword(sysconf.pci1234[i]);
> + }
> + for(i=sysconf.hc_possible_num; i<HC_POSSIBLE_NUM; i++) { // in case we
> set array size to other than 8
>
Comment on a separate line, please.
> + lenp += acpigen_write_dword(0x0);
> + }
> +
> + acpigen_patch_len(lenp - 1);
>
acpigen_patch_len is a name which does not really reveal function purpose.
> + len += lenp;
> +
> + len += acpigen_write_name("HCDN");
> + lenp = acpigen_write_package(HC_POSSIBLE_NUM);
> +
> + for(i=0;i<sysconf.hc_possible_num;i++) {
> + lenp += acpigen_write_dword(sysconf.hcdn[i]);
> + }
> + for(i=sysconf.hc_possible_num; i<HC_POSSIBLE_NUM; i++) { // in case we
> set array size to other than 8
> + lenp += acpigen_write_dword(0x20202020);
> + }
> + acpigen_patch_len(lenp - 1);
> + len += lenp;
> +
> + return len;
> +}
> +
> +static int k8acpi_write_pci_data(int dlen, char *name, int offset) {
> + device_t dev;
> + uint32_t dword;
> + int len, lenp, i;
> +
> + dev = dev_find_slot(0, PCI_DEVFN(0x18, 1));
> +
> + len = acpigen_write_name(name);
> + lenp = acpigen_write_package(dlen);
> + for(i=0; i<dlen; i++) {
> + dword = pci_read_config32(dev, offset+i*4);
> + lenp += acpigen_write_dword(dword);
> + }
> + // minus the opcode
> + acpigen_patch_len(lenp - 1);
> + return len + lenp;
> +}
> +
> +
> +int k8acpi_write_vars(void)
> +{
> + int lens;
>
Maybe use len instead of lens?
> + msr_t msr;
> + char pscope[] = "\\._SB_PCI0";
> +
> + lens = acpigen_write_scope(pscope);
> + lens += k8acpi_write_pci_data(4, "BUSN", 0xe0);
> + lens += k8acpi_write_pci_data(8, "PCIO", 0xc0);
> + lens += k8acpi_write_pci_data(16, "MMIO", 0x80);
> + lens += acpigen_write_name_byte("SBLK", sysconf.sblk);
> + lens += acpigen_write_name_byte("CBST",
> + ((sysconf.pci1234[0] >> 12) & 0xff) ? 0xf : 0x0);
> + lens += acpigen_write_name_dword("SBDN", sysconf.sbdn);
> + msr = rdmsr(TOP_MEM);
> + lens += acpigen_write_name_dword("TOM1", msr.lo);
>
I know the original code did not have it, but can you please add TOM2?
> +
> + lens += k8acpi_write_HT();
> + //minus opcode
> + acpigen_patch_len(lens - 1);
> + return lens;
> +}
>
> // used by acpi_tables.h
>
> Index: coreboot-v2/src/northbridge/amd/amdk8/amdk8_acpi.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ coreboot-v2/src/northbridge/amd/amdk8/amdk8_acpi.h 2009-02-01
> 11:28:13.236202005 +0100
> @@ -0,0 +1,25 @@
> +/*
> + * This file is part of the coreboot project.
> + *
> + * Copyright (C) 2009 Rudolf Marek <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License v2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +#ifndef AMDK8_ACPI_H
> +#define AMDK8_ACPI_H
> +#include <arch/acpigen.h>
> +
> +int k8acpi_write_vars(void);
> +
> +#endif
>
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
--
coreboot mailing list: [email protected]
http://www.coreboot.org/mailman/listinfo/coreboot
