| Message ID | 1341325365-21393-2-git-send-email-andrew@lunn.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Rob, Arnd
It would be good if you could ACK, NACK, make comments on this patch.
Its the key patch which most other Orion DT patches build on, and i
would like to get this moving upstream, so allowing more DT conversion
for all Orion platforms.
Thanks
Andrew
On Tue, Jul 03, 2012 at 04:22:34PM +0200, Andrew Lunn wrote:
> 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 <andrew@lunn.ch>
> ---
> .../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(-)
>
> diff --git a/Documentation/devicetree/bindings/arm/mrvl/intc.txt b/Documentation/devicetree/bindings/arm/mrvl/intc.txt
> index 80b9a94..8927e10 100644
> --- a/Documentation/devicetree/bindings/arm/mrvl/intc.txt
> +++ b/Documentation/devicetree/bindings/arm/mrvl/intc.txt
> @@ -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>;
> + };
> diff --git a/Documentation/devicetree/bindings/gpio/mrvl-gpio.txt b/Documentation/devicetree/bindings/gpio/mrvl-gpio.txt
> index 05428f3..e137874 100644
> --- a/Documentation/devicetree/bindings/gpio/mrvl-gpio.txt
> +++ b/Documentation/devicetree/bindings/gpio/mrvl-gpio.txt
> @@ -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@10100 {
> + compatible = "marvell,orion-gpio";
> + #gpio-cells = <2>;
> + gpio-controller;
> + reg = <0x10100 0x40>;
> + ngpio = <32>;
> + interrupts = <35>, <36>, <37>, <38>;
> + };
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index a91009c..39bb941 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1105,6 +1105,7 @@ config PLAT_ORION
> bool
> select CLKSRC_MMIO
> select GENERIC_IRQ_CHIP
> + select IRQ_DOMAIN
> select COMMON_CLK
>
> config PLAT_PXA
> diff --git a/arch/arm/boot/dts/kirkwood.dtsi b/arch/arm/boot/dts/kirkwood.dtsi
> index 926528b..66544d7 100644
> --- a/arch/arm/boot/dts/kirkwood.dtsi
> +++ b/arch/arm/boot/dts/kirkwood.dtsi
> @@ -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@f1000000 {
> compatible = "simple-bus";
> @@ -9,6 +18,24 @@
> #address-cells = <1>;
> #size-cells = <1>;
>
> + gpio0: gpio@10100 {
> + compatible = "marvell,orion-gpio";
> + #gpio-cells = <2>;
> + gpio-controller;
> + reg = <0x10100 0x40>;
> + ngpio = <32>;
> + interrupts = <35>, <36>, <37>, <38>;
> + };
> +
> + gpio1: gpio@10140 {
> + compatible = "marvell,orion-gpio";
> + #gpio-cells = <2>;
> + gpio-controller;
> + reg = <0x10140 0x40>;
> + ngpio = <18>;
> + interrupts = <39>, <40>, <41>;
> + };
> +
> serial@12000 {
> compatible = "ns16550a";
> reg = <0x12000 0x100>;
> diff --git a/arch/arm/mach-dove/irq.c b/arch/arm/mach-dove/irq.c
> index f07fd16..9bc97a5 100644
> --- a/arch/arm/mach-dove/irq.c
> +++ b/arch/arm/mach-dove/irq.c
> @@ -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
> diff --git a/arch/arm/mach-kirkwood/board-dt.c b/arch/arm/mach-kirkwood/board-dt.c
> index edc3f8a..27ac3d8 100644
> --- a/arch/arm/mach-kirkwood/board-dt.c
> +++ b/arch/arm/mach-kirkwood/board-dt.c
> @@ -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 <jason@lakedaemon.net> */
> .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,
> diff --git a/arch/arm/mach-kirkwood/irq.c b/arch/arm/mach-kirkwood/irq.c
> index c4c68e5..720063f 100644
> --- a/arch/arm/mach-kirkwood/irq.c
> +++ b/arch/arm/mach-kirkwood/irq.c
> @@ -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);
> }
> diff --git a/arch/arm/mach-mv78xx0/irq.c b/arch/arm/mach-mv78xx0/irq.c
> index e421b70..eff9a75 100644
> --- a/arch/arm/mach-mv78xx0/irq.c
> +++ b/arch/arm/mach-mv78xx0/irq.c
> @@ -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);
> }
> diff --git a/arch/arm/mach-orion5x/irq.c b/arch/arm/mach-orion5x/irq.c
> index b1b45ff..17da709 100644
> --- a/arch/arm/mach-orion5x/irq.c
> +++ b/arch/arm/mach-orion5x/irq.c
> @@ -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);
> }
> diff --git a/arch/arm/plat-orion/gpio.c b/arch/arm/plat-orion/gpio.c
> index af95af2..dfda74f 100644
> --- a/arch/arm/plat-orion/gpio.c
> +++ b/arch/arm/plat-orion/gpio.c
> @@ -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
> diff --git a/arch/arm/plat-orion/include/plat/gpio.h b/arch/arm/plat-orion/include/plat/gpio.h
> index bec0c98..81c6fc8 100644
> --- a/arch/arm/plat-orion/include/plat/gpio.h
> +++ b/arch/arm/plat-orion/include/plat/gpio.h
> @@ -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
> diff --git a/arch/arm/plat-orion/include/plat/irq.h b/arch/arm/plat-orion/include/plat/irq.h
> index f05eeab..50547e4 100644
> --- a/arch/arm/plat-orion/include/plat/irq.h
> +++ b/arch/arm/plat-orion/include/plat/irq.h
> @@ -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
> diff --git a/arch/arm/plat-orion/irq.c b/arch/arm/plat-orion/irq.c
> index 2d5b9c1..d751964 100644
> --- a/arch/arm/plat-orion/irq.c
> +++ b/arch/arm/plat-orion/irq.c
> @@ -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
> --
> 1.7.10
>
Hello Andrew, Le Tue, 3 Jul 2012 16:22:34 +0200, Andrew Lunn <andrew@lunn.ch> a écrit : > 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 <andrew@lunn.ch> 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? Thanks, Thomas
On Thu, Jul 05, 2012 at 11:02:51AM +0200, Thomas Petazzoni wrote: > Hello Andrew, > > Le Tue, 3 Jul 2012 16:22:34 +0200, > Andrew Lunn <andrew@lunn.ch> a ??crit : > > > 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 <andrew@lunn.ch> > > 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. > 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. > 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. > 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. > 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. If however, you have to focus solely on Armada 370/XP, then the community can work on making your code generic at a late date. Andrew
Hello, Le Thu, 5 Jul 2012 11:48:24 +0200, Andrew Lunn <andrew@lunn.ch> a écrit : > Hi Thomas > > You are not the only one working in this area. Arnaud Patard said he > was look at this as well. Ok, I added Arnaud in the Cc list. Arnaud, what is the status of you work? > > 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. For the DT binding, I took my inspiration from the MXS and Samsung pinctrl drivers, as those platforms also have independent pin muxing (while some platforms such as Tegra, have hardware pin groups). The .dtsi side looks like: pinctrl@d0018000 { compatible = "marvell,mv78230-pinmux"; reg = <0xd0018000 0x38>; uart_2: uart-2 { marvell,pins = <42 43>; marvell,pins-function = <1>; }; uart_3: uart-3 { marvell,pins = <44 45>; marvell,pins-function = <2>; }; uart_0_control: uart-0-control { marvell,pins = <42 43>; marvell,pins-function = <2>; }; uart_1_control: uart-1-control { marvell,pins = <46 47>; marvell,pins-function = <2>; }; uart_2_control: uart-2-control { marvell,pins = <44 45>; marvell,pins-function = <1>; }; uart_3_control: uart-3-control { marvell,pins = <46 47>; marvell,pins-function = <1>; }; }; This defines the various pin muxing options, and then when instantiating a device, you do: test@12 { compatible = "marvell,testdrv"; pinctrl-names = "default"; pinctrl-0 = <&uart_0_control>; }; (This is with a dummy test driver that I'm using for testing). Then the driver can use the pinctrl API to get the proper muxing for its pins. > > 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. Ok, thanks for those hints. > > 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. Thanks, Thomas
Andrew Lunn <andrew@lunn.ch> writes: Hi, > On Thu, Jul 05, 2012 at 11:02:51AM +0200, Thomas Petazzoni wrote: >> Hello Andrew, >> >> Le Tue, 3 Jul 2012 16:22:34 +0200, >> Andrew Lunn <andrew@lunn.ch> a ??crit : >> >> > 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 <andrew@lunn.ch> >> >> 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. 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. > >> 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. > >> 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. > >> 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. > >> 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. agreed. it should be written with all platforms in mind, as long as they're compatible. It can only make things better imho. Arnaud
Le Thu, 05 Jul 2012 12:11:40 +0200, Arnaud Patard (Rtp) <arnaud.patard@rtp-net.org> a écrit : > > 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. 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. However, using the pinmux driver will require changes in the device drivers, so that they request the pinmux for their devices. > > 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. Certainly, especially for MPP/GPIO that are definitely very similar. Best regards, Thomas
> 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 number of banks varies. Orion5x has one, Dove has three, etc. > > * 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. That is good to hear. Thanks Andrew
Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes: > Le Thu, 05 Jul 2012 12:11:40 +0200, > Arnaud Patard (Rtp) <arnaud.patard@rtp-net.org> a écrit : > >> > 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. > > 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. > > 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 ? Arnaud
Le Thu, 05 Jul 2012 12:38:51 +0200, Arnaud Patard (Rtp) <arnaud.patard@rtp-net.org> a écrit : > > 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. Ok, this is a bit trickier, but we can probably do something for it as well. > > 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? Best regards, Thomas
Hi Thomas > 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. Andrew
On 07/05/2012 01:48 PM, Andrew Lunn wrote: >> 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 output-only/non-interrupt gpios could be marked as such in DT. The common code should not install irqs if marked by e.g. "marvell,gpio-no-irq". The gpio driver already fails to set input direction on output-only pins. This could be marked by e.g. "marvell,gpio-output-only" in DT. Sebastian
On Tuesday 03 July 2012, Andrew Lunn wrote: > 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. Overall looks very good. > diff --git a/Documentation/devicetree/bindings/arm/mrvl/intc.txt b/Documentation/devicetree/bindings/arm/mrvl/intc.txt > index 80b9a94..8927e10 100644 > --- a/Documentation/devicetree/bindings/arm/mrvl/intc.txt > +++ b/Documentation/devicetree/bindings/arm/mrvl/intc.txt > @@ -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. I think you should clarify that the "reg" property can be multiple 4-byte ranges, because that is fairly unusual. > +#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 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? Arnd
Hello, Le Thu, 5 Jul 2012 11:48:24 +0200, Andrew Lunn <andrew@lunn.ch> a écrit : > 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? Thanks! Thomas
> > +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. O.K. The example is already like this, but i can add some more words. > > +#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 > > 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. Andrew
On Thu, Jul 05, 2012 at 02:58:01PM +0200, Thomas Petazzoni wrote: > Hello, > > Le Thu, 5 Jul 2012 11:48:24 +0200, > Andrew Lunn <andrew@lunn.ch> a ??crit : > > > 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? 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? Andrew
Le Thu, 5 Jul 2012 15:15:22 +0200, Andrew Lunn <andrew@lunn.ch> a écrit : > > > 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? > > Basically you are asking, why should i use the framework when i can do > it by hand. Agreed :) > What are the advantages if ignoring the framework and doing it by > hand? Many GPIO drivers directly use irq_chip (though it's true some of them use irq_chip_generic), and the irq_chip_generic framework doesn't have really proper support for the other new irqdomain framework, so I was wondering what the best solution was. I found the proposal from Rob Herring to improve that situation, but apparently the conclusion from Grant Likely was "Let's discuss this at the next Connect". That patch set was posted in January, so two Linaro Connects happened since them, I don't know if progress has been made here. I guess I'll just stick to irq_chip_generic + irqdomain_legacy for now. Best regards, Thomas
> I guess I'll just stick to irq_chip_generic + irqdomain_legacy for now.
Hi Thomas
That is what my patch currently does. Feel free to re-use parts of the
orion code.
Andrew
Le Thu, 5 Jul 2012 15:33:10 +0200, Andrew Lunn <andrew@lunn.ch> a écrit : > That is what my patch currently does. Feel free to re-use parts of the > orion code. Yes, sure! I have applied your patch and read it, I definitely intend to re-use many parts of it. Thomas
On Thursday 05 July 2012, Andrew Lunn wrote: > > > > 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. 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. > 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. Yes, that makes sense. Using the linear domain should solve all these nicely. Arnd
> > 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. > > 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? Andrew
On 07/05/2012 03:08 PM, Andrew Lunn wrote: > 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? For mach-dove it could be something like: gpio: gpio-controller { compatible = "marvell, orion-gpio"; ... bank0@d0400 { reg = <0xd0400 0x40>; ngpio = <8>; mask-offset = <0>; interrupts = <12>; }; bank1@d0400 { reg = <0xd0400 0x40>; ngpio = <8>; mask-offset = <8>; interrupts = <13>; }; ... bank4@d0420 { reg = <0xd0420 0x40>; ngpio = <32>; interrupts = <61>; }; bank5@e8400 { reg = <0xe8400 0x20>; ngpio = <8>; marvell,orion-gpio-output-only; }; }; 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>; Sebastian
On Thu, Jul 05, 2012 at 04:14:50PM +0200, Sebastian Hesselbarth wrote: > On 07/05/2012 03:08 PM, Andrew Lunn wrote: > >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? Everything is possible. I did think about having just one gpio controller, since as you said, it makes it easier to describe gpios = <&gpio 71 0>; instead of gpios = <&gpio3 7 0>; But most SoCs seem to have multiple GPIO controllers in the .dtsi files. Only picoxcell has banks inside a single gpio controller. So it would be a bit unusual. What you suggest also adds a lot of complexity to the code and probably means a lot less of the code can be shared with non-DT GPIO handling code. The natural size of a GPIO controller is 32 lines, not 8. The current interrupt handling does not actually care which IRQ of a bank of 32 lines fired, it looks at all lines. The handler_data in the IRQ points to the GPIO chip, not a subsection of the GPIO chip. > bank5@e8400 { > reg = <0xe8400 0x20>; > ngpio = <8>; > marvell,orion-gpio-output-only; Some Orions mixed GPIOs GPOs and GPIs within one controller. So that is not generic enough. Probably the pinmux DT description needs to be able to specify per pin what a line can do. Andrew
On Thursday 05 July 2012, Sebastian Hesselbarth wrote: > 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@d0400 { > reg = <0xd0400 0x40>; > ngpio = <8>; > mask-offset = <0>; > interrupts = <12>; > }; > > bank1@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. > > 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: > @@ -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, > +}; I think the latter one needs to be +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
On Thursday 05 July 2012, Andrew Lunn wrote: > > > 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. > > > > 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? Probably not much. I've reread the of_irq_init code again and it seems that the effect would be the same, perhaps appearing a little more similar to what the other platforms do. Please add my Acked-by: to your patch. Arnd
On 07/05/2012 04:54 PM, Arnd Bergmann wrote: > This way you have multiple nodes with the same register > and different names, which is not how it normally works. Ok. >> 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. 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) gpio@d0400 { compatible = "marvell,orion-gpio"; gpio-controller; reg = <0xd0400 0x20>, /* GPIO[31: 0] */ <0xd0420 0x20>, /* GPIO[63:32] */ <0xe8400 0x0c>; /* GPO [71:64] */ ngpio = <72>; interrupts = <12 13 14 15>, <61>; }; Sebastian
> Something completely different I just noticed in the original patch: > > > @@ -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, > > +}; > > I think the latter one needs to be > > +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. Andrew
Sebastian Hesselbarth <sebastian.hesselbarth@googlemail.com> writes: > On 07/05/2012 04:54 PM, Arnd Bergmann wrote: >> This way you have multiple nodes with the same register >> and different names, which is not how it normally works. > > Ok. > >>> 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. > > 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) They're numbered as [63:0] and [71:64] but they're on 3 different banks. iirc, there may even be some differences with the way the banks are dealing interrupts, so I don't see any reason to not represent the 3 banks in DT. Arnaud
On 7/5/2012 4:54 AM, Arnd Bergmann wrote: > On Thursday 05 July 2012, Sebastian Hesselbarth wrote: >> 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@d0400 { >> reg = <0xd0400 0x40>; >> ngpio = <8>; >> mask-offset = <0>; >> interrupts = <12>; >> }; >> >> bank1@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. The "mask-offset" property is really a "reg" in disguise. "reg" is considerably more general than just "memory mapped register address". It really means "any numeric identifier that makes sense in the context of a parent device". Therefore, one could say: gpio: gpio-controller { compatible = "marvell, orion-gpio"; reg = <0xd0400 0x40>; #address-cells = <1>; #size-cells = <0>; ... bank0@0 { reg = <0x0>; ngpio = <8>; mask-offset = <0>; interrupts = <12>; }; bank1@8 { reg = <0x8>; ngpio = <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>; > > 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: > >> @@ -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, >> +}; > > I think the latter one needs to be > > +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@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/devicetree-discuss >
On Thursday 05 July 2012, Andrew Lunn wrote: > > I think the latter one needs to be > > > > +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. 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. Arnd
On Fri, Jul 06, 2012 at 08:08:23PM +0000, Arnd Bergmann wrote: > On Thursday 05 July 2012, Andrew Lunn wrote: > > > I think the latter one needs to be > > > > > > +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. > > 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); > } > } 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; 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); } } orion_gpio_chip_find(pinoff) when called with pinoff 32, 40, 48, or 56 will return the same gpio chip. The loop: for (i = 0; i < ochip->chip.ngpio; i++) { will iterate over all lines of the controller. > 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. Here is your mistake. It not a secondary handler. It is a function which might trigger a secondary handler, if the status bit requires that the secondary handler should be called.. > 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. I would say the current code does redundant stuff. This code has also been tested on a Dove by Sebastian Hesselbarth and he did not notice anything strange happening on his system. Andrew
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?
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? > -----Original Message----- > From: devicetree-discuss [mailto:devicetree-discuss- > bounces+dvomlehn=cisco.com@lists.ozlabs.org] On Behalf Of Mitch Bradley > Sent: Friday, July 06, 2012 5:25 PM > To: linux-arm-kernel@lists.infradead.org; devicetree- > discuss@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@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/devicetree-discuss
On 7/6/2012 3:23 PM, David VomLehn (dvomlehn) wrote: > 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". Thanks, Mitch > How about doing the same with your FDT? > >> -----Original Message----- >> From: devicetree-discuss [mailto:devicetree-discuss- >> bounces+dvomlehn=cisco.com@lists.ozlabs.org] On Behalf Of Mitch Bradley >> Sent: Friday, July 06, 2012 5:25 PM >> To: linux-arm-kernel@lists.infradead.org; devicetree- >> discuss@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@lists.ozlabs.org >> https://lists.ozlabs.org/listinfo/devicetree-discuss > >
On Fri, 6 Jul 2012, Mitch Bradley wrote: > On 7/6/2012 3:23 PM, David VomLehn (dvomlehn) wrote: > > 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. > 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. > 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. > 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. > 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
On 7/8/2012 6:30 PM, Nicolas Pitre wrote: > On Fri, 6 Jul 2012, Mitch Bradley wrote: > >> On 7/6/2012 3:23 PM, David VomLehn (dvomlehn) wrote: >>> 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. > >> 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. > >> 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. > >> 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. > >> 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. > > > Nicolas >
On Wed, 11 Jul 2012, Mitch Bradley wrote: > On 7/8/2012 6:30 PM, Nicolas Pitre wrote: > > On Fri, 6 Jul 2012, Mitch Bradley wrote: > > > >> 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. Indeed, that won't work in that case. > Putting them just below the 128 MiB boundary seems to work. Or at the 128MB mark, or any location between 64MB and 512MB should be fine in practice. Nicolas
[adding u-boot list] On 07/12/2012 01:52 AM, Mitch Bradley wrote: > On 7/8/2012 6:30 PM, Nicolas Pitre wrote: >> On Fri, 6 Jul 2012, Mitch Bradley wrote: >> >>> On 7/6/2012 3:23 PM, David VomLehn (dvomlehn) wrote: >>>> 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. >> >>> 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. >> >>> 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. >> >>> 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. >> >>> 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. 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. Rob > >> >> >> Nicolas >> > > _______________________________________________ > devicetree-discuss mailing list > devicetree-discuss@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/devicetree-discuss >
Hi Rob, On Thu, 12 Jul 2012 15:34:17 -0500, Rob Herring <robherring2@gmail.com> wrote: > [adding u-boot list] > > On 07/12/2012 01:52 AM, Mitch Bradley wrote: > > On 7/8/2012 6:30 PM, Nicolas Pitre wrote: > >> On Fri, 6 Jul 2012, Mitch Bradley wrote: > >> > >>> On 7/6/2012 3:23 PM, David VomLehn (dvomlehn) wrote: > >>>> 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. > >> > >>> 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. > >> > >>> 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. > >> > >>> 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. > >> > >>> 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. > > 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. Amicalement,
Dear Albert ARIBAUD, 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. > > 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. Best regards, Wolfgang Denk
On 07/12/2012 04:47 PM, Wolfgang Denk wrote: > Dear Albert ARIBAUD, > > 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. >> >> 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@lists.denx.de/msg86475.html Rob
Hi Rob, On Thu, 12 Jul 2012 20:28:22 -0500, Rob Herring <robherring2@gmail.com> wrote: > On 07/12/2012 04:47 PM, Wolfgang Denk wrote: > > Dear Albert ARIBAUD, > > > > 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. > >> > >> 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@lists.denx.de/msg86475.html > > Rob As for the default behavior, this becomes a board config issue, not a U-Boot issue. Regarding the patch, as a bugfix it will go in u-boot-arm/master for 2012.07. With all this, do we have the overall issue covered? Amicalement,
diff --git a/Documentation/devicetree/bindings/arm/mrvl/intc.txt b/Documentation/devicetree/bindings/arm/mrvl/intc.txt index 80b9a94..8927e10 100644 --- a/Documentation/devicetree/bindings/arm/mrvl/intc.txt +++ b/Documentation/devicetree/bindings/arm/mrvl/intc.txt @@ -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>; + }; diff --git a/Documentation/devicetree/bindings/gpio/mrvl-gpio.txt b/Documentation/devicetree/bindings/gpio/mrvl-gpio.txt index 05428f3..e137874 100644 --- a/Documentation/devicetree/bindings/gpio/mrvl-gpio.txt +++ b/Documentation/devicetree/bindings/gpio/mrvl-gpio.txt @@ -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@10100 { + compatible = "marvell,orion-gpio"; + #gpio-cells = <2>; + gpio-controller; + reg = <0x10100 0x40>; + ngpio = <32>; + interrupts = <35>, <36>, <37>, <38>; + }; diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index a91009c..39bb941 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -1105,6 +1105,7 @@ config PLAT_ORION bool select CLKSRC_MMIO select GENERIC_IRQ_CHIP + select IRQ_DOMAIN select COMMON_CLK config PLAT_PXA diff --git a/arch/arm/boot/dts/kirkwood.dtsi b/arch/arm/boot/dts/kirkwood.dtsi index 926528b..66544d7 100644 --- a/arch/arm/boot/dts/kirkwood.dtsi +++ b/arch/arm/boot/dts/kirkwood.dtsi @@ -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@f1000000 { compatible = "simple-bus"; @@ -9,6 +18,24 @@ #address-cells = <1>; #size-cells = <1>; + gpio0: gpio@10100 { + compatible = "marvell,orion-gpio"; + #gpio-cells = <2>; + gpio-controller; + reg = <0x10100 0x40>; + ngpio = <32>; + interrupts = <35>, <36>, <37>, <38>; + }; + + gpio1: gpio@10140 { + compatible = "marvell,orion-gpio"; + #gpio-cells = <2>; + gpio-controller; + reg = <0x10140 0x40>; + ngpio = <18>; + interrupts = <39>, <40>, <41>; + }; + serial@12000 { compatible = "ns16550a"; reg = <0x12000 0x100>; diff --git a/arch/arm/mach-dove/irq.c b/arch/arm/mach-dove/irq.c index f07fd16..9bc97a5 100644 --- a/arch/arm/mach-dove/irq.c +++ b/arch/arm/mach-dove/irq.c @@ -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 diff --git a/arch/arm/mach-kirkwood/board-dt.c b/arch/arm/mach-kirkwood/board-dt.c index edc3f8a..27ac3d8 100644 --- a/arch/arm/mach-kirkwood/board-dt.c +++ b/arch/arm/mach-kirkwood/board-dt.c @@ -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 <jason@lakedaemon.net> */ .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, diff --git a/arch/arm/mach-kirkwood/irq.c b/arch/arm/mach-kirkwood/irq.c index c4c68e5..720063f 100644 --- a/arch/arm/mach-kirkwood/irq.c +++ b/arch/arm/mach-kirkwood/irq.c @@ -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); } diff --git a/arch/arm/mach-mv78xx0/irq.c b/arch/arm/mach-mv78xx0/irq.c index e421b70..eff9a75 100644 --- a/arch/arm/mach-mv78xx0/irq.c +++ b/arch/arm/mach-mv78xx0/irq.c @@ -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); } diff --git a/arch/arm/mach-orion5x/irq.c b/arch/arm/mach-orion5x/irq.c index b1b45ff..17da709 100644 --- a/arch/arm/mach-orion5x/irq.c +++ b/arch/arm/mach-orion5x/irq.c @@ -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); } diff --git a/arch/arm/plat-orion/gpio.c b/arch/arm/plat-orion/gpio.c index af95af2..dfda74f 100644 --- a/arch/arm/plat-orion/gpio.c +++ b/arch/arm/plat-orion/gpio.c @@ -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 diff --git a/arch/arm/plat-orion/include/plat/gpio.h b/arch/arm/plat-orion/include/plat/gpio.h index bec0c98..81c6fc8 100644 --- a/arch/arm/plat-orion/include/plat/gpio.h +++ b/arch/arm/plat-orion/include/plat/gpio.h @@ -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 diff --git a/arch/arm/plat-orion/include/plat/irq.h b/arch/arm/plat-orion/include/plat/irq.h index f05eeab..50547e4 100644 --- a/arch/arm/plat-orion/include/plat/irq.h +++ b/arch/arm/plat-orion/include/plat/irq.h @@ -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 diff --git a/arch/arm/plat-orion/irq.c b/arch/arm/plat-orion/irq.c index 2d5b9c1..d751964 100644 --- a/arch/arm/plat-orion/irq.c +++ b/arch/arm/plat-orion/irq.c @@ -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
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 <andrew@lunn.ch> --- .../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(-)