Re: [PATCH 04/12] SFI: create include/linux/sfi.h

Ingo Molnar
2009-07-10T06:49:19Z

* Len Brown <lenb@kernel.org> wrote:

> From: Feng Tang <feng.tang@intel.com>
>
> include/linux/include/sfi.h defines everything that customers
> of SFI need to know in order to use the SFI suport in the kernel.
>
> The primary API is sfi_table_parse(), where a driver or another part
> of the kernel can supply a handler to parse the named table.
>
> sfi.h also includes the currently defined table signatures and table formats.
>
> Signed-off-by: Feng Tang <feng.tang@intel.com>
> Signed-off-by: Len Brown <len.brown@intel.com>
> ---
> include/linux/sfi.h | 198 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 198 insertions(+), 0 deletions(-)
> create mode 100644 include/linux/sfi.h
>
> diff --git a/include/linux/sfi.h b/include/linux/sfi.h
> new file mode 100644
> index 0000000..f00f4da
> --- /dev/null
> +++ b/include/linux/sfi.h
> @@ -0,0 +1,198 @@
> +/* sfi.h Simple Firmware Interface */
> +
> +/*
> + * Copyright (C) 2009, Intel Corp.
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions, and the following disclaimer,
> + * without modification.
> + * 2. Redistributions in binary form must reproduce at minimum a disclaimer
> + * substantially similar to the "NO WARRANTY" disclaimer below
> + * ("Disclaimer") and any redistribution must be conditioned upon
> + * including a substantially similar Disclaimer requirement for further
> + * binary redistribution.
> + * 3. Neither the names of the above-listed copyright holders nor the names
> + * of any contributors may be used to endorse or promote products derived
> + * from this software without specific prior written permission.
> + *
> + * Alternatively, this software may be distributed under the terms of the
> + * GNU General Public License ("GPL") version 2 as published by the Free
> + * Software Foundation.
> + *
> + * NO WARRANTY
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * HOLDERS OR CONTRIBUTORS BE LIABLE FOR SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
> + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING
> + * IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGES.
> + */
> +
> +#ifndef _LINUX_SFI_H
> +#define _LINUX_SFI_H
> +
> +/* Table signatures reserved by the SFI specification */
> +#define SFI_SIG_SYST "SYST"
> +#define SFI_SIG_FREQ "FREQ"
> +#define SFI_SIG_IDLE "IDLE"
> +#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_SPIB "SPIB"
> +#define SFI_SIG_I2CB "I2CB"
> +#define SFI_SIG_GPEM "GPEM"

Sidenote: these strings, if used in different places, might be
instantiated in a duplicated way, bloating the kernel a tiny bit. It
might be better to do an intermediate const array and return
elements of them.

> +
> +#define SFI_ACPI_TABLE (1 << 0)
> +#define SFI_NORMAL_TABLE (1 << 1)
> +
> +#define SFI_SIGNATURE_SIZE 4
> +#define SFI_OEM_ID_SIZE 6
> +#define SFI_OEM_TABLE_ID_SIZE 8
> +
> +#define SFI_SYST_SEARCH_BEGIN 0x000E0000
> +#define SFI_SYST_SEARCH_END 0x000FFFFF

(see my suggestion about the proper type of these in my other mail)

> +
> +#define SFI_GET_NUM_ENTRIES(ptable, entry_type) \
> + ((ptable->header.length - sizeof(struct sfi_table_header)) / \
> + (sizeof(entry_type)))
> +/*
> + * Table structures must be byte-packed to match the SFI specification,
> + * as they are provided by the BIOS.
> + */
> +struct sfi_table_header {
> + char signature[SFI_SIGNATURE_SIZE];
> + u32 length;
> + u8 revision;
> + u8 checksum;

These could be abbreviated with common terms:

s/signature/sig
s/length/len
s/revision/rev
s/checksum/csum

> + char oem_id[SFI_OEM_ID_SIZE];
> + char oem_table_id[SFI_OEM_TABLE_ID_SIZE];
> +} __packed;
> +
> +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 phy_start;

s/phy_start/phys_start

> + u64 vir_start;

s/vir_start/virt_start

> + u64 pages;
> + u64 attrib;
> +} __packed;
> +
> +struct sfi_cpu_table_entry {
> + u32 apicid;

s/apicid/apic_id

(i realize that apicid is still the commonly used one in arch/x86,
but that's not a reason to continue that bad practice with new
code.)

> +} __packed;
> +
> +struct sfi_cstate_table_entry {
> + u32 hint; /* MWAIT hint */
> + u32 latency; /* latency in ms */
> +} __packed;
> +
> +struct sfi_apic_table_entry {
> + u64 phy_addr; /* phy base addr for APIC reg */

s/phy_addr/phys_addr

> +} __packed;
> +
> +struct sfi_freq_table_entry {
> + u32 freq;

is this in Hz or in kHz? (should be reflected in the name: say
freq_khz)

I suspect it's HZ here - but that's not unambiguous - often cpufreq
settings are in khz.

> + u32 latency; /* transition latency in ms */
> + u32 ctrl_val; /* value to write to PERF_CTL */

PERF_CTL is an u64 MSR.

> +} __packed;
> +
> +struct sfi_wake_table_entry {
> + u64 phy_addr; /* pointer to where the wake vector locates */

s/phy_addr/phys_addr

> +} __packed;
> +
> +struct sfi_timer_table_entry {
> + u64 phy_addr; /* phy base addr for the timer */

s/phy_addr/phys_addr

> + u32 freq; /* in HZ */

So we limit frequency to ~4 GHz?

> + u32 irq;
> +} __packed;
> +
> +struct sfi_rtc_table_entry {
> + u64 phy_addr; /* phy base addr for the RTC */

s/phy_addr/phys_addr

> + u32 irq;
> +} __packed;
> +
> +struct sfi_spi_table_entry {
> + u16 host_num; /* attached to host 0, 1...*/
> + u16 cs; /* chip select */
> + u16 irq_info;
> + char name[16];
> + u8 dev_info[10];
> +} __packed;
> +
> +struct sfi_i2c_table_entry {
> + u16 host_num;
> + u16 addr; /* slave addr */
> + u16 irq_info;
> + char name[16];
> + u8 dev_info[10];
> +} __packed;
> +
> +struct sfi_gpe_table_entry {
> + u16 logical_id; /* logical id */
> + u16 phy_id; /* physical GPE id */

s/phy_id/phys_id

> +} __packed;
> +
> +
> +typedef int (*sfi_table_handler) (struct sfi_table_header *table);
> +
> +#ifdef CONFIG_SFI

(stray tab.)

> +extern int __init sfi_init_memory_map(void);
> +extern void __init sfi_init(void);
> +extern int __init sfi_platform_init(void);
> +extern void __init sfi_init_late(void);
> +
> +int sfi_table_parse(char *signature, char *oem_id, char *oem_table_id,
> + uint flag, sfi_table_handler handler);

some prototypes come with externs ... some without. Please use one
variant.

> +
> +extern int sfi_disabled;
> +static inline void disable_sfi(void)
> +{
> + sfi_disabled = 1;
> +}
> +
> +#else /* !CONFIG_SFI */
> +
> +static inline int sfi_init_memory_map(void)
> +{
> + return -1;
> +}
> +
> +static inline void sfi_init(void)
> +{
> + return;
> +}

no need for that return.

Thanks,

Ingo