[EMAIL PROTECTED] wrote: > Quoting Uwe Hermann <[EMAIL PROTECTED]>: > > >> On Sat, Oct 27, 2007 at 10:41:34AM -0400, [EMAIL PROTECTED] wrote: >> >>> Here is a patch with mostly trivial fixups for i82801x_lpc.c. The main >>> fixup is the PIRQ Routing Control. >>> >>> 1. These are 8 bit registers not 32. >>> >> Yep. >> >> >> >>> 2. 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? >
uint16_t model; model = pci_read_config16(dev, 0x2); if (model != [ICH model #] && model != [ICH0 model) [Do PIRQE-H] >> >>> +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. >> > > ?????? > You can use similar model checking as above. You might also be able to just use dev->model? >> >>> +} >>> + >>> /* 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. > Me either, but it matched the mew-vms, so I left it. >> >>> - >>> - //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 BTW, there's about a 90% chance this will break the asus mew-vm. Please do, this is for the best ;). I'll fix it later, as soon as I can get the cn700 off my hands. -Corey -- linuxbios mailing list linuxbios@linuxbios.org http://www.linuxbios.org/mailman/listinfo/linuxbios