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

[PATCH v4 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[3] 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[2]:
	- 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

v3[2]->v4:
	- Incorporated all the review comments from Mark R

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://lore.kernel.org/r/20200506164411.3284-1-sudeep.holla@arm.com/
[3] 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    |  22 ++---
 drivers/firmware/smccc/Kconfig  |  25 ++++++
 drivers/firmware/smccc/Makefile |   4 +
 drivers/firmware/smccc/smccc.c  |  31 +++++++
 drivers/firmware/smccc/soc_id.c | 151 ++++++++++++++++++++++++++++++++
 include/linux/arm-smccc.h       |  27 +++++-
 include/linux/psci.h            |   7 --
 11 files changed, 256 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 v4 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.

Tested-by: Etienne Carriere <etienne.carriere@st.com>
Reviewed-by: Etienne Carriere <etienne.carriere@st.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/Kconfig       |  6 ++----
 drivers/firmware/smccc/Kconfig | 16 ++++++++++++++++
 2 files changed, 18 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..27b675d76235
--- /dev/null
+++ b/drivers/firmware/smccc/Kconfig
@@ -0,0 +1,16 @@
+# 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 v4 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(i.e. DEN0028C).

Tested-by: Etienne Carriere <etienne.carriere@st.com>
Reviewed-by: Steven Price <steven.price@arm.com>
Reviewed-by: Etienne Carriere <etienne.carriere@st.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 include/linux/arm-smccc.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index 59494df0f55b..31b15db9685d 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -10,7 +10,9 @@
 /*
  * 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
+ *
+ * This code is up-to-date with version DEN 0028 B
  */
 
 #define ARM_SMCCC_STD_CALL	        _AC(0,U)
-- 
2.17.1

[PATCH v4 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).

Tested-by: Etienne Carriere <etienne.carriere@st.com>
Reviewed-by: Steven Price <steven.price@arm.com>
Reviewed-by: Etienne Carriere <etienne.carriere@st.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 include/linux/arm-smccc.h | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index 31b15db9685d..c3784ba8e2a4 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -12,7 +12,7 @@
  * specified in
  * https://developer.arm.com/docs/den0028/latest
  *
- * This code is up-to-date with version DEN 0028 B
+ * This code is up-to-date with version DEN 0028 C
  */
 
 #define ARM_SMCCC_STD_CALL	        _AC(0,U)
@@ -58,6 +58,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,				\
@@ -316,10 +317,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 0028 C
+ */
 #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 v4 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.

Tested-by: Etienne Carriere <etienne.carriere@st.com>
Reviewed-by: Steven Price <steven.price@arm.com>
Reviewed-by: Etienne Carriere <etienne.carriere@st.com>
Acked-by: Mark Rutland <mark.rutland@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 v4 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.

Tested-by: Etienne Carriere <etienne.carriere@st.com>
Reviewed-by: Etienne Carriere <etienne.carriere@st.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 MAINTAINERS                     |  9 +++++++++
 drivers/firmware/Makefile       |  3 ++-
 drivers/firmware/psci/psci.c    | 20 +++++---------------
 drivers/firmware/smccc/Makefile |  3 +++
 drivers/firmware/smccc/smccc.c  | 26 ++++++++++++++++++++++++++
 include/linux/psci.h            |  2 --
 6 files changed, 45 insertions(+), 18 deletions(-)
 create mode 100644 drivers/firmware/smccc/Makefile
 create mode 100644 drivers/firmware/smccc/smccc.c

diff --git a/MAINTAINERS b/MAINTAINERS
index ecc0749810b0..2df80272b35e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15467,6 +15467,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..1330a698a178 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;
@@ -90,6 +79,7 @@ static u32 psci_function_id[PSCI_FN_MAX];
 
 static u32 psci_cpu_suspend_feature;
 static bool psci_system_reset2_supported;
+void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit);
 
 static inline bool psci_has_ext_power_state(void)
 {
@@ -242,7 +232,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 +402,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..de92a4b9f8f6
--- /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/init.h>
+#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/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 v4 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.

PSCI initialises both the SMCCC version and conduit. Similar to the
conduit, let us provide accessors to fetch the SMCCC version also so
that other SMCCC v1.1+ features can use it.

Tested-by: Etienne Carriere <etienne.carriere@st.com>
Reviewed-by: Steven Price <steven.price@arm.com>
Reviewed-by: Etienne Carriere <etienne.carriere@st.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/smccc/smccc.c |  5 +++++
 include/linux/arm-smccc.h      | 11 +++++++++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/firmware/smccc/smccc.c b/drivers/firmware/smccc/smccc.c
index de92a4b9f8f6..4e80921ee212 100644
--- a/drivers/firmware/smccc/smccc.c
+++ b/drivers/firmware/smccc/smccc.c
@@ -24,3 +24,8 @@ enum arm_smccc_conduit arm_smccc_1_1_get_conduit(void)
 
 	return smccc_conduit;
 }
+
+u32 arm_smccc_get_version(void)
+{
+	return smccc_version;
+}
diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index c3784ba8e2a4..c491d210e3c3 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -100,6 +100,17 @@ enum arm_smccc_conduit {
  */
 enum arm_smccc_conduit arm_smccc_1_1_get_conduit(void);
 
+/**
+ * arm_smccc_get_version()
+ *
+ * Returns the version to be used for SMCCCv1.1 or later.
+ *
+ * When SMCCCv1.1 or above is not present, returns SMCCCv1.0, but this
+ * does not imply the presence of firmware or a valid conduit. Caller
+ * handling SMCCCv1.0 must determine the conduit by other means.
+ */
+u32 arm_smccc_get_version(void);
+
 /**
  * struct arm_smccc_res - Result from SMC/HVC call
  * @a0-a3 result values from registers 0 to 3
-- 
2.17.1

[PATCH v4 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.

Tested-by: Etienne Carriere <etienne.carriere@st.com>
Reviewed-by: Steven Price <steven.price@arm.com>
Acked-by: Etienne Carriere <etienne.carriere@st.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/smccc/Kconfig  |   9 ++
 drivers/firmware/smccc/Makefile |   1 +
 drivers/firmware/smccc/soc_id.c | 151 ++++++++++++++++++++++++++++++++
 include/linux/arm-smccc.h       |   5 ++
 4 files changed, 166 insertions(+)
 create mode 100644 drivers/firmware/smccc/soc_id.c

diff --git a/drivers/firmware/smccc/Kconfig b/drivers/firmware/smccc/Kconfig
index 27b675d76235..15e7466179a6 100644
--- a/drivers/firmware/smccc/Kconfig
+++ b/drivers/firmware/smccc/Kconfig
@@ -14,3 +14,12 @@ config HAVE_ARM_SMCCC_DISCOVERY
 	 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.
+
+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..7e59e95e1fd3
--- /dev/null
+++ b/drivers/firmware/smccc/soc_id.c
@@ -0,0 +1,151 @@
+// 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 ssize_t
+jep106_cont_bank_code_show(struct device *dev, struct device_attribute *attr,
+			   char *buf)
+{
+	return sprintf(buf, "0x%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, "0x%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_get_version() < ARM_SMCCC_VERSION_1_2)
+		return 0;
+
+	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);
+
+	if (res.a0 == SMCCC_RET_NOT_SUPPORTED) {
+		pr_info("ARCH_SOC_ID not implemented, skipping ....\n");
+		return 0;
+	}
+
+	if ((int)res.a0 < 0) {
+		pr_info("ARCH_FEATURES(ARCH_SOC_ID) returned error: %lx\n",
+			res.a0);
+		return -EINVAL;
+	}
+
+        arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_SOC_ID, 0, &res);
+        if ((int)res.a0 < 0) {
+                pr_err("ARCH_SOC_ID(0) returned error: %lx\n", res.a0);
+                return -EINVAL;
+        }
+
+	soc_id_version = res.a0;
+
+        arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_SOC_ID, 1, &res);
+        if ((int)res.a0 < 0) {
+                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;
+
+	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 c491d210e3c3..6510f1bfcb05 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -70,6 +70,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 v4 7/7] firmware: smccc: Add ARCH_SOC_ID support

[Arnd Bergmann]

On Mon, May 18, 2020 at 11:12 AM Sudeep Holla <sudeep.holla@arm.com> wrote:

> +static ssize_t
> +jep106_cont_bank_code_show(struct device *dev, struct device_attribute *attr,
> +                          char *buf)
> +{
> +       return sprintf(buf, "0x%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, "0x%02x\n", JEP106_ID_CODE(soc_id_version));
> +}

I think we should try hard to avoid nonstandard attributes for the soc device.

Did you run into a problem with finding one of the existing attributes
that can be used to hold the fields?

       Arnd

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

[Sudeep Holla]

On Mon, May 18, 2020 at 11:30:21AM +0200, Arnd Bergmann wrote:
> On Mon, May 18, 2020 at 11:12 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> > +static ssize_t
> > +jep106_cont_bank_code_show(struct device *dev, struct device_attribute *attr,
> > +                          char *buf)
> > +{
> > +       return sprintf(buf, "0x%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, "0x%02x\n", JEP106_ID_CODE(soc_id_version));
> > +}
>
> I think we should try hard to avoid nonstandard attributes for the soc device.
>

I agree with that in general but this is bit different for below mentioned
reason.

> Did you run into a problem with finding one of the existing attributes
> that can be used to hold the fields?
>

Not really! The 2 JEP106 codes can be used to derive the manufacturer which
could match one of the existing attributes. However doing so might require
importing the huge JEP106 list as it needs to be maintained and updated
in the kernel. Also that approach will have the compatibility issue and
that is the reason for introducing these attributes representing raw
values for userspace.

--
Regards,
Sudeep

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

[Will Deacon]

On Mon, 18 May 2020 10:12:15 +0100, Sudeep Holla wrote:
> 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[3] fixing the original
> implementation there.
> 
> [...]

Applied to arm64 (for-next/smccc), thanks!

[1/7] firmware: smccc: Add HAVE_ARM_SMCCC_DISCOVERY to identify SMCCC v1.1 and above
      https://git.kernel.org/arm64/c/e5bfb21d98b6
[2/7] firmware: smccc: Update link to latest SMCCC specification
      https://git.kernel.org/arm64/c/15c704ab6244
[3/7] firmware: smccc: Add the definition for SMCCCv1.2 version/error codes
      https://git.kernel.org/arm64/c/0441bfe7f00a
[4/7] firmware: smccc: Drop smccc_version enum and use ARM_SMCCC_VERSION_1_x instead
      https://git.kernel.org/arm64/c/ad5a57dfe434
[5/7] firmware: smccc: Refactor SMCCC specific bits into separate file
      https://git.kernel.org/arm64/c/f2ae97062a48
[6/7] firmware: smccc: Add function to fetch SMCCC version
      https://git.kernel.org/arm64/c/a4fb17465182
[7/7] firmware: smccc: Add ARCH_SOC_ID support
      https://git.kernel.org/arm64/c/ce6488f0ce09

Arnd -- Sudeep's reply to you about the sysfs groups seemed reasonable to me,
but please shout if you'd rather I dropped this in order to pursue an
alternative approach.

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

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

[Arnd Bergmann]

On Mon, May 18, 2020 at 1:55 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Mon, May 18, 2020 at 11:30:21AM +0200, Arnd Bergmann wrote:
> > On Mon, May 18, 2020 at 11:12 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > > +static ssize_t
> > > +jep106_cont_bank_code_show(struct device *dev, struct device_attribute *attr,
> > > +                          char *buf)
> > > +{
> > > +       return sprintf(buf, "0x%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, "0x%02x\n", JEP106_ID_CODE(soc_id_version));
> > > +}
> >
> > I think we should try hard to avoid nonstandard attributes for the soc device.
> >
>
> I agree with that in general but this is bit different for below mentioned
> reason.
>
> > Did you run into a problem with finding one of the existing attributes
> > that can be used to hold the fields?
> >
>
> Not really! The 2 JEP106 codes can be used to derive the manufacturer which
> could match one of the existing attributes. However doing so might require
> importing the huge JEP106 list as it needs to be maintained and updated
> in the kernel. Also that approach will have the compatibility issue and
> that is the reason for introducing these attributes representing raw
> values for userspace.

I was thinking they codes could just be part of the normal strings rather
than get translated. Can you give an example what they would look like
with your current code?

If  you think they should be standard attributes, how about adding them
to the default list, and hardcoding them in the other soc device drivers
based on the information we have available there?

      Arnd

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

[Arnd Bergmann]

On Wed, May 20, 2020 at 11:29 PM Will Deacon <will@kernel.org> wrote:
>
> On Mon, 18 May 2020 10:12:15 +0100, Sudeep Holla wrote:
> > 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[3] fixing the original
> > implementation there.
> >
> > [...]
>
> Applied to arm64 (for-next/smccc), thanks!
>
> [1/7] firmware: smccc: Add HAVE_ARM_SMCCC_DISCOVERY to identify SMCCC v1.1 and above
>       https://git.kernel.org/arm64/c/e5bfb21d98b6
> [2/7] firmware: smccc: Update link to latest SMCCC specification
>       https://git.kernel.org/arm64/c/15c704ab6244
> [3/7] firmware: smccc: Add the definition for SMCCCv1.2 version/error codes
>       https://git.kernel.org/arm64/c/0441bfe7f00a
> [4/7] firmware: smccc: Drop smccc_version enum and use ARM_SMCCC_VERSION_1_x instead
>       https://git.kernel.org/arm64/c/ad5a57dfe434
> [5/7] firmware: smccc: Refactor SMCCC specific bits into separate file
>       https://git.kernel.org/arm64/c/f2ae97062a48
> [6/7] firmware: smccc: Add function to fetch SMCCC version
>       https://git.kernel.org/arm64/c/a4fb17465182
> [7/7] firmware: smccc: Add ARCH_SOC_ID support
>       https://git.kernel.org/arm64/c/ce6488f0ce09
>
> Arnd -- Sudeep's reply to you about the sysfs groups seemed reasonable to me,
> but please shout if you'd rather I dropped this in order to pursue an
> alternative approach.

I missed the reply earlier, thanks for pointing me to it again.

I'm not entirely convinced, but don't revert it for now because of that,
I assume we can find a solution.

However, please have a look at the build failure report for patch 5
and fix it if you can see what went wrong.

        Arnd

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

[Sudeep Holla]

On Wed, May 20, 2020 at 11:54:16PM +0200, Arnd Bergmann wrote:
> On Wed, May 20, 2020 at 11:29 PM Will Deacon <will@kernel.org> wrote:
> >
> > On Mon, 18 May 2020 10:12:15 +0100, Sudeep Holla wrote:
> > > 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[3] fixing the original
> > > implementation there.
> > >
> > > [...]
> >
> > Applied to arm64 (for-next/smccc), thanks!
> >
> > [1/7] firmware: smccc: Add HAVE_ARM_SMCCC_DISCOVERY to identify SMCCC v1.1 and above
> >       https://git.kernel.org/arm64/c/e5bfb21d98b6
> > [2/7] firmware: smccc: Update link to latest SMCCC specification
> >       https://git.kernel.org/arm64/c/15c704ab6244
> > [3/7] firmware: smccc: Add the definition for SMCCCv1.2 version/error codes
> >       https://git.kernel.org/arm64/c/0441bfe7f00a
> > [4/7] firmware: smccc: Drop smccc_version enum and use ARM_SMCCC_VERSION_1_x instead
> >       https://git.kernel.org/arm64/c/ad5a57dfe434
> > [5/7] firmware: smccc: Refactor SMCCC specific bits into separate file
> >       https://git.kernel.org/arm64/c/f2ae97062a48
> > [6/7] firmware: smccc: Add function to fetch SMCCC version
> >       https://git.kernel.org/arm64/c/a4fb17465182
> > [7/7] firmware: smccc: Add ARCH_SOC_ID support
> >       https://git.kernel.org/arm64/c/ce6488f0ce09
> >
> > Arnd -- Sudeep's reply to you about the sysfs groups seemed reasonable to me,
> > but please shout if you'd rather I dropped this in order to pursue an
> > alternative approach.
>
> I missed the reply earlier, thanks for pointing me to it again.
>
> I'm not entirely convinced, but don't revert it for now because of that,
> I assume we can find a solution.
>

I liked your idea of making this generic and hardcode values if required
for other drivers. I will take a look at that/

> However, please have a look at the build failure report for patch 5
> and fix it if you can see what went wrong.
>

Any pointers for that failure ? I seem to have missed them. I pushed
branch couple of times to my tree but got build success both times.
Any specific config or compilers ?

--
Regards,
Sudeep

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

[Sudeep Holla]

On Wed, May 20, 2020 at 11:51:47PM +0200, Arnd Bergmann wrote:
> On Mon, May 18, 2020 at 1:55 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Mon, May 18, 2020 at 11:30:21AM +0200, Arnd Bergmann wrote:
> > > On Mon, May 18, 2020 at 11:12 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > > +static ssize_t
> > > > +jep106_cont_bank_code_show(struct device *dev, struct device_attribute *attr,
> > > > +                          char *buf)
> > > > +{
> > > > +       return sprintf(buf, "0x%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, "0x%02x\n", JEP106_ID_CODE(soc_id_version));
> > > > +}
> > >
> > > I think we should try hard to avoid nonstandard attributes for the soc device.
> > >
> >
> > I agree with that in general but this is bit different for below mentioned
> > reason.
> >
> > > Did you run into a problem with finding one of the existing attributes
> > > that can be used to hold the fields?
> > >
> >
> > Not really! The 2 JEP106 codes can be used to derive the manufacturer which
> > could match one of the existing attributes. However doing so might require
> > importing the huge JEP106 list as it needs to be maintained and updated
> > in the kernel. Also that approach will have the compatibility issue and
> > that is the reason for introducing these attributes representing raw
> > values for userspace.
>
> I was thinking they codes could just be part of the normal strings rather
> than get translated. Can you give an example what they would look like
> with your current code?
>

Sure. Couple of example:
Cont Code   Identifier       Manufacturer
0		0x1		AMD
0		0x0e		Freescale (Motorola)
4		0x3b		ARM

I initially thought of value like "jep106-0-1" for AMD "jep-4-3b" for
ARM,..etc for the standard attribute family or machine. But I was not
convinced fully on that approach as it will be deviation from normal
values in those attributes. Further this represents the vendor name
rather than the family or machine.

> If  you think they should be standard attributes, how about adding them
> to the default list, and hardcoding them in the other soc device drivers
> based on the information we have available there?
>

That may be possible, I can take a look at the existing drivers and
check if that is feasible(which I think should be). Thanks for that
suggestion.

--
Regards,
Sudeep

[1] https://github.com/skottler/memtest86/blob/master/jedec_id.h

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

[Arnd Bergmann]

On Thu, May 21, 2020 at 9:07 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Wed, May 20, 2020 at 11:54:16PM +0200, Arnd Bergmann wrote:
> > On Wed, May 20, 2020 at 11:29 PM Will Deacon <will@kernel.org> wrote:
> > >
> > > On Mon, 18 May 2020 10:12:15 +0100, Sudeep Holla wrote:
> > > > 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[3] fixing the original
> > > > implementation there.
> > > >
> > > > [...]
> > >
> > > Applied to arm64 (for-next/smccc), thanks!
> > >
> > > [1/7] firmware: smccc: Add HAVE_ARM_SMCCC_DISCOVERY to identify SMCCC v1.1 and above
> > >       https://git.kernel.org/arm64/c/e5bfb21d98b6
> > > [2/7] firmware: smccc: Update link to latest SMCCC specification
> > >       https://git.kernel.org/arm64/c/15c704ab6244
> > > [3/7] firmware: smccc: Add the definition for SMCCCv1.2 version/error codes
> > >       https://git.kernel.org/arm64/c/0441bfe7f00a
> > > [4/7] firmware: smccc: Drop smccc_version enum and use ARM_SMCCC_VERSION_1_x instead
> > >       https://git.kernel.org/arm64/c/ad5a57dfe434
> > > [5/7] firmware: smccc: Refactor SMCCC specific bits into separate file
> > >       https://git.kernel.org/arm64/c/f2ae97062a48
> > > [6/7] firmware: smccc: Add function to fetch SMCCC version
> > >       https://git.kernel.org/arm64/c/a4fb17465182
> > > [7/7] firmware: smccc: Add ARCH_SOC_ID support
> > >       https://git.kernel.org/arm64/c/ce6488f0ce09
> > >
> > > Arnd -- Sudeep's reply to you about the sysfs groups seemed reasonable to me,
> > > but please shout if you'd rather I dropped this in order to pursue an
> > > alternative approach.
> >
> > I missed the reply earlier, thanks for pointing me to it again.
> >
> > I'm not entirely convinced, but don't revert it for now because of that,
> > I assume we can find a solution.
> >
>
> I liked your idea of making this generic and hardcode values if required
> for other drivers. I will take a look at that/
>
> > However, please have a look at the build failure report for patch 5
> > and fix it if you can see what went wrong.
> >
>
> Any pointers for that failure ? I seem to have missed them. I pushed
> branch couple of times to my tree but got build success both times.
> Any specific config or compilers ?

See below for the reply from the 0day build bot to your email. It seems it
was not sent to the mailing list, but you were on Cc. Looking at it now,
the fix should be trivial.

    Arnd

8<---
I love your patch! Perhaps something to improve:

[auto build test WARNING on soc/for-next]
[also build test WARNING on arm64/for-next/core linus/master v5.7-rc6
next-20200519]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Sudeep-Holla/firmware-smccc-Add-basic-SMCCC-v1-2-ARCH_SOC_ID-support/20200518-171401
base:   https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
config: arm64-randconfig-r026-20200519 (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project
135b877874fae96b4372c8a3fbfaa8ff44ff86e3)
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross
-O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

>> drivers/firmware/smccc/smccc.c:14:13: warning: no previous prototype for function 'arm_smccc_version_init' [-Wmissing-prototypes]
void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit)
^
drivers/firmware/smccc/smccc.c:14:1: note: declare 'static' if the
function is not intended to be used outside of this translation unit
void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit)
^
static
1 warning generated.

vim +/arm_smccc_version_init +14 drivers/firmware/smccc/smccc.c

    13
  > 14 void __init arm_smccc_version_init(u32 version, enum
arm_smccc_conduit conduit)
    15 {
    16 smccc_version = version;
    17 smccc_conduit = conduit;
    18 }
    19

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

[Will Deacon]

On Thu, May 21, 2020 at 09:34:10AM +0200, Arnd Bergmann wrote:
> On Thu, May 21, 2020 at 9:07 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > On Wed, May 20, 2020 at 11:54:16PM +0200, Arnd Bergmann wrote:
> > > On Wed, May 20, 2020 at 11:29 PM Will Deacon <will@kernel.org> wrote:
> > > > Applied to arm64 (for-next/smccc), thanks!
> > > >
> > > > Arnd -- Sudeep's reply to you about the sysfs groups seemed reasonable to me,
> > > > but please shout if you'd rather I dropped this in order to pursue an
> > > > alternative approach.
> > >
> > > I missed the reply earlier, thanks for pointing me to it again.

D'oh, I took your silence as "no objections". Oh well!

> > > I'm not entirely convinced, but don't revert it for now because of that,
> > > I assume we can find a solution.

Ok, cheers. It's on a separate branch so it's easy enough to drop if
necessary (i.e. no reverts needed). Sudeep -- please send any extra patches
on top of the branch.

> > I liked your idea of making this generic and hardcode values if required
> > for other drivers. I will take a look at that/
> >
> > > However, please have a look at the build failure report for patch 5
> > > and fix it if you can see what went wrong.
> > >
> >
> > Any pointers for that failure ? I seem to have missed them. I pushed
> > branch couple of times to my tree but got build success both times.
> > Any specific config or compilers ?
> 
> See below for the reply from the 0day build bot to your email. It seems it
> was not sent to the mailing list, but you were on Cc. Looking at it now,
> the fix should be trivial.

[...]

> >> drivers/firmware/smccc/smccc.c:14:13: warning: no previous prototype for function 'arm_smccc_version_init' [-Wmissing-prototypes]
> void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit)
> ^
> drivers/firmware/smccc/smccc.c:14:1: note: declare 'static' if the
> function is not intended to be used outside of this translation unit
> void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit)

I saw that when I applied the patches, but since the function is called from
another compilation unit (psci/psci.o), I just ignored it as we have loads
of these already and it only screams if you build with W=1.

Will

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

[Sudeep Holla]

On Thu, May 21, 2020 at 09:34:10AM +0200, Arnd Bergmann wrote:
> On Thu, May 21, 2020 at 9:07 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Wed, May 20, 2020 at 11:54:16PM +0200, Arnd Bergmann wrote:
> > > On Wed, May 20, 2020 at 11:29 PM Will Deacon <will@kernel.org> wrote:
> > > >
> > > > On Mon, 18 May 2020 10:12:15 +0100, Sudeep Holla wrote:
> > > > > 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[3] fixing the original
> > > > > implementation there.
> > > > >
> > > > > [...]
> > > >
> > > > Applied to arm64 (for-next/smccc), thanks!
> > > >
> > > > [1/7] firmware: smccc: Add HAVE_ARM_SMCCC_DISCOVERY to identify SMCCC v1.1 and above
> > > >       https://git.kernel.org/arm64/c/e5bfb21d98b6
> > > > [2/7] firmware: smccc: Update link to latest SMCCC specification
> > > >       https://git.kernel.org/arm64/c/15c704ab6244
> > > > [3/7] firmware: smccc: Add the definition for SMCCCv1.2 version/error codes
> > > >       https://git.kernel.org/arm64/c/0441bfe7f00a
> > > > [4/7] firmware: smccc: Drop smccc_version enum and use ARM_SMCCC_VERSION_1_x instead
> > > >       https://git.kernel.org/arm64/c/ad5a57dfe434
> > > > [5/7] firmware: smccc: Refactor SMCCC specific bits into separate file
> > > >       https://git.kernel.org/arm64/c/f2ae97062a48
> > > > [6/7] firmware: smccc: Add function to fetch SMCCC version
> > > >       https://git.kernel.org/arm64/c/a4fb17465182
> > > > [7/7] firmware: smccc: Add ARCH_SOC_ID support
> > > >       https://git.kernel.org/arm64/c/ce6488f0ce09
> > > >
> > > > Arnd -- Sudeep's reply to you about the sysfs groups seemed reasonable to me,
> > > > but please shout if you'd rather I dropped this in order to pursue an
> > > > alternative approach.
> > >
> > > I missed the reply earlier, thanks for pointing me to it again.
> > >
> > > I'm not entirely convinced, but don't revert it for now because of that,
> > > I assume we can find a solution.
> > >
> >
> > I liked your idea of making this generic and hardcode values if required
> > for other drivers. I will take a look at that/
> >
> > > However, please have a look at the build failure report for patch 5
> > > and fix it if you can see what went wrong.
> > >
> >
> > Any pointers for that failure ? I seem to have missed them. I pushed
> > branch couple of times to my tree but got build success both times.
> > Any specific config or compilers ?
> 
> See below for the reply from the 0day build bot to your email. It seems it
> was not sent to the mailing list, but you were on Cc. Looking at it now,
> the fix should be trivial.
> 

Ah, clang it is. I must start building with clang regularly.
Thanks for pointing it out. Somehow few of these kbuild-bot emails
has been marked junk last few days. Sorry for the noise.

-- 
Regards,
Sudeep

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

[Sudeep Holla]

On Thu, May 21, 2020 at 08:57:56AM +0100, Will Deacon wrote:
> On Thu, May 21, 2020 at 09:34:10AM +0200, Arnd Bergmann wrote:
> > On Thu, May 21, 2020 at 9:07 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > On Wed, May 20, 2020 at 11:54:16PM +0200, Arnd Bergmann wrote:
> > > > On Wed, May 20, 2020 at 11:29 PM Will Deacon <will@kernel.org> wrote:
> > > > > Applied to arm64 (for-next/smccc), thanks!
> > > > >
> > > > > Arnd -- Sudeep's reply to you about the sysfs groups seemed reasonable to me,
> > > > > but please shout if you'd rather I dropped this in order to pursue an
> > > > > alternative approach.
> > > >
> > > > I missed the reply earlier, thanks for pointing me to it again.
> 
> D'oh, I took your silence as "no objections". Oh well!
> 
> > > > I'm not entirely convinced, but don't revert it for now because of that,
> > > > I assume we can find a solution.
> 
> Ok, cheers. It's on a separate branch so it's easy enough to drop if
> necessary (i.e. no reverts needed). Sudeep -- please send any extra patches
> on top of the branch.
>

Indeed, it is also last patch in the series. However if Arnd is happy
with the sysfs names, we can move to generic code later without breaking
anything.

We need not revert or drop it now. I will leave that to you or Arnd to
decide. Just that it may be too late to get acks for all the soc sysfs
drivers in time for v5.8

I am fine if you want to drop the last patch.

> > > I liked your idea of making this generic and hardcode values if required
> > > for other drivers. I will take a look at that/
> > >
> > > > However, please have a look at the build failure report for patch 5
> > > > and fix it if you can see what went wrong.
> > > >
> > >
> > > Any pointers for that failure ? I seem to have missed them. I pushed
> > > branch couple of times to my tree but got build success both times.
> > > Any specific config or compilers ?
> > 
> > See below for the reply from the 0day build bot to your email. It seems it
> > was not sent to the mailing list, but you were on Cc. Looking at it now,
> > the fix should be trivial.
> 
> [...]
> 
> > >> drivers/firmware/smccc/smccc.c:14:13: warning: no previous prototype for function 'arm_smccc_version_init' [-Wmissing-prototypes]
> > void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit)
> > ^
> > drivers/firmware/smccc/smccc.c:14:1: note: declare 'static' if the
> > function is not intended to be used outside of this translation unit
> > void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit)
> 
> I saw that when I applied the patches, but since the function is called from
> another compilation unit (psci/psci.o), I just ignored it as we have loads
> of these already and it only screams if you build with W=1.
> 

/me confused. Do you need the fix for this warning or you are happy to ignore?

-- 
Regards,
Sudeep

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

[Arnd Bergmann]

On Thu, May 21, 2020 at 10:11 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> On Thu, May 21, 2020 at 08:57:56AM +0100, Will Deacon wrote:
> > On Thu, May 21, 2020 at 09:34:10AM +0200, Arnd Bergmann wrote:
> > > On Thu, May 21, 2020 at 9:07 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > On Wed, May 20, 2020 at 11:54:16PM +0200, Arnd Bergmann wrote:
> > > > > On Wed, May 20, 2020 at 11:29 PM Will Deacon <will@kernel.org> wrote:
> > > > > > Applied to arm64 (for-next/smccc), thanks!
> > > > > >
> > > > > > Arnd -- Sudeep's reply to you about the sysfs groups seemed reasonable to me,
> > > > > > but please shout if you'd rather I dropped this in order to pursue an
> > > > > > alternative approach.
> > > > >
> > > > > I missed the reply earlier, thanks for pointing me to it again.
> >
> > D'oh, I took your silence as "no objections". Oh well!
> >
> > > > > I'm not entirely convinced, but don't revert it for now because of that,
> > > > > I assume we can find a solution.
> >
> > Ok, cheers. It's on a separate branch so it's easy enough to drop if
> > necessary (i.e. no reverts needed). Sudeep -- please send any extra patches
> > on top of the branch.
> >
>
> Indeed, it is also last patch in the series. However if Arnd is happy
> with the sysfs names, we can move to generic code later without breaking
> anything.
>
> We need not revert or drop it now. I will leave that to you or Arnd to
> decide. Just that it may be too late to get acks for all the soc sysfs
> drivers in time for v5.8
>
> I am fine if you want to drop the last patch.

Ok, let's drop that patch then and make sure we do something that
everyone is happy with later on. I'm already in favor of adding
a more reliable soc_device instance based on this, but we need to
be sure we don't screw up the contents of the attributes when we
can't change them later.

> > > >> drivers/firmware/smccc/smccc.c:14:13: warning: no previous prototype for function 'arm_smccc_version_init' [-Wmissing-prototypes]
> > > void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit)
> > > ^
> > > drivers/firmware/smccc/smccc.c:14:1: note: declare 'static' if the
> > > function is not intended to be used outside of this translation unit
> > > void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit)
> >
> > I saw that when I applied the patches, but since the function is called from
> > another compilation unit (psci/psci.o), I just ignored it as we have loads
> > of these already and it only screams if you build with W=1.
> >
>
> /me confused. Do you need the fix for this warning or you are happy to ignore?

I want a fix for that, as I hope we can eventually turn this warning on by
default and stop playing whack-a-mole when they come up. Most of these
warnings are harmless, but occasionally the prototypes don't match exactly
and cause real bugs depending on the configuration, and ensuring both
sides include a common header file is an easy way to make it work
more reliably.

Note that the warning should come up for either W=1 or C=1, and I also
think that
new code should generally be written sparse-clean and have no warnings with
'make C=1' as a rule.

      Arnd

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

[Sudeep Holla]

On Thu, May 21, 2020 at 11:06:23AM +0200, Arnd Bergmann wrote:
> On Thu, May 21, 2020 at 10:11 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > On Thu, May 21, 2020 at 08:57:56AM +0100, Will Deacon wrote:
> > > On Thu, May 21, 2020 at 09:34:10AM +0200, Arnd Bergmann wrote:
> > > > On Thu, May 21, 2020 at 9:07 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > > On Wed, May 20, 2020 at 11:54:16PM +0200, Arnd Bergmann wrote:
> > > > > > On Wed, May 20, 2020 at 11:29 PM Will Deacon <will@kernel.org> wrote:
> > > > > > > Applied to arm64 (for-next/smccc), thanks!
> > > > > > >
> > > > > > > Arnd -- Sudeep's reply to you about the sysfs groups seemed reasonable to me,
> > > > > > > but please shout if you'd rather I dropped this in order to pursue an
> > > > > > > alternative approach.
> > > > > >
> > > > > > I missed the reply earlier, thanks for pointing me to it again.
> > >
> > > D'oh, I took your silence as "no objections". Oh well!
> > >
> > > > > > I'm not entirely convinced, but don't revert it for now because of that,
> > > > > > I assume we can find a solution.
> > >
> > > Ok, cheers. It's on a separate branch so it's easy enough to drop if
> > > necessary (i.e. no reverts needed). Sudeep -- please send any extra patches
> > > on top of the branch.
> > >
> >
> > Indeed, it is also last patch in the series. However if Arnd is happy
> > with the sysfs names, we can move to generic code later without breaking
> > anything.
> >
> > We need not revert or drop it now. I will leave that to you or Arnd to
> > decide. Just that it may be too late to get acks for all the soc sysfs
> > drivers in time for v5.8
> >
> > I am fine if you want to drop the last patch.
>
> Ok, let's drop that patch then and make sure we do something that
> everyone is happy with later on. I'm already in favor of adding
> a more reliable soc_device instance based on this, but we need to
> be sure we don't screw up the contents of the attributes when we
> can't change them later.
>

Sure. Will, please drop the last patch in the series. I will rework
moving the custom attributes to the core.

> > > > >> drivers/firmware/smccc/smccc.c:14:13: warning: no previous prototype for function 'arm_smccc_version_init' [-Wmissing-prototypes]
> > > > void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit)
> > > > ^
> > > > drivers/firmware/smccc/smccc.c:14:1: note: declare 'static' if the
> > > > function is not intended to be used outside of this translation unit
> > > > void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit)
> > >
> > > I saw that when I applied the patches, but since the function is called from
> > > another compilation unit (psci/psci.o), I just ignored it as we have loads
> > > of these already and it only screams if you build with W=1.
> > >
> >
> > /me confused. Do you need the fix for this warning or you are happy to ignore?
>
> I want a fix for that, as I hope we can eventually turn this warning on by
> default and stop playing whack-a-mole when they come up. Most of these
> warnings are harmless, but occasionally the prototypes don't match exactly
> and cause real bugs depending on the configuration, and ensuring both
> sides include a common header file is an easy way to make it work
> more reliably.
>

Agreed.

> Note that the warning should come up for either W=1 or C=1, and I also
> think that new code should generally be written sparse-clean and have
> no warnings with 'make C=1' as a rule.
>

Sure, I am facing issues with clang-8, it fails to build arm_smccc_1_1_invoke
which I think Nick was mentioning in some other thread. I will try latest
clang.

--
Regards,
Sudeep

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

[Will Deacon]

On Thu, May 21, 2020 at 11:06:23AM +0200, Arnd Bergmann wrote:
> On Thu, May 21, 2020 at 10:11 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > Indeed, it is also last patch in the series. However if Arnd is happy
> > with the sysfs names, we can move to generic code later without breaking
> > anything.
> >
> > We need not revert or drop it now. I will leave that to you or Arnd to
> > decide. Just that it may be too late to get acks for all the soc sysfs
> > drivers in time for v5.8
> >
> > I am fine if you want to drop the last patch.
> 
> Ok, let's drop that patch then and make sure we do something that
> everyone is happy with later on. I'm already in favor of adding
> a more reliable soc_device instance based on this, but we need to
> be sure we don't screw up the contents of the attributes when we
> can't change them later.
> 
> > > > >> drivers/firmware/smccc/smccc.c:14:13: warning: no previous prototype for function 'arm_smccc_version_init' [-Wmissing-prototypes]
> > > > void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit)
> > > > ^
> > > > drivers/firmware/smccc/smccc.c:14:1: note: declare 'static' if the
> > > > function is not intended to be used outside of this translation unit
> > > > void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit)
> > >
> > > I saw that when I applied the patches, but since the function is called from
> > > another compilation unit (psci/psci.o), I just ignored it as we have loads
> > > of these already and it only screams if you build with W=1.
> > >
> >
> > /me confused. Do you need the fix for this warning or you are happy to ignore?
> 
> I want a fix for that, as I hope we can eventually turn this warning on by
> default and stop playing whack-a-mole when they come up. Most of these
> warnings are harmless, but occasionally the prototypes don't match exactly
> and cause real bugs depending on the configuration, and ensuring both
> sides include a common header file is an easy way to make it work
> more reliably.
> 
> Note that the warning should come up for either W=1 or C=1, and I also
> think that
> new code should generally be written sparse-clean and have no warnings with
> 'make C=1' as a rule.

Fair enough. Is anybody working on a tree-wide sweep for this, like we've
done for other things such as zero-length arrays? If so, I can start
enforcing this in the arch code as well (I haven't been so far, even though
I do run sparse on every commit).

Anyway, I've dropped the last patch from the branch, and we can put a fix
for the missing prototype on top.

Will

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

[Sudeep Holla]

On Thu, May 21, 2020 at 10:17:39AM +0100, Will Deacon wrote:
> On Thu, May 21, 2020 at 11:06:23AM +0200, Arnd Bergmann wrote:
> > On Thu, May 21, 2020 at 10:11 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > Indeed, it is also last patch in the series. However if Arnd is happy
> > > with the sysfs names, we can move to generic code later without breaking
> > > anything.
> > >
> > > We need not revert or drop it now. I will leave that to you or Arnd to
> > > decide. Just that it may be too late to get acks for all the soc sysfs
> > > drivers in time for v5.8
> > >
> > > I am fine if you want to drop the last patch.
> > 
> > Ok, let's drop that patch then and make sure we do something that
> > everyone is happy with later on. I'm already in favor of adding
> > a more reliable soc_device instance based on this, but we need to
> > be sure we don't screw up the contents of the attributes when we
> > can't change them later.
> > 
> > > > > >> drivers/firmware/smccc/smccc.c:14:13: warning: no previous prototype for function 'arm_smccc_version_init' [-Wmissing-prototypes]
> > > > > void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit)
> > > > > ^
> > > > > drivers/firmware/smccc/smccc.c:14:1: note: declare 'static' if the
> > > > > function is not intended to be used outside of this translation unit
> > > > > void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit)
> > > >
> > > > I saw that when I applied the patches, but since the function is called from
> > > > another compilation unit (psci/psci.o), I just ignored it as we have loads
> > > > of these already and it only screams if you build with W=1.
> > > >
> > >
> > > /me confused. Do you need the fix for this warning or you are happy to ignore?
> > 
> > I want a fix for that, as I hope we can eventually turn this warning on by
> > default and stop playing whack-a-mole when they come up. Most of these
> > warnings are harmless, but occasionally the prototypes don't match exactly
> > and cause real bugs depending on the configuration, and ensuring both
> > sides include a common header file is an easy way to make it work
> > more reliably.
> > 
> > Note that the warning should come up for either W=1 or C=1, and I also
> > think that
> > new code should generally be written sparse-clean and have no warnings with
> > 'make C=1' as a rule.
> 
> Fair enough. Is anybody working on a tree-wide sweep for this, like we've
> done for other things such as zero-length arrays? If so, I can start
> enforcing this in the arch code as well (I haven't been so far, even though
> I do run sparse on every commit).
> 
> Anyway, I've dropped the last patch from the branch, and we can put a fix
> for the missing prototype on top.
> 

Thanks Will, sorry for the trouble. Though I can send the fix for the
missing prototype right away, I would like to get my clang setup working
as an opportunity. clang-8 that I have is failing vanilla v5.7-rc6
when expanding arm_smccc_1_1_*

-- 
Regards,
Sudeep

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

[Arnd Bergmann]

On Thu, May 21, 2020 at 11:17 AM Will Deacon <will@kernel.org> wrote:
> On Thu, May 21, 2020 at 11:06:23AM +0200, Arnd Bergmann wrote:
> > On Thu, May 21, 2020 at 10:11 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > Note that the warning should come up for either W=1 or C=1, and I also
> > think that
> > new code should generally be written sparse-clean and have no warnings with
> > 'make C=1' as a rule.
>
> Fair enough. Is anybody working on a tree-wide sweep for this, like we've
> done for other things such as zero-length arrays? If so, I can start
> enforcing this in the arch code as well (I haven't been so far, even though
> I do run sparse on every commit).

I've done some work on that a few years ago, and there are always
some cleanup patches for C=1 and W=1 warnings, most recently
with an increase from Huawei's automated build testing + manual
patching.

I have not looked in a while, but it always seemed to be somewhere
between "too much to do by myself" and "small enough that it should
really be done" as build warnings go.

       Arnd

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

[Russell King - ARM Linux admin]

On Thu, May 21, 2020 at 11:06:23AM +0200, Arnd Bergmann wrote:
> Note that the warning should come up for either W=1 or C=1, and I also
> think that
> new code should generally be written sparse-clean and have no warnings with
> 'make C=1' as a rule.

No, absolutely not, that's a stupid idea, there are corner cases
where hiding a sparse warning is the wrong thing to do.  Look at
many of the cases in fs/ for example.

See https://lkml.org/lkml/2004/9/12/249 which should make anyone
who sees a use of __force in some random code stop and question
why it is there, and whether it is actually correct, or just there
to hide a sparse warning.

Remember, sparse is there to warn that something isn't quite right,
and the view taken is, if it isn't right, then we don't "cast the
warning away" with __force, even if we intend not to fix the code
immediately.

So, going for "sparse-clean" is actually not correct. Going for
"no unnecessary warnings" is.

And don't think what I've said above doesn't happen; I've rejected
patches from people who've gone around trying to fix every sparse
warning that they see by throwing __force incorrectly at it.

The thing is, if you hide all the warnings, even for incorrect code,
then sparse becomes completely useless to identify where things in
the code are not quite correct.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

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

[Will Deacon]

On Thu, May 21, 2020 at 10:26:27AM +0100, Sudeep Holla wrote:
> On Thu, May 21, 2020 at 10:17:39AM +0100, Will Deacon wrote:
> > On Thu, May 21, 2020 at 11:06:23AM +0200, Arnd Bergmann wrote:
> > > On Thu, May 21, 2020 at 10:11 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > /me confused. Do you need the fix for this warning or you are happy to ignore?
> > > 
> > > I want a fix for that, as I hope we can eventually turn this warning on by
> > > default and stop playing whack-a-mole when they come up. Most of these
> > > warnings are harmless, but occasionally the prototypes don't match exactly
> > > and cause real bugs depending on the configuration, and ensuring both
> > > sides include a common header file is an easy way to make it work
> > > more reliably.
> > > 
> > > Note that the warning should come up for either W=1 or C=1, and I also
> > > think that
> > > new code should generally be written sparse-clean and have no warnings with
> > > 'make C=1' as a rule.
> > 
> > Fair enough. Is anybody working on a tree-wide sweep for this, like we've
> > done for other things such as zero-length arrays? If so, I can start
> > enforcing this in the arch code as well (I haven't been so far, even though
> > I do run sparse on every commit).
> > 
> > Anyway, I've dropped the last patch from the branch, and we can put a fix
> > for the missing prototype on top.
> > 
> 
> Thanks Will, sorry for the trouble. Though I can send the fix for the
> missing prototype right away, I would like to get my clang setup working
> as an opportunity. clang-8 that I have is failing vanilla v5.7-rc6
> when expanding arm_smccc_1_1_*

No trouble at all, really. I also saw this from Nathan the other day, which
may help you get up and running with clang:

https://lore.kernel.org/r/20200520024736.GA854786@ubuntu-s3-xlarge-x86

(but I haven't tried it myself, as I'm just using the Android binary)

Will

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

[Sudeep Holla]

On Thu, May 21, 2020 at 11:14:38AM +0100, Will Deacon wrote:
> On Thu, May 21, 2020 at 10:26:27AM +0100, Sudeep Holla wrote:
> > On Thu, May 21, 2020 at 10:17:39AM +0100, Will Deacon wrote:
> > > On Thu, May 21, 2020 at 11:06:23AM +0200, Arnd Bergmann wrote:
> > > > On Thu, May 21, 2020 at 10:11 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > > /me confused. Do you need the fix for this warning or you are happy to ignore?
> > > > 
> > > > I want a fix for that, as I hope we can eventually turn this warning on by
> > > > default and stop playing whack-a-mole when they come up. Most of these
> > > > warnings are harmless, but occasionally the prototypes don't match exactly
> > > > and cause real bugs depending on the configuration, and ensuring both
> > > > sides include a common header file is an easy way to make it work
> > > > more reliably.
> > > > 
> > > > Note that the warning should come up for either W=1 or C=1, and I also
> > > > think that
> > > > new code should generally be written sparse-clean and have no warnings with
> > > > 'make C=1' as a rule.
> > > 
> > > Fair enough. Is anybody working on a tree-wide sweep for this, like we've
> > > done for other things such as zero-length arrays? If so, I can start
> > > enforcing this in the arch code as well (I haven't been so far, even though
> > > I do run sparse on every commit).
> > > 
> > > Anyway, I've dropped the last patch from the branch, and we can put a fix
> > > for the missing prototype on top.
> > > 
> > 
> > Thanks Will, sorry for the trouble. Though I can send the fix for the
> > missing prototype right away, I would like to get my clang setup working
> > as an opportunity. clang-8 that I have is failing vanilla v5.7-rc6
> > when expanding arm_smccc_1_1_*
> 
> No trouble at all, really. I also saw this from Nathan the other day, which
> may help you get up and running with clang:
> 
> https://lore.kernel.org/r/20200520024736.GA854786@ubuntu-s3-xlarge-x86
> 

Thanks for the link, I will try some other time(may be next time I run
into clang issue 😄). Anyway upgrading to clang-11 fixed the build issue.
I will soon post the patch for missing prototype.

-- 
Regards,
Sudeep

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

[Arnd Bergmann]

On Thu, May 21, 2020 at 12:14 PM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Thu, May 21, 2020 at 11:06:23AM +0200, Arnd Bergmann wrote:
> > Note that the warning should come up for either W=1 or C=1, and I also
> > think that
> > new code should generally be written sparse-clean and have no warnings with
> > 'make C=1' as a rule.
>
> No, absolutely not, that's a stupid idea, there are corner cases
> where hiding a sparse warning is the wrong thing to do.  Look at
> many of the cases in fs/ for example.
>
> See https://lkml.org/lkml/2004/9/12/249 which should make anyone
> who sees a use of __force in some random code stop and question
> why it is there, and whether it is actually correct, or just there
> to hide a sparse warning.
>
> Remember, sparse is there to warn that something isn't quite right,
> and the view taken is, if it isn't right, then we don't "cast the
> warning away" with __force, even if we intend not to fix the code
> immediately.
>
> So, going for "sparse-clean" is actually not correct. Going for
> "no unnecessary warnings" is.
>
> And don't think what I've said above doesn't happen; I've rejected
> patches from people who've gone around trying to fix every sparse
> warning that they see by throwing __force incorrectly at it.
>
> The thing is, if you hide all the warnings, even for incorrect code,
> then sparse becomes completely useless to identify where things in
> the code are not quite correct.

Adding __force is almost always the wrong solution, and I explictly
was not talking about existing code here where changing it would
risk introducing bugs or require bad hacks.

However, when writing a new driver, sparse warnings usually
indicate that you are doing something wrong that is better addressed
by doing something different that does not involve adding __force.

      Arnd

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

[Russell King - ARM Linux admin]

On Thu, May 21, 2020 at 12:31:32PM +0200, Arnd Bergmann wrote:
> On Thu, May 21, 2020 at 12:14 PM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> > On Thu, May 21, 2020 at 11:06:23AM +0200, Arnd Bergmann wrote:
> > > Note that the warning should come up for either W=1 or C=1, and I also
> > > think that
> > > new code should generally be written sparse-clean and have no warnings with
> > > 'make C=1' as a rule.
> >
> > No, absolutely not, that's a stupid idea, there are corner cases
> > where hiding a sparse warning is the wrong thing to do.  Look at
> > many of the cases in fs/ for example.
> >
> > See https://lkml.org/lkml/2004/9/12/249 which should make anyone
> > who sees a use of __force in some random code stop and question
> > why it is there, and whether it is actually correct, or just there
> > to hide a sparse warning.
> >
> > Remember, sparse is there to warn that something isn't quite right,
> > and the view taken is, if it isn't right, then we don't "cast the
> > warning away" with __force, even if we intend not to fix the code
> > immediately.
> >
> > So, going for "sparse-clean" is actually not correct. Going for
> > "no unnecessary warnings" is.
> >
> > And don't think what I've said above doesn't happen; I've rejected
> > patches from people who've gone around trying to fix every sparse
> > warning that they see by throwing __force incorrectly at it.
> >
> > The thing is, if you hide all the warnings, even for incorrect code,
> > then sparse becomes completely useless to identify where things in
> > the code are not quite correct.
> 
> Adding __force is almost always the wrong solution, and I explictly
> was not talking about existing code here where changing it would
> risk introducing bugs or require bad hacks.

I'm using existing code to illustrate the problem with your idea of
"sparse-clean" new code, trying to show you that it is not about
being sparse clean, but about being correct.

> However, when writing a new driver, sparse warnings usually
> indicate that you are doing something wrong that is better addressed
> by doing something different that does not involve adding __force.

Right, but if you lay down a rule that says "new submissions must be
sparse clean" you will get people using __force to shut sparse up.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up