[PATCH 00/13] scpi: Add support for legacy SCPI protocol

[PATCH 00/13] 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 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

The first patch is here to provide vendor commands on the official SCPI protocol,
it can be delayed to another patchset.

Patches 8 & 9 are only here to demo how Rockchip support could be implemented, these
patches should be delayed to a rockchip specific patchset.

The last patch depends on the "Platform MHU" dtsi patch.

Neil Armstrong (13):
  scpi: Add vendor_send_message to enable access to vendor commands
  scpi: Add alternative legacy structures and macros
  scpi: Add legacy send, prepare and handle remote functions
  scpi: Add legacy SCP functions calling legacy_scpi_send_message
  scpi: move of_match table before probe functions
  scpi: add priv_scpi_ops and fill legacy structure
  scpi: ignore init_versions failure if reported not supported
  scpi: add a vendor_msg mechanism in case the mailbox message differs
  scpi: implement rockchip support via the vendor_msg mechanism
  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                        | 455 ++++++++++++++++++++-
 include/linux/scpi_protocol.h                      |   4 +
 4 files changed, 490 insertions(+), 22 deletions(-)

-- 
1.9.1

[PATCH 01/13] scpi: Add vendor_send_message to enable access to vendor commands

[Neil Armstrong]

Adds an optional vendor_send_message to the scpi to enable sending
vendor platform specific commands to the SCP firmware.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/firmware/arm_scpi.c   | 26 +++++++++++++++++++++++---
 include/linux/scpi_protocol.h |  4 ++++
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index 4388937..403783a 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -46,6 +46,8 @@
 
 #define CMD_ID_SHIFT		0
 #define CMD_ID_MASK		0x7f
+#define CMD_SET_SHIFT		7
+#define CMD_SET_MASK		0x1
 #define CMD_TOKEN_ID_SHIFT	8
 #define CMD_TOKEN_ID_MASK	0xff
 #define CMD_DATA_SIZE_SHIFT	16
@@ -53,6 +55,10 @@
 #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 PACK_EXT_SCPI_CMD(cmd_id, tx_sz)		\
+	((((cmd_id) & CMD_ID_MASK) << CMD_ID_SHIFT) |	\
+	(CMD_SET_MASK << CMD_SET_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))
 
@@ -344,8 +350,8 @@ 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(u8 cmd, void *tx_buf, unsigned int tx_len,
+			       void *rx_buf, unsigned int rx_len, bool extn)
 {
 	int ret;
 	u8 chan;
@@ -360,7 +366,8 @@ static int scpi_send_message(u8 cmd, void *tx_buf, unsigned int tx_len,
 		return -ENOMEM;
 
 	msg->slot = BIT(SCPI_SLOT);
-	msg->cmd = PACK_SCPI_CMD(cmd, tx_len);
+	msg->cmd = extn ? PACK_EXT_SCPI_CMD(cmd, tx_len) :
+			  PACK_SCPI_CMD(cmd, tx_len);
 	msg->tx_buf = tx_buf;
 	msg->tx_len = tx_len;
 	msg->rx_buf = rx_buf;
@@ -385,6 +392,18 @@ out:
 	return ret > 0 ? scpi_to_linux_errno(ret) : ret;
 }
 
+static int scpi_send_message(u8 cmd, void *tx_buf, unsigned int tx_len,
+			     void *rx_buf, unsigned int rx_len)
+{
+	return __scpi_send_message(cmd, tx_buf, tx_len, rx_buf, rx_len, false);
+}
+
+static int scpi_ext_send_message(u8 cmd, void *tx_buf, unsigned int tx_len,
+				 void *rx_buf, unsigned int rx_len)
+{
+	return __scpi_send_message(cmd, tx_buf, tx_len, rx_buf, rx_len, true);
+}
+
 static u32 scpi_get_version(void)
 {
 	return scpi_info->protocol_version;
@@ -578,6 +597,7 @@ static struct scpi_ops scpi_ops = {
 	.sensor_get_value = scpi_sensor_get_value,
 	.device_get_power_state = scpi_device_get_power_state,
 	.device_set_power_state = scpi_device_set_power_state,
+	.vendor_send_message = scpi_ext_send_message,
 };
 
 struct scpi_ops *get_scpi_ops(void)
diff --git a/include/linux/scpi_protocol.h b/include/linux/scpi_protocol.h
index dc5f989..2b68581 100644
--- a/include/linux/scpi_protocol.h
+++ b/include/linux/scpi_protocol.h
@@ -58,6 +58,8 @@ struct scpi_sensor_info {
  *	OPP is an index to the list return by @dvfs_get_info
  * @dvfs_get_info: returns the DVFS capabilities of the given power
  *	domain. It includes the OPP list and the latency information
+ * @vendor_send_message: vendor specific message sending, arg can specify
+ *	a scpi implementation specific argument
  */
 struct scpi_ops {
 	u32 (*get_version)(void);
@@ -72,6 +74,8 @@ struct scpi_ops {
 	int (*sensor_get_value)(u16, u64 *);
 	int (*device_get_power_state)(u16);
 	int (*device_set_power_state)(u16, u8);
+	int (*vendor_send_message)(u8 cmd, void *tx_buf, unsigned int tx_len,
+				   void *rx_buf, unsigned int rx_len);
 };
 
 #if IS_REACHABLE(CONFIG_ARM_SCPI_PROTOCOL)
-- 
1.9.1

Re: [PATCH 01/13] scpi: Add vendor_send_message to enable access to vendor commands

[Sudeep Holla]

On 18/08/16 11:10, Neil Armstrong wrote:
> Adds an optional vendor_send_message to the scpi to enable sending
> vendor platform specific commands to the SCP firmware.
>

I don't see any users of vendor_send_message in this series, so I prefer
it to be dropped and introduced when required.

Also I had a different view on how to introduce this[1]. I would rather
wait until the requirement comes that enables us to make use of it.
Looks like you took parts of it and introduces vendor_send_message
allowing users to send any data which I don't like especially without
knowing how will it be (ab)used.

-- 
Regards,
Sudeep

[1] http://www.spinics.net/lists/kernel/msg2263649.html

Re: [PATCH 01/13] scpi: Add vendor_send_message to enable access to vendor commands

[Neil Armstrong]

On 08/18/2016 05:53 PM, Sudeep Holla wrote:
> 
> 
> On 18/08/16 11:10, Neil Armstrong wrote:
>> Adds an optional vendor_send_message to the scpi to enable sending
>> vendor platform specific commands to the SCP firmware.
>>
> 
> I don't see any users of vendor_send_message in this series, so I prefer
> it to be dropped and introduced when required.
> 
> Also I had a different view on how to introduce this[1]. I would rather
> wait until the requirement comes that enables us to make use of it.
> Looks like you took parts of it and introduces vendor_send_message
> allowing users to send any data which I don't like especially without
> knowing how will it be (ab)used.
> 

Hi Sudeep,

Indeed, I won't ask you to merge patches 1, 8 and 9, they need refactoring and
a separate patchset.

Vendor commands are not necessary for Amlogic, this is a nice to have since they
have a "user" command that can be extended by a firmware loaded by u-boot.

Could you review the following patches and I'll post a reduced v2 ASAP.

Thanks,
Neil

Re: [PATCH 01/13] scpi: Add vendor_send_message to enable access to vendor commands

[Sudeep Holla]

On 19/08/16 09:00, Neil Armstrong wrote:
> On 08/18/2016 05:53 PM, Sudeep Holla wrote:
>>
>>
>> On 18/08/16 11:10, Neil Armstrong wrote:
>>> Adds an optional vendor_send_message to the scpi to enable sending
>>> vendor platform specific commands to the SCP firmware.
>>>
>>
>> I don't see any users of vendor_send_message in this series, so I prefer
>> it to be dropped and introduced when required.
>>
>> Also I had a different view on how to introduce this[1]. I would rather
>> wait until the requirement comes that enables us to make use of it.
>> Looks like you took parts of it and introduces vendor_send_message
>> allowing users to send any data which I don't like especially without
>> knowing how will it be (ab)used.
>>
>
> Hi Sudeep,
>
> Indeed, I won't ask you to merge patches 1, 8 and 9, they need refactoring and
> a separate patchset.
>

Ah OK,then better if you put then at the end. Otherwise it makes it
difficult to review.

> Vendor commands are not necessary for Amlogic, this is a nice to have
> since they have a "user" command that can be extended by a firmware
> loaded by u-boot.
>

Good :)

> Could you review the following patches and I'll post a reduced v2
> ASAP.
>

Yes I was doing that yesterday, sorry got distracted. I will continue today.

-- 
Regards,
Sudeep

[PATCH 02/13] scpi: Add alternative legacy structures 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.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
I checked against Amlogic implementation documentation and on-device, the channel selection
via legacy_scpi_get_chan() is needed and fails without it.
The sender_id is not needed so it was dropped.

 drivers/firmware/arm_scpi.c | 84 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index 403783a..0bb6134 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -52,6 +52,8 @@
 #define CMD_TOKEN_ID_MASK	0xff
 #define CMD_DATA_SIZE_SHIFT	16
 #define CMD_DATA_SIZE_MASK	0x1ff
+#define CMD_LEGACY_DATA_SIZE_SHIFT	20
+#define CMD_LEGACY_DATA_SIZE_MASK	0x1ff
 #define PACK_SCPI_CMD(cmd_id, tx_sz)			\
 	((((cmd_id) & CMD_ID_MASK) << CMD_ID_SHIFT) |	\
 	(((tx_sz) & CMD_DATA_SIZE_MASK) << CMD_DATA_SIZE_SHIFT))
@@ -61,6 +63,9 @@
 	(((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)
@@ -138,6 +143,42 @@ enum scpi_std_cmd {
 	SCPI_CMD_COUNT
 };
 
+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
+};
+
 struct scpi_xfer {
 	u32 slot; /* has to be first element */
 	u32 cmd;
@@ -183,6 +224,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;
@@ -208,6 +254,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 {
@@ -237,6 +289,10 @@ struct sensor_value {
 	__le32 hi_val;
 } __packed;
 
+struct legacy_sensor_value {
+	__le32 val;
+} __packed;
+
 struct dev_pstate_set {
 	u16 dev_id;
 	u8 pstate;
@@ -328,6 +384,34 @@ static void scpi_tx_prepare(struct mbox_client *c, void *msg)
 	mem->command = cpu_to_le32(t->cmd);
 }
 
+static int legacy_high_priority_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,
+};
+
+static int legacy_scpi_get_chan(u8 cmd)
+{
+	int idx;
+
+	for (idx = 0; idx < ARRAY_SIZE(legacy_high_priority_cmds); idx++)
+		if (cmd == legacy_high_priority_cmds[idx])
+			return 1;
+
+	return 0;
+}
+
 static struct scpi_xfer *get_scpi_xfer(struct scpi_chan *ch)
 {
 	struct scpi_xfer *t;
-- 
1.9.1

Re: [PATCH 02/13] scpi: Add alternative legacy structures and macros

[Sudeep Holla]

On 18/08/16 11:10, Neil Armstrong wrote:
> In order to support the legacy SCPI protocol variant, add back the structures
> and macros that varies against the final specification.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
> I checked against Amlogic implementation documentation and on-device, the channel selection
> via legacy_scpi_get_chan() is needed and fails without it.
> The sender_id is not needed so it was dropped.
>
>  drivers/firmware/arm_scpi.c | 84 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
>
> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
> index 403783a..0bb6134 100644
> --- a/drivers/firmware/arm_scpi.c
> +++ b/drivers/firmware/arm_scpi.c

[...]

> @@ -183,6 +224,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;
> @@ -208,6 +254,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 {
> @@ -237,6 +289,10 @@ struct sensor_value {
>  	__le32 hi_val;
>  } __packed;
>
> +struct legacy_sensor_value {
> +	__le32 val;
> +} __packed;
> +

Not required, new one is backward compatible. Otherwise this patch looks
fine.

-- 
Regards,
Sudeep

Re: [PATCH 02/13] scpi: Add alternative legacy structures and macros

[Neil Armstrong]

On 08/18/2016 07:16 PM, Sudeep Holla wrote:
> 
> 
> On 18/08/16 11:10, Neil Armstrong wrote:
>> In order to support the legacy SCPI protocol variant, add back the structures
>> and macros that varies against the final specification.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>> I checked against Amlogic implementation documentation and on-device, the channel selection
>> via legacy_scpi_get_chan() is needed and fails without it.
>> The sender_id is not needed so it was dropped.
>>
>>  drivers/firmware/arm_scpi.c | 84 +++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 84 insertions(+)
>>
>> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
>> index 403783a..0bb6134 100644
>> --- a/drivers/firmware/arm_scpi.c
>> +++ b/drivers/firmware/arm_scpi.c
> 
> [...]
> 
>> @@ -183,6 +224,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;
>> @@ -208,6 +254,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 {
>> @@ -237,6 +289,10 @@ struct sensor_value {
>>      __le32 hi_val;
>>  } __packed;
>>
>> +struct legacy_sensor_value {
>> +    __le32 val;
>> +} __packed;
>> +
> 
> Not required, new one is backward compatible. Otherwise this patch looks
> fine.

Indeed, will use the non-legacy structure.

Thanks,
Neil

[PATCH 03/13] scpi: Add legacy send, prepare and handle remote functions

[Neil Armstrong]

In order to support the legacy SCPI procotol, add specific message_send,
prepare_tx and handle_remote functions since the legacy procotol
do not support message queuing and does not store the command word in the
tx_payload data.

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

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index 0bb6134..50b1297 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -202,6 +202,7 @@ struct scpi_chan {
 	spinlock_t rx_lock; /* locking for the rx pending list */
 	struct mutex xfers_lock;
 	u8 token;
+	struct scpi_xfer *t;
 };
 
 struct scpi_drvinfo {
@@ -364,6 +365,23 @@ static void scpi_handle_remote_msg(struct mbox_client *c, void *msg)
 	scpi_process_cmd(ch, cmd);
 }
 
+static void legacy_scpi_handle_remote_msg(struct mbox_client *c, void *__msg)
+{
+	struct scpi_chan *ch =
+		container_of(c, struct scpi_chan, cl);
+	struct legacy_scpi_shared_mem *mem = ch->rx_payload;
+	unsigned int len;
+
+	len = ch->t->rx_len;
+
+	ch->t->status = le32_to_cpu(mem->status);
+
+	if (len)
+		memcpy_fromio(ch->t->rx_buf, mem->payload, len);
+
+	complete(&ch->t->done);
+}
+
 static void scpi_tx_prepare(struct mbox_client *c, void *msg)
 {
 	unsigned long flags;
@@ -384,6 +402,15 @@ 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)
+{
+	struct scpi_chan *ch =
+		container_of(c, struct scpi_chan, cl);
+
+	if (ch->t->tx_buf && ch->t->tx_len)
+		memcpy_toio(ch->tx_payload, ch->t->tx_buf, ch->t->tx_len);
+}
+
 static int legacy_high_priority_cmds[] = {
 	LEGACY_SCPI_CMD_GET_CSS_PWR_STATE,
 	LEGACY_SCPI_CMD_CFG_PWR_STATE_STAT,
@@ -434,6 +461,48 @@ static void put_scpi_xfer(struct scpi_xfer *t, struct scpi_chan *ch)
 	mutex_unlock(&ch->xfers_lock);
 }
 
+static int legacy_scpi_send_message(u8 cmd, void *tx_buf, unsigned int tx_len,
+				void *rx_buf, unsigned int rx_len)
+{
+	int ret;
+	u8 chan;
+	struct scpi_xfer *msg;
+	struct scpi_chan *scpi_chan;
+
+	chan = legacy_scpi_get_chan(cmd);
+	scpi_chan = scpi_info->channels + chan;
+
+	msg = get_scpi_xfer(scpi_chan);
+	if (!msg)
+		return -ENOMEM;
+
+	mutex_lock(&scpi_chan->xfers_lock);
+
+	msg->cmd = PACK_LEGACY_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);
+	scpi_chan->t = msg;
+
+	ret = mbox_send_message(scpi_chan->chan, &msg->cmd);
+	if (ret < 0)
+		goto out;
+
+	if (!wait_for_completion_timeout(&msg->done, MAX_RX_TIMEOUT))
+		ret = -ETIMEDOUT;
+	else
+		/* first status word */
+		ret = msg->status;
+out:
+	mutex_unlock(&scpi_chan->xfers_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;
+}
+
 static int __scpi_send_message(u8 cmd, void *tx_buf, unsigned int tx_len,
 			       void *rx_buf, unsigned int rx_len, bool extn)
 {
-- 
1.9.1

Re: [PATCH 03/13] scpi: Add legacy send, prepare and handle remote functions

[Sudeep Holla]

On 18/08/16 11:10, Neil Armstrong wrote:
> In order to support the legacy SCPI procotol, add specific message_send,
> prepare_tx and handle_remote functions since the legacy procotol
> do not support message queuing and does not store the command word in the
> tx_payload data.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/firmware/arm_scpi.c | 69 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
>
> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
> index 0bb6134..50b1297 100644
> --- a/drivers/firmware/arm_scpi.c
> +++ b/drivers/firmware/arm_scpi.c
> @@ -202,6 +202,7 @@ struct scpi_chan {
>  	spinlock_t rx_lock; /* locking for the rx pending list */
>  	struct mutex xfers_lock;
>  	u8 token;
> +	struct scpi_xfer *t;
>  };
>
>  struct scpi_drvinfo {
> @@ -364,6 +365,23 @@ static void scpi_handle_remote_msg(struct mbox_client *c, void *msg)
>  	scpi_process_cmd(ch, cmd);
>  }
>
> +static void legacy_scpi_handle_remote_msg(struct mbox_client *c, void *__msg)
> +{
> +	struct scpi_chan *ch =
> +		container_of(c, struct scpi_chan, cl);
> +	struct legacy_scpi_shared_mem *mem = ch->rx_payload;
> +	unsigned int len;
> +
> +	len = ch->t->rx_len;
> +
> +	ch->t->status = le32_to_cpu(mem->status);
> +
> +	if (len)
> +		memcpy_fromio(ch->t->rx_buf, mem->payload, len);
> +
> +	complete(&ch->t->done);
> +}
> +
>  static void scpi_tx_prepare(struct mbox_client *c, void *msg)
>  {
>  	unsigned long flags;
> @@ -384,6 +402,15 @@ 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)
> +{
> +	struct scpi_chan *ch =
> +		container_of(c, struct scpi_chan, cl);
> +
> +	if (ch->t->tx_buf && ch->t->tx_len)
> +		memcpy_toio(ch->tx_payload, ch->t->tx_buf, ch->t->tx_len);


I see that you are not using the list. Any particular reason for that ?

IMO, that *might* help to reuse more code, but I may be wrong. Let's see
Some commands like DVFS take more time compared to simple query type of
commands. Queuing does help there instead of blocking the channel until
the receipt of response.

> +}
> +
>  static int legacy_high_priority_cmds[] = {
>  	LEGACY_SCPI_CMD_GET_CSS_PWR_STATE,
>  	LEGACY_SCPI_CMD_CFG_PWR_STATE_STAT,
> @@ -434,6 +461,48 @@ static void put_scpi_xfer(struct scpi_xfer *t, struct scpi_chan *ch)
>  	mutex_unlock(&ch->xfers_lock);
>  }
>
> +static int legacy_scpi_send_message(u8 cmd, void *tx_buf, unsigned int tx_len,
> +				void *rx_buf, unsigned int rx_len)
> +{
> +	int ret;
> +	u8 chan;
> +	struct scpi_xfer *msg;
> +	struct scpi_chan *scpi_chan;
> +
> +	chan = legacy_scpi_get_chan(cmd);
> +	scpi_chan = scpi_info->channels + chan;
> +
> +	msg = get_scpi_xfer(scpi_chan);
> +	if (!msg)
> +		return -ENOMEM;
> +
> +	mutex_lock(&scpi_chan->xfers_lock);
> +

May be you can copy msg->cmd to msg->slot and that may help to reuse
more code or worst-case keep them aligned.

> +	msg->cmd = PACK_LEGACY_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);
> +	scpi_chan->t = msg;
> +
> +	ret = mbox_send_message(scpi_chan->chan, &msg->cmd);

If slot is initialized to cmd, then you can pass msg itself above.
Then you can evaluate how much this function deviates from
scpi_send_message and try to re-use.

> +	if (ret < 0)
> +		goto out;
> +
> +	if (!wait_for_completion_timeout(&msg->done, MAX_RX_TIMEOUT))
> +		ret = -ETIMEDOUT;
> +	else
> +		/* first status word */
> +		ret = msg->status;
> +out:
> +	mutex_unlock(&scpi_chan->xfers_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;
> +}
> +
>  static int __scpi_send_message(u8 cmd, void *tx_buf, unsigned int tx_len,
>  			       void *rx_buf, unsigned int rx_len, bool extn)
>  {
>

[Nit]: Not sure if we need this as a separate patch. It might just
generate warnings, anyways we can merge into one later.

-- 
Regards,
Sudeep

Re: [PATCH 03/13] scpi: Add legacy send, prepare and handle remote functions

[Neil Armstrong]

On 08/19/2016 06:13 PM, Sudeep Holla wrote:
> 
> 
> On 18/08/16 11:10, Neil Armstrong wrote:
>> In order to support the legacy SCPI procotol, add specific message_send,
>> prepare_tx and handle_remote functions since the legacy procotol
>> do not support message queuing and does not store the command word in the
>> tx_payload data.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/firmware/arm_scpi.c | 69 +++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 69 insertions(+)
>>
>> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
>> index 0bb6134..50b1297 100644
>> --- a/drivers/firmware/arm_scpi.c
>> +++ b/drivers/firmware/arm_scpi.c
>> @@ -202,6 +202,7 @@ struct scpi_chan {
>>      spinlock_t rx_lock; /* locking for the rx pending list */
>>      struct mutex xfers_lock;
>>      u8 token;
>> +    struct scpi_xfer *t;
>>  };
>>
>>  struct scpi_drvinfo {
>> @@ -364,6 +365,23 @@ static void scpi_handle_remote_msg(struct mbox_client *c, void *msg)
>>      scpi_process_cmd(ch, cmd);
>>  }
>>
>> +static void legacy_scpi_handle_remote_msg(struct mbox_client *c, void *__msg)
>> +{
>> +    struct scpi_chan *ch =
>> +        container_of(c, struct scpi_chan, cl);
>> +    struct legacy_scpi_shared_mem *mem = ch->rx_payload;
>> +    unsigned int len;
>> +
>> +    len = ch->t->rx_len;
>> +
>> +    ch->t->status = le32_to_cpu(mem->status);
>> +
>> +    if (len)
>> +        memcpy_fromio(ch->t->rx_buf, mem->payload, len);
>> +
>> +    complete(&ch->t->done);
>> +}
>> +
>>  static void scpi_tx_prepare(struct mbox_client *c, void *msg)
>>  {
>>      unsigned long flags;
>> @@ -384,6 +402,15 @@ 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)
>> +{
>> +    struct scpi_chan *ch =
>> +        container_of(c, struct scpi_chan, cl);
>> +
>> +    if (ch->t->tx_buf && ch->t->tx_len)
>> +        memcpy_toio(ch->tx_payload, ch->t->tx_buf, ch->t->tx_len);
> 
> 
> I see that you are not using the list. Any particular reason for that ?
> 
> IMO, that *might* help to reuse more code, but I may be wrong. Let's see
> Some commands like DVFS take more time compared to simple query type of
> commands. Queuing does help there instead of blocking the channel until
> the receipt of response.

I'll like to use the list, but, the "cmd" value is not stored in the shared tx
memory, so we cannot recover the original tranfer from reading the tx memory cmd.

This is why I added a "struct scpi_xfer *t;" in the scpi_chan structure to store
the current transfer.

> 
>> +}
>> +
>>  static int legacy_high_priority_cmds[] = {
>>      LEGACY_SCPI_CMD_GET_CSS_PWR_STATE,
>>      LEGACY_SCPI_CMD_CFG_PWR_STATE_STAT,
>> @@ -434,6 +461,48 @@ static void put_scpi_xfer(struct scpi_xfer *t, struct scpi_chan *ch)
>>      mutex_unlock(&ch->xfers_lock);
>>  }
>>
>> +static int legacy_scpi_send_message(u8 cmd, void *tx_buf, unsigned int tx_len,
>> +                void *rx_buf, unsigned int rx_len)
>> +{
>> +    int ret;
>> +    u8 chan;
>> +    struct scpi_xfer *msg;
>> +    struct scpi_chan *scpi_chan;
>> +
>> +    chan = legacy_scpi_get_chan(cmd);
>> +    scpi_chan = scpi_info->channels + chan;
>> +
>> +    msg = get_scpi_xfer(scpi_chan);
>> +    if (!msg)
>> +        return -ENOMEM;
>> +
>> +    mutex_lock(&scpi_chan->xfers_lock);
>> +
> 
> May be you can copy msg->cmd to msg->slot and that may help to reuse
> more code or worst-case keep them aligned.

Yes, it could be. But since the msg is not reused bu the tx_prepare and handle_response,
we can pass anything here.
And for the rockchip case, we must pass an xfer unrelated pointer here since they need
a specially crafted memory structure for the mailbox.

> 
>> +    msg->cmd = PACK_LEGACY_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);
>> +    scpi_chan->t = msg;
>> +
>> +    ret = mbox_send_message(scpi_chan->chan, &msg->cmd);
> 
> If slot is initialized to cmd, then you can pass msg itself above.
> Then you can evaluate how much this function deviates from
> scpi_send_message and try to re-use.

The function deviates quite a lot since the queing is not used.

> 
>> +    if (ret < 0)
>> +        goto out;
>> +
>> +    if (!wait_for_completion_timeout(&msg->done, MAX_RX_TIMEOUT))
>> +        ret = -ETIMEDOUT;
>> +    else
>> +        /* first status word */
>> +        ret = msg->status;
>> +out:
>> +    mutex_unlock(&scpi_chan->xfers_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;
>> +}
>> +
>>  static int __scpi_send_message(u8 cmd, void *tx_buf, unsigned int tx_len,
>>                     void *rx_buf, unsigned int rx_len, bool extn)
>>  {
>>
> 
> [Nit]: Not sure if we need this as a separate patch. It might just
> generate warnings, anyways we can merge into one later.

I'll prefer to have functionnaly separate patches for now to clarify the changes.
I'll eventually merge them for the final apply if needed.

Thanks,
Neil

Re: [PATCH 03/13] scpi: Add legacy send, prepare and handle remote functions

[Sudeep Holla]

On 23/08/16 09:15, Neil Armstrong wrote:
> On 08/19/2016 06:13 PM, Sudeep Holla wrote:
>>
>>
>> On 18/08/16 11:10, Neil Armstrong wrote:
>>> In order to support the legacy SCPI procotol, add specific message_send,
>>> prepare_tx and handle_remote functions since the legacy procotol
>>> do not support message queuing and does not store the command word in the
>>> tx_payload data.
>>>
>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>> ---
>>>  drivers/firmware/arm_scpi.c | 69 +++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 69 insertions(+)
>>>
>>> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
>>> index 0bb6134..50b1297 100644
>>> --- a/drivers/firmware/arm_scpi.c
>>> +++ b/drivers/firmware/arm_scpi.c

[..]

>>>
>>> +static void legacy_scpi_tx_prepare(struct mbox_client *c, void *__msg)
>>> +{
>>> +    struct scpi_chan *ch =
>>> +        container_of(c, struct scpi_chan, cl);
>>> +
>>> +    if (ch->t->tx_buf && ch->t->tx_len)
>>> +        memcpy_toio(ch->tx_payload, ch->t->tx_buf, ch->t->tx_len);
>>
>>
>> I see that you are not using the list. Any particular reason for that ?
>>
>> IMO, that *might* help to reuse more code, but I may be wrong. Let's see
>> Some commands like DVFS take more time compared to simple query type of
>> commands. Queuing does help there instead of blocking the channel until
>> the receipt of response.
>
> I'll like to use the list, but, the "cmd" value is not stored in the shared tx
> memory, so we cannot recover the original tranfer from reading the tx memory cmd.
>

Even in the current driver we read the mem->command and search the list
in scpi_process_cmd. Instead *(u32 *)msg gives the command value, no ?

> This is why I added a "struct scpi_xfer *t;" in the scpi_chan structure to store
> the current transfer.
>

I don't like that. I am trying to get rid of that.
1. list is not being used
2. scpi_xfer is stashed even though we have only one command in
    progress at any time in your case.

>>
>>> +}
>>> +
>>>  static int legacy_high_priority_cmds[] = {
>>>      LEGACY_SCPI_CMD_GET_CSS_PWR_STATE,
>>>      LEGACY_SCPI_CMD_CFG_PWR_STATE_STAT,
>>> @@ -434,6 +461,48 @@ static void put_scpi_xfer(struct scpi_xfer *t, struct scpi_chan *ch)
>>>      mutex_unlock(&ch->xfers_lock);
>>>  }
>>>
>>> +static int legacy_scpi_send_message(u8 cmd, void *tx_buf, unsigned int tx_len,
>>> +                void *rx_buf, unsigned int rx_len)
>>> +{
>>> +    int ret;
>>> +    u8 chan;
>>> +    struct scpi_xfer *msg;
>>> +    struct scpi_chan *scpi_chan;
>>> +
>>> +    chan = legacy_scpi_get_chan(cmd);
>>> +    scpi_chan = scpi_info->channels + chan;
>>> +
>>> +    msg = get_scpi_xfer(scpi_chan);
>>> +    if (!msg)
>>> +        return -ENOMEM;
>>> +
>>> +    mutex_lock(&scpi_chan->xfers_lock);
>>> +
>>
>> May be you can copy msg->cmd to msg->slot and that may help to reuse
>> more code or worst-case keep them aligned.
>
> Yes, it could be. But since the msg is not reused bu the tx_prepare and handle_response,
> we can pass anything here.

IIUC, we should be able to handle it using msg pte in handle_remote_msg

> And for the rockchip case, we must pass an xfer unrelated pointer here since they need
> a specially crafted memory structure for the mailbox.
>

We can consider that when they upstream the driver. Let's not consider
it now.

>>
>>> +    msg->cmd = PACK_LEGACY_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);
>>> +    scpi_chan->t = msg;
>>> +
>>> +    ret = mbox_send_message(scpi_chan->chan, &msg->cmd);
>>
>> If slot is initialized to cmd, then you can pass msg itself above.
>> Then you can evaluate how much this function deviates from
>> scpi_send_message and try to re-use.
>
> The function deviates quite a lot since the queing is not used.
>

You have not given me the reason for not using the list yet.
If the msg ptr is used in scpi_handle_remote_msg, you should be able to
make use of it.

>>
>>> +    if (ret < 0)
>>> +        goto out;
>>> +
>>> +    if (!wait_for_completion_timeout(&msg->done, MAX_RX_TIMEOUT))
>>> +        ret = -ETIMEDOUT;
>>> +    else
>>> +        /* first status word */
>>> +        ret = msg->status;
>>> +out:
>>> +    mutex_unlock(&scpi_chan->xfers_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;
>>> +}
>>> +
>>>  static int __scpi_send_message(u8 cmd, void *tx_buf, unsigned int tx_len,
>>>                     void *rx_buf, unsigned int rx_len, bool extn)
>>>  {
>>>
>>
>> [Nit]: Not sure if we need this as a separate patch. It might just
>> generate warnings, anyways we can merge into one later.
>
> I'll prefer to have functionnaly separate patches for now to clarify the changes.
> I'll eventually merge them for the final apply if needed.
>

Ah OK, then fine.

-- 
Regards,
Sudeep

[PATCH 04/13] scpi: Add legacy SCP functions calling legacy_scpi_send_message

[Neil Armstrong]

In order to support legacy SCP functions from kernel-wide driver, add legacy
functions using the legacy command enums and calling legacy_scpi_send_message.

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

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index 50b1297..bb9965f 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -578,6 +578,8 @@ scpi_clk_get_range(u16 clk_id, unsigned long *min, unsigned long *max)
 	return ret;
 }
 
+/* scpi_clk_get_range not available for legacy */
+
 static unsigned long scpi_clk_get_val(u16 clk_id)
 {
 	int ret;
@@ -589,6 +591,18 @@ static unsigned long scpi_clk_get_val(u16 clk_id)
 	return ret ? ret : le32_to_cpu(clk.rate);
 }
 
+static unsigned long legacy_scpi_clk_get_val(u16 clk_id)
+{
+	int ret;
+	struct clk_get_value clk;
+	__le16 le_clk_id = cpu_to_le16(clk_id);
+
+	ret = legacy_scpi_send_message(LEGACY_SCPI_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 stat;
@@ -601,6 +615,19 @@ 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 legacy_scpi_send_message(LEGACY_SCPI_CMD_SET_CLOCK_VALUE,
+					&clk, sizeof(clk),
+					&stat, sizeof(stat));
+}
+
 static int scpi_dvfs_get_idx(u8 domain)
 {
 	int ret;
@@ -611,6 +638,17 @@ static int scpi_dvfs_get_idx(u8 domain)
 	return ret ? ret : dvfs_idx;
 }
 
+static int legacy_scpi_dvfs_get_idx(u8 domain)
+{
+	int ret;
+	u8 dvfs_idx;
+
+	ret = legacy_scpi_send_message(LEGACY_SCPI_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 stat;
@@ -620,6 +658,16 @@ static int scpi_dvfs_set_idx(u8 domain, u8 index)
 				 &stat, sizeof(stat));
 }
 
+static int legacy_scpi_dvfs_set_idx(u8 domain, u8 index)
+{
+	int stat;
+	struct dvfs_set dvfs = {domain, index};
+
+	return legacy_scpi_send_message(LEGACY_SCPI_CMD_SET_DVFS,
+					&dvfs, sizeof(dvfs),
+					&stat, sizeof(stat));
+}
+
 static int opp_cmp_func(const void *opp1, const void *opp2)
 {
 	const struct scpi_opp *t1 = opp1, *t2 = opp2;
@@ -627,6 +675,13 @@ static int opp_cmp_func(const void *opp1, const void *opp2)
 	return t1->freq - t2->freq;
 }
 
+static int legacy_scpi_dvfs_get_info(u8 domain, struct dvfs_info *buf)
+{
+	return legacy_scpi_send_message(LEGACY_SCPI_CMD_GET_DVFS_INFO,
+					&domain, sizeof(domain),
+					buf, sizeof(*buf));
+}
+
 static struct scpi_dvfs_info *scpi_dvfs_get_info(u8 domain)
 {
 	struct scpi_dvfs_info *info;
@@ -683,6 +738,20 @@ static int scpi_sensor_get_capability(u16 *sensors)
 	return ret;
 }
 
+static int legacy_scpi_sensor_get_capability(u16 *sensors)
+{
+	struct sensor_capabilities cap_buf;
+	int ret;
+
+	ret = legacy_scpi_send_message(LEGACY_SCPI_CMD_SENSOR_CAPABILITIES,
+				       NULL, 0,
+				       &cap_buf, sizeof(cap_buf));
+	if (!ret)
+		*sensors = le16_to_cpu(cap_buf.sensors);
+
+	return ret;
+}
+
 static int scpi_sensor_get_info(u16 sensor_id, struct scpi_sensor_info *info)
 {
 	__le16 id = cpu_to_le16(sensor_id);
@@ -699,6 +768,24 @@ static int scpi_sensor_get_info(u16 sensor_id, struct scpi_sensor_info *info)
 	return ret;
 }
 
+static int legacy_scpi_sensor_get_info(u16 sensor_id,
+				       struct scpi_sensor_info *info)
+{
+	__le16 id = cpu_to_le16(sensor_id);
+	struct _scpi_sensor_info _info;
+	int ret;
+
+	ret = legacy_scpi_send_message(LEGACY_SCPI_CMD_SENSOR_INFO,
+				       &id, sizeof(id),
+				       &_info, sizeof(_info));
+	if (!ret) {
+		memcpy(info, &_info, sizeof(*info));
+		info->sensor_id = le16_to_cpu(_info.sensor_id);
+	}
+
+	return ret;
+}
+
 static int scpi_sensor_get_value(u16 sensor, u64 *val)
 {
 	__le16 id = cpu_to_le16(sensor);
@@ -714,6 +801,21 @@ 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 legacy_sensor_value buf;
+	int ret;
+
+	ret = legacy_scpi_send_message(LEGACY_SCPI_CMD_SENSOR_VALUE,
+				       &id, sizeof(id),
+				       &buf, sizeof(buf));
+	if (!ret)
+		*val = (u64)le32_to_cpu(buf.val);
+
+	return ret;
+}
+
 static int scpi_device_get_power_state(u16 dev_id)
 {
 	int ret;
@@ -773,6 +875,22 @@ static int scpi_init_versions(struct scpi_drvinfo *info)
 	return ret;
 }
 
+static int legacy_scpi_init_versions(struct scpi_drvinfo *info)
+{
+	int ret;
+	struct scp_capabilities caps;
+
+	ret = legacy_scpi_send_message(LEGACY_SCPI_CMD_SCPI_CAPABILITIES,
+				       NULL, 0,
+				       &caps, sizeof(caps));
+	if (!ret) {
+		info->protocol_version = le32_to_cpu(caps.protocol_version);
+		info->firmware_version = le32_to_cpu(caps.platform_version);
+	}
+
+	return ret;
+}
+
 static ssize_t protocol_version_show(struct device *dev,
 				     struct device_attribute *attr, char *buf)
 {
-- 
1.9.1

Re: [PATCH 04/13] scpi: Add legacy SCP functions calling legacy_scpi_send_message

[Sudeep Holla]

On 18/08/16 11:10, Neil Armstrong wrote:
> In order to support legacy SCP functions from kernel-wide driver, add legacy
> functions using the legacy command enums and calling legacy_scpi_send_message.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/firmware/arm_scpi.c | 118 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 118 insertions(+)
>
> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
> index 50b1297..bb9965f 100644
> --- a/drivers/firmware/arm_scpi.c
> +++ b/drivers/firmware/arm_scpi.c
> @@ -578,6 +578,8 @@ scpi_clk_get_range(u16 clk_id, unsigned long *min, unsigned long *max)
>  	return ret;
>  }
>
> +/* scpi_clk_get_range not available for legacy */
> +
>  static unsigned long scpi_clk_get_val(u16 clk_id)
>  {
>  	int ret;
> @@ -589,6 +591,18 @@ static unsigned long scpi_clk_get_val(u16 clk_id)
>  	return ret ? ret : le32_to_cpu(clk.rate);
>  }
>
> +static unsigned long legacy_scpi_clk_get_val(u16 clk_id)
> +{
> +	int ret;
> +	struct clk_get_value clk;
> +	__le16 le_clk_id = cpu_to_le16(clk_id);
> +
> +	ret = legacy_scpi_send_message(LEGACY_SCPI_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 stat;
> @@ -601,6 +615,19 @@ 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 legacy_scpi_send_message(LEGACY_SCPI_CMD_SET_CLOCK_VALUE,
> +					&clk, sizeof(clk),
> +					&stat, sizeof(stat));

Except this one which has a different structure format, why do we need
to define legacy versions of other functions ? Can't we play with
function pointer or have a boolean in drvinfo structure and use then in
the existing functions as I had shown in one of the earlier emails.

-- 
Regards,
Sudeep

Re: [PATCH 04/13] scpi: Add legacy SCP functions calling legacy_scpi_send_message

[Neil Armstrong]

On 08/19/2016 06:22 PM, Sudeep Holla wrote:
> 
> 
> On 18/08/16 11:10, Neil Armstrong wrote:
>> In order to support legacy SCP functions from kernel-wide driver, add legacy
>> functions using the legacy command enums and calling legacy_scpi_send_message.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/firmware/arm_scpi.c | 118 ++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 118 insertions(+)
>>
>> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
>> index 50b1297..bb9965f 100644
>> --- a/drivers/firmware/arm_scpi.c
>> +++ b/drivers/firmware/arm_scpi.c
>> @@ -578,6 +578,8 @@ scpi_clk_get_range(u16 clk_id, unsigned long *min, unsigned long *max)
>>      return ret;
>>  }
>>
>> +/* scpi_clk_get_range not available for legacy */
>> +
>>  static unsigned long scpi_clk_get_val(u16 clk_id)
>>  {
>>      int ret;
>> @@ -589,6 +591,18 @@ static unsigned long scpi_clk_get_val(u16 clk_id)
>>      return ret ? ret : le32_to_cpu(clk.rate);
>>  }
>>
>> +static unsigned long legacy_scpi_clk_get_val(u16 clk_id)
>> +{
>> +    int ret;
>> +    struct clk_get_value clk;
>> +    __le16 le_clk_id = cpu_to_le16(clk_id);
>> +
>> +    ret = legacy_scpi_send_message(LEGACY_SCPI_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 stat;
>> @@ -601,6 +615,19 @@ 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 legacy_scpi_send_message(LEGACY_SCPI_CMD_SET_CLOCK_VALUE,
>> +                    &clk, sizeof(clk),
>> +                    &stat, sizeof(stat));
> 
> Except this one which has a different structure format, why do we need
> to define legacy versions of other functions ? Can't we play with
> function pointer or have a boolean in drvinfo structure and use then in
> the existing functions as I had shown in one of the earlier emails.
> 

The main problem is that the command indexes deviates starting at
SCPI_CMD_SET_CSS_PWR_STATE, I'll be pleased to know how to implement it.

Should I add a test :
if (scpi_drvinfo->is_legacy)
	legacy_scpi_send_message(...)
else
	scpi_send_message(...)

In each function ?

My strategy was to leave the "final" function untouched ans provide
alternatives to legacy.
I can add this "is_legacy" if/else instead of ops structures.

Please tell me how you'll implement this, so I'll adapt the merge.

Neil

Re: [PATCH 04/13] scpi: Add legacy SCP functions calling legacy_scpi_send_message

[Sudeep Holla]

On 23/08/16 09:19, Neil Armstrong wrote:
> On 08/19/2016 06:22 PM, Sudeep Holla wrote:
>>
>>
>> On 18/08/16 11:10, Neil Armstrong wrote:
>>> In order to support legacy SCP functions from kernel-wide driver, add legacy
>>> functions using the legacy command enums and calling legacy_scpi_send_message.
>>>
>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>> ---
>>>  drivers/firmware/arm_scpi.c | 118 ++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 118 insertions(+)
>>>
>>> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
>>> index 50b1297..bb9965f 100644
>>> --- a/drivers/firmware/arm_scpi.c
>>> +++ b/drivers/firmware/arm_scpi.c
>>> @@ -578,6 +578,8 @@ scpi_clk_get_range(u16 clk_id, unsigned long *min, unsigned long *max)
>>>      return ret;
>>>  }
>>>
>>> +/* scpi_clk_get_range not available for legacy */
>>> +
>>>  static unsigned long scpi_clk_get_val(u16 clk_id)
>>>  {
>>>      int ret;
>>> @@ -589,6 +591,18 @@ static unsigned long scpi_clk_get_val(u16 clk_id)
>>>      return ret ? ret : le32_to_cpu(clk.rate);
>>>  }
>>>
>>> +static unsigned long legacy_scpi_clk_get_val(u16 clk_id)
>>> +{
>>> +    int ret;
>>> +    struct clk_get_value clk;
>>> +    __le16 le_clk_id = cpu_to_le16(clk_id);
>>> +
>>> +    ret = legacy_scpi_send_message(LEGACY_SCPI_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 stat;
>>> @@ -601,6 +615,19 @@ 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 legacy_scpi_send_message(LEGACY_SCPI_CMD_SET_CLOCK_VALUE,
>>> +                    &clk, sizeof(clk),
>>> +                    &stat, sizeof(stat));
>>
>> Except this one which has a different structure format, why do we need
>> to define legacy versions of other functions ? Can't we play with
>> function pointer or have a boolean in drvinfo structure and use then in
>> the existing functions as I had shown in one of the earlier emails.
>>
>
> The main problem is that the command indexes deviates starting at
> SCPI_CMD_SET_CSS_PWR_STATE, I'll be pleased to know how to implement it.
>

Yes, I was thinking of some kind of mapping to new index using an array.

-- 
Regards,
Sudeep

[PATCH 05/13] scpi: move of_match table before probe functions

[Neil Armstrong]

Move the of_match table to prapre adding new compatible strings, with
match data in order to be used by the probe function.

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

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index bb9965f..b0d911b 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -972,6 +972,13 @@ static int scpi_alloc_xfer_list(struct device *dev, struct scpi_chan *ch)
 	return 0;
 }
 
+static const struct of_device_id scpi_of_match[] = {
+	{.compatible = "arm,scpi"},
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, scpi_of_match);
+
 static int scpi_probe(struct platform_device *pdev)
 {
 	int count, idx, ret;
@@ -1069,13 +1076,6 @@ err:
 	return of_platform_populate(dev->of_node, NULL, NULL, dev);
 }
 
-static const struct of_device_id scpi_of_match[] = {
-	{.compatible = "arm,scpi"},
-	{},
-};
-
-MODULE_DEVICE_TABLE(of, scpi_of_match);
-
 static struct platform_driver scpi_driver = {
 	.driver = {
 		.name = "scpi_protocol",
-- 
1.9.1

Re: [PATCH 05/13] scpi: move of_match table before probe functions

[Sudeep Holla]

On 18/08/16 11:10, Neil Armstrong wrote:
> Move the of_match table to prapre adding new compatible strings, with
> match data in order to be used by the probe function.
>

I understand the need of this change, but can be part of the patch
introducing the need for this.

-- 
Regards,
Sudeep

Re: [PATCH 05/13] scpi: move of_match table before probe functions

[Neil Armstrong]

On 08/19/2016 06:24 PM, Sudeep Holla wrote:
> 
> 
> On 18/08/16 11:10, Neil Armstrong wrote:
>> Move the of_match table to prapre adding new compatible strings, with
>> match data in order to be used by the probe function.
>>
> 
> I understand the need of this change, but can be part of the patch
> introducing the need for this.
> 

Yes, but I did not want to pollute the next patch, I'll merge them for v2.

Neil

[PATCH 06/13] scpi: add priv_scpi_ops and fill legacy structure

[Neil Armstrong]

In order to use the legacy functions variants, add a new priv_scpi_ops
structure that will contain the internal alterne functions and then use these
alternate call in the probe function.

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

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index b0d911b..3fe39fe 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -213,6 +213,7 @@ struct scpi_drvinfo {
 	struct scpi_ops *scpi_ops;
 	struct scpi_chan *channels;
 	struct scpi_dvfs_info *dvfs[MAX_DVFS_DOMAINS];
+	const struct priv_scpi_ops *ops;
 };
 
 /*
@@ -299,6 +300,17 @@ struct dev_pstate_set {
 	u8 pstate;
 } __packed;
 
+struct priv_scpi_ops {
+	/* Internal Specific Ops */
+	void (*handle_remote_msg)(struct mbox_client *c, void *msg);
+	void (*tx_prepare)(struct mbox_client *c, void *msg);
+	/* Message Specific Ops */
+	int (*init_versions)(struct scpi_drvinfo *info);
+	int (*dvfs_get_info)(u8 domain, struct dvfs_info *buf);
+	/* System wide Ops */
+	struct scpi_ops *scpi_ops;
+};
+
 static struct scpi_drvinfo *scpi_info;
 
 static int scpi_linux_errmap[SCPI_ERR_MAX] = {
@@ -695,9 +707,12 @@ 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),
+	if (scpi_info->ops && scpi_info->ops->dvfs_get_info)
+		ret = scpi_info->ops->dvfs_get_info(domain, &buf);
+	else
+		ret = scpi_send_message(SCPI_CMD_GET_DVFS_INFO,
+					&domain, sizeof(domain),
 				&buf, sizeof(buf));
-
 	if (ret)
 		return ERR_PTR(ret);
 
@@ -855,6 +870,22 @@ static struct scpi_ops scpi_ops = {
 	.vendor_send_message = scpi_ext_send_message,
 };
 
+static struct scpi_ops legacy_scpi_ops = {
+	.get_version = scpi_get_version,
+	.clk_get_range = NULL,
+	.clk_get_val = legacy_scpi_clk_get_val,
+	.clk_set_val = legacy_scpi_clk_set_val,
+	.dvfs_get_idx = legacy_scpi_dvfs_get_idx,
+	.dvfs_set_idx = legacy_scpi_dvfs_set_idx,
+	.dvfs_get_info = scpi_dvfs_get_info,
+	.sensor_get_capability = legacy_scpi_sensor_get_capability,
+	.sensor_get_info = legacy_scpi_sensor_get_info,
+	.sensor_get_value = legacy_scpi_sensor_get_value,
+	.device_get_power_state = NULL,
+	.device_set_power_state = NULL,
+	.vendor_send_message = legacy_scpi_send_message,
+};
+
 struct scpi_ops *get_scpi_ops(void)
 {
 	return scpi_info ? scpi_info->scpi_ops : NULL;
@@ -972,8 +1003,17 @@ static int scpi_alloc_xfer_list(struct device *dev, struct scpi_chan *ch)
 	return 0;
 }
 
+static const struct priv_scpi_ops scpi_legacy_ops = {
+	.handle_remote_msg = legacy_scpi_handle_remote_msg,
+	.tx_prepare = legacy_scpi_tx_prepare,
+	.init_versions = legacy_scpi_init_versions,
+	.dvfs_get_info = legacy_scpi_dvfs_get_info,
+	.scpi_ops = &legacy_scpi_ops,
+};
+
 static const struct of_device_id scpi_of_match[] = {
 	{.compatible = "arm,scpi"},
+	{.compatible = "amlogic,meson-gxbb-scpi", .data = &scpi_legacy_ops},
 	{},
 };
 
@@ -986,11 +1026,18 @@ static int scpi_probe(struct platform_device *pdev)
 	struct scpi_chan *scpi_chan;
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
+	const struct of_device_id *match;
+
+	match = of_match_device(scpi_of_match, &pdev->dev);
+	if (!match)
+		return -EINVAL;
 
 	scpi_info = devm_kzalloc(dev, sizeof(*scpi_info), GFP_KERNEL);
 	if (!scpi_info)
 		return -ENOMEM;
 
+	scpi_info->ops = match->data;
+
 	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);
@@ -1023,7 +1070,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;
+		if (scpi_info->ops && scpi_info->ops->handle_remote_msg)
+			cl->rx_callback = scpi_info->ops->handle_remote_msg;
+		else
+			cl->rx_callback = scpi_handle_remote_msg;
+		if (scpi_info->ops && scpi_info->ops->tx_prepare)
+			cl->tx_prepare = scpi_info->ops->tx_prepare;
+		else
 		cl->tx_prepare = scpi_tx_prepare;
 		cl->tx_block = true;
 		cl->tx_tout = 20;
@@ -1054,6 +1107,9 @@ err:
 	scpi_info->num_chans = count;
 	platform_set_drvdata(pdev, scpi_info);
 
+	if (scpi_info->ops && scpi_info->ops->init_versions)
+		ret = scpi_info->ops->init_versions(scpi_info);
+	else
 	ret = scpi_init_versions(scpi_info);
 	if (ret) {
 		dev_err(dev, "incorrect or no SCP firmware found\n");
@@ -1067,7 +1123,11 @@ err:
 		  FW_REV_MAJOR(scpi_info->firmware_version),
 		  FW_REV_MINOR(scpi_info->firmware_version),
 		  FW_REV_PATCH(scpi_info->firmware_version));
-	scpi_info->scpi_ops = &scpi_ops;
+
+	if (scpi_info->ops && scpi_info->ops->scpi_ops)
+		scpi_info->scpi_ops = scpi_info->ops->scpi_ops;
+	else
+		scpi_info->scpi_ops = &scpi_ops;
 
 	ret = sysfs_create_groups(&dev->kobj, versions_groups);
 	if (ret)
-- 
1.9.1

Re: [PATCH 06/13] scpi: add priv_scpi_ops and fill legacy structure

[Sudeep Holla]

On 18/08/16 11:10, Neil Armstrong wrote:
> In order to use the legacy functions variants, add a new priv_scpi_ops
> structure that will contain the internal alterne functions and then use these
> alternate call in the probe function.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/firmware/arm_scpi.c | 68 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 64 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
> index b0d911b..3fe39fe 100644
> --- a/drivers/firmware/arm_scpi.c
> +++ b/drivers/firmware/arm_scpi.c
> @@ -213,6 +213,7 @@ struct scpi_drvinfo {
>  	struct scpi_ops *scpi_ops;
>  	struct scpi_chan *channels;
>  	struct scpi_dvfs_info *dvfs[MAX_DVFS_DOMAINS];
> +	const struct priv_scpi_ops *ops;
>  };
>
>  /*
> @@ -299,6 +300,17 @@ struct dev_pstate_set {
>  	u8 pstate;
>  } __packed;
>
> +struct priv_scpi_ops {
> +	/* Internal Specific Ops */
> +	void (*handle_remote_msg)(struct mbox_client *c, void *msg);
> +	void (*tx_prepare)(struct mbox_client *c, void *msg);
> +	/* Message Specific Ops */
> +	int (*init_versions)(struct scpi_drvinfo *info);
> +	int (*dvfs_get_info)(u8 domain, struct dvfs_info *buf);
> +	/* System wide Ops */
> +	struct scpi_ops *scpi_ops;
> +};
> +


I fail to understand the need for this. Can you please explain the issue
you would face without this ?

>  static struct scpi_drvinfo *scpi_info;
>
>  static int scpi_linux_errmap[SCPI_ERR_MAX] = {
> @@ -695,9 +707,12 @@ 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),
> +	if (scpi_info->ops && scpi_info->ops->dvfs_get_info)
> +		ret = scpi_info->ops->dvfs_get_info(domain, &buf);
> +	else
> +		ret = scpi_send_message(SCPI_CMD_GET_DVFS_INFO,
> +					&domain, sizeof(domain),
>  				&buf, sizeof(buf));
> -
>  	if (ret)
>  		return ERR_PTR(ret);
>
> @@ -855,6 +870,22 @@ static struct scpi_ops scpi_ops = {
>  	.vendor_send_message = scpi_ext_send_message,
>  };
>
> +static struct scpi_ops legacy_scpi_ops = {
> +	.get_version = scpi_get_version,
> +	.clk_get_range = NULL,
> +	.clk_get_val = legacy_scpi_clk_get_val,
> +	.clk_set_val = legacy_scpi_clk_set_val,
> +	.dvfs_get_idx = legacy_scpi_dvfs_get_idx,
> +	.dvfs_set_idx = legacy_scpi_dvfs_set_idx,
> +	.dvfs_get_info = scpi_dvfs_get_info,
> +	.sensor_get_capability = legacy_scpi_sensor_get_capability,
> +	.sensor_get_info = legacy_scpi_sensor_get_info,
> +	.sensor_get_value = legacy_scpi_sensor_get_value,
> +	.device_get_power_state = NULL,
> +	.device_set_power_state = NULL,
> +	.vendor_send_message = legacy_scpi_send_message,

I think we need not have this at all if you follow the suggestion I had
in the previous patch. Try and let's see how it would look.

> +};
> +
>  struct scpi_ops *get_scpi_ops(void)
>  {
>  	return scpi_info ? scpi_info->scpi_ops : NULL;
> @@ -972,8 +1003,17 @@ static int scpi_alloc_xfer_list(struct device *dev, struct scpi_chan *ch)
>  	return 0;
>  }
>
> +static const struct priv_scpi_ops scpi_legacy_ops = {
> +	.handle_remote_msg = legacy_scpi_handle_remote_msg,
> +	.tx_prepare = legacy_scpi_tx_prepare,
> +	.init_versions = legacy_scpi_init_versions,
> +	.dvfs_get_info = legacy_scpi_dvfs_get_info,
> +	.scpi_ops = &legacy_scpi_ops,
> +};
> +

Ditto, can go away.

-- 
Regards,
Sudeep

Re: [PATCH 06/13] scpi: add priv_scpi_ops and fill legacy structure

[Neil Armstrong]

On 08/19/2016 06:39 PM, Sudeep Holla wrote:
> 
> 
> On 18/08/16 11:10, Neil Armstrong wrote:
>> In order to use the legacy functions variants, add a new priv_scpi_ops
>> structure that will contain the internal alterne functions and then use these
>> alternate call in the probe function.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/firmware/arm_scpi.c | 68 ++++++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 64 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
>> index b0d911b..3fe39fe 100644
>> --- a/drivers/firmware/arm_scpi.c
>> +++ b/drivers/firmware/arm_scpi.c
>> @@ -213,6 +213,7 @@ struct scpi_drvinfo {
>>      struct scpi_ops *scpi_ops;
>>      struct scpi_chan *channels;
>>      struct scpi_dvfs_info *dvfs[MAX_DVFS_DOMAINS];
>> +    const struct priv_scpi_ops *ops;
>>  };
>>
>>  /*
>> @@ -299,6 +300,17 @@ struct dev_pstate_set {
>>      u8 pstate;
>>  } __packed;
>>
>> +struct priv_scpi_ops {
>> +    /* Internal Specific Ops */
>> +    void (*handle_remote_msg)(struct mbox_client *c, void *msg);
>> +    void (*tx_prepare)(struct mbox_client *c, void *msg);
>> +    /* Message Specific Ops */
>> +    int (*init_versions)(struct scpi_drvinfo *info);
>> +    int (*dvfs_get_info)(u8 domain, struct dvfs_info *buf);
>> +    /* System wide Ops */
>> +    struct scpi_ops *scpi_ops;
>> +};
>> +
> 
> 
> I fail to understand the need for this. Can you please explain the issue
> you would face without this ?
> 
>>  static struct scpi_drvinfo *scpi_info;
>>
>>  static int scpi_linux_errmap[SCPI_ERR_MAX] = {
>> @@ -695,9 +707,12 @@ 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),
>> +    if (scpi_info->ops && scpi_info->ops->dvfs_get_info)
>> +        ret = scpi_info->ops->dvfs_get_info(domain, &buf);
>> +    else
>> +        ret = scpi_send_message(SCPI_CMD_GET_DVFS_INFO,
>> +                    &domain, sizeof(domain),
>>                  &buf, sizeof(buf));
>> -
>>      if (ret)
>>          return ERR_PTR(ret);
>>
>> @@ -855,6 +870,22 @@ static struct scpi_ops scpi_ops = {
>>      .vendor_send_message = scpi_ext_send_message,
>>  };
>>
>> +static struct scpi_ops legacy_scpi_ops = {
>> +    .get_version = scpi_get_version,
>> +    .clk_get_range = NULL,
>> +    .clk_get_val = legacy_scpi_clk_get_val,
>> +    .clk_set_val = legacy_scpi_clk_set_val,
>> +    .dvfs_get_idx = legacy_scpi_dvfs_get_idx,
>> +    .dvfs_set_idx = legacy_scpi_dvfs_set_idx,
>> +    .dvfs_get_info = scpi_dvfs_get_info,
>> +    .sensor_get_capability = legacy_scpi_sensor_get_capability,
>> +    .sensor_get_info = legacy_scpi_sensor_get_info,
>> +    .sensor_get_value = legacy_scpi_sensor_get_value,
>> +    .device_get_power_state = NULL,
>> +    .device_set_power_state = NULL,
>> +    .vendor_send_message = legacy_scpi_send_message,
> 
> I think we need not have this at all if you follow the suggestion I had
> in the previous patch. Try and let's see how it would look.

If you confirm you want the if/else as said in patch 4.

But clk_get_range, device_get/set_power_state are not available in legacy,
I think we should still have this alternate structure.

>> +};
>> +
>>  struct scpi_ops *get_scpi_ops(void)
>>  {
>>      return scpi_info ? scpi_info->scpi_ops : NULL;
>> @@ -972,8 +1003,17 @@ static int scpi_alloc_xfer_list(struct device *dev, struct scpi_chan *ch)
>>      return 0;
>>  }
>>
>> +static const struct priv_scpi_ops scpi_legacy_ops = {
>> +    .handle_remote_msg = legacy_scpi_handle_remote_msg,
>> +    .tx_prepare = legacy_scpi_tx_prepare,
>> +    .init_versions = legacy_scpi_init_versions,
>> +    .dvfs_get_info = legacy_scpi_dvfs_get_info,
>> +    .scpi_ops = &legacy_scpi_ops,
>> +};
>> +
> 
> Ditto, can go away.
> 

Yes, in the if/else is_legacy variant this should go away.

Thanks,
Neil

Re: [PATCH 06/13] scpi: add priv_scpi_ops and fill legacy structure

[Sudeep Holla]

On 23/08/16 09:22, Neil Armstrong wrote:
> On 08/19/2016 06:39 PM, Sudeep Holla wrote:
>>
>>
>> On 18/08/16 11:10, Neil Armstrong wrote:
>>> In order to use the legacy functions variants, add a new priv_scpi_ops
>>> structure that will contain the internal alterne functions and then use these
>>> alternate call in the probe function.
>>>
>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>> ---
>>>  drivers/firmware/arm_scpi.c | 68 ++++++++++++++++++++++++++++++++++++++++++---
>>>  1 file changed, 64 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
>>> index b0d911b..3fe39fe 100644
>>> --- a/drivers/firmware/arm_scpi.c
>>> +++ b/drivers/firmware/arm_scpi.c
>>> @@ -213,6 +213,7 @@ struct scpi_drvinfo {
>>>      struct scpi_ops *scpi_ops;
>>>      struct scpi_chan *channels;
>>>      struct scpi_dvfs_info *dvfs[MAX_DVFS_DOMAINS];
>>> +    const struct priv_scpi_ops *ops;
>>>  };
>>>
>>>  /*
>>> @@ -299,6 +300,17 @@ struct dev_pstate_set {
>>>      u8 pstate;
>>>  } __packed;
>>>
>>> +struct priv_scpi_ops {
>>> +    /* Internal Specific Ops */
>>> +    void (*handle_remote_msg)(struct mbox_client *c, void *msg);
>>> +    void (*tx_prepare)(struct mbox_client *c, void *msg);
>>> +    /* Message Specific Ops */
>>> +    int (*init_versions)(struct scpi_drvinfo *info);
>>> +    int (*dvfs_get_info)(u8 domain, struct dvfs_info *buf);
>>> +    /* System wide Ops */
>>> +    struct scpi_ops *scpi_ops;
>>> +};
>>> +
>>
>>
>> I fail to understand the need for this. Can you please explain the issue
>> you would face without this ?
>>
>>>  static struct scpi_drvinfo *scpi_info;
>>>
>>>  static int scpi_linux_errmap[SCPI_ERR_MAX] = {
>>> @@ -695,9 +707,12 @@ 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),
>>> +    if (scpi_info->ops && scpi_info->ops->dvfs_get_info)
>>> +        ret = scpi_info->ops->dvfs_get_info(domain, &buf);
>>> +    else
>>> +        ret = scpi_send_message(SCPI_CMD_GET_DVFS_INFO,
>>> +                    &domain, sizeof(domain),
>>>                  &buf, sizeof(buf));
>>> -
>>>      if (ret)
>>>          return ERR_PTR(ret);
>>>
>>> @@ -855,6 +870,22 @@ static struct scpi_ops scpi_ops = {
>>>      .vendor_send_message = scpi_ext_send_message,
>>>  };
>>>
>>> +static struct scpi_ops legacy_scpi_ops = {
>>> +    .get_version = scpi_get_version,
>>> +    .clk_get_range = NULL,
>>> +    .clk_get_val = legacy_scpi_clk_get_val,
>>> +    .clk_set_val = legacy_scpi_clk_set_val,
>>> +    .dvfs_get_idx = legacy_scpi_dvfs_get_idx,
>>> +    .dvfs_set_idx = legacy_scpi_dvfs_set_idx,
>>> +    .dvfs_get_info = scpi_dvfs_get_info,
>>> +    .sensor_get_capability = legacy_scpi_sensor_get_capability,
>>> +    .sensor_get_info = legacy_scpi_sensor_get_info,
>>> +    .sensor_get_value = legacy_scpi_sensor_get_value,
>>> +    .device_get_power_state = NULL,
>>> +    .device_set_power_state = NULL,
>>> +    .vendor_send_message = legacy_scpi_send_message,
>>
>> I think we need not have this at all if you follow the suggestion I had
>> in the previous patch. Try and let's see how it would look.
>
> If you confirm you want the if/else as said in patch 4.
>
> But clk_get_range, device_get/set_power_state are not available in legacy,
> I think we should still have this alternate structure.
>

I was thinking of overriding the pointers accordingly at the probe time
as the common list is bigger than the one that differs.

-- 
Regards,
Sudeep

[PATCH 07/13] scpi: ignore init_versions failure if reported not supported

[Neil Armstrong]

In Amlogic GXBB Legacy SCPI, the LEGACY_SCPI_CMD_SCPI_CAPABILITIES report
as SCPI_ERR_SUPPORT, so do not fail if this command is not supported.

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

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index 3fe39fe..d3be4c5 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -1111,12 +1111,13 @@ err:
 		ret = scpi_info->ops->init_versions(scpi_info);
 	else
 	ret = scpi_init_versions(scpi_info);
-	if (ret) {
+	if (ret && ret != -EOPNOTSUPP) {
 		dev_err(dev, "incorrect or no SCP firmware found\n");
 		scpi_remove(pdev);
 		return ret;
 	}
 
+	if (ret != -EOPNOTSUPP) {
 	_dev_info(dev, "SCP Protocol %d.%d Firmware %d.%d.%d version\n",
 		  PROTOCOL_REV_MAJOR(scpi_info->protocol_version),
 		  PROTOCOL_REV_MINOR(scpi_info->protocol_version),
@@ -1124,15 +1125,16 @@ err:
 		  FW_REV_MINOR(scpi_info->firmware_version),
 		  FW_REV_PATCH(scpi_info->firmware_version));
 
+		ret = sysfs_create_groups(&dev->kobj, versions_groups);
+		if (ret)
+			dev_err(dev, "unable to create sysfs version group\n");
+	}
+
 	if (scpi_info->ops && scpi_info->ops->scpi_ops)
 		scpi_info->scpi_ops = scpi_info->ops->scpi_ops;
 	else
 		scpi_info->scpi_ops = &scpi_ops;
 
-	ret = sysfs_create_groups(&dev->kobj, versions_groups);
-	if (ret)
-		dev_err(dev, "unable to create sysfs version group\n");
-
 	return of_platform_populate(dev->of_node, NULL, NULL, dev);
 }
 
-- 
1.9.1

Re: [PATCH 07/13] scpi: ignore init_versions failure if reported not supported

[Sudeep Holla]

On 18/08/16 11:11, Neil Armstrong wrote:
> In Amlogic GXBB Legacy SCPI, the LEGACY_SCPI_CMD_SCPI_CAPABILITIES report
> as SCPI_ERR_SUPPORT, so do not fail if this command is not supported.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/firmware/arm_scpi.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
> index 3fe39fe..d3be4c5 100644
> --- a/drivers/firmware/arm_scpi.c
> +++ b/drivers/firmware/arm_scpi.c
> @@ -1111,12 +1111,13 @@ err:
>  		ret = scpi_info->ops->init_versions(scpi_info);
>  	else
>  	ret = scpi_init_versions(scpi_info);
> -	if (ret) {
> +	if (ret && ret != -EOPNOTSUPP) {
>  		dev_err(dev, "incorrect or no SCP firmware found\n");
>  		scpi_remove(pdev);
>  		return ret;
>  	}
>

Why not deal it in init_versions itself.

> +	if (ret != -EOPNOTSUPP) {
>  	_dev_info(dev, "SCP Protocol %d.%d Firmware %d.%d.%d version\n",
>  		  PROTOCOL_REV_MAJOR(scpi_info->protocol_version),
>  		  PROTOCOL_REV_MINOR(scpi_info->protocol_version),

Why not have default value like 0.0 ? Just add a comment. Since get
version is exported out, IMO having default value makes more sense. What
do you think ?

> @@ -1124,15 +1125,16 @@ err:
>  		  FW_REV_MINOR(scpi_info->firmware_version),
>  		  FW_REV_PATCH(scpi_info->firmware_version));
>
> +		ret = sysfs_create_groups(&dev->kobj, versions_groups);
> +		if (ret)
> +			dev_err(dev, "unable to create sysfs version group\n");
> +	}
> +

Again this can stay as is if we have default.

-- 
Regards,
Sudeep

Re: [PATCH 07/13] scpi: ignore init_versions failure if reported not supported

[Neil Armstrong]

On 08/19/2016 06:46 PM, Sudeep Holla wrote:
> 
> 
> On 18/08/16 11:11, Neil Armstrong wrote:
>> In Amlogic GXBB Legacy SCPI, the LEGACY_SCPI_CMD_SCPI_CAPABILITIES report
>> as SCPI_ERR_SUPPORT, so do not fail if this command is not supported.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/firmware/arm_scpi.c | 12 +++++++-----
>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
>> index 3fe39fe..d3be4c5 100644
>> --- a/drivers/firmware/arm_scpi.c
>> +++ b/drivers/firmware/arm_scpi.c
>> @@ -1111,12 +1111,13 @@ err:
>>          ret = scpi_info->ops->init_versions(scpi_info);
>>      else
>>      ret = scpi_init_versions(scpi_info);
>> -    if (ret) {
>> +    if (ret && ret != -EOPNOTSUPP) {
>>          dev_err(dev, "incorrect or no SCP firmware found\n");
>>          scpi_remove(pdev);
>>          return ret;
>>      }
>>
> 
> Why not deal it in init_versions itself.
> 
>> +    if (ret != -EOPNOTSUPP) {
>>      _dev_info(dev, "SCP Protocol %d.%d Firmware %d.%d.%d version\n",
>>            PROTOCOL_REV_MAJOR(scpi_info->protocol_version),
>>            PROTOCOL_REV_MINOR(scpi_info->protocol_version),
> 
> Why not have default value like 0.0 ? Just add a comment. Since get
> version is exported out, IMO having default value makes more sense. What
> do you think ?
> 
>> @@ -1124,15 +1125,16 @@ err:
>>            FW_REV_MINOR(scpi_info->firmware_version),
>>            FW_REV_PATCH(scpi_info->firmware_version));
>>
>> +        ret = sysfs_create_groups(&dev->kobj, versions_groups);
>> +        if (ret)
>> +            dev_err(dev, "unable to create sysfs version group\n");
>> +    }
>> +
> 
> Again this can stay as is if we have default.
> 

Printing version 0.0 firmware 0.0.0 is a nonsense for me...

Neil

Re: [PATCH 07/13] scpi: ignore init_versions failure if reported not supported

[Sudeep Holla]

On 23/08/16 09:23, Neil Armstrong wrote:
> On 08/19/2016 06:46 PM, Sudeep Holla wrote:
>>
>>
>> On 18/08/16 11:11, Neil Armstrong wrote:
>>> In Amlogic GXBB Legacy SCPI, the LEGACY_SCPI_CMD_SCPI_CAPABILITIES report
>>> as SCPI_ERR_SUPPORT, so do not fail if this command is not supported.
>>>
>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>> ---
>>>  drivers/firmware/arm_scpi.c | 12 +++++++-----
>>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
>>> index 3fe39fe..d3be4c5 100644
>>> --- a/drivers/firmware/arm_scpi.c
>>> +++ b/drivers/firmware/arm_scpi.c
>>> @@ -1111,12 +1111,13 @@ err:
>>>          ret = scpi_info->ops->init_versions(scpi_info);
>>>      else
>>>      ret = scpi_init_versions(scpi_info);
>>> -    if (ret) {
>>> +    if (ret && ret != -EOPNOTSUPP) {
>>>          dev_err(dev, "incorrect or no SCP firmware found\n");
>>>          scpi_remove(pdev);
>>>          return ret;
>>>      }
>>>
>>
>> Why not deal it in init_versions itself.
>>
>>> +    if (ret != -EOPNOTSUPP) {
>>>      _dev_info(dev, "SCP Protocol %d.%d Firmware %d.%d.%d version\n",
>>>            PROTOCOL_REV_MAJOR(scpi_info->protocol_version),
>>>            PROTOCOL_REV_MINOR(scpi_info->protocol_version),
>>
>> Why not have default value like 0.0 ? Just add a comment. Since get
>> version is exported out, IMO having default value makes more sense. What
>> do you think ?
>>
>>> @@ -1124,15 +1125,16 @@ err:
>>>            FW_REV_MINOR(scpi_info->firmware_version),
>>>            FW_REV_PATCH(scpi_info->firmware_version));
>>>
>>> +        ret = sysfs_create_groups(&dev->kobj, versions_groups);
>>> +        if (ret)
>>> +            dev_err(dev, "unable to create sysfs version group\n");
>>> +    }
>>> +
>>
>> Again this can stay as is if we have default.
>>
>
> Printing version 0.0 firmware 0.0.0 is a nonsense for me...
>

OK 0.0 was a wrong example. May be 0.1 ?

Since the driver has already exposed, hypothetically user-space can use
that information, so IMO, we need to expose some static version for pre-v1.0

I am surprised that capability is not supported as this was present even
in that legacy SCPI. Do you know what happens if you send that command ?
Have you done some experiments on that ?

-- 
Regards,
Sudeep

Re: [PATCH 07/13] scpi: ignore init_versions failure if reported not supported

[Neil Armstrong]

On 08/23/2016 04:54 PM, Sudeep Holla wrote:
> 
> 
> On 23/08/16 09:23, Neil Armstrong wrote:
>> On 08/19/2016 06:46 PM, Sudeep Holla wrote:
>>>
>>>
>>> On 18/08/16 11:11, Neil Armstrong wrote:
>>>> In Amlogic GXBB Legacy SCPI, the LEGACY_SCPI_CMD_SCPI_CAPABILITIES report
>>>> as SCPI_ERR_SUPPORT, so do not fail if this command is not supported.
>>>>
>>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>>> ---
>>>>  drivers/firmware/arm_scpi.c | 12 +++++++-----
>>>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
>>>> index 3fe39fe..d3be4c5 100644
>>>> --- a/drivers/firmware/arm_scpi.c
>>>> +++ b/drivers/firmware/arm_scpi.c
>>>> @@ -1111,12 +1111,13 @@ err:
>>>>          ret = scpi_info->ops->init_versions(scpi_info);
>>>>      else
>>>>      ret = scpi_init_versions(scpi_info);
>>>> -    if (ret) {
>>>> +    if (ret && ret != -EOPNOTSUPP) {
>>>>          dev_err(dev, "incorrect or no SCP firmware found\n");
>>>>          scpi_remove(pdev);
>>>>          return ret;
>>>>      }
>>>>
>>>
>>> Why not deal it in init_versions itself.
>>>
>>>> +    if (ret != -EOPNOTSUPP) {
>>>>      _dev_info(dev, "SCP Protocol %d.%d Firmware %d.%d.%d version\n",
>>>>            PROTOCOL_REV_MAJOR(scpi_info->protocol_version),
>>>>            PROTOCOL_REV_MINOR(scpi_info->protocol_version),
>>>
>>> Why not have default value like 0.0 ? Just add a comment. Since get
>>> version is exported out, IMO having default value makes more sense. What
>>> do you think ?
>>>
>>>> @@ -1124,15 +1125,16 @@ err:
>>>>            FW_REV_MINOR(scpi_info->firmware_version),
>>>>            FW_REV_PATCH(scpi_info->firmware_version));
>>>>
>>>> +        ret = sysfs_create_groups(&dev->kobj, versions_groups);
>>>> +        if (ret)
>>>> +            dev_err(dev, "unable to create sysfs version group\n");
>>>> +    }
>>>> +
>>>
>>> Again this can stay as is if we have default.
>>>
>>
>> Printing version 0.0 firmware 0.0.0 is a nonsense for me...
>>
> 
> OK 0.0 was a wrong example. May be 0.1 ?
> 
> Since the driver has already exposed, hypothetically user-space can use
> that information, so IMO, we need to expose some static version for pre-v1.0
> 
> I am surprised that capability is not supported as this was present even
> in that legacy SCPI. Do you know what happens if you send that command ?
> Have you done some experiments on that ?
> 

I've experimented and returns EOPNOTSUPP, Amlogic confirmed to us the command was not implemented.

This a clearly a corner-case.

Neil

Re: [PATCH 07/13] scpi: ignore init_versions failure if reported not supported

[Sudeep Holla]

On 23/08/16 15:55, Neil Armstrong wrote:
> On 08/23/2016 04:54 PM, Sudeep Holla wrote:
>>
>>
>> On 23/08/16 09:23, Neil Armstrong wrote:
>>> On 08/19/2016 06:46 PM, Sudeep Holla wrote:
>>>>
>>>>
>>>> On 18/08/16 11:11, Neil Armstrong wrote:
>>>>> In Amlogic GXBB Legacy SCPI, the LEGACY_SCPI_CMD_SCPI_CAPABILITIES report
>>>>> as SCPI_ERR_SUPPORT, so do not fail if this command is not supported.
>>>>>
>>>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>>>> ---
>>>>>  drivers/firmware/arm_scpi.c | 12 +++++++-----
>>>>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
>>>>> index 3fe39fe..d3be4c5 100644
>>>>> --- a/drivers/firmware/arm_scpi.c
>>>>> +++ b/drivers/firmware/arm_scpi.c
>>>>> @@ -1111,12 +1111,13 @@ err:
>>>>>          ret = scpi_info->ops->init_versions(scpi_info);
>>>>>      else
>>>>>      ret = scpi_init_versions(scpi_info);
>>>>> -    if (ret) {
>>>>> +    if (ret && ret != -EOPNOTSUPP) {
>>>>>          dev_err(dev, "incorrect or no SCP firmware found\n");
>>>>>          scpi_remove(pdev);
>>>>>          return ret;
>>>>>      }
>>>>>
>>>>
>>>> Why not deal it in init_versions itself.
>>>>
>>>>> +    if (ret != -EOPNOTSUPP) {
>>>>>      _dev_info(dev, "SCP Protocol %d.%d Firmware %d.%d.%d version\n",
>>>>>            PROTOCOL_REV_MAJOR(scpi_info->protocol_version),
>>>>>            PROTOCOL_REV_MINOR(scpi_info->protocol_version),
>>>>
>>>> Why not have default value like 0.0 ? Just add a comment. Since get
>>>> version is exported out, IMO having default value makes more sense. What
>>>> do you think ?
>>>>
>>>>> @@ -1124,15 +1125,16 @@ err:
>>>>>            FW_REV_MINOR(scpi_info->firmware_version),
>>>>>            FW_REV_PATCH(scpi_info->firmware_version));
>>>>>
>>>>> +        ret = sysfs_create_groups(&dev->kobj, versions_groups);
>>>>> +        if (ret)
>>>>> +            dev_err(dev, "unable to create sysfs version group\n");
>>>>> +    }
>>>>> +
>>>>
>>>> Again this can stay as is if we have default.
>>>>
>>>
>>> Printing version 0.0 firmware 0.0.0 is a nonsense for me...
>>>
>>
>> OK 0.0 was a wrong example. May be 0.1 ?
>>
>> Since the driver has already exposed, hypothetically user-space can use
>> that information, so IMO, we need to expose some static version for pre-v1.0
>>
>> I am surprised that capability is not supported as this was present even
>> in that legacy SCPI. Do you know what happens if you send that command ?
>> Have you done some experiments on that ?
>>
>
> I've experimented and returns EOPNOTSUPP, Amlogic confirmed to us the command was not implemented.
>
> This a clearly a corner-case.
>

OK, thanks for the confirmation.
Not exporting anything could be kind of breaking ABI as it was not made
optional when introduced :( (you can blame me ;))

-- 
Regards,
Sudeep

[PATCH 08/13] scpi: add a vendor_msg mechanism in case the mailbox message differs

[Neil Armstrong]

In case the mailbox message pointer must contain a specific structure.
add a mechanism to override the pointer sent to the mailbox by a
vendor specific data initialized by a vendor speficic function.

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

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index d3be4c5..275feef 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -189,6 +189,7 @@ struct scpi_xfer {
 	unsigned int rx_len;
 	struct list_head node;
 	struct completion done;
+	void *vendor_msg;
 };
 
 struct scpi_chan {
@@ -203,6 +204,7 @@ struct scpi_chan {
 	struct mutex xfers_lock;
 	u8 token;
 	struct scpi_xfer *t;
+	void *vendor_data;
 };
 
 struct scpi_drvinfo {
@@ -302,6 +304,8 @@ struct dev_pstate_set {
 
 struct priv_scpi_ops {
 	/* Internal Specific Ops */
+	int (*init)(struct device *dev, struct scpi_chan *chan);
+	int (*prepare)(struct scpi_chan *chan);
 	void (*handle_remote_msg)(struct mbox_client *c, void *msg);
 	void (*tx_prepare)(struct mbox_client *c, void *msg);
 	/* Message Specific Ops */
@@ -498,7 +502,18 @@ static int legacy_scpi_send_message(u8 cmd, void *tx_buf, unsigned int tx_len,
 	init_completion(&msg->done);
 	scpi_chan->t = msg;
 
-	ret = mbox_send_message(scpi_chan->chan, &msg->cmd);
+	/* Call the prepare hook to eventually set the vendor_msg */
+	if (scpi_info->ops &&
+	    scpi_info->ops->prepare) {
+		ret = scpi_info->ops->prepare(scpi_chan);
+		if (ret) {
+			mutex_unlock(&scpi_chan->xfers_lock);
+			return ret;
+		}
+	} else
+		msg->vendor_msg = &msg->cmd;
+
+	ret = mbox_send_message(scpi_chan->chan, msg->vendor_msg);
 	if (ret < 0)
 		goto out;
 
@@ -1069,6 +1084,12 @@ static int scpi_probe(struct platform_device *pdev)
 		}
 		pchan->tx_payload = pchan->rx_payload + (size >> 1);
 
+		if (scpi_info->ops && scpi_info->ops->init) {
+			ret = scpi_info->ops->init(dev, pchan);
+			if (ret)
+				goto err;
+		}
+
 		cl->dev = dev;
 		if (scpi_info->ops && scpi_info->ops->handle_remote_msg)
 			cl->rx_callback = scpi_info->ops->handle_remote_msg;
-- 
1.9.1

Re: [PATCH 08/13] scpi: add a vendor_msg mechanism in case the mailbox message differs

[Sudeep Holla]

On 18/08/16 11:11, Neil Armstrong wrote:
> In case the mailbox message pointer must contain a specific structure.
> add a mechanism to override the pointer sent to the mailbox by a
> vendor specific data initialized by a vendor speficic function.
>

As discussed, I won't look at this and next patch. You can drop from now.

-- 
Regards,
Sudeep

[PATCH 09/13] scpi: implement rockchip support via the vendor_msg mechanism

[Neil Armstrong]

In order to support the Rockchip legacy procotol, implement the vendor
mailbox specific functions and add ops to the of_match table.

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

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index 275feef..d73db7c 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -455,6 +455,37 @@ static int legacy_scpi_get_chan(u8 cmd)
 	return 0;
 }
 
+/* Rockchip SoCs needs a special structure as a message */
+
+struct rockchip_scpi_xfer {
+	u32 cmd;
+	int rx_size;
+};
+
+static int rockchip_init(struct device *dev, struct scpi_chan *chan)
+{
+	chan->vendor_data = devm_kmalloc(dev,
+					 sizeof(struct rockchip_scpi_xfer),
+					 GFP_KERNEL);
+	if (!chan->vendor_data)
+		return -ENOMEM;
+
+	return 0;
+}
+
+static int rockchip_prepare(struct scpi_chan *chan)
+{
+	struct scpi_xfer *msg = chan->t;
+	struct rockchip_scpi_xfer *xfer = chan->vendor_data;
+
+	xfer->cmd = msg->cmd;
+	xfer->rx_size = msg->rx_len;
+
+	msg->vendor_msg = xfer;
+
+	return 0;
+}
+
 static struct scpi_xfer *get_scpi_xfer(struct scpi_chan *ch)
 {
 	struct scpi_xfer *t;
@@ -1026,9 +1057,21 @@ static const struct priv_scpi_ops scpi_legacy_ops = {
 	.scpi_ops = &legacy_scpi_ops,
 };
 
+static const struct priv_scpi_ops scpi_rockchip_ops = {
+	.init = rockchip_init,
+	.prepare = rockchip_prepare,
+	.handle_remote_msg = legacy_scpi_handle_remote_msg,
+	.tx_prepare = legacy_scpi_tx_prepare,
+	.init_versions = legacy_scpi_init_versions,
+	.dvfs_get_info = legacy_scpi_dvfs_get_info,
+	.scpi_ops = &legacy_scpi_ops,
+};
+
 static const struct of_device_id scpi_of_match[] = {
 	{.compatible = "arm,scpi"},
 	{.compatible = "amlogic,meson-gxbb-scpi", .data = &scpi_legacy_ops},
+	{.compatible = "rockchip,rk3368-scpi", .data = &scpi_rockchip_ops},
+	{.compatible = "rockchip,rk3399-scpi", .data = &scpi_rockchip_ops},
 	{},
 };
 
-- 
1.9.1

[PATCH 10/13] 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 d73db7c..699bed7 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -74,7 +74,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 11/13] dt-bindings: Add support for Amlogic GXBB SCPI Interface

[Neil Armstrong]

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

Re: [PATCH 11/13] dt-bindings: Add support for Amlogic GXBB SCPI Interface

[Rob Herring]

On Thu, Aug 18, 2016 at 12:11:04PM +0200, Neil Armstrong wrote:
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  Documentation/devicetree/bindings/arm/arm,scpi.txt | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

Acked-by: Rob Herring <robh@kernel.org>

[PATCH 12/13] 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 13/13] 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