Attention is currently required from: Alexander Couzens, Nicholas Chin, Nicholas Sudsgaard, Paul Menzel.
Patch set 5:Code-Review +1
13 comments:
Patchset:
Beside a small nits and places where I wished for a comment,
this looks very good :)
Feel free to ignore any comment that I already marked resolved
and take the comments mentioning "schematics" with a grain of
salt as we might have different versions of them.
File src/mainboard/lenovo/thinkcentre_m710s/Makefile.mk:
Patch Set #5, Line 6: ramstage-y += early_init.c
This is not needed in ramstage.
File src/mainboard/lenovo/thinkcentre_m710s/devicetree.cb:
Patch Set #5, Line 5: # FSP Configuration
This is a remnant from times where we configured a lot of FSP directly at
the toplevel. The `eist_enable` in particular is a coreboot setting. So
please drop the comment.
Patch Set #5, Line 10: device ref system_agent on end
This one is essential and hence already enabled in the `chipset.cb`. You
can drop this at your convenience.
Patch Set #5, Line 14: register "PcieRpClkSrcNumber[0]" = "0"
Did you test the x16 slot? Please mention that one explicitly in the
commit message.
The `PcieRp*` settings are actually for the PCH root ports. Though, it
seems Intel forgot to add ClkReq settings for the PEG. If this works,
it deserves a comment.
Patch Set #5, Line 51: register "PcieRpClkReqSupport[4]" = "false"
In the schematics it seems connected as CLKREQ_LAN#. If it's not working
please add a comment that it's unknown why.
I see a PCIe-PCI bridge in the schematics on `pcie_rp6`. Is it not populated?
Patch Set #5, Line 112: # Vendor values dumped using util/superiotool.
Did you push a superiotool patch yet? I'm still a little concerned about
the shift in LDNs we were seeing. But if the current port works, e.g. COM1
in the OS, then the numbers here must be correct.
File src/mainboard/lenovo/thinkcentre_m710s/early_init.c:
Please join this with `bootblock.c`. We usually have either one of them.
File src/mainboard/lenovo/thinkcentre_m710s/gpio.h:
Patch Set #5, Line 14: _PAD_CFG_STRUCT(GPP_A0, PAD_FUNC(NF1) | PAD_RESET(PLTRST) | PAD_TRIG(OFF) | PAD_BUF(TX_DISABLE) | (1 << 1), 0),
Technically the _-prefixed macros are reserved for internal use in
soc/intel/common/. If there is no compatible high-level macro, that's
something to investigate.
I don't mind merging it like this as there are other cases in the tree.
But I'll skip the GPIOs in this review.
File src/mainboard/lenovo/thinkcentre_m710s/hda_verb.c:
Patch Set #5, Line 20: AZALIA_PIN_CFG(0, 0x1e, 0x411111f0),
We could decode these (there are plenty of definitions and macros
in device/azalia*.h). Though, having this done by hand once, I know
it's not fun. We should write a tool for the decoding, I guess. Or
live with the encoded numbers (nobody seems to care anyway).
Anyway, this is good enough. Just wanted to have it mentioned that
it's not all magic ;)
File src/mainboard/lenovo/thinkcentre_m710s/romstage.c:
First should be 60, for the 4 DIMM configuration.
No space after the casts, please.
To view, visit change 80343. To unsubscribe, or for help writing mail filters, visit settings.