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.
*/ int __init cpu_read_ops(struct device_node *dn, int cpu) {
- Read a cpu's enable method and record it in cpu_ops.
- 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