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

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

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 <redacted>
---
 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

On 18/08/16 11:10, Neil Armstrong wrote:

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.

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

On 08/19/2016 06:13 PM, Sudeep Holla wrote:

On 18/08/16 11:10, Neil Armstrong wrote:

quoted

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 <redacted>
---
 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.

quoted

+}
+
 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.

quoted

+    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.

quoted

+    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

On 23/08/16 09:15, Neil Armstrong wrote:

On 08/19/2016 06:13 PM, Sudeep Holla wrote:

quoted

On 18/08/16 11:10, Neil Armstrong wrote:

quoted

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 <redacted>
---
 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

[..]

quoted

quoted

+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.

quoted

quoted

+}
+
 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.

quoted

quoted

+    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.

quoted

quoted

+    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

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 <redacted>
---
 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

On 18/08/16 11:10, Neil Armstrong wrote:

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

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.

Ditto, can go away.

-- 
Regards,
Sudeep

On 08/19/2016 06:39 PM, Sudeep Holla wrote:

On 18/08/16 11:10, Neil Armstrong wrote:

quoted

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 <redacted>
---
 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 ?

quoted

 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.

quoted

+};
+
 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

On 23/08/16 09:22, Neil Armstrong wrote:

On 08/19/2016 06:39 PM, Sudeep Holla wrote:

quoted

On 18/08/16 11:10, Neil Armstrong wrote:

quoted

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 <redacted>
---
 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 ?

quoted

 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

Signed-off-by: Neil Armstrong <redacted>
---
 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 at 1 {

@@ -68,6 +69,7 @@
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x1>;
 			enable-method = "psci";
+			clocks = <&scpi_dvfs 0>;
 		};
 
 		cpu2: cpu at 2 {

@@ -75,6 +77,7 @@
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x2>;
 			enable-method = "psci";
+			clocks = <&scpi_dvfs 0>;
 		};
 
 		cpu3: cpu at 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 at 0 {
+				compatible = "arm,scpi-dvfs-clocks";
+				#clock-cells = <1>;
+				clock-indices = <0>;
+				clock-output-names = "vcpu";
+			};
+		};
+
+		scpi_sensors: sensors {
+			compatible = "arm,scpi-sensors";
+			#thermal-sensor-cells = <1>;
+		};
+	};
+
 	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 at 0 {
+				compatible = "amlogic,meson-gxbb-scp-shmem";
+				reg = <0x13000 0x400>;
+			};
+
+			cpu_scp_hpri: scp-shmem at 200 {
+				compatible = "amlogic,meson-gxbb-scp-shmem";
+				reg = <0x13400 0x400>;
+			};
 		};
 
 		cbus: cbus at c1100000 {

-- 
1.9.1

Signed-off-by: Neil Armstrong <redacted>
---
 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 at c8000000 {
+			compatible = "amlogic,meson-gxbb-sram", "mmio-sram";
+			reg = <0x0 0xc8000000 0x0 0x14000>;
+
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0 0x0 0xc8000000 0x14000>;
+		};
+
 		cbus: cbus at c1100000 {
 			compatible = "simple-bus";
 			reg = <0x0 0xc1100000 0x0 0x100000>;

-- 
1.9.1

Signed-off-by: Neil Armstrong <redacted>
---
 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

On Thu, Aug 18, 2016 at 12:11:04PM +0200, Neil Armstrong wrote:

Signed-off-by: Neil Armstrong <redacted>
---
 Documentation/devicetree/bindings/arm/arm,scpi.txt | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

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

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

Signed-off-by: Neil Armstrong <redacted>
---
 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

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 <redacted>
---
 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

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 <redacted>
---
 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

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

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 <redacted>
---
 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

On 18/08/16 11:11, Neil Armstrong wrote:

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 ?

Again this can stay as is if we have default.

-- 
Regards,
Sudeep

On 08/19/2016 06:46 PM, Sudeep Holla wrote:

On 18/08/16 11:11, Neil Armstrong wrote:

quoted

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 <redacted>
---
 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.

quoted

+    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 ?

quoted

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

On 23/08/16 09:23, Neil Armstrong wrote:

On 08/19/2016 06:46 PM, Sudeep Holla wrote:

quoted

On 18/08/16 11:11, Neil Armstrong wrote:

quoted

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 <redacted>
---
 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.

quoted

+    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 ?

quoted

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

On 08/23/2016 04:54 PM, Sudeep Holla wrote:

On 23/08/16 09:23, Neil Armstrong wrote:

quoted

On 08/19/2016 06:46 PM, Sudeep Holla wrote:

quoted

On 18/08/16 11:11, Neil Armstrong wrote:

quoted

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 <redacted>
---
 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.

quoted

+    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 ?

quoted

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

On 23/08/16 15:55, Neil Armstrong wrote:

On 08/23/2016 04:54 PM, Sudeep Holla wrote:

quoted

On 23/08/16 09:23, Neil Armstrong wrote:

quoted

On 08/19/2016 06:46 PM, Sudeep Holla wrote:

quoted

On 18/08/16 11:11, Neil Armstrong wrote:

quoted

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 <redacted>
---
 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.

quoted

+    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 ?

quoted

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

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 <redacted>
---
 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

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

On 08/19/2016 06:24 PM, Sudeep Holla wrote:

On 18/08/16 11:10, Neil Armstrong wrote:

quoted

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

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 <redacted>
---
 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);