Hi, Marco

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

quoted

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.

quoted

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

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