Dear Arnd Bergmann,

On Tue, 26 Mar 2013 21:10:15 +0000, Arnd Bergmann wrote:

quoted

To which children? Only to the main-intc children? If so,
armada_370_xp_mpic_of_init() would be called with a 'device_node *'
that points to the main-intc, correct? Then it would have to go back up
in the Device Tree to find the msi node? It's doable of course, but
sounds strange, no?

I was thinking of registering two init functions as well.

But then which of the two init functions would do the of_iomap() (or
whatever ioremap()ing function you use) ?

Remember, the very reason what we have *one* driver for both interrupt
controllers is because the registers are completely mixed. So to me
it's really the case of *one* device providing *two* features, like a
device that would be both an Ethernet interface and a SPI controller.
You have a single ->probe() function that gets called when the device
is detected, and this ->probe() function registers both an Ethernet
interface and a SPI controller.

To me, the case we have here is really identical: we have one single set
of registers that provide multiple features.

quoted

To me, the topology of the hardware is really that a single device
provides two features: the main interrupt controller and the MSI
interrupt controller. But I will adapt to whatever DT binding you
propose.

My preference would be to have no sub-nodes but rather make the
code deal with an interrupt controller that has an interrupt domain
for regular IRQs but can also handle MSI.

Hum, ok.

quoted

quoted

I still wonder if the real solution shouldn't instead be to make the
irq domain code MSI aware. For instance, you don't really need a
cell to describe an interrupt because the interrupt number is
not a hardware property. So an MSI using device doesn't really
needs an "msis" or "interrupts" property, just an "msi-parent",
and we can add code to handle as a separate domain even if you
have a single device node that can do both level and message
interrupts.

So the msi-parent property should point to the single mpic node? But
then the IRQ domain for MSI cannot be registered on this single MPIC
node. Then, what would be the first argument of:

        armada_370_xp_msi_domain =
                irq_domain_add_linear(msi_node, PCI_MSI_DOORBELL_NR,
                                      &armada_370_xp_msi_irq_ops, NULL);

and how would the PCIe driver get a pointer to this IRQ domain? (In the
currently proposed code, it just does a irq_find_host(), which sounded
very simple and straightforward).

I guess one way would be to have a single domain that is responsible
for the controller and handles both MSI and legacy interrupts. That
could probably be done without changes to the interrupt domain code.

Another option would be to add an irq_domain_add_msi() function that
creates a domain which is also tied to the same device node but does
not interact with it when going through the legacy interrupts.
You could then add a matching msi_find_host() or irq_find_msi_host()
function to return the domain.

This option seems doable. Not sure yet how to do it exactly, but at
least I understand the idea.

quoted

To clarify: I really don't mind reworking the code, but unfortunately
"make the IRQ domain code MSI aware" is too vague for me to understand
the direction you're thinking of.

I don't have specific code in mind yet, mainly playing around with the
possibilities for now.

Sure, I understand. But I also don't have specific ideas: the current
code works fine for me, and I don't find it really ugly. So if you don't
point me in the direction that you think would make the code less ugly,
then I'm lost.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com