[RFC PATCH v3 0/8] scpi: Add SCPI registry to handle legacy protocol

[RFC PATCH v3 0/8] scpi: Add SCPI registry to handle legacy protocol

[Neil Armstrong]

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

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

The Amlogic GXBB platform is using this legacy protocol, as the RK3368 & RK3399
platforms. Only the GXBB example is provided here, but it's unclear if other
Amlogic ARMv8 based SoCs uses this legacy procotol.

In order to support the legacy protocol :
 - Move the scpi_get_ops to a thin registry layer
 - Change the arm_scpi.c to use the registry layer
 - Add a separate config option to build the registry layer
 - Add the legacy SCPI driver based on the new implementation
 - For example, add the Amlogic GXBB MHU and SCPI DT cpufreq & sensors nodes

Changes since RFC v2 at https://lkml.org/lkml/2016/6/21/324 :
 - Moved MHU to a separate patchset posted at http://lkml.kernel.org/r/1470732737-18391-1-git-send-email-narmstrong@baylibre.com
 - Added a vendor send_message callback into scpi_ops to implement vendor commands
 - Implement EXT commands on arm scpi as vendor send_command
 - Added Rockchip variant mailbox handling

Initial RFC discution tread can be found at https://lkml.org/lkml/2016/5/26/111

@Sudeep: I know you wished I merged the legacy into the arm_scpi, but can you take a look at the vendor extension and how I implemented the rockchip mailbox handling ?

Neil Armstrong (8):
  firmware: Add a SCPI registry to handle multiple implementations
  firmware: scpi: Switch arm_scpi to use new registry
  scpi: Add vendor_send_message to enable access to vendor commands
  firmware: arm_scpi: Enable vendor_send_message as ext commands
  firmware: Add legacy SCPI protocol driver
  dt-bindings: arm: Update arm,scpi bindings with Meson GXBB SCPI
  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/Kconfig                           |  24 +
 drivers/firmware/Makefile                          |   2 +
 drivers/firmware/arm_scpi.c                        |  40 +-
 drivers/firmware/legacy_scpi.c                     | 710 +++++++++++++++++++++
 drivers/firmware/scpi.c                            |  94 +++
 include/linux/scpi_protocol.h                      |  20 +-
 8 files changed, 928 insertions(+), 15 deletions(-)
 create mode 100644 drivers/firmware/legacy_scpi.c
 create mode 100644 drivers/firmware/scpi.c

-- 
2.7.0

[RFC PATCH v3 1/8] firmware: Add a SCPI registry to handle multiple implementations

[Neil Armstrong]

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/firmware/Kconfig      |  4 ++
 drivers/firmware/Makefile     |  1 +
 drivers/firmware/scpi.c       | 94 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/scpi_protocol.h | 15 ++++++-
 4 files changed, 113 insertions(+), 1 deletion(-)
 create mode 100644 drivers/firmware/scpi.c

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 0e22f24..ff85511 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -8,9 +8,13 @@ menu "Firmware Drivers"
 config ARM_PSCI_FW
 	bool
 
+config SCPI_FW
+	bool
+
 config ARM_SCPI_PROTOCOL
 	tristate "ARM System Control and Power Interface (SCPI) Message Protocol"
 	depends on MAILBOX
+	select SCPI_FW
 	help
 	  System Control and Power Interface (SCPI) Message Protocol is
 	  defined for the purpose of communication between the Application
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 44a59dc..968197f 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_SCPI_FW)		+= scpi.o
 obj-$(CONFIG_ARM_SCPI_PROTOCOL)	+= arm_scpi.o
 obj-$(CONFIG_ARM_SCPI_POWER_DOMAIN) += scpi_pm_domain.o
 obj-$(CONFIG_DMI)		+= dmi_scan.o
diff --git a/drivers/firmware/scpi.c b/drivers/firmware/scpi.c
new file mode 100644
index 0000000..87a559a
--- /dev/null
+++ b/drivers/firmware/scpi.c
@@ -0,0 +1,94 @@
+/*
+ * System Control and Power Interface (SCPI) Message Protocol registry
+ *
+ * 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 struct scpi_ops *g_ops;
+
+struct scpi_ops *get_scpi_ops(void)
+{
+	return g_ops;
+}
+EXPORT_SYMBOL_GPL(get_scpi_ops);
+
+int scpi_ops_register(struct scpi_ops *ops)
+{
+	if (!ops)
+		return -EINVAL;
+
+	if (g_ops)
+		return -EEXIST;
+
+	g_ops = ops;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(scpi_ops_register);
+
+void scpi_ops_unregister(struct scpi_ops *ops)
+{
+	if (g_ops == ops)
+		g_ops = NULL;
+}
+EXPORT_SYMBOL_GPL(scpi_ops_unregister);
+
+static void devm_scpi_ops_unregister(struct device *dev, void *res)
+{
+	scpi_ops_unregister(*(struct scpi_ops **)res);
+}
+
+int devm_scpi_ops_register(struct device *dev,
+				struct scpi_ops *ops)
+{
+	struct scpi_ops **rcops;
+	int ret;
+
+	rcops = devres_alloc(devm_scpi_ops_unregister, sizeof(*ops),
+			     GFP_KERNEL);
+	if (!rcops)
+		return -ENOMEM;
+
+	ret = scpi_ops_register(ops);
+	if (!ret) {
+		*rcops = ops;
+		devres_add(dev, rcops);
+	} else
+		devres_free(rcops);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(devm_scpi_ops_register);
diff --git a/include/linux/scpi_protocol.h b/include/linux/scpi_protocol.h
index dc5f989..8e21e3a 100644
--- a/include/linux/scpi_protocol.h
+++ b/include/linux/scpi_protocol.h
@@ -74,8 +74,21 @@ struct scpi_ops {
 	int (*device_set_power_state)(u16, u8);
 };
 
-#if IS_REACHABLE(CONFIG_ARM_SCPI_PROTOCOL)
+#if IS_REACHABLE(CONFIG_SCPI_FW)
 struct scpi_ops *get_scpi_ops(void);
+int scpi_ops_register(struct scpi_ops *drv);
+void scpi_ops_unregister(struct scpi_ops *drv);
+int devm_scpi_ops_register(struct device *dev, struct scpi_ops *drv);
 #else
 static inline struct scpi_ops *get_scpi_ops(void) { return NULL; }
+
+static inline int scpi_ops_register(struct scpi_ops *drv) { return -ENOTSUPP; }
+
+static inline void scpi_ops_unregister(struct scpi_ops *drv) { }
+
+static inline int devm_scpi_ops_register(struct device *dev,
+					 struct scpi_ops *drv)
+{
+	return -ENOTSUPP;
+}
 #endif
-- 
2.7.0

[RFC PATCH v3 2/8] firmware: scpi: Switch arm_scpi to use new registry

[Neil Armstrong]

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

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index 4388937..c6d6528 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -162,7 +162,6 @@ 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];
 };
@@ -580,12 +579,6 @@ static struct scpi_ops scpi_ops = {
 	.device_set_power_state = scpi_device_set_power_state,
 };
 
-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;
@@ -769,7 +762,10 @@ 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;
+
+	ret = devm_scpi_ops_register(dev, &scpi_ops);
+	if (ret)
+		return ret;
 
 	ret = sysfs_create_groups(&dev->kobj, versions_groups);
 	if (ret)
-- 
2.7.0

[RFC PATCH v3 3/8] scpi: Add vendor_send_message to enable access to vendor commands

[Neil Armstrong]

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

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 include/linux/scpi_protocol.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/scpi_protocol.h b/include/linux/scpi_protocol.h
index 8e21e3a..dd5d81d 100644
--- a/include/linux/scpi_protocol.h
+++ b/include/linux/scpi_protocol.h
@@ -58,6 +58,8 @@ struct scpi_sensor_info {
  *	OPP is an index to the list return by @dvfs_get_info
  * @dvfs_get_info: returns the DVFS capabilities of the given power
  *	domain. It includes the OPP list and the latency information
+ * @vendor_send_message: vendor specific message sending, arg can specify
+ *	a scpi implementation specific argument
  */
 struct scpi_ops {
 	u32 (*get_version)(void);
@@ -72,6 +74,9 @@ struct scpi_ops {
 	int (*sensor_get_value)(u16, u64 *);
 	int (*device_get_power_state)(u16);
 	int (*device_set_power_state)(u16, u8);
+	int (*vendor_send_message)(u8 cmd, unsigned long arg,
+				   void *tx_buf, unsigned int tx_len,
+				   void *rx_buf, unsigned int rx_len);
 };
 
 #if IS_REACHABLE(CONFIG_SCPI_FW)
-- 
2.7.0

[RFC PATCH v3 4/8] firmware: arm_scpi: Enable vendor_send_message as ext commands

[Neil Armstrong]

Implement official SCPI external commands with the newly introduced
vendor_send_message in ops.

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

diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
index c6d6528..7290182 100644
--- a/drivers/firmware/arm_scpi.c
+++ b/drivers/firmware/arm_scpi.c
@@ -46,6 +46,8 @@
 
 #define CMD_ID_SHIFT		0
 #define CMD_ID_MASK		0x7f
+#define CMD_SET_SHIFT		7
+#define CMD_SET_MASK		0x1
 #define CMD_TOKEN_ID_SHIFT	8
 #define CMD_TOKEN_ID_MASK	0xff
 #define CMD_DATA_SIZE_SHIFT	16
@@ -53,6 +55,10 @@
 #define PACK_SCPI_CMD(cmd_id, tx_sz)			\
 	((((cmd_id) & CMD_ID_MASK) << CMD_ID_SHIFT) |	\
 	(((tx_sz) & CMD_DATA_SIZE_MASK) << CMD_DATA_SIZE_SHIFT))
+#define PACK_EXT_SCPI_CMD(cmd_id, tx_sz)		\
+	((((cmd_id) & CMD_ID_MASK) << CMD_ID_SHIFT) |	\
+	(CMD_SET_MASK << CMD_SET_SHIFT) |		\
+	(((tx_sz) & CMD_DATA_SIZE_MASK) << CMD_DATA_SIZE_SHIFT))
 #define ADD_SCPI_TOKEN(cmd, token)			\
 	((cmd) |= (((token) & CMD_TOKEN_ID_MASK) << CMD_TOKEN_ID_SHIFT))
 
@@ -343,8 +349,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 +365,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 +391,20 @@ out:
 	return ret > 0 ? scpi_to_linux_errno(ret) : ret;
 }
 
+static int scpi_send_message(u8 cmd, void *tx_buf, unsigned int tx_len,
+			     void *rx_buf, unsigned int rx_len)
+{
+	return __scpi_send_message(cmd, tx_buf, tx_len, rx_buf, rx_len, false);
+}
+
+static int scpi_ext_send_message(u8 cmd, unsigned long arg,
+				 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;
@@ -577,6 +598,7 @@ static struct scpi_ops scpi_ops = {
 	.sensor_get_value = scpi_sensor_get_value,
 	.device_get_power_state = scpi_device_get_power_state,
 	.device_set_power_state = scpi_device_set_power_state,
+	.vendor_send_message = scpi_ext_send_message,
 };
 
 static int scpi_init_versions(struct scpi_drvinfo *info)
-- 
2.7.0

[RFC PATCH v3 5/8] firmware: Add legacy SCPI protocol driver

[Neil Armstrong]

Add legacy protocol driver for an early published SCPI implementation
by supporting old command indexes and structure.
This driver also supports vendor messages and rockchip specific
mailbox data structure for message passing to SCP.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/firmware/Kconfig       |  20 ++
 drivers/firmware/Makefile      |   1 +
 drivers/firmware/legacy_scpi.c | 710 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 731 insertions(+)
 create mode 100644 drivers/firmware/legacy_scpi.c

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index ff85511..f6226b7 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -40,6 +40,26 @@ config ARM_SCPI_POWER_DOMAIN
 	  This enables support for the SCPI power domains which can be
 	  enabled or disabled via the SCP firmware
 
+config LEGACY_SCPI_PROTOCOL
+	bool "Legacy System Control and Power Interface (SCPI) Message Protocol"
+	default y if ARCH_MESON
+	select SCPI_FW
+	help
+	  System Control and Power Interface (SCPI) Message Protocol is
+	  defined for the purpose of communication between the Application
+	  Cores(AP) and the System Control Processor(SCP). The MHU peripheral
+	  provides a mechanism for inter-processor communication between SCP
+	  and AP.
+
+	  SCP controls most of the power managament on the Application
+	  Processors. It 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.
+
+	  This protocol library provides interface for all the client drivers
+	  making use of the features offered by the legacy SCP protocol.
+
 config EDD
 	tristate "BIOS Enhanced Disk Drive calls determine boot disk"
 	depends on X86
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 968197f..46ed784 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_ARM_PSCI_FW)	+= psci.o
 obj-$(CONFIG_SCPI_FW)		+= scpi.o
 obj-$(CONFIG_ARM_SCPI_PROTOCOL)	+= arm_scpi.o
 obj-$(CONFIG_ARM_SCPI_POWER_DOMAIN) += scpi_pm_domain.o
+obj-$(CONFIG_LEGACY_SCPI_PROTOCOL) += legacy_scpi.o
 obj-$(CONFIG_DMI)		+= dmi_scan.o
 obj-$(CONFIG_DMI_SYSFS)		+= dmi-sysfs.o
 obj-$(CONFIG_EDD)		+= edd.o
diff --git a/drivers/firmware/legacy_scpi.c b/drivers/firmware/legacy_scpi.c
new file mode 100644
index 0000000..e7d7462
--- /dev/null
+++ b/drivers/firmware/legacy_scpi.c
@@ -0,0 +1,710 @@
+/*
+ * Legacy System Control and Power Interface (SCPI) Message Protocol driver
+ *
+ * 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) 2016 BayLibre, SAS.
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ *
+ * Heavily based on arm_scpi.c from :
+ * Copyright (C) 2015 ARM Ltd.
+ *
+ * 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/>.
+ */
+
+/*
+ * DISCLAIMER
+ *
+ * This SCPI implementation is based on a technology preview release
+ * and new ARMv8 SoCs implementations should use the standard SCPI
+ * implementation as defined in the ARM DUI 0922G and implemented
+ * in the arm_scpi.c driver.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/bitmap.h>
+#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/mailbox_client.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/printk.h>
+#include <linux/scpi_protocol.h>
+#include <linux/slab.h>
+#include <linux/sort.h>
+#include <linux/spinlock.h>
+
+#define CMD_ID_SHIFT		0
+#define CMD_ID_MASK		0x7f
+#define CMD_SENDER_ID_SHIFT	8
+#define CMD_SENDER_ID_MASK	0xff
+#define CMD_DATA_SIZE_SHIFT	20
+#define CMD_DATA_SIZE_MASK	0x1ff
+#define PACK_SCPI_CMD(cmd_id, sender, tx_sz)				\
+	((((cmd_id) & CMD_ID_MASK) << CMD_ID_SHIFT) |			\
+	(((sender) & CMD_SENDER_ID_MASK) << CMD_SENDER_ID_SHIFT) |	\
+	(((tx_sz) & CMD_DATA_SIZE_MASK) << CMD_DATA_SIZE_SHIFT))
+
+#define CMD_SIZE(cmd)	(((cmd) >> CMD_DATA_SIZE_SHIFT) & CMD_DATA_SIZE_MASK)
+#define CMD_UNIQ_MASK	(CMD_TOKEN_ID_MASK << CMD_TOKEN_ID_SHIFT | CMD_ID_MASK)
+#define CMD_XTRACT_UNIQ(cmd)	((cmd) & CMD_UNIQ_MASK)
+
+#define MAX_DVFS_DOMAINS	3
+#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)
+
+#define MAX_RX_TIMEOUT          (msecs_to_jiffies(30))
+
+enum legacy_scpi_error_codes {
+	SCPI_SUCCESS = 0, /* Success */
+	SCPI_ERR_PARAM = 1, /* Invalid parameter(s) */
+	SCPI_ERR_ALIGN = 2, /* Invalid alignment */
+	SCPI_ERR_SIZE = 3, /* Invalid size */
+	SCPI_ERR_HANDLER = 4, /* Invalid handler/callback */
+	SCPI_ERR_ACCESS = 5, /* Invalid access/permission denied */
+	SCPI_ERR_RANGE = 6, /* Value out of range */
+	SCPI_ERR_TIMEOUT = 7, /* Timeout has occurred */
+	SCPI_ERR_NOMEM = 8, /* Invalid memory area or pointer */
+	SCPI_ERR_PWRSTATE = 9, /* Invalid power state */
+	SCPI_ERR_SUPPORT = 10, /* Not supported or disabled */
+	SCPI_ERR_DEVICE = 11, /* Device error */
+	SCPI_ERR_BUSY = 12, /* Device busy */
+	SCPI_ERR_MAX
+};
+
+enum legacy_scpi_client_id {
+	SCPI_CL_NONE,
+	SCPI_CL_CLOCKS,
+	SCPI_CL_DVFS,
+	SCPI_CL_POWER,
+	SCPI_CL_THERMAL,
+	SCPI_MAX,
+};
+
+enum legacy_scpi_std_cmd {
+	SCPI_CMD_INVALID		= 0x00,
+	SCPI_CMD_SCPI_READY		= 0x01,
+	SCPI_CMD_SCPI_CAPABILITIES	= 0x02,
+	SCPI_CMD_EVENT			= 0x03,
+	SCPI_CMD_SET_CSS_PWR_STATE	= 0x04,
+	SCPI_CMD_GET_CSS_PWR_STATE	= 0x05,
+	SCPI_CMD_CFG_PWR_STATE_STAT	= 0x06,
+	SCPI_CMD_GET_PWR_STATE_STAT	= 0x07,
+	SCPI_CMD_SYS_PWR_STATE		= 0x08,
+	SCPI_CMD_L2_READY		= 0x09,
+	SCPI_CMD_SET_AP_TIMER		= 0x0a,
+	SCPI_CMD_CANCEL_AP_TIME		= 0x0b,
+	SCPI_CMD_DVFS_CAPABILITIES	= 0x0c,
+	SCPI_CMD_GET_DVFS_INFO		= 0x0d,
+	SCPI_CMD_SET_DVFS		= 0x0e,
+	SCPI_CMD_GET_DVFS		= 0x0f,
+	SCPI_CMD_GET_DVFS_STAT		= 0x10,
+	SCPI_CMD_SET_RTC		= 0x11,
+	SCPI_CMD_GET_RTC		= 0x12,
+	SCPI_CMD_CLOCK_CAPABILITIES	= 0x13,
+	SCPI_CMD_SET_CLOCK_INDEX	= 0x14,
+	SCPI_CMD_SET_CLOCK_VALUE	= 0x15,
+	SCPI_CMD_GET_CLOCK_VALUE	= 0x16,
+	SCPI_CMD_PSU_CAPABILITIES	= 0x17,
+	SCPI_CMD_SET_PSU		= 0x18,
+	SCPI_CMD_GET_PSU		= 0x19,
+	SCPI_CMD_SENSOR_CAPABILITIES	= 0x1a,
+	SCPI_CMD_SENSOR_INFO		= 0x1b,
+	SCPI_CMD_SENSOR_VALUE		= 0x1c,
+	SCPI_CMD_SENSOR_CFG_PERIODIC	= 0x1d,
+	SCPI_CMD_SENSOR_CFG_BOUNDS	= 0x1e,
+	SCPI_CMD_SENSOR_ASYNC_VALUE	= 0x1f,
+	SCPI_CMD_COUNT
+};
+
+struct legacy_scpi_xfer {
+	u32 cmd;
+	u32 status;
+	const void *tx_buf;
+	void *rx_buf;
+	unsigned int tx_len;
+	unsigned int rx_len;
+	struct completion done;
+	void *vendor_msg;
+};
+
+struct legacy_scpi_chan {
+	struct mbox_client cl;
+	struct mbox_chan *chan;
+	void __iomem *tx_payload;
+	void __iomem *rx_payload;
+	struct mutex xfers_lock;
+	struct legacy_scpi_xfer t;
+	void *vendor_data;
+};
+
+struct legacy_scpi_ops {
+	int (*init)(struct device *dev, struct legacy_scpi_chan *chan);
+	int (*prepare)(struct legacy_scpi_chan *chan, unsigned long arg);
+};
+
+struct legacy_scpi_drvinfo {
+	int num_chans;
+	struct legacy_scpi_chan *channels;
+	struct scpi_dvfs_info *dvfs[MAX_DVFS_DOMAINS];
+	const struct legacy_scpi_ops *ops;
+};
+
+/*
+ * The SCP firmware only executes in little-endian mode, so any buffers
+ * shared through SCPI should have their contents converted to little-endian
+ */
+struct legacy_scpi_shared_mem {
+	__le32 status;
+	u8 payload[0];
+} __packed;
+
+struct scp_capabilities {
+	__le32 protocol_version;
+	__le32 event_version;
+	__le32 platform_version;
+	__le32 commands[4];
+} __packed;
+
+struct clk_get_info {
+	__le16 id;
+	__le16 flags;
+	__le32 min_rate;
+	__le32 max_rate;
+	u8 name[20];
+} __packed;
+
+struct clk_get_value {
+	__le32 rate;
+} __packed;
+
+struct clk_set_value {
+	__le32 rate;
+	__le16 id;
+	__le16 reserved;
+} __packed;
+
+struct dvfs_info {
+	__le32 header;
+	struct {
+		__le32 freq;
+		__le32 m_volt;
+	} opps[MAX_DVFS_OPPS];
+} __packed;
+
+struct dvfs_get {
+	u8 index;
+} __packed;
+
+struct dvfs_set {
+	u8 domain;
+	u8 index;
+} __packed;
+
+struct sensor_capabilities {
+	__le16 sensors;
+} __packed;
+
+struct sensor_info {
+	__le16 sensor_id;
+	u8 class;
+	u8 trigger_type;
+	char name[20];
+};
+
+struct sensor_value {
+	__le32 val;
+} __packed;
+
+static struct legacy_scpi_drvinfo *legacy_scpi_info;
+
+static int legacy_scpi_linux_errmap[SCPI_ERR_MAX] = {
+	/* better than switch case as long as return value is continuous */
+	0, /* SCPI_SUCCESS */
+	-EINVAL, /* SCPI_ERR_PARAM */
+	-ENOEXEC, /* SCPI_ERR_ALIGN */
+	-EMSGSIZE, /* SCPI_ERR_SIZE */
+	-EINVAL, /* SCPI_ERR_HANDLER */
+	-EACCES, /* SCPI_ERR_ACCESS */
+	-ERANGE, /* SCPI_ERR_RANGE */
+	-ETIMEDOUT, /* SCPI_ERR_TIMEOUT */
+	-ENOMEM, /* SCPI_ERR_NOMEM */
+	-EINVAL, /* SCPI_ERR_PWRSTATE */
+	-EOPNOTSUPP, /* SCPI_ERR_SUPPORT */
+	-EIO, /* SCPI_ERR_DEVICE */
+	-EBUSY, /* SCPI_ERR_BUSY */
+};
+
+static inline int legacy_scpi_to_linux_errno(int errno)
+{
+	if (errno >= SCPI_SUCCESS && errno < SCPI_ERR_MAX)
+		return legacy_scpi_linux_errmap[errno];
+	return -EIO;
+}
+
+static void legacy_scpi_handle_remote_msg(struct mbox_client *c, void *msg)
+{
+	struct legacy_scpi_chan *ch =
+		container_of(c, struct legacy_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 legacy_scpi_tx_prepare(struct mbox_client *c, void *msg)
+{
+	struct legacy_scpi_chan *ch =
+		container_of(c, struct legacy_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 high_priority_cmds[] = {
+	SCPI_CMD_GET_CSS_PWR_STATE,
+	SCPI_CMD_CFG_PWR_STATE_STAT,
+	SCPI_CMD_GET_PWR_STATE_STAT,
+	SCPI_CMD_SET_DVFS,
+	SCPI_CMD_GET_DVFS,
+	SCPI_CMD_SET_RTC,
+	SCPI_CMD_GET_RTC,
+	SCPI_CMD_SET_CLOCK_INDEX,
+	SCPI_CMD_SET_CLOCK_VALUE,
+	SCPI_CMD_GET_CLOCK_VALUE,
+	SCPI_CMD_SET_PSU,
+	SCPI_CMD_GET_PSU,
+	SCPI_CMD_SENSOR_CFG_PERIODIC,
+	SCPI_CMD_SENSOR_CFG_BOUNDS,
+};
+
+static int legacy_scpi_get_chan(u8 cmd)
+{
+	int idx;
+
+	for (idx = 0; idx < ARRAY_SIZE(high_priority_cmds); idx++)
+		if (cmd == high_priority_cmds[idx])
+			return 1;
+
+	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 legacy_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 legacy_scpi_chan *chan,
+				    unsigned long arg)
+{
+	struct legacy_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 int legacy_scpi_send_message(u8 cmd, unsigned long arg,
+				void *tx_buf, unsigned int tx_len,
+				void *rx_buf, unsigned int rx_len)
+{
+	int ret;
+	u8 chan;
+	struct legacy_scpi_xfer *msg;
+	struct legacy_scpi_chan *legacy_scpi_chan;
+
+	chan = legacy_scpi_get_chan(cmd);
+	legacy_scpi_chan = legacy_scpi_info->channels + chan;
+
+	msg = &legacy_scpi_chan->t;
+
+	mutex_lock(&legacy_scpi_chan->xfers_lock);
+
+	msg->cmd = PACK_SCPI_CMD(cmd, arg, 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);
+
+	/* Call the prepare hook to eventually set the vendor_msg */
+	if (legacy_scpi_info->ops &&
+	    legacy_scpi_info->ops->prepare) {
+		ret = legacy_scpi_info->ops->prepare(legacy_scpi_chan, arg);
+		if (ret) {
+			mutex_unlock(&legacy_scpi_chan->xfers_lock);
+			return ret;
+		}
+	} else
+		msg->vendor_msg = &msg->cmd;
+
+	ret = mbox_send_message(legacy_scpi_chan->chan, msg->vendor_msg);
+	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(&legacy_scpi_chan->xfers_lock);
+
+	/* SCPI error codes > 0, translate them to Linux scale*/
+	return ret > 0 ? legacy_scpi_to_linux_errno(ret) : ret;
+}
+
+static u32 legacy_scpi_get_version(void)
+{
+	return 1; /* v0.1 */
+}
+
+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(SCPI_CMD_GET_CLOCK_VALUE,
+				SCPI_CL_CLOCKS,
+				&le_clk_id, sizeof(le_clk_id),
+				&clk, sizeof(clk));
+
+	return ret ? ret : le32_to_cpu(clk.rate);
+}
+
+static int legacy_scpi_clk_set_val(u16 clk_id, unsigned long rate)
+{
+	int stat;
+	struct clk_set_value clk = {
+		.id = cpu_to_le16(clk_id),
+		.rate = cpu_to_le32(rate)
+	};
+
+	return legacy_scpi_send_message(SCPI_CMD_SET_CLOCK_VALUE,
+				SCPI_CL_CLOCKS,
+				&clk, sizeof(clk),
+				&stat, sizeof(stat));
+}
+
+static int legacy_scpi_dvfs_get_idx(u8 domain)
+{
+	int ret;
+	struct dvfs_get dvfs;
+
+	ret = legacy_scpi_send_message(SCPI_CMD_GET_DVFS, SCPI_CL_DVFS,
+				&domain, sizeof(domain),
+				&dvfs, sizeof(dvfs));
+
+	return ret ? ret : dvfs.index;
+}
+
+static int legacy_scpi_dvfs_set_idx(u8 domain, u8 index)
+{
+	int stat;
+	struct dvfs_set dvfs = {domain, index};
+
+	return legacy_scpi_send_message(SCPI_CMD_SET_DVFS, SCPI_CL_DVFS,
+				&dvfs, sizeof(dvfs),
+				&stat, sizeof(stat));
+}
+
+static struct scpi_dvfs_info *legacy_scpi_dvfs_get_info(u8 domain)
+{
+	struct scpi_dvfs_info *info;
+	struct scpi_opp *opp;
+	struct dvfs_info buf;
+	int ret, i;
+
+	if (domain >= MAX_DVFS_DOMAINS)
+		return ERR_PTR(-EINVAL);
+
+	if (legacy_scpi_info->dvfs[domain])	/* data already populated */
+		return legacy_scpi_info->dvfs[domain];
+
+	ret = legacy_scpi_send_message(SCPI_CMD_GET_DVFS_INFO, SCPI_CL_DVFS,
+				&domain, sizeof(domain),
+				&buf, sizeof(buf));
+
+	if (ret)
+		return ERR_PTR(ret);
+
+	info = kmalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return ERR_PTR(-ENOMEM);
+
+	info->count = DVFS_OPP_COUNT(buf.header);
+	info->latency = DVFS_LATENCY(buf.header) * 1000; /* uS to nS */
+
+	info->opps = kcalloc(info->count, sizeof(*opp), GFP_KERNEL);
+	if (!info->opps) {
+		kfree(info);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	for (i = 0, opp = info->opps; i < info->count; i++, opp++) {
+		opp->freq = le32_to_cpu(buf.opps[i].freq);
+		opp->m_volt = le32_to_cpu(buf.opps[i].m_volt);
+	}
+
+	legacy_scpi_info->dvfs[domain] = info;
+	return info;
+}
+
+static int legacy_scpi_sensor_get_capability(u16 *sensors)
+{
+	struct sensor_capabilities cap_buf;
+	int ret;
+
+	ret = legacy_scpi_send_message(SCPI_CMD_SENSOR_CAPABILITIES,
+				SCPI_CL_THERMAL, NULL, 0, &cap_buf,
+				sizeof(cap_buf));
+	if (!ret)
+		*sensors = le16_to_cpu(cap_buf.sensors);
+
+	return ret;
+}
+
+static int legacy_scpi_sensor_get_info(u16 sensor_id,
+				     struct scpi_sensor_info *info)
+{
+	__le16 id = cpu_to_le16(sensor_id);
+	struct sensor_info _info;
+	int ret;
+
+	ret = legacy_scpi_send_message(SCPI_CMD_SENSOR_INFO, SCPI_CL_THERMAL,
+				&id, sizeof(id),
+				&_info, sizeof(_info));
+	if (!ret) {
+		memcpy(info, &_info, sizeof(*info));
+		info->sensor_id = le16_to_cpu(_info.sensor_id);
+	}
+
+	return ret;
+}
+
+static int legacy_scpi_sensor_get_value(u16 sensor, u64 *val)
+{
+	__le16 id = cpu_to_le16(sensor);
+	struct sensor_value buf;
+	int ret;
+
+	ret = legacy_scpi_send_message(SCPI_CMD_SENSOR_VALUE, SCPI_CL_THERMAL,
+				&id, sizeof(id),
+				&buf, sizeof(buf));
+	if (!ret)
+		*val = (u64)le32_to_cpu(buf.val);
+
+	return ret;
+}
+
+static struct scpi_ops legacy_scpi_ops = {
+	.get_version = legacy_scpi_get_version,
+	.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 = legacy_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,
+	.vendor_send_message = legacy_scpi_send_message,
+};
+
+static void legacy_scpi_free_channels(struct device *dev,
+				struct legacy_scpi_chan *pchan, int count)
+{
+	int i;
+
+	for (i = 0; i < count && pchan->chan; i++, pchan++)
+		mbox_free_channel(pchan->chan);
+}
+
+static int legacy_scpi_remove(struct platform_device *pdev)
+{
+	int i;
+	struct device *dev = &pdev->dev;
+	struct legacy_scpi_drvinfo *info = platform_get_drvdata(pdev);
+
+	/* stop exporting SCPI ops */
+	legacy_scpi_info = NULL;
+
+	of_platform_depopulate(dev);
+	legacy_scpi_free_channels(dev, info->channels, info->num_chans);
+	platform_set_drvdata(pdev, NULL);
+
+	for (i = 0; i < MAX_DVFS_DOMAINS && info->dvfs[i]; i++) {
+		kfree(info->dvfs[i]->opps);
+		kfree(info->dvfs[i]);
+	}
+
+	return 0;
+}
+
+static const struct legacy_scpi_ops scpi_rockchip_ops = {
+	.init = rockchip_init,
+	.prepare = rockchip_prepare,
+};
+
+static const struct of_device_id legacy_scpi_of_match[] = {
+	{.compatible = "amlogic,meson-gxbb-scpi"},
+	{.compatible = "rockchip,rk3368-scpi", .data = &scpi_rockchip_ops},
+	{.compatible = "rockchip,rk3399-scpi", .data = &scpi_rockchip_ops},
+	{ /* sentinel */ },
+};
+
+static int legacy_scpi_probe(struct platform_device *pdev)
+{
+	int count, idx, ret;
+	struct resource res;
+	struct legacy_scpi_chan *legacy_scpi_chan;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	const struct of_device_id *match;
+	const struct legacy_scpi_ops *ops;
+
+	match = of_match_device(legacy_scpi_of_match, &pdev->dev);
+	if (!match)
+		return -EINVAL;
+
+	ops = match->data;
+
+	legacy_scpi_info = devm_kzalloc(dev, sizeof(*legacy_scpi_info),
+					GFP_KERNEL);
+	if (!legacy_scpi_info)
+		return -ENOMEM;
+
+	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);
+		return -ENODEV;
+	}
+
+	legacy_scpi_chan = devm_kcalloc(dev, count,
+				sizeof(*legacy_scpi_chan), GFP_KERNEL);
+	if (!legacy_scpi_chan)
+		return -ENOMEM;
+
+	for (idx = 0; idx < count; idx++) {
+		resource_size_t size;
+		struct legacy_scpi_chan *pchan = legacy_scpi_chan + idx;
+		struct mbox_client *cl = &pchan->cl;
+		struct device_node *shmem = of_parse_phandle(np, "shmem", idx);
+
+		if (of_address_to_resource(shmem, 0, &res)) {
+			dev_err(dev, "failed to get SCPI payload mem resource\n");
+			ret = -EINVAL;
+			goto err;
+		}
+
+		size = resource_size(&res);
+
+		pchan->rx_payload = devm_ioremap(dev, res.start, size);
+		if (!pchan->rx_payload) {
+			dev_err(dev, "failed to ioremap SCPI payload\n");
+			ret = -EADDRNOTAVAIL;
+			goto err;
+		}
+		pchan->tx_payload = pchan->rx_payload + (size >> 1);
+
+		if (ops && ops->init) {
+			ret = ops->init(dev, pchan);
+			if (ret)
+				goto err;
+		}
+
+		cl->dev = dev;
+		cl->rx_callback = legacy_scpi_handle_remote_msg;
+		cl->tx_prepare = legacy_scpi_tx_prepare;
+		cl->tx_block = true;
+		cl->tx_tout = 20;
+		cl->knows_txdone = false; /* controller can't ack */
+
+		mutex_init(&pchan->xfers_lock);
+
+		pchan->chan = mbox_request_channel(cl, idx);
+		if (!IS_ERR(pchan->chan))
+			continue;
+		else
+			ret = PTR_ERR(pchan->chan);
+err:
+		legacy_scpi_free_channels(dev, legacy_scpi_chan, idx);
+		legacy_scpi_info = NULL;
+		return ret;
+	}
+
+	legacy_scpi_info->ops = ops;
+	legacy_scpi_info->channels = legacy_scpi_chan;
+	legacy_scpi_info->num_chans = count;
+	platform_set_drvdata(pdev, legacy_scpi_info);
+
+	ret = devm_scpi_ops_register(dev, &legacy_scpi_ops);
+	if (ret)
+		return ret;
+
+	return of_platform_populate(dev->of_node, NULL, NULL, dev);
+}
+
+MODULE_DEVICE_TABLE(of, legacy_scpi_of_match);
+
+static struct platform_driver legacy_scpi_driver = {
+	.driver = {
+		.name = "legacy-scpi",
+		.of_match_table = legacy_scpi_of_match,
+	},
+	.probe = legacy_scpi_probe,
+	.remove = legacy_scpi_remove,
+};
+module_platform_driver(legacy_scpi_driver);
+
+MODULE_AUTHOR("Sudeep Holla <sudeep.holla@arm.com>");
+MODULE_AUTHOR("Neil Armstrong <narmstrong@baylibre.com>");
+MODULE_DESCRIPTION("ARM Legacy SCPI mailbox protocol driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.0

[RFC PATCH v3 6/8] dt-bindings: arm: Update arm,scpi bindings with Meson GXBB SCPI

[Neil Armstrong]

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

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

Re: [RFC PATCH v3 0/8] scpi: Add SCPI registry to handle legacy protocol

[Neil Armstrong]

Hi Frank,

On 08/10/2016 12:19 PM, Frank Wang wrote:
> Hi Neil,
> 
> On 2016/8/9 18:29, Neil Armstrong wrote:

[...]

>>
>> Changes since RFC v2 at https://lkml.org/lkml/2016/6/21/324 :
>>   - Moved MHU to a separate patchset posted at
>> http://lkml.kernel.org/r/1470732737-18391-1-git-send-email-narmstrong@baylib
>> re.com
>>   - Added a vendor send_message callback into scpi_ops to implement vendor
>> commands
>>   - Implement EXT commands on arm scpi as vendor send_command
>>   - Added Rockchip variant mailbox handling
> 
> Two questions,
> 
> 1. For legacy_scpi, I saw you have implemented .vendor_send_message api, however, it seems that does not distinguish standard and extended command, so it may be misleading if the vendor send out a non-standard command from their own driver module.

Actually, the vendor_send_message is for non-standard command, and since it's vendor code is should be specific to the platform and won't have any effect on other platforms.
DDR and other rockchip specific command shoudl go through this API.
The legacy_scpi code does not distinguish standard and extended command since nothing was made into the protocol for this.

> 
> 2. For arm_scpi, it have already distinguished standard and extended command, but unfortunately, there was no rockchip_scpi_xfer structure something like in legacy_scpi driver, so if some vendor (just like Rockchip :-)) wanna switch the driver from legacy_scpi to arm_scpi, maybe this is a bit problem, how do you think?

Yes, it could be added the same way it was added to legacy_scpi, Sudeep made the assumption it would work with any mailbox controller, but since the mailbox framework does not make any assumption about the data type between the controller and the client, such adaptation is necessary.

Neil
> 
> BR.
> Frank
> 
>> Initial RFC discution tread can be found at
>> https://lkml.org/lkml/2016/5/26/111
>>
>> @Sudeep: I know you wished I merged the legacy into the arm_scpi, but can
>> you take a look at the vendor extension and how I implemented the rockchip
>> mailbox handling ?
>>
>> Neil Armstrong (8):
>>    firmware: Add a SCPI registry to handle multiple implementations
>>    firmware: scpi: Switch arm_scpi to use new registry
>>    scpi: Add vendor_send_message to enable access to vendor commands
>>    firmware: arm_scpi: Enable vendor_send_message as ext commands
>>    firmware: Add legacy SCPI protocol driver
>>    dt-bindings: arm: Update arm,scpi bindings with Meson GXBB SCPI
>>    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/Kconfig                           |  24 +
>>   drivers/firmware/Makefile                          |   2 +
>>   drivers/firmware/arm_scpi.c                        |  40 +-
>>   drivers/firmware/legacy_scpi.c                     | 710
>> +++++++++++++++++++++
>>   drivers/firmware/scpi.c                            |  94 +++
>>   include/linux/scpi_protocol.h                      |  20 +-
>>   8 files changed, 928 insertions(+), 15 deletions(-)
>>   create mode 100644 drivers/firmware/legacy_scpi.c
>>   create mode 100644 drivers/firmware/scpi.c
>>
> 
> 

Re: [RFC PATCH v3 5/8] firmware: Add legacy SCPI protocol driver

[Sudeep Holla]

Hi Neil,

On 09/08/16 11:29, Neil Armstrong wrote:
> Add legacy protocol driver for an early published SCPI implementation
> by supporting old command indexes and structure.
> This driver also supports vendor messages and rockchip specific
> mailbox data structure for message passing to SCP.
>

Sorry for the delay but I expected some attempts to reduce the
duplication of code we have here. I really don't like the duplication of
the code. As I mentioned earlier it can be reduced. I see lot of scope
for that and I see that you made zero attempts since v2.

> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/firmware/Kconfig       |  20 ++
>  drivers/firmware/Makefile      |   1 +
>  drivers/firmware/legacy_scpi.c | 710 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 731 insertions(+)
>  create mode 100644 drivers/firmware/legacy_scpi.c
>
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index ff85511..f6226b7 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -40,6 +40,26 @@ config ARM_SCPI_POWER_DOMAIN
>  	  This enables support for the SCPI power domains which can be
>  	  enabled or disabled via the SCP firmware
>
> +config LEGACY_SCPI_PROTOCOL
> +	bool "Legacy System Control and Power Interface (SCPI) Message Protocol"
> +	default y if ARCH_MESON
> +	select SCPI_FW
> +	help
> +	  System Control and Power Interface (SCPI) Message Protocol is
> +	  defined for the purpose of communication between the Application
> +	  Cores(AP) and the System Control Processor(SCP). The MHU peripheral
> +	  provides a mechanism for inter-processor communication between SCP
> +	  and AP.
> +
> +	  SCP controls most of the power managament on the Application
> +	  Processors. It 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.
> +
> +	  This protocol library provides interface for all the client drivers
> +	  making use of the features offered by the legacy SCP protocol.
> +

Why do we need this ? If we plan to enable this in defconfig, I would
rather not introduce this as it serves nothing extra over the
compatible. We need runtime check for this with DT compatibles.


[...]


> +enum legacy_scpi_client_id {
> +	SCPI_CL_NONE,
> +	SCPI_CL_CLOCKS,
> +	SCPI_CL_DVFS,
> +	SCPI_CL_POWER,
> +	SCPI_CL_THERMAL,
> +	SCPI_MAX,
> +};
> +

As I said before these values were introduced by me initially and I
reckon firmware doesn't depend on that. Have you really tested dropping
them ? This must go as they are useless and we now have tokens which are
much better.

I will stop here and ask why can't you start with simple change like
below ? Then we can add or re-define the structures/enums when
absolutely needed. Please don't just copy the entire driver and make
changes where-ever needed or please try to adapt to the new driver and
try to deviate as less as required by the firmware.

Regards,
Sudeep

--->8

diff --git i/drivers/firmware/arm_scpi.c w/drivers/firmware/arm_scpi.c
index 438893762076..b394ffb5939c 100644
--- i/drivers/firmware/arm_scpi.c
+++ w/drivers/firmware/arm_scpi.c
@@ -82,6 +82,9 @@

  #define MAX_RX_TIMEOUT		(msecs_to_jiffies(30))

+typedef int (*scpi_send_msg_func)(u8 cmd, void *tx_buf, unsigned int 
tx_len,
+				  void *rx_buf, unsigned int rx_len);
+
  enum scpi_error_codes {
  	SCPI_SUCCESS = 0, /* Success */
  	SCPI_ERR_PARAM = 1, /* Invalid parameter(s) */
@@ -162,6 +165,7 @@ struct scpi_drvinfo {
  	u32 firmware_version;
  	int num_chans;
  	atomic_t next_chan;
+	scpi_send_msg_func send_msg;
  	struct scpi_ops *scpi_ops;
  	struct scpi_chan *channels;
  	struct scpi_dvfs_info *dvfs[MAX_DVFS_DOMAINS];
@@ -397,8 +401,8 @@ scpi_clk_get_range(u16 clk_id, unsigned long *min, 
unsigned long *max)
  	struct clk_get_info clk;
  	__le16 le_clk_id = cpu_to_le16(clk_id);

-	ret = scpi_send_message(SCPI_CMD_GET_CLOCK_INFO, &le_clk_id,
-				sizeof(le_clk_id), &clk, sizeof(clk));
+	ret = scpi_info->send_msg(SCPI_CMD_GET_CLOCK_INFO, &le_clk_id,
+				  sizeof(le_clk_id), &clk, sizeof(clk));
  	if (!ret) {
  		*min = le32_to_cpu(clk.min_rate);
  		*max = le32_to_cpu(clk.max_rate);
@@ -412,8 +416,8 @@ static unsigned long scpi_clk_get_val(u16 clk_id)
  	struct clk_get_value clk;
  	__le16 le_clk_id = cpu_to_le16(clk_id);

-	ret = scpi_send_message(SCPI_CMD_GET_CLOCK_VALUE, &le_clk_id,
-				sizeof(le_clk_id), &clk, sizeof(clk));
+	ret = scpi_info->send_msg(SCPI_CMD_GET_CLOCK_VALUE, &le_clk_id,
+				  sizeof(le_clk_id), &clk, sizeof(clk));
  	return ret ? ret : le32_to_cpu(clk.rate);
  }

@@ -425,8 +429,8 @@ static int scpi_clk_set_val(u16 clk_id, unsigned 
long rate)
  		.rate = cpu_to_le32(rate)
  	};

-	return scpi_send_message(SCPI_CMD_SET_CLOCK_VALUE, &clk, sizeof(clk),
-				 &stat, sizeof(stat));
+	return scpi_info->send_msg(SCPI_CMD_SET_CLOCK_VALUE, &clk, sizeof(clk),
+				   &stat, sizeof(stat));
  }

  static int scpi_dvfs_get_idx(u8 domain)
@@ -434,8 +438,8 @@ static int scpi_dvfs_get_idx(u8 domain)
  	int ret;
  	u8 dvfs_idx;

-	ret = scpi_send_message(SCPI_CMD_GET_DVFS, &domain, sizeof(domain),
-				&dvfs_idx, sizeof(dvfs_idx));
+	ret = scpi_info->send_msg(SCPI_CMD_GET_DVFS, &domain, sizeof(domain),
+				  &dvfs_idx, sizeof(dvfs_idx));
  	return ret ? ret : dvfs_idx;
  }

@@ -444,8 +448,8 @@ static int scpi_dvfs_set_idx(u8 domain, u8 index)
  	int stat;
  	struct dvfs_set dvfs = {domain, index};

-	return scpi_send_message(SCPI_CMD_SET_DVFS, &dvfs, sizeof(dvfs),
-				 &stat, sizeof(stat));
+	return scpi_info->send_msg(SCPI_CMD_SET_DVFS, &dvfs, sizeof(dvfs),
+				   &stat, sizeof(stat));
  }

  static int opp_cmp_func(const void *opp1, const void *opp2)
@@ -468,8 +472,8 @@ static struct scpi_dvfs_info *scpi_dvfs_get_info(u8 
domain)
  	if (scpi_info->dvfs[domain])	/* data already populated */
  		return scpi_info->dvfs[domain];

-	ret = scpi_send_message(SCPI_CMD_GET_DVFS_INFO, &domain, sizeof(domain),
-				&buf, sizeof(buf));
+	ret = scpi_info->send_msg(SCPI_CMD_GET_DVFS_INFO, &domain,
+				  sizeof(domain), &buf, sizeof(buf));

  	if (ret)
  		return ERR_PTR(ret);
@@ -503,8 +507,8 @@ static int scpi_sensor_get_capability(u16 *sensors)
  	struct sensor_capabilities cap_buf;
  	int ret;

-	ret = scpi_send_message(SCPI_CMD_SENSOR_CAPABILITIES, NULL, 0, &cap_buf,
-				sizeof(cap_buf));
+	ret = scpi_info->send_msg(SCPI_CMD_SENSOR_CAPABILITIES, NULL, 0,
+				  &cap_buf, sizeof(cap_buf));
  	if (!ret)
  		*sensors = le16_to_cpu(cap_buf.sensors);

@@ -517,8 +521,8 @@ static int scpi_sensor_get_info(u16 sensor_id, 
struct scpi_sensor_info *info)
  	struct _scpi_sensor_info _info;
  	int ret;

-	ret = scpi_send_message(SCPI_CMD_SENSOR_INFO, &id, sizeof(id),
-				&_info, sizeof(_info));
+	ret = scpi_info->send_msg(SCPI_CMD_SENSOR_INFO, &id, sizeof(id),
+				  &_info, sizeof(_info));
  	if (!ret) {
  		memcpy(info, &_info, sizeof(*info));
  		info->sensor_id = le16_to_cpu(_info.sensor_id);
@@ -533,8 +537,8 @@ static int scpi_sensor_get_value(u16 sensor, u64 *val)
  	struct sensor_value buf;
  	int ret;

-	ret = scpi_send_message(SCPI_CMD_SENSOR_VALUE, &id, sizeof(id),
-				&buf, sizeof(buf));
+	ret = scpi_info->send_msg(SCPI_CMD_SENSOR_VALUE, &id, sizeof(id),
+				  &buf, sizeof(buf));
  	if (!ret)
  		*val = (u64)le32_to_cpu(buf.hi_val) << 32 |
  			le32_to_cpu(buf.lo_val);
@@ -548,8 +552,8 @@ static int scpi_device_get_power_state(u16 dev_id)
  	u8 pstate;
  	__le16 id = cpu_to_le16(dev_id);

-	ret = scpi_send_message(SCPI_CMD_GET_DEVICE_PWR_STATE, &id,
-				sizeof(id), &pstate, sizeof(pstate));
+	ret = scpi_info->send_msg(SCPI_CMD_GET_DEVICE_PWR_STATE, &id,
+				  sizeof(id), &pstate, sizeof(pstate));
  	return ret ? ret : pstate;
  }

@@ -561,8 +565,8 @@ static int scpi_device_set_power_state(u16 dev_id, 
u8 pstate)
  		.pstate = pstate,
  	};

-	return scpi_send_message(SCPI_CMD_SET_DEVICE_PWR_STATE, &dev_set,
-				 sizeof(dev_set), &stat, sizeof(stat));
+	return scpi_info->send_msg(SCPI_CMD_SET_DEVICE_PWR_STATE, &dev_set,
+				   sizeof(dev_set), &stat, sizeof(stat));
  }

  static struct scpi_ops scpi_ops = {
@@ -591,8 +595,8 @@ static int scpi_init_versions(struct scpi_drvinfo *info)
  	int ret;
  	struct scp_capabilities caps;

-	ret = scpi_send_message(SCPI_CMD_SCPI_CAPABILITIES, NULL, 0,
-				&caps, sizeof(caps));
+	ret = scpi_info->send_msg(SCPI_CMD_SCPI_CAPABILITIES, NULL, 0,
+				  &caps, sizeof(caps));
  	if (!ret) {
  		info->protocol_version = le32_to_cpu(caps.protocol_version);
  		info->firmware_version = le32_to_cpu(caps.platform_version);
@@ -681,6 +685,14 @@ 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", .data = scpi_send_message},
+	{.compatible = "arm,scpi-legacy", .data = scpi_send_message},
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, scpi_of_match);
+
  static int scpi_probe(struct platform_device *pdev)
  {
  	int count, idx, ret;
@@ -688,11 +700,15 @@ 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;

  	scpi_info = devm_kzalloc(dev, sizeof(*scpi_info), GFP_KERNEL);
  	if (!scpi_info)
  		return -ENOMEM;

+	match = of_match_device(scpi_of_match, dev);
+	scpi_info->send_msg = 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);
@@ -778,13 +794,6 @@ static int scpi_probe(struct platform_device *pdev)
  	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",

Re: [RFC PATCH v3 5/8] firmware: Add legacy SCPI protocol driver

[Neil Armstrong]

On 08/15/2016 03:35 PM, Sudeep Holla wrote:
> Hi Neil,
> 
> On 09/08/16 11:29, Neil Armstrong wrote:
>> Add legacy protocol driver for an early published SCPI implementation
>> by supporting old command indexes and structure.
>> This driver also supports vendor messages and rockchip specific
>> mailbox data structure for message passing to SCP.
>>
> 

Hi Sudeep,

> Sorry for the delay but I expected some attempts to reduce the
> duplication of code we have here. I really don't like the duplication of
> the code. As I mentioned earlier it can be reduced. I see lot of scope
> for that and I see that you made zero attempts since v2.

Yes, this post is an intermediate RFC to show to rockchip guys how I could implement support
for their vendor commands, the next RFC will certainly merge the two drivers with minimal code
duplication.

> 
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/firmware/Kconfig       |  20 ++
>>  drivers/firmware/Makefile      |   1 +
>>  drivers/firmware/legacy_scpi.c | 710 +++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 731 insertions(+)
>>  create mode 100644 drivers/firmware/legacy_scpi.c
>>
>> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
>> index ff85511..f6226b7 100644
>> --- a/drivers/firmware/Kconfig
>> +++ b/drivers/firmware/Kconfig
>> @@ -40,6 +40,26 @@ config ARM_SCPI_POWER_DOMAIN
>>        This enables support for the SCPI power domains which can be
>>        enabled or disabled via the SCP firmware
>>
>> +config LEGACY_SCPI_PROTOCOL
>> +    bool "Legacy System Control and Power Interface (SCPI) Message Protocol"
>> +    default y if ARCH_MESON
>> +    select SCPI_FW
>> +    help
>> +      System Control and Power Interface (SCPI) Message Protocol is
>> +      defined for the purpose of communication between the Application
>> +      Cores(AP) and the System Control Processor(SCP). The MHU peripheral
>> +      provides a mechanism for inter-processor communication between SCP
>> +      and AP.
>> +
>> +      SCP controls most of the power managament on the Application
>> +      Processors. It 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.
>> +
>> +      This protocol library provides interface for all the client drivers
>> +      making use of the features offered by the legacy SCP protocol.
>> +
> 
> Why do we need this ? If we plan to enable this in defconfig, I would
> rather not introduce this as it serves nothing extra over the
> compatible. We need runtime check for this with DT compatibles.
> 
> 
> [...]
> 
> 
>> +enum legacy_scpi_client_id {
>> +    SCPI_CL_NONE,
>> +    SCPI_CL_CLOCKS,
>> +    SCPI_CL_DVFS,
>> +    SCPI_CL_POWER,
>> +    SCPI_CL_THERMAL,
>> +    SCPI_MAX,
>> +};
>> +
> 
> As I said before these values were introduced by me initially and I
> reckon firmware doesn't depend on that. Have you really tested dropping
> them ? This must go as they are useless and we now have tokens which are
> much better.

I am not sure, I'm currently waiting for Amlogic's SCPI FW specification to be sure
these are not needed at all, in the meantime I will still implement them.

> 
> I will stop here and ask why can't you start with simple change like
> below ? Then we can add or re-define the structures/enums when
> absolutely needed. Please don't just copy the entire driver and make
> changes where-ever needed or please try to adapt to the new driver and
> try to deviate as less as required by the firmware.

It will be done in RFC v4.


> Regards,
> Sudeep

Thanks,
Neil

> 
> --->8
> 
> diff --git i/drivers/firmware/arm_scpi.c w/drivers/firmware/arm_scpi.c
> index 438893762076..b394ffb5939c 100644
> --- i/drivers/firmware/arm_scpi.c
> +++ w/drivers/firmware/arm_scpi.c
> @@ -82,6 +82,9 @@
> 
>  #define MAX_RX_TIMEOUT        (msecs_to_jiffies(30))
> 
> +typedef int (*scpi_send_msg_func)(u8 cmd, void *tx_buf, unsigned int tx_len,
> +                  void *rx_buf, unsigned int rx_len);
> +
>  enum scpi_error_codes {
>      SCPI_SUCCESS = 0, /* Success */
>      SCPI_ERR_PARAM = 1, /* Invalid parameter(s) */
> @@ -162,6 +165,7 @@ struct scpi_drvinfo {
>      u32 firmware_version;
>      int num_chans;
>      atomic_t next_chan;
> +    scpi_send_msg_func send_msg;
>      struct scpi_ops *scpi_ops;
>      struct scpi_chan *channels;
>      struct scpi_dvfs_info *dvfs[MAX_DVFS_DOMAINS];
> @@ -397,8 +401,8 @@ scpi_clk_get_range(u16 clk_id, unsigned long *min, unsigned long *max)
>      struct clk_get_info clk;
>      __le16 le_clk_id = cpu_to_le16(clk_id);
> 
> -    ret = scpi_send_message(SCPI_CMD_GET_CLOCK_INFO, &le_clk_id,
> -                sizeof(le_clk_id), &clk, sizeof(clk));
> +    ret = scpi_info->send_msg(SCPI_CMD_GET_CLOCK_INFO, &le_clk_id,
> +                  sizeof(le_clk_id), &clk, sizeof(clk));
>      if (!ret) {
>          *min = le32_to_cpu(clk.min_rate);
>          *max = le32_to_cpu(clk.max_rate);
> @@ -412,8 +416,8 @@ static unsigned long scpi_clk_get_val(u16 clk_id)
>      struct clk_get_value clk;
>      __le16 le_clk_id = cpu_to_le16(clk_id);
> 
> -    ret = scpi_send_message(SCPI_CMD_GET_CLOCK_VALUE, &le_clk_id,
> -                sizeof(le_clk_id), &clk, sizeof(clk));
> +    ret = scpi_info->send_msg(SCPI_CMD_GET_CLOCK_VALUE, &le_clk_id,
> +                  sizeof(le_clk_id), &clk, sizeof(clk));
>      return ret ? ret : le32_to_cpu(clk.rate);
>  }
> 
> @@ -425,8 +429,8 @@ static int scpi_clk_set_val(u16 clk_id, unsigned long rate)
>          .rate = cpu_to_le32(rate)
>      };
> 
> -    return scpi_send_message(SCPI_CMD_SET_CLOCK_VALUE, &clk, sizeof(clk),
> -                 &stat, sizeof(stat));
> +    return scpi_info->send_msg(SCPI_CMD_SET_CLOCK_VALUE, &clk, sizeof(clk),
> +                   &stat, sizeof(stat));
>  }
> 
>  static int scpi_dvfs_get_idx(u8 domain)
> @@ -434,8 +438,8 @@ static int scpi_dvfs_get_idx(u8 domain)
>      int ret;
>      u8 dvfs_idx;
> 
> -    ret = scpi_send_message(SCPI_CMD_GET_DVFS, &domain, sizeof(domain),
> -                &dvfs_idx, sizeof(dvfs_idx));
> +    ret = scpi_info->send_msg(SCPI_CMD_GET_DVFS, &domain, sizeof(domain),
> +                  &dvfs_idx, sizeof(dvfs_idx));
>      return ret ? ret : dvfs_idx;
>  }
> 
> @@ -444,8 +448,8 @@ static int scpi_dvfs_set_idx(u8 domain, u8 index)
>      int stat;
>      struct dvfs_set dvfs = {domain, index};
> 
> -    return scpi_send_message(SCPI_CMD_SET_DVFS, &dvfs, sizeof(dvfs),
> -                 &stat, sizeof(stat));
> +    return scpi_info->send_msg(SCPI_CMD_SET_DVFS, &dvfs, sizeof(dvfs),
> +                   &stat, sizeof(stat));
>  }
> 
>  static int opp_cmp_func(const void *opp1, const void *opp2)
> @@ -468,8 +472,8 @@ static struct scpi_dvfs_info *scpi_dvfs_get_info(u8 domain)
>      if (scpi_info->dvfs[domain])    /* data already populated */
>          return scpi_info->dvfs[domain];
> 
> -    ret = scpi_send_message(SCPI_CMD_GET_DVFS_INFO, &domain, sizeof(domain),
> -                &buf, sizeof(buf));
> +    ret = scpi_info->send_msg(SCPI_CMD_GET_DVFS_INFO, &domain,
> +                  sizeof(domain), &buf, sizeof(buf));
> 
>      if (ret)
>          return ERR_PTR(ret);
> @@ -503,8 +507,8 @@ static int scpi_sensor_get_capability(u16 *sensors)
>      struct sensor_capabilities cap_buf;
>      int ret;
> 
> -    ret = scpi_send_message(SCPI_CMD_SENSOR_CAPABILITIES, NULL, 0, &cap_buf,
> -                sizeof(cap_buf));
> +    ret = scpi_info->send_msg(SCPI_CMD_SENSOR_CAPABILITIES, NULL, 0,
> +                  &cap_buf, sizeof(cap_buf));
>      if (!ret)
>          *sensors = le16_to_cpu(cap_buf.sensors);
> 
> @@ -517,8 +521,8 @@ static int scpi_sensor_get_info(u16 sensor_id, struct scpi_sensor_info *info)
>      struct _scpi_sensor_info _info;
>      int ret;
> 
> -    ret = scpi_send_message(SCPI_CMD_SENSOR_INFO, &id, sizeof(id),
> -                &_info, sizeof(_info));
> +    ret = scpi_info->send_msg(SCPI_CMD_SENSOR_INFO, &id, sizeof(id),
> +                  &_info, sizeof(_info));
>      if (!ret) {
>          memcpy(info, &_info, sizeof(*info));
>          info->sensor_id = le16_to_cpu(_info.sensor_id);
> @@ -533,8 +537,8 @@ static int scpi_sensor_get_value(u16 sensor, u64 *val)
>      struct sensor_value buf;
>      int ret;
> 
> -    ret = scpi_send_message(SCPI_CMD_SENSOR_VALUE, &id, sizeof(id),
> -                &buf, sizeof(buf));
> +    ret = scpi_info->send_msg(SCPI_CMD_SENSOR_VALUE, &id, sizeof(id),
> +                  &buf, sizeof(buf));
>      if (!ret)
>          *val = (u64)le32_to_cpu(buf.hi_val) << 32 |
>              le32_to_cpu(buf.lo_val);
> @@ -548,8 +552,8 @@ static int scpi_device_get_power_state(u16 dev_id)
>      u8 pstate;
>      __le16 id = cpu_to_le16(dev_id);
> 
> -    ret = scpi_send_message(SCPI_CMD_GET_DEVICE_PWR_STATE, &id,
> -                sizeof(id), &pstate, sizeof(pstate));
> +    ret = scpi_info->send_msg(SCPI_CMD_GET_DEVICE_PWR_STATE, &id,
> +                  sizeof(id), &pstate, sizeof(pstate));
>      return ret ? ret : pstate;
>  }
> 
> @@ -561,8 +565,8 @@ static int scpi_device_set_power_state(u16 dev_id, u8 pstate)
>          .pstate = pstate,
>      };
> 
> -    return scpi_send_message(SCPI_CMD_SET_DEVICE_PWR_STATE, &dev_set,
> -                 sizeof(dev_set), &stat, sizeof(stat));
> +    return scpi_info->send_msg(SCPI_CMD_SET_DEVICE_PWR_STATE, &dev_set,
> +                   sizeof(dev_set), &stat, sizeof(stat));
>  }
> 
>  static struct scpi_ops scpi_ops = {
> @@ -591,8 +595,8 @@ static int scpi_init_versions(struct scpi_drvinfo *info)
>      int ret;
>      struct scp_capabilities caps;
> 
> -    ret = scpi_send_message(SCPI_CMD_SCPI_CAPABILITIES, NULL, 0,
> -                &caps, sizeof(caps));
> +    ret = scpi_info->send_msg(SCPI_CMD_SCPI_CAPABILITIES, NULL, 0,
> +                  &caps, sizeof(caps));
>      if (!ret) {
>          info->protocol_version = le32_to_cpu(caps.protocol_version);
>          info->firmware_version = le32_to_cpu(caps.platform_version);
> @@ -681,6 +685,14 @@ 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", .data = scpi_send_message},
> +    {.compatible = "arm,scpi-legacy", .data = scpi_send_message},
> +    {},
> +};
> +
> +MODULE_DEVICE_TABLE(of, scpi_of_match);
> +
>  static int scpi_probe(struct platform_device *pdev)
>  {
>      int count, idx, ret;
> @@ -688,11 +700,15 @@ 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;
> 
>      scpi_info = devm_kzalloc(dev, sizeof(*scpi_info), GFP_KERNEL);
>      if (!scpi_info)
>          return -ENOMEM;
> 
> +    match = of_match_device(scpi_of_match, dev);
> +    scpi_info->send_msg = 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);
> @@ -778,13 +794,6 @@ static int scpi_probe(struct platform_device *pdev)
>      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",

Re: [RFC PATCH v3 5/8] firmware: Add legacy SCPI protocol driver

[Sudeep Holla]

Hi Neil,

On 16/08/16 10:41, Neil Armstrong wrote:
> On 08/15/2016 03:35 PM, Sudeep Holla wrote:
>> Hi Neil,
>>
>> On 09/08/16 11:29, Neil Armstrong wrote:
>>> Add legacy protocol driver for an early published SCPI implementation
>>> by supporting old command indexes and structure.
>>> This driver also supports vendor messages and rockchip specific
>>> mailbox data structure for message passing to SCP.
>>>
>>
>
> Hi Sudeep,
>
>> Sorry for the delay but I expected some attempts to reduce the
>> duplication of code we have here. I really don't like the duplication of
>> the code. As I mentioned earlier it can be reduced. I see lot of scope
>> for that and I see that you made zero attempts since v2.
>
> Yes, this post is an intermediate RFC to show to rockchip guys how I
> could implement supportfor their vendor commands, the next RFC will
> certainly merge the two  drivers with minimal code duplication.
>

Ah sorry for the comment then, I didn't realize that. I missed those
discussions.

>>> +enum legacy_scpi_client_id {
>>> +    SCPI_CL_NONE,
>>> +    SCPI_CL_CLOCKS,
>>> +    SCPI_CL_DVFS,
>>> +    SCPI_CL_POWER,
>>> +    SCPI_CL_THERMAL,
>>> +    SCPI_MAX,
>>> +};
>>> +
>>
>> As I said before these values were introduced by me initially and I
>> reckon firmware doesn't depend on that. Have you really tested dropping
>> them ? This must go as they are useless and we now have tokens which are
>> much better.
>
> I am not sure, I'm currently waiting for Amlogic's SCPI FW specification to be sure
> these are not needed at all, in the meantime I will still implement them.
>

Just change and give it a spin on your board. The initial spec mentions
that the SCP should return this as is as sent by the requester. I
thought of this grouping initially and then found certain limitations
and went with token which is bit more useful compared to this after some
review.

>>
>> I will stop here and ask why can't you start with simple change like
>> below ? Then we can add or re-define the structures/enums when
>> absolutely needed. Please don't just copy the entire driver and make
>> changes where-ever needed or please try to adapt to the new driver and
>> try to deviate as less as required by the firmware.
>
> It will be done in RFC v4.
>

Cool, thanks. I will try to review ASAP, so that we can target v4.9

-- 
Regards,
Sudeep