Hi Maggie,
thanks for the new review. I have changed the patch according to your
suggestions.
Do you think the new patch is OK?
Regards,
Carl-Daniel
On 11.12.2008 09:44, Li, Maggie wrote:
> Hi, Carl-Daniel
>
> I have some suggestion below for you.
>
> Best regards
> Maggie li
>
> -----Original Message-----
> From: Carl-Daniel Hailfinger [mailto:[email protected]]
> Sent: Thursday, December 11, 2008 10:13 AM
> To: Li, Maggie
> Cc: ron minnich; Bao, Zheng; [email protected]; Xie, Michael; Huang,
> FrankR; Feng, Libo; Wang, Qingpei
> Subject: Re: [coreboot] patch for support mainboard pistachio:
>
> 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.
>
> [Li, Maggie] I did a test on my pistachio board to find that 0x68 and 0x6c do
> affect SPI. I have not a FWH board, so I am not sure whether FWH will be
> affected.
>
Thanks for testing! There are only very few RS690 boards for Intel out
there and I think they are no longer in production, so we can ignore
them for now.
>> }
>>
>> @@ -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?
> [Li, Maggie] The same IO Base and IO Limit here is meaningful when we set the
> bridge to be subtractive. At early setup stage, we have to make sure that
> data can go through port 0x80. After resource is allocated, the bridge has
> its own positive and subtractive window. Any data going to the devices behind
> the bridge can go through positive window. Data to port 0x80 will go through
> subtractive window if no other bridge accepts it after four cycles.
>
> I think it is better to remove the comment "/* FIXME: This does not make
> sense if we want to decode port 0x80 */" now.
>
Yes, thanks! I added your explanation as comment to the code.
If you agree to the new patch, it would be great if you could give an
Acked-by: statement.
Improve comments in early SB600 setup, handle non-LPC strapping and
document verification against the data sheets.
Signed-off-by: Carl-Daniel Hailfinger <[email protected]>
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
(working copy)
@@ -53,11 +53,17 @@
/***************************************
* 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 rev. 3.03, section 3.1
+* (LPC ISA Bridge)
***************************************/
static void sb600_lpc_init(void)
{
@@ -68,32 +74,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(0000): 512KB, 0xfff0(0000): 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 +217,36 @@
/* 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);
+ /* The same IO Base and IO Limit here is meaningful because we set the
+ * bridge to be subtractive. During early setup stage, we have to make
+ * sure that data can go through 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 +260,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 (working copy)
@@ -53,11 +53,17 @@
/***************************************
* 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 rev. 3.03, section 3.1
+* (LPC ISA Bridge)
***************************************/
static void sb600_lpc_init(void)
{
@@ -68,32 +74,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(0000): 512KB, 0xfff0(0000): 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 +217,36 @@
/* 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);
+ /* The same IO Base and IO Limit here is meaningful because we set the
+ * bridge to be subtractive. During early setup stage, we have to make
+ * sure that data can go through 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 +260,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
