[PATCH v3 0/3] add mailbox support for i.MX7D

[PATCH v3 0/3] add mailbox support for i.MX7D

[Oleksij Rempel]

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_MXS 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.


Oleksij Rempel (3):
  dt-bindings: arm: fsl: add mu binding 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                 | 294 ++++++++++++++++++
 5 files changed, 353 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/fsl,mu.txt
 create mode 100644 drivers/mailbox/imx-mailbox.c

-- 
2.18.0

[PATCH v3 1/3] dt-bindings: arm: fsl: add mu binding doc

[Oleksij Rempel]

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.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 .../devicetree/bindings/mailbox/fsl,mu.txt    | 32 +++++++++++++++++++
 1 file changed, 32 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..5d48dd75b98d
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
@@ -0,0 +1,32 @@
+NXP i.MX Messaging Unit (MU)
+--------------------------------------------------------------------
+
+Required properties:
+-------------------
+- compatible :	should be "fsl,<chip>-mu", the supported chips include:
+		imx6sx	- i.MX 6SoloX
+		imx7d	- i.MX 7Dual
+		imx7s	- i.MX 7Solo
+		imx7ulp	- i.MX 7ULP
+		imx8qm	- i.MX 8QM
+		imx8qxp	- 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 - 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-a : Should be set for side A MU.
+
+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 v3 2/3] 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..5bc0d4109da1 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>;
+				fsl,mu-side-a;
+				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>;
+				status = "disabled";
+			};
+
 			usbotg1: usb@30b10000 {
 				compatible = "fsl,imx7d-usb", "fsl,imx27-usb";
 				reg = <0x30b10000 0x200>;
-- 
2.18.0

[PATCH v3 3/3] 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 | 294 ++++++++++++++++++++++++++++++++++
 3 files changed, 302 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..31353366a007
--- /dev/null
+++ b/drivers/mailbox/imx-mailbox.c
@@ -0,0 +1,294 @@
+// 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;
+};
+
+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_a;
+};
+
+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;
+	char *irq_desc;
+	int ret;
+
+	irq_desc = devm_kasprintf(priv->dev, GFP_KERNEL, "imx_mu_chan[%i]",
+				  cp->idx);
+	if (!irq_desc)
+		return -ENOMEM;
+
+	ret = devm_request_irq(priv->dev, cp->irq, imx_mu_isr,
+			       IRQF_SHARED, 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);
+}
+
+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-a"))
+		priv->side_a = 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_imx7d(struct imx_mu_priv *priv)
+{
+	/* Set default config */
+	if (priv->side_a)
+		imx_mu_write(priv, 0, IMX_MU_xCR);
+}
+
+static const struct imx_mu_cfg imx_mu_cfg_imx7d = {
+	.chans = IMX_MU_MAX_CHANS,
+	.init_hw = imx_mu_init_imx7d,
+};
+
+static const struct of_device_id imx_mu_dt_ids[] = {
+	{ .compatible = "fsl,imx7s-mu", .data = &imx_mu_cfg_imx7d },
+	{ },
+};
+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 v3 1/3] dt-bindings: arm: fsl: add mu binding doc

[Rob Herring]

On Mon, Jul 16, 2018 at 01:42:07PM +0200, Oleksij Rempel wrote:
> 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.

v3? I already gave my R-by on v4 from Dong. Please coordinate your work 
on this.

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

RE: [PATCH v3 1/3] dt-bindings: arm: fsl: add mu binding doc

[A.s. Dong]

> -----Original Message-----
> From: Rob Herring [mailto:robh@kernel.org]
> Sent: Monday, July 16, 2018 11:17 PM
> To: Oleksij Rempel <o.rempel@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>; Fabio Estevam
> <fabio.estevam@nxp.com>; 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 v3 1/3] dt-bindings: arm: fsl: add mu binding doc
> 
> On Mon, Jul 16, 2018 at 01:42:07PM +0200, Oleksij Rempel wrote:
> > 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.
> 
> v3? I already gave my R-by on v4 from Dong. Please coordinate your work on
> this.

Okay.

Oleksij, 
could you re-add the M4 support part in a separate patch based on
the already acked SCU one?
I guess you can re-send with the acked one.

Regards
Dong Aisheng

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

Re: [PATCH v3 1/3] dt-bindings: arm: fsl: add mu binding doc

[Oleksij Rempel]

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

Hi,

On 17.07.2018 06:51, A.s. Dong wrote:
>> -----Original Message-----
>> From: Rob Herring [mailto:robh@kernel.org]
>> Sent: Monday, July 16, 2018 11:17 PM
>> To: Oleksij Rempel <o.rempel@pengutronix.de>
>> Cc: Shawn Guo <shawnguo@kernel.org>; Fabio Estevam
>> <fabio.estevam@nxp.com>; 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 v3 1/3] dt-bindings: arm: fsl: add mu binding doc
>>
>> On Mon, Jul 16, 2018 at 01:42:07PM +0200, Oleksij Rempel wrote:
>>> 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.
>>
>> v3? I already gave my R-by on v4 from Dong. Please coordinate your work on
>> this.
> 
> Okay.
> 
> Oleksij, 
> could you re-add the M4 support part in a separate patch based on
> the already acked SCU one?
> I guess you can re-send with the acked one.

Rob, what do you prefer: two patches for one new file or one squashed patch?


[-- 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 v3 1/3] dt-bindings: arm: fsl: add mu binding doc

[A.s. Dong]

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> Sent: Monday, July 16, 2018 7:42 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 v3 1/3] dt-bindings: arm: fsl: add mu binding doc
> 
> 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.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  .../devicetree/bindings/mailbox/fsl,mu.txt    | 32 +++++++++++++++++++
>  1 file changed, 32 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..5d48dd75b98d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> @@ -0,0 +1,32 @@
> +NXP i.MX Messaging Unit (MU)
> +--------------------------------------------------------------------
> +
> +Required properties:
> +-------------------
> +- compatible :	should be "fsl,<chip>-mu", the supported chips include:
> +		imx6sx	- i.MX 6SoloX
> +		imx7d	- i.MX 7Dual
> +		imx7s	- i.MX 7Solo
> +		imx7ulp	- i.MX 7ULP
> +		imx8qm	- i.MX 8QM
> +		imx8qxp	- 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 - 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-a : Should be set for side A MU.

For this property, how about doing like:
fsl,mu-side: An Integer represents the MU side. If missing this property, it's default to Side A
	       which is mostly used by A core.
	       0: MU Side A
	       1: MU Side B

Regards
Dong Aisheng
		
> +
> +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

RE: [PATCH v3 2/3] ARM: dts: imx7s: add i.MX7 messaging unit support

[A.s. Dong]

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> Sent: Monday, July 16, 2018 7:42 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 v3 2/3] ARM: dts: imx7s: add i.MX7 messaging unit support
> 
> 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..5bc0d4109da1 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>;
> +				fsl,mu-side-a;

Excpet this property which I commented in patch 1, otherwise, this path
Is okay to me.

Regards
Dong Aisheng

> +				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>;
> +				status = "disabled";
> +			};
> +
>  			usbotg1: usb@30b10000 {
>  				compatible = "fsl,imx7d-usb", "fsl,imx27-usb";
>  				reg = <0x30b10000 0x200>;
> --
> 2.18.0

RE: [PATCH v3 0/3] add mailbox support for i.MX7D

[A.s. Dong]

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> Sent: Monday, July 16, 2018 7:42 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 v3 0/3] add mailbox support for i.MX7D
> 
> 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_MXS instead of SOX_IMX7

It's ARCH_MXC, but no affection on real patches.

Regards
Dong Aisheng

> 
> 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.
> 
> 
> Oleksij Rempel (3):
>   dt-bindings: arm: fsl: add mu binding 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                 | 294 ++++++++++++++++++
>  5 files changed, 353 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>  create mode 100644 drivers/mailbox/imx-mailbox.c
> 
> --
> 2.18.0

Re: [PATCH v3 1/3] dt-bindings: arm: fsl: add mu binding doc

[Oleksij Rempel]

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



On 17.07.2018 07:00, A.s. Dong wrote:
>> -----Original Message-----
>> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
>> Sent: Monday, July 16, 2018 7:42 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 v3 1/3] dt-bindings: arm: fsl: add mu binding doc
>>
>> 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.
>>
>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>> ---
>>  .../devicetree/bindings/mailbox/fsl,mu.txt    | 32 +++++++++++++++++++
>>  1 file changed, 32 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..5d48dd75b98d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>> @@ -0,0 +1,32 @@
>> +NXP i.MX Messaging Unit (MU)
>> +--------------------------------------------------------------------
>> +
>> +Required properties:
>> +-------------------
>> +- compatible :	should be "fsl,<chip>-mu", the supported chips include:
>> +		imx6sx	- i.MX 6SoloX
>> +		imx7d	- i.MX 7Dual
>> +		imx7s	- i.MX 7Solo
>> +		imx7ulp	- i.MX 7ULP
>> +		imx8qm	- i.MX 8QM
>> +		imx8qxp	- 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 - 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-a : Should be set for side A MU.
> 
> For this property, how about doing like:
> fsl,mu-side: An Integer represents the MU side.

All this SoCs have MUs with only two sides. Why do we need explicit
annotation for both parts?

> If missing this property, it's default to Side A

So, why do we need optional integer, which is set by default as side A?
This is why I made it bool.

> 	       which is mostly used by A core.

And you will need to explicit set side=B for SCU. Correct?

> 	       0: MU Side A
> 	       1: MU Side B



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

[A.s. Dong]

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> Sent: Monday, July 16, 2018 7:42 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 v3 3/3] mailbox: Add support for i.MX7D messaging unit
> 
> The Mailbox controller is able to send messages (up to 4 32 bit words)
> between the endpoints.
> 
> This driver was tested using the mailbox-test driver sending messages
> between the Cortex-A7 and the Cortex-M4.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/mailbox/Kconfig       |   6 +
>  drivers/mailbox/Makefile      |   2 +
>  drivers/mailbox/imx-mailbox.c | 294
> ++++++++++++++++++++++++++++++++++
>  3 files changed, 302 insertions(+)
>  create mode 100644 drivers/mailbox/imx-mailbox.c
> 

Generally it looks good to me, a few minor comments:

> 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..31353366a007
> --- /dev/null
> +++ b/drivers/mailbox/imx-mailbox.c
> @@ -0,0 +1,294 @@
> +// 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;
> +};
> +
> +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_a;
> +};
> +
> +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;

Better re-order from long to short.

> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +	struct imx_mu_con_priv *cp = chan->con_priv;
> +

Unnecessary blank line?

> +	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;
> +	char *irq_desc;
> +	int ret;
> +
> +	irq_desc = devm_kasprintf(priv->dev, GFP_KERNEL,
> "imx_mu_chan[%i]",
> +				  cp->idx);

I like the name differentiation, just wondering whether this could
cause memory leak if users repeatly open/close MU channels due to
I see no free.

> +	if (!irq_desc)
> +		return -ENOMEM;
> +
> +	ret = devm_request_irq(priv->dev, cp->irq, imx_mu_isr,
> +			       IRQF_SHARED, 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); }
> +
> +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-a"))
> +		priv->side_a = true;

See property comments in former emails.

> +
> +	priv->mbox.dev = dev;
> +	priv->mbox.ops = &imx_mu_ops;
> +	priv->mbox.chans = priv->mbox_chans;
> +	priv->mbox.num_chans = chans;
> +	priv->mbox.txdone_irq = true;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	if (priv->dcfg->init_hw)
> +		priv->dcfg->init_hw(priv);
> +
> +	return mbox_controller_register(&priv->mbox);
> +}
> +
> +static int imx_mu_remove(struct platform_device *pdev) {
> +	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
> +
> +	mbox_controller_unregister(&priv->mbox);
> +	clk_disable_unprepare(priv->clk);
> +
> +	return 0;
> +}
> +
> +
> +static void imx_mu_init_imx7d(struct imx_mu_priv *priv) {

I guess we could remove the soc postfix now.

> +	/* Set default config */
> +	if (priv->side_a)
> +		imx_mu_write(priv, 0, IMX_MU_xCR);
> +}
> +
> +static const struct imx_mu_cfg imx_mu_cfg_imx7d = {
> +	.chans = IMX_MU_MAX_CHANS,

What's point of another chans here?
This can also be controlled by IMX_MU_MAX_CHANS.

> +	.init_hw = imx_mu_init_imx7d,

Can we imagine a diferent .init_hw callback for another SoC?
If no, how about make it default as I see the implementation
is quite simple and seems not SoC specific. Then we probably
can totally remove struct imx_mu_cfg.

> +};
> +
> +static const struct of_device_id imx_mu_dt_ids[] = {
> +	{ .compatible = "fsl,imx7s-mu", .data = &imx_mu_cfg_imx7d },
> +	{ },
> +};
> +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);

Do you think if we can escalated it a bit earlier as it's used by SCU?
e.g. core_initcall ?

Regards
Dong Aisheng

> +
> +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 v3 1/3] dt-bindings: arm: fsl: add mu binding doc

[A.s. Dong]

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> Sent: Tuesday, July 17, 2018 1:45 PM
> To: A.s. Dong <aisheng.dong@nxp.com>; Shawn Guo
> <shawnguo@kernel.org>; Fabio Estevam <fabio.estevam@nxp.com>; Rob
> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>
> Cc: devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v3 1/3] dt-bindings: arm: fsl: add mu binding doc
> 
> 
> 
> On 17.07.2018 07:00, A.s. Dong wrote:
> >> -----Original Message-----
> >> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> >> Sent: Monday, July 16, 2018 7:42 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 v3 1/3] dt-bindings: arm: fsl: add mu binding doc
> >>
> >> 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.
> >>
> >> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> >> ---
> >>  .../devicetree/bindings/mailbox/fsl,mu.txt    | 32
> +++++++++++++++++++
> >>  1 file changed, 32 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..5d48dd75b98d
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> >> @@ -0,0 +1,32 @@
> >> +NXP i.MX Messaging Unit (MU)
> >> +--------------------------------------------------------------------
> >> +
> >> +Required properties:
> >> +-------------------
> >> +- compatible :	should be "fsl,<chip>-mu", the supported chips
> include:
> >> +		imx6sx	- i.MX 6SoloX
> >> +		imx7d	- i.MX 7Dual
> >> +		imx7s	- i.MX 7Solo
> >> +		imx7ulp	- i.MX 7ULP
> >> +		imx8qm	- i.MX 8QM
> >> +		imx8qxp	- 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 - 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-a : Should be set for side A MU.
> >
> > For this property, how about doing like:
> > fsl,mu-side: An Integer represents the MU side.
> 
> All this SoCs have MUs with only two sides. Why do we need explicit
> annotation for both parts?
> 
> > If missing this property, it's default to Side A
> 
> So, why do we need optional integer, which is set by default as side A?
> This is why I made it bool.

Yes, A bool probably is better. 

> 
> > 	       which is mostly used by A core.
> 
> And you will need to explicit set side=B for SCU. Correct?

SCU is using side A. AFAIK all SoC A core is using side A by default.
That's why I think it can be default one. User is free to specify both
In device tree. Does it make sense to you?

Regards
Dong Aisheng

> 
> > 	       0: MU Side A
> > 	       1: MU Side B
> 

Re: [PATCH v3 1/3] dt-bindings: arm: fsl: add mu binding doc

[Oleksij Rempel]

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



On 17.07.2018 08:26, A.s. Dong wrote:
>> -----Original Message-----
>> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
>> Sent: Tuesday, July 17, 2018 1:45 PM
>> To: A.s. Dong <aisheng.dong@nxp.com>; Shawn Guo
>> <shawnguo@kernel.org>; Fabio Estevam <fabio.estevam@nxp.com>; Rob
>> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>
>> Cc: devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>> kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>
>> Subject: Re: [PATCH v3 1/3] dt-bindings: arm: fsl: add mu binding doc
>>
>>
>>
>> On 17.07.2018 07:00, A.s. Dong wrote:
>>>> -----Original Message-----
>>>> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
>>>> Sent: Monday, July 16, 2018 7:42 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 v3 1/3] dt-bindings: arm: fsl: add mu binding doc
>>>>
>>>> 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.
>>>>
>>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>>>> ---
>>>>  .../devicetree/bindings/mailbox/fsl,mu.txt    | 32
>> +++++++++++++++++++
>>>>  1 file changed, 32 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..5d48dd75b98d
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>>>> @@ -0,0 +1,32 @@
>>>> +NXP i.MX Messaging Unit (MU)
>>>> +--------------------------------------------------------------------
>>>> +
>>>> +Required properties:
>>>> +-------------------
>>>> +- compatible :	should be "fsl,<chip>-mu", the supported chips
>> include:
>>>> +		imx6sx	- i.MX 6SoloX
>>>> +		imx7d	- i.MX 7Dual
>>>> +		imx7s	- i.MX 7Solo
>>>> +		imx7ulp	- i.MX 7ULP
>>>> +		imx8qm	- i.MX 8QM
>>>> +		imx8qxp	- 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 - 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-a : Should be set for side A MU.
>>>
>>> For this property, how about doing like:
>>> fsl,mu-side: An Integer represents the MU side.
>>
>> All this SoCs have MUs with only two sides. Why do we need explicit
>> annotation for both parts?
>>
>>> If missing this property, it's default to Side A
>>
>> So, why do we need optional integer, which is set by default as side A?
>> This is why I made it bool.
> 
> Yes, A bool probably is better. 
> 
>>
>>> 	       which is mostly used by A core.
>>
>> And you will need to explicit set side=B for SCU. Correct?
> 
> SCU is using side A. AFAIK all SoC A core is using side A by default.
> That's why I think it can be default one. User is free to specify both
> In device tree. Does it make sense to you?

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

[Oleksij Rempel]

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



On 17.07.2018 08:21, A.s. Dong wrote:
>> -----Original Message-----
>> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
>> Sent: Monday, July 16, 2018 7:42 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 v3 3/3] mailbox: Add support for i.MX7D messaging unit
>>
>> The Mailbox controller is able to send messages (up to 4 32 bit words)
>> between the endpoints.
>>
>> This driver was tested using the mailbox-test driver sending messages
>> between the Cortex-A7 and the Cortex-M4.
>>
>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>> ---
>>  drivers/mailbox/Kconfig       |   6 +
>>  drivers/mailbox/Makefile      |   2 +
>>  drivers/mailbox/imx-mailbox.c | 294
>> ++++++++++++++++++++++++++++++++++
>>  3 files changed, 302 insertions(+)
>>  create mode 100644 drivers/mailbox/imx-mailbox.c
>>
> 
> Generally it looks good to me, a few minor comments:
> 
>> 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..31353366a007
>> --- /dev/null
>> +++ b/drivers/mailbox/imx-mailbox.c
>> @@ -0,0 +1,294 @@
>> +// 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;
>> +};
>> +
>> +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_a;
>> +};
>> +
>> +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;
> 
> Better re-order from long to short.

"chis" is used by by following declarations. It make no sense to reorder it.

> 
>> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>> +	struct imx_mu_con_priv *cp = chan->con_priv;
>> +
> 
> Unnecessary blank line?

ok,

>> +	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;
>> +	char *irq_desc;
>> +	int ret;
>> +
>> +	irq_desc = devm_kasprintf(priv->dev, GFP_KERNEL,
>> "imx_mu_chan[%i]",
>> +				  cp->idx);
> 
> I like the name differentiation, just wondering whether this could
> cause memory leak if users repeatly open/close MU channels due to
> I see no free.

good point.

> 
>> +	if (!irq_desc)
>> +		return -ENOMEM;
>> +
>> +	ret = devm_request_irq(priv->dev, cp->irq, imx_mu_isr,
>> +			       IRQF_SHARED, 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); }
>> +
>> +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-a"))
>> +		priv->side_a = true;
> 
> See property comments in former emails.

ok.

>> +
>> +	priv->mbox.dev = dev;
>> +	priv->mbox.ops = &imx_mu_ops;
>> +	priv->mbox.chans = priv->mbox_chans;
>> +	priv->mbox.num_chans = chans;
>> +	priv->mbox.txdone_irq = true;
>> +
>> +	platform_set_drvdata(pdev, priv);
>> +
>> +	if (priv->dcfg->init_hw)
>> +		priv->dcfg->init_hw(priv);
>> +
>> +	return mbox_controller_register(&priv->mbox);
>> +}
>> +
>> +static int imx_mu_remove(struct platform_device *pdev) {
>> +	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
>> +
>> +	mbox_controller_unregister(&priv->mbox);
>> +	clk_disable_unprepare(priv->clk);
>> +
>> +	return 0;
>> +}
>> +
>> +
>> +static void imx_mu_init_imx7d(struct imx_mu_priv *priv) {
> 
> I guess we could remove the soc postfix now.

ok.

> 
>> +	/* Set default config */
>> +	if (priv->side_a)
>> +		imx_mu_write(priv, 0, IMX_MU_xCR);
>> +}
>> +
>> +static const struct imx_mu_cfg imx_mu_cfg_imx7d = {
>> +	.chans = IMX_MU_MAX_CHANS,
> 
> What's point of another chans here?
> This can also be controlled by IMX_MU_MAX_CHANS.

SCU has one channel configuration.
It is possible to exctend this driver with existing MU to provide 8
channels: 4 channels with FIFO (can be used stand alone) + 4 channes
based on General purpose interrupt and should be used with shared memory.

>> +	.init_hw = imx_mu_init_imx7d,
> 
> Can we imagine a diferent .init_hw callback for another SoC?

yes, for example SCU case, or any other case where A side is used on
slave system.

> If no, how about make it default as I see the implementation
> is quite simple and seems not SoC specific. Then we probably
> can totally remove struct imx_mu_cfg.

i prefer not to do it.

>> +};
>> +
>> +static const struct of_device_id imx_mu_dt_ids[] = {
>> +	{ .compatible = "fsl,imx7s-mu", .data = &imx_mu_cfg_imx7d },
>> +	{ },
>> +};
>> +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);
> 
> Do you think if we can escalated it a bit earlier as it's used by SCU?
> e.g. core_initcall ?

Some more testing shoukld be done. Same MU driver is on master side a
clock consumer on slave side it is clock provider. It is valid for my
R&D project as well.
Let us concentrate on generic part first and then extend it as needed.


[-- 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 v3 1/3] dt-bindings: arm: fsl: add mu binding doc

[A.s. Dong]

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> Sent: Tuesday, July 17, 2018 2:32 PM
> To: A.s. Dong <aisheng.dong@nxp.com>; Shawn Guo
> <shawnguo@kernel.org>; Fabio Estevam <fabio.estevam@nxp.com>; Rob
> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>
> Cc: devicetree@vger.kernel.org; kernel@pengutronix.de; linux-arm-
> kernel@lists.infradead.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v3 1/3] dt-bindings: arm: fsl: add mu binding doc
> 
> 
> 
> On 17.07.2018 08:26, A.s. Dong wrote:
> >> -----Original Message-----
> >> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> >> Sent: Tuesday, July 17, 2018 1:45 PM
> >> To: A.s. Dong <aisheng.dong@nxp.com>; Shawn Guo
> >> <shawnguo@kernel.org>; Fabio Estevam <fabio.estevam@nxp.com>;
> Rob
> >> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>
> >> Cc: devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> >> kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>
> >> Subject: Re: [PATCH v3 1/3] dt-bindings: arm: fsl: add mu binding doc
> >>
> >>
> >>
> >> On 17.07.2018 07:00, A.s. Dong wrote:
> >>>> -----Original Message-----
> >>>> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> >>>> Sent: Monday, July 16, 2018 7:42 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 v3 1/3] dt-bindings: arm: fsl: add mu binding doc
> >>>>
> >>>> 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.
> >>>>
> >>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> >>>> ---
> >>>>  .../devicetree/bindings/mailbox/fsl,mu.txt    | 32
> >> +++++++++++++++++++
> >>>>  1 file changed, 32 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..5d48dd75b98d
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> >>>> @@ -0,0 +1,32 @@
> >>>> +NXP i.MX Messaging Unit (MU)
> >>>> +------------------------------------------------------------------
> >>>> +--
> >>>> +
> >>>> +Required properties:
> >>>> +-------------------
> >>>> +- compatible :	should be "fsl,<chip>-mu", the supported chips
> >> include:
> >>>> +		imx6sx	- i.MX 6SoloX
> >>>> +		imx7d	- i.MX 7Dual
> >>>> +		imx7s	- i.MX 7Solo
> >>>> +		imx7ulp	- i.MX 7ULP
> >>>> +		imx8qm	- i.MX 8QM
> >>>> +		imx8qxp	- 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 - 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-a : Should be set for side A MU.
> >>>
> >>> For this property, how about doing like:
> >>> fsl,mu-side: An Integer represents the MU side.
> >>
> >> All this SoCs have MUs with only two sides. Why do we need explicit
> >> annotation for both parts?
> >>
> >>> If missing this property, it's default to Side A
> >>
> >> So, why do we need optional integer, which is set by default as side A?
> >> This is why I made it bool.
> >
> > Yes, A bool probably is better.
> >
> >>
> >>> 	       which is mostly used by A core.
> >>
> >> And you will need to explicit set side=B for SCU. Correct?
> >
> > SCU is using side A. AFAIK all SoC A core is using side A by default.
> > That's why I think it can be default one. User is free to specify both
> > In device tree. Does it make sense to you?
> 
> Ok.

My original assumption is we can specify the side explicitly in device tree
 (default to side A if missing) like:
mu0a: mailbox@30aa0000 {
        compatible = "fsl,imx7s-mu";
        ...
        fsl,mu-side = <0>;
};

mu0b: mailbox@30ab0000 {
        compatible = "fsl,imx7s-mu";
        ...
        fsl,mu-side = <1>;
};

But you're right it can be simply indicated by a bool as well.
mu0a: mailbox@30aa0000 {
        compatible = "fsl,imx7s-mu";
        ...
};

mu0b: mailbox@30ab0000 {
        compatible = "fsl,imx7s-mu";
        ...
        fsl,mu-side-b; 
};

I'm okay with both. You can prefer as you wish.

Regards
Dong Aisheng

RE: [PATCH v3 3/3] mailbox: Add support for i.MX7D messaging unit

[A.s. Dong]

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> Sent: Tuesday, July 17, 2018 2:43 PM
> To: A.s. Dong <aisheng.dong@nxp.com>; Shawn Guo
> <shawnguo@kernel.org>; Fabio Estevam <fabio.estevam@nxp.com>; Rob
> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>
> Cc: devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v3 3/3] mailbox: Add support for i.MX7D messaging unit
> 
> 
> 
> On 17.07.2018 08:21, A.s. Dong wrote:
> >> -----Original Message-----
> >> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> >> Sent: Monday, July 16, 2018 7:42 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 v3 3/3] mailbox: Add support for i.MX7D messaging
> >> unit
> >>
> >> The Mailbox controller is able to send messages (up to 4 32 bit
> >> words) between the endpoints.
> >>
> >> This driver was tested using the mailbox-test driver sending messages
> >> between the Cortex-A7 and the Cortex-M4.
> >>
> >> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> >> ---
> >>  drivers/mailbox/Kconfig       |   6 +
> >>  drivers/mailbox/Makefile      |   2 +
> >>  drivers/mailbox/imx-mailbox.c | 294
> >> ++++++++++++++++++++++++++++++++++
> >>  3 files changed, 302 insertions(+)
> >>  create mode 100644 drivers/mailbox/imx-mailbox.c
> >>
> >
> > Generally it looks good to me, a few minor comments:
> >
> >> 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..31353366a007
> >> --- /dev/null
> >> +++ b/drivers/mailbox/imx-mailbox.c
> >> @@ -0,0 +1,294 @@
> >> +// 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;
> >> +};
> >> +
> >> +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_a;
> >> +};
> >> +
> >> +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;
> >
> > Better re-order from long to short.
> 
> "chis" is used by by following declarations. It make no sense to reorder it.
> 

Yes, you're right.

> >
> >> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> >> +	struct imx_mu_con_priv *cp = chan->con_priv;
> >> +
> >
> > Unnecessary blank line?
> 
> ok,
> 
> >> +	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;
> >> +	char *irq_desc;
> >> +	int ret;
> >> +
> >> +	irq_desc = devm_kasprintf(priv->dev, GFP_KERNEL,
> >> "imx_mu_chan[%i]",
> >> +				  cp->idx);
> >
> > I like the name differentiation, just wondering whether this could
> > cause memory leak if users repeatly open/close MU channels due to I
> > see no free.
> 
> good point.
> 
> >
> >> +	if (!irq_desc)
> >> +		return -ENOMEM;
> >> +
> >> +	ret = devm_request_irq(priv->dev, cp->irq, imx_mu_isr,
> >> +			       IRQF_SHARED, 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); }
> >> +
> >> +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-a"))
> >> +		priv->side_a = true;
> >
> > See property comments in former emails.
> 
> ok.
> 
> >> +
> >> +	priv->mbox.dev = dev;
> >> +	priv->mbox.ops = &imx_mu_ops;
> >> +	priv->mbox.chans = priv->mbox_chans;
> >> +	priv->mbox.num_chans = chans;
> >> +	priv->mbox.txdone_irq = true;
> >> +
> >> +	platform_set_drvdata(pdev, priv);
> >> +
> >> +	if (priv->dcfg->init_hw)
> >> +		priv->dcfg->init_hw(priv);
> >> +
> >> +	return mbox_controller_register(&priv->mbox);
> >> +}
> >> +
> >> +static int imx_mu_remove(struct platform_device *pdev) {
> >> +	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
> >> +
> >> +	mbox_controller_unregister(&priv->mbox);
> >> +	clk_disable_unprepare(priv->clk);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +
> >> +static void imx_mu_init_imx7d(struct imx_mu_priv *priv) {
> >
> > I guess we could remove the soc postfix now.
> 
> ok.
> 
> >
> >> +	/* Set default config */
> >> +	if (priv->side_a)
> >> +		imx_mu_write(priv, 0, IMX_MU_xCR); }
> >> +
> >> +static const struct imx_mu_cfg imx_mu_cfg_imx7d = {
> >> +	.chans = IMX_MU_MAX_CHANS,
> >
> > What's point of another chans here?
> > This can also be controlled by IMX_MU_MAX_CHANS.
> 
> SCU has one channel configuration.

I guess the SCU one channel(4 rx/tx register) actually is not the one channel
specified here. e.g. For M4 case, users can still specify only using one chan
(1 rx/tx register). They're slightly different.
So for SCU case, we actually did not use the struct imx_mu_cfg.

> It is possible to exctend this driver with existing MU to provide 8
> channels: 4 channels with FIFO (can be used stand alone) + 4 channes based
> on General purpose interrupt and should be used with shared memory.

Yes, but that still seems common to me. Not per SoC specific.

> 
> >> +	.init_hw = imx_mu_init_imx7d,
> >
> > Can we imagine a diferent .init_hw callback for another SoC?
> 
> yes, for example SCU case, or any other case where A side is used on slave
> system.
> 

SCU does not use this as we still did not see SoC specific init_hw requirement.

> > If no, how about make it default as I see the implementation is quite
> > simple and seems not SoC specific. Then we probably can totally remove
> > struct imx_mu_cfg.
> 
> i prefer not to do it.
> 
> >> +};
> >> +
> >> +static const struct of_device_id imx_mu_dt_ids[] = {
> >> +	{ .compatible = "fsl,imx7s-mu", .data = &imx_mu_cfg_imx7d },
> >> +	{ },
> >> +};
> >> +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);
> >
> > Do you think if we can escalated it a bit earlier as it's used by SCU?
> > e.g. core_initcall ?
> 
> Some more testing shoukld be done. Same MU driver is on master side a
> clock consumer on slave side it is clock provider. It is valid for my R&D project
> as well.
> Let us concentrate on generic part first and then extend it as needed.

Okay to me.

Regards
Dong Aisheng

Re: [PATCH v3 3/3] mailbox: Add support for i.MX7D messaging unit

[Oleksij Rempel]

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



On 17.07.2018 09:07, A.s. Dong wrote:
>> -----Original Message-----
>> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
>> Sent: Tuesday, July 17, 2018 2:43 PM
>> To: A.s. Dong <aisheng.dong@nxp.com>; Shawn Guo
>> <shawnguo@kernel.org>; Fabio Estevam <fabio.estevam@nxp.com>; Rob
>> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>
>> Cc: devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>> kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>
>> Subject: Re: [PATCH v3 3/3] mailbox: Add support for i.MX7D messaging unit
>>
>>
>>
>> On 17.07.2018 08:21, A.s. Dong wrote:
>>>> -----Original Message-----
>>>> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
>>>> Sent: Monday, July 16, 2018 7:42 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 v3 3/3] mailbox: Add support for i.MX7D messaging
>>>> unit
>>>>
>>>> The Mailbox controller is able to send messages (up to 4 32 bit
>>>> words) between the endpoints.
>>>>
>>>> This driver was tested using the mailbox-test driver sending messages
>>>> between the Cortex-A7 and the Cortex-M4.
>>>>
>>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>>>> ---
>>>>  drivers/mailbox/Kconfig       |   6 +
>>>>  drivers/mailbox/Makefile      |   2 +
>>>>  drivers/mailbox/imx-mailbox.c | 294
>>>> ++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 302 insertions(+)
>>>>  create mode 100644 drivers/mailbox/imx-mailbox.c
>>>>
>>>
>>> Generally it looks good to me, a few minor comments:
>>>
>>>> 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..31353366a007
>>>> --- /dev/null
>>>> +++ b/drivers/mailbox/imx-mailbox.c
>>>> @@ -0,0 +1,294 @@
>>>> +// 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;
>>>> +};
>>>> +
>>>> +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_a;
>>>> +};
>>>> +
>>>> +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;
>>>
>>> Better re-order from long to short.
>>
>> "chis" is used by by following declarations. It make no sense to reorder it.
>>
> 
> Yes, you're right.
> 
>>>
>>>> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>>>> +	struct imx_mu_con_priv *cp = chan->con_priv;
>>>> +
>>>
>>> Unnecessary blank line?
>>
>> ok,
>>
>>>> +	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;
>>>> +	char *irq_desc;
>>>> +	int ret;
>>>> +
>>>> +	irq_desc = devm_kasprintf(priv->dev, GFP_KERNEL,
>>>> "imx_mu_chan[%i]",
>>>> +				  cp->idx);
>>>
>>> I like the name differentiation, just wondering whether this could
>>> cause memory leak if users repeatly open/close MU channels due to I
>>> see no free.
>>
>> good point.
>>
>>>
>>>> +	if (!irq_desc)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	ret = devm_request_irq(priv->dev, cp->irq, imx_mu_isr,
>>>> +			       IRQF_SHARED, 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); }
>>>> +
>>>> +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-a"))
>>>> +		priv->side_a = true;
>>>
>>> See property comments in former emails.
>>
>> ok.
>>
>>>> +
>>>> +	priv->mbox.dev = dev;
>>>> +	priv->mbox.ops = &imx_mu_ops;
>>>> +	priv->mbox.chans = priv->mbox_chans;
>>>> +	priv->mbox.num_chans = chans;
>>>> +	priv->mbox.txdone_irq = true;
>>>> +
>>>> +	platform_set_drvdata(pdev, priv);
>>>> +
>>>> +	if (priv->dcfg->init_hw)
>>>> +		priv->dcfg->init_hw(priv);
>>>> +
>>>> +	return mbox_controller_register(&priv->mbox);
>>>> +}
>>>> +
>>>> +static int imx_mu_remove(struct platform_device *pdev) {
>>>> +	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
>>>> +
>>>> +	mbox_controller_unregister(&priv->mbox);
>>>> +	clk_disable_unprepare(priv->clk);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +
>>>> +static void imx_mu_init_imx7d(struct imx_mu_priv *priv) {
>>>
>>> I guess we could remove the soc postfix now.
>>
>> ok.
>>
>>>
>>>> +	/* Set default config */
>>>> +	if (priv->side_a)
>>>> +		imx_mu_write(priv, 0, IMX_MU_xCR); }
>>>> +
>>>> +static const struct imx_mu_cfg imx_mu_cfg_imx7d = {
>>>> +	.chans = IMX_MU_MAX_CHANS,
>>>
>>> What's point of another chans here?
>>> This can also be controlled by IMX_MU_MAX_CHANS.
>>
>> SCU has one channel configuration.
> 
> I guess the SCU one channel(4 rx/tx register) actually is not the one channel
> specified here. e.g. For M4 case, users can still specify only using one chan
> (1 rx/tx register). They're slightly different.
> So for SCU case, we actually did not use the struct imx_mu_cfg.
> 
>> It is possible to exctend this driver with existing MU to provide 8
>> channels: 4 channels with FIFO (can be used stand alone) + 4 channes based
>> on General purpose interrupt and should be used with shared memory.
> 
> Yes, but that still seems common to me. Not per SoC specific.

Correct, it is not SoC specific, it is end-product specific. All depends
on what exactly is on opposite side of MU. Currently I don't see any
other way as providing product specific compatible with needed
configuration. And current driver version allows it without ugly patching.

>>
>>>> +	.init_hw = imx_mu_init_imx7d,
>>>
>>> Can we imagine a diferent .init_hw callback for another SoC?
>>
>> yes, for example SCU case, or any other case where A side is used on slave
>> system.
>>
> 
> SCU does not use this as we still did not see SoC specific init_hw requirement.
> 
>>> If no, how about make it default as I see the implementation is quite
>>> simple and seems not SoC specific. Then we probably can totally remove
>>> struct imx_mu_cfg.
>>
>> i prefer not to do it.
>>
>>>> +};
>>>> +
>>>> +static const struct of_device_id imx_mu_dt_ids[] = {
>>>> +	{ .compatible = "fsl,imx7s-mu", .data = &imx_mu_cfg_imx7d },
>>>> +	{ },
>>>> +};
>>>> +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);
>>>
>>> Do you think if we can escalated it a bit earlier as it's used by SCU?
>>> e.g. core_initcall ?
>>
>> Some more testing shoukld be done. Same MU driver is on master side a
>> clock consumer on slave side it is clock provider. It is valid for my R&D project
>> as well.
>> Let us concentrate on generic part first and then extend it as needed.
> 
> Okay to me.
> 
> Regards
> Dong Aisheng
> 


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

[A.s. Dong]

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> Sent: Tuesday, July 17, 2018 3:14 PM
> To: A.s. Dong <aisheng.dong@nxp.com>; Shawn Guo
> <shawnguo@kernel.org>; Fabio Estevam <fabio.estevam@nxp.com>; Rob
> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>
> Cc: devicetree@vger.kernel.org; kernel@pengutronix.de; linux-arm-
> kernel@lists.infradead.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v3 3/3] mailbox: Add support for i.MX7D messaging unit
> 
> 
> 
> On 17.07.2018 09:07, A.s. Dong wrote:
> >> -----Original Message-----
> >> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> >> Sent: Tuesday, July 17, 2018 2:43 PM
> >> To: A.s. Dong <aisheng.dong@nxp.com>; Shawn Guo
> >> <shawnguo@kernel.org>; Fabio Estevam <fabio.estevam@nxp.com>;
> Rob
> >> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>
> >> Cc: devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> >> kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>
> >> Subject: Re: [PATCH v3 3/3] mailbox: Add support for i.MX7D messaging
> >> unit
> >>
> >>
> >>
> >> On 17.07.2018 08:21, A.s. Dong wrote:
> >>>> -----Original Message-----
> >>>> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> >>>> Sent: Monday, July 16, 2018 7:42 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 v3 3/3] mailbox: Add support for i.MX7D messaging
> >>>> unit
> >>>>
> >>>> The Mailbox controller is able to send messages (up to 4 32 bit
> >>>> words) between the endpoints.
> >>>>
> >>>> This driver was tested using the mailbox-test driver sending
> >>>> messages between the Cortex-A7 and the Cortex-M4.
> >>>>
> >>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> >>>> ---
> >>>>  drivers/mailbox/Kconfig       |   6 +
> >>>>  drivers/mailbox/Makefile      |   2 +
> >>>>  drivers/mailbox/imx-mailbox.c | 294
> >>>> ++++++++++++++++++++++++++++++++++
> >>>>  3 files changed, 302 insertions(+)  create mode 100644
> >>>> drivers/mailbox/imx-mailbox.c
> >>>>
> >>>
> >>> Generally it looks good to me, a few minor comments:
> >>>
> >>>> 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..31353366a007
> >>>> --- /dev/null
> >>>> +++ b/drivers/mailbox/imx-mailbox.c
> >>>> @@ -0,0 +1,294 @@
> >>>> +// 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;
> >>>> +};
> >>>> +
> >>>> +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_a;
> >>>> +};
> >>>> +
> >>>> +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;
> >>>
> >>> Better re-order from long to short.
> >>
> >> "chis" is used by by following declarations. It make no sense to reorder it.
> >>
> >
> > Yes, you're right.
> >
> >>>
> >>>> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> >>>> +	struct imx_mu_con_priv *cp = chan->con_priv;
> >>>> +
> >>>
> >>> Unnecessary blank line?
> >>
> >> ok,
> >>
> >>>> +	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;
> >>>> +	char *irq_desc;
> >>>> +	int ret;
> >>>> +
> >>>> +	irq_desc = devm_kasprintf(priv->dev, GFP_KERNEL,
> >>>> "imx_mu_chan[%i]",
> >>>> +				  cp->idx);
> >>>
> >>> I like the name differentiation, just wondering whether this could
> >>> cause memory leak if users repeatly open/close MU channels due to I
> >>> see no free.
> >>
> >> good point.
> >>
> >>>
> >>>> +	if (!irq_desc)
> >>>> +		return -ENOMEM;
> >>>> +
> >>>> +	ret = devm_request_irq(priv->dev, cp->irq, imx_mu_isr,
> >>>> +			       IRQF_SHARED, 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); }
> >>>> +
> >>>> +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-a"))
> >>>> +		priv->side_a = true;
> >>>
> >>> See property comments in former emails.
> >>
> >> ok.
> >>
> >>>> +
> >>>> +	priv->mbox.dev = dev;
> >>>> +	priv->mbox.ops = &imx_mu_ops;
> >>>> +	priv->mbox.chans = priv->mbox_chans;
> >>>> +	priv->mbox.num_chans = chans;
> >>>> +	priv->mbox.txdone_irq = true;
> >>>> +
> >>>> +	platform_set_drvdata(pdev, priv);
> >>>> +
> >>>> +	if (priv->dcfg->init_hw)
> >>>> +		priv->dcfg->init_hw(priv);
> >>>> +
> >>>> +	return mbox_controller_register(&priv->mbox);
> >>>> +}
> >>>> +
> >>>> +static int imx_mu_remove(struct platform_device *pdev) {
> >>>> +	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
> >>>> +
> >>>> +	mbox_controller_unregister(&priv->mbox);
> >>>> +	clk_disable_unprepare(priv->clk);
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>> +
> >>>> +static void imx_mu_init_imx7d(struct imx_mu_priv *priv) {
> >>>
> >>> I guess we could remove the soc postfix now.
> >>
> >> ok.
> >>
> >>>
> >>>> +	/* Set default config */
> >>>> +	if (priv->side_a)
> >>>> +		imx_mu_write(priv, 0, IMX_MU_xCR); }
> >>>> +
> >>>> +static const struct imx_mu_cfg imx_mu_cfg_imx7d = {
> >>>> +	.chans = IMX_MU_MAX_CHANS,
> >>>
> >>> What's point of another chans here?
> >>> This can also be controlled by IMX_MU_MAX_CHANS.
> >>
> >> SCU has one channel configuration.
> >
> > I guess the SCU one channel(4 rx/tx register) actually is not the one
> > channel specified here. e.g. For M4 case, users can still specify only
> > using one chan
> > (1 rx/tx register). They're slightly different.
> > So for SCU case, we actually did not use the struct imx_mu_cfg.
> >
> >> It is possible to exctend this driver with existing MU to provide 8
> >> channels: 4 channels with FIFO (can be used stand alone) + 4 channes
> >> based on General purpose interrupt and should be used with shared
> memory.
> >
> > Yes, but that still seems common to me. Not per SoC specific.
> 
> Correct, it is not SoC specific, it is end-product specific. All depends on what
> exactly is on opposite side of MU. Currently I don't see any other way as
> providing product specific compatible with needed configuration. And
> current driver version allows it without ugly patching.
> 

I think the point is your current implementation of .hw_init() for MX7D
is quite general to other SoCs as well, not much end-product specific.

Let's see it's only a simple initialization of MU and reset both side.
+static void imx_mu_init_imx7d(struct imx_mu_priv *priv) {
+	/* Set default config */
+	if (priv->side_a)
+		imx_mu_write(priv, 0, IMX_MU_xCR);
+}
That's why I thought it could be moved into general part instead of being
provided as SoC specific .hw_init() callback.

Because I'm afraid we may write similar code for other SoCs as well:
e.g.
static const struct imx_mu_cfg imx_mu_cfg_imx6sx = {
        .chans = IMX_MU_MAX_CHANS,
        .init_hw = imx_mu_init_imx7d,
};

static const struct imx_mu_cfg imx_mu_cfg_imx7d = {
        .chans = IMX_MU_MAX_CHANS,
        .init_hw = imx_mu_init_imx7d,
};

static const struct imx_mu_cfg imx_mu_cfg_imx7ulp = {
        .chans = IMX_MU_MAX_CHANS,
        .init_hw = imx_mu_init_imx7d,
};

static const struct imx_mu_cfg imx_mu_cfg_imx8qxp = {
        .chans = IMX_MU_MAX_CHANS,
        .init_hw = imx_mu_init_imx7d,
};

static const struct imx_mu_cfg imx_mu_cfg_imx8qm = {
        .chans = IMX_MU_MAX_CHANS,
        .init_hw = imx_mu_init_imx7d,
};

static const struct imx_mu_cfg imx_mu_cfg_imx8mq = {
        .chans = IMX_MU_MAX_CHANS,
        .init_hw = imx_mu_init_imx7d,
};

Anyway, I'm not strongly against it. Maybe we can also refine
it later when we see the real problem.

Regards
Dong Aisheng

> >>
> >>>> +	.init_hw = imx_mu_init_imx7d,
> >>>
> >>> Can we imagine a diferent .init_hw callback for another SoC?
> >>
> >> yes, for example SCU case, or any other case where A side is used on
> >> slave system.
> >>
> >
> > SCU does not use this as we still did not see SoC specific init_hw
> requirement.
> >
> >>> If no, how about make it default as I see the implementation is
> >>> quite simple and seems not SoC specific. Then we probably can
> >>> totally remove struct imx_mu_cfg.
> >>
> >> i prefer not to do it.
> >>
> >>>> +};
> >>>> +
> >>>> +static const struct of_device_id imx_mu_dt_ids[] = {
> >>>> +	{ .compatible = "fsl,imx7s-mu", .data = &imx_mu_cfg_imx7d },
> >>>> +	{ },
> >>>> +};
> >>>> +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);
> >>>
> >>> Do you think if we can escalated it a bit earlier as it's used by SCU?
> >>> e.g. core_initcall ?
> >>
> >> Some more testing shoukld be done. Same MU driver is on master side a
> >> clock consumer on slave side it is clock provider. It is valid for my
> >> R&D project as well.
> >> Let us concentrate on generic part first and then extend it as needed.
> >
> > Okay to me.
> >
> > Regards
> > Dong Aisheng
> >

Re: [PATCH v3 3/3] mailbox: Add support for i.MX7D messaging unit

[Oleksij Rempel]

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



On 17.07.2018 09:26, A.s. Dong wrote:
>> -----Original Message-----
>> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
>> Sent: Tuesday, July 17, 2018 3:14 PM
>> To: A.s. Dong <aisheng.dong@nxp.com>; Shawn Guo
>> <shawnguo@kernel.org>; Fabio Estevam <fabio.estevam@nxp.com>; Rob
>> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>
>> Cc: devicetree@vger.kernel.org; kernel@pengutronix.de; linux-arm-
>> kernel@lists.infradead.org; dl-linux-imx <linux-imx@nxp.com>
>> Subject: Re: [PATCH v3 3/3] mailbox: Add support for i.MX7D messaging unit
>>
>>
>>
>> On 17.07.2018 09:07, A.s. Dong wrote:
>>>> -----Original Message-----
>>>> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
>>>> Sent: Tuesday, July 17, 2018 2:43 PM
>>>> To: A.s. Dong <aisheng.dong@nxp.com>; Shawn Guo
>>>> <shawnguo@kernel.org>; Fabio Estevam <fabio.estevam@nxp.com>;
>> Rob
>>>> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>
>>>> Cc: devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>>>> kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>
>>>> Subject: Re: [PATCH v3 3/3] mailbox: Add support for i.MX7D messaging
>>>> unit
>>>>
>>>>
>>>>
>>>> On 17.07.2018 08:21, A.s. Dong wrote:
>>>>>> -----Original Message-----
>>>>>> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
>>>>>> Sent: Monday, July 16, 2018 7:42 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 v3 3/3] mailbox: Add support for i.MX7D messaging
>>>>>> unit
>>>>>>
>>>>>> The Mailbox controller is able to send messages (up to 4 32 bit
>>>>>> words) between the endpoints.
>>>>>>
>>>>>> This driver was tested using the mailbox-test driver sending
>>>>>> messages between the Cortex-A7 and the Cortex-M4.
>>>>>>
>>>>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>>>>>> ---
>>>>>>  drivers/mailbox/Kconfig       |   6 +
>>>>>>  drivers/mailbox/Makefile      |   2 +
>>>>>>  drivers/mailbox/imx-mailbox.c | 294
>>>>>> ++++++++++++++++++++++++++++++++++
>>>>>>  3 files changed, 302 insertions(+)  create mode 100644
>>>>>> drivers/mailbox/imx-mailbox.c
>>>>>>
>>>>>
>>>>> Generally it looks good to me, a few minor comments:
>>>>>
>>>>>> 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..31353366a007
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/mailbox/imx-mailbox.c
>>>>>> @@ -0,0 +1,294 @@
>>>>>> +// 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;
>>>>>> +};
>>>>>> +
>>>>>> +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_a;
>>>>>> +};
>>>>>> +
>>>>>> +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;
>>>>>
>>>>> Better re-order from long to short.
>>>>
>>>> "chis" is used by by following declarations. It make no sense to reorder it.
>>>>
>>>
>>> Yes, you're right.
>>>
>>>>>
>>>>>> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>>>>>> +	struct imx_mu_con_priv *cp = chan->con_priv;
>>>>>> +
>>>>>
>>>>> Unnecessary blank line?
>>>>
>>>> ok,
>>>>
>>>>>> +	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;
>>>>>> +	char *irq_desc;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	irq_desc = devm_kasprintf(priv->dev, GFP_KERNEL,
>>>>>> "imx_mu_chan[%i]",
>>>>>> +				  cp->idx);
>>>>>
>>>>> I like the name differentiation, just wondering whether this could
>>>>> cause memory leak if users repeatly open/close MU channels due to I
>>>>> see no free.
>>>>
>>>> good point.
>>>>
>>>>>
>>>>>> +	if (!irq_desc)
>>>>>> +		return -ENOMEM;
>>>>>> +
>>>>>> +	ret = devm_request_irq(priv->dev, cp->irq, imx_mu_isr,
>>>>>> +			       IRQF_SHARED, 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); }
>>>>>> +
>>>>>> +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-a"))
>>>>>> +		priv->side_a = true;
>>>>>
>>>>> See property comments in former emails.
>>>>
>>>> ok.
>>>>
>>>>>> +
>>>>>> +	priv->mbox.dev = dev;
>>>>>> +	priv->mbox.ops = &imx_mu_ops;
>>>>>> +	priv->mbox.chans = priv->mbox_chans;
>>>>>> +	priv->mbox.num_chans = chans;
>>>>>> +	priv->mbox.txdone_irq = true;
>>>>>> +
>>>>>> +	platform_set_drvdata(pdev, priv);
>>>>>> +
>>>>>> +	if (priv->dcfg->init_hw)
>>>>>> +		priv->dcfg->init_hw(priv);
>>>>>> +
>>>>>> +	return mbox_controller_register(&priv->mbox);
>>>>>> +}
>>>>>> +
>>>>>> +static int imx_mu_remove(struct platform_device *pdev) {
>>>>>> +	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
>>>>>> +
>>>>>> +	mbox_controller_unregister(&priv->mbox);
>>>>>> +	clk_disable_unprepare(priv->clk);
>>>>>> +
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +
>>>>>> +
>>>>>> +static void imx_mu_init_imx7d(struct imx_mu_priv *priv) {
>>>>>
>>>>> I guess we could remove the soc postfix now.
>>>>
>>>> ok.
>>>>
>>>>>
>>>>>> +	/* Set default config */
>>>>>> +	if (priv->side_a)
>>>>>> +		imx_mu_write(priv, 0, IMX_MU_xCR); }
>>>>>> +
>>>>>> +static const struct imx_mu_cfg imx_mu_cfg_imx7d = {
>>>>>> +	.chans = IMX_MU_MAX_CHANS,
>>>>>
>>>>> What's point of another chans here?
>>>>> This can also be controlled by IMX_MU_MAX_CHANS.
>>>>
>>>> SCU has one channel configuration.
>>>
>>> I guess the SCU one channel(4 rx/tx register) actually is not the one
>>> channel specified here. e.g. For M4 case, users can still specify only
>>> using one chan
>>> (1 rx/tx register). They're slightly different.
>>> So for SCU case, we actually did not use the struct imx_mu_cfg.
>>>
>>>> It is possible to exctend this driver with existing MU to provide 8
>>>> channels: 4 channels with FIFO (can be used stand alone) + 4 channes
>>>> based on General purpose interrupt and should be used with shared
>> memory.
>>>
>>> Yes, but that still seems common to me. Not per SoC specific.
>>
>> Correct, it is not SoC specific, it is end-product specific. All depends on what
>> exactly is on opposite side of MU. Currently I don't see any other way as
>> providing product specific compatible with needed configuration. And
>> current driver version allows it without ugly patching.
>>
> 
> I think the point is your current implementation of .hw_init() for MX7D
> is quite general to other SoCs as well, not much end-product specific.
> 
> Let's see it's only a simple initialization of MU and reset both side.
> +static void imx_mu_init_imx7d(struct imx_mu_priv *priv) {
> +	/* Set default config */
> +	if (priv->side_a)
> +		imx_mu_write(priv, 0, IMX_MU_xCR);
> +}
> That's why I thought it could be moved into general part instead of being
> provided as SoC specific .hw_init() callback.


Here is probably some misunderstanding. I'm not against
imx_mu_cfg_generic. I just prefer to keep "struct imx_mu_cfg", because i
already see me patching it back in some months .

> Because I'm afraid we may write similar code for other SoCs as well:
> e.g.
> static const struct imx_mu_cfg imx_mu_cfg_imx6sx = {
>         .chans = IMX_MU_MAX_CHANS,
>         .init_hw = imx_mu_init_imx7d,
> };
> 
> static const struct imx_mu_cfg imx_mu_cfg_imx7d = {
>         .chans = IMX_MU_MAX_CHANS,
>         .init_hw = imx_mu_init_imx7d,
> };
> 
> static const struct imx_mu_cfg imx_mu_cfg_imx7ulp = {
>         .chans = IMX_MU_MAX_CHANS,
>         .init_hw = imx_mu_init_imx7d,
> };
> 
> static const struct imx_mu_cfg imx_mu_cfg_imx8qxp = {
>         .chans = IMX_MU_MAX_CHANS,
>         .init_hw = imx_mu_init_imx7d,
> };
> 
> static const struct imx_mu_cfg imx_mu_cfg_imx8qm = {
>         .chans = IMX_MU_MAX_CHANS,
>         .init_hw = imx_mu_init_imx7d,
> };
> 
> static const struct imx_mu_cfg imx_mu_cfg_imx8mq = {
>         .chans = IMX_MU_MAX_CHANS,
>         .init_hw = imx_mu_init_imx7d,
> };
> 
> Anyway, I'm not strongly against it. Maybe we can also refine
> it later when we see the real problem.
> 
> Regards
> Dong Aisheng
> 
>>>>
>>>>>> +	.init_hw = imx_mu_init_imx7d,
>>>>>
>>>>> Can we imagine a diferent .init_hw callback for another SoC?
>>>>
>>>> yes, for example SCU case, or any other case where A side is used on
>>>> slave system.
>>>>
>>>
>>> SCU does not use this as we still did not see SoC specific init_hw
>> requirement.
>>>
>>>>> If no, how about make it default as I see the implementation is
>>>>> quite simple and seems not SoC specific. Then we probably can
>>>>> totally remove struct imx_mu_cfg.
>>>>
>>>> i prefer not to do it.
>>>>
>>>>>> +};
>>>>>> +
>>>>>> +static const struct of_device_id imx_mu_dt_ids[] = {
>>>>>> +	{ .compatible = "fsl,imx7s-mu", .data = &imx_mu_cfg_imx7d },
>>>>>> +	{ },
>>>>>> +};
>>>>>> +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);
>>>>>
>>>>> Do you think if we can escalated it a bit earlier as it's used by SCU?
>>>>> e.g. core_initcall ?
>>>>
>>>> Some more testing shoukld be done. Same MU driver is on master side a
>>>> clock consumer on slave side it is clock provider. It is valid for my
>>>> R&D project as well.
>>>> Let us concentrate on generic part first and then extend it as needed.
>>>
>>> Okay to me.
>>>
>>> Regards
>>> Dong Aisheng
>>>
> 


[-- 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 v3 1/3] dt-bindings: arm: fsl: add mu binding doc

[Oleksij Rempel]

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



On 17.07.2018 08:50, A.s. Dong wrote:
>> -----Original Message-----
>> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
>> Sent: Tuesday, July 17, 2018 2:32 PM
>> To: A.s. Dong <aisheng.dong@nxp.com>; Shawn Guo
>> <shawnguo@kernel.org>; Fabio Estevam <fabio.estevam@nxp.com>; Rob
>> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>
>> Cc: devicetree@vger.kernel.org; kernel@pengutronix.de; linux-arm-
>> kernel@lists.infradead.org; dl-linux-imx <linux-imx@nxp.com>
>> Subject: Re: [PATCH v3 1/3] dt-bindings: arm: fsl: add mu binding doc
>>
>>
>>
>> On 17.07.2018 08:26, A.s. Dong wrote:
>>>> -----Original Message-----
>>>> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
>>>> Sent: Tuesday, July 17, 2018 1:45 PM
>>>> To: A.s. Dong <aisheng.dong@nxp.com>; Shawn Guo
>>>> <shawnguo@kernel.org>; Fabio Estevam <fabio.estevam@nxp.com>;
>> Rob
>>>> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>
>>>> Cc: devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>>>> kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>
>>>> Subject: Re: [PATCH v3 1/3] dt-bindings: arm: fsl: add mu binding doc
>>>>
>>>>
>>>>
>>>> On 17.07.2018 07:00, A.s. Dong wrote:
>>>>>> -----Original Message-----
>>>>>> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
>>>>>> Sent: Monday, July 16, 2018 7:42 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 v3 1/3] dt-bindings: arm: fsl: add mu binding doc
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>>>>>> ---
>>>>>>  .../devicetree/bindings/mailbox/fsl,mu.txt    | 32
>>>> +++++++++++++++++++
>>>>>>  1 file changed, 32 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..5d48dd75b98d
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>>>>>> @@ -0,0 +1,32 @@
>>>>>> +NXP i.MX Messaging Unit (MU)
>>>>>> +------------------------------------------------------------------
>>>>>> +--
>>>>>> +
>>>>>> +Required properties:
>>>>>> +-------------------
>>>>>> +- compatible :	should be "fsl,<chip>-mu", the supported chips
>>>> include:
>>>>>> +		imx6sx	- i.MX 6SoloX
>>>>>> +		imx7d	- i.MX 7Dual
>>>>>> +		imx7s	- i.MX 7Solo
>>>>>> +		imx7ulp	- i.MX 7ULP
>>>>>> +		imx8qm	- i.MX 8QM
>>>>>> +		imx8qxp	- 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 - 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-a : Should be set for side A MU.
>>>>>
>>>>> For this property, how about doing like:
>>>>> fsl,mu-side: An Integer represents the MU side.
>>>>
>>>> All this SoCs have MUs with only two sides. Why do we need explicit
>>>> annotation for both parts?
>>>>
>>>>> If missing this property, it's default to Side A
>>>>
>>>> So, why do we need optional integer, which is set by default as side A?
>>>> This is why I made it bool.
>>>
>>> Yes, A bool probably is better.
>>>
>>>>
>>>>> 	       which is mostly used by A core.
>>>>
>>>> And you will need to explicit set side=B for SCU. Correct?
>>>
>>> SCU is using side A. AFAIK all SoC A core is using side A by default.
>>> That's why I think it can be default one. User is free to specify both
>>> In device tree. Does it make sense to you?
>>
>> Ok.
> 
> My original assumption is we can specify the side explicitly in device tree
>  (default to side A if missing) like:
> mu0a: mailbox@30aa0000 {
>         compatible = "fsl,imx7s-mu";
>         ...
>         fsl,mu-side = <0>;
> };
> 
> mu0b: mailbox@30ab0000 {
>         compatible = "fsl,imx7s-mu";
>         ...
>         fsl,mu-side = <1>;
> };
> 
> But you're right it can be simply indicated by a bool as well.
> mu0a: mailbox@30aa0000 {
>         compatible = "fsl,imx7s-mu";
>         ...
> };
> 
> mu0b: mailbox@30ab0000 {
>         compatible = "fsl,imx7s-mu";
>         ...
>         fsl,mu-side-b; 
> };
> 
> I'm okay with both. You can prefer as you wish.

There are advantages and disadvantages in both cases:
- bool is using less space and limited to only needed part but mistakes
can happen and some will forget to add this line.
- int part is good only if it is not optional.


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

[A.s. Dong]

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> Sent: Tuesday, July 17, 2018 3:49 PM
> To: A.s. Dong <aisheng.dong@nxp.com>; Shawn Guo
> <shawnguo@kernel.org>; Fabio Estevam <fabio.estevam@nxp.com>; Rob
> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>
> Cc: devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v3 3/3] mailbox: Add support for i.MX7D messaging unit
> 
> 
> 
> On 17.07.2018 09:26, A.s. Dong wrote:
> >> -----Original Message-----
> >> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> >> Sent: Tuesday, July 17, 2018 3:14 PM
> >> To: A.s. Dong <aisheng.dong@nxp.com>; Shawn Guo
> >> <shawnguo@kernel.org>; Fabio Estevam <fabio.estevam@nxp.com>;
> Rob
> >> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>
> >> Cc: devicetree@vger.kernel.org; kernel@pengutronix.de; linux-arm-
> >> kernel@lists.infradead.org; dl-linux-imx <linux-imx@nxp.com>
> >> Subject: Re: [PATCH v3 3/3] mailbox: Add support for i.MX7D messaging
> >> unit
> >>
> >>
> >>
> >> On 17.07.2018 09:07, A.s. Dong wrote:
> >>>> -----Original Message-----
> >>>> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> >>>> Sent: Tuesday, July 17, 2018 2:43 PM
> >>>> To: A.s. Dong <aisheng.dong@nxp.com>; Shawn Guo
> >>>> <shawnguo@kernel.org>; Fabio Estevam <fabio.estevam@nxp.com>;
> >> Rob
> >>>> Herring <robh+dt@kernel.org>; Mark Rutland
> <mark.rutland@arm.com>
> >>>> Cc: devicetree@vger.kernel.org;
> >>>> linux-arm-kernel@lists.infradead.org;
> >>>> kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>
> >>>> Subject: Re: [PATCH v3 3/3] mailbox: Add support for i.MX7D
> >>>> messaging unit
> >>>>
> >>>>
> >>>>
> >>>> On 17.07.2018 08:21, A.s. Dong wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> >>>>>> Sent: Monday, July 16, 2018 7:42 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 v3 3/3] mailbox: Add support for i.MX7D messaging
> >>>>>> unit
> >>>>>>
> >>>>>> The Mailbox controller is able to send messages (up to 4 32 bit
> >>>>>> words) between the endpoints.
> >>>>>>
> >>>>>> This driver was tested using the mailbox-test driver sending
> >>>>>> messages between the Cortex-A7 and the Cortex-M4.
> >>>>>>
> >>>>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> >>>>>> ---
> >>>>>>  drivers/mailbox/Kconfig       |   6 +
> >>>>>>  drivers/mailbox/Makefile      |   2 +
> >>>>>>  drivers/mailbox/imx-mailbox.c | 294
> >>>>>> ++++++++++++++++++++++++++++++++++
> >>>>>>  3 files changed, 302 insertions(+)  create mode 100644
> >>>>>> drivers/mailbox/imx-mailbox.c
> >>>>>>
> >>>>>
> >>>>> Generally it looks good to me, a few minor comments:
> >>>>>
> >>>>>> 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..31353366a007
> >>>>>> --- /dev/null
> >>>>>> +++ b/drivers/mailbox/imx-mailbox.c
> >>>>>> @@ -0,0 +1,294 @@
> >>>>>> +// 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;
> >>>>>> +};
> >>>>>> +
> >>>>>> +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_a;
> >>>>>> +};
> >>>>>> +
> >>>>>> +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;
> >>>>>
> >>>>> Better re-order from long to short.
> >>>>
> >>>> "chis" is used by by following declarations. It make no sense to reorder
> it.
> >>>>
> >>>
> >>> Yes, you're right.
> >>>
> >>>>>
> >>>>>> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> >>>>>> +	struct imx_mu_con_priv *cp = chan->con_priv;
> >>>>>> +
> >>>>>
> >>>>> Unnecessary blank line?
> >>>>
> >>>> ok,
> >>>>
> >>>>>> +	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;
> >>>>>> +	char *irq_desc;
> >>>>>> +	int ret;
> >>>>>> +
> >>>>>> +	irq_desc = devm_kasprintf(priv->dev, GFP_KERNEL,
> >>>>>> "imx_mu_chan[%i]",
> >>>>>> +				  cp->idx);
> >>>>>
> >>>>> I like the name differentiation, just wondering whether this could
> >>>>> cause memory leak if users repeatly open/close MU channels due to
> >>>>> I see no free.
> >>>>
> >>>> good point.
> >>>>
> >>>>>
> >>>>>> +	if (!irq_desc)
> >>>>>> +		return -ENOMEM;
> >>>>>> +
> >>>>>> +	ret = devm_request_irq(priv->dev, cp->irq, imx_mu_isr,
> >>>>>> +			       IRQF_SHARED, 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); }
> >>>>>> +
> >>>>>> +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-a"))
> >>>>>> +		priv->side_a = true;
> >>>>>
> >>>>> See property comments in former emails.
> >>>>
> >>>> ok.
> >>>>
> >>>>>> +
> >>>>>> +	priv->mbox.dev = dev;
> >>>>>> +	priv->mbox.ops = &imx_mu_ops;
> >>>>>> +	priv->mbox.chans = priv->mbox_chans;
> >>>>>> +	priv->mbox.num_chans = chans;
> >>>>>> +	priv->mbox.txdone_irq = true;
> >>>>>> +
> >>>>>> +	platform_set_drvdata(pdev, priv);
> >>>>>> +
> >>>>>> +	if (priv->dcfg->init_hw)
> >>>>>> +		priv->dcfg->init_hw(priv);
> >>>>>> +
> >>>>>> +	return mbox_controller_register(&priv->mbox);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static int imx_mu_remove(struct platform_device *pdev) {
> >>>>>> +	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
> >>>>>> +
> >>>>>> +	mbox_controller_unregister(&priv->mbox);
> >>>>>> +	clk_disable_unprepare(priv->clk);
> >>>>>> +
> >>>>>> +	return 0;
> >>>>>> +}
> >>>>>> +
> >>>>>> +
> >>>>>> +static void imx_mu_init_imx7d(struct imx_mu_priv *priv) {
> >>>>>
> >>>>> I guess we could remove the soc postfix now.
> >>>>
> >>>> ok.
> >>>>
> >>>>>
> >>>>>> +	/* Set default config */
> >>>>>> +	if (priv->side_a)
> >>>>>> +		imx_mu_write(priv, 0, IMX_MU_xCR); }
> >>>>>> +
> >>>>>> +static const struct imx_mu_cfg imx_mu_cfg_imx7d = {
> >>>>>> +	.chans = IMX_MU_MAX_CHANS,
> >>>>>
> >>>>> What's point of another chans here?
> >>>>> This can also be controlled by IMX_MU_MAX_CHANS.
> >>>>
> >>>> SCU has one channel configuration.
> >>>
> >>> I guess the SCU one channel(4 rx/tx register) actually is not the
> >>> one channel specified here. e.g. For M4 case, users can still
> >>> specify only using one chan
> >>> (1 rx/tx register). They're slightly different.
> >>> So for SCU case, we actually did not use the struct imx_mu_cfg.
> >>>
> >>>> It is possible to exctend this driver with existing MU to provide 8
> >>>> channels: 4 channels with FIFO (can be used stand alone) + 4
> >>>> channes based on General purpose interrupt and should be used with
> >>>> shared
> >> memory.
> >>>
> >>> Yes, but that still seems common to me. Not per SoC specific.
> >>
> >> Correct, it is not SoC specific, it is end-product specific. All
> >> depends on what exactly is on opposite side of MU. Currently I don't
> >> see any other way as providing product specific compatible with
> >> needed configuration. And current driver version allows it without ugly
> patching.
> >>
> >
> > I think the point is your current implementation of .hw_init() for
> > MX7D is quite general to other SoCs as well, not much end-product specific.
> >
> > Let's see it's only a simple initialization of MU and reset both side.
> > +static void imx_mu_init_imx7d(struct imx_mu_priv *priv) {
> > +	/* Set default config */
> > +	if (priv->side_a)
> > +		imx_mu_write(priv, 0, IMX_MU_xCR);
> > +}
> > That's why I thought it could be moved into general part instead of
> > being provided as SoC specific .hw_init() callback.
> 
> 
> Here is probably some misunderstanding. I'm not against
> imx_mu_cfg_generic. I just prefer to keep "struct imx_mu_cfg", because i
> already see me patching it back in some months .

My former suggestion is removing "struct imx_mu_cfg" because I did not
see different requirement.
But if you insist and you already see real requirement, I'm okay to keep it
and change later if we face real problems.

Regards
Dong Aisheng

> 
> > Because I'm afraid we may write similar code for other SoCs as well:
> > e.g.
> > static const struct imx_mu_cfg imx_mu_cfg_imx6sx = {
> >         .chans = IMX_MU_MAX_CHANS,
> >         .init_hw = imx_mu_init_imx7d,
> > };
> >
> > static const struct imx_mu_cfg imx_mu_cfg_imx7d = {
> >         .chans = IMX_MU_MAX_CHANS,
> >         .init_hw = imx_mu_init_imx7d,
> > };
> >
> > static const struct imx_mu_cfg imx_mu_cfg_imx7ulp = {
> >         .chans = IMX_MU_MAX_CHANS,
> >         .init_hw = imx_mu_init_imx7d,
> > };
> >
> > static const struct imx_mu_cfg imx_mu_cfg_imx8qxp = {
> >         .chans = IMX_MU_MAX_CHANS,
> >         .init_hw = imx_mu_init_imx7d,
> > };
> >
> > static const struct imx_mu_cfg imx_mu_cfg_imx8qm = {
> >         .chans = IMX_MU_MAX_CHANS,
> >         .init_hw = imx_mu_init_imx7d,
> > };
> >
> > static const struct imx_mu_cfg imx_mu_cfg_imx8mq = {
> >         .chans = IMX_MU_MAX_CHANS,
> >         .init_hw = imx_mu_init_imx7d,
> > };
> >
> > Anyway, I'm not strongly against it. Maybe we can also refine it later
> > when we see the real problem.
> >
> > Regards
> > Dong Aisheng
> >
> >>>>
> >>>>>> +	.init_hw = imx_mu_init_imx7d,
> >>>>>
> >>>>> Can we imagine a diferent .init_hw callback for another SoC?
> >>>>
> >>>> yes, for example SCU case, or any other case where A side is used
> >>>> on slave system.
> >>>>
> >>>
> >>> SCU does not use this as we still did not see SoC specific init_hw
> >> requirement.
> >>>
> >>>>> If no, how about make it default as I see the implementation is
> >>>>> quite simple and seems not SoC specific. Then we probably can
> >>>>> totally remove struct imx_mu_cfg.
> >>>>
> >>>> i prefer not to do it.
> >>>>
> >>>>>> +};
> >>>>>> +
> >>>>>> +static const struct of_device_id imx_mu_dt_ids[] = {
> >>>>>> +	{ .compatible = "fsl,imx7s-mu", .data =
> &imx_mu_cfg_imx7d },
> >>>>>> +	{ },
> >>>>>> +};
> >>>>>> +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);
> >>>>>
> >>>>> Do you think if we can escalated it a bit earlier as it's used by SCU?
> >>>>> e.g. core_initcall ?
> >>>>
> >>>> Some more testing shoukld be done. Same MU driver is on master side
> >>>> a clock consumer on slave side it is clock provider. It is valid
> >>>> for my R&D project as well.
> >>>> Let us concentrate on generic part first and then extend it as needed.
> >>>
> >>> Okay to me.
> >>>
> >>> Regards
> >>> Dong Aisheng
> >>>
> >

RE: [PATCH v3 1/3] dt-bindings: arm: fsl: add mu binding doc

[A.s. Dong]

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> Sent: Tuesday, July 17, 2018 12:56 PM
> To: A.s. Dong <aisheng.dong@nxp.com>; Rob Herring <robh@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>; devicetree@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam
> <fabio.estevam@nxp.com>; Shawn Guo <shawnguo@kernel.org>; linux-
> arm-kernel@lists.infradead.org
> Subject: Re: [PATCH v3 1/3] dt-bindings: arm: fsl: add mu binding doc
> 
> Hi,
> 
> On 17.07.2018 06:51, A.s. Dong wrote:
> >> -----Original Message-----
> >> From: Rob Herring [mailto:robh@kernel.org]
> >> Sent: Monday, July 16, 2018 11:17 PM
> >> To: Oleksij Rempel <o.rempel@pengutronix.de>
> >> Cc: Shawn Guo <shawnguo@kernel.org>; Fabio Estevam
> >> <fabio.estevam@nxp.com>; 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 v3 1/3] dt-bindings: arm: fsl: add mu binding doc
> >>
> >> On Mon, Jul 16, 2018 at 01:42:07PM +0200, Oleksij Rempel wrote:
> >>> 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.
> >>
> >> v3? I already gave my R-by on v4 from Dong. Please coordinate your
> >> work on this.
> >
> > Okay.
> >
> > Oleksij,
> > could you re-add the M4 support part in a separate patch based on the
> > already acked SCU one?
> > I guess you can re-send with the acked one.
> 
> Rob, what do you prefer: two patches for one new file or one squashed
> patch?

I guess Rob's meaning is two separate patches as they're two features.
We probably can send and test in case Rob is missing this mail.
If any issue, we can simply change later. Anyway, that allow us to
review a new version of other part instead of pending here.

Regards
Dong Aisheng