-----Original Message----- From: coreboot-bounces@coreboot.org [mailto:coreboot-bounces@coreboot.org] On Behalf Of Peter Stuge Sent: Friday, January 30, 2009 9:17 AM To: coreboot@coreboot.org Subject: Re: [coreboot] msrtool CS5536 interrupt and DIVIL LBAR MSRs
Peter Stuge wrote:
Can someone please review these register definitions? Thank you!
+ { 0x51400008, MSRTYPE_RDWR, MSR2(0, 0), "DIVIL_LBAR_IRQ", "Local BAR - IRQ Mapper", { + { 15, 8, "BASE_ADDR", "Base Address in I/O Space", PRESENT_HEX, { + { BITVAL_EOT } + }}, + { 7, 8, RESERVED }, + { BITS_EOT }
This would match the docs better if it were 15,11 BASE_ADDR & 4,5 Reserved. You could also add a comment saying that because it's a base address (See page 104) more bits are reserved. It just took me extra time to check.
+ { 0x5140000f, MSRTYPE_RDWR, MSR2(0, 0), "DIVIL_LBAR_PMS", "Local BAR - Power Management Support", { ... + { 15, 8, "BASE_ADDR", "Base Address in I/O Space", PRESENT_HEX, { + { BITVAL_EOT } + }}, + { 7, 8, RESERVED }, + { BITS_EOT }
Same thing, only this time it would be 15,9 BASE_ADDR & 6,7 Reserved, even though both places refer to pg 104. I didn't take enough time to understand the difference between the two.
+ { 0x51400026, MSRTYPE_RDONLY, MSR2(0, 0), "PIC_XIRR_STS_LOW", "IRQ Mapper Extended Interrupt Request Status Low", { + { 63, 32, RESERVED }, + { 31, 1, "IG7_STS_Z", "Unrestricted Source Z Input 7", PRESENT_BIN, { + { MSR1(0), "No interrupt." }, + { MSR1(1), "Interrupt status." }, + { BITVAL_EOT }
I think "Interrupt status" should be replaced with "Interrupt set" or something that suggests that there is an interrupt.
Besides that I didn't see anything amiss.
Acked-by: Myles Watson mylesgw@gmail.com
Thanks, Myles