On Tue, Mar 26, 2013 at 05:52:22PM +0100, Thomas Petazzoni wrote: > This commit introduces the support for the MSI interrupts in the > armada-370-xp interrupt controller driver. It registers an IRQ domain > associated with the 'msi' node in the Device Tree, which the PCI > controller will refer to in a followup commit. I've got some MSI stuff working on Kirkwood using the doorbells, so I can confirm this general approach works in HW. What do you think it would take to extend this to other Marvell SOCs? BTW, in my testing on Kirkwood I found that register ..40010 in the PEX had to be set to the internal register base to make this work. Did you find something similar? > The MSI interrupts use the 16 high doorbells, and are therefore > notified using IRQ1 of the main interrupt controller. I was initially a bit surprised this was the high doorbell bits, but I checked and it is right, the 16 bit MSI maps to the high two bytes in the 32 bit MemWr TLP. FWIW, MSI-X is not restricted to 16 bits, so if you can detect from the PCI layer if it is setting up MSI or MSI-X you could allocate low bits first to MSI-X and high bits first to MSI, increasing the number of available MSI/MSI-X vectors. > + - marvell,doorbell: Gives the physical address at which PCIe > + devices should write to signal an MSI interrupt. Why is this necessary? Can't the doorbell register physical address be computed by the driver? AFAIK there is no possibility for address translation on SOC inbound TLPs. Thinking about it a bit, maybe less magic code is needed here, be explicit about the available interrupts in the DT: pcie-controller { msi-interrupts = <0xd0020a04 (1<<16) &msi 16 0xd0020a04 (1<<17) &msi 17 [..] msi-x-interrupts = <0xd0020a04 (1<<1) &msi 1 0xd0020a04 (1<<2) &msi 2 [..] There is a better chance of that supporting other Marvell SOCs.. Not sure, just throwing it out there. Regarding the sub node, I'm not sure it should be necessary. You don't need to differentiate the MSI vs non-MSI case in the doorbell register at the interrupt controller level. Eg this .. > +#ifdef CONFIG_PCI_MSI > +static struct irq_chip armada_370_xp_msi_irq_chip = { > + .name = "armada_370_xp_msi_irq", > + .irq_enable = unmask_msi_irq, > + .irq_disable = mask_msi_irq, > + .irq_mask = mask_msi_irq, > + .irq_unmask = unmask_msi_irq, > +}; .. isn't necessary for doorbell. With MSI cases on other archs there is no local MSI interrupt controller, the MSI MemWr TLP directly creates an interrupt at the CPU. This requires using the *_msi_irq functions to control the interrupt at the source, since there is no control at the destination. However, Marvell's doorbell can be controlled at the destination, so it is better to handle it that way, especially since it creates symmetry with the IPI usage. I used: priv->gc = irq_alloc_generic_chip("kirkwood_doorbell_irq", 1, irq_base, priv->base, handle_fasteoi_irq); [..] ct->regs.mask = 4; ct->regs.eoi = 0; ct->chip.irq_eoi = irq_gc_eoi_inv; ct->chip.irq_mask = irq_gc_mask_clr_bit; ct->chip.irq_unmask = irq_gc_mask_set_bit; Which should work correctly for the IPI and MSI cases. The handle_fasteoi_irq is important. > #ifdef CONFIG_SMP > void armada_mpic_send_doorbell(const struct cpumask *mask, unsigned int irq) > { > @@ -220,12 +280,39 @@ armada_370_xp_handle_irq(struct pt_regs *regs) > if (irqnr > 1022) > break; > > - if (irqnr > 0) { > + if (irqnr > 1) { > irqnr = irq_find_mapping(armada_370_xp_mpic_domain, > irqnr); > handle_IRQ(irqnr, regs); > continue; > } > + > +#ifdef CONFIG_PCI_MSI > + /* MSI handling */ > + if (irqnr == 1) { > + u32 msimask, msinr; > + > + msimask = readl_relaxed(per_cpu_int_base + > + ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS) > + & PCI_MSI_DOORBELL_MASK; > + > + writel(~PCI_MSI_DOORBELL_MASK, per_cpu_int_base + > + ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS); Be careful here, the ordering of the EOI in relation to the handler is important. If you use my stuff from above then the MSI and IPI cases are identical and the core code handles ack'ing the doorbell via irq_eoi at the right moment, you can just uniformly iterate over all 32 bits and there is no need to care if it is MSI or IPI. Also, I'm not super familiar with the irq stuff, but is irq_find_mapping the best way? Most of the drivers I looked at used irq_alloc_descs to get a contiguous range of irq numbers and then just used a simple offset in the handle_irq... Jason -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html