* [PATCH v3 0/3] This patches provide ADSP IPC support for MT8195.
@ 2021-11-24 8:45 allen-kh.cheng
2021-11-24 8:45 ` [PATCH v3 1/3] dt-bindings: mediatek: add adsp-mbox document allen-kh.cheng
` (2 more replies)
0 siblings, 3 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
Mediatek ADSP IPC is used to send notification or short message between
processors with dsp.
It will place the message to the share buffer and will access the ADSP mailbox
registers to kick dsp.
Two mailboxes used to send notification or short message between processors with
dsp
Add changes history from series
changes since v2:
- seperate adsp_mailbox into two instances
changes since v1:
- fix dt_binding_check error
Allen-KH Cheng (3):
dt-bindings: mediatek: add adsp-mbox document
mailbox: mediatek: add support for adsp mailbox controller
firmware: mediatek: add adsp ipc protocol interface
.../bindings/mailbox/mtk,adsp-mbox.yaml | 53 +++++
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 ++++++++++++
drivers/mailbox/Kconfig | 7 +
drivers/mailbox/Makefile | 2 +
drivers/mailbox/mtk-adsp-mailbox.c | 187 ++++++++++++++++++
.../linux/firmware/mediatek/mtk-adsp-ipc.h | 72 +++++++
10 files changed, 465 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mailbox/mtk,adsp-mbox.yaml
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 drivers/mailbox/mtk-adsp-mailbox.c
create mode 100644 include/linux/firmware/mediatek/mtk-adsp-ipc.h
--
2.18.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [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
* [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 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 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 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 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
* 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
end of thread, other threads:[~2021-11-25 8:25 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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 10:25 ` Tzung-Bi Shih
2021-11-25 1:51 ` allen-kh.cheng
2021-11-25 6:23 ` Tzung-Bi Shih
2021-11-25 8:24 ` allen-kh.cheng
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
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).