On Wed, Jan 28, 2015 at 02:51:23PM +0000, Marc Zyngier wrote:
> void __weak pcibios_update_irq(struct pci_dev *dev, int irq)
> {
> - dev_dbg(&dev->dev, "assigning IRQ %02d\n", irq);
> - pci_write_config_byte(dev, PCI_INTERRUPT_LINE, irq);
> + struct irq_data *d;
> +
> + d = irq_get_irq_data(irq);
> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
> + while (d->parent_data)
> + d = d->parent_data;
> +#endif
> + dev_dbg(&dev->dev, "assigning IRQ %02ld\n", d->hwirq);
> + pci_write_config_byte(dev, PCI_INTERRUPT_LINE, d->hwirq);
I'm really not convinced about this being the correct thing to do.
Let's take an older ARM system, such as a Footbridge based system with a
PCI southbridge.
Such a system has IRQs 0-15 as the PCI southbridge ISA interrupts. Then
there are four PCI interrupts provided by the on-board Footbridge.
Right now, PCI devices are programmed with the OS specific interrupt
number - eg:
00:06.1 IDE interface: Contaq Microsystems 82c693 (prog-if 80 [Master])
Flags: medium devsel, IRQ 14
30: 00 00 00 00 00 00 00 00 00 00 00 00 0e 01 00 00
00:06.2 IDE interface: Contaq Microsystems 82c693 (prog-if 00 [])
Flags: medium devsel, IRQ 15
30: 00 00 00 00 00 00 00 00 00 00 00 00 0f 02 00 00
00:06.3 USB Controller: Contaq Microsystems 82c693 (prog-if 10 [OHCI])
Flags: medium devsel, IRQ 12
30: 00 00 00 00 00 00 00 00 00 00 00 00 0c 01 00 00
00:07.0 Mass storage controller: Integrated Technology Express, Inc. IT/ITE8212
Dual channel ATA RAID controller (rev 13)
Flags: bus master, 66MHz, medium devsel, latency 0, IRQ 24
30: 00 00 02 04 80 00 00 00 00 00 00 00 18 01 08 08
00:08.0 Ethernet controller: 3Com Corporation 3c905C-TX/TX-M [Tornado] (rev 74)
Flags: bus master, medium devsel, latency 32, IRQ 22
30: 00 00 06 04 dc 00 00 00 00 00 00 00 16 01 0a 0a
00:09.0 VGA compatible controller: S3 Inc. 86c775/86c785 [Trio 64V2/DX or /GX] (rev 16) (prog-if 00 [VGA controller])
Flags: medium devsel, IRQ 21
30: 00 00 00 0c 00 00 00 00 00 00 00 00 15 01 00 00
What your change would mean is that the IRQs currently being programmed
>= 16 would be programmed into with numbers with 16 removed from them.
This means that legacy stuff (eg on the Southbridge which really do signal
via the ISA IRQ controller) end up using the same number range as those
which take PCI specific IRQs.
This surely is not sane.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
Hi Russell,
On 02/02/15 16:33, Russell King - ARM Linux wrote:
> On Wed, Jan 28, 2015 at 02:51:23PM +0000, Marc Zyngier wrote:
>> void __weak pcibios_update_irq(struct pci_dev *dev, int irq)
>> {
>> - dev_dbg(&dev->dev, "assigning IRQ %02d\n", irq);
>> - pci_write_config_byte(dev, PCI_INTERRUPT_LINE, irq);
>> + struct irq_data *d;
>> +
>> + d = irq_get_irq_data(irq);
>> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
>> + while (d->parent_data)
>> + d = d->parent_data;
>> +#endif
>> + dev_dbg(&dev->dev, "assigning IRQ %02ld\n", d->hwirq);
>> + pci_write_config_byte(dev, PCI_INTERRUPT_LINE, d->hwirq);
>
> I'm really not convinced about this being the correct thing to do.
>
> Let's take an older ARM system, such as a Footbridge based system with a
> PCI southbridge.
>
> Such a system has IRQs 0-15 as the PCI southbridge ISA interrupts. Then
> there are four PCI interrupts provided by the on-board Footbridge.
>
> Right now, PCI devices are programmed with the OS specific interrupt
> number - eg:
>
> 00:06.1 IDE interface: Contaq Microsystems 82c693 (prog-if 80 [Master])
> Flags: medium devsel, IRQ 14
> 30: 00 00 00 00 00 00 00 00 00 00 00 00 0e 01 00 00
>
> 00:06.2 IDE interface: Contaq Microsystems 82c693 (prog-if 00 [])
> Flags: medium devsel, IRQ 15
> 30: 00 00 00 00 00 00 00 00 00 00 00 00 0f 02 00 00
>
> 00:06.3 USB Controller: Contaq Microsystems 82c693 (prog-if 10 [OHCI])
> Flags: medium devsel, IRQ 12
> 30: 00 00 00 00 00 00 00 00 00 00 00 00 0c 01 00 00
>
> 00:07.0 Mass storage controller: Integrated Technology Express, Inc. IT/ITE8212
> Dual channel ATA RAID controller (rev 13)
> Flags: bus master, 66MHz, medium devsel, latency 0, IRQ 24
> 30: 00 00 02 04 80 00 00 00 00 00 00 00 18 01 08 08
>
> 00:08.0 Ethernet controller: 3Com Corporation 3c905C-TX/TX-M [Tornado] (rev 74)
> Flags: bus master, medium devsel, latency 32, IRQ 22
> 30: 00 00 06 04 dc 00 00 00 00 00 00 00 16 01 0a 0a
>
> 00:09.0 VGA compatible controller: S3 Inc. 86c775/86c785 [Trio 64V2/DX or /GX] (rev 16) (prog-if 00 [VGA controller])
> Flags: medium devsel, IRQ 21
> 30: 00 00 00 0c 00 00 00 00 00 00 00 00 15 01 00 00
>
> What your change would mean is that the IRQs currently being programmed
>> = 16 would be programmed into with numbers with 16 removed from them.
> This means that legacy stuff (eg on the Southbridge which really do signal
> via the ISA IRQ controller) end up using the same number range as those
> which take PCI specific IRQs.
>
> This surely is not sane.
I suppose this is ebsa285? I must confess I don't see how to distinguish
the two cases (the GIC case uses a purely virtual number, and the
footbridge case uses something that seems to be physical).
A very easy fix would be to entirely contain this change within #ifdef
CONFIG_IRQ_DOMAIN_HIERARCHY/#endif, but that doesn't fill me with
confidence.
What I don't get is how the hwirq field is set in this case. It probably
isn't very useful (as there is no domain lookup), so I would have hoped
to see irq == hwirq. Obviously, this is not the case. What am I missing?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On Mon, Feb 02, 2015 at 06:08:17PM +0000, Marc Zyngier wrote:
> Hi Russell,
>
> On 02/02/15 16:33, Russell King - ARM Linux wrote:
> > What your change would mean is that the IRQs currently being programmed
> >> = 16 would be programmed into with numbers with 16 removed from them.
> > This means that legacy stuff (eg on the Southbridge which really do signal
> > via the ISA IRQ controller) end up using the same number range as those
> > which take PCI specific IRQs.
> >
> > This surely is not sane.
>
> I suppose this is ebsa285? I must confess I don't see how to distinguish
> the two cases (the GIC case uses a purely virtual number, and the
> footbridge case uses something that seems to be physical).
Yep.
> A very easy fix would be to entirely contain this change within #ifdef
> CONFIG_IRQ_DOMAIN_HIERARCHY/#endif, but that doesn't fill me with
> confidence.
>
> What I don't get is how the hwirq field is set in this case. It probably
> isn't very useful (as there is no domain lookup), so I would have hoped
> to see irq == hwirq. Obviously, this is not the case. What am I missing?
Well, it depends how this register is to be interpreted. The PCI specs
say that it's used to communicate the interrupt line routing information
from POST to device drivers and operating systems. "The value in this
register tells which input of the system interrupt controller(s) the
device's interrupt pin is connected to." Note the plural there - which
imples that any per-interrupt controller numbering scheme is quite
certainly wrong.
It also implies that there is a known, shared numberspace between POST and
OS implementations on a platform which is used by PCI devices to describe
how the PCI interrupts are wired up.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.