Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751089AbdIEKDR (ORCPT ); Tue, 5 Sep 2017 06:03:17 -0400 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:37216 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750741AbdIEKDN (ORCPT ); Tue, 5 Sep 2017 06:03:13 -0400 From: Julien Thierry Subject: Re: [PATCH v2 03/18] firmware: arm_scmi: add basic driver infrastructure for SCMI 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-4-git-send-email-sudeep.holla@arm.com> Message-ID: Date: Tue, 5 Sep 2017 11:03:08 +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-4-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: 19685 Lines: 552 Hi Sudeep, I am not sure what the patch does is correct when having a big endian kernel dealing with scmi_shared_mem. Unless there is a reason not to have SCMI with big endian kernel, please see remarks below. On 04/08/17 15:31, Sudeep Holla wrote: > The SCMI is intended to allow OSPM to manage various functions that are > provided by the hardware platform it is running on, including power and > performance functions. SCMI provides two levels of abstraction, protocols > and transports. Protocols define individual groups of system control and > management messages. A protocol specification describes the messages > that it supports. Transports describe the method by which protocol > messages are communicated between agents and the platform. > > This patch adds basic infrastructure to manage the message allocation, > initialisation, packing/unpacking and shared memory management. > > Cc: Arnd Bergmann > Signed-off-by: Sudeep Holla > --- > drivers/firmware/Kconfig | 21 + > drivers/firmware/Makefile | 1 + > drivers/firmware/arm_scmi/Makefile | 2 + > drivers/firmware/arm_scmi/common.h | 74 ++++ > drivers/firmware/arm_scmi/driver.c | 774 +++++++++++++++++++++++++++++++++++++ > include/linux/scmi_protocol.h | 48 +++ > 6 files changed, 920 insertions(+) > create mode 100644 drivers/firmware/arm_scmi/Makefile > create mode 100644 drivers/firmware/arm_scmi/common.h > create mode 100644 drivers/firmware/arm_scmi/driver.c > create mode 100644 include/linux/scmi_protocol.h > > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig > index 6e4ed5a9c6fd..c3d1a12763ce 100644 > --- a/drivers/firmware/Kconfig > +++ b/drivers/firmware/Kconfig > @@ -19,6 +19,27 @@ config ARM_PSCI_CHECKER > on and off through hotplug, so for now torture tests and PSCI checker > are mutually exclusive. > > +config ARM_SCMI_PROTOCOL > + tristate "ARM System Control and Management Interface (SCMI) Message Protocol" > + depends on ARM || ARM64 || COMPILE_TEST > + depends on MAILBOX > + help > + ARM System Control and Management Interface (SCMI) protocol is a > + set of operating system-independent software interfaces that are > + used in system management. SCMI is extensible and currently provides > + interfaces for: Discovery and self-description of the interfaces > + it supports, Power domain management which is the ability to place > + a given device or domain into the various power-saving states that > + it supports, Performance management which is the ability to control > + the performance of a domain that is composed of compute engines > + such as application processors and other accelerators, Clock > + management which is the ability to set and inquire rates on platform > + managed clocks and Sensor management which is the ability to read > + sensor data, and be notified of sensor value. > + > + This protocol library provides interface for all the client drivers > + making use of the features offered by the SCMI. > + > config ARM_SCPI_PROTOCOL > tristate "ARM System Control and Power Interface (SCPI) Message Protocol" > depends on ARM || ARM64 || COMPILE_TEST > diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile > index a37f12e8d137..91d3ff62c653 100644 > --- a/drivers/firmware/Makefile > +++ b/drivers/firmware/Makefile > @@ -23,6 +23,7 @@ obj-$(CONFIG_QCOM_SCM_32) += qcom_scm-32.o > CFLAGS_qcom_scm-32.o :=$(call as-instr,.arch armv7-a\n.arch_extension sec,-DREQUIRES_SEC=1) -march=armv7-a > obj-$(CONFIG_TI_SCI_PROTOCOL) += ti_sci.o > > +obj-$(CONFIG_ARM_SCMI_PROTOCOL) += arm_scmi/ > obj-y += broadcom/ > obj-y += meson/ > obj-$(CONFIG_GOOGLE_FIRMWARE) += google/ > diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile > new file mode 100644 > index 000000000000..58e94c95e523 > --- /dev/null > +++ b/drivers/firmware/arm_scmi/Makefile > @@ -0,0 +1,2 @@ > +obj-$(CONFIG_ARM_SCMI_PROTOCOL) = arm_scmi.o > +arm_scmi-y = driver.o > diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h > new file mode 100644 > index 000000000000..a6829afc17e3 > --- /dev/null > +++ b/drivers/firmware/arm_scmi/common.h > @@ -0,0 +1,74 @@ > +/* > + * System Control and Management Interface (SCMI) Message Protocol > + * driver common header file containing some definitions, structures > + * and function prototypes used in all the different SCMI protocols. > + * > + * 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 > +#include > +#include > + > +/** > + * struct scmi_msg_hdr - Message(Tx/Rx) header > + * > + * @id: The identifier of the command being sent > + * @protocol_id: The identifier of the protocol used to send @id command > + * @seq: The token to identify the message. when a message/command returns, > + * the platform returns the whole message header unmodified including > + * the token. > + */ > +struct scmi_msg_hdr { > + u8 id; > + u8 protocol_id; > + u16 seq; > + u32 status; > + bool poll_completion; > +}; > + > +/** > + * struct scmi_msg - Message(Tx/Rx) structure > + * > + * @buf: Buffer pointer > + * @len: Length of data in the Buffer > + */ > +struct scmi_msg { > + void *buf; > + size_t len; > +}; > + > +/** > + * struct scmi_xfer - Structure representing a message flow > + * > + * @hdr: Transmit message header > + * @tx: Transmit message > + * @rx: Receive message, the buffer should be pre-allocated to store > + * message. If request-ACK protocol is used, we can reuse the same > + * buffer for the rx path as we use for the tx path. > + * @done: completion event > + */ > + > +struct scmi_xfer { > + struct scmi_msg_hdr hdr; > + struct scmi_msg tx; > + struct scmi_msg rx; > + struct completion done; > +}; > + > +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); > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c > new file mode 100644 > index 000000000000..139d6980f270 > --- /dev/null > +++ b/drivers/firmware/arm_scmi/driver.c > @@ -0,0 +1,774 @@ > +/* > + * System Control and Management Interface (SCMI) Message Protocol driver > + * > + * SCMI Message Protocol is used between the System Control Processor(SCP) > + * and the Application Processors(AP). The Message Handling Unit(MHU) > + * provides a mechanism for inter-processor communication between SCP's > + * Cortex M3 and AP. > + * > + * SCP offers control and management of the core/cluster power states, > + * various power domain DVFS including the core/cluster, certain system > + * clocks configuration, thermal sensors and many others. > + * > + * 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "common.h" > + > +#define MSG_ID_SHIFT 0 > +#define MSG_ID_MASK 0xff > +#define MSG_TYPE_SHIFT 8 > +#define MSG_TYPE_MASK 0x3 > +#define MSG_PROTOCOL_ID_SHIFT 10 > +#define MSG_PROTOCOL_ID_MASK 0xff > +#define MSG_TOKEN_ID_SHIFT 18 > +#define MSG_TOKEN_ID_MASK 0x3ff > +#define MSG_XTRACT_TOKEN(header) \ > + (((header) >> MSG_TOKEN_ID_SHIFT) & MSG_TOKEN_ID_MASK) > + > +enum scmi_error_codes { > + SCMI_SUCCESS = 0, /* Success */ > + SCMI_ERR_SUPPORT = -1, /* Not supported */ > + SCMI_ERR_PARAMS = -2, /* Invalid Parameters */ > + SCMI_ERR_ACCESS = -3, /* Invalid access/permission denied */ > + SCMI_ERR_ENTRY = -4, /* Not found */ > + SCMI_ERR_RANGE = -5, /* Value out of range */ > + SCMI_ERR_BUSY = -6, /* Device busy */ > + SCMI_ERR_COMMS = -7, /* Communication Error */ > + SCMI_ERR_GENERIC = -8, /* Generic Error */ > + SCMI_ERR_HARDWARE = -9, /* Hardware Error */ > + SCMI_ERR_PROTOCOL = -10,/* Protocol Error */ > + SCMI_ERR_MAX > +}; > + > +/* List of all SCMI devices active in system */ > +static LIST_HEAD(scmi_list); > +/* Protection for the entire list */ > +static DEFINE_MUTEX(scmi_list_mutex); > + > +/** > + * struct scmi_xfers_info - Structure to manage transfer information > + * > + * @sem_xfer_count: Counting Semaphore for managing max simultaneous > + * Messages. > + * @xfer_block: Preallocated Message array > + * @xfer_alloc_table: Bitmap table for allocated messages. > + * Index of this bitmap table is also used for message > + * sequence identifier. > + * @xfer_lock: Protection for message allocation > + */ > +struct scmi_xfers_info { > + struct semaphore sem_xfer_count; > + struct scmi_xfer *xfer_block; > + unsigned long *xfer_alloc_table; > + /* protect transfer allocation */ > + spinlock_t xfer_lock; > +}; > + > +/** > + * struct scmi_desc - Description of SoC integration > + * > + * @max_rx_timeout_ms: Timeout for communication with SoC (in Milliseconds) > + * @max_msg: Maximum number of messages that can be pending > + * simultaneously in the system > + * @max_msg_size: Maximum size of data per message that can be handled. > + */ > +struct scmi_desc { > + int max_rx_timeout_ms; > + int max_msg; > + int max_msg_size; > +}; > + > +/** > + * struct scmi_info - Structure representing a SCMI instance > + * > + * @dev: Device pointer > + * @desc: SoC description for this instance > + * @handle: Instance of SCMI handle to send to clients > + * @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 > + * @node: list head > + * @users: Number of users of this instance > + */ > +struct scmi_info { > + struct device *dev; > + const struct scmi_desc *desc; > + struct scmi_handle handle; > + struct mbox_client cl; > + struct mbox_chan *tx_chan; > + struct mbox_chan *rx_chan; > + void __iomem *tx_payload; > + void __iomem *rx_payload; > + struct scmi_xfers_info minfo; > + struct list_head node; > + int users; > +}; > + > +#define client_to_scmi_info(c) container_of(c, struct scmi_info, cl) > +#define handle_to_scmi_info(h) container_of(h, struct scmi_info, handle) > + > +/* > + * The SCP firmware providing SCM interface to OSPM and other agents must > + * execute only in little-endian mode as per SCMI specification, so any buffers > + * shared through SCMI should have their contents converted to little-endian > + */ > +struct scmi_shared_mem { > + __le32 reserved; > + __le32 channel_status; > +#define SCMI_SHMEM_CHAN_STAT_CHANNEL_ERROR BIT(1) > +#define SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE BIT(0) > + __le32 reserved1[2]; > + __le32 flags; > +#define SCMI_SHMEM_FLAG_INTR_ENABLED BIT(0) > + __le32 length; > + __le32 msg_header; > + u8 msg_payload[0]; > +}; > + > +static int scmi_linux_errmap[] = { > + /* better than switch case as long as return value is continuous */ > + 0, /* SCMI_SUCCESS */ > + -EOPNOTSUPP, /* SCMI_ERR_SUPPORT */ > + -EINVAL, /* SCMI_ERR_PARAM */ > + -EACCES, /* SCMI_ERR_ACCESS */ > + -ENOENT, /* SCMI_ERR_ENTRY */ > + -ERANGE, /* SCMI_ERR_RANGE */ > + -EBUSY, /* SCMI_ERR_BUSY */ > + -ECOMM, /* SCMI_ERR_COMMS */ > + -EIO, /* SCMI_ERR_GENERIC */ > + -EREMOTEIO, /* SCMI_ERR_HARDWARE */ > + -EPROTO, /* SCMI_ERR_PROTOCOL */ > +}; > + > +static inline int scmi_to_linux_errno(int errno) > +{ > + if (errno < SCMI_SUCCESS && errno > SCMI_ERR_MAX) > + return scmi_linux_errmap[-errno]; > + return -EIO; > +} > + > +/** > + * scmi_dump_header_dbg() - Helper to dump a message header. > + * > + * @dev: Device pointer corresponding to the SCMI entity > + * @hdr: pointer to header. > + */ > +static inline void scmi_dump_header_dbg(struct device *dev, > + struct scmi_msg_hdr *hdr) > +{ > + dev_dbg(dev, "Command ID: %x Sequence ID: %x Protocol: %x\n", > + hdr->id, hdr->seq, hdr->protocol_id); > +} > + > +static void scmi_fetch_response(struct scmi_xfer *xfer, > + struct scmi_shared_mem *mem) > +{ > + xfer->hdr.status = le32_to_cpu(*(__le32 *)mem->msg_payload); > + /* Skip the length of header and statues in payload area i.e 8 bytes*/ > + xfer->rx.len = min_t(size_t, xfer->rx.len, mem->length - 8); > + > + /* Take a copy to the rx buffer.. */ > + memcpy_fromio(xfer->rx.buf, mem->msg_payload + 4, xfer->rx.len); > +} > + > +/** > + * scmi_rx_callback() - mailbox client callback for receive messages > + * > + * @cl: client pointer > + * @m: mailbox message > + * > + * Processes one received message to appropriate transfer information and > + * signals completion of the transfer. > + * > + * NOTE: This function will be invoked in IRQ context, hence should be > + * as optimal as possible. > + */ > +static void scmi_rx_callback(struct mbox_client *cl, void *m) > +{ > + u16 xfer_id; > + struct scmi_xfer *xfer; > + struct scmi_info *info = client_to_scmi_info(cl); > + struct scmi_xfers_info *minfo = &info->minfo; > + struct device *dev = info->dev; > + struct scmi_shared_mem *mem = info->tx_payload; > + > + xfer_id = MSG_XTRACT_TOKEN(mem->msg_header); > + Don't we need to convert msg_header to cpu endian? > + /* > + * Are we even expecting this? > + */ > + if (!test_bit(xfer_id, minfo->xfer_alloc_table)) { > + dev_err(dev, "message for %d is not expected!\n", xfer_id); > + return; > + } > + > + xfer = &minfo->xfer_block[xfer_id]; > + > + scmi_dump_header_dbg(dev, &xfer->hdr); > + /* Is the message of valid length? */ > + if (xfer->rx.len > info->desc->max_msg_size) { > + dev_err(dev, "unable to handle %zu xfer(max %d)\n", > + xfer->rx.len, info->desc->max_msg_size); > + return; > + } > + > + scmi_fetch_response(xfer, mem); > + complete(&xfer->done); > +} > + > +/** > + * pack_scmi_header() - packs and returns 32-bit header > + * > + * @hdr: pointer to header containing all the information on message id, > + * protocol id and sequence id. > + */ > +static inline u32 pack_scmi_header(struct scmi_msg_hdr *hdr) > +{ > + return ((hdr->id & MSG_ID_MASK) << MSG_ID_SHIFT) | > + ((hdr->seq & MSG_TOKEN_ID_MASK) << MSG_TOKEN_ID_SHIFT) | > + ((hdr->protocol_id & MSG_PROTOCOL_ID_MASK) << MSG_PROTOCOL_ID_SHIFT); > +} > + > +/** > + * scmi_tx_prepare() - mailbox client callback to prepare for the transfer > + * > + * @cl: client pointer > + * @m: mailbox message > + * > + * This function prepares the shared memory which contains the header and the > + * payload. > + */ > +static void scmi_tx_prepare(struct mbox_client *cl, void *m) > +{ > + struct scmi_xfer *t = m; > + struct scmi_info *info = client_to_scmi_info(cl); > + struct scmi_shared_mem *mem = info->tx_payload; > + > + mem->channel_status = 0x0; /* Mark channel busy + clear error */ > + mem->flags = t->hdr.poll_completion ? 0 : SCMI_SHMEM_FLAG_INTR_ENABLED; > + mem->length = sizeof(mem->msg_header) + t->tx.len; > + mem->msg_header = cpu_to_le32(pack_scmi_header(&t->hdr)); > + if (t->tx.buf) > + memcpy_toio(mem->msg_payload, t->tx.buf, t->tx.len); > +} I thought every member of scmi_shared_mem should be in le, not just the header. If that is the case, why do mem->length and mem->flags not get converted to le? If it is not the case, should they be of type __le32? (same remark applies in scmi_fetch_response for mem->length) > + > +/** > + * scmi_one_xfer_get() - Allocate one message > + * > + * @handle: SCMI entity handle > + * > + * Helper function which is used by various command functions that are > + * exposed to clients of this driver for allocating a message traffic event. > + * > + * This function can sleep depending on pending requests already in the system > + * for the SCMI entity. Further, this also holds a spinlock to maintain > + * integrity of internal data structures. > + * > + * Return: 0 if all went fine, else corresponding error. > + */ > +static struct scmi_xfer *scmi_one_xfer_get(const struct scmi_handle *handle) > +{ > + u16 xfer_id; > + int ret, timeout; > + struct scmi_xfer *xfer; > + unsigned long flags, bit_pos; > + struct scmi_info *info = handle_to_scmi_info(handle); > + struct scmi_xfers_info *minfo = &info->minfo; > + > + /* > + * Ensure we have only controlled number of pending messages. > + * Ideally, we might just have to wait a single message, be > + * conservative and wait 5 times that.. > + */ > + timeout = msecs_to_jiffies(info->desc->max_rx_timeout_ms) * 5; > + ret = down_timeout(&minfo->sem_xfer_count, timeout); > + if (ret < 0) > + return ERR_PTR(ret); > + > + /* Keep the locked section as small as possible */ > + spin_lock_irqsave(&minfo->xfer_lock, flags); > + bit_pos = find_first_zero_bit(minfo->xfer_alloc_table, > + info->desc->max_msg); > + if (bit_pos == info->desc->max_msg) { > + spin_unlock_irqrestore(&minfo->xfer_lock, flags); > + return ERR_PTR(-ENOMEM); > + } > + set_bit(bit_pos, minfo->xfer_alloc_table); > + spin_unlock_irqrestore(&minfo->xfer_lock, flags); > + Is it needed to disable IRQs here? Since the callback called in IRQ context only reads the xfer_alloc_table without modification nor taking locks, can't we just do spin_lock/spin_unlock? > + xfer_id = bit_pos; > + > + xfer = &minfo->xfer_block[xfer_id]; > + xfer->hdr.seq = xfer_id; > + reinit_completion(&xfer->done); > + > + return xfer; > +} > + > +/** > + * scmi_one_xfer_put() - Release a message > + * > + * @minfo: transfer info pointer > + * @xfer: message that was reserved by scmi_one_xfer_get > + * > + * This holds a spinlock to maintain integrity of internal data structures. > + */ > +void scmi_one_xfer_put(const struct scmi_handle *handle, struct scmi_xfer *xfer) > +{ > + unsigned long flags; > + struct scmi_info *info = handle_to_scmi_info(handle); > + struct scmi_xfers_info *minfo = &info->minfo; > + > + /* > + * Keep the locked section as small as possible > + * NOTE: we might escape with smp_mb and no lock here.. > + * but just be conservative and symmetric. > + */ > + spin_lock_irqsave(&minfo->xfer_lock, flags); > + clear_bit(xfer->hdr.seq, minfo->xfer_alloc_table); > + spin_unlock_irqrestore(&minfo->xfer_lock, flags); > + Same question as before. Cheer, -- Julien Thierry