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 controllerdriver. 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(...);