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

Corey Osgood corey.osgood at gmail.com
Sun Oct 28 02:33:58 CEST 2007


joe at smittys.pointclark.net wrote:
> 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?
>   

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




More information about the coreboot mailing list