[PATCH v7 0/6] add mailbox support for i.MX7D

[PATCH v7 0/6] add mailbox support for i.MX7D

[Oleksij Rempel]

20180626 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

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

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

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

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

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

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

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

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

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

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

-- 
2.18.0

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

[Oleksij Rempel]

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

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

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

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

Re: [PATCH v7 1/6] dt-bindings: mailbox: allow mbox-cells to be equal to 0

[Jassi Brar]

On Thu, Jul 26, 2018 at 12:23 PM, Oleksij Rempel
<o.rempel@pengutronix.de> wrote:
> From: Dong Aisheng <aisheng.dong@nxp.com>
>
> Mailbox devices may have only one channel which means the mbox-cells
> at least 1 does not make sense for this type devices. Let's remove
> that limitation to allow the mbox-cells to be equal to 0.
>
OK

But please realise that you'll have to write more code -- implement
your own of_xlate() callback.
Most developers would prefer to call the only channel as channel
number 0, and use the common of_xlate implementation.

-jassi

RE: [PATCH v7 1/6] dt-bindings: mailbox: allow mbox-cells to be equal to 0

[A.s. Dong]

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> Sent: Thursday, July 26, 2018 5:42 PM
> To: Oleksij Rempel <o.rempel@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>; Fabio Estevam
> <fabio.estevam@nxp.com>; Rob Herring <robh+dt@kernel.org>; Mark
> Rutland <mark.rutland@arm.com>; A.s. Dong <aisheng.dong@nxp.com>;
> Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>; Sudeep Holla
> <sudeep.holla@arm.com>; Devicetree List <devicetree@vger.kernel.org>;
> Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; , Sascha Hauer
> <kernel@pengutronix.de>; , linux-arm-kernel@lists.infradead.org, linux-
> mediatek@lists.infradead.org, srv_heupstream <linux-arm-
> kernel@lists.infradead.org>; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v7 1/6] dt-bindings: mailbox: allow mbox-cells to be
> equal to 0
> 
> On Thu, Jul 26, 2018 at 12:23 PM, Oleksij Rempel <o.rempel@pengutronix.de>
> wrote:
> > From: Dong Aisheng <aisheng.dong@nxp.com>
> >
> > Mailbox devices may have only one channel which means the mbox-cells
> > at least 1 does not make sense for this type devices. Let's remove
> > that limitation to allow the mbox-cells to be equal to 0.
> >
> OK
> 
> But please realise that you'll have to write more code -- implement your own
> of_xlate() callback.
> Most developers would prefer to call the only channel as channel number 0,
> and use the common of_xlate implementation.
> 

Yes, that's true because the common of_xlate assuming the mbox-cells is 1.
And there's already such users:
arch/arm/boot/dts/bcm283x.dtsi

More comments will be on your reply of patch-3.

Regards
Dong Aisheng

> -jassi

Re: [PATCH v7 1/6] dt-bindings: mailbox: allow mbox-cells to be equal to 0

[Jassi Brar]

On Thu, Jul 26, 2018 at 4:30 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
>> -----Original Message-----
>> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
>> Sent: Thursday, July 26, 2018 5:42 PM
>> To: Oleksij Rempel <o.rempel@pengutronix.de>
>> Cc: Shawn Guo <shawnguo@kernel.org>; Fabio Estevam
>> <fabio.estevam@nxp.com>; Rob Herring <robh+dt@kernel.org>; Mark
>> Rutland <mark.rutland@arm.com>; A.s. Dong <aisheng.dong@nxp.com>;
>> Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>; Sudeep Holla
>> <sudeep.holla@arm.com>; Devicetree List <devicetree@vger.kernel.org>;
>> Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; , Sascha Hauer
>> <kernel@pengutronix.de>; , linux-arm-kernel@lists.infradead.org, linux-
>> mediatek@lists.infradead.org, srv_heupstream <linux-arm-
>> kernel@lists.infradead.org>; dl-linux-imx <linux-imx@nxp.com>
>> Subject: Re: [PATCH v7 1/6] dt-bindings: mailbox: allow mbox-cells to be
>> equal to 0
>>
>> On Thu, Jul 26, 2018 at 12:23 PM, Oleksij Rempel <o.rempel@pengutronix.de>
>> wrote:
>> > From: Dong Aisheng <aisheng.dong@nxp.com>
>> >
>> > Mailbox devices may have only one channel which means the mbox-cells
>> > at least 1 does not make sense for this type devices. Let's remove
>> > that limitation to allow the mbox-cells to be equal to 0.
>> >
>> OK
>>
>> But please realise that you'll have to write more code -- implement your own
>> of_xlate() callback.
>> Most developers would prefer to call the only channel as channel number 0,
>> and use the common of_xlate implementation.
>>
>
> Yes, that's true because the common of_xlate assuming the mbox-cells is 1.
> And there's already such users:
> arch/arm/boot/dts/bcm283x.dtsi
>
You mean that is a good thing, right?

RE: [PATCH v7 1/6] dt-bindings: mailbox: allow mbox-cells to be equal to 0

[A.s. Dong]

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> Sent: Thursday, July 26, 2018 7:37 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>; Sudeep Holla
> <sudeep.holla@arm.com>; Devicetree List <devicetree@vger.kernel.org>;
> Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; , Sascha Hauer
> <kernel@pengutronix.de>; , linux-arm-kernel@lists.infradead.org, linux-
> mediatek@lists.infradead.org, srv_heupstream <linux-arm-
> kernel@lists.infradead.org>; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v7 1/6] dt-bindings: mailbox: allow mbox-cells to be
> equal to 0
> 
> On Thu, Jul 26, 2018 at 4:30 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
> >> -----Original Message-----
> >> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> >> Sent: Thursday, July 26, 2018 5:42 PM
> >> To: Oleksij Rempel <o.rempel@pengutronix.de>
> >> Cc: Shawn Guo <shawnguo@kernel.org>; Fabio Estevam
> >> <fabio.estevam@nxp.com>; Rob Herring <robh+dt@kernel.org>; Mark
> >> Rutland <mark.rutland@arm.com>; A.s. Dong <aisheng.dong@nxp.com>;
> >> Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>; Sudeep Holla
> >> <sudeep.holla@arm.com>; Devicetree List <devicetree@vger.kernel.org>;
> >> Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; , Sascha
> >> Hauer <kernel@pengutronix.de>; ,
> >> linux-arm-kernel@lists.infradead.org, linux-
> >> mediatek@lists.infradead.org, srv_heupstream <linux-arm-
> >> kernel@lists.infradead.org>; dl-linux-imx <linux-imx@nxp.com>
> >> Subject: Re: [PATCH v7 1/6] dt-bindings: mailbox: allow mbox-cells to
> >> be equal to 0
> >>
> >> On Thu, Jul 26, 2018 at 12:23 PM, Oleksij Rempel
> >> <o.rempel@pengutronix.de>
> >> wrote:
> >> > From: Dong Aisheng <aisheng.dong@nxp.com>
> >> >
> >> > Mailbox devices may have only one channel which means the
> >> > mbox-cells at least 1 does not make sense for this type devices.
> >> > Let's remove that limitation to allow the mbox-cells to be equal to 0.
> >> >
> >> OK
> >>
> >> But please realise that you'll have to write more code -- implement
> >> your own
> >> of_xlate() callback.
> >> Most developers would prefer to call the only channel as channel
> >> number 0, and use the common of_xlate implementation.
> >>
> >
> > Yes, that's true because the common of_xlate assuming the mbox-cells is 1.
> > And there's already such users:
> > arch/arm/boot/dts/bcm283x.dtsi
> >
> You mean that is a good thing, right?

No, I mean there's similar HW in kernel already that only has one channel.
Are you suggesting even for one channel mailbox, we should still use
Mbox-cells 1 and use 'mboxes = <&mailbox 0>' in devicetree?

Regards
Dong Aisheng

Re: [PATCH v7 1/6] dt-bindings: mailbox: allow mbox-cells to be equal to 0

[Jassi Brar]

On Thu, Jul 26, 2018 at 5:25 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
>> -----Original Message-----
>> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
>> Sent: Thursday, July 26, 2018 7:37 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>; Sudeep Holla
>> <sudeep.holla@arm.com>; Devicetree List <devicetree@vger.kernel.org>;
>> Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; , Sascha Hauer
>> <kernel@pengutronix.de>; , linux-arm-kernel@lists.infradead.org, linux-
>> mediatek@lists.infradead.org, srv_heupstream <linux-arm-
>> kernel@lists.infradead.org>; dl-linux-imx <linux-imx@nxp.com>
>> Subject: Re: [PATCH v7 1/6] dt-bindings: mailbox: allow mbox-cells to be
>> equal to 0
>>
>> On Thu, Jul 26, 2018 at 4:30 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
>> >> -----Original Message-----
>> >> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
>> >> Sent: Thursday, July 26, 2018 5:42 PM
>> >> To: Oleksij Rempel <o.rempel@pengutronix.de>
>> >> Cc: Shawn Guo <shawnguo@kernel.org>; Fabio Estevam
>> >> <fabio.estevam@nxp.com>; Rob Herring <robh+dt@kernel.org>; Mark
>> >> Rutland <mark.rutland@arm.com>; A.s. Dong <aisheng.dong@nxp.com>;
>> >> Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>; Sudeep Holla
>> >> <sudeep.holla@arm.com>; Devicetree List <devicetree@vger.kernel.org>;
>> >> Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; , Sascha
>> >> Hauer <kernel@pengutronix.de>; ,
>> >> linux-arm-kernel@lists.infradead.org, linux-
>> >> mediatek@lists.infradead.org, srv_heupstream <linux-arm-
>> >> kernel@lists.infradead.org>; dl-linux-imx <linux-imx@nxp.com>
>> >> Subject: Re: [PATCH v7 1/6] dt-bindings: mailbox: allow mbox-cells to
>> >> be equal to 0
>> >>
>> >> On Thu, Jul 26, 2018 at 12:23 PM, Oleksij Rempel
>> >> <o.rempel@pengutronix.de>
>> >> wrote:
>> >> > From: Dong Aisheng <aisheng.dong@nxp.com>
>> >> >
>> >> > Mailbox devices may have only one channel which means the
>> >> > mbox-cells at least 1 does not make sense for this type devices.
>> >> > Let's remove that limitation to allow the mbox-cells to be equal to 0.
>> >> >
>> >> OK
>> >>
>> >> But please realise that you'll have to write more code -- implement
>> >> your own
>> >> of_xlate() callback.
>> >> Most developers would prefer to call the only channel as channel
>> >> number 0, and use the common of_xlate implementation.
>> >>
>> >
>> > Yes, that's true because the common of_xlate assuming the mbox-cells is 1.
>> > And there's already such users:
>> > arch/arm/boot/dts/bcm283x.dtsi
>> >
>> You mean that is a good thing, right?
>
> No, I mean there's similar HW in kernel already that only has one channel.
> Are you suggesting even for one channel mailbox, we should still use
> Mbox-cells 1 and use 'mboxes = <&mailbox 0>' in devicetree?
>
As i said, I am ok with the mbox-cells to be 0.

But if you have two instances of the controller, one with 4 channels
and the other with 1 channel. Then you can/should not have different
bindings.

And, if the controller can have only 1 channel, then please feel free
to have mbox-cells=0 and implement your own of_xlate().

[PATCH v7 2/6] 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

Re: [PATCH v7 2/6] dt-bindings: arm: fsl: add mu binding doc

[Jassi Brar]

On Thu, Jul 26, 2018 at 12:23 PM, Oleksij Rempel
<o.rempel@pengutronix.de> wrote:
> 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
>
Please consider my reply to patch-3

[PATCH v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU channel support

[Oleksij Rempel]

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

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

diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
index 90e4905dfc69..113d6ab931ef 100644
--- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
+++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
@@ -22,7 +22,14 @@ Required properties:
 - reg :		Should contain the registers location and length
 - interrupts :	Interrupt number. The interrupt specifier format depends
 		on the interrupt controller parent.
-- #mbox-cells:  Must be 0. Number of cells in a mailbox
+- #mbox-cells:  Must be:
+		0 - for single channel mode. i.MX8* SCU protocol specific.
+		1 - for multichannel (generic) mode.
+
+Optional properties:
+-------------------
+- clocks :	phandle to the input clock.
+- fsl,mu-side-b : Should be set for side B MU.
 
 Examples:
 --------
-- 
2.18.0

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

[Jassi Brar]

On Thu, Jul 26, 2018 at 12:23 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.
>
So the hardware actually supports 4 channels.

> -- #mbox-cells:  Must be 0. Number of cells in a mailbox
> +- #mbox-cells:  Must be:
> +               0 - for single channel mode. i.MX8* SCU protocol specific.
> +               1 - for multichannel (generic) mode.
> +
No, please.
DT bindings should reflect the real hardware, and not the software
mode we want the driver to work in.
Please define mbox-cells=1  and have the i.MX8* platform always ask
for channel-0.

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

[Oleksij Rempel]

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



On 26.07.2018 11:49, Jassi Brar wrote:
> On Thu, Jul 26, 2018 at 12:23 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.
>>
> So the hardware actually supports 4 channels.
> 
>> -- #mbox-cells:  Must be 0. Number of cells in a mailbox
>> +- #mbox-cells:  Must be:
>> +               0 - for single channel mode. i.MX8* SCU protocol specific.
>> +               1 - for multichannel (generic) mode.
>> +
> No, please.
> DT bindings should reflect the real hardware, and not the software
> mode we want the driver to work in.
> Please define mbox-cells=1  and have the i.MX8* platform always ask
> for channel-0.

We spend already some week to publicly talk about this..
The problem is, imx8 has multiple MUs, only some of them communicate
directly with SCU. SCU has some well defined protocol and
MU should be used and configured in some SCU specific way. From NXP
perspective, SCU firmware should not be replaceable and from Linux side
seen as part of the HW.

@Dong, is it correct description?


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

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

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

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

[Jassi Brar]

On Thu, Jul 26, 2018 at 4:27 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
>
>
> On 26.07.2018 11:49, Jassi Brar wrote:
>> On Thu, Jul 26, 2018 at 12:23 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.
>>>
>> So the hardware actually supports 4 channels.
>>
>>> -- #mbox-cells:  Must be 0. Number of cells in a mailbox
>>> +- #mbox-cells:  Must be:
>>> +               0 - for single channel mode. i.MX8* SCU protocol specific.
>>> +               1 - for multichannel (generic) mode.
>>> +
>> No, please.
>> DT bindings should reflect the real hardware, and not the software
>> mode we want the driver to work in.
>> Please define mbox-cells=1  and have the i.MX8* platform always ask
>> for channel-0.
>
> We spend already some week to publicly talk about this..
>
You should have kept me in the loop.
I know, I am not that important, but for the sake of the protocol you
should have CC'ed the mailbox maintainer :)

> The problem is, imx8 has multiple MUs
>
You mean multiple instances of the MU. Some have 4 channels, some 1.
Is that correct?

>, only some of them communicate
> directly with SCU.
>
OK

> SCU has some well defined protocol and
> MU should be used and configured in some SCU specific way. From NXP
> perspective, SCU firmware should not be replaceable and from Linux side
> seen as part of the HW.
>
Sure. But the behaviour (protocol) should be implemented in the client
driver, not the controller driver.

RE: [PATCH v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU channel support

[A.s. Dong]

Hi Jassi,

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> Sent: Thursday, July 26, 2018 5:50 PM
> To: Oleksij Rempel <o.rempel@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>; Fabio Estevam
> <fabio.estevam@nxp.com>; Rob Herring <robh+dt@kernel.org>; Mark
> Rutland <mark.rutland@arm.com>; A.s. Dong <aisheng.dong@nxp.com>;
> 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 v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU
> channel support
> 
> On Thu, Jul 26, 2018 at 12:23 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.
> >
> So the hardware actually supports 4 channels.
> 
> > -- #mbox-cells:  Must be 0. Number of cells in a mailbox
> > +- #mbox-cells:  Must be:
> > +               0 - for single channel mode. i.MX8* SCU protocol specific.
> > +               1 - for multichannel (generic) mode.
> > +
> No, please.
> DT bindings should reflect the real hardware, and not the software mode we
> want the driver to work in.
> Please define mbox-cells=1  and have the i.MX8* platform always ask for
> channel-0.

The reality is that MU hardware does not define channels in reference manual.
However, it does have four separate data tx/rx register which can be used
as 'virtual' channels which is supported by this current driver.

See below HW description from the reference manual:
For messaging, the MU has four, 32-bit write-only transmit registers and four, 32-bit
read-only receive registers on the Processor B and Processor A-sides. These registers are
used for sending messages to each other.

Passing short messages
Transmit register(s) can be used to pass short messages
from one to four words in length. For example, when a four-word message is desired,
only one of the registers needs to have its corresponding interrupt enable bit set at the
receiver side; the message’s first three words are written to the registers whose
interrupt is masked, and the fourth word is written to the other register (which
triggers an interrupt at the receiver side).

The reference manual is at here: (Chapter 42 Messaging Unit (MU)
https://www.nxp.com/docs/en/reference-manual/IMX6ULRM.pdf

And SCU firmware does use MU in above way specified by reference manual.
Even from HW point of view, it's still one channel only mailbox.
That's why we change the mbox-cells to 0 accordingly to avoid confusing
the multi channels users supported in this driver.

As per Sasha's request, we're going to support both protocol (SCU and generic M4)
in one mailbox driver, that means we need deal with this two case property.

If we use mbox-cells 1 case for SCU, then the using would be like:
mboxes = <&mailbox 0> which is mixed with the normal multi channel
users. And we also must state clearly in binding doc that for mbox-cells
 1 case for SCU, the cell value must be 0 which is also confusing with
Multi channel users.

How do you suggest for such case?

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

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

[Jassi Brar]

On Thu, Jul 26, 2018 at 5:21 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
> Hi Jassi,
>
>> -----Original Message-----
>> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
>> Sent: Thursday, July 26, 2018 5:50 PM
>> To: Oleksij Rempel <o.rempel@pengutronix.de>
>> Cc: Shawn Guo <shawnguo@kernel.org>; Fabio Estevam
>> <fabio.estevam@nxp.com>; Rob Herring <robh+dt@kernel.org>; Mark
>> Rutland <mark.rutland@arm.com>; A.s. Dong <aisheng.dong@nxp.com>;
>> 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 v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU
>> channel support
>>
>> On Thu, Jul 26, 2018 at 12:23 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.
>> >
>> So the hardware actually supports 4 channels.
>>
>> > -- #mbox-cells:  Must be 0. Number of cells in a mailbox
>> > +- #mbox-cells:  Must be:
>> > +               0 - for single channel mode. i.MX8* SCU protocol specific.
>> > +               1 - for multichannel (generic) mode.
>> > +
>> No, please.
>> DT bindings should reflect the real hardware, and not the software mode we
>> want the driver to work in.
>> Please define mbox-cells=1  and have the i.MX8* platform always ask for
>> channel-0.
>
> The reality is that MU hardware does not define channels in reference manual.
> However, it does have four separate data tx/rx register which can be used
> as 'virtual' channels which is supported by this current driver.
>
> See below HW description from the reference manual:
> For messaging, the MU has four, 32-bit write-only transmit registers and four, 32-bit
> read-only receive registers on the Processor B and Processor A-sides. These registers are
> used for sending messages to each other.
>
> Passing short messages
> Transmit register(s) can be used to pass short messages
> from one to four words in length. For example, when a four-word message is desired,
> only one of the registers needs to have its corresponding interrupt enable bit set at the
> receiver side; the message’s first three words are written to the registers whose
> interrupt is masked, and the fourth word is written to the other register (which
> triggers an interrupt at the receiver side).
>
> The reference manual is at here: (Chapter 42 Messaging Unit (MU)
> https://www.nxp.com/docs/en/reference-manual/IMX6ULRM.pdf
>
Thanks. That makes life much easier.

> And SCU firmware does use MU in above way specified by reference manual.
> Even from HW point of view, it's still one channel only mailbox.
> That's why we change the mbox-cells to 0 accordingly to avoid confusing
> the multi channels users supported in this driver.
>
> As per Sasha's request, we're going to support both protocol (SCU and generic M4)
> in one mailbox driver, that means we need deal with this two case property.
>
> If we use mbox-cells 1 case for SCU, then the using would be like:
> mboxes = <&mailbox 0> which is mixed with the normal multi channel
> users. And we also must state clearly in binding doc that for mbox-cells
>  1 case for SCU, the cell value must be 0 which is also confusing with
> Multi channel users.
>
> How do you suggest for such case?
>
Let me read the spec and try to see/shine the light :)

Cheers!

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

RE: [PATCH v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU channel support

[A.s. Dong]

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> Sent: Thursday, July 26, 2018 8:27 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 v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU
> channel support

[...]

> > And SCU firmware does use MU in above way specified by reference
> manual.
> > Even from HW point of view, it's still one channel only mailbox.
> > That's why we change the mbox-cells to 0 accordingly to avoid
> > confusing the multi channels users supported in this driver.
> >
> > As per Sasha's request, we're going to support both protocol (SCU and
> > generic M4) in one mailbox driver, that means we need deal with this two
> case property.
> >
> > If we use mbox-cells 1 case for SCU, then the using would be like:
> > mboxes = <&mailbox 0> which is mixed with the normal multi channel
> > users. And we also must state clearly in binding doc that for
> > mbox-cells
> >  1 case for SCU, the cell value must be 0 which is also confusing with
> > Multi channel users.
> >
> > How do you suggest for such case?
> >
> Let me read the spec and try to see/shine the light :)

Great, many thanks!

Regards
Dong Aisheng

> 
> Cheers!

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

[Jassi Brar]

On Thu, Jul 26, 2018 at 5:21 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
>
> The reference manual is at here: (Chapter 42 Messaging Unit (MU)
> https://www.nxp.com/docs/en/reference-manual/IMX6ULRM.pdf
>
You seem to have shared a wrong link. It has
       Chapter 42  Synchronous Audio Interface (SAI)

and finding "Messaging Unit" shows no result.

RE: [PATCH v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU channel support

[A.s. Dong]

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> Sent: Thursday, July 26, 2018 9:50 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 v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU
> channel support
> 
> On Thu, Jul 26, 2018 at 5:21 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
> >
> > The reference manual is at here: (Chapter 42 Messaging Unit (MU)
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fw
> ww.nxp.com%2Fdocs%2Fen%2Freference-
> manual%2FIMX6ULRM.pdf&amp;data=02%7C01%7Caisheng.dong%40nxp.co
> m%7Ca1e872436611497795bb08d5f2fe9feb%7C686ea1d3bc2b4c6fa92cd99c5c
> 301635%7C0%7C0%7C636682097774333065&amp;sdata=Zqf9kgJrutPtQE8OP
> %2BDu1%2BvULXWSQcgHbszwIWkTNS0%3D&amp;reserved=0
> >
> You seem to have shared a wrong link. It has
>        Chapter 42  Synchronous Audio Interface (SAI)
> 
> and finding "Messaging Unit" shows no result.

Sorry, it should be:
https://www.nxp.com/docs/en/reference-manual/IMX6SXRM.pdf

Regards
Dong Aisheng

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

[Jassi Brar]

On Thu, Jul 26, 2018 at 5:21 PM, A.s. Dong <aisheng.dong@nxp.com> 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.
>> >
>> So the hardware actually supports 4 channels.
>>
>> > -- #mbox-cells:  Must be 0. Number of cells in a mailbox
>> > +- #mbox-cells:  Must be:
>> > +               0 - for single channel mode. i.MX8* SCU protocol specific.
>> > +               1 - for multichannel (generic) mode.
>> > +
>> No, please.
>> DT bindings should reflect the real hardware, and not the software mode we
>> want the driver to work in.
>> Please define mbox-cells=1  and have the i.MX8* platform always ask for
>> channel-0.
>
> The reality is that MU hardware does not define channels in reference manual.
> However, it does have four separate data tx/rx register which can be used
> as 'virtual' channels which is supported by this current driver.
>
> See below HW description from the reference manual:
> For messaging, the MU has four, 32-bit write-only transmit registers and four, 32-bit
> read-only receive registers on the Processor B and Processor A-sides. These registers are
> used for sending messages to each other.
>
For a while please forget the protocol(user) level usage, and consider
only what your h/w is.

MU has 4 pairs of TX_Reg + TX_IRQ, and 4 pairs of RX_Reg + RX_IRQ.
(MU also has 4 "doorbell" type channels that it calls GP, but those
are not managed here, so lets not worry atm).

By definition, a mailbox channel is simply a signal, optionally with
data attached. So, MU has 4 TX and 4 RX channels.

The MU driver should populate 8 unidirectional (4 Tx and 4 RX)
channels and set each tx/rx operation to trigger the corresponding
interrupt. This is not my whim, this is how the controller is!

The SCU is poorly implemented as it ignores 3 irqs and club all 4
register together (there are many other cons of this approach but lets
not get into that).
Personally, I would push-back on such a bad design. But if you claim
you have no choice but to support SCU as such, the work around could
be simpler than defining a new "scu mode" altogether.

#mbox-cells:  Must be 3.
                      First cell is 1 for TX and 0 for RX channel
                      Second cell is index of the channel [0,1,2 or 3]
                      Third cell is 1 if the channel triggers an IRQ,
0 if not. That is ACR.RIE/TIE bits are set or not.

Normal clients would always request a channel with irqs enabled.
The SCU client would request all 8 channels -- TX/RX[0,1,2] with irqs
disabled, TX/RX[3] with irqs enabled. And SCU will read/write 4 word
data over 4 rx/tx channels, instead of the virtually concocted one.


> short messages
> Transmit register(s) can be used to pass short messages
> from one to four words in length. For example, when a four-word message is desired,
> only one of the registers needs to have its corresponding interrupt enable bit set at the
> receiver side; the message’s first three words are written to the registers whose
> interrupt is masked, and the fourth word is written to the other register (which
> triggers an interrupt at the receiver side).
>
> The reference manual is at here: (Chapter 42 Messaging Unit (MU)
> https://www.nxp.com/docs/en/reference-manual/IMX6ULRM.pdf
>
> And SCU firmware does use MU in above way specified by reference manual.
> Even from HW point of view, it's still one channel only mailbox.
>
Please realise that any manual is written by a mere mortal afterall.
How the controller works is set in stone, but how the controller can
be used ... is just someones suggestion.

The approach I suggest above, conforms to the api and prevents a
provider dancing to the tunes of a user.

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

RE: [PATCH v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU channel support

[A.s. Dong]

Hi Jassi,

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> Sent: Thursday, July 26, 2018 11:44 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 v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU
> channel support
> 
> On Thu, Jul 26, 2018 at 5:21 PM, A.s. Dong <aisheng.dong@nxp.com> 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.
> >> >
> >> So the hardware actually supports 4 channels.
> >>
> >> > -- #mbox-cells:  Must be 0. Number of cells in a mailbox
> >> > +- #mbox-cells:  Must be:
> >> > +               0 - for single channel mode. i.MX8* SCU protocol specific.
> >> > +               1 - for multichannel (generic) mode.
> >> > +
> >> No, please.
> >> DT bindings should reflect the real hardware, and not the software
> >> mode we want the driver to work in.
> >> Please define mbox-cells=1  and have the i.MX8* platform always ask
> >> for channel-0.
> >
> > The reality is that MU hardware does not define channels in reference
> manual.
> > However, it does have four separate data tx/rx register which can be
> > used as 'virtual' channels which is supported by this current driver.
> >
> > See below HW description from the reference manual:
> > For messaging, the MU has four, 32-bit write-only transmit registers
> > and four, 32-bit read-only receive registers on the Processor B and
> > Processor A-sides. These registers are used for sending messages to each
> other.
> >
> For a while please forget the protocol(user) level usage, and consider only
> what your h/w is.
> 
> MU has 4 pairs of TX_Reg + TX_IRQ, and 4 pairs of RX_Reg + RX_IRQ.
> (MU also has 4 "doorbell" type channels that it calls GP, but those are not
> managed here, so lets not worry atm).
> 
> By definition, a mailbox channel is simply a signal, optionally with data
> attached. So, MU has 4 TX and 4 RX channels.
> 
> The MU driver should populate 8 unidirectional (4 Tx and 4 RX) channels and
> set each tx/rx operation to trigger the corresponding interrupt. This is not my
> whim, this is how the controller is!
> 

This looks like reasonable to me, theoretically. 
Just not sure whether it's necessary to support it because we probably will never use
like that in reality, then it might become meaningless complicity introduced
and error prone.

And AFAIK ARM MHU is doing the same way as MU which looks like also unidirectional channel.
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0515b/CHDGBGIF.html
drivers/mailbox/arm_mhu.c

Sascha & Oleksij,
Any idea about this?

> The SCU is poorly implemented as it ignores 3 irqs and club all 4 register
> together (there are many other cons of this approach but lets not get into
> that).
> Personally, I would push-back on such a bad design. But if you claim you have
> no choice but to support SCU as such, the work around could be simpler than
> defining a new "scu mode" altogether.
> 

This is one of the recommended ways designed in HW reference manual and it
allows to send frame information up to 4 words without using shared memory.
SCU just follows it, so it's hard to believe it's a bad design.

> #mbox-cells:  Must be 3.
>                       First cell is 1 for TX and 0 for RX channel
>                       Second cell is index of the channel [0,1,2 or 3]
>                       Third cell is 1 if the channel triggers an IRQ,
> 0 if not. That is ACR.RIE/TIE bits are set or not.
> 
> Normal clients would always request a channel with irqs enabled.
> The SCU client would request all 8 channels -- TX/RX[0,1,2] with irqs disabled,
> TX/RX[3] with irqs enabled. And SCU will read/write 4 word data over 4 rx/tx
> channels, instead of the virtually concocted one.
> 

It may work If SCU protocol data length is fixed to 4 words. However, it's length
is flexible for different SVC service. That means this binding won't work for SCU.
And it will introduce much complexities during the implementation.

Instead, we're using polling mode for both TX/RX and the data size is stored in
the msg header and sending msgs using all 4 data tx/rx registers as a channel fifo.

> 
> > short messages
> > Transmit register(s) can be used to pass short messages from one to
> > four words in length. For example, when a four-word message is
> > desired, only one of the registers needs to have its corresponding
> > interrupt enable bit set at the receiver side; the message’s first
> > three words are written to the registers whose interrupt is masked,
> > and the fourth word is written to the other register (which triggers an
> interrupt at the receiver side).
> >
> > The reference manual is at here: (Chapter 42 Messaging Unit (MU)
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fw
> ww
> > .nxp.com%2Fdocs%2Fen%2Freference-
> manual%2FIMX6ULRM.pdf&amp;data=02%7C0
> >
> 1%7Caisheng.dong%40nxp.com%7Cef349d7c9caf46c4260008d5f30e9ef5%7C6
> 86ea1
> >
> d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636682166494860332&amp;sdat
> a=54rE
> >
> iDm%2BGD6EY8NE64ck1LXVGoUCtWYrHXPoWqcujUo%3D&amp;reserved=0
> >
> > And SCU firmware does use MU in above way specified by reference
> manual.
> > Even from HW point of view, it's still one channel only mailbox.
> >
> Please realise that any manual is written by a mere mortal afterall.
> How the controller works is set in stone, but how the controller can be
> used ... is just someones suggestion.
> 
> The approach I suggest above, conforms to the api and prevents a provider
> dancing to the tunes of a user.

First of all, really appreciate for your suggestions. 
It may be hard to find a common binding with the same mbox-cells.
It looks like we just need a property is distinguish how the Mailbox is used
In one channel or multi channel mode.

Directly checking mbox-cells seems the most easy way and it is already Acked
by Rob. Can't this way be Okay to you?

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

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

[Jassi Brar]

On Fri, Jul 27, 2018 at 9:32 AM, A.s. Dong <aisheng.dong@nxp.com> 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.
>> >> >
>> >> So the hardware actually supports 4 channels.
>> >>
>> >> > -- #mbox-cells:  Must be 0. Number of cells in a mailbox
>> >> > +- #mbox-cells:  Must be:
>> >> > +               0 - for single channel mode. i.MX8* SCU protocol specific.
>> >> > +               1 - for multichannel (generic) mode.
>> >> > +
>> >> No, please.
>> >> DT bindings should reflect the real hardware, and not the software
>> >> mode we want the driver to work in.
>> >> Please define mbox-cells=1  and have the i.MX8* platform always ask
>> >> for channel-0.
>> >
>> > The reality is that MU hardware does not define channels in reference
>> manual.
>> > However, it does have four separate data tx/rx register which can be
>> > used as 'virtual' channels which is supported by this current driver.
>> >
>> > See below HW description from the reference manual:
>> > For messaging, the MU has four, 32-bit write-only transmit registers
>> > and four, 32-bit read-only receive registers on the Processor B and
>> > Processor A-sides. These registers are used for sending messages to each
>> other.
>> >
>> For a while please forget the protocol(user) level usage, and consider only
>> what your h/w is.
>>
>> MU has 4 pairs of TX_Reg + TX_IRQ, and 4 pairs of RX_Reg + RX_IRQ.
>> (MU also has 4 "doorbell" type channels that it calls GP, but those are not
>> managed here, so lets not worry atm).
>>
>> By definition, a mailbox channel is simply a signal, optionally with data
>> attached. So, MU has 4 TX and 4 RX channels.
>>
>> The MU driver should populate 8 unidirectional (4 Tx and 4 RX) channels and
>> set each tx/rx operation to trigger the corresponding interrupt. This is not my
>> whim, this is how the controller is!
>>
>
> This looks like reasonable to me, theoretically.
> Just not sure whether it's necessary to support it because we probably will never use
> like that in reality, then it might become meaningless complicity introduced
> and error prone.
>
It _is_ necessary to write controller driver independent of client drivers.


> And AFAIK ARM MHU is doing the same way as MU which looks like also unidirectional channel.
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0515b/CHDGBGIF.html
> drivers/mailbox/arm_mhu.c
>
MHU driver is doing exactly what the data sheet says. I know because I
wrote the driver.


>> The SCU is poorly implemented as it ignores 3 irqs and club all 4 register
>> together (there are many other cons of this approach but lets not get into
>> that).
>> Personally, I would push-back on such a bad design. But if you claim you have
>> no choice but to support SCU as such, the work around could be simpler than
>> defining a new "scu mode" altogether.
>>
>
> This is one of the recommended ways designed in HW reference manual and it
> allows to send frame information up to 4 words without using shared memory.
> SCU just follows it, so it's hard to believe it's a bad design.
>
>> #mbox-cells:  Must be 3.
>>                       First cell is 1 for TX and 0 for RX channel
>>                       Second cell is index of the channel [0,1,2 or 3]
>>                       Third cell is 1 if the channel triggers an IRQ,
>> 0 if not. That is ACR.RIE/TIE bits are set or not.
>>
>> Normal clients would always request a channel with irqs enabled.
>> The SCU client would request all 8 channels -- TX/RX[0,1,2] with irqs disabled,
>> TX/RX[3] with irqs enabled. And SCU will read/write 4 word data over 4 rx/tx
>> channels, instead of the virtually concocted one.
>>
>
> It may work If SCU protocol data length is fixed to 4 words. However, it's length
> is flexible for different SVC service. That means this binding won't work for SCU.
> And it will introduce much complexities during the implementation.
>
> Instead, we're using polling mode for both TX/RX and the data size is stored in
> the msg header and sending msgs using all 4 data tx/rx registers as a channel fifo.
>
You first give me the "Passing short messages" usecase (quite a bad
one) and ask how it can work. When I give you a solution, you
effectively say "well, my usecase isn't even that". I feel I just
wasted my time.


>>
>> > short messages
>> > Transmit register(s) can be used to pass short messages from one to
>> > four words in length. For example, when a four-word message is
>> > desired, only one of the registers needs to have its corresponding
>> > interrupt enable bit set at the receiver side; the message’s first
>> > three words are written to the registers whose interrupt is masked,
>> > and the fourth word is written to the other register (which triggers an
>> interrupt at the receiver side).
>> >
>> > The reference manual is at here: (Chapter 42 Messaging Unit (MU)
>> >
>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fw
>> ww
>> > .nxp.com%2Fdocs%2Fen%2Freference-
>> manual%2FIMX6ULRM.pdf&amp;data=02%7C0
>> >
>> 1%7Caisheng.dong%40nxp.com%7Cef349d7c9caf46c4260008d5f30e9ef5%7C6
>> 86ea1
>> >
>> d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636682166494860332&amp;sdat
>> a=54rE
>> >
>> iDm%2BGD6EY8NE64ck1LXVGoUCtWYrHXPoWqcujUo%3D&amp;reserved=0
>> >
>> > And SCU firmware does use MU in above way specified by reference
>> manual.
>> > Even from HW point of view, it's still one channel only mailbox.
>> >
>> Please realise that any manual is written by a mere mortal afterall.
>> How the controller works is set in stone, but how the controller can be
>> used ... is just someones suggestion.
>>
>> The approach I suggest above, conforms to the api and prevents a provider
>> dancing to the tunes of a user.
>
> First of all, really appreciate for your suggestions.
> It may be hard to find a common binding with the same mbox-cells.
> It looks like we just need a property is distinguish how the Mailbox is used
> In one channel or multi channel mode.
>
I get the idea you were ready to see the code merged in the coming
window and be done with it. And now it feels lazy to change the code.
I am sorry, but I can't allow controller drivers "emulating" some mode
for a client driver. That is moving a part of client code into the
controller driver.


> Directly checking mbox-cells seems the most easy way and it is already Acked
> by Rob. Can't this way be Okay to you?
>
Rob is indeed far better than I am. But he also has to look into 100
other subsystems, whereas I am only looking into mailbox. I have time
to dig into your datasheets. I believe Rob will point out anything
wrong I suggest.

BTW, the submitted driver doesn't even support the SCU usecase. Why
the bindings?

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

RE: [PATCH v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU channel support

[A.s. Dong]

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> Sent: Friday, July 27, 2018 12:56 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; , Sascha Hauer
> <kernel@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>; , 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 v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU
> channel support
> 
> On Fri, Jul 27, 2018 at 9:32 AM, A.s. Dong <aisheng.dong@nxp.com> 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.
> >> >> >
> >> >> So the hardware actually supports 4 channels.
> >> >>
> >> >> > -- #mbox-cells:  Must be 0. Number of cells in a mailbox
> >> >> > +- #mbox-cells:  Must be:
> >> >> > +               0 - for single channel mode. i.MX8* SCU protocol specific.
> >> >> > +               1 - for multichannel (generic) mode.
> >> >> > +
> >> >> No, please.
> >> >> DT bindings should reflect the real hardware, and not the software
> >> >> mode we want the driver to work in.
> >> >> Please define mbox-cells=1  and have the i.MX8* platform always
> >> >> ask for channel-0.
> >> >
> >> > The reality is that MU hardware does not define channels in
> >> > reference
> >> manual.
> >> > However, it does have four separate data tx/rx register which can
> >> > be used as 'virtual' channels which is supported by this current driver.
> >> >
> >> > See below HW description from the reference manual:
> >> > For messaging, the MU has four, 32-bit write-only transmit
> >> > registers and four, 32-bit read-only receive registers on the
> >> > Processor B and Processor A-sides. These registers are used for
> >> > sending messages to each
> >> other.
> >> >
> >> For a while please forget the protocol(user) level usage, and
> >> consider only what your h/w is.
> >>
> >> MU has 4 pairs of TX_Reg + TX_IRQ, and 4 pairs of RX_Reg + RX_IRQ.
> >> (MU also has 4 "doorbell" type channels that it calls GP, but those
> >> are not managed here, so lets not worry atm).
> >>
> >> By definition, a mailbox channel is simply a signal, optionally with
> >> data attached. So, MU has 4 TX and 4 RX channels.
> >>
> >> The MU driver should populate 8 unidirectional (4 Tx and 4 RX)
> >> channels and set each tx/rx operation to trigger the corresponding
> >> interrupt. This is not my whim, this is how the controller is!
> >>
> >
> > This looks like reasonable to me, theoretically.
> > Just not sure whether it's necessary to support it because we probably
> > will never use like that in reality, then it might become meaningless
> > complicity introduced and error prone.
> >
> It _is_ necessary to write controller driver independent of client drivers.
> 

Yes, that's true. What if we think we're writing driver against HW capabilities
which support 4 pair of channel links(tx/rx)? 
It looks like independent of client drivers and may make life easily.
Does it make sense?

> 
> > And AFAIK ARM MHU is doing the same way as MU which looks like also
> unidirectional channel.
> >
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Finfo
> >
> center.arm.com%2Fhelp%2Findex.jsp%3Ftopic%3D%2Fcom.arm.doc.ddi0515
> b%2F
> >
> CHDGBGIF.html&amp;data=02%7C01%7Caisheng.dong%40nxp.com%7Cb728
> 16362983
> >
> 4208f9e908d5f37d3e00%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%
> 7C6366
> >
> 82641593785009&amp;sdata=VsG0oXxEObPRwT5KVss2eZthSTMTR2%2BMrv
> PqhDUpPGU
> > %3D&amp;reserved=0
> > drivers/mailbox/arm_mhu.c
> >
> MHU driver is doing exactly what the data sheet says. I know because I wrote
> the driver.
> 

Hmm... Maybe I missed something, but seems no from what I see:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0922g/CCHHGIAH.html

Let's see the data sheet:
2.1.1. Physical channels
The CSS MHU peripheral provides physical channels that are used for communication between
the SCP and the AP.  These channels are called physical channels because their implementation
is fixed in hardware.  Each physical channel is unidirectional (SCP to AP or AP to SCP).

A physical channel comprises:
A 32-bit STAT (STATUS) register:
A 32-bit SET register:
A 32-bit CLEAR register:

The driver seems composes them into a pair of links with both tx and rx physical channel.
drivers/mailbox/arm_mhu.c
#define MHU_CHANS       3

struct mhu_link {
        unsigned irq;
        void __iomem *tx_reg;
        void __iomem *rx_reg;
};

for (i = 0; i < MHU_CHANS; i++) {
        mhu->chan[i].con_priv = &mhu->mlink[i];
        mhu->mlink[i].irq = adev->irq[i];
        mhu->mlink[i].rx_reg = mhu->base + mhu_reg[i];
        mhu->mlink[i].tx_reg = mhu->mlink[i].rx_reg + TX_REG_OFFSET;
}
And the driver totally supports 3 links lp, hp and sec. Each links actually
are using two unidirectional physical channels. Am I correct?

> 
> >> The SCU is poorly implemented as it ignores 3 irqs and club all 4
> >> register together (there are many other cons of this approach but
> >> lets not get into that).
> >> Personally, I would push-back on such a bad design. But if you claim
> >> you have no choice but to support SCU as such, the work around could
> >> be simpler than defining a new "scu mode" altogether.
> >>
> >
> > This is one of the recommended ways designed in HW reference manual
> > and it allows to send frame information up to 4 words without using shared
> memory.
> > SCU just follows it, so it's hard to believe it's a bad design.
> >
> >> #mbox-cells:  Must be 3.
> >>                       First cell is 1 for TX and 0 for RX channel
> >>                       Second cell is index of the channel [0,1,2 or 3]
> >>                       Third cell is 1 if the channel triggers an IRQ,
> >> 0 if not. That is ACR.RIE/TIE bits are set or not.
> >>
> >> Normal clients would always request a channel with irqs enabled.
> >> The SCU client would request all 8 channels -- TX/RX[0,1,2] with irqs
> >> disabled, TX/RX[3] with irqs enabled. And SCU will read/write 4 word
> >> data over 4 rx/tx channels, instead of the virtually concocted one.
> >>
> >
> > It may work If SCU protocol data length is fixed to 4 words. However,
> > it's length is flexible for different SVC service. That means this binding
> won't work for SCU.
> > And it will introduce much complexities during the implementation.
> >
> > Instead, we're using polling mode for both TX/RX and the data size is
> > stored in the msg header and sending msgs using all 4 data tx/rx registers
> as a channel fifo.
> >
> You first give me the "Passing short messages" usecase (quite a bad
> one) and ask how it can work. When I give you a solution, you effectively say
> "well, my usecase isn't even that". I feel I just wasted my time.
> 

Sorry for may not clear, "Passing short message' usecase is to tell how the
HW is working on one channel mode sending up to 4 words in one time
As specified in reference manual.

SCU does work that way, the only difference is it's using polling mode
rather than interrupt driven.  The point is the data size may be different
for each msg, so we can't simply know which data register interrupt
should be enable from static data defined in device tree.

> 
> >>
> >> > short messages
> >> > Transmit register(s) can be used to pass short messages from one to
> >> > four words in length. For example, when a four-word message is
> >> > desired, only one of the registers needs to have its corresponding
> >> > interrupt enable bit set at the receiver side; the message’s first
> >> > three words are written to the registers whose interrupt is masked,
> >> > and the fourth word is written to the other register (which
> >> > triggers an
> >> interrupt at the receiver side).
> >> >
> >> > The reference manual is at here: (Chapter 42 Messaging Unit (MU)
> >> >
> >>
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fw
> >> ww
> >> > .nxp.com%2Fdocs%2Fen%2Freference-
> >> manual%2FIMX6ULRM.pdf&amp;data=02%7C0
> >> >
> >>
> 1%7Caisheng.dong%40nxp.com%7Cef349d7c9caf46c4260008d5f30e9ef5%7C6
> >> 86ea1
> >> >
> >>
> d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636682166494860332&amp;sdat
> >> a=54rE
> >> >
> >>
> iDm%2BGD6EY8NE64ck1LXVGoUCtWYrHXPoWqcujUo%3D&amp;reserved=0
> >> >
> >> > And SCU firmware does use MU in above way specified by reference
> >> manual.
> >> > Even from HW point of view, it's still one channel only mailbox.
> >> >
> >> Please realise that any manual is written by a mere mortal afterall.
> >> How the controller works is set in stone, but how the controller can
> >> be used ... is just someones suggestion.
> >>
> >> The approach I suggest above, conforms to the api and prevents a
> >> provider dancing to the tunes of a user.
> >
> > First of all, really appreciate for your suggestions.
> > It may be hard to find a common binding with the same mbox-cells.
> > It looks like we just need a property is distinguish how the Mailbox
> > is used In one channel or multi channel mode.
> >
> I get the idea you were ready to see the code merged in the coming window
> and be done with it. And now it feels lazy to change the code.

For me, I'm glad to change if we have a clear better solution.
And I do appreciate your suggestion and review time.

Why I'm a bit hesitate now is because your suggestion may not work for SCU,
(see above explanation), but it does work for generic M4 case.
But we' re now going to support both protocol in one mailbox driver.
Any suggestion on how to treat them properly if change the binding?

> I am sorry, but I can't allow controller drivers "emulating" some mode for a
> client driver. That is moving a part of client code into the controller driver.
> 

Okay, let's figure out it first.
Would you be more specific on what "emulating" did you mean in controller driver?
Sending up to 4 words capability in one channel mode as specified in reference manual?
That's I'm a bit confusing because I thought it's HW capability independent of client driver.

Or anything else?

> 
> > Directly checking mbox-cells seems the most easy way and it is already
> > Acked by Rob. Can't this way be Okay to you?
> >
> Rob is indeed far better than I am. But he also has to look into 100 other
> subsystems, whereas I am only looking into mailbox. I have time to dig into
> your datasheets. I believe Rob will point out anything wrong I suggest.
> 

Yes, you're in the fair enough authority to judge it. Thanks for your effort.

> BTW, the submitted driver doesn't even support the SCU usecase. Why the
> bindings?

Because that patch is firstly Acked by Rob. Others are reworked and ready to be sent
out against this patch series. But it seems we still have unresolved issues now as you
pointed out. We can first resolve them. Or do you need me to send out for your reference?

Regards
Dong Aisheng


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

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

[Jassi Brar]

On Fri, Jul 27, 2018 at 11:30 AM, A.s. Dong <aisheng.dong@nxp.com> wrote:
>> -----Original Message-----
>> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
>> Sent: Friday, July 27, 2018 12:56 PM
>> To: A.s. Dong <aisheng.dong@nxp.com>
>> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; , Sascha Hauer
>> <kernel@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>; , 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 v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU
>> channel support
>>
>> On Fri, Jul 27, 2018 at 9:32 AM, A.s. Dong <aisheng.dong@nxp.com> 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.
>> >> >> >
>> >> >> So the hardware actually supports 4 channels.
>> >> >>
>> >> >> > -- #mbox-cells:  Must be 0. Number of cells in a mailbox
>> >> >> > +- #mbox-cells:  Must be:
>> >> >> > +               0 - for single channel mode. i.MX8* SCU protocol specific.
>> >> >> > +               1 - for multichannel (generic) mode.
>> >> >> > +
>> >> >> No, please.
>> >> >> DT bindings should reflect the real hardware, and not the software
>> >> >> mode we want the driver to work in.
>> >> >> Please define mbox-cells=1  and have the i.MX8* platform always
>> >> >> ask for channel-0.
>> >> >
>> >> > The reality is that MU hardware does not define channels in
>> >> > reference
>> >> manual.
>> >> > However, it does have four separate data tx/rx register which can
>> >> > be used as 'virtual' channels which is supported by this current driver.
>> >> >
>> >> > See below HW description from the reference manual:
>> >> > For messaging, the MU has four, 32-bit write-only transmit
>> >> > registers and four, 32-bit read-only receive registers on the
>> >> > Processor B and Processor A-sides. These registers are used for
>> >> > sending messages to each
>> >> other.
>> >> >
>> >> For a while please forget the protocol(user) level usage, and
>> >> consider only what your h/w is.
>> >>
>> >> MU has 4 pairs of TX_Reg + TX_IRQ, and 4 pairs of RX_Reg + RX_IRQ.
>> >> (MU also has 4 "doorbell" type channels that it calls GP, but those
>> >> are not managed here, so lets not worry atm).
>> >>
>> >> By definition, a mailbox channel is simply a signal, optionally with
>> >> data attached. So, MU has 4 TX and 4 RX channels.
>> >>
>> >> The MU driver should populate 8 unidirectional (4 Tx and 4 RX)
>> >> channels and set each tx/rx operation to trigger the corresponding
>> >> interrupt. This is not my whim, this is how the controller is!
>> >>
>> >
>> > This looks like reasonable to me, theoretically.
>> > Just not sure whether it's necessary to support it because we probably
>> > will never use like that in reality, then it might become meaningless
>> > complicity introduced and error prone.
>> >
>> It _is_ necessary to write controller driver independent of client drivers.
>>
>
> Yes, that's true. What if we think we're writing driver against HW capabilities
> which support 4 pair of channel links(tx/rx)?
> It looks like independent of client drivers and may make life easily.
> Does it make sense?
>
No, that would be emulation.
Why doesn't a usb device controller (udc) driver emulate FSG/HID etc,
by "thinking" it has a hardware backed storage/keyboard? It doesn't
because that is the job of upper protocol layer.


>>
>> > And AFAIK ARM MHU is doing the same way as MU which looks like also
>> unidirectional channel.
>> >
>> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Finfo
>> >
>> center.arm.com%2Fhelp%2Findex.jsp%3Ftopic%3D%2Fcom.arm.doc.ddi0515
>> b%2F
>> >
>> CHDGBGIF.html&amp;data=02%7C01%7Caisheng.dong%40nxp.com%7Cb728
>> 16362983
>> >
>> 4208f9e908d5f37d3e00%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%
>> 7C6366
>> >
>> 82641593785009&amp;sdata=VsG0oXxEObPRwT5KVss2eZthSTMTR2%2BMrv
>> PqhDUpPGU
>> > %3D&amp;reserved=0
>> > drivers/mailbox/arm_mhu.c
>> >
>> MHU driver is doing exactly what the data sheet says. I know because I wrote
>> the driver.
>>
>
> Hmm... Maybe I missed something, but seems no from what I see:
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0922g/CCHHGIAH.html
>
That is not the MHU datasheet.

Read Section 3.6 at page-3-41 of
http://infocenter.arm.com/help/topic/com.arm.doc.ddi0515b/DDI0515B_juno_arm_development_platform_soc_trm.pdf

This is another reason I can't let a bad code (emulation code)
through, because people start looking for examples to justify their
implementation rather than fixing it.


>>
>> >> The SCU is poorly implemented as it ignores 3 irqs and club all 4
>> >> register together (there are many other cons of this approach but
>> >> lets not get into that).
>> >> Personally, I would push-back on such a bad design. But if you claim
>> >> you have no choice but to support SCU as such, the work around could
>> >> be simpler than defining a new "scu mode" altogether.
>> >>
>> >
>> > This is one of the recommended ways designed in HW reference manual
>> > and it allows to send frame information up to 4 words without using shared
>> memory.
>> > SCU just follows it, so it's hard to believe it's a bad design.
>> >
>> >> #mbox-cells:  Must be 3.
>> >>                       First cell is 1 for TX and 0 for RX channel
>> >>                       Second cell is index of the channel [0,1,2 or 3]
>> >>                       Third cell is 1 if the channel triggers an IRQ,
>> >> 0 if not. That is ACR.RIE/TIE bits are set or not.
>> >>
>> >> Normal clients would always request a channel with irqs enabled.
>> >> The SCU client would request all 8 channels -- TX/RX[0,1,2] with irqs
>> >> disabled, TX/RX[3] with irqs enabled. And SCU will read/write 4 word
>> >> data over 4 rx/tx channels, instead of the virtually concocted one.
>> >>
>> >
>> > It may work If SCU protocol data length is fixed to 4 words. However,
>> > it's length is flexible for different SVC service. That means this binding
>> won't work for SCU.
>> > And it will introduce much complexities during the implementation.
>> >
>> > Instead, we're using polling mode for both TX/RX and the data size is
>> > stored in the msg header and sending msgs using all 4 data tx/rx registers
>> as a channel fifo.
>> >
>> You first give me the "Passing short messages" usecase (quite a bad
>> one) and ask how it can work. When I give you a solution, you effectively say
>> "well, my usecase isn't even that". I feel I just wasted my time.
>>
>
> Sorry for may not clear, "Passing short message' usecase is to tell how the
> HW is working on one channel mode sending up to 4 words in one time
> As specified in reference manual.
>
> SCU does work that way, the only difference is it's using polling mode
> rather than interrupt driven.  The point is the data size may be different
> for each msg, so we can't simply know which data register interrupt
> should be enable from static data defined in device tree.
>
And you think passing variable data through registers is a better idea
than passing packets via shared-memory?


>>
>> >>
>> >> > short messages
>> >> > Transmit register(s) can be used to pass short messages from one to
>> >> > four words in length. For example, when a four-word message is
>> >> > desired, only one of the registers needs to have its corresponding
>> >> > interrupt enable bit set at the receiver side; the message’s first
>> >> > three words are written to the registers whose interrupt is masked,
>> >> > and the fourth word is written to the other register (which
>> >> > triggers an
>> >> interrupt at the receiver side).
>> >> >
>> >> > The reference manual is at here: (Chapter 42 Messaging Unit (MU)
>> >> >
>> >>
>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fw
>> >> ww
>> >> > .nxp.com%2Fdocs%2Fen%2Freference-
>> >> manual%2FIMX6ULRM.pdf&amp;data=02%7C0
>> >> >
>> >>
>> 1%7Caisheng.dong%40nxp.com%7Cef349d7c9caf46c4260008d5f30e9ef5%7C6
>> >> 86ea1
>> >> >
>> >>
>> d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636682166494860332&amp;sdat
>> >> a=54rE
>> >> >
>> >>
>> iDm%2BGD6EY8NE64ck1LXVGoUCtWYrHXPoWqcujUo%3D&amp;reserved=0
>> >> >
>> >> > And SCU firmware does use MU in above way specified by reference
>> >> manual.
>> >> > Even from HW point of view, it's still one channel only mailbox.
>> >> >
>> >> Please realise that any manual is written by a mere mortal afterall.
>> >> How the controller works is set in stone, but how the controller can
>> >> be used ... is just someones suggestion.
>> >>
>> >> The approach I suggest above, conforms to the api and prevents a
>> >> provider dancing to the tunes of a user.
>> >
>> > First of all, really appreciate for your suggestions.
>> > It may be hard to find a common binding with the same mbox-cells.
>> > It looks like we just need a property is distinguish how the Mailbox
>> > is used In one channel or multi channel mode.
>> >
>> I get the idea you were ready to see the code merged in the coming window
>> and be done with it. And now it feels lazy to change the code.
>
> For me, I'm glad to change if we have a clear better solution.
> And I do appreciate your suggestion and review time.
>
> Why I'm a bit hesitate now is because your suggestion may not work for SCU,
> (see above explanation), but it does work for generic M4 case.
> But we' re now going to support both protocol in one mailbox driver.
> Any suggestion on how to treat them properly if change the binding?
>
In your last post you said "This looks like reasonable to me, theoretically"
My suggestion is the same because I don't see why it won't work.


>> I am sorry, but I can't allow controller drivers "emulating" some mode for a
>> client driver. That is moving a part of client code into the controller driver.
>>
>
> Okay, let's figure out it first.
> Would you be more specific on what "emulating" did you mean in controller driver?
> Sending up to 4 words capability in one channel mode as specified in reference manual?
> That's I'm a bit confusing because I thought it's HW capability independent of client driver.
>
> Or anything else?
>
Emulation means pretending something that we are not.

The hardware has 8 unidirectional channels. But your protocol (SCU
implementation) assumes there is one _virtual_ channel that has 4
registers and 1/0 irq --- which is not true. Instead of fixing the
assumption in your protocol or emulating the virtual channel in the
protocol level (user of a MU), you want to add code in the controller
driver that ignores interrupts and club the 4 independent channels
together.

There is no end to protocols and their kinky assumptions, adding
"xyz-mode" support for each protocol isn't scalable.


>>
>> > Directly checking mbox-cells seems the most easy way and it is already
>> > Acked by Rob. Can't this way be Okay to you?
>> >
>> Rob is indeed far better than I am. But he also has to look into 100 other
>> subsystems, whereas I am only looking into mailbox. I have time to dig into
>> your datasheets. I believe Rob will point out anything wrong I suggest.
>>
>
> Yes, you're in the fair enough authority to judge it. Thanks for your effort.
>
>> BTW, the submitted driver doesn't even support the SCU usecase. Why the
>> bindings?
>
> Because that patch is firstly Acked by Rob. Others are reworked and ready to be sent
> out against this patch series. But it seems we still have unresolved issues now as you
> pointed out. We can first resolve them. Or do you need me to send out for your reference?
>
I am sure Rob took the best decision at the time with whatever
information provided to him.
Now, after reading the datasheet, we have the option to avoid
implementing consumer code in the provider driver.

Thanks.

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

RE: [PATCH v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU channel support

[A.s. Dong]

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> Sent: Friday, July 27, 2018 2:47 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; , Sascha Hauer
> <kernel@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>; , 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 v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU
> channel support
> 
> On Fri, Jul 27, 2018 at 11:30 AM, A.s. Dong <aisheng.dong@nxp.com> wrote:
> >> -----Original Message-----
> >> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> >> Sent: Friday, July 27, 2018 12:56 PM
> >> To: A.s. Dong <aisheng.dong@nxp.com>
> >> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; , Sascha Hauer
> >> <kernel@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>; ,
> >> 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 v7 3/6] dt-bindings: mailbox: imx-mu: add generic
> >> MU channel support
> >>
> >> On Fri, Jul 27, 2018 at 9:32 AM, A.s. Dong <aisheng.dong@nxp.com> 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.
> >> >> >> >
> >> >> >> So the hardware actually supports 4 channels.
> >> >> >>
> >> >> >> > -- #mbox-cells:  Must be 0. Number of cells in a mailbox
> >> >> >> > +- #mbox-cells:  Must be:
> >> >> >> > +               0 - for single channel mode. i.MX8* SCU protocol specific.
> >> >> >> > +               1 - for multichannel (generic) mode.
> >> >> >> > +
> >> >> >> No, please.
> >> >> >> DT bindings should reflect the real hardware, and not the
> >> >> >> software mode we want the driver to work in.
> >> >> >> Please define mbox-cells=1  and have the i.MX8* platform always
> >> >> >> ask for channel-0.
> >> >> >
> >> >> > The reality is that MU hardware does not define channels in
> >> >> > reference
> >> >> manual.
> >> >> > However, it does have four separate data tx/rx register which
> >> >> > can be used as 'virtual' channels which is supported by this current
> driver.
> >> >> >
> >> >> > See below HW description from the reference manual:
> >> >> > For messaging, the MU has four, 32-bit write-only transmit
> >> >> > registers and four, 32-bit read-only receive registers on the
> >> >> > Processor B and Processor A-sides. These registers are used for
> >> >> > sending messages to each
> >> >> other.
> >> >> >
> >> >> For a while please forget the protocol(user) level usage, and
> >> >> consider only what your h/w is.
> >> >>
> >> >> MU has 4 pairs of TX_Reg + TX_IRQ, and 4 pairs of RX_Reg + RX_IRQ.
> >> >> (MU also has 4 "doorbell" type channels that it calls GP, but
> >> >> those are not managed here, so lets not worry atm).
> >> >>
> >> >> By definition, a mailbox channel is simply a signal, optionally
> >> >> with data attached. So, MU has 4 TX and 4 RX channels.
> >> >>
> >> >> The MU driver should populate 8 unidirectional (4 Tx and 4 RX)
> >> >> channels and set each tx/rx operation to trigger the corresponding
> >> >> interrupt. This is not my whim, this is how the controller is!
> >> >>
> >> >
> >> > This looks like reasonable to me, theoretically.
> >> > Just not sure whether it's necessary to support it because we
> >> > probably will never use like that in reality, then it might become
> >> > meaningless complicity introduced and error prone.
> >> >
> >> It _is_ necessary to write controller driver independent of client drivers.
> >>
> >
> > Yes, that's true. What if we think we're writing driver against HW
> > capabilities which support 4 pair of channel links(tx/rx)?
> > It looks like independent of client drivers and may make life easily.
> > Does it make sense?
> >
> No, that would be emulation.
> Why doesn't a usb device controller (udc) driver emulate FSG/HID etc, by
> "thinking" it has a hardware backed storage/keyboard? It doesn't because
> that is the job of upper protocol layer.
> 

Sorry I'm not quite familiar with USB device.
IMHO the HW supports many capabilities, but it does not mean
we need support them all. For MU case, a pair of channel link capable of both tx/rx
seems like a better using. It's irrelevant of client drivers. It's simply that HW supports
different kind of modes (one channel mode, one link mode, separate per register mode)
and we just support link mode.

Another reason is I doubt that we may never use per register mode in a different register pair
in the future. For example:
AP:
node {
	...
	// cell 0 meaning: 0: tx 1: rx	cell1 meaning: channel id
	Mboxes = <&mbox 0 1 &mbox 1 2>
	Mbox-names = "tx", "rx";
>

M4:
node {
	...
	Mboxes = <&mbox 0 2 &mbox 1 1>
	Mbox-names = "tx", "rx";
>
This make things complicated and error prone as I said before.

But that's just my understanding and may overlook something, if you still think
we should do exactly as above, I will not against it because it does work for M4 case.

Then the left question is how we handle SCU case?

> >>
> >> > And AFAIK ARM MHU is doing the same way as MU which looks like also
> >> unidirectional channel.
> >> >
> >>
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Finf
> >> o
> >> >
> >>
> center.arm.com%2Fhelp%2Findex.jsp%3Ftopic%3D%2Fcom.arm.doc.ddi0515
> >> b%2F
> >> >
> >>
> CHDGBGIF.html&amp;data=02%7C01%7Caisheng.dong%40nxp.com%7Cb728
> >> 16362983
> >> >
> >>
> 4208f9e908d5f37d3e00%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%
> >> 7C6366
> >> >
> >>
> 82641593785009&amp;sdata=VsG0oXxEObPRwT5KVss2eZthSTMTR2%2BMrv
> >> PqhDUpPGU
> >> > %3D&amp;reserved=0
> >> > drivers/mailbox/arm_mhu.c
> >> >
> >> MHU driver is doing exactly what the data sheet says. I know because
> >> I wrote the driver.
> >>
> >
> > Hmm... Maybe I missed something, but seems no from what I see:
> >
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Finfo
> >
> center.arm.com%2Fhelp%2Findex.jsp%3Ftopic%3D%2Fcom.arm.doc.dui0922
> g%2F
> >
> CCHHGIAH.html&amp;data=02%7C01%7Caisheng.dong%40nxp.com%7Cd2b1
> 23b89dae
> >
> 4a00cb7108d5f38cb611%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%
> 7C6366
> >
> 82708036553137&amp;sdata=4SMUpH%2FO9MWArC%2BjbPy%2BrbNAqUla
> o9IezKUi7UX
> > gIyQ%3D&amp;reserved=0
> >
> That is not the MHU datasheet.
> 
> Read Section 3.6 at page-3-41 of
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Finfo
> center.arm.com%2Fhelp%2Ftopic%2Fcom.arm.doc.ddi0515b%2FDDI0515B_j
> uno_arm_development_platform_soc_trm.pdf&amp;data=02%7C01%7Cais
> heng.dong%40nxp.com%7Cd2b123b89dae4a00cb7108d5f38cb611%7C686ea1
> d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C636682708036553137&amp;sdat
> a=eUd75VnwEs44ZoVV0n7R1G3UfAH%2B%2BpeoGns7Mq5UBN8%3D&amp;
> reserved=0
> 
> This is another reason I can't let a bad code (emulation code) through,
> because people start looking for examples to justify their implementation
> rather than fixing it.
> 

I'm a bit confusing....
The section 3.6 you pointed is the MHU register description. It does not conflict
with what I see from ARM doc center that each physical channel is unidirectional.

See below:
Chan 1:
0x000 SCP_INTR_L_STAT
0x008 SCP_INTR_L_SET
0x010 SCP _INTR_L_CLEAR

Chan 2:
0x020 SCP_INTR_H_STAT
0x028 SCP_INTR_H_SET
0x030 SCP _INTR_H_CLEAR

Chan 3:
0x100 CPU_INTR_L_STAT
0x108 CPU_INTR_L_SET
0x110 CPU_INTR_L_CLEAR

Chan 4:
0x120 CPU_INTR_H_STAT
0x128 CPU_INTR_H_SET
0x130 CPU_INTR_H_CLEAR

Chan 5:
0x200 SCP_INTR_S_STAT
0x208 SCP_INTR_S_SET
0x210 SCP _INTR_S_CLEAR

Chan 6:
0x300 CPU_INTR_S_STAT
0x308 CPU_INTR_S_SET
0x310 CPU_INTR_S_CLEAR

And the driver compose them into 3 channel links (lp, hp and sec).
Am I wrong?

> 
> >>
> >> >> The SCU is poorly implemented as it ignores 3 irqs and club all 4
> >> >> register together (there are many other cons of this approach but
> >> >> lets not get into that).
> >> >> Personally, I would push-back on such a bad design. But if you
> >> >> claim you have no choice but to support SCU as such, the work
> >> >> around could be simpler than defining a new "scu mode" altogether.
> >> >>
> >> >
> >> > This is one of the recommended ways designed in HW reference
> manual
> >> > and it allows to send frame information up to 4 words without using
> >> > shared
> >> memory.
> >> > SCU just follows it, so it's hard to believe it's a bad design.
> >> >
> >> >> #mbox-cells:  Must be 3.
> >> >>                       First cell is 1 for TX and 0 for RX channel
> >> >>                       Second cell is index of the channel [0,1,2 or 3]
> >> >>                       Third cell is 1 if the channel triggers an
> >> >> IRQ,
> >> >> 0 if not. That is ACR.RIE/TIE bits are set or not.
> >> >>
> >> >> Normal clients would always request a channel with irqs enabled.
> >> >> The SCU client would request all 8 channels -- TX/RX[0,1,2] with
> >> >> irqs disabled, TX/RX[3] with irqs enabled. And SCU will read/write
> >> >> 4 word data over 4 rx/tx channels, instead of the virtually concocted
> one.
> >> >>
> >> >
> >> > It may work If SCU protocol data length is fixed to 4 words.
> >> > However, it's length is flexible for different SVC service. That
> >> > means this binding
> >> won't work for SCU.
> >> > And it will introduce much complexities during the implementation.
> >> >
> >> > Instead, we're using polling mode for both TX/RX and the data size
> >> > is stored in the msg header and sending msgs using all 4 data tx/rx
> >> > registers
> >> as a channel fifo.
> >> >
> >> You first give me the "Passing short messages" usecase (quite a bad
> >> one) and ask how it can work. When I give you a solution, you
> >> effectively say "well, my usecase isn't even that". I feel I just wasted my
> time.
> >>
> >
> > Sorry for may not clear, "Passing short message' usecase is to tell
> > how the HW is working on one channel mode sending up to 4 words in one
> > time As specified in reference manual.
> >
> > SCU does work that way, the only difference is it's using polling mode
> > rather than interrupt driven.  The point is the data size may be
> > different for each msg, so we can't simply know which data register
> > interrupt should be enable from static data defined in device tree.
> >
> And you think passing variable data through registers is a better idea than
> passing packets via shared-memory?
> 

You got me. :-)
I've no idea about which one is better. The problem is SCU firmware is already
there passing packets through data registers, we have no way to change it.

> 
> >>
> >> >>
> >> >> > short messages
> >> >> > Transmit register(s) can be used to pass short messages from one
> >> >> > to four words in length. For example, when a four-word message
> >> >> > is desired, only one of the registers needs to have its
> >> >> > corresponding interrupt enable bit set at the receiver side; the
> >> >> > message’s first three words are written to the registers whose
> >> >> > interrupt is masked, and the fourth word is written to the other
> >> >> > register (which triggers an
> >> >> interrupt at the receiver side).
> >> >> >
> >> >> > The reference manual is at here: (Chapter 42 Messaging Unit (MU)
> >> >> >
> >> >>
> >>
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fw
> >> >> ww
> >> >> > .nxp.com%2Fdocs%2Fen%2Freference-
> >> >> manual%2FIMX6ULRM.pdf&amp;data=02%7C0
> >> >> >
> >> >>
> >>
> 1%7Caisheng.dong%40nxp.com%7Cef349d7c9caf46c4260008d5f30e9ef5%7C6
> >> >> 86ea1
> >> >> >
> >> >>
> >>
> d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636682166494860332&amp;sdat
> >> >> a=54rE
> >> >> >
> >> >>
> >>
> iDm%2BGD6EY8NE64ck1LXVGoUCtWYrHXPoWqcujUo%3D&amp;reserved=0
> >> >> >
> >> >> > And SCU firmware does use MU in above way specified by reference
> >> >> manual.
> >> >> > Even from HW point of view, it's still one channel only mailbox.
> >> >> >
> >> >> Please realise that any manual is written by a mere mortal afterall.
> >> >> How the controller works is set in stone, but how the controller
> >> >> can be used ... is just someones suggestion.
> >> >>
> >> >> The approach I suggest above, conforms to the api and prevents a
> >> >> provider dancing to the tunes of a user.
> >> >
> >> > First of all, really appreciate for your suggestions.
> >> > It may be hard to find a common binding with the same mbox-cells.
> >> > It looks like we just need a property is distinguish how the
> >> > Mailbox is used In one channel or multi channel mode.
> >> >
> >> I get the idea you were ready to see the code merged in the coming
> >> window and be done with it. And now it feels lazy to change the code.
> >
> > For me, I'm glad to change if we have a clear better solution.
> > And I do appreciate your suggestion and review time.
> >
> > Why I'm a bit hesitate now is because your suggestion may not work for
> > SCU, (see above explanation), but it does work for generic M4 case.
> > But we' re now going to support both protocol in one mailbox driver.
> > Any suggestion on how to treat them properly if change the binding?
> >
> In your last post you said "This looks like reasonable to me, theoretically"
> My suggestion is the same because I don't see why it won't work.
> 

Sorry for not clear. I mean it's reasonable for M4 generic using case.
But not for SCU case.

> 
> >> I am sorry, but I can't allow controller drivers "emulating" some
> >> mode for a client driver. That is moving a part of client code into the
> controller driver.
> >>
> >
> > Okay, let's figure out it first.
> > Would you be more specific on what "emulating" did you mean in
> controller driver?
> > Sending up to 4 words capability in one channel mode as specified in
> reference manual?
> > That's I'm a bit confusing because I thought it's HW capability independent
> of client driver.
> >
> > Or anything else?
> >
> Emulation means pretending something that we are not.
> 
> The hardware has 8 unidirectional channels. But your protocol (SCU
> implementation) assumes there is one _virtual_ channel that has 4 registers
> and 1/0 irq --- which is not true. Instead of fixing the assumption in your
> protocol or emulating the virtual channel in the protocol level (user of a MU),
> you want to add code in the controller driver that ignores interrupts and club
> the 4 independent channels together.
> 

This stucks me. This is how the hardware is designed  and suggested to use
in hardware reference manual. And now you're telling me this is wrong and
we should not use the design in reference manual...

> There is no end to protocols and their kinky assumptions, adding "xyz-mode"
> support for each protocol isn't scalable.
> 

This is also how Sascha suggested me to move to mailbox to support
both protocols and handle them differently in mailbox driver.

Honestly I was hesitated to do that before because I doubt the value
we can gain if switching to mailbox besides the unnecessary complexity
introduced and performance downgrading (extra execution of a few
unnecessary code in mailbox API and It's even worse if use 8 separate
channels for SCU, comparing to original only tens of lines of library API way),
but Sascha insisted...

The mailbox itself is somehow protocol specific (doorbell, signal, data packet and etc)
It's hard for us to find a common way to support two totally different protocols.
So the controller needs to know different protocol it is transferring.

> 
> >>
> >> > Directly checking mbox-cells seems the most easy way and it is
> >> > already Acked by Rob. Can't this way be Okay to you?
> >> >
> >> Rob is indeed far better than I am. But he also has to look into 100
> >> other subsystems, whereas I am only looking into mailbox. I have time
> >> to dig into your datasheets. I believe Rob will point out anything wrong I
> suggest.
> >>
> >
> > Yes, you're in the fair enough authority to judge it. Thanks for your effort.
> >
> >> BTW, the submitted driver doesn't even support the SCU usecase. Why
> >> the bindings?
> >
> > Because that patch is firstly Acked by Rob. Others are reworked and
> > ready to be sent out against this patch series. But it seems we still
> > have unresolved issues now as you pointed out. We can first resolve them.
> Or do you need me to send out for your reference?
> >
> I am sure Rob took the best decision at the time with whatever information
> provided to him.
> Now, after reading the datasheet, we have the option to avoid implementing
> consumer code in the provider driver.

I have one doubt, irrelevant of SCU protocol, from the datasheet, the hardware
does support transfer message up to 4 words in one channel mode and it is the
recommended way in datasheet for less than 4 words frame transferring,
why switching to mailbox framework, we can't use it now?
And makes us lose the HW capability.

If that, what's meaning for us to switch to mailbox framework?

Regards
Dong Aisheng

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

RE: [PATCH v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU channel support

[A.s. Dong]

> -----Original Message-----
> From: A.s. Dong
> Sent: Friday, July 27, 2018 4:42 PM
> To: Jassi Brar <jassisinghbrar@gmail.com>
> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; , Sascha Hauer
> <kernel@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>; , 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 v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU
> channel support
> 
> > -----Original Message-----
> > From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> > Sent: Friday, July 27, 2018 2:47 PM
> > To: A.s. Dong <aisheng.dong@nxp.com>
> > Cc: Oleksij Rempel <o.rempel@pengutronix.de>; , Sascha Hauer
> > <kernel@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>; ,
> > 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 v7 3/6] dt-bindings: mailbox: imx-mu: add generic
> > MU channel support
> >
> > On Fri, Jul 27, 2018 at 11:30 AM, A.s. Dong <aisheng.dong@nxp.com> wrote:
> > >> -----Original Message-----
> > >> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> > >> Sent: Friday, July 27, 2018 12:56 PM
> > >> To: A.s. Dong <aisheng.dong@nxp.com>
> > >> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; , Sascha Hauer
> > >> <kernel@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>; ,
> > >> 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 v7 3/6] dt-bindings: mailbox: imx-mu: add
> > >> generic MU channel support
> > >>
> > >> On Fri, Jul 27, 2018 at 9:32 AM, A.s. Dong <aisheng.dong@nxp.com>
> 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.
> > >> >> >> >
> > >> >> >> So the hardware actually supports 4 channels.
> > >> >> >>
> > >> >> >> > -- #mbox-cells:  Must be 0. Number of cells in a mailbox
> > >> >> >> > +- #mbox-cells:  Must be:
> > >> >> >> > +               0 - for single channel mode. i.MX8* SCU protocol
> specific.
> > >> >> >> > +               1 - for multichannel (generic) mode.
> > >> >> >> > +
> > >> >> >> No, please.
> > >> >> >> DT bindings should reflect the real hardware, and not the
> > >> >> >> software mode we want the driver to work in.
> > >> >> >> Please define mbox-cells=1  and have the i.MX8* platform
> > >> >> >> always ask for channel-0.
> > >> >> >
> > >> >> > The reality is that MU hardware does not define channels in
> > >> >> > reference
> > >> >> manual.
> > >> >> > However, it does have four separate data tx/rx register which
> > >> >> > can be used as 'virtual' channels which is supported by this
> > >> >> > current
> > driver.
> > >> >> >
> > >> >> > See below HW description from the reference manual:
> > >> >> > For messaging, the MU has four, 32-bit write-only transmit
> > >> >> > registers and four, 32-bit read-only receive registers on the
> > >> >> > Processor B and Processor A-sides. These registers are used
> > >> >> > for sending messages to each
> > >> >> other.
> > >> >> >
> > >> >> For a while please forget the protocol(user) level usage, and
> > >> >> consider only what your h/w is.
> > >> >>
> > >> >> MU has 4 pairs of TX_Reg + TX_IRQ, and 4 pairs of RX_Reg + RX_IRQ.
> > >> >> (MU also has 4 "doorbell" type channels that it calls GP, but
> > >> >> those are not managed here, so lets not worry atm).
> > >> >>
> > >> >> By definition, a mailbox channel is simply a signal, optionally
> > >> >> with data attached. So, MU has 4 TX and 4 RX channels.
> > >> >>
> > >> >> The MU driver should populate 8 unidirectional (4 Tx and 4 RX)
> > >> >> channels and set each tx/rx operation to trigger the
> > >> >> corresponding interrupt. This is not my whim, this is how the
> controller is!
> > >> >>
> > >> >
> > >> > This looks like reasonable to me, theoretically.
> > >> > Just not sure whether it's necessary to support it because we
> > >> > probably will never use like that in reality, then it might
> > >> > become meaningless complicity introduced and error prone.
> > >> >
> > >> It _is_ necessary to write controller driver independent of client drivers.
> > >>
> > >
> > > Yes, that's true. What if we think we're writing driver against HW
> > > capabilities which support 4 pair of channel links(tx/rx)?
> > > It looks like independent of client drivers and may make life easily.
> > > Does it make sense?
> > >
> > No, that would be emulation.
> > Why doesn't a usb device controller (udc) driver emulate FSG/HID etc,
> > by "thinking" it has a hardware backed storage/keyboard? It doesn't
> > because that is the job of upper protocol layer.
> >
> 
> Sorry I'm not quite familiar with USB device.
> IMHO the HW supports many capabilities, but it does not mean we need
> support them all. For MU case, a pair of channel link capable of both tx/rx
> seems like a better using. It's irrelevant of client drivers. It's simply that HW
> supports different kind of modes (one channel mode, one link mode,
> separate per register mode) and we just support link mode.
> 
> Another reason is I doubt that we may never use per register mode in a
> different register pair in the future. For example:
> AP:
> node {
> 	...
> 	// cell 0 meaning: 0: tx 1: rx	cell1 meaning: channel id
> 	Mboxes = <&mbox 0 1 &mbox 1 2>
> 	Mbox-names = "tx", "rx";
> >
> 
> M4:
> node {
> 	...
> 	Mboxes = <&mbox 0 2 &mbox 1 1>
> 	Mbox-names = "tx", "rx";
> >
> This make things complicated and error prone as I said before.
> 
> But that's just my understanding and may overlook something, if you still
> think we should do exactly as above, I will not against it because it does work
> for M4 case.
> 
> Then the left question is how we handle SCU case?
> 
> > >>
> > >> > And AFAIK ARM MHU is doing the same way as MU which looks like
> > >> > also
> > >> unidirectional channel.
> > >> >
> > >>
> > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Finf
> > >> o
> > >> >
> > >>
> >
> center.arm.com%2Fhelp%2Findex.jsp%3Ftopic%3D%2Fcom.arm.doc.ddi0515
> > >> b%2F
> > >> >
> > >>
> >
> CHDGBGIF.html&amp;data=02%7C01%7Caisheng.dong%40nxp.com%7Cb728
> > >> 16362983
> > >> >
> > >>
> > 4208f9e908d5f37d3e00%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%
> > >> 7C6366
> > >> >
> > >>
> >
> 82641593785009&amp;sdata=VsG0oXxEObPRwT5KVss2eZthSTMTR2%2BMrv
> > >> PqhDUpPGU
> > >> > %3D&amp;reserved=0
> > >> > drivers/mailbox/arm_mhu.c
> > >> >
> > >> MHU driver is doing exactly what the data sheet says. I know
> > >> because I wrote the driver.
> > >>
> > >
> > > Hmm... Maybe I missed something, but seems no from what I see:
> > >
> >
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Finfo
> > >
> >
> center.arm.com%2Fhelp%2Findex.jsp%3Ftopic%3D%2Fcom.arm.doc.dui0922
> > g%2F
> > >
> >
> CCHHGIAH.html&amp;data=02%7C01%7Caisheng.dong%40nxp.com%7Cd2b1
> > 23b89dae
> > >
> > 4a00cb7108d5f38cb611%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%
> > 7C6366
> > >
> >
> 82708036553137&amp;sdata=4SMUpH%2FO9MWArC%2BjbPy%2BrbNAqUla
> > o9IezKUi7UX
> > > gIyQ%3D&amp;reserved=0
> > >
> > That is not the MHU datasheet.
> >
> > Read Section 3.6 at page-3-41 of
> >
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Finfo
> >
> center.arm.com%2Fhelp%2Ftopic%2Fcom.arm.doc.ddi0515b%2FDDI0515B_j
> >
> uno_arm_development_platform_soc_trm.pdf&amp;data=02%7C01%7Cais
> >
> heng.dong%40nxp.com%7Cd2b123b89dae4a00cb7108d5f38cb611%7C686ea1
> >
> d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C636682708036553137&amp;sdat
> >
> a=eUd75VnwEs44ZoVV0n7R1G3UfAH%2B%2BpeoGns7Mq5UBN8%3D&amp;
> > reserved=0
> >
> > This is another reason I can't let a bad code (emulation code)
> > through, because people start looking for examples to justify their
> > implementation rather than fixing it.
> >
> 
> I'm a bit confusing....
> The section 3.6 you pointed is the MHU register description. It does not
> conflict with what I see from ARM doc center that each physical channel is
> unidirectional.
> 
> See below:
> Chan 1:
> 0x000 SCP_INTR_L_STAT
> 0x008 SCP_INTR_L_SET
> 0x010 SCP _INTR_L_CLEAR
> 
> Chan 2:
> 0x020 SCP_INTR_H_STAT
> 0x028 SCP_INTR_H_SET
> 0x030 SCP _INTR_H_CLEAR
> 
> Chan 3:
> 0x100 CPU_INTR_L_STAT
> 0x108 CPU_INTR_L_SET
> 0x110 CPU_INTR_L_CLEAR
> 
> Chan 4:
> 0x120 CPU_INTR_H_STAT
> 0x128 CPU_INTR_H_SET
> 0x130 CPU_INTR_H_CLEAR
> 
> Chan 5:
> 0x200 SCP_INTR_S_STAT
> 0x208 SCP_INTR_S_SET
> 0x210 SCP _INTR_S_CLEAR
> 
> Chan 6:
> 0x300 CPU_INTR_S_STAT
> 0x308 CPU_INTR_S_SET
> 0x310 CPU_INTR_S_CLEAR
> 
> And the driver compose them into 3 channel links (lp, hp and sec).
> Am I wrong?
> 
> >
> > >>
> > >> >> The SCU is poorly implemented as it ignores 3 irqs and club all
> > >> >> 4 register together (there are many other cons of this approach
> > >> >> but lets not get into that).
> > >> >> Personally, I would push-back on such a bad design. But if you
> > >> >> claim you have no choice but to support SCU as such, the work
> > >> >> around could be simpler than defining a new "scu mode" altogether.
> > >> >>
> > >> >
> > >> > This is one of the recommended ways designed in HW reference
> > manual
> > >> > and it allows to send frame information up to 4 words without
> > >> > using shared
> > >> memory.
> > >> > SCU just follows it, so it's hard to believe it's a bad design.
> > >> >
> > >> >> #mbox-cells:  Must be 3.
> > >> >>                       First cell is 1 for TX and 0 for RX channel
> > >> >>                       Second cell is index of the channel [0,1,2 or 3]
> > >> >>                       Third cell is 1 if the channel triggers an
> > >> >> IRQ,
> > >> >> 0 if not. That is ACR.RIE/TIE bits are set or not.
> > >> >>
> > >> >> Normal clients would always request a channel with irqs enabled.
> > >> >> The SCU client would request all 8 channels -- TX/RX[0,1,2] with
> > >> >> irqs disabled, TX/RX[3] with irqs enabled. And SCU will
> > >> >> read/write
> > >> >> 4 word data over 4 rx/tx channels, instead of the virtually
> > >> >> concocted
> > one.
> > >> >>
> > >> >
> > >> > It may work If SCU protocol data length is fixed to 4 words.
> > >> > However, it's length is flexible for different SVC service. That
> > >> > means this binding
> > >> won't work for SCU.
> > >> > And it will introduce much complexities during the implementation.
> > >> >
> > >> > Instead, we're using polling mode for both TX/RX and the data
> > >> > size is stored in the msg header and sending msgs using all 4
> > >> > data tx/rx registers
> > >> as a channel fifo.
> > >> >
> > >> You first give me the "Passing short messages" usecase (quite a bad
> > >> one) and ask how it can work. When I give you a solution, you
> > >> effectively say "well, my usecase isn't even that". I feel I just
> > >> wasted my
> > time.
> > >>
> > >
> > > Sorry for may not clear, "Passing short message' usecase is to tell
> > > how the HW is working on one channel mode sending up to 4 words in
> > > one time As specified in reference manual.
> > >
> > > SCU does work that way, the only difference is it's using polling
> > > mode rather than interrupt driven.  The point is the data size may
> > > be different for each msg, so we can't simply know which data
> > > register interrupt should be enable from static data defined in device tree.
> > >
> > And you think passing variable data through registers is a better idea
> > than passing packets via shared-memory?
> >
> 
> You got me. :-)
> I've no idea about which one is better. The problem is SCU firmware is
> already there passing packets through data registers, we have no way to
> change it.
> 
> >
> > >>
> > >> >>
> > >> >> > short messages
> > >> >> > Transmit register(s) can be used to pass short messages from
> > >> >> > one to four words in length. For example, when a four-word
> > >> >> > message is desired, only one of the registers needs to have
> > >> >> > its corresponding interrupt enable bit set at the receiver
> > >> >> > side; the message’s first three words are written to the
> > >> >> > registers whose interrupt is masked, and the fourth word is
> > >> >> > written to the other register (which triggers an
> > >> >> interrupt at the receiver side).
> > >> >> >
> > >> >> > The reference manual is at here: (Chapter 42 Messaging Unit
> > >> >> > (MU)
> > >> >> >
> > >> >>
> > >>
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fw
> > >> >> ww
> > >> >> > .nxp.com%2Fdocs%2Fen%2Freference-
> > >> >> manual%2FIMX6ULRM.pdf&amp;data=02%7C0
> > >> >> >
> > >> >>
> > >>
> >
> 1%7Caisheng.dong%40nxp.com%7Cef349d7c9caf46c4260008d5f30e9ef5%7C6
> > >> >> 86ea1
> > >> >> >
> > >> >>
> > >>
> >
> d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636682166494860332&amp;sdat
> > >> >> a=54rE
> > >> >> >
> > >> >>
> > >>
> >
> iDm%2BGD6EY8NE64ck1LXVGoUCtWYrHXPoWqcujUo%3D&amp;reserved=0
> > >> >> >
> > >> >> > And SCU firmware does use MU in above way specified by
> > >> >> > reference
> > >> >> manual.
> > >> >> > Even from HW point of view, it's still one channel only mailbox.
> > >> >> >
> > >> >> Please realise that any manual is written by a mere mortal afterall.
> > >> >> How the controller works is set in stone, but how the controller
> > >> >> can be used ... is just someones suggestion.
> > >> >>
> > >> >> The approach I suggest above, conforms to the api and prevents a
> > >> >> provider dancing to the tunes of a user.
> > >> >
> > >> > First of all, really appreciate for your suggestions.
> > >> > It may be hard to find a common binding with the same mbox-cells.
> > >> > It looks like we just need a property is distinguish how the
> > >> > Mailbox is used In one channel or multi channel mode.
> > >> >
> > >> I get the idea you were ready to see the code merged in the coming
> > >> window and be done with it. And now it feels lazy to change the code.
> > >
> > > For me, I'm glad to change if we have a clear better solution.
> > > And I do appreciate your suggestion and review time.
> > >
> > > Why I'm a bit hesitate now is because your suggestion may not work
> > > for SCU, (see above explanation), but it does work for generic M4 case.
> > > But we' re now going to support both protocol in one mailbox driver.
> > > Any suggestion on how to treat them properly if change the binding?
> > >
> > In your last post you said "This looks like reasonable to me, theoretically"
> > My suggestion is the same because I don't see why it won't work.
> >
> 
> Sorry for not clear. I mean it's reasonable for M4 generic using case.
> But not for SCU case.
> 
> >
> > >> I am sorry, but I can't allow controller drivers "emulating" some
> > >> mode for a client driver. That is moving a part of client code into
> > >> the
> > controller driver.
> > >>
> > >
> > > Okay, let's figure out it first.
> > > Would you be more specific on what "emulating" did you mean in
> > controller driver?
> > > Sending up to 4 words capability in one channel mode as specified in
> > reference manual?
> > > That's I'm a bit confusing because I thought it's HW capability
> > > independent
> > of client driver.
> > >
> > > Or anything else?
> > >
> > Emulation means pretending something that we are not.
> >
> > The hardware has 8 unidirectional channels. But your protocol (SCU
> > implementation) assumes there is one _virtual_ channel that has 4
> > registers and 1/0 irq --- which is not true. Instead of fixing the
> > assumption in your protocol or emulating the virtual channel in the
> > protocol level (user of a MU), you want to add code in the controller
> > driver that ignores interrupts and club the 4 independent channels
> together.
> >
> 
> This stucks me. This is how the hardware is designed  and suggested to use in
> hardware reference manual. And now you're telling me this is wrong and we
> should not use the design in reference manual...
> 
> > There is no end to protocols and their kinky assumptions, adding "xyz-
> mode"
> > support for each protocol isn't scalable.
> >
> 
> This is also how Sascha suggested me to move to mailbox to support both
> protocols and handle them differently in mailbox driver.
> 
> Honestly I was hesitated to do that before because I doubt the value we can
> gain if switching to mailbox besides the unnecessary complexity introduced
> and performance downgrading (extra execution of a few unnecessary code
> in mailbox API and It's even worse if use 8 separate channels for SCU,
> comparing to original only tens of lines of library API way), but Sascha
> insisted...
> 
> The mailbox itself is somehow protocol specific (doorbell, signal, data packet
> and etc) It's hard for us to find a common way to support two totally
> different protocols.
> So the controller needs to know different protocol it is transferring.
> 
> >
> > >>
> > >> > Directly checking mbox-cells seems the most easy way and it is
> > >> > already Acked by Rob. Can't this way be Okay to you?
> > >> >
> > >> Rob is indeed far better than I am. But he also has to look into
> > >> 100 other subsystems, whereas I am only looking into mailbox. I
> > >> have time to dig into your datasheets. I believe Rob will point out
> > >> anything wrong I
> > suggest.
> > >>
> > >
> > > Yes, you're in the fair enough authority to judge it. Thanks for your effort.
> > >
> > >> BTW, the submitted driver doesn't even support the SCU usecase. Why
> > >> the bindings?
> > >
> > > Because that patch is firstly Acked by Rob. Others are reworked and
> > > ready to be sent out against this patch series. But it seems we
> > > still have unresolved issues now as you pointed out. We can first resolve
> them.
> > Or do you need me to send out for your reference?
> > >
> > I am sure Rob took the best decision at the time with whatever
> > information provided to him.
> > Now, after reading the datasheet, we have the option to avoid
> > implementing consumer code in the provider driver.
> 
> I have one doubt, irrelevant of SCU protocol, from the datasheet, the
> hardware does support transfer message up to 4 words in one channel mode
> and it is the recommended way in datasheet for less than 4 words frame
> transferring, why switching to mailbox framework, we can't use it now?
> And makes us lose the HW capability.
> 

See TI mailbox does in a quite similar way as SCU.
The difference is TI mailbox use TX/Rx buffer while i.MX MU uses 4 TX/RX regsiters.
And TI mailbox support send up to 8 words in TX/RX buffer in one time while
i.MX MU supports Send up to 4 words in one time.

truct ti_msgmgr_message {
        size_t len;
        u8 *buf;
};
drivers/mailbox/ti-msgmgr.c: ti_msgmgr_send_data()

Regards
Dong Aisheng

> If that, what's meaning for us to switch to mailbox framework?
> 
> Regards
> Dong Aisheng
> 
> >
> > Thanks.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jassi Brar]

On Fri, Jul 27, 2018 at 2:12 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:


>> >> >>
>> >> >> MU has 4 pairs of TX_Reg + TX_IRQ, and 4 pairs of RX_Reg + RX_IRQ.
>> >> >> (MU also has 4 "doorbell" type channels that it calls GP, but
>> >> >> those are not managed here, so lets not worry atm).
>> >> >>
>> >> >> By definition, a mailbox channel is simply a signal, optionally
>> >> >> with data attached. So, MU has 4 TX and 4 RX channels.
>> >> >>
>> >> >> The MU driver should populate 8 unidirectional (4 Tx and 4 RX)
>> >> >> channels and set each tx/rx operation to trigger the corresponding
>> >> >> interrupt. This is not my whim, this is how the controller is!
>> >> >>
>> >> >
>> >> > This looks like reasonable to me, theoretically.
>> >> > Just not sure whether it's necessary to support it because we
>> >> > probably will never use like that in reality, then it might become
>> >> > meaningless complicity introduced and error prone.
>> >> >
>> >> It _is_ necessary to write controller driver independent of client drivers.
>> >>
>> >
>> > Yes, that's true. What if we think we're writing driver against HW
>> > capabilities which support 4 pair of channel links(tx/rx)?
>> > It looks like independent of client drivers and may make life easily.
>> > Does it make sense?
>> >
>> No, that would be emulation.
>> Why doesn't a usb device controller (udc) driver emulate FSG/HID etc, by
>> "thinking" it has a hardware backed storage/keyboard? It doesn't because
>> that is the job of upper protocol layer.
>>
>
> Sorry I'm not quite familiar with USB device.
>
Which subsystem are you familiar with? Every stack is designed like
that -- controller drivers reflect the physical hardware, any
emulation/protocol is implemented in the user layer.


> Another reason is I doubt that we may never use per register mode in a different register pair
> in the future.
>
You may never use, but if the driver reflects the controller as
precisely as it is, linux could support any usecase that some
baremetal code could support on your platform.
For example, you could support "normal" and "scu" mode over the 3-cell
mechanism I suggested. But your original proposal/assumption of
"scu-mode", failed immediately, hence Oleksij had to imlpement the
"normal" mode.


> For example:
> AP:
> node {
>         ...
>         // cell 0 meaning: 0: tx 1: rx  cell1 meaning: channel id
>         Mboxes = <&mbox 0 1 &mbox 1 2>
>         Mbox-names = "tx", "rx";
>>
>
> M4:
> node {
>         ...
>         Mboxes = <&mbox 0 2 &mbox 1 1>
>         Mbox-names = "tx", "rx";
>>
> This make things complicated and error prone as I said before.
>
I don't see how.


> But that's just my understanding and may overlook something, if you still think
> we should do exactly as above, I will not against it because it does work for M4 case.
>
Cool, done.


> Then the left question is how we handle SCU case?
>
Please point me to the code that you worry might not work.


>
> I'm a bit confusing....
> The section 3.6 you pointed is the MHU register description. It does not conflict
> with what I see from ARM doc center that each physical channel is unidirectional.
>
> See below:
> Chan 1:
> 0x000 SCP_INTR_L_STAT
> 0x008 SCP_INTR_L_SET
> 0x010 SCP _INTR_L_CLEAR
>
> Chan 2:
> 0x020 SCP_INTR_H_STAT
> 0x028 SCP_INTR_H_SET
> 0x030 SCP _INTR_H_CLEAR
>
> Chan 3:
> 0x100 CPU_INTR_L_STAT
> 0x108 CPU_INTR_L_SET
> 0x110 CPU_INTR_L_CLEAR
>
> Chan 4:
> 0x120 CPU_INTR_H_STAT
> 0x128 CPU_INTR_H_SET
> 0x130 CPU_INTR_H_CLEAR
>
> Chan 5:
> 0x200 SCP_INTR_S_STAT
> 0x208 SCP_INTR_S_SET
> 0x210 SCP _INTR_S_CLEAR
>
> Chan 6:
> 0x300 CPU_INTR_S_STAT
> 0x308 CPU_INTR_S_SET
> 0x310 CPU_INTR_S_CLEAR
>
> And the driver compose them into 3 channel links (lp, hp and sec).
> Am I wrong?
>
The RX and TX are not independent of each other, their priorities are
tied together in hardware. So it makes more sense (as compared to
declaring them independent) to club them together as one channel.
Whatever example you take - MHU or TI - you won't find a driver that
implements a virtual mode like "scu-mode".


>> >
>> > Sorry for may not clear, "Passing short message' usecase is to tell
>> > how the HW is working on one channel mode sending up to 4 words in one
>> > time As specified in reference manual.
>> >
>> > SCU does work that way, the only difference is it's using polling mode
>> > rather than interrupt driven.  The point is the data size may be
>> > different for each msg, so we can't simply know which data register
>> > interrupt should be enable from static data defined in device tree.
>> >
>> And you think passing variable data through registers is a better idea than
>> passing packets via shared-memory?
>>
>
> You got me. :-)
> I've no idea about which one is better.
>
Passing data directly via controller makes sense only when your
protocol defines fixed length packets that fit into registers (usually
FIFOs) or there is no shared memory between the processors.
Otherwise, you write a data packet at some located in shmem, and
provide its start and size along with the code of expected operation
on it, over the mailbox. Please refer to "Passing frame information"
para of page-2743 of your manual.
That is another reason the SCU implementation is broken - it has
variable length packets and yet use registers to pass them around.


> The problem is SCU firmware is already
> there passing packets through data registers, we have no way to change it.
>
OK. But what is SCU exactly? Part of ATF? Do you get some binary from
third party? If the last, you shouldn't be paying hsit for such a
design.


>>
>> The hardware has 8 unidirectional channels. But your protocol (SCU
>> implementation) assumes there is one _virtual_ channel that has 4 registers
>> and 1/0 irq --- which is not true. Instead of fixing the assumption in your
>> protocol or emulating the virtual channel in the protocol level (user of a MU),
>> you want to add code in the controller driver that ignores interrupts and club
>> the 4 independent channels together.
>>
>
> This stucks me. This is how the hardware is designed  and suggested to use
> in hardware reference manual. And now you're telling me this is wrong and
> we should not use the design in reference manual...
>
What you call "suggested to use in hardware reference manual" is just
1/5 examples of usecase.
And SCU is not even doing that correctly (it uses variable length
packet, unlike the fixed 4-words suggestion).

A manual can suggest multiple ways of implementing a usecase. It is
our job to chose the best one.

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

[Oleksij Rempel]

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

Hi Aishen, Jassie,

i'm lost in this discussion. Please, as soon as I need to add some
changes to my patches, notify me. Preferably on top for email.

On 28.07.2018 15:09, Jassi Brar wrote:
> On Fri, Jul 27, 2018 at 2:12 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
> 
> 
>>>>>>>
>>>>>>> MU has 4 pairs of TX_Reg + TX_IRQ, and 4 pairs of RX_Reg + RX_IRQ.
>>>>>>> (MU also has 4 "doorbell" type channels that it calls GP, but
>>>>>>> those are not managed here, so lets not worry atm).
>>>>>>>
>>>>>>> By definition, a mailbox channel is simply a signal, optionally
>>>>>>> with data attached. So, MU has 4 TX and 4 RX channels.
>>>>>>>
>>>>>>> The MU driver should populate 8 unidirectional (4 Tx and 4 RX)
>>>>>>> channels and set each tx/rx operation to trigger the corresponding
>>>>>>> interrupt. This is not my whim, this is how the controller is!
>>>>>>>
>>>>>>
>>>>>> This looks like reasonable to me, theoretically.
>>>>>> Just not sure whether it's necessary to support it because we
>>>>>> probably will never use like that in reality, then it might become
>>>>>> meaningless complicity introduced and error prone.
>>>>>>
>>>>> It _is_ necessary to write controller driver independent of client drivers.
>>>>>
>>>>
>>>> Yes, that's true. What if we think we're writing driver against HW
>>>> capabilities which support 4 pair of channel links(tx/rx)?
>>>> It looks like independent of client drivers and may make life easily.
>>>> Does it make sense?
>>>>
>>> No, that would be emulation.
>>> Why doesn't a usb device controller (udc) driver emulate FSG/HID etc, by
>>> "thinking" it has a hardware backed storage/keyboard? It doesn't because
>>> that is the job of upper protocol layer.
>>>
>>
>> Sorry I'm not quite familiar with USB device.
>>
> Which subsystem are you familiar with? Every stack is designed like
> that -- controller drivers reflect the physical hardware, any
> emulation/protocol is implemented in the user layer.
> 
> 
>> Another reason is I doubt that we may never use per register mode in a different register pair
>> in the future.
>>
> You may never use, but if the driver reflects the controller as
> precisely as it is, linux could support any usecase that some
> baremetal code could support on your platform.
> For example, you could support "normal" and "scu" mode over the 3-cell
> mechanism I suggested. But your original proposal/assumption of
> "scu-mode", failed immediately, hence Oleksij had to imlpement the
> "normal" mode.
> 
> 
>> For example:
>> AP:
>> node {
>>         ...
>>         // cell 0 meaning: 0: tx 1: rx  cell1 meaning: channel id
>>         Mboxes = <&mbox 0 1 &mbox 1 2>
>>         Mbox-names = "tx", "rx";
>>>
>>
>> M4:
>> node {
>>         ...
>>         Mboxes = <&mbox 0 2 &mbox 1 1>
>>         Mbox-names = "tx", "rx";
>>>
>> This make things complicated and error prone as I said before.
>>
> I don't see how.
> 
> 
>> But that's just my understanding and may overlook something, if you still think
>> we should do exactly as above, I will not against it because it does work for M4 case.
>>
> Cool, done.
> 
> 
>> Then the left question is how we handle SCU case?
>>
> Please point me to the code that you worry might not work.
> 
> 
>>
>> I'm a bit confusing....
>> The section 3.6 you pointed is the MHU register description. It does not conflict
>> with what I see from ARM doc center that each physical channel is unidirectional.
>>
>> See below:
>> Chan 1:
>> 0x000 SCP_INTR_L_STAT
>> 0x008 SCP_INTR_L_SET
>> 0x010 SCP _INTR_L_CLEAR
>>
>> Chan 2:
>> 0x020 SCP_INTR_H_STAT
>> 0x028 SCP_INTR_H_SET
>> 0x030 SCP _INTR_H_CLEAR
>>
>> Chan 3:
>> 0x100 CPU_INTR_L_STAT
>> 0x108 CPU_INTR_L_SET
>> 0x110 CPU_INTR_L_CLEAR
>>
>> Chan 4:
>> 0x120 CPU_INTR_H_STAT
>> 0x128 CPU_INTR_H_SET
>> 0x130 CPU_INTR_H_CLEAR
>>
>> Chan 5:
>> 0x200 SCP_INTR_S_STAT
>> 0x208 SCP_INTR_S_SET
>> 0x210 SCP _INTR_S_CLEAR
>>
>> Chan 6:
>> 0x300 CPU_INTR_S_STAT
>> 0x308 CPU_INTR_S_SET
>> 0x310 CPU_INTR_S_CLEAR
>>
>> And the driver compose them into 3 channel links (lp, hp and sec).
>> Am I wrong?
>>
> The RX and TX are not independent of each other, their priorities are
> tied together in hardware. So it makes more sense (as compared to
> declaring them independent) to club them together as one channel.
> Whatever example you take - MHU or TI - you won't find a driver that
> implements a virtual mode like "scu-mode".
> 
> 
>>>>
>>>> Sorry for may not clear, "Passing short message' usecase is to tell
>>>> how the HW is working on one channel mode sending up to 4 words in one
>>>> time As specified in reference manual.
>>>>
>>>> SCU does work that way, the only difference is it's using polling mode
>>>> rather than interrupt driven.  The point is the data size may be
>>>> different for each msg, so we can't simply know which data register
>>>> interrupt should be enable from static data defined in device tree.
>>>>
>>> And you think passing variable data through registers is a better idea than
>>> passing packets via shared-memory?
>>>
>>
>> You got me. :-)
>> I've no idea about which one is better.
>>
> Passing data directly via controller makes sense only when your
> protocol defines fixed length packets that fit into registers (usually
> FIFOs) or there is no shared memory between the processors.
> Otherwise, you write a data packet at some located in shmem, and
> provide its start and size along with the code of expected operation
> on it, over the mailbox. Please refer to "Passing frame information"
> para of page-2743 of your manual.
> That is another reason the SCU implementation is broken - it has
> variable length packets and yet use registers to pass them around.
> 
> 
>> The problem is SCU firmware is already
>> there passing packets through data registers, we have no way to change it.
>>
> OK. But what is SCU exactly? Part of ATF? Do you get some binary from
> third party? If the last, you shouldn't be paying hsit for such a
> design.
> 
> 
>>>
>>> The hardware has 8 unidirectional channels. But your protocol (SCU
>>> implementation) assumes there is one _virtual_ channel that has 4 registers
>>> and 1/0 irq --- which is not true. Instead of fixing the assumption in your
>>> protocol or emulating the virtual channel in the protocol level (user of a MU),
>>> you want to add code in the controller driver that ignores interrupts and club
>>> the 4 independent channels together.
>>>
>>
>> This stucks me. This is how the hardware is designed  and suggested to use
>> in hardware reference manual. And now you're telling me this is wrong and
>> we should not use the design in reference manual...
>>
> What you call "suggested to use in hardware reference manual" is just
> 1/5 examples of usecase.
> And SCU is not even doing that correctly (it uses variable length
> packet, unlike the fixed 4-words suggestion).
> 
> A manual can suggest multiple ways of implementing a usecase. It is
> our job to chose the best one.
> 


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

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

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

RE: [PATCH v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU channel support

[A.s. Dong]

Hi Oleksij,

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> Sent: Monday, July 30, 2018 3:36 PM
> To: Jassi Brar <jassisinghbrar@gmail.com>; A.s. Dong
> <aisheng.dong@nxp.com>
> Cc: , Sascha Hauer <kernel@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>; ", 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 v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU
> channel support
> 
> Hi Aishen, Jassie,
> 
> i'm lost in this discussion. Please, as soon as I need to add some changes to
> my patches, notify me. Preferably on top for email.
> 

I just send you an email of Jassie's early suggestion.

According to the suggestion, the binding needs to be changed into 8 separate
TX/RX channels. You can quickly start from there.

Regards
Dong Aisheng

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

[Jassi Brar]

On Mon, Jul 30, 2018 at 1:05 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> Hi Aishen, Jassie,
>
> i'm lost in this discussion. Please, as soon as I need to add some
> changes to my patches, notify me. Preferably on top for email.
>
I am ok with however you choose to implement, 8 unidirectional or 4
bidirectional channels or whatever.

We just can't have protocol specific s/w modes in the controller drivers.

The best solution is to fix the SCU firmware. If that is _really_
impossible, I provided a solution (3 cells work around). If you have a
better idea please feel free to propose and implement that.

It will also help if you could share the user code of "scu-mode". If
there is no such code (and we know the driver doesn't respect the
"scu-mode" property) why do we even have that binding? Maybe drop it.

thnx.

RE: [PATCH v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU channel support

[A.s. Dong]

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> Sent: Monday, July 30, 2018 9:05 PM
> To: Oleksij Rempel <o.rempel@pengutronix.de>
> Cc: A.s. Dong <aisheng.dong@nxp.com>; , Sascha Hauer
> <kernel@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>; ", linux-arm-
> kernel"@lists.infradead.org; , linux-arm-kernel@lists.infradead.org, linux-
> mediatek@lists.infradead.org, srv_heupstream <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 v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU
> channel support
> 
> On Mon, Jul 30, 2018 at 1:05 PM, Oleksij Rempel <o.rempel@pengutronix.de>
> wrote:
> > Hi Aishen, Jassie,
> >
> > i'm lost in this discussion. Please, as soon as I need to add some
> > changes to my patches, notify me. Preferably on top for email.
> >
> I am ok with however you choose to implement, 8 unidirectional or 4
> bidirectional channels or whatever.
> 
> We just can't have protocol specific s/w modes in the controller drivers.
> 

Do you think implement "Passing short messages" defined in reference
manual in controller driver is protocol specific?
That seems like our most argument before.

> The best solution is to fix the SCU firmware. If that is _really_ impossible, I
> provided a solution (3 cells work around). If you have a better idea please
> feel free to propose and implement that.
> 

That's out of our control.
SCU firmware is not using Linux and there's no mailbox framework.
It's simply follow "passing short messages" way to send multi words with 4 data
Registers.
 
> It will also help if you could share the user code of "scu-mode". If there is no
> such code (and we know the driver doesn't respect the "scu-mode" property)
> why do we even have that binding? Maybe drop it.

You can refer to this patch for the user code of "scu-mode".
[RFC PATCH v6 2/2] mailbox: imx-mailbox: add scu protocol support
https://www.spinics.net/lists/arm-kernel/msg665135.html

And I do admit that there're two protocol specific bits in the driver.
1. Unlike TI message header, SCU size is in the second u8.
Ti message header:
struct ti_msgmgr_message {
        size_t len;
        u8 *buf;
};
Controller get the size from the first u32.

SCU message header:
struct sc_rpc_msg {
        uint8_t ver;
        uint8_t size;
        uint8_t svc; 
        uint8_t func;
};
Controller get the size from the second u8.

And a full SCU message is like:
struct imx_sc_msg_req_set_clock_rate {
        struct sc_rpc_msg hdr;
        u32 rate;
        u16 resource;
        u8 clk;
} __packed;

2. There's no synchronous read function in mailbox framework,
So we use mbox_client_peek_data to read data which can't return
the read data. In order to save a mem copy from read data to the
data buffer, the controller driver saves the tx data buffer in itself. 
(Anyway to improve? Maybe we should implement a synchronous
mbox_client_read_data()?)

And for SCU IPC client, it's something like:
int sc_call_rpc(sc_ipc_t ipc, void *msg, bool no_resp)
{
        ...
        mbox_send_message(sc_ipc->chan, msg);

        if (!no_resp) 
                ret = mbox_client_peek_data(sc_ipc->chan);

        mbox_client_txdone(sc_ipc->chan, ret);
        ...
}

int sc_ipc_open(sc_ipc_t *ipc, struct device *dev)
{
        sc_ipc->cl.dev = dev; 
        sc_ipc->cl.tx_block = false;
        sc_ipc->cl.knows_txdone = true;

        sc_ipc->chan = mbox_request_channel(&sc_ipc->cl, 0);
        ...
}

Any better ideas?

Regards
Dong Aisheng

> 
> thnx.

RE: [PATCH v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU channel support

[A.s. Dong]

[...]
> > It will also help if you could share the user code of "scu-mode". If
> > there is no such code (and we know the driver doesn't respect the
> > "scu-mode" property) why do we even have that binding? Maybe drop it.
> 
> You can refer to this patch for the user code of "scu-mode".
> [RFC PATCH v6 2/2] mailbox: imx-mailbox: add scu protocol support
> https://www.spinics.net/lists/arm-kernel/msg665135.html
> 
> And I do admit that there're two protocol specific bits in the driver.
> 1. Unlike TI message header, SCU size is in the second u8.
> Ti message header:
> struct ti_msgmgr_message {
>         size_t len;
>         u8 *buf;
> };
> Controller get the size from the first u32.
> 
> SCU message header:
> struct sc_rpc_msg {
>         uint8_t ver;
>         uint8_t size;
>         uint8_t svc;
>         uint8_t func;
> };
> Controller get the size from the second u8.
> 
> And a full SCU message is like:
> struct imx_sc_msg_req_set_clock_rate {
>         struct sc_rpc_msg hdr;
>         u32 rate;
>         u16 resource;
>         u8 clk;
> } __packed;
> 
> 2. There's no synchronous read function in mailbox framework, So we use
> mbox_client_peek_data to read data which can't return the read data. In
> order to save a mem copy from read data to the data buffer, the controller
> driver saves the tx data buffer in itself.
> (Anyway to improve? Maybe we should implement a synchronous
> mbox_client_read_data()?)
> 
> And for SCU IPC client, it's something like:
> int sc_call_rpc(sc_ipc_t ipc, void *msg, bool no_resp) {
>         ...
>         mbox_send_message(sc_ipc->chan, msg);
> 
>         if (!no_resp)
>                 ret = mbox_client_peek_data(sc_ipc->chan);
> 
>         mbox_client_txdone(sc_ipc->chan, ret);
>         ...
> }
> 
> int sc_ipc_open(sc_ipc_t *ipc, struct device *dev) {
>         sc_ipc->cl.dev = dev;
>         sc_ipc->cl.tx_block = false;
>         sc_ipc->cl.knows_txdone = true;
> 
>         sc_ipc->chan = mbox_request_channel(&sc_ipc->cl, 0);
>         ...
> }
> 
> Any better ideas?
> 

And this the original implementation used by NXP internally without mailbox.
As it's quite simple and suppose more easy to help you understand the using of SCU.

void mu_send_msg(struct mu_priv *priv, uint32_t index, uint32_t msg)
{
        ...
        /* Wait TX register to be empty. */ 
        while (!(readl_relaxed(priv->base + MU_ASR) & mask))
                ;
        writel_relaxed(msg, priv->base + MU_ATR0  + (index * 4));
}

/*
 * Wait to receive message from the other core.
 */
void mu_receive_msg(struct mu_priv *priv, uint32_t index, uint32_t *msg)
{
        ...
        /* Wait RX register to be full. */
        while (!(readl_relaxed(priv->base + MU_ASR) & mask))
                ;
        *msg = readl_relaxed(priv->base + MU_ARR0 + (index * 4));
}

static void sc_ipc_read(struct sc_ipc *sc_ipc, void *data)
{
        sc_rpc_msg_t *msg = (sc_rpc_msg_t *)data;
 
        /* Read first word */
        mu_receive_msg(sc_ipc->mu, 0, (uint32_t *)msg);
        count++;
 
        /* Read remaining words */
        while (count < msg->size) {
                mu_receive_msg(sc_ipc->mu, count % MU_RR_COUNT,
                               &(msg->DATA.u32[count - 1]));
                count++;
        }
}

static void sc_ipc_write(struct sc_ipc *sc_ipc, void *data)
{
        sc_rpc_msg_t *msg = (sc_rpc_msg_t *)data;
        uint8_t count = 0;

        /* Write first word */
        mu_send_msg(sc_ipc->mu, 0, *((uint32_t *)msg));
        count++;

        while (count < msg->size) {
                mu_send_msg(sc_ipc->mu, count % MU_TR_COUNT,
                            msg->DATA.u32[count - 1]);
                count++;
        }
}

void sc_call_rpc(sc_ipc_t ipc, sc_rpc_msg_t *msg, bool no_resp)
{
        sc_ipc_write(sc_ipc, msg);
        if (!no_resp)
                sc_ipc_read(sc_ipc, msg);
}

For details, see:
https://www.spinics.net/lists/arm-kernel/msg659138.html

Regards
Dong Aisheng

> Regards
> Dong Aisheng
> 
> >
> > thnx.

RE: [PATCH v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU channel support

[A.s. Dong]

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> Sent: Monday, July 30, 2018 9:05 PM
> To: Oleksij Rempel <o.rempel@pengutronix.de>
> Cc: A.s. Dong <aisheng.dong@nxp.com>; , Sascha Hauer
> <kernel@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>; ", linux-arm-
> kernel"@lists.infradead.org; , linux-arm-kernel@lists.infradead.org, linux-
> mediatek@lists.infradead.org, srv_heupstream <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 v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU
> channel support
> 
> On Mon, Jul 30, 2018 at 1:05 PM, Oleksij Rempel <o.rempel@pengutronix.de>
> wrote:
> > Hi Aishen, Jassie,
> >
> > i'm lost in this discussion. Please, as soon as I need to add some
> > changes to my patches, notify me. Preferably on top for email.
> >
> I am ok with however you choose to implement, 8 unidirectional or 4
> bidirectional channels or whatever.
> 
> We just can't have protocol specific s/w modes in the controller drivers.
> 
> The best solution is to fix the SCU firmware. If that is _really_ impossible, I
> provided a solution (3 cells work around). If you have a better idea please
> feel free to propose and implement that.
> 
> It will also help if you could share the user code of "scu-mode". If there is no
> such code (and we know the driver doesn't respect the "scu-mode" property)
> why do we even have that binding? Maybe drop it.

The initial idea is using mbox-cells value to distinguish which MU mode is used
(single chan or multi chans).

Regards
Dong Aisheng

> 
> thnx.

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

[Oleksij Rempel]

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

On Mon, Jul 30, 2018 at 06:34:56PM +0530, Jassi Brar wrote:
> On Mon, Jul 30, 2018 at 1:05 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> > Hi Aishen, Jassie,
> >
> > i'm lost in this discussion. Please, as soon as I need to add some
> > changes to my patches, notify me. Preferably on top for email.
> >
> I am ok with however you choose to implement, 8 unidirectional or 4
> bidirectional channels or whatever.
> 
> We just can't have protocol specific s/w modes in the controller drivers.
> 
> The best solution is to fix the SCU firmware. If that is _really_
> impossible, I provided a solution (3 cells work around). If you have a
> better idea please feel free to propose and implement that.
> 
> It will also help if you could share the user code of "scu-mode". If
> there is no such code (and we know the driver doesn't respect the
> "scu-mode" property) why do we even have that binding? Maybe drop it.

Tomorrow I have a time slot to address your generic iMX MU suggestions.
So, what is better, uni- or bi-directional channels? Should I implement
*all* (4TX+FIFO, 4RX+FIFO, 4TX-simple, 4RX-simple) channels in this run?
If yes, how should it be reflected in DT?
- 1 cell: &mu [0-15]
- 2 cells: &mu [0-7] [TX,RX]
- 3 cells: &mu [0-4] [FIFO,NOFIFO] [TX,RX]

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

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

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

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

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

[Jassi Brar]

On Mon, Jul 30, 2018 at 8:14 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> On Mon, Jul 30, 2018 at 06:34:56PM +0530, Jassi Brar wrote:
>> On Mon, Jul 30, 2018 at 1:05 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
>> > Hi Aishen, Jassie,
>> >
>> > i'm lost in this discussion. Please, as soon as I need to add some
>> > changes to my patches, notify me. Preferably on top for email.
>> >
>> I am ok with however you choose to implement, 8 unidirectional or 4
>> bidirectional channels or whatever.
>>
>> We just can't have protocol specific s/w modes in the controller drivers.
>>
>> The best solution is to fix the SCU firmware. If that is _really_
>> impossible, I provided a solution (3 cells work around). If you have a
>> better idea please feel free to propose and implement that.
>>
>> It will also help if you could share the user code of "scu-mode". If
>> there is no such code (and we know the driver doesn't respect the
>> "scu-mode" property) why do we even have that binding? Maybe drop it.
>
> Tomorrow I have a time slot to address your generic iMX MU suggestions.
> So, what is better, uni- or bi-directional channels?
>
The datasheet indicates there are 4 tx and 4 rx channels. So 8
uni-directional channels (which allow more fine-grained/efficient
resource allocation btw).

> Should I implement
> *all* (4TX+FIFO, 4RX+FIFO, 4TX-simple, 4RX-simple) channels in this run?
>
>From datasheet, each of 8 channels should be defined as signal+data
i.e, IRQ + TX/RX_Reg.
The rest 4 GP channels are doorbells (irq only).

So we can have 2-cells.
    First cell is 0->Tx, 1->RX, 2->Doorbell
    Second cell is index of the channel {0,3}

Now you may implement only RX+TX, and leave 'doorbell' out for future.
Thats ok, because we wouldn't have to change bindings then.

However, if SCU (in its current form) must be supported. We may need
to add the third cell (irq enable or not) or some better way, right
now.

> If yes, how should it be reflected in DT?
> - 1 cell: &mu [0-15]
> - 2 cells: &mu [0-7] [TX,RX]
> - 3 cells: &mu [0-4] [FIFO,NOFIFO] [TX,RX]
>
It may be possible for a device binding to have variable cells, but I
haven't seen any. And I am not sure that would look neat either.

RE: [PATCH v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU channel support

[A.s. Dong]

Hi Jassi,

> > If yes, how should it be reflected in DT?
> > - 1 cell: &mu [0-15]
> > - 2 cells: &mu [0-7] [TX,RX]
> > - 3 cells: &mu [0-4] [FIFO,NOFIFO] [TX,RX]
> >
> It may be possible for a device binding to have variable cells, but I haven't
> seen any. And I am not sure that would look neat either.

Please refer to 
Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
- #interrupt-cells : Specifies the number of cells needed to encode an
  interrupt source. Must be a single cell with a value of at least 3.
  If the system requires describing PPI affinity, then the value must
  be at least 4.

Regards
Dong Aisheng

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

[Jassi Brar]

On Mon, Jul 30, 2018 at 8:32 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> On Mon, Jul 30, 2018 at 8:14 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
>> On Mon, Jul 30, 2018 at 06:34:56PM +0530, Jassi Brar wrote:
>>> On Mon, Jul 30, 2018 at 1:05 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
>>> > Hi Aishen, Jassie,
>>> >
>>> > i'm lost in this discussion. Please, as soon as I need to add some
>>> > changes to my patches, notify me. Preferably on top for email.
>>> >
>>> I am ok with however you choose to implement, 8 unidirectional or 4
>>> bidirectional channels or whatever.
>>>
>>> We just can't have protocol specific s/w modes in the controller drivers.
>>>
>>> The best solution is to fix the SCU firmware. If that is _really_
>>> impossible, I provided a solution (3 cells work around). If you have a
>>> better idea please feel free to propose and implement that.
>>>
>>> It will also help if you could share the user code of "scu-mode". If
>>> there is no such code (and we know the driver doesn't respect the
>>> "scu-mode" property) why do we even have that binding? Maybe drop it.
>>
>> Tomorrow I have a time slot to address your generic iMX MU suggestions.
>> So, what is better, uni- or bi-directional channels?
>>
> The datasheet indicates there are 4 tx and 4 rx channels. So 8
> uni-directional channels (which allow more fine-grained/efficient
> resource allocation btw).
>
>> Should I implement
>> *all* (4TX+FIFO, 4RX+FIFO, 4TX-simple, 4RX-simple) channels in this run?
>>
> From datasheet, each of 8 channels should be defined as signal+data
> i.e, IRQ + TX/RX_Reg.
> The rest 4 GP channels are doorbells (irq only).
>
> So we can have 2-cells.
>     First cell is 0->Tx, 1->RX, 2->Doorbell
>     Second cell is index of the channel {0,3}
>
> Now you may implement only RX+TX, and leave 'doorbell' out for future.
> Thats ok, because we wouldn't have to change bindings then.
>
> However, if SCU (in its current form) must be supported. We may need
> to add the third cell (irq enable or not) or some better way, right
> now.
>
Looking at imx_mu_scu_send_data(), which simply polls on the tx, I
think we don't even need third cell for scu client. A simple 2-cell, 8
uni-dir channel setup should work.
If I see the scu client driver, I could confirm how it would work.

So, lets please do the 8 uni-directional, 2 cells implementation.

Regards.

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

[Oleksij Rempel]

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

On Mon, Jul 30, 2018 at 09:48:35PM +0530, Jassi Brar wrote:
> On Mon, Jul 30, 2018 at 8:32 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> > On Mon, Jul 30, 2018 at 8:14 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> >> On Mon, Jul 30, 2018 at 06:34:56PM +0530, Jassi Brar wrote:
> >>> On Mon, Jul 30, 2018 at 1:05 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> >>> > Hi Aishen, Jassie,
> >>> >
> >>> > i'm lost in this discussion. Please, as soon as I need to add some
> >>> > changes to my patches, notify me. Preferably on top for email.
> >>> >
> >>> I am ok with however you choose to implement, 8 unidirectional or 4
> >>> bidirectional channels or whatever.
> >>>
> >>> We just can't have protocol specific s/w modes in the controller drivers.
> >>>
> >>> The best solution is to fix the SCU firmware. If that is _really_
> >>> impossible, I provided a solution (3 cells work around). If you have a
> >>> better idea please feel free to propose and implement that.
> >>>
> >>> It will also help if you could share the user code of "scu-mode". If
> >>> there is no such code (and we know the driver doesn't respect the
> >>> "scu-mode" property) why do we even have that binding? Maybe drop it.
> >>
> >> Tomorrow I have a time slot to address your generic iMX MU suggestions.
> >> So, what is better, uni- or bi-directional channels?
> >>
> > The datasheet indicates there are 4 tx and 4 rx channels. So 8
> > uni-directional channels (which allow more fine-grained/efficient
> > resource allocation btw).
> >
> >> Should I implement
> >> *all* (4TX+FIFO, 4RX+FIFO, 4TX-simple, 4RX-simple) channels in this run?
> >>
> > From datasheet, each of 8 channels should be defined as signal+data
> > i.e, IRQ + TX/RX_Reg.
> > The rest 4 GP channels are doorbells (irq only).
> >
> > So we can have 2-cells.
> >     First cell is 0->Tx, 1->RX, 2->Doorbell
> >     Second cell is index of the channel {0,3}
> >
> > Now you may implement only RX+TX, and leave 'doorbell' out for future.
> > Thats ok, because we wouldn't have to change bindings then.
> >
> > However, if SCU (in its current form) must be supported. We may need
> > to add the third cell (irq enable or not) or some better way, right
> > now.
> >
> Looking at imx_mu_scu_send_data(), which simply polls on the tx, I
> think we don't even need third cell for scu client. A simple 2-cell, 8
> uni-dir channel setup should work.
> If I see the scu client driver, I could confirm how it would work.
> 
> So, lets please do the 8 uni-directional, 2 cells implementation.

Just in case.. to avoid confusion (or add more...). There are 4 GIP (General Interrupt Request n
Pending bits) or RX-doorbell and 4 GIR (General Interrupt Request bits)
or TX-doorbell.

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

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

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

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

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

[Jassi Brar]

On Mon, Jul 30, 2018 at 10:19 PM, Oleksij Rempel
<o.rempel@pengutronix.de> wrote:
> On Mon, Jul 30, 2018 at 09:48:35PM +0530, Jassi Brar wrote:
>> On Mon, Jul 30, 2018 at 8:32 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
>> > On Mon, Jul 30, 2018 at 8:14 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
>> >> On Mon, Jul 30, 2018 at 06:34:56PM +0530, Jassi Brar wrote:
>> >>> On Mon, Jul 30, 2018 at 1:05 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
>> >>> > Hi Aishen, Jassie,
>> >>> >
>> >>> > i'm lost in this discussion. Please, as soon as I need to add some
>> >>> > changes to my patches, notify me. Preferably on top for email.
>> >>> >
>> >>> I am ok with however you choose to implement, 8 unidirectional or 4
>> >>> bidirectional channels or whatever.
>> >>>
>> >>> We just can't have protocol specific s/w modes in the controller drivers.
>> >>>
>> >>> The best solution is to fix the SCU firmware. If that is _really_
>> >>> impossible, I provided a solution (3 cells work around). If you have a
>> >>> better idea please feel free to propose and implement that.
>> >>>
>> >>> It will also help if you could share the user code of "scu-mode". If
>> >>> there is no such code (and we know the driver doesn't respect the
>> >>> "scu-mode" property) why do we even have that binding? Maybe drop it.
>> >>
>> >> Tomorrow I have a time slot to address your generic iMX MU suggestions.
>> >> So, what is better, uni- or bi-directional channels?
>> >>
>> > The datasheet indicates there are 4 tx and 4 rx channels. So 8
>> > uni-directional channels (which allow more fine-grained/efficient
>> > resource allocation btw).
>> >
>> >> Should I implement
>> >> *all* (4TX+FIFO, 4RX+FIFO, 4TX-simple, 4RX-simple) channels in this run?
>> >>
>> > From datasheet, each of 8 channels should be defined as signal+data
>> > i.e, IRQ + TX/RX_Reg.
>> > The rest 4 GP channels are doorbells (irq only).
>> >
>> > So we can have 2-cells.
>> >     First cell is 0->Tx, 1->RX, 2->Doorbell
>> >     Second cell is index of the channel {0,3}
>> >
>> > Now you may implement only RX+TX, and leave 'doorbell' out for future.
>> > Thats ok, because we wouldn't have to change bindings then.
>> >
>> > However, if SCU (in its current form) must be supported. We may need
>> > to add the third cell (irq enable or not) or some better way, right
>> > now.
>> >
>> Looking at imx_mu_scu_send_data(), which simply polls on the tx, I
>> think we don't even need third cell for scu client. A simple 2-cell, 8
>> uni-dir channel setup should work.
>> If I see the scu client driver, I could confirm how it would work.
>>
>> So, lets please do the 8 uni-directional, 2 cells implementation.
>
> Just in case.. to avoid confusion (or add more...). There are 4 GIP (General Interrupt Request n
> Pending bits) or RX-doorbell and 4 GIR (General Interrupt Request bits)
> or TX-doorbell.
>
Ok. But the approach remains the same.

thanks.

RE: [PATCH v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU channel support

[A.s. Dong]

Hi Jassi,

> > However, if SCU (in its current form) must be supported. We may need
> > to add the third cell (irq enable or not) or some better way, right
> > now.
> >
> Looking at imx_mu_scu_send_data(), which simply polls on the tx, I think we
> don't even need third cell for scu client. A simple 2-cell, 8 uni-dir channel
> setup should work.

How would you suggest to use for SCU in 2 -cells?

> If I see the scu client driver, I could confirm how it would work.
> 

I've already given a general overview from here:
https://www.spinics.net/lists/arm-kernel/msg669202.html

Do you need me to send the full scu client driver patch for the reference?
If yes, please feel free to let me know.

Regards
Dong Aisheng

> So, lets please do the 8 uni-directional, 2 cells implementation.
> 
> Regards.

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

[Jassi Brar]

On Tue, Jul 31, 2018 at 12:51 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
> Hi Jassi,
>
>> > However, if SCU (in its current form) must be supported. We may need
>> > to add the third cell (irq enable or not) or some better way, right
>> > now.
>> >
>> Looking at imx_mu_scu_send_data(), which simply polls on the tx, I think we
>> don't even need third cell for scu client. A simple 2-cell, 8 uni-dir channel
>> setup should work.
>
> How would you suggest to use for SCU in 2 -cells?
>
>> If I see the scu client driver, I could confirm how it would work.
>>
>
> I've already given a general overview from here:
> https://www.spinics.net/lists/arm-kernel/msg669202.html
>
No, that is controller side code.

> Do you need me to send the full scu client driver patch for the reference?
> If yes, please feel free to let me know.
>
Yes, that'll make it clearer.

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

[Jassi Brar]

On Tue, Jul 31, 2018 at 3:45 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> On Tue, Jul 31, 2018 at 12:51 PM, A.s. Dong <aisheng.dong@nxp.com> wrote:
>> Hi Jassi,
>>
>>> > However, if SCU (in its current form) must be supported. We may need
>>> > to add the third cell (irq enable or not) or some better way, right
>>> > now.
>>> >
>>> Looking at imx_mu_scu_send_data(), which simply polls on the tx, I think we
>>> don't even need third cell for scu client. A simple 2-cell, 8 uni-dir channel
>>> setup should work.
>>
>> How would you suggest to use for SCU in 2 -cells?
>>
>>> If I see the scu client driver, I could confirm how it would work.
>>>
>>
>> I've already given a general overview from here:
>> https://www.spinics.net/lists/arm-kernel/msg669202.html
>>
> No, that is controller side code.
>
>> Do you need me to send the full scu client driver patch for the reference?
>> If yes, please feel free to let me know.
>>
> Yes, that'll make it clearer.
>
Thanks for sharing the patch offline.

I don't see why it can't be adapted to work with the simple 2-cell
implementation. Instead of one virtual, ask for 4 physical channels
and send data in u32 chunks sequentially over them.
In fact, that should make it even better by removing the tight-loop
(zero delay!!) polling in send_data() -- MU does support interrupts,
why not use them to avoid polling?  If you find it hard, I can make a
patch on top of your client patchset, once Oleksij is done with the MU
driver.

I have some more suggestions for the client driver. But since I don't
have to live with them (you do), I can't demand you make those
changes. If you like, please feel free to cc me on the next revision
of the client driver.

RE: [PATCH v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU channel support

[A.s. Dong]

Hi Jassi,

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> Sent: Tuesday, July 31, 2018 8:42 PM
> To: A.s. Dong <aisheng.dong@nxp.com>

[...]

> >> Do you need me to send the full scu client driver patch for the reference?
> >> If yes, please feel free to let me know.
> >>
> > Yes, that'll make it clearer.
> >
> Thanks for sharing the patch offline.
> 
> I don't see why it can't be adapted to work with the simple 2-cell
> implementation. Instead of one virtual, ask for 4 physical channels and send
> data in u32 chunks sequentially over them.
> In fact, that should make it even better by removing the tight-loop (zero
> delay!!) polling in send_data() -- MU does support interrupts, why not use
> them to avoid polling?  If you find it hard, I can make a patch on top of your
> client patchset, once Oleksij is done with the MU driver.
> 

As you insist, I will try to implement multi channels mode, then we can do a
clear comparison to see which one is better for SCU mode rather than assumptions.

First of all,  using interrupt for each chan will likely to terribly slow down the SCU
Message handling speed. SCU Messages usually are handled by SCU firmware very
quickly in a few microseconds. (< 10us). That's why we use polling mode for SCU
message handling.

If we break down a SCU message into a few separate MU channel interrupts driven,
There will be a lot of unnecessary and meaningless interrupt handling.
And the whole flow may look like:

Sending Word 0 ->
Chan 0 interrupt -> (meaningless)
Sending Word 1 ->
Chan1 interrupt -> (meaningless)
Sending Word 2 ->
Chan2 interrupt -> (meaningless)
Sending Word 3 ->
Chan3 interrupt-> (meaningless)

Sending Word 4 on Chan 0 again ->
Wait for Chan 0 interrupt (meaningless) ->
...

Then waiting for Rx:

Chan 0 Rx interrupt ->
Read Word 0   ->
(Then we know response size here)

Chan 1 Rx interrupt ->
Read Word 1 ->
Chan 2 Rx interrupt ->
Read Word 2 ->
Chan 3 Rx interrupt ->
Read Word 3 ->

(If msg size > 4 words)
Chan 0 Rx interrupt ->
Read Word 4

...
And we must use a complicated way to map the received data of each channel
into one SCU msg buffer and need handle the sequence properly for size > 4
words case

Is this what you intended?

Per my understanding, there're several issues for this follow:
1) Many unnecessary and meaningless interrupts hanllding of Tx and RX
2) Performance downgrading
    A 4 word SCU MSG may need 8 interrupts to handle which terribly slow
    down the speed. (Interrupt latency will be terrible if many)
3) Complex RX flow in order to align with framework design

Any comments about these issues?

Generally, from what I see, the mailbox framework really is not designed for SCU type
devices that handling msg sending or receiving via multiple channels.  Including the
ring buffer supported for handling several msgs on the same channel and rich
completion mechanism, none of them are suitable for SCU MU in multiple channel mode.

What we really actually needed is a quite simple way to handle MSG Tx and Tx
synchronously.

int sc_call_rpc(sc_ipc_t ipc, void *msg, bool no_resp)
{       
        ...
        ret = mbox_send_message(sc_ipc->chan, msg);
        if (!no_resp) 
                ret = mbox_receive_message(sc_ipc->chan, msg);
        ...
        return ret;
}

For me, I don't think the above way driven by interrupt is suitable for SCU.
But I will still keep investigating if any better way to handle it in multi chan
mode as you wish.

How would you suggest for such situation in order to support SCU properly?

> I have some more suggestions for the client driver. But since I don't have to
> live with them (you do), I can't demand you make those changes. If you like,
> please feel free to cc me on the next revision of the client driver.

I certainly appreciate any clear and valuable improvement suggestions.
Just feel free to let me know if any better idea.

Regards
Dong Aisheng

RE: [PATCH v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU channel support

[A.s. Dong]

Hi Jassi,

Do you have any comments about this reply?

Sascha,
What's your option?

Regards
Dong Aisheng

> -----Original Message-----
> From: A.s. Dong
> Sent: Thursday, August 2, 2018 5:24 PM
> To: 'Jassi Brar' <jassisinghbrar@gmail.com>
> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; , Sascha Hauer
> <kernel@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>; , linux-arm-kernel@lists.infradead.org,
> linux-mediatek@lists.infradead.org, srv_heupstream <linux-arm-
> kernel@lists.infradead.org>; , linux-arm-kernel@lists.infradead.org, linux-
> mediatek@lists.infradead.org, srv_heupstream <linux-
> mediatek@lists.infradead.org>; Devicetree List
> <devicetree@vger.kernel.org>; dl-linux-imx <linux-imx@nxp.com>
> Subject: RE: [PATCH v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU
> channel support
> 
> Hi Jassi,
> 
> > -----Original Message-----
> > From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> > Sent: Tuesday, July 31, 2018 8:42 PM
> > To: A.s. Dong <aisheng.dong@nxp.com>
> 
> [...]
> 
> > >> Do you need me to send the full scu client driver patch for the reference?
> > >> If yes, please feel free to let me know.
> > >>
> > > Yes, that'll make it clearer.
> > >
> > Thanks for sharing the patch offline.
> >
> > I don't see why it can't be adapted to work with the simple 2-cell
> > implementation. Instead of one virtual, ask for 4 physical channels
> > and send data in u32 chunks sequentially over them.
> > In fact, that should make it even better by removing the tight-loop
> > (zero
> > delay!!) polling in send_data() -- MU does support interrupts, why not
> > use them to avoid polling?  If you find it hard, I can make a patch on
> > top of your client patchset, once Oleksij is done with the MU driver.
> >
> 
> As you insist, I will try to implement multi channels mode, then we can do a
> clear comparison to see which one is better for SCU mode rather than
> assumptions.
> 
> First of all,  using interrupt for each chan will likely to terribly slow down the
> SCU Message handling speed. SCU Messages usually are handled by SCU
> firmware very quickly in a few microseconds. (< 10us). That's why we use
> polling mode for SCU message handling.
> 
> If we break down a SCU message into a few separate MU channel interrupts
> driven, There will be a lot of unnecessary and meaningless interrupt handling.
> And the whole flow may look like:
> 
> Sending Word 0 ->
> Chan 0 interrupt -> (meaningless)
> Sending Word 1 ->
> Chan1 interrupt -> (meaningless)
> Sending Word 2 ->
> Chan2 interrupt -> (meaningless)
> Sending Word 3 ->
> Chan3 interrupt-> (meaningless)
> 
> Sending Word 4 on Chan 0 again ->
> Wait for Chan 0 interrupt (meaningless) -> ...
> 
> Then waiting for Rx:
> 
> Chan 0 Rx interrupt ->
> Read Word 0   ->
> (Then we know response size here)
> 
> Chan 1 Rx interrupt ->
> Read Word 1 ->
> Chan 2 Rx interrupt ->
> Read Word 2 ->
> Chan 3 Rx interrupt ->
> Read Word 3 ->
> 
> (If msg size > 4 words)
> Chan 0 Rx interrupt ->
> Read Word 4
> 
> ...
> And we must use a complicated way to map the received data of each
> channel into one SCU msg buffer and need handle the sequence properly for
> size > 4 words case
> 
> Is this what you intended?
> 
> Per my understanding, there're several issues for this follow:
> 1) Many unnecessary and meaningless interrupts hanllding of Tx and RX
> 2) Performance downgrading
>     A 4 word SCU MSG may need 8 interrupts to handle which terribly slow
>     down the speed. (Interrupt latency will be terrible if many)
> 3) Complex RX flow in order to align with framework design
> 
> Any comments about these issues?
> 
> Generally, from what I see, the mailbox framework really is not designed for
> SCU type devices that handling msg sending or receiving via multiple
> channels.  Including the ring buffer supported for handling several msgs on
> the same channel and rich completion mechanism, none of them are suitable
> for SCU MU in multiple channel mode.
> 
> What we really actually needed is a quite simple way to handle MSG Tx and
> Tx synchronously.
> 
> int sc_call_rpc(sc_ipc_t ipc, void *msg, bool no_resp)
> {
>         ...
>         ret = mbox_send_message(sc_ipc->chan, msg);
>         if (!no_resp)
>                 ret = mbox_receive_message(sc_ipc->chan, msg);
>         ...
>         return ret;
> }
> 
> For me, I don't think the above way driven by interrupt is suitable for SCU.
> But I will still keep investigating if any better way to handle it in multi chan
> mode as you wish.
> 
> How would you suggest for such situation in order to support SCU properly?
> 
> > I have some more suggestions for the client driver. But since I don't
> > have to live with them (you do), I can't demand you make those
> > changes. If you like, please feel free to cc me on the next revision of the
> client driver.
> 
> I certainly appreciate any clear and valuable improvement suggestions.
> Just feel free to let me know if any better idea.
> 
> Regards
> Dong Aisheng

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

[Jassi Brar]

On Thu, Aug 9, 2018 at 7:52 AM, A.s. Dong <aisheng.dong@nxp.com> wrote:
> Hi Jassi,
>
> Do you have any comments about this reply?
>
I did read your post. And my opinion isn't changed. If mailbox is new
to you, please try to see how other subsystems works, especially SPI,
I2C and DMA.

Thanks.

RE: [PATCH v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU channel support

[A.s. Dong]

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> Sent: Thursday, August 9, 2018 10:56 AM
[...]
> On Thu, Aug 9, 2018 at 7:52 AM, A.s. Dong <aisheng.dong@nxp.com> wrote:
> > Hi Jassi,
> >
> > Do you have any comments about this reply?
> >
> I did read your post. And my opinion isn't changed. If mailbox is new to you,
> please try to see how other subsystems works, especially SPI, I2C and DMA.
> 

Okay.
What's your comments about the known issues if doing that way?
1) Many unnecessary and meaningless interrupts handling of Tx and RX
2) Performance downgrading
    A 4 word SCU MSG may need 8 interrupts to handle which terribly slow
    down the speed. (Interrupt latency will be terrible if many). SCU msg
    are mostly handled within 10 us.
3) Complex RX flow in order to align with framework design

Are you referring to them as not a real issue?
Or even they're issues/limitations, you would still prefer multi channels way
In order to be consistent with mailbox subsystem by sacrificing the
performance drop?

And to be clear, the working flow you want is exactly as follows, right?
Sending Word 0 ->
Chan 0 interrupt -> (meaningless)
Sending Word 1 ->
Chan1 interrupt -> (meaningless)
Sending Word 2 ->
Chan2 interrupt -> (meaningless)
Sending Word 3 ->
Chan3 interrupt-> (meaningless)

Sending Word 4 on Chan 0 again ->
Wait for Chan 0 interrupt (meaningless) -> ...

Then waiting for Rx:

Chan 0 Rx interrupt ->
Read Word 0   ->
(Then we know response size here)

Chan 1 Rx interrupt ->
Read Word 1 ->
Chan 2 Rx interrupt ->
Read Word 2 ->
Chan 3 Rx interrupt ->
Read Word 3 ->

(If msg size > 4 words)
Chan 0 Rx interrupt ->
Read Word 4

Regards
Dong Aisheng

> Thanks.

RE: [PATCH v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU channel support

[A.s. Dong]

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> Sent: Saturday, July 28, 2018 9:10 PM
> To: A.s. Dong <aisheng.dong@nxp.com>
> Cc: Oleksij Rempel <o.rempel@pengutronix.de>; , Sascha Hauer
> <kernel@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>; , 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 v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU
> channel support
> 

[...]

> >> >
> >> > Sorry for may not clear, "Passing short message' usecase is to tell
> >> > how the HW is working on one channel mode sending up to 4 words in
> >> > one time As specified in reference manual.
> >> >
> >> > SCU does work that way, the only difference is it's using polling
> >> > mode rather than interrupt driven.  The point is the data size may
> >> > be different for each msg, so we can't simply know which data
> >> > register interrupt should be enable from static data defined in device
> tree.
> >> >
> >> And you think passing variable data through registers is a better
> >> idea than passing packets via shared-memory?
> >>
> >
> > You got me. :-)
> > I've no idea about which one is better.
> >
> Passing data directly via controller makes sense only when your protocol
> defines fixed length packets that fit into registers (usually
> FIFOs) or there is no shared memory between the processors.

Don't have to be fixed length packets. Every data packets less than 4 words
can be transferred in one time by controller perfectly well as it has 4 data registers.
No shared memory need.

> Otherwise, you write a data packet at some located in shmem, and provide
> its start and size along with the code of expected operation on it, over the
> mailbox. Please refer to "Passing frame information"
> para of page-2743 of your manual.
> That is another reason the SCU implementation is broken - it has variable
> length packets and yet use registers to pass them around.
> 

" Passing frame information" is used for long message transfer via shared memory.
But almost all the SCU data frames are less than 4 words. (only quite a few of them,
2~3, over 4 words) For these types of messages, it's obviously more efficient to transfer
them in "Passing short message' way as the MU hardware supports well.

So I believe our SCU firmware has already done the best choice in a most efficient
way (register write is obviously faster than memory copies). Thus, no shared memory
need to be used for SCU protocol case.

> 
> > The problem is SCU firmware is already there passing packets through
> > data registers, we have no way to change it.
> >
> OK. But what is SCU exactly? Part of ATF? Do you get some binary from third
> party? If the last, you shouldn't be paying hsit for such a design.
> 

The System Controller Firmware (SCFW) is a low-level system function
which runs on a dedicated Cortex-M core to provide power, clock, and
resource management. It exists on some i.MX8 processors. e.g. i.MX8QM
(QM, QP), and i.MX8QX (QXP, DX). 

It's developed by NXP internally.

> 
> >>
> >> The hardware has 8 unidirectional channels. But your protocol (SCU
> >> implementation) assumes there is one _virtual_ channel that has 4
> >> registers and 1/0 irq --- which is not true. Instead of fixing the
> >> assumption in your protocol or emulating the virtual channel in the
> >> protocol level (user of a MU), you want to add code in the controller
> >> driver that ignores interrupts and club the 4 independent channels
> together.
> >>
> >
> > This stucks me. This is how the hardware is designed  and suggested to
> > use in hardware reference manual. And now you're telling me this is
> > wrong and we should not use the design in reference manual...
> >
> What you call "suggested to use in hardware reference manual" is just
> 1/5 examples of usecase.
> And SCU is not even doing that correctly (it uses variable length packet,
> unlike the fixed 4-words suggestion).

SCU is doing well. Please see my above reply.

> 
> A manual can suggest multiple ways of implementing a usecase. It is our job
> to chose the best one.

Well, great. That is also our orientation.
Because we thought the way " Passing short message " is the most suitable way for
SCU and is the most efficient way as the most SCU Messages are less than 4 words
and MU hardware supports transfer them well. We could not think out a reason to
not use it. That's why we choose that.

A simple objection that it uses variable length packet seems not quite convincible.
Unless we have more convincible reasons...

How do you think?

For example, what's the real benefits we can get if we drop the HW capability of
"passing Short messages" for SCU in one channel way and use separate 8 Tx/Rx channel
Instead to send them each time? What about the Performance downgrading? 
Is it valuable comparing to the complexity introduced?

Regards
Dong Aisheng

RE: [PATCH v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU channel support

[A.s. Dong]

Hi Oleksij & Sascha,

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar@gmail.com]
> Sent: Thursday, July 26, 2018 11:44 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 v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU
> channel support
> 
> On Thu, Jul 26, 2018 at 5:21 PM, A.s. Dong <aisheng.dong@nxp.com> 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.
> >> >
> >> So the hardware actually supports 4 channels.
> >>
> >> > -- #mbox-cells:  Must be 0. Number of cells in a mailbox
> >> > +- #mbox-cells:  Must be:
> >> > +               0 - for single channel mode. i.MX8* SCU protocol specific.
> >> > +               1 - for multichannel (generic) mode.
> >> > +
> >> No, please.
> >> DT bindings should reflect the real hardware, and not the software
> >> mode we want the driver to work in.
> >> Please define mbox-cells=1  and have the i.MX8* platform always ask
> >> for channel-0.
> >
> > The reality is that MU hardware does not define channels in reference
> manual.
> > However, it does have four separate data tx/rx register which can be
> > used as 'virtual' channels which is supported by this current driver.
> >
> > See below HW description from the reference manual:
> > For messaging, the MU has four, 32-bit write-only transmit registers
> > and four, 32-bit read-only receive registers on the Processor B and
> > Processor A-sides. These registers are used for sending messages to each
> other.
> >
> For a while please forget the protocol(user) level usage, and consider only
> what your h/w is.
> 
> MU has 4 pairs of TX_Reg + TX_IRQ, and 4 pairs of RX_Reg + RX_IRQ.
> (MU also has 4 "doorbell" type channels that it calls GP, but those are not
> managed here, so lets not worry atm).
> 
> By definition, a mailbox channel is simply a signal, optionally with data
> attached. So, MU has 4 TX and 4 RX channels.
> 
> The MU driver should populate 8 unidirectional (4 Tx and 4 RX) channels and
> set each tx/rx operation to trigger the corresponding interrupt. This is not my
> whim, this is how the controller is!
> 
> The SCU is poorly implemented as it ignores 3 irqs and club all 4 register
> together (there are many other cons of this approach but lets not get into
> that).
> Personally, I would push-back on such a bad design. But if you claim you have
> no choice but to support SCU as such, the work around could be simpler than
> defining a new "scu mode" altogether.
> 
> #mbox-cells:  Must be 3.
>                       First cell is 1 for TX and 0 for RX channel
>                       Second cell is index of the channel [0,1,2 or 3]
>                       Third cell is 1 if the channel triggers an IRQ,
> 0 if not. That is ACR.RIE/TIE bits are set or not.
> 
> Normal clients would always request a channel with irqs enabled.
> The SCU client would request all 8 channels -- TX/RX[0,1,2] with irqs disabled,
> TX/RX[3] with irqs enabled. And SCU will read/write 4 word data over 4 rx/tx
> channels, instead of the virtually concocted one.
> 

Sascha & Oleksij,
Do you have any comments on this new approach suggested by Jassi?
This looks like may work for M4 case.

But I'm still discussing with Jassi on whether it's suitable for SCU as well?
As that means we drop the MU Hardware capability to send multi words
in one time but send one word on each channel one time which seems like
less efficiency and more complexity introduced for SCU case.

Regards
Dong Aisheng

> 
> > short messages
> > Transmit register(s) can be used to pass short messages from one to
> > four words in length. For example, when a four-word message is
> > desired, only one of the registers needs to have its corresponding
> > interrupt enable bit set at the receiver side; the message’s first
> > three words are written to the registers whose interrupt is masked,
> > and the fourth word is written to the other register (which triggers an
> interrupt at the receiver side).
> >
> > The reference manual is at here: (Chapter 42 Messaging Unit (MU)
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fw
> ww
> > .nxp.com%2Fdocs%2Fen%2Freference-
> manual%2FIMX6ULRM.pdf&amp;data=02%7C0
> >
> 1%7Caisheng.dong%40nxp.com%7Cef349d7c9caf46c4260008d5f30e9ef5%7C6
> 86ea1
> >
> d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636682166494860332&amp;sdat
> a=54rE
> >
> iDm%2BGD6EY8NE64ck1LXVGoUCtWYrHXPoWqcujUo%3D&amp;reserved=0
> >
> > And SCU firmware does use MU in above way specified by reference
> manual.
> > Even from HW point of view, it's still one channel only mailbox.
> >
> Please realise that any manual is written by a mere mortal afterall.
> How the controller works is set in stone, but how the controller can be
> used ... is just someones suggestion.
> 
> The approach I suggest above, conforms to the api and prevents a provider
> dancing to the tunes of a user.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v7 4/6] dt-bindings: mailbox: imx-mu: add i.MX6SX and i.MX7S SoCs.

[Oleksij Rempel]

This are currently tested SoCs with imx-mailbox driver.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 Documentation/devicetree/bindings/mailbox/fsl,mu.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
index 113d6ab931ef..5616d2afca45 100644
--- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
+++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
@@ -18,7 +18,7 @@ Messaging Unit Device Node:
 Required properties:
 -------------------
 - compatible :	should be "fsl,<chip>-mu", the supported chips include
-		imx8qxp, imx8qm.
+		imx6sx, imx7s, imx8qxp, imx8qm.
 - reg :		Should contain the registers location and length
 - interrupts :	Interrupt number. The interrupt specifier format depends
 		on the interrupt controller parent.
-- 
2.18.0

RE: [PATCH v7 4/6] dt-bindings: mailbox: imx-mu: add i.MX6SX and i.MX7S SoCs.

[A.s. Dong]

> -----Original Message-----
> From: Oleksij Rempel [mailto:o.rempel@pengutronix.de]
> Sent: Thursday, July 26, 2018 2:53 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 v7 4/6] dt-bindings: mailbox: imx-mu: add i.MX6SX and
> i.MX7S SoCs.
> 
> This are currently tested SoCs with imx-mailbox driver.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

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

Regards
Dong Aisheng

Re: [PATCH v7 4/6] dt-bindings: mailbox: imx-mu: add i.MX6SX and i.MX7S SoCs.

[Jassi Brar]

On Thu, Jul 26, 2018 at 12:23 PM, Oleksij Rempel
<o.rempel@pengutronix.de> wrote:
> This are currently tested SoCs with imx-mailbox driver.
>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/mailbox/fsl,mu.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> index 113d6ab931ef..5616d2afca45 100644
> --- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> @@ -18,7 +18,7 @@ Messaging Unit Device Node:
>  Required properties:
>  -------------------
>  - compatible : should be "fsl,<chip>-mu", the supported chips include
> -               imx8qxp, imx8qm.
> +               imx6sx, imx7s, imx8qxp, imx8qm.
>
This is not scalable. Do we add every new SoC that contains the same controller?
I think the controller name, if it has one or create a new one like
'imx-mu',  should be used here.
So,
    compatible : should be "fsl,imx-mu"

BTW, the driver doesn't even probe anything other than
'fsl,imx6sx-mu', unlike what the binding says.

Re: [PATCH v7 4/6] dt-bindings: mailbox: imx-mu: add i.MX6SX and i.MX7S SoCs.

[Lucas Stach]

Hi Jassi,

Am Donnerstag, den 26.07.2018, 15:25 +0530 schrieb Jassi Brar:
> On Thu, Jul 26, 2018 at 12:23 PM, Oleksij Rempel
> > <o.rempel@pengutronix.de> wrote:
> > This are currently tested SoCs with imx-mailbox driver.
> > 
> > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  Documentation/devicetree/bindings/mailbox/fsl,mu.txt | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > index 113d6ab931ef..5616d2afca45 100644
> > --- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > @@ -18,7 +18,7 @@ Messaging Unit Device Node:
> >  Required properties:
> >  -------------------
> >  - compatible : should be "fsl,<chip>-mu", the supported chips include
> > -               imx8qxp, imx8qm.
> > +               imx6sx, imx7s, imx8qxp, imx8qm.
> > 
> 
> This is not scalable. Do we add every new SoC that contains the same controller?

Yes, we do. This is a policy direction from the DT maintainers. If we
ever going to want to validate DTs against the binding, all compatibles
used in the DTs must be specified in the binding.

As we can't really tell if the controller is exactly the same or even
has some SoC integration bugs, we generally add a new compatible for
each SoC to key off any workarounds necessary in the driver without the
need to change the DTs, breaking compatibility.

Regards,
Lucas

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

Re: [PATCH v7 4/6] dt-bindings: mailbox: imx-mu: add i.MX6SX and i.MX7S SoCs.

[Jassi Brar]

On Thu, Jul 26, 2018 at 4:11 PM, Lucas Stach <l.stach@pengutronix.de> wrote:
> Hi Jassi,
>
> Am Donnerstag, den 26.07.2018, 15:25 +0530 schrieb Jassi Brar:
>> On Thu, Jul 26, 2018 at 12:23 PM, Oleksij Rempel
>> > <o.rempel@pengutronix.de> wrote:
>> > This are currently tested SoCs with imx-mailbox driver.
>> >
>> > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>> > ---
>> >  Documentation/devicetree/bindings/mailbox/fsl,mu.txt | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>> > index 113d6ab931ef..5616d2afca45 100644
>> > --- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>> > +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>> > @@ -18,7 +18,7 @@ Messaging Unit Device Node:
>> >  Required properties:
>> >  -------------------
>> >  - compatible : should be "fsl,<chip>-mu", the supported chips include
>> > -               imx8qxp, imx8qm.
>> > +               imx6sx, imx7s, imx8qxp, imx8qm.
>> >
>>
>> This is not scalable. Do we add every new SoC that contains the same controller?
>
> Yes, we do. This is a policy direction from the DT maintainers.
>
I would love to read the post/documentation.

Consider the same h/w - controller and platforms, but only the the MU
chapter said the controller name is, say, 'MU121'. I am sure now you
will see it correct to call it "fsl,mu121" compatible.
What changed? just the name, right?


> If we
> ever going to want to validate DTs against the binding, all compatibles
> used in the DTs must be specified in the binding.
>
> As we can't really tell if the controller is exactly the same or even
> has some SoC integration bugs, we generally add a new compatible for
> each SoC to key off any workarounds necessary in the driver without the
> need to change the DTs, breaking compatibility.
>
I think if the h/w resources and behaviour remain the same and the
documentation does not call it by a different name -- it is safe to
assume its the same IP. Especially when the driver is absolutely
indifferent to the 5 SoC names.

If/when we find the controller changes, we could revisit the binding
and add another compatible option and modify the driver to catch that
and adapt.

Re: [PATCH v7 4/6] dt-bindings: mailbox: imx-mu: add i.MX6SX and i.MX7S SoCs.

[Lucas Stach]

Am Donnerstag, den 26.07.2018, 16:45 +0530 schrieb Jassi Brar:
> On Thu, Jul 26, 2018 at 4:11 PM, Lucas Stach <l.stach@pengutronix.de>
> wrote:
> > Hi Jassi,
> > 
> > Am Donnerstag, den 26.07.2018, 15:25 +0530 schrieb Jassi Brar:
> > > On Thu, Jul 26, 2018 at 12:23 PM, Oleksij Rempel
> > > > <o.rempel@pengutronix.de> wrote:
> > > > This are currently tested SoCs with imx-mailbox driver.
> > > > 
> > > > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > > 
> > > > ---
> > > >  Documentation/devicetree/bindings/mailbox/fsl,mu.txt | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > > > b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > > > index 113d6ab931ef..5616d2afca45 100644
> > > > --- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > > > +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > > > @@ -18,7 +18,7 @@ Messaging Unit Device Node:
> > > >  Required properties:
> > > >  -------------------
> > > >  - compatible : should be "fsl,<chip>-mu", the supported chips
> > > > include
> > > > -               imx8qxp, imx8qm.
> > > > +               imx6sx, imx7s, imx8qxp, imx8qm.
> > > > 
> > > 
> > > This is not scalable. Do we add every new SoC that contains the
> > > same controller?
> > 
> > Yes, we do. This is a policy direction from the DT maintainers.
> > 
> 
> I would love to read the post/documentation.
> 
> Consider the same h/w - controller and platforms, but only the the MU
> chapter said the controller name is, say, 'MU121'. I am sure now you
> will see it correct to call it "fsl,mu121" compatible.
> What changed? just the name, right?
> 
> 
> > If we
> > ever going to want to validate DTs against the binding, all
> > compatibles
> > used in the DTs must be specified in the binding.
> > 
> > As we can't really tell if the controller is exactly the same or
> > even
> > has some SoC integration bugs, we generally add a new compatible
> > for
> > each SoC to key off any workarounds necessary in the driver without
> > the
> > need to change the DTs, breaking compatibility.
> > 
> 
> I think if the h/w resources and behaviour remain the same and the
> documentation does not call it by a different name -- it is safe to
> assume its the same IP. Especially when the driver is absolutely
> indifferent to the 5 SoC names.

Even if it is the same IP core, the SoC integration might have bugs
that need different behavior from the driver. We've already had that
case with the i.MX6 SPI controller.

> If/when we find the controller changes, we could revisit the binding
> and add another compatible option and modify the driver to catch that
> and adapt.

That's way too late. If we want to keep DTs stable the DT must include
forward looking compatibles, to allow the driver to key any required
behavior changes from already present compatibles. Otherwise you
require the DT to be updated in step-lock with the kernel or need much
more awkward detection in the drivers. IP integration bugs might be
found way after the initial DT has been written.

Regards,
Lucas

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

Re: [PATCH v7 4/6] dt-bindings: mailbox: imx-mu: add i.MX6SX and i.MX7S SoCs.

[Jassi Brar]

On Thu, Jul 26, 2018 at 5:05 PM, Lucas Stach <l.stach@pengutronix.de> wrote:
> Am Donnerstag, den 26.07.2018, 16:45 +0530 schrieb Jassi Brar:
>> On Thu, Jul 26, 2018 at 4:11 PM, Lucas Stach <l.stach@pengutronix.de>
>> wrote:
>> > Hi Jassi,
>> >
>> > Am Donnerstag, den 26.07.2018, 15:25 +0530 schrieb Jassi Brar:
>> > > On Thu, Jul 26, 2018 at 12:23 PM, Oleksij Rempel
>> > > > <o.rempel@pengutronix.de> wrote:
>> > > > This are currently tested SoCs with imx-mailbox driver.
>> > > >
>> > > > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>> > > >
>> > > > ---
>> > > >  Documentation/devicetree/bindings/mailbox/fsl,mu.txt | 2 +-
>> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > > >
>> > > > diff --git
>> > > > a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>> > > > b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>> > > > index 113d6ab931ef..5616d2afca45 100644
>> > > > --- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>> > > > +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>> > > > @@ -18,7 +18,7 @@ Messaging Unit Device Node:
>> > > >  Required properties:
>> > > >  -------------------
>> > > >  - compatible : should be "fsl,<chip>-mu", the supported chips
>> > > > include
>> > > > -               imx8qxp, imx8qm.
>> > > > +               imx6sx, imx7s, imx8qxp, imx8qm.
>> > > >
>> > >
>> > > This is not scalable. Do we add every new SoC that contains the
>> > > same controller?
>> >
>> > Yes, we do. This is a policy direction from the DT maintainers.
>> >
>>
>> I would love to read the post/documentation.
>>
>> Consider the same h/w - controller and platforms, but only the the MU
>> chapter said the controller name is, say, 'MU121'. I am sure now you
>> will see it correct to call it "fsl,mu121" compatible.
>> What changed? just the name, right?
>>
>>
>> > If we
>> > ever going to want to validate DTs against the binding, all
>> > compatibles
>> > used in the DTs must be specified in the binding.
>> >
>> > As we can't really tell if the controller is exactly the same or
>> > even
>> > has some SoC integration bugs, we generally add a new compatible
>> > for
>> > each SoC to key off any workarounds necessary in the driver without
>> > the
>> > need to change the DTs, breaking compatibility.
>> >
>>
>> I think if the h/w resources and behaviour remain the same and the
>> documentation does not call it by a different name -- it is safe to
>> assume its the same IP. Especially when the driver is absolutely
>> indifferent to the 5 SoC names.
>
> Even if it is the same IP core, the SoC integration might have bugs
> that need different behavior from the driver. We've already had that
> case with the i.MX6 SPI controller.
>
For h/w quirks/bugs, a new "has-that-bug" property makes better sense.
Or, if you insist, a new compatible based on the first soc that has
the buggy block.


>> If/when we find the controller changes, we could revisit the binding
>> and add another compatible option and modify the driver to catch that
>> and adapt.
>
> That's way too late. If we want to keep DTs stable
>
How do you keep the DT stable by explicitly defining every new SoC to
the compatible list in DT, _add_ to the driver.... only to have the
driver absolutely not care which SoC is it?
Which is the situation right now with this patchset.

thnx.

Re: [PATCH v7 4/6] dt-bindings: mailbox: imx-mu: add i.MX6SX and i.MX7S SoCs.

[Vladimir Zapolskiy]

On 07/26/2018 02:46 PM, Jassi Brar wrote:
> On Thu, Jul 26, 2018 at 5:05 PM, Lucas Stach <l.stach@pengutronix.de> wrote:
>> Am Donnerstag, den 26.07.2018, 16:45 +0530 schrieb Jassi Brar:
>>> On Thu, Jul 26, 2018 at 4:11 PM, Lucas Stach <l.stach@pengutronix.de>
>>> wrote:
>>>> Hi Jassi,
>>>>
>>>> Am Donnerstag, den 26.07.2018, 15:25 +0530 schrieb Jassi Brar:
>>>>> On Thu, Jul 26, 2018 at 12:23 PM, Oleksij Rempel
>>>>>> <o.rempel@pengutronix.de> wrote:
>>>>>> This are currently tested SoCs with imx-mailbox driver.
>>>>>>
>>>>>>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>>>>>>
>>>>>> ---
>>>>>>  Documentation/devicetree/bindings/mailbox/fsl,mu.txt | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git
>>>>>> a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>>>>>> b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>>>>>> index 113d6ab931ef..5616d2afca45 100644
>>>>>> --- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>>>>>> +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>>>>>> @@ -18,7 +18,7 @@ Messaging Unit Device Node:
>>>>>>  Required properties:
>>>>>>  -------------------
>>>>>>  - compatible : should be "fsl,<chip>-mu", the supported chips
>>>>>> include
>>>>>> -               imx8qxp, imx8qm.
>>>>>> +               imx6sx, imx7s, imx8qxp, imx8qm.
>>>>>>
>>>>>
>>>>> This is not scalable. Do we add every new SoC that contains the
>>>>> same controller?
>>>>
>>>> Yes, we do. This is a policy direction from the DT maintainers.
>>>>
>>>
>>> I would love to read the post/documentation.
>>>
>>> Consider the same h/w - controller and platforms, but only the the MU
>>> chapter said the controller name is, say, 'MU121'. I am sure now you
>>> will see it correct to call it "fsl,mu121" compatible.
>>> What changed? just the name, right?
>>>
>>>
>>>> If we
>>>> ever going to want to validate DTs against the binding, all
>>>> compatibles
>>>> used in the DTs must be specified in the binding.
>>>>
>>>> As we can't really tell if the controller is exactly the same or
>>>> even
>>>> has some SoC integration bugs, we generally add a new compatible
>>>> for
>>>> each SoC to key off any workarounds necessary in the driver without
>>>> the
>>>> need to change the DTs, breaking compatibility.
>>>>
>>>
>>> I think if the h/w resources and behaviour remain the same and the
>>> documentation does not call it by a different name -- it is safe to
>>> assume its the same IP. Especially when the driver is absolutely
>>> indifferent to the 5 SoC names.
>>
>> Even if it is the same IP core, the SoC integration might have bugs
>> that need different behavior from the driver. We've already had that
>> case with the i.MX6 SPI controller.
>>
> For h/w quirks/bugs, a new "has-that-bug" property makes better sense.
> Or, if you insist, a new compatible based on the first soc that has
> the buggy block.
> 

How do you propose to handle this property in the driver, if once flashed
DTB does not contain it?

>>> If/when we find the controller changes, we could revisit the binding
>>> and add another compatible option and modify the driver to catch that
>>> and adapt.
>>
>> That's way too late. If we want to keep DTs stable
>>
> How do you keep the DT stable by explicitly defining every new SoC to
> the compatible list in DT, _add_ to the driver.... only to have the
> driver absolutely not care which SoC is it?
> Which is the situation right now with this patchset.
> 

For sake of simplicity you can assume that a driver can be changed in
future, and SoC/board device tree bindings can not be changed ever.

--
Best wishes,
Vladimir

Re: [PATCH v7 4/6] dt-bindings: mailbox: imx-mu: add i.MX6SX and i.MX7S SoCs.

[Jassi Brar]

On Thu, Jul 26, 2018 at 5:21 PM, Vladimir Zapolskiy
<vladimir_zapolskiy@mentor.com> wrote:
> On 07/26/2018 02:46 PM, Jassi Brar wrote:
>> On Thu, Jul 26, 2018 at 5:05 PM, Lucas Stach <l.stach@pengutronix.de> wrote:
>>> Am Donnerstag, den 26.07.2018, 16:45 +0530 schrieb Jassi Brar:
>>>> On Thu, Jul 26, 2018 at 4:11 PM, Lucas Stach <l.stach@pengutronix.de>
>>>> wrote:
>>>>> Hi Jassi,
>>>>>
>>>>> Am Donnerstag, den 26.07.2018, 15:25 +0530 schrieb Jassi Brar:
>>>>>> On Thu, Jul 26, 2018 at 12:23 PM, Oleksij Rempel
>>>>>>> <o.rempel@pengutronix.de> wrote:
>>>>>>> This are currently tested SoCs with imx-mailbox driver.
>>>>>>>
>>>>>>>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>>>>>>>
>>>>>>> ---
>>>>>>>  Documentation/devicetree/bindings/mailbox/fsl,mu.txt | 2 +-
>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git
>>>>>>> a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>>>>>>> b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>>>>>>> index 113d6ab931ef..5616d2afca45 100644
>>>>>>> --- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>>>>>>> @@ -18,7 +18,7 @@ Messaging Unit Device Node:
>>>>>>>  Required properties:
>>>>>>>  -------------------
>>>>>>>  - compatible : should be "fsl,<chip>-mu", the supported chips
>>>>>>> include
>>>>>>> -               imx8qxp, imx8qm.
>>>>>>> +               imx6sx, imx7s, imx8qxp, imx8qm.
>>>>>>>
>>>>>>
>>>>>> This is not scalable. Do we add every new SoC that contains the
>>>>>> same controller?
>>>>>
>>>>> Yes, we do. This is a policy direction from the DT maintainers.
>>>>>
>>>>
>>>> I would love to read the post/documentation.
>>>>
>>>> Consider the same h/w - controller and platforms, but only the the MU
>>>> chapter said the controller name is, say, 'MU121'. I am sure now you
>>>> will see it correct to call it "fsl,mu121" compatible.
>>>> What changed? just the name, right?
>>>>
>>>>
>>>>> If we
>>>>> ever going to want to validate DTs against the binding, all
>>>>> compatibles
>>>>> used in the DTs must be specified in the binding.
>>>>>
>>>>> As we can't really tell if the controller is exactly the same or
>>>>> even
>>>>> has some SoC integration bugs, we generally add a new compatible
>>>>> for
>>>>> each SoC to key off any workarounds necessary in the driver without
>>>>> the
>>>>> need to change the DTs, breaking compatibility.
>>>>>
>>>>
>>>> I think if the h/w resources and behaviour remain the same and the
>>>> documentation does not call it by a different name -- it is safe to
>>>> assume its the same IP. Especially when the driver is absolutely
>>>> indifferent to the 5 SoC names.
>>>
>>> Even if it is the same IP core, the SoC integration might have bugs
>>> that need different behavior from the driver. We've already had that
>>> case with the i.MX6 SPI controller.
>>>
>> For h/w quirks/bugs, a new "has-that-bug" property makes better sense.
>> Or, if you insist, a new compatible based on the first soc that has
>> the buggy block.
>>
>
> How do you propose to handle this property in the driver, if once flashed
> DTB does not contain it?
>
I see, your point is - what if a bug appears only in a particular SoC ?
Well, if the controller is same the bug should appear to all SoCs.
Do you think these SoCs have different versions of MU ?

Re: [PATCH v7 4/6] dt-bindings: mailbox: imx-mu: add i.MX6SX and i.MX7S SoCs.

[Vladimir Zapolskiy]

On 07/26/2018 03:00 PM, Jassi Brar wrote:
> On Thu, Jul 26, 2018 at 5:21 PM, Vladimir Zapolskiy
> <vladimir_zapolskiy@mentor.com> wrote:
>> On 07/26/2018 02:46 PM, Jassi Brar wrote:
>>> On Thu, Jul 26, 2018 at 5:05 PM, Lucas Stach <l.stach@pengutronix.de> wrote:
>>>> Am Donnerstag, den 26.07.2018, 16:45 +0530 schrieb Jassi Brar:
>>>>> On Thu, Jul 26, 2018 at 4:11 PM, Lucas Stach <l.stach@pengutronix.de>
>>>>> wrote:
>>>>>> Hi Jassi,
>>>>>>
>>>>>> Am Donnerstag, den 26.07.2018, 15:25 +0530 schrieb Jassi Brar:
>>>>>>> On Thu, Jul 26, 2018 at 12:23 PM, Oleksij Rempel
>>>>>>>> <o.rempel@pengutronix.de> wrote:
>>>>>>>> This are currently tested SoCs with imx-mailbox driver.
>>>>>>>>
>>>>>>>>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>>>>>>>>
>>>>>>>> ---
>>>>>>>>  Documentation/devicetree/bindings/mailbox/fsl,mu.txt | 2 +-
>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git
>>>>>>>> a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>>>>>>>> b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>>>>>>>> index 113d6ab931ef..5616d2afca45 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>>>>>>>> +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>>>>>>>> @@ -18,7 +18,7 @@ Messaging Unit Device Node:
>>>>>>>>  Required properties:
>>>>>>>>  -------------------
>>>>>>>>  - compatible : should be "fsl,<chip>-mu", the supported chips
>>>>>>>> include
>>>>>>>> -               imx8qxp, imx8qm.
>>>>>>>> +               imx6sx, imx7s, imx8qxp, imx8qm.
>>>>>>>>
>>>>>>>
>>>>>>> This is not scalable. Do we add every new SoC that contains the
>>>>>>> same controller?
>>>>>>
>>>>>> Yes, we do. This is a policy direction from the DT maintainers.
>>>>>>
>>>>>
>>>>> I would love to read the post/documentation.
>>>>>
>>>>> Consider the same h/w - controller and platforms, but only the the MU
>>>>> chapter said the controller name is, say, 'MU121'. I am sure now you
>>>>> will see it correct to call it "fsl,mu121" compatible.
>>>>> What changed? just the name, right?
>>>>>
>>>>>
>>>>>> If we
>>>>>> ever going to want to validate DTs against the binding, all
>>>>>> compatibles
>>>>>> used in the DTs must be specified in the binding.
>>>>>>
>>>>>> As we can't really tell if the controller is exactly the same or
>>>>>> even
>>>>>> has some SoC integration bugs, we generally add a new compatible
>>>>>> for
>>>>>> each SoC to key off any workarounds necessary in the driver without
>>>>>> the
>>>>>> need to change the DTs, breaking compatibility.
>>>>>>
>>>>>
>>>>> I think if the h/w resources and behaviour remain the same and the
>>>>> documentation does not call it by a different name -- it is safe to
>>>>> assume its the same IP. Especially when the driver is absolutely
>>>>> indifferent to the 5 SoC names.
>>>>
>>>> Even if it is the same IP core, the SoC integration might have bugs
>>>> that need different behavior from the driver. We've already had that
>>>> case with the i.MX6 SPI controller.
>>>>
>>> For h/w quirks/bugs, a new "has-that-bug" property makes better sense.
>>> Or, if you insist, a new compatible based on the first soc that has
>>> the buggy block.
>>>
>>
>> How do you propose to handle this property in the driver, if once flashed
>> DTB does not contain it?
>>
> I see, your point is - what if a bug appears only in a particular SoC ?
> Well, if the controller is same the bug should appear to all SoCs.
> Do you think these SoCs have different versions of MU ?
> 

I'm not a hardware developer, but I'm quite sure that strictly speaking
IPs are different on different SoCs, the diversity of differences can be
negligible, or it can be discovered in future and considered as
significant, i.e. two IPs may be found as incompatible ones. Note that
the DTBs flashed on boards are already prepared for the worst scenario.

But to some extend, and in today's knowledge, the IPs are compatible,
let's say they are compatible to "fsl,imx6sx-mu" version, the one which
is found in the published driver.

--
Best wishes,
Vladimir

Re: [PATCH v7 4/6] dt-bindings: mailbox: imx-mu: add i.MX6SX and i.MX7S SoCs.

[Lucas Stach]

Am Donnerstag, den 26.07.2018, 17:16 +0530 schrieb Jassi Brar:
> > On Thu, Jul 26, 2018 at 5:05 PM, Lucas Stach <l.stach@pengutronix.de> wrote:
> > Am Donnerstag, den 26.07.2018, 16:45 +0530 schrieb Jassi Brar:
> > > On Thu, Jul 26, 2018 at 4:11 PM, Lucas Stach <l.stach@pengutronix.de>
> > > wrote:
> > > > Hi Jassi,
> > > > 
> > > > Am Donnerstag, den 26.07.2018, 15:25 +0530 schrieb Jassi Brar:
> > > > > On Thu, Jul 26, 2018 at 12:23 PM, Oleksij Rempel
> > > > > > > > > > > > <o.rempel@pengutronix.de> wrote:
> > > > > > This are currently tested SoCs with imx-mailbox driver.
> > > > > > 
> > > > > > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > > > > 
> > > > > > ---
> > > > > >  Documentation/devicetree/bindings/mailbox/fsl,mu.txt | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git
> > > > > > a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > > > > > b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > > > > > index 113d6ab931ef..5616d2afca45 100644
> > > > > > --- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > > > > > +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > > > > > @@ -18,7 +18,7 @@ Messaging Unit Device Node:
> > > > > >  Required properties:
> > > > > >  -------------------
> > > > > >  - compatible : should be "fsl,<chip>-mu", the supported chips
> > > > > > include
> > > > > > -               imx8qxp, imx8qm.
> > > > > > +               imx6sx, imx7s, imx8qxp, imx8qm.
> > > > > > 
> > > > > 
> > > > > This is not scalable. Do we add every new SoC that contains the
> > > > > same controller?
> > > > 
> > > > Yes, we do. This is a policy direction from the DT maintainers.
> > > > 
> > > 
> > > I would love to read the post/documentation.
> > > 
> > > Consider the same h/w - controller and platforms, but only the the MU
> > > chapter said the controller name is, say, 'MU121'. I am sure now you
> > > will see it correct to call it "fsl,mu121" compatible.
> > > What changed? just the name, right?
> > > 
> > > 
> > > > If we
> > > > ever going to want to validate DTs against the binding, all
> > > > compatibles
> > > > used in the DTs must be specified in the binding.
> > > > 
> > > > As we can't really tell if the controller is exactly the same or
> > > > even
> > > > has some SoC integration bugs, we generally add a new compatible
> > > > for
> > > > each SoC to key off any workarounds necessary in the driver without
> > > > the
> > > > need to change the DTs, breaking compatibility.
> > > > 
> > > 
> > > I think if the h/w resources and behaviour remain the same and the
> > > documentation does not call it by a different name -- it is safe to
> > > assume its the same IP. Especially when the driver is absolutely
> > > indifferent to the 5 SoC names.
> > 
> > Even if it is the same IP core, the SoC integration might have bugs
> > that need different behavior from the driver. We've already had that
> > case with the i.MX6 SPI controller.
> > 
> 
> For h/w quirks/bugs, a new "has-that-bug" property makes better sense.
> Or, if you insist, a new compatible based on the first soc that has
> the buggy block.
> 

Hardware bugs might not be known at the the time the DT is deployed.
The DT is not part of the kernel, but might be part of a fixed device
firmware.

> > > If/when we find the controller changes, we could revisit the binding
> > > and add another compatible option and modify the driver to catch that
> > > and adapt.
> > 
> > That's way too late. If we want to keep DTs stable
> > 
> 
> How do you keep the DT stable by explicitly defining every new SoC to
> the compatible list in DT, _add_ to the driver.... only to have the
> driver absolutely not care which SoC is it?
> Which is the situation right now with this patchset.

It means we can later, in a newer kernel change the driver to use the
compatible to change behavior without the need to update the DT at that
point.

Regards,
Lucas

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

Re: [PATCH v7 4/6] dt-bindings: mailbox: imx-mu: add i.MX6SX and i.MX7S SoCs.

[Vladimir Zapolskiy]

On 07/26/2018 02:15 PM, Jassi Brar wrote:
> On Thu, Jul 26, 2018 at 4:11 PM, Lucas Stach <l.stach@pengutronix.de> wrote:
>> Hi Jassi,
>>
>> Am Donnerstag, den 26.07.2018, 15:25 +0530 schrieb Jassi Brar:
>>> On Thu, Jul 26, 2018 at 12:23 PM, Oleksij Rempel
>>>> <o.rempel@pengutronix.de> wrote:
>>>> This are currently tested SoCs with imx-mailbox driver.
>>>>
>>>>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>>>> ---
>>>>  Documentation/devicetree/bindings/mailbox/fsl,mu.txt | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>>>> index 113d6ab931ef..5616d2afca45 100644
>>>> --- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>>>> +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>>>> @@ -18,7 +18,7 @@ Messaging Unit Device Node:
>>>>  Required properties:
>>>>  -------------------
>>>>  - compatible : should be "fsl,<chip>-mu", the supported chips include
>>>> -               imx8qxp, imx8qm.
>>>> +               imx6sx, imx7s, imx8qxp, imx8qm.
>>>>
>>>
>>> This is not scalable. Do we add every new SoC that contains the same controller?
>>
>> Yes, we do. This is a policy direction from the DT maintainers.
>>
> I would love to read the post/documentation.

Some notes are found in Documentation/devicetree/bindings/ABI.txt

To guarantee "a stable binding" of a compatible property, to avoid
unintentionally added incompatibilities and to fix a list of supported
compatibles in the device driver code there should be a SoC specific
compatible value in the list of 'compatible' property elements.

> Consider the same h/w - controller and platforms, but only the the MU
> chapter said the controller name is, say, 'MU121'. I am sure now you
> will see it correct to call it "fsl,mu121" compatible.
> What changed? just the name, right?
> 
> 
>> If we
>> ever going to want to validate DTs against the binding, all compatibles
>> used in the DTs must be specified in the binding.
>>
>> As we can't really tell if the controller is exactly the same or even
>> has some SoC integration bugs, we generally add a new compatible for
>> each SoC to key off any workarounds necessary in the driver without the
>> need to change the DTs, breaking compatibility.
>>
> I think if the h/w resources and behaviour remain the same and the
> documentation does not call it by a different name -- it is safe to
> assume its the same IP. Especially when the driver is absolutely
> indifferent to the 5 SoC names.
> 
> If/when we find the controller changes, we could revisit the binding
> and add another compatible option and modify the driver to catch that
> and adapt.
> 

Unfortunately it does not work well this way due to limited possibilities
to distinugush different device IPs on different SoCs, if identical
compatibles are given in both cases, and often it is not obvious that
two IPs are different.

--
Best wishes,
Vladimir

Re: [PATCH v7 4/6] dt-bindings: mailbox: imx-mu: add i.MX6SX and i.MX7S SoCs.

[Jassi Brar]

On Thu, Jul 26, 2018 at 5:14 PM, Vladimir Zapolskiy
<vladimir_zapolskiy@mentor.com> wrote:
> On 07/26/2018 02:15 PM, Jassi Brar wrote:
>> On Thu, Jul 26, 2018 at 4:11 PM, Lucas Stach <l.stach@pengutronix.de> wrote:
>>> Hi Jassi,
>>>
>>> Am Donnerstag, den 26.07.2018, 15:25 +0530 schrieb Jassi Brar:
>>>> On Thu, Jul 26, 2018 at 12:23 PM, Oleksij Rempel
>>>>> <o.rempel@pengutronix.de> wrote:
>>>>> This are currently tested SoCs with imx-mailbox driver.
>>>>>
>>>>>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>>>>> ---
>>>>>  Documentation/devicetree/bindings/mailbox/fsl,mu.txt | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>>>>> index 113d6ab931ef..5616d2afca45 100644
>>>>> --- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>>>>> +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>>>>> @@ -18,7 +18,7 @@ Messaging Unit Device Node:
>>>>>  Required properties:
>>>>>  -------------------
>>>>>  - compatible : should be "fsl,<chip>-mu", the supported chips include
>>>>> -               imx8qxp, imx8qm.
>>>>> +               imx6sx, imx7s, imx8qxp, imx8qm.
>>>>>
>>>>
>>>> This is not scalable. Do we add every new SoC that contains the same controller?
>>>
>>> Yes, we do. This is a policy direction from the DT maintainers.
>>>
>> I would love to read the post/documentation.
>
> Some notes are found in Documentation/devicetree/bindings/ABI.txt
>
Thanks. I have been through the ABI.txt.

> To guarantee "a stable binding" of a compatible property, to avoid
> unintentionally added incompatibilities and to fix a list of supported
> compatibles in the device driver code there should be a SoC specific
> compatible value in the list of 'compatible' property elements.
>
>> Consider the same h/w - controller and platforms, but only the the MU
>> chapter said the controller name is, say, 'MU121'. I am sure now you
>> will see it correct to call it "fsl,mu121" compatible.
>> What changed? just the name, right?
>>
>>
>>> If we
>>> ever going to want to validate DTs against the binding, all compatibles
>>> used in the DTs must be specified in the binding.
>>>
>>> As we can't really tell if the controller is exactly the same or even
>>> has some SoC integration bugs, we generally add a new compatible for
>>> each SoC to key off any workarounds necessary in the driver without the
>>> need to change the DTs, breaking compatibility.
>>>
>> I think if the h/w resources and behaviour remain the same and the
>> documentation does not call it by a different name -- it is safe to
>> assume its the same IP. Especially when the driver is absolutely
>> indifferent to the 5 SoC names.
>>
>> If/when we find the controller changes, we could revisit the binding
>> and add another compatible option and modify the driver to catch that
>> and adapt.
>>
>
> Unfortunately it does not work well this way due to limited possibilities
> to distinugush different device IPs on different SoCs, if identical
> compatibles are given in both cases, and often it is not obvious that
> two IPs are different.
>
Please note the submitted driver absolutely don't care which of the
five SoCs it is.
In other words, all these SoCs have the same controller.  So its about
have just one compatible right now, and add more if some new SoC comes
with a variation of the controller.

Re: [PATCH v7 4/6] dt-bindings: mailbox: imx-mu: add i.MX6SX and i.MX7S SoCs.

[Vladimir Zapolskiy]

On 07/26/2018 02:52 PM, Jassi Brar wrote:
> On Thu, Jul 26, 2018 at 5:14 PM, Vladimir Zapolskiy
> <vladimir_zapolskiy@mentor.com> wrote:
>> On 07/26/2018 02:15 PM, Jassi Brar wrote:
>>> On Thu, Jul 26, 2018 at 4:11 PM, Lucas Stach <l.stach@pengutronix.de> wrote:
>>>> Hi Jassi,
>>>>
>>>> Am Donnerstag, den 26.07.2018, 15:25 +0530 schrieb Jassi Brar:
>>>>> On Thu, Jul 26, 2018 at 12:23 PM, Oleksij Rempel
>>>>>> <o.rempel@pengutronix.de> wrote:
>>>>>> This are currently tested SoCs with imx-mailbox driver.
>>>>>>
>>>>>>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>>>>>> ---
>>>>>>  Documentation/devicetree/bindings/mailbox/fsl,mu.txt | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>>>>>> index 113d6ab931ef..5616d2afca45 100644
>>>>>> --- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>>>>>> +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>>>>>> @@ -18,7 +18,7 @@ Messaging Unit Device Node:
>>>>>>  Required properties:
>>>>>>  -------------------
>>>>>>  - compatible : should be "fsl,<chip>-mu", the supported chips include
>>>>>> -               imx8qxp, imx8qm.
>>>>>> +               imx6sx, imx7s, imx8qxp, imx8qm.
>>>>>>
>>>>>
>>>>> This is not scalable. Do we add every new SoC that contains the same controller?
>>>>
>>>> Yes, we do. This is a policy direction from the DT maintainers.
>>>>
>>> I would love to read the post/documentation.
>>
>> Some notes are found in Documentation/devicetree/bindings/ABI.txt
>>
> Thanks. I have been through the ABI.txt.
> 
>> To guarantee "a stable binding" of a compatible property, to avoid
>> unintentionally added incompatibilities and to fix a list of supported
>> compatibles in the device driver code there should be a SoC specific
>> compatible value in the list of 'compatible' property elements.
>>
>>> Consider the same h/w - controller and platforms, but only the the MU
>>> chapter said the controller name is, say, 'MU121'. I am sure now you
>>> will see it correct to call it "fsl,mu121" compatible.
>>> What changed? just the name, right?
>>>
>>>
>>>> If we
>>>> ever going to want to validate DTs against the binding, all compatibles
>>>> used in the DTs must be specified in the binding.
>>>>
>>>> As we can't really tell if the controller is exactly the same or even
>>>> has some SoC integration bugs, we generally add a new compatible for
>>>> each SoC to key off any workarounds necessary in the driver without the
>>>> need to change the DTs, breaking compatibility.
>>>>
>>> I think if the h/w resources and behaviour remain the same and the
>>> documentation does not call it by a different name -- it is safe to
>>> assume its the same IP. Especially when the driver is absolutely
>>> indifferent to the 5 SoC names.
>>>
>>> If/when we find the controller changes, we could revisit the binding
>>> and add another compatible option and modify the driver to catch that
>>> and adapt.
>>>
>>
>> Unfortunately it does not work well this way due to limited possibilities
>> to distinugush different device IPs on different SoCs, if identical
>> compatibles are given in both cases, and often it is not obvious that
>> two IPs are different.
>>
> Please note the submitted driver absolutely don't care which of the
> five SoCs it is.

True.

> In other words, all these SoCs have the same controller.

False :)

> So its about have just one compatible right now, and add more if some
> new SoC comes with a variation of the controller.
> 

True. The driver will be changed in this case, unfortunately the bindings
are not so volatile.

--
Best wishes,
Vladimir

Re: [PATCH v7 4/6] dt-bindings: mailbox: imx-mu: add i.MX6SX and i.MX7S SoCs.

[Jassi Brar]

On Thu, Jul 26, 2018 at 5:25 PM, Vladimir Zapolskiy
<vladimir_zapolskiy@mentor.com> wrote:
> On 07/26/2018 02:52 PM, Jassi Brar wrote:
>> On Thu, Jul 26, 2018 at 5:14 PM, Vladimir Zapolskiy
>> <vladimir_zapolskiy@mentor.com> wrote:

>>>
>> Please note the submitted driver absolutely don't care which of the
>> five SoCs it is.
>
> True.
>

>> In other words, all these SoCs have the same controller.
>
> False :)
>
OK, so the controllers are not identical, but same enough to have a
common driver?
Is the driver not tested enough or are you planning to add more features?

>> So its about have just one compatible right now, and add more if some
>> new SoC comes with a variation of the controller.
>>
>
> True. The driver will be changed in this case, unfortunately the bindings
> are not so volatile.
>
The volatility of the bindings will be same when you add a new SoC
compatible ;)  And you must add to bindings+driver, or the same h/w
and same driver won't work for your new platform.

Re: [PATCH v7 4/6] dt-bindings: mailbox: imx-mu: add i.MX6SX and i.MX7S SoCs.

[Vladimir Zapolskiy]

On 07/26/2018 03:10 PM, Jassi Brar wrote:
> On Thu, Jul 26, 2018 at 5:25 PM, Vladimir Zapolskiy
> <vladimir_zapolskiy-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org> wrote:
>> On 07/26/2018 02:52 PM, Jassi Brar wrote:
>>> On Thu, Jul 26, 2018 at 5:14 PM, Vladimir Zapolskiy
>>> <vladimir_zapolskiy-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org> wrote:
> 
>>>>
>>> Please note the submitted driver absolutely don't care which of the
>>> five SoCs it is.
>>
>> True.
>>
> 
>>> In other words, all these SoCs have the same controller.
>>
>> False :)
>>

To be more precice, the statement itself may be true or false, but
the implication ("In other words, ...") is definitely false.

> OK, so the controllers are not identical, but same enough to have a
> common driver?

Right, the controllers are compatible, but likely they are non-identical.

> Is the driver not tested enough or are you planning to add more features?
> 

Test results can not serve as a formal eternal proof, but they are good as
a hint. Also I'm not aware of any pending features to be added to the driver.

>>> So its about have just one compatible right now, and add more if some
>>> new SoC comes with a variation of the controller.
>>>
>>
>> True. The driver will be changed in this case, unfortunately the bindings
>> are not so volatile.
>>
> The volatility of the bindings will be same when you add a new SoC
> compatible ;)  And you must add to bindings+driver, or the same h/w
> and same driver won't work for your new platform.
> 

The (non-)volatilitily of the bindings is important only in retrospective
view.

Adding a new compatible to the documentation and unrelated board DTBs is
irrelevant, and it is quite predictable that it will happen, when a new
SoC with the same "compatible" IP is released.

--
Best wishes,
Vladimir

Re: [PATCH v7 4/6] dt-bindings: mailbox: imx-mu: add i.MX6SX and i.MX7S SoCs.

[Rob Herring]

On Thu, Jul 26, 2018 at 03:25:15PM +0530, Jassi Brar wrote:
> On Thu, Jul 26, 2018 at 12:23 PM, Oleksij Rempel
> <o.rempel@pengutronix.de> wrote:
> > This are currently tested SoCs with imx-mailbox driver.
> >
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  Documentation/devicetree/bindings/mailbox/fsl,mu.txt | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > index 113d6ab931ef..5616d2afca45 100644
> > --- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
> > @@ -18,7 +18,7 @@ Messaging Unit Device Node:
> >  Required properties:
> >  -------------------
> >  - compatible : should be "fsl,<chip>-mu", the supported chips include
> > -               imx8qxp, imx8qm.
> > +               imx6sx, imx7s, imx8qxp, imx8qm.
> >
> This is not scalable. Do we add every new SoC that contains the same controller?
> I think the controller name, if it has one or create a new one like
> 'imx-mu',  should be used here.
> So,
>     compatible : should be "fsl,imx-mu"

No, for all the reasons already correctly explained in this thread.

> BTW, the driver doesn't even probe anything other than
> 'fsl,imx6sx-mu', unlike what the binding says.

This aspect should be documented here though. The documentation should 
clearly define what are valid combinations of compatibles.

Rob

Re: [PATCH v7 4/6] dt-bindings: mailbox: imx-mu: add i.MX6SX and i.MX7S SoCs.

[Jassi Brar]

On Tue, Jul 31, 2018 at 3:52 AM, Rob Herring <robh@kernel.org> wrote:
> On Thu, Jul 26, 2018 at 03:25:15PM +0530, Jassi Brar wrote:
>> On Thu, Jul 26, 2018 at 12:23 PM, Oleksij Rempel
>> <o.rempel@pengutronix.de> wrote:
>> > This are currently tested SoCs with imx-mailbox driver.
>> >
>> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>> > ---
>> >  Documentation/devicetree/bindings/mailbox/fsl,mu.txt | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>> > index 113d6ab931ef..5616d2afca45 100644
>> > --- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>> > +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>> > @@ -18,7 +18,7 @@ Messaging Unit Device Node:
>> >  Required properties:
>> >  -------------------
>> >  - compatible : should be "fsl,<chip>-mu", the supported chips include
>> > -               imx8qxp, imx8qm.
>> > +               imx6sx, imx7s, imx8qxp, imx8qm.
>> >
>> This is not scalable. Do we add every new SoC that contains the same controller?
>> I think the controller name, if it has one or create a new one like
>> 'imx-mu',  should be used here.
>> So,
>>     compatible : should be "fsl,imx-mu"
>
> No, for all the reasons already correctly explained in this thread.
>
OK. I curious
>> BTW, the driver doesn't even probe anything other than
>> 'fsl,imx6sx-mu', unlike what the binding says.
>
> This aspect should be documented here though. The documentation should
> clearly define what are valid combinations of compatibles.
>
> Rob

Re: [PATCH v7 4/6] dt-bindings: mailbox: imx-mu: add i.MX6SX and i.MX7S SoCs.

[Jassi Brar]

On Tue, Jul 31, 2018 at 3:52 AM, Rob Herring <robh@kernel.org> wrote:
> On Thu, Jul 26, 2018 at 03:25:15PM +0530, Jassi Brar wrote:
>> On Thu, Jul 26, 2018 at 12:23 PM, Oleksij Rempel
>> <o.rempel@pengutronix.de> wrote:
>> > This are currently tested SoCs with imx-mailbox driver.
>> >
>> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>> > ---
>> >  Documentation/devicetree/bindings/mailbox/fsl,mu.txt | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>> > index 113d6ab931ef..5616d2afca45 100644
>> > --- a/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>> > +++ b/Documentation/devicetree/bindings/mailbox/fsl,mu.txt
>> > @@ -18,7 +18,7 @@ Messaging Unit Device Node:
>> >  Required properties:
>> >  -------------------
>> >  - compatible : should be "fsl,<chip>-mu", the supported chips include
>> > -               imx8qxp, imx8qm.
>> > +               imx6sx, imx7s, imx8qxp, imx8qm.
>> >
>> This is not scalable. Do we add every new SoC that contains the same controller?
>> I think the controller name, if it has one or create a new one like
>> 'imx-mu',  should be used here.
>> So,
>>     compatible : should be "fsl,imx-mu"
>
> No, for all the reasons already correctly explained in this thread.
>
OK. I am curious to see what soc specific fixes we need in future.

Thanks.

[PATCH v7 5/6] ARM: dts: imx7s: add i.MX7 messaging unit support

[Oleksij Rempel]

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

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

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

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

[Lucas Stach]

Am Donnerstag, den 26.07.2018, 08:53 +0200 schrieb Oleksij Rempel:
> Define the Messaging Unit (MU) for i.MX7 in the processor's dtsi.
> The respective driver is added in the next commit.
> 
> > Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

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

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

[PATCH v7 6/6] 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 | 276 ++++++++++++++++++++++++++++++++++
 3 files changed, 284 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..ecfddca5e31c
--- /dev/null
+++ b/drivers/mailbox/imx-mailbox.c
@@ -0,0 +1,276 @@
+// 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_TEn(x)	BIT(20 + (3 - (x)))
+#define IMX_MU_xSR_RFn(x)	BIT(24 + (3 - (x)))
+#define IMX_MU_xSR_BRDIP	BIT(9)
+
+/* Control Register */
+#define IMX_MU_xCR		0x24
+/* Transmit Interrupt Enable */
+#define IMX_MU_xCR_TIEn(x)	BIT(20 + (3 - (x)))
+/* Receive Interrupt Enable */
+#define IMX_MU_xCR_RIEn(x)	BIT(24 + (3 - (x)))
+
+#define IMX_MU_CHANS	4u
+
+struct imx_mu_con_priv {
+	int			irq;
+	unsigned int		idx;
+	char			*irq_desc;
+};
+
+struct imx_mu_priv {
+	struct device		*dev;
+	void __iomem		*base;
+	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;
+
+	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 irqreturn_t imx_mu_isr(int irq, void *p)
+{
+	struct mbox_chan *chan = p;
+	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
+	struct imx_mu_con_priv *cp = chan->con_priv;
+	u32 val, ctrl, dat;
+
+	ctrl = imx_mu_read(priv, IMX_MU_xCR);
+	val = imx_mu_read(priv, IMX_MU_xSR);
+	val &= IMX_MU_xSR_TEn(cp->idx) | IMX_MU_xSR_RFn(cp->idx);
+	val &= ctrl & (IMX_MU_xCR_TIEn(cp->idx) | IMX_MU_xCR_RIEn(cp->idx));
+	if (!val)
+		return IRQ_NONE;
+
+	if (val & IMX_MU_xSR_TEn(cp->idx)) {
+		imx_mu_xcr_rmw(priv, 0, IMX_MU_xCR_TIEn(cp->idx));
+		mbox_chan_txdone(chan, 0);
+	}
+
+	if (val & IMX_MU_xSR_RFn(cp->idx)) {
+		dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
+		mbox_chan_received_data(chan, (void *)&dat);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static bool imx_mu_last_tx_done(struct mbox_chan *chan)
+{
+	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
+	struct imx_mu_con_priv *cp = chan->con_priv;
+
+	/* 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;
+
+	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_xSR_TEn(cp->idx), 0);
+
+	return 0;
+}
+
+static int imx_mu_startup(struct mbox_chan *chan)
+{
+	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
+	struct imx_mu_con_priv *cp = chan->con_priv;
+	int ret;
+
+	cp->irq_desc = kasprintf(GFP_KERNEL, "imx_mu_chan[%i]", cp->idx);
+	if (!cp->irq_desc)
+		return -ENOMEM;
+
+	ret = request_irq(cp->irq, imx_mu_isr, IRQF_SHARED, cp->irq_desc,
+			  chan);
+	if (ret) {
+		dev_err(priv->dev,
+			"Unable to acquire IRQ %d\n", cp->irq);
+		return ret;
+	}
+
+	imx_mu_xcr_rmw(priv, IMX_MU_xCR_RIEn(cp->idx), 0);
+
+	return 0;
+}
+
+static void imx_mu_shutdown(struct mbox_chan *chan)
+{
+	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
+	struct imx_mu_con_priv *cp = chan->con_priv;
+
+	imx_mu_xcr_rmw(priv, 0,
+		   IMX_MU_xCR_TIEn(cp->idx) | IMX_MU_xCR_RIEn(cp->idx));
+
+	free_irq(cp->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 void imx_mu_init_generic(struct imx_mu_priv *priv)
+{
+	if (priv->side_b)
+		return;
+
+	/* Set default MU configuration */
+	imx_mu_write(priv, 0, IMX_MU_xCR);
+}
+
+static int imx_mu_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct resource *iomem;
+	struct imx_mu_priv *priv;
+	unsigned int i;
+	int irq, ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+
+	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(&pdev->dev, iomem);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	priv->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(priv->clk)) {
+		if (PTR_ERR(priv->clk) != -ENOENT)
+			return PTR_ERR(priv->clk);
+
+		priv->clk = NULL;
+	}
+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret) {
+		dev_err(dev, "Failed to enable clock\n");
+		return ret;
+	}
+
+	for (i = 0; i < IMX_MU_CHANS; i++) {
+		struct imx_mu_con_priv *cp = &priv->con_priv[i];
+
+		cp->idx = i;
+		cp->irq = irq;
+		priv->mbox_chans[i].con_priv = cp;
+	}
+
+	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.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 v7 6/6] mailbox: Add support for i.MX7D messaging unit

[Lucas Stach]

Am Donnerstag, den 26.07.2018, 08:53 +0200 schrieb Oleksij Rempel:
> The Mailbox controller is able to send messages (up to 4 32 bit words)
> between the endpoints.
> 
> This driver was tested using the mailbox-test driver sending messages
> between the Cortex-A7 and the Cortex-M4.
> 
> > Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>
> > Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
>  drivers/mailbox/Kconfig       |   6 +
>  drivers/mailbox/Makefile      |   2 +
>  drivers/mailbox/imx-mailbox.c | 276 ++++++++++++++++++++++++++++++++++
>  3 files changed, 284 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..ecfddca5e31c
> --- /dev/null
> +++ b/drivers/mailbox/imx-mailbox.c
> @@ -0,0 +1,276 @@
> +// 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_TEn(x)	BIT(20 + (3 - (x)))
> > +#define IMX_MU_xSR_RFn(x)	BIT(24 + (3 - (x)))
> > +#define IMX_MU_xSR_BRDIP	BIT(9)
> +
> +/* Control Register */
> > +#define IMX_MU_xCR		0x24
> +/* Transmit Interrupt Enable */
> > +#define IMX_MU_xCR_TIEn(x)	BIT(20 + (3 - (x)))
> +/* Receive Interrupt Enable */
> > +#define IMX_MU_xCR_RIEn(x)	BIT(24 + (3 - (x)))
> +
> > +#define IMX_MU_CHANS	4u
> +
> +struct imx_mu_con_priv {
> > > +	int			irq;
> > > +	unsigned int		idx;
> > > +	char			*irq_desc;
> +};
> +
> +struct imx_mu_priv {
> > > +	struct device		*dev;
> > > +	void __iomem		*base;
> > > +	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;
> +
> > > +	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 irqreturn_t imx_mu_isr(int irq, void *p)
> +{
> > +	struct mbox_chan *chan = p;
> > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > +	u32 val, ctrl, dat;
> +
> > +	ctrl = imx_mu_read(priv, IMX_MU_xCR);
> > +	val = imx_mu_read(priv, IMX_MU_xSR);
> > +	val &= IMX_MU_xSR_TEn(cp->idx) | IMX_MU_xSR_RFn(cp->idx);
> > +	val &= ctrl & (IMX_MU_xCR_TIEn(cp->idx) | IMX_MU_xCR_RIEn(cp->idx));
> > +	if (!val)
> > +		return IRQ_NONE;
> +
> > +	if (val & IMX_MU_xSR_TEn(cp->idx)) {
> > +		imx_mu_xcr_rmw(priv, 0, IMX_MU_xCR_TIEn(cp->idx));
> > +		mbox_chan_txdone(chan, 0);
> > +	}
> +
> > +	if (val & IMX_MU_xSR_RFn(cp->idx)) {
> > +		dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
> > +		mbox_chan_received_data(chan, (void *)&dat);
> > +	}
> +
> > +	return IRQ_HANDLED;
> +}
> +
> +static bool imx_mu_last_tx_done(struct mbox_chan *chan)
> +{
> > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > +	struct imx_mu_con_priv *cp = chan->con_priv;
> +
> > +	/* 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;
> +
> > +	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_xSR_TEn(cp->idx), 0);
> +
> > +	return 0;
> +}
> +
> +static int imx_mu_startup(struct mbox_chan *chan)
> +{
> > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > +	struct imx_mu_con_priv *cp = chan->con_priv;
> > +	int ret;
> +
> > +	cp->irq_desc = kasprintf(GFP_KERNEL, "imx_mu_chan[%i]", cp->idx);
> > +	if (!cp->irq_desc)
> > +		return -ENOMEM;
> +
> > +	ret = request_irq(cp->irq, imx_mu_isr, IRQF_SHARED, cp->irq_desc,
> > +			  chan);
> > +	if (ret) {
> > +		dev_err(priv->dev,
> > +			"Unable to acquire IRQ %d\n", cp->irq);
> > +		return ret;
> > +	}
> +
> > +	imx_mu_xcr_rmw(priv, IMX_MU_xCR_RIEn(cp->idx), 0);
> +
> > +	return 0;
> +}
> +
> +static void imx_mu_shutdown(struct mbox_chan *chan)
> +{
> > +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> > +	struct imx_mu_con_priv *cp = chan->con_priv;
> +
> > +	imx_mu_xcr_rmw(priv, 0,
> > +		   IMX_MU_xCR_TIEn(cp->idx) | IMX_MU_xCR_RIEn(cp->idx));
> +
> > +	free_irq(cp->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 void imx_mu_init_generic(struct imx_mu_priv *priv)
> +{
> > +	if (priv->side_b)
> > +		return;
> +
> > +	/* Set default MU configuration */
> > +	imx_mu_write(priv, 0, IMX_MU_xCR);
> +}
> +
> +static int imx_mu_probe(struct platform_device *pdev)
> +{
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *np = dev->of_node;
> > +	struct resource *iomem;
> > +	struct imx_mu_priv *priv;
> > +	unsigned int i;
> > +	int irq, ret;
> +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> +
> > +	priv->dev = dev;
> +
> > +	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	priv->base = devm_ioremap_resource(&pdev->dev, iomem);
> > +	if (IS_ERR(priv->base))
> > +		return PTR_ERR(priv->base);
> +
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq < 0)
> > +		return irq;
> +
> > +	priv->clk = devm_clk_get(dev, NULL);
> > +	if (IS_ERR(priv->clk)) {
> > +		if (PTR_ERR(priv->clk) != -ENOENT)
> > +			return PTR_ERR(priv->clk);
> +
> > +		priv->clk = NULL;
> > +	}
> +
> > +	ret = clk_prepare_enable(priv->clk);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to enable clock\n");
> > +		return ret;
> > +	}
> +
> > +	for (i = 0; i < IMX_MU_CHANS; i++) {
> > +		struct imx_mu_con_priv *cp = &priv->con_priv[i];
> +
> > +		cp->idx = i;
> > +		cp->irq = irq;
> > +		priv->mbox_chans[i].con_priv = cp;
> > +	}
> +
> > +	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.txdone_irq = true;
> +
> > +	platform_set_drvdata(pdev, priv);
> +
> > +	imx_mu_init_generic(priv);
> +
> > +	return mbox_controller_register(&priv->mbox);
> +}
> +
> +static int imx_mu_remove(struct platform_device *pdev)
> +{
> > +	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
> +
> > +	mbox_controller_unregister(&priv->mbox);
> > +	clk_disable_unprepare(priv->clk);
> +
> > +	return 0;
> +}
> +
> +static const struct of_device_id imx_mu_dt_ids[] = {
> > +	{ .compatible = "fsl,imx6sx-mu" },
> > +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, imx_mu_dt_ids);
> +
> +static struct platform_driver imx_mu_driver = {
> > > +	.probe		= imx_mu_probe,
> > > +	.remove		= imx_mu_remove,
> > +	.driver = {
> > > +		.name	= "imx_mu",
> > +		.of_match_table = imx_mu_dt_ids,
> > +	},
> +};
> +module_platform_driver(imx_mu_driver);
> +
> > +MODULE_AUTHOR("Oleksij Rempel <o.rempel@pengutronix.de>");
> +MODULE_DESCRIPTION("Message Unit driver for i.MX");
> +MODULE_LICENSE("GPL v2");

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

Re: [PATCH v7 6/6] mailbox: Add support for i.MX7D messaging unit

[Jassi Brar]

On Thu, Jul 26, 2018 at 12:23 PM, Oleksij Rempel
<o.rempel@pengutronix.de> wrote:

.....

> +
> +/* Control Register */
> +#define IMX_MU_xCR             0x24
> +/* Transmit Interrupt Enable */
> +#define IMX_MU_xCR_TIEn(x)     BIT(20 + (3 - (x)))
> +/* Receive Interrupt Enable */
> +#define IMX_MU_xCR_RIEn(x)     BIT(24 + (3 - (x)))
> +
> +#define IMX_MU_CHANS   4u
> +
The 'u' unnecessarily refreshes my knowledge of C  :)


> +struct imx_mu_con_priv {
> +       int                     irq;
> +       unsigned int            idx;
> +       char                    *irq_desc;
>
Having      char  irq_desc[16];    seems simpler than free/kfree upon
each startup/shutdown. Simply use snprintf() in probe().

The rest of the driver seems fine.

Cheers!

Re: [PATCH v7 6/6] mailbox: Add support for i.MX7D messaging unit

[Oleksij Rempel]

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



On 26.07.2018 12:15, Jassi Brar wrote:
> On Thu, Jul 26, 2018 at 12:23 PM, Oleksij Rempel
> <o.rempel@pengutronix.de> wrote:
> 
> .....
> 
>> +
>> +/* Control Register */
>> +#define IMX_MU_xCR             0x24
>> +/* Transmit Interrupt Enable */
>> +#define IMX_MU_xCR_TIEn(x)     BIT(20 + (3 - (x)))
>> +/* Receive Interrupt Enable */
>> +#define IMX_MU_xCR_RIEn(x)     BIT(24 + (3 - (x)))
>> +
>> +#define IMX_MU_CHANS   4u
>> +
> The 'u' unnecessarily refreshes my knowledge of C  :)

ok

> 
>> +struct imx_mu_con_priv {
>> +       int                     irq;
>> +       unsigned int            idx;
>> +       char                    *irq_desc;
>>
> Having      char  irq_desc[16];    seems simpler than free/kfree upon
> each startup/shutdown. Simply use snprintf() in probe().

Currently all known configurations use one or two channels. I don't see
any sense to allocate all at the probe.


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

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

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

Re: [PATCH v7 6/6] mailbox: Add support for i.MX7D messaging unit

[Jassi Brar]

On Thu, Jul 26, 2018 at 4:21 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:

>>
>>> +struct imx_mu_con_priv {
>>> +       int                     irq;
>>> +       unsigned int            idx;
>>> +       char                    *irq_desc;
>>>
>> Having      char  irq_desc[16];    seems simpler than free/kfree upon
>> each startup/shutdown. Simply use snprintf() in probe().
>
> Currently all known configurations use one or two channels. I don't see
> any sense to allocate all at the probe.
>
Just because only 2 out of 4 channels are being used, it doesn't make sense?

What is the concern?  "wasting" 16x2 bytes of memory?  How many bytes,
LOC and execution time does kasprintf() waste?

Re: [PATCH v7 6/6] mailbox: Add support for i.MX7D messaging unit

[Oleksij Rempel]

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



On 26.07.2018 13:09, Jassi Brar wrote:
> On Thu, Jul 26, 2018 at 4:21 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> 
>>>
>>>> +struct imx_mu_con_priv {
>>>> +       int                     irq;
>>>> +       unsigned int            idx;
>>>> +       char                    *irq_desc;
>>>>
>>> Having      char  irq_desc[16];    seems simpler than free/kfree upon
>>> each startup/shutdown. Simply use snprintf() in probe().
>>
>> Currently all known configurations use one or two channels. I don't see
>> any sense to allocate all at the probe.
>>
> Just because only 2 out of 4 channels are being used, it doesn't make sense?
> 
> What is the concern?  "wasting" 16x2 bytes of memory?  How many bytes,
> LOC and execution time does kasprintf() waste?

Startup or shutdown is not speed critical path in this driver, or do i
miss some thing? Speed vs size is not valid argumentation here.


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

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

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

Re: [PATCH v7 6/6] mailbox: Add support for i.MX7D messaging unit

[Jassi Brar]

On Thu, Jul 26, 2018 at 5:12 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
>
>
> On 26.07.2018 13:09, Jassi Brar wrote:
>> On Thu, Jul 26, 2018 at 4:21 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
>>
>>>>
>>>>> +struct imx_mu_con_priv {
>>>>> +       int                     irq;
>>>>> +       unsigned int            idx;
>>>>> +       char                    *irq_desc;
>>>>>
>>>> Having      char  irq_desc[16];    seems simpler than free/kfree upon
>>>> each startup/shutdown. Simply use snprintf() in probe().
>>>
>>> Currently all known configurations use one or two channels. I don't see
>>> any sense to allocate all at the probe.
>>>
>> Just because only 2 out of 4 channels are being used, it doesn't make sense?
>>
>> What is the concern?  "wasting" 16x2 bytes of memory?  How many bytes,
>> LOC and execution time does kasprintf() waste?
>
> Startup or shutdown is not speed critical path in this driver, or do i
> miss some thing?
>
No they are not.

> Speed vs size is not valid argumentation here.
>
If the code looks cleaner and smaller and lesser expensive execution
wise, at least I see that as a valid enough argument.

But OK, if you insist.  If someone sends a "cleanup" patch tomorrow, I
am going to thank and apply.