* Re: [PATCH v3 2/3] mailbox: mediatek: add support for adsp mailbox controller
[not found] ` <20211124084514.28002-3-allen-kh.cheng@mediatek.com>
@ 2021-11-24 10:25 ` Tzung-Bi Shih
[not found] ` <f3642bcd031fbfd461b5efae1eba4816cc4856b2.camel@mediatek.com>
0 siblings, 1 reply; 4+ messages in thread
From: Tzung-Bi Shih @ 2021-11-24 10:25 UTC (permalink / raw)
To: allen-kh.cheng
Cc: Jassi Brar, Rob Herring, Matthias Brugger, devicetree,
Linux-ALSA, Kai Vehmanen, Liam Girdwood, cujomalainey,
linux-kernel, Takashi Iwai, Ranjani Sridharan,
Pierre-Louis Bossart, Project_Global_Chrome_Upstream_Group,
Mark Brown, linux-mediatek, Daniel Baluta, linux-arm-kernel,
sound-open-firmware
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(...);
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3 3/3] firmware: mediatek: add adsp ipc protocol interface
[not found] ` <20211124084514.28002-4-allen-kh.cheng@mediatek.com>
@ 2021-11-24 10:25 ` Tzung-Bi Shih
[not found] ` <43976e8aab2e3055195d8e9c2f466a804e4d2ba7.camel@mediatek.com>
0 siblings, 1 reply; 4+ messages in thread
From: Tzung-Bi Shih @ 2021-11-24 10:25 UTC (permalink / raw)
To: allen-kh.cheng
Cc: Jassi Brar, Rob Herring, Matthias Brugger, Linux-ALSA,
cujomalainey, Pierre-Louis Bossart, Liam Girdwood,
Ranjani Sridharan, Kai Vehmanen, Daniel Baluta, Mark Brown,
Jaroslav Kysela, Takashi Iwai,
Project_Global_Chrome_Upstream_Group, linux-kernel, devicetree,
linux-arm-kernel, linux-mediatek, sound-open-firmware
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3 2/3] mailbox: mediatek: add support for adsp mailbox controller
[not found] ` <f3642bcd031fbfd461b5efae1eba4816cc4856b2.camel@mediatek.com>
@ 2021-11-25 6:23 ` Tzung-Bi Shih
0 siblings, 0 replies; 4+ messages in thread
From: Tzung-Bi Shih @ 2021-11-25 6:23 UTC (permalink / raw)
To: allen-kh.cheng
Cc: Jassi Brar, Rob Herring, Matthias Brugger, devicetree,
Linux-ALSA, Kai Vehmanen, Liam Girdwood, cujomalainey,
linux-kernel, Takashi Iwai, Ranjani Sridharan,
Pierre-Louis Bossart, Project_Global_Chrome_Upstream_Group,
Mark Brown, linux-mediatek, Daniel Baluta, linux-arm-kernel,
sound-open-firmware
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?
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3 3/3] firmware: mediatek: add adsp ipc protocol interface
[not found] ` <43976e8aab2e3055195d8e9c2f466a804e4d2ba7.camel@mediatek.com>
@ 2021-11-25 6:26 ` Tzung-Bi Shih
0 siblings, 0 replies; 4+ messages in thread
From: Tzung-Bi Shih @ 2021-11-25 6:26 UTC (permalink / raw)
To: allen-kh.cheng
Cc: Jassi Brar, Rob Herring, Matthias Brugger, Linux-ALSA,
cujomalainey, Pierre-Louis Bossart, Liam Girdwood,
Ranjani Sridharan, Kai Vehmanen, Daniel Baluta, Mark Brown,
Jaroslav Kysela, Takashi Iwai,
Project_Global_Chrome_Upstream_Group, linux-kernel, devicetree,
linux-arm-kernel, linux-mediatek, sound-open-firmware
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-11-25 6:31 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20211124084514.28002-1-allen-kh.cheng@mediatek.com>
[not found] ` <20211124084514.28002-3-allen-kh.cheng@mediatek.com>
2021-11-24 10:25 ` [PATCH v3 2/3] mailbox: mediatek: add support for adsp mailbox controller Tzung-Bi Shih
[not found] ` <f3642bcd031fbfd461b5efae1eba4816cc4856b2.camel@mediatek.com>
2021-11-25 6:23 ` Tzung-Bi Shih
[not found] ` <20211124084514.28002-4-allen-kh.cheng@mediatek.com>
2021-11-24 10:25 ` [PATCH v3 3/3] firmware: mediatek: add adsp ipc protocol interface Tzung-Bi Shih
[not found] ` <43976e8aab2e3055195d8e9c2f466a804e4d2ba7.camel@mediatek.com>
2021-11-25 6:26 ` Tzung-Bi Shih
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).