On Mon, Mar 16, 2015 at 11:45:25AM +0000, Hanjun Guo wrote:

On 2015?03?13? 22:51, Lorenzo Pieralisi wrote:

quoted

On Wed, Mar 11, 2015 at 12:39:36PM +0000, Hanjun Guo wrote:

[...]

quoted

+static void __init psci_0_2_set_functions(void)
+{
+	pr_info("Using standard PSCI v0.2 function IDs\n");
+	psci_function_id[PSCI_FN_CPU_SUSPEND] = PSCI_0_2_FN64_CPU_SUSPEND;
+	psci_ops.cpu_suspend = psci_cpu_suspend;
+
+	psci_function_id[PSCI_FN_CPU_OFF] = PSCI_0_2_FN_CPU_OFF;
+	psci_ops.cpu_off = psci_cpu_off;
+
+	psci_function_id[PSCI_FN_CPU_ON] = PSCI_0_2_FN64_CPU_ON;
+	psci_ops.cpu_on = psci_cpu_on;
+
+	psci_function_id[PSCI_FN_MIGRATE] = PSCI_0_2_FN64_MIGRATE;
+	psci_ops.migrate = psci_migrate;
+
+	psci_function_id[PSCI_FN_AFFINITY_INFO] = PSCI_0_2_FN64_AFFINITY_INFO;
+	psci_ops.affinity_info = psci_affinity_info;
+
+	psci_function_id[PSCI_FN_MIGRATE_INFO_TYPE] =
+		PSCI_0_2_FN_MIGRATE_INFO_TYPE;
+	psci_ops.migrate_info_type = psci_migrate_info_type;
+
+	arm_pm_restart = psci_sys_reset;
+
+	pm_power_off = psci_sys_poweroff;
+}
+
  /*
   * PSCI Function IDs for v0.2+ are well defined so use
   * standard values.

@@ -306,29 +335,7 @@ static int __init psci_0_2_init(struct device_node *np)
  		}
  	}

-	pr_info("Using standard PSCI v0.2 function IDs\n");
-	psci_function_id[PSCI_FN_CPU_SUSPEND] = PSCI_0_2_FN64_CPU_SUSPEND;
-	psci_ops.cpu_suspend = psci_cpu_suspend;
-
-	psci_function_id[PSCI_FN_CPU_OFF] = PSCI_0_2_FN_CPU_OFF;
-	psci_ops.cpu_off = psci_cpu_off;
-
-	psci_function_id[PSCI_FN_CPU_ON] = PSCI_0_2_FN64_CPU_ON;
-	psci_ops.cpu_on = psci_cpu_on;
-
-	psci_function_id[PSCI_FN_MIGRATE] = PSCI_0_2_FN64_MIGRATE;
-	psci_ops.migrate = psci_migrate;
-
-	psci_function_id[PSCI_FN_AFFINITY_INFO] = PSCI_0_2_FN64_AFFINITY_INFO;
-	psci_ops.affinity_info = psci_affinity_info;
-
-	psci_function_id[PSCI_FN_MIGRATE_INFO_TYPE] =
-		PSCI_0_2_FN_MIGRATE_INFO_TYPE;
-	psci_ops.migrate_info_type = psci_migrate_info_type;
-
-	arm_pm_restart = psci_sys_reset;
-
-	pm_power_off = psci_sys_poweroff;
+	psci_0_2_set_functions();

You should have factored out the firmware version probing too, that's
the only way we can detect the PSCI firmware version when booting through
ACPI. You can end up initializing pointers for v0.2+ with a mismatching
version implemented in PSCI firmware, eg 0.1.

We should do that incrementally, I will put together a patch to
factor out the FW version probing first, you can rebase on top of it.

Incrementally patches on top of this patch set? I think v10 of this
patch set is ready for merge, but I'm open for suggestions if we will
not miss the merge window for Catalin.

I gave you my suggestion, I will try to get the prerequisite patch
queued asap, it is not a big deal but that's something that should be
fixed otherwise I would not have flagged this up.

I will post the patch asap, if we fail to get that in we will see what
to do, I do not expect this to be a blocking point.

quoted

quoted

  out_put_node:
  	of_node_put(np);

@@ -381,7 +388,7 @@ static const struct of_device_id psci_of_match[] __initconst = {
  	{},
  };

-int __init psci_init(void)
+int __init psci_dt_init(void)
  {
  	struct device_node *np;
  	const struct of_device_id *matched_np;

@@ -396,6 +403,29 @@ int __init psci_init(void)
  	return init_fn(np);
  }

+/*
+ * We use PSCI 0.2+ when ACPI is deployed on ARM64 and it's
+ * explicitly clarified in SBBR
+ */
+int __init psci_acpi_init(void)
+{
+	if (!acpi_psci_present()) {
+		pr_info("is not implemented in ACPI.\n");
+		return -EOPNOTSUPP;
+	}

If PSCI is not present, that's a problem related to SMP init, right ?
That's where a warning should be printed if any, not here, the SBBR
mandates PSCI as secondaries bring up method, warn otherwise.

The SBBR is also said that if PSCI is not available, Parking protocol
will be used as secondaries bring up method, so I said that it is ok
to me that we don't print warn message for no PSCI support when parsing
FADT.

So maybe we can go back to the previous solution, print some warning
message if no PSCI when parsing FADT?

You answered your own question. It is not what it is mandated, but if
a platform boots with parking protocol, do you think the information
you are printing in:

if (!acpi_psci_present()) {
	pr_info("is not implemented in ACPI.\n");
        ^^^

is useful to them ?

What should be flagged up is a missing boot method for secondaries,
a missing PSCI is not per-se an error, that's why I said it should
be done when preparing CPUs for SMP init.

No big deal at all, but I would remove the pr_info above.

Lorenzo