[PATCH v3 0/8] scpi: Add support for legacy SCPI protocol

[PATCH v3 0/8] scpi: 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 v2 at : http://lkml.kernel.org/r/1471952816-30877-1-git-send-email-narmstrong@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@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

Neil Armstrong (8):
  scpi: Add cmd indirection table to prepare for legacy commands
  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

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

-- 
1.9.1

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

[Neil Armstrong]

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

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

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index 4388937..9a87687 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;
@@ -390,6 +424,19 @@ static u32 scpi_get_version(void)
 	return scpi_info->protocol_version;
 }
 
+static inline int check_cmd(unsigned int offset)
+{
+	if (offset >= CMD_MAX_COUNT ||
+	    !scpi_info ||
+	    !scpi_info->scpi_cmds)
+		return -EINVAL;
+
+	if (scpi_info->scpi_cmds[offset] < 0)
+		return -EOPNOTSUPP;
+
+	return 0;
+}
+
 static int
 scpi_clk_get_range(u16 clk_id, unsigned long *min, unsigned long *max)
 {
@@ -397,8 +444,13 @@ scpi_clk_get_range(u16 clk_id, unsigned long *min, unsigned long *max)
 	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,
-				sizeof(le_clk_id), &clk, sizeof(clk));
+	ret = check_cmd(CMD_GET_CLOCK_INFO);
+	if (ret)
+		return ret;
+
+	ret = scpi_send_message(scpi_info->scpi_cmds[CMD_GET_CLOCK_INFO],
+				&le_clk_id, sizeof(le_clk_id),
+				&clk, sizeof(clk));
 	if (!ret) {
 		*min = le32_to_cpu(clk.min_rate);
 		*max = le32_to_cpu(clk.max_rate);
@@ -412,20 +464,32 @@ 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,
-				sizeof(le_clk_id), &clk, sizeof(clk));
+	ret = check_cmd(CMD_GET_CLOCK_VALUE);
+	if (ret)
+		return ret;
+
+	ret = scpi_send_message(scpi_info->scpi_cmds[CMD_GET_CLOCK_VALUE],
+				&le_clk_id, sizeof(le_clk_id),
+				&clk, sizeof(clk));
+
 	return ret ? ret : le32_to_cpu(clk.rate);
 }
 
 static int scpi_clk_set_val(u16 clk_id, unsigned long rate)
 {
+	int ret;
 	int stat;
 	struct clk_set_value clk = {
 		.id = cpu_to_le16(clk_id),
 		.rate = cpu_to_le32(rate)
 	};
 
-	return scpi_send_message(SCPI_CMD_SET_CLOCK_VALUE, &clk, sizeof(clk),
+	ret = check_cmd(CMD_SET_CLOCK_VALUE);
+	if (ret)
+		return ret;
+
+	return scpi_send_message(scpi_info->scpi_cmds[CMD_SET_CLOCK_VALUE],
+				 &clk, sizeof(clk),
 				 &stat, sizeof(stat));
 }
 
@@ -434,17 +498,29 @@ 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 = check_cmd(CMD_GET_DVFS);
+	if (ret)
+		return ret;
+
+	ret = scpi_send_message(scpi_info->scpi_cmds[CMD_GET_DVFS],
+				&domain, sizeof(domain),
 				&dvfs_idx, sizeof(dvfs_idx));
+
 	return ret ? ret : dvfs_idx;
 }
 
 static int scpi_dvfs_set_idx(u8 domain, u8 index)
 {
+	int ret;
 	int stat;
 	struct dvfs_set dvfs = {domain, index};
 
-	return scpi_send_message(SCPI_CMD_SET_DVFS, &dvfs, sizeof(dvfs),
+	ret = check_cmd(CMD_SET_DVFS);
+	if (ret)
+		return ret;
+
+	return scpi_send_message(scpi_info->scpi_cmds[CMD_SET_DVFS],
+				 &dvfs, sizeof(dvfs),
 				 &stat, sizeof(stat));
 }
 
@@ -468,9 +544,13 @@ 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),
-				&buf, sizeof(buf));
+	ret = check_cmd(CMD_GET_DVFS_INFO);
+	if (ret)
+		return ERR_PTR(ret);
 
+	ret = scpi_send_message(scpi_info->scpi_cmds[CMD_GET_DVFS_INFO],
+				&domain, sizeof(domain),
+				&buf, sizeof(buf));
 	if (ret)
 		return ERR_PTR(ret);
 
@@ -503,8 +583,12 @@ 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 = check_cmd(CMD_SENSOR_CAPABILITIES);
+	if (ret)
+		return ret;
+
+	ret = scpi_send_message(scpi_info->scpi_cmds[CMD_SENSOR_CAPABILITIES],
+				NULL, 0, &cap_buf, sizeof(cap_buf));
 	if (!ret)
 		*sensors = le16_to_cpu(cap_buf.sensors);
 
@@ -517,7 +601,12 @@ 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 = check_cmd(CMD_SENSOR_INFO);
+	if (ret)
+		return ret;
+
+	ret = scpi_send_message(scpi_info->scpi_cmds[CMD_SENSOR_INFO],
+				&id, sizeof(id),
 				&_info, sizeof(_info));
 	if (!ret) {
 		memcpy(info, &_info, sizeof(*info));
@@ -533,8 +622,12 @@ 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),
-				&buf, sizeof(buf));
+	ret = check_cmd(CMD_SENSOR_VALUE);
+	if (ret)
+		return ret;
+
+	ret = scpi_send_message(scpi_info->scpi_cmds[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);
@@ -548,21 +641,31 @@ 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 = check_cmd(CMD_GET_DEVICE_PWR_STATE);
+	if (ret)
+		return ret;
+
+	ret = scpi_send_message(scpi_info->scpi_cmds[CMD_GET_DEVICE_PWR_STATE],
+				&id, sizeof(id), &pstate, sizeof(pstate));
 	return ret ? ret : pstate;
 }
 
 static int scpi_device_set_power_state(u16 dev_id, u8 pstate)
 {
+	int ret;
 	int stat;
 	struct dev_pstate_set dev_set = {
 		.dev_id = cpu_to_le16(dev_id),
 		.pstate = pstate,
 	};
 
-	return scpi_send_message(SCPI_CMD_SET_DEVICE_PWR_STATE, &dev_set,
-				 sizeof(dev_set), &stat, sizeof(stat));
+	ret = check_cmd(CMD_SET_DEVICE_PWR_STATE);
+	if (ret)
+		return ret;
+
+	return scpi_send_message(scpi_info->scpi_cmds[CMD_SET_DEVICE_PWR_STATE],
+				 &dev_set, sizeof(dev_set),
+				 &stat, sizeof(stat));
 }
 
 static struct scpi_ops scpi_ops = {
@@ -591,6 +694,10 @@ static int scpi_init_versions(struct scpi_drvinfo *info)
 	int ret;
 	struct scp_capabilities caps;
 
+	ret = check_cmd(CMD_SCPI_CAPABILITIES);
+	if (ret)
+		return ret;
+
 	ret = scpi_send_message(SCPI_CMD_SCPI_CAPABILITIES, NULL, 0,
 				&caps, sizeof(caps));
 	if (!ret) {
@@ -756,6 +863,8 @@ err:
 	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

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

[Sudeep Holla]

Hi Neil,

On 07/09/16 16:34, Neil Armstrong wrote:
> Add indirection table to permit multiple command values for legacy support.
>

I wrote the most of the patch and you changed the author too ;)

> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/firmware/arm_scpi.c | 145 ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 127 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
> index 4388937..9a87687 100644
> --- a/drivers/firmware/arm_scpi.c
> +++ b/drivers/firmware/arm_scpi.c

[..]

> @@ -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;
> @@ -390,6 +424,19 @@ static u32 scpi_get_version(void)
>  	return scpi_info->protocol_version;
>  }
>
> +static inline int check_cmd(unsigned int offset)
> +{
> +	if (offset >= CMD_MAX_COUNT ||

If we call scpi_send_message internally(as it's static) why is this
check needed ?


> +	    !scpi_info ||
> +	    !scpi_info->scpi_cmds)

Will be even reach to this point if above is true ?

> +		return -EINVAL;
> +
> +	if (scpi_info->scpi_cmds[offset] < 0)
> +		return -EOPNOTSUPP;

IMO just above couple of lines in the beginning of scpi_send_message
will suffice. You can just add this to my original patch.

>  static int
>  scpi_clk_get_range(u16 clk_id, unsigned long *min, unsigned long *max)
>  {
> @@ -397,8 +444,13 @@ scpi_clk_get_range(u16 clk_id, unsigned long *min, unsigned long *max)
>  	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,
> -				sizeof(le_clk_id), &clk, sizeof(clk));
> +	ret = check_cmd(CMD_GET_CLOCK_INFO);
> +	if (ret)
> +		return ret;
> +

It's totally unnecessary to add check in each and every function calling
scpi_send_message, why not add it to scpi_send_message instead.

-- 
Regards,
Sudeep

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

[Neil Armstrong]

On 09/19/2016 04:41 PM, Sudeep Holla wrote:
> Hi Neil,
> 
> On 07/09/16 16:34, Neil Armstrong wrote:
>> Add indirection table to permit multiple command values for legacy support.
>>
> 
> I wrote the most of the patch and you changed the author too ;)

Sorry, forgot this ! v4 will have it !
> 
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/firmware/arm_scpi.c | 145 ++++++++++++++++++++++++++++++++++++++------
>>  1 file changed, 127 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
>> index 4388937..9a87687 100644
>> --- a/drivers/firmware/arm_scpi.c
>> +++ b/drivers/firmware/arm_scpi.c
> 
> [..]
> 
>> @@ -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;
>> @@ -390,6 +424,19 @@ static u32 scpi_get_version(void)
>>      return scpi_info->protocol_version;
>>  }
>>
>> +static inline int check_cmd(unsigned int offset)
>> +{
>> +    if (offset >= CMD_MAX_COUNT ||
> 
> If we call scpi_send_message internally(as it's static) why is this
> check needed ?
> 
> 
>> +        !scpi_info ||
>> +        !scpi_info->scpi_cmds)
> 
> Will be even reach to this point if above is true ?
> 
>> +        return -EINVAL;
>> +
>> +    if (scpi_info->scpi_cmds[offset] < 0)
>> +        return -EOPNOTSUPP;
> 
> IMO just above couple of lines in the beginning of scpi_send_message
> will suffice. You can just add this to my original patch.

Will do.

> 
>>  static int
>>  scpi_clk_get_range(u16 clk_id, unsigned long *min, unsigned long *max)
>>  {
>> @@ -397,8 +444,13 @@ scpi_clk_get_range(u16 clk_id, unsigned long *min, unsigned long *max)
>>      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,
>> -                sizeof(le_clk_id), &clk, sizeof(clk));
>> +    ret = check_cmd(CMD_GET_CLOCK_INFO);
>> +    if (ret)
>> +        return ret;
>> +
> 
> It's totally unnecessary to add check in each and every function calling
> scpi_send_message, why not add it to scpi_send_message instead.
> 

This was my first thought, I should have stayed at this !

Thanks,
Neil

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

[Neil Armstrong]

In order to support the legacy SCPI protocol variant, add back the structures
and macros that varies against the final specification.
Add indirection table for legacy commands.
Add bitmap field for channel selection
Add support for legacy in scpi_send_message.

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

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index 9a87687..9ba1020 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -50,11 +50,16 @@
 #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_UNIQ_MASK	(CMD_TOKEN_ID_MASK << CMD_TOKEN_ID_SHIFT | CMD_ID_MASK)
@@ -133,6 +138,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 +225,21 @@ static int scpi_std_commands[CMD_MAX_COUNT] = {
 	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;
@@ -193,8 +268,10 @@ struct scpi_chan {
 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 +288,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 +318,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 {
@@ -336,6 +424,39 @@ static void scpi_handle_remote_msg(struct mbox_client *c, void *msg)
 	scpi_process_cmd(ch, cmd);
 }
 
+static void legacy_scpi_process_cmd(struct scpi_chan *ch)
+{
+	unsigned long flags;
+	struct scpi_xfer *t;
+
+	spin_lock_irqsave(&ch->rx_lock, flags);
+	if (list_empty(&ch->rx_pending)) {
+		spin_unlock_irqrestore(&ch->rx_lock, flags);
+		return;
+	}
+
+	t = list_first_entry(&ch->rx_pending, struct scpi_xfer, node);
+	list_del(&t->node);
+
+	/* check if wait_for_completion is in progress or timed-out */
+	if (t && !completion_done(&t->done)) {
+		struct legacy_scpi_shared_mem *mem = ch->rx_payload;
+		unsigned int len = t->rx_len;
+
+		t->status = le32_to_cpu(mem->status);
+		memcpy_fromio(t->rx_buf, mem->payload, len);
+		complete(&t->done);
+	}
+	spin_unlock_irqrestore(&ch->rx_lock, flags);
+}
+
+static void legacy_scpi_handle_remote_msg(struct mbox_client *c, void *_msg)
+{
+	struct scpi_chan *ch = container_of(c, struct scpi_chan, cl);
+
+	legacy_scpi_process_cmd(ch);
+}
+
 static void scpi_tx_prepare(struct mbox_client *c, void *msg)
 {
 	unsigned long flags;
@@ -356,6 +477,21 @@ static void scpi_tx_prepare(struct mbox_client *c, void *msg)
 	mem->command = cpu_to_le32(t->cmd);
 }
 
+static void legacy_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);
+
+	if (t->tx_buf)
+		memcpy_toio(ch->tx_payload, t->tx_buf, t->tx_len);
+	if (t->rx_buf) {
+		spin_lock_irqsave(&ch->rx_lock, flags);
+		list_add_tail(&t->node, &ch->rx_pending);
+		spin_unlock_irqrestore(&ch->rx_lock, flags);
+	}
+}
+
 static struct scpi_xfer *get_scpi_xfer(struct scpi_chan *ch)
 {
 	struct scpi_xfer *t;
@@ -386,15 +522,25 @@ static int scpi_send_message(u8 cmd, void *tx_buf, unsigned int tx_len,
 	struct scpi_xfer *msg;
 	struct scpi_chan *scpi_chan;
 
-	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) {
+		mutex_lock(&scpi_chan->xfers_lock);
+		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;
@@ -402,7 +548,7 @@ static int scpi_send_message(u8 cmd, void *tx_buf, unsigned int tx_len,
 	init_completion(&msg->done);
 
 	ret = mbox_send_message(scpi_chan->chan, msg);
-	if (ret < 0 || !rx_buf)
+	if (ret < 0 || (!scpi_info->is_legacy && !rx_buf))
 		goto out;
 
 	if (!wait_for_completion_timeout(&msg->done, MAX_RX_TIMEOUT))
@@ -411,7 +557,12 @@ static int scpi_send_message(u8 cmd, void *tx_buf, unsigned int tx_len,
 		/* first status word */
 		ret = msg->status;
 out:
-	if (ret < 0 && rx_buf) /* remove entry from the list if timed-out */
+	if (scpi_info->is_legacy) {
+		if (ret < 0)
+			legacy_scpi_process_cmd(scpi_chan);
+		mutex_unlock(&scpi_chan->xfers_lock);
+	} else if (ret < 0 && rx_buf)
+		/* remove entry from the list if timed-out */
 		scpi_process_cmd(scpi_chan, msg->cmd);
 
 	put_scpi_xfer(msg, scpi_chan);
@@ -493,6 +644,24 @@ 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 ret;
+	int stat;
+	struct legacy_clk_set_value clk = {
+		.id = cpu_to_le16(clk_id),
+		.rate = cpu_to_le32(rate)
+	};
+
+	ret = check_cmd(CMD_SET_CLOCK_VALUE);
+	if (ret)
+		return ret;
+
+	return scpi_send_message(scpi_info->scpi_cmds[CMD_SET_CLOCK_VALUE],
+				 &clk, sizeof(clk),
+				 &stat, sizeof(stat));
+}
+
 static int scpi_dvfs_get_idx(u8 domain)
 {
 	int ret;
@@ -635,6 +804,24 @@ static int scpi_sensor_get_value(u16 sensor, u64 *val)
 	return ret;
 }
 
+static int legacy_scpi_sensor_get_value(u16 sensor, u64 *val)
+{
+	__le16 id = cpu_to_le16(sensor);
+	struct sensor_value buf;
+	int ret;
+
+	ret = check_cmd(CMD_SENSOR_VALUE);
+	if (ret)
+		return ret;
+
+	ret = scpi_send_message(scpi_info->scpi_cmds[CMD_SENSOR_VALUE],
+				&id, sizeof(id), &buf, sizeof(buf));
+	if (!ret)
+		*val = (u64)le32_to_cpu(buf.lo_val);
+
+	return ret;
+}
+
 static int scpi_device_get_power_state(u16 dev_id)
 {
 	int ret;
@@ -832,8 +1019,13 @@ static int scpi_probe(struct platform_device *pdev)
 		pchan->tx_payload = pchan->rx_payload + (size >> 1);
 
 		cl->dev = dev;
-		cl->rx_callback = scpi_handle_remote_msg;
-		cl->tx_prepare = scpi_tx_prepare;
+		if (scpi_info->is_legacy) {
+			cl->rx_callback = legacy_scpi_handle_remote_msg;
+			cl->tx_prepare = legacy_scpi_tx_prepare;
+		} else {
+			cl->rx_callback = scpi_handle_remote_msg;
+			cl->tx_prepare = scpi_tx_prepare;
+		}
 		cl->tx_block = true;
 		cl->tx_tout = 20;
 		cl->knows_txdone = false; /* controller can't ack */
@@ -865,6 +1057,18 @@ err:
 
 	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_ops.sensor_get_value = legacy_scpi_sensor_get_value;
+		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

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

[Sudeep Holla]

On 07/09/16 16:34, Neil Armstrong wrote:
> In order to support the legacy SCPI protocol variant, add back the structures
> and macros that varies against the final specification.
> Add indirection table for legacy commands.
> Add bitmap field for channel selection
> Add support for legacy in scpi_send_message.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/firmware/arm_scpi.c | 218 ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 211 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
> index 9a87687..9ba1020 100644
> --- a/drivers/firmware/arm_scpi.c
> +++ b/drivers/firmware/arm_scpi.c

[..]

> @@ -336,6 +424,39 @@ static void scpi_handle_remote_msg(struct mbox_client *c, void *msg)
>  	scpi_process_cmd(ch, cmd);
>  }
>
> +static void legacy_scpi_process_cmd(struct scpi_chan *ch)
> +{
> +	unsigned long flags;
> +	struct scpi_xfer *t;
> +
> +	spin_lock_irqsave(&ch->rx_lock, flags);
> +	if (list_empty(&ch->rx_pending)) {
> +		spin_unlock_irqrestore(&ch->rx_lock, flags);
> +		return;
> +	}
> +
> +	t = list_first_entry(&ch->rx_pending, struct scpi_xfer, node);
> +	list_del(&t->node);
> +

This is a bad assumption that it will be always first. The legacy SCPI
did support multiple commands at a time and they can be reordered when
SCP responds to them. Except this it's almost same scpi_process_cmd. You
should be able to use it as is if you pass the command.

> +	/* check if wait_for_completion is in progress or timed-out */
> +	if (t && !completion_done(&t->done)) {
> +		struct legacy_scpi_shared_mem *mem = ch->rx_payload;
> +		unsigned int len = t->rx_len;
> +
> +		t->status = le32_to_cpu(mem->status);
> +		memcpy_fromio(t->rx_buf, mem->payload, len);
> +		complete(&t->done);
> +	}
> +	spin_unlock_irqrestore(&ch->rx_lock, flags);
> +}
> +
> +static void legacy_scpi_handle_remote_msg(struct mbox_client *c, void *_msg)
> +{
> +	struct scpi_chan *ch = container_of(c, struct scpi_chan, cl);
> +
> +	legacy_scpi_process_cmd(ch);

You will get the command in *_msg IIRC. So you can just pass that to
scpi_process_cmd. You can even reuse scpi_handle_remote_msg

diff --git i/drivers/firmware/arm_scpi.c w/drivers/firmware/arm_scpi.c
index edf1a3327041..165f2fc3b627 100644
--- i/drivers/firmware/arm_scpi.c
+++ w/drivers/firmware/arm_scpi.c
@@ -419,7 +419,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 (ch->is_legacy)
+               cmd = *(u32 *)msg;
+       else
+               cmd = le32_to_cpu(mem->command);

         scpi_process_cmd(ch, cmd);
  }

> +}
> +
>  static void scpi_tx_prepare(struct mbox_client *c, void *msg)
>  {
>  	unsigned long flags;
> @@ -356,6 +477,21 @@ static void scpi_tx_prepare(struct mbox_client *c, void *msg)
>  	mem->command = cpu_to_le32(t->cmd);
>  }
>
> +static void legacy_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);
> +
> +	if (t->tx_buf)
> +		memcpy_toio(ch->tx_payload, t->tx_buf, t->tx_len);
> +	if (t->rx_buf) {
> +		spin_lock_irqsave(&ch->rx_lock, flags);
> +		list_add_tail(&t->node, &ch->rx_pending);
> +		spin_unlock_irqrestore(&ch->rx_lock, flags);
> +	}
> +}

Again here the only difference is token addition. I think we should
retain that as it's helpful in debugging and I don't think it will have
any issues. Worst case we can make it conditional but let's check if we
can retain it first.

> @@ -386,15 +522,25 @@ static int scpi_send_message(u8 cmd, void *tx_buf, unsigned int tx_len,
>  	struct scpi_xfer *msg;
>  	struct scpi_chan *scpi_chan;
>
> -	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) {
> +		mutex_lock(&scpi_chan->xfers_lock);

Why does legacy need a different locking scheme ?

[...]

> @@ -635,6 +804,24 @@ static int scpi_sensor_get_value(u16 sensor, u64 *val)
>  	return ret;
>  }
>
> +static int legacy_scpi_sensor_get_value(u16 sensor, u64 *val)
> +{
> +	__le16 id = cpu_to_le16(sensor);
> +	struct sensor_value buf;
> +	int ret;
> +
> +	ret = check_cmd(CMD_SENSOR_VALUE);
> +	if (ret)
> +		return ret;
> +
> +	ret = scpi_send_message(scpi_info->scpi_cmds[CMD_SENSOR_VALUE],
> +				&id, sizeof(id), &buf, sizeof(buf));
> +	if (!ret)
> +		*val = (u64)le32_to_cpu(buf.lo_val);
> +

This is not needed as it's backward compatible as discussed before.
Any particular reason you retained it here ?

-- 
Regards,
Sudeep

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

[Neil Armstrong]

On 09/19/2016 05:24 PM, Sudeep Holla wrote:
> 
> 
> On 07/09/16 16:34, Neil Armstrong wrote:
>> In order to support the legacy SCPI protocol variant, add back the structures
>> and macros that varies against the final specification.
>> Add indirection table for legacy commands.
>> Add bitmap field for channel selection
>> Add support for legacy in scpi_send_message.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/firmware/arm_scpi.c | 218 ++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 211 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
>> index 9a87687..9ba1020 100644
>> --- a/drivers/firmware/arm_scpi.c
>> +++ b/drivers/firmware/arm_scpi.c
> 
> [..]
> 
>> @@ -336,6 +424,39 @@ static void scpi_handle_remote_msg(struct mbox_client *c, void *msg)
>>      scpi_process_cmd(ch, cmd);
>>  }
>>
>> +static void legacy_scpi_process_cmd(struct scpi_chan *ch)
>> +{
>> +    unsigned long flags;
>> +    struct scpi_xfer *t;
>> +
>> +    spin_lock_irqsave(&ch->rx_lock, flags);
>> +    if (list_empty(&ch->rx_pending)) {
>> +        spin_unlock_irqrestore(&ch->rx_lock, flags);
>> +        return;
>> +    }
>> +
>> +    t = list_first_entry(&ch->rx_pending, struct scpi_xfer, node);
>> +    list_del(&t->node);
>> +
> 
> This is a bad assumption that it will be always first. The legacy SCPI
> did support multiple commands at a time and they can be reordered when
> SCP responds to them. Except this it's almost same scpi_process_cmd. You
> should be able to use it as is if you pass the command.

I would be happy this was the case...

> 
>> +    /* check if wait_for_completion is in progress or timed-out */
>> +    if (t && !completion_done(&t->done)) {
>> +        struct legacy_scpi_shared_mem *mem = ch->rx_payload;
>> +        unsigned int len = t->rx_len;
>> +
>> +        t->status = le32_to_cpu(mem->status);
>> +        memcpy_fromio(t->rx_buf, mem->payload, len);
>> +        complete(&t->done);
>> +    }
>> +    spin_unlock_irqrestore(&ch->rx_lock, flags);
>> +}
>> +
>> +static void legacy_scpi_handle_remote_msg(struct mbox_client *c, void *_msg)
>> +{
>> +    struct scpi_chan *ch = container_of(c, struct scpi_chan, cl);
>> +
>> +    legacy_scpi_process_cmd(ch);
> 
> You will get the command in *_msg IIRC. So you can just pass that to
> scpi_process_cmd. You can even reuse scpi_handle_remote_msg

But Amlogic SCP firmware does not answer the command but only the first bit...
so we cannot queue commands because we cannot find back the queued command
from the replied MHU STAT value.

> 
> diff --git i/drivers/firmware/arm_scpi.c w/drivers/firmware/arm_scpi.c
> index edf1a3327041..165f2fc3b627 100644
> --- i/drivers/firmware/arm_scpi.c
> +++ w/drivers/firmware/arm_scpi.c
> @@ -419,7 +419,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 (ch->is_legacy)
> +               cmd = *(u32 *)msg;
> +       else
> +               cmd = le32_to_cpu(mem->command);
> 
>         scpi_process_cmd(ch, cmd);
>  }
> 
>> +}
>> +
>>  static void scpi_tx_prepare(struct mbox_client *c, void *msg)
>>  {
>>      unsigned long flags;
>> @@ -356,6 +477,21 @@ static void scpi_tx_prepare(struct mbox_client *c, void *msg)
>>      mem->command = cpu_to_le32(t->cmd);
>>  }
>>
>> +static void legacy_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);
>> +
>> +    if (t->tx_buf)
>> +        memcpy_toio(ch->tx_payload, t->tx_buf, t->tx_len);
>> +    if (t->rx_buf) {
>> +        spin_lock_irqsave(&ch->rx_lock, flags);
>> +        list_add_tail(&t->node, &ch->rx_pending);
>> +        spin_unlock_irqrestore(&ch->rx_lock, flags);
>> +    }
>> +}
> 
> Again here the only difference is token addition. I think we should
> retain that as it's helpful in debugging and I don't think it will have
> any issues. Worst case we can make it conditional but let's check if we
> can retain it first.

Yes token addition works.

> 
>> @@ -386,15 +522,25 @@ static int scpi_send_message(u8 cmd, void *tx_buf, unsigned int tx_len,
>>      struct scpi_xfer *msg;
>>      struct scpi_chan *scpi_chan;
>>
>> -    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) {
>> +        mutex_lock(&scpi_chan->xfers_lock);
> 
> Why does legacy need a different locking scheme ?

Since we cannot queue, locking seems a really good idea...

> 
> [...]
> 
>> @@ -635,6 +804,24 @@ static int scpi_sensor_get_value(u16 sensor, u64 *val)
>>      return ret;
>>  }
>>
>> +static int legacy_scpi_sensor_get_value(u16 sensor, u64 *val)
>> +{
>> +    __le16 id = cpu_to_le16(sensor);
>> +    struct sensor_value buf;
>> +    int ret;
>> +
>> +    ret = check_cmd(CMD_SENSOR_VALUE);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = scpi_send_message(scpi_info->scpi_cmds[CMD_SENSOR_VALUE],
>> +                &id, sizeof(id), &buf, sizeof(buf));
>> +    if (!ret)
>> +        *val = (u64)le32_to_cpu(buf.lo_val);
>> +
> 
> This is not needed as it's backward compatible as discussed before.
> Any particular reason you retained it here ?
> 

Sudeep,

I merged the commands as asked but Amlogic's SCP firmware only replies the value 1 in the MHU STAT registers.

This implies that :
 - We cannot distinguish what command ended in scpi_handle_remote_msg
 - We cannot find the last rx command in rx_pending list
 - We cannot read rx data length from the replied command
 - We cannot push multiple commands
 - We also need to wait for TX commands completion
 - We need locking in scpi_send_message around mbox_send_message and completion

I have an highly tweaked version with simplified path for legacy but with merged handle_remote_msg, tx_prepare and send_message functions
I will post shortly.

Neil

[PATCH v3 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 9ba1020..6a16100 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -891,6 +891,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 v3 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 6a16100..60a76e63 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -979,6 +979,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;
@@ -991,6 +996,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);
@@ -1097,6 +1105,7 @@ err:
 
 static const struct of_device_id scpi_of_match[] = {
 	{.compatible = "arm,scpi"},
+	{.compatible = "amlogic,meson-gxbb-scpi"},
 	{},
 };
 
-- 
1.9.1

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

[Sudeep Holla]

On 07/09/16 16:34, Neil Armstrong wrote:
> 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 6a16100..60a76e63 100644
> --- a/drivers/firmware/arm_scpi.c
> +++ b/drivers/firmware/arm_scpi.c
> @@ -979,6 +979,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"},
> +	{},
> +};

This needs to be documented.

> +
>  static int scpi_probe(struct platform_device *pdev)
>  {
>  	int count, idx, ret;
> @@ -991,6 +996,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);
> @@ -1097,6 +1105,7 @@ err:
>
>  static const struct of_device_id scpi_of_match[] = {
>  	{.compatible = "arm,scpi"},
> +	{.compatible = "amlogic,meson-gxbb-scpi"},

I also prefer adding "arm,legacy-scpi". So far AMLogic has followed the
legacy specification except the capabilities. You could use
"amlogic,meson-gxbb-scpi" for that and "arm,legacy-scpi" for the probe
part or we can add the use of "amlogic,meson-gxbb-scpi" later but just
document it now.

-- 
Regards,
Sudeep

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

[Sudeep Holla]

On 19/09/16 16:50, Sudeep Holla wrote:
>
>
> On 07/09/16 16:34, Neil Armstrong wrote:
>> 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 6a16100..60a76e63 100644
>> --- a/drivers/firmware/arm_scpi.c
>> +++ b/drivers/firmware/arm_scpi.c
>> @@ -979,6 +979,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"},
>> +    {},
>> +};
>
> This needs to be documented.
>

Ignore that I see you have done that in the later patch.

-- 
Regards,
Sudeep

[PATCH v3 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 60a76e63..b422ad3 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -68,7 +68,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 v3 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 v3 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 bdbf6e7..2748007 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
@@ -124,6 +124,15 @@
 		#size-cells = <2>;
 		ranges;
 
+		sram: sram@c8000000 {
+			compatible = "amlogic,meson-gxbb-sram", "mmio-sram";
+			reg = <0x0 0xc8000000 0x0 0x14000>;
+
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0 0x0 0xc8000000 0x14000>;
+		};
+
 		cbus: cbus@c1100000 {
 			compatible = "simple-bus";
 			reg = <0x0 0xc1100000 0x0 0x100000>;
-- 
1.9.1

[PATCH v3 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 | 36 +++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
index 2748007..257845a 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
@@ -61,6 +61,7 @@
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x0>;
 			enable-method = "psci";
+			clocks = <&scpi_dvfs 0>;
 		};
 
 		cpu1: cpu@1 {
@@ -68,6 +69,7 @@
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x1>;
 			enable-method = "psci";
+			clocks = <&scpi_dvfs 0>;
 		};
 
 		cpu2: cpu@2 {
@@ -75,6 +77,7 @@
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x2>;
 			enable-method = "psci";
+			clocks = <&scpi_dvfs 0>;
 		};
 
 		cpu3: cpu@3 {
@@ -82,6 +85,7 @@
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x3>;
 			enable-method = "psci";
+			clocks = <&scpi_dvfs 0>;
 		};
 	};
 
@@ -99,6 +103,28 @@
 		method = "smc";
 	};
 
+	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@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>;
+		};
+	};
+
 	timer {
 		compatible = "arm,armv8-timer";
 		interrupts = <GIC_PPI 13
@@ -131,6 +157,16 @@
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges = <0 0x0 0xc8000000 0x14000>;
+
+			cpu_scp_lpri: scp-shmem@0 {
+				compatible = "amlogic,meson-gxbb-scp-shmem";
+				reg = <0x13000 0x400>;
+			};
+
+			cpu_scp_hpri: scp-shmem@200 {
+				compatible = "amlogic,meson-gxbb-scp-shmem";
+				reg = <0x13400 0x400>;
+			};
 		};
 
 		cbus: cbus@c1100000 {
-- 
1.9.1