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

Hello,
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.
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.

3. The rest of the fixups were trivial.

Signed-off-by: Joseph Smith

Thanks - Joe

[Uwe Hermann]

On Sat, Oct 27, 2007 at 10:41:34AM -0400, [e-mail] wrote:
> Here is a patch with mostly trivial fixups for i82801x_lpc.c. The main 
> fixup is the PIRQ Routing Control.
> These are 8 bit registers not 32.

Yep.
> 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?

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?
> 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()?
> 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*.
> /* 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*.
> 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?
> }
>
> +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.
> +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?
> //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?
> /* 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.
> /* Set the value for LPC I/F Enables Register  */
> pci_write_config16(dev, LPC_EN, 0x300F);

Same for LPC_EN, different on ICH8.
> /* 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.

Quoting Uwe Hermann :
> On Sat, Oct 27, 2007 at 10:41:34AM -0400, [e-mail] wrote:
> > Here is a patch with mostly trivial fixups for i82801x_lpc.c. The main
> > fixup is the PIRQ Routing Control.
> > These are 8 bit registers not 32.
>
> Yep.
> > 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

[Corey Osgood]

[e-mail] wrote:
> Quoting Uwe Hermann :
> > On Sat, Oct 27, 2007 at 10:41:34AM -0400, [e-mail] wrote:
> > > Here is a patch with mostly trivial fixups for i82801x_lpc.c. The main
> > > fixup is the PIRQ Routing Control.
> > > These are 8 bit registers not 32.
> >
> > Yep.
> > > 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

Quoting [e-mail]:
> Hello,
> Here is a patch with mostly trivial fixups for i82801xx_lpc.c. The main
> fixup is the PIRQ Routing Control.
> These are 8 bit registers not 32.
> 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.
> The rest of the fixups were trivial.
>
> Signed-off-by: Joseph Smith

Ok, here is the revised patch per Uwe's and Corey's suggestions.

Thanks - Joe

[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

Quoting 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.

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?
> > #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??
> > #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??
> > #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

[Corey Osgood]

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

Quoting Corey Osgood :
> [e-mail] wrote:
> > Quoting 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.
> >
> > 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

[Corey Osgood]

[e-mail] wrote:
> Quoting Corey Osgood :
> > [e-mail] wrote:
> > > Quoting Corey Osgood :
> > > > [e-mail] wrote:
> > > > > 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.

Meant to comment before, oops. I meant uint16_t, of course. Not sure why
there would be build errors from that...
> > > > > #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).

Okay, can we do GPIO_BASE_ICH0_5 and then GPIO_BASE_ICH6_9? And similar
for GPIO_CNTL?

After that, swear to god, done nitpicking.

Thanks,
Corey

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

Quoting [e-mail]:
> Quoting [e-mail]:
> > Hello,
> > Here is a patch with mostly trivial fixups for i82801xx_lpc.c. The main
> > fixup is the PIRQ Routing Control.
> > These are 8 bit registers not 32.
> > 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.
> > The rest of the fixups were trivial.
> >
> > Signed-off-by: Joseph Smith
>
> Ok, here is the revised patch per Uwe's and Corey's suggestions.

OK, Now here is the final revised patch #3.

Thanks - Joe

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

[Corey Osgood]

[e-mail] wrote:
> Quoting [e-mail]:
> > Quoting [e-mail]:
> > > Hello,
> > > Here is a patch with mostly trivial fixups for i82801xx_lpc.c. The main
> > > fixup is the PIRQ Routing Control.
> > > These are 8 bit registers not 32.
> > > 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.
> > > The rest of the fixups were trivial.
> > >
> > > Signed-off-by: Joseph Smith
> >
> > Ok, here is the revised patch per Uwe's and Corey's suggestions.
>
> OK, Now here is the final revised patch #3.
>
> Thanks - Joe

Thanks for not shooting me ;)

Acked-by: Corey Osgood

[Uwe Hermann]

On Mon, Oct 29, 2007 at 11:32:00PM -0400, Corey Osgood wrote:
> > The register address 0x58 (ICH0-ICH5) vs 0x48 (ICH6-ICH9).
>
> Okay, can we do GPIO_BASE_ICH0_5 and then GPIO_BASE_ICH6_9? And similar
> for GPIO_CNTL?

Yep, sounds good.

Please don't forget the Signed-off-by for the next patch repost, I think
I'll commit that one :)

Uwe.

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

Quoting [e-mail]:
> Quoting [e-mail]:
> > Quoting [e-mail]:
> > > Hello,
> > > Here is a patch with mostly trivial fixups for i82801xx_lpc.c. The main
> > > fixup is the PIRQ Routing Control.
> > > These are 8 bit registers not 32.
> > > 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.
> > > The rest of the fixups were trivial.
> > >
> > > Signed-off-by: Joseph Smith
> >
> > Ok, here is the revised patch per Uwe's and Corey's suggestions.
>
> OK, Now here is the final revised patch #3.
>
> Thanks - Joe

Thanks for not shooting me ;)

Acked-by: Corey Osgood

Signed-off-by: Joseph Smith

Thanks - Joe

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

Quoting [e-mail]:
> Quoting [e-mail]:
> > Quoting [e-mail]:
> > > Quoting [e-mail]:
> > > > Hello,
> > > > Here is a patch with mostly trivial fixups for i82801xx_lpc.c. The main
> > > > fixup is the PIRQ Routing Control.
> > > > These are 8 bit registers not 32.
> > > > 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.
> > > > The rest of the fixups were trivial.
> > > >
> > > > Signed-off-by: Joseph Smith
> > >
> > > Ok, here is the revised patch per Uwe's and Corey's suggestions.
> >
> > OK, Now here is the final revised patch #3.
> >
> > Thanks - Joe
>
> Thanks for not shooting me ;)
>
> Acked-by: Corey Osgood
>
> Signed-off-by: Joseph Smith
>
> Thanks - Joe

Bueler.....Bueler....Bueler....

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

[Uwe Hermann]

On Tue, Oct 30, 2007 at 10:48:59AM -0400, [e-mail] wrote:
> Signed-off-by: Joseph Smith 
> Acked-by: Corey Osgood

Committed in r2912 with some fixups (patch didn't apply for some reason).

Uwe.

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

Quoting Uwe Hermann :
> On Tue, Oct 30, 2007 at 10:48:59AM -0400, [e-mail] wrote:
> > Signed-off-by: Joseph Smith 
> > Acked-by: Corey Osgood
>
> Committed in r2912 with some fixups (patch didn't apply for some reason).
> Uwe.

Thanks Uwe.
What did you have to change to apply the patch??

Thanks - Joe

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

[Uwe Hermann]

On Tue, Oct 30, 2007 at 06:34:21PM -0400, [e-mail] wrote:
> What did you have to change to apply the patch??

The *.h had some rejects, but I didn't figure out what the problem was
exactly. I manually performed the changes from the patch to the svn file,
hope I didn't accidentally break something. I also fixed some more
cosmetics in the patch (but no code changes).

Uwe.

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

Quoting Uwe Hermann :
> On Tue, Oct 30, 2007 at 06:34:21PM -0400, [e-mail] wrote:
> > What did you have to change to apply the patch??
>
> The *.h had some rejects, but I didn't figure out what the problem was
> exactly. I manually performed the changes from the patch to the svn file,
> hope I didn't accidentally break something. I also fixed some more
> cosmetics in the patch (but no code changes).
>
> Uwe.

Hmm that's weird. It built ok against my local repository ok. I will  
try a new build against the new revision.

Thanks - Joe

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

Quoting [e-mail]:
> Quoting Uwe Hermann :
> > On Tue, Oct 30, 2007 at 06:34:21PM -0400, [e-mail] wrote:
> > > What did you have to change to apply the patch??
> >
> > The *.h had some rejects, but I didn't figure out what the problem was
> > exactly. I manually performed the changes from the patch to the svn file,
> > hope I didn't accidentally break something. I also fixed some more
> > cosmetics in the patch (but no code changes).
> >
> > Uwe.
>
> Hmm that's weird. It built ok against my local repository ok. I will
> try a new build against the new revision.

Builds just fine.
Thanks again - Joe