-----Original Message-----
From: Jassi Brar [mailto:jassisinghbrar at gmail.com]
Sent: Friday, July 27, 2018 12:56 PM
To: A.s. Dong <aisheng.dong@nxp.com>
Cc: Oleksij Rempel <o.rempel@pengutronix.de>; , Sascha Hauer
[off-list ref]; Shawn Guo [off-list ref]; Fabio
Estevam [off-list ref]; Rob Herring [off-list ref];
Mark Rutland [off-list ref]; Vladimir Zapolskiy
[off-list ref]; , linux-arm-kernel at lists.infradead.org,
linux-mediatek at lists.infradead.org, srv_heupstream <linux-arm-
kernel at lists.infradead.org>; Devicetree List [off-list ref];
dl-linux-imx [off-list ref]
Subject: Re: [PATCH v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU
channel support

On Fri, Jul 27, 2018 at 9:32 AM, A.s. Dong [off-list ref] wrote:

quoted

quoted

quoted

quoted

quoted

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

So the hardware actually supports 4 channels.

quoted

-- #mbox-cells:  Must be 0. Number of cells in a mailbox
+- #mbox-cells:  Must be:
+               0 - for single channel mode. i.MX8* SCU protocol specific.
+               1 - for multichannel (generic) mode.
+

No, please.
DT bindings should reflect the real hardware, and not the software
mode we want the driver to work in.
Please define mbox-cells=1  and have the i.MX8* platform always
ask for channel-0.

The reality is that MU hardware does not define channels in
reference

manual.

quoted

However, it does have four separate data tx/rx register which can
be used as 'virtual' channels which is supported by this current driver.

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

other.

quoted

For a while please forget the protocol(user) level usage, and
consider only what your h/w is.

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

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

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

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

It _is_ necessary to write controller driver independent of client drivers.

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

quoted

And AFAIK ARM MHU is doing the same way as MU which looks like also

unidirectional channel.

quoted

https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Finfo

quoted

center.arm.com%2Fhelp%2Findex.jsp%3Ftopic%3D%2Fcom.arm.doc.ddi0515
b%2F

quoted

CHDGBGIF.html&data=02%7C01%7Caisheng.dong%40nxp.com%7Cb728
16362983

quoted

4208f9e908d5f37d3e00%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%
7C6366

quoted

82641593785009&sdata=VsG0oXxEObPRwT5KVss2eZthSTMTR2%2BMrv
PqhDUpPGU

quoted

%3D&reserved=0
drivers/mailbox/arm_mhu.c

MHU driver is doing exactly what the data sheet says. I know because I wrote
the driver.

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

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

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

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

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

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

quoted

quoted

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

This is one of the recommended ways designed in HW reference manual
and it allows to send frame information up to 4 words without using shared

memory.

quoted

SCU just follows it, so it's hard to believe it's a bad design.

quoted

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

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

It may work If SCU protocol data length is fixed to 4 words. However,
it's length is flexible for different SVC service. That means this binding

won't work for SCU.

quoted

And it will introduce much complexities during the implementation.

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

as a channel fifo.

quoted

You first give me the "Passing short messages" usecase (quite a bad
one) and ask how it can work. When I give you a solution, you effectively say
"well, my usecase isn't even that". I feel I just wasted my time.

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

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

quoted

quoted

quoted

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

interrupt at the receiver side).

quoted

The reference manual is at here: (Chapter 42 Messaging Unit (MU)

https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fw

quoted

quoted

ww

quoted

.nxp.com%2Fdocs%2Fen%2Freference-

manual%2FIMX6ULRM.pdf&data=02%7C0

quoted

1%7Caisheng.dong%40nxp.com%7Cef349d7c9caf46c4260008d5f30e9ef5%7C6

quoted

quoted

86ea1

quoted

d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636682166494860332&sdat

quoted

quoted

a=54rE

quoted

iDm%2BGD6EY8NE64ck1LXVGoUCtWYrHXPoWqcujUo%3D&reserved=0

quoted

quoted

quoted

And SCU firmware does use MU in above way specified by reference

manual.

quoted

Even from HW point of view, it's still one channel only mailbox.

Please realise that any manual is written by a mere mortal afterall.
How the controller works is set in stone, but how the controller can
be used ... is just someones suggestion.

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

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

I get the idea you were ready to see the code merged in the coming window
and be done with it. And now it feels lazy to change the code.

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

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

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

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

Or anything else?

quoted

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

Rob is indeed far better than I am. But he also has to look into 100 other
subsystems, whereas I am only looking into mailbox. I have time to dig into
your datasheets. I believe Rob will point out anything wrong I suggest.

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

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

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

Regards
Dong Aisheng