Re: [PATCH V4 3/5] mailbox: imx: add imx mu support

[Jassi Brar]

On Wed, Jul 11, 2018 at 6:28 PM, A.s. Dong <aisheng.dong@xxxxxxx> wrote:
> Hi Jassi,
>
>> -----Original Message-----
>> From: Jassi Brar [mailto:jassisinghbrar@xxxxxxxxx]
>> Sent: Wednesday, July 11, 2018 6:44 PM
>> To: A.s. Dong <aisheng.dong@xxxxxxx>
>> Cc: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>; linux-arm-
>> kernel@xxxxxxxxxxxxxxxxxxx; dongas86@xxxxxxxxx; linux-
>> kernel@xxxxxxxxxxxxxxx; Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>; dl-
>> linux-imx <linux-imx@xxxxxxx>; kernel@xxxxxxxxxxxxxx; Fabio Estevam
>> <fabio.estevam@xxxxxxx>; shawnguo@xxxxxxxxxx
>> Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
>>
>> On Wed, Jul 11, 2018 at 4:07 PM, A.s. Dong <aisheng.dong@xxxxxxx> wrote:
>> >
>> > > -----Original Message-----
>> > > From: Sascha Hauer [mailto:s.hauer@xxxxxxxxxxxxxx]
>> > > Sent: Wednesday, July 11, 2018 3:55 PM
>> > > To: A.s. Dong <aisheng.dong@xxxxxxx>
>> > > Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; dongas86@xxxxxxxxx; Jassi
>> > > Brar <jassisinghbrar@xxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
>> > > Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>; dl-linux-imx
>> > > <linux-imx@xxxxxxx>; kernel@xxxxxxxxxxxxxx; Fabio Estevam
>> > > <fabio.estevam@xxxxxxx>; shawnguo@xxxxxxxxxx
>> > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
>> > >
>> > > On Wed, Jul 11, 2018 at 07:29:38AM +0000, A.s. Dong wrote:
>> > > > Hi Sascha,
>> > > >
>> > > > > -----Original Message-----
>> > > > > From: Sascha Hauer [mailto:s.hauer@xxxxxxxxxxxxxx]
>> > > > > Sent: Tuesday, July 10, 2018 10:20 PM
>> > > > > To: A.s. Dong <aisheng.dong@xxxxxxx>
>> > > > > Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; dongas86@xxxxxxxxx;
>> > > > > Jassi Brar <jassisinghbrar@xxxxxxxxx>;
>> > > > > linux-kernel@xxxxxxxxxxxxxxx; Oleksij Rempel
>> > > > > <o.rempel@xxxxxxxxxxxxxx>; dl-linux-imx <linux-imx@xxxxxxx>;
>> > > > > kernel@xxxxxxxxxxxxxx; Fabio Estevam <fabio.estevam@xxxxxxx>;
>> > > > > shawnguo@xxxxxxxxxx
>> > > > > Subject: Re: [PATCH V4 3/5] mailbox: imx: add imx mu support
>> > > > >
>> > > > > Hi,
>> > > > >
>> > > > > On Sun, Jul 08, 2018 at 10:56:55PM +0800, Dong Aisheng wrote:
>> > > > > > This is used for i.MX multi core communication.
>> > > > > > e.g. A core to SCU firmware(M core) on MX8.
>> > > > > >
>> > > > > > Tx is using polling mode while Rx is interrupt driven and
>> > > > > > schedule a hrtimer to receive remain words if have more than
>> > > > > > 4 words.
>> > > > >
>> > > > > You told us that using interrupts is not possible due to
>> > > > > miserable performance, we then provided you a way with which you
>> could poll.
>> > > > > Why are you using interrupts now?
>> > > > >
>> > > >
>> > > > Because mailbox framework does not support sync rx now, I think we
>> > > > do not need to wait for that feature done first as it's
>> > > > independent and separate features of framework.
>> > >
>> > > You can wait forever for this feature, nobody will add it for you.
>> > > It's up to you to add support for that feature. Who else should add this
>> feature if not you?
>> > > And when will you add that feature if not now when you actually need it?
>> > > It is common practice that you adjust the frameworks to your needs
>> > > rather than working around them.
>> > >
>> >
>> > I'm willing to add it. Just because you said Jassi already had the
>> > idea on how to Implement it and does not add much complexity. So I just
>> want to see his patches.
>> > But if he did not work on it, I can also help on it.
>> >
>> I am not much aware of the history of this conversation... but it seems you
>> need to make use of mbox_chan_ops.peek_data().
>>
>> If not that, please let me know the requirement.
>>
>
> Thanks for the suggestion.
> It looks to me may work.
>
> From the definition, it seems it's used to pull data from remote side.
> /**
>  * mbox_client_peek_data - A way for client driver to pull data
>  *                      received from remote by the controller.
>  * @chan: Mailbox channel assigned to this client.
>  *
>  * A poke to controller driver for any received data.
>  * The data is actually passed onto client via the
>  * mbox_chan_received_data()
>  * The call can be made from atomic context, so the controller's
>  * implementation of peek_data() must not sleep.
>  *
>  * Return: True, if controller has, and is going to push after this,
>  *          some data.
>  *         False, if controller doesn't have any data to be read.
>  */
> bool mbox_client_peek_data(struct mbox_chan *chan)
> {
>         if (chan->mbox->ops->peek_data)
>                 return chan->mbox->ops->peek_data(chan);
>
>         return false;
> }
> EXPORT_SYMBOL_GPL(mbox_client_peek_data);
> But it seems most users in kernel simply implement it as a data available
> Checking rather than receiving it.
> See:
> drivers/mailbox/ti-msgmgr.c
> drivers/mailbox/mailbox-altera.c
>
> Only bcm uses it to receive data.
> drivers/mailbox/bcm-flexrm-mailbox.c
>
> For our requirement, we want to implement sync receiving protocol like:
> Sc_call_rpc()
> {
>         mbox_send_message(chan, msg)
>         If (!no_resp)
>                 // rx also stored in msg
>                 mbox_receive_msg_in_polling(chan, msg);
>         mbox_client_txdone();
> }
>
> If using peek_data, it can be:
> Sc_call_rpc()
> {
>         mbox_send_message(chan, msg)
>         If (!no_resp)
>                 // rx also stored in msg
>                 Mbox_client_peek_data(chan);
>
Yes, and you may want to loop for a certain amount of time if peek
keeps returning false.

>         mbox_client_txdone();
> }
>
> And for mu controller driver .peek_data():
> imx_mu_peek_data(chan)
> {
>         // get first word and parse data size
>         imx_mu_receive_msg(&mu->chans, 0, mu->msg);
>
>         raw_data = (u8 *)mu->msg;
>         size = raw_data[1];
>
>         // receive rest of them
>         for (i = 1; i < size; i++) {
>                 ret = imx_mu_receive_msg(&mu->chans, i % 4, mu->msg + i);
>                 if (ret)
>                         return false;
>         }
>
>         mbox_chan_received_data(&mu->chans, (void *)mu->msg);
>
Not sure how your controller works. But the peek() callback only
_checks_ if there is some data available to be read. Please note that
peek() can not sleep.
So if the data fetching doesn't sleep you can do that here, otherwise
peek has to schedule the actual fetching of data from remote and
providing to the client via mbox_chan_received_data.

-jassi