Re: [PATCH RFC v3 2/3] firmware: mediatek: Add vcp ipc protocol interface

[Krzysztof Kozlowski <krzk@kernel.org>]

On 17/03/2025 12:03, Jjian Zhou wrote:
> Some of mediatek processors contain the Risc-V coprocessor.
> 
> The communication between Host CPU and vcp firmware is
> taking place using a shared memory area for message passing.
> 
> VCP IPC protocol offers (send/recv) interfaces using
> mediatek-mailbox APIs.
> 
> Signed-off-by: Jjian Zhou <jjian.zhou@mediatek.com>
> ---
>  drivers/firmware/Kconfig                      |   9 +
>  drivers/firmware/Makefile                     |   1 +
>  drivers/firmware/mtk-vcp-ipc.c                | 481 ++++++++++++++++++
>  include/linux/firmware/mediatek/mtk-vcp-ipc.h | 151 ++++++
>  4 files changed, 642 insertions(+)
>  create mode 100644 drivers/firmware/mtk-vcp-ipc.c
>  create mode 100644 include/linux/firmware/mediatek/mtk-vcp-ipc.h

Do not send new versions while previous discussion is still going.

You still did not respond to previous feedback and you already sent two
versions repeating the same mistake.

> 
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 37e43f287e78..98c4ff667836 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -179,6 +179,15 @@ config MTK_ADSP_IPC
>  	  ADSP exists on some mtk processors.
>  	  Client might use shared memory to exchange information with ADSP.
>  

..

> +
> +/*
> + * mtk_vcp_ipc_send - send ipc command to MTK VCP
> + *
> + * @ipidev: VCP struct mtk_ipi_device handle
> + * @id: id of the feature IPI
> + * @data: message address
> + * @len: message length
> + *
> + * Return: Zero for success from mbox_send_message
> + *         negative value for error
> + */
> +int mtk_vcp_ipc_send(struct mtk_ipi_device *ipidev, u32 id, void *data, u32 len)
> +{
> +	struct device *dev;
> +	struct mtk_mbox_info *minfo;
> +	struct mtk_ipi_chan_table *table;
> +	struct mtk_vcp_ipc *vcp_ipc;
> +	int ret;
> +
> +	if (!ipidev || !ipidev->ipi_inited || !data)
> +		return IPI_UNAVAILABLE;
> +	vcp_ipc = ipidev->vcp_ipc;
> +	if (!vcp_ipc)
> +		return IPI_UNAVAILABLE;
> +
> +	table = ipidev->table;
> +	dev = ipidev->vcp_ipc->dev;
> +	minfo = &ipidev->vcp_ipc->info_table[table[id].mbox];
> +	if (!minfo) {
> +		dev_err(dev, "%s IPI%d minfo is invalid.\n", ipidev->name, id);
> +		return IPI_UNAVAILABLE;
> +	}
> +
> +	if (len > table[id].msg_size)
> +		return IPI_MSG_TOO_BIG;
> +	else if (!len)
> +		len = table[id].msg_size;
> +
> +	mutex_lock(&table[id].mutex_send);
> +
> +	minfo->ipi_info.msg = data;
> +	minfo->ipi_info.len = len;
> +	minfo->ipi_info.id = id;
> +	minfo->ipi_info.index = table[id].send_index;
> +	minfo->ipi_info.slot_ofs = table[id].send_ofs * MBOX_SLOT_SIZE;
> +
> +	ret = mbox_send_message(minfo->ch, &minfo->ipi_info);
> +	mutex_unlock(&table[id].mutex_send);
> +	if (ret < 0) {
> +		dev_err(dev, "%s IPI%d send failed.\n", ipidev->name, id);
> +		return IPI_MBOX_ERR;
> +	}
> +
> +	return IPI_ACTION_DONE;
> +}
> +EXPORT_SYMBOL(mtk_vcp_ipc_send);

Drop export - no users

Anyway, every export must be GPL.

> +
> +/*
> + * mtk_vcp_ipc_send_compl - send ipc command to MTK VCP
> + *
> + * @ipidev: VCP struct mtk_ipi_device handle
> + * @id: id of the feature IPI
> + * @data: message address
> + * @len: message length
> + * @timeout_ms:
> + *
> + * Return: Zero for success from mbox_send_message
> + *         negative value for error
> + */
> +int mtk_vcp_ipc_send_compl(struct mtk_ipi_device *ipidev, u32 id,
> +			   void *data, u32 len, u32 timeout_ms)
> +{
> +	struct device *dev;
> +	struct mtk_mbox_info *minfo;
> +	struct mtk_ipi_chan_table *table;
> +	struct mtk_vcp_ipc *vcp_ipc;
> +	int ret;
> +
> +	if (!ipidev || !ipidev->ipi_inited || !data)
> +		return IPI_UNAVAILABLE;
> +	vcp_ipc = ipidev->vcp_ipc;
> +	if (!vcp_ipc)
> +		return IPI_UNAVAILABLE;
> +
> +	table = ipidev->table;
> +	dev = ipidev->vcp_ipc->dev;
> +	minfo = &ipidev->vcp_ipc->info_table[table[id].mbox];
> +	if (!minfo) {
> +		dev_err(dev, "%s IPI%d minfo is invalid.\n", ipidev->name, id);
> +		return IPI_UNAVAILABLE;
> +	}
> +
> +	if (len > table[id].msg_size)
> +		return IPI_MSG_TOO_BIG;
> +	else if (!len)
> +		len = table[id].msg_size;
> +
> +	mutex_lock(&table[id].mutex_send);
> +
> +	minfo->ipi_info.msg = data;
> +	minfo->ipi_info.len = len;
> +	minfo->ipi_info.id = id;
> +	minfo->ipi_info.index = table[id].send_index;
> +	minfo->ipi_info.slot_ofs = table[id].send_ofs * MBOX_SLOT_SIZE;
> +
> +	atomic_inc(&table[id].holder);
> +
> +	ret = mbox_send_message(minfo->ch, &minfo->ipi_info);
> +	if (ret < 0) {
> +		atomic_set(&table[id].holder, 0);
> +		mutex_unlock(&table[id].mutex_send);
> +		dev_err(dev, "%s IPI%d send failed.\n", ipidev->name, id);
> +		return IPI_MBOX_ERR;
> +	}
> +
> +	/* wait for completion */
> +	ret = wait_for_completion_timeout(&table[id].notify,
> +					  msecs_to_jiffies(timeout_ms));
> +	atomic_set(&table[id].holder, 0);
> +	if (ret > 0)
> +		ret = IPI_ACTION_DONE;
> +
> +	mutex_unlock(&table[id].mutex_send);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(mtk_vcp_ipc_send_compl);

NAK, no users.

> +
> +int mtk_vcp_mbox_ipc_register(struct mtk_ipi_device *ipidev, int id,
> +			      mbox_pin_cb_t cb, void *prdata, void *msg)
> +{
> +	if (!ipidev || !ipidev->ipi_inited)
> +		return IPI_DEV_ILLEGAL;
> +	if (!msg)
> +		return IPI_NO_MSGBUF;
> +
> +	if (ipidev->table[id].pin_buf)
> +		return IPI_ALREADY_USED;
> +	ipidev->table[id].mbox_pin_cb = cb;
> +	ipidev->table[id].pin_buf = msg;
> +	ipidev->table[id].prdata = prdata;
> +
> +	return IPI_ACTION_DONE;
> +}
> +EXPORT_SYMBOL(mtk_vcp_mbox_ipc_register);

NAK, no users.


> +
> +int mtk_vcp_mbox_ipc_unregister(struct mtk_ipi_device *ipidev, int id)
> +{
> +	if (!ipidev || !ipidev->ipi_inited)
> +		return IPI_DEV_ILLEGAL;
> +
> +	/* Drop the ipi and reset the record */
> +	complete(&ipidev->table[id].notify);
> +
> +	ipidev->table[id].mbox_pin_cb = NULL;
> +	ipidev->table[id].pin_buf = NULL;
> +	ipidev->table[id].prdata = NULL;
> +
> +	return IPI_ACTION_DONE;
> +}
> +EXPORT_SYMBOL(mtk_vcp_mbox_ipc_unregister);

NAK, no users.


> +
> +static void mtk_fill_in_entry(struct mtk_ipi_chan_table *entry, const u32 ipi_id,
> +			      const struct mtk_mbox_table *mbdev)
> +{
> +	const struct mtk_mbox_send_table *mbox_send = mbdev->send_table;
> +	u32 index;
> +
> +	for (index = 0; index < mbdev->send_count; index++) {
> +		if (ipi_id != mbox_send[index].ipi_id)
> +			continue;
> +
> +		entry->send_ofs = mbox_send[index].offset;
> +		entry->send_index = mbox_send[index].pin_index;
> +		entry->msg_size = mbox_send[index].msg_size;
> +		entry->mbox = mbox_send[index].mbox_id;
> +		return;
> +	}
> +
> +	entry->mbox = -ENOENT;
> +}
> +
> +int mtk_vcp_ipc_device_register(struct mtk_ipi_device *ipidev,
> +				u32 ipi_chan_count, struct mtk_vcp_ipc *vcp_ipc)
> +{
> +	struct mtk_ipi_chan_table *ipi_chan_table;
> +	struct mtk_mbox_table *mbdev;
> +	u32 index;
> +
> +	if (!vcp_ipc || !ipidev)
> +		return -EINVAL;
> +
> +	ipi_chan_table = kcalloc(ipi_chan_count,
> +				 sizeof(struct mtk_ipi_chan_table), GFP_KERNEL);
> +	if (!ipi_chan_table)
> +		return -ENOMEM;
> +
> +	mbdev = vcp_ipc->mbdev;
> +	vcp_ipc->ipi_priv = (void *)ipidev;
> +	ipidev->table = ipi_chan_table;
> +	ipidev->vcp_ipc = vcp_ipc;
> +
> +	for (index = 0; index < ipi_chan_count; index++) {
> +		atomic_set(&ipi_chan_table[index].holder, 0);
> +		mutex_init(&ipi_chan_table[index].mutex_send);
> +		init_completion(&ipi_chan_table[index].notify);
> +		mtk_fill_in_entry(&ipi_chan_table[index], index, mbdev);
> +	}
> +
> +	ipidev->ipi_inited = 1;
> +
> +	dev_dbg(vcp_ipc->dev, "%s (with %d IPI) has registered.\n",
> +		ipidev->name, ipi_chan_count);
> +
> +	return IPI_ACTION_DONE;
> +}
> +EXPORT_SYMBOL(mtk_vcp_ipc_device_register);

NAK, no users.


> +
> +static int setup_mbox_table(struct mtk_mbox_table *mbdev, u32 mbox)
> +{
> +	struct mtk_mbox_send_table *mbox_send = &mbdev->send_table[0];
> +	struct mtk_mbox_recv_table *mbox_recv = &mbdev->recv_table[0];
> +	u32 i, last_ofs = 0, last_idx = 0, last_slot = 0, last_sz = 0;
> +
> +	for (i = 0; i < mbdev->send_count; i++) {
> +		if (mbox == mbox_send[i].mbox_id) {
> +			mbox_send[i].offset = last_ofs + last_slot;
> +			mbox_send[i].pin_index = last_idx + last_sz;
> +			last_idx = mbox_send[i].pin_index;
> +			last_sz = DIV_ROUND_UP(mbox_send[i].msg_size, MBOX_SLOT_ALIGN);
> +			last_ofs = last_sz * MBOX_SLOT_ALIGN;
> +			last_slot = last_idx * MBOX_SLOT_ALIGN;
> +		} else if (mbox < mbox_send[i].mbox_id) {
> +			/* no need to search the rest id */
> +			break;
> +		}
> +	}
> +
> +	for (i = 0; i < mbdev->recv_count; i++) {
> +		if (mbox == mbox_recv[i].mbox_id) {
> +			mbox_recv[i].offset = last_ofs + last_slot;
> +			mbox_recv[i].pin_index = last_idx + last_sz;
> +			last_idx = mbox_recv[i].pin_index;
> +			last_sz = DIV_ROUND_UP(mbox_recv[i].msg_size, MBOX_SLOT_ALIGN);
> +			last_ofs = last_sz * MBOX_SLOT_ALIGN;
> +			last_slot = last_idx * MBOX_SLOT_ALIGN;
> +		} else if (mbox < mbox_recv[i].mbox_id) {
> +			/* no need to search the rest id */
> +			break;
> +		}
> +	}
> +
> +	if (last_idx > MBOX_MAX_PIN || (last_ofs + last_slot) > MAX_SLOT_NUM)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int mtk_vcp_ipc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct mtk_vcp_ipc *vcp_ipc;
> +	struct mbox_client *cl;
> +	struct mtk_mbox_info *minfo;
> +	int ret;
> +	u32 mbox, i;
> +	struct mtk_mbox_table *mbox_data = dev_get_platdata(dev);
> +
> +	device_set_of_node_from_dev(&pdev->dev, pdev->dev.parent);
> +
> +	vcp_ipc = devm_kzalloc(dev, sizeof(*vcp_ipc), GFP_KERNEL);
> +	if (!vcp_ipc)
> +		return -ENOMEM;
> +
> +	if (!mbox_data) {

Check goes immediately after declaration. I doubt it is useful in the
first place as this cannot even happen...


> +		dev_err(dev, "No platform data available\n");

No, drop. Cannot happen or fix your drivers. Who provides the platdata here?

> +		return -EINVAL;
> +	}
> +	vcp_ipc->mbdev = mbox_data;
> +
> +	/* alloc and init mmup_mbox_info */
> +	vcp_ipc->info_table = vzalloc(sizeof(*vcp_ipc->info_table) * VCP_MBOX_NUM);
> +	if (!vcp_ipc->info_table)
> +		return -ENOMEM;
> +
> +	/* create mbox dev */
> +	for (mbox = 0; mbox < VCP_MBOX_NUM; mbox++) {
> +		minfo = &vcp_ipc->info_table[mbox];
> +		minfo->mbox_id = mbox;
> +		minfo->vcp_ipc = vcp_ipc;
> +		spin_lock_init(&minfo->mbox_lock);
> +
> +		ret = setup_mbox_table(vcp_ipc->mbdev, mbox);
> +		if (ret)
> +			return ret;
> +
> +		cl = &minfo->cl;
> +		cl->dev = &pdev->dev;
> +		cl->tx_block = false;
> +		cl->knows_txdone = false;
> +		cl->tx_prepare = NULL;
> +		cl->rx_callback = mtk_vcp_ipc_recv;
> +		minfo->ch = mbox_request_channel_byname(cl, mbox_names[mbox]);
> +		if (IS_ERR(minfo->ch)) {
> +			ret = PTR_ERR(minfo->ch);
> +			if (ret != -EPROBE_DEFER)
> +				dev_err(dev, "Failed to request mbox channel %s ret %d\n",
> +					mbox_names[mbox], ret);

Do not open code dev_err_probe.

> +
> +			for (i = 0; i < mbox; i++) {
> +				minfo = &vcp_ipc->info_table[i];
> +				mbox_free_channel(minfo->ch);
> +			}
> +
> +			vfree(vcp_ipc->info_table);
> +			return ret;
> +		}
> +	}
> +
> +	vcp_ipc->dev = dev;
> +	dev_set_drvdata(dev, vcp_ipc);
> +	dev_dbg(dev, "MTK VCP IPC initialized\n");

No, drop


Best regards,
Krzysztof