> -----Original Message-----
> From: [email protected] [mailto:[email protected]]
> On Behalf Of Peter Stuge
> Sent: Friday, January 30, 2009 9:17 AM
> To: [email protected]
> 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 <[email protected]>
Thanks,
Myles
--
coreboot mailing list: [email protected]
http://www.coreboot.org/mailman/listinfo/coreboot
