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

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

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

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

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

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

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

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

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

--
2.7.0

Reply | Threaded
Open this post in threaded view
|

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

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

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

Signed-off-by: Neil Armstrong <[hidden email]>
---
 drivers/firmware/Makefile |   1 +
 drivers/firmware/scpi.c   | 110 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 111 insertions(+)
 create mode 100644 drivers/firmware/scpi.c

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

Reply | Threaded
Open this post in threaded view
|

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

Neil Armstrong-3
In reply to this post by Neil Armstrong-3
Update the include/linux/scpi_protocol.h with the new registry calls.

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

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

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

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

Reply | Threaded
Open this post in threaded view
|

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

Sudeep Holla
In reply to this post by Neil Armstrong-3
Hi Neil,

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

Agreed.

> - does not specify a strong "standard"

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

> - does not specify a strong MHU interface specification
>

Not really required, any mailbox must do.

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

I assume you mean command index here

> new messages types,

Also fine with extended command set.

> different messages data format or

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

> different MHU communication scheme.

Not a problem as I mentioned above.

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

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

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

--
Regards,
Sudeep

-->8

diff --git i/drivers/firmware/arm_scpi.c w/drivers/firmware/arm_scpi.c
index 7e3e595c9f30..7e46aa103690 100644
--- i/drivers/firmware/arm_scpi.c
+++ w/drivers/firmware/arm_scpi.c
@@ -46,6 +46,8 @@

  #define CMD_ID_SHIFT 0
  #define CMD_ID_MASK 0x7f
+#define CMD_SET_SHIFT 7
+#define CMD_SET_MASK 0x1
  #define CMD_TOKEN_ID_SHIFT 8
  #define CMD_TOKEN_ID_MASK 0xff
  #define CMD_DATA_SIZE_SHIFT 16
@@ -53,6 +55,10 @@
  #define PACK_SCPI_CMD(cmd_id, tx_sz) \
  ((((cmd_id) & CMD_ID_MASK) << CMD_ID_SHIFT) | \
  (((tx_sz) & CMD_DATA_SIZE_MASK) << CMD_DATA_SIZE_SHIFT))
+#define PACK_EXT_SCPI_CMD(cmd_id, tx_sz) \
+ ((((cmd_id) & CMD_ID_MASK) << CMD_ID_SHIFT) | \
+ (CMD_SET_MASK << CMD_SET_SHIFT) | \
+ (((tx_sz) & CMD_DATA_SIZE_MASK) << CMD_DATA_SIZE_SHIFT))
  #define ADD_SCPI_TOKEN(cmd, token) \
  ((cmd) |= (((token) & CMD_TOKEN_ID_MASK) << CMD_TOKEN_ID_SHIFT))

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

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

  /*
@@ -343,8 +353,8 @@ static void put_scpi_xfer(struct scpi_xfer *t,
struct scpi_chan *ch)
  mutex_unlock(&ch->xfers_lock);
  }

-static int scpi_send_message(u8 cmd, void *tx_buf, unsigned int tx_len,
-     void *rx_buf, unsigned int rx_len)
+static int __scpi_send_message(u8 cmd, void *tx_buf, unsigned int tx_len,
+       void *rx_buf, unsigned int rx_len, bool extn)
  {
  int ret;
  u8 chan;
@@ -359,7 +369,8 @@ static int scpi_send_message(u8 cmd, void *tx_buf,
unsigned int tx_len,
  return -ENOMEM;

  msg->slot = BIT(SCPI_SLOT);
- msg->cmd = PACK_SCPI_CMD(cmd, tx_len);
+ msg->cmd = extn ? PACK_EXT_SCPI_CMD(cmd, tx_len) :
+  PACK_SCPI_CMD(cmd, tx_len);
  msg->tx_buf = tx_buf;
  msg->tx_len = tx_len;
  msg->rx_buf = rx_buf;
@@ -384,6 +395,18 @@ static int scpi_send_message(u8 cmd, void *tx_buf,
unsigned int tx_len,
  return ret > 0 ? scpi_to_linux_errno(ret) : ret;
  }

+static int scpi_send_message(u8 cmd, void *tx_buf, unsigned int tx_len,
+     void *rx_buf, unsigned int rx_len)
+{
+ return __scpi_send_message(cmd, tx_buf, tx_len, rx_buf, rx_len, false);
+}
+
+static int scpi_send_ext_message(u8 cmd, void *tx_buf, unsigned int tx_len,
+ void *rx_buf, unsigned int rx_len)
+{
+ return __scpi_send_message(cmd, tx_buf, tx_len, rx_buf, rx_len, true);
+}
+
  static u32 scpi_get_version(void)
  {
  return scpi_info->protocol_version;
@@ -574,6 +597,29 @@ static int scpi_init_versions(struct scpi_drvinfo
*info)
  return ret;
  }

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

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

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