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

joe at smittys.pointclark.net joe at smittys.pointclark.net
Sun Oct 28 02:21:22 CEST 2007


Quoting Uwe Hermann <uwe at hermann-uwe.de>:

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




More information about the coreboot mailing list