| Message ID | 1570410959-32563-1-git-send-email-Anson.Huang@nxp.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 51f5afabc07a13e3d030076c772a1c36e1687b99 |
| Headers | show |
| Series | [V2] firmware: imx: Skip return value check for some special SCU firmware APIs | expand |
Hi Anson, On 19-10-07 09:15, 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. > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > --- > Changes since V1: > - Use direct API check instead of calling another function to check. > - This patch is based on https://patchwork.kernel.org/patch/11129553/ Thanks for this v2. It would be good to change the callers within this series. Regards, Marco > --- > drivers/firmware/imx/imx-scu.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-scu.c > index 869be7a..03b43b7 100644 > --- a/drivers/firmware/imx/imx-scu.c > +++ b/drivers/firmware/imx/imx-scu.c > @@ -162,6 +162,7 @@ static int imx_scu_ipc_write(struct imx_sc_ipc *sc_ipc, void *msg) > */ > int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, void *msg, bool have_resp) > { > + uint8_t saved_svc, saved_func; > struct imx_sc_rpc_msg *hdr; > int ret; > > @@ -171,8 +172,11 @@ int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, void *msg, bool have_resp) > mutex_lock(&sc_ipc->lock); > reinit_completion(&sc_ipc->done); > > - if (have_resp) > + if (have_resp) { > sc_ipc->msg = msg; > + saved_svc = ((struct imx_sc_rpc_msg *)msg)->svc; > + saved_func = ((struct imx_sc_rpc_msg *)msg)->func; > + } > sc_ipc->count = 0; > ret = imx_scu_ipc_write(sc_ipc, msg); > if (ret < 0) { > @@ -191,6 +195,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 ((saved_svc == IMX_SC_RPC_SVC_MISC) && > + (saved_func == IMX_SC_MISC_FUNC_UNIQUE_ID || > + saved_func == IMX_SC_MISC_FUNC_GET_BUTTON_STATUS)) > + ret = 0; > } > > out: > -- > 2.7.4 > > >
Hi, Marco > On 19-10-07 09:15, 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. > > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > > --- > > Changes since V1: > > - Use direct API check instead of calling another function to check. > > - This patch is based on > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc > > > hwork.kernel.org%2Fpatch%2F11129553%2F&data=02%7C01%7Canson. > huang% > > > 40nxp.com%7C2de0a6be69b74cc249ad08d74afc9730%7C686ea1d3bc2b4c6f > a92cd99 > > > c5c301635%7C0%7C0%7C637060321046247040&sdata=RMFAdLKGKb6 > mEdhycrzHX > > R03E6Qr5pWyRc8Zk6ErlBc%3D&reserved=0 > > Thanks for this v2. It would be good to change the callers within this series. NOT quite understand your point, the callers does NOT need to be changed, those 2 special APIs callers are already following the right way of calling the APIs. Anson
Hi Anson, On 19-10-08 00:48, Anson Huang wrote: > Hi, Marco > > > On 19-10-07 09:15, 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. > > > > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > > > --- > > > Changes since V1: > > > - Use direct API check instead of calling another function to check. > > > - This patch is based on > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc > > > > > hwork.kernel.org%2Fpatch%2F11129553%2F&data=02%7C01%7Canson. > > huang% > > > > > 40nxp.com%7C2de0a6be69b74cc249ad08d74afc9730%7C686ea1d3bc2b4c6f > > a92cd99 > > > > > c5c301635%7C0%7C0%7C637060321046247040&sdata=RMFAdLKGKb6 > > mEdhycrzHX > > > R03E6Qr5pWyRc8Zk6ErlBc%3D&reserved=0 > > > > Thanks for this v2. It would be good to change the callers within this series. > > NOT quite understand your point, the callers does NOT need to be changed, those > 2 special APIs callers are already following the right way of calling the APIs. Ah okay. I searched the 5.4-rc2 tag and found the soc_uid_show() as only user but this user sets the have_resp field to false. Is this intended? Regards, Marco > Anson
Hi, Marco > On 19-10-08 00:48, Anson Huang wrote: > > Hi, Marco > > > > > On 19-10-07 09:15, 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. > > > > > > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > > > > --- > > > > Changes since V1: > > > > - Use direct API check instead of calling another function to check. > > > > - This patch is based on > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F > > > > patc > > > > > > > > hwork.kernel.org%2Fpatch%2F11129553%2F&data=02%7C01%7Canson. > > > huang% > > > > > > > > 40nxp.com%7C2de0a6be69b74cc249ad08d74afc9730%7C686ea1d3bc2b4c6f > > > a92cd99 > > > > > > > > c5c301635%7C0%7C0%7C637060321046247040&sdata=RMFAdLKGKb6 > > > mEdhycrzHX > > > > R03E6Qr5pWyRc8Zk6ErlBc%3D&reserved=0 > > > > > > Thanks for this v2. It would be good to change the callers within this > series. > > > > NOT quite understand your point, the callers does NOT need to be > > changed, those > > 2 special APIs callers are already following the right way of calling the APIs. > > Ah okay. I searched the 5.4-rc2 tag and found the soc_uid_show() as only > user but this user sets the have_resp field to false. Is this intended? I already fixed it and patch applied by Shawn, see below: https://patchwork.kernel.org/patch/11129497/ Anson
On 19-10-09 08:28, Anson Huang wrote: > Hi, Marco > > > On 19-10-08 00:48, Anson Huang wrote: > > > Hi, Marco > > > > > > > On 19-10-07 09:15, 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. > > > > > > > > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > > > > > --- > > > > > Changes since V1: > > > > > - Use direct API check instead of calling another function to check. > > > > > - This patch is based on > > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F > > > > > patc > > > > > > > > > > > hwork.kernel.org%2Fpatch%2F11129553%2F&data=02%7C01%7Canson. > > > > huang% > > > > > > > > > > > 40nxp.com%7C2de0a6be69b74cc249ad08d74afc9730%7C686ea1d3bc2b4c6f > > > > a92cd99 > > > > > > > > > > > c5c301635%7C0%7C0%7C637060321046247040&sdata=RMFAdLKGKb6 > > > > mEdhycrzHX > > > > > R03E6Qr5pWyRc8Zk6ErlBc%3D&reserved=0 > > > > > > > > Thanks for this v2. It would be good to change the callers within this > > series. > > > > > > NOT quite understand your point, the callers does NOT need to be > > > changed, those > > > 2 special APIs callers are already following the right way of calling the APIs. > > > > Ah okay. I searched the 5.4-rc2 tag and found the soc_uid_show() as only > > user but this user sets the have_resp field to false. Is this intended? > > I already fixed it and patch applied by Shawn, see below: > > https://patchwork.kernel.org/patch/11129497/ I see :) So one last question, please check the other thread. Regards, Marco > > Anson >
Hi Anson, On 19-10-07 09:15, 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. > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > --- > Changes since V1: > - Use direct API check instead of calling another function to check. > - This patch is based on https://patchwork.kernel.org/patch/11129553/ > --- > drivers/firmware/imx/imx-scu.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-scu.c > index 869be7a..03b43b7 100644 > --- a/drivers/firmware/imx/imx-scu.c > +++ b/drivers/firmware/imx/imx-scu.c > @@ -162,6 +162,7 @@ static int imx_scu_ipc_write(struct imx_sc_ipc *sc_ipc, void *msg) > */ > int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, void *msg, bool have_resp) > { > + uint8_t saved_svc, saved_func; > struct imx_sc_rpc_msg *hdr; > int ret; > > @@ -171,8 +172,11 @@ int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, void *msg, bool have_resp) > mutex_lock(&sc_ipc->lock); > reinit_completion(&sc_ipc->done); > > - if (have_resp) > + if (have_resp) { > sc_ipc->msg = msg; > + saved_svc = ((struct imx_sc_rpc_msg *)msg)->svc; Why do we need to check the svc too? > + saved_func = ((struct imx_sc_rpc_msg *)msg)->func; Nitpick, should we call it requested_func/req_func? Regards, Marco > + } > sc_ipc->count = 0; > ret = imx_scu_ipc_write(sc_ipc, msg); > if (ret < 0) { > @@ -191,6 +195,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 ((saved_svc == IMX_SC_RPC_SVC_MISC) && > + (saved_func == IMX_SC_MISC_FUNC_UNIQUE_ID || > + saved_func == IMX_SC_MISC_FUNC_GET_BUTTON_STATUS)) > + ret = 0; > } > > out: > -- > 2.7.4 > > >
Hi, Marco > On 19-10-07 09:15, 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. > > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > > --- > > Changes since V1: > > - Use direct API check instead of calling another function to check. > > - This patch is based on > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc > > > hwork.kernel.org%2Fpatch%2F11129553%2F&data=02%7C01%7Canson. > huang% > > > 40nxp.com%7Cbefd2849a124462caa4a08d74c972dc9%7C686ea1d3bc2b4c6f > a92cd99 > > > c5c301635%7C0%7C0%7C637062084506889431&sdata=7fW8hZB4AaUK > 9QTKTJQR7 > > LuV2nGo6e%2Fqb%2Fqmn4ykquk%3D&reserved=0 > > --- > > drivers/firmware/imx/imx-scu.c | 16 +++++++++++++++- > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/firmware/imx/imx-scu.c > > b/drivers/firmware/imx/imx-scu.c index 869be7a..03b43b7 100644 > > --- a/drivers/firmware/imx/imx-scu.c > > +++ b/drivers/firmware/imx/imx-scu.c > > @@ -162,6 +162,7 @@ static int imx_scu_ipc_write(struct imx_sc_ipc > *sc_ipc, void *msg) > > */ > > int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, void *msg, bool > > have_resp) { > > + uint8_t saved_svc, saved_func; > > struct imx_sc_rpc_msg *hdr; > > int ret; > > > > @@ -171,8 +172,11 @@ int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, > void *msg, bool have_resp) > > mutex_lock(&sc_ipc->lock); > > reinit_completion(&sc_ipc->done); > > > > - if (have_resp) > > + if (have_resp) { > > sc_ipc->msg = msg; > > + saved_svc = ((struct imx_sc_rpc_msg *)msg)->svc; > > Why do we need to check the svc too? It is because the SCU firmware API has many different category called SVC, each category has many different function, and the function value could be same in each category, for example, there are IRQ, PM, MISC etc. SVC category, and each of them could have function type defined as 0, 1, 2 .... That is why I need to save both SVC and FUNC to identify the SCU FW API. See below: PM SVC: #define PM_FUNC_SET_PARTITION_POWER_MODE 1U /* Index for pm_set_partition_power_mode() RPC call */ #define PM_FUNC_GET_SYS_POWER_MODE 2U /* Index for pm_get_sys_power_mode() RPC call */ #define PM_FUNC_SET_RESOURCE_POWER_MODE 3U /* Index for pm_set_resource_power_mode() RPC call */ MISC SVC: #define MISC_FUNC_SET_CONTROL 1U /* Index for misc_set_control() RPC call */ #define MISC_FUNC_GET_CONTROL 2U /* Index for misc_get_control() RPC call */ #define MISC_FUNC_SET_MAX_DMA_GROUP 4U /* Index for misc_set_max_dma_group() RPC call */ > > > + saved_func = ((struct imx_sc_rpc_msg *)msg)->func; > > Nitpick, should we call it requested_func/req_func? OK, I will change them If I have to sent out a new version, otherwise, I think the saved_func and saved_svc should also be fine. Thanks, Anson
Hi Anson, On 19-10-09 09:09, Anson Huang wrote: > Hi, Marco > > > On 19-10-07 09:15, 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. > > > > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > > > --- > > > Changes since V1: > > > - Use direct API check instead of calling another function to check. > > > - This patch is based on > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc > > > > > hwork.kernel.org%2Fpatch%2F11129553%2F&data=02%7C01%7Canson. > > huang% > > > > > 40nxp.com%7Cbefd2849a124462caa4a08d74c972dc9%7C686ea1d3bc2b4c6f > > a92cd99 > > > > > c5c301635%7C0%7C0%7C637062084506889431&sdata=7fW8hZB4AaUK > > 9QTKTJQR7 > > > LuV2nGo6e%2Fqb%2Fqmn4ykquk%3D&reserved=0 > > > --- > > > drivers/firmware/imx/imx-scu.c | 16 +++++++++++++++- > > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/firmware/imx/imx-scu.c > > > b/drivers/firmware/imx/imx-scu.c index 869be7a..03b43b7 100644 > > > --- a/drivers/firmware/imx/imx-scu.c > > > +++ b/drivers/firmware/imx/imx-scu.c > > > @@ -162,6 +162,7 @@ static int imx_scu_ipc_write(struct imx_sc_ipc > > *sc_ipc, void *msg) > > > */ > > > int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, void *msg, bool > > > have_resp) { > > > + uint8_t saved_svc, saved_func; > > > struct imx_sc_rpc_msg *hdr; > > > int ret; > > > > > > @@ -171,8 +172,11 @@ int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, > > void *msg, bool have_resp) > > > mutex_lock(&sc_ipc->lock); > > > reinit_completion(&sc_ipc->done); > > > > > > - if (have_resp) > > > + if (have_resp) { > > > sc_ipc->msg = msg; > > > + saved_svc = ((struct imx_sc_rpc_msg *)msg)->svc; > > > > Why do we need to check the svc too? > > It is because the SCU firmware API has many different category called SVC, each category has > many different function, and the function value could be same in each category, > for example, there are IRQ, PM, MISC etc. SVC category, and each of them could have function > type defined as 0, 1, 2 .... That is why I need to save both SVC and FUNC to identify the SCU FW > API. See below: > > PM SVC: > #define PM_FUNC_SET_PARTITION_POWER_MODE 1U /* Index for pm_set_partition_power_mode() RPC call */ > #define PM_FUNC_GET_SYS_POWER_MODE 2U /* Index for pm_get_sys_power_mode() RPC call */ > #define PM_FUNC_SET_RESOURCE_POWER_MODE 3U /* Index for pm_set_resource_power_mode() RPC call */ > > MISC SVC: > #define MISC_FUNC_SET_CONTROL 1U /* Index for misc_set_control() RPC call */ > #define MISC_FUNC_GET_CONTROL 2U /* Index for misc_get_control() RPC call */ > #define MISC_FUNC_SET_MAX_DMA_GROUP 4U /* Index for misc_set_max_dma_group() RPC call */ Ahh, okay get it. Thanks for the explanation. > > > > > + saved_func = ((struct imx_sc_rpc_msg *)msg)->func; > > > > Nitpick, should we call it requested_func/req_func? > > OK, I will change them If I have to sent out a new version, otherwise, I think the saved_func and saved_svc > should also be fine. Just a nitpick ;) Feel free to add my Reviewed-by: Marco Felsch <m.felsch@pengutronix.de> Regards, Marco > > Thanks, > Anson
On Mon, Oct 07, 2019 at 09:15:59AM +0800, 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. > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> Applied, thanks.
diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-scu.c index 869be7a..03b43b7 100644 --- a/drivers/firmware/imx/imx-scu.c +++ b/drivers/firmware/imx/imx-scu.c @@ -162,6 +162,7 @@ static int imx_scu_ipc_write(struct imx_sc_ipc *sc_ipc, void *msg) */ int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, void *msg, bool have_resp) { + uint8_t saved_svc, saved_func; struct imx_sc_rpc_msg *hdr; int ret; @@ -171,8 +172,11 @@ int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, void *msg, bool have_resp) mutex_lock(&sc_ipc->lock); reinit_completion(&sc_ipc->done); - if (have_resp) + if (have_resp) { sc_ipc->msg = msg; + saved_svc = ((struct imx_sc_rpc_msg *)msg)->svc; + saved_func = ((struct imx_sc_rpc_msg *)msg)->func; + } sc_ipc->count = 0; ret = imx_scu_ipc_write(sc_ipc, msg); if (ret < 0) { @@ -191,6 +195,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 ((saved_svc == IMX_SC_RPC_SVC_MISC) && + (saved_func == IMX_SC_MISC_FUNC_UNIQUE_ID || + saved_func == IMX_SC_MISC_FUNC_GET_BUTTON_STATUS)) + ret = 0; } out:
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. Signed-off-by: Anson Huang <Anson.Huang@nxp.com> --- Changes since V1: - Use direct API check instead of calling another function to check. - This patch is based on https://patchwork.kernel.org/patch/11129553/ --- drivers/firmware/imx/imx-scu.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)