[PATCH v4 0/5] Device Bus support for Marvell EBU SoC

[PATCH v4 0/5] Device Bus support for Marvell EBU SoC

[Ezequiel Garcia]

This patchset continues the effort to add Device Bus support
on Marvell EBU SoC.

If you're reading this for the first time you can grab the full details here:

  http://www.spinics.net/lists/arm-kernel/msg236151.html

This new version changes the way each chip select is described in the DT.
Now there's a separate node for each Device Bus chip select, like this:

	devbus-bootcs at d0010400 {
		compatible = "marvell,mvebu-devbus";
		reg = <0xd0010400 0x8>;
		ranges = <0 0 0xf0000000 0x8000000>; /* CS0 @addr 0xf000000, size 0x8000000 */

		device at 0 {
			reg = <0 0 0x8000000>;
		}
	};

	// ...

	devbus-cs1 at d0010410 {
		compatible = "marvell,mvebu-devbus";
		reg = <0xd0010410 0x8>;
	};

In particular, please note that once the window handling is properly added
to the DT, and we remove the mbus usage from this driver, in the above DT nodes
the ranges property value will have to be changed, but the ABI will be the same.

This applies on top of v3.9-rc5 with mbus patches applied, which can 
be found in Jason Cooper's mvebus/soc branch.
If anyone wants to try this, there's a public branch available at:

  https://github.com/MISL-EBU-System-SW/mainline-public/tree/mvebu-devbus-v4

How does this version look regarding its inclusion in v3.10?
@Jason, @Arnd: if you think this v4 meets your expectation,
I'd like to have your Ack so I can ask Jason Cooper to apply this.

If the driver looks good and the temporary window handling is accepted,
then I can prepare a rebased patchset on top of Gregory's LPAE work,
or on top of any other required patches.

Changes from v3:

  * Rename devbus,dev-width to devbus,bus-width and changed the
    parameter value from bytes to bits: <8>, <16>, ...
    Suggested by Arnd Bergmann.

  * As suggested by Jason Gunthorpe and agreed by Arnd Bergmann.
    Reworked the whole devbus child chip-select device tree design.
    Now there's a separate devbus at addr DT node for each controller
    chip select. The impact of this is:
      1. Timing parameters now go into the parent node.
      2. There can only be one child per devbus node.

  * Remove unneeded bank-width parameter read.

Changes from v2:

  * Replace the 'NOR' specific probe by a generic probe.
    Currently, we don't see anything specific to NOR in this driver.
    Suggested by Jason Gunthorpe.

  * Add a check to detect that every timing parameter is specified
    in the device tree node.
    Suggested by Jason Gunthorpe.

Changes from v1:

  * Added a remove() function to unregister childs and remove address windows,
    as suggested by Thomas Petazzoni.

  * Remove unneeded 'inline' prefix on functions as suggested by Thomas Petazzoni.

  * Timings parameters are now mandatory and expressed in picoseconds,
    as requested by Jason Gunthorpe.
    This is consistent with OMAP's GPMC dts binding.

  * Fix documentation as suggested by Jason Gunthorpe.

Thanks!

Ezequiel Garcia (5):
  drivers: memory: Introduce Marvell EBU Device Bus driver
  ARM: mvebu: Add Device Bus support for Armada 370/XP SoC
  ARM: mvebu: Add support for NOR flash device on Armada XP-GP board
  ARM: mvebu: Add support for NOR flash device on Openblocks AX3 board
  ARM: mvebu: Add Device Bus and CFI flash memory support to defconfig

 .../bindings/memory-controllers/mvebu-devbus.txt   | 156 ++++++++++
 arch/arm/boot/dts/armada-370-xp.dtsi               |  45 +++
 arch/arm/boot/dts/armada-xp-gp.dts                 |  29 ++
 arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts   |  29 ++
 arch/arm/configs/mvebu_defconfig                   |   7 +
 drivers/memory/Kconfig                             |  10 +
 drivers/memory/Makefile                            |   1 +
 drivers/memory/mvebu-devbus.c                      | 344 +++++++++++++++++++++
 8 files changed, 621 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/mvebu-devbus.txt
 create mode 100644 drivers/memory/mvebu-devbus.c

-- 
1.8.1.5

[PATCH v4 1/5] drivers: memory: Introduce Marvell EBU Device Bus driver

[Ezequiel Garcia]

Marvell EBU SoCs such as Armada 370/XP, Orion5x (88f5xxx) and
Discovery (mv78xx0) supports a Device Bus controller to access several
kinds of memories and I/O devices (NOR, NAND, SRAM, FPGA).

This commit adds a driver to handle this controller. So far only
Armada 370, Armada XP and Discovery SoCs are supported.

The driver must be registered through a device tree node;
as explained in the binding document.

Each Device Bus node in the device tree, represents a chip select.
For each node, the driver will:
  * set timing parameters
  * register one child device
  * setup an address decoding window, using the mbus driver

Keep in mind the address decoding window setup is only a temporary hack.
This code will be removed from this devbus driver as soon as a proper device
tree binding for the mbus driver is added.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 .../bindings/memory-controllers/mvebu-devbus.txt   | 156 ++++++++++
 drivers/memory/Kconfig                             |  10 +
 drivers/memory/Makefile                            |   1 +
 drivers/memory/mvebu-devbus.c                      | 344 +++++++++++++++++++++
 4 files changed, 511 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/mvebu-devbus.txt
 create mode 100644 drivers/memory/mvebu-devbus.c

diff --git a/Documentation/devicetree/bindings/memory-controllers/mvebu-devbus.txt b/Documentation/devicetree/bindings/memory-controllers/mvebu-devbus.txt
new file mode 100644
index 0000000..9ddcd6c
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/mvebu-devbus.txt
@@ -0,0 +1,156 @@
+Device tree bindings for MVEBU Device Bus controllers
+
+The Device Bus controller available in some Marvell's SoC allows to control
+different types of standard memory and I/O devices such as NOR, NAND, and FPGA.
+The actual devices are instantiated from the child nodes of a Device Bus node.
+
+Required properties:
+
+ - compatible:          Currently only Armada 370/XP SoC are supported,
+                        with this compatible string:
+
+                        marvell,mvebu-devbus
+
+ - reg:                 A resource specifier for the register space.
+                        This is the base address of a chip select within
+			the controller's register space.
+                        (see the example below)
+
+ - #address-cells:      Must be set to 2 to allow memory address translation
+ - #size-cells:         Must be set to 1 to allow CS address passing
+ - ranges:              Must be set up to reflect the memory layout with four
+                        integer values for each chip-select line in use:
+                        <cs-number> 0 <physical address of mapping> <size>
+
+Mandatory timing properties for child nodes:
+
+Read parameters:
+
+ - devbus,turn-off-ps:  Defines the time during which the controller does not
+                        drive the AD bus after the completion of a device read.
+                        This prevents contentions on the Device Bus after a read
+                        cycle from a slow device.
+
+ - devbus,bus-width:    Defines the bus width (e.g. <16>)
+
+ - devbus,badr-skew-ps: Defines the time delay from from A[2:0] toggle,
+                        to read data sample. This parameter is useful for
+                        synchronous pipelined devices, where the address
+                        precedes the read data by one or two cycles.
+
+ - devbus,acc-first-ps: Defines the time delay from the negation of
+                        ALE[0] to the cycle that the first read data is sampled
+                        by the controller.
+
+ - devbus,acc-next-ps:  Defines the time delay between the cycle that
+                        samples data N and the cycle that samples data N+1
+                        (in burst accesses).
+
+ - devbus,rd-setup-ps:  Defines the time delay between DEV_CSn assertion to
+			DEV_OEn assertion. If set to 0 (default),
+                        DEV_OEn and DEV_CSn are asserted at the same cycle.
+                        This parameter has no affect on <acc-first-ps> parameter
+                        (no affect on first data sample). Set <rd-setup-ps>
+                        to a value smaller than <acc-first-ps>.
+
+ - devbus,rd-hold-ps:   Defines the time between the last data sample to the
+			de-assertion of DEV_CSn. If set to 0 (default),
+			DEV_OEn and DEV_CSn are de-asserted at the same cycle
+			(the cycle of the last data sample).
+                        This parameter has no affect on DEV_OEn de-assertion.
+                        DEV_OEn is always de-asserted the next cycle after
+                        last data sampled. Also this parameter has no
+                        affect on <turn-off-ps> parameter.
+                        Set <rd-hold-ps> to a value smaller than <turn-off-ps>.
+
+Write parameters:
+
+ - devbus,ale-wr-ps:    Defines the time delay from the ALE[0] negation cycle
+			to the DEV_WEn assertion.
+
+ - devbus,wr-low-ps:    Defines the time during which DEV_WEn is active.
+                        A[2:0] and Data are kept valid as long as DEV_WEn
+                        is active. This parameter defines the setup time of
+                        address and data to DEV_WEn rise.
+
+ - devbus,wr-high-ps:   Defines the time during which DEV_WEn is kept
+                        inactive (high) between data beats of a burst write.
+                        DEV_A[2:0] and Data are kept valid (do not toggle) for
+                        <wr-high-ps> - <tick> ps.
+			This parameter defines the hold time of address and
+			data after DEV_WEn rise.
+
+ - devbus,sync-enable: Synchronous device enable.
+                       1: True
+                       0: False
+
+An example for an Armada XP GP board, with a 16 MiB NOR device as child
+is showed below. Note that the Device Bus driver is in charge of allocating
+the mbus address decoding window for each of its child devices.
+The window is created using the chip select specified in the child
+device node together with the base address and size specified in the ranges
+property. For instance, in the example below the allocated decoding window
+will start at base address 0xf0000000, with a size 0x1000000 (16 MiB)
+for chip select 0 (a.k.a DEV_BOOTCS).
+
+This address window handling is done in this mvebu-devbus only as a temporary
+solution. It will be removed when the support for mbus device tree binding is
+added.
+
+The reg property implicitly specifies the chip select as this:
+
+  0x10400: DEV_BOOTCS
+  0x10408: DEV_CS0
+  0x10410: DEV_CS1
+  0x10418: DEV_CS2
+  0x10420: DEV_CS3
+
+Example:
+
+	devbus-bootcs at d0010400 {
+		status = "okay";
+		ranges = <0 0 0xf0000000 0x1000000>; /* CS0 @addr 0xf0000000, size 0x1000000 */
+		#address-cells = <2>;
+		#size-cells = <1>;
+
+		/* Device Bus parameters are required */
+
+		/* Read parameters */
+		devbus,bus-width    = <8>;
+		devbus,turn-off-ps  = <60000>;
+		devbus,badr-skew-ps = <0>;
+		devbus,acc-first-ps = <124000>;
+		devbus,acc-next-ps  = <248000>;
+		devbus,rd-setup-ps  = <0>;
+		devbus,rd-hold-ps   = <0>;
+
+		/* Write parameters */
+		devbus,sync-enable = <0>;
+		devbus,wr-high-ps  = <60000>;
+		devbus,wr-low-ps   = <60000>;
+		devbus,ale-wr-ps   = <60000>;
+
+		flash at 0 {
+			compatible = "cfi-flash";
+
+			/* CS0, 16 MiB */
+			reg = <0 0 0x1000000>;
+			bank-width = <2>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			/*
+			 * We split the 16 MiB in two partitions,
+			 * just as an example.
+			 */
+			partition at 0 {
+				label = "First";
+				reg = <0 0x800000>;
+			};
+
+			partition at 800000 {
+				label = "Second";
+				reg = <0x800000 0x800000>;
+			};
+		};
+	};
diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
index 067f311..9a81ab3 100644
--- a/drivers/memory/Kconfig
+++ b/drivers/memory/Kconfig
@@ -20,6 +20,16 @@ config TI_EMIF
 	  parameters and other settings during frequency, voltage and
 	  temperature changes
 
+config MVEBU_DEVBUS
+	tristate "Marvell EBU Device Bus Controller"
+	default y
+	depends on PLAT_ORION && OF
+	help
+	  This driver is for the Device Bus controller available in some
+	  Marvell EBU SoCs such as Discovery (mv78xx0), Orion (88f5xxx) and
+	  Armada 370 and Armada XP. This controller allows to handle flash
+	  devices such as NOR, NAND, SRAM, and FPGA.
+
 config TEGRA20_MC
 	bool "Tegra20 Memory Controller(MC) driver"
 	default y
diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
index 9cce5d7..969d923 100644
--- a/drivers/memory/Makefile
+++ b/drivers/memory/Makefile
@@ -6,5 +6,6 @@ ifeq ($(CONFIG_DDR),y)
 obj-$(CONFIG_OF)		+= of_memory.o
 endif
 obj-$(CONFIG_TI_EMIF)		+= emif.o
+obj-$(CONFIG_MVEBU_DEVBUS)	+= mvebu-devbus.o
 obj-$(CONFIG_TEGRA20_MC)	+= tegra20-mc.o
 obj-$(CONFIG_TEGRA30_MC)	+= tegra30-mc.o
diff --git a/drivers/memory/mvebu-devbus.c b/drivers/memory/mvebu-devbus.c
new file mode 100644
index 0000000..2f8ce3a
--- /dev/null
+++ b/drivers/memory/mvebu-devbus.c
@@ -0,0 +1,344 @@
+/*
+ * Marvell EBU SoC Device Bus Controller
+ * (memory controller for NOR/NAND/SRAM/FPGA devices)
+ *
+ * Copyright (C) 2013 Marvell
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/clk.h>
+#include <linux/mbus.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+
+/* Register definitions */
+#define DEV_WIDTH_BIT		30
+#define BADR_SKEW_BIT		28
+#define RD_HOLD_BIT		23
+#define ACC_NEXT_BIT		17
+#define RD_SETUP_BIT		12
+#define ACC_FIRST_BIT		6
+
+#define SYNC_ENABLE_BIT		24
+#define WR_HIGH_BIT		16
+#define WR_LOW_BIT		8
+
+#define READ_PARAM_OFFSET	0x0
+#define WRITE_PARAM_OFFSET	0x4
+
+static const char * const devbus_wins[] = {
+	"devbus-boot",
+	"devbus-cs0",
+	"devbus-cs1",
+	"devbus-cs2",
+	"devbus-cs3",
+};
+
+struct devbus_read_params {
+	u32 bus_width;
+	u32 badr_skew;
+	u32 turn_off;
+	u32 acc_first;
+	u32 acc_next;
+	u32 rd_setup;
+	u32 rd_hold;
+};
+
+struct devbus_write_params {
+	u32 sync_enable;
+	u32 wr_high;
+	u32 wr_low;
+	u32 ale_wr;
+};
+
+struct devbus {
+	struct device *dev;
+	void __iomem *base;
+	unsigned long tick_ps;
+
+	/*
+	 * Remove this once the allocation window handling
+	 * is moved out of here, to be described in the DT.
+	 */
+	struct resource child_mem;
+	struct platform_device *child;
+};
+
+static int get_timing_param_ps(struct devbus *devbus,
+			       struct device_node *node,
+			       const char *name,
+			       u32 *ticks)
+{
+	u32 time_ps;
+	int err;
+
+	err = of_property_read_u32(node, name, &time_ps);
+	if (err < 0) {
+		dev_err(devbus->dev, "%s has no '%s' property\n",
+			name, node->full_name);
+		return err;
+	}
+
+	*ticks = (time_ps + devbus->tick_ps - 1) / devbus->tick_ps;
+
+	dev_dbg(devbus->dev, "%s: %u ps -> 0x%x\n",
+		name, time_ps, *ticks);
+	return 0;
+}
+
+static int devbus_set_timing_params(struct devbus *devbus,
+				    struct device_node *node)
+{
+	struct devbus_read_params r;
+	struct devbus_write_params w;
+	u32 value;
+	int err;
+
+	dev_dbg(devbus->dev, "Setting timing parameter, tick is %lu ps\n",
+		devbus->tick_ps);
+
+	/* Get read timings */
+	err = of_property_read_u32(node, "devbus,bus-width", &r.bus_width);
+	if (err < 0) {
+		dev_err(devbus->dev,
+			"%s has no 'devbus,bus-width' property\n",
+			node->full_name);
+		return err;
+	}
+	/* Convert bit width to byte width */
+	r.bus_width /= 8;
+
+	err = get_timing_param_ps(devbus, node, "devbus,badr-skew-ps",
+				 &r.badr_skew);
+	if (err < 0)
+		return err;
+
+	err = get_timing_param_ps(devbus, node, "devbus,turn-off-ps",
+				 &r.turn_off);
+	if (err < 0)
+		return err;
+
+	err = get_timing_param_ps(devbus, node, "devbus,acc-first-ps",
+				 &r.acc_first);
+	if (err < 0)
+		return err;
+
+	err = get_timing_param_ps(devbus, node, "devbus,acc-next-ps",
+				 &r.acc_next);
+	if (err < 0)
+		return err;
+
+	err = get_timing_param_ps(devbus, node, "devbus,rd-setup-ps",
+				 &r.rd_setup);
+	if (err < 0)
+		return err;
+
+	err = get_timing_param_ps(devbus, node, "devbus,rd-hold-ps",
+				 &r.rd_hold);
+	if (err < 0)
+		return err;
+
+	/* Get write timings */
+	err = of_property_read_u32(node, "devbus,sync-enable",
+				  &w.sync_enable);
+	if (err < 0) {
+		dev_err(devbus->dev,
+			"%s has no 'devbus,sync-enable' property\n",
+			node->full_name);
+		return err;
+	}
+
+	err = get_timing_param_ps(devbus, node, "devbus,ale-wr-ps",
+				 &w.ale_wr);
+	if (err < 0)
+		return err;
+
+	err = get_timing_param_ps(devbus, node, "devbus,wr-low-ps",
+				 &w.wr_low);
+	if (err < 0)
+		return err;
+
+	err = get_timing_param_ps(devbus, node, "devbus,wr-high-ps",
+				 &w.wr_high);
+	if (err < 0)
+		return err;
+
+	/* Set read timings */
+	value = r.bus_width << DEV_WIDTH_BIT |
+		r.badr_skew << BADR_SKEW_BIT |
+		r.rd_hold   << RD_HOLD_BIT   |
+		r.acc_next  << ACC_NEXT_BIT  |
+		r.rd_setup  << RD_SETUP_BIT  |
+		r.acc_first << ACC_FIRST_BIT |
+		r.turn_off;
+
+	dev_dbg(devbus->dev, "read parameters register 0x%p = 0x%x\n",
+		devbus->base + READ_PARAM_OFFSET,
+		value);
+
+	writel(value, devbus->base + READ_PARAM_OFFSET);
+
+	/* Set write timings */
+	value = w.sync_enable  << SYNC_ENABLE_BIT |
+		w.wr_low       << WR_LOW_BIT      |
+		w.wr_high      << WR_HIGH_BIT     |
+		w.ale_wr;
+
+	dev_dbg(devbus->dev, "write parameters register: 0x%p = 0x%x\n",
+		devbus->base + WRITE_PARAM_OFFSET,
+		value);
+
+	writel(value, devbus->base + WRITE_PARAM_OFFSET);
+
+	return 0;
+}
+
+static int mvebu_devbus_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *node = pdev->dev.of_node;
+	struct device_node *child;
+	struct devbus *devbus;
+	struct resource *res;
+	struct clk *clk;
+	unsigned long rate;
+	int err, cs;
+
+	devbus = devm_kzalloc(&pdev->dev, sizeof(struct devbus), GFP_KERNEL);
+	if (!devbus)
+		return -ENOMEM;
+
+	devbus->dev = dev;
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	devbus->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(devbus->base))
+		return PTR_ERR(devbus->base);
+
+	clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+	clk_prepare_enable(clk);
+
+	/*
+	 * Obtain clock period in picoseconds,
+	 * we need this in order to convert timing
+	 * parameters from cycles to picoseconds.
+	 */
+	rate = clk_get_rate(clk) / 1000;
+	devbus->tick_ps = 1000000000 / rate;
+
+	/* We only need the first child */
+	if (of_get_child_count(node) > 1) {
+		dev_err(dev, "%s has more than one child\n", node->full_name);
+		return -EINVAL;
+	}
+
+	child = of_get_next_child(node, NULL);
+	if (!child) {
+		dev_err(dev, "%s has no childs\n", node->full_name);
+		return -EINVAL;
+	}
+
+	/* Read the device tree node and set the new timing parameters */
+	err = devbus_set_timing_params(devbus, node);
+	if (err < 0)
+		return err;
+
+	/*
+	 * Allocate an address window for this device.
+	 * If the device probing fails, then we won't be able to
+	 * remove the allocated address decoding window.
+	 *
+	 * FIXME: This is only a temporary hack! We need to do this here
+	 * because we still don't have device tree bindings for mbus.
+	 * Once that support is added, we will declare these address windows
+	 * statically in the device tree, and remove the window configuration
+	 * from here.
+	 */
+
+	/* Get the CS to choose the window string */
+	err = of_property_read_u32(node, "ranges", &cs);
+	if (err < 0)
+		return err;
+
+	/* Translate the child's memory region to create the window */
+	if (of_address_to_resource(child, 0, &devbus->child_mem)) {
+		dev_err(dev, "%s has malformed 'reg' property\n",
+			child->full_name);
+		return -EINVAL;
+	}
+
+	err = mvebu_mbus_add_window(devbus_wins[cs], devbus->child_mem.start,
+				    resource_size(&devbus->child_mem));
+	if (err < 0)
+		return err;
+
+	devbus->child = of_platform_device_create(child, NULL, &pdev->dev);
+	if (!devbus->child) {
+		dev_warn(dev, "cannot create child device %s\n", child->name);
+		/* Remove the allocated window */
+		mvebu_mbus_del_window(devbus->child_mem.start,
+				      resource_size(&devbus->child_mem));
+	}
+
+	platform_set_drvdata(pdev, devbus);
+
+	return 0;
+}
+
+static int mvebu_devbus_remove(struct platform_device *pdev)
+{
+	struct devbus *devbus = platform_get_drvdata(pdev);
+
+	/* Release device */
+	of_device_unregister(devbus->child);
+
+	/* Release window */
+	mvebu_mbus_del_window(devbus->child_mem.start,
+			      resource_size(&devbus->child_mem));
+
+	devbus->child = NULL;
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+static const struct of_device_id mvebu_devbus_of_match[] = {
+	{ .compatible = "marvell,mvebu-devbus" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, mvebu_devbus_of_match);
+
+static struct platform_driver mvebu_devbus_driver = {
+	.probe		= mvebu_devbus_probe,
+	.remove		= mvebu_devbus_remove,
+	.driver		= {
+		.name	= "mvebu-devbus",
+		.owner	= THIS_MODULE,
+		.of_match_table = mvebu_devbus_of_match,
+	},
+};
+
+module_platform_driver(mvebu_devbus_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Ezequiel Garcia <ezequiel.garcia@free-electrons.com>");
+MODULE_DESCRIPTION("Marvell EBU SoC Device Bus controller");
-- 
1.8.1.5

[PATCH v4 2/5] ARM: mvebu: Add Device Bus support for Armada 370/XP SoC

[Ezequiel Garcia]

Armada 370 and Armada XP SoC have a Device Bus controller to
handle NOR, NAND, SRAM and FPGA devices.

This patch adds the device tree nodes to enable the controller,
where each node in the device tree, represents a chip select.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/boot/dts/armada-370-xp.dtsi | 45 ++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 7704829..fe90b64 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -176,6 +176,51 @@
 			clocks = <&coreclk 0>;
 			status = "disabled";
 		};
+
+		devbus-bootcs at d0010400 {
+			compatible = "marvell,mvebu-devbus";
+			reg = <0xd0010400 0x8>;
+			#address-cells = <2>;
+			#size-cells = <1>;
+			clocks = <&coreclk 0>;
+			status = "disabled";
+		};
+
+		devbus-cs0 at d0010408 {
+			compatible = "marvell,mvebu-devbus";
+			reg = <0xd0010408 0x8>;
+			#address-cells = <2>;
+			#size-cells = <1>;
+			clocks = <&coreclk 0>;
+			status = "disabled";
+		};
+
+		devbus-cs1 at d0010410 {
+			compatible = "marvell,mvebu-devbus";
+			reg = <0xd0010410 0x8>;
+			#address-cells = <2>;
+			#size-cells = <1>;
+			clocks = <&coreclk 0>;
+			status = "disabled";
+		};
+
+		devbus-cs2 at d0010418 {
+			compatible = "marvell,mvebu-devbus";
+			reg = <0xd0010418 0x8>;
+			#address-cells = <2>;
+			#size-cells = <1>;
+			clocks = <&coreclk 0>;
+			status = "disabled";
+		};
+
+		devbus-cs3 at d0010420 {
+			compatible = "marvell,mvebu-devbus";
+			reg = <0xd0010420 0x8>;
+			#address-cells = <2>;
+			#size-cells = <1>;
+			clocks = <&coreclk 0>;
+			status = "disabled";
+		};
 	};
 };
 
-- 
1.8.1.5

[PATCH v4 3/5] ARM: mvebu: Add support for NOR flash device on Armada XP-GP board

[Ezequiel Garcia]

The Armada XP Development Board DB-MV784MP-GP has a NOR flash device
connected to the Device Bus. This commit adds the device tree node
to support this device.

This SoC supports a flexible and dynamic decoding window allocation
scheme; but since this feature is still not implemented we need
to specify the window base address in the device tree node itself.

This base address has been selected in a completely arbitrary fashion.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/boot/dts/armada-xp-gp.dts | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/arch/arm/boot/dts/armada-xp-gp.dts b/arch/arm/boot/dts/armada-xp-gp.dts
index 1c8afe2..39f6a4d 100644
--- a/arch/arm/boot/dts/armada-xp-gp.dts
+++ b/arch/arm/boot/dts/armada-xp-gp.dts
@@ -109,5 +109,34 @@
 				spi-max-frequency = <108000000>;
 			};
 		};
+
+		devbus-bootcs at d0010400 {
+			status = "okay";
+			ranges = <0 0 0xf0000000 0x1000000>; /* CS0 @addr 0xf000000, size 0x1000000 */
+
+			/* Device Bus parameters are required */
+
+			/* Read parameters */
+			devbus,bus-width    = <8>;
+			devbus,turn-off-ps  = <60000>;
+			devbus,badr-skew-ps = <0>;
+			devbus,acc-first-ps = <124000>;
+			devbus,acc-next-ps  = <248000>;
+			devbus,rd-setup-ps  = <0>;
+			devbus,rd-hold-ps   = <0>;
+
+			/* Write parameters */
+			devbus,sync-enable = <0>;
+			devbus,wr-high-ps  = <60000>;
+			devbus,wr-low-ps   = <60000>;
+			devbus,ale-wr-ps   = <60000>;
+
+			/* NOR 16 MiB */
+			nor at 0 {
+				compatible = "cfi-flash";
+				reg = <0 0 0x1000000>;
+				bank-width = <2>;
+			};
+		};
 	};
 };
-- 
1.8.1.5

[PATCH v4 4/5] ARM: mvebu: Add support for NOR flash device on Openblocks AX3 board

[Ezequiel Garcia]

The Plat'home Openblocks AX3 has a 128 MiB NOR flash device connected
to the Device Bus. This commit adds the device tree node to support this device.

The SoC supports a flexible and dynamic decoding window allocation scheme;
but since this feature is still not implemented we need to specify the window
base address in the device tree node itself.
This base address has been selected in a completely arbitrary fashion.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts | 29 ++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts b/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
index 3818a82..04576c2 100644
--- a/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
+++ b/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
@@ -139,5 +139,34 @@
 		usb at d0051000 {
 			status = "okay";
 		};
+
+		devbus-bootcs at d0010400 {
+			status = "okay";
+			ranges = <0 0 0xf0000000 0x8000000>; /* CS0 @addr 0xf000000, size 0x8000000 */
+
+			/* Device Bus parameters are required */
+
+			/* Read parameters */
+			devbus,bus-width    = <8>;
+			devbus,turn-off-ps  = <60000>;
+			devbus,badr-skew-ps = <0>;
+			devbus,acc-first-ps = <124000>;
+			devbus,acc-next-ps  = <248000>;
+			devbus,rd-setup-ps  = <0>;
+			devbus,rd-hold-ps   = <0>;
+
+			/* Write parameters */
+			devbus,sync-enable = <0>;
+			devbus,wr-high-ps  = <60000>;
+			devbus,wr-low-ps   = <60000>;
+			devbus,ale-wr-ps   = <60000>;
+
+			/* NOR 128 MiB */
+			nor at 0 {
+				compatible = "cfi-flash";
+				reg = <0 0 0x8000000>;
+				bank-width = <2>;
+			};
+		};
 	};
 };
-- 
1.8.1.5

[PATCH v4 5/5] ARM: mvebu: Add Device Bus and CFI flash memory support to defconfig

[Ezequiel Garcia]

This patch selects the devbus driver as part of the mvebu default
config, along with the required options to detect and support
CFI flash memories.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/configs/mvebu_defconfig | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/configs/mvebu_defconfig b/arch/arm/configs/mvebu_defconfig
index 2ec8119..fe8ba26 100644
--- a/arch/arm/configs/mvebu_defconfig
+++ b/arch/arm/configs/mvebu_defconfig
@@ -46,6 +46,11 @@ CONFIG_I2C_MV64XXX=y
 CONFIG_MTD=y
 CONFIG_MTD_CHAR=y
 CONFIG_MTD_M25P80=y
+CONFIG_MTD_CFI=y
+CONFIG_MTD_CFI_INTELEXT=y
+CONFIG_MTD_CFI_AMDSTD=y
+CONFIG_MTD_CFI_STAA=y
+CONFIG_MTD_PHYSMAP_OF=y
 CONFIG_SERIAL_8250_DW=y
 CONFIG_GPIOLIB=y
 CONFIG_GPIO_SYSFS=y
@@ -65,6 +70,8 @@ CONFIG_RTC_DRV_S35390A=y
 CONFIG_RTC_DRV_MV=y
 CONFIG_DMADEVICES=y
 CONFIG_MV_XOR=y
+CONFIG_MEMORY=y
+CONFIG_MVEBU_DEVBUS=y
 # CONFIG_IOMMU_SUPPORT is not set
 CONFIG_EXT2_FS=y
 CONFIG_EXT3_FS=y
-- 
1.8.1.5

[PATCH v4 0/5] Device Bus support for Marvell EBU SoC

[Arnd Bergmann]

On Monday 08 April 2013, Ezequiel Garcia wrote:
>   * Rename devbus,dev-width to devbus,bus-width and changed the
>     parameter value from bytes to bits: <8>, <16>, ...
>     Suggested by Arnd Bergmann.
> 
>   * As suggested by Jason Gunthorpe and agreed by Arnd Bergmann.
>     Reworked the whole devbus child chip-select device tree design.
>     Now there's a separate devbus at addr DT node for each controller
>     chip select. The impact of this is:
>       1. Timing parameters now go into the parent node.
>       2. There can only be one child per devbus node.
> 
>   * Remove unneeded bank-width parameter read.
> 

Whole series:

Acked-by: Arnd Bergmann <arnd@arndb.de>

It may be nicer to use #address-cells=<2> for devbus device nodes now that
each of them has only a 32 bit address space, but the current code is correct
as well using a 64 bit notation.

	Arnd

[PATCH v4 0/5] Device Bus support for Marvell EBU SoC

[Ezequiel Garcia]

On Mon, Apr 08, 2013 at 04:17:31PM +0200, Arnd Bergmann wrote:
> On Monday 08 April 2013, Ezequiel Garcia wrote:
> >   * Rename devbus,dev-width to devbus,bus-width and changed the
> >     parameter value from bytes to bits: <8>, <16>, ...
> >     Suggested by Arnd Bergmann.
> > 
> >   * As suggested by Jason Gunthorpe and agreed by Arnd Bergmann.
> >     Reworked the whole devbus child chip-select device tree design.
> >     Now there's a separate devbus at addr DT node for each controller
> >     chip select. The impact of this is:
> >       1. Timing parameters now go into the parent node.
> >       2. There can only be one child per devbus node.
> > 
> >   * Remove unneeded bank-width parameter read.
> > 
> 
> Whole series:
> 
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> 
> It may be nicer to use #address-cells=<2> for devbus device nodes now that
> each of them has only a 32 bit address space, but the current code is correct
> as well using a 64 bit notation.
> 

I'll probably be sending a v5 fixing this later.

@Jason Cooper:
How would you like to handle this, regarding the LPAE series?
Do you want me to rebase this devbus on top of that?

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v4 1/5] drivers: memory: Introduce Marvell EBU Device Bus driver

[Jason Gunthorpe]

Looks OK to me, though with mbus dt bindings we'd see some more
changes.

Just to note on them for future:

> +	child = of_get_next_child(node, NULL);
> +	if (!child) {
> +		dev_err(dev, "%s has no childs\n", node->full_name);
> +		return -EINVAL;
> +	}

This isn't really necessary anymore, the ranges of the node itself
should be parsed to get the window start/end. The driver should never
need to look at the children directly..

> +	/* Get the CS to choose the window string */
> +	err = of_property_read_u32(node, "ranges", &cs);
> +	if (err < 0)
> +		return err;

.. and this is why you've kept the 2 cells address - the top cell is
still encoding the target id. This would go away eventually too..

Maybe it would be better in the interm to compute the CS offset from
the control register offset?

(reg.start % 0x400)/8 should do the trick?

> +	devbus->child = of_platform_device_create(child, NULL, &pdev->dev);
> +	if (!devbus->child) {
> +		dev_warn(dev, "cannot create child device %s\n", child->name);
> +		/* Remove the allocated window */
> +		mvebu_mbus_del_window(devbus->child_mem.start,
> +				      resource_size(&devbus->child_mem));
> +	}

This can probably just be of_platform_populate or similar to do all
children? For instance, I often use many DT nodes to represent a FPGA,
since the my FPGA's tend to have many functionally orthogonal units
inside.

Regards,
Jason

[PATCH v4 0/5] Device Bus support for Marvell EBU SoC

[Jason Cooper]

On Mon, Apr 08, 2013 at 12:29:34PM -0300, Ezequiel Garcia wrote:
> On Mon, Apr 08, 2013 at 04:17:31PM +0200, Arnd Bergmann wrote:
> > On Monday 08 April 2013, Ezequiel Garcia wrote:
> > >   * Rename devbus,dev-width to devbus,bus-width and changed the
> > >     parameter value from bytes to bits: <8>, <16>, ...
> > >     Suggested by Arnd Bergmann.
> > > 
> > >   * As suggested by Jason Gunthorpe and agreed by Arnd Bergmann.
> > >     Reworked the whole devbus child chip-select device tree design.
> > >     Now there's a separate devbus at addr DT node for each controller
> > >     chip select. The impact of this is:
> > >       1. Timing parameters now go into the parent node.
> > >       2. There can only be one child per devbus node.
> > > 
> > >   * Remove unneeded bank-width parameter read.
> > > 
> > 
> > Whole series:
> > 
> > Acked-by: Arnd Bergmann <arnd@arndb.de>
> > 
> > It may be nicer to use #address-cells=<2> for devbus device nodes now that
> > each of them has only a 32 bit address space, but the current code is correct
> > as well using a 64 bit notation.
> > 
> 
> I'll probably be sending a v5 fixing this later.
> 
> @Jason Cooper:
> How would you like to handle this, regarding the LPAE series?
> Do you want me to rebase this devbus on top of that?

Let's avoid the dependency and merge them both as is.  Once v3.10 drops,
you can send a fixup patch to move it to 64bit.

thx,

Jason.

[PATCH v4 0/5] Device Bus support for Marvell EBU SoC

[Jason Cooper]

On Mon, Apr 08, 2013 at 02:30:55PM -0400, Jason Cooper wrote:
> On Mon, Apr 08, 2013 at 12:29:34PM -0300, Ezequiel Garcia wrote:
> > On Mon, Apr 08, 2013 at 04:17:31PM +0200, Arnd Bergmann wrote:
> > > On Monday 08 April 2013, Ezequiel Garcia wrote:
> > > >   * Rename devbus,dev-width to devbus,bus-width and changed the
> > > >     parameter value from bytes to bits: <8>, <16>, ...
> > > >     Suggested by Arnd Bergmann.
> > > > 
> > > >   * As suggested by Jason Gunthorpe and agreed by Arnd Bergmann.
> > > >     Reworked the whole devbus child chip-select device tree design.
> > > >     Now there's a separate devbus at addr DT node for each controller
> > > >     chip select. The impact of this is:
> > > >       1. Timing parameters now go into the parent node.
> > > >       2. There can only be one child per devbus node.
> > > > 
> > > >   * Remove unneeded bank-width parameter read.
> > > > 
> > > 
> > > Whole series:
> > > 
> > > Acked-by: Arnd Bergmann <arnd@arndb.de>
> > > 
> > > It may be nicer to use #address-cells=<2> for devbus device nodes now that
> > > each of them has only a 32 bit address space, but the current code is correct
> > > as well using a 64 bit notation.
> > > 
> > 
> > I'll probably be sending a v5 fixing this later.
> > 
> > @Jason Cooper:
> > How would you like to handle this, regarding the LPAE series?
> > Do you want me to rebase this devbus on top of that?
> 
> Let's avoid the dependency and merge them both as is.  Once v3.10 drops,
> you can send a fixup patch to move it to 64bit.

After talking with gregory, I'll merge everything as-is, then have him
rebase the LPAE changes on top of mvebu/dt.

thx,

Jason.

[PATCH v4 1/5] drivers: memory: Introduce Marvell EBU Device Bus driver

[Arnd Bergmann]

On Monday 08 April 2013, Jason Gunthorpe wrote:
> > +     /* Get the CS to choose the window string */
> > +     err = of_property_read_u32(node, "ranges", &cs);
> > +     if (err < 0)
> > +             return err;
> 
> .. and this is why you've kept the 2 cells address - the top cell is
> still encoding the target id. This would go away eventually too..
> 
> Maybe it would be better in the interm to compute the CS offset from
> the control register offset?
> 
> (reg.start % 0x400)/8 should do the trick?

Yes, we came to the axact same conclusion on IRC.

> > +     devbus->child = of_platform_device_create(child, NULL, &pdev->dev);
> > +     if (!devbus->child) {
> > +             dev_warn(dev, "cannot create child device %s\n", child->name);
> > +             /* Remove the allocated window */
> > +             mvebu_mbus_del_window(devbus->child_mem.start,
> > +                                   resource_size(&devbus->child_mem));
> > +     }
> 
> This can probably just be of_platform_populate or similar to do all
> children? For instance, I often use many DT nodes to represent a FPGA,
> since the my FPGA's tend to have many functionally orthogonal units
> inside.

Good idea, yes.

	Arnd

[PATCH v4 1/5] drivers: memory: Introduce Marvell EBU Device Bus driver

[Ezequiel Garcia]

Hi Jason

On Mon, Apr 08, 2013 at 11:19:40AM -0600, Jason Gunthorpe wrote:
> Looks OK to me, though with mbus dt bindings we'd see some more
> changes.
> 

Thanks for the review!

> Just to note on them for future:
> 
> > +	child = of_get_next_child(node, NULL);
> > +	if (!child) {
> > +		dev_err(dev, "%s has no childs\n", node->full_name);
> > +		return -EINVAL;
> > +	}
> 
> This isn't really necessary anymore, the ranges of the node itself
> should be parsed to get the window start/end. The driver should never
> need to look at the children directly..
> 

You mean the above will not be necessary in the *future*,
once we add mbus DT bindings, right?

AFAICT, it's needed for the current workaround to work.
FWIW, the whole code below the *FIXME* is part of the temporary
to-be-removed thing.

Maybe I'll add a special comment on each chunk that's meant
to be removed to avoid confusions.

> > +	/* Get the CS to choose the window string */
> > +	err = of_property_read_u32(node, "ranges", &cs);
> > +	if (err < 0)
> > +		return err;
> 
> .. and this is why you've kept the 2 cells address - the top cell is
> still encoding the target id. This would go away eventually too..
> 
> Maybe it would be better in the interm to compute the CS offset from
> the control register offset?
> 
> (reg.start % 0x400)/8 should do the trick?
> 

Yes, I'll do this and drop the 2-cell address.

> > +	devbus->child = of_platform_device_create(child, NULL, &pdev->dev);
> > +	if (!devbus->child) {
> > +		dev_warn(dev, "cannot create child device %s\n", child->name);
> > +		/* Remove the allocated window */
> > +		mvebu_mbus_del_window(devbus->child_mem.start,
> > +				      resource_size(&devbus->child_mem));
> > +	}
> 
> This can probably just be of_platform_populate or similar to do all
> children? For instance, I often use many DT nodes to represent a FPGA,
> since the my FPGA's tend to have many functionally orthogonal units
> inside.
> 

In the past I've found it's not possible to use of_platform_populate,
altough I can be wrong.

The problem seems to be that through of_platform_populate() it's
possible that the child device (cfi-flash, for instance) gets probed
before the devbus controller. In other words, the actual parent-child
relationship gets lost and it's necesarry to use some child probe deferal
mechanism.

This has been recently discussed [1] when implementing GPMC where for simplicity
the of_platform_device_create() solution was chosen.
Anyway, I'll try of_platform_populate early tomorrow.

Thanks,

[1]  http://www.mail-archive.com/linux-omap at vger.kernel.org/msg85897.html
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v4 1/5] drivers: memory: Introduce Marvell EBU Device Bus driver

[Arnd Bergmann]

On Tuesday 09 April 2013, Ezequiel Garcia wrote:
> > This can probably just be of_platform_populate or similar to do all
> > children? For instance, I often use many DT nodes to represent a FPGA,
> > since the my FPGA's tend to have many functionally orthogonal units
> > inside.
> > 
> 
> In the past I've found it's not possible to use of_platform_populate,
> altough I can be wrong.
> 
> The problem seems to be that through of_platform_populate() it's
> possible that the child device (cfi-flash, for instance) gets probed
> before the devbus controller. In other words, the actual parent-child
> relationship gets lost and it's necesarry to use some child probe deferal
> mechanism.
> 
> This has been recently discussed [1] when implementing GPMC where for simplicity
> the of_platform_device_create() solution was chosen.
> Anyway, I'll try of_platform_populate early tomorrow.

I think the problem mentioned in that thread was that the device would
automatically get probed by the top-level of_platform_populate() when
you mark the devbus as compatible="simple-bus", which is not what Jason
was referring to. Instead you should call of_platform_populate in the
probe() function of the devbus device to probe its children after
the bus is set up.

	Arnd

[PATCH v4 1/5] drivers: memory: Introduce Marvell EBU Device Bus driver

[Ezequiel Garcia]

Hi Arnd,

On Tue, Apr 09, 2013 at 11:40:51AM +0200, Arnd Bergmann wrote:
> On Tuesday 09 April 2013, Ezequiel Garcia wrote:
> > > This can probably just be of_platform_populate or similar to do all
> > > children? For instance, I often use many DT nodes to represent a FPGA,
> > > since the my FPGA's tend to have many functionally orthogonal units
> > > inside.
> > > 
> > 
> > In the past I've found it's not possible to use of_platform_populate,
> > altough I can be wrong.
> > 
> > The problem seems to be that through of_platform_populate() it's
> > possible that the child device (cfi-flash, for instance) gets probed
> > before the devbus controller. In other words, the actual parent-child
> > relationship gets lost and it's necesarry to use some child probe deferal
> > mechanism.
> > 
> > This has been recently discussed [1] when implementing GPMC where for simplicity
> > the of_platform_device_create() solution was chosen.
> > Anyway, I'll try of_platform_populate early tomorrow.
> 
> I think the problem mentioned in that thread was that the device would
> automatically get probed by the top-level of_platform_populate() when
> you mark the devbus as compatible="simple-bus", which is not what Jason
> was referring to. Instead you should call of_platform_populate in the
> probe() function of the devbus device to probe its children after
> the bus is set up.
> 

Ah! yes, you're right...

Well, in that case the only issue I can foresee is that if we decide
to use of_platform_populate we won't be able to unregister child
devices from the remove() callback.

Indeed, the benefits of using of_platform_populate are interesting,
but I don't know how much of an issue this represents.

If we can't unregister child devices, we can't remove address windows.
Now, this is not a big deal, since we plan to define them statically in
the DT anyway.
In that case, it seems we shouldn't allow this driver to be a module, uh?

(actually we currently can't have mvebu-devbus as a module, because
mbus API is not exported, but we can fix that if we want).

What do you think?
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v4 1/5] drivers: memory: Introduce Marvell EBU Device Bus driver

[Arnd Bergmann]

On Tuesday 09 April 2013, Ezequiel Garcia wrote:
> Ah! yes, you're right...
> 
> Well, in that case the only issue I can foresee is that if we decide
> to use of_platform_populate we won't be able to unregister child
> devices from the remove() callback.
> 
> Indeed, the benefits of using of_platform_populate are interesting,
> but I don't know how much of an issue this represents.
> 
> If we can't unregister child devices, we can't remove address windows.
> Now, this is not a big deal, since we plan to define them statically in
> the DT anyway.
> In that case, it seems we shouldn't allow this driver to be a module, uh?
> 
> (actually we currently can't have mvebu-devbus as a module, because
> mbus API is not exported, but we can fix that if we want).

I think it's still reasonable to make it a module, but it might need to
be one without a module_exit() call to prevent unloading.

We could also try to add the opposite of of_platform_populate to remove
an entire subtree.

	Arnd

[PATCH v4 1/5] drivers: memory: Introduce Marvell EBU Device Bus driver

[Ezequiel Garcia]

On Tue, Apr 09, 2013 at 12:44:07PM +0200, Arnd Bergmann wrote:
> On Tuesday 09 April 2013, Ezequiel Garcia wrote:
> > Ah! yes, you're right...
> > 
> > Well, in that case the only issue I can foresee is that if we decide
> > to use of_platform_populate we won't be able to unregister child
> > devices from the remove() callback.
> > 
> > Indeed, the benefits of using of_platform_populate are interesting,
> > but I don't know how much of an issue this represents.
> > 
> > If we can't unregister child devices, we can't remove address windows.
> > Now, this is not a big deal, since we plan to define them statically in
> > the DT anyway.
> > In that case, it seems we shouldn't allow this driver to be a module, uh?
> > 
> > (actually we currently can't have mvebu-devbus as a module, because
> > mbus API is not exported, but we can fix that if we want).
> 
> I think it's still reasonable to make it a module, but it might need to
> be one without a module_exit() call to prevent unloading.
> 
> We could also try to add the opposite of of_platform_populate to remove
> an entire subtree.
> 

Yes, good idea.

I'll send a v5 in a minute, with your acked-by,
maintaining the of_platform_device_create for now.

I think at this point it's reasonable to add of_platform_populate
as a follow-up patch, together with the mbus-related fixes.

Thanks,
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v4 1/5] drivers: memory: Introduce Marvell EBU Device Bus driver

[Jason Gunthorpe]

On Tue, Apr 09, 2013 at 12:44:07PM +0200, Arnd Bergmann wrote:

> I think it's still reasonable to make it a module, but it might need
> to be one without a module_exit() call to prevent unloading.

Agree, I think it is better to properly handle a broader set of
possible DT's than to have module unloading. The DT is supposed to be
stable..

Jason

[PATCH v4 1/5] drivers: memory: Introduce Marvell EBU Device Bus driver

[Jason Gunthorpe]

On Mon, Apr 08, 2013 at 09:42:54PM -0300, Ezequiel Garcia wrote:
> > Just to note on them for future:
> > 
> > > +	child = of_get_next_child(node, NULL);
> > > +	if (!child) {
> > > +		dev_err(dev, "%s has no childs\n", node->full_name);
> > > +		return -EINVAL;
> > > +	}
> > 
> > This isn't really necessary anymore, the ranges of the node itself
> > should be parsed to get the window start/end. The driver should never
> > need to look at the children directly..
> > 
> 
> You mean the above will not be necessary in the *future*,
> once we add mbus DT bindings, right?

No, you could do this now.

Look at your binding:

+               devbus-bootcs at d0010400 {
+                       status = "okay";
+                       ranges = <0 0xf0000000 0x1000000>; /* @addr 0xf000000, size 0x1000000 */

Parse that ranges and you get the base and size of the window.

Then 'ranges' is mandatory and 'ranges' must have a single entry.

No need to look at children at all. You can remove all the child
parsing code, and then the driver is able to handle a wider range of
possible DT uses.

Jason

[PATCH v4 1/5] drivers: memory: Introduce Marvell EBU Device Bus driver

[Ezequiel Garcia]

Hi Jason,

On Tue, Apr 09, 2013 at 10:03:16AM -0600, Jason Gunthorpe wrote:
> 
> No need to look at children at all. You can remove all the child
> parsing code, and then the driver is able to handle a wider range of
> possible DT uses.
> 

Agreed. I just sent a v6 fixing this and using of_platform_populate()
to create every child available.

How does it look?

Thanks for the feedback!
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com