Patchwork [rfc] ACPI cleanup proposal

login
register
about
Submitter Cristian Magherusan-Stanciu
Date 2011-06-02 18:20:18
Message ID <BANLkTikra-O_ZZag-i=T1_+Y9Zk7keY=Cg@mail.gmail.com>
Download mbox | patch
Permalink /patch/3016/
State New
Headers show

Comments

Cristian Magherusan-Stanciu - 2011-06-02 18:20:18
I found a few issues in the patch, please try the one attached to this
message instead.
Sorry for the clumsiness.

~Cristi
Mark Marshall - 2011-06-03 11:54:30
On 02/06/2011 19:20, Cristian Măgherușan-Stanciu wrote:
> I found a few issues in the patch, please try the one attached to this
> message instead.
> Sorry for the clumsiness.
>
> ~Cristi
>
Hi.

In the new functions, all of the form acpi_write_foo, there is a pointer 
passed in that is of type acpi_header_t, called foo.  This "in" 
parameter is then set to a value by the new function?  This doesn't seem 
to be correct - either the acpi_header_t * variable should be local to 
the function or it should be a "in/out" parameter?

(I think dsdt, for instance, should be in/out.  hpet should be local to 
acpi_write_hpet).

MM

Patch

diff --git a/src/arch/x86/boot/acpi.c b/src/arch/x86/boot/acpi.c
index 957ec45..460f88f 100644
--- a/src/arch/x86/boot/acpi.c
+++ b/src/arch/x86/boot/acpi.c
@@ -16,11 +16,6 @@ 
  */
 
 /*
- * Each system port implementing ACPI has to provide two functions:
- *
- *   write_acpi_tables()
- *   acpi_dump_apics()
- *
  * See Kontron 986LCD-M port for a good example of an ACPI implementation
  * in coreboot.
  */
@@ -465,6 +460,101 @@  void acpi_write_rsdp(acpi_rsdp_t *rsdp, acpi_rsdt_t *rsdt, acpi_xsdt_t *xsdt)
 	rsdp->ext_checksum = acpi_checksum((void *)rsdp, sizeof(acpi_rsdp_t));
 }
 
+
+
+void acpi_write_dsdt(acpi_header_t *dsdt, const unsigned char AmlCode[], unsigned long *current)
+{
+        *current = ALIGN(*current, 64);
+        printk(BIOS_DEBUG, "ACPI:    * DSDT at %lx\n", *current);
+        dsdt = (acpi_header_t *) *current; // it will used by fadt
+        memcpy(dsdt, &AmlCode, sizeof(acpi_header_t));
+        *current += dsdt->length;
+        memcpy(dsdt, &AmlCode, dsdt->length);
+        printk(BIOS_DEBUG, "ACPI:    * DSDT @ %p Length %x\n",dsdt,dsdt->length);
+}
+
+void acpi_write_facs(acpi_facs_t *facs, unsigned long *current)
+{
+	/* FACS */ // it needs 64 bit alignment
+	*current = ALIGN(*current, 64);
+	printk(BIOS_DEBUG, "ACPI:       * FACS at %lx\n", *current);
+	facs = (acpi_facs_t *) *current; // it will be used by fadt
+	*current += sizeof(acpi_facs_t);
+	acpi_create_facs(facs);
+}
+
+
+void acpi_write_fadt(acpi_fadt_t *fadt, acpi_facs_t *facs, acpi_header_t *dsdt, acpi_rsdp_t *rsdp, unsigned long *current)
+{
+	*current = ALIGN(*current, 64);
+	printk(BIOS_DEBUG, "ACPI:    * FADT at %lx\n", *current);
+	fadt = (acpi_fadt_t *) *current;
+	*current += sizeof(acpi_fadt_t);
+	acpi_create_fadt(fadt, facs, dsdt);
+	acpi_add_table(rsdp, fadt);
+}
+
+void acpi_write_hpet(acpi_hpet_t *hpet, acpi_rsdp_t *rsdp, unsigned long *current)
+{
+	*current = ALIGN(*current, 64);
+	printk(BIOS_DEBUG, "ACPI:    * HPET at %lx\n", *current);
+	hpet = (acpi_hpet_t *) *current;
+	*current += sizeof(acpi_hpet_t);
+	acpi_create_hpet(hpet);
+	acpi_add_table(rsdp, hpet);
+}
+
+void acpi_write_madt(acpi_madt_t *madt, acpi_rsdp_t *rsdp, unsigned long *current)
+{
+	*current = ALIGN(*current, 64);
+	printk(BIOS_DEBUG, "ACPI:    * MADT at %lx\n", *current);
+	madt = (acpi_madt_t *) *current;
+	acpi_create_madt(madt);
+	*current += madt->header.length;
+	acpi_add_table(rsdp, madt);
+}
+
+void acpi_write_srat(acpi_srat_t *srat, acpi_rsdp_t *rsdp, unsigned long *current)
+{
+	*current = ALIGN(*current, 64);
+	printk(BIOS_DEBUG, "ACPI:    * SRAT at %lx\n", *current);
+	srat = (acpi_srat_t *) *current;
+	acpi_create_srat(srat);
+	*current += srat->header.length;
+	acpi_add_table(rsdp, srat);
+}
+
+void acpi_write_slit(acpi_slit_t *slit, acpi_rsdp_t *rsdp, unsigned long *current)
+{
+	*current = ALIGN(*current, 64);
+	printk(BIOS_DEBUG, "ACPI:   * SLIT at %lx\n", *current);
+	slit = (acpi_slit_t *) *current;
+	acpi_create_slit(slit);
+	*current += slit->header.length;
+	acpi_add_table(rsdp, slit);
+}
+void acpi_write_mcfg(acpi_mcfg_t *mcfg, acpi_rsdp_t *rsdp, unsigned long *current)
+{
+	*current = ALIGN(*current, 64);
+	printk(BIOS_DEBUG, "ACPI:    * MCFG at %lx\n", *current);
+	mcfg = (acpi_mcfg_t *) *current;
+	acpi_create_mcfg(mcfg);
+	*current += mcfg->header.length;
+	acpi_add_table(rsdp, mcfg);
+}
+
+void acpi_write_ssdt_generated(acpi_header_t *ssdt, acpi_rsdp_t *rsdp,  unsigned long *current)
+{
+	printk(BIOS_DEBUG, "ACPI:    * SSDT\n");
+	ssdt = (acpi_header_t *) *current;
+
+	acpi_create_ssdt_generator(ssdt, "DYNADATA");
+	*current += ssdt->length;
+	acpi_add_table(rsdp, ssdt);
+
+}
+
+
 #if CONFIG_HAVE_ACPI_RESUME == 1
 void suspend_resume(void)
 {
diff --git a/src/arch/x86/include/arch/acpi.h b/src/arch/x86/include/arch/acpi.h
index 030745d..433aa3e 100644
--- a/src/arch/x86/include/arch/acpi.h
+++ b/src/arch/x86/include/arch/acpi.h
@@ -357,8 +357,9 @@  typedef struct acpi_ecdt {
 	u8 ec_id[];				/* EC ID  */
 } __attribute__ ((packed)) acpi_ecdt_t;
 
-/* These are implemented by the target port or north/southbridge. */
+/* This is implemented by the target port or north/southbridge. */
 unsigned long write_acpi_tables(unsigned long addr);
+
 unsigned long acpi_fill_madt(unsigned long current);
 unsigned long acpi_fill_mcfg(unsigned long current);
 unsigned long acpi_fill_srat(unsigned long current);
@@ -411,6 +412,15 @@  unsigned long acpi_create_slic(unsigned long current);
 void acpi_write_rsdt(acpi_rsdt_t *rsdt);
 void acpi_write_xsdt(acpi_xsdt_t *xsdt);
 void acpi_write_rsdp(acpi_rsdp_t *rsdp, acpi_rsdt_t *rsdt, acpi_xsdt_t *xsdt);
+void acpi_write_dsdt(acpi_header_t *dsdt, const unsigned char AmlCode[], unsigned long *current);
+void acpi_write_facs(acpi_facs_t *facs, unsigned long *current);
+void acpi_write_fadt(acpi_fadt_t *fadt, acpi_facs_t *facs, acpi_header_t *dsdt, acpi_rsdp_t *rsdp, unsigned long *current);
+void acpi_write_hpet(acpi_hpet_t *hpet, acpi_rsdp_t *rsdp, unsigned long *current);
+void acpi_write_madt(acpi_madt_t *madt, acpi_rsdp_t *rsdp, unsigned long *current);
+void acpi_write_srat(acpi_srat_t *srat, acpi_rsdp_t *rsdp, unsigned long *current);
+void acpi_write_slit(acpi_slit_t *slit, acpi_rsdp_t *rsdp, unsigned long *current);
+void acpi_write_ssdt_generated(acpi_header_t *ssdt, acpi_rsdp_t *rsdp,  unsigned long *current);
+void acpi_write_mcfg(acpi_mcfg_t *mcfg, acpi_rsdp_t *rsdp, unsigned long *current);
 
 #if CONFIG_HAVE_ACPI_RESUME
 /* 0 = S0, 1 = S1 ...*/
diff --git a/src/mainboard/asus/m2v-mx_se/acpi_tables.c b/src/mainboard/asus/m2v-mx_se/acpi_tables.c
index 73e3768..1e9b739 100644
--- a/src/mainboard/asus/m2v-mx_se/acpi_tables.c
+++ b/src/mainboard/asus/m2v-mx_se/acpi_tables.c
@@ -95,17 +95,17 @@  unsigned long acpi_fill_ssdt_generator(unsigned long current, const char *oem_ta
 unsigned long write_acpi_tables(unsigned long start)
 {
 	unsigned long current;
-	acpi_rsdp_t *rsdp;
-	acpi_srat_t *srat;
-	acpi_rsdt_t *rsdt;
-	acpi_mcfg_t *mcfg;
-	acpi_hpet_t *hpet;
-	acpi_madt_t *madt;
-	acpi_fadt_t *fadt;
-	acpi_facs_t *facs;
-	acpi_slit_t *slit;
-	acpi_header_t *ssdt;
-	acpi_header_t *dsdt;
+	acpi_rsdp_t *rsdp = NULL;
+	acpi_srat_t *srat = NULL;
+	acpi_rsdt_t *rsdt = NULL;
+	acpi_mcfg_t *mcfg = NULL;
+	acpi_hpet_t *hpet = NULL;
+	acpi_madt_t *madt = NULL;
+	acpi_fadt_t *fadt = NULL;
+	acpi_facs_t *facs = NULL;
+	acpi_slit_t *slit = NULL;
+	acpi_header_t *ssdt = NULL;
+	acpi_header_t *dsdt = NULL;
 
 	/* Align ACPI tables to 16 byte. */
 	start = (start + 0x0f) & -0x10;
@@ -125,71 +125,20 @@  unsigned long write_acpi_tables(unsigned long start)
 	acpi_write_rsdp(rsdp, rsdt, NULL);
 	acpi_write_rsdt(rsdt);
 
-	/* We explicitly add these tables later on: */
-	printk(BIOS_DEBUG, "ACPI:     * FACS\n");
-
-	/* we should align FACS to 64B as per ACPI specs */
-
-	current = ALIGN(current, 64);
-	facs = (acpi_facs_t *) current;
-	current += sizeof(acpi_facs_t);
-	acpi_create_facs(facs);
-
-	dsdt = (acpi_header_t *) current;
-	memcpy(dsdt, &AmlCode, sizeof(acpi_header_t));
-	current += dsdt->length;
-	memcpy(dsdt, &AmlCode, dsdt->length);
-	dsdt->checksum = 0;	/* Don't trust iasl to get this right. */
-	dsdt->checksum = acpi_checksum((u8*)dsdt, dsdt->length);
-	printk(BIOS_DEBUG, "ACPI:     * DSDT @ %p Length %x\n", dsdt,
-		     dsdt->length);
-	printk(BIOS_DEBUG, "ACPI:     * FADT\n");
-
-	fadt = (acpi_fadt_t *) current;
-	current += sizeof(acpi_fadt_t);
-
-	acpi_create_fadt(fadt, facs, dsdt);
-	acpi_add_table(rsdp, fadt);
-
-	printk(BIOS_DEBUG, "ACPI:    * HPET\n");
-	hpet = (acpi_hpet_t *) current;
-	current += sizeof(acpi_hpet_t);
-	acpi_create_hpet(hpet);
-	acpi_add_table(rsdp, hpet);
-
-	/* If we want to use HPET timers Linux wants an MADT. */
-	printk(BIOS_DEBUG, "ACPI:    * MADT\n");
-	madt = (acpi_madt_t *) current;
-	acpi_create_madt(madt);
-	current += madt->header.length;
-	acpi_add_table(rsdp, madt);
-
-	printk(BIOS_DEBUG, "ACPI:    * MCFG\n");
-	mcfg = (acpi_mcfg_t *) current;
-	acpi_create_mcfg(mcfg);
-	current += mcfg->header.length;
-	acpi_add_table(rsdp, mcfg);
-
-	printk(BIOS_DEBUG, "ACPI:    * SRAT\n");
-	srat = (acpi_srat_t *) current;
-	acpi_create_srat(srat);
-	current += srat->header.length;
-	acpi_add_table(rsdp, srat);
-
-	/* SLIT */
-        printk(BIOS_DEBUG, "ACPI:    * SLIT\n");
-        slit = (acpi_slit_t *) current;
-        acpi_create_slit(slit);
-        current+=slit->header.length;
-        acpi_add_table(rsdp,slit);
-
-	/* SSDT */
-	printk(BIOS_DEBUG, "ACPI:    * SSDT\n");
-	ssdt = (acpi_header_t *)current;
-
-	acpi_create_ssdt_generator(ssdt, "DYNADATA");
-	current += ssdt->length;
-	acpi_add_table(rsdp, ssdt);
+	acpi_write_dsdt(dsdt, AmlCode , &current);
+	acpi_write_facs(facs, &current);
+	acpi_write_fadt(fadt, facs, dsdt, rsdp, &current);
+	acpi_write_hpet(hpet, rsdp, &current);
+
+	/* If we want to use HPET Timers Linux wants an MADT */
+	acpi_write_madt(madt, rsdp, &current);
+
+	acpi_write_mcfg(mcfg, rsdp, &current);
+
+	acpi_write_srat(srat, rsdp, &current);
+	acpi_write_slit(slit, rsdp, &current);
+
+	acpi_write_ssdt_generated(ssdt, rsdp, &current);
 
 	printk(BIOS_INFO, "ACPI: done.\n");
 	return current;