Re: [PATCH v7 2/3] drivers/bus: Freescale Management Complex (fsl-mc) bus driver

[Alexander Graf <agraf@suse.de>]

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 <German.Rivera@freescale.com>
> Signed-off-by: Stuart Yoder <stuart.yoder@freescale.com>
> ---
> 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 <German.Rivera@freescale.com>
> + *
> + * 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 <linux/fsl/mc-private.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_address.h>
> +#include <linux/ioport.h>
> +#include <linux/slab.h>
> +#include <linux/limits.h>
> +#include <linux/fsl/dpmng.h>
> +#include <linux/fsl/mc-sys.h>
> +#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