[coreboot] msrtool CS5536 interrupt and DIVIL LBAR MSRs

Mart Raudsepp mart.raudsepp at artecdesign.ee
Tue Mar 17 18:13:27 CET 2009


Ü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





More information about the coreboot mailing list