On Mon, Feb 20, 2023 at 05:36:48PM +0100, Andrew Jones wrote: > On Thu, Feb 16, 2023 at 11:50:30PM +0530, Sunil V L wrote: > > RHCT is a new table defined for RISC-V to communicate the > > features of the CPU to the OS. Create a new architecture folder > > in drivers/acpi and add RHCT parsing code. > > > > Signed-off-by: Sunil V L <sunilvl@xxxxxxxxxxxxxxxx> > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx> > > --- > > arch/riscv/include/asm/acpi.h | 9 ++++ > > drivers/acpi/Makefile | 2 + > > drivers/acpi/riscv/Makefile | 2 + > > drivers/acpi/riscv/rhct.c | 92 +++++++++++++++++++++++++++++++++++ > > 4 files changed, 105 insertions(+) > > create mode 100644 drivers/acpi/riscv/Makefile > > create mode 100644 drivers/acpi/riscv/rhct.c > > > > diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h > > index 4a3622b38159..7bc49f65c86b 100644 > > --- a/arch/riscv/include/asm/acpi.h > > +++ b/arch/riscv/include/asm/acpi.h > > @@ -58,6 +58,15 @@ static inline bool acpi_has_cpu_in_madt(void) > > > > static inline void arch_fix_phys_package_id(int num, u32 slot) { } > > > > +int acpi_get_riscv_isa(struct acpi_table_header *table, > > + unsigned int cpu, const char **isa); > > +#else > > +static inline int acpi_get_riscv_isa(struct acpi_table_header *table, > > + unsigned int cpu, const char **isa) > > +{ > > + return -EINVAL; > > +} > > + > > #endif /* CONFIG_ACPI */ > > > > #endif /*_ASM_ACPI_H*/ > > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile > > index feb36c0b9446..3fc5a0d54f6e 100644 > > --- a/drivers/acpi/Makefile > > +++ b/drivers/acpi/Makefile > > @@ -131,3 +131,5 @@ obj-y += dptf/ > > obj-$(CONFIG_ARM64) += arm64/ > > > > obj-$(CONFIG_ACPI_VIOT) += viot.o > > + > > +obj-$(CONFIG_RISCV) += riscv/ > > diff --git a/drivers/acpi/riscv/Makefile b/drivers/acpi/riscv/Makefile > > new file mode 100644 > > index 000000000000..8b3b126e0b94 > > --- /dev/null > > +++ b/drivers/acpi/riscv/Makefile > > @@ -0,0 +1,2 @@ > > +# SPDX-License-Identifier: GPL-2.0-only > > +obj-y += rhct.o > > diff --git a/drivers/acpi/riscv/rhct.c b/drivers/acpi/riscv/rhct.c > > new file mode 100644 > > index 000000000000..5bafc236d627 > > --- /dev/null > > +++ b/drivers/acpi/riscv/rhct.c > > @@ -0,0 +1,92 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Copyright (C) 2022-2023, Ventana Micro Systems Inc > > + * Author: Sunil V L <sunilvl@xxxxxxxxxxxxxxxx> > > + * > > + */ > > + > > +#define pr_fmt(fmt) "ACPI: RHCT: " fmt > > + > > +#include <linux/acpi.h> > > + > > +static void acpi_rhct_warn_missing(void) > > +{ > > + pr_warn_once("No RHCT table found\n"); > > +} > > + > > +static struct acpi_table_header *acpi_get_rhct(void) > > +{ > > + static struct acpi_table_header *rhct; > > + acpi_status status; > > + > > + /* > > + * RHCT will be used at runtime on every CPU, so we > > + * don't need to call acpi_put_table() to release the table mapping. > > + */ > > + if (!rhct) { > > + status = acpi_get_table(ACPI_SIG_RHCT, 0, &rhct); > > + if (ACPI_FAILURE(status)) > > + acpi_rhct_warn_missing(); > > Probably don't need the wrapper function for this one callsite. Also, > returning NULL here, rather than relying on acpi_get_table() to set > rhct to NULL would be a bit more robust. > Sure. Will update. > > + } > > + > > + return rhct; > > +} > > + > > +/* > > + * During early boot, the caller should call acpi_get_table() and pass its pointer to > > + * these functions(and free up later). At run time, since this table can be used > > + * multiple times, pass NULL so that the table remains in memory > > ...multiple times, NULL may be passed in order to use the cached table. > Okay. > > + */ > > +int acpi_get_riscv_isa(struct acpi_table_header *table, unsigned int acpi_cpu_id, const char **isa) > > +{ > > + struct acpi_rhct_node_header *node, *ref_node, *end; > > + struct acpi_table_rhct *rhct; > > + struct acpi_rhct_hart_info *hart_info; > > + struct acpi_rhct_isa_string *isa_node; > > + u32 *hart_info_node_offset; > > + int i, j; > > + u32 size_hdr = sizeof(struct acpi_rhct_node_header); > > + u32 size_hartinfo = sizeof(struct acpi_rhct_hart_info); > > + > > + if (acpi_disabled) { > > + pr_debug("%s: acpi is disabled\n", __func__); > > + return -1; > > This seems like something that should never happen and easy to catch > and fix with a BUG_ON. Is there any chance that BUG'ing here would > be a bad idea? > Yes, we can use BUG_ON. Will update. > > + } > > + > > + if (!table) { > > + rhct = (struct acpi_table_rhct *)acpi_get_rhct(); > > + if (!rhct) > > + return -ENOENT; > > + } else { > > + rhct = (struct acpi_table_rhct *)table; > > + } > > + > > + node = ACPI_ADD_PTR(struct acpi_rhct_node_header, rhct, rhct->node_offset); > > + end = ACPI_ADD_PTR(struct acpi_rhct_node_header, rhct, rhct->header.length); > > + > > + for (i = 0; i < rhct->node_count; i++) { > > + if (node >= end) > > + break; > > for (node = ACPI_ADD_PTR(struct acpi_rhct_node_header, rhct, rhct->node_offset); > node < end; > node = ACPI_ADD_PTR(struct acpi_rhct_node_header, node node->length)) > > > + switch (node->type) { > > + case ACPI_RHCT_NODE_TYPE_HART_INFO: > > if (node->type == ACPI_RHCT_NODE_TYPE_HART_INFO) > > > + hart_info = ACPI_ADD_PTR(struct acpi_rhct_hart_info, node, size_hdr); > > + hart_info_node_offset = ACPI_ADD_PTR(u32, hart_info, size_hartinfo); > > + if (acpi_cpu_id != hart_info->uid) > > + break; > > With the above suggested changes, this 'break' becomes 'continue'. > Okay. > > + for (j = 0; j < hart_info->num_offsets; j++) { > > + ref_node = ACPI_ADD_PTR(struct acpi_rhct_node_header, > > + rhct, hart_info_node_offset[j]); > > + if (ref_node->type == ACPI_RHCT_NODE_TYPE_ISA_STRING) { > > + isa_node = ACPI_ADD_PTR(struct acpi_rhct_isa_string, > > + ref_node, size_hdr); > > + *isa = isa_node->isa; > > + return 0; > > + } > > + } > > + break; > > + } > > + node = ACPI_ADD_PTR(struct acpi_rhct_node_header, node, node->length); > > + } > > + > > + return -1; > > +} > > -- > > 2.34.1 > > > > Other than the nits, > > Reviewed-by: Andrew Jones <ajones@xxxxxxxxxxxxxxxx> > Thanks! Sunil