Message-ID: <DB3PR0402MB3916B2243D4B452B460EA892F5820@DB3PR0402MB3916.eurprd04.prod.outlook.com>
Date:   Mon, 30 Sep 2019 08:32:14 +0000
From:   Anson Huang <anson.huang@....com>
To:     Marco Felsch <m.felsch@...gutronix.de>
CC:     Leonard Crestez <leonard.crestez@....com>,
        Aisheng Dong <aisheng.dong@....com>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        dl-linux-imx <linux-imx@....com>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "festevam@...il.com" <festevam@...il.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: RE: [PATCH] firmware: imx: Skip return value check for some special
 SCU firmware APIs

Hi, Marco

> On 19-09-30 07:42, Anson Huang wrote:
> > Hi, Leonard
> >
> > > On 2019-09-27 4:20 AM, Anson Huang wrote:
> > > >> On 2019-09-26 1:06 PM, Marco Felsch wrote:
> > > >>> On 19-09-26 08:03, Anson Huang wrote:
> > > >>>>> On 19-09-25 18:07, Anson Huang wrote:
> > > >>>>>> The SCU firmware does NOT always have return value stored in
> > > >>>>>> message header's function element even the API has response
> > > >>>>>> data, those special APIs are defined as void function in SCU
> > > >>>>>> firmware, so they should be treated as return success always.
> > > >>>>>>
> > > >>>>>> +static const struct imx_sc_rpc_msg whitelist[] = {
> > > >>>>>> +	{ .svc = IMX_SC_RPC_SVC_MISC, .func =
> > > >>>>> IMX_SC_MISC_FUNC_UNIQUE_ID },
> > > >>>>>> +	{ .svc = IMX_SC_RPC_SVC_MISC, .func =
> > > >>>>>> +IMX_SC_MISC_FUNC_GET_BUTTON_STATUS }, };
> > > >>>>>
> > > >>>>> Is this going to be extended in the near future? I see some
> > > >>>>> upcoming problems here if someone uses a different
> > > >>>>> scu-fw<->kernel combination as nxp would suggest.
> > > >>>>
> > > >>>> Could be, but I checked the current APIs, ONLY these 2 will be
> > > >>>> used in Linux kernel, so I ONLY add these 2 APIs for now.
> > > >>>
> > > >>> Okay.
> > > >>>
> > > >>>> However, after rethink, maybe we should add another imx_sc_rpc
> > > >>>> API for those special APIs? To avoid checking it for all the
> > > >>>> APIs called which
> > > >> may impact some performance.
> > > >>>> Still under discussion, if you have better idea, please advise, thanks!
> > > >>
> > > >> My suggestion is to refactor the code and add a new API for the
> > > >> this "no error value" convention. Internally they can call a
> > > >> common function with flags.
> > > >
> > > > If I understand your point correctly, that means the loop check of
> > > > whether the API is with "no error value" for every API still NOT
> > > > be skipped, it is just refactoring the code, right?
> > >
> > > There would be no "loop" anywhere: the responsibility would fall on
> > > the call to call the right RPC function. In the current layering
> > > scheme (drivers -> RPC ->
> > > mailbox) the RPC layer treats all calls the same and it's up the the
> > > caller to provide information about calling convention.
> > >
> > > An example implementation:
> > > * Rename imx_sc_rpc_call to __imx_sc_rpc_call_flags
> > > * Make a tiny imx_sc_rpc_call wrapper which just converts
> > > resp/noresp to a flag
> > > * Make get button status call __imx_sc_rpc_call_flags with the
> > > _IMX_SC_RPC_NOERROR flag
> > >
> > > Hope this makes my suggestion clearer? Pushing this to the caller is
> > > a bit ugly but I think it's worth preserving the fact that the imx
> > > rpc core treats services in an uniform way.
> >
> > It is clear now, so essentially it is same as 2 separate APIs, still
> > need to change the button driver and uid driver to use the special
> > flag, meanwhile, need to change the third parament of imx_sc_rpc_call()
> from bool to u32.
> >
> > If no one opposes this approach, I will redo the patch together with
> > the button driver and uid driver after holiday.
> 
> As Ansons said that are two approaches and in both ways the caller needs to
> know if the error code is valid. Extending the flags seems better to me but it
> looks still not that good. One question, does the scu-fw set the error-msg to
> something? If not than why should we specify a flag or a other api?
> Nowadays the caller needs to know that the error-msg-field isn't set so if the
> caller sets the msg-packet to zero and fills the rpc-id the error-msg-field
> shouldn't be touched by the firmware. So it should be zero.

The flow are as below for those special APIs with response data but no return value from SCU FW:

1. caller sends msg with a header field and data field, the header field has svc ID and function ID;
2. SCU FW will service the caller and then clear the SVC ID before return, the response data will be
Put in msg data field, and if the APIs has return value, SCU FW will put the return value in function ID of msg;  

The caller has no chance to set the msg-packet to zero and rpc-id, it needs to pass correct rpc-id to SCU FW and
Get response data from SCU FW, and for those special APIs has function ID NOT over-written by SCU FW's return
Value, but the function ID is a unsigned int, and the SCU FW return value is also a unsigned int, so we have no
idea to separate them for no-return value API or error-return API.

With new approach, I can use below 2 flags, the ugly point is user need to know which API to call.

+++ b/include/linux/firmware/imx/ipc.h
@@ -35,6 +35,11 @@ struct imx_sc_rpc_msg {
        uint8_t func;
 };

+#define IMX_SC_RPC_HAVE_RESP   BIT(0) /* caller has response data */
+#define IMX_SC_RPC_NOERROR     BIT(1) /* caller has response data but no return value from SCU FW */
+
+int imx_scu_call_rpc_flags(struct imx_sc_ipc *ipc, void *msg, u32 flags);

Anson

Powered by blists - more mailing lists