Re: [PATCH V4 13/23] RISC-V: cpufeature: Add ACPI support in riscv_fill_hwcap()

[Conor Dooley]

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:
> > > On ACPI based systems, the information about the hart
> > > like ISA is provided by the RISC-V Hart Capabilities Table (RHCT).
> > > Enable filling up hwcap structure based on the information in RHCT.
> > > 
> > > Signed-off-by: Sunil V L <sunilvl@xxxxxxxxxxxxxxxx>
> > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > > Reviewed-by: Andrew Jones <ajones@xxxxxxxxxxxxxxxx>
> > > ---
> > >  arch/riscv/kernel/cpufeature.c | 39 ++++++++++++++++++++++++++++++----
> > >  1 file changed, 35 insertions(+), 4 deletions(-)
> > > 
> > > 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);
> > 
> > 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.

Cheers,
Conor.

> 
> > I'd swear I gave you a (conditional) R-b on v3 though, no?
> > Reviewed-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>

Attachment: signature.asc
Description: PGP signature