[1/4] mailbox: arm_mhuv2: add device tree binding documentation

From: Morten Borup Petersen <morten.petersen@arm.com>

This patch adds device tree binding for Message Handling Unit
controller version 2 from Arm.

Signed-off-by: Morten Borup Petersen <morten.petersen@arm.com>
Signed-off-by: Tushar Khandelwal <tushar.khandelwal@arm.com>
Cc: robh+dt@kernel.org
Cc: mark.rutland@arm.com
Cc: devicetree@vger.kernel.org
Cc: jassisinghbrar@gmail.com
---
 .../devicetree/bindings/mailbox/arm,mhuv2.txt | 108 ++++++++++++++++++
 1 file changed, 108 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt

On Wed, Jul 17, 2019 at 2:26 PM Tushar Khandelwal
<tushar.khandelwal@arm.com> wrote:

> diff --git a/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
> new file mode 100644
> index 000000000000..3a05593414bc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
> @@ -0,0 +1,108 @@
> +Arm MHUv2 Mailbox Driver
> +========================
> +
> +The Arm Message-Handling-Unit (MHU) Version 2 is a mailbox controller that has
> +between 1 and 124 channel windows to provide unidirectional communication with
> +remote processor(s).
> +
> +Given the unidirectional nature of the device, an MHUv2 mailbox may only be
> +written to or read from. If a pair of MHU devices is implemented between two
> +processing elements to provide bidirectional communication, these must be
> +specified as two separate mailboxes.
> +
> +A device tree node for an Arm MHUv2 device must specify either a receiver frame
> +or a sender frame, indicating which end of the unidirectional MHU device which
> +the device node entry describes.
> +
> +An MHU device must be specified with a transport protocol. The transport
> +protocol of an MHU device determines the method of data transmission as well as
> +the number of provided mailboxes.
> +Following are the possible transport protocol types:
> +- Single-word: An MHU device implements as many mailboxes as it
> +               provides channel windows. Data is transmitted through
> +               the MHU registers.
> +- Multi-word:  An MHU device implements a single mailbox. All channel windows
> +               will be used during transmission. Data is transmitted through
> +               the MHU registers.
> +- Doorbell:    An MHU device implements as many mailboxes as there are flag
> +               bits available in its channel windows. Optionally, data may
> +               be transmitted through a shared memory region, wherein the MHU
> +               is used strictly as an interrupt generation mechanism.
> +
> +Mailbox Device Node:
> +====================
> +
> +Required properties:
> +--------------------
> +- compatible:  Shall be "arm,mhuv2" & "arm,primecell"
> +- reg:         Contains the mailbox register address range (base
> +               address and length)
> +- #mbox-cells  Shall be 1 - the index of the channel needed.
> +- mhu-frame    Frame type of the device.
> +               Shall be either "sender" or "receiver"
> +- mhu-protocol Transport protocol of the device. Shall be one of the
> +               following: "single-word", "multi-word", "doorbell"
> +
> +Required properties (receiver frame):
> +-------------------------------------
> +- interrupts:  Contains the interrupt information corresponding to the
> +               combined interrupt of the receiver frame
> +
> +Example:
> +--------
> +
> +       mbox_mw_tx: mhu@10000000 {
> +               compatible = "arm,mhuv2","arm,primecell";
> +               reg = <0x10000000 0x1000>;
> +               clocks = <&refclk100mhz>;
> +               clock-names = "apb_pclk";
> +               #mbox-cells = <1>;
> +               mhu-protocol = "multi-word";
> +               mhu-frame = "sender";
> +       };
> +
> +       mbox_sw_tx: mhu@10000000 {
> +               compatible = "arm,mhuv2","arm,primecell";
> +               reg = <0x11000000 0x1000>;
> +               clocks = <&refclk100mhz>;
> +               clock-names = "apb_pclk";
> +               #mbox-cells = <1>;
> +               mhu-protocol = "single-word";
> +               mhu-frame = "sender";
> +       };
> +
> +       mbox_db_rx: mhu@10000000 {
> +               compatible = "arm,mhuv2","arm,primecell";
> +               reg = <0x12000000 0x1000>;
> +               clocks = <&refclk100mhz>;
> +               clock-names = "apb_pclk";
> +               #mbox-cells = <1>;
> +               interrupts = <0 45 4>;
> +               interrupt-names = "mhu_rx";
> +               mhu-protocol = "doorbell";
> +               mhu-frame = "receiver";
> +       };
> +
> +       mhu_client: scb@2e000000 {
> +       compatible = "fujitsu,mb86s70-scb-1.0";
> +       reg = <0 0x2e000000 0x4000>;
> +       mboxes =
> +               // For multi-word frames, client may only instantiate a single
> +               // mailbox for a mailbox controller
> +               <&mbox_mw_tx 0>,
> +
> +               // For single-word frames, client may instantiate as many
> +               // mailboxes as there are channel windows in the MHU
> +                <&mbox_sw_tx 0>,
> +                <&mbox_sw_tx 1>,
> +                <&mbox_sw_tx 2>,
> +                <&mbox_sw_tx 3>,
> +
> +               // For doorbell frames, client may instantiate as many mailboxes
> +               // as there are bits available in the combined number of channel
> +               // windows ((channel windows * 32) mailboxes)
> +                <mbox_db_rx 0>,
> +                <mbox_db_rx 1>,
> +                ...
> +                <mbox_db_rx 17>;
> +       };

If the mhuv2 instance implements, say, 3 channel windows between
sender (linux) and receiver (firmware), and Linux runs two protocols
each requiring 1 and 2-word sized messages respectively. The hardware
supports that by assigning windows [0] and [1,2] to each protocol.
However, I don't think the driver can support that. Or does it?

Also I see problem with implementing "protocol modes" in the
controller driver - 'mhu-protocol' property should go away.  And
'mhu-frame' is unncessary - presence of interrupt property could imply
'receiver', otherwise 'sender'.

Cheers!

On Sun, Jul 21, 2019 at 4:58 PM Jassi Brar <jassisinghbrar@gmail.com> wrote:
>
> On Wed, Jul 17, 2019 at 2:26 PM Tushar Khandelwal
> <tushar.khandelwal@arm.com> wrote:
>
> > diff --git a/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
> > new file mode 100644
> > index 000000000000..3a05593414bc
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
> > @@ -0,0 +1,108 @@
> > +Arm MHUv2 Mailbox Driver
> > +========================
> > +
> > +The Arm Message-Handling-Unit (MHU) Version 2 is a mailbox controller that has
> > +between 1 and 124 channel windows to provide unidirectional communication with
> > +remote processor(s).
> > +
> > +Given the unidirectional nature of the device, an MHUv2 mailbox may only be
> > +written to or read from. If a pair of MHU devices is implemented between two
> > +processing elements to provide bidirectional communication, these must be
> > +specified as two separate mailboxes.
> > +
> > +A device tree node for an Arm MHUv2 device must specify either a receiver frame
> > +or a sender frame, indicating which end of the unidirectional MHU device which
> > +the device node entry describes.
> > +
> > +An MHU device must be specified with a transport protocol. The transport
> > +protocol of an MHU device determines the method of data transmission as well as
> > +the number of provided mailboxes.
> > +Following are the possible transport protocol types:
> > +- Single-word: An MHU device implements as many mailboxes as it
> > +               provides channel windows. Data is transmitted through
> > +               the MHU registers.
> > +- Multi-word:  An MHU device implements a single mailbox. All channel windows
> > +               will be used during transmission. Data is transmitted through
> > +               the MHU registers.
> > +- Doorbell:    An MHU device implements as many mailboxes as there are flag
> > +               bits available in its channel windows. Optionally, data may
> > +               be transmitted through a shared memory region, wherein the MHU
> > +               is used strictly as an interrupt generation mechanism.
> > +
> > +Mailbox Device Node:
> > +====================
> > +
> > +Required properties:
> > +--------------------
> > +- compatible:  Shall be "arm,mhuv2" & "arm,primecell"
> > +- reg:         Contains the mailbox register address range (base
> > +               address and length)
> > +- #mbox-cells  Shall be 1 - the index of the channel needed.
> > +- mhu-frame    Frame type of the device.
> > +               Shall be either "sender" or "receiver"
> > +- mhu-protocol Transport protocol of the device. Shall be one of the
> > +               following: "single-word", "multi-word", "doorbell"
> > +
> > +Required properties (receiver frame):
> > +-------------------------------------
> > +- interrupts:  Contains the interrupt information corresponding to the
> > +               combined interrupt of the receiver frame
> > +
> > +Example:
> > +--------
> > +
> > +       mbox_mw_tx: mhu@10000000 {
> > +               compatible = "arm,mhuv2","arm,primecell";
> > +               reg = <0x10000000 0x1000>;
> > +               clocks = <&refclk100mhz>;
> > +               clock-names = "apb_pclk";
> > +               #mbox-cells = <1>;
> > +               mhu-protocol = "multi-word";
> > +               mhu-frame = "sender";
> > +       };
> > +
> > +       mbox_sw_tx: mhu@10000000 {
> > +               compatible = "arm,mhuv2","arm,primecell";
> > +               reg = <0x11000000 0x1000>;
> > +               clocks = <&refclk100mhz>;
> > +               clock-names = "apb_pclk";
> > +               #mbox-cells = <1>;
> > +               mhu-protocol = "single-word";
> > +               mhu-frame = "sender";
> > +       };
> > +
> > +       mbox_db_rx: mhu@10000000 {
> > +               compatible = "arm,mhuv2","arm,primecell";
> > +               reg = <0x12000000 0x1000>;
> > +               clocks = <&refclk100mhz>;
> > +               clock-names = "apb_pclk";
> > +               #mbox-cells = <1>;
> > +               interrupts = <0 45 4>;
> > +               interrupt-names = "mhu_rx";
> > +               mhu-protocol = "doorbell";
> > +               mhu-frame = "receiver";
> > +       };
> > +
> > +       mhu_client: scb@2e000000 {
> > +       compatible = "fujitsu,mb86s70-scb-1.0";
> > +       reg = <0 0x2e000000 0x4000>;
> > +       mboxes =
> > +               // For multi-word frames, client may only instantiate a single
> > +               // mailbox for a mailbox controller
> > +               <&mbox_mw_tx 0>,
> > +
> > +               // For single-word frames, client may instantiate as many
> > +               // mailboxes as there are channel windows in the MHU
> > +                <&mbox_sw_tx 0>,
> > +                <&mbox_sw_tx 1>,
> > +                <&mbox_sw_tx 2>,
> > +                <&mbox_sw_tx 3>,
> > +
> > +               // For doorbell frames, client may instantiate as many mailboxes
> > +               // as there are bits available in the combined number of channel
> > +               // windows ((channel windows * 32) mailboxes)
> > +                <mbox_db_rx 0>,
> > +                <mbox_db_rx 1>,
> > +                ...
> > +                <mbox_db_rx 17>;
> > +       };
>
> If the mhuv2 instance implements, say, 3 channel windows between
> sender (linux) and receiver (firmware), and Linux runs two protocols
> each requiring 1 and 2-word sized messages respectively. The hardware
> supports that by assigning windows [0] and [1,2] to each protocol.
> However, I don't think the driver can support that. Or does it?
>
Thinking about it, IMO, the mbox-cell should carry a 128 (4x32) bit
mask specifying the set of windows (corresponding to the bits set in
the mask) associated with the channel.
And the controller driver should see any channel as associated with
variable number of windows 'N', where N is [0,124]

mhu_client1: proto1@2e000000 {
       .....
       mboxes = <&mbox 0x0 0x0 0x0 0x1>
}

mhu_client2: proto2@2f000000 {
       .....
       mboxes = <&mbox 0x0 0x0 0x0 0x6>
}

Cheers!

On 7/21/19 11:58 PM, Jassi Brar wrote:
> On Wed, Jul 17, 2019 at 2:26 PM Tushar Khandelwal
> <tushar.khandelwal@arm.com> wrote:
> 
>> diff --git a/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
>> new file mode 100644
>> index 000000000000..3a05593414bc
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
>> @@ -0,0 +1,108 @@
>> +Arm MHUv2 Mailbox Driver
>> +========================
>> +
>> +The Arm Message-Handling-Unit (MHU) Version 2 is a mailbox controller that has
>> +between 1 and 124 channel windows to provide unidirectional communication with
>> +remote processor(s).
>> +
>> +Given the unidirectional nature of the device, an MHUv2 mailbox may only be
>> +written to or read from. If a pair of MHU devices is implemented between two
>> +processing elements to provide bidirectional communication, these must be
>> +specified as two separate mailboxes.
>> +
>> +A device tree node for an Arm MHUv2 device must specify either a receiver frame
>> +or a sender frame, indicating which end of the unidirectional MHU device which
>> +the device node entry describes.
>> +
>> +An MHU device must be specified with a transport protocol. The transport
>> +protocol of an MHU device determines the method of data transmission as well as
>> +the number of provided mailboxes.
>> +Following are the possible transport protocol types:
>> +- Single-word: An MHU device implements as many mailboxes as it
>> +               provides channel windows. Data is transmitted through
>> +               the MHU registers.
>> +- Multi-word:  An MHU device implements a single mailbox. All channel windows
>> +               will be used during transmission. Data is transmitted through
>> +               the MHU registers.
>> +- Doorbell:    An MHU device implements as many mailboxes as there are flag
>> +               bits available in its channel windows. Optionally, data may
>> +               be transmitted through a shared memory region, wherein the MHU
>> +               is used strictly as an interrupt generation mechanism.
>> +
>> +Mailbox Device Node:
>> +====================
>> +
>> +Required properties:
>> +--------------------
>> +- compatible:  Shall be "arm,mhuv2" & "arm,primecell"
>> +- reg:         Contains the mailbox register address range (base
>> +               address and length)
>> +- #mbox-cells  Shall be 1 - the index of the channel needed.
>> +- mhu-frame    Frame type of the device.
>> +               Shall be either "sender" or "receiver"
>> +- mhu-protocol Transport protocol of the device. Shall be one of the
>> +               following: "single-word", "multi-word", "doorbell"
>> +
>> +Required properties (receiver frame):
>> +-------------------------------------
>> +- interrupts:  Contains the interrupt information corresponding to the
>> +               combined interrupt of the receiver frame
>> +
>> +Example:
>> +--------
>> +
>> +       mbox_mw_tx: mhu@10000000 {
>> +               compatible = "arm,mhuv2","arm,primecell";
>> +               reg = <0x10000000 0x1000>;
>> +               clocks = <&refclk100mhz>;
>> +               clock-names = "apb_pclk";
>> +               #mbox-cells = <1>;
>> +               mhu-protocol = "multi-word";
>> +               mhu-frame = "sender";
>> +       };
>> +
>> +       mbox_sw_tx: mhu@10000000 {
>> +               compatible = "arm,mhuv2","arm,primecell";
>> +               reg = <0x11000000 0x1000>;
>> +               clocks = <&refclk100mhz>;
>> +               clock-names = "apb_pclk";
>> +               #mbox-cells = <1>;
>> +               mhu-protocol = "single-word";
>> +               mhu-frame = "sender";
>> +       };
>> +
>> +       mbox_db_rx: mhu@10000000 {
>> +               compatible = "arm,mhuv2","arm,primecell";
>> +               reg = <0x12000000 0x1000>;
>> +               clocks = <&refclk100mhz>;
>> +               clock-names = "apb_pclk";
>> +               #mbox-cells = <1>;
>> +               interrupts = <0 45 4>;
>> +               interrupt-names = "mhu_rx";
>> +               mhu-protocol = "doorbell";
>> +               mhu-frame = "receiver";
>> +       };
>> +
>> +       mhu_client: scb@2e000000 {
>> +       compatible = "fujitsu,mb86s70-scb-1.0";
>> +       reg = <0 0x2e000000 0x4000>;
>> +       mboxes =
>> +               // For multi-word frames, client may only instantiate a single
>> +               // mailbox for a mailbox controller
>> +               <&mbox_mw_tx 0>,
>> +
>> +               // For single-word frames, client may instantiate as many
>> +               // mailboxes as there are channel windows in the MHU
>> +                <&mbox_sw_tx 0>,
>> +                <&mbox_sw_tx 1>,
>> +                <&mbox_sw_tx 2>,
>> +                <&mbox_sw_tx 3>,
>> +
>> +               // For doorbell frames, client may instantiate as many mailboxes
>> +               // as there are bits available in the combined number of channel
>> +               // windows ((channel windows * 32) mailboxes)
>> +                <mbox_db_rx 0>,
>> +                <mbox_db_rx 1>,
>> +                ...
>> +                <mbox_db_rx 17>;
>> +       };
> 
> If the mhuv2 instance implements, say, 3 channel windows between
> sender (linux) and receiver (firmware), and Linux runs two protocols
> each requiring 1 and 2-word sized messages respectively. The hardware
> supports that by assigning windows [0] and [1,2] to each protocol.
> However, I don't think the driver can support that. Or does it?
> 

Correct, this version of the driver does not support mixing-and-matching
protocols for an MHU device.
Given the current use-cases for the driver, we do not currently see a
need for this functionality. However, as you mention, the hardware does
not restrict this and it would be possible to add in a future version.

> Also I see problem with implementing "protocol modes" in the
> controller driver - 'mhu-protocol' property should go away.  And
> 'mhu-frame' is unncessary - presence of interrupt property could imply
> 'receiver', otherwise 'sender'.
> 
> Cheers!
> 

I agree that the 'mhu-frame' property can be removed and the frame type
can be deduced from whether an interrupt property is present.
In regards to 'mhu-protocol', i still see value in having it as a device
tree property. As mentioned above, mixing protocols within an MHU is not
currently supported. We decided to specify the protocol for an MHU in
the device tree, given that the protocol influences how many mboxes it
is allowed for a mailbox client to register with a controller,

Thanks,
Morten

On 7/25/19 7:49 AM, Jassi Brar wrote:
> On Sun, Jul 21, 2019 at 4:58 PM Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>
>> On Wed, Jul 17, 2019 at 2:26 PM Tushar Khandelwal
>> <tushar.khandelwal@arm.com> wrote:
>>
>>> diff --git a/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
>>> new file mode 100644
>>> index 000000000000..3a05593414bc
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
>>> @@ -0,0 +1,108 @@
>>> +Arm MHUv2 Mailbox Driver
>>> +========================
>>> +
>>> +The Arm Message-Handling-Unit (MHU) Version 2 is a mailbox controller that has
>>> +between 1 and 124 channel windows to provide unidirectional communication with
>>> +remote processor(s).
>>> +
>>> +Given the unidirectional nature of the device, an MHUv2 mailbox may only be
>>> +written to or read from. If a pair of MHU devices is implemented between two
>>> +processing elements to provide bidirectional communication, these must be
>>> +specified as two separate mailboxes.
>>> +
>>> +A device tree node for an Arm MHUv2 device must specify either a receiver frame
>>> +or a sender frame, indicating which end of the unidirectional MHU device which
>>> +the device node entry describes.
>>> +
>>> +An MHU device must be specified with a transport protocol. The transport
>>> +protocol of an MHU device determines the method of data transmission as well as
>>> +the number of provided mailboxes.
>>> +Following are the possible transport protocol types:
>>> +- Single-word: An MHU device implements as many mailboxes as it
>>> +               provides channel windows. Data is transmitted through
>>> +               the MHU registers.
>>> +- Multi-word:  An MHU device implements a single mailbox. All channel windows
>>> +               will be used during transmission. Data is transmitted through
>>> +               the MHU registers.
>>> +- Doorbell:    An MHU device implements as many mailboxes as there are flag
>>> +               bits available in its channel windows. Optionally, data may
>>> +               be transmitted through a shared memory region, wherein the MHU
>>> +               is used strictly as an interrupt generation mechanism.
>>> +
>>> +Mailbox Device Node:
>>> +====================
>>> +
>>> +Required properties:
>>> +--------------------
>>> +- compatible:  Shall be "arm,mhuv2" & "arm,primecell"
>>> +- reg:         Contains the mailbox register address range (base
>>> +               address and length)
>>> +- #mbox-cells  Shall be 1 - the index of the channel needed.
>>> +- mhu-frame    Frame type of the device.
>>> +               Shall be either "sender" or "receiver"
>>> +- mhu-protocol Transport protocol of the device. Shall be one of the
>>> +               following: "single-word", "multi-word", "doorbell"
>>> +
>>> +Required properties (receiver frame):
>>> +-------------------------------------
>>> +- interrupts:  Contains the interrupt information corresponding to the
>>> +               combined interrupt of the receiver frame
>>> +
>>> +Example:
>>> +--------
>>> +
>>> +       mbox_mw_tx: mhu@10000000 {
>>> +               compatible = "arm,mhuv2","arm,primecell";
>>> +               reg = <0x10000000 0x1000>;
>>> +               clocks = <&refclk100mhz>;
>>> +               clock-names = "apb_pclk";
>>> +               #mbox-cells = <1>;
>>> +               mhu-protocol = "multi-word";
>>> +               mhu-frame = "sender";
>>> +       };
>>> +
>>> +       mbox_sw_tx: mhu@10000000 {
>>> +               compatible = "arm,mhuv2","arm,primecell";
>>> +               reg = <0x11000000 0x1000>;
>>> +               clocks = <&refclk100mhz>;
>>> +               clock-names = "apb_pclk";
>>> +               #mbox-cells = <1>;
>>> +               mhu-protocol = "single-word";
>>> +               mhu-frame = "sender";
>>> +       };
>>> +
>>> +       mbox_db_rx: mhu@10000000 {
>>> +               compatible = "arm,mhuv2","arm,primecell";
>>> +               reg = <0x12000000 0x1000>;
>>> +               clocks = <&refclk100mhz>;
>>> +               clock-names = "apb_pclk";
>>> +               #mbox-cells = <1>;
>>> +               interrupts = <0 45 4>;
>>> +               interrupt-names = "mhu_rx";
>>> +               mhu-protocol = "doorbell";
>>> +               mhu-frame = "receiver";
>>> +       };
>>> +
>>> +       mhu_client: scb@2e000000 {
>>> +       compatible = "fujitsu,mb86s70-scb-1.0";
>>> +       reg = <0 0x2e000000 0x4000>;
>>> +       mboxes =
>>> +               // For multi-word frames, client may only instantiate a single
>>> +               // mailbox for a mailbox controller
>>> +               <&mbox_mw_tx 0>,
>>> +
>>> +               // For single-word frames, client may instantiate as many
>>> +               // mailboxes as there are channel windows in the MHU
>>> +                <&mbox_sw_tx 0>,
>>> +                <&mbox_sw_tx 1>,
>>> +                <&mbox_sw_tx 2>,
>>> +                <&mbox_sw_tx 3>,
>>> +
>>> +               // For doorbell frames, client may instantiate as many mailboxes
>>> +               // as there are bits available in the combined number of channel
>>> +               // windows ((channel windows * 32) mailboxes)
>>> +                <mbox_db_rx 0>,
>>> +                <mbox_db_rx 1>,
>>> +                ...
>>> +                <mbox_db_rx 17>;
>>> +       };
>>
>> If the mhuv2 instance implements, say, 3 channel windows between
>> sender (linux) and receiver (firmware), and Linux runs two protocols
>> each requiring 1 and 2-word sized messages respectively. The hardware
>> supports that by assigning windows [0] and [1,2] to each protocol.
>> However, I don't think the driver can support that. Or does it?
>>
> Thinking about it, IMO, the mbox-cell should carry a 128 (4x32) bit
> mask specifying the set of windows (corresponding to the bits set in
> the mask) associated with the channel.
> And the controller driver should see any channel as associated with
> variable number of windows 'N', where N is [0,124]
> 
> mhu_client1: proto1@2e000000 {
>        .....
>        mboxes = <&mbox 0x0 0x0 0x0 0x1>
> }
> 
> mhu_client2: proto2@2f000000 {
>        .....
>        mboxes = <&mbox 0x0 0x0 0x0 0x6>
> }
> 
> Cheers!
> 

As mentioned in the response to your initial comment, the driver does
not currently support mixing protocols.
If mixing protocols is to be supported in the future, then this seems
like a suitable way of specifying which channels are associated with
which mailboxes (especially for mixing single- and multi-word modes).
However, there still is an issue in that both single-word and doorbell
requires only 1 channel window - and as such, the transport protocol
cannot be deduced from merely the number of masked channel windows.
Furthermore, for doorbell, a mbox may be registered for _each_ available
bit within a channel window (further complicating things if we were to
include mixing protocols in this initial driver version), making
assigning channel windows to mailboxes semantically different from when
assigning to single- or multi-word.

Thanks,
Morten

On Sun, Jul 28, 2019 at 4:28 PM Morten Borup Petersen <morten_bp@live.dk> wrote:
>
>
>
> On 7/25/19 7:49 AM, Jassi Brar wrote:
> > On Sun, Jul 21, 2019 at 4:58 PM Jassi Brar <jassisinghbrar@gmail.com> wrote:
> >>
> >> On Wed, Jul 17, 2019 at 2:26 PM Tushar Khandelwal
> >> <tushar.khandelwal@arm.com> wrote:
> >>
> >>> diff --git a/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
> >>> new file mode 100644
> >>> index 000000000000..3a05593414bc
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
> >>> @@ -0,0 +1,108 @@
> >>> +Arm MHUv2 Mailbox Driver
> >>> +========================
> >>> +
> >>> +The Arm Message-Handling-Unit (MHU) Version 2 is a mailbox controller that has
> >>> +between 1 and 124 channel windows to provide unidirectional communication with
> >>> +remote processor(s).
> >>> +
> >>> +Given the unidirectional nature of the device, an MHUv2 mailbox may only be
> >>> +written to or read from. If a pair of MHU devices is implemented between two
> >>> +processing elements to provide bidirectional communication, these must be
> >>> +specified as two separate mailboxes.
> >>> +
> >>> +A device tree node for an Arm MHUv2 device must specify either a receiver frame
> >>> +or a sender frame, indicating which end of the unidirectional MHU device which
> >>> +the device node entry describes.
> >>> +
> >>> +An MHU device must be specified with a transport protocol. The transport
> >>> +protocol of an MHU device determines the method of data transmission as well as
> >>> +the number of provided mailboxes.
> >>> +Following are the possible transport protocol types:
> >>> +- Single-word: An MHU device implements as many mailboxes as it
> >>> +               provides channel windows. Data is transmitted through
> >>> +               the MHU registers.
> >>> +- Multi-word:  An MHU device implements a single mailbox. All channel windows
> >>> +               will be used during transmission. Data is transmitted through
> >>> +               the MHU registers.
> >>> +- Doorbell:    An MHU device implements as many mailboxes as there are flag
> >>> +               bits available in its channel windows. Optionally, data may
> >>> +               be transmitted through a shared memory region, wherein the MHU
> >>> +               is used strictly as an interrupt generation mechanism.
> >>> +
> >>> +Mailbox Device Node:
> >>> +====================
> >>> +
> >>> +Required properties:
> >>> +--------------------
> >>> +- compatible:  Shall be "arm,mhuv2" & "arm,primecell"
> >>> +- reg:         Contains the mailbox register address range (base
> >>> +               address and length)
> >>> +- #mbox-cells  Shall be 1 - the index of the channel needed.
> >>> +- mhu-frame    Frame type of the device.
> >>> +               Shall be either "sender" or "receiver"
> >>> +- mhu-protocol Transport protocol of the device. Shall be one of the
> >>> +               following: "single-word", "multi-word", "doorbell"
> >>> +
> >>> +Required properties (receiver frame):
> >>> +-------------------------------------
> >>> +- interrupts:  Contains the interrupt information corresponding to the
> >>> +               combined interrupt of the receiver frame
> >>> +
> >>> +Example:
> >>> +--------
> >>> +
> >>> +       mbox_mw_tx: mhu@10000000 {
> >>> +               compatible = "arm,mhuv2","arm,primecell";
> >>> +               reg = <0x10000000 0x1000>;
> >>> +               clocks = <&refclk100mhz>;
> >>> +               clock-names = "apb_pclk";
> >>> +               #mbox-cells = <1>;
> >>> +               mhu-protocol = "multi-word";
> >>> +               mhu-frame = "sender";
> >>> +       };
> >>> +
> >>> +       mbox_sw_tx: mhu@10000000 {
> >>> +               compatible = "arm,mhuv2","arm,primecell";
> >>> +               reg = <0x11000000 0x1000>;
> >>> +               clocks = <&refclk100mhz>;
> >>> +               clock-names = "apb_pclk";
> >>> +               #mbox-cells = <1>;
> >>> +               mhu-protocol = "single-word";
> >>> +               mhu-frame = "sender";
> >>> +       };
> >>> +
> >>> +       mbox_db_rx: mhu@10000000 {
> >>> +               compatible = "arm,mhuv2","arm,primecell";
> >>> +               reg = <0x12000000 0x1000>;
> >>> +               clocks = <&refclk100mhz>;
> >>> +               clock-names = "apb_pclk";
> >>> +               #mbox-cells = <1>;
> >>> +               interrupts = <0 45 4>;
> >>> +               interrupt-names = "mhu_rx";
> >>> +               mhu-protocol = "doorbell";
> >>> +               mhu-frame = "receiver";
> >>> +       };
> >>> +
> >>> +       mhu_client: scb@2e000000 {
> >>> +       compatible = "fujitsu,mb86s70-scb-1.0";
> >>> +       reg = <0 0x2e000000 0x4000>;
> >>> +       mboxes =
> >>> +               // For multi-word frames, client may only instantiate a single
> >>> +               // mailbox for a mailbox controller
> >>> +               <&mbox_mw_tx 0>,
> >>> +
> >>> +               // For single-word frames, client may instantiate as many
> >>> +               // mailboxes as there are channel windows in the MHU
> >>> +                <&mbox_sw_tx 0>,
> >>> +                <&mbox_sw_tx 1>,
> >>> +                <&mbox_sw_tx 2>,
> >>> +                <&mbox_sw_tx 3>,
> >>> +
> >>> +               // For doorbell frames, client may instantiate as many mailboxes
> >>> +               // as there are bits available in the combined number of channel
> >>> +               // windows ((channel windows * 32) mailboxes)
> >>> +                <mbox_db_rx 0>,
> >>> +                <mbox_db_rx 1>,
> >>> +                ...
> >>> +                <mbox_db_rx 17>;
> >>> +       };
> >>
> >> If the mhuv2 instance implements, say, 3 channel windows between
> >> sender (linux) and receiver (firmware), and Linux runs two protocols
> >> each requiring 1 and 2-word sized messages respectively. The hardware
> >> supports that by assigning windows [0] and [1,2] to each protocol.
> >> However, I don't think the driver can support that. Or does it?
> >>
> > Thinking about it, IMO, the mbox-cell should carry a 128 (4x32) bit
> > mask specifying the set of windows (corresponding to the bits set in
> > the mask) associated with the channel.
> > And the controller driver should see any channel as associated with
> > variable number of windows 'N', where N is [0,124]
> >
> > mhu_client1: proto1@2e000000 {
> >        .....
> >        mboxes = <&mbox 0x0 0x0 0x0 0x1>
> > }
> >
> > mhu_client2: proto2@2f000000 {
> >        .....
> >        mboxes = <&mbox 0x0 0x0 0x0 0x6>
> > }
> >
> > Cheers!
> >
>
> As mentioned in the response to your initial comment, the driver does
> not currently support mixing protocols.
>
Thanks for acknowledging that limitation. But lets also address it.

> If mixing protocols is to be supported in the future, then this seems
> like a suitable way of specifying which channels are associated with
> which mailboxes (especially for mixing single- and multi-word modes).
>
We can not change DT bindings again when we feel like updating the driver.
The bindings should precisely and completely define the h/w, not what
mode we currently implement.
It is not for pure idealism, it actually makes the code simpler and futureproof.

> However, there still is an issue in that both single-word and doorbell
> requires only 1 channel window - and as such, the transport protocol
> cannot be deduced from merely the number of masked channel windows.
>
I don't see why the driver should worry -- the channel carries 32-bit
message or some random value just to trigger an interrupt is purely
upto the client driver.

> Furthermore, for doorbell, a mbox may be registered for _each_ available
> bit within a channel window (further complicating things if we were to
> include mixing protocols in this initial driver version), making
> assigning channel windows to mailboxes semantically different from when
> assigning to single- or multi-word.
>
Not sure about that, that would be implementing virtual channels. Each
window carries one signal, and that is the minimum bandwidth assigned
to a channel.

Thanks

On 7/31/19 9:31 AM, Jassi Brar wrote:
> On Sun, Jul 28, 2019 at 4:28 PM Morten Borup Petersen <morten_bp@live.dk> wrote:
>>
>>
>>
>> On 7/25/19 7:49 AM, Jassi Brar wrote:
>>> On Sun, Jul 21, 2019 at 4:58 PM Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>>>
>>>> On Wed, Jul 17, 2019 at 2:26 PM Tushar Khandelwal
>>>> <tushar.khandelwal@arm.com> wrote:
>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
>>>>> new file mode 100644
>>>>> index 000000000000..3a05593414bc
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
>>>>> @@ -0,0 +1,108 @@
>>>>> +Arm MHUv2 Mailbox Driver
>>>>> +========================
>>>>> +
>>>>> +The Arm Message-Handling-Unit (MHU) Version 2 is a mailbox controller that has
>>>>> +between 1 and 124 channel windows to provide unidirectional communication with
>>>>> +remote processor(s).
>>>>> +
>>>>> +Given the unidirectional nature of the device, an MHUv2 mailbox may only be
>>>>> +written to or read from. If a pair of MHU devices is implemented between two
>>>>> +processing elements to provide bidirectional communication, these must be
>>>>> +specified as two separate mailboxes.
>>>>> +
>>>>> +A device tree node for an Arm MHUv2 device must specify either a receiver frame
>>>>> +or a sender frame, indicating which end of the unidirectional MHU device which
>>>>> +the device node entry describes.
>>>>> +
>>>>> +An MHU device must be specified with a transport protocol. The transport
>>>>> +protocol of an MHU device determines the method of data transmission as well as
>>>>> +the number of provided mailboxes.
>>>>> +Following are the possible transport protocol types:
>>>>> +- Single-word: An MHU device implements as many mailboxes as it
>>>>> +               provides channel windows. Data is transmitted through
>>>>> +               the MHU registers.
>>>>> +- Multi-word:  An MHU device implements a single mailbox. All channel windows
>>>>> +               will be used during transmission. Data is transmitted through
>>>>> +               the MHU registers.
>>>>> +- Doorbell:    An MHU device implements as many mailboxes as there are flag
>>>>> +               bits available in its channel windows. Optionally, data may
>>>>> +               be transmitted through a shared memory region, wherein the MHU
>>>>> +               is used strictly as an interrupt generation mechanism.
>>>>> +
>>>>> +Mailbox Device Node:
>>>>> +====================
>>>>> +
>>>>> +Required properties:
>>>>> +--------------------
>>>>> +- compatible:  Shall be "arm,mhuv2" & "arm,primecell"
>>>>> +- reg:         Contains the mailbox register address range (base
>>>>> +               address and length)
>>>>> +- #mbox-cells  Shall be 1 - the index of the channel needed.
>>>>> +- mhu-frame    Frame type of the device.
>>>>> +               Shall be either "sender" or "receiver"
>>>>> +- mhu-protocol Transport protocol of the device. Shall be one of the
>>>>> +               following: "single-word", "multi-word", "doorbell"
>>>>> +
>>>>> +Required properties (receiver frame):
>>>>> +-------------------------------------
>>>>> +- interrupts:  Contains the interrupt information corresponding to the
>>>>> +               combined interrupt of the receiver frame
>>>>> +
>>>>> +Example:
>>>>> +--------
>>>>> +
>>>>> +       mbox_mw_tx: mhu@10000000 {
>>>>> +               compatible = "arm,mhuv2","arm,primecell";
>>>>> +               reg = <0x10000000 0x1000>;
>>>>> +               clocks = <&refclk100mhz>;
>>>>> +               clock-names = "apb_pclk";
>>>>> +               #mbox-cells = <1>;
>>>>> +               mhu-protocol = "multi-word";
>>>>> +               mhu-frame = "sender";
>>>>> +       };
>>>>> +
>>>>> +       mbox_sw_tx: mhu@10000000 {
>>>>> +               compatible = "arm,mhuv2","arm,primecell";
>>>>> +               reg = <0x11000000 0x1000>;
>>>>> +               clocks = <&refclk100mhz>;
>>>>> +               clock-names = "apb_pclk";
>>>>> +               #mbox-cells = <1>;
>>>>> +               mhu-protocol = "single-word";
>>>>> +               mhu-frame = "sender";
>>>>> +       };
>>>>> +
>>>>> +       mbox_db_rx: mhu@10000000 {
>>>>> +               compatible = "arm,mhuv2","arm,primecell";
>>>>> +               reg = <0x12000000 0x1000>;
>>>>> +               clocks = <&refclk100mhz>;
>>>>> +               clock-names = "apb_pclk";
>>>>> +               #mbox-cells = <1>;
>>>>> +               interrupts = <0 45 4>;
>>>>> +               interrupt-names = "mhu_rx";
>>>>> +               mhu-protocol = "doorbell";
>>>>> +               mhu-frame = "receiver";
>>>>> +       };
>>>>> +
>>>>> +       mhu_client: scb@2e000000 {
>>>>> +       compatible = "fujitsu,mb86s70-scb-1.0";
>>>>> +       reg = <0 0x2e000000 0x4000>;
>>>>> +       mboxes =
>>>>> +               // For multi-word frames, client may only instantiate a single
>>>>> +               // mailbox for a mailbox controller
>>>>> +               <&mbox_mw_tx 0>,
>>>>> +
>>>>> +               // For single-word frames, client may instantiate as many
>>>>> +               // mailboxes as there are channel windows in the MHU
>>>>> +                <&mbox_sw_tx 0>,
>>>>> +                <&mbox_sw_tx 1>,
>>>>> +                <&mbox_sw_tx 2>,
>>>>> +                <&mbox_sw_tx 3>,
>>>>> +
>>>>> +               // For doorbell frames, client may instantiate as many mailboxes
>>>>> +               // as there are bits available in the combined number of channel
>>>>> +               // windows ((channel windows * 32) mailboxes)
>>>>> +                <mbox_db_rx 0>,
>>>>> +                <mbox_db_rx 1>,
>>>>> +                ...
>>>>> +                <mbox_db_rx 17>;
>>>>> +       };
>>>>
>>>> If the mhuv2 instance implements, say, 3 channel windows between
>>>> sender (linux) and receiver (firmware), and Linux runs two protocols
>>>> each requiring 1 and 2-word sized messages respectively. The hardware
>>>> supports that by assigning windows [0] and [1,2] to each protocol.
>>>> However, I don't think the driver can support that. Or does it?
>>>>
>>> Thinking about it, IMO, the mbox-cell should carry a 128 (4x32) bit
>>> mask specifying the set of windows (corresponding to the bits set in
>>> the mask) associated with the channel.
>>> And the controller driver should see any channel as associated with
>>> variable number of windows 'N', where N is [0,124]
>>>
>>> mhu_client1: proto1@2e000000 {
>>>        .....
>>>        mboxes = <&mbox 0x0 0x0 0x0 0x1>
>>> }
>>>
>>> mhu_client2: proto2@2f000000 {
>>>        .....
>>>        mboxes = <&mbox 0x0 0x0 0x0 0x6>
>>> }
>>>
>>> Cheers!
>>>
>>
>> As mentioned in the response to your initial comment, the driver does
>> not currently support mixing protocols.
>>
> Thanks for acknowledging that limitation. But lets also address it.
> 

We are hesitant to dedicate time to developing mixing protocols given
that we don't have any current usecase nor any current platform which
would support this.

>> If mixing protocols is to be supported in the future, then this seems
>> like a suitable way of specifying which channels are associated with
>> which mailboxes (especially for mixing single- and multi-word modes).
>>
> We can not change DT bindings again when we feel like updating the driver.
> The bindings should precisely and completely define the h/w, not what
> mode we currently implement.
> It is not for pure idealism, it actually makes the code simpler and futureproof.
> 
>> However, there still is an issue in that both single-word and doorbell
>> requires only 1 channel window - and as such, the transport protocol
>> cannot be deduced from merely the number of masked channel windows.
>>
> I don't see why the driver should worry -- the channel carries 32-bit
> message or some random value just to trigger an interrupt is purely
> upto the client driver.
> 

With the current design, it is not up to the client driver to
distinguish which bit was set within a channel window when an interrupt
was raised in doorbell mode. Currently, in doorbell mode, each bit
within a channel isspecified to be a distinct mailbox. Having this,
different mailbox clients may register mailboxes for different bits
within a single MHU device.
with the current design, when an interrupt is raised and an MHU is in
doorbell mode,  the MHU driver will be responsible for deducing which
flag bit was set and from this set bit decide which mailbox to trigger a
callback on.
This is why we need to be able to specify the bit number when in
doorbell mode, in the device tree.

>> Furthermore, for doorbell, a mbox may be registered for _each_ available
>> bit within a channel window (further complicating things if we were to
>> include mixing protocols in this initial driver version), making
>> assigning channel windows to mailboxes semantically different from when
>> assigning to single- or multi-word.
>>
> Not sure about that, that would be implementing virtual channels. Each
> window carries one signal, and that is the minimum bandwidth assigned
> to a channel.
> 
> Thanks
>
If implementing transport protocol mixing would be a requirement for the
acceptance of the driver, then we agree on that the format which you've
suggested would be a clean solution. However, given that we would like
to keep the ability to specify doorbell mailboxes in the device tree, we
suggest a format such as the following:

mhu_client1: proto2@2f000000 {
       .....
       /* Requesting to use channel window 0 of &mbox, registering
          a mailbox in singe-word mode. */
       mboxes = <&mbox 0x0 0x0 0x0 0x1>
}


mhu_client2: proto1@2e000000 {
        .....
	/* Requesting to use channel window 1 of &mbox, registering
           mailboxes in doorbell mode, using bits 3 and 5. The MHU
           driver is able to discern between putting channel window 1
           into doorbell mode over single word mode, given the
           presence of the extra argument. */
        mboxes = <&mbox 0x0 0x0 0x0 0x2 3>,
                 <&mbox 0x0 0x0 0x0 0x2 5>
}

This would remove the ambiguity around deducing a mailbox to be in
single-word or doorbell mode.
Deciding to put channel window(s) into multi-word mode would be, as you
proposed, to mask more than one channel for a mailbox, ie:

mhu_client3: proto2@2f000000 {
       .....
       /* Requesting to use channel window 2-3 of &mbox, registering
          a mailbox in multi-word mode. */
       mboxes = <&mbox 0x0 0x0 0x0 0xC>
}


Thanks,

Morten

On Thu, Jul 25, 2019 at 12:49:58AM -0500, Jassi Brar wrote:
> On Sun, Jul 21, 2019 at 4:58 PM Jassi Brar <jassisinghbrar@gmail.com> wrote:
> >

[...]

> > If the mhuv2 instance implements, say, 3 channel windows between
> > sender (linux) and receiver (firmware), and Linux runs two protocols
> > each requiring 1 and 2-word sized messages respectively. The hardware
> > supports that by assigning windows [0] and [1,2] to each protocol.
> > However, I don't think the driver can support that. Or does it?
> >
> Thinking about it, IMO, the mbox-cell should carry a 128 (4x32) bit
> mask specifying the set of windows (corresponding to the bits set in
> the mask) associated with the channel.
> And the controller driver should see any channel as associated with
> variable number of windows 'N', where N is [0,124]
>
> mhu_client1: proto1@2e000000 {
>        .....
>        mboxes = <&mbox 0x0 0x0 0x0 0x1>
> }
>
> mhu_client2: proto2@2f000000 {
>        .....
>        mboxes = <&mbox 0x0 0x0 0x0 0x6>
> }
>

This still doesn't address the overhead I mentioned in my arm_mhu_v1
series.

As per you suggestion, we will have one channel with all possible
bit mask value to specify the window. Let's imagine that 2 protocols
share the same channel, then the requests are serialised.
E.g. if bits 0 and 1 are allocated for say protocol#1 and bits 2 and 3
for protocol#2.

Further protocol#1 has higher latency requirements like sched-governor
DVFS and there are 3-4 pending requests on protocol#2, then the incoming
requests for protocol#1 is blocked.

This is definitely overhead and I have seen lots of issue around this
and hence I was requesting that we need to create individual channels
for each of these. Having abstraction on top to multiplex or arbitrate
won't help.

--
Regards,
Sudeep

On Sun, Jul 21, 2019 at 04:58:04PM -0500, Jassi Brar wrote:
> On Wed, Jul 17, 2019 at 2:26 PM Tushar Khandelwal
> <tushar.khandelwal@arm.com> wrote:
>
> > diff --git a/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
> > new file mode 100644
> > index 000000000000..3a05593414bc
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
> > @@ -0,0 +1,108 @@
> > +Arm MHUv2 Mailbox Driver
> > +========================
> > +
> > +The Arm Message-Handling-Unit (MHU) Version 2 is a mailbox controller that has
> > +between 1 and 124 channel windows to provide unidirectional communication with
> > +remote processor(s).
> > +
> > +Given the unidirectional nature of the device, an MHUv2 mailbox may only be
> > +written to or read from. If a pair of MHU devices is implemented between two
> > +processing elements to provide bidirectional communication, these must be
> > +specified as two separate mailboxes.
> > +
> > +A device tree node for an Arm MHUv2 device must specify either a receiver frame
> > +or a sender frame, indicating which end of the unidirectional MHU device which
> > +the device node entry describes.
> > +
> > +An MHU device must be specified with a transport protocol. The transport
> > +protocol of an MHU device determines the method of data transmission as well as
> > +the number of provided mailboxes.
> > +Following are the possible transport protocol types:
> > +- Single-word: An MHU device implements as many mailboxes as it
> > +               provides channel windows. Data is transmitted through
> > +               the MHU registers.
> > +- Multi-word:  An MHU device implements a single mailbox. All channel windows
> > +               will be used during transmission. Data is transmitted through
> > +               the MHU registers.
> > +- Doorbell:    An MHU device implements as many mailboxes as there are flag
> > +               bits available in its channel windows. Optionally, data may
> > +               be transmitted through a shared memory region, wherein the MHU
> > +               is used strictly as an interrupt generation mechanism.
> > +
> > +Mailbox Device Node:
> > +====================
> > +
> > +Required properties:
> > +--------------------
> > +- compatible:  Shall be "arm,mhuv2" & "arm,primecell"
> > +- reg:         Contains the mailbox register address range (base
> > +               address and length)
> > +- #mbox-cells  Shall be 1 - the index of the channel needed.
> > +- mhu-frame    Frame type of the device.
> > +               Shall be either "sender" or "receiver"
> > +- mhu-protocol Transport protocol of the device. Shall be one of the
> > +               following: "single-word", "multi-word", "doorbell"
> > +
> > +Required properties (receiver frame):
> > +-------------------------------------
> > +- interrupts:  Contains the interrupt information corresponding to the
> > +               combined interrupt of the receiver frame
> > +
> > +Example:
> > +--------
> > +
> > +       mbox_mw_tx: mhu@10000000 {
> > +               compatible = "arm,mhuv2","arm,primecell";
> > +               reg = <0x10000000 0x1000>;
> > +               clocks = <&refclk100mhz>;
> > +               clock-names = "apb_pclk";
> > +               #mbox-cells = <1>;
> > +               mhu-protocol = "multi-word";
> > +               mhu-frame = "sender";
> > +       };
> > +
> > +       mbox_sw_tx: mhu@10000000 {
> > +               compatible = "arm,mhuv2","arm,primecell";
> > +               reg = <0x11000000 0x1000>;
> > +               clocks = <&refclk100mhz>;
> > +               clock-names = "apb_pclk";
> > +               #mbox-cells = <1>;
> > +               mhu-protocol = "single-word";
> > +               mhu-frame = "sender";
> > +       };
> > +
> > +       mbox_db_rx: mhu@10000000 {
> > +               compatible = "arm,mhuv2","arm,primecell";
> > +               reg = <0x12000000 0x1000>;
> > +               clocks = <&refclk100mhz>;
> > +               clock-names = "apb_pclk";
> > +               #mbox-cells = <1>;
> > +               interrupts = <0 45 4>;
> > +               interrupt-names = "mhu_rx";
> > +               mhu-protocol = "doorbell";
> > +               mhu-frame = "receiver";
> > +       };
> > +
> > +       mhu_client: scb@2e000000 {
> > +       compatible = "fujitsu,mb86s70-scb-1.0";
> > +       reg = <0 0x2e000000 0x4000>;
> > +       mboxes =
> > +               // For multi-word frames, client may only instantiate a single
> > +               // mailbox for a mailbox controller
> > +               <&mbox_mw_tx 0>,
> > +
> > +               // For single-word frames, client may instantiate as many
> > +               // mailboxes as there are channel windows in the MHU
> > +                <&mbox_sw_tx 0>,
> > +                <&mbox_sw_tx 1>,
> > +                <&mbox_sw_tx 2>,
> > +                <&mbox_sw_tx 3>,
> > +
> > +               // For doorbell frames, client may instantiate as many mailboxes
> > +               // as there are bits available in the combined number of channel
> > +               // windows ((channel windows * 32) mailboxes)
> > +                <mbox_db_rx 0>,
> > +                <mbox_db_rx 1>,
> > +                ...
> > +                <mbox_db_rx 17>;
> > +       };
>
> If the mhuv2 instance implements, say, 3 channel windows between
> sender (linux) and receiver (firmware), and Linux runs two protocols
> each requiring 1 and 2-word sized messages respectively. The hardware
> supports that by assigning windows [0] and [1,2] to each protocol.
> However, I don't think the driver can support that. Or does it?
>

FWIW, the IP is designed to cover wide range of usecase from IoT to servers
with variable window length. I don't see the need to complicate the driver
supporting mix-n-match at the cost of latency. Each platform choose one
transport protocol for all it's use.

--
Regards,
Sudeep

On 02/08/2019, 11:54, "Sudeep Holla" <sudeep.holla@arm.com> wrote:

    On Thu, Jul 25, 2019 at 12:49:58AM -0500, Jassi Brar wrote:
    > On Sun, Jul 21, 2019 at 4:58 PM Jassi Brar <jassisinghbrar@gmail.com> wrote:
    > >

    [...]

    > > If the mhuv2 instance implements, say, 3 channel windows between
    > > sender (linux) and receiver (firmware), and Linux runs two protocols
    > > each requiring 1 and 2-word sized messages respectively. The hardware
    > > supports that by assigning windows [0] and [1,2] to each protocol.
    > > However, I don't think the driver can support that. Or does it?
    > >
    > Thinking about it, IMO, the mbox-cell should carry a 128 (4x32) bit
    > mask specifying the set of windows (corresponding to the bits set in
    > the mask) associated with the channel.
    > And the controller driver should see any channel as associated with
    > variable number of windows 'N', where N is [0,124]
    >
    > mhu_client1: proto1@2e000000 {
    >        .....
    >        mboxes = <&mbox 0x0 0x0 0x0 0x1>
    > }
    >
    > mhu_client2: proto2@2f000000 {
    >        .....
    >        mboxes = <&mbox 0x0 0x0 0x0 0x6>
    > }
    >

    This still doesn't address the overhead I mentioned in my arm_mhu_v1
    series.

    As per you suggestion, we will have one channel with all possible
    bit mask value to specify the window. Let's imagine that 2 protocols
    share the same channel, then the requests are serialised.
    E.g. if bits 0 and 1 are allocated for say protocol#1 and bits 2 and 3
    for protocol#2.

At a given time only one protocol can be used by a client. No mix-match
of protocols are handled by the driver currently. Also its not possible to address all
possible scenarios offered by the IP. That's why the current driver design is
based on the implementation in the existing platforms.

    Further protocol#1 has higher latency requirements like sched-governor
    DVFS and there are 3-4 pending requests on protocol#2, then the incoming
    requests for protocol#1 is blocked.

    This is definitely overhead and I have seen lots of issue around this
    and hence I was requesting that we need to create individual channels
    for each of these. Having abstraction on top to multiplex or arbitrate
    won't help.

Also the (mbox-cells) approach will not allow us to differentiate between
single-word and doorbell which is required to make the controller driver
aware of the data expected whether it's a pointer to a location or in
register itself.

--Tushar
    --
    Regards,
    Sudeep


IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

On 02/08/2019, 11:59, "Sudeep Holla" <sudeep.holla@arm.com> wrote:

    On Sun, Jul 21, 2019 at 04:58:04PM -0500, Jassi Brar wrote:
    > On Wed, Jul 17, 2019 at 2:26 PM Tushar Khandelwal
    > <tushar.khandelwal@arm.com> wrote:
    >
    > > diff --git a/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
    > > new file mode 100644
    > > index 000000000000..3a05593414bc
    > > --- /dev/null
    > > +++ b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
    > > @@ -0,0 +1,108 @@
    > > +Arm MHUv2 Mailbox Driver
    > > +========================
    > > +
    > > +The Arm Message-Handling-Unit (MHU) Version 2 is a mailbox controller that has
    > > +between 1 and 124 channel windows to provide unidirectional communication with
    > > +remote processor(s).
    > > +
    > > +Given the unidirectional nature of the device, an MHUv2 mailbox may only be
    > > +written to or read from. If a pair of MHU devices is implemented between two
    > > +processing elements to provide bidirectional communication, these must be
    > > +specified as two separate mailboxes.
    > > +
    > > +A device tree node for an Arm MHUv2 device must specify either a receiver frame
    > > +or a sender frame, indicating which end of the unidirectional MHU device which
    > > +the device node entry describes.
    > > +
    > > +An MHU device must be specified with a transport protocol. The transport
    > > +protocol of an MHU device determines the method of data transmission as well as
    > > +the number of provided mailboxes.
    > > +Following are the possible transport protocol types:
    > > +- Single-word: An MHU device implements as many mailboxes as it
    > > +               provides channel windows. Data is transmitted through
    > > +               the MHU registers.
    > > +- Multi-word:  An MHU device implements a single mailbox. All channel windows
    > > +               will be used during transmission. Data is transmitted through
    > > +               the MHU registers.
    > > +- Doorbell:    An MHU device implements as many mailboxes as there are flag
    > > +               bits available in its channel windows. Optionally, data may
    > > +               be transmitted through a shared memory region, wherein the MHU
    > > +               is used strictly as an interrupt generation mechanism.
    > > +
    > > +Mailbox Device Node:
    > > +====================
    > > +
    > > +Required properties:
    > > +--------------------
    > > +- compatible:  Shall be "arm,mhuv2" & "arm,primecell"
    > > +- reg:         Contains the mailbox register address range (base
    > > +               address and length)
    > > +- #mbox-cells  Shall be 1 - the index of the channel needed.
    > > +- mhu-frame    Frame type of the device.
    > > +               Shall be either "sender" or "receiver"
    > > +- mhu-protocol Transport protocol of the device. Shall be one of the
    > > +               following: "single-word", "multi-word", "doorbell"
    > > +
    > > +Required properties (receiver frame):
    > > +-------------------------------------
    > > +- interrupts:  Contains the interrupt information corresponding to the
    > > +               combined interrupt of the receiver frame
    > > +
    > > +Example:
    > > +--------
    > > +
    > > +       mbox_mw_tx: mhu@10000000 {
    > > +               compatible = "arm,mhuv2","arm,primecell";
    > > +               reg = <0x10000000 0x1000>;
    > > +               clocks = <&refclk100mhz>;
    > > +               clock-names = "apb_pclk";
    > > +               #mbox-cells = <1>;
    > > +               mhu-protocol = "multi-word";
    > > +               mhu-frame = "sender";
    > > +       };
    > > +
    > > +       mbox_sw_tx: mhu@10000000 {
    > > +               compatible = "arm,mhuv2","arm,primecell";
    > > +               reg = <0x11000000 0x1000>;
    > > +               clocks = <&refclk100mhz>;
    > > +               clock-names = "apb_pclk";
    > > +               #mbox-cells = <1>;
    > > +               mhu-protocol = "single-word";
    > > +               mhu-frame = "sender";
    > > +       };
    > > +
    > > +       mbox_db_rx: mhu@10000000 {
    > > +               compatible = "arm,mhuv2","arm,primecell";
    > > +               reg = <0x12000000 0x1000>;
    > > +               clocks = <&refclk100mhz>;
    > > +               clock-names = "apb_pclk";
    > > +               #mbox-cells = <1>;
    > > +               interrupts = <0 45 4>;
    > > +               interrupt-names = "mhu_rx";
    > > +               mhu-protocol = "doorbell";
    > > +               mhu-frame = "receiver";
    > > +       };
    > > +
    > > +       mhu_client: scb@2e000000 {
    > > +       compatible = "fujitsu,mb86s70-scb-1.0";
    > > +       reg = <0 0x2e000000 0x4000>;
    > > +       mboxes =
    > > +               // For multi-word frames, client may only instantiate a single
    > > +               // mailbox for a mailbox controller
    > > +               <&mbox_mw_tx 0>,
    > > +
    > > +               // For single-word frames, client may instantiate as many
    > > +               // mailboxes as there are channel windows in the MHU
    > > +                <&mbox_sw_tx 0>,
    > > +                <&mbox_sw_tx 1>,
    > > +                <&mbox_sw_tx 2>,
    > > +                <&mbox_sw_tx 3>,
    > > +
    > > +               // For doorbell frames, client may instantiate as many mailboxes
    > > +               // as there are bits available in the combined number of channel
    > > +               // windows ((channel windows * 32) mailboxes)
    > > +                <mbox_db_rx 0>,
    > > +                <mbox_db_rx 1>,
    > > +                ...
    > > +                <mbox_db_rx 17>;
    > > +       };
    >
    > If the mhuv2 instance implements, say, 3 channel windows between
    > sender (linux) and receiver (firmware), and Linux runs two protocols
    > each requiring 1 and 2-word sized messages respectively. The hardware
    > supports that by assigning windows [0] and [1,2] to each protocol.
    > However, I don't think the driver can support that. Or does it?
    >

    FWIW, the IP is designed to cover wide range of usecase from IoT to servers
    with variable window length. I don't see the need to complicate the driver
    supporting mix-n-match at the cost of latency. Each platform choose one
    transport protocol for all it's use.

The driver design is to address the most probable scenarios and not all.
Single-word : Client gets one 32-bit window
Doorbell : Client gets 32 data pointers (arm_message)
Multi-word: Client gets all channels available in the platform.

--Tushar
    --
    Regards,
    Sudeep


IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

On 8/7/19 1:17 PM, Tushar Khandelwal wrote:
> 
> 
> On 02/08/2019, 11:59, "Sudeep Holla" <sudeep.holla@arm.com> wrote:
> 
>     On Sun, Jul 21, 2019 at 04:58:04PM -0500, Jassi Brar wrote:
>     > On Wed, Jul 17, 2019 at 2:26 PM Tushar Khandelwal
>     > <tushar.khandelwal@arm.com> wrote:
>     >
>     > > diff --git a/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
>     > > new file mode 100644
>     > > index 000000000000..3a05593414bc
>     > > --- /dev/null
>     > > +++ b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
>     > > @@ -0,0 +1,108 @@
>     > > +Arm MHUv2 Mailbox Driver
>     > > +========================
>     > > +
>     > > +The Arm Message-Handling-Unit (MHU) Version 2 is a mailbox controller that has
>     > > +between 1 and 124 channel windows to provide unidirectional communication with
>     > > +remote processor(s).
>     > > +
>     > > +Given the unidirectional nature of the device, an MHUv2 mailbox may only be
>     > > +written to or read from. If a pair of MHU devices is implemented between two
>     > > +processing elements to provide bidirectional communication, these must be
>     > > +specified as two separate mailboxes.
>     > > +
>     > > +A device tree node for an Arm MHUv2 device must specify either a receiver frame
>     > > +or a sender frame, indicating which end of the unidirectional MHU device which
>     > > +the device node entry describes.
>     > > +
>     > > +An MHU device must be specified with a transport protocol. The transport
>     > > +protocol of an MHU device determines the method of data transmission as well as
>     > > +the number of provided mailboxes.
>     > > +Following are the possible transport protocol types:
>     > > +- Single-word: An MHU device implements as many mailboxes as it
>     > > +               provides channel windows. Data is transmitted through
>     > > +               the MHU registers.
>     > > +- Multi-word:  An MHU device implements a single mailbox. All channel windows
>     > > +               will be used during transmission. Data is transmitted through
>     > > +               the MHU registers.
>     > > +- Doorbell:    An MHU device implements as many mailboxes as there are flag
>     > > +               bits available in its channel windows. Optionally, data may
>     > > +               be transmitted through a shared memory region, wherein the MHU
>     > > +               is used strictly as an interrupt generation mechanism.
>     > > +
>     > > +Mailbox Device Node:
>     > > +====================
>     > > +
>     > > +Required properties:
>     > > +--------------------
>     > > +- compatible:  Shall be "arm,mhuv2" & "arm,primecell"
>     > > +- reg:         Contains the mailbox register address range (base
>     > > +               address and length)
>     > > +- #mbox-cells  Shall be 1 - the index of the channel needed.
>     > > +- mhu-frame    Frame type of the device.
>     > > +               Shall be either "sender" or "receiver"
>     > > +- mhu-protocol Transport protocol of the device. Shall be one of the
>     > > +               following: "single-word", "multi-word", "doorbell"
>     > > +
>     > > +Required properties (receiver frame):
>     > > +-------------------------------------
>     > > +- interrupts:  Contains the interrupt information corresponding to the
>     > > +               combined interrupt of the receiver frame
>     > > +
>     > > +Example:
>     > > +--------
>     > > +
>     > > +       mbox_mw_tx: mhu@10000000 {
>     > > +               compatible = "arm,mhuv2","arm,primecell";
>     > > +               reg = <0x10000000 0x1000>;
>     > > +               clocks = <&refclk100mhz>;
>     > > +               clock-names = "apb_pclk";
>     > > +               #mbox-cells = <1>;
>     > > +               mhu-protocol = "multi-word";
>     > > +               mhu-frame = "sender";
>     > > +       };
>     > > +
>     > > +       mbox_sw_tx: mhu@10000000 {
>     > > +               compatible = "arm,mhuv2","arm,primecell";
>     > > +               reg = <0x11000000 0x1000>;
>     > > +               clocks = <&refclk100mhz>;
>     > > +               clock-names = "apb_pclk";
>     > > +               #mbox-cells = <1>;
>     > > +               mhu-protocol = "single-word";
>     > > +               mhu-frame = "sender";
>     > > +       };
>     > > +
>     > > +       mbox_db_rx: mhu@10000000 {
>     > > +               compatible = "arm,mhuv2","arm,primecell";
>     > > +               reg = <0x12000000 0x1000>;
>     > > +               clocks = <&refclk100mhz>;
>     > > +               clock-names = "apb_pclk";
>     > > +               #mbox-cells = <1>;
>     > > +               interrupts = <0 45 4>;
>     > > +               interrupt-names = "mhu_rx";
>     > > +               mhu-protocol = "doorbell";
>     > > +               mhu-frame = "receiver";
>     > > +       };
>     > > +
>     > > +       mhu_client: scb@2e000000 {
>     > > +       compatible = "fujitsu,mb86s70-scb-1.0";
>     > > +       reg = <0 0x2e000000 0x4000>;
>     > > +       mboxes =
>     > > +               // For multi-word frames, client may only instantiate a single
>     > > +               // mailbox for a mailbox controller
>     > > +               <&mbox_mw_tx 0>,
>     > > +
>     > > +               // For single-word frames, client may instantiate as many
>     > > +               // mailboxes as there are channel windows in the MHU
>     > > +                <&mbox_sw_tx 0>,
>     > > +                <&mbox_sw_tx 1>,
>     > > +                <&mbox_sw_tx 2>,
>     > > +                <&mbox_sw_tx 3>,
>     > > +
>     > > +               // For doorbell frames, client may instantiate as many mailboxes
>     > > +               // as there are bits available in the combined number of channel
>     > > +               // windows ((channel windows * 32) mailboxes)
>     > > +                <mbox_db_rx 0>,
>     > > +                <mbox_db_rx 1>,
>     > > +                ...
>     > > +                <mbox_db_rx 17>;
>     > > +       };
>     >
>     > If the mhuv2 instance implements, say, 3 channel windows between
>     > sender (linux) and receiver (firmware), and Linux runs two protocols
>     > each requiring 1 and 2-word sized messages respectively. The hardware
>     > supports that by assigning windows [0] and [1,2] to each protocol.
>     > However, I don't think the driver can support that. Or does it?
>     >
> 
>     FWIW, the IP is designed to cover wide range of usecase from IoT to servers
>     with variable window length. I don't see the need to complicate the driver
>     supporting mix-n-match at the cost of latency. Each platform choose one
>     transport protocol for all it's use.
> 
> The driver design is to address the most probable scenarios and not all.
> Single-word : Client gets one 32-bit window
> Doorbell : Client gets 32 data pointers (arm_message)
> Multi-word: Client gets all channels available in the platform.
> 
> --Tushar
>

To elaborate for better understanding;
arm_message is used when a mailbox client is to transmit data through an
MHUv2 mailbox (ie. in-band transmission), wherein the MHUv2 is in a
single-word or multi-word protocol mode.
arm_message allows for a mailbox client to transmit data with arbitrary
length, without having to know the technicalities of underlying
transport protocol, number of MHU channels etc. As such, the mailbox
client driver code may remain unchanged, if the underlying transport
protocol is changed from ie. single-word to multi-word, or if more
channel-windows are added to an MHUv2 device in multi-word mode.
Transmission of data as well as maximum utilization of the available
channel windows (in multi-word mode) is solely job for the MHUv2 driver.

Given the differences between using an MHUv2 mailbox in single- and
multi-word mode compared to doorbell mode, it is however expected that
mailbox client driver code must be changed if switching from single- or
multi-word to doorbell.
As per the MHUv2 spec, when in doorbell mode, it is expected that the
receiver and transmitter has decided beforehand both where data is
placed in shared memory, as well as the format of this data. Here we do
not require that arm_message is used, given that all data is transmitted
out-of-band, and the MHUv2 is only used as an interrupt generating
mechanism.

To give an example of how arm_message may be used;
Say, a client-driver protocol has been written as follows:

> struct {
>     int cmd;
>     int[4] args;
> } foo_t;

Now, a client driver may transmit this through a mailbox which has an
MHUv2 as the controller - we do not need to know the underlying
transport protocol, just that it has to be either single- or multi word.

> // Create user-specific protocol struct
> foo_t foo;
>
> /* Modify foo with the data to be transmitted... */
>
> /* Wrap message into arm_message format, so the MHUv2 driver knows how
> much data to send*/
>
> arm_message msg;
> msg.data = &foo;
> msg.len = sizeof(foo);
>
> /* Transmit the arm_message */
> mbox_send_message(mbox, &msg);
>

On the receiver side, there should be some accompanying logic in the
client driver which is aware of foo_t and from this may reconstruct the
received message. Reconstruction of a received message is a task for the
mailbox client driver.

- Morten

On Fri, Aug 2, 2019 at 5:41 AM Morten Borup Petersen <morten_bp@live.dk> wrote:
>
>
>
> On 7/31/19 9:31 AM, Jassi Brar wrote:
> > On Sun, Jul 28, 2019 at 4:28 PM Morten Borup Petersen <morten_bp@live.dk> wrote:
> >>
> >>
> >>
> >> On 7/25/19 7:49 AM, Jassi Brar wrote:
> >>> On Sun, Jul 21, 2019 at 4:58 PM Jassi Brar <jassisinghbrar@gmail.com> wrote:
> >>>>
> >>>> On Wed, Jul 17, 2019 at 2:26 PM Tushar Khandelwal
> >>>> <tushar.khandelwal@arm.com> wrote:
> >>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
> >>>>> new file mode 100644
> >>>>> index 000000000000..3a05593414bc
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
> >>>>> @@ -0,0 +1,108 @@
> >>>>> +Arm MHUv2 Mailbox Driver
> >>>>> +========================
> >>>>> +
> >>>>> +The Arm Message-Handling-Unit (MHU) Version 2 is a mailbox controller that has
> >>>>> +between 1 and 124 channel windows to provide unidirectional communication with
> >>>>> +remote processor(s).
> >>>>> +
> >>>>> +Given the unidirectional nature of the device, an MHUv2 mailbox may only be
> >>>>> +written to or read from. If a pair of MHU devices is implemented between two
> >>>>> +processing elements to provide bidirectional communication, these must be
> >>>>> +specified as two separate mailboxes.
> >>>>> +
> >>>>> +A device tree node for an Arm MHUv2 device must specify either a receiver frame
> >>>>> +or a sender frame, indicating which end of the unidirectional MHU device which
> >>>>> +the device node entry describes.
> >>>>> +
> >>>>> +An MHU device must be specified with a transport protocol. The transport
> >>>>> +protocol of an MHU device determines the method of data transmission as well as
> >>>>> +the number of provided mailboxes.
> >>>>> +Following are the possible transport protocol types:
> >>>>> +- Single-word: An MHU device implements as many mailboxes as it
> >>>>> +               provides channel windows. Data is transmitted through
> >>>>> +               the MHU registers.
> >>>>> +- Multi-word:  An MHU device implements a single mailbox. All channel windows
> >>>>> +               will be used during transmission. Data is transmitted through
> >>>>> +               the MHU registers.
> >>>>> +- Doorbell:    An MHU device implements as many mailboxes as there are flag
> >>>>> +               bits available in its channel windows. Optionally, data may
> >>>>> +               be transmitted through a shared memory region, wherein the MHU
> >>>>> +               is used strictly as an interrupt generation mechanism.
> >>>>> +
> >>>>> +Mailbox Device Node:
> >>>>> +====================
> >>>>> +
> >>>>> +Required properties:
> >>>>> +--------------------
> >>>>> +- compatible:  Shall be "arm,mhuv2" & "arm,primecell"
> >>>>> +- reg:         Contains the mailbox register address range (base
> >>>>> +               address and length)
> >>>>> +- #mbox-cells  Shall be 1 - the index of the channel needed.
> >>>>> +- mhu-frame    Frame type of the device.
> >>>>> +               Shall be either "sender" or "receiver"
> >>>>> +- mhu-protocol Transport protocol of the device. Shall be one of the
> >>>>> +               following: "single-word", "multi-word", "doorbell"
> >>>>> +
> >>>>> +Required properties (receiver frame):
> >>>>> +-------------------------------------
> >>>>> +- interrupts:  Contains the interrupt information corresponding to the
> >>>>> +               combined interrupt of the receiver frame
> >>>>> +
> >>>>> +Example:
> >>>>> +--------
> >>>>> +
> >>>>> +       mbox_mw_tx: mhu@10000000 {
> >>>>> +               compatible = "arm,mhuv2","arm,primecell";
> >>>>> +               reg = <0x10000000 0x1000>;
> >>>>> +               clocks = <&refclk100mhz>;
> >>>>> +               clock-names = "apb_pclk";
> >>>>> +               #mbox-cells = <1>;
> >>>>> +               mhu-protocol = "multi-word";
> >>>>> +               mhu-frame = "sender";
> >>>>> +       };
> >>>>> +
> >>>>> +       mbox_sw_tx: mhu@10000000 {
> >>>>> +               compatible = "arm,mhuv2","arm,primecell";
> >>>>> +               reg = <0x11000000 0x1000>;
> >>>>> +               clocks = <&refclk100mhz>;
> >>>>> +               clock-names = "apb_pclk";
> >>>>> +               #mbox-cells = <1>;
> >>>>> +               mhu-protocol = "single-word";
> >>>>> +               mhu-frame = "sender";
> >>>>> +       };
> >>>>> +
> >>>>> +       mbox_db_rx: mhu@10000000 {
> >>>>> +               compatible = "arm,mhuv2","arm,primecell";
> >>>>> +               reg = <0x12000000 0x1000>;
> >>>>> +               clocks = <&refclk100mhz>;
> >>>>> +               clock-names = "apb_pclk";
> >>>>> +               #mbox-cells = <1>;
> >>>>> +               interrupts = <0 45 4>;
> >>>>> +               interrupt-names = "mhu_rx";
> >>>>> +               mhu-protocol = "doorbell";
> >>>>> +               mhu-frame = "receiver";
> >>>>> +       };
> >>>>> +
> >>>>> +       mhu_client: scb@2e000000 {
> >>>>> +       compatible = "fujitsu,mb86s70-scb-1.0";
> >>>>> +       reg = <0 0x2e000000 0x4000>;
> >>>>> +       mboxes =
> >>>>> +               // For multi-word frames, client may only instantiate a single
> >>>>> +               // mailbox for a mailbox controller
> >>>>> +               <&mbox_mw_tx 0>,
> >>>>> +
> >>>>> +               // For single-word frames, client may instantiate as many
> >>>>> +               // mailboxes as there are channel windows in the MHU
> >>>>> +                <&mbox_sw_tx 0>,
> >>>>> +                <&mbox_sw_tx 1>,
> >>>>> +                <&mbox_sw_tx 2>,
> >>>>> +                <&mbox_sw_tx 3>,
> >>>>> +
> >>>>> +               // For doorbell frames, client may instantiate as many mailboxes
> >>>>> +               // as there are bits available in the combined number of channel
> >>>>> +               // windows ((channel windows * 32) mailboxes)
> >>>>> +                <mbox_db_rx 0>,
> >>>>> +                <mbox_db_rx 1>,
> >>>>> +                ...
> >>>>> +                <mbox_db_rx 17>;
> >>>>> +       };
> >>>>
> >>>> If the mhuv2 instance implements, say, 3 channel windows between
> >>>> sender (linux) and receiver (firmware), and Linux runs two protocols
> >>>> each requiring 1 and 2-word sized messages respectively. The hardware
> >>>> supports that by assigning windows [0] and [1,2] to each protocol.
> >>>> However, I don't think the driver can support that. Or does it?
> >>>>
> >>> Thinking about it, IMO, the mbox-cell should carry a 128 (4x32) bit
> >>> mask specifying the set of windows (corresponding to the bits set in
> >>> the mask) associated with the channel.
> >>> And the controller driver should see any channel as associated with
> >>> variable number of windows 'N', where N is [0,124]
> >>>
> >>> mhu_client1: proto1@2e000000 {
> >>>        .....
> >>>        mboxes = <&mbox 0x0 0x0 0x0 0x1>
> >>> }
> >>>
> >>> mhu_client2: proto2@2f000000 {
> >>>        .....
> >>>        mboxes = <&mbox 0x0 0x0 0x0 0x6>
> >>> }
> >>>
> >>> Cheers!
> >>>
> >>
> >> As mentioned in the response to your initial comment, the driver does
> >> not currently support mixing protocols.
> >>
> > Thanks for acknowledging that limitation. But lets also address it.
> >
>
> We are hesitant to dedicate time to developing mixing protocols given
> that we don't have any current usecase nor any current platform which
> would support this.
>
Can you please share the client code against which you tested this driver?
From my past experience, I realise it is much more efficient to tidyup
the code myself, than endlessly trying to explain the benefits.

Thanks

On Tue, Aug 13, 2019 at 11:36:56AM -0500, Jassi Brar wrote:
[...]

> > >>
> > >> As mentioned in the response to your initial comment, the driver does
> > >> not currently support mixing protocols.
> > >>
> > > Thanks for acknowledging that limitation. But lets also address it.
> > >
> >
> > We are hesitant to dedicate time to developing mixing protocols given
> > that we don't have any current usecase nor any current platform which
> > would support this.
> >
> Can you please share the client code against which you tested this driver?
> From my past experience, I realise it is much more efficient to tidyup
> the code myself, than endlessly trying to explain the benefits.
>

Thanks for the patience and offer. Can we try the same with MHUv1 and SCMI
upstream driver.

The firmware just uses High Priority physical channel bit 0 and 2 as Tx
and bit 1 and 3 as Rx. Bit 2 and 3 are for perf which shouldn't get blocked
by bit 0 and 1. I mean I can have 10 requests covering clocks/sensors and
others on bit 0 and 1, but the bits 2 and 3 are dedicated for DVFS and
shouldn't be blocked because of other non DVFS requests.

The DT looks something like this(modified MHU binding for 2 cells)

	mailbox: mhu@2b1f0000 {
		compatible = "arm,primecell";
		reg = <0x0 0x2b1f0000 0x0 0x1000>;
		interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>,
			     <GIC_SPI 35 IRQ_TYPE_LEVEL_HIGH>;
		interrupt-names = "mhu_lpri_rx",
				  "mhu_hpri_rx";
		#mbox-cells = <2>;
		mbox-name = "ARM-MHU";
		clocks = <&soc_refclk100mhz>;
		clock-names = "apb_pclk";
	};
	firmware {
		scmi {
			compatible = "arm,scmi";
			mbox-names = "tx", "rx";
			mboxes = <&mailbox 0 0 &mailbox 0 1>;
			#address-cells = <1>;
			#size-cells = <0>;

			scmi_devpd: protocol@11 {
				reg = <0x11>;
				#power-domain-cells = <1>;
			};

			scmi_dvfs: protocol@13 {
				reg = <0x13>;
				#clock-cells = <1>;
				mbox-names = "tx", "rx";
				mboxes = <&mailbox 0 2 &mailbox 0 3>;
			};

			scmi_clk: protocol@14 {
				reg = <0x14>;
				#clock-cells = <1>;
			};

			scmi_sensors0: protocol@15 {
				reg = <0x15>;
				#thermal-sensor-cells = <1>;
			};
		};
	};

--
Regards,
Sudeep

On Wed, Aug 14, 2019 at 5:05 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Tue, Aug 13, 2019 at 11:36:56AM -0500, Jassi Brar wrote:
> [...]
>
> > > >>
> > > >> As mentioned in the response to your initial comment, the driver does
> > > >> not currently support mixing protocols.
> > > >>
> > > > Thanks for acknowledging that limitation. But lets also address it.
> > > >
> > >
> > > We are hesitant to dedicate time to developing mixing protocols given
> > > that we don't have any current usecase nor any current platform which
> > > would support this.
> > >
> > Can you please share the client code against which you tested this driver?
> > From my past experience, I realise it is much more efficient to tidyup
> > the code myself, than endlessly trying to explain the benefits.
> >
>
> Thanks for the patience and offer.
>
Ok, but the offer is to Morten for MHUv2 driver.

> Can we try the same with MHUv1 and SCMI
> upstream driver.
>
MHUv1 driver is fine as it is.
I did try my best to keep you from messing the SCMI driver, without success
https://lkml.org/lkml/2017/8/7/924

On Wed, Aug 14, 2019 at 09:52:25AM -0500, Jassi Brar wrote:
> On Wed, Aug 14, 2019 at 5:05 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Tue, Aug 13, 2019 at 11:36:56AM -0500, Jassi Brar wrote:
> > [...]
> >
> > > > >>
> > > > >> As mentioned in the response to your initial comment, the driver does
> > > > >> not currently support mixing protocols.
> > > > >>
> > > > > Thanks for acknowledging that limitation. But lets also address it.
> > > > >
> > > >
> > > > We are hesitant to dedicate time to developing mixing protocols given
> > > > that we don't have any current usecase nor any current platform which
> > > > would support this.
> > > >
> > > Can you please share the client code against which you tested this driver?
> > > From my past experience, I realise it is much more efficient to tidyup
> > > the code myself, than endlessly trying to explain the benefits.
> > >
> >
> > Thanks for the patience and offer.
> >
> Ok, but the offer is to Morten for MHUv2 driver.
> 
> > Can we try the same with MHUv1 and SCMI
> > upstream driver.
> >
> MHUv1 driver is fine as it is.
> I did try my best to keep you from messing the SCMI driver, without success
> https://lkml.org/lkml/2017/8/7/924

I disagree, you haven't told me how to address the usecase which I mentioned
with the abstraction/multiplexer on top of MHU as you have been suggesting.

I am sure MHUv2 will have the same usecase.

--
Regards,
Sudeep

On 13/08/2019 17:36, Jassi Brar wrote:
> On Fri, Aug 2, 2019 at 5:41 AM Morten Borup Petersen <morten_bp@live.dk> wrote:
>>
>>
>>
>> On 7/31/19 9:31 AM, Jassi Brar wrote:
>>> On Sun, Jul 28, 2019 at 4:28 PM Morten Borup Petersen <morten_bp@live.dk> wrote:
>>>>
>>>>
>>>>
>>>> On 7/25/19 7:49 AM, Jassi Brar wrote:
>>>>> On Sun, Jul 21, 2019 at 4:58 PM Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>>>>>
>>>>>> On Wed, Jul 17, 2019 at 2:26 PM Tushar Khandelwal
>>>>>> <tushar.khandelwal@arm.com> wrote:
>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..3a05593414bc
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
>>>>>>> @@ -0,0 +1,108 @@
>>>>>>> +Arm MHUv2 Mailbox Driver
>>>>>>> +========================
>>>>>>> +
>>>>>>> +The Arm Message-Handling-Unit (MHU) Version 2 is a mailbox controller that has
>>>>>>> +between 1 and 124 channel windows to provide unidirectional communication with
>>>>>>> +remote processor(s).
>>>>>>> +
>>>>>>> +Given the unidirectional nature of the device, an MHUv2 mailbox may only be
>>>>>>> +written to or read from. If a pair of MHU devices is implemented between two
>>>>>>> +processing elements to provide bidirectional communication, these must be
>>>>>>> +specified as two separate mailboxes.
>>>>>>> +
>>>>>>> +A device tree node for an Arm MHUv2 device must specify either a receiver frame
>>>>>>> +or a sender frame, indicating which end of the unidirectional MHU device which
>>>>>>> +the device node entry describes.
>>>>>>> +
>>>>>>> +An MHU device must be specified with a transport protocol. The transport
>>>>>>> +protocol of an MHU device determines the method of data transmission as well as
>>>>>>> +the number of provided mailboxes.
>>>>>>> +Following are the possible transport protocol types:
>>>>>>> +- Single-word: An MHU device implements as many mailboxes as it
>>>>>>> +               provides channel windows. Data is transmitted through
>>>>>>> +               the MHU registers.
>>>>>>> +- Multi-word:  An MHU device implements a single mailbox. All channel windows
>>>>>>> +               will be used during transmission. Data is transmitted through
>>>>>>> +               the MHU registers.
>>>>>>> +- Doorbell:    An MHU device implements as many mailboxes as there are flag
>>>>>>> +               bits available in its channel windows. Optionally, data may
>>>>>>> +               be transmitted through a shared memory region, wherein the MHU
>>>>>>> +               is used strictly as an interrupt generation mechanism.
>>>>>>> +
>>>>>>> +Mailbox Device Node:
>>>>>>> +====================
>>>>>>> +
>>>>>>> +Required properties:
>>>>>>> +--------------------
>>>>>>> +- compatible:  Shall be "arm,mhuv2" & "arm,primecell"
>>>>>>> +- reg:         Contains the mailbox register address range (base
>>>>>>> +               address and length)
>>>>>>> +- #mbox-cells  Shall be 1 - the index of the channel needed.
>>>>>>> +- mhu-frame    Frame type of the device.
>>>>>>> +               Shall be either "sender" or "receiver"
>>>>>>> +- mhu-protocol Transport protocol of the device. Shall be one of the
>>>>>>> +               following: "single-word", "multi-word", "doorbell"
>>>>>>> +
>>>>>>> +Required properties (receiver frame):
>>>>>>> +-------------------------------------
>>>>>>> +- interrupts:  Contains the interrupt information corresponding to the
>>>>>>> +               combined interrupt of the receiver frame
>>>>>>> +
>>>>>>> +Example:
>>>>>>> +--------
>>>>>>> +
>>>>>>> +       mbox_mw_tx: mhu@10000000 {
>>>>>>> +               compatible = "arm,mhuv2","arm,primecell";
>>>>>>> +               reg = <0x10000000 0x1000>;
>>>>>>> +               clocks = <&refclk100mhz>;
>>>>>>> +               clock-names = "apb_pclk";
>>>>>>> +               #mbox-cells = <1>;
>>>>>>> +               mhu-protocol = "multi-word";
>>>>>>> +               mhu-frame = "sender";
>>>>>>> +       };
>>>>>>> +
>>>>>>> +       mbox_sw_tx: mhu@10000000 {
>>>>>>> +               compatible = "arm,mhuv2","arm,primecell";
>>>>>>> +               reg = <0x11000000 0x1000>;
>>>>>>> +               clocks = <&refclk100mhz>;
>>>>>>> +               clock-names = "apb_pclk";
>>>>>>> +               #mbox-cells = <1>;
>>>>>>> +               mhu-protocol = "single-word";
>>>>>>> +               mhu-frame = "sender";
>>>>>>> +       };
>>>>>>> +
>>>>>>> +       mbox_db_rx: mhu@10000000 {
>>>>>>> +               compatible = "arm,mhuv2","arm,primecell";
>>>>>>> +               reg = <0x12000000 0x1000>;
>>>>>>> +               clocks = <&refclk100mhz>;
>>>>>>> +               clock-names = "apb_pclk";
>>>>>>> +               #mbox-cells = <1>;
>>>>>>> +               interrupts = <0 45 4>;
>>>>>>> +               interrupt-names = "mhu_rx";
>>>>>>> +               mhu-protocol = "doorbell";
>>>>>>> +               mhu-frame = "receiver";
>>>>>>> +       };
>>>>>>> +
>>>>>>> +       mhu_client: scb@2e000000 {
>>>>>>> +       compatible = "fujitsu,mb86s70-scb-1.0";
>>>>>>> +       reg = <0 0x2e000000 0x4000>;
>>>>>>> +       mboxes =
>>>>>>> +               // For multi-word frames, client may only instantiate a single
>>>>>>> +               // mailbox for a mailbox controller
>>>>>>> +               <&mbox_mw_tx 0>,
>>>>>>> +
>>>>>>> +               // For single-word frames, client may instantiate as many
>>>>>>> +               // mailboxes as there are channel windows in the MHU
>>>>>>> +                <&mbox_sw_tx 0>,
>>>>>>> +                <&mbox_sw_tx 1>,
>>>>>>> +                <&mbox_sw_tx 2>,
>>>>>>> +                <&mbox_sw_tx 3>,
>>>>>>> +
>>>>>>> +               // For doorbell frames, client may instantiate as many mailboxes
>>>>>>> +               // as there are bits available in the combined number of channel
>>>>>>> +               // windows ((channel windows * 32) mailboxes)
>>>>>>> +                <mbox_db_rx 0>,
>>>>>>> +                <mbox_db_rx 1>,
>>>>>>> +                ...
>>>>>>> +                <mbox_db_rx 17>;
>>>>>>> +       };
>>>>>>
>>>>>> If the mhuv2 instance implements, say, 3 channel windows between
>>>>>> sender (linux) and receiver (firmware), and Linux runs two protocols
>>>>>> each requiring 1 and 2-word sized messages respectively. The hardware
>>>>>> supports that by assigning windows [0] and [1,2] to each protocol.
>>>>>> However, I don't think the driver can support that. Or does it?
>>>>>>
>>>>> Thinking about it, IMO, the mbox-cell should carry a 128 (4x32) bit
>>>>> mask specifying the set of windows (corresponding to the bits set in
>>>>> the mask) associated with the channel.
>>>>> And the controller driver should see any channel as associated with
>>>>> variable number of windows 'N', where N is [0,124]
>>>>>
>>>>> mhu_client1: proto1@2e000000 {
>>>>>         .....
>>>>>         mboxes = <&mbox 0x0 0x0 0x0 0x1>
>>>>> }
>>>>>
>>>>> mhu_client2: proto2@2f000000 {
>>>>>         .....
>>>>>         mboxes = <&mbox 0x0 0x0 0x0 0x6>
>>>>> }
>>>>>
>>>>> Cheers!
>>>>>
>>>>
>>>> As mentioned in the response to your initial comment, the driver does
>>>> not currently support mixing protocols.
>>>>
>>> Thanks for acknowledging that limitation. But lets also address it.
>>>
>>
>> We are hesitant to dedicate time to developing mixing protocols given
>> that we don't have any current usecase nor any current platform which
>> would support this.
>>
> Can you please share the client code against which you tested this driver?
>  From my past experience, I realise it is much more efficient to tidyup
> the code myself, than endlessly trying to explain the benefits.
> 

Yes, I will share that soon.

> Thanks
>

On 14/08/2019 17:51, Sudeep Holla wrote:
> On Wed, Aug 14, 2019 at 09:52:25AM -0500, Jassi Brar wrote:
>> On Wed, Aug 14, 2019 at 5:05 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>>>
>>> On Tue, Aug 13, 2019 at 11:36:56AM -0500, Jassi Brar wrote:
>>> [...]
>>>
>>>>>>>
>>>>>>> As mentioned in the response to your initial comment, the driver does
>>>>>>> not currently support mixing protocols.
>>>>>>>
>>>>>> Thanks for acknowledging that limitation. But lets also address it.
>>>>>>
>>>>>
>>>>> We are hesitant to dedicate time to developing mixing protocols given
>>>>> that we don't have any current usecase nor any current platform which
>>>>> would support this.
>>>>>
>>>> Can you please share the client code against which you tested this driver?
>>>>  From my past experience, I realise it is much more efficient to tidyup
>>>> the code myself, than endlessly trying to explain the benefits.
>>>>
>>>
>>> Thanks for the patience and offer.
>>>
>> Ok, but the offer is to Morten for MHUv2 driver.
>>
>>> Can we try the same with MHUv1 and SCMI
>>> upstream driver.
>>>
>> MHUv1 driver is fine as it is.
>> I did try my best to keep you from messing the SCMI driver, without success
>> https://lkml.org/lkml/2017/8/7/924
> 
> I disagree, you haven't told me how to address the usecase which I mentioned
> with the abstraction/multiplexer on top of MHU as you have been suggesting.
> 
> I am sure MHUv2 will have the same usecase.
> 

MHUv2 driver is addressing existing (door-bell) use case as well as new 
(multi-word) use case using new IP features.

> --
> Regards,
> Sudeep
>