* RE: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
@ 2019-09-27 1:20 ` Anson Huang
0 siblings, 0 replies; 42+ messages in thread
From: Anson Huang @ 2019-09-27 1:20 UTC (permalink / raw)
To: Leonard Crestez, Marco Felsch, Aisheng Dong
Cc: shawnguo, s.hauer, linux-kernel, dl-linux-imx, kernel, festevam,
linux-arm-kernel
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
2019-09-27 1:20 ` Anson Huang
@ 2019-09-27 9:06 ` Marco Felsch
-1 siblings, 0 replies; 42+ messages in thread
From: Marco Felsch @ 2019-09-27 9:06 UTC (permalink / raw)
To: Anson Huang
Cc: Leonard Crestez, Aisheng Dong, shawnguo, s.hauer, kernel,
festevam, linux-arm-kernel, linux-kernel, dl-linux-imx
Hi Anson, Leonard,
On 19-09-27 01:20, Anson Huang wrote:
> 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?
How makes this things easier?
> > > 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 ;)
> > 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?
Regards,
Marco
> > 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
--
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 |
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
@ 2019-09-27 9:06 ` Marco Felsch
0 siblings, 0 replies; 42+ messages in thread
From: Marco Felsch @ 2019-09-27 9:06 UTC (permalink / raw)
To: Anson Huang
Cc: Aisheng Dong, shawnguo, s.hauer, linux-kernel, dl-linux-imx,
kernel, Leonard Crestez, festevam, linux-arm-kernel
Hi Anson, Leonard,
On 19-09-27 01:20, Anson Huang wrote:
> 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?
How makes this things easier?
> > > 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 ;)
> > 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?
Regards,
Marco
> > 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
--
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
^ permalink raw reply [flat|nested] 42+ messages in thread
* RE: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
2019-09-27 9:06 ` Marco Felsch
@ 2019-09-27 9:27 ` Anson Huang
-1 siblings, 0 replies; 42+ messages in thread
From: Anson Huang @ 2019-09-27 9:27 UTC (permalink / raw)
To: Marco Felsch
Cc: Leonard Crestez, Aisheng Dong, shawnguo, s.hauer, kernel,
festevam, linux-arm-kernel, linux-kernel, dl-linux-imx
Hi, Marco
> On 19-09-27 01:20, Anson Huang wrote:
> > 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?
>
> How makes this things easier?
I think it is just for making a better SW layer.
>
> > > > 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 ;)
We can't say it is a bug, the SCU FW owner design it in this way, there are
some void function in SCU FW API, and once there is response data from SCU,
that means the API call is successful, and void function does NOT have a return
value for caller.
So it is just current SCU IPC driver in kernel NOT 100% matching SCU FW, those
void function has such return value issue.
Anson.
^ permalink raw reply [flat|nested] 42+ messages in thread
* RE: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
@ 2019-09-27 9:27 ` Anson Huang
0 siblings, 0 replies; 42+ messages in thread
From: Anson Huang @ 2019-09-27 9:27 UTC (permalink / raw)
To: Marco Felsch
Cc: Aisheng Dong, shawnguo, s.hauer, linux-kernel, dl-linux-imx,
kernel, Leonard Crestez, festevam, linux-arm-kernel
Hi, Marco
> On 19-09-27 01:20, Anson Huang wrote:
> > 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?
>
> How makes this things easier?
I think it is just for making a better SW layer.
>
> > > > 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 ;)
We can't say it is a bug, the SCU FW owner design it in this way, there are
some void function in SCU FW API, and once there is response data from SCU,
that means the API call is successful, and void function does NOT have a return
value for caller.
So it is just current SCU IPC driver in kernel NOT 100% matching SCU FW, those
void function has such return value issue.
Anson.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
2019-09-27 9:06 ` Marco Felsch
@ 2019-09-27 11:22 ` Leonard Crestez
-1 siblings, 0 replies; 42+ messages in thread
From: Leonard Crestez @ 2019-09-27 11:22 UTC (permalink / raw)
To: Marco Felsch, Anson Huang
Cc: Aisheng Dong, shawnguo, s.hauer, kernel, festevam,
linux-arm-kernel, linux-kernel, dl-linux-imx
On 27.09.2019 12:06, Marco Felsch wrote:
> Hi Anson, Leonard,
>
> On 19-09-27 01:20, Anson Huang wrote:
>> 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.
>>
>>>> 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 ;)
>>> 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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
@ 2019-09-27 11:22 ` Leonard Crestez
0 siblings, 0 replies; 42+ messages in thread
From: Leonard Crestez @ 2019-09-27 11:22 UTC (permalink / raw)
To: Marco Felsch, Anson Huang
Cc: Aisheng Dong, shawnguo, s.hauer, linux-kernel, dl-linux-imx,
kernel, festevam, linux-arm-kernel
On 27.09.2019 12:06, Marco Felsch wrote:
> Hi Anson, Leonard,
>
> On 19-09-27 01:20, Anson Huang wrote:
>> 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.
>>
>>>> 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 ;)
>>> 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
^ permalink raw reply [flat|nested] 42+ messages in thread
* RE: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
2019-09-27 11:22 ` Leonard Crestez
@ 2019-09-29 1:12 ` Anson Huang
-1 siblings, 0 replies; 42+ messages in thread
From: Anson Huang @ 2019-09-29 1:12 UTC (permalink / raw)
To: Leonard Crestez, Marco Felsch
Cc: Aisheng Dong, shawnguo, s.hauer, kernel, festevam,
linux-arm-kernel, linux-kernel, dl-linux-imx
Hi, Leonard/Marco
I think we should get aligned first, my original thought is to let SCU API caller NOT aware of those special APIs, so have to do the special handling in imx_scu_call_rpc(). And the short loop check has to be used which would impact the performance a little bit I think. But Leonard stated the caller should know the SCU FW API's usage, if so, then I think the special callers can just skip the return value check, adding a comment to describe the reason, would it be much more easier than changing the imx_scu_call_rpc()? Or any other suggestion?
Anson
> On 27.09.2019 12:06, Marco Felsch wrote:
> > Hi Anson, Leonard,
> >
> > On 19-09-27 01:20, Anson Huang wrote:
> >> 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.
> >>
> >>>> 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 ;)
>
> >>> 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
^ permalink raw reply [flat|nested] 42+ messages in thread
* RE: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
@ 2019-09-29 1:12 ` Anson Huang
0 siblings, 0 replies; 42+ messages in thread
From: Anson Huang @ 2019-09-29 1:12 UTC (permalink / raw)
To: Leonard Crestez, Marco Felsch
Cc: Aisheng Dong, shawnguo, s.hauer, linux-kernel, dl-linux-imx,
kernel, festevam, linux-arm-kernel
Hi, Leonard/Marco
I think we should get aligned first, my original thought is to let SCU API caller NOT aware of those special APIs, so have to do the special handling in imx_scu_call_rpc(). And the short loop check has to be used which would impact the performance a little bit I think. But Leonard stated the caller should know the SCU FW API's usage, if so, then I think the special callers can just skip the return value check, adding a comment to describe the reason, would it be much more easier than changing the imx_scu_call_rpc()? Or any other suggestion?
Anson
> On 27.09.2019 12:06, Marco Felsch wrote:
> > Hi Anson, Leonard,
> >
> > On 19-09-27 01:20, Anson Huang wrote:
> >> 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.
> >>
> >>>> 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 ;)
>
> >>> 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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
2019-09-27 1:20 ` Anson Huang
@ 2019-09-27 11:16 ` Leonard Crestez
-1 siblings, 0 replies; 42+ messages in thread
From: Leonard Crestez @ 2019-09-27 11:16 UTC (permalink / raw)
To: Anson Huang, Marco Felsch, Aisheng Dong
Cc: shawnguo, s.hauer, kernel, festevam, linux-arm-kernel,
linux-kernel, dl-linux-imx
On 27.09.2019 04:20, Anson Huang wrote:
>> 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?
>> 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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
@ 2019-09-27 11:16 ` Leonard Crestez
0 siblings, 0 replies; 42+ messages in thread
From: Leonard Crestez @ 2019-09-27 11:16 UTC (permalink / raw)
To: Anson Huang, Marco Felsch, Aisheng Dong
Cc: shawnguo, s.hauer, linux-kernel, dl-linux-imx, kernel, festevam,
linux-arm-kernel
On 27.09.2019 04:20, Anson Huang wrote:
>> 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?
>> 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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
2019-09-27 1:20 ` Anson Huang
@ 2019-09-30 7:28 ` Leonard Crestez
-1 siblings, 0 replies; 42+ messages in thread
From: Leonard Crestez @ 2019-09-30 7:28 UTC (permalink / raw)
To: Anson Huang, Marco Felsch, Aisheng Dong
Cc: shawnguo, s.hauer, kernel, festevam, linux-arm-kernel,
linux-kernel, dl-linux-imx
On 2019-09-27 4:20 AM, Anson Huang wrote:
>> 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?
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.
>>> 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?
No, I am saying that the caller (button driver) should be responsible
for calling with a special flag which states that the RPC call.
In internal tree this is handled inside the machine-generated function
calls, right? These are mostly skipped in upstream but maybe for these
particular calls we can manually add wrappers inside
"drivers/firmware/imx/misc.c".
And even if the functions "return void" from a SCFW perspective it still
makes sense to return general kernel errors, for example from mailbox.
--
Regards,
Leonard
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
@ 2019-09-30 7:28 ` Leonard Crestez
0 siblings, 0 replies; 42+ messages in thread
From: Leonard Crestez @ 2019-09-30 7:28 UTC (permalink / raw)
To: Anson Huang, Marco Felsch, Aisheng Dong
Cc: shawnguo, s.hauer, linux-kernel, dl-linux-imx, kernel, festevam,
linux-arm-kernel
On 2019-09-27 4:20 AM, Anson Huang wrote:
>> 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?
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.
>>> 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?
No, I am saying that the caller (button driver) should be responsible
for calling with a special flag which states that the RPC call.
In internal tree this is handled inside the machine-generated function
calls, right? These are mostly skipped in upstream but maybe for these
particular calls we can manually add wrappers inside
"drivers/firmware/imx/misc.c".
And even if the functions "return void" from a SCFW perspective it still
makes sense to return general kernel errors, for example from mailbox.
--
Regards,
Leonard
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 42+ messages in thread
* RE: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
2019-09-30 7:28 ` Leonard Crestez
@ 2019-09-30 7:42 ` Anson Huang
-1 siblings, 0 replies; 42+ messages in thread
From: Anson Huang @ 2019-09-30 7:42 UTC (permalink / raw)
To: Leonard Crestez, Marco Felsch, Aisheng Dong
Cc: shawnguo, s.hauer, kernel, festevam, linux-arm-kernel,
linux-kernel, dl-linux-imx
Hi, Leonard
> On 2019-09-27 4:20 AM, Anson Huang wrote:
> >> 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?
>
> 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.
Anson
^ permalink raw reply [flat|nested] 42+ messages in thread
* RE: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
@ 2019-09-30 7:42 ` Anson Huang
0 siblings, 0 replies; 42+ messages in thread
From: Anson Huang @ 2019-09-30 7:42 UTC (permalink / raw)
To: Leonard Crestez, Marco Felsch, Aisheng Dong
Cc: shawnguo, s.hauer, linux-kernel, dl-linux-imx, kernel, festevam,
linux-arm-kernel
Hi, Leonard
> On 2019-09-27 4:20 AM, Anson Huang wrote:
> >> 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?
>
> 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.
Anson
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 42+ messages in thread
* RE: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
2019-09-30 7:42 ` Anson Huang
@ 2019-09-30 7:54 ` Anson Huang
-1 siblings, 0 replies; 42+ messages in thread
From: Anson Huang @ 2019-09-30 7:54 UTC (permalink / raw)
To: Leonard Crestez, Marco Felsch, Aisheng Dong
Cc: shawnguo, s.hauer, kernel, festevam, linux-arm-kernel,
linux-kernel, dl-linux-imx
Hi, Leonard
> > On 2019-09-27 4:20 AM, Anson Huang wrote:
> > >> 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?
> >
> > 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.
Correct one thing, no need to change the parameter of imx_sc_rpc_call(), just add
another API using the flag as parameter, and imx_sc_rpc_call() calls the new API.
Anson
>
> If no one opposes this approach, I will redo the patch together with the
> button driver and uid driver after holiday.
>
> Anson
^ permalink raw reply [flat|nested] 42+ messages in thread
* RE: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
@ 2019-09-30 7:54 ` Anson Huang
0 siblings, 0 replies; 42+ messages in thread
From: Anson Huang @ 2019-09-30 7:54 UTC (permalink / raw)
To: Leonard Crestez, Marco Felsch, Aisheng Dong
Cc: shawnguo, s.hauer, linux-kernel, dl-linux-imx, kernel, festevam,
linux-arm-kernel
Hi, Leonard
> > On 2019-09-27 4:20 AM, Anson Huang wrote:
> > >> 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?
> >
> > 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.
Correct one thing, no need to change the parameter of imx_sc_rpc_call(), just add
another API using the flag as parameter, and imx_sc_rpc_call() calls the new API.
Anson
>
> If no one opposes this approach, I will redo the patch together with the
> button driver and uid driver after holiday.
>
> Anson
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
2019-09-30 7:42 ` Anson Huang
@ 2019-09-30 8:14 ` Marco Felsch
-1 siblings, 0 replies; 42+ messages in thread
From: Marco Felsch @ 2019-09-30 8:14 UTC (permalink / raw)
To: Anson Huang
Cc: Leonard Crestez, Aisheng Dong, shawnguo, s.hauer, linux-kernel,
dl-linux-imx, kernel, festevam, linux-arm-kernel
Hi Anson, Leonard,
On 19-09-30 07:42, Anson Huang wrote:
> Hi, Leonard
>
> > On 2019-09-27 4:20 AM, Anson Huang wrote:
> > >> 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?
> >
> > 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 |
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
@ 2019-09-30 8:14 ` Marco Felsch
0 siblings, 0 replies; 42+ messages in thread
From: Marco Felsch @ 2019-09-30 8:14 UTC (permalink / raw)
To: Anson Huang
Cc: Aisheng Dong, festevam, s.hauer, linux-kernel, dl-linux-imx,
kernel, Leonard Crestez, shawnguo, linux-arm-kernel
Hi Anson, Leonard,
On 19-09-30 07:42, Anson Huang wrote:
> Hi, Leonard
>
> > On 2019-09-27 4:20 AM, Anson Huang wrote:
> > >> 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?
> >
> > 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
^ permalink raw reply [flat|nested] 42+ messages in thread
* RE: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
2019-09-30 8:14 ` Marco Felsch
@ 2019-09-30 8:32 ` Anson Huang
-1 siblings, 0 replies; 42+ messages in thread
From: Anson Huang @ 2019-09-30 8:32 UTC (permalink / raw)
To: Marco Felsch
Cc: Leonard Crestez, Aisheng Dong, shawnguo, s.hauer, linux-kernel,
dl-linux-imx, kernel, festevam, linux-arm-kernel
Hi, Marco
> On 19-09-30 07:42, Anson Huang wrote:
> > Hi, Leonard
> >
> > > On 2019-09-27 4:20 AM, Anson Huang wrote:
> > > >> 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?
> > >
> > > 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.
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
^ permalink raw reply [flat|nested] 42+ messages in thread
* RE: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
@ 2019-09-30 8:32 ` Anson Huang
0 siblings, 0 replies; 42+ messages in thread
From: Anson Huang @ 2019-09-30 8:32 UTC (permalink / raw)
To: Marco Felsch
Cc: Aisheng Dong, festevam, s.hauer, linux-kernel, dl-linux-imx,
kernel, Leonard Crestez, shawnguo, linux-arm-kernel
Hi, Marco
> On 19-09-30 07:42, Anson Huang wrote:
> > Hi, Leonard
> >
> > > On 2019-09-27 4:20 AM, Anson Huang wrote:
> > > >> 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?
> > >
> > > 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.
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
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
2019-09-30 8:32 ` Anson Huang
@ 2019-09-30 10:02 ` Marco Felsch
-1 siblings, 0 replies; 42+ messages in thread
From: Marco Felsch @ 2019-09-30 10:02 UTC (permalink / raw)
To: Anson Huang
Cc: Aisheng Dong, festevam, s.hauer, linux-kernel, dl-linux-imx,
kernel, Leonard Crestez, shawnguo, linux-arm-kernel
Hi Anson,
On 19-09-30 08:32, Anson Huang wrote:
> Hi, Marco
>
> > On 19-09-30 07:42, Anson Huang wrote:
> > > Hi, Leonard
> > >
> > > > On 2019-09-27 4:20 AM, Anson Huang wrote:
> > > > >> 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?
> > > >
> > > > 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.
>
> 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;
Thanks for the declaration :)
> 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.
I see.
> With new approach, I can use below 2 flags, the ugly point is user need to know which API to call.
I don't see any improve using flags because the caller still needs to
know if the scu-fw works (sorry for that) correctly. So we should go to
adapt your approach to handle that within the core and improve the
caller usage.
What about this:
8<-------------------------------------------------------------------------------
diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-scu.c
index 04a24a863d6e..8f406a0784a4 100644
--- a/drivers/firmware/imx/imx-scu.c
+++ b/drivers/firmware/imx/imx-scu.c
@@ -184,6 +184,16 @@ int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, void *msg, bool have_resp)
/* response status is stored in hdr->func field */
hdr = msg;
ret = hdr->func;
+
+ /*
+ * Some special SCU firmware APIs do NOT have return value
+ * in hdr->func, but they do have response data, those special
+ * APIs are defined as void function in SCU firmware, so they
+ * should be treated as return success always.
+ */
+ if (hdr->func == IMX_SC_MISC_FUNC_UNIQUE_ID ||
+ hdr->func == IMX_SC_MISC_FUNC_GET_BUTTON_STATUS)
+ ret = 0;
}
out:
8<-------------------------------------------------------------------------------
As you and Leonard said, this scu-fw behaviour is intended. So this will
be not changed over the time else we need a scu-fw version check too.
Also as you said those special functions shouldn't be extended I think a
simple if-statement should work and no performance regressions are
expected.
Regards,
Marco
> +++ 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
>
--
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 |
^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
@ 2019-09-30 10:02 ` Marco Felsch
0 siblings, 0 replies; 42+ messages in thread
From: Marco Felsch @ 2019-09-30 10:02 UTC (permalink / raw)
To: Anson Huang
Cc: Aisheng Dong, shawnguo, s.hauer, linux-kernel, dl-linux-imx,
kernel, Leonard Crestez, festevam, linux-arm-kernel
Hi Anson,
On 19-09-30 08:32, Anson Huang wrote:
> Hi, Marco
>
> > On 19-09-30 07:42, Anson Huang wrote:
> > > Hi, Leonard
> > >
> > > > On 2019-09-27 4:20 AM, Anson Huang wrote:
> > > > >> 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?
> > > >
> > > > 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.
>
> 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;
Thanks for the declaration :)
> 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.
I see.
> With new approach, I can use below 2 flags, the ugly point is user need to know which API to call.
I don't see any improve using flags because the caller still needs to
know if the scu-fw works (sorry for that) correctly. So we should go to
adapt your approach to handle that within the core and improve the
caller usage.
What about this:
8<-------------------------------------------------------------------------------
diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-scu.c
index 04a24a863d6e..8f406a0784a4 100644
--- a/drivers/firmware/imx/imx-scu.c
+++ b/drivers/firmware/imx/imx-scu.c
@@ -184,6 +184,16 @@ int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, void *msg, bool have_resp)
/* response status is stored in hdr->func field */
hdr = msg;
ret = hdr->func;
+
+ /*
+ * Some special SCU firmware APIs do NOT have return value
+ * in hdr->func, but they do have response data, those special
+ * APIs are defined as void function in SCU firmware, so they
+ * should be treated as return success always.
+ */
+ if (hdr->func == IMX_SC_MISC_FUNC_UNIQUE_ID ||
+ hdr->func == IMX_SC_MISC_FUNC_GET_BUTTON_STATUS)
+ ret = 0;
}
out:
8<-------------------------------------------------------------------------------
As you and Leonard said, this scu-fw behaviour is intended. So this will
be not changed over the time else we need a scu-fw version check too.
Also as you said those special functions shouldn't be extended I think a
simple if-statement should work and no performance regressions are
expected.
Regards,
Marco
> +++ 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
>
--
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
^ permalink raw reply related [flat|nested] 42+ messages in thread
* RE: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
2019-09-30 10:02 ` Marco Felsch
@ 2019-10-07 1:21 ` Anson Huang
-1 siblings, 0 replies; 42+ messages in thread
From: Anson Huang @ 2019-10-07 1:21 UTC (permalink / raw)
To: Marco Felsch
Cc: Aisheng Dong, festevam, s.hauer, linux-kernel, dl-linux-imx,
kernel, Leonard Crestez, shawnguo, linux-arm-kernel
Hi, Marco
> On 19-09-30 08:32, Anson Huang wrote:
> > Hi, Marco
> >
> > > On 19-09-30 07:42, Anson Huang wrote:
> > > > Hi, Leonard
> > > >
> > > > > On 2019-09-27 4:20 AM, Anson Huang wrote:
> > > > > >> 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?
> > > > >
> > > > > 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.
> >
> > 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;
>
> Thanks for the declaration :)
>
> > 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.
>
> I see.
>
> > With new approach, I can use below 2 flags, the ugly point is user need to
> know which API to call.
>
> I don't see any improve using flags because the caller still needs to know if
> the scu-fw works (sorry for that) correctly. So we should go to adapt your
> approach to handle that within the core and improve the caller usage.
>
> What about this:
>
> 8<-------------------------------------------------------------------------------
>
> diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-
> scu.c index 04a24a863d6e..8f406a0784a4 100644
> --- a/drivers/firmware/imx/imx-scu.c
> +++ b/drivers/firmware/imx/imx-scu.c
> @@ -184,6 +184,16 @@ int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, void
> *msg, bool have_resp)
> /* response status is stored in hdr->func field */
> hdr = msg;
> ret = hdr->func;
> +
> + /*
> + * Some special SCU firmware APIs do NOT have return value
> + * in hdr->func, but they do have response data, those
> special
> + * APIs are defined as void function in SCU firmware, so they
> + * should be treated as return success always.
> + */
> + if (hdr->func == IMX_SC_MISC_FUNC_UNIQUE_ID ||
> + hdr->func == IMX_SC_MISC_FUNC_GET_BUTTON_STATUS)
> + ret = 0;
> }
>
> out:
> 8<-------------------------------------------------------------------------------
>
> As you and Leonard said, this scu-fw behaviour is intended. So this will be
> not changed over the time else we need a scu-fw version check too.
> Also as you said those special functions shouldn't be extended I think a
> simple if-statement should work and no performance regressions are
> expected.
>
I agree to just check the special APIs in the imx_scu_call_rpc() function, it can avoid calling
another function to check as my V1 patch did, also no need to add another API for users, so
that users no need to know which API to call. But I can NOT use the example you listed upper
directly, the return value from SCU FW could be an error value which is same as the hdr->func,
so I need to saved the original hdr->func and compare them, see below, please help review V2
patch, thanks.
38 + if (have_resp) {
39 sc_ipc->msg = msg;
40 + saved_svc = ((struct imx_sc_rpc_msg *)msg)->svc;
41 + saved_func = ((struct imx_sc_rpc_msg *)msg)->func;
42 + }
50 + /*
51 + * Some special SCU firmware APIs do NOT have return value
52 + * in hdr->func, but they do have response data, those special
53 + * APIs are defined as void function in SCU firmware, so they
54 + * should be treated as return success always.
55 + */
56 + if ((saved_svc == IMX_SC_RPC_SVC_MISC) &&
57 + (saved_func == IMX_SC_MISC_FUNC_UNIQUE_ID ||
58 + saved_func == IMX_SC_MISC_FUNC_GET_BUTTON_STATUS))
59 + ret = 0;
Anson
^ permalink raw reply [flat|nested] 42+ messages in thread
* RE: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
@ 2019-10-07 1:21 ` Anson Huang
0 siblings, 0 replies; 42+ messages in thread
From: Anson Huang @ 2019-10-07 1:21 UTC (permalink / raw)
To: Marco Felsch
Cc: Aisheng Dong, shawnguo, s.hauer, linux-kernel, dl-linux-imx,
kernel, Leonard Crestez, festevam, linux-arm-kernel
Hi, Marco
> On 19-09-30 08:32, Anson Huang wrote:
> > Hi, Marco
> >
> > > On 19-09-30 07:42, Anson Huang wrote:
> > > > Hi, Leonard
> > > >
> > > > > On 2019-09-27 4:20 AM, Anson Huang wrote:
> > > > > >> 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?
> > > > >
> > > > > 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.
> >
> > 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;
>
> Thanks for the declaration :)
>
> > 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.
>
> I see.
>
> > With new approach, I can use below 2 flags, the ugly point is user need to
> know which API to call.
>
> I don't see any improve using flags because the caller still needs to know if
> the scu-fw works (sorry for that) correctly. So we should go to adapt your
> approach to handle that within the core and improve the caller usage.
>
> What about this:
>
> 8<-------------------------------------------------------------------------------
>
> diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-
> scu.c index 04a24a863d6e..8f406a0784a4 100644
> --- a/drivers/firmware/imx/imx-scu.c
> +++ b/drivers/firmware/imx/imx-scu.c
> @@ -184,6 +184,16 @@ int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, void
> *msg, bool have_resp)
> /* response status is stored in hdr->func field */
> hdr = msg;
> ret = hdr->func;
> +
> + /*
> + * Some special SCU firmware APIs do NOT have return value
> + * in hdr->func, but they do have response data, those
> special
> + * APIs are defined as void function in SCU firmware, so they
> + * should be treated as return success always.
> + */
> + if (hdr->func == IMX_SC_MISC_FUNC_UNIQUE_ID ||
> + hdr->func == IMX_SC_MISC_FUNC_GET_BUTTON_STATUS)
> + ret = 0;
> }
>
> out:
> 8<-------------------------------------------------------------------------------
>
> As you and Leonard said, this scu-fw behaviour is intended. So this will be
> not changed over the time else we need a scu-fw version check too.
> Also as you said those special functions shouldn't be extended I think a
> simple if-statement should work and no performance regressions are
> expected.
>
I agree to just check the special APIs in the imx_scu_call_rpc() function, it can avoid calling
another function to check as my V1 patch did, also no need to add another API for users, so
that users no need to know which API to call. But I can NOT use the example you listed upper
directly, the return value from SCU FW could be an error value which is same as the hdr->func,
so I need to saved the original hdr->func and compare them, see below, please help review V2
patch, thanks.
38 + if (have_resp) {
39 sc_ipc->msg = msg;
40 + saved_svc = ((struct imx_sc_rpc_msg *)msg)->svc;
41 + saved_func = ((struct imx_sc_rpc_msg *)msg)->func;
42 + }
50 + /*
51 + * Some special SCU firmware APIs do NOT have return value
52 + * in hdr->func, but they do have response data, those special
53 + * APIs are defined as void function in SCU firmware, so they
54 + * should be treated as return success always.
55 + */
56 + if ((saved_svc == IMX_SC_RPC_SVC_MISC) &&
57 + (saved_func == IMX_SC_MISC_FUNC_UNIQUE_ID ||
58 + saved_func == IMX_SC_MISC_FUNC_GET_BUTTON_STATUS))
59 + ret = 0;
Anson
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
2019-10-07 1:21 ` Anson Huang
@ 2019-10-07 8:07 ` Marco Felsch
-1 siblings, 0 replies; 42+ messages in thread
From: Marco Felsch @ 2019-10-07 8:07 UTC (permalink / raw)
To: Anson Huang
Cc: Aisheng Dong, festevam, s.hauer, linux-kernel, dl-linux-imx,
kernel, Leonard Crestez, shawnguo, linux-arm-kernel
On 19-10-07 01:21, Anson Huang wrote:
> Hi, Marco
>
> > On 19-09-30 08:32, Anson Huang wrote:
> > > Hi, Marco
> > >
> > > > On 19-09-30 07:42, Anson Huang wrote:
> > > > > Hi, Leonard
> > > > >
> > > > > > On 2019-09-27 4:20 AM, Anson Huang wrote:
> > > > > > >> 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?
> > > > > >
> > > > > > 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.
> > >
> > > 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;
> >
> > Thanks for the declaration :)
> >
> > > 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.
> >
> > I see.
> >
> > > With new approach, I can use below 2 flags, the ugly point is user need to
> > know which API to call.
> >
> > I don't see any improve using flags because the caller still needs to know if
> > the scu-fw works (sorry for that) correctly. So we should go to adapt your
> > approach to handle that within the core and improve the caller usage.
> >
> > What about this:
> >
> > 8<-------------------------------------------------------------------------------
> >
> > diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-
> > scu.c index 04a24a863d6e..8f406a0784a4 100644
> > --- a/drivers/firmware/imx/imx-scu.c
> > +++ b/drivers/firmware/imx/imx-scu.c
> > @@ -184,6 +184,16 @@ int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, void
> > *msg, bool have_resp)
> > /* response status is stored in hdr->func field */
> > hdr = msg;
> > ret = hdr->func;
> > +
> > + /*
> > + * Some special SCU firmware APIs do NOT have return value
> > + * in hdr->func, but they do have response data, those
> > special
> > + * APIs are defined as void function in SCU firmware, so they
> > + * should be treated as return success always.
> > + */
> > + if (hdr->func == IMX_SC_MISC_FUNC_UNIQUE_ID ||
> > + hdr->func == IMX_SC_MISC_FUNC_GET_BUTTON_STATUS)
> > + ret = 0;
> > }
> >
> > out:
> > 8<-------------------------------------------------------------------------------
> >
> > As you and Leonard said, this scu-fw behaviour is intended. So this will be
> > not changed over the time else we need a scu-fw version check too.
> > Also as you said those special functions shouldn't be extended I think a
> > simple if-statement should work and no performance regressions are
> > expected.
> >
>
> I agree to just check the special APIs in the imx_scu_call_rpc() function, it can avoid calling
> another function to check as my V1 patch did, also no need to add another API for users, so
> that users no need to know which API to call. But I can NOT use the example you listed upper
> directly, the return value from SCU FW could be an error value which is same as the hdr->func,
I tought the SCU FW won't touch this field.
> so I need to saved the original hdr->func and compare them, see below, please help review V2
> patch, thanks.
I did :)
Regards,
Marco
>
> 38 + if (have_resp) {
> 39 sc_ipc->msg = msg;
> 40 + saved_svc = ((struct imx_sc_rpc_msg *)msg)->svc;
> 41 + saved_func = ((struct imx_sc_rpc_msg *)msg)->func;
> 42 + }
>
> 50 + /*
> 51 + * Some special SCU firmware APIs do NOT have return value
> 52 + * in hdr->func, but they do have response data, those special
> 53 + * APIs are defined as void function in SCU firmware, so they
> 54 + * should be treated as return success always.
> 55 + */
> 56 + if ((saved_svc == IMX_SC_RPC_SVC_MISC) &&
> 57 + (saved_func == IMX_SC_MISC_FUNC_UNIQUE_ID ||
> 58 + saved_func == IMX_SC_MISC_FUNC_GET_BUTTON_STATUS))
> 59 + ret = 0;
>
> Anson
--
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 |
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs
@ 2019-10-07 8:07 ` Marco Felsch
0 siblings, 0 replies; 42+ messages in thread
From: Marco Felsch @ 2019-10-07 8:07 UTC (permalink / raw)
To: Anson Huang
Cc: Aisheng Dong, shawnguo, s.hauer, linux-kernel, dl-linux-imx,
kernel, Leonard Crestez, festevam, linux-arm-kernel
On 19-10-07 01:21, Anson Huang wrote:
> Hi, Marco
>
> > On 19-09-30 08:32, Anson Huang wrote:
> > > Hi, Marco
> > >
> > > > On 19-09-30 07:42, Anson Huang wrote:
> > > > > Hi, Leonard
> > > > >
> > > > > > On 2019-09-27 4:20 AM, Anson Huang wrote:
> > > > > > >> 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?
> > > > > >
> > > > > > 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.
> > >
> > > 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;
> >
> > Thanks for the declaration :)
> >
> > > 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.
> >
> > I see.
> >
> > > With new approach, I can use below 2 flags, the ugly point is user need to
> > know which API to call.
> >
> > I don't see any improve using flags because the caller still needs to know if
> > the scu-fw works (sorry for that) correctly. So we should go to adapt your
> > approach to handle that within the core and improve the caller usage.
> >
> > What about this:
> >
> > 8<-------------------------------------------------------------------------------
> >
> > diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-
> > scu.c index 04a24a863d6e..8f406a0784a4 100644
> > --- a/drivers/firmware/imx/imx-scu.c
> > +++ b/drivers/firmware/imx/imx-scu.c
> > @@ -184,6 +184,16 @@ int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, void
> > *msg, bool have_resp)
> > /* response status is stored in hdr->func field */
> > hdr = msg;
> > ret = hdr->func;
> > +
> > + /*
> > + * Some special SCU firmware APIs do NOT have return value
> > + * in hdr->func, but they do have response data, those
> > special
> > + * APIs are defined as void function in SCU firmware, so they
> > + * should be treated as return success always.
> > + */
> > + if (hdr->func == IMX_SC_MISC_FUNC_UNIQUE_ID ||
> > + hdr->func == IMX_SC_MISC_FUNC_GET_BUTTON_STATUS)
> > + ret = 0;
> > }
> >
> > out:
> > 8<-------------------------------------------------------------------------------
> >
> > As you and Leonard said, this scu-fw behaviour is intended. So this will be
> > not changed over the time else we need a scu-fw version check too.
> > Also as you said those special functions shouldn't be extended I think a
> > simple if-statement should work and no performance regressions are
> > expected.
> >
>
> I agree to just check the special APIs in the imx_scu_call_rpc() function, it can avoid calling
> another function to check as my V1 patch did, also no need to add another API for users, so
> that users no need to know which API to call. But I can NOT use the example you listed upper
> directly, the return value from SCU FW could be an error value which is same as the hdr->func,
I tought the SCU FW won't touch this field.
> so I need to saved the original hdr->func and compare them, see below, please help review V2
> patch, thanks.
I did :)
Regards,
Marco
>
> 38 + if (have_resp) {
> 39 sc_ipc->msg = msg;
> 40 + saved_svc = ((struct imx_sc_rpc_msg *)msg)->svc;
> 41 + saved_func = ((struct imx_sc_rpc_msg *)msg)->func;
> 42 + }
>
> 50 + /*
> 51 + * Some special SCU firmware APIs do NOT have return value
> 52 + * in hdr->func, but they do have response data, those special
> 53 + * APIs are defined as void function in SCU firmware, so they
> 54 + * should be treated as return success always.
> 55 + */
> 56 + if ((saved_svc == IMX_SC_RPC_SVC_MISC) &&
> 57 + (saved_func == IMX_SC_MISC_FUNC_UNIQUE_ID ||
> 58 + saved_func == IMX_SC_MISC_FUNC_GET_BUTTON_STATUS))
> 59 + ret = 0;
>
> Anson
--
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
^ permalink raw reply [flat|nested] 42+ messages in thread