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

[Corey Osgood]

[e-mail] 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.

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.
> +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
> 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
> #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?
> #define GPIO_CNTL        0x5C
> +#define GC            0x4C

Again, what's GC? GameCube? :p
> #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

Damn, I made one hell of a mess when I hacked this together, didn't I?
Well, thanks for cleaning it up :)

Cheers,
Corey