| Message ID | 1519403030-21189-1-git-send-email-sudeep.holla@arm.com |
|---|---|
| Headers | show |
| Series | firmware: ARM System Control and Management Interface(SCMI) support | expand |
Hi Philippe, On 26/02/18 15:57, Philippe Ombredanne wrote: > Sudeep, [...] >> --- /dev/null >> +++ b/drivers/firmware/arm_scmi/common.h >> @@ -0,0 +1,77 @@ >> +/* >> + * 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/>. >> + */ > > Would you consider using the SPDX tags [1] instead of this legalese? > Thanks! > Sure, I have fixed locally [1] for now, will post if there are any other changes before PR. -- Regards, Sudeep [1] https://git.kernel.org/sudeep.holla/linux/h/for-list/new_arm_scmi -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 23 Feb 2018 16:23:33 +0000 Sudeep Holla <sudeep.holla@arm.com> 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. > Hi Sudeep, A bit of a drive by review as I was curious and happen to have been looking at the spec. Anyhow my main comments in here are about consistency of naming. In many ways it doesn't matter what naming convention you go with, but it is good to make sure you then use it consistently. > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> <snip> > + > +#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. Something looks odd with indenting here... > + */ > +struct scmi_msg_hdr { > + u8 id; I think this is called message_id in the spec, would it be worth matching that here? > + u8 protocol_id; > + u16 seq; > + u32 status; > + bool poll_completion; > +}; status and poll completion could do with documenting. > + > +/** > + * 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 > + */ > + No blank line here. > +struct scmi_xfer { > + void *con_priv; > + 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); > +int scmi_handle_put(const struct scmi_handle *handle); > +struct scmi_handle *scmi_handle_get(struct device *dev); > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c > new file mode 100644 > index 000000000000..fa0e9cf31f4c > --- /dev/null > +++ b/drivers/firmware/arm_scmi/driver.c > @@ -0,0 +1,689 @@ > +/* > + * 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 Interesting to note you don't specify type in your header structure above but do have it here. I guess that is because it is always 0 when you care about. Might be nice to be consistent though? > +#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 > + * > + * @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 scmi_xfer *xfer_block; > + unsigned long *xfer_alloc_table; > + /* protect transfer allocation */ This is here as warning suppression as it's clearly documented above. Personally I've always just marked those downs as false positives rather than having the ugliness of documenting it twice. > + 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 > + * @tx_payload: Transmit mailbox channel payload area > + * @minfo: Message info > + * @node: list head Nitpick of the day :) Inconsistent capitalization. Also useful to know which list this is for... > + * @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; > + void __iomem *tx_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) > + > +/* > + * SCMI specification requires all parameters, message headers, return > + * arguments or any protocol data to be expressed in little endian > + * format only. > + */ > +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 const 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 __iomem *mem) > +{ > + xfer->hdr.status = ioread32(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, ioread32(&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 __iomem *mem = info->tx_payload; > + > + xfer_id = MSG_XTRACT_TOKEN(ioread32(&mem->msg_header)); > + > + /* > + * Are we even expecting this? > + */ Single line comment syntax probably better here. Also the error text makes it obvious anyway so not sure this comment adds much... > + 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 __iomem *mem = info->tx_payload; > + > + /* Mark channel busy + clear error */ > + iowrite32(0x0, &mem->channel_status); > + iowrite32(t->hdr.poll_completion ? 0 : SCMI_SHMEM_FLAG_INTR_ENABLED, > + &mem->flags); > + iowrite32(sizeof(mem->msg_header) + t->tx.len, &mem->length); > + iowrite32(pack_scmi_header(&t->hdr), &mem->msg_header); > + if (t->tx.buf) > + memcpy_toio(mem->msg_payload, t->tx.buf, t->tx.len); > +} > + > +/** > + * 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) Maybe it's just me, but I think this would be more clearly named as scmi_xfer_alloc. I'd assume we were dealing with one anyway as it's not called scmi_xfers_get and the get to my mind implies a reference counter rather than allocating an xfer for use... > +{ > + u16 xfer_id; > + 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; > + > + /* 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); > + > + 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); > +} > + > +/** > + * scmi_do_xfer() - Do one transfer This kind of makes my point above about no need for _one_ in naming. You never put it here! > + * > + * @info: Pointer to SCMI entity information > + * @xfer: Transfer to initiate and wait for response > + * > + * Return: -ETIMEDOUT in case of no response, if transmit error, > + * return corresponding error, else if all goes well, > + * return 0. > + */ > +int scmi_do_xfer(const struct scmi_handle *handle, struct scmi_xfer *xfer) > +{ > + int ret; > + int timeout; > + struct scmi_info *info = handle_to_scmi_info(handle); > + struct device *dev = info->dev; > + > + ret = mbox_send_message(info->tx_chan, xfer); > + if (ret < 0) { > + dev_dbg(dev, "mbox send fail %d\n", ret); > + return ret; > + } > + > + /* mbox_send_message returns non-negative value on success, so reset */ > + ret = 0; > + > + /* And we wait for the response. */ > + timeout = msecs_to_jiffies(info->desc->max_rx_timeout_ms); > + if (!wait_for_completion_timeout(&xfer->done, timeout)) { > + dev_err(dev, "mbox timed out in resp(caller: %pS)\n", > + (void *)_RET_IP_); > + ret = -ETIMEDOUT; > + } else if (xfer->hdr.status) { > + ret = scmi_to_linux_errno(xfer->hdr.status); > + } > + /* > + * NOTE: we might prefer not to need the mailbox ticker to manage the > + * transfer queueing since the protocol layer queues things by itself. > + * Unfortunately, we have to kick the mailbox framework after we have > + * received our message. > + */ > + mbox_client_txdone(info->tx_chan, ret); > + > + return ret; > +} > + > +/** > + * scmi_one_xfer_init() - Allocate and initialise one message Could perhaps make the alloc part of this clear in the name? > + * > + * @handle: SCMI entity handle > + * @msg_id: Message identifier > + * @msg_prot_id: Protocol identifier for the message It's called prot_id. Run the kernel-doc script on this an it'll probably point out little inconsistencies like this. > + * @tx_size: transmit message size > + * @rx_size: receive message size > + * @p: pointer to the allocated and initialised message This is a pointer we want to set to this, it's not a pointer to it when first called. > + * > + * This function allocates the message using @scmi_one_xfer_get and > + * initialise the header. If we are describing it, should describe everything - also sets the lengths. > + * > + * Return: 0 if all went fine with @p pointing to message, else > + * corresponding error. > + */ > +int scmi_one_xfer_init(const struct scmi_handle *handle, u8 msg_id, u8 prot_id, > + size_t tx_size, size_t rx_size, struct scmi_xfer **p) > +{ > + int ret; > + struct scmi_xfer *xfer; > + struct scmi_info *info = handle_to_scmi_info(handle); > + struct device *dev = info->dev; > + > + /* Ensure we have sane transfer sizes */ > + if (rx_size > info->desc->max_msg_size || > + tx_size > info->desc->max_msg_size) > + return -ERANGE; > + > + xfer = scmi_one_xfer_get(handle); > + if (IS_ERR(xfer)) { > + ret = PTR_ERR(xfer); > + dev_err(dev, "failed to get free message slot(%d)\n", ret); > + return ret; > + } > + > + xfer->tx.len = tx_size; > + xfer->rx.len = rx_size ? : info->desc->max_msg_size; > + xfer->hdr.id = msg_id; > + xfer->hdr.protocol_id = prot_id; > + xfer->hdr.poll_completion = false; > + > + *p = xfer; blank line here perhaps. > + return 0; > +} > + > +/** > + * scmi_handle_get() - Get the SCMI handle for a device Spacing before SCMI is odd. BTW this is what I'd expect a _get function to be doing - it's retrieving the thing in question and incrementing a reference counter to keep it around. > + * > + * @dev: pointer to device for which we want SCMI handle > + * > + * NOTE: The function does not track individual clients of the framework > + * and is expected to be maintained by caller of SCMI protocol library. > + * scmi_handle_put must be balanced with successful scmi_handle_get > + * > + * Return: pointer to handle if successful, NULL on error > + */ > +struct scmi_handle *scmi_handle_get(struct device *dev) > +{ > + struct list_head *p; > + struct scmi_info *info; > + struct scmi_handle *handle = NULL; > + > + mutex_lock(&scmi_list_mutex); > + list_for_each(p, &scmi_list) { > + info = list_entry(p, struct scmi_info, node); > + if (dev->parent == info->dev) { > + handle = &info->handle; > + info->users++; > + break; > + } > + } > + mutex_unlock(&scmi_list_mutex); > + > + return handle; > +} > + > +/** > + * scmi_handle_put() - Release the handle acquired by scmi_handle_get > + * > + * @handle: handle acquired by scmi_handle_get > + * > + * NOTE: The function does not track individual clients of the framework > + * and is expected to be maintained by caller of SCMI protocol library. Again, odd spacing before SCMI.. > + * scmi_handle_put must be balanced with successful scmi_handle_get > + * > + * Return: 0 is successfully released > + * if null was passed, it returns -EINVAL; > + */ > +int scmi_handle_put(const struct scmi_handle *handle) > +{ > + struct scmi_info *info; > + > + if (!handle) > + return -EINVAL; > + > + info = handle_to_scmi_info(handle); > + mutex_lock(&scmi_list_mutex); > + if (!WARN_ON(!info->users)) > + info->users--; > + mutex_unlock(&scmi_list_mutex); > + > + return 0; > +} > + > +static const struct scmi_desc scmi_generic_desc = { > + .max_rx_timeout_ms = 30, /* we may increase this if required */ Inconsistent capitalization of comment. Doesn't really matter which but looks a bit odd here with it different on two lines next to each other. > + .max_msg = 20, /* Limited by MBOX_TX_QUEUE_LEN */ > + .max_msg_size = 128, > +}; > + > +/* Each compatible listed below must have descriptor associated with it */ > +static const struct of_device_id scmi_of_match[] = { > + { .compatible = "arm,scmi", .data = &scmi_generic_desc }, > + { /* Sentinel */ }, > +}; > + > +MODULE_DEVICE_TABLE(of, scmi_of_match); > + > +static int scmi_xfer_info_init(struct scmi_info *sinfo) > +{ > + int i; > + struct scmi_xfer *xfer; > + struct device *dev = sinfo->dev; > + const struct scmi_desc *desc = sinfo->desc; > + struct scmi_xfers_info *info = &sinfo->minfo; > + > + /* Pre-allocated messages, no more than what hdr.seq can support */ > + if (WARN_ON(desc->max_msg >= (MSG_TOKEN_ID_MASK + 1))) { > + dev_err(dev, "Maximum message of %d exceeds supported %d\n", > + desc->max_msg, MSG_TOKEN_ID_MASK + 1); > + return -EINVAL; > + } > + > + info->xfer_block = devm_kcalloc(dev, desc->max_msg, > + sizeof(*info->xfer_block), GFP_KERNEL); > + if (!info->xfer_block) > + return -ENOMEM; > + > + info->xfer_alloc_table = devm_kcalloc(dev, BITS_TO_LONGS(desc->max_msg), > + sizeof(long), GFP_KERNEL); > + if (!info->xfer_alloc_table) > + return -ENOMEM; Hmm. I wonder if it is worth adding a devm_bitmap_alloc? > + > + bitmap_zero(info->xfer_alloc_table, desc->max_msg); kcalloc zeros the memory. > + > + /* Pre-initialize the buffer pointer to pre-allocated buffers */ > + for (i = 0, xfer = info->xfer_block; i < desc->max_msg; i++, xfer++) { > + xfer->rx.buf = devm_kcalloc(dev, sizeof(u8), desc->max_msg_size, > + GFP_KERNEL); > + if (!xfer->rx.buf) > + return -ENOMEM; > + > + xfer->tx.buf = xfer->rx.buf; > + init_completion(&xfer->done); > + } > + > + spin_lock_init(&info->xfer_lock); > + > + return 0; > +} > + > +static int scmi_mailbox_check(struct device_node *np) > +{ > + struct of_phandle_args arg; > + > + return of_parse_phandle_with_args(np, "mboxes", "#mbox-cells", 0, &arg); > +} > + > +static int scmi_mbox_free_channel(struct scmi_info *info) Some of the naming in here could do with being adjusted to be obviously 'balanced'. The moment I see a free I expect a matched alloc but in this case the alloc is done in scmi_mbox_chan_setup which at very least should be scmi_mbox_setup_channel and should really imply that it is doing allocation as well. > +{ > + if (!IS_ERR_OR_NULL(info->tx_chan)) { > + mbox_free_channel(info->tx_chan); > + info->tx_chan = NULL; > + } > + > + return 0; > +} > + > +static int scmi_remove(struct platform_device *pdev) > +{ > + int ret = 0; > + struct scmi_info *info = platform_get_drvdata(pdev); > + > + mutex_lock(&scmi_list_mutex); > + if (info->users) > + ret = -EBUSY; > + else > + list_del(&info->node); > + mutex_unlock(&scmi_list_mutex); > + > + if (!ret) This would have a more readable flow if you just did if (ret) return ret; return sci_mbox_free_channel(info) > + /* Safe to free channels since no more users */ > + return scmi_mbox_free_channel(info); > + > + return ret; > +} > + > +static inline int scmi_mbox_chan_setup(struct scmi_info *info) > +{ > + int ret; > + struct resource res; > + resource_size_t size; > + struct device *dev = info->dev; > + struct device_node *shmem, *np = dev->of_node; > + struct mbox_client *cl; > + > + cl = &info->cl; > + cl->dev = dev; > + cl->rx_callback = scmi_rx_callback; > + cl->tx_prepare = scmi_tx_prepare; > + cl->tx_block = false; > + cl->knows_txdone = true; > + > + shmem = of_parse_phandle(np, "shmem", 0); > + ret = of_address_to_resource(shmem, 0, &res); > + of_node_put(shmem); > + if (ret) { > + dev_err(dev, "failed to get SCMI Tx payload mem resource\n"); > + return ret; > + } > + > + size = resource_size(&res); > + info->tx_payload = devm_ioremap(dev, res.start, size); > + if (!info->tx_payload) { > + dev_err(dev, "failed to ioremap SCMI Tx payload\n"); > + return -EADDRNOTAVAIL; > + } > + > + /* Transmit channel is first entry i.e. index 0 */ > + info->tx_chan = mbox_request_channel(cl, 0); > + if (IS_ERR(info->tx_chan)) { > + ret = PTR_ERR(info->tx_chan); > + if (ret != -EPROBE_DEFER) > + dev_err(dev, "failed to request SCMI Tx mailbox\n"); > + return ret; > + } > + > + return 0; > +} > + > +static int scmi_probe(struct platform_device *pdev) > +{ > + int ret; > + struct scmi_handle *handle; > + const struct scmi_desc *desc; > + struct scmi_info *info; > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + > + /* Only mailbox method supported, check for the presence of one */ > + if (scmi_mailbox_check(np)) { > + dev_err(dev, "no mailbox found in %pOF\n", np); > + return -EINVAL; > + } > + > + desc = of_match_device(scmi_of_match, dev)->data; > + > + info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL); > + if (!info) > + return -ENOMEM; > + > + info->dev = dev; > + info->desc = desc; > + INIT_LIST_HEAD(&info->node); > + > + ret = scmi_xfer_info_init(info); > + if (ret) > + return ret; > + > + platform_set_drvdata(pdev, info); > + > + handle = &info->handle; > + handle->dev = info->dev; > + > + ret = scmi_mbox_chan_setup(info); > + if (ret) > + return ret; > + > + mutex_lock(&scmi_list_mutex); > + list_add_tail(&info->node, &scmi_list); > + mutex_unlock(&scmi_list_mutex); > + > + return 0; > +} > + > +static struct platform_driver scmi_driver = { > + .driver = { > + .name = "arm-scmi", > + .of_match_table = scmi_of_match, > + }, > + .probe = scmi_probe, > + .remove = scmi_remove, > +}; > + > +module_platform_driver(scmi_driver); > + > +MODULE_ALIAS("platform: arm-scmi"); > +MODULE_AUTHOR("Sudeep Holla <sudeep.holla@arm.com>"); > +MODULE_DESCRIPTION("ARM SCMI protocol driver"); > +MODULE_LICENSE("GPL v2"); > diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h > new file mode 100644 > index 000000000000..854ed2479993 > --- /dev/null > +++ b/include/linux/scmi_protocol.h > @@ -0,0 +1,27 @@ > +/* > + * SCMI Message Protocol driver header > + * > + * 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/types.h> > + > +/** > + * struct scmi_handle - Handle returned to ARM SCMI clients for usage. > + * > + * @dev: pointer to the SCMI device > + */ > +struct scmi_handle { > + struct device *dev; > +}; -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 23 Feb 2018 16:23:34 +0000 Sudeep Holla <sudeep.holla@arm.com> 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. > <snip> 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), &t); > + 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 = &info->handle; > handle->dev = info->dev; > + handle->version = &info->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(&scmi_list_mutex); > list_add_tail(&info->node, &scmi_list); > mutex_unlock(&scmi_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 <linux/types.h> > > +#define SCMI_MAX_STR_SIZE 16 > + > +/** > + * 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 > + * base protocol. > + * @num_agents: Number of agents in the system. > + * @impl_ver: A vendor-specific implementation version. > + * @vendor_id: A vendor identifier(Null terminated ASCII string) > + * @sub_vendor_id: A sub-vendor identifier(Null terminated ASCII string) > + */ > +struct scmi_revision_info { > + u16 major_ver; > + u16 minor_ver; > + u8 num_protocols; > + u8 num_agents; > + u32 impl_ver; > + char vendor_id[SCMI_MAX_STR_SIZE]; > + char sub_vendor_id[SCMI_MAX_STR_SIZE]; > +}; > + > /** > * struct scmi_handle - Handle returned to ARM SCMI clients for usage. > * > * @dev: pointer to the SCMI device > + * @version: pointer to the structure containing SCMI version information > */ > struct scmi_handle { > struct device *dev; > + struct scmi_revision_info *version; > +}; > + > +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, > }; -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 23 Feb 2018 16:23:35 +0000 Sudeep Holla <sudeep.holla@arm.com> wrote: > The SCMI specification encompasses various protocols. However, not every > protocol has to be present on a given platform/implementation as not > every protocol is relevant for it. > > Furthermore, the platform chooses which protocols it exposes to a given > agent. The only protocol that must be implemented is the base protocol. > The base protocol is used by an agent to discover which protocols are > available to it. > > In order to enumerate the discovered implemented protocols, this patch > adds support for a separate scmi protocol bus. It also adds mechanism to > register support for different protocols. > > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> Comments inline. Jonathan > --- > drivers/firmware/arm_scmi/Makefile | 3 +- > drivers/firmware/arm_scmi/bus.c | 232 +++++++++++++++++++++++++++++++++++++ > drivers/firmware/arm_scmi/common.h | 1 + > include/linux/scmi_protocol.h | 64 ++++++++++ > 4 files changed, 299 insertions(+), 1 deletion(-) > create mode 100644 drivers/firmware/arm_scmi/bus.c > > diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile > index 5d9c7ef35f0f..5f4ec2613db6 100644 > --- a/drivers/firmware/arm_scmi/Makefile > +++ b/drivers/firmware/arm_scmi/Makefile > @@ -1,3 +1,4 @@ > -obj-y = scmi-driver.o scmi-protocols.o > +obj-y = scmi-bus.o scmi-driver.o scmi-protocols.o > +scmi-bus-y = bus.o > scmi-driver-y = driver.o > scmi-protocols-y = base.o > diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c > new file mode 100644 > index 000000000000..58bb4f46fde1 > --- /dev/null > +++ b/drivers/firmware/arm_scmi/bus.c > @@ -0,0 +1,232 @@ > +/* > + * System Control and Management Interface (SCMI) Message Protocol bus layer > + * > + * 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/>. > + */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/types.h> > +#include <linux/module.h> > +#include <linux/kernel.h> > +#include <linux/slab.h> > +#include <linux/device.h> > + > +#include "common.h" > + > +static DEFINE_IDA(scmi_bus_id); > +static DEFINE_IDR(scmi_protocols); > +static DEFINE_SPINLOCK(protocol_lock); > + > +static const struct scmi_device_id * > +scmi_dev_match_id(struct scmi_device *scmi_dev, struct scmi_driver *scmi_drv) > +{ > + const struct scmi_device_id *id = scmi_drv->id_table; > + > + if (!id) > + return NULL; > + > + for (; id->protocol_id; id++) > + if (id->protocol_id == scmi_dev->protocol_id) > + return id; > + > + return NULL; > +} > + > +static int scmi_dev_match(struct device *dev, struct device_driver *drv) > +{ > + struct scmi_driver *scmi_drv = to_scmi_driver(drv); > + struct scmi_device *scmi_dev = to_scmi_dev(dev); > + const struct scmi_device_id *id; > + > + id = scmi_dev_match_id(scmi_dev, scmi_drv); > + if (id) > + return 1; > + > + return 0; > +} > + > +static int scmi_protocol_init(int protocol_id, struct scmi_handle *handle) > +{ > + scmi_prot_init_fn_t fn = idr_find(&scmi_protocols, protocol_id); > + > + if (unlikely(!fn)) > + return -EINVAL; > + return fn(handle); > +} > + > +static int scmi_dev_probe(struct device *dev) > +{ > + struct scmi_driver *scmi_drv = to_scmi_driver(dev->driver); > + struct scmi_device *scmi_dev = to_scmi_dev(dev); > + const struct scmi_device_id *id; > + int ret; > + > + id = scmi_dev_match_id(scmi_dev, scmi_drv); > + if (!id) > + return -ENODEV; > + > + if (!scmi_dev->handle) > + return -EPROBE_DEFER; > + > + ret = scmi_protocol_init(scmi_dev->protocol_id, scmi_dev->handle); > + if (ret) > + return ret; > + > + return scmi_drv->probe(scmi_dev); > +} > + > +static int scmi_dev_remove(struct device *dev) > +{ > + struct scmi_driver *scmi_drv = to_scmi_driver(dev->driver); > + struct scmi_device *scmi_dev = to_scmi_dev(dev); > + > + if (scmi_drv->remove) > + scmi_drv->remove(scmi_dev); > + > + return 0; > +} > + > +static struct bus_type scmi_bus_type = { > + .name = "scmi_protocol", > + .match = scmi_dev_match, > + .probe = scmi_dev_probe, > + .remove = scmi_dev_remove, > +}; > + > +int scmi_driver_register(struct scmi_driver *driver, struct module *owner, > + const char *mod_name) > +{ > + int retval; > + > + driver->driver.bus = &scmi_bus_type; > + driver->driver.name = driver->name; > + driver->driver.owner = owner; > + driver->driver.mod_name = mod_name; > + > + retval = driver_register(&driver->driver); > + if (!retval) > + pr_debug("registered new scmi driver %s\n", driver->name); > + > + return retval; > +} > +EXPORT_SYMBOL_GPL(scmi_driver_register); > + > +void scmi_driver_unregister(struct scmi_driver *driver) > +{ > + driver_unregister(&driver->driver); > +} > +EXPORT_SYMBOL_GPL(scmi_driver_unregister); > + > +struct scmi_device * > +scmi_device_create(struct device_node *np, struct device *parent, int protocol) > +{ > + int id, retval; > + struct scmi_device *scmi_dev; > + > + id = ida_simple_get(&scmi_bus_id, 1, 0, GFP_KERNEL); > + if (id < 0) > + return NULL; > + > + scmi_dev = kzalloc(sizeof(*scmi_dev), GFP_KERNEL); > + if (!scmi_dev) > + goto no_mem; > + > + scmi_dev->id = id; > + scmi_dev->protocol_id = protocol; > + scmi_dev->dev.parent = parent; > + scmi_dev->dev.of_node = np; > + scmi_dev->dev.bus = &scmi_bus_type; > + dev_set_name(&scmi_dev->dev, "scmi_dev.%d", id); > + > + retval = device_register(&scmi_dev->dev); > + if (!retval) > + return scmi_dev; This approach of the good path being the indented return is not that common in kernel code. Personally I would have found this a lot more readable with a goto used for the error path. (This was true in various earlier places, but here it is really nasty so I thought I'd raise it). > + > + put_device(&scmi_dev->dev); > + kfree(scmi_dev); > +no_mem: > + ida_simple_remove(&scmi_bus_id, id); > + return NULL; > +} > + > +void scmi_device_destroy(struct scmi_device *scmi_dev) > +{ > + scmi_handle_put(scmi_dev->handle); > + device_unregister(&scmi_dev->dev); > + ida_simple_remove(&scmi_bus_id, scmi_dev->id); Why not reorder the create function to do the alloc then the ida_simple get - that way you could avoid making reviewers wonder why you have different ordering in the two functions. > + kfree(scmi_dev); > +} > + > +void scmi_set_handle(struct scmi_device *scmi_dev) > +{ > + scmi_dev->handle = scmi_handle_get(&scmi_dev->dev); > +} > + > +int scmi_protocol_register(int protocol_id, scmi_prot_init_fn_t fn) > +{ > + int ret; > + > + spin_lock(&protocol_lock); > + ret = idr_alloc(&scmi_protocols, fn, protocol_id, protocol_id + 1, > + GFP_ATOMIC); > + if (ret != protocol_id) > + pr_err("unable to allocate SCMI idr slot, err %d\n", ret); Error check and print could be outside the lock. > + spin_unlock(&protocol_lock); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(scmi_protocol_register); > + > +void scmi_protocol_unregister(int protocol_id) > +{ > + spin_lock(&protocol_lock); > + idr_remove(&scmi_protocols, protocol_id); > + spin_unlock(&protocol_lock); > +} > +EXPORT_SYMBOL_GPL(scmi_protocol_unregister); > + > +static int __scmi_devices_unregister(struct device *dev, void *data) > +{ > + struct scmi_device *scmi_dev = to_scmi_dev(dev); > + > + scmi_device_destroy(scmi_dev); > + return 0; > +} > + > +static void scmi_devices_unregister(void) > +{ > + bus_for_each_dev(&scmi_bus_type, NULL, NULL, __scmi_devices_unregister); > +} > + > +static int __init scmi_bus_init(void) > +{ > + int retval; > + > + retval = bus_register(&scmi_bus_type); > + if (retval) > + pr_err("scmi protocol bus register failed (%d)\n", retval); > + > + return retval; > +} > +subsys_initcall(scmi_bus_init); > + > +static void __exit scmi_bus_exit(void) > +{ > + scmi_devices_unregister(); > + bus_unregister(&scmi_bus_type); > + ida_destroy(&scmi_bus_id); > +} > +module_exit(scmi_bus_exit); > diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h > index bc767f4997e0..f1eeacaef57b 100644 > --- a/drivers/firmware/arm_scmi/common.h > +++ b/drivers/firmware/arm_scmi/common.h > @@ -107,6 +107,7 @@ 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_handle_put(const struct scmi_handle *handle); > struct scmi_handle *scmi_handle_get(struct device *dev); > +void scmi_set_handle(struct scmi_device *scmi_dev); > 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); > diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h > index 664da3d763f2..db995126134d 100644 > --- a/include/linux/scmi_protocol.h > +++ b/include/linux/scmi_protocol.h > @@ -15,6 +15,7 @@ > * 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/device.h> > #include <linux/types.h> > > #define SCMI_MAX_STR_SIZE 16 > @@ -62,3 +63,66 @@ enum scmi_std_protocol { > SCMI_PROTOCOL_CLOCK = 0x14, > SCMI_PROTOCOL_SENSOR = 0x15, > }; > + > +struct scmi_device { > + u32 id; > + u8 protocol_id; > + struct device dev; > + struct scmi_handle *handle; > +}; > + > +#define to_scmi_dev(d) container_of(d, struct scmi_device, dev) > + > +struct scmi_device * > +scmi_device_create(struct device_node *np, struct device *parent, int protocol); > +void scmi_device_destroy(struct scmi_device *scmi_dev); > + > +struct scmi_device_id { > + u8 protocol_id; > +}; > + > +struct scmi_driver { > + const char *name; > + int (*probe)(struct scmi_device *sdev); > + void (*remove)(struct scmi_device *sdev); > + const struct scmi_device_id *id_table; > + > + struct device_driver driver; > +}; > + > +#define to_scmi_driver(d) container_of(d, struct scmi_driver, driver) > + > +#ifdef CONFIG_ARM_SCMI_PROTOCOL > +int scmi_driver_register(struct scmi_driver *driver, > + struct module *owner, const char *mod_name); > +void scmi_driver_unregister(struct scmi_driver *driver); > +#else > +static inline int > +scmi_driver_register(struct scmi_driver *driver, struct module *owner, > + const char *mod_name) > +{ > + return -EINVAL; > +} > + > +static inline void scmi_driver_unregister(struct scmi_driver *driver) {} > +#endif /* CONFIG_ARM_SCMI_PROTOCOL */ > + > +#define scmi_register(driver) \ > + scmi_driver_register(driver, THIS_MODULE, KBUILD_MODNAME) > +#define scmi_unregister(driver) \ > + scmi_driver_unregister(driver) > + > +/** > + * module_scmi_driver() - Helper macro for registering a scmi driver > + * @__scmi_driver: scmi_driver structure > + * > + * Helper macro for scmi drivers to set up proper module init / exit > + * functions. Replaces module_init() and module_exit() and keeps people from > + * printing pointless things to the kernel log when their driver is loaded. > + */ > +#define module_scmi_driver(__scmi_driver) \ > + module_driver(__scmi_driver, scmi_register, scmi_unregister) > + > +typedef int (*scmi_prot_init_fn_t)(struct scmi_handle *); > +int scmi_protocol_register(int protocol_id, scmi_prot_init_fn_t fn); > +void scmi_protocol_unregister(int protocol_id); -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 23 Feb 2018 16:23:36 +0000 Sudeep Holla <sudeep.holla@arm.com> wrote: > The performance protocol is intended for the performance management of > group(s) of device(s) that run in the same performance domain. It > includes even the CPUs. A performance domain is defined by a set of > devices that always have to run at the same performance level. > For example, a set of CPUs that share a voltage domain, and have a > common frequency control, is said to be in the same performance domain. > > The commands in this protocol provide functionality to describe the > protocol version, describe various attribute flags, set and get the > performance level of a domain. It also supports discovery of the list > of performance levels supported by a performance domain, and the > properties of each performance level. > <snip> > + > +static int scmi_perf_attributes_get(const struct scmi_handle *handle, > + struct scmi_perf_info *pi) > +{ > + int ret; > + struct scmi_xfer *t; > + struct scmi_msg_resp_perf_attributes *attr; > + > + ret = scmi_one_xfer_init(handle, PROTOCOL_ATTRIBUTES, > + SCMI_PROTOCOL_PERF, 0, sizeof(*attr), &t); > + if (ret) > + return ret; > + > + attr = t->rx.buf; > + > + ret = scmi_do_xfer(handle, t); > + if (!ret) { Use a goto for the error path rather than indenting all this good path stuff. The same would help readability in various other places. > + u16 flags = le16_to_cpu(attr->flags); > + > + pi->num_domains = le16_to_cpu(attr->num_domains); > + pi->power_scale_mw = POWER_SCALE_IN_MILLIWATT(flags); > + pi->stats_addr = le32_to_cpu(attr->stats_addr_low) | > + (u64)le32_to_cpu(attr->stats_addr_high) << 32; > + pi->stats_size = le32_to_cpu(attr->stats_size); > + } > + > + scmi_one_xfer_put(handle, t); > + return ret; > +} > + ... > + > +static struct scmi_perf_ops perf_ops = { > + .limits_set = scmi_perf_limits_set, > + .limits_get = scmi_perf_limits_get, > + .level_set = scmi_perf_level_set, > + .level_get = scmi_perf_level_get, > + .device_domain_id = scmi_dev_domain_id, > + .get_transition_latency = scmi_dvfs_get_transition_latency, > + .add_opps_to_device = scmi_dvfs_add_opps_to_device, > + .freq_set = scmi_dvfs_freq_set, > + .freq_get = scmi_dvfs_freq_get, > +}; > + > +static int scmi_perf_protocol_init(struct scmi_handle *handle) > +{ > + int domain; > + u32 version; > + struct scmi_perf_info *pinfo; > + > + scmi_version_get(handle, SCMI_PROTOCOL_PERF, &version); > + > + dev_dbg(handle->dev, "Performance Version %d.%d\n", > + PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version)); > + > + pinfo = devm_kzalloc(handle->dev, sizeof(*pinfo), GFP_KERNEL); > + if (!pinfo) > + return -ENOMEM; > + > + scmi_perf_attributes_get(handle, pinfo); > + > + pinfo->dom_info = devm_kcalloc(handle->dev, pinfo->num_domains, > + sizeof(*pinfo->dom_info), GFP_KERNEL); > + if (!pinfo->dom_info) > + return -ENOMEM; > + > + for (domain = 0; domain < pinfo->num_domains; domain++) { > + struct perf_dom_info *dom = pinfo->dom_info + domain; > + > + scmi_perf_domain_attributes_get(handle, domain, dom); > + scmi_perf_describe_levels_get(handle, domain, dom); > + } > + > + handle->perf_ops = &perf_ops; > + handle->perf_priv = pinfo; > + > + return 0; > +} > + > +static int __init scmi_perf_init(void) > +{ > + return scmi_protocol_register(SCMI_PROTOCOL_PERF, > + &scmi_perf_protocol_init); > +} > +subsys_initcall(scmi_perf_init); > diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h > index db995126134d..d80f4c9a0fad 100644 > --- a/include/linux/scmi_protocol.h > +++ b/include/linux/scmi_protocol.h > @@ -44,15 +44,57 @@ struct scmi_revision_info { > char sub_vendor_id[SCMI_MAX_STR_SIZE]; > }; > > +struct scmi_handle; > + > +/** > + * struct scmi_perf_ops - represents the various operations provided > + * by SCMI Performance Protocol > + * > + * @limits_set: sets limits on the performance level of a domain > + * @limits_get: gets limits on the performance level of a domain > + * @level_set: sets the performance level of a domain > + * @level_get: gets the performance level of a domain > + * @device_domain_id: gets the scmi domain id for a given device > + * @get_transition_latency: gets the DVFS transition latency for a given device > + * @add_opps_to_device: adds all the OPPs for a given device > + * @freq_set: sets the frequency for a given device using sustained frequency > + * to sustained performance level mapping > + * @freq_get: gets the frequency for a given device using sustained frequency > + * to sustained performance level mapping > + */ > +struct scmi_perf_ops { > + int (*limits_set)(const struct scmi_handle *handle, u32 domain, > + u32 max_perf, u32 min_perf); > + int (*limits_get)(const struct scmi_handle *handle, u32 domain, > + u32 *max_perf, u32 *min_perf); > + int (*level_set)(const struct scmi_handle *handle, u32 domain, > + u32 level); > + int (*level_get)(const struct scmi_handle *handle, u32 domain, > + u32 *level); > + int (*device_domain_id)(struct device *dev); > + int (*get_transition_latency)(const struct scmi_handle *handle, > + struct device *dev); Naming consistency would improve this. transition_latency_get for example. > + int (*add_opps_to_device)(const struct scmi_handle *handle, > + struct device *dev); > + int (*freq_set)(const struct scmi_handle *handle, u32 domain, > + unsigned long rate); > + int (*freq_get)(const struct scmi_handle *handle, u32 domain, > + unsigned long *rate); > +}; > + > /** > * struct scmi_handle - Handle returned to ARM SCMI clients for usage. > * > * @dev: pointer to the SCMI device > * @version: pointer to the structure containing SCMI version information > + * @perf_ops: pointer to set of performance protocol operations > */ > struct scmi_handle { > struct device *dev; > struct scmi_revision_info *version; > + struct scmi_perf_ops *perf_ops; > + /* for protocol internal use */ > + void *perf_priv; > }; > > enum scmi_std_protocol { -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/03/18 14:35, Jonathan Cameron wrote: > On Fri, 23 Feb 2018 16:23:43 +0000 > Sudeep Holla <sudeep.holla@arm.com> wrote: > >> In order to support per-protocol channels if available, we need to >> factor out all the mailbox channel information(Tx/Rx payload and >> channel handle) out of the main SCMI instance information structure. >> >> This patch refactors the existing channel information into a separate >> chan_info structure. >> >> Cc: Arnd Bergmann <arnd@arndb.de> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > A little odd to present a series to mainline that does a fairly important > refactor mid way through. Why not push this down to the start and always support > the separate chan_info structure? > Since the development started long back and this was added later, I preferred to keep functionality working and didn't want to push change down in the stack. -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> >> +/** > >> + * 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) > > Maybe it's just me, but I think this would be more clearly named as > > scmi_xfer_alloc. > > > > Agreed to some extent. The reason I didn't have it as alloc as they are > preallocated and this just returns a reference to free slot in that > preallocated array. > > > I'd assume we were dealing with one anyway as it's not called scmi_xfers_get > > and the get to my mind implies a reference counter rather than allocating > > an xfer for use... > > > > Ah OK, I get your concerne with _get/_put but _alloc/_free is equally > bad then in the contect of preallocated buffers. Sure, this is always a fun question. Lots of other options _assign _deassign works but never feels nice. > ... > > >> + .max_msg = 20, /* Limited by MBOX_TX_QUEUE_LEN */ > >> + .max_msg_size = 128, > >> +}; > >> + > >> +/* Each compatible listed below must have descriptor associated with it */ > >> +static const struct of_device_id scmi_of_match[] = { > >> + { .compatible = "arm,scmi", .data = &scmi_generic_desc }, > >> + { /* Sentinel */ }, > >> +}; > >> + > >> +MODULE_DEVICE_TABLE(of, scmi_of_match); > >> + > >> +static int scmi_xfer_info_init(struct scmi_info *sinfo) > >> +{ > >> + int i; > >> + struct scmi_xfer *xfer; > >> + struct device *dev = sinfo->dev; > >> + const struct scmi_desc *desc = sinfo->desc; > >> + struct scmi_xfers_info *info = &sinfo->minfo; > >> + > >> + /* Pre-allocated messages, no more than what hdr.seq can support */ > >> + if (WARN_ON(desc->max_msg >= (MSG_TOKEN_ID_MASK + 1))) { > >> + dev_err(dev, "Maximum message of %d exceeds supported %d\n", > >> + desc->max_msg, MSG_TOKEN_ID_MASK + 1); > >> + return -EINVAL; > >> + } > >> + > >> + info->xfer_block = devm_kcalloc(dev, desc->max_msg, > >> + sizeof(*info->xfer_block), GFP_KERNEL); > >> + if (!info->xfer_block) > >> + return -ENOMEM; > >> + > >> + info->xfer_alloc_table = devm_kcalloc(dev, BITS_TO_LONGS(desc->max_msg), > >> + sizeof(long), GFP_KERNEL); > >> + if (!info->xfer_alloc_table) > >> + return -ENOMEM; > > Hmm. I wonder if it is worth adding a devm_bitmap_alloc? > > > > OK > > >> + > >> + bitmap_zero(info->xfer_alloc_table, desc->max_msg); > > kcalloc zeros the memory. > > > >> + > >> + /* Pre-initialize the buffer pointer to pre-allocated buffers */ > >> + for (i = 0, xfer = info->xfer_block; i < desc->max_msg; i++, xfer++) { > >> + xfer->rx.buf = devm_kcalloc(dev, sizeof(u8), desc->max_msg_size, > >> + GFP_KERNEL); > >> + if (!xfer->rx.buf) > >> + return -ENOMEM; > >> + > >> + xfer->tx.buf = xfer->rx.buf; > >> + init_completion(&xfer->done); > >> + } > >> + > >> + spin_lock_init(&info->xfer_lock); > >> + > >> + return 0; > >> +} > >> + > >> +static int scmi_mailbox_check(struct device_node *np) > >> +{ > >> + struct of_phandle_args arg; > >> + > >> + return of_parse_phandle_with_args(np, "mboxes", "#mbox-cells", 0, &arg); > >> +} > >> + > >> +static int scmi_mbox_free_channel(struct scmi_info *info) > > Some of the naming in here could do with being adjusted to be obviously > > 'balanced'. The moment I see a free I expect a matched alloc but in this > > case the alloc is done in scmi_mbox_chan_setup which at very least > > should be scmi_mbox_setup_channel and should really imply that it is > > doing allocation as well. > > > > That's inline with mailbox APIs. oh goody. Fair enough if ugly > ... > OK -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Sudeep Holla (2018-02-23 08:23:46) > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c > new file mode 100644 > index 000000000000..1e4d7a57779b > --- /dev/null > +++ b/drivers/clk/clk-scmi.c > + hws[idx] = &sclk->hw; > + } > + } > + > + return of_clk_add_hw_provider(np, of_clk_hw_onecell_get, clk_data); devm_of_clk_add_hw_provider? > +} > + > +static void scmi_clocks_remove(struct scmi_device *sdev) > +{ > + struct device *dev = &sdev->dev; > + struct device_node *np = dev->of_node; > + > + of_clk_del_provider(np); > +} > + Drop? You can keep my acked-by otherwise. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Sudeep Holla (2018-03-20 05:08:51) > > >> +} > >> + > >> +static void scmi_clocks_remove(struct scmi_device *sdev) > >> +{ > >> + struct device *dev = &sdev->dev; > >> + struct device_node *np = dev->of_node; > >> + > >> + of_clk_del_provider(np); > >> +} > >> + > > > > Drop? > > > > Sure, I will have to do these changes as add-on patch as this original > patch is already queued in ARM SoC tree for v4.17 > Ok! -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi all, ARM System Control and Management Interface(SCMI) is more flexible and easily extensible than any of the existing interfaces. Many vendors were involved in the making of this formal specification and is now published[1]. There is a strong trend in the industry to provide micro-controllers in systems to abstract various power, or other system management tasks. These controllers usually have similar interfaces, both in terms of the functions that are provided by them, and in terms of how requests are communicated to them. This specification is to standardise and avoid (any further) fragmentation in the design of such interface by various vendors. It currently doesn't support notification, asynchronous/delayed response, perf/power statistics region and sensor register region to name a few. Some of the ideas of message allocation/management are borrowed from TI SCI. Changes: v5[7]->v6: - Dropped notification APIs as it's not fully supported, leftovers from development - Removed unnecessary mutex usage for allocation - Replace udelay with cpu_relax, found nice helper spin_until_cond - Added inline to couple of functions in header files - Fixed few checkpatch errors v4[6]->v5: - Rebased to v4.15-rc6 - Updated all the gathered Ack/Reviewed-by tags(which includes all the drivers using SCMI protocol) v3[5]->v4[6]: - Added SCMI protocol bus to enumerate supported protocols as suggested by Arnd - Dropped the abstraction to mailbox as it may be optional v2[4]->v3: - Addressed various comments received so far(clock, hwmon and cpufreq drivers along with scmi drivers) - Hwmon driver now uses core layer to create and manage sysfs attributes - Added a shim layer to abstract the mailbox interface to support any custom adaptation required by the controller driver - Simple ARM MHU shim layer using newly added abstraction v1[3]->v2[4]: - Additional support for polling based DVFS and per protocol channels - Dependent drivers(clock, hwmon, cpufreq and power domains) - Various other review comments and issued found during testing addressed - Explicit binding for method dropped as even SMC based method are adviertised as mailbox RFC[2]->v1[3]: - Add generic mailbox binding for shared memory(Rob H) - Dropped compatibles per protocol(Suggested by Matt S) - Dropped lot of unnecessary pointer casting(Arnd B) - Dropped packing of structures(Arnd B) - Few other changes/additions based initial testing with firmware providing SCMI interface to OSPM -- Regards, Sudeep [1] http://infocenter.arm.com/help/topic/com.arm.doc.den0056a/index.html [2] https://marc.info/?l=linux-kernel&m=149685193627620&w=2 [3] https://marc.info/?l=linux-arm-kernel&m=149849482623492&w=2 [4] https://marc.info/?l=devicetree&m=150185763105926&w=2 [5] https://marc.info/?l=devicetree&m=150660452015351&w=2 [6] https://marc.info/?l=devicetree&m=150972061408961&w=2 [7] https://marc.info/?l=devicetree&m=151846114506163&w=2 Sudeep Holla (20): dt-bindings: mailbox: add support for mailbox client shared memory dt-bindings: arm: add support for ARM System Control and Management Interface(SCMI) protocol firmware: arm_scmi: add basic driver infrastructure for SCMI firmware: arm_scmi: add common infrastructure and support for base protocol firmware: arm_scmi: add scmi protocol bus to enumerate protocol devices firmware: arm_scmi: add initial support for performance protocol firmware: arm_scmi: add initial support for clock protocol firmware: arm_scmi: add initial support for power protocol firmware: arm_scmi: add initial support for sensor protocol firmware: arm_scmi: probe and initialise all the supported protocols firmware: arm_scmi: add support for polling based SCMI transfers firmware: arm_scmi: add option for polling based performance domain operations firmware: arm_scmi: refactor in preparation to support per-protocol channels firmware: arm_scmi: add per-protocol channels support using idr objects firmware: arm_scmi: add device power domain support using genpd clk: add support for clocks provided by SCMI hwmon: (core) Add hwmon_max to hwmon_sensor_types enumeration hwmon: add support for sensors exported via ARM SCMI cpufreq: add support for CPU DVFS based on SCMI message protocol cpufreq: scmi: add support for fast frequency switching Documentation/devicetree/bindings/arm/arm,scmi.txt | 179 +++++ .../devicetree/bindings/mailbox/mailbox.txt | 28 + MAINTAINERS | 11 +- drivers/clk/Kconfig | 10 + drivers/clk/Makefile | 1 + drivers/clk/clk-scmi.c | 213 +++++ drivers/cpufreq/Kconfig.arm | 11 + drivers/cpufreq/Makefile | 1 + drivers/cpufreq/scmi-cpufreq.c | 272 +++++++ drivers/firmware/Kconfig | 34 + drivers/firmware/Makefile | 1 + drivers/firmware/arm_scmi/Makefile | 5 + drivers/firmware/arm_scmi/base.c | 264 ++++++ drivers/firmware/arm_scmi/bus.c | 232 ++++++ drivers/firmware/arm_scmi/clock.c | 353 +++++++++ drivers/firmware/arm_scmi/common.h | 116 +++ drivers/firmware/arm_scmi/driver.c | 882 +++++++++++++++++++++ drivers/firmware/arm_scmi/perf.c | 492 ++++++++++++ drivers/firmware/arm_scmi/power.c | 232 ++++++ drivers/firmware/arm_scmi/scmi_pm_domain.c | 140 ++++ drivers/firmware/arm_scmi/sensors.c | 302 +++++++ drivers/hwmon/Kconfig | 12 + drivers/hwmon/Makefile | 1 + drivers/hwmon/scmi-hwmon.c | 233 ++++++ include/linux/hwmon.h | 1 + include/linux/scmi_protocol.h | 288 +++++++ 26 files changed, 4309 insertions(+), 5 deletions(-) create mode 100644 Documentation/devicetree/bindings/arm/arm,scmi.txt create mode 100644 drivers/clk/clk-scmi.c create mode 100644 drivers/cpufreq/scmi-cpufreq.c create mode 100644 drivers/firmware/arm_scmi/Makefile create mode 100644 drivers/firmware/arm_scmi/base.c create mode 100644 drivers/firmware/arm_scmi/bus.c create mode 100644 drivers/firmware/arm_scmi/clock.c create mode 100644 drivers/firmware/arm_scmi/common.h create mode 100644 drivers/firmware/arm_scmi/driver.c create mode 100644 drivers/firmware/arm_scmi/perf.c create mode 100644 drivers/firmware/arm_scmi/power.c create mode 100644 drivers/firmware/arm_scmi/scmi_pm_domain.c create mode 100644 drivers/firmware/arm_scmi/sensors.c create mode 100644 drivers/hwmon/scmi-hwmon.c create mode 100644 include/linux/scmi_protocol.h -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html