RE: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs

[Anson Huang]

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.

Anson