From 00126945902d2335c3b1f1c12db4ae2afeb4da0f Mon Sep 17 00:00:00 2001 From: Heiner Kallweit Date: Fri, 29 Sep 2017 23:43:54 +0200 Subject: [PATCH 01/11] firmware: arm_scpi: remove usage of drvdata and don't reset scpi_info to null There's no benefit using drvdata as variable scpi_info is global. Setting scpi_info to NULL in scpi_remove isn't needed too. If arm_scpi is built-in, then this code is never used. And if arm_scpi is built as a module and some other module calls get_scpi_ops() then due to this dependency scpi_remove is called only after the other module has been removed. Last but not least, users usually store the result of get_scpi_ops(), therefore setting scpi_info to NULL wouldn't really help. Signed-off-by: Heiner Kallweit [sudeep.holla: reworded the commit log a bit] Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scpi.c | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c index 7da9f1b83ebec..a7f686e9dc1c8 100644 --- a/drivers/firmware/arm_scpi.c +++ b/drivers/firmware/arm_scpi.c @@ -862,8 +862,6 @@ static int scpi_init_versions(struct scpi_drvinfo *info) static ssize_t protocol_version_show(struct device *dev, struct device_attribute *attr, char *buf) { - struct scpi_drvinfo *scpi_info = dev_get_drvdata(dev); - return sprintf(buf, "%d.%d\n", PROTOCOL_REV_MAJOR(scpi_info->protocol_version), PROTOCOL_REV_MINOR(scpi_info->protocol_version)); @@ -873,8 +871,6 @@ static DEVICE_ATTR_RO(protocol_version); static ssize_t firmware_version_show(struct device *dev, struct device_attribute *attr, char *buf) { - struct scpi_drvinfo *scpi_info = dev_get_drvdata(dev); - return sprintf(buf, "%d.%d.%d\n", FW_REV_MAJOR(scpi_info->firmware_version), FW_REV_MINOR(scpi_info->firmware_version), @@ -905,21 +901,17 @@ static int scpi_remove(struct platform_device *pdev) { int i; struct device *dev = &pdev->dev; - struct scpi_drvinfo *info = platform_get_drvdata(pdev); - - scpi_info = NULL; /* stop exporting SCPI ops through get_scpi_ops */ of_platform_depopulate(dev); sysfs_remove_groups(&dev->kobj, versions_groups); - scpi_free_channels(dev, info->channels, info->num_chans); - platform_set_drvdata(pdev, NULL); + scpi_free_channels(dev, scpi_info->channels, scpi_info->num_chans); - for (i = 0; i < MAX_DVFS_DOMAINS && info->dvfs[i]; i++) { - kfree(info->dvfs[i]->opps); - kfree(info->dvfs[i]); + for (i = 0; i < MAX_DVFS_DOMAINS && scpi_info->dvfs[i]; i++) { + kfree(scpi_info->dvfs[i]->opps); + kfree(scpi_info->dvfs[i]); } - devm_kfree(dev, info->channels); - devm_kfree(dev, info); + devm_kfree(dev, scpi_info->channels); + devm_kfree(dev, scpi_info); return 0; } @@ -1027,8 +1019,6 @@ static int scpi_probe(struct platform_device *pdev) scpi_info->num_chans = count; scpi_info->commands = scpi_std_commands; - platform_set_drvdata(pdev, scpi_info); - if (scpi_info->is_legacy) { /* Replace with legacy variants */ scpi_ops.clk_set_val = legacy_scpi_clk_set_val; From cfe8a9c9aaf47af3d9e6fe4c4b03f4a88844746d Mon Sep 17 00:00:00 2001 From: Heiner Kallweit Date: Fri, 29 Sep 2017 23:44:01 +0200 Subject: [PATCH 02/11] firmware: arm_scpi: remove two unneeded devm_kfree's in scpi_remove Both memory areas are free'd anyway when the device is destroyed, so we don't have to do it manually. Signed-off-by: Heiner Kallweit Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scpi.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c index a7f686e9dc1c8..b99af7af64fff 100644 --- a/drivers/firmware/arm_scpi.c +++ b/drivers/firmware/arm_scpi.c @@ -910,8 +910,6 @@ static int scpi_remove(struct platform_device *pdev) kfree(scpi_info->dvfs[i]->opps); kfree(scpi_info->dvfs[i]); } - devm_kfree(dev, scpi_info->channels); - devm_kfree(dev, scpi_info); return 0; } From 931cf0c53e6999375eac65f3650a656b88634b9d Mon Sep 17 00:00:00 2001 From: Heiner Kallweit Date: Fri, 29 Sep 2017 23:44:05 +0200 Subject: [PATCH 03/11] firmware: arm_scpi: pre-populate dvfs info in scpi_probe Pre-populating the dvfs info data in scpi_probe allows to make all memory allocations device-managed. This helps to simplify scpi_remove and eventually to get rid of scpi_remove completely. Signed-off-by: Heiner Kallweit [sudeep.holla: changed to continue probe even if scpi_dvfs_populate_info fails] Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scpi.c | 48 ++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 22 deletions(-) diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c index b99af7af64fff..97a22a73c2d28 100644 --- a/drivers/firmware/arm_scpi.c +++ b/drivers/firmware/arm_scpi.c @@ -644,35 +644,35 @@ static int opp_cmp_func(const void *opp1, const void *opp2) } static struct scpi_dvfs_info *scpi_dvfs_get_info(u8 domain) +{ + if (domain >= MAX_DVFS_DOMAINS) + return ERR_PTR(-EINVAL); + + return scpi_info->dvfs[domain] ?: ERR_PTR(-EINVAL); +} + +static int scpi_dvfs_populate_info(struct device *dev, u8 domain) { struct scpi_dvfs_info *info; struct scpi_opp *opp; struct dvfs_info buf; int ret, i; - if (domain >= MAX_DVFS_DOMAINS) - return ERR_PTR(-EINVAL); - - if (scpi_info->dvfs[domain]) /* data already populated */ - return scpi_info->dvfs[domain]; - ret = scpi_send_message(CMD_GET_DVFS_INFO, &domain, sizeof(domain), &buf, sizeof(buf)); if (ret) - return ERR_PTR(ret); + return ret; - info = kmalloc(sizeof(*info), GFP_KERNEL); + info = devm_kmalloc(dev, sizeof(*info), GFP_KERNEL); if (!info) - return ERR_PTR(-ENOMEM); + return -ENOMEM; info->count = DVFS_OPP_COUNT(buf.header); info->latency = DVFS_LATENCY(buf.header) * 1000; /* uS to nS */ - info->opps = kcalloc(info->count, sizeof(*opp), GFP_KERNEL); - if (!info->opps) { - kfree(info); - return ERR_PTR(-ENOMEM); - } + info->opps = devm_kcalloc(dev, info->count, sizeof(*opp), GFP_KERNEL); + if (!info->opps) + return -ENOMEM; for (i = 0, opp = info->opps; i < info->count; i++, opp++) { opp->freq = le32_to_cpu(buf.opps[i].freq); @@ -682,7 +682,15 @@ static struct scpi_dvfs_info *scpi_dvfs_get_info(u8 domain) sort(info->opps, info->count, sizeof(*opp), opp_cmp_func, NULL); scpi_info->dvfs[domain] = info; - return info; + return 0; +} + +static void scpi_dvfs_populate(struct device *dev) +{ + int domain; + + for (domain = 0; domain < MAX_DVFS_DOMAINS; domain++) + scpi_dvfs_populate_info(dev, domain); } static int scpi_dev_domain_id(struct device *dev) @@ -899,18 +907,12 @@ scpi_free_channels(struct device *dev, struct scpi_chan *pchan, int count) static int scpi_remove(struct platform_device *pdev) { - int i; struct device *dev = &pdev->dev; of_platform_depopulate(dev); sysfs_remove_groups(&dev->kobj, versions_groups); scpi_free_channels(dev, scpi_info->channels, scpi_info->num_chans); - for (i = 0; i < MAX_DVFS_DOMAINS && scpi_info->dvfs[i]; i++) { - kfree(scpi_info->dvfs[i]->opps); - kfree(scpi_info->dvfs[i]); - } - return 0; } @@ -1016,6 +1018,7 @@ static int scpi_probe(struct platform_device *pdev) scpi_info->channels = scpi_chan; scpi_info->num_chans = count; scpi_info->commands = scpi_std_commands; + scpi_info->scpi_ops = &scpi_ops; if (scpi_info->is_legacy) { /* Replace with legacy variants */ @@ -1035,13 +1038,14 @@ static int scpi_probe(struct platform_device *pdev) return ret; } + scpi_dvfs_populate(dev); + _dev_info(dev, "SCP Protocol %d.%d Firmware %d.%d.%d version\n", PROTOCOL_REV_MAJOR(scpi_info->protocol_version), PROTOCOL_REV_MINOR(scpi_info->protocol_version), FW_REV_MAJOR(scpi_info->firmware_version), FW_REV_MINOR(scpi_info->firmware_version), FW_REV_PATCH(scpi_info->firmware_version)); - scpi_info->scpi_ops = &scpi_ops; ret = sysfs_create_groups(&dev->kobj, versions_groups); if (ret) From 27c54cd3142184b7798e082d06c17135cafb3f60 Mon Sep 17 00:00:00 2001 From: Heiner Kallweit Date: Fri, 29 Sep 2017 23:44:09 +0200 Subject: [PATCH 04/11] firmware: arm_scpi: make freeing mbox channels device-managed Make freeing the mbox channels device-managed, thus further simplifying scpi_remove and and one further step to get rid of scpi_remove. Signed-off-by: Heiner Kallweit Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scpi.c | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c index 97a22a73c2d28..ee19b365c2892 100644 --- a/drivers/firmware/arm_scpi.c +++ b/drivers/firmware/arm_scpi.c @@ -893,16 +893,13 @@ static struct attribute *versions_attrs[] = { }; ATTRIBUTE_GROUPS(versions); -static void -scpi_free_channels(struct device *dev, struct scpi_chan *pchan, int count) +static void scpi_free_channels(void *data) { + struct scpi_drvinfo *info = data; int i; - for (i = 0; i < count && pchan->chan; i++, pchan++) { - mbox_free_channel(pchan->chan); - devm_kfree(dev, pchan->xfers); - devm_iounmap(dev, pchan->rx_payload); - } + for (i = 0; i < info->num_chans; i++) + mbox_free_channel(info->channels[i].chan); } static int scpi_remove(struct platform_device *pdev) @@ -911,7 +908,6 @@ static int scpi_remove(struct platform_device *pdev) of_platform_depopulate(dev); sysfs_remove_groups(&dev->kobj, versions_groups); - scpi_free_channels(dev, scpi_info->channels, scpi_info->num_chans); return 0; } @@ -944,7 +940,6 @@ static int scpi_probe(struct platform_device *pdev) { int count, idx, ret; struct resource res; - struct scpi_chan *scpi_chan; struct device *dev = &pdev->dev; struct device_node *np = dev->of_node; @@ -961,13 +956,19 @@ static int scpi_probe(struct platform_device *pdev) return -ENODEV; } - scpi_chan = devm_kcalloc(dev, count, sizeof(*scpi_chan), GFP_KERNEL); - if (!scpi_chan) + scpi_info->channels = devm_kcalloc(dev, count, sizeof(struct scpi_chan), + GFP_KERNEL); + if (!scpi_info->channels) return -ENOMEM; - for (idx = 0; idx < count; idx++) { + ret = devm_add_action(dev, scpi_free_channels, scpi_info); + if (ret) + return ret; + + for (; scpi_info->num_chans < count; scpi_info->num_chans++) { resource_size_t size; - struct scpi_chan *pchan = scpi_chan + idx; + int idx = scpi_info->num_chans; + struct scpi_chan *pchan = scpi_info->channels + idx; struct mbox_client *cl = &pchan->cl; struct device_node *shmem = of_parse_phandle(np, "shmem", idx); @@ -975,15 +976,14 @@ static int scpi_probe(struct platform_device *pdev) of_node_put(shmem); if (ret) { dev_err(dev, "failed to get SCPI payload mem resource\n"); - goto err; + return ret; } size = resource_size(&res); pchan->rx_payload = devm_ioremap(dev, res.start, size); if (!pchan->rx_payload) { dev_err(dev, "failed to ioremap SCPI payload\n"); - ret = -EADDRNOTAVAIL; - goto err; + return -EADDRNOTAVAIL; } pchan->tx_payload = pchan->rx_payload + (size >> 1); @@ -1009,14 +1009,9 @@ static int scpi_probe(struct platform_device *pdev) dev_err(dev, "failed to get channel%d err %d\n", idx, ret); } -err: - scpi_free_channels(dev, scpi_chan, idx); - scpi_info = NULL; return ret; } - scpi_info->channels = scpi_chan; - scpi_info->num_chans = count; scpi_info->commands = scpi_std_commands; scpi_info->scpi_ops = &scpi_ops; From 95998c7a6cf76a24a553bec7f6fafdef6b4d3374 Mon Sep 17 00:00:00 2001 From: Heiner Kallweit Date: Fri, 29 Sep 2017 23:44:15 +0200 Subject: [PATCH 05/11] firmware: arm_scpi: remove scpi_remove sysfs_create_groups and of_platform_populate can be replaced with the device-managed versions what allows us to remove scpi_remove. Signed-off-by: Heiner Kallweit Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scpi.c | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c index ee19b365c2892..f713a64c10528 100644 --- a/drivers/firmware/arm_scpi.c +++ b/drivers/firmware/arm_scpi.c @@ -902,16 +902,6 @@ static void scpi_free_channels(void *data) mbox_free_channel(info->channels[i].chan); } -static int scpi_remove(struct platform_device *pdev) -{ - struct device *dev = &pdev->dev; - - of_platform_depopulate(dev); - sysfs_remove_groups(&dev->kobj, versions_groups); - - return 0; -} - #define MAX_SCPI_XFERS 10 static int scpi_alloc_xfer_list(struct device *dev, struct scpi_chan *ch) { @@ -1029,7 +1019,6 @@ static int scpi_probe(struct platform_device *pdev) ret = scpi_init_versions(scpi_info); if (ret) { dev_err(dev, "incorrect or no SCP firmware found\n"); - scpi_remove(pdev); return ret; } @@ -1042,11 +1031,11 @@ static int scpi_probe(struct platform_device *pdev) FW_REV_MINOR(scpi_info->firmware_version), FW_REV_PATCH(scpi_info->firmware_version)); - ret = sysfs_create_groups(&dev->kobj, versions_groups); + ret = devm_device_add_groups(dev, versions_groups); if (ret) dev_err(dev, "unable to create sysfs version group\n"); - return of_platform_populate(dev->of_node, NULL, NULL, dev); + return devm_of_platform_populate(dev); } static const struct of_device_id scpi_of_match[] = { @@ -1063,7 +1052,6 @@ static struct platform_driver scpi_driver = { .of_match_table = scpi_of_match, }, .probe = scpi_probe, - .remove = scpi_remove, }; module_platform_driver(scpi_driver); From d9e324ea138b43c2cfb6b01e3ae02a84be55aef4 Mon Sep 17 00:00:00 2001 From: Heiner Kallweit Date: Wed, 4 Oct 2017 20:56:44 +0200 Subject: [PATCH 06/11] firmware: arm_scpi: improve struct dvfs_info to make code better readable Making the header subfields members of struct dvfs_info allows to make the code better readable and avoids some macro magic. In addition remove a useless statement using info->latency. Signed-off-by: Heiner Kallweit Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scpi.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c index f713a64c10528..412f1c4cf6162 100644 --- a/drivers/firmware/arm_scpi.c +++ b/drivers/firmware/arm_scpi.c @@ -72,8 +72,6 @@ #define MAX_DVFS_DOMAINS 8 #define MAX_DVFS_OPPS 16 -#define DVFS_LATENCY(hdr) (le32_to_cpu(hdr) >> 16) -#define DVFS_OPP_COUNT(hdr) ((le32_to_cpu(hdr) >> 8) & 0xff) #define PROTOCOL_REV_MINOR_BITS 16 #define PROTOCOL_REV_MINOR_MASK ((1U << PROTOCOL_REV_MINOR_BITS) - 1) @@ -328,7 +326,9 @@ struct legacy_clk_set_value { } __packed; struct dvfs_info { - __le32 header; + u8 domain; + u8 opp_count; + __le16 latency; struct { __le32 freq; __le32 m_volt; @@ -667,8 +667,8 @@ static int scpi_dvfs_populate_info(struct device *dev, u8 domain) if (!info) return -ENOMEM; - info->count = DVFS_OPP_COUNT(buf.header); - info->latency = DVFS_LATENCY(buf.header) * 1000; /* uS to nS */ + info->count = buf.opp_count; + info->latency = le16_to_cpu(buf.latency) * 1000; /* uS to nS */ info->opps = devm_kcalloc(dev, info->count, sizeof(*opp), GFP_KERNEL); if (!info->opps) @@ -721,9 +721,6 @@ static int scpi_dvfs_get_transition_latency(struct device *dev) if (IS_ERR(info)) return PTR_ERR(info); - if (!info->latency) - return 0; - return info->latency; } From 4864dca72d3fb2586ffee0c92bd13d02cedfee2f Mon Sep 17 00:00:00 2001 From: Heiner Kallweit Date: Wed, 4 Oct 2017 21:00:39 +0200 Subject: [PATCH 07/11] firmware: arm_scpi: improve handling of protocol and firmware version subfields By using FIELD_GET and proper masks we can avoid quite some shifting and masking macro magic and make the code better readable. Signed-off-by: Heiner Kallweit Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scpi.c | 43 ++++++++++++++++--------------------- 1 file changed, 19 insertions(+), 24 deletions(-) diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c index 412f1c4cf6162..d305b926c6cda 100644 --- a/drivers/firmware/arm_scpi.c +++ b/drivers/firmware/arm_scpi.c @@ -28,6 +28,7 @@ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt #include +#include #include #include #include @@ -73,18 +74,12 @@ #define MAX_DVFS_DOMAINS 8 #define MAX_DVFS_OPPS 16 -#define PROTOCOL_REV_MINOR_BITS 16 -#define PROTOCOL_REV_MINOR_MASK ((1U << PROTOCOL_REV_MINOR_BITS) - 1) -#define PROTOCOL_REV_MAJOR(x) ((x) >> PROTOCOL_REV_MINOR_BITS) -#define PROTOCOL_REV_MINOR(x) ((x) & PROTOCOL_REV_MINOR_MASK) +#define PROTO_REV_MAJOR_MASK GENMASK(31, 16) +#define PROTO_REV_MINOR_MASK GENMASK(15, 0) -#define FW_REV_MAJOR_BITS 24 -#define FW_REV_MINOR_BITS 16 -#define FW_REV_PATCH_MASK ((1U << FW_REV_MINOR_BITS) - 1) -#define FW_REV_MINOR_MASK ((1U << FW_REV_MAJOR_BITS) - 1) -#define FW_REV_MAJOR(x) ((x) >> FW_REV_MAJOR_BITS) -#define FW_REV_MINOR(x) (((x) & FW_REV_MINOR_MASK) >> FW_REV_MINOR_BITS) -#define FW_REV_PATCH(x) ((x) & FW_REV_PATCH_MASK) +#define FW_REV_MAJOR_MASK GENMASK(31, 24) +#define FW_REV_MINOR_MASK GENMASK(23, 16) +#define FW_REV_PATCH_MASK GENMASK(15, 0) #define MAX_RX_TIMEOUT (msecs_to_jiffies(30)) @@ -867,19 +862,19 @@ static int scpi_init_versions(struct scpi_drvinfo *info) static ssize_t protocol_version_show(struct device *dev, struct device_attribute *attr, char *buf) { - return sprintf(buf, "%d.%d\n", - PROTOCOL_REV_MAJOR(scpi_info->protocol_version), - PROTOCOL_REV_MINOR(scpi_info->protocol_version)); + return sprintf(buf, "%lu.%lu\n", + FIELD_GET(PROTO_REV_MAJOR_MASK, scpi_info->protocol_version), + FIELD_GET(PROTO_REV_MINOR_MASK, scpi_info->protocol_version)); } static DEVICE_ATTR_RO(protocol_version); static ssize_t firmware_version_show(struct device *dev, struct device_attribute *attr, char *buf) { - return sprintf(buf, "%d.%d.%d\n", - FW_REV_MAJOR(scpi_info->firmware_version), - FW_REV_MINOR(scpi_info->firmware_version), - FW_REV_PATCH(scpi_info->firmware_version)); + return sprintf(buf, "%lu.%lu.%lu\n", + FIELD_GET(FW_REV_MAJOR_MASK, scpi_info->firmware_version), + FIELD_GET(FW_REV_MINOR_MASK, scpi_info->firmware_version), + FIELD_GET(FW_REV_PATCH_MASK, scpi_info->firmware_version)); } static DEVICE_ATTR_RO(firmware_version); @@ -1021,12 +1016,12 @@ static int scpi_probe(struct platform_device *pdev) scpi_dvfs_populate(dev); - _dev_info(dev, "SCP Protocol %d.%d Firmware %d.%d.%d version\n", - PROTOCOL_REV_MAJOR(scpi_info->protocol_version), - PROTOCOL_REV_MINOR(scpi_info->protocol_version), - FW_REV_MAJOR(scpi_info->firmware_version), - FW_REV_MINOR(scpi_info->firmware_version), - FW_REV_PATCH(scpi_info->firmware_version)); + _dev_info(dev, "SCP Protocol %lu.%lu Firmware %lu.%lu.%lu version\n", + FIELD_GET(PROTO_REV_MAJOR_MASK, scpi_info->protocol_version), + FIELD_GET(PROTO_REV_MINOR_MASK, scpi_info->protocol_version), + FIELD_GET(FW_REV_MAJOR_MASK, scpi_info->firmware_version), + FIELD_GET(FW_REV_MINOR_MASK, scpi_info->firmware_version), + FIELD_GET(FW_REV_PATCH_MASK, scpi_info->firmware_version)); ret = devm_device_add_groups(dev, versions_groups); if (ret) From 5c7ae6467df557f25daf42e78202531bc44c83df Mon Sep 17 00:00:00 2001 From: Heiner Kallweit Date: Wed, 4 Oct 2017 21:05:17 +0200 Subject: [PATCH 08/11] firmware: arm_scpi: improve struct sensor_value lo_val and hi_val together in this order are a little endian 64 bit value. Therefore we can simplify struct sensor_value and the code by defining it as a __le64 value and by using le64_to_cpu. Signed-off-by: Heiner Kallweit Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scpi.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c index d305b926c6cda..c923d1ebfa3e3 100644 --- a/drivers/firmware/arm_scpi.c +++ b/drivers/firmware/arm_scpi.c @@ -347,9 +347,8 @@ struct _scpi_sensor_info { }; struct sensor_value { - __le32 lo_val; - __le32 hi_val; -} __packed; + __le64 val; +}; struct dev_pstate_set { __le16 dev_id; @@ -785,11 +784,10 @@ static int scpi_sensor_get_value(u16 sensor, u64 *val) return ret; if (scpi_info->is_legacy) - /* only 32-bits supported, hi_val can be junk */ - *val = le32_to_cpu(buf.lo_val); + /* only 32-bits supported, upper 32 bits can be junk */ + *val = le32_to_cpup((__le32 *)&buf.val); else - *val = (u64)le32_to_cpu(buf.hi_val) << 32 | - le32_to_cpu(buf.lo_val); + *val = le64_to_cpu(buf.val); return 0; } From 1b36633ea9c80f91fab505002dd7548febc51812 Mon Sep 17 00:00:00 2001 From: Sudeep Holla Date: Thu, 5 Oct 2017 11:31:36 +0100 Subject: [PATCH 09/11] firmware: arm_scpi: drop unnecessary type cast to scpi_shared_mem This patch drops the only present type cast of the SCPI payload pointer to scpi_shared_mem inorder to align with other occurrences, IOW for consistency. Reviewed-by: Heiner Kallweit Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scpi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c index c923d1ebfa3e3..a888970f1347c 100644 --- a/drivers/firmware/arm_scpi.c +++ b/drivers/firmware/arm_scpi.c @@ -453,7 +453,7 @@ static void scpi_tx_prepare(struct mbox_client *c, void *msg) unsigned long flags; struct scpi_xfer *t = msg; struct scpi_chan *ch = container_of(c, struct scpi_chan, cl); - struct scpi_shared_mem *mem = (struct scpi_shared_mem *)ch->tx_payload; + struct scpi_shared_mem *mem = ch->tx_payload; if (t->tx_buf) { if (scpi_info->is_legacy) From 48bee74a354b211c4e4cb62e6a7b7986749747cf Mon Sep 17 00:00:00 2001 From: Sudeep Holla Date: Thu, 5 Oct 2017 11:40:49 +0100 Subject: [PATCH 10/11] firmware: arm_scpi: remove all single element structures Both clk_get_value and sensor_value structures contains a single element and hence needs no packing making the whole structure defination unnecessary. This patch gets rid of both those unnecessary structures. Reviewed-by: Heiner Kallweit Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scpi.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c index a888970f1347c..f0c37a4ecddf7 100644 --- a/drivers/firmware/arm_scpi.c +++ b/drivers/firmware/arm_scpi.c @@ -304,10 +304,6 @@ struct clk_get_info { u8 name[20]; } __packed; -struct clk_get_value { - __le32 rate; -} __packed; - struct clk_set_value { __le16 id; __le16 reserved; @@ -346,10 +342,6 @@ struct _scpi_sensor_info { char name[20]; }; -struct sensor_value { - __le64 val; -}; - struct dev_pstate_set { __le16 dev_id; u8 pstate; @@ -577,13 +569,13 @@ scpi_clk_get_range(u16 clk_id, unsigned long *min, unsigned long *max) static unsigned long scpi_clk_get_val(u16 clk_id) { int ret; - struct clk_get_value clk; + __le32 rate; __le16 le_clk_id = cpu_to_le16(clk_id); ret = scpi_send_message(CMD_GET_CLOCK_VALUE, &le_clk_id, - sizeof(le_clk_id), &clk, sizeof(clk)); + sizeof(le_clk_id), &rate, sizeof(rate)); - return ret ? ret : le32_to_cpu(clk.rate); + return ret ? ret : le32_to_cpu(rate); } static int scpi_clk_set_val(u16 clk_id, unsigned long rate) @@ -775,19 +767,19 @@ static int scpi_sensor_get_info(u16 sensor_id, struct scpi_sensor_info *info) static int scpi_sensor_get_value(u16 sensor, u64 *val) { __le16 id = cpu_to_le16(sensor); - struct sensor_value buf; + __le64 value; int ret; ret = scpi_send_message(CMD_SENSOR_VALUE, &id, sizeof(id), - &buf, sizeof(buf)); + &value, sizeof(value)); if (ret) return ret; if (scpi_info->is_legacy) /* only 32-bits supported, upper 32 bits can be junk */ - *val = le32_to_cpup((__le32 *)&buf.val); + *val = le32_to_cpup((__le32 *)&value); else - *val = le64_to_cpu(buf.val); + *val = le64_to_cpu(value); return 0; } From 430594c1c7f5051f0d99ed9d08d086d20587cdd1 Mon Sep 17 00:00:00 2001 From: Heiner Kallweit Date: Fri, 6 Oct 2017 20:59:05 +0200 Subject: [PATCH 11/11] firmware: arm_scpi: silence sparse warnings At several positions in the code sparse complains about incorrect access to __iomem annotated memory. Fix this and make sparse happy. Signed-off-by: Heiner Kallweit Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scpi.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c index f0c37a4ecddf7..dfb373c8ba2a4 100644 --- a/drivers/firmware/arm_scpi.c +++ b/drivers/firmware/arm_scpi.c @@ -405,19 +405,20 @@ static void scpi_process_cmd(struct scpi_chan *ch, u32 cmd) unsigned int len; if (scpi_info->is_legacy) { - struct legacy_scpi_shared_mem *mem = ch->rx_payload; + struct legacy_scpi_shared_mem __iomem *mem = + ch->rx_payload; /* RX Length is not replied by the legacy Firmware */ len = match->rx_len; - match->status = le32_to_cpu(mem->status); + match->status = ioread32(&mem->status); memcpy_fromio(match->rx_buf, mem->payload, len); } else { - struct scpi_shared_mem *mem = ch->rx_payload; + struct scpi_shared_mem __iomem *mem = ch->rx_payload; len = min(match->rx_len, CMD_SIZE(cmd)); - match->status = le32_to_cpu(mem->status); + match->status = ioread32(&mem->status); memcpy_fromio(match->rx_buf, mem->payload, len); } @@ -431,11 +432,11 @@ static void scpi_process_cmd(struct scpi_chan *ch, u32 cmd) static void scpi_handle_remote_msg(struct mbox_client *c, void *msg) { struct scpi_chan *ch = container_of(c, struct scpi_chan, cl); - struct scpi_shared_mem *mem = ch->rx_payload; + struct scpi_shared_mem __iomem *mem = ch->rx_payload; u32 cmd = 0; if (!scpi_info->is_legacy) - cmd = le32_to_cpu(mem->command); + cmd = ioread32(&mem->command); scpi_process_cmd(ch, cmd); } @@ -445,7 +446,7 @@ static void scpi_tx_prepare(struct mbox_client *c, void *msg) unsigned long flags; struct scpi_xfer *t = msg; struct scpi_chan *ch = container_of(c, struct scpi_chan, cl); - struct scpi_shared_mem *mem = ch->tx_payload; + struct scpi_shared_mem __iomem *mem = ch->tx_payload; if (t->tx_buf) { if (scpi_info->is_legacy) @@ -464,7 +465,7 @@ static void scpi_tx_prepare(struct mbox_client *c, void *msg) } if (!scpi_info->is_legacy) - mem->command = cpu_to_le32(t->cmd); + iowrite32(t->cmd, &mem->command); } static struct scpi_xfer *get_scpi_xfer(struct scpi_chan *ch)