[U-Boot] [PATCH 07/20] x86: Add support for the Simple Firmware Interface (SFI)
Bin Meng
bmeng.cn at gmail.com
Tue Apr 28 08:51:05 CEST 2015
Hi Simon,
On Tue, Apr 28, 2015 at 6:48 AM, Simon Glass <sjg at chromium.org> wrote:
> This provides a way of passing information to Linux without requiring the
> full ACPI horror. Provide a rudimentary implementation sufficient to be
> recognised and parsed by Linux.
>
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
It's great to see we are adding more configuration tables support in U-Boot.
> arch/x86/Kconfig | 28 +++++++++
> arch/x86/lib/Makefile | 1 +
> arch/x86/lib/sfi.c | 171 ++++++++++++++++++++++++++++++++++++++++++++++++++
> arch/x86/lib/zimage.c | 7 +++
> include/linux/sfi.h | 139 ++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 346 insertions(+)
> create mode 100644 arch/x86/lib/sfi.c
> create mode 100644 include/linux/sfi.h
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index aaceaef..30a08ec 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -499,6 +499,34 @@ config PCIE_ECAM_BASE
> assigned to PCI devices - i.e. the memory and prefetch regions, as
> passed to pci_set_region().
>
> +config SFI
Can we rename this to "GENERATE_SFI_TABLE" under menu "System tables"
in arch/x86/Kconfig? See more comments below.
> + bool "SFI (Simple Firmware Interface) Support"
> + ---help---
I think just 'help', like other options.
> + The Simple Firmware Interface (SFI) provides a lightweight method
> + for platform firmware to pass information to the operating system
> + via static tables in memory. Kernel SFI support is required to
> + boot on SFI-only platforms. If you have ACPI tables then these are
> + used instead.
The indention is wrong for this paragraph.
> + For more information, see http://simplefirmware.org
> +
> + Say 'Y' here to enable the kernel to boot properly on SFI-only
> + platforms.
I guess this is from Linux kernel, so could be removed for U-Boot?
> +config SFI_BASE
We can just drop this config option, and let U-Boot calculate its
address in write_tables(). See more comments below.
> + hex "SFI base address (0xe0000 to 0xff000)"
> + default 0xe0000
> + depends on SFI
> + help
> + The OS searches addresses in the range 0xe00000 to 0xffff0 for the
> + Simple Firmware Interface (SFI) header. Use this option to determine
> + where the table will be placed. It must be a multiple of 16 bytes and
> + the header part (which U-Boot places at the end) must not cross a 4KB
> + boundary. A 4KB-aligned address is recommended for these reasons.
> +
> + U-Boot writes this table in sfi_write_tables() just before booting
> + the OS.
> +
> config BOOTSTAGE
> default y
>
> diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile
> index 0178fe1..c55b9be 100644
> --- a/arch/x86/lib/Makefile
> +++ b/arch/x86/lib/Makefile
> @@ -26,6 +26,7 @@ obj-y += pirq_routing.o
> obj-y += relocate.o
> obj-y += physmem.o
> obj-$(CONFIG_X86_RAMTEST) += ramtest.o
> +obj-$(CONFIG_SFI) += sfi.o
I think it is safe to say: obj-y += sfi.o
> obj-y += string.o
> obj-y += tables.o
> obj-$(CONFIG_SYS_X86_TSC_TIMER) += tsc_timer.o
> diff --git a/arch/x86/lib/sfi.c b/arch/x86/lib/sfi.c
> new file mode 100644
> index 0000000..060651b
> --- /dev/null
> +++ b/arch/x86/lib/sfi.c
> @@ -0,0 +1,171 @@
> +/*
> + * Copyright (c) 2015 Google, Inc
> + * Written by Simon Glass <sjg at chromium.org>
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +/*
> + * Intel Simple Firmware Interface (SFI)
> + *
> + * Yet another way to pass information to the Linux kernel.
> + *
> + * See https://simplefirmware.org/ for details
> + */
> +
> +#include <common.h>
> +#include <cpu.h>
> +#include <dm.h>
> +#include <asm/cpu.h>
> +#include <asm/ioapic.h>
> +#include <dm/uclass-internal.h>
> +#include <linux/sfi.h>
> +
> +struct table_info {
> + u32 base;
> + int ptr;
> + u32 entry_start;
> + u64 table[16];
I assume 16 is SFI_TABLE_MAX_ENTRIES?
> + int count;
> +};
> +
> +void *get_entry_start(struct table_info *tab)
Should this function be static?
> +{
> + if (tab->count == ARRAY_SIZE(tab->table))
Then we can just write: tab->count == SFI_TABLE_MAX_ENTRIES
> + return NULL;
> + tab->entry_start = tab->base + tab->ptr;
> + tab->table[tab->count] = tab->entry_start;
> + tab->entry_start += sizeof(struct sfi_table_header);
> +
> + return (void *)tab->entry_start;
> +}
> +
> +static void finish_table(struct table_info *tab, const char *sig, void *entry)
> +{
> + struct sfi_table_header *hdr;
> + uint8_t *p;
> + int sum;
> +
> + hdr = (struct sfi_table_header *)(tab->base + tab->ptr);
> + strcpy(hdr->sig, sig);
> + hdr->len = sizeof(*hdr) + ((ulong)entry - tab->entry_start);
> + hdr->rev = 1;
> + strncpy(hdr->oem_id, "U-Boot", SFI_OEM_ID_SIZE);
> + strncpy(hdr->oem_table_id, "Table v1", SFI_OEM_TABLE_ID_SIZE);
> + hdr->csum = 0;
> + for (sum = 0, p = (uint8_t *)hdr; (void *)p < entry; p++)
> + sum += *p;
> + hdr->csum = 256 - (sum & 255);
The above can be replaced to:
hdr->csum = 0;
hdr->csum = table_compute_checksum(hdr, hdr->len);
> + tab->ptr += hdr->len;
> + tab->ptr = ALIGN(tab->ptr, 16);
> + tab->count++;
> +}
> +
> +static int sfi_write_system_header(struct table_info *tab)
> +{
> + u64 *entry = get_entry_start(tab);
> + int i;
> +
> + if (!entry)
> + return -ENOSPC;
Need a blank line here.
> + for (i = 0; i < tab->count; i++)
> + *entry++ = tab->table[i];
> + finish_table(tab, SFI_SIG_SYST, entry);
> +
> + return 0;
> +}
> +
> +static int sfi_write_cpus(struct table_info *tab)
> +{
> + struct sfi_cpu_table_entry *entry = get_entry_start(tab);
> + struct udevice *dev;
> + int count = 0;
> +
> + if (!entry)
> + return -ENOSPC;
Need a blank line here.
> + for (uclass_find_first_device(UCLASS_CPU, &dev);
UCLASS_CPU is introduced in #8 of this series. Please reorder the patches.
> + dev;
> + uclass_find_next_device(&dev)) {
> + struct cpu_platdata *plat = dev_get_parent_platdata(dev);
> +
> + if (!device_active(dev))
> + continue;
> + entry->apic_id = plat->cpu_id;
> + entry++;
> + count++;
> + }
> +
> + /* Omit the table if there is only one CPU */
> + if (count > 1)
> + finish_table(tab, SFI_SIG_CPUS, entry);
> +
> + return 0;
> +}
> +
> +static int sfi_write_apic(struct table_info *tab)
> +{
> + struct sfi_apic_table_entry *entry = get_entry_start(tab);
> +
> + if (!entry)
> + return -ENOSPC;
> +
> + entry->phys_addr = IO_APIC_ADDR;
> + entry++;
> + finish_table(tab, SFI_SIG_APIC, entry);
> +
> + return 0;
> +}
> +
> +static int sfi_write_rtc(struct table_info *tab)
> +{
> + struct sfi_rtc_table_entry *entry = get_entry_start(tab);
> +
> + if (!entry)
> + return -ENOSPC;
> +
> + entry->phys_addr = CONFIG_SYS_ISA_IO_BASE_ADDRESS + 0x70;
> + entry->irq = 0; /* Should be 8? */
> + entry++;
> + finish_table(tab, SFI_SIG_MRTC, entry);
> +
> + return 0;
> +}
Is this RTC table a must-have that without it Linux cannot boot? If
not, I think we can leave it unimplemented.
> +static int sfi_write_xsdt(struct table_info *tab)
> +{
> + struct sfi_xsdt_header *entry = get_entry_start(tab);
> +
> + if (!entry)
> + return -ENOSPC;
> +
> + entry->oem_revision = 1;
> + entry->creator_id = 1;
> + entry->creator_revision = 1;
> + entry++;
> + finish_table(tab, SFI_SIG_XSDT, entry);
> +
> + return 0;
> +}
> +
> +int sfi_write_tables(void)
> +{
> + struct table_info table;
> +
> + table.base = CONFIG_SFI_BASE;
> + table.ptr = 0;
> + table.count = 0;
> + sfi_write_cpus(&table);
> + sfi_write_apic(&table);
> + sfi_write_rtc(&table);
> +
> + /*
> + * The SFI specification marks the XSDT table as option, but Linux 4.0
> + * crashes on start-up when it is not provided.
> + */
> + sfi_write_xsdt(&table);
> +
> + /* Finally, write out the system header which points to the others */
> + sfi_write_system_header(&table);
> +
> + return 0;
> +}
> diff --git a/arch/x86/lib/zimage.c b/arch/x86/lib/zimage.c
> index c3f8a73..d2c68f6 100644
> --- a/arch/x86/lib/zimage.c
> +++ b/arch/x86/lib/zimage.c
> @@ -24,6 +24,7 @@
> #include <asm/arch/timestamp.h>
> #endif
> #include <linux/compiler.h>
> +#include <linux/sfi.h>
>
> DECLARE_GLOBAL_DATA_PTR;
>
> @@ -232,9 +233,15 @@ int setup_zimage(struct boot_params *setup_base, char *cmd_line, int auto_boot,
> {
> struct setup_header *hdr = &setup_base->hdr;
> int bootproto = get_boot_protocol(hdr);
> + int ret;
>
> setup_base->e820_entries = install_e820_map(
> ARRAY_SIZE(setup_base->e820_map), setup_base->e820_map);
> + ret = sfi_write_tables();
> + if (ret && ret != -ENOSYS) {
> + printf("Failed to write SFI tables: err=%d\n", ret);
> + return ret;
> + }
Can we move this to arch/x86/lib/tables.c which is the central place
to write x86 configuration tables? Like this:
void write_tables(void)
{
u32 __maybe_unused rom_table_end = ROM_TABLE_ADDR;
#ifdef CONFIG_GENERATE_PIRQ_TABLE
rom_table_end = write_pirq_routing_table(rom_table_end);
rom_table_end = ALIGN(rom_table_end, 1024);
#endif
#ifdef CONFIG_GENERATE_SFI_TABLE
rom_table_end = write_sfi_table(rom_table_end);
rom_table_end = ALIGN(rom_table_end, 1024);
#endif
}
> if (bootproto == 0x0100) {
> setup_base->screen_info.cl_magic = COMMAND_LINE_MAGIC;
> diff --git a/include/linux/sfi.h b/include/linux/sfi.h
> new file mode 100644
> index 0000000..4094fc7
> --- /dev/null
> +++ b/include/linux/sfi.h
I believe it is better to put this file to arch/x86/include/asm/ as
this is x86-specific tables (like the apic stuff).
> @@ -0,0 +1,139 @@
> +/*
> + * Copyright(c) 2009 Intel Corporation. All rights reserved.
> + *
> + * SPDX-License-Identifier: GPL-2.0+ BSD-3-Clause
> + */
> +
> +#ifndef _LINUX_SFI_H
> +#define _LINUX_SFI_H
> +
> +#include <errno.h>
> +#include <linux/types.h>
> +
> +/* Table signatures reserved by the SFI specification */
> +#define SFI_SIG_SYST "SYST"
> +#define SFI_SIG_FREQ "FREQ"
> +#define SFI_SIG_IDLE "IDLE"
The SFI spec v0.8.2 deleted the "IDEL" table.
> +#define SFI_SIG_CPUS "CPUS"
> +#define SFI_SIG_MTMR "MTMR"
> +#define SFI_SIG_MRTC "MRTC"
> +#define SFI_SIG_MMAP "MMAP"
> +#define SFI_SIG_APIC "APIC"
> +#define SFI_SIG_XSDT "XSDT"
> +#define SFI_SIG_WAKE "WAKE"
> +#define SFI_SIG_DEVS "DEVS"
> +#define SFI_SIG_GPIO "GPIO"
> +
> +#define SFI_SIGNATURE_SIZE 4
> +#define SFI_OEM_ID_SIZE 6
> +#define SFI_OEM_TABLE_ID_SIZE 8
> +
> +#define SFI_NAME_LEN 16
> +
> +#define SFI_SYST_SEARCH_BEGIN 0x000e0000
> +#define SFI_SYST_SEARCH_END 0x000fffff
I think we can remove these two macros.
> +#define SFI_GET_NUM_ENTRIES(ptable, entry_type) \
> + ((ptable->header.len - sizeof(struct sfi_table_header)) / \
> + (sizeof(entry_type)))
> +
> +/* Table structures must be byte-packed to match the SFI specification */
> +struct sfi_table_header {
> + char sig[SFI_SIGNATURE_SIZE];
> + u32 len;
> + u8 rev;
> + u8 csum;
> + char oem_id[SFI_OEM_ID_SIZE];
> + char oem_table_id[SFI_OEM_TABLE_ID_SIZE];
> +} __packed;
IIRC, the checkpatch.pl will complain if __packed is put after the
structure defines. If that's true, please fix it globally.
> +struct sfi_table_simple {
> + struct sfi_table_header header;
> + u64 pentry[1];
> +} __packed;
> +
> +/* Comply with UEFI spec 2.1 */
> +struct sfi_mem_entry {
> + u32 type;
> + u64 phys_start;
> + u64 virt_start;
> + u64 pages;
> + u64 attrib;
> +} __packed;
> +
> +struct sfi_cpu_table_entry {
> + u32 apic_id;
> +} __packed;
> +
> +struct sfi_cstate_table_entry {
> + u32 hint; /* MWAIT hint */
> + u32 latency; /* latency in ms */
> +} __packed;
> +
> +struct sfi_apic_table_entry {
> + u64 phys_addr; /* phy base addr for APIC reg */
> +} __packed;
> +
> +struct sfi_freq_table_entry {
> + u32 freq_mhz; /* in MHZ */
> + u32 latency; /* transition latency in ms */
> + u32 ctrl_val; /* value to write to PERF_CTL */
> +} __packed;
> +
> +struct sfi_wake_table_entry {
> + u64 phys_addr; /* pointer to where the wake vector locates */
> +} __packed;
> +
> +struct sfi_timer_table_entry {
> + u64 phys_addr; /* phy base addr for the timer */
> + u32 freq_hz; /* in HZ */
> + u32 irq;
> +} __packed;
> +
> +struct sfi_rtc_table_entry {
> + u64 phys_addr; /* phy base addr for the RTC */
> + u32 irq;
> +} __packed;
> +
> +struct sfi_device_table_entry {
> + u8 type; /* bus type, I2C, SPI or ...*/
> +#define SFI_DEV_TYPE_SPI 0
> +#define SFI_DEV_TYPE_I2C 1
> +#define SFI_DEV_TYPE_UART 2
> +#define SFI_DEV_TYPE_HSI 3
> +#define SFI_DEV_TYPE_IPC 4
There is one more type added in SFI spec v0.8.2 which is SD. And
please move these defines out of the structure.
> + u8 host_num; /* attached to host 0, 1...*/
> + u16 addr;
> + u8 irq;
> + u32 max_freq;
> + char name[SFI_NAME_LEN];
> +} __packed;
> +
> +struct sfi_gpio_table_entry {
> + char controller_name[SFI_NAME_LEN];
> + u16 pin_no;
> + char pin_name[SFI_NAME_LEN];
> +} __packed;
> +
> +struct sfi_xsdt_header {
> + uint32_t oem_revision;
> + uint32_t creator_id;
> + uint32_t creator_revision;
> +};
> +
> +typedef int (*sfi_table_handler) (struct sfi_table_header *table);
> +
> +#ifdef CONFIG_SFI
There is no need to wrap this with #ifdefs.
> +/**
> + * sfi_write_tables() - Write Simple Firmware Interface tables
> + */
> +int sfi_write_tables(void);
> +
> +#else
> +
> +static inline int sfi_write_tables(void) { return -ENOSYS; }
> +
> +#endif
> +
> +#endif /*_LINUX_SFI_H */
> --
Regards,
Bin
More information about the U-Boot
mailing list