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 <[email protected]>
---
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 <[email protected]>
+ *
+ */
+
+#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 <[email protected]>");
+MODULE_DESCRIPTION("Amlogic Meson GX SOC SM driver");
+MODULE_LICENSE("GPL");
--
2.34.1
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/[email protected]/config)
compiler: arceb-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231124/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
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 <[email protected]>
> ---
> 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 <[email protected]>
> + *
> + */
> +
> +#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 <[email protected]>");
> +MODULE_DESCRIPTION("Amlogic Meson GX SOC SM driver");
> +MODULE_LICENSE("GPL");
> --
> 2.34.1
>
>
--
Best Regards,
Evgeny Bachinin