On 27.09.2019 04:20, Anson Huang wrote:

quoted

On 2019-09-26 1:06 PM, Marco Felsch wrote:

quoted

On 19-09-26 08:03, Anson Huang wrote:

quoted

quoted

On 19-09-25 18:07, Anson Huang wrote:

quoted

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 },

quoted

+	{ .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.

quoted

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.

quoted

quoted

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?

quoted

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?

No, I mean there should be no loop enumerating svc/func ids: *the caller 
should know* that it's calling a func which doesn't return an error code 
and call a different variant of imx_scu_call_rpc

Maybe add an internal __imx_scu_call_rpc_flags and turn the current 
imx_scu_call_rpc into a wrapper.

--
Regards,
Leonard

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel