Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932683AbbBZOCY (ORCPT ); Thu, 26 Feb 2015 09:02:24 -0500 Received: from cantor2.suse.de ([195.135.220.15]:44651 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932251AbbBZOCU (ORCPT ); Thu, 26 Feb 2015 09:02:20 -0500 Message-ID: <54EF2769.5020805@suse.de> Date: Thu, 26 Feb 2015 15:02:17 +0100 From: Alexander Graf User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: "J. German Rivera" , gregkh@linuxfoundation.org, arnd@arndb.de, linux-kernel@vger.kernel.org CC: stuart.yoder@freescale.com, Kim.Phillips@freescale.com, scottwood@freescale.com, bhamciu1@freescale.com, R89243@freescale.com, Geoff.Thorpe@freescale.com, bhupesh.sharma@freescale.com, nir.erez@freescale.com, richard.schmitt@freescale.com Subject: Re: [PATCH v7 2/3] drivers/bus: Freescale Management Complex (fsl-mc) bus driver References: <1424816514-26369-1-git-send-email-German.Rivera@freescale.com> <1424816514-26369-3-git-send-email-German.Rivera@freescale.com> In-Reply-To: <1424816514-26369-3-git-send-email-German.Rivera@freescale.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 15193 Lines: 517 On 24.02.15 23:21, J. German Rivera wrote: > Platform device driver that sets up the basic bus infrastructure > for the fsl-mc bus type, including support for adding/removing > fsl-mc devices, register/unregister of fsl-mc drivers, and bus > match support to bind devices to drivers. > > Signed-off-by: J. German Rivera > Signed-off-by: Stuart Yoder > --- > Changes in v7: > - Refactored MC bus data structures > - Removed creation of fsl_mc_io object from fsl_mc_add_device() > - Addressed comments from Alex Graf: > * Use the 'ranges' property for the 'fsl,qoriq-mc' node as a way to > translate MC-returned addresses (bus relative addresses) to > system physical addresses. > > Changes addressed in v6: > - Fixed new checkpatch warnings > > Changes addressed in v5: > - Addressed comments from Alex Graf: > * Renamed dprc_get_container_id() call as dpmng_get_container_id(). > * Added TODO comment to use the 'ranges' property of the > fsl-mc DT node. > > Changes addressed in v4: > - Addressed comments from Alex Graf: > * Added missing scope limitations and more descriptive help > text for the FSL_MC_BUS config option. > - Fixed bug related to setting fsl_mc_bus_type.dev_root too > late. > > Changes in v3: > - Addressed changes from Kim Phillips: > * Renamed files: > drivers/bus/fsl-mc/fsl_mc_bus.c -> drivers/bus/fsl-mc/mc-bus.c > include/linux/fsl_mc.h -> include/linux/fsl/mc.h > include/linux/fsl_mc_private.h -> include/linux/fsl/mc-private.h > > - Addressed comments from Timur Tabi: > * Changed all functions that had goto out/error when no common cleanup > was done, to just have multiple return points. > * Replaced error cleanup boolean flags with multiple exit points. > > Changes in v2: > - Addressed comment from Joe Perches: > * Changed pr_debug to dev_dbg in fsl_mc_bus_match > > - Addressed comments from Kim Phillips and Alex Graf: > * Changed version check to allow the driver to run with MC > firmware that has major version number greater than or equal > to the driver's major version number. > * Removed minor version check > > - Removed unused variable parent_dev in fsl_mc_device_remove > > drivers/bus/Kconfig | 3 + > drivers/bus/Makefile | 3 + > drivers/bus/fsl-mc/Kconfig | 24 ++ > drivers/bus/fsl-mc/Makefile | 14 + > drivers/bus/fsl-mc/mc-bus.c | 758 +++++++++++++++++++++++++++++++++++++++++ > include/linux/fsl/mc-private.h | 67 ++++ > include/linux/fsl/mc.h | 136 ++++++++ > 7 files changed, 1005 insertions(+) > create mode 100644 drivers/bus/fsl-mc/Kconfig > create mode 100644 drivers/bus/fsl-mc/Makefile > create mode 100644 drivers/bus/fsl-mc/mc-bus.c > create mode 100644 include/linux/fsl/mc-private.h > create mode 100644 include/linux/fsl/mc.h > > diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig > index b99729e..dde3ec2 100644 > --- a/drivers/bus/Kconfig > +++ b/drivers/bus/Kconfig > @@ -67,4 +67,7 @@ config VEXPRESS_CONFIG > help > Platform configuration infrastructure for the ARM Ltd. > Versatile Express. > + > +source "drivers/bus/fsl-mc/Kconfig" > + > endmenu > diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile > index 2973c18..6abcab1 100644 > --- a/drivers/bus/Makefile > +++ b/drivers/bus/Makefile > @@ -15,3 +15,6 @@ obj-$(CONFIG_ARM_CCI) += arm-cci.o > obj-$(CONFIG_ARM_CCN) += arm-ccn.o > > obj-$(CONFIG_VEXPRESS_CONFIG) += vexpress-config.o > + > +# Freescale Management Complex (MC) bus drivers > +obj-$(CONFIG_FSL_MC_BUS) += fsl-mc/ > diff --git a/drivers/bus/fsl-mc/Kconfig b/drivers/bus/fsl-mc/Kconfig > new file mode 100644 > index 0000000..0d779d9 > --- /dev/null > +++ b/drivers/bus/fsl-mc/Kconfig > @@ -0,0 +1,24 @@ > +# > +# Freescale Management Complex (MC) bus drivers > +# > +# Copyright (C) 2014 Freescale Semiconductor, Inc. > +# > +# This file is released under the GPLv2 > +# > + > +config FSL_MC_BUS > + tristate "Freescale Management Complex (MC) bus driver" > + depends on OF && ARM64 > + help > + Driver to enable the bus infrastructure for the Freescale > + QorIQ Management Complex (fsl-mc). The fsl-mc is a hardware > + module of the QorIQ LS2 SoCs, that does resource management > + for hardware building-blocks in the SoC that can be used > + to dynamically create networking hardware objects such as > + network interfaces (NICs), crypto accelerator instances, > + or L2 switches. > + > + Only enable this option when building the kernel for > + Freescale QorQIQ LS2xxxx SoCs. > + > + > diff --git a/drivers/bus/fsl-mc/Makefile b/drivers/bus/fsl-mc/Makefile > new file mode 100644 > index 0000000..decd339 > --- /dev/null > +++ b/drivers/bus/fsl-mc/Makefile > @@ -0,0 +1,14 @@ > +# > +# Freescale Management Complex (MC) bus drivers > +# > +# Copyright (C) 2014 Freescale Semiconductor, Inc. > +# > +# This file is released under the GPLv2 > +# > +obj-$(CONFIG_FSL_MC_BUS) += mc-bus-driver.o > + > +mc-bus-driver-objs := mc-bus.o \ > + mc-sys.o \ > + dprc.o \ > + dpmng.o > + > diff --git a/drivers/bus/fsl-mc/mc-bus.c b/drivers/bus/fsl-mc/mc-bus.c > new file mode 100644 > index 0000000..01407cc > --- /dev/null > +++ b/drivers/bus/fsl-mc/mc-bus.c > @@ -0,0 +1,758 @@ > +/* > + * Freescale Management Complex (MC) bus driver > + * > + * Copyright (C) 2014 Freescale Semiconductor, Inc. > + * Author: German Rivera > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include "dprc-cmd.h" > + > +static struct kmem_cache *mc_dev_cache; > + > +/** > + * fsl_mc_bus_match - device to driver matching callback > + * @dev: the MC object device structure to match against > + * @drv: the device driver to search for matching MC object device id > + * structures > + * > + * Returns 1 on success, 0 otherwise. > + */ > +static int fsl_mc_bus_match(struct device *dev, struct device_driver *drv) > +{ > + const struct fsl_mc_device_match_id *id; > + struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev); > + struct fsl_mc_driver *mc_drv = to_fsl_mc_driver(drv); > + bool found = false; > + > + if (WARN_ON(!fsl_mc_bus_type.dev_root)) > + goto out; > + > + if (!mc_drv->match_id_table) > + goto out; > + > + /* > + * If the object is not 'plugged' don't match. > + * Only exception is the root DPRC, which is a special case. > + */ > + if ((mc_dev->obj_desc.state & DPRC_OBJ_STATE_PLUGGED) == 0 && > + &mc_dev->dev != fsl_mc_bus_type.dev_root) Does this mean there can only be one fsl-mc per machine? Is this a reasonable limitation? Wouldn't it be a lot cleaner to have individual buses for each fsl-mc dt node? If hardware only implements one today that's fine, but it means we don't have to completely reengineer everything tomorrow then. > + goto out; > + > + /* > + * Traverse the match_id table of the given driver, trying to find > + * a matching for the given MC object device. > + */ > + for (id = mc_drv->match_id_table; id->vendor != 0x0; id++) { > + if (id->vendor == mc_dev->obj_desc.vendor && > + strcmp(id->obj_type, mc_dev->obj_desc.type) == 0 && > + id->ver_major == mc_dev->obj_desc.ver_major && > + id->ver_minor == mc_dev->obj_desc.ver_minor) { > + found = true; > + break; > + } > + } > + > +out: > + dev_dbg(dev, "%smatched\n", found ? "" : "not "); > + return found; > +} > + > +/** > + * fsl_mc_bus_uevent - callback invoked when a device is added > + */ > +static int fsl_mc_bus_uevent(struct device *dev, struct kobj_uevent_env *env) > +{ > + pr_debug("%s invoked\n", __func__); > + return 0; > +} > + > +struct bus_type fsl_mc_bus_type = { > + .name = "fsl-mc", > + .match = fsl_mc_bus_match, > + .uevent = fsl_mc_bus_uevent, > +}; > +EXPORT_SYMBOL_GPL(fsl_mc_bus_type); Why does this have to get exported? > + > +static int fsl_mc_driver_probe(struct device *dev) > +{ > + struct fsl_mc_driver *mc_drv; > + struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev); > + int error; > + > + if (WARN_ON(!dev->driver)) > + return -EINVAL; > + > + mc_drv = to_fsl_mc_driver(dev->driver); > + if (WARN_ON(!mc_drv->probe)) > + return -EINVAL; > + > + error = mc_drv->probe(mc_dev); > + if (error < 0) { > + dev_err(dev, "MC object device probe callback failed: %d\n", > + error); > + return error; > + } > + > + return 0; > +} > + > +static int fsl_mc_driver_remove(struct device *dev) > +{ > + struct fsl_mc_driver *mc_drv = to_fsl_mc_driver(dev->driver); > + struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev); > + int error; > + > + if (WARN_ON(!dev->driver)) > + return -EINVAL; > + > + error = mc_drv->remove(mc_dev); > + if (error < 0) { > + dev_err(dev, > + "MC object device remove callback failed: %d\n", > + error); > + return error; > + } > + > + return 0; > +} > + > +static void fsl_mc_driver_shutdown(struct device *dev) > +{ > + struct fsl_mc_driver *mc_drv = to_fsl_mc_driver(dev->driver); > + struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev); > + > + mc_drv->shutdown(mc_dev); > +} > + > +/** > + * __fsl_mc_driver_register - registers a child device driver with the > + * MC bus > + * > + * This function is implicitly invoked from the registration function of > + * fsl_mc device drivers, which is generated by the > + * module_fsl_mc_driver() macro. > + */ > +int __fsl_mc_driver_register(struct fsl_mc_driver *mc_driver, > + struct module *owner) > +{ > + int error; > + > + mc_driver->driver.owner = owner; > + mc_driver->driver.bus = &fsl_mc_bus_type; > + > + /* > + * Set default driver callbacks, if not set by the child driver: > + */ > + if (mc_driver->probe) > + mc_driver->driver.probe = fsl_mc_driver_probe; I don't understand this. I thought you want to put them as default in case they're *not* set? > + > + if (mc_driver->remove) > + mc_driver->driver.remove = fsl_mc_driver_remove; > + > + if (mc_driver->shutdown) > + mc_driver->driver.shutdown = fsl_mc_driver_shutdown; > + > + error = driver_register(&mc_driver->driver); > + if (error < 0) { > + pr_err("driver_register() failed for %s: %d\n", > + mc_driver->driver.name, error); > + return error; > + } > + > + pr_info("MC object device driver %s registered\n", > + mc_driver->driver.name); > + return 0; > +} > +EXPORT_SYMBOL_GPL(__fsl_mc_driver_register); > + > +/** > + * fsl_mc_driver_unregister - unregisters a device driver from the > + * MC bus > + */ > +void fsl_mc_driver_unregister(struct fsl_mc_driver *mc_driver) > +{ > + driver_unregister(&mc_driver->driver); > +} > +EXPORT_SYMBOL_GPL(fsl_mc_driver_unregister); > + > +static int get_dprc_icid(struct fsl_mc_io *mc_io, > + int container_id, uint16_t *icid) > +{ > + uint16_t dprc_handle; > + struct dprc_attributes attr; > + int error; > + > + error = dprc_open(mc_io, container_id, &dprc_handle); > + if (error < 0) { > + pr_err("dprc_open() failed: %d\n", error); > + return error; > + } > + > + memset(&attr, 0, sizeof(attr)); > + error = dprc_get_attributes(mc_io, dprc_handle, &attr); > + if (error < 0) { > + pr_err("dprc_get_attributes() failed: %d\n", error); > + goto common_cleanup; > + } > + > + *icid = attr.icid; > + error = 0; > + > +common_cleanup: > + (void)dprc_close(mc_io, dprc_handle); > + return error; > +} > + > +static int translate_mc_addr(uint64_t mc_addr, phys_addr_t *phys_addr) This should probably check that both start and end are within range. > +{ > + int i; > + struct fsl_mc *mc = dev_get_drvdata(fsl_mc_bus_type.dev_root->parent); > + > + if (mc->num_translation_ranges == 0) { > + /* > + * Do identity mapping: > + */ > + *phys_addr = mc_addr; > + return 0; > + } > + [...] > +/** > + * fsl_mc_device_remove - Remove a MC object device from being visible to > + * Linux > + * > + * @mc_dev: Pointer to a MC object device object > + */ > +void fsl_mc_device_remove(struct fsl_mc_device *mc_dev) > +{ > + struct fsl_mc_bus *mc_bus = NULL; > + > + kfree(mc_dev->regions); > + > + /* > + * The device-specific remove callback will get invoked by device_del() > + */ > + device_del(&mc_dev->dev); > + put_device(&mc_dev->dev); > + > + if (strcmp(mc_dev->obj_desc.type, "dprc") == 0) { I think it'll make the code easier to understand if you extract the strcmp into a helper function that actually explains what it does. Something like fsl_mc_dev_is_container(). > + struct fsl_mc_io *mc_io = mc_dev->mc_io; > + > + mc_bus = to_fsl_mc_bus(mc_dev); > + fsl_destroy_mc_io(mc_io); > + if (&mc_dev->dev == fsl_mc_bus_type.dev_root) > + fsl_mc_bus_type.dev_root = NULL; > + } > + > + mc_dev->mc_io = NULL; > + if (mc_bus) > + devm_kfree(mc_dev->dev.parent, mc_bus); > + else > + kmem_cache_free(mc_dev_cache, mc_dev); > +} > +EXPORT_SYMBOL_GPL(fsl_mc_device_remove); [...] > + dev_info(&pdev->dev, > + "Freescale Management Complex Firmware version: %u.%u.%u\n", > + mc_version.major, mc_version.minor, mc_version.revision); > + > + if (mc_version.major < MC_VER_MAJOR) { > + dev_err(&pdev->dev, > + "ERROR: MC firmware version not supported by driver (driver version: %u.%u)\n", > + MC_VER_MAJOR, MC_VER_MINOR); > + error = -ENOTSUPP; > + goto error_cleanup_mc_io; > + } > + > + if (mc_version.major > MC_VER_MAJOR) { How about we introduce 2 defines for MIN/MAX from the beginning? Then we can adjust the range by only touching the defines later :). > + dev_warn(&pdev->dev, > + "WARNING: driver may not support newer MC firmware features (driver version: %u.%u)\n", > + MC_VER_MAJOR, MC_VER_MINOR); > + } [...] > +static int __init fsl_mc_bus_driver_init(void) > +{ > + int error; > + > + mc_dev_cache = kmem_cache_create("fsl_mc_device", > + sizeof(struct fsl_mc_device), 0, 0, > + NULL); > + if (!mc_dev_cache) { > + pr_err("Could not create fsl_mc_device cache\n"); > + return -ENOMEM; > + } > + > + error = bus_register(&fsl_mc_bus_type); > + if (error < 0) { > + pr_err("fsl-mc bus type registration failed: %d\n", error); > + goto error_cleanup_cache; > + } > + > + pr_info("fsl-mc bus type registered\n"); > + > + error = platform_driver_register(&fsl_mc_bus_driver); > + if (error < 0) { > + pr_err("platform_driver_register() failed: %d\n", error); > + goto error_cleanup_bus; > + } > + > + return 0; > + > +error_cleanup_bus: > + bus_unregister(&fsl_mc_bus_type); > + > +error_cleanup_cache: > + kmem_cache_destroy(mc_dev_cache); > + return error; > +} > + > +postcore_initcall(fsl_mc_bus_driver_init); How does this get hooked up when compiled as a module? Alex -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/