| Message ID | 20230130182225.2471414-14-sunilvl@ventanamicro.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Palmer Dabbelt |
| Headers | show |
| Series | Add basic ACPI support for RISC-V | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/tree_selection | fail | Failed to apply to next/pending-fixes or riscv/for-next |
On Mon, Jan 30, 2023 at 11:52:14PM +0530, Sunil V L wrote: > Add function to parse the RINTC structure in > the MADT table and create the required initializations to > enable SMP boot on ACPI based platforms. > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > --- > arch/riscv/include/asm/acpi.h | 7 ++++ > arch/riscv/kernel/smpboot.c | 73 ++++++++++++++++++++++++++++++++++- > 2 files changed, 79 insertions(+), 1 deletion(-) > > diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h > index c5cb9f96d404..d1f1e53ec657 100644 > --- a/arch/riscv/include/asm/acpi.h > +++ b/arch/riscv/include/asm/acpi.h > @@ -58,6 +58,13 @@ static inline bool acpi_has_cpu_in_madt(void) > } > > static inline void arch_fix_phys_package_id(int num, u32 slot) { } > + > +#ifdef CONFIG_ACPI_NUMA > +int acpi_numa_get_nid(unsigned int cpu); > +#else > +static inline int acpi_numa_get_nid(unsigned int cpu) { return NUMA_NO_NODE; } > +#endif /* CONFIG_ACPI_NUMA */ > + > #endif > > #endif /*_ASM_ACPI_H*/ > diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c > index 26214ddefaa4..e48cf88d0bc1 100644 > --- a/arch/riscv/kernel/smpboot.c > +++ b/arch/riscv/kernel/smpboot.c > @@ -8,6 +8,7 @@ > * Copyright (C) 2017 SiFive > */ > > +#include <linux/acpi.h> > #include <linux/arch_topology.h> > #include <linux/module.h> > #include <linux/init.h> > @@ -70,6 +71,73 @@ void __init smp_prepare_cpus(unsigned int max_cpus) > } > } > > +#ifdef CONFIG_ACPI > +static unsigned int cpu_count = 1; > + > +static int __init > +acpi_parse_rintc(union acpi_subtable_headers *header, > + const unsigned long end) This all fits on one line. And also avoids the checkpatch complaint from what you have currently done... > +{ > + unsigned long hart; > + bool found_boot_cpu = false; > + > + struct acpi_madt_rintc *processor; > + > + processor = (struct acpi_madt_rintc *)header; Why not combine the above two lines? > + /* RINTC entry which has !ACPI_MADT_ENABLED is not enabled so skip */ This comment is a bit -ENOPARSE. Please reword it in a way that is understandable to mere mortals like myself. > + if (!(processor->flags & ACPI_MADT_ENABLED)) > + return 0; > + > + hart = processor->hart_id; > + if (hart < 0) > + return 0; Newline here please > + if (hart == cpuid_to_hartid_map(0)) { > + BUG_ON(found_boot_cpu); > + found_boot_cpu = 1; This is a bool, why not assign a bool value to it so it looks more intentional? I know this is copied from the dt code, but that should really be on too IMO. > + early_map_cpu_to_node(0, acpi_numa_get_nid(cpu_count)); > + return 0; > + } And a newline here too... > + if (cpu_count >= NR_CPUS) { > + pr_warn("Invalid cpuid [%d] for hartid [%lu]\n", > + cpu_count, hart); > + return 0; > + } > + > + cpuid_to_hartid_map(cpu_count) = hart; > + early_map_cpu_to_node(cpu_count, acpi_numa_get_nid(cpu_count)); > + cpu_count++; ...and also here please! > + return 0; > +} > + > +static void __init acpi_parse_and_init_cpus(void) > +{ > + int cpuid; > + > + cpu_set_ops(0); While I'm at it suggesting newline additions, adding them before comments would be great too. > + /* > + * do a walk of MADT to determine how many CPUs > + * we have including disabled CPUs, and get information > + * we need for SMP init. > + */ > + acpi_table_parse_madt(ACPI_MADT_TYPE_RINTC, > + acpi_parse_rintc, 0); > + > + /* > + * NUMA - TODO > + */ TODO before merging, or TODO at some indeterminate point in the future? Anyways, this is all nits & this largely seem to resemble the dt code, so with the nits fixed (and an s/ACPI: // in $subject): Reviewed-by: Conor Dooley <conor.dooley@microchip.com> Cheers, Conor. > + for (cpuid = 1; cpuid < nr_cpu_ids; cpuid++) { > + if (cpuid_to_hartid_map(cpuid) != INVALID_HARTID) { > + cpu_set_ops(cpuid); > + set_cpu_possible(cpuid, true); > + } > + } > +} > +#else > +#define acpi_parse_and_init_cpus(...) do { } while (0) > +#endif > + > static void __init of_parse_and_init_cpus(void) > { > struct device_node *dn; > @@ -118,7 +186,10 @@ static void __init of_parse_and_init_cpus(void) > > void __init setup_smp(void) > { > - of_parse_and_init_cpus(); > + if (acpi_disabled) > + of_parse_and_init_cpus(); > + else > + acpi_parse_and_init_cpus(); > } > > static int start_secondary_cpu(int cpu, struct task_struct *tidle) > -- > 2.38.0 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Wed, Feb 08, 2023 at 10:10:14PM +0000, Conor Dooley wrote: > On Mon, Jan 30, 2023 at 11:52:14PM +0530, Sunil V L wrote: > > Add function to parse the RINTC structure in > > the MADT table and create the required initializations to > > enable SMP boot on ACPI based platforms. > > > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > > --- > > arch/riscv/include/asm/acpi.h | 7 ++++ > > arch/riscv/kernel/smpboot.c | 73 ++++++++++++++++++++++++++++++++++- > > 2 files changed, 79 insertions(+), 1 deletion(-) > > > > diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h > > index c5cb9f96d404..d1f1e53ec657 100644 > > --- a/arch/riscv/include/asm/acpi.h > > +++ b/arch/riscv/include/asm/acpi.h > > @@ -58,6 +58,13 @@ static inline bool acpi_has_cpu_in_madt(void) > > } > > > > static inline void arch_fix_phys_package_id(int num, u32 slot) { } > > + > > +#ifdef CONFIG_ACPI_NUMA > > +int acpi_numa_get_nid(unsigned int cpu); > > +#else > > +static inline int acpi_numa_get_nid(unsigned int cpu) { return NUMA_NO_NODE; } > > +#endif /* CONFIG_ACPI_NUMA */ > > + > > #endif > > > > #endif /*_ASM_ACPI_H*/ > > diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c > > index 26214ddefaa4..e48cf88d0bc1 100644 > > --- a/arch/riscv/kernel/smpboot.c > > +++ b/arch/riscv/kernel/smpboot.c > > @@ -8,6 +8,7 @@ > > * Copyright (C) 2017 SiFive > > */ > > > > +#include <linux/acpi.h> > > #include <linux/arch_topology.h> > > #include <linux/module.h> > > #include <linux/init.h> > > @@ -70,6 +71,73 @@ void __init smp_prepare_cpus(unsigned int max_cpus) > > } > > } > > > > +#ifdef CONFIG_ACPI > > +static unsigned int cpu_count = 1; > > + > > +static int __init > > +acpi_parse_rintc(union acpi_subtable_headers *header, > > + const unsigned long end) > > This all fits on one line. And also avoids the checkpatch complaint from > what you have currently done... > Okay. > > +{ > > + unsigned long hart; > > + bool found_boot_cpu = false; > > + > > + struct acpi_madt_rintc *processor; > > + > > + processor = (struct acpi_madt_rintc *)header; > > Why not combine the above two lines? > > > + /* RINTC entry which has !ACPI_MADT_ENABLED is not enabled so skip */ > > This comment is a bit -ENOPARSE. Please reword it in a way that is > understandable to mere mortals like myself. > Okay, let me try :-). > > + if (!(processor->flags & ACPI_MADT_ENABLED)) > > + return 0; > > + > > + hart = processor->hart_id; > > + if (hart < 0) > > + return 0; > > Newline here please > > > + if (hart == cpuid_to_hartid_map(0)) { > > + BUG_ON(found_boot_cpu); > > + found_boot_cpu = 1; > > This is a bool, why not assign a bool value to it so it looks more > intentional? I know this is copied from the dt code, but that should > really be on too IMO. > Okay. > > + early_map_cpu_to_node(0, acpi_numa_get_nid(cpu_count)); > > + return 0; > > + } > > And a newline here too... > Okay. > > + if (cpu_count >= NR_CPUS) { > > + pr_warn("Invalid cpuid [%d] for hartid [%lu]\n", > > + cpu_count, hart); > > + return 0; > > + } > > + > > + cpuid_to_hartid_map(cpu_count) = hart; > > + early_map_cpu_to_node(cpu_count, acpi_numa_get_nid(cpu_count)); > > + cpu_count++; > > ...and also here please! > Okay. > > + return 0; > > +} > > + > > +static void __init acpi_parse_and_init_cpus(void) > > +{ > > + int cpuid; > > + > > + cpu_set_ops(0); > > While I'm at it suggesting newline additions, adding them before > comments would be great too. > Okay. > > + /* > > + * do a walk of MADT to determine how many CPUs > > + * we have including disabled CPUs, and get information > > + * we need for SMP init. > > + */ > > + acpi_table_parse_madt(ACPI_MADT_TYPE_RINTC, > > + acpi_parse_rintc, 0); > > + > > + /* > > + * NUMA - TODO > > + */ > > TODO before merging, or TODO at some indeterminate point in the future? > Will remove this comment. We need to add NUMA after basic support is done. > Anyways, this is all nits & this largely seem to resemble the dt code, > so with the nits fixed (and an s/ACPI: // in $subject): > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > Thanks! Sunil
diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h index c5cb9f96d404..d1f1e53ec657 100644 --- a/arch/riscv/include/asm/acpi.h +++ b/arch/riscv/include/asm/acpi.h @@ -58,6 +58,13 @@ static inline bool acpi_has_cpu_in_madt(void) } static inline void arch_fix_phys_package_id(int num, u32 slot) { } + +#ifdef CONFIG_ACPI_NUMA +int acpi_numa_get_nid(unsigned int cpu); +#else +static inline int acpi_numa_get_nid(unsigned int cpu) { return NUMA_NO_NODE; } +#endif /* CONFIG_ACPI_NUMA */ + #endif #endif /*_ASM_ACPI_H*/ diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c index 26214ddefaa4..e48cf88d0bc1 100644 --- a/arch/riscv/kernel/smpboot.c +++ b/arch/riscv/kernel/smpboot.c @@ -8,6 +8,7 @@ * Copyright (C) 2017 SiFive */ +#include <linux/acpi.h> #include <linux/arch_topology.h> #include <linux/module.h> #include <linux/init.h> @@ -70,6 +71,73 @@ void __init smp_prepare_cpus(unsigned int max_cpus) } } +#ifdef CONFIG_ACPI +static unsigned int cpu_count = 1; + +static int __init +acpi_parse_rintc(union acpi_subtable_headers *header, + const unsigned long end) +{ + unsigned long hart; + bool found_boot_cpu = false; + + struct acpi_madt_rintc *processor; + + processor = (struct acpi_madt_rintc *)header; + + /* RINTC entry which has !ACPI_MADT_ENABLED is not enabled so skip */ + if (!(processor->flags & ACPI_MADT_ENABLED)) + return 0; + + hart = processor->hart_id; + if (hart < 0) + return 0; + if (hart == cpuid_to_hartid_map(0)) { + BUG_ON(found_boot_cpu); + found_boot_cpu = 1; + early_map_cpu_to_node(0, acpi_numa_get_nid(cpu_count)); + return 0; + } + if (cpu_count >= NR_CPUS) { + pr_warn("Invalid cpuid [%d] for hartid [%lu]\n", + cpu_count, hart); + return 0; + } + + cpuid_to_hartid_map(cpu_count) = hart; + early_map_cpu_to_node(cpu_count, acpi_numa_get_nid(cpu_count)); + cpu_count++; + return 0; +} + +static void __init acpi_parse_and_init_cpus(void) +{ + int cpuid; + + cpu_set_ops(0); + /* + * do a walk of MADT to determine how many CPUs + * we have including disabled CPUs, and get information + * we need for SMP init. + */ + acpi_table_parse_madt(ACPI_MADT_TYPE_RINTC, + acpi_parse_rintc, 0); + + /* + * NUMA - TODO + */ + + for (cpuid = 1; cpuid < nr_cpu_ids; cpuid++) { + if (cpuid_to_hartid_map(cpuid) != INVALID_HARTID) { + cpu_set_ops(cpuid); + set_cpu_possible(cpuid, true); + } + } +} +#else +#define acpi_parse_and_init_cpus(...) do { } while (0) +#endif + static void __init of_parse_and_init_cpus(void) { struct device_node *dn; @@ -118,7 +186,10 @@ static void __init of_parse_and_init_cpus(void) void __init setup_smp(void) { - of_parse_and_init_cpus(); + if (acpi_disabled) + of_parse_and_init_cpus(); + else + acpi_parse_and_init_cpus(); } static int start_secondary_cpu(int cpu, struct task_struct *tidle)
Add function to parse the RINTC structure in the MADT table and create the required initializations to enable SMP boot on ACPI based platforms. Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> --- arch/riscv/include/asm/acpi.h | 7 ++++ arch/riscv/kernel/smpboot.c | 73 ++++++++++++++++++++++++++++++++++- 2 files changed, 79 insertions(+), 1 deletion(-)