Re: [PATCH v3 2/3] mailbox: mediatek: add support for adsp mailbox controller

Re: [PATCH v3 2/3] mailbox: mediatek: add support for adsp mailbox controller

[Tzung-Bi Shih]

On Wed, Nov 24, 2021 at 04:45:13PM +0800, allen-kh.cheng wrote:
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index c9fc06c7e685..fc99d9fc80af 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -236,6 +236,13 @@ config MTK_CMDQ_MBOX
>  	  critical time limitation, such as updating display configuration
>  	  during the vblank.
>  
> +config MTK_ADSP_IPC_MBOX
> +	tristate "MediaTek ADSP Mailbox Controller"
> +	depends on ARCH_MEDIATEK || COMPILE_TEST
> +	help
> +	  Say yes here to add support for MediaTek ADSP IPC mailbox controller
> +	  driver. It is used to send short messages between processors with dsp.

Although the file didn't maintain alphabetical order, to be neat, moving MTK_ADSP_IPC_MBOX before MTK_CMDQ_MBOX makes more sense.

> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index c2089f04887e..479a9ae56d5e 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -51,6 +51,8 @@ obj-$(CONFIG_STM32_IPCC) 	+= stm32-ipcc.o
>  
>  obj-$(CONFIG_MTK_CMDQ_MBOX)	+= mtk-cmdq-mailbox.o
>  
> +obj-$(CONFIG_MTK_ADSP_IPC_MBOX)	+= mtk-adsp-mailbox.o
> +

Ditto.  Move CONFIG_MTK_ADSP_IPC_MBOX before CONFIG_MTK_CMDQ_MBOX without blank line separation.

> diff --git a/drivers/mailbox/mtk-adsp-mailbox.c b/drivers/mailbox/mtk-adsp-mailbox.c
[...]
> +static irqreturn_t mtk_adsp_ipc_irq_handler(int irq, void *data)
> +{
> +	struct mbox_chan *ch = (struct mbox_chan *)data;

The cast should be able to remove.

> +static irqreturn_t mtk_adsp_ipc_handler(int irq, void *data)
> +{
> +	struct mbox_chan *ch = (struct mbox_chan *)data;

The cast should be able to remove.

> +static int mtk_adsp_mbox_startup(struct mbox_chan *chan)
> +{
> +	struct adsp_mbox_ch_info *ch_info = chan->con_priv;
> +	void __iomem *reg = ch_info->va_reg;
> +
> +	/* Clear DSP mbox command */
> +	writel(0xFFFFFFFF, reg + MTK_ADSP_MBOX_IN_CMD_CLR);
> +	writel(0xFFFFFFFF, reg + MTK_ADSP_MBOX_OUT_CMD_CLR);
> +
> +	return 0;
> +}
> +
> +static void mtk_adsp_mbox_shutdown(struct mbox_chan *chan)
> +{
> +	chan->con_priv = NULL;
> +}

Shall mtk_adsp_mbox_shutdown() also clear DSP mbox?  I.e.:
writel(0xFFFFFFFF, reg + MTK_ADSP_MBOX_IN_CMD_CLR);
writel(0xFFFFFFFF, reg + MTK_ADSP_MBOX_OUT_CMD_CLR);

> +static int mtk_adsp_mbox_send_data(struct mbox_chan *chan, void *data)
> +{
> +	struct adsp_mbox_ch_info *ch_info = chan->con_priv;
> +	void __iomem *reg = ch_info->va_reg;
> +
> +	spin_lock(&ch_info->lock);
> +	writel(ch_info->ipc_op_val, reg + MTK_ADSP_MBOX_IN_CMD);
> +	spin_unlock(&ch_info->lock);

Why does it need the lock?

Is the write to MTK_ADSP_MBOX_IN_CMD a synchronous operation?
- If yes, I failed to understand why does it need the lock.  Every calls to mtk_adsp_mbox_send_data() should wait for the data transfer completion.
- If no, I also failed to understand why.  mtk_adsp_mbox_send_data() has no way to be aware of the transfer completion.  Would expect a loop for checking the completion for the case.

> +static bool mtk_adsp_mbox_last_tx_done(struct mbox_chan *chan)
> +{
> +	struct adsp_mbox_ch_info *ch_info = chan->con_priv;
> +	void __iomem *reg = ch_info->va_reg;
> +	u32 op = readl(reg + MTK_ADSP_MBOX_IN_CMD);
> +
> +	return (op == 0) ? true : false;

To be concise, return readl(...) == 0;

> +static int mtk_adsp_mbox_probe(struct platform_device *pdev)
> +{
[...]
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(dev, "no adsp mbox register resource\n");
> +		return -ENXIO;
> +	}
> +
> +	size = resource_size(res);
> +	priv->va_mboxreg = devm_ioremap(dev, (phys_addr_t)res->start, size);
> +	if (IS_ERR(priv->va_mboxreg))
> +		return PTR_ERR(priv->va_mboxreg);

Use devm_platform_ioremap_resource(), it should be equivalent.

> +	/* set adsp mbox channel info */
> +	ch_info = devm_kzalloc(mbox->dev, sizeof(*ch_info), GFP_KERNEL);

To be neat, use dev instead of mbox->dev.

> +	ret = devm_mbox_controller_register(dev, &priv->mbox);
> +	if (ret < 0)
> +		dev_err(dev, "error: failed to register mailbox:%d\n", ret);
> +
> +	return ret;

To be concise, return devm_mbox_controller_register(...);

Re: [PATCH v3 2/3] mailbox: mediatek: add support for adsp mailbox controller

[Tzung-Bi Shih]

On Thu, Nov 25, 2021 at 09:51:27AM +0800, allen-kh.cheng wrote:
> On Wed, 2021-11-24 at 18:25 +0800, Tzung-Bi Shih wrote:
> > > On Wed, Nov 24, 2021 at 04:45:13PM +0800, allen-kh.cheng wrote:
> > > > > +static int mtk_adsp_mbox_send_data(struct mbox_chan *chan,
> > > void
> > > > > *data)
> > > > > +{
> > > > > +  struct adsp_mbox_ch_info *ch_info = chan->con_priv;
> > > > > +  void __iomem *reg = ch_info->va_reg;
> > > > > +
> > > > > +  spin_lock(&ch_info->lock);
> > > > > +  writel(ch_info->ipc_op_val, reg + MTK_ADSP_MBOX_IN_CMD);
> > > > > +  spin_unlock(&ch_info->lock);
> > 
> > >
> > > Why does it need the lock?
> > >
> > > Is the write to MTK_ADSP_MBOX_IN_CMD a synchronous operation?
> > > - If yes, I failed to understand why does it need the lock.  Every
> > > calls to mtk_adsp_mbox_send_data() should wait for the data
> > transfer
> > > completion.
> > > - If no, I also failed to understand
> > why.  mtk_adsp_mbox_send_data()
> > > has no way to be aware of the transfer completion.  Would expect a
> > > loop for checking the completion for the case.
> > >
> >  
> 
> In ADSP MBOX IPC flow,
>  
> Host would call mbox send data when the shared data transfer completed.
>  
> (mtk_adsp_mbox_send_data will notice client using MTK_ADSP_MBOX_IN_CMD)
>  
> It’s more like a signal.
>  
> In general case,
>  
> There may be some hosts use the same mbox channel.
>  
> I think it’s better using lock to protect access to
> MTK_ADSP_MBOX_IN_CMD registers

I still failed to understand.  What if 2 hosts notify the same client by writing MTK_ADSP_MBOX_IN_CMD at the same time?

Re: [PATCH v3 3/3] firmware: mediatek: add adsp ipc protocol interface

[Tzung-Bi Shih]

On Wed, Nov 24, 2021 at 04:45:14PM +0800, allen-kh.cheng wrote:
>  drivers/firmware/Kconfig                      |   1 +
>  drivers/firmware/Makefile                     |   1 +
>  drivers/firmware/mediatek/Kconfig             |  10 ++
>  drivers/firmware/mediatek/Makefile            |   2 +
>  drivers/firmware/mediatek/mtk-adsp-ipc.c      | 130 ++++++++++++++++++
>  .../linux/firmware/mediatek/mtk-adsp-ipc.h    |  72 ++++++++++
>  6 files changed, 216 insertions(+)
>  create mode 100644 drivers/firmware/mediatek/Kconfig
>  create mode 100644 drivers/firmware/mediatek/Makefile
>  create mode 100644 drivers/firmware/mediatek/mtk-adsp-ipc.c
>  create mode 100644 include/linux/firmware/mediatek/mtk-adsp-ipc.h

The patch should move before the 2nd patch in the series as the 2nd patch uses mtk-adsp-ipc.h.

> diff --git a/drivers/firmware/mediatek/mtk-adsp-ipc.c b/drivers/firmware/mediatek/mtk-adsp-ipc.c
[...]
> +int adsp_ipc_send(struct mtk_adsp_ipc *ipc, unsigned int idx, uint32_t op)
> +{
> +	struct mtk_adsp_chan *dsp_chan = &ipc->chans[idx];
> +	struct adsp_mbox_ch_info *ch_info = dsp_chan->ch->con_priv;
> +	int ret;
> +
> +	if (idx >= MTK_ADSP_MBOX_NUM)
> +		return -EINVAL;

If idx >= MTK_ADSP_MBOX_NUM, the invalid memory access has occurred at beginning of the function.

> +static int mtk_adsp_ipc_probe(struct platform_device *pdev)
> +{
[...]
> +	device_set_of_node_from_dev(&pdev->dev, pdev->dev.parent);

Why does it need to call device_set_of_node_from_dev()?

> +	for (i = 0; i < MTK_ADSP_MBOX_NUM; i++) {
> +		chan_name = kasprintf(GFP_KERNEL, "mbox%d", i);
> +		if (!chan_name)
> +			return -ENOMEM;
> +
> +		dsp_chan = &dsp_ipc->chans[i];
> +		cl = &dsp_chan->cl;
> +		cl->dev = dev->parent;
> +		cl->tx_block = false;
> +		cl->knows_txdone = false;
> +		cl->tx_prepare = NULL;
> +		cl->rx_callback = adsp_ipc_recv;
> +
> +		dsp_chan->ipc = dsp_ipc;
> +		dsp_chan->idx = i;
> +		dsp_chan->ch = mbox_request_channel_byname(cl, chan_name);
> +		if (IS_ERR(dsp_chan->ch)) {
> +			ret = PTR_ERR(dsp_chan->ch);
> +			if (ret != -EPROBE_DEFER)
> +				dev_err(dev, "Failed to request mbox chan %d ret %d\n",
> +					i, ret);

If ret == -EPROBE_DEFER, wouldn't it need to return -EPROBE_DEFER?  It doesn't retry later if -EPROBE_DEFER.

Re: [PATCH v3 3/3] firmware: mediatek: add adsp ipc protocol interface

[Tzung-Bi Shih]

On Thu, Nov 25, 2021 at 09:47:22AM +0800, allen-kh.cheng wrote:
> On Wed, 2021-11-24 at 18:25 +0800, Tzung-Bi Shih wrote:
> > On Wed, Nov 24, 2021 at 04:45:14PM +0800, allen-kh.cheng wrote:
> > > +	for (i = 0; i < MTK_ADSP_MBOX_NUM; i++) {
> > > +		chan_name = kasprintf(GFP_KERNEL, "mbox%d", i);
> > > +		if (!chan_name)
> > > +			return -ENOMEM;
> > > +
> > > +		dsp_chan = &dsp_ipc->chans[i];
> > > +		cl = &dsp_chan->cl;
> > > +		cl->dev = dev->parent;
> > > +		cl->tx_block = false;
> > > +		cl->knows_txdone = false;
> > > +		cl->tx_prepare = NULL;
> > > +		cl->rx_callback = adsp_ipc_recv;
> > > +
> > > +		dsp_chan->ipc = dsp_ipc;
> > > +		dsp_chan->idx = i;
> > > +		dsp_chan->ch = mbox_request_channel_byname(cl,
> > > chan_name);
> > > +		if (IS_ERR(dsp_chan->ch)) {
> > > +			ret = PTR_ERR(dsp_chan->ch);
> > > +			if (ret != -EPROBE_DEFER)
> > > +				dev_err(dev, "Failed to request mbox
> > > chan %d ret %d\n",
> > > +					i, ret);
> > 
> > If ret == -EPROBE_DEFER, wouldn't it need to return
> > -EPROBE_DEFER?  It doesn't retry later if -EPROBE_DEFER.
> 
> If ret != -EPROBE_DEFER, it will show a error message then goto out.
> 
> If ret == -EPROBE_DEFER, probe funcation also will goto out. 
> 
> 
> Both of them will return ret. will not go to next round.

I see.  I misunderstood.