Re: kirkwood devicetree respin

Re: kirkwood devicetree respin

[Arnd Bergmann]

On Monday 12 March 2012, Jason Cooper wrote:
> I haven't had a chance to push this series to the ml yet (maybe
> tonight), but I wanted to give Jamie something better to go off of.
> 
> Only issue I have (I think unrelated) is that orion-ehci isn't coming
> up.  But these patches don't touch that, so I think it might be my
> config again.  Although it doesn't look like it...

I looked at your patches but also couldn't find anything that hints
at why ehci would break.

> Also, serial comes up, works without earlyprintk, but says irq = 0.
> Could be due to the way I setup serial in dtsi/dts.

Well, in this series you don't have any interrupt-controller node
in the device tree, so the interrupt number lookup fails for
any irq you put into the device tree.

I wanted to understand how this works for myself, so I've tried
implementing it in the patch below. This probably doesn't work
right away, but it should give you an idea of what needs to
be done.

It is based on the latest irqdomain series from
git://git.secretlab.ca/git/linux-2.6.git irqdomain/next
together with your own patches.

Grant, can you have a look at this? I'm trying to help out
multiple people right now who are converting a number of platforms
to devicetree. It would be good to know if I'm giving the right
recommendations.

Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>

diff --git a/arch/arm/boot/dts/kirkwood.dtsi b/arch/arm/boot/dts/kirkwood.dtsi
index 3474ef8..762f8e2 100644
--- a/arch/arm/boot/dts/kirkwood.dtsi
+++ b/arch/arm/boot/dts/kirkwood.dtsi
@@ -2,6 +2,7 @@
 
 / {
 	compatible = "mrvl,kirkwood";
+	interrupt-parent = <&irq>;
 
 	ocp@f1000000 {
 		compatible = "simple-bus";
@@ -9,6 +10,22 @@
 		#address-cells = <1>;
 		#size-cells = <1>;
 
+		irq: irq@f1020204 {
+			compatible = "mrvl,kirkwood-irq", "mrvl,orion-irq";
+			interrupt-controller;
+			#interrupt-cells = <1>;
+			reg = <0x20204 0x4 0x14>;
+		};
+
+		gpio: gpio@f1010100 {
+			compatible = "mrvl,kirkwood-gpio", "mrvl,orion-gpio";
+			gpio-controller;
+			interrupt-controller;
+			#interrupt-cells = <1>;
+			#gpio-cells = <1>;
+			reg = <0x10100 0x40 0x10140 0x40>;
+		};
+
 		serial@12000 {
 			compatible = "ns16550a";
 			reg = <0x12000 0x100>;
diff --git a/arch/arm/mach-kirkwood/board-dt.c b/arch/arm/mach-kirkwood/board-dt.c
index 0819240..67a356c 100644
--- a/arch/arm/mach-kirkwood/board-dt.c
+++ b/arch/arm/mach-kirkwood/board-dt.c
@@ -16,6 +16,7 @@
 #include <linux/of_platform.h>
 #include <asm/mach/arch.h>
 #include <asm/mach/map.h>
+#include <plat/irq.h>
 #include <mach/bridge-regs.h>
 #include "common.h"
 
@@ -67,7 +68,7 @@ DT_MACHINE_START(KIRKWOOD_DT, "Marvell Kirkwood (Flattened Device Tree)")
 	/* Maintainer: Jason Cooper <jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org> */
 	.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..a0e13d2 100644
--- a/arch/arm/mach-kirkwood/irq.c
+++ b/arch/arm/mach-kirkwood/irq.c
@@ -46,3 +46,4 @@ void __init kirkwood_init_irq(void)
 	irq_set_chained_handler(IRQ_KIRKWOOD_GPIO_HIGH_16_23,
 				gpio_irq_handler);
 }
+
diff --git a/arch/arm/plat-orion/include/plat/irq.h b/arch/arm/plat-orion/include/plat/irq.h
index f05eeab..dc83270 100644
--- a/arch/arm/plat-orion/include/plat/irq.h
+++ b/arch/arm/plat-orion/include/plat/irq.h
@@ -13,5 +13,6 @@
 
 void orion_irq_init(unsigned int irq_start, void __iomem *maskaddr);
 
+void orion_dt_init_irq(void);
 
 #endif
diff --git a/arch/arm/plat-orion/irq.c b/arch/arm/plat-orion/irq.c
index 2d5b9c1..2e8e5db 100644
--- a/arch/arm/plat-orion/irq.c
+++ b/arch/arm/plat-orion/irq.c
@@ -12,7 +12,12 @@
 #include <linux/init.h>
 #include <linux/irq.h>
 #include <linux/io.h>
+#include <linux/irqdomain.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 +37,67 @@ 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_dt_irq_setup(struct device_node *node, struct device_node *parent)
+{
+	int i;
+	void __iomem *reg;
+
+	for (i = 0; (reg = of_iomap(node, i)) != NULL; i++) {
+		orion_irq_init(i * 32, reg);
+		irq_domain_add_legacy(node, 32, i * 32, 0,
+					&irq_domain_simple_ops, NULL);
+	}
+
+	return 0;
+}
+
+static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
+{
+	orion_gpio_irq_handler((long)irq_data_get_irq_handler_data(&desc->irq_data));
+}
+
+unsigned int irq_orion_gpio_start = 64; /* can be overridden from platform */
+static int __init orion_dt_gpio_irq_setup(struct device_node *node, struct device_node *parent)
+{
+	int i;
+	void __iomem *reg;
+	for (i = 0; (reg = of_iomap(node, i)) != NULL; i++) {
+		int nr_irqs;
+
+		for (nr_irqs = 0; nr_irqs < 4; nr_irqs++) {
+			int parent_irq;
+			parent_irq = irq_of_parse_and_map(node, i * 4 + nr_irqs);
+			if (!parent_irq)
+				break;
+
+			irq_domain_add_legacy(node, 8,
+					irq_orion_gpio_start + nr_irqs * 8,
+					i * 32 + nr_irqs * 8,
+					&irq_domain_simple_ops,
+					(void *)parent_irq);
+			irq_set_chained_handler(parent_irq, gpio_irq_handler);
+			irq_set_handler_data(parent_irq,
+					(void *)(i * 32 + nr_irqs * 8));
+		}
+
+		orion_gpio_init(i * 32, nr_irqs, (u32)reg, 0,
+				irq_orion_gpio_start);
+		irq_orion_gpio_start += nr_irqs;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id orion_dt_irq_match[] __initconst = {
+	{ .compatible = "mrvl,orion-irq", .data = &orion_dt_irq_setup },
+	{ .compatible = "mrvl,orion-gpio", .data = &orion_dt_gpio_irq_setup },
+	{ }
+};
+
+void __init orion_dt_init_irq(void)
+{
+	of_irq_init(orion_dt_irq_match);
+}
+#endif

Re: kirkwood devicetree respin

[Arnd Bergmann]

On Tuesday 13 March 2012, Arnd Bergmann wrote:
> +
> +               gpio: gpio@f1010100 {
> +                       compatible = "mrvl,kirkwood-gpio", "mrvl,orion-gpio";
> +                       gpio-controller;
> +                       interrupt-controller;
> +                       #interrupt-cells = <1>;
> +                       #gpio-cells = <1>;
> +                       reg = <0x10100 0x40 0x10140 0x40>;
> +               };
> +


Found the first problem in my series straight away, this needs to have
an additional line with

			interrupts = <35 36 37 38 39 40 41>;

	Arnd

Re: kirkwood devicetree respin

[Jason Cooper]

On Tue, Mar 13, 2012 at 03:57:49PM +0000, Arnd Bergmann wrote:
> On Monday 12 March 2012, Jason Cooper wrote:
> > I haven't had a chance to push this series to the ml yet (maybe
> > tonight), but I wanted to give Jamie something better to go off of.
> > 
> > Only issue I have (I think unrelated) is that orion-ehci isn't coming
> > up.  But these patches don't touch that, so I think it might be my
> > config again.  Although it doesn't look like it...
> 
> I looked at your patches but also couldn't find anything that hints
> at why ehci would break.

most likely a config issue, I'll keep an eye on it after we get a
working intc.

> > Also, serial comes up, works without earlyprintk, but says irq = 0.
> > Could be due to the way I setup serial in dtsi/dts.
> 
> Well, in this series you don't have any interrupt-controller node
> in the device tree, so the interrupt number lookup fails for
> any irq you put into the device tree.

I was thinking the same thing.

> I wanted to understand how this works for myself, so I've tried
> implementing it in the patch below. This probably doesn't work
> right away, but it should give you an idea of what needs to
> be done.
> 
> It is based on the latest irqdomain series from
> git://git.secretlab.ca/git/linux-2.6.git irqdomain/next
> together with your own patches.

Arnd, thanks!  I was struggling a bit with this as I'm having trouble
visualizing the HW based on the existing code.  I'll test this out
shortly.

thx,

Jason.

> Grant, can you have a look at this? I'm trying to help out
> multiple people right now who are converting a number of platforms
> to devicetree. It would be good to know if I'm giving the right
> recommendations.
> 
> Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> 
> diff --git a/arch/arm/boot/dts/kirkwood.dtsi b/arch/arm/boot/dts/kirkwood.dtsi
> index 3474ef8..762f8e2 100644
> --- a/arch/arm/boot/dts/kirkwood.dtsi
> +++ b/arch/arm/boot/dts/kirkwood.dtsi
> @@ -2,6 +2,7 @@
>  
>  / {
>  	compatible = "mrvl,kirkwood";
> +	interrupt-parent = <&irq>;
>  
>  	ocp@f1000000 {
>  		compatible = "simple-bus";
> @@ -9,6 +10,22 @@
>  		#address-cells = <1>;
>  		#size-cells = <1>;
>  
> +		irq: irq@f1020204 {
> +			compatible = "mrvl,kirkwood-irq", "mrvl,orion-irq";
> +			interrupt-controller;
> +			#interrupt-cells = <1>;
> +			reg = <0x20204 0x4 0x14>;
> +		};
> +
> +		gpio: gpio@f1010100 {
> +			compatible = "mrvl,kirkwood-gpio", "mrvl,orion-gpio";
> +			gpio-controller;
> +			interrupt-controller;
> +			#interrupt-cells = <1>;
> +			#gpio-cells = <1>;
> +			reg = <0x10100 0x40 0x10140 0x40>;
> +		};
> +
>  		serial@12000 {
>  			compatible = "ns16550a";
>  			reg = <0x12000 0x100>;
> diff --git a/arch/arm/mach-kirkwood/board-dt.c b/arch/arm/mach-kirkwood/board-dt.c
> index 0819240..67a356c 100644
> --- a/arch/arm/mach-kirkwood/board-dt.c
> +++ b/arch/arm/mach-kirkwood/board-dt.c
> @@ -16,6 +16,7 @@
>  #include <linux/of_platform.h>
>  #include <asm/mach/arch.h>
>  #include <asm/mach/map.h>
> +#include <plat/irq.h>
>  #include <mach/bridge-regs.h>
>  #include "common.h"
>  
> @@ -67,7 +68,7 @@ DT_MACHINE_START(KIRKWOOD_DT, "Marvell Kirkwood (Flattened Device Tree)")
>  	/* Maintainer: Jason Cooper <jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org> */
>  	.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..a0e13d2 100644
> --- a/arch/arm/mach-kirkwood/irq.c
> +++ b/arch/arm/mach-kirkwood/irq.c
> @@ -46,3 +46,4 @@ void __init kirkwood_init_irq(void)
>  	irq_set_chained_handler(IRQ_KIRKWOOD_GPIO_HIGH_16_23,
>  				gpio_irq_handler);
>  }
> +
> diff --git a/arch/arm/plat-orion/include/plat/irq.h b/arch/arm/plat-orion/include/plat/irq.h
> index f05eeab..dc83270 100644
> --- a/arch/arm/plat-orion/include/plat/irq.h
> +++ b/arch/arm/plat-orion/include/plat/irq.h
> @@ -13,5 +13,6 @@
>  
>  void orion_irq_init(unsigned int irq_start, void __iomem *maskaddr);
>  
> +void orion_dt_init_irq(void);
>  
>  #endif
> diff --git a/arch/arm/plat-orion/irq.c b/arch/arm/plat-orion/irq.c
> index 2d5b9c1..2e8e5db 100644
> --- a/arch/arm/plat-orion/irq.c
> +++ b/arch/arm/plat-orion/irq.c
> @@ -12,7 +12,12 @@
>  #include <linux/init.h>
>  #include <linux/irq.h>
>  #include <linux/io.h>
> +#include <linux/irqdomain.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 +37,67 @@ 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_dt_irq_setup(struct device_node *node, struct device_node *parent)
> +{
> +	int i;
> +	void __iomem *reg;
> +
> +	for (i = 0; (reg = of_iomap(node, i)) != NULL; i++) {
> +		orion_irq_init(i * 32, reg);
> +		irq_domain_add_legacy(node, 32, i * 32, 0,
> +					&irq_domain_simple_ops, NULL);
> +	}
> +
> +	return 0;
> +}
> +
> +static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> +{
> +	orion_gpio_irq_handler((long)irq_data_get_irq_handler_data(&desc->irq_data));
> +}
> +
> +unsigned int irq_orion_gpio_start = 64; /* can be overridden from platform */
> +static int __init orion_dt_gpio_irq_setup(struct device_node *node, struct device_node *parent)
> +{
> +	int i;
> +	void __iomem *reg;
> +	for (i = 0; (reg = of_iomap(node, i)) != NULL; i++) {
> +		int nr_irqs;
> +
> +		for (nr_irqs = 0; nr_irqs < 4; nr_irqs++) {
> +			int parent_irq;
> +			parent_irq = irq_of_parse_and_map(node, i * 4 + nr_irqs);
> +			if (!parent_irq)
> +				break;
> +
> +			irq_domain_add_legacy(node, 8,
> +					irq_orion_gpio_start + nr_irqs * 8,
> +					i * 32 + nr_irqs * 8,
> +					&irq_domain_simple_ops,
> +					(void *)parent_irq);
> +			irq_set_chained_handler(parent_irq, gpio_irq_handler);
> +			irq_set_handler_data(parent_irq,
> +					(void *)(i * 32 + nr_irqs * 8));
> +		}
> +
> +		orion_gpio_init(i * 32, nr_irqs, (u32)reg, 0,
> +				irq_orion_gpio_start);
> +		irq_orion_gpio_start += nr_irqs;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id orion_dt_irq_match[] __initconst = {
> +	{ .compatible = "mrvl,orion-irq", .data = &orion_dt_irq_setup },
> +	{ .compatible = "mrvl,orion-gpio", .data = &orion_dt_gpio_irq_setup },
> +	{ }
> +};
> +
> +void __init orion_dt_init_irq(void)
> +{
> +	of_irq_init(orion_dt_irq_match);
> +}
> +#endif

Re: kirkwood devicetree respin

[Jason Cooper]

On Tue, Mar 13, 2012 at 03:57:49PM +0000, Arnd Bergmann wrote:
> On Monday 12 March 2012, Jason Cooper wrote:
> > Also, serial comes up, works without earlyprintk, but says irq = 0.
> > Could be due to the way I setup serial in dtsi/dts.
> 
> Well, in this series you don't have any interrupt-controller node
> in the device tree, so the interrupt number lookup fails for
> any irq you put into the device tree.
> 
> I wanted to understand how this works for myself, so I've tried
> implementing it in the patch below. This probably doesn't work
> right away, but it should give you an idea of what needs to
> be done.
> 
> It is based on the latest irqdomain series from
> git://git.secretlab.ca/git/linux-2.6.git irqdomain/next
> together with your own patches.
> 
> Grant, can you have a look at this? I'm trying to help out
> multiple people right now who are converting a number of platforms
> to devicetree. It would be good to know if I'm giving the right
> recommendations.

Arnd, I got this to compile, however:

> 
> Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> 
> diff --git a/arch/arm/boot/dts/kirkwood.dtsi b/arch/arm/boot/dts/kirkwood.dtsi
> index 3474ef8..762f8e2 100644
> --- a/arch/arm/boot/dts/kirkwood.dtsi
> +++ b/arch/arm/boot/dts/kirkwood.dtsi
> @@ -2,6 +2,7 @@
>  
>  / {
>  	compatible = "mrvl,kirkwood";
> +	interrupt-parent = <&irq>;
>  
>  	ocp@f1000000 {
>  		compatible = "simple-bus";
> @@ -9,6 +10,22 @@
>  		#address-cells = <1>;
>  		#size-cells = <1>;
>  
> +		irq: irq@f1020204 {
> +			compatible = "mrvl,kirkwood-irq", "mrvl,orion-irq";
> +			interrupt-controller;
> +			#interrupt-cells = <1>;
> +			reg = <0x20204 0x4 0x14>;
> +		};
> +
> +		gpio: gpio@f1010100 {
> +			compatible = "mrvl,kirkwood-gpio", "mrvl,orion-gpio";
> +			gpio-controller;
> +			interrupt-controller;
> +			#interrupt-cells = <1>;
> +			#gpio-cells = <1>;

			interrupts = <...>; /* added as mentioned */

> +			reg = <0x10100 0x40 0x10140 0x40>;

orion_dt_gpio_irq_setup(...) only iterates over the first region (i=0),
which sets the max hwirqs to 32 which causes this WARN_ON():

------------[ cut here ]------------
WARNING: at kernel/irq/irqdomain.c:75
irq_domain_legacy_revmap+0x24/0x7c()
Modules linked in:
[<c000d644>] (unwind_backtrace+0x0/0xf0) from [<c0015484>] (warn_slowpath_common+0x4c/0x64)
[<c0015484>] (warn_slowpath_common+0x4c/0x64) from [<c00154b8>] (warn_slowpath_null+0x1c/0x24)
[<c00154b8>] (warn_slowpath_null+0x1c/0x24) from [<c005b35c>] (irq_domain_legacy_revmap+0x24/0x7c)
[<c005b35c>] (irq_domain_legacy_revmap+0x24/0x7c) from [<c005b608>] (irq_create_mapping+0x20/0x12c)
[<c005b608>] (irq_create_mapping+0x20/0x12c) from [<c005b790>] (irq_create_of_mapping+0x7c/0xf0)
[<c005b790>] (irq_create_of_mapping+0x7c/0xf0) from [<c02d4a5c>] (irq_of_parse_and_map+0x30/0x38)
[<c02d4a5c>] (irq_of_parse_and_map+0x30/0x38) from [<c04fd3b8>] (orion_dt_gpio_irq_setup+0x50/0x108)
[<c04fd3b8>] (orion_dt_gpio_irq_setup+0x50/0x108) from [<c050fdbc>] (of_irq_init+0x164/0x298)
[<c050fdbc>] (of_irq_init+0x164/0x298) from [<c04f90f4>] (init_IRQ+0x14/0x1c)  
[<c04f90f4>] (init_IRQ+0x14/0x1c) from [<c04f7614>] (start_kernel+0x190/0x300)
[<c04f7614>] (start_kernel+0x190/0x300) from [<0000803c>] (0x803c)
---[ end trace 1b75b31a2719ed1c ]---
   hwirq = 35
   first_hwirq = 0
   size = 32

I'll keep digging tomorrow...

Thanks for the help,

Jason.

> +		};
> +
>  		serial@12000 {
>  			compatible = "ns16550a";
>  			reg = <0x12000 0x100>;
> diff --git a/arch/arm/mach-kirkwood/board-dt.c b/arch/arm/mach-kirkwood/board-dt.c
> index 0819240..67a356c 100644
> --- a/arch/arm/mach-kirkwood/board-dt.c
> +++ b/arch/arm/mach-kirkwood/board-dt.c
> @@ -16,6 +16,7 @@
>  #include <linux/of_platform.h>
>  #include <asm/mach/arch.h>
>  #include <asm/mach/map.h>
> +#include <plat/irq.h>
>  #include <mach/bridge-regs.h>
>  #include "common.h"
>  
> @@ -67,7 +68,7 @@ DT_MACHINE_START(KIRKWOOD_DT, "Marvell Kirkwood (Flattened Device Tree)")
>  	/* Maintainer: Jason Cooper <jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org> */
>  	.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..a0e13d2 100644
> --- a/arch/arm/mach-kirkwood/irq.c
> +++ b/arch/arm/mach-kirkwood/irq.c
> @@ -46,3 +46,4 @@ void __init kirkwood_init_irq(void)
>  	irq_set_chained_handler(IRQ_KIRKWOOD_GPIO_HIGH_16_23,
>  				gpio_irq_handler);
>  }
> +
> diff --git a/arch/arm/plat-orion/include/plat/irq.h b/arch/arm/plat-orion/include/plat/irq.h
> index f05eeab..dc83270 100644
> --- a/arch/arm/plat-orion/include/plat/irq.h
> +++ b/arch/arm/plat-orion/include/plat/irq.h
> @@ -13,5 +13,6 @@
>  
>  void orion_irq_init(unsigned int irq_start, void __iomem *maskaddr);
>  
> +void orion_dt_init_irq(void);
>  
>  #endif
> diff --git a/arch/arm/plat-orion/irq.c b/arch/arm/plat-orion/irq.c
> index 2d5b9c1..2e8e5db 100644
> --- a/arch/arm/plat-orion/irq.c
> +++ b/arch/arm/plat-orion/irq.c
> @@ -12,7 +12,12 @@
>  #include <linux/init.h>
>  #include <linux/irq.h>
>  #include <linux/io.h>
> +#include <linux/irqdomain.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 +37,67 @@ 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_dt_irq_setup(struct device_node *node, struct device_node *parent)
> +{
> +	int i;
> +	void __iomem *reg;
> +
> +	for (i = 0; (reg = of_iomap(node, i)) != NULL; i++) {
> +		orion_irq_init(i * 32, reg);
> +		irq_domain_add_legacy(node, 32, i * 32, 0,
> +					&irq_domain_simple_ops, NULL);
> +	}
> +
> +	return 0;
> +}
> +
> +static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> +{
> +	orion_gpio_irq_handler((long)irq_data_get_irq_handler_data(&desc->irq_data));
> +}
> +
> +unsigned int irq_orion_gpio_start = 64; /* can be overridden from platform */
> +static int __init orion_dt_gpio_irq_setup(struct device_node *node, struct device_node *parent)
> +{
> +	int i;
> +	void __iomem *reg;
> +	for (i = 0; (reg = of_iomap(node, i)) != NULL; i++) {
> +		int nr_irqs;
> +
> +		for (nr_irqs = 0; nr_irqs < 4; nr_irqs++) {
> +			int parent_irq;
> +			parent_irq = irq_of_parse_and_map(node, i * 4 + nr_irqs);
> +			if (!parent_irq)
> +				break;
> +
> +			irq_domain_add_legacy(node, 8,
> +					irq_orion_gpio_start + nr_irqs * 8,
> +					i * 32 + nr_irqs * 8,
> +					&irq_domain_simple_ops,
> +					(void *)parent_irq);
> +			irq_set_chained_handler(parent_irq, gpio_irq_handler);
> +			irq_set_handler_data(parent_irq,
> +					(void *)(i * 32 + nr_irqs * 8));
> +		}
> +
> +		orion_gpio_init(i * 32, nr_irqs, (u32)reg, 0,
> +				irq_orion_gpio_start);
> +		irq_orion_gpio_start += nr_irqs;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id orion_dt_irq_match[] __initconst = {
> +	{ .compatible = "mrvl,orion-irq", .data = &orion_dt_irq_setup },
> +	{ .compatible = "mrvl,orion-gpio", .data = &orion_dt_gpio_irq_setup },
> +	{ }
> +};
> +
> +void __init orion_dt_init_irq(void)
> +{
> +	of_irq_init(orion_dt_irq_match);
> +}
> +#endif

Re: kirkwood devicetree respin

[Jason Cooper]

On Tue, Mar 13, 2012 at 03:57:49PM +0000, Arnd Bergmann wrote:
> On Monday 12 March 2012, Jason Cooper wrote:
> > Only issue I have (I think unrelated) is that orion-ehci isn't coming
> > up.  But these patches don't touch that, so I think it might be my
> > config again.  Although it doesn't look like it...
> 
> I looked at your patches but also couldn't find anything that hints
> at why ehci would break.

I figured it out.  Apparently, dreamplug is the first board to put root
on usb-attached microsd card.  And I'm the first one to try to compile
orion-ehci into the kernel.  ;-)

The following fix the problem:

diff --git a/drivers/usb/host/ehci-orion.c b/drivers/usb/host/ehci-orion.c
index 6c6a5a3..0808417 100644
--- a/drivers/usb/host/ehci-orion.c
+++ b/drivers/usb/host/ehci-orion.c
@@ -318,3 +318,5 @@ static struct platform_driver ehci_orion_driver = {
        .shutdown       = usb_hcd_platform_shutdown,
        .driver.name    = "orion-ehci",
 };
+
+module_platform_driver(ehci_orion_driver);



I figure most folks will build orion-ehci as a module and use an initrd.
In the meantime, I'll carry this patch in my stack for v3.5.  Unless you
would prefer to do something different.

thx,

Jason.

Re: kirkwood devicetree respin

[Nicolas Pitre]

On Tue, 20 Mar 2012, Jason Cooper wrote:

> On Tue, Mar 13, 2012 at 03:57:49PM +0000, Arnd Bergmann wrote:
> > On Monday 12 March 2012, Jason Cooper wrote:
> > > Only issue I have (I think unrelated) is that orion-ehci isn't coming
> > > up.  But these patches don't touch that, so I think it might be my
> > > config again.  Although it doesn't look like it...
> > 
> > I looked at your patches but also couldn't find anything that hints
> > at why ehci would break.
> 
> I figured it out.  Apparently, dreamplug is the first board to put root
> on usb-attached microsd card.  And I'm the first one to try to compile
> orion-ehci into the kernel.  ;-)
> 
> The following fix the problem:
> 
> diff --git a/drivers/usb/host/ehci-orion.c b/drivers/usb/host/ehci-orion.c
> index 6c6a5a3..0808417 100644
> --- a/drivers/usb/host/ehci-orion.c
> +++ b/drivers/usb/host/ehci-orion.c
> @@ -318,3 +318,5 @@ static struct platform_driver ehci_orion_driver = {
>         .shutdown       = usb_hcd_platform_shutdown,
>         .driver.name    = "orion-ehci",
>  };
> +
> +module_platform_driver(ehci_orion_driver);
> 
> 
> 
> I figure most folks will build orion-ehci as a module and use an initrd.

Possibly, however that doesn't mean this shouldn't be fixed anyway.


Nicolas

Re: kirkwood devicetree respin

[Jason Cooper]

On Tue, Mar 20, 2012 at 02:03:23PM -0400, Nicolas Pitre wrote:
> On Tue, 20 Mar 2012, Jason Cooper wrote:
> 
> > On Tue, Mar 13, 2012 at 03:57:49PM +0000, Arnd Bergmann wrote:
> > > On Monday 12 March 2012, Jason Cooper wrote:
> > > > Only issue I have (I think unrelated) is that orion-ehci isn't coming
> > > > up.  But these patches don't touch that, so I think it might be my
> > > > config again.  Although it doesn't look like it...
> > > 
> > > I looked at your patches but also couldn't find anything that hints
> > > at why ehci would break.
> > 
> > I figured it out.  Apparently, dreamplug is the first board to put root
> > on usb-attached microsd card.  And I'm the first one to try to compile
> > orion-ehci into the kernel.  ;-)
> > 
> > The following fix the problem:
> > 
> > diff --git a/drivers/usb/host/ehci-orion.c b/drivers/usb/host/ehci-orion.c
> > index 6c6a5a3..0808417 100644
> > --- a/drivers/usb/host/ehci-orion.c
> > +++ b/drivers/usb/host/ehci-orion.c
> > @@ -318,3 +318,5 @@ static struct platform_driver ehci_orion_driver = {
> >         .shutdown       = usb_hcd_platform_shutdown,
> >         .driver.name    = "orion-ehci",
> >  };
> > +
> > +module_platform_driver(ehci_orion_driver);
> > 
> > 
> > 
> > I figure most folks will build orion-ehci as a module and use an initrd.
> 
> Possibly, however that doesn't mean this shouldn't be fixed anyway.

Yes, I didn't mean to imply otherwise.  I know new patches for v3.4 are
no longer being accepted (they should already be in for-next, right?)
So, I'm not sure what the disposition of this patch should be.

If it's appropriate to push it into v3.4[-rc?], great.  I just didn't
want to assume that and appear to be pushing patches after the for-next
window closed.  I'll send it whereever you guys advise.

thx,

Jason.

Re: kirkwood devicetree respin

[Nicolas Pitre]

On Tue, 20 Mar 2012, Jason Cooper wrote:

> On Tue, Mar 20, 2012 at 02:03:23PM -0400, Nicolas Pitre wrote:
> > On Tue, 20 Mar 2012, Jason Cooper wrote:
> > 
> > > I figure most folks will build orion-ehci as a module and use an initrd.
> > 
> > Possibly, however that doesn't mean this shouldn't be fixed anyway.
> 
> Yes, I didn't mean to imply otherwise.  I know new patches for v3.4 are
> no longer being accepted (they should already be in for-next, right?)
> So, I'm not sure what the disposition of this patch should be.

Just send it to arm-soc as usual, identifying it as a fix.
Or, given that this is USB related, you might send it to 
linux-usb-u79uwXL29TaiAVqoAR/hOA@public.gmane.org

> If it's appropriate to push it into v3.4[-rc?], great.  I just didn't
> want to assume that and appear to be pushing patches after the for-next
> window closed.  I'll send it whereever you guys advise.

It is always appropriate to send bug fixes.  This is the kind of patches 
for which there is a -rc period.

It is too late for v3.3 now as it was released 2 days ago.  However if 
v3.3 is also broken then the fix for v3.4-rc can carry the 
"CC: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" annotation so it'll be picked up for 
v3.3.1 as well.


Nicolas

Re: kirkwood devicetree respin

[Arnd Bergmann]

On Tuesday 20 March 2012, Jason Cooper wrote:
> diff --git a/drivers/usb/host/ehci-orion.c b/drivers/usb/host/ehci-orion.c
> index 6c6a5a3..0808417 100644
> --- a/drivers/usb/host/ehci-orion.c
> +++ b/drivers/usb/host/ehci-orion.c
> @@ -318,3 +318,5 @@ static struct platform_driver ehci_orion_driver = {
>         .shutdown       = usb_hcd_platform_shutdown,
>         .driver.name    = "orion-ehci",
>  };
> +
> +module_platform_driver(ehci_orion_driver);
> 
This actually looks wrong to me, because the ehci driver has an
interesting way of pulling in platform specific drivers.

I'm pretty sure that it will break modular builds, which can only
have one module_init function in them. It's not clear to me why this
patch actually fixes the problem either. Could it be that you have
multiple platform driver back-ends for ehci enabled at the same time?

That should not be possible in theory and at least give a warning, but
the ehci probing method is a bit fragile so I would not be too surprised.

	Arnd

Re: kirkwood devicetree respin

[Jason Cooper]

On Tue, Mar 20, 2012 at 03:03:45PM -0400, Nicolas Pitre wrote:
> On Tue, 20 Mar 2012, Jason Cooper wrote:
> 
> > On Tue, Mar 20, 2012 at 02:03:23PM -0400, Nicolas Pitre wrote:
> > > On Tue, 20 Mar 2012, Jason Cooper wrote:
> > > 
> > > > I figure most folks will build orion-ehci as a module and use an initrd.
> > > 
> > > Possibly, however that doesn't mean this shouldn't be fixed anyway.
> > 
> > Yes, I didn't mean to imply otherwise.  I know new patches for v3.4 are
> > no longer being accepted (they should already be in for-next, right?)
> > So, I'm not sure what the disposition of this patch should be.
> 
> Just send it to arm-soc as usual, identifying it as a fix.
> Or, given that this is USB related, you might send it to 
> linux-usb-u79uwXL29TaiAVqoAR/hOA@public.gmane.org

Ok.

> > If it's appropriate to push it into v3.4[-rc?], great.  I just didn't
> > want to assume that and appear to be pushing patches after the for-next
> > window closed.  I'll send it whereever you guys advise.
> 
> It is always appropriate to send bug fixes.  This is the kind of patches 
> for which there is a -rc period.

good to know.

> It is too late for v3.3 now as it was released 2 days ago.  However if 
> v3.3 is also broken then the fix for v3.4-rc can carry the 
> "CC: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" annotation so it'll be picked up for 
> v3.3.1 as well.

It should only be broken for folks trying to boot a mainline u-boot and
kernel on the dreamplug without an initrd.  That requires v3.4.  There
maybe some other folks I don't know about so I'll add the CC just in
case.

Thanks for the clarification,

Jason.

Re: kirkwood devicetree respin

[Andrew Lunn]

On Tue, Mar 20, 2012 at 07:15:07PM +0000, Arnd Bergmann wrote:
> On Tuesday 20 March 2012, Jason Cooper wrote:
> > diff --git a/drivers/usb/host/ehci-orion.c b/drivers/usb/host/ehci-orion.c
> > index 6c6a5a3..0808417 100644
> > --- a/drivers/usb/host/ehci-orion.c
> > +++ b/drivers/usb/host/ehci-orion.c
> > @@ -318,3 +318,5 @@ static struct platform_driver ehci_orion_driver = {
> >         .shutdown       = usb_hcd_platform_shutdown,
> >         .driver.name    = "orion-ehci",
> >  };
> > +
> > +module_platform_driver(ehci_orion_driver);
> > 
> This actually looks wrong to me, because the ehci driver has an
> interesting way of pulling in platform specific drivers.
> 
> I'm pretty sure that it will break modular builds, which can only
> have one module_init function in them. It's not clear to me why this
> patch actually fixes the problem either. Could it be that you have
> multiple platform driver back-ends for ehci enabled at the same time?
> 
> That should not be possible in theory and at least give a warning, but
> the ehci probing method is a bit fragile so I would not be too surprised.

Hi Jason

I fell fowl of something in this area. Do you have CONFIG_USB_EHCI_MV=y?
Make sure you do not have this. 

drivers/usb/host/ehci-hcd.c has some unusual code, and if you have
both CONFIG_USB_EHCI_MV and CONFIG_PLAT_ORION, the CONFIG_USB_EHCI_MV
is given preference and the Orion driver never gets its probe
functions called etc.

	  Andrew

Re: kirkwood devicetree respin

[Jason Cooper]

Adding linux-usb to the CC:.

We're trying to figure out how to compile the orion-ehci.c driver into
the kernel.  This is needed for the Globalscale Technologies Dreamplug
to boot and find it's root filesystem (on an internal usb attached
microsd card).

On Tue, Mar 20, 2012 at 07:15:07PM +0000, Arnd Bergmann wrote:
> On Tuesday 20 March 2012, Jason Cooper wrote:
> > diff --git a/drivers/usb/host/ehci-orion.c b/drivers/usb/host/ehci-orion.c
> > index 6c6a5a3..0808417 100644
> > --- a/drivers/usb/host/ehci-orion.c
> > +++ b/drivers/usb/host/ehci-orion.c
> > @@ -318,3 +318,5 @@ static struct platform_driver ehci_orion_driver = {
> >         .shutdown       = usb_hcd_platform_shutdown,
> >         .driver.name    = "orion-ehci",
> >  };
> > +
> > +module_platform_driver(ehci_orion_driver);
> > 
> This actually looks wrong to me, because the ehci driver has an
> interesting way of pulling in platform specific drivers.
> 
> I'm pretty sure that it will break modular builds, which can only
> have one module_init function in them. It's not clear to me why this
> patch actually fixes the problem either. Could it be that you have
> multiple platform driver back-ends for ehci enabled at the same time?

Yes, it depends on ehci-hcd:

config USB_EHCI_MV
        bool "EHCI support for Marvell on-chip controller"
        depends on USB_EHCI_HCD
        select USB_EHCI_ROOT_HUB_TT

> That should not be possible in theory and at least give a warning, but
> the ehci probing method is a bit fragile so I would not be too surprised.

I used to get the following build warning:

  CC      drivers/usb/host/ehci-hcd.o
  CC      kernel/module.o
drivers/usb/host/ehci-hcd.c:1376:0: warning: "PLATFORM_DRIVER" redefined
drivers/usb/host/ehci-hcd.c:1296:0: note: this is the location of the
previous definition
drivers/usb/host/ehci-orion.c:315:31: warning: 'ehci_orion_driver'
defined but not used
  LD      kernel/built-in.o
  LD      drivers/usb/host/built-in.o
  LD      drivers/usb/built-in.o
  LD      drivers/built-in.o


But after the patch, I get this:

  CC      drivers/usb/host/ehci-hcd.o
  CC      kernel/module.o
drivers/usb/host/ehci-hcd.c:1376:0: warning: "PLATFORM_DRIVER" redefined
drivers/usb/host/ehci-hcd.c:1296:0: note: this is the location of the
previous definition
  LD      kernel/built-in.o
  LD      drivers/usb/host/built-in.o
  LD      drivers/usb/built-in.o
  LD      drivers/built-in.o


As for boot,

ehci_hcd: USB 2.0 'Enhanced' Host Controller (EHCI) Driver 
orion-ehci orion-ehci.0: Marvell Orion EHCI 
orion-ehci orion-ehci.0: new USB bus registered, assigned bus number 1 
orion-ehci orion-ehci.0: irq 19, io mem 0xf1050000 
orion-ehci orion-ehci.0: USB 2.0 started, EHCI 1.00 
hub 1-0:1.0: USB hub found 
hub 1-0:1.0: 1 port detected 


No errors/warnings reported.

without the patch, I get the first line (ehci_hcd:...) and nothing else.
Then, a panic because it can't find /dev/sda2.

thx,

Jason.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: kirkwood devicetree respin

[Jason Cooper]

On Tue, Mar 20, 2012 at 08:32:22PM +0100, Andrew Lunn wrote:
> On Tue, Mar 20, 2012 at 07:15:07PM +0000, Arnd Bergmann wrote:
> > On Tuesday 20 March 2012, Jason Cooper wrote:
> > > diff --git a/drivers/usb/host/ehci-orion.c b/drivers/usb/host/ehci-orion.c
> > > index 6c6a5a3..0808417 100644
> > > --- a/drivers/usb/host/ehci-orion.c
> > > +++ b/drivers/usb/host/ehci-orion.c
> > > @@ -318,3 +318,5 @@ static struct platform_driver ehci_orion_driver = {
> > >         .shutdown       = usb_hcd_platform_shutdown,
> > >         .driver.name    = "orion-ehci",
> > >  };
> > > +
> > > +module_platform_driver(ehci_orion_driver);
> > > 
> > This actually looks wrong to me, because the ehci driver has an
> > interesting way of pulling in platform specific drivers.
> > 
> > I'm pretty sure that it will break modular builds, which can only
> > have one module_init function in them. It's not clear to me why this
> > patch actually fixes the problem either. Could it be that you have
> > multiple platform driver back-ends for ehci enabled at the same time?
> > 
> > That should not be possible in theory and at least give a warning, but
> > the ehci probing method is a bit fragile so I would not be too surprised.
> 
> Hi Jason
> 
> I fell fowl of something in this area. Do you have CONFIG_USB_EHCI_MV=y?
> Make sure you do not have this. 
> 
> drivers/usb/host/ehci-hcd.c has some unusual code, and if you have
> both CONFIG_USB_EHCI_MV and CONFIG_PLAT_ORION, the CONFIG_USB_EHCI_MV
> is given preference and the Orion driver never gets its probe
> functions called etc.

waddaya know?  The compile warnings went away, and the d*mn thing booted
fine.

Is there a scenario where someone would want to select
CONFIG_USB_EHCI_MV in menuconfig?

thx,

Jason.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: kirkwood devicetree respin

[Andrew Lunn]

> > Hi Jason
> > 
> > I fell fowl of something in this area. Do you have CONFIG_USB_EHCI_MV=y?
> > Make sure you do not have this. 
> > 
> > drivers/usb/host/ehci-hcd.c has some unusual code, and if you have
> > both CONFIG_USB_EHCI_MV and CONFIG_PLAT_ORION, the CONFIG_USB_EHCI_MV
> > is given preference and the Orion driver never gets its probe
> > functions called etc.
> 
> waddaya know?  The compile warnings went away, and the d*mn thing booted
> fine.
> 
> Is there a scenario where someone would want to select
> CONFIG_USB_EHCI_MV in menuconfig?

Not on an Orion platform, as far as i know. Maybe

config USB_EHCI_MV
        bool "EHCI support for Marvell on-chip controller"
-        depends on USB_EHCI_HCD
+        depends on USB_EHCI_HCD && !PLAT_ORION
        select USB_EHCI_ROOT_HUB_TT
        ---help---
          Enables support for Marvell (including PXA and MMP series) on-chip
          USB SPH and OTG controller. SPH is a single port host, and it can
          only be EHCI host. OTG is controller that can switch to host mode.

Maybe also -Werror for that one file to catch other similar cases?

Andrew

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: kirkwood devicetree respin

[Arnd Bergmann]

On Tuesday 20 March 2012, Andrew Lunn wrote:
> > Is there a scenario where someone would want to select
> > CONFIG_USB_EHCI_MV in menuconfig?
> 
> Not on an Orion platform, as far as i know. Maybe
> 
> config USB_EHCI_MV
>         bool "EHCI support for Marvell on-chip controller"
> -        depends on USB_EHCI_HCD
> +        depends on USB_EHCI_HCD && !PLAT_ORION
>         select USB_EHCI_ROOT_HUB_TT
>         ---help---
>           Enables support for Marvell (including PXA and MMP series) on-chip
>           USB SPH and OTG controller. SPH is a single port host, and it can
>           only be EHCI host. OTG is controller that can switch to host mode.

Well, rihgt now you can select it on anything, including non-ARM architectures,
and it fails whenever you select it in addition to another platform
driver.

If you want to add a dependency, it should be

	depends on PLAT_PXA

Most other platform drivers have a dependency on the platform
they are for, but USB_EHCI_MV was only recently added, and nobody
has bothered to fix this yet.

> Maybe also -Werror for that one file to catch other similar cases?

No, we are actually trying to make sure that any configuration you pick
results in a kernel that builds, so that would be counterproductive.

The problem will be much bigger when we get to the point where you
can actually build a multiplatform kernel, e.g. one that works
on both PXA and Kirkwood because then it will still be broken
for at least one of the two.

We recently had a discussion about how to solve this correctly, see
the email thread at http://lkml.org/lkml/2012/2/25/45 leading
up to http://lkml.org/lkml/2012/2/28/299 .
The problem is the same for ehci and ohci, and I think a lot of
people would welcome a proper fix for the situation.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: kirkwood devicetree respin

[Jason Cooper]

On Tue, Mar 20, 2012 at 08:57:38PM +0000, Arnd Bergmann wrote:
> On Tuesday 20 March 2012, Andrew Lunn wrote:
> > > Is there a scenario where someone would want to select
> > > CONFIG_USB_EHCI_MV in menuconfig?
> > 
> > Not on an Orion platform, as far as i know. Maybe
> > 
> > config USB_EHCI_MV
> >         bool "EHCI support for Marvell on-chip controller"
> > -        depends on USB_EHCI_HCD
> > +        depends on USB_EHCI_HCD && !PLAT_ORION
> >         select USB_EHCI_ROOT_HUB_TT
> >         ---help---
> >           Enables support for Marvell (including PXA and MMP series) on-chip
> >           USB SPH and OTG controller. SPH is a single port host, and it can
> >           only be EHCI host. OTG is controller that can switch to host mode.
> 
> Well, rihgt now you can select it on anything, including non-ARM architectures,
> and it fails whenever you select it in addition to another platform
> driver.
> 
> If you want to add a dependency, it should be
> 
> 	depends on PLAT_PXA
> 
> Most other platform drivers have a dependency on the platform
> they are for, but USB_EHCI_MV was only recently added, and nobody
> has bothered to fix this yet.

It's my understanding that will make the driver visible in menuconfig
for PLAT_PXA.  That may be useful, but what I'm trying to fix is a new
user (me) from *selecting* USB_EHCI_MV on PLAT_ORION.  It breaks when
you do that (at runtime).

The logic in ehci-hcd.c takes care of it for us:

#ifdef CONFIG_PLAT_ORION
#include "ehci-orion.c"
#define PLATFORM_DRIVER         ehci_orion_driver
#endif

Which, as you mentioned earlier, is unusual.

Andrew's logic (!PLAT_ORION) makes sure the driver is not visible in
menuconfig (when ehci-hcd would pick it up) and is visible in
non-PLAT_ORION boards (where ehci-hcd wouldn't pick it up).

My question is, are there any non-PLAT_ORION boards that would use this?
Would it work on those boards if compiled outside of ehci-hcd?

Although, it worked when I added the module_platform_driver() macro.
maybe we could put some logic around that:

#ifndef CONFIG_PLAT_ORION
module_platform_driver(ehci_orion_driver);
#endif

This, in conjuctions with Andrew's patch, above, might cover all the
bases.  However, I have no way to test it.

> > Maybe also -Werror for that one file to catch other similar cases?
> 
> No, we are actually trying to make sure that any configuration you pick
> results in a kernel that builds, so that would be counterproductive.

I would argue it should build *and* work.  Otherwise, there's no point
in having a successful compile.  So, maybe the answer is _not_ to have
it as a configuration option.  Or, at least, invisible in menuconfig.

> The problem will be much bigger when we get to the point where you
> can actually build a multiplatform kernel, e.g. one that works
> on both PXA and Kirkwood because then it will still be broken
> for at least one of the two.

I think there are a few other things that would be broken first, right
now.  ;-)

> We recently had a discussion about how to solve this correctly, see
> the email thread at http://lkml.org/lkml/2012/2/25/45 leading
> up to http://lkml.org/lkml/2012/2/28/299 .
> The problem is the same for ehci and ohci, and I think a lot of
> people would welcome a proper fix for the situation.

+1

thx,

Jason.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: kirkwood devicetree respin

[Arnd Bergmann]

On Tuesday 20 March 2012, Jason Cooper wrote:
> > If you want to add a dependency, it should be
> > 
> >       depends on PLAT_PXA
> > 
> > Most other platform drivers have a dependency on the platform
> > they are for, but USB_EHCI_MV was only recently added, and nobody
> > has bothered to fix this yet.
>
> It's my understanding that will make the driver visible in menuconfig
> for PLAT_PXA.  That may be useful, but what I'm trying to fix is a new
> user (me) from selecting USB_EHCI_MV on PLAT_ORION.  It breaks when
> you do that (at runtime).

Are those two things not the same? The dependency makes the driver
visible only on PLAT_PXA, which means it is invisible on PLAT_ORION
and you can no longer select it. PLAT_PXA and PLAT_ORION are mutually
exclusive.

> Andrew's logic (!PLAT_ORION) makes sure the driver is not visible in
> menuconfig (when ehci-hcd would pick it up) and is visible in
> non-PLAT_ORION boards (where ehci-hcd wouldn't pick it up).
> 
> My question is, are there any non-PLAT_ORION boards that would use this?
> Would it work on those boards if compiled outside of ehci-hcd?

ehci-orion only makes sense on PLAT_ORION, and they never have any
other platform ehci driver.

ehci-mv only makese sense on PLAT_PXA, and they  also don't have any
other platform ehci driver.

> Although, it worked when I added the module_platform_driver() macro.
> maybe we could put some logic around that:
> 
> #ifndef CONFIG_PLAT_ORION
> module_platform_driver(ehci_orion_driver);
> #endif

No, this is just wrong. It expands to module_init(), and
there is already a module_init in ehci-hcd.c, and you must
not have more than one of these when building as a module,
otherwise you get a duplicate symbol definition error.

> > > Maybe also -Werror for that one file to catch other similar cases?
> > 
> > No, we are actually trying to make sure that any configuration you pick
> > results in a kernel that builds, so that would be counterproductive.
> 
> I would argue it should build and work.  Otherwise, there's no point
> in having a successful compile.  So, maybe the answer is not to have
> it as a configuration option.  Or, at least, invisible in menuconfig.

Making USB_EHCI_MV invisible everywhere would also be possible, in that
case the right logic would be

config USB_EHCI_MV
	def_bool y
	depends on USB_EHCI_HCD && PLAT_PXA
	select USB_EHCI_ROOT_HUB_TT

This would unconditionally enable the pxa ehci driver whenever the
common ehci code is enabled, which is a reasonable choice, and matches
the behavior of the orion driver.

> > The problem will be much bigger when we get to the point where you
> > can actually build a multiplatform kernel, e.g. one that works
> > on both PXA and Kirkwood because then it will still be broken
> > for at least one of the two.
> 
> I think there are a few other things that would be broken first, right
> now.  ;-)

Yes, we're working on them one by one. At some point we'll get to this one.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: kirkwood devicetree respin

[Jason Cooper]

On Tue, Mar 20, 2012 at 10:15:09PM +0000, Arnd Bergmann wrote:
> On Tuesday 20 March 2012, Jason Cooper wrote:
> > > If you want to add a dependency, it should be
> > > 
> > >       depends on PLAT_PXA
> > > 
> > > Most other platform drivers have a dependency on the platform
> > > they are for, but USB_EHCI_MV was only recently added, and nobody
> > > has bothered to fix this yet.
> >
> > It's my understanding that will make the driver visible in menuconfig
> > for PLAT_PXA.  That may be useful, but what I'm trying to fix is a new
> > user (me) from selecting USB_EHCI_MV on PLAT_ORION.  It breaks when
> > you do that (at runtime).
> 
> Are those two things not the same? The dependency makes the driver
> visible only on PLAT_PXA, which means it is invisible on PLAT_ORION
> and you can no longer select it. PLAT_PXA and PLAT_ORION are mutually
> exclusive.

Ok, that's what I missed.

...
> > > > Maybe also -Werror for that one file to catch other similar cases?
> > > 
> > > No, we are actually trying to make sure that any configuration you pick
> > > results in a kernel that builds, so that would be counterproductive.
> > 
> > I would argue it should build and work.  Otherwise, there's no point
> > in having a successful compile.  So, maybe the answer is not to have
> > it as a configuration option.  Or, at least, invisible in menuconfig.
> 
> Making USB_EHCI_MV invisible everywhere would also be possible, in that
> case the right logic would be
> 
> config USB_EHCI_MV
> 	def_bool y
> 	depends on USB_EHCI_HCD && PLAT_PXA
> 	select USB_EHCI_ROOT_HUB_TT
> 
> This would unconditionally enable the pxa ehci driver whenever the
> common ehci code is enabled, which is a reasonable choice, and matches
> the behavior of the orion driver.

Ok, I'll gin up a patch for this and CC: stable / linux-usb.

thx,

Jason.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: kirkwood devicetree respin

[Nicolas Pitre]

On Tue, 20 Mar 2012, Arnd Bergmann wrote:

> ehci-orion only makes sense on PLAT_ORION, and they never have any
> other platform ehci driver.
> 
> ehci-mv only makese sense on PLAT_PXA, and they  also don't have any
> other platform ehci driver.

BTW, as someone who once was the active maintainer for PXA, and later 
the active maintainer for Orion, I should say that ehci-mv is a terribly 
bad name (as demonstrated, not all Marvell SOCs use ehci-mv) and 
confusion would be much reduced if it was renamed to ehci-pxa.


Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: kirkwood devicetree respin

[Haojian Zhuang]

On Wed, Mar 21, 2012 at 11:54 AM, Nicolas Pitre <nico-vtqb6HGKxmzR7s880joybQ@public.gmane.org> wrote:
> On Tue, 20 Mar 2012, Arnd Bergmann wrote:
>
>> ehci-orion only makes sense on PLAT_ORION, and they never have any
>> other platform ehci driver.
>>
>> ehci-mv only makese sense on PLAT_PXA, and they  also don't have any
>> other platform ehci driver.
>
> BTW, as someone who once was the active maintainer for PXA, and later
> the active maintainer for Orion, I should say that ehci-mv is a terribly
> bad name (as demonstrated, not all Marvell SOCs use ehci-mv) and
> confusion would be much reduced if it was renamed to ehci-pxa.
>
>
OK. We could try to rename ehci-mv to ehci-pxa.

By the way, we aren't allowed to append "depends on PLAT_PXA" in Kconfig.

Feliple,

Is it ok for appending "depends on PLAT_PXA" into Kconfig now?

Best Regards
Haojian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: kirkwood devicetree respin

[Andrew Lunn]

On Wed, Mar 21, 2012 at 04:21:45PM +0800, Haojian Zhuang wrote:
> On Wed, Mar 21, 2012 at 11:54 AM, Nicolas Pitre <nico-vtqb6HGKxmzR7s880joybQ@public.gmane.org> wrote:
> > On Tue, 20 Mar 2012, Arnd Bergmann wrote:
> >
> >> ehci-orion only makes sense on PLAT_ORION, and they never have any
> >> other platform ehci driver.
> >>
> >> ehci-mv only makese sense on PLAT_PXA, and they ?also don't have any
> >> other platform ehci driver.
> >
> > BTW, as someone who once was the active maintainer for PXA, and later
> > the active maintainer for Orion, I should say that ehci-mv is a terribly
> > bad name (as demonstrated, not all Marvell SOCs use ehci-mv) and
> > confusion would be much reduced if it was renamed to ehci-pxa.
> >
> >
> OK. We could try to rename ehci-mv to ehci-pxa.
> 
> By the way, we aren't allowed to append "depends on PLAT_PXA" in Kconfig.

Hi Hoajian

Could you explain that a bit more? What was the reason you could not
append it? Is the driver used by some other platform as well?

       Thanks
  	Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: kirkwood devicetree respin

[Haojian Zhuang]

On Wed, Mar 21, 2012 at 4:34 PM, Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org> wrote:
> On Wed, Mar 21, 2012 at 04:21:45PM +0800, Haojian Zhuang wrote:
>> On Wed, Mar 21, 2012 at 11:54 AM, Nicolas Pitre <nico-vtqb6HGKxmzR7s880joybQ@public.gmane.org> wrote:
>> > On Tue, 20 Mar 2012, Arnd Bergmann wrote:
>> >
>> >> ehci-orion only makes sense on PLAT_ORION, and they never have any
>> >> other platform ehci driver.
>> >>
>> >> ehci-mv only makese sense on PLAT_PXA, and they ?also don't have any
>> >> other platform ehci driver.
>> >
>> > BTW, as someone who once was the active maintainer for PXA, and later
>> > the active maintainer for Orion, I should say that ehci-mv is a terribly
>> > bad name (as demonstrated, not all Marvell SOCs use ehci-mv) and
>> > confusion would be much reduced if it was renamed to ehci-pxa.
>> >
>> >
>> OK. We could try to rename ehci-mv to ehci-pxa.
>>
>> By the way, we aren't allowed to append "depends on PLAT_PXA" in Kconfig.
>
> Hi Hoajian
>
> Could you explain that a bit more? What was the reason you could not
> append it? Is the driver used by some other platform as well?
>
>       Thanks
>        Andrew

The driver isn't used by any other platform. It's only used in
arch-pxa & arch-mmp.

I can't find the original mail. But I remembered that maintainer said
the driver should be built with any other platform, even with x86. So
we aren't allowed to append it in Kconfig.

Best Regards
Haojian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: kirkwood devicetree respin

[Arnd Bergmann]

On Wednesday 21 March 2012, Haojian Zhuang wrote:
> The driver isn't used by any other platform. It's only used in
> arch-pxa & arch-mmp.
> 
> I can't find the original mail. But I remembered that maintainer said
> the driver should be built with any other platform, even with x86. So
> we aren't allowed to append it in Kconfig.

I generally make the recommendation that any driver should be able to
get built on any system. However, in this particular case, it's clearly
a bug, because you cannot build it together with another platform driver.

That will change when we get to multiplatform builds on ARM that where we
actually need to find a way to make that possible.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: kirkwood devicetree respin

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1309 bytes --]

On Wed, Mar 21, 2012 at 10:09:40AM +0000, Arnd Bergmann wrote:
> On Wednesday 21 March 2012, Haojian Zhuang wrote:
> > The driver isn't used by any other platform. It's only used in
> > arch-pxa & arch-mmp.
> > 
> > I can't find the original mail. But I remembered that maintainer said
> > the driver should be built with any other platform, even with x86. So
> > we aren't allowed to append it in Kconfig.
> 
> I generally make the recommendation that any driver should be able to
> get built on any system. However, in this particular case, it's clearly
> a bug, because you cannot build it together with another platform driver.
> 
> That will change when we get to multiplatform builds on ARM that where we
> actually need to find a way to make that possible.

I have explained an easy way to achieve that to Alan Stern, but he did
not like it. Basically, we should add a platform_device to ehci-hcd.ko
and make ehci-{omap,mv,fsl,atmel,etc} a parent device/driver which
basically passes the correct resources and handle platform-specific
details.

That way, we could build everything together and udev would load the
correct modules for us.

For an example of what I mean, see drivers/usb/dwc3/core.c,
drivers/usb/dwc3/dwc3-omap.c, and drivers/usb/dwc3/dwc3-pci.c

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: kirkwood devicetree respin

[Arnd Bergmann]

On Wednesday 21 March 2012, Felipe Balbi wrote:
> I have explained an easy way to achieve that to Alan Stern, but he did
> not like it. Basically, we should add a platform_device to ehci-hcd.ko
> and make ehci-{omap,mv,fsl,atmel,etc} a parent device/driver which
> basically passes the correct resources and handle platform-specific
> details.
> 
> That way, we could build everything together and udev would load the
> correct modules for us.
> 
> For an example of what I mean, see drivers/usb/dwc3/core.c,
> drivers/usb/dwc3/dwc3-omap.c, and drivers/usb/dwc3/dwc3-pci.c

While that way clearly works and can solve the problem we have
with ehci today, I think it's easier and more consistent with
other drivers to do it the opposite way, as we have recently
discussed in the context of ohci:

Make the base driver a loadable module that does not register
any struct device_driver at all, but just exports functions to
other module. The hardware specific drivers then each register
to their own bus and use the functions exported by the
main driver, with a way to override them with their own versions.

Alan suggested a variation of that where we actually export
a default "struct hc_driver" instead of the individual functions,
which has the advantage of having to export fewer symbols, but
otherwise is similar to what we do elsewhere.

See libata, sdhci, serial-8250 or xhci for examples of this way.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: kirkwood devicetree respin

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1972 bytes --]

On Wed, Mar 21, 2012 at 01:17:38PM +0000, Arnd Bergmann wrote:
> On Wednesday 21 March 2012, Felipe Balbi wrote:
> > I have explained an easy way to achieve that to Alan Stern, but he did
> > not like it. Basically, we should add a platform_device to ehci-hcd.ko
> > and make ehci-{omap,mv,fsl,atmel,etc} a parent device/driver which
> > basically passes the correct resources and handle platform-specific
> > details.
> > 
> > That way, we could build everything together and udev would load the
> > correct modules for us.
> > 
> > For an example of what I mean, see drivers/usb/dwc3/core.c,
> > drivers/usb/dwc3/dwc3-omap.c, and drivers/usb/dwc3/dwc3-pci.c
> 
> While that way clearly works and can solve the problem we have
> with ehci today, I think it's easier and more consistent with
> other drivers to do it the opposite way, as we have recently
> discussed in the context of ohci:
> 
> Make the base driver a loadable module that does not register
> any struct device_driver at all, but just exports functions to
> other module. The hardware specific drivers then each register
> to their own bus and use the functions exported by the
> main driver, with a way to override them with their own versions.
> 
> Alan suggested a variation of that where we actually export
> a default "struct hc_driver" instead of the individual functions,
> which has the advantage of having to export fewer symbols, but
> otherwise is similar to what we do elsewhere.
> 
> See libata, sdhci, serial-8250 or xhci for examples of this way.

that is likely to work, indeed. My suggestion would make PM simpler
though and avoid some code duplication in the long run, meaning that
ehci-hcd.ko could have its own dev_pm_ops which e.g. knows how to save
EHCI's context while ehci-omap.c knows how to save OMAP-specific
context, toggle clocks, set pads in safe mode (when needed) and so on.

But fair enough, both ways would work fine.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

EHCI_MV confusion was: Re: kirkwood devicetree respin

[Jason Cooper]

On Tue, Mar 20, 2012 at 11:54:53PM -0400, Nicolas Pitre wrote:
> On Tue, 20 Mar 2012, Arnd Bergmann wrote:
> 
> > ehci-orion only makes sense on PLAT_ORION, and they never have any
> > other platform ehci driver.
> > 
> > ehci-mv only makese sense on PLAT_PXA, and they  also don't have any
> > other platform ehci driver.
> 
> BTW, as someone who once was the active maintainer for PXA, and later 
> the active maintainer for Orion, I should say that ehci-mv is a terribly 
> bad name (as demonstrated, not all Marvell SOCs use ehci-mv) and 
> confusion would be much reduced if it was renamed to ehci-pxa.

How about like this (to differentiate against pxa168):

diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 353cdd4..e1bc205 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -194,8 +194,8 @@ config USB_EHCI_S5P
        help
         Enable support for the S5P SOC's on-chip EHCI controller.
 
-config USB_EHCI_MV
-       bool "EHCI support for Marvell on-chip controller"
+config USB_EHCI_MV_PXA
+       bool "EHCI support for Marvell PXA on-chip controller"
        depends on USB_EHCI_HCD
        select USB_EHCI_ROOT_HUB_TT
        ---help---
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index a007a9f..4e1ec71 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1371,8 +1371,8 @@ MODULE_LICENSE ("GPL");
 #define PLATFORM_DRIVER                ehci_xls_driver
 #endif
 
-#ifdef CONFIG_USB_EHCI_MV
-#include "ehci-mv.c"
+#ifdef CONFIG_USB_EHCI_MV_PXA
+#include "ehci-mvpxa.c"
 #define        PLATFORM_DRIVER         ehci_mv_driver
 #endif

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: EHCI_MV confusion was: Re: kirkwood devicetree respin

[Alan Stern]

On Wed, 21 Mar 2012, Jason Cooper wrote:

> On Tue, Mar 20, 2012 at 11:54:53PM -0400, Nicolas Pitre wrote:
> > On Tue, 20 Mar 2012, Arnd Bergmann wrote:
> > 
> > > ehci-orion only makes sense on PLAT_ORION, and they never have any
> > > other platform ehci driver.
> > > 
> > > ehci-mv only makese sense on PLAT_PXA, and they  also don't have any
> > > other platform ehci driver.
> > 
> > BTW, as someone who once was the active maintainer for PXA, and later 
> > the active maintainer for Orion, I should say that ehci-mv is a terribly 
> > bad name (as demonstrated, not all Marvell SOCs use ehci-mv) and 
> > confusion would be much reduced if it was renamed to ehci-pxa.
> 
> How about like this (to differentiate against pxa168):
> 
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index 353cdd4..e1bc205 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -194,8 +194,8 @@ config USB_EHCI_S5P
>         help
>          Enable support for the S5P SOC's on-chip EHCI controller.
>  
> -config USB_EHCI_MV
> -       bool "EHCI support for Marvell on-chip controller"
> +config USB_EHCI_MV_PXA
> +       bool "EHCI support for Marvell PXA on-chip controller"
>         depends on USB_EHCI_HCD
>         select USB_EHCI_ROOT_HUB_TT
>         ---help---
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index a007a9f..4e1ec71 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -1371,8 +1371,8 @@ MODULE_LICENSE ("GPL");
>  #define PLATFORM_DRIVER                ehci_xls_driver
>  #endif
>  
> -#ifdef CONFIG_USB_EHCI_MV
> -#include "ehci-mv.c"
> +#ifdef CONFIG_USB_EHCI_MV_PXA
> +#include "ehci-mvpxa.c"
>  #define        PLATFORM_DRIVER         ehci_mv_driver
>  #endif

You guys have spent an awful lot of time and effort beating a dead 
horse.  See commit a219b666e89bd2f7810b4eaaf4d7382ad0e6ecb1 (usb: host: 
add dependence for USB_EHCI_MV).

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: EHCI_MV confusion was: Re: kirkwood devicetree respin

[Jason Cooper]

On Wed, Mar 21, 2012 at 09:56:24AM -0400, Alan Stern wrote:
> On Wed, 21 Mar 2012, Jason Cooper wrote:
> 
> > On Tue, Mar 20, 2012 at 11:54:53PM -0400, Nicolas Pitre wrote:
> > > On Tue, 20 Mar 2012, Arnd Bergmann wrote:
> > > 
> > > > ehci-orion only makes sense on PLAT_ORION, and they never have any
> > > > other platform ehci driver.
> > > > 
> > > > ehci-mv only makese sense on PLAT_PXA, and they  also don't have any
> > > > other platform ehci driver.
> > > 
> > > BTW, as someone who once was the active maintainer for PXA, and later 
> > > the active maintainer for Orion, I should say that ehci-mv is a terribly 
> > > bad name (as demonstrated, not all Marvell SOCs use ehci-mv) and 
> > > confusion would be much reduced if it was renamed to ehci-pxa.
> > 
> > How about like this (to differentiate against pxa168):
> > 
> > diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> > index 353cdd4..e1bc205 100644
> > --- a/drivers/usb/host/Kconfig
> > +++ b/drivers/usb/host/Kconfig
> > @@ -194,8 +194,8 @@ config USB_EHCI_S5P
> >         help
> >          Enable support for the S5P SOC's on-chip EHCI controller.
> >  
> > -config USB_EHCI_MV
> > -       bool "EHCI support for Marvell on-chip controller"
> > +config USB_EHCI_MV_PXA
> > +       bool "EHCI support for Marvell PXA on-chip controller"
> >         depends on USB_EHCI_HCD
> >         select USB_EHCI_ROOT_HUB_TT
> >         ---help---
> > diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> > index a007a9f..4e1ec71 100644
> > --- a/drivers/usb/host/ehci-hcd.c
> > +++ b/drivers/usb/host/ehci-hcd.c
> > @@ -1371,8 +1371,8 @@ MODULE_LICENSE ("GPL");
> >  #define PLATFORM_DRIVER                ehci_xls_driver
> >  #endif
> >  
> > -#ifdef CONFIG_USB_EHCI_MV
> > -#include "ehci-mv.c"
> > +#ifdef CONFIG_USB_EHCI_MV_PXA
> > +#include "ehci-mvpxa.c"
> >  #define        PLATFORM_DRIVER         ehci_mv_driver
> >  #endif
> 
> You guys have spent an awful lot of time and effort beating a dead 
> horse.  See commit a219b666e89bd2f7810b4eaaf4d7382ad0e6ecb1 (usb: host: 
> add dependence for USB_EHCI_MV).

Ahh, that explains it.  My series was based off of v3.3-rc3, this made
it in v3.3-rc5.

thx,

Jason.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: EHCI_MV confusion was: Re: kirkwood devicetree respin

[Nicolas Pitre]

On Wed, 21 Mar 2012, Alan Stern wrote:

> On Wed, 21 Mar 2012, Jason Cooper wrote:
> 
> > On Tue, Mar 20, 2012 at 11:54:53PM -0400, Nicolas Pitre wrote:
> > > On Tue, 20 Mar 2012, Arnd Bergmann wrote:
> > > 
> > > > ehci-orion only makes sense on PLAT_ORION, and they never have any
> > > > other platform ehci driver.
> > > > 
> > > > ehci-mv only makese sense on PLAT_PXA, and they  also don't have any
> > > > other platform ehci driver.
> > > 
> > > BTW, as someone who once was the active maintainer for PXA, and later 
> > > the active maintainer for Orion, I should say that ehci-mv is a terribly 
> > > bad name (as demonstrated, not all Marvell SOCs use ehci-mv) and 
> > > confusion would be much reduced if it was renamed to ehci-pxa.
> > 
> > How about like this (to differentiate against pxa168):
> > 
> > diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> > index 353cdd4..e1bc205 100644
> > --- a/drivers/usb/host/Kconfig
> > +++ b/drivers/usb/host/Kconfig
> > @@ -194,8 +194,8 @@ config USB_EHCI_S5P
> >         help
> >          Enable support for the S5P SOC's on-chip EHCI controller.
> >  
> > -config USB_EHCI_MV
> > -       bool "EHCI support for Marvell on-chip controller"
> > +config USB_EHCI_MV_PXA
> > +       bool "EHCI support for Marvell PXA on-chip controller"
> >         depends on USB_EHCI_HCD
> >         select USB_EHCI_ROOT_HUB_TT
> >         ---help---
> > diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> > index a007a9f..4e1ec71 100644
> > --- a/drivers/usb/host/ehci-hcd.c
> > +++ b/drivers/usb/host/ehci-hcd.c
> > @@ -1371,8 +1371,8 @@ MODULE_LICENSE ("GPL");
> >  #define PLATFORM_DRIVER                ehci_xls_driver
> >  #endif
> >  
> > -#ifdef CONFIG_USB_EHCI_MV
> > -#include "ehci-mv.c"
> > +#ifdef CONFIG_USB_EHCI_MV_PXA
> > +#include "ehci-mvpxa.c"
> >  #define        PLATFORM_DRIVER         ehci_mv_driver
> >  #endif
> 
> You guys have spent an awful lot of time and effort beating a dead 
> horse.  See commit a219b666e89bd2f7810b4eaaf4d7382ad0e6ecb1 (usb: host: 
> add dependence for USB_EHCI_MV).

I think there is still value in the rename.  "mv" is a really bad 
qualifier for an IP block from a company that has many of them for the 
same purpose.


Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: EHCI_MV confusion was: Re: kirkwood devicetree respin

[Alan Stern]

On Wed, 21 Mar 2012, Nicolas Pitre wrote:

> On Wed, 21 Mar 2012, Alan Stern wrote:
> 
> > On Wed, 21 Mar 2012, Jason Cooper wrote:
> > 
> > > On Tue, Mar 20, 2012 at 11:54:53PM -0400, Nicolas Pitre wrote:
> > > > On Tue, 20 Mar 2012, Arnd Bergmann wrote:
> > > > 
> > > > > ehci-orion only makes sense on PLAT_ORION, and they never have any
> > > > > other platform ehci driver.
> > > > > 
> > > > > ehci-mv only makese sense on PLAT_PXA, and they  also don't have any
> > > > > other platform ehci driver.
> > > > 
> > > > BTW, as someone who once was the active maintainer for PXA, and later 
> > > > the active maintainer for Orion, I should say that ehci-mv is a terribly 
> > > > bad name (as demonstrated, not all Marvell SOCs use ehci-mv) and 
> > > > confusion would be much reduced if it was renamed to ehci-pxa.

> I think there is still value in the rename.  "mv" is a really bad 
> qualifier for an IP block from a company that has many of them for the 
> same purpose.

Then you should discuss this with the driver's maintainer (CC'ed).

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: kirkwood devicetree respin

[Paul Zimmerman]

> From: linux-usb-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-usb-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Felipe Balbi
> Sent: Wednesday, March 21, 2012 6:23 AM
> 
> On Wed, Mar 21, 2012 at 01:17:38PM +0000, Arnd Bergmann wrote:
> > On Wednesday 21 March 2012, Felipe Balbi wrote:
> > > I have explained an easy way to achieve that to Alan Stern, but he did
> > > not like it. Basically, we should add a platform_device to ehci-hcd.ko
> > > and make ehci-{omap,mv,fsl,atmel,etc} a parent device/driver which
> > > basically passes the correct resources and handle platform-specific
> > > details.
> > >
> > > That way, we could build everything together and udev would load the
> > > correct modules for us.
> > >
> > > For an example of what I mean, see drivers/usb/dwc3/core.c,
> > > drivers/usb/dwc3/dwc3-omap.c, and drivers/usb/dwc3/dwc3-pci.c
> >
> > While that way clearly works and can solve the problem we have
> > with ehci today, I think it's easier and more consistent with
> > other drivers to do it the opposite way, as we have recently
> > discussed in the context of ohci:
> >
> > Make the base driver a loadable module that does not register
> > any struct device_driver at all, but just exports functions to
> > other module. The hardware specific drivers then each register
> > to their own bus and use the functions exported by the
> > main driver, with a way to override them with their own versions.
> >
> > Alan suggested a variation of that where we actually export
> > a default "struct hc_driver" instead of the individual functions,
> > which has the advantage of having to export fewer symbols, but
> > otherwise is similar to what we do elsewhere.
> >
> > See libata, sdhci, serial-8250 or xhci for examples of this way.
> 
> that is likely to work, indeed. My suggestion would make PM simpler
> though and avoid some code duplication in the long run, meaning that
> ehci-hcd.ko could have its own dev_pm_ops which e.g. knows how to save
> EHCI's context while ehci-omap.c knows how to save OMAP-specific
> context, toggle clocks, set pads in safe mode (when needed) and so on.
> 
> But fair enough, both ways would work fine.

I would also recommend your/Alan's approach to this over Felipe's way.
We have a DWC3 platform with an (admittedly oddball) PM implementation
that requires the bus driver to have some knowledge of the base driver's
internal state, and needs the two drivers to be able to communicate with
each other. With the base driver being a separate platform device, this
is nearly impossible to achieve in a clean way. If the DWC3 driver was
designed the way you and Alan are recommending, it would be easy.

I'm trying to convince Felipe to change the DWC3 driver design to
accommodate this, but I hold out little hope for that :)

-- 
Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: EHCI_MV confusion was: Re: kirkwood devicetree respin

[Neil Zhang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 1776 bytes --]


> -----Original Message-----
> From: Alan Stern [mailto:stern@rowland.harvard.edu]
> Sent: 2012Äê3ÔÂ22ÈÕ 0:28
> To: Nicolas Pitre
> Cc: Neil Zhang; Jason Cooper; Arnd Bergmann; Andrew Lunn; Jamie Lentin; USB
> list; michael@walle.cc; devicetree-discuss@lists.ozlabs.org
> Subject: Re: EHCI_MV confusion was: Re: kirkwood devicetree respin
> 
> On Wed, 21 Mar 2012, Nicolas Pitre wrote:
> 
> > On Wed, 21 Mar 2012, Alan Stern wrote:
> >
> > > On Wed, 21 Mar 2012, Jason Cooper wrote:
> > >
> > > > On Tue, Mar 20, 2012 at 11:54:53PM -0400, Nicolas Pitre wrote:
> > > > > On Tue, 20 Mar 2012, Arnd Bergmann wrote:
> > > > >
> > > > > > ehci-orion only makes sense on PLAT_ORION, and they never have any
> > > > > > other platform ehci driver.
> > > > > >
> > > > > > ehci-mv only makese sense on PLAT_PXA, and they  also don't have any
> > > > > > other platform ehci driver.
> > > > >
> > > > > BTW, as someone who once was the active maintainer for PXA, and later
> > > > > the active maintainer for Orion, I should say that ehci-mv is a
> terribly
> > > > > bad name (as demonstrated, not all Marvell SOCs use ehci-mv) and
> > > > > confusion would be much reduced if it was renamed to ehci-pxa.
> 
> > I think there is still value in the rename.  "mv" is a really bad
> > qualifier for an IP block from a company that has many of them for the
> > same purpose.

We have submitted a patch to remove ehci-pxa168.c.
And we have plan to rename the driver name for ehci-mv.c.
Thanks.

> 
> Then you should discuss this with the driver's maintainer (CC'ed).
> 
> Alan Stern


Best Regards,
Neil Zhang
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±ºÆâžØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&¢îý»\x05ËÛÔØï¦v¬Îf\x1dp)¹¹br	šê+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹\x1e®w¥¢¸?™¨è­Ú&¢)ߢ^[f

Re: kirkwood devicetree respin

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3325 bytes --]

On Wed, Mar 21, 2012 at 08:06:12PM +0000, Paul Zimmerman wrote:
> > From: linux-usb-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-usb-owner-u79uwXL29Tb/PtFMR13I2A@public.gmane.orgel.org] On Behalf Of Felipe Balbi
> > Sent: Wednesday, March 21, 2012 6:23 AM
> > 
> > On Wed, Mar 21, 2012 at 01:17:38PM +0000, Arnd Bergmann wrote:
> > > On Wednesday 21 March 2012, Felipe Balbi wrote:
> > > > I have explained an easy way to achieve that to Alan Stern, but he did
> > > > not like it. Basically, we should add a platform_device to ehci-hcd.ko
> > > > and make ehci-{omap,mv,fsl,atmel,etc} a parent device/driver which
> > > > basically passes the correct resources and handle platform-specific
> > > > details.
> > > >
> > > > That way, we could build everything together and udev would load the
> > > > correct modules for us.
> > > >
> > > > For an example of what I mean, see drivers/usb/dwc3/core.c,
> > > > drivers/usb/dwc3/dwc3-omap.c, and drivers/usb/dwc3/dwc3-pci.c
> > >
> > > While that way clearly works and can solve the problem we have
> > > with ehci today, I think it's easier and more consistent with
> > > other drivers to do it the opposite way, as we have recently
> > > discussed in the context of ohci:
> > >
> > > Make the base driver a loadable module that does not register
> > > any struct device_driver at all, but just exports functions to
> > > other module. The hardware specific drivers then each register
> > > to their own bus and use the functions exported by the
> > > main driver, with a way to override them with their own versions.
> > >
> > > Alan suggested a variation of that where we actually export
> > > a default "struct hc_driver" instead of the individual functions,
> > > which has the advantage of having to export fewer symbols, but
> > > otherwise is similar to what we do elsewhere.
> > >
> > > See libata, sdhci, serial-8250 or xhci for examples of this way.
> > 
> > that is likely to work, indeed. My suggestion would make PM simpler
> > though and avoid some code duplication in the long run, meaning that
> > ehci-hcd.ko could have its own dev_pm_ops which e.g. knows how to save
> > EHCI's context while ehci-omap.c knows how to save OMAP-specific
> > context, toggle clocks, set pads in safe mode (when needed) and so on.
> > 
> > But fair enough, both ways would work fine.
> 
> I would also recommend your/Alan's approach to this over Felipe's way.
> We have a DWC3 platform with an (admittedly oddball) PM implementation
> that requires the bus driver to have some knowledge of the base driver's
> internal state, and needs the two drivers to be able to communicate with
> each other. With the base driver being a separate platform device, this
> is nearly impossible to achieve in a clean way. If the DWC3 driver was
> designed the way you and Alan are recommending, it would be easy.
> 
> I'm trying to convince Felipe to change the DWC3 driver design to
> accommodate this, but I hold out little hope for that :)

yeah, I'm not thinking on taking that patch, sorry. Didn't you say SNPS
had agreed on rebuilding the FPGA system so that it's a more standard
PCIe implementation ?

I still owe you another possible implementation for the whole PM thing,
sorry about the delay.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: kirkwood devicetree respin

[Paul Zimmerman]

> From: Felipe Balbi [mailto:balbi-l0cyMroinI0@public.gmane.org]
> Sent: Thursday, March 22, 2012 1:13 AM
> 
> On Wed, Mar 21, 2012 at 08:06:12PM +0000, Paul Zimmerman wrote:
> > > From: linux-usb-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-usb-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Felipe
> Balbi
> > > Sent: Wednesday, March 21, 2012 6:23 AM
> > >
> > > On Wed, Mar 21, 2012 at 01:17:38PM +0000, Arnd Bergmann wrote:
> > > > On Wednesday 21 March 2012, Felipe Balbi wrote:
> > > > > I have explained an easy way to achieve that to Alan Stern, but he did
> > > > > not like it. Basically, we should add a platform_device to ehci-hcd.ko
> > > > > and make ehci-{omap,mv,fsl,atmel,etc} a parent device/driver which
> > > > > basically passes the correct resources and handle platform-specific
> > > > > details.
> > > > >
> > > > > That way, we could build everything together and udev would load the
> > > > > correct modules for us.
> > > > >
> > > > > For an example of what I mean, see drivers/usb/dwc3/core.c,
> > > > > drivers/usb/dwc3/dwc3-omap.c, and drivers/usb/dwc3/dwc3-pci.c
> > > >
> > > > While that way clearly works and can solve the problem we have
> > > > with ehci today, I think it's easier and more consistent with
> > > > other drivers to do it the opposite way, as we have recently
> > > > discussed in the context of ohci:
> > > >
> > > > Make the base driver a loadable module that does not register
> > > > any struct device_driver at all, but just exports functions to
> > > > other module. The hardware specific drivers then each register
> > > > to their own bus and use the functions exported by the
> > > > main driver, with a way to override them with their own versions.
> > > >
> > > > Alan suggested a variation of that where we actually export
> > > > a default "struct hc_driver" instead of the individual functions,
> > > > which has the advantage of having to export fewer symbols, but
> > > > otherwise is similar to what we do elsewhere.
> > > >
> > > > See libata, sdhci, serial-8250 or xhci for examples of this way.
> > >
> > > that is likely to work, indeed. My suggestion would make PM simpler
> > > though and avoid some code duplication in the long run, meaning that
> > > ehci-hcd.ko could have its own dev_pm_ops which e.g. knows how to save
> > > EHCI's context while ehci-omap.c knows how to save OMAP-specific
> > > context, toggle clocks, set pads in safe mode (when needed) and so on.
> > >
> > > But fair enough, both ways would work fine.
> >
> > I would also recommend your/Alan's approach to this over Felipe's way.
> > We have a DWC3 platform with an (admittedly oddball) PM implementation
> > that requires the bus driver to have some knowledge of the base driver's
> > internal state, and needs the two drivers to be able to communicate with
> > each other. With the base driver being a separate platform device, this
> > is nearly impossible to achieve in a clean way. If the DWC3 driver was
> > designed the way you and Alan are recommending, it would be easy.
> >
> > I'm trying to convince Felipe to change the DWC3 driver design to
> > accommodate this, but I hold out little hope for that :)
> 
> yeah, I'm not thinking on taking that patch, sorry. Didn't you say SNPS
> had agreed on rebuilding the FPGA system so that it's a more standard
> PCIe implementation ?
> 
> I still owe you another possible implementation for the whole PM thing,
> sorry about the delay.

Dropped the other folks from CC since they probably aren't interested.

The FPGA rework is very low priority, those guys have more important
stuff to work on at the moment. Plus management doesn't understand
why the current design won't work, since the Synopsys driver supports
it OK.

So I'm not sure when the FPGA rework will happen.

-- 
Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: kirkwood devicetree respin

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1846 bytes --]

Hi,

On Thu, Mar 22, 2012 at 06:28:26PM +0000, Paul Zimmerman wrote:
> > > > context, toggle clocks, set pads in safe mode (when needed) and so on.
> > > >
> > > > But fair enough, both ways would work fine.
> > >
> > > I would also recommend your/Alan's approach to this over Felipe's way.
> > > We have a DWC3 platform with an (admittedly oddball) PM implementation
> > > that requires the bus driver to have some knowledge of the base driver's
> > > internal state, and needs the two drivers to be able to communicate with
> > > each other. With the base driver being a separate platform device, this
> > > is nearly impossible to achieve in a clean way. If the DWC3 driver was
> > > designed the way you and Alan are recommending, it would be easy.
> > >
> > > I'm trying to convince Felipe to change the DWC3 driver design to
> > > accommodate this, but I hold out little hope for that :)
> > 
> > yeah, I'm not thinking on taking that patch, sorry. Didn't you say SNPS
> > had agreed on rebuilding the FPGA system so that it's a more standard
> > PCIe implementation ?
> > 
> > I still owe you another possible implementation for the whole PM thing,
> > sorry about the delay.
> 
> Dropped the other folks from CC since they probably aren't interested.
> 
> The FPGA rework is very low priority, those guys have more important
> stuff to work on at the moment. Plus management doesn't understand
> why the current design won't work, since the Synopsys driver supports
> it OK.
> 
> So I'm not sure when the FPGA rework will happen.

Ok, so let's work with what we have now. I'll try to shuffle your
patches around to the way I think they could/should be done, but I'll
need your help testing since I don't have any hibernation-enabled
version of the IP at hand. Hope it's ok with you ;-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: kirkwood devicetree respin

[Paul Zimmerman]

> From: Felipe Balbi [mailto:balbi-l0cyMroinI0@public.gmane.org]
> Sent: Tuesday, March 27, 2012 2:05 AM
> 
> Hi,
> 
> On Thu, Mar 22, 2012 at 06:28:26PM +0000, Paul Zimmerman wrote:
> > > > > context, toggle clocks, set pads in safe mode (when needed) and so on.
> > > > >
> > > > > But fair enough, both ways would work fine.
> > > >
> > > > I would also recommend your/Alan's approach to this over Felipe's way.
> > > > We have a DWC3 platform with an (admittedly oddball) PM implementation
> > > > that requires the bus driver to have some knowledge of the base driver's
> > > > internal state, and needs the two drivers to be able to communicate with
> > > > each other. With the base driver being a separate platform device, this
> > > > is nearly impossible to achieve in a clean way. If the DWC3 driver was
> > > > designed the way you and Alan are recommending, it would be easy.
> > > >
> > > > I'm trying to convince Felipe to change the DWC3 driver design to
> > > > accommodate this, but I hold out little hope for that :)
> > >
> > > yeah, I'm not thinking on taking that patch, sorry. Didn't you say SNPS
> > > had agreed on rebuilding the FPGA system so that it's a more standard
> > > PCIe implementation ?
> > >
> > > I still owe you another possible implementation for the whole PM thing,
> > > sorry about the delay.
> >
> > Dropped the other folks from CC since they probably aren't interested.
> >
> > The FPGA rework is very low priority, those guys have more important
> > stuff to work on at the moment. Plus management doesn't understand
> > why the current design won't work, since the Synopsys driver supports
> > it OK.
> >
> > So I'm not sure when the FPGA rework will happen.
> 
> Ok, so let's work with what we have now. I'll try to shuffle your
> patches around to the way I think they could/should be done, but I'll
> need your help testing since I don't have any hibernation-enabled
> version of the IP at hand. Hope it's ok with you ;-)

Sure. I'd like to make some progress on this.

-- 
Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: kirkwood devicetree respin

[Grant Likely]

On Tue, 13 Mar 2012 15:57:49 +0000, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Monday 12 March 2012, Jason Cooper wrote:
> > I haven't had a chance to push this series to the ml yet (maybe
> > tonight), but I wanted to give Jamie something better to go off of.
> > 
> > Only issue I have (I think unrelated) is that orion-ehci isn't coming
> > up.  But these patches don't touch that, so I think it might be my
> > config again.  Although it doesn't look like it...
> 
> I looked at your patches but also couldn't find anything that hints
> at why ehci would break.
> 
> > Also, serial comes up, works without earlyprintk, but says irq = 0.
> > Could be due to the way I setup serial in dtsi/dts.
> 
> Well, in this series you don't have any interrupt-controller node
> in the device tree, so the interrupt number lookup fails for
> any irq you put into the device tree.
> 
> I wanted to understand how this works for myself, so I've tried
> implementing it in the patch below. This probably doesn't work
> right away, but it should give you an idea of what needs to
> be done.
> 
> It is based on the latest irqdomain series from
> git://git.secretlab.ca/git/linux-2.6.git irqdomain/next
> together with your own patches.
> 
> Grant, can you have a look at this? I'm trying to help out
> multiple people right now who are converting a number of platforms
> to devicetree. It would be good to know if I'm giving the right
> recommendations.
> 
> Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> 
> diff --git a/arch/arm/boot/dts/kirkwood.dtsi b/arch/arm/boot/dts/kirkwood.dtsi
> index 3474ef8..762f8e2 100644
> --- a/arch/arm/boot/dts/kirkwood.dtsi
> +++ b/arch/arm/boot/dts/kirkwood.dtsi
> @@ -2,6 +2,7 @@
>  
>  / {
>  	compatible = "mrvl,kirkwood";
> +	interrupt-parent = <&irq>;
>  
>  	ocp@f1000000 {
>  		compatible = "simple-bus";
> @@ -9,6 +10,22 @@
>  		#address-cells = <1>;
>  		#size-cells = <1>;
>  
> +		irq: irq@f1020204 {
> +			compatible = "mrvl,kirkwood-irq", "mrvl,orion-irq";
> +			interrupt-controller;
> +			#interrupt-cells = <1>;
> +			reg = <0x20204 0x4 0x14>;
> +		};
> +
> +		gpio: gpio@f1010100 {
> +			compatible = "mrvl,kirkwood-gpio", "mrvl,orion-gpio";
> +			gpio-controller;
> +			interrupt-controller;
> +			#interrupt-cells = <1>;
> +			#gpio-cells = <1>;
> +			reg = <0x10100 0x40 0x10140 0x40>;
> +		};
> +
>  		serial@12000 {
>  			compatible = "ns16550a";
>  			reg = <0x12000 0x100>;
> diff --git a/arch/arm/mach-kirkwood/board-dt.c b/arch/arm/mach-kirkwood/board-dt.c
> index 0819240..67a356c 100644
> --- a/arch/arm/mach-kirkwood/board-dt.c
> +++ b/arch/arm/mach-kirkwood/board-dt.c
> @@ -16,6 +16,7 @@
>  #include <linux/of_platform.h>
>  #include <asm/mach/arch.h>
>  #include <asm/mach/map.h>
> +#include <plat/irq.h>
>  #include <mach/bridge-regs.h>
>  #include "common.h"
>  
> @@ -67,7 +68,7 @@ DT_MACHINE_START(KIRKWOOD_DT, "Marvell Kirkwood (Flattened Device Tree)")
>  	/* Maintainer: Jason Cooper <jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org> */
>  	.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..a0e13d2 100644
> --- a/arch/arm/mach-kirkwood/irq.c
> +++ b/arch/arm/mach-kirkwood/irq.c
> @@ -46,3 +46,4 @@ void __init kirkwood_init_irq(void)
>  	irq_set_chained_handler(IRQ_KIRKWOOD_GPIO_HIGH_16_23,
>  				gpio_irq_handler);
>  }
> +
> diff --git a/arch/arm/plat-orion/include/plat/irq.h b/arch/arm/plat-orion/include/plat/irq.h
> index f05eeab..dc83270 100644
> --- a/arch/arm/plat-orion/include/plat/irq.h
> +++ b/arch/arm/plat-orion/include/plat/irq.h
> @@ -13,5 +13,6 @@
>  
>  void orion_irq_init(unsigned int irq_start, void __iomem *maskaddr);
>  
> +void orion_dt_init_irq(void);
>  
>  #endif
> diff --git a/arch/arm/plat-orion/irq.c b/arch/arm/plat-orion/irq.c
> index 2d5b9c1..2e8e5db 100644
> --- a/arch/arm/plat-orion/irq.c
> +++ b/arch/arm/plat-orion/irq.c
> @@ -12,7 +12,12 @@
>  #include <linux/init.h>
>  #include <linux/irq.h>
>  #include <linux/io.h>
> +#include <linux/irqdomain.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 +37,67 @@ 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_dt_irq_setup(struct device_node *node, struct device_node *parent)
> +{
> +	int i;
> +	void __iomem *reg;
> +
> +	for (i = 0; (reg = of_iomap(node, i)) != NULL; i++) {
> +		orion_irq_init(i * 32, reg);
> +		irq_domain_add_legacy(node, 32, i * 32, 0,
> +					&irq_domain_simple_ops, NULL);

I'm rather late to this dicussion, and it appears that things have
progressed since you posted this initial patch, but I wanted to
comment on this.

I really don't want the irq_domain registration code separated from
the irq_driver itself.  That was done for some of the early DT irq
support code, but that was very much a temporary measure until the
support logic is in place.

What should be done instead is the irq controller driver should be
converted to always use an irq_domain (DT and non-DT).

> +	}
> +
> +	return 0;
> +}
> +
> +static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> +{
> +	orion_gpio_irq_handler((long)irq_data_get_irq_handler_data(&desc->irq_data));
> +}
> +
> +unsigned int irq_orion_gpio_start = 64; /* can be overridden from platform */
> +static int __init orion_dt_gpio_irq_setup(struct device_node *node, struct device_node *parent)
> +{
> +	int i;
> +	void __iomem *reg;
> +	for (i = 0; (reg = of_iomap(node, i)) != NULL; i++) {
> +		int nr_irqs;
> +
> +		for (nr_irqs = 0; nr_irqs < 4; nr_irqs++) {
> +			int parent_irq;
> +			parent_irq = irq_of_parse_and_map(node, i * 4 + nr_irqs);
> +			if (!parent_irq)
> +				break;
> +
> +			irq_domain_add_legacy(node, 8,
> +					irq_orion_gpio_start + nr_irqs * 8,
> +					i * 32 + nr_irqs * 8,
> +					&irq_domain_simple_ops,
> +					(void *)parent_irq);

It has already been pointed out that irq_domain_add_*() can only be
called once for a give *device_node.  However, it is fine and
supported to have a single node cover all of the banks of a gpio
controller.

> +			irq_set_chained_handler(parent_irq, gpio_irq_handler);
> +			irq_set_handler_data(parent_irq,
> +					(void *)(i * 32 + nr_irqs * 8));
> +		}
> +
> +		orion_gpio_init(i * 32, nr_irqs, (u32)reg, 0,
> +				irq_orion_gpio_start);
> +		irq_orion_gpio_start += nr_irqs;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id orion_dt_irq_match[] __initconst = {
> +	{ .compatible = "mrvl,orion-irq", .data = &orion_dt_irq_setup },
> +	{ .compatible = "mrvl,orion-gpio", .data = &orion_dt_gpio_irq_setup },
> +	{ }
> +};
> +
> +void __init orion_dt_init_irq(void)
> +{
> +	of_irq_init(orion_dt_irq_match);
> +}
> +#endif

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies,Ltd.