Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751520AbdIENjt (ORCPT ); Tue, 5 Sep 2017 09:39:49 -0400 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:40834 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750731AbdIENjn (ORCPT ); Tue, 5 Sep 2017 09:39:43 -0400 Subject: Re: [PATCH v2 04/18] firmware: arm_scmi: add common infrastructure and support for base protocol To: Sudeep Holla , ALKML , LKML , DTML Cc: Nishanth Menon , Harb Abdulhamid , Arnd Bergmann , Jassi Brar , Ryan Harkin , Roy Franz , Loc Ho , Alexey Klimov References: <1501857104-11279-1-git-send-email-sudeep.holla@arm.com> <1501857104-11279-5-git-send-email-sudeep.holla@arm.com> From: Julien Thierry Message-ID: <9a6b0722-1299-fb0a-6c75-9aceb248659f@arm.com> Date: Tue, 5 Sep 2017 14:39:38 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <1501857104-11279-5-git-send-email-sudeep.holla@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 14039 Lines: 442 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 > Signed-off-by: Sudeep Holla > --- > 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 . > + */ > + > +#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 > +#include > +#include > +#include > #include > #include > > +#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