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

[Sudeep Holla]

Hi Neil,

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

Agreed.

> - does not specify a strong "standard"

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

> - does not specify a strong MHU interface specification
>

Not really required, any mailbox must do.

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

I assume you mean command index here

> new messages types,

Also fine with extended command set.

> different messages data format or

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

> different MHU communication scheme.

Not a problem as I mentioned above.

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

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

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

-- 
Regards,
Sudeep

-->8

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

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

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

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

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

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

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

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

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

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

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