Tim Crawford has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47892 )
Change subject: mb/system76/oryp5: Add System76 Oryx Pro 5 ......................................................................
Patch Set 5:
(8 comments)
https://review.coreboot.org/c/coreboot/+/47892/5/Documentation/mainboard/sys... File Documentation/mainboard/system76/oryp5.md:
https://review.coreboot.org/c/coreboot/+/47892/5/Documentation/mainboard/sys... PS5, Line 12: - eDP 16.1" or 17.3" 1920x1080 @ 144 Hz LCD
Does backlight control work properly? I'd suggest testing under Windows. […]
I will have to rebase and check. (They worked on Linux, which was good enough for me.)
https://review.coreboot.org/c/coreboot/+/47892/5/Documentation/mainboard/sys... PS5, Line 15: 2666 MHz
DDR4-2666 runs at 1333 MHz, or 2666 MT/s.
These are from the marketing material, which it seems most vendors use MHz instead of MT/s. I will probably change this to the channel 0/1 format used in the lemp0 documentation.
https://review.coreboot.org/c/coreboot/+/47892/5/src/mainboard/system76/oryp... File src/mainboard/system76/oryp5/Kconfig:
https://review.coreboot.org/c/coreboot/+/47892/5/src/mainboard/system76/oryp... PS5, Line 12: HAVE_SMI_HANDLER
Isn't this selected elsewhere?
Selected by the SOC, will remove.
https://review.coreboot.org/c/coreboot/+/47892/2/src/mainboard/system76/oryp... File src/mainboard/system76/oryp5/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/47892/2/src/mainboard/system76/oryp... PS2, Line 26: register "SaGv" = "SaGv_Enabled"
Halo is the name of high-performance BGA CPUs. That is, the models ending in H, such as i7-9750H. […]
SaGv is enabled on the RVP. (It was copied from CNL RVP, so a chance it was just overlooked?)
https://review.coreboot.org/c/coreboot/+/47892/5/src/mainboard/system76/oryp... File src/mainboard/system76/oryp5/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/47892/5/src/mainboard/system76/oryp... PS5, Line 41: WARNING: must then be mapped from FSP value to PCH value
Did you (or whoever originally wrote this) mean `from PCH value to FSP value`?
Potentially. But I'm more inclined to just remove the comments like this. They may be useful in a guide for porting a board, but not really as comments in *every* board (what we currently have).
https://review.coreboot.org/c/coreboot/+/47892/5/src/mainboard/system76/oryp... File src/mainboard/system76/oryp5/gpio.c:
https://review.coreboot.org/c/coreboot/+/47892/5/src/mainboard/system76/oryp... PS5, Line 98: PAD_NC(GPP_C20, UP_20K), // UART2_RXD (test point) : PAD_NC(GPP_C21, UP_20K), // UART2_TXD (test point)
This conflicts with the devicetree setting
Changing these back to NF. Should I move them to early_gpio.c as well?
https://review.coreboot.org/c/coreboot/+/47892/2/src/mainboard/system76/oryp... File src/mainboard/system76/oryp5/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/47892/2/src/mainboard/system76/oryp... PS2, Line 25: // Enable DMIC microphone on ALC1220 : 0x02050036, : 0x02042a6a,
I guess people put these here because the cim_verb_data format only allows groups of 4 DWORDs. […]
The ALC1220 spec we got doesn't have the supported verbs, so I can't even confirm what these actually do.
https://review.coreboot.org/c/coreboot/+/47892/5/src/mainboard/system76/oryp... File src/mainboard/system76/oryp5/romstage.c:
https://review.coreboot.org/c/coreboot/+/47892/5/src/mainboard/system76/oryp... PS5, Line 25: 0
Shouldn't this be synchronized with ONBOARD_VGA_IS_PRIMARY in SoC code?
What do you mean by synchronized?
I probably don't need this set at all. Will remove it if default of AUTO works.
-
Tim Crawford (Code Review)