Message-ID: <VI1PR0601MB2111A5A4E951F011D389A8978FD90@VI1PR0601MB2111.eurprd06.prod.outlook.com>
Date:   Fri, 2 Aug 2019 10:41:43 +0000
From:   Morten Borup Petersen <morten_bp@...e.dk>
To:     Jassi Brar <jassisinghbrar@...il.com>
CC:     Tushar Khandelwal <tushar.khandelwal@....com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "tushar.2nov@...il.com" <tushar.2nov@...il.com>,
        "nd@....com" <nd@....com>,
        Morten Borup Petersen <morten.petersen@....com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Devicetree List <devicetree@...r.kernel.org>
Subject: Re: [PATCH 1/4] mailbox: arm_mhuv2: add device tree binding
 documentation



On 7/31/19 9:31 AM, Jassi Brar wrote:
> On Sun, Jul 28, 2019 at 4:28 PM Morten Borup Petersen <morten_bp@...e.dk> wrote:
>>
>>
>>
>> On 7/25/19 7:49 AM, Jassi Brar wrote:
>>> On Sun, Jul 21, 2019 at 4:58 PM Jassi Brar <jassisinghbrar@...il.com> wrote:
>>>>
>>>> On Wed, Jul 17, 2019 at 2:26 PM Tushar Khandelwal
>>>> <tushar.khandelwal@....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@...00000 {
>>>>> +               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@...00000 {
>>>>> +               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@...00000 {
>>>>> +               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@...00000 {
>>>>> +       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@...00000 {
>>>        .....
>>>        mboxes = <&mbox 0x0 0x0 0x0 0x1>
>>> }
>>>
>>> mhu_client2: proto2@...00000 {
>>>        .....
>>>        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@...00000 {
       .....
       /* Requesting to use channel window 0 of &mbox, registering
          a mailbox in singe-word mode. */
       mboxes = <&mbox 0x0 0x0 0x0 0x1>
}


mhu_client2: proto1@...00000 {
        .....
	/* 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@...00000 {
       .....
       /* Requesting to use channel window 2-3 of &mbox, registering
          a mailbox in multi-word mode. */
       mboxes = <&mbox 0x0 0x0 0x0 0xC>
}


Thanks,

Morten

Powered by blists - more mailing lists