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@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 */ }
Carl-Daniel Hailfinger wrote:
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.
I feel very strongly that we do not need more layers.
//Peter
On Wed, Dec 10, 2008 at 5:12 PM, Peter Stuge peter@stuge.se wrote:
Carl-Daniel Hailfinger wrote:
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.
I feel very strongly that we do not need more layers.
There are problems anyway. What if the doco are known to be wrong, due to an NDA, and you can't even say "the doco is wrong".
Sorry, I vote with peter.
ron
On 11.12.2008 02:19, ron minnich wrote:
On Wed, Dec 10, 2008 at 5:12 PM, Peter Stuge peter@stuge.se wrote:
Carl-Daniel Hailfinger wrote:
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.
I feel very strongly that we do not need more layers.
Let me rephrase that. I do not want to hold back any commits. That would be insane.
However, whenever someone goes through in-tree code and checks the code against the data sheets and thinks that the code is OK, he/she should be free (not obliged) to improve annotation/comments and add a comment that he/she verified the code against the data sheets.
There are problems anyway. What if the doco are known to be wrong, due to an NDA, and you can't even say "the doco is wrong".
If the NDA is so strict that you can't even say that the docs are wrong, how are you preventing erroneous "bugfixes" from being committed? I honestly have no idea how to solve that problem and it exists regardless of whether my RFC is accepted or not.
Sorry, I vote with peter.
I understand that because my original RFC implied things I didn't want to suggest. How about the new text I proposed above?
Regards, Carl-Daniel
On Thu, Dec 11, 2008 at 2:57 PM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote:
On 11.12.2008 02:19, ron minnich wrote:
On Wed, Dec 10, 2008 at 5:12 PM, Peter Stuge peter@stuge.se wrote:
Carl-Daniel Hailfinger wrote:
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.
I feel very strongly that we do not need more layers.
Let me rephrase that. I do not want to hold back any commits. That would be insane.
However, whenever someone goes through in-tree code and checks the code against the data sheets and thinks that the code is OK, he/she should be free (not obliged) to improve annotation/comments and add a comment that he/she verified the code against the data sheets.
IMO, if we do this, we need to also require the datasheet revision/release date and if any update notes or errata are taken into account.
-Corey
There are problems anyway. What if the doco are known to be wrong, due to an NDA, and you can't even say "the doco is wrong".
If the NDA is so strict that you can't even say that the docs are wrong, how are you preventing erroneous "bugfixes" from being committed? I honestly have no idea how to solve that problem and it exists regardless of whether my RFC is accepted or not.
Sorry, I vote with peter.
I understand that because my original RFC implied things I didn't want to suggest. How about the new text I proposed above?
Regards, Carl-Daniel
-- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
On 11.12.2008 21:27, Corey Osgood wrote:
On Thu, Dec 11, 2008 at 2:57 PM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote:
On 11.12.2008 02:19, ron minnich wrote:
On Wed, Dec 10, 2008 at 5:12 PM, Peter Stuge peter@stuge.se wrote:
Carl-Daniel Hailfinger wrote:
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.
I feel very strongly that we do not need more layers.
Let me rephrase that. I do not want to hold back any commits. That would be insane.
However, whenever someone goes through in-tree code and checks the code against the data sheets and thinks that the code is OK, he/she should be free (not obliged) to improve annotation/comments and add a comment that he/she verified the code against the data sheets.
IMO, if we do this, we need to also require the datasheet revision/release date and if any update notes or errata are taken into account.
Good point.
Regards, Carl-Daniel
On Thu, Dec 11, 2008 at 11:57 AM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
However, whenever someone goes through in-tree code and checks the code against the data sheets and thinks that the code is OK, he/she should be free (not obliged) to improve annotation/comments and add a comment that he/she verified the code against the data sheets.
sure. Improved informative comments are always welcome.
thanks
ron
Carl-Daniel Hailfinger wrote:
However, whenever someone goes through in-tree code and checks the code against the data sheets and thinks that the code is OK, he/she should be free (not obliged) to improve annotation/comments and add a comment that he/she verified the code against the data sheets.
It seems your whole point is something to the effect of:
"Document the source of registers and values kthxbye."
As said, we do not need more formalia - even if it would have some practical gain, which I don't believe is the case.
A friendly reminder to document stuff maybe once every other year is nice however!
//Peter
-
Carl-Daniel Hailfinger -
Corey Osgood -
Peter Stuge -
ron minnich