Can someone please review these register definitions? Thank you!
//Peter
Peter Stuge wrote:
Can someone please review these register definitions? Thank you!
I know it is mind numbing, but I would really like someone else to have a look before committing this. Thanks again!
//Peter
Peter Stuge wrote:
Can someone please review these register definitions? Thank you!
Attaching latest version also available at http://stuge.se/mt.cs5536_pic_divil3.patch
//Peter
-----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
2009/1/30 Peter Stuge peter@stuge.se:
Peter Stuge wrote:
Can someone please review these register definitions? Thank you!
Attaching latest version also available at http://stuge.se/mt.cs5536_pic_divil3.patch
const struct msrdef cs5536_msrs[] = { + /* 0x51400008-0x5140000f per 33238G pages 356-361 */ + /* 0x51400015 per 33238G pages 365-366 */ + /* 0x51400020-0x51400027 per 33238G pages 379-385 */ + { 0x51400008, MSRTYPE_RDWR, MSR2(0, 0), "DIVIL_LBAR_IRQ", "Local BAR - IRQ Mapper", { + { 63, 15, RESERVED }, + { 48, 1, RESERVED },
I'm sure there is some reason, but why isn't this just "{ 63, 16, RESERVED }," ?
+ { 47, 4, "IO_MASK", "I/O Address Mask Value", PRESENT_BIN, {
The masks are probably most readable as hex, especially to match the display type of the BAR.
+ { 0x51400009, MSRTYPE_RDWR, MSR2(0, 0), "DIVIL_LBAR_KEL", "Local BAR - KEL from USB OHC Host Controller", {
Copied directly from the spec, but just "Local BAR - KEL from USB OHC" wouldn't propagate RAS Syndrome.
+ { 0x51400020, MSRTYPE_RDWR, MSR2(0, 0), "PIC_YSEL_LOW", "IRQ Mapper Unrestricted Y Select Low", { + { 63, 32, RESERVED }, + { 31, 4, "MAP_Y7", "Map Unrestricted Y Input 7", PRESENT_BIN, {
HEX is maybe more readable for all of these selects?
+ { 0, 1, "IG8_STS_PRIM", "Primary Input 8", PRESENT_BIN, { + { MSR1(0), "No interrupt." }, + { MSR1(1), "Interrupt status." },
Like Myles said, "Interrupt set" or maybe "Interrupt requested" for value '1'
Tom Sylla wrote:
- { 0x51400008, MSRTYPE_RDWR, MSR2(0, 0), "DIVIL_LBAR_IRQ", "Local BAR
- IRQ Mapper", {
{ 63, 15, RESERVED },
{ 48, 1, RESERVED },
I'm sure there is some reason, but why isn't this just "{ 63, 16, RESERVED }," ?
It does not matter to msrtool at the moment. I made it two separate fields primarily because they are separate in the data book. In the future maybe msrtool will also be writing to MSRs, in which case it becomes more important because the latter is always write 0, but again there's no way to express that in msrtool right now. Maybe fields for that should be added sooner rather than later?
{ 47, 4, "IO_MASK", "I/O Address Mask Value", PRESENT_BIN, {
The masks are probably most readable as hex, especially to match the display type of the BAR.
Could certainly change that. I like them different because they have a different length than the base address, and I find binary easier than hex for masks. A habit I guess.
- { 0x51400009, MSRTYPE_RDWR, MSR2(0, 0), "DIVIL_LBAR_KEL", "Local BAR
- KEL from USB OHC Host Controller", {
Copied directly from the spec, but just "Local BAR - KEL from USB OHC" wouldn't propagate RAS Syndrome.
Thank you!
- { 0x51400020, MSRTYPE_RDWR, MSR2(0, 0), "PIC_YSEL_LOW", "IRQ Mapper
Unrestricted Y Select Low", {
{ 63, 32, RESERVED },
{ 31, 4, "MAP_Y7", "Map Unrestricted Y Input 7", PRESENT_BIN, {
HEX is maybe more readable for all of these selects?
I used binary because the bitdefs are listed that way in the data book. All selects have full bitdef text descriptions and they have the number in decimal, so I think it should be ok.
{ 0, 1, "IG8_STS_PRIM", "Primary Input 8", PRESENT_BIN, {
{ MSR1(0), "No interrupt." },
{ MSR1(1), "Interrupt status." },
Like Myles said, "Interrupt set" or maybe "Interrupt requested" for value '1'
I like set over requested, but status over set. Maybe signal active or simply condition or signalled?
//Peter
Ühel kenal päeval, N, 2009-02-05 kell 04:05, kirjutas Peter Stuge:
Tom Sylla wrote:
- { 0x51400008, MSRTYPE_RDWR, MSR2(0, 0), "DIVIL_LBAR_IRQ", "Local BAR
- IRQ Mapper", {
{ 63, 15, RESERVED },
{ 48, 1, RESERVED },
I'm sure there is some reason, but why isn't this just "{ 63, 16, RESERVED }," ?
It does not matter to msrtool at the moment. I made it two separate fields primarily because they are separate in the data book. In the future maybe msrtool will also be writing to MSRs, in which case it becomes more important because the latter is always write 0, but again there's no way to express that in msrtool right now. Maybe fields for that should be added sooner rather than later?
{ 47, 4, "IO_MASK", "I/O Address Mask Value", PRESENT_BIN, {
The masks are probably most readable as hex, especially to match the display type of the BAR.
Could certainly change that. I like them different because they have a different length than the base address, and I find binary easier than hex for masks. A habit I guess.
- { 0x51400009, MSRTYPE_RDWR, MSR2(0, 0), "DIVIL_LBAR_KEL", "Local BAR
- KEL from USB OHC Host Controller", {
Copied directly from the spec, but just "Local BAR - KEL from USB OHC" wouldn't propagate RAS Syndrome.
Thank you!
- { 0x51400020, MSRTYPE_RDWR, MSR2(0, 0), "PIC_YSEL_LOW", "IRQ Mapper
Unrestricted Y Select Low", {
{ 63, 32, RESERVED },
{ 31, 4, "MAP_Y7", "Map Unrestricted Y Input 7", PRESENT_BIN, {
HEX is maybe more readable for all of these selects?
I used binary because the bitdefs are listed that way in the data book. All selects have full bitdef text descriptions and they have the number in decimal, so I think it should be ok.
{ 0, 1, "IG8_STS_PRIM", "Primary Input 8", PRESENT_BIN, {
{ MSR1(0), "No interrupt." },
{ MSR1(1), "Interrupt status." },
Like Myles said, "Interrupt set" or maybe "Interrupt requested" for value '1'
I like set over requested, but status over set. Maybe signal active or simply condition or signalled?
I was surprised to see this patch not included yet in some form. Do you need more review to an updated patch or just this poke? :)
Regards, Mart Raudsepp
Mart Raudsepp wrote:
I was surprised to see this patch not included yet in some form. Do you need more review to an updated patch or just this poke? :)
Poke is good. Thanks.
//Peter
Mart Raudsepp wrote:
{ 0, 1, "IG8_STS_PRIM", "Primary Input 8", PRESENT_BIN, {
{ MSR1(0), "No interrupt." },
{ MSR1(1), "Interrupt status." },
Like Myles said, "Interrupt set" or maybe "Interrupt requested" for value '1'
I like set over requested, but status over set. Maybe signal active or simply condition or signalled?
I was surprised to see this patch not included yet in some form. Do you need more review to an updated patch or just this poke? :)
Finally committed in r4414. Thanks to all!
I settled on "INTERRUPT" to make it stand out a little in a long decode output.
//Peter