Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40952 )
Change subject: nb/intel/sandybridge: Correct IOSAV register notes ......................................................................
Patch Set 1:
(15 comments)
https://review.coreboot.org/c/coreboot/+/40952/1/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/mchbar_regs.h:
https://review.coreboot.org/c/coreboot/+/40952/1/src/northbridge/intel/sandy... PS1, Line 14: every how many
`as well as an address auto-increment value that is added after a given number of command executions […]
Done
https://review.coreboot.org/c/coreboot/+/40952/1/src/northbridge/intel/sandy... PS1, Line 15: Every subseq can be programmed into several MCHBAR registers
"There are 4 sets of subseq registers in MCHBAR" maybe? the into several mchbar registers is rather […]
Done
https://review.coreboot.org/c/coreboot/+/40952/1/src/northbridge/intel/sandy... PS1, Line 16: * When firing up IOSAV, one is required to specify the number of subseqs it should use.
Column width is much too high for running text, IMHO; around 64 chars […]
Done
https://review.coreboot.org/c/coreboot/+/40952/1/src/northbridge/intel/sandy... PS1, Line 19: * channel: [0..1]
Oh, right. […]
I kept the ranges in ascending order, and flipped the bitfields
https://review.coreboot.org/c/coreboot/+/40952/1/src/northbridge/intel/sandy... PS1, Line 23: 'closed'
ah. when reading endpoints i had to think of usb...
Done
https://review.coreboot.org/c/coreboot/+/40952/1/src/northbridge/intel/sandy... PS1, Line 23: endpoints
"limits" usually
I reworded the sentence and used "bounds"
https://review.coreboot.org/c/coreboot/+/40952/1/src/northbridge/intel/sandy... PS1, Line 30: * Defines the address, bank and rank
`bank` and `rank` are also addresses. Maybe: […]
Done
https://review.coreboot.org/c/coreboot/+/40952/1/src/northbridge/intel/sandy... PS1, Line 30: Defines
"configures"?
Done
https://review.coreboot.org/c/coreboot/+/40952/1/src/northbridge/intel/sandy... PS1, Line 32: as per the rules defined in the
"as configured in the" instead?
Done
https://review.coreboot.org/c/coreboot/+/40952/1/src/northbridge/intel/sandy... PS1, Line 33: loaded
stored makes it clearer for me that it changes the contents of that register
I used "written" instead, should be clear enough
https://review.coreboot.org/c/coreboot/+/40952/1/src/northbridge/intel/sandy... PS1, Line 40: encoded
Binary encoded. […]
Done
https://review.coreboot.org/c/coreboot/+/40952/1/src/northbridge/intel/sandy... PS1, Line 47: RAS/CAS address
`Row / Column Address` for consistency?
Done
https://review.coreboot.org/c/coreboot/+/40952/1/src/northbridge/intel/sandy... PS1, Line 69: shall
"will"?
Done
https://review.coreboot.org/c/coreboot/+/40952/1/src/northbridge/intel/sandy... PS1, Line 69: Defines
"configures"
Done
https://review.coreboot.org/c/coreboot/+/40952/1/src/northbridge/intel/sandy... PS1, Line 110: of
`in`?
Done