Re: [PATCH 2/2] remoterpoc: mediatek: vcp: Add vcp remoteproc driver

[Krzysztof Kozlowski <krzk@kernel.org>]

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@mediatek.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@mediatek.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