On Tuesday 26 March 2013, Thomas Petazzoni wrote:

Dear Arnd Bergmann,

On Tue, 26 Mar 2013 18:38:22 +0000, Arnd Bergmann wrote:

quoted

quoted

    Note that both the parent and the child node need to have the
    'interrupt-controller' empty property:
    
      * The interrupt-controller property is needed in the main
    interrupt controller node (interrupt-controller at d0020000)
because the of_irq_init() function skips nodes that are matching
the given compatible string, but that don't have the
interrupt-controller property.
    
     * The interrupt-controller property is needed in the child
    interrupt controller node (main-intc at d0020000) otherwise the
    resolution done by of_irq_map_one() doesn't work.

If you add compatible properties to the children, that would work
I suppose.

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.

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.

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.

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.

	Arnd