Attention is currently required from: Paul Menzel, Angel Pons, Arthur Heymans. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47223 )
Change subject: nb/intel/haswell/pcie.c: Add missing pre-ASPM init ......................................................................
Patch Set 6:
(8 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/47223/comment/995e7541_638aa055 PS6, Line 8: Is this currently done by MRC? not done at all?
File src/northbridge/intel/haswell/haswell.h:
https://review.coreboot.org/c/coreboot/+/47223/comment/814aeecb_c99719be PS6, Line 33: 0x11a Datasheet says 0x114 (0x11a is status)? BIOS spec too.
File src/northbridge/intel/haswell/pcie.c:
https://review.coreboot.org/c/coreboot/+/47223/comment/100b484b_fc83e1d3 PS6, Line 62: slotcap |= func << 19; BIOS spec says this should be unique. In case MRC does something similar to the PCH root ports, it would be good to document the numbering scheme. In case it doesn't, I'd prefer to leave it 0 until we want to program it properly (see below).
I guess this should actually be decided by the devicetree of the mainboard. PCIe spec says:
"This field must be initialized to zero for Ports connected to devices that are either integrated on the system board or integrated within the same silicon as the Switch device or Root Port."
IOW, we shouldn't set this >0 if there is no physical slot. And also initializing it with the function number may lead to a spurious 0.
https://review.coreboot.org/c/coreboot/+/47223/comment/6e4331ce_2a36de9f PS6, Line 68: /* 75 watts power limit */ How to know if that's true for a board?
https://review.coreboot.org/c/coreboot/+/47223/comment/779ae947_cb7835c6 PS6, Line 69: 0x3ff `0xff` (the rest is already set above)
https://review.coreboot.org/c/coreboot/+/47223/comment/4da106c9_66f60300 PS6, Line 75: or No need to read, just pci_write_config16() would do.
https://review.coreboot.org/c/coreboot/+/47223/comment/5ad83aba_d0b99d68 PS6, Line 85: pci_or_config32(dev, PEG_DCAP2, 1 << 19); Hmm, BIOS spec says to lock (read, write) the original value.
https://review.coreboot.org/c/coreboot/+/47223/comment/589383ef_9d06b382 PS6, Line 109: 0xac Should be LCAP?