On Fri, 23 Feb 2018 16:23:35 +0000 Sudeep Holla <sudeep.holla@xxxxxxx> 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@xxxxxxxx> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> > Signed-off-by: Sudeep Holla <sudeep.holla@xxxxxxx> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
