[PATCH v6 0/5] add mailbox support for i.MX7D

[PATCH v6 0/5] add mailbox support for i.MX7D

[Oleksij Rempel]

20180622 changes v6:
- include one more patch provided by Aisheng
- DT: add fall back compatible fsl,imx6sx-mu
- imx-mailbox: for now, use only fsl,imx6sx-mu

20180621 changes v5:
- DT: revert most of the changes from previous version
- imx-mailbox: remove struct imx_mu_cfg
- imx-mailbox: remove !! from imx_mu_last_tx_done()

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 (2):
  dt-bindings: mailbox: allow mbox-cells to be equal to 0
  dt-bindings: arm: fsl: add mu binding doc

Oleksij Rempel (3):
  dt-bindings: mailbox: imx-mu: add generic MU channel support
  ARM: dts: imx7s: add i.MX7 messaging unit support
  mailbox: Add support for i.MX7D messaging unit

 .../devicetree/bindings/mailbox/fsl,mu.txt    |  41 +++
 .../devicetree/bindings/mailbox/mailbox.txt   |   3 +-
 arch/arm/boot/dts/imx7s.dtsi                  |  19 ++
 drivers/mailbox/Kconfig                       |   6 +
 drivers/mailbox/Makefile                      |   2 +
 drivers/mailbox/imx-mailbox.c                 | 273 ++++++++++++++++++
 6 files changed, 342 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mailbox/fsl,mu.txt
 create mode 100644 drivers/mailbox/imx-mailbox.c

-- 
2.18.0

[PATCH v6 1/5] dt-bindings: mailbox: allow mbox-cells to be equal to 0

[Oleksij Rempel]

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

Mailbox devices may have only one channel which means the mbox-cells
at least 1 does not make sense for this type devices. Let's remove
that limitation to allow the mbox-cells to be equal to 0.

Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 Documentation/devicetree/bindings/mailbox/mailbox.txt | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/mailbox/mailbox.txt b/Documentation/devicetree/bindings/mailbox/mailbox.txt
index af8ecee2ac68..c2fcd054141a 100644
--- a/Documentation/devicetree/bindings/mailbox/mailbox.txt
+++ b/Documentation/devicetree/bindings/mailbox/mailbox.txt
@@ -6,8 +6,7 @@ assign appropriate mailbox channel to client drivers.
 * Mailbox Controller
 
 Required property:
-- #mbox-cells: Must be at least 1. Number of cells in a mailbox
-		specifier.
+- #mbox-cells: Number of cells in a mailbox specifier.
 
 Example:
 	mailbox: mailbox {
-- 
2.18.0

[PATCH v6 2/5] 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 v6 3/5] dt-bindings: mailbox: imx-mu: add generic MU channel support

[Oleksij Rempel]

Each MU has four pairs of rx/tx data register with four rx/tx interrupts
which can also be used as a separate channel.

Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 Documentation/devicetree/bindings/mailbox/fsl,mu.txt | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
index 90e4905dfc69..113d6ab931ef 100644
--- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
+++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
@@ -22,7 +22,14 @@ Required properties:
 - 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 v6 4/5] 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.

Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
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..191a0286fa3b 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", "fsl,imx6sx-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", "fsl,imx6sx-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 v6 5/5] 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.

Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/mailbox/Kconfig       |   6 +
 drivers/mailbox/Makefile      |   2 +
 drivers/mailbox/imx-mailbox.c | 273 ++++++++++++++++++++++++++++++++++
 3 files changed, 281 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..29cf2876db01
--- /dev/null
+++ b/drivers/mailbox/imx-mailbox.c
@@ -0,0 +1,273 @@
+// 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_CHANS	4u
+
+struct imx_mu_con_priv {
+	int			irq;
+	unsigned int		idx;
+	char			*irq_desc;
+};
+
+struct imx_mu_priv {
+	struct device		*dev;
+	void __iomem		*base;
+
+	struct mbox_controller	mbox;
+	struct mbox_chan	mbox_chans[IMX_MU_CHANS];
+
+	struct imx_mu_con_priv  con_priv[IMX_MU_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 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 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;
+	unsigned int i;
+	int irq, ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	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;
+
+	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;
+	}
+
+	for (i = 0; i < IMX_MU_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 = IMX_MU_CHANS;
+	priv->mbox.txdone_irq = true;
+
+	platform_set_drvdata(pdev, priv);
+
+	imx_mu_init_generic(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 const struct of_device_id imx_mu_dt_ids[] = {
+	{ .compatible = "fsl,imx6sx-mu" },
+	{ },
+};
+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 v6 0/5] add mailbox support for i.MX7D

[A.s. Dong]

Hi Oleksij,

Thanks for the work.

Hi Jassi,
Do you think if we can have your tag and let those patches go through
Shawn's tree as the later QXP support patches depends on them?

Regards
Dong Aisheng

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> Sent: Sunday, July 22, 2018 2:39 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>;
> Vladimir Zapolskiy <vladimir_zapolskiy@mentor.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 v6 0/5] add mailbox support for i.MX7D
> 
> 20180622 changes v6:
> - include one more patch provided by Aisheng
> - DT: add fall back compatible fsl,imx6sx-mu
> - imx-mailbox: for now, use only fsl,imx6sx-mu
> 
> 20180621 changes v5:
> - DT: revert most of the changes from previous version
> - imx-mailbox: remove struct imx_mu_cfg
> - imx-mailbox: remove !! from imx_mu_last_tx_done()
> 
> 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 (2):
>   dt-bindings: mailbox: allow mbox-cells to be equal to 0
>   dt-bindings: arm: fsl: add mu binding doc
> 
> Oleksij Rempel (3):
>   dt-bindings: mailbox: imx-mu: add generic MU channel support
>   ARM: dts: imx7s: add i.MX7 messaging unit support
>   mailbox: Add support for i.MX7D messaging unit
> 
>  .../devicetree/bindings/mailbox/fsl,mu.txt    |  41 +++
>  .../devicetree/bindings/mailbox/mailbox.txt   |   3 +-
>  arch/arm/boot/dts/imx7s.dtsi                  |  19 ++
>  drivers/mailbox/Kconfig                       |   6 +
>  drivers/mailbox/Makefile                      |   2 +
>  drivers/mailbox/imx-mailbox.c                 | 273 ++++++++++++++++++
>  6 files changed, 342 insertions(+), 2 deletions(-)  create mode 100644
> Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>  create mode 100644 drivers/mailbox/imx-mailbox.c
> 
> --
> 2.18.0

Re: [PATCH v6 0/5] add mailbox support for i.MX7D

[Jassi Brar]

On Sun, Jul 22, 2018 at 4:14 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
> Hi Oleksij,
>
> Thanks for the work.
>
> Hi Jassi,
> Do you think if we can have your tag and let those patches go through
> Shawn's tree as the later QXP support patches depends on them?
>
Hmm...  I don't find the 5 patches in my mailbox. Are you sure I was CC'ed?

 Dong Aisheng (2):
   dt-bindings: mailbox: allow mbox-cells to be equal to 0
   dt-bindings: arm: fsl: add mu binding doc

 Oleksij Rempel (3):
   dt-bindings: mailbox: imx-mu: add generic MU channel support
   ARM: dts: imx7s: add i.MX7 messaging unit support
   mailbox: Add support for i.MX7D messaging unit

Re: [PATCH v6 4/5] ARM: dts: imx7s: add i.MX7 messaging unit support

[Lucas Stach]

Am Sonntag, den 22.07.2018, 08:39 +0200 schrieb Oleksij Rempel:
> Define the Messaging Unit (MU) for i.MX7 in the processor's dtsi.
> The respective driver is added in the next commit.
> 
> > Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
> > 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..191a0286fa3b 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", "fsl,imx6sx-mu";

Those compatibles are missing documentation in the binding.

> +				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", "fsl,imx6sx-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>;

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

Re: [PATCH v6 5/5] mailbox: Add support for i.MX7D messaging unit

[Lucas Stach]

Am Sonntag, den 22.07.2018, 08:39 +0200 schrieb 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.
> 
> > Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/mailbox/Kconfig       |   6 +
>  drivers/mailbox/Makefile      |   2 +
>  drivers/mailbox/imx-mailbox.c | 273 ++++++++++++++++++++++++++++++++++
>  3 files changed, 281 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..29cf2876db01
> --- /dev/null
> +++ b/drivers/mailbox/imx-mailbox.c
> @@ -0,0 +1,273 @@
> +// 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_CHANS	4u
> +
> +struct imx_mu_con_priv {
> > > +	int			irq;
> > > +	unsigned int		idx;
> > > +	char			*irq_desc;
> +};
> +
> +struct imx_mu_priv {
> > > +	struct device		*dev;
> > > +	void __iomem		*base;
> +
> > > +	struct mbox_controller	mbox;
> > > +	struct mbox_chan	mbox_chans[IMX_MU_CHANS];
> +
> > +	struct imx_mu_con_priv  con_priv[IMX_MU_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);

This driver is never going to be used on a device with port based IO,
so iowrite doesn't make much sense here, just use writel. Same comment
applies to the below read function.

Also, given that those functions are not really shortening the code in
the user they may also be removed completely IMHO.
  
> +}
> +
> +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);

I guess
"return imx_mu_read(priv, IMX_MU_xSR) & IMX_MU_xSR_TEn(cp->idx);" is
shorter and equally well understood.

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

In multi-channel mode this RMW cycle needs some kind of locking. As
this register is also changed from the irq handler, this probably needs
to be a irqsave spinlock.

> +
> > +	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);

Using the devm_ variants of those functions doesn't make sense when the
resources aren't tied to the device lifetime. As you are tearing them
down manually in imx_mu_shutdown anyways, just use the raw variants of
those functions.

> +	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 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 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;
> > +	unsigned int i;
> > +	int irq, ret;
> +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> +
> > +	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;
> +
> > +	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;
> > +	}
> +
> > +	for (i = 0; i < IMX_MU_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;

No need for the if clause here. Just assign the return value from
of_property_read_bool to priv->side_b.

> +
> > +	priv->mbox.dev = dev;
> > +	priv->mbox.ops = &imx_mu_ops;
> > +	priv->mbox.chans = priv->mbox_chans;
> > +	priv->mbox.num_chans = IMX_MU_CHANS;
> > +	priv->mbox.txdone_irq = true;
> +
> > +	platform_set_drvdata(pdev, priv);
> +
> > +	imx_mu_init_generic(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 const struct of_device_id imx_mu_dt_ids[] = {
> > +	{ .compatible = "fsl,imx6sx-mu" },
> > +	{ },
> +};
> +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");

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

Re: [PATCH v6 5/5] mailbox: Add support for i.MX7D messaging unit

[Oleksij Rempel]

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

On Mon, Jul 23, 2018 at 07:19:40PM +0200, Lucas Stach wrote:
> Am Sonntag, den 22.07.2018, 08:39 +0200 schrieb 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.
> > 
> > > Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
> > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  drivers/mailbox/Kconfig       |   6 +
> >  drivers/mailbox/Makefile      |   2 +
> >  drivers/mailbox/imx-mailbox.c | 273 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 281 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..29cf2876db01
> > --- /dev/null
> > +++ b/drivers/mailbox/imx-mailbox.c
> > @@ -0,0 +1,273 @@
> > +// 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_CHANS	4u
> > +
> > +struct imx_mu_con_priv {
> > > > +	int			irq;
> > > > +	unsigned int		idx;
> > > > +	char			*irq_desc;
> > +};
> > +
> > +struct imx_mu_priv {
> > > > +	struct device		*dev;
> > > > +	void __iomem		*base;
> > +
> > > > +	struct mbox_controller	mbox;
> > > > +	struct mbox_chan	mbox_chans[IMX_MU_CHANS];
> > +
> > > +	struct imx_mu_con_priv  con_priv[IMX_MU_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);
> 
> This driver is never going to be used on a device with port based IO,
> so iowrite doesn't make much sense here, just use writel. Same comment
> applies to the below read function.

I read this and do not understand what is actual problem:
https://lwn.net/Articles/102232/
"By default, these functions are simply wrappers around readb() and friends.
The explicit pointer type for the argument will generate warnings, however,
if a driver passes in an integer type."

So, is it wrong or not? If yes, why?

> Also, given that those functions are not really shortening the code in
> the user they may also be removed completely IMHO.

Is it subjective or technical issue?

> > +}
> > +
> > +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);
> 
> I guess
> "return imx_mu_read(priv, IMX_MU_xSR) & IMX_MU_xSR_TEn(cp->idx);" is
> shorter and equally well understood.

Same as before:
Is it subjective or technical issue?

> > +}
> > +
> > +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);
> 
> In multi-channel mode this RMW cycle needs some kind of locking. As
> this register is also changed from the irq handler, this probably needs
> to be a irqsave spinlock.

ok.

> > +
> > > +	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);
> 
> Using the devm_ variants of those functions doesn't make sense when the
> resources aren't tied to the device lifetime. As you are tearing them
> down manually in imx_mu_shutdown anyways, just use the raw variants of
> those functions.

So, it is not a problem and there is no advantage. Right?

> > +	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 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 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;
> > > +	unsigned int i;
> > > +	int irq, ret;
> > +
> > > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > +	if (!priv)
> > > +		return -ENOMEM;
> > +
> > > +	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;
> > +
> > > +	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;
> > > +	}
> > +
> > > +	for (i = 0; i < IMX_MU_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;
> 
> No need for the if clause here. Just assign the return value from
> of_property_read_bool to priv->side_b.

Not against kernel style and not broken.
Is it subjective or technical issue? I assume I like more red color...

> > +
> > > +	priv->mbox.dev = dev;
> > > +	priv->mbox.ops = &imx_mu_ops;
> > > +	priv->mbox.chans = priv->mbox_chans;
> > > +	priv->mbox.num_chans = IMX_MU_CHANS;
> > > +	priv->mbox.txdone_irq = true;
> > +
> > > +	platform_set_drvdata(pdev, priv);
> > +
> > > +	imx_mu_init_generic(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 const struct of_device_id imx_mu_dt_ids[] = {
> > > +	{ .compatible = "fsl,imx6sx-mu" },
> > > +	{ },
> > +};
> > +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");
> 

-- 
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 v6 5/5] mailbox: Add support for i.MX7D messaging unit

[Vladimir Zapolskiy]

Hi Oleksij,

On 07/22/2018 09:39 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.
> 
> Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

[snip]

> +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;
> +

Again I would suggest to move this allocation to the loop in .probe function.

[snip]

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

^^^^ right over here.

> +		priv->mbox_chans[i].con_priv = cp;
> +	}
> +

please feel free to add my technical side review tag to the next version:

Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>

FWIW I find that review comments from Lucas are pretty valid, I would
recommend to incorporate them.

--
Best wishes,
Vladimir

RE: [PATCH v6 4/5] ARM: dts: imx7s: add i.MX7 messaging unit support

[A.s. Dong]

> -----Original Message-----
> From: Lucas Stach [mailto:l.stach@pengutronix.de]
> Sent: Tuesday, July 24, 2018 12:57 AM
> 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>; Vladimir Zapolskiy
> <vladimir_zapolskiy@mentor.com>
> Cc: dl-linux-imx <linux-imx@nxp.com>; linux-arm-kernel@lists.infradead.org;
> kernel@pengutronix.de; devicetree@vger.kernel.org
> Subject: Re: [PATCH v6 4/5] ARM: dts: imx7s: add i.MX7 messaging unit
> support
> 
> Am Sonntag, den 22.07.2018, 08:39 +0200 schrieb Oleksij Rempel:
> > Define the Messaging Unit (MU) for i.MX7 in the processor's dtsi.
> > The respective driver is added in the next commit.
> >
> > > Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
> > > 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..191a0286fa3b 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", "fsl,imx6sx-mu";
> 
> Those compatibles are missing documentation in the binding.
> 

Sorry for the overlooking.
They seem to be got missed during a updated binding doc review.

Oleksij,
Would you add them back and resend the patch series?

Regards
Dong Aisheng

> > +				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", "fsl,imx6sx-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>;
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v6 5/5] mailbox: Add support for i.MX7D messaging unit

[A.s. Dong]

> -----Original Message-----
> From: Lucas Stach [mailto:l.stach@pengutronix.de]
> Sent: Tuesday, July 24, 2018 1:20 AM
> 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>; Vladimir Zapolskiy
> <vladimir_zapolskiy@mentor.com>
> Cc: dl-linux-imx <linux-imx@nxp.com>; linux-arm-kernel@lists.infradead.org;
> kernel@pengutronix.de; devicetree@vger.kernel.org
> Subject: Re: [PATCH v6 5/5] mailbox: Add support for i.MX7D messaging unit
> 

[...]

> > +}
> > +
> > +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);
> 
> In multi-channel mode this RMW cycle needs some kind of locking. As this
> register is also changed from the irq handler, this probably needs to be a
> irqsave spinlock.
> 

Good catch!
They do need to be protected.

Regards
Dong Aisheng

RE: [PATCH v6 5/5] mailbox: Add support for i.MX7D messaging unit

[A.s. Dong]

> -----Original Message-----
> From: Vladimir Zapolskiy [mailto:vz@mleia.com]
> Sent: Tuesday, July 24, 2018 7:31 AM
> 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>; Vladimir Zapolskiy
> <vladimir_zapolskiy@mentor.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 v6 5/5] mailbox: Add support for i.MX7D messaging unit
> 
> Hi Oleksij,
> 
> On 07/22/2018 09:39 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.
> >
> > Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> 
> [snip]
> 
> > +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;
> > +
> 
> Again I would suggest to move this allocation to the loop in .probe function.
> 
> [snip]
> 

+1
Good point.

> > +
> > +	for (i = 0; i < IMX_MU_CHANS; i++) {
> > +		struct imx_mu_con_priv *cp = &priv->con_priv[i];
> > +
> > +		cp->idx = i;
> > +		cp->irq = irq;
> 
> ^^^^ right over here.
> 
> > +		priv->mbox_chans[i].con_priv = cp;
> > +	}
> > +
> 
> please feel free to add my technical side review tag to the next version:
> 
> Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>
> 
> FWIW I find that review comments from Lucas are pretty valid, I would
> recommend to incorporate them.

+1

Regards
Dong Aisheng

> 
> --
> Best wishes,
> Vladimir

RE: [PATCH v6 0/5] add mailbox support for i.MX7D

[A.s. Dong]

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> Sent: Monday, July 23, 2018 11:33 PM
> To: 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>; Shawn Guo <shawnguo@kernel.org>; Fabio
> Estevam <fabio.estevam@nxp.com>; Rob Herring <robh+dt@kernel.org>;
> Mark Rutland <mark.rutland@arm.com>; Vladimir Zapolskiy
> <vladimir_zapolskiy@mentor.com>
> Subject: Re: [PATCH v6 0/5] add mailbox support for i.MX7D
> 
> On Sun, Jul 22, 2018 at 4:14 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
> > Hi Oleksij,
> >
> > Thanks for the work.
> >
> > Hi Jassi,
> > Do you think if we can have your tag and let those patches go through
> > Shawn's tree as the later QXP support patches depends on them?
> >
> Hmm...  I don't find the 5 patches in my mailbox. Are you sure I was CC'ed?

I guess you are missed to be CC'ed.

Oleksij,
As there're some new review comments and may need a small change,
please do not miss to CC Jassi (Mailbox Maintainer) in the next version
of your patch series.

Regards
Dong Aisheng

> 
>  Dong Aisheng (2):
>    dt-bindings: mailbox: allow mbox-cells to be equal to 0
>    dt-bindings: arm: fsl: add mu binding doc
> 
>  Oleksij Rempel (3):
>    dt-bindings: mailbox: imx-mu: add generic MU channel support
>    ARM: dts: imx7s: add i.MX7 messaging unit support
>    mailbox: Add support for i.MX7D messaging unit

Re: [PATCH v6 5/5] mailbox: Add support for i.MX7D messaging unit

[Oleksij Rempel]

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



On 24.07.2018 01:31, Vladimir Zapolskiy wrote:
> Hi Oleksij,
> 
> On 07/22/2018 09:39 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.
>>
>> Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> 
> [snip]
> 
>> +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;
>> +
> 
> Again I would suggest to move this allocation to the loop in .probe function.

Why? In my case I use 2 channels. Why should I allocate 4 descriptors
for using only 2 of them?

The SCU driver will use only one channel...

> [snip]
> 
>> +
>> +	for (i = 0; i < IMX_MU_CHANS; i++) {
>> +		struct imx_mu_con_priv *cp = &priv->con_priv[i];
>> +
>> +		cp->idx = i;
>> +		cp->irq = irq;
> 
> ^^^^ right over here.
> 
>> +		priv->mbox_chans[i].con_priv = cp;
>> +	}
>> +
> 
> please feel free to add my technical side review tag to the next version:
> 
> Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>
> 
> FWIW I find that review comments from Lucas are pretty valid, I would
> recommend to incorporate them.

Ok


[-- 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 v6 5/5] mailbox: Add support for i.MX7D messaging unit

[Oleksij Rempel]

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

Hi Lucas,

here is more detailed response:

On 23.07.2018 19:19, Lucas Stach wrote:
> Am Sonntag, den 22.07.2018, 08:39 +0200 schrieb 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.
>>
>>> Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>> ---
>>  drivers/mailbox/Kconfig       |   6 +
>>  drivers/mailbox/Makefile      |   2 +
>>  drivers/mailbox/imx-mailbox.c | 273 ++++++++++++++++++++++++++++++++++
>>  3 files changed, 281 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..29cf2876db01
>> --- /dev/null
>> +++ b/drivers/mailbox/imx-mailbox.c
>> @@ -0,0 +1,273 @@
>> +// 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_CHANS	4u
>> +
>> +struct imx_mu_con_priv {
>>>> +	int			irq;
>>>> +	unsigned int		idx;
>>>> +	char			*irq_desc;
>> +};
>> +
>> +struct imx_mu_priv {
>>>> +	struct device		*dev;
>>>> +	void __iomem		*base;
>> +
>>>> +	struct mbox_controller	mbox;
>>>> +	struct mbox_chan	mbox_chans[IMX_MU_CHANS];
>> +
>>> +	struct imx_mu_con_priv  con_priv[IMX_MU_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);
> 
> This driver is never going to be used on a device with port based IO,
> so iowrite doesn't make much sense here, just use writel. Same comment
> applies to the below read function.

As before I don't really understand the difference. I took some time for
learning and here are my findings:
- historical background of ioread/iowrite functions. What I can find is
this LWN article: https://lwn.net/Articles/102232/ . The main point
seems to be "The first of these is a new __iomem annotation used to mark
pointers to I/O memory" ... " As with __user, the __iomem marker serves
a documentation role in the kernel code;"

In modern kernel the difference between ioread32 and readl seems to be
completely disappeared.
with this LWN article (2016):
https://lwn.net/Articles/698014/
the readl and reade32 are always in the same group..

If there is some documentation which will say why I should use readl()
instead of ioread32() i would really love to read it.

> Also, given that those functions are not really shortening the code in
> the user they may also be removed completely IMHO.

Wrong assumption..  I use this functions for tracing. It is just to easy
to add two trace points...  From technical perspective I don't see any
advantage or disadvantage of having this functions.
If it is my personal preference, then I decide to keep it.

>> +}
>> +
>> +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);
> 
> I guess
> "return imx_mu_read(priv, IMX_MU_xSR) & IMX_MU_xSR_TEn(cp->idx);" is
> shorter and equally well understood.

So, previous comment was saying, "those functions are not really
shortening the code"

I would apply same comment: "those .. are not really shortening the code".

Do we really need to make review of "personal coding style
preferences".. which are not against kernel coding style in 6. review
round...?!
For example:
kernel coding style is pink... in this case we are talking about
difference between lavender pink and carnation pink.


>> +}
>> +
>> +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);
> 
> In multi-channel mode this RMW cycle needs some kind of locking. As
> this register is also changed from the irq handler, this probably needs
> to be a irqsave spinlock.

Thank you, i need to fix it.

>> +
>>> +	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);
> 
> Using the devm_ variants of those functions doesn't make sense when the
> resources aren't tied to the device lifetime. As you are tearing them
> down manually in imx_mu_shutdown anyways, just use the raw variants of
> those functions.

I have nothing against it.

>> +	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 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 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;
>>> +	unsigned int i;
>>> +	int irq, ret;
>> +
>>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>> +	if (!priv)
>>> +		return -ENOMEM;
>> +
>>> +	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;
>> +
>>> +	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;
>>> +	}
>> +
>>> +	for (i = 0; i < IMX_MU_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;
> 
> No need for the if clause here. Just assign the return value from
> of_property_read_bool to priv->side_b.

ok.

Thank you for a review!


[-- 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 v6 5/5] mailbox: Add support for i.MX7D messaging unit

[Lucas Stach]

Am Dienstag, den 24.07.2018, 07:14 +0200 schrieb Oleksij Rempel:
> Hi Lucas,
> 
> here is more detailed response:
> 
> On 23.07.2018 19:19, Lucas Stach wrote:
[...]
> > > +};
> > > +
> > > +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);
> > 
> > This driver is never going to be used on a device with port based IO,
> > so iowrite doesn't make much sense here, just use writel. Same comment
> > applies to the below read function.
> 
> As before I don't really understand the difference. I took some time for
> learning and here are my findings:
> - historical background of ioread/iowrite functions. What I can find is
> > this LWN article: https://lwn.net/Articles/102232/ . The main point
> seems to be "The first of these is a new __iomem annotation used to mark
> pointers to I/O memory" ... " As with __user, the __iomem marker serves
> a documentation role in the kernel code;"
> 
> In modern kernel the difference between ioread32 and readl seems to be
> completely disappeared.
> with this LWN article (2016):
> https://lwn.net/Articles/698014/
> the readl and reade32 are always in the same group..

Okay, as discussed offline this is more a thing of personal preference,
since this maps to the same thing internally when used on a
architecture without IO ports. I slightly dislike those as they don't
have a _relaxed counterpart, but other than that there should be no
difference. As we don't use any of the relaxed stuff in this driver
it's really about the color of the bikeshed, so feel free to keep it
your way.

> If there is some documentation which will say why I should use readl()
> instead of ioread32() i would really love to read it.
> 
> > Also, given that those functions are not really shortening the code in
> > the user they may also be removed completely IMHO.
> 
> Wrong assumption..  I use this functions for tracing. It is just to easy
> to add two trace points...  From technical perspective I don't see any
> advantage or disadvantage of having this functions.
> If it is my personal preference, then I decide to keep it.

That IMHO implies that I'm perfectly fine with you ignoring this
comment. :)

[...]
> > > +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);
> > 
> > Using the devm_ variants of those functions doesn't make sense when the
> > resources aren't tied to the device lifetime. As you are tearing them
> > down manually in imx_mu_shutdown anyways, just use the raw variants of
> > those functions.
> 
> I have nothing against it.

Thanks, as this is something I feel more strongly about. As the devm_
stuff is meant to tie the lifetime of an object to the device/driver
lifetime using them in a context where there is no relation at all is
kind of an API abuse to me.

Regards,
Lucas

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

Re: [PATCH v6 3/5] dt-bindings: mailbox: imx-mu: add generic MU channel support

[Rob Herring]

On Sun, Jul 22, 2018 at 08:39:21AM +0200, Oleksij Rempel wrote:
> Each MU has four pairs of rx/tx data register with four rx/tx interrupts
> which can also be used as a separate channel.
> 
> Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/mailbox/fsl,mu.txt | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

Reviewed-by: Rob Herring <robh@kernel.org>