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?
>
>
>> +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
--
linuxbios mailing list
linuxbios@linuxbios.org
http://www.linuxbios.org/mailman/listinfo/linuxbios
