Hi Anson, Leonard,

On 19-09-30 07:42, Anson Huang wrote:

Hi, Leonard

quoted

On 2019-09-27 4:20 AM, Anson Huang wrote:

quoted

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?

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.

Regards,
  Marco

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

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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