From cf0fd404455ce13850cc15423a3c2958933de384 Mon Sep 17 00:00:00 2001 From: Leonard Crestez Date: Wed, 4 Sep 2019 10:54:58 +0300 Subject: [PATCH 1/7] firmware: imx: warn on unexpected RX The imx_scu_call_rpc function returns the result inside the same "msg" struct containing the transmitted message. This is implemented by holding a pointer to msg (which is usually on the stack) in sc_imx_rpc and writing to it from imx_scu_rx_callback. This means that if the have_resp parameter is incorrect or SCU sends an unexpected response for any reason the most likely result is kernel stack corruption. Fix this by only setting sc_imx_rpc.msg for the duration of the imx_scu_call_rpc call and warning in imx_scu_rx_callback if unset. Print the unexpected response data to help debugging. Signed-off-by: Leonard Crestez Acked-by: Anson Huang Signed-off-by: Shawn Guo --- drivers/firmware/imx/imx-scu.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-scu.c index 04a24a863d6e..869be7a5172c 100644 --- a/drivers/firmware/imx/imx-scu.c +++ b/drivers/firmware/imx/imx-scu.c @@ -107,6 +107,12 @@ static void imx_scu_rx_callback(struct mbox_client *c, void *msg) struct imx_sc_rpc_msg *hdr; u32 *data = msg; + if (!sc_ipc->msg) { + dev_warn(sc_ipc->dev, "unexpected rx idx %d 0x%08x, ignore!\n", + sc_chan->idx, *data); + return; + } + if (sc_chan->idx == 0) { hdr = msg; sc_ipc->rx_size = hdr->size; @@ -165,7 +171,8 @@ 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); - sc_ipc->msg = msg; + if (have_resp) + sc_ipc->msg = msg; sc_ipc->count = 0; ret = imx_scu_ipc_write(sc_ipc, msg); if (ret < 0) { @@ -187,6 +194,7 @@ int imx_scu_call_rpc(struct imx_sc_ipc *sc_ipc, void *msg, bool have_resp) } out: + sc_ipc->msg = NULL; mutex_unlock(&sc_ipc->lock); dev_dbg(sc_ipc->dev, "RPC SVC done\n"); From 51f5afabc07a13e3d030076c772a1c36e1687b99 Mon Sep 17 00:00:00 2001 From: Anson Huang Date: Mon, 7 Oct 2019 09:15:59 +0800 Subject: [PATCH 2/7] firmware: imx: Skip return value check for some special SCU firmware APIs 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 Reviewed-by: Marco Felsch Signed-off-by: Shawn Guo --- 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 869be7a5172c..03b43b7a6d1d 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: From 0e4e8cc30a2940c57448af1376e40d3c0996fb29 Mon Sep 17 00:00:00 2001 From: Daniel Baluta Date: Mon, 14 Oct 2019 18:32:28 +0300 Subject: [PATCH 3/7] firmware: imx: Remove call to devm_of_platform_populate IMX DSP device is created by SOF layer. The current call to devm_of_platform_populate is not needed and it doesn't produce any effects. Fixes: ffbf23d50353915d ("firmware: imx: Add DSP IPC protocol interface) Signed-off-by: Daniel Baluta Signed-off-by: Shawn Guo --- drivers/firmware/imx/imx-dsp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/firmware/imx/imx-dsp.c b/drivers/firmware/imx/imx-dsp.c index a43d2db5cbdb..4265e9dbed84 100644 --- a/drivers/firmware/imx/imx-dsp.c +++ b/drivers/firmware/imx/imx-dsp.c @@ -114,7 +114,7 @@ static int imx_dsp_probe(struct platform_device *pdev) dev_info(dev, "NXP i.MX DSP IPC initialized\n"); - return devm_of_platform_populate(dev); + return 0; out: kfree(chan_name); for (j = 0; j < i; j++) { From e4f9eefbb8a976bb86dbdc9d2dd1a2a113801464 Mon Sep 17 00:00:00 2001 From: "Ben Dooks (Codethink)" Date: Tue, 22 Oct 2019 16:40:09 +0100 Subject: [PATCH 4/7] firmware: imx: add missing include of Include for the declarations of the functions exported from this driver. This fixes the following sparse warnings: drivers/firmware/imx/imx-scu-irq.c:45:5: warning: symbol 'imx_scu_irq_register_notifier' was not declared. Should it be static? drivers/firmware/imx/imx-scu-irq.c:52:5: warning: symbol 'imx_scu_irq_unregister_notifier' was not declared. Should it be static? drivers/firmware/imx/imx-scu-irq.c:97:5: warning: symbol 'imx_scu_irq_group_enable' was not declared. Should it be static? drivers/firmware/imx/imx-scu-irq.c:130:5: warning: symbol 'imx_scu_enable_general_irq_channel' was not declared. Should it be static? Signed-off-by: Ben Dooks (Codethink) Signed-off-by: Shawn Guo --- drivers/firmware/imx/imx-scu-irq.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/firmware/imx/imx-scu-irq.c b/drivers/firmware/imx/imx-scu-irq.c index 687121f8c4d5..db655e87cdc8 100644 --- a/drivers/firmware/imx/imx-scu-irq.c +++ b/drivers/firmware/imx/imx-scu-irq.c @@ -8,6 +8,7 @@ #include #include +#include #include #define IMX_SC_IRQ_FUNC_ENABLE 1 From a0708f559e4fa2239b84e927e661d26e6864e185 Mon Sep 17 00:00:00 2001 From: Anson Huang Date: Fri, 25 Oct 2019 14:56:22 +0800 Subject: [PATCH 5/7] soc: imx8: Using existing serial_number instead of UID The soc_device_attribute structure already contains a serial_number attribute to show SoC's unique ID, just use it to show SoC's unique ID instead of creating a new file called soc_uid. Signed-off-by: Anson Huang Signed-off-by: Shawn Guo --- drivers/soc/imx/soc-imx8.c | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/drivers/soc/imx/soc-imx8.c b/drivers/soc/imx/soc-imx8.c index b9831576dd25..fcbf745a9971 100644 --- a/drivers/soc/imx/soc-imx8.c +++ b/drivers/soc/imx/soc-imx8.c @@ -29,14 +29,6 @@ struct imx8_soc_data { static u64 soc_uid; -static ssize_t soc_uid_show(struct device *dev, - struct device_attribute *attr, char *buf) -{ - return sprintf(buf, "%016llX\n", soc_uid); -} - -static DEVICE_ATTR_RO(soc_uid); - static u32 __init imx8mq_soc_revision(void) { struct device_node *np; @@ -174,22 +166,25 @@ static int __init imx8_soc_init(void) goto free_soc; } - soc_dev = soc_device_register(soc_dev_attr); - if (IS_ERR(soc_dev)) { - ret = PTR_ERR(soc_dev); + soc_dev_attr->serial_number = kasprintf(GFP_KERNEL, "%016llX", soc_uid); + if (!soc_dev_attr->serial_number) { + ret = -ENOMEM; goto free_rev; } - ret = device_create_file(soc_device_to_device(soc_dev), - &dev_attr_soc_uid); - if (ret) - goto free_rev; + soc_dev = soc_device_register(soc_dev_attr); + if (IS_ERR(soc_dev)) { + ret = PTR_ERR(soc_dev); + goto free_serial_number; + } if (IS_ENABLED(CONFIG_ARM_IMX_CPUFREQ_DT)) platform_device_register_simple("imx-cpufreq-dt", -1, NULL, 0); return 0; +free_serial_number: + kfree(soc_dev_attr->serial_number); free_rev: if (strcmp(soc_dev_attr->revision, "unknown")) kfree(soc_dev_attr->revision); From 7ae399b7d009c7348b9451ac41cd49671b31cf3a Mon Sep 17 00:00:00 2001 From: Anson Huang Date: Fri, 25 Oct 2019 14:56:23 +0800 Subject: [PATCH 6/7] soc: imx-scu: Using existing serial_number instead of UID The soc_device_attribute structure already contains a serial_number attribute to show SoC's unique ID, just use it to show SoC's unique ID instead of creating a new file called soc_uid. Signed-off-by: Anson Huang Signed-off-by: Shawn Guo --- drivers/soc/imx/soc-imx-scu.c | 36 +++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/drivers/soc/imx/soc-imx-scu.c b/drivers/soc/imx/soc-imx-scu.c index 50831ebf126a..f2eace5b50fe 100644 --- a/drivers/soc/imx/soc-imx-scu.c +++ b/drivers/soc/imx/soc-imx-scu.c @@ -33,12 +33,10 @@ struct imx_sc_msg_misc_get_soc_uid { u32 uid_high; } __packed; -static ssize_t soc_uid_show(struct device *dev, - struct device_attribute *attr, char *buf) +static int imx_scu_soc_uid(u64 *soc_uid) { struct imx_sc_msg_misc_get_soc_uid msg; struct imx_sc_rpc_msg *hdr = &msg.hdr; - u64 soc_uid; int ret; hdr->ver = IMX_SC_RPC_VERSION; @@ -52,15 +50,13 @@ static ssize_t soc_uid_show(struct device *dev, return ret; } - soc_uid = msg.uid_high; - soc_uid <<= 32; - soc_uid |= msg.uid_low; + *soc_uid = msg.uid_high; + *soc_uid <<= 32; + *soc_uid |= msg.uid_low; - return sprintf(buf, "%016llX\n", soc_uid); + return 0; } -static DEVICE_ATTR_RO(soc_uid); - static int imx_scu_soc_id(void) { struct imx_sc_msg_misc_get_soc_id msg; @@ -89,6 +85,7 @@ static int imx_scu_soc_probe(struct platform_device *pdev) struct soc_device_attribute *soc_dev_attr; struct soc_device *soc_dev; int id, ret; + u64 uid = 0; u32 val; ret = imx_scu_get_handle(&soc_ipc_handle); @@ -112,6 +109,10 @@ static int imx_scu_soc_probe(struct platform_device *pdev) if (id < 0) return -EINVAL; + ret = imx_scu_soc_uid(&uid); + if (ret < 0) + return -EINVAL; + /* format soc_id value passed from SCU firmware */ val = id & 0x1f; soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "0x%x", val); @@ -130,19 +131,22 @@ static int imx_scu_soc_probe(struct platform_device *pdev) goto free_soc_id; } - soc_dev = soc_device_register(soc_dev_attr); - if (IS_ERR(soc_dev)) { - ret = PTR_ERR(soc_dev); + soc_dev_attr->serial_number = kasprintf(GFP_KERNEL, "%016llX", uid); + if (!soc_dev_attr->serial_number) { + ret = -ENOMEM; goto free_revision; } - ret = device_create_file(soc_device_to_device(soc_dev), - &dev_attr_soc_uid); - if (ret) - goto free_revision; + soc_dev = soc_device_register(soc_dev_attr); + if (IS_ERR(soc_dev)) { + ret = PTR_ERR(soc_dev); + goto free_serial_number; + } return 0; +free_serial_number: + kfree(soc_dev_attr->serial_number); free_revision: kfree(soc_dev_attr->revision); free_soc_id: From 768e1a8e093677f5e0e7d0a447c1a856c16cbb66 Mon Sep 17 00:00:00 2001 From: Leonard Crestez Date: Wed, 30 Oct 2019 01:17:39 +0200 Subject: [PATCH 7/7] soc: imx8mq: Read SOC revision from TF-A SOC revision on older imx8mq is not available in fuses so on anything other than B1 current code just reports "unknown". TF-A already handles this by parsing the ROM and exposes the value through a SMC call. Call this instead of reimplementing the workaround in the kernel itself. Signed-off-by: Leonard Crestez Reviewed-by: Abel Vesa Signed-off-by: Shawn Guo --- drivers/soc/imx/soc-imx8.c | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/drivers/soc/imx/soc-imx8.c b/drivers/soc/imx/soc-imx8.c index fcbf745a9971..d84ed736cdb0 100644 --- a/drivers/soc/imx/soc-imx8.c +++ b/drivers/soc/imx/soc-imx8.c @@ -9,6 +9,7 @@ #include #include #include +#include #include #define REV_B1 0x21 @@ -16,6 +17,8 @@ #define IMX8MQ_SW_INFO_B1 0x40 #define IMX8MQ_SW_MAGIC_B1 0xff0055aa +#define IMX_SIP_GET_SOC_INFO 0xc2000006 + #define OCOTP_UID_LOW 0x410 #define OCOTP_UID_HIGH 0x420 @@ -29,6 +32,22 @@ struct imx8_soc_data { static u64 soc_uid; +#ifdef CONFIG_HAVE_ARM_SMCCC +static u32 imx8mq_soc_revision_from_atf(void) +{ + struct arm_smccc_res res; + + arm_smccc_smc(IMX_SIP_GET_SOC_INFO, 0, 0, 0, 0, 0, 0, 0, &res); + + if (res.a0 == SMCCC_RET_NOT_SUPPORTED) + return 0; + else + return res.a0 & 0xff; +} +#else +static inline u32 imx8mq_soc_revision_from_atf(void) { return 0; }; +#endif + static u32 __init imx8mq_soc_revision(void) { struct device_node *np; @@ -43,9 +62,16 @@ static u32 __init imx8mq_soc_revision(void) ocotp_base = of_iomap(np, 0); WARN_ON(!ocotp_base); - magic = readl_relaxed(ocotp_base + IMX8MQ_SW_INFO_B1); - if (magic == IMX8MQ_SW_MAGIC_B1) - rev = REV_B1; + /* + * SOC revision on older imx8mq is not available in fuses so query + * the value from ATF instead. + */ + rev = imx8mq_soc_revision_from_atf(); + if (!rev) { + magic = readl_relaxed(ocotp_base + IMX8MQ_SW_INFO_B1); + if (magic == IMX8MQ_SW_MAGIC_B1) + rev = REV_B1; + } soc_uid = readl_relaxed(ocotp_base + OCOTP_UID_HIGH); soc_uid <<= 32;