Re: [PATCHv3 1/6] arm/arm64: smccc/psci: add arm_smccc_1_1_get_conduit()

On Mon, Aug 12, 2019 at 04:06:35PM +0100, Mark Rutland wrote:

On Mon, Aug 12, 2019 at 04:03:29PM +0100, Dave Martin wrote:

quoted

On Fri, Aug 09, 2019 at 02:22:40PM +0100, Mark Rutland wrote:

quoted

SMCCC callers are currently amassing a collection of enums for the SMCCC
conduit, and are having to dig into the PSCI driver's internals in order
to figure out what to do.

Let's clean this up, with common SMCCC_CONDUIT_* definitions, and an
arm_smccc_1_1_get_conduit() helper that abstracts the PSCI driver's
internal state.

We can kill off the PSCI_CONDUIT_* definitions once we've migrated users
over to the new interface.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Acked-by: Lorenzo Pieralisi <redacted>
Acked-by: Will Deacon <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
---
 drivers/firmware/psci/psci.c | 15 +++++++++++++++
 include/linux/arm-smccc.h    | 16 ++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index f82ccd39a913..5f31f1bea1af 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c

@@ -57,6 +57,21 @@ struct psci_operations psci_ops = {
 	.smccc_version = SMCCC_VERSION_1_0,
 };
 
+enum arm_smccc_conduit arm_smccc_1_1_get_conduit(void)

Do we expect this to be specific to SMCCC v1.1?

I intend it to be 1.1+

It seems overspecific, but I guess we can address this later if it
becomes an issue.  This is an internal API for now (at worst I might
envisage it being EXPORT_SYMBOL_GPL()).

quoted

quoted

+{
+	if (psci_ops.smccc_version < SMCCC_VERSION_1_1)
+		return SMCCC_CONDUIT_NONE;
+
+	switch (psci_ops.conduit) {
+	case PSCI_CONDUIT_SMC:
+		return SMCCC_CONDUIT_SMC;
+	case PSCI_CONDUIT_HVC:
+		return SMCCC_CONDUIT_HVC;
+	default:
+		return SMCCC_CONDUIT_NONE;
+	}
+}
+
 typedef unsigned long (psci_fn)(unsigned long, unsigned long,
 				unsigned long, unsigned long);
 static psci_fn *invoke_psci_fn;

diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index 080012a6f025..df01a8579034 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h

@@ -80,6 +80,22 @@
 
 #include <linux/linkage.h>
 #include <linux/types.h>
+
+enum arm_smccc_conduit {
+	SMCCC_CONDUIT_NONE,

If this is intended to have the value 0, is it worth making that
explicit?  I can never remember whether enums start at 1 or 0 by
default...

They start at 0. I intend that checks are done explicitly against an
enum value, so I'm not sure that matters.

Not really.

It depends whether code like if (!arm_smccc_1_1_get_conduit()) { ... }
is considered sane or not.

If we don't think people should be doing this, omitting the explicit
value specifier seems fine.

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel