From mboxrd@z Thu Jan 1 00:00:00 1970 From: Oleksij Rempel Subject: Re: [PATCH v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU channel support Date: Mon, 30 Jul 2018 09:35:43 +0200 Message-ID: References: <20180726065331.6186-1-o.rempel@pengutronix.de> <20180726065331.6186-4-o.rempel@pengutronix.de> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5599432849341716580==" Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Jassi Brar , "A.s. Dong" Cc: Mark Rutland , Devicetree List , Rob Herring , ", linux-arm-kernel"@lists.infradead.org, srv_heupstream , ", Sascha Hauer" , Fabio Estevam , linux-mediatek@lists.infradead.org, Shawn Guo , Vladimir Zapolskiy , dl-linux-imx List-Id: linux-mediatek@lists.infradead.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --===============5599432849341716580== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="P0zko6jFhNP8z1cNJ6tfSmqdR1iaw0ok9" This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --P0zko6jFhNP8z1cNJ6tfSmqdR1iaw0ok9 Content-Type: multipart/mixed; boundary="VpFWl5GtRBYIKYKDR11PanKckHsI4adPs"; protected-headers="v1" From: Oleksij Rempel To: Jassi Brar , "A.s. Dong" Cc: ", Sascha Hauer" , Shawn Guo , Fabio Estevam , Rob Herring , Mark Rutland , Vladimir Zapolskiy , ", linux-arm-kernel"@lists.infradead.org, linux-mediatek@lists.infradead.org, srv_heupstream , Devicetree List , dl-linux-imx Message-ID: Subject: Re: [PATCH v7 3/6] dt-bindings: mailbox: imx-mu: add generic MU channel support References: <20180726065331.6186-1-o.rempel@pengutronix.de> <20180726065331.6186-4-o.rempel@pengutronix.de> In-Reply-To: --VpFWl5GtRBYIKYKDR11PanKckHsI4adPs Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable Hi Aishen, Jassie, i'm lost in this discussion. Please, as soon as I need to add some changes to my patches, notify me. Preferably on top for email. On 28.07.2018 15:09, Jassi Brar wrote: > On Fri, Jul 27, 2018 at 2:12 PM, A.s. Dong wrote= : >=20 >=20 >>>>>>> >>>>>>> MU has 4 pairs of TX_Reg + TX_IRQ, and 4 pairs of RX_Reg + RX_IRQ= =2E >>>>>>> (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 correspondin= g >>>>>>> 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= =2E >>>> 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 beca= use >>> that is the job of upper protocol layer. >>> >> >> Sorry I'm not quite familiar with USB device. >> > Which subsystem are you familiar with? Every stack is designed like > that -- controller drivers reflect the physical hardware, any > emulation/protocol is implemented in the user layer. >=20 >=20 >> Another reason is I doubt that we may never use per register mode in a= different register pair >> in the future. >> > You may never use, but if the driver reflects the controller as > precisely as it is, linux could support any usecase that some > baremetal code could support on your platform. > For example, you could support "normal" and "scu" mode over the 3-cell > mechanism I suggested. But your original proposal/assumption of > "scu-mode", failed immediately, hence Oleksij had to imlpement the > "normal" mode. >=20 >=20 >> For example: >> AP: >> node { >> ... >> // cell 0 meaning: 0: tx 1: rx cell1 meaning: channel id >> Mboxes =3D <&mbox 0 1 &mbox 1 2> >> Mbox-names =3D "tx", "rx"; >>> >> >> M4: >> node { >> ... >> Mboxes =3D <&mbox 0 2 &mbox 1 1> >> Mbox-names =3D "tx", "rx"; >>> >> This make things complicated and error prone as I said before. >> > I don't see how. >=20 >=20 >> But that's just my understanding and may overlook something, if you st= ill think >> we should do exactly as above, I will not against it because it does w= ork for M4 case. >> > Cool, done. >=20 >=20 >> Then the left question is how we handle SCU case? >> > Please point me to the code that you worry might not work. >=20 >=20 >> >> I'm a bit confusing.... >> The section 3.6 you pointed is the MHU register description. It does n= ot conflict >> with what I see from ARM doc center that each physical channel is unid= irectional. >> >> 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 RX and TX are not independent of each other, their priorities are > tied together in hardware. So it makes more sense (as compared to > declaring them independent) to club them together as one channel. > Whatever example you take - MHU or TI - you won't find a driver that > implements a virtual mode like "scu-mode". >=20 >=20 >>>> >>>> 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 o= ne >>>> time As specified in reference manual. >>>> >>>> SCU does work that way, the only difference is it's using polling mo= de >>>> 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 ide= a than >>> passing packets via shared-memory? >>> >> >> You got me. :-) >> I've no idea about which one is better. >> > Passing data directly via controller makes sense only when your > protocol defines fixed length packets that fit into registers (usually > FIFOs) or there is no shared memory between the processors. > Otherwise, you write a data packet at some located in shmem, and > provide its start and size along with the code of expected operation > on it, over the mailbox. Please refer to "Passing frame information" > para of page-2743 of your manual. > That is another reason the SCU implementation is broken - it has > variable length packets and yet use registers to pass them around. >=20 >=20 >> The problem is SCU firmware is already >> there passing packets through data registers, we have no way to change= it. >> > OK. But what is SCU exactly? Part of ATF? Do you get some binary from > third party? If the last, you shouldn't be paying hsit for such a > design. >=20 >=20 >>> >>> The hardware has 8 unidirectional channels. But your protocol (SCU >>> implementation) assumes there is one _virtual_ channel that has 4 reg= isters >>> and 1/0 irq --- which is not true. Instead of fixing the assumption i= n 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... >> > What you call "suggested to use in hardware reference manual" is just > 1/5 examples of usecase. > And SCU is not even doing that correctly (it uses variable length > packet, unlike the fixed 4-words suggestion). >=20 > A manual can suggest multiple ways of implementing a usecase. It is > our job to chose the best one. >=20 --VpFWl5GtRBYIKYKDR11PanKckHsI4adPs-- --P0zko6jFhNP8z1cNJ6tfSmqdR1iaw0ok9 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAEBCAAdFiEEpENFL0P3hvQ7p0DDdQOiSHVI77QFAltev88ACgkQdQOiSHVI 77Qw/Qf/Ue3D29j4fDaNE4Mzy++XEZ75T2Uu2lx+1q3ePWx8PdB4/qA4gEwIJweO DsBMP/KmCK/m8Gle/MkkeyUEBeX1HN7urZduQTvDV62jlhBGuuh4xb7Cd4j876NY m6yC6NFXGc/biVrRUalOFW1NitbejOFjRSrjaruNGbOFiL286dUuBEaKpHQPjO1f LHq4j9ADXY5wzB2T6h4t4RYJI1t7p/k97CFW98Herf1lvVqPdFEo9fkOyaAOQOQd 7rnUWLxviSRu1LPRl/bzoyUp45+DXnpTZwPLU0FApV8HJy7Y2AxDvvIWdwK/fafy 33xWZH7kDnNtBu7+n/YYvgqY/xtcfQ== =Wlau -----END PGP SIGNATURE----- --P0zko6jFhNP8z1cNJ6tfSmqdR1iaw0ok9-- --===============5599432849341716580== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel --===============5599432849341716580==--