| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250611062939.GA9120@nxa18884-linux>
Date: Wed, 11 Jun 2025 14:29:39 +0800
From: Peng Fan <peng.fan@....nxp.com>
To: Junhui Liu <junhui.liu@...moral.tech>
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
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@...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?
There is no need, I overlooked RPROC_DETACHED
Regards,
Peng
Powered by blists - more mailing lists