[coreboot] [RFC] Add reviewed-by markers to code sections

[Carl-Daniel Hailfinger]

Hi,

I'm currently trying to get my Asus M2A-VM board (690G/SB600) board
running in v2. It hangs directly after HT reset. That led me to dig into
the code and compare it to the data sheets.

In the past, reviews were mostly centered on coding style (not only
cosmetics, but also code flow) and general sanity. While that is
definitely needed, I propose another layer on top of this:

Verification of the code and comments against data sheet recommendations
and documentation.

Of course such work is neither really sexy nor does it have always an
immediate effect on the usability of a particular board, but it can find
bugs and hidden assumptions which may come to bite us later. I already
found real bugs by comparing code and data sheets in the past, but I
can't remember all the code I went through that way. The situation is
probably similar for other reviewers.

Adding a "Reviewed-by:" tag to the doxygen comments of each function
would be a nice way to say that the function has been reviewed. Pointing
to the section in the docs against which the code was verified is a ncie
added bonus. Plus, comments are really cheap and don't impact compile
time significantly.

Example patch for a reviewed function:

Review and annotate sb600_lpc_init().

Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>

Index: src/southbridge/amd/sb600/sb600_early_setup.c
===================================================================
--- src/southbridge/amd/sb600/sb600_early_setup.c	(Revision 3803)
+++ 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);
 	pci_write_config8(dev, 0x44, reg8);
 
-	/* PS/2 keyboard, ACPI */
+	/* Decode port 0x60-0x67 (PS/2 keyboard, 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 */
+	/* 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 LPC ROM chip larger than 512 kB will not work although we enable a range of 1 MB below
+#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  */
 }
 


-- 
http://www.hailfinger.org/