* [PATCH v3 1/3] dt-bindings: mediatek: add adsp-mbox document
2021-11-24 8:45 [PATCH v3 0/3] This patches provide ADSP IPC support for MT8195 allen-kh.cheng
@ 2021-11-24 8:45 ` allen-kh.cheng
2021-11-24 8:45 ` [PATCH v3 2/3] mailbox: mediatek: add support for adsp mailbox controller allen-kh.cheng
2021-11-24 8:45 ` [PATCH v3 3/3] firmware: mediatek: add adsp ipc protocol interface allen-kh.cheng
2 siblings, 0 replies; 11+ messages in thread
From: allen-kh.cheng @ 2021-11-24 8:45 UTC (permalink / raw)
To: Jassi Brar, Rob Herring, Matthias Brugger
Cc: devicetree, Linux-ALSA, Allen-KH Cheng, Kai Vehmanen,
Liam Girdwood, cujomalainey, linux-kernel, Takashi Iwai,
Ranjani Sridharan, Pierre-Louis Bossart,
Project_Global_Chrome_Upstream_Group, tzungbi, Mark Brown,
linux-mediatek, Daniel Baluta, linux-arm-kernel,
sound-open-firmware
From: Allen-KH Cheng <Allen-KH.Cheng@mediatek.com>
This patch adds document for mediatek adsp mbox
Signed-off-by: Allen-KH Cheng <Allen-KH.Cheng@mediatek.com>
---
.../bindings/mailbox/mtk,adsp-mbox.yaml | 53 +++++++++++++++++++
1 file changed, 53 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mailbox/mtk,adsp-mbox.yaml
diff --git a/Documentation/devicetree/bindings/mailbox/mtk,adsp-mbox.yaml b/Documentation/devicetree/bindings/mailbox/mtk,adsp-mbox.yaml
new file mode 100644
index 000000000000..f365182fa598
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/mtk,adsp-mbox.yaml
@@ -0,0 +1,53 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mailbox/mtk,adsp-mbox.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Mediatek ADSP mailbox
+
+maintainers:
+ - Allen-KH Cheng <Allen-KH.Cheng@mediatek.com>
+
+description: |
+ The MTK ADSP mailbox Inter-Processor Communication (IPC) enables the SoC
+ to ommunicate with ADSP by passing messages through two mailbox channels.
+ The MTK ADSP mailbox IPC also provides the ability for one processor to
+ signal the other processor using interrupts.
+
+properties:
+ compatible:
+ items:
+ - const: mediatek,mt8195-adsp-mbox
+
+ "#mbox-cells":
+ const: 0
+
+ reg:
+ description:
+ Physical address base for dsp mbox base registers.
+
+ interrupts:
+ description:
+ adsp mbox interrupt
+
+required:
+ - compatible
+ - "#mbox-cells"
+ - reg
+ - interrupts
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ adsp_mailbox0:adsp_mailbox0@10816000 {
+ compatible = "mediatek,mt8195-adsp-mbox";
+ #mbox-cells = <0>;
+ reg = <0x10816000 0x1000>;
+ interrupts = <GIC_SPI 702 IRQ_TYPE_LEVEL_HIGH 0>;
+ };
+
--
2.18.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v3 2/3] mailbox: mediatek: add support for adsp mailbox controller
2021-11-24 8:45 [PATCH v3 0/3] This patches provide ADSP IPC support for MT8195 allen-kh.cheng
2021-11-24 8:45 ` [PATCH v3 1/3] dt-bindings: mediatek: add adsp-mbox document allen-kh.cheng
@ 2021-11-24 8:45 ` allen-kh.cheng
2021-11-24 10:25 ` Tzung-Bi Shih
2021-11-24 8:45 ` [PATCH v3 3/3] firmware: mediatek: add adsp ipc protocol interface allen-kh.cheng
2 siblings, 1 reply; 11+ messages in thread
From: allen-kh.cheng @ 2021-11-24 8:45 UTC (permalink / raw)
To: Jassi Brar, Rob Herring, Matthias Brugger
Cc: devicetree, Linux-ALSA, Allen-KH Cheng, Kai Vehmanen,
Liam Girdwood, cujomalainey, linux-kernel, Takashi Iwai,
Ranjani Sridharan, Pierre-Louis Bossart,
Project_Global_Chrome_Upstream_Group, tzungbi, Mark Brown,
linux-mediatek, Daniel Baluta, linux-arm-kernel,
sound-open-firmware
From: Allen-KH Cheng <Allen-KH.Cheng@mediatek.com>
This patch is to for MediaTek ADSP IPC mailbox controller driver
It is used to send short messages between processors with adsp
Signed-off-by: Allen-KH Cheng <Allen-KH.Cheng@mediatek.com>
---
drivers/mailbox/Kconfig | 7 ++
drivers/mailbox/Makefile | 2 +
drivers/mailbox/mtk-adsp-mailbox.c | 187 +++++++++++++++++++++++++++++
3 files changed, 196 insertions(+)
create mode 100644 drivers/mailbox/mtk-adsp-mailbox.c
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.
+
config ZYNQMP_IPI_MBOX
bool "Xilinx ZynqMP IPI Mailbox"
depends on ARCH_ZYNQMP && OF
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
+
obj-$(CONFIG_ZYNQMP_IPI_MBOX) += zynqmp-ipi-mailbox.o
obj-$(CONFIG_SUN6I_MSGBOX) += sun6i-msgbox.o
diff --git a/drivers/mailbox/mtk-adsp-mailbox.c b/drivers/mailbox/mtk-adsp-mailbox.c
new file mode 100644
index 000000000000..b814b003aeb2
--- /dev/null
+++ b/drivers/mailbox/mtk-adsp-mailbox.c
@@ -0,0 +1,187 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2021 MediaTek Corporation. All rights reserved.
+ * Author: Allen-KH Cheng <allen-kh.cheng@mediatek.com>
+ */
+
+#include <linux/firmware/mediatek/mtk-adsp-ipc.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/slab.h>
+
+/* adsp mbox register offset */
+#define MTK_ADSP_MBOX_IN_CMD 0x00
+#define MTK_ADSP_MBOX_IN_CMD_CLR 0x04
+#define MTK_ADSP_MBOX_OUT_CMD 0x1c
+#define MTK_ADSP_MBOX_OUT_CMD_CLR 0x20
+#define MTK_ADSP_MBOX_IN_MSG0 0x08
+#define MTK_ADSP_MBOX_IN_MSG1 0x0C
+#define MTK_ADSP_MBOX_OUT_MSG0 0x24
+#define MTK_ADSP_MBOX_OUT_MSG1 0x28
+
+struct mtk_adsp_mbox_priv {
+ struct device *dev;
+ struct mbox_controller mbox;
+ void __iomem *va_mboxreg;
+};
+
+static irqreturn_t mtk_adsp_ipc_irq_handler(int irq, void *data)
+{
+ struct mbox_chan *ch = (struct mbox_chan *)data;
+ struct adsp_mbox_ch_info *ch_info = ch->con_priv;
+ void __iomem *reg = ch_info->va_reg;
+ u32 op = readl(reg + MTK_ADSP_MBOX_OUT_CMD);
+
+ writel(op, reg + MTK_ADSP_MBOX_OUT_CMD_CLR);
+
+ return IRQ_WAKE_THREAD;
+}
+
+static irqreturn_t mtk_adsp_ipc_handler(int irq, void *data)
+{
+ struct mbox_chan *ch = (struct mbox_chan *)data;
+ struct adsp_mbox_ch_info *ch_info = ch->con_priv;
+
+ mbox_chan_received_data(ch, ch_info);
+
+ return IRQ_HANDLED;
+}
+
+static struct mbox_chan *mtk_adsp_mbox_xlate(struct mbox_controller *mbox,
+ const struct of_phandle_args *sp)
+{
+ return &mbox->chans[sp->args[0]];
+}
+
+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;
+}
+
+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);
+
+ return 0;
+}
+
+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;
+}
+
+static const struct mbox_chan_ops adsp_mbox_chan_ops = {
+ .send_data = mtk_adsp_mbox_send_data,
+ .startup = mtk_adsp_mbox_startup,
+ .shutdown = mtk_adsp_mbox_shutdown,
+ .last_tx_done = mtk_adsp_mbox_last_tx_done,
+};
+
+static int mtk_adsp_mbox_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct mbox_controller *mbox;
+ struct mtk_adsp_mbox_priv *priv;
+ struct resource *res;
+ struct adsp_mbox_ch_info *ch_info;
+ u32 size;
+ int ret;
+ int irq;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ mbox = &priv->mbox;
+ mbox->dev = dev;
+ mbox->ops = &adsp_mbox_chan_ops;
+ mbox->txdone_irq = false;
+ mbox->txdone_poll = true;
+ mbox->of_xlate = mtk_adsp_mbox_xlate;
+ mbox->num_chans = 1;
+ mbox->chans = devm_kzalloc(mbox->dev, sizeof(*mbox->chans), GFP_KERNEL);
+ if (!mbox->chans)
+ return -ENOMEM;
+
+ 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);
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0)
+ return irq;
+
+ ret = devm_request_threaded_irq(dev, irq,
+ mtk_adsp_ipc_irq_handler, mtk_adsp_ipc_handler,
+ IRQF_TRIGGER_NONE, dev_name(dev),
+ mbox->chans);
+ if (ret < 0)
+ return ret;
+
+ /* set adsp mbox channel info */
+ ch_info = devm_kzalloc(mbox->dev, sizeof(*ch_info), GFP_KERNEL);
+ if (!ch_info)
+ return -ENOMEM;
+
+ spin_lock_init(&ch_info->lock);
+ ch_info->va_reg = priv->va_mboxreg;
+ mbox->chans->con_priv = ch_info;
+ platform_set_drvdata(pdev, priv);
+ ret = devm_mbox_controller_register(dev, &priv->mbox);
+ if (ret < 0)
+ dev_err(dev, "error: failed to register mailbox:%d\n", ret);
+
+ return ret;
+}
+
+static const struct of_device_id mtk_adsp_mbox_of_match[] = {
+ { .compatible = "mediatek,mt8195-adsp-mbox", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, mtk_adsp_mbox_of_match);
+
+static struct platform_driver mtk_adsp_ipc_mbox_driver = {
+ .probe = mtk_adsp_mbox_probe,
+ .driver = {
+ .name = "mtk_adsp_mbox",
+ .of_match_table = mtk_adsp_mbox_of_match,
+ },
+};
+module_platform_driver(mtk_adsp_ipc_mbox_driver);
+
+MODULE_AUTHOR("Allen-KH Cheng <Allen-KH.Cheng@mediatek.com>");
+MODULE_DESCRIPTION("MTK ADSP mailbox IPC driver");
+MODULE_LICENSE("GPL v2");
--
2.18.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v3 2/3] mailbox: mediatek: add support for adsp mailbox controller
2021-11-24 8:45 ` [PATCH v3 2/3] mailbox: mediatek: add support for adsp mailbox controller allen-kh.cheng
@ 2021-11-24 10:25 ` Tzung-Bi Shih
2021-11-25 1:51 ` allen-kh.cheng
0 siblings, 1 reply; 11+ messages in thread
From: Tzung-Bi Shih @ 2021-11-24 10:25 UTC (permalink / raw)
To: allen-kh.cheng
Cc: devicetree, Linux-ALSA, Kai Vehmanen, cujomalainey,
Daniel Baluta, Mark Brown, Jassi Brar, Liam Girdwood,
linux-kernel, Project_Global_Chrome_Upstream_Group, Rob Herring,
linux-mediatek, Ranjani Sridharan, Matthias Brugger,
Takashi Iwai, Pierre-Louis Bossart, 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] 11+ messages in thread* Re: [PATCH v3 2/3] mailbox: mediatek: add support for adsp mailbox controller
2021-11-24 10:25 ` Tzung-Bi Shih
@ 2021-11-25 1:51 ` allen-kh.cheng
2021-11-25 6:23 ` Tzung-Bi Shih
0 siblings, 1 reply; 11+ messages in thread
From: allen-kh.cheng @ 2021-11-25 1:51 UTC (permalink / raw)
To: Tzung-Bi Shih
Cc: devicetree, Linux-ALSA, Kai Vehmanen, cujomalainey,
Daniel Baluta, Mark Brown, Jassi Brar, Liam Girdwood,
linux-kernel, Project_Global_Chrome_Upstream_Group, Rob Herring,
linux-mediatek, Ranjani Sridharan, Matthias Brugger,
Takashi Iwai, Pierre-Louis Bossart, linux-arm-kernel,
sound-open-firmware
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: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.
> >
>
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
Thanks,
Allen
> > > > +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] 11+ messages in thread* Re: [PATCH v3 2/3] mailbox: mediatek: add support for adsp mailbox controller
2021-11-25 1:51 ` allen-kh.cheng
@ 2021-11-25 6:23 ` Tzung-Bi Shih
2021-11-25 8:24 ` allen-kh.cheng
0 siblings, 1 reply; 11+ messages in thread
From: Tzung-Bi Shih @ 2021-11-25 6:23 UTC (permalink / raw)
To: allen-kh.cheng
Cc: devicetree, Linux-ALSA, Kai Vehmanen, cujomalainey,
Daniel Baluta, Mark Brown, Jassi Brar, Liam Girdwood,
linux-kernel, Project_Global_Chrome_Upstream_Group, Rob Herring,
linux-mediatek, Ranjani Sridharan, Matthias Brugger,
Takashi Iwai, Pierre-Louis Bossart, 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] 11+ messages in thread* Re: [PATCH v3 2/3] mailbox: mediatek: add support for adsp mailbox controller
2021-11-25 6:23 ` Tzung-Bi Shih
@ 2021-11-25 8:24 ` allen-kh.cheng
0 siblings, 0 replies; 11+ messages in thread
From: allen-kh.cheng @ 2021-11-25 8:24 UTC (permalink / raw)
To: Tzung-Bi Shih
Cc: devicetree, Linux-ALSA, Kai Vehmanen, cujomalainey,
Daniel Baluta, Mark Brown, Jassi Brar, Liam Girdwood,
linux-kernel, Project_Global_Chrome_Upstream_Group, Rob Herring,
linux-mediatek, Ranjani Sridharan, Matthias Brugger,
Takashi Iwai, Pierre-Louis Bossart, linux-arm-kernel,
sound-open-firmware
On Thu, 2021-11-25 at 14:23 +0800, Tzung-Bi Shih wrote:
> > 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?
Hi Tzung-Bi,
After I think carefully. There is no need to add lock in
mtk_adsp_mbox_send_data.
In our dsp ipc design, we only have one host(ap) and one client(dsp).
If sof ip message transfer is complete, host will use mbox notice dsp
message arrived.
(MTK_ADSP_MBOX_IN_CMD is signal to trigger mbox irq)
I will remove this in next version.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 3/3] firmware: mediatek: add adsp ipc protocol interface
2021-11-24 8:45 [PATCH v3 0/3] This patches provide ADSP IPC support for MT8195 allen-kh.cheng
2021-11-24 8:45 ` [PATCH v3 1/3] dt-bindings: mediatek: add adsp-mbox document allen-kh.cheng
2021-11-24 8:45 ` [PATCH v3 2/3] mailbox: mediatek: add support for adsp mailbox controller allen-kh.cheng
@ 2021-11-24 8:45 ` allen-kh.cheng
2021-11-24 10:25 ` Tzung-Bi Shih
2 siblings, 1 reply; 11+ messages in thread
From: allen-kh.cheng @ 2021-11-24 8:45 UTC (permalink / raw)
To: Jassi Brar, Rob Herring, Matthias Brugger
Cc: devicetree, Linux-ALSA, Allen-KH Cheng, Kai Vehmanen,
Liam Girdwood, cujomalainey, linux-kernel, Takashi Iwai,
Ranjani Sridharan, Pierre-Louis Bossart,
Project_Global_Chrome_Upstream_Group, tzungbi, Mark Brown,
linux-mediatek, Daniel Baluta, linux-arm-kernel,
sound-open-firmware
From: Allen-KH Cheng <Allen-KH.Cheng@mediatek.com>
Some of mediatek processors contain
the Tensilica HiFix DSP for audio processing.
The communication between Host CPU and DSP firmware is
taking place using a shared memory area for message passing.
ADSP IPC protocol offers (send/recv) interfaces using
mediatek-mailbox APIs.
We use two mbox channels to implement a request-reply protocol.
Signed-off-by: Allen-KH Cheng <Allen-KH.Cheng@mediatek.com>
---
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
diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 220a58cf0a44..005f76a9a31a 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -301,6 +301,7 @@ source "drivers/firmware/broadcom/Kconfig"
source "drivers/firmware/google/Kconfig"
source "drivers/firmware/efi/Kconfig"
source "drivers/firmware/imx/Kconfig"
+source "drivers/firmware/mediatek/Kconfig"
source "drivers/firmware/meson/Kconfig"
source "drivers/firmware/psci/Kconfig"
source "drivers/firmware/smccc/Kconfig"
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 5ced0673d94b..c0d1f3bdeae4 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -33,6 +33,7 @@ obj-$(CONFIG_GOOGLE_FIRMWARE) += google/
obj-$(CONFIG_EFI) += efi/
obj-$(CONFIG_UEFI_CPER) += efi/
obj-y += imx/
+obj-y += mediatek/
obj-y += psci/
obj-y += smccc/
obj-y += tegra/
diff --git a/drivers/firmware/mediatek/Kconfig b/drivers/firmware/mediatek/Kconfig
new file mode 100644
index 000000000000..84a3b3bf5146
--- /dev/null
+++ b/drivers/firmware/mediatek/Kconfig
@@ -0,0 +1,10 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config MTK_ADSP_IPC
+ tristate "MTK ADSP IPC Protocol driver"
+ depends on MTK_ADSP_IPC_MBOX
+ help
+ Say yes here to add support for the MediaTek ADSP IPC protocol
+ between host AP (Linux) and the firmware running on ADSP.
+ ADSP exists on some mtk processors.
+ Client might use shared memory to exchange information with ADSP side.
+
diff --git a/drivers/firmware/mediatek/Makefile b/drivers/firmware/mediatek/Makefile
new file mode 100644
index 000000000000..4e840b65650d
--- /dev/null
+++ b/drivers/firmware/mediatek/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_MTK_ADSP_IPC) += mtk-adsp-ipc.o
diff --git a/drivers/firmware/mediatek/mtk-adsp-ipc.c b/drivers/firmware/mediatek/mtk-adsp-ipc.c
new file mode 100644
index 000000000000..7307c55b5628
--- /dev/null
+++ b/drivers/firmware/mediatek/mtk-adsp-ipc.c
@@ -0,0 +1,130 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2021 MediaTek Corporation. All rights reserved.
+ * Author: Allen-KH Cheng <allen-kh.cheng@mediatek.com>
+ */
+
+#include <linux/firmware/mediatek/mtk-adsp-ipc.h>
+#include <linux/kernel.h>
+#include <linux/mailbox_client.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+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;
+
+ ch_info->ipc_op_val = op;
+ ret = mbox_send_message(dsp_chan->ch, NULL);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+EXPORT_SYMBOL(adsp_ipc_send);
+
+static void adsp_ipc_recv(struct mbox_client *c, void *msg)
+{
+ struct mtk_adsp_chan *chan = container_of(c, struct mtk_adsp_chan, cl);
+
+ if (chan->idx == MTK_ADSP_MBOX_REPLY)
+ chan->ipc->ops->handle_reply(chan->ipc);
+ else
+ chan->ipc->ops->handle_request(chan->ipc);
+}
+
+static int mtk_adsp_ipc_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct mtk_adsp_ipc *dsp_ipc;
+ struct mtk_adsp_chan *dsp_chan;
+ struct mbox_client *cl;
+ char *chan_name;
+ int ret;
+ int i, j;
+
+ device_set_of_node_from_dev(&pdev->dev, pdev->dev.parent);
+
+ dsp_ipc = devm_kzalloc(dev, sizeof(*dsp_ipc), GFP_KERNEL);
+ if (!dsp_ipc)
+ return -ENOMEM;
+
+ 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);
+ goto out;
+ }
+
+ dev_dbg(dev, "request mbox chan %s\n", chan_name);
+ kfree(chan_name);
+ }
+
+ dsp_ipc->dev = dev;
+ dev_set_drvdata(dev, dsp_ipc);
+ dev_dbg(dev, "MTK ADSP IPC initialized\n");
+
+ return 0;
+
+out:
+ kfree(chan_name);
+ for (j = 0; j < i; j++) {
+ dsp_chan = &dsp_ipc->chans[j];
+ mbox_free_channel(dsp_chan->ch);
+ }
+
+ return ret;
+}
+
+static int mtk_adsp_remove(struct platform_device *pdev)
+{
+ struct mtk_adsp_chan *dsp_chan;
+ struct mtk_adsp_ipc *dsp_ipc;
+ int i;
+
+ dsp_ipc = dev_get_drvdata(&pdev->dev);
+
+ for (i = 0; i < MTK_ADSP_MBOX_NUM; i++) {
+ dsp_chan = &dsp_ipc->chans[i];
+ mbox_free_channel(dsp_chan->ch);
+ }
+
+ return 0;
+}
+
+static struct platform_driver mtk_adsp_ipc_driver = {
+ .driver = {
+ .name = "mtk-adsp-ipc",
+ },
+ .probe = mtk_adsp_ipc_probe,
+ .remove = mtk_adsp_remove,
+};
+builtin_platform_driver(mtk_adsp_ipc_driver);
+
+MODULE_AUTHOR("Allen-KH Cheng <allen-kh.cheng@mediatek.com>");
+MODULE_DESCRIPTION("MTK ADSP IPC protocol driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/firmware/mediatek/mtk-adsp-ipc.h b/include/linux/firmware/mediatek/mtk-adsp-ipc.h
new file mode 100644
index 000000000000..26ae4ed9deb2
--- /dev/null
+++ b/include/linux/firmware/mediatek/mtk-adsp-ipc.h
@@ -0,0 +1,72 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2021 MediaTek Inc.
+ */
+
+#ifndef MTK_ADSP_IPC_H
+#define MTK_ADSP_IPC_H
+
+#include <linux/device.h>
+#include <linux/types.h>
+#include <linux/mailbox_controller.h>
+#include <linux/mailbox_client.h>
+
+#define MTK_ADSP_IPC_REQ 0
+#define MTK_ADSP_IPC_RSP 1
+#define MTK_ADSP_IPC_OP_REQ 0x1
+#define MTK_ADSP_IPC_OP_RSP 0x2
+
+enum {
+ MTK_ADSP_MBOX_REPLY,
+ MTK_ADSP_MBOX_REQUEST,
+ MTK_ADSP_MBOX_NUM,
+};
+
+struct mtk_adsp_ipc;
+
+struct mtk_adsp_ipc_ops {
+ void (*handle_reply)(struct mtk_adsp_ipc *ipc);
+ void (*handle_request)(struct mtk_adsp_ipc *ipc);
+};
+
+struct mtk_adsp_chan {
+ struct mtk_adsp_ipc *ipc;
+ struct mbox_client cl;
+ struct mbox_chan *ch;
+ char *name;
+ int idx;
+};
+
+struct mtk_adsp_ipc {
+ struct mtk_adsp_chan chans[MTK_ADSP_MBOX_NUM];
+ struct device *dev;
+ struct mtk_adsp_ipc_ops *ops;
+ void *private_data;
+};
+
+struct adsp_mbox_ch_info {
+ u32 ipc_op_val;
+ void __iomem *va_reg;
+ /* protect access to adsp mbox registers */
+ spinlock_t lock;
+};
+
+static inline void adsp_ipc_set_data(struct mtk_adsp_ipc *ipc, void *data)
+{
+ if (!ipc)
+ return;
+
+ ipc->private_data = data;
+}
+
+static inline void *adsp_ipc_get_data(struct mtk_adsp_ipc *ipc)
+{
+ if (!ipc)
+ return NULL;
+
+ return ipc->private_data;
+}
+
+int adsp_ipc_send(struct mtk_adsp_ipc *ipc, unsigned int idx, uint32_t op);
+
+#endif /* MTK_ADSP_IPC_H */
--
2.18.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v3 3/3] firmware: mediatek: add adsp ipc protocol interface
2021-11-24 8:45 ` [PATCH v3 3/3] firmware: mediatek: add adsp ipc protocol interface allen-kh.cheng
@ 2021-11-24 10:25 ` Tzung-Bi Shih
2021-11-25 1:47 ` allen-kh.cheng
0 siblings, 1 reply; 11+ messages in thread
From: Tzung-Bi Shih @ 2021-11-24 10:25 UTC (permalink / raw)
To: allen-kh.cheng
Cc: devicetree, Linux-ALSA, Kai Vehmanen, cujomalainey, linux-kernel,
Mark Brown, Jassi Brar, Pierre-Louis Bossart, Liam Girdwood,
Project_Global_Chrome_Upstream_Group, Rob Herring,
linux-mediatek, Ranjani Sridharan, Matthias Brugger,
Takashi Iwai, Daniel Baluta, linux-arm-kernel,
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] 11+ messages in thread* Re: [PATCH v3 3/3] firmware: mediatek: add adsp ipc protocol interface
2021-11-24 10:25 ` Tzung-Bi Shih
@ 2021-11-25 1:47 ` allen-kh.cheng
2021-11-25 6:26 ` Tzung-Bi Shih
0 siblings, 1 reply; 11+ messages in thread
From: allen-kh.cheng @ 2021-11-25 1:47 UTC (permalink / raw)
To: Tzung-Bi Shih
Cc: devicetree, Linux-ALSA, Kai Vehmanen, cujomalainey, linux-kernel,
Mark Brown, Jassi Brar, Pierre-Louis Bossart, Liam Girdwood,
Project_Global_Chrome_Upstream_Group, Rob Herring,
linux-mediatek, Ranjani Sridharan, Matthias Brugger,
Takashi Iwai, Daniel Baluta, linux-arm-kernel,
sound-open-firmware
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v3 3/3] firmware: mediatek: add adsp ipc protocol interface
2021-11-25 1:47 ` allen-kh.cheng
@ 2021-11-25 6:26 ` Tzung-Bi Shih
0 siblings, 0 replies; 11+ messages in thread
From: Tzung-Bi Shih @ 2021-11-25 6:26 UTC (permalink / raw)
To: allen-kh.cheng
Cc: devicetree, Linux-ALSA, Kai Vehmanen, cujomalainey, linux-kernel,
Mark Brown, Jassi Brar, Pierre-Louis Bossart, Liam Girdwood,
Project_Global_Chrome_Upstream_Group, Rob Herring,
linux-mediatek, Ranjani Sridharan, Matthias Brugger,
Takashi Iwai, Daniel Baluta, linux-arm-kernel,
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] 11+ messages in thread