[PATCH] versatile: don't generate a duplicate IRQ domain

[PATCH] versatile: don't generate a duplicate IRQ domain

[Jamie Iles]

Now that the VIC driver handles the irqdomain natively we don't need to
generate one in the versatile core code.  Longer term we should move the
initialisation of both IRQ controllers to using of_irq_init() but
that'll need a little more work.

Cc: Russell King <linux@arm.linux.org.uk>
Cc: Grant Likely <grant.likely@secretlab.ca>
Signed-off-by: Jamie Iles <jamie@jamieiles.com>
---

I don't have a versatile platform so I can't run-test this, but I
suspect that when booting with device tree without this patch it'll warn
with "error: irq_desc already assigned to a domain".

I don't think this can be converted to of_irq_init() straight away
without having all of the devices using IRQ's from DT and not relying on
static IRQ numbers and without the hardware to test it I don't have
enough confidence to do that.

 arch/arm/mach-versatile/core.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-versatile/core.c b/arch/arm/mach-versatile/core.c
index cbcda61..e716fa4 100644
--- a/arch/arm/mach-versatile/core.c
+++ b/arch/arm/mach-versatile/core.c
@@ -86,12 +86,6 @@ static struct fpga_irq_data sic_irq = {
 #define PIC_MASK	0
 #endif
 
-/* Lookup table for finding a DT node that represents the vic instance */
-static const struct of_device_id vic_of_match[] __initconst = {
-	{ .compatible = "arm,versatile-vic", },
-	{}
-};
-
 static const struct of_device_id sic_of_match[] __initconst = {
 	{ .compatible = "arm,versatile-sic", },
 	{}
@@ -100,7 +94,6 @@ static const struct of_device_id sic_of_match[] __initconst = {
 void __init versatile_init_irq(void)
 {
 	vic_init(VA_VIC_BASE, IRQ_VIC_START, ~0, 0);
-	irq_domain_generate_simple(vic_of_match, VERSATILE_VIC_BASE, IRQ_VIC_START);
 
 	writel(~0, VA_SIC_BASE + SIC_IRQ_ENABLE_CLEAR);
 
-- 
1.7.5.4

[PATCH] versatile: don't generate a duplicate IRQ domain

[Russell King - ARM Linux]

On Fri, Jan 06, 2012 at 04:39:57PM +0000, Jamie Iles wrote:
> Now that the VIC driver handles the irqdomain natively we don't need to
> generate one in the versatile core code.  Longer term we should move the
> initialisation of both IRQ controllers to using of_irq_init() but
> that'll need a little more work.
> 
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Signed-off-by: Jamie Iles <jamie@jamieiles.com>

Please confirm that this patch can be applied ontop of commit
356b95424cfb456e14a59eaa579422ce014c424b
(Merge branch 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/geert/linux-m68k)

> I don't have a versatile platform so I can't run-test this, but I
> suspect that when booting with device tree without this patch it'll warn
> with "error: irq_desc already assigned to a domain".
> 
> I don't think this can be converted to of_irq_init() straight away
> without having all of the devices using IRQ's from DT and not relying on
> static IRQ numbers and without the hardware to test it I don't have
> enough confidence to do that.

What's needed to properly test this?  Note that I don't run these boards
with DT.

[PATCH] versatile: don't generate a duplicate IRQ domain

[Jamie Iles]

Hi Russell,

On Thu, Jan 12, 2012 at 08:37:56PM +0000, Russell King - ARM Linux wrote:
> On Fri, Jan 06, 2012 at 04:39:57PM +0000, Jamie Iles wrote:
> > Now that the VIC driver handles the irqdomain natively we don't need to
> > generate one in the versatile core code.  Longer term we should move the
> > initialisation of both IRQ controllers to using of_irq_init() but
> > that'll need a little more work.
> > 
> > Cc: Russell King <linux@arm.linux.org.uk>
> > Cc: Grant Likely <grant.likely@secretlab.ca>
> > Signed-off-by: Jamie Iles <jamie@jamieiles.com>
> 
> Please confirm that this patch can be applied ontop of commit
> 356b95424cfb456e14a59eaa579422ce014c424b
> (Merge branch 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/geert/linux-m68k)

OK, this does apply, but it doesn't fail in the way I had expected.  
Actually, it doesn't barf at all because the generated domain that my 
patch removes has 0 IRQ's so it never gets attached to the irq_desc's.

However, this patch is still valid as we end up with two domains 
associated with the VIC but we get lucky as the real domain is 
registered first.  I can update the patch description to reflect this if 
you still want to apply it.

Incidentally, I've managed to get the versatile kernel booting under 
qemu (using an experimental build with DT support, up to trying to mount 
a rootfs), which seems to work nicely.  I just need to find a suitable 
rootfs and that'll be a nice test system.

Jamie

[PATCH] versatile: don't generate a duplicate IRQ domain

[Russell King - ARM Linux]

On Thu, Jan 12, 2012 at 09:58:01PM +0000, Jamie Iles wrote:
> Hi Russell,
> 
> On Thu, Jan 12, 2012 at 08:37:56PM +0000, Russell King - ARM Linux wrote:
> > On Fri, Jan 06, 2012 at 04:39:57PM +0000, Jamie Iles wrote:
> > > Now that the VIC driver handles the irqdomain natively we don't need to
> > > generate one in the versatile core code.  Longer term we should move the
> > > initialisation of both IRQ controllers to using of_irq_init() but
> > > that'll need a little more work.
> > > 
> > > Cc: Russell King <linux@arm.linux.org.uk>
> > > Cc: Grant Likely <grant.likely@secretlab.ca>
> > > Signed-off-by: Jamie Iles <jamie@jamieiles.com>
> > 
> > Please confirm that this patch can be applied ontop of commit
> > 356b95424cfb456e14a59eaa579422ce014c424b
> > (Merge branch 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/geert/linux-m68k)
> 
> OK, this does apply, but it doesn't fail in the way I had expected.  
> Actually, it doesn't barf at all because the generated domain that my 
> patch removes has 0 IRQ's so it never gets attached to the irq_desc's.

If you say...

> However, this patch is still valid as we end up with two domains 
> associated with the VIC but we get lucky as the real domain is 
> registered first.  I can update the patch description to reflect this if 
> you still want to apply it.

If you think the patch comments don't reflect what the patch is doing
then yes please.

I don't know how this irq domain stuff works so I really can't say
anything much about this patch other than trust you that it's correct.

[PATCH] versatile: don't generate a duplicate IRQ domain

[Grant Likely]

On Thu, Jan 12, 2012 at 3:14 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Jan 12, 2012 at 09:58:01PM +0000, Jamie Iles wrote:
>> Hi Russell,
>>
>> On Thu, Jan 12, 2012 at 08:37:56PM +0000, Russell King - ARM Linux wrote:
>> > On Fri, Jan 06, 2012 at 04:39:57PM +0000, Jamie Iles wrote:
>> > > Now that the VIC driver handles the irqdomain natively we don't need to
>> > > generate one in the versatile core code. ?Longer term we should move the
>> > > initialisation of both IRQ controllers to using of_irq_init() but
>> > > that'll need a little more work.
>> > >
>> > > Cc: Russell King <linux@arm.linux.org.uk>
>> > > Cc: Grant Likely <grant.likely@secretlab.ca>
>> > > Signed-off-by: Jamie Iles <jamie@jamieiles.com>
>> >
>> > Please confirm that this patch can be applied ontop of commit
>> > 356b95424cfb456e14a59eaa579422ce014c424b
>> > (Merge branch 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/geert/linux-m68k)
>>
>> OK, this does apply, but it doesn't fail in the way I had expected.
>> Actually, it doesn't barf at all because the generated domain that my
>> patch removes has 0 IRQ's so it never gets attached to the irq_desc's.
>
> If you say...
>
>> However, this patch is still valid as we end up with two domains
>> associated with the VIC but we get lucky as the real domain is
>> registered first. ?I can update the patch description to reflect this if
>> you still want to apply it.
>
> If you think the patch comments don't reflect what the patch is doing
> then yes please.
>
> I don't know how this irq domain stuff works so I really can't say
> anything much about this patch other than trust you that it's correct.

This patch looks incomplete to me.  With it applied I don't think the
vic's dt node won't get associated with the irq_domain, which will
make dt irq mapping fail to work.  The node pointer needs to get
passed into the vic initialization routine if the
irq_domain_generate_simple() call is to be removed.

g.

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

[PATCH] versatile: don't generate a duplicate IRQ domain

[Russell King - ARM Linux]

On Thu, Jan 12, 2012 at 04:07:54PM -0700, Grant Likely wrote:
> On Thu, Jan 12, 2012 at 3:14 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Thu, Jan 12, 2012 at 09:58:01PM +0000, Jamie Iles wrote:
> >> Hi Russell,
> >>
> >> On Thu, Jan 12, 2012 at 08:37:56PM +0000, Russell King - ARM Linux wrote:
> >> > On Fri, Jan 06, 2012 at 04:39:57PM +0000, Jamie Iles wrote:
> >> > > Now that the VIC driver handles the irqdomain natively we don't need to
> >> > > generate one in the versatile core code. ?Longer term we should move the
> >> > > initialisation of both IRQ controllers to using of_irq_init() but
> >> > > that'll need a little more work.
> >> > >
> >> > > Cc: Russell King <linux@arm.linux.org.uk>
> >> > > Cc: Grant Likely <grant.likely@secretlab.ca>
> >> > > Signed-off-by: Jamie Iles <jamie@jamieiles.com>
> >> >
> >> > Please confirm that this patch can be applied ontop of commit
> >> > 356b95424cfb456e14a59eaa579422ce014c424b
> >> > (Merge branch 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/geert/linux-m68k)
> >>
> >> OK, this does apply, but it doesn't fail in the way I had expected.
> >> Actually, it doesn't barf at all because the generated domain that my
> >> patch removes has 0 IRQ's so it never gets attached to the irq_desc's.
> >
> > If you say...
> >
> >> However, this patch is still valid as we end up with two domains
> >> associated with the VIC but we get lucky as the real domain is
> >> registered first. ?I can update the patch description to reflect this if
> >> you still want to apply it.
> >
> > If you think the patch comments don't reflect what the patch is doing
> > then yes please.
> >
> > I don't know how this irq domain stuff works so I really can't say
> > anything much about this patch other than trust you that it's correct.
> 
> This patch looks incomplete to me.  With it applied I don't think the
> vic's dt node won't get associated with the irq_domain, which will
> make dt irq mapping fail to work.  The node pointer needs to get
> passed into the vic initialization routine if the
> irq_domain_generate_simple() call is to be removed.

Even with this:

        v->domain.irq_base = irq;
        v->domain.nr_irq = 32;
#ifdef CONFIG_OF_IRQ
        v->domain.of_node = of_node_get(node);
#endif /* CONFIG_OF */
        v->domain.ops = &irq_domain_simple_ops;
        irq_domain_add(&v->domain);

which can be found in vic_register() ?

[PATCH] versatile: don't generate a duplicate IRQ domain

[Jamie Iles]

On Thu, Jan 12, 2012 at 11:18:11PM +0000, Russell King - ARM Linux wrote:
> On Thu, Jan 12, 2012 at 04:07:54PM -0700, Grant Likely wrote:
> > On Thu, Jan 12, 2012 at 3:14 PM, Russell King - ARM Linux
> > <linux@arm.linux.org.uk> wrote:
> > > On Thu, Jan 12, 2012 at 09:58:01PM +0000, Jamie Iles wrote:
> > >> Hi Russell,
> > >>
> > >> On Thu, Jan 12, 2012 at 08:37:56PM +0000, Russell King - ARM Linux wrote:
> > >> > On Fri, Jan 06, 2012 at 04:39:57PM +0000, Jamie Iles wrote:
> > >> > > Now that the VIC driver handles the irqdomain natively we don't need to
> > >> > > generate one in the versatile core code. ?Longer term we should move the
> > >> > > initialisation of both IRQ controllers to using of_irq_init() but
> > >> > > that'll need a little more work.
> > >> > >
> > >> > > Cc: Russell King <linux@arm.linux.org.uk>
> > >> > > Cc: Grant Likely <grant.likely@secretlab.ca>
> > >> > > Signed-off-by: Jamie Iles <jamie@jamieiles.com>
> > >> >
> > >> > Please confirm that this patch can be applied ontop of commit
> > >> > 356b95424cfb456e14a59eaa579422ce014c424b
> > >> > (Merge branch 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/geert/linux-m68k)
> > >>
> > >> OK, this does apply, but it doesn't fail in the way I had expected.
> > >> Actually, it doesn't barf at all because the generated domain that my
> > >> patch removes has 0 IRQ's so it never gets attached to the irq_desc's.
> > >
> > > If you say...
> > >
> > >> However, this patch is still valid as we end up with two domains
> > >> associated with the VIC but we get lucky as the real domain is
> > >> registered first. ?I can update the patch description to reflect this if
> > >> you still want to apply it.
> > >
> > > If you think the patch comments don't reflect what the patch is doing
> > > then yes please.
> > >
> > > I don't know how this irq domain stuff works so I really can't say
> > > anything much about this patch other than trust you that it's correct.
> > 
> > This patch looks incomplete to me.  With it applied I don't think the
> > vic's dt node won't get associated with the irq_domain, which will
> > make dt irq mapping fail to work.  The node pointer needs to get
> > passed into the vic initialization routine if the
> > irq_domain_generate_simple() call is to be removed.
> 
> Even with this:
> 
>         v->domain.irq_base = irq;
>         v->domain.nr_irq = 32;
> #ifdef CONFIG_OF_IRQ
>         v->domain.of_node = of_node_get(node);
> #endif /* CONFIG_OF */
>         v->domain.ops = &irq_domain_simple_ops;
>         irq_domain_add(&v->domain);
> 
> which can be found in vic_register() ?

No, because:

	vic_init()
	  __vic_init() -> with NULL np pointer
	    vic_register()
	
So we won't have the device_node for the VIC.  I think I really need to 
convert the versatile stuff over to the full of_irq_init() method of 
probing, which isn't as bad as I thought as everything is populated from 
the DT there but it looked like it could be registered statically too 
from versatile_init().  I'll try and get everything going from the DT 
now.

Jamie

[PATCH] versatile: don't generate a duplicate IRQ domain

[Russell King - ARM Linux]

On Thu, Jan 12, 2012 at 11:26:11PM +0000, Jamie Iles wrote:
> No, because:
> 
> 	vic_init()
> 	  __vic_init() -> with NULL np pointer
> 	    vic_register()
> 	
> So we won't have the device_node for the VIC.  I think I really need to 
> convert the versatile stuff over to the full of_irq_init() method of 
> probing, which isn't as bad as I thought as everything is populated from 
> the DT there but it looked like it could be registered statically too 
> from versatile_init().  I'll try and get everything going from the DT 
> now.

Won't this make Versatile a DT-only platform?

[PATCH] versatile: don't generate a duplicate IRQ domain

[Jamie Iles]

On Thu, Jan 12, 2012 at 11:28:27PM +0000, Russell King - ARM Linux wrote:
> On Thu, Jan 12, 2012 at 11:26:11PM +0000, Jamie Iles wrote:
> > No, because:
> > 
> > 	vic_init()
> > 	  __vic_init() -> with NULL np pointer
> > 	    vic_register()
> > 	
> > So we won't have the device_node for the VIC.  I think I really need to 
> > convert the versatile stuff over to the full of_irq_init() method of 
> > probing, which isn't as bad as I thought as everything is populated from 
> > the DT there but it looked like it could be registered statically too 
> > from versatile_init().  I'll try and get everything going from the DT 
> > now.
> 
> Won't this make Versatile a DT-only platform?

No, crap explanation by me there.  I'll add a versatile_dt_irq_init() 
that creates the controllers from the DT, for the non DT case everything 
will be as before using the static assignments and static irq_descs.  
I'll make sure to test using both booting methods.

Jamie