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

[allen-kh.cheng <allen-kh.cheng@mediatek.com>]

Hi Tzung-Bi,

Thanks for your suggestions.

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:
> >  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()?

The original design regards mt8195 sof of_node as a parent deivce of
mtk-adsp-ipc.

device_set_of_node_from_dev will set of_node_reuse flag to prevent
driver from attempting to claim any mbox ipc resources already claimed
by the sof dsp device.

> 
> > +	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.

Thanks,
Allen