diff mbox

[v2,08/18] ARM64 / ACPI: Get the enable method for SMP initialization in ACPI way

Message ID 1407166105-17675-9-git-send-email-hanjun.guo@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Hanjun Guo Aug. 4, 2014, 3:28 p.m. UTC
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/smp.h  |    3 +-
 arch/arm64/kernel/acpi.c      |    9 ++++++
 arch/arm64/kernel/cpu_ops.c   |   62 ++++++++++++++++++++++++++++++++++++-----
 arch/arm64/kernel/smp.c       |   27 +++++++++++++++++-
 5 files changed, 113 insertions(+), 9 deletions(-)

Comments

Catalin Marinas Aug. 18, 2014, 2:27 p.m. UTC | #1
On Mon, Aug 04, 2014 at 04:28:15PM +0100, Hanjun Guo wrote:
> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index e877967..022f4ad 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);

Since this is not a static function and ACPI-specific, could you prefix
it with something like acpi (or arm64_acpi_ unless it gets too long)?

> diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
> index d62d12f..05bc314 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;
> @@ -49,12 +51,44 @@ static const struct cpu_operations * __init cpu_get_ops(const char *name)
>  	return NULL;
>  }
>  
> +#ifdef CONFIG_ACPI
> +/*
> + * Get a cpu's boot method in the ACPI way.
> + */
> +static char * __init acpi_get_cpu_boot_method(void)
> +{
> +	/*
> +	 * For ACPI 5.1, only two kind of methods are provided,
> +	 * Parking protocol and PSCI, but Parking protocol is
> +	 * specified for 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:
> +		return "psci";
> +	case ACPI_SMP_BOOT_PARKING_PROTOCOL:
> +	default:
> +		return NULL;
> +	}
> +}

Actually, do we even need to define smp_boot_protocol()? Is it used
anywhere else apart from this patch (I still haven't gone through all
patches)?

> +#else
> +static inline char * __init acpi_get_cpu_boot_method(void) { return NULL; }
> +#endif
> +
>  /*
> - * Read a cpu's enable method from the device tree and record it in cpu_ops.
> + * Read a cpu's enable method and record it in cpu_ops.
>   */
>  int __init cpu_read_ops(struct device_node *dn, int cpu)
>  {
> -	const char *enable_method = of_get_property(dn, "enable-method", NULL);
> +	const char *enable_method;
> +
> +	if (!acpi_disabled) {
> +		enable_method = acpi_get_cpu_boot_method();
> +		goto get_ops;

Are we always guaranteed an enable_method here? You should have an
if/else of ACPI vs DT and keep the if (!enable_method) check for both
cases.
Sudeep Holla Aug. 18, 2014, 6:34 p.m. UTC | #2
On 04/08/14 16:28, 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/smp.h  |    3 +-
>   arch/arm64/kernel/acpi.c      |    9 ++++++
>   arch/arm64/kernel/cpu_ops.c   |   62 ++++++++++++++++++++++++++++++++++++-----
>   arch/arm64/kernel/smp.c       |   27 +++++++++++++++++-
>   5 files changed, 113 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index e877967..022f4ad 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);
> +
>   #define acpi_strict 1	/* No out-of-spec workarounds on ARM64 */
>   extern int acpi_disabled;
>   extern int acpi_noirq;
> diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h
> index a498f2c..282932c2 100644
> --- a/arch/arm64/include/asm/smp.h
> +++ b/arch/arm64/include/asm/smp.h
> @@ -39,7 +39,8 @@ 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)
> + * Discover the set of possible CPUs and determine their
> + * SMP operations.
>    */
>   extern void smp_init_cpus(void);
>
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> index 9e07d99..8a54b4e 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -170,6 +170,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;
> +}
> +

Which do you need this here ? Can't you use acpi_psci_present directly
in acpi_get_cpu_boot_method ?

>   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..05bc314 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;
> @@ -49,12 +51,44 @@ static const struct cpu_operations * __init cpu_get_ops(const char *name)
>   	return NULL;
>   }
>
> +#ifdef CONFIG_ACPI
> +/*
> + * Get a cpu's boot method in the ACPI way.
> + */
> +static char * __init acpi_get_cpu_boot_method(void)
> +{
> +	/*
> +	 * For ACPI 5.1, only two kind of methods are provided,
> +	 * Parking protocol and PSCI, but Parking protocol is
> +	 * specified for 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:
> +		return "psci";
> +	case ACPI_SMP_BOOT_PARKING_PROTOCOL:
> +	default:
> +		return NULL;
> +	}

Use acpi_psci_present as mentioned above.

> +}
> +#else
> +static inline char * __init acpi_get_cpu_boot_method(void) { return NULL; }
> +#endif
> +
>   /*
> - * Read a cpu's enable method from the device tree and record it in cpu_ops.
> + * Read a cpu's enable method and record it in cpu_ops.
>    */
>   int __init cpu_read_ops(struct device_node *dn, int cpu)
>   {
> -	const char *enable_method = of_get_property(dn, "enable-method", NULL);
> +	const char *enable_method;
> +
> +	if (!acpi_disabled) {
> +		enable_method = acpi_get_cpu_boot_method();
> +		goto get_ops;
> +	}
> +
> +	enable_method = of_get_property(dn, "enable-method", NULL);
>   	if (!enable_method) {
>   		/*
>   		 * The boot CPU may not have an enable method (e.g. when
> @@ -66,10 +100,17 @@ int __init cpu_read_ops(struct device_node *dn, int cpu)
>   		return -ENOENT;
>   	}
>
> +get_ops:
>   	cpu_ops[cpu] = cpu_get_ops(enable_method);
>   	if (!cpu_ops[cpu]) {
> -		pr_warn("%s: unsupported enable-method property: %s\n",
> -			dn->full_name, enable_method);
> +		if (acpi_disabled) {
> +			pr_warn("%s: unsupported enable-method property: %s\n",
> +				dn->full_name, enable_method);
> +		} else {
> +			pr_warn("CPU %d: boot protocol unsupported or unknown\n",
> +				cpu);
> +		}
> +
>   		return -EOPNOTSUPP;
>   	}
>
> @@ -78,7 +119,14 @@ int __init cpu_read_ops(struct device_node *dn, int cpu)
>
>   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_read_ops(NULL, 0);
> +		return;
> +	}
> +

Again not good to mix ACPI in DT functions forcing you to pass
device_node ptr as NULL, better to separate this. Once you gather all
this !acpi_disabled case, you can create appropriate abstractions to be
used in setup.c

E.g. here you check !acpi_disabled and pass NULL for DT node to
cpu_read_ops and hence again you check for !acpi_disabled in
cpu_read_ops. So you need first identify all these checks and put in one
place to understand well how you can refactor existing code to avoid
these multiple checks.


> +	dn = of_get_cpu_node(0, NULL);
>   	if (!dn) {
>   		pr_err("Failed to find device node for boot cpu\n");
>   		return;
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 8f1d37c..e21bbc9 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;
> @@ -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_read_ops(NULL, 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;
>
Geoff Levand Aug. 18, 2014, 6:56 p.m. UTC | #3
Hi Hanjun,

On Mon, 2014-08-04 at 23:28 +0800, Hanjun Guo wrote:
> --- 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

By having this preprocessor conditional in the header leads
to a proliferation of preprocessor conditionals since any
code that includes this header will also need to have
preprocessor conditionals.  Another down side of having
this is that this code will not get a build test for
builds with CONFIG_ACPI=n.

> +/*
> + * 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);

The name smp_boot_protocol() seems like it would be a generic
routine, but it is acpi specific.  Maybe:

enum acpi_boot_protocol_type {...};

enum acpi_boot_protocol_type acpi_boot_protocol(void);

> --- a/arch/arm64/kernel/cpu_ops.c
> +++ b/arch/arm64/kernel/cpu_ops.c
> @@ -49,12 +51,44 @@ static const struct cpu_operations * __init cpu_get_ops(const char *name)
>  	return NULL;
>  }
>  
> +#ifdef CONFIG_ACPI
> +/*
> + * Get a cpu's boot method in the ACPI way.
> + */
> +static char * __init acpi_get_cpu_boot_method(void)
> +{
> +	/*
> +	 * For ACPI 5.1, only two kind of methods are provided,
> +	 * Parking protocol and PSCI, but Parking protocol is
> +	 * specified for 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:
> +		return "psci";
> +	case ACPI_SMP_BOOT_PARKING_PROTOCOL:
> +	default:
> +		return NULL;
> +	}
> +}
> +#else
> +static inline char * __init acpi_get_cpu_boot_method(void) { return NULL; }
> +#endif

Since this is inside a C source file, the inline keyword
isn't needed since the optimizer will inline regardless.

With that said, I think it would be cleaner to have this
as:

static char * __init acpi_get_cpu_boot_method(void)
{
#ifdef CONFIG_ACPI
	return NULL;
#else
 ...
#endif
}

Or better to make smp_boot_protocol() callable regardless
of CONFIG_ACPI and then no preprocessor conditionals at all
would be needed.

> +
>  /*
> - * Read a cpu's enable method from the device tree and record it in cpu_ops.
> + * Read a cpu's enable method and record it in cpu_ops.
>   */
>  int __init cpu_read_ops(struct device_node *dn, int cpu)
>  {
> -	const char *enable_method = of_get_property(dn, "enable-method", NULL);
> +	const char *enable_method;
> +
> +	if (!acpi_disabled) {
> +		enable_method = acpi_get_cpu_boot_method();
> +		goto get_ops;
> +	}
> +
> +	enable_method = of_get_property(dn, "enable-method", NULL);
>  	if (!enable_method) {
>  		/*
>  		 * The boot CPU may not have an enable method (e.g. when
> @@ -66,10 +100,17 @@ int __init cpu_read_ops(struct device_node *dn, int cpu)
>  		return -ENOENT;
>  	}
>  
> +get_ops:
>  	cpu_ops[cpu] = cpu_get_ops(enable_method);
>  	if (!cpu_ops[cpu]) {
> -		pr_warn("%s: unsupported enable-method property: %s\n",
> -			dn->full_name, enable_method);
> +		if (acpi_disabled) {
> +			pr_warn("%s: unsupported enable-method property: %s\n",
> +				dn->full_name, enable_method);
> +		} else {
> +			pr_warn("CPU %d: boot protocol unsupported or unknown\n",
> +				cpu);
> +		}
> +

Can't we have this more integrated, maybe something like this?

	enable_method = acpi_disabled ? of_get_property(dn, "enable-method", NULL)
		: acpi_get_cpu_boot_method();
	message = acpi_disabled ? dn->full_name : "";

	...
	
	pr_warn("CPU %d: %s unsupported enable-method property: %s\n",
				cpu, message, enable_method)

-Geoff
Hanjun Guo Aug. 19, 2014, 8:32 a.m. UTC | #4
On 2014-8-18 22:27, Catalin Marinas wrote:
> On Mon, Aug 04, 2014 at 04:28:15PM +0100, Hanjun Guo wrote:
>> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
>> index e877967..022f4ad 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);
> 
> Since this is not a static function and ACPI-specific, could you prefix
> it with something like acpi (or arm64_acpi_ unless it gets too long)?
> 
>> diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
>> index d62d12f..05bc314 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;
>> @@ -49,12 +51,44 @@ static const struct cpu_operations * __init cpu_get_ops(const char *name)
>>  	return NULL;
>>  }
>>  
>> +#ifdef CONFIG_ACPI
>> +/*
>> + * Get a cpu's boot method in the ACPI way.
>> + */
>> +static char * __init acpi_get_cpu_boot_method(void)
>> +{
>> +	/*
>> +	 * For ACPI 5.1, only two kind of methods are provided,
>> +	 * Parking protocol and PSCI, but Parking protocol is
>> +	 * specified for 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:
>> +		return "psci";
>> +	case ACPI_SMP_BOOT_PARKING_PROTOCOL:
>> +	default:
>> +		return NULL;
>> +	}
>> +}
> 
> Actually, do we even need to define smp_boot_protocol()? Is it used
> anywhere else apart from this patch (I still haven't gone through all
> patches)?

It is just used in this patch. I think we can make the ACPI boot protocol
scalable in this way, if we support another boot protocol in ACPI in the
future, we can easily update the function to support it, does it make sense?

> 
>> +#else
>> +static inline char * __init acpi_get_cpu_boot_method(void) { return NULL; }
>> +#endif
>> +
>>  /*
>> - * Read a cpu's enable method from the device tree and record it in cpu_ops.
>> + * Read a cpu's enable method and record it in cpu_ops.
>>   */
>>  int __init cpu_read_ops(struct device_node *dn, int cpu)
>>  {
>> -	const char *enable_method = of_get_property(dn, "enable-method", NULL);
>> +	const char *enable_method;
>> +
>> +	if (!acpi_disabled) {
>> +		enable_method = acpi_get_cpu_boot_method();
>> +		goto get_ops;
> 
> Are we always guaranteed an enable_method here? You should have an
> if/else of ACPI vs DT and keep the if (!enable_method) check for both
> cases.

ok, I will fix it.

Thanks
Hanjun
Hanjun Guo Aug. 19, 2014, 11:26 a.m. UTC | #5
On 2014-8-19 2:34, Sudeep Holla wrote:
> On 04/08/14 16:28, Hanjun Guo wrote:
[...]
>>   /* 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);
>> +
[...]
>> +/* 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;
>> +}
>> +
> 
> Which do you need this here ? Can't you use acpi_psci_present directly
> in acpi_get_cpu_boot_method ?

My intent was to make the code scalable if we introduce another (or more)
boot protocol in ACPI, does it make sense to you?

> 
>>   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..05bc314 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;
>> @@ -49,12 +51,44 @@ static const struct cpu_operations * __init
>> cpu_get_ops(const char *name)
>>       return NULL;
>>   }
>>
>> +#ifdef CONFIG_ACPI
>> +/*
>> + * Get a cpu's boot method in the ACPI way.
>> + */
>> +static char * __init acpi_get_cpu_boot_method(void)
>> +{
>> +    /*
>> +     * For ACPI 5.1, only two kind of methods are provided,
>> +     * Parking protocol and PSCI, but Parking protocol is
>> +     * specified for 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:
>> +        return "psci";
>> +    case ACPI_SMP_BOOT_PARKING_PROTOCOL:
>> +    default:
>> +        return NULL;
>> +    }
> 
> Use acpi_psci_present as mentioned above.
> 
>> +}
>> +#else
>> +static inline char * __init acpi_get_cpu_boot_method(void) { return NULL; }
>> +#endif
>> +
>>   /*
>> - * Read a cpu's enable method from the device tree and record it in cpu_ops.
>> + * Read a cpu's enable method and record it in cpu_ops.
>>    */
>>   int __init cpu_read_ops(struct device_node *dn, int cpu)
>>   {
>> -    const char *enable_method = of_get_property(dn, "enable-method", NULL);
>> +    const char *enable_method;
>> +
>> +    if (!acpi_disabled) {
>> +        enable_method = acpi_get_cpu_boot_method();
>> +        goto get_ops;
>> +    }
>> +
>> +    enable_method = of_get_property(dn, "enable-method", NULL);
>>       if (!enable_method) {
>>           /*
>>            * The boot CPU may not have an enable method (e.g. when
>> @@ -66,10 +100,17 @@ int __init cpu_read_ops(struct device_node *dn, int cpu)
>>           return -ENOENT;
>>       }
>>
>> +get_ops:
>>       cpu_ops[cpu] = cpu_get_ops(enable_method);
>>       if (!cpu_ops[cpu]) {
>> -        pr_warn("%s: unsupported enable-method property: %s\n",
>> -            dn->full_name, enable_method);
>> +        if (acpi_disabled) {
>> +            pr_warn("%s: unsupported enable-method property: %s\n",
>> +                dn->full_name, enable_method);
>> +        } else {
>> +            pr_warn("CPU %d: boot protocol unsupported or unknown\n",
>> +                cpu);
>> +        }
>> +
>>           return -EOPNOTSUPP;
>>       }
>>
>> @@ -78,7 +119,14 @@ int __init cpu_read_ops(struct device_node *dn, int cpu)
>>
>>   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_read_ops(NULL, 0);
>> +        return;
>> +    }
>> +
> 
> Again not good to mix ACPI in DT functions forcing you to pass
> device_node ptr as NULL, better to separate this. 

I separate them in the first version, and combine tham as Geoff suggested
for scalable reasons.

> Once you gather all
> this !acpi_disabled case, you can create appropriate abstractions to be
> used in setup.c
> 
> E.g. here you check !acpi_disabled and pass NULL for DT node to
> cpu_read_ops and hence again you check for !acpi_disabled in
> cpu_read_ops. So you need first identify all these checks and put in one
> place to understand well how you can refactor existing code to avoid
> these multiple checks.

I will remove the multiple acpi_disabled checks and refactor the code.

Thanks
Hanjun
Hanjun Guo Aug. 19, 2014, 12:11 p.m. UTC | #6
On 2014-8-19 2:56, Geoff Levand wrote:
> Hi Hanjun,

Hi Geoff,

> 
> On Mon, 2014-08-04 at 23:28 +0800, Hanjun Guo wrote:
>> --- 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
> 
> By having this preprocessor conditional in the header leads
> to a proliferation of preprocessor conditionals since any
> code that includes this header will also need to have
> preprocessor conditionals.  Another down side of having
> this is that this code will not get a build test for
> builds with CONFIG_ACPI=n.

I will move some definitions out of preprocessor conditional and
introduce some stub function when CONFIG_ACPI is disabled, then
I think I can remove all the preprocessor conditionals in .c file.

> 
>> +/*
>> + * 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);
> 
> The name smp_boot_protocol() seems like it would be a generic
> routine, but it is acpi specific.  Maybe:
> 
> enum acpi_boot_protocol_type {...};
> 
> enum acpi_boot_protocol_type acpi_boot_protocol(void);

Agreed.

> 
>> --- a/arch/arm64/kernel/cpu_ops.c
>> +++ b/arch/arm64/kernel/cpu_ops.c
>> @@ -49,12 +51,44 @@ static const struct cpu_operations * __init cpu_get_ops(const char *name)
>>  	return NULL;
>>  }
>>  
>> +#ifdef CONFIG_ACPI
>> +/*
>> + * Get a cpu's boot method in the ACPI way.
>> + */
>> +static char * __init acpi_get_cpu_boot_method(void)
>> +{
>> +	/*
>> +	 * For ACPI 5.1, only two kind of methods are provided,
>> +	 * Parking protocol and PSCI, but Parking protocol is
>> +	 * specified for 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:
>> +		return "psci";
>> +	case ACPI_SMP_BOOT_PARKING_PROTOCOL:
>> +	default:
>> +		return NULL;
>> +	}
>> +}
>> +#else
>> +static inline char * __init acpi_get_cpu_boot_method(void) { return NULL; }
>> +#endif
> 
> Since this is inside a C source file, the inline keyword
> isn't needed since the optimizer will inline regardless.
> 
> With that said, I think it would be cleaner to have this
> as:
> 
> static char * __init acpi_get_cpu_boot_method(void)
> {
> #ifdef CONFIG_ACPI
> 	return NULL;
> #else
>  ...
> #endif
> }
> 
> Or better to make smp_boot_protocol() callable regardless
> of CONFIG_ACPI and then no preprocessor conditionals at all
> would be needed.
> 
>> +
>>  /*
>> - * Read a cpu's enable method from the device tree and record it in cpu_ops.
>> + * Read a cpu's enable method and record it in cpu_ops.
>>   */
>>  int __init cpu_read_ops(struct device_node *dn, int cpu)
>>  {
>> -	const char *enable_method = of_get_property(dn, "enable-method", NULL);
>> +	const char *enable_method;
>> +
>> +	if (!acpi_disabled) {
>> +		enable_method = acpi_get_cpu_boot_method();
>> +		goto get_ops;
>> +	}
>> +
>> +	enable_method = of_get_property(dn, "enable-method", NULL);
>>  	if (!enable_method) {
>>  		/*
>>  		 * The boot CPU may not have an enable method (e.g. when
>> @@ -66,10 +100,17 @@ int __init cpu_read_ops(struct device_node *dn, int cpu)
>>  		return -ENOENT;
>>  	}
>>  
>> +get_ops:
>>  	cpu_ops[cpu] = cpu_get_ops(enable_method);
>>  	if (!cpu_ops[cpu]) {
>> -		pr_warn("%s: unsupported enable-method property: %s\n",
>> -			dn->full_name, enable_method);
>> +		if (acpi_disabled) {
>> +			pr_warn("%s: unsupported enable-method property: %s\n",
>> +				dn->full_name, enable_method);
>> +		} else {
>> +			pr_warn("CPU %d: boot protocol unsupported or unknown\n",
>> +				cpu);
>> +		}
>> +
> 
> Can't we have this more integrated, maybe something like this?
> 
> 	enable_method = acpi_disabled ? of_get_property(dn, "enable-method", NULL)
> 		: acpi_get_cpu_boot_method();

I like this :)

> 	message = acpi_disabled ? dn->full_name : "";
> 
> 	...
> 	
> 	pr_warn("CPU %d: %s unsupported enable-method property: %s\n",
> 				cpu, message, enable_method)

In ACPI, there is no enable-method property, it is a term from, so I think the
message printed can be separated.

Thanks
Hanjun
Geoff Levand Aug. 19, 2014, 7:25 p.m. UTC | #7
Hi Hanjun,

On Tue, 2014-08-19 at 20:11 +0800, Hanjun Guo wrote:
> On 2014-8-19 2:56, Geoff Levand wrote:

> > 	message = acpi_disabled ? dn->full_name : "";
> > 
> > 	...
> > 	
> > 	pr_warn("CPU %d: %s unsupported enable-method property: %s\n",
> > 				cpu, message, enable_method)
> 
> In ACPI, there is no enable-method property, it is a term from, so I think the
> message printed can be separated.

I think it better to have a single message that can cover all
than to have separate messages.  Wouldn't the enable method be
"acpi-parking" or "acpi-psci"?  Then something like this would
work:

	pr_warn("CPU %d: %s Unsupported enable method: %s\n", cpu, message, enable_method);

-Geoff
Hanjun Guo Aug. 20, 2014, 3:25 a.m. UTC | #8
On 2014-8-20 3:25, Geoff Levand wrote:
> Hi Hanjun,
> 
> On Tue, 2014-08-19 at 20:11 +0800, Hanjun Guo wrote:
>> On 2014-8-19 2:56, Geoff Levand wrote:
> 
>>> 	message = acpi_disabled ? dn->full_name : "";
>>>
>>> 	...
>>> 	
>>> 	pr_warn("CPU %d: %s unsupported enable-method property: %s\n",
>>> 				cpu, message, enable_method)
>>
>> In ACPI, there is no enable-method property, it is a term from, so I think the
>> message printed can be separated.
> 
> I think it better to have a single message that can cover all
> than to have separate messages.  Wouldn't the enable method be
> "acpi-parking" or "acpi-psci"?  Then something like this would
> work:
> 
> 	pr_warn("CPU %d: %s Unsupported enable method: %s\n", cpu, message, enable_method);

Thanks for the suggestion, I will update it in next version.

Thanks
Hanjun
Catalin Marinas Aug. 20, 2014, 2:52 p.m. UTC | #9
On Tue, Aug 19, 2014 at 09:32:25AM +0100, Hanjun Guo wrote:
> On 2014-8-18 22:27, Catalin Marinas wrote:
> > On Mon, Aug 04, 2014 at 04:28:15PM +0100, Hanjun Guo wrote:
> >> +#ifdef CONFIG_ACPI
> >> +/*
> >> + * Get a cpu's boot method in the ACPI way.
> >> + */
> >> +static char * __init acpi_get_cpu_boot_method(void)
> >> +{
> >> +	/*
> >> +	 * For ACPI 5.1, only two kind of methods are provided,
> >> +	 * Parking protocol and PSCI, but Parking protocol is
> >> +	 * specified for 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:
> >> +		return "psci";
> >> +	case ACPI_SMP_BOOT_PARKING_PROTOCOL:
> >> +	default:
> >> +		return NULL;
> >> +	}
> >> +}
> > 
> > Actually, do we even need to define smp_boot_protocol()? Is it used
> > anywhere else apart from this patch (I still haven't gone through all
> > patches)?
> 
> It is just used in this patch. I think we can make the ACPI boot protocol
> scalable in this way, if we support another boot protocol in ACPI in the
> future, we can easily update the function to support it, does it make sense?

Not really. You just add additional code, enums, functions when all you
do is check for acpi_psci_present() (or whatever new protocol you would
get). If the enum is never going to be used outside this file, don't
bother with additional functions.

BTW, it would be nicer if the acpi related functions are contained in as
fewer files as possible. So here you could keep
acpi_get_cpu_boot_method() in the acpi.c file. It only returns a string.
Hanjun Guo Aug. 21, 2014, 3:06 a.m. UTC | #10
On 2014-8-20 22:52, Catalin Marinas wrote:
> On Tue, Aug 19, 2014 at 09:32:25AM +0100, Hanjun Guo wrote:
>> On 2014-8-18 22:27, Catalin Marinas wrote:
>>> On Mon, Aug 04, 2014 at 04:28:15PM +0100, Hanjun Guo wrote:
>>>> +#ifdef CONFIG_ACPI
>>>> +/*
>>>> + * Get a cpu's boot method in the ACPI way.
>>>> + */
>>>> +static char * __init acpi_get_cpu_boot_method(void)
>>>> +{
>>>> +	/*
>>>> +	 * For ACPI 5.1, only two kind of methods are provided,
>>>> +	 * Parking protocol and PSCI, but Parking protocol is
>>>> +	 * specified for 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:
>>>> +		return "psci";
>>>> +	case ACPI_SMP_BOOT_PARKING_PROTOCOL:
>>>> +	default:
>>>> +		return NULL;
>>>> +	}
>>>> +}
>>>
>>> Actually, do we even need to define smp_boot_protocol()? Is it used
>>> anywhere else apart from this patch (I still haven't gone through all
>>> patches)?
>>
>> It is just used in this patch. I think we can make the ACPI boot protocol
>> scalable in this way, if we support another boot protocol in ACPI in the
>> future, we can easily update the function to support it, does it make sense?
> 
> Not really. You just add additional code, enums, functions when all you
> do is check for acpi_psci_present() (or whatever new protocol you would
> get). If the enum is never going to be used outside this file, don't
> bother with additional functions.
> 
> BTW, it would be nicer if the acpi related functions are contained in as
> fewer files as possible. So here you could keep
> acpi_get_cpu_boot_method() in the acpi.c file. It only returns a string.

ok, I will update them.

Thanks
Hanjun
diff mbox

Patch

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index e877967..022f4ad 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);
+
 #define acpi_strict 1	/* No out-of-spec workarounds on ARM64 */
 extern int acpi_disabled;
 extern int acpi_noirq;
diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h
index a498f2c..282932c2 100644
--- a/arch/arm64/include/asm/smp.h
+++ b/arch/arm64/include/asm/smp.h
@@ -39,7 +39,8 @@  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)
+ * Discover the set of possible CPUs and determine their
+ * SMP operations.
  */
 extern void smp_init_cpus(void);
 
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 9e07d99..8a54b4e 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -170,6 +170,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..05bc314 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;
@@ -49,12 +51,44 @@  static const struct cpu_operations * __init cpu_get_ops(const char *name)
 	return NULL;
 }
 
+#ifdef CONFIG_ACPI
+/*
+ * Get a cpu's boot method in the ACPI way.
+ */
+static char * __init acpi_get_cpu_boot_method(void)
+{
+	/*
+	 * For ACPI 5.1, only two kind of methods are provided,
+	 * Parking protocol and PSCI, but Parking protocol is
+	 * specified for 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:
+		return "psci";
+	case ACPI_SMP_BOOT_PARKING_PROTOCOL:
+	default:
+		return NULL;
+	}
+}
+#else
+static inline char * __init acpi_get_cpu_boot_method(void) { return NULL; }
+#endif
+
 /*
- * Read a cpu's enable method from the device tree and record it in cpu_ops.
+ * Read a cpu's enable method and record it in cpu_ops.
  */
 int __init cpu_read_ops(struct device_node *dn, int cpu)
 {
-	const char *enable_method = of_get_property(dn, "enable-method", NULL);
+	const char *enable_method;
+
+	if (!acpi_disabled) {
+		enable_method = acpi_get_cpu_boot_method();
+		goto get_ops;
+	}
+
+	enable_method = of_get_property(dn, "enable-method", NULL);
 	if (!enable_method) {
 		/*
 		 * The boot CPU may not have an enable method (e.g. when
@@ -66,10 +100,17 @@  int __init cpu_read_ops(struct device_node *dn, int cpu)
 		return -ENOENT;
 	}
 
+get_ops:
 	cpu_ops[cpu] = cpu_get_ops(enable_method);
 	if (!cpu_ops[cpu]) {
-		pr_warn("%s: unsupported enable-method property: %s\n",
-			dn->full_name, enable_method);
+		if (acpi_disabled) {
+			pr_warn("%s: unsupported enable-method property: %s\n",
+				dn->full_name, enable_method);
+		} else {
+			pr_warn("CPU %d: boot protocol unsupported or unknown\n",
+				cpu);
+		}
+
 		return -EOPNOTSUPP;
 	}
 
@@ -78,7 +119,14 @@  int __init cpu_read_ops(struct device_node *dn, int cpu)
 
 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_read_ops(NULL, 0);
+		return;
+	}
+
+	dn = of_get_cpu_node(0, NULL);
 	if (!dn) {
 		pr_err("Failed to find device node for boot cpu\n");
 		return;
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 8f1d37c..e21bbc9 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;
@@ -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_read_ops(NULL, 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;