Re: [PATCH 2/2] drivers: remoteproc: Add C906L controller for Sophgo CV1800B SoC

[Peng Fan <peng.fan@oss.nxp.com>]

On Tue, Jun 10, 2025 at 03:42:57AM +0000, Junhui Liu wrote:
>Hi Peng,
>Thanks for your review.
>
>On 09/06/2025 16:43, Peng Fan wrote:
>> On Sun, Jun 08, 2025 at 10:37:40AM +0800, Junhui Liu wrote:
>>>Add initial support for the C906L remote processor found in the Sophgo
>>>CV1800B SoC. The C906L is an asymmetric core typically used to run an
>>>RTOS. This driver enables firmware loading and start/stop control of the
>>>C906L processor via the remoteproc framework.
>>>
>>>The C906L and the main application processor can communicate through
>>>mailboxes [1]. Support for mailbox-based functionality will be added in
>>>a separate patch.
>>>
>>>Link: https://lore.kernel.org/linux-riscv/20250520-cv18xx-mbox-v4-0-fd4f1c676d6e@pigmoral.tech/ [1]
>>>Signed-off-by: Junhui Liu <junhui.liu@pigmoral.tech>
>>>---
>>> drivers/remoteproc/Kconfig                |   9 ++
>>> drivers/remoteproc/Makefile               |   1 +
>>> drivers/remoteproc/sophgo_cv1800b_c906l.c | 233 ++++++++++++++++++++++++++++++
>>> 3 files changed, 243 insertions(+)
>>>
>>>diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>>>index 83962a114dc9fdb3260e6e922602f2da53106265..7b09a8f00332605ee528ff7c21c31091c10c2bf5 100644
>>>--- a/drivers/remoteproc/Kconfig
>>>+++ b/drivers/remoteproc/Kconfig
>>>@@ -299,6 +299,15 @@ config RCAR_REMOTEPROC
>>> 	  This can be either built-in or a loadable module.
>>> 	  If compiled as module (M), the module name is rcar_rproc.
>>> 
>>>+config SOPHGO_CV1800B_C906L
>>>+	tristate "Sophgo CV1800B C906L remoteproc support"
>>>+	depends on ARCH_SOPHGO || COMPILE_TEST
>>>+	help
>>>+	  Say y here to support CV1800B C906L remote processor via the remote
>>>+	  processor framework.
>>>+
>>>+	  It's safe to say N here.
>>>+
>>> config ST_REMOTEPROC
>>> 	tristate "ST remoteproc support"
>>> 	depends on ARCH_STI
>>>diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
>>>index 1c7598b8475d6057a3e044b41e3515103b7aa9f1..3c1e9387491cedc9dda8219f1e9130a84538156f 100644
>>>--- a/drivers/remoteproc/Makefile
>>>+++ b/drivers/remoteproc/Makefile
>>>@@ -33,6 +33,7 @@ obj-$(CONFIG_QCOM_WCNSS_PIL)		+= qcom_wcnss_pil.o
>>> qcom_wcnss_pil-y			+= qcom_wcnss.o
>>> qcom_wcnss_pil-y			+= qcom_wcnss_iris.o
>>> obj-$(CONFIG_RCAR_REMOTEPROC)		+= rcar_rproc.o
>>>+obj-$(CONFIG_SOPHGO_CV1800B_C906L)	+= sophgo_cv1800b_c906l.o
>>> obj-$(CONFIG_ST_REMOTEPROC)		+= st_remoteproc.o
>>> obj-$(CONFIG_ST_SLIM_REMOTEPROC)	+= st_slim_rproc.o
>>> obj-$(CONFIG_STM32_RPROC)		+= stm32_rproc.o
>>>diff --git a/drivers/remoteproc/sophgo_cv1800b_c906l.c b/drivers/remoteproc/sophgo_cv1800b_c906l.c
>>>new file mode 100644
>>>index 0000000000000000000000000000000000000000..f3c8d8fd4f796d0cf64f8ab0dd797e017b8e8be7
>>>--- /dev/null
>>>+++ b/drivers/remoteproc/sophgo_cv1800b_c906l.c
>>>@@ -0,0 +1,233 @@
>>>+// SPDX-License-Identifier: GPL-2.0-or-later
>>>+/*
>>>+ * Copyright (C) 2025 Junhui Liu <junhui.liu@pigmoral.tech>
>>>+ */
>>>+
>>>+#include <linux/mfd/syscon.h>
>>>+#include <linux/module.h>
>>>+#include <linux/of_device.h>
>>>+#include <linux/of_reserved_mem.h>
>>>+#include <linux/platform_device.h>
>>>+#include <linux/remoteproc.h>
>>>+#include <linux/reset.h>
>>>+#include <linux/regmap.h>
>>>+
>>>+#include "remoteproc_internal.h"
>>>+
>>>+#define CV1800B_SYS_C906L_CTRL_REG	0x04
>>>+#define   CV1800B_SYS_C906L_CTRL_EN	BIT(13)
>> 
>> Align the format.
>> 
>> '#include <linux/bits.h>' should be added for BIT
>> 
>
>Will do in v2.
>
>>>+
>>>+#define CV1800B_SYS_C906L_BOOTADDR_REG	0x20
>>>+
>>>+/**
>>>+ * struct cv1800b_c906l - C906L remoteproc structure
>>>+ * @dev: private pointer to the device
>>>+ * @reset: reset control handle
>>>+ * @rproc: the remote processor handle
>>>+ * @syscon: regmap for accessing security system registers
>>>+ */
>>>+struct cv1800b_c906l {
>>>+	struct device *dev;
>>>+	struct reset_control *reset;
>>>+	struct rproc *rproc;
>>>+	struct regmap *syscon;
>>>+};
>>>+
>>>+static int cv1800b_c906l_mem_alloc(struct rproc *rproc,
>>>+				   struct rproc_mem_entry *mem)
>>>+{
>>>+	void *va;
>>>+
>>>+	va = ioremap_wc(mem->dma, mem->len);
>>>+	if (IS_ERR_OR_NULL(va))
>> 
>> Use "if (!va)"?
>
>Will do in v2.
>
>> 
>>>+		return -ENOMEM;
>>>+
>>>+	/* Update memory entry va */
>>>+	mem->va = va;
>>>+
>>>+	return 0;
>>>+}
>>>+
>>>+static int cv1800b_c906l_mem_release(struct rproc *rproc,
>>>+				     struct rproc_mem_entry *mem)
>>>+{
>>>+	iounmap(mem->va);
>>>+
>>>+	return 0;
>>>+}
>>>+
>>>+static int cv1800b_c906l_add_carveout(struct rproc *rproc)
>>>+{
>>>+	struct device *dev = rproc->dev.parent;
>>>+	struct device_node *np = dev->of_node;
>>>+	struct of_phandle_iterator it;
>>>+	struct rproc_mem_entry *mem;
>>>+	struct reserved_mem *rmem;
>>>+
>>>+	/* Register associated reserved memory regions */
>>>+	of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
>>>+	while (of_phandle_iterator_next(&it) == 0) {
>>>+		rmem = of_reserved_mem_lookup(it.node);
>>>+		if (!rmem) {
>>>+			of_node_put(it.node);
>>>+			return -EINVAL;
>>>+		}
>> 
>> Is there a need to handle vdev0buffer?
>
>I'll exclude it.
>
>> 
>>>+
>>>+		mem = rproc_mem_entry_init(dev, NULL, (dma_addr_t)rmem->base,
>>>+					   rmem->size, rmem->base,
>>>+					   cv1800b_c906l_mem_alloc,
>>>+					   cv1800b_c906l_mem_release,
>>>+					   it.node->name);
>>>+
>>>+		if (!mem) {
>>>+			of_node_put(it.node);
>>>+			return -ENOMEM;
>>>+		}
>>>+
>>>+		rproc_add_carveout(rproc, mem);
>>>+	}
>>>+
>>>+	return 0;
>>>+}
>>>+
>>>+static int cv1800b_c906l_prepare(struct rproc *rproc)
>>>+{
>>>+	struct cv1800b_c906l *priv = rproc->priv;
>>>+	int ret;
>>>+
>>>+	ret = cv1800b_c906l_add_carveout(rproc);
>>>+	if (ret)
>>>+		return ret;
>>>+
>>>+	/*
>>>+	 * This control bit must be set to enable the C906L remote processor.
>>>+	 * Note that once the remote processor is running, merely clearing
>>>+	 * this bit will not stop its execution.
>>>+	 */
>>>+	return regmap_update_bits(priv->syscon, CV1800B_SYS_C906L_CTRL_REG,
>>>+				  CV1800B_SYS_C906L_CTRL_EN,
>>>+				  CV1800B_SYS_C906L_CTRL_EN);
>>>+}
>>>+
>>>+static int cv1800b_c906l_start(struct rproc *rproc)
>>>+{
>>>+	struct cv1800b_c906l *priv = rproc->priv;
>>>+	u32 bootaddr[2];
>>>+	int ret;
>>>+
>>>+	bootaddr[0] = lower_32_bits(rproc->bootaddr);
>>>+	bootaddr[1] = upper_32_bits(rproc->bootaddr);
>>>+
>>>+	ret = regmap_bulk_write(priv->syscon, CV1800B_SYS_C906L_BOOTADDR_REG,
>>>+				bootaddr, ARRAY_SIZE(bootaddr));
>>>+	if (ret)
>>>+		return ret;
>>>+
>>>+	return reset_control_deassert(priv->reset);
>>>+}
>>>+
>>>+static int cv1800b_c906l_stop(struct rproc *rproc)
>>>+{
>>>+	struct cv1800b_c906l *priv = rproc->priv;
>>>+
>>>+	return reset_control_assert(priv->reset);
>>>+}
>>>+
>>>+static int cv1800b_c906l_parse_fw(struct rproc *rproc,
>>>+				  const struct firmware *fw)
>>>+{
>>>+	int ret;
>>>+
>>>+	ret = rproc_elf_load_rsc_table(rproc, fw);
>>>+	if (ret == -EINVAL) {
>>>+		dev_info(&rproc->dev, "No resource table in elf\n");
>>>+		ret = 0;
>>>+	}
>>>+
>>>+	return ret;
>>>+}
>>>+
>>>+static const struct rproc_ops cv1800b_c906l_ops = {
>>>+	.prepare = cv1800b_c906l_prepare,
>>>+	.start = cv1800b_c906l_start,
>>>+	.stop = cv1800b_c906l_stop,
>>>+	.load = rproc_elf_load_segments,
>>>+	.parse_fw = cv1800b_c906l_parse_fw,
>>>+	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
>>>+	.sanity_check = rproc_elf_sanity_check,
>>>+	.get_boot_addr = rproc_elf_get_boot_addr,
>> 
>> Seems your setup does not support attach mode, so better add
>> attach hook and return -ENOTSUPP?
>
>I checked the remoteproc framework code and found that the attach
>function will only be called when 'rproc->state == RPROC_DETACHED', and
>it seems that rproc->state will not be set to RPROC_DETACHED unless I do
>so explicitly in the driver or an implemented detach function is called,
>neither of which happens in this driver.
>
>Given this, do we still need to add an attach hook even though it will
>not be called in practice?

There is no need, I overlooked RPROC_DETACHED

Regards,
Peng