[LinuxBIOS] [v2][PATCH] Trivial fixups for i82801xx_lpc
Corey Osgood
corey.osgood at gmail.com
Tue Oct 30 02:17:09 CET 2007
joe at smittys.pointclark.net wrote:
> Quoting Corey Osgood <corey.osgood at gmail.com>:
>
>> joe at smittys.pointclark.net wrote:
>>> Index: src/southbridge/intel/i82801xx/i82801xx_lpc.c
>>> ===================================================================
>>> --- src/southbridge/intel/i82801xx/i82801xx_lpc.c (revision 2901)
>>> +++ src/southbridge/intel/i82801xx/i82801xx_lpc.c (working copy)
>>> @@ -32,14 +32,55 @@
>>> #include <arch/io.h>
>>> #include "i82801xx.h"
>>>
>>> +/* ACPI Base Address Register 32 bits Default 0x00000001 */
>>> +#define PMBASE_ADDR 0x00000401
>>
>> Please use PMBASE_ADDR 0x0400 (or however many leading 0s, it shouldn't
>> matter). When setting the register, use (PMBASE_ADDR | 1). You
>> *shouldn't* actually need the 1 at all, the datasheets say this bit is
>> hardwired to 1, but better safe then sorry. This way, if you want to set
>> registers in the PMIO, you can do outb(PMBASE_ADDR + <offset>, data) (or
>> is it data first? anyway, you get the point). Also push back off into
>> the header with the rest of the defines, please.
>>
> OK, I can do the (PMBASE_ADDR | 1) If it makes things easier. Little
> confused about putting this in the header though. I thought the
> general rule is only to put address's in the header but not actual
> values.
>
>>
>> Eventually, I think we can do something similar to uwe's idex_enable
>> with these IO bases, because they do tend to vary by default from board
>> to board, and we may run across a situation where the default that works
>> for everyone else simply doesn't work for a particular board/chip
>>
>>> +
>>> #define NMI_OFF 0
>>>
>>> -void i82801xx_enable_ioapic(struct device *dev)
>>> +/* 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.
>>> + */
>>> +
>>> +#define PIRQA 0x03
>>> +#define PIRQB 0x05
>>> +#define PIRQC 0x06
>>> +#define PIRQD 0x07
>>> +#define PIRQE 0x09
>>> +#define PIRQF 0x0A
>>> +#define PIRQG 0x0B
>>> +#define PIRQH 0x0C
>>
>> ^^^This is good
>>
>>> +
>>> +/* GPIO Base Address Register 32 bits Default 0x00000001 */
>>> +#define GPIO_BASE_ADDR 0x00000501
>>
>> Again, same thing here, although probably won't do anything with the
>> GPIOs, it's nice to be able to.
>>
> Ok, but same as above.
>>
>>> +
>>> +void i82801xx_enable_apic(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 (I/O space) */
>>> + pci_write_config32(dev, PMBASE, PMBASE_ADDR);
>>> + /* Enable ACPI I/O and power management */
>>> + pci_write_config8(dev, ACPI_CNTL, 0x10);
>>> +
>>> reg32 = pci_read_config32(dev, GEN_CNTL);
>>> reg32 |= (3 << 7); /* Enable IOAPIC */
>>> reg32 |= (1 << 13); /* Coprocessor error enable */
>>> @@ -58,8 +99,8 @@
>>> 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)
>>> @@ -72,22 +113,70 @@
>>> /* TODO: Explain/#define the real meaning of these magic
>>> numbers. */
>>> }
>>>
>>> -void i82801xx_lpc_route_dma(struct device *dev, uint8_t mask)
>>> +static void i82801xx_pirq_init(device_t dev, uint16_t ich_model)
>>> {
>>> - uint16_t reg16;
>>> - int i;
>>> + /* Route PIRQA - PIRQD */
>>> + 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);
>>>
>>> - reg16 = pci_read_config16(dev, PCI_DMA_CFG);
>>> - reg16 &= 0x300;
>>> - for (i = 0; i < 8; i++) {
>>> - if (i == 4)
>>> - continue;
>>> - reg16 |= ((mask & (1 << i)) ? 3 : 1) << (i * 2);
>>> + /* Route PIRQE - PIRQH for ICH2-ICH9 */
>>> + if (ich_model >= 0x2440) {
>>> + 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);
>>> }
>>> - pci_write_config16(dev, PCI_DMA_CFG, reg16);
>>> }
>>>
>>> -/* TODO: Needs serious cleanup/comments. */
>>> +static void i82801xx_power_options(device_t dev)
>>> +{
>>> + uint8_t byte;
>>> + int pwr_on = -1;
>>> + int nmi_option;
>>> +
>>> + /* power after power fail */
>>> + /* FIXME this doesn't work! */
>>> + /* Which state do we want to goto after g3 (power restored)?
>>> + * 0 == S0 Full On
>>> + * 1 == S5 Soft Off
>>> + */
>>> + pci_write_config8(dev, GEN_PMCON_3, pwr_on ? 0 : 1);
>>> + printk_info("Set power %s if power fails\n", pwr_on ? "on" :
>>> "off");
>>> +
>>> + /* Set up NMI on errors */
>>> + byte = inb(0x61);
>>> + byte &= ~(1 << 3); /* IOCHK# NMI Enable */
>>> + byte &= ~(1 << 2); /* PCI SERR# Enable */
>>> + outb(byte, 0x61);
>>> + byte = inb(0x70);
>>> +
>>> + nmi_option = NMI_OFF;
>>> + get_option(&nmi_option, "nmi");
>>> + if (nmi_option) {
>>> + byte &= ~(1 << 7); /* Set NMI */
>>> + outb(byte, 0x70);
>>> + }
>>> +}
>>> +
>>> +static void gpio_init(device_t dev, uint16_t ich_model)
>>> +{
>>> + /* Set the value for GPIO base address Register
>>> + * and Enable GPIO.
>>> + * Note: ICH-ICH5 registers differ from ICH6-ICH9.
>>> + */
>>> + if (ich_model <= 0x24D0) {
>>> + pci_write_config32(dev, GPIO_BASE, GPIO_BASE_ADDR);
>>> + pci_write_config8(dev, GPIO_CNTL, 0x10);
>>> + } else if (ich_model >= 0x2640) {
>>> + pci_write_config32(dev, GPIOBASE, GPIO_BASE_ADDR);
>>> + pci_write_config8(dev, GC, 0x10);
>>> + }
>>> +
>>> +
>>> +}
>>> +
>>> void i82801xx_rtc_init(struct device *dev)
>>> {
>>> uint8_t reg8;
>>> @@ -103,40 +192,41 @@
>>> reg32 = pci_read_config32(dev, GEN_STS);
>>> rtc_failed |= reg32 & (1 << 2);
>>> rtc_init(rtc_failed);
>>> +
>>> + /* Enable access to the upper 128 byte bank of CMOS RAM */
>>> + pci_write_config8(dev, RTC_CONF, 0x04);
>>> }
>>>
>>> -void i82801xx_1f0_misc(struct device *dev)
>>> +void i82801xx_lpc_route_dma(struct device *dev, uint8_t mask)
>>> {
>>> - /* TODO: break this down into smaller functions */
>>> + uint16_t reg16;
>>> + int i;
>>>
>>> - //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);
>>> + reg16 = pci_read_config16(dev, PCI_DMA_CFG);
>>> + reg16 &= 0x300;
>>> + for (i = 0; i < 8; i++) {
>>> + if (i == 4)
>>> + continue;
>>> + reg16 |= ((mask & (1 << i)) ? 3 : 1) << (i * 2);
>>> + }
>>> + pci_write_config16(dev, PCI_DMA_CFG, reg16);
>>> +}
>>>
>>> - //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);
>>> -
>>> - //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 */
>>> +static void i82801xx_lpc_decode_en(device_t dev, uint16_t ich_model)
>>> +{
>>> + /* 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.
>>> + * We also need to set the value for LPC I/F Enables Register.
>>> + * Note: ICH-ICH5 registers differ from ICH6-ICH9.
>>> + */
>>> + if (ich_model <= 0x24D0) {
>>> 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);
>>> + pci_write_config16(dev, LPC_EN, 0x300F);
>>> + } else if (ich_model >= 0x2640) {
>>> + pci_write_config8(dev, LPC_IO_DEC, 0x10);
>>> + pci_write_config16(dev, LPC_EN2, 0x300F);
>>> + }
>>> }
>>>
>>> static void enable_hpet(struct device *dev)
>>> @@ -163,59 +253,38 @@
>>>
>>> static void lpc_init(struct device *dev)
>>> {
>>> - uint8_t byte;
>>> - int pwr_on = -1;
>>> - int nmi_option;
>>> + unsigned ich_model;
>> uint_16 or u16 please
>>
> I tried that originally but got a build error. That's why I used
> unsigned. uint_16 is defined in the actual function anyways.
>>
>>> + ich_model = pci_read_config16(dev, 0x2);
>>> + /* Set the value for PCI Command Register */
>>> + pci_write_config16(dev, PCI_COMMAND, 0x000f);
>>> +
>>> /* IO APIC initialization */
>>> - i82801xx_enable_ioapic(dev);
>>> + i82801xx_enable_apic(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.
>>> - */
>>> + /* Setup the PIRQ */
>>> + i82801xx_pirq_init(dev, ich_model);
>>>
>>> - /* Posted memory write enable */
>>> - byte = pci_read_config8(dev, 0x46);
>>> - pci_write_config8(dev, 0x46, byte | (1 << 0));
>>> -#endif
>>> + /* Setup Power Options */
>>> + i82801xx_power_options(dev);
>>>
>>> - /* power after power fail */
>>> - /* FIXME this doesn't work! */
>>> - /* Which state do we want to goto after g3 (power restored)?
>>> - * 0 == S0 Full On
>>> - * 1 == S5 Soft Off
>>> - */
>>> - pci_write_config8(dev, GEN_PMCON_3, pwr_on ? 0 : 1);
>>> - printk_info("Set power %s if power fails\n", pwr_on ? "on" :
>>> "off");
>>> + /* Set the state of the gpio lines */
>>> + gpio_init(dev, ich_model);
>>>
>>> - /* Set up NMI on errors */
>>> - byte = inb(0x61);
>>> - byte &= ~(1 << 3); /* IOCHK# NMI Enable */
>>> - byte &= ~(1 << 2); /* PCI SERR# Enable */
>>> - outb(byte, 0x61);
>>> - byte = inb(0x70);
>>> -
>>> - nmi_option = NMI_OFF;
>>> - get_option(&nmi_option, "nmi");
>>> - if (nmi_option) {
>>> - byte &= ~(1 << 7); /* Set NMI */
>>> - outb(byte, 0x70);
>>> - }
>>> -
>>> /* Initialize the real time clock */
>>> i82801xx_rtc_init(dev);
>>>
>>> + /* Route DMA */
>>> i82801xx_lpc_route_dma(dev, 0xff);
>>>
>>> - /* Initialize isa dma */
>>> + /* Initialize ISA DMA */
>>> isa_dma_init();
>>>
>>> - i82801xx_1f0_misc(dev);
>>> + /* Setup Decode Ports and LPC I/F Enables */
>>> + i82801xx_lpc_decode_en(dev, ich_model);
>>> +
>>> /* 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 2901)
>>> +++ src/southbridge/intel/i82801xx/i82801xx.h (working copy)
>>> @@ -35,16 +35,24 @@
>>>
>>> #define PCICMD 0x04
>>
>> This is already defined in pci.h (I think). It's PCI_COMMAND, IIRC
>>
> OK, but is it being used by the other c files?
Yes, but I can't seem to find one at the moment. I know that it's
included with <device/pci.h> (which in turn includes pci_def.h, where
PCI_COMMAND is defined). IIRC the fresh vt8237r uses it.
>>
>>> #define PMBASE 0x40
>>> -#define PM_BASE_ADDR 0x1100
>>> #define ACPI_CNTL 0x44
>>> #define BIOS_CNTL 0x4E
>>> #define GPIO_BASE 0x58
>>> -#define GPIO_BASE_ADDR 0x1180
>>> +#define GPIOBASE 0x48
>>
>> What's GPIO_BASE vs GPIOBASE? Better names possible?
>>
> GPIOBASE is what intel calls it in the ICH6-9 datasheets.
> What should I name this??
GPIOBASE makes perfect sense, the confusion is, what's the difference
between that and GPIO_BASE
>>
>>> #define GPIO_CNTL 0x5C
>>> +#define GC 0x4C
>>
>> Again, what's GC? GameCube? :p
>>
> GC is what intel calls it in the ICH6-9 datasheets.
> What should I name this??
What's it actually control/do? I'm sure you can come up with something.
Perhaps even just leave the define and include a comment stating what it
is. Up to you.
>>
>>> #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_IO_DEC 0x80
>>> #define LPC_EN 0xE6
>>> +#define LPC_EN2 0x82
>>> #define FUNC_DIS 0xF2
>>>
>>> #define CMD 0x04
>>
>> Is this used the same as PCICMD above? If so, both should be nuked in
>> favor of PCI_COMMAND
>>
> Agree, no need for duplicated code. But again, is it being used by the
> other c files?
>>
>> Damn, I made one hell of a mess when I hacked this together, didn't I?
>> Well, thanks for cleaning it up :)
>>
>> Cheers,
>> Corey
>>
>
>
>
> Thanks - Joe
>
More information about the coreboot
mailing list