[PATCH v4 0/8] Add support for legacy SCPI protocol

[PATCH v4 0/8] Add support for legacy SCPI protocol

[Neil Armstrong]

This patchset aims to support the legacy SCPI firmware implementation that was
delivered as early technology preview for the JUNO platform.

Finally a stable, maintained and public implementation for the SCPI protocol
has been upstreamed part of the JUNO support and it is the recommended way
of implementing SCP communication on ARMv8 platforms.

The Amlogic GXBB platform is using this legacy protocol, as the RK3368 & RK3399
platforms. This patchset will only add support for Amlogic GXBB SoC.

This patchset add support for the legacy protocol in the arm_scpi.c file,
avoiding code duplication.

Last RFC discution tread can be found at : https://lkml.org/lkml/2016/8/9/210

Changes since v3 at : http://lkml.kernel.org/r/1473262477-18045-1-git-send-email-narmstrong at baylibre.com
 - Changed back author to Sudeep Holla for first patch
 - Merged legacy functions to scpi_send_message, tx_prepare and handle_remote_message
 - Added legacy locking scheme
 - Merged back legacy_scpi_sensor_get_value into scpi_sensor_get_value
 - Rebased on linux-next-20161004 with patchset [1]

Changes since v2 at : http://lkml.kernel.org/r/1471952816-30877-1-git-send-email-narmstrong at baylibre.com
 - Added command indirection table and use it in each commands
 - Added bitmap for high priority commands
 - Cleaned up legacy tx_prepare/handle_message to align to standard functions
 - Dropped legacy_scpi_ops

Changes since v1 at : http://lkml.kernel.org/r/1471515066-3626-1-git-send-email-narmstrong at baylibre.com
 - Dropped vendor_send_message and rockchip vendor mechanism patches
 - Merged alternate functions into main functions using is_legacy boolean
 - Added DT match table to set is_legacy to true
 - Kept alternate scpi_ops structure for legacy

[1] http://lkml.kernel.org/r/1475595430-30075-1-git-send-email-narmstrong at baylibre.com

Neil Armstrong (7):
  scpi: Add alternative legacy structures, functions and macros
  scpi: Do not fail if get_capabilities is not implemented
  scpi: Add support for Legacy match table for Amlogic GXBB SoC
  scpi: grow MAX_DVFS_OPPS to 16 entries
  dt-bindings: Add support for Amlogic GXBB SCPI Interface
  ARM64: dts: meson-gxbb: Add SRAM node
  ARM64: dts: meson-gxbb: Add SCPI with cpufreq & sensors Nodes

Sudeep Holla (1):
  scpi: Add cmd indirection table to prepare for legacy commands

 Documentation/devicetree/bindings/arm/arm,scpi.txt |   8 +-
 arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi        |  57 ++++
 drivers/firmware/arm_scpi.c                        | 317 ++++++++++++++++++---
 3 files changed, 339 insertions(+), 43 deletions(-)

-- 
1.9.1

[PATCH v4 1/8] scpi: Add cmd indirection table to prepare for legacy commands

[Neil Armstrong]

From: Sudeep Holla <sudeep.holla@arm.com>

Add indirection table to permit multiple command values for legacy support.

[narmstrong at baylibre.com: Added cmd check in scpi_send_message]

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/firmware/arm_scpi.c | 81 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 64 insertions(+), 17 deletions(-)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index ce2bc2a..498afa0 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -99,6 +99,7 @@ enum scpi_error_codes {
 	SCPI_ERR_MAX
 };
 
+/* SCPI Standard commands */
 enum scpi_std_cmd {
 	SCPI_CMD_INVALID		= 0x00,
 	SCPI_CMD_SCPI_READY		= 0x01,
@@ -132,6 +133,38 @@ enum scpi_std_cmd {
 	SCPI_CMD_COUNT
 };
 
+/* List all commands used by this driver, used as indexes */
+enum scpi_drv_cmds {
+	CMD_SCPI_CAPABILITIES = 0,
+	CMD_GET_CLOCK_INFO,
+	CMD_GET_CLOCK_VALUE,
+	CMD_SET_CLOCK_VALUE,
+	CMD_GET_DVFS,
+	CMD_SET_DVFS,
+	CMD_GET_DVFS_INFO,
+	CMD_SENSOR_CAPABILITIES,
+	CMD_SENSOR_INFO,
+	CMD_SENSOR_VALUE,
+	CMD_SET_DEVICE_PWR_STATE,
+	CMD_GET_DEVICE_PWR_STATE,
+	CMD_MAX_COUNT,
+};
+
+static int scpi_std_commands[CMD_MAX_COUNT] = {
+	SCPI_CMD_SCPI_CAPABILITIES,
+	SCPI_CMD_GET_CLOCK_INFO,
+	SCPI_CMD_GET_CLOCK_VALUE,
+	SCPI_CMD_SET_CLOCK_VALUE,
+	SCPI_CMD_GET_DVFS,
+	SCPI_CMD_SET_DVFS,
+	SCPI_CMD_GET_DVFS_INFO,
+	SCPI_CMD_SENSOR_CAPABILITIES,
+	SCPI_CMD_SENSOR_INFO,
+	SCPI_CMD_SENSOR_VALUE,
+	SCPI_CMD_SET_DEVICE_PWR_STATE,
+	SCPI_CMD_GET_DEVICE_PWR_STATE,
+};
+
 struct scpi_xfer {
 	u32 slot; /* has to be first element */
 	u32 cmd;
@@ -161,6 +194,7 @@ struct scpi_drvinfo {
 	u32 protocol_version;
 	u32 firmware_version;
 	int num_chans;
+	int *scpi_cmds;
 	atomic_t next_chan;
 	struct scpi_ops *scpi_ops;
 	struct scpi_chan *channels;
@@ -344,14 +378,24 @@ static void put_scpi_xfer(struct scpi_xfer *t, struct scpi_chan *ch)
 	mutex_unlock(&ch->xfers_lock);
 }
 
-static int scpi_send_message(u8 cmd, void *tx_buf, unsigned int tx_len,
-			     void *rx_buf, unsigned int rx_len)
+static int scpi_send_message(unsigned int offset, void *tx_buf,
+			     unsigned int tx_len, void *rx_buf,
+			     unsigned int rx_len)
 {
 	int ret;
 	u8 chan;
+	u8 cmd;
 	struct scpi_xfer *msg;
 	struct scpi_chan *scpi_chan;
 
+	if (offset >= CMD_MAX_COUNT || !scpi_info->scpi_cmds)
+		return -EINVAL;
+
+	if (scpi_info->scpi_cmds[offset] < 0)
+		return -EOPNOTSUPP;
+
+	cmd = scpi_info->scpi_cmds[offset];
+
 	chan = atomic_inc_return(&scpi_info->next_chan) % scpi_info->num_chans;
 	scpi_chan = scpi_info->channels + chan;
 
@@ -397,7 +441,7 @@ static u32 scpi_get_version(void)
 	struct clk_get_info clk;
 	__le16 le_clk_id = cpu_to_le16(clk_id);
 
-	ret = scpi_send_message(SCPI_CMD_GET_CLOCK_INFO, &le_clk_id,
+	ret = scpi_send_message(CMD_GET_CLOCK_INFO, &le_clk_id,
 				sizeof(le_clk_id), &clk, sizeof(clk));
 	if (!ret) {
 		*min = le32_to_cpu(clk.min_rate);
@@ -412,8 +456,9 @@ static unsigned long scpi_clk_get_val(u16 clk_id)
 	struct clk_get_value clk;
 	__le16 le_clk_id = cpu_to_le16(clk_id);
 
-	ret = scpi_send_message(SCPI_CMD_GET_CLOCK_VALUE, &le_clk_id,
+	ret = scpi_send_message(CMD_GET_CLOCK_VALUE, &le_clk_id,
 				sizeof(le_clk_id), &clk, sizeof(clk));
+
 	return ret ? ret : le32_to_cpu(clk.rate);
 }
 
@@ -425,7 +470,7 @@ static int scpi_clk_set_val(u16 clk_id, unsigned long rate)
 		.rate = cpu_to_le32(rate)
 	};
 
-	return scpi_send_message(SCPI_CMD_SET_CLOCK_VALUE, &clk, sizeof(clk),
+	return scpi_send_message(CMD_SET_CLOCK_VALUE, &clk, sizeof(clk),
 				 &stat, sizeof(stat));
 }
 
@@ -434,8 +479,9 @@ static int scpi_dvfs_get_idx(u8 domain)
 	int ret;
 	u8 dvfs_idx;
 
-	ret = scpi_send_message(SCPI_CMD_GET_DVFS, &domain, sizeof(domain),
+	ret = scpi_send_message(CMD_GET_DVFS, &domain, sizeof(domain),
 				&dvfs_idx, sizeof(dvfs_idx));
+
 	return ret ? ret : dvfs_idx;
 }
 
@@ -444,7 +490,7 @@ static int scpi_dvfs_set_idx(u8 domain, u8 index)
 	int stat;
 	struct dvfs_set dvfs = {domain, index};
 
-	return scpi_send_message(SCPI_CMD_SET_DVFS, &dvfs, sizeof(dvfs),
+	return scpi_send_message(CMD_SET_DVFS, &dvfs, sizeof(dvfs),
 				 &stat, sizeof(stat));
 }
 
@@ -468,9 +514,8 @@ static struct scpi_dvfs_info *scpi_dvfs_get_info(u8 domain)
 	if (scpi_info->dvfs[domain])	/* data already populated */
 		return scpi_info->dvfs[domain];
 
-	ret = scpi_send_message(SCPI_CMD_GET_DVFS_INFO, &domain, sizeof(domain),
+	ret = scpi_send_message(CMD_GET_DVFS_INFO, &domain, sizeof(domain),
 				&buf, sizeof(buf));
-
 	if (ret)
 		return ERR_PTR(ret);
 
@@ -503,8 +548,8 @@ static int scpi_sensor_get_capability(u16 *sensors)
 	struct sensor_capabilities cap_buf;
 	int ret;
 
-	ret = scpi_send_message(SCPI_CMD_SENSOR_CAPABILITIES, NULL, 0, &cap_buf,
-				sizeof(cap_buf));
+	ret = scpi_send_message(CMD_SENSOR_CAPABILITIES, NULL, 0,
+				&cap_buf, sizeof(cap_buf));
 	if (!ret)
 		*sensors = le16_to_cpu(cap_buf.sensors);
 
@@ -517,7 +562,7 @@ static int scpi_sensor_get_info(u16 sensor_id, struct scpi_sensor_info *info)
 	struct _scpi_sensor_info _info;
 	int ret;
 
-	ret = scpi_send_message(SCPI_CMD_SENSOR_INFO, &id, sizeof(id),
+	ret = scpi_send_message(CMD_SENSOR_INFO, &id, sizeof(id),
 				&_info, sizeof(_info));
 	if (!ret) {
 		memcpy(info, &_info, sizeof(*info));
@@ -533,7 +578,7 @@ static int scpi_sensor_get_value(u16 sensor, u64 *val)
 	struct sensor_value buf;
 	int ret;
 
-	ret = scpi_send_message(SCPI_CMD_SENSOR_VALUE, &id, sizeof(id),
+	ret = scpi_send_message(CMD_SENSOR_VALUE, &id, sizeof(id),
 				&buf, sizeof(buf));
 	if (!ret)
 		*val = (u64)le32_to_cpu(buf.hi_val) << 32 |
@@ -548,8 +593,8 @@ static int scpi_device_get_power_state(u16 dev_id)
 	u8 pstate;
 	__le16 id = cpu_to_le16(dev_id);
 
-	ret = scpi_send_message(SCPI_CMD_GET_DEVICE_PWR_STATE, &id,
-				sizeof(id), &pstate, sizeof(pstate));
+	ret = scpi_send_message(CMD_GET_DEVICE_PWR_STATE, &id, sizeof(id),
+				&pstate, sizeof(pstate));
 	return ret ? ret : pstate;
 }
 
@@ -561,7 +606,7 @@ static int scpi_device_set_power_state(u16 dev_id, u8 pstate)
 		.pstate = pstate,
 	};
 
-	return scpi_send_message(SCPI_CMD_SET_DEVICE_PWR_STATE, &dev_set,
+	return scpi_send_message(CMD_SET_DEVICE_PWR_STATE, &dev_set,
 				 sizeof(dev_set), &stat, sizeof(stat));
 }
 
@@ -591,7 +636,7 @@ static int scpi_init_versions(struct scpi_drvinfo *info)
 	int ret;
 	struct scp_capabilities caps;
 
-	ret = scpi_send_message(SCPI_CMD_SCPI_CAPABILITIES, NULL, 0,
+	ret = scpi_send_message(CMD_SCPI_CAPABILITIES, NULL, 0,
 				&caps, sizeof(caps));
 	if (!ret) {
 		info->protocol_version = le32_to_cpu(caps.protocol_version);
@@ -757,6 +802,8 @@ static int scpi_probe(struct platform_device *pdev)
 	scpi_info->num_chans = count;
 	platform_set_drvdata(pdev, scpi_info);
 
+	scpi_info->scpi_cmds = scpi_std_commands;
+
 	ret = scpi_init_versions(scpi_info);
 	if (ret) {
 		dev_err(dev, "incorrect or no SCP firmware found\n");
-- 
1.9.1

[PATCH v4 2/8] scpi: Add alternative legacy structures, functions and macros

[Neil Armstrong]

This patch adds support for the Legacy SCPI protocol in early JUNO versions and
shipped Amlogic ARMv8 based SoCs. Some Rockchip SoC are also known to use this
version of protocol with extended vendor commands
.
In order to support the legacy SCPI protocol variant, add back the structures
and macros that varies against the final specification.
Then add indirection table for legacy commands.
Finally Add bitmap field for channel selection since the Legacy protocol mandates to
send a selected subset of the commands on the high priority channel instead of the
low priority channel.

The message sending path differs from the final SCPI procotocol because the
Amlogic SCP firmware always reply 1 instead of a special value containing the command
byte and replied rx data length.
For this reason commands queuing cannot be used and we assume the reply command is
the head of the rx_pending list since we ensure sequential command sending with a
separate dedicated mutex.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/firmware/arm_scpi.c | 221 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 199 insertions(+), 22 deletions(-)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index 498afa0..6244eb1 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -50,13 +50,20 @@
 #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_XTRACT_UNIQ(cmd)	((cmd) & CMD_UNIQ_MASK)
 
@@ -133,6 +140,61 @@ enum scpi_std_cmd {
 	SCPI_CMD_COUNT
 };
 
+/* SCPI Legacy Commands */
+enum legacy_scpi_std_cmd {
+	LEGACY_SCPI_CMD_INVALID			= 0x00,
+	LEGACY_SCPI_CMD_SCPI_READY		= 0x01,
+	LEGACY_SCPI_CMD_SCPI_CAPABILITIES	= 0x02,
+	LEGACY_SCPI_CMD_EVENT			= 0x03,
+	LEGACY_SCPI_CMD_SET_CSS_PWR_STATE	= 0x04,
+	LEGACY_SCPI_CMD_GET_CSS_PWR_STATE	= 0x05,
+	LEGACY_SCPI_CMD_CFG_PWR_STATE_STAT	= 0x06,
+	LEGACY_SCPI_CMD_GET_PWR_STATE_STAT	= 0x07,
+	LEGACY_SCPI_CMD_SYS_PWR_STATE		= 0x08,
+	LEGACY_SCPI_CMD_L2_READY		= 0x09,
+	LEGACY_SCPI_CMD_SET_AP_TIMER		= 0x0a,
+	LEGACY_SCPI_CMD_CANCEL_AP_TIME		= 0x0b,
+	LEGACY_SCPI_CMD_DVFS_CAPABILITIES	= 0x0c,
+	LEGACY_SCPI_CMD_GET_DVFS_INFO		= 0x0d,
+	LEGACY_SCPI_CMD_SET_DVFS		= 0x0e,
+	LEGACY_SCPI_CMD_GET_DVFS		= 0x0f,
+	LEGACY_SCPI_CMD_GET_DVFS_STAT		= 0x10,
+	LEGACY_SCPI_CMD_SET_RTC			= 0x11,
+	LEGACY_SCPI_CMD_GET_RTC			= 0x12,
+	LEGACY_SCPI_CMD_CLOCK_CAPABILITIES	= 0x13,
+	LEGACY_SCPI_CMD_SET_CLOCK_INDEX		= 0x14,
+	LEGACY_SCPI_CMD_SET_CLOCK_VALUE		= 0x15,
+	LEGACY_SCPI_CMD_GET_CLOCK_VALUE		= 0x16,
+	LEGACY_SCPI_CMD_PSU_CAPABILITIES	= 0x17,
+	LEGACY_SCPI_CMD_SET_PSU			= 0x18,
+	LEGACY_SCPI_CMD_GET_PSU			= 0x19,
+	LEGACY_SCPI_CMD_SENSOR_CAPABILITIES	= 0x1a,
+	LEGACY_SCPI_CMD_SENSOR_INFO		= 0x1b,
+	LEGACY_SCPI_CMD_SENSOR_VALUE		= 0x1c,
+	LEGACY_SCPI_CMD_SENSOR_CFG_PERIODIC	= 0x1d,
+	LEGACY_SCPI_CMD_SENSOR_CFG_BOUNDS	= 0x1e,
+	LEGACY_SCPI_CMD_SENSOR_ASYNC_VALUE	= 0x1f,
+	LEGACY_SCPI_CMD_COUNT
+};
+
+/* List all commands that are required to go through the high priority link */
+static int legacy_hpriority_cmds[] = {
+	LEGACY_SCPI_CMD_GET_CSS_PWR_STATE,
+	LEGACY_SCPI_CMD_CFG_PWR_STATE_STAT,
+	LEGACY_SCPI_CMD_GET_PWR_STATE_STAT,
+	LEGACY_SCPI_CMD_SET_DVFS,
+	LEGACY_SCPI_CMD_GET_DVFS,
+	LEGACY_SCPI_CMD_SET_RTC,
+	LEGACY_SCPI_CMD_GET_RTC,
+	LEGACY_SCPI_CMD_SET_CLOCK_INDEX,
+	LEGACY_SCPI_CMD_SET_CLOCK_VALUE,
+	LEGACY_SCPI_CMD_GET_CLOCK_VALUE,
+	LEGACY_SCPI_CMD_SET_PSU,
+	LEGACY_SCPI_CMD_GET_PSU,
+	LEGACY_SCPI_CMD_SENSOR_CFG_PERIODIC,
+	LEGACY_SCPI_CMD_SENSOR_CFG_BOUNDS,
+};
+
 /* List all commands used by this driver, used as indexes */
 enum scpi_drv_cmds {
 	CMD_SCPI_CAPABILITIES = 0,
@@ -165,6 +227,21 @@ enum scpi_drv_cmds {
 	SCPI_CMD_GET_DEVICE_PWR_STATE,
 };
 
+static int scpi_legacy_commands[CMD_MAX_COUNT] = {
+	LEGACY_SCPI_CMD_SCPI_CAPABILITIES,
+	-1, /* GET_CLOCK_INFO */
+	LEGACY_SCPI_CMD_GET_CLOCK_VALUE,
+	LEGACY_SCPI_CMD_SET_CLOCK_VALUE,
+	LEGACY_SCPI_CMD_GET_DVFS,
+	LEGACY_SCPI_CMD_SET_DVFS,
+	LEGACY_SCPI_CMD_GET_DVFS_INFO,
+	LEGACY_SCPI_CMD_SENSOR_CAPABILITIES,
+	LEGACY_SCPI_CMD_SENSOR_INFO,
+	LEGACY_SCPI_CMD_SENSOR_VALUE,
+	-1, /* SET_DEVICE_PWR_STATE */
+	-1, /* GET_DEVICE_PWR_STATE */
+};
+
 struct scpi_xfer {
 	u32 slot; /* has to be first element */
 	u32 cmd;
@@ -187,14 +264,17 @@ struct scpi_chan {
 	struct scpi_xfer *xfers;
 	spinlock_t rx_lock; /* locking for the rx pending list */
 	struct mutex xfers_lock;
+	struct mutex legacy_lock;
 	u8 token;
 };
 
 struct scpi_drvinfo {
 	u32 protocol_version;
 	u32 firmware_version;
+	bool is_legacy;
 	int num_chans;
 	int *scpi_cmds;
+	DECLARE_BITMAP(cmd_priority, LEGACY_SCPI_CMD_COUNT);
 	atomic_t next_chan;
 	struct scpi_ops *scpi_ops;
 	struct scpi_chan *channels;
@@ -211,6 +291,11 @@ struct scpi_shared_mem {
 	u8 payload[0];
 } __packed;
 
+struct legacy_scpi_shared_mem {
+	__le32 status;
+	u8 payload[0];
+} __packed;
+
 struct scp_capabilities {
 	__le32 protocol_version;
 	__le32 event_version;
@@ -236,6 +321,12 @@ struct clk_set_value {
 	__le32 rate;
 } __packed;
 
+struct legacy_clk_set_value {
+	__le32 rate;
+	__le16 id;
+	__le16 reserved;
+} __packed;
+
 struct dvfs_info {
 	__le32 header;
 	struct {
@@ -307,21 +398,46 @@ static void scpi_process_cmd(struct scpi_chan *ch, u32 cmd)
 		return;
 	}
 
-	list_for_each_entry(t, &ch->rx_pending, node)
-		if (CMD_XTRACT_UNIQ(t->cmd) == CMD_XTRACT_UNIQ(cmd)) {
-			list_del(&t->node);
-			match = t;
-			break;
-		}
+	/* Command type is not replied by the SCP Firmware in legacy Mode
+	 * We should consider that command is the head of pending RX commands
+	 * if the list is not empty. In TX only mode, the list would be empty.
+	 */
+	if (scpi_info->is_legacy) {
+		match = list_first_entry(&ch->rx_pending, struct scpi_xfer,
+					 node);
+		list_del(&match->node);
+	} else {
+		list_for_each_entry(t, &ch->rx_pending, node)
+			if (CMD_XTRACT_UNIQ(t->cmd) == CMD_XTRACT_UNIQ(cmd)) {
+				list_del(&t->node);
+				match = t;
+				break;
+			}
+	}
 	/* check if wait_for_completion is in progress or timed-out */
 	if (match && !completion_done(&match->done)) {
-		struct scpi_shared_mem *mem = ch->rx_payload;
-		unsigned int len = min(match->rx_len, CMD_SIZE(cmd));
+		unsigned int len;
+
+		if (scpi_info->is_legacy) {
+			struct legacy_scpi_shared_mem *mem = ch->rx_payload;
+
+			/* RX Length is not replied by the lagcy Firmware */
+			len = match->rx_len;
+
+			match->status = le32_to_cpu(mem->status);
+			memcpy_fromio(match->rx_buf, mem->payload, len);
+		} else {
+			struct scpi_shared_mem *mem = ch->rx_payload;
+
+			len = min(match->rx_len, CMD_SIZE(cmd));
+
+			match->status = le32_to_cpu(mem->status);
+			memcpy_fromio(match->rx_buf, mem->payload, len);
+		}
 
-		match->status = le32_to_cpu(mem->status);
-		memcpy_fromio(match->rx_buf, mem->payload, len);
 		if (match->rx_len > len)
 			memset(match->rx_buf + len, 0, match->rx_len - len);
+
 		complete(&match->done);
 	}
 	spin_unlock_irqrestore(&ch->rx_lock, flags);
@@ -331,7 +447,12 @@ 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;
-	u32 cmd = le32_to_cpu(mem->command);
+	u32 cmd;
+
+	if (scpi_info->is_legacy)
+		cmd = *(u32 *)msg;
+	else
+		cmd = le32_to_cpu(mem->command);
 
 	scpi_process_cmd(ch, cmd);
 }
@@ -343,17 +464,26 @@ static void scpi_tx_prepare(struct mbox_client *c, void *msg)
 	struct scpi_chan *ch = container_of(c, struct scpi_chan, cl);
 	struct scpi_shared_mem *mem = (struct scpi_shared_mem *)ch->tx_payload;
 
-	if (t->tx_buf)
-		memcpy_toio(mem->payload, t->tx_buf, t->tx_len);
+	if (t->tx_buf) {
+		if (scpi_info->is_legacy)
+			memcpy_toio(ch->tx_payload, t->tx_buf, t->tx_len);
+		else
+			memcpy_toio(mem->payload, t->tx_buf, t->tx_len);
+	}
+
 	if (t->rx_buf) {
 		if (!(++ch->token))
 			++ch->token;
 		ADD_SCPI_TOKEN(t->cmd, ch->token);
+		if (scpi_info->is_legacy)
+			t->slot = t->cmd;
 		spin_lock_irqsave(&ch->rx_lock, flags);
 		list_add_tail(&t->node, &ch->rx_pending);
 		spin_unlock_irqrestore(&ch->rx_lock, flags);
 	}
-	mem->command = cpu_to_le32(t->cmd);
+
+	if (!scpi_info->is_legacy)
+		mem->command = cpu_to_le32(t->cmd);
 }
 
 static struct scpi_xfer *get_scpi_xfer(struct scpi_chan *ch)
@@ -396,21 +526,37 @@ static int scpi_send_message(unsigned int offset, void *tx_buf,
 
 	cmd = scpi_info->scpi_cmds[offset];
 
-	chan = atomic_inc_return(&scpi_info->next_chan) % scpi_info->num_chans;
+	if (scpi_info->is_legacy)
+		chan = test_bit(cmd, scpi_info->cmd_priority) ? 1 : 0;
+	else
+		chan = atomic_inc_return(&scpi_info->next_chan) %
+			scpi_info->num_chans;
 	scpi_chan = scpi_info->channels + chan;
 
 	msg = get_scpi_xfer(scpi_chan);
 	if (!msg)
 		return -ENOMEM;
 
-	msg->slot = BIT(SCPI_SLOT);
-	msg->cmd = PACK_SCPI_CMD(cmd, tx_len);
+	if (scpi_info->is_legacy) {
+		msg->cmd = PACK_LEGACY_SCPI_CMD(cmd, tx_len);
+		msg->slot = msg->cmd;
+	} else {
+		msg->slot = BIT(SCPI_SLOT);
+		msg->cmd = PACK_SCPI_CMD(cmd, tx_len);
+	}
 	msg->tx_buf = tx_buf;
 	msg->tx_len = tx_len;
 	msg->rx_buf = rx_buf;
 	msg->rx_len = rx_len;
 	init_completion(&msg->done);
 
+	/* Since we cannot distinguish the original command in the
+	 * MHU reply stat value from a Legacy SCP firmware, ensure
+	 * sequential command sending to the firmware.
+	 */
+	if (scpi_info->is_legacy)
+		mutex_lock(&scpi_chan->legacy_lock);
+
 	ret = mbox_send_message(scpi_chan->chan, msg);
 	if (ret < 0 || !rx_buf)
 		goto out;
@@ -421,9 +567,13 @@ static int scpi_send_message(unsigned int offset, void *tx_buf,
 		/* first status word */
 		ret = msg->status;
 out:
-	if (ret < 0 && rx_buf) /* remove entry from the list if timed-out */
+	if (ret < 0 && rx_buf)
+		/* remove entry from the list if timed-out */
 		scpi_process_cmd(scpi_chan, msg->cmd);
 
+	if (scpi_info->is_legacy)
+		mutex_unlock(&scpi_chan->legacy_lock);
+
 	put_scpi_xfer(msg, scpi_chan);
 	/* SCPI error codes > 0, translate them to Linux scale*/
 	return ret > 0 ? scpi_to_linux_errno(ret) : ret;
@@ -474,6 +624,18 @@ static int scpi_clk_set_val(u16 clk_id, unsigned long rate)
 				 &stat, sizeof(stat));
 }
 
+static int legacy_scpi_clk_set_val(u16 clk_id, unsigned long rate)
+{
+	int stat;
+	struct legacy_clk_set_value clk = {
+		.id = cpu_to_le16(clk_id),
+		.rate = cpu_to_le32(rate)
+	};
+
+	return scpi_send_message(CMD_SET_CLOCK_VALUE, &clk, sizeof(clk),
+				 &stat, sizeof(stat));
+}
+
 static int scpi_dvfs_get_idx(u8 domain)
 {
 	int ret;
@@ -525,7 +687,6 @@ static struct scpi_dvfs_info *scpi_dvfs_get_info(u8 domain)
 
 	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);
@@ -580,9 +741,13 @@ static int scpi_sensor_get_value(u16 sensor, u64 *val)
 
 	ret = scpi_send_message(CMD_SENSOR_VALUE, &id, sizeof(id),
 				&buf, sizeof(buf));
-	if (!ret)
-		*val = (u64)le32_to_cpu(buf.hi_val) << 32 |
-			le32_to_cpu(buf.lo_val);
+	if (!ret) {
+		if (scpi_info->is_legacy)
+			*val = (u64)le32_to_cpu(buf.lo_val);
+		else
+			*val = (u64)le32_to_cpu(buf.hi_val) << 32 |
+				le32_to_cpu(buf.lo_val);
+	}
 
 	return ret;
 }
@@ -781,6 +946,7 @@ static int scpi_probe(struct platform_device *pdev)
 		INIT_LIST_HEAD(&pchan->xfers_list);
 		spin_lock_init(&pchan->rx_lock);
 		mutex_init(&pchan->xfers_lock);
+		mutex_init(&pchan->legacy_lock);
 
 		ret = scpi_alloc_xfer_list(dev, pchan);
 		if (!ret) {
@@ -804,6 +970,17 @@ static int scpi_probe(struct platform_device *pdev)
 
 	scpi_info->scpi_cmds = scpi_std_commands;
 
+	if (scpi_info->is_legacy) {
+		/* Replace with legacy variants */
+		scpi_ops.clk_set_val = legacy_scpi_clk_set_val;
+		scpi_info->scpi_cmds = scpi_legacy_commands;
+
+		/* Fill priority bitmap */
+		for (idx = 0; idx < ARRAY_SIZE(legacy_hpriority_cmds); idx++)
+			set_bit(legacy_hpriority_cmds[idx],
+				scpi_info->cmd_priority);
+	}
+
 	ret = scpi_init_versions(scpi_info);
 	if (ret) {
 		dev_err(dev, "incorrect or no SCP firmware found\n");
-- 
1.9.1

[PATCH v4 2/8] scpi: Add alternative legacy structures, functions and macros

[Sudeep Holla]

Hi Neil,

Sorry, I could not reply to your response on v3. Anyways I will review v4.

On 05/10/16 08:33, Neil Armstrong wrote:
> This patch adds support for the Legacy SCPI protocol in early JUNO versions and
> shipped Amlogic ARMv8 based SoCs. Some Rockchip SoC are also known to use this
> version of protocol with extended vendor commands
> .
> In order to support the legacy SCPI protocol variant, add back the structures
> and macros that varies against the final specification.
> Then add indirection table for legacy commands.
> Finally Add bitmap field for channel selection since the Legacy protocol mandates to
> send a selected subset of the commands on the high priority channel instead of the
> low priority channel.
>
> The message sending path differs from the final SCPI procotocol because the
> Amlogic SCP firmware always reply 1 instead of a special value containing the command
> byte and replied rx data length.
> For this reason commands queuing cannot be used and we assume the reply command is
> the head of the rx_pending list since we ensure sequential command sending with a
> separate dedicated mutex.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/firmware/arm_scpi.c | 221 +++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 199 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
> index 498afa0..6244eb1 100644
> --- a/drivers/firmware/arm_scpi.c
> +++ b/drivers/firmware/arm_scpi.c

[...]

> @@ -307,21 +398,46 @@ static void scpi_process_cmd(struct scpi_chan *ch, u32 cmd)
>  		return;
>  	}
>
> -	list_for_each_entry(t, &ch->rx_pending, node)
> -		if (CMD_XTRACT_UNIQ(t->cmd) == CMD_XTRACT_UNIQ(cmd)) {
> -			list_del(&t->node);
> -			match = t;
> -			break;
> -		}
> +	/* Command type is not replied by the SCP Firmware in legacy Mode
> +	 * We should consider that command is the head of pending RX commands
> +	 * if the list is not empty. In TX only mode, the list would be empty.
> +	 */
> +	if (scpi_info->is_legacy) {
> +		match = list_first_entry(&ch->rx_pending, struct scpi_xfer,
> +					 node);
> +		list_del(&match->node);
> +	} else {
> +		list_for_each_entry(t, &ch->rx_pending, node)
> +			if (CMD_XTRACT_UNIQ(t->cmd) == CMD_XTRACT_UNIQ(cmd)) {
> +				list_del(&t->node);
> +				match = t;
> +				break;
> +			}
> +	}
>  	/* check if wait_for_completion is in progress or timed-out */
>  	if (match && !completion_done(&match->done)) {
> -		struct scpi_shared_mem *mem = ch->rx_payload;
> -		unsigned int len = min(match->rx_len, CMD_SIZE(cmd));
> +		unsigned int len;
> +
> +		if (scpi_info->is_legacy) {
> +			struct legacy_scpi_shared_mem *mem = ch->rx_payload;
> +
> +			/* RX Length is not replied by the lagcy Firmware */
> +			len = match->rx_len;
> +
> +			match->status = le32_to_cpu(mem->status);
> +			memcpy_fromio(match->rx_buf, mem->payload, len);

The above 2 seems common to both, no ?

> +		} else {
> +			struct scpi_shared_mem *mem = ch->rx_payload;
> +
> +			len = min(match->rx_len, CMD_SIZE(cmd));
> +
> +			match->status = le32_to_cpu(mem->status);
> +			memcpy_fromio(match->rx_buf, mem->payload, len);
> +		}
>
> -		match->status = le32_to_cpu(mem->status);
> -		memcpy_fromio(match->rx_buf, mem->payload, len);
>  		if (match->rx_len > len)
>  			memset(match->rx_buf + len, 0, match->rx_len - len);
> +

Spurious ?

>  		complete(&match->done);
>  	}
>  	spin_unlock_irqrestore(&ch->rx_lock, flags);
> @@ -331,7 +447,12 @@ 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;
> -	u32 cmd = le32_to_cpu(mem->command);
> +	u32 cmd;
> +
> +	if (scpi_info->is_legacy)
> +		cmd = *(u32 *)msg;

Do we need do this if it doesn't contain command ?

> +	else
> +		cmd = le32_to_cpu(mem->command);
>
>  	scpi_process_cmd(ch, cmd);
>  }
> @@ -343,17 +464,26 @@ static void scpi_tx_prepare(struct mbox_client *c, void *msg)
>  	struct scpi_chan *ch = container_of(c, struct scpi_chan, cl);
>  	struct scpi_shared_mem *mem = (struct scpi_shared_mem *)ch->tx_payload;
>
> -	if (t->tx_buf)
> -		memcpy_toio(mem->payload, t->tx_buf, t->tx_len);
> +	if (t->tx_buf) {
> +		if (scpi_info->is_legacy)
> +			memcpy_toio(ch->tx_payload, t->tx_buf, t->tx_len);
> +		else
> +			memcpy_toio(mem->payload, t->tx_buf, t->tx_len);
> +	}
> +
>  	if (t->rx_buf) {
>  		if (!(++ch->token))
>  			++ch->token;
>  		ADD_SCPI_TOKEN(t->cmd, ch->token);
> +		if (scpi_info->is_legacy)
> +			t->slot = t->cmd;

I thought passing token was not an issue from your previous response,
but you are overriding it here, why ?

>  		spin_lock_irqsave(&ch->rx_lock, flags);
>  		list_add_tail(&t->node, &ch->rx_pending);
>  		spin_unlock_irqrestore(&ch->rx_lock, flags);
>  	}
> -	mem->command = cpu_to_le32(t->cmd);
> +
> +	if (!scpi_info->is_legacy)
> +		mem->command = cpu_to_le32(t->cmd);
>  }
>
>  static struct scpi_xfer *get_scpi_xfer(struct scpi_chan *ch)
> @@ -396,21 +526,37 @@ static int scpi_send_message(unsigned int offset, void *tx_buf,
>
>  	cmd = scpi_info->scpi_cmds[offset];
>
> -	chan = atomic_inc_return(&scpi_info->next_chan) % scpi_info->num_chans;
> +	if (scpi_info->is_legacy)
> +		chan = test_bit(cmd, scpi_info->cmd_priority) ? 1 : 0;
> +	else
> +		chan = atomic_inc_return(&scpi_info->next_chan) %
> +			scpi_info->num_chans;
>  	scpi_chan = scpi_info->channels + chan;
>
>  	msg = get_scpi_xfer(scpi_chan);
>  	if (!msg)
>  		return -ENOMEM;
>
> -	msg->slot = BIT(SCPI_SLOT);
> -	msg->cmd = PACK_SCPI_CMD(cmd, tx_len);
> +	if (scpi_info->is_legacy) {
> +		msg->cmd = PACK_LEGACY_SCPI_CMD(cmd, tx_len);
> +		msg->slot = msg->cmd;
> +	} else {
> +		msg->slot = BIT(SCPI_SLOT);
> +		msg->cmd = PACK_SCPI_CMD(cmd, tx_len);
> +	}
>  	msg->tx_buf = tx_buf;
>  	msg->tx_len = tx_len;
>  	msg->rx_buf = rx_buf;
>  	msg->rx_len = rx_len;
>  	init_completion(&msg->done);
>
> +	/* Since we cannot distinguish the original command in the
> +	 * MHU reply stat value from a Legacy SCP firmware, ensure
> +	 * sequential command sending to the firmware.
> +	 */

OK this comment now questions the existence of this extra lock.
The mailbox will always send the commands in the sequential order.
It's only firmware that can re-order the response. Since that can't
happen in you case, I really don't see the need for this.

Please explain the race you would see without this locking. Yes I
understand that only one command is supposed to be sent to firmware at a
time. Suppose you allow more callers here, all will wait on the
completion flags and the first in the list gets unblocked right ?
I am just trying to understand if there's real need for this extra
lock when we already have that from the list.

> +	if (scpi_info->is_legacy)
> +		mutex_lock(&scpi_chan->legacy_lock);
> +
>  	ret = mbox_send_message(scpi_chan->chan, msg);
>  	if (ret < 0 || !rx_buf)
>  		goto out;
> @@ -421,9 +567,13 @@ static int scpi_send_message(unsigned int offset, void *tx_buf,
>  		/* first status word */
>  		ret = msg->status;
>  out:
> -	if (ret < 0 && rx_buf) /* remove entry from the list if timed-out */
> +	if (ret < 0 && rx_buf)
> +		/* remove entry from the list if timed-out */
>  		scpi_process_cmd(scpi_chan, msg->cmd);
>
> +	if (scpi_info->is_legacy)
> +		mutex_unlock(&scpi_chan->legacy_lock);
> +
>  	put_scpi_xfer(msg, scpi_chan);
>  	/* SCPI error codes > 0, translate them to Linux scale*/
>  	return ret > 0 ? scpi_to_linux_errno(ret) : ret;

[...]

> @@ -525,7 +687,6 @@ static struct scpi_dvfs_info *scpi_dvfs_get_info(u8 domain)
>
>  	info->count = DVFS_OPP_COUNT(buf.header);
>  	info->latency = DVFS_LATENCY(buf.header) * 1000; /* uS to nS */
> -

Spurious ?

>  	info->opps = kcalloc(info->count, sizeof(*opp), GFP_KERNEL);
>  	if (!info->opps) {
>  		kfree(info);
> @@ -580,9 +741,13 @@ static int scpi_sensor_get_value(u16 sensor, u64 *val)
>
>  	ret = scpi_send_message(CMD_SENSOR_VALUE, &id, sizeof(id),
>  				&buf, sizeof(buf));
> -	if (!ret)
> -		*val = (u64)le32_to_cpu(buf.hi_val) << 32 |
> -			le32_to_cpu(buf.lo_val);
> +	if (!ret) {
> +		if (scpi_info->is_legacy)
> +			*val = (u64)le32_to_cpu(buf.lo_val);
> +		else
> +			*val = (u64)le32_to_cpu(buf.hi_val) << 32 |
> +				le32_to_cpu(buf.lo_val);
> +	}

Not required as I have mentioned couple of times in previous versions,
it's zero filled by the driver.

-- 
Regards,
Sudeep

[PATCH v4 2/8] scpi: Add alternative legacy structures, functions and macros

[Neil Armstrong]

On 10/10/2016 04:36 PM, Sudeep Holla wrote:
> Hi Neil,
> 
> Sorry, I could not reply to your response on v3. Anyways I will review v4.
> 
> On 05/10/16 08:33, Neil Armstrong wrote:
>> This patch adds support for the Legacy SCPI protocol in early JUNO versions and
>> shipped Amlogic ARMv8 based SoCs. Some Rockchip SoC are also known to use this
>> version of protocol with extended vendor commands
>> .
>> In order to support the legacy SCPI protocol variant, add back the structures
>> and macros that varies against the final specification.
>> Then add indirection table for legacy commands.
>> Finally Add bitmap field for channel selection since the Legacy protocol mandates to
>> send a selected subset of the commands on the high priority channel instead of the
>> low priority channel.
>>
>> The message sending path differs from the final SCPI procotocol because the
>> Amlogic SCP firmware always reply 1 instead of a special value containing the command
>> byte and replied rx data length.
>> For this reason commands queuing cannot be used and we assume the reply command is
>> the head of the rx_pending list since we ensure sequential command sending with a
>> separate dedicated mutex.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/firmware/arm_scpi.c | 221 +++++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 199 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
>> index 498afa0..6244eb1 100644
>> --- a/drivers/firmware/arm_scpi.c
>> +++ b/drivers/firmware/arm_scpi.c
> 
> [...]
> 
>> @@ -307,21 +398,46 @@ static void scpi_process_cmd(struct scpi_chan *ch, u32 cmd)
>>          return;
>>      }
>>
>> -    list_for_each_entry(t, &ch->rx_pending, node)
>> -        if (CMD_XTRACT_UNIQ(t->cmd) == CMD_XTRACT_UNIQ(cmd)) {
>> -            list_del(&t->node);
>> -            match = t;
>> -            break;
>> -        }
>> +    /* Command type is not replied by the SCP Firmware in legacy Mode
>> +     * We should consider that command is the head of pending RX commands
>> +     * if the list is not empty. In TX only mode, the list would be empty.
>> +     */
>> +    if (scpi_info->is_legacy) {
>> +        match = list_first_entry(&ch->rx_pending, struct scpi_xfer,
>> +                     node);
>> +        list_del(&match->node);
>> +    } else {
>> +        list_for_each_entry(t, &ch->rx_pending, node)
>> +            if (CMD_XTRACT_UNIQ(t->cmd) == CMD_XTRACT_UNIQ(cmd)) {
>> +                list_del(&t->node);
>> +                match = t;
>> +                break;
>> +            }
>> +    }
>>      /* check if wait_for_completion is in progress or timed-out */
>>      if (match && !completion_done(&match->done)) {
>> -        struct scpi_shared_mem *mem = ch->rx_payload;
>> -        unsigned int len = min(match->rx_len, CMD_SIZE(cmd));
>> +        unsigned int len;
>> +
>> +        if (scpi_info->is_legacy) {
>> +            struct legacy_scpi_shared_mem *mem = ch->rx_payload;
>> +
>> +            /* RX Length is not replied by the lagcy Firmware */
>> +            len = match->rx_len;
>> +
>> +            match->status = le32_to_cpu(mem->status);
>> +            memcpy_fromio(match->rx_buf, mem->payload, len);
> 
> The above 2 seems common to both, no ?

No, the shared_mem structure differs.

> 
>> +        } else {
>> +            struct scpi_shared_mem *mem = ch->rx_payload;
>> +
>> +            len = min(match->rx_len, CMD_SIZE(cmd));
>> +
>> +            match->status = le32_to_cpu(mem->status);
>> +            memcpy_fromio(match->rx_buf, mem->payload, len);
>> +        }
>>
>> -        match->status = le32_to_cpu(mem->status);
>> -        memcpy_fromio(match->rx_buf, mem->payload, len);
>>          if (match->rx_len > len)
>>              memset(match->rx_buf + len, 0, match->rx_len - len);
>> +
> 
> Spurious ?

Yep

> 
>>          complete(&match->done);
>>      }
>>      spin_unlock_irqrestore(&ch->rx_lock, flags);
>> @@ -331,7 +447,12 @@ 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;
>> -    u32 cmd = le32_to_cpu(mem->command);
>> +    u32 cmd;
>> +
>> +    if (scpi_info->is_legacy)
>> +        cmd = *(u32 *)msg;
> 
> Do we need do this if it doesn't contain command ?

No, will remove.

> 
>> +    else
>> +        cmd = le32_to_cpu(mem->command);
>>
>>      scpi_process_cmd(ch, cmd);
>>  }
>> @@ -343,17 +464,26 @@ static void scpi_tx_prepare(struct mbox_client *c, void *msg)
>>      struct scpi_chan *ch = container_of(c, struct scpi_chan, cl);
>>      struct scpi_shared_mem *mem = (struct scpi_shared_mem *)ch->tx_payload;
>>
>> -    if (t->tx_buf)
>> -        memcpy_toio(mem->payload, t->tx_buf, t->tx_len);
>> +    if (t->tx_buf) {
>> +        if (scpi_info->is_legacy)
>> +            memcpy_toio(ch->tx_payload, t->tx_buf, t->tx_len);
>> +        else
>> +            memcpy_toio(mem->payload, t->tx_buf, t->tx_len);
>> +    }
>> +
>>      if (t->rx_buf) {
>>          if (!(++ch->token))
>>              ++ch->token;
>>          ADD_SCPI_TOKEN(t->cmd, ch->token);
>> +        if (scpi_info->is_legacy)
>> +            t->slot = t->cmd;
> 
> I thought passing token was not an issue from your previous response,
> but you are overriding it here, why ?

Indeed, I can leave it, but it's useless since it won't serve to distinguish multiple similar commands.

> 
>>          spin_lock_irqsave(&ch->rx_lock, flags);
>>          list_add_tail(&t->node, &ch->rx_pending);
>>          spin_unlock_irqrestore(&ch->rx_lock, flags);
>>      }
>> -    mem->command = cpu_to_le32(t->cmd);
>> +
>> +    if (!scpi_info->is_legacy)
>> +        mem->command = cpu_to_le32(t->cmd);
>>  }
>>
>>  static struct scpi_xfer *get_scpi_xfer(struct scpi_chan *ch)
>> @@ -396,21 +526,37 @@ static int scpi_send_message(unsigned int offset, void *tx_buf,
>>
>>      cmd = scpi_info->scpi_cmds[offset];
>>
>> -    chan = atomic_inc_return(&scpi_info->next_chan) % scpi_info->num_chans;
>> +    if (scpi_info->is_legacy)
>> +        chan = test_bit(cmd, scpi_info->cmd_priority) ? 1 : 0;
>> +    else
>> +        chan = atomic_inc_return(&scpi_info->next_chan) %
>> +            scpi_info->num_chans;
>>      scpi_chan = scpi_info->channels + chan;
>>
>>      msg = get_scpi_xfer(scpi_chan);
>>      if (!msg)
>>          return -ENOMEM;
>>
>> -    msg->slot = BIT(SCPI_SLOT);
>> -    msg->cmd = PACK_SCPI_CMD(cmd, tx_len);
>> +    if (scpi_info->is_legacy) {
>> +        msg->cmd = PACK_LEGACY_SCPI_CMD(cmd, tx_len);
>> +        msg->slot = msg->cmd;
>> +    } else {
>> +        msg->slot = BIT(SCPI_SLOT);
>> +        msg->cmd = PACK_SCPI_CMD(cmd, tx_len);
>> +    }
>>      msg->tx_buf = tx_buf;
>>      msg->tx_len = tx_len;
>>      msg->rx_buf = rx_buf;
>>      msg->rx_len = rx_len;
>>      init_completion(&msg->done);
>>
>> +    /* Since we cannot distinguish the original command in the
>> +     * MHU reply stat value from a Legacy SCP firmware, ensure
>> +     * sequential command sending to the firmware.
>> +     */
> 
> OK this comment now questions the existence of this extra lock.
> The mailbox will always send the commands in the sequential order.
> It's only firmware that can re-order the response. Since that can't
> happen in you case, I really don't see the need for this.
> 
> Please explain the race you would see without this locking. Yes I
> understand that only one command is supposed to be sent to firmware at a
> time. Suppose you allow more callers here, all will wait on the
> completion flags and the first in the list gets unblocked right ?
> I am just trying to understand if there's real need for this extra
> lock when we already have that from the list.

In my current tests I have huge kernel hang when having multiple callers,
I must find out where this issue comes from...
In any case, we have an issue about the command sequencing.
If we push a tx-only command and then right after a tx-rx command, the
mailbox callback from the first command won't be able to distinguish which
command is handled !
In this case, the rx_pending list will not be empty, some garbage will be returned
to the second command handler and the real data from the second command handling
will be lost thinking it's a tx-only command.


We have two choices here :
 - Also push the tx-only commands to the rx_pending list, and also wait for their completion
 - Add an extra lock

What is your preferred scheme ?

>> +    if (scpi_info->is_legacy)
>> +        mutex_lock(&scpi_chan->legacy_lock);
>> +
>>      ret = mbox_send_message(scpi_chan->chan, msg);
>>      if (ret < 0 || !rx_buf)
>>          goto out;
>> @@ -421,9 +567,13 @@ static int scpi_send_message(unsigned int offset, void *tx_buf,
>>          /* first status word */
>>          ret = msg->status;
>>  out:
>> -    if (ret < 0 && rx_buf) /* remove entry from the list if timed-out */
>> +    if (ret < 0 && rx_buf)
>> +        /* remove entry from the list if timed-out */
>>          scpi_process_cmd(scpi_chan, msg->cmd);
>>
>> +    if (scpi_info->is_legacy)
>> +        mutex_unlock(&scpi_chan->legacy_lock);
>> +
>>      put_scpi_xfer(msg, scpi_chan);
>>      /* SCPI error codes > 0, translate them to Linux scale*/
>>      return ret > 0 ? scpi_to_linux_errno(ret) : ret;
> 
> [...]
> 
>> @@ -525,7 +687,6 @@ static struct scpi_dvfs_info *scpi_dvfs_get_info(u8 domain)
>>
>>      info->count = DVFS_OPP_COUNT(buf.header);
>>      info->latency = DVFS_LATENCY(buf.header) * 1000; /* uS to nS */
>> -
> 
> Spurious ?

Indeed.

> 
>>      info->opps = kcalloc(info->count, sizeof(*opp), GFP_KERNEL);
>>      if (!info->opps) {
>>          kfree(info);
>> @@ -580,9 +741,13 @@ static int scpi_sensor_get_value(u16 sensor, u64 *val)
>>
>>      ret = scpi_send_message(CMD_SENSOR_VALUE, &id, sizeof(id),
>>                  &buf, sizeof(buf));
>> -    if (!ret)
>> -        *val = (u64)le32_to_cpu(buf.hi_val) << 32 |
>> -            le32_to_cpu(buf.lo_val);
>> +    if (!ret) {
>> +        if (scpi_info->is_legacy)
>> +            *val = (u64)le32_to_cpu(buf.lo_val);
>> +        else
>> +            *val = (u64)le32_to_cpu(buf.hi_val) << 32 |
>> +                le32_to_cpu(buf.lo_val);
>> +    }
> 
> Not required as I have mentioned couple of times in previous versions,
> it's zero filled by the driver.
> 

OK

I will fix the issues, but I need your advice for the locking scheme. I really want this
to be merged and be able to go forward !

Thanks,
Neil

[PATCH v4 2/8] scpi: Add alternative legacy structures, functions and macros

[Sudeep Holla]

On 17/10/16 09:25, Neil Armstrong wrote:
> On 10/10/2016 04:36 PM, Sudeep Holla wrote:
>> Hi Neil,
>>
>> Sorry, I could not reply to your response on v3. Anyways I will review v4.
>>
>> On 05/10/16 08:33, Neil Armstrong wrote:
>>> This patch adds support for the Legacy SCPI protocol in early JUNO versions and
>>> shipped Amlogic ARMv8 based SoCs. Some Rockchip SoC are also known to use this
>>> version of protocol with extended vendor commands
>>> .
>>> In order to support the legacy SCPI protocol variant, add back the structures
>>> and macros that varies against the final specification.
>>> Then add indirection table for legacy commands.
>>> Finally Add bitmap field for channel selection since the Legacy protocol mandates to
>>> send a selected subset of the commands on the high priority channel instead of the
>>> low priority channel.
>>>
>>> The message sending path differs from the final SCPI procotocol because the
>>> Amlogic SCP firmware always reply 1 instead of a special value containing the command
>>> byte and replied rx data length.
>>> For this reason commands queuing cannot be used and we assume the reply command is
>>> the head of the rx_pending list since we ensure sequential command sending with a
>>> separate dedicated mutex.
>>>
>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>> ---
>>>  drivers/firmware/arm_scpi.c | 221 +++++++++++++++++++++++++++++++++++++++-----
>>>  1 file changed, 199 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
>>> index 498afa0..6244eb1 100644
>>> --- a/drivers/firmware/arm_scpi.c
>>> +++ b/drivers/firmware/arm_scpi.c
>>
>> [...]
>>
>>> @@ -307,21 +398,46 @@ static void scpi_process_cmd(struct scpi_chan *ch, u32 cmd)
>>>          return;
>>>      }
>>>
>>> -    list_for_each_entry(t, &ch->rx_pending, node)
>>> -        if (CMD_XTRACT_UNIQ(t->cmd) == CMD_XTRACT_UNIQ(cmd)) {
>>> -            list_del(&t->node);
>>> -            match = t;
>>> -            break;
>>> -        }
>>> +    /* Command type is not replied by the SCP Firmware in legacy Mode
>>> +     * We should consider that command is the head of pending RX commands
>>> +     * if the list is not empty. In TX only mode, the list would be empty.
>>> +     */
>>> +    if (scpi_info->is_legacy) {
>>> +        match = list_first_entry(&ch->rx_pending, struct scpi_xfer,
>>> +                     node);
>>> +        list_del(&match->node);
>>> +    } else {
>>> +        list_for_each_entry(t, &ch->rx_pending, node)
>>> +            if (CMD_XTRACT_UNIQ(t->cmd) == CMD_XTRACT_UNIQ(cmd)) {
>>> +                list_del(&t->node);
>>> +                match = t;
>>> +                break;
>>> +            }
>>> +    }
>>>      /* check if wait_for_completion is in progress or timed-out */
>>>      if (match && !completion_done(&match->done)) {
>>> -        struct scpi_shared_mem *mem = ch->rx_payload;
>>> -        unsigned int len = min(match->rx_len, CMD_SIZE(cmd));
>>> +        unsigned int len;
>>> +
>>> +        if (scpi_info->is_legacy) {
>>> +            struct legacy_scpi_shared_mem *mem = ch->rx_payload;
>>> +
>>> +            /* RX Length is not replied by the lagcy Firmware */

Typo above legacy

>>> +            len = match->rx_len;
>>> +
>>> +            match->status = le32_to_cpu(mem->status);
>>> +            memcpy_fromio(match->rx_buf, mem->payload, len);
>>
>> The above 2 seems common to both, no ?
>
> No, the shared_mem structure differs.
>

Yes I see that, I was just referring the last 2 statements.

>>
>>> +        } else {
>>> +            struct scpi_shared_mem *mem = ch->rx_payload;
>>> +
>>> +            len = min(match->rx_len, CMD_SIZE(cmd));
>>> +
>>> +            match->status = le32_to_cpu(mem->status);
>>> +            memcpy_fromio(match->rx_buf, mem->payload, len);

and the above 2 can be moved out of the conditions, no ?

if (scpi_info->is_legacy) {
	struct legacy_scpi_shared_mem *mem = ch->rx_payload;
	len = match->rx_len;
} else {
	struct scpi_shared_mem *mem = ch->rx_payload;
	len = min(match->rx_len, CMD_SIZE(cmd));
}
match->status = le32_to_cpu(mem->status);
memcpy_fromio(match->rx_buf, mem->payload, len);

should work.


[...]

>>
>>> +    else
>>> +        cmd = le32_to_cpu(mem->command);
>>>
>>>      scpi_process_cmd(ch, cmd);
>>>  }
>>> @@ -343,17 +464,26 @@ static void scpi_tx_prepare(struct mbox_client *c, void *msg)
>>>      struct scpi_chan *ch = container_of(c, struct scpi_chan, cl);
>>>      struct scpi_shared_mem *mem = (struct scpi_shared_mem *)ch->tx_payload;
>>>
>>> -    if (t->tx_buf)
>>> -        memcpy_toio(mem->payload, t->tx_buf, t->tx_len);
>>> +    if (t->tx_buf) {
>>> +        if (scpi_info->is_legacy)
>>> +            memcpy_toio(ch->tx_payload, t->tx_buf, t->tx_len);
>>> +        else
>>> +            memcpy_toio(mem->payload, t->tx_buf, t->tx_len);
>>> +    }
>>> +
>>>      if (t->rx_buf) {
>>>          if (!(++ch->token))
>>>              ++ch->token;
>>>          ADD_SCPI_TOKEN(t->cmd, ch->token);
>>> +        if (scpi_info->is_legacy)
>>> +            t->slot = t->cmd;
>>
>> I thought passing token was not an issue from your previous response,
>> but you are overriding it here, why ?
>
> Indeed, I can leave it, but it's useless since it won't serve to
> distinguish multiple similar commands.
>

OK, I don't see any point in such micro optimization, so please retain it.

[...]

>>> +    /* Since we cannot distinguish the original command in the
>>> +     * MHU reply stat value from a Legacy SCP firmware, ensure
>>> +     * sequential command sending to the firmware.
>>> +     */
>>
>> OK this comment now questions the existence of this extra lock.
>> The mailbox will always send the commands in the sequential order.
>> It's only firmware that can re-order the response. Since that can't
>> happen in you case, I really don't see the need for this.
>>
>> Please explain the race you would see without this locking. Yes I
>> understand that only one command is supposed to be sent to firmware at a
>> time. Suppose you allow more callers here, all will wait on the
>> completion flags and the first in the list gets unblocked right ?
>> I am just trying to understand if there's real need for this extra
>> lock when we already have that from the list.
>
> In my current tests I have huge kernel hang when having multiple callers,
> I must find out where this issue comes from...

Yes IMO, you should understand the root cause of this issue. There may
be issue with the existing driver itself. But just adding a lock just to
avoid the hang without understanding it is wrong.

> In any case, we have an issue about the command sequencing. If we
> push a tx-only command and then right after a tx-rx command, the
> mailbox callback from the first command won't be able to distinguish
> which command is handled !

Hmm, how exactly ? I won't expect scpi_handle_remote_msg to becalled in
that case.

> In this case, the rx_pending list will not be empty, some garbage
> will be returned to the second command handler and the real data from
> the second command handling will be lost thinking it's a tx-only
> command.
>

Yes as I said why is scpi_handle_remote_msg called for tx only command.
And more over we don't have any tx only command in the driver, I am
still unable to understand the issue you are facing. Are you sure you
have tx-only command in the failure/hang case ?

>
> We have two choices here : - Also push the tx-only commands to the
> rx_pending list, and also wait for their completion

See above, I need to know details on this tx-only command. In fact, they
may not be tx-only as SCP is sending some response back, may just status.

> - Add an extra lock
>

Not this for sure.

> What is your preferred scheme ?
>

Option 1 if it legitimate case. I mean we may be misunderstanding the
definition of tx-only command.


>>> +    if (scpi_info->is_legacy)
>>> +        mutex_lock(&scpi_chan->legacy_lock);
>>> +
>>>      ret = mbox_send_message(scpi_chan->chan, msg);
>>>      if (ret < 0 || !rx_buf)
>>>          goto out;
>>> @@ -421,9 +567,13 @@ static int scpi_send_message(unsigned int offset, void *tx_buf,
>>>          /* first status word */
>>>          ret = msg->status;
>>>  out:
>>> -    if (ret < 0 && rx_buf) /* remove entry from the list if timed-out */
>>> +    if (ret < 0 && rx_buf)
>>> +        /* remove entry from the list if timed-out */
>>>          scpi_process_cmd(scpi_chan, msg->cmd);
>>>
>>> +    if (scpi_info->is_legacy)
>>> +        mutex_unlock(&scpi_chan->legacy_lock);
>>> +
>>>      put_scpi_xfer(msg, scpi_chan);
>>>      /* SCPI error codes > 0, translate them to Linux scale*/
>>>      return ret > 0 ? scpi_to_linux_errno(ret) : ret;
>>

[...]

>
> I will fix the issues, but I need your advice for the locking scheme. I really want this
> to be merged and be able to go forward !
>

Yes I agree and I have no major concern with the series now except the
locking.

-- 
Regards,
Sudeep

[PATCH v4 2/8] scpi: Add alternative legacy structures, functions and macros

[Neil Armstrong]

On 10/17/2016 01:16 PM, Sudeep Holla wrote:
> 
> 
> On 17/10/16 09:25, Neil Armstrong wrote:
>> On 10/10/2016 04:36 PM, Sudeep Holla wrote:
>>> Hi Neil,
>>>
>>> Sorry, I could not reply to your response on v3. Anyways I will review v4.
>>>
>>> On 05/10/16 08:33, Neil Armstrong wrote:
>>>> This patch adds support for the Legacy SCPI protocol in early JUNO versions and
>>>> shipped Amlogic ARMv8 based SoCs. Some Rockchip SoC are also known to use this
>>>> version of protocol with extended vendor commands
>>>> .
>>>> In order to support the legacy SCPI protocol variant, add back the structures
>>>> and macros that varies against the final specification.
>>>> Then add indirection table for legacy commands.
>>>> Finally Add bitmap field for channel selection since the Legacy protocol mandates to
>>>> send a selected subset of the commands on the high priority channel instead of the
>>>> low priority channel.
>>>>
>>>> The message sending path differs from the final SCPI procotocol because the
>>>> Amlogic SCP firmware always reply 1 instead of a special value containing the command
>>>> byte and replied rx data length.
>>>> For this reason commands queuing cannot be used and we assume the reply command is
>>>> the head of the rx_pending list since we ensure sequential command sending with a
>>>> separate dedicated mutex.
>>>>
>>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>>> ---
>>>>  drivers/firmware/arm_scpi.c | 221 +++++++++++++++++++++++++++++++++++++++-----
>>>>  1 file changed, 199 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
>>>> index 498afa0..6244eb1 100644
>>>> --- a/drivers/firmware/arm_scpi.c
>>>> +++ b/drivers/firmware/arm_scpi.c
>>>
>>> [...]
>>>
>>>> @@ -307,21 +398,46 @@ static void scpi_process_cmd(struct scpi_chan *ch, u32 cmd)
>>>>          return;
>>>>      }
>>>>
>>>> -    list_for_each_entry(t, &ch->rx_pending, node)
>>>> -        if (CMD_XTRACT_UNIQ(t->cmd) == CMD_XTRACT_UNIQ(cmd)) {
>>>> -            list_del(&t->node);
>>>> -            match = t;
>>>> -            break;
>>>> -        }
>>>> +    /* Command type is not replied by the SCP Firmware in legacy Mode
>>>> +     * We should consider that command is the head of pending RX commands
>>>> +     * if the list is not empty. In TX only mode, the list would be empty.
>>>> +     */
>>>> +    if (scpi_info->is_legacy) {
>>>> +        match = list_first_entry(&ch->rx_pending, struct scpi_xfer,
>>>> +                     node);
>>>> +        list_del(&match->node);
>>>> +    } else {
>>>> +        list_for_each_entry(t, &ch->rx_pending, node)
>>>> +            if (CMD_XTRACT_UNIQ(t->cmd) == CMD_XTRACT_UNIQ(cmd)) {
>>>> +                list_del(&t->node);
>>>> +                match = t;
>>>> +                break;
>>>> +            }
>>>> +    }
>>>>      /* check if wait_for_completion is in progress or timed-out */
>>>>      if (match && !completion_done(&match->done)) {
>>>> -        struct scpi_shared_mem *mem = ch->rx_payload;
>>>> -        unsigned int len = min(match->rx_len, CMD_SIZE(cmd));
>>>> +        unsigned int len;
>>>> +
>>>> +        if (scpi_info->is_legacy) {
>>>> +            struct legacy_scpi_shared_mem *mem = ch->rx_payload;
>>>> +
>>>> +            /* RX Length is not replied by the lagcy Firmware */
> 
> Typo above legacy
> 
>>>> +            len = match->rx_len;
>>>> +
>>>> +            match->status = le32_to_cpu(mem->status);
>>>> +            memcpy_fromio(match->rx_buf, mem->payload, len);
>>>
>>> The above 2 seems common to both, no ?
>>
>> No, the shared_mem structure differs.
>>
> 
> Yes I see that, I was just referring the last 2 statements.
> 
>>>
>>>> +        } else {
>>>> +            struct scpi_shared_mem *mem = ch->rx_payload;
>>>> +
>>>> +            len = min(match->rx_len, CMD_SIZE(cmd));
>>>> +
>>>> +            match->status = le32_to_cpu(mem->status);
>>>> +            memcpy_fromio(match->rx_buf, mem->payload, len);
> 
> and the above 2 can be moved out of the conditions, no ?
> 
> if (scpi_info->is_legacy) {
>     struct legacy_scpi_shared_mem *mem = ch->rx_payload;
>     len = match->rx_len;
> } else {
>     struct scpi_shared_mem *mem = ch->rx_payload;
>     len = min(match->rx_len, CMD_SIZE(cmd));
> }
> match->status = le32_to_cpu(mem->status);
> memcpy_fromio(match->rx_buf, mem->payload, len);
> 
> should work.

Well, we will have "error: ?mem? undeclared (first use in this function)" since mem is not declared outside the if/else.

I don't see good solutions even with an union.

> 
> [...]
> 
>>>
>>>> +    else
>>>> +        cmd = le32_to_cpu(mem->command);
>>>>
>>>>      scpi_process_cmd(ch, cmd);
>>>>  }
>>>> @@ -343,17 +464,26 @@ static void scpi_tx_prepare(struct mbox_client *c, void *msg)
>>>>      struct scpi_chan *ch = container_of(c, struct scpi_chan, cl);
>>>>      struct scpi_shared_mem *mem = (struct scpi_shared_mem *)ch->tx_payload;
>>>>
>>>> -    if (t->tx_buf)
>>>> -        memcpy_toio(mem->payload, t->tx_buf, t->tx_len);
>>>> +    if (t->tx_buf) {
>>>> +        if (scpi_info->is_legacy)
>>>> +            memcpy_toio(ch->tx_payload, t->tx_buf, t->tx_len);
>>>> +        else
>>>> +            memcpy_toio(mem->payload, t->tx_buf, t->tx_len);
>>>> +    }
>>>> +
>>>>      if (t->rx_buf) {
>>>>          if (!(++ch->token))
>>>>              ++ch->token;
>>>>          ADD_SCPI_TOKEN(t->cmd, ch->token);
>>>> +        if (scpi_info->is_legacy)
>>>> +            t->slot = t->cmd;
>>>
>>> I thought passing token was not an issue from your previous response,
>>> but you are overriding it here, why ?
>>
>> Indeed, I can leave it, but it's useless since it won't serve to
>> distinguish multiple similar commands.
>>
> 
> OK, I don't see any point in such micro optimization, so please retain it.
> 

I misread my code, I leaved the token passing, but I copy back the cmd to the slot which is used by the MHU.
If I remove the "t->slot = t->cmd;", the token won't be passed to the FW.

> [...]
> 
>>>> +    /* Since we cannot distinguish the original command in the
>>>> +     * MHU reply stat value from a Legacy SCP firmware, ensure
>>>> +     * sequential command sending to the firmware.
>>>> +     */
>>>
>>> OK this comment now questions the existence of this extra lock.
>>> The mailbox will always send the commands in the sequential order.
>>> It's only firmware that can re-order the response. Since that can't
>>> happen in you case, I really don't see the need for this.
>>>
>>> Please explain the race you would see without this locking. Yes I
>>> understand that only one command is supposed to be sent to firmware at a
>>> time. Suppose you allow more callers here, all will wait on the
>>> completion flags and the first in the list gets unblocked right ?
>>> I am just trying to understand if there's real need for this extra
>>> lock when we already have that from the list.
>>
>> In my current tests I have huge kernel hang when having multiple callers,
>> I must find out where this issue comes from...
> 
> Yes IMO, you should understand the root cause of this issue. There may
> be issue with the existing driver itself. But just adding a lock just to
> avoid the hang without understanding it is wrong.
> 
>> In any case, we have an issue about the command sequencing. If we
>> push a tx-only command and then right after a tx-rx command, the
>> mailbox callback from the first command won't be able to distinguish
>> which command is handled !
> 
> Hmm, how exactly ? I won't expect scpi_handle_remote_msg to becalled in
> that case.
> 
>> In this case, the rx_pending list will not be empty, some garbage
>> will be returned to the second command handler and the real data from
>> the second command handling will be lost thinking it's a tx-only
>> command.
>>
> 
> Yes as I said why is scpi_handle_remote_msg called for tx only command.
> And more over we don't have any tx only command in the driver, I am
> still unable to understand the issue you are facing. Are you sure you
> have tx-only command in the failure/hang case ?
> 
>>
>> We have two choices here : - Also push the tx-only commands to the
>> rx_pending list, and also wait for their completion
> 
> See above, I need to know details on this tx-only command. In fact, they
> may not be tx-only as SCP is sending some response back, may just status.
> 
>> - Add an extra lock
>>
> 
> Not this for sure.
> 
>> What is your preferred scheme ?
>>
> 
> Option 1 if it legitimate case. I mean we may be misunderstanding the
> definition of tx-only command.
> 
> 
>>>> +    if (scpi_info->is_legacy)
>>>> +        mutex_lock(&scpi_chan->legacy_lock);
>>>> +
>>>>      ret = mbox_send_message(scpi_chan->chan, msg);
>>>>      if (ret < 0 || !rx_buf)
>>>>          goto out;
>>>> @@ -421,9 +567,13 @@ static int scpi_send_message(unsigned int offset, void *tx_buf,
>>>>          /* first status word */
>>>>          ret = msg->status;
>>>>  out:
>>>> -    if (ret < 0 && rx_buf) /* remove entry from the list if timed-out */
>>>> +    if (ret < 0 && rx_buf)
>>>> +        /* remove entry from the list if timed-out */
>>>>          scpi_process_cmd(scpi_chan, msg->cmd);
>>>>
>>>> +    if (scpi_info->is_legacy)
>>>> +        mutex_unlock(&scpi_chan->legacy_lock);
>>>> +
>>>>      put_scpi_xfer(msg, scpi_chan);
>>>>      /* SCPI error codes > 0, translate them to Linux scale*/
>>>>      return ret > 0 ? scpi_to_linux_errno(ret) : ret;
>>>
> 
> [...]
> 
>>
>> I will fix the issues, but I need your advice for the locking scheme. I really want this
>> to be merged and be able to go forward !
>>
> 
> Yes I agree and I have no major concern with the series now except the
> locking.
> 

[PATCH v4 2/8] scpi: Add alternative legacy structures, functions and macros

[Sudeep Holla]

On 19/10/16 11:28, Neil Armstrong wrote:
> On 10/17/2016 01:16 PM, Sudeep Holla wrote:

[...]

>>
>> and the above 2 can be moved out of the conditions, no ?
>>
>> if (scpi_info->is_legacy) {
>>     struct legacy_scpi_shared_mem *mem = ch->rx_payload;
>>     len = match->rx_len;
>> } else {
>>     struct scpi_shared_mem *mem = ch->rx_payload;
>>     len = min(match->rx_len, CMD_SIZE(cmd));
>> }
>> match->status = le32_to_cpu(mem->status);
>> memcpy_fromio(match->rx_buf, mem->payload, len);
>>
>> should work.
>
> Well, we will have "error: ?mem? undeclared (first use in this
> function)" since mem is not declared outside the if/else.
>
> I don't see good solutions even with an union.

Right, I missed to see that. You can leave it as you had before then.


[...]
>>>>>      if (t->rx_buf) {
>>>>>          if (!(++ch->token))
>>>>>              ++ch->token;
>>>>>          ADD_SCPI_TOKEN(t->cmd, ch->token);
>>>>> +        if (scpi_info->is_legacy)
>>>>> +            t->slot = t->cmd;
>>>>
>>>> I thought passing token was not an issue from your previous response,
>>>> but you are overriding it here, why ?
>>>
>>> Indeed, I can leave it, but it's useless since it won't serve to
>>> distinguish multiple similar commands.
>>>
>>
>> OK, I don't see any point in such micro optimization, so please retain it.
>>
>
> I misread my code, I leaved the token passing, but I copy back the
> cmd  to the slot which is used by the MHU.
> If I remove the "t->slot = t->cmd;", the token won't be passed to the
> FW.
>

Right, sorry I think I misled you :)

-- 
Regards,
Sudeep

[PATCH v4 2/8] scpi: Add alternative legacy structures, functions and macros

[Sudeep Holla]

On 19/10/16 11:37, Sudeep Holla wrote:
>
>
> On 19/10/16 11:28, Neil Armstrong wrote:
>> On 10/17/2016 01:16 PM, Sudeep Holla wrote:
>
> [...]
>
>>>
>>> and the above 2 can be moved out of the conditions, no ?
>>>
>>> if (scpi_info->is_legacy) {
>>>     struct legacy_scpi_shared_mem *mem = ch->rx_payload;
>>>     len = match->rx_len;
>>> } else {
>>>     struct scpi_shared_mem *mem = ch->rx_payload;
>>>     len = min(match->rx_len, CMD_SIZE(cmd));
>>> }
>>> match->status = le32_to_cpu(mem->status);
>>> memcpy_fromio(match->rx_buf, mem->payload, len);
>>>
>>> should work.
>>
>> Well, we will have "error: ?mem? undeclared (first use in this
>> function)" since mem is not declared outside the if/else.
>>
>> I don't see good solutions even with an union.
>
> Right, I missed to see that. You can leave it as you had before then.
>
>
> [...]
>>>>>>      if (t->rx_buf) {
>>>>>>          if (!(++ch->token))
>>>>>>              ++ch->token;
>>>>>>          ADD_SCPI_TOKEN(t->cmd, ch->token);
>>>>>> +        if (scpi_info->is_legacy)
>>>>>> +            t->slot = t->cmd;
>>>>>
>>>>> I thought passing token was not an issue from your previous response,
>>>>> but you are overriding it here, why ?
>>>>
>>>> Indeed, I can leave it, but it's useless since it won't serve to
>>>> distinguish multiple similar commands.
>>>>
>>>
>>> OK, I don't see any point in such micro optimization, so please
>>> retain it.
>>>
>>
>> I misread my code, I leaved the token passing, but I copy back the
>> cmd  to the slot which is used by the MHU.
>> If I remove the "t->slot = t->cmd;", the token won't be passed to the
>> FW.
>>
>
> Right, sorry I think I misled you :)
>

On the other hand, if we are unable to use that in the driver as you
just have FIFO in the list for this legacy mode, I think we can drop it
completely. I know I had argued otherwise before, but that was before I
realized that we need to keep FIFO in the list ;). Let me know if you
are OK to drop it.

-- 
Regards,
Sudeep

[PATCH v4 3/8] scpi: Do not fail if get_capabilities is not implemented

[Neil Armstrong]

On Amlogic SCPI legacy implementation, the GET_CAPABILITIES is not
supported, failover by using 0.0.0 version.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/firmware/arm_scpi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index 6244eb1..1fb3bbf 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -807,6 +807,10 @@ static int scpi_init_versions(struct scpi_drvinfo *info)
 		info->protocol_version = le32_to_cpu(caps.protocol_version);
 		info->firmware_version = le32_to_cpu(caps.platform_version);
 	}
+	/* Ignore error if not implemented */
+	if (scpi_info->is_legacy && ret == -EOPNOTSUPP)
+		return 0;
+
 	return ret;
 }
 
-- 
1.9.1

[PATCH v4 4/8] scpi: Add support for Legacy match table for Amlogic GXBB SoC

[Neil Armstrong]

Add new DT match table to setup the is_legacy boolean value across
the scpi functions.
Add the Amlogic GXBB SoC compatible for platform and as legacy match entry.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/firmware/arm_scpi.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index 1fb3bbf..21fafbe 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -895,6 +895,11 @@ static int scpi_alloc_xfer_list(struct device *dev, struct scpi_chan *ch)
 	return 0;
 }
 
+static const struct of_device_id legacy_scpi_of_match[] = {
+	{.compatible = "amlogic,meson-gxbb-scpi"},
+	{},
+};
+
 static int scpi_probe(struct platform_device *pdev)
 {
 	int count, idx, ret;
@@ -907,6 +912,9 @@ static int scpi_probe(struct platform_device *pdev)
 	if (!scpi_info)
 		return -ENOMEM;
 
+	if (of_match_device(legacy_scpi_of_match, &pdev->dev))
+		scpi_info->is_legacy = true;
+
 	count = of_count_phandle_with_args(np, "mboxes", "#mbox-cells");
 	if (count < 0) {
 		dev_err(dev, "no mboxes property in '%s'\n", np->full_name);
@@ -1009,6 +1017,7 @@ static int scpi_probe(struct platform_device *pdev)
 
 static const struct of_device_id scpi_of_match[] = {
 	{.compatible = "arm,scpi"},
+	{.compatible = "amlogic,meson-gxbb-scpi"},
 	{},
 };
 
-- 
1.9.1

[PATCH v4 5/8] scpi: grow MAX_DVFS_OPPS to 16 entries

[Neil Armstrong]

Since Amlogic SoCs reports more than 8 OPPs per domains, grow the structure
size to 16.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 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 21fafbe..bc15348 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -70,7 +70,7 @@
 #define SCPI_SLOT		0
 
 #define MAX_DVFS_DOMAINS	8
-#define MAX_DVFS_OPPS		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)
 
-- 
1.9.1

[PATCH v4 6/8] dt-bindings: Add support for Amlogic GXBB SCPI Interface

[Neil Armstrong]

Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 Documentation/devicetree/bindings/arm/arm,scpi.txt | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/arm,scpi.txt b/Documentation/devicetree/bindings/arm/arm,scpi.txt
index faa4b44..04bc171 100644
--- a/Documentation/devicetree/bindings/arm/arm,scpi.txt
+++ b/Documentation/devicetree/bindings/arm/arm,scpi.txt
@@ -7,7 +7,7 @@ by Linux to initiate various system control and power operations.
 
 Required properties:
 
-- compatible : should be "arm,scpi"
+- compatible : should be "arm,scpi" or "amlogic,meson-gxbb-scpi"
 - mboxes: List of phandle and mailbox channel specifiers
 	  All the channels reserved by remote SCP firmware for use by
 	  SCPI message protocol should be specified in any order
@@ -60,7 +60,8 @@ A small area of SRAM is reserved for SCPI communication between application
 processors and SCP.
 
 Required properties:
-- compatible : should be "arm,juno-sram-ns" for Non-secure SRAM on Juno
+- compatible : should be "arm,juno-sram-ns" for Non-secure SRAM on Juno,
+		or "amlogic,meson-gxbb-sram" for Amlogic GXBB SoC.
 
 The rest of the properties should follow the generic mmio-sram description
 found in ../../sram/sram.txt
@@ -70,7 +71,8 @@ Each sub-node represents the reserved area for SCPI.
 Required sub-node properties:
 - reg : The base offset and size of the reserved area with the SRAM
 - compatible : should be "arm,juno-scp-shmem" for Non-secure SRAM based
-	       shared memory on Juno platforms
+	       shared memory on Juno platforms or
+	       "amlogic,meson-gxbb-scp-shmem" for Amlogic GXBB SoC.
 
 Sensor bindings for the sensors based on SCPI Message Protocol
 --------------------------------------------------------------
-- 
1.9.1

[PATCH v4 6/8] dt-bindings: Add support for Amlogic GXBB SCPI Interface

[Olof Johansson]

Hi,


On Wed, Oct 5, 2016 at 12:33 AM, Neil Armstrong <narmstrong@baylibre.com> wrote:
> Acked-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  Documentation/devicetree/bindings/arm/arm,scpi.txt | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/arm/arm,scpi.txt b/Documentation/devicetree/bindings/arm/arm,scpi.txt
> index faa4b44..04bc171 100644
> --- a/Documentation/devicetree/bindings/arm/arm,scpi.txt
> +++ b/Documentation/devicetree/bindings/arm/arm,scpi.txt
> @@ -7,7 +7,7 @@ by Linux to initiate various system control and power operations.
>
>  Required properties:
>
> -- compatible : should be "arm,scpi"
> +- compatible : should be "arm,scpi" or "amlogic,meson-gxbb-scpi"

This doesn't seem right to document here. If anything you might want
to have a table of more-specific-compatibles for specific
implementations, but "arm,scpi" should still be the compatible of the
node (just not the most specific one).

Also, documenting it here indiciates that non-amlogic implementations
can/should use that compatible, which is misleading.

>  - mboxes: List of phandle and mailbox channel specifiers
>           All the channels reserved by remote SCP firmware for use by
>           SCPI message protocol should be specified in any order
> @@ -60,7 +60,8 @@ A small area of SRAM is reserved for SCPI communication between application
>  processors and SCP.
>
>  Required properties:
> -- compatible : should be "arm,juno-sram-ns" for Non-secure SRAM on Juno
> +- compatible : should be "arm,juno-sram-ns" for Non-secure SRAM on Juno,
> +               or "amlogic,meson-gxbb-sram" for Amlogic GXBB SoC.

Maybe you'd be better of with a meson-specific document that refers to
these but with different compatible values.

Come to think of it, the Juno-specific one maybe shouldn't be in
arm,scpi at all, since that adds confusion here.

It's somewhat confusing that ARM is both a platform, architecture and
in some cases implementer of specific systems. :)

>  The rest of the properties should follow the generic mmio-sram description
>  found in ../../sram/sram.txt
> @@ -70,7 +71,8 @@ Each sub-node represents the reserved area for SCPI.
>  Required sub-node properties:
>  - reg : The base offset and size of the reserved area with the SRAM
>  - compatible : should be "arm,juno-scp-shmem" for Non-secure SRAM based
> -              shared memory on Juno platforms
> +              shared memory on Juno platforms or
> +              "amlogic,meson-gxbb-scp-shmem" for Amlogic GXBB SoC.

Same here. It won't scale if all vendors are expected to add an entry here.


-Olof

[PATCH v4 6/8] dt-bindings: Add support for Amlogic GXBB SCPI Interface

[Sudeep Holla]

On Sat, Oct 29, 2016 at 11:39:05AM -0700, Olof Johansson wrote:
> Hi,
> 
> 
> On Wed, Oct 5, 2016 at 12:33 AM, Neil Armstrong <narmstrong@baylibre.com> wrote:
> > Acked-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> > ---
> >  Documentation/devicetree/bindings/arm/arm,scpi.txt | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/arm/arm,scpi.txt b/Documentation/devicetree/bindings/arm/arm,scpi.txt
> > index faa4b44..04bc171 100644
> > --- a/Documentation/devicetree/bindings/arm/arm,scpi.txt
> > +++ b/Documentation/devicetree/bindings/arm/arm,scpi.txt
> > @@ -7,7 +7,7 @@ by Linux to initiate various system control and power operations.
> >
> >  Required properties:
> >
> > -- compatible : should be "arm,scpi"
> > +- compatible : should be "arm,scpi" or "amlogic,meson-gxbb-scpi"
> 
> This doesn't seem right to document here. If anything you might want
> to have a table of more-specific-compatibles for specific
> implementations, but "arm,scpi" should still be the compatible of the
> node (just not the most specific one).
> 

I completely agree with you and I was pushing for a generic "arm,legacy-scpi"
compatible until this binding was acked by Rob.

Anyways, I will add the generic compatible and post the changes.

> Also, documenting it here indiciates that non-amlogic implementations
> can/should use that compatible, which is misleading.
>

Agreed, it's better to keep them out of this generic binding document.

> >  - mboxes: List of phandle and mailbox channel specifiers
> >           All the channels reserved by remote SCP firmware for use by
> >           SCPI message protocol should be specified in any order
> > @@ -60,7 +60,8 @@ A small area of SRAM is reserved for SCPI communication between application
> >  processors and SCP.
> >
> >  Required properties:
> > -- compatible : should be "arm,juno-sram-ns" for Non-secure SRAM on Juno
> > +- compatible : should be "arm,juno-sram-ns" for Non-secure SRAM on Juno,
> > +               or "amlogic,meson-gxbb-sram" for Amlogic GXBB SoC.
> 
> Maybe you'd be better of with a meson-specific document that refers to
> these but with different compatible values.
> 
> Come to think of it, the Juno-specific one maybe shouldn't be in
> arm,scpi at all, since that adds confusion here.
> 
> It's somewhat confusing that ARM is both a platform, architecture and
> in some cases implementer of specific systems. :)
> 

Sorry for that, I will move all juno specific references in the binding
out of this document(except the examples, which I assume should be fine)

> >  The rest of the properties should follow the generic mmio-sram description
> >  found in ../../sram/sram.txt
> > @@ -70,7 +71,8 @@ Each sub-node represents the reserved area for SCPI.
> >  Required sub-node properties:
> >  - reg : The base offset and size of the reserved area with the SRAM
> >  - compatible : should be "arm,juno-scp-shmem" for Non-secure SRAM based
> > -              shared memory on Juno platforms
> > +              shared memory on Juno platforms or
> > +              "amlogic,meson-gxbb-scp-shmem" for Amlogic GXBB SoC.
> 
> Same here. It won't scale if all vendors are expected to add an entry here.
> 

I will rework the patches to address the concerns as I too did share same
concern.


Hi Neil,

You may need to rework the DTS files based on that, please be aware of
that and make the necessary changes.

--
Regards,
Sudeep

[PATCH v4 6/8] dt-bindings: Add support for Amlogic GXBB SCPI Interface

[Rob Herring]

On Wed, Nov 2, 2016 at 5:20 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> On Sat, Oct 29, 2016 at 11:39:05AM -0700, Olof Johansson wrote:
>> Hi,
>>
>>
>> On Wed, Oct 5, 2016 at 12:33 AM, Neil Armstrong <narmstrong@baylibre.com> wrote:
>> > Acked-by: Rob Herring <robh@kernel.org>
>> > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> > ---
>> >  Documentation/devicetree/bindings/arm/arm,scpi.txt | 8 +++++---
>> >  1 file changed, 5 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/Documentation/devicetree/bindings/arm/arm,scpi.txt b/Documentation/devicetree/bindings/arm/arm,scpi.txt
>> > index faa4b44..04bc171 100644
>> > --- a/Documentation/devicetree/bindings/arm/arm,scpi.txt
>> > +++ b/Documentation/devicetree/bindings/arm/arm,scpi.txt
>> > @@ -7,7 +7,7 @@ by Linux to initiate various system control and power operations.
>> >
>> >  Required properties:
>> >
>> > -- compatible : should be "arm,scpi"
>> > +- compatible : should be "arm,scpi" or "amlogic,meson-gxbb-scpi"
>>
>> This doesn't seem right to document here. If anything you might want
>> to have a table of more-specific-compatibles for specific
>> implementations, but "arm,scpi" should still be the compatible of the
>> node (just not the most specific one).
>>
>
> I completely agree with you and I was pushing for a generic "arm,legacy-scpi"
> compatible until this binding was acked by Rob.

Just because I ack something, that doesn't mean don't review or
comment on it further.

>
> Anyways, I will add the generic compatible and post the changes.
>
>> Also, documenting it here indiciates that non-amlogic implementations
>> can/should use that compatible, which is misleading.
>>
>
> Agreed, it's better to keep them out of this generic binding document.
>
>> >  - mboxes: List of phandle and mailbox channel specifiers
>> >           All the channels reserved by remote SCP firmware for use by
>> >           SCPI message protocol should be specified in any order
>> > @@ -60,7 +60,8 @@ A small area of SRAM is reserved for SCPI communication between application
>> >  processors and SCP.
>> >
>> >  Required properties:
>> > -- compatible : should be "arm,juno-sram-ns" for Non-secure SRAM on Juno
>> > +- compatible : should be "arm,juno-sram-ns" for Non-secure SRAM on Juno,
>> > +               or "amlogic,meson-gxbb-sram" for Amlogic GXBB SoC.
>>
>> Maybe you'd be better of with a meson-specific document that refers to
>> these but with different compatible values.
>>
>> Come to think of it, the Juno-specific one maybe shouldn't be in
>> arm,scpi at all, since that adds confusion here.
>>
>> It's somewhat confusing that ARM is both a platform, architecture and
>> in some cases implementer of specific systems. :)
>>
>
> Sorry for that, I will move all juno specific references in the binding
> out of this document(except the examples, which I assume should be fine)
>
>> >  The rest of the properties should follow the generic mmio-sram description
>> >  found in ../../sram/sram.txt
>> > @@ -70,7 +71,8 @@ Each sub-node represents the reserved area for SCPI.
>> >  Required sub-node properties:
>> >  - reg : The base offset and size of the reserved area with the SRAM
>> >  - compatible : should be "arm,juno-scp-shmem" for Non-secure SRAM based
>> > -              shared memory on Juno platforms
>> > +              shared memory on Juno platforms or
>> > +              "amlogic,meson-gxbb-scp-shmem" for Amlogic GXBB SoC.
>>
>> Same here. It won't scale if all vendors are expected to add an entry here.
>>
>
> I will rework the patches to address the concerns as I too did share same
> concern.

Guess I was optimistic that *every* platform wouldn't be different in
some way. I should know better by now...

Rob

[PATCH v4 6/8] dt-bindings: Add support for Amlogic GXBB SCPI Interface

[Sudeep Holla]

On 02/11/16 21:51, Rob Herring wrote:
> On Wed, Nov 2, 2016 at 5:20 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> On Sat, Oct 29, 2016 at 11:39:05AM -0700, Olof Johansson wrote:
>>> Hi,
>>>
>>>
>>> On Wed, Oct 5, 2016 at 12:33 AM, Neil Armstrong <narmstrong@baylibre.com> wrote:
>>>> Acked-by: Rob Herring <robh@kernel.org>
>>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>>> ---
>>>>  Documentation/devicetree/bindings/arm/arm,scpi.txt | 8 +++++---
>>>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/arm/arm,scpi.txt b/Documentation/devicetree/bindings/arm/arm,scpi.txt
>>>> index faa4b44..04bc171 100644
>>>> --- a/Documentation/devicetree/bindings/arm/arm,scpi.txt
>>>> +++ b/Documentation/devicetree/bindings/arm/arm,scpi.txt
>>>> @@ -7,7 +7,7 @@ by Linux to initiate various system control and power operations.
>>>>
>>>>  Required properties:
>>>>
>>>> -- compatible : should be "arm,scpi"
>>>> +- compatible : should be "arm,scpi" or "amlogic,meson-gxbb-scpi"
>>>
>>> This doesn't seem right to document here. If anything you might want
>>> to have a table of more-specific-compatibles for specific
>>> implementations, but "arm,scpi" should still be the compatible of the
>>> node (just not the most specific one).
>>>
>>
>> I completely agree with you and I was pushing for a generic "arm,legacy-scpi"
>> compatible until this binding was acked by Rob.
>
> Just because I ack something, that doesn't mean don't review or
> comment on it further.
>

Sorry I didn't mean to say that. I was initially pushing for the generic
binding and since it was reworked many times already, I didn't want to
postpone any further just for sake of that. I completely understand that
component maintainers have to review the bindings too. So clearly it was
my mistake.

>>
>> Anyways, I will add the generic compatible and post the changes.
>>
>>> Also, documenting it here indiciates that non-amlogic implementations
>>> can/should use that compatible, which is misleading.
>>>
>>
>> Agreed, it's better to keep them out of this generic binding document.
>>
>>>>  - mboxes: List of phandle and mailbox channel specifiers
>>>>           All the channels reserved by remote SCP firmware for use by
>>>>           SCPI message protocol should be specified in any order
>>>> @@ -60,7 +60,8 @@ A small area of SRAM is reserved for SCPI communication between application
>>>>  processors and SCP.
>>>>
>>>>  Required properties:
>>>> -- compatible : should be "arm,juno-sram-ns" for Non-secure SRAM on Juno
>>>> +- compatible : should be "arm,juno-sram-ns" for Non-secure SRAM on Juno,
>>>> +               or "amlogic,meson-gxbb-sram" for Amlogic GXBB SoC.
>>>
>>> Maybe you'd be better of with a meson-specific document that refers to
>>> these but with different compatible values.
>>>
>>> Come to think of it, the Juno-specific one maybe shouldn't be in
>>> arm,scpi at all, since that adds confusion here.
>>>
>>> It's somewhat confusing that ARM is both a platform, architecture and
>>> in some cases implementer of specific systems. :)
>>>
>>
>> Sorry for that, I will move all juno specific references in the binding
>> out of this document(except the examples, which I assume should be fine)
>>
>>>>  The rest of the properties should follow the generic mmio-sram description
>>>>  found in ../../sram/sram.txt
>>>> @@ -70,7 +71,8 @@ Each sub-node represents the reserved area for SCPI.
>>>>  Required sub-node properties:
>>>>  - reg : The base offset and size of the reserved area with the SRAM
>>>>  - compatible : should be "arm,juno-scp-shmem" for Non-secure SRAM based
>>>> -              shared memory on Juno platforms
>>>> +              shared memory on Juno platforms or
>>>> +              "amlogic,meson-gxbb-scp-shmem" for Amlogic GXBB SoC.
>>>
>>> Same here. It won't scale if all vendors are expected to add an entry here.
>>>
>>
>> I will rework the patches to address the concerns as I too did share same
>> concern.
>
> Guess I was optimistic that *every* platform wouldn't be different in
> some way. I should know better by now...

:)

-- 
Regards,
Sudeep

[PATCH v4 6/8] dt-bindings: Add support for Amlogic GXBB SCPI Interface

[Neil Armstrong]

On 11/02/2016 11:20 PM, Sudeep Holla wrote:
> On Sat, Oct 29, 2016 at 11:39:05AM -0700, Olof Johansson wrote:
> 
> I will rework the patches to address the concerns as I too did share same
> concern.
> 
> 
> Hi Neil,
> 
> You may need to rework the DTS files based on that, please be aware of
> that and make the necessary changes.


Hi,

I will post the necessary changes since kevin already applied the previous patches.

Thanks,
Neil

> 
> --
> Regards,
> Sudeep
> 

[PATCH v4 7/8] ARM64: dts: meson-gxbb: Add SRAM node

[Neil Armstrong]

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
index 443811b..88f98f5 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
@@ -94,6 +94,15 @@
 			status = "disabled";
 		};
 
+		sram: sram at c8000000 {
+			compatible = "amlogic,meson-gxbb-sram", "mmio-sram";
+			reg = <0x0 0xc8000000 0x0 0x14000>;
+
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0 0x0 0xc8000000 0x14000>;
+		};
+
 		usb0: usb at c9000000 {
 			compatible = "amlogic,meson-gxbb-usb", "snps,dwc2";
 			reg = <0x0 0xc9000000 0x0 0x40000>;
-- 
1.9.1

[PATCH v4 8/8] ARM64: dts: meson-gxbb: Add SCPI with cpufreq & sensors Nodes

[Neil Armstrong]

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 48 +++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
index 88f98f5..3fdb2d7 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
@@ -74,6 +74,28 @@
 		};
 	};
 
+	scpi {
+		compatible = "amlogic,meson-gxbb-scpi";
+		mboxes = <&mailbox 1 &mailbox 2>;
+		shmem = <&cpu_scp_lpri &cpu_scp_hpri>;
+
+		clocks {
+			compatible = "arm,scpi-clocks";
+
+			scpi_dvfs: scpi_clocks at 0 {
+				compatible = "arm,scpi-dvfs-clocks";
+				#clock-cells = <1>;
+				clock-indices = <0>;
+				clock-output-names = "vcpu";
+			};
+		};
+
+		scpi_sensors: sensors {
+			compatible = "arm,scpi-sensors";
+			#thermal-sensor-cells = <1>;
+		};
+	};
+
 	soc {
 		usb0_phy: phy at c0000000 {
 			compatible = "amlogic,meson-gxbb-usb2-phy";
@@ -101,6 +123,16 @@
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges = <0 0x0 0xc8000000 0x14000>;
+
+			cpu_scp_lpri: scp-shmem at 0 {
+				compatible = "amlogic,meson-gxbb-scp-shmem";
+				reg = <0x13000 0x400>;
+			};
+
+			cpu_scp_hpri: scp-shmem at 200 {
+				compatible = "amlogic,meson-gxbb-scp-shmem";
+				reg = <0x13400 0x400>;
+			};
 		};
 
 		usb0: usb at c9000000 {
@@ -143,6 +175,22 @@
 	};
 };
 
+&cpu0 {
+	clocks = <&scpi_dvfs 0>;
+};
+
+&cpu1 {
+	clocks = <&scpi_dvfs 0>;
+};
+
+&cpu2 {
+	clocks = <&scpi_dvfs 0>;
+};
+
+&cpu3 {
+	clocks = <&scpi_dvfs 0>;
+};
+
 &cbus {
 	reset: reset-controller at 4404 {
 		compatible = "amlogic,meson-gxbb-reset";
-- 
1.9.1