Skip to contentSkip to conversation

firmware: arm_scpi: series with smaller improvements

PATCH New Heiner Kallweit 3 participants 16 messages Dec 5, 2017 – Feb 28, 2018

Subsystems (2)
Arm/Amlogic Meson SoC support System control & power/management interface (SCPI/SCMI) Message Protocol drivers
lore
Share & tools
lore

Heiner Kallweit

author
#0 lore raw

cover firmware: arm_scpi: series with smaller improvements

After recent revert of all changes since 4.14 this is a resubmit of the patch series, reduced to patches which should not cause any regression.
Based on a remark from Kevin I added one patch for improving the version info for pre-1.0 firmware that doesn't provide version information.

Best should be to apply it to a devel branch first so that the Baylibre Amlogic team can test before mainlining.

Heiner Kallweit (10):
firmware: arm_scpi: remove two unneeded devm_kfree's in scpi_remove
firmware: arm_scpi: make freeing mbox channels device-managed
firmware: arm_scpi: make scpi_probe completely device-managed
firmware: arm_scpi: improve struct dvfs_info to make code better readable
firmware: arm_scpi: improve handling of protocol and firmware version subfields
firmware: arm_scpi: improve struct sensor_value
firmware: arm_scpi: silence sparse warnings
firmware: arm_scpi: remove struct sensor_capabilities
firmware: arm_scpi: use FIELD_GET/_PREP to simplify macro definitions
firmware: arm_scpi: improve info message for pre-1.0 firmware

Sudeep Holla (2):
firmware: arm_scpi: drop unnecessary type cast to scpi_shared_mem
firmware: arm_scpi: remove all single element structures

 drivers/firmware/arm_scpi.c | 211 +++++++++++++++++++-------------------------
 1 file changed, 89 insertions(+), 122 deletions(-)
Signature · 1 line
2.15.1
author
#1 lore raw

PATCH 1/12 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 <[email protected]>
---
 drivers/firmware/arm_scpi.c | 2 --
 1 file changed, 2 deletions(-)
drivers/firmware/arm_scpi.c +0 -2
index 7da9f1b8..2a30d255 100644
@@ -918,8 +918,6 @@ static int scpi_remove(struct platform_device *pdev)
 		kfree(info->dvfs[i]->opps);
 		kfree(info->dvfs[i]);
 	}
-	devm_kfree(dev, info->channels);
-	devm_kfree(dev, info);
 
 	return 0;
 }
Signature · 1 line
2.15.1
author
#2 lore raw

PATCH 2/12 firmware: arm_scpi: make freeing mbox channels device-managed

In reply to Heiner Kallweit

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 <[email protected]>
---
 drivers/firmware/arm_scpi.c | 37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)
drivers/firmware/arm_scpi.c +16 -21
index 2a30d255..4447738d 100644
@@ -889,16 +889,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, info->channels, info->num_chans);
 	platform_set_drvdata(pdev, NULL);
 
 	for (i = 0; i < MAX_DVFS_DOMAINS && info->dvfs[i]; i++) {

@@ -950,7 +946,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;
 

@@ -967,13 +962,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);
 

@@ -981,15 +982,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);
 

@@ -1015,14 +1015,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;
 
 	platform_set_drvdata(pdev, scpi_info);
Signature · 1 line
2.15.1
author
#3 lore raw

PATCH 3/12 firmware: arm_scpi: make scpi_probe completely device-managed

In reply to Heiner Kallweit

Replace two remaining functions in probe with their devm versions.

Signed-off-by Heiner Kallweit <[email protected]>
---
 drivers/firmware/arm_scpi.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)
drivers/firmware/arm_scpi.c +2 -8
index 4447738d..a6f6039e 100644
@@ -901,15 +901,10 @@ static void scpi_free_channels(void *data)
 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);
-	platform_set_drvdata(pdev, NULL);
-
 	for (i = 0; i < MAX_DVFS_DOMAINS && info->dvfs[i]; i++) {
 		kfree(info->dvfs[i]->opps);
 		kfree(info->dvfs[i]);

@@ -1036,7 +1031,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;
 	}
 

@@ -1048,11 +1042,11 @@ static int scpi_probe(struct platform_device *pdev)
 		  FW_REV_PATCH(scpi_info->firmware_version));
 	scpi_info->scpi_ops = &scpi_ops;
 
-	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[] = {
Signature · 1 line
2.15.1
author
#4 lore raw

PATCH 4/12 firmware: arm_scpi: improve struct dvfs_info to make code better readable

In reply to Heiner Kallweit

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 <[email protected]>
---
 drivers/firmware/arm_scpi.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)
drivers/firmware/arm_scpi.c +5 -8
index a6f6039e..9eeb53b7 100644
@@ -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;

@@ -665,8 +665,8 @@ static struct scpi_dvfs_info *scpi_dvfs_get_info(u8 domain)
 	if (!info)
 		return ERR_PTR(-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 = kcalloc(info->count, sizeof(*opp), GFP_KERNEL);
 	if (!info->opps) {

@@ -713,9 +713,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;
 }
Signature · 1 line
2.15.1
author
#5 lore raw

PATCH 5/12 firmware: arm_scpi: improve handling of protocol and firmware version subfields

In reply to Heiner Kallweit

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 <[email protected]>
---
 drivers/firmware/arm_scpi.c | 43 +++++++++++++++++++------------------------
 1 file changed, 19 insertions(+), 24 deletions(-)
drivers/firmware/arm_scpi.c +19 -24
index 9eeb53b7..63441e40 100644
@@ -28,6 +28,7 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/bitmap.h>
+#include <linux/bitfield.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/export.h>

@@ -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))
 

@@ -861,9 +856,9 @@ static ssize_t protocol_version_show(struct device *dev,
 {
 	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));
+	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);
 

@@ -872,10 +867,10 @@ static ssize_t firmware_version_show(struct device *dev,
 {
 	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),
-		       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);
 

@@ -1031,12 +1026,12 @@ static int scpi_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	_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));
 	scpi_info->scpi_ops = &scpi_ops;
 
 	ret = devm_device_add_groups(dev, versions_groups);
Signature · 1 line
2.15.1
author
#6 lore raw

PATCH 6/12 firmware: arm_scpi: improve struct sensor_value

In reply to Heiner Kallweit

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 <[email protected]>
---
 drivers/firmware/arm_scpi.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)
drivers/firmware/arm_scpi.c +5 -7
index 63441e40..4447af48 100644
@@ -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;

@@ -777,11 +776,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;
 }
Signature · 1 line
2.15.1
author
#7 lore raw

PATCH 7/12 firmware: arm_scpi: drop unnecessary type cast to scpi_shared_mem

In reply to Heiner Kallweit

From: Sudeep Holla <[email protected]>
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 <[email protected]>
Signed-off-by Sudeep Holla <[email protected]>
---
 drivers/firmware/arm_scpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/firmware/arm_scpi.c +1 -1
index 4447af48..e02d5820 100644
@@ -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)
Signature · 1 line
2.15.1
author
#8 lore raw

PATCH 8/12 firmware: arm_scpi: remove all single element structures

In reply to Heiner Kallweit

From: Sudeep Holla <[email protected]>
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 <[email protected]>
Signed-off-by Sudeep Holla <[email protected]>
---
 drivers/firmware/arm_scpi.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)
drivers/firmware/arm_scpi.c +7 -15
index e02d5820..3a722e5a 100644
@@ -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)

@@ -767,19 +759,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;
 }
Signature · 1 line
2.15.1
author
#9 lore raw

PATCH 9/12 firmware: arm_scpi: silence sparse warnings

In reply to Heiner Kallweit

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 <[email protected]>
---
 drivers/firmware/arm_scpi.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)
drivers/firmware/arm_scpi.c +9 -8
index 3a722e5a..b653ada4 100644
@@ -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)
Signature · 1 line
2.15.1
author
#10 lore raw

PATCH 10/12 firmware: arm_scpi: remove struct sensor_capabilities

In reply to Heiner Kallweit

One more single-element struct was left, remove it.

Signed-off-by Heiner Kallweit <[email protected]>
---
 drivers/firmware/arm_scpi.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)
drivers/firmware/arm_scpi.c +4 -8
index b653ada4..5695b1f4 100644
@@ -331,10 +331,6 @@ struct dvfs_set {
 	u8 index;
 } __packed;
 
-struct sensor_capabilities {
-	__le16 sensors;
-} __packed;
-
 struct _scpi_sensor_info {
 	__le16 sensor_id;
 	u8 class;

@@ -730,13 +726,13 @@ static int scpi_dvfs_add_opps_to_device(struct device *dev)
 
 static int scpi_sensor_get_capability(u16 *sensors)
 {
-	struct sensor_capabilities cap_buf;
+	__le16 cap;
 	int ret;
 
-	ret = scpi_send_message(CMD_SENSOR_CAPABILITIES, NULL, 0, &cap_buf,
-				sizeof(cap_buf));
+	ret = scpi_send_message(CMD_SENSOR_CAPABILITIES, NULL, 0, &cap,
+				sizeof(cap));
 	if (!ret)
-		*sensors = le16_to_cpu(cap_buf.sensors);
+		*sensors = le16_to_cpu(cap);
 
 	return ret;
 }
Signature · 1 line
2.15.1
author
#11 lore raw

PATCH 11/12 firmware: arm_scpi: use FIELD_GET/_PREP to simplify macro definitions

In reply to Heiner Kallweit

Macro definitions can be simplified by making use of the FIELD_GET/_PREP bitfield macros.

Signed-off-by Heiner Kallweit <[email protected]>
---
 drivers/firmware/arm_scpi.c | 38 +++++++++++++++-----------------------
 1 file changed, 15 insertions(+), 23 deletions(-)
drivers/firmware/arm_scpi.c +15 -23
index 5695b1f4..bc7055a6 100644
@@ -46,27 +46,19 @@
 #include <linux/sort.h>
 #include <linux/spinlock.h>
 
-#define CMD_ID_SHIFT		0
-#define CMD_ID_MASK		0x7f
-#define CMD_TOKEN_ID_SHIFT	8
-#define CMD_TOKEN_ID_MASK	0xff
-#define CMD_DATA_SIZE_SHIFT	16
-#define CMD_DATA_SIZE_MASK	0x1ff
-#define CMD_LEGACY_DATA_SIZE_SHIFT	20
-#define CMD_LEGACY_DATA_SIZE_MASK	0x1ff
-#define PACK_SCPI_CMD(cmd_id, tx_sz)			\
-	((((cmd_id) & CMD_ID_MASK) << CMD_ID_SHIFT) |	\
-	(((tx_sz) & CMD_DATA_SIZE_MASK) << CMD_DATA_SIZE_SHIFT))
-#define ADD_SCPI_TOKEN(cmd, token)			\
-	((cmd) |= (((token) & CMD_TOKEN_ID_MASK) << CMD_TOKEN_ID_SHIFT))
-#define PACK_LEGACY_SCPI_CMD(cmd_id, tx_sz)				\
-	((((cmd_id) & CMD_ID_MASK) << CMD_ID_SHIFT) |			       \
-	(((tx_sz) & CMD_LEGACY_DATA_SIZE_MASK) << CMD_LEGACY_DATA_SIZE_SHIFT))
-
-#define CMD_SIZE(cmd)	(((cmd) >> CMD_DATA_SIZE_SHIFT) & CMD_DATA_SIZE_MASK)
-#define CMD_LEGACY_SIZE(cmd)	(((cmd) >> CMD_LEGACY_DATA_SIZE_SHIFT) & \
-					CMD_LEGACY_DATA_SIZE_MASK)
-#define CMD_UNIQ_MASK	(CMD_TOKEN_ID_MASK << CMD_TOKEN_ID_SHIFT | CMD_ID_MASK)
+#define CMD_ID_MASK		GENMASK(6, 0)
+#define CMD_TOKEN_ID_MASK	GENMASK(15, 8)
+#define CMD_DATA_SIZE_MASK	GENMASK(24, 16)
+#define CMD_LEGACY_DATA_SIZE_MASK	GENMASK(28, 20)
+#define PACK_SCPI_CMD(cmd_id, tx_sz)		\
+	(FIELD_PREP(CMD_ID_MASK, cmd_id) |	\
+	FIELD_PREP(CMD_DATA_SIZE_MASK, tx_sz))
+#define PACK_LEGACY_SCPI_CMD(cmd_id, tx_sz)	\
+	(FIELD_PREP(CMD_ID_MASK, cmd_id) |	\
+	FIELD_PREP(CMD_LEGACY_DATA_SIZE_MASK, tx_sz))
+
+#define CMD_SIZE(cmd)	FIELD_GET(CMD_DATA_SIZE_MASK, cmd)
+#define CMD_UNIQ_MASK	(CMD_TOKEN_ID_MASK | CMD_ID_MASK)
 #define CMD_XTRACT_UNIQ(cmd)	((cmd) & CMD_UNIQ_MASK)
 
 #define SCPI_SLOT		0

@@ -412,7 +404,7 @@ static void scpi_process_cmd(struct scpi_chan *ch, u32 cmd)
 		} else {
 			struct scpi_shared_mem __iomem *mem = ch->rx_payload;
 
-			len = min(match->rx_len, CMD_SIZE(cmd));
+			len = min_t(unsigned int, match->rx_len, CMD_SIZE(cmd));
 
 			match->status = ioread32(&mem->status);
 			memcpy_fromio(match->rx_buf, mem->payload, len);

@@ -454,7 +446,7 @@ static void scpi_tx_prepare(struct mbox_client *c, void *msg)
 	if (t->rx_buf) {
 		if (!(++ch->token))
 			++ch->token;
-		ADD_SCPI_TOKEN(t->cmd, ch->token);
+		t->cmd |= FIELD_PREP(CMD_TOKEN_ID_MASK, ch->token);
 		spin_lock_irqsave(&ch->rx_lock, flags);
 		list_add_tail(&t->node, &ch->rx_pending);
 		spin_unlock_irqrestore(&ch->rx_lock, flags);
Signature · 1 line
2.15.1
author
#12 lore raw

PATCH 12/12 firmware: arm_scpi: improve info message for pre-1.0 firmware

In reply to Heiner Kallweit

On legacy pre-1.0 firmware versions so far the following message is printed which may cause some confusion:
SCP Protocol 0.0 Firmware 0.0.0 version

Therefore replace the message with the following if firmware doesn't provide usable version information:
SCP Protocol legacy pre-1.0 firmware

Signed-off-by Heiner Kallweit <[email protected]>
---
 drivers/firmware/arm_scpi.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)
drivers/firmware/arm_scpi.c +15 -6
index bc7055a6..6d7a6c0a 100644
@@ -1005,12 +1005,21 @@ static int scpi_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	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));
+	if (scpi_info->is_legacy && !scpi_info->protocol_version &&
+	    !scpi_info->firmware_version)
+		dev_info(dev, "SCP Protocol legacy pre-1.0 firmware\n");
+	else
+		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));
 	scpi_info->scpi_ops = &scpi_ops;
 
 	ret = devm_device_add_groups(dev, versions_groups);
Signature · 1 line
2.15.1
#13 lore raw

cover firmware: arm_scpi: series with smaller improvements

In reply to Heiner Kallweit

Hi Heiner,

(Sorry for the late response, was off for a month)

Quoted 10 lines · Heiner Kallweit

After recent revert of all changes since 4.14 this is a resubmit of the patch series, reduced to patches which should not cause any regression.
Based on a remark from Kevin I added one patch for improving the version info for pre-1.0 firmware that doesn't provide version information.

Best should be to apply it to a devel branch first so that the Baylibre Amlogic team can test before mainlining.

I am still waiting for tested by from Amlogic team for this series. However I prefer even the patch you dropped as causing the issue to be tested as it's nice cleanup to have. I strongly think that the issue Kevin reported was due to broken DVFS in the scp firmware in which case the solution is to have the dvfs node in DT removed or disabled rather than not having that patch at all.

Please post that patch on rebased on top of this series and get it tested with DVFS nodes disabled.

Signature · 2 lines
Regards,
Sudeep
maintainer
#14 lore raw

cover firmware: arm_scpi: series with smaller improvements

Quoted 12 lines · Sudeep Holla

Hi Heiner,

(Sorry for the late response, was off for a month)

Quoted 7 lines · Heiner Kallweit

After recent revert of all changes since 4.14 this is a resubmit of the patch series, reduced to patches which should not cause any regression.
Based on a remark from Kevin I added one patch for improving the version info for pre-1.0 firmware that doesn't provide version information.

Quoted 5 lines
Quoted 3 lines

Best should be to apply it to a devel branch first so that the Baylibre Amlogic team can test before mainlining.

I am still waiting for tested by from Amlogic team for this series.

I (finally) tested this series on a few of the boards that were failing last time, and those issues are gone.

So, for the series...

Tested-by Kevin Hilman <[email protected]>
Quoted 5 lines

However I prefer even the patch you dropped as causing the issue to be tested as it's nice cleanup to have. I strongly think that the issue Kevin reported was due to broken DVFS in the scp firmware in which case the solution is to have the dvfs node in DT removed or disabled rather than not having that patch at all.

Yes, I suspect we're up against broken firmware also.

Quoted 2 lines

Please post that patch on rebased on top of this series and get it tested with DVFS nodes disabled.

I'll give that a test as well,

Thanks,

Kevin

#15 lore raw

cover firmware: arm_scpi: series with smaller improvements

Quoted 18 lines · Kevin Hilman
Quoted 16 lines · Sudeep Holla

Hi Heiner,

(Sorry for the late response, was off for a month)

Quoted 10 lines · Heiner Kallweit

After recent revert of all changes since 4.14 this is a resubmit of the patch series, reduced to patches which should not cause any regression.
Based on a remark from Kevin I added one patch for improving the version info for pre-1.0 firmware that doesn't provide version information.

Best should be to apply it to a devel branch first so that the Baylibre Amlogic team can test before mainlining.

Quoted 9 lines
Quoted 1 line

I am still waiting for tested by from Amlogic team for this series.

I (finally) tested this series on a few of the boards that were failing last time, and those issues are gone.

So, for the series...

Tested-by Kevin Hilman <[email protected]>

Thanks, pulled the series[1] with Kevin's tested-by now.

Signature · 4 lines
Regards,
Sudeep

[1] https://git.kernel.org/sudeep.holla/linux/h/for-next/scpi