Re: [U-Boot] [PATCH 07/20] x86: Add support for the Simple Firmware Interface (SFI)

Hi Simon,

On Tue, Apr 28, 2015 at 6:48 AM, Simon Glass sjg@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@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@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