msrtool CS5536 interrupt and DIVIL LBAR MSRs

[Peter Stuge]

Can someone please review these register definitions? Thank you!

//Peter

[Peter Stuge]

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]

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

[Myles Watson]

> -----Original Message-----
> From: [e-mail] [mailto:[e-mail]]
> On Behalf Of Peter Stuge
> Sent: Friday, January 30, 2009 9:17 AM
> To: [e-mail]
> 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

Thanks,
Myles

[Tom Sylla]

2009/1/30 Peter Stuge :
> 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'

[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?

//Peter

[Mart Raudsepp]

Ü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

[Peter Stuge]

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

[Peter Stuge]

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