[PATCH v5 0/4] soc: amlogic: add new meson-gx-socinfo-sm driver

[PATCH v5 0/4] soc: amlogic: add new meson-gx-socinfo-sm driver

[Viacheslav Bocharov]

The Amlogic Meson SoC Secure Monitor implements a call to retrieve an
unique SoC ID starting from the GX Family and all new families.
But GX-family chips (e.g. GXB, GXL and newer) supports also 128-bit
chip ID. 128-bit chip ID consists 32-bit SoC version and 96-bit OTP data.

This is next attempt to publish data from the Amlogic secure monitor
chipid call. After discussions with Neil Armstrong, it was decided to
publish the chipid call results through the soc driver. Since
soc_device_match cannot wait for the soc driver to load, and the secure
monitor calls in turn depend on the sm driver, it was necessary to create
a new driver rather than expand an existing one.

In the patches, in addition to writing the driver:
- convert commonly used structures and functions of the meson-gx-socinfo
driver to a header file.
- add secure-monitor references for amlogic,meson-gx-ao-secure sections
in dts files of the a1, axg, g12, gx families.


---

Patch based on top:
- add new A113X SoC https://lore.kernel.org/linux-kernel/171766521601.3911648.2220702176918701226.b4-ty@linaro.org/T/
- Add S905L ID https://lore.kernel.org/linux-kernel/171766521524.3911648.14792995642693649032.b4-ty@linaro.org/T/

https://git.kernel.org/pub/scm/linux/kernel/git/amlogic/linux.git (v6.11/drivers)

Changes
 v4 [1] -> v5:
 - split patch for A113X SoC id
 - fix warnings
 - add dt-bindings
 v3 [2] -> v4:
 - rebase
 - fix double free of pointers, thanks to Krzysztof Kozlowski

 v2 [3] -> v3:
 - rebase
 - update dependency in Kconfig for MESON_GX_SOCINFO_SM
 - add links to secure-monitor in soc driver sections for A1, AXG, GX, G12

 v1 [4] -> v2:
 - create cpu_id structure for socinfo variable
 - create meson_sm_chip_id for result of sm call
 - remove shared functions
 - move from funcs for bit operations to C bit fields


Links:
 - [1] https://lore.kernel.org/linux-kernel/20240516112849.3803674-2-adeep@lexina.in/
 - [2] https://lore.kernel.org/linux-arm-kernel/ZfMJ_Z07QkwFbOuQ@bogus/T/
 - [3] https://lore.kernel.org/linux-arm-kernel/20240221143654.544444-1-adeep@lexina.in/
 - [4] https://lore.kernel.org/linux-arm-kernel/202311242104.RjBPI3uI-lkp@intel.com/T/


Viacheslav Bocharov (4):
  soc: amlogic: meson-gx-socinfo: move common code to header file
  soc: amlogic: meson-gx-socinfo-sm: Add Amlogic secure-monitor SoC
    Information driver
  dt-bindings: arm: amlogic: amlogic,meson-gx-ao-secure: add
    secure-monitor property
  arm64: dts: meson: add dts links to secure-monitor for soc driver in
    a1, axg, gx, g12

 .../amlogic/amlogic,meson-gx-ao-secure.yaml   |   4 +
 arch/arm64/boot/dts/amlogic/meson-a1.dtsi     |   1 +
 arch/arm64/boot/dts/amlogic/meson-axg.dtsi    |   1 +
 .../boot/dts/amlogic/meson-g12-common.dtsi    |   1 +
 arch/arm64/boot/dts/amlogic/meson-gx.dtsi     |   1 +
 drivers/soc/amlogic/Kconfig                   |  10 +
 drivers/soc/amlogic/Makefile                  |   1 +
 .../soc/amlogic/meson-gx-socinfo-internal.h   | 128 ++++++++++++
 drivers/soc/amlogic/meson-gx-socinfo-sm.c     | 190 ++++++++++++++++++
 drivers/soc/amlogic/meson-gx-socinfo.c        | 140 ++-----------
 10 files changed, 352 insertions(+), 125 deletions(-)
 create mode 100644 drivers/soc/amlogic/meson-gx-socinfo-internal.h
 create mode 100644 drivers/soc/amlogic/meson-gx-socinfo-sm.c


base-commit: 9584e2b31432a608bf29d074ac39e855eeee2207
-- 
2.45.2

[PATCH v5 1/4] soc: amlogic: meson-gx-socinfo: move common code to header file

[Viacheslav Bocharov]

Move common constants and inline functions from meson-gx-socinfo driver
to header file. Create new structures for store meson64 cpu_id and chip_id.

Signed-off-by: Viacheslav Bocharov <adeep@lexina.in>
---
 .../soc/amlogic/meson-gx-socinfo-internal.h   | 122 +++++++++++++++
 drivers/soc/amlogic/meson-gx-socinfo.c        | 140 ++----------------
 2 files changed, 137 insertions(+), 125 deletions(-)
 create mode 100644 drivers/soc/amlogic/meson-gx-socinfo-internal.h

diff --git a/drivers/soc/amlogic/meson-gx-socinfo-internal.h b/drivers/soc/amlogic/meson-gx-socinfo-internal.h
new file mode 100644
index 000000000000..42cd57e67390
--- /dev/null
+++ b/drivers/soc/amlogic/meson-gx-socinfo-internal.h
@@ -0,0 +1,122 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (c) 2017 BayLibre, SAS
+ * Copyright (c) 2024 JetHome
+ * Author: Neil Armstrong <neil.armstrong@linaro.org>
+ * Author: Viacheslav Bocharov <adeep@lexina.in>
+ *
+ */
+
+#ifndef _MESON_GX_SOCINFO_INTERNAL_H_
+#define _MESON_GX_SOCINFO_INTERNAL_H_
+
+#include <linux/types.h>
+
+#define AO_SEC_SD_CFG8		0xe0
+#define AO_SEC_SOCINFO_OFFSET	AO_SEC_SD_CFG8
+
+union meson_cpu_id {
+	struct { // cpu_id v1
+		u32 layout_ver:4;
+		u32 reserved:4;
+		u32 chip_rev:8;
+		u32 pack_id:8;
+		u32 major_id:8;
+	} v1;
+	struct { // cpu_id v2
+		u32 major_id:8;
+		u32 chip_rev:8;
+		u32 pack_id:8;
+		u32 reserved:4;
+		u32 layout_ver:4;
+	} v2;
+	u32	raw;
+};
+
+struct meson_sm_chip_id {
+	u32 version;
+	union meson_cpu_id cpu_id;
+	u8 serial[12];
+};
+
+static const struct meson_gx_soc_id {
+	const char *name;
+	unsigned int id;
+} soc_ids[] = {
+	{ "GXBB", 0x1f },
+	{ "GXTVBB", 0x20 },
+	{ "GXL", 0x21 },
+	{ "GXM", 0x22 },
+	{ "TXL", 0x23 },
+	{ "TXLX", 0x24 },
+	{ "AXG", 0x25 },
+	{ "GXLX", 0x26 },
+	{ "TXHD", 0x27 },
+	{ "G12A", 0x28 },
+	{ "G12B", 0x29 },
+	{ "SM1", 0x2b },
+	{ "A1", 0x2c },
+};
+
+static const struct meson_gx_package_id {
+	const char *name;
+	unsigned int major_id;
+	unsigned int pack_id;
+	unsigned int pack_mask;
+} soc_packages[] = {
+	{ "S905", 0x1f, 0, 0x20 }, /* pack_id != 0x20 */
+	{ "S905H", 0x1f, 0x3, 0xf }, /* pack_id & 0xf == 0x3 */
+	{ "S905M", 0x1f, 0x20, 0xf0 }, /* pack_id == 0x20 */
+	{ "S905D", 0x21, 0, 0xf0 },
+	{ "S905X", 0x21, 0x80, 0xf0 },
+	{ "S905W", 0x21, 0xa0, 0xf0 },
+	{ "S905L", 0x21, 0xc0, 0xf0 },
+	{ "S905M2", 0x21, 0xe0, 0xf0 },
+	{ "S805X", 0x21, 0x30, 0xf0 },
+	{ "S805Y", 0x21, 0xb0, 0xf0 },
+	{ "S912", 0x22, 0, 0x0 }, /* Only S912 is known for GXM */
+	{ "962X", 0x24, 0x10, 0xf0 },
+	{ "962E", 0x24, 0x20, 0xf0 },
+	{ "A113X", 0x25, 0x37, 0xff },
+	{ "A113X", 0x25, 0x43, 0xff },
+	{ "A113D", 0x25, 0x22, 0xff },
+	{ "S905L", 0x26, 0, 0x0 },
+	{ "S905D2", 0x28, 0x10, 0xf0 },
+	{ "S905Y2", 0x28, 0x30, 0xf0 },
+	{ "S905X2", 0x28, 0x40, 0xf0 },
+	{ "A311D", 0x29, 0x10, 0xf0 },
+	{ "S922X", 0x29, 0x40, 0xf0 },
+	{ "S905D3", 0x2b, 0x4, 0xf5 },
+	{ "S905X3", 0x2b, 0x5, 0xf5 },
+	{ "S905X3", 0x2b, 0x10, 0x3f },
+	{ "S905D3", 0x2b, 0x30, 0x3f },
+	{ "A113L", 0x2c, 0x0, 0xf8 },
+};
+
+static inline const char *socinfo_v1_to_package_id(union meson_cpu_id socinfo)
+{
+	int i;
+
+	for (i = 0 ; i < ARRAY_SIZE(soc_packages) ; ++i) {
+		if (soc_packages[i].major_id == socinfo.v1.major_id &&
+		    soc_packages[i].pack_id ==
+				(socinfo.v1.pack_id & soc_packages[i].pack_mask))
+			return soc_packages[i].name;
+	}
+
+	return "Unknown";
+}
+
+static inline const char *socinfo_v1_to_soc_id(union meson_cpu_id socinfo)
+{
+	int i;
+
+	for (i = 0 ; i < ARRAY_SIZE(soc_ids) ; ++i) {
+		if (soc_ids[i].id == socinfo.v1.major_id)
+			return soc_ids[i].name;
+	}
+
+	return "Unknown";
+}
+
+#endif /* _MESON_GX_SOCINFO_INTERNAL_H_ */
diff --git a/drivers/soc/amlogic/meson-gx-socinfo.c b/drivers/soc/amlogic/meson-gx-socinfo.c
index 8809a948201a..ca7b82c07083 100644
--- a/drivers/soc/amlogic/meson-gx-socinfo.c
+++ b/drivers/soc/amlogic/meson-gx-socinfo.c
@@ -1,8 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0+
 /*
  * Copyright (c) 2017 BayLibre, SAS
  * Author: Neil Armstrong <narmstrong@baylibre.com>
  *
- * SPDX-License-Identifier: GPL-2.0+
  */
 
 #include <linux/io.h>
@@ -12,120 +12,10 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/sys_soc.h>
-#include <linux/bitfield.h>
 #include <linux/regmap.h>
 #include <linux/mfd/syscon.h>
 
-#define AO_SEC_SD_CFG8		0xe0
-#define AO_SEC_SOCINFO_OFFSET	AO_SEC_SD_CFG8
-
-#define SOCINFO_MAJOR	GENMASK(31, 24)
-#define SOCINFO_PACK	GENMASK(23, 16)
-#define SOCINFO_MINOR	GENMASK(15, 8)
-#define SOCINFO_MISC	GENMASK(7, 0)
-
-static const struct meson_gx_soc_id {
-	const char *name;
-	unsigned int id;
-} soc_ids[] = {
-	{ "GXBB", 0x1f },
-	{ "GXTVBB", 0x20 },
-	{ "GXL", 0x21 },
-	{ "GXM", 0x22 },
-	{ "TXL", 0x23 },
-	{ "TXLX", 0x24 },
-	{ "AXG", 0x25 },
-	{ "GXLX", 0x26 },
-	{ "TXHD", 0x27 },
-	{ "G12A", 0x28 },
-	{ "G12B", 0x29 },
-	{ "SM1", 0x2b },
-	{ "A1", 0x2c },
-};
-
-static const struct meson_gx_package_id {
-	const char *name;
-	unsigned int major_id;
-	unsigned int pack_id;
-	unsigned int pack_mask;
-} soc_packages[] = {
-	{ "S905", 0x1f, 0, 0x20 }, /* pack_id != 0x20 */
-	{ "S905H", 0x1f, 0x3, 0xf }, /* pack_id & 0xf == 0x3 */
-	{ "S905M", 0x1f, 0x20, 0xf0 }, /* pack_id == 0x20 */
-	{ "S905D", 0x21, 0, 0xf0 },
-	{ "S905X", 0x21, 0x80, 0xf0 },
-	{ "S905W", 0x21, 0xa0, 0xf0 },
-	{ "S905L", 0x21, 0xc0, 0xf0 },
-	{ "S905M2", 0x21, 0xe0, 0xf0 },
-	{ "S805X", 0x21, 0x30, 0xf0 },
-	{ "S805Y", 0x21, 0xb0, 0xf0 },
-	{ "S912", 0x22, 0, 0x0 }, /* Only S912 is known for GXM */
-	{ "962X", 0x24, 0x10, 0xf0 },
-	{ "962E", 0x24, 0x20, 0xf0 },
-	{ "A113X", 0x25, 0x37, 0xff },
-	{ "A113X", 0x25, 0x43, 0xff },
-	{ "A113D", 0x25, 0x22, 0xff },
-	{ "S905L", 0x26, 0, 0x0 },
-	{ "S905D2", 0x28, 0x10, 0xf0 },
-	{ "S905Y2", 0x28, 0x30, 0xf0 },
-	{ "S905X2", 0x28, 0x40, 0xf0 },
-	{ "A311D", 0x29, 0x10, 0xf0 },
-	{ "S922X", 0x29, 0x40, 0xf0 },
-	{ "S905D3", 0x2b, 0x4, 0xf5 },
-	{ "S905X3", 0x2b, 0x5, 0xf5 },
-	{ "S905X3", 0x2b, 0x10, 0x3f },
-	{ "S905D3", 0x2b, 0x30, 0x3f },
-	{ "A113L", 0x2c, 0x0, 0xf8 },
-};
-
-static inline unsigned int socinfo_to_major(u32 socinfo)
-{
-	return FIELD_GET(SOCINFO_MAJOR, socinfo);
-}
-
-static inline unsigned int socinfo_to_minor(u32 socinfo)
-{
-	return FIELD_GET(SOCINFO_MINOR, socinfo);
-}
-
-static inline unsigned int socinfo_to_pack(u32 socinfo)
-{
-	return FIELD_GET(SOCINFO_PACK, socinfo);
-}
-
-static inline unsigned int socinfo_to_misc(u32 socinfo)
-{
-	return FIELD_GET(SOCINFO_MISC, socinfo);
-}
-
-static const char *socinfo_to_package_id(u32 socinfo)
-{
-	unsigned int pack = socinfo_to_pack(socinfo);
-	unsigned int major = socinfo_to_major(socinfo);
-	int i;
-
-	for (i = 0 ; i < ARRAY_SIZE(soc_packages) ; ++i) {
-		if (soc_packages[i].major_id == major &&
-		    soc_packages[i].pack_id ==
-				(pack & soc_packages[i].pack_mask))
-			return soc_packages[i].name;
-	}
-
-	return "Unknown";
-}
-
-static const char *socinfo_to_soc_id(u32 socinfo)
-{
-	unsigned int id = socinfo_to_major(socinfo);
-	int i;
-
-	for (i = 0 ; i < ARRAY_SIZE(soc_ids) ; ++i) {
-		if (soc_ids[i].id == id)
-			return soc_ids[i].name;
-	}
-
-	return "Unknown";
-}
+#include "meson-gx-socinfo-internal.h"
 
 static int __init meson_gx_socinfo_init(void)
 {
@@ -133,7 +23,7 @@ static int __init meson_gx_socinfo_init(void)
 	struct soc_device *soc_dev;
 	struct device_node *np;
 	struct regmap *regmap;
-	unsigned int socinfo;
+	union meson_cpu_id socinfo;
 	struct device *dev;
 	int ret;
 
@@ -162,11 +52,11 @@ static int __init meson_gx_socinfo_init(void)
 		return -ENODEV;
 	}
 
-	ret = regmap_read(regmap, AO_SEC_SOCINFO_OFFSET, &socinfo);
+	ret = regmap_read(regmap, AO_SEC_SOCINFO_OFFSET, &socinfo.raw);
 	if (ret < 0)
 		return ret;
 
-	if (!socinfo) {
+	if (!socinfo.raw) {
 		pr_err("%s: invalid chipid value\n", __func__);
 		return -EINVAL;
 	}
@@ -177,13 +67,13 @@ static int __init meson_gx_socinfo_init(void)
 
 	soc_dev_attr->family = "Amlogic Meson";
 	soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%x:%x - %x:%x",
-					   socinfo_to_major(socinfo),
-					   socinfo_to_minor(socinfo),
-					   socinfo_to_pack(socinfo),
-					   socinfo_to_misc(socinfo));
+					   socinfo.v1.major_id,
+					   socinfo.v1.chip_rev,
+					   socinfo.v1.pack_id,
+					   (socinfo.v1.reserved<<4) + socinfo.v1.layout_ver);
 	soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "%s (%s)",
-					 socinfo_to_soc_id(socinfo),
-					 socinfo_to_package_id(socinfo));
+					 socinfo_v1_to_soc_id(socinfo),
+					 socinfo_v1_to_package_id(socinfo));
 
 	soc_dev = soc_device_register(soc_dev_attr);
 	if (IS_ERR(soc_dev)) {
@@ -196,10 +86,10 @@ static int __init meson_gx_socinfo_init(void)
 
 	dev_info(dev, "Amlogic Meson %s Revision %x:%x (%x:%x) Detected\n",
 			soc_dev_attr->soc_id,
-			socinfo_to_major(socinfo),
-			socinfo_to_minor(socinfo),
-			socinfo_to_pack(socinfo),
-			socinfo_to_misc(socinfo));
+			socinfo.v1.major_id,
+			socinfo.v1.chip_rev,
+			socinfo.v1.pack_id,
+			(socinfo.v1.reserved<<4) + socinfo.v1.layout_ver);
 
 	return 0;
 }
-- 
2.45.2

[PATCH v5 2/4] soc: amlogic: meson-gx-socinfo-sm: Add Amlogic secure-monitor SoC Information driver

[Viacheslav Bocharov]

Amlogic SoCs have a SoC information secure-monitor call for SoC type,
package type, revision information and chipid.
This patchs adds support for secure-monitor call decoding and exposing
with the SoC bus infrastructure in addition to the previous SoC
Information driver.

Signed-off-by: Viacheslav Bocharov <adeep@lexina.in>
---
 drivers/soc/amlogic/Kconfig                   |  10 +
 drivers/soc/amlogic/Makefile                  |   1 +
 .../soc/amlogic/meson-gx-socinfo-internal.h   |  14 +-
 drivers/soc/amlogic/meson-gx-socinfo-sm.c     | 190 ++++++++++++++++++
 4 files changed, 211 insertions(+), 4 deletions(-)
 create mode 100644 drivers/soc/amlogic/meson-gx-socinfo-sm.c

diff --git a/drivers/soc/amlogic/Kconfig b/drivers/soc/amlogic/Kconfig
index d08e398bdad4..82fc77ca3b4b 100644
--- a/drivers/soc/amlogic/Kconfig
+++ b/drivers/soc/amlogic/Kconfig
@@ -26,6 +26,16 @@ config MESON_GX_SOCINFO
 	  Say yes to support decoding of Amlogic Meson GX SoC family
 	  information about the type, package and version.
 
+config MESON_GX_SOCINFO_SM
+	bool "Amlogic Meson GX SoC Information driver via Secure Monitor"
+	depends on (ARM64 && ARCH_MESON || COMPILE_TEST) && MESON_SM=y
+	default ARCH_MESON && MESON_SM
+	select SOC_BUS
+	help
+	  Say yes to support decoding of Amlogic Meson GX SoC family
+	  information about the type, package and version via secure
+	  monitor call.
+
 config MESON_MX_SOCINFO
 	bool "Amlogic Meson MX SoC Information driver"
 	depends on (ARM && ARCH_MESON) || COMPILE_TEST
diff --git a/drivers/soc/amlogic/Makefile b/drivers/soc/amlogic/Makefile
index c25f835e6a26..45d9d6f5904c 100644
--- a/drivers/soc/amlogic/Makefile
+++ b/drivers/soc/amlogic/Makefile
@@ -2,4 +2,5 @@
 obj-$(CONFIG_MESON_CANVAS) += meson-canvas.o
 obj-$(CONFIG_MESON_CLK_MEASURE) += meson-clk-measure.o
 obj-$(CONFIG_MESON_GX_SOCINFO) += meson-gx-socinfo.o
+obj-$(CONFIG_MESON_GX_SOCINFO_SM) += meson-gx-socinfo-sm.o
 obj-$(CONFIG_MESON_MX_SOCINFO) += meson-mx-socinfo.o
diff --git a/drivers/soc/amlogic/meson-gx-socinfo-internal.h b/drivers/soc/amlogic/meson-gx-socinfo-internal.h
index 42cd57e67390..5559573c9024 100644
--- a/drivers/soc/amlogic/meson-gx-socinfo-internal.h
+++ b/drivers/soc/amlogic/meson-gx-socinfo-internal.h
@@ -33,10 +33,16 @@ union meson_cpu_id {
 	u32	raw;
 };
 
-struct meson_sm_chip_id {
-	u32 version;
-	union meson_cpu_id cpu_id;
-	u8 serial[12];
+union meson_sm_chip_id {
+	struct { // cpu_id v2
+		u32 version;
+		union meson_cpu_id cpu_id;
+		u8 serial[12];
+	} v2;
+	struct { // raw
+		u32 version;
+		u8 buf[12 + sizeof(union meson_cpu_id)];
+	} raw;
 };
 
 static const struct meson_gx_soc_id {
diff --git a/drivers/soc/amlogic/meson-gx-socinfo-sm.c b/drivers/soc/amlogic/meson-gx-socinfo-sm.c
new file mode 100644
index 000000000000..e12f45cd8de2
--- /dev/null
+++ b/drivers/soc/amlogic/meson-gx-socinfo-sm.c
@@ -0,0 +1,190 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2017 BayLibre, SAS
+ * Copyright (c) 2024 JetHome
+ * Author: Neil Armstrong <neil.armstrong@linaro.org>
+ * Author: Viacheslav Bocharov <adeep@lexina.in>
+ *
+ */
+
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/sys_soc.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+
+#include <linux/firmware/meson/meson_sm.h>
+
+#include "meson-gx-socinfo-internal.h"
+
+static char *socinfo_get_chipid(struct device *dev, struct meson_sm_firmware *fw,
+			       union meson_cpu_id *socinfo)
+{
+	char *buf;
+	union meson_sm_chip_id *id_buf;
+	int ret;
+
+	id_buf = kzalloc(sizeof(union meson_sm_chip_id)+1, GFP_KERNEL);
+	if (!id_buf)
+		return NULL;
+
+	ret = meson_sm_call_read(fw, id_buf, sizeof(union meson_sm_chip_id), SM_GET_CHIP_ID,
+				 2, 0, 0, 0, 0);
+	if (ret < 0) {
+		kfree(id_buf);
+		return NULL;
+	}
+	dev_info(dev, "got sm version call %i\n", id_buf->raw.version);
+
+	if (id_buf->raw.version != 2) {
+
+		u8 tmp;
+		/**
+		 * Legacy 12-byte chip ID read out, transform data
+		 * to expected order format
+		 */
+		memmove((void *)&id_buf->v2.serial, (void *)&id_buf->raw.buf, 12);
+		for (int i = 0; i < 6; i++) {
+			tmp = id_buf->v2.serial[i];
+			id_buf->v2.serial[i] = id_buf->v2.serial[11 - i];
+			id_buf->v2.serial[11 - i] = tmp;
+		}
+		id_buf->v2.cpu_id.v2.major_id = socinfo->v1.major_id;
+		id_buf->v2.cpu_id.v2.pack_id = socinfo->v1.pack_id;
+		id_buf->v2.cpu_id.v2.chip_rev = socinfo->v1.chip_rev;
+		id_buf->v2.cpu_id.v2.reserved = socinfo->v1.reserved;
+		id_buf->v2.cpu_id.v2.layout_ver = socinfo->v1.layout_ver;
+	} else {
+		/**
+		 * rewrite socinfo from regmap with value from secure monitor call
+		 */
+		socinfo->v1.major_id = id_buf->v2.cpu_id.v2.major_id;
+		socinfo->v1.pack_id = id_buf->v2.cpu_id.v2.pack_id;
+		socinfo->v1.chip_rev = id_buf->v2.cpu_id.v2.chip_rev;
+		socinfo->v1.reserved = id_buf->v2.cpu_id.v2.reserved;
+		socinfo->v1.layout_ver = id_buf->v2.cpu_id.v2.layout_ver;
+	}
+
+	buf = devm_kasprintf(dev, GFP_KERNEL, "%4phN%12phN", &(id_buf->v2.cpu_id),
+			     &(id_buf->v2.serial));
+
+	kfree(id_buf);
+
+	return buf;
+}
+
+static int meson_gx_socinfo_sm_probe(struct platform_device *pdev)
+{
+	struct soc_device_attribute *soc_dev_attr;
+	struct soc_device *soc_dev;
+	struct device_node *sm_np;
+	struct meson_sm_firmware *fw;
+	struct regmap *regmap;
+	union meson_cpu_id socinfo;
+	struct device *dev;
+	int ret;
+
+	/* check if chip-id is available */
+	if (!of_property_read_bool(pdev->dev.of_node, "amlogic,has-chip-id"))
+		return -ENODEV;
+
+	/* node should be a syscon */
+	regmap = syscon_node_to_regmap(pdev->dev.of_node);
+	if (IS_ERR(regmap))
+		return dev_err_probe(&pdev->dev, PTR_ERR(regmap), "failed to get regmap\n");
+
+	sm_np = of_parse_phandle(pdev->dev.of_node, "secure-monitor", 0);
+	if (!sm_np) {
+		dev_err(&pdev->dev, "no secure-monitor node found\n");
+		return -EINVAL;
+	}
+
+	fw = meson_sm_get(sm_np);
+	of_node_put(sm_np);
+	if (!fw) {
+		dev_dbg(&pdev->dev, "secure-monitor device not ready, probe later\n");
+		return -EPROBE_DEFER;
+	}
+
+	ret = regmap_read(regmap, AO_SEC_SOCINFO_OFFSET, &socinfo.raw);
+	if (ret < 0)
+		return ret;
+
+	if (!socinfo.raw) {
+		dev_err(&pdev->dev, "invalid regmap chipid value\n");
+		return -EINVAL;
+	}
+
+	soc_dev_attr = devm_kzalloc(&pdev->dev, sizeof(*soc_dev_attr),
+				    GFP_KERNEL);
+	if (!soc_dev_attr)
+		return -ENOMEM;
+
+	soc_dev_attr->serial_number = socinfo_get_chipid(&pdev->dev, fw, &socinfo);
+
+	soc_dev_attr->family = "Amlogic Meson";
+	soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%x:%x - %x:%x",
+					   socinfo.v1.major_id,
+					   socinfo.v1.chip_rev,
+					   socinfo.v1.pack_id,
+					   (socinfo.v1.reserved<<4) + socinfo.v1.layout_ver);
+	soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "%s (%s)",
+					 socinfo_v1_to_soc_id(socinfo),
+					 socinfo_v1_to_package_id(socinfo));
+
+	soc_dev = soc_device_register(soc_dev_attr);
+
+
+	if (IS_ERR(soc_dev)) {
+		kfree(soc_dev_attr->revision);
+		kfree_const(soc_dev_attr->soc_id);
+		return PTR_ERR(soc_dev);
+	}
+
+	dev = soc_device_to_device(soc_dev);
+	platform_set_drvdata(pdev, soc_dev);
+
+	dev_info(dev, "Amlogic Meson %s Revision %x:%x (%x:%x) Detected (SM)\n",
+			soc_dev_attr->soc_id,
+			socinfo.v1.major_id,
+			socinfo.v1.chip_rev,
+			socinfo.v1.pack_id,
+			(socinfo.v1.reserved<<4) + socinfo.v1.layout_ver);
+
+	return PTR_ERR_OR_ZERO(dev);
+}
+
+
+static int meson_gx_socinfo_sm_remove(struct platform_device *pdev)
+{
+	struct soc_device *soc_dev = platform_get_drvdata(pdev);
+
+	soc_device_unregister(soc_dev);
+	return 0;
+}
+
+static const struct of_device_id meson_gx_socinfo_match[] = {
+	{ .compatible = "amlogic,meson-gx-ao-secure", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, meson_gx_socinfo_match);
+
+static struct platform_driver meson_gx_socinfo_driver = {
+	.probe = meson_gx_socinfo_sm_probe,
+	.remove	= meson_gx_socinfo_sm_remove,
+	.driver = {
+		.name = "meson-gx-socinfo-sm",
+		.of_match_table = meson_gx_socinfo_match,
+	},
+};
+
+
+module_platform_driver(meson_gx_socinfo_driver);
+
+MODULE_AUTHOR("Viacheslav Bocharov <adeep@lexina.in>");
+MODULE_DESCRIPTION("Amlogic Meson GX SOC SM driver");
+MODULE_LICENSE("GPL");
-- 
2.45.2

[PATCH v5 3/4] dt-bindings: arm: amlogic: amlogic,meson-gx-ao-secure: add secure-monitor property

[Viacheslav Bocharov]

Add secure-monitor property to schema for meson-gx-socinfo-sm driver.

Signed-off-by: Viacheslav Bocharov <adeep@lexina.in>
---
 .../bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml      | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml b/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml
index 7dff32f373cb..1128a794ec89 100644
--- a/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml
+++ b/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml
@@ -32,6 +32,10 @@ properties:
   reg:
     maxItems: 1
 
+  secure-monitor:
+    description: phandle to the secure-monitor node
+    $ref: /schemas/types.yaml#/definitions/phandle
+
   amlogic,has-chip-id:
     description: |
       A firmware register encodes the SoC type, package and revision
-- 
2.45.2

[PATCH v5 4/4] arm64: dts: meson: add dts links to secure-monitor for soc driver in a1, axg, gx, g12

[Viacheslav Bocharov]

Add links to secure-monitor in soc driver section for A1, AXG, GX, G12
Amlogic family for use with meson-socinfo-sm driver.

Signed-off-by: Viacheslav Bocharov <adeep@lexina.in>
---
 arch/arm64/boot/dts/amlogic/meson-a1.dtsi         | 1 +
 arch/arm64/boot/dts/amlogic/meson-axg.dtsi        | 1 +
 arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi | 1 +
 arch/arm64/boot/dts/amlogic/meson-gx.dtsi         | 1 +
 4 files changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-a1.dtsi b/arch/arm64/boot/dts/amlogic/meson-a1.dtsi
index c03e207ea6c5..d47f056117fc 100644
--- a/arch/arm64/boot/dts/amlogic/meson-a1.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-a1.dtsi
@@ -407,6 +407,7 @@ hwrng: rng@5118 {
 			sec_AO: ao-secure@5a20 {
 				compatible = "amlogic,meson-gx-ao-secure", "syscon";
 				reg = <0x0 0x5a20 0x0 0x140>;
+				secure-monitor = <&sm>;
 				amlogic,has-chip-id;
 			};
 
diff --git a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
index 6d12b760b90f..45791ec6694a 100644
--- a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
@@ -1689,6 +1689,7 @@ mux {
 			sec_AO: ao-secure@140 {
 				compatible = "amlogic,meson-gx-ao-secure", "syscon";
 				reg = <0x0 0x140 0x0 0x140>;
+				secure-monitor = <&sm>;
 				amlogic,has-chip-id;
 			};
 
diff --git a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
index b058ed78faf0..11422d0be22b 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
@@ -2045,6 +2045,7 @@ cec_AO: cec@100 {
 			sec_AO: ao-secure@140 {
 				compatible = "amlogic,meson-gx-ao-secure", "syscon";
 				reg = <0x0 0x140 0x0 0x140>;
+				secure-monitor = <&sm>;
 				amlogic,has-chip-id;
 			};
 
diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
index 2673f0dbafe7..656e08b3d872 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
@@ -471,6 +471,7 @@ cec_AO: cec@100 {
 			sec_AO: ao-secure@140 {
 				compatible = "amlogic,meson-gx-ao-secure", "syscon";
 				reg = <0x0 0x140 0x0 0x140>;
+				secure-monitor = <&sm>;
 				amlogic,has-chip-id;
 			};
 
-- 
2.45.2

Re: [PATCH v5 3/4] dt-bindings: arm: amlogic: amlogic,meson-gx-ao-secure: add secure-monitor property

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1259 bytes --]

On Mon, Jun 10, 2024 at 11:39:49AM +0300, Viacheslav Bocharov wrote:
> Add secure-monitor property to schema for meson-gx-socinfo-sm driver.

"bindings are for hardware, not drivers". Why purpose does the "secure
monitor" serve that the secure firmware needs a reference to it?

Thanks,
Conor.

> 
> Signed-off-by: Viacheslav Bocharov <adeep@lexina.in>
> ---
>  .../bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml      | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml b/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml
> index 7dff32f373cb..1128a794ec89 100644
> --- a/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml
> +++ b/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml
> @@ -32,6 +32,10 @@ properties:
>    reg:
>      maxItems: 1
>  
> +  secure-monitor:

Missing a vendor prefix.

> +    description: phandle to the secure-monitor node
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +
>    amlogic,has-chip-id:
>      description: |
>        A firmware register encodes the SoC type, package and revision
> -- 
> 2.45.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v5 3/4] dt-bindings: arm: amlogic: amlogic,meson-gx-ao-secure: add secure-monitor property

[Viacheslav]

Hi!

10/06/2024 19.08, Conor Dooley wrote:
> On Mon, Jun 10, 2024 at 11:39:49AM +0300, Viacheslav Bocharov wrote:
>> Add secure-monitor property to schema for meson-gx-socinfo-sm driver.
> 
> "bindings are for hardware, not drivers". Why purpose does the "secure
> monitor" serve that the secure firmware needs a reference to it?

This driver is an extension to the meson-gx-socinfo driver: it 
supplements information obtained from the register with information from 
the SM_GET_CHIP_ID secure monitor call. Due to the specifics of the 
module loading order, we cannot do away with meson-gx-socinfo, as it is 
used for platform identification in some drivers. Therefore, the 
extended information is formatted as a separate driver, which is loaded 
after the secure-monitor driver.

The ability to obtain additional information depends on the support for 
the call in the secure-monitor, which can be described by an additional 
link from the amlogic,meson-gx-ao-secure node to the secure-monitor 
node, similar to how it is done for amlogic,meson-gxbb-efuse.

> 
> Thanks,
> Conor.
> 
>>
>> Signed-off-by: Viacheslav Bocharov <adeep@lexina.in>
>> ---
>>   .../bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml      | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml b/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml
>> index 7dff32f373cb..1128a794ec89 100644
>> --- a/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml
>> +++ b/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml
>> @@ -32,6 +32,10 @@ properties:
>>     reg:
>>       maxItems: 1
>>   
>> +  secure-monitor:
> 
> Missing a vendor prefix.
> 
>> +    description: phandle to the secure-monitor node
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +
>>     amlogic,has-chip-id:
>>       description: |
>>         A firmware register encodes the SoC type, package and revision
>> -- 
>> 2.45.2
>>
>>
>> _______________________________________________
>> linux-amlogic mailing list
>> linux-amlogic@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-amlogic

--
with regards,
Viacheslav

Re: [PATCH v5 3/4] dt-bindings: arm: amlogic: amlogic,meson-gx-ao-secure: add secure-monitor property

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2816 bytes --]

On Tue, Jun 11, 2024 at 01:25:11PM +0300, Viacheslav wrote:
> Hi!
> 
> 10/06/2024 19.08, Conor Dooley wrote:
> > On Mon, Jun 10, 2024 at 11:39:49AM +0300, Viacheslav Bocharov wrote:
> > > Add secure-monitor property to schema for meson-gx-socinfo-sm driver.
> > 
> > "bindings are for hardware, not drivers". Why purpose does the "secure
> > monitor" serve that the secure firmware needs a reference to it?
> 
> This driver is an extension to the meson-gx-socinfo driver: it supplements
> information obtained from the register with information from the
> SM_GET_CHIP_ID secure monitor call. Due to the specifics of the module
> loading order, we cannot do away with meson-gx-socinfo, as it is used for
> platform identification in some drivers. Therefore, the extended information
> is formatted as a separate driver, which is loaded after the secure-monitor
> driver.

Please stop talking about drivers, this is a binding which is about
hardware. Please provide, in your next version, a commit message that
justifies adding this property without talking about driver probing
order etc, and instead focuses on what service the "secure monitor"
provides etc.

> The ability to obtain additional information depends on the support for the
> call in the secure-monitor, which can be described by an additional link
> from the amlogic,meson-gx-ao-secure node to the secure-monitor node, similar
> to how it is done for amlogic,meson-gxbb-efuse.
> 
> > 
> > Thanks,
> > Conor.
> > 
> > > 
> > > Signed-off-by: Viacheslav Bocharov <adeep@lexina.in>
> > > ---
> > >   .../bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml      | 4 ++++
> > >   1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml b/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml
> > > index 7dff32f373cb..1128a794ec89 100644
> > > --- a/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml
> > > +++ b/Documentation/devicetree/bindings/arm/amlogic/amlogic,meson-gx-ao-secure.yaml
> > > @@ -32,6 +32,10 @@ properties:
> > >     reg:
> > >       maxItems: 1
> > > +  secure-monitor:
> > 
> > Missing a vendor prefix.
> > 
> > > +    description: phandle to the secure-monitor node
> > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > +
> > >     amlogic,has-chip-id:
> > >       description: |
> > >         A firmware register encodes the SoC type, package and revision
> > > -- 
> > > 2.45.2
> > > 
> > > 
> > > _______________________________________________
> > > linux-amlogic mailing list
> > > linux-amlogic@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-amlogic
> 
> --
> with regards,
> Viacheslav

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v5 3/4] dt-bindings: arm: amlogic: amlogic,meson-gx-ao-secure: add secure-monitor property

[Rob Herring]

On Tue, Jun 11, 2024 at 07:07:28PM +0100, Conor Dooley wrote:
> On Tue, Jun 11, 2024 at 01:25:11PM +0300, Viacheslav wrote:
> > Hi!
> > 
> > 10/06/2024 19.08, Conor Dooley wrote:
> > > On Mon, Jun 10, 2024 at 11:39:49AM +0300, Viacheslav Bocharov wrote:
> > > > Add secure-monitor property to schema for meson-gx-socinfo-sm driver.
> > > 
> > > "bindings are for hardware, not drivers". Why purpose does the "secure
> > > monitor" serve that the secure firmware needs a reference to it?
> > 
> > This driver is an extension to the meson-gx-socinfo driver: it supplements
> > information obtained from the register with information from the
> > SM_GET_CHIP_ID secure monitor call. Due to the specifics of the module
> > loading order, we cannot do away with meson-gx-socinfo, as it is used for
> > platform identification in some drivers. Therefore, the extended information
> > is formatted as a separate driver, which is loaded after the secure-monitor
> > driver.
> 
> Please stop talking about drivers, this is a binding which is about
> hardware. Please provide, in your next version, a commit message that
> justifies adding this property without talking about driver probing
> order etc, and instead focuses on what service the "secure monitor"
> provides etc.

To put it another way, how many secure monitors does 1 system have? 

What do you do if the property is not present? You didn't make it 
required which is good because that would be an ABI break.

You only need a link in DT if there are different possible providers or 
some per consumer information to describe (e.g. an interrupt number or 
clock ID). You don't have the latter and likely there is only 1 possible 
provider. 

Rob

Re: [PATCH v5 3/4] dt-bindings: arm: amlogic: amlogic,meson-gx-ao-secure: add secure-monitor property

[Viacheslav]

Thanks for review.

13/06/2024 19.42, Rob Herring wrote:
> On Tue, Jun 11, 2024 at 07:07:28PM +0100, Conor Dooley wrote:
>> On Tue, Jun 11, 2024 at 01:25:11PM +0300, Viacheslav wrote:
>>> Hi!
>>>
>>> 10/06/2024 19.08, Conor Dooley wrote:
>>>> On Mon, Jun 10, 2024 at 11:39:49AM +0300, Viacheslav Bocharov wrote:
>>>>> Add secure-monitor property to schema for meson-gx-socinfo-sm driver.
>>>>
>>>> "bindings are for hardware, not drivers". Why purpose does the "secure
>>>> monitor" serve that the secure firmware needs a reference to it?
>>>
>>> This driver is an extension to the meson-gx-socinfo driver: it supplements
>>> information obtained from the register with information from the
>>> SM_GET_CHIP_ID secure monitor call. Due to the specifics of the module
>>> loading order, we cannot do away with meson-gx-socinfo, as it is used for
>>> platform identification in some drivers. Therefore, the extended information
>>> is formatted as a separate driver, which is loaded after the secure-monitor
>>> driver.
>>
>> Please stop talking about drivers, this is a binding which is about
>> hardware. Please provide, in your next version, a commit message that
>> justifies adding this property without talking about driver probing
>> order etc, and instead focuses on what service the "secure monitor"
>> provides etc.
> 
> To put it another way, how many secure monitors does 1 system have?

One per system in current device tree.


> 
> What do you do if the property is not present? You didn't make it
> required which is good because that would be an ABI break.

We need an indication of the ability to use the secure-monitor to obtain 
additional information within the soc driver. It seemed to me that using 
an explicit reference to the secure-monitor is the best choice.

> 
> You only need a link in DT if there are different possible providers or
> some per consumer information to describe (e.g. an interrupt number or
> clock ID). You don't have the latter and likely there is only 1 possible
> provider.

Would replacing the reference to sm with an option, for example, 
use-secure-monitor = <1>; look more appropriate in this case?


> 
> Rob
> 
> 
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic

-- 
Viacheslav

Re: [PATCH v5 3/4] dt-bindings: arm: amlogic: amlogic,meson-gx-ao-secure: add secure-monitor property

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3126 bytes --]

On Mon, Jun 17, 2024 at 11:21:30AM +0300, Viacheslav wrote:
> Thanks for review.
> 
> 13/06/2024 19.42, Rob Herring wrote:
> > On Tue, Jun 11, 2024 at 07:07:28PM +0100, Conor Dooley wrote:
> > > On Tue, Jun 11, 2024 at 01:25:11PM +0300, Viacheslav wrote:
> > > > Hi!
> > > > 
> > > > 10/06/2024 19.08, Conor Dooley wrote:
> > > > > On Mon, Jun 10, 2024 at 11:39:49AM +0300, Viacheslav Bocharov wrote:
> > > > > > Add secure-monitor property to schema for meson-gx-socinfo-sm driver.
> > > > > 
> > > > > "bindings are for hardware, not drivers". Why purpose does the "secure
> > > > > monitor" serve that the secure firmware needs a reference to it?
> > > > 
> > > > This driver is an extension to the meson-gx-socinfo driver: it supplements
> > > > information obtained from the register with information from the
> > > > SM_GET_CHIP_ID secure monitor call. Due to the specifics of the module
> > > > loading order, we cannot do away with meson-gx-socinfo, as it is used for
> > > > platform identification in some drivers. Therefore, the extended information
> > > > is formatted as a separate driver, which is loaded after the secure-monitor
> > > > driver.
> > > 
> > > Please stop talking about drivers, this is a binding which is about
> > > hardware. Please provide, in your next version, a commit message that
> > > justifies adding this property without talking about driver probing
> > > order etc, and instead focuses on what service the "secure monitor"
> > > provides etc.
> > 
> > To put it another way, how many secure monitors does 1 system have?
> 
> One per system in current device tree.

One per system, or one is currently described per system, but more might
be added later?

> > What do you do if the property is not present? You didn't make it
> > required which is good because that would be an ABI break.
> 
> We need an indication of the ability to use the secure-monitor to obtain
> additional information within the soc driver. It seemed to me that using an
> explicit reference to the secure-monitor is the best choice.
> 
> > 
> > You only need a link in DT if there are different possible providers or
> > some per consumer information to describe (e.g. an interrupt number or
> > clock ID). You don't have the latter and likely there is only 1 possible
> > provider.
> 
> Would replacing the reference to sm with an option, for example,
> use-secure-monitor = <1>; look more appropriate in this case?

Perhaps a silly question, but (provided there's only one per system, why
can't the secure-monitor driver expose a function that you can call to get
a reference to the system-monitor? I did something similar before with
a call to in mpfs_sys_controller_get() mpfs_rng_probe(). Granted,
mpfs-rng is probed from software so it's slightly different to your
case, but the principle is the same and it's not unheard of for code in
drivers/soc to expose interfaces to other drivers like this. You can
just call a function like that, and know whether there's a secure
monitor, without having to retrofit a DT property.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v5 3/4] dt-bindings: arm: amlogic: amlogic,meson-gx-ao-secure: add secure-monitor property

[Viacheslav]

17/06/2024 19.57, Conor Dooley пишет:
> On Mon, Jun 17, 2024 at 11:21:30AM +0300, Viacheslav wrote:
>> Thanks for review.
>>
>> 13/06/2024 19.42, Rob Herring wrote:
>>> On Tue, Jun 11, 2024 at 07:07:28PM +0100, Conor Dooley wrote:
>>>> On Tue, Jun 11, 2024 at 01:25:11PM +0300, Viacheslav wrote:
>>>>> Hi!
>>>>>
>>>>> 10/06/2024 19.08, Conor Dooley wrote:
>>>>>> On Mon, Jun 10, 2024 at 11:39:49AM +0300, Viacheslav Bocharov wrote:
>>>>>>> Add secure-monitor property to schema for meson-gx-socinfo-sm driver.
>>>>>>
>>>>>> "bindings are for hardware, not drivers". Why purpose does the "secure
>>>>>> monitor" serve that the secure firmware needs a reference to it?
>>>>>
>>>>> This driver is an extension to the meson-gx-socinfo driver: it supplements
>>>>> information obtained from the register with information from the
>>>>> SM_GET_CHIP_ID secure monitor call. Due to the specifics of the module
>>>>> loading order, we cannot do away with meson-gx-socinfo, as it is used for
>>>>> platform identification in some drivers. Therefore, the extended information
>>>>> is formatted as a separate driver, which is loaded after the secure-monitor
>>>>> driver.
>>>>
>>>> Please stop talking about drivers, this is a binding which is about
>>>> hardware. Please provide, in your next version, a commit message that
>>>> justifies adding this property without talking about driver probing
>>>> order etc, and instead focuses on what service the "secure monitor"
>>>> provides etc.
>>>
>>> To put it another way, how many secure monitors does 1 system have?
>>
>> One per system in current device tree.
> 
> One per system, or one is currently described per system, but more might
> be added later?

it turns out to be one per system. It's either there or it's not.

> 
>>> What do you do if the property is not present? You didn't make it
>>> required which is good because that would be an ABI break.
>>
>> We need an indication of the ability to use the secure-monitor to obtain
>> additional information within the soc driver. It seemed to me that using an
>> explicit reference to the secure-monitor is the best choice.
>>
>>>
>>> You only need a link in DT if there are different possible providers or
>>> some per consumer information to describe (e.g. an interrupt number or
>>> clock ID). You don't have the latter and likely there is only 1 possible
>>> provider.
>>
>> Would replacing the reference to sm with an option, for example,
>> use-secure-monitor = <1>; look more appropriate in this case?
> 
> Perhaps a silly question, but (provided there's only one per system, why
> can't the secure-monitor driver expose a function that you can call to get
> a reference to the system-monitor? I did something similar before with
> a call to in mpfs_sys_controller_get() mpfs_rng_probe(). Granted,
> mpfs-rng is probed from software so it's slightly different to your
> case, but the principle is the same and it's not unheard of for code in
> drivers/soc to expose interfaces to other drivers like this. You can
> just call a function like that, and know whether there's a secure
> monitor, without having to retrofit a DT property.

That could be an option. But again, nothing prevents me from searching 
for the secure-monitor node throughout the entire DT array.

The question is more about something else, let me try to explain from 
the beginning:

We currently have a soc driver that uses only the register to get basic 
information and it must be loaded early because other modules' behavior 
depends on its information.
There is an option to supplement the register information with 
information from the secure-monitor.
For this, we had to write a new driver that uses the same register 
information as a fallback but can wait for the secure-monitor driver to 
load and add its information to soc.
It seemed logical to me to keep the DT structure the same and just add a 
reference to the secure-monitor (or as a second option, create a 
variable indicating support) for those SoCs that have been tested and 
can provide this information.
Not all Amlogic SoCs support this call, in some (mostly newer 
generations of SoCs), this call returns incorrect information and we and 
colleagues are still figuring out what has changed. But most established 
platforms support this.
We could add this information retrieval to the secure-monitor itself, 
but that would be a completely different story and would not constitute 
a soc driver.

In the end, we need information about the support of the secure-monitor 
call for obtaining information for the soc driver. In my opinion, this 
can only be done by specifying it in the DT in specific files for 
Amlogic platforms: either by referencing the SM or by an option that 
allows checking the SM.

> 
> Thanks,
> Conor.
> 
> 
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v5 3/4] dt-bindings: arm: amlogic: amlogic,meson-gx-ao-secure: add secure-monitor property

[Krzysztof Kozlowski]

On 20/06/2024 09:14, Viacheslav wrote:
> 
> 
> 17/06/2024 19.57, Conor Dooley пишет:
>> On Mon, Jun 17, 2024 at 11:21:30AM +0300, Viacheslav wrote:
>>> Thanks for review.
>>>
>>> 13/06/2024 19.42, Rob Herring wrote:
>>>> On Tue, Jun 11, 2024 at 07:07:28PM +0100, Conor Dooley wrote:
>>>>> On Tue, Jun 11, 2024 at 01:25:11PM +0300, Viacheslav wrote:
>>>>>> Hi!
>>>>>>
>>>>>> 10/06/2024 19.08, Conor Dooley wrote:
>>>>>>> On Mon, Jun 10, 2024 at 11:39:49AM +0300, Viacheslav Bocharov wrote:
>>>>>>>> Add secure-monitor property to schema for meson-gx-socinfo-sm driver.
>>>>>>>
>>>>>>> "bindings are for hardware, not drivers". Why purpose does the "secure
>>>>>>> monitor" serve that the secure firmware needs a reference to it?
>>>>>>
>>>>>> This driver is an extension to the meson-gx-socinfo driver: it supplements
>>>>>> information obtained from the register with information from the
>>>>>> SM_GET_CHIP_ID secure monitor call. Due to the specifics of the module
>>>>>> loading order, we cannot do away with meson-gx-socinfo, as it is used for
>>>>>> platform identification in some drivers. Therefore, the extended information
>>>>>> is formatted as a separate driver, which is loaded after the secure-monitor
>>>>>> driver.
>>>>>
>>>>> Please stop talking about drivers, this is a binding which is about
>>>>> hardware. Please provide, in your next version, a commit message that
>>>>> justifies adding this property without talking about driver probing
>>>>> order etc, and instead focuses on what service the "secure monitor"
>>>>> provides etc.
>>>>
>>>> To put it another way, how many secure monitors does 1 system have?
>>>
>>> One per system in current device tree.
>>
>> One per system, or one is currently described per system, but more might
>> be added later?
> 
> it turns out to be one per system. It's either there or it's not.
> 
>>
>>>> What do you do if the property is not present? You didn't make it
>>>> required which is good because that would be an ABI break.
>>>
>>> We need an indication of the ability to use the secure-monitor to obtain
>>> additional information within the soc driver. It seemed to me that using an
>>> explicit reference to the secure-monitor is the best choice.
>>>
>>>>
>>>> You only need a link in DT if there are different possible providers or
>>>> some per consumer information to describe (e.g. an interrupt number or
>>>> clock ID). You don't have the latter and likely there is only 1 possible
>>>> provider.
>>>
>>> Would replacing the reference to sm with an option, for example,
>>> use-secure-monitor = <1>; look more appropriate in this case?
>>
>> Perhaps a silly question, but (provided there's only one per system, why
>> can't the secure-monitor driver expose a function that you can call to get
>> a reference to the system-monitor? I did something similar before with
>> a call to in mpfs_sys_controller_get() mpfs_rng_probe(). Granted,
>> mpfs-rng is probed from software so it's slightly different to your
>> case, but the principle is the same and it's not unheard of for code in
>> drivers/soc to expose interfaces to other drivers like this. You can
>> just call a function like that, and know whether there's a secure
>> monitor, without having to retrofit a DT property.
> 
> That could be an option. But again, nothing prevents me from searching 
> for the secure-monitor node throughout the entire DT array.
> 
> The question is more about something else, let me try to explain from 
> the beginning:
> 
> We currently have a soc driver that uses only the register to get basic 
> information and it must be loaded early because other modules' behavior 
> depends on its information.

Please provide name/link to the upstream source code (downstream does
not matter).

> There is an option to supplement the register information with 
> information from the secure-monitor.
> For this, we had to write a new driver that uses the same register 
> information as a fallback but can wait for the secure-monitor driver to 
> load and add its information to soc.
> It seemed logical to me to keep the DT structure the same and just add a 
> reference to the secure-monitor (or as a second option, create a 
> variable indicating support) for those SoCs that have been tested and 
> can provide this information.
> Not all Amlogic SoCs support this call, in some (mostly newer 
> generations of SoCs), this call returns incorrect information and we and 
> colleagues are still figuring out what has changed. But most established 
> platforms support this.
> We could add this information retrieval to the secure-monitor itself, 
> but that would be a completely different story and would not constitute 
> a soc driver.
> 
> In the end, we need information about the support of the secure-monitor 
> call for obtaining information for the soc driver. In my opinion, this 
> can only be done by specifying it in the DT in specific files for 
> Amlogic platforms: either by referencing the SM or by an option that 
> allows checking the SM.

That's not the only option. This is SoC specific so can be deduced from
the compatible as well. And this is kind of obvious from this patchset
(actually patch 4): you add it per SoC.

Best regards,
Krzysztof

Re: [PATCH v5 3/4] dt-bindings: arm: amlogic: amlogic,meson-gx-ao-secure: add secure-monitor property

[Krzysztof Kozlowski]

On 20/06/2024 09:19, Krzysztof Kozlowski wrote:
> On 20/06/2024 09:14, Viacheslav wrote:
>>
>>
>> 17/06/2024 19.57, Conor Dooley пишет:
>>> On Mon, Jun 17, 2024 at 11:21:30AM +0300, Viacheslav wrote:
>>>> Thanks for review.
>>>>
>>>> 13/06/2024 19.42, Rob Herring wrote:
>>>>> On Tue, Jun 11, 2024 at 07:07:28PM +0100, Conor Dooley wrote:
>>>>>> On Tue, Jun 11, 2024 at 01:25:11PM +0300, Viacheslav wrote:
>>>>>>> Hi!
>>>>>>>
>>>>>>> 10/06/2024 19.08, Conor Dooley wrote:
>>>>>>>> On Mon, Jun 10, 2024 at 11:39:49AM +0300, Viacheslav Bocharov wrote:
>>>>>>>>> Add secure-monitor property to schema for meson-gx-socinfo-sm driver.
>>>>>>>>
>>>>>>>> "bindings are for hardware, not drivers". Why purpose does the "secure
>>>>>>>> monitor" serve that the secure firmware needs a reference to it?
>>>>>>>
>>>>>>> This driver is an extension to the meson-gx-socinfo driver: it supplements
>>>>>>> information obtained from the register with information from the
>>>>>>> SM_GET_CHIP_ID secure monitor call. Due to the specifics of the module
>>>>>>> loading order, we cannot do away with meson-gx-socinfo, as it is used for
>>>>>>> platform identification in some drivers. Therefore, the extended information
>>>>>>> is formatted as a separate driver, which is loaded after the secure-monitor
>>>>>>> driver.
>>>>>>
>>>>>> Please stop talking about drivers, this is a binding which is about
>>>>>> hardware. Please provide, in your next version, a commit message that
>>>>>> justifies adding this property without talking about driver probing
>>>>>> order etc, and instead focuses on what service the "secure monitor"
>>>>>> provides etc.
>>>>>
>>>>> To put it another way, how many secure monitors does 1 system have?
>>>>
>>>> One per system in current device tree.
>>>
>>> One per system, or one is currently described per system, but more might
>>> be added later?
>>
>> it turns out to be one per system. It's either there or it's not.
>>
>>>
>>>>> What do you do if the property is not present? You didn't make it
>>>>> required which is good because that would be an ABI break.
>>>>
>>>> We need an indication of the ability to use the secure-monitor to obtain
>>>> additional information within the soc driver. It seemed to me that using an
>>>> explicit reference to the secure-monitor is the best choice.
>>>>
>>>>>
>>>>> You only need a link in DT if there are different possible providers or
>>>>> some per consumer information to describe (e.g. an interrupt number or
>>>>> clock ID). You don't have the latter and likely there is only 1 possible
>>>>> provider.
>>>>
>>>> Would replacing the reference to sm with an option, for example,
>>>> use-secure-monitor = <1>; look more appropriate in this case?
>>>
>>> Perhaps a silly question, but (provided there's only one per system, why
>>> can't the secure-monitor driver expose a function that you can call to get
>>> a reference to the system-monitor? I did something similar before with
>>> a call to in mpfs_sys_controller_get() mpfs_rng_probe(). Granted,
>>> mpfs-rng is probed from software so it's slightly different to your
>>> case, but the principle is the same and it's not unheard of for code in
>>> drivers/soc to expose interfaces to other drivers like this. You can
>>> just call a function like that, and know whether there's a secure
>>> monitor, without having to retrofit a DT property.
>>
>> That could be an option. But again, nothing prevents me from searching 
>> for the secure-monitor node throughout the entire DT array.
>>
>> The question is more about something else, let me try to explain from 
>> the beginning:
>>
>> We currently have a soc driver that uses only the register to get basic 
>> information and it must be loaded early because other modules' behavior 
>> depends on its information.
> 
> Please provide name/link to the upstream source code (downstream does
> not matter).
> 
>> There is an option to supplement the register information with 
>> information from the secure-monitor.
>> For this, we had to write a new driver that uses the same register 
>> information as a fallback but can wait for the secure-monitor driver to 
>> load and add its information to soc.
>> It seemed logical to me to keep the DT structure the same and just add a 
>> reference to the secure-monitor (or as a second option, create a 
>> variable indicating support) for those SoCs that have been tested and 
>> can provide this information.
>> Not all Amlogic SoCs support this call, in some (mostly newer 
>> generations of SoCs), this call returns incorrect information and we and 
>> colleagues are still figuring out what has changed. But most established 
>> platforms support this.
>> We could add this information retrieval to the secure-monitor itself, 
>> but that would be a completely different story and would not constitute 
>> a soc driver.
>>
>> In the end, we need information about the support of the secure-monitor 
>> call for obtaining information for the soc driver. In my opinion, this 
>> can only be done by specifying it in the DT in specific files for 
>> Amlogic platforms: either by referencing the SM or by an option that 
>> allows checking the SM.
> 
> That's not the only option. This is SoC specific so can be deduced from
> the compatible as well. And this is kind of obvious from this patchset
> (actually patch 4): you add it per SoC.

BTW, that's one more DT maintainer (so the third) telling you property
is not needed yet. I think we used enough of our time here.

Best regards,
Krzysztof

Re: [PATCH v5 3/4] dt-bindings: arm: amlogic: amlogic,meson-gx-ao-secure: add secure-monitor property

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3457 bytes --]

On Mon, Jun 17, 2024 at 05:57:53PM +0100, Conor Dooley wrote:
> On Mon, Jun 17, 2024 at 11:21:30AM +0300, Viacheslav wrote:
> > Thanks for review.
> > 
> > 13/06/2024 19.42, Rob Herring wrote:
> > > On Tue, Jun 11, 2024 at 07:07:28PM +0100, Conor Dooley wrote:
> > > > On Tue, Jun 11, 2024 at 01:25:11PM +0300, Viacheslav wrote:
> > > > > Hi!
> > > > > 
> > > > > 10/06/2024 19.08, Conor Dooley wrote:
> > > > > > On Mon, Jun 10, 2024 at 11:39:49AM +0300, Viacheslav Bocharov wrote:
> > > > > > > Add secure-monitor property to schema for meson-gx-socinfo-sm driver.
> > > > > > 
> > > > > > "bindings are for hardware, not drivers". Why purpose does the "secure
> > > > > > monitor" serve that the secure firmware needs a reference to it?
> > > > > 
> > > > > This driver is an extension to the meson-gx-socinfo driver: it supplements
> > > > > information obtained from the register with information from the
> > > > > SM_GET_CHIP_ID secure monitor call. Due to the specifics of the module
> > > > > loading order, we cannot do away with meson-gx-socinfo, as it is used for
> > > > > platform identification in some drivers. Therefore, the extended information
> > > > > is formatted as a separate driver, which is loaded after the secure-monitor
> > > > > driver.
> > > > 
> > > > Please stop talking about drivers, this is a binding which is about
> > > > hardware. Please provide, in your next version, a commit message that
> > > > justifies adding this property without talking about driver probing
> > > > order etc, and instead focuses on what service the "secure monitor"
> > > > provides etc.
> > > 
> > > To put it another way, how many secure monitors does 1 system have?
> > 
> > One per system in current device tree.
> 
> One per system, or one is currently described per system, but more might
> be added later?
> 
> > > What do you do if the property is not present? You didn't make it
> > > required which is good because that would be an ABI break.
> > 
> > We need an indication of the ability to use the secure-monitor to obtain
> > additional information within the soc driver. It seemed to me that using an
> > explicit reference to the secure-monitor is the best choice.
> > 
> > > 
> > > You only need a link in DT if there are different possible providers or
> > > some per consumer information to describe (e.g. an interrupt number or
> > > clock ID). You don't have the latter and likely there is only 1 possible
> > > provider.
> > 
> > Would replacing the reference to sm with an option, for example,
> > use-secure-monitor = <1>; look more appropriate in this case?
> 
> Perhaps a silly question, but (provided there's only one per system, why
> can't the secure-monitor driver expose a function that you can call to get
> a reference to the system-monitor? I did something similar before with
> a call to in mpfs_sys_controller_get() mpfs_rng_probe(). Granted,
> mpfs-rng is probed from software so it's slightly different to your
> case, but the principle is the same and it's not unheard of for code in
> drivers/soc to expose interfaces to other drivers like this. You can
> just call a function like that, and know whether there's a secure
> monitor, without having to retrofit a DT property.

Another thing, without having a driver expose an API, is calling
of_find_compatible_node() to find the node. That also doesn't require
retrofitting properties.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]