lkml.org 
[lkml]   [2017]   [Sep]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v2 03/18] firmware: arm_scmi: add basic driver infrastructure for SCMI
    Date
    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 <arnd@arndb.de>
    > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
    > ---
    > 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 <http://www.gnu.org/licenses/>.
    > + */
    > +
    > +#include <linux/completion.h>
    > +#include <linux/scmi_protocol.h>
    > +#include <linux/types.h>
    > +
    > +/**
    > + * 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 <http://www.gnu.org/licenses/>.
    > + */
    > +
    > +#include <linux/bitmap.h>
    > +#include <linux/export.h>
    > +#include <linux/io.h>
    > +#include <linux/kernel.h>
    > +#include <linux/mailbox_client.h>
    > +#include <linux/module.h>
    > +#include <linux/of_address.h>
    > +#include <linux/of_device.h>
    > +#include <linux/semaphore.h>
    > +#include <linux/slab.h>
    > +
    > +#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

    \
     
     \ /
      Last update: 2017-09-05 12:03    [W:4.447 / U:0.096 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site