| 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: <aa757d1f-4c5a-4a66-aa96-9d611a8e6bae@kernel.org>
Date: Wed, 2 Apr 2025 13:16:22 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Xiangzhi Tang <xiangzhi.tang@...iatek.com>,
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>, Matthias Brugger <matthias.bgg@...il.com>,
AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Cc: linux-remoteproc@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org,
Project_Global_Chrome_Upstream_Group@...iatek.com, jjian.zhou@...iatek.com,
hailong.fan@...iatek.com
Subject: Re: [PATCH 2/2] remoterpoc: mediatek: vcp: Add vcp remoteproc driver
On 02/04/2025 11:19, Xiangzhi Tang wrote:
> Add host driver to control the mediatek Risc-V coprocessor
>
> 1.Support rproc mechanism to load vcm firmware from filesystem
> 2.Support SMC services to request ATF to setting vcp boot sequence
> 3.Host communicated with VCP depends on VCP IPC interfaces
>
> Signed-off-by: Xiangzhi Tang <xiangzhi.tang@...iatek.com>
> ---
> drivers/remoteproc/Kconfig | 12 +
> drivers/remoteproc/Makefile | 4 +
> drivers/remoteproc/mtk_vcp_common.c | 982 ++++++++++++++++++++++
> drivers/remoteproc/mtk_vcp_common.h | 251 ++++++
> drivers/remoteproc/mtk_vcp_rproc.c | 724 ++++++++++++++++
> drivers/remoteproc/mtk_vcp_rproc.h | 107 +++
> include/linux/remoteproc/mtk_vcp_public.h | 138 +++
> include/linux/soc/mediatek/mtk_sip_svc.h | 3 +
> 8 files changed, 2221 insertions(+)
> create mode 100644 drivers/remoteproc/mtk_vcp_common.c
> create mode 100644 drivers/remoteproc/mtk_vcp_common.h
> create mode 100644 drivers/remoteproc/mtk_vcp_rproc.c
> create mode 100644 drivers/remoteproc/mtk_vcp_rproc.h
> create mode 100644 include/linux/remoteproc/mtk_vcp_public.h
>
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index 83962a114dc9..28e71c6c6dd3 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -64,6 +64,18 @@ config MTK_SCP
>
> It's safe to say N here.
>
> +config MTK_VCP_RPROC
> + tristate "MediaTek VCP support"
> + depends on ARCH_MEDIATEK || COMPILE_TEST
> + depends on ARCH_DMA_ADDR_T_64BIT
> + select MTK_VCP_IPC
> + select MTK_VCP_MBOX
> + help
> + Say y here to support MediaTek's Video Companion Processor (VCP) via
> + the remote processor framework.
> +
> + It's safe to say N here.
> +
> config OMAP_REMOTEPROC
> tristate "OMAP remoteproc support"
> depends on ARCH_OMAP4 || SOC_OMAP5 || SOC_DRA7XX
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index 5ff4e2fee4ab..327043b31b37 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -15,6 +15,10 @@ obj-$(CONFIG_IMX_REMOTEPROC) += imx_rproc.o
> obj-$(CONFIG_IMX_DSP_REMOTEPROC) += imx_dsp_rproc.o
> obj-$(CONFIG_INGENIC_VPU_RPROC) += ingenic_rproc.o
> obj-$(CONFIG_MTK_SCP) += mtk_scp.o mtk_scp_ipi.o
> +obj-$(CONFIG_MTK_VCP_RPROC) += mtk_vcp.o
> +mtk_vcp-$(CONFIG_MTK_VCP_RPROC) += mtk_vcp_rproc.o
> +mtk_vcp-$(CONFIG_MTK_VCP_RPROC) += mtk_vcp_irq.o
> +mtk_vcp-$(CONFIG_MTK_VCP_RPROC) += mtk_vcp_loader.o
> obj-$(CONFIG_OMAP_REMOTEPROC) += omap_remoteproc.o
> obj-$(CONFIG_WKUP_M3_RPROC) += wkup_m3_rproc.o
> obj-$(CONFIG_DA8XX_REMOTEPROC) += da8xx_remoteproc.o
> diff --git a/drivers/remoteproc/mtk_vcp_common.c b/drivers/remoteproc/mtk_vcp_common.c
> new file mode 100644
> index 000000000000..e02c6e61b990
> --- /dev/null
> +++ b/drivers/remoteproc/mtk_vcp_common.c
> @@ -0,0 +1,982 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2025 MediaTek Corporation. All rights reserved.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/delay.h>
> +#include <linux/dma-buf.h>
> +#include <linux/dma-heap.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/fs.h>
> +#include <linux/interrupt.h>
> +#include <linux/iommu.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_fdt.h>
> +#include <linux/of_reserved_mem.h>
> +#include <linux/poll.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/timer.h>
> +#include <uapi/linux/dma-heap.h>
> +
> +#include "mtk_vcp_common.h"
> +#include "mtk_vcp_rproc.h"
> +
> +static bool vcp_ready[VCP_CORE_TOTAL];
> +/* vcp ready status for notify */
> +static DEFINE_MUTEX(vcp_ready_mutex);
> +static DEFINE_MUTEX(vcp_pw_clk_mutex);
> +static DEFINE_MUTEX(vcp_A_notify_mutex);
> +static DEFINE_MUTEX(vcp_feature_mutex);
Way too many global variables and mutexes. Why is all this designed as
singleton?
...
> +int vcp_wdt_irq_init(struct mtk_vcp_device *vcp)
> +{
> + int ret;
> +
> + ret = devm_request_irq(vcp->dev, platform_get_irq(vcp->pdev, 0),
> + vcp_irq_handler, IRQF_ONESHOT,
> + vcp->pdev->name, vcp);
> + if (ret)
> + dev_err(vcp->dev, "failed to request wdt irq\n");
> +
> + return ret;
> +}
> +
> +MODULE_AUTHOR("Xiangzhi Tang <xiangzhi.tang@...iatek.com>");
> +MODULE_DESCRIPTION("MTK VCP Controller");
> +MODULE_LICENSE("GPL");
How many drivers do you have? I think I saw only one in Makefile.
...
> +static const struct rproc_ops mtk_vcp_ops = {
> + .load = mtk_vcp_load,
> + .start = mtk_vcp_start,
> + .stop = mtk_vcp_stop,
> +};
> +
> +
> +struct mtk_mbox_send_table send_data[] = {
Why not static? Not const?
> + { .msg_size = 18, .ipi_id = 0, .mbox_id = 0 },
> +
> + { .msg_size = 8, .ipi_id = 15, .mbox_id = 1 },
> + { .msg_size = 18, .ipi_id = 16, .mbox_id = 1 },
> + { .msg_size = 2, .ipi_id = 9, .mbox_id = 1 },
> +
> + { .msg_size = 18, .ipi_id = 11, .mbox_id = 2 },
> + { .msg_size = 2, .ipi_id = 2, .mbox_id = 2 },
> + { .msg_size = 3, .ipi_id = 3, .mbox_id = 2 },
> + { .msg_size = 2, .ipi_id = 32, .mbox_id = 2 },
> +
> + { .msg_size = 2, .ipi_id = 33, .mbox_id = 3 },
> + { .msg_size = 2, .ipi_id = 13, .mbox_id = 3 },
> + { .msg_size = 2, .ipi_id = 35, .mbox_id = 3 },
> +
> + { .msg_size = 2, .ipi_id = 20, .mbox_id = 4 },
> + { .msg_size = 3, .ipi_id = 21, .mbox_id = 4 },
> + { .msg_size = 2, .ipi_id = 23, .mbox_id = 4 }
> +};
> +
> +struct mtk_mbox_recv_table recv_data[] = {
> + { .recv_opt = 0, .msg_size = 18, .ipi_id = 1, .mbox_id = 0 },
> +
> + { .recv_opt = 1, .msg_size = 8, .ipi_id = 15, .mbox_id = 1 },
> + { .recv_opt = 0, .msg_size = 18, .ipi_id = 17, .mbox_id = 1 },
> + { .recv_opt = 0, .msg_size = 2, .ipi_id = 10, .mbox_id = 1 },
> +
> + { .recv_opt = 0, .msg_size = 18, .ipi_id = 12, .mbox_id = 2 },
> + { .recv_opt = 0, .msg_size = 1, .ipi_id = 5, .mbox_id = 2 },
> + { .recv_opt = 1, .msg_size = 1, .ipi_id = 2, .mbox_id = 2 },
> +
> + { .recv_opt = 0, .msg_size = 2, .ipi_id = 34, .mbox_id = 3 },
> + { .recv_opt = 0, .msg_size = 2, .ipi_id = 14, .mbox_id = 3 },
> +
> + { .recv_opt = 0, .msg_size = 1, .ipi_id = 26, .mbox_id = 4 },
> + { .recv_opt = 1, .msg_size = 1, .ipi_id = 20, .mbox_id = 4 }
> +};
> +
> +struct mtk_mbox_table ipc_table = {
> + .send_table = {
> + { .msg_size = 18, .ipi_id = 0, .mbox_id = 0 },
> +
> + { .msg_size = 8, .ipi_id = 15, .mbox_id = 1 },
> + { .msg_size = 18, .ipi_id = 16, .mbox_id = 1 },
> + { .msg_size = 2, .ipi_id = 9, .mbox_id = 1 },
> +
> + { .msg_size = 18, .ipi_id = 11, .mbox_id = 2 },
> + { .msg_size = 2, .ipi_id = 2, .mbox_id = 2 },
> + { .msg_size = 3, .ipi_id = 3, .mbox_id = 2 },
> + { .msg_size = 2, .ipi_id = 32, .mbox_id = 2 },
> +
> + { .msg_size = 2, .ipi_id = 33, .mbox_id = 3 },
> + { .msg_size = 2, .ipi_id = 13, .mbox_id = 3 },
> + { .msg_size = 2, .ipi_id = 35, .mbox_id = 3 },
> +
> + { .msg_size = 2, .ipi_id = 20, .mbox_id = 4 },
> + { .msg_size = 3, .ipi_id = 21, .mbox_id = 4 },
> + { .msg_size = 2, .ipi_id = 23, .mbox_id = 4 }
> + },
> + .recv_table = {
> + { .recv_opt = 0, .msg_size = 18, .ipi_id = 1, .mbox_id = 0 },
> +
> + { .recv_opt = 1, .msg_size = 8, .ipi_id = 15, .mbox_id = 1 },
> + { .recv_opt = 0, .msg_size = 18, .ipi_id = 17, .mbox_id = 1 },
> + { .recv_opt = 0, .msg_size = 2, .ipi_id = 10, .mbox_id = 1 },
> +
> + { .recv_opt = 0, .msg_size = 18, .ipi_id = 12, .mbox_id = 2 },
> + { .recv_opt = 0, .msg_size = 1, .ipi_id = 5, .mbox_id = 2 },
> + { .recv_opt = 1, .msg_size = 1, .ipi_id = 2, .mbox_id = 2 },
> +
> + { .recv_opt = 0, .msg_size = 2, .ipi_id = 34, .mbox_id = 3 },
> + { .recv_opt = 0, .msg_size = 2, .ipi_id = 14, .mbox_id = 3 },
> +
> + { .recv_opt = 0, .msg_size = 1, .ipi_id = 26, .mbox_id = 4 },
> + { .recv_opt = 1, .msg_size = 1, .ipi_id = 20, .mbox_id = 4 }
> + },
> + .recv_count = ARRAY_SIZE(recv_data),
> + .send_count = ARRAY_SIZE(send_data),
> +};
> +
> +static int vcp_ipi_mbox_init(struct mtk_vcp_device *vcp)
> +{
> + struct mtk_vcp_ipc *vcp_ipc;
> + struct platform_device *pdev;
> + int ret;
> +
> + pdev = platform_device_register_data(vcp->dev, "mtk-vcp-ipc",
> + PLATFORM_DEVID_NONE, &ipc_table,
> + sizeof(ipc_table));
> + if (IS_ERR(pdev)) {
> + ret = PTR_ERR(pdev);
> + dev_err(vcp->dev, "failed to create mtk-vcp-ipc device\n");
> + return ret;
> + }
> +
> + vcp_ipc = dev_get_drvdata(&pdev->dev);
> + if (!vcp_ipc) {
> + ret = -EPROBE_DEFER;
> + dev_err(vcp->dev, "failed to get drvdata\n");
> + return ret;
> + }
> +
> + ret = mtk_vcp_ipc_device_register(vcp->ipi_dev, VCP_IPI_COUNT, vcp_ipc);
> + if (ret) {
> + dev_err(vcp->dev, "ipi_dev_register failed, ret %d\n", ret);
> + return ret;
> + }
> +
> + return ret;
> +}
> +
> +static int vcp_multi_core_init(struct platform_device *pdev,
> + struct mtk_vcp_of_cluster *vcp_cluster,
> + enum vcp_core_id core_id)
> +{
> + int ret;
> +
> + ret = of_property_read_u32(pdev->dev.of_node, "twohart",
> + &vcp_cluster->twohart[core_id]);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to twohart property\n");
> + return ret;
> + }
> + ret = of_property_read_u32(pdev->dev.of_node, "sram-offset",
> + &vcp_cluster->sram_offset[core_id]);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to sram-offset property\n");
> + return ret;
> + }
> +
> + return ret;
> +}
> +
> +static bool vcp_is_single_core(struct platform_device *pdev,
> + struct mtk_vcp_of_cluster *vcp_cluster)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev_of_node(dev);
> + struct device_node *child;
> + u32 num_cores = 0;
> +
> + for_each_available_child_of_node(np, child)
> + num_cores++;
> + vcp_cluster->core_nums = num_cores;
> +
> + return num_cores < VCP_CORE_TOTAL ? true : false;
> +}
> +
> +static int vcp_add_single_core(struct platform_device *pdev,
> + struct mtk_vcp_of_cluster *vcp_cluster)
> +{
> + return 0;
> +}
> +
> +static int vcp_add_multi_core(struct platform_device *pdev,
> + struct mtk_vcp_of_cluster *vcp_cluster)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev_of_node(dev);
> + struct device_node *child;
> + struct platform_device *cpdev;
> + struct mtk_vcp_device *vcp;
> + struct rproc *rproc;
> + const struct mtk_vcp_of_data *vcp_of_data;
> + int ret = 0;
> +
> + vcp_of_data = of_device_get_match_data(dev);
> + rproc = devm_rproc_alloc(dev, np->name, &mtk_vcp_ops,
> + vcp_of_data->platdata.fw_name,
> + sizeof(struct mtk_vcp_device));
> + if (!rproc)
> + return dev_err_probe(dev, -ENOMEM, "unable to allocate remoteproc\n");
This does not look right. Allocation failures should not result in printks.
...
> +
> + for_each_available_child_of_node(np, child) {
> + if (of_device_is_compatible(child, "mediatek,vcp-core")) {
> + cpdev = of_find_device_by_node(child);
> + if (!cpdev) {
> + ret = -ENODEV;
> + dev_err(dev, "Not found platform device for core\n");
You leak np, use scoped.
> + return ret;
> + }
> + ret = vcp_multi_core_init(cpdev, vcp_cluster, VCP_ID);
> + } else if (of_device_is_compatible(child, "mediatek,mmup-core")) {
> + cpdev = of_find_device_by_node(child);
> + if (!cpdev) {
> + ret = -ENODEV;
> + dev_err(dev, "Not found platform device for core\n");
> + return ret;
Same problems.
> + }
> + ret = vcp_multi_core_init(cpdev, vcp_cluster, MMUP_ID);
> + }
> + }
..
> +static struct platform_driver mtk_vcp_device = {
> + .probe = vcp_device_probe,
> + .remove = vcp_device_remove,
> + .shutdown = vcp_device_shutdown,
> + .driver = {
> + .name = "mtk-vcp",
> + .owner = THIS_MODULE,
Clean your driver from all 10-yo code, before you upstream... Or just
start from recent driver so you won't repeat the same mistakes/issues we
already fixed.
> + .of_match_table = of_match_ptr(vcp_of_ids),
Same, drop of_match_ptr.
Best regards,
Krzysztof
Powered by blists - more mailing lists