Re: [coreboot] patch: move audio, ehci, ohci IRQs from irq_table to cs5536 dts

[Carl-Daniel Hailfinger]

Hi Ron,

I merged two mails from you with similar topic to answer them in one swoop.

On 03.05.2008 07:51, ron minnich wrote:
> This is not signed off, as it does not work.
>
> It sets up the interrupts correctly, or at least as defined in the dts:
>
> 00:0f.3 Multimedia audio controller: Advanced Micro Devices [AMD]
> CS5536 [Geode companion] Audio (rev 0)
>         Subsystem: Advanced Micro Devices [AMD] CS5536 [Geode companion] Audio
>         Control: I/O+ Mem- BusMaster- SpecCycle- MemWINV- VGASnoop-
> ParErr+ Stepping- SERR- FastB2B-
>         Status: Cap- 66Mhz+ UDF- FastB2B+ ParErr- DEVSEL=medium
> > TAbort- <TAbort- <MAbort- >SERR- <PERR-
>     Interrupt: pin B routed to IRQ 10
>     Region 0: I/O ports at 1880 [size=128]
>
> 00:0f.4 USB Controller: Advanced Micro Devices [AMD] CS5536 [Geode
> companion] OHC (rev 02) (prog-if 10 )
>         Subsystem: Advanced Micro Devices [AMD] CS5536 [Geode companion] OHC
>         Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop-
> ParErr- Stepping- SERR- FastB2B-
>         Status: Cap+ 66Mhz+ UDF- FastB2B- ParErr- DEVSEL=medium
> > TAbort- <TAbort- <MAbort- >SERR- <PERR-
>     Latency: 0
>     Interrupt: pin D routed to IRQ 5
>     Region 0: Memory at fe016000 (32-bit, non-prefetchable) [size=4K]
>     Capabilities: [40] Power Management version 2
>             Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA
>
> PME(D0+,D1-,D2-,D3hot+,D3cold+)
>                 Status: D0 PME-Enable- DSel=0 DScale=0 PME-
>
> 00:0f.5 USB Controller: Advanced Micro Devices [AMD] CS5536 [Geode
> companion] EHC (rev 02) (prog-if 20 )
>         Subsystem: Advanced Micro Devices [AMD] CS5536 [Geode companion] EHC
>         Control: I/O- Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop-
> ParErr- Stepping- SERR- FastB2B-
>         Status: Cap+ 66Mhz+ UDF- FastB2B- ParErr- DEVSEL=medium
> > TAbort- <TAbort- <MAbort- >SERR- <PERR-
>     Interrupt: pin D routed to IRQ 5
>     Region 0: Memory at fe017000 (32-bit, non-prefetchable) [size=4K]
>     Capabilities: [40] Power Management version 2
>             Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA
>
> PME(D0+,D1-,D2-,D3hot+,D3cold+)
>                 Status: D0 PME-Enable- DSel=0 DScale=0 PME-
>
> But they appear not to be wired internally:
> pbx ~ # cat /proc/interrupts
>            CPU0
>   0:      48248    XT-PIC-XT        timer
>   2:          0    XT-PIC-XT        cascade
>   4:       2333    XT-PIC-XT        serial
>   5:          0    XT-PIC-XT        ohci_hcd:usb1
>   7:          4    XT-PIC-XT
>   8:          0    XT-PIC-XT        rtc
>  10:         21    XT-PIC-XT        eth0
>  14:      13886    XT-PIC-XT        ide0
> NMI:          0   Non-maskable interrupts
> TRM:          0   Thermal event interrupts
> SPU:          0   Spurious interrupts
> ERR:          4
>
> So if I push a USB part in, then I get no interrupts.
>
> If I leave the USB interrupts in the irq_table, I get interrupts.
>
> What piece am I missing here? I assume I'm not wiring something up in
> the x,y,z unrestricted interrupt register.
>
> Further, what interrupts are safe to use? We keep reusing 10, 11, 5,
> etc. Any reason not to use (e.g.) 6 or 13 or 12?
> Is there a Geode rule I need to know?
>
> thanks
>
> ron

On 03.05.2008 06:46, ron minnich wrote:
> (I'm hoping someone will do this :-)
>
> One issue I'm having is that the hfcpci card (or driver) can't
> tolerate shared interrupts.
> But we have shared interrupts on the various cs5536 boards, since we
> put USB interrupt info in the table.
>
> To allow us to have non-shared interrupts for all devices, we need to
> get the USB interrupts out of the PIR table. They are not really
> routed via the standard IRQ router anyway -- they are internal -- and
> don't need to share interrupt #s with the other devices that are
> actually routed via the interrupt routing hardware.
>
> Put simply, having the USB f.3,4,5 devices in the PIR table works, but
> is really a bit of a mistake.

So an abstract model of the hardware would be:
- standard IRQ router, settings from PIR table
- CS5536-internal virtual IRQ router, settings outside PIR table.
> OK, where do we put the IRQ info for the USB devices? In the DTS for
> the cs5536.
>
> SO in DTS for the cs5536 we need three more properties,
> usbf3
> usbf4
> usbf5
>
> with reasonable settings for them. Then in the chipset init code, we
> need to see if these are set and, if so, set the IRQ line in the f.3,
> f.4, and f.5 devices.
>
> If this is confusing I can explain in more detail, but later, I'm
> tired, and the ISDN card just locked up again. How can such a simple
> driver have so many failure modes? Oh, wait, it's not simple, sigh.
>
> but, short form: on the cs5536, USB interrupts should not be described
> in the PIR table, but via settings derived from dts. They should be
> initialized in cs5536 setup code, no in the PIR setup code. That will
> allow us to have non-shared interrupts on the various PCI slots on,
> e.g., alix1c, and allow broken drivers like hfcpci to work.

Sorry, but I have to disagree with the design. Your proposed change
scatters interrupt settings and routing info into totally different
types of files. Either interrupt settings are something we want to have
in the DTS (and that would mean we have to keep the PIR table in the dts
as well) or they are kept in separate tables in irq_tables.h.
> Index: southbridge/amd/cs5536/cs5536.c
>
> --- southbridge/amd/cs5536/cs5536.c	(revision 674)
> +++ southbridge/amd/cs5536/cs5536.c	(working copy)
> @@ -410,6 +410,28 @@
>  #define PADEN_SET		(1 << 7)
>
> /**
> Depending on settings in the config struct, manage audio setup.
> @param sb Southbridge config structure.
> */
>
> +static void audio_init(struct southbridge_amd_cs5536_dts_config *sb)
> +{
> u32 *bar;
> struct msr msr;
> struct device *dev;
> if (!sb->audio_enable)
> return;
> dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
> 	      PCI_DEVICE_ID_AMD_CS5536_AUDIO, 0);

I see dev_pci_find_device crop up more and more often in v3 and it
scares me. Is the device tree code really that bad that we can't avoid
dev_pci_find_device? Or did I misunderstand the benefits of the device tree?
> if (dev) {
> pci_write_config8(dev, PCI_INTERRUPT_LINE, sb->audio_irq);
> }
>
> +}
>
> +/**
> Depending on settings in the config struct, manage USB setup.
> @param sb Southbridge config structure.
>
> @@ -438,9 +460,21 @@
> /* EECP=50h, IST=01h, ASPC=1 */
> *(bar + HCCPARAMS) = 0x00005012;
> /* set interrupt line */
> pci_write_config8(dev, PCI_INTERRUPT_LINE, sb->ehci_irq);
>
> }
>
> dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
> 	      PCI_DEVICE_ID_AMD_CS5536_OHCI, 0);

Same dev_find_pci_device objection here.
> if (dev) {
> /* set interrupt line */
> pci_write_config8(dev, PCI_INTERRUPT_LINE, sb->ohci_irq);
> }
> dev = dev_find_pci_device(PCI_VENDOR_ID_AMD,
> 	      PCI_DEVICE_ID_AMD_CS5536_OTG, 0);

Same dev_find_pci_device objection here.
> if (dev) {
>    	bar = (u32 *) pci_read_config32(dev, PCI_BASE_ADDRESS_0);
> @@ -649,6 +683,8 @@
>
> enable_USB_port4(sb);
> audio_init(sb);
> /* disable unwanted virtual PCI devices */
>  for (i = 0; 0 != sb->unwanted_vpci[i]; i++) {
>   hide_vpci(sb->unwanted_vpci[i]);

I see v3 as a big promise of clean design, readable code and easy
porting and I fiercely try to keep it that way. This motivates me to
work on secret projects which will transform the current v3 code to
fulfill its design destiny. Expect code sometime in July.

Regards,
Carl-Daniel