Date:   Tue, 5 Sep 2017 14:39:38 +0100
From:   Julien Thierry <julien.thierry@....com>
To:     Sudeep Holla <sudeep.holla@....com>,
        ALKML <linux-arm-kernel@...ts.infradead.org>,
        LKML <linux-kernel@...r.kernel.org>,
        DTML <devicetree@...r.kernel.org>
Cc:     Nishanth Menon <nm@...com>, Harb Abdulhamid <harba@...eaurora.org>,
        Arnd Bergmann <arnd@...db.de>,
        Jassi Brar <jassisinghbrar@...il.com>,
        Ryan Harkin <Ryan.Harkin@....com>,
        Roy Franz <roy.franz@...ium.com>, Loc Ho <lho@....com>,
        Alexey Klimov <alexey.klimov@....com>
Subject: Re: [PATCH v2 04/18] firmware: arm_scmi: add common infrastructure
 and support for base protocol

Hi Sudeep,

On 04/08/17 15:31, 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.
> 
> Cc: Arnd Bergmann <arnd@...db.de>
> Signed-off-by: Sudeep Holla <sudeep.holla@....com>
> ---
>   drivers/firmware/arm_scmi/Makefile |   2 +-
>   drivers/firmware/arm_scmi/base.c   | 293 +++++++++++++++++++++++++++++++++++++
>   drivers/firmware/arm_scmi/common.h |  46 ++++++
>   drivers/firmware/arm_scmi/driver.c |  67 +++++++++
>   include/linux/scmi_protocol.h      |  28 ++++
>   5 files changed, 435 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 58e94c95e523..21d01d1d6b9c 100644
> --- a/drivers/firmware/arm_scmi/Makefile
> +++ b/drivers/firmware/arm_scmi/Makefile
> @@ -1,2 +1,2 @@
>   obj-$(CONFIG_ARM_SCMI_PROTOCOL)	= arm_scmi.o
> -arm_scmi-y = driver.o
> +arm_scmi-y = base.o driver.o
> diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
> new file mode 100644
> index 000000000000..9bfffbe95c21
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/base.c
> @@ -0,0 +1,293 @@
> +/*
> + * 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 <http://www.gnu.org/licenses/>.
> + */
> +
> +#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), &t);
> +	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, &t);
> +	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 on success, else appropriate SCMI error.
> + */
> +static int
> +scmi_base_implementation_version_get(const struct scmi_handle *handle)
> +{
> +	int ret;
> +	u32 *impl_ver;
> +	struct scmi_xfer *t;
> +	struct scmi_revision_info *rev = handle->version;
> +
> +	ret = scmi_one_xfer_init(handle, BASE_DISCOVER_IMPLEMENT_VERSION,
> +				 SCMI_PROTOCOL_BASE, 0, sizeof(*impl_ver), &t);
> +	if (ret)
> +		return ret;
> +
> +	ret = scmi_do_xfer(handle, t);
> +	if (!ret) {
> +		impl_ver = t->rx.buf;
> +		rev->impl_ver = le32_to_cpu(*impl_ver);
> +	}
> +
> +	scmi_one_xfer_put(handle, t);
> +	return ret;
> +}
> +
> +/**
> + * scmi_base_implementation_list_get() - gets the list of protocols it is
> + *	OSPM is allowed to access
> + *
> + * @handle - SCMI entity handle
> + * @protocols_imp - pointer to hold the list of protocol identifiers
> + *
> + * Return: 0 on success, else appropriate SCMI error.
> + */
> +static int scmi_base_implementation_list_get(const struct scmi_handle *handle,
> +					     u8 *protocols_imp)
> +{
> +	u8 *list;
> +	int ret, loop;
> +	struct scmi_xfer *t;
> +	__le32 *num_skip, *num_ret;
> +	u32 tot_num_ret = 0, loop_num_ret;
> +	struct device *dev = handle->dev;
> +
> +	ret = scmi_one_xfer_init(handle, BASE_DISCOVER_LIST_PROTOCOLS,
> +				 SCMI_PROTOCOL_BASE, sizeof(*num_skip), 0, &t);
> +	if (ret)
> +		return ret;
> +
> +	num_skip = t->tx.buf;
> +	num_ret = t->rx.buf;
> +	list = t->rx.buf + sizeof(*num_ret);
> +
> +	do {
> +		/* Set the number of protocols to be skipped/already read */
> +		*num_skip = cpu_to_le32(tot_num_ret);
> +
> +		ret = scmi_do_xfer(handle, t);
> +		if (ret)
> +			break;
> +
> +		loop_num_ret = le32_to_cpu(*num_ret);
> +		if (tot_num_ret + loop_num_ret > MAX_PROTOCOLS_IMP) {
> +			dev_err(dev, "No. of Protocol > MAX_PROTOCOLS_IMP");
> +			break;
> +		}
> +
> +		for (loop = 0; loop < loop_num_ret; loop++)
> +			protocols_imp[tot_num_ret + loop] = *(list + loop);
> +
> +		tot_num_ret += loop_num_ret;
> +	} while (loop_num_ret);
> +
> +	scmi_one_xfer_put(handle, t);
> +	return ret;
> +}
> +
> +/**
> + * scmi_base_discover_agent_get() - discover the name of an agent
> + *
> + * @handle - SCMI entity handle
> + * @id - Agent identifier
> + * @name - Agent identifier ASCII string
> + *
> + * An agent id of 0 is reserved to identify the platform itself.
> + * Generally operating system is represented as "OSPM"
> + *
> + * Return: 0 on success, else appropriate SCMI error.
> + */
> +static int scmi_base_discover_agent_get(const struct scmi_handle *handle,
> +					int id, char *name)
> +{
> +	int ret;
> +	struct scmi_xfer *t;
> +
> +	ret = scmi_one_xfer_init(handle, BASE_DISCOVER_AGENT,
> +				 SCMI_PROTOCOL_BASE, sizeof(__le32),
> +				 SCMI_MAX_STR_SIZE, &t);
> +	if (ret)
> +		return ret;
> +
> +	*(__le32 *)t->tx.buf = cpu_to_le32(id);
> +
> +	ret = scmi_do_xfer(handle, t);
> +	if (!ret)
> +		memcpy(name, t->rx.buf, SCMI_MAX_STR_SIZE);
> +
> +	scmi_one_xfer_put(handle, t);
> +	return ret;
> +}
> +
> +/**
> + * scmi_base_error_notifications_enable() - register/unregister for
> + *	notifications of errors in the platform
> + *
> + * @handle - SCMI entity handle
> + * @enable - Enable/Disable the notification
> + *
> + * Return: 0 on success, else appropriate SCMI error.
> + */
> +static int
> +scmi_base_error_notifications_enable(const struct scmi_handle *handle, bool en)
> +{
> +	int ret;
> +	struct scmi_xfer *t;
> +
> +	ret = scmi_one_xfer_init(handle, BASE_NOTIFY_ERRORS, SCMI_PROTOCOL_BASE,
> +				 sizeof(__le32), 0, &t);
> +	if (ret)
> +		return ret;
> +
> +	*(__le32 *)t->tx.buf = cpu_to_le32(en & BIT(0));
> +
> +	ret = scmi_do_xfer(handle, t);
> +
> +	scmi_one_xfer_put(handle, t);
> +	return ret;
> +}
> +
> +int scmi_base_protocol_init(struct scmi_handle *h)
> +{
> +	int id, ret;
> +	u8 *prot_imp;
> +	u32 version;
> +	char name[SCMI_MAX_STR_SIZE];
> +	const struct scmi_handle *handle = h;
> +	struct device *dev = handle->dev;
> +	struct scmi_revision_info *rev = handle->version;
> +
> +	ret = scmi_version_get(handle, SCMI_PROTOCOL_BASE, &version);
> +	if (ret)
> +		return ret;
> +
> +	prot_imp = devm_kcalloc(dev, MAX_PROTOCOLS_IMP, sizeof(u8), GFP_KERNEL);
> +	if (!prot_imp)
> +		return -ENOMEM;
> +
> +	rev->major_ver = PROTOCOL_REV_MAJOR(version),
> +	rev->minor_ver = PROTOCOL_REV_MINOR(version);
> +
> +	scmi_base_attributes_get(handle);
> +	scmi_base_vendor_id_get(handle, false);
> +	scmi_base_vendor_id_get(handle, true);
> +	scmi_base_implementation_version_get(handle);
> +	scmi_base_implementation_list_get(handle, prot_imp);
> +	scmi_base_error_notifications_enable(handle, true);
> +	scmi_setup_protocol_implemented(handle, prot_imp);
> +
> +	dev_info(dev, "SCMI Protocol v%d.%d '%s:%s' Firmware version 0x%x\n",
> +		 rev->major_ver, rev->minor_ver, rev->vendor_id,
> +		 rev->sub_vendor_id, rev->impl_ver);
> +	dev_dbg(dev, "Found %d protocol(s) %d agent(s)\n", rev->num_protocols,
> +		rev->num_agents);
> +
> +	for (id = 0; id < rev->num_agents; id++) {
> +		scmi_base_discover_agent_get(handle, id, name);
> +		dev_dbg(dev, "Agent %d: %s\n", id, name);
> +	}
> +
> +	return 0;
> +}
> diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> index a6829afc17e3..e3fe5d9acc82 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -19,9 +19,50 @@
>    */
>   
>   #include <linux/completion.h>
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
>   #include <linux/scmi_protocol.h>
>   #include <linux/types.h>
>   
> +#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 MAX_PROTOCOLS_IMP	16
> +
> +enum scmi_std_protocol {
> +	SCMI_PROTOCOL_BASE = 0x10,
> +	SCMI_PROTOCOL_POWER = 0x11,
> +	SCMI_PROTOCOL_SYSTEM = 0x12,
> +	SCMI_PROTOCOL_PERF = 0x13,
> +	SCMI_PROTOCOL_CLOCK = 0x14,
> +	SCMI_PROTOCOL_SENSOR = 0x15,
> +};
> +
> +enum scmi_common_cmd {
> +	PROTOCOL_VERSION = 0x0,
> +	PROTOCOL_ATTRIBUTES = 0x1,
> +	PROTOCOL_MESSAGE_ATTRIBUTES = 0x2,
> +};
> +
> +/**
> + * struct scmi_msg_resp_prot_version - Response for a message
> + *
> + * @major_version: Major version of the ABI that firmware supports
> + * @minor_version: Minor version of the ABI that firmware supports
> + *
> + * In general, ABI version changes follow the rule that minor version increments
> + * are backward compatible. Major revision changes in ABI may not be
> + * backward compatible.
> + *
> + * Response to a generic message with message type SCMI_MSG_VERSION
> + */
> +struct scmi_msg_resp_prot_version {
> +	__le16 minor_version;
> +	__le16 major_version;
> +};
> +
>   /**
>    * struct scmi_msg_hdr - Message(Tx/Rx) header
>    *
> @@ -72,3 +113,8 @@ void scmi_one_xfer_put(const struct scmi_handle *h, struct scmi_xfer *xfer);
>   int scmi_do_xfer(const struct scmi_handle *h, struct scmi_xfer *xfer);
>   int scmi_one_xfer_init(const struct scmi_handle *h, u8 msg_id, u8 prot_id,
>   		       size_t tx_size, size_t rx_size, struct scmi_xfer **p);
> +int scmi_version_get(const struct scmi_handle *h, u8 protocol, u32 *version);
> +void scmi_setup_protocol_implemented(const struct scmi_handle *handle,
> +				     u8 *prot_imp);
> +
> +int scmi_base_protocol_init(struct scmi_handle *h);
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 139d6980f270..601d0d7210d9 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -108,18 +108,22 @@ 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
>    * @rx_chan: Receive mailbox channel
>    * @tx_payload: Transmit mailbox channel payload area
>    * @rx_payload: Receive mailbox channel payload area
>    * @minfo: Message info
> + * @protocols_imp: list of protocols implemented
>    * @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;
> @@ -127,6 +131,7 @@ struct scmi_info {
>   	void __iomem *tx_payload;
>   	void __iomem *rx_payload;
>   	struct scmi_xfers_info minfo;
> +	u8 *protocols_imp;

Both the base protocol and driver part rely on this being of size 
MAX_PROTOCOLS_IMP (if existing).

Could this be a "u8 protocols_imp[MAX_PROTOCOLS_IMP]" instead? Or at 
least add a comment making it clearer at the scmi_info definition that 
this array will have MAX_PROTOCOLS_IMP elements in all scmi_info instances?

Other than that, patch seems fine.

Cheers,

-- 
Julien Thierry

Powered by blists - more mailing lists