diff mbox

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

Message ID 57472450.4000709@arm.com
State New
Headers show

Commit Message

Sudeep Holla May 26, 2016, 4:29 p.m. UTC 
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

Comments

Sudeep Holla June 1, 2016, 10:10 a.m. UTC | #1 
On 30/05/16 09:30, Neil Armstrong wrote:
> On 05/27/2016 10:17 AM, Neil Armstrong wrote:


[..]

>

> While looking for other ARMv8 based platform, I found that the RK3368

> platform has the same SCPI implementation as Amlogic.

>

> They extended it with DDR, system and thermal commands.

>

> Look at :

> https://github.com/geekboxzone/mmallow_kernel/blob/geekbox/drivers/mailbox/scpi_cmd.h

>

>https://github.com/geekboxzone/mmallow_kernel/blob/geekbox/drivers/mailbox/scpi_protocol.c

>



> So the SCPI must have a framework to allow different protocol

> versions, and must allow command extension. Grouping Rockchip and

> Amlogic should be done, thus needing a generic name like vendor_scpi

> or with a version.

>


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

> Sudeep, could you somehow find out which version of the protocol

> AmLogic and Rockchip based their SCPI development ?

>


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

-- 
Regards,
Sudeep
Sudeep Holla June 1, 2016, 4:34 p.m. UTC | #2 
On 01/06/16 17:30, Kevin Hilman wrote:
> [ + Heiko, who may know about the Rockchip implementation ]

>

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

>

>> On 30/05/16 09:30, Neil Armstrong wrote:

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

>>

>> [..]

>>

>>>

>>> While looking for other ARMv8 based platform, I found that the RK3368

>>> platform has the same SCPI implementation as Amlogic.

>>>

>>> They extended it with DDR, system and thermal commands.

>>>

>>> Look at :

>>> https://github.com/geekboxzone/mmallow_kernel/blob/geekbox/drivers/mailbox/scpi_cmd.h

>>>

>>> https://github.com/geekboxzone/mmallow_kernel/blob/geekbox/drivers/mailbox/scpi_protocol.c

>>>

>>

>>

>>> So the SCPI must have a framework to allow different protocol

>>> versions, and must allow command extension. Grouping Rockchip and

>>> Amlogic should be done, thus needing a generic name like vendor_scpi

>>> or with a version.

>>>

>>

>> Makes sense. I understand the need to reuse and I need a bit of time to

>> have a look at the code(both Amlogic one's you have pointed out and the

>> Rockchip one) in detail to see what's the best way to proceed. I will

>> have a look at this later this week and get back to you.

>>

>>> Sudeep, could you somehow find out which version of the protocol

>>> AmLogic and Rockchip based their SCPI development ?

>>>

>>

>> Yes I tried checking with Rockchip but didn't get a response. But my

>> guess is that it was some preliminary unpublished version of SCPI

>> unfortunately :(

>

> And if one partner did that, probably everyone else did as well, but

> this being the ARM universe, they all did it slightly differently. :(

>


No doubt :)

> We know from experience, that this happens all the time in the absence

> of a clear standard, so this framework will need to be extended to be

> useful.

>


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

-- 
Regards,
Sudeep
Sudeep Holla June 6, 2016, 5:10 p.m. UTC | #3 
On 30/05/16 09:30, Neil Armstrong wrote:
> On 05/27/2016 10:17 AM, Neil Armstrong wrote:


>

> While looking for other ARMv8 based platform, I found that the RK3368

> platform has the same SCPI implementation as Amlogic.

>

> They extended it with DDR, system and thermal commands.

>

> Look at :

> https://github.com/geekboxzone/mmallow_kernel/blob/geekbox/drivers/mailbox/scpi_cmd.h

>

>https://github.com/geekboxzone/mmallow_kernel/blob/geekbox/driver/mailbox/scpi_protocol.c

>

> So the SCPI must have a framework to allow different protocol

> versions, and must allow command extension. Grouping Rockchip and

> Amlogic should be done, thus needing a generic name like vendor_scpi

> or with a version.

>

> Sudeep, could you somehow find out which version of the protocol

> AmLogic and Rockchip based their SCPI development ?

>

>


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

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

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

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

-- 
Regards,
Sudeep
diff mbox

Patch

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