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

[Bin Meng <bmeng.cn@gmail.com>]

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