| 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: <2aff75e4cf76cd7fe9f2e45ce174e5380363e501.camel@mediatek.com>
Date: Sun, 6 Apr 2025 07:29:53 +0000
From: Xiangzhi Tang (唐相志)
<Xiangzhi.Tang@...iatek.com>
To: "krzk@...nel.org" <krzk@...nel.org>, "conor+dt@...nel.org"
<conor+dt@...nel.org>, "robh@...nel.org" <robh@...nel.org>,
"matthias.bgg@...il.com" <matthias.bgg@...il.com>, "krzk+dt@...nel.org"
<krzk+dt@...nel.org>, "andersson@...nel.org" <andersson@...nel.org>,
AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
"mathieu.poirier@...aro.org" <mathieu.poirier@...aro.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
"linux-remoteproc@...r.kernel.org" <linux-remoteproc@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
Hailong Fan (范海龙) <Hailong.Fan@...iatek.com>,
Project_Global_Chrome_Upstream_Group
<Project_Global_Chrome_Upstream_Group@...iatek.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
Jjian Zhou (周建) <Jjian.Zhou@...iatek.com>
Subject: Re: [PATCH 2/2] remoterpoc: mediatek: vcp: Add vcp remoteproc driver
On Wed, 2025-04-02 at 13:16 +0200, Krzysztof Kozlowski wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>
>
> 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?
> Thanks comment, this devices is designed for dua-core.
>
>
> ...
>
> > +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.
> Thanks commnet, it is have two driver, I will fix it on v1 version.
>
>
> ...
>
> > +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?
> Thanks comment, I will fix it on v1 version
>
> > + { .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.
> Thanks comment, I will check thie err handler agnin
>
>
> ...
>
> > +
> > + 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.
> I'm not quite able to understand your meaning, Could you please
explain it for me? Thanks very much.
>
> > + 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.
> Thanks comment, I will fix it on v1 version
>
> > + .of_match_table = of_match_ptr(vcp_of_ids),
>
> Same, drop of_match_ptr.
>
>
> Best regards,
> Krzysztof
Powered by blists - more mailing lists