Hi Maggie,
thank you for the fast review! I have changed the things you suggested in the hope this will be better. Please see below.
Best regards, Carl-Daniel Hailfinger
On 10.12.2008 07:56, Li, Maggie wrote:
Hi, Carl-Daniel
I have some suggestion about your modification. Please see below.
Best regards Maggie li
-----Original Message----- From: Carl-Daniel Hailfinger [mailto:c-d.hailfinger.devel.2006@gmx.net] Sent: Wednesday, December 10, 2008 10:19 AM To: ron minnich Cc: Li, Maggie; Bao, Zheng; coreboot@coreboot.org; Xie, Michael; Huang, FrankR; Feng, Libo; Wang, Qingpei Subject: Re: [coreboot] patch for support mainboard pistachio:
Dear Maggie,
can you please look at the attached patch and review it? I have tried to improve comments and warn about possible problems in early SB600 setup.
Thank you!
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
The patch is also inlined into this mail for better viewing.
Regards, Carl-Daniel Hailfinger
Index: LinuxBIOSv2-asus_m2a-vm/src/southbridge/amd/sb600/sb600_early_setup.c
--- LinuxBIOSv2-asus_m2a-vm/src/southbridge/amd/sb600/sb600_early_setup.c (Revision 3808) +++ LinuxBIOSv2-asus_m2a-vm/src/southbridge/amd/sb600/sb600_early_setup.c (Arbeitskopie) @@ -53,11 +53,15 @@
/***************************************
- Legacy devices are mapped to LPC space.
-* serial port 0 +* Serial port 0
- KBC Port
- ACPI Micro-controller port
-* LPC ROM size, +* LPC ROM size +* This function does not change port 0x80 decoding. +* Console output through any port besides 0x3f8 is unsupported.
- NOTE: Call me ASAP, because I will reset LPC ROM size!
+* Reviewed-by: Carl-Daniel Hailfinger +* Reviewed against AMD SB600 Register Reference Manual 3.03, section 3.1 (LPC ISA Bridge) ***************************************/ static void sb600_lpc_init(void) { @@ -68,31 +72,38 @@ /* Enable lpc controller */ dev = pci_locate_device(PCI_ID(0x1002, 0x4385), 0); /* SMBUS controller */ reg32 = pci_read_config32(dev, 0x64);
- reg32 |= 0x00100000;
reg32 |= 1 << 20; pci_write_config32(dev, 0x64, reg32);
dev = pci_locate_device(PCI_ID(0x1002, 0x438d), 0); /* LPC Controller */
- /* Serial 0 */
- /* Decode port 0x3f8-0x3ff (Serial 0) */
+#warning Serial port decode on LPC is hardcoded to 0x3f8 reg8 = pci_read_config8(dev, 0x44);
- reg8 |= (1 << 6);
- reg8 |= 1 << 6; pci_write_config8(dev, 0x44, reg8);
- /* PS/2 keyboard, ACPI */
- /* Decode port 0x60-0x67 (PS/2 keyboard, ACPI) */
[Li, Maggie] Decode port 60 & 64h (PS/2 keyboard), and port 62 & 66h (ACPI)
Thanks, I changed this.
reg8 = pci_read_config8(dev, 0x47); reg8 |= (1 << 5) | (1 << 6); pci_write_config8(dev, 0x47, reg8);
/* SuperIO, LPC ROM */ reg8 = pci_read_config8(dev, 0x48);
- reg8 |= (1 << 1) | (1 << 0); /* enable Super IO config port 2e-2h, 4e-4f */
- reg8 |= (1 << 3) | (1 << 4); /* enable for LPC ROM address range1&2, Enable 512KB rom access at 0xFFF80000 - 0xFFFFFFFF */
- reg8 |= 1 << 6; /* enable for RTC I/O range */
/* Decode ports 0x2e-0x2f, 0x4e-0x4f (SuperI/O configuration) */
reg8 |= (1 << 1) | (1 << 0);
/* Decode variable LPC ROM address ranges 1&2 (see register 0x68-0x6b, 0x6c-0x6f) */
reg8 |= (1 << 3) | (1 << 4);
/* Decode port 0x70-0x73 (RTC) */
reg8 |= 1 << 6; pci_write_config8(dev, 0x48, reg8);
/* hardware should enable LPC ROM by pin strapes */
- /* rom access at 0xFFF80000/0xFFF00000 - 0xFFFFFFFF */
- /* ROM access at 0xFFF80000/0xFFF00000 - 0xFFFFFFFF */ /* See detail in BDG-215SB600-03.pdf page 15. */
- pci_write_config16(dev, 0x68, 0x000e); /* enable LPC ROM range, 0xfff8: 512KB, 0xfff0: 1MB; */
+#warning If Lpc_Rom strap is enabled, the line below has no effect, and if Lpc_Rom strap is disabled, a write to 0x6a is missing
- pci_write_config16(dev, 0x68, 0x000e); /* enable LPC ROM range mirroring below 1 MB */
+#warning Due to FWH_*_IDSEL defaults, any FWH ROM chip larger than 512 kB will not work although we enable a range of 1 MB below. LPC ROMs are not affected. +#warning If Lpc_Rom strap is disabled, a write to 0x6e is missing pci_write_config16(dev, 0x6c, 0xfff0); /* enable LPC ROM range, 0xfff8: 512KB, 0xfff0: 1MB */ [Li, Maggie] I think the warning about 0x68 and 0x6c is somewhat confusing. If Lpc_Rom strap is disabled, default values for register 0x6a and 0x6e are correct. Do we have to write them? Should the register 0x68, 0x6c be for setting LPC ROM, and register 0x70 for FWH ROM? Should we write some code for setting FWH ROM here?
Yes, the warning was confusing. My apologies.
Will the settings in register 0x68 and 0x6c also affect FWH and SPI? If yes, we can simply set them unconditionally.
I have prepared a new patch which assumes 0x68 and 0x6c affect FWH and SPI. Setting these registers again (if LPC strap was active) is not a problem and it makes the code simpler.
}
@@ -201,26 +212,33 @@ /* P2P Bridge */ dev = pci_locate_device(PCI_ID(0x1002, 0x4384), 0);
/* Chip Control: Enable subtractive decoding */ byte = pci_read_config8(dev, 0x40); byte |= 1 << 5; pci_write_config8(dev, 0x40, byte);
/* Misc Control: Enable subtractive decoding if 0x40 bit 5 is set */ byte = pci_read_config8(dev, 0x4B); byte |= 1 << 7; pci_write_config8(dev, 0x4B, byte);
/* FIXME: This does not make sense if we want to decode port 0x80 */
I am not sure about this comment. If IO Base and IO Limit are 0xf000, nothing will be passed through. Am I wrong?
/* IO Base: 0xf000 */ byte = pci_read_config8(dev, 0x1C); byte |= 0xF << 4; pci_write_config8(dev, 0x1C, byte);
/* IO Limit: 0xf000 */ byte = pci_read_config8(dev, 0x1D); byte |= 0xF << 4; pci_write_config8(dev, 0x1D, byte);
/* PCI Command: Enable IO response */ byte = pci_read_config8(dev, 0x04); byte |= 1 << 0; pci_write_config8(dev, 0x04, byte);
/* LPC controller */ dev = pci_locate_device(PCI_ID(0x1002, 0x438D), 0);
byte = pci_read_config8(dev, 0x4A);
@@ -234,13 +252,13 @@ device_t dev; u32 reg32;
- /* enable lpc controller */
- /* Enable LPC controller */ dev = pci_locate_device(PCI_ID(0x1002, 0x4385), 0); reg32 = pci_read_config32(dev, 0x64); reg32 |= 0x00100000; /* lpcEnable */ pci_write_config32(dev, 0x64, reg32);
- /* enable prot80 LPC decode in pci function 3 configuration space. */
- /* Enable port 80 LPC decode in pci function 3 configuration space. */ dev = pci_locate_device(PCI_ID(0x1002, 0x438d), 0); byte = pci_read_config8(dev, 0x4a); byte |= 1 << 5; /* enable port 80 */
New patch is attached and inline.
Index: LinuxBIOSv2-asus_m2a-vm/src/southbridge/amd/sb600/sb600_early_setup.c =================================================================== --- LinuxBIOSv2-asus_m2a-vm/src/southbridge/amd/sb600/sb600_early_setup.c (Revision 3808) +++ LinuxBIOSv2-asus_m2a-vm/src/southbridge/amd/sb600/sb600_early_setup.c (Arbeitskopie) @@ -53,11 +53,16 @@
/*************************************** * Legacy devices are mapped to LPC space. -* serial port 0 +* Serial port 0 * KBC Port * ACPI Micro-controller port -* LPC ROM size, +* LPC ROM size +* This function does not change port 0x80 decoding. +* Console output through any port besides 0x3f8 is unsupported. +* If you use FWH ROMs, you have to setup IDSEL. * NOTE: Call me ASAP, because I will reset LPC ROM size! +* Reviewed-by: Carl-Daniel Hailfinger +* Reviewed against AMD SB600 Register Reference Manual 3.03, section 3.1 (LPC ISA Bridge) ***************************************/ static void sb600_lpc_init(void) { @@ -68,32 +73,42 @@ /* Enable lpc controller */ dev = pci_locate_device(PCI_ID(0x1002, 0x4385), 0); /* SMBUS controller */ reg32 = pci_read_config32(dev, 0x64); - reg32 |= 0x00100000; + reg32 |= 1 << 20; pci_write_config32(dev, 0x64, reg32);
dev = pci_locate_device(PCI_ID(0x1002, 0x438d), 0); /* LPC Controller */ - /* Serial 0 */ + /* Decode port 0x3f8-0x3ff (Serial 0) */ +#warning Serial port decode on LPC is hardcoded to 0x3f8 reg8 = pci_read_config8(dev, 0x44); - reg8 |= (1 << 6); + reg8 |= 1 << 6; pci_write_config8(dev, 0x44, reg8);
- /* PS/2 keyboard, ACPI */ + /* Decode port 0x60 & 0x64 (PS/2 keyboard) and port 0x62 & 0x66 (ACPI)*/ reg8 = pci_read_config8(dev, 0x47); reg8 |= (1 << 5) | (1 << 6); pci_write_config8(dev, 0x47, reg8);
/* SuperIO, LPC ROM */ reg8 = pci_read_config8(dev, 0x48); - reg8 |= (1 << 1) | (1 << 0); /* enable Super IO config port 2e-2h, 4e-4f */ - reg8 |= (1 << 3) | (1 << 4); /* enable for LPC ROM address range1&2, Enable 512KB rom access at 0xFFF80000 - 0xFFFFFFFF */ - reg8 |= 1 << 6; /* enable for RTC I/O range */ + /* Decode ports 0x2e-0x2f, 0x4e-0x4f (SuperI/O configuration) */ + reg8 |= (1 << 1) | (1 << 0); + /* Decode variable LPC ROM address ranges 1&2 (see register 0x68-0x6b, 0x6c-0x6f) */ + reg8 |= (1 << 3) | (1 << 4); + /* Decode port 0x70-0x73 (RTC) */ + reg8 |= 1 << 6; pci_write_config8(dev, 0x48, reg8);
- /* hardware should enable LPC ROM by pin strapes */ - /* rom access at 0xFFF80000/0xFFF00000 - 0xFFFFFFFF */ + /* hardware should enable LPC ROM by pin straps */ + /* ROM access at 0xFFF80000/0xFFF00000 - 0xFFFFFFFF */ /* See detail in BDG-215SB600-03.pdf page 15. */ - pci_write_config16(dev, 0x68, 0x000e); /* enable LPC ROM range, 0xfff8: 512KB, 0xfff0: 1MB; */ - pci_write_config16(dev, 0x6c, 0xfff0); /* enable LPC ROM range, 0xfff8: 512KB, 0xfff0: 1MB */ + /* enable LPC ROM range mirroring start 0x000e(0000) */ + pci_write_config16(dev, 0x68, 0x000e); + /* enable LPC ROM range mirroring end 0x000f(ffff) */ + pci_write_config16(dev, 0x6a, 0x000f); + /* enable LPC ROM range start, 0xfff8: 512KB, 0xfff0: 1MB */ + pci_write_config16(dev, 0x6c, 0xfff0); + /* enable LPC ROM range end at 0xffff(ffff) */ + pci_write_config16(dev, 0x6e, 0xffff); }
/* what is its usage? */ @@ -201,26 +216,33 @@ /* P2P Bridge */ dev = pci_locate_device(PCI_ID(0x1002, 0x4384), 0);
+ /* Chip Control: Enable subtractive decoding */ byte = pci_read_config8(dev, 0x40); byte |= 1 << 5; pci_write_config8(dev, 0x40, byte);
+ /* Misc Control: Enable subtractive decoding if 0x40 bit 5 is set */ byte = pci_read_config8(dev, 0x4B); byte |= 1 << 7; pci_write_config8(dev, 0x4B, byte);
+ /* FIXME: This does not make sense if we want to decode port 0x80 */ + /* IO Base: 0xf000 */ byte = pci_read_config8(dev, 0x1C); byte |= 0xF << 4; pci_write_config8(dev, 0x1C, byte);
+ /* IO Limit: 0xf000 */ byte = pci_read_config8(dev, 0x1D); byte |= 0xF << 4; pci_write_config8(dev, 0x1D, byte);
+ /* PCI Command: Enable IO response */ byte = pci_read_config8(dev, 0x04); byte |= 1 << 0; pci_write_config8(dev, 0x04, byte);
+ /* LPC controller */ dev = pci_locate_device(PCI_ID(0x1002, 0x438D), 0);
byte = pci_read_config8(dev, 0x4A); @@ -234,13 +256,13 @@ device_t dev; u32 reg32;
- /* enable lpc controller */ + /* Enable LPC controller */ dev = pci_locate_device(PCI_ID(0x1002, 0x4385), 0); reg32 = pci_read_config32(dev, 0x64); reg32 |= 0x00100000; /* lpcEnable */ pci_write_config32(dev, 0x64, reg32);
- /* enable prot80 LPC decode in pci function 3 configuration space. */ + /* Enable port 80 LPC decode in pci function 3 configuration space. */ dev = pci_locate_device(PCI_ID(0x1002, 0x438d), 0); byte = pci_read_config8(dev, 0x4a); byte |= 1 << 5; /* enable port 80 */