> -----Original Message----- > From: A.s. Dong > Sent: Friday, July 27, 2018 4:42 PM > To: Jassi Brar <jassisinghbrar@xxxxxxxxx> > Cc: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>; , Sascha Hauer > <kernel@xxxxxxxxxxxxxx>; Shawn Guo <shawnguo@xxxxxxxxxx>; Fabio > Estevam <fabio.estevam@xxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; > Mark Rutland <mark.rutland@xxxxxxx>; Vladimir Zapolskiy > <vladimir_zapolskiy@xxxxxxxxxx>; , linux-arm-kernel@xxxxxxxxxxxxxxxxxxx, > linux-mediatek@xxxxxxxxxxxxxxxxxxx, srv_heupstream <linux-arm- > kernel@xxxxxxxxxxxxxxxxxxx>; Devicetree List <devicetree@xxxxxxxxxxxxxxx>; > dl-linux-imx <linux-imx@xxxxxxx> > Subject: RE: [PATCH v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU > channel support > > > -----Original Message----- > > From: Jassi Brar [mailto:jassisinghbrar@xxxxxxxxx] > > Sent: Friday, July 27, 2018 2:47 PM > > To: A.s. Dong <aisheng.dong@xxxxxxx> > > Cc: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>; , Sascha Hauer > > <kernel@xxxxxxxxxxxxxx>; Shawn Guo <shawnguo@xxxxxxxxxx>; Fabio > > Estevam <fabio.estevam@xxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; > > Mark Rutland <mark.rutland@xxxxxxx>; Vladimir Zapolskiy > > <vladimir_zapolskiy@xxxxxxxxxx>; , > > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx, > > linux-mediatek@xxxxxxxxxxxxxxxxxxx, srv_heupstream <linux-arm- > > kernel@xxxxxxxxxxxxxxxxxxx>; Devicetree List > > <devicetree@xxxxxxxxxxxxxxx>; dl-linux-imx <linux-imx@xxxxxxx> > > 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@xxxxxxx> wrote: > > >> -----Original Message----- > > >> From: Jassi Brar [mailto:jassisinghbrar@xxxxxxxxx] > > >> Sent: Friday, July 27, 2018 12:56 PM > > >> To: A.s. Dong <aisheng.dong@xxxxxxx> > > >> Cc: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>; , Sascha Hauer > > >> <kernel@xxxxxxxxxxxxxx>; Shawn Guo <shawnguo@xxxxxxxxxx>; Fabio > > >> Estevam <fabio.estevam@xxxxxxx>; Rob Herring > <robh+dt@xxxxxxxxxx>; > > >> Mark Rutland <mark.rutland@xxxxxxx>; Vladimir Zapolskiy > > >> <vladimir_zapolskiy@xxxxxxxxxx>; , > > >> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx, > > >> linux-mediatek@xxxxxxxxxxxxxxxxxxx, srv_heupstream <linux-arm- > > >> kernel@xxxxxxxxxxxxxxxxxxx>; Devicetree List > > >> <devicetree@xxxxxxxxxxxxxxx>; dl-linux-imx <linux-imx@xxxxxxx> > > >> 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@xxxxxxx> > 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&data=02%7C01%7Caisheng.dong%40nxp.com%7Cb728 > > >> 16362983 > > >> > > > >> > > 4208f9e908d5f37d3e00%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0% > > >> 7C6366 > > >> > > > >> > > > 82641593785009&sdata=VsG0oXxEObPRwT5KVss2eZthSTMTR2%2BMrv > > >> PqhDUpPGU > > >> > %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: > > > > > > 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&data=02%7C01%7Caisheng.dong%40nxp.com%7Cd2b1 > > 23b89dae > > > > > 4a00cb7108d5f38cb611%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1% > > 7C6366 > > > > > > 82708036553137&sdata=4SMUpH%2FO9MWArC%2BjbPy%2BrbNAqUla > > o9IezKUi7UX > > > gIyQ%3D&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&data=02%7C01%7Cais > > > heng.dong%40nxp.com%7Cd2b123b89dae4a00cb7108d5f38cb611%7C686ea1 > > > d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C636682708036553137&sdat > > > a=eUd75VnwEs44ZoVV0n7R1G3UfAH%2B%2BpeoGns7Mq5UBN8%3D& > > 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&data=02%7C0 > > >> >> > > > >> >> > > >> > > > 1%7Caisheng.dong%40nxp.com%7Cef349d7c9caf46c4260008d5f30e9ef5%7C6 > > >> >> 86ea1 > > >> >> > > > >> >> > > >> > > > d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636682166494860332&sdat > > >> >> a=54rE > > >> >> > > > >> >> > > >> > > > iDm%2BGD6EY8NE64ck1LXVGoUCtWYrHXPoWqcujUo%3D&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. ��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
