[PATCH v2 0/4] add mailbox support for i.MX7D

[PATCH v2 0/4] add mailbox support for i.MX7D

[Oleksij Rempel]

20180615 changes v2:
- DT: use mailbox@ instead of mu@
- DT: change interrupts description
- clk: use imx_clk_gate4 instead of imx_clk_gate2
- imx-mailbox: remove last_tx_done support
- imx-mailbox: fix module description 

This patches are providing support for mailbox (Messaging Unit)
for i.MX7D.
Functionality was tested on PHYTEC phyBOARD-Zeta i.MX7D with
Linux running on all cores: ARM Cortex-A7 and ARM Cortex-M4.

Both parts of i.MX messaging Unit are visible for all CPUs available
on i.MX7D. Communication worked independent of MU side in combination
with CPU. For example MU-A used on ARM Cortex-A7 and MU-B used on ARM Cortex-M4
or other ways around.

The question to NXP developers: are there are limitations or
recommendations about MU vs CPU combination? The i.MX7D documentation
talks about "Processor A" and "Processor B". It is not quite clear what
processor it actually is (A7 or M4).

Oleksij Rempel (4):
  clk: imx7d: add IMX7D_MU_ROOT_CLK
  dt-bindings: mailbox: provide imx-mailbox documentation
  ARM: dts: imx7s: add i.MX7 messaging unit support
  mailbox: Add support for i.MX7D messaging unit

 .../bindings/mailbox/imx-mailbox.txt          |  35 +++
 arch/arm/boot/dts/imx7s.dtsi                  |  18 ++
 drivers/clk/imx/clk-imx7d.c                   |   1 +
 drivers/mailbox/Kconfig                       |   6 +
 drivers/mailbox/Makefile                      |   2 +
 drivers/mailbox/imx-mailbox.c                 | 288 ++++++++++++++++++
 6 files changed, 350 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/imx-mailbox.txt
 create mode 100644 drivers/mailbox/imx-mailbox.c

-- 
2.17.1

[PATCH v2 1/4] clk: imx7d: add IMX7D_MU_ROOT_CLK

[Oleksij Rempel]

This clock is needed for iMX mailbox driver

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/clk/imx/clk-imx7d.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/imx/clk-imx7d.c b/drivers/clk/imx/clk-imx7d.c
index 975a20d3cc94..c7159bfac77a 100644
--- a/drivers/clk/imx/clk-imx7d.c
+++ b/drivers/clk/imx/clk-imx7d.c
@@ -795,6 +795,7 @@ static void __init imx7d_clocks_init(struct device_node *ccm_node)
 	clks[IMX7D_DRAM_ALT_ROOT_CLK] = imx_clk_gate4("dram_alt_root_clk", "dram_alt_post_div", base + 0x4130, 0);
 	clks[IMX7D_OCOTP_CLK] = imx_clk_gate4("ocotp_clk", "ipg_root_clk", base + 0x4230, 0);
 	clks[IMX7D_SNVS_CLK] = imx_clk_gate4("snvs_clk", "ipg_root_clk", base + 0x4250, 0);
+	clks[IMX7D_MU_ROOT_CLK] = imx_clk_gate4("mu_root_clk", "ipg_root_clk", base + 0x4270, 0);
 	clks[IMX7D_CAAM_CLK] = imx_clk_gate4("caam_clk", "ipg_root_clk", base + 0x4240, 0);
 	clks[IMX7D_USB_HSIC_ROOT_CLK] = imx_clk_gate4("usb_hsic_root_clk", "usb_hsic_post_div", base + 0x4690, 0);
 	clks[IMX7D_SDMA_CORE_CLK] = imx_clk_gate4("sdma_root_clk", "ahb_root_clk", base + 0x4480, 0);
-- 
2.17.1

[PATCH v2 2/4] dt-bindings: mailbox: provide imx-mailbox documentation

[Oleksij Rempel]

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 .../bindings/mailbox/imx-mailbox.txt          | 35 +++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/imx-mailbox.txt

diff --git a/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt b/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt
new file mode 100644
index 000000000000..1577b86f1206
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt
@@ -0,0 +1,35 @@
+i.MX Messaging Unit
+===================
+
+The i.MX Messaging Unit (MU) contains two register sets: "A" and "B". In most
+cases they are accessible from all Processor Units. On one hand, at least for
+mailbox functionality, it makes no difference which application or processor is
+using which set of the MU. On other hand, the register sets for each of the MU
+parts are not identical.
+
+Required properties:
+- compatible :	Shell be one of:
+                    "fsl,imx7s-mu-a" and "fsl,imx7s-mu-b" for i.MX7S or i.MX7D
+- reg :		physical base address of the mailbox and length of
+		memory mapped region.
+- #mbox-cells:	Common mailbox binding property to identify the number
+		of cells required for the mailbox specifier. Should be 1.
+- interrupts :	The interrupt number
+- clocks     :  phandle to the input clock.
+
+Example:
+	mu0a: mailbox@30aa0000 {
+		compatible = "fsl,imx7s-mu-a";
+		reg = <0x30aa0000 0x28>;
+		interrupts = <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&clks IMX7D_MU_ROOT_CLK>;
+		#mbox-cells = <1>;
+	};
+
+	mu0b: mailbox@30ab0000 {
+		compatible = "fsl,imx7s-mu-b";
+		reg = <0x30ab0000 0x28>;
+		interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&clks IMX7D_MU_ROOT_CLK>;
+		#mbox-cells = <1>;
+	};
-- 
2.17.1

[PATCH v2 3/4] ARM: dts: imx7s: add i.MX7 messaging unit support

[Oleksij Rempel]

Define the Messaging Unit (MU) for i.MX7 in the processor's dtsi.
The respective driver is added in the next commit.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 arch/arm/boot/dts/imx7s.dtsi | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
index d9437e773b37..87a82c1f4dfd 100644
--- a/arch/arm/boot/dts/imx7s.dtsi
+++ b/arch/arm/boot/dts/imx7s.dtsi
@@ -1008,6 +1008,24 @@
 				status = "disabled";
 			};
 
+			mu0a: mailbox@30aa0000 {
+				compatible = "fsl,imx7s-mu-a";
+				reg = <0x30aa0000 0x10000>;
+				interrupts = <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>;
+				clocks = <&clks IMX7D_MU_ROOT_CLK>;
+				#mbox-cells = <1>;
+				status = "disabled";
+			};
+
+			mu0b: mailbox@30ab0000 {
+				compatible = "fsl,imx7s-mu-b";
+				reg = <0x30ab0000 0x10000>;
+				interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>;
+				clocks = <&clks IMX7D_MU_ROOT_CLK>;
+				#mbox-cells = <1>;
+				status = "disabled";
+			};
+
 			usbotg1: usb@30b10000 {
 				compatible = "fsl,imx7d-usb", "fsl,imx27-usb";
 				reg = <0x30b10000 0x200>;
-- 
2.17.1

[PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit

[Oleksij Rempel]

The Mailbox controller is able to send messages (up to 4 32 bit words)
between the endpoints.

This driver was tested using the mailbox-test driver sending messages
between the Cortex-A7 and the Cortex-M4.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/mailbox/Kconfig       |   6 +
 drivers/mailbox/Makefile      |   2 +
 drivers/mailbox/imx-mailbox.c | 288 ++++++++++++++++++++++++++++++++++
 3 files changed, 296 insertions(+)
 create mode 100644 drivers/mailbox/imx-mailbox.c

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index a2bb27446dce..e1d2738a2e4c 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -15,6 +15,12 @@ config ARM_MHU
 	  The controller has 3 mailbox channels, the last of which can be
 	  used in Secure mode only.
 
+config IMX_MBOX
+	tristate "iMX Mailbox"
+	depends on SOC_IMX7D || COMPILE_TEST
+	help
+	  Mailbox implementation for iMX7D Messaging Unit (MU).
+
 config PLATFORM_MHU
 	tristate "Platform MHU Mailbox"
 	depends on OF
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index cc23c3a43fcd..ba2fe1b6dd62 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)	+= mailbox-test.o
 
 obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o
 
+obj-$(CONFIG_IMX_MBOX)	+= imx-mailbox.o
+
 obj-$(CONFIG_PLATFORM_MHU)	+= platform_mhu.o
 
 obj-$(CONFIG_PL320_MBOX)	+= pl320-ipc.o
diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
new file mode 100644
index 000000000000..e3f621cb1d30
--- /dev/null
+++ b/drivers/mailbox/imx-mailbox.c
@@ -0,0 +1,288 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018 Pengutronix, Oleksij Rempel <o.rempel@pengutronix.de>
+ */
+
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+
+/* Transmit Register */
+#define IMX_MU_xTRn(x)		(0x00 + 4 * (x))
+/* Receive Register */
+#define IMX_MU_xRRn(x)		(0x10 + 4 * (x))
+/* Status Register */
+#define IMX_MU_xSR		0x20
+#define IMX_MU_xSR_TEn(x)	BIT(20 + (x))
+#define IMX_MU_xSR_RFn(x)	BIT(24 + (x))
+#define IMX_MU_xSR_BRDIP	BIT(9)
+
+/* Control Register */
+#define IMX_MU_xCR		0x24
+/* Transmit Interrupt Enable */
+#define IMX_MU_xCR_TIEn(x)	BIT(20 + (x))
+/* Receive Interrupt Enable */
+#define IMX_MU_xCR_RIEn(x)	BIT(24 + (x))
+
+#define IMX_MU_MAX_CHANS	4u
+
+struct imx_mu_priv;
+
+struct imx_mu_cfg {
+	unsigned int		chans;
+	void (*init_hw)(struct imx_mu_priv *priv);
+};
+
+struct imx_mu_con_priv {
+	int			irq;
+	unsigned int		bidx;
+	unsigned int		idx;
+};
+
+struct imx_mu_priv {
+	struct device		*dev;
+	const struct imx_mu_cfg	*dcfg;
+	void __iomem		*base;
+
+	struct mbox_controller	mbox;
+	struct mbox_chan	mbox_chans[IMX_MU_MAX_CHANS];
+
+	struct imx_mu_con_priv  con_priv[IMX_MU_MAX_CHANS];
+	struct clk		*clk;
+};
+
+static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller *mbox)
+{
+	return container_of(mbox, struct imx_mu_priv, mbox);
+}
+
+static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs)
+{
+	iowrite32(val, priv->base + offs);
+}
+
+static u32 imx_mu_read(struct imx_mu_priv *priv, u32 offs)
+{
+	return ioread32(priv->base + offs);
+}
+
+static u32 imx_mu_rmw(struct imx_mu_priv *priv, u32 offs, u32 set, u32 clr)
+{
+	u32 val;
+
+	val = imx_mu_read(priv, offs);
+	val &= ~clr;
+	val |= set;
+	imx_mu_write(priv, val, offs);
+
+	return val;
+}
+
+static irqreturn_t imx_mu_isr(int irq, void *p)
+{
+	struct mbox_chan *chan = p;
+	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
+	struct imx_mu_con_priv *cp = chan->con_priv;
+
+	u32 val, dat;
+
+	val = imx_mu_read(priv, IMX_MU_xSR);
+	val &= IMX_MU_xSR_TEn(cp->bidx) | IMX_MU_xSR_RFn(cp->bidx);
+	if (!val)
+		return IRQ_NONE;
+
+	if (val & IMX_MU_xSR_TEn(cp->bidx)) {
+		imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp->bidx));
+		mbox_chan_txdone(chan, 0);
+	}
+
+	if (val & IMX_MU_xSR_RFn(cp->bidx)) {
+		dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
+		mbox_chan_received_data(chan, (void *)&dat);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static bool imx_mu_last_tx_done(struct mbox_chan *chan)
+{
+	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
+	struct imx_mu_con_priv *cp = chan->con_priv;
+	u32 val;
+
+	val = imx_mu_read(priv, IMX_MU_xSR);
+	/* test if transmit register is empty */
+	return (!!(val & IMX_MU_xSR_TEn(cp->bidx)));
+}
+
+static int imx_mu_send_data(struct mbox_chan *chan, void *data)
+{
+	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
+	struct imx_mu_con_priv *cp = chan->con_priv;
+	u32 *arg = data;
+
+	if (!imx_mu_last_tx_done(chan))
+		return -EBUSY;
+
+	imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx));
+	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->bidx), 0);
+
+	return 0;
+}
+
+static int imx_mu_startup(struct mbox_chan *chan)
+{
+	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
+	struct imx_mu_con_priv *cp = chan->con_priv;
+	int ret;
+
+	ret = request_irq(cp->irq, imx_mu_isr,
+			  IRQF_SHARED, "imx_mu_chan", chan);
+	if (ret) {
+		dev_err(chan->mbox->dev,
+			"Unable to acquire IRQ %d\n", cp->irq);
+		return ret;
+	}
+
+	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->bidx), 0);
+
+	return 0;
+}
+
+static void imx_mu_shutdown(struct mbox_chan *chan)
+{
+	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
+	struct imx_mu_con_priv *cp = chan->con_priv;
+
+	imx_mu_rmw(priv, IMX_MU_xCR, 0,
+		   IMX_MU_xCR_TIEn(cp->bidx) | IMX_MU_xCR_RIEn(cp->bidx));
+
+	free_irq(cp->irq, chan);
+}
+
+static const struct mbox_chan_ops imx_mu_ops = {
+	.send_data = imx_mu_send_data,
+	.startup = imx_mu_startup,
+	.shutdown = imx_mu_shutdown,
+};
+
+static int imx_mu_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *iomem;
+	struct imx_mu_priv *priv;
+	const struct imx_mu_cfg *dcfg;
+	unsigned int i, chans;
+	int irq, ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	dcfg = of_device_get_match_data(dev);
+	if (!dcfg)
+		return -EINVAL;
+
+	priv->dcfg = dcfg;
+	priv->dev = dev;
+
+	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(&pdev->dev, iomem);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq <= 0)
+		return irq < 0 ? irq : -EINVAL;
+
+	priv->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(priv->clk)) {
+		if (PTR_ERR(priv->clk) == -ENOENT) {
+			priv->clk = NULL;
+		} else {
+			dev_err(dev, "Failed to get clock\n");
+			return PTR_ERR(priv->clk);
+		}
+	}
+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret) {
+		dev_err(dev, "Failed to enable clock\n");
+		return ret;
+	}
+
+	chans = min(dcfg->chans, IMX_MU_MAX_CHANS);
+	/* Initialize channel identifiers */
+	for (i = 0; i < chans; i++) {
+		struct imx_mu_con_priv *cp = &priv->con_priv[i];
+
+		cp->bidx = 3 - i;
+		cp->idx = i;
+		cp->irq = irq;
+		priv->mbox_chans[i].con_priv = cp;
+	}
+
+	priv->mbox.dev = dev;
+	priv->mbox.ops = &imx_mu_ops;
+	priv->mbox.chans = priv->mbox_chans;
+	priv->mbox.num_chans = chans;
+	priv->mbox.txdone_irq = true;
+
+	platform_set_drvdata(pdev, priv);
+
+	if (priv->dcfg->init_hw)
+		priv->dcfg->init_hw(priv);
+
+	return mbox_controller_register(&priv->mbox);
+}
+
+static int imx_mu_remove(struct platform_device *pdev)
+{
+	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
+
+	mbox_controller_unregister(&priv->mbox);
+	clk_disable_unprepare(priv->clk);
+
+	return 0;
+}
+
+
+static void imx_mu_init_imx7d_a(struct imx_mu_priv *priv)
+{
+	/* Set default config */
+	imx_mu_write(priv, 0, IMX_MU_xCR);
+}
+
+static const struct imx_mu_cfg imx_mu_cfg_imx7d_a = {
+	.chans = IMX_MU_MAX_CHANS,
+	.init_hw = imx_mu_init_imx7d_a,
+};
+
+static const struct imx_mu_cfg imx_mu_cfg_imx7d_b = {
+	.chans = IMX_MU_MAX_CHANS,
+};
+
+static const struct of_device_id imx_mu_dt_ids[] = {
+	{ .compatible = "fsl,imx7s-mu-a", .data = &imx_mu_cfg_imx7d_a },
+	{ .compatible = "fsl,imx7s-mu-b", .data = &imx_mu_cfg_imx7d_b },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, imx_mu_dt_ids);
+
+static struct platform_driver imx_mu_driver = {
+	.probe		= imx_mu_probe,
+	.remove		= imx_mu_remove,
+	.driver = {
+		.name	= "imx_mu",
+		.of_match_table = imx_mu_dt_ids,
+	},
+};
+module_platform_driver(imx_mu_driver);
+
+MODULE_AUTHOR("Oleksij Rempel <o.rempel@pengutronix.de>");
+MODULE_DESCRIPTION("Message Unit driver for i.MX");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

Re: [PATCH v2 2/4] dt-bindings: mailbox: provide imx-mailbox documentation

[Leonard Crestez]

On Fri, 2018-06-15 at 11:51 +0200, Oleksij Rempel wrote:
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  .../bindings/mailbox/imx-mailbox.txt          | 35 +++++++++++++++++++
>  1 file changed, 35 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mailbox/imx-mailbox.txt

A recent patch was posted which adds a similar but different binding
for the MU on 8qm/8qxp SOCs:

https://patchwork.kernel.org/patch/10468885/

Looking at manuals side-by-side the hardware seems to be the same so
there should be a single binding. Right?

That series I pointed to uses the MU to implement a communication with
a special "SCU" core which runs NXP firmware for handling details like
power management. However imx8 socs also have other MUs and M4 cores
for customers to use pretty exactly like they would on 7d.

The hardware exposes a very generic interface and my impression is that
 drivers for the MU are actually highly specific to what is on the
other side of the MU. For example your driver code seems to be mapping
the 4 MU registers to separate "channels" but for SCU messages are
written in all registers in a round-robin way.

Shouldn't your MU-using driver be a separate node which references the
MU by phandle? Like in this patch:

https://patchwork.kernel.org/patch/10468887/

> diff --git a/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt b/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt
> new file mode 100644
> index 000000000000..1577b86f1206
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt
> @@ -0,0 +1,35 @@
> +i.MX Messaging Unit
> +===================
> +
> +The i.MX Messaging Unit (MU) contains two register sets: "A" and "B". In most
> +cases they are accessible from all Processor Units. On one hand, at least for
> +mailbox functionality, it makes no difference which application or processor is
> +using which set of the MU. On other hand, the register sets for each of the MU
> +parts are not identical.
> +
> +Required properties:
> +- compatible :	Shell be one of:
> +                    "fsl,imx7s-mu-a" and "fsl,imx7s-mu-b" for i.MX7S or i.MX7D
> +- reg :		physical base address of the mailbox and length of
> +		memory mapped region.
> +- #mbox-cells:	Common mailbox binding property to identify the number
> +		of cells required for the mailbox specifier. Should be 1.
> +- interrupts :	The interrupt number
> +- clocks     :  phandle to the input clock.
> +
> +Example:
> +	mu0a: mailbox@30aa0000 {
> +		compatible = "fsl,imx7s-mu-a";
> +		reg = <0x30aa0000 0x28>;
> +		interrupts = <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&clks IMX7D_MU_ROOT_CLK>;
> +		#mbox-cells = <1>;
> +	};
> +
> +	mu0b: mailbox@30ab0000 {
> +		compatible = "fsl,imx7s-mu-b";
> +		reg = <0x30ab0000 0x28>;
> +		interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&clks IMX7D_MU_ROOT_CLK>;
> +		#mbox-cells = <1>;
> +	};

Re: [PATCH v2 2/4] dt-bindings: mailbox: provide imx-mailbox documentation

[Oleksij Rempel]

[-- Attachment #1.1.1: Type: text/plain, Size: 5711 bytes --]

Hi Leonard,

On 18.06.2018 10:53, Leonard Crestez wrote:
> On Fri, 2018-06-15 at 11:51 +0200, Oleksij Rempel wrote:
>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>> ---
>>  .../bindings/mailbox/imx-mailbox.txt          | 35 +++++++++++++++++++
>>  1 file changed, 35 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mailbox/imx-mailbox.txt
> 
> A recent patch was posted which adds a similar but different binding
> for the MU on 8qm/8qxp SOCs:
> 
> https://patchwork.kernel.org/patch/10468885/
> 
> Looking at manuals side-by-side the hardware seems to be the same so
> there should be a single binding. Right?

yes. This is why it make no sense to create imx8 specific MU driver.

> That series I pointed to uses the MU to implement a communication with
> a special "SCU" core which runs NXP firmware for handling details like
> power management. However imx8 socs also have other MUs and M4 cores
> for customers to use pretty exactly like they would on 7d.
> 
> The hardware exposes a very generic interface and my impression is that
>  drivers for the MU are actually highly specific to what is on the
> other side of the MU. For example your driver code seems to be mapping
> the 4 MU registers to separate "channels" but for SCU messages are
> written in all registers in a round-robin way.

ok.. let's take some of IMX8 SCU driver code to see if there any difference:

-- this part of the code is blocking write procedure for one channeler
per write-- correct?
+void mu_send_msg(struct mu_priv *priv, uint32_t index, uint32_t msg)
+{
+	uint32_t mask = MU_SR_TE0_MASK >> index;
+
+	/* Wait TX register to be empty. */
+	while (!(readl_relaxed(priv->base + MU_ASR) & mask))
+		;
+	writel_relaxed(msg, priv->base + MU_ATR0  + (index * 4));
+}
+EXPORT_SYMBOL_GPL(mu_send_msg);

+static void sc_ipc_write(struct sc_ipc *sc_ipc, void *data)
+{
+	sc_rpc_msg_t *msg = (sc_rpc_msg_t *) data;
+	uint8_t count = 0;
+
+	/* Check size */
+	if (msg->size > SC_RPC_MAX_MSG)
+		return;
+
+	/* Write first word */
+	mu_send_msg(sc_ipc->mu_base, 0, *((uint32_t *) msg));
+	count++;

--- in this loop we are writing to one channel per loop and waiting
until the channel was done ----

+	/* Write remaining words */
+	while (count < msg->size) {
+		mu_send_msg(sc_ipc->mu_base, count % MU_TR_COUNT,
+			    msg->DATA.u32[count - 1]);
+		count++;
+	}
+}


... and here is a proof that sc_ipc_write will do in some cases 5 rounds
(5 * 4 bytes = 20 bytes single message) with probable busy waiting for
each channel, which make me think that shared memory would be a better deal.

+sc_err_t sc_misc_seco_image_load(sc_ipc_t ipc, uint32_t addr_src,
+				 uint32_t addr_dst, uint32_t len, bool fw)
+{
+	sc_rpc_msg_t msg;
+	uint8_t result;
+
+	RPC_VER(&msg) = SC_RPC_VERSION;
+	RPC_SVC(&msg) = (uint8_t)SC_RPC_SVC_MISC;
+	RPC_FUNC(&msg) = (uint8_t)MISC_FUNC_SECO_IMAGE_LOAD;
+	RPC_U32(&msg, 0) = addr_src;
+	RPC_U32(&msg, 4) = addr_dst;
+	RPC_U32(&msg, 8) = len;
+	RPC_U8(&msg, 12) = (uint8_t)fw;
+	RPC_SIZE(&msg) = 5;
+
+	sc_call_rpc(ipc, &msg, false);
+
+	result = RPC_R8(&msg);
+	return (sc_err_t)result;
+}
+

So, the same code with mailbox framework will be some thing like this:

	/* Write remaining words */
	while (count < msg->size) {
		mbox_send_message(sc_ipc->mbox_chan[count % MU_TR_COUNT],
msg->DATA.u32[count - 1]);
		count++;
	}

to provide identical behavior we should set mbox_client->tx_block = true
and implement polling mode in the mailbox driver.

Please note. I don't think sc_ipc_write() implementation is good. I just
don't expect it will be ever changed.

> Shouldn't your MU-using driver be a separate node which references the
> MU by phandle? Like in this patch:

sure. But it is generic, not mailbox controller specific and make no
sense to describe client binding again in the controller binding.

> https://patchwork.kernel.org/patch/10468887/
> 
>> diff --git a/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt b/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt
>> new file mode 100644
>> index 000000000000..1577b86f1206
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt
>> @@ -0,0 +1,35 @@
>> +i.MX Messaging Unit
>> +===================
>> +
>> +The i.MX Messaging Unit (MU) contains two register sets: "A" and "B". In most
>> +cases they are accessible from all Processor Units. On one hand, at least for
>> +mailbox functionality, it makes no difference which application or processor is
>> +using which set of the MU. On other hand, the register sets for each of the MU
>> +parts are not identical.
>> +
>> +Required properties:
>> +- compatible :	Shell be one of:
>> +                    "fsl,imx7s-mu-a" and "fsl,imx7s-mu-b" for i.MX7S or i.MX7D
>> +- reg :		physical base address of the mailbox and length of
>> +		memory mapped region.
>> +- #mbox-cells:	Common mailbox binding property to identify the number
>> +		of cells required for the mailbox specifier. Should be 1.
>> +- interrupts :	The interrupt number
>> +- clocks     :  phandle to the input clock.
>> +
>> +Example:
>> +	mu0a: mailbox@30aa0000 {
>> +		compatible = "fsl,imx7s-mu-a";
>> +		reg = <0x30aa0000 0x28>;
>> +		interrupts = <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>;
>> +		clocks = <&clks IMX7D_MU_ROOT_CLK>;
>> +		#mbox-cells = <1>;
>> +	};
>> +
>> +	mu0b: mailbox@30ab0000 {
>> +		compatible = "fsl,imx7s-mu-b";
>> +		reg = <0x30ab0000 0x28>;
>> +		interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>;
>> +		clocks = <&clks IMX7D_MU_ROOT_CLK>;
>> +		#mbox-cells = <1>;
>> +	};


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit

[A.s. Dong]

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> Sent: Friday, June 15, 2018 5:51 PM
> To: Shawn Guo <shawnguo@kernel.org>; Fabio Estevam
> <fabio.estevam@nxp.com>; Rob Herring <robh+dt@kernel.org>; Mark
> Rutland <mark.rutland@arm.com>; A.s. Dong <aisheng.dong@nxp.com>
> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; kernel@pengutronix.de;
> linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-
> clk@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit
> 
> The Mailbox controller is able to send messages (up to 4 32 bit words)
> between the endpoints.
> 
> This driver was tested using the mailbox-test driver sending messages
> between the Cortex-A7 and the Cortex-M4.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/mailbox/Kconfig       |   6 +
>  drivers/mailbox/Makefile      |   2 +
>  drivers/mailbox/imx-mailbox.c | 288
> ++++++++++++++++++++++++++++++++++
>  3 files changed, 296 insertions(+)
>  create mode 100644 drivers/mailbox/imx-mailbox.c
> 
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> a2bb27446dce..e1d2738a2e4c 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -15,6 +15,12 @@ config ARM_MHU
>  	  The controller has 3 mailbox channels, the last of which can be
>  	  used in Secure mode only.
> 
> +config IMX_MBOX
> +	tristate "iMX Mailbox"
> +	depends on SOC_IMX7D || COMPILE_TEST
> +	help
> +	  Mailbox implementation for iMX7D Messaging Unit (MU).
> +
>  config PLATFORM_MHU
>  	tristate "Platform MHU Mailbox"
>  	depends on OF
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index
> cc23c3a43fcd..ba2fe1b6dd62 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)	+= mailbox-test.o
> 
>  obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o
> 
> +obj-$(CONFIG_IMX_MBOX)	+= imx-mailbox.o
> +
>  obj-$(CONFIG_PLATFORM_MHU)	+= platform_mhu.o
> 
>  obj-$(CONFIG_PL320_MBOX)	+= pl320-ipc.o
> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> new file mode 100644 index 000000000000..e3f621cb1d30
> --- /dev/null
> +++ b/drivers/mailbox/imx-mailbox.c
> @@ -0,0 +1,288 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018 Pengutronix, Oleksij Rempel
> +<o.rempel@pengutronix.de>  */
> +
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +
> +/* Transmit Register */
> +#define IMX_MU_xTRn(x)		(0x00 + 4 * (x))
> +/* Receive Register */
> +#define IMX_MU_xRRn(x)		(0x10 + 4 * (x))
> +/* Status Register */
> +#define IMX_MU_xSR		0x20
> +#define IMX_MU_xSR_TEn(x)	BIT(20 + (x))
> +#define IMX_MU_xSR_RFn(x)	BIT(24 + (x))
> +#define IMX_MU_xSR_BRDIP	BIT(9)
> +
> +/* Control Register */
> +#define IMX_MU_xCR		0x24
> +/* Transmit Interrupt Enable */
> +#define IMX_MU_xCR_TIEn(x)	BIT(20 + (x))
> +/* Receive Interrupt Enable */
> +#define IMX_MU_xCR_RIEn(x)	BIT(24 + (x))
> +
> +#define IMX_MU_MAX_CHANS	4u
> +
> +struct imx_mu_priv;
> +
> +struct imx_mu_cfg {
> +	unsigned int		chans;
> +	void (*init_hw)(struct imx_mu_priv *priv); };
> +
> +struct imx_mu_con_priv {
> +	int			irq;
> +	unsigned int		bidx;
> +	unsigned int		idx;
> +};
> +
> +struct imx_mu_priv {
> +	struct device		*dev;
> +	const struct imx_mu_cfg	*dcfg;
> +	void __iomem		*base;
> +
> +	struct mbox_controller	mbox;
> +	struct mbox_chan	mbox_chans[IMX_MU_MAX_CHANS];
> +
> +	struct imx_mu_con_priv  con_priv[IMX_MU_MAX_CHANS];
> +	struct clk		*clk;
> +};
> +
> +static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller *mbox)
> +{
> +	return container_of(mbox, struct imx_mu_priv, mbox); }
> +
> +static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs) {
> +	iowrite32(val, priv->base + offs);
> +}
> +
> +static u32 imx_mu_read(struct imx_mu_priv *priv, u32 offs) {
> +	return ioread32(priv->base + offs);
> +}
> +
> +static u32 imx_mu_rmw(struct imx_mu_priv *priv, u32 offs, u32 set, u32
> +clr) {
> +	u32 val;
> +
> +	val = imx_mu_read(priv, offs);
> +	val &= ~clr;
> +	val |= set;
> +	imx_mu_write(priv, val, offs);
> +
> +	return val;
> +}
> +
> +static irqreturn_t imx_mu_isr(int irq, void *p) {
> +	struct mbox_chan *chan = p;
> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +	struct imx_mu_con_priv *cp = chan->con_priv;
> +
> +	u32 val, dat;
> +
> +	val = imx_mu_read(priv, IMX_MU_xSR);
> +	val &= IMX_MU_xSR_TEn(cp->bidx) | IMX_MU_xSR_RFn(cp->bidx);
> +	if (!val)
> +		return IRQ_NONE;
> +
> +	if (val & IMX_MU_xSR_TEn(cp->bidx)) {

I'm wondering whether this can work properly for multi consumers
at the same time. 
Because xSR_TEn is 1 by default and four virtual channels actually
are using the same interrupt. That means channel 1 interrupt may
cause channel 2 believe it's txdone.
Have we tested such using?

> +		imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp-
> >bidx));
> +		mbox_chan_txdone(chan, 0);
> +	}
> +
> +	if (val & IMX_MU_xSR_RFn(cp->bidx)) {
> +		dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
> +		mbox_chan_received_data(chan, (void *)&dat);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static bool imx_mu_last_tx_done(struct mbox_chan *chan) {
> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +	struct imx_mu_con_priv *cp = chan->con_priv;
> +	u32 val;
> +
> +	val = imx_mu_read(priv, IMX_MU_xSR);
> +	/* test if transmit register is empty */
> +	return (!!(val & IMX_MU_xSR_TEn(cp->bidx))); }
> +
> +static int imx_mu_send_data(struct mbox_chan *chan, void *data) {
> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +	struct imx_mu_con_priv *cp = chan->con_priv;
> +	u32 *arg = data;
> +
> +	if (!imx_mu_last_tx_done(chan))
> +		return -EBUSY;
> +
> +	imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx));
> +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->bidx), 0);
> +
> +	return 0;
> +}

Since Sascha is requesting to write a generic MU mailbox driver for both
SCU MU and M4 case, the current way using virtual channels in this patch
only send one word a time obviously can't fit for SCU MU clients well.
Do you think if we can refer to TI case to design a generic data transfer
protocol to allow send multi words which is more close to SCU?
include/linux/soc/ti/ti-msgmgr.h
struct ti_msgmgr_message {
        size_t len;
        u8 *buf;
};  

Or we try to support both type transfer protocols in this driver?
That may introduce much complexities, personally I'm not quite
like that.

Regards
Dong Aisheng

> +
> +static int imx_mu_startup(struct mbox_chan *chan) {
> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +	struct imx_mu_con_priv *cp = chan->con_priv;
> +	int ret;
> +
> +	ret = request_irq(cp->irq, imx_mu_isr,
> +			  IRQF_SHARED, "imx_mu_chan", chan);
> +	if (ret) {
> +		dev_err(chan->mbox->dev,
> +			"Unable to acquire IRQ %d\n", cp->irq);
> +		return ret;
> +	}
> +
> +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->bidx), 0);
> +
> +	return 0;
> +}
> +
> +static void imx_mu_shutdown(struct mbox_chan *chan) {
> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +	struct imx_mu_con_priv *cp = chan->con_priv;
> +
> +	imx_mu_rmw(priv, IMX_MU_xCR, 0,
> +		   IMX_MU_xCR_TIEn(cp->bidx) | IMX_MU_xCR_RIEn(cp-
> >bidx));
> +
> +	free_irq(cp->irq, chan);
> +}
> +
> +static const struct mbox_chan_ops imx_mu_ops = {
> +	.send_data = imx_mu_send_data,
> +	.startup = imx_mu_startup,
> +	.shutdown = imx_mu_shutdown,
> +};
> +
> +static int imx_mu_probe(struct platform_device *pdev) {
> +	struct device *dev = &pdev->dev;
> +	struct resource *iomem;
> +	struct imx_mu_priv *priv;
> +	const struct imx_mu_cfg *dcfg;
> +	unsigned int i, chans;
> +	int irq, ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	dcfg = of_device_get_match_data(dev);
> +	if (!dcfg)
> +		return -EINVAL;
> +
> +	priv->dcfg = dcfg;
> +	priv->dev = dev;
> +
> +	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	priv->base = devm_ioremap_resource(&pdev->dev, iomem);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq <= 0)
> +		return irq < 0 ? irq : -EINVAL;
> +
> +	priv->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(priv->clk)) {
> +		if (PTR_ERR(priv->clk) == -ENOENT) {
> +			priv->clk = NULL;
> +		} else {
> +			dev_err(dev, "Failed to get clock\n");
> +			return PTR_ERR(priv->clk);
> +		}
> +	}
> +
> +	ret = clk_prepare_enable(priv->clk);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable clock\n");
> +		return ret;
> +	}
> +
> +	chans = min(dcfg->chans, IMX_MU_MAX_CHANS);
> +	/* Initialize channel identifiers */
> +	for (i = 0; i < chans; i++) {
> +		struct imx_mu_con_priv *cp = &priv->con_priv[i];
> +
> +		cp->bidx = 3 - i;
> +		cp->idx = i;
> +		cp->irq = irq;
> +		priv->mbox_chans[i].con_priv = cp;
> +	}
> +
> +	priv->mbox.dev = dev;
> +	priv->mbox.ops = &imx_mu_ops;
> +	priv->mbox.chans = priv->mbox_chans;
> +	priv->mbox.num_chans = chans;
> +	priv->mbox.txdone_irq = true;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	if (priv->dcfg->init_hw)
> +		priv->dcfg->init_hw(priv);
> +
> +	return mbox_controller_register(&priv->mbox);
> +}
> +
> +static int imx_mu_remove(struct platform_device *pdev) {
> +	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
> +
> +	mbox_controller_unregister(&priv->mbox);
> +	clk_disable_unprepare(priv->clk);
> +
> +	return 0;
> +}
> +
> +
> +static void imx_mu_init_imx7d_a(struct imx_mu_priv *priv) {
> +	/* Set default config */
> +	imx_mu_write(priv, 0, IMX_MU_xCR);
> +}
> +
> +static const struct imx_mu_cfg imx_mu_cfg_imx7d_a = {
> +	.chans = IMX_MU_MAX_CHANS,
> +	.init_hw = imx_mu_init_imx7d_a,
> +};
> +
> +static const struct imx_mu_cfg imx_mu_cfg_imx7d_b = {
> +	.chans = IMX_MU_MAX_CHANS,
> +};
> +
> +static const struct of_device_id imx_mu_dt_ids[] = {
> +	{ .compatible = "fsl,imx7s-mu-a", .data = &imx_mu_cfg_imx7d_a },
> +	{ .compatible = "fsl,imx7s-mu-b", .data = &imx_mu_cfg_imx7d_b },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, imx_mu_dt_ids);
> +
> +static struct platform_driver imx_mu_driver = {
> +	.probe		= imx_mu_probe,
> +	.remove		= imx_mu_remove,
> +	.driver = {
> +		.name	= "imx_mu",
> +		.of_match_table = imx_mu_dt_ids,
> +	},
> +};
> +module_platform_driver(imx_mu_driver);
> +
> +MODULE_AUTHOR("Oleksij Rempel <o.rempel@pengutronix.de>");
> +MODULE_DESCRIPTION("Message Unit driver for i.MX");
> MODULE_LICENSE("GPL
> +v2");
> --
> 2.17.1

Re: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit

[Oleksij Rempel]

[-- Attachment #1.1.1: Type: text/plain, Size: 12193 bytes --]



On 26.06.2018 12:09, A.s. Dong wrote:
>> -----Original Message-----
>> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
>> Sent: Friday, June 15, 2018 5:51 PM
>> To: Shawn Guo <shawnguo@kernel.org>; Fabio Estevam
>> <fabio.estevam@nxp.com>; Rob Herring <robh+dt@kernel.org>; Mark
>> Rutland <mark.rutland@arm.com>; A.s. Dong <aisheng.dong@nxp.com>
>> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; kernel@pengutronix.de;
>> linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-
>> clk@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
>> Subject: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit
>>
>> The Mailbox controller is able to send messages (up to 4 32 bit words)
>> between the endpoints.
>>
>> This driver was tested using the mailbox-test driver sending messages
>> between the Cortex-A7 and the Cortex-M4.
>>
>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>> ---
>>  drivers/mailbox/Kconfig       |   6 +
>>  drivers/mailbox/Makefile      |   2 +
>>  drivers/mailbox/imx-mailbox.c | 288
>> ++++++++++++++++++++++++++++++++++
>>  3 files changed, 296 insertions(+)
>>  create mode 100644 drivers/mailbox/imx-mailbox.c
>>
>> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
>> a2bb27446dce..e1d2738a2e4c 100644
>> --- a/drivers/mailbox/Kconfig
>> +++ b/drivers/mailbox/Kconfig
>> @@ -15,6 +15,12 @@ config ARM_MHU
>>  	  The controller has 3 mailbox channels, the last of which can be
>>  	  used in Secure mode only.
>>
>> +config IMX_MBOX
>> +	tristate "iMX Mailbox"
>> +	depends on SOC_IMX7D || COMPILE_TEST
>> +	help
>> +	  Mailbox implementation for iMX7D Messaging Unit (MU).
>> +
>>  config PLATFORM_MHU
>>  	tristate "Platform MHU Mailbox"
>>  	depends on OF
>> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index
>> cc23c3a43fcd..ba2fe1b6dd62 100644
>> --- a/drivers/mailbox/Makefile
>> +++ b/drivers/mailbox/Makefile
>> @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)	+= mailbox-test.o
>>
>>  obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o
>>
>> +obj-$(CONFIG_IMX_MBOX)	+= imx-mailbox.o
>> +
>>  obj-$(CONFIG_PLATFORM_MHU)	+= platform_mhu.o
>>
>>  obj-$(CONFIG_PL320_MBOX)	+= pl320-ipc.o
>> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
>> new file mode 100644 index 000000000000..e3f621cb1d30
>> --- /dev/null
>> +++ b/drivers/mailbox/imx-mailbox.c
>> @@ -0,0 +1,288 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018 Pengutronix, Oleksij Rempel
>> +<o.rempel@pengutronix.de>  */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mailbox_controller.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +
>> +/* Transmit Register */
>> +#define IMX_MU_xTRn(x)		(0x00 + 4 * (x))
>> +/* Receive Register */
>> +#define IMX_MU_xRRn(x)		(0x10 + 4 * (x))
>> +/* Status Register */
>> +#define IMX_MU_xSR		0x20
>> +#define IMX_MU_xSR_TEn(x)	BIT(20 + (x))
>> +#define IMX_MU_xSR_RFn(x)	BIT(24 + (x))
>> +#define IMX_MU_xSR_BRDIP	BIT(9)
>> +
>> +/* Control Register */
>> +#define IMX_MU_xCR		0x24
>> +/* Transmit Interrupt Enable */
>> +#define IMX_MU_xCR_TIEn(x)	BIT(20 + (x))
>> +/* Receive Interrupt Enable */
>> +#define IMX_MU_xCR_RIEn(x)	BIT(24 + (x))
>> +
>> +#define IMX_MU_MAX_CHANS	4u
>> +
>> +struct imx_mu_priv;
>> +
>> +struct imx_mu_cfg {
>> +	unsigned int		chans;
>> +	void (*init_hw)(struct imx_mu_priv *priv); };
>> +
>> +struct imx_mu_con_priv {
>> +	int			irq;
>> +	unsigned int		bidx;
>> +	unsigned int		idx;
>> +};
>> +
>> +struct imx_mu_priv {
>> +	struct device		*dev;
>> +	const struct imx_mu_cfg	*dcfg;
>> +	void __iomem		*base;
>> +
>> +	struct mbox_controller	mbox;
>> +	struct mbox_chan	mbox_chans[IMX_MU_MAX_CHANS];
>> +
>> +	struct imx_mu_con_priv  con_priv[IMX_MU_MAX_CHANS];
>> +	struct clk		*clk;
>> +};
>> +
>> +static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller *mbox)
>> +{
>> +	return container_of(mbox, struct imx_mu_priv, mbox); }
>> +
>> +static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs) {
>> +	iowrite32(val, priv->base + offs);
>> +}
>> +
>> +static u32 imx_mu_read(struct imx_mu_priv *priv, u32 offs) {
>> +	return ioread32(priv->base + offs);
>> +}
>> +
>> +static u32 imx_mu_rmw(struct imx_mu_priv *priv, u32 offs, u32 set, u32
>> +clr) {
>> +	u32 val;
>> +
>> +	val = imx_mu_read(priv, offs);
>> +	val &= ~clr;
>> +	val |= set;
>> +	imx_mu_write(priv, val, offs);
>> +
>> +	return val;
>> +}
>> +
>> +static irqreturn_t imx_mu_isr(int irq, void *p) {
>> +	struct mbox_chan *chan = p;
>> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>> +	struct imx_mu_con_priv *cp = chan->con_priv;
>> +
>> +	u32 val, dat;
>> +
>> +	val = imx_mu_read(priv, IMX_MU_xSR);
>> +	val &= IMX_MU_xSR_TEn(cp->bidx) | IMX_MU_xSR_RFn(cp->bidx);
>> +	if (!val)
>> +		return IRQ_NONE;
>> +
>> +	if (val & IMX_MU_xSR_TEn(cp->bidx)) {
> 
> I'm wondering whether this can work properly for multi consumers
> at the same time. 
> Because xSR_TEn is 1 by default and four virtual channels actually
> are using the same interrupt. That means channel 1 interrupt may
> cause channel 2 believe it's txdone.
> Have we tested such using?

see imx_mu_send_data()
..... imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->bidx), 0);

> 
>> +		imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp-
>>> bidx));

and here ^^^
TX status is enabled only for send and disabled as soon at transfer is done.

>> +		mbox_chan_txdone(chan, 0);
>> +	}
>> +
>> +	if (val & IMX_MU_xSR_RFn(cp->bidx)) {
>> +		dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
>> +		mbox_chan_received_data(chan, (void *)&dat);
>> +	}
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static bool imx_mu_last_tx_done(struct mbox_chan *chan) {
>> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>> +	struct imx_mu_con_priv *cp = chan->con_priv;
>> +	u32 val;
>> +
>> +	val = imx_mu_read(priv, IMX_MU_xSR);
>> +	/* test if transmit register is empty */
>> +	return (!!(val & IMX_MU_xSR_TEn(cp->bidx))); }
>> +
>> +static int imx_mu_send_data(struct mbox_chan *chan, void *data) {
>> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>> +	struct imx_mu_con_priv *cp = chan->con_priv;
>> +	u32 *arg = data;
>> +
>> +	if (!imx_mu_last_tx_done(chan))
>> +		return -EBUSY;
>> +
>> +	imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx));
>> +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->bidx), 0);
>> +
>> +	return 0;
>> +}
> 
> Since Sascha is requesting to write a generic MU mailbox driver for both
> SCU MU and M4 case, the current way using virtual channels in this patch
> only send one word a time obviously can't fit for SCU MU clients well.
> Do you think if we can refer to TI case to design a generic data transfer
> protocol to allow send multi words which is more close to SCU?

According to your code, you are able to send 1 word message. It means,
your SCU is configured to trigger an interrupt or status update if REG0
was written. The same is true for 2, 3, 4 and 5 word messages.

If the MU configuration would look like you it described, you would be
forced to write always 4 words, even for 1 word message.

> include/linux/soc/ti/ti-msgmgr.h
> struct ti_msgmgr_message {
>         size_t len;
>         u8 *buf;
> };  
> 
> Or we try to support both type transfer protocols in this driver?

Sure. ti-msgmgr.c is a good example. You will probably need reduced
variant of it. It is generic enough to make it useful not only for SCU.

> That may introduce much complexities, personally I'm not quite
> like that.

I expect 50-150 lines of extra code.

> Regards
> Dong Aisheng
> 
>> +
>> +static int imx_mu_startup(struct mbox_chan *chan) {
>> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>> +	struct imx_mu_con_priv *cp = chan->con_priv;
>> +	int ret;
>> +
>> +	ret = request_irq(cp->irq, imx_mu_isr,
>> +			  IRQF_SHARED, "imx_mu_chan", chan);
>> +	if (ret) {
>> +		dev_err(chan->mbox->dev,
>> +			"Unable to acquire IRQ %d\n", cp->irq);
>> +		return ret;
>> +	}
>> +
>> +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->bidx), 0);
>> +
>> +	return 0;
>> +}
>> +
>> +static void imx_mu_shutdown(struct mbox_chan *chan) {
>> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>> +	struct imx_mu_con_priv *cp = chan->con_priv;
>> +
>> +	imx_mu_rmw(priv, IMX_MU_xCR, 0,
>> +		   IMX_MU_xCR_TIEn(cp->bidx) | IMX_MU_xCR_RIEn(cp-
>>> bidx));
>> +
>> +	free_irq(cp->irq, chan);
>> +}
>> +
>> +static const struct mbox_chan_ops imx_mu_ops = {
>> +	.send_data = imx_mu_send_data,
>> +	.startup = imx_mu_startup,
>> +	.shutdown = imx_mu_shutdown,
>> +};
>> +
>> +static int imx_mu_probe(struct platform_device *pdev) {
>> +	struct device *dev = &pdev->dev;
>> +	struct resource *iomem;
>> +	struct imx_mu_priv *priv;
>> +	const struct imx_mu_cfg *dcfg;
>> +	unsigned int i, chans;
>> +	int irq, ret;
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	dcfg = of_device_get_match_data(dev);
>> +	if (!dcfg)
>> +		return -EINVAL;
>> +
>> +	priv->dcfg = dcfg;
>> +	priv->dev = dev;
>> +
>> +	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	priv->base = devm_ioremap_resource(&pdev->dev, iomem);
>> +	if (IS_ERR(priv->base))
>> +		return PTR_ERR(priv->base);
>> +
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq <= 0)
>> +		return irq < 0 ? irq : -EINVAL;
>> +
>> +	priv->clk = devm_clk_get(dev, NULL);
>> +	if (IS_ERR(priv->clk)) {
>> +		if (PTR_ERR(priv->clk) == -ENOENT) {
>> +			priv->clk = NULL;
>> +		} else {
>> +			dev_err(dev, "Failed to get clock\n");
>> +			return PTR_ERR(priv->clk);
>> +		}
>> +	}
>> +
>> +	ret = clk_prepare_enable(priv->clk);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to enable clock\n");
>> +		return ret;
>> +	}
>> +
>> +	chans = min(dcfg->chans, IMX_MU_MAX_CHANS);
>> +	/* Initialize channel identifiers */
>> +	for (i = 0; i < chans; i++) {
>> +		struct imx_mu_con_priv *cp = &priv->con_priv[i];
>> +
>> +		cp->bidx = 3 - i;
>> +		cp->idx = i;
>> +		cp->irq = irq;
>> +		priv->mbox_chans[i].con_priv = cp;
>> +	}
>> +
>> +	priv->mbox.dev = dev;
>> +	priv->mbox.ops = &imx_mu_ops;
>> +	priv->mbox.chans = priv->mbox_chans;
>> +	priv->mbox.num_chans = chans;
>> +	priv->mbox.txdone_irq = true;
>> +
>> +	platform_set_drvdata(pdev, priv);
>> +
>> +	if (priv->dcfg->init_hw)
>> +		priv->dcfg->init_hw(priv);
>> +
>> +	return mbox_controller_register(&priv->mbox);
>> +}
>> +
>> +static int imx_mu_remove(struct platform_device *pdev) {
>> +	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
>> +
>> +	mbox_controller_unregister(&priv->mbox);
>> +	clk_disable_unprepare(priv->clk);
>> +
>> +	return 0;
>> +}
>> +
>> +
>> +static void imx_mu_init_imx7d_a(struct imx_mu_priv *priv) {
>> +	/* Set default config */
>> +	imx_mu_write(priv, 0, IMX_MU_xCR);
>> +}
>> +
>> +static const struct imx_mu_cfg imx_mu_cfg_imx7d_a = {
>> +	.chans = IMX_MU_MAX_CHANS,
>> +	.init_hw = imx_mu_init_imx7d_a,
>> +};
>> +
>> +static const struct imx_mu_cfg imx_mu_cfg_imx7d_b = {
>> +	.chans = IMX_MU_MAX_CHANS,
>> +};
>> +
>> +static const struct of_device_id imx_mu_dt_ids[] = {
>> +	{ .compatible = "fsl,imx7s-mu-a", .data = &imx_mu_cfg_imx7d_a },
>> +	{ .compatible = "fsl,imx7s-mu-b", .data = &imx_mu_cfg_imx7d_b },
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(of, imx_mu_dt_ids);
>> +
>> +static struct platform_driver imx_mu_driver = {
>> +	.probe		= imx_mu_probe,
>> +	.remove		= imx_mu_remove,
>> +	.driver = {
>> +		.name	= "imx_mu",
>> +		.of_match_table = imx_mu_dt_ids,
>> +	},
>> +};
>> +module_platform_driver(imx_mu_driver);
>> +
>> +MODULE_AUTHOR("Oleksij Rempel <o.rempel@pengutronix.de>");
>> +MODULE_DESCRIPTION("Message Unit driver for i.MX");
>> MODULE_LICENSE("GPL
>> +v2");
>> --
>> 2.17.1
> 
> 
> 


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit

[A.s. Dong]

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> Sent: Tuesday, June 26, 2018 6:56 PM
> To: A.s. Dong <aisheng.dong@nxp.com>; Shawn Guo
> <shawnguo@kernel.org>; Fabio Estevam <fabio.estevam@nxp.com>; Rob
> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>
> Cc: devicetree@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; linux-
> arm-kernel@lists.infradead.org; kernel@pengutronix.de; linux-
> clk@vger.kernel.org
> Subject: Re: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit
> 
> 
> 
> On 26.06.2018 12:09, A.s. Dong wrote:
> >> -----Original Message-----
> >> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> >> Sent: Friday, June 15, 2018 5:51 PM
> >> To: Shawn Guo <shawnguo@kernel.org>; Fabio Estevam
> >> <fabio.estevam@nxp.com>; Rob Herring <robh+dt@kernel.org>; Mark
> >> Rutland <mark.rutland@arm.com>; A.s. Dong <aisheng.dong@nxp.com>
> >> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; kernel@pengutronix.de;
> >> linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org;
> >> linux- clk@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> >> Subject: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging
> >> unit
> >>
> >> The Mailbox controller is able to send messages (up to 4 32 bit
> >> words) between the endpoints.
> >>
> >> This driver was tested using the mailbox-test driver sending messages
> >> between the Cortex-A7 and the Cortex-M4.
> >>
> >> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> >> ---
> >>  drivers/mailbox/Kconfig       |   6 +
> >>  drivers/mailbox/Makefile      |   2 +
> >>  drivers/mailbox/imx-mailbox.c | 288
> >> ++++++++++++++++++++++++++++++++++
> >>  3 files changed, 296 insertions(+)
> >>  create mode 100644 drivers/mailbox/imx-mailbox.c
> >>
> >> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> >> a2bb27446dce..e1d2738a2e4c 100644
> >> --- a/drivers/mailbox/Kconfig
> >> +++ b/drivers/mailbox/Kconfig
> >> @@ -15,6 +15,12 @@ config ARM_MHU
> >>  	  The controller has 3 mailbox channels, the last of which can be
> >>  	  used in Secure mode only.
> >>
> >> +config IMX_MBOX
> >> +	tristate "iMX Mailbox"
> >> +	depends on SOC_IMX7D || COMPILE_TEST
> >> +	help
> >> +	  Mailbox implementation for iMX7D Messaging Unit (MU).
> >> +
> >>  config PLATFORM_MHU
> >>  	tristate "Platform MHU Mailbox"
> >>  	depends on OF
> >> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> >> index
> >> cc23c3a43fcd..ba2fe1b6dd62 100644
> >> --- a/drivers/mailbox/Makefile
> >> +++ b/drivers/mailbox/Makefile
> >> @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)	+= mailbox-test.o
> >>
> >>  obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o
> >>
> >> +obj-$(CONFIG_IMX_MBOX)	+= imx-mailbox.o
> >> +
> >>  obj-$(CONFIG_PLATFORM_MHU)	+= platform_mhu.o
> >>
> >>  obj-$(CONFIG_PL320_MBOX)	+= pl320-ipc.o
> >> diff --git a/drivers/mailbox/imx-mailbox.c
> >> b/drivers/mailbox/imx-mailbox.c new file mode 100644 index
> >> 000000000000..e3f621cb1d30
> >> --- /dev/null
> >> +++ b/drivers/mailbox/imx-mailbox.c
> >> @@ -0,0 +1,288 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright (c) 2018 Pengutronix, Oleksij Rempel
> >> +<o.rempel@pengutronix.de>  */
> >> +
> >> +#include <linux/clk.h>
> >> +#include <linux/interrupt.h>
> >> +#include <linux/io.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/mailbox_controller.h> #include <linux/module.h>
> >> +#include <linux/of_device.h>
> >> +
> >> +/* Transmit Register */
> >> +#define IMX_MU_xTRn(x)		(0x00 + 4 * (x))
> >> +/* Receive Register */
> >> +#define IMX_MU_xRRn(x)		(0x10 + 4 * (x))
> >> +/* Status Register */
> >> +#define IMX_MU_xSR		0x20
> >> +#define IMX_MU_xSR_TEn(x)	BIT(20 + (x))
> >> +#define IMX_MU_xSR_RFn(x)	BIT(24 + (x))
> >> +#define IMX_MU_xSR_BRDIP	BIT(9)
> >> +
> >> +/* Control Register */
> >> +#define IMX_MU_xCR		0x24
> >> +/* Transmit Interrupt Enable */
> >> +#define IMX_MU_xCR_TIEn(x)	BIT(20 + (x))
> >> +/* Receive Interrupt Enable */
> >> +#define IMX_MU_xCR_RIEn(x)	BIT(24 + (x))
> >> +
> >> +#define IMX_MU_MAX_CHANS	4u
> >> +
> >> +struct imx_mu_priv;
> >> +
> >> +struct imx_mu_cfg {
> >> +	unsigned int		chans;
> >> +	void (*init_hw)(struct imx_mu_priv *priv); };
> >> +
> >> +struct imx_mu_con_priv {
> >> +	int			irq;
> >> +	unsigned int		bidx;
> >> +	unsigned int		idx;
> >> +};
> >> +
> >> +struct imx_mu_priv {
> >> +	struct device		*dev;
> >> +	const struct imx_mu_cfg	*dcfg;
> >> +	void __iomem		*base;
> >> +
> >> +	struct mbox_controller	mbox;
> >> +	struct mbox_chan	mbox_chans[IMX_MU_MAX_CHANS];
> >> +
> >> +	struct imx_mu_con_priv  con_priv[IMX_MU_MAX_CHANS];
> >> +	struct clk		*clk;
> >> +};
> >> +
> >> +static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller
> >> +*mbox) {
> >> +	return container_of(mbox, struct imx_mu_priv, mbox); }
> >> +
> >> +static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs) {
> >> +	iowrite32(val, priv->base + offs);
> >> +}
> >> +
> >> +static u32 imx_mu_read(struct imx_mu_priv *priv, u32 offs) {
> >> +	return ioread32(priv->base + offs); }
> >> +
> >> +static u32 imx_mu_rmw(struct imx_mu_priv *priv, u32 offs, u32 set,
> >> +u32
> >> +clr) {
> >> +	u32 val;
> >> +
> >> +	val = imx_mu_read(priv, offs);
> >> +	val &= ~clr;
> >> +	val |= set;
> >> +	imx_mu_write(priv, val, offs);
> >> +
> >> +	return val;
> >> +}
> >> +
> >> +static irqreturn_t imx_mu_isr(int irq, void *p) {
> >> +	struct mbox_chan *chan = p;
> >> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> >> +	struct imx_mu_con_priv *cp = chan->con_priv;
> >> +
> >> +	u32 val, dat;
> >> +
> >> +	val = imx_mu_read(priv, IMX_MU_xSR);
> >> +	val &= IMX_MU_xSR_TEn(cp->bidx) | IMX_MU_xSR_RFn(cp->bidx);
> >> +	if (!val)
> >> +		return IRQ_NONE;
> >> +
> >> +	if (val & IMX_MU_xSR_TEn(cp->bidx)) {
> >
> > I'm wondering whether this can work properly for multi consumers at
> > the same time.
> > Because xSR_TEn is 1 by default and four virtual channels actually are
> > using the same interrupt. That means channel 1 interrupt may cause
> > channel 2 believe it's txdone.
> > Have we tested such using?
> 
> see imx_mu_send_data()
> ..... imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->bidx), 0);
> 
> >
> >> +		imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp-
> >>> bidx));
> 
> and here ^^^
> TX status is enabled only for send and disabled as soon at transfer is done.
> 

That controls the interrupt enable signal, I'm not sure if the status bit
Won't be set if not enable interrupt. I've not tested it.

Have you double checked it?

For n = {0, 1, 2, 3} Processor A Transmit Register n Empty. (Read-only)
• The TEn bit is set to “1” after the BRRn register is read on the Processor B-side.
• After the TEn bit is set to “1”, the TEn bit signals the Processor A-side that the ATRn register is
ready to be written on the Processor A-side, and a Transmit n interrupt is issued on the Processor
A-side (if the TEn bit in the ACR register is set to “1”).
...

> >> +		mbox_chan_txdone(chan, 0);
> >> +	}
> >> +
> >> +	if (val & IMX_MU_xSR_RFn(cp->bidx)) {
> >> +		dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
> >> +		mbox_chan_received_data(chan, (void *)&dat);
> >> +	}
> >> +
> >> +	return IRQ_HANDLED;
> >> +}
> >> +
> >> +static bool imx_mu_last_tx_done(struct mbox_chan *chan) {
> >> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> >> +	struct imx_mu_con_priv *cp = chan->con_priv;
> >> +	u32 val;
> >> +
> >> +	val = imx_mu_read(priv, IMX_MU_xSR);
> >> +	/* test if transmit register is empty */
> >> +	return (!!(val & IMX_MU_xSR_TEn(cp->bidx))); }
> >> +
> >> +static int imx_mu_send_data(struct mbox_chan *chan, void *data) {
> >> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> >> +	struct imx_mu_con_priv *cp = chan->con_priv;
> >> +	u32 *arg = data;
> >> +
> >> +	if (!imx_mu_last_tx_done(chan))
> >> +		return -EBUSY;
> >> +
> >> +	imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx));
> >> +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->bidx), 0);
> >> +
> >> +	return 0;
> >> +}
> >
> > Since Sascha is requesting to write a generic MU mailbox driver for
> > both SCU MU and M4 case, the current way using virtual channels in
> > this patch only send one word a time obviously can't fit for SCU MU clients
> well.
> > Do you think if we can refer to TI case to design a generic data
> > transfer protocol to allow send multi words which is more close to SCU?
> 
> According to your code, you are able to send 1 word message. It means, your
> SCU is configured to trigger an interrupt or status update if REG0 was written.
> The same is true for 2, 3, 4 and 5 word messages.
> 

SCU is interrupt driven already for the first word.
We do can send word one by one but the performance would be terrible
comparing to write 4 a time.

> If the MU configuration would look like you it described, you would be forced
> to write always 4 words, even for 1 word message.
> 
> > include/linux/soc/ti/ti-msgmgr.h
> > struct ti_msgmgr_message {
> >         size_t len;
> >         u8 *buf;
> > };
> >
> > Or we try to support both type transfer protocols in this driver?
> 
> Sure. ti-msgmgr.c is a good example. You will probably need reduced variant
> of it. It is generic enough to make it useful not only for SCU.
> 

Sascha needs a common design for both M4 and SCU.If decide to do that,
you probably need update this patch as well.

But even doing like TI style, it still need hack for SCU as the data size offset
Is different. However, that would be a much smaller hack than doing based
On this driver.

> > That may introduce much complexities, personally I'm not quite like
> > that.
> 
> I expect 50-150 lines of extra code.
> 

Hope that could be true.
Do you have suggestion on how to keep two type protocol co-exist
If you thought that would work without two much extra complexity?
Have you tried it already based this driver?

Regards
Dong Aisheng

> > Regards
> > Dong Aisheng
> >
> >> +
> >> +static int imx_mu_startup(struct mbox_chan *chan) {
> >> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> >> +	struct imx_mu_con_priv *cp = chan->con_priv;
> >> +	int ret;
> >> +
> >> +	ret = request_irq(cp->irq, imx_mu_isr,
> >> +			  IRQF_SHARED, "imx_mu_chan", chan);
> >> +	if (ret) {
> >> +		dev_err(chan->mbox->dev,
> >> +			"Unable to acquire IRQ %d\n", cp->irq);
> >> +		return ret;
> >> +	}
> >> +
> >> +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->bidx), 0);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static void imx_mu_shutdown(struct mbox_chan *chan) {
> >> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> >> +	struct imx_mu_con_priv *cp = chan->con_priv;
> >> +
> >> +	imx_mu_rmw(priv, IMX_MU_xCR, 0,
> >> +		   IMX_MU_xCR_TIEn(cp->bidx) | IMX_MU_xCR_RIEn(cp-
> >>> bidx));
> >> +
> >> +	free_irq(cp->irq, chan);
> >> +}
> >> +
> >> +static const struct mbox_chan_ops imx_mu_ops = {
> >> +	.send_data = imx_mu_send_data,
> >> +	.startup = imx_mu_startup,
> >> +	.shutdown = imx_mu_shutdown,
> >> +};
> >> +
> >> +static int imx_mu_probe(struct platform_device *pdev) {
> >> +	struct device *dev = &pdev->dev;
> >> +	struct resource *iomem;
> >> +	struct imx_mu_priv *priv;
> >> +	const struct imx_mu_cfg *dcfg;
> >> +	unsigned int i, chans;
> >> +	int irq, ret;
> >> +
> >> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >> +	if (!priv)
> >> +		return -ENOMEM;
> >> +
> >> +	dcfg = of_device_get_match_data(dev);
> >> +	if (!dcfg)
> >> +		return -EINVAL;
> >> +
> >> +	priv->dcfg = dcfg;
> >> +	priv->dev = dev;
> >> +
> >> +	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> +	priv->base = devm_ioremap_resource(&pdev->dev, iomem);
> >> +	if (IS_ERR(priv->base))
> >> +		return PTR_ERR(priv->base);
> >> +
> >> +	irq = platform_get_irq(pdev, 0);
> >> +	if (irq <= 0)
> >> +		return irq < 0 ? irq : -EINVAL;
> >> +
> >> +	priv->clk = devm_clk_get(dev, NULL);
> >> +	if (IS_ERR(priv->clk)) {
> >> +		if (PTR_ERR(priv->clk) == -ENOENT) {
> >> +			priv->clk = NULL;
> >> +		} else {
> >> +			dev_err(dev, "Failed to get clock\n");
> >> +			return PTR_ERR(priv->clk);
> >> +		}
> >> +	}
> >> +
> >> +	ret = clk_prepare_enable(priv->clk);
> >> +	if (ret) {
> >> +		dev_err(dev, "Failed to enable clock\n");
> >> +		return ret;
> >> +	}
> >> +
> >> +	chans = min(dcfg->chans, IMX_MU_MAX_CHANS);
> >> +	/* Initialize channel identifiers */
> >> +	for (i = 0; i < chans; i++) {
> >> +		struct imx_mu_con_priv *cp = &priv->con_priv[i];
> >> +
> >> +		cp->bidx = 3 - i;
> >> +		cp->idx = i;
> >> +		cp->irq = irq;
> >> +		priv->mbox_chans[i].con_priv = cp;
> >> +	}
> >> +
> >> +	priv->mbox.dev = dev;
> >> +	priv->mbox.ops = &imx_mu_ops;
> >> +	priv->mbox.chans = priv->mbox_chans;
> >> +	priv->mbox.num_chans = chans;
> >> +	priv->mbox.txdone_irq = true;
> >> +
> >> +	platform_set_drvdata(pdev, priv);
> >> +
> >> +	if (priv->dcfg->init_hw)
> >> +		priv->dcfg->init_hw(priv);
> >> +
> >> +	return mbox_controller_register(&priv->mbox);
> >> +}
> >> +
> >> +static int imx_mu_remove(struct platform_device *pdev) {
> >> +	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
> >> +
> >> +	mbox_controller_unregister(&priv->mbox);
> >> +	clk_disable_unprepare(priv->clk);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +
> >> +static void imx_mu_init_imx7d_a(struct imx_mu_priv *priv) {
> >> +	/* Set default config */
> >> +	imx_mu_write(priv, 0, IMX_MU_xCR);
> >> +}
> >> +
> >> +static const struct imx_mu_cfg imx_mu_cfg_imx7d_a = {
> >> +	.chans = IMX_MU_MAX_CHANS,
> >> +	.init_hw = imx_mu_init_imx7d_a,
> >> +};
> >> +
> >> +static const struct imx_mu_cfg imx_mu_cfg_imx7d_b = {
> >> +	.chans = IMX_MU_MAX_CHANS,
> >> +};
> >> +
> >> +static const struct of_device_id imx_mu_dt_ids[] = {
> >> +	{ .compatible = "fsl,imx7s-mu-a", .data = &imx_mu_cfg_imx7d_a },
> >> +	{ .compatible = "fsl,imx7s-mu-b", .data = &imx_mu_cfg_imx7d_b },
> >> +	{ },
> >> +};
> >> +MODULE_DEVICE_TABLE(of, imx_mu_dt_ids);
> >> +
> >> +static struct platform_driver imx_mu_driver = {
> >> +	.probe		= imx_mu_probe,
> >> +	.remove		= imx_mu_remove,
> >> +	.driver = {
> >> +		.name	= "imx_mu",
> >> +		.of_match_table = imx_mu_dt_ids,
> >> +	},
> >> +};
> >> +module_platform_driver(imx_mu_driver);
> >> +
> >> +MODULE_AUTHOR("Oleksij Rempel <o.rempel@pengutronix.de>");
> >> +MODULE_DESCRIPTION("Message Unit driver for i.MX");
> >> MODULE_LICENSE("GPL
> >> +v2");
> >> --
> >> 2.17.1
> >
> >
> >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit

[Sascha Hauer]

On Tue, Jun 26, 2018 at 10:09:14AM +0000, A.s. Dong wrote:
> Since Sascha is requesting to write a generic MU mailbox driver for both
> SCU MU and M4 case, the current way using virtual channels in this patch
> only send one word a time obviously can't fit for SCU MU clients well.
> Do you think if we can refer to TI case to design a generic data transfer
> protocol to allow send multi words which is more close to SCU?
> include/linux/soc/ti/ti-msgmgr.h
> struct ti_msgmgr_message {
>         size_t len;
>         u8 *buf;
> };  
> 
> Or we try to support both type transfer protocols in this driver?
> That may introduce much complexities, personally I'm not quite
> like that.

To give you an idea what I am suggesting see the following patch.
It implements SCU mode ontop of Oleksijs patch. Untested of course
and as said, the mailbox framework lacks some way of synchronous
receive, that would still have to be implemented and Jassi might
have his own ideas how this could be done, but it doesn't add much
complexity.

Sascha


------------------------------8<--------------------------------

>From fad77078a1f1b5719fcc3c1ab789ce9cca4c9212 Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Tue, 26 Jun 2018 15:07:04 +0200
Subject: [PATCH] mailbox: imx-mu: Implement SCU mode

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mailbox/imx-mailbox.c        | 89 +++++++++++++++++++++++++++-
 include/dt-bindings/mailbox/imx-mu.h | 10 ++++
 2 files changed, 98 insertions(+), 1 deletion(-)
 create mode 100644 include/dt-bindings/mailbox/imx-mu.h

diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
index e3f621cb1d30..a4803d1eae1d 100644
--- a/drivers/mailbox/imx-mailbox.c
+++ b/drivers/mailbox/imx-mailbox.c
@@ -10,6 +10,8 @@
 #include <linux/mailbox_controller.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
+#include <linux/iopoll.h>
+#include <dt-bindings/mailbox/imx-mu.h>
 
 /* Transmit Register */
 #define IMX_MU_xTRn(x)		(0x00 + 4 * (x))
@@ -28,7 +30,7 @@
 /* Receive Interrupt Enable */
 #define IMX_MU_xCR_RIEn(x)	BIT(24 + (x))
 
-#define IMX_MU_MAX_CHANS	4u
+#define IMX_MU_MAX_CHANS	5u
 
 struct imx_mu_priv;
 
@@ -119,12 +121,80 @@ static bool imx_mu_last_tx_done(struct mbox_chan *chan)
 	return (!!(val & IMX_MU_xSR_TEn(cp->bidx)));
 }
 
+// Take from some include file
+struct sc_rpc_msg {
+	uint8_t version;
+	uint8_t size;
+	uint8_t svc;
+	uint8_t func;
+};
+
+#define MU_DATA_TIME_OUT_US    (100 * USEC_PER_MSEC)
+
+static int imx_mu_scu_send_data(struct imx_mu_priv *priv, void *data)
+{
+	struct sc_rpc_msg *msg = data;
+	uint32_t *msg_raw = data;
+	int i, ret;
+
+	for (i = 0; i < msg->size + 1; i++) {
+		int index = i % 4;
+		uint32_t asr;
+
+		/* Wait TX register to be empty. */
+		ret = readl_poll_timeout_atomic(priv->base + IMX_MU_xSR, asr,
+						asr & IMX_MU_xSR_TEn(4 - index),
+						0, MU_DATA_TIME_OUT_US);
+		if (ret)
+			goto out;
+
+		writel(msg_raw[i], priv->base + IMX_MU_xTRn(4 - index));
+	}
+
+	ret = 0;
+out:
+	mbox_chan_txdone(chan, ret);
+	return ret;
+}
+
+static int imx_mu_scu_receive_data(struct imx_mu_priv *priv, void *data)
+{
+	struct sc_rpc_msg *msg = data;
+	uint32_t *msg_raw = data;
+	int i, ret, size = 8;
+
+	for (i = 0; i < size; i++) {
+		int index = i % 4;
+		uint32_t asr;
+
+		/* Wait TX register to be empty. */
+		ret = readl_poll_timeout_atomic(priv->base + IMX_MU_xSR, asr,
+						asr & IMX_MU_xSR_RFn(4 - index),
+						0, MU_DATA_TIME_OUT_US);
+		if (ret)
+			return ret;
+
+		msg_raw[i] = readl(priv->base + IMX_MU_xRRn(4 - index));
+
+		if (i == 0) {
+			size = msg->size;
+			if (size > 7)
+				return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
 static int imx_mu_send_data(struct mbox_chan *chan, void *data)
 {
 	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
 	struct imx_mu_con_priv *cp = chan->con_priv;
 	u32 *arg = data;
 
+	if (cp->idx == IMX_MU_CHANNEL_IMX8_SCU)
+		return imx_mu_scu_send_data(priv, data);
+
 	if (!imx_mu_last_tx_done(chan))
 		return -EBUSY;
 
@@ -134,6 +204,17 @@ static int imx_mu_send_data(struct mbox_chan *chan, void *data)
 	return 0;
 }
 
+static int imx_mu_receive_data(struct mbox_chan *chan, void *data)
+{
+	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
+	struct imx_mu_con_priv *cp = chan->con_priv;
+
+	if (cp->idx == IMX_MU_CHANNEL_IMX8_SCU)
+		return imx_mu_scu_receive_data(priv, data);
+
+	return -EINVAL;
+}
+
 static int imx_mu_startup(struct mbox_chan *chan)
 {
 	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
@@ -166,6 +247,12 @@ static void imx_mu_shutdown(struct mbox_chan *chan)
 
 static const struct mbox_chan_ops imx_mu_ops = {
 	.send_data = imx_mu_send_data,
+	/*
+	 * FIXME: This has to be implemented in the mailbox framework.
+	 * Currently there is only support for asynchronous read using
+	 * callbacks which is not suitable for the SCU usecase.
+	 */
+//	.synchronous_read = imx_mu_receive_data,
 	.startup = imx_mu_startup,
 	.shutdown = imx_mu_shutdown,
 };
diff --git a/include/dt-bindings/mailbox/imx-mu.h b/include/dt-bindings/mailbox/imx-mu.h
new file mode 100644
index 000000000000..4836266a7e57
--- /dev/null
+++ b/include/dt-bindings/mailbox/imx-mu.h
@@ -0,0 +1,10 @@
+#ifndef __DT_BINDINGS_MAILBOX_IMX_MU_H
+#define __DT_BINDINGS_MAILBOX_IMX_MU_H
+
+#define IMX_MU_CHANNEL0		0
+#define IMX_MU_CHANNEL1		1
+#define IMX_MU_CHANNEL2		2
+#define IMX_MU_CHANNEL3		3
+#define IMX_MU_CHANNEL_IMX8_SCU 4
+
+#endif /* __DT_BINDINGS_MAILBOX_IMX_MU_H */
-- 
2.17.1

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit

[A.s. Dong]

> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer@pengutronix.de]
> Sent: Tuesday, June 26, 2018 9:23 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; Shawn Guo
> <shawnguo@kernel.org>; Fabio Estevam <fabio.estevam@nxp.com>; Rob
> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>;
> devicetree@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; linux-arm-
> kernel@lists.infradead.org; kernel@pengutronix.de; linux-
> clk@vger.kernel.org
> Subject: Re: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit
> 
> On Tue, Jun 26, 2018 at 10:09:14AM +0000, A.s. Dong wrote:
> > Since Sascha is requesting to write a generic MU mailbox driver for
> > both SCU MU and M4 case, the current way using virtual channels in
> > this patch only send one word a time obviously can't fit for SCU MU clients
> well.
> > Do you think if we can refer to TI case to design a generic data
> > transfer protocol to allow send multi words which is more close to SCU?
> > include/linux/soc/ti/ti-msgmgr.h
> > struct ti_msgmgr_message {
> >         size_t len;
> >         u8 *buf;
> > };
> >
> > Or we try to support both type transfer protocols in this driver?
> > That may introduce much complexities, personally I'm not quite like
> > that.
> 
> To give you an idea what I am suggesting see the following patch.
> It implements SCU mode ontop of Oleksijs patch. Untested of course and as
> said, the mailbox framework lacks some way of synchronous receive, that
> would still have to be implemented and Jassi might have his own ideas how
> this could be done, but it doesn't add much complexity.
> 

Thanks for this.
We did similar thing as I described in another mail.
The different is we use interrupt and hrtimer to polling for rx which is the most
complicated part (Tx is quite simple). Be noted that for this demo code, we do
not need enable RX interrupt if using synchronize polling.

If the mailbox framework can support synchronize polling, that would
be good and will be much the same as we did in library way. Then we
obviously can switch to mailbox way for SCU as well.

But as I repeat before, from your demo code, you can also see those bits are
much specific to SCU MU protocol which I don't think is quite suitable to be
merged into a generic MU driver, except we can find a much common way for
handling both SCU MU and M4 case (e.g. change to TI style data transfer protocol).

But anyway, I will try it as you guys insist. Then we do not need stop at this point
for the whole QXP upstream work.

Regards
Dong Aisheng

> Sascha
> 
> 
> ------------------------------8<--------------------------------
> 
> From fad77078a1f1b5719fcc3c1ab789ce9cca4c9212 Mon Sep 17 00:00:00 2001
> From: Sascha Hauer <s.hauer@pengutronix.de>
> Date: Tue, 26 Jun 2018 15:07:04 +0200
> Subject: [PATCH] mailbox: imx-mu: Implement SCU mode
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/mailbox/imx-mailbox.c        | 89 +++++++++++++++++++++++++++-
>  include/dt-bindings/mailbox/imx-mu.h | 10 ++++
>  2 files changed, 98 insertions(+), 1 deletion(-)  create mode 100644
> include/dt-bindings/mailbox/imx-mu.h
> 
> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> index e3f621cb1d30..a4803d1eae1d 100644
> --- a/drivers/mailbox/imx-mailbox.c
> +++ b/drivers/mailbox/imx-mailbox.c
> @@ -10,6 +10,8 @@
>  #include <linux/mailbox_controller.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
> +#include <linux/iopoll.h>
> +#include <dt-bindings/mailbox/imx-mu.h>
> 
>  /* Transmit Register */
>  #define IMX_MU_xTRn(x)		(0x00 + 4 * (x))
> @@ -28,7 +30,7 @@
>  /* Receive Interrupt Enable */
>  #define IMX_MU_xCR_RIEn(x)	BIT(24 + (x))
> 
> -#define IMX_MU_MAX_CHANS	4u
> +#define IMX_MU_MAX_CHANS	5u
> 
>  struct imx_mu_priv;
> 
> @@ -119,12 +121,80 @@ static bool imx_mu_last_tx_done(struct
> mbox_chan *chan)
>  	return (!!(val & IMX_MU_xSR_TEn(cp->bidx)));  }
> 
> +// Take from some include file
> +struct sc_rpc_msg {
> +	uint8_t version;
> +	uint8_t size;
> +	uint8_t svc;
> +	uint8_t func;
> +};
> +
> +#define MU_DATA_TIME_OUT_US    (100 * USEC_PER_MSEC)
> +
> +static int imx_mu_scu_send_data(struct imx_mu_priv *priv, void *data) {
> +	struct sc_rpc_msg *msg = data;
> +	uint32_t *msg_raw = data;
> +	int i, ret;
> +
> +	for (i = 0; i < msg->size + 1; i++) {
> +		int index = i % 4;
> +		uint32_t asr;
> +
> +		/* Wait TX register to be empty. */
> +		ret = readl_poll_timeout_atomic(priv->base + IMX_MU_xSR,
> asr,
> +						asr & IMX_MU_xSR_TEn(4 -
> index),
> +						0,
> MU_DATA_TIME_OUT_US);
> +		if (ret)
> +			goto out;
> +
> +		writel(msg_raw[i], priv->base + IMX_MU_xTRn(4 - index));
> +	}
> +
> +	ret = 0;
> +out:
> +	mbox_chan_txdone(chan, ret);
> +	return ret;
> +}
> +
> +static int imx_mu_scu_receive_data(struct imx_mu_priv *priv, void
> +*data) {
> +	struct sc_rpc_msg *msg = data;
> +	uint32_t *msg_raw = data;
> +	int i, ret, size = 8;
> +
> +	for (i = 0; i < size; i++) {
> +		int index = i % 4;
> +		uint32_t asr;
> +
> +		/* Wait TX register to be empty. */
> +		ret = readl_poll_timeout_atomic(priv->base + IMX_MU_xSR,
> asr,
> +						asr & IMX_MU_xSR_RFn(4 -
> index),
> +						0,
> MU_DATA_TIME_OUT_US);
> +		if (ret)
> +			return ret;
> +
> +		msg_raw[i] = readl(priv->base + IMX_MU_xRRn(4 - index));
> +
> +		if (i == 0) {
> +			size = msg->size;
> +			if (size > 7)
> +				return -EINVAL;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int imx_mu_send_data(struct mbox_chan *chan, void *data)  {
>  	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>  	struct imx_mu_con_priv *cp = chan->con_priv;
>  	u32 *arg = data;
> 
> +	if (cp->idx == IMX_MU_CHANNEL_IMX8_SCU)
> +		return imx_mu_scu_send_data(priv, data);
> +
>  	if (!imx_mu_last_tx_done(chan))
>  		return -EBUSY;
> 
> @@ -134,6 +204,17 @@ static int imx_mu_send_data(struct mbox_chan
> *chan, void *data)
>  	return 0;
>  }
> 
> +static int imx_mu_receive_data(struct mbox_chan *chan, void *data) {
> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +	struct imx_mu_con_priv *cp = chan->con_priv;
> +
> +	if (cp->idx == IMX_MU_CHANNEL_IMX8_SCU)
> +		return imx_mu_scu_receive_data(priv, data);
> +
> +	return -EINVAL;
> +}
> +
>  static int imx_mu_startup(struct mbox_chan *chan)  {
>  	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox); @@ -
> 166,6 +247,12 @@ static void imx_mu_shutdown(struct mbox_chan *chan)
> 
>  static const struct mbox_chan_ops imx_mu_ops = {
>  	.send_data = imx_mu_send_data,
> +	/*
> +	 * FIXME: This has to be implemented in the mailbox framework.
> +	 * Currently there is only support for asynchronous read using
> +	 * callbacks which is not suitable for the SCU usecase.
> +	 */
> +//	.synchronous_read = imx_mu_receive_data,
>  	.startup = imx_mu_startup,
>  	.shutdown = imx_mu_shutdown,
>  };
> diff --git a/include/dt-bindings/mailbox/imx-mu.h b/include/dt-
> bindings/mailbox/imx-mu.h
> new file mode 100644
> index 000000000000..4836266a7e57
> --- /dev/null
> +++ b/include/dt-bindings/mailbox/imx-mu.h
> @@ -0,0 +1,10 @@
> +#ifndef __DT_BINDINGS_MAILBOX_IMX_MU_H
> +#define __DT_BINDINGS_MAILBOX_IMX_MU_H
> +
> +#define IMX_MU_CHANNEL0		0
> +#define IMX_MU_CHANNEL1		1
> +#define IMX_MU_CHANNEL2		2
> +#define IMX_MU_CHANNEL3		3
> +#define IMX_MU_CHANNEL_IMX8_SCU 4
> +
> +#endif /* __DT_BINDINGS_MAILBOX_IMX_MU_H */
> --
> 2.17.1
> 
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 |
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> w.pengutronix.de%2F&data=02%7C01%7Caisheng.dong%40nxp.com%7C80f
> 356f57b1546baa07c08d5db67fe40%7C686ea1d3bc2b4c6fa92cd99c5c301635%7
> C0%7C0%7C636656162050199526&sdata=D0CE9mVcj8CakYwAET0aVUpGMe
> Ulz8thqzRfDgGlzkA%3D&reserved=0  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit

[Oleksij Rempel]

[-- Attachment #1.1.1: Type: text/plain, Size: 15295 bytes --]



On 26.06.2018 14:07, A.s. Dong wrote:
>> -----Original Message-----
>> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
>> Sent: Tuesday, June 26, 2018 6:56 PM
>> To: A.s. Dong <aisheng.dong@nxp.com>; Shawn Guo
>> <shawnguo@kernel.org>; Fabio Estevam <fabio.estevam@nxp.com>; Rob
>> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>
>> Cc: devicetree@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; linux-
>> arm-kernel@lists.infradead.org; kernel@pengutronix.de; linux-
>> clk@vger.kernel.org
>> Subject: Re: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit
>>
>>
>>
>> On 26.06.2018 12:09, A.s. Dong wrote:
>>>> -----Original Message-----
>>>> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
>>>> Sent: Friday, June 15, 2018 5:51 PM
>>>> To: Shawn Guo <shawnguo@kernel.org>; Fabio Estevam
>>>> <fabio.estevam@nxp.com>; Rob Herring <robh+dt@kernel.org>; Mark
>>>> Rutland <mark.rutland@arm.com>; A.s. Dong <aisheng.dong@nxp.com>
>>>> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; kernel@pengutronix.de;
>>>> linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org;
>>>> linux- clk@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
>>>> Subject: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging
>>>> unit
>>>>
>>>> The Mailbox controller is able to send messages (up to 4 32 bit
>>>> words) between the endpoints.
>>>>
>>>> This driver was tested using the mailbox-test driver sending messages
>>>> between the Cortex-A7 and the Cortex-M4.
>>>>
>>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>>>> ---
>>>>  drivers/mailbox/Kconfig       |   6 +
>>>>  drivers/mailbox/Makefile      |   2 +
>>>>  drivers/mailbox/imx-mailbox.c | 288
>>>> ++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 296 insertions(+)
>>>>  create mode 100644 drivers/mailbox/imx-mailbox.c
>>>>
>>>> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
>>>> a2bb27446dce..e1d2738a2e4c 100644
>>>> --- a/drivers/mailbox/Kconfig
>>>> +++ b/drivers/mailbox/Kconfig
>>>> @@ -15,6 +15,12 @@ config ARM_MHU
>>>>  	  The controller has 3 mailbox channels, the last of which can be
>>>>  	  used in Secure mode only.
>>>>
>>>> +config IMX_MBOX
>>>> +	tristate "iMX Mailbox"
>>>> +	depends on SOC_IMX7D || COMPILE_TEST
>>>> +	help
>>>> +	  Mailbox implementation for iMX7D Messaging Unit (MU).
>>>> +
>>>>  config PLATFORM_MHU
>>>>  	tristate "Platform MHU Mailbox"
>>>>  	depends on OF
>>>> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
>>>> index
>>>> cc23c3a43fcd..ba2fe1b6dd62 100644
>>>> --- a/drivers/mailbox/Makefile
>>>> +++ b/drivers/mailbox/Makefile
>>>> @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)	+= mailbox-test.o
>>>>
>>>>  obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o
>>>>
>>>> +obj-$(CONFIG_IMX_MBOX)	+= imx-mailbox.o
>>>> +
>>>>  obj-$(CONFIG_PLATFORM_MHU)	+= platform_mhu.o
>>>>
>>>>  obj-$(CONFIG_PL320_MBOX)	+= pl320-ipc.o
>>>> diff --git a/drivers/mailbox/imx-mailbox.c
>>>> b/drivers/mailbox/imx-mailbox.c new file mode 100644 index
>>>> 000000000000..e3f621cb1d30
>>>> --- /dev/null
>>>> +++ b/drivers/mailbox/imx-mailbox.c
>>>> @@ -0,0 +1,288 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Copyright (c) 2018 Pengutronix, Oleksij Rempel
>>>> +<o.rempel@pengutronix.de>  */
>>>> +
>>>> +#include <linux/clk.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/mailbox_controller.h> #include <linux/module.h>
>>>> +#include <linux/of_device.h>
>>>> +
>>>> +/* Transmit Register */
>>>> +#define IMX_MU_xTRn(x)		(0x00 + 4 * (x))
>>>> +/* Receive Register */
>>>> +#define IMX_MU_xRRn(x)		(0x10 + 4 * (x))
>>>> +/* Status Register */
>>>> +#define IMX_MU_xSR		0x20
>>>> +#define IMX_MU_xSR_TEn(x)	BIT(20 + (x))
>>>> +#define IMX_MU_xSR_RFn(x)	BIT(24 + (x))
>>>> +#define IMX_MU_xSR_BRDIP	BIT(9)
>>>> +
>>>> +/* Control Register */
>>>> +#define IMX_MU_xCR		0x24
>>>> +/* Transmit Interrupt Enable */
>>>> +#define IMX_MU_xCR_TIEn(x)	BIT(20 + (x))
>>>> +/* Receive Interrupt Enable */
>>>> +#define IMX_MU_xCR_RIEn(x)	BIT(24 + (x))
>>>> +
>>>> +#define IMX_MU_MAX_CHANS	4u
>>>> +
>>>> +struct imx_mu_priv;
>>>> +
>>>> +struct imx_mu_cfg {
>>>> +	unsigned int		chans;
>>>> +	void (*init_hw)(struct imx_mu_priv *priv); };
>>>> +
>>>> +struct imx_mu_con_priv {
>>>> +	int			irq;
>>>> +	unsigned int		bidx;
>>>> +	unsigned int		idx;
>>>> +};
>>>> +
>>>> +struct imx_mu_priv {
>>>> +	struct device		*dev;
>>>> +	const struct imx_mu_cfg	*dcfg;
>>>> +	void __iomem		*base;
>>>> +
>>>> +	struct mbox_controller	mbox;
>>>> +	struct mbox_chan	mbox_chans[IMX_MU_MAX_CHANS];
>>>> +
>>>> +	struct imx_mu_con_priv  con_priv[IMX_MU_MAX_CHANS];
>>>> +	struct clk		*clk;
>>>> +};
>>>> +
>>>> +static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller
>>>> +*mbox) {
>>>> +	return container_of(mbox, struct imx_mu_priv, mbox); }
>>>> +
>>>> +static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs) {
>>>> +	iowrite32(val, priv->base + offs);
>>>> +}
>>>> +
>>>> +static u32 imx_mu_read(struct imx_mu_priv *priv, u32 offs) {
>>>> +	return ioread32(priv->base + offs); }
>>>> +
>>>> +static u32 imx_mu_rmw(struct imx_mu_priv *priv, u32 offs, u32 set,
>>>> +u32
>>>> +clr) {
>>>> +	u32 val;
>>>> +
>>>> +	val = imx_mu_read(priv, offs);
>>>> +	val &= ~clr;
>>>> +	val |= set;
>>>> +	imx_mu_write(priv, val, offs);
>>>> +
>>>> +	return val;
>>>> +}
>>>> +
>>>> +static irqreturn_t imx_mu_isr(int irq, void *p) {
>>>> +	struct mbox_chan *chan = p;
>>>> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>>>> +	struct imx_mu_con_priv *cp = chan->con_priv;
>>>> +
>>>> +	u32 val, dat;
>>>> +
>>>> +	val = imx_mu_read(priv, IMX_MU_xSR);
>>>> +	val &= IMX_MU_xSR_TEn(cp->bidx) | IMX_MU_xSR_RFn(cp->bidx);
>>>> +	if (!val)
>>>> +		return IRQ_NONE;
>>>> +
>>>> +	if (val & IMX_MU_xSR_TEn(cp->bidx)) {
>>>
>>> I'm wondering whether this can work properly for multi consumers at
>>> the same time.
>>> Because xSR_TEn is 1 by default and four virtual channels actually are
>>> using the same interrupt. That means channel 1 interrupt may cause
>>> channel 2 believe it's txdone.
>>> Have we tested such using?
>>
>> see imx_mu_send_data()
>> ..... imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->bidx), 0);
>>
>>>
>>>> +		imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp-
>>>>> bidx));
>>
>> and here ^^^
>> TX status is enabled only for send and disabled as soon at transfer is done.
>>
> 
> That controls the interrupt enable signal, I'm not sure if the status bit
> Won't be set if not enable interrupt. I've not tested it.
> 
> Have you double checked it?
> 
> For n = {0, 1, 2, 3} Processor A Transmit Register n Empty. (Read-only)
> • The TEn bit is set to “1” after the BRRn register is read on the Processor B-side.
> • After the TEn bit is set to “1”, the TEn bit signals the Processor A-side that the ATRn register is
> ready to be written on the Processor A-side, and a Transmit n interrupt is issued on the Processor
> A-side (if the TEn bit in the ACR register is set to “1”).

You are right.
> mdw phys 0x30aa0024 # read control register
0x30aa0024: 08000000  # onyl RIE for channel 0 is enabled
> mdw phys 0x30aa0020 # read status register
0x30aa0020: 00f00200  # all TE are 1

So i need to compare Control with Status regs to see if we expect an
Interrupt on for the TX.

> 
>>>> +		mbox_chan_txdone(chan, 0);
>>>> +	}
>>>> +
>>>> +	if (val & IMX_MU_xSR_RFn(cp->bidx)) {
>>>> +		dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
>>>> +		mbox_chan_received_data(chan, (void *)&dat);
>>>> +	}
>>>> +
>>>> +	return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static bool imx_mu_last_tx_done(struct mbox_chan *chan) {
>>>> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>>>> +	struct imx_mu_con_priv *cp = chan->con_priv;
>>>> +	u32 val;
>>>> +
>>>> +	val = imx_mu_read(priv, IMX_MU_xSR);
>>>> +	/* test if transmit register is empty */
>>>> +	return (!!(val & IMX_MU_xSR_TEn(cp->bidx))); }
>>>> +
>>>> +static int imx_mu_send_data(struct mbox_chan *chan, void *data) {
>>>> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>>>> +	struct imx_mu_con_priv *cp = chan->con_priv;
>>>> +	u32 *arg = data;
>>>> +
>>>> +	if (!imx_mu_last_tx_done(chan))
>>>> +		return -EBUSY;
>>>> +
>>>> +	imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx));
>>>> +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->bidx), 0);
>>>> +
>>>> +	return 0;
>>>> +}
>>>
>>> Since Sascha is requesting to write a generic MU mailbox driver for
>>> both SCU MU and M4 case, the current way using virtual channels in
>>> this patch only send one word a time obviously can't fit for SCU MU clients
>> well.
>>> Do you think if we can refer to TI case to design a generic data
>>> transfer protocol to allow send multi words which is more close to SCU?
>>
>> According to your code, you are able to send 1 word message. It means, your
>> SCU is configured to trigger an interrupt or status update if REG0 was written.
>> The same is true for 2, 3, 4 and 5 word messages.
>>
> 
> SCU is interrupt driven already for the first word.
> We do can send word one by one but the performance would be terrible
> comparing to write 4 a time.
> 
>> If the MU configuration would look like you it described, you would be forced
>> to write always 4 words, even for 1 word message.
>>
>>> include/linux/soc/ti/ti-msgmgr.h
>>> struct ti_msgmgr_message {
>>>         size_t len;
>>>         u8 *buf;
>>> };
>>>
>>> Or we try to support both type transfer protocols in this driver?
>>
>> Sure. ti-msgmgr.c is a good example. You will probably need reduced variant
>> of it. It is generic enough to make it useful not only for SCU.
>>
> 
> Sascha needs a common design for both M4 and SCU.If decide to do that,
> you probably need update this patch as well.
> 
> But even doing like TI style, it still need hack for SCU as the data size offset
> Is different. However, that would be a much smaller hack than doing based
> On this driver.
> 
>>> That may introduce much complexities, personally I'm not quite like
>>> that.
>>
>> I expect 50-150 lines of extra code.
>>
> 
> Hope that could be true.
> Do you have suggestion on how to keep two type protocol co-exist
> If you thought that would work without two much extra complexity?
> Have you tried it already based this driver?
> 
> Regards
> Dong Aisheng
> 
>>> Regards
>>> Dong Aisheng
>>>
>>>> +
>>>> +static int imx_mu_startup(struct mbox_chan *chan) {
>>>> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>>>> +	struct imx_mu_con_priv *cp = chan->con_priv;
>>>> +	int ret;
>>>> +
>>>> +	ret = request_irq(cp->irq, imx_mu_isr,
>>>> +			  IRQF_SHARED, "imx_mu_chan", chan);
>>>> +	if (ret) {
>>>> +		dev_err(chan->mbox->dev,
>>>> +			"Unable to acquire IRQ %d\n", cp->irq);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->bidx), 0);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static void imx_mu_shutdown(struct mbox_chan *chan) {
>>>> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>>>> +	struct imx_mu_con_priv *cp = chan->con_priv;
>>>> +
>>>> +	imx_mu_rmw(priv, IMX_MU_xCR, 0,
>>>> +		   IMX_MU_xCR_TIEn(cp->bidx) | IMX_MU_xCR_RIEn(cp-
>>>>> bidx));
>>>> +
>>>> +	free_irq(cp->irq, chan);
>>>> +}
>>>> +
>>>> +static const struct mbox_chan_ops imx_mu_ops = {
>>>> +	.send_data = imx_mu_send_data,
>>>> +	.startup = imx_mu_startup,
>>>> +	.shutdown = imx_mu_shutdown,
>>>> +};
>>>> +
>>>> +static int imx_mu_probe(struct platform_device *pdev) {
>>>> +	struct device *dev = &pdev->dev;
>>>> +	struct resource *iomem;
>>>> +	struct imx_mu_priv *priv;
>>>> +	const struct imx_mu_cfg *dcfg;
>>>> +	unsigned int i, chans;
>>>> +	int irq, ret;
>>>> +
>>>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>>> +	if (!priv)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	dcfg = of_device_get_match_data(dev);
>>>> +	if (!dcfg)
>>>> +		return -EINVAL;
>>>> +
>>>> +	priv->dcfg = dcfg;
>>>> +	priv->dev = dev;
>>>> +
>>>> +	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +	priv->base = devm_ioremap_resource(&pdev->dev, iomem);
>>>> +	if (IS_ERR(priv->base))
>>>> +		return PTR_ERR(priv->base);
>>>> +
>>>> +	irq = platform_get_irq(pdev, 0);
>>>> +	if (irq <= 0)
>>>> +		return irq < 0 ? irq : -EINVAL;
>>>> +
>>>> +	priv->clk = devm_clk_get(dev, NULL);
>>>> +	if (IS_ERR(priv->clk)) {
>>>> +		if (PTR_ERR(priv->clk) == -ENOENT) {
>>>> +			priv->clk = NULL;
>>>> +		} else {
>>>> +			dev_err(dev, "Failed to get clock\n");
>>>> +			return PTR_ERR(priv->clk);
>>>> +		}
>>>> +	}
>>>> +
>>>> +	ret = clk_prepare_enable(priv->clk);
>>>> +	if (ret) {
>>>> +		dev_err(dev, "Failed to enable clock\n");
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	chans = min(dcfg->chans, IMX_MU_MAX_CHANS);
>>>> +	/* Initialize channel identifiers */
>>>> +	for (i = 0; i < chans; i++) {
>>>> +		struct imx_mu_con_priv *cp = &priv->con_priv[i];
>>>> +
>>>> +		cp->bidx = 3 - i;
>>>> +		cp->idx = i;
>>>> +		cp->irq = irq;
>>>> +		priv->mbox_chans[i].con_priv = cp;
>>>> +	}
>>>> +
>>>> +	priv->mbox.dev = dev;
>>>> +	priv->mbox.ops = &imx_mu_ops;
>>>> +	priv->mbox.chans = priv->mbox_chans;
>>>> +	priv->mbox.num_chans = chans;
>>>> +	priv->mbox.txdone_irq = true;
>>>> +
>>>> +	platform_set_drvdata(pdev, priv);
>>>> +
>>>> +	if (priv->dcfg->init_hw)
>>>> +		priv->dcfg->init_hw(priv);
>>>> +
>>>> +	return mbox_controller_register(&priv->mbox);
>>>> +}
>>>> +
>>>> +static int imx_mu_remove(struct platform_device *pdev) {
>>>> +	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
>>>> +
>>>> +	mbox_controller_unregister(&priv->mbox);
>>>> +	clk_disable_unprepare(priv->clk);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +
>>>> +static void imx_mu_init_imx7d_a(struct imx_mu_priv *priv) {
>>>> +	/* Set default config */
>>>> +	imx_mu_write(priv, 0, IMX_MU_xCR);
>>>> +}
>>>> +
>>>> +static const struct imx_mu_cfg imx_mu_cfg_imx7d_a = {
>>>> +	.chans = IMX_MU_MAX_CHANS,
>>>> +	.init_hw = imx_mu_init_imx7d_a,
>>>> +};
>>>> +
>>>> +static const struct imx_mu_cfg imx_mu_cfg_imx7d_b = {
>>>> +	.chans = IMX_MU_MAX_CHANS,
>>>> +};
>>>> +
>>>> +static const struct of_device_id imx_mu_dt_ids[] = {
>>>> +	{ .compatible = "fsl,imx7s-mu-a", .data = &imx_mu_cfg_imx7d_a },
>>>> +	{ .compatible = "fsl,imx7s-mu-b", .data = &imx_mu_cfg_imx7d_b },
>>>> +	{ },
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, imx_mu_dt_ids);
>>>> +
>>>> +static struct platform_driver imx_mu_driver = {
>>>> +	.probe		= imx_mu_probe,
>>>> +	.remove		= imx_mu_remove,
>>>> +	.driver = {
>>>> +		.name	= "imx_mu",
>>>> +		.of_match_table = imx_mu_dt_ids,
>>>> +	},
>>>> +};
>>>> +module_platform_driver(imx_mu_driver);
>>>> +
>>>> +MODULE_AUTHOR("Oleksij Rempel <o.rempel@pengutronix.de>");
>>>> +MODULE_DESCRIPTION("Message Unit driver for i.MX");
>>>> MODULE_LICENSE("GPL
>>>> +v2");
>>>> --
>>>> 2.17.1
>>>
>>>
>>>
> 


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/4] clk: imx7d: add IMX7D_MU_ROOT_CLK

[Stephen Boyd]

Quoting Oleksij Rempel (2018-06-15 02:51:04)
> This clock is needed for iMX mailbox driver
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---

Acked-by: Stephen Boyd <sboyd@kernel.org>

but I think you'll want some imx maintainer to review it still.

Re: [PATCH v2 1/4] clk: imx7d: add IMX7D_MU_ROOT_CLK

[Oleksij Rempel]

[-- Attachment #1.1.1: Type: text/plain, Size: 385 bytes --]

Hallo Fabio,

can you please review it.

On 09.07.2018 08:22, Stephen Boyd wrote:
> Quoting Oleksij Rempel (2018-06-15 02:51:04)
>> This clock is needed for iMX mailbox driver
>>
>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>> ---
> 
> Acked-by: Stephen Boyd <sboyd@kernel.org>
> 
> but I think you'll want some imx maintainer to review it still.
> 
> 


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 1/4] clk: imx7d: add IMX7D_MU_ROOT_CLK

[A.s. Dong]

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> Sent: Friday, June 15, 2018 5:51 PM
> To: Shawn Guo <shawnguo@kernel.org>; Fabio Estevam
> <fabio.estevam@nxp.com>; Rob Herring <robh+dt@kernel.org>; Mark
> Rutland <mark.rutland@arm.com>; A.s. Dong <aisheng.dong@nxp.com>
> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; kernel@pengutronix.de;
> linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-
> clk@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: [PATCH v2 1/4] clk: imx7d: add IMX7D_MU_ROOT_CLK
> 
> This clock is needed for iMX mailbox driver
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>

I think this can go separately through CLK tree.

Regards
Dong Aisheng

> ---
>  drivers/clk/imx/clk-imx7d.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/clk/imx/clk-imx7d.c b/drivers/clk/imx/clk-imx7d.c index
> 975a20d3cc94..c7159bfac77a 100644
> --- a/drivers/clk/imx/clk-imx7d.c
> +++ b/drivers/clk/imx/clk-imx7d.c
> @@ -795,6 +795,7 @@ static void __init imx7d_clocks_init(struct
> device_node *ccm_node)
>  	clks[IMX7D_DRAM_ALT_ROOT_CLK] =
> imx_clk_gate4("dram_alt_root_clk", "dram_alt_post_div", base + 0x4130, 0);
>  	clks[IMX7D_OCOTP_CLK] = imx_clk_gate4("ocotp_clk",
> "ipg_root_clk", base + 0x4230, 0);
>  	clks[IMX7D_SNVS_CLK] = imx_clk_gate4("snvs_clk", "ipg_root_clk",
> base + 0x4250, 0);
> +	clks[IMX7D_MU_ROOT_CLK] = imx_clk_gate4("mu_root_clk",
> "ipg_root_clk",
> +base + 0x4270, 0);
>  	clks[IMX7D_CAAM_CLK] = imx_clk_gate4("caam_clk", "ipg_root_clk",
> base + 0x4240, 0);
>  	clks[IMX7D_USB_HSIC_ROOT_CLK] =
> imx_clk_gate4("usb_hsic_root_clk", "usb_hsic_post_div", base + 0x4690, 0);
>  	clks[IMX7D_SDMA_CORE_CLK] = imx_clk_gate4("sdma_root_clk",
> "ahb_root_clk", base + 0x4480, 0);
> --
> 2.17.1

Re: [PATCH v2 1/4] clk: imx7d: add IMX7D_MU_ROOT_CLK

[Fabio Estevam]

On Fri, Jun 15, 2018 at 6:51 AM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> This clock is needed for iMX mailbox driver
>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>

Re: [PATCH v2 1/4] clk: imx7d: add IMX7D_MU_ROOT_CLK

[Stephen Boyd]

Quoting Oleksij Rempel (2018-06-15 02:51:04)
> This clock is needed for iMX mailbox driver
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---

Applied to clk-next

RE: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit

[A.s. Dong]

Hi Oleksij,

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> Sent: Friday, June 15, 2018 5:51 PM
> To: Shawn Guo <shawnguo@kernel.org>; Fabio Estevam
> <fabio.estevam@nxp.com>; Rob Herring <robh+dt@kernel.org>; Mark
> Rutland <mark.rutland@arm.com>; A.s. Dong <aisheng.dong@nxp.com>
> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; kernel@pengutronix.de;
> linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-
> clk@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit
> 
> The Mailbox controller is able to send messages (up to 4 32 bit words)
> between the endpoints.
> 

This is not correct according to current implementation as we abstract them
into 4 virtual channels while each 'channel' can send only one word one time.
We probably need explain such limitation in commit message as well.

I'm not strongly against this way. But it makes the controller lose the HW
capability to send up to 4 words. I'd just like to know a bit history or reason
why we decided to do that. Do we design it for specific users case for M4?
And are we assuming there will be no real users of multi words send requirement?

> This driver was tested using the mailbox-test driver sending messages
> between the Cortex-A7 and the Cortex-M4.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/mailbox/Kconfig       |   6 +
>  drivers/mailbox/Makefile      |   2 +
>  drivers/mailbox/imx-mailbox.c | 288
> ++++++++++++++++++++++++++++++++++
>  3 files changed, 296 insertions(+)
>  create mode 100644 drivers/mailbox/imx-mailbox.c
> 
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> a2bb27446dce..e1d2738a2e4c 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -15,6 +15,12 @@ config ARM_MHU
>  	  The controller has 3 mailbox channels, the last of which can be
>  	  used in Secure mode only.
> 
> +config IMX_MBOX
> +	tristate "iMX Mailbox"
> +	depends on SOC_IMX7D || COMPILE_TEST

Better change to ARCH_MXC as other platform does.

> +	help
> +	  Mailbox implementation for iMX7D Messaging Unit (MU).

Ditto

> +
>  config PLATFORM_MHU
>  	tristate "Platform MHU Mailbox"
>  	depends on OF
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index
> cc23c3a43fcd..ba2fe1b6dd62 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)	+= mailbox-test.o
> 
>  obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o
> 
> +obj-$(CONFIG_IMX_MBOX)	+= imx-mailbox.o
> +
>  obj-$(CONFIG_PLATFORM_MHU)	+= platform_mhu.o
> 
>  obj-$(CONFIG_PL320_MBOX)	+= pl320-ipc.o
> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> new file mode 100644 index 000000000000..e3f621cb1d30
> --- /dev/null
> +++ b/drivers/mailbox/imx-mailbox.c
> @@ -0,0 +1,288 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018 Pengutronix, Oleksij Rempel
> +<o.rempel@pengutronix.de>  */
> +
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +
> +/* Transmit Register */
> +#define IMX_MU_xTRn(x)		(0x00 + 4 * (x))
> +/* Receive Register */
> +#define IMX_MU_xRRn(x)		(0x10 + 4 * (x))
> +/* Status Register */
> +#define IMX_MU_xSR		0x20
> +#define IMX_MU_xSR_TEn(x)	BIT(20 + (x))
> +#define IMX_MU_xSR_RFn(x)	BIT(24 + (x))
> +#define IMX_MU_xSR_BRDIP	BIT(9)
> +
> +/* Control Register */
> +#define IMX_MU_xCR		0x24
> +/* Transmit Interrupt Enable */
> +#define IMX_MU_xCR_TIEn(x)	BIT(20 + (x))
> +/* Receive Interrupt Enable */
> +#define IMX_MU_xCR_RIEn(x)	BIT(24 + (x))
> +
> +#define IMX_MU_MAX_CHANS	4u
> +
> +struct imx_mu_priv;
> +
> +struct imx_mu_cfg {
> +	unsigned int		chans;
> +	void (*init_hw)(struct imx_mu_priv *priv); };
> +
> +struct imx_mu_con_priv {
> +	int			irq;
> +	unsigned int		bidx;
> +	unsigned int		idx;
> +};
> +
> +struct imx_mu_priv {
> +	struct device		*dev;
> +	const struct imx_mu_cfg	*dcfg;
> +	void __iomem		*base;
> +
> +	struct mbox_controller	mbox;
> +	struct mbox_chan	mbox_chans[IMX_MU_MAX_CHANS];
> +
> +	struct imx_mu_con_priv  con_priv[IMX_MU_MAX_CHANS];
> +	struct clk		*clk;
> +};
> +
> +static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller *mbox)
> +{
> +	return container_of(mbox, struct imx_mu_priv, mbox); }
> +
> +static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs) {
> +	iowrite32(val, priv->base + offs);
> +}
> +
> +static u32 imx_mu_read(struct imx_mu_priv *priv, u32 offs) {
> +	return ioread32(priv->base + offs);
> +}
> +
> +static u32 imx_mu_rmw(struct imx_mu_priv *priv, u32 offs, u32 set, u32
> +clr) {
> +	u32 val;
> +
> +	val = imx_mu_read(priv, offs);
> +	val &= ~clr;
> +	val |= set;
> +	imx_mu_write(priv, val, offs);
> +
> +	return val;
> +}
> +
> +static irqreturn_t imx_mu_isr(int irq, void *p) {
> +	struct mbox_chan *chan = p;
> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +	struct imx_mu_con_priv *cp = chan->con_priv;
> +
> +	u32 val, dat;
> +
> +	val = imx_mu_read(priv, IMX_MU_xSR);
> +	val &= IMX_MU_xSR_TEn(cp->bidx) | IMX_MU_xSR_RFn(cp->bidx);
> +	if (!val)
> +		return IRQ_NONE;
> +
> +	if (val & IMX_MU_xSR_TEn(cp->bidx)) {
> +		imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp-
> >bidx));
> +		mbox_chan_txdone(chan, 0);
> +	}
> +
> +	if (val & IMX_MU_xSR_RFn(cp->bidx)) {
> +		dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
> +		mbox_chan_received_data(chan, (void *)&dat);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static bool imx_mu_last_tx_done(struct mbox_chan *chan) {
> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +	struct imx_mu_con_priv *cp = chan->con_priv;
> +	u32 val;
> +
> +	val = imx_mu_read(priv, IMX_MU_xSR);
> +	/* test if transmit register is empty */
> +	return (!!(val & IMX_MU_xSR_TEn(cp->bidx))); }
> +
> +static int imx_mu_send_data(struct mbox_chan *chan, void *data) {
> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +	struct imx_mu_con_priv *cp = chan->con_priv;
> +	u32 *arg = data;
> +
> +	if (!imx_mu_last_tx_done(chan))
> +		return -EBUSY;
> +
> +	imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx));
> +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->bidx), 0);
> +
> +	return 0;
> +}
> +
> +static int imx_mu_startup(struct mbox_chan *chan) {
> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +	struct imx_mu_con_priv *cp = chan->con_priv;
> +	int ret;
> +
> +	ret = request_irq(cp->irq, imx_mu_isr,
> +			  IRQF_SHARED, "imx_mu_chan", chan);

I guess no need to assign the irq for each cp as we have only one irq.

> +	if (ret) {
> +		dev_err(chan->mbox->dev,
> +			"Unable to acquire IRQ %d\n", cp->irq);
> +		return ret;
> +	}
> +
> +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->bidx), 0);
> +
> +	return 0;
> +}
> +
> +static void imx_mu_shutdown(struct mbox_chan *chan) {
> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +	struct imx_mu_con_priv *cp = chan->con_priv;
> +
> +	imx_mu_rmw(priv, IMX_MU_xCR, 0,
> +		   IMX_MU_xCR_TIEn(cp->bidx) | IMX_MU_xCR_RIEn(cp-
> >bidx));
> +
> +	free_irq(cp->irq, chan);
> +}
> +
> +static const struct mbox_chan_ops imx_mu_ops = {
> +	.send_data = imx_mu_send_data,
> +	.startup = imx_mu_startup,
> +	.shutdown = imx_mu_shutdown,
> +};
> +
> +static int imx_mu_probe(struct platform_device *pdev) {
> +	struct device *dev = &pdev->dev;
> +	struct resource *iomem;
> +	struct imx_mu_priv *priv;
> +	const struct imx_mu_cfg *dcfg;
> +	unsigned int i, chans;
> +	int irq, ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	dcfg = of_device_get_match_data(dev);
> +	if (!dcfg)
> +		return -EINVAL;
> +
> +	priv->dcfg = dcfg;
> +	priv->dev = dev;
> +
> +	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	priv->base = devm_ioremap_resource(&pdev->dev, iomem);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq <= 0)
> +		return irq < 0 ? irq : -EINVAL;
> +
> +	priv->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(priv->clk)) {
> +		if (PTR_ERR(priv->clk) == -ENOENT) {
> +			priv->clk = NULL;
> +		} else {
> +			dev_err(dev, "Failed to get clock\n");

I guess we may not need print it for DEFER_PROBE error case.

> +			return PTR_ERR(priv->clk);
> +		}
> +	}
> +
> +	ret = clk_prepare_enable(priv->clk);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable clock\n");
> +		return ret;
> +	}
> +
> +	chans = min(dcfg->chans, IMX_MU_MAX_CHANS);
> +	/* Initialize channel identifiers */
> +	for (i = 0; i < chans; i++) {
> +		struct imx_mu_con_priv *cp = &priv->con_priv[i];
> +
> +		cp->bidx = 3 - i;

We may not need it if we improve the macro to calculate bidx by idx?

> +		cp->idx = i;
> +		cp->irq = irq;
> +		priv->mbox_chans[i].con_priv = cp;
> +	}
> +
> +	priv->mbox.dev = dev;
> +	priv->mbox.ops = &imx_mu_ops;
> +	priv->mbox.chans = priv->mbox_chans;
> +	priv->mbox.num_chans = chans;
> +	priv->mbox.txdone_irq = true;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	if (priv->dcfg->init_hw)
> +		priv->dcfg->init_hw(priv);
> +
> +	return mbox_controller_register(&priv->mbox);
> +}
> +
> +static int imx_mu_remove(struct platform_device *pdev) {
> +	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
> +
> +	mbox_controller_unregister(&priv->mbox);
> +	clk_disable_unprepare(priv->clk);
> +
> +	return 0;
> +}
> +
> +
> +static void imx_mu_init_imx7d_a(struct imx_mu_priv *priv) {
> +	/* Set default config */
> +	imx_mu_write(priv, 0, IMX_MU_xCR);

This will reset both MU Side A and B.
So we may need make sure Side B is initialized after A?

> +}
> +
> +static const struct imx_mu_cfg imx_mu_cfg_imx7d_a = {
> +	.chans = IMX_MU_MAX_CHANS,
> +	.init_hw = imx_mu_init_imx7d_a,
> +};
> +
> +static const struct imx_mu_cfg imx_mu_cfg_imx7d_b = {
> +	.chans = IMX_MU_MAX_CHANS,
> +};
> +
> +static const struct of_device_id imx_mu_dt_ids[] = {
> +	{ .compatible = "fsl,imx7s-mu-a", .data = &imx_mu_cfg_imx7d_a },
> +	{ .compatible = "fsl,imx7s-mu-b", .data = &imx_mu_cfg_imx7d_b },

I'm not sure whether we already have the decision to use fsl,<soc>-mu compatible
String and use property to specify the mu side.
Can you double check if we can switch to that way?

And would you update the binding doc for M4 support according to the qxp mu one
Which Is already signed by Rob's tag?

Regards
Dong Aisheng

> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, imx_mu_dt_ids);
> +
> +static struct platform_driver imx_mu_driver = {
> +	.probe		= imx_mu_probe,
> +	.remove		= imx_mu_remove,
> +	.driver = {
> +		.name	= "imx_mu",
> +		.of_match_table = imx_mu_dt_ids,
> +	},
> +};
> +module_platform_driver(imx_mu_driver);
> +
> +MODULE_AUTHOR("Oleksij Rempel <o.rempel@pengutronix.de>");
> +MODULE_DESCRIPTION("Message Unit driver for i.MX");
> MODULE_LICENSE("GPL
> +v2");
> --
> 2.17.1

Re: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit

[Oleksij Rempel]

[-- Attachment #1.1: Type: text/plain, Size: 13064 bytes --]

Hi,

Beside, what is equivalent of name and family name is in your name?
I know it is different in China, so I won't to avoid confusion with:
"Hi $name," format :)

On Thu, Jul 12, 2018 at 11:28:16AM +0000, A.s. Dong wrote:
> Hi Oleksij,
> 
> > -----Original Message-----
> > From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> > Sent: Friday, June 15, 2018 5:51 PM
> > To: Shawn Guo <shawnguo@kernel.org>; Fabio Estevam
> > <fabio.estevam@nxp.com>; Rob Herring <robh+dt@kernel.org>; Mark
> > Rutland <mark.rutland@arm.com>; A.s. Dong <aisheng.dong@nxp.com>
> > Cc: Oleksij Rempel <o.rempel@pengutronix.de>; kernel@pengutronix.de;
> > linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-
> > clk@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > Subject: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit
> > 
> > The Mailbox controller is able to send messages (up to 4 32 bit words)
> > between the endpoints.
> > 
> 
> This is not correct according to current implementation as we abstract them
> into 4 virtual channels while each 'channel' can send only one word one time.
> We probably need explain such limitation in commit message as well.
> 
> I'm not strongly against this way. But it makes the controller lose the HW
> capability to send up to 4 words. I'd just like to know a bit history or reason
> why we decided to do that. Do we design it for specific users case for M4?

no, it is R&D.

> And are we assuming there will be no real users of multi words send requirement?

no. In my experience, each imaginable Brainfuck configuration will
actually happen some day in some design for $reasons.
So, no assumptions, just currently working configuration of my R&D
project.

> > This driver was tested using the mailbox-test driver sending messages
> > between the Cortex-A7 and the Cortex-M4.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  drivers/mailbox/Kconfig       |   6 +
> >  drivers/mailbox/Makefile      |   2 +
> >  drivers/mailbox/imx-mailbox.c | 288
> > ++++++++++++++++++++++++++++++++++
> >  3 files changed, 296 insertions(+)
> >  create mode 100644 drivers/mailbox/imx-mailbox.c
> > 
> > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > a2bb27446dce..e1d2738a2e4c 100644
> > --- a/drivers/mailbox/Kconfig
> > +++ b/drivers/mailbox/Kconfig
> > @@ -15,6 +15,12 @@ config ARM_MHU
> >  	  The controller has 3 mailbox channels, the last of which can be
> >  	  used in Secure mode only.
> > 
> > +config IMX_MBOX
> > +	tristate "iMX Mailbox"
> > +	depends on SOC_IMX7D || COMPILE_TEST
> 
> Better change to ARCH_MXC as other platform does.

ok

> > +	help
> > +	  Mailbox implementation for iMX7D Messaging Unit (MU).
> 
> Ditto

ok

> > +
> >  config PLATFORM_MHU
> >  	tristate "Platform MHU Mailbox"
> >  	depends on OF
> > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index
> > cc23c3a43fcd..ba2fe1b6dd62 100644
> > --- a/drivers/mailbox/Makefile
> > +++ b/drivers/mailbox/Makefile
> > @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)	+= mailbox-test.o
> > 
> >  obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o
> > 
> > +obj-$(CONFIG_IMX_MBOX)	+= imx-mailbox.o
> > +
> >  obj-$(CONFIG_PLATFORM_MHU)	+= platform_mhu.o
> > 
> >  obj-$(CONFIG_PL320_MBOX)	+= pl320-ipc.o
> > diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> > new file mode 100644 index 000000000000..e3f621cb1d30
> > --- /dev/null
> > +++ b/drivers/mailbox/imx-mailbox.c
> > @@ -0,0 +1,288 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018 Pengutronix, Oleksij Rempel
> > +<o.rempel@pengutronix.de>  */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mailbox_controller.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +
> > +/* Transmit Register */
> > +#define IMX_MU_xTRn(x)		(0x00 + 4 * (x))
> > +/* Receive Register */
> > +#define IMX_MU_xRRn(x)		(0x10 + 4 * (x))
> > +/* Status Register */
> > +#define IMX_MU_xSR		0x20
> > +#define IMX_MU_xSR_TEn(x)	BIT(20 + (x))
> > +#define IMX_MU_xSR_RFn(x)	BIT(24 + (x))
> > +#define IMX_MU_xSR_BRDIP	BIT(9)
> > +
> > +/* Control Register */
> > +#define IMX_MU_xCR		0x24
> > +/* Transmit Interrupt Enable */
> > +#define IMX_MU_xCR_TIEn(x)	BIT(20 + (x))
> > +/* Receive Interrupt Enable */
> > +#define IMX_MU_xCR_RIEn(x)	BIT(24 + (x))
> > +
> > +#define IMX_MU_MAX_CHANS	4u
> > +
> > +struct imx_mu_priv;
> > +
> > +struct imx_mu_cfg {
> > +	unsigned int		chans;
> > +	void (*init_hw)(struct imx_mu_priv *priv); };
> > +
> > +struct imx_mu_con_priv {
> > +	int			irq;
> > +	unsigned int		bidx;
> > +	unsigned int		idx;
> > +};
> > +
> > +struct imx_mu_priv {
> > +	struct device		*dev;
> > +	const struct imx_mu_cfg	*dcfg;
> > +	void __iomem		*base;
> > +
> > +	struct mbox_controller	mbox;
> > +	struct mbox_chan	mbox_chans[IMX_MU_MAX_CHANS];
> > +
> > +	struct imx_mu_con_priv  con_priv[IMX_MU_MAX_CHANS];
> > +	struct clk		*clk;
> > +};
> > +
> > +static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller *mbox)
> > +{
> > +	return container_of(mbox, struct imx_mu_priv, mbox); }
> > +
> > +static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs) {
> > +	iowrite32(val, priv->base + offs);
> > +}
> > +
> > +static u32 imx_mu_read(struct imx_mu_priv *priv, u32 offs) {
> > +	return ioread32(priv->base + offs);
> > +}
> > +
> > +static u32 imx_mu_rmw(struct imx_mu_priv *priv, u32 offs, u32 set, u32
> > +clr) {
> > +	u32 val;
> > +
> > +	val = imx_mu_read(priv, offs);
> > +	val &= ~clr;
> > +	val |= set;
> > +	imx_mu_write(priv, val, offs);
> > +
> > +	return val;
> > +}
> > +
> > +static irqreturn_t imx_mu_isr(int irq, void *p) {
> > +	struct mbox_chan *chan = p;
> > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > +
> > +	u32 val, dat;
> > +
> > +	val = imx_mu_read(priv, IMX_MU_xSR);
> > +	val &= IMX_MU_xSR_TEn(cp->bidx) | IMX_MU_xSR_RFn(cp->bidx);
> > +	if (!val)
> > +		return IRQ_NONE;
> > +
> > +	if (val & IMX_MU_xSR_TEn(cp->bidx)) {
> > +		imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp-
> > >bidx));
> > +		mbox_chan_txdone(chan, 0);
> > +	}
> > +
> > +	if (val & IMX_MU_xSR_RFn(cp->bidx)) {
> > +		dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
> > +		mbox_chan_received_data(chan, (void *)&dat);
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static bool imx_mu_last_tx_done(struct mbox_chan *chan) {
> > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > +	u32 val;
> > +
> > +	val = imx_mu_read(priv, IMX_MU_xSR);
> > +	/* test if transmit register is empty */
> > +	return (!!(val & IMX_MU_xSR_TEn(cp->bidx))); }
> > +
> > +static int imx_mu_send_data(struct mbox_chan *chan, void *data) {
> > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > +	u32 *arg = data;
> > +
> > +	if (!imx_mu_last_tx_done(chan))
> > +		return -EBUSY;
> > +
> > +	imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx));
> > +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->bidx), 0);
> > +
> > +	return 0;
> > +}
> > +
> > +static int imx_mu_startup(struct mbox_chan *chan) {
> > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > +	int ret;
> > +
> > +	ret = request_irq(cp->irq, imx_mu_isr,
> > +			  IRQF_SHARED, "imx_mu_chan", chan);
> 
> I guess no need to assign the irq for each cp as we have only one irq.

all irq chip controller have one sink and number of source limited to
imagination or amount of bits in a register. May be we will need some day to
write a irqchip driver to make it work as chained irq controller.

So, I don't see any technical reason to not do it. Are you?

> > +	if (ret) {
> > +		dev_err(chan->mbox->dev,
> > +			"Unable to acquire IRQ %d\n", cp->irq);
> > +		return ret;
> > +	}
> > +
> > +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->bidx), 0);
> > +
> > +	return 0;
> > +}
> > +
> > +static void imx_mu_shutdown(struct mbox_chan *chan) {
> > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > +
> > +	imx_mu_rmw(priv, IMX_MU_xCR, 0,
> > +		   IMX_MU_xCR_TIEn(cp->bidx) | IMX_MU_xCR_RIEn(cp-
> > >bidx));
> > +
> > +	free_irq(cp->irq, chan);
> > +}
> > +
> > +static const struct mbox_chan_ops imx_mu_ops = {
> > +	.send_data = imx_mu_send_data,
> > +	.startup = imx_mu_startup,
> > +	.shutdown = imx_mu_shutdown,
> > +};
> > +
> > +static int imx_mu_probe(struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	struct resource *iomem;
> > +	struct imx_mu_priv *priv;
> > +	const struct imx_mu_cfg *dcfg;
> > +	unsigned int i, chans;
> > +	int irq, ret;
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	dcfg = of_device_get_match_data(dev);
> > +	if (!dcfg)
> > +		return -EINVAL;
> > +
> > +	priv->dcfg = dcfg;
> > +	priv->dev = dev;
> > +
> > +	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	priv->base = devm_ioremap_resource(&pdev->dev, iomem);
> > +	if (IS_ERR(priv->base))
> > +		return PTR_ERR(priv->base);
> > +
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq <= 0)
> > +		return irq < 0 ? irq : -EINVAL;
> > +
> > +	priv->clk = devm_clk_get(dev, NULL);
> > +	if (IS_ERR(priv->clk)) {
> > +		if (PTR_ERR(priv->clk) == -ENOENT) {
> > +			priv->clk = NULL;
> > +		} else {
> > +			dev_err(dev, "Failed to get clock\n");
> 
> I guess we may not need print it for DEFER_PROBE error case.

ok.

> > +			return PTR_ERR(priv->clk);
> > +		}
> > +	}
> > +
> > +	ret = clk_prepare_enable(priv->clk);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to enable clock\n");
> > +		return ret;
> > +	}
> > +
> > +	chans = min(dcfg->chans, IMX_MU_MAX_CHANS);
> > +	/* Initialize channel identifiers */
> > +	for (i = 0; i < chans; i++) {
> > +		struct imx_mu_con_priv *cp = &priv->con_priv[i];
> > +
> > +		cp->bidx = 3 - i;
> 
> We may not need it if we improve the macro to calculate bidx by idx?

Are all implementation of NXP MU have reversed bit order?
Will it fit good for one channel implementation?

> > +		cp->idx = i;
> > +		cp->irq = irq;
> > +		priv->mbox_chans[i].con_priv = cp;
> > +	}
> > +
> > +	priv->mbox.dev = dev;
> > +	priv->mbox.ops = &imx_mu_ops;
> > +	priv->mbox.chans = priv->mbox_chans;
> > +	priv->mbox.num_chans = chans;
> > +	priv->mbox.txdone_irq = true;
> > +
> > +	platform_set_drvdata(pdev, priv);
> > +
> > +	if (priv->dcfg->init_hw)
> > +		priv->dcfg->init_hw(priv);
> > +
> > +	return mbox_controller_register(&priv->mbox);
> > +}
> > +
> > +static int imx_mu_remove(struct platform_device *pdev) {
> > +	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
> > +
> > +	mbox_controller_unregister(&priv->mbox);
> > +	clk_disable_unprepare(priv->clk);
> > +
> > +	return 0;
> > +}
> > +
> > +
> > +static void imx_mu_init_imx7d_a(struct imx_mu_priv *priv) {
> > +	/* Set default config */
> > +	imx_mu_write(priv, 0, IMX_MU_xCR);
> 
> This will reset both MU Side A and B.
> So we may need make sure Side B is initialized after A?

I assume it is implementation specific, as soon as it will be needed,
we may introduce extra DT flag. No need to cover all possible cases if
we don't have to.

> > +}
> > +
> > +static const struct imx_mu_cfg imx_mu_cfg_imx7d_a = {
> > +	.chans = IMX_MU_MAX_CHANS,
> > +	.init_hw = imx_mu_init_imx7d_a,
> > +};
> > +
> > +static const struct imx_mu_cfg imx_mu_cfg_imx7d_b = {
> > +	.chans = IMX_MU_MAX_CHANS,
> > +};
> > +
> > +static const struct of_device_id imx_mu_dt_ids[] = {
> > +	{ .compatible = "fsl,imx7s-mu-a", .data = &imx_mu_cfg_imx7d_a },
> > +	{ .compatible = "fsl,imx7s-mu-b", .data = &imx_mu_cfg_imx7d_b },
> 
> I'm not sure whether we already have the decision to use fsl,<soc>-mu compatible
> String and use property to specify the mu side.
> Can you double check if we can switch to that way?

ok.

> And would you update the binding doc for M4 support according to the qxp mu one
> Which Is already signed by Rob's tag?

ok.

So, should I update my patch set including DT binding documentation
prior to yours?

If yes, can you please contact Rob to avoid confusions.

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit

[A.s. Dong]

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> Sent: Saturday, July 14, 2018 3:02 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: Shawn Guo <shawnguo@kernel.org>; Fabio Estevam
> <fabio.estevam@nxp.com>; Rob Herring <robh+dt@kernel.org>; Mark
> Rutland <mark.rutland@arm.com>; devicetree@vger.kernel.org; dl-linux-
> imx <linux-imx@nxp.com>; linux-arm-kernel@lists.infradead.org;
> kernel@pengutronix.de; linux-clk@vger.kernel.org
> Subject: Re: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit
> 
> Hi,
> 
> Beside, what is equivalent of name and family name is in your name?
> I know it is different in China, so I won't to avoid confusion with:
> "Hi $name," format :)
> 

Dong is my family name. Either Hi A.S or Hi Dong is fine to me. :)

> On Thu, Jul 12, 2018 at 11:28:16AM +0000, A.s. Dong wrote:
> > Hi Oleksij,
> >
> > > -----Original Message-----
> > > From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> > > Sent: Friday, June 15, 2018 5:51 PM
> > > To: Shawn Guo <shawnguo@kernel.org>; Fabio Estevam
> > > <fabio.estevam@nxp.com>; Rob Herring <robh+dt@kernel.org>; Mark
> > > Rutland <mark.rutland@arm.com>; A.s. Dong <aisheng.dong@nxp.com>
> > > Cc: Oleksij Rempel <o.rempel@pengutronix.de>;
> kernel@pengutronix.de;
> > > linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org;
> > > linux- clk@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > > Subject: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging
> > > unit
> > >
> > > The Mailbox controller is able to send messages (up to 4 32 bit
> > > words) between the endpoints.
> > >
> >
> > This is not correct according to current implementation as we abstract
> > them into 4 virtual channels while each 'channel' can send only one word
> one time.
> > We probably need explain such limitation in commit message as well.
> >
> > I'm not strongly against this way. But it makes the controller lose
> > the HW capability to send up to 4 words. I'd just like to know a bit
> > history or reason why we decided to do that. Do we design it for specific
> users case for M4?
> 
> no, it is R&D.
> 

Got it

> > And are we assuming there will be no real users of multi words send
> requirement?
> 
> no. In my experience, each imaginable Brainfuck configuration will actually
> happen some day in some design for $reasons.
> So, no assumptions, just currently working configuration of my R&D project.
> 

I'm fine with as it is currently. We don't have to address all possible
Issues in one time.

> > > This driver was tested using the mailbox-test driver sending
> > > messages between the Cortex-A7 and the Cortex-M4.
> > >
> > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > ---
> > >  drivers/mailbox/Kconfig       |   6 +
> > >  drivers/mailbox/Makefile      |   2 +
> > >  drivers/mailbox/imx-mailbox.c | 288
> > > ++++++++++++++++++++++++++++++++++
> > >  3 files changed, 296 insertions(+)
> > >  create mode 100644 drivers/mailbox/imx-mailbox.c
> > >
> > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > > a2bb27446dce..e1d2738a2e4c 100644
> > > --- a/drivers/mailbox/Kconfig
> > > +++ b/drivers/mailbox/Kconfig
> > > @@ -15,6 +15,12 @@ config ARM_MHU
> > >  	  The controller has 3 mailbox channels, the last of which can be
> > >  	  used in Secure mode only.
> > >
> > > +config IMX_MBOX
> > > +	tristate "iMX Mailbox"
> > > +	depends on SOC_IMX7D || COMPILE_TEST
> >
> > Better change to ARCH_MXC as other platform does.
> 
> ok
> 
> > > +	help
> > > +	  Mailbox implementation for iMX7D Messaging Unit (MU).
> >
> > Ditto
> 
> ok
> 
> > > +
> > >  config PLATFORM_MHU
> > >  	tristate "Platform MHU Mailbox"
> > >  	depends on OF
> > > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> > > index
> > > cc23c3a43fcd..ba2fe1b6dd62 100644
> > > --- a/drivers/mailbox/Makefile
> > > +++ b/drivers/mailbox/Makefile
> > > @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)	+= mailbox-test.o
> > >
> > >  obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o
> > >
> > > +obj-$(CONFIG_IMX_MBOX)	+= imx-mailbox.o
> > > +
> > >  obj-$(CONFIG_PLATFORM_MHU)	+= platform_mhu.o
> > >
> > >  obj-$(CONFIG_PL320_MBOX)	+= pl320-ipc.o
> > > diff --git a/drivers/mailbox/imx-mailbox.c
> > > b/drivers/mailbox/imx-mailbox.c new file mode 100644 index
> > > 000000000000..e3f621cb1d30
> > > --- /dev/null
> > > +++ b/drivers/mailbox/imx-mailbox.c
> > > @@ -0,0 +1,288 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (c) 2018 Pengutronix, Oleksij Rempel
> > > +<o.rempel@pengutronix.de>  */
> > > +
> > > +#include <linux/clk.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/io.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/mailbox_controller.h> #include <linux/module.h>
> > > +#include <linux/of_device.h>
> > > +
> > > +/* Transmit Register */
> > > +#define IMX_MU_xTRn(x)		(0x00 + 4 * (x))
> > > +/* Receive Register */
> > > +#define IMX_MU_xRRn(x)		(0x10 + 4 * (x))
> > > +/* Status Register */
> > > +#define IMX_MU_xSR		0x20
> > > +#define IMX_MU_xSR_TEn(x)	BIT(20 + (x))
> > > +#define IMX_MU_xSR_RFn(x)	BIT(24 + (x))
> > > +#define IMX_MU_xSR_BRDIP	BIT(9)
> > > +
> > > +/* Control Register */
> > > +#define IMX_MU_xCR		0x24
> > > +/* Transmit Interrupt Enable */
> > > +#define IMX_MU_xCR_TIEn(x)	BIT(20 + (x))
> > > +/* Receive Interrupt Enable */
> > > +#define IMX_MU_xCR_RIEn(x)	BIT(24 + (x))
> > > +
> > > +#define IMX_MU_MAX_CHANS	4u
> > > +
> > > +struct imx_mu_priv;
> > > +
> > > +struct imx_mu_cfg {
> > > +	unsigned int		chans;
> > > +	void (*init_hw)(struct imx_mu_priv *priv); };
> > > +
> > > +struct imx_mu_con_priv {
> > > +	int			irq;
> > > +	unsigned int		bidx;
> > > +	unsigned int		idx;
> > > +};
> > > +
> > > +struct imx_mu_priv {
> > > +	struct device		*dev;
> > > +	const struct imx_mu_cfg	*dcfg;
> > > +	void __iomem		*base;
> > > +
> > > +	struct mbox_controller	mbox;
> > > +	struct mbox_chan	mbox_chans[IMX_MU_MAX_CHANS];
> > > +
> > > +	struct imx_mu_con_priv  con_priv[IMX_MU_MAX_CHANS];
> > > +	struct clk		*clk;
> > > +};
> > > +
> > > +static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller
> > > +*mbox) {
> > > +	return container_of(mbox, struct imx_mu_priv, mbox); }
> > > +
> > > +static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs) {
> > > +	iowrite32(val, priv->base + offs); }
> > > +
> > > +static u32 imx_mu_read(struct imx_mu_priv *priv, u32 offs) {
> > > +	return ioread32(priv->base + offs); }
> > > +
> > > +static u32 imx_mu_rmw(struct imx_mu_priv *priv, u32 offs, u32 set,
> > > +u32
> > > +clr) {
> > > +	u32 val;
> > > +
> > > +	val = imx_mu_read(priv, offs);
> > > +	val &= ~clr;
> > > +	val |= set;
> > > +	imx_mu_write(priv, val, offs);
> > > +
> > > +	return val;
> > > +}
> > > +
> > > +static irqreturn_t imx_mu_isr(int irq, void *p) {
> > > +	struct mbox_chan *chan = p;
> > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > +
> > > +	u32 val, dat;
> > > +
> > > +	val = imx_mu_read(priv, IMX_MU_xSR);
> > > +	val &= IMX_MU_xSR_TEn(cp->bidx) | IMX_MU_xSR_RFn(cp->bidx);
> > > +	if (!val)
> > > +		return IRQ_NONE;
> > > +
> > > +	if (val & IMX_MU_xSR_TEn(cp->bidx)) {
> > > +		imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp-
> > > >bidx));
> > > +		mbox_chan_txdone(chan, 0);
> > > +	}
> > > +
> > > +	if (val & IMX_MU_xSR_RFn(cp->bidx)) {
> > > +		dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
> > > +		mbox_chan_received_data(chan, (void *)&dat);
> > > +	}
> > > +
> > > +	return IRQ_HANDLED;
> > > +}
> > > +
> > > +static bool imx_mu_last_tx_done(struct mbox_chan *chan) {
> > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > +	u32 val;
> > > +
> > > +	val = imx_mu_read(priv, IMX_MU_xSR);
> > > +	/* test if transmit register is empty */
> > > +	return (!!(val & IMX_MU_xSR_TEn(cp->bidx))); }
> > > +
> > > +static int imx_mu_send_data(struct mbox_chan *chan, void *data) {
> > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > +	u32 *arg = data;
> > > +
> > > +	if (!imx_mu_last_tx_done(chan))
> > > +		return -EBUSY;
> > > +
> > > +	imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx));
> > > +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->bidx), 0);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int imx_mu_startup(struct mbox_chan *chan) {
> > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > +	int ret;
> > > +
> > > +	ret = request_irq(cp->irq, imx_mu_isr,
> > > +			  IRQF_SHARED, "imx_mu_chan", chan);
> >
> > I guess no need to assign the irq for each cp as we have only one irq.
> 
> all irq chip controller have one sink and number of source limited to
> imagination or amount of bits in a register. May be we will need some day to
> write a irqchip driver to make it work as chained irq controller.
> 

We do need write an irqchip driver later because MU still supports another
four general purpose interrupts which will be used by SCU firmware.
But I don't think it's necessary to abstract them for TX/RX interrutps.
This makes thing simple.

> So, I don't see any technical reason to not do it. Are you?
> 
> > > +	if (ret) {
> > > +		dev_err(chan->mbox->dev,
> > > +			"Unable to acquire IRQ %d\n", cp->irq);
> > > +		return ret;
> > > +	}
> > > +
> > > +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->bidx), 0);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void imx_mu_shutdown(struct mbox_chan *chan) {
> > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > +
> > > +	imx_mu_rmw(priv, IMX_MU_xCR, 0,
> > > +		   IMX_MU_xCR_TIEn(cp->bidx) | IMX_MU_xCR_RIEn(cp-
> > > >bidx));
> > > +
> > > +	free_irq(cp->irq, chan);
> > > +}
> > > +
> > > +static const struct mbox_chan_ops imx_mu_ops = {
> > > +	.send_data = imx_mu_send_data,
> > > +	.startup = imx_mu_startup,
> > > +	.shutdown = imx_mu_shutdown,
> > > +};
> > > +
> > > +static int imx_mu_probe(struct platform_device *pdev) {
> > > +	struct device *dev = &pdev->dev;
> > > +	struct resource *iomem;
> > > +	struct imx_mu_priv *priv;
> > > +	const struct imx_mu_cfg *dcfg;
> > > +	unsigned int i, chans;
> > > +	int irq, ret;
> > > +
> > > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > +	if (!priv)
> > > +		return -ENOMEM;
> > > +
> > > +	dcfg = of_device_get_match_data(dev);
> > > +	if (!dcfg)
> > > +		return -EINVAL;
> > > +
> > > +	priv->dcfg = dcfg;
> > > +	priv->dev = dev;
> > > +
> > > +	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +	priv->base = devm_ioremap_resource(&pdev->dev, iomem);
> > > +	if (IS_ERR(priv->base))
> > > +		return PTR_ERR(priv->base);
> > > +
> > > +	irq = platform_get_irq(pdev, 0);
> > > +	if (irq <= 0)
> > > +		return irq < 0 ? irq : -EINVAL;
> > > +
> > > +	priv->clk = devm_clk_get(dev, NULL);
> > > +	if (IS_ERR(priv->clk)) {
> > > +		if (PTR_ERR(priv->clk) == -ENOENT) {
> > > +			priv->clk = NULL;
> > > +		} else {
> > > +			dev_err(dev, "Failed to get clock\n");
> >
> > I guess we may not need print it for DEFER_PROBE error case.
> 
> ok.
> 
> > > +			return PTR_ERR(priv->clk);
> > > +		}
> > > +	}
> > > +
> > > +	ret = clk_prepare_enable(priv->clk);
> > > +	if (ret) {
> > > +		dev_err(dev, "Failed to enable clock\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	chans = min(dcfg->chans, IMX_MU_MAX_CHANS);
> > > +	/* Initialize channel identifiers */
> > > +	for (i = 0; i < chans; i++) {
> > > +		struct imx_mu_con_priv *cp = &priv->con_priv[i];
> > > +
> > > +		cp->bidx = 3 - i;
> >
> > We may not need it if we improve the macro to calculate bidx by idx?
> 
> Are all implementation of NXP MU have reversed bit order?

AFAIK NXP MU library is used for all known platforms with MUs.
So I guess yes.

> Will it fit good for one channel implementation?

If you see the SCU MU patches I sent, you will see I also need
convert the channel index to channel mask. But here you're
using bidx where SCU MU does have. So have a common
channel index to mask macro may be good for both using.

> 
> > > +		cp->idx = i;
> > > +		cp->irq = irq;
> > > +		priv->mbox_chans[i].con_priv = cp;
> > > +	}
> > > +
> > > +	priv->mbox.dev = dev;
> > > +	priv->mbox.ops = &imx_mu_ops;
> > > +	priv->mbox.chans = priv->mbox_chans;
> > > +	priv->mbox.num_chans = chans;
> > > +	priv->mbox.txdone_irq = true;
> > > +
> > > +	platform_set_drvdata(pdev, priv);
> > > +
> > > +	if (priv->dcfg->init_hw)
> > > +		priv->dcfg->init_hw(priv);
> > > +
> > > +	return mbox_controller_register(&priv->mbox);
> > > +}
> > > +
> > > +static int imx_mu_remove(struct platform_device *pdev) {
> > > +	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
> > > +
> > > +	mbox_controller_unregister(&priv->mbox);
> > > +	clk_disable_unprepare(priv->clk);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +
> > > +static void imx_mu_init_imx7d_a(struct imx_mu_priv *priv) {
> > > +	/* Set default config */
> > > +	imx_mu_write(priv, 0, IMX_MU_xCR);
> >
> > This will reset both MU Side A and B.
> > So we may need make sure Side B is initialized after A?
> 
> I assume it is implementation specific, as soon as it will be needed, we may
> introduce extra DT flag. No need to cover all possible cases if we don't have
> to.
> 

We shouldn't reset SCU side, but SCU MU is not using it. So I'm okay.
We just need to know the limitation later. Probably a note added here
is better.

> > > +}
> > > +
> > > +static const struct imx_mu_cfg imx_mu_cfg_imx7d_a = {
> > > +	.chans = IMX_MU_MAX_CHANS,
> > > +	.init_hw = imx_mu_init_imx7d_a,
> > > +};
> > > +
> > > +static const struct imx_mu_cfg imx_mu_cfg_imx7d_b = {
> > > +	.chans = IMX_MU_MAX_CHANS,
> > > +};
> > > +
> > > +static const struct of_device_id imx_mu_dt_ids[] = {
> > > +	{ .compatible = "fsl,imx7s-mu-a", .data = &imx_mu_cfg_imx7d_a },
> > > +	{ .compatible = "fsl,imx7s-mu-b", .data = &imx_mu_cfg_imx7d_b },
> >
> > I'm not sure whether we already have the decision to use fsl,<soc>-mu
> > compatible String and use property to specify the mu side.
> > Can you double check if we can switch to that way?
> 
> ok.
> 
> > And would you update the binding doc for M4 support according to the
> > qxp mu one Which Is already signed by Rob's tag?
> 
> ok.
> 
> So, should I update my patch set including DT binding documentation prior to
> yours?
> 

I guess you can pick that patch and send with yours. Once your part is
reviewed ok (should be quick) then I can send the SCU part based on your
Patch series.

Finally, I'm glad that we meet an agreement now. As we're trying to
Speed up the mx8qxp support and targets to hit v4.19 kernel.
So hopefully you could help send the updated patch series soon.
Then I can follow up with my work. :)

Regards
Dong Aisheng

> If yes, can you please contact Rob to avoid confusions.
> 
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit

[Oleksij Rempel]

[-- Attachment #1.1: Type: text/plain, Size: 17370 bytes --]

On Sat, Jul 14, 2018 at 08:49:28AM +0000, A.s. Dong wrote:
> > -----Original Message-----
> > From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> > Sent: Saturday, July 14, 2018 3:02 PM
> > To: A.s. Dong <aisheng.dong@nxp.com>
> > Cc: Shawn Guo <shawnguo@kernel.org>; Fabio Estevam
> > <fabio.estevam@nxp.com>; Rob Herring <robh+dt@kernel.org>; Mark
> > Rutland <mark.rutland@arm.com>; devicetree@vger.kernel.org; dl-linux-
> > imx <linux-imx@nxp.com>; linux-arm-kernel@lists.infradead.org;
> > kernel@pengutronix.de; linux-clk@vger.kernel.org
> > Subject: Re: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit
> > 
> > Hi,
> > 
> > Beside, what is equivalent of name and family name is in your name?
> > I know it is different in China, so I won't to avoid confusion with:
> > "Hi $name," format :)
> > 
> 
> Dong is my family name. Either Hi A.S or Hi Dong is fine to me. :)
> 
> > On Thu, Jul 12, 2018 at 11:28:16AM +0000, A.s. Dong wrote:
> > > Hi Oleksij,
> > >
> > > > -----Original Message-----
> > > > From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> > > > Sent: Friday, June 15, 2018 5:51 PM
> > > > To: Shawn Guo <shawnguo@kernel.org>; Fabio Estevam
> > > > <fabio.estevam@nxp.com>; Rob Herring <robh+dt@kernel.org>; Mark
> > > > Rutland <mark.rutland@arm.com>; A.s. Dong <aisheng.dong@nxp.com>
> > > > Cc: Oleksij Rempel <o.rempel@pengutronix.de>;
> > kernel@pengutronix.de;
> > > > linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org;
> > > > linux- clk@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > > > Subject: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging
> > > > unit
> > > >
> > > > The Mailbox controller is able to send messages (up to 4 32 bit
> > > > words) between the endpoints.
> > > >
> > >
> > > This is not correct according to current implementation as we abstract
> > > them into 4 virtual channels while each 'channel' can send only one word
> > one time.
> > > We probably need explain such limitation in commit message as well.
> > >
> > > I'm not strongly against this way. But it makes the controller lose
> > > the HW capability to send up to 4 words. I'd just like to know a bit
> > > history or reason why we decided to do that. Do we design it for specific
> > users case for M4?
> > 
> > no, it is R&D.
> > 
> 
> Got it
> 
> > > And are we assuming there will be no real users of multi words send
> > requirement?
> > 
> > no. In my experience, each imaginable Brainfuck configuration will actually
> > happen some day in some design for $reasons.
> > So, no assumptions, just currently working configuration of my R&D project.
> > 
> 
> I'm fine with as it is currently. We don't have to address all possible
> Issues in one time.
> 
> > > > This driver was tested using the mailbox-test driver sending
> > > > messages between the Cortex-A7 and the Cortex-M4.
> > > >
> > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > > ---
> > > >  drivers/mailbox/Kconfig       |   6 +
> > > >  drivers/mailbox/Makefile      |   2 +
> > > >  drivers/mailbox/imx-mailbox.c | 288
> > > > ++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 296 insertions(+)
> > > >  create mode 100644 drivers/mailbox/imx-mailbox.c
> > > >
> > > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > > > a2bb27446dce..e1d2738a2e4c 100644
> > > > --- a/drivers/mailbox/Kconfig
> > > > +++ b/drivers/mailbox/Kconfig
> > > > @@ -15,6 +15,12 @@ config ARM_MHU
> > > >  	  The controller has 3 mailbox channels, the last of which can be
> > > >  	  used in Secure mode only.
> > > >
> > > > +config IMX_MBOX
> > > > +	tristate "iMX Mailbox"
> > > > +	depends on SOC_IMX7D || COMPILE_TEST
> > >
> > > Better change to ARCH_MXC as other platform does.
> > 
> > ok
> > 
> > > > +	help
> > > > +	  Mailbox implementation for iMX7D Messaging Unit (MU).
> > >
> > > Ditto
> > 
> > ok
> > 
> > > > +
> > > >  config PLATFORM_MHU
> > > >  	tristate "Platform MHU Mailbox"
> > > >  	depends on OF
> > > > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> > > > index
> > > > cc23c3a43fcd..ba2fe1b6dd62 100644
> > > > --- a/drivers/mailbox/Makefile
> > > > +++ b/drivers/mailbox/Makefile
> > > > @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)	+= mailbox-test.o
> > > >
> > > >  obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o
> > > >
> > > > +obj-$(CONFIG_IMX_MBOX)	+= imx-mailbox.o
> > > > +
> > > >  obj-$(CONFIG_PLATFORM_MHU)	+= platform_mhu.o
> > > >
> > > >  obj-$(CONFIG_PL320_MBOX)	+= pl320-ipc.o
> > > > diff --git a/drivers/mailbox/imx-mailbox.c
> > > > b/drivers/mailbox/imx-mailbox.c new file mode 100644 index
> > > > 000000000000..e3f621cb1d30
> > > > --- /dev/null
> > > > +++ b/drivers/mailbox/imx-mailbox.c
> > > > @@ -0,0 +1,288 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Copyright (c) 2018 Pengutronix, Oleksij Rempel
> > > > +<o.rempel@pengutronix.de>  */
> > > > +
> > > > +#include <linux/clk.h>
> > > > +#include <linux/interrupt.h>
> > > > +#include <linux/io.h>
> > > > +#include <linux/kernel.h>
> > > > +#include <linux/mailbox_controller.h> #include <linux/module.h>
> > > > +#include <linux/of_device.h>
> > > > +
> > > > +/* Transmit Register */
> > > > +#define IMX_MU_xTRn(x)		(0x00 + 4 * (x))
> > > > +/* Receive Register */
> > > > +#define IMX_MU_xRRn(x)		(0x10 + 4 * (x))
> > > > +/* Status Register */
> > > > +#define IMX_MU_xSR		0x20
> > > > +#define IMX_MU_xSR_TEn(x)	BIT(20 + (x))
> > > > +#define IMX_MU_xSR_RFn(x)	BIT(24 + (x))
> > > > +#define IMX_MU_xSR_BRDIP	BIT(9)
> > > > +
> > > > +/* Control Register */
> > > > +#define IMX_MU_xCR		0x24
> > > > +/* Transmit Interrupt Enable */
> > > > +#define IMX_MU_xCR_TIEn(x)	BIT(20 + (x))
> > > > +/* Receive Interrupt Enable */
> > > > +#define IMX_MU_xCR_RIEn(x)	BIT(24 + (x))
> > > > +
> > > > +#define IMX_MU_MAX_CHANS	4u
> > > > +
> > > > +struct imx_mu_priv;
> > > > +
> > > > +struct imx_mu_cfg {
> > > > +	unsigned int		chans;
> > > > +	void (*init_hw)(struct imx_mu_priv *priv); };
> > > > +
> > > > +struct imx_mu_con_priv {
> > > > +	int			irq;
> > > > +	unsigned int		bidx;
> > > > +	unsigned int		idx;
> > > > +};
> > > > +
> > > > +struct imx_mu_priv {
> > > > +	struct device		*dev;
> > > > +	const struct imx_mu_cfg	*dcfg;
> > > > +	void __iomem		*base;
> > > > +
> > > > +	struct mbox_controller	mbox;
> > > > +	struct mbox_chan	mbox_chans[IMX_MU_MAX_CHANS];
> > > > +
> > > > +	struct imx_mu_con_priv  con_priv[IMX_MU_MAX_CHANS];
> > > > +	struct clk		*clk;
> > > > +};
> > > > +
> > > > +static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller
> > > > +*mbox) {
> > > > +	return container_of(mbox, struct imx_mu_priv, mbox); }
> > > > +
> > > > +static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs) {
> > > > +	iowrite32(val, priv->base + offs); }
> > > > +
> > > > +static u32 imx_mu_read(struct imx_mu_priv *priv, u32 offs) {
> > > > +	return ioread32(priv->base + offs); }
> > > > +
> > > > +static u32 imx_mu_rmw(struct imx_mu_priv *priv, u32 offs, u32 set,
> > > > +u32
> > > > +clr) {
> > > > +	u32 val;
> > > > +
> > > > +	val = imx_mu_read(priv, offs);
> > > > +	val &= ~clr;
> > > > +	val |= set;
> > > > +	imx_mu_write(priv, val, offs);
> > > > +
> > > > +	return val;
> > > > +}
> > > > +
> > > > +static irqreturn_t imx_mu_isr(int irq, void *p) {
> > > > +	struct mbox_chan *chan = p;
> > > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > > +
> > > > +	u32 val, dat;
> > > > +
> > > > +	val = imx_mu_read(priv, IMX_MU_xSR);
> > > > +	val &= IMX_MU_xSR_TEn(cp->bidx) | IMX_MU_xSR_RFn(cp->bidx);
> > > > +	if (!val)
> > > > +		return IRQ_NONE;
> > > > +
> > > > +	if (val & IMX_MU_xSR_TEn(cp->bidx)) {
> > > > +		imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp-
> > > > >bidx));
> > > > +		mbox_chan_txdone(chan, 0);
> > > > +	}
> > > > +
> > > > +	if (val & IMX_MU_xSR_RFn(cp->bidx)) {
> > > > +		dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
> > > > +		mbox_chan_received_data(chan, (void *)&dat);
> > > > +	}
> > > > +
> > > > +	return IRQ_HANDLED;
> > > > +}
> > > > +
> > > > +static bool imx_mu_last_tx_done(struct mbox_chan *chan) {
> > > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > > +	u32 val;
> > > > +
> > > > +	val = imx_mu_read(priv, IMX_MU_xSR);
> > > > +	/* test if transmit register is empty */
> > > > +	return (!!(val & IMX_MU_xSR_TEn(cp->bidx))); }
> > > > +
> > > > +static int imx_mu_send_data(struct mbox_chan *chan, void *data) {
> > > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > > +	u32 *arg = data;
> > > > +
> > > > +	if (!imx_mu_last_tx_done(chan))
> > > > +		return -EBUSY;
> > > > +
> > > > +	imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx));
> > > > +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->bidx), 0);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int imx_mu_startup(struct mbox_chan *chan) {
> > > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > > +	int ret;
> > > > +
> > > > +	ret = request_irq(cp->irq, imx_mu_isr,
> > > > +			  IRQF_SHARED, "imx_mu_chan", chan);
> > >
> > > I guess no need to assign the irq for each cp as we have only one irq.
> > 
> > all irq chip controller have one sink and number of source limited to
> > imagination or amount of bits in a register. May be we will need some day to
> > write a irqchip driver to make it work as chained irq controller.
> > 
> 
> We do need write an irqchip driver later because MU still supports another
> four general purpose interrupts which will be used by SCU firmware.
> But I don't think it's necessary to abstract them for TX/RX interrutps.
> This makes thing simple.

You just gave me one more reason to keep current version :)

> > So, I don't see any technical reason to not do it. Are you?
> > 
> > > > +	if (ret) {
> > > > +		dev_err(chan->mbox->dev,
> > > > +			"Unable to acquire IRQ %d\n", cp->irq);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->bidx), 0);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static void imx_mu_shutdown(struct mbox_chan *chan) {
> > > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > > +
> > > > +	imx_mu_rmw(priv, IMX_MU_xCR, 0,
> > > > +		   IMX_MU_xCR_TIEn(cp->bidx) | IMX_MU_xCR_RIEn(cp-
> > > > >bidx));
> > > > +
> > > > +	free_irq(cp->irq, chan);
> > > > +}
> > > > +
> > > > +static const struct mbox_chan_ops imx_mu_ops = {
> > > > +	.send_data = imx_mu_send_data,
> > > > +	.startup = imx_mu_startup,
> > > > +	.shutdown = imx_mu_shutdown,
> > > > +};
> > > > +
> > > > +static int imx_mu_probe(struct platform_device *pdev) {
> > > > +	struct device *dev = &pdev->dev;
> > > > +	struct resource *iomem;
> > > > +	struct imx_mu_priv *priv;
> > > > +	const struct imx_mu_cfg *dcfg;
> > > > +	unsigned int i, chans;
> > > > +	int irq, ret;
> > > > +
> > > > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > > +	if (!priv)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	dcfg = of_device_get_match_data(dev);
> > > > +	if (!dcfg)
> > > > +		return -EINVAL;
> > > > +
> > > > +	priv->dcfg = dcfg;
> > > > +	priv->dev = dev;
> > > > +
> > > > +	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > +	priv->base = devm_ioremap_resource(&pdev->dev, iomem);
> > > > +	if (IS_ERR(priv->base))
> > > > +		return PTR_ERR(priv->base);
> > > > +
> > > > +	irq = platform_get_irq(pdev, 0);
> > > > +	if (irq <= 0)
> > > > +		return irq < 0 ? irq : -EINVAL;
> > > > +
> > > > +	priv->clk = devm_clk_get(dev, NULL);
> > > > +	if (IS_ERR(priv->clk)) {
> > > > +		if (PTR_ERR(priv->clk) == -ENOENT) {
> > > > +			priv->clk = NULL;
> > > > +		} else {
> > > > +			dev_err(dev, "Failed to get clock\n");
> > >
> > > I guess we may not need print it for DEFER_PROBE error case.
> > 
> > ok.
> > 
> > > > +			return PTR_ERR(priv->clk);
> > > > +		}
> > > > +	}
> > > > +
> > > > +	ret = clk_prepare_enable(priv->clk);
> > > > +	if (ret) {
> > > > +		dev_err(dev, "Failed to enable clock\n");
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	chans = min(dcfg->chans, IMX_MU_MAX_CHANS);
> > > > +	/* Initialize channel identifiers */
> > > > +	for (i = 0; i < chans; i++) {
> > > > +		struct imx_mu_con_priv *cp = &priv->con_priv[i];
> > > > +
> > > > +		cp->bidx = 3 - i;
> > >
> > > We may not need it if we improve the macro to calculate bidx by idx?
> > 
> > Are all implementation of NXP MU have reversed bit order?
> 
> AFAIK NXP MU library is used for all known platforms with MUs.
> So I guess yes.
> 
> > Will it fit good for one channel implementation?
> 
> If you see the SCU MU patches I sent, you will see I also need
> convert the channel index to channel mask. But here you're
> using bidx where SCU MU does have. So have a common
> channel index to mask macro may be good for both using.

ok

> > 
> > > > +		cp->idx = i;
> > > > +		cp->irq = irq;
> > > > +		priv->mbox_chans[i].con_priv = cp;
> > > > +	}
> > > > +
> > > > +	priv->mbox.dev = dev;
> > > > +	priv->mbox.ops = &imx_mu_ops;
> > > > +	priv->mbox.chans = priv->mbox_chans;
> > > > +	priv->mbox.num_chans = chans;
> > > > +	priv->mbox.txdone_irq = true;
> > > > +
> > > > +	platform_set_drvdata(pdev, priv);
> > > > +
> > > > +	if (priv->dcfg->init_hw)
> > > > +		priv->dcfg->init_hw(priv);
> > > > +
> > > > +	return mbox_controller_register(&priv->mbox);
> > > > +}
> > > > +
> > > > +static int imx_mu_remove(struct platform_device *pdev) {
> > > > +	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
> > > > +
> > > > +	mbox_controller_unregister(&priv->mbox);
> > > > +	clk_disable_unprepare(priv->clk);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +
> > > > +static void imx_mu_init_imx7d_a(struct imx_mu_priv *priv) {
> > > > +	/* Set default config */
> > > > +	imx_mu_write(priv, 0, IMX_MU_xCR);
> > >
> > > This will reset both MU Side A and B.
> > > So we may need make sure Side B is initialized after A?
> > 
> > I assume it is implementation specific, as soon as it will be needed, we may
> > introduce extra DT flag. No need to cover all possible cases if we don't have
> > to.
> > 
> 
> We shouldn't reset SCU side, but SCU MU is not using it. So I'm okay.
> We just need to know the limitation later. Probably a note added here
> is better.

ok

> > > > +}
> > > > +
> > > > +static const struct imx_mu_cfg imx_mu_cfg_imx7d_a = {
> > > > +	.chans = IMX_MU_MAX_CHANS,
> > > > +	.init_hw = imx_mu_init_imx7d_a,
> > > > +};
> > > > +
> > > > +static const struct imx_mu_cfg imx_mu_cfg_imx7d_b = {
> > > > +	.chans = IMX_MU_MAX_CHANS,
> > > > +};
> > > > +
> > > > +static const struct of_device_id imx_mu_dt_ids[] = {
> > > > +	{ .compatible = "fsl,imx7s-mu-a", .data = &imx_mu_cfg_imx7d_a },
> > > > +	{ .compatible = "fsl,imx7s-mu-b", .data = &imx_mu_cfg_imx7d_b },
> > >
> > > I'm not sure whether we already have the decision to use fsl,<soc>-mu
> > > compatible String and use property to specify the mu side.
> > > Can you double check if we can switch to that way?
> > 
> > ok.
> > 
> > > And would you update the binding doc for M4 support according to the
> > > qxp mu one Which Is already signed by Rob's tag?
> > 
> > ok.
> > 
> > So, should I update my patch set including DT binding documentation prior to
> > yours?
> > 
> 
> I guess you can pick that patch and send with yours. Once your part is
> reviewed ok (should be quick) then I can send the SCU part based on your
> Patch series.

Normally it is preferred to squash all history for newly created files.
I'll take your patch as base with minimal changes and send some comments
to Rob.

> Finally, I'm glad that we meet an agreement now. As we're trying to
> Speed up the mx8qxp support and targets to hit v4.19 kernel.
> So hopefully you could help send the updated patch series soon.
> Then I can follow up with my work. :)

I'll try to finish it and resend new version at Monday. Ok?

> Regards
> Dong Aisheng
> 
> > If yes, can you please contact Rob to avoid confusions.
> > 
> > --
> > Pengutronix e.K.                           |                             |
> > Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit

[A.s. Dong]

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> Sent: Saturday, July 14, 2018 5:42 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: Shawn Guo <shawnguo@kernel.org>; Fabio Estevam
> <fabio.estevam@nxp.com>; Rob Herring <robh+dt@kernel.org>; Mark
> Rutland <mark.rutland@arm.com>; devicetree@vger.kernel.org; dl-linux-
> imx <linux-imx@nxp.com>; linux-arm-kernel@lists.infradead.org;
> kernel@pengutronix.de; linux-clk@vger.kernel.org
> Subject: Re: [PATCH v2 4/4] mailbox: Add support for i.MX7D messaging unit
> 
> On Sat, Jul 14, 2018 at 08:49:28AM +0000, A.s. Dong wrote:
> > > -----Original Message-----
> > > From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> > > Sent: Saturday, July 14, 2018 3:02 PM
> > > To: A.s. Dong <aisheng.dong@nxp.com>
> > > Cc: Shawn Guo <shawnguo@kernel.org>; Fabio Estevam
> > > <fabio.estevam@nxp.com>; Rob Herring <robh+dt@kernel.org>; Mark
> > > Rutland <mark.rutland@arm.com>; devicetree@vger.kernel.org;
> > > dl-linux- imx <linux-imx@nxp.com>;
> > > linux-arm-kernel@lists.infradead.org;
> > > kernel@pengutronix.de; linux-clk@vger.kernel.org
> > > Subject: Re: [PATCH v2 4/4] mailbox: Add support for i.MX7D
> > > messaging unit
> > >
> > > Hi,
> > >
> > > Beside, what is equivalent of name and family name is in your name?
> > > I know it is different in China, so I won't to avoid confusion with:
> > > "Hi $name," format :)
> > >
> >
> > Dong is my family name. Either Hi A.S or Hi Dong is fine to me. :)
> >
> > > On Thu, Jul 12, 2018 at 11:28:16AM +0000, A.s. Dong wrote:
> > > > Hi Oleksij,
> > > >
> > > > > -----Original Message-----
> > > > > From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> > > > > Sent: Friday, June 15, 2018 5:51 PM
> > > > > To: Shawn Guo <shawnguo@kernel.org>; Fabio Estevam
> > > > > <fabio.estevam@nxp.com>; Rob Herring <robh+dt@kernel.org>;
> Mark
> > > > > Rutland <mark.rutland@arm.com>; A.s. Dong
> <aisheng.dong@nxp.com>
> > > > > Cc: Oleksij Rempel <o.rempel@pengutronix.de>;
> > > kernel@pengutronix.de;
> > > > > linux-arm-kernel@lists.infradead.org;
> > > > > devicetree@vger.kernel.org;
> > > > > linux- clk@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > > > > Subject: [PATCH v2 4/4] mailbox: Add support for i.MX7D
> > > > > messaging unit
> > > > >
> > > > > The Mailbox controller is able to send messages (up to 4 32 bit
> > > > > words) between the endpoints.
> > > > >
> > > >
> > > > This is not correct according to current implementation as we
> > > > abstract them into 4 virtual channels while each 'channel' can
> > > > send only one word
> > > one time.
> > > > We probably need explain such limitation in commit message as well.
> > > >
> > > > I'm not strongly against this way. But it makes the controller
> > > > lose the HW capability to send up to 4 words. I'd just like to
> > > > know a bit history or reason why we decided to do that. Do we
> > > > design it for specific
> > > users case for M4?
> > >
> > > no, it is R&D.
> > >
> >
> > Got it
> >
> > > > And are we assuming there will be no real users of multi words
> > > > send
> > > requirement?
> > >
> > > no. In my experience, each imaginable Brainfuck configuration will
> > > actually happen some day in some design for $reasons.
> > > So, no assumptions, just currently working configuration of my R&D
> project.
> > >
> >
> > I'm fine with as it is currently. We don't have to address all
> > possible Issues in one time.
> >
> > > > > This driver was tested using the mailbox-test driver sending
> > > > > messages between the Cortex-A7 and the Cortex-M4.
> > > > >
> > > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > > > ---
> > > > >  drivers/mailbox/Kconfig       |   6 +
> > > > >  drivers/mailbox/Makefile      |   2 +
> > > > >  drivers/mailbox/imx-mailbox.c | 288
> > > > > ++++++++++++++++++++++++++++++++++
> > > > >  3 files changed, 296 insertions(+)  create mode 100644
> > > > > drivers/mailbox/imx-mailbox.c
> > > > >
> > > > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> > > > > index a2bb27446dce..e1d2738a2e4c 100644
> > > > > --- a/drivers/mailbox/Kconfig
> > > > > +++ b/drivers/mailbox/Kconfig
> > > > > @@ -15,6 +15,12 @@ config ARM_MHU
> > > > >  	  The controller has 3 mailbox channels, the last of which can
> be
> > > > >  	  used in Secure mode only.
> > > > >
> > > > > +config IMX_MBOX
> > > > > +	tristate "iMX Mailbox"
> > > > > +	depends on SOC_IMX7D || COMPILE_TEST
> > > >
> > > > Better change to ARCH_MXC as other platform does.
> > >
> > > ok
> > >
> > > > > +	help
> > > > > +	  Mailbox implementation for iMX7D Messaging Unit (MU).
> > > >
> > > > Ditto
> > >
> > > ok
> > >
> > > > > +
> > > > >  config PLATFORM_MHU
> > > > >  	tristate "Platform MHU Mailbox"
> > > > >  	depends on OF
> > > > > diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> > > > > index
> > > > > cc23c3a43fcd..ba2fe1b6dd62 100644
> > > > > --- a/drivers/mailbox/Makefile
> > > > > +++ b/drivers/mailbox/Makefile
> > > > > @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)	+= mailbox-
> test.o
> > > > >
> > > > >  obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o
> > > > >
> > > > > +obj-$(CONFIG_IMX_MBOX)	+= imx-mailbox.o
> > > > > +
> > > > >  obj-$(CONFIG_PLATFORM_MHU)	+= platform_mhu.o
> > > > >
> > > > >  obj-$(CONFIG_PL320_MBOX)	+= pl320-ipc.o
> > > > > diff --git a/drivers/mailbox/imx-mailbox.c
> > > > > b/drivers/mailbox/imx-mailbox.c new file mode 100644 index
> > > > > 000000000000..e3f621cb1d30
> > > > > --- /dev/null
> > > > > +++ b/drivers/mailbox/imx-mailbox.c
> > > > > @@ -0,0 +1,288 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * Copyright (c) 2018 Pengutronix, Oleksij Rempel
> > > > > +<o.rempel@pengutronix.de>  */
> > > > > +
> > > > > +#include <linux/clk.h>
> > > > > +#include <linux/interrupt.h>
> > > > > +#include <linux/io.h>
> > > > > +#include <linux/kernel.h>
> > > > > +#include <linux/mailbox_controller.h> #include <linux/module.h>
> > > > > +#include <linux/of_device.h>
> > > > > +
> > > > > +/* Transmit Register */
> > > > > +#define IMX_MU_xTRn(x)		(0x00 + 4 * (x))
> > > > > +/* Receive Register */
> > > > > +#define IMX_MU_xRRn(x)		(0x10 + 4 * (x))
> > > > > +/* Status Register */
> > > > > +#define IMX_MU_xSR		0x20
> > > > > +#define IMX_MU_xSR_TEn(x)	BIT(20 + (x))
> > > > > +#define IMX_MU_xSR_RFn(x)	BIT(24 + (x))
> > > > > +#define IMX_MU_xSR_BRDIP	BIT(9)
> > > > > +
> > > > > +/* Control Register */
> > > > > +#define IMX_MU_xCR		0x24
> > > > > +/* Transmit Interrupt Enable */
> > > > > +#define IMX_MU_xCR_TIEn(x)	BIT(20 + (x))
> > > > > +/* Receive Interrupt Enable */
> > > > > +#define IMX_MU_xCR_RIEn(x)	BIT(24 + (x))
> > > > > +
> > > > > +#define IMX_MU_MAX_CHANS	4u
> > > > > +
> > > > > +struct imx_mu_priv;
> > > > > +
> > > > > +struct imx_mu_cfg {
> > > > > +	unsigned int		chans;
> > > > > +	void (*init_hw)(struct imx_mu_priv *priv); };
> > > > > +
> > > > > +struct imx_mu_con_priv {
> > > > > +	int			irq;
> > > > > +	unsigned int		bidx;
> > > > > +	unsigned int		idx;
> > > > > +};
> > > > > +
> > > > > +struct imx_mu_priv {
> > > > > +	struct device		*dev;
> > > > > +	const struct imx_mu_cfg	*dcfg;
> > > > > +	void __iomem		*base;
> > > > > +
> > > > > +	struct mbox_controller	mbox;
> > > > > +	struct mbox_chan
> 	mbox_chans[IMX_MU_MAX_CHANS];
> > > > > +
> > > > > +	struct imx_mu_con_priv  con_priv[IMX_MU_MAX_CHANS];
> > > > > +	struct clk		*clk;
> > > > > +};
> > > > > +
> > > > > +static struct imx_mu_priv *to_imx_mu_priv(struct
> > > > > +mbox_controller
> > > > > +*mbox) {
> > > > > +	return container_of(mbox, struct imx_mu_priv, mbox); }
> > > > > +
> > > > > +static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs)
> {
> > > > > +	iowrite32(val, priv->base + offs); }
> > > > > +
> > > > > +static u32 imx_mu_read(struct imx_mu_priv *priv, u32 offs) {
> > > > > +	return ioread32(priv->base + offs); }
> > > > > +
> > > > > +static u32 imx_mu_rmw(struct imx_mu_priv *priv, u32 offs, u32
> > > > > +set,
> > > > > +u32
> > > > > +clr) {
> > > > > +	u32 val;
> > > > > +
> > > > > +	val = imx_mu_read(priv, offs);
> > > > > +	val &= ~clr;
> > > > > +	val |= set;
> > > > > +	imx_mu_write(priv, val, offs);
> > > > > +
> > > > > +	return val;
> > > > > +}
> > > > > +
> > > > > +static irqreturn_t imx_mu_isr(int irq, void *p) {
> > > > > +	struct mbox_chan *chan = p;
> > > > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > > > +
> > > > > +	u32 val, dat;
> > > > > +
> > > > > +	val = imx_mu_read(priv, IMX_MU_xSR);
> > > > > +	val &= IMX_MU_xSR_TEn(cp->bidx) | IMX_MU_xSR_RFn(cp-
> >bidx);
> > > > > +	if (!val)
> > > > > +		return IRQ_NONE;
> > > > > +
> > > > > +	if (val & IMX_MU_xSR_TEn(cp->bidx)) {
> > > > > +		imx_mu_rmw(priv, IMX_MU_xCR, 0,
> IMX_MU_xCR_TIEn(cp-
> > > > > >bidx));
> > > > > +		mbox_chan_txdone(chan, 0);
> > > > > +	}
> > > > > +
> > > > > +	if (val & IMX_MU_xSR_RFn(cp->bidx)) {
> > > > > +		dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
> > > > > +		mbox_chan_received_data(chan, (void *)&dat);
> > > > > +	}
> > > > > +
> > > > > +	return IRQ_HANDLED;
> > > > > +}
> > > > > +
> > > > > +static bool imx_mu_last_tx_done(struct mbox_chan *chan) {
> > > > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > > > +	u32 val;
> > > > > +
> > > > > +	val = imx_mu_read(priv, IMX_MU_xSR);
> > > > > +	/* test if transmit register is empty */
> > > > > +	return (!!(val & IMX_MU_xSR_TEn(cp->bidx))); }
> > > > > +
> > > > > +static int imx_mu_send_data(struct mbox_chan *chan, void *data) {
> > > > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > > > +	u32 *arg = data;
> > > > > +
> > > > > +	if (!imx_mu_last_tx_done(chan))
> > > > > +		return -EBUSY;
> > > > > +
> > > > > +	imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx));
> > > > > +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp-
> >bidx), 0);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int imx_mu_startup(struct mbox_chan *chan) {
> > > > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > > > +	int ret;
> > > > > +
> > > > > +	ret = request_irq(cp->irq, imx_mu_isr,
> > > > > +			  IRQF_SHARED, "imx_mu_chan", chan);
> > > >
> > > > I guess no need to assign the irq for each cp as we have only one irq.
> > >
> > > all irq chip controller have one sink and number of source limited
> > > to imagination or amount of bits in a register. May be we will need
> > > some day to write a irqchip driver to make it work as chained irq
> controller.
> > >
> >
> > We do need write an irqchip driver later because MU still supports
> > another four general purpose interrupts which will be used by SCU
> firmware.
> > But I don't think it's necessary to abstract them for TX/RX interrutps.
> > This makes thing simple.
> 
> You just gave me one more reason to keep current version :)
> 

Anyway, I don't think it's a must change.
You can keep it.

> > > So, I don't see any technical reason to not do it. Are you?
> > >
> > > > > +	if (ret) {
> > > > > +		dev_err(chan->mbox->dev,
> > > > > +			"Unable to acquire IRQ %d\n", cp->irq);
> > > > > +		return ret;
> > > > > +	}
> > > > > +
> > > > > +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp-
> >bidx), 0);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static void imx_mu_shutdown(struct mbox_chan *chan) {
> > > > > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > > > > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > > > > +
> > > > > +	imx_mu_rmw(priv, IMX_MU_xCR, 0,
> > > > > +		   IMX_MU_xCR_TIEn(cp->bidx) |
> IMX_MU_xCR_RIEn(cp-
> > > > > >bidx));
> > > > > +
> > > > > +	free_irq(cp->irq, chan);
> > > > > +}
> > > > > +
> > > > > +static const struct mbox_chan_ops imx_mu_ops = {
> > > > > +	.send_data = imx_mu_send_data,
> > > > > +	.startup = imx_mu_startup,
> > > > > +	.shutdown = imx_mu_shutdown,
> > > > > +};
> > > > > +
> > > > > +static int imx_mu_probe(struct platform_device *pdev) {
> > > > > +	struct device *dev = &pdev->dev;
> > > > > +	struct resource *iomem;
> > > > > +	struct imx_mu_priv *priv;
> > > > > +	const struct imx_mu_cfg *dcfg;
> > > > > +	unsigned int i, chans;
> > > > > +	int irq, ret;
> > > > > +
> > > > > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > > > +	if (!priv)
> > > > > +		return -ENOMEM;
> > > > > +
> > > > > +	dcfg = of_device_get_match_data(dev);
> > > > > +	if (!dcfg)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	priv->dcfg = dcfg;
> > > > > +	priv->dev = dev;
> > > > > +
> > > > > +	iomem = platform_get_resource(pdev, IORESOURCE_MEM,
> 0);
> > > > > +	priv->base = devm_ioremap_resource(&pdev->dev, iomem);
> > > > > +	if (IS_ERR(priv->base))
> > > > > +		return PTR_ERR(priv->base);
> > > > > +
> > > > > +	irq = platform_get_irq(pdev, 0);
> > > > > +	if (irq <= 0)
> > > > > +		return irq < 0 ? irq : -EINVAL;
> > > > > +
> > > > > +	priv->clk = devm_clk_get(dev, NULL);
> > > > > +	if (IS_ERR(priv->clk)) {
> > > > > +		if (PTR_ERR(priv->clk) == -ENOENT) {
> > > > > +			priv->clk = NULL;
> > > > > +		} else {
> > > > > +			dev_err(dev, "Failed to get clock\n");
> > > >
> > > > I guess we may not need print it for DEFER_PROBE error case.
> > >
> > > ok.
> > >
> > > > > +			return PTR_ERR(priv->clk);
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	ret = clk_prepare_enable(priv->clk);
> > > > > +	if (ret) {
> > > > > +		dev_err(dev, "Failed to enable clock\n");
> > > > > +		return ret;
> > > > > +	}
> > > > > +
> > > > > +	chans = min(dcfg->chans, IMX_MU_MAX_CHANS);
> > > > > +	/* Initialize channel identifiers */
> > > > > +	for (i = 0; i < chans; i++) {
> > > > > +		struct imx_mu_con_priv *cp = &priv->con_priv[i];
> > > > > +
> > > > > +		cp->bidx = 3 - i;
> > > >
> > > > We may not need it if we improve the macro to calculate bidx by idx?
> > >
> > > Are all implementation of NXP MU have reversed bit order?
> >
> > AFAIK NXP MU library is used for all known platforms with MUs.
> > So I guess yes.
> >
> > > Will it fit good for one channel implementation?
> >
> > If you see the SCU MU patches I sent, you will see I also need convert
> > the channel index to channel mask. But here you're using bidx where
> > SCU MU does have. So have a common channel index to mask macro may
> be
> > good for both using.
> 
> ok
> 
> > >
> > > > > +		cp->idx = i;
> > > > > +		cp->irq = irq;
> > > > > +		priv->mbox_chans[i].con_priv = cp;
> > > > > +	}
> > > > > +
> > > > > +	priv->mbox.dev = dev;
> > > > > +	priv->mbox.ops = &imx_mu_ops;
> > > > > +	priv->mbox.chans = priv->mbox_chans;
> > > > > +	priv->mbox.num_chans = chans;
> > > > > +	priv->mbox.txdone_irq = true;
> > > > > +
> > > > > +	platform_set_drvdata(pdev, priv);
> > > > > +
> > > > > +	if (priv->dcfg->init_hw)
> > > > > +		priv->dcfg->init_hw(priv);
> > > > > +
> > > > > +	return mbox_controller_register(&priv->mbox);
> > > > > +}
> > > > > +
> > > > > +static int imx_mu_remove(struct platform_device *pdev) {
> > > > > +	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
> > > > > +
> > > > > +	mbox_controller_unregister(&priv->mbox);
> > > > > +	clk_disable_unprepare(priv->clk);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +
> > > > > +static void imx_mu_init_imx7d_a(struct imx_mu_priv *priv) {
> > > > > +	/* Set default config */
> > > > > +	imx_mu_write(priv, 0, IMX_MU_xCR);
> > > >
> > > > This will reset both MU Side A and B.
> > > > So we may need make sure Side B is initialized after A?
> > >
> > > I assume it is implementation specific, as soon as it will be
> > > needed, we may introduce extra DT flag. No need to cover all
> > > possible cases if we don't have to.
> > >
> >
> > We shouldn't reset SCU side, but SCU MU is not using it. So I'm okay.
> > We just need to know the limitation later. Probably a note added here
> > is better.
> 
> ok
> 
> > > > > +}
> > > > > +
> > > > > +static const struct imx_mu_cfg imx_mu_cfg_imx7d_a = {
> > > > > +	.chans = IMX_MU_MAX_CHANS,
> > > > > +	.init_hw = imx_mu_init_imx7d_a, };
> > > > > +
> > > > > +static const struct imx_mu_cfg imx_mu_cfg_imx7d_b = {
> > > > > +	.chans = IMX_MU_MAX_CHANS,
> > > > > +};
> > > > > +
> > > > > +static const struct of_device_id imx_mu_dt_ids[] = {
> > > > > +	{ .compatible = "fsl,imx7s-mu-a", .data =
> &imx_mu_cfg_imx7d_a },
> > > > > +	{ .compatible = "fsl,imx7s-mu-b", .data =
> &imx_mu_cfg_imx7d_b
> > > > > +},
> > > >
> > > > I'm not sure whether we already have the decision to use
> > > > fsl,<soc>-mu compatible String and use property to specify the mu side.
> > > > Can you double check if we can switch to that way?
> > >
> > > ok.
> > >
> > > > And would you update the binding doc for M4 support according to
> > > > the qxp mu one Which Is already signed by Rob's tag?
> > >
> > > ok.
> > >
> > > So, should I update my patch set including DT binding documentation
> > > prior to yours?
> > >
> >
> > I guess you can pick that patch and send with yours. Once your part is
> > reviewed ok (should be quick) then I can send the SCU part based on
> > your Patch series.
> 
> Normally it is preferred to squash all history for newly created files.
> I'll take your patch as base with minimal changes and send some comments
> to Rob.
> 
> > Finally, I'm glad that we meet an agreement now. As we're trying to
> > Speed up the mx8qxp support and targets to hit v4.19 kernel.
> > So hopefully you could help send the updated patch series soon.
> > Then I can follow up with my work. :)
> 
> I'll try to finish it and resend new version at Monday. Ok?
> 

Good to me.
Thanks

Regards
Dong Aisheng

> > Regards
> > Dong Aisheng
> >
> > > If yes, can you please contact Rob to avoid confusions.
> > >
> > > --
> > > Pengutronix e.K.                           |                             |
> > > Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> > > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0
> |
> > > Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> >
> 
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |