[RFC PATCH 0/2] scpi: Add SCPI framework to handle vendors variants

[RFC PATCH 0/2] scpi: Add SCPI framework to handle vendors variants

[Neil Armstrong]

Since the current SCPI implementation, based on [0]:
- is (at leat) JUNO specific
- does not specify a strong "standard"
- does not specify a strong MHU interface specification

SoC vendors could implement a variant with slight changes in message
indexes, new messages types, different messages data format or different MHU
communication scheme.

To keep the spirit of the SCPI interface, add a thin "register" layer to get
the ops from the parent node and switch the drivers using the ops to use
the new of_scpi_ops_get() call.

[0] http://infocenter.arm.com/help/topic/com.arm.doc.dui0922b/index.html

Neil Armstrong (2):
  firmware: Add a SCPI framework to handle multiple vendors
    implementation
  firmware: scpi: Switch scpi drivers to use new Framework calls

 drivers/clk/clk-scpi.c         |  18 ++++---
 drivers/cpufreq/scpi-cpufreq.c |   7 +--
 drivers/firmware/Makefile      |   1 +
 drivers/firmware/arm_scpi.c    |  18 +++----
 drivers/firmware/scpi.c        | 110 +++++++++++++++++++++++++++++++++++++++++
 drivers/hwmon/scpi-hwmon.c     |   6 +--
 include/linux/scpi_protocol.h  |  33 ++++++++++++-
 7 files changed, 169 insertions(+), 24 deletions(-)
 create mode 100644 drivers/firmware/scpi.c

-- 
2.7.0

[RFC PATCH 1/2] firmware: Add a SCPI framework to handle multiple vendors implementation

[Neil Armstrong]

Add a thin "register" interface for SCPI driver in order to register their
ops along their node.

Since nodes using the SCPI ops are currently sub-nodes, does not implement
phandle xlate stuff.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/firmware/Makefile |   1 +
 drivers/firmware/scpi.c   | 110 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 111 insertions(+)
 create mode 100644 drivers/firmware/scpi.c

diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 474bada..701a791 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -2,6 +2,7 @@
 # Makefile for the linux kernel.
 #
 obj-$(CONFIG_ARM_PSCI_FW)	+= psci.o
+obj-$(CONFIG_ARM_SCPI_PROTOCOL)	+= scpi.o
 obj-$(CONFIG_ARM_SCPI_PROTOCOL)	+= arm_scpi.o
 obj-$(CONFIG_DMI)		+= dmi_scan.o
 obj-$(CONFIG_DMI_SYSFS)		+= dmi-sysfs.o
diff --git a/drivers/firmware/scpi.c b/drivers/firmware/scpi.c
new file mode 100644
index 0000000..ba94ac4
--- /dev/null
+++ b/drivers/firmware/scpi.c
@@ -0,0 +1,110 @@
+/*
+ * System Control and Power Interface (SCPI) Message Protocol framework
+ *
+ * SCPI Message Protocol is used between the System Control Processor(SCP)
+ * and the Application Processors(AP). The Message Handling Unit(MHU)
+ * provides a mechanism for inter-processor communication between SCP's
+ * Cortex M3 and AP.
+ *
+ * SCP offers control and management of the core/cluster power states,
+ * various power domain DVFS including the core/cluster, certain system
+ * clocks configuration, thermal sensors and many others.
+ *
+ * Copyright (C) 2015 ARM Ltd.
+ * Copyright (C) 2016 BayLibre, SAS.
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/export.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/of.h>
+#include <linux/scpi_protocol.h>
+#include <linux/spinlock.h>
+
+static DEFINE_MUTEX(scpi_list_mutex);
+static LIST_HEAD(scpi_drivers_list);
+
+struct scpi_ops *of_scpi_ops_get(struct device_node *node)
+{
+	struct scpi_ops *ops = NULL;
+	struct scpi_driver *r;
+
+	if (!node)
+		return ERR_PTR(-EINVAL);
+
+	mutex_lock(&scpi_list_mutex);
+	list_for_each_entry(r, &scpi_drivers_list, list) {
+		if (node == r->node) {
+			ops = r->ops;
+			break;
+		}
+	}
+	mutex_unlock(&scpi_list_mutex);
+
+	if (!ops)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	return ops;
+}
+EXPORT_SYMBOL_GPL(of_scpi_ops_get);
+
+int scpi_driver_register(struct scpi_driver *drv)
+{
+	mutex_lock(&scpi_list_mutex);
+	list_add(&drv->list, &scpi_drivers_list);
+	mutex_unlock(&scpi_list_mutex);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(scpi_driver_register);
+
+void scpi_driver_unregister(struct scpi_driver *drv)
+{
+	mutex_lock(&scpi_list_mutex);
+	list_del(&drv->list);
+	mutex_unlock(&scpi_list_mutex);
+}
+EXPORT_SYMBOL_GPL(scpi_driver_unregister);
+
+static void devm_scpi_driver_unregister(struct device *dev, void *res)
+{
+	scpi_driver_unregister(*(struct scpi_driver **)res);
+}
+
+int devm_scpi_driver_register(struct device *dev,
+				struct scpi_driver *drv)
+{
+	struct scpi_driver **rcdrv;
+	int ret;
+
+	rcdrv = devres_alloc(devm_scpi_driver_unregister, sizeof(*drv),
+			     GFP_KERNEL);
+	if (!rcdrv)
+		return -ENOMEM;
+
+	ret = scpi_driver_register(drv);
+	if (!ret) {
+		*rcdrv = drv;
+		devres_add(dev, rcdrv);
+	} else
+		devres_free(rcdrv);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(devm_scpi_driver_register);
-- 
2.7.0

[RFC PATCH 2/2] firmware: scpi: Switch scpi drivers to use new Framework calls

[Neil Armstrong]

Update the include/linux/scpi_protocol.h with the new registry calls.

Switch following drivers to use the new SCPI registry layer :
 - drivers/clk/clk-scpi.c
 - drivers/cpufreq/scpi-cpufreq.c
 - drivers/hwmon/scpi-hwmon.c

And finally switch drivers/firmware/arm_scpi.c to use scpi_driver_register().

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/clk/clk-scpi.c         | 18 +++++++++++-------
 drivers/cpufreq/scpi-cpufreq.c |  7 ++++---
 drivers/firmware/arm_scpi.c    | 18 +++++++++---------
 drivers/hwmon/scpi-hwmon.c     |  6 +++---
 include/linux/scpi_protocol.h  | 33 +++++++++++++++++++++++++++++++--
 5 files changed, 58 insertions(+), 24 deletions(-)

diff --git a/drivers/clk/clk-scpi.c b/drivers/clk/clk-scpi.c
index 6962ee5..18ffaf5 100644
--- a/drivers/clk/clk-scpi.c
+++ b/drivers/clk/clk-scpi.c
@@ -148,7 +148,8 @@ static const struct of_device_id scpi_clk_match[] = {
 
 static struct clk *
 scpi_clk_ops_init(struct device *dev, const struct of_device_id *match,
-		  struct scpi_clk *sclk, const char *name)
+		  struct scpi_clk *sclk, const char *name,
+		  struct scpi_ops *scpi_ops)
 {
 	struct clk_init_data init;
 	struct clk *clk;
@@ -159,7 +160,7 @@ scpi_clk_ops_init(struct device *dev, const struct of_device_id *match,
 	init.num_parents = 0;
 	init.ops = match->data;
 	sclk->hw.init = &init;
-	sclk->scpi_ops = get_scpi_ops();
+	sclk->scpi_ops = scpi_ops;
 
 	if (init.ops == &scpi_dvfs_ops) {
 		sclk->info = sclk->scpi_ops->dvfs_get_info(sclk->id);
@@ -200,7 +201,8 @@ scpi_of_clk_src_get(struct of_phandle_args *clkspec, void *data)
 }
 
 static int scpi_clk_add(struct device *dev, struct device_node *np,
-			const struct of_device_id *match)
+			const struct of_device_id *match,
+			struct scpi_ops *scpi_ops)
 {
 	struct clk **clks;
 	int idx, count;
@@ -249,7 +251,7 @@ static int scpi_clk_add(struct device *dev, struct device_node *np,
 
 		sclk->id = val;
 
-		clks[idx] = scpi_clk_ops_init(dev, match, sclk, name);
+		clks[idx] = scpi_clk_ops_init(dev, match, sclk, name, scpi_ops);
 		if (IS_ERR_OR_NULL(clks[idx]))
 			dev_err(dev, "failed to register clock '%s'\n", name);
 		else
@@ -281,15 +283,17 @@ static int scpi_clocks_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct device_node *child, *np = dev->of_node;
 	const struct of_device_id *match;
+	struct scpi_ops *scpi_ops;
 
-	if (!get_scpi_ops())
-		return -ENXIO;
+	scpi_ops = of_scpi_ops_get(of_get_parent(np));
+	if (IS_ERR(scpi_ops))
+		return PTR_ERR(scpi_ops);
 
 	for_each_available_child_of_node(np, child) {
 		match = of_match_node(scpi_clk_match, child);
 		if (!match)
 			continue;
-		ret = scpi_clk_add(dev, child, match);
+		ret = scpi_clk_add(dev, child, match, scpi_ops);
 		if (ret) {
 			scpi_clocks_remove(pdev);
 			of_node_put(child);
diff --git a/drivers/cpufreq/scpi-cpufreq.c b/drivers/cpufreq/scpi-cpufreq.c
index e8a7bf5..158f1d80 100644
--- a/drivers/cpufreq/scpi-cpufreq.c
+++ b/drivers/cpufreq/scpi-cpufreq.c
@@ -25,6 +25,7 @@
 #include <linux/pm_opp.h>
 #include <linux/scpi_protocol.h>
 #include <linux/types.h>
+#include <linux/of.h>
 
 #include "arm_big_little.h"
 
@@ -88,9 +89,9 @@ static struct cpufreq_arm_bL_ops scpi_cpufreq_ops = {
 
 static int scpi_cpufreq_probe(struct platform_device *pdev)
 {
-	scpi_ops = get_scpi_ops();
-	if (!scpi_ops)
-		return -EIO;
+	scpi_ops = of_scpi_ops_get(of_get_parent(pdev->dev.of_node));
+	if (IS_ERR(scpi_ops))
+		return PTR_ERR(scpi_ops);
 
 	return bL_cpufreq_register(&scpi_cpufreq_ops);
 }
diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index 7e3e595..00fc849 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -162,9 +162,9 @@ struct scpi_drvinfo {
 	u32 firmware_version;
 	int num_chans;
 	atomic_t next_chan;
-	struct scpi_ops *scpi_ops;
 	struct scpi_chan *channels;
 	struct scpi_dvfs_info *dvfs[MAX_DVFS_DOMAINS];
+	struct scpi_driver drv;
 };
 
 /*
@@ -526,7 +526,7 @@ static int scpi_sensor_get_info(u16 sensor_id, struct scpi_sensor_info *info)
 	return ret;
 }
 
-int scpi_sensor_get_value(u16 sensor, u64 *val)
+static int scpi_sensor_get_value(u16 sensor, u64 *val)
 {
 	__le16 id = cpu_to_le16(sensor);
 	struct sensor_value buf;
@@ -554,12 +554,6 @@ static struct scpi_ops scpi_ops = {
 	.sensor_get_value = scpi_sensor_get_value,
 };
 
-struct scpi_ops *get_scpi_ops(void)
-{
-	return scpi_info ? scpi_info->scpi_ops : NULL;
-}
-EXPORT_SYMBOL_GPL(get_scpi_ops);
-
 static int scpi_init_versions(struct scpi_drvinfo *info)
 {
 	int ret;
@@ -743,7 +737,13 @@ 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;
+
+	scpi_info->drv.node = dev->of_node;
+	scpi_info->drv.ops = &scpi_ops;
+
+	ret = devm_scpi_driver_register(dev, &scpi_info->drv);
+	if (ret)
+		return ret;
 
 	ret = sysfs_create_groups(&dev->kobj, versions_groups);
 	if (ret)
diff --git a/drivers/hwmon/scpi-hwmon.c b/drivers/hwmon/scpi-hwmon.c
index 912b449..45d36d1 100644
--- a/drivers/hwmon/scpi-hwmon.c
+++ b/drivers/hwmon/scpi-hwmon.c
@@ -120,9 +120,9 @@ static int scpi_hwmon_probe(struct platform_device *pdev)
 	struct scpi_sensors *scpi_sensors;
 	int ret, idx;
 
-	scpi_ops = get_scpi_ops();
-	if (!scpi_ops)
-		return -EPROBE_DEFER;
+	scpi_ops = of_scpi_ops_get(of_get_parent(dev.of_node));
+	if (IS_ERR(scpi_ops))
+		return PTR_ERR(scpi_ops);
 
 	ret = scpi_ops->sensor_get_capability(&nr_sensors);
 	if (ret)
diff --git a/include/linux/scpi_protocol.h b/include/linux/scpi_protocol.h
index 35de50a..fc27034 100644
--- a/include/linux/scpi_protocol.h
+++ b/include/linux/scpi_protocol.h
@@ -72,8 +72,37 @@ struct scpi_ops {
 	int (*sensor_get_value)(u16, u64 *);
 };
 
+struct scpi_driver {
+	struct device_node *node;
+	struct scpi_ops *ops;
+	struct list_head list;
+};
+
 #if IS_REACHABLE(CONFIG_ARM_SCPI_PROTOCOL)
-struct scpi_ops *get_scpi_ops(void);
+struct scpi_ops *of_scpi_ops_get(struct device_node *node);
+
+int scpi_driver_register(struct scpi_driver *drv);
+
+void scpi_driver_unregister(struct scpi_driver *drv);
+
+int devm_scpi_driver_register(struct device *dev,
+				struct scpi_driver *drv);
 #else
-static inline struct scpi_ops *get_scpi_ops(void) { return NULL; }
+struct scpi_ops *of_scpi_ops_get(struct device_node *node)
+{
+	return ERR_PTR(-ENOTSUPP);
+}
+
+int scpi_driver_register(struct scpi_driver *drv)
+{
+	return -ENOTSUPP;
+}
+
+void scpi_driver_unregister(struct scpi_driver *drv) { }
+
+int devm_scpi_driver_register(struct device *dev,
+				struct scpi_driver *drv)
+{
+	return -ENOTSUPP;
+}
 #endif
-- 
2.7.0

Re: [RFC PATCH 0/2] scpi: Add SCPI framework to handle vendors variants

[Sudeep Holla]

Hi Neil,

On 26/05/16 10:38, Neil Armstrong wrote:
> Since the current SCPI implementation, based on [0]:
> - is (at leat) JUNO specific

Agreed.

> - does not specify a strong "standard"

Not exactly, it's extensible. Refer section 3.2.2. Get SCP capability
Extended Set Enabled bit.

> - does not specify a strong MHU interface specification
>

Not really required, any mailbox must do.

> SoC vendors could implement a variant with slight changes in message
> indexes,

I assume you mean command index here

> new messages types,

Also fine with extended command set.

> different messages data format or

you mean the header or payload ? If they don't follow the header, then
how can be group them as same protocol ?

> different MHU communication scheme.

Not a problem as I mentioned above.

>
> To keep the spirit of the SCPI interface, add a thin "register" layer to get
> the ops from the parent node and switch the drivers using the ops to use
> the new of_scpi_ops_get() call.
>

All I can see is that you share the code to register such drivers which
is hardly anything. It's hard to say all drivers use same protocol or
interface after this change. I may be missing to see something here so
it would be easy to appreciate/review this change with one user of this.

My idea of extending this driver if vendor implements extensions based
on SCPI specification is something like below:

-- 
Regards,
Sudeep

-->8

diff --git i/drivers/firmware/arm_scpi.c w/drivers/firmware/arm_scpi.c
index 7e3e595c9f30..7e46aa103690 100644
--- i/drivers/firmware/arm_scpi.c
+++ w/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))

@@ -132,6 +138,9 @@ enum scpi_std_cmd {
  	SCPI_CMD_COUNT
  };

+enum scpi_vendor_ext_cmd {
+};
+
  struct scpi_xfer {
  	u32 slot; /* has to be first element */
  	u32 cmd;
@@ -165,6 +174,7 @@ struct scpi_drvinfo {
  	struct scpi_ops *scpi_ops;
  	struct scpi_chan *channels;
  	struct scpi_dvfs_info *dvfs[MAX_DVFS_DOMAINS];
+	void *scpi_ext_ops;
  };

  /*
@@ -343,8 +353,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;
@@ -359,7 +369,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;
@@ -384,6 +395,18 @@ static int scpi_send_message(u8 cmd, void *tx_buf, 
unsigned int tx_len,
  	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_send_ext_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;
@@ -574,6 +597,29 @@ static int scpi_init_versions(struct scpi_drvinfo 
*info)
  	return ret;
  }

+static struct scpi_vendor_ext_ops scpi_vendor_ext_ops = {
+};
+
+void *get_scpi_ext_ops(void)
+{
+	return scpi_info ? scpi_info->scpi_ext_ops : NULL;
+}
+EXPORT_SYMBOL_GPL(get_scpi_ext_ops);
+
+static const struct of_device_id scpi_extentions_of_match[] = {
+	{.compatible = "vendor,scpi", .data = &scpi_vendor_ext_ops},
+	{},
+};
+
+static void
+scpi_init_extensions(struct scpi_drvinfo *info, struct device_node *np)
+{
+	const struct of_device_id *of_id;
+
+	if (np && (of_id = of_match_node(scpi_extentions_of_match, np)))
+		info->scpi_ext_ops = (void *)of_id->data;
+}
+
  static ssize_t protocol_version_show(struct device *dev,
  				     struct device_attribute *attr, char *buf)
  {
@@ -745,6 +791,8 @@ static int scpi_probe(struct platform_device *pdev)
  		  FW_REV_PATCH(scpi_info->firmware_version));
  	scpi_info->scpi_ops = &scpi_ops;

+	scpi_init_extensions(scpi_info, np);
+
  	ret = sysfs_create_groups(&dev->kobj, versions_groups);
  	if (ret)
  		dev_err(dev, "unable to create sysfs version group\n");
diff --git i/include/linux/scpi_protocol.h w/include/linux/scpi_protocol.h
index 35de50a65665..052f6aa1ae4b 100644
--- i/include/linux/scpi_protocol.h
+++ w/include/linux/scpi_protocol.h
@@ -72,8 +72,13 @@ struct scpi_ops {
  	int (*sensor_get_value)(u16, u64 *);
  };

+struct scpi_vendor_ext_ops {
+};
+
  #if IS_REACHABLE(CONFIG_ARM_SCPI_PROTOCOL)
  struct scpi_ops *get_scpi_ops(void);
+void *get_scpi_ext_ops(void);
  #else
  static inline struct scpi_ops *get_scpi_ops(void) { return NULL; }
+static inline void *get_scpi_ext_ops(void) { return NULL; }
  #endif

Re: [RFC PATCH 0/2] scpi: Add SCPI framework to handle vendors variants

[Neil Armstrong]

Hi,
On 05/26/2016 06:29 PM, Sudeep Holla wrote:
> Hi Neil,
> 
> On 26/05/16 10:38, Neil Armstrong wrote:
>> Since the current SCPI implementation, based on [0]:
>> - is (at leat) JUNO specific
> 
> Agreed.
> 
>> - does not specify a strong "standard"
> 
> Not exactly, it's extensible. Refer section 3.2.2. Get SCP capability
> Extended Set Enabled bit.

Well, I was not thinking about an extension, but this should be upstreamed for sure.

> 
>> - does not specify a strong MHU interface specification
>>
> 
> Not really required, any mailbox must do.
> 
>> SoC vendors could implement a variant with slight changes in message
>> indexes,
> 
> I assume you mean command index here
> 
>> new messages types,
> 
> Also fine with extended command set.
> 
>> different messages data format or
> 
> you mean the header or payload ? If they don't follow the header, then
> how can be group them as same protocol ?
> 
>> different MHU communication scheme.
> 
> Not a problem as I mentioned above.
> 
>>
>> To keep the spirit of the SCPI interface, add a thin "register" layer to get
>> the ops from the parent node and switch the drivers using the ops to use
>> the new of_scpi_ops_get() call.
>>
> 
> All I can see is that you share the code to register such drivers which
> is hardly anything. It's hard to say all drivers use same protocol or
> interface after this change. I may be missing to see something here so
> it would be easy to appreciate/review this change with one user of this.

I'm actually working on an "SCPI" implementation from Amlogic for their arm64 plaform,
and their implementation is slightly different but enough to need a separate driver.

For instance, the differences are :
- they do not implement virtual channels
- they pass the command word by the mailbox interface
- they use the "sender id" to group the command in a sort of "class"
- the payload size is shifted by 20 instead of 16
- the command word is not in the SRAM, so we cannot use a rx command queue, only a single command by channel at a single time
- the command indexes are slightly shifted
- in clk_set_value, "rate" is first instead of last entry
- in sensor_value, they only use a 32bit entry
- some commands are only accepted on the high priority channel, the others only on the low priority
- MAX_DVFS_DOMAINS is 3 instead of 8
- MAX_DVFS_OPPS is 16 instead of 8

But, It's still a "SCPI" interface by design and usage because :
- they implement 90% of the same commands, in the same way
- the usage is exactly the same and architecture is similar
- they use the same error code scheme

Finally, it would be stupid to add an exact copy of the scpi-sensors and scpi-clocks !
The scpi-cpufreq is another story since they only have a single A53 cluster, it's not adapted.

This is why I wrote a very thin layer, and it can also clean up the arm_scpi and the scpi driver to use a cleaner registry interface.
It would also be a good point to add a private_data to the ops and pass it to the scpi callbacks, to completely remove the global variables.

I found an issue with scpi-cpufreq, since the device is created by the scpi-clocks probe, it does not have a proper node and my current of_scpi_ops_get won't work...
The scpi-cpufreq should have a proper DT node and use the deffered probe to register after the scpi-clocks.

> 
> My idea of extending this driver if vendor implements extensions based
> on SCPI specification is something like below:
> 

Neil

Re: [RFC PATCH 0/2] scpi: Add SCPI framework to handle vendors variants

[Neil Armstrong]

On 05/27/2016 10:17 AM, Neil Armstrong wrote:
> Hi,
> On 05/26/2016 06:29 PM, Sudeep Holla wrote:
>> Hi Neil,
>>> To keep the spirit of the SCPI interface, add a thin "register" layer to get
>>> the ops from the parent node and switch the drivers using the ops to use
>>> the new of_scpi_ops_get() call.
>>>
>>
>> All I can see is that you share the code to register such drivers which
>> is hardly anything. It's hard to say all drivers use same protocol or
>> interface after this change. I may be missing to see something here so
>> it would be easy to appreciate/review this change with one user of this.
> 
> I'm actually working on a "SCPI" implementation from Amlogic for their arm64 platform,
> and their implementation is slightly different but enough to need a separate driver.
> 
> For instance, the differences are :
> - they do not implement virtual channels
> - they pass the command word by the mailbox interface
> - they use the "sender id" to group the command in a sort of "class"
> - the payload size is shifted by 20 instead of 16
> - the command word is not in the SRAM, so we cannot use a rx command queue, only a single command by channel at a single time
> - the command indexes are slightly shifted
> - in clk_set_value, "rate" is first instead of last entry
> - in sensor_value, they only use a 32bit entry
> - some commands are only accepted on the high priority channel, the others only on the low priority
> - MAX_DVFS_DOMAINS is 3 instead of 8
> - MAX_DVFS_OPPS is 16 instead of 8
> 
> But, It's still a "SCPI" interface by design and usage because :
> - they implement 90% of the same commands, in the same way
> - the usage is exactly the same and architecture is similar
> - they use the same error code scheme
> 
> Finally, it would be stupid to add an exact copy of the scpi-sensors and scpi-clocks !
> The scpi-cpufreq is another story since they only have a single A53 cluster, it's not adapted.
> 
> This is why I wrote a very thin layer, and it can also clean up the arm_scpi and the scpi driver to use a cleaner registry interface.
> It would also be a good point to add a private_data to the ops and pass it to the scpi callbacks, to completely remove the global variables.
> 
> I found an issue with scpi-cpufreq, since the device is created by the scpi-clocks probe, it does not have a proper node and my current of_scpi_ops_get won't work...
> The scpi-cpufreq should have a proper DT node and use the deffered probe to register after the scpi-clocks.

Actually I managed to make the Amlogic SCPI SCPI implementation work using a separate driver and the registry layer just fine.
The cpufreq works, like the scpi-dvfs-clocks and the scpi-hwmon drivers.

You can have a look at my github tree :
https://github.com/superna9999/linux/compare/28165ec7a99be98123aa89540bf2cfc24df19498...superna9999:amlogic/v4.7/scpi+fw-v0

The Amlogic SCPI implementation lies on top of this RFC patchset.
The current patchset need a fix in scpi-hwmon and for scpi-cpufreq but it works like a charm.

Neil

Re: [RFC PATCH 0/2] scpi: Add SCPI framework to handle vendors variants

[Neil Armstrong]

On 05/27/2016 10:17 AM, Neil Armstrong wrote:
> Hi,
> On 05/26/2016 06:29 PM, Sudeep Holla wrote:
>> Hi Neil,
>>
>> On 26/05/16 10:38, Neil Armstrong wrote:
>>> Since the current SCPI implementation, based on [0]:
>>> - is (at leat) JUNO specific
>>
>> Agreed.
>>
>>> - does not specify a strong "standard"
>>
>> Not exactly, it's extensible. Refer section 3.2.2. Get SCP capability
>> Extended Set Enabled bit.
> 
> Well, I was not thinking about an extension, but this should be upstreamed for sure.
> 
>>
>>> - does not specify a strong MHU interface specification
>>>
>>
>> Not really required, any mailbox must do.
>>
>>> SoC vendors could implement a variant with slight changes in message
>>> indexes,
>>
>> I assume you mean command index here
>>
>>> new messages types,
>>
>> Also fine with extended command set.
>>
>>> different messages data format or
>>
>> you mean the header or payload ? If they don't follow the header, then
>> how can be group them as same protocol ?
>>
>>> different MHU communication scheme.
>>
>> Not a problem as I mentioned above.
>>
>>>
>>> To keep the spirit of the SCPI interface, add a thin "register" layer to get
>>> the ops from the parent node and switch the drivers using the ops to use
>>> the new of_scpi_ops_get() call.
>>>
>>
>> All I can see is that you share the code to register such drivers which
>> is hardly anything. It's hard to say all drivers use same protocol or
>> interface after this change. I may be missing to see something here so
>> it would be easy to appreciate/review this change with one user of this.
> 
> I'm actually working on an "SCPI" implementation from Amlogic for their arm64 plaform,
> and their implementation is slightly different but enough to need a separate driver.
> 
> For instance, the differences are :
> - they do not implement virtual channels
> - they pass the command word by the mailbox interface
> - they use the "sender id" to group the command in a sort of "class"
> - the payload size is shifted by 20 instead of 16
> - the command word is not in the SRAM, so we cannot use a rx command queue, only a single command by channel at a single time
> - the command indexes are slightly shifted
> - in clk_set_value, "rate" is first instead of last entry
> - in sensor_value, they only use a 32bit entry
> - some commands are only accepted on the high priority channel, the others only on the low priority
> - MAX_DVFS_DOMAINS is 3 instead of 8
> - MAX_DVFS_OPPS is 16 instead of 8
> 
> But, It's still a "SCPI" interface by design and usage because :
> - they implement 90% of the same commands, in the same way
> - the usage is exactly the same and architecture is similar
> - they use the same error code scheme
> 
> Finally, it would be stupid to add an exact copy of the scpi-sensors and scpi-clocks !
> The scpi-cpufreq is another story since they only have a single A53 cluster, it's not adapted.
> 
> This is why I wrote a very thin layer, and it can also clean up the arm_scpi and the scpi driver to use a cleaner registry interface.
> It would also be a good point to add a private_data to the ops and pass it to the scpi callbacks, to completely remove the global variables.
> 
> I found an issue with scpi-cpufreq, since the device is created by the scpi-clocks probe, it does not have a proper node and my current of_scpi_ops_get won't work...
> The scpi-cpufreq should have a proper DT node and use the deffered probe to register after the scpi-clocks.
> 
>>
>> My idea of extending this driver if vendor implements extensions based
>> on SCPI specification is something like below:
>>

While looking for other ARMv8 based platform, I found that the RK3368 platform has the same SCPI implementation as Amlogic.

They extended it with DDR, system and thermal commands.

Look at :
https://github.com/geekboxzone/mmallow_kernel/blob/geekbox/drivers/mailbox/scpi_cmd.h
https://github.com/geekboxzone/mmallow_kernel/blob/geekbox/drivers/mailbox/scpi_protocol.c

So the SCPI must have a framework to allow different protocol versions, and must allow command extension.
Grouping Rockchip and Amlogic should be done, thus needing a generic name like vendor_scpi or with a version.

Sudeep, could you somehow find out which version of the protocol AmLogic and Rockchip based their SCPI development ?


Thanks !
Neil

> Neil
> 

Re: [RFC PATCH 0/2] scpi: Add SCPI framework to handle vendors variants

[Sudeep Holla]

On 30/05/16 09:30, Neil Armstrong wrote:
> On 05/27/2016 10:17 AM, Neil Armstrong wrote:

[..]

>
> While looking for other ARMv8 based platform, I found that the RK3368
> platform has the same SCPI implementation as Amlogic.
>
> They extended it with DDR, system and thermal commands.
>
> Look at :
> https://github.com/geekboxzone/mmallow_kernel/blob/geekbox/drivers/mailbox/scpi_cmd.h
>
>https://github.com/geekboxzone/mmallow_kernel/blob/geekbox/drivers/mailbox/scpi_protocol.c
>


> So the SCPI must have a framework to allow different protocol
> versions, and must allow command extension. Grouping Rockchip and
> Amlogic should be done, thus needing a generic name like vendor_scpi
> or with a version.
>

Makes sense. I understand the need to reuse and I need a bit of time to
have a look at the code(both Amlogic one's you have pointed out and the
Rockchip one) in detail to see what's the best way to proceed. I will
have a look at this later this week and get back to you.

> Sudeep, could you somehow find out which version of the protocol
> AmLogic and Rockchip based their SCPI development ?
>

Yes I tried checking with Rockchip but didn't get a response. But my
guess is that it was some preliminary unpublished version of SCPI
unfortunately :(

-- 
Regards,
Sudeep

Re: [RFC PATCH 0/2] scpi: Add SCPI framework to handle vendors variants

[Kevin Hilman]

[ + Heiko, who may know about the Rockchip implementation ]

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

> On 30/05/16 09:30, Neil Armstrong wrote:
>> On 05/27/2016 10:17 AM, Neil Armstrong wrote:
>
> [..]
>
>>
>> While looking for other ARMv8 based platform, I found that the RK3368
>> platform has the same SCPI implementation as Amlogic.
>>
>> They extended it with DDR, system and thermal commands.
>>
>> Look at :
>> https://github.com/geekboxzone/mmallow_kernel/blob/geekbox/drivers/mailbox/scpi_cmd.h
>>
>>https://github.com/geekboxzone/mmallow_kernel/blob/geekbox/drivers/mailbox/scpi_protocol.c
>>
>
>
>> So the SCPI must have a framework to allow different protocol
>> versions, and must allow command extension. Grouping Rockchip and
>> Amlogic should be done, thus needing a generic name like vendor_scpi
>> or with a version.
>>
>
> Makes sense. I understand the need to reuse and I need a bit of time to
> have a look at the code(both Amlogic one's you have pointed out and the
> Rockchip one) in detail to see what's the best way to proceed. I will
> have a look at this later this week and get back to you.
>
>> Sudeep, could you somehow find out which version of the protocol
>> AmLogic and Rockchip based their SCPI development ?
>>
>
> Yes I tried checking with Rockchip but didn't get a response. But my
> guess is that it was some preliminary unpublished version of SCPI
> unfortunately :(

And if one partner did that, probably everyone else did as well, but
this being the ARM universe, they all did it slightly differently. :(

We know from experience, that this happens all the time in the absence
of a clear standard, so this framework will need to be extended to be
useful.

Thanks,

Kevin

Re: [RFC PATCH 0/2] scpi: Add SCPI framework to handle vendors variants

[Sudeep Holla]

On 01/06/16 17:30, Kevin Hilman wrote:
> [ + Heiko, who may know about the Rockchip implementation ]
>
> Sudeep Holla <sudeep.holla@arm.com> writes:
>
>> On 30/05/16 09:30, Neil Armstrong wrote:
>>> On 05/27/2016 10:17 AM, Neil Armstrong wrote:
>>
>> [..]
>>
>>>
>>> While looking for other ARMv8 based platform, I found that the RK3368
>>> platform has the same SCPI implementation as Amlogic.
>>>
>>> They extended it with DDR, system and thermal commands.
>>>
>>> Look at :
>>> https://github.com/geekboxzone/mmallow_kernel/blob/geekbox/drivers/mailbox/scpi_cmd.h
>>>
>>> https://github.com/geekboxzone/mmallow_kernel/blob/geekbox/drivers/mailbox/scpi_protocol.c
>>>
>>
>>
>>> So the SCPI must have a framework to allow different protocol
>>> versions, and must allow command extension. Grouping Rockchip and
>>> Amlogic should be done, thus needing a generic name like vendor_scpi
>>> or with a version.
>>>
>>
>> Makes sense. I understand the need to reuse and I need a bit of time to
>> have a look at the code(both Amlogic one's you have pointed out and the
>> Rockchip one) in detail to see what's the best way to proceed. I will
>> have a look at this later this week and get back to you.
>>
>>> Sudeep, could you somehow find out which version of the protocol
>>> AmLogic and Rockchip based their SCPI development ?
>>>
>>
>> Yes I tried checking with Rockchip but didn't get a response. But my
>> guess is that it was some preliminary unpublished version of SCPI
>> unfortunately :(
>
> And if one partner did that, probably everyone else did as well, but
> this being the ARM universe, they all did it slightly differently. :(
>

No doubt :)

> We know from experience, that this happens all the time in the absence
> of a clear standard, so this framework will need to be extended to be
> useful.
>

Completely agreed, better to gather all the information possible before
we proceed. I will try to check if I can get hold of old version
internally in the meantime.

-- 
Regards,
Sudeep

Re: [RFC PATCH 0/2] scpi: Add SCPI framework to handle vendors variants

[Heiko Stübner]

Hi,

Am Mittwoch, 1. Juni 2016, 09:30:16 schrieb Kevin Hilman:
> [ + Heiko, who may know about the Rockchip implementation ]
>
> Sudeep Holla <sudeep.holla@arm.com> writes:
> > On 30/05/16 09:30, Neil Armstrong wrote:
> >> On 05/27/2016 10:17 AM, Neil Armstrong wrote:
> > [..]
> > 
> >> While looking for other ARMv8 based platform, I found that the RK3368
> >> platform has the same SCPI implementation as Amlogic.
> >> 
> >> They extended it with DDR, system and thermal commands.
> >> 
> >> Look at :
> >> https://github.com/geekboxzone/mmallow_kernel/blob/geekbox/drivers/mailbo
> >> x/scpi_cmd.h>>
> >>https://github.com/geekboxzone/mmallow_kernel/blob/geekbox/drivers/mailbox
> >>/scpi_protocol.c>>
> >> So the SCPI must have a framework to allow different protocol
> >> versions, and must allow command extension. Grouping Rockchip and
> >> Amlogic should be done, thus needing a generic name like vendor_scpi
> >> or with a version.
> > 
> > Makes sense. I understand the need to reuse and I need a bit of time to
> > have a look at the code(both Amlogic one's you have pointed out and the
> > Rockchip one) in detail to see what's the best way to proceed. I will
> > have a look at this later this week and get back to you.
> > 
> >> Sudeep, could you somehow find out which version of the protocol
> >> AmLogic and Rockchip based their SCPI development ?
> > 
> > Yes I tried checking with Rockchip but didn't get a response. But my
> > guess is that it was some preliminary unpublished version of SCPI
> > unfortunately :(

I only glanced a bit on the scpi stuff of the rk3368, but it seems you already 
found the rockchip implementation above.

The mailbox driver entered mainline recently, but I think it differs a bit 
from the one used there.

I've also added Caesar, who did the upstreaming of the mailbox driver, maybe 
he knows more about the scpi side as well.


Heiko

Re: [RFC PATCH 0/2] scpi: Add SCPI framework to handle vendors variants

[Sudeep Holla]

On 30/05/16 09:30, Neil Armstrong wrote:
> On 05/27/2016 10:17 AM, Neil Armstrong wrote:

>
> While looking for other ARMv8 based platform, I found that the RK3368
> platform has the same SCPI implementation as Amlogic.
>
> They extended it with DDR, system and thermal commands.
>
> Look at :
> https://github.com/geekboxzone/mmallow_kernel/blob/geekbox/drivers/mailbox/scpi_cmd.h
>
>https://github.com/geekboxzone/mmallow_kernel/blob/geekbox/driver/mailbox/scpi_protocol.c
>
> So the SCPI must have a framework to allow different protocol
> versions, and must allow command extension. Grouping Rockchip and
> Amlogic should be done, thus needing a generic name like vendor_scpi
> or with a version.
>
> Sudeep, could you somehow find out which version of the protocol
> AmLogic and Rockchip based their SCPI development ?
>
>

As Caesar Wang replied, they had a previous version of SCPI and I
suggested on how to extend the command set previously in private.
Not sure whats the progress on that

Anyways I had a look at geekbox driver and it looks mostly based on the
initial driver I wrote for initial SCPI versions. I am worried that your
extension might help people to develop their own protocol instead of
following the existing standards. SCPI is published now, so I vendors
use that rather than making up their own. Also ARM is trying to
standardize something in this area like PSCI, but that may take little
more time as it's under discussion with vendors.

Though this initial version of SCPI is not published, I am sure it is
almost same as v1.0 except that the CMD is not part of payload like
v1.0. In v1.0 it's part of payload and the mailbox is used just for
doorbell mechanism.

IMO, we can add some compatible to indicate the pre v1.0 protocol
and add the support to the existing driver itself. Let me know your
thoughts.

-- 
Regards,
Sudeep

Re: [RFC PATCH 0/2] scpi: Add SCPI framework to handle vendors variants

[Neil Armstrong]

On 06/06/2016 07:10 PM, Sudeep Holla wrote:
> 
> 
> On 30/05/16 09:30, Neil Armstrong wrote:
>> On 05/27/2016 10:17 AM, Neil Armstrong wrote:
> 
>>
>> While looking for other ARMv8 based platform, I found that the RK3368
>> platform has the same SCPI implementation as Amlogic.
>>
>> They extended it with DDR, system and thermal commands.
>>
>> Look at :
>> https://github.com/geekboxzone/mmallow_kernel/blob/geekbox/drivers/mailbox/scpi_cmd.h
>>
>> https://github.com/geekboxzone/mmallow_kernel/blob/geekbox/driver/mailbox/scpi_protocol.c
>>
>> So the SCPI must have a framework to allow different protocol
>> versions, and must allow command extension. Grouping Rockchip and
>> Amlogic should be done, thus needing a generic name like vendor_scpi
>> or with a version.
>>
>> Sudeep, could you somehow find out which version of the protocol
>> AmLogic and Rockchip based their SCPI development ?
>>
>>
> 
> As Caesar Wang replied, they had a previous version of SCPI and I
> suggested on how to extend the command set previously in private.
> Not sure whats the progress on that
This extension mechanism is OK for me.

> 
> Anyways I had a look at geekbox driver and it looks mostly based on the
> initial driver I wrote for initial SCPI versions. I am worried that your
> extension might help people to develop their own protocol instead of
> following the existing standards. SCPI is published now, so I vendors
> use that rather than making up their own. Also ARM is trying to
> standardize something in this area like PSCI, but that may take little
> more time as it's under discussion with vendors.
Sure, I understand ARM's point of view.
But keep in mind that some vendors already diverted by using one of your initial
out-of-tree code. This must be handled upstream whatever ARM business strategy is.

My proposal was a first step, it can be simplified by removing the list stuff
since we can consider there will be only one SCP interface.

> 
> Though this initial version of SCPI is not published, I am sure it is
> almost same as v1.0 except that the CMD is not part of payload like
> v1.0. In v1.0 it's part of payload and the mailbox is used just for
> doorbell mechanism.
I hoped it would be this simple, but it touches core defines and structure
that will over complicate the current driver. (i.e. the CMD indexes that differs,
the CMD size shift, the high and low priority redirection or struct ordering)

> IMO, we can add some compatible to indicate the pre v1.0 protocol
> and add the support to the existing driver itself. Let me know your
> thoughts.
> 

My proposal is :
- add a registry layer but with only a single scpi instance (no mode OF involved, remove drivers changes)
- add a scpi_legacy.c driver that only supports the old SCPI like Amlogic and Rockchip, and add a disclaimer for new developed SoCs
- add your extension capability to handle Amlogic's and Rockchip's extended commands

If you agree, I'll post a new patchset for review with these changes.

Thanks,
Neil

Re: [RFC PATCH 0/2] scpi: Add SCPI framework to handle vendors variants

[Sudeep Holla]

On 20/06/16 11:25, Neil Armstrong wrote:
> On 06/06/2016 07:10 PM, Sudeep Holla wrote:

[...]

>>
>> Though this initial version of SCPI is not published, I am sure it is
>> almost same as v1.0 except that the CMD is not part of payload like
>> v1.0. In v1.0 it's part of payload and the mailbox is used just for
>> doorbell mechanism.
> I hoped it would be this simple, but it touches core defines and structure
> that will over complicate the current driver. (i.e. the CMD indexes that differs,
> the CMD size shift, the high and low priority redirection or struct ordering)
>

Ah ok, understood.

>> IMO, we can add some compatible to indicate the pre v1.0 protocol
>> and add the support to the existing driver itself. Let me know your
>> thoughts.
>>
>
> My proposal is :
> - add a registry layer but with only a single scpi instance (no mode OF involved, remove drivers changes)
> - add a scpi_legacy.c driver that only supports the old SCPI like Amlogic and Rockchip, and add a disclaimer for new developed SoCs
> - add your extension capability to handle Amlogic's and Rockchip's extended commands
>
> If you agree, I'll post a new patchset for review with these changes.
>

Yes that sounds better. Also post along with the users to make it easy 
to review even if they are not completely ready for upstream.

-- 
Regards,
Sudeep