| Message ID | 20230404182037.863533-14-sunilvl@ventanamicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add basic ACPI support for RISC-V | expand |
On Wed, Apr 05, 2023 at 03:31:24PM +0100, Conor Dooley wrote: > On Wed, Apr 05, 2023 at 07:05:42PM +0530, Sunil V L wrote: > > On Tue, Apr 04, 2023 at 09:57:19PM +0100, Conor Dooley wrote: > > > On Tue, Apr 04, 2023 at 11:50:27PM +0530, Sunil V L wrote: ... > > > > - if (of_property_read_string(node, "riscv,isa", &isa)) { > > > > - pr_warn("Unable to find \"riscv,isa\" devicetree entry\n"); > > > > - continue; > > > > + if (acpi_disabled) { > > > > + node = of_cpu_device_node_get(cpu); > > > > + if (node) { > > > > + rc = of_property_read_string(node, "riscv,isa", &isa); > > > > > > Hmm, after digging in the previous patch, I think this is actually not > > > possible to fail? We already validated it when setting up the mask of > > > possible cpus, but I think leaving the error handling here makes things > > > a lot more obvious. > > > > > Yeah, do you prefer to merge these patches again since only in this > > patch, we change the loop to for_each_possible_cpu() from > > for_each_of_cpu_node() which actually makes riscv_of_processor_hartid() > > not useful? > > Yah, all 3 of us mistakenly thought that that was an unrelated cleanup > on the last revision, but clearly it is not. > Squash it back IMO, sorry for my part in the extra work generated. Yup, please squash back in. Sorry about that, Sunil! drew
Hey Sunil, On Tue, Apr 04, 2023 at 11:50:27PM +0530, Sunil V L wrote: > @@ -103,14 +109,36 @@ void __init riscv_fill_hwcap(void) > > bitmap_zero(riscv_isa, RISCV_ISA_EXT_MAX); > > - for_each_of_cpu_node(node) { > + if (!acpi_disabled) { > + status = acpi_get_table(ACPI_SIG_RHCT, 0, &rhct); > + if (ACPI_FAILURE(status)) > + return; > + } > + > + for_each_possible_cpu(cpu) { > unsigned long this_hwcap = 0; > DECLARE_BITMAP(this_isa, RISCV_ISA_EXT_MAX); > const char *temp; > > - if (of_property_read_string(node, "riscv,isa", &isa)) { > - pr_warn("Unable to find \"riscv,isa\" devicetree entry\n"); > - continue; > + if (acpi_disabled) { > + node = of_cpu_device_node_get(cpu); > + if (node) { > + rc = of_property_read_string(node, "riscv,isa", &isa); > + of_node_put(node); > + if (rc) { > + pr_warn("Unable to find \"riscv,isa\" devicetree entry\n"); > + continue; > + } > + } else { > + pr_warn("Unable to find cpu node\n"); > + continue; I was poking at this the last few days and went back to look at the ACPI code again. Is there a reason we don't do early-return here? IOW: node = of_cpu_device_node_get(cpu); if (!node) { pr_warn() continue; } rc = of_property_read_string(node, "riscv,isa", &isa); of_node_put(node); if (rc) { pr_warn(); continue; } Cheers, Conor. > + } > + } else { > + rc = acpi_get_riscv_isa(rhct, cpu, &isa); > + if (rc < 0) { > + pr_warn("Unable to get ISA for the hart - %d\n", cpu); > + continue; > + } > } > > temp = isa; > @@ -243,6 +271,9 @@ void __init riscv_fill_hwcap(void)
On Sat, Apr 29, 2023 at 11:31:20AM +0100, Conor Dooley wrote: > Hey Sunil, > > On Tue, Apr 04, 2023 at 11:50:27PM +0530, Sunil V L wrote: > > > @@ -103,14 +109,36 @@ void __init riscv_fill_hwcap(void) > > > > bitmap_zero(riscv_isa, RISCV_ISA_EXT_MAX); > > > > - for_each_of_cpu_node(node) { > > + if (!acpi_disabled) { > > + status = acpi_get_table(ACPI_SIG_RHCT, 0, &rhct); > > + if (ACPI_FAILURE(status)) > > + return; > > + } > > + > > + for_each_possible_cpu(cpu) { > > unsigned long this_hwcap = 0; > > DECLARE_BITMAP(this_isa, RISCV_ISA_EXT_MAX); > > const char *temp; > > > > - if (of_property_read_string(node, "riscv,isa", &isa)) { > > - pr_warn("Unable to find \"riscv,isa\" devicetree entry\n"); > > - continue; > > + if (acpi_disabled) { > > + node = of_cpu_device_node_get(cpu); > > + if (node) { > > + rc = of_property_read_string(node, "riscv,isa", &isa); > > + of_node_put(node); > > + if (rc) { > > + pr_warn("Unable to find \"riscv,isa\" devicetree entry\n"); > > + continue; > > + } > > + } else { > > + pr_warn("Unable to find cpu node\n"); > > + continue; > > I was poking at this the last few days and went back to look at the ACPI > code again. Is there a reason we don't do early-return here? IOW: > > node = of_cpu_device_node_get(cpu); > if (!node) { > pr_warn() > continue; > } > > rc = of_property_read_string(node, "riscv,isa", &isa); > of_node_put(node); > if (rc) { > pr_warn(); > continue; > } > This looks better. Will update when I send the next revision of the series. Thank you!, Conor.
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c index 63e56ce04162..5d2065b937e5 100644 --- a/arch/riscv/kernel/cpufeature.c +++ b/arch/riscv/kernel/cpufeature.c @@ -6,6 +6,7 @@ * Copyright (C) 2017 SiFive */ +#include <linux/acpi.h> #include <linux/bitmap.h> #include <linux/ctype.h> #include <linux/libfdt.h> @@ -13,6 +14,8 @@ #include <linux/memory.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/of_device.h> +#include <asm/acpi.h> #include <asm/alternative.h> #include <asm/cacheflush.h> #include <asm/errata_list.h> @@ -91,6 +94,9 @@ void __init riscv_fill_hwcap(void) char print_str[NUM_ALPHA_EXTS + 1]; int i, j, rc; unsigned long isa2hwcap[26] = {0}; + struct acpi_table_header *rhct; + acpi_status status; + unsigned int cpu; isa2hwcap['i' - 'a'] = COMPAT_HWCAP_ISA_I; isa2hwcap['m' - 'a'] = COMPAT_HWCAP_ISA_M; @@ -103,14 +109,36 @@ void __init riscv_fill_hwcap(void) bitmap_zero(riscv_isa, RISCV_ISA_EXT_MAX); - for_each_of_cpu_node(node) { + if (!acpi_disabled) { + status = acpi_get_table(ACPI_SIG_RHCT, 0, &rhct); + if (ACPI_FAILURE(status)) + return; + } + + for_each_possible_cpu(cpu) { unsigned long this_hwcap = 0; DECLARE_BITMAP(this_isa, RISCV_ISA_EXT_MAX); const char *temp; - if (of_property_read_string(node, "riscv,isa", &isa)) { - pr_warn("Unable to find \"riscv,isa\" devicetree entry\n"); - continue; + if (acpi_disabled) { + node = of_cpu_device_node_get(cpu); + if (node) { + rc = of_property_read_string(node, "riscv,isa", &isa); + of_node_put(node); + if (rc) { + pr_warn("Unable to find \"riscv,isa\" devicetree entry\n"); + continue; + } + } else { + pr_warn("Unable to find cpu node\n"); + continue; + } + } else { + rc = acpi_get_riscv_isa(rhct, cpu, &isa); + if (rc < 0) { + pr_warn("Unable to get ISA for the hart - %d\n", cpu); + continue; + } } temp = isa; @@ -243,6 +271,9 @@ void __init riscv_fill_hwcap(void) bitmap_and(riscv_isa, riscv_isa, this_isa, RISCV_ISA_EXT_MAX); } + if (!acpi_disabled && rhct) + acpi_put_table((struct acpi_table_header *)rhct); + /* We don't support systems with F but without D, so mask those out * here. */ if ((elf_hwcap & COMPAT_HWCAP_ISA_F) && !(elf_hwcap & COMPAT_HWCAP_ISA_D)) {