[U-Boot] [UBOOT PATCH v5 1/3] x86: Generate a valid ACPI table

Saket Sinha saket.sinha89 at gmail.com
Fri Aug 21 06:33:09 CEST 2015


Hi Bin,

Please find my response inline -



On Tue, Aug 18, 2015 at 12:36 PM, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Saket,
>
> On Tue, Aug 18, 2015 at 3:29 AM, Saket Sinha <saket.sinha89 at gmail.com> wrote:
>> Implement write_acpi_table() to create a minimal working ACPI table.
>> This includes writing FACS, XSDT, RSDP, FADT, MCFG, MADT, DSDT & SSDT
>> ACPI table entries.
>>
>> Use a Kconfig option GENERATE_ACPI_TABLE to tell U-Boot whether we need
>> actually write the APCI table just like we did for PIRQ routing, MP table
>> and SFI tables. With ACPI table existence, linux kernel gets control of
>> power management, thermal management, configuration management and
>> monitoring in hardware.
>>
>
> Nice write-up!
>
>> Signed-off-by: Saket Sinha <saket.sinha89 at gmail.com>
>> ---
>>
>>  arch/x86/Kconfig                  |   9 +
>>  arch/x86/include/asm/acpi_table.h | 390 ++++++++++++++++++++++++++++++++++
>>  arch/x86/lib/Makefile             |   1 +
>>  arch/x86/lib/acpi_table.c         | 433 ++++++++++++++++++++++++++++++++++++++
>>  arch/x86/lib/tables.c             |   5 +
>>  scripts/Makefile.lib              |  11 +
>>  6 files changed, 849 insertions(+)
>>  create mode 100644 arch/x86/include/asm/acpi_table.h
>>  create mode 100644 arch/x86/lib/acpi_table.c
>>
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index 01ed760..ae881a1 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -344,6 +344,15 @@ config GENERATE_MP_TABLE
>>           multiprocessing as well as symmetric I/O interrupt handling with
>>           the local APIC and I/O APIC.
>>
>> +config GENERATE_ACPI_TABLE
>> +       bool "Generate an ACPI (Advanced Configuration and Power Interface) table"
>> +       default n
>> +       help
>> +         The Advanced Configuration and Power Interface (ACPI) specification
>> +         provides an open standard for device configuration and management
>> +         by the operating system. It defines platform-independent interfaces
>> +         for configuration and power management monitoring.
>> +
>>  endmenu
>>
>>  config MAX_PIRQ_LINKS
>> diff --git a/arch/x86/include/asm/acpi_table.h b/arch/x86/include/asm/acpi_table.h
>> new file mode 100644
>> index 0000000..a813a0a
>> --- /dev/null
>> +++ b/arch/x86/include/asm/acpi_table.h
>> @@ -0,0 +1,390 @@
>> +/*
>> + * Based on acpi.c from coreboot
>> + *
>> + * Copyright (C) 2015, Saket Sinha <saket.sinha89 at gmail.com>
>> + *
>> + * SPDX-License-Identifier: GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <malloc.h>
>> +#include <asm/post.h>
>> +#include <linux/string.h>
>> +
>> +#define RSDP_SIG "RSD PTR "  /* RSDT pointer signature */
>> +#define ACPI_TABLE_CREATOR "UBOOT   "  /* Must be exactly 8 bytes long! */
>> +#define OEM_ID "UBOOT "    /* Must be exactly 6 bytes long! */
>> +#define ASLC "INTL"      /* Must be exactly 4 bytes long! */
>> +
>
> Nits: could we use tab to between macro names, value and comments?

Addressed in patchset series v6.

>
>> +#define OEM_REVISION 42
>> +#define ASL_COMPILER_REVISION 42
>> +
>
> ditto
>

Addressed in patchset series v6.

>> +#define APM_CNT 0xb2
>> +#define APM_CNT_CST_CONTROL 0x85
>> +#define APM_CNT_PST_CONTROL 0x80
>> +#define APM_CNT_ACPI_DISABLE 0x1e
>> +#define APM_CNT_ACPI_ENABLE 0xe1
>> +#define APM_CNT_MBI_UPDATE 0xeb
>> +#define APM_CNT_GNVS_UPDATE 0xea
>> +#define APM_CNT_FINALIZE 0xcb
>> +#define APM_CNT_LEGACY 0xcc
>> +#define APM_ST 0xb3
>> +
>
> ditto, and can you add some comments on what these are?
>

Addressed in patchset series v6.

>> +#define MP_IRQ_POLARITY_DEFAULT 0x0
>> +#define MP_IRQ_POLARITY_HIGH 0x1
>> +#define MP_IRQ_POLARITY_LOW 0x3
>> +#define MP_IRQ_POLARITY_MASK 0x3
>> +#define MP_IRQ_TRIGGER_DEFAULT 0x0
>> +#define MP_IRQ_TRIGGER_EDGE 0x4
>> +#define MP_IRQ_TRIGGER_LEVEL 0xc
>> +#define MP_IRQ_TRIGGER_MASK 0xc
>> +
>
> ditto, and can you add some comments on what these are?
>

Addressed in patchset series v6.

>> +#define ACTL 0x00
>> +#define SCIS_MASK 0x07
>> +#define SCIS_IRQ9 0x00
>> +#define SCIS_IRQ10 0x01
>> +#define SCIS_IRQ11 0x02
>> +#define SCIS_IRQ20 0x04
>> +#define SCIS_IRQ21 0x05
>> +#define SCIS_IRQ22 0x06
>> +#define SCIS_IRQ23 0x07
>> +
>
> ditto, and can you add some comments on what these are?
>

Addressed in patchset series v6.

>> +enum acpi_bus_type {
>> +       PIC = 0,
>> +       APIC = 2,
>> +       ETHIGH = 5
>> +};
>> +
>
> I remember I pointed this out in previous version that the name of
> PIC, APIC is too generic.
>

Addressed in patchset series v6.

>> +#define ACPI_REV_ACPI_1_0 1
>> +#define ACPI_REV_ACPI_2_0 1
>> +#define ACPI_REV_ACPI_3_0 2
>> +#define ACPI_REV_ACPI_4_0 3
>> +#define ACPI_REV_ACPI_5_0 5
>> +
>> +#define ACPI_RSDP_REV_ACPI_1_0 0
>> +#define ACPI_RSDP_REV_ACPI_2_0 2
>> +
>> +typedef struct acpi_gen_regaddr {
>> +       u8  space_id;    /* Address space ID */
>> +       u8  bit_width;   /* Register size in bits */
>> +       u8  bit_offset;  /* Register bit offset */
>> +       union {
>> +               /* Reserved in ACPI 2.0 - 2.0b */
>> +               u8  resv;
>> +               /* Access size in ACPI 2.0c/3.0/4.0/5.0 */
>> +               u8  access_size;
>> +       };
>> +       u32 addrl; /* Register address, low 32 bits */
>> +       u32 addrh; /* Register address, high 32 bits */
>> +} acpi_addr_t;
>> +
>> +
>> +/* RSDP (Root System Description Pointer)
>> +Note: ACPI 1.0 didn't have length, xsdt_address, and ext_checksum. */
>
> Please use multi-line comment format. Please fix this globally (I
> think I pointed this out in previous review)
>

Addressed in patchset series v6.


>> +struct acpi_rsdp {
>> +       char  signature[8];     /* RSDP signature */
>> +       u8    checksum;         /* Checksum of the first 20 bytes */
>> +       char  oem_id[6];        /* OEM ID */
>> +       u8    revision;         /* 0 for ACPI 1.0, 2 for ACPI 2.0/3.0/4.0 */
>> +       u32   rsdt_address;     /* Physical address of RSDT (32 bits) */
>> +       u32   length;           /* Total RSDP length (incl. extended part) */
>> +       u64   xsdt_address;     /* Physical address of XSDT (64 bits) */
>> +       u8    ext_checksum;     /* Checksum of the whole table */
>> +       u8    reserved[3];
>> +};
>> +
>> +enum acpi_address_space_type {
>> +       ACPI_ADDRESS_SPACE_MEMORY = 0,  /* System memory */
>> +       ACPI_ADDRESS_SPACE_IO = 1,      /* System I/O */
>> +       ACPI_ADDRESS_SPACE_PCI = 2,     /* PCI config space */
>> +       ACPI_ADDRESS_SPACE_EC = 3,      /* Embedded controller */
>> +       ACPI_ADDRESS_SPACE_SMBUS = 4,   /* SMBus */
>> +       ACPI_ADDRESS_SPACE_PCC = 0x0a,  /* Platform Comm. Channel */
>> +       ACPI_ADDRESS_SPACE_FIXED = 0x7f /* Functional fixed hardware */
>> +};
>> +
>> +#define ACPI_FFIXEDHW_VENDOR_INTEL 1   /* Intel */
>> +#define ACPI_FFIXEDHW_CLASS_HLT 0      /* C1 Halt */
>> +#define ACPI_FFIXEDHW_CLASS_IO_HLT 1   /* C1 I/O then Halt */
>> +#define ACPI_FFIXEDHW_CLASS_MWAIT 2    /* MWAIT Native C-state */
>> +#define ACPI_FFIXEDHW_FLAG_HW_COORD 1  /* Hardware Coordination bit */
>> +#define ACPI_FFIXEDHW_FLAG_BM_STS 2    /* BM_STS avoidance bit */
>> +
>> +/* Access size definitions for Generic address structure */
>> +enum acpi_address_space_size {
>> +       ACPI_ACCESS_SIZE_UNDEFINED = 0, /* Undefined (legacy reasons) */
>> +       ACPI_ACCESS_SIZE_BYTE_ACCESS = 1,
>> +       ACPI_ACCESS_SIZE_WORD_ACCESS = 2,
>> +       ACPI_ACCESS_SIZE_DWORD_ACCESS = 3,
>> +       ACPI_ACCESS_SIZE_QWORD_ACCESS = 4
>> +};
>> +
>> +/* Generic ACPI header, provided by (almost) all tables */
>> +typedef struct acpi_table_header {
>> +       char signature[4];           /* ACPI signature (4 ASCII characters) */
>> +       u32  length;                 /* Table length in bytes (incl. header) */
>> +       u8   revision;               /* Table version (not ACPI version!) */
>> +       volatile u8 checksum;        /* To make sum of entire table == 0 */
>> +       char oem_id[6];              /* OEM identification */
>> +       char oem_table_id[8];        /* OEM table identification */
>> +       u32  oem_revision;           /* OEM revision number */
>> +       char asl_compiler_id[4];     /* ASL compiler vendor ID */
>> +       u32  asl_compiler_revision;  /* ASL compiler revision number */
>> +} acpi_header_t;
>> +
>> +/* A maximum number of 32 ACPI tables ought to be enough for now. */
>
> Please remove the ending .
>

Addressed in patchset series v6.

>> +#define MAX_ACPI_TABLES 32
>> +
>> +/* RSDT (Root System Description Table) */
>> +struct acpi_rsdt {
>> +       struct acpi_table_header header;
>> +       u32 entry[MAX_ACPI_TABLES];
>> +};
>> +
>> +/* XSDT (Extended System Description Table) */
>> +struct acpi_xsdt {
>> +       struct acpi_table_header header;
>> +       u64 entry[MAX_ACPI_TABLES];
>> +};
>> +
>> +/* MCFG (PCI Express MMIO config space BAR description table) */
>> +struct acpi_mcfg {
>> +       struct acpi_table_header header;
>> +       u8 reserved[8];
>> +};
>> +
>> +struct acpi_mcfg_mmconfig {
>> +       u32 base_address;
>> +       u32 base_reserved;
>> +       u16 pci_segment_group_number;
>> +       u8 start_bus_number;
>> +       u8 end_bus_number;
>> +       u8 reserved[4];
>> +};
>> +
>> +/* MADT (Multiple APIC Description Table) */
>> +struct acpi_madt {
>> +       struct acpi_table_header header;
>> +       u32 lapic_addr;                 /* Local APIC address */
>> +       u32 flags;                      /* Multiple APIC flags */
>> +} acpi_madt_t;
>> +
>> +enum dev_scope_type {
>> +       SCOPE_PCI_ENDPOINT = 1,
>> +       SCOPE_PCI_SUB = 2,
>> +       SCOPE_IOAPIC = 3,
>> +       SCOPE_MSI_HPET = 4
>> +};
>> +
>> +typedef struct dev_scope {
>> +       u8 type;
>> +       u8 length;
>> +       u8 reserved[2];
>> +       u8 enumeration;
>> +       u8 start_bus;
>> +       struct {
>> +               u8 dev;
>> +               u8 fn;
>> +       } path[0];
>> +} __packed dev_scope_t;
>> +
>> +/* MADT: APIC Structure Type*/
>> +enum acpi_apic_types {
>> +       LOCALAPIC       = 0,    /* Processor local APIC */
>> +       IOAPIC = 1, /* I/O APIC */
>> +       IRQSOURCEOVERRIDE = 2, /* Interrupt source override */
>> +       NMITYPE = 3, /* NMI source */
>> +       LOCALNMITYPE = 4, /* Local APIC NMI */
>> +       LAPICADDRESSOVERRIDE = 5, /* Local APIC address override */
>> +       IOSAPIC = 6, /* I/O SAPIC */
>> +       LOCALSAPIC = 7, /* Local SAPIC */
>> +       PLATFORMIRQSOURCES = 8, /* Platform interrupt sources */
>> +       LOCALX2SAPIC = 9, /* Processor local x2APIC */
>> +       LOCALX2APICNMI  = 10, /* Local x2APIC NMI */
>> +};
>> +
>> +/* MADT: Processor Local APIC Structure */
>> +struct acpi_madt_lapic {
>> +       u8 type;                             /* Type (0) */
>> +       u8 length;                      /* Length in bytes (8) */
>> +       u8 processor_id;                /* ACPI processor ID */
>> +       u8 apic_id;                     /* Local APIC ID */
>> +       u32 flags;                      /* Local APIC flags */
>> +};
>> +
>> +#define LOCAL_APIC_FLAG_ENABLED (1 << 0)
>> +/* bits 1-31: reserved */
>> +#define PCAT_COMPAT (1 << 0)
>> +/* bits 1-31: reserved */
>> +
>> +/* MADT: Local APIC NMI Structure */
>> +struct acpi_madt_lapic_nmi {
>> +       u8 type;                        /* Type (4) */
>> +       u8 length;              /* Length in bytes (6) */
>> +       u8 processor_id;        /* ACPI processor ID */
>> +       u16 flags;              /* MPS INTI flags */
>> +       u8 lint;                        /* Local APIC LINT# */
>> +};
>> +
>> +/* MADT: I/O APIC Structure */
>> +struct acpi_madt_ioapic {
>> +       u8 type;                        /* Type (1) */
>> +       u8 length;              /* Length in bytes (12) */
>> +       u8 ioapic_id;           /* I/O APIC ID */
>> +       u8 reserved;
>> +       u32 ioapic_addr;        /* I/O APIC address */
>> +       u32 gsi_base;           /* Global system interrupt base */
>> +};
>> +
>> +/* MADT: Interrupt Source Override Structure */
>> +struct acpi_madt_irqoverride {
>> +       u8 type;                        /* Type (2) */
>> +       u8 length;              /* Length in bytes (10) */
>> +       u8 bus;                 /* ISA (0) */
>> +       u8 source;              /* Bus-relative int. source (IRQ) */
>> +       u32 gsirq;              /* Global system interrupt */
>> +       u16 flags;              /* MPS INTI flags */
>> +};
>> +
>> +/* FADT (Fixed ACPI Description Table) */
>> +struct __packed acpi_fadt {
>> +       struct acpi_table_header header;
>> +       u32 firmware_ctrl;
>> +       u32 dsdt;
>> +       u8 model;
>> +       u8 preferred_pm_profile;
>> +       u16 sci_int;
>> +       u32 smi_cmd;
>> +       u8 acpi_enable;
>> +       u8 acpi_disable;
>> +       u8 s4bios_req;
>> +       u8 pstate_cnt;
>> +       u32 pm1a_evt_blk;
>> +       u32 pm1b_evt_blk;
>> +       u32 pm1a_cnt_blk;
>> +       u32 pm1b_cnt_blk;
>> +       u32 pm2_cnt_blk;
>> +       u32 pm_tmr_blk;
>> +       u32 gpe0_blk;
>> +       u32 gpe1_blk;
>> +       u8 pm1_evt_len;
>> +       u8 pm1_cnt_len;
>> +       u8 pm2_cnt_len;
>> +       u8 pm_tmr_len;
>> +       u8 gpe0_blk_len;
>> +       u8 gpe1_blk_len;
>> +       u8 gpe1_base;
>> +       u8 cst_cnt;
>> +       u16 p_lvl2_lat;
>> +       u16 p_lvl3_lat;
>> +       u16 flush_size;
>> +       u16 flush_stride;
>> +       u8 duty_offset;
>> +       u8 duty_width;
>> +       u8 day_alrm;
>> +       u8 mon_alrm;
>> +       u8 century;
>> +       u16 iapc_boot_arch;
>> +       u8 res2;
>> +       u32 flags;
>> +       struct acpi_gen_regaddr reset_reg;
>> +       u8 reset_value;
>> +       u8 res3;
>> +       u8 res4;
>> +       u8 res5;
>> +       u32 x_firmware_ctl_l;
>> +       u32 x_firmware_ctl_h;
>> +       u32 x_dsdt_l;
>> +       u32 x_dsdt_h;
>> +       struct acpi_gen_regaddr x_pm1a_evt_blk;
>> +       struct acpi_gen_regaddr x_pm1b_evt_blk;
>> +       struct acpi_gen_regaddr x_pm1a_cnt_blk;
>> +       struct acpi_gen_regaddr x_pm1b_cnt_blk;
>> +       struct acpi_gen_regaddr x_pm2_cnt_blk;
>> +       struct acpi_gen_regaddr x_pm_tmr_blk;
>> +       struct acpi_gen_regaddr x_gpe0_blk;
>> +       struct acpi_gen_regaddr x_gpe1_blk;
>> +};
>> +
>> +/* Flags for p_lvl2_lat and p_lvl3_lat */
>> +#define ACPI_FADT_C2_NOT_SUPPORTED     101
>> +#define ACPI_FADT_C3_NOT_SUPPORTED     1001
>> +
>> +/* FADT Feature Flags */
>> +#define ACPI_FADT_WBINVD       (1 << 0)
>> +#define ACPI_FADT_WBINVD_FLUSH (1 << 1)
>> +#define ACPI_FADT_C1_SUPPORTED (1 << 2)
>> +#define ACPI_FADT_C2_MP_SUPPORTED (1 << 3)
>> +#define ACPI_FADT_POWER_BUTTON (1 << 4)
>> +#define ACPI_FADT_SLEEP_BUTTON (1 << 5)
>> +#define ACPI_FADT_FIXED_RTC (1 << 6)
>> +#define ACPI_FADT_S4_RTC_WAKE  (1 << 7)
>> +#define ACPI_FADT_32BIT_TIMER  (1 << 8)
>> +#define ACPI_FADT_DOCKING_SUPPORTED (1 << 9)
>> +#define ACPI_FADT_RESET_REGISTER (1 << 10)
>> +#define ACPI_FADT_SEALED_CASE  (1 << 11)
>> +#define ACPI_FADT_HEADLESS (1 << 12)
>> +#define ACPI_FADT_SLEEP_TYPE (1 << 13)
>> +#define ACPI_FADT_PCI_EXPRESS_WAKE     (1 << 14)
>> +#define ACPI_FADT_PLATFORM_CLOCK (1 << 15)
>> +#define ACPI_FADT_S4_RTC_VALID (1 << 16)
>> +#define ACPI_FADT_REMOTE_POWER_ON (1 << 17)
>> +#define ACPI_FADT_APIC_CLUSTER (1 << 18)
>> +#define ACPI_FADT_APIC_PHYSICAL (1 << 19)
>> +/* Bits 20-31: reserved ACPI 3.0 & 4.0 */
>> +#define ACPI_FADT_HW_REDUCED_ACPI (1 << 20)
>> +#define ACPI_FADT_LOW_PWR_IDLE_S0 (1 << 21)
>> +/* bits 22-31: reserved ACPI 5.0 */
>> +
>
> Nits: could we use tab to between macro names, value and comments?
>

Addressed in patchset series v6.

>> +/* FADT Boot Architecture Flags */
>> +#define ACPI_FADT_LEGACY_DEVICES (1 << 0)
>> +#define ACPI_FADT_8042 (1 << 1)
>> +#define ACPI_FADT_VGA_NOT_PRESENT (1 << 2)
>> +#define ACPI_FADT_MSI_NOT_SUPPORTED (1 << 3)
>> +#define ACPI_FADT_NO_PCIE_ASPM_CONTROL (1 << 4)
>> +#define ACPI_FADT_LEGACY_FREE 0x00     /* No legacy devices (including 8042) */
>> +
>
> ditto.
>

Addressed in patchset series v6.

>> +/* FADT Preferred Power Management Profile */
>> +#define PM_UNSPECIFIED 0
>> +#define PM_DESKTOP 1
>> +#define PM_MOBILE 2
>> +#define PM_WORKSTATION 3
>> +#define PM_ENTERPRISE_SERVER 4
>> +#define PM_SOHO_SERVER 5
>> +#define PM_APPLIANCE_PC 6
>> +#define PM_PERFORMANCE_SERVER 7
>> +#define PM_TABLET 8, /* ACPI 5.0 */
>> +
>
> ditto.
>

Addressed in patchset series v6.

>> +/* FACS (Firmware ACPI Control Structure) */
>> +struct acpi_facs {
>> +       char signature[4];                      /* "FACS" */
>> +       u32 length;                             /* Length in bytes (>= 64) */
>> +       u32 hardware_signature;                 /* Hardware signature */
>> +       u32 firmware_waking_vector;             /* Firmware waking vector */
>> +       u32 global_lock;                        /* Global lock */
>> +       u32 flags;                              /* FACS flags */
>> +       u32 x_firmware_waking_vector_l;         /* X FW waking vector, low */
>> +       u32 x_firmware_waking_vector_h;         /* X FW waking vector, high */
>> +       u8 version;                             /* ACPI 4.0: 2 */
>> +       u8 resv[31];                            /* FIXME: 4.0: ospm_flags */
>> +};
>> +
>> +/* FACS flags */
>> +#define ACPI_FACS_S4BIOS_F (1 << 0)
>> +#define ACPI_FACS_64BIT_WAKE_F (1 << 1)
>> +/* Bits 31..2: reserved */
>> +
>> +/* These can be used by the target port. */
>> +
>> +unsigned long acpi_create_madt_lapics(unsigned long current);
>> +int acpi_create_madt_ioapic(struct acpi_madt_ioapic *ioapic, u8 id, u32 addr,
>
> Can we always use u32 for the parameters? like u32 id
>

These will make me change all the structure and recheck that nothing
breaks since these values are getting assigned to the table structures
directly.
I do not think this makes any sense to have 4 bytes for something that
can be fitted in 1 byte.

>> +               u32 gsi_base);
>> +int acpi_create_madt_irqoverride(struct acpi_madt_irqoverride *irqoverride,
>> +               u8 bus, u8 source, u32 gsirq, u16 flags);
>
> Ditto, for bus, source and flags
>

Addressed in patchset series v6.

>> +unsigned long acpi_fill_madt(unsigned long current);
>> +void acpi_create_fadt(struct acpi_fadt *fadt, struct acpi_facs *facs,
>> +               void *dsdt);
>
> I believe checkpatch.pl will report indention warning here.
>

Addressed in patchset series v6.

>> +int acpi_create_madt_lapic_nmi(struct acpi_madt_lapic_nmi *lapic_nmi, u8 cpu,
>> +               u16 flags, u8 lint);
>
> Ditto, for cpu, flags and lint.
>

I do not think this makes any sense to have 4 bytes for something that
can be fitted in 1 byte.

>> +unsigned long write_acpi_tables(unsigned long start);
>> +
>> diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile
>> index dcfe9ee..6ecd6db 100644
>> --- a/arch/x86/lib/Makefile
>> +++ b/arch/x86/lib/Makefile
>> @@ -30,6 +30,7 @@ obj-y += physmem.o
>>  obj-$(CONFIG_X86_RAMTEST) += ramtest.o
>>  obj-y += sfi.o
>>  obj-y  += string.o
>> +obj-$(CONFIG_GENERATE_ACPI_TABLE) += acpi_table.o
>>  obj-y  += tables.o
>>  obj-$(CONFIG_SYS_X86_TSC_TIMER)        += tsc_timer.o
>>  obj-$(CONFIG_CMD_ZBOOT)        += zimage.o
>> diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
>> new file mode 100644
>> index 0000000..bd880ea
>> --- /dev/null
>> +++ b/arch/x86/lib/acpi_table.c
>> @@ -0,0 +1,433 @@
>> +/*
>> + * Based on acpi.c from coreboot
>> + *
>> + * Copyright (C) 2015, Saket Sinha <saket.sinha89 at gmail.com>
>> + *
>> + * SPDX-License-Identifier: GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <cpu.h>
>> +#include <dm.h>
>> +#include <dm/uclass-internal.h>
>> +#include <dm/lists.h>
>> +#include <asm/acpi_table.h>
>> +#include <asm/cpu.h>
>> +#include <asm/ioapic.h>
>> +#include <asm/lapic.h>
>> +#include <asm/tables.h>
>> +#include <asm/pci.h>
>> +
>> +
>> +extern const unsigned char AmlCode[];
>> +
>
> Please add a comment block here to explain why we have to use
> CamelCase, in case someone later wants to improve but will fail.
>

Addressed in patchset series v6.

>> +/**
>> +* Add an ACPI table to the RSDT (and XSDT) structure, recalculate length
>> +* and checksum.
>> +*/
>> +static void acpi_add_table(struct acpi_rsdp *rsdp, void *table)
>> +{
>> +       int i, entries_num;
>> +       struct acpi_rsdt *rsdt;
>> +       struct acpi_xsdt *xsdt = NULL;
>> +
>> +       /* The RSDT is mandatory while the XSDT is not */
>> +       rsdt = (struct acpi_rsdt *)rsdp->rsdt_address;
>> +
>> +       if (rsdp->xsdt_address)
>> +               xsdt = (struct acpi_xsdt *)((u32)rsdp->xsdt_address);
>> +
>> +       /* This should always be MAX_ACPI_TABLES */
>> +       entries_num = ARRAY_SIZE(rsdt->entry);
>> +
>> +       for (i = 0; i < entries_num; i++) {
>> +               if (rsdt->entry[i] == 0)
>> +                       break;
>> +       }
>> +
>> +       if (i >= entries_num) {
>> +               debug("ACPI: Error: too many tables.\n");
>> +               return;
>> +       }
>> +
>> +       /* Add table to the RSDT */
>> +       rsdt->entry[i] = (u32)table;
>> +
>> +       /* Fix RSDT length or the kernel will assume invalid entries */
>> +       rsdt->header.length = sizeof(acpi_header_t) + (sizeof(u32) * (i + 1));
>> +
>> +       /* Re-calculate checksum. */
>
> Nits: please remove the ending .
>

Addressed in patchset series v6.

>> +       rsdt->header.checksum = 0;
>> +       rsdt->header.checksum = table_compute_checksum((u8 *)rsdt,
>> +                       rsdt->header.length);
>> +
>> +       /*
>> +        * And now the same thing for the XSDT. We use the same index as for
>> +        * now we want the XSDT and RSDT to always be in sync in uboot
>
> Should be U-Boot
>

Addressed in patchset series v6.

>> +        */
>> +       if (xsdt) {
>> +               /* Add table to the XSDT */
>> +               xsdt->entry[i] = (u64)(u32)table;
>> +
>> +               /* Fix XSDT length */
>> +               xsdt->header.length = sizeof(acpi_header_t) +
>> +                                                       (sizeof(u64) * (i + 1));
>
> Can we adjust the indention here, to align with sizeof above?
>

Addressed in patchset series v6.

>> +
>> +               /* Re-calculate checksum */
>> +               xsdt->header.checksum = 0;
>> +               xsdt->header.checksum = table_compute_checksum((u8 *)xsdt,
>> +                               xsdt->header.length);
>> +       }
>> +}
>> +
>> +static int acpi_create_madt_lapic(struct acpi_madt_lapic *lapic,
>> +               u8 cpu, u8 apic)
>
> I believe chckpatch.pl will report indention warnings. Please fix this globally.
>

Addressed in patchset series v6.

>> +{
>> +       lapic->type = LOCALAPIC; /* Local APIC structure */
>> +       lapic->length = sizeof(struct acpi_madt_lapic);
>> +       lapic->flags = LOCAL_APIC_FLAG_ENABLED; /* Processor/LAPIC enabled */
>> +       lapic->processor_id = cpu;
>> +       lapic->apic_id = apic;
>> +
>> +       return lapic->length;
>> +}
>> +
>> +unsigned long acpi_create_madt_lapics(unsigned long current)
>> +{
>> +       struct udevice *dev;
>> +
>> +       for (uclass_find_first_device(UCLASS_CPU, &dev);
>> +               dev;
>> +               uclass_find_next_device(&dev)) {
>> +               struct cpu_platdata *plat = dev_get_parent_platdata(dev);
>> +
>> +               current += acpi_create_madt_lapic
>> +                       ((struct acpi_madt_lapic *)current,
>> +                        plat->cpu_id, plat->cpu_id);
>> +               }
>> +               return current;
>> +}
>> +
>> +int acpi_create_madt_ioapic(struct acpi_madt_ioapic *ioapic, u8 id, u32 addr,
>> +                                                       u32 gsi_base)
>> +{
>> +       ioapic->type = IOAPIC;
>> +       ioapic->length = sizeof(struct acpi_madt_ioapic);
>> +       ioapic->reserved = 0x00;
>> +       ioapic->gsi_base = gsi_base;
>> +       ioapic->ioapic_id = id;
>> +       ioapic->ioapic_addr = addr;
>> +
>> +       return ioapic->length;
>> +}
>> +
>> +int acpi_create_madt_irqoverride(struct acpi_madt_irqoverride *irqoverride,
>> +                       u8 bus, u8 source, u32 gsirq, u16 flags)
>> +{
>> +       irqoverride->type = IRQSOURCEOVERRIDE;
>> +       irqoverride->length = sizeof(struct acpi_madt_irqoverride);
>> +       irqoverride->bus = bus;
>> +       irqoverride->source = source;
>> +       irqoverride->gsirq = gsirq;
>> +       irqoverride->flags = flags;
>> +
>> +       return irqoverride->length;
>> +}
>> +
>> +int acpi_create_madt_lapic_nmi(struct acpi_madt_lapic_nmi *lapic_nmi,
>> +               u8 cpu, u16 flags, u8 lint)
>> +{
>> +       lapic_nmi->type = LOCALNMITYPE;
>> +       lapic_nmi->length = sizeof(struct acpi_madt_lapic_nmi);
>> +       lapic_nmi->flags = flags;
>> +       lapic_nmi->processor_id = cpu;
>> +       lapic_nmi->lint = lint;
>> +
>> +       return lapic_nmi->length;
>> +}
>> +
>> +static void fill_header(acpi_header_t *header, char *signature, int length)
>> +{
>> +       memcpy(header->signature, signature, length);
>> +       memcpy(header->oem_id, OEM_ID, 6);
>> +       memcpy(header->oem_table_id, ACPI_TABLE_CREATOR, 8);
>> +       memcpy(header->asl_compiler_id, ASLC, 4);
>> +}
>> +
>> +static void acpi_create_madt(struct acpi_madt *madt)
>> +{
>> +       acpi_header_t *header = &(madt->header);
>> +       unsigned long current = (unsigned long)madt + sizeof(struct acpi_madt);
>> +
>> +       memset((void *)madt, 0, sizeof(struct acpi_madt));
>> +
>> +       /* Fill out header fields. */
>
> Nits: please remove the ending .
>

Addressed in patchset series v6.

>> +       fill_header(header, "APIC", 4);
>> +       header->length = sizeof(struct acpi_madt);
>> +
>> +       /* ACPI 1.0/2.0: 1, ACPI 3.0: 2, ACPI 4.0: 3 */
>> +       header->revision = ACPI_REV_ACPI_2_0;
>> +
>> +       madt->lapic_addr = LAPIC_DEFAULT_BASE;
>> +       madt->flags = PCAT_COMPAT;
>> +
>> +       current = acpi_fill_madt(current);
>> +
>> +       /* (Re)calculate length and checksum */
>> +       header->length = current - (unsigned long)madt;
>> +
>> +       header->checksum = table_compute_checksum((void *)madt, header->length);
>> +}
>> +
>> +static int acpi_create_mcfg_mmconfig(struct acpi_mcfg_mmconfig *mmconfig,
>> +               u32 base, u16 seg_nr, u8 start, u8 end)
>> +{
>> +       memset(mmconfig, 0, sizeof(*mmconfig));
>> +       mmconfig->base_address = base;
>> +       mmconfig->base_reserved = 0;
>> +       mmconfig->pci_segment_group_number = seg_nr;
>> +       mmconfig->start_bus_number = start;
>> +       mmconfig->end_bus_number = end;
>> +
>> +       return sizeof(struct acpi_mcfg_mmconfig);
>> +}
>> +
>> +static unsigned long acpi_fill_mcfg(unsigned long current)
>> +{
>> +       current += acpi_create_mcfg_mmconfig
>> +               ((struct        acpi_mcfg_mmconfig *)current,
>
> Please remove the multiple spaces here.
>

Addressed in patchset series v6.

>> +                CONFIG_PCIE_ECAM_BASE, 0x0, 0x0, 255);
>> +
>> +       return current;
>> +}
>> +
>> +/* MCFG is defined in the PCI Firmware Specification 3.0 */
>> +static void acpi_create_mcfg(struct acpi_mcfg *mcfg)
>> +{
>> +       acpi_header_t *header = &(mcfg->header);
>> +       unsigned long current = (unsigned long)mcfg + sizeof(struct acpi_mcfg);
>> +
>> +       memset((void *)mcfg, 0, sizeof(struct acpi_mcfg));
>> +
>> +       /* Fill out header fields */
>> +       fill_header(header, "MCFG", 4);
>> +       header->length = sizeof(struct acpi_mcfg);
>> +
>> +       /* ACPI 1.0/2.0: 1, ACPI 3.0: 2, ACPI 4.0: 3 */
>> +       header->revision = ACPI_REV_ACPI_2_0;
>> +
>> +       current = acpi_fill_mcfg(current);
>> +
>> +       /* (Re)calculate length and checksum */
>> +       header->length = current - (unsigned long)mcfg;
>> +       header->checksum = table_compute_checksum((void *)mcfg, header->length);
>> +}
>> +
>> +static void acpi_create_facs(struct acpi_facs *facs)
>> +{
>> +       memset((void *)facs, 0, sizeof(struct acpi_facs));
>> +
>> +       memcpy(facs->signature, "FACS", 4);
>> +       facs->length = sizeof(struct acpi_facs);
>> +       facs->hardware_signature = 0;
>> +       facs->firmware_waking_vector = 0;
>> +       facs->global_lock = 0;
>> +       facs->flags = 0;
>> +       facs->x_firmware_waking_vector_l = 0;
>> +       facs->x_firmware_waking_vector_h = 0;
>> +       facs->version = 1; /* ACPI 1.0: 0, ACPI 2.0/3.0: 1, ACPI 4.0: 2 */
>> +}
>> +
>> +static void acpi_write_rsdt(struct acpi_rsdt *rsdt)
>> +{
>> +       acpi_header_t *header = &(rsdt->header);
>> +
>> +       /* Fill out header fields */
>> +       fill_header(header, "RSDT", 4);
>> +       header->length = sizeof(struct acpi_rsdt);
>> +
>> +       /* ACPI 1.0/2.0: 1, ACPI 3.0: 2, ACPI 4.0: 3 */
>> +       header->revision = ACPI_REV_ACPI_2_0;
>> +
>> +       /* Entries are filled in later, we come with an empty set */
>> +
>> +       /* Fix checksum */
>> +       header->checksum = table_compute_checksum((void *)rsdt,
>> +                       sizeof(struct acpi_rsdt));
>> +}
>> +
>> +static void acpi_write_xsdt(struct acpi_xsdt *xsdt)
>> +{
>> +       acpi_header_t *header = &(xsdt->header);
>> +
>> +       /* Fill out header fields */
>> +       fill_header(header, "XSDT", 4);
>> +       header->length = sizeof(struct acpi_xsdt);
>> +
>> +       /* ACPI 1.0/2.0: 1, ACPI 3.0: 2, ACPI 4.0: 3 */
>> +       header->revision = ACPI_REV_ACPI_2_0;
>> +
>> +       /* Entries are filled in later, we come with an empty set */
>> +
>> +       /* Fix checksum */
>> +       header->checksum = table_compute_checksum((void *)xsdt,
>> +                       sizeof(struct acpi_xsdt));
>> +}
>> +
>> +static void acpi_write_rsdp(struct acpi_rsdp *rsdp, struct acpi_rsdt *rsdt,
>> +               struct acpi_xsdt *xsdt)
>> +{
>> +       memset(rsdp, 0, sizeof(struct acpi_rsdp));
>> +
>> +       memcpy(rsdp->signature, RSDP_SIG, 8);
>> +       memcpy(rsdp->oem_id, OEM_ID, 6);
>> +
>> +       rsdp->length = sizeof(struct acpi_rsdp);
>> +       rsdp->rsdt_address = (u32)rsdt;
>> +
>> +       /*
>> +       * Revision: ACPI 1.0: 0, ACPI 2.0/3.0/4.0: 2
>> +       *
>> +       * Some OSes expect an XSDT to be present for RSD PTR revisions >= 2.
>> +       * If we don't have an ACPI XSDT, force ACPI 1.0 (and thus RSD PTR
>> +       * revision 0)
>> +       */
>> +       if (xsdt == NULL) {
>> +               rsdp->revision = ACPI_RSDP_REV_ACPI_1_0;
>> +       } else {
>> +               rsdp->xsdt_address = (u64)(u32)xsdt;
>> +               rsdp->revision = ACPI_RSDP_REV_ACPI_2_0;
>> +       }
>> +
>> +       /* Calculate checksums */
>> +       rsdp->checksum = table_compute_checksum((void *)rsdp, 20);
>> +       rsdp->ext_checksum = table_compute_checksum((void *)rsdp,
>> +                       sizeof(struct acpi_rsdp));
>> +}
>> +
>> +static void acpi_create_ssdt_generator(acpi_header_t *ssdt,
>> +               const char *oem_table_id)
>> +{
>> +       unsigned long current = (unsigned long)ssdt + sizeof(acpi_header_t);
>> +
>> +       memset((void *)ssdt, 0, sizeof(acpi_header_t));
>> +
>> +       memcpy(&ssdt->signature, "SSDT", 4);
>> +       /* Access size in ACPI 2.0c/3.0/4.0/5.0 */
>> +       ssdt->revision = ACPI_REV_ACPI_3_0;
>> +       memcpy(&ssdt->oem_id, OEM_ID, 6);
>> +       memcpy(&ssdt->oem_table_id, oem_table_id, 8);
>> +       ssdt->oem_revision = OEM_REVISION;
>> +       memcpy(&ssdt->asl_compiler_id, ASLC, 4);
>> +       ssdt->asl_compiler_revision = ASL_COMPILER_REVISION;
>> +       ssdt->length = sizeof(acpi_header_t);
>> +
>> +       /* (Re)calculate length and checksum */
>> +       ssdt->length = current - (unsigned long)ssdt;
>> +       ssdt->checksum = table_compute_checksum((void *)ssdt, ssdt->length);
>> +}
>> +
>> +unsigned long write_acpi_tables(unsigned long start)
>> +{
>> +       unsigned long current;
>> +       struct acpi_rsdp *rsdp;
>> +       struct acpi_rsdt *rsdt;
>> +       struct acpi_xsdt *xsdt;
>> +       struct acpi_facs *facs;
>> +       acpi_header_t *dsdt;
>> +       struct acpi_fadt *fadt;
>> +       struct acpi_mcfg *mcfg;
>> +       struct acpi_madt *madt;
>> +       acpi_header_t *ssdt;
>> +
>> +       current = start;
>> +
>> +       /* Align ACPI tables to 16byte */
>> +       current = (ALIGN(current, 16));
>
> Please remove the unnecessary outer () and fix this globally in this routine.
>

Addressed in patchset series v6.

>> +
>> +       debug("ACPI: Writing ACPI tables at %lx.\n", start);
>> +
>> +       /* We need at least an RSDP and an RSDT Table */
>> +       rsdp = (struct acpi_rsdp *)current;
>> +       current += sizeof(struct acpi_rsdp);
>> +       current = (ALIGN(current, 16));
>> +       rsdt = (struct acpi_rsdt *)current;
>> +       current += sizeof(struct acpi_rsdt);
>> +       current = (ALIGN(current, 16));
>> +       xsdt = (struct acpi_xsdt *)current;
>> +       current += sizeof(struct acpi_xsdt);
>> +       current = (ALIGN(current, 16));
>> +
>> +       /* clear all table memory */
>> +       memset((void *)start, 0, current - start);
>> +
>> +       acpi_write_rsdp(rsdp, rsdt, xsdt);
>> +       acpi_write_rsdt(rsdt);
>> +       acpi_write_xsdt(xsdt);
>> +
>> +       debug("ACPI:    * FACS\n");
>> +       facs = (struct acpi_facs *)current;
>> +       current += sizeof(struct acpi_facs);
>> +       current = (ALIGN(current, 16));
>> +
>> +       acpi_create_facs(facs);
>> +
>> +       debug("ACPI:    * DSDT\n");
>> +       dsdt = (acpi_header_t *)current;
>> +       memcpy(dsdt, &AmlCode, sizeof(acpi_header_t));
>> +       if (dsdt->length >= sizeof(acpi_header_t)) {
>> +               current += sizeof(acpi_header_t);
>> +               memcpy((char *)current,
>> +                               (char *)&AmlCode + sizeof(acpi_header_t),
>> +                               dsdt->length - sizeof(acpi_header_t));
>> +               current += dsdt->length - sizeof(acpi_header_t);
>> +
>> +               /* (Re)calculate length and checksum */
>> +               dsdt->length = current - (unsigned long)dsdt;
>> +               dsdt->checksum = 0;
>> +               dsdt->checksum = table_compute_checksum((void *)dsdt,
>> +                               dsdt->length);
>> +       }
>> +       current = (ALIGN(current, 16));
>> +
>> +       debug("ACPI:    * FADT\n");
>> +       fadt = (struct acpi_fadt *)current;
>> +       current += sizeof(struct acpi_fadt);
>> +       current = (ALIGN(current, 16));
>> +       acpi_create_fadt(fadt, facs, dsdt);
>> +       acpi_add_table(rsdp, fadt);
>> +
>> +       debug("ACPI:    * MCFG\n");
>> +       mcfg = (struct acpi_mcfg *)current;
>> +       acpi_create_mcfg(mcfg);
>> +       if (mcfg->header.length > sizeof(struct acpi_mcfg)) {
>> +               current += mcfg->header.length;
>> +               current = (ALIGN(current, 16));
>> +               acpi_add_table(rsdp, mcfg);
>> +       }
>> +
>> +       debug("ACPI:    * MADT\n");
>> +       madt = (struct acpi_madt *)current;
>> +       acpi_create_madt(madt);
>> +       if (madt->header.length > sizeof(struct acpi_madt)) {
>> +               current += madt->header.length;
>> +               acpi_add_table(rsdp, madt);
>> +       }
>> +       current = (ALIGN(current, 16));
>> +
>> +       debug("ACPI:    * SSDT\n");
>> +       ssdt = (acpi_header_t *)current;
>> +       acpi_create_ssdt_generator(ssdt, ACPI_TABLE_CREATOR);
>> +       if (ssdt->length > sizeof(acpi_header_t)) {
>> +               current += ssdt->length;
>> +               acpi_add_table(rsdp, ssdt);
>> +               current = (ALIGN(current, 16));
>> +       }
>> +
>> +
>
> Nits: please remove one blank linke.
>

Addressed in patchset series v6.

>> +       debug("current = %lx\n", current);
>> +
>> +       debug("ACPI: done.\n");
>
> Nits: please add one blank line before return.
>

Addressed in patchset series v6.

>> +       return current;
>> +}
>> +
>> diff --git a/arch/x86/lib/tables.c b/arch/x86/lib/tables.c
>> index 75ffbc1..f15b2e2 100644
>> --- a/arch/x86/lib/tables.c
>> +++ b/arch/x86/lib/tables.c
>> @@ -8,6 +8,7 @@
>>  #include <asm/sfi.h>
>>  #include <asm/mpspec.h>
>>  #include <asm/tables.h>
>> +#include <asm/acpi_table.h>
>>
>>  u8 table_compute_checksum(void *v, int len)
>>  {
>> @@ -51,4 +52,8 @@ void write_tables(void)
>>         rom_table_end = write_mp_table(rom_table_end);
>>         rom_table_end = ALIGN(rom_table_end, 1024);
>>  #endif
>> +#ifdef CONFIG_GENERATE_ACPI_TABLE
>> +       rom_table_end = write_acpi_tables(rom_table_end);
>> +       rom_table_end = ALIGN(rom_table_end, 1024);
>> +#endif
>>  }
>> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
>> index 1c949fc..ed30bf5 100644
>> --- a/scripts/Makefile.lib
>> +++ b/scripts/Makefile.lib
>> @@ -297,6 +297,17 @@ $(obj)/%.dtb: $(src)/%.dts FORCE
>>
>>  dtc-tmp = $(subst $(comma),_,$(dot-target).dts.tmp)
>>
>> +# ACPI
>> +# ---------------------------------------------------------------------------
>> +quiet_cmd_acpi_c_asl= ASL     $@
>> +cmd_acpi_c_asl=         \
>> +       $(CPP) -x assembler-with-cpp -P -o $<.tmp $<; \
>> +       iasl -p $< -tc -va $<.tmp; \
>> +       mv $(patsubst %.asl,%.hex,$<) $@
>> +
>> +$(obj)/%.c:    $(src)/%.asl
>> +       $(call cmd,acpi_c_asl)
>> +
>>  # Bzip2
>>  # ---------------------------------------------------------------------------
>>
>> --
>
> Regards,
> Bin

Regards,
Saket Sinha



More information about the U-Boot mailing list