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

["A.s. Dong" <aisheng.dong@nxp.com>]

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

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

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

Regards
Dong Aisheng

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