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:[EMAIL PROTECTED]
> Sent: Wednesday, December 10, 2008 10:19 AM
> To: ron minnich
> Cc: Li, Maggie; Bao, Zheng; [email protected]; 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 <[EMAIL PROTECTED]>
>
> 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 */
--
http://www.hailfinger.org/
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 */
--
coreboot mailing list: [email protected]
http://www.coreboot.org/mailman/listinfo/coreboot
