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

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

[Oleksij Rempel]

20180731 changes v7:
- imx-mailbox & DT: rework driver from 4 channel bidirectional mode to
  16 channel unidirectional mode. 8 extra channels are implemented on
  top of General Purpose Interrupt bits provided by this MU.

20180726 changes v7:
- DT: add i.MX6SX and i.MX7S to the documentation.
- imx-mailbox: don't use devm_ functions for startup and shutdown.
- imx-mailbox: rename imx_mu_rmw to imx_mu_xcr_rmw and add locks
- imx-mailbox: pass of_property_read_bool directly to side_b

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

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

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

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

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

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

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

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

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

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

-- 
2.18.0

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

[Oleksij Rempel]

20180731 changes v7:
- imx-mailbox & DT: rework driver from 4 channel bidirectional mode to
  16 channel unidirectional mode. 8 extra channels are implemented on
  top of General Purpose Interrupt bits provided by this MU.

20180726 changes v7:
- DT: add i.MX6SX and i.MX7S to the documentation.
- imx-mailbox: don't use devm_ functions for startup and shutdown.
- imx-mailbox: rename imx_mu_rmw to imx_mu_xcr_rmw and add locks
- imx-mailbox: pass of_property_read_bool directly to side_b

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

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

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

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

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

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

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

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

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

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

-- 
2.18.0

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

[Oleksij Rempel]

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

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

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

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

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

[Oleksij Rempel]

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

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

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

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

[Oleksij Rempel]

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

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

diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
index 90e4905dfc69..9efd3a9ade44 100644
--- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
+++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
@@ -18,11 +18,33 @@ Messaging Unit Device Node:
 Required properties:
 -------------------
 - compatible :	should be "fsl,<chip>-mu", the supported chips include
-		imx8qxp, imx8qm.
+		imx6sx, imx7s, imx8qxp, imx8qm.
+		The "fsl,imx6sx-mu" compatible is seen as generic and should
+		be included together with SoC specific compatible.
 - reg :		Should contain the registers location and length
 - interrupts :	Interrupt number. The interrupt specifier format depends
 		on the interrupt controller parent.
-- #mbox-cells:  Must be 0. Number of cells in a mailbox
+- #mbox-cells:  Must be 2.
+			  <&phandle type channel>
+			    phandle   : Label name of controller
+			    type      : Channel type
+			    channel   : Channel number
+
+		This MU support 4 type of unidirectional channels, each type
+		has 4 channels. A total of 16 channels. Following types are
+		supported:
+		0 - TX channel with 32bit transmit register and IRQ transmit
+		acknowledgment support.
+		1 - RX channel with 32bit receive register and IRQ support
+		2 - TX doorbell channel. Without own register and no ACK support.
+		3 - RX doorbell channel.
+		The doorbell channels should be used with shared memory and protocol
+		level acknowledgment if needed.
+
+Optional properties:
+-------------------
+- clocks :	phandle to the input clock.
+- fsl,mu-side-b : Should be set for side B MU.
 
 Examples:
 --------
@@ -30,5 +52,5 @@ lsio_mu0: mailbox@5d1b0000 {
 	compatible = "fsl,imx8qxp-mu";
 	reg = <0x0 0x5d1b0000 0x0 0x10000>;
 	interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
-	#mbox-cells = <0>;
+	#mbox-cells = <2>;
 };
-- 
2.18.0

[PATCH v8 2/4] dt-bindings: mailbox: imx-mu: add generic MU channel support

[Oleksij Rempel]

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

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

diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
index 90e4905dfc69..9efd3a9ade44 100644
--- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
+++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
@@ -18,11 +18,33 @@ Messaging Unit Device Node:
 Required properties:
 -------------------
 - compatible :	should be "fsl,<chip>-mu", the supported chips include
-		imx8qxp, imx8qm.
+		imx6sx, imx7s, imx8qxp, imx8qm.
+		The "fsl,imx6sx-mu" compatible is seen as generic and should
+		be included together with SoC specific compatible.
 - reg :		Should contain the registers location and length
 - interrupts :	Interrupt number. The interrupt specifier format depends
 		on the interrupt controller parent.
-- #mbox-cells:  Must be 0. Number of cells in a mailbox
+- #mbox-cells:  Must be 2.
+			  <&phandle type channel>
+			    phandle   : Label name of controller
+			    type      : Channel type
+			    channel   : Channel number
+
+		This MU support 4 type of unidirectional channels, each type
+		has 4 channels. A total of 16 channels. Following types are
+		supported:
+		0 - TX channel with 32bit transmit register and IRQ transmit
+		acknowledgment support.
+		1 - RX channel with 32bit receive register and IRQ support
+		2 - TX doorbell channel. Without own register and no ACK support.
+		3 - RX doorbell channel.
+		The doorbell channels should be used with shared memory and protocol
+		level acknowledgment if needed.
+
+Optional properties:
+-------------------
+- clocks :	phandle to the input clock.
+- fsl,mu-side-b : Should be set for side B MU.
 
 Examples:
 --------
@@ -30,5 +52,5 @@ lsio_mu0: mailbox at 5d1b0000 {
 	compatible = "fsl,imx8qxp-mu";
 	reg = <0x0 0x5d1b0000 0x0 0x10000>;
 	interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>;
-	#mbox-cells = <0>;
+	#mbox-cells = <2>;
 };
-- 
2.18.0

Re: [PATCH v8 2/4] dt-bindings: mailbox: imx-mu: add generic MU channel support

[Jassi Brar]

On Tue, Jul 31, 2018 at 7:41 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> Each MU has four pairs of rx/tx data register with four rx/tx interrupts
> which can also be used as a separate channel.
>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  .../devicetree/bindings/mailbox/fsl,mu.txt    | 28 +++++++++++++++++--
>  1 file changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> index 90e4905dfc69..9efd3a9ade44 100644
> --- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> @@ -18,11 +18,33 @@ Messaging Unit Device Node:
>  Required properties:
>  -------------------
>  - compatible : should be "fsl,<chip>-mu", the supported chips include
> -               imx8qxp, imx8qm.
> +               imx6sx, imx7s, imx8qxp, imx8qm.
> +               The "fsl,imx6sx-mu" compatible is seen as generic and should
> +               be included together with SoC specific compatible.
>  - reg :                Should contain the registers location and length
>  - interrupts : Interrupt number. The interrupt specifier format depends
>                 on the interrupt controller parent.
> -- #mbox-cells:  Must be 0. Number of cells in a mailbox
> +- #mbox-cells:  Must be 2.
>
This seems like modifying the bindings. But since nothing exists yet,
maybe we should merge patch 1 and 2 ?


> +                         <&phandle type channel>
> +                           phandle   : Label name of controller
> +                           type      : Channel type
> +                           channel   : Channel number
> +
> +               This MU support 4 type of unidirectional channels, each type
> +               has 4 channels. A total of 16 channels. Following types are
> +               supported:
> +               0 - TX channel with 32bit transmit register and IRQ transmit
> +               acknowledgment support.
> +               1 - RX channel with 32bit receive register and IRQ support
> +               2 - TX doorbell channel. Without own register and no ACK support.
> +               3 - RX doorbell channel.
>
Thanks. This is great.

> +               The doorbell channels should be used with shared memory and protocol
> +               level acknowledgment if needed.
> +
I would avoid this. People might get notions that they have to use
shmem with doorbell -- a trivial protocol might mean doing some fixed
action (like reset) whenever the doorbell rings.


> +Optional properties:
> +-------------------
> +- clocks :     phandle to the input clock.
> +- fsl,mu-side-b : Should be set for side B MU.
>
Nit: can we call this 'fsl,no-init' ?

[PATCH v8 2/4] dt-bindings: mailbox: imx-mu: add generic MU channel support

[Jassi Brar]

On Tue, Jul 31, 2018 at 7:41 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> Each MU has four pairs of rx/tx data register with four rx/tx interrupts
> which can also be used as a separate channel.
>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  .../devicetree/bindings/mailbox/fsl,mu.txt    | 28 +++++++++++++++++--
>  1 file changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> index 90e4905dfc69..9efd3a9ade44 100644
> --- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> @@ -18,11 +18,33 @@ Messaging Unit Device Node:
>  Required properties:
>  -------------------
>  - compatible : should be "fsl,<chip>-mu", the supported chips include
> -               imx8qxp, imx8qm.
> +               imx6sx, imx7s, imx8qxp, imx8qm.
> +               The "fsl,imx6sx-mu" compatible is seen as generic and should
> +               be included together with SoC specific compatible.
>  - reg :                Should contain the registers location and length
>  - interrupts : Interrupt number. The interrupt specifier format depends
>                 on the interrupt controller parent.
> -- #mbox-cells:  Must be 0. Number of cells in a mailbox
> +- #mbox-cells:  Must be 2.
>
This seems like modifying the bindings. But since nothing exists yet,
maybe we should merge patch 1 and 2 ?


> +                         <&phandle type channel>
> +                           phandle   : Label name of controller
> +                           type      : Channel type
> +                           channel   : Channel number
> +
> +               This MU support 4 type of unidirectional channels, each type
> +               has 4 channels. A total of 16 channels. Following types are
> +               supported:
> +               0 - TX channel with 32bit transmit register and IRQ transmit
> +               acknowledgment support.
> +               1 - RX channel with 32bit receive register and IRQ support
> +               2 - TX doorbell channel. Without own register and no ACK support.
> +               3 - RX doorbell channel.
>
Thanks. This is great.

> +               The doorbell channels should be used with shared memory and protocol
> +               level acknowledgment if needed.
> +
I would avoid this. People might get notions that they have to use
shmem with doorbell -- a trivial protocol might mean doing some fixed
action (like reset) whenever the doorbell rings.


> +Optional properties:
> +-------------------
> +- clocks :     phandle to the input clock.
> +- fsl,mu-side-b : Should be set for side B MU.
>
Nit: can we call this 'fsl,no-init' ?

Re: [PATCH v8 2/4] dt-bindings: mailbox: imx-mu: add generic MU channel support

[Oleksij Rempel]

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

On Tue, Jul 31, 2018 at 09:28:09PM +0530, Jassi Brar wrote:
> On Tue, Jul 31, 2018 at 7:41 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> > Each MU has four pairs of rx/tx data register with four rx/tx interrupts
> > which can also be used as a separate channel.
> >
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  .../devicetree/bindings/mailbox/fsl,mu.txt    | 28 +++++++++++++++++--
> >  1 file changed, 25 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > index 90e4905dfc69..9efd3a9ade44 100644
> > --- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > @@ -18,11 +18,33 @@ Messaging Unit Device Node:
> >  Required properties:
> >  -------------------
> >  - compatible : should be "fsl,<chip>-mu", the supported chips include
> > -               imx8qxp, imx8qm.
> > +               imx6sx, imx7s, imx8qxp, imx8qm.
> > +               The "fsl,imx6sx-mu" compatible is seen as generic and should
> > +               be included together with SoC specific compatible.
> >  - reg :                Should contain the registers location and length
> >  - interrupts : Interrupt number. The interrupt specifier format depends
> >                 on the interrupt controller parent.
> > -- #mbox-cells:  Must be 0. Number of cells in a mailbox
> > +- #mbox-cells:  Must be 2.
> >
> This seems like modifying the bindings. But since nothing exists yet,
> maybe we should merge patch 1 and 2 ?

For some weeks I already asked to squash this patches.. it was not
ACKed. We can try again... @Rob, @Aisheng, should I merge this two
patches?

> 
> > +                         <&phandle type channel>
> > +                           phandle   : Label name of controller
> > +                           type      : Channel type
> > +                           channel   : Channel number
> > +
> > +               This MU support 4 type of unidirectional channels, each type
> > +               has 4 channels. A total of 16 channels. Following types are
> > +               supported:
> > +               0 - TX channel with 32bit transmit register and IRQ transmit
> > +               acknowledgment support.
> > +               1 - RX channel with 32bit receive register and IRQ support
> > +               2 - TX doorbell channel. Without own register and no ACK support.
> > +               3 - RX doorbell channel.
> >
> Thanks. This is great.
> 
> > +               The doorbell channels should be used with shared memory and protocol
> > +               level acknowledgment if needed.
> > +
> I would avoid this. People might get notions that they have to use
> shmem with doorbell -- a trivial protocol might mean doing some fixed
> action (like reset) whenever the doorbell rings.

Ok.

> 
> > +Optional properties:
> > +-------------------
> > +- clocks :     phandle to the input clock.
> > +- fsl,mu-side-b : Should be set for side B MU.
> >
> Nit: can we call this 'fsl,no-init' ?

No. It is HW description and not functionality which is present in
current Linux driver.

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

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

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

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

[PATCH v8 2/4] dt-bindings: mailbox: imx-mu: add generic MU channel support

[Oleksij Rempel]

On Tue, Jul 31, 2018 at 09:28:09PM +0530, Jassi Brar wrote:
> On Tue, Jul 31, 2018 at 7:41 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> > Each MU has four pairs of rx/tx data register with four rx/tx interrupts
> > which can also be used as a separate channel.
> >
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  .../devicetree/bindings/mailbox/fsl,mu.txt    | 28 +++++++++++++++++--
> >  1 file changed, 25 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > index 90e4905dfc69..9efd3a9ade44 100644
> > --- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > @@ -18,11 +18,33 @@ Messaging Unit Device Node:
> >  Required properties:
> >  -------------------
> >  - compatible : should be "fsl,<chip>-mu", the supported chips include
> > -               imx8qxp, imx8qm.
> > +               imx6sx, imx7s, imx8qxp, imx8qm.
> > +               The "fsl,imx6sx-mu" compatible is seen as generic and should
> > +               be included together with SoC specific compatible.
> >  - reg :                Should contain the registers location and length
> >  - interrupts : Interrupt number. The interrupt specifier format depends
> >                 on the interrupt controller parent.
> > -- #mbox-cells:  Must be 0. Number of cells in a mailbox
> > +- #mbox-cells:  Must be 2.
> >
> This seems like modifying the bindings. But since nothing exists yet,
> maybe we should merge patch 1 and 2 ?

For some weeks I already asked to squash this patches.. it was not
ACKed. We can try again... @Rob, @Aisheng, should I merge this two
patches?

> 
> > +                         <&phandle type channel>
> > +                           phandle   : Label name of controller
> > +                           type      : Channel type
> > +                           channel   : Channel number
> > +
> > +               This MU support 4 type of unidirectional channels, each type
> > +               has 4 channels. A total of 16 channels. Following types are
> > +               supported:
> > +               0 - TX channel with 32bit transmit register and IRQ transmit
> > +               acknowledgment support.
> > +               1 - RX channel with 32bit receive register and IRQ support
> > +               2 - TX doorbell channel. Without own register and no ACK support.
> > +               3 - RX doorbell channel.
> >
> Thanks. This is great.
> 
> > +               The doorbell channels should be used with shared memory and protocol
> > +               level acknowledgment if needed.
> > +
> I would avoid this. People might get notions that they have to use
> shmem with doorbell -- a trivial protocol might mean doing some fixed
> action (like reset) whenever the doorbell rings.

Ok.

> 
> > +Optional properties:
> > +-------------------
> > +- clocks :     phandle to the input clock.
> > +- fsl,mu-side-b : Should be set for side B MU.
> >
> Nit: can we call this 'fsl,no-init' ?

No. It is HW description and not functionality which is present in
current Linux driver.

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180801/07207792/attachment.sig>

RE: [PATCH v8 2/4] dt-bindings: mailbox: imx-mu: add generic MU channel support

[A.s. Dong]

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]

[...]

> > > diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > > b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > > index 90e4905dfc69..9efd3a9ade44 100644
> > > --- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > > +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > > @@ -18,11 +18,33 @@ Messaging Unit Device Node:
> > >  Required properties:
> > >  -------------------
> > >  - compatible : should be "fsl,<chip>-mu", the supported chips include
> > > -               imx8qxp, imx8qm.
> > > +               imx6sx, imx7s, imx8qxp, imx8qm.
> > > +               The "fsl,imx6sx-mu" compatible is seen as generic and should
> > > +               be included together with SoC specific compatible.
> > >  - reg :                Should contain the registers location and length
> > >  - interrupts : Interrupt number. The interrupt specifier format depends
> > >                 on the interrupt controller parent.
> > > -- #mbox-cells:  Must be 0. Number of cells in a mailbox
> > > +- #mbox-cells:  Must be 2.
> > >
> > This seems like modifying the bindings. But since nothing exists yet,
> > maybe we should merge patch 1 and 2 ?
> 
> For some weeks I already asked to squash this patches.. it was not ACKed.
> We can try again... @Rob, @Aisheng, should I merge this two patches?
> 

I guess It may depend on whether we use single channel mode for SCU.

I'm not sure we've already got the conclusion that multi channel mode
is suitable for SCU. I'm trying to implement it but meet some problems.
I've just explained in another reply. Hope Jassie can shine some light.

Regards
Dong Aisheng

> >
> > > +                         <&phandle type channel>
> > > +                           phandle   : Label name of controller
> > > +                           type      : Channel type
> > > +                           channel   : Channel number
> > > +
> > > +               This MU support 4 type of unidirectional channels, each type
> > > +               has 4 channels. A total of 16 channels. Following types are
> > > +               supported:
> > > +               0 - TX channel with 32bit transmit register and IRQ transmit
> > > +               acknowledgment support.
> > > +               1 - RX channel with 32bit receive register and IRQ support
> > > +               2 - TX doorbell channel. Without own register and no ACK
> support.
> > > +               3 - RX doorbell channel.
> > >
> > Thanks. This is great.
> >
> > > +               The doorbell channels should be used with shared memory and
> protocol
> > > +               level acknowledgment if needed.
> > > +
> > I would avoid this. People might get notions that they have to use
> > shmem with doorbell -- a trivial protocol might mean doing some fixed
> > action (like reset) whenever the doorbell rings.
> 
> Ok.
> 
> >
> > > +Optional properties:
> > > +-------------------
> > > +- clocks :     phandle to the input clock.
> > > +- fsl,mu-side-b : Should be set for side B MU.
> > >
> > Nit: can we call this 'fsl,no-init' ?
> 
> No. It is HW description and not functionality which is present in current Linux
> driver.
> 
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH v8 2/4] dt-bindings: mailbox: imx-mu: add generic MU channel support

[A.s. Dong]

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel at pengutronix.de]

[...]

> > > diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > > b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > > index 90e4905dfc69..9efd3a9ade44 100644
> > > --- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > > +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > > @@ -18,11 +18,33 @@ Messaging Unit Device Node:
> > >  Required properties:
> > >  -------------------
> > >  - compatible : should be "fsl,<chip>-mu", the supported chips include
> > > -               imx8qxp, imx8qm.
> > > +               imx6sx, imx7s, imx8qxp, imx8qm.
> > > +               The "fsl,imx6sx-mu" compatible is seen as generic and should
> > > +               be included together with SoC specific compatible.
> > >  - reg :                Should contain the registers location and length
> > >  - interrupts : Interrupt number. The interrupt specifier format depends
> > >                 on the interrupt controller parent.
> > > -- #mbox-cells:  Must be 0. Number of cells in a mailbox
> > > +- #mbox-cells:  Must be 2.
> > >
> > This seems like modifying the bindings. But since nothing exists yet,
> > maybe we should merge patch 1 and 2 ?
> 
> For some weeks I already asked to squash this patches.. it was not ACKed.
> We can try again... @Rob, @Aisheng, should I merge this two patches?
> 

I guess It may depend on whether we use single channel mode for SCU.

I'm not sure we've already got the conclusion that multi channel mode
is suitable for SCU. I'm trying to implement it but meet some problems.
I've just explained in another reply. Hope Jassie can shine some light.

Regards
Dong Aisheng

> >
> > > +                         <&phandle type channel>
> > > +                           phandle   : Label name of controller
> > > +                           type      : Channel type
> > > +                           channel   : Channel number
> > > +
> > > +               This MU support 4 type of unidirectional channels, each type
> > > +               has 4 channels. A total of 16 channels. Following types are
> > > +               supported:
> > > +               0 - TX channel with 32bit transmit register and IRQ transmit
> > > +               acknowledgment support.
> > > +               1 - RX channel with 32bit receive register and IRQ support
> > > +               2 - TX doorbell channel. Without own register and no ACK
> support.
> > > +               3 - RX doorbell channel.
> > >
> > Thanks. This is great.
> >
> > > +               The doorbell channels should be used with shared memory and
> protocol
> > > +               level acknowledgment if needed.
> > > +
> > I would avoid this. People might get notions that they have to use
> > shmem with doorbell -- a trivial protocol might mean doing some fixed
> > action (like reset) whenever the doorbell rings.
> 
> Ok.
> 
> >
> > > +Optional properties:
> > > +-------------------
> > > +- clocks :     phandle to the input clock.
> > > +- fsl,mu-side-b : Should be set for side B MU.
> > >
> > Nit: can we call this 'fsl,no-init' ?
> 
> No. It is HW description and not functionality which is present in current Linux
> driver.
> 
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v8 2/4] dt-bindings: mailbox: imx-mu: add generic MU channel support

[Rob Herring]

On Tue, Jul 31, 2018 at 11:05 PM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
>
> On Tue, Jul 31, 2018 at 09:28:09PM +0530, Jassi Brar wrote:
> > On Tue, Jul 31, 2018 at 7:41 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> > > Each MU has four pairs of rx/tx data register with four rx/tx interrupts
> > > which can also be used as a separate channel.
> > >
> > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > ---
> > >  .../devicetree/bindings/mailbox/fsl,mu.txt    | 28 +++++++++++++++++--
> > >  1 file changed, 25 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > > index 90e4905dfc69..9efd3a9ade44 100644
> > > --- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > > +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > > @@ -18,11 +18,33 @@ Messaging Unit Device Node:
> > >  Required properties:
> > >  -------------------
> > >  - compatible : should be "fsl,<chip>-mu", the supported chips include
> > > -               imx8qxp, imx8qm.
> > > +               imx6sx, imx7s, imx8qxp, imx8qm.
> > > +               The "fsl,imx6sx-mu" compatible is seen as generic and should
> > > +               be included together with SoC specific compatible.
> > >  - reg :                Should contain the registers location and length
> > >  - interrupts : Interrupt number. The interrupt specifier format depends
> > >                 on the interrupt controller parent.
> > > -- #mbox-cells:  Must be 0. Number of cells in a mailbox
> > > +- #mbox-cells:  Must be 2.
> > >
> > This seems like modifying the bindings. But since nothing exists yet,
> > maybe we should merge patch 1 and 2 ?
>
> For some weeks I already asked to squash this patches.. it was not
> ACKed. We can try again... @Rob, @Aisheng, should I merge this two
> patches?

Whatever you like. I don't really care.

Rob

[PATCH v8 2/4] dt-bindings: mailbox: imx-mu: add generic MU channel support

[Rob Herring]

On Tue, Jul 31, 2018 at 11:05 PM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
>
> On Tue, Jul 31, 2018 at 09:28:09PM +0530, Jassi Brar wrote:
> > On Tue, Jul 31, 2018 at 7:41 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> > > Each MU has four pairs of rx/tx data register with four rx/tx interrupts
> > > which can also be used as a separate channel.
> > >
> > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > ---
> > >  .../devicetree/bindings/mailbox/fsl,mu.txt    | 28 +++++++++++++++++--
> > >  1 file changed, 25 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > > index 90e4905dfc69..9efd3a9ade44 100644
> > > --- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > > +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > > @@ -18,11 +18,33 @@ Messaging Unit Device Node:
> > >  Required properties:
> > >  -------------------
> > >  - compatible : should be "fsl,<chip>-mu", the supported chips include
> > > -               imx8qxp, imx8qm.
> > > +               imx6sx, imx7s, imx8qxp, imx8qm.
> > > +               The "fsl,imx6sx-mu" compatible is seen as generic and should
> > > +               be included together with SoC specific compatible.
> > >  - reg :                Should contain the registers location and length
> > >  - interrupts : Interrupt number. The interrupt specifier format depends
> > >                 on the interrupt controller parent.
> > > -- #mbox-cells:  Must be 0. Number of cells in a mailbox
> > > +- #mbox-cells:  Must be 2.
> > >
> > This seems like modifying the bindings. But since nothing exists yet,
> > maybe we should merge patch 1 and 2 ?
>
> For some weeks I already asked to squash this patches.. it was not
> ACKed. We can try again... @Rob, @Aisheng, should I merge this two
> patches?

Whatever you like. I don't really care.

Rob

RE: [PATCH v8 2/4] dt-bindings: mailbox: imx-mu: add generic MU channel support

[A.s. Dong]

Hi Jassi,

> > +               The doorbell channels should be used with shared memory and
> protocol
> > +               level acknowledgment if needed.
> > +
> I would avoid this. People might get notions that they have to use shmem
> with doorbell -- a trivial protocol might mean doing some fixed action (like
> reset) whenever the doorbell rings.
>

That's right.
i.MX8 using the general purpose interrupt for peripherals. No shmem needed.
e.g. RTC, Watchdog and ON/OFF interrupt.

BTW, this means the peripheral will use mailbox doorbell channels to handle
Interrupts. Is there such user case in kernel we can refer to?

> 
> > +Optional properties:
> > +-------------------
> > +- clocks :     phandle to the input clock.
> > +- fsl,mu-side-b : Should be set for side B MU.
> >
> Nit: can we call this 'fsl,no-init' ?

Fsl,mu-side-b is more describing HW.

Regards
Dong Aisheng

[PATCH v8 2/4] dt-bindings: mailbox: imx-mu: add generic MU channel support

[A.s. Dong]

Hi Jassi,

> > +               The doorbell channels should be used with shared memory and
> protocol
> > +               level acknowledgment if needed.
> > +
> I would avoid this. People might get notions that they have to use shmem
> with doorbell -- a trivial protocol might mean doing some fixed action (like
> reset) whenever the doorbell rings.
>

That's right.
i.MX8 using the general purpose interrupt for peripherals. No shmem needed.
e.g. RTC, Watchdog and ON/OFF interrupt.

BTW, this means the peripheral will use mailbox doorbell channels to handle
Interrupts. Is there such user case in kernel we can refer to?

> 
> > +Optional properties:
> > +-------------------
> > +- clocks :     phandle to the input clock.
> > +- fsl,mu-side-b : Should be set for side B MU.
> >
> Nit: can we call this 'fsl,no-init' ?

Fsl,mu-side-b is more describing HW.

Regards
Dong Aisheng

Re: [PATCH v8 2/4] dt-bindings: mailbox: imx-mu: add generic MU channel support

[Jassi Brar]

On Wed, Aug 1, 2018 at 2:28 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
> Hi Jassi,
>
>> > +               The doorbell channels should be used with shared memory and
>> protocol
>> > +               level acknowledgment if needed.
>> > +
>> I would avoid this. People might get notions that they have to use shmem
>> with doorbell -- a trivial protocol might mean doing some fixed action (like
>> reset) whenever the doorbell rings.
>>
>
> That's right.
> i.MX8 using the general purpose interrupt for peripherals. No shmem needed.
> e.g. RTC, Watchdog and ON/OFF interrupt.
>
> BTW, this means the peripheral will use mailbox doorbell channels to handle
> Interrupts. Is there such user case in kernel we can refer to?
>
I don't find any publicly in kernel.
It should be simple though. Acquire the RX channel, and populate the
rx_callback() with the "interrupt" handler.

[PATCH v8 2/4] dt-bindings: mailbox: imx-mu: add generic MU channel support

[Jassi Brar]

On Wed, Aug 1, 2018 at 2:28 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
> Hi Jassi,
>
>> > +               The doorbell channels should be used with shared memory and
>> protocol
>> > +               level acknowledgment if needed.
>> > +
>> I would avoid this. People might get notions that they have to use shmem
>> with doorbell -- a trivial protocol might mean doing some fixed action (like
>> reset) whenever the doorbell rings.
>>
>
> That's right.
> i.MX8 using the general purpose interrupt for peripherals. No shmem needed.
> e.g. RTC, Watchdog and ON/OFF interrupt.
>
> BTW, this means the peripheral will use mailbox doorbell channels to handle
> Interrupts. Is there such user case in kernel we can refer to?
>
I don't find any publicly in kernel.
It should be simple though. Acquire the RX channel, and populate the
rx_callback() with the "interrupt" handler.

RE: [PATCH v8 2/4] dt-bindings: mailbox: imx-mu: add generic MU channel support

[A.s. Dong]

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> Sent: Wednesday, August 1, 2018 5:58 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; Shawn Guo
> <shawnguo@kernel.org>; Fabio Estevam <fabio.estevam@nxp.com>; Rob
> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>;
> Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>; , Sascha Hauer
> <kernel@pengutronix.de>; , linux-arm-kernel@lists.infradead.org, linux-
> mediatek@lists.infradead.org, srv_heupstream <linux-arm-
> kernel@lists.infradead.org>; Devicetree List <devicetree@vger.kernel.org>;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v8 2/4] dt-bindings: mailbox: imx-mu: add generic MU
> channel support
> 
> On Wed, Aug 1, 2018 at 2:28 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
> > Hi Jassi,
> >
> >> > +               The doorbell channels should be used with shared
> >> > + memory and
> >> protocol
> >> > +               level acknowledgment if needed.
> >> > +
> >> I would avoid this. People might get notions that they have to use
> >> shmem with doorbell -- a trivial protocol might mean doing some fixed
> >> action (like
> >> reset) whenever the doorbell rings.
> >>
> >
> > That's right.
> > i.MX8 using the general purpose interrupt for peripherals. No shmem
> needed.
> > e.g. RTC, Watchdog and ON/OFF interrupt.
> >
> > BTW, this means the peripheral will use mailbox doorbell channels to
> > handle Interrupts. Is there such user case in kernel we can refer to?
> >
> I don't find any publicly in kernel.
> It should be simple though. Acquire the RX channel, and populate the
> rx_callback() with the "interrupt" handler.

Got it.
Thanks

Regards
Dong Aisheng

[PATCH v8 2/4] dt-bindings: mailbox: imx-mu: add generic MU channel support

[A.s. Dong]

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar at gmail.com]
> Sent: Wednesday, August 1, 2018 5:58 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; Shawn Guo
> <shawnguo@kernel.org>; Fabio Estevam <fabio.estevam@nxp.com>; Rob
> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>;
> Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>; , Sascha Hauer
> <kernel@pengutronix.de>; , linux-arm-kernel at lists.infradead.org, linux-
> mediatek at lists.infradead.org, srv_heupstream <linux-arm-
> kernel at lists.infradead.org>; Devicetree List <devicetree@vger.kernel.org>;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v8 2/4] dt-bindings: mailbox: imx-mu: add generic MU
> channel support
> 
> On Wed, Aug 1, 2018 at 2:28 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
> > Hi Jassi,
> >
> >> > +               The doorbell channels should be used with shared
> >> > + memory and
> >> protocol
> >> > +               level acknowledgment if needed.
> >> > +
> >> I would avoid this. People might get notions that they have to use
> >> shmem with doorbell -- a trivial protocol might mean doing some fixed
> >> action (like
> >> reset) whenever the doorbell rings.
> >>
> >
> > That's right.
> > i.MX8 using the general purpose interrupt for peripherals. No shmem
> needed.
> > e.g. RTC, Watchdog and ON/OFF interrupt.
> >
> > BTW, this means the peripheral will use mailbox doorbell channels to
> > handle Interrupts. Is there such user case in kernel we can refer to?
> >
> I don't find any publicly in kernel.
> It should be simple though. Acquire the RX channel, and populate the
> rx_callback() with the "interrupt" handler.

Got it.
Thanks

Regards
Dong Aisheng

Re: [PATCH v8 2/4] dt-bindings: mailbox: imx-mu: add generic MU channel support

[Rob Herring]

On Tue, Jul 31, 2018 at 04:11:44PM +0200, Oleksij Rempel wrote:
> Each MU has four pairs of rx/tx data register with four rx/tx interrupts
> which can also be used as a separate channel.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  .../devicetree/bindings/mailbox/fsl,mu.txt    | 28 +++++++++++++++++--
>  1 file changed, 25 insertions(+), 3 deletions(-)

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

[PATCH v8 2/4] dt-bindings: mailbox: imx-mu: add generic MU channel support

[Rob Herring]

On Tue, Jul 31, 2018 at 04:11:44PM +0200, Oleksij Rempel wrote:
> Each MU has four pairs of rx/tx data register with four rx/tx interrupts
> which can also be used as a separate channel.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  .../devicetree/bindings/mailbox/fsl,mu.txt    | 28 +++++++++++++++++--
>  1 file changed, 25 insertions(+), 3 deletions(-)

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

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

[Oleksij Rempel]

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

Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 arch/arm/boot/dts/imx7s.dtsi | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
index ce85b3ca1a55..3581fc2cc01d 100644
--- a/arch/arm/boot/dts/imx7s.dtsi
+++ b/arch/arm/boot/dts/imx7s.dtsi
@@ -1001,6 +1001,25 @@
 				status = "disabled";
 			};
 
+			mu0a: mailbox@30aa0000 {
+				compatible = "fsl,imx7s-mu", "fsl,imx6sx-mu";
+				reg = <0x30aa0000 0x10000>;
+				interrupts = <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>;
+				clocks = <&clks IMX7D_MU_ROOT_CLK>;
+				#mbox-cells = <2>;
+				status = "disabled";
+			};
+
+			mu0b: mailbox@30ab0000 {
+				compatible = "fsl,imx7s-mu", "fsl,imx6sx-mu";
+				reg = <0x30ab0000 0x10000>;
+				interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>;
+				clocks = <&clks IMX7D_MU_ROOT_CLK>;
+				#mbox-cells = <2>;
+				fsl,mu-side-b;
+				status = "disabled";
+			};
+
 			usbotg1: usb@30b10000 {
 				compatible = "fsl,imx7d-usb", "fsl,imx27-usb";
 				reg = <0x30b10000 0x200>;
-- 
2.18.0

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

[Oleksij Rempel]

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

Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 arch/arm/boot/dts/imx7s.dtsi | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
index ce85b3ca1a55..3581fc2cc01d 100644
--- a/arch/arm/boot/dts/imx7s.dtsi
+++ b/arch/arm/boot/dts/imx7s.dtsi
@@ -1001,6 +1001,25 @@
 				status = "disabled";
 			};
 
+			mu0a: mailbox at 30aa0000 {
+				compatible = "fsl,imx7s-mu", "fsl,imx6sx-mu";
+				reg = <0x30aa0000 0x10000>;
+				interrupts = <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>;
+				clocks = <&clks IMX7D_MU_ROOT_CLK>;
+				#mbox-cells = <2>;
+				status = "disabled";
+			};
+
+			mu0b: mailbox at 30ab0000 {
+				compatible = "fsl,imx7s-mu", "fsl,imx6sx-mu";
+				reg = <0x30ab0000 0x10000>;
+				interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>;
+				clocks = <&clks IMX7D_MU_ROOT_CLK>;
+				#mbox-cells = <2>;
+				fsl,mu-side-b;
+				status = "disabled";
+			};
+
 			usbotg1: usb at 30b10000 {
 				compatible = "fsl,imx7d-usb", "fsl,imx27-usb";
 				reg = <0x30b10000 0x200>;
-- 
2.18.0

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

[Oleksij Rempel]

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

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

Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>
Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/mailbox/Kconfig       |   6 +
 drivers/mailbox/Makefile      |   2 +
 drivers/mailbox/imx-mailbox.c | 371 ++++++++++++++++++++++++++++++++++
 3 files changed, 379 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..6559a4695019
--- /dev/null
+++ b/drivers/mailbox/imx-mailbox.c
@@ -0,0 +1,371 @@
+// 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>
+#include <linux/slab.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_GIPn(x)	BIT(28 + (3 - (x)))
+#define IMX_MU_xSR_RFn(x)	BIT(24 + (3 - (x)))
+#define IMX_MU_xSR_TEn(x)	BIT(20 + (3 - (x)))
+#define IMX_MU_xSR_BRDIP	BIT(9)
+
+/* Control Register */
+#define IMX_MU_xCR		0x24
+/* General Purpose Interrupt Enable */
+#define IMX_MU_xCR_GIEn(x)	BIT(28 + (3 - (x)))
+/* Receive Interrupt Enable */
+#define IMX_MU_xCR_RIEn(x)	BIT(24 + (3 - (x)))
+/* Transmit Interrupt Enable */
+#define IMX_MU_xCR_TIEn(x)	BIT(20 + (3 - (x)))
+/* General Purpose Interrupt Request */
+#define IMX_MU_xCR_GIRn(x)	BIT(16 + (3 - (x)))
+
+#define IMX_MU_CHANS	16
+
+enum imx_mu_chan_type {
+	IMX_MU_TYPE_TX,		/* Tx with FIFO */
+	IMX_MU_TYPE_RX,		/* Rx with FIFO */
+	IMX_MU_TYPE_TXDB,	/* Tx doorbell */
+	IMX_MU_TYPE_RXDB,	/* Rx doorbell */
+};
+
+struct imx_mu_con_priv {
+	unsigned int		idx;
+	char			*irq_desc;
+	enum imx_mu_chan_type	type;
+	struct work_struct	work;
+	struct mbox_chan	*chan;
+};
+
+struct imx_mu_priv {
+	struct device		*dev;
+	void __iomem		*base;
+	spinlock_t		xcr_lock; /* control register lock */
+
+	struct mbox_controller	mbox;
+	struct mbox_chan	mbox_chans[IMX_MU_CHANS];
+
+	struct imx_mu_con_priv  con_priv[IMX_MU_CHANS];
+	struct clk		*clk;
+	int			irq;
+
+	bool			side_b;
+};
+
+static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller *mbox)
+{
+	return container_of(mbox, struct imx_mu_priv, mbox);
+}
+
+static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs)
+{
+	iowrite32(val, priv->base + offs);
+}
+
+static u32 imx_mu_read(struct imx_mu_priv *priv, u32 offs)
+{
+	return ioread32(priv->base + offs);
+}
+
+static u32 imx_mu_xcr_rmw(struct imx_mu_priv *priv, u32 set, u32 clr)
+{
+	unsigned long flags;
+	u32 val;
+
+	spin_lock_irqsave(&priv->xcr_lock, flags);
+	val = imx_mu_read(priv, IMX_MU_xCR);
+	val &= ~clr;
+	val |= set;
+	imx_mu_write(priv, val, IMX_MU_xCR);
+	spin_unlock_irqrestore(&priv->xcr_lock, flags);
+
+	return val;
+}
+
+static void imx_mu_txdb_work(struct work_struct *work)
+{
+	struct imx_mu_con_priv *cp =
+			container_of(work, struct imx_mu_con_priv, work);
+	mbox_chan_txdone(cp->chan, 0);
+}
+
+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);
+
+	switch (cp->type) {
+	case IMX_MU_TYPE_TX:
+		val &= IMX_MU_xSR_TEn(cp->idx) &
+			(ctrl & IMX_MU_xCR_TIEn(cp->idx));
+		break;
+	case IMX_MU_TYPE_RX:
+		val &= IMX_MU_xSR_RFn(cp->idx) &
+			(ctrl & IMX_MU_xCR_RIEn(cp->idx));
+		break;
+	case IMX_MU_TYPE_RXDB:
+		val &= IMX_MU_xSR_GIPn(cp->idx) &
+			(ctrl & IMX_MU_xCR_GIEn(cp->idx));
+		break;
+	default:
+		break;
+	}
+
+	if (!val) {
+		return IRQ_NONE;
+	} else if (IMX_MU_xSR_TEn(cp->idx) == val) {
+		imx_mu_xcr_rmw(priv, 0, IMX_MU_xCR_TIEn(cp->idx));
+		mbox_chan_txdone(chan, 0);
+	} else if (IMX_MU_xSR_RFn(cp->idx) == val) {
+		dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
+		mbox_chan_received_data(chan, (void *)&dat);
+	} else if (IMX_MU_xSR_GIPn(cp->idx) == val) {
+		imx_mu_write(priv, IMX_MU_xSR_GIPn(cp->idx), IMX_MU_xSR);
+		mbox_chan_received_data(chan, NULL);
+	} else {
+		dev_warn_ratelimited(priv->dev, "Not handled interrupt\n");
+		return IRQ_NONE;
+	}
+
+	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;
+
+	/* test if transmit register is empty */
+	return imx_mu_read(priv, IMX_MU_xSR) & 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;
+
+	switch (cp->type) {
+	case IMX_MU_TYPE_TX:
+		if (!imx_mu_last_tx_done(chan))
+			return -EBUSY;
+		imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx));
+		imx_mu_xcr_rmw(priv, IMX_MU_xCR_TIEn(cp->idx), 0);
+		break;
+	case IMX_MU_TYPE_TXDB:
+		imx_mu_xcr_rmw(priv, IMX_MU_xCR_GIRn(cp->idx), 0);
+		schedule_work(&cp->work);
+		break;
+	default:
+		dev_warn_ratelimited(priv->dev, "Send data on wrong channel type: %d\n", cp->type);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int imx_mu_startup(struct mbox_chan *chan)
+{
+	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
+	struct imx_mu_con_priv *cp = chan->con_priv;
+	int ret;
+
+	if (cp->type == IMX_MU_TYPE_TXDB) {
+		/* Tx doorbell don't have ACK support */
+		INIT_WORK(&cp->work, imx_mu_txdb_work);
+		return 0;
+	}
+
+	cp->irq_desc = kasprintf(GFP_KERNEL, "imx_mu_chan[%i-%i]", cp->type,
+				 cp->idx);
+	if (!cp->irq_desc)
+		return -ENOMEM;
+
+	ret = request_irq(priv->irq, imx_mu_isr, IRQF_SHARED, cp->irq_desc,
+			  chan);
+	if (ret) {
+		dev_err(priv->dev,
+			"Unable to acquire IRQ %d\n", priv->irq);
+		return ret;
+	}
+
+	switch (cp->type) {
+		break;
+	case IMX_MU_TYPE_RX:
+		imx_mu_xcr_rmw(priv, IMX_MU_xCR_RIEn(cp->idx), 0);
+		break;
+	case IMX_MU_TYPE_RXDB:
+		imx_mu_xcr_rmw(priv, IMX_MU_xCR_GIEn(cp->idx), 0);
+		break;
+	default:
+		break;
+	}
+
+	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;
+
+	if (IMX_MU_TYPE_TXDB == cp->type)
+		flush_work(&cp->work);
+
+	imx_mu_xcr_rmw(priv, 0,
+		   IMX_MU_xCR_TIEn(cp->idx) | IMX_MU_xCR_RIEn(cp->idx));
+
+	free_irq(priv->irq, chan);
+	kfree(cp->irq_desc);
+}
+
+static const struct mbox_chan_ops imx_mu_ops = {
+	.send_data = imx_mu_send_data,
+	.startup = imx_mu_startup,
+	.shutdown = imx_mu_shutdown,
+};
+
+static struct mbox_chan * imx_mu_xlate(struct mbox_controller *mbox,
+				       const struct of_phandle_args *sp)
+{
+	u32 type, idx, chan;
+
+	if (sp->args_count != 2) {
+		dev_err(mbox->dev, "Invalid argument count %d\n", sp->args_count);
+		return ERR_PTR(-EINVAL);
+	}
+
+	type = sp->args[0]; /* channel type */
+	idx = sp->args[1]; /* index */
+	chan = type * 4 + idx;
+
+	if (chan >= mbox->num_chans) {
+		dev_err(mbox->dev, "Not supported channel number: %d. (type: %d, idx: %d)\n", chan, type, idx);
+		return ERR_PTR(-EINVAL);
+	}
+
+	return &mbox->chans[chan];
+}
+
+static void imx_mu_init_generic(struct imx_mu_priv *priv)
+{
+	if (priv->side_b)
+		return;
+
+	/* Set default MU configuration */
+	imx_mu_write(priv, 0, IMX_MU_xCR);
+}
+
+static int imx_mu_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct resource *iomem;
+	struct imx_mu_priv *priv;
+	unsigned int i;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+
+	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(&pdev->dev, iomem);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	priv->irq = platform_get_irq(pdev, 0);
+	if (priv->irq < 0)
+		return priv->irq;
+
+	priv->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(priv->clk)) {
+		if (PTR_ERR(priv->clk) != -ENOENT)
+			return PTR_ERR(priv->clk);
+
+		priv->clk = NULL;
+	}
+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret) {
+		dev_err(dev, "Failed to enable clock\n");
+		return ret;
+	}
+
+	for (i = 0; i < IMX_MU_CHANS; i++) {
+		struct imx_mu_con_priv *cp = &priv->con_priv[i];
+
+		cp->idx = i % 4;
+		cp->type = (i - cp->idx) >> 2;
+		cp->chan = &priv->mbox_chans[i];
+		priv->mbox_chans[i].con_priv = cp;
+	}
+
+	priv->side_b = of_property_read_bool(np, "fsl,mu-side-b");
+
+	spin_lock_init(&priv->xcr_lock);
+
+	priv->mbox.dev = dev;
+	priv->mbox.ops = &imx_mu_ops;
+	priv->mbox.chans = priv->mbox_chans;
+	priv->mbox.num_chans = IMX_MU_CHANS;
+	priv->mbox.of_xlate = imx_mu_xlate;
+	priv->mbox.txdone_irq = true;
+
+	platform_set_drvdata(pdev, priv);
+
+	imx_mu_init_generic(priv);
+
+	return mbox_controller_register(&priv->mbox);
+}
+
+static int imx_mu_remove(struct platform_device *pdev)
+{
+	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
+
+	mbox_controller_unregister(&priv->mbox);
+	clk_disable_unprepare(priv->clk);
+
+	return 0;
+}
+
+static const struct of_device_id imx_mu_dt_ids[] = {
+	{ .compatible = "fsl,imx6sx-mu" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, imx_mu_dt_ids);
+
+static struct platform_driver imx_mu_driver = {
+	.probe		= imx_mu_probe,
+	.remove		= imx_mu_remove,
+	.driver = {
+		.name	= "imx_mu",
+		.of_match_table = imx_mu_dt_ids,
+	},
+};
+module_platform_driver(imx_mu_driver);
+
+MODULE_AUTHOR("Oleksij Rempel <o.rempel@pengutronix.de>");
+MODULE_DESCRIPTION("Message Unit driver for i.MX");
+MODULE_LICENSE("GPL v2");
-- 
2.18.0

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

[Oleksij Rempel]

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

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

Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>
Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/mailbox/Kconfig       |   6 +
 drivers/mailbox/Makefile      |   2 +
 drivers/mailbox/imx-mailbox.c | 371 ++++++++++++++++++++++++++++++++++
 3 files changed, 379 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..6559a4695019
--- /dev/null
+++ b/drivers/mailbox/imx-mailbox.c
@@ -0,0 +1,371 @@
+// 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>
+#include <linux/slab.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_GIPn(x)	BIT(28 + (3 - (x)))
+#define IMX_MU_xSR_RFn(x)	BIT(24 + (3 - (x)))
+#define IMX_MU_xSR_TEn(x)	BIT(20 + (3 - (x)))
+#define IMX_MU_xSR_BRDIP	BIT(9)
+
+/* Control Register */
+#define IMX_MU_xCR		0x24
+/* General Purpose Interrupt Enable */
+#define IMX_MU_xCR_GIEn(x)	BIT(28 + (3 - (x)))
+/* Receive Interrupt Enable */
+#define IMX_MU_xCR_RIEn(x)	BIT(24 + (3 - (x)))
+/* Transmit Interrupt Enable */
+#define IMX_MU_xCR_TIEn(x)	BIT(20 + (3 - (x)))
+/* General Purpose Interrupt Request */
+#define IMX_MU_xCR_GIRn(x)	BIT(16 + (3 - (x)))
+
+#define IMX_MU_CHANS	16
+
+enum imx_mu_chan_type {
+	IMX_MU_TYPE_TX,		/* Tx with FIFO */
+	IMX_MU_TYPE_RX,		/* Rx with FIFO */
+	IMX_MU_TYPE_TXDB,	/* Tx doorbell */
+	IMX_MU_TYPE_RXDB,	/* Rx doorbell */
+};
+
+struct imx_mu_con_priv {
+	unsigned int		idx;
+	char			*irq_desc;
+	enum imx_mu_chan_type	type;
+	struct work_struct	work;
+	struct mbox_chan	*chan;
+};
+
+struct imx_mu_priv {
+	struct device		*dev;
+	void __iomem		*base;
+	spinlock_t		xcr_lock; /* control register lock */
+
+	struct mbox_controller	mbox;
+	struct mbox_chan	mbox_chans[IMX_MU_CHANS];
+
+	struct imx_mu_con_priv  con_priv[IMX_MU_CHANS];
+	struct clk		*clk;
+	int			irq;
+
+	bool			side_b;
+};
+
+static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller *mbox)
+{
+	return container_of(mbox, struct imx_mu_priv, mbox);
+}
+
+static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs)
+{
+	iowrite32(val, priv->base + offs);
+}
+
+static u32 imx_mu_read(struct imx_mu_priv *priv, u32 offs)
+{
+	return ioread32(priv->base + offs);
+}
+
+static u32 imx_mu_xcr_rmw(struct imx_mu_priv *priv, u32 set, u32 clr)
+{
+	unsigned long flags;
+	u32 val;
+
+	spin_lock_irqsave(&priv->xcr_lock, flags);
+	val = imx_mu_read(priv, IMX_MU_xCR);
+	val &= ~clr;
+	val |= set;
+	imx_mu_write(priv, val, IMX_MU_xCR);
+	spin_unlock_irqrestore(&priv->xcr_lock, flags);
+
+	return val;
+}
+
+static void imx_mu_txdb_work(struct work_struct *work)
+{
+	struct imx_mu_con_priv *cp =
+			container_of(work, struct imx_mu_con_priv, work);
+	mbox_chan_txdone(cp->chan, 0);
+}
+
+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);
+
+	switch (cp->type) {
+	case IMX_MU_TYPE_TX:
+		val &= IMX_MU_xSR_TEn(cp->idx) &
+			(ctrl & IMX_MU_xCR_TIEn(cp->idx));
+		break;
+	case IMX_MU_TYPE_RX:
+		val &= IMX_MU_xSR_RFn(cp->idx) &
+			(ctrl & IMX_MU_xCR_RIEn(cp->idx));
+		break;
+	case IMX_MU_TYPE_RXDB:
+		val &= IMX_MU_xSR_GIPn(cp->idx) &
+			(ctrl & IMX_MU_xCR_GIEn(cp->idx));
+		break;
+	default:
+		break;
+	}
+
+	if (!val) {
+		return IRQ_NONE;
+	} else if (IMX_MU_xSR_TEn(cp->idx) == val) {
+		imx_mu_xcr_rmw(priv, 0, IMX_MU_xCR_TIEn(cp->idx));
+		mbox_chan_txdone(chan, 0);
+	} else if (IMX_MU_xSR_RFn(cp->idx) == val) {
+		dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
+		mbox_chan_received_data(chan, (void *)&dat);
+	} else if (IMX_MU_xSR_GIPn(cp->idx) == val) {
+		imx_mu_write(priv, IMX_MU_xSR_GIPn(cp->idx), IMX_MU_xSR);
+		mbox_chan_received_data(chan, NULL);
+	} else {
+		dev_warn_ratelimited(priv->dev, "Not handled interrupt\n");
+		return IRQ_NONE;
+	}
+
+	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;
+
+	/* test if transmit register is empty */
+	return imx_mu_read(priv, IMX_MU_xSR) & 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;
+
+	switch (cp->type) {
+	case IMX_MU_TYPE_TX:
+		if (!imx_mu_last_tx_done(chan))
+			return -EBUSY;
+		imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx));
+		imx_mu_xcr_rmw(priv, IMX_MU_xCR_TIEn(cp->idx), 0);
+		break;
+	case IMX_MU_TYPE_TXDB:
+		imx_mu_xcr_rmw(priv, IMX_MU_xCR_GIRn(cp->idx), 0);
+		schedule_work(&cp->work);
+		break;
+	default:
+		dev_warn_ratelimited(priv->dev, "Send data on wrong channel type: %d\n", cp->type);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int imx_mu_startup(struct mbox_chan *chan)
+{
+	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
+	struct imx_mu_con_priv *cp = chan->con_priv;
+	int ret;
+
+	if (cp->type == IMX_MU_TYPE_TXDB) {
+		/* Tx doorbell don't have ACK support */
+		INIT_WORK(&cp->work, imx_mu_txdb_work);
+		return 0;
+	}
+
+	cp->irq_desc = kasprintf(GFP_KERNEL, "imx_mu_chan[%i-%i]", cp->type,
+				 cp->idx);
+	if (!cp->irq_desc)
+		return -ENOMEM;
+
+	ret = request_irq(priv->irq, imx_mu_isr, IRQF_SHARED, cp->irq_desc,
+			  chan);
+	if (ret) {
+		dev_err(priv->dev,
+			"Unable to acquire IRQ %d\n", priv->irq);
+		return ret;
+	}
+
+	switch (cp->type) {
+		break;
+	case IMX_MU_TYPE_RX:
+		imx_mu_xcr_rmw(priv, IMX_MU_xCR_RIEn(cp->idx), 0);
+		break;
+	case IMX_MU_TYPE_RXDB:
+		imx_mu_xcr_rmw(priv, IMX_MU_xCR_GIEn(cp->idx), 0);
+		break;
+	default:
+		break;
+	}
+
+	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;
+
+	if (IMX_MU_TYPE_TXDB == cp->type)
+		flush_work(&cp->work);
+
+	imx_mu_xcr_rmw(priv, 0,
+		   IMX_MU_xCR_TIEn(cp->idx) | IMX_MU_xCR_RIEn(cp->idx));
+
+	free_irq(priv->irq, chan);
+	kfree(cp->irq_desc);
+}
+
+static const struct mbox_chan_ops imx_mu_ops = {
+	.send_data = imx_mu_send_data,
+	.startup = imx_mu_startup,
+	.shutdown = imx_mu_shutdown,
+};
+
+static struct mbox_chan * imx_mu_xlate(struct mbox_controller *mbox,
+				       const struct of_phandle_args *sp)
+{
+	u32 type, idx, chan;
+
+	if (sp->args_count != 2) {
+		dev_err(mbox->dev, "Invalid argument count %d\n", sp->args_count);
+		return ERR_PTR(-EINVAL);
+	}
+
+	type = sp->args[0]; /* channel type */
+	idx = sp->args[1]; /* index */
+	chan = type * 4 + idx;
+
+	if (chan >= mbox->num_chans) {
+		dev_err(mbox->dev, "Not supported channel number: %d. (type: %d, idx: %d)\n", chan, type, idx);
+		return ERR_PTR(-EINVAL);
+	}
+
+	return &mbox->chans[chan];
+}
+
+static void imx_mu_init_generic(struct imx_mu_priv *priv)
+{
+	if (priv->side_b)
+		return;
+
+	/* Set default MU configuration */
+	imx_mu_write(priv, 0, IMX_MU_xCR);
+}
+
+static int imx_mu_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct resource *iomem;
+	struct imx_mu_priv *priv;
+	unsigned int i;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+
+	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(&pdev->dev, iomem);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	priv->irq = platform_get_irq(pdev, 0);
+	if (priv->irq < 0)
+		return priv->irq;
+
+	priv->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(priv->clk)) {
+		if (PTR_ERR(priv->clk) != -ENOENT)
+			return PTR_ERR(priv->clk);
+
+		priv->clk = NULL;
+	}
+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret) {
+		dev_err(dev, "Failed to enable clock\n");
+		return ret;
+	}
+
+	for (i = 0; i < IMX_MU_CHANS; i++) {
+		struct imx_mu_con_priv *cp = &priv->con_priv[i];
+
+		cp->idx = i % 4;
+		cp->type = (i - cp->idx) >> 2;
+		cp->chan = &priv->mbox_chans[i];
+		priv->mbox_chans[i].con_priv = cp;
+	}
+
+	priv->side_b = of_property_read_bool(np, "fsl,mu-side-b");
+
+	spin_lock_init(&priv->xcr_lock);
+
+	priv->mbox.dev = dev;
+	priv->mbox.ops = &imx_mu_ops;
+	priv->mbox.chans = priv->mbox_chans;
+	priv->mbox.num_chans = IMX_MU_CHANS;
+	priv->mbox.of_xlate = imx_mu_xlate;
+	priv->mbox.txdone_irq = true;
+
+	platform_set_drvdata(pdev, priv);
+
+	imx_mu_init_generic(priv);
+
+	return mbox_controller_register(&priv->mbox);
+}
+
+static int imx_mu_remove(struct platform_device *pdev)
+{
+	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
+
+	mbox_controller_unregister(&priv->mbox);
+	clk_disable_unprepare(priv->clk);
+
+	return 0;
+}
+
+static const struct of_device_id imx_mu_dt_ids[] = {
+	{ .compatible = "fsl,imx6sx-mu" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, imx_mu_dt_ids);
+
+static struct platform_driver imx_mu_driver = {
+	.probe		= imx_mu_probe,
+	.remove		= imx_mu_remove,
+	.driver = {
+		.name	= "imx_mu",
+		.of_match_table = imx_mu_dt_ids,
+	},
+};
+module_platform_driver(imx_mu_driver);
+
+MODULE_AUTHOR("Oleksij Rempel <o.rempel@pengutronix.de>");
+MODULE_DESCRIPTION("Message Unit driver for i.MX");
+MODULE_LICENSE("GPL v2");
-- 
2.18.0

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

[Jassi Brar]

Hi Oleksij,

 A fast and fine turnaround! Mostly nits and a suggestion follows ...

On Tue, Jul 31, 2018 at 7:41 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> The Mailbox controller is able to send messages (up to 4 32 bit words)
> between the endpoints.
>
> This driver was tested using the mailbox-test driver sending messages
> between the Cortex-A7 and the Cortex-M4.
>
Do we need something MU specific here?
I mean MU doesn't really care if the endpoints are CA, CR or CM. So,
maybe just the two-line ritual intro to MU?


> +enum imx_mu_chan_type {
> +       IMX_MU_TYPE_TX,         /* Tx with FIFO */
> +       IMX_MU_TYPE_RX,         /* Rx with FIFO */
>
nit: its a register, not fifo


> +
> +static void imx_mu_txdb_work(struct work_struct *work)
> +{
> +       struct imx_mu_con_priv *cp =
> +                       container_of(work, struct imx_mu_con_priv, work);
> +       mbox_chan_txdone(cp->chan, 0);
> +}
>
The api assumes a controller have same type of channels. So we are
having to do this here.
I am not sure, but would declaring two mailbox controllers (one with
doorbells and the other data channels) work?
If not, at least we could use a tasklet instead of a work queue?


> +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;
> +
> +       /* test if transmit register is empty */
> +       return imx_mu_read(priv, IMX_MU_xSR) & 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;
> +
> +       switch (cp->type) {
> +       case IMX_MU_TYPE_TX:
> +               if (!imx_mu_last_tx_done(chan))
> +                       return -EBUSY;
>
This test should be moved to imx_mu_startup().
The api won't ever call if last tx was not done, and so it doesn't
know how to recover from send_data() failure.


> +
> +static int imx_mu_startup(struct mbox_chan *chan)
> +{
> +       struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +       struct imx_mu_con_priv *cp = chan->con_priv;
> +       int ret;
> +
> +       if (cp->type == IMX_MU_TYPE_TXDB) {
> +               /* Tx doorbell don't have ACK support */
> +               INIT_WORK(&cp->work, imx_mu_txdb_work);
> +               return 0;
> +       }
> +
> +       cp->irq_desc = kasprintf(GFP_KERNEL, "imx_mu_chan[%i-%i]", cp->type,
> +                                cp->idx);
> +       if (!cp->irq_desc)
> +               return -ENOMEM;
>
Probably you won't do but still .... name of irq is but one channel
property. So you could set it in probe() and avoid creating another
situation when startup could fail.


> +       ret = request_irq(priv->irq, imx_mu_isr, IRQF_SHARED, cp->irq_desc,
> +                         chan);
> +       if (ret) {
> +               dev_err(priv->dev,
> +                       "Unable to acquire IRQ %d\n", priv->irq);
> +               return ret;
> +       }
> +
> +       switch (cp->type) {
> +               break;
>
Is this intentional?


> +
> +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;
> +
> +       if (IMX_MU_TYPE_TXDB == cp->type)
>
nit: usually we do (cp->type == IMX_MU_TYPE_TXDB)


Thanks

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

[Jassi Brar]

Hi Oleksij,

 A fast and fine turnaround! Mostly nits and a suggestion follows ...

On Tue, Jul 31, 2018 at 7:41 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> The Mailbox controller is able to send messages (up to 4 32 bit words)
> between the endpoints.
>
> This driver was tested using the mailbox-test driver sending messages
> between the Cortex-A7 and the Cortex-M4.
>
Do we need something MU specific here?
I mean MU doesn't really care if the endpoints are CA, CR or CM. So,
maybe just the two-line ritual intro to MU?


> +enum imx_mu_chan_type {
> +       IMX_MU_TYPE_TX,         /* Tx with FIFO */
> +       IMX_MU_TYPE_RX,         /* Rx with FIFO */
>
nit: its a register, not fifo


> +
> +static void imx_mu_txdb_work(struct work_struct *work)
> +{
> +       struct imx_mu_con_priv *cp =
> +                       container_of(work, struct imx_mu_con_priv, work);
> +       mbox_chan_txdone(cp->chan, 0);
> +}
>
The api assumes a controller have same type of channels. So we are
having to do this here.
I am not sure, but would declaring two mailbox controllers (one with
doorbells and the other data channels) work?
If not, at least we could use a tasklet instead of a work queue?


> +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;
> +
> +       /* test if transmit register is empty */
> +       return imx_mu_read(priv, IMX_MU_xSR) & 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;
> +
> +       switch (cp->type) {
> +       case IMX_MU_TYPE_TX:
> +               if (!imx_mu_last_tx_done(chan))
> +                       return -EBUSY;
>
This test should be moved to imx_mu_startup().
The api won't ever call if last tx was not done, and so it doesn't
know how to recover from send_data() failure.


> +
> +static int imx_mu_startup(struct mbox_chan *chan)
> +{
> +       struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +       struct imx_mu_con_priv *cp = chan->con_priv;
> +       int ret;
> +
> +       if (cp->type == IMX_MU_TYPE_TXDB) {
> +               /* Tx doorbell don't have ACK support */
> +               INIT_WORK(&cp->work, imx_mu_txdb_work);
> +               return 0;
> +       }
> +
> +       cp->irq_desc = kasprintf(GFP_KERNEL, "imx_mu_chan[%i-%i]", cp->type,
> +                                cp->idx);
> +       if (!cp->irq_desc)
> +               return -ENOMEM;
>
Probably you won't do but still .... name of irq is but one channel
property. So you could set it in probe() and avoid creating another
situation when startup could fail.


> +       ret = request_irq(priv->irq, imx_mu_isr, IRQF_SHARED, cp->irq_desc,
> +                         chan);
> +       if (ret) {
> +               dev_err(priv->dev,
> +                       "Unable to acquire IRQ %d\n", priv->irq);
> +               return ret;
> +       }
> +
> +       switch (cp->type) {
> +               break;
>
Is this intentional?


> +
> +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;
> +
> +       if (IMX_MU_TYPE_TXDB == cp->type)
>
nit: usually we do (cp->type == IMX_MU_TYPE_TXDB)


Thanks

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

[Oleksij Rempel]

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

On Tue, Jul 31, 2018 at 09:51:37PM +0530, Jassi Brar wrote:
> Hi Oleksij,
> 
>  A fast and fine turnaround! Mostly nits and a suggestion follows ...
> 
> On Tue, Jul 31, 2018 at 7:41 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> > The Mailbox controller is able to send messages (up to 4 32 bit words)
> > between the endpoints.
> >
> > This driver was tested using the mailbox-test driver sending messages
> > between the Cortex-A7 and the Cortex-M4.
> >
> Do we need something MU specific here?
> I mean MU doesn't really care if the endpoints are CA, CR or CM. So,
> maybe just the two-line ritual intro to MU?

Till now, every additional word about MU added more disagreement in this
topic. I prefer the reduced variant.

> 
> > +enum imx_mu_chan_type {
> > +       IMX_MU_TYPE_TX,         /* Tx with FIFO */
> > +       IMX_MU_TYPE_RX,         /* Rx with FIFO */
> >
> nit: its a register, not fifo

ok

> 
> > +
> > +static void imx_mu_txdb_work(struct work_struct *work)
> > +{
> > +       struct imx_mu_con_priv *cp =
> > +                       container_of(work, struct imx_mu_con_priv, work);
> > +       mbox_chan_txdone(cp->chan, 0);
> > +}
> >
> The api assumes a controller have same type of channels. So we are
> having to do this here.
> I am not sure, but would declaring two mailbox controllers (one with
> doorbells and the other data channels) work?

I was thinking about it and decided that this pain was not worth it.

> If not, at least we could use a tasklet instead of a work queue?

ok. probably it is better to fix the mailbox framework.... may be not right
now.

> 
> > +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;
> > +
> > +       /* test if transmit register is empty */
> > +       return imx_mu_read(priv, IMX_MU_xSR) & 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;
> > +
> > +       switch (cp->type) {
> > +       case IMX_MU_TYPE_TX:
> > +               if (!imx_mu_last_tx_done(chan))
> > +                       return -EBUSY;
> >
> This test should be moved to imx_mu_startup().
> The api won't ever call if last tx was not done, and so it doesn't
> know how to recover from send_data() failure.

ok.

> 
> > +
> > +static int imx_mu_startup(struct mbox_chan *chan)
> > +{
> > +       struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > +       struct imx_mu_con_priv *cp = chan->con_priv;
> > +       int ret;
> > +
> > +       if (cp->type == IMX_MU_TYPE_TXDB) {
> > +               /* Tx doorbell don't have ACK support */
> > +               INIT_WORK(&cp->work, imx_mu_txdb_work);
> > +               return 0;
> > +       }
> > +
> > +       cp->irq_desc = kasprintf(GFP_KERNEL, "imx_mu_chan[%i-%i]", cp->type,
> > +                                cp->idx);
> > +       if (!cp->irq_desc)
> > +               return -ENOMEM;
> >
> Probably you won't do but still .... name of irq is but one channel
> property. So you could set it in probe() and avoid creating another
> situation when startup could fail.

You mean, fail on probe with no mem is better then fail on startup? Why?

> 
> > +       ret = request_irq(priv->irq, imx_mu_isr, IRQF_SHARED, cp->irq_desc,
> > +                         chan);
> > +       if (ret) {
> > +               dev_err(priv->dev,
> > +                       "Unable to acquire IRQ %d\n", priv->irq);
> > +               return ret;
> > +       }
> > +
> > +       switch (cp->type) {
> > +               break;
> >
> Is this intentional?

no. thx.

> 
> > +
> > +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;
> > +
> > +       if (IMX_MU_TYPE_TXDB == cp->type)
> >
> nit: usually we do (cp->type == IMX_MU_TYPE_TXDB)

why? I do it with simple reason:
this error will be detected by compiler
if (IMX_MU_TYPE_TXDB = cp->type)

this error will silently fail
if (cp->type = IMX_MU_TYPE_TXDB)

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

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

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

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

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

[Oleksij Rempel]

On Tue, Jul 31, 2018 at 09:51:37PM +0530, Jassi Brar wrote:
> Hi Oleksij,
> 
>  A fast and fine turnaround! Mostly nits and a suggestion follows ...
> 
> On Tue, Jul 31, 2018 at 7:41 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> > The Mailbox controller is able to send messages (up to 4 32 bit words)
> > between the endpoints.
> >
> > This driver was tested using the mailbox-test driver sending messages
> > between the Cortex-A7 and the Cortex-M4.
> >
> Do we need something MU specific here?
> I mean MU doesn't really care if the endpoints are CA, CR or CM. So,
> maybe just the two-line ritual intro to MU?

Till now, every additional word about MU added more disagreement in this
topic. I prefer the reduced variant.

> 
> > +enum imx_mu_chan_type {
> > +       IMX_MU_TYPE_TX,         /* Tx with FIFO */
> > +       IMX_MU_TYPE_RX,         /* Rx with FIFO */
> >
> nit: its a register, not fifo

ok

> 
> > +
> > +static void imx_mu_txdb_work(struct work_struct *work)
> > +{
> > +       struct imx_mu_con_priv *cp =
> > +                       container_of(work, struct imx_mu_con_priv, work);
> > +       mbox_chan_txdone(cp->chan, 0);
> > +}
> >
> The api assumes a controller have same type of channels. So we are
> having to do this here.
> I am not sure, but would declaring two mailbox controllers (one with
> doorbells and the other data channels) work?

I was thinking about it and decided that this pain was not worth it.

> If not, at least we could use a tasklet instead of a work queue?

ok. probably it is better to fix the mailbox framework.... may be not right
now.

> 
> > +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;
> > +
> > +       /* test if transmit register is empty */
> > +       return imx_mu_read(priv, IMX_MU_xSR) & 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;
> > +
> > +       switch (cp->type) {
> > +       case IMX_MU_TYPE_TX:
> > +               if (!imx_mu_last_tx_done(chan))
> > +                       return -EBUSY;
> >
> This test should be moved to imx_mu_startup().
> The api won't ever call if last tx was not done, and so it doesn't
> know how to recover from send_data() failure.

ok.

> 
> > +
> > +static int imx_mu_startup(struct mbox_chan *chan)
> > +{
> > +       struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > +       struct imx_mu_con_priv *cp = chan->con_priv;
> > +       int ret;
> > +
> > +       if (cp->type == IMX_MU_TYPE_TXDB) {
> > +               /* Tx doorbell don't have ACK support */
> > +               INIT_WORK(&cp->work, imx_mu_txdb_work);
> > +               return 0;
> > +       }
> > +
> > +       cp->irq_desc = kasprintf(GFP_KERNEL, "imx_mu_chan[%i-%i]", cp->type,
> > +                                cp->idx);
> > +       if (!cp->irq_desc)
> > +               return -ENOMEM;
> >
> Probably you won't do but still .... name of irq is but one channel
> property. So you could set it in probe() and avoid creating another
> situation when startup could fail.

You mean, fail on probe with no mem is better then fail on startup? Why?

> 
> > +       ret = request_irq(priv->irq, imx_mu_isr, IRQF_SHARED, cp->irq_desc,
> > +                         chan);
> > +       if (ret) {
> > +               dev_err(priv->dev,
> > +                       "Unable to acquire IRQ %d\n", priv->irq);
> > +               return ret;
> > +       }
> > +
> > +       switch (cp->type) {
> > +               break;
> >
> Is this intentional?

no. thx.

> 
> > +
> > +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;
> > +
> > +       if (IMX_MU_TYPE_TXDB == cp->type)
> >
> nit: usually we do (cp->type == IMX_MU_TYPE_TXDB)

why? I do it with simple reason:
this error will be detected by compiler
if (IMX_MU_TYPE_TXDB = cp->type)

this error will silently fail
if (cp->type = IMX_MU_TYPE_TXDB)

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180801/3154343e/attachment.sig>

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

[Jassi Brar]

On Wed, Aug 1, 2018 at 11:01 AM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> On Tue, Jul 31, 2018 at 09:51:37PM +0530, Jassi Brar wrote:

>> > The Mailbox controller is able to send messages (up to 4 32 bit words)
>> > between the endpoints.
>> >
>> > This driver was tested using the mailbox-test driver sending messages
>> > between the Cortex-A7 and the Cortex-M4.
>> >
>> Do we need something MU specific here?
>> I mean MU doesn't really care if the endpoints are CA, CR or CM. So,
>> maybe just the two-line ritual intro to MU?
>
> Till now, every additional word about MU added more disagreement in this
> topic. I prefer the reduced variant.
>
OK, but we don't usually specify "tested on ..." but leave details of
the controller, in the new driver commit.


>> > +
>> > +static void imx_mu_txdb_work(struct work_struct *work)
>> > +{
>> > +       struct imx_mu_con_priv *cp =
>> > +                       container_of(work, struct imx_mu_con_priv, work);
>> > +       mbox_chan_txdone(cp->chan, 0);
>> > +}
>> >
>> The api assumes a controller have same type of channels. So we are
>> having to do this here.
>> I am not sure, but would declaring two mailbox controllers (one with
>> doorbells and the other data channels) work?
>
> I was thinking about it and decided that this pain was not worth it.
>
OK.

But just fyi, a common use of doorbell is to hint the master to take
actions like reset, suspend or expect some periodic broadcast or some
hotpath where we wouldn't want to sleep.
Another usecase is data transfer (where speed matters) via
doorbell+shmem, having to sleep after each packet is going to kill
performance.


>> If not, at least we could use a tasklet instead of a work queue?
>
> ok. probably it is better to fix the mailbox framework.... may be not right
> now.
>
No, the framework is fine. MU is the only controller that has two
types of channels.


>> > +
>> > +static int imx_mu_startup(struct mbox_chan *chan)
>> > +{
>> > +       struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>> > +       struct imx_mu_con_priv *cp = chan->con_priv;
>> > +       int ret;
>> > +
>> > +       if (cp->type == IMX_MU_TYPE_TXDB) {
>> > +               /* Tx doorbell don't have ACK support */
>> > +               INIT_WORK(&cp->work, imx_mu_txdb_work);
>> > +               return 0;
>> > +       }
>> > +
>> > +       cp->irq_desc = kasprintf(GFP_KERNEL, "imx_mu_chan[%i-%i]", cp->type,
>> > +                                cp->idx);
>> > +       if (!cp->irq_desc)
>> > +               return -ENOMEM;
>> >
>> Probably you won't do but still .... name of irq is but one channel
>> property. So you could set it in probe() and avoid creating another
>> situation when startup could fail.
>
> You mean, fail on probe with no mem is better then fail on startup? Why?
>
Fail early :)
But if you just have irq_name[8] in the channel struct, it won't even
fail in probe. And is more consistent with other channel
initialisations..... you see name of irq doesn't change across channel
requests.

But OK, I am running out of energy and interest.


>> > +
>> > +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;
>> > +
>> > +       if (IMX_MU_TYPE_TXDB == cp->type)
>> >
>> nit: usually we do (cp->type == IMX_MU_TYPE_TXDB)
>
> why? I do it with simple reason:
> this error will be detected by compiler
> if (IMX_MU_TYPE_TXDB = cp->type)
>
> this error will silently fail
> if (cp->type = IMX_MU_TYPE_TXDB)
>
Because it reads like
     if (2 is equal to the variable)
instead of
    if (the variable is equal to 2)

Most developers prefer latter. In fact you too, in startup()
So please atleast make it consistent either way.

Cheers!

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

[Jassi Brar]

On Wed, Aug 1, 2018 at 11:01 AM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> On Tue, Jul 31, 2018 at 09:51:37PM +0530, Jassi Brar wrote:

>> > The Mailbox controller is able to send messages (up to 4 32 bit words)
>> > between the endpoints.
>> >
>> > This driver was tested using the mailbox-test driver sending messages
>> > between the Cortex-A7 and the Cortex-M4.
>> >
>> Do we need something MU specific here?
>> I mean MU doesn't really care if the endpoints are CA, CR or CM. So,
>> maybe just the two-line ritual intro to MU?
>
> Till now, every additional word about MU added more disagreement in this
> topic. I prefer the reduced variant.
>
OK, but we don't usually specify "tested on ..." but leave details of
the controller, in the new driver commit.


>> > +
>> > +static void imx_mu_txdb_work(struct work_struct *work)
>> > +{
>> > +       struct imx_mu_con_priv *cp =
>> > +                       container_of(work, struct imx_mu_con_priv, work);
>> > +       mbox_chan_txdone(cp->chan, 0);
>> > +}
>> >
>> The api assumes a controller have same type of channels. So we are
>> having to do this here.
>> I am not sure, but would declaring two mailbox controllers (one with
>> doorbells and the other data channels) work?
>
> I was thinking about it and decided that this pain was not worth it.
>
OK.

But just fyi, a common use of doorbell is to hint the master to take
actions like reset, suspend or expect some periodic broadcast or some
hotpath where we wouldn't want to sleep.
Another usecase is data transfer (where speed matters) via
doorbell+shmem, having to sleep after each packet is going to kill
performance.


>> If not, at least we could use a tasklet instead of a work queue?
>
> ok. probably it is better to fix the mailbox framework.... may be not right
> now.
>
No, the framework is fine. MU is the only controller that has two
types of channels.


>> > +
>> > +static int imx_mu_startup(struct mbox_chan *chan)
>> > +{
>> > +       struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>> > +       struct imx_mu_con_priv *cp = chan->con_priv;
>> > +       int ret;
>> > +
>> > +       if (cp->type == IMX_MU_TYPE_TXDB) {
>> > +               /* Tx doorbell don't have ACK support */
>> > +               INIT_WORK(&cp->work, imx_mu_txdb_work);
>> > +               return 0;
>> > +       }
>> > +
>> > +       cp->irq_desc = kasprintf(GFP_KERNEL, "imx_mu_chan[%i-%i]", cp->type,
>> > +                                cp->idx);
>> > +       if (!cp->irq_desc)
>> > +               return -ENOMEM;
>> >
>> Probably you won't do but still .... name of irq is but one channel
>> property. So you could set it in probe() and avoid creating another
>> situation when startup could fail.
>
> You mean, fail on probe with no mem is better then fail on startup? Why?
>
Fail early :)
But if you just have irq_name[8] in the channel struct, it won't even
fail in probe. And is more consistent with other channel
initialisations..... you see name of irq doesn't change across channel
requests.

But OK, I am running out of energy and interest.


>> > +
>> > +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;
>> > +
>> > +       if (IMX_MU_TYPE_TXDB == cp->type)
>> >
>> nit: usually we do (cp->type == IMX_MU_TYPE_TXDB)
>
> why? I do it with simple reason:
> this error will be detected by compiler
> if (IMX_MU_TYPE_TXDB = cp->type)
>
> this error will silently fail
> if (cp->type = IMX_MU_TYPE_TXDB)
>
Because it reads like
     if (2 is equal to the variable)
instead of
    if (the variable is equal to 2)

Most developers prefer latter. In fact you too, in startup()
So please atleast make it consistent either way.

Cheers!

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

[A.s. Dong]

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]

[...]

> > +
> > +static void imx_mu_txdb_work(struct work_struct *work) {
> > +       struct imx_mu_con_priv *cp =
> > +                       container_of(work, struct imx_mu_con_priv, work);
> > +       mbox_chan_txdone(cp->chan, 0); }
> >
> The api assumes a controller have same type of channels. So we are having
> to do this here.
> I am not sure, but would declaring two mailbox controllers (one with
> doorbells and the other data channels) work?
> If not, at least we could use a tasklet instead of a work queue?
> 

I'm also a bit curious what the meaning of calling mbox_chan_txdone
for a doorbell mailbox?

Is there any recommended way to use it?

Regards
Dong Aisheng

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

[A.s. Dong]

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar at gmail.com]

[...]

> > +
> > +static void imx_mu_txdb_work(struct work_struct *work) {
> > +       struct imx_mu_con_priv *cp =
> > +                       container_of(work, struct imx_mu_con_priv, work);
> > +       mbox_chan_txdone(cp->chan, 0); }
> >
> The api assumes a controller have same type of channels. So we are having
> to do this here.
> I am not sure, but would declaring two mailbox controllers (one with
> doorbells and the other data channels) work?
> If not, at least we could use a tasklet instead of a work queue?
> 

I'm also a bit curious what the meaning of calling mbox_chan_txdone
for a doorbell mailbox?

Is there any recommended way to use it?

Regards
Dong Aisheng

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

[Jassi Brar]

On Wed, Aug 1, 2018 at 1:14 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
>> -----Original Message-----
>> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
>
> [...]
>
>> > +
>> > +static void imx_mu_txdb_work(struct work_struct *work) {
>> > +       struct imx_mu_con_priv *cp =
>> > +                       container_of(work, struct imx_mu_con_priv, work);
>> > +       mbox_chan_txdone(cp->chan, 0); }
>> >
>> The api assumes a controller have same type of channels. So we are having
>> to do this here.
>> I am not sure, but would declaring two mailbox controllers (one with
>> doorbells and the other data channels) work?
>> If not, at least we could use a tasklet instead of a work queue?
>>
>
> I'm also a bit curious what the meaning of calling mbox_chan_txdone
> for a doorbell mailbox?
>
> Is there any recommended way to use it?
>
The framework submits a message (a signal with or without data) for
controller to transmit. Obviously, it has to know when the transfer
completes (so that it can submit the next message).

Depending on the controller h/w design, there can be three ways.

a) Controller has some irq that fires when the signal is consumed by
the other end. Like MU does. So upon TX-IRQ, the controller driver
calls mbox_chan_txdone() to tell the framework that the last submitted
transfer has completed. This is indicated by setting only  txdone_irq
= true

b) Controller does not have any tx-irq, but it can read some register
to know the status. That is it has to be polled. The framework calls
last_tx_done() periodically to check the status. This is indicated by
setting only  txdone_poll = true.

c) Controller has neither irq, nor any status register. This is
indicated by setting only  txdone_irq = false and  txdone_poll =
false.   In this case, the protocol/client has to tell the framework
(using some protocol level indicator like some ack packet reception)
when the last submitted message was transferred, by calling
mbox_client_txdone()

A 'doorbell' is  (c) type channel -- just an irq raised at other end
without any ack irq or status bit to check. So, the client would
simply do
                mbox_send_message(chan, msg);
                mbox_client_txdone(chan, 0);

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

[Jassi Brar]

On Wed, Aug 1, 2018 at 1:14 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
>> -----Original Message-----
>> From: Jassi Brar [mailto:jassisinghbrar at gmail.com]
>
> [...]
>
>> > +
>> > +static void imx_mu_txdb_work(struct work_struct *work) {
>> > +       struct imx_mu_con_priv *cp =
>> > +                       container_of(work, struct imx_mu_con_priv, work);
>> > +       mbox_chan_txdone(cp->chan, 0); }
>> >
>> The api assumes a controller have same type of channels. So we are having
>> to do this here.
>> I am not sure, but would declaring two mailbox controllers (one with
>> doorbells and the other data channels) work?
>> If not, at least we could use a tasklet instead of a work queue?
>>
>
> I'm also a bit curious what the meaning of calling mbox_chan_txdone
> for a doorbell mailbox?
>
> Is there any recommended way to use it?
>
The framework submits a message (a signal with or without data) for
controller to transmit. Obviously, it has to know when the transfer
completes (so that it can submit the next message).

Depending on the controller h/w design, there can be three ways.

a) Controller has some irq that fires when the signal is consumed by
the other end. Like MU does. So upon TX-IRQ, the controller driver
calls mbox_chan_txdone() to tell the framework that the last submitted
transfer has completed. This is indicated by setting only  txdone_irq
= true

b) Controller does not have any tx-irq, but it can read some register
to know the status. That is it has to be polled. The framework calls
last_tx_done() periodically to check the status. This is indicated by
setting only  txdone_poll = true.

c) Controller has neither irq, nor any status register. This is
indicated by setting only  txdone_irq = false and  txdone_poll =
false.   In this case, the protocol/client has to tell the framework
(using some protocol level indicator like some ack packet reception)
when the last submitted message was transferred, by calling
mbox_client_txdone()

A 'doorbell' is  (c) type channel -- just an irq raised at other end
without any ack irq or status bit to check. So, the client would
simply do
                mbox_send_message(chan, msg);
                mbox_client_txdone(chan, 0);

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

[A.s. Dong]

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> Sent: Wednesday, August 1, 2018 5:45 PM

[...]

> >> > +
> >> > +static void imx_mu_txdb_work(struct work_struct *work) {
> >> > +       struct imx_mu_con_priv *cp =
> >> > +                       container_of(work, struct imx_mu_con_priv, work);
> >> > +       mbox_chan_txdone(cp->chan, 0); }
> >> >
> >> The api assumes a controller have same type of channels. So we are
> >> having to do this here.
> >> I am not sure, but would declaring two mailbox controllers (one with
> >> doorbells and the other data channels) work?
> >> If not, at least we could use a tasklet instead of a work queue?
> >>
> >
> > I'm also a bit curious what the meaning of calling mbox_chan_txdone
> > for a doorbell mailbox?
> >
> > Is there any recommended way to use it?
> >
> The framework submits a message (a signal with or without data) for
> controller to transmit. Obviously, it has to know when the transfer completes
> (so that it can submit the next message).
> 
> Depending on the controller h/w design, there can be three ways.
> 
> a) Controller has some irq that fires when the signal is consumed by the
> other end. Like MU does. So upon TX-IRQ, the controller driver calls
> mbox_chan_txdone() to tell the framework that the last submitted transfer
> has completed. This is indicated by setting only  txdone_irq = true
> 
> b) Controller does not have any tx-irq, but it can read some register to know
> the status. That is it has to be polled. The framework calls
> last_tx_done() periodically to check the status. This is indicated by setting
> only  txdone_poll = true.
> 
> c) Controller has neither irq, nor any status register. This is indicated by
> setting only  txdone_irq = false and  txdone_poll =
> false.   In this case, the protocol/client has to tell the framework
> (using some protocol level indicator like some ack packet reception) when
> the last submitted message was transferred, by calling
> mbox_client_txdone()
> 

Thanks for a detailed and clear explanation.

> A 'doorbell' is  (c) type channel -- just an irq raised at other end without any
> ack irq or status bit to check. So, the client would simply do
>                 mbox_send_message(chan, msg);
>                 mbox_client_txdone(chan, 0);

Just a bit wondering whether we need client to call mbox_client_txdone()
here as it actually does not know whether it's done. Not sure if it's better to
handle them in mailbox controller driver or framework.
(Oleksij seems do it in the controller driver in this patch)

And see mbox_client_txdone() definition, it seems it's only used by TXDONE_BY_ACK
case. Is the doorbell mailbox without IRQ or ACK support still suitable to call it to
notify the framework the transfer is done?

void mbox_client_txdone(struct mbox_chan *chan, int r)                                                                                                                                                              
{
        if (unlikely(!(chan->txdone_method & TXDONE_BY_ACK))) {
                dev_err(chan->mbox->dev, "Client can't run the TX ticker\n");                                                                                                                                       
                return;
        }

        tx_tick(chan, r);
}

For i.MX MU case, the controller is defined as TXDONE_BY_IRQ for all channels
Including TX doorbell. That's probably why we use mbox_chan_txdone to complete
It in controller driver. Slightly a bit strange.
Not sure if better idea to handle it in controller driver or framework.

Regards
Dong Aisheng

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

[A.s. Dong]

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar at gmail.com]
> Sent: Wednesday, August 1, 2018 5:45 PM

[...]

> >> > +
> >> > +static void imx_mu_txdb_work(struct work_struct *work) {
> >> > +       struct imx_mu_con_priv *cp =
> >> > +                       container_of(work, struct imx_mu_con_priv, work);
> >> > +       mbox_chan_txdone(cp->chan, 0); }
> >> >
> >> The api assumes a controller have same type of channels. So we are
> >> having to do this here.
> >> I am not sure, but would declaring two mailbox controllers (one with
> >> doorbells and the other data channels) work?
> >> If not, at least we could use a tasklet instead of a work queue?
> >>
> >
> > I'm also a bit curious what the meaning of calling mbox_chan_txdone
> > for a doorbell mailbox?
> >
> > Is there any recommended way to use it?
> >
> The framework submits a message (a signal with or without data) for
> controller to transmit. Obviously, it has to know when the transfer completes
> (so that it can submit the next message).
> 
> Depending on the controller h/w design, there can be three ways.
> 
> a) Controller has some irq that fires when the signal is consumed by the
> other end. Like MU does. So upon TX-IRQ, the controller driver calls
> mbox_chan_txdone() to tell the framework that the last submitted transfer
> has completed. This is indicated by setting only  txdone_irq = true
> 
> b) Controller does not have any tx-irq, but it can read some register to know
> the status. That is it has to be polled. The framework calls
> last_tx_done() periodically to check the status. This is indicated by setting
> only  txdone_poll = true.
> 
> c) Controller has neither irq, nor any status register. This is indicated by
> setting only  txdone_irq = false and  txdone_poll =
> false.   In this case, the protocol/client has to tell the framework
> (using some protocol level indicator like some ack packet reception) when
> the last submitted message was transferred, by calling
> mbox_client_txdone()
> 

Thanks for a detailed and clear explanation.

> A 'doorbell' is  (c) type channel -- just an irq raised at other end without any
> ack irq or status bit to check. So, the client would simply do
>                 mbox_send_message(chan, msg);
>                 mbox_client_txdone(chan, 0);

Just a bit wondering whether we need client to call mbox_client_txdone()
here as it actually does not know whether it's done. Not sure if it's better to
handle them in mailbox controller driver or framework.
(Oleksij seems do it in the controller driver in this patch)

And see mbox_client_txdone() definition, it seems it's only used by TXDONE_BY_ACK
case. Is the doorbell mailbox without IRQ or ACK support still suitable to call it to
notify the framework the transfer is done?

void mbox_client_txdone(struct mbox_chan *chan, int r)                                                                                                                                                              
{
        if (unlikely(!(chan->txdone_method & TXDONE_BY_ACK))) {
                dev_err(chan->mbox->dev, "Client can't run the TX ticker\n");                                                                                                                                       
                return;
        }

        tx_tick(chan, r);
}

For i.MX MU case, the controller is defined as TXDONE_BY_IRQ for all channels
Including TX doorbell. That's probably why we use mbox_chan_txdone to complete
It in controller driver. Slightly a bit strange.
Not sure if better idea to handle it in controller driver or framework.

Regards
Dong Aisheng

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

[A.s. Dong]

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> Sent: Tuesday, July 31, 2018 10:12 PM
> To: Shawn Guo <shawnguo@kernel.org>; Fabio Estevam
> <fabio.estevam@nxp.com>; Rob Herring <robh+dt@kernel.org>; Mark
> Rutland <mark.rutland@arm.com>; A.s. Dong <aisheng.dong@nxp.com>;
> Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>; Jassi Brar
> <jassisinghbrar@gmail.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 v8 4/4] mailbox: Add support for i.MX7D messaging unit
> 
> The Mailbox controller is able to send messages (up to 4 32 bit words)
> between the endpoints.
> 

I'm not sure but is this still valid after change to multi channels mode?
We actually lost the HW capability to send messages up to 4 words
in the multi channels mode.
And such commit message probably will confuse people.
Worth a improve?

Otherwise this patch mostly is okay to me except a few bits already
pointed out by Jassi.

Regards
Dong Aisheng

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

[A.s. Dong]

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel at pengutronix.de]
> Sent: Tuesday, July 31, 2018 10:12 PM
> To: Shawn Guo <shawnguo@kernel.org>; Fabio Estevam
> <fabio.estevam@nxp.com>; Rob Herring <robh+dt@kernel.org>; Mark
> Rutland <mark.rutland@arm.com>; A.s. Dong <aisheng.dong@nxp.com>;
> Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>; Jassi Brar
> <jassisinghbrar@gmail.com>
> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; kernel at pengutronix.de;
> linux-arm-kernel at lists.infradead.org; devicetree at vger.kernel.org; dl-linux-
> imx <linux-imx@nxp.com>
> Subject: [PATCH v8 4/4] mailbox: Add support for i.MX7D messaging unit
> 
> The Mailbox controller is able to send messages (up to 4 32 bit words)
> between the endpoints.
> 

I'm not sure but is this still valid after change to multi channels mode?
We actually lost the HW capability to send messages up to 4 words
in the multi channels mode.
And such commit message probably will confuse people.
Worth a improve?

Otherwise this patch mostly is okay to me except a few bits already
pointed out by Jassi.

Regards
Dong Aisheng

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

[Oleksij Rempel]

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

On Wed, Aug 01, 2018 at 07:52:43AM +0000, A.s. Dong wrote:
> > -----Original Message-----
> > From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> > Sent: Tuesday, July 31, 2018 10:12 PM
> > To: Shawn Guo <shawnguo@kernel.org>; Fabio Estevam
> > <fabio.estevam@nxp.com>; Rob Herring <robh+dt@kernel.org>; Mark
> > Rutland <mark.rutland@arm.com>; A.s. Dong <aisheng.dong@nxp.com>;
> > Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>; Jassi Brar
> > <jassisinghbrar@gmail.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 v8 4/4] mailbox: Add support for i.MX7D messaging unit
> > 
> > The Mailbox controller is able to send messages (up to 4 32 bit words)
> > between the endpoints.
> > 
> 
> I'm not sure but is this still valid after change to multi channels mode?
> We actually lost the HW capability to send messages up to 4 words
> in the multi channels mode.
> And such commit message probably will confuse people.
> Worth a improve?
> 
> Otherwise this patch mostly is okay to me except a few bits already
> pointed out by Jassi.

Ok, I schedule this task for Thursday or Friday.

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

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

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

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

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

[Oleksij Rempel]

On Wed, Aug 01, 2018 at 07:52:43AM +0000, A.s. Dong wrote:
> > -----Original Message-----
> > From: Oleksij Rempel [mailto:o.rempel at pengutronix.de]
> > Sent: Tuesday, July 31, 2018 10:12 PM
> > To: Shawn Guo <shawnguo@kernel.org>; Fabio Estevam
> > <fabio.estevam@nxp.com>; Rob Herring <robh+dt@kernel.org>; Mark
> > Rutland <mark.rutland@arm.com>; A.s. Dong <aisheng.dong@nxp.com>;
> > Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>; Jassi Brar
> > <jassisinghbrar@gmail.com>
> > Cc: Oleksij Rempel <o.rempel@pengutronix.de>; kernel at pengutronix.de;
> > linux-arm-kernel at lists.infradead.org; devicetree at vger.kernel.org; dl-linux-
> > imx <linux-imx@nxp.com>
> > Subject: [PATCH v8 4/4] mailbox: Add support for i.MX7D messaging unit
> > 
> > The Mailbox controller is able to send messages (up to 4 32 bit words)
> > between the endpoints.
> > 
> 
> I'm not sure but is this still valid after change to multi channels mode?
> We actually lost the HW capability to send messages up to 4 words
> in the multi channels mode.
> And such commit message probably will confuse people.
> Worth a improve?
> 
> Otherwise this patch mostly is okay to me except a few bits already
> pointed out by Jassi.

Ok, I schedule this task for Thursday or Friday.

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180801/b9a2ebf3/attachment.sig>