Hey, %subject: riscv: obtain ACPI RSDP from FFI. This subject is a bit unhelpful because FFI would commonly mean "foreign function interface" & you have not yet introduced it. It seems like it would be better to do s/FFI/devicetree/ or similar. Please also drop the full stop from the commit messages ;) Please use a cover letter for multi-patch series & include changelogs. +CC Sunil, Alex: Can you guys please take a look at this & see if it is something that we want to do (ACPI without EFI)? On Sun, Jul 02, 2023 at 05:57:32PM +0800, Yunhui Cui wrote: > 1. We need to enable the ACPI function on RISC-V. RISC-V already supports ACPI, the "we" in this commit message is confusing. Who is "we"? Bytedance? > When booting with > Coreboot, we encounter two problems: > a. Coreboot does not support EFI > b. On RISC-V, only the DTS channel can be used. We support ACPI, so this seems inaccurate. Could you explain it better please? > 2. Based on this, we have added an interface for obtaining firmware > information transfer through FDT, named FFI. Please use the long form of "FFI" before using the short form, since you are inventing this & noone knows what it means yet. > 3. We not only use FFI to pass ACPI RSDP, but also pass other > firmware information as an extension. This patch doesn't do that though? > +RISC-V FDT FIRMWARE INTERFACE (FFI) SUPPORT > +M: Yunhui Cui cuiyunhui@xxxxxxxxxxxxx > +S: Maintained > +F: arch/riscv/include/asm/ffi.h > +F: arch/riscv/kernel/ffi.c Please add this in alphabetical order, these entries have recently been resorted. That said, maintainers entry for a trivial file in arch code seems a wee bit odd, seems like it would be better suited rolled up into your other entry for the interface, like how Ard's one looks for EFI? > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > index b49793cf34eb..2e1c40fb2300 100644 > --- a/arch/riscv/Kconfig > +++ b/arch/riscv/Kconfig > @@ -785,6 +785,16 @@ config EFI > allow the kernel to be booted as an EFI application. This > is only useful on systems that have UEFI firmware. > > +config FFI > + bool "Fdt firmware interface" > + depends on OF > + default y > + help > + Added an interface to obtain firmware information transfer > + through FDT, named FFI. Some bootloaders do not support EFI, > + such as coreboot. > + We can pass firmware information through FFI, such as ACPI. I don't understand your Kconfig setup. Why don't you just have one option (the one from patch 2/3), instead of adding 2 different but similarly named options? > config CC_HAVE_STACKPROTECTOR_TLS > def_bool $(cc-option,-mstack-protector-guard=tls -mstack-protector-guard-reg=tp -mstack-protector-guard-offset=0) > > diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h > index f71ce21ff684..f9d1625dd159 100644 > --- a/arch/riscv/include/asm/acpi.h > +++ b/arch/riscv/include/asm/acpi.h > @@ -15,6 +15,8 @@ > /* Basic configuration for ACPI */ > #ifdef CONFIG_ACPI > > +#include <asm/ffi.h> > + > typedef u64 phys_cpuid_t; > #define PHYS_CPUID_INVALID INVALID_HARTID > > @@ -66,6 +68,13 @@ int acpi_get_riscv_isa(struct acpi_table_header *table, > unsigned int cpu, const char **isa); > > static inline int acpi_numa_get_nid(unsigned int cpu) { return NUMA_NO_NODE; } > + > +#define ACPI_HAVE_ARCH_GET_ROOT_POINTER How come this is not set in Kconfig like HAVE_FOO options usually are? > +static inline u64 acpi_arch_get_root_pointer(void) > +{ > + return acpi_rsdp; > +} > + > #else > static inline void acpi_init_rintc_map(void) { } > static inline struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu) > diff --git a/arch/riscv/include/asm/ffi.h b/arch/riscv/include/asm/ffi.h > new file mode 100644 > index 000000000000..847af02abd87 > --- /dev/null > +++ b/arch/riscv/include/asm/ffi.h > @@ -0,0 +1,9 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +#ifndef _ASM_FFI_H > +#define _ASM_FFI_H > + > +extern u64 acpi_rsdp; /stuff/linux/drivers/acpi/osl.c:178:22: error: redefinition of 'acpi_rsdp' with a different type: 'unsigned long' vs 'u64' (aka 'unsigned long long') Fails to build when Kexec is enabled. > +extern void ffi_init(void); > + > +#endif /* _ASM_FFI_H */ > diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile > index 506cc4a9a45a..274e06f4da33 100644 > --- a/arch/riscv/kernel/Makefile > +++ b/arch/riscv/kernel/Makefile > @@ -92,6 +92,7 @@ obj-$(CONFIG_CRASH_CORE) += crash_core.o > obj-$(CONFIG_JUMP_LABEL) += jump_label.o > > obj-$(CONFIG_EFI) += efi.o > +obj-$(CONFIG_FFI) += ffi.o This file uses tabs for alignment, not spaces. > obj-$(CONFIG_COMPAT) += compat_syscall_table.o > obj-$(CONFIG_COMPAT) += compat_signal.o > obj-$(CONFIG_COMPAT) += compat_vdso/ > diff --git a/arch/riscv/kernel/ffi.c b/arch/riscv/kernel/ffi.c > new file mode 100644 > index 000000000000..c5ac2b5d9148 > --- /dev/null > +++ b/arch/riscv/kernel/ffi.c > @@ -0,0 +1,37 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * ffi.c - FDT FIRMWARE INTERFACE > + */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/of.h> > +#include <linux/of_fdt.h> > +#include <linux/libfdt.h> > + > +u64 acpi_rsdp; > + > +void __init ffi_acpi_root_pointer(void) > +{ > + int cfgtbl, len; > + fdt64_t *prop; > + > + cfgtbl = fdt_path_offset(initial_boot_params, "/cfgtables"); > + if (cfgtbl < 0) { > + pr_info("firmware table not found.\n"); > + return; > + } > + > + prop = fdt_getprop_w(initial_boot_params, cfgtbl, "acpi_phy_ptr", &len); > + if (!prop || len != sizeof(u64)) > + pr_info("acpi_rsdp not found.\n"); > + else > + acpi_rsdp = fdt64_to_cpu(*prop); > + > + pr_debug("acpi rsdp: %llx\n", acpi_rsdp); Same comments here about undocumented DT properties and pr_*()s that likely are not wanted (or correct). > +} > + > +void __init ffi_init(void) > +{ > + ffi_acpi_root_pointer(); What happens if, on a system with "normal" ACPI support, ffi_init() is called & ffi_acpi_root_pointer() calls things like fdt_path_offset()? > +} > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > index 971fe776e2f8..5a933d6b6acb 100644 > --- a/arch/riscv/kernel/setup.c > +++ b/arch/riscv/kernel/setup.c > @@ -36,6 +36,7 @@ > #include <asm/thread_info.h> > #include <asm/kasan.h> > #include <asm/efi.h> > +#include <asm/ffi.h> > > #include "head.h" > > @@ -279,6 +280,7 @@ void __init setup_arch(char **cmdline_p) > parse_early_param(); > > efi_init(); > + ffi_init(); What provides ffi_init() if CONFIG_FFI is disabled? > paging_init(); > > /* Parse the ACPI tables for possible boot-time configuration */ Cheers, Conor.
Attachment:
signature.asc
Description: PGP signature