Re: [RFC PATCH v2] irqchip: add support for SMP irq router

[Marc Zyngier <marc.zyngier@arm.com>]

Thomas already commented on some aspects of the code, so I'm not going
to do another review (I'll do the next round).

On 20/07/16 12:06, Sebastian Frias wrote:
> Hi Thomas,
> 
> Thanks for your comments.
> I appreciate the time you dedicated to the review.
> I will take your comments into account, in the meanwhile, and since this was
> a RFC, would it be possible to also get some feedback/comments from a
> conceptual point of view?
> 
> I'm copy/pasting some of the questions I attached to the RFC email here:
> 
> ----
> 2) In order to get a virq mapping for the domains associated with the outputs
> (the outputs connected to the GIC) I request a sort of "Fake HW IRQ"
> See irq-tango_v4.c:1400
> 
> 	/* To request a virq we need a HW IRQ, use a "Fake HW IRQ" */
> 	hwirq      = index + irqrouter->input_count + irqrouter->swirq_count;

Why are you even trying to establish a route from a made up interrupt to
a GIC line as the driver is just creating domains? Haven't you realized
that everything gets created on demand, as DT interrupt specifiers gets
parsed?

So the only time where you create a irq_fwspec out of thin air is when
you actually allocate *one* interrupt. And that's the only time. You
don't pre-populate stuff, you don't pre-allocate stuff.

> This feels a bit like a hack, so suggestions are welcome.

Just look at all the existing drivers in the tree that use stacked
domains. They are all based on the same template. At least try and
follow it.

> 
> 3) The file is called 'irq-tango_v4.c' but I think it should match the
> compatible string, so I was thinking of renaming:
> irq-tango_v4.c => irq-sigma_smp_irqrouter.c
> irq-tango_v4.h => irq-sigma_smp_irqrouter.h
> 
> What do you think?

And why should it match? Am I going to rename the GIC driver to be
"arm,gic-400_or_arm,arm11mp-gic_or_arm,arm1176jzf-devchip-gic_or_...c"?

No.

> 
> 4) Do I have to do something more to handle the affinity stuff?
> 
> 6) More of a theoretical question:
> I have to #include <dt-bindings/interrupt-controller/irq-tango_v4.h> from

No you don't. This is for DT people to play with, and I don't care about
it in the irqchip code.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...