[PATCH v3 0/7] firmware: smccc: Add basic SMCCC v1.2 + ARCH_SOC_ID support

[PATCH v3 0/7] firmware: smccc: Add basic SMCCC v1.2 + ARCH_SOC_ID support

[Sudeep Holla]

Hi,

This patch series adds support for SMCCCv1.2 ARCH_SOC_ID.
This doesn't add other changes added in SMCCC v1.2 yet. They will
follow these soon along with its first user SPCI/PSA-FF.

This is tested using upstream TF-A + the patch[2] fixing the original
implementation there.


v1[0]->v2[1]:
	- Incorporated comments from Steven Price in patch 5/5
	- Fixed build for CONFIG_PSCI_FW=n on some arm32 platforms
	- Added Steven Price's review tags

v2[1]->v3:
	- Incorporated additional comments from Steven Price in patch 5/5
	  and added his review tags
	- Refactored SMCCC code from PSCI and moved it under
	  drivers/firmware/smccc/smccc.c
	- Also moved soc_id.c under drivers/firmware/smccc

Regards,
Sudeep

[0] https://lore.kernel.org/r/20200430114814.14116-1-sudeep.holla@arm.com/
[1] https://lore.kernel.org/r/20200504092905.10580-1-sudeep.holla@arm.com/
[2] https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/4002

Sudeep Holla (7):
  firmware: smccc: Add HAVE_ARM_SMCCC_DISCOVERY to identify SMCCC v1.1 and above
  firmware: smccc: Update link to latest SMCCC specification
  firmware: smccc: Add the definition for SMCCCv1.2 version/error codes
  firmware: smccc: Drop smccc_version enum and use ARM_SMCCC_VERSION_1_x instead
  firmware: smccc: Refactor SMCCC specific bits into separate file
  firmware: smccc: Add function to fetch SMCCC version
  firmware: smccc: Add ARCH_SOC_ID support

 MAINTAINERS                     |   9 ++
 arch/arm64/kernel/paravirt.c    |   2 +-
 drivers/firmware/Kconfig        |   6 +-
 drivers/firmware/Makefile       |   3 +-
 drivers/firmware/psci/psci.c    |  21 +---
 drivers/firmware/smccc/Kconfig  |  25 +++++
 drivers/firmware/smccc/Makefile |   4 +
 drivers/firmware/smccc/smccc.c  |  30 ++++++
 drivers/firmware/smccc/soc_id.c | 168 ++++++++++++++++++++++++++++++++
 include/linux/arm-smccc.h       |  34 ++++++-
 include/linux/psci.h            |   7 --
 11 files changed, 278 insertions(+), 31 deletions(-)
 create mode 100644 drivers/firmware/smccc/Kconfig
 create mode 100644 drivers/firmware/smccc/Makefile
 create mode 100644 drivers/firmware/smccc/smccc.c
 create mode 100644 drivers/firmware/smccc/soc_id.c

-- 
2.17.1

[PATCH v3 1/7] firmware: smccc: Add HAVE_ARM_SMCCC_DISCOVERY to identify SMCCC v1.1 and above

[Sudeep Holla]

SMCCC v1.0 lacked discoverability of version and features. To accelerate
adoption of few mitigations and protect systems more rapidly from various
vulnerability, PSCI v1.0 was updated to add SMCCC discovery mechanism
though the PSCI firmware implementation of PSCI_FEATURES(SMCCC_VERSION)
which returns success on firmware compliant to SMCCC v1.1 and above.

This inturn makes SMCCC v1.1 and above dependent on ARM_PSCI_FW for
backward compatibility. Let us introduce a new hidden config for the
same to build more features on top of SMCCC v1.1 and above.

While at it, also sort alphabetically the psci entry.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/Kconfig       |  6 ++----
 drivers/firmware/smccc/Kconfig | 17 +++++++++++++++++
 2 files changed, 19 insertions(+), 4 deletions(-)
 create mode 100644 drivers/firmware/smccc/Kconfig

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 8007d4aa76dc..4843e94713a4 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -295,15 +295,13 @@ config TURRIS_MOX_RWTM
 	  other manufacturing data and also utilize the Entropy Bit Generator
 	  for hardware random number generation.
 
-config HAVE_ARM_SMCCC
-	bool
-
-source "drivers/firmware/psci/Kconfig"
 source "drivers/firmware/broadcom/Kconfig"
 source "drivers/firmware/google/Kconfig"
 source "drivers/firmware/efi/Kconfig"
 source "drivers/firmware/imx/Kconfig"
 source "drivers/firmware/meson/Kconfig"
+source "drivers/firmware/psci/Kconfig"
+source "drivers/firmware/smccc/Kconfig"
 source "drivers/firmware/tegra/Kconfig"
 source "drivers/firmware/xilinx/Kconfig"
 
diff --git a/drivers/firmware/smccc/Kconfig b/drivers/firmware/smccc/Kconfig
new file mode 100644
index 000000000000..d93f1ab52cb2
--- /dev/null
+++ b/drivers/firmware/smccc/Kconfig
@@ -0,0 +1,17 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config HAVE_ARM_SMCCC
+	bool
+	help
+	  Include support for the Secure Monitor Call (SMC) and Hypervisor
+	  Call (HVC) instructions on Armv7 and above architectures.
+
+config HAVE_ARM_SMCCC_DISCOVERY
+	bool
+	depends on ARM_PSCI_FW
+	default y
+	help
+	 SMCCC v1.0 lacked discoverability and hence PSCI v1.0 was updated
+	 to add SMCCC discovery mechanism though the PSCI firmware
+	 implementation of PSCI_FEATURES(SMCCC_VERSION) which returns
+	 success on firmware compliant to SMCCC v1.1 and above.
+
-- 
2.17.1

[PATCH v3 2/7] firmware: smccc: Update link to latest SMCCC specification

[Sudeep Holla]

The current link gets redirected to the revision B published in November
2016 though it actually points to the original revision A published in
June 2013.

Let us update the link to point to the latest version, so that it
doesn't get stale anytime soon. Currently it points to v1.2 published in
March 2020.

Reviewed-by: Steven Price <steven.price@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 include/linux/arm-smccc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index 59494df0f55b..6c1d1eda3be4 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -10,7 +10,7 @@
 /*
  * This file provides common defines for ARM SMC Calling Convention as
  * specified in
- * http://infocenter.arm.com/help/topic/com.arm.doc.den0028a/index.html
+ * https://developer.arm.com/docs/den0028/latest
  */
 
 #define ARM_SMCCC_STD_CALL	        _AC(0,U)
-- 
2.17.1

[PATCH v3 3/7] firmware: smccc: Add the definition for SMCCCv1.2 version/error codes

[Sudeep Holla]

Add the definition for SMCCC v1.2 version and new error code added.
While at it, also add a note that ARM DEN 0070A is deprecated and is
now merged into the main SMCCC specification(ARM DEN 0028C).

Reviewed-by: Steven Price <steven.price@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 include/linux/arm-smccc.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index 6c1d1eda3be4..9d9a2e42e919 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -56,6 +56,7 @@
 
 #define ARM_SMCCC_VERSION_1_0		0x10000
 #define ARM_SMCCC_VERSION_1_1		0x10001
+#define ARM_SMCCC_VERSION_1_2		0x10002
 
 #define ARM_SMCCC_VERSION_FUNC_ID					\
 	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,				\
@@ -314,10 +315,14 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
  */
 #define arm_smccc_1_1_hvc(...)	__arm_smccc_1_1(SMCCC_HVC_INST, __VA_ARGS__)
 
-/* Return codes defined in ARM DEN 0070A */
+/*
+ * Return codes defined in ARM DEN 0070A
+ * ARM DEN 0070A is now merged/consolidated into ARM DEN 0028C
+ */
 #define SMCCC_RET_SUCCESS			0
 #define SMCCC_RET_NOT_SUPPORTED			-1
 #define SMCCC_RET_NOT_REQUIRED			-2
+#define SMCCC_RET_INVALID_PARAMETER		-3
 
 /*
  * Like arm_smccc_1_1* but always returns SMCCC_RET_NOT_SUPPORTED.
-- 
2.17.1

[PATCH v3 4/7] firmware: smccc: Drop smccc_version enum and use ARM_SMCCC_VERSION_1_x instead

[Sudeep Holla]

Instead of maintaining 2 sets of enums/macros for tracking SMCCC version,
let us drop smccc_version enum and use ARM_SMCCC_VERSION_1_x directly
instead.

This is in preparation to drop smccc_version here and move it separately
under drivers/firmware/smccc.

Reviewed-by: Steven Price <steven.price@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/arm64/kernel/paravirt.c | 2 +-
 drivers/firmware/psci/psci.c | 8 ++++----
 include/linux/psci.h         | 7 +------
 3 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/kernel/paravirt.c b/arch/arm64/kernel/paravirt.c
index 1ef702b0be2d..295d66490584 100644
--- a/arch/arm64/kernel/paravirt.c
+++ b/arch/arm64/kernel/paravirt.c
@@ -120,7 +120,7 @@ static bool has_pv_steal_clock(void)
 	struct arm_smccc_res res;
 
 	/* To detect the presence of PV time support we require SMCCC 1.1+ */
-	if (psci_ops.smccc_version < SMCCC_VERSION_1_1)
+	if (arm_smccc_1_1_get_conduit() == SMCCC_CONDUIT_NONE)
 		return false;
 
 	arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_FEATURES_FUNC_ID,
diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index 2937d44b5df4..6a56d7196697 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -54,12 +54,12 @@ bool psci_tos_resident_on(int cpu)
 
 struct psci_operations psci_ops = {
 	.conduit = SMCCC_CONDUIT_NONE,
-	.smccc_version = SMCCC_VERSION_1_0,
+	.smccc_version = ARM_SMCCC_VERSION_1_0,
 };
 
 enum arm_smccc_conduit arm_smccc_1_1_get_conduit(void)
 {
-	if (psci_ops.smccc_version < SMCCC_VERSION_1_1)
+	if (psci_ops.smccc_version < ARM_SMCCC_VERSION_1_1)
 		return SMCCC_CONDUIT_NONE;
 
 	return psci_ops.conduit;
@@ -411,8 +411,8 @@ static void __init psci_init_smccc(void)
 	if (feature != PSCI_RET_NOT_SUPPORTED) {
 		u32 ret;
 		ret = invoke_psci_fn(ARM_SMCCC_VERSION_FUNC_ID, 0, 0, 0);
-		if (ret == ARM_SMCCC_VERSION_1_1) {
-			psci_ops.smccc_version = SMCCC_VERSION_1_1;
+		if (ret >= ARM_SMCCC_VERSION_1_1) {
+			psci_ops.smccc_version = ret;
 			ver = ret;
 		}
 	}
diff --git a/include/linux/psci.h b/include/linux/psci.h
index a67712b73b6c..29bd0671e5bb 100644
--- a/include/linux/psci.h
+++ b/include/linux/psci.h
@@ -21,11 +21,6 @@ bool psci_power_state_is_valid(u32 state);
 int psci_set_osi_mode(void);
 bool psci_has_osi_support(void);
 
-enum smccc_version {
-	SMCCC_VERSION_1_0,
-	SMCCC_VERSION_1_1,
-};
-
 struct psci_operations {
 	u32 (*get_version)(void);
 	int (*cpu_suspend)(u32 state, unsigned long entry_point);
@@ -36,7 +31,7 @@ struct psci_operations {
 			unsigned long lowest_affinity_level);
 	int (*migrate_info_type)(void);
 	enum arm_smccc_conduit conduit;
-	enum smccc_version smccc_version;
+	u32 smccc_version;
 };
 
 extern struct psci_operations psci_ops;
-- 
2.17.1

[PATCH v3 5/7] firmware: smccc: Refactor SMCCC specific bits into separate file

[Sudeep Holla]

In order to add newer SMCCC v1.1+ functionality and to avoid cluttering
PSCI firmware driver with SMCCC bits, let us move the SMCCC specific
details under drivers/firmware/smccc/smccc.c

We can also drop conduit and smccc_version from psci_operations structure
as SMCCC was the sole user and now it maintains those.

No functionality change in this patch though.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 MAINTAINERS                     |  9 +++++++++
 drivers/firmware/Makefile       |  3 ++-
 drivers/firmware/psci/psci.c    | 19 ++++---------------
 drivers/firmware/smccc/Makefile |  3 +++
 drivers/firmware/smccc/smccc.c  | 26 ++++++++++++++++++++++++++
 include/linux/arm-smccc.h       | 11 +++++++++++
 include/linux/psci.h            |  2 --
 7 files changed, 55 insertions(+), 18 deletions(-)
 create mode 100644 drivers/firmware/smccc/Makefile
 create mode 100644 drivers/firmware/smccc/smccc.c

Hi Mark, Lorenzo,

I have replicated PSCI entry in MAINTAINERS file and added myself to
for SMCCC entry. If you prefer I can merge it under PSCI. Let me know
your preference along with other review comments.

Regards,
Sudeep


diff --git a/MAINTAINERS b/MAINTAINERS
index 2926327e4976..f32b0dfa49be 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15474,6 +15474,15 @@ M:	Nicolas Pitre <nico@fluxnic.net>
 S:	Odd Fixes
 F:	drivers/net/ethernet/smsc/smc91x.*
 
+SECURE MONITOR CALL(SMC) CALLING CONVENTION (SMCCC)
+M:	Mark Rutland <mark.rutland@arm.com>
+M:	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
+M:	Sudeep Holla <sudeep.holla@arm.com>
+L:	linux-arm-kernel@lists.infradead.org
+S:	Maintained
+F:	drivers/firmware/smccc/
+F:	include/linux/arm-smccc.h
+
 SMIA AND SMIA++ IMAGE SENSOR DRIVER
 M:	Sakari Ailus <sakari.ailus@linux.intel.com>
 L:	linux-media@vger.kernel.org
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index e9fb838af4df..99510be9f5ed 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -23,12 +23,13 @@ obj-$(CONFIG_TRUSTED_FOUNDATIONS) += trusted_foundations.o
 obj-$(CONFIG_TURRIS_MOX_RWTM)	+= turris-mox-rwtm.o
 
 obj-$(CONFIG_ARM_SCMI_PROTOCOL)	+= arm_scmi/
-obj-y				+= psci/
 obj-y				+= broadcom/
 obj-y				+= meson/
 obj-$(CONFIG_GOOGLE_FIRMWARE)	+= google/
 obj-$(CONFIG_EFI)		+= efi/
 obj-$(CONFIG_UEFI_CPER)		+= efi/
 obj-y				+= imx/
+obj-y				+= psci/
+obj-y				+= smccc/
 obj-y				+= tegra/
 obj-y				+= xilinx/
diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index 6a56d7196697..92013ecc2d9e 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -46,25 +46,14 @@
  * require cooperation with a Trusted OS driver.
  */
 static int resident_cpu = -1;
+struct psci_operations psci_ops;
+static enum arm_smccc_conduit psci_conduit = SMCCC_CONDUIT_NONE;
 
 bool psci_tos_resident_on(int cpu)
 {
 	return cpu == resident_cpu;
 }
 
-struct psci_operations psci_ops = {
-	.conduit = SMCCC_CONDUIT_NONE,
-	.smccc_version = ARM_SMCCC_VERSION_1_0,
-};
-
-enum arm_smccc_conduit arm_smccc_1_1_get_conduit(void)
-{
-	if (psci_ops.smccc_version < ARM_SMCCC_VERSION_1_1)
-		return SMCCC_CONDUIT_NONE;
-
-	return psci_ops.conduit;
-}
-
 typedef unsigned long (psci_fn)(unsigned long, unsigned long,
 				unsigned long, unsigned long);
 static psci_fn *invoke_psci_fn;
@@ -242,7 +231,7 @@ static void set_conduit(enum arm_smccc_conduit conduit)
 		WARN(1, "Unexpected PSCI conduit %d\n", conduit);
 	}
 
-	psci_ops.conduit = conduit;
+	psci_conduit = conduit;
 }
 
 static int get_set_conduit_method(struct device_node *np)
@@ -412,7 +401,7 @@ static void __init psci_init_smccc(void)
 		u32 ret;
 		ret = invoke_psci_fn(ARM_SMCCC_VERSION_FUNC_ID, 0, 0, 0);
 		if (ret >= ARM_SMCCC_VERSION_1_1) {
-			psci_ops.smccc_version = ret;
+			arm_smccc_version_init(ret, psci_conduit);
 			ver = ret;
 		}
 	}
diff --git a/drivers/firmware/smccc/Makefile b/drivers/firmware/smccc/Makefile
new file mode 100644
index 000000000000..6f369fe3f0b9
--- /dev/null
+++ b/drivers/firmware/smccc/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+obj-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)	+= smccc.o
diff --git a/drivers/firmware/smccc/smccc.c b/drivers/firmware/smccc/smccc.c
new file mode 100644
index 000000000000..488699aae24f
--- /dev/null
+++ b/drivers/firmware/smccc/smccc.c
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2020 Arm Limited
+ */
+
+#define pr_fmt(fmt) "smccc: " fmt
+
+#include <linux/arm-smccc.h>
+
+static u32 smccc_version = ARM_SMCCC_VERSION_1_0;
+static enum arm_smccc_conduit smccc_conduit = SMCCC_CONDUIT_NONE;
+
+void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit)
+{
+	smccc_version = version;
+	smccc_conduit = conduit;
+}
+
+enum arm_smccc_conduit arm_smccc_1_1_get_conduit(void)
+{
+	if (smccc_version < ARM_SMCCC_VERSION_1_1)
+		return SMCCC_CONDUIT_NONE;
+
+	return smccc_conduit;
+}
+
diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index 9d9a2e42e919..11fb20bfa8f7 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -5,6 +5,7 @@
 #ifndef __LINUX_ARM_SMCCC_H
 #define __LINUX_ARM_SMCCC_H
 
+#include <linux/init.h>
 #include <uapi/linux/const.h>
 
 /*
@@ -89,6 +90,16 @@ enum arm_smccc_conduit {
 	SMCCC_CONDUIT_HVC,
 };
 
+/**
+ * arm_smccc_version_init() - Sets SMCCC version and conduit
+ * @version: SMCCC version v1.1 or above
+ * @conduit: SMCCC_CONDUIT_SMC or SMCCC_CONDUIT_HVC
+ *
+ * When SMCCCv1.1 or above is not present, defaults to ARM_SMCCC_VERSION_1_0
+ * and SMCCC_CONDUIT_NONE respectively.
+ */
+void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit);
+
 /**
  * arm_smccc_1_1_get_conduit()
  *
diff --git a/include/linux/psci.h b/include/linux/psci.h
index 29bd0671e5bb..14ad9b9ebcd6 100644
--- a/include/linux/psci.h
+++ b/include/linux/psci.h
@@ -30,8 +30,6 @@ struct psci_operations {
 	int (*affinity_info)(unsigned long target_affinity,
 			unsigned long lowest_affinity_level);
 	int (*migrate_info_type)(void);
-	enum arm_smccc_conduit conduit;
-	u32 smccc_version;
 };
 
 extern struct psci_operations psci_ops;
-- 
2.17.1

[PATCH v3 6/7] firmware: smccc: Add function to fetch SMCCC version

[Sudeep Holla]

For backward compatibility reasons, PSCI maintains SMCCC version as
SMCCC didn't provide ARM_SMCCC_VERSION_FUNC_ID until v1.1

Let us provide accessors to fetch the SMCCC version in PSCI so that
other SMCCC v1.1+ features can use it.

Reviewed-by: Steven Price <steven.price@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/smccc/smccc.c | 4 ++++
 include/linux/arm-smccc.h      | 9 +++++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/firmware/smccc/smccc.c b/drivers/firmware/smccc/smccc.c
index 488699aae24f..672974df0dfe 100644
--- a/drivers/firmware/smccc/smccc.c
+++ b/drivers/firmware/smccc/smccc.c
@@ -24,3 +24,7 @@ enum arm_smccc_conduit arm_smccc_1_1_get_conduit(void)
 	return smccc_conduit;
 }
 
+u32 arm_smccc_version_get(void)
+{
+	return smccc_version;
+}
diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index 11fb20bfa8f7..8dd54dad1ec5 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -109,6 +109,15 @@ void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit);
  */
 enum arm_smccc_conduit arm_smccc_1_1_get_conduit(void);
 
+/**
+ * arm_smccc_version_get()
+ *
+ * Returns the version to be used for SMCCCv1.1 or later.
+ *
+ * When SMCCCv1.1 or above is not present, assumes and returns SMCCCv1.0.
+ */
+u32 arm_smccc_version_get(void);
+
 /**
  * struct arm_smccc_res - Result from SMC/HVC call
  * @a0-a3 result values from registers 0 to 3
-- 
2.17.1

[PATCH v3 7/7] firmware: smccc: Add ARCH_SOC_ID support

[Sudeep Holla]

SMCCC v1.2 adds a new optional function SMCCC_ARCH_SOC_ID to obtain a
SiP defined SoC identification value. Add support for the same.

Also using the SoC bus infrastructure, let us expose the platform
specific SoC atrributes under sysfs. We also provide custom sysfs for
the vendor ID as JEP-106 bank and identification code.

Reviewed-by: Steven Price <steven.price@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/smccc/Kconfig  |   8 ++
 drivers/firmware/smccc/Makefile |   1 +
 drivers/firmware/smccc/soc_id.c | 168 ++++++++++++++++++++++++++++++++
 include/linux/arm-smccc.h       |   5 +
 4 files changed, 182 insertions(+)
 create mode 100644 drivers/firmware/smccc/soc_id.c

diff --git a/drivers/firmware/smccc/Kconfig b/drivers/firmware/smccc/Kconfig
index d93f1ab52cb2..15e7466179a6 100644
--- a/drivers/firmware/smccc/Kconfig
+++ b/drivers/firmware/smccc/Kconfig
@@ -15,3 +15,11 @@ config HAVE_ARM_SMCCC_DISCOVERY
 	 implementation of PSCI_FEATURES(SMCCC_VERSION) which returns
 	 success on firmware compliant to SMCCC v1.1 and above.
 
+config ARM_SMCCC_SOC_ID
+	bool "SoC bus device for the ARM SMCCC SOC_ID"
+	depends on HAVE_ARM_SMCCC_DISCOVERY
+	default y
+	select SOC_BUS
+	help
+	  Include support for the SoC bus on the ARM SMCCC firmware based
+	  platforms providing some sysfs information about the SoC variant.
diff --git a/drivers/firmware/smccc/Makefile b/drivers/firmware/smccc/Makefile
index 6f369fe3f0b9..72ab84042832 100644
--- a/drivers/firmware/smccc/Makefile
+++ b/drivers/firmware/smccc/Makefile
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0
 #
 obj-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)	+= smccc.o
+obj-$(CONFIG_ARM_SMCCC_SOC_ID)	+= soc_id.o
diff --git a/drivers/firmware/smccc/soc_id.c b/drivers/firmware/smccc/soc_id.c
new file mode 100644
index 000000000000..dc5dd3f1f59b
--- /dev/null
+++ b/drivers/firmware/smccc/soc_id.c
@@ -0,0 +1,168 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2020 Arm Limited
+ */
+
+#define pr_fmt(fmt) "SMCCC: SOC_ID: " fmt
+
+#include <linux/arm-smccc.h>
+#include <linux/bitfield.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/sys_soc.h>
+
+#define SMCCC_SOC_ID_JEP106_BANK_IDX_MASK	GENMASK(30, 24)
+/*
+ * As per the SMC Calling Convention specification v1.2 (ARM DEN 0028C)
+ * Section 7.4 SMCCC_ARCH_SOC_ID bits[23:16] are JEP-106 identification
+ * code with parity bit for the SiP. We can drop the parity bit.
+ */
+#define SMCCC_SOC_ID_JEP106_ID_CODE_MASK	GENMASK(22, 16)
+#define SMCCC_SOC_ID_IMP_DEF_SOC_ID_MASK	GENMASK(15, 0)
+
+/* The bank index is equal to the for continuation code bank number - 1 */
+#define JEP106_BANK_CONT_CODE(x)	\
+	(u8)(FIELD_GET(SMCCC_SOC_ID_JEP106_BANK_IDX_MASK, (x)) + 1)
+#define JEP106_ID_CODE(x)	\
+	(u8)(FIELD_GET(SMCCC_SOC_ID_JEP106_ID_CODE_MASK, (x)))
+#define IMP_DEF_SOC_ID(x)	\
+	(u16)(FIELD_GET(SMCCC_SOC_ID_IMP_DEF_SOC_ID_MASK, (x)))
+
+static int soc_id_version;
+static struct soc_device *soc_dev;
+static struct soc_device_attribute *soc_dev_attr;
+
+static int smccc_map_error_codes(unsigned long a0)
+{
+	if (a0 >= SMCCC_RET_SUCCESS)
+		return 0;
+	else if (a0 == SMCCC_RET_INVALID_PARAMETER)
+		return -EINVAL;
+	else if (a0 == SMCCC_RET_NOT_SUPPORTED)
+		return -EOPNOTSUPP;
+	return -EINVAL;
+}
+
+static int smccc_soc_id_support_check(void)
+{
+	struct arm_smccc_res res;
+
+	if (arm_smccc_1_1_get_conduit() == SMCCC_CONDUIT_NONE) {
+		pr_err("%s: invalid SMCCC conduit\n", __func__);
+		return -EOPNOTSUPP;
+	}
+
+	arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_FEATURES_FUNC_ID,
+			     ARM_SMCCC_ARCH_SOC_ID, &res);
+
+	return smccc_map_error_codes(res.a0);
+}
+
+static ssize_t
+jep106_cont_bank_code_show(struct device *dev, struct device_attribute *attr,
+			   char *buf)
+{
+	return sprintf(buf, "%02x\n", JEP106_BANK_CONT_CODE(soc_id_version));
+}
+
+static DEVICE_ATTR_RO(jep106_cont_bank_code);
+
+static ssize_t
+jep106_identification_code_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%02x\n", JEP106_ID_CODE(soc_id_version));
+}
+
+static DEVICE_ATTR_RO(jep106_identification_code);
+
+static struct attribute *jep106_id_attrs[] = {
+	&dev_attr_jep106_cont_bank_code.attr,
+	&dev_attr_jep106_identification_code.attr,
+	NULL
+};
+
+ATTRIBUTE_GROUPS(jep106_id);
+
+static int __init smccc_soc_init(void)
+{
+	struct device *dev;
+	int ret, soc_id_rev;
+	struct arm_smccc_res res;
+	static char soc_id_str[8], soc_id_rev_str[12];
+
+	if (arm_smccc_version_get() < ARM_SMCCC_VERSION_1_2)
+		return 0;
+
+	ret = smccc_soc_id_support_check();
+	if (ret) {
+		pr_info("Feature not implemented, skipping ....\n");
+		return 0;
+	}
+
+	arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_SOC_ID, 0, &res);
+
+	ret = smccc_map_error_codes(res.a0);
+	if (ret) {
+		pr_err("Failed to fetch version, Err = %d\n", ret);
+		return ret;
+	}
+
+	soc_id_version = res.a0;
+
+	arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_SOC_ID, 1, &res);
+
+	ret = smccc_map_error_codes(res.a0);
+	if (ret) {
+		pr_err("Failed to fetch revision, Err = %d\n", ret);
+		return ret;
+	}
+
+	soc_id_rev = res.a0;
+
+	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
+	if (!soc_dev_attr)
+		return -ENOMEM;
+
+	sprintf(soc_id_str, "0x%04x", IMP_DEF_SOC_ID(soc_id_version));
+	sprintf(soc_id_rev_str, "0x%08x", soc_id_rev);
+
+	soc_dev_attr->soc_id = soc_id_str;
+	soc_dev_attr->revision = soc_id_rev_str;
+
+	soc_dev = soc_device_register(soc_dev_attr);
+	if (IS_ERR(soc_dev)) {
+		ret = PTR_ERR(soc_dev);
+		goto free_soc;
+	}
+
+	dev = soc_device_to_device(soc_dev);
+
+	ret = devm_device_add_groups(dev, jep106_id_groups);
+	if (ret) {
+		dev_err(dev, "sysfs create failed: %d\n", ret);
+		goto unregister_soc;
+	}
+
+	pr_info("ID = %s Revision = %s\n", soc_dev_attr->soc_id,
+		soc_dev_attr->revision);
+
+	return 0;
+
+unregister_soc:
+	soc_device_unregister(soc_dev);
+free_soc:
+	kfree(soc_dev_attr);
+	return ret;
+}
+module_init(smccc_soc_init);
+
+static void __exit smccc_soc_exit(void)
+{
+	if (soc_dev)
+		soc_device_unregister(soc_dev);
+	kfree(soc_dev_attr);
+}
+module_exit(smccc_soc_exit);
diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index 8dd54dad1ec5..368dabe99d09 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -69,6 +69,11 @@
 			   ARM_SMCCC_SMC_32,				\
 			   0, 1)
 
+#define ARM_SMCCC_ARCH_SOC_ID						\
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,				\
+			   ARM_SMCCC_SMC_32,				\
+			   0, 2)
+
 #define ARM_SMCCC_ARCH_WORKAROUND_1					\
 	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,				\
 			   ARM_SMCCC_SMC_32,				\
-- 
2.17.1

RE: [PATCH v3 0/7] firmware: smccc: Add basic SMCCC v1.2 + ARCH_SOC_ID support

[Etienne Carriere]

> From: Sudeep Holla <sudeep.holla@arm.com>
>
> Hi,
>
> This patch series adds support for SMCCCv1.2 ARCH_SOC_ID.
> This doesn't add other changes added in SMCCC v1.2 yet. They will
> follow these soon along with its first user SPCI/PSA-FF.
>
> This is tested using upstream TF-A + the patch[2] fixing the original
> implementation there.
>
>
> v1[0]->v2[1]:
> - Incorporated comments from Steven Price in patch 5/5
> - Fixed build for CONFIG_PSCI_FW=n on some arm32 platforms
> - Added Steven Price's review tags
>
> v2[1]->v3:
> - Incorporated additional comments from Steven Price in patch 5/5
>  and added his review tags
> - Refactored SMCCC code from PSCI and moved it under
>  drivers/firmware/smccc/smccc.c
> - Also moved soc_id.c under drivers/firmware/smccc
>
> Regards,
> Sudeep

Hello Sudeep,

In case it helps. I have successfully tested the 7 patches series
on some platforms, playing a bit with few configurations.
Qemu emulator for arm64/cortex-a57 with TF-A (v2.x) as secure firmware.
Qemu emulator for arm/cortex-a15. OP-TEE (v3.x) as secure firmware.
A stm32mp15 device (arm/cortex-a7), tested both TF-A (v2.x) and
OP-TEE (3.7.0, 3.9.0-rc) as runtime secure firmware.

Helper functions arm_smccc_1_1_get_conduit()/arm_smccc_1_1_invoke() 
works as expected AFAICT. No regression seen with older secure
firmwares.

For the patches 1 to 6, as I poorly tested [v3,7/7] soc ids,
based on tag next-20200505 [1]:
Tested-by: Etienne Carriere <etienne.carriere@st.com>
Reviewed-by: Etienne Carriere <etienne.carriere@st.com>

For [v3,7/7] firmware: smccc: Add ARCH_SOC_ID support
Acked-by: Etienne Carriere <etienne.carriere@st.com>

[1] 7def1ef0f72c ("Add linux-next specific files for 20200505")

Regards,
Etienne

Re: [PATCH v3 0/7] firmware: smccc: Add basic SMCCC v1.2 + ARCH_SOC_ID support

[Sudeep Holla]

On Fri, May 15, 2020 at 09:50:32AM +0200, Etienne Carriere wrote:
> > From: Sudeep Holla <sudeep.holla@arm.com>
> >
> > Hi,
> >
> > This patch series adds support for SMCCCv1.2 ARCH_SOC_ID.
> > This doesn't add other changes added in SMCCC v1.2 yet. They will
> > follow these soon along with its first user SPCI/PSA-FF.
> >
> > This is tested using upstream TF-A + the patch[2] fixing the original
> > implementation there.
> >
> >
> > v1[0]->v2[1]:
> > - Incorporated comments from Steven Price in patch 5/5
> > - Fixed build for CONFIG_PSCI_FW=n on some arm32 platforms
> > - Added Steven Price's review tags
> >
> > v2[1]->v3:
> > - Incorporated additional comments from Steven Price in patch 5/5
> >  and added his review tags
> > - Refactored SMCCC code from PSCI and moved it under
> >  drivers/firmware/smccc/smccc.c
> > - Also moved soc_id.c under drivers/firmware/smccc
> >
> > Regards,
> > Sudeep
> 
> Hello Sudeep,
> 
> In case it helps. I have successfully tested the 7 patches series
> on some platforms, playing a bit with few configurations.
> Qemu emulator for arm64/cortex-a57 with TF-A (v2.x) as secure firmware.
> Qemu emulator for arm/cortex-a15. OP-TEE (v3.x) as secure firmware.
> A stm32mp15 device (arm/cortex-a7), tested both TF-A (v2.x) and
> OP-TEE (3.7.0, 3.9.0-rc) as runtime secure firmware.
> 
> Helper functions arm_smccc_1_1_get_conduit()/arm_smccc_1_1_invoke() 
> works as expected AFAICT. No regression seen with older secure
> firmwares.
> 
> For the patches 1 to 6, as I poorly tested [v3,7/7] soc ids,
> based on tag next-20200505 [1]:
> Tested-by: Etienne Carriere <etienne.carriere@st.com>
> Reviewed-by: Etienne Carriere <etienne.carriere@st.com>
> 
> For [v3,7/7] firmware: smccc: Add ARCH_SOC_ID support
> Acked-by: Etienne Carriere <etienne.carriere@st.com>
> 
> [1] 7def1ef0f72c ("Add linux-next specific files for 20200505")
> 

Thanks for review and testing Etienne. Much appreciated!

-- 
Regards,
Sudeep

Re: [PATCH v3 1/7] firmware: smccc: Add HAVE_ARM_SMCCC_DISCOVERY to identify SMCCC v1.1 and above

[Mark Rutland]

On Wed, May 06, 2020 at 05:44:05PM +0100, Sudeep Holla wrote:
> SMCCC v1.0 lacked discoverability of version and features. To accelerate
> adoption of few mitigations and protect systems more rapidly from various
> vulnerability, PSCI v1.0 was updated to add SMCCC discovery mechanism
> though the PSCI firmware implementation of PSCI_FEATURES(SMCCC_VERSION)
> which returns success on firmware compliant to SMCCC v1.1 and above.
> 
> This inturn makes SMCCC v1.1 and above dependent on ARM_PSCI_FW for
> backward compatibility. Let us introduce a new hidden config for the
> same to build more features on top of SMCCC v1.1 and above.
> 
> While at it, also sort alphabetically the psci entry.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

> ---
>  drivers/firmware/Kconfig       |  6 ++----
>  drivers/firmware/smccc/Kconfig | 17 +++++++++++++++++
>  2 files changed, 19 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/firmware/smccc/Kconfig
> 
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 8007d4aa76dc..4843e94713a4 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -295,15 +295,13 @@ config TURRIS_MOX_RWTM
>  	  other manufacturing data and also utilize the Entropy Bit Generator
>  	  for hardware random number generation.
>  
> -config HAVE_ARM_SMCCC
> -	bool
> -
> -source "drivers/firmware/psci/Kconfig"
>  source "drivers/firmware/broadcom/Kconfig"
>  source "drivers/firmware/google/Kconfig"
>  source "drivers/firmware/efi/Kconfig"
>  source "drivers/firmware/imx/Kconfig"
>  source "drivers/firmware/meson/Kconfig"
> +source "drivers/firmware/psci/Kconfig"
> +source "drivers/firmware/smccc/Kconfig"
>  source "drivers/firmware/tegra/Kconfig"
>  source "drivers/firmware/xilinx/Kconfig"
>  
> diff --git a/drivers/firmware/smccc/Kconfig b/drivers/firmware/smccc/Kconfig
> new file mode 100644
> index 000000000000..d93f1ab52cb2
> --- /dev/null
> +++ b/drivers/firmware/smccc/Kconfig
> @@ -0,0 +1,17 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config HAVE_ARM_SMCCC
> +	bool
> +	help
> +	  Include support for the Secure Monitor Call (SMC) and Hypervisor
> +	  Call (HVC) instructions on Armv7 and above architectures.
> +
> +config HAVE_ARM_SMCCC_DISCOVERY
> +	bool
> +	depends on ARM_PSCI_FW
> +	default y
> +	help
> +	 SMCCC v1.0 lacked discoverability and hence PSCI v1.0 was updated
> +	 to add SMCCC discovery mechanism though the PSCI firmware
> +	 implementation of PSCI_FEATURES(SMCCC_VERSION) which returns
> +	 success on firmware compliant to SMCCC v1.1 and above.
> +
> -- 
> 2.17.1
> 

Re: [PATCH v3 2/7] firmware: smccc: Update link to latest SMCCC specification

[Mark Rutland]

On Wed, May 06, 2020 at 05:44:06PM +0100, Sudeep Holla wrote:
> The current link gets redirected to the revision B published in November
> 2016 though it actually points to the original revision A published in
> June 2013.
> 
> Let us update the link to point to the latest version, so that it
> doesn't get stale anytime soon. Currently it points to v1.2 published in
> March 2020.
> 
> Reviewed-by: Steven Price <steven.price@arm.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Changing the link is fine, but could we also add a line to make it clear
which version of spec was written against, e.g.

| This code is up-to-date with version DEN 0028 A

... as that was previously implicit in the documentation link, and it
makes clear what the code is aware of and what it is trying to handle.
Iknow we'll have to update it periodically, but I think that's
worthwthile.

With that:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

> ---
>  include/linux/arm-smccc.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> index 59494df0f55b..6c1d1eda3be4 100644
> --- a/include/linux/arm-smccc.h
> +++ b/include/linux/arm-smccc.h
> @@ -10,7 +10,7 @@
>  /*
>   * This file provides common defines for ARM SMC Calling Convention as
>   * specified in
> - * http://infocenter.arm.com/help/topic/com.arm.doc.den0028a/index.html
> + * https://developer.arm.com/docs/den0028/latest
>   */
>  
>  #define ARM_SMCCC_STD_CALL	        _AC(0,U)
> -- 
> 2.17.1
> 

Re: [PATCH v3 3/7] firmware: smccc: Add the definition for SMCCCv1.2 version/error codes

[Mark Rutland]

On Wed, May 06, 2020 at 05:44:07PM +0100, Sudeep Holla wrote:
> Add the definition for SMCCC v1.2 version and new error code added.
> While at it, also add a note that ARM DEN 0070A is deprecated and is
> now merged into the main SMCCC specification(ARM DEN 0028C).
> 
> Reviewed-by: Steven Price <steven.price@arm.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Hmm... the SMCCC v1.2 doc still seems to be EAC rather than a final
release.

I don't expect that this would change, but I am a little hesitant to add
other stuff based on an unfinalized spec. Do we know when the final
release will be?

Thanks,
Mark.

> ---
>  include/linux/arm-smccc.h | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> index 6c1d1eda3be4..9d9a2e42e919 100644
> --- a/include/linux/arm-smccc.h
> +++ b/include/linux/arm-smccc.h
> @@ -56,6 +56,7 @@
>  
>  #define ARM_SMCCC_VERSION_1_0		0x10000
>  #define ARM_SMCCC_VERSION_1_1		0x10001
> +#define ARM_SMCCC_VERSION_1_2		0x10002
>  
>  #define ARM_SMCCC_VERSION_FUNC_ID					\
>  	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,				\
> @@ -314,10 +315,14 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
>   */
>  #define arm_smccc_1_1_hvc(...)	__arm_smccc_1_1(SMCCC_HVC_INST, __VA_ARGS__)
>  
> -/* Return codes defined in ARM DEN 0070A */
> +/*
> + * Return codes defined in ARM DEN 0070A
> + * ARM DEN 0070A is now merged/consolidated into ARM DEN 0028C
> + */
>  #define SMCCC_RET_SUCCESS			0
>  #define SMCCC_RET_NOT_SUPPORTED			-1
>  #define SMCCC_RET_NOT_REQUIRED			-2
> +#define SMCCC_RET_INVALID_PARAMETER		-3
>  
>  /*
>   * Like arm_smccc_1_1* but always returns SMCCC_RET_NOT_SUPPORTED.
> -- 
> 2.17.1
> 

Re: [PATCH v3 4/7] firmware: smccc: Drop smccc_version enum and use ARM_SMCCC_VERSION_1_x instead

[Mark Rutland]

On Wed, May 06, 2020 at 05:44:08PM +0100, Sudeep Holla wrote:
> Instead of maintaining 2 sets of enums/macros for tracking SMCCC version,
> let us drop smccc_version enum and use ARM_SMCCC_VERSION_1_x directly
> instead.
> 
> This is in preparation to drop smccc_version here and move it separately
> under drivers/firmware/smccc.
> 
> Reviewed-by: Steven Price <steven.price@arm.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

> ---
>  arch/arm64/kernel/paravirt.c | 2 +-
>  drivers/firmware/psci/psci.c | 8 ++++----
>  include/linux/psci.h         | 7 +------
>  3 files changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/kernel/paravirt.c b/arch/arm64/kernel/paravirt.c
> index 1ef702b0be2d..295d66490584 100644
> --- a/arch/arm64/kernel/paravirt.c
> +++ b/arch/arm64/kernel/paravirt.c
> @@ -120,7 +120,7 @@ static bool has_pv_steal_clock(void)
>  	struct arm_smccc_res res;
>  
>  	/* To detect the presence of PV time support we require SMCCC 1.1+ */
> -	if (psci_ops.smccc_version < SMCCC_VERSION_1_1)
> +	if (arm_smccc_1_1_get_conduit() == SMCCC_CONDUIT_NONE)
>  		return false;
>  
>  	arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_FEATURES_FUNC_ID,
> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> index 2937d44b5df4..6a56d7196697 100644
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
> @@ -54,12 +54,12 @@ bool psci_tos_resident_on(int cpu)
>  
>  struct psci_operations psci_ops = {
>  	.conduit = SMCCC_CONDUIT_NONE,
> -	.smccc_version = SMCCC_VERSION_1_0,
> +	.smccc_version = ARM_SMCCC_VERSION_1_0,
>  };
>  
>  enum arm_smccc_conduit arm_smccc_1_1_get_conduit(void)
>  {
> -	if (psci_ops.smccc_version < SMCCC_VERSION_1_1)
> +	if (psci_ops.smccc_version < ARM_SMCCC_VERSION_1_1)
>  		return SMCCC_CONDUIT_NONE;
>  
>  	return psci_ops.conduit;
> @@ -411,8 +411,8 @@ static void __init psci_init_smccc(void)
>  	if (feature != PSCI_RET_NOT_SUPPORTED) {
>  		u32 ret;
>  		ret = invoke_psci_fn(ARM_SMCCC_VERSION_FUNC_ID, 0, 0, 0);
> -		if (ret == ARM_SMCCC_VERSION_1_1) {
> -			psci_ops.smccc_version = SMCCC_VERSION_1_1;
> +		if (ret >= ARM_SMCCC_VERSION_1_1) {
> +			psci_ops.smccc_version = ret;
>  			ver = ret;
>  		}
>  	}
> diff --git a/include/linux/psci.h b/include/linux/psci.h
> index a67712b73b6c..29bd0671e5bb 100644
> --- a/include/linux/psci.h
> +++ b/include/linux/psci.h
> @@ -21,11 +21,6 @@ bool psci_power_state_is_valid(u32 state);
>  int psci_set_osi_mode(void);
>  bool psci_has_osi_support(void);
>  
> -enum smccc_version {
> -	SMCCC_VERSION_1_0,
> -	SMCCC_VERSION_1_1,
> -};
> -
>  struct psci_operations {
>  	u32 (*get_version)(void);
>  	int (*cpu_suspend)(u32 state, unsigned long entry_point);
> @@ -36,7 +31,7 @@ struct psci_operations {
>  			unsigned long lowest_affinity_level);
>  	int (*migrate_info_type)(void);
>  	enum arm_smccc_conduit conduit;
> -	enum smccc_version smccc_version;
> +	u32 smccc_version;
>  };
>  
>  extern struct psci_operations psci_ops;
> -- 
> 2.17.1
> 

Re: [PATCH v3 5/7] firmware: smccc: Refactor SMCCC specific bits into separate file

[Mark Rutland]

On Wed, May 06, 2020 at 05:44:09PM +0100, Sudeep Holla wrote:
> In order to add newer SMCCC v1.1+ functionality and to avoid cluttering
> PSCI firmware driver with SMCCC bits, let us move the SMCCC specific
> details under drivers/firmware/smccc/smccc.c
> 
> We can also drop conduit and smccc_version from psci_operations structure
> as SMCCC was the sole user and now it maintains those.
> 
> No functionality change in this patch though.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  MAINTAINERS                     |  9 +++++++++
>  drivers/firmware/Makefile       |  3 ++-
>  drivers/firmware/psci/psci.c    | 19 ++++---------------
>  drivers/firmware/smccc/Makefile |  3 +++
>  drivers/firmware/smccc/smccc.c  | 26 ++++++++++++++++++++++++++
>  include/linux/arm-smccc.h       | 11 +++++++++++
>  include/linux/psci.h            |  2 --
>  7 files changed, 55 insertions(+), 18 deletions(-)
>  create mode 100644 drivers/firmware/smccc/Makefile
>  create mode 100644 drivers/firmware/smccc/smccc.c
> 
> Hi Mark, Lorenzo,
> 
> I have replicated PSCI entry in MAINTAINERS file and added myself to
> for SMCCC entry. If you prefer I can merge it under PSCI. Let me know
> your preference along with other review comments.

> +SECURE MONITOR CALL(SMC) CALLING CONVENTION (SMCCC)
> +M:	Mark Rutland <mark.rutland@arm.com>
> +M:	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> +M:	Sudeep Holla <sudeep.holla@arm.com>
> +L:	linux-arm-kernel@lists.infradead.org
> +S:	Maintained
> +F:	drivers/firmware/smccc/
> +F:	include/linux/arm-smccc.h

As per the above, I'm fine with having this separate from the PSCI
entry, and I'm fine with sharing this maintainership.

> diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h

> +/**
> + * arm_smccc_version_init() - Sets SMCCC version and conduit
> + * @version: SMCCC version v1.1 or above
> + * @conduit: SMCCC_CONDUIT_SMC or SMCCC_CONDUIT_HVC
> + *
> + * When SMCCCv1.1 or above is not present, defaults to ARM_SMCCC_VERSION_1_0
> + * and SMCCC_CONDUIT_NONE respectively.
> + */
> +void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit);

Given we only expect the PSCI code to call this, could we avoid putting
this in a header and just define it within psci.c?

Either way:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

Re: [PATCH v3 6/7] firmware: smccc: Add function to fetch SMCCC version

[Mark Rutland]

On Wed, May 06, 2020 at 05:44:10PM +0100, Sudeep Holla wrote:
> For backward compatibility reasons, PSCI maintains SMCCC version as
> SMCCC didn't provide ARM_SMCCC_VERSION_FUNC_ID until v1.1
> 
> Let us provide accessors to fetch the SMCCC version in PSCI so that
> other SMCCC v1.1+ features can use it.

Stale commit message? This was factored out of PSCI in the prior commit.

> Reviewed-by: Steven Price <steven.price@arm.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/smccc/smccc.c | 4 ++++
>  include/linux/arm-smccc.h      | 9 +++++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/firmware/smccc/smccc.c b/drivers/firmware/smccc/smccc.c
> index 488699aae24f..672974df0dfe 100644
> --- a/drivers/firmware/smccc/smccc.c
> +++ b/drivers/firmware/smccc/smccc.c
> @@ -24,3 +24,7 @@ enum arm_smccc_conduit arm_smccc_1_1_get_conduit(void)
>  	return smccc_conduit;
>  }
>  
> +u32 arm_smccc_version_get(void)
> +{
> +	return smccc_version;
> +}

Could we please call this arm_smccc_get_version(), to align with the
existing arm_smccc_1_1_get_conduit()?

> diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> index 11fb20bfa8f7..8dd54dad1ec5 100644
> --- a/include/linux/arm-smccc.h
> +++ b/include/linux/arm-smccc.h
> @@ -109,6 +109,15 @@ void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit);
>   */
>  enum arm_smccc_conduit arm_smccc_1_1_get_conduit(void);
>  
> +/**
> + * arm_smccc_version_get()
> + *
> + * Returns the version to be used for SMCCCv1.1 or later.
> + *
> + * When SMCCCv1.1 or above is not present, assumes and returns SMCCCv1.0.
> + */
> +u32 arm_smccc_version_get(void);

Can we please reword the last line to something like:

| When SMCCCv1.1 or above is not present, returns SMCCCv1.0, but this
| does not imply the presence of firmware or a valid conduit. Callers
| handling SMCCCv1.0 must determine the conduit by other means.

With all that:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

Re: [PATCH v3 2/7] firmware: smccc: Update link to latest SMCCC specification

[Sudeep Holla]

On Fri, May 15, 2020 at 12:37:44PM +0100, Mark Rutland wrote:
> On Wed, May 06, 2020 at 05:44:06PM +0100, Sudeep Holla wrote:
> > The current link gets redirected to the revision B published in November
> > 2016 though it actually points to the original revision A published in
> > June 2013.
> >
> > Let us update the link to point to the latest version, so that it
> > doesn't get stale anytime soon. Currently it points to v1.2 published in
> > March 2020.
> >
> > Reviewed-by: Steven Price <steven.price@arm.com>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>
> Changing the link is fine, but could we also add a line to make it clear
> which version of spec was written against, e.g.
>
> | This code is up-to-date with version DEN 0028 A
>
> ... as that was previously implicit in the documentation link, and it
> makes clear what the code is aware of and what it is trying to handle.
> Iknow we'll have to update it periodically, but I think that's
> worthwthile.
>

Makes sense, I will update.

> With that:
>
> Acked-by: Mark Rutland <mark.rutland@arm.com>
>

Thanks.

--
Regards,
Sudeep

Re: [PATCH v3 7/7] firmware: smccc: Add ARCH_SOC_ID support

[Mark Rutland]

On Wed, May 06, 2020 at 05:44:11PM +0100, Sudeep Holla wrote:
> SMCCC v1.2 adds a new optional function SMCCC_ARCH_SOC_ID to obtain a
> SiP defined SoC identification value. Add support for the same.
> 
> Also using the SoC bus infrastructure, let us expose the platform
> specific SoC atrributes under sysfs. We also provide custom sysfs for
> the vendor ID as JEP-106 bank and identification code.
> 
> Reviewed-by: Steven Price <steven.price@arm.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

As with the earlier patch referring to SMCCCv1.2 specifically, I'm
somewhat wary of this until the SMCCCv1.2 spec is final. Do we know what
the timeline is for that?

That needn't hold up the pure refactoring/cleanup portions of this
series, and regardless I have some comments below on this patch.

> ---
>  drivers/firmware/smccc/Kconfig  |   8 ++
>  drivers/firmware/smccc/Makefile |   1 +
>  drivers/firmware/smccc/soc_id.c | 168 ++++++++++++++++++++++++++++++++
>  include/linux/arm-smccc.h       |   5 +
>  4 files changed, 182 insertions(+)
>  create mode 100644 drivers/firmware/smccc/soc_id.c
> 
> diff --git a/drivers/firmware/smccc/Kconfig b/drivers/firmware/smccc/Kconfig
> index d93f1ab52cb2..15e7466179a6 100644
> --- a/drivers/firmware/smccc/Kconfig
> +++ b/drivers/firmware/smccc/Kconfig
> @@ -15,3 +15,11 @@ config HAVE_ARM_SMCCC_DISCOVERY
>  	 implementation of PSCI_FEATURES(SMCCC_VERSION) which returns
>  	 success on firmware compliant to SMCCC v1.1 and above.
>  
> +config ARM_SMCCC_SOC_ID
> +	bool "SoC bus device for the ARM SMCCC SOC_ID"
> +	depends on HAVE_ARM_SMCCC_DISCOVERY
> +	default y
> +	select SOC_BUS
> +	help
> +	  Include support for the SoC bus on the ARM SMCCC firmware based
> +	  platforms providing some sysfs information about the SoC variant.
> diff --git a/drivers/firmware/smccc/Makefile b/drivers/firmware/smccc/Makefile
> index 6f369fe3f0b9..72ab84042832 100644
> --- a/drivers/firmware/smccc/Makefile
> +++ b/drivers/firmware/smccc/Makefile
> @@ -1,3 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0
>  #
>  obj-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)	+= smccc.o
> +obj-$(CONFIG_ARM_SMCCC_SOC_ID)	+= soc_id.o
> diff --git a/drivers/firmware/smccc/soc_id.c b/drivers/firmware/smccc/soc_id.c
> new file mode 100644
> index 000000000000..dc5dd3f1f59b
> --- /dev/null
> +++ b/drivers/firmware/smccc/soc_id.c
> @@ -0,0 +1,168 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2020 Arm Limited
> + */
> +
> +#define pr_fmt(fmt) "SMCCC: SOC_ID: " fmt
> +
> +#include <linux/arm-smccc.h>
> +#include <linux/bitfield.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/sys_soc.h>
> +
> +#define SMCCC_SOC_ID_JEP106_BANK_IDX_MASK	GENMASK(30, 24)
> +/*
> + * As per the SMC Calling Convention specification v1.2 (ARM DEN 0028C)
> + * Section 7.4 SMCCC_ARCH_SOC_ID bits[23:16] are JEP-106 identification
> + * code with parity bit for the SiP. We can drop the parity bit.
> + */
> +#define SMCCC_SOC_ID_JEP106_ID_CODE_MASK	GENMASK(22, 16)
> +#define SMCCC_SOC_ID_IMP_DEF_SOC_ID_MASK	GENMASK(15, 0)
> +
> +/* The bank index is equal to the for continuation code bank number - 1 */
> +#define JEP106_BANK_CONT_CODE(x)	\
> +	(u8)(FIELD_GET(SMCCC_SOC_ID_JEP106_BANK_IDX_MASK, (x)) + 1)
> +#define JEP106_ID_CODE(x)	\
> +	(u8)(FIELD_GET(SMCCC_SOC_ID_JEP106_ID_CODE_MASK, (x)))
> +#define IMP_DEF_SOC_ID(x)	\
> +	(u16)(FIELD_GET(SMCCC_SOC_ID_IMP_DEF_SOC_ID_MASK, (x)))
> +
> +static int soc_id_version;
> +static struct soc_device *soc_dev;
> +static struct soc_device_attribute *soc_dev_attr;
> +
> +static int smccc_map_error_codes(unsigned long a0)
> +{
> +	if (a0 >= SMCCC_RET_SUCCESS)
> +		return 0;

As SMCCC_RET_SUCCESS is 0, and a0 is an unsigned long, this condition is
true for all values of a0.

I don't think this function is particularly helpful, since in the case
of a failure it'd be nicer to log the original FW error code to dmesg
for debugging, so we may as well leave it to the caller to check for
the EOPNOTSUPP / error cases.

e.g. where NOT_SUPPORTED is legitimate callers can do:

	unsigned long ret = smccc_call_foo(...);
	if (ret == SMCCC_RET_NOT_SUPPORTED)
		return 0;
	if ((int)ret < 0) 
		pr_err("FOO returned %lx\n", ret)
		return -EINVAL;
	}

	consume_ret_somehow(ret);

... and where NOT_SUPPORTED is not legitimate, they can avoid the
explicit check for that:

	unsigned long ret = smccc_call_foo(...);
	if ((int)ret < 0) {
		pr_err("FOO returned %lx\n", ret)
		return -EINVAL;
	}

	consume_ret_somehow(ret);

> +	else if (a0 == SMCCC_RET_INVALID_PARAMETER)
> +		return -EINVAL;
> +	else if (a0 == SMCCC_RET_NOT_SUPPORTED)
> +		return -EOPNOTSUPP;
> +	return -EINVAL;
> +}
> +
> +static int smccc_soc_id_support_check(void)
> +{
> +	struct arm_smccc_res res;
> +
> +	if (arm_smccc_1_1_get_conduit() == SMCCC_CONDUIT_NONE) {
> +		pr_err("%s: invalid SMCCC conduit\n", __func__);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_FEATURES_FUNC_ID,
> +			     ARM_SMCCC_ARCH_SOC_ID, &res);
> +
> +	return smccc_map_error_codes(res.a0);
> +}

Which I think means we may as well get rid of this, too, and fold the
conduit check into the start of smccc_soc_init().

> +
> +static ssize_t
> +jep106_cont_bank_code_show(struct device *dev, struct device_attribute *attr,
> +			   char *buf)
> +{
> +	return sprintf(buf, "%02x\n", JEP106_BANK_CONT_CODE(soc_id_version));
> +}

For the regs/identification values, we added a '0x' prefix to make it
explicit that the values are hex. Can/should we do that here, or is that
against conventions for soc bus files?

> +
> +static DEVICE_ATTR_RO(jep106_cont_bank_code);
> +
> +static ssize_t
> +jep106_identification_code_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "%02x\n", JEP106_ID_CODE(soc_id_version));
> +}
> +
> +static DEVICE_ATTR_RO(jep106_identification_code);
> +
> +static struct attribute *jep106_id_attrs[] = {
> +	&dev_attr_jep106_cont_bank_code.attr,
> +	&dev_attr_jep106_identification_code.attr,
> +	NULL
> +};
> +
> +ATTRIBUTE_GROUPS(jep106_id);
> +
> +static int __init smccc_soc_init(void)
> +{
> +	struct device *dev;
> +	int ret, soc_id_rev;
> +	struct arm_smccc_res res;
> +	static char soc_id_str[8], soc_id_rev_str[12];
> +
> +	if (arm_smccc_version_get() < ARM_SMCCC_VERSION_1_2)
> +		return 0;
> +
> +	ret = smccc_soc_id_support_check();
> +	if (ret) {
> +		pr_info("Feature not implemented, skipping ....\n");
> +		return 0;
> +	}

As above, I think we should expand smccc_soc_id_support_check() inline
here.

> +
> +	arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_SOC_ID, 0, &res);
> +
> +	ret = smccc_map_error_codes(res.a0);
> +	if (ret) {
> +		pr_err("Failed to fetch version, Err = %d\n", ret);
> +		return ret;
> +	}
> +
> +	soc_id_version = res.a0;

... and I think this'd be clearer like:

	arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_SOC_ID, 0, &res);
	if ((int)res.a0) {
		pr_err("ARCH_SOC_ID(0) returned error: %lx\n", res.a0); 
		return -EINVAL;
	}

> +	arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_SOC_ID, 1, &res);
> +
> +	ret = smccc_map_error_codes(res.a0);
> +	if (ret) {
> +		pr_err("Failed to fetch revision, Err = %d\n", ret);
> +		return ret;
> +	}

... likewise:

	arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_SOC_ID, 1, &res);
	if ((int)res.a0) {
		pr_err("ARCH_SOC_ID(1) returned error: %lx\n", res.a0); 
		return -EINVAL;
	}

> +
> +	soc_id_rev = res.a0;
> +
> +	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> +	if (!soc_dev_attr)
> +		return -ENOMEM;
> +
> +	sprintf(soc_id_str, "0x%04x", IMP_DEF_SOC_ID(soc_id_version));
> +	sprintf(soc_id_rev_str, "0x%08x", soc_id_rev);
> +
> +	soc_dev_attr->soc_id = soc_id_str;
> +	soc_dev_attr->revision = soc_id_rev_str;

Is there any expected format for these? e.g. would it make sense to add
a prefix showing that these are JEP codes?

Do we need to care about platform code which might also regsiter a soc
device? ... or do we expect those to be mutually exclusive?

Thanks,
Mark.

> +
> +	soc_dev = soc_device_register(soc_dev_attr);
> +	if (IS_ERR(soc_dev)) {
> +		ret = PTR_ERR(soc_dev);
> +		goto free_soc;
> +	}
> +
> +	dev = soc_device_to_device(soc_dev);
> +
> +	ret = devm_device_add_groups(dev, jep106_id_groups);
> +	if (ret) {
> +		dev_err(dev, "sysfs create failed: %d\n", ret);
> +		goto unregister_soc;
> +	}
> +
> +	pr_info("ID = %s Revision = %s\n", soc_dev_attr->soc_id,
> +		soc_dev_attr->revision);
> +
> +	return 0;
> +
> +unregister_soc:
> +	soc_device_unregister(soc_dev);
> +free_soc:
> +	kfree(soc_dev_attr);
> +	return ret;
> +}
> +module_init(smccc_soc_init);
> +
> +static void __exit smccc_soc_exit(void)
> +{
> +	if (soc_dev)
> +		soc_device_unregister(soc_dev);
> +	kfree(soc_dev_attr);
> +}
> +module_exit(smccc_soc_exit);
> diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> index 8dd54dad1ec5..368dabe99d09 100644
> --- a/include/linux/arm-smccc.h
> +++ b/include/linux/arm-smccc.h
> @@ -69,6 +69,11 @@
>  			   ARM_SMCCC_SMC_32,				\
>  			   0, 1)
>  
> +#define ARM_SMCCC_ARCH_SOC_ID						\
> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,				\
> +			   ARM_SMCCC_SMC_32,				\
> +			   0, 2)
> +
>  #define ARM_SMCCC_ARCH_WORKAROUND_1					\
>  	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,				\
>  			   ARM_SMCCC_SMC_32,				\
> -- 
> 2.17.1
> 

Re: [PATCH v3 5/7] firmware: smccc: Refactor SMCCC specific bits into separate file

[Sudeep Holla]

On Fri, May 15, 2020 at 12:49:53PM +0100, Mark Rutland wrote:
> On Wed, May 06, 2020 at 05:44:09PM +0100, Sudeep Holla wrote:
> > In order to add newer SMCCC v1.1+ functionality and to avoid cluttering
> > PSCI firmware driver with SMCCC bits, let us move the SMCCC specific
> > details under drivers/firmware/smccc/smccc.c
> >
> > We can also drop conduit and smccc_version from psci_operations structure
> > as SMCCC was the sole user and now it maintains those.
> >
> > No functionality change in this patch though.
> >
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  MAINTAINERS                     |  9 +++++++++
> >  drivers/firmware/Makefile       |  3 ++-
> >  drivers/firmware/psci/psci.c    | 19 ++++---------------
> >  drivers/firmware/smccc/Makefile |  3 +++
> >  drivers/firmware/smccc/smccc.c  | 26 ++++++++++++++++++++++++++
> >  include/linux/arm-smccc.h       | 11 +++++++++++
> >  include/linux/psci.h            |  2 --
> >  7 files changed, 55 insertions(+), 18 deletions(-)
> >  create mode 100644 drivers/firmware/smccc/Makefile
> >  create mode 100644 drivers/firmware/smccc/smccc.c
> >
> > Hi Mark, Lorenzo,
> >
> > I have replicated PSCI entry in MAINTAINERS file and added myself to
> > for SMCCC entry. If you prefer I can merge it under PSCI. Let me know
> > your preference along with other review comments.
>
> > +SECURE MONITOR CALL(SMC) CALLING CONVENTION (SMCCC)
> > +M:	Mark Rutland <mark.rutland@arm.com>
> > +M:	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > +M:	Sudeep Holla <sudeep.holla@arm.com>
> > +L:	linux-arm-kernel@lists.infradead.org
> > +S:	Maintained
> > +F:	drivers/firmware/smccc/
> > +F:	include/linux/arm-smccc.h
>
> As per the above, I'm fine with having this separate from the PSCI
> entry, and I'm fine with sharing this maintainership.
>

Thanks.

> > diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
>
> > +/**
> > + * arm_smccc_version_init() - Sets SMCCC version and conduit
> > + * @version: SMCCC version v1.1 or above
> > + * @conduit: SMCCC_CONDUIT_SMC or SMCCC_CONDUIT_HVC
> > + *
> > + * When SMCCCv1.1 or above is not present, defaults to ARM_SMCCC_VERSION_1_0
> > + * and SMCCC_CONDUIT_NONE respectively.
> > + */
> > +void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit);
>
> Given we only expect the PSCI code to call this, could we avoid putting
> this in a header and just define it within psci.c?
>

Sure I will do that, I was playing on safer side to avoid any tools picking on
such trivial things 😄

> Either way:
>
> Acked-by: Mark Rutland <mark.rutland@arm.com>
>

Thanks again.

--
Regards,
Sudeep

Re: [PATCH v3 6/7] firmware: smccc: Add function to fetch SMCCC version

[Sudeep Holla]

On Fri, May 15, 2020 at 01:08:11PM +0100, Mark Rutland wrote:
> On Wed, May 06, 2020 at 05:44:10PM +0100, Sudeep Holla wrote:
> > For backward compatibility reasons, PSCI maintains SMCCC version as
> > SMCCC didn't provide ARM_SMCCC_VERSION_FUNC_ID until v1.1
> > 
> > Let us provide accessors to fetch the SMCCC version in PSCI so that
> > other SMCCC v1.1+ features can use it.
> 
> Stale commit message? This was factored out of PSCI in the prior commit.
>

Duh ! Will drop that.

> > Reviewed-by: Steven Price <steven.price@arm.com>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  drivers/firmware/smccc/smccc.c | 4 ++++
> >  include/linux/arm-smccc.h      | 9 +++++++++
> >  2 files changed, 13 insertions(+)
> > 
> > diff --git a/drivers/firmware/smccc/smccc.c b/drivers/firmware/smccc/smccc.c
> > index 488699aae24f..672974df0dfe 100644
> > --- a/drivers/firmware/smccc/smccc.c
> > +++ b/drivers/firmware/smccc/smccc.c
> > @@ -24,3 +24,7 @@ enum arm_smccc_conduit arm_smccc_1_1_get_conduit(void)
> >  	return smccc_conduit;
> >  }
> >  
> > +u32 arm_smccc_version_get(void)
> > +{
> > +	return smccc_version;
> > +}
> 
> Could we please call this arm_smccc_get_version(), to align with the
> existing arm_smccc_1_1_get_conduit()?
>

Right will fix that. (I may suddenly got into SCMI mode where Greg or
someone asked me change all the function names to have verb at the end 😁)

> > diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> > index 11fb20bfa8f7..8dd54dad1ec5 100644
> > --- a/include/linux/arm-smccc.h
> > +++ b/include/linux/arm-smccc.h
> > @@ -109,6 +109,15 @@ void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit);
> >   */
> >  enum arm_smccc_conduit arm_smccc_1_1_get_conduit(void);
> >  
> > +/**
> > + * arm_smccc_version_get()
> > + *
> > + * Returns the version to be used for SMCCCv1.1 or later.
> > + *
> > + * When SMCCCv1.1 or above is not present, assumes and returns SMCCCv1.0.
> > + */
> > +u32 arm_smccc_version_get(void);
> 
> Can we please reword the last line to something like:
>
> | When SMCCCv1.1 or above is not present, returns SMCCCv1.0, but this
> | does not imply the presence of firmware or a valid conduit. Callers
> | handling SMCCCv1.0 must determine the conduit by other means.
>

Sure

> With all that:
> 
> Acked-by: Mark Rutland <mark.rutland@arm.com>
> 

Thanks,

-- 
Regards,
Sudeep

Re: [PATCH v3 3/7] firmware: smccc: Add the definition for SMCCCv1.2 version/error codes

[Sudeep Holla]

On Fri, May 15, 2020 at 12:38:01PM +0100, Mark Rutland wrote:
> On Wed, May 06, 2020 at 05:44:07PM +0100, Sudeep Holla wrote:
> > Add the definition for SMCCC v1.2 version and new error code added.
> > While at it, also add a note that ARM DEN 0070A is deprecated and is
> > now merged into the main SMCCC specification(ARM DEN 0028C).
> > 
> > Reviewed-by: Steven Price <steven.price@arm.com>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> 
> Hmm... the SMCCC v1.2 doc still seems to be EAC rather than a final
> release.
>

Right, I was told final release sometime in the recent past 😄
I mean April or mid-May, I will check on that but yes I agree on your
concerns.

> I don't expect that this would change, but I am a little hesitant to add
> other stuff based on an unfinalized spec. Do we know when the final
> release will be?
>

I have asked for the same as I write this email.

-- 
Regards,
Sudeep

Re: [PATCH v3 7/7] firmware: smccc: Add ARCH_SOC_ID support

[Sudeep Holla]

On Fri, May 15, 2020 at 01:50:05PM +0100, Mark Rutland wrote:
> On Wed, May 06, 2020 at 05:44:11PM +0100, Sudeep Holla wrote:
> > SMCCC v1.2 adds a new optional function SMCCC_ARCH_SOC_ID to obtain a
> > SiP defined SoC identification value. Add support for the same.
> >
> > Also using the SoC bus infrastructure, let us expose the platform
> > specific SoC atrributes under sysfs. We also provide custom sysfs for
> > the vendor ID as JEP-106 bank and identification code.
> >
> > Reviewed-by: Steven Price <steven.price@arm.com>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>
> As with the earlier patch referring to SMCCCv1.2 specifically, I'm
> somewhat wary of this until the SMCCCv1.2 spec is final. Do we know what
> the timeline is for that?
>

Not yet as mentioned in previous email, hopefully soon.

> That needn't hold up the pure refactoring/cleanup portions of this
> series, and regardless I have some comments below on this patch.
>

Sure, thanks.

> > ---
> >  drivers/firmware/smccc/Kconfig  |   8 ++
> >  drivers/firmware/smccc/Makefile |   1 +
> >  drivers/firmware/smccc/soc_id.c | 168 ++++++++++++++++++++++++++++++++
> >  include/linux/arm-smccc.h       |   5 +
> >  4 files changed, 182 insertions(+)
> >  create mode 100644 drivers/firmware/smccc/soc_id.c
> >
> > diff --git a/drivers/firmware/smccc/Kconfig b/drivers/firmware/smccc/Kconfig
> > index d93f1ab52cb2..15e7466179a6 100644
> > --- a/drivers/firmware/smccc/Kconfig
> > +++ b/drivers/firmware/smccc/Kconfig
> > @@ -15,3 +15,11 @@ config HAVE_ARM_SMCCC_DISCOVERY
> >  	 implementation of PSCI_FEATURES(SMCCC_VERSION) which returns
> >  	 success on firmware compliant to SMCCC v1.1 and above.
> >
> > +config ARM_SMCCC_SOC_ID
> > +	bool "SoC bus device for the ARM SMCCC SOC_ID"
> > +	depends on HAVE_ARM_SMCCC_DISCOVERY
> > +	default y
> > +	select SOC_BUS
> > +	help
> > +	  Include support for the SoC bus on the ARM SMCCC firmware based
> > +	  platforms providing some sysfs information about the SoC variant.
> > diff --git a/drivers/firmware/smccc/Makefile b/drivers/firmware/smccc/Makefile
> > index 6f369fe3f0b9..72ab84042832 100644
> > --- a/drivers/firmware/smccc/Makefile
> > +++ b/drivers/firmware/smccc/Makefile
> > @@ -1,3 +1,4 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  #
> >  obj-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)	+= smccc.o
> > +obj-$(CONFIG_ARM_SMCCC_SOC_ID)	+= soc_id.o
> > diff --git a/drivers/firmware/smccc/soc_id.c b/drivers/firmware/smccc/soc_id.c
> > new file mode 100644
> > index 000000000000..dc5dd3f1f59b
> > --- /dev/null
> > +++ b/drivers/firmware/smccc/soc_id.c
> > @@ -0,0 +1,168 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2020 Arm Limited
> > + */
> > +
> > +#define pr_fmt(fmt) "SMCCC: SOC_ID: " fmt
> > +
> > +#include <linux/arm-smccc.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/slab.h>
> > +#include <linux/sys_soc.h>
> > +
> > +#define SMCCC_SOC_ID_JEP106_BANK_IDX_MASK	GENMASK(30, 24)
> > +/*
> > + * As per the SMC Calling Convention specification v1.2 (ARM DEN 0028C)
> > + * Section 7.4 SMCCC_ARCH_SOC_ID bits[23:16] are JEP-106 identification
> > + * code with parity bit for the SiP. We can drop the parity bit.
> > + */
> > +#define SMCCC_SOC_ID_JEP106_ID_CODE_MASK	GENMASK(22, 16)
> > +#define SMCCC_SOC_ID_IMP_DEF_SOC_ID_MASK	GENMASK(15, 0)
> > +
> > +/* The bank index is equal to the for continuation code bank number - 1 */
> > +#define JEP106_BANK_CONT_CODE(x)	\
> > +	(u8)(FIELD_GET(SMCCC_SOC_ID_JEP106_BANK_IDX_MASK, (x)) + 1)
> > +#define JEP106_ID_CODE(x)	\
> > +	(u8)(FIELD_GET(SMCCC_SOC_ID_JEP106_ID_CODE_MASK, (x)))
> > +#define IMP_DEF_SOC_ID(x)	\
> > +	(u16)(FIELD_GET(SMCCC_SOC_ID_IMP_DEF_SOC_ID_MASK, (x)))
> > +
> > +static int soc_id_version;
> > +static struct soc_device *soc_dev;
> > +static struct soc_device_attribute *soc_dev_attr;
> > +
> > +static int smccc_map_error_codes(unsigned long a0)
> > +{
> > +	if (a0 >= SMCCC_RET_SUCCESS)
> > +		return 0;
>
> As SMCCC_RET_SUCCESS is 0, and a0 is an unsigned long, this condition is
> true for all values of a0.
>

Ah right.

> I don't think this function is particularly helpful, since in the case
> of a failure it'd be nicer to log the original FW error code to dmesg
> for debugging, so we may as well leave it to the caller to check for
> the EOPNOTSUPP / error cases.
>
> e.g. where NOT_SUPPORTED is legitimate callers can do:
>
> 	unsigned long ret = smccc_call_foo(...);
> 	if (ret == SMCCC_RET_NOT_SUPPORTED)
> 		return 0;
> 	if ((int)ret < 0)
> 		pr_err("FOO returned %lx\n", ret)
> 		return -EINVAL;
> 	}
>
> 	consume_ret_somehow(ret);
>
> ... and where NOT_SUPPORTED is not legitimate, they can avoid the
> explicit check for that:
>
> 	unsigned long ret = smccc_call_foo(...);
> 	if ((int)ret < 0) {
> 		pr_err("FOO returned %lx\n", ret)
> 		return -EINVAL;
> 	}
>
> 	consume_ret_somehow(ret);
>
> > +	else if (a0 == SMCCC_RET_INVALID_PARAMETER)
> > +		return -EINVAL;
> > +	else if (a0 == SMCCC_RET_NOT_SUPPORTED)
> > +		return -EOPNOTSUPP;
> > +	return -EINVAL;
> > +}
> > +
> > +static int smccc_soc_id_support_check(void)
> > +{
> > +	struct arm_smccc_res res;
> > +
> > +	if (arm_smccc_1_1_get_conduit() == SMCCC_CONDUIT_NONE) {
> > +		pr_err("%s: invalid SMCCC conduit\n", __func__);
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_FEATURES_FUNC_ID,
> > +			     ARM_SMCCC_ARCH_SOC_ID, &res);
> > +
> > +	return smccc_map_error_codes(res.a0);
> > +}
>
> Which I think means we may as well get rid of this, too, and fold the
> conduit check into the start of smccc_soc_init().
>

Will do so.

> > +
> > +static ssize_t
> > +jep106_cont_bank_code_show(struct device *dev, struct device_attribute *attr,
> > +			   char *buf)
> > +{
> > +	return sprintf(buf, "%02x\n", JEP106_BANK_CONT_CODE(soc_id_version));
> > +}
>
> For the regs/identification values, we added a '0x' prefix to make it
> explicit that the values are hex. Can/should we do that here, or is that
> against conventions for soc bus files?
>

Nope, thanks for pointing that. I clearly missed to see that. These are
custom attributes and soc bus doesn't deal with these. I will fix it.

> > +
> > +static DEVICE_ATTR_RO(jep106_cont_bank_code);
> > +
> > +static ssize_t
> > +jep106_identification_code_show(struct device *dev,
> > +				struct device_attribute *attr, char *buf)
> > +{
> > +	return sprintf(buf, "%02x\n", JEP106_ID_CODE(soc_id_version));
> > +}
> > +
> > +static DEVICE_ATTR_RO(jep106_identification_code);
> > +
> > +static struct attribute *jep106_id_attrs[] = {
> > +	&dev_attr_jep106_cont_bank_code.attr,
> > +	&dev_attr_jep106_identification_code.attr,
> > +	NULL
> > +};
> > +
> > +ATTRIBUTE_GROUPS(jep106_id);
> > +
> > +static int __init smccc_soc_init(void)
> > +{
> > +	struct device *dev;
> > +	int ret, soc_id_rev;
> > +	struct arm_smccc_res res;
> > +	static char soc_id_str[8], soc_id_rev_str[12];
> > +
> > +	if (arm_smccc_version_get() < ARM_SMCCC_VERSION_1_2)
> > +		return 0;
> > +
> > +	ret = smccc_soc_id_support_check();
> > +	if (ret) {
> > +		pr_info("Feature not implemented, skipping ....\n");
> > +		return 0;
> > +	}
>
> As above, I think we should expand smccc_soc_id_support_check() inline
> here.
>
> > +
> > +	arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_SOC_ID, 0, &res);
> > +
> > +	ret = smccc_map_error_codes(res.a0);
> > +	if (ret) {
> > +		pr_err("Failed to fetch version, Err = %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	soc_id_version = res.a0;
>
> ... and I think this'd be clearer like:
>
> 	arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_SOC_ID, 0, &res);
> 	if ((int)res.a0) {
> 		pr_err("ARCH_SOC_ID(0) returned error: %lx\n", res.a0);
> 		return -EINVAL;
> 	}
>
> > +	arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_SOC_ID, 1, &res);
> > +
> > +	ret = smccc_map_error_codes(res.a0);
> > +	if (ret) {
> > +		pr_err("Failed to fetch revision, Err = %d\n", ret);
> > +		return ret;
> > +	}
>
> ... likewise:
>
> 	arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_SOC_ID, 1, &res);
> 	if ((int)res.a0) {
> 		pr_err("ARCH_SOC_ID(1) returned error: %lx\n", res.a0);
> 		return -EINVAL;
> 	}
>
> > +
> > +	soc_id_rev = res.a0;
> > +
> > +	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> > +	if (!soc_dev_attr)
> > +		return -ENOMEM;
> > +
> > +	sprintf(soc_id_str, "0x%04x", IMP_DEF_SOC_ID(soc_id_version));
> > +	sprintf(soc_id_rev_str, "0x%08x", soc_id_rev);
> > +
> > +	soc_dev_attr->soc_id = soc_id_str;
> > +	soc_dev_attr->revision = soc_id_rev_str;
>
> Is there any expected format for these? e.g. would it make sense to add
> a prefix showing that these are JEP codes?
>

SoC ID and revision are just simple integers and match the SoC bus generic
code. The JEP106 codes(ID and bank cont number) are custom and I thought
the names of the sysfs indicate that. Initially I added JEP but dropped
it in favour of easy parsing for userspace as the file name already
indicates that it is JEP106 encoding.

> Do we need to care about platform code which might also regsiter a soc
> device? ... or do we expect those to be mutually exclusive?
>

Yes, they won't clash, but if platform code do register it, it will
appear as separate group entry.

--
Regards,
Sudeep