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

Uwe Hermann uwe at hermann-uwe.de
Sat Oct 27 17:55:03 CEST 2007


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

Yep.


> 2. There were PIRQ channels that were disabled. This should not be 
> determined here but by irq_tables.c or by acpi_tables.c on a mainboard 
> level.

Yes, sounds correct. Is this tested on hardware?


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.
-- 
http://www.hermann-uwe.de  | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20071027/4e2ce2c4/attachment.sig>


More information about the coreboot mailing list