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

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

[Oleksij Rempel]

20180618 changes v4:
- DT: change fsl,mu-side-a to fsl,mu-side-b
- DT: split the patches.
- DT: add all currently known SoCs.
- imx-mailbox: free allocated irq name on channel shutdown
- imx-mailbox: rename *_imx7 functions to *_generic

20180615 changes v3:
- DT: remove prosaic part of documentation. It describes software
  or firmware specific usage and not relevant for HW description.
- DT: use <soc>-mu instead of <soc>-mu-<mu side> and add fsl,mu-side-a
  parameter.
- DT: add most of know i.MX variants with MU
- imx-mailbox: use macros instead of precalculated bit index.
- imx-mailbox: remove warning message for clk.
- imx-mailbox: use imx_mu_chan[%idx] for devm_request_irq.
- imx-mailbox: depend on ARCH_MXC instead of SOX_IMX7

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.

Dong Aisheng (1):
  dt-bindings: arm: fsl: add mu binding doc

Oleksij Rempel (3):
  dt-bindings: arm: fsl: rework mu doc
  ARM: dts: imx7s: add i.MX7 messaging unit support
  mailbox: Add support for i.MX7D messaging unit

 .../devicetree/bindings/mailbox/fsl,mu.txt    |  32 ++
 arch/arm/boot/dts/imx7s.dtsi                  |  19 ++
 drivers/mailbox/Kconfig                       |   6 +
 drivers/mailbox/Makefile                      |   2 +
 drivers/mailbox/imx-mailbox.c                 | 300 ++++++++++++++++++
 5 files changed, 359 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/fsl,mu.txt
 create mode 100644 drivers/mailbox/imx-mailbox.c

-- 
2.18.0

[PATCH v4 1/4] dt-bindings: arm: fsl: add mu binding doc

[Oleksij Rempel]

From: Dong Aisheng <aisheng.dong@nxp.com>

The Messaging Unit module enables two processors within
the SoC to communicate and coordinate by passing messages
(e.g. data, status and control) through the MU interface.

Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree@vger.kernel.org
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 .../devicetree/bindings/mailbox/fsl,mu.txt    | 34 +++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/fsl,mu.txt

diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
new file mode 100644
index 000000000000..90e4905dfc69
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
@@ -0,0 +1,34 @@
+NXP i.MX Messaging Unit (MU)
+--------------------------------------------------------------------
+
+The Messaging Unit module enables two processors within the SoC to
+communicate and coordinate by passing messages (e.g. data, status
+and control) through the MU interface. The MU also provides the ability
+for one processor to signal the other processor using interrupts.
+
+Because the MU manages the messaging between processors, the MU uses
+different clocks (from each side of the different peripheral buses).
+Therefore, the MU must synchronize the accesses from one side to the
+other. The MU accomplishes synchronization using two sets of matching
+registers (Processor A-facing, Processor B-facing).
+
+Messaging Unit Device Node:
+=============================
+
+Required properties:
+-------------------
+- compatible :	should be "fsl,<chip>-mu", the supported chips include
+		imx8qxp, imx8qm.
+- reg :		Should contain the registers location and length
+- interrupts :	Interrupt number. The interrupt specifier format depends
+		on the interrupt controller parent.
+- #mbox-cells:  Must be 0. Number of cells in a mailbox
+
+Examples:
+--------
+lsio_mu0: mailbox@5d1b0000 {
+	compatible = "fsl,imx8qxp-mu";
+	reg = <0x0 0x5d1b0000 0x0 0x10000>;
+	interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
+	#mbox-cells = <0>;
+};
-- 
2.18.0

[PATCH v4 2/4] dt-bindings: arm: fsl: rework mu doc

[Oleksij Rempel]

- Remove software specific description. MU has many use cases
limited only by imagination. So remove this confusing part.
- Add all currently known SoCs with MU
- Make sure this documentation covers not only Firmware specific use
case. Parameters for generic configurations should be described as well.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 .../devicetree/bindings/mailbox/fsl,mu.txt    | 32 +++++++++----------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
index 90e4905dfc69..9d5e6ee61e22 100644
--- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
+++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
@@ -1,28 +1,26 @@
 NXP i.MX Messaging Unit (MU)
 --------------------------------------------------------------------
 
-The Messaging Unit module enables two processors within the SoC to
-communicate and coordinate by passing messages (e.g. data, status
-and control) through the MU interface. The MU also provides the ability
-for one processor to signal the other processor using interrupts.
-
-Because the MU manages the messaging between processors, the MU uses
-different clocks (from each side of the different peripheral buses).
-Therefore, the MU must synchronize the accesses from one side to the
-other. The MU accomplishes synchronization using two sets of matching
-registers (Processor A-facing, Processor B-facing).
-
-Messaging Unit Device Node:
-=============================
-
 Required properties:
 -------------------
-- compatible :	should be "fsl,<chip>-mu", the supported chips include
-		imx8qxp, imx8qm.
+- compatible :	should be "fsl,<chip>-mu", the supported chips include:
+		fsl,imx6sx-mu	- i.MX 6SoloX
+		fsl,imx7d-mu	- i.MX 7Dual
+		fsl,imx7s-mu	- i.MX 7Solo
+		fsl,imx7ulp-mu	- i.MX 7ULP
+		fsl,imx8qm-mu	- i.MX 8QM
+		fsl,imx8qxp-mu	- i.MX 8QXP
 - reg :		Should contain the registers location and length
 - interrupts :	Interrupt number. The interrupt specifier format depends
 		on the interrupt controller parent.
-- #mbox-cells:  Must be 0. Number of cells in a mailbox
+- #mbox-cells:  Must be:
+		0 - for single channel mode. i.MX8* SCU protocol specific.
+		1 - for multichannel (generic) mode.
+
+Optional properties:
+-------------------
+- clocks :	phandle to the input clock.
+- fsl,mu-side-b : Should be set for side B MU.
 
 Examples:
 --------
-- 
2.18.0

RE: [PATCH v4 2/4] dt-bindings: arm: fsl: rework mu doc

[A.s. Dong]

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> Sent: Wednesday, July 18, 2018 3:13 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; dl-linux-
> imx <linux-imx@nxp.com>
> Subject: [PATCH v4 2/4] dt-bindings: arm: fsl: rework mu doc
> 

The patch title may be improved like:
dt-bindings: mailbox: imx-mu: add generic MU channel support

And explain in commit message:
Each MU has four pairs of rx/tx data register with four rx/tx interrupts which can also
be used as a separate channel. This patch intends to .....

> - Remove software specific description. MU has many use cases limited only
> by imagination. So remove this confusing part.
> - Add all currently known SoCs with MU
> - Make sure this documentation covers not only Firmware specific use case.
> Parameters for generic configurations should be described as well.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  .../devicetree/bindings/mailbox/fsl,mu.txt    | 32 +++++++++----------
>  1 file changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> index 90e4905dfc69..9d5e6ee61e22 100644
> --- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> @@ -1,28 +1,26 @@
>  NXP i.MX Messaging Unit (MU)
>  --------------------------------------------------------------------
> 
> -The Messaging Unit module enables two processors within the SoC to -
> communicate and coordinate by passing messages (e.g. data, status -and
> control) through the MU interface. The MU also provides the ability -for one
> processor to signal the other processor using interrupts.
> -
> -Because the MU manages the messaging between processors, the MU uses
> -different clocks (from each side of the different peripheral buses).
> -Therefore, the MU must synchronize the accesses from one side to the -
> other. The MU accomplishes synchronization using two sets of matching -
> registers (Processor A-facing, Processor B-facing).
> -
> -Messaging Unit Device Node:
> -=============================
> -

Here you removed all the HW description of MU from the reference manual
which is a bit strange to me.

The reason you given is "remove software specific description. MU has many use
cases limited only by imagination. So remove this confusing part" looks not quit
convince. What confusing did you mean?
Is it " passing messages (e.g. data, status -and control)"?
Those are HW capabilities and I'm not sure it's quite necessary to remove them.

Regards
Dong Aisheng

>  Required properties:
>  -------------------
> -- compatible :	should be "fsl,<chip>-mu", the supported chips include
> -		imx8qxp, imx8qm.
> +- compatible :	should be "fsl,<chip>-mu", the supported chips include:
> +		fsl,imx6sx-mu	- i.MX 6SoloX
> +		fsl,imx7d-mu	- i.MX 7Dual
> +		fsl,imx7s-mu	- i.MX 7Solo
> +		fsl,imx7ulp-mu	- i.MX 7ULP
> +		fsl,imx8qm-mu	- i.MX 8QM
> +		fsl,imx8qxp-mu	- i.MX 8QXP
>  - reg :		Should contain the registers location and length
>  - interrupts :	Interrupt number. The interrupt specifier format depends
>  		on the interrupt controller parent.
> -- #mbox-cells:  Must be 0. Number of cells in a mailbox
> +- #mbox-cells:  Must be:
> +		0 - for single channel mode. i.MX8* SCU protocol specific.
> +		1 - for multichannel (generic) mode.
> +
> +Optional properties:
> +-------------------
> +- clocks :	phandle to the input clock.
> +- fsl,mu-side-b : Should be set for side B MU.
> 
>  Examples:
>  --------
> --
> 2.18.0

[PATCH v4 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 | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
index ce85b3ca1a55..038e0eafc549 100644
--- a/arch/arm/boot/dts/imx7s.dtsi
+++ b/arch/arm/boot/dts/imx7s.dtsi
@@ -1001,6 +1001,25 @@
 				status = "disabled";
 			};
 
+			mu0a: mailbox@30aa0000 {
+				compatible = "fsl,imx7s-mu";
+				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";
+				reg = <0x30ab0000 0x10000>;
+				interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>;
+				clocks = <&clks IMX7D_MU_ROOT_CLK>;
+				#mbox-cells = <1>;
+				fsl,mu-side-b;
+				status = "disabled";
+			};
+
 			usbotg1: usb@30b10000 {
 				compatible = "fsl,imx7d-usb", "fsl,imx27-usb";
 				reg = <0x30b10000 0x200>;
-- 
2.18.0

[PATCH v4 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 | 300 ++++++++++++++++++++++++++++++++++
 3 files changed, 308 insertions(+)
 create mode 100644 drivers/mailbox/imx-mailbox.c

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index a2bb27446dce..79060ddc380d 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 "i.MX Mailbox"
+	depends on ARCH_MXC || COMPILE_TEST
+	help
+	  Mailbox implementation for i.MX 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..ad8797127b1f
--- /dev/null
+++ b/drivers/mailbox/imx-mailbox.c
@@ -0,0 +1,300 @@
+// 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 + (3 - (x)))
+#define IMX_MU_xSR_RFn(x)	BIT(24 + (3 - (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 + (3 - (x)))
+/* Receive Interrupt Enable */
+#define IMX_MU_xCR_RIEn(x)	BIT(24 + (3 - (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		idx;
+	char			*irq_desc;
+};
+
+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;
+
+	bool			side_b;
+};
+
+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, ctrl, dat;
+
+	ctrl = imx_mu_read(priv, IMX_MU_xCR);
+	val = imx_mu_read(priv, IMX_MU_xSR);
+	val &= IMX_MU_xSR_TEn(cp->idx) | IMX_MU_xSR_RFn(cp->idx);
+	val &= ctrl & (IMX_MU_xCR_TIEn(cp->idx) | IMX_MU_xCR_RIEn(cp->idx));
+	if (!val)
+		return IRQ_NONE;
+
+	if (val & IMX_MU_xSR_TEn(cp->idx)) {
+		imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp->idx));
+		mbox_chan_txdone(chan, 0);
+	}
+
+	if (val & IMX_MU_xSR_RFn(cp->idx)) {
+		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->idx)));
+}
+
+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->idx), 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;
+
+	cp->irq_desc = devm_kasprintf(priv->dev, GFP_KERNEL, "imx_mu_chan[%i]",
+				      cp->idx);
+	if (!cp->irq_desc)
+		return -ENOMEM;
+
+	ret = devm_request_irq(priv->dev, cp->irq, imx_mu_isr,
+			       IRQF_SHARED, cp->irq_desc, chan);
+	if (ret) {
+		dev_err(priv->dev,
+			"Unable to acquire IRQ %d\n", cp->irq);
+		return ret;
+	}
+
+	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->idx), 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->idx) | IMX_MU_xCR_RIEn(cp->idx));
+
+	devm_free_irq(priv->dev, cp->irq, chan);
+	devm_kfree(priv->dev, cp->irq_desc);
+}
+
+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 device_node *np = dev->of_node;
+	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)
+			return PTR_ERR(priv->clk);
+
+		priv->clk = NULL;
+	}
+
+	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->idx = i;
+		cp->irq = irq;
+		priv->mbox_chans[i].con_priv = cp;
+	}
+
+	if (of_property_read_bool(np, "fsl,mu-side-b"))
+		priv->side_b = true;
+
+	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_generic(struct imx_mu_priv *priv)
+{
+	if (priv->side_b)
+		return;
+
+	/* Set default MU configuration */
+	imx_mu_write(priv, 0, IMX_MU_xCR);
+}
+
+static const struct imx_mu_cfg imx_mu_cfg_generic = {
+	.chans = IMX_MU_MAX_CHANS,
+	.init_hw = imx_mu_init_generic,
+};
+
+static const struct of_device_id imx_mu_dt_ids[] = {
+	{ .compatible = "fsl,imx6sx-mu", .data = &imx_mu_cfg_generic },
+	{ .compatible = "fsl,imx7d-mu", .data = &imx_mu_cfg_generic },
+	{ .compatible = "fsl,imx7s-mu", .data = &imx_mu_cfg_generic },
+	{ .compatible = "fsl,imx7ulp-mu", .data = &imx_mu_cfg_generic },
+	{ .compatible = "fsl,imx8qm-mu", .data = &imx_mu_cfg_generic },
+	{ .compatible = "fsl,imx8qxp-mu", .data = &imx_mu_cfg_generic },
+	{ },
+};
+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.18.0

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

[Vladimir Zapolskiy]

Hi Oleksij,

On 07/18/2018 10:12 AM, Oleksij Rempel wrote:
> 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 | 300 ++++++++++++++++++++++++++++++++++
>  3 files changed, 308 insertions(+)
>  create mode 100644 drivers/mailbox/imx-mailbox.c
> 
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index a2bb27446dce..79060ddc380d 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 "i.MX Mailbox"
> +	depends on ARCH_MXC || COMPILE_TEST
> +	help
> +	  Mailbox implementation for i.MX 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..ad8797127b1f
> --- /dev/null
> +++ b/drivers/mailbox/imx-mailbox.c
> @@ -0,0 +1,300 @@
> +// 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 + (3 - (x)))
> +#define IMX_MU_xSR_RFn(x)	BIT(24 + (3 - (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 + (3 - (x)))
> +/* Receive Interrupt Enable */
> +#define IMX_MU_xCR_RIEn(x)	BIT(24 + (3 - (x)))
> +
> +#define IMX_MU_MAX_CHANS	4u
> +
> +struct imx_mu_priv;
> +
> +struct imx_mu_cfg {
> +	unsigned int		chans;

Basically this field is not used, everywhere in the driver its usage can
be replaced by IMX_MU_MAX_CHANS, and it makes sense to rename the latter,
and 'chan' local variable from .probe is also removed.

I suggest that you add this field at the time when you to add controller
specific data other than 'imx_mu_cfg_generic'. 

> +	void (*init_hw)(struct imx_mu_priv *priv);
> +};
> +
> +struct imx_mu_con_priv {
> +	int			irq;
> +	unsigned int		idx;
> +	char			*irq_desc;
> +};
> +
> +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;
> +
> +	bool			side_b;
> +};
> +
> +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, ctrl, dat;
> +
> +	ctrl = imx_mu_read(priv, IMX_MU_xCR);
> +	val = imx_mu_read(priv, IMX_MU_xSR);
> +	val &= IMX_MU_xSR_TEn(cp->idx) | IMX_MU_xSR_RFn(cp->idx);
> +	val &= ctrl & (IMX_MU_xCR_TIEn(cp->idx) | IMX_MU_xCR_RIEn(cp->idx));
> +	if (!val)
> +		return IRQ_NONE;
> +
> +	if (val & IMX_MU_xSR_TEn(cp->idx)) {
> +		imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp->idx));
> +		mbox_chan_txdone(chan, 0);
> +	}
> +
> +	if (val & IMX_MU_xSR_RFn(cp->idx)) {
> +		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->idx)));

Because the function returns bool value, double negation is not needed.

	return val & IMX_MU_xSR_TEn(cp->idx);

is good enough.

> +}
> +
> +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->idx), 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;
> +
> +	cp->irq_desc = devm_kasprintf(priv->dev, GFP_KERNEL, "imx_mu_chan[%i]",
> +				      cp->idx);
> +	if (!cp->irq_desc)
> +		return -ENOMEM;
> +
> +	ret = devm_request_irq(priv->dev, cp->irq, imx_mu_isr,
> +			       IRQF_SHARED, cp->irq_desc, chan);
> +	if (ret) {
> +		dev_err(priv->dev,
> +			"Unable to acquire IRQ %d\n", cp->irq);
> +		return ret;
> +	}
> +
> +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->idx), 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->idx) | IMX_MU_xCR_RIEn(cp->idx));
> +
> +	devm_free_irq(priv->dev, cp->irq, chan);
> +	devm_kfree(priv->dev, cp->irq_desc);
> +}
> +
> +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 device_node *np = dev->of_node;
> +	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;

Please don't check or handle 'irq == 0' case specially, it is dead code.

	if (irq < 0)
		return irq;

is good enough.

> +
> +	priv->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(priv->clk)) {
> +		if (PTR_ERR(priv->clk) != -ENOENT)
> +			return PTR_ERR(priv->clk);
> +
> +		priv->clk = NULL;
> +	}
> +
> +	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 */

The comment above is trivial, please remove it.

> +	for (i = 0; i < chans; i++) {
> +		struct imx_mu_con_priv *cp = &priv->con_priv[i];
> +
> +		cp->idx = i;
> +		cp->irq = irq;

I read it as a single irq for all channels.

Why do you dynamically init more channel data imx_mu_startup() based on irq value?
Aren't 'cp->irq_desc' all equal? Isn't just a single devm_request_irq() in .probe
sufficient?

> +		priv->mbox_chans[i].con_priv = cp;
> +	}
> +
> +	if (of_property_read_bool(np, "fsl,mu-side-b"))
> +		priv->side_b = true;
> +
> +	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_generic(struct imx_mu_priv *priv)
> +{
> +	if (priv->side_b)
> +		return;
> +
> +	/* Set default MU configuration */
> +	imx_mu_write(priv, 0, IMX_MU_xCR);
> +}
> +
> +static const struct imx_mu_cfg imx_mu_cfg_generic = {
> +	.chans = IMX_MU_MAX_CHANS,

Here IMX_MU_MAX_CHANS macro name is questionable, see my top comment.
For clarity here 'MAX' is not expected, it shall be the exact controller
specific value, something like s/MAX/NUM/ may be considered.

> +	.init_hw = imx_mu_init_generic,
> +};
> +
> +static const struct of_device_id imx_mu_dt_ids[] = {
> +	{ .compatible = "fsl,imx6sx-mu", .data = &imx_mu_cfg_generic },
> +	{ .compatible = "fsl,imx7d-mu", .data = &imx_mu_cfg_generic },
> +	{ .compatible = "fsl,imx7s-mu", .data = &imx_mu_cfg_generic },
> +	{ .compatible = "fsl,imx7ulp-mu", .data = &imx_mu_cfg_generic },
> +	{ .compatible = "fsl,imx8qm-mu", .data = &imx_mu_cfg_generic },
> +	{ .compatible = "fsl,imx8qxp-mu", .data = &imx_mu_cfg_generic },

It sounds like the list will be constantly extending. Is there any chance
to introduce just one generic compatible in the driver, and describe
the whole set of compatibles in documentation section only?

> +	{ },
> +};
> +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");
> 

--
Best wishes,
Vladimir

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

[A.s. Dong]

> -----Original Message-----
> From: Vladimir Zapolskiy [mailto:vladimir_zapolskiy@mentor.com]
> Sent: Wednesday, July 18, 2018 3:58 PM
> To: 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>; A.s.
> Dong <aisheng.dong@nxp.com>
> Cc: kernel@pengutronix.de; linux-arm-kernel@lists.infradead.org;
> devicetree@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v4 4/4] mailbox: Add support for i.MX7D messaging unit
> 
> Hi Oleksij,
> 
> On 07/18/2018 10:12 AM, Oleksij Rempel wrote:
> > 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 | 300
> > ++++++++++++++++++++++++++++++++++
> >  3 files changed, 308 insertions(+)
> >  create mode 100644 drivers/mailbox/imx-mailbox.c
> >
> > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > a2bb27446dce..79060ddc380d 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 "i.MX Mailbox"
> > +	depends on ARCH_MXC || COMPILE_TEST
> > +	help
> > +	  Mailbox implementation for i.MX 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..ad8797127b1f
> > --- /dev/null
> > +++ b/drivers/mailbox/imx-mailbox.c
> > @@ -0,0 +1,300 @@
> > +// 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 + (3 - (x)))
> > +#define IMX_MU_xSR_RFn(x)	BIT(24 + (3 - (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 + (3 - (x)))
> > +/* Receive Interrupt Enable */
> > +#define IMX_MU_xCR_RIEn(x)	BIT(24 + (3 - (x)))
> > +
> > +#define IMX_MU_MAX_CHANS	4u
> > +
> > +struct imx_mu_priv;
> > +
> > +struct imx_mu_cfg {
> > +	unsigned int		chans;
> 
> Basically this field is not used, everywhere in the driver its usage can be
> replaced by IMX_MU_MAX_CHANS, and it makes sense to rename the latter,
> and 'chan' local variable from .probe is also removed.
> 
> I suggest that you add this field at the time when you to add controller
> specific data other than 'imx_mu_cfg_generic'.
> 
> > +	void (*init_hw)(struct imx_mu_priv *priv); };
> > +
> > +struct imx_mu_con_priv {
> > +	int			irq;
> > +	unsigned int		idx;
> > +	char			*irq_desc;
> > +};
> > +
> > +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;
> > +
> > +	bool			side_b;
> > +};
> > +
> > +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, ctrl, dat;
> > +
> > +	ctrl = imx_mu_read(priv, IMX_MU_xCR);
> > +	val = imx_mu_read(priv, IMX_MU_xSR);
> > +	val &= IMX_MU_xSR_TEn(cp->idx) | IMX_MU_xSR_RFn(cp->idx);
> > +	val &= ctrl & (IMX_MU_xCR_TIEn(cp->idx) | IMX_MU_xCR_RIEn(cp-
> >idx));
> > +	if (!val)
> > +		return IRQ_NONE;
> > +
> > +	if (val & IMX_MU_xSR_TEn(cp->idx)) {
> > +		imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp-
> >idx));
> > +		mbox_chan_txdone(chan, 0);
> > +	}
> > +
> > +	if (val & IMX_MU_xSR_RFn(cp->idx)) {
> > +		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->idx)));
> 
> Because the function returns bool value, double negation is not needed.
> 
> 	return val & IMX_MU_xSR_TEn(cp->idx);
> 
> is good enough.
> 
> > +}
> > +
> > +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->idx), 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;
> > +
> > +	cp->irq_desc = devm_kasprintf(priv->dev, GFP_KERNEL,
> "imx_mu_chan[%i]",
> > +				      cp->idx);
> > +	if (!cp->irq_desc)
> > +		return -ENOMEM;
> > +
> > +	ret = devm_request_irq(priv->dev, cp->irq, imx_mu_isr,
> > +			       IRQF_SHARED, cp->irq_desc, chan);
> > +	if (ret) {
> > +		dev_err(priv->dev,
> > +			"Unable to acquire IRQ %d\n", cp->irq);
> > +		return ret;
> > +	}
> > +
> > +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->idx), 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->idx) | IMX_MU_xCR_RIEn(cp-
> >idx));
> > +
> > +	devm_free_irq(priv->dev, cp->irq, chan);
> > +	devm_kfree(priv->dev, cp->irq_desc); }
> > +
> > +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 device_node *np = dev->of_node;
> > +	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;
> 
> Please don't check or handle 'irq == 0' case specially, it is dead code.
> 
> 	if (irq < 0)
> 		return irq;
> 
> is good enough.
> 
> > +
> > +	priv->clk = devm_clk_get(dev, NULL);
> > +	if (IS_ERR(priv->clk)) {
> > +		if (PTR_ERR(priv->clk) != -ENOENT)
> > +			return PTR_ERR(priv->clk);
> > +
> > +		priv->clk = NULL;
> > +	}
> > +
> > +	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 */
> 
> The comment above is trivial, please remove it.
> 
> > +	for (i = 0; i < chans; i++) {
> > +		struct imx_mu_con_priv *cp = &priv->con_priv[i];
> > +
> > +		cp->idx = i;
> > +		cp->irq = irq;
> 
> I read it as a single irq for all channels.
> 
> Why do you dynamically init more channel data imx_mu_startup() based on
> irq value?
> Aren't 'cp->irq_desc' all equal? Isn't just a single devm_request_irq()
> in .probe sufficient?

They're used to differentiate the channel irq name (probably initialize here is enough).
And the code replies on devm_request_irq to pass the corresponding data of each channel,
That why calls request_irq multiple times.

> 
> > +		priv->mbox_chans[i].con_priv = cp;
> > +	}
> > +
> > +	if (of_property_read_bool(np, "fsl,mu-side-b"))
> > +		priv->side_b = true;
> > +
> > +	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_generic(struct imx_mu_priv *priv) {
> > +	if (priv->side_b)
> > +		return;
> > +
> > +	/* Set default MU configuration */
> > +	imx_mu_write(priv, 0, IMX_MU_xCR);
> > +}
> > +
> > +static const struct imx_mu_cfg imx_mu_cfg_generic = {
> > +	.chans = IMX_MU_MAX_CHANS,
> 
> Here IMX_MU_MAX_CHANS macro name is questionable, see my top
> comment.
> For clarity here 'MAX' is not expected, it shall be the exact controller specific
> value, something like s/MAX/NUM/ may be considered.
> 

I actually gave the same suggestion in last round review, but Oleksij seems have
his own reason and insist to keep it.

I even suggest to totally remove struct imx_mu_cfg now and add it later
once we see real requirement.

> > +	.init_hw = imx_mu_init_generic,
> > +};
> > +
> > +static const struct of_device_id imx_mu_dt_ids[] = {
> > +	{ .compatible = "fsl,imx6sx-mu", .data = &imx_mu_cfg_generic },
> > +	{ .compatible = "fsl,imx7d-mu", .data = &imx_mu_cfg_generic },
> > +	{ .compatible = "fsl,imx7s-mu", .data = &imx_mu_cfg_generic },

7d and 7s should be the same.

> > +	{ .compatible = "fsl,imx7ulp-mu", .data = &imx_mu_cfg_generic },
> > +	{ .compatible = "fsl,imx8qm-mu", .data = &imx_mu_cfg_generic },
> > +	{ .compatible = "fsl,imx8qxp-mu", .data = &imx_mu_cfg_generic },
> 
> It sounds like the list will be constantly extending. Is there any chance to
> introduce just one generic compatible in the driver, and describe the whole
> set of compatibles in documentation section only?
> 

I think only keeping "fsl,imx6sx-mu" should be enough. The ulp is slightly different,
but could be extended later.

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");
> >
> 
> --
> Best wishes,
> Vladimir

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

[Oleksij Rempel]

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

Hi Vladimir,

On Wed, Jul 18, 2018 at 10:57:42AM +0300, Vladimir Zapolskiy wrote:
> Hi Oleksij,
> 
> On 07/18/2018 10:12 AM, Oleksij Rempel wrote:
> > 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 | 300 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 308 insertions(+)
> >  create mode 100644 drivers/mailbox/imx-mailbox.c
> > 
> > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> > index a2bb27446dce..79060ddc380d 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 "i.MX Mailbox"
> > +	depends on ARCH_MXC || COMPILE_TEST
> > +	help
> > +	  Mailbox implementation for i.MX 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..ad8797127b1f
> > --- /dev/null
> > +++ b/drivers/mailbox/imx-mailbox.c
> > @@ -0,0 +1,300 @@
> > +// 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 + (3 - (x)))
> > +#define IMX_MU_xSR_RFn(x)	BIT(24 + (3 - (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 + (3 - (x)))
> > +/* Receive Interrupt Enable */
> > +#define IMX_MU_xCR_RIEn(x)	BIT(24 + (3 - (x)))
> > +
> > +#define IMX_MU_MAX_CHANS	4u
> > +
> > +struct imx_mu_priv;
> > +
> > +struct imx_mu_cfg {
> > +	unsigned int		chans;
> 
> Basically this field is not used, everywhere in the driver its usage can
> be replaced by IMX_MU_MAX_CHANS, and it makes sense to rename the latter,
> and 'chan' local variable from .probe is also removed.

except of one channel implementation for SCU which should be added as
next step.
But for now, sure, I can remove it.

> I suggest that you add this field at the time when you to add controller
> specific data other than 'imx_mu_cfg_generic'. 
> 
> > +	void (*init_hw)(struct imx_mu_priv *priv);
> > +};
> > +
> > +struct imx_mu_con_priv {
> > +	int			irq;
> > +	unsigned int		idx;
> > +	char			*irq_desc;
> > +};
> > +
> > +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;
> > +
> > +	bool			side_b;
> > +};
> > +
> > +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, ctrl, dat;
> > +
> > +	ctrl = imx_mu_read(priv, IMX_MU_xCR);
> > +	val = imx_mu_read(priv, IMX_MU_xSR);
> > +	val &= IMX_MU_xSR_TEn(cp->idx) | IMX_MU_xSR_RFn(cp->idx);
> > +	val &= ctrl & (IMX_MU_xCR_TIEn(cp->idx) | IMX_MU_xCR_RIEn(cp->idx));
> > +	if (!val)
> > +		return IRQ_NONE;
> > +
> > +	if (val & IMX_MU_xSR_TEn(cp->idx)) {
> > +		imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp->idx));
> > +		mbox_chan_txdone(chan, 0);
> > +	}
> > +
> > +	if (val & IMX_MU_xSR_RFn(cp->idx)) {
> > +		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->idx)));
> 
> Because the function returns bool value, double negation is not needed.
> 
> 	return val & IMX_MU_xSR_TEn(cp->idx);
> 
> is good enough.

Here is objdump of two version build for cortex m4.
My version:
        return readl(addr);
   c:   f3bf 8f4f       dsb     sy
        return (!!(val & IMX_MU_xSR_TEn(cp->idx)));
  10:   6844            ldr     r4, [r0, #4]
        u32 *arg = data;

        if (!imx_mu_last_tx_done(chan))
  12:   f1c4 0517       rsb     r5, r4, #23
  16:   40ea            lsrs    r2, r5
  18:   07d2            lsls    r2, r2, #31
  1a:   d51b            bpl.n   54 <imx_mu_send_data+0x54>
        iowrite32(val, priv->base + offs);
  1c:   f853 2c08       ldr.w   r2, [r3, #-8]
                return -EBUSY;

Your version:
   c:   f3bf 8f4f       dsb     sy
        return val & IMX_MU_xSR_TEn(cp->idx);
  10:   6865            ldr     r5, [r4, #4]
        u32 *arg = data;

        if (!imx_mu_last_tx_done(chan))
  12:   2201            movs    r2, #1
  14:   f1c5 0017       rsb     r0, r5, #23
  18:   fa02 f000       lsl.w   r0, r2, r0
  1c:   4230            tst     r0, r6
  1e:   d019            beq.n   54 <imx_mu_send_data+0x54>
        iowrite32(val, priv->base + offs);
  20:   f853 0c08       ldr.w   r0, [r3, #-8]
                return -EBUSY;

Why your suggestion need more instructions?


> > +}
> > +
> > +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->idx), 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;
> > +
> > +	cp->irq_desc = devm_kasprintf(priv->dev, GFP_KERNEL, "imx_mu_chan[%i]",
> > +				      cp->idx);
> > +	if (!cp->irq_desc)
> > +		return -ENOMEM;
> > +
> > +	ret = devm_request_irq(priv->dev, cp->irq, imx_mu_isr,
> > +			       IRQF_SHARED, cp->irq_desc, chan);
> > +	if (ret) {
> > +		dev_err(priv->dev,
> > +			"Unable to acquire IRQ %d\n", cp->irq);
> > +		return ret;
> > +	}
> > +
> > +	imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->idx), 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->idx) | IMX_MU_xCR_RIEn(cp->idx));
> > +
> > +	devm_free_irq(priv->dev, cp->irq, chan);
> > +	devm_kfree(priv->dev, cp->irq_desc);
> > +}
> > +
> > +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 device_node *np = dev->of_node;
> > +	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;
> 
> Please don't check or handle 'irq == 0' case specially, it is dead code.
> 
> 	if (irq < 0)
> 		return irq;
> 
> is good enough.

ok

> > +
> > +	priv->clk = devm_clk_get(dev, NULL);
> > +	if (IS_ERR(priv->clk)) {
> > +		if (PTR_ERR(priv->clk) != -ENOENT)
> > +			return PTR_ERR(priv->clk);
> > +
> > +		priv->clk = NULL;
> > +	}
> > +
> > +	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 */
> 
> The comment above is trivial, please remove it.

ok

> > +	for (i = 0; i < chans; i++) {
> > +		struct imx_mu_con_priv *cp = &priv->con_priv[i];
> > +
> > +		cp->idx = i;
> > +		cp->irq = irq;
> 
> I read it as a single irq for all channels.
> 
> Why do you dynamically init more channel data imx_mu_startup() based on irq value?
> Aren't 'cp->irq_desc' all equal? Isn't just a single devm_request_irq() in .probe
> sufficient?

See previous discussion on v3

> > +		priv->mbox_chans[i].con_priv = cp;
> > +	}
> > +
> > +	if (of_property_read_bool(np, "fsl,mu-side-b"))
> > +		priv->side_b = true;
> > +
> > +	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_generic(struct imx_mu_priv *priv)
> > +{
> > +	if (priv->side_b)
> > +		return;
> > +
> > +	/* Set default MU configuration */
> > +	imx_mu_write(priv, 0, IMX_MU_xCR);
> > +}
> > +
> > +static const struct imx_mu_cfg imx_mu_cfg_generic = {
> > +	.chans = IMX_MU_MAX_CHANS,
> 
> Here IMX_MU_MAX_CHANS macro name is questionable, see my top comment.
> For clarity here 'MAX' is not expected, it shall be the exact controller
> specific value, something like s/MAX/NUM/ may be considered.

this MU provide 4+4 interrupts for TX/RX registers and 4 general purpose interrupts
which can be reused for mailbox with shared memory or as irq controller.
The NXP implementation of SCU on i.MX8 is using one MU as one channel.

I'm not sure how many channels should be defined as it is endproduct
specific. So far MAX == 4 until other already existing implementations go
upstream.

> > +	.init_hw = imx_mu_init_generic,
> > +};
> > +
> > +static const struct of_device_id imx_mu_dt_ids[] = {
> > +	{ .compatible = "fsl,imx6sx-mu", .data = &imx_mu_cfg_generic },
> > +	{ .compatible = "fsl,imx7d-mu", .data = &imx_mu_cfg_generic },
> > +	{ .compatible = "fsl,imx7s-mu", .data = &imx_mu_cfg_generic },
> > +	{ .compatible = "fsl,imx7ulp-mu", .data = &imx_mu_cfg_generic },
> > +	{ .compatible = "fsl,imx8qm-mu", .data = &imx_mu_cfg_generic },
> > +	{ .compatible = "fsl,imx8qxp-mu", .data = &imx_mu_cfg_generic },
> 
> It sounds like the list will be constantly extending. Is there any chance
> to introduce just one generic compatible in the driver, and describe
> the whole set of compatibles in documentation section only?

Experience with iMX* IPs showed - it is worth do describe each IP in each
SoC with SoC specific name. See SPI, UART and other iMX driver. Why we
should make here an exception?

> > +	{ },
> > +};
> > +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");
> > 
> 
> --
> Best wishes,
> Vladimir
> 

-- 
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 v4 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: Saturday, July 21, 2018 7:40 PM
> To: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.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>; A.s. Dong <aisheng.dong@nxp.com>;
> kernel@pengutronix.de; linux-arm-kernel@lists.infradead.org;
> devicetree@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v4 4/4] mailbox: Add support for i.MX7D messaging unit
> 

[...]

> > > +
> > > +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_generic(struct imx_mu_priv *priv) {
> > > +	if (priv->side_b)
> > > +		return;
> > > +
> > > +	/* Set default MU configuration */
> > > +	imx_mu_write(priv, 0, IMX_MU_xCR); }
> > > +
> > > +static const struct imx_mu_cfg imx_mu_cfg_generic = {
> > > +	.chans = IMX_MU_MAX_CHANS,
> >
> > Here IMX_MU_MAX_CHANS macro name is questionable, see my top
> comment.
> > For clarity here 'MAX' is not expected, it shall be the exact
> > controller specific value, something like s/MAX/NUM/ may be considered.
> 
> this MU provide 4+4 interrupts for TX/RX registers and 4 general purpose
> interrupts which can be reused for mailbox with shared memory or as irq
> controller.
> The NXP implementation of SCU on i.MX8 is using one MU as one channel.
> 
> I'm not sure how many channels should be defined as it is endproduct
> specific. So far MAX == 4 until other already existing implementations go
> upstream.
> 

4 is ok to me currently.

> > > +	.init_hw = imx_mu_init_generic,
> > > +};
> > > +
> > > +static const struct of_device_id imx_mu_dt_ids[] = {
> > > +	{ .compatible = "fsl,imx6sx-mu", .data = &imx_mu_cfg_generic },
> > > +	{ .compatible = "fsl,imx7d-mu", .data = &imx_mu_cfg_generic },
> > > +	{ .compatible = "fsl,imx7s-mu", .data = &imx_mu_cfg_generic },
> > > +	{ .compatible = "fsl,imx7ulp-mu", .data = &imx_mu_cfg_generic },
> > > +	{ .compatible = "fsl,imx8qm-mu", .data = &imx_mu_cfg_generic },
> > > +	{ .compatible = "fsl,imx8qxp-mu", .data = &imx_mu_cfg_generic },
> >
> > It sounds like the list will be constantly extending. Is there any
> > chance to introduce just one generic compatible in the driver, and
> > describe the whole set of compatibles in documentation section only?
> 
> Experience with iMX* IPs showed - it is worth do describe each IP in each SoC
> with SoC specific name. See SPI, UART and other iMX driver. Why we should
> make here an exception?
> 

Even SPI, UART driver you mentioned here do not describe all the SoC compatible
strings. New ones are usually added when we need different SoC specific .data.
But here all SoC specific .data actually is the same one which seems like an
unnecessary thing in driver.

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");
> > >
> >
> > --
> > Best wishes,
> > Vladimir
> >
> 
> --
> 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 |