On 27.09.2019 12:06, Marco Felsch wrote:

Hi Anson, Leonard,

On 19-09-27 01:20, Anson Huang wrote:

quoted

Hi, Leonard

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.

quoted

quoted

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.

This means those APIs which already have this bug will not be fixed?
IMHO this sounds a bit weird since this is a changeable peace of code ;)

It's not a bug, it's a documented feature ;)

quoted

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)

Why should I specify if a error code is returned?

Because you're already defining the message struct and you're already 
specifying if a response is required.

The assumption is that anyone adding a SCFW call to a driver is already 
looking at SCFW documentation which describes the binary message format.

--
Regards,
Leonard

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