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

[PATCH 0/5] 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 the second 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.
- transfer the chipid sm call constants to a header file (perhaps they
need renaming?).
- add secure-monitor references for amlogic,meson-gx-ao-secure sections
in dts files of the a1, axg, g12, gx families.

Viacheslav Bocharov (5):
  soc: amlogic: meson-gx-socinfo: move common code to header file
  soc: amlogic: meson-gx-socinfo: move common code to exported function
  firmware: meson_sm: move common definitions to header file
  soc: amlogic: Add Amlogic secure-monitor SoC Information driver
  arm64: dts: meson: add dts links to secure-monitor for soc driver in
    a1, axg, gx, g12

 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/firmware/meson/meson_sm.c             |   4 -
 drivers/soc/amlogic/Kconfig                   |  10 +
 drivers/soc/amlogic/Makefile                  |   1 +
 .../soc/amlogic/meson-gx-socinfo-internal.h   | 102 ++++++++++
 drivers/soc/amlogic/meson-gx-socinfo-sm.c     | 178 ++++++++++++++++++
 drivers/soc/amlogic/meson-gx-socinfo.c        | 106 ++---------
 include/linux/firmware/meson/meson_sm.h       |   4 +
 11 files changed, 317 insertions(+), 92 deletions(-)
 create mode 100644 drivers/soc/amlogic/meson-gx-socinfo-internal.h
 create mode 100644 drivers/soc/amlogic/meson-gx-socinfo-sm.c

-- 
2.34.1

[PATCH 1/5] 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

Signed-off-by: Viacheslav Bocharov <adeep@lexina.in>
---
 .../soc/amlogic/meson-gx-socinfo-internal.h   | 99 +++++++++++++++++++
 drivers/soc/amlogic/meson-gx-socinfo.c        | 80 +--------------
 2 files changed, 100 insertions(+), 79 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..884cf8fb580f
--- /dev/null
+++ b/drivers/soc/amlogic/meson-gx-socinfo-internal.h
@@ -0,0 +1,99 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2017 BayLibre, SAS
+ * Copyright (C) 2023 JetHome
+ *
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ * Author: Viacheslav Bocharov <adeep@lexina.in>
+ */
+
+#ifndef _MESON_GX_SOCINFO_INTERNAL_H_
+#define _MESON_GX_SOCINFO_INTERNAL_H_
+
+#include <linux/bitfield.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 },
+	{ "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);
+}
+
+#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 6abb730344ab..9d7921c0fb91 100644
--- a/drivers/soc/amlogic/meson-gx-socinfo.c
+++ b/drivers/soc/amlogic/meson-gx-socinfo.c
@@ -16,85 +16,7 @@
 #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 },
-	{ "A113D", 0x25, 0x22, 0xff },
-	{ "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);
-}
+#include "meson-gx-socinfo-internal.h"
 
 static const char *socinfo_to_package_id(u32 socinfo)
 {
-- 
2.34.1

[PATCH 2/5] soc: amlogic: meson-gx-socinfo: move common code to exported function

[Viacheslav Bocharov]

Move common code fill soc_device_attribute to common function for
later use

Signed-off-by: Viacheslav Bocharov <adeep@lexina.in>
---
 .../soc/amlogic/meson-gx-socinfo-internal.h   |  3 +++
 drivers/soc/amlogic/meson-gx-socinfo.c        | 26 ++++++++++++-------
 2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/soc/amlogic/meson-gx-socinfo-internal.h b/drivers/soc/amlogic/meson-gx-socinfo-internal.h
index 884cf8fb580f..5a742cc16fc8 100644
--- a/drivers/soc/amlogic/meson-gx-socinfo-internal.h
+++ b/drivers/soc/amlogic/meson-gx-socinfo-internal.h
@@ -95,5 +95,8 @@ static inline unsigned int socinfo_to_misc(u32 socinfo)
 	return FIELD_GET(SOCINFO_MISC, socinfo);
 }
 
+int meson_gx_socinfo_prepare_soc_driver_attr(struct soc_device_attribute *soc_dev_attr,
+					     unsigned int socattr);
+
 #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 9d7921c0fb91..8cf69dd238ee 100644
--- a/drivers/soc/amlogic/meson-gx-socinfo.c
+++ b/drivers/soc/amlogic/meson-gx-socinfo.c
@@ -47,6 +47,22 @@ static const char *socinfo_to_soc_id(u32 socinfo)
 	return "Unknown";
 }
 
+int meson_gx_socinfo_prepare_soc_driver_attr(struct soc_device_attribute *soc_dev_attr,
+					     unsigned int socattr)
+{
+	soc_dev_attr->family = "Amlogic Meson";
+	soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%x:%x - %x:%x",
+					   socinfo_to_major(socattr),
+					   socinfo_to_minor(socattr),
+					   socinfo_to_pack(socattr),
+					   socinfo_to_misc(socattr));
+	soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "%s (%s)",
+					 socinfo_to_soc_id(socattr),
+					 socinfo_to_package_id(socattr));
+	return 0;
+}
+EXPORT_SYMBOL(meson_gx_socinfo_prepare_soc_driver_attr);
+
 static int __init meson_gx_socinfo_init(void)
 {
 	struct soc_device_attribute *soc_dev_attr;
@@ -95,15 +111,7 @@ static int __init meson_gx_socinfo_init(void)
 	if (!soc_dev_attr)
 		return -ENODEV;
 
-	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));
-	soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "%s (%s)",
-					 socinfo_to_soc_id(socinfo),
-					 socinfo_to_package_id(socinfo));
+	meson_gx_socinfo_prepare_soc_driver_attr(soc_dev_attr, socinfo);
 
 	soc_dev = soc_device_register(soc_dev_attr);
 	if (IS_ERR(soc_dev)) {
-- 
2.34.1

[PATCH 3/5] firmware: meson_sm: move common definitions to header file

[Viacheslav Bocharov]

Move SM_CHIPID_* constants from firmware meson sm driver to
header file.

Signed-off-by: Viacheslav Bocharov <adeep@lexina.in>
---
 drivers/firmware/meson/meson_sm.c       | 4 ----
 include/linux/firmware/meson/meson_sm.h | 4 ++++
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/meson/meson_sm.c b/drivers/firmware/meson/meson_sm.c
index 402851ed4ac0..96e50811336f 100644
--- a/drivers/firmware/meson/meson_sm.c
+++ b/drivers/firmware/meson/meson_sm.c
@@ -240,10 +240,6 @@ struct meson_sm_firmware *meson_sm_get(struct device_node *sm_node)
 }
 EXPORT_SYMBOL_GPL(meson_sm_get);
 
-#define SM_CHIP_ID_LENGTH	119
-#define SM_CHIP_ID_OFFSET	4
-#define SM_CHIP_ID_SIZE		12
-
 static ssize_t serial_show(struct device *dev, struct device_attribute *attr,
 			 char *buf)
 {
diff --git a/include/linux/firmware/meson/meson_sm.h b/include/linux/firmware/meson/meson_sm.h
index 8eaf8922ab02..f62acd2bf52a 100644
--- a/include/linux/firmware/meson/meson_sm.h
+++ b/include/linux/firmware/meson/meson_sm.h
@@ -7,6 +7,10 @@
 #ifndef _MESON_SM_FW_H_
 #define _MESON_SM_FW_H_
 
+#define SM_CHIP_ID_LENGTH	119
+#define SM_CHIP_ID_OFFSET	4
+#define SM_CHIP_ID_SIZE		12
+
 enum {
 	SM_EFUSE_READ,
 	SM_EFUSE_WRITE,
-- 
2.34.1

Re: [PATCH 3/5] firmware: meson_sm: move common definitions to header file

[Evgeny Bachinin]

On Wed, Nov 22, 2023 at 03:56:41PM +0300, Viacheslav Bocharov wrote:
> Move SM_CHIPID_* constants from firmware meson sm driver to
> header file.
> 
> Signed-off-by: Viacheslav Bocharov <adeep@lexina.in>
> ---
>  drivers/firmware/meson/meson_sm.c       | 4 ----
>  include/linux/firmware/meson/meson_sm.h | 4 ++++
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/firmware/meson/meson_sm.c b/drivers/firmware/meson/meson_sm.c
> index 402851ed4ac0..96e50811336f 100644
> --- a/drivers/firmware/meson/meson_sm.c
> +++ b/drivers/firmware/meson/meson_sm.c
> @@ -240,10 +240,6 @@ struct meson_sm_firmware *meson_sm_get(struct device_node *sm_node)
>  }
>  EXPORT_SYMBOL_GPL(meson_sm_get);
>  
> -#define SM_CHIP_ID_LENGTH	119
> -#define SM_CHIP_ID_OFFSET	4
> -#define SM_CHIP_ID_SIZE		12
> -
>  static ssize_t serial_show(struct device *dev, struct device_attribute *attr,
>  			 char *buf)
>  {
> diff --git a/include/linux/firmware/meson/meson_sm.h b/include/linux/firmware/meson/meson_sm.h
> index 8eaf8922ab02..f62acd2bf52a 100644
> --- a/include/linux/firmware/meson/meson_sm.h
> +++ b/include/linux/firmware/meson/meson_sm.h
> @@ -7,6 +7,10 @@
>  #ifndef _MESON_SM_FW_H_
>  #define _MESON_SM_FW_H_
>  
> +#define SM_CHIP_ID_LENGTH	119

Does anybody know why this value is 119 bytes?
if in-shmem data, arrived from BL31, contains only up to 20 bytes
(in case of chipID v2):
+-----------+---------+---------------------------------+
| chipID ver|  cpu_id |        12b-serial               |
|  4 bytes  | 4 bytes |(per particular die unique data) |
+-----------+---------+---------------------------------+

> +#define SM_CHIP_ID_OFFSET	4
> +#define SM_CHIP_ID_SIZE		12

It seems that either the value (12) is incorrect or the current naming
is misleading. This is because the chipID is 16 bytes. Of course,
likely the SoC serial was meant here...

Furthermore, it appears that SM_CHIP_ID_SIZE is an unused symbol. It
has been unused since its creation in
0789724f86a5 ('firmware: meson_sm: Add serial number sysfs entry')

> +
>  enum {
>  	SM_EFUSE_READ,
>  	SM_EFUSE_WRITE,
> -- 
> 2.34.1
> 
> 

-- 
Best Regards,
Evgeny Bachinin

[PATCH 4/5] soc: amlogic: 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 +
 drivers/soc/amlogic/meson-gx-socinfo-sm.c | 178 ++++++++++++++++++++++
 3 files changed, 189 insertions(+)
 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..5634ecb60478 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 && MESON_GX_SOCINFO && MESON_SM) || COMPILE_TEST
+	default MESON_GX_SOCINFO && MESON_SM
+	select SOC_BUS
+	help
+	  Say yes to support decoding of Amlogic Meson GX SoC family
+	  information about the type, package and version from 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-sm.c b/drivers/soc/amlogic/meson-gx-socinfo-sm.c
new file mode 100644
index 000000000000..52bf3bce09e2
--- /dev/null
+++ b/drivers/soc/amlogic/meson-gx-socinfo-sm.c
@@ -0,0 +1,178 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2023 JetHome
+ * 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/bitfield.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_cpuid(struct device *dev, struct meson_sm_firmware *fw,
+			       unsigned int *socinfo)
+{
+	char *buf;
+	uint8_t *id_buf;
+	int chip_id_version;
+	int ret;
+
+	id_buf = devm_kzalloc(dev, SM_CHIP_ID_LENGTH, GFP_KERNEL);
+	if (!id_buf)
+		return NULL;
+
+	ret = meson_sm_call_read(fw, id_buf, SM_CHIP_ID_LENGTH, SM_GET_CHIP_ID,
+				 2, 0, 0, 0, 0);
+	if (ret < 0) {
+		kfree(id_buf);
+		return NULL;
+	}
+
+	chip_id_version = *((unsigned int *)id_buf);
+
+	if (chip_id_version != 2) {
+		uint8_t tmp;
+		/**
+		 * Legacy 12-byte chip ID read out, transform data
+		 * to expected order format
+		 */
+
+		memmove(&id_buf[SM_CHIP_ID_OFFSET + 4], &id_buf[SM_CHIP_ID_OFFSET], 12);
+		for (int i = 0; i < 6; i++) {
+			tmp = id_buf[i + SM_CHIP_ID_OFFSET + 4];
+			id_buf[i + SM_CHIP_ID_OFFSET + 4] = id_buf[15 - i + SM_CHIP_ID_OFFSET];
+			id_buf[15 - i + SM_CHIP_ID_OFFSET] = tmp;
+		}
+		*(uint32_t *)(id_buf + SM_CHIP_ID_OFFSET) =
+					((*socinfo & 0xff000000)	|	// Family ID
+					((*socinfo << 8) & 0xff0000)	|	// Chip Revision
+					((*socinfo >> 8) & 0xff00))	|	// Package ID
+					((*socinfo) & 0xff);			// Misc
+	} else {
+		*socinfo = id_buf[SM_CHIP_ID_OFFSET] << 24 |	// Family ID
+		   id_buf[SM_CHIP_ID_OFFSET + 2] << 16 |	// Chip revision
+		   id_buf[SM_CHIP_ID_OFFSET + 1] << 8 |		// Package ID
+		   id_buf[SM_CHIP_ID_OFFSET + 3];		// Misc
+	}
+
+	buf = kasprintf(GFP_KERNEL, "%16phN\n", &id_buf[SM_CHIP_ID_OFFSET]);
+	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;
+	unsigned int 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)) {
+		dev_err(&pdev->dev, "failed to get regmap\n");
+		return -ENODEV;
+	}
+
+	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 -ENODEV;
+	}
+
+	fw = meson_sm_get(sm_np);
+	of_node_put(sm_np);
+	if (!fw)
+		return -EPROBE_DEFER;
+
+	dev_err(&pdev->dev, "secure-monitor node found\n");
+
+	ret = regmap_read(regmap, AO_SEC_SOCINFO_OFFSET, &socinfo);
+	if (ret < 0)
+		return ret;
+
+	if (!socinfo) {
+		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_cpuid(&pdev->dev, fw, &socinfo);
+
+	meson_gx_socinfo_prepare_soc_driver_attr(soc_dev_attr, 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);
+		kfree(soc_dev_attr);
+		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 at SM driver %x\n",
+			soc_dev_attr->soc_id,
+			socinfo_to_major(socinfo),
+			socinfo_to_minor(socinfo),
+			socinfo_to_pack(socinfo),
+			socinfo_to_misc(socinfo), socinfo);
+
+	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.34.1

Re: [PATCH 4/5] soc: amlogic: Add Amlogic secure-monitor SoC Information driver

[kernel test robot]

Hi Viacheslav,

kernel test robot noticed the following build warnings:

[auto build test WARNING on rockchip/for-next]
[also build test WARNING on linus/master v6.7-rc2 next-20231124]
[cannot apply to arm/for-next kvmarm/next arm/fixes]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Viacheslav-Bocharov/soc-amlogic-meson-gx-socinfo-move-common-code-to-header-file/20231122-232150
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git for-next
patch link:    https://lore.kernel.org/r/20231122125643.1717160-5-adeep%40lexina.in
patch subject: [PATCH 4/5] soc: amlogic: Add Amlogic secure-monitor SoC Information driver
config: arc-allmodconfig (https://download.01.org/0day-ci/archive/20231124/202311242104.RjBPI3uI-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231124/202311242104.RjBPI3uI-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311242104.RjBPI3uI-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/soc/amlogic/meson-gx-socinfo-sm.c:21:
>> drivers/soc/amlogic/meson-gx-socinfo-internal.h:48:3: warning: 'soc_packages' defined but not used [-Wunused-const-variable=]
      48 | } soc_packages[] = {
         |   ^~~~~~~~~~~~
>> drivers/soc/amlogic/meson-gx-socinfo-internal.h:26:3: warning: 'soc_ids' defined but not used [-Wunused-const-variable=]
      26 | } soc_ids[] = {
         |   ^~~~~~~


vim +/soc_packages +48 drivers/soc/amlogic/meson-gx-socinfo-internal.h

2fed8e24da3985 Viacheslav Bocharov 2023-11-22  22  
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  23  static const struct meson_gx_soc_id {
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  24  	const char *name;
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  25  	unsigned int id;
2fed8e24da3985 Viacheslav Bocharov 2023-11-22 @26  } soc_ids[] = {
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  27  	{ "GXBB", 0x1f },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  28  	{ "GXTVBB", 0x20 },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  29  	{ "GXL", 0x21 },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  30  	{ "GXM", 0x22 },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  31  	{ "TXL", 0x23 },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  32  	{ "TXLX", 0x24 },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  33  	{ "AXG", 0x25 },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  34  	{ "GXLX", 0x26 },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  35  	{ "TXHD", 0x27 },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  36  	{ "G12A", 0x28 },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  37  	{ "G12B", 0x29 },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  38  	{ "SM1", 0x2b },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  39  	{ "A1", 0x2c },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  40  };
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  41  
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  42  
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  43  static const struct meson_gx_package_id {
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  44  	const char *name;
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  45  	unsigned int major_id;
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  46  	unsigned int pack_id;
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  47  	unsigned int pack_mask;
2fed8e24da3985 Viacheslav Bocharov 2023-11-22 @48  } soc_packages[] = {
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  49  	{ "S905", 0x1f, 0, 0x20 }, /* pack_id != 0x20 */
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  50  	{ "S905H", 0x1f, 0x3, 0xf }, /* pack_id & 0xf == 0x3 */
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  51  	{ "S905M", 0x1f, 0x20, 0xf0 }, /* pack_id == 0x20 */
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  52  	{ "S905D", 0x21, 0, 0xf0 },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  53  	{ "S905X", 0x21, 0x80, 0xf0 },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  54  	{ "S905W", 0x21, 0xa0, 0xf0 },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  55  	{ "S905L", 0x21, 0xc0, 0xf0 },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  56  	{ "S905M2", 0x21, 0xe0, 0xf0 },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  57  	{ "S805X", 0x21, 0x30, 0xf0 },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  58  	{ "S805Y", 0x21, 0xb0, 0xf0 },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  59  	{ "S912", 0x22, 0, 0x0 }, /* Only S912 is known for GXM */
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  60  	{ "962X", 0x24, 0x10, 0xf0 },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  61  	{ "962E", 0x24, 0x20, 0xf0 },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  62  	{ "A113X", 0x25, 0x37, 0xff },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  63  	{ "A113X", 0x25, 0x43, 0xff },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  64  	{ "A113D", 0x25, 0x22, 0xff },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  65  	{ "S905D2", 0x28, 0x10, 0xf0 },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  66  	{ "S905Y2", 0x28, 0x30, 0xf0 },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  67  	{ "S905X2", 0x28, 0x40, 0xf0 },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  68  	{ "A311D", 0x29, 0x10, 0xf0 },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  69  	{ "S922X", 0x29, 0x40, 0xf0 },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  70  	{ "S905D3", 0x2b, 0x4, 0xf5 },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  71  	{ "S905X3", 0x2b, 0x5, 0xf5 },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  72  	{ "S905X3", 0x2b, 0x10, 0x3f },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  73  	{ "S905D3", 0x2b, 0x30, 0x3f },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  74  	{ "A113L", 0x2c, 0x0, 0xf8 },
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  75  };
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  76  
2fed8e24da3985 Viacheslav Bocharov 2023-11-22  77  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 4/5] soc: amlogic: Add Amlogic secure-monitor SoC Information driver

[Evgeny Bachinin]

please, see notes below

On Wed, Nov 22, 2023 at 03:56:42PM +0300, Viacheslav Bocharov wrote:
> 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


s/patchs/patch/


> 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 +
>  drivers/soc/amlogic/meson-gx-socinfo-sm.c | 178 ++++++++++++++++++++++
>  3 files changed, 189 insertions(+)
>  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..5634ecb60478 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 && MESON_GX_SOCINFO && MESON_SM) || COMPILE_TEST
> +	default MESON_GX_SOCINFO && MESON_SM
> +	select SOC_BUS
> +	help
> +	  Say yes to support decoding of Amlogic Meson GX SoC family
> +	  information about the type, package and version from 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-sm.c b/drivers/soc/amlogic/meson-gx-socinfo-sm.c
> new file mode 100644
> index 000000000000..52bf3bce09e2
> --- /dev/null
> +++ b/drivers/soc/amlogic/meson-gx-socinfo-sm.c
> @@ -0,0 +1,178 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2023 JetHome
> + * 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/bitfield.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_cpuid(struct device *dev, struct meson_sm_firmware *fw,
> +			       unsigned int *socinfo)
> +{
> +	char *buf;
> +	uint8_t *id_buf;
> +	int chip_id_version;
> +	int ret;
> +
> +	id_buf = devm_kzalloc(dev, SM_CHIP_ID_LENGTH, GFP_KERNEL);
> +	if (!id_buf)
> +		return NULL;
> +
> +	ret = meson_sm_call_read(fw, id_buf, SM_CHIP_ID_LENGTH, SM_GET_CHIP_ID,
> +				 2, 0, 0, 0, 0);
> +	if (ret < 0) {
> +		kfree(id_buf);
> +		return NULL;
> +	}
> +
> +	chip_id_version = *((unsigned int *)id_buf);
> +
> +	if (chip_id_version != 2) {
> +		uint8_t tmp;

minor:
The scope of the variable 'tmp' can be reduced.

Up to you, guys. I just highlighted here.

> +		/**
> +		 * Legacy 12-byte chip ID read out, transform data

The Amlogic chipID v1 and v2 are both 16 bytes long. Probably,
the "serial" was intended here under "12 byte". However, since we are
dealing with chipID in this function, wouldn't it be better to just
remove "12-byte"?"


> +		 * to expected order format
> +		 */
> +
> +		memmove(&id_buf[SM_CHIP_ID_OFFSET + 4], &id_buf[SM_CHIP_ID_OFFSET], 12);
> +		for (int i = 0; i < 6; i++) {
> +			tmp = id_buf[i + SM_CHIP_ID_OFFSET + 4];
> +			id_buf[i + SM_CHIP_ID_OFFSET + 4] = id_buf[15 - i + SM_CHIP_ID_OFFSET];
> +			id_buf[15 - i + SM_CHIP_ID_OFFSET] = tmp;
> +		}
> +		*(uint32_t *)(id_buf + SM_CHIP_ID_OFFSET) =
> +					((*socinfo & 0xff000000)	|	// Family ID
> +					((*socinfo << 8) & 0xff0000)	|	// Chip Revision
> +					((*socinfo >> 8) & 0xff00))	|	// Package ID
> +					((*socinfo) & 0xff);			// Misc
> +	} else {
> +		*socinfo = id_buf[SM_CHIP_ID_OFFSET] << 24 |	// Family ID
> +		   id_buf[SM_CHIP_ID_OFFSET + 2] << 16 |	// Chip revision
> +		   id_buf[SM_CHIP_ID_OFFSET + 1] << 8 |		// Package ID
> +		   id_buf[SM_CHIP_ID_OFFSET + 3];		// Misc
> +	}
> +
> +	buf = kasprintf(GFP_KERNEL, "%16phN\n", &id_buf[SM_CHIP_ID_OFFSET]);
> +	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;
> +	unsigned int socinfo;
> +	struct device *dev;
> +	int ret;
> +
> +	/* check if chip-id is available */

My apologies for nitpicking, but looks like the term "has-chip-id" is
misleading, too.
AFAIU it does not reflect the presence of the chipID in a particular
Amlogic SoC. Instead, it specifies the driver's ability to read the
cpu_id value from the register (via regmap). Therefore, I think,
it would be more accurate to call it as "has-cpu-id", although it seems
"has-chip-id" term is a legacy now.

> +	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)) {
> +		dev_err(&pdev->dev, "failed to get regmap\n");
> +		return -ENODEV;
> +	}
> +
> +	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 -ENODEV;
> +	}
> +
> +	fw = meson_sm_get(sm_np);
> +	of_node_put(sm_np);
> +	if (!fw)
> +		return -EPROBE_DEFER;
> +
> +	dev_err(&pdev->dev, "secure-monitor node found\n");

Debug leftover?
Strange to see an error in the non-error path. I mean is it proper
log level?

> +
> +	ret = regmap_read(regmap, AO_SEC_SOCINFO_OFFSET, &socinfo);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (!socinfo) {
> +		dev_err(&pdev->dev, "invalid regmap chipid value\n");

s/chipid/cpuid/ ?
because value read from register is actually 4 byte cpuid

> +		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_cpuid(&pdev->dev, fw, &socinfo);

Several notes.

1) Could you please clarify, why don't you pass socinfo by value?

What's the necessity to overwrite inside socinfo_get_cpuid() the
socinfo value read above via regmap?

2) Seems, again names' collision.
Actually, the function returns the chipid as a retval (16 bytes,
consisting of cpu_id + SoC serial), but the function is named as
socinfo_get_cpuid(). The reason for this could be that the distinct
function carries out two actions (returning socinfo and chipid) instead
of just one specific action.

All in all, what do you think, could the function be renamed as
s/socinfo_get_cpuid/socinfo_get_chipid/ ?

> +
> +	meson_gx_socinfo_prepare_soc_driver_attr(soc_dev_attr, 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);
> +		kfree(soc_dev_attr);
> +		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 at SM driver %x\n",
> +			soc_dev_attr->soc_id,
> +			socinfo_to_major(socinfo),
> +			socinfo_to_minor(socinfo),
> +			socinfo_to_pack(socinfo),
> +			socinfo_to_misc(socinfo), socinfo);
> +
> +	return PTR_ERR_OR_ZERO(dev);
> +}
> +
> +

is extra line supposed?

> +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,
> +	},
> +};
> +
> +

extra line?

> +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.34.1
> 
> 

-- 
Best Regards,
Evgeny Bachinin

[PATCH 5/5] 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.

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 648e7f49424f..449b328d62b1 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 a49aa62e3f9f..6e80bdc525e5 100644
--- a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
@@ -1660,6 +1660,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 ff68b911b729..0a6b703b0dc0 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
@@ -2026,6 +2026,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.34.1

Re: [DMARC error][DKIM error] [PATCH 5/5] arm64: dts: meson: add dts links to secure-monitor for soc driver in a1, axg, gx, g12

[Dmitry Rokosov]

Hello Viacheslav,

On Wed, Nov 22, 2023 at 03:56:43PM +0300, Viacheslav Bocharov wrote:
> Add links to secure-monitor in soc driver section for A1, AXG, GX, G12
> Amlogic family.
> 
> 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 648e7f49424f..449b328d62b1 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>;

I suppose it's better and more consistent to use the same hierarchy
pattern as Secure PWRC uses: to be a child of Secure Monitor.

Please see the example below from meson-a1.dtsi:


	sm: secure-monitor {
		compatible = "amlogic,meson-gxbb-sm";

		pwrc: power-controller {
			compatible = "amlogic,meson-a1-pwrc";
			#power-domain-cells = <1>;
		};
	};

>  				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 a49aa62e3f9f..6e80bdc525e5 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
> @@ -1660,6 +1660,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 ff68b911b729..0a6b703b0dc0 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
> @@ -2026,6 +2026,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.34.1
> 
> 
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic

-- 
Thank you,
Dmitry

Re: [DMARC error][DKIM error] [PATCH 0/5] soc: amlogic: add new meson-gx-socinfo-sm driver

[Dmitry Rokosov]

Hello Neil,

May I put in my two cents on this patch series?

There appears to be a misunderstanding regarding the terminology used.
Allow me to clarify my perspective.

The original Amlogic chipid has the following format:

    4 bytes      12 bytes
    +-------+-------------------+
    |       |                   |
    | CPUID | SOC SERIAL NUMBER |
    |       |                   |
    +-------+-------------------+
    0                          15


In the current uboot [1] and kernel [2] upstream, only the SOC SERIAL
NUMBER bytes are read from efuse OTP storage (and it isn't swapped, as
Amlogic reference code does [3]). We refer to these bytes as "serial".

The original chipid value is utilized in several algorithms (for
example, in the Amlogic boot protocols), making it crucial to have the
ability to read the original chipid value as intended by the vendor.

In my opinion, we should align our naming terminology as follows:
    - "chipid" - Represents the complete Amlogic SoC ID, includes
                 "cpuid" and "serial"
    - "serial" - 12 byte unique SoC number, identifying particular die

We strongly believe that this patch series is essential and are highly interested in seeing it applied to the upstream linux-amlogic, for the following reasons:
    - We use chipid for device identification in our boards
    - The Amlogic boot protocols utilize the full version of chipid
      (ADNL, Optimus). As I mentioned in the IRC, we are preparing a
      patch series for uboot incorporating them.
    - in OPTEE: for generation of SSK (Secure Storage Key) [4]
    - RPMB: for generation of RPMB key, further provisioned into RPMB
      controller (thus particular SoC is bound to particular eMMC

Therefore, I propose that we rename "soc_id" in the Viacheslav patch
series to "chipid" and subsequently port this patch series to uboot.

What are your thoughts on this matter?

Links:
[1] - https://elixir.bootlin.com/u-boot/v2024.01/source/arch/arm/mach-meson/sm.c#L84
[2] - https://elixir.bootlin.com/linux/v6.7.4/source/drivers/firmware/meson/meson_sm.c#L268
[3] - https://github.com/CoreELEC/u-boot/blob/3efc85a8370796bcec3bcadcdecec9aed973f4a9/arch/arm/mach-meson/g12a/bl31_apis.c#L398-L417
[4] - https://github.com/OP-TEE/optee_os/blob/5df2a985b2ffd0b6f1107f12ca2a88203bf31328/core/tee/tee_fs_key_manager.c#L152

On Wed, Nov 22, 2023 at 03:56:38PM +0300, Viacheslav Bocharov wrote:
> 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 the second 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.
> - transfer the chipid sm call constants to a header file (perhaps they
> need renaming?).
> - add secure-monitor references for amlogic,meson-gx-ao-secure sections
> in dts files of the a1, axg, g12, gx families.
> 
> Viacheslav Bocharov (5):
>   soc: amlogic: meson-gx-socinfo: move common code to header file
>   soc: amlogic: meson-gx-socinfo: move common code to exported function
>   firmware: meson_sm: move common definitions to header file
>   soc: amlogic: Add Amlogic secure-monitor SoC Information driver
>   arm64: dts: meson: add dts links to secure-monitor for soc driver in
>     a1, axg, gx, g12
> 
>  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/firmware/meson/meson_sm.c             |   4 -
>  drivers/soc/amlogic/Kconfig                   |  10 +
>  drivers/soc/amlogic/Makefile                  |   1 +
>  .../soc/amlogic/meson-gx-socinfo-internal.h   | 102 ++++++++++
>  drivers/soc/amlogic/meson-gx-socinfo-sm.c     | 178 ++++++++++++++++++
>  drivers/soc/amlogic/meson-gx-socinfo.c        | 106 ++---------
>  include/linux/firmware/meson/meson_sm.h       |   4 +
>  11 files changed, 317 insertions(+), 92 deletions(-)
>  create mode 100644 drivers/soc/amlogic/meson-gx-socinfo-internal.h
>  create mode 100644 drivers/soc/amlogic/meson-gx-socinfo-sm.c
> 
> -- 
> 2.34.1
> 
> 
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic

-- 
Thank you,
Dmitry

Re: [DMARC error][DKIM error] [PATCH 0/5] soc: amlogic: add new meson-gx-socinfo-sm driver

[Neil Armstrong]

On 16/02/2024 08:47, Dmitry Rokosov wrote:
> Hello Neil,
> 
> May I put in my two cents on this patch series?
> 
> There appears to be a misunderstanding regarding the terminology used.
> Allow me to clarify my perspective.
> 
> The original Amlogic chipid has the following format:
> 
>      4 bytes      12 bytes
>      +-------+-------------------+
>      |       |                   |
>      | CPUID | SOC SERIAL NUMBER |
>      |       |                   |
>      +-------+-------------------+
>      0                          15
> 
> 
> In the current uboot [1] and kernel [2] upstream, only the SOC SERIAL
> NUMBER bytes are read from efuse OTP storage (and it isn't swapped, as
> Amlogic reference code does [3]). We refer to these bytes as "serial".
> 
> The original chipid value is utilized in several algorithms (for
> example, in the Amlogic boot protocols), making it crucial to have the
> ability to read the original chipid value as intended by the vendor.
> 
> In my opinion, we should align our naming terminology as follows:
>      - "chipid" - Represents the complete Amlogic SoC ID, includes
>                   "cpuid" and "serial"
>      - "serial" - 12 byte unique SoC number, identifying particular die
> 
> We strongly believe that this patch series is essential and are highly interested in seeing it applied to the upstream linux-amlogic, for the following reasons:
>      - We use chipid for device identification in our boards
>      - The Amlogic boot protocols utilize the full version of chipid
>        (ADNL, Optimus). As I mentioned in the IRC, we are preparing a
>        patch series for uboot incorporating them.
>      - in OPTEE: for generation of SSK (Secure Storage Key) [4]
>      - RPMB: for generation of RPMB key, further provisioned into RPMB
>        controller (thus particular SoC is bound to particular eMMC
> 
> Therefore, I propose that we rename "soc_id" in the Viacheslav patch
> series to "chipid" and subsequently port this patch series to uboot.
> 
> What are your thoughts on this matter?

I'm perfectly fine with that, but I don't like the shared functions, the only
shared stuff are the soc id tables, the shared functions is not important enough
to be shared.

Neil

> 
> Links:
> [1] - https://elixir.bootlin.com/u-boot/v2024.01/source/arch/arm/mach-meson/sm.c#L84
> [2] - https://elixir.bootlin.com/linux/v6.7.4/source/drivers/firmware/meson/meson_sm.c#L268
> [3] - https://github.com/CoreELEC/u-boot/blob/3efc85a8370796bcec3bcadcdecec9aed973f4a9/arch/arm/mach-meson/g12a/bl31_apis.c#L398-L417
> [4] - https://github.com/OP-TEE/optee_os/blob/5df2a985b2ffd0b6f1107f12ca2a88203bf31328/core/tee/tee_fs_key_manager.c#L152
> 
> On Wed, Nov 22, 2023 at 03:56:38PM +0300, Viacheslav Bocharov wrote:
>> 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 the second 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.
>> - transfer the chipid sm call constants to a header file (perhaps they
>> need renaming?).
>> - add secure-monitor references for amlogic,meson-gx-ao-secure sections
>> in dts files of the a1, axg, g12, gx families.
>>
>> Viacheslav Bocharov (5):
>>    soc: amlogic: meson-gx-socinfo: move common code to header file
>>    soc: amlogic: meson-gx-socinfo: move common code to exported function
>>    firmware: meson_sm: move common definitions to header file
>>    soc: amlogic: Add Amlogic secure-monitor SoC Information driver
>>    arm64: dts: meson: add dts links to secure-monitor for soc driver in
>>      a1, axg, gx, g12
>>
>>   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/firmware/meson/meson_sm.c             |   4 -
>>   drivers/soc/amlogic/Kconfig                   |  10 +
>>   drivers/soc/amlogic/Makefile                  |   1 +
>>   .../soc/amlogic/meson-gx-socinfo-internal.h   | 102 ++++++++++
>>   drivers/soc/amlogic/meson-gx-socinfo-sm.c     | 178 ++++++++++++++++++
>>   drivers/soc/amlogic/meson-gx-socinfo.c        | 106 ++---------
>>   include/linux/firmware/meson/meson_sm.h       |   4 +
>>   11 files changed, 317 insertions(+), 92 deletions(-)
>>   create mode 100644 drivers/soc/amlogic/meson-gx-socinfo-internal.h
>>   create mode 100644 drivers/soc/amlogic/meson-gx-socinfo-sm.c
>>
>> -- 
>> 2.34.1
>>
>>
>> _______________________________________________
>> linux-amlogic mailing list
>> linux-amlogic@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-amlogic
>