[LinuxBIOS] [v2][PATCH] Trivial fixups for i82801xx_lpc

joe at smittys.pointclark.net joe at smittys.pointclark.net
Tue Oct 30 03:56:50 CET 2007


Quoting Corey Osgood <corey.osgood at gmail.com>:

> 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
>
The register address 0x58 (ICH0-ICH5) vs 0x48 (ICH6-ICH9).
>
>>>
>>>>  #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.
>
Same as GPIO_CNTL but for (ICH6-ICH9). The register address 0x5C  
(ICH0-ICH5) vs 0x4C (ICH6-ICH9). There is a comment in the gpio_init()  
function.
>
>>>
>>>>  #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