IRQ, GPIO SPI, I2C, etc DTC support

Both IRQ and GPIO controllers can now be represented in DT. The IRQ controllers are setup first, and then the GPIO controllers. Interrupts for GPIO lines are placed directly after the main interrupts in the interrupt space.

Signed-off-by Andrew Lunn <[email protected]>

---
 .../devicetree/bindings/arm/mrvl/intc.txt          |   19 +++
 .../devicetree/bindings/gpio/mrvl-gpio.txt         |   23 +++
 arch/arm/Kconfig                                   |    1 +
 arch/arm/boot/dts/kirkwood.dtsi                    |   27 ++++
 arch/arm/mach-dove/irq.c                           |   58 +++----
 arch/arm/mach-kirkwood/board-dt.c                  |    3 +-
 arch/arm/mach-kirkwood/irq.c                       |   38 ++---
 arch/arm/mach-mv78xx0/irq.c                        |   22 +--
 arch/arm/mach-orion5x/irq.c                        |   22 +--
 arch/arm/plat-orion/gpio.c                         |  166 +++++++++++++++-----
 arch/arm/plat-orion/include/plat/gpio.h            |   16 +-
 arch/arm/plat-orion/include/plat/irq.h             |    3 +-
 arch/arm/plat-orion/irq.c                          |   40 +++++
 13 files changed, 306 insertions(+), 132 deletions(-)

Documentation/devicetree/bindings/arm/mrvl/intc.txt +19 -0

index 80b9a94..8927e10 100644
@@ -38,3 +38,22 @@ Example:
 		reg-names = "mux status", "mux mask";
 		mrvl,intc-nr-irqs = <2>;
 	};
+
+* Marvell Orion Interrupt controller
+
+Required properties
+- compatible :  Should be "marvell,orion-intc".
+- #interrupt-cells: Specifies the number of cells needed to encode an
+  interrupt source. Supported value is <1>.
+- interrupt-controller : Declare this node to be an interrupt controller.
+- reg : Interrupt mask address.
+
+Example:
+
+	intc: interrupt-controller {
+        	compatible = "marvell,orion-intc", "marvell,intc";
+		interrupt-controller;
+		#interrupt-cells = <1>;
+                reg = <0xfed20204 0x04>,
+		      <0xfed20214 0x04>;
+        };

Documentation/devicetree/bindings/gpio/mrvl-gpio.txt +23 -0

index 05428f3..e137874 100644
@@ -27,3 +27,26 @@ Example:
 		interrupt-controller;
 		#interrupt-cells = <1>;
       };
+
+* Marvell Orion GPIO Controller
+
+Required properties:
+- compatible         : Should be "marvell,orion-gpio"
+- reg                : Address and length of the register set for controller.
+- gpio-controller    : So we know this is a gpio controller.
+- ngpio              : How many gpios this controller has.
+- interrupts	     : Up to 4 Interrupts for the controller.
+
+Optional properties:
+- mask-offset        : For SMP Orions, offset for Nth CPU
+
+Example:
+
+		gpio0: gpio at 10100 {
+			compatible = "marvell,orion-gpio";
+			#gpio-cells = <2>;
+			gpio-controller;
+			reg = <0x10100 0x40>;
+			ngpio = <32>;
+			interrupts = <35>, <36>, <37>, <38>;
+		};

arch/arm/Kconfig +1 -0

index a91009c..39bb941 100644
@@ -1105,6 +1105,7 @@ config PLAT_ORION
 	bool
 	select CLKSRC_MMIO
 	select GENERIC_IRQ_CHIP
+	select IRQ_DOMAIN
 	select COMMON_CLK
 
 config PLAT_PXA

arch/arm/boot/dts/kirkwood.dtsi +27 -0

index 926528b..66544d7 100644
@@ -2,6 +2,15 @@
 
 / {
 	compatible = "mrvl,kirkwood";
+	interrupt-parent = <&intc>;
+
+	intc: interrupt-controller {
+		compatible = "marvell,orion-intc", "marvell,intc";
+		interrupt-controller;
+		#interrupt-cells = <1>;
+		reg = <0xf1020204 0x04>,
+		      <0xf1020214 0x04>;
+	};
 
 	ocp at f1000000 {
 		compatible = "simple-bus";

@@ -9,6 +18,24 @@
 		#address-cells = <1>;
 		#size-cells = <1>;
 
+		gpio0: gpio at 10100 {
+			compatible = "marvell,orion-gpio";
+			#gpio-cells = <2>;
+			gpio-controller;
+			reg = <0x10100 0x40>;
+			ngpio = <32>;
+			interrupts = <35>, <36>, <37>, <38>;
+		};
+
+		gpio1: gpio at 10140 {
+			compatible = "marvell,orion-gpio";
+			#gpio-cells = <2>;
+			gpio-controller;
+			reg = <0x10140 0x40>;
+			ngpio = <18>;
+			interrupts = <39>, <40>, <41>;
+		};
+
 		serial at 12000 {
 			compatible = "ns16550a";
 			reg = <0x12000 0x100>;

arch/arm/mach-dove/irq.c +29 -29

index f07fd16..9bc97a5 100644
@@ -20,22 +20,6 @@
 #include <mach/bridge-regs.h>
 #include "common.h"
 
-static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
-{
-	int irqoff;
-	BUG_ON(irq < IRQ_DOVE_GPIO_0_7 || irq > IRQ_DOVE_HIGH_GPIO);
-
-	irqoff = irq <= IRQ_DOVE_GPIO_16_23 ? irq - IRQ_DOVE_GPIO_0_7 :
-		3 + irq - IRQ_DOVE_GPIO_24_31;
-
-	orion_gpio_irq_handler(irqoff << 3);
-	if (irq == IRQ_DOVE_HIGH_GPIO) {
-		orion_gpio_irq_handler(40);
-		orion_gpio_irq_handler(48);
-		orion_gpio_irq_handler(56);
-	}
-}
-
 static void pmu_irq_mask(struct irq_data *d)
 {
 	int pin = irq_to_pmu(d->irq);

@@ -90,6 +74,27 @@ static void pmu_irq_handler(unsigned int irq, struct irq_desc *desc)
 	}
 }
 
+static int __initdata gpio0_irqs[4] = {
+	IRQ_DOVE_GPIO_0_7,
+	IRQ_DOVE_GPIO_8_15,
+	IRQ_DOVE_GPIO_16_23,
+	IRQ_DOVE_GPIO_24_31,
+};
+
+static int __initdata gpio1_irqs[4] = {
+	IRQ_DOVE_HIGH_GPIO,
+	0,
+	0,
+	0,
+};
+
+static int __initdata gpio2_irqs[4] = {
+	0,
+	0,
+	0,
+	0,
+};
+
 void __init dove_init_irq(void)
 {
 	int i;

@@ -100,19 +105,14 @@ void __init dove_init_irq(void)
 	/*
 	 * Initialize gpiolib for GPIOs 0-71.
 	 */
-	orion_gpio_init(0, 32, DOVE_GPIO_LO_VIRT_BASE, 0,
-			IRQ_DOVE_GPIO_START);
-	irq_set_chained_handler(IRQ_DOVE_GPIO_0_7, gpio_irq_handler);
-	irq_set_chained_handler(IRQ_DOVE_GPIO_8_15, gpio_irq_handler);
-	irq_set_chained_handler(IRQ_DOVE_GPIO_16_23, gpio_irq_handler);
-	irq_set_chained_handler(IRQ_DOVE_GPIO_24_31, gpio_irq_handler);
-
-	orion_gpio_init(32, 32, DOVE_GPIO_HI_VIRT_BASE, 0,
-			IRQ_DOVE_GPIO_START + 32);
-	irq_set_chained_handler(IRQ_DOVE_HIGH_GPIO, gpio_irq_handler);
-
-	orion_gpio_init(64, 8, DOVE_GPIO2_VIRT_BASE, 0,
-			IRQ_DOVE_GPIO_START + 64);
+	orion_gpio_init(NULL, 0, 32, (void __iomem *)DOVE_GPIO_LO_VIRT_BASE, 0,
+			IRQ_DOVE_GPIO_START, gpio0_irqs);
+
+	orion_gpio_init(NULL, 32, 32, (void __iomem *)DOVE_GPIO_HI_VIRT_BASE, 0,
+			IRQ_DOVE_GPIO_START + 32, gpio1_irqs);
+
+	orion_gpio_init(NULL, 64, 8, (void __iomem *)DOVE_GPIO2_VIRT_BASE, 0,
+			IRQ_DOVE_GPIO_START + 64, gpio2_irqs);
 
 	/*
 	 * Mask and clear PMU interrupts

arch/arm/mach-kirkwood/board-dt.c +2 -1

index edc3f8a..27ac3d8 100644
@@ -18,6 +18,7 @@
 #include <asm/mach/arch.h>
 #include <asm/mach/map.h>
 #include <mach/bridge-regs.h>
+#include <plat/irq.h>
 #include "common.h"
 
 static struct of_device_id kirkwood_dt_match_table[] __initdata = {

@@ -84,7 +85,7 @@ DT_MACHINE_START(KIRKWOOD_DT, "Marvell Kirkwood (Flattened Device Tree)")
 	/* Maintainer: Jason Cooper <[email protected]> */
 	.map_io		= kirkwood_map_io,
 	.init_early	= kirkwood_init_early,
-	.init_irq	= kirkwood_init_irq,
+	.init_irq	= orion_dt_init_irq,
 	.timer		= &kirkwood_timer,
 	.init_machine	= kirkwood_dt_init,
 	.restart	= kirkwood_restart,

arch/arm/mach-kirkwood/irq.c +16 -22

index c4c68e5..720063f 100644
@@ -9,20 +9,23 @@
  */
 #include <linux/gpio.h>
 #include <linux/kernel.h>
-#include <linux/init.h>
 #include <linux/irq.h>
-#include <linux/io.h>
 #include <mach/bridge-regs.h>
 #include <plat/irq.h>
-#include "common.h"
 
-static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
-{
-	BUG_ON(irq < IRQ_KIRKWOOD_GPIO_LOW_0_7);
-	BUG_ON(irq > IRQ_KIRKWOOD_GPIO_HIGH_16_23);
+static int __initdata gpio0_irqs[4] = {
+	IRQ_KIRKWOOD_GPIO_LOW_0_7,
+	IRQ_KIRKWOOD_GPIO_LOW_8_15,
+	IRQ_KIRKWOOD_GPIO_LOW_16_23,
+	IRQ_KIRKWOOD_GPIO_LOW_24_31,
+};
 
-	orion_gpio_irq_handler((irq - IRQ_KIRKWOOD_GPIO_LOW_0_7) << 3);
-}
+static int __initdata gpio1_irqs[4] = {
+	IRQ_KIRKWOOD_GPIO_HIGH_0_7,
+	IRQ_KIRKWOOD_GPIO_HIGH_8_15,
+	IRQ_KIRKWOOD_GPIO_HIGH_16_23,
+	0,
+};
 
 void __init kirkwood_init_irq(void)
 {

@@ -32,17 +35,8 @@ void __init kirkwood_init_irq(void)
 	/*
 	 * Initialize gpiolib for GPIOs 0-49.
 	 */
-	orion_gpio_init(0, 32, GPIO_LOW_VIRT_BASE, 0,
-			IRQ_KIRKWOOD_GPIO_START);
-	irq_set_chained_handler(IRQ_KIRKWOOD_GPIO_LOW_0_7, gpio_irq_handler);
-	irq_set_chained_handler(IRQ_KIRKWOOD_GPIO_LOW_8_15, gpio_irq_handler);
-	irq_set_chained_handler(IRQ_KIRKWOOD_GPIO_LOW_16_23, gpio_irq_handler);
-	irq_set_chained_handler(IRQ_KIRKWOOD_GPIO_LOW_24_31, gpio_irq_handler);
-
-	orion_gpio_init(32, 18, GPIO_HIGH_VIRT_BASE, 0,
-			IRQ_KIRKWOOD_GPIO_START + 32);
-	irq_set_chained_handler(IRQ_KIRKWOOD_GPIO_HIGH_0_7, gpio_irq_handler);
-	irq_set_chained_handler(IRQ_KIRKWOOD_GPIO_HIGH_8_15, gpio_irq_handler);
-	irq_set_chained_handler(IRQ_KIRKWOOD_GPIO_HIGH_16_23,
-				gpio_irq_handler);
+	orion_gpio_init(NULL, 0, 32, (void __iomem *)GPIO_LOW_VIRT_BASE, 0,
+			IRQ_KIRKWOOD_GPIO_START, gpio0_irqs);
+	orion_gpio_init(NULL, 32, 18, (void __iomem *)GPIO_HIGH_VIRT_BASE, 0,
+			IRQ_KIRKWOOD_GPIO_START + 32, gpio1_irqs);
 }

arch/arm/mach-mv78xx0/irq.c +8 -14

index e421b70..eff9a75 100644
@@ -9,19 +9,17 @@
  */
 #include <linux/gpio.h>
 #include <linux/kernel.h>
-#include <linux/init.h>
-#include <linux/pci.h>
 #include <linux/irq.h>
 #include <mach/bridge-regs.h>
 #include <plat/irq.h>
 #include "common.h"
 
-static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
-{
-	BUG_ON(irq < IRQ_MV78XX0_GPIO_0_7 || irq > IRQ_MV78XX0_GPIO_24_31);
-
-	orion_gpio_irq_handler((irq - IRQ_MV78XX0_GPIO_0_7) << 3);
-}
+static int __initdata gpio0_irqs[4] = {
+	IRQ_MV78XX0_GPIO_0_7,
+	IRQ_MV78XX0_GPIO_8_15,
+	IRQ_MV78XX0_GPIO_16_23,
+	IRQ_MV78XX0_GPIO_24_31,
+};
 
 void __init mv78xx0_init_irq(void)
 {

@@ -34,11 +32,7 @@ void __init mv78xx0_init_irq(void)
 	 * registers for core #1 are at an offset of 0x18 from those of
 	 * core #0.)
 	 */
-	orion_gpio_init(0, 32, GPIO_VIRT_BASE,
+	orion_gpio_init(NULL, 0, 32, (void __iomem *)GPIO_VIRT_BASE,
 			mv78xx0_core_index() ? 0x18 : 0,
-			IRQ_MV78XX0_GPIO_START);
-	irq_set_chained_handler(IRQ_MV78XX0_GPIO_0_7, gpio_irq_handler);
-	irq_set_chained_handler(IRQ_MV78XX0_GPIO_8_15, gpio_irq_handler);
-	irq_set_chained_handler(IRQ_MV78XX0_GPIO_16_23, gpio_irq_handler);
-	irq_set_chained_handler(IRQ_MV78XX0_GPIO_24_31, gpio_irq_handler);
+			IRQ_MV78XX0_GPIO_START, gpio0_irqs);
 }

arch/arm/mach-orion5x/irq.c +8 -14

index b1b45ff..17da709 100644
@@ -11,19 +11,16 @@
  */
 #include <linux/gpio.h>
 #include <linux/kernel.h>
-#include <linux/init.h>
 #include <linux/irq.h>
-#include <linux/io.h>
 #include <mach/bridge-regs.h>
 #include <plat/irq.h>
-#include "common.h"
 
-static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
-{
-	BUG_ON(irq < IRQ_ORION5X_GPIO_0_7 || irq > IRQ_ORION5X_GPIO_24_31);
-
-	orion_gpio_irq_handler((irq - IRQ_ORION5X_GPIO_0_7) << 3);
-}
+static int __initdata gpio0_irqs[4] = {
+	IRQ_ORION5X_GPIO_0_7,
+	IRQ_ORION5X_GPIO_8_15,
+	IRQ_ORION5X_GPIO_16_23,
+	IRQ_ORION5X_GPIO_24_31,
+};
 
 void __init orion5x_init_irq(void)
 {

@@ -32,9 +29,6 @@ void __init orion5x_init_irq(void)
 	/*
 	 * Initialize gpiolib for GPIOs 0-31.
 	 */
-	orion_gpio_init(0, 32, GPIO_VIRT_BASE, 0, IRQ_ORION5X_GPIO_START);
-	irq_set_chained_handler(IRQ_ORION5X_GPIO_0_7, gpio_irq_handler);
-	irq_set_chained_handler(IRQ_ORION5X_GPIO_8_15, gpio_irq_handler);
-	irq_set_chained_handler(IRQ_ORION5X_GPIO_16_23, gpio_irq_handler);
-	irq_set_chained_handler(IRQ_ORION5X_GPIO_24_31, gpio_irq_handler);
+	orion_gpio_init(NULL, 0, 32, (void __iomem *)GPIO_VIRT_BASE, 0,
+			IRQ_ORION5X_GPIO_START, gpio0_irqs);
 }

arch/arm/plat-orion/gpio.c +125 -41

index af95af2..dfda74f 100644
@@ -8,15 +8,22 @@
  * warranty of any kind, whether express or implied.
  */
 
+#define DEBUG
+
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/irq.h>
+#include <linux/irqdomain.h>
 #include <linux/module.h>
 #include <linux/spinlock.h>
 #include <linux/bitops.h>
 #include <linux/io.h>
 #include <linux/gpio.h>
 #include <linux/leds.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <plat/gpio.h>
 
 /*
  * GPIO unit register offsets.

@@ -38,6 +45,7 @@ struct orion_gpio_chip {
 	unsigned long		valid_output;
 	int			mask_offset;
 	int			secondary_irq_base;
+	struct irq_domain       *domain;
 };
 
 static void __iomem *GPIO_OUT(struct orion_gpio_chip *ochip)

@@ -222,10 +230,10 @@ static int orion_gpio_to_irq(struct gpio_chip *chip, unsigned pin)
 	struct orion_gpio_chip *ochip =
 		container_of(chip, struct orion_gpio_chip, chip);
 
-	return ochip->secondary_irq_base + pin;
+	return irq_create_mapping(ochip->domain,
+				  ochip->secondary_irq_base + pin);
 }
 
-
 /*
  * Orion-specific GPIO API extensions.
  */

@@ -353,12 +361,10 @@ static int gpio_irq_set_type(struct irq_data *d, u32 type)
 	int pin;
 	u32 u;
 
-	pin = d->irq - gc->irq_base;
+	pin = d->hwirq - ochip->secondary_irq_base;
 
 	u = readl(GPIO_IO_CONF(ochip)) & (1 << pin);
 	if (!u) {
-		printk(KERN_ERR "orion gpio_irq_set_type failed "
-				"(irq %d, pin %d).\n", d->irq, pin);
 		return -EINVAL;
 	}
 

@@ -397,17 +403,53 @@ static int gpio_irq_set_type(struct irq_data *d, u32 type)
 			u &= ~(1 << pin);	/* rising */
 		writel(u, GPIO_IN_POL(ochip));
 	}
-
 	return 0;
 }
 
-void __init orion_gpio_init(int gpio_base, int ngpio,
-			    u32 base, int mask_offset, int secondary_irq_base)
+static void gpio_irq_handler(unsigned irq, struct irq_desc *desc)
+{
+	struct orion_gpio_chip *ochip = irq_get_handler_data(irq);
+	u32 cause, type;
+	int i;
+
+	if (ochip == NULL)
+		return;
+
+	cause = readl(GPIO_DATA_IN(ochip)) & readl(GPIO_LEVEL_MASK(ochip));
+	cause |= readl(GPIO_EDGE_CAUSE(ochip)) & readl(GPIO_EDGE_MASK(ochip));
+
+	for (i = 0; i < ochip->chip.ngpio; i++) {
+		int irq;
+
+		irq = ochip->secondary_irq_base + i;
+
+		if (!(cause & (1 << i)))
+			continue;
+
+		type = irqd_get_trigger_type(irq_get_irq_data(irq));
+		if ((type & IRQ_TYPE_SENSE_MASK) == IRQ_TYPE_EDGE_BOTH) {
+			/* Swap polarity (race with GPIO line) */
+			u32 polarity;
+
+			polarity = readl(GPIO_IN_POL(ochip));
+			polarity ^= 1 << i;
+			writel(polarity, GPIO_IN_POL(ochip));
+		}
+		generic_handle_irq(irq);
+	}
+}
+
+void __init orion_gpio_init(struct device_node *np,
+			    int gpio_base, int ngpio,
+			    void __iomem *base, int mask_offset,
+			    int secondary_irq_base,
+			    int irqs[4])
 {
 	struct orion_gpio_chip *ochip;
 	struct irq_chip_generic *gc;
 	struct irq_chip_type *ct;
 	char gc_label[16];
+	int i;
 
 	if (orion_gpio_chip_count == ARRAY_SIZE(orion_gpio_chips))
 		return;

@@ -426,6 +468,10 @@ void __init orion_gpio_init(int gpio_base, int ngpio,
 	ochip->chip.base = gpio_base;
 	ochip->chip.ngpio = ngpio;
 	ochip->chip.can_sleep = 0;
+#ifdef CONFIG_OF
+	ochip->chip.of_node = np;
+#endif
+
 	spin_lock_init(&ochip->lock);
 	ochip->base = (void __iomem *)base;
 	ochip->valid_input = 0;

@@ -435,8 +481,6 @@ void __init orion_gpio_init(int gpio_base, int ngpio,
 
 	gpiochip_add(&ochip->chip);
 
-	orion_gpio_chip_count++;
-
 	/*
 	 * Mask and clear GPIO interrupts.
 	 */

@@ -444,16 +488,28 @@ void __init orion_gpio_init(int gpio_base, int ngpio,
 	writel(0, GPIO_EDGE_MASK(ochip));
 	writel(0, GPIO_LEVEL_MASK(ochip));
 
-	gc = irq_alloc_generic_chip("orion_gpio_irq", 2, secondary_irq_base,
+	/* Setup the interrupt handlers. Each chip can have up to 4
+	 * interrupt handlers, with each handler dealing with 8 GPIO
+	 * pins. */
+
+	for (i = 0; i < 4; i++) {
+		if (irqs[i]) {
+			irq_set_handler_data(irqs[i], ochip);
+			irq_set_chained_handler(irqs[i], gpio_irq_handler);
+		}
+	}
+
+	gc = irq_alloc_generic_chip("orion_gpio_irq", 2,
+				    secondary_irq_base,
 				    ochip->base, handle_level_irq);
 	gc->private = ochip;
-
 	ct = gc->chip_types;
 	ct->regs.mask = ochip->mask_offset + GPIO_LEVEL_MASK_OFF;
 	ct->type = IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW;
 	ct->chip.irq_mask = irq_gc_mask_clr_bit;
 	ct->chip.irq_unmask = irq_gc_mask_set_bit;
 	ct->chip.irq_set_type = gpio_irq_set_type;
+	ct->chip.name = ochip->chip.label;
 
 	ct++;
 	ct->regs.mask = ochip->mask_offset + GPIO_EDGE_MASK_OFF;

@@ -464,41 +520,69 @@ void __init orion_gpio_init(int gpio_base, int ngpio,
 	ct->chip.irq_unmask = irq_gc_mask_set_bit;
 	ct->chip.irq_set_type = gpio_irq_set_type;
 	ct->handler = handle_edge_irq;
+	ct->chip.name = ochip->chip.label;
 
 	irq_setup_generic_chip(gc, IRQ_MSK(ngpio), IRQ_GC_INIT_MASK_CACHE,
 			       IRQ_NOREQUEST, IRQ_LEVEL | IRQ_NOPROBE);
-}
 
-void orion_gpio_irq_handler(int pinoff)
-{
-	struct orion_gpio_chip *ochip;
-	u32 cause, type;
-	int i;
-
-	ochip = orion_gpio_chip_find(pinoff);
-	if (ochip == NULL)
-		return;
-
-	cause = readl(GPIO_DATA_IN(ochip)) & readl(GPIO_LEVEL_MASK(ochip));
-	cause |= readl(GPIO_EDGE_CAUSE(ochip)) & readl(GPIO_EDGE_MASK(ochip));
-
-	for (i = 0; i < ochip->chip.ngpio; i++) {
-		int irq;
+	/* Setup irq domain on top of the generic chip. */
+	ochip->domain = irq_domain_add_legacy(np,
+					      ochip->chip.ngpio,
+					      ochip->secondary_irq_base,
+					      ochip->secondary_irq_base,
+					      &irq_domain_simple_ops,
+					      ochip);
+	if (!ochip->domain)
+		panic("%s: couldn't allocate irq domain (DT).\n",
+		      ochip->chip.label);
 
-		irq = ochip->secondary_irq_base + i;
+	orion_gpio_chip_count++;
+}
 
-		if (!(cause & (1 << i)))
-			continue;
+#ifdef CONFIG_OF
+static void __init orion_gpio_of_init_one(struct device_node *np,
+					  int irq_gpio_base)
+{
+	int ngpio, gpio_base, mask_offset;
+	void __iomem *base;
+	int ret, i;
+	int irqs[4];
+	int secondary_irq_base;
+
+	ret = of_property_read_u32(np, "ngpio", &ngpio);
+	if (ret)
+		goto out;
+	ret = of_property_read_u32(np, "mask-offset", &mask_offset);
+	if (ret == -EINVAL)
+		mask_offset = 0;
+	else
+		goto out;
+	base = of_iomap(np, 0);
+	if (!base)
+		goto out;
+
+	secondary_irq_base = irq_gpio_base + (32 * orion_gpio_chip_count);
+	gpio_base = 32 * orion_gpio_chip_count;
+
+	/* Get the interrupt numbers. Each chip can have up to 4
+	 * interrupt handlers, with each handler dealing with 8 GPIO
+	 * pins. */
+
+	for (i = 0; i < 4; i++)
+		irqs[i] = irq_of_parse_and_map(np, i);
+
+	orion_gpio_init(np, gpio_base, ngpio, base, mask_offset,
+			secondary_irq_base, irqs);
+	return;
+out:
+	pr_err("%s: %s: missing mandatory property\n", __func__, np->name);
+}
 
-		type = irqd_get_trigger_type(irq_get_irq_data(irq));
-		if ((type & IRQ_TYPE_SENSE_MASK) == IRQ_TYPE_EDGE_BOTH) {
-			/* Swap polarity (race with GPIO line) */
-			u32 polarity;
+void __init orion_gpio_of_init(int irq_gpio_base)
+{
+	struct device_node *np;
 
-			polarity = readl(GPIO_IN_POL(ochip));
-			polarity ^= 1 << i;
-			writel(polarity, GPIO_IN_POL(ochip));
-		}
-		generic_handle_irq(irq);
-	}
+	for_each_compatible_node(np, NULL, "marvell,orion-gpio")
+		orion_gpio_of_init_one(np, irq_gpio_base);
 }
+#endif

arch/arm/plat-orion/include/plat/gpio.h +7 -9

index bec0c98..81c6fc8 100644
@@ -13,7 +13,7 @@
 
 #include <linux/init.h>
 #include <linux/types.h>
-
+#include <linux/irqdomain.h>
 /*
  * Orion-specific GPIO API extensions.
  */

@@ -27,13 +27,11 @@ int orion_gpio_led_blink_set(unsigned gpio, int state,
 void orion_gpio_set_valid(unsigned pin, int mode);
 
 /* Initialize gpiolib. */
-void __init orion_gpio_init(int gpio_base, int ngpio,
-			    u32 base, int mask_offset, int secondary_irq_base);
-
-/*
- * GPIO interrupt handling.
- */
-void orion_gpio_irq_handler(int irqoff);
-
+void __init orion_gpio_init(struct device_node *np,
+			    int gpio_base, int ngpio,
+			    void __iomem *base, int mask_offset,
+			    int secondary_irq_base,
+			    int irq[4]);
 
+void __init orion_gpio_of_init(int irq_gpio_base);
 #endif

arch/arm/plat-orion/include/plat/irq.h +1 -2

index f05eeab..50547e4 100644
@@ -12,6 +12,5 @@
 #define __PLAT_IRQ_H
 
 void orion_irq_init(unsigned int irq_start, void __iomem *maskaddr);
-
-
+void __init orion_dt_init_irq(void);
 #endif

arch/arm/plat-orion/irq.c +40 -0

index 2d5b9c1..d751964 100644
@@ -11,8 +11,12 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/irq.h>
+#include <linux/irqdomain.h>
 #include <linux/io.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
 #include <plat/irq.h>
+#include <plat/gpio.h>
 
 void __init orion_irq_init(unsigned int irq_start, void __iomem *maskaddr)
 {

@@ -32,3 +36,39 @@ void __init orion_irq_init(unsigned int irq_start, void __iomem *maskaddr)
 	irq_setup_generic_chip(gc, IRQ_MSK(32), IRQ_GC_INIT_MASK_CACHE,
 			       IRQ_NOREQUEST, IRQ_LEVEL | IRQ_NOPROBE);
 }
+
+#ifdef CONFIG_OF
+static int __init orion_add_irq_domain(struct device_node *np,
+				       struct device_node *interrupt_parent)
+{
+	int i = 0, irq_gpio;
+	void __iomem *base;
+
+	do {
+		base = of_iomap(np, i);
+		if (base) {
+			orion_irq_init(i * 32, base);
+			i++;
+		}
+	} while (base);
+
+	irq_domain_add_legacy(np, i * 32, 0, 0,
+			      &irq_domain_simple_ops, NULL);
+
+	irq_gpio = i * 32;
+	orion_gpio_of_init(irq_gpio);
+
+	return 0;
+}
+
+static const struct of_device_id orion_irq_match[] = {
+	{ .compatible = "marvell,orion-intc",
+	  .data = orion_add_irq_domain, },
+	{},
+};
+
+void __init orion_dt_init_irq(void)
+{
+	of_irq_init(orion_irq_match);
+}
+#endif

Signature · 1 line

1.7.10

Both IRQ and GPIO controllers can now be represented in DT. The IRQ controllers are setup first, and then the GPIO controllers. Interrupts for GPIO lines are placed directly after the main interrupts in the interrupt space.

Signed-off-by Andrew Lunn <[email protected]>

Hello Andrew,

Le Tue, 3 Jul 2012 16:22:34 +0200,
Andrew Lunn <[email protected]> a ??crit :

Quoted 5 lines

Both IRQ and GPIO controllers can now be represented in DT. The IRQ controllers are setup first, and then the GPIO controllers. Interrupts for GPIO lines are placed directly after the main interrupts in the interrupt space.

Quoted 1 line

Signed-off-by Andrew Lunn <[email protected]>

I have started working on a pinctrl driver for mvebu, which would handle pin muxing (MPP) + gpio + gpio interrupts.

So far, the pin muxing part is working (needs some polishing, but the foundation is here), with device tree bindings. I think the pin muxing could be used for Orion as well.

Now, I'm planning to start working on the gpio + gpio interrupts parts of the driver, and I'm wondering how to interact with your work on the matter.

My understanding is that the new way of doing a pinmux+gpio driver is to implement it in drivers/pinctrl/, which I have started doing.

Should I continue working on a drivers/pinctrl/ driver for mvebu for Armada 370/XP, and then we see together if it makes sense to extend to Orion, and if so, what changes are needed?

Hi Thomas

You are not the only one working in this area. Arnaud Patard said he was look at this as well.

Quoted 3 lines

So far, the pin muxing part is working (needs some polishing, but the foundation is here), with device tree bindings. I think the pin muxing could be used for Orion as well.

Great. This is one of the big things which we are missing when moving a system over the DT. Being able to describe this in DT in a standardized way is very much welcome.

Quoted 3 lines

Now, I'm planning to start working on the gpio + gpio interrupts parts of the driver, and I'm wondering how to interact with your work on the matter.

I've publicly said, what i have is enough to get it working, but i know its not the end solution. What i have is enough that gpio-key, gpio-led, as described in DT, works. Boards can start using these and as far as i can tell, the binding should not need to change. The GPIO controller binding should also be sufficiently generic that it should also not need changes when we replace the driver with pinctrl.

The biggest problem i had, is the interaction between generic chip interrupts and irqdomain. There has been work to integrate the two, but its stalled. Either the work needs restarting and completing, or you need to throw away the use of generic interrupt so that you can use irqdomain linear. IMHO, throwing away generic interrupt is the wrong way.

The other thing you need to keep in mind is the namespace issues between normal interrupts and GPIO interrupts. The way my patch works is that normal interrupts are setup first, counting how many have been created. Then GPIO interrupts are added, starting off where the normal interrupts ended. Since 370/XP uses different interrupt code, you need to think how to solve this issue.

Quoted 3 lines

Should I continue working on a drivers/pinctrl/ driver for mvebu for Armada 370/XP, and then we see together if it makes sense to extend to Orion, and if so, what changes are needed?

I know Marvell has contracted you to port to Armada 370/XP, not mvebu, aka all chips which might be able to share this code. This IMHO is wrong. So i personally would put the kirkwood and the Armada 370/XP data sheets next to each other, and from the beginning on write the driver so it supports them all. This assumes the ASIC engineers have not radically changed anything... Please also try to stick to the DT binding i've used.

I've looked at the datasheet for the 88F6281, and the GPIO registers look fairly similar with Armada 370/XP, except that:

* On 88F6281, there are two GPIO banks, while on Armada 370/XP it's a continuous range of registers that control the GPIOs. But it can be assimilated as one bank handling more than 32 GPIOs. So shouldn't be a big problem to handle.

* The Armada 370/XP have additional set/clear registers that allow to modify the GPIO states without having to do a read/mask/write sequence, which is nicer on SMP. I guess we can handle that as a variant inside the driver without too much problem.

Marvell has given me a 88F6282 eval board, so I guess they are fine with me spending some time ensuring that the new code works properly on the previous platforms, especially in this case where the amount of work does not seem to be enormous to keep the compatibility.

Quoted 12 lines

Hello Andrew,

Le Tue, 3 Jul 2012 16:22:34 +0200,
Andrew Lunn <[email protected]> a ??crit :

Quoted 6 lines

Both IRQ and GPIO controllers can now be represented in DT. The IRQ controllers are setup first, and then the GPIO controllers. Interrupts for GPIO lines are placed directly after the main interrupts in the interrupt space.

Signed-off-by Andrew Lunn <[email protected]>

Quoted 2 lines

I have started working on a pinctrl driver for mvebu, which would handle pin muxing (MPP) + gpio + gpio interrupts.

Hi Thomas

You are not the only one working in this area. Arnaud Patard said he was look at this as well.

Quoted 4 lines

So far, the pin muxing part is working (needs some polishing, but the foundation is here), with device tree bindings. I think the pin muxing could be used for Orion as well.

Great. This is one of the big things which we are missing when moving a system over the DT. Being able to describe this in DT in a standardized way is very much welcome.

Quoted 3 lines

Now, I'm planning to start working on the gpio + gpio interrupts parts of the driver, and I'm wondering how to interact with your work on the matter.

I've publicly said, what i have is enough to get it working, but i know its not the end solution. What i have is enough that gpio-key, gpio-led, as described in DT, works. Boards can start using these and as far as i can tell, the binding should not need to change. The GPIO controller binding should also be sufficiently generic that it should also not need changes when we replace the driver with pinctrl.

The biggest problem i had, is the interaction between generic chip interrupts and irqdomain. There has been work to integrate the two, but its stalled. Either the work needs restarting and completing, or you need to throw away the use of generic interrupt so that you can use irqdomain linear. IMHO, throwing away generic interrupt is the wrong way.

The other thing you need to keep in mind is the namespace issues between normal interrupts and GPIO interrupts. The way my patch works is that normal interrupts are setup first, counting how many have been created. Then GPIO interrupts are added, starting off where the normal interrupts ended. Since 370/XP uses different interrupt code, you need to think how to solve this issue.

Quoted 3 lines

My understanding is that the new way of doing a pinmux+gpio driver is to implement it in drivers/pinctrl/, which I have started doing.

Great.

Quoted 3 lines

Should I continue working on a drivers/pinctrl/ driver for mvebu for Armada 370/XP, and then we see together if it makes sense to extend to Orion, and if so, what changes are needed?

I know Marvell has contracted you to port to Armada 370/XP, not mvebu, aka all chips which might be able to share this code. This IMHO is wrong. So i personally would put the kirkwood and the Armada 370/XP data sheets next to each other, and from the beginning on write the driver so it supports them all. This assumes the ASIC engineers have not radically changed anything... Please also try to stick to the DT binding i've used.

Quoted 2 lines

You are not the only one working in this area. Arnaud Patard said he was look at this as well.

yeah, but tbh I've not made anything yet. If Thomas has already some code for it, we should try to make it "generic" so as to use it on armada xp and orion platforms.

Quoted 7 lines

I know Marvell has contracted you to port to Armada 370/XP, not mvebu, aka all chips which might be able to share this code. This IMHO is wrong. So i personally would put the kirkwood and the Armada 370/XP data sheets next to each other, and from the beginning on write the driver so it supports them all. This assumes the ASIC engineers have not radically changed anything... Please also try to stick to the DT binding i've used.

agreed. it should be written with all platforms in mind, as long as they're compatible. It can only make things better imho.

Le Thu, 05 Jul 2012 12:11:40 +0200,
Arnaud Patard (Rtp) <[email protected]> a ?crit :

Quoted 3 lines

Quoted 2 lines

You are not the only one working in this area. Arnaud Patard said he was look at this as well.

Quoted 3 lines

yeah, but tbh I've not made anything yet. If Thomas has already some code for it, we should try to make it "generic" so as to use it on armada xp and orion platforms.

The MPP registers are identical on Armada XP/370 and 88F6281 (not sure which other SoC datasheet I should be checking). Basically, it's just a range of contiguous registers, with 4 bits per pin to select the function.

So my pinmux driver should simply work as is for Orion as well. The only difference between platforms is the number of MPP pins that are available, but this number also varies between versions of Armada XP and Armada 370, so I already support this in the driver.

Quoted 4 lines

The MPP registers are identical on Armada XP/370 and 88F6281 (not sure which other SoC datasheet I should be checking). Basically, it's just a range of contiguous registers, with 4 bits per pin to select the function.

iirc, other SoCs are similar. The small exception being dove I guess. Dove has a 3rd gpo [the gpios are output only] bank but to be used as gpio require that a special bit is set and it's for all gpios of this bank. You'll find this bit in the "general mpp configuration register" if you look at the datasheet.

Quoted 4 lines

So my pinmux driver should simply work as is for Orion as well. The only difference between platforms is the number of MPP pins that are available, but this number also varies between versions of Armada XP and Armada 370, so I already support this in the driver.

Are there some output-only gpio on armada xp/370 like on kirkwood/dove ?

Yes. I am not sure yet how to describe those in the DT, or even if it is actually useful to describe them. Wouldn't it be simpler to just leave to the user of the GPIO to use a GPIO that's appropriate for its usage, i.e not use a GPO when input is needed?

Quoted 4 lines

Yes. I am not sure yet how to describe those in the DT, or even if it is actually useful to describe them. Wouldn't it be simpler to just leave to the user of the GPIO to use a GPIO that's appropriate for its usage, i.e not use a GPO when input is needed?

We assume the hardware designer has got the basic hardware right. Its not going to work otherwise.

What we are trying to detect is a DT author making a typo, assigning a gpio-key to a GPO pin, for example. The current MPP scheme will detect this sort of error and issue a warning to the kernel logs.

The biggest problem i had, is the interaction between generic chip interrupts and irqdomain. There has been work to integrate the two, but its stalled. Either the work needs restarting and completing, or you need to throw away the use of generic interrupt so that you can use irqdomain linear. IMHO, throwing away generic interrupt is the wrong way.

Hello,

Le Thu, 5 Jul 2012 11:48:24 +0200,
Andrew Lunn <[email protected]> a ??crit :

Quoted 6 lines

The biggest problem i had, is the interaction between generic chip interrupts and irqdomain. There has been work to integrate the two, but its stalled. Either the work needs restarting and completing, or you need to throw away the use of generic interrupt so that you can use irqdomain linear. IMHO, throwing away generic interrupt is the wrong way.

Can you expand on why you think it would be wrong to throw away the usage of irq_chip_generic, compared to implementing directly an irq_chip?

Quoted 7 lines

Quoted 6 lines

The biggest problem i had, is the interaction between generic chip interrupts and irqdomain. There has been work to integrate the two, but its stalled. Either the work needs restarting and completing, or you need to throw away the use of generic interrupt so that you can use irqdomain linear. IMHO, throwing away generic interrupt is the wrong way.

Quoted 3 lines

Can you expand on why you think it would be wrong to throw away the usage of irq_chip_generic, compared to implementing directly an irq_chip?

Basically you are asking, why should i use the framework when i can do it by hand.

What are the advantages if ignoring the framework and doing it by hand?

I guess I'll just stick to irq_chip_generic + irqdomain_legacy for now.

That is what my patch currently does. Feel free to re-use parts of the orion code.

Both IRQ and GPIO controllers can now be represented in DT. The IRQ controllers are setup first, and then the GPIO controllers. Interrupts for GPIO lines are placed directly after the main interrupts in the interrupt space.

Documentation/devicetree/bindings/arm/mrvl/intc.txt +0 -0

index 80b9a94..8927e10 100644
@@ -38,3 +38,22 @@ Example:
 		reg-names = "mux status", "mux mask";
 		mrvl,intc-nr-irqs = <2>;

Documentation/devicetree/bindings/arm/mrvl/intc.txt +25 -0

+#ifdef CONFIG_OF
+static int __init orion_add_irq_domain(struct device_node *np,
+				       struct device_node *interrupt_parent)
+{
+	int i = 0, irq_gpio;
+	void __iomem *base;
+
+	do {
+		base = of_iomap(np, i);
+		if (base) {
+			orion_irq_init(i * 32, base);
+			i++;
+		}
+	} while (base);
+
+	irq_domain_add_legacy(np, i * 32, 0, 0,
+			      &irq_domain_simple_ops, NULL);
+
+	irq_gpio = i * 32;
+	orion_gpio_of_init(irq_gpio);
+
+	return 0;
+}
+
+static const struct of_device_id orion_irq_match[] = {

Documentation/devicetree/bindings/arm/mrvl/intc.txt Example: · line 49

Quoted 6 lines

Documentation/devicetree/bindings/arm/mrvl/intc.txt +6 -0

+Required properties
+- compatible :  Should be "marvell,orion-intc".
+- #interrupt-cells: Specifies the number of cells needed to encode an
+  interrupt source. Supported value is <1>.
+- interrupt-controller : Declare this node to be an interrupt controller.
+- reg : Interrupt mask address.

I think you should clarify that the "reg" property can be multiple 4-byte ranges, because that is fairly unusual.

Quoted 32 lines

arch/arm/plat-orion/irq.c +32 -0

+#ifdef CONFIG_OF
+static int __init orion_add_irq_domain(struct device_node *np,
+				       struct device_node *interrupt_parent)
+{
+	int i = 0, irq_gpio;
+	void __iomem *base;
+
+	do {
+		base = of_iomap(np, i);
+		if (base) {
+			orion_irq_init(i * 32, base);
+			i++;
+		}
+	} while (base);
+
+	irq_domain_add_legacy(np, i * 32, 0, 0,
+			      &irq_domain_simple_ops, NULL);
+
+	irq_gpio = i * 32;
+	orion_gpio_of_init(irq_gpio);
+
+	return 0;
+}
+
+static const struct of_device_id orion_irq_match[] = {
+	{ .compatible = "marvell,orion-intc",
+	  .data = orion_add_irq_domain, },
+	{},
+};
+
+void __init orion_dt_init_irq(void)
+{

arch/arm/plat-orion/irq.c void __init orion_irq_init(unsigned int irq_start, void __iomem *maskaddr) · line 74

Quoted 3 lines

arch/arm/plat-orion/irq.c +3 -0

+	of_irq_init(orion_irq_match);
+}
+#endif

I'm wondering about this one. The other platforms usually put the secondary interrupt controllers into the same match table, while you call orion_gpio_of_init from orion_add_irq_domain. Can you explain why you do this? Does it have any disadvantages?

Quoted 5 lines

I'm wondering about this one. The other platforms usually put the secondary interrupt controllers into the same match table, while you call orion_gpio_of_init from orion_add_irq_domain. Can you explain why you do this? Does it have any disadvantages?

The issue is knowing what IRQ number to use for the secondary interrupts.

Orion use generic chip interrupts, both for the main interrupts and the GPIO interrupts. This does not yet support irq domain, so i have to layer a legacy domain on top. The legacy domain needs to know the first IRQ and the number of IRQs. For the primary IRQs that is easy. However, GPIO IRQ is not so easy, it depends on how many primary IRQs there are. This is not fixed. Orion5x has 32, Dove 64, kirkwood, 64, and mv78xx0 has 96. I need to know this number when adding the GPIO secondary IRQ legacy domain. By calling orion_gpio_of_init() in the orion_add_irq_domain() i have this number to hand. If i used to entries in the match table, i would have to put this number into some global variable, or somehow ask the IRQ subsystem what the next free IRQ number is.

As for disadvantages, humm. Dove has yet more interrupts, from the PMU. They are currently unsupported in DT. When we add support for the PMU interrupt controller, we are going to have the same problem, what IRQ base should it use. Either we extend the chaining, calling dove_pmu_of_init from orion_gpio_of_init(), where we know the next free IRQ. Or we find out how to ask the IRQ subsystem for the next available. Better still, the work to make generic chip interrupts irq domain aware would get completed, and we can swap all this code to irq domain linear and this whole probable probably goes away.

Quoted 4 lines

Orion use generic chip interrupts, both for the main interrupts and the GPIO interrupts. This does not yet support irq domain, so i have to layer a legacy domain on top. The legacy domain needs to know the first IRQ and the number of IRQs. For the primary IRQs that is

Quoted 8 lines

easy. However, GPIO IRQ is not so easy, it depends on how many primary IRQs there are. This is not fixed. Orion5x has 32, Dove 64, kirkwood, 64, and mv78xx0 has 96. I need to know this number when adding the GPIO secondary IRQ legacy domain. By calling orion_gpio_of_init() in the orion_add_irq_domain() i have this number to hand. If i used to entries in the match table, i would have to put this number into some global variable, or somehow ask the IRQ subsystem what the next free IRQ number is.

But couldn't you store the number of interrupts for the primary controller in a local variable? I think the of_irq code already guarantees that the parent is probed first.

Quoted 13 lines

Quoted 12 lines

Orion use generic chip interrupts, both for the main interrupts and the GPIO interrupts. This does not yet support irq domain, so i have to layer a legacy domain on top. The legacy domain needs to know the first IRQ and the number of IRQs. For the primary IRQs that is easy. However, GPIO IRQ is not so easy, it depends on how many primary IRQs there are. This is not fixed. Orion5x has 32, Dove 64, kirkwood, 64, and mv78xx0 has 96. I need to know this number when adding the GPIO secondary IRQ legacy domain. By calling orion_gpio_of_init() in the orion_add_irq_domain() i have this number to hand. If i used to entries in the match table, i would have to put this number into some global variable, or somehow ask the IRQ subsystem what the next free IRQ number is.

Quoted 3 lines

But couldn't you store the number of interrupts for the primary controller in a local variable? I think the of_irq code already guarantees that the parent is probed first.

Sure i could. But what is the advantage of that?

The issue is knowing what IRQ number to use for the secondary interrupts.

Orion use generic chip interrupts, both for the main interrupts and the GPIO interrupts. This does not yet support irq domain, so i have to layer a legacy domain on top. The legacy domain needs to know the first IRQ and the number of IRQs. For the primary IRQs that is easy. However, GPIO IRQ is not so easy, it depends on how many primary IRQs there are. This is not fixed. Orion5x has 32, Dove 64, kirkwood, 64, and mv78xx0 has 96. I need to know this number when adding the GPIO secondary IRQ legacy domain. By calling orion_gpio_of_init() in the orion_add_irq_domain() i have this number to hand. If i used to entries in the match table, i would have to put this number into some global variable, or somehow ask the IRQ subsystem what the next free IRQ number is.

Quoted 15 lines · Andrew Lunn

The issue is knowing what IRQ number to use for the secondary interrupts.

Orion use generic chip interrupts, both for the main interrupts and the GPIO interrupts. This does not yet support irq domain, so i have to layer a legacy domain on top. The legacy domain needs to know the first IRQ and the number of IRQs. For the primary IRQs that is easy. However, GPIO IRQ is not so easy, it depends on how many primary IRQs there are. This is not fixed. Orion5x has 32, Dove 64, kirkwood, 64, and mv78xx0 has 96. I need to know this number when adding the GPIO secondary IRQ legacy domain. By calling orion_gpio_of_init() in the orion_add_irq_domain() i have this number to hand. If i used to entries in the match table, i would have to put this number into some global variable, or somehow ask the IRQ subsystem what the next free IRQ number is.

Andrew,

is it possible to group all gpio banks into one DT description?

	bank5 at e8400 {
		reg = <0xe8400 0x20>;
		ngpio = <8>;
		marvell,orion-gpio-output-only;

Andrew,

is it possible to group all gpio banks into one DT description? For mach-dove it could be something like:

gpio: gpio-controller {
        compatible = "marvell, orion-gpio";
        ...

        bank0 at d0400 {
                reg = <0xd0400 0x40>;
                ngpio = <8>;
                mask-offset = <0>;
                interrupts = <12>;

        };

        bank1 at d0400 {
                reg = <0xd0400 0x40>;
                ngpio = <8>;
                mask-offset = <8>;
                interrupts = <13>;
        };

This would have the advantage that DT describes gpio-to-irq dependencies. Moreover, nodes that reference gpios can do gpios = <&gpio 71 0>; instead of gpios = <&gpio3 7 0>;

arch/arm/mach-dove/irq.c +3 -0

@@ -90,6 +74,27 @@ static void pmu_irq_handler(unsigned int irq, struct irq_desc *desc)
      }
 }
 
+static int __initdata gpio0_irqs[4] = {
+     IRQ_DOVE_GPIO_0_7,
+     IRQ_DOVE_GPIO_8_15,

This way you have multiple nodes with the same register and different names, which is not how it normally works.

Quoted 3 lines

This would have the advantage that DT describes gpio-to-irq dependencies. Moreover, nodes that reference gpios can do gpios =<&gpio 71 0>; instead of gpios =<&gpio3 7 0>;

Is that desired?

The device tree representation should match what is in the data sheet normally. If they are in a single continuous number range, then we should probably have a single device node with multiple register ranges rather than one device node for each 32-bit register. Looking at arch/arm/plat-orion/gpio.c I think that is not actually the case though and having separate banks is more logical.

Quoted 2 lines · Arnd Bergmann

This way you have multiple nodes with the same register and different names, which is not how it normally works.

Ok.

Quoted 4 lines

Quoted 3 lines

This would have the advantage that DT describes gpio-to-irq dependencies. Moreover, nodes that reference gpios can do gpios =<&gpio 71 0>; instead of gpios =<&gpio3 7 0>;

Quoted 8 lines

Is that desired?

The device tree representation should match what is in the data sheet normally. If they are in a single continuous number range, then we should probably have a single device node with multiple register ranges rather than one device node for each 32-bit register. Looking at arch/arm/plat-orion/gpio.c I think that is not actually the case though and having separate banks is more logical.

Well, looking at the datasheet of Dove GPIOs are numbered [63:0] plus GPOs [71:64]. This dt will be a lot shorter and maybe it is describing the hardware as it is. (Not sure about the syntax for irqs, though)

Something completely different I just noticed in the original patch:

Quoted 7 lines

arch/arm/mach-dove/irq.c +3 -0

@@ -90,6 +74,27 @@ static void pmu_irq_handler(unsigned int irq, struct irq_desc *desc)
      }
 }
 
+static int __initdata gpio0_irqs[4] = {
+     IRQ_DOVE_GPIO_0_7,
+     IRQ_DOVE_GPIO_8_15,

arch/arm/mach-dove/irq.c static void pmu_irq_handler(unsigned int irq, struct irq_desc *desc) · line 89

Quoted 10 lines

arch/arm/mach-dove/irq.c +10 -0

+     IRQ_DOVE_GPIO_16_23,
+     IRQ_DOVE_GPIO_24_31,
+};
+
+static int __initdata gpio1_irqs[4] = {
+     IRQ_DOVE_HIGH_GPIO,
+     0,
+     0,
+     0,
+};

I think the latter one needs to be

arch/arm/mach-dove/irq.c +6 -0

+static int __initdata gpio1_irqs[4] = {
+     IRQ_DOVE_HIGH_GPIO,
+     IRQ_DOVE_HIGH_GPIO,
+     IRQ_DOVE_HIGH_GPIO,
+     IRQ_DOVE_HIGH_GPIO,
+};

so we register all four parts to the same primary IRQ. The same is true for the devicetree representation.

Quoted 11 lines

I think the latter one needs to be

arch/arm/mach-dove/irq.c +6 -0

+static int __initdata gpio1_irqs[4] = {
+     IRQ_DOVE_HIGH_GPIO,
+     IRQ_DOVE_HIGH_GPIO,
+     IRQ_DOVE_HIGH_GPIO,
+     IRQ_DOVE_HIGH_GPIO,
+};

so we register all four parts to the same primary IRQ. The same is true for the devicetree representation.

Nope, does not work like that.

It does not matter which IRQ of a GPIO chip fires. It looks at the IRQ cause bits for all lines and fires off the secondary ISR as needed for the whole chip. So in effect, there is a mapping IRQ->GPIO chip, not IRQ->1/4 of GPIO chip. With what you suggest above, you would end up with four chained interrupt handlers, all being handled by the same interrupt handler for one chio, and the last three in the chain would never do anything since the first one does all the work.

Quoted 14 lines · Andrew Lunn

Quoted 11 lines

I think the latter one needs to be

(quoted diff) +6 -0

+static int __initdata gpio1_irqs[4] = {
+     IRQ_DOVE_HIGH_GPIO,
+     IRQ_DOVE_HIGH_GPIO,
+     IRQ_DOVE_HIGH_GPIO,
+     IRQ_DOVE_HIGH_GPIO,
+};

so we register all four parts to the same primary IRQ. The same is true for the devicetree representation.

Nope, does not work like that.

Quoted 7 lines

It does not matter which IRQ of a GPIO chip fires. It looks at the IRQ cause bits for all lines and fires off the secondary ISR as needed for the whole chip. So in effect, there is a mapping IRQ->GPIO chip, not IRQ->1/4 of GPIO chip. With what you suggest above, you would end up with four chained interrupt handlers, all being handled by the same interrupt handler for one chio, and the last three in the chain would never do anything since the first one does all the work.

Does it really?

The handler function I'm looking at is

static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)

{
        int irqoff;
        BUG_ON(irq < IRQ_DOVE_GPIO_0_7 || irq > IRQ_DOVE_HIGH_GPIO);

        irqoff = irq <= IRQ_DOVE_GPIO_16_23 ? irq - IRQ_DOVE_GPIO_0_7 :
                3 + irq - IRQ_DOVE_GPIO_24_31;

        orion_gpio_irq_handler(irqoff << 3);
        if (irq == IRQ_DOVE_HIGH_GPIO) {
                orion_gpio_irq_handler(40);
                orion_gpio_irq_handler(48);
                orion_gpio_irq_handler(56);
        }
}

My reading of this is a manual hardwired implementation of a primary handler that triggers the secondary handler four times when it's called with a specific argument.

If you want to keep that behavior, this handler cannot be generic across all mvebu socs, whereas registering four chained handlers for the same primary interrupt would have the same effect at a very small runtime overhead without the need for any special case.

-----Original Message-----
From: devicetree-discuss [mailto:devicetree-discuss- bounces+dvomlehn=cisco.com at lists.ozlabs.org] On Behalf Of Mitch Bradley
Sent: Friday, July 06, 2012 5:25 PM
To: linux-arm-kernel at lists.infradead.org; devicetree- discuss at lists.ozlabs.org
Subject: Where to put a large bootloader-supplied device tree on ARM ?

I'm passing a flatted device tree from OLPC's bootloader (which is a full Open Firmware implementation) to the kernel. If I put the FDT at the "traditional" address 0x100, bad things happen when the DT is larger than 16K. The FDT extends past the 0x4000 boundary and gets overwritten by the early page tables which are written at that address.

I have found various places in memory where I can place it without it being overwritten, and things work, but that seems dodgy and likely to break randomly.

Has there been any discussion about where large-ish FDT images should be located?

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss at lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

The kernel *must* go where it is linked, but the FDT contains only relative references and is thus free to go anywhere. The same is true of ramdisks, which are usually placed after the kernel.

How about doing the same with your FDT?

Quoted 26 lines

-----Original Message-----
From: devicetree-discuss [mailto:devicetree-discuss- bounces+dvomlehn=cisco.com at lists.ozlabs.org] On Behalf Of Mitch Bradley
Sent: Friday, July 06, 2012 5:25 PM
To: linux-arm-kernel at lists.infradead.org; devicetree- discuss at lists.ozlabs.org
Subject: Where to put a large bootloader-supplied device tree on ARM ?

I'm passing a flatted device tree from OLPC's bootloader (which is a full Open Firmware implementation) to the kernel. If I put the FDT at the "traditional" address 0x100, bad things happen when the DT is larger than 16K. The FDT extends past the 0x4000 boundary and gets overwritten by the early page tables which are written at that address.

I have found various places in memory where I can place it without it being overwritten, and things work, but that seems dodgy and likely to break randomly.

Has there been any discussion about where large-ish FDT images should be located?

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss at lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Quoted 4 lines · David VomLehn (dvomlehn)

The kernel *must* go where it is linked, but the FDT contains only relative references and is thus free to go anywhere. The same is true of ramdisks, which
are usually placed after the kernel.

Right, but the kernel image is compressed, so after decompression it expands into the area just after it. Also, the .bss segment is in that vicinity.

There's some code in arch/arm/boot/compressed/head.S to relocate device tree blobs, but it requires CONFIG_ARM_APPENDED_DTB which is not recommended - arch/arm/Kconfig recommends using the documented boot protocol istead .

Documentation/arm/Booting says
to put the dtb "in a region of memory where the kernel decompressor will not overwrite it", further recommending the first 16KiB.

As noted, the first 16KiB loses if the dtb is too large. And "where the kernel decompressor will not overwrite it" says what won't work, not what will. It appears that the decompressor works out its addresses dynamically, so there's no hard prescription even for what to avoid.

For now, I'm putting the initrd at the end of memory and the dtb below that. That seems to work, but I'm unsure whether or not I'm just "getting lucky".

Quoted 5 lines · Mitch Bradley

Quoted 4 lines · David VomLehn (dvomlehn)

The kernel *must* go where it is linked, but the FDT contains only relative references and is thus free to go anywhere. The same is true of ramdisks, which
are usually placed after the kernel.

The kernel must go where it is linked *only* if you are using the 'Image' output. When using 'zImage' you can put the kernel anywhere in memory, or in the first 128MB of RAM if CONFIG_AUTO_ZRELADDR is used.

Quoted 2 lines

Right, but the kernel image is compressed, so after decompression it expands into the area just after it. Also, the .bss segment is in that vicinity.

To be exact, the compressed kernel moves itself out of the region where the decompressed kernel will end up before doing the decompression, but only if necessary. So it is a good idea to load zImage away from the decompressed kernel area to avoid this extra move and save some fraction of a second on boot time.

Quoted 4 lines

There's some code in arch/arm/boot/compressed/head.S to relocate device tree blobs, but it requires CONFIG_ARM_APPENDED_DTB which is not recommended - arch/arm/Kconfig recommends using the documented boot protocol istead .

This is in case a DTB is appended to zImage. When the DTB is detected, the moving of zImage out of the decompressed area must take care of moving the DTB as well.

Quoted 9 lines

Documentation/arm/Booting says
to put the dtb "in a region of memory where the kernel decompressor will not overwrite it", further recommending the first 16KiB.

As noted, the first 16KiB loses if the dtb is too large. And "where the kernel decompressor will not overwrite it" says what won't work, not what will. It appears that the decompressor works out its addresses dynamically, so there's no hard prescription even for what to avoid.

A good rule of thumb is to take the size of the decompressed kernel and multiply this by 3. Rounding up is also fine. So for example if your arch/arm/boot/Image is 5MB, then putting anciliary data such as a ramdisk or a large DTB from 16MB into RAM or above should be fine.

Quoted 3 lines

For now, I'm putting the initrd at the end of memory and the dtb below that. That seems to work, but I'm unsure whether or not I'm just "getting lucky".

That's also perfectly fine.

Nicolas

Quoted 6 lines · Nicolas Pitre

Quoted 3 lines · Mitch Bradley

For now, I'm putting the initrd at the end of memory and the dtb below that. That seems to work, but I'm unsure whether or not I'm just "getting lucky".

Quoted 1 line

That's also perfectly fine.

Alas, that worked for machines with 512 MiB of main memory, but failed on 1 GiB machines. My guess is that, when the initrd and dtb are near the top of a 1 GiB memory, the virtual address gets too near the top of the kernel's 1 GiB of virtual space (which starts at 0xc0000000), perhaps colliding with the VMALLOC space.

Putting them just below the 128 MiB boundary seems to work.

Quoted 45 lines · Nicolas Pitre

Quoted 5 lines · Mitch Bradley

Quoted 4 lines · David VomLehn (dvomlehn)

The kernel *must* go where it is linked, but the FDT contains only relative references and is thus free to go anywhere. The same is true of ramdisks, which
are usually placed after the kernel.

The kernel must go where it is linked *only* if you are using the 'Image' output. When using 'zImage' you can put the kernel anywhere in memory, or in the first 128MB of RAM if CONFIG_AUTO_ZRELADDR is used.

Quoted 2 lines

Right, but the kernel image is compressed, so after decompression it expands into the area just after it. Also, the .bss segment is in that vicinity.

To be exact, the compressed kernel moves itself out of the region where the decompressed kernel will end up before doing the decompression, but only if necessary. So it is a good idea to load zImage away from the decompressed kernel area to avoid this extra move and save some fraction of a second on boot time.

Quoted 4 lines

There's some code in arch/arm/boot/compressed/head.S to relocate device tree blobs, but it requires CONFIG_ARM_APPENDED_DTB which is not recommended - arch/arm/Kconfig recommends using the documented boot protocol istead .

This is in case a DTB is appended to zImage. When the DTB is detected, the moving of zImage out of the decompressed area must take care of moving the DTB as well.

Quoted 9 lines

Documentation/arm/Booting says
to put the dtb "in a region of memory where the kernel decompressor will not overwrite it", further recommending the first 16KiB.

As noted, the first 16KiB loses if the dtb is too large. And "where the kernel decompressor will not overwrite it" says what won't work, not what will. It appears that the decompressor works out its addresses dynamically, so there's no hard prescription even for what to avoid.

A good rule of thumb is to take the size of the decompressed kernel and multiply this by 3. Rounding up is also fine. So for example if your arch/arm/boot/Image is 5MB, then putting anciliary data such as a ramdisk or a large DTB from 16MB into RAM or above should be fine.

Quoted 5 lines

Quoted 3 lines

For now, I'm putting the initrd at the end of memory and the dtb below that. That seems to work, but I'm unsure whether or not I'm just "getting lucky".

That's also perfectly fine.

Alas, that worked for machines with 512 MiB of main memory, but failed on 1 GiB machines. My guess is that, when the initrd and dtb are near the top of a 1 GiB memory, the virtual address gets too near the top of the kernel's 1 GiB of virtual space (which starts at 0xc0000000), perhaps colliding with the VMALLOC space.

Putting them just below the 128 MiB boundary seems to work.

Quoted 5 lines

Nicolas

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss at lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

[adding u-boot list]

Quoted 59 lines · Mitch Bradley

Quoted 45 lines · Nicolas Pitre

Quoted 5 lines · Mitch Bradley

Quoted 4 lines · David VomLehn (dvomlehn)

The kernel *must* go where it is linked, but the FDT contains only relative references and is thus free to go anywhere. The same is true of ramdisks, which
are usually placed after the kernel.

The kernel must go where it is linked *only* if you are using the 'Image' output. When using 'zImage' you can put the kernel anywhere in memory, or in the first 128MB of RAM if CONFIG_AUTO_ZRELADDR is used.

Quoted 2 lines

Right, but the kernel image is compressed, so after decompression it expands into the area just after it. Also, the .bss segment is in that vicinity.

To be exact, the compressed kernel moves itself out of the region where the decompressed kernel will end up before doing the decompression, but only if necessary. So it is a good idea to load zImage away from the decompressed kernel area to avoid this extra move and save some fraction of a second on boot time.

Quoted 4 lines

There's some code in arch/arm/boot/compressed/head.S to relocate device tree blobs, but it requires CONFIG_ARM_APPENDED_DTB which is not recommended - arch/arm/Kconfig recommends using the documented boot protocol istead .

This is in case a DTB is appended to zImage. When the DTB is detected, the moving of zImage out of the decompressed area must take care of moving the DTB as well.

Quoted 9 lines

Documentation/arm/Booting says
to put the dtb "in a region of memory where the kernel decompressor will not overwrite it", further recommending the first 16KiB.

As noted, the first 16KiB loses if the dtb is too large. And "where the kernel decompressor will not overwrite it" says what won't work, not what will. It appears that the decompressor works out its addresses dynamically, so there's no hard prescription even for what to avoid.

A good rule of thumb is to take the size of the decompressed kernel and multiply this by 3. Rounding up is also fine. So for example if your arch/arm/boot/Image is 5MB, then putting anciliary data such as a ramdisk or a large DTB from 16MB into RAM or above should be fine.

Quoted 5 lines

Quoted 3 lines

For now, I'm putting the initrd at the end of memory and the dtb below that. That seems to work, but I'm unsure whether or not I'm just "getting lucky".

That's also perfectly fine.

Alas, that worked for machines with 512 MiB of main memory, but failed on 1 GiB machines. My guess is that, when the initrd and dtb are near the top of a 1 GiB memory, the virtual address gets too near the top of the kernel's 1 GiB of virtual space (which starts at 0xc0000000), perhaps colliding with the VMALLOC space.

Quoted 1 line

Putting them just below the 128 MiB boundary seems to work.

Interesting. I think this is also a problem on u-boot just waiting to happen. u-boot locates itself at the end of RAM and likes to copy the fdt and initrd to just below that. Any machine with 1G+ is going to hit this. I avoided it because I limited u-boot to 512MB on highbank.

If I'm not mistaken, yes U-Boot loads itself as high as it can, and I don't know about the FDT, but no, U-Boot does not "like" to load initrd "just below that": it loads initrd where the boot commands tell it to, and the boot commands are written by board developers. Nothing in U-Boot forces initrd to be loaded as high as possible.

That leaves the question of the FDT, though -- I'm not familiar enough with it (yet) to tell if it is always located just under U-Boot or if its placement is controllable by board commands.

Dear Albert ARIBAUD,

Quoted 6 lines · In message <20120712233801.0411daa7@lilith> you wrote:

If I'm not mistaken, yes U-Boot loads itself as high as it can, and I don't know about the FDT, but no, U-Boot does not "like" to load initrd "just below that": it loads initrd where the boot commands tell it to, and the boot commands are written by board developers. Nothing in U-Boot forces initrd to be loaded as high as possible.

Quoted 4 lines

That leaves the question of the FDT, though -- I'm not familiar enough with it (yet) to tell if it is always located just under U-Boot or if its placement is controllable by board commands.

You can always just set the "fdt_high" and "initrd_high" evironment variables to restrict the positioning of initrd and FDT in RAM; see the README for details.

Quoted 9 lines · Wolfgang Denk

Dear Albert ARIBAUD,

Quoted 6 lines · In message <20120712233801.0411daa7@lilith> you wrote:

If I'm not mistaken, yes U-Boot loads itself as high as it can, and I don't know about the FDT, but no, U-Boot does not "like" to load initrd "just below that": it loads initrd where the boot commands tell it to, and the boot commands are written by board developers. Nothing in U-Boot forces initrd to be loaded as high as possible.

u-boot loads the initrd where you tell it, then bootm relocates it for some reason.

Quoted 8 lines

Quoted 4 lines

That leaves the question of the FDT, though -- I'm not familiar enough with it (yet) to tell if it is always located just under U-Boot or if its placement is controllable by board commands.

You can always just set the "fdt_high" and "initrd_high" evironment variables to restrict the positioning of initrd and FDT in RAM; see the README for details.

Yes, I'm aware of all this, but this is not the default behavior and the default behavior will not work in this case. It also has another bug related to the relocation that I fixed:

http://www.mail-archive.com/u-boot at lists.denx.de/msg86475.html

Rob

Quoted 14 lines · Sebastian Hesselbarth

Andrew,

is it possible to group all gpio banks into one DT description? For mach-dove it could be something like:

gpio: gpio-controller {
         compatible = "marvell, orion-gpio";
         ...

         bank0 at d0400 {
                 reg = <0xd0400 0x40>;
                 ngpio = <8>;
                 mask-offset = <0>;
                 interrupts = <12>;

Quoted 8 lines

         };

         bank1 at d0400 {
                 reg = <0xd0400 0x40>;
                 ngpio = <8>;
                 mask-offset = <8>;
                 interrupts = <13>;
         };

This way you have multiple nodes with the same register and different names, which is not how it normally works.

Quoted 5 lines

This would have the advantage that DT describes gpio-to-irq dependencies. Moreover, nodes that reference gpios can do gpios = <&gpio 71 0>; instead of gpios = <&gpio3 7 0>;

Is that desired?

The device tree representation should match what is in the data sheet normally. If they are in a single continuous number range, then we should probably have a single device node with multiple register ranges rather than one device node for each 32-bit register. Looking at arch/arm/plat-orion/gpio.c I think that is not actually the case though and having separate banks is more logical.

Something completely different I just noticed in the original patch:

Quoted 7 lines

arch/arm/mach-dove/irq.c +3 -0

@@ -90,6 +74,27 @@ static void pmu_irq_handler(unsigned int irq, struct irq_desc *desc)
       }
  }
 
+static int __initdata gpio0_irqs[4] = {
+     IRQ_DOVE_GPIO_0_7,
+     IRQ_DOVE_GPIO_8_15,

arch/arm/mach-dove/irq.c static void pmu_irq_handler(unsigned int irq, struct irq_desc *desc) · line 89

Quoted 10 lines

arch/arm/mach-dove/irq.c +10 -0

+     IRQ_DOVE_GPIO_16_23,
+     IRQ_DOVE_GPIO_24_31,
+};
+
+static int __initdata gpio1_irqs[4] = {
+     IRQ_DOVE_HIGH_GPIO,
+     0,
+     0,
+     0,
+};

I think the latter one needs to be

arch/arm/mach-dove/irq.c +6 -0

+static int __initdata gpio1_irqs[4] = {
+     IRQ_DOVE_HIGH_GPIO,
+     IRQ_DOVE_HIGH_GPIO,
+     IRQ_DOVE_HIGH_GPIO,
+     IRQ_DOVE_HIGH_GPIO,
+};

so we register all four parts to the same primary IRQ. The same is true for the devicetree representation.

Arnd

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss at lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Extends the driver to get properties from device tree. Also extend the kirkwood DT support to supply the needed properties.

Signed-off-by Andrew Lunn <[email protected]>

---

drivers/i2c/busses/i2c-mv64xxx.c +22 -4

 	drv_data = kzalloc(sizeof(struct mv64xxx_i2c_data), GFP_KERNEL);

@@ -546,19 +549,35 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 	init_waitqueue_head(&drv_data->waitq);
 	spin_lock_init(&drv_data->lock);
 
-	drv_data->freq_m = pdata->freq_m;
-	drv_data->freq_n = pdata->freq_n;
-	drv_data->irq = platform_get_irq(pd, 0);
+	if (pd->dev.of_node) {
+		of_property_read_u32(pd->dev.of_node, "frequency-m",
+				     &drv_data->freq_m);
+		of_property_read_u32(pd->dev.of_node, "frequency-n",
+				     &drv_data->freq_n);
+		drv_data->irq = irq_of_parse_and_map(pd->dev.of_node, 0);
+	} else {
+		drv_data->freq_m = pdata->freq_m;
+		drv_data->freq_n = pdata->freq_n;
+		drv_data->irq = platform_get_irq(pd, 0);
+	}
+
 	if (drv_data->irq < 0) {
 		rc = -ENXIO;
 		goto exit_unmap_regs;
 	}
+
 	drv_data->adapter.dev.parent = &pd->dev;
 	drv_data->adapter.algo = &mv64xxx_i2c_algo;
 	drv_data->adapter.owner = THIS_MODULE;
 	drv_data->adapter.class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
-	drv_data->adapter.timeout = msecs_to_jiffies(pdata->timeout);
+	if (pd->dev.of_node)
+		drv_data->adapter.timeout = msecs_to_jiffies(
+			of_property_read_u32(pd->dev.of_node, "timeout-ms",
+					     &drv_data->freq_n));
+	else
+		drv_data->adapter.timeout = msecs_to_jiffies(pdata->timeout);
 	drv_data->adapter.nr = pd->id;
+	drv_data->adapter.dev.of_node = pd->dev.of_node;
 	platform_set_drvdata(pd, drv_data);
 	i2c_set_adapdata(&drv_data->adapter, drv_data);
 

@@ -577,6 +596,8 @@ mv64xxx_i2c_probe(struct platform_device *pd)
 		goto exit_free_irq;
 	}
 
+	of_i2c_register_devices(&drv_data->adapter);
+
 	return 0;
 
 	exit_free_irq:

drivers/i2c/busses/i2c-mv64xxx.c +6 -0

+	{ .compatible = "marvell,mv64xxx-i2c", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, mv64xxx_i2c_of_match_table);
+
 static struct platform_driver mv64xxx_i2c_driver = {
 	.probe	= mv64xxx_i2c_probe,
 	.remove	= __devexit_p(mv64xxx_i2c_remove),
 	.driver	= {
 		.owner	= THIS_MODULE,
 		.name	= MV64XXX_I2C_CTLR_NAME,
+		.of_match_table = of_match_ptr(mv64xxx_i2c_of_match_table),
 	},
 };
 

Signature · 7 lines

1.7.10


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hello Andrew,

Quoted 5 lines · On Tuesday 03 July 2012 16:22:40 Andrew Lunn wrote:

Extends the driver to get properties from device tree. Also extend the kirkwood DT support to supply the needed properties.

Signed-off-by Andrew Lunn <[email protected]>

---

[snip]

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_probe(struct platform_device *pd) · line 535

Quoted 3 lines

drivers/i2c/busses/i2c-mv64xxx.c +1 -1

-	if ((pd->id != 0) || !pdata)
+	if ((!pdata && !pd->dev.of_node) || (pdata && (pd->id != 0)))
 		return -ENODEV;

This is more a reminder than a real remark, but the driver here should not be limiting us to a single platform_device. In fact kirkwood55 (88F6282) has two I2C controllers for instance.

Quoted 6 lines · Florian Fainelli

Hello Andrew,

Quoted 3 lines · On Tuesday 03 July 2012 16:22:40 Andrew Lunn wrote:

Extends the driver to get properties from device tree. Also extend the kirkwood DT support to supply the needed properties.

Quoted 8 lines

Quoted 2 lines

Signed-off-by Andrew Lunn <[email protected]>

---

[snip]

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_probe(struct platform_device *pd) · line 535

Quoted 3 lines

drivers/i2c/busses/i2c-mv64xxx.c +1 -1

-	if ((pd->id != 0) || !pdata)
+	if ((!pdata && !pd->dev.of_node) || (pdata && (pd->id != 0)))
 		return -ENODEV;

This is more a reminder than a real remark, but the driver here should not

Quoted 1 line

limiting us to a single platform_device. In fact kirkwood55 (88F6282) has

Quoted 1 line

I2C controllers for instance.

Yes, i don't understand this code. It looks impossible to use it using platform_data with more than one controller. Any idea why its like this? I didn't want to change the behavior because i don't understand why its like this.

However, it should be possible to instantiate multiple I2C controllers using DT. However, i've only tested it with one.

Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hello Andrew,

Quoted 7 lines · On Tuesday 03 July 2012 18:58:39 Andrew Lunn wrote:

Quoted 6 lines · Florian Fainelli

Hello Andrew,

Quoted 3 lines · On Tuesday 03 July 2012 16:22:40 Andrew Lunn wrote:

Extends the driver to get properties from device tree. Also extend the kirkwood DT support to supply the needed properties.

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

Signed-off-by Andrew Lunn <[email protected]>

---

[snip]

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_i2c_probe(struct platform_device *pd) · line 535

Quoted 3 lines

drivers/i2c/busses/i2c-mv64xxx.c +1 -1

-	if ((pd->id != 0) || !pdata)
+	if ((!pdata && !pd->dev.of_node) || (pdata && (pd->id != 0)))
 		return -ENODEV;

This is more a reminder than a real remark, but the driver here should not

be

Quoted 1 line

Quoted 1 line

limiting us to a single platform_device. In fact kirkwood55 (88F6282) has

two

Quoted 6 lines

Quoted 1 line

I2C controllers for instance.

Yes, i don't understand this code. It looks impossible to use it using platform_data with more than one controller. Any idea why its like this? I didn't want to change the behavior because i don't understand why its like this.

No idea, we just stumbled over this with a colleague the other day, and just removing the check made our second I2C controller work, so I don't see any reason why there is such a limitation. That said, this is for a subsequent patch.

The two different variants of QNAP TS devices, varying by SoC, put the GPIO keys on different GPIO lines. Hence we need two different DT board descriptions, which share the same board-ts219.c file.

Signed-off-by Andrew Lunn <[email protected]>

Acked-by Jason Cooper <[email protected]>

---

arch/arm/mach-kirkwood/Kconfig +4 -0

+	select ARCH_KIRKWOOD_DT
+	select ARM_APPENDED_DTB
+	select ARM_ATAG_DTB_COMPAT
+	help

index 9248fa2..e0c2324 100644

arch/arm/mach-kirkwood/common.h +5 -0

@@ -58,6 +58,11 @@ void dreamplug_init(void);
 #else
 static inline void dreamplug_init(void) {};
 #endif
+#ifdef CONFIG_MACH_TS219_DT
+void qnap_dt_ts219_init(void);
+#else
+static inline void qnap_dt_ts219_init(void) {};
+#endif
 
 #ifdef CONFIG_MACH_DLINK_KIRKWOOD_DT
 void dnskw_init(void);

Signature · 7 lines

1.7.10


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hi Andrew,

Quoted 3 lines · On Tuesday 03 July 2012 16:22:41 Andrew Lunn wrote:

The two different variants of QNAP TS devices, varying by SoC, put the GPIO keys on different GPIO lines. Hence we need two different DT board descriptions, which share the same board-ts219.c file.

Why don't you reorder this patch to be after patch 11/12 so that you do not need to update this file in patch 11/12?

Quoted 5 lines

Signed-off-by Andrew Lunn <[email protected]>

Acked-by Jason Cooper <[email protected]>

---

[snip]

Review

Quoted 3 lines

(quoted diff) +2 -0

+config MACH_TS219_DT
+	bool "Device Tree for QNAP TS-110, TS-119, TS-119P+, TS-210, TS-219, 

TS-219P and TS-219P+ Turbo NAS"

This looks too long, how about just "Device Tree for QNAP TS-11x, TS-21x NAS" for instance?