| Message ID | 1496851812-19623-1-git-send-email-sudeep.holla@arm.com |
|---|---|
| Headers | show |
| Series | firmware: ARM System Control and Management Interface(SCMI) support | expand |
On Wed, Jun 7, 2017 at 9:10 AM, 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. > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > drivers/firmware/arm_scmi/Makefile | 2 +- > drivers/firmware/arm_scmi/base.c | 290 +++++++++++++++++++++++++++++++++++++ > drivers/firmware/arm_scmi/common.h | 46 ++++++ > drivers/firmware/arm_scmi/driver.c | 67 +++++++++ > include/linux/scmi_protocol.h | 28 ++++ > 5 files changed, 432 insertions(+), 1 deletion(-) > create mode 100644 drivers/firmware/arm_scmi/base.c > > diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile > index 58e94c95e523..21d01d1d6b9c 100644 > --- a/drivers/firmware/arm_scmi/Makefile > +++ b/drivers/firmware/arm_scmi/Makefile > @@ -1,2 +1,2 @@ > obj-$(CONFIG_ARM_SCMI_PROTOCOL) = arm_scmi.o > -arm_scmi-y = driver.o > +arm_scmi-y = base.o driver.o > diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c > new file mode 100644 > index 000000000000..1191a409ea73 > --- /dev/null > +++ b/drivers/firmware/arm_scmi/base.c > @@ -0,0 +1,290 @@ > +/* > + * System Control and Management Interface (SCMI) Base Protocol > + * > + * Copyright (C) 2017 ARM Ltd. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + * > + * You should have received a copy of the GNU General Public License along > + * with this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#include "common.h" > + > +enum scmi_base_protocol_cmd { > + BASE_DISCOVER_VENDOR = 0x3, > + BASE_DISCOVER_SUB_VENDOR = 0x4, > + BASE_DISCOVER_IMPLEMENT_VERSION = 0x5, > + BASE_DISCOVER_LIST_PROTOCOLS = 0x6, > + BASE_DISCOVER_AGENT = 0x7, > + BASE_NOTIFY_ERRORS = 0x8, > +}; > + > +struct scmi_msg_resp_base_attributes { > + u8 num_protocols; > + u8 num_agents; > + __le16 reserved; > +} __packed; > + > +/** > + * scmi_base_attributes_get() - gets the implementation details > + * that are associated with the base protocol. > + * > + * @handle - SCMI entity handle > + * > + * Return: 0 on success, else appropriate SCMI error. > + */ > +static int scmi_base_attributes_get(struct scmi_handle *handle) > +{ > + int ret; > + struct scmi_xfer *t; > + struct scmi_msg_resp_base_attributes *attr_info; > + struct scmi_revision_info *rev = handle->version; > + > + ret = scmi_one_xfer_init(handle, PROTOCOL_ATTRIBUTES, > + SCMI_PROTOCOL_BASE, 0, sizeof(*attr_info), &t); > + if (ret) > + return ret; > + > + ret = scmi_do_xfer(handle, t); > + if (!ret) { > + attr_info = (struct scmi_msg_resp_base_attributes *)t->rx.buf; > + rev->num_protocols = attr_info->num_protocols; > + rev->num_agents = attr_info->num_agents; > + } > + > + scmi_put_one_xfer(handle, t); > + return ret; > +} > + > +/** > + * scmi_base_vendor_id_get() - gets vendor/subvendor identifier ASCII string. > + * > + * @handle - SCMI entity handle > + * @sub_vendor - specify true if sub-vendor ID is needed > + * > + * Return: 0 on success, else appropriate SCMI error. > + */ > +static int scmi_base_vendor_id_get(struct scmi_handle *handle, bool sub_vendor) > +{ > + u8 cmd; > + int ret, size; > + char *vendor_id; > + struct scmi_xfer *t; > + struct scmi_revision_info *rev = handle->version; > + > + if (sub_vendor) { > + cmd = BASE_DISCOVER_SUB_VENDOR; > + vendor_id = rev->sub_vendor_id; > + size = ARRAY_SIZE(rev->sub_vendor_id); > + } else { > + cmd = BASE_DISCOVER_VENDOR; > + vendor_id = rev->vendor_id; > + size = ARRAY_SIZE(rev->vendor_id); > + } > + > + ret = scmi_one_xfer_init(handle, cmd, SCMI_PROTOCOL_BASE, 0, size, &t); > + if (ret) > + return ret; > + > + ret = scmi_do_xfer(handle, t); > + if (!ret) > + memcpy(vendor_id, t->rx.buf, size); > + > + scmi_put_one_xfer(handle, t); > + return ret; > +} > + > +/** > + * scmi_base_implementation_version_get() - gets a vendor-specific > + * implementation 32-bit version. The format of the version number is > + * vendor-specific > + * > + * @handle - SCMI entity handle > + * > + * Return: 0 on success, else appropriate SCMI error. > + */ > +static int scmi_base_implementation_version_get(struct scmi_handle *handle) > +{ > + int ret; > + u32 *impl_ver; > + struct scmi_xfer *t; > + struct scmi_revision_info *rev = handle->version; > + > + ret = scmi_one_xfer_init(handle, BASE_DISCOVER_IMPLEMENT_VERSION, > + SCMI_PROTOCOL_BASE, 0, sizeof(*impl_ver), &t); > + if (ret) > + return ret; > + > + ret = scmi_do_xfer(handle, t); > + if (ret) { > + impl_ver = (u32 *)t->rx.buf; > + rev->impl_ver = le32_to_cpu(*impl_ver); > + } > + Should be (!ret) > + scmi_put_one_xfer(handle, t); > + return ret; > +} > + > +/** > + * scmi_base_implementation_list_get() - gets the list of protocols it is > + * OSPM is allowed to access > + * > + * @handle - SCMI entity handle > + * @protocols_imp - pointer to hold the list of protocol identifiers > + * > + * Return: 0 on success, else appropriate SCMI error. > + */ > +static int scmi_base_implementation_list_get(struct scmi_handle *handle, > + u8 *protocols_imp) > +{ > + u8 *list; > + int ret, loop; > + struct scmi_xfer *t; > + __le32 *num_skip, *num_ret; > + u32 tot_num_ret = 0, loop_num_ret; > + struct device *dev = handle->dev; > + > + ret = scmi_one_xfer_init(handle, BASE_DISCOVER_LIST_PROTOCOLS, > + SCMI_PROTOCOL_BASE, sizeof(*num_skip), 0, &t); > + if (ret) > + return ret; > + > + num_skip = (__le32 *)t->tx.buf; > + num_ret = (__le32 *)t->rx.buf; > + list = t->rx.buf + sizeof(*num_ret); > + > + do { > + /* Set the number of protocols to be skipped/already read */ > + *num_skip = cpu_to_le32(tot_num_ret); > + > + ret = scmi_do_xfer(handle, t); > + if (ret) > + break; > + > + loop_num_ret = le32_to_cpu(*num_ret); > + if (tot_num_ret + loop_num_ret > MAX_PROTOCOLS_IMP) { > + dev_err(dev, "No. of Protocol > MAX_PROTOCOLS_IMP"); > + break; > + } > + > + for (loop = 0; loop < loop_num_ret; loop++) > + protocols_imp[tot_num_ret + loop] = *(list + loop); > + > + tot_num_ret += loop_num_ret; > + } while (loop_num_ret); > + > + scmi_put_one_xfer(handle, t); > + return ret; > +} > + > +/** > + * scmi_base_discover_agent_get() - discover the name of an agent > + * > + * @handle - SCMI entity handle > + * @id - Agent identifier > + * @name - Agent identifier ASCII string > + * > + * An agent id of 0 is reserved to identify the platform itself. > + * Generally operating system is represented as "OSPM" > + * > + * Return: 0 on success, else appropriate SCMI error. > + */ > +static int > +scmi_base_discover_agent_get(struct scmi_handle *handle, int id, char *name) > +{ > + int ret; > + struct scmi_xfer *t; > + > + ret = scmi_one_xfer_init(handle, BASE_DISCOVER_AGENT, > + SCMI_PROTOCOL_BASE, sizeof(__le32), > + SCMI_MAX_STR_SIZE, &t); > + if (ret) > + return ret; > + > + *(__le32 *)t->tx.buf = cpu_to_le32(id); > + > + ret = scmi_do_xfer(handle, t); > + if (!ret) > + memcpy(name, t->rx.buf, SCMI_MAX_STR_SIZE); > + > + scmi_put_one_xfer(handle, t); > + return ret; > +} > + > +/** > + * scmi_base_error_notifications_enable() - register/unregister for > + * notifications of errors in the platform > + * > + * @handle - SCMI entity handle > + * @enable - Enable/Disable the notification > + * > + * Return: 0 on success, else appropriate SCMI error. > + */ > +static int scmi_base_error_notifications_enable(struct scmi_handle *handle, > + bool enable) > +{ > + int ret; > + struct scmi_xfer *t; > + > + ret = scmi_one_xfer_init(handle, BASE_NOTIFY_ERRORS, SCMI_PROTOCOL_BASE, > + sizeof(__le32), 0, &t); > + if (ret) > + return ret; > + > + *(__le32 *)t->tx.buf = cpu_to_le32(enable & BIT(0)); > + > + ret = scmi_do_xfer(handle, t); > + > + scmi_put_one_xfer(handle, t); > + return ret; > +} > + > +int scmi_base_protocol_init(struct scmi_handle *handle) > +{ > + int id, ret; > + u8 *prot_imp; > + u32 version; > + char name[SCMI_MAX_STR_SIZE]; > + struct device *dev = handle->dev; > + struct scmi_revision_info *rev = handle->version; > + > + ret = scmi_version_get(handle, SCMI_PROTOCOL_BASE, &version); > + if (ret) > + return ret; > + > + prot_imp = devm_kcalloc(dev, MAX_PROTOCOLS_IMP, sizeof(u8), GFP_KERNEL); > + if (!prot_imp) > + return -ENOMEM; > + > + rev->major_ver = PROTOCOL_REV_MAJOR(version), > + rev->minor_ver = PROTOCOL_REV_MINOR(version); > + > + scmi_base_attributes_get(handle); > + scmi_base_vendor_id_get(handle, false); > + scmi_base_vendor_id_get(handle, true); > + scmi_base_implementation_version_get(handle); > + scmi_base_implementation_list_get(handle, prot_imp); > + scmi_base_error_notifications_enable(handle, true); > + scmi_setup_protocol_implemented(handle, prot_imp); > + > + dev_info(dev, "SCMI Protocol %d.%d '%s:%s' Firmware Version 0x%x\n", > + rev->major_ver, rev->minor_ver, rev->vendor_id, > + rev->sub_vendor_id, rev->impl_ver); > + dev_dbg(dev, "Found %d protocol(s) %d agent(s)\n", rev->num_protocols, > + rev->num_agents); > + > + for (id = 0; id < rev->num_agents; id++) { > + scmi_base_discover_agent_get(handle, id, name); > + dev_dbg(dev, "Agent %d: %s\n", id, name); > + } > + > + return 0; > +} > diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h > index a3038efa3a8d..24bc51dcc6c5 100644 > --- a/drivers/firmware/arm_scmi/common.h > +++ b/drivers/firmware/arm_scmi/common.h > @@ -19,9 +19,50 @@ > */ > > #include <linux/completion.h> > +#include <linux/device.h> > +#include <linux/errno.h> > +#include <linux/kernel.h> > #include <linux/scmi_protocol.h> > #include <linux/types.h> > > +#define PROTOCOL_REV_MINOR_BITS 16 > +#define PROTOCOL_REV_MINOR_MASK ((1U << PROTOCOL_REV_MINOR_BITS) - 1) > +#define PROTOCOL_REV_MAJOR(x) ((x) >> PROTOCOL_REV_MINOR_BITS) > +#define PROTOCOL_REV_MINOR(x) ((x) & PROTOCOL_REV_MINOR_MASK) > +#define MAX_PROTOCOLS_IMP 16 > + > +enum scmi_std_protocol { > + SCMI_PROTOCOL_BASE = 0x10, > + SCMI_PROTOCOL_POWER = 0x11, > + SCMI_PROTOCOL_SYSTEM = 0x12, > + SCMI_PROTOCOL_PERF = 0x13, > + SCMI_PROTOCOL_CLOCK = 0x14, > + SCMI_PROTOCOL_SENSOR = 0x15, > +}; > + > +enum scmi_common_cmd { > + PROTOCOL_VERSION = 0x0, > + PROTOCOL_ATTRIBUTES = 0x1, > + PROTOCOL_MESSAGE_ATTRIBUTES = 0x2, > +}; > + > +/** > + * struct scmi_msg_resp_prot_version - Response for a message > + * > + * @major_version: Major version of the ABI that firmware supports > + * @minor_version: Minor version of the ABI that firmware supports > + * > + * In general, ABI version changes follow the rule that minor version increments > + * are backward compatible. Major revision changes in ABI may not be > + * backward compatible. > + * > + * Response to a generic message with message type SCMI_MSG_VERSION > + */ > +struct scmi_msg_resp_prot_version { > + __le16 minor_version; > + __le16 major_version; > +} __packed; > + > /** > * struct scmi_msg_hdr - Message(Tx/Rx) header > * > @@ -72,3 +113,8 @@ void scmi_put_one_xfer(struct scmi_handle *h, struct scmi_xfer *xfer); > int scmi_do_xfer(struct scmi_handle *h, struct scmi_xfer *xfer); > int scmi_one_xfer_init(struct scmi_handle *h, u8 msg_id, u8 msg_prot_id, > size_t tx_size, size_t rx_size, struct scmi_xfer **p); > +int scmi_version_get(struct scmi_handle *h, u8 protocol, u32 *version); > +bool scmi_is_protocol_implemented(struct scmi_handle *h, u8 prot_id); > +void scmi_setup_protocol_implemented(struct scmi_handle *handle, u8 *prot_imp); > + > +int scmi_base_protocol_init(struct scmi_handle *h); > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c > index f01e0643ac7d..7b653c932edc 100644 > --- a/drivers/firmware/arm_scmi/driver.c > +++ b/drivers/firmware/arm_scmi/driver.c > @@ -108,18 +108,22 @@ struct scmi_desc { > * @dev: Device pointer > * @desc: SoC description for this instance > * @handle: Instance of SCMI handle to send to clients > + * @version: SCMI revision information containing protocol version, > + * implementation version and (sub-)vendor identification. > * @cl: Mailbox Client > * @tx_chan: Transmit mailbox channel > * @rx_chan: Receive mailbox channel > * @tx_payload: Transmit mailbox channel payload area > * @rx_payload: Receive mailbox channel payload area > * @minfo: Message info > + * @protocols_imp: list of protocols implemented > * @node: list head > * @users: Number of users of this instance > */ > struct scmi_info { > struct device *dev; > const struct scmi_desc *desc; > + struct scmi_revision_info version; > struct scmi_handle handle; > struct mbox_client cl; > struct mbox_chan *tx_chan; > @@ -127,6 +131,7 @@ struct scmi_info { > void __iomem *tx_payload; > void __iomem *rx_payload; > struct scmi_xfers_info minfo; > + u8 *protocols_imp; > struct list_head node; > int users; > }; > @@ -445,6 +450,57 @@ int scmi_one_xfer_init(struct scmi_handle *handle, u8 msg_id, u8 msg_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(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; > + > + rev_info = (__le32 *)t->rx.buf; > + > + ret = scmi_do_xfer(handle, t); > + if (!ret) > + *version = le32_to_cpu(*rev_info); > + > + scmi_put_one_xfer(handle, t); > + return ret; > +} > + > +void scmi_setup_protocol_implemented(struct scmi_handle *handle, u8 *prot_imp) > +{ > + struct scmi_info *info = handle_to_scmi_info(handle); > + > + info->protocols_imp = prot_imp; > +} > + > +bool scmi_is_protocol_implemented(struct scmi_handle *handle, u8 prot_id) > +{ > + int i; > + struct scmi_info *info = handle_to_scmi_info(handle); > + > + if (!info->protocols_imp) > + return false; > + > + for (i = 0; i < MAX_PROTOCOLS_IMP; i++) > + if (info->protocols_imp[i] == prot_id) > + return true; > + return false; > +} > + > +/** > * scmi_handle_get() - Get the SCMI handle for a device > * > * @dev: pointer to device for which we want SCMI handle > @@ -642,6 +698,11 @@ static int scmi_probe(struct platform_device *pdev) > > desc = of_match_device(scmi_of_match, dev)->data; > > + if (of_property_match_string(np, "method", "mailbox-doorbell") < 0) { > + dev_err(dev, "invalid method property in %s\n", np->full_name); > + return -EINVAL; > + } > + > info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL); > if (!info) > return -ENOMEM; > @@ -687,6 +748,12 @@ static int scmi_probe(struct platform_device *pdev) > > handle = &info->handle; > handle->dev = info->dev; > + handle->version = &info->version; > + ret = scmi_base_protocol_init(handle); > + if (ret) { > + dev_err(dev, "unable to communicate with SCMI(%d)\n", ret); > + goto out; > + } > > mutex_lock(&scmi_list_mutex); > list_add_tail(&info->node, &scmi_list); > diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h > index 0c795a765110..901976fe211f 100644 > --- a/include/linux/scmi_protocol.h > +++ b/include/linux/scmi_protocol.h > @@ -17,13 +17,41 @@ > */ > #include <linux/types.h> > > +#define SCMI_MAX_STR_SIZE 16 > + > +/** > + * struct scmi_revision_info - version information 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; > }; > > #if IS_REACHABLE(CONFIG_ARM_SCMI_PROTOCOL) > -- > 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
On Wed, Jun 7, 2017 at 6:10 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > +struct scmi_msg_resp_power_attributes { > + __le16 num_domains; > + __le16 reserved; > + __le32 stats_addr_low; > + __le32 stats_addr_high; > + __le32 stats_size; > +} __packed; > + > +struct scmi_msg_resp_power_domain_attributes { > + __le32 flags; > +#define SUPPORTS_STATE_SET_NOTIFY(x) ((x) & BIT(31)) > +#define SUPPORTS_STATE_SET_ASYNC(x) ((x) & BIT(30)) > +#define SUPPORTS_STATE_SET_SYNC(x) ((x) & BIT(29)) > + u8 name[SCMI_MAX_STR_SIZE]; > +} __packed; I think it would be better to leave out the __packed here, which can lead to rather inefficient code. It's only really a problem when building with -mstrict-align, but it's better to write code in a way that doesn't rely on that. > +static int > +scmi_power_domain_attributes_get(struct scmi_handle *handle, u32 domain, > + struct power_dom_info *dom_info) > +{ > + int ret; > + struct scmi_xfer *t; > + struct scmi_msg_resp_power_domain_attributes *attr; > + > + ret = scmi_one_xfer_init(handle, POWER_DOMAIN_ATTRIBUTES, > + SCMI_PROTOCOL_POWER, sizeof(domain), > + sizeof(*attr), &t); > + if (ret) > + return ret; > + > + *(__le32 *)t->tx.buf = cpu_to_le32(domain); > + attr = (struct scmi_msg_resp_power_domain_attributes *)t->rx.buf; It seems you require a similar pattern in each caller of scmi_one_xfer_init(), but it seems a little clumsy to always require those casts, so maybe there is a nicer way to do this. How about making scmi_one_xfer_init() act as an allocation function and having it return the buffer or a PTR_ERR? It also seems odd to have it named 'init' but actually allocate the scmi_xfer structure rather than filling a local variable that gets passed by reference. Arnd -- 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 Roy, On 07/06/17 20:18, Roy Franz wrote: > On Wed, Jun 7, 2017 at 9:10 AM, 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. >> >> 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 | 737 +++++++++++++++++++++++++++++++++++++ >> include/linux/scmi_protocol.h | 48 +++ >> 6 files changed, 883 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 >> [...] >> + >> +#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 only executes in little-endian mode, so any buffers >> + * shared through SCMI should have their contents converted to little-endian >> + */ > > nit: > This really has more to do with the SCMI protocol defining everything > as little endian, rather the endian-ness of the SCP, right? There could be SCP > implementations that are not Cortex M3s or little endian. > Thanks for taking time to review this RFC, much appreciated. All valid points(on this and other patches) and fixed locally now. Also thanks for saving time in debugging these issues. I should be able to do some testing next week. -- 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
On 07/06/17 21:38, Arnd Bergmann wrote: > On Wed, Jun 7, 2017 at 6:10 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > >> +struct scmi_msg_resp_power_attributes { >> + __le16 num_domains; >> + __le16 reserved; >> + __le32 stats_addr_low; >> + __le32 stats_addr_high; >> + __le32 stats_size; >> +} __packed; >> + >> +struct scmi_msg_resp_power_domain_attributes { >> + __le32 flags; >> +#define SUPPORTS_STATE_SET_NOTIFY(x) ((x) & BIT(31)) >> +#define SUPPORTS_STATE_SET_ASYNC(x) ((x) & BIT(30)) >> +#define SUPPORTS_STATE_SET_SYNC(x) ((x) & BIT(29)) >> + u8 name[SCMI_MAX_STR_SIZE]; >> +} __packed; > > I think it would be better to leave out the __packed here, which > can lead to rather inefficient code. It's only really a problem when > building with -mstrict-align, but it's better to write code in a way that > doesn't rely on that. > I assume you are referring to above structure only and not general across all the structures ? I will have a look at this one. >> +static int >> +scmi_power_domain_attributes_get(struct scmi_handle *handle, u32 domain, >> + struct power_dom_info *dom_info) >> +{ >> + int ret; >> + struct scmi_xfer *t; >> + struct scmi_msg_resp_power_domain_attributes *attr; >> + >> + ret = scmi_one_xfer_init(handle, POWER_DOMAIN_ATTRIBUTES, >> + SCMI_PROTOCOL_POWER, sizeof(domain), >> + sizeof(*attr), &t); >> + if (ret) >> + return ret; >> + >> + *(__le32 *)t->tx.buf = cpu_to_le32(domain); >> + attr = (struct scmi_msg_resp_power_domain_attributes *)t->rx.buf; > > It seems you require a similar pattern in each caller of scmi_one_xfer_init(), > but it seems a little clumsy to always require those casts, so maybe there > is a nicer way to do this. How about making scmi_one_xfer_init() act > as an allocation function and having it return the buffer or a PTR_ERR? > Yes I agree it doesn't looks all nice. I have changed these few times while developing and then thought it's better to get some suggestions. I am open to any suggestions that will help to make these nicer. > It also seems odd to have it named 'init' but actually allocate the scmi_xfer > structure rather than filling a local variable that gets passed by reference. > It does initialise but partially. scmi_one_xfer_get does pure allocation while scmi_one_xfer_init initialise header variables and also tx/rx size. But if you think it's odd, I will looks at ways to make it better. -- 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
On Thu, Jun 8, 2017 at 11:39 AM, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > On 07/06/17 21:38, Arnd Bergmann wrote: >> On Wed, Jun 7, 2017 at 6:10 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> >>> +struct scmi_msg_resp_power_attributes { >>> + __le16 num_domains; >>> + __le16 reserved; >>> + __le32 stats_addr_low; >>> + __le32 stats_addr_high; >>> + __le32 stats_size; >>> +} __packed; >>> + >>> +struct scmi_msg_resp_power_domain_attributes { >>> + __le32 flags; >>> +#define SUPPORTS_STATE_SET_NOTIFY(x) ((x) & BIT(31)) >>> +#define SUPPORTS_STATE_SET_ASYNC(x) ((x) & BIT(30)) >>> +#define SUPPORTS_STATE_SET_SYNC(x) ((x) & BIT(29)) >>> + u8 name[SCMI_MAX_STR_SIZE]; >>> +} __packed; >> >> I think it would be better to leave out the __packed here, which >> can lead to rather inefficient code. It's only really a problem when >> building with -mstrict-align, but it's better to write code in a way that >> doesn't rely on that. >> > > I assume you are referring to above structure only and not general > across all the structures ? I will have a look at this one. I meant all of them, from my first look they all seem to have natural alignment on all members anyway. If there is one that doesn't, I would suggest annotating the individual unaligned members with __packed. >>> +static int >>> +scmi_power_domain_attributes_get(struct scmi_handle *handle, u32 domain, >>> + struct power_dom_info *dom_info) >>> +{ >>> + int ret; >>> + struct scmi_xfer *t; >>> + struct scmi_msg_resp_power_domain_attributes *attr; >>> + >>> + ret = scmi_one_xfer_init(handle, POWER_DOMAIN_ATTRIBUTES, >>> + SCMI_PROTOCOL_POWER, sizeof(domain), >>> + sizeof(*attr), &t); >>> + if (ret) >>> + return ret; >>> + >>> + *(__le32 *)t->tx.buf = cpu_to_le32(domain); >>> + attr = (struct scmi_msg_resp_power_domain_attributes *)t->rx.buf; >> >> It seems you require a similar pattern in each caller of scmi_one_xfer_init(), >> but it seems a little clumsy to always require those casts, so maybe there >> is a nicer way to do this. How about making scmi_one_xfer_init() act >> as an allocation function and having it return the buffer or a PTR_ERR? >> > > Yes I agree it doesn't looks all nice. I have changed these few times > while developing and then thought it's better to get some suggestions. I > am open to any suggestions that will help to make these nicer. > >> It also seems odd to have it named 'init' but actually allocate the scmi_xfer >> structure rather than filling a local variable that gets passed by reference. >> > > It does initialise but partially. scmi_one_xfer_get does pure allocation > while scmi_one_xfer_init initialise header variables and also tx/rx > size. But if you think it's odd, I will looks at ways to make it better. Yes, I'm still thinking about it, but I think we can do better. If a function has both allocation and initialization parts in it, I would probably name it *_alloc() rather than *_init(). What is the relation between scmi_one_xfer_get() and scmi_one_xfer_init()? Do we need both in some callers, or just one of the two? Arnd -- 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 08/06/17 12:06, Arnd Bergmann wrote: > On Thu, Jun 8, 2017 at 11:39 AM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> >> >> On 07/06/17 21:38, Arnd Bergmann wrote: >>> On Wed, Jun 7, 2017 at 6:10 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >>> >>>> +struct scmi_msg_resp_power_attributes { >>>> + __le16 num_domains; >>>> + __le16 reserved; >>>> + __le32 stats_addr_low; >>>> + __le32 stats_addr_high; >>>> + __le32 stats_size; >>>> +} __packed; >>>> + >>>> +struct scmi_msg_resp_power_domain_attributes { >>>> + __le32 flags; >>>> +#define SUPPORTS_STATE_SET_NOTIFY(x) ((x) & BIT(31)) >>>> +#define SUPPORTS_STATE_SET_ASYNC(x) ((x) & BIT(30)) >>>> +#define SUPPORTS_STATE_SET_SYNC(x) ((x) & BIT(29)) >>>> + u8 name[SCMI_MAX_STR_SIZE]; >>>> +} __packed; >>> >>> I think it would be better to leave out the __packed here, which >>> can lead to rather inefficient code. It's only really a problem when >>> building with -mstrict-align, but it's better to write code in a way that >>> doesn't rely on that. >>> >> >> I assume you are referring to above structure only and not general >> across all the structures ? I will have a look at this one. > > I meant all of them, from my first look they all seem to have natural > alignment on all members anyway. If there is one that doesn't, I would > suggest annotating the individual unaligned members with __packed. > OK, I will take a deeper look. Thanks for the suggestion. >>>> +static int >>>> +scmi_power_domain_attributes_get(struct scmi_handle *handle, u32 domain, >>>> + struct power_dom_info *dom_info) >>>> +{ >>>> + int ret; >>>> + struct scmi_xfer *t; >>>> + struct scmi_msg_resp_power_domain_attributes *attr; >>>> + >>>> + ret = scmi_one_xfer_init(handle, POWER_DOMAIN_ATTRIBUTES, >>>> + SCMI_PROTOCOL_POWER, sizeof(domain), >>>> + sizeof(*attr), &t); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + *(__le32 *)t->tx.buf = cpu_to_le32(domain); >>>> + attr = (struct scmi_msg_resp_power_domain_attributes *)t->rx.buf; >>> >>> It seems you require a similar pattern in each caller of scmi_one_xfer_init(), >>> but it seems a little clumsy to always require those casts, so maybe there >>> is a nicer way to do this. How about making scmi_one_xfer_init() act >>> as an allocation function and having it return the buffer or a PTR_ERR? >>> >> >> Yes I agree it doesn't looks all nice. I have changed these few times >> while developing and then thought it's better to get some suggestions. I >> am open to any suggestions that will help to make these nicer. >> >>> It also seems odd to have it named 'init' but actually allocate the scmi_xfer >>> structure rather than filling a local variable that gets passed by reference. >>> >> >> It does initialise but partially. scmi_one_xfer_get does pure allocation >> while scmi_one_xfer_init initialise header variables and also tx/rx >> size. But if you think it's odd, I will looks at ways to make it better. > > Yes, I'm still thinking about it, but I think we can do better. If a function > has both allocation and initialization parts in it, I would probably name > it *_alloc() rather than *_init(). > > What is the relation between scmi_one_xfer_get() and > scmi_one_xfer_init()? Do we need both in some callers, or > just one of the two? Currently only scmi_one_xfer_init is used. Initially I was using scmi_one_xfer_get and initialising at callsite. Strictly speaking, all the allocations are done at probe time, it's only grabbing and releasing one at a time at runtime, hence the name _get and _put. I can merge _init into _get. The way it-is is just artifact of how it got developed :( -- 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
On Wed, Jun 07, 2017 at 05:10:05PM +0100, Sudeep Holla wrote: > This patch adds devicetree binding for System Control and Management > Interface (SCMI) Message Protocol used between the Application Cores(AP) > and the System Control Processor(SCP). The MHU peripheral provides a > mechanism for inter-processor communication between SCP's M3 processor > 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. > > SCMI protocol is developed as better replacement to the existing SCPI > which is not flexible and easily extensible. > > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > Documentation/devicetree/bindings/arm/arm,scmi.txt | 193 +++++++++++++++++++++ > 1 file changed, 193 insertions(+) > create mode 100644 Documentation/devicetree/bindings/arm/arm,scmi.txt > > diff --git a/Documentation/devicetree/bindings/arm/arm,scmi.txt b/Documentation/devicetree/bindings/arm/arm,scmi.txt > new file mode 100644 > index 000000000000..d6e4b7eff199 > --- /dev/null > +++ b/Documentation/devicetree/bindings/arm/arm,scmi.txt > @@ -0,0 +1,193 @@ > +System Control and Management Interface (SCMI) Message Protocol > +---------------------------------------------------------- > + > +The SCMI is intended to allow agents such as OSPM to manage various functions > +that are provided by the hardware platform it is running on, including power > +and performance functions. > + > +This binding is intended to define the interface the firmware implementing > +the SCMI as described in ARM document number ARM DUI 0922B ("ARM System Control > +and Management Interface Platform Design Document")[0] provide for OSPM in > +the device tree. > + > +Required properties: > + > +- compatible : shall be "arm,scmi" Convince me that this genericish string is specific enough. > +- method : The method of calling the SCMI firmware. Only permitted value > + currently is: > + "mailbox-doorbell" : When mailbox doorbell is used as a mechanism > + to alert the presence of a messages and/or > + notification > +- mboxes: List of phandle and mailbox channel specifiers. It should contain > + exactly one or two mailboxes, one for transmitting messages("tx") > + and another optional for receiving the notifications("rx") if > + supported. > +- mbox-names: shall be "tx" or "rx" ...and optionally "rx" > +- shmem : List of phandle pointing to the shared memory(SHM) area between the > + processors using these mailboxes for IPC, one for each mailbox > + SHM can be any memory reserved for the purpose of this communication > + between the processors. Maybe the mailbox binding should have a standard property for this? > + > +See Documentation/devicetree/bindings/mailbox/mailbox.txt for more details > +about the generic mailbox controller and client driver bindings. > + > +Each protocol supported shall have a sub-node with corresponding compatible > +as described in the following sections. If the platform supports dedicated > +communication channel for a particular protocol, the 3 properties namely: > +mboxes, mbox-names and shmem shall be present in the sub-node corresponding > +to that protocol. > + > +Clock/Performance bindings for the clocks/OPPs based on SCMI Message Protocol > +------------------------------------------------------------ > + > +This binding uses the common clock binding[1]. > + > +Required properties: > +- compatible : shall be "arm,scmi-clocks" or "arm,scmi-perf-domains" > + arm,scmi-clocks: > + These clocks either provide an entire range of values > + between the limits or only discrete points each at fixed > + step size between the limits. The firmware provides > + mechanism to discover them. > + arm,scmi-perf-domains: > + These are OPPs(not just simple clocks), i.e. discrete > + performance levels that are supported by the platform. > + Again the firmware provides mechanism to discover the > + performance and other attributes associated with the > + levels. > + > +Other required properties for all clocks(all from common clock binding): > +- #clock-cells : Should be 1. Contains the Clock ID value used by SCMI commands. > +- clock-indices: The identifying number for the clocks(i.e.clock_id) in the > + node. It can be non linear and hence provide the mapping of > + identifiers. > + > +Power domain bindings for the power domains based on SCMI Message Protocol > +------------------------------------------------------------ > + > +This binding uses the generic power domain binding[4]. > + > +PM domain providers > +=================== > + > +Required properties: > + - #power-domain-cells : Should be 1. Contains the device or the power > + domain ID value used by SCMI commands. > + > +PM domain consumers > +=================== > + > +Required properties: > + - power-domains : A phandle and PM domain specifier as defined by bindings of > + the power controller specified by phandle. > + > +Sensor bindings for the sensors based on SCMI Message Protocol > +-------------------------------------------------------------- > +SCMI provides an API to access the various sensors on the SoC. > + > +Required properties: > +- compatible : should be "arm,scmi-sensors". > +- #thermal-sensor-cells: should be set to 1. This property follows the > + thermal device tree bindings[2]. > + > + Valid cell values are raw identifiers (Sensor ID) > + as used by the firmware. Refer to platform details > + for your implementation for the IDs to use. > + > +SRAM and Shared Memory for SCMI > +------------------------------- > + > +A small area of SRAM is reserved for SCMI communication between application > +processors and SCP. > + > +The properties should follow the generic mmio-sram description found in [3] > + > +Each sub-node represents the reserved area for SCMI. > + > +Required sub-node properties: > +- reg : The base offset and size of the reserved area with the SRAM > +- compatible : should be "arm,scp-shmem" for Non-secure SRAM based > + shared memory > + > +[0] http://infocenter.arm.com/help/topic/com.arm.doc.den0056a/index.html > +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt > +[2] Documentation/devicetree/bindings/thermal/thermal.txt > +[3] Documentation/devicetree/bindings/sram/sram.txt > +[4] Documentation/devicetree/bindings/power/power_domain.txt > + > +Example: > + > +sram: sram@50000000 { > + compatible = "arm,juno-sram-ns", "mmio-sram"; > + reg = <0x0 0x50000000 0x0 0x10000>; > + > + #address-cells = <1>; > + #size-cells = <1>; > + ranges = <0 0x0 0x50000000 0x10000>; > + > + cpu_scp_lpri: scp-shmem@0 { > + compatible = "arm,juno-scp-shmem"; > + reg = <0x0 0x200>; > + }; > + > + cpu_scp_hpri: scp-shmem@200 { > + compatible = "arm,juno-scp-shmem"; > + reg = <0x200 0x200>; > + }; > +}; > + > +mailbox: mailbox0@40000000 { > + .... > + #mbox-cells = <1>; > +}; > + > +scmi_protocol: scmi@2e000000 { > + compatible = "arm,scmi"; > + mboxes = <&mailbox 0 &mailbox 1>; > + shmem = <&cpu_scp_lpri &cpu_scp_hpri>; > + > + scmi_dvfs: clocks@0 { > + compatible = "arm,scmi-perf-domains"; > + #clock-cells = <1>; > + clock-indices = <0>, <1>, <2>; > + }; > + scmi_clk: clocks@3 { > + compatible = "arm,scmi-clocks"; > + #clock-cells = <1>; > + clock-indices = <3>, <4>; > + }; > + > + scmi_sensors: sensors { > + compatible = "arm,scmi-sensors"; > + #thermal-sensor-cells = <1>; > + }; > + > + scmi_devpd: power-domains { > + compatible = "arm,scmi-power-domains"; > + #power-domain-cells = <1>; > + }; > +}; > + > +cpu@0 { > + ... > + reg = <0 0>; > + clocks = <&scmi_dvfs 0>; > +}; > + > +hdlcd@7ff60000 { > + ... > + reg = <0 0x7ff60000 0 0x1000>; > + clocks = <&scmi_clk 4>; > + power-domains = <&scmi_devpd 1>; > +}; > + > +thermal-zones { > + soc_thermal { > + polling-delay-passive = <100>; > + polling-delay = <1000>; > + > + /* sensor ID */ > + thermal-sensors = <&scmi_sensors0 3>; > + ... > + }; > +}; > -- > 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
On 09/06/17 15:16, Rob Herring wrote: > On Wed, Jun 07, 2017 at 05:10:05PM +0100, Sudeep Holla wrote: >> This patch adds devicetree binding for System Control and Management >> Interface (SCMI) Message Protocol used between the Application Cores(AP) >> and the System Control Processor(SCP). The MHU peripheral provides a >> mechanism for inter-processor communication between SCP's M3 processor >> 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. >> >> SCMI protocol is developed as better replacement to the existing SCPI >> which is not flexible and easily extensible. >> >> Cc: Rob Herring <robh+dt@kernel.org> >> Cc: Mark Rutland <mark.rutland@arm.com> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >> --- >> Documentation/devicetree/bindings/arm/arm,scmi.txt | 193 +++++++++++++++++++++ >> 1 file changed, 193 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/arm/arm,scmi.txt >> >> diff --git a/Documentation/devicetree/bindings/arm/arm,scmi.txt b/Documentation/devicetree/bindings/arm/arm,scmi.txt >> new file mode 100644 >> index 000000000000..d6e4b7eff199 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/arm/arm,scmi.txt >> @@ -0,0 +1,193 @@ >> +System Control and Management Interface (SCMI) Message Protocol >> +---------------------------------------------------------- >> + >> +The SCMI is intended to allow agents such as OSPM to manage various functions >> +that are provided by the hardware platform it is running on, including power >> +and performance functions. >> + >> +This binding is intended to define the interface the firmware implementing >> +the SCMI as described in ARM document number ARM DUI 0922B ("ARM System Control >> +and Management Interface Platform Design Document")[0] provide for OSPM in >> +the device tree. >> + >> +Required properties: >> + >> +- compatible : shall be "arm,scmi" > > Convince me that this genericish string is specific enough. > Now that you raised this point, I think we generate so many 4 letter acronyms that it can collide. How about "arm,sys-ctl-mgmt-if" >> +- method : The method of calling the SCMI firmware. Only permitted value >> + currently is: >> + "mailbox-doorbell" : When mailbox doorbell is used as a mechanism >> + to alert the presence of a messages and/or >> + notification >> +- mboxes: List of phandle and mailbox channel specifiers. It should contain >> + exactly one or two mailboxes, one for transmitting messages("tx") >> + and another optional for receiving the notifications("rx") if >> + supported. >> +- mbox-names: shall be "tx" or "rx" > > ...and optionally "rx" > OK >> +- shmem : List of phandle pointing to the shared memory(SHM) area between the >> + processors using these mailboxes for IPC, one for each mailbox >> + SHM can be any memory reserved for the purpose of this communication >> + between the processors. > > Maybe the mailbox binding should have a standard property for this? > Do you mean as part of it's client binding ? If so, agreed. I can come up with that proposal. -- 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
On Fri, Jun 9, 2017 at 9:47 AM, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > On 09/06/17 15:16, Rob Herring wrote: >> On Wed, Jun 07, 2017 at 05:10:05PM +0100, Sudeep Holla wrote: >>> This patch adds devicetree binding for System Control and Management >>> Interface (SCMI) Message Protocol used between the Application Cores(AP) >>> and the System Control Processor(SCP). The MHU peripheral provides a >>> mechanism for inter-processor communication between SCP's M3 processor >>> 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. >>> >>> SCMI protocol is developed as better replacement to the existing SCPI >>> which is not flexible and easily extensible. >>> >>> Cc: Rob Herring <robh+dt@kernel.org> >>> Cc: Mark Rutland <mark.rutland@arm.com> >>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >>> --- >>> Documentation/devicetree/bindings/arm/arm,scmi.txt | 193 +++++++++++++++++++++ >>> 1 file changed, 193 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/arm/arm,scmi.txt >>> >>> diff --git a/Documentation/devicetree/bindings/arm/arm,scmi.txt b/Documentation/devicetree/bindings/arm/arm,scmi.txt >>> new file mode 100644 >>> index 000000000000..d6e4b7eff199 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/arm/arm,scmi.txt >>> @@ -0,0 +1,193 @@ >>> +System Control and Management Interface (SCMI) Message Protocol >>> +---------------------------------------------------------- >>> + >>> +The SCMI is intended to allow agents such as OSPM to manage various functions >>> +that are provided by the hardware platform it is running on, including power >>> +and performance functions. >>> + >>> +This binding is intended to define the interface the firmware implementing >>> +the SCMI as described in ARM document number ARM DUI 0922B ("ARM System Control >>> +and Management Interface Platform Design Document")[0] provide for OSPM in >>> +the device tree. >>> + >>> +Required properties: >>> + >>> +- compatible : shall be "arm,scmi" >> >> Convince me that this genericish string is specific enough. >> > > Now that you raised this point, I think we generate so many 4 letter > acronyms that it can collide. How about "arm,sys-ctl-mgmt-if" I was more concerned about needing versioning or vendor specific compatible strings that we needed with SCPI. Is there a spec version or is that discoverable? >>> +- method : The method of calling the SCMI firmware. Only permitted value >>> + currently is: >>> + "mailbox-doorbell" : When mailbox doorbell is used as a mechanism >>> + to alert the presence of a messages and/or >>> + notification >>> +- mboxes: List of phandle and mailbox channel specifiers. It should contain >>> + exactly one or two mailboxes, one for transmitting messages("tx") >>> + and another optional for receiving the notifications("rx") if >>> + supported. >>> +- mbox-names: shall be "tx" or "rx" >> >> ...and optionally "rx" >> > > OK > >>> +- shmem : List of phandle pointing to the shared memory(SHM) area between the >>> + processors using these mailboxes for IPC, one for each mailbox >>> + SHM can be any memory reserved for the purpose of this communication >>> + between the processors. >> >> Maybe the mailbox binding should have a standard property for this? >> > > Do you mean as part of it's client binding ? If so, agreed. I can come > up with that proposal. Yes. Rob -- 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 09/06/17 16:39, Rob Herring wrote: > On Fri, Jun 9, 2017 at 9:47 AM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> >> >> On 09/06/17 15:16, Rob Herring wrote: >>> On Wed, Jun 07, 2017 at 05:10:05PM +0100, Sudeep Holla wrote: >>>> This patch adds devicetree binding for System Control and Management >>>> Interface (SCMI) Message Protocol used between the Application Cores(AP) >>>> and the System Control Processor(SCP). The MHU peripheral provides a >>>> mechanism for inter-processor communication between SCP's M3 processor >>>> 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. >>>> >>>> SCMI protocol is developed as better replacement to the existing SCPI >>>> which is not flexible and easily extensible. >>>> >>>> Cc: Rob Herring <robh+dt@kernel.org> >>>> Cc: Mark Rutland <mark.rutland@arm.com> >>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >>>> --- >>>> Documentation/devicetree/bindings/arm/arm,scmi.txt | 193 +++++++++++++++++++++ >>>> 1 file changed, 193 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/arm/arm,scmi.txt >>>> >>>> diff --git a/Documentation/devicetree/bindings/arm/arm,scmi.txt b/Documentation/devicetree/bindings/arm/arm,scmi.txt >>>> new file mode 100644 >>>> index 000000000000..d6e4b7eff199 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/arm/arm,scmi.txt >>>> @@ -0,0 +1,193 @@ >>>> +System Control and Management Interface (SCMI) Message Protocol >>>> +---------------------------------------------------------- >>>> + >>>> +The SCMI is intended to allow agents such as OSPM to manage various functions >>>> +that are provided by the hardware platform it is running on, including power >>>> +and performance functions. >>>> + >>>> +This binding is intended to define the interface the firmware implementing >>>> +the SCMI as described in ARM document number ARM DUI 0922B ("ARM System Control >>>> +and Management Interface Platform Design Document")[0] provide for OSPM in >>>> +the device tree. >>>> + >>>> +Required properties: >>>> + >>>> +- compatible : shall be "arm,scmi" >>> >>> Convince me that this genericish string is specific enough. >>> >> >> Now that you raised this point, I think we generate so many 4 letter >> acronyms that it can collide. How about "arm,sys-ctl-mgmt-if" > > I was more concerned about needing versioning or vendor specific > compatible strings that we needed with SCPI. Is there a spec version > or is that discoverable? > Ah ok, it's discoverable and each protocol must implement PROTOCOL_VERSION. We have specification version implemented, vendor id and firmware implementation version all of which is mandatory. >>>> +- method : The method of calling the SCMI firmware. Only permitted value >>>> + currently is: >>>> + "mailbox-doorbell" : When mailbox doorbell is used as a mechanism >>>> + to alert the presence of a messages and/or >>>> + notification >>>> +- mboxes: List of phandle and mailbox channel specifiers. It should contain >>>> + exactly one or two mailboxes, one for transmitting messages("tx") >>>> + and another optional for receiving the notifications("rx") if >>>> + supported. >>>> +- mbox-names: shall be "tx" or "rx" >>> >>> ...and optionally "rx" >>> >> >> OK >> >>>> +- shmem : List of phandle pointing to the shared memory(SHM) area between the >>>> + processors using these mailboxes for IPC, one for each mailbox >>>> + SHM can be any memory reserved for the purpose of this communication >>>> + between the processors. >>> >>> Maybe the mailbox binding should have a standard property for this? >>> >> >> Do you mean as part of it's client binding ? If so, agreed. I can come >> up with that proposal. > > Yes. Thanks, will do. -- 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
Hi Matt, Thanks for starting this discussion on the list. On Fri, Jun 09, 2017 at 01:12:50PM -0500, Matt Sealey wrote: > Hullo all, > > This is a long one.. apologies for odd linefeeds and so on. > > On Wed, Jun 7, 2017 at 11:10 AM, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > +Clock/Performance bindings for the clocks/OPPs based on SCMI Message > Protocol > > +------------------------------------------------------------ > > + > > +This binding uses the common clock binding[1]. > > + > > +Required properties: > > +- compatible : shall be "arm,scmi-clocks" or "arm,scmi-perf-domains" > > After a little thought, there are a couple objections to be made here. > Firstly, the SCMI protocol families are discoverable - you only really need > to know that it is usable (and where to use it, mboxes etc.) - whether it > supports the clock management, performance domains, power domains et al. > protocols is a function of querying the base protocol for a list. > > These protocols are identified by a value, several of which are > standardized, some being vendor extension numbers. All protocols must be > able to be queried for information. > Agreed on most the above. > As such, defining compatible properties for each protocol is treading that > fine line of tying device trees to particular driver subsystems and giving > operating systems an ability to ignore any discovery procedure. While I > can't make a case for clock management (which should obviously conform with > a particular clock management definition in DT, as already defined), there > is plenty of past evidence of bindings for particular devices being mis-used > or used in non-intended ways (regulators as reset GPIOs is the one that > immediately came to mind) in lieu of a more fleshed out way of defining a > particular class of device for a binding. The same would be true of tying a > 'performance domain' to the concept of clock management. > As I mentioned in private, I reused clock for simplicity and most of the platforms already do that. But yes, that's no valid argument to just continue the legacy. I am fine to break clock and performance domains. The main problem IMO is that it's not well defined either in theis specification or architecture to an extent that we can define a standard binding and live with it. We are/already have seen lost of churn around this bindings and that's one of the reason I chose to reuse existing bindings. > From the point of view of being able to specify things against a particular > binding (whatever that might be), one could imagine something that mapped > protocols to those bindings without introducing compatible names. SCMI ids > would be verbatim, and per-protocol. Things like clock-indices are therefore > not relevant and defining which indices go with which protocol at the SCMI > level isn't needed anymore. It is really up to the protocol how many cells > it would need to define it's protocol behavior but for the purpose of some > standardization, we could imagine a binding that defined protocols as such: > > scmi: arm_scmi { > compatible = "arm,scmi,1.0"; I prefer v1.0 dropped based on the same argument that it's discoverable. If we don't agree on that then the whole discussion falls apart. I assume you agree on dropping versioning just to continue the discussion here. > mboxes = <y>; > shmem = <x>; > protocols { > scmi_clocks: protocol@0x14 { > #whatever-cells = 3; > }; > foo_smic: protocol@0x89 { > #foo-cells = 4; > #bar-cells = 5; > }; > }; > }; > > uart: myuart@80000000 { > compatible = "arm,pl011"; > clocks = <&foo_smic 3>; > }; > > If you manage to get a device tree that specifies a clock but there is no > protocol 0x89 then you're just as hosed as if you specified an > arm,scmi-clocks node when the protocol was not supported by SCMI itself, so > we don't gain any new dangers, but we do gain the ability to instantiate > SCMI, discover protocols, and then load drivers against those protocols, > without duplicating the discovery process with a hardcoded tree. Device > trees, from my point of view, are a contract between the SoC & board > designer and the OS (helped along by firmware, hopefully). They shouldn't be > dictating the driver behavior to be applied at this kind of level. Agreed. > Device trees need to be rock solid - agile development is fine but as soon > as you ship, changing the device tree means cutting off support for existing > software, or only working with augmented features on new software and > severely reduced functionality on old software. That can be as simple as not > being able to go to Turbo mode, or as bad as an inability to apply thermal > limits and burning someone's board. If we define a specific binding of a > specific protocol to a specific way of interacting with that device which is > a purely software construct (like treating performance domain as an abstract > clock interface with a particular number of cells or clock-like behavior), > then you lock down protocols to *existing* OS subsystems. That means > maintaining that subsystem and device tree specification to retain behavior, > which is a lot of maintaining. In total agreement with you. > It shouldn't be necessary to actually define which protocols exist and what > number of cells they use in the device tree binding, because this is > actually documented elsewhere. Just as we do not create compatible > properties for new devices which work the same as old ones, or redefine > features which would otherwise be ascertained by some kind of discovery (PCI > devices, USB devices, but even as simple just reading out an ID register > from a device to determine if it has a particular feature), we shouldn't do > this to lock down a further discovery model for activity types or > programmers' models under those protocols. Reasonable. > Here's where I fall down: with a variable number of cells per protocol > required (potentially) and no method to be able to assign a particular > protocol's device or functionality to something else, discovery of what maps > where has to be done as well. There's little of that in SCMI, that is to say > it wouldn't be possible to infer that clock_id 20 in protocol 0x14 is the > clock that goes with cpu@0. It is also, per the SCMI spec, a firmware table > responsibility to define any dependencies on other parts of the protocol > (for instance, clock trees). The best I can think of right now is that this > would just have to be done on a global SCMI level: > > scmi: arm_scmi { > compatible = "arm,scmi,1.0"; > mboxes = <y>; > shmem = <x>; > disable-protocols = <0x87, 0x88>; > // these are buggy, don't load drivers even if they're implemented > > #whatever-cells = 3; > required-whatever-prop; > > #foo-cells = 4; > > #bar-cells = 5; > optional-bar-prop = <5, 10, 15>; > > #clock-cells = 10; > }; > > #define SCMI_PROTO_CLOCKMGMT 0x14 > #define SCMI_PROTO_VENDOR_CLOCKS 0x89 > uart: myuart { > compatible = "arm,pl011"; > clocks = <&scmi SCMI_PROTO_CLOCKMGMT 3 0 0 0 0 0 0 0 0>, <&scmi > SCMIU_PROTO_VENDOR_CLOCKS 3 4 7 99 0 0 0x9000 3>; > }; > OK, I really don't like this. But if other's think this is better, then I am fine with that. I prefer the first option you have present. IMO we might collide with the property name soon i.e. 2 different existing bindings having same property name. > .. it is up to the driver to figure out what exactly this does in real > life, without having to lock it to the clock et al. binding, but at least > all drivers using protocols must be able to parse the number of cells > defined. If a protocol only needs 3 cells, but another needs 10 cells for > the same thing, then both protocols will by definition be defined as 10-cell > groups. It implies hat any device that can be controlled or affected by SCMI > can list the devices, and the protocol driver will be required to parse the > remaining cells and ignore them. As long as the device tree can cover all > cases in it's #foo-cells or other binding properties, it would be most > flexible here. > > I don't like the lockdown of having to cover every binding that gets used > whether it's truly for that device type or not, but it would be the most > flexible within the current framework, without defeating discovery. Sounds good, but as I said I am worried if we collide. > We would do well to come up with a way of defining abstract interfaces to > firmware or other processors that don't rely on there being a fixed binding > that dictates what kind of device it is, where there isn't a way of defining > that device type in a generic manner. There's no way of doing this right now > and letting the driver in the OS know what's necessary - well, there is, > things like RTAS support on CHRP got away with this in the old days, and > PSCI does something very similar (which covers quite a lot of CPU management > and also system domain activity), so it's not like we've come up against the > issue before. But it's not been resolved when a device node would need to > refer back to those abstraction interface nodes where there's not a > reasonable way of binding the definition of the reference. Exactly, I am trying to reuse existing bindings with minimum change to provide the glue to SCMI interface. But I am open for any suggestions. > RTAS had a property in every device node that depended on it and would be > affected by it, "used-by-rtas". Perhaps we could augment devices with an > "managed-by" property likewise to point to the protocol node. > #protocol-cells might be a nice way of defining cell sizes generically > (where protocol would be the name of the protocol - #scmi-cells perhaps - > and the format of #scmi-cells would need to include the protocol number). > Wrapping that up in a generic binding means each protocol then gets control > of it's own world, and each device that is affected by that protocol would > be able to realize this. Interesting, I need more opinions here. > If anyone comes up with a particular PSCI-like protocol for anything here > that kind of adaptable binding for device/platform abstraction frameworks > with non-discrete methods of working (i.e. it might not be able to be > defined as "just a clock" or "just a power domain" or "just a thermal > framework" or any combination without reducing functionality that would > otherwise come from some built-in discovery system) would come in very > handy. > > Just thoughts right now, though. I definitely don't have the whole story > down, but it is definitely something to think about. Sure, thanks again for such a detailed mail. Hope to see more discussions if we need to make such drastic changes. -- 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
Hi all, Let me begin admitting that we are introducing yet another protocol to achieve same things as many existing protocols like ARM SCPI, TI SCI, QCOM RPM, Nvidia Tegra BPMP, and so on. All I can say is that this new ARM System Control and Management Interface(SCMI) is more flexible and easily extensible than any of the existing ones. Many vendors were involved in the making of this formal specification and is now officially 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. This patch set is intended to get feedback on the design and structure of the code. This is not complete and not fully tested due to non-availability of firmware with full feature set at this time. It currently doesn't support notification, asynchronous/delayed response, perf/power statistics region and sensor register region to name a few. I have borrowed some of the ideas of message allocation/management from TI SCI. -- Regards, Sudeep [1] http://infocenter.arm.com/help/topic/com.arm.doc.den0056a/index.html Sudeep Holla (8): Documentation: add DT binding 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 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 Documentation/devicetree/bindings/arm/arm,scmi.txt | 193 +++++ drivers/firmware/Kconfig | 21 + drivers/firmware/Makefile | 1 + drivers/firmware/arm_scmi/Makefile | 2 + drivers/firmware/arm_scmi/base.c | 290 +++++++ drivers/firmware/arm_scmi/clock.c | 340 +++++++++ drivers/firmware/arm_scmi/common.h | 127 ++++ drivers/firmware/arm_scmi/driver.c | 832 +++++++++++++++++++++ drivers/firmware/arm_scmi/perf.c | 398 ++++++++++ drivers/firmware/arm_scmi/power.c | 237 ++++++ drivers/firmware/arm_scmi/sensors.c | 269 +++++++ include/linux/scmi_protocol.h | 160 ++++ 12 files changed, 2870 insertions(+) create mode 100644 Documentation/devicetree/bindings/arm/arm,scmi.txt create mode 100644 drivers/firmware/arm_scmi/Makefile create mode 100644 drivers/firmware/arm_scmi/base.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/sensors.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