Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35439 )
Change subject: sb/intel/ibexpeak: Add CIR initialization ......................................................................
Patch Set 1: -Code-Review
(19 comments)
Here's some suggestions pulled out of intel magic, hope they help
https://review.coreboot.org/c/coreboot/+/35439/1/src/mainboard/lenovo/x201/r... File src/mainboard/lenovo/x201/romstage.c:
https://review.coreboot.org/c/coreboot/+/35439/1/src/mainboard/lenovo/x201/r... PS1, Line 69: 0x02060100 PRSTS, written as 0x3310 in lpc.c; my system has the same value. Not sure what the lpc code does with it, but it doesn't look like it's going to end like this
https://review.coreboot.org/c/coreboot/+/35439/1/src/southbridge/intel/ibexp... File src/southbridge/intel/ibexpeak/early_cir.c:
https://review.coreboot.org/c/coreboot/+/35439/1/src/southbridge/intel/ibexp... PS1, Line 30: CIR7 According to intel magic: Bitfield [3:0] should be 0xf
Unchecked fields: Value for [31:9] is 0x0 Value for [8:7] is 0x0 Value for [6:4] is 0x0
Should be fine.
https://review.coreboot.org/c/coreboot/+/35439/1/src/southbridge/intel/ibexp... PS1, Line 37: 0x3b09 My LPC device ID
https://review.coreboot.org/c/coreboot/+/35439/1/src/southbridge/intel/ibexp... PS1, Line 41: CIR6 According to intel magic: Bitfield [23:21] should be 0x3 Bitfield [7] should be 0x0
Unchecked fields: Value for [31:24] is 0x13 Value for [20:8] is 0x830 Value for [6:0] is 0x40
https://review.coreboot.org/c/coreboot/+/35439/1/src/southbridge/intel/ibexp... PS1, Line 48: pci_write_config8(PCH_LPC_DEV, CIR4, 0x45); OK
https://review.coreboot.org/c/coreboot/+/35439/1/src/southbridge/intel/ibexp... PS1, Line 50: RCBA32(CIR8) = 0x4000000; OK
https://review.coreboot.org/c/coreboot/+/35439/1/src/southbridge/intel/ibexp... PS1, Line 51: PMIR According to intel magic: Bitfield [31:30] should be 0x3 (for production machines) Bitfield [29:21] should be 0x0 Bitfield [20] should be 0x0 Bitfield [19:0] should be 0x00300
Side note: Bitfield [31:30] is related to the Management Engine. Its default value of 0x0 is used in debug environments.
https://review.coreboot.org/c/coreboot/+/35439/1/src/southbridge/intel/ibexp... PS1, Line 52: RCBA32(0x3318) = 0x1020000; /* undocumented */ According to intel magic, the value is correct
https://review.coreboot.org/c/coreboot/+/35439/1/src/southbridge/intel/ibexp... PS1, Line 58: 0x7f8fdf80 Intel magic suggests 0x7F8F9F80 instead
https://review.coreboot.org/c/coreboot/+/35439/1/src/southbridge/intel/ibexp... PS1, Line 59: /* Does not match EDS */ I don't think anybody tested Lynnfield/Clarksfield hardware. I would use the EDS value.
Is it me, or does this look similar to CIR17 but for another kind of CPU?
https://review.coreboot.org/c/coreboot/+/35439/1/src/southbridge/intel/ibexp... PS1, Line 62: 0x6002000 Intel magic suggests 0x2000 instead
https://review.coreboot.org/c/coreboot/+/35439/1/src/southbridge/intel/ibexp... PS1, Line 68: c.x86_model == 0x25 My CPU model (i3-370M)
https://review.coreboot.org/c/coreboot/+/35439/1/src/southbridge/intel/ibexp... PS1, Line 70: RCBA32(CIR10) = 0xfffff; According to intel magic: CIR10 Field 1 — R/W. BIOS must program this field to 00000000h for Lynnfield/Clarksfield-based systems and 000FFF80h for Havendale/Auburndale-based systems.
My system doesn't seem to program any value here (default 0x0)
https://review.coreboot.org/c/coreboot/+/35439/1/src/southbridge/intel/ibexp... PS1, Line 71: RCBA32(CIR15) = 0x7f8fdfff; Intel magic suggests 0x7F8F9FFF instead
https://review.coreboot.org/c/coreboot/+/35439/1/src/southbridge/intel/ibexp... PS1, Line 72: RCBA32(CIR17) = 0x3900; Intel magic suggests 0x2900 instead, but my system has 0x3900
https://review.coreboot.org/c/coreboot/+/35439/1/src/southbridge/intel/ibexp... PS1, Line 73: RCBA32(CIR19) = 0x10000; Intel magic suggests 0x10001 instead, but my system has 0x10000
https://review.coreboot.org/c/coreboot/+/35439/1/src/southbridge/intel/ibexp... PS1, Line 74: RCBA32(CIR20) = 0x1004b; OK
https://review.coreboot.org/c/coreboot/+/35439/1/src/southbridge/intel/ibexp... PS1, Line 75: RCBA32(CIR21) = 0x6000008; Intel magic says this value on my system is correct, but other intel magic suggests 0x0000008
https://review.coreboot.org/c/coreboot/+/35439/1/src/southbridge/intel/ibexp... PS1, Line 79: RCBA32(CIR22) = 0x10000; OK