On 2014-8-19 2:34, Sudeep Holla wrote:

On 04/08/14 16:28, Hanjun Guo wrote:

[...]

quoted

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

[...]

quoted

+/* 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?

quoted

  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.

quoted

+}
+#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