| Message ID | 1406206825-15590-11-git-send-email-hanjun.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Thu, Jul 24, 2014 at 02:00:16PM +0100, Hanjun Guo wrote: > ACPI 5.1 only has two explicit methods to boot up SMP, > PSCI and Parking protocol, but the Parking protocol is > only suitable for ARMv7 now, so make PSCI as the only way > for the SMP boot protocol before some updates for the > ACPI spec or the Parking protocol spec. > > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org> > --- > arch/arm64/include/asm/acpi.h | 21 +++++++++++++++ > arch/arm64/include/asm/cpu_ops.h | 9 ++++++- > arch/arm64/include/asm/smp.h | 2 +- > arch/arm64/kernel/acpi.c | 9 +++++++ > arch/arm64/kernel/cpu_ops.c | 52 ++++++++++++++++++++++++++++++++++---- > arch/arm64/kernel/smp.c | 29 +++++++++++++++++++-- > 6 files changed, 113 insertions(+), 9 deletions(-) > > diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h > index 5ce85f8..6240327 100644 > --- a/arch/arm64/include/asm/acpi.h > +++ b/arch/arm64/include/asm/acpi.h > @@ -14,6 +14,27 @@ > > /* Basic configuration for ACPI */ > #ifdef CONFIG_ACPI > +/* > + * ACPI 5.1 only has two explicit methods to > + * boot up SMP, PSCI and Parking protocol, > + * but the Parking protocol is only defined > + * for ARMv7 now, so make PSCI as the only > + * way for the SMP boot protocol before some > + * updates for the ACPI spec or the Parking > + * protocol spec. > + * > + * This enum is intend to make the boot method > + * scalable when above updates are happended, > + * which NOT means to support all of them. > + */ > +enum acpi_smp_boot_protocol { > + ACPI_SMP_BOOT_PSCI, > + ACPI_SMP_BOOT_PARKING_PROTOCOL, > + ACPI_SMP_BOOT_PROTOCOL_MAX > +}; > + > +enum acpi_smp_boot_protocol smp_boot_protocol(void); > + > extern int acpi_disabled; > extern int acpi_noirq; > extern int acpi_pci_disabled; > diff --git a/arch/arm64/include/asm/cpu_ops.h b/arch/arm64/include/asm/cpu_ops.h > index d7b4b38..2a7c6fd 100644 > --- a/arch/arm64/include/asm/cpu_ops.h > +++ b/arch/arm64/include/asm/cpu_ops.h > @@ -61,7 +61,14 @@ struct cpu_operations { > }; > > extern const struct cpu_operations *cpu_ops[NR_CPUS]; > -extern int __init cpu_read_ops(struct device_node *dn, int cpu); > +extern int __init cpu_of_read_ops(struct device_node *dn, int cpu); > + > +#ifdef CONFIG_ACPI > +extern int __init cpu_acpi_read_ops(int cpu); > +#else > +static inline int __init cpu_acpi_read_ops(int cpu) { return -ENODEV; } > +#endif > + > extern void __init cpu_read_bootcpu_ops(void); > > #endif /* ifndef __ASM_CPU_OPS_H */ > diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h > index a498f2c..a5cea56 100644 > --- a/arch/arm64/include/asm/smp.h > +++ b/arch/arm64/include/asm/smp.h > @@ -39,7 +39,7 @@ extern void show_ipi_list(struct seq_file *p, int prec); > extern void handle_IPI(int ipinr, struct pt_regs *regs); > > /* > - * Setup the set of possible CPUs (via set_cpu_possible) > + * Platform specific SMP operations > */ > extern void smp_init_cpus(void); While the originial comment is out of date, the new form is plain wrong. Something like "Discover the set of possible CPUs and determine their SMP operations" would be a better description of what's going on here. > > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c > index ff0f6a0..2af6662 100644 > --- a/arch/arm64/kernel/acpi.c > +++ b/arch/arm64/kernel/acpi.c > @@ -184,6 +184,15 @@ static int __init acpi_parse_madt_gic_cpu_interface_entries(void) > return 0; > } > > +/* Protocol to bring up secondary CPUs */ > +enum acpi_smp_boot_protocol smp_boot_protocol(void) > +{ > + if (acpi_psci_present) > + return ACPI_SMP_BOOT_PSCI; > + else > + return ACPI_SMP_BOOT_PARKING_PROTOCOL; > +} > + > static int __init acpi_parse_fadt(struct acpi_table_header *table) > { > struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table; > diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c > index d62d12f..4d9b3cf 100644 > --- a/arch/arm64/kernel/cpu_ops.c > +++ b/arch/arm64/kernel/cpu_ops.c > @@ -16,11 +16,13 @@ > * along with this program. If not, see <http://www.gnu.org/licenses/>. > */ > > -#include <asm/cpu_ops.h> > -#include <asm/smp_plat.h> > #include <linux/errno.h> > #include <linux/of.h> > #include <linux/string.h> > +#include <linux/acpi.h> > + > +#include <asm/cpu_ops.h> > +#include <asm/smp_plat.h> Was the header move just for consistency with other files, or is there some ordering requirement here? > > extern const struct cpu_operations smp_spin_table_ops; > extern const struct cpu_operations cpu_psci_ops; > @@ -52,7 +54,7 @@ static const struct cpu_operations * __init cpu_get_ops(const char *name) > /* > * Read a cpu's enable method from the device tree and record it in cpu_ops. > */ > -int __init cpu_read_ops(struct device_node *dn, int cpu) > +int __init cpu_of_read_ops(struct device_node *dn, int cpu) > { > const char *enable_method = of_get_property(dn, "enable-method", NULL); > if (!enable_method) { > @@ -76,12 +78,52 @@ int __init cpu_read_ops(struct device_node *dn, int cpu) > return 0; > } > > +#ifdef CONFIG_ACPI > +/* > + * Read a cpu's enable method in the ACPI way and record it in cpu_ops. > + */ > +int __init cpu_acpi_read_ops(int cpu) > +{ > + /* > + * For ACPI 5.1, only two kind of methods are provided, > + * Parking protocol and PSCI, but Parking protocol is > + * used on ARMv7 only, so make PSCI as the only method > + * for SMP initialization before the ACPI spec or Parking > + * protocol spec is updated. > + */ That comment is a little misleading. The parking protocol is _specified_ for ARMv7 only. > + switch (smp_boot_protocol()) { > + case ACPI_SMP_BOOT_PSCI: > + cpu_ops[cpu] = cpu_get_ops("psci"); > + break; > + case ACPI_SMP_BOOT_PARKING_PROTOCOL: > + default: > + cpu_ops[cpu] = NULL; > + break; > + } > + > + if (!cpu_ops[cpu]) { > + pr_warn("CPU %d: unsupported enable-method, only PSCI is supported\n", > + cpu); > + return -EOPNOTSUPP; > + } That's going to require changes as things get updated, and "enable-method" is a term from DT rather than ACPI. How about: "CPU%d: boot protocol unsupported or unknown\n". Cheers, Mark.
On 2014-7-24 23:47, Mark Rutland wrote: > On Thu, Jul 24, 2014 at 02:00:16PM +0100, Hanjun Guo wrote: >> ACPI 5.1 only has two explicit methods to boot up SMP, >> PSCI and Parking protocol, but the Parking protocol is >> only suitable for ARMv7 now, so make PSCI as the only way >> for the SMP boot protocol before some updates for the >> ACPI spec or the Parking protocol spec. >> >> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org> >> --- >> arch/arm64/include/asm/acpi.h | 21 +++++++++++++++ >> arch/arm64/include/asm/cpu_ops.h | 9 ++++++- >> arch/arm64/include/asm/smp.h | 2 +- >> arch/arm64/kernel/acpi.c | 9 +++++++ >> arch/arm64/kernel/cpu_ops.c | 52 ++++++++++++++++++++++++++++++++++---- >> arch/arm64/kernel/smp.c | 29 +++++++++++++++++++-- >> 6 files changed, 113 insertions(+), 9 deletions(-) [...] >> /* >> - * Setup the set of possible CPUs (via set_cpu_possible) >> + * Platform specific SMP operations >> */ >> extern void smp_init_cpus(void); > > While the originial comment is out of date, the new form is plain wrong. > > Something like "Discover the set of possible CPUs and determine their > SMP operations" would be a better description of what's going on here. Agreed. > >> >> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c >> index ff0f6a0..2af6662 100644 >> --- a/arch/arm64/kernel/acpi.c >> +++ b/arch/arm64/kernel/acpi.c >> @@ -184,6 +184,15 @@ static int __init acpi_parse_madt_gic_cpu_interface_entries(void) >> return 0; >> } >> >> +/* Protocol to bring up secondary CPUs */ >> +enum acpi_smp_boot_protocol smp_boot_protocol(void) >> +{ >> + if (acpi_psci_present) >> + return ACPI_SMP_BOOT_PSCI; >> + else >> + return ACPI_SMP_BOOT_PARKING_PROTOCOL; >> +} >> + >> static int __init acpi_parse_fadt(struct acpi_table_header *table) >> { >> struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table; >> diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c >> index d62d12f..4d9b3cf 100644 >> --- a/arch/arm64/kernel/cpu_ops.c >> +++ b/arch/arm64/kernel/cpu_ops.c >> @@ -16,11 +16,13 @@ >> * along with this program. If not, see <http://www.gnu.org/licenses/>. >> */ >> >> -#include <asm/cpu_ops.h> >> -#include <asm/smp_plat.h> >> #include <linux/errno.h> >> #include <linux/of.h> >> #include <linux/string.h> >> +#include <linux/acpi.h> >> + >> +#include <asm/cpu_ops.h> >> +#include <asm/smp_plat.h> > > Was the header move just for consistency with other files, or is there > some ordering requirement here? Ordering requirement, it will meet compile error for "return -ENODEV;" in the head file which I introduced, I know I can include the right head file in asm/cpu_ops.h, but I think move the asm/xx.h after the include of linux/xx.h would be fine. > >> >> extern const struct cpu_operations smp_spin_table_ops; >> extern const struct cpu_operations cpu_psci_ops; >> @@ -52,7 +54,7 @@ static const struct cpu_operations * __init cpu_get_ops(const char *name) >> /* >> * Read a cpu's enable method from the device tree and record it in cpu_ops. >> */ >> -int __init cpu_read_ops(struct device_node *dn, int cpu) >> +int __init cpu_of_read_ops(struct device_node *dn, int cpu) >> { >> const char *enable_method = of_get_property(dn, "enable-method", NULL); >> if (!enable_method) { >> @@ -76,12 +78,52 @@ int __init cpu_read_ops(struct device_node *dn, int cpu) >> return 0; >> } >> >> +#ifdef CONFIG_ACPI >> +/* >> + * Read a cpu's enable method in the ACPI way and record it in cpu_ops. >> + */ >> +int __init cpu_acpi_read_ops(int cpu) >> +{ >> + /* >> + * For ACPI 5.1, only two kind of methods are provided, >> + * Parking protocol and PSCI, but Parking protocol is >> + * used on ARMv7 only, so make PSCI as the only method >> + * for SMP initialization before the ACPI spec or Parking >> + * protocol spec is updated. >> + */ > > That comment is a little misleading. The parking protocol is _specified_ > for ARMv7 only. Agreed. But I'm just wondering if the Parking protocol is modified and then support ARMv8, how to describe it in the comments? > >> + switch (smp_boot_protocol()) { >> + case ACPI_SMP_BOOT_PSCI: >> + cpu_ops[cpu] = cpu_get_ops("psci"); >> + break; >> + case ACPI_SMP_BOOT_PARKING_PROTOCOL: >> + default: >> + cpu_ops[cpu] = NULL; >> + break; >> + } >> + >> + if (!cpu_ops[cpu]) { >> + pr_warn("CPU %d: unsupported enable-method, only PSCI is supported\n", >> + cpu); >> + return -EOPNOTSUPP; >> + } > > That's going to require changes as things get updated, and > "enable-method" is a term from DT rather than ACPI. > > How about: > > "CPU%d: boot protocol unsupported or unknown\n". Pretty fine to me :) Thanks Hanjun
On Fri, Jul 25, 2014 at 11:51:37AM +0100, Hanjun Guo wrote: > On 2014-7-24 23:47, Mark Rutland wrote: > > On Thu, Jul 24, 2014 at 02:00:16PM +0100, Hanjun Guo wrote: > >> ACPI 5.1 only has two explicit methods to boot up SMP, > >> PSCI and Parking protocol, but the Parking protocol is > >> only suitable for ARMv7 now, so make PSCI as the only way > >> for the SMP boot protocol before some updates for the > >> ACPI spec or the Parking protocol spec. > >> > >> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > >> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org> > >> --- > >> arch/arm64/include/asm/acpi.h | 21 +++++++++++++++ > >> arch/arm64/include/asm/cpu_ops.h | 9 ++++++- > >> arch/arm64/include/asm/smp.h | 2 +- > >> arch/arm64/kernel/acpi.c | 9 +++++++ > >> arch/arm64/kernel/cpu_ops.c | 52 ++++++++++++++++++++++++++++++++++---- > >> arch/arm64/kernel/smp.c | 29 +++++++++++++++++++-- > >> 6 files changed, 113 insertions(+), 9 deletions(-) > [...] > >> /* > >> - * Setup the set of possible CPUs (via set_cpu_possible) > >> + * Platform specific SMP operations > >> */ > >> extern void smp_init_cpus(void); > > > > While the originial comment is out of date, the new form is plain wrong. > > > > Something like "Discover the set of possible CPUs and determine their > > SMP operations" would be a better description of what's going on here. > > Agreed. Ok. > > > >> > >> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c > >> index ff0f6a0..2af6662 100644 > >> --- a/arch/arm64/kernel/acpi.c > >> +++ b/arch/arm64/kernel/acpi.c > >> @@ -184,6 +184,15 @@ static int __init acpi_parse_madt_gic_cpu_interface_entries(void) > >> return 0; > >> } > >> > >> +/* Protocol to bring up secondary CPUs */ > >> +enum acpi_smp_boot_protocol smp_boot_protocol(void) > >> +{ > >> + if (acpi_psci_present) > >> + return ACPI_SMP_BOOT_PSCI; > >> + else > >> + return ACPI_SMP_BOOT_PARKING_PROTOCOL; > >> +} > >> + > >> static int __init acpi_parse_fadt(struct acpi_table_header *table) > >> { > >> struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table; > >> diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c > >> index d62d12f..4d9b3cf 100644 > >> --- a/arch/arm64/kernel/cpu_ops.c > >> +++ b/arch/arm64/kernel/cpu_ops.c > >> @@ -16,11 +16,13 @@ > >> * along with this program. If not, see <http://www.gnu.org/licenses/>. > >> */ > >> > >> -#include <asm/cpu_ops.h> > >> -#include <asm/smp_plat.h> > >> #include <linux/errno.h> > >> #include <linux/of.h> > >> #include <linux/string.h> > >> +#include <linux/acpi.h> > >> + > >> +#include <asm/cpu_ops.h> > >> +#include <asm/smp_plat.h> > > > > Was the header move just for consistency with other files, or is there > > some ordering requirement here? > > Ordering requirement, it will meet compile error for "return -ENODEV;" > in the head file which I introduced, I know I can include the right > head file in asm/cpu_ops.h, but I think move the asm/xx.h after the > include of linux/xx.h would be fine. If a file uses something defined in a header, it should include that header. So if you need ENODEV in cpu_ops.h, make cpu_ops.h include <linux/errno.h>. It is not fine to deliberately rely on implicit includes. Moving the headers around is a separate issue. I have no problem with doing that for consistency with other files. > > > > >> > >> extern const struct cpu_operations smp_spin_table_ops; > >> extern const struct cpu_operations cpu_psci_ops; > >> @@ -52,7 +54,7 @@ static const struct cpu_operations * __init cpu_get_ops(const char *name) > >> /* > >> * Read a cpu's enable method from the device tree and record it in cpu_ops. > >> */ > >> -int __init cpu_read_ops(struct device_node *dn, int cpu) > >> +int __init cpu_of_read_ops(struct device_node *dn, int cpu) > >> { > >> const char *enable_method = of_get_property(dn, "enable-method", NULL); > >> if (!enable_method) { > >> @@ -76,12 +78,52 @@ int __init cpu_read_ops(struct device_node *dn, int cpu) > >> return 0; > >> } > >> > >> +#ifdef CONFIG_ACPI > >> +/* > >> + * Read a cpu's enable method in the ACPI way and record it in cpu_ops. > >> + */ > >> +int __init cpu_acpi_read_ops(int cpu) > >> +{ > >> + /* > >> + * For ACPI 5.1, only two kind of methods are provided, > >> + * Parking protocol and PSCI, but Parking protocol is > >> + * used on ARMv7 only, so make PSCI as the only method > >> + * for SMP initialization before the ACPI spec or Parking > >> + * protocol spec is updated. > >> + */ > > > > That comment is a little misleading. The parking protocol is _specified_ > > for ARMv7 only. > > Agreed. But I'm just wondering if the Parking protocol is modified and > then support ARMv8, how to describe it in the comments? We update the comments if and when that happens. > > > >> + switch (smp_boot_protocol()) { > >> + case ACPI_SMP_BOOT_PSCI: > >> + cpu_ops[cpu] = cpu_get_ops("psci"); > >> + break; > >> + case ACPI_SMP_BOOT_PARKING_PROTOCOL: > >> + default: > >> + cpu_ops[cpu] = NULL; > >> + break; > >> + } > >> + > >> + if (!cpu_ops[cpu]) { > >> + pr_warn("CPU %d: unsupported enable-method, only PSCI is supported\n", > >> + cpu); > >> + return -EOPNOTSUPP; > >> + } > > > > That's going to require changes as things get updated, and > > "enable-method" is a term from DT rather than ACPI. > > > > How about: > > > > "CPU%d: boot protocol unsupported or unknown\n". > > Pretty fine to me :) Ok. Cheers, Mark.
On 2014-7-25 20:24, Mark Rutland wrote: > On Fri, Jul 25, 2014 at 11:51:37AM +0100, Hanjun Guo wrote: >> On 2014-7-24 23:47, Mark Rutland wrote: >>> On Thu, Jul 24, 2014 at 02:00:16PM +0100, Hanjun Guo wrote: >>>> ACPI 5.1 only has two explicit methods to boot up SMP, >>>> PSCI and Parking protocol, but the Parking protocol is >>>> only suitable for ARMv7 now, so make PSCI as the only way >>>> for the SMP boot protocol before some updates for the >>>> ACPI spec or the Parking protocol spec. >>>> [...] >>>> -#include <asm/cpu_ops.h> >>>> -#include <asm/smp_plat.h> >>>> #include <linux/errno.h> >>>> #include <linux/of.h> >>>> #include <linux/string.h> >>>> +#include <linux/acpi.h> >>>> + >>>> +#include <asm/cpu_ops.h> >>>> +#include <asm/smp_plat.h> >>> >>> Was the header move just for consistency with other files, or is there >>> some ordering requirement here? >> >> Ordering requirement, it will meet compile error for "return -ENODEV;" >> in the head file which I introduced, I know I can include the right >> head file in asm/cpu_ops.h, but I think move the asm/xx.h after the >> include of linux/xx.h would be fine. > > If a file uses something defined in a header, it should include that > header. So if you need ENODEV in cpu_ops.h, make cpu_ops.h include > <linux/errno.h>. It is not fine to deliberately rely on implicit > includes. ok, will fix that in next version. > > Moving the headers around is a separate issue. I have no problem with > doing that for consistency with other files. > >> >>> >>>> >>>> extern const struct cpu_operations smp_spin_table_ops; >>>> extern const struct cpu_operations cpu_psci_ops; >>>> @@ -52,7 +54,7 @@ static const struct cpu_operations * __init cpu_get_ops(const char *name) >>>> /* >>>> * Read a cpu's enable method from the device tree and record it in cpu_ops. >>>> */ >>>> -int __init cpu_read_ops(struct device_node *dn, int cpu) >>>> +int __init cpu_of_read_ops(struct device_node *dn, int cpu) >>>> { >>>> const char *enable_method = of_get_property(dn, "enable-method", NULL); >>>> if (!enable_method) { >>>> @@ -76,12 +78,52 @@ int __init cpu_read_ops(struct device_node *dn, int cpu) >>>> return 0; >>>> } >>>> >>>> +#ifdef CONFIG_ACPI >>>> +/* >>>> + * Read a cpu's enable method in the ACPI way and record it in cpu_ops. >>>> + */ >>>> +int __init cpu_acpi_read_ops(int cpu) >>>> +{ >>>> + /* >>>> + * For ACPI 5.1, only two kind of methods are provided, >>>> + * Parking protocol and PSCI, but Parking protocol is >>>> + * used on ARMv7 only, so make PSCI as the only method >>>> + * for SMP initialization before the ACPI spec or Parking >>>> + * protocol spec is updated. >>>> + */ >>> >>> That comment is a little misleading. The parking protocol is _specified_ >>> for ARMv7 only. >> >> Agreed. But I'm just wondering if the Parking protocol is modified and >> then support ARMv8, how to describe it in the comments? > > We update the comments if and when that happens. Ok. Thanks Hanjun
Hi, On Thu, Jul 24, 2014 at 09:00:16PM +0800, Hanjun Guo wrote: > +/* > + * In ACPI mode, the cpu possible map was enumerated before SMP > + * initialization when MADT table was parsed, so we can get the > + * possible map here to initialize CPUs. > + */ The DT smp init will warn if the kernel has been build with too low NR_CPUS. Does the ACPI core already warn, or did that go missing with this separate code path? > +static void __init acpi_smp_init_cpus(void) > +{ > + int cpu; > + > + for_each_possible_cpu(cpu) { > + if (cpu_acpi_read_ops(cpu) != 0) > + continue; > + > + cpu_ops[cpu]->cpu_init(NULL, cpu); > + } > +} > + > +void __init smp_init_cpus(void) > +{ > + if (acpi_disabled) > + of_smp_init_cpus(); > + else > + acpi_smp_init_cpus(); I'm liking these deeply split code paths less and less every time I see them. :( I would prefer to set up shared state in separate functions, but keep the control flow the same. Right now you're splitting it completely. I.e. split data setup between the two, but do the loop calling cpu_init() the same way. (Yes, that will require you to refactor the DT code path a bit too...) -Olof -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 2014-7-31 14:54, Olof Johansson wrote: > Hi, > > On Thu, Jul 24, 2014 at 09:00:16PM +0800, Hanjun Guo wrote: >> +/* >> + * In ACPI mode, the cpu possible map was enumerated before SMP >> + * initialization when MADT table was parsed, so we can get the >> + * possible map here to initialize CPUs. >> + */ > > The DT smp init will warn if the kernel has been build with too low NR_CPUS. > Does the ACPI core already warn, or did that go missing with this separate code > path? ACPI code will warn, it is in PATCH 07/19, + if (enabled_cpus >= NR_CPUS) { + pr_warn("NR_CPUS limit of %d reached, Processor %d/0x%llx ignored.\n", + NR_CPUS, total_cpus, mpidr); + return -EINVAL; + } > >> +static void __init acpi_smp_init_cpus(void) >> +{ >> + int cpu; >> + >> + for_each_possible_cpu(cpu) { >> + if (cpu_acpi_read_ops(cpu) != 0) >> + continue; >> + >> + cpu_ops[cpu]->cpu_init(NULL, cpu); >> + } >> +} >> + >> +void __init smp_init_cpus(void) >> +{ >> + if (acpi_disabled) >> + of_smp_init_cpus(); >> + else >> + acpi_smp_init_cpus(); > > I'm liking these deeply split code paths less and less every time I see > them. :( > > I would prefer to set up shared state in separate functions, but keep the > control flow the same. Right now you're splitting it completely. > > I.e. split data setup between the two, but do the loop calling cpu_init() > the same way. (Yes, that will require you to refactor the DT code path > a bit too...) OK, I will dive into the code and figure out if I can fix that as you suggested, thanks for your comments :) Best Regards Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hanjun, On Thu, 2014-07-24 at 21:00 +0800, Hanjun Guo wrote: > ACPI 5.1 only has two explicit methods to boot up SMP, > PSCI and Parking protocol, but the Parking protocol is > only suitable for ARMv7 now, so make PSCI as the only way > for the SMP boot protocol before some updates for the > ACPI spec or the Parking protocol spec. > > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org> > --- > arch/arm64/include/asm/acpi.h | 21 +++++++++++++++ > arch/arm64/include/asm/cpu_ops.h | 9 ++++++- > arch/arm64/include/asm/smp.h | 2 +- > arch/arm64/kernel/acpi.c | 9 +++++++ > arch/arm64/kernel/cpu_ops.c | 52 ++++++++++++++++++++++++++++++++++---- > arch/arm64/kernel/smp.c | 29 +++++++++++++++++++-- > 6 files changed, 113 insertions(+), 9 deletions(-) > > diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h > index 5ce85f8..6240327 100644 > --- a/arch/arm64/include/asm/acpi.h > +++ b/arch/arm64/include/asm/acpi.h > @@ -14,6 +14,27 @@ > > /* Basic configuration for ACPI */ > #ifdef CONFIG_ACPI ^^ This seems to be a tab (\t) character here, which is a strange thing for me to see... > +/* > + * ACPI 5.1 only has two explicit methods to > + * boot up SMP, PSCI and Parking protocol, > + * but the Parking protocol is only defined > + * for ARMv7 now, so make PSCI as the only > + * way for the SMP boot protocol before some > + * updates for the ACPI spec or the Parking > + * protocol spec. > + * > + * This enum is intend to make the boot method > + * scalable when above updates are happended, > + * which NOT means to support all of them. > + */ This comment will become out of date soon (I hope), and it is often the case that these short term comments are not removed, so I think it better to put this kind of note into the commit message, not the code. > +enum acpi_smp_boot_protocol { > + ACPI_SMP_BOOT_PSCI, > + ACPI_SMP_BOOT_PARKING_PROTOCOL, > + ACPI_SMP_BOOT_PROTOCOL_MAX > +}; > + > +enum acpi_smp_boot_protocol smp_boot_protocol(void); > + > extern int acpi_disabled; > extern int acpi_noirq; > extern int acpi_pci_disabled; > diff --git a/arch/arm64/include/asm/cpu_ops.h b/arch/arm64/include/asm/cpu_ops.h > index d7b4b38..2a7c6fd 100644 > --- a/arch/arm64/include/asm/cpu_ops.h > +++ b/arch/arm64/include/asm/cpu_ops.h > @@ -61,7 +61,14 @@ struct cpu_operations { > }; > > extern const struct cpu_operations *cpu_ops[NR_CPUS]; > -extern int __init cpu_read_ops(struct device_node *dn, int cpu); > +extern int __init cpu_of_read_ops(struct device_node *dn, int cpu); > + > +#ifdef CONFIG_ACPI > +extern int __init cpu_acpi_read_ops(int cpu); > +#else > +static inline int __init cpu_acpi_read_ops(int cpu) { return -ENODEV; } > +#endif This looks messy and not scalable for new enable methods. It seems a better way is to retain cpu_read_ops() and its functionality, which is to return the proper enable method for that cpu in a generic way. Is there some reason you can't integrate acpi into the existing cpu_ops and need to make this completely parallel method? > extern void __init cpu_read_bootcpu_ops(void); > > #endif /* ifndef __ASM_CPU_OPS_H */ > diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h > index a498f2c..a5cea56 100644 > --- a/arch/arm64/include/asm/smp.h > +++ b/arch/arm64/include/asm/smp.h > @@ -39,7 +39,7 @@ extern void show_ipi_list(struct seq_file *p, int prec); > extern void handle_IPI(int ipinr, struct pt_regs *regs); > > /* > - * Setup the set of possible CPUs (via set_cpu_possible) > + * Platform specific SMP operations > */ > extern void smp_init_cpus(void); > > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c > index ff0f6a0..2af6662 100644 > --- a/arch/arm64/kernel/acpi.c > +++ b/arch/arm64/kernel/acpi.c > @@ -184,6 +184,15 @@ static int __init acpi_parse_madt_gic_cpu_interface_entries(void) > return 0; > } > > +/* Protocol to bring up secondary CPUs */ > +enum acpi_smp_boot_protocol smp_boot_protocol(void) > +{ > + if (acpi_psci_present) > + return ACPI_SMP_BOOT_PSCI; > + else > + return ACPI_SMP_BOOT_PARKING_PROTOCOL; > +} > + > static int __init acpi_parse_fadt(struct acpi_table_header *table) > { > struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table; > diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c > index d62d12f..4d9b3cf 100644 > --- a/arch/arm64/kernel/cpu_ops.c > +++ b/arch/arm64/kernel/cpu_ops.c > @@ -16,11 +16,13 @@ > * along with this program. If not, see <http://www.gnu.org/licenses/>. > */ > > -#include <asm/cpu_ops.h> > -#include <asm/smp_plat.h> > #include <linux/errno.h> > #include <linux/of.h> > #include <linux/string.h> > +#include <linux/acpi.h> > + > +#include <asm/cpu_ops.h> > +#include <asm/smp_plat.h> > > extern const struct cpu_operations smp_spin_table_ops; > extern const struct cpu_operations cpu_psci_ops; > @@ -52,7 +54,7 @@ static const struct cpu_operations * __init cpu_get_ops(const char *name) > /* > * Read a cpu's enable method from the device tree and record it in cpu_ops. > */ > -int __init cpu_read_ops(struct device_node *dn, int cpu) > +int __init cpu_of_read_ops(struct device_node *dn, int cpu) > { > const char *enable_method = of_get_property(dn, "enable-method", NULL); > if (!enable_method) { > @@ -76,12 +78,52 @@ int __init cpu_read_ops(struct device_node *dn, int cpu) > return 0; > } > > +#ifdef CONFIG_ACPI > +/* > + * Read a cpu's enable method in the ACPI way and record it in cpu_ops. > + */ > +int __init cpu_acpi_read_ops(int cpu) > +{ > + /* > + * For ACPI 5.1, only two kind of methods are provided, > + * Parking protocol and PSCI, but Parking protocol is > + * used on ARMv7 only, so make PSCI as the only method > + * for SMP initialization before the ACPI spec or Parking > + * protocol spec is updated. > + */ Again, this comment will get old fast (I hope). > + switch (smp_boot_protocol()) { > + case ACPI_SMP_BOOT_PSCI: > + cpu_ops[cpu] = cpu_get_ops("psci"); > + break; > + case ACPI_SMP_BOOT_PARKING_PROTOCOL: > + default: > + cpu_ops[cpu] = NULL; > + break; > + } > + > + if (!cpu_ops[cpu]) { > + pr_warn("CPU %d: unsupported enable-method, only PSCI is supported\n", > + cpu); > + return -EOPNOTSUPP; > + } > + > + return 0; > +} > +#endif > + > void __init cpu_read_bootcpu_ops(void) > { > - struct device_node *dn = of_get_cpu_node(0, NULL); > + struct device_node *dn; > + > + if (!acpi_disabled) { > + cpu_acpi_read_ops(0); > + return; > + } > + > + dn = of_get_cpu_node(0, NULL); > if (!dn) { > pr_err("Failed to find device node for boot cpu\n"); > return; > } > - cpu_read_ops(dn, 0); > + cpu_of_read_ops(dn, 0); > } > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c > index 8f1d37c..cb71662 100644 > --- a/arch/arm64/kernel/smp.c > +++ b/arch/arm64/kernel/smp.c > @@ -315,7 +315,7 @@ static void (*smp_cross_call)(const struct cpumask *, unsigned int); > * cpu logical map array containing MPIDR values related to logical > * cpus. Assumes that cpu_logical_map(0) has already been initialized. > */ > -void __init smp_init_cpus(void) > +static void __init of_smp_init_cpus(void) > { > struct device_node *dn = NULL; > unsigned int i, cpu = 1; > @@ -387,7 +387,7 @@ void __init smp_init_cpus(void) > if (cpu >= NR_CPUS) > goto next; > > - if (cpu_read_ops(dn, cpu) != 0) > + if (cpu_of_read_ops(dn, cpu) != 0) > goto next; > > if (cpu_ops[cpu]->cpu_init(dn, cpu)) > @@ -418,6 +418,31 @@ next: > set_cpu_possible(i, true); > } > > +/* > + * In ACPI mode, the cpu possible map was enumerated before SMP > + * initialization when MADT table was parsed, so we can get the > + * possible map here to initialize CPUs. > + */ > +static void __init acpi_smp_init_cpus(void) > +{ > + int cpu; > + > + for_each_possible_cpu(cpu) { > + if (cpu_acpi_read_ops(cpu) != 0) > + continue; > + > + cpu_ops[cpu]->cpu_init(NULL, cpu); > + } > +} > + > +void __init smp_init_cpus(void) > +{ > + if (acpi_disabled) > + of_smp_init_cpus(); > + else > + acpi_smp_init_cpus(); > +} This is the same as cpu_ops, is acpi so special we need a completely parallel method of initializing secondary cpus? -Geoff -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2014-8-1 2:52, Geoff Levand wrote: > Hi Hanjun, Hi Geoff, > > On Thu, 2014-07-24 at 21:00 +0800, Hanjun Guo wrote: >> ACPI 5.1 only has two explicit methods to boot up SMP, >> PSCI and Parking protocol, but the Parking protocol is >> only suitable for ARMv7 now, so make PSCI as the only way >> for the SMP boot protocol before some updates for the >> ACPI spec or the Parking protocol spec. >> >> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org> >> --- >> arch/arm64/include/asm/acpi.h | 21 +++++++++++++++ >> arch/arm64/include/asm/cpu_ops.h | 9 ++++++- >> arch/arm64/include/asm/smp.h | 2 +- >> arch/arm64/kernel/acpi.c | 9 +++++++ >> arch/arm64/kernel/cpu_ops.c | 52 ++++++++++++++++++++++++++++++++++---- >> arch/arm64/kernel/smp.c | 29 +++++++++++++++++++-- >> 6 files changed, 113 insertions(+), 9 deletions(-) >> >> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h >> index 5ce85f8..6240327 100644 >> --- a/arch/arm64/include/asm/acpi.h >> +++ b/arch/arm64/include/asm/acpi.h >> @@ -14,6 +14,27 @@ >> >> /* Basic configuration for ACPI */ >> #ifdef CONFIG_ACPI > > ^^ This seems to be a tab (\t) character here, which is a strange thing > for me to see... Yes, seems to be a tab but actually not :) > >> +/* >> + * ACPI 5.1 only has two explicit methods to >> + * boot up SMP, PSCI and Parking protocol, >> + * but the Parking protocol is only defined >> + * for ARMv7 now, so make PSCI as the only >> + * way for the SMP boot protocol before some >> + * updates for the ACPI spec or the Parking >> + * protocol spec. >> + * >> + * This enum is intend to make the boot method >> + * scalable when above updates are happended, >> + * which NOT means to support all of them. >> + */ > > This comment will become out of date soon (I hope), and it is often the > case that these short term comments are not removed, so I think it > better to put this kind of note into the commit message, not the code. This will need some time, the spec is not ready and we still need more time for people to implement Parking protocol, I think we can keep this comments until Parking protocol driver is ready, and update them at that time. > >> +enum acpi_smp_boot_protocol { >> + ACPI_SMP_BOOT_PSCI, >> + ACPI_SMP_BOOT_PARKING_PROTOCOL, >> + ACPI_SMP_BOOT_PROTOCOL_MAX >> +}; >> + >> +enum acpi_smp_boot_protocol smp_boot_protocol(void); >> + >> extern int acpi_disabled; >> extern int acpi_noirq; >> extern int acpi_pci_disabled; >> diff --git a/arch/arm64/include/asm/cpu_ops.h b/arch/arm64/include/asm/cpu_ops.h >> index d7b4b38..2a7c6fd 100644 >> --- a/arch/arm64/include/asm/cpu_ops.h >> +++ b/arch/arm64/include/asm/cpu_ops.h >> @@ -61,7 +61,14 @@ struct cpu_operations { >> }; >> >> extern const struct cpu_operations *cpu_ops[NR_CPUS]; >> -extern int __init cpu_read_ops(struct device_node *dn, int cpu); >> +extern int __init cpu_of_read_ops(struct device_node *dn, int cpu); >> + >> +#ifdef CONFIG_ACPI >> +extern int __init cpu_acpi_read_ops(int cpu); >> +#else >> +static inline int __init cpu_acpi_read_ops(int cpu) { return -ENODEV; } >> +#endif > > This looks messy and not scalable for new enable methods. It > seems a better way is to retain cpu_read_ops() and its functionality, > which is to return the proper enable method for that cpu in a generic > way. > > Is there some reason you can't integrate acpi into the existing > cpu_ops and need to make this completely parallel method? > >> extern void __init cpu_read_bootcpu_ops(void); >> >> #endif /* ifndef __ASM_CPU_OPS_H */ >> diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h >> index a498f2c..a5cea56 100644 >> --- a/arch/arm64/include/asm/smp.h >> +++ b/arch/arm64/include/asm/smp.h >> @@ -39,7 +39,7 @@ extern void show_ipi_list(struct seq_file *p, int prec); >> extern void handle_IPI(int ipinr, struct pt_regs *regs); >> >> /* >> - * Setup the set of possible CPUs (via set_cpu_possible) >> + * Platform specific SMP operations >> */ >> extern void smp_init_cpus(void); >> >> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c >> index ff0f6a0..2af6662 100644 >> --- a/arch/arm64/kernel/acpi.c >> +++ b/arch/arm64/kernel/acpi.c >> @@ -184,6 +184,15 @@ static int __init acpi_parse_madt_gic_cpu_interface_entries(void) >> return 0; >> } >> >> +/* Protocol to bring up secondary CPUs */ >> +enum acpi_smp_boot_protocol smp_boot_protocol(void) >> +{ >> + if (acpi_psci_present) >> + return ACPI_SMP_BOOT_PSCI; >> + else >> + return ACPI_SMP_BOOT_PARKING_PROTOCOL; >> +} >> + >> static int __init acpi_parse_fadt(struct acpi_table_header *table) >> { >> struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table; >> diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c >> index d62d12f..4d9b3cf 100644 >> --- a/arch/arm64/kernel/cpu_ops.c >> +++ b/arch/arm64/kernel/cpu_ops.c >> @@ -16,11 +16,13 @@ >> * along with this program. If not, see <http://www.gnu.org/licenses/>. >> */ >> >> -#include <asm/cpu_ops.h> >> -#include <asm/smp_plat.h> >> #include <linux/errno.h> >> #include <linux/of.h> >> #include <linux/string.h> >> +#include <linux/acpi.h> >> + >> +#include <asm/cpu_ops.h> >> +#include <asm/smp_plat.h> >> >> extern const struct cpu_operations smp_spin_table_ops; >> extern const struct cpu_operations cpu_psci_ops; >> @@ -52,7 +54,7 @@ static const struct cpu_operations * __init cpu_get_ops(const char *name) >> /* >> * Read a cpu's enable method from the device tree and record it in cpu_ops. >> */ >> -int __init cpu_read_ops(struct device_node *dn, int cpu) >> +int __init cpu_of_read_ops(struct device_node *dn, int cpu) >> { >> const char *enable_method = of_get_property(dn, "enable-method", NULL); >> if (!enable_method) { >> @@ -76,12 +78,52 @@ int __init cpu_read_ops(struct device_node *dn, int cpu) >> return 0; >> } >> >> +#ifdef CONFIG_ACPI >> +/* >> + * Read a cpu's enable method in the ACPI way and record it in cpu_ops. >> + */ >> +int __init cpu_acpi_read_ops(int cpu) >> +{ >> + /* >> + * For ACPI 5.1, only two kind of methods are provided, >> + * Parking protocol and PSCI, but Parking protocol is >> + * used on ARMv7 only, so make PSCI as the only method >> + * for SMP initialization before the ACPI spec or Parking >> + * protocol spec is updated. >> + */ > > Again, this comment will get old fast (I hope). > >> + switch (smp_boot_protocol()) { >> + case ACPI_SMP_BOOT_PSCI: >> + cpu_ops[cpu] = cpu_get_ops("psci"); >> + break; >> + case ACPI_SMP_BOOT_PARKING_PROTOCOL: >> + default: >> + cpu_ops[cpu] = NULL; >> + break; >> + } >> + >> + if (!cpu_ops[cpu]) { >> + pr_warn("CPU %d: unsupported enable-method, only PSCI is supported\n", >> + cpu); >> + return -EOPNOTSUPP; >> + } >> + >> + return 0; >> +} >> +#endif >> + >> void __init cpu_read_bootcpu_ops(void) >> { >> - struct device_node *dn = of_get_cpu_node(0, NULL); >> + struct device_node *dn; >> + >> + if (!acpi_disabled) { >> + cpu_acpi_read_ops(0); >> + return; >> + } >> + >> + dn = of_get_cpu_node(0, NULL); >> if (!dn) { >> pr_err("Failed to find device node for boot cpu\n"); >> return; >> } >> - cpu_read_ops(dn, 0); >> + cpu_of_read_ops(dn, 0); >> } >> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c >> index 8f1d37c..cb71662 100644 >> --- a/arch/arm64/kernel/smp.c >> +++ b/arch/arm64/kernel/smp.c >> @@ -315,7 +315,7 @@ static void (*smp_cross_call)(const struct cpumask *, unsigned int); >> * cpu logical map array containing MPIDR values related to logical >> * cpus. Assumes that cpu_logical_map(0) has already been initialized. >> */ >> -void __init smp_init_cpus(void) >> +static void __init of_smp_init_cpus(void) >> { >> struct device_node *dn = NULL; >> unsigned int i, cpu = 1; >> @@ -387,7 +387,7 @@ void __init smp_init_cpus(void) >> if (cpu >= NR_CPUS) >> goto next; >> >> - if (cpu_read_ops(dn, cpu) != 0) >> + if (cpu_of_read_ops(dn, cpu) != 0) >> goto next; >> >> if (cpu_ops[cpu]->cpu_init(dn, cpu)) >> @@ -418,6 +418,31 @@ next: >> set_cpu_possible(i, true); >> } >> >> +/* >> + * In ACPI mode, the cpu possible map was enumerated before SMP >> + * initialization when MADT table was parsed, so we can get the >> + * possible map here to initialize CPUs. >> + */ >> +static void __init acpi_smp_init_cpus(void) >> +{ >> + int cpu; >> + >> + for_each_possible_cpu(cpu) { >> + if (cpu_acpi_read_ops(cpu) != 0) >> + continue; >> + >> + cpu_ops[cpu]->cpu_init(NULL, cpu); >> + } >> +} >> + >> +void __init smp_init_cpus(void) >> +{ >> + if (acpi_disabled) >> + of_smp_init_cpus(); >> + else >> + acpi_smp_init_cpus(); >> +} > > This is the same as cpu_ops, is acpi so special we need a completely > parallel method of initializing secondary cpus? Good point, Olof suggested the same here, I'm working on it and try to implement the code as you suggested, if I can't I will comment on it for the reasons that why I can't make it. Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index 5ce85f8..6240327 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -14,6 +14,27 @@ /* Basic configuration for ACPI */ #ifdef CONFIG_ACPI +/* + * ACPI 5.1 only has two explicit methods to + * boot up SMP, PSCI and Parking protocol, + * but the Parking protocol is only defined + * for ARMv7 now, so make PSCI as the only + * way for the SMP boot protocol before some + * updates for the ACPI spec or the Parking + * protocol spec. + * + * This enum is intend to make the boot method + * scalable when above updates are happended, + * which NOT means to support all of them. + */ +enum acpi_smp_boot_protocol { + ACPI_SMP_BOOT_PSCI, + ACPI_SMP_BOOT_PARKING_PROTOCOL, + ACPI_SMP_BOOT_PROTOCOL_MAX +}; + +enum acpi_smp_boot_protocol smp_boot_protocol(void); + extern int acpi_disabled; extern int acpi_noirq; extern int acpi_pci_disabled; diff --git a/arch/arm64/include/asm/cpu_ops.h b/arch/arm64/include/asm/cpu_ops.h index d7b4b38..2a7c6fd 100644 --- a/arch/arm64/include/asm/cpu_ops.h +++ b/arch/arm64/include/asm/cpu_ops.h @@ -61,7 +61,14 @@ struct cpu_operations { }; extern const struct cpu_operations *cpu_ops[NR_CPUS]; -extern int __init cpu_read_ops(struct device_node *dn, int cpu); +extern int __init cpu_of_read_ops(struct device_node *dn, int cpu); + +#ifdef CONFIG_ACPI +extern int __init cpu_acpi_read_ops(int cpu); +#else +static inline int __init cpu_acpi_read_ops(int cpu) { return -ENODEV; } +#endif + extern void __init cpu_read_bootcpu_ops(void); #endif /* ifndef __ASM_CPU_OPS_H */ diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h index a498f2c..a5cea56 100644 --- a/arch/arm64/include/asm/smp.h +++ b/arch/arm64/include/asm/smp.h @@ -39,7 +39,7 @@ extern void show_ipi_list(struct seq_file *p, int prec); extern void handle_IPI(int ipinr, struct pt_regs *regs); /* - * Setup the set of possible CPUs (via set_cpu_possible) + * Platform specific SMP operations */ extern void smp_init_cpus(void); diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index ff0f6a0..2af6662 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -184,6 +184,15 @@ static int __init acpi_parse_madt_gic_cpu_interface_entries(void) return 0; } +/* Protocol to bring up secondary CPUs */ +enum acpi_smp_boot_protocol smp_boot_protocol(void) +{ + if (acpi_psci_present) + return ACPI_SMP_BOOT_PSCI; + else + return ACPI_SMP_BOOT_PARKING_PROTOCOL; +} + static int __init acpi_parse_fadt(struct acpi_table_header *table) { struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table; diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c index d62d12f..4d9b3cf 100644 --- a/arch/arm64/kernel/cpu_ops.c +++ b/arch/arm64/kernel/cpu_ops.c @@ -16,11 +16,13 @@ * along with this program. If not, see <http://www.gnu.org/licenses/>. */ -#include <asm/cpu_ops.h> -#include <asm/smp_plat.h> #include <linux/errno.h> #include <linux/of.h> #include <linux/string.h> +#include <linux/acpi.h> + +#include <asm/cpu_ops.h> +#include <asm/smp_plat.h> extern const struct cpu_operations smp_spin_table_ops; extern const struct cpu_operations cpu_psci_ops; @@ -52,7 +54,7 @@ static const struct cpu_operations * __init cpu_get_ops(const char *name) /* * Read a cpu's enable method from the device tree and record it in cpu_ops. */ -int __init cpu_read_ops(struct device_node *dn, int cpu) +int __init cpu_of_read_ops(struct device_node *dn, int cpu) { const char *enable_method = of_get_property(dn, "enable-method", NULL); if (!enable_method) { @@ -76,12 +78,52 @@ int __init cpu_read_ops(struct device_node *dn, int cpu) return 0; } +#ifdef CONFIG_ACPI +/* + * Read a cpu's enable method in the ACPI way and record it in cpu_ops. + */ +int __init cpu_acpi_read_ops(int cpu) +{ + /* + * For ACPI 5.1, only two kind of methods are provided, + * Parking protocol and PSCI, but Parking protocol is + * used on ARMv7 only, so make PSCI as the only method + * for SMP initialization before the ACPI spec or Parking + * protocol spec is updated. + */ + switch (smp_boot_protocol()) { + case ACPI_SMP_BOOT_PSCI: + cpu_ops[cpu] = cpu_get_ops("psci"); + break; + case ACPI_SMP_BOOT_PARKING_PROTOCOL: + default: + cpu_ops[cpu] = NULL; + break; + } + + if (!cpu_ops[cpu]) { + pr_warn("CPU %d: unsupported enable-method, only PSCI is supported\n", + cpu); + return -EOPNOTSUPP; + } + + return 0; +} +#endif + void __init cpu_read_bootcpu_ops(void) { - struct device_node *dn = of_get_cpu_node(0, NULL); + struct device_node *dn; + + if (!acpi_disabled) { + cpu_acpi_read_ops(0); + return; + } + + dn = of_get_cpu_node(0, NULL); if (!dn) { pr_err("Failed to find device node for boot cpu\n"); return; } - cpu_read_ops(dn, 0); + cpu_of_read_ops(dn, 0); } diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c index 8f1d37c..cb71662 100644 --- a/arch/arm64/kernel/smp.c +++ b/arch/arm64/kernel/smp.c @@ -315,7 +315,7 @@ static void (*smp_cross_call)(const struct cpumask *, unsigned int); * cpu logical map array containing MPIDR values related to logical * cpus. Assumes that cpu_logical_map(0) has already been initialized. */ -void __init smp_init_cpus(void) +static void __init of_smp_init_cpus(void) { struct device_node *dn = NULL; unsigned int i, cpu = 1; @@ -387,7 +387,7 @@ void __init smp_init_cpus(void) if (cpu >= NR_CPUS) goto next; - if (cpu_read_ops(dn, cpu) != 0) + if (cpu_of_read_ops(dn, cpu) != 0) goto next; if (cpu_ops[cpu]->cpu_init(dn, cpu)) @@ -418,6 +418,31 @@ next: set_cpu_possible(i, true); } +/* + * In ACPI mode, the cpu possible map was enumerated before SMP + * initialization when MADT table was parsed, so we can get the + * possible map here to initialize CPUs. + */ +static void __init acpi_smp_init_cpus(void) +{ + int cpu; + + for_each_possible_cpu(cpu) { + if (cpu_acpi_read_ops(cpu) != 0) + continue; + + cpu_ops[cpu]->cpu_init(NULL, cpu); + } +} + +void __init smp_init_cpus(void) +{ + if (acpi_disabled) + of_smp_init_cpus(); + else + acpi_smp_init_cpus(); +} + void __init smp_prepare_cpus(unsigned int max_cpus) { int err;