[LinuxBIOS] [v2][PATCH] Trivial fixups for i82801xx_lpc
Corey Osgood
corey.osgood at gmail.com
Sun Oct 28 02:33:58 CEST 2007
joe at smittys.pointclark.net wrote:
> Quoting Uwe Hermann <uwe at hermann-uwe.de>:
>
>
>> On Sat, Oct 27, 2007 at 10:41:34AM -0400, joe at smittys.pointclark.net 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
More information about the coreboot
mailing list