Hi,
This patch series adds support for:
1. SCPI(System Control and Power Interface) mailbox protocol
driver. It uses ARM MHU mailbox controller driver on Juno
but can work with any mailbox controllers using standard
mailbox APIs
2. Add support for clocks provided by SCP firmware through
the SCPI interface
3. Using the existing arm_big_little cpufreq driver and the
newly added SCPI clock driver, it also adds support for DVFS
on ARM64 JUNO development platforms.
Regards,
Sudeep
Sudeep Holla (4):
mailbox: add support for System Control and Power Interface(SCPI)
protocol
clk: add support for clocks provided by SCP(System Control Processor)
clk: scpi: add support for cpufreq virtual device
cpufreq: arm_big_little: add SCPI interface driver
.../devicetree/bindings/mailbox/arm,scpi.txt | 121 ++++
drivers/clk/Kconfig | 10 +
drivers/clk/Makefile | 1 +
drivers/clk/clk-scpi.c | 357 +++++++++++
drivers/cpufreq/Kconfig.arm | 9 +
drivers/cpufreq/Makefile | 1 +
drivers/cpufreq/scpi-cpufreq.c | 103 +++
drivers/mailbox/Kconfig | 19 +
drivers/mailbox/Makefile | 2 +
drivers/mailbox/scpi_protocol.c | 694 +++++++++++++++++++++
include/linux/scpi_protocol.h | 57 ++
11 files changed, 1374 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mailbox/arm,scpi.txt
create mode 100644 drivers/clk/clk-scpi.c
create mode 100644 drivers/cpufreq/scpi-cpufreq.c
create mode 100644 drivers/mailbox/scpi_protocol.c
create mode 100644 include/linux/scpi_protocol.h
--
1.9.1
This patch adds support for System Control and Power Interface (SCPI)
Message Protocol used between the Application Cores(AP) and the System
Control Processor(SCP). The MHU peripheral provides a mechanism for
inter-processor communication between SCP's M3 processor 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.
This protocol driver provides interface for all the client drivers using
SCPI to make use of the features offered by the SCP.
Signed-off-by: Sudeep Holla <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Mark Rutland <[email protected]>
CC: Jassi Brar <[email protected]>
Cc: Liviu Dudau <[email protected]>
Cc: Lorenzo Pieralisi <[email protected]>
Cc: Jon Medhurst (Tixy) <[email protected]>
Cc: [email protected]
---
.../devicetree/bindings/mailbox/arm,scpi.txt | 121 ++++
drivers/mailbox/Kconfig | 19 +
drivers/mailbox/Makefile | 2 +
drivers/mailbox/scpi_protocol.c | 694 +++++++++++++++++++++
include/linux/scpi_protocol.h | 57 ++
5 files changed, 893 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mailbox/arm,scpi.txt
create mode 100644 drivers/mailbox/scpi_protocol.c
create mode 100644 include/linux/scpi_protocol.h
diff --git a/Documentation/devicetree/bindings/mailbox/arm,scpi.txt b/Documentation/devicetree/bindings/mailbox/arm,scpi.txt
new file mode 100644
index 000000000000..5db235f69e54
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/arm,scpi.txt
@@ -0,0 +1,121 @@
+System Control and Power Interface (SCPI) Message Protocol
+----------------------------------------------------------
+
+Required properties:
+
+- compatible : should be "arm,scpi"
+- mboxes: List of phandle and mailbox channel specifiers
+- shmem : List of phandle pointing to the shared memory(SHM) area between the
+ processors using these mailboxes for IPC, one for each mailbox
+
+See Documentation/devicetree/bindings/mailbox/mailbox.txt
+for more details about the generic mailbox controller and
+client driver bindings.
+
+Clock bindings for the clocks based on SCPI Message Protocol
+------------------------------------------------------------
+
+This binding uses the common clock binding[1].
+
+Required properties:
+- compatible : shall be one of the following:
+ "arm,scpi-clocks" - for the container node with all the clocks
+ based on the SCPI protocol
+ "arm,scpi-dvfs" - all the clocks that are variable and index based.
+ These clocks don't provide the full range between the limits
+ but only discrete points within the range. The firmware
+ provides the mapping for each such operating frequency and the
+ index associated with it. The firmware also manages the
+ voltage scaling appropriately with the clock scaling.
+ "arm,scpi-clk" - all the clocks that are variable and provide full
+ range within the specified range. The firmware provides the
+ supported range for each clock.
+
+Required properties for all clocks(all from common clock binding):
+- #clock-cells : should be set to 1 as each of the SCPI clocks have multiple
+ outputs. The clock specifier will be the index to an entry in the list
+ of output clocks.
+- clock-output-names : shall be the corresponding names of the outputs.
+- clock-indices: The identifyng number for the clocks(clock_id) in the node as
+ expected by the firmware. It can be non linear and hence provide the
+ mapping of identifiers into the clock-output-names array.
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Example:
+
+sram: sram@50000000 {
+ compatible = "arm,juno-sram-ns", "mmio-sram";
+ reg = <0x0 0x50000000 0x0 0x10000>;
+
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0 0x0 0x50000000 0x10000>;
+
+ cpu_scp_lpri: scp-shmem@0 {
+ compatible = "arm,juno-scp-shmem";
+ reg = <0x0 0x200>;
+ };
+
+ cpu_scp_hpri: scp-shmem@200 {
+ compatible = "arm,juno-scp-shmem";
+ reg = <0x200 0x200>;
+ };
+};
+
+mailbox: mailbox0@40000000 {
+ ....
+ #mbox-cells = <1>;
+};
+
+scpi_protocol: scpi@2e000000 {
+ compatible = "arm,scpi";
+ mboxes = <&mailbox 0 &mailbox 1>;
+ shmem = <&cpu_scp_lpri &cpu_scp_hpri>;
+
+ clocks {
+ compatible = "arm,scpi-clocks";
+
+ scpi_dvfs: scpi_clocks@0 {
+ compatible = "arm,scpi-dvfs";
+ #clock-cells = <1>;
+ clock-indices = <0>, <1>, <2>;
+ clock-output-names = "vbig", "vlittle", "vgpu";
+ };
+ scpi_clk: scpi_clocks@3 {
+ compatible = "arm,scpi-clk";
+ #clock-cells = <1>;
+ clock-indices = <3>, <4>;
+ clock-output-names = "pxlclk0", "pxlclk1";
+ };
+ };
+};
+
+cpu@0 {
+ ...
+ reg = <0 0>;
+ clocks = <&scpi_dvfs 0>;
+ clock-names = "big";
+};
+
+hdlcd@7ff60000 {
+ ...
+ reg = <0 0x7ff60000 0 0x1000>;
+ clocks = <&scpi_clk 1>;
+ clock-names = "pxlclk";
+};
+
+In the above example, the #clock-cells is set to 1 as required.
+scpi_dvfs has 3 output clocks namely: vbig, vlittle and vgpu with 0, 1
+and 2 as clock-indices. scpi_clk has 2 output clocks namely: pxlclk0 and
+pxlclk1 with 3 and 4 as clock-indices.
+
+The first consumer in the example is cpu@0 and it has vbig as input clock.
+The index '0' in the clock specifier here points to the first entry in the
+output clocks of scpi_dvfs for which clock_id asrequired by the firmware
+is 0.
+
+Similarly the second example is hdlcd@7ff60000 and it has pxlclk0 as input
+clock. The index '1' in the clock specifier here points to the second entry
+in the output clocks of scpi_clocks for which clock_id as required by the
+firmware is 4.
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 84b0a2d74d60..20373cf84320 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -15,6 +15,25 @@ config ARM_MHU
The controller has 3 mailbox channels, the last of which can be
used in Secure mode only.
+config ARM_SCPI_PROTOCOL
+ tristate "ARM System Control and Power Interface (SCPI) Message Protocol"
+ depends on ARM_MHU
+ 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 SCP.
+
config PL320_MBOX
bool "ARM PL320 Mailbox"
depends on ARM_AMBA
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index b18201e97e29..762760e19d0f 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -11,3 +11,5 @@ obj-$(CONFIG_OMAP2PLUS_MBOX) += omap-mailbox.o
obj-$(CONFIG_PCC) += pcc.o
obj-$(CONFIG_ALTERA_MBOX) += mailbox-altera.o
+
+obj-$(CONFIG_ARM_SCPI_PROTOCOL) += scpi_protocol.o
diff --git a/drivers/mailbox/scpi_protocol.c b/drivers/mailbox/scpi_protocol.c
new file mode 100644
index 000000000000..c74575bca845
--- /dev/null
+++ b/drivers/mailbox/scpi_protocol.c
@@ -0,0 +1,694 @@
+/*
+ * 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) 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/>.
+ */
+
+#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/spinlock.h>
+
+#define CMD_ID_SHIFT 0
+#define CMD_ID_MASK 0x7f
+#define CMD_TOKEN_ID_SHIFT 8
+#define CMD_TOKEN_ID_MASK 0xff
+#define CMD_DATA_SIZE_SHIFT 16
+#define CMD_DATA_SIZE_MASK 0x1ff
+#define PACK_SCPI_CMD(cmd_id, token, tx_sz) \
+ ((((cmd_id) & CMD_ID_MASK) << CMD_ID_SHIFT) | \
+ (((token) & CMD_TOKEN_ID_MASK) << CMD_TOKEN_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 SCPI_SLOT 0
+
+#define MAX_DVFS_DOMAINS 8
+#define MAX_DVFS_OPPS 8
+#define DVFS_LATENCY(hdr) (le32_to_cpu(hdr) >> 16)
+#define DVFS_OPP_COUNT(hdr) ((le32_to_cpu(hdr) >> 8) & 0xff)
+
+#define PROTOCOL_REV_MINOR_BITS 16
+#define PROTOCOL_REV_MINOR_MASK ((1U << PROTOCOL_REV_MINOR_BITS) - 1)
+#define PROTOCOL_REV_MAJOR(x) ((x) >> PROTOCOL_REV_MINOR_BITS)
+#define PROTOCOL_REV_MINOR(x) ((x) & PROTOCOL_REV_MINOR_MASK)
+
+#define FW_REV_MAJOR_BITS 24
+#define FW_REV_MINOR_BITS 16
+#define FW_REV_PATCH_MASK ((1U << FW_REV_MINOR_BITS) - 1)
+#define FW_REV_MINOR_MASK ((1U << FW_REV_MAJOR_BITS) - 1)
+#define FW_REV_MAJOR(x) ((x) >> FW_REV_MAJOR_BITS)
+#define FW_REV_MINOR(x) (((x) & FW_REV_MINOR_MASK) >> FW_REV_MINOR_BITS)
+#define FW_REV_PATCH(x) ((x) & FW_REV_PATCH_MASK)
+
+#define MAX_RX_TIMEOUT (msecs_to_jiffies(30))
+
+enum 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 scpi_std_cmd {
+ SCPI_CMD_INVALID = 0x00,
+ SCPI_CMD_SCPI_READY = 0x01,
+ SCPI_CMD_SCPI_CAPABILITIES = 0x02,
+ SCPI_CMD_SET_CSS_PWR_STATE = 0x03,
+ SCPI_CMD_GET_CSS_PWR_STATE = 0x04,
+ SCPI_CMD_SET_SYS_PWR_STATE = 0x05,
+ SCPI_CMD_SET_CPU_TIMER = 0x06,
+ SCPI_CMD_CANCEL_CPU_TIMER = 0x07,
+ SCPI_CMD_DVFS_CAPABILITIES = 0x08,
+ SCPI_CMD_GET_DVFS_INFO = 0x09,
+ SCPI_CMD_SET_DVFS = 0x0a,
+ SCPI_CMD_GET_DVFS = 0x0b,
+ SCPI_CMD_GET_DVFS_STAT = 0x0c,
+ SCPI_CMD_CLOCK_CAPABILITIES = 0x0d,
+ SCPI_CMD_GET_CLOCK_INFO = 0x0e,
+ SCPI_CMD_SET_CLOCK_VALUE = 0x0f,
+ SCPI_CMD_GET_CLOCK_VALUE = 0x10,
+ SCPI_CMD_PSU_CAPABILITIES = 0x11,
+ SCPI_CMD_GET_PSU_INFO = 0x12,
+ SCPI_CMD_SET_PSU = 0x13,
+ SCPI_CMD_GET_PSU = 0x14,
+ SCPI_CMD_SENSOR_CAPABILITIES = 0x15,
+ SCPI_CMD_SENSOR_INFO = 0x16,
+ SCPI_CMD_SENSOR_VALUE = 0x17,
+ SCPI_CMD_SENSOR_CFG_PERIODIC = 0x18,
+ SCPI_CMD_SENSOR_CFG_BOUNDS = 0x19,
+ SCPI_CMD_SENSOR_ASYNC_VALUE = 0x1a,
+ SCPI_CMD_SET_DEVICE_PWR_STATE = 0x1b,
+ SCPI_CMD_GET_DEVICE_PWR_STATE = 0x1c,
+ SCPI_CMD_COUNT
+};
+
+struct scpi_xfer {
+ u32 slot; /* has to be first element */
+ u32 cmd;
+ u32 status;
+ const void *tx_buf;
+ void *rx_buf;
+ unsigned int tx_len;
+ struct list_head node;
+ struct completion done;
+};
+
+struct scpi_chan {
+ struct mbox_client cl;
+ struct mbox_chan *chan;
+ void __iomem *tx_payload;
+ void __iomem *rx_payload;
+ struct list_head rx_pending;
+ struct list_head xfers_list;
+ struct scpi_xfer *xfers;
+ spinlock_t rx_lock; /* locking for the rx pending list */
+ struct mutex xfers_lock;
+ atomic_t token;
+};
+
+struct scpi_drvinfo {
+ u32 protocol_version;
+ 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];
+};
+
+/*
+ * The SCP firmware only executes in little-endian mode, so any buffers
+ * shared through SCPI should have their contents converted to little-endian
+ */
+struct scpi_shared_mem {
+ __le32 command;
+ __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 {
+ __le16 id;
+ __le16 reserved;
+ __le32 rate;
+} __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;
+
+static struct scpi_drvinfo *scpi_info;
+
+static int 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 scpi_to_linux_errno(int errno)
+{
+ if (errno >= SCPI_SUCCESS && errno < SCPI_ERR_MAX)
+ return scpi_linux_errmap[errno];
+ return -EIO;
+}
+
+static void scpi_process_cmd(struct scpi_chan *ch, u32 cmd)
+{
+ unsigned long flags;
+ struct scpi_xfer *t, *match = NULL;
+
+ spin_lock_irqsave(&ch->rx_lock, flags);
+ if (list_empty(&ch->rx_pending)) {
+ spin_unlock_irqrestore(&ch->rx_lock, flags);
+ return;
+ }
+
+ list_for_each_entry(t, &ch->rx_pending, node)
+ if (CMD_XTRACT_UNIQ(t->cmd) == CMD_XTRACT_UNIQ(cmd)) {
+ list_del(&t->node);
+ match = t;
+ break;
+ }
+ /* check if wait_for_completion is in progress or timed-out */
+ if (match && !completion_done(&match->done)) {
+ struct scpi_shared_mem *mem = ch->rx_payload;
+
+ match->status = le32_to_cpu(mem->status);
+ memcpy_fromio(match->rx_buf, mem->payload, CMD_SIZE(cmd));
+ complete(&match->done);
+ }
+ spin_unlock_irqrestore(&ch->rx_lock, flags);
+}
+
+static void scpi_handle_remote_msg(struct mbox_client *c, void *msg)
+{
+ struct scpi_chan *ch = container_of(c, struct scpi_chan, cl);
+ struct scpi_shared_mem *mem = ch->rx_payload;
+ u32 cmd = le32_to_cpu(mem->command);
+
+ scpi_process_cmd(ch, cmd);
+}
+
+static void scpi_tx_prepare(struct mbox_client *c, void *msg)
+{
+ unsigned long flags;
+ struct scpi_xfer *t = msg;
+ struct scpi_chan *ch = container_of(c, struct scpi_chan, cl);
+ struct scpi_shared_mem *mem = (struct scpi_shared_mem *)ch->tx_payload;
+
+ mem->command = cpu_to_le32(t->cmd);
+ if (t->tx_buf)
+ memcpy_toio(mem->payload, t->tx_buf, t->tx_len);
+ if (t->rx_buf) {
+ spin_lock_irqsave(&ch->rx_lock, flags);
+ list_add_tail(&t->node, &ch->rx_pending);
+ spin_unlock_irqrestore(&ch->rx_lock, flags);
+ }
+}
+
+static struct scpi_xfer *get_scpi_xfer(struct scpi_chan *ch)
+{
+ struct scpi_xfer *t;
+
+ mutex_lock(&ch->xfers_lock);
+ if (list_empty(&ch->xfers_list)) {
+ mutex_unlock(&ch->xfers_lock);
+ return NULL;
+ }
+ t = list_first_entry(&ch->xfers_list, struct scpi_xfer, node);
+ list_del(&t->node);
+ mutex_unlock(&ch->xfers_lock);
+ return t;
+}
+
+static void put_scpi_xfer(struct scpi_xfer *t, struct scpi_chan *ch)
+{
+ mutex_lock(&ch->xfers_lock);
+ list_add_tail(&t->node, &ch->xfers_list);
+ mutex_unlock(&ch->xfers_lock);
+}
+
+static int
+scpi_send_message(u8 cmd, void *tx_buf, unsigned int len, void *rx_buf)
+{
+ int ret;
+ u8 token, chan;
+ struct scpi_xfer *msg;
+ struct scpi_chan *scpi_chan;
+
+ chan = atomic_inc_return(&scpi_info->next_chan) % scpi_info->num_chans;
+ scpi_chan = scpi_info->channels + chan;
+
+ msg = get_scpi_xfer(scpi_chan);
+ if (!msg)
+ return -ENOMEM;
+
+ token = atomic_inc_return(&scpi_chan->token) & CMD_TOKEN_ID_MASK;
+
+ msg->slot = BIT(SCPI_SLOT);
+ msg->cmd = PACK_SCPI_CMD(cmd, token, len);
+ msg->tx_buf = tx_buf;
+ msg->tx_len = len;
+ msg->rx_buf = rx_buf;
+ init_completion(&msg->done);
+
+ ret = mbox_send_message(scpi_chan->chan, msg);
+ if (ret < 0 || !rx_buf)
+ goto out;
+
+ if (!wait_for_completion_timeout(&msg->done, MAX_RX_TIMEOUT))
+ ret = -ETIMEDOUT;
+ else
+ /* first status word */
+ ret = le32_to_cpu(msg->status);
+out:
+ if (ret < 0 && rx_buf) /* remove entry from the list if timed-out */
+ scpi_process_cmd(scpi_chan, msg->cmd);
+
+ put_scpi_xfer(msg, scpi_chan);
+ /* SCPI error codes > 0, translate them to Linux scale*/
+ return ret > 0 ? scpi_to_linux_errno(ret) : ret;
+}
+
+static u32 scpi_get_version(void)
+{
+ return scpi_info->protocol_version;
+}
+
+static int
+scpi_clk_get_range(u16 clk_id, unsigned long *min, unsigned long *max)
+{
+ int ret;
+ 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);
+ if (!ret) {
+ *min = le32_to_cpu(clk.min_rate);
+ *max = le32_to_cpu(clk.max_rate);
+ }
+ return ret;
+}
+
+static unsigned long scpi_clk_get_val(u16 clk_id)
+{
+ int ret;
+ 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);
+ return ret ? ret : le32_to_cpu(clk.rate);
+}
+
+static int scpi_clk_set_val(u16 clk_id, unsigned long rate)
+{
+ int stat;
+ struct clk_set_value clk = {
+ cpu_to_le16(clk_id), 0, cpu_to_le32(rate)
+ };
+
+ return scpi_send_message(SCPI_CMD_SET_CLOCK_VALUE,
+ &clk, sizeof(clk), &stat);
+}
+
+static int scpi_dvfs_get_idx(u8 domain)
+{
+ int ret;
+ struct dvfs_get dvfs;
+
+ ret = scpi_send_message(SCPI_CMD_GET_DVFS,
+ &domain, sizeof(domain), &dvfs);
+ return ret ? ret : dvfs.index;
+}
+
+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);
+}
+
+static struct scpi_dvfs_info *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 (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);
+
+ 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);
+ }
+
+ scpi_info->dvfs[domain] = info;
+ return info;
+}
+
+static struct scpi_ops scpi_ops = {
+ .get_version = scpi_get_version,
+ .clk_get_range = scpi_clk_get_range,
+ .clk_get_val = scpi_clk_get_val,
+ .clk_set_val = scpi_clk_set_val,
+ .dvfs_get_idx = scpi_dvfs_get_idx,
+ .dvfs_set_idx = scpi_dvfs_set_idx,
+ .dvfs_get_info = scpi_dvfs_get_info,
+};
+
+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;
+ struct scp_capabilities caps;
+
+ ret = scpi_send_message(SCPI_CMD_SCPI_CAPABILITIES, NULL, 0, &caps);
+ if (!ret) {
+ info->protocol_version = le32_to_cpu(caps.protocol_version);
+ info->firmware_version = le32_to_cpu(caps.platform_version);
+ }
+ return ret;
+}
+
+static ssize_t protocol_version_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct scpi_drvinfo *scpi_info = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%d.%d\n",
+ PROTOCOL_REV_MAJOR(scpi_info->protocol_version),
+ PROTOCOL_REV_MINOR(scpi_info->protocol_version));
+}
+static DEVICE_ATTR_RO(protocol_version);
+
+static ssize_t firmware_version_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct scpi_drvinfo *scpi_info = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%d.%d.%d\n",
+ FW_REV_MAJOR(scpi_info->firmware_version),
+ FW_REV_MINOR(scpi_info->firmware_version),
+ FW_REV_PATCH(scpi_info->firmware_version));
+}
+static DEVICE_ATTR_RO(firmware_version);
+
+static struct attribute *versions_attrs[] = {
+ &dev_attr_firmware_version.attr,
+ &dev_attr_protocol_version.attr,
+ NULL,
+};
+ATTRIBUTE_GROUPS(versions);
+
+static void
+scpi_free_channels(struct device *dev, struct scpi_chan *pchan, int count)
+{
+ int i;
+
+ for (i = 0; i < count && pchan->chan; i++, pchan++) {
+ mbox_free_channel(pchan->chan);
+ devm_kfree(dev, pchan->xfers);
+ devm_iounmap(dev, pchan->rx_payload);
+ }
+}
+
+static int scpi_remove(struct platform_device *pdev)
+{
+ int i;
+ struct device *dev = &pdev->dev;
+ struct scpi_drvinfo *info = platform_get_drvdata(pdev);
+
+ scpi_info = NULL; /* stop exporting SCPI ops through get_scpi_ops */
+
+ of_platform_depopulate(dev);
+ sysfs_remove_groups(&dev->kobj, versions_groups);
+ 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]);
+ }
+ devm_kfree(dev, info->channels);
+ devm_kfree(dev, info);
+
+ return 0;
+}
+
+#define MAX_SCPI_XFERS 10
+static int scpi_alloc_xfer_list(struct device *dev, struct scpi_chan *ch)
+{
+ int i;
+ struct scpi_xfer *xfers;
+
+ xfers = devm_kzalloc(dev, MAX_SCPI_XFERS * sizeof(*xfers), GFP_KERNEL);
+ if (!xfers)
+ return -ENOMEM;
+
+ ch->xfers = xfers;
+ for (i = 0; i < MAX_SCPI_XFERS; i++, xfers++)
+ list_add_tail(&xfers->node, &ch->xfers_list);
+ return 0;
+}
+
+static int scpi_probe(struct platform_device *pdev)
+{
+ int count, idx, ret;
+ struct resource res;
+ struct scpi_chan *scpi_chan;
+ struct device *dev = &pdev->dev;
+ struct device_node *np = dev->of_node;
+
+ scpi_info = devm_kzalloc(dev, sizeof(*scpi_info), GFP_KERNEL);
+ if (!scpi_info) {
+ dev_err(dev, "failed to allocate memory for scpi drvinfo\n");
+ 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;
+ }
+
+ scpi_chan = devm_kcalloc(dev, count, sizeof(*scpi_chan), GFP_KERNEL);
+ if (!scpi_chan) {
+ dev_err(dev, "failed to allocate memory scpi chaninfo\n");
+ return -ENOMEM;
+ }
+
+ for (idx = 0; idx < count; idx++) {
+ resource_size_t size;
+ struct scpi_chan *pchan = 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);
+
+ cl->dev = dev;
+ cl->rx_callback = scpi_handle_remote_msg;
+ cl->tx_prepare = scpi_tx_prepare;
+ cl->tx_block = true;
+ cl->tx_tout = 50;
+ cl->knows_txdone = false; /* controller can ack */
+
+ INIT_LIST_HEAD(&pchan->rx_pending);
+ INIT_LIST_HEAD(&pchan->xfers_list);
+ spin_lock_init(&pchan->rx_lock);
+ mutex_init(&pchan->xfers_lock);
+
+ ret = scpi_alloc_xfer_list(dev, pchan);
+ if (!ret) {
+ pchan->chan = mbox_request_channel(cl, idx);
+ if (!IS_ERR(pchan->chan))
+ continue;
+ ret = -EPROBE_DEFER;
+ dev_err(dev, "failed to acquire channel#%d\n", idx);
+ }
+err:
+ scpi_free_channels(dev, scpi_chan, idx);
+ scpi_info = NULL;
+ return ret;
+ }
+
+ scpi_info->channels = scpi_chan;
+ scpi_info->num_chans = count;
+ platform_set_drvdata(pdev, scpi_info);
+
+ ret = scpi_init_versions(scpi_info);
+ if (ret) {
+ dev_err(dev, "incorrect or no SCP firmware found\n");
+ scpi_remove(pdev);
+ return ret;
+ }
+
+ _dev_info(dev, "SCP Protocol %d.%d Firmware %d.%d.%d version\n",
+ PROTOCOL_REV_MAJOR(scpi_info->protocol_version),
+ PROTOCOL_REV_MINOR(scpi_info->protocol_version),
+ 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 = sysfs_create_groups(&dev->kobj, versions_groups);
+ if (ret)
+ dev_err(dev, "unable to create sysfs version group\n");
+
+ return of_platform_populate(dev->of_node, NULL, NULL, dev);
+}
+
+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",
+ .of_match_table = scpi_of_match,
+ },
+ .probe = scpi_probe,
+ .remove = scpi_remove,
+};
+module_platform_driver(scpi_driver);
+
+MODULE_AUTHOR("Sudeep Holla <[email protected]>");
+MODULE_DESCRIPTION("ARM SCPI mailbox protocol driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/scpi_protocol.h b/include/linux/scpi_protocol.h
new file mode 100644
index 000000000000..a33fb2937230
--- /dev/null
+++ b/include/linux/scpi_protocol.h
@@ -0,0 +1,57 @@
+/*
+ * SCPI Message Protocol driver header
+ *
+ * Copyright (C) 2014 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/>.
+ */
+#include <linux/types.h>
+
+struct scpi_opp {
+ u32 freq;
+ u32 m_volt;
+} __packed;
+
+struct scpi_dvfs_info {
+ unsigned int count;
+ unsigned int latency; /* in nanoseconds */
+ struct scpi_opp *opps;
+};
+
+/**
+ * struct scpi_ops - represents the various operations provided
+ * by SCP through SCPI message protocol
+ * @get_version: returns the major and minor revision on the SCPI
+ * message protocol
+ * @clk_get_range: gets clock range limit(min - max in Hz)
+ * @clk_get_val: gets clock value(in Hz)
+ * @clk_set_val: sets the clock value, setting to 0 will disable the
+ * clock (if supported)
+ * @dvfs_get_idx: gets the Operating Point of the given power domain.
+ * OPP is an index to the list return by @dvfs_get_info
+ * @dvfs_set_idx: sets the Operating Point of the given power domain.
+ * 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
+ */
+struct scpi_ops {
+ u32 (*get_version)(void);
+ int (*clk_get_range)(u16, unsigned long *, unsigned long *);
+ unsigned long (*clk_get_val)(u16);
+ int (*clk_set_val)(u16, unsigned long);
+ int (*dvfs_get_idx)(u8);
+ int (*dvfs_set_idx)(u8, u8);
+ struct scpi_dvfs_info *(*dvfs_get_info)(u8);
+};
+
+struct scpi_ops *get_scpi_ops(void);
--
1.9.1
On some ARM based systems, a separate Cortex-M based System Control
Processor(SCP) provides the overall power, clock, reset and system
control. System Control and Power Interface(SCPI) Message Protocol
is defined for the communication between the Application Cores(AP)
and the SCP.
This patch adds support for the clocks provided by SCP using SCPI
protocol.
Signed-off-by: Sudeep Holla <[email protected]>
Cc: Mike Turquette <[email protected]>
Cc: Stephen Boyd <[email protected]>
Cc: Liviu Dudau <[email protected]>
Cc: Lorenzo Pieralisi <[email protected]>
Cc: Jon Medhurst (Tixy) <[email protected]>
Cc: [email protected]
---
drivers/clk/Kconfig | 10 ++
drivers/clk/Makefile | 1 +
drivers/clk/clk-scpi.c | 344 +++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 355 insertions(+)
create mode 100644 drivers/clk/clk-scpi.c
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 9897f353bf1a..906bf7dd72a2 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -59,6 +59,16 @@ config COMMON_CLK_RK808
clocked at 32KHz each. Clkout1 is always on, Clkout2 can off
by control register.
+config COMMON_CLK_SCPI
+ tristate "Clock driver controlled via SCPI interface"
+ depends on ARM_SCPI_PROTOCOL
+ ---help---
+ This driver provides support for clocks that are controlled
+ by firmware that implements the SCPI interface.
+
+ This driver uses SCPI Message Protocol to interact with the
+ firware providing all the clock controls.
+
config COMMON_CLK_SI5351
tristate "Clock driver for SiLabs 5351A/B/C"
depends on I2C
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 3d00c25382c5..442ab6ebd5b1 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -36,6 +36,7 @@ obj-$(CONFIG_COMMON_CLK_PALMAS) += clk-palmas.o
obj-$(CONFIG_CLK_QORIQ) += clk-qoriq.o
obj-$(CONFIG_COMMON_CLK_RK808) += clk-rk808.o
obj-$(CONFIG_COMMON_CLK_S2MPS11) += clk-s2mps11.o
+obj-$(CONFIG_COMMON_CLK_SCPI) += clk-scpi.o
obj-$(CONFIG_COMMON_CLK_SI5351) += clk-si5351.o
obj-$(CONFIG_COMMON_CLK_SI570) += clk-si570.o
obj-$(CONFIG_CLK_TWL6040) += clk-twl6040.o
diff --git a/drivers/clk/clk-scpi.c b/drivers/clk/clk-scpi.c
new file mode 100644
index 000000000000..70a2d57da32d
--- /dev/null
+++ b/drivers/clk/clk-scpi.c
@@ -0,0 +1,344 @@
+/*
+ * System Control and Power Interface (SCPI) Protocol based clock driver
+ *
+ * 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/>.
+ */
+
+#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/of.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/scpi_protocol.h>
+
+struct scpi_clk {
+ u32 id;
+ const char *name;
+ struct clk_hw hw;
+ struct scpi_dvfs_info *info;
+ unsigned long rate_min;
+ unsigned long rate_max;
+};
+
+#define to_scpi_clk(clk) container_of(clk, struct scpi_clk, hw)
+
+static struct scpi_ops *scpi_ops;
+
+static unsigned long scpi_clk_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct scpi_clk *clk = to_scpi_clk(hw);
+
+ return scpi_ops->clk_get_val(clk->id);
+}
+
+static long scpi_clk_round_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long *parent_rate)
+{
+ struct scpi_clk *clk = to_scpi_clk(hw);
+
+ if (WARN_ON(clk->rate_min && rate < clk->rate_min))
+ rate = clk->rate_min;
+ if (WARN_ON(clk->rate_max && rate > clk->rate_max))
+ rate = clk->rate_max;
+
+ return rate;
+}
+
+static int scpi_clk_set_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long parent_rate)
+{
+ struct scpi_clk *clk = to_scpi_clk(hw);
+
+ return scpi_ops->clk_set_val(clk->id, rate);
+}
+
+static struct clk_ops scpi_clk_ops = {
+ .recalc_rate = scpi_clk_recalc_rate,
+ .round_rate = scpi_clk_round_rate,
+ .set_rate = scpi_clk_set_rate,
+};
+
+/* find closest match to given frequency in OPP table */
+static int __scpi_dvfs_round_rate(struct scpi_clk *clk, unsigned long rate)
+{
+ int idx;
+ u32 fmin = 0, fmax = ~0, ftmp;
+ struct scpi_opp *opp = clk->info->opps;
+
+ for (idx = 0; idx < clk->info->count; idx++, opp++) {
+ ftmp = opp->freq;
+ if (ftmp >= (u32)rate) {
+ if (ftmp <= fmax)
+ fmax = ftmp;
+ } else {
+ if (ftmp >= fmin)
+ fmin = ftmp;
+ }
+ }
+ if (fmax != ~0)
+ return fmax;
+ return fmin;
+}
+
+static unsigned long scpi_dvfs_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct scpi_clk *clk = to_scpi_clk(hw);
+ int idx = scpi_ops->dvfs_get_idx(clk->id);
+ struct scpi_opp *opp;
+
+ if (idx < 0)
+ return 0;
+
+ opp = clk->info->opps + idx;
+ return opp->freq;
+}
+
+static long scpi_dvfs_round_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long *parent_rate)
+{
+ struct scpi_clk *clk = to_scpi_clk(hw);
+
+ return __scpi_dvfs_round_rate(clk, rate);
+}
+
+static int __scpi_find_dvfs_index(struct scpi_clk *clk, unsigned long rate)
+{
+ int idx, max_opp = clk->info->count;
+ struct scpi_opp *opp = clk->info->opps;
+
+ for (idx = 0; idx < max_opp; idx++, opp++)
+ if (opp->freq == rate)
+ break;
+ return (idx == max_opp) ? -EINVAL : idx;
+}
+
+static int scpi_dvfs_set_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long parent_rate)
+{
+ struct scpi_clk *clk = to_scpi_clk(hw);
+ int ret = __scpi_find_dvfs_index(clk, rate);
+
+ if (ret < 0)
+ return ret;
+ return scpi_ops->dvfs_set_idx(clk->id, (u8)ret);
+}
+
+static struct clk_ops scpi_dvfs_ops = {
+ .recalc_rate = scpi_dvfs_recalc_rate,
+ .round_rate = scpi_dvfs_round_rate,
+ .set_rate = scpi_dvfs_set_rate,
+};
+
+static struct clk *
+scpi_dvfs_ops_init(struct device *dev, struct device_node *np,
+ struct scpi_clk *sclk)
+{
+ struct clk_init_data init;
+ struct scpi_dvfs_info *info;
+
+ init.name = sclk->name;
+ init.flags = CLK_IS_ROOT;
+ init.num_parents = 0;
+ init.ops = &scpi_dvfs_ops;
+ sclk->hw.init = &init;
+
+ info = scpi_ops->dvfs_get_info(sclk->id);
+ if (IS_ERR(info))
+ return (struct clk *)info;
+
+ sclk->info = info;
+
+ return devm_clk_register(dev, &sclk->hw);
+}
+
+static struct clk *
+scpi_clk_ops_init(struct device *dev, struct device_node *np,
+ struct scpi_clk *sclk)
+{
+ struct clk_init_data init;
+ int ret;
+
+ init.name = sclk->name;
+ init.flags = CLK_IS_ROOT;
+ init.num_parents = 0;
+ init.ops = &scpi_clk_ops;
+ sclk->hw.init = &init;
+
+ ret = scpi_ops->clk_get_range(sclk->id, &sclk->rate_min,
+ &sclk->rate_max);
+ if (!sclk->rate_max)
+ ret = -EINVAL;
+ if (ret)
+ return ERR_PTR(ret);
+
+ return devm_clk_register(dev, &sclk->hw);
+}
+
+static const struct of_device_id scpi_clk_match[] = {
+ { .compatible = "arm,scpi-dvfs", .data = scpi_dvfs_ops_init, },
+ { .compatible = "arm,scpi-clk", .data = scpi_clk_ops_init, },
+ {}
+};
+
+static int scpi_clk_probe(struct platform_device *pdev)
+{
+ struct clk **clks;
+ int idx, count;
+ struct clk_onecell_data *clk_data;
+ struct device *dev = &pdev->dev;
+ struct device_node *np = dev->of_node;
+ struct clk *(*clk_ops_init)(struct device *, struct device_node *,
+ struct scpi_clk *);
+
+ if (!of_device_is_available(np))
+ return -ENODEV;
+
+ clk_ops_init = of_match_node(scpi_clk_match, np)->data;
+ if (!clk_ops_init)
+ return -ENODEV;
+
+ count = of_property_count_strings(np, "clock-output-names");
+ if (count < 0) {
+ dev_err(dev, "%s: invalid clock output count\n", np->name);
+ return -EINVAL;
+ }
+
+ clk_data = devm_kmalloc(dev, sizeof(*clk_data), GFP_KERNEL);
+ if (!clk_data) {
+ dev_err(dev, "failed to allocate clock provider data\n");
+ return -ENOMEM;
+ }
+
+ clks = devm_kmalloc(dev, count * sizeof(*clks), GFP_KERNEL);
+ if (!clks) {
+ dev_err(dev, "failed to allocate clock providers\n");
+ return -ENOMEM;
+ }
+
+ for (idx = 0; idx < count; idx++) {
+ struct scpi_clk *sclk;
+ u32 val;
+
+ sclk = devm_kzalloc(dev, sizeof(*sclk), GFP_KERNEL);
+ if (!sclk) {
+ dev_err(dev, "failed to allocate scpi clocks\n");
+ return -ENOMEM;
+ }
+
+ if (of_property_read_string_index(np, "clock-output-names",
+ idx, &sclk->name)) {
+ dev_err(dev, "invalid clock name @ %s\n", np->name);
+ return -EINVAL;
+ }
+
+ if (of_property_read_u32_index(np, "clock-indices",
+ idx, &val)) {
+ dev_err(dev, "invalid clock index @ %s\n", np->name);
+ return -EINVAL;
+ }
+
+ sclk->id = val;
+
+ clks[idx] = clk_ops_init(dev, np, sclk);
+ if (IS_ERR(clks[idx])) {
+ dev_err(dev, "failed to register clock '%s'\n",
+ sclk->name);
+ }
+
+ dev_dbg(dev, "Registered clock '%s'\n", sclk->name);
+ }
+
+ clk_data->clks = clks;
+ clk_data->clk_num = idx;
+ of_clk_add_provider(np, of_clk_src_onecell_get, clk_data);
+
+ platform_set_drvdata(pdev, clk_data);
+
+ return 0;
+}
+
+static int scpi_clk_remove(struct platform_device *pdev)
+{
+ of_clk_del_provider(pdev->dev.of_node);
+ platform_set_drvdata(pdev, NULL);
+ return 0;
+}
+
+static struct platform_driver scpi_clk_driver = {
+ .driver = {
+ .name = "scpi_clk",
+ .of_match_table = scpi_clk_match,
+ },
+ .probe = scpi_clk_probe,
+ .remove = scpi_clk_remove,
+};
+
+static int scpi_clocks_probe(struct platform_device *pdev)
+{
+ scpi_ops = get_scpi_ops();
+ if (!scpi_ops)
+ return -ENXIO;
+
+ return of_platform_populate(pdev->dev.of_node, scpi_clk_match,
+ NULL, &pdev->dev);
+}
+
+static int scpi_clocks_remove(struct platform_device *pdev)
+{
+ of_platform_depopulate(&pdev->dev);
+ scpi_ops = NULL;
+ return 0;
+}
+
+static const struct of_device_id scpi_clocks_ids[] = {
+ { .compatible = "arm,scpi-clocks", },
+ {}
+};
+
+static struct platform_driver scpi_clocks_driver = {
+ .driver = {
+ .name = "scpi_clocks",
+ .of_match_table = scpi_clocks_ids,
+ },
+ .probe = scpi_clocks_probe,
+ .remove = scpi_clocks_remove,
+};
+
+static int __init scpi_clocks_init(void)
+{
+ int ret;
+
+ ret = platform_driver_register(&scpi_clk_driver);
+ if (ret)
+ return ret;
+
+ return platform_driver_register(&scpi_clocks_driver);
+}
+module_init(scpi_clocks_init);
+
+static void __exit scpi_clocks_exit(void)
+{
+ platform_driver_unregister(&scpi_clk_driver);
+ platform_driver_unregister(&scpi_clocks_driver);
+}
+module_exit(scpi_clocks_exit);
+
+MODULE_AUTHOR("Sudeep Holla <[email protected]>");
+MODULE_DESCRIPTION("ARM SCPI clock driver");
+MODULE_LICENSE("GPL");
--
1.9.1
The clocks for the CPUs are provided by SCP and are managed by this
clock driver. So the cpufreq device needs to be added only after the
clock get registered and removed when this driver is unloaded.
This patch manages the cpufreq virtual device based on the clock
availability.
Signed-off-by: Sudeep Holla <[email protected]>
Cc: Mike Turquette <[email protected]>
Cc: Stephen Boyd <[email protected]>
Cc: Liviu Dudau <[email protected]>
Cc: Lorenzo Pieralisi <[email protected]>
Cc: Jon Medhurst (Tixy) <[email protected]>
Cc: [email protected]
---
drivers/clk/clk-scpi.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/clk/clk-scpi.c b/drivers/clk/clk-scpi.c
index 70a2d57da32d..0c048f7d0c28 100644
--- a/drivers/clk/clk-scpi.c
+++ b/drivers/clk/clk-scpi.c
@@ -37,6 +37,7 @@ struct scpi_clk {
#define to_scpi_clk(clk) container_of(clk, struct scpi_clk, hw)
static struct scpi_ops *scpi_ops;
+static struct platform_device *cpufreq_dev;
static unsigned long scpi_clk_recalc_rate(struct clk_hw *hw,
unsigned long parent_rate)
@@ -270,11 +271,23 @@ static int scpi_clk_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, clk_data);
+ if (clk_ops_init == scpi_dvfs_ops_init) {
+ /* Add virtual cpufreq device depending SCPI clock */
+ cpufreq_dev = platform_device_register_simple("scpi-cpufreq",
+ -1, NULL, 0);
+ if (!cpufreq_dev)
+ pr_warn("unable to register cpufreq device");
+ }
return 0;
}
static int scpi_clk_remove(struct platform_device *pdev)
{
+ if (cpufreq_dev) {
+ platform_device_unregister(cpufreq_dev);
+ cpufreq_dev = NULL;
+ }
+
of_clk_del_provider(pdev->dev.of_node);
platform_set_drvdata(pdev, NULL);
return 0;
--
1.9.1
On some ARM based systems, a separate Cortex-M based System Control
Processor(SCP) provides the overall power, clock, reset and system
control including CPU DVFS. SCPI Message Protocol is used to
communicate with the SCPI.
This patch adds a interface driver for adding OPPs and registering
the arm_big_little cpufreq driver for such systems.
Signed-off-by: Sudeep Holla <[email protected]>
Cc: Viresh Kumar <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: [email protected]
---
drivers/cpufreq/Kconfig.arm | 9 ++++
drivers/cpufreq/Makefile | 1 +
drivers/cpufreq/scpi-cpufreq.c | 103 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 113 insertions(+)
create mode 100644 drivers/cpufreq/scpi-cpufreq.c
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 4f3dbc8cf729..9e678bf1687c 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -24,6 +24,15 @@ config ARM_VEXPRESS_SPC_CPUFREQ
This add the CPUfreq driver support for Versatile Express
big.LITTLE platforms using SPC for power management.
+config ARM_SCPI_CPUFREQ
+ tristate "SCPI based CPUfreq driver"
+ depends on ARM_BIG_LITTLE_CPUFREQ && ARM_SCPI_PROTOCOL
+ help
+ This add the CPUfreq driver support for ARM big.LITTLE platforms
+ using SCPI interface for CPU power management.
+
+ This driver works only if firmware the supporting CPU DVFS adhere
+ to SCPI protocol.
config ARM_EXYNOS_CPUFREQ
tristate "SAMSUNG EXYNOS CPUfreq Driver"
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index cdce92ae2e8b..02fc9f849d4b 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -79,6 +79,7 @@ obj-$(CONFIG_ARM_SA1110_CPUFREQ) += sa1110-cpufreq.o
obj-$(CONFIG_ARM_SPEAR_CPUFREQ) += spear-cpufreq.o
obj-$(CONFIG_ARM_TEGRA_CPUFREQ) += tegra-cpufreq.o
obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
+obj-$(CONFIG_ARM_SCPI_CPUFREQ) += scpi-cpufreq.o
##################################################################################
# PowerPC platform drivers
diff --git a/drivers/cpufreq/scpi-cpufreq.c b/drivers/cpufreq/scpi-cpufreq.c
new file mode 100644
index 000000000000..4c2c11a9dfc6
--- /dev/null
+++ b/drivers/cpufreq/scpi-cpufreq.c
@@ -0,0 +1,103 @@
+/*
+ * SCPI CPUFreq Interface driver
+ *
+ * It provides necessary ops to arm_big_little cpufreq driver.
+ *
+ * Copyright (C) 2015 ARM Ltd.
+ * Sudeep Holla <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/cpufreq.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm_opp.h>
+#include <linux/scpi_protocol.h>
+#include <linux/types.h>
+
+#include "arm_big_little.h"
+
+static struct scpi_ops *scpi_ops;
+
+static int scpi_init_opp_table(struct device *cpu_dev)
+{
+ u8 domain = topology_physical_package_id(cpu_dev->id);
+ struct scpi_dvfs_info *info;
+ struct scpi_opp *opp;
+ int idx, ret = 0;
+
+ if ((dev_pm_opp_get_opp_count(cpu_dev)) > 0)
+ return 0;
+ info = scpi_ops->dvfs_get_info(domain);
+ if (IS_ERR(info))
+ return PTR_ERR(info);
+
+ opp = info->opps;
+ if (!opp)
+ return -EIO;
+
+ for (idx = 0; idx < info->count; idx++, opp++) {
+ ret = dev_pm_opp_add(cpu_dev, opp->freq, opp->m_volt * 1000);
+ if (ret) {
+ dev_warn(cpu_dev, "failed to add opp %uHz %umV\n",
+ opp->freq, opp->m_volt);
+ return ret;
+ }
+ }
+ return ret;
+}
+
+static int scpi_get_transition_latency(struct device *cpu_dev)
+{
+ u8 domain = topology_physical_package_id(cpu_dev->id);
+ struct scpi_dvfs_info *info;
+
+ info = scpi_ops->dvfs_get_info(domain);
+ if (IS_ERR(info))
+ return PTR_ERR(info);
+
+ return info->latency;
+}
+
+static struct cpufreq_arm_bL_ops scpi_cpufreq_ops = {
+ .name = "scpi",
+ .get_transition_latency = scpi_get_transition_latency,
+ .init_opp_table = scpi_init_opp_table,
+};
+
+static int scpi_cpufreq_probe(struct platform_device *pdev)
+{
+ scpi_ops = get_scpi_ops();
+ if (!scpi_ops)
+ return -EIO;
+
+ return bL_cpufreq_register(&scpi_cpufreq_ops);
+}
+
+static int scpi_cpufreq_remove(struct platform_device *pdev)
+{
+ bL_cpufreq_unregister(&scpi_cpufreq_ops);
+ return 0;
+}
+
+static struct platform_driver scpi_cpufreq_platdrv = {
+ .driver = {
+ .name = "scpi-cpufreq",
+ .owner = THIS_MODULE,
+ },
+ .probe = scpi_cpufreq_probe,
+ .remove = scpi_cpufreq_remove,
+};
+module_platform_driver(scpi_cpufreq_platdrv);
+
+MODULE_LICENSE("GPL");
--
1.9.1
On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:
> Hi,
>
> This patch series adds support for:
> 1. SCPI(System Control and Power Interface) mailbox protocol
> driver.
Is there a public document with the final protocol specification?
I have spotted an inconsistency between driver and a draft doc I have
but before commenting on things like that it would be good to have the
final version of the document instead.
--
Tixy
Just one nit: a license mismatch.
On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:
> --- /dev/null
> +++ b/drivers/mailbox/scpi_protocol.c
> + * 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/>.
This states the license is GPL v2.
> +MODULE_LICENSE("GPL");
And, according to include/linux/module.h, this states the license is GPL
v2 or later. So I think either the comment at the top of this file or
the license ident used in the MODULE_LICENSE() macro should be changed.
Likewise for 2/4 and 4/4.
Thanks,
Paul Bolle
On 28/04/15 08:36, Paul Bolle wrote:
> Just one nit: a license mismatch.
>
> On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:
>> --- /dev/null
>> +++ b/drivers/mailbox/scpi_protocol.c
>
>> + * 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/>.
>
> This states the license is GPL v2.
>
>> +MODULE_LICENSE("GPL");
>
> And, according to include/linux/module.h, this states the license is GPL
> v2 or later. So I think either the comment at the top of this file or
> the license ident used in the MODULE_LICENSE() macro should be changed.
>
> Likewise for 2/4 and 4/4.
Thanks for pointing this out. Will fix in the next version.
Regards,
Sudeep
Hi Tixy,
On 27/04/15 19:11, Jon Medhurst (Tixy) wrote:
> On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:
>> Hi,
>>
>> This patch series adds support for:
>> 1. SCPI(System Control and Power Interface) mailbox protocol
>> driver.
>
> Is there a public document with the final protocol specification?
> I have spotted an inconsistency between driver and a draft doc I have
> but before commenting on things like that it would be good to have the
> final version of the document instead.
>
Thanks for having a look at the driver. I am already chasing this up
with the concerned team to check when will this be made public. IIUC you
do have a non-confidential (preliminary)version of document. So you can
comment on the inconsistency you have observed and I can check if it's
issue with the code or the document.
Regards,
Sudeep
On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:
> This patch adds support for System Control and Power Interface (SCPI)
> Message Protocol used between the Application Cores(AP) and the System
> Control Processor(SCP). The MHU peripheral provides a mechanism for
> inter-processor communication between SCP's M3 processor 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.
>
> This protocol driver provides interface for all the client drivers using
> SCPI to make use of the features offered by the SCP.
>
> Signed-off-by: Sudeep Holla <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Mark Rutland <[email protected]>
> CC: Jassi Brar <[email protected]>
> Cc: Liviu Dudau <[email protected]>
> Cc: Lorenzo Pieralisi <[email protected]>
> Cc: Jon Medhurst (Tixy) <[email protected]>
> Cc: [email protected]
> ---
There are several spelling errors but I won't point out each, sure you
can find them with a spellcheck ;-) I'll just comment on the code...
[...]
> +++ b/drivers/mailbox/scpi_protocol.c
> @@ -0,0 +1,694 @@
> +/*
> + * 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) 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/>.
> + */
> +
> +#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/spinlock.h>
> +
> +#define CMD_ID_SHIFT 0
> +#define CMD_ID_MASK 0x7f
> +#define CMD_TOKEN_ID_SHIFT 8
> +#define CMD_TOKEN_ID_MASK 0xff
> +#define CMD_DATA_SIZE_SHIFT 16
> +#define CMD_DATA_SIZE_MASK 0x1ff
> +#define PACK_SCPI_CMD(cmd_id, token, tx_sz) \
> + ((((cmd_id) & CMD_ID_MASK) << CMD_ID_SHIFT) | \
> + (((token) & CMD_TOKEN_ID_MASK) << CMD_TOKEN_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 SCPI_SLOT 0
> +
> +#define MAX_DVFS_DOMAINS 8
> +#define MAX_DVFS_OPPS 8
> +#define DVFS_LATENCY(hdr) (le32_to_cpu(hdr) >> 16)
> +#define DVFS_OPP_COUNT(hdr) ((le32_to_cpu(hdr) >> 8) & 0xff)
> +
> +#define PROTOCOL_REV_MINOR_BITS 16
> +#define PROTOCOL_REV_MINOR_MASK ((1U << PROTOCOL_REV_MINOR_BITS) - 1)
> +#define PROTOCOL_REV_MAJOR(x) ((x) >> PROTOCOL_REV_MINOR_BITS)
> +#define PROTOCOL_REV_MINOR(x) ((x) & PROTOCOL_REV_MINOR_MASK)
> +
> +#define FW_REV_MAJOR_BITS 24
> +#define FW_REV_MINOR_BITS 16
> +#define FW_REV_PATCH_MASK ((1U << FW_REV_MINOR_BITS) - 1)
> +#define FW_REV_MINOR_MASK ((1U << FW_REV_MAJOR_BITS) - 1)
> +#define FW_REV_MAJOR(x) ((x) >> FW_REV_MAJOR_BITS)
> +#define FW_REV_MINOR(x) (((x) & FW_REV_MINOR_MASK) >> FW_REV_MINOR_BITS)
> +#define FW_REV_PATCH(x) ((x) & FW_REV_PATCH_MASK)
> +
> +#define MAX_RX_TIMEOUT (msecs_to_jiffies(30))
> +
> +enum 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 scpi_std_cmd {
> + SCPI_CMD_INVALID = 0x00,
> + SCPI_CMD_SCPI_READY = 0x01,
> + SCPI_CMD_SCPI_CAPABILITIES = 0x02,
> + SCPI_CMD_SET_CSS_PWR_STATE = 0x03,
> + SCPI_CMD_GET_CSS_PWR_STATE = 0x04,
> + SCPI_CMD_SET_SYS_PWR_STATE = 0x05,
> + SCPI_CMD_SET_CPU_TIMER = 0x06,
> + SCPI_CMD_CANCEL_CPU_TIMER = 0x07,
> + SCPI_CMD_DVFS_CAPABILITIES = 0x08,
> + SCPI_CMD_GET_DVFS_INFO = 0x09,
> + SCPI_CMD_SET_DVFS = 0x0a,
> + SCPI_CMD_GET_DVFS = 0x0b,
> + SCPI_CMD_GET_DVFS_STAT = 0x0c,
> + SCPI_CMD_CLOCK_CAPABILITIES = 0x0d,
> + SCPI_CMD_GET_CLOCK_INFO = 0x0e,
> + SCPI_CMD_SET_CLOCK_VALUE = 0x0f,
> + SCPI_CMD_GET_CLOCK_VALUE = 0x10,
> + SCPI_CMD_PSU_CAPABILITIES = 0x11,
> + SCPI_CMD_GET_PSU_INFO = 0x12,
> + SCPI_CMD_SET_PSU = 0x13,
> + SCPI_CMD_GET_PSU = 0x14,
> + SCPI_CMD_SENSOR_CAPABILITIES = 0x15,
> + SCPI_CMD_SENSOR_INFO = 0x16,
> + SCPI_CMD_SENSOR_VALUE = 0x17,
> + SCPI_CMD_SENSOR_CFG_PERIODIC = 0x18,
> + SCPI_CMD_SENSOR_CFG_BOUNDS = 0x19,
> + SCPI_CMD_SENSOR_ASYNC_VALUE = 0x1a,
> + SCPI_CMD_SET_DEVICE_PWR_STATE = 0x1b,
> + SCPI_CMD_GET_DEVICE_PWR_STATE = 0x1c,
> + SCPI_CMD_COUNT
> +};
> +
> +struct scpi_xfer {
> + u32 slot; /* has to be first element */
> + u32 cmd;
> + u32 status;
> + const void *tx_buf;
> + void *rx_buf;
> + unsigned int tx_len;
> + struct list_head node;
> + struct completion done;
> +};
> +
> +struct scpi_chan {
> + struct mbox_client cl;
> + struct mbox_chan *chan;
> + void __iomem *tx_payload;
> + void __iomem *rx_payload;
> + struct list_head rx_pending;
> + struct list_head xfers_list;
> + struct scpi_xfer *xfers;
> + spinlock_t rx_lock; /* locking for the rx pending list */
> + struct mutex xfers_lock;
> + atomic_t token;
> +};
> +
> +struct scpi_drvinfo {
> + u32 protocol_version;
> + 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];
> +};
> +
> +/*
> + * The SCP firmware only executes in little-endian mode, so any buffers
> + * shared through SCPI should have their contents converted to little-endian
> + */
> +struct scpi_shared_mem {
> + __le32 command;
> + __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 {
> + __le16 id;
> + __le16 reserved;
> + __le32 rate;
> +} __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;
> +
> +static struct scpi_drvinfo *scpi_info;
> +
> +static int 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 scpi_to_linux_errno(int errno)
> +{
> + if (errno >= SCPI_SUCCESS && errno < SCPI_ERR_MAX)
> + return scpi_linux_errmap[errno];
> + return -EIO;
> +}
> +
> +static void scpi_process_cmd(struct scpi_chan *ch, u32 cmd)
> +{
> + unsigned long flags;
> + struct scpi_xfer *t, *match = NULL;
> +
> + spin_lock_irqsave(&ch->rx_lock, flags);
> + if (list_empty(&ch->rx_pending)) {
> + spin_unlock_irqrestore(&ch->rx_lock, flags);
> + return;
> + }
> +
> + list_for_each_entry(t, &ch->rx_pending, node)
> + if (CMD_XTRACT_UNIQ(t->cmd) == CMD_XTRACT_UNIQ(cmd)) {
So if UNIQ here isn't actually unique amongst all pending requests, its
possible we'll pick the wrong one. There's a couple of scenarios where
that can happen, comments further down about that.
> + list_del(&t->node);
> + match = t;
> + break;
> + }
> + /* check if wait_for_completion is in progress or timed-out */
> + if (match && !completion_done(&match->done)) {
> + struct scpi_shared_mem *mem = ch->rx_payload;
> +
> + match->status = le32_to_cpu(mem->status);
> + memcpy_fromio(match->rx_buf, mem->payload, CMD_SIZE(cmd));
> + complete(&match->done);
> + }
> + spin_unlock_irqrestore(&ch->rx_lock, flags);
> +}
> +
> +static void scpi_handle_remote_msg(struct mbox_client *c, void *msg)
> +{
> + struct scpi_chan *ch = container_of(c, struct scpi_chan, cl);
> + struct scpi_shared_mem *mem = ch->rx_payload;
> + u32 cmd = le32_to_cpu(mem->command);
> +
> + scpi_process_cmd(ch, cmd);
> +}
> +
> +static void scpi_tx_prepare(struct mbox_client *c, void *msg)
> +{
> + unsigned long flags;
> + struct scpi_xfer *t = msg;
> + struct scpi_chan *ch = container_of(c, struct scpi_chan, cl);
> + struct scpi_shared_mem *mem = (struct scpi_shared_mem *)ch->tx_payload;
> +
> + mem->command = cpu_to_le32(t->cmd);
> + if (t->tx_buf)
> + memcpy_toio(mem->payload, t->tx_buf, t->tx_len);
> + if (t->rx_buf) {
> + spin_lock_irqsave(&ch->rx_lock, flags);
> + list_add_tail(&t->node, &ch->rx_pending);
> + spin_unlock_irqrestore(&ch->rx_lock, flags);
> + }
> +}
> +
> +static struct scpi_xfer *get_scpi_xfer(struct scpi_chan *ch)
> +{
> + struct scpi_xfer *t;
> +
> + mutex_lock(&ch->xfers_lock);
> + if (list_empty(&ch->xfers_list)) {
> + mutex_unlock(&ch->xfers_lock);
> + return NULL;
> + }
> + t = list_first_entry(&ch->xfers_list, struct scpi_xfer, node);
> + list_del(&t->node);
> + mutex_unlock(&ch->xfers_lock);
> + return t;
> +}
> +
> +static void put_scpi_xfer(struct scpi_xfer *t, struct scpi_chan *ch)
> +{
> + mutex_lock(&ch->xfers_lock);
> + list_add_tail(&t->node, &ch->xfers_list);
> + mutex_unlock(&ch->xfers_lock);
> +}
> +
> +static int
> +scpi_send_message(u8 cmd, void *tx_buf, unsigned int len, void *rx_buf)
> +{
So the caller doesn't specify the length of rx_buf, wouldn't this be a
good idea?
That way we could truncate data sent from the SCP which would prevent
buffer overruns due to buggy SCP or Linux code. It would also allow the
SCP message format to be extended in the future in a backwards
compatible way.
And we could zero fill any data that was too short to allow
compatibility with Linux drivers using any new extended format messages
on older SCP firmware. Or at least so any bugs behave more consistently
by seeing zeros instead of random data left over from old messages.
> + int ret;
> + u8 token, chan;
> + struct scpi_xfer *msg;
> + struct scpi_chan *scpi_chan;
> +
> + chan = atomic_inc_return(&scpi_info->next_chan) % scpi_info->num_chans;
> + scpi_chan = scpi_info->channels + chan;
> +
> + msg = get_scpi_xfer(scpi_chan);
> + if (!msg)
> + return -ENOMEM;
> +
> + token = atomic_inc_return(&scpi_chan->token) & CMD_TOKEN_ID_MASK;
So, this 8 bit token is what's used to 'uniquely' identify a pending
command. But as it's just an incrementing value, then if one command
gets delayed for long enough that 256 more are issued then we will have
a non-unique value and scpi_process_cmd can go wrong.
Note, this delay doesn't just have to be at the SCPI end. We could get
preempted here (?) before actually sending the command to the SCP and
other kernel threads or processes could send those other 256 commands
before we get to run again.
Wouldn't it be better instead to have scpi_alloc_xfer_list add a unique
number to each struct scpi_xfer.
> +
> + msg->slot = BIT(SCPI_SLOT);
> + msg->cmd = PACK_SCPI_CMD(cmd, token, len);
> + msg->tx_buf = tx_buf;
> + msg->tx_len = len;
> + msg->rx_buf = rx_buf;
> + init_completion(&msg->done);
> +
> + ret = mbox_send_message(scpi_chan->chan, msg);
> + if (ret < 0 || !rx_buf)
> + goto out;
> +
> + if (!wait_for_completion_timeout(&msg->done, MAX_RX_TIMEOUT))
> + ret = -ETIMEDOUT;
> + else
> + /* first status word */
> + ret = le32_to_cpu(msg->status);
> +out:
> + if (ret < 0 && rx_buf) /* remove entry from the list if timed-out */
So, even with my suggestion that the unique message identifies are
fixed values stored in struct scpi_xfer, we can still have the situation
where we timeout a request, that scpi_xfer then getting used for another
request, and finally the SCP completes the request that we timed out,
which has the same 'unique' value as the later one.
One way to handle that it to not have any timeout on requests and assume
the firmware isn't buggy.
Another way is to have something more closely approximating unique in
the message, e.g. a 64-bit incrementing count. I realise though that
ARM have already finished the spec so we're limited to 8-bits :-(
> + scpi_process_cmd(scpi_chan, msg->cmd);
> +
> + put_scpi_xfer(msg, scpi_chan);
> + /* SCPI error codes > 0, translate them to Linux scale*/
> + return ret > 0 ? scpi_to_linux_errno(ret) : ret;
> +}
> +
> +static u32 scpi_get_version(void)
> +{
> + return scpi_info->protocol_version;
> +}
> +
> +static int
> +scpi_clk_get_range(u16 clk_id, unsigned long *min, unsigned long *max)
> +{
> + int ret;
> + 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);
> + if (!ret) {
> + *min = le32_to_cpu(clk.min_rate);
> + *max = le32_to_cpu(clk.max_rate);
> + }
> + return ret;
> +}
> +
> +static unsigned long scpi_clk_get_val(u16 clk_id)
> +{
> + int ret;
> + 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);
> + return ret ? ret : le32_to_cpu(clk.rate);
> +}
> +
> +static int scpi_clk_set_val(u16 clk_id, unsigned long rate)
> +{
> + int stat;
> + struct clk_set_value clk = {
> + cpu_to_le16(clk_id), 0, cpu_to_le32(rate)
I know that '0' is what I suggested when I spotted the 'reserved' member
wasn't being allowed for, but I have since thought that the more robust
way of initialising structures here, and in other functions below,
might be with this syntax:
.id = cpu_to_le16(clk_id),
.rate = cpu_to_le32(rate)
> + };
> +
> + return scpi_send_message(SCPI_CMD_SET_CLOCK_VALUE,
> + &clk, sizeof(clk), &stat);
> +}
> +
> +static int scpi_dvfs_get_idx(u8 domain)
> +{
> + int ret;
> + struct dvfs_get dvfs;
> +
> + ret = scpi_send_message(SCPI_CMD_GET_DVFS,
> + &domain, sizeof(domain), &dvfs);
> + return ret ? ret : dvfs.index;
> +}
> +
> +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);
> +}
> +
> +static struct scpi_dvfs_info *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 (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);
> +
> + 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);
> + }
> +
> + scpi_info->dvfs[domain] = info;
> + return info;
> +}
> +
> +static struct scpi_ops scpi_ops = {
> + .get_version = scpi_get_version,
> + .clk_get_range = scpi_clk_get_range,
> + .clk_get_val = scpi_clk_get_val,
> + .clk_set_val = scpi_clk_set_val,
> + .dvfs_get_idx = scpi_dvfs_get_idx,
> + .dvfs_set_idx = scpi_dvfs_set_idx,
> + .dvfs_get_info = scpi_dvfs_get_info,
> +};
> +
> +struct scpi_ops *get_scpi_ops(void)
> +{
> + return scpi_info ? scpi_info->scpi_ops : NULL;
> +}
> +EXPORT_SYMBOL_GPL(get_scpi_ops);
I'm curious to know why the interface for users of this driver is an
array of function pointers rather than just exporting functions
statically. The pointer array would be the sort of thing you'd do if
there were more than one possible provider of this interface in the
kernel, is this what we expect?
> +
> +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);
> + if (!ret) {
> + info->protocol_version = le32_to_cpu(caps.protocol_version);
> + info->firmware_version = le32_to_cpu(caps.platform_version);
> + }
> + return ret;
> +}
> +
> +static ssize_t protocol_version_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct scpi_drvinfo *scpi_info = dev_get_drvdata(dev);
> +
> + return sprintf(buf, "%d.%d\n",
> + PROTOCOL_REV_MAJOR(scpi_info->protocol_version),
> + PROTOCOL_REV_MINOR(scpi_info->protocol_version));
> +}
> +static DEVICE_ATTR_RO(protocol_version);
> +
> +static ssize_t firmware_version_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct scpi_drvinfo *scpi_info = dev_get_drvdata(dev);
> +
> + return sprintf(buf, "%d.%d.%d\n",
> + FW_REV_MAJOR(scpi_info->firmware_version),
> + FW_REV_MINOR(scpi_info->firmware_version),
> + FW_REV_PATCH(scpi_info->firmware_version));
> +}
> +static DEVICE_ATTR_RO(firmware_version);
> +
> +static struct attribute *versions_attrs[] = {
> + &dev_attr_firmware_version.attr,
> + &dev_attr_protocol_version.attr,
> + NULL,
> +};
> +ATTRIBUTE_GROUPS(versions);
> +
> +static void
> +scpi_free_channels(struct device *dev, struct scpi_chan *pchan, int count)
> +{
> + int i;
> +
> + for (i = 0; i < count && pchan->chan; i++, pchan++) {
> + mbox_free_channel(pchan->chan);
> + devm_kfree(dev, pchan->xfers);
> + devm_iounmap(dev, pchan->rx_payload);
> + }
> +}
> +
> +static int scpi_remove(struct platform_device *pdev)
> +{
> + int i;
> + struct device *dev = &pdev->dev;
> + struct scpi_drvinfo *info = platform_get_drvdata(pdev);
> +
> + scpi_info = NULL; /* stop exporting SCPI ops through get_scpi_ops */
> +
> + of_platform_depopulate(dev);
> + sysfs_remove_groups(&dev->kobj, versions_groups);
> + 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]);
> + }
> + devm_kfree(dev, info->channels);
> + devm_kfree(dev, info);
> +
> + return 0;
> +}
> +
> +#define MAX_SCPI_XFERS 10
> +static int scpi_alloc_xfer_list(struct device *dev, struct scpi_chan *ch)
> +{
> + int i;
> + struct scpi_xfer *xfers;
> +
> + xfers = devm_kzalloc(dev, MAX_SCPI_XFERS * sizeof(*xfers), GFP_KERNEL);
> + if (!xfers)
> + return -ENOMEM;
> +
> + ch->xfers = xfers;
> + for (i = 0; i < MAX_SCPI_XFERS; i++, xfers++)
> + list_add_tail(&xfers->node, &ch->xfers_list);
> + return 0;
> +}
> +
> +static int scpi_probe(struct platform_device *pdev)
> +{
> + int count, idx, ret;
> + struct resource res;
> + struct scpi_chan *scpi_chan;
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> +
> + scpi_info = devm_kzalloc(dev, sizeof(*scpi_info), GFP_KERNEL);
> + if (!scpi_info) {
> + dev_err(dev, "failed to allocate memory for scpi drvinfo\n");
> + 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;
> + }
> +
> + scpi_chan = devm_kcalloc(dev, count, sizeof(*scpi_chan), GFP_KERNEL);
> + if (!scpi_chan) {
> + dev_err(dev, "failed to allocate memory scpi chaninfo\n");
> + return -ENOMEM;
> + }
> +
> + for (idx = 0; idx < count; idx++) {
> + resource_size_t size;
> + struct scpi_chan *pchan = 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);
> +
> + cl->dev = dev;
> + cl->rx_callback = scpi_handle_remote_msg;
> + cl->tx_prepare = scpi_tx_prepare;
> + cl->tx_block = true;
> + cl->tx_tout = 50;
> + cl->knows_txdone = false; /* controller can ack */
> +
> + INIT_LIST_HEAD(&pchan->rx_pending);
> + INIT_LIST_HEAD(&pchan->xfers_list);
> + spin_lock_init(&pchan->rx_lock);
> + mutex_init(&pchan->xfers_lock);
> +
> + ret = scpi_alloc_xfer_list(dev, pchan);
> + if (!ret) {
> + pchan->chan = mbox_request_channel(cl, idx);
> + if (!IS_ERR(pchan->chan))
> + continue;
> + ret = -EPROBE_DEFER;
> + dev_err(dev, "failed to acquire channel#%d\n", idx);
> + }
> +err:
> + scpi_free_channels(dev, scpi_chan, idx);
Think we need to add one to 'idx' above, otherwise we won't free up
resources we successfully allocated to the current channel before we got
the error.
Actually, we also fail to free scpi_chan and scpi_info, so should we not
just call scpi_remove here instead? (Would require some tweaks as
scpi_info and drvdata aren't set until a bit further down.)
> + scpi_info = NULL;
> + return ret;
> + }
> +
> + scpi_info->channels = scpi_chan;
> + scpi_info->num_chans = count;
> + platform_set_drvdata(pdev, scpi_info);
> +
> + ret = scpi_init_versions(scpi_info);
> + if (ret) {
> + dev_err(dev, "incorrect or no SCP firmware found\n");
> + scpi_remove(pdev);
> + return ret;
> + }
> +
> + _dev_info(dev, "SCP Protocol %d.%d Firmware %d.%d.%d version\n",
> + PROTOCOL_REV_MAJOR(scpi_info->protocol_version),
> + PROTOCOL_REV_MINOR(scpi_info->protocol_version),
> + 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 = sysfs_create_groups(&dev->kobj, versions_groups);
> + if (ret)
> + dev_err(dev, "unable to create sysfs version group\n");
> +
> + return of_platform_populate(dev->of_node, NULL, NULL, dev);
> +}
> +
> +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",
> + .of_match_table = scpi_of_match,
> + },
> + .probe = scpi_probe,
> + .remove = scpi_remove,
> +};
> +module_platform_driver(scpi_driver);
> +
> +MODULE_AUTHOR("Sudeep Holla <[email protected]>");
> +MODULE_DESCRIPTION("ARM SCPI mailbox protocol driver");
> +MODULE_LICENSE("GPL");
[...]
--
Tixy
On 27/04/15 19:11, Jon Medhurst (Tixy) wrote:
> On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:
>> Hi,
>>
>> This patch series adds support for:
>> 1. SCPI(System Control and Power Interface) mailbox protocol
>> driver.
>
> Is there a public document with the final protocol specification?
> I have spotted an inconsistency between driver and a draft doc I have
> but before commenting on things like that it would be good to have the
> final version of the document instead.
>
And finally it's hosted in public. It can be accessed @[1] or [2]
Regards,
Sudeep
[1]
http://community.arm.com/servlet/JiveServlet/download/8401-40-18262/DUI0922A_scp_message_interface.pdf
[2]
https://wiki.linaro.org/ARM/Juno?action=AttachFile&do=get&target=DUI0922A_scp_message_interface.pdf
On 27 April 2015 at 17:10, Sudeep Holla <[email protected]> wrote:
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 4f3dbc8cf729..9e678bf1687c 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -24,6 +24,15 @@ config ARM_VEXPRESS_SPC_CPUFREQ
> This add the CPUfreq driver support for Versatile Express
> big.LITTLE platforms using SPC for power management.
>
> +config ARM_SCPI_CPUFREQ
> + tristate "SCPI based CPUfreq driver"
> + depends on ARM_BIG_LITTLE_CPUFREQ && ARM_SCPI_PROTOCOL
> + help
> + This add the CPUfreq driver support for ARM big.LITTLE platforms
> + using SCPI interface for CPU power management.
> +
> + This driver works only if firmware the supporting CPU DVFS adhere
> + to SCPI protocol.
Wanna reword that ?
>
> config ARM_EXYNOS_CPUFREQ
> tristate "SAMSUNG EXYNOS CPUfreq Driver"
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index cdce92ae2e8b..02fc9f849d4b 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -79,6 +79,7 @@ obj-$(CONFIG_ARM_SA1110_CPUFREQ) += sa1110-cpufreq.o
> obj-$(CONFIG_ARM_SPEAR_CPUFREQ) += spear-cpufreq.o
> obj-$(CONFIG_ARM_TEGRA_CPUFREQ) += tegra-cpufreq.o
> obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
> +obj-$(CONFIG_ARM_SCPI_CPUFREQ) += scpi-cpufreq.o
>
> ##################################################################################
> # PowerPC platform drivers
> diff --git a/drivers/cpufreq/scpi-cpufreq.c b/drivers/cpufreq/scpi-cpufreq.c
> new file mode 100644
> index 000000000000..4c2c11a9dfc6
> --- /dev/null
> +++ b/drivers/cpufreq/scpi-cpufreq.c
> @@ -0,0 +1,103 @@
> +/*
> + * SCPI CPUFreq Interface driver
> + *
> + * It provides necessary ops to arm_big_little cpufreq driver.
> + *
> + * Copyright (C) 2015 ARM Ltd.
> + * Sudeep Holla <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/cpufreq.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
> +#include <linux/scpi_protocol.h>
> +#include <linux/types.h>
> +
> +#include "arm_big_little.h"
> +
> +static struct scpi_ops *scpi_ops;
> +
> +static int scpi_init_opp_table(struct device *cpu_dev)
> +{
> + u8 domain = topology_physical_package_id(cpu_dev->id);
> + struct scpi_dvfs_info *info;
> + struct scpi_opp *opp;
> + int idx, ret = 0;
> +
> + if ((dev_pm_opp_get_opp_count(cpu_dev)) > 0)
> + return 0;
Why, who would have added it ?
> + info = scpi_ops->dvfs_get_info(domain);
Isn't calling this twice costly for getting the same information ?
> + if (IS_ERR(info))
> + return PTR_ERR(info);
> +
> + opp = info->opps;
> + if (!opp)
> + return -EIO;
> +
> + for (idx = 0; idx < info->count; idx++, opp++) {
> + ret = dev_pm_opp_add(cpu_dev, opp->freq, opp->m_volt * 1000);
> + if (ret) {
> + dev_warn(cpu_dev, "failed to add opp %uHz %umV\n",
> + opp->freq, opp->m_volt);
Don't you want to free earlier OPPs here ?
> + return ret;
> + }
> + }
> + return ret;
> +}
> +
> +static int scpi_get_transition_latency(struct device *cpu_dev)
> +{
> + u8 domain = topology_physical_package_id(cpu_dev->id);
> + struct scpi_dvfs_info *info;
> +
> + info = scpi_ops->dvfs_get_info(domain);
> + if (IS_ERR(info))
> + return PTR_ERR(info);
> +
> + return info->latency;
> +}
> +
> +static struct cpufreq_arm_bL_ops scpi_cpufreq_ops = {
> + .name = "scpi",
> + .get_transition_latency = scpi_get_transition_latency,
> + .init_opp_table = scpi_init_opp_table,
Don't want to free/remove OPPs ?
> +};
> +
> +static int scpi_cpufreq_probe(struct platform_device *pdev)
> +{
> + scpi_ops = get_scpi_ops();
> + if (!scpi_ops)
> + return -EIO;
> +
> + return bL_cpufreq_register(&scpi_cpufreq_ops);
> +}
> +
> +static int scpi_cpufreq_remove(struct platform_device *pdev)
> +{
> + bL_cpufreq_unregister(&scpi_cpufreq_ops);
> + return 0;
> +}
> +
> +static struct platform_driver scpi_cpufreq_platdrv = {
> + .driver = {
> + .name = "scpi-cpufreq",
> + .owner = THIS_MODULE,
> + },
> + .probe = scpi_cpufreq_probe,
> + .remove = scpi_cpufreq_remove,
> +};
> +module_platform_driver(scpi_cpufreq_platdrv);
> +
> +MODULE_LICENSE("GPL");
GPL V2 ?
Author/Description missing ..
> --
> 1.9.1
>
Hi Viresh,
Thanks for the review.
On 29/04/15 06:44, Viresh Kumar wrote:
> On 27 April 2015 at 17:10, Sudeep Holla <[email protected]> wrote:
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index 4f3dbc8cf729..9e678bf1687c 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -24,6 +24,15 @@ config ARM_VEXPRESS_SPC_CPUFREQ
>> This add the CPUfreq driver support for Versatile Express
>> big.LITTLE platforms using SPC for power management.
>>
>> +config ARM_SCPI_CPUFREQ
>> + tristate "SCPI based CPUfreq driver"
>> + depends on ARM_BIG_LITTLE_CPUFREQ && ARM_SCPI_PROTOCOL
>> + help
>> + This add the CPUfreq driver support for ARM big.LITTLE platforms
>> + using SCPI interface for CPU power management.
>> +
>> + This driver works only if firmware the supporting CPU DVFS adhere
>> + to SCPI protocol.
>
> Wanna reword that ?
>
Ok
[...]
>> +static int scpi_init_opp_table(struct device *cpu_dev)
>> +{
>> + u8 domain = topology_physical_package_id(cpu_dev->id);
>> + struct scpi_dvfs_info *info;
>> + struct scpi_opp *opp;
>> + int idx, ret = 0;
>> +
>> + if ((dev_pm_opp_get_opp_count(cpu_dev)) > 0)
>> + return 0;
>
> Why, who would have added it ?
>
IIRC, it was added to prevent spurious duplicate OPP addition messages
during CPU hotplug. I will check it again.
>> + info = scpi_ops->dvfs_get_info(domain);
>
> Isn't calling this twice costly for getting the same information ?
>
No the SCPI protocol saves them and return just the pointer if it's
already populated.
>> + if (IS_ERR(info))
>> + return PTR_ERR(info);
>> +
>> + opp = info->opps;
>> + if (!opp)
>> + return -EIO;
>> +
>> + for (idx = 0; idx < info->count; idx++, opp++) {
>> + ret = dev_pm_opp_add(cpu_dev, opp->freq, opp->m_volt * 1000);
>> + if (ret) {
>> + dev_warn(cpu_dev, "failed to add opp %uHz %umV\n",
>> + opp->freq, opp->m_volt);
>
> Don't you want to free earlier OPPs here ?
>
Make sense will fix.
>> +static struct cpufreq_arm_bL_ops scpi_cpufreq_ops = {
>> + .name = "scpi",
>> + .get_transition_latency = scpi_get_transition_latency,
>> + .init_opp_table = scpi_init_opp_table,
>
> Don't want to free/remove OPPs ?
>
Ah I see a new function is added, will fix it. In-fact this driver was
written before that and was held up since the firmware was not stable.
[...]
>> +static struct platform_driver scpi_cpufreq_platdrv = {
>> + .driver = {
>> + .name = "scpi-cpufreq",
>> + .owner = THIS_MODULE,
>> + },
>> + .probe = scpi_cpufreq_probe,
>> + .remove = scpi_cpufreq_remove,
>> +};
>> +module_platform_driver(scpi_cpufreq_platdrv);
>> +
>> +MODULE_LICENSE("GPL");
>
> GPL V2 ?
>
> Author/Description missing ..
>
Will fix it in next version.
Regards,
Sudeep
On 28/04/15 14:54, Jon Medhurst (Tixy) wrote:
> On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:
>> This patch adds support for System Control and Power Interface (SCPI)
>> Message Protocol used between the Application Cores(AP) and the System
>> Control Processor(SCP). The MHU peripheral provides a mechanism for
>> inter-processor communication between SCP's M3 processor 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.
>>
>> This protocol driver provides interface for all the client drivers using
>> SCPI to make use of the features offered by the SCP.
>>
>> Signed-off-by: Sudeep Holla <[email protected]>
>> Cc: Rob Herring <[email protected]>
>> Cc: Mark Rutland <[email protected]>
>> CC: Jassi Brar <[email protected]>
>> Cc: Liviu Dudau <[email protected]>
>> Cc: Lorenzo Pieralisi <[email protected]>
>> Cc: Jon Medhurst (Tixy) <[email protected]>
>> Cc: [email protected]
>> ---
>
> There are several spelling errors but I won't point out each, sure you
> can find them with a spellcheck ;-) I'll just comment on the code...
>
OK :)
> [...]
>> +++ b/drivers/mailbox/scpi_protocol.c
>> @@ -0,0 +1,694 @@
>> +/*
>> + * 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) 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/>.
>> + */
>> +
[...]
>> +
>> +static inline int scpi_to_linux_errno(int errno)
>> +{
>> + if (errno >= SCPI_SUCCESS && errno < SCPI_ERR_MAX)
>> + return scpi_linux_errmap[errno];
>> + return -EIO;
>> +}
>> +
>> +static void scpi_process_cmd(struct scpi_chan *ch, u32 cmd)
>> +{
>> + unsigned long flags;
>> + struct scpi_xfer *t, *match = NULL;
>> +
>> + spin_lock_irqsave(&ch->rx_lock, flags);
>> + if (list_empty(&ch->rx_pending)) {
>> + spin_unlock_irqrestore(&ch->rx_lock, flags);
>> + return;
>> + }
>> +
>> + list_for_each_entry(t, &ch->rx_pending, node)
>> + if (CMD_XTRACT_UNIQ(t->cmd) == CMD_XTRACT_UNIQ(cmd)) {
>
> So if UNIQ here isn't actually unique amongst all pending requests, its
> possible we'll pick the wrong one. There's a couple of scenarios where
> that can happen, comments further down about that.
>
>> + list_del(&t->node);
>> + match = t;
>> + break;
>> + }
>> + /* check if wait_for_completion is in progress or timed-out */
>> + if (match && !completion_done(&match->done)) {
>> + struct scpi_shared_mem *mem = ch->rx_payload;
>> +
>> + match->status = le32_to_cpu(mem->status);
>> + memcpy_fromio(match->rx_buf, mem->payload, CMD_SIZE(cmd));
>> + complete(&match->done);
>> + }
>> + spin_unlock_irqrestore(&ch->rx_lock, flags);
>> +}
>> +
>> +static void scpi_handle_remote_msg(struct mbox_client *c, void *msg)
>> +{
>> + struct scpi_chan *ch = container_of(c, struct scpi_chan, cl);
>> + struct scpi_shared_mem *mem = ch->rx_payload;
>> + u32 cmd = le32_to_cpu(mem->command);
>> +
>> + scpi_process_cmd(ch, cmd);
>> +}
>> +
>> +static void scpi_tx_prepare(struct mbox_client *c, void *msg)
>> +{
>> + unsigned long flags;
>> + struct scpi_xfer *t = msg;
>> + struct scpi_chan *ch = container_of(c, struct scpi_chan, cl);
>> + struct scpi_shared_mem *mem = (struct scpi_shared_mem *)ch->tx_payload;
>> +
>> + mem->command = cpu_to_le32(t->cmd);
>> + if (t->tx_buf)
>> + memcpy_toio(mem->payload, t->tx_buf, t->tx_len);
>> + if (t->rx_buf) {
>> + spin_lock_irqsave(&ch->rx_lock, flags);
>> + list_add_tail(&t->node, &ch->rx_pending);
>> + spin_unlock_irqrestore(&ch->rx_lock, flags);
>> + }
>> +}
>> +
>> +static struct scpi_xfer *get_scpi_xfer(struct scpi_chan *ch)
>> +{
>> + struct scpi_xfer *t;
>> +
>> + mutex_lock(&ch->xfers_lock);
>> + if (list_empty(&ch->xfers_list)) {
>> + mutex_unlock(&ch->xfers_lock);
>> + return NULL;
>> + }
>> + t = list_first_entry(&ch->xfers_list, struct scpi_xfer, node);
>> + list_del(&t->node);
>> + mutex_unlock(&ch->xfers_lock);
>> + return t;
>> +}
>> +
>> +static void put_scpi_xfer(struct scpi_xfer *t, struct scpi_chan *ch)
>> +{
>> + mutex_lock(&ch->xfers_lock);
>> + list_add_tail(&t->node, &ch->xfers_list);
>> + mutex_unlock(&ch->xfers_lock);
>> +}
>> +
>> +static int
>> +scpi_send_message(u8 cmd, void *tx_buf, unsigned int len, void *rx_buf)
>> +{
>
> So the caller doesn't specify the length of rx_buf, wouldn't this be a
> good idea?
>
> That way we could truncate data sent from the SCP which would prevent
> buffer overruns due to buggy SCP or Linux code. It would also allow the
> SCP message format to be extended in the future in a backwards
> compatible way.
>
> And we could zero fill any data that was too short to allow
> compatibility with Linux drivers using any new extended format messages
> on older SCP firmware. Or at least so any bugs behave more consistently
> by seeing zeros instead of random data left over from old messages.
>
Make sense, will add len in next version.
>> + int ret;
>> + u8 token, chan;
>> + struct scpi_xfer *msg;
>> + struct scpi_chan *scpi_chan;
>> +
>> + chan = atomic_inc_return(&scpi_info->next_chan) % scpi_info->num_chans;
>> + scpi_chan = scpi_info->channels + chan;
>> +
>> + msg = get_scpi_xfer(scpi_chan);
>> + if (!msg)
>> + return -ENOMEM;
>> +
>> + token = atomic_inc_return(&scpi_chan->token) & CMD_TOKEN_ID_MASK;
>
> So, this 8 bit token is what's used to 'uniquely' identify a pending
> command. But as it's just an incrementing value, then if one command
> gets delayed for long enough that 256 more are issued then we will have
> a non-unique value and scpi_process_cmd can go wrong.
>
IMO by the time 256 message are queued up and serviced we would timeout
on the initial command. Moreover the core mailbox has sent the mailbox
length to 20(MBOX_TX_QUEUE_LEN) which needs to removed to even get the
remote chance of hit the corner case.
> Note, this delay doesn't just have to be at the SCPI end. We could get
> preempted here (?) before actually sending the command to the SCP and
> other kernel threads or processes could send those other 256 commands
> before we get to run again.
>
Agreed, but we would still timeout after 3 jiffies max.
> Wouldn't it be better instead to have scpi_alloc_xfer_list add a unique
> number to each struct scpi_xfer.
>
One of reason using it part of command is that SCP gives it back in the
response to compare.
>> +
>> + msg->slot = BIT(SCPI_SLOT);
>> + msg->cmd = PACK_SCPI_CMD(cmd, token, len);
>> + msg->tx_buf = tx_buf;
>> + msg->tx_len = len;
>> + msg->rx_buf = rx_buf;
>> + init_completion(&msg->done);
>> +
>> + ret = mbox_send_message(scpi_chan->chan, msg);
>> + if (ret < 0 || !rx_buf)
>> + goto out;
>> +
>> + if (!wait_for_completion_timeout(&msg->done, MAX_RX_TIMEOUT))
>> + ret = -ETIMEDOUT;
>> + else
>> + /* first status word */
>> + ret = le32_to_cpu(msg->status);
>> +out:
>> + if (ret < 0 && rx_buf) /* remove entry from the list if timed-out */
>
> So, even with my suggestion that the unique message identifies are
> fixed values stored in struct scpi_xfer, we can still have the situation
> where we timeout a request, that scpi_xfer then getting used for another
> request, and finally the SCP completes the request that we timed out,
> which has the same 'unique' value as the later one.
>
As explained above I can't imagine hitting this condition. I will think
more on that again.
> One way to handle that it to not have any timeout on requests and assume
> the firmware isn't buggy.
>
That's something I can't do ;) based on my experience so far. It's good
to assume firmware *can be buggy* and handle all possible errors. Think
about the development firmware using this driver. This has been very
useful when I was testing the development versions. Even under stress
conditions I still see timeouts(very rarely though), so my personal
preference is to have them.
> Another way is to have something more closely approximating unique in
> the message, e.g. a 64-bit incrementing count. I realise though that
> ARM have already finished the spec so we're limited to 8-bits :-(
>
>> + scpi_process_cmd(scpi_chan, msg->cmd);
>> +
>> + put_scpi_xfer(msg, scpi_chan);
>> + /* SCPI error codes > 0, translate them to Linux scale*/
>> + return ret > 0 ? scpi_to_linux_errno(ret) : ret;
>> +}
>> +
>> +static u32 scpi_get_version(void)
>> +{
>> + return scpi_info->protocol_version;
>> +}
>> +
>> +static int
>> +scpi_clk_get_range(u16 clk_id, unsigned long *min, unsigned long *max)
>> +{
>> + int ret;
>> + 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);
>> + if (!ret) {
>> + *min = le32_to_cpu(clk.min_rate);
>> + *max = le32_to_cpu(clk.max_rate);
>> + }
>> + return ret;
>> +}
>> +
>> +static unsigned long scpi_clk_get_val(u16 clk_id)
>> +{
>> + int ret;
>> + 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);
>> + return ret ? ret : le32_to_cpu(clk.rate);
>> +}
>> +
>> +static int scpi_clk_set_val(u16 clk_id, unsigned long rate)
>> +{
>> + int stat;
>> + struct clk_set_value clk = {
>> + cpu_to_le16(clk_id), 0, cpu_to_le32(rate)
>
> I know that '0' is what I suggested when I spotted the 'reserved' member
> wasn't being allowed for, but I have since thought that the more robust
> way of initialising structures here, and in other functions below,
> might be with this syntax:
>
> .id = cpu_to_le16(clk_id),
> .rate = cpu_to_le32(rate)
>
Ok will update.
[...]
>> +static int scpi_probe(struct platform_device *pdev)
>> +{
>> + int count, idx, ret;
>> + struct resource res;
>> + struct scpi_chan *scpi_chan;
>> + struct device *dev = &pdev->dev;
>> + struct device_node *np = dev->of_node;
>> +
>> + scpi_info = devm_kzalloc(dev, sizeof(*scpi_info), GFP_KERNEL);
>> + if (!scpi_info) {
>> + dev_err(dev, "failed to allocate memory for scpi drvinfo\n");
>> + 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;
>> + }
>> +
>> + scpi_chan = devm_kcalloc(dev, count, sizeof(*scpi_chan), GFP_KERNEL);
>> + if (!scpi_chan) {
>> + dev_err(dev, "failed to allocate memory scpi chaninfo\n");
>> + return -ENOMEM;
>> + }
>> +
>> + for (idx = 0; idx < count; idx++) {
>> + resource_size_t size;
>> + struct scpi_chan *pchan = 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);
>> +
>> + cl->dev = dev;
>> + cl->rx_callback = scpi_handle_remote_msg;
>> + cl->tx_prepare = scpi_tx_prepare;
>> + cl->tx_block = true;
>> + cl->tx_tout = 50;
>> + cl->knows_txdone = false; /* controller can ack */
>> +
>> + INIT_LIST_HEAD(&pchan->rx_pending);
>> + INIT_LIST_HEAD(&pchan->xfers_list);
>> + spin_lock_init(&pchan->rx_lock);
>> + mutex_init(&pchan->xfers_lock);
>> +
>> + ret = scpi_alloc_xfer_list(dev, pchan);
>> + if (!ret) {
>> + pchan->chan = mbox_request_channel(cl, idx);
>> + if (!IS_ERR(pchan->chan))
>> + continue;
>> + ret = -EPROBE_DEFER;
>> + dev_err(dev, "failed to acquire channel#%d\n", idx);
>> + }
>> +err:
>> + scpi_free_channels(dev, scpi_chan, idx);
>
> Think we need to add one to 'idx' above, otherwise we won't free up
> resources we successfully allocated to the current channel before we got
> the error.
>
> Actually, we also fail to free scpi_chan and scpi_info, so should we not
> just call scpi_remove here instead? (Would require some tweaks as
> scpi_info and drvdata aren't set until a bit further down.)
>
Ok need to look at this again. Few thinks I left assuming devm_* will
handle it. I will also try to check if there's any memleaks.
Regards,
Sudeep
On Wed, 2015-04-29 at 11:53 +0100, Sudeep Holla wrote:
> On 28/04/15 14:54, Jon Medhurst (Tixy) wrote:
> > On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:
[...]
> >> + int ret;
> >> + u8 token, chan;
> >> + struct scpi_xfer *msg;
> >> + struct scpi_chan *scpi_chan;
> >> +
> >> + chan = atomic_inc_return(&scpi_info->next_chan) % scpi_info->num_chans;
> >> + scpi_chan = scpi_info->channels + chan;
> >> +
> >> + msg = get_scpi_xfer(scpi_chan);
> >> + if (!msg)
> >> + return -ENOMEM;
> >> +
> >> + token = atomic_inc_return(&scpi_chan->token) & CMD_TOKEN_ID_MASK;
> >
> > So, this 8 bit token is what's used to 'uniquely' identify a pending
> > command. But as it's just an incrementing value, then if one command
> > gets delayed for long enough that 256 more are issued then we will have
> > a non-unique value and scpi_process_cmd can go wrong.
> >
>
> IMO by the time 256 message are queued up and serviced we would timeout
> on the initial command. Moreover the core mailbox has sent the mailbox
> length to 20(MBOX_TX_QUEUE_LEN) which needs to removed to even get the
> remote chance of hit the corner case.
The corner case can be hit even if the queue length is only 2, because
other processes/cpus can use the other message we don't own here and
they can send then receive a message using that, 256 times. The corner
case doesn't require 256 simultaneous outstanding requests.
That is the reason I suggested that rather than using an incrementing
value for the 'unique' token, that each message instead contain the
value of the token to use with it.
>
> > Note, this delay doesn't just have to be at the SCPI end. We could get
> > preempted here (?) before actually sending the command to the SCP and
> > other kernel threads or processes could send those other 256 commands
> > before we get to run again.
> >
>
> Agreed, but we would still timeout after 3 jiffies max.
But we haven't started any timeout yet, the 3 jiffies won't start until
we get scheduled again and call wait_for_completion_timeout below.
>
> > Wouldn't it be better instead to have scpi_alloc_xfer_list add a unique
> > number to each struct scpi_xfer.
> >
>
> One of reason using it part of command is that SCP gives it back in the
> response to compare.
Can't we fill the token in the command from the value stored in the
struct scpi_xfer we are using to send that command?
> >> +
> >> + msg->slot = BIT(SCPI_SLOT);
> >> + msg->cmd = PACK_SCPI_CMD(cmd, token, len);
> >> + msg->tx_buf = tx_buf;
> >> + msg->tx_len = len;
> >> + msg->rx_buf = rx_buf;
> >> + init_completion(&msg->done);
> >> +
> >> + ret = mbox_send_message(scpi_chan->chan, msg);
> >> + if (ret < 0 || !rx_buf)
> >> + goto out;
> >> +
> >> + if (!wait_for_completion_timeout(&msg->done, MAX_RX_TIMEOUT))
> >> + ret = -ETIMEDOUT;
> >> + else
> >> + /* first status word */
> >> + ret = le32_to_cpu(msg->status);
> >> +out:
> >> + if (ret < 0 && rx_buf) /* remove entry from the list if timed-out */
> >
> > So, even with my suggestion that the unique message identifies are
> > fixed values stored in struct scpi_xfer, we can still have the situation
> > where we timeout a request, that scpi_xfer then getting used for another
> > request, and finally the SCP completes the request that we timed out,
> > which has the same 'unique' value as the later one.
> >
>
> As explained above I can't imagine hitting this condition. I will think
> more on that again.
I can imagine :-) If we timeout and discard messages, and reuse it's
unique id, there is always the possibility of this confusion occurring.
No amount of coding in the kernel can get around that. The only thing
you can do to get out of this quandary is make assumptions about how the
SCP firmware behaves.
>
> > One way to handle that it to not have any timeout on requests and assume
> > the firmware isn't buggy.
> >
>
> That's something I can't do ;) based on my experience so far. It's good
> to assume firmware *can be buggy* and handle all possible errors.
I'm inclined to agree.
> Think
> about the development firmware using this driver. This has been very
> useful when I was testing the development versions. Even under stress
> conditions I still see timeouts(very rarely though), so my personal
> preference is to have them.
But the SCPI protocol unfortunately doesn't seem to allow us to robustly
handle timeouts. Well, we could keep a list of tokens used in timed out
messages, and not reuse them. But if, as you say, timeouts do occur,
then with only 256 available, we are likely to run out.
When I brought this up 9 months ago, it was pointed out that the
limitation of an 8-bit token for a message because was because the
protocol designers had were cramming it into the 32-bit value poked into
the MHU register. The new finished protocol spec doesn't use the MHU
register any more for this data, but the limitations we're kept by
specifying the same command data format but just stored in the shared
memory. Pity the opportunity wasn't taken to expand the token size to
something that allowed more robust use.
--
Tixy
On Wed, 2015-04-29 at 12:43 +0100, Jon Medhurst (Tixy) wrote:
> On Wed, 2015-04-29 at 11:53 +0100, Sudeep Holla wrote:
> > On 28/04/15 14:54, Jon Medhurst (Tixy) wrote:
> > > On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:
> [...]
> > >> + int ret;
> > >> + u8 token, chan;
> > >> + struct scpi_xfer *msg;
> > >> + struct scpi_chan *scpi_chan;
> > >> +
> > >> + chan = atomic_inc_return(&scpi_info->next_chan) % scpi_info->num_chans;
> > >> + scpi_chan = scpi_info->channels + chan;
> > >> +
> > >> + msg = get_scpi_xfer(scpi_chan);
> > >> + if (!msg)
> > >> + return -ENOMEM;
> > >> +
> > >> + token = atomic_inc_return(&scpi_chan->token) & CMD_TOKEN_ID_MASK;
> > >
> > > So, this 8 bit token is what's used to 'uniquely' identify a pending
> > > command. But as it's just an incrementing value, then if one command
> > > gets delayed for long enough that 256 more are issued then we will have
> > > a non-unique value and scpi_process_cmd can go wrong.
> > >
> >
> > IMO by the time 256 message are queued up and serviced we would timeout
> > on the initial command. Moreover the core mailbox has sent the mailbox
> > length to 20(MBOX_TX_QUEUE_LEN) which needs to removed to even get the
> > remote chance of hit the corner case.
>
> The corner case can be hit even if the queue length is only 2, because
> other processes/cpus can use the other message we don't own here and
> they can send then receive a message using that, 256 times. The corner
> case doesn't require 256 simultaneous outstanding requests.
>
> That is the reason I suggested that rather than using an incrementing
> value for the 'unique' token, that each message instead contain the
> value of the token to use with it.
Of course, I failed to mention that this solution to this problem makes
thing worse for the situation where we timeout messages, because the
same token will get reused quicker in that case. So, in practice, if we
have timeouts, and a unchangeable protocol limitation of 256 tokens,
then using those tokens in order, for each message sent is probably the
best we can do.
Perhaps that's the clue, generate and add the token to the message, just
before transmission via the MHU, at a point where we know no other
request can overtake us. In scpi_tx_prepare? Perhaps, it might also be
good to only use up a token if we are expecting a response, and use zero
for other messages?
Something like this totally untested patch...
diff --git a/drivers/mailbox/scpi_protocol.c b/drivers/mailbox/scpi_protocol.c
index c74575b..5818d9b 100644
--- a/drivers/mailbox/scpi_protocol.c
+++ b/drivers/mailbox/scpi_protocol.c
@@ -286,14 +286,23 @@ static void scpi_tx_prepare(struct mbox_client *c, void *msg)
struct scpi_chan *ch = container_of(c, struct scpi_chan, cl);
struct scpi_shared_mem *mem = (struct scpi_shared_mem *)ch->tx_payload;
- mem->command = cpu_to_le32(t->cmd);
if (t->tx_buf)
memcpy_toio(mem->payload, t->tx_buf, t->tx_len);
if (t->rx_buf) {
+ int token;
spin_lock_irqsave(&ch->rx_lock, flags);
+ /*
+ * Presumably we can do this token setting outside
+ * spinlock and still be safe from concurrency?
+ */
+ do
+ token = (++ch->token) & CMD_TOKEN_ID_MASK;
+ while(!token);
+ t->cmd |= token << CMD_TOKEN_ID_SHIFT;
list_add_tail(&t->node, &ch->rx_pending);
spin_unlock_irqrestore(&ch->rx_lock, flags);
}
+ mem->command = cpu_to_le32(t->cmd);
}
static struct scpi_xfer *get_scpi_xfer(struct scpi_chan *ch)
@@ -322,7 +331,7 @@ static int
scpi_send_message(u8 cmd, void *tx_buf, unsigned int len, void *rx_buf)
{
int ret;
- u8 token, chan;
+ u8 chan;
struct scpi_xfer *msg;
struct scpi_chan *scpi_chan;
@@ -333,10 +342,8 @@ scpi_send_message(u8 cmd, void *tx_buf, unsigned int len, void *rx_buf)
if (!msg)
return -ENOMEM;
- token = atomic_inc_return(&scpi_chan->token) & CMD_TOKEN_ID_MASK;
-
msg->slot = BIT(SCPI_SLOT);
- msg->cmd = PACK_SCPI_CMD(cmd, token, len);
+ msg->cmd = PACK_SCPI_CMD(cmd, 0, len);
msg->tx_buf = tx_buf;
msg->tx_len = len;
msg->rx_buf = rx_buf;
--
Tixy
Hi Tixy,
On 29/04/15 12:43, Jon Medhurst (Tixy) wrote:
> On Wed, 2015-04-29 at 11:53 +0100, Sudeep Holla wrote:
>> On 28/04/15 14:54, Jon Medhurst (Tixy) wrote:
>>> On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:
> [...]
>>>> + int ret;
>>>> + u8 token, chan;
>>>> + struct scpi_xfer *msg;
>>>> + struct scpi_chan *scpi_chan;
>>>> +
>>>> + chan = atomic_inc_return(&scpi_info->next_chan) % scpi_info->num_chans;
>>>> + scpi_chan = scpi_info->channels + chan;
>>>> +
>>>> + msg = get_scpi_xfer(scpi_chan);
>>>> + if (!msg)
>>>> + return -ENOMEM;
>>>> +
>>>> + token = atomic_inc_return(&scpi_chan->token) & CMD_TOKEN_ID_MASK;
>>>
>>> So, this 8 bit token is what's used to 'uniquely' identify a pending
>>> command. But as it's just an incrementing value, then if one command
>>> gets delayed for long enough that 256 more are issued then we will have
>>> a non-unique value and scpi_process_cmd can go wrong.
>>>
>>
>> IMO by the time 256 message are queued up and serviced we would timeout
>> on the initial command. Moreover the core mailbox has sent the mailbox
>> length to 20(MBOX_TX_QUEUE_LEN) which needs to removed to even get the
>> remote chance of hit the corner case.
>
> The corner case can be hit even if the queue length is only 2, because
> other processes/cpus can use the other message we don't own here and
> they can send then receive a message using that, 256 times. The corner
> case doesn't require 256 simultaneous outstanding requests.
>
Good point, I missed it completely.
> That is the reason I suggested that rather than using an incrementing
> value for the 'unique' token, that each message instead contain the
> value of the token to use with it.
>
>>
>>> Note, this delay doesn't just have to be at the SCPI end. We could get
>>> preempted here (?) before actually sending the command to the SCP and
>>> other kernel threads or processes could send those other 256 commands
>>> before we get to run again.
>>>
>>
>> Agreed, but we would still timeout after 3 jiffies max.
>
> But we haven't started any timeout yet, the 3 jiffies won't start until
> we get scheduled again and call wait_for_completion_timeout below.
Agreed.
>>
>>> Wouldn't it be better instead to have scpi_alloc_xfer_list add a unique
>>> number to each struct scpi_xfer.
>>>
>>
>> One of reason using it part of command is that SCP gives it back in the
>> response to compare.
>
> Can't we fill the token in the command from the value stored in the
> struct scpi_xfer we are using to send that command?
>
Yes we can but 256 limitation still exists but solve some issues at-least.
>>>> +
>>>> + msg->slot = BIT(SCPI_SLOT);
>>>> + msg->cmd = PACK_SCPI_CMD(cmd, token, len);
>>>> + msg->tx_buf = tx_buf;
>>>> + msg->tx_len = len;
>>>> + msg->rx_buf = rx_buf;
>>>> + init_completion(&msg->done);
>>>> +
>>>> + ret = mbox_send_message(scpi_chan->chan, msg);
>>>> + if (ret < 0 || !rx_buf)
>>>> + goto out;
>>>> +
>>>> + if (!wait_for_completion_timeout(&msg->done, MAX_RX_TIMEOUT))
>>>> + ret = -ETIMEDOUT;
>>>> + else
>>>> + /* first status word */
>>>> + ret = le32_to_cpu(msg->status);
>>>> +out:
>>>> + if (ret < 0 && rx_buf) /* remove entry from the list if timed-out */
>>>
>>> So, even with my suggestion that the unique message identifies are
>>> fixed values stored in struct scpi_xfer, we can still have the situation
>>> where we timeout a request, that scpi_xfer then getting used for another
>>> request, and finally the SCP completes the request that we timed out,
>>> which has the same 'unique' value as the later one.
>>>
>>
>> As explained above I can't imagine hitting this condition. I will think
>> more on that again.
>
> I can imagine :-) If we timeout and discard messages, and reuse it's
> unique id, there is always the possibility of this confusion occurring.
> No amount of coding in the kernel can get around that. The only thing
> you can do to get out of this quandary is make assumptions about how the
> SCP firmware behaves.
>
Agreed again.
>>
>>> One way to handle that it to not have any timeout on requests and assume
>>> the firmware isn't buggy.
>>>
>>
>> That's something I can't do ;) based on my experience so far. It's good
>> to assume firmware *can be buggy* and handle all possible errors.
>
> I'm inclined to agree.
>
Thanks :)
>> Think
>> about the development firmware using this driver. This has been very
>> useful when I was testing the development versions. Even under stress
>> conditions I still see timeouts(very rarely though), so my personal
>> preference is to have them.
>
> But the SCPI protocol unfortunately doesn't seem to allow us to robustly
> handle timeouts. Well, we could keep a list of tokens used in timed out
> messages, and not reuse them. But if, as you say, timeouts do occur,
> then with only 256 available, we are likely to run out.
>
Yes :(
> When I brought this up 9 months ago, it was pointed out that the
> limitation of an 8-bit token for a message because was because the
> protocol designers had were cramming it into the 32-bit value poked into
> the MHU register. The new finished protocol spec doesn't use the MHU
> register any more for this data, but the limitations we're kept by
> specifying the same command data format but just stored in the shared
> memory. Pity the opportunity wasn't taken to expand the token size to
> something that allowed more robust use.
>
IMO may not be true, since the whole redesign was to align something
similar to ACPI PCC, they got influenced too much from it. Even that
has just 64-bit header and they tried to keep the same.
Regards,
Sudeep
On 29/04/15 13:25, Jon Medhurst (Tixy) wrote:
> On Wed, 2015-04-29 at 12:43 +0100, Jon Medhurst (Tixy) wrote:
>> On Wed, 2015-04-29 at 11:53 +0100, Sudeep Holla wrote:
>>> On 28/04/15 14:54, Jon Medhurst (Tixy) wrote:
>>>> On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:
>> [...]
>>>>> + int ret;
>>>>> + u8 token, chan;
>>>>> + struct scpi_xfer *msg;
>>>>> + struct scpi_chan *scpi_chan;
>>>>> +
>>>>> + chan = atomic_inc_return(&scpi_info->next_chan) % scpi_info->num_chans;
>>>>> + scpi_chan = scpi_info->channels + chan;
>>>>> +
>>>>> + msg = get_scpi_xfer(scpi_chan);
>>>>> + if (!msg)
>>>>> + return -ENOMEM;
>>>>> +
>>>>> + token = atomic_inc_return(&scpi_chan->token) & CMD_TOKEN_ID_MASK;
>>>>
>>>> So, this 8 bit token is what's used to 'uniquely' identify a pending
>>>> command. But as it's just an incrementing value, then if one command
>>>> gets delayed for long enough that 256 more are issued then we will have
>>>> a non-unique value and scpi_process_cmd can go wrong.
>>>>
>>>
>>> IMO by the time 256 message are queued up and serviced we would timeout
>>> on the initial command. Moreover the core mailbox has sent the mailbox
>>> length to 20(MBOX_TX_QUEUE_LEN) which needs to removed to even get the
>>> remote chance of hit the corner case.
>>
>> The corner case can be hit even if the queue length is only 2, because
>> other processes/cpus can use the other message we don't own here and
>> they can send then receive a message using that, 256 times. The corner
>> case doesn't require 256 simultaneous outstanding requests.
>>
>> That is the reason I suggested that rather than using an incrementing
>> value for the 'unique' token, that each message instead contain the
>> value of the token to use with it.
>
> Of course, I failed to mention that this solution to this problem makes
> thing worse for the situation where we timeout messages, because the
> same token will get reused quicker in that case. So, in practice, if we
> have timeouts, and a unchangeable protocol limitation of 256 tokens,
> then using those tokens in order, for each message sent is probably the
> best we can do.
>
I agree, I think we must be happy with that for now :)
> Perhaps that's the clue, generate and add the token to the message, just
> before transmission via the MHU, at a point where we know no other
> request can overtake us. In scpi_tx_prepare? Perhaps, it might also be
> good to only use up a token if we are expecting a response, and use zero
> for other messages?
>
> Something like this totally untested patch...
>
Looks good and best we can do with the limitations we have IMO
Regards,
Sudeep
On Wed, 2015-04-29 at 13:25 +0100, Jon Medhurst (Tixy) wrote:
> diff --git a/drivers/mailbox/scpi_protocol.c
> b/drivers/mailbox/scpi_protocol.c
> index c74575b..5818d9b 100644
> --- a/drivers/mailbox/scpi_protocol.c
> +++ b/drivers/mailbox/scpi_protocol.c
> @@ -286,14 +286,23 @@ static void scpi_tx_prepare(struct mbox_client
> *c, void *msg)
> struct scpi_chan *ch = container_of(c, struct scpi_chan, cl);
> struct scpi_shared_mem *mem = (struct scpi_shared_mem
> *)ch->tx_payload;
>
> - mem->command = cpu_to_le32(t->cmd);
> if (t->tx_buf)
> memcpy_toio(mem->payload, t->tx_buf, t->tx_len);
> if (t->rx_buf) {
> + int token;
> spin_lock_irqsave(&ch->rx_lock, flags);
> + /*
> + * Presumably we can do this token setting outside
> + * spinlock and still be safe from concurrency?
> + */
To answer my own question, yes, the four lines below can be moved up
above the spin_lock_irqsave. Because we had better be safe from
concurrency here as we are also writing to the channel's shared memory
area.
> + do
> + token = (++ch->token) & CMD_TOKEN_ID_MASK;
> + while(!token);
> + t->cmd |= token << CMD_TOKEN_ID_SHIFT;
> list_add_tail(&t->node, &ch->rx_pending);
> spin_unlock_irqrestore(&ch->rx_lock, flags);
> }
> + mem->command = cpu_to_le32(t->cmd);
> }
>
> static struct scpi_xfer *get_scpi_xfer(struct scpi_chan *ch)
--
Tixy
On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:
> On some ARM based systems, a separate Cortex-M based System Control
> Processor(SCP) provides the overall power, clock, reset and system
> control including CPU DVFS. SCPI Message Protocol is used to
> communicate with the SCPI.
>
> This patch adds a interface driver for adding OPPs and registering
> the arm_big_little cpufreq driver for such systems.
>
> Signed-off-by: Sudeep Holla <[email protected]>
> Cc: Viresh Kumar <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: [email protected]
> ---
> drivers/cpufreq/Kconfig.arm | 9 ++++
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/scpi-cpufreq.c | 103 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 113 insertions(+)
> create mode 100644 drivers/cpufreq/scpi-cpufreq.c
>
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 4f3dbc8cf729..9e678bf1687c 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -24,6 +24,15 @@ config ARM_VEXPRESS_SPC_CPUFREQ
> This add the CPUfreq driver support for Versatile Express
> big.LITTLE platforms using SPC for power management.
>
> +config ARM_SCPI_CPUFREQ
> + tristate "SCPI based CPUfreq driver"
> + depends on ARM_BIG_LITTLE_CPUFREQ && ARM_SCPI_PROTOCOL
And ARM_BIG_LITTLE_CPUFREQ depends on CONFIG_ARM, so we can't build this
for arm64, which is the only platform (Juno) we have to run these
patches on. Unless you're prepared for a horrid hack...
https://github.com/ARM-software/linux/commit/b9ceaa0cbd7c57d57ee7e69146cc627697570f6e
or a pair of less horrid ones...
http://git.linaro.org/kernel/linux-linaro-tracking.git/commit/4b5dd8ff98613b7e90c8f3214522a00ab6900fe9
http://git.linaro.org/kernel/linux-linaro-tracking.git/commit/8641dbfe253f6a9061742ca11f769fc1d82c2aaa
Any reason why the above two aren't suitable for mainline Linux? The
second was actually committed then reverted because it broke arm64
builds, but the other patch fixes that.
--
Tixy
On 01/05/15 14:19, Jon Medhurst (Tixy) wrote:
> On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:
>> On some ARM based systems, a separate Cortex-M based System Control
>> Processor(SCP) provides the overall power, clock, reset and system
>> control including CPU DVFS. SCPI Message Protocol is used to
>> communicate with the SCPI.
>>
>> This patch adds a interface driver for adding OPPs and registering
>> the arm_big_little cpufreq driver for such systems.
>>
>> Signed-off-by: Sudeep Holla <[email protected]>
>> Cc: Viresh Kumar <[email protected]>
>> Cc: "Rafael J. Wysocki" <[email protected]>
>> Cc: [email protected]
>> ---
>> drivers/cpufreq/Kconfig.arm | 9 ++++
>> drivers/cpufreq/Makefile | 1 +
>> drivers/cpufreq/scpi-cpufreq.c | 103 +++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 113 insertions(+)
>> create mode 100644 drivers/cpufreq/scpi-cpufreq.c
>>
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index 4f3dbc8cf729..9e678bf1687c 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -24,6 +24,15 @@ config ARM_VEXPRESS_SPC_CPUFREQ
>> This add the CPUfreq driver support for Versatile Express
>> big.LITTLE platforms using SPC for power management.
>>
>> +config ARM_SCPI_CPUFREQ
>> + tristate "SCPI based CPUfreq driver"
>> + depends on ARM_BIG_LITTLE_CPUFREQ && ARM_SCPI_PROTOCOL
>
> And ARM_BIG_LITTLE_CPUFREQ depends on CONFIG_ARM, so we can't build this
> for arm64, which is the only platform (Juno) we have to run these
> patches on.
Yes I know
> Unless you're prepared for a horrid hack...
> https://github.com/ARM-software/linux/commit/b9ceaa0cbd7c57d57ee7e69146cc627697570f6e
I need to check if it can be improved before I post on the list and
hence dropped it in the first version of this patch-set.
> or a pair of less horrid ones...
> http://git.linaro.org/kernel/linux-linaro-tracking.git/commit/4b5dd8ff98613b7e90c8f3214522a00ab6900fe9
No not this one, this was already discussed long back I believe.
It doesn't make sense to add bl-switcher for ARM64 IMO.
> http://git.linaro.org/kernel/linux-linaro-tracking.git/commit/8641dbfe253f6a9061742ca11f769fc1d82c2aaa
>
Yes the Kconfig changes looks better than what I have now.
> Any reason why the above two aren't suitable for mainline Linux? The
> second was actually committed then reverted because it broke arm64
> builds, but the other patch fixes that.
>
Yes IMO since arm-big-little also supports multi-cluster cpufreq(yes I
know it can be merged into cpufreq-dt once we have OPPv2 bindings, but
until then), it should handle the config where CONFIG_BL_SWITCHER=n and
that's what I have attempted in my patch above.
On Fri, 2015-05-01 at 14:32 +0100, Sudeep Holla wrote:
>
> On 01/05/15 14:19, Jon Medhurst (Tixy) wrote:
> > On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:
> >> On some ARM based systems, a separate Cortex-M based System Control
> >> Processor(SCP) provides the overall power, clock, reset and system
> >> control including CPU DVFS. SCPI Message Protocol is used to
> >> communicate with the SCPI.
> >>
> >> This patch adds a interface driver for adding OPPs and registering
> >> the arm_big_little cpufreq driver for such systems.
> >>
> >> Signed-off-by: Sudeep Holla <[email protected]>
> >> Cc: Viresh Kumar <[email protected]>
> >> Cc: "Rafael J. Wysocki" <[email protected]>
> >> Cc: [email protected]
> >> ---
> >> drivers/cpufreq/Kconfig.arm | 9 ++++
> >> drivers/cpufreq/Makefile | 1 +
> >> drivers/cpufreq/scpi-cpufreq.c | 103 +++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 113 insertions(+)
> >> create mode 100644 drivers/cpufreq/scpi-cpufreq.c
> >>
> >> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> >> index 4f3dbc8cf729..9e678bf1687c 100644
> >> --- a/drivers/cpufreq/Kconfig.arm
> >> +++ b/drivers/cpufreq/Kconfig.arm
> >> @@ -24,6 +24,15 @@ config ARM_VEXPRESS_SPC_CPUFREQ
> >> This add the CPUfreq driver support for Versatile Express
> >> big.LITTLE platforms using SPC for power management.
> >>
> >> +config ARM_SCPI_CPUFREQ
> >> + tristate "SCPI based CPUfreq driver"
> >> + depends on ARM_BIG_LITTLE_CPUFREQ && ARM_SCPI_PROTOCOL
> >
> > And ARM_BIG_LITTLE_CPUFREQ depends on CONFIG_ARM, so we can't build this
> > for arm64, which is the only platform (Juno) we have to run these
> > patches on.
>
> Yes I know
>
> > Unless you're prepared for a horrid hack...
> > https://github.com/ARM-software/linux/commit/b9ceaa0cbd7c57d57ee7e69146cc627697570f6e
>
> I need to check if it can be improved before I post on the list and
> hence dropped it in the first version of this patch-set.
>
> > or a pair of less horrid ones...
> > http://git.linaro.org/kernel/linux-linaro-tracking.git/commit/4b5dd8ff98613b7e90c8f3214522a00ab6900fe9
>
> No not this one, this was already discussed long back I believe.
> It doesn't make sense to add bl-switcher for ARM64 IMO.
>
> > http://git.linaro.org/kernel/linux-linaro-tracking.git/commit/8641dbfe253f6a9061742ca11f769fc1d82c2aaa
> >
>
> Yes the Kconfig changes looks better than what I have now.
>
> > Any reason why the above two aren't suitable for mainline Linux? The
> > second was actually committed then reverted because it broke arm64
> > builds, but the other patch fixes that.
> >
> Yes IMO since arm-big-little also supports multi-cluster cpufreq(yes I
> know it can be merged into cpufreq-dt once we have OPPv2 bindings, but
> until then), it should handle the config where CONFIG_BL_SWITCHER=n and
> that's what I have attempted in my patch above.
OK, that all makes sense, just wanted to make sure things didn't get
overlooked, because having code that has no users in mainline seemed a
bit odd.
--
Tixy
On 01/05/15 15:12, Jon Medhurst (Tixy) wrote:
> On Fri, 2015-05-01 at 14:32 +0100, Sudeep Holla wrote:
>>
>> On 01/05/15 14:19, Jon Medhurst (Tixy) wrote:
>>> On Mon, 2015-04-27 at 12:40 +0100, Sudeep Holla wrote:
>>>> On some ARM based systems, a separate Cortex-M based System Control
>>>> Processor(SCP) provides the overall power, clock, reset and system
>>>> control including CPU DVFS. SCPI Message Protocol is used to
>>>> communicate with the SCPI.
>>>>
>>>> This patch adds a interface driver for adding OPPs and registering
>>>> the arm_big_little cpufreq driver for such systems.
>>>>
>>>> Signed-off-by: Sudeep Holla <[email protected]>
>>>> Cc: Viresh Kumar <[email protected]>
>>>> Cc: "Rafael J. Wysocki" <[email protected]>
>>>> Cc: [email protected]
>>>> ---
>>>> drivers/cpufreq/Kconfig.arm | 9 ++++
>>>> drivers/cpufreq/Makefile | 1 +
>>>> drivers/cpufreq/scpi-cpufreq.c | 103 +++++++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 113 insertions(+)
>>>> create mode 100644 drivers/cpufreq/scpi-cpufreq.c
>>>>
>>>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>>>> index 4f3dbc8cf729..9e678bf1687c 100644
>>>> --- a/drivers/cpufreq/Kconfig.arm
>>>> +++ b/drivers/cpufreq/Kconfig.arm
>>>> @@ -24,6 +24,15 @@ config ARM_VEXPRESS_SPC_CPUFREQ
>>>> This add the CPUfreq driver support for Versatile Express
>>>> big.LITTLE platforms using SPC for power management.
>>>>
>>>> +config ARM_SCPI_CPUFREQ
>>>> + tristate "SCPI based CPUfreq driver"
>>>> + depends on ARM_BIG_LITTLE_CPUFREQ && ARM_SCPI_PROTOCOL
>>>
>>> And ARM_BIG_LITTLE_CPUFREQ depends on CONFIG_ARM, so we can't build this
>>> for arm64, which is the only platform (Juno) we have to run these
>>> patches on.
>>
>> Yes I know
>>
>>> Unless you're prepared for a horrid hack...
>>> https://github.com/ARM-software/linux/commit/b9ceaa0cbd7c57d57ee7e69146cc627697570f6e
>>
>> I need to check if it can be improved before I post on the list and
>> hence dropped it in the first version of this patch-set.
>>
>>> or a pair of less horrid ones...
>>> http://git.linaro.org/kernel/linux-linaro-tracking.git/commit/4b5dd8ff98613b7e90c8f3214522a00ab6900fe9
>>
>> No not this one, this was already discussed long back I believe.
>> It doesn't make sense to add bl-switcher for ARM64 IMO.
>>
>>> http://git.linaro.org/kernel/linux-linaro-tracking.git/commit/8641dbfe253f6a9061742ca11f769fc1d82c2aaa
>>>
>>
>> Yes the Kconfig changes looks better than what I have now.
>>
>>> Any reason why the above two aren't suitable for mainline Linux? The
>>> second was actually committed then reverted because it broke arm64
>>> builds, but the other patch fixes that.
>>>
>> Yes IMO since arm-big-little also supports multi-cluster cpufreq(yes I
>> know it can be merged into cpufreq-dt once we have OPPv2 bindings, but
>> until then), it should handle the config where CONFIG_BL_SWITCHER=n and
>> that's what I have attempted in my patch above.
>
> OK, that all makes sense, just wanted to make sure things didn't get
> overlooked, because having code that has no users in mainline seemed a
> bit odd.
>
No issues, it's better to have remainders like this :). I plan to post
something in the next version.
Regards,
Sudeep
On Fri, 2015-05-01 at 15:15 +0100, Sudeep Holla wrote:
> No issues, it's better to have remainders like this :). I plan to post
> something in the next version.
Will that also include the other piece of the puzzle to get the
big.LITTLE cpufreq driver running? ;-) ...
https://github.com/ARM-software/linux/commit/29c33a5865437f0745e8e3a9beb5f82694257b4d
With that, and device-tree updates [1], I've finally managed to get this
patch series running on Juno with v4.1-rc1 and they pass some basic
smoke testing.
--
Tixy
On 01/05/15 18:10, Jon Medhurst (Tixy) wrote:
> On Fri, 2015-05-01 at 15:15 +0100, Sudeep Holla wrote:
>> No issues, it's better to have remainders like this :). I plan to post
>> something in the next version.
>
> Will that also include the other piece of the puzzle to get the
> big.LITTLE cpufreq driver running? ;-) ...
> https://github.com/ARM-software/linux/commit/29c33a5865437f0745e8e3a9beb5f82694257b4d
That is already posted separately[1] and acked-by Viresh.
I am trying to put independent patches(not related to Juno/SCPI)
separate so that they will be no dependency to merge that.
Regards,
Sudeep
[1] https://lkml.org/lkml/2015/4/27/174
On Mon, Apr 27, 2015 at 12:40:44PM +0100, Sudeep Holla wrote:
> On some ARM based systems, a separate Cortex-M based System Control
> Processor(SCP) provides the overall power, clock, reset and system
> control. System Control and Power Interface(SCPI) Message Protocol
> is defined for the communication between the Application Cores(AP)
> and the SCP.
>
> This patch adds support for the clocks provided by SCP using SCPI
> protocol.
>
> Signed-off-by: Sudeep Holla <[email protected]>
> Cc: Mike Turquette <[email protected]>
> Cc: Stephen Boyd <[email protected]>
> Cc: Liviu Dudau <[email protected]>
> Cc: Lorenzo Pieralisi <[email protected]>
> Cc: Jon Medhurst (Tixy) <[email protected]>
> Cc: [email protected]
> ---
> drivers/clk/Kconfig | 10 ++
> drivers/clk/Makefile | 1 +
> drivers/clk/clk-scpi.c | 344 +++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 355 insertions(+)
> create mode 100644 drivers/clk/clk-scpi.c
>
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 9897f353bf1a..906bf7dd72a2 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -59,6 +59,16 @@ config COMMON_CLK_RK808
> clocked at 32KHz each. Clkout1 is always on, Clkout2 can off
> by control register.
>
> +config COMMON_CLK_SCPI
> + tristate "Clock driver controlled via SCPI interface"
> + depends on ARM_SCPI_PROTOCOL
> + ---help---
> + This driver provides support for clocks that are controlled
> + by firmware that implements the SCPI interface.
> +
> + This driver uses SCPI Message Protocol to interact with the
> + firware providing all the clock controls.
/s/firware/firmware
> +
> config COMMON_CLK_SI5351
> tristate "Clock driver for SiLabs 5351A/B/C"
> depends on I2C
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 3d00c25382c5..442ab6ebd5b1 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -36,6 +36,7 @@ obj-$(CONFIG_COMMON_CLK_PALMAS) += clk-palmas.o
> obj-$(CONFIG_CLK_QORIQ) += clk-qoriq.o
> obj-$(CONFIG_COMMON_CLK_RK808) += clk-rk808.o
> obj-$(CONFIG_COMMON_CLK_S2MPS11) += clk-s2mps11.o
> +obj-$(CONFIG_COMMON_CLK_SCPI) += clk-scpi.o
> obj-$(CONFIG_COMMON_CLK_SI5351) += clk-si5351.o
> obj-$(CONFIG_COMMON_CLK_SI570) += clk-si570.o
> obj-$(CONFIG_CLK_TWL6040) += clk-twl6040.o
> diff --git a/drivers/clk/clk-scpi.c b/drivers/clk/clk-scpi.c
> new file mode 100644
> index 000000000000..70a2d57da32d
> --- /dev/null
> +++ b/drivers/clk/clk-scpi.c
> @@ -0,0 +1,344 @@
> +/*
> + * System Control and Power Interface (SCPI) Protocol based clock driver
> + *
> + * 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/>.
> + */
> +
> +#include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/scpi_protocol.h>
> +
> +struct scpi_clk {
> + u32 id;
> + const char *name;
> + struct clk_hw hw;
> + struct scpi_dvfs_info *info;
> + unsigned long rate_min;
> + unsigned long rate_max;
> +};
> +
> +#define to_scpi_clk(clk) container_of(clk, struct scpi_clk, hw)
> +
> +static struct scpi_ops *scpi_ops;
> +
> +static unsigned long scpi_clk_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct scpi_clk *clk = to_scpi_clk(hw);
> +
> + return scpi_ops->clk_get_val(clk->id);
> +}
> +
> +static long scpi_clk_round_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long *parent_rate)
> +{
> + struct scpi_clk *clk = to_scpi_clk(hw);
> +
> + if (WARN_ON(clk->rate_min && rate < clk->rate_min))
> + rate = clk->rate_min;
> + if (WARN_ON(clk->rate_max && rate > clk->rate_max))
> + rate = clk->rate_max;
> +
> + return rate;
> +}
> +
> +static int scpi_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate)
> +{
> + struct scpi_clk *clk = to_scpi_clk(hw);
> +
> + return scpi_ops->clk_set_val(clk->id, rate);
> +}
> +
> +static struct clk_ops scpi_clk_ops = {
> + .recalc_rate = scpi_clk_recalc_rate,
> + .round_rate = scpi_clk_round_rate,
> + .set_rate = scpi_clk_set_rate,
> +};
> +
> +/* find closest match to given frequency in OPP table */
> +static int __scpi_dvfs_round_rate(struct scpi_clk *clk, unsigned long rate)
> +{
> + int idx;
> + u32 fmin = 0, fmax = ~0, ftmp;
> + struct scpi_opp *opp = clk->info->opps;
> +
> + for (idx = 0; idx < clk->info->count; idx++, opp++) {
> + ftmp = opp->freq;
> + if (ftmp >= (u32)rate) {
> + if (ftmp <= fmax)
> + fmax = ftmp;
> + } else {
> + if (ftmp >= fmin)
> + fmin = ftmp;
> + }
> + }
Is the opp list sorted ? Can't you break instead of going through
the whole opp set ?
> + if (fmax != ~0)
> + return fmax;
> + return fmin;
return fmax != ~0 ? fmax : fmin;
> +}
> +
> +static unsigned long scpi_dvfs_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct scpi_clk *clk = to_scpi_clk(hw);
> + int idx = scpi_ops->dvfs_get_idx(clk->id);
> + struct scpi_opp *opp;
> +
> + if (idx < 0)
> + return 0;
> +
> + opp = clk->info->opps + idx;
> + return opp->freq;
> +}
> +
> +static long scpi_dvfs_round_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long *parent_rate)
> +{
> + struct scpi_clk *clk = to_scpi_clk(hw);
> +
> + return __scpi_dvfs_round_rate(clk, rate);
> +}
> +
> +static int __scpi_find_dvfs_index(struct scpi_clk *clk, unsigned long rate)
> +{
> + int idx, max_opp = clk->info->count;
> + struct scpi_opp *opp = clk->info->opps;
> +
> + for (idx = 0; idx < max_opp; idx++, opp++)
> + if (opp->freq == rate)
> + break;
return idx;
> + return (idx == max_opp) ? -EINVAL : idx;
return -EINVAL;
> +}
> +
> +static int scpi_dvfs_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate)
> +{
> + struct scpi_clk *clk = to_scpi_clk(hw);
> + int ret = __scpi_find_dvfs_index(clk, rate);
> +
> + if (ret < 0)
> + return ret;
> + return scpi_ops->dvfs_set_idx(clk->id, (u8)ret);
> +}
> +
> +static struct clk_ops scpi_dvfs_ops = {
> + .recalc_rate = scpi_dvfs_recalc_rate,
> + .round_rate = scpi_dvfs_round_rate,
> + .set_rate = scpi_dvfs_set_rate,
> +};
> +
> +static struct clk *
> +scpi_dvfs_ops_init(struct device *dev, struct device_node *np,
> + struct scpi_clk *sclk)
> +{
> + struct clk_init_data init;
> + struct scpi_dvfs_info *info;
> +
> + init.name = sclk->name;
> + init.flags = CLK_IS_ROOT;
> + init.num_parents = 0;
> + init.ops = &scpi_dvfs_ops;
> + sclk->hw.init = &init;
> +
> + info = scpi_ops->dvfs_get_info(sclk->id);
> + if (IS_ERR(info))
> + return (struct clk *)info;
Can't you just return NULL ?
> +
> + sclk->info = info;
> +
> + return devm_clk_register(dev, &sclk->hw);
> +}
> +
> +static struct clk *
> +scpi_clk_ops_init(struct device *dev, struct device_node *np,
> + struct scpi_clk *sclk)
> +{
> + struct clk_init_data init;
> + int ret;
> +
> + init.name = sclk->name;
> + init.flags = CLK_IS_ROOT;
> + init.num_parents = 0;
> + init.ops = &scpi_clk_ops;
> + sclk->hw.init = &init;
> +
> + ret = scpi_ops->clk_get_range(sclk->id, &sclk->rate_min,
> + &sclk->rate_max);
> + if (!sclk->rate_max)
> + ret = -EINVAL;
> + if (ret)
> + return ERR_PTR(ret);
Ditto, this ERR_PTR song and dance is not nice at all.
> +
> + return devm_clk_register(dev, &sclk->hw);
> +}
These two functions are identical apart from the ops, can't we have
just one function and differentiate behavior according to the matching
compatible string ?
> +static const struct of_device_id scpi_clk_match[] = {
> + { .compatible = "arm,scpi-dvfs", .data = scpi_dvfs_ops_init, },
> + { .compatible = "arm,scpi-clk", .data = scpi_clk_ops_init, },
> + {}
> +};
> +
> +static int scpi_clk_probe(struct platform_device *pdev)
> +{
> + struct clk **clks;
> + int idx, count;
> + struct clk_onecell_data *clk_data;
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> + struct clk *(*clk_ops_init)(struct device *, struct device_node *,
> + struct scpi_clk *);
> +
> + if (!of_device_is_available(np))
> + return -ENODEV;
> +
> + clk_ops_init = of_match_node(scpi_clk_match, np)->data;
> + if (!clk_ops_init)
> + return -ENODEV;
> +
> + count = of_property_count_strings(np, "clock-output-names");
> + if (count < 0) {
> + dev_err(dev, "%s: invalid clock output count\n", np->name);
> + return -EINVAL;
> + }
> +
> + clk_data = devm_kmalloc(dev, sizeof(*clk_data), GFP_KERNEL);
> + if (!clk_data) {
> + dev_err(dev, "failed to allocate clock provider data\n");
> + return -ENOMEM;
> + }
> +
> + clks = devm_kmalloc(dev, count * sizeof(*clks), GFP_KERNEL);
> + if (!clks) {
> + dev_err(dev, "failed to allocate clock providers\n");
> + return -ENOMEM;
> + }
> +
> + for (idx = 0; idx < count; idx++) {
> + struct scpi_clk *sclk;
> + u32 val;
> +
> + sclk = devm_kzalloc(dev, sizeof(*sclk), GFP_KERNEL);
> + if (!sclk) {
> + dev_err(dev, "failed to allocate scpi clocks\n");
> + return -ENOMEM;
> + }
> +
> + if (of_property_read_string_index(np, "clock-output-names",
> + idx, &sclk->name)) {
> + dev_err(dev, "invalid clock name @ %s\n", np->name);
> + return -EINVAL;
> + }
> +
> + if (of_property_read_u32_index(np, "clock-indices",
> + idx, &val)) {
> + dev_err(dev, "invalid clock index @ %s\n", np->name);
> + return -EINVAL;
> + }
> +
> + sclk->id = val;
> +
> + clks[idx] = clk_ops_init(dev, np, sclk);
> + if (IS_ERR(clks[idx])) {
See above.
> + dev_err(dev, "failed to register clock '%s'\n",
> + sclk->name);
> + }
> +
> + dev_dbg(dev, "Registered clock '%s'\n", sclk->name);
> + }
> +
> + clk_data->clks = clks;
> + clk_data->clk_num = idx;
> + of_clk_add_provider(np, of_clk_src_onecell_get, clk_data);
> +
> + platform_set_drvdata(pdev, clk_data);
Where is this used ?
> +
> + return 0;
> +}
> +
> +static int scpi_clk_remove(struct platform_device *pdev)
> +{
> + of_clk_del_provider(pdev->dev.of_node);
> + platform_set_drvdata(pdev, NULL);
> + return 0;
> +}
> +
> +static struct platform_driver scpi_clk_driver = {
> + .driver = {
> + .name = "scpi_clk",
> + .of_match_table = scpi_clk_match,
> + },
> + .probe = scpi_clk_probe,
> + .remove = scpi_clk_remove,
> +};
> +
> +static int scpi_clocks_probe(struct platform_device *pdev)
> +{
> + scpi_ops = get_scpi_ops();
> + if (!scpi_ops)
> + return -ENXIO;
> +
> + return of_platform_populate(pdev->dev.of_node, scpi_clk_match,
> + NULL, &pdev->dev);
Do you really need to create a platform_device per sub-node ?
> +}
> +
> +static int scpi_clocks_remove(struct platform_device *pdev)
> +{
> + of_platform_depopulate(&pdev->dev);
> + scpi_ops = NULL;
> + return 0;
> +}
> +
> +static const struct of_device_id scpi_clocks_ids[] = {
> + { .compatible = "arm,scpi-clocks", },
> + {}
> +};
> +
> +static struct platform_driver scpi_clocks_driver = {
> + .driver = {
> + .name = "scpi_clocks",
> + .of_match_table = scpi_clocks_ids,
> + },
> + .probe = scpi_clocks_probe,
> + .remove = scpi_clocks_remove,
> +};
Why do you need two drivers for the clocks ? Isn't the parent device
tree node and respective platform device enough to initialize all
the subnodes clocks ?
> +static int __init scpi_clocks_init(void)
> +{
> + int ret;
> +
> + ret = platform_driver_register(&scpi_clk_driver);
> + if (ret)
> + return ret;
> +
> + return platform_driver_register(&scpi_clocks_driver);
See above.
Thanks,
Lorenzo
> +}
> +module_init(scpi_clocks_init);
> +
> +static void __exit scpi_clocks_exit(void)
> +{
> + platform_driver_unregister(&scpi_clk_driver);
> + platform_driver_unregister(&scpi_clocks_driver);
> +}
> +module_exit(scpi_clocks_exit);
> +
> +MODULE_AUTHOR("Sudeep Holla <[email protected]>");
> +MODULE_DESCRIPTION("ARM SCPI clock driver");
> +MODULE_LICENSE("GPL");
> --
> 1.9.1
>
On Mon, Apr 27, 2015 at 5:10 PM, Sudeep Holla <[email protected]> wrote:
> This patch adds support for System Control and Power Interface (SCPI)
> Message Protocol used between the Application Cores(AP) and the System
> Control Processor(SCP). The MHU peripheral provides a mechanism for
> inter-processor communication between SCP's M3 processor 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.
>
> This protocol driver provides interface for all the client drivers using
> SCPI to make use of the features offered by the SCP.
>
Is the SCPI specification available somewhere to look into?
> Signed-off-by: Sudeep Holla <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Mark Rutland <[email protected]>
> CC: Jassi Brar <[email protected]>
> Cc: Liviu Dudau <[email protected]>
> Cc: Lorenzo Pieralisi <[email protected]>
> Cc: Jon Medhurst (Tixy) <[email protected]>
> Cc: [email protected]
> ---
> .../devicetree/bindings/mailbox/arm,scpi.txt | 121 ++++
> drivers/mailbox/Kconfig | 19 +
> drivers/mailbox/Makefile | 2 +
> drivers/mailbox/scpi_protocol.c | 694 +++++++++++++++++++++
>
Why in drivers/mailbox/ ? This is a 'consumer' driver and seems
Juno(ARM) specific.
-Jassi
On 13/05/15 17:52, Jassi Brar wrote:
> On Mon, Apr 27, 2015 at 5:10 PM, Sudeep Holla <[email protected]> wrote:
>> This patch adds support for System Control and Power Interface (SCPI)
>> Message Protocol used between the Application Cores(AP) and the System
>> Control Processor(SCP). The MHU peripheral provides a mechanism for
>> inter-processor communication between SCP's M3 processor 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.
>>
>> This protocol driver provides interface for all the client drivers using
>> SCPI to make use of the features offered by the SCP.
>>
> Is the SCPI specification available somewhere to look into?
>
Yes sorry posted the link separately(as reply to Tixy in the cover
letter) since it was not available when I posted the patches.
You can grab the protocol @[1] or [2]
>> Signed-off-by: Sudeep Holla <[email protected]>
>> Cc: Rob Herring <[email protected]>
>> Cc: Mark Rutland <[email protected]>
>> CC: Jassi Brar <[email protected]>
>> Cc: Liviu Dudau <[email protected]>
>> Cc: Lorenzo Pieralisi <[email protected]>
>> Cc: Jon Medhurst (Tixy) <[email protected]>
>> Cc: [email protected]
>> ---
>> .../devicetree/bindings/mailbox/arm,scpi.txt | 121 ++++
>> drivers/mailbox/Kconfig | 19 +
>> drivers/mailbox/Makefile | 2 +
>> drivers/mailbox/scpi_protocol.c | 694 +++++++++++++++++++++
>>
> Why in drivers/mailbox/ ? This is a 'consumer' driver and seems
> Juno(ARM) specific.
>
Not just JUNO alone though it's first one to use, it will used in next
few platforms(foreseeable future) from ARM Ltd.
I have put that in drivers/mailbox for 2 reasons:
1. It's mailbox protocol :)
2. ARM64 doesn't have platform code like ARM32 and moreover it's
strictly not specific to JUNO or any single platform. It may
get reused on other platforms.
Regards,
Sudeep
[1]
http://community.arm.com/servlet/JiveServlet/download/8401-40-18262/DUI0922A_scp_message_interface.pdf
[2]
https://wiki.linaro.org/ARM/Juno?action=AttachFile&do=get&target=DUI0922A_scp_message_interface.pdf
On Wed, May 13, 2015 at 10:39 PM, Sudeep Holla <[email protected]> wrote:
> On 13/05/15 17:52, Jassi Brar wrote:
>>
>>> This patch adds support for System Control and Power Interface (SCPI)
>>> Message Protocol used between the Application Cores(AP) and the System
>>> Control Processor(SCP). The MHU peripheral provides a mechanism for
>>> inter-processor communication between SCP's M3 processor 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.
>>>
>>> This protocol driver provides interface for all the client drivers using
>>> SCPI to make use of the features offered by the SCP.
>>>
>> Is the SCPI specification available somewhere to look into?
>>
>
> Yes sorry posted the link separately(as reply to Tixy in the cover
> letter) since it was not available when I posted the patches.
> You can grab the protocol @[1] or [2]
>
Thanks for the link. I wish I had access to the spec earlier.
>>> .../devicetree/bindings/mailbox/arm,scpi.txt | 121 ++++
>>> drivers/mailbox/Kconfig | 19 +
>>> drivers/mailbox/Makefile | 2 +
>>> drivers/mailbox/scpi_protocol.c | 694
>>> +++++++++++++++++++++
>>>
>> Why in drivers/mailbox/ ? This is a 'consumer' driver and seems
>> Juno(ARM) specific.
>>
>
> Not just JUNO alone though it's first one to use, it will used in next
> few platforms(foreseeable future) from ARM Ltd.
>
> I have put that in drivers/mailbox for 2 reasons:
> 1. It's mailbox protocol :)
>
client/protocol drivers don't usually reside with controller drivers.
drivers/firmware/ seems more appropriate.
> 2. ARM64 doesn't have platform code like ARM32 and moreover it's
> strictly not specific to JUNO or any single platform. It may
> get reused on other platforms.
>
drivers/firmware/ should do too.
BTW is scpi_protocol.c meant/tested to work over arm_mhu.c? The spec
says so but I don't see how because you pass 'struct scpi_xfer*' as
the message whereas arm_mhu.c expects u32*
On Thu, May 14, 2015 at 12:32 PM, Jassi Brar <[email protected]> wrote:
>
> BTW is scpi_protocol.c meant/tested to work over arm_mhu.c? The spec
> says so but I don't see how because you pass 'struct scpi_xfer*' as
> the message whereas arm_mhu.c expects u32*
>
It seems your remote doesn't interpret the value in STAT register...
so it just works.
However the SCPI spec recommends seeing STAT register as '31 slots'
... maybe we should try to support that.
thanks.
On 14/05/15 08:30, Jassi Brar wrote:
> On Thu, May 14, 2015 at 12:32 PM, Jassi Brar <[email protected]> wrote:
>>
>> BTW is scpi_protocol.c meant/tested to work over arm_mhu.c? The spec
>> says so but I don't see how because you pass 'struct scpi_xfer*' as
>> the message whereas arm_mhu.c expects u32*
>>
Yes it's tested using arm_mhu.c, and I have even sent updates to the
binding that's incomplete as of now and *must* be pulled into v4.1.
Please make sure it gets in. Otherwise clocks are optional but the
driver fails to probe without that. I was initially wondering why the
MHU probe is not called.
scpi_xfer has the slot as first element which will have the right
doorbell bit(in this case slot#0) set always.
> It seems your remote doesn't interpret the value in STAT register...
> so it just works.
Not exactly. If you look at Figure 2-1 in the spec, it shows how STAT
(along with SET/CLEAR) is used to identify the protocol. The remote
can implement multiple protocol. E.g. SCPI(on Slot#0 - main topic of
this series), ACPI(PCC/CPPC say on Slot#1), ABC(on Slot#X)...etc.
> However the SCPI spec recommends seeing STAT register as '31 slots'
> ... maybe we should try to support that.
>
Correct but only Slot#0 is used for SCPI.
Regards,
Sudeep
On 04/27, Sudeep Holla wrote:
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 9897f353bf1a..906bf7dd72a2 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -59,6 +59,16 @@ config COMMON_CLK_RK808
> clocked at 32KHz each. Clkout1 is always on, Clkout2 can off
> by control register.
>
> +config COMMON_CLK_SCPI
> + tristate "Clock driver controlled via SCPI interface"
> + depends on ARM_SCPI_PROTOCOL
|| COMPILE_TEST ?
> + ---help---
> + This driver provides support for clocks that are controlled
> + by firmware that implements the SCPI interface.
> +
> + This driver uses SCPI Message Protocol to interact with the
> + firware providing all the clock controls.
> +
> config COMMON_CLK_SI5351
> tristate "Clock driver for SiLabs 5351A/B/C"
> depends on I2C
> diff --git a/drivers/clk/clk-scpi.c b/drivers/clk/clk-scpi.c
> new file mode 100644
> index 000000000000..70a2d57da32d
> --- /dev/null
> +++ b/drivers/clk/clk-scpi.c
> @@ -0,0 +1,344 @@
> +#include <linux/clkdev.h>
Is clkdev used here?
> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/scpi_protocol.h>
> +
[...]
> +
> +static int scpi_dvfs_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate)
> +{
> + struct scpi_clk *clk = to_scpi_clk(hw);
> + int ret = __scpi_find_dvfs_index(clk, rate);
> +
> + if (ret < 0)
> + return ret;
> + return scpi_ops->dvfs_set_idx(clk->id, (u8)ret);
> +}
> +
> +static struct clk_ops scpi_dvfs_ops = {
const?
> + .recalc_rate = scpi_dvfs_recalc_rate,
> + .round_rate = scpi_dvfs_round_rate,
> + .set_rate = scpi_dvfs_set_rate,
> +};
> +
> +static struct clk *
> +scpi_dvfs_ops_init(struct device *dev, struct device_node *np,
> + struct scpi_clk *sclk)
> +{
> + struct clk_init_data init;
> + struct scpi_dvfs_info *info;
> +
> + init.name = sclk->name;
> + init.flags = CLK_IS_ROOT;
> + init.num_parents = 0;
> + init.ops = &scpi_dvfs_ops;
> + sclk->hw.init = &init;
> +
> + info = scpi_ops->dvfs_get_info(sclk->id);
> + if (IS_ERR(info))
> + return (struct clk *)info;
return ERR_CAST(info)?
> +
> + sclk->info = info;
> +
> + return devm_clk_register(dev, &sclk->hw);
> +}
> +
> +
[...]
> +static int scpi_clk_probe(struct platform_device *pdev)
> +{
> + struct clk **clks;
> + int idx, count;
> + struct clk_onecell_data *clk_data;
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> + struct clk *(*clk_ops_init)(struct device *, struct device_node *,
> + struct scpi_clk *);
> +
> + if (!of_device_is_available(np))
> + return -ENODEV;
Is this because of_platform_populate() populates disabled nodes?
> +
> + clk_ops_init = of_match_node(scpi_clk_match, np)->data;
> + if (!clk_ops_init)
> + return -ENODEV;
> +
> + count = of_property_count_strings(np, "clock-output-names");
> + if (count < 0) {
> + dev_err(dev, "%s: invalid clock output count\n", np->name);
> + return -EINVAL;
> + }
> +
> + clk_data = devm_kmalloc(dev, sizeof(*clk_data), GFP_KERNEL);
> + if (!clk_data) {
> + dev_err(dev, "failed to allocate clock provider data\n");
> + return -ENOMEM;
> + }
> +
> + clks = devm_kmalloc(dev, count * sizeof(*clks), GFP_KERNEL);
devm_kcalloc()?
> + if (!clks) {
> + dev_err(dev, "failed to allocate clock providers\n");
Error messages on allocation failures are useless. Please remove.
> + return -ENOMEM;
> + }
> +
> + for (idx = 0; idx < count; idx++) {
> + struct scpi_clk *sclk;
[...]
> +static int scpi_clk_remove(struct platform_device *pdev)
> +{
> + of_clk_del_provider(pdev->dev.of_node);
> + platform_set_drvdata(pdev, NULL);
We don't need to set platform_drvdata to NULL here.
> + return 0;
> +}
> +
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 04/27, Sudeep Holla wrote:
> The clocks for the CPUs are provided by SCP and are managed by this
> clock driver. So the cpufreq device needs to be added only after the
> clock get registered and removed when this driver is unloaded.
>
> This patch manages the cpufreq virtual device based on the clock
> availability.
>
> Signed-off-by: Sudeep Holla <[email protected]>
The cpufreq device can't handle probe defer? I suppose we do it
this way because we need to create a platform device somewhere
and this is the best place to do so?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Hi Stephen,
On 21/05/15 00:43, Stephen Boyd wrote:
> On 04/27, Sudeep Holla wrote:
>> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
>> index 9897f353bf1a..906bf7dd72a2 100644
>> --- a/drivers/clk/Kconfig
>> +++ b/drivers/clk/Kconfig
>> @@ -59,6 +59,16 @@ config COMMON_CLK_RK808
>> clocked at 32KHz each. Clkout1 is always on, Clkout2 can off
>> by control register.
>>
>> +config COMMON_CLK_SCPI
>> + tristate "Clock driver controlled via SCPI interface"
>> + depends on ARM_SCPI_PROTOCOL
>
> || COMPILE_TEST ?
>
[...]
>
>> + return -ENOMEM;
>> + }
>> +
>> + for (idx = 0; idx < count; idx++) {
>> + struct scpi_clk *sclk;
> [...]
>> +static int scpi_clk_remove(struct platform_device *pdev)
>> +{
>> + of_clk_del_provider(pdev->dev.of_node);
>> + platform_set_drvdata(pdev, NULL);
>
> We don't need to set platform_drvdata to NULL here.
>
Thanks for the review. I have posted v2 of this series and some of the
comments are not applicable(already done or code removed). I have
fixed all the remaining comments locally for v3.
Regards,
Sudeep
On 21/05/15 00:45, Stephen Boyd wrote:
> On 04/27, Sudeep Holla wrote:
>> The clocks for the CPUs are provided by SCP and are managed by this
>> clock driver. So the cpufreq device needs to be added only after the
>> clock get registered and removed when this driver is unloaded.
>>
>> This patch manages the cpufreq virtual device based on the clock
>> availability.
>>
>> Signed-off-by: Sudeep Holla <[email protected]>
>
> The cpufreq device can't handle probe defer? I suppose we do it
> this way because we need to create a platform device somewhere
> and this is the best place to do so?
>
Correct, unless the communication with the SCP firmware is established
and the CPU clocks are registered, it makes no sense to register CPUFreq
virtual device. This seems to be best place IMO.
And yes, thanks to the absence of platform code on ARM64, where such
code was traditionally stashed away and got away with such scenarios.
Let me if you disagree with this or have any other suggestions.
Regards,
Sudeep