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

[Anson Huang <anson.huang@nxp.com>]

Hi, Leonard

> 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?

> 
> > Adding a special api shouldn't be the right fix. Imagine if someone
> > (not a nxp-developer) wants to add a new driver. How could he be
> > expected to know which api he should use. The better abbroach would be
> > to fix the scu-fw instead of adding quirks..

Yes, fixing SCU FW is the best solution, but we have talked to SCU FW owner, the SCU
FW released has been finalized, so the API implementation can NOT be changed, but
they will pay attention to this issue for new added APIs later. That means the number
of APIs having this issue a very limited. 

> 
> Right now developers who want to make SCFW calls in upstream need to
> define the message struct in their driver based on protocol documentation.
> This includes:
> 
> * Binary layout of the message (a packed struct)
> * If the message has a response (already a bool flag)
> * If an error code is returned (this patch adds support for it)
> 
> Since callers are already exposed to the binary protocol exposing them to
> minor quirks of the calling convention also seems reasonable. Having the
> low-level IPC code peek at message IDs seems like a hack; this belong at a
> slightly higher level.

A little confused, so what you suggested is to add make the imx_scu_call_rpc()
becomes the "slightly higher level" API, then in this API, check the message IDs
to decide whether to return error value, then calls a new API which will have
the low-level IPC code, the this new API will have a flag passed from imx_scu_call_rpc()
function, am I right?

Anson