Quoting Uwe Hermann uwe@hermann-uwe.de:
On Sat, Oct 27, 2007 at 10:41:34AM -0400, joe@smittys.pointclark.net wrote:
Here is a patch with mostly trivial fixups for i82801x_lpc.c. The main fixup is the PIRQ Routing Control.
- These are 8 bit registers not 32.
Yep.
- There were PIRQ channels that were disabled. This should not be
determined here but by irq_tables.c or by acpi_tables.c on a mainboard level.
Yes, sounds correct. Is this tested on hardware?
Yes, I am able to specify which devices use which IRQ's through irq_tables.c or by acpi_tables.c.
Note: I'll add some remarks about the compatibility of the code for other ICH* southbridges. As far as I know the code was not yet verified for all existing ICH* versions (but should be reasonably portable for ICH-ICH5 or so). See comments below for some hints to make it work on _all_ of the ICH versions.
As a quick indicator, I compared the datasheets for ICH/ICH0 and ICH8 and tried to spot differences. In order to _really_ be sure, we should check _all_ the datasheets of course (I'll probably do that soonish, but not for this review yet).
Index: src/southbridge/intel/i82801xx/i82801xx_lpc.c
--- src/southbridge/intel/i82801xx/i82801xx_lpc.c (revision 2899) +++ src/southbridge/intel/i82801xx/i82801xx_lpc.c (working copy) @@ -34,12 +34,47 @@
#define NMI_OFF 0
+/* PIRQ[n]_ROUT[3:0] - PIRQ Routing Control
- 0x00 - 0000 = Reserved
- 0x01 - 0001 = Reserved
- 0x02 - 0010 = Reserved
- 0x03 - 0011 = IRQ3
- 0x04 - 0100 = IRQ4
- 0x05 - 0101 = IRQ5
- 0x06 - 0110 = IRQ6
- 0x07 - 0111 = IRQ7
- 0x08 - 1000 = Reserved
- 0x09 - 1001 = IRQ9
- 0x0A - 1010 = IRQ10
- 0x0B - 1011 = IRQ11
- 0x0C - 1100 = IRQ12
- 0x0D - 1101 = Reserved
- 0x0E - 1110 = IRQ14
- 0x0F - 1111 = IRQ15
- PIRQ[n]_ROUT[7] - PIRQ Routing Control
- 0x80 - The PIRQ is not routed.
- */
Yep, nice. Seems to be the same for all ICH* southbridges.
+#define PIRQA 0x03 +#define PIRQB 0x05 +#define PIRQC 0x06 +#define PIRQD 0x07 +#define PIRQE 0x09 +#define PIRQF 0x0A +#define PIRQG 0x0B +#define PIRQH 0x0C
Hm, where do these numbers come from?
These are the IRQ's per datasheet that should only be used for PCI devices.
void i82801xx_enable_ioapic(struct device *dev) { uint32_t reg32; volatile uint32_t *ioapic_index = (volatile uint32_t *)0xfec00000; volatile uint32_t *ioapic_data = (volatile uint32_t *)0xfec00010;
- /* Set ACPI base address to 0x1100 (I/O space) */
Is the 0x1100 from the comment correct? Do we want to to be configurable? I'd not mention the "0x1100" in the comment but rather say "PM_BASE_ADDR".
Oh, and why is this in i82801xx_enable_ioapic()? Maybe is should be i82801xx_enable_acpi() or i82801xx_enable_pm()?
I think we should just have one - i82801xx_enable_acpi() - no need to complicate things more.
- pci_write_config32(dev, PMBASE, PM_BASE_ADDR | 1);
PMBASE is the same in ICH/ICH0 and ICH8, same size and location in both, so I guess it's the same for all ICH*.
yes
- /* Enable ACPI I/O and power management */
- pci_write_config8(dev, ACPI_CNTL, 0x10);
ACPI_CTNL is found in ICH and ICH8, same size and location in both, so I guess it's the same for all ICH*.
yes
- reg32 = pci_read_config32(dev, GEN_CNTL); reg32 |= (3 << 7); /* Enable IOAPIC */ reg32 |= (1 << 13); /* Coprocessor error enable */
@@ -58,20 +93,29 @@ die("APIC Error\n");
/* TODO: From i82801ca, needed/useful on other ICH? */
- *ioapic_index = 3; // Select Boot Configuration register
- *ioapic_data = 1; // Use Processor System Bus to deliver interrupts
- *ioapic_index = 3; /* Select Boot Configuration register */
- *ioapic_data = 1; /* Use Processor System Bus to deliver interrupts */
}
void i82801xx_enable_serial_irqs(struct device *dev) {
- /* Set packet length and toggle silent mode bit. */
- pci_write_config8(dev, SERIRQ_CNTL,
(1 << 7) | (1 << 6) | ((21 - 17) << 2) | (0 << 0));- pci_write_config8(dev, SERIRQ_CNTL,
(1 << 7) | (0 << 6) | ((21 - 17) << 2) | (0 << 0));- /* TODO: Explain/#define the real meaning of these magic numbers. */
- /* Set SERIRQ packet length. */
- pci_write_config8(dev, SERIRQ_CNTL, 0xD0);
It's called SIRQ_CNTL on ICH8, but same size and location thankfully.
_But_, you're changing the code here, I think. 0xd0 means "enable sirq" and "continuous mode", while the above code did something different (and it was correct according to this note from the ICH8 datasheet):
NOTE: For systems using Quiet Mode, this bit should be set to 1 (Continuous Mode) for at least one frame after coming out of reset before switching back to Quiet Mode. Failure to do so will result in the ICH8 not recognizing SERIRQ interrupts.
The old code thus switched/toggled bit 6 in order to enter quiet mode, while the new code stays in continuous mode. Is that by design? Why?
Not sure what Quiet mode does but I have 4 different i82801xx boards, and a dump of this register indicated they were all running in Continuous Mode 0xD0 with the original bios.
}
+static void i82801xx_pirq_init(device_t dev) +{
- /* Route PIRQA - PIRQH */
- pci_write_config8(dev, PIRQA_ROUT, PIRQA);
- pci_write_config8(dev, PIRQB_ROUT, PIRQB);
- pci_write_config8(dev, PIRQC_ROUT, PIRQC);
- pci_write_config8(dev, PIRQD_ROUT, PIRQD);
- pci_write_config8(dev, PIRQE_ROUT, PIRQE);
- pci_write_config8(dev, PIRQF_ROUT, PIRQF);
- pci_write_config8(dev, PIRQG_ROUT, PIRQG);
- pci_write_config8(dev, PIRQH_ROUT, PIRQH);
+}
This _almost_ works. It's fine for ICH2-ICH8 or so (I guess), but ICH/ICH0 only have PIRA-PIRQD if I didn't miss something in the datasheet.
So the code needs a small fix to only attempt the writes to A-D on the ICH/ICH0, otherwise it's very nice.
How could this be setup?
+static void gpio_init(device_t dev) +{
- /* Set the value for GPIO base address Register */
- pci_write_config32(dev, GPIO_BASE, 0x00000501);
Careful, this will need fixes for newer ICH*'s.
GPIO_BASE is at 0x58 for ICH, but at 0x48 for ICH8. Same size and meaning of the bits, but the location is different.
Please check all datasheets from ICH-ICH9 (or whatever is the latest) and adapt the code to work for all of them.
???????
- /* Enable GPIO */
- pci_write_config8(dev, GPIO_CNTL, 0x10);
Same for GPIO_CNTL. Same size and meaning, but different location.
??????
+}
/* TODO: Needs serious cleanup/comments. */ void i82801xx_rtc_init(struct device *dev) { @@ -103,40 +155,9 @@ reg32 = pci_read_config32(dev, GEN_STS); rtc_failed |= reg32 & (1 << 2); rtc_init(rtc_failed); -}
-void i82801xx_1f0_misc(struct device *dev) -{
- /* TODO: break this down into smaller functions */
- //move to acpi_enable or something
- /* Set ACPI base address to 0x1100 (I/O space) */
- pci_write_config32(dev, PMBASE, PM_BASE_ADDR | 1);
- /* Enable ACPI I/O and power management */
- pci_write_config8(dev, ACPI_CNTL, 0x10);
- /* Set GPIO base address to 0x1180 (I/O space) */
- pci_write_config32(dev, GPIO_BASE, GPIO_BASE_ADDR | 1);
- /* Enable GPIO */
- pci_write_config8(dev, GPIO_CNTL, 0x10);
- //get rid of?
- /* Route PIRQA to IRQ11, PIRQB to IRQ3, PIRQC to IRQ5, PIRQD to IRQ10 */
- pci_write_config32(dev, PIRQA_ROUT, 0x0A05030B);
- /* Route PIRQE to IRQ7. Leave PIRQF - PIRQH unrouted */
- pci_write_config8(dev, PIRQE_ROUT, 0x07);
Any idea why this exact routing was configured? Specific for some board it was used for back then?
Not sure, hence the fixup.
- //move to i82801xx_init
- /* Prevent LPC disabling, enable parity errors, and SERR# (System
Error) */
- pci_write_config16(dev, PCI_COMMAND, 0x014f);
/* Enable access to the upper 128 byte bank of CMOS RAM */ pci_write_config8(dev, RTC_CONF, 0x04);
- /* Decode 0x3F8-0x3FF (COM1) for COMA port, 0x2F8-0x2FF (COM2) for COMB */
- pci_write_config8(dev, COM_DEC, 0x10);
- /* LPT decode defaults to 0x378-0x37F and 0x778-0x77F
* Floppy decode defaults to 0x3F0-0x3F5, 0x3F7 */- /* Enable: COMA, COMB, LPT, Floppy
* Disable: Microcontroller, Sound, Gameport */- pci_write_config16(dev, LPC_EN, 0x000F);
}
static void enable_hpet(struct device *dev) @@ -167,23 +188,17 @@ int pwr_on = -1; int nmi_option;
- /* Set the value for PCI Command Register */
- pci_write_config16(dev, PCI_COMMAND, 0x000f);
It's 0x014f above but 0x000f here. Rationale?
Parity, and SERR# should be set by OS level not bios.
/* IO APIC initialization */ i82801xx_enable_ioapic(dev);
i82801xx_enable_serial_irqs(dev);
- /* TODO: Find out if this is being used/works */
-#ifdef SUSPICIOUS_LOOKING_CODE
- /* The ICH-4 datasheet does not mention this configuration register.
* This code may have been inherited (incorrectly) from code for* the AMD 766 southbridge, which *does* support this functionality.*/
- /* Set up the PIRQ */
- i82801xx_pirq_init(dev);
- /* Posted memory write enable */
- byte = pci_read_config8(dev, 0x46);
- pci_write_config8(dev, 0x46, byte | (1 << 0));
-#endif
- /* power after power fail */ /* FIXME this doesn't work! */ /* Which state do we want to goto after g3 (power restored)?
@@ -207,6 +222,9 @@ outb(byte, 0x70); }
- /* Set the state of the gpio lines */
- gpio_init(dev);
- /* Initialize the real time clock */ i82801xx_rtc_init(dev);
@@ -215,7 +233,15 @@ /* Initialize isa dma */ isa_dma_init();
- i82801xx_1f0_misc(dev);
- /* Decode 0x3F8-0x3FF (COM1) for COMA port, 0x2F8-0x2FF (COM2) for COMB.
* LPT decode defaults to 0x378-0x37F and 0x778-0x77F* Floppy decode defaults to 0x3F0-0x3F5, 0x3F7*/- pci_write_config8(dev, COM_DEC, 0x10);
COM_DEC won't work for ICH8. We can commit this right now, but please add a comment that it'll only work for ICH-ICH5 (or so) for now.
OK
- /* Set the value for LPC I/F Enables Register */
- pci_write_config16(dev, LPC_EN, 0x300F);
Same for LPC_EN, different on ICH8.
OK
- /* Initialize the High Precision Event Timers, if present */ enable_hpet(dev);
} Index: src/southbridge/intel/i82801xx/i82801xx.h =================================================================== --- src/southbridge/intel/i82801xx/i82801xx.h (revision 2899) +++ src/southbridge/intel/i82801xx/i82801xx.h (working copy) @@ -42,7 +42,13 @@ #define GPIO_BASE_ADDR 0x1180 #define GPIO_CNTL 0x5C #define PIRQA_ROUT 0x60 +#define PIRQB_ROUT 0x61 +#define PIRQC_ROUT 0x62 +#define PIRQD_ROUT 0x63 #define PIRQE_ROUT 0x68 +#define PIRQF_ROUT 0x69 +#define PIRQG_ROUT 0x6A +#define PIRQH_ROUT 0x6B #define COM_DEC 0xE0 #define LPC_EN 0xE6 #define FUNC_DIS 0xF2
Yep, looks good.
Uwe.
http://www.hermann-uwe.de | http://www.holsham-traders.de http://www.crazy-hacks.org | http://www.unmaintained-free-software.org
Thanks - Joe