[PATCH 4/4] mfd: Samsung: Add Samsung sysmgr driver

[PATCH 4/4] mfd: Samsung: Add Samsung sysmgr driver

[Dongjin Yang]

This driver is used for SoCs produced by Samsung Foundry to provide
Samsung sysmgr API. The read/write request of sysmgr is delivered to
Samsung secure monitor call.

Signed-off-by: Dongjin Yang <dj76.yang@samsung.com>
---
 MAINTAINERS                        |   2 +
 drivers/mfd/Kconfig                |  11 +++
 drivers/mfd/Makefile               |   1 +
 drivers/mfd/samsung-sysmgr.c       | 167 +++++++++++++++++++++++++++++++++++++
 include/linux/mfd/samsung-sysmgr.h |  30 +++++++
 5 files changed, 211 insertions(+)
 create mode 100644 drivers/mfd/samsung-sysmgr.c
 create mode 100644 include/linux/mfd/samsung-sysmgr.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 55cb8901ccdc..44ad4bd406a9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1870,9 +1870,11 @@ F:	arch/arm/mach-artpec
 F:	drivers/clk/axis
 F:	drivers/crypto/axis
 F:	drivers/firmware/samsung-smc-svc.c
+F:	drivers/mfd/samsung-sysmgr.c
 F:	drivers/mmc/host/usdhi6rol0.c
 F:	drivers/pinctrl/pinctrl-artpec*
 F:	include/linux/firmware/samsung-smc-svc.h
+F:	include/linux/mfd/samsung-sysmgr.h
 
 ARM/ASPEED I2C DRIVER
 M:	Brendan Higgins <brendanhiggins@google.com>
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 3b59456f5545..ce6ab5842bf0 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -51,6 +51,17 @@ config MFD_ACT8945A
 	  linear regulators, along with a complete ActivePath battery
 	  charger.
 
+config MFD_SAMSUNG_SYSMGR
+	bool "System Manager for Samsung Foundry platforms"
+	depends on ARCH_ARTPEC && OF
+	select MFD_SYSCON
+	select SAMSUNG_SECURE_SERVICE
+	help
+	  Select this to get System Manager support for SoCs which use
+	  Samsung Foundry platforms.
+	  This System Manager has depedency on Samsung Secure Service
+	  for providing secure service call.
+
 config MFD_SUN4I_GPADC
 	tristate "Allwinner sunxi platforms' GPADC MFD driver"
 	select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 858cacf659d6..490f041d1262 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -248,6 +248,7 @@ obj-$(CONFIG_INTEL_SOC_PMIC_MRFLD)	+= intel_soc_pmic_mrfld.o
 
 obj-$(CONFIG_MFD_ALTERA_A10SR)	+= altera-a10sr.o
 obj-$(CONFIG_MFD_ALTERA_SYSMGR) += altera-sysmgr.o
+obj-$(CONFIG_MFD_SAMSUNG_SYSMGR) += samsung-sysmgr.o
 obj-$(CONFIG_MFD_STPMIC1)	+= stpmic1.o
 obj-$(CONFIG_MFD_SUN4I_GPADC)	+= sun4i-gpadc.o
 
diff --git a/drivers/mfd/samsung-sysmgr.c b/drivers/mfd/samsung-sysmgr.c
new file mode 100644
index 000000000000..a202e8c4c4f2
--- /dev/null
+++ b/drivers/mfd/samsung-sysmgr.c
@@ -0,0 +1,167 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Samsung Electronics, Co. Ltd.
+ * Copyright (C) 2018-2019, Intel Corporation.
+ * Copyright (C) 2012 Freescale Semiconductor, Inc.
+ * Copyright (C) 2012 Linaro Ltd.
+ *
+ * Inspired by drivers/mfd/altera-sysmgr.c
+ */
+
+#include <linux/arm-smccc.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/mfd/samsung-sysmgr.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+/**
+ * struct samsung_sysmgr - Samsung System Manager
+ * @regmap: the regmap used for System Manager accesses.
+ * @base  : the base address for the System Manager
+ */
+struct samsung_sysmgr {
+	struct regmap   *regmap;
+	resource_size_t *base;
+};
+
+static struct platform_driver samsung_sysmgr_driver;
+
+static struct regmap_config mmio_regmap_cfg = {
+	.name = "sysmgr_mmio",
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.fast_io = true,
+	.use_single_read = true,
+	.use_single_write = true,
+};
+
+static struct regmap_config samsung_smccc_regmap_cfg = {
+	.name = "samsung_sysmgr_smccc",
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.fast_io = true,
+	.use_single_read = true,
+	.use_single_write = true,
+	.reg_read = samsung_smc_reg_read,
+	.reg_write = samsung_smc_reg_write,
+};
+
+/**
+ * samsung_sysmgr_regmap_lookup_by_phandle
+ * Find the sysmgr previous configured in probe() and return regmap property.
+ * Return: regmap if found or error if not found.
+ */
+struct regmap *samsung_sysmgr_regmap_lookup_by_phandle(struct device_node *np,
+						       const char *property)
+{
+	struct device *dev;
+	struct samsung_sysmgr *sysmgr;
+	struct device_node *sysmgr_np;
+
+	if (property)
+		sysmgr_np = of_parse_phandle(np, property, 0);
+	else
+		sysmgr_np = np;
+
+	if (!sysmgr_np)
+		return ERR_PTR(-ENODEV);
+
+	dev = driver_find_device_by_of_node(&samsung_sysmgr_driver.driver,
+					    (void *)sysmgr_np);
+	of_node_put(sysmgr_np);
+	if (!dev)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	sysmgr = dev_get_drvdata(dev);
+
+	return sysmgr->regmap;
+}
+EXPORT_SYMBOL_GPL(samsung_sysmgr_regmap_lookup_by_phandle);
+
+static int sysmgr_probe(struct platform_device *pdev)
+{
+	struct samsung_sysmgr *sysmgr;
+	struct regmap *regmap;
+	struct resource *res;
+	struct device *dev = &pdev->dev;
+	struct regmap_config sysmgr_config =
+		*((struct regmap_config *)of_device_get_match_data(dev));
+
+	sysmgr = devm_kzalloc(dev, sizeof(*sysmgr), GFP_KERNEL);
+	if (!sysmgr)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENOENT;
+
+	sysmgr_config.max_register = resource_size(res) -
+				     sysmgr_config.reg_stride;
+	if (sysmgr_config.reg_read) {
+		/* Need physical address for SMCC call */
+		sysmgr->base = (resource_size_t *)res->start;
+		regmap = devm_regmap_init(dev, NULL, sysmgr->base,
+					  &sysmgr_config);
+	} else {
+		sysmgr->base = devm_ioremap(dev, res->start,
+					    resource_size(res));
+		if (!sysmgr->base)
+			return -ENOMEM;
+
+		regmap = devm_regmap_init_mmio(dev, sysmgr->base,
+					       &sysmgr_config);
+	}
+
+	if (IS_ERR(regmap)) {
+		pr_err("regmap init failed\n");
+		return PTR_ERR(regmap);
+	}
+
+	sysmgr->regmap = regmap;
+
+	platform_set_drvdata(pdev, sysmgr);
+
+	return 0;
+}
+
+static const struct of_device_id samsung_sysmgr_of_match[] = {
+	{ .compatible = "samsung,sys-mgr", .data = &mmio_regmap_cfg },
+	{
+	  .compatible = "samsung,sys-mgr-smccc",
+	  .data = &samsung_smccc_regmap_cfg
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, samsung_sysmgr_of_match);
+
+static struct platform_driver samsung_sysmgr_driver = {
+	.probe =  sysmgr_probe,
+	.driver = {
+		.name = "samsung,system_manager",
+		.of_match_table = samsung_sysmgr_of_match,
+	},
+};
+
+static int __init samsung_sysmgr_init(void)
+{
+	return platform_driver_register(&samsung_sysmgr_driver);
+}
+core_initcall(samsung_sysmgr_init);
+
+static void __exit samsung_sysmgr_exit(void)
+{
+	platform_driver_unregister(&samsung_sysmgr_driver);
+}
+module_exit(samsung_sysmgr_exit);
+
+MODULE_AUTHOR("Dongjin Yang <dj76.yang@samsung.com>");
+MODULE_DESCRIPTION("Samsung System Manager driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/samsung-sysmgr.h b/include/linux/mfd/samsung-sysmgr.h
new file mode 100644
index 000000000000..d6887cb86ea8
--- /dev/null
+++ b/include/linux/mfd/samsung-sysmgr.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2019 Samsung Electronics, Co. Ltd.
+ * Copyright (C) 2018-2019 Intel Corporation
+ * Copyright (C) 2012 Freescale Semiconductor, Inc.
+ * Copyright (C) 2012 Linaro Ltd.
+ */
+
+#ifndef __LINUX_MFD_SAMSUNG_SYSMGR_H__
+#define __LINUX_MFD_SAMSUNG_SYSMGR_H__
+
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/firmware/samsung-smc-svc.h>
+
+struct device_node;
+
+#if defined(CONFIG_MFD_SAMSUNG_SYSMGR)
+struct regmap *samsung_sysmgr_regmap_lookup_by_phandle(struct device_node *np,
+						       const char *property);
+#else
+static inline struct regmap *
+samsung_sysmgr_regmap_lookup_by_phandle(struct device_node *np,
+					const char *property)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+#endif
+
+#endif /* __LINUX_MFD_SAMSUNG_SYSMGR_H__ */
-- 
2.9.5

Re: [PATCH 4/4] mfd: Samsung: Add Samsung sysmgr driver

[Krzysztof Kozlowski]

On 13/07/2022 06:57, Dongjin Yang wrote:
> This driver is used for SoCs produced by Samsung Foundry to provide
> Samsung sysmgr API. The read/write request of sysmgr is delivered to
> Samsung secure monitor call.
> 
> Signed-off-by: Dongjin Yang <dj76.yang@samsung.com>
> ---
>  MAINTAINERS                        |   2 +
>  drivers/mfd/Kconfig                |  11 +++
>  drivers/mfd/Makefile               |   1 +
>  drivers/mfd/samsung-sysmgr.c       | 167 +++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/samsung-sysmgr.h |  30 +++++++
>  5 files changed, 211 insertions(+)
>  create mode 100644 drivers/mfd/samsung-sysmgr.c
>  create mode 100644 include/linux/mfd/samsung-sysmgr.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 55cb8901ccdc..44ad4bd406a9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1870,9 +1870,11 @@ F:	arch/arm/mach-artpec
>  F:	drivers/clk/axis
>  F:	drivers/crypto/axis
>  F:	drivers/firmware/samsung-smc-svc.c
> +F:	drivers/mfd/samsung-sysmgr.c
>  F:	drivers/mmc/host/usdhi6rol0.c
>  F:	drivers/pinctrl/pinctrl-artpec*
>  F:	include/linux/firmware/samsung-smc-svc.h
> +F:	include/linux/mfd/samsung-sysmgr.h

Not related to Axis/Artpec SoC.

>  
>  ARM/ASPEED I2C DRIVER
>  M:	Brendan Higgins <brendanhiggins@google.com>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 3b59456f5545..ce6ab5842bf0 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -51,6 +51,17 @@ config MFD_ACT8945A
>  	  linear regulators, along with a complete ActivePath battery
>  	  charger.
>  
> +config MFD_SAMSUNG_SYSMGR
> +	bool "System Manager for Samsung Foundry platforms"
> +	depends on ARCH_ARTPEC && OF

Samsung Foundry does not match ARTPEC... Artpec 6 is not Samsung Foundry
SoC, is it?

Missing compile test.

> +	select MFD_SYSCON
> +	select SAMSUNG_SECURE_SERVICE
> +	help
> +	  Select this to get System Manager support for SoCs which use
> +	  Samsung Foundry platforms.
> +	  This System Manager has depedency on Samsung Secure Service
> +	  for providing secure service call.

Looking at the driver, it does literally nothing. Looks like workaround
for incomplete bindings and DTS. It's a no-go.

> +
>  config MFD_SUN4I_GPADC
>  	tristate "Allwinner sunxi platforms' GPADC MFD driver"
>  	select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 858cacf659d6..490f041d1262 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -248,6 +248,7 @@ obj-$(CONFIG_INTEL_SOC_PMIC_MRFLD)	+= intel_soc_pmic_mrfld.o
>  
>  obj-$(CONFIG_MFD_ALTERA_A10SR)	+= altera-a10sr.o
>  obj-$(CONFIG_MFD_ALTERA_SYSMGR) += altera-sysmgr.o
> +obj-$(CONFIG_MFD_SAMSUNG_SYSMGR) += samsung-sysmgr.o
>  obj-$(CONFIG_MFD_STPMIC1)	+= stpmic1.o
>  obj-$(CONFIG_MFD_SUN4I_GPADC)	+= sun4i-gpadc.o
>  
> diff --git a/drivers/mfd/samsung-sysmgr.c b/drivers/mfd/samsung-sysmgr.c
> new file mode 100644
> index 000000000000..a202e8c4c4f2
> --- /dev/null
> +++ b/drivers/mfd/samsung-sysmgr.c
> @@ -0,0 +1,167 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 Samsung Electronics, Co. Ltd.
> + * Copyright (C) 2018-2019, Intel Corporation.
> + * Copyright (C) 2012 Freescale Semiconductor, Inc.
> + * Copyright (C) 2012 Linaro Ltd.
> + *
> + * Inspired by drivers/mfd/altera-sysmgr.c
> + */
> +
> +#include <linux/arm-smccc.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/mfd/samsung-sysmgr.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +/**
> + * struct samsung_sysmgr - Samsung System Manager
> + * @regmap: the regmap used for System Manager accesses.
> + * @base  : the base address for the System Manager
> + */
> +struct samsung_sysmgr {
> +	struct regmap   *regmap;
> +	resource_size_t *base;
> +};
> +
> +static struct platform_driver samsung_sysmgr_driver;

No, no static variables.

> +
> +static struct regmap_config mmio_regmap_cfg = {
> +	.name = "sysmgr_mmio",
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +	.fast_io = true,
> +	.use_single_read = true,
> +	.use_single_write = true,
> +};
> +
> +static struct regmap_config samsung_smccc_regmap_cfg = {
> +	.name = "samsung_sysmgr_smccc",
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +	.fast_io = true,
> +	.use_single_read = true,
> +	.use_single_write = true,
> +	.reg_read = samsung_smc_reg_read,
> +	.reg_write = samsung_smc_reg_write,
> +};
> +
> +/**
> + * samsung_sysmgr_regmap_lookup_by_phandle
> + * Find the sysmgr previous configured in probe() and return regmap property.
> + * Return: regmap if found or error if not found.
> + */
> +struct regmap *samsung_sysmgr_regmap_lookup_by_phandle(struct device_node *np,
> +						       const char *property)
> +{
> +	struct device *dev;
> +	struct samsung_sysmgr *sysmgr;
> +	struct device_node *sysmgr_np;
> +
> +	if (property)
> +		sysmgr_np = of_parse_phandle(np, property, 0);
> +	else
> +		sysmgr_np = np;
> +
> +	if (!sysmgr_np)
> +		return ERR_PTR(-ENODEV);
> +
> +	dev = driver_find_device_by_of_node(&samsung_sysmgr_driver.driver,
> +					    (void *)sysmgr_np);
> +	of_node_put(sysmgr_np);
> +	if (!dev)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	sysmgr = dev_get_drvdata(dev);
> +
> +	return sysmgr->regmap;
> +}
> +EXPORT_SYMBOL_GPL(samsung_sysmgr_regmap_lookup_by_phandle);

This breaks layers/encapsulation and looks like a hack for incomplete
bindings/DTS. No, no exporting regmaps.

> +
> +static int sysmgr_probe(struct platform_device *pdev)
> +{
> +	struct samsung_sysmgr *sysmgr;
> +	struct regmap *regmap;
> +	struct resource *res;
> +	struct device *dev = &pdev->dev;
> +	struct regmap_config sysmgr_config =
> +		*((struct regmap_config *)of_device_get_match_data(dev));
> +
> +	sysmgr = devm_kzalloc(dev, sizeof(*sysmgr), GFP_KERNEL);
> +	if (!sysmgr)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENOENT;
> +
> +	sysmgr_config.max_register = resource_size(res) -
> +				     sysmgr_config.reg_stride;
> +	if (sysmgr_config.reg_read) {
> +		/* Need physical address for SMCC call */
> +		sysmgr->base = (resource_size_t *)res->start;
> +		regmap = devm_regmap_init(dev, NULL, sysmgr->base,
> +					  &sysmgr_config);
> +	} else {
> +		sysmgr->base = devm_ioremap(dev, res->start,
> +					    resource_size(res));
> +		if (!sysmgr->base)
> +			return -ENOMEM;
> +
> +		regmap = devm_regmap_init_mmio(dev, sysmgr->base,
> +					       &sysmgr_config);
> +	}
> +
> +	if (IS_ERR(regmap)) {
> +		pr_err("regmap init failed\n");

dev_err

> +		return PTR_ERR(regmap);
> +	}
> +
> +	sysmgr->regmap = regmap;
> +
> +	platform_set_drvdata(pdev, sysmgr);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id samsung_sysmgr_of_match[] = {
> +	{ .compatible = "samsung,sys-mgr", .data = &mmio_regmap_cfg },
> +	{
> +	  .compatible = "samsung,sys-mgr-smccc",
> +	  .data = &samsung_smccc_regmap_cfg
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, samsung_sysmgr_of_match);
> +
> +static struct platform_driver samsung_sysmgr_driver = {
> +	.probe =  sysmgr_probe,
> +	.driver = {
> +		.name = "samsung,system_manager",
> +		.of_match_table = samsung_sysmgr_of_match,
> +	},
> +};
> +
> +static int __init samsung_sysmgr_init(void)
> +{
> +	return platform_driver_register(&samsung_sysmgr_driver);
> +}
> +core_initcall(samsung_sysmgr_init);

module_platform_driver() instead.

> +
> +static void __exit samsung_sysmgr_exit(void)
> +{
> +	platform_driver_unregister(&samsung_sysmgr_driver);
> +}
> +module_exit(samsung_sysmgr_exit);
> +
> +MODULE_AUTHOR("Dongjin Yang <dj76.yang@samsung.com>");
> +MODULE_DESCRIPTION("Samsung System Manager driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/samsung-sysmgr.h b/include/linux/mfd/samsung-sysmgr.h
> new file mode 100644
> index 000000000000..d6887cb86ea8
> --- /dev/null
> +++ b/include/linux/mfd/samsung-sysmgr.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2019 Samsung Electronics, Co. Ltd.
> + * Copyright (C) 2018-2019 Intel Corporation
> + * Copyright (C) 2012 Freescale Semiconductor, Inc.
> + * Copyright (C) 2012 Linaro Ltd.
> + */
> +
> +#ifndef __LINUX_MFD_SAMSUNG_SYSMGR_H__
> +#define __LINUX_MFD_SAMSUNG_SYSMGR_H__
> +
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/firmware/samsung-smc-svc.h>
> +
> +struct device_node;
> +
> +#if defined(CONFIG_MFD_SAMSUNG_SYSMGR)

No ifdefs, no stubs.



Best regards,
Krzysztof

Re: [PATCH 4/4] mfd: Samsung: Add Samsung sysmgr driver

[Jesper Nilsson]

Hi,


Thanks for your comments Krzysztof and sorry for being late in replying, I'll
try to fill in the blanks for some of the issues below.


The ARTPEC-8 is derived from a Samsung design, but built to order for Axis, so
long term responsibility will fall to Axis (me and Lars primarily).

The ARTPEC-6 and ARTPEC-7 were built by an other vendor and are quite different
not to mention that they are 32-bit ARMs, compared to ARM 64-bit for the ARTPEC-8.


The driver in this patchset is derived from the drivers/mfd/altera-sysmgr.c and
solves the same problem, in where the SoC system controller IP is a collection
of registers that controls quite a lot of different peripheral functions (from
PCIe and Ethernet to PWM) and is reachable only through SMC calls. I think the
naming of the software was set as samsung-sysmgr since it is not Axis design at
all, but (to my knowledge) only existing in ARTPEC-8 as yet. I can't recall why
the mfd driver route was selected, but it could be that the earlier
implementation was more complex and used both smc and direct mmio writes.

The users of this system manager would initially be the ARTPEC-8 DWC EQoS and
ARTPEC-8 DWC PCIe drivers sent in other patch-sets.


I believe a possible alternative to solve the system manager problem is to open
code the SMC calls directly from the drivers in question, quite a lot of
drivers seem to do this, notably a specific altera driver
(drivers/edac/altera_edac.c) even though it also has a reference to the above
mentioned altera-sysmgr regmap... :-)

Does that seem reasonable?

Thanks for your patience and excuses for the top-posting.

/Jesper

On Wed, Jul 13, 2022 at 09:27:52AM +0200, Krzysztof Kozlowski wrote:
> On 13/07/2022 06:57, Dongjin Yang wrote:
[...]

> Best regards,
> Krzysztof

/^JN - Jesper Nilsson
-- 
               Jesper Nilsson -- jesper.nilsson@axis.com

Re: [PATCH 4/4] mfd: Samsung: Add Samsung sysmgr driver

[Krzysztof Kozlowski]

On 01/09/2022 14:20, Jesper Nilsson wrote:
> Hi,
> 
> 
> Thanks for your comments Krzysztof and sorry for being late in replying, I'll
> try to fill in the blanks for some of the issues below.
> 
> 
> The ARTPEC-8 is derived from a Samsung design, but built to order for Axis, so
> long term responsibility will fall to Axis (me and Lars primarily).

Sure, yet still the code looked quite generic, not Artpec specific. At
least as far as I remember, because it was long time ago since I
commented on this. This looked like generic driver, so not really tied
to SoC platform, so it deserves even merging with existing similar drivers.

It is very likely that Samsung will re-use this bloc for several other
customers wanting FPGA, so it should not be made specific to Artpec.
Naming "samsung xxx" is okay (if it cannot be merged to existing FPGA
manager driver), but then make it a separate maintainer entry, add you
(Axis/Artpec folks), me and maybe someone from Samsung as maintainers.

> The ARTPEC-6 and ARTPEC-7 were built by an other vendor and are quite different
> not to mention that they are 32-bit ARMs, compared to ARM 64-bit for the ARTPEC-8.
> 
> 
> The driver in this patchset is derived from the drivers/mfd/altera-sysmgr.c and
> solves the same problem, in where the SoC system controller IP is a collection
> of registers that controls quite a lot of different peripheral functions (from
> PCIe and Ethernet to PWM) and is reachable only through SMC calls. I think the
> naming of the software was set as samsung-sysmgr since it is not Axis design at
> all, but (to my knowledge) only existing in ARTPEC-8 as yet. I can't recall why
> the mfd driver route was selected, but it could be that the earlier
> implementation was more complex and used both smc and direct mmio writes.
> 
> The users of this system manager would initially be the ARTPEC-8 DWC EQoS and
> ARTPEC-8 DWC PCIe drivers sent in other patch-sets.

The patchset did not contain or reference (if I remember correctly) any
 users of exposed public API, so it cannot be accepted. One cannot add
API and not use it. With users and description of the problem the driver
is solving, it's different case... but the patchset lacked both.

> I believe a possible alternative to solve the system manager problem is to open
> code the SMC calls directly from the drivers in question, quite a lot of
> drivers seem to do this, notably a specific altera driver
> (drivers/edac/altera_edac.c) even though it also has a reference to the above
> mentioned altera-sysmgr regmap... :-)
> 
> Does that seem reasonable?

Exposing SMC firmware pieces with dedicated firmware driver is already
practiced, although I don't know whether it is recommended or not. The
driver however did not expose it and instead rather was creating unused,
undocumented loophole (at least that was my impression... but again -
long time ago).

Several drivers are using ARM SMC calls, so using this would be probably
OK. If ARM SMC calls work in this case - use it and most problems are gone.

Recently Apple M1 SMC driver is being upstreamed and I did not see any
objections to its concept.

> 
> Thanks for your patience and excuses for the top-posting.
> 
> /Jesper

Best regards,
Krzysztof

Re: [PATCH 4/4] mfd: Samsung: Add Samsung sysmgr driver

[Dongjin Yang]

On 14/07/2022 04:28, Krzysztof Kozlowski wrote:
> On 13/07/2022 06:57, Dongjin Yang wrote:
> > This driver is used for SoCs produced by Samsung Foundry to provide
> > Samsung sysmgr API. The read/write request of sysmgr is delivered to
> > Samsung secure monitor call.
> > 
> > Signed-off-by: Dongjin Yang <dj76.yang@samsung.com>
> > ---
> >  MAINTAINERS                        |   2 +
> >  drivers/mfd/Kconfig                |  11 +++
> >  drivers/mfd/Makefile               |   1 +
> >  drivers/mfd/samsung-sysmgr.c       | 167 +++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/samsung-sysmgr.h |  30 +++++++
> >  5 files changed, 211 insertions(+)
> >  create mode 100644 drivers/mfd/samsung-sysmgr.c
> >  create mode 100644 include/linux/mfd/samsung-sysmgr.h
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 55cb8901ccdc..44ad4bd406a9 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1870,9 +1870,11 @@ F:        arch/arm/mach-artpec
> >  F:        drivers/clk/axis
> >  F:        drivers/crypto/axis
> >  F:        drivers/firmware/samsung-smc-svc.c
> > +F:        drivers/mfd/samsung-sysmgr.c
> >  F:        drivers/mmc/host/usdhi6rol0.c
> >  F:        drivers/pinctrl/pinctrl-artpec*
> >  F:        include/linux/firmware/samsung-smc-svc.h
> > +F:        include/linux/mfd/samsung-sysmgr.h
> 
> Not related to Axis/Artpec SoC.
> 

It is Artpec8 SoC.

> >  
> >  ARM/ASPEED I2C DRIVER
> >  M:        Brendan Higgins <brendanhiggins@google.com>
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 3b59456f5545..ce6ab5842bf0 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -51,6 +51,17 @@ config MFD_ACT8945A
> >            linear regulators, along with a complete ActivePath battery
> >            charger.
> >  
> > +config MFD_SAMSUNG_SYSMGR
> > +        bool "System Manager for Samsung Foundry platforms"
> > +        depends on ARCH_ARTPEC && OF
> 
> Samsung Foundry does not match ARTPEC... Artpec 6 is not Samsung Foundry
> SoC, is it?
> 

This is for Artpec8. 

> Missing compile test.
> 

Sorry, I will run it.

> > +        select MFD_SYSCON
> > +        select SAMSUNG_SECURE_SERVICE
> > +        help
> > +          Select this to get System Manager support for SoCs which use
> > +          Samsung Foundry platforms.
> > +          This System Manager has depedency on Samsung Secure Service
> > +          for providing secure service call.
> 
> Looking at the driver, it does literally nothing. Looks like workaround
> for incomplete bindings and DTS. It's a no-go.
> 

This driver is for providing secure smc read/write to other IP driver in Artpec8 SoC.


> > +
> >  config MFD_SUN4I_GPADC
> >          tristate "Allwinner sunxi platforms' GPADC MFD driver"
> >          select MFD_CORE
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 858cacf659d6..490f041d1262 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -248,6 +248,7 @@ obj-$(CONFIG_INTEL_SOC_PMIC_MRFLD)        += intel_soc_pmic_mrfld.o
> >  
> >  obj-$(CONFIG_MFD_ALTERA_A10SR)        += altera-a10sr.o
> >  obj-$(CONFIG_MFD_ALTERA_SYSMGR) += altera-sysmgr.o
> > +obj-$(CONFIG_MFD_SAMSUNG_SYSMGR) += samsung-sysmgr.o
> >  obj-$(CONFIG_MFD_STPMIC1)        += stpmic1.o
> >  obj-$(CONFIG_MFD_SUN4I_GPADC)        += sun4i-gpadc.o
> >  
> > diff --git a/drivers/mfd/samsung-sysmgr.c b/drivers/mfd/samsung-sysmgr.c
> > new file mode 100644
> > index 000000000000..a202e8c4c4f2
> > --- /dev/null
> > +++ b/drivers/mfd/samsung-sysmgr.c
> > @@ -0,0 +1,167 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2019 Samsung Electronics, Co. Ltd.
> > + * Copyright (C) 2018-2019, Intel Corporation.
> > + * Copyright (C) 2012 Freescale Semiconductor, Inc.
> > + * Copyright (C) 2012 Linaro Ltd.
> > + *
> > + * Inspired by drivers/mfd/altera-sysmgr.c
> > + */
> > +
> > +#include <linux/arm-smccc.h>
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/mfd/samsung-sysmgr.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +
> > +/**
> > + * struct samsung_sysmgr - Samsung System Manager
> > + * @regmap: the regmap used for System Manager accesses.
> > + * @base  : the base address for the System Manager
> > + */
> > +struct samsung_sysmgr {
> > +        struct regmap   *regmap;
> > +        resource_size_t *base;
> > +};
> > +
> > +static struct platform_driver samsung_sysmgr_driver;
> 
> No, no static variables.
> 

Ok, I will change this.

> > +
> > +static struct regmap_config mmio_regmap_cfg = {
> > +        .name = "sysmgr_mmio",
> > +        .reg_bits = 32,
> > +        .reg_stride = 4,
> > +        .val_bits = 32,
> > +        .fast_io = true,
> > +        .use_single_read = true,
> > +        .use_single_write = true,
> > +};
> > +
> > +static struct regmap_config samsung_smccc_regmap_cfg = {
> > +        .name = "samsung_sysmgr_smccc",
> > +        .reg_bits = 32,
> > +        .reg_stride = 4,
> > +        .val_bits = 32,
> > +        .fast_io = true,
> > +        .use_single_read = true,
> > +        .use_single_write = true,
> > +        .reg_read = samsung_smc_reg_read,
> > +        .reg_write = samsung_smc_reg_write,
> > +};
> > +
> > +/**
> > + * samsung_sysmgr_regmap_lookup_by_phandle
> > + * Find the sysmgr previous configured in probe() and return regmap property.
> > + * Return: regmap if found or error if not found.
> > + */
> > +struct regmap *samsung_sysmgr_regmap_lookup_by_phandle(struct device_node *np,
> > +                                                       const char *property)
> > +{
> > +        struct device *dev;
> > +        struct samsung_sysmgr *sysmgr;
> > +        struct device_node *sysmgr_np;
> > +
> > +        if (property)
> > +                sysmgr_np = of_parse_phandle(np, property, 0);
> > +        else
> > +                sysmgr_np = np;
> > +
> > +        if (!sysmgr_np)
> > +                return ERR_PTR(-ENODEV);
> > +
> > +        dev = driver_find_device_by_of_node(&samsung_sysmgr_driver.driver,
> > +                                            (void *)sysmgr_np);
> > +        of_node_put(sysmgr_np);
> > +        if (!dev)
> > +                return ERR_PTR(-EPROBE_DEFER);
> > +
> > +        sysmgr = dev_get_drvdata(dev);
> > +
> > +        return sysmgr->regmap;
> > +}
> > +EXPORT_SYMBOL_GPL(samsung_sysmgr_regmap_lookup_by_phandle);
> 
> This breaks layers/encapsulation and looks like a hack for incomplete
> bindings/DTS. No, no exporting regmaps.
> 

Similar with syscon (syscon_regmap_lookup_by_phandle), but needed additional secure smc call.

> > +
> > +static int sysmgr_probe(struct platform_device *pdev)
> > +{
> > +        struct samsung_sysmgr *sysmgr;
> > +        struct regmap *regmap;
> > +        struct resource *res;
> > +        struct device *dev = &pdev->dev;
> > +        struct regmap_config sysmgr_config =
> > +                *((struct regmap_config *)of_device_get_match_data(dev));
> > +
> > +        sysmgr = devm_kzalloc(dev, sizeof(*sysmgr), GFP_KERNEL);
> > +        if (!sysmgr)
> > +                return -ENOMEM;
> > +
> > +        res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +        if (!res)
> > +                return -ENOENT;
> > +
> > +        sysmgr_config.max_register = resource_size(res) -
> > +                                     sysmgr_config.reg_stride;
> > +        if (sysmgr_config.reg_read) {
> > +                /* Need physical address for SMCC call */
> > +                sysmgr->base = (resource_size_t *)res->start;
> > +                regmap = devm_regmap_init(dev, NULL, sysmgr->base,
> > +                                          &sysmgr_config);
> > +        } else {
> > +                sysmgr->base = devm_ioremap(dev, res->start,
> > +                                            resource_size(res));
> > +                if (!sysmgr->base)
> > +                        return -ENOMEM;
> > +
> > +                regmap = devm_regmap_init_mmio(dev, sysmgr->base,
> > +                                               &sysmgr_config);
> > +        }
> > +
> > +        if (IS_ERR(regmap)) {
> > +                pr_err("regmap init failed\n");
> 
> dev_err

Will change it.

> 
> > +                return PTR_ERR(regmap);
> > +        }
> > +
> > +        sysmgr->regmap = regmap;
> > +
> > +        platform_set_drvdata(pdev, sysmgr);
> > +
> > +        return 0;
> > +}
> > +
> > +static const struct of_device_id samsung_sysmgr_of_match[] = {
> > +        { .compatible = "samsung,sys-mgr", .data = &mmio_regmap_cfg },
> > +        {
> > +          .compatible = "samsung,sys-mgr-smccc",
> > +          .data = &samsung_smccc_regmap_cfg
> > +        },
> > +        {},
> > +};
> > +MODULE_DEVICE_TABLE(of, samsung_sysmgr_of_match);
> > +
> > +static struct platform_driver samsung_sysmgr_driver = {
> > +        .probe =  sysmgr_probe,
> > +        .driver = {
> > +                .name = "samsung,system_manager",
> > +                .of_match_table = samsung_sysmgr_of_match,
> > +        },
> > +};
> > +
> > +static int __init samsung_sysmgr_init(void)
> > +{
> > +        return platform_driver_register(&samsung_sysmgr_driver);
> > +}
> > +core_initcall(samsung_sysmgr_init);
> 
> module_platform_driver() instead.

ok, I will change it.

> 
> > +
> > +static void __exit samsung_sysmgr_exit(void)
> > +{
> > +        platform_driver_unregister(&samsung_sysmgr_driver);
> > +}
> > +module_exit(samsung_sysmgr_exit);
> > +
> > +MODULE_AUTHOR("Dongjin Yang <dj76.yang@samsung.com>");
> > +MODULE_DESCRIPTION("Samsung System Manager driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/mfd/samsung-sysmgr.h b/include/linux/mfd/samsung-sysmgr.h
> > new file mode 100644
> > index 000000000000..d6887cb86ea8
> > --- /dev/null
> > +++ b/include/linux/mfd/samsung-sysmgr.h
> > @@ -0,0 +1,30 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2019 Samsung Electronics, Co. Ltd.
> > + * Copyright (C) 2018-2019 Intel Corporation
> > + * Copyright (C) 2012 Freescale Semiconductor, Inc.
> > + * Copyright (C) 2012 Linaro Ltd.
> > + */
> > +
> > +#ifndef __LINUX_MFD_SAMSUNG_SYSMGR_H__
> > +#define __LINUX_MFD_SAMSUNG_SYSMGR_H__
> > +
> > +#include <linux/err.h>
> > +#include <linux/errno.h>
> > +#include <linux/firmware/samsung-smc-svc.h>
> > +
> > +struct device_node;
> > +
> > +#if defined(CONFIG_MFD_SAMSUNG_SYSMGR)
> 
> No ifdefs, no stubs.
> 

is it ok to move stub code into samsung-smc-svc.c

> 
> 
> Best regards,
> Krzysztof

Re: [PATCH 4/4] mfd: Samsung: Add Samsung sysmgr driver

[Krzysztof Kozlowski]

On 26/07/2022 09:51, Dongjin Yang wrote:
> On 14/07/2022 04:28, Krzysztof Kozlowski wrote:
>> On 13/07/2022 06:57, Dongjin Yang wrote:
>>>  This driver is used for SoCs produced by Samsung Foundry to provide
>>>  Samsung sysmgr API. The read/write request of sysmgr is delivered to
>>>  Samsung secure monitor call.
>>>  
>>>  Signed-off-by: Dongjin Yang <dj76.yang@samsung.com>
>>>  ---
>>>   MAINTAINERS                        |   2 +
>>>   drivers/mfd/Kconfig                |  11 +++
>>>   drivers/mfd/Makefile               |   1 +
>>>   drivers/mfd/samsung-sysmgr.c       | 167 +++++++++++++++++++++++++++++++++++++
>>>   include/linux/mfd/samsung-sysmgr.h |  30 +++++++
>>>   5 files changed, 211 insertions(+)
>>>   create mode 100644 drivers/mfd/samsung-sysmgr.c
>>>   create mode 100644 include/linux/mfd/samsung-sysmgr.h
>>>  
>>>  diff --git a/MAINTAINERS b/MAINTAINERS
>>>  index 55cb8901ccdc..44ad4bd406a9 100644
>>>  --- a/MAINTAINERS
>>>  +++ b/MAINTAINERS
>>>  @@ -1870,9 +1870,11 @@ F:        arch/arm/mach-artpec
>>>   F:        drivers/clk/axis
>>>   F:        drivers/crypto/axis
>>>   F:        drivers/firmware/samsung-smc-svc.c
>>>  +F:        drivers/mfd/samsung-sysmgr.c
>>>   F:        drivers/mmc/host/usdhi6rol0.c
>>>   F:        drivers/pinctrl/pinctrl-artpec*
>>>   F:        include/linux/firmware/samsung-smc-svc.h
>>>  +F:        include/linux/mfd/samsung-sysmgr.h
>>
>> Not related to Axis/Artpec SoC.
>>
> 
> It is Artpec8 SoC.
> 
>>>   
>>>   ARM/ASPEED I2C DRIVER
>>>   M:        Brendan Higgins <brendanhiggins@google.com>
>>>  diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>>>  index 3b59456f5545..ce6ab5842bf0 100644
>>>  --- a/drivers/mfd/Kconfig
>>>  +++ b/drivers/mfd/Kconfig
>>>  @@ -51,6 +51,17 @@ config MFD_ACT8945A
>>>             linear regulators, along with a complete ActivePath battery
>>>             charger.
>>>   
>>>  +config MFD_SAMSUNG_SYSMGR
>>>  +        bool "System Manager for Samsung Foundry platforms"
>>>  +        depends on ARCH_ARTPEC && OF
>>
>> Samsung Foundry does not match ARTPEC... Artpec 6 is not Samsung Foundry
>> SoC, is it?
>>
> 
> This is for Artpec8. 
> 
>> Missing compile test.
>>
> 
> Sorry, I will run it.
> 
>>>  +        select MFD_SYSCON
>>>  +        select SAMSUNG_SECURE_SERVICE
>>>  +        help
>>>  +          Select this to get System Manager support for SoCs which use
>>>  +          Samsung Foundry platforms.
>>>  +          This System Manager has depedency on Samsung Secure Service
>>>  +          for providing secure service call.
>>
>> Looking at the driver, it does literally nothing. Looks like workaround
>> for incomplete bindings and DTS. It's a no-go.
>>
> 
> This driver is for providing secure smc read/write to other IP driver in Artpec8 SoC.

There are no users of it and as I said - it looks like a workaround for
incomplete bindings. You do not justify here chosen design.


(...)

>>>  +EXPORT_SYMBOL_GPL(samsung_sysmgr_regmap_lookup_by_phandle);
>>
>> This breaks layers/encapsulation and looks like a hack for incomplete
>> bindings/DTS. No, no exporting regmaps.
>>
> 
> Similar with syscon (syscon_regmap_lookup_by_phandle), but needed additional secure smc call.

That's not what the code is doing.

Best regards,
Krzysztof