Re: [PATCH v6 04/20] firmware: arm_scmi: add common infrastructure and support for base protocol

2018-03-05 Thread Jonathan Cameron 
On Fri, 23 Feb 2018 16:23:34 +
Sudeep Holla  wrote:

> The base protocol describes the properties of the implementation and
> provide generic error management. The base protocol provides commands
> to describe protocol version, discover implementation specific
> attributes and vendor/sub-vendor identification, list of protocols
> implemented and the various agents are in the system including OSPM
> and the platform. It also supports registering for notifications of
> platform errors.
> 
> This protocol is mandatory. This patch adds support for the same along
> with some basic infrastructure to add support for other protocols.
> 


Minor comments inline.

> diff --git a/drivers/firmware/arm_scmi/driver.c 
> b/drivers/firmware/arm_scmi/driver.c
> index fa0e9cf31f4c..ba784b8ec170 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -105,21 +105,27 @@ struct scmi_desc {
>   * @dev: Device pointer
>   * @desc: SoC description for this instance
>   * @handle: Instance of SCMI handle to send to clients
> + * @version: SCMI revision information containing protocol version,
> + *   implementation version and (sub-)vendor identification.
>   * @cl: Mailbox Client
>   * @tx_chan: Transmit mailbox channel
>   * @tx_payload: Transmit mailbox channel payload area
>   * @minfo: Message info
> + * @protocols_imp: list of protocols implemented, currently maximum of
> + *   MAX_PROTOCOLS_IMP elements allocated by the base protocol
If it is fixed size, is there a benefit in not just putting it in here
as a fixed size array and simplifying the allocation?

>   * @node: list head
>   * @users: Number of users of this instance
>   */
>  struct scmi_info {
>   struct device *dev;
>   const struct scmi_desc *desc;
> + struct scmi_revision_info version;
>   struct scmi_handle handle;
>   struct mbox_client cl;
>   struct mbox_chan *tx_chan;
>   void __iomem *tx_payload;
>   struct scmi_xfers_info minfo;
> + u8 *protocols_imp;
>   struct list_head node;
>   int users;
>  };
> @@ -433,6 +439,45 @@ int scmi_one_xfer_init(const struct scmi_handle *handle, 
> u8 msg_id, u8 prot_id,
>  }
>  
>  /**
> + * scmi_version_get() - command to get the revision of the SCMI entity
> + *
> + * @handle: Handle to SCMI entity information
> + *
> + * Updates the SCMI information in the internal data structure.
> + *
> + * Return: 0 if all went fine, else return appropriate error.
> + */
> +int scmi_version_get(const struct scmi_handle *handle, u8 protocol,
> +  u32 *version)
> +{
> + int ret;
> + __le32 *rev_info;
> + struct scmi_xfer *t;
> +
> + ret = scmi_one_xfer_init(handle, PROTOCOL_VERSION, protocol, 0,
> +  sizeof(*version), );
> + if (ret)
> + return ret;
> +
> + ret = scmi_do_xfer(handle, t);
> + if (!ret) {
> + rev_info = t->rx.buf;
> + *version = le32_to_cpu(*rev_info);
> + }
> +
> + scmi_one_xfer_put(handle, t);
blank line before all simple returns is common kernel coding style
and does help for readability (a little bit)

> + return ret;
> +}
> +
> +void scmi_setup_protocol_implemented(const struct scmi_handle *handle,
> +  u8 *prot_imp)
> +{
> + struct scmi_info *info = handle_to_scmi_info(handle);
> +
> + info->protocols_imp = prot_imp;
> +}
> +
> +/**
>   * scmi_handle_get() - Get the  SCMI handle for a device
>   *
>   * @dev: pointer to device for which we want SCMI handle
> @@ -660,11 +705,19 @@ static int scmi_probe(struct platform_device *pdev)
>  
>   handle = >handle;
>   handle->dev = info->dev;
> + handle->version = >version;
>  
>   ret = scmi_mbox_chan_setup(info);
>   if (ret)
>   return ret;
>  
> + ret = scmi_base_protocol_init(handle);
> + if (ret) {
> + dev_err(dev, "unable to communicate with SCMI(%d)\n", ret);
> + scmi_mbox_free_channel(info);
> + return ret;
> + }
> +
>   mutex_lock(_list_mutex);
>   list_add_tail(>node, _list);
>   mutex_unlock(_list_mutex);
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index 854ed2479993..664da3d763f2 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -17,11 +17,48 @@
>   */
>  #include 
>  
> +#define SCMI_MAX_STR_SIZE16
> +
> +/**
> + * struct scmi_revision_info - version information structure
This isn't really just the version stuff. It's more about discovery
of what is present.
Perhaps the naming could make that clear if you want to keep this
as one structure?

> + *
> + * @major_ver: Major ABI version. Change here implies risk of backward
> + *   compatibility break.
> + * @minor_ver: Minor ABI version. Change here implies new feature addition,
> + *   or compatible change in ABI.
> + * @num_protocols: Number of protocols that are

Re: [PATCH v6 04/20] firmware: arm_scmi: add common infrastructure and support for base protocol

2018-03-05 Thread Jonathan Cameron 
On Fri, 23 Feb 2018 16:23:34 +
Sudeep Holla  wrote:

> The base protocol describes the properties of the implementation and
> provide generic error management. The base protocol provides commands
> to describe protocol version, discover implementation specific
> attributes and vendor/sub-vendor identification, list of protocols
> implemented and the various agents are in the system including OSPM
> and the platform. It also supports registering for notifications of
> platform errors.
> 
> This protocol is mandatory. This patch adds support for the same along
> with some basic infrastructure to add support for other protocols.
> 


Minor comments inline.

> diff --git a/drivers/firmware/arm_scmi/driver.c 
> b/drivers/firmware/arm_scmi/driver.c
> index fa0e9cf31f4c..ba784b8ec170 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -105,21 +105,27 @@ struct scmi_desc {
>   * @dev: Device pointer
>   * @desc: SoC description for this instance
>   * @handle: Instance of SCMI handle to send to clients
> + * @version: SCMI revision information containing protocol version,
> + *   implementation version and (sub-)vendor identification.
>   * @cl: Mailbox Client
>   * @tx_chan: Transmit mailbox channel
>   * @tx_payload: Transmit mailbox channel payload area
>   * @minfo: Message info
> + * @protocols_imp: list of protocols implemented, currently maximum of
> + *   MAX_PROTOCOLS_IMP elements allocated by the base protocol
If it is fixed size, is there a benefit in not just putting it in here
as a fixed size array and simplifying the allocation?

>   * @node: list head
>   * @users: Number of users of this instance
>   */
>  struct scmi_info {
>   struct device *dev;
>   const struct scmi_desc *desc;
> + struct scmi_revision_info version;
>   struct scmi_handle handle;
>   struct mbox_client cl;
>   struct mbox_chan *tx_chan;
>   void __iomem *tx_payload;
>   struct scmi_xfers_info minfo;
> + u8 *protocols_imp;
>   struct list_head node;
>   int users;
>  };
> @@ -433,6 +439,45 @@ int scmi_one_xfer_init(const struct scmi_handle *handle, 
> u8 msg_id, u8 prot_id,
>  }
>  
>  /**
> + * scmi_version_get() - command to get the revision of the SCMI entity
> + *
> + * @handle: Handle to SCMI entity information
> + *
> + * Updates the SCMI information in the internal data structure.
> + *
> + * Return: 0 if all went fine, else return appropriate error.
> + */
> +int scmi_version_get(const struct scmi_handle *handle, u8 protocol,
> +  u32 *version)
> +{
> + int ret;
> + __le32 *rev_info;
> + struct scmi_xfer *t;
> +
> + ret = scmi_one_xfer_init(handle, PROTOCOL_VERSION, protocol, 0,
> +  sizeof(*version), );
> + if (ret)
> + return ret;
> +
> + ret = scmi_do_xfer(handle, t);
> + if (!ret) {
> + rev_info = t->rx.buf;
> + *version = le32_to_cpu(*rev_info);
> + }
> +
> + scmi_one_xfer_put(handle, t);
blank line before all simple returns is common kernel coding style
and does help for readability (a little bit)

> + return ret;
> +}
> +
> +void scmi_setup_protocol_implemented(const struct scmi_handle *handle,
> +  u8 *prot_imp)
> +{
> + struct scmi_info *info = handle_to_scmi_info(handle);
> +
> + info->protocols_imp = prot_imp;
> +}
> +
> +/**
>   * scmi_handle_get() - Get the  SCMI handle for a device
>   *
>   * @dev: pointer to device for which we want SCMI handle
> @@ -660,11 +705,19 @@ static int scmi_probe(struct platform_device *pdev)
>  
>   handle = >handle;
>   handle->dev = info->dev;
> + handle->version = >version;
>  
>   ret = scmi_mbox_chan_setup(info);
>   if (ret)
>   return ret;
>  
> + ret = scmi_base_protocol_init(handle);
> + if (ret) {
> + dev_err(dev, "unable to communicate with SCMI(%d)\n", ret);
> + scmi_mbox_free_channel(info);
> + return ret;
> + }
> +
>   mutex_lock(_list_mutex);
>   list_add_tail(>node, _list);
>   mutex_unlock(_list_mutex);
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index 854ed2479993..664da3d763f2 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -17,11 +17,48 @@
>   */
>  #include 
>  
> +#define SCMI_MAX_STR_SIZE16
> +
> +/**
> + * struct scmi_revision_info - version information structure
This isn't really just the version stuff. It's more about discovery
of what is present.
Perhaps the naming could make that clear if you want to keep this
as one structure?

> + *
> + * @major_ver: Major ABI version. Change here implies risk of backward
> + *   compatibility break.
> + * @minor_ver: Minor ABI version. Change here implies new feature addition,
> + *   or compatible change in ABI.
> + * @num_protocols: Number of protocols that are implemented, excluding the
> + *

[PATCH v6 04/20] firmware: arm_scmi: add common infrastructure and support for base protocol

2018-02-23 Thread Sudeep Holla 
The base protocol describes the properties of the implementation and
provide generic error management. The base protocol provides commands
to describe protocol version, discover implementation specific
attributes and vendor/sub-vendor identification, list of protocols
implemented and the various agents are in the system including OSPM
and the platform. It also supports registering for notifications of
platform errors.

This protocol is mandatory. This patch adds support for the same along
with some basic infrastructure to add support for other protocols.

Cc: Arnd Bergmann 
Cc: Greg Kroah-Hartman 
Signed-off-by: Sudeep Holla 
---
 drivers/firmware/arm_scmi/Makefile |   3 +-
 drivers/firmware/arm_scmi/base.c   | 264 +
 drivers/firmware/arm_scmi/common.h |  37 ++
 drivers/firmware/arm_scmi/driver.c |  53 
 include/linux/scmi_protocol.h  |  37 ++
 5 files changed, 393 insertions(+), 1 deletion(-)
 create mode 100644 drivers/firmware/arm_scmi/base.c

diff --git a/drivers/firmware/arm_scmi/Makefile 
b/drivers/firmware/arm_scmi/Makefile
index b2a24ba2b636..5d9c7ef35f0f 100644
--- a/drivers/firmware/arm_scmi/Makefile
+++ b/drivers/firmware/arm_scmi/Makefile
@@ -1,2 +1,3 @@
-obj-y  = scmi-driver.o
+obj-y  = scmi-driver.o scmi-protocols.o
 scmi-driver-y = driver.o
+scmi-protocols-y = base.o
diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
new file mode 100644
index ..708a21f85a12
--- /dev/null
+++ b/drivers/firmware/arm_scmi/base.c
@@ -0,0 +1,264 @@
+/*
+ * System Control and Management Interface (SCMI) Base Protocol
+ *
+ * Copyright (C) 2017 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 .
+ */
+
+#include "common.h"
+
+enum scmi_base_protocol_cmd {
+   BASE_DISCOVER_VENDOR = 0x3,
+   BASE_DISCOVER_SUB_VENDOR = 0x4,
+   BASE_DISCOVER_IMPLEMENT_VERSION = 0x5,
+   BASE_DISCOVER_LIST_PROTOCOLS = 0x6,
+   BASE_DISCOVER_AGENT = 0x7,
+   BASE_NOTIFY_ERRORS = 0x8,
+};
+
+struct scmi_msg_resp_base_attributes {
+   u8 num_protocols;
+   u8 num_agents;
+   __le16 reserved;
+};
+
+/**
+ * scmi_base_attributes_get() - gets the implementation details
+ * that are associated with the base protocol.
+ *
+ * @handle - SCMI entity handle
+ *
+ * Return: 0 on success, else appropriate SCMI error.
+ */
+static int scmi_base_attributes_get(const struct scmi_handle *handle)
+{
+   int ret;
+   struct scmi_xfer *t;
+   struct scmi_msg_resp_base_attributes *attr_info;
+   struct scmi_revision_info *rev = handle->version;
+
+   ret = scmi_one_xfer_init(handle, PROTOCOL_ATTRIBUTES,
+SCMI_PROTOCOL_BASE, 0, sizeof(*attr_info), );
+   if (ret)
+   return ret;
+
+   ret = scmi_do_xfer(handle, t);
+   if (!ret) {
+   attr_info = t->rx.buf;
+   rev->num_protocols = attr_info->num_protocols;
+   rev->num_agents = attr_info->num_agents;
+   }
+
+   scmi_one_xfer_put(handle, t);
+   return ret;
+}
+
+/**
+ * scmi_base_vendor_id_get() - gets vendor/subvendor identifier ASCII string.
+ *
+ * @handle - SCMI entity handle
+ * @sub_vendor - specify true if sub-vendor ID is needed
+ *
+ * Return: 0 on success, else appropriate SCMI error.
+ */
+static int
+scmi_base_vendor_id_get(const struct scmi_handle *handle, bool sub_vendor)
+{
+   u8 cmd;
+   int ret, size;
+   char *vendor_id;
+   struct scmi_xfer *t;
+   struct scmi_revision_info *rev = handle->version;
+
+   if (sub_vendor) {
+   cmd = BASE_DISCOVER_SUB_VENDOR;
+   vendor_id = rev->sub_vendor_id;
+   size = ARRAY_SIZE(rev->sub_vendor_id);
+   } else {
+   cmd = BASE_DISCOVER_VENDOR;
+   vendor_id = rev->vendor_id;
+   size = ARRAY_SIZE(rev->vendor_id);
+   }
+
+   ret = scmi_one_xfer_init(handle, cmd, SCMI_PROTOCOL_BASE, 0, size, );
+   if (ret)
+   return ret;
+
+   ret = scmi_do_xfer(handle, t);
+   if (!ret)
+   memcpy(vendor_id, t->rx.buf, size);
+
+   scmi_one_xfer_put(handle, t);
+   return ret;
+}
+
+/**
+ * scmi_base_implementation_version_get() - gets a vendor-specific
+ * implementation 32-bit version. The format of the version number is
+ *

[PATCH v6 04/20] firmware: arm_scmi: add common infrastructure and support for base protocol

2018-02-23 Thread Sudeep Holla 
The base protocol describes the properties of the implementation and
provide generic error management. The base protocol provides commands
to describe protocol version, discover implementation specific
attributes and vendor/sub-vendor identification, list of protocols
implemented and the various agents are in the system including OSPM
and the platform. It also supports registering for notifications of
platform errors.

This protocol is mandatory. This patch adds support for the same along
with some basic infrastructure to add support for other protocols.

Cc: Arnd Bergmann 
Cc: Greg Kroah-Hartman 
Signed-off-by: Sudeep Holla 
---
 drivers/firmware/arm_scmi/Makefile |   3 +-
 drivers/firmware/arm_scmi/base.c   | 264 +
 drivers/firmware/arm_scmi/common.h |  37 ++
 drivers/firmware/arm_scmi/driver.c |  53 
 include/linux/scmi_protocol.h  |  37 ++
 5 files changed, 393 insertions(+), 1 deletion(-)
 create mode 100644 drivers/firmware/arm_scmi/base.c

diff --git a/drivers/firmware/arm_scmi/Makefile 
b/drivers/firmware/arm_scmi/Makefile
index b2a24ba2b636..5d9c7ef35f0f 100644
--- a/drivers/firmware/arm_scmi/Makefile
+++ b/drivers/firmware/arm_scmi/Makefile
@@ -1,2 +1,3 @@
-obj-y  = scmi-driver.o
+obj-y  = scmi-driver.o scmi-protocols.o
 scmi-driver-y = driver.o
+scmi-protocols-y = base.o
diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
new file mode 100644
index ..708a21f85a12
--- /dev/null
+++ b/drivers/firmware/arm_scmi/base.c
@@ -0,0 +1,264 @@
+/*
+ * System Control and Management Interface (SCMI) Base Protocol
+ *
+ * Copyright (C) 2017 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 .
+ */
+
+#include "common.h"
+
+enum scmi_base_protocol_cmd {
+   BASE_DISCOVER_VENDOR = 0x3,
+   BASE_DISCOVER_SUB_VENDOR = 0x4,
+   BASE_DISCOVER_IMPLEMENT_VERSION = 0x5,
+   BASE_DISCOVER_LIST_PROTOCOLS = 0x6,
+   BASE_DISCOVER_AGENT = 0x7,
+   BASE_NOTIFY_ERRORS = 0x8,
+};
+
+struct scmi_msg_resp_base_attributes {
+   u8 num_protocols;
+   u8 num_agents;
+   __le16 reserved;
+};
+
+/**
+ * scmi_base_attributes_get() - gets the implementation details
+ * that are associated with the base protocol.
+ *
+ * @handle - SCMI entity handle
+ *
+ * Return: 0 on success, else appropriate SCMI error.
+ */
+static int scmi_base_attributes_get(const struct scmi_handle *handle)
+{
+   int ret;
+   struct scmi_xfer *t;
+   struct scmi_msg_resp_base_attributes *attr_info;
+   struct scmi_revision_info *rev = handle->version;
+
+   ret = scmi_one_xfer_init(handle, PROTOCOL_ATTRIBUTES,
+SCMI_PROTOCOL_BASE, 0, sizeof(*attr_info), );
+   if (ret)
+   return ret;
+
+   ret = scmi_do_xfer(handle, t);
+   if (!ret) {
+   attr_info = t->rx.buf;
+   rev->num_protocols = attr_info->num_protocols;
+   rev->num_agents = attr_info->num_agents;
+   }
+
+   scmi_one_xfer_put(handle, t);
+   return ret;
+}
+
+/**
+ * scmi_base_vendor_id_get() - gets vendor/subvendor identifier ASCII string.
+ *
+ * @handle - SCMI entity handle
+ * @sub_vendor - specify true if sub-vendor ID is needed
+ *
+ * Return: 0 on success, else appropriate SCMI error.
+ */
+static int
+scmi_base_vendor_id_get(const struct scmi_handle *handle, bool sub_vendor)
+{
+   u8 cmd;
+   int ret, size;
+   char *vendor_id;
+   struct scmi_xfer *t;
+   struct scmi_revision_info *rev = handle->version;
+
+   if (sub_vendor) {
+   cmd = BASE_DISCOVER_SUB_VENDOR;
+   vendor_id = rev->sub_vendor_id;
+   size = ARRAY_SIZE(rev->sub_vendor_id);
+   } else {
+   cmd = BASE_DISCOVER_VENDOR;
+   vendor_id = rev->vendor_id;
+   size = ARRAY_SIZE(rev->vendor_id);
+   }
+
+   ret = scmi_one_xfer_init(handle, cmd, SCMI_PROTOCOL_BASE, 0, size, );
+   if (ret)
+   return ret;
+
+   ret = scmi_do_xfer(handle, t);
+   if (!ret)
+   memcpy(vendor_id, t->rx.buf, size);
+
+   scmi_one_xfer_put(handle, t);
+   return ret;
+}
+
+/**
+ * scmi_base_implementation_version_get() - gets a vendor-specific
+ * implementation 32-bit version. The format of the version number is
+ * vendor-specific
+ *
+ * @handle - SCMI entity handle
+ *
+ * Return: 0