All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 2/4] firmware: Samsung: Add secure monitor driver
       [not found] <CGME20220713045516epcms1p86b3f6a8795d767faac65eb947405f911@epcms1p8>
@ 2022-07-13  4:55 ` Dongjin Yang
  2022-07-13  4:58   ` Randy Dunlap
                     ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Dongjin Yang @ 2022-07-13  4:55 UTC (permalink / raw)
  To: jesper.nilsson, lars.persson, robh+dt, krzk+dt, lee.jones
  Cc: javierm, bjorn.andersson, linux-arm-kernel, devicetree,
	linux-kernel, Moon-Ki Jun, Sang Min Kim, Wangseok Lee,
	Dongjin Yang

Introduce a driver to provide calls into secure monitor mode.
This driver is used for SoCs produced by Samsung Foundry to provide
SMC call. This patch supports register read/write request to secure
monitor. Also, SMC call request which uses shared memory to exchange
the data between kernel and secure monitor.

Signed-off-by: Dongjin Yang <dj76.yang@samsung.com>
---
 MAINTAINERS                              |   2 +
 drivers/firmware/Kconfig                 |  11 +++
 drivers/firmware/Makefile                |   1 +
 drivers/firmware/samsung-smc-svc.c       | 154 +++++++++++++++++++++++++++++++
 include/linux/firmware/samsung-smc-svc.h |  59 ++++++++++++
 5 files changed, 227 insertions(+)
 create mode 100644 drivers/firmware/samsung-smc-svc.c
 create mode 100644 include/linux/firmware/samsung-smc-svc.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 6763746c349f..d173043ffb46 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1868,8 +1868,10 @@ F:	arch/arm/boot/dts/artpec6*
 F:	arch/arm/mach-artpec
 F:	drivers/clk/axis
 F:	drivers/crypto/axis
+F:	drivers/firmware/samsung-smc-svc.c
 F:	drivers/mmc/host/usdhi6rol0.c
 F:	drivers/pinctrl/pinctrl-artpec*
+F:	include/linux/firmware/samsung-smc-svc.h
 
 ARM/ASPEED I2C DRIVER
 M:	Brendan Higgins <brendanhiggins@google.com>
diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index e5cfb01353d8..4b0f2d033f58 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -217,6 +217,17 @@ config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
 
 	  Say Y here to enable "download mode" by default.
 
+config SAMSUNG_SECURE_SERVICE
+	bool "Samsung Foundry Secure Service Layer"
+	depends on HAVE_ARM_SMCCC
+	default n
+	help
+	  Support secure service layer for SoCs which is manufactured by
+	  Samsung Foundry.
+
+	  This option provide support of secure monitor service call using
+	  Trusted Foundations.
+
 config SYSFB
 	bool
 	default y
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 4e58cb474a68..985e30a9665f 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_RASPBERRYPI_FIRMWARE) += raspberrypi.o
 obj-$(CONFIG_FW_CFG_SYSFS)	+= qemu_fw_cfg.o
 obj-$(CONFIG_QCOM_SCM)		+= qcom-scm.o
 qcom-scm-objs += qcom_scm.o qcom_scm-smc.o qcom_scm-legacy.o
+obj-$(CONFIG_SAMSUNG_SECURE_SERVICE) += samsung-smc-svc.o
 obj-$(CONFIG_SYSFB)		+= sysfb.o
 obj-$(CONFIG_SYSFB_SIMPLEFB)	+= sysfb_simplefb.o
 obj-$(CONFIG_TI_SCI_PROTOCOL)	+= ti_sci.o
diff --git a/drivers/firmware/samsung-smc-svc.c b/drivers/firmware/samsung-smc-svc.c
new file mode 100644
index 000000000000..eb3a5285cf2b
--- /dev/null
+++ b/drivers/firmware/samsung-smc-svc.c
@@ -0,0 +1,154 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019 Samsung Electronics Co., Ltd.
+ * Author: Kihyun Yoon<kihyun.yoon@samsung.com>
+ * Author: Dongjin Yang<dj76.yang@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/types.h>
+#include <linux/errno.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/firmware/samsung-smc-svc.h>
+
+struct samsung_smc_version {
+	u32 major;
+	u32 minor;
+};
+
+struct samsung_smc_data {
+	struct samsung_smc_version version;
+	u32 svc_cmd_list[SAMSUNG_SIP_NR_SIP];
+};
+
+struct samsung_smc_data *svc_data;
+
+static int samsung_smc_svc_init(void);
+
+static int __samsung_get_svc_cmd(u32 cmd_idx)
+{
+	if (cmd_idx >= SAMSUNG_SIP_NR_SIP)
+		return SAMSUNG_SIP_NOT_SUPPORTED;
+
+	/* Initialize this driver if it is not ready */
+	if (!svc_data && samsung_smc_svc_init()) {
+		pr_err("samsung,smccc_service initialization is failed\n");
+		return SAMSUNG_SIP_NOT_SUPPORTED;
+	}
+
+	return svc_data->svc_cmd_list[cmd_idx];
+}
+
+int samsung_smc_call(u32 cmd_idx,
+		     unsigned long a0, unsigned long a1,
+		     unsigned long a2, unsigned long a3,
+		     u32 *val)
+
+{
+	struct arm_smccc_res res;
+	u32 cmd = __samsung_get_svc_cmd(cmd_idx);
+
+	if (cmd == SAMSUNG_SIP_NOT_SUPPORTED)
+		return -EOPNOTSUPP;
+
+	arm_smccc_smc(cmd, a0, a1, a2, a3, 0, 0, 0, &res);
+	if (res.a0 == SMCCC_RET_NOT_SUPPORTED)
+		return -EINVAL;
+
+	if (val)
+		*val = (u32)res.a1;
+
+	return 0;
+}
+EXPORT_SYMBOL(samsung_smc_call);
+
+int samsung_smc_reg_read(void *base, u32 reg, u32 *val)
+{
+	return samsung_smc_call(SAMSUNG_SIP_READ_REG,
+				(unsigned long)base, reg, 0, 0, val);
+}
+EXPORT_SYMBOL(samsung_smc_reg_read);
+
+int samsung_smc_reg_write(void *base, u32 reg, u32 val)
+{
+	return samsung_smc_call(SAMSUNG_SIP_WRITE_REG,
+				  (unsigned long)base, reg, val, 0, NULL);
+}
+EXPORT_SYMBOL(samsung_smc_reg_write);
+
+static int samsung_smc_check_version(struct samsung_smc_version *ver)
+{
+	struct arm_smccc_res res;
+
+	arm_smccc_smc(SAMSUNG_SIP_SVC_VERSION, 0, 0, 0, 0, 0, 0, 0, &res);
+
+	if ((u32)res.a0 != ver->major || (u32)res.a1 != ver->minor)
+		return -EINVAL;
+
+	pr_info("Samsung SVC version [%d.%d] is detected\n",
+		ver->major, ver->minor);
+
+	return 0;
+}
+
+static const struct samsung_smc_data svc_0_1 = {
+	.version = {
+		.major = 0,
+		.minor = 1
+	},
+	.svc_cmd_list = {
+		[SAMSUNG_SIP_READ_REG] = 0x82000020,
+		[SAMSUNG_SIP_WRITE_REG] = 0x82000021,
+		/* TODO: SMC call for clock driver should be added */
+	}
+};
+
+static const struct of_device_id samsung_smc_of_match[] = {
+	{ .compatible = "samsung,smccc-svc", .data = &svc_0_1 },
+	{ /* sentinel */ }
+};
+
+static int samsung_smc_svc_init(void)
+{
+	struct device_node *fw_np;
+	struct device_node *np;
+	const struct of_device_id *match;
+	int ret;
+
+	if (svc_data) {
+		pr_info("samsung,smccc_service is already initialized\n");
+		return 0;
+	}
+
+	fw_np = of_find_node_by_name(NULL, "firmware");
+	if (!fw_np)
+		return -ENODEV;
+
+	np = of_find_matching_node_and_match(fw_np, samsung_smc_of_match,
+					     &match);
+	if (!np)
+		return -ENODEV;
+
+	svc_data = (struct samsung_smc_data *)match->data;
+	if (!svc_data)
+		return -ENODEV;
+
+	ret = samsung_smc_check_version(&svc_data->version);
+	if (ret) {
+		pr_err("samsung svc version is not matched\n");
+		return ret;
+	}
+
+	pr_info("samsung smc svc is initialized\n");
+	return 0;
+}
+
+early_initcall(samsung_smc_svc_init);
diff --git a/include/linux/firmware/samsung-smc-svc.h b/include/linux/firmware/samsung-smc-svc.h
new file mode 100644
index 000000000000..9c94fd3e10a2
--- /dev/null
+++ b/include/linux/firmware/samsung-smc-svc.h
@@ -0,0 +1,59 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __SAMSUNG_SMC_H
+#define __SAMSUNG_SMC_H
+
+#include <linux/arm-smccc.h>
+
+#ifdef CONFIG_SAMSUNG_SECURE_SERVICE
+
+#define SAMSUNG_SIP_CALL_VAL(x) \
+		ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+		ARM_SMCCC_SMC_32, ARM_SMCCC_OWNER_SIP, x)
+
+/* Common SIP SVC number */
+#define SAMSUING_SIP_SVC_CALL_COUNT	SAMSUNG_SIP_CALL_VAL(0xff00)
+#define SAMSUING_SIP_SVC_UID		SAMSUNG_SIP_CALL_VAL(0xff01)
+#define SAMSUNG_SIP_SVC_VERSION		SAMSUNG_SIP_CALL_VAL(0xff03)
+#define SAMSUNG_SIP_NOT_SUPPORTED	0x0
+
+enum {
+	/* SIP enumeration for V0.1 */
+	SAMSUNG_SIP_READ_REG,
+	SAMSUNG_SIP_WRITE_REG,
+	/* Add here for later version */
+	SAMSUNG_SIP_NR_SIP
+};
+
+int samsung_smc_call(u32 cmd_idx,
+		     unsigned long a0, unsigned long a1,
+		     unsigned long a2, unsigned long a3,
+		     u32 *val);
+int samsung_smc_reg_read(void *addr, unsigned int reg, u32 *val);
+int samsung_smc_reg_write(void *addr, unsigned int reg, u32 val);
+
+#else
+
+static inline int samsung_smc_call(u32 cmd_idx,
+				   unsigned long a0, unsigned long a1,
+				   unsigned long a2, unsigned long a3,
+				   u32 *val)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline int samsung_smc_reg_read(void *base,
+				       unsigned int reg, u32 *val)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline int samsung_smc_reg_write(void *base,
+					unsigned int reg, u32 val)
+{
+	return -EOPNOTSUPP;
+}
+
+#endif
+
+#endif /* __SAMSUNG_SMC_H */
-- 
2.9.5

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/4] firmware: Samsung: Add secure monitor driver
  2022-07-13  4:55 ` [PATCH 2/4] firmware: Samsung: Add secure monitor driver Dongjin Yang
@ 2022-07-13  4:58   ` Randy Dunlap
  2022-07-13  7:11   ` Krzysztof Kozlowski
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 6+ messages in thread
From: Randy Dunlap @ 2022-07-13  4:58 UTC (permalink / raw)
  To: dj76.yang, jesper.nilsson, lars.persson, robh+dt, krzk+dt, lee.jones
  Cc: javierm, bjorn.andersson, linux-arm-kernel, devicetree,
	linux-kernel, Moon-Ki Jun, Sang Min Kim, Wangseok Lee

Hi--

On 7/12/22 21:55, Dongjin Yang wrote:
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index e5cfb01353d8..4b0f2d033f58 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -217,6 +217,17 @@ config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
>  
>  	  Say Y here to enable "download mode" by default.
>  
> +config SAMSUNG_SECURE_SERVICE
> +	bool "Samsung Foundry Secure Service Layer"
> +	depends on HAVE_ARM_SMCCC
> +	default n

Drop that line, it's the default anyway.

> +	help
> +	  Support secure service layer for SoCs which is manufactured by

	                                        which are

> +	  Samsung Foundry.
> +
> +	  This option provide support of secure monitor service call using
> +	  Trusted Foundations.

-- 
~Randy

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/4] firmware: Samsung: Add secure monitor driver
  2022-07-13  4:55 ` [PATCH 2/4] firmware: Samsung: Add secure monitor driver Dongjin Yang
  2022-07-13  4:58   ` Randy Dunlap
@ 2022-07-13  7:11   ` Krzysztof Kozlowski
       [not found]   ` <CGME20220713045516epcms1p86b3f6a8795d767faac65eb947405f911@epcms1p4>
       [not found]   ` <CGME20220713045516epcms1p86b3f6a8795d767faac65eb947405f911@epcms1p6>
  3 siblings, 0 replies; 6+ messages in thread
From: Krzysztof Kozlowski @ 2022-07-13  7:11 UTC (permalink / raw)
  To: dj76.yang, jesper.nilsson, lars.persson, robh+dt, krzk+dt, lee.jones
  Cc: javierm, bjorn.andersson, linux-arm-kernel, devicetree,
	linux-kernel, Moon-Ki Jun, Sang Min Kim, Wangseok Lee

On 13/07/2022 06:55, Dongjin Yang wrote:
> Introduce a driver to provide calls into secure monitor mode.
> This driver is used for SoCs produced by Samsung Foundry to provide
> SMC call. This patch supports register read/write request to secure
> monitor. Also, SMC call request which uses shared memory to exchange
> the data between kernel and secure monitor.
> 
> Signed-off-by: Dongjin Yang <dj76.yang@samsung.com>
> ---
>  MAINTAINERS                              |   2 +
>  drivers/firmware/Kconfig                 |  11 +++
>  drivers/firmware/Makefile                |   1 +
>  drivers/firmware/samsung-smc-svc.c       | 154 +++++++++++++++++++++++++++++++
>  include/linux/firmware/samsung-smc-svc.h |  59 ++++++++++++
>  5 files changed, 227 insertions(+)
>  create mode 100644 drivers/firmware/samsung-smc-svc.c
>  create mode 100644 include/linux/firmware/samsung-smc-svc.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6763746c349f..d173043ffb46 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1868,8 +1868,10 @@ F:	arch/arm/boot/dts/artpec6*
>  F:	arch/arm/mach-artpec
>  F:	drivers/clk/axis
>  F:	drivers/crypto/axis
> +F:	drivers/firmware/samsung-smc-svc.c
>  F:	drivers/mmc/host/usdhi6rol0.c
>  F:	drivers/pinctrl/pinctrl-artpec*
> +F:	include/linux/firmware/samsung-smc-svc.h

Same as for bindings - not related to Artpec platforms. Either add
dedicated entry or put it under Samsung SoC.

>  
>  ARM/ASPEED I2C DRIVER
>  M:	Brendan Higgins <brendanhiggins@google.com>
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index e5cfb01353d8..4b0f2d033f58 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -217,6 +217,17 @@ config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
>  
>  	  Say Y here to enable "download mode" by default.
>  
> +config SAMSUNG_SECURE_SERVICE
> +	bool "Samsung Foundry Secure Service Layer"
> +	depends on HAVE_ARM_SMCCC
> +	default n
> +	help
> +	  Support secure service layer for SoCs which is manufactured by
> +	  Samsung Foundry.
> +
> +	  This option provide support of secure monitor service call using
> +	  Trusted Foundations.
> +
>  config SYSFB
>  	bool
>  	default y
> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
> index 4e58cb474a68..985e30a9665f 100644
> --- a/drivers/firmware/Makefile
> +++ b/drivers/firmware/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_RASPBERRYPI_FIRMWARE) += raspberrypi.o
>  obj-$(CONFIG_FW_CFG_SYSFS)	+= qemu_fw_cfg.o
>  obj-$(CONFIG_QCOM_SCM)		+= qcom-scm.o
>  qcom-scm-objs += qcom_scm.o qcom_scm-smc.o qcom_scm-legacy.o
> +obj-$(CONFIG_SAMSUNG_SECURE_SERVICE) += samsung-smc-svc.o
>  obj-$(CONFIG_SYSFB)		+= sysfb.o
>  obj-$(CONFIG_SYSFB_SIMPLEFB)	+= sysfb_simplefb.o
>  obj-$(CONFIG_TI_SCI_PROTOCOL)	+= ti_sci.o
> diff --git a/drivers/firmware/samsung-smc-svc.c b/drivers/firmware/samsung-smc-svc.c
> new file mode 100644
> index 000000000000..eb3a5285cf2b
> --- /dev/null
> +++ b/drivers/firmware/samsung-smc-svc.c
> @@ -0,0 +1,154 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019 Samsung Electronics Co., Ltd.
> + * Author: Kihyun Yoon<kihyun.yoon@samsung.com>
> + * Author: Dongjin Yang<dj76.yang@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *

Drop boiler plate with trailing empty '*' comment.

> + */
> +
> +#include <linux/types.h>
> +#include <linux/errno.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/firmware/samsung-smc-svc.h>

Order the headers by name.

> +
> +struct samsung_smc_version {
> +	u32 major;
> +	u32 minor;
> +};
> +
> +struct samsung_smc_data {
> +	struct samsung_smc_version version;
> +	u32 svc_cmd_list[SAMSUNG_SIP_NR_SIP];
> +};
> +
> +struct samsung_smc_data *svc_data;

No, no globals. Even no static variables.

> +
> +static int samsung_smc_svc_init(void);
> +
> +static int __samsung_get_svc_cmd(u32 cmd_idx)
> +{
> +	if (cmd_idx >= SAMSUNG_SIP_NR_SIP)
> +		return SAMSUNG_SIP_NOT_SUPPORTED;
> +
> +	/* Initialize this driver if it is not ready */
> +	if (!svc_data && samsung_smc_svc_init()) {
> +		pr_err("samsung,smccc_service initialization is failed\n");
> +		return SAMSUNG_SIP_NOT_SUPPORTED;
> +	}
> +
> +	return svc_data->svc_cmd_list[cmd_idx];
> +}
> +
> +int samsung_smc_call(u32 cmd_idx,
> +		     unsigned long a0, unsigned long a1,
> +		     unsigned long a2, unsigned long a3,
> +		     u32 *val)
> +
> +{
> +	struct arm_smccc_res res;
> +	u32 cmd = __samsung_get_svc_cmd(cmd_idx);
> +
> +	if (cmd == SAMSUNG_SIP_NOT_SUPPORTED)
> +		return -EOPNOTSUPP;
> +
> +	arm_smccc_smc(cmd, a0, a1, a2, a3, 0, 0, 0, &res);
> +	if (res.a0 == SMCCC_RET_NOT_SUPPORTED)
> +		return -EINVAL;
> +
> +	if (val)
> +		*val = (u32)res.a1;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(samsung_smc_call);

EXPORT_SYMBOL_GPL

You need kerneldoc for all these.

> +
> +int samsung_smc_reg_read(void *base, u32 reg, u32 *val)
> +{
> +	return samsung_smc_call(SAMSUNG_SIP_READ_REG,
> +				(unsigned long)base, reg, 0, 0, val);
> +}
> +EXPORT_SYMBOL(samsung_smc_reg_read);
> +
> +int samsung_smc_reg_write(void *base, u32 reg, u32 val)
> +{
> +	return samsung_smc_call(SAMSUNG_SIP_WRITE_REG,
> +				  (unsigned long)base, reg, val, 0, NULL);
> +}
> +EXPORT_SYMBOL(samsung_smc_reg_write);
> +
> +static int samsung_smc_check_version(struct samsung_smc_version *ver)
> +{
> +	struct arm_smccc_res res;
> +
> +	arm_smccc_smc(SAMSUNG_SIP_SVC_VERSION, 0, 0, 0, 0, 0, 0, 0, &res);
> +
> +	if ((u32)res.a0 != ver->major || (u32)res.a1 != ver->minor)
> +		return -EINVAL;
> +
> +	pr_info("Samsung SVC version [%d.%d] is detected\n",
> +		ver->major, ver->minor);
> +
> +	return 0;
> +}
> +
> +static const struct samsung_smc_data svc_0_1 = {
> +	.version = {
> +		.major = 0,
> +		.minor = 1
> +	},
> +	.svc_cmd_list = {
> +		[SAMSUNG_SIP_READ_REG] = 0x82000020,
> +		[SAMSUNG_SIP_WRITE_REG] = 0x82000021,
> +		/* TODO: SMC call for clock driver should be added */
> +	}
> +};
> +
> +static const struct of_device_id samsung_smc_of_match[] = {
> +	{ .compatible = "samsung,smccc-svc", .data = &svc_0_1 },
> +	{ /* sentinel */ }
> +};
> +
> +static int samsung_smc_svc_init(void)
> +{
> +	struct device_node *fw_np;
> +	struct device_node *np;
> +	const struct of_device_id *match;
> +	int ret;
> +
> +	if (svc_data) {
> +		pr_info("samsung,smccc_service is already initialized\n");
> +		return 0;
> +	}
> +
> +	fw_np = of_find_node_by_name(NULL, "firmware");
> +	if (!fw_np)
> +		return -ENODEV;
> +
> +	np = of_find_matching_node_and_match(fw_np, samsung_smc_of_match,
> +					     &match);
> +	if (!np)
> +		return -ENODEV;
> +
> +	svc_data = (struct samsung_smc_data *)match->data;
> +	if (!svc_data)
> +		return -ENODEV;
> +
> +	ret = samsung_smc_check_version(&svc_data->version);
> +	if (ret) {
> +		pr_err("samsung svc version is not matched\n");
> +		return ret;
> +	}
> +
> +	pr_info("samsung smc svc is initialized\n");

No useless success printks. We have tracing and sysfs for that.

> +	return 0;
> +}
> +
> +early_initcall(samsung_smc_svc_init);

No. Make it a platform driver.

> diff --git a/include/linux/firmware/samsung-smc-svc.h b/include/linux/firmware/samsung-smc-svc.h
> new file mode 100644
> index 000000000000..9c94fd3e10a2
> --- /dev/null
> +++ b/include/linux/firmware/samsung-smc-svc.h
> @@ -0,0 +1,59 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __SAMSUNG_SMC_H
> +#define __SAMSUNG_SMC_H

Extend the header gaurd to match path.

> +
> +#include <linux/arm-smccc.h>
> +
> +#ifdef CONFIG_SAMSUNG_SECURE_SERVICE

No ifdefs in headers. This is not a subsystem.

> +
> +#define SAMSUNG_SIP_CALL_VAL(x) \
> +		ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> +		ARM_SMCCC_SMC_32, ARM_SMCCC_OWNER_SIP, x)

Need kerneldoc.

> +
> +/* Common SIP SVC number */
> +#define SAMSUING_SIP_SVC_CALL_COUNT	SAMSUNG_SIP_CALL_VAL(0xff00)
> +#define SAMSUING_SIP_SVC_UID		SAMSUNG_SIP_CALL_VAL(0xff01)
> +#define SAMSUNG_SIP_SVC_VERSION		SAMSUNG_SIP_CALL_VAL(0xff03)
> +#define SAMSUNG_SIP_NOT_SUPPORTED	0x0

Need kerneldoc.


> +
> +enum {
> +	/* SIP enumeration for V0.1 */
> +	SAMSUNG_SIP_READ_REG,
> +	SAMSUNG_SIP_WRITE_REG,
> +	/* Add here for later version */
> +	SAMSUNG_SIP_NR_SIP
> +};

Need kerneldoc.


> +
> +int samsung_smc_call(u32 cmd_idx,
> +		     unsigned long a0, unsigned long a1,
> +		     unsigned long a2, unsigned long a3,
> +		     u32 *val);
> +int samsung_smc_reg_read(void *addr, unsigned int reg, u32 *val);
> +int samsung_smc_reg_write(void *addr, unsigned int reg, u32 val);
> +

Need kerneldoc in the driver.


> +#else
> +

No stubs, no ifdefs. It's not a subsystem.



Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/4] firmware: Samsung: Add secure monitor driver
       [not found]   ` <CGME20220713045516epcms1p86b3f6a8795d767faac65eb947405f911@epcms1p4>
@ 2022-07-15  2:02     ` Dongjin Yang
  0 siblings, 0 replies; 6+ messages in thread
From: Dongjin Yang @ 2022-07-15  2:02 UTC (permalink / raw)
  To: Randy Dunlap, jesper.nilsson, lars.persson, robh+dt, krzk+dt, lee.jones
  Cc: javierm, bjorn.andersson, linux-arm-kernel, devicetree,
	linux-kernel, Moon-Ki Jun, Sang Min Kim, Wangseok Lee

On 7/13/22 04:58, Randy Dunlap wrote:
> Hi--
> 
> On 7/12/22 21:55, Dongjin Yang wrote:
> > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> > index e5cfb01353d8..4b0f2d033f58 100644
> > --- a/drivers/firmware/Kconfig
> > +++ b/drivers/firmware/Kconfig
> > @@ -217,6 +217,17 @@ config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
> >  
> >            Say Y here to enable "download mode" by default.
> >  
> > +config SAMSUNG_SECURE_SERVICE
> > +        bool "Samsung Foundry Secure Service Layer"
> > +        depends on HAVE_ARM_SMCCC
> > +        default n
> 
> Drop that line, it's the default anyway.

Sure thanks.

> 
> > +        help
> > +          Support secure service layer for SoCs which is manufactured by
> 
>                                                 which are

Thanks for correcting.

> 
> > +          Samsung Foundry.
> > +
> > +          This option provide support of secure monitor service call using
> > +          Trusted Foundations.
> 
> -- 
> ~Randy


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/4] firmware: Samsung: Add secure monitor driver
       [not found]   ` <CGME20220713045516epcms1p86b3f6a8795d767faac65eb947405f911@epcms1p6>
@ 2022-07-26  0:04     ` Dongjin Yang
  2022-07-26  6:47       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 6+ messages in thread
From: Dongjin Yang @ 2022-07-26  0:04 UTC (permalink / raw)
  To: Krzysztof Kozlowski, jesper.nilsson, lars.persson, robh+dt,
	krzk+dt, lee.jones
  Cc: javierm, bjorn.andersson, linux-arm-kernel, devicetree,
	linux-kernel, Moon-Ki Jun, Sang Min Kim, Wangseok Lee


On 14/07/2022 04:28, Krzysztof Kozlowski wrote:
> On 13/07/2022 06:55, Dongjin Yang wrote:
> > Introduce a driver to provide calls into secure monitor mode.
> > This driver is used for SoCs produced by Samsung Foundry to provide
> > SMC call. This patch supports register read/write request to secure
> > monitor. Also, SMC call request which uses shared memory to exchange
> > the data between kernel and secure monitor.
> > 
> > Signed-off-by: Dongjin Yang <dj76.yang@samsung.com>
> > ---
> >  MAINTAINERS                              |   2 +
> >  drivers/firmware/Kconfig                 |  11 +++
> >  drivers/firmware/Makefile                |   1 +
> >  drivers/firmware/samsung-smc-svc.c       | 154 +++++++++++++++++++++++++++++++
> >  include/linux/firmware/samsung-smc-svc.h |  59 ++++++++++++
> >  5 files changed, 227 insertions(+)
> >  create mode 100644 drivers/firmware/samsung-smc-svc.c
> >  create mode 100644 include/linux/firmware/samsung-smc-svc.h
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 6763746c349f..d173043ffb46 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1868,8 +1868,10 @@ F:        arch/arm/boot/dts/artpec6*
> >  F:        arch/arm/mach-artpec
> >  F:        drivers/clk/axis
> >  F:        drivers/crypto/axis
> > +F:        drivers/firmware/samsung-smc-svc.c
> >  F:        drivers/mmc/host/usdhi6rol0.c
> >  F:        drivers/pinctrl/pinctrl-artpec*
> > +F:        include/linux/firmware/samsung-smc-svc.h
> 
> Same as for bindings - not related to Artpec platforms. Either add
> dedicated entry or put it under Samsung SoC.
> 

This patch is for Artpec8 SoC.

> >  
> >  ARM/ASPEED I2C DRIVER
> >  M:        Brendan Higgins <brendanhiggins@google.com>
> > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> > index e5cfb01353d8..4b0f2d033f58 100644
> > --- a/drivers/firmware/Kconfig
> > +++ b/drivers/firmware/Kconfig
> > @@ -217,6 +217,17 @@ config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
> >  
> >            Say Y here to enable "download mode" by default.
> >  
> > +config SAMSUNG_SECURE_SERVICE
> > +        bool "Samsung Foundry Secure Service Layer"
> > +        depends on HAVE_ARM_SMCCC
> > +        default n
> > +        help
> > +          Support secure service layer for SoCs which is manufactured by
> > +          Samsung Foundry.
> > +
> > +          This option provide support of secure monitor service call using
> > +          Trusted Foundations.
> > +
> >  config SYSFB
> >          bool
> >          default y
> > diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
> > index 4e58cb474a68..985e30a9665f 100644
> > --- a/drivers/firmware/Makefile
> > +++ b/drivers/firmware/Makefile
> > @@ -19,6 +19,7 @@ obj-$(CONFIG_RASPBERRYPI_FIRMWARE) += raspberrypi.o
> >  obj-$(CONFIG_FW_CFG_SYSFS)        += qemu_fw_cfg.o
> >  obj-$(CONFIG_QCOM_SCM)                += qcom-scm.o
> >  qcom-scm-objs += qcom_scm.o qcom_scm-smc.o qcom_scm-legacy.o
> > +obj-$(CONFIG_SAMSUNG_SECURE_SERVICE) += samsung-smc-svc.o
> >  obj-$(CONFIG_SYSFB)                += sysfb.o
> >  obj-$(CONFIG_SYSFB_SIMPLEFB)        += sysfb_simplefb.o
> >  obj-$(CONFIG_TI_SCI_PROTOCOL)        += ti_sci.o
> > diff --git a/drivers/firmware/samsung-smc-svc.c b/drivers/firmware/samsung-smc-svc.c
> > new file mode 100644
> > index 000000000000..eb3a5285cf2b
> > --- /dev/null
> > +++ b/drivers/firmware/samsung-smc-svc.c
> > @@ -0,0 +1,154 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2019 Samsung Electronics Co., Ltd.
> > + * Author: Kihyun Yoon<kihyun.yoon@samsung.com>
> > + * Author: Dongjin Yang<dj76.yang@samsung.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> 
> Drop boiler plate with trailing empty '*' comment.
> 

Yes,

> > + */
> > +
> > +#include <linux/types.h>
> > +#include <linux/errno.h>
> > +#include <linux/module.h>
> > +#include <linux/io.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/firmware/samsung-smc-svc.h>
> 
> Order the headers by name.
> 

Yes I will,

> > +
> > +struct samsung_smc_version {
> > +        u32 major;
> > +        u32 minor;
> > +};
> > +
> > +struct samsung_smc_data {
> > +        struct samsung_smc_version version;
> > +        u32 svc_cmd_list[SAMSUNG_SIP_NR_SIP];
> > +};
> > +
> > +struct samsung_smc_data *svc_data;
> 
> No, no globals. Even no static variables.
> 

Yes, let me change the driver

> > +
> > +static int samsung_smc_svc_init(void);
> > +
> > +static int __samsung_get_svc_cmd(u32 cmd_idx)
> > +{
> > +        if (cmd_idx >= SAMSUNG_SIP_NR_SIP)
> > +                return SAMSUNG_SIP_NOT_SUPPORTED;
> > +
> > +        /* Initialize this driver if it is not ready */
> > +        if (!svc_data && samsung_smc_svc_init()) {
> > +                pr_err("samsung,smccc_service initialization is failed\n");
> > +                return SAMSUNG_SIP_NOT_SUPPORTED;
> > +        }
> > +
> > +        return svc_data->svc_cmd_list[cmd_idx];
> > +}
> > +
> > +int samsung_smc_call(u32 cmd_idx,
> > +                     unsigned long a0, unsigned long a1,
> > +                     unsigned long a2, unsigned long a3,
> > +                     u32 *val)
> > +
> > +{
> > +        struct arm_smccc_res res;
> > +        u32 cmd = __samsung_get_svc_cmd(cmd_idx);
> > +
> > +        if (cmd == SAMSUNG_SIP_NOT_SUPPORTED)
> > +                return -EOPNOTSUPP;
> > +
> > +        arm_smccc_smc(cmd, a0, a1, a2, a3, 0, 0, 0, &res);
> > +        if (res.a0 == SMCCC_RET_NOT_SUPPORTED)
> > +                return -EINVAL;
> > +
> > +        if (val)
> > +                *val = (u32)res.a1;
> > +
> > +        return 0;
> > +}
> > +EXPORT_SYMBOL(samsung_smc_call);
> 
> EXPORT_SYMBOL_GPL
> 
> You need kerneldoc for all these.
> 

Yes,

> > +
> > +int samsung_smc_reg_read(void *base, u32 reg, u32 *val)
> > +{
> > +        return samsung_smc_call(SAMSUNG_SIP_READ_REG,
> > +                                (unsigned long)base, reg, 0, 0, val);
> > +}
> > +EXPORT_SYMBOL(samsung_smc_reg_read);
> > +
> > +int samsung_smc_reg_write(void *base, u32 reg, u32 val)
> > +{
> > +        return samsung_smc_call(SAMSUNG_SIP_WRITE_REG,
> > +                                  (unsigned long)base, reg, val, 0, NULL);
> > +}
> > +EXPORT_SYMBOL(samsung_smc_reg_write);
> > +
> > +static int samsung_smc_check_version(struct samsung_smc_version *ver)
> > +{
> > +        struct arm_smccc_res res;
> > +
> > +        arm_smccc_smc(SAMSUNG_SIP_SVC_VERSION, 0, 0, 0, 0, 0, 0, 0, &res);
> > +
> > +        if ((u32)res.a0 != ver->major || (u32)res.a1 != ver->minor)
> > +                return -EINVAL;
> > +
> > +        pr_info("Samsung SVC version [%d.%d] is detected\n",
> > +                ver->major, ver->minor);
> > +
> > +        return 0;
> > +}
> > +
> > +static const struct samsung_smc_data svc_0_1 = {
> > +        .version = {
> > +                .major = 0,
> > +                .minor = 1
> > +        },
> > +        .svc_cmd_list = {
> > +                [SAMSUNG_SIP_READ_REG] = 0x82000020,
> > +                [SAMSUNG_SIP_WRITE_REG] = 0x82000021,
> > +                /* TODO: SMC call for clock driver should be added */
> > +        }
> > +};
> > +
> > +static const struct of_device_id samsung_smc_of_match[] = {
> > +        { .compatible = "samsung,smccc-svc", .data = &svc_0_1 },
> > +        { /* sentinel */ }
> > +};
> > +
> > +static int samsung_smc_svc_init(void)
> > +{
> > +        struct device_node *fw_np;
> > +        struct device_node *np;
> > +        const struct of_device_id *match;
> > +        int ret;
> > +
> > +        if (svc_data) {
> > +                pr_info("samsung,smccc_service is already initialized\n");
> > +                return 0;
> > +        }
> > +
> > +        fw_np = of_find_node_by_name(NULL, "firmware");
> > +        if (!fw_np)
> > +                return -ENODEV;
> > +
> > +        np = of_find_matching_node_and_match(fw_np, samsung_smc_of_match,
> > +                                             &match);
> > +        if (!np)
> > +                return -ENODEV;
> > +
> > +        svc_data = (struct samsung_smc_data *)match->data;
> > +        if (!svc_data)
> > +                return -ENODEV;
> > +
> > +        ret = samsung_smc_check_version(&svc_data->version);
> > +        if (ret) {
> > +                pr_err("samsung svc version is not matched\n");
> > +                return ret;
> > +        }
> > +
> > +        pr_info("samsung smc svc is initialized\n");
> 
> No useless success printks. We have tracing and sysfs for that.
> 

Ok,

> > +        return 0;
> > +}
> > +
> > +early_initcall(samsung_smc_svc_init);
> 
> No. Make it a platform driver.
> 

Ok, I will change,

> > diff --git a/include/linux/firmware/samsung-smc-svc.h b/include/linux/firmware/samsung-smc-svc.h
> > new file mode 100644
> > index 000000000000..9c94fd3e10a2
> > --- /dev/null
> > +++ b/include/linux/firmware/samsung-smc-svc.h
> > @@ -0,0 +1,59 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#ifndef __SAMSUNG_SMC_H
> > +#define __SAMSUNG_SMC_H
> 
> Extend the header gaurd to match path.
> 

Yes,

> > +
> > +#include <linux/arm-smccc.h>
> > +
> > +#ifdef CONFIG_SAMSUNG_SECURE_SERVICE
> 
> No ifdefs in headers. This is not a subsystem.
> 

Yes, I will remove,

> > +
> > +#define SAMSUNG_SIP_CALL_VAL(x) \
> > +                ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> > +                ARM_SMCCC_SMC_32, ARM_SMCCC_OWNER_SIP, x)
> 
> Need kerneldoc.
> 

Yes, I will add kerneldoc.

> > +
> > +/* Common SIP SVC number */
> > +#define SAMSUING_SIP_SVC_CALL_COUNT        SAMSUNG_SIP_CALL_VAL(0xff00)
> > +#define SAMSUING_SIP_SVC_UID                SAMSUNG_SIP_CALL_VAL(0xff01)
> > +#define SAMSUNG_SIP_SVC_VERSION                SAMSUNG_SIP_CALL_VAL(0xff03)
> > +#define SAMSUNG_SIP_NOT_SUPPORTED        0x0
> 
> Need kerneldoc.
> 
> 
> > +
> > +enum {
> > +        /* SIP enumeration for V0.1 */
> > +        SAMSUNG_SIP_READ_REG,
> > +        SAMSUNG_SIP_WRITE_REG,
> > +        /* Add here for later version */
> > +        SAMSUNG_SIP_NR_SIP
> > +};
> 
> Need kerneldoc.
> 
> 
> > +
> > +int samsung_smc_call(u32 cmd_idx,
> > +                     unsigned long a0, unsigned long a1,
> > +                     unsigned long a2, unsigned long a3,
> > +                     u32 *val);
> > +int samsung_smc_reg_read(void *addr, unsigned int reg, u32 *val);
> > +int samsung_smc_reg_write(void *addr, unsigned int reg, u32 val);
> > +
> 
> Need kerneldoc in the driver.
> 
> 
> > +#else
> > +
> 
> No stubs, no ifdefs. It's not a subsystem.
> 
> 
> 
> Best regards,
> Krzysztof


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/4] firmware: Samsung: Add secure monitor driver
  2022-07-26  0:04     ` Dongjin Yang
@ 2022-07-26  6:47       ` Krzysztof Kozlowski
  0 siblings, 0 replies; 6+ messages in thread
From: Krzysztof Kozlowski @ 2022-07-26  6:47 UTC (permalink / raw)
  To: dj76.yang, jesper.nilsson, lars.persson, robh+dt, krzk+dt, lee.jones
  Cc: javierm, bjorn.andersson, linux-arm-kernel, devicetree,
	linux-kernel, Moon-Ki Jun, Sang Min Kim, Wangseok Lee

On 26/07/2022 02:04, Dongjin Yang wrote:
> 
> On 14/07/2022 04:28, Krzysztof Kozlowski wrote:
>> On 13/07/2022 06:55, Dongjin Yang wrote:
>>>  Introduce a driver to provide calls into secure monitor mode.
>>>  This driver is used for SoCs produced by Samsung Foundry to provide
>>>  SMC call. This patch supports register read/write request to secure
>>>  monitor. Also, SMC call request which uses shared memory to exchange
>>>  the data between kernel and secure monitor.
>>>  
>>>  Signed-off-by: Dongjin Yang <dj76.yang@samsung.com>
>>>  ---
>>>   MAINTAINERS                              |   2 +
>>>   drivers/firmware/Kconfig                 |  11 +++
>>>   drivers/firmware/Makefile                |   1 +
>>>   drivers/firmware/samsung-smc-svc.c       | 154 +++++++++++++++++++++++++++++++
>>>   include/linux/firmware/samsung-smc-svc.h |  59 ++++++++++++
>>>   5 files changed, 227 insertions(+)
>>>   create mode 100644 drivers/firmware/samsung-smc-svc.c
>>>   create mode 100644 include/linux/firmware/samsung-smc-svc.h
>>>  
>>>  diff --git a/MAINTAINERS b/MAINTAINERS
>>>  index 6763746c349f..d173043ffb46 100644
>>>  --- a/MAINTAINERS
>>>  +++ b/MAINTAINERS
>>>  @@ -1868,8 +1868,10 @@ F:        arch/arm/boot/dts/artpec6*
>>>   F:        arch/arm/mach-artpec
>>>   F:        drivers/clk/axis
>>>   F:        drivers/crypto/axis
>>>  +F:        drivers/firmware/samsung-smc-svc.c
>>>   F:        drivers/mmc/host/usdhi6rol0.c
>>>   F:        drivers/pinctrl/pinctrl-artpec*
>>>  +F:        include/linux/firmware/samsung-smc-svc.h
>>
>> Same as for bindings - not related to Artpec platforms. Either add
>> dedicated entry or put it under Samsung SoC.
>>
> 
> This patch is for Artpec8 SoC.

This does not answer my comment at all.


> 


Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-07-26  6:47 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20220713045516epcms1p86b3f6a8795d767faac65eb947405f911@epcms1p8>
2022-07-13  4:55 ` [PATCH 2/4] firmware: Samsung: Add secure monitor driver Dongjin Yang
2022-07-13  4:58   ` Randy Dunlap
2022-07-13  7:11   ` Krzysztof Kozlowski
     [not found]   ` <CGME20220713045516epcms1p86b3f6a8795d767faac65eb947405f911@epcms1p4>
2022-07-15  2:02     ` Dongjin Yang
     [not found]   ` <CGME20220713045516epcms1p86b3f6a8795d767faac65eb947405f911@epcms1p6>
2022-07-26  0:04     ` Dongjin Yang
2022-07-26  6:47       ` Krzysztof Kozlowski

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.