Message-ID: <184791843e98e0a0.ed7541b3db6a6586.57e5fabaf9bf62ee@Jude-Air.local>
Date: Tue, 10 Jun 2025 03:42:57 +0000
From: "Junhui Liu" <junhui.liu@...moral.tech>
To: "Peng Fan" <peng.fan@....nxp.com>
Cc: "Bjorn Andersson" <andersson@...nel.org>, 
	"Mathieu Poirier" <mathieu.poirier@...aro.org>, 
	"Rob Herring" <robh@...nel.org>, 
	"Krzysztof Kozlowski" <krzk+dt@...nel.org>, 
	"Conor Dooley" <conor+dt@...nel.org>, 
	"Chen Wang" <unicorn_wang@...look.com>, 
	"Inochi Amaoto" <inochiama@...il.com>, 
	"Philipp Zabel" <p.zabel@...gutronix.de>, 
	"Paul Walmsley" <paul.walmsley@...ive.com>, 
	"Palmer Dabbelt" <palmer@...belt.com>, "Albert Ou" <aou@...s.berkeley.edu>, 
	"Alexandre Ghiti" <alex@...ti.fr>, <linux-remoteproc@...r.kernel.org>, 
	<devicetree@...r.kernel.org>, <sophgo@...ts.linux.dev>, 
	<linux-kernel@...r.kernel.org>, <linux-riscv@...ts.infradead.org>
Subject: Re: [PATCH 2/2] drivers: remoteproc: Add C906L controller for Sophgo
	 CV1800B SoC

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@...moral.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@...moral.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?

> 
>>+};
>>+
>>+static int cv1800b_c906l_probe(struct platform_device *pdev)
>>+{
>>+	struct device *dev = &pdev->dev;
>>+	struct device_node *np = dev->of_node;
>>+	struct cv1800b_c906l *priv;
>>+	struct rproc *rproc;
>>+	const char *fw_name;
>>+	int ret;
>>+
>>+	ret = rproc_of_parse_firmware(dev, 0, &fw_name);
>>+	if (ret)
>>+		return dev_err_probe(dev, ret, "No firmware filename given\n");
>>+
>>+	rproc = devm_rproc_alloc(dev, dev_name(dev), &cv1800b_c906l_ops,
>>+				 fw_name, sizeof(*priv));
>>+	if (!rproc)
>>+		return dev_err_probe(dev, -ENOMEM,
>>+				     "unable to allocate remoteproc\n");
>>+
>>+	rproc->has_iommu = false;
>>+
>>+	priv = rproc->priv;
>>+	priv->dev = dev;
>>+	priv->rproc = rproc;
>>+
>>+	priv->syscon = syscon_regmap_lookup_by_phandle(np, "sophgo,syscon");
>>+	if (IS_ERR(priv->syscon))
>>+		return PTR_ERR(priv->syscon);
>>+
>>+	priv->reset = devm_reset_control_get_exclusive(dev, NULL);
>>+	if (IS_ERR(priv->reset))
>>+		return dev_err_probe(dev, PTR_ERR(priv->reset),
>>+				     "failed to get reset control handle\n");
>>+
>>+	platform_set_drvdata(pdev, rproc);
>>+
>>+	ret = devm_rproc_add(dev, rproc);
>>+	if (ret)
>>+		return dev_err_probe(dev, ret, "rproc_add failed\n");
>>+
>>+	return 0;
>>+}
>>+
>>+static void cv1800b_c906l_remove(struct platform_device *pdev)
>>+{
>>+	struct rproc *rproc = platform_get_drvdata(pdev);
>>+
>>+	if (atomic_read(&rproc->power) > 0)
>>+		rproc_shutdown(rproc);
> 
> I think the remoteproc framework should block remove to be executed
> if 'power > 0'.  If not, the framework should be enhanced.

Okay. I realized that rproc_shutdown() will be called in rproc_del(), I
will remove rproc_shutdown() and only keep rproc_del()

> 
> Regards,
> Peng

-- 
Best regards,
Junhui Liu

Powered by blists - more mailing lists