Angel Pons 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:
(10 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. Some panel settings seem to be missing, which have been added recently in CB:48749
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.
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?
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"
What's Halo? Is there a way I can verify that?
Halo is the name of high-performance BGA CPUs. That is, the models ending in H, such as i7-9750H.
Enabling SaGv makes FSP-M train the memory at two different speeds, which then allows dynamically changing the memory frequency in order to save power. However, memory training takes a long time (15 seconds, or even more), twice as much when SaGv is enabled.
I don't know of a way to verify if SaGv is being used, though you could check if disabling SaGv or setting it to FixedHigh has an impact on battery life. You should also see that booting after reflashing or changing the DIMMs is about twice as fast, which can save lots of time when testing.
https://review.coreboot.org/c/coreboot/+/47892/3/src/mainboard/system76/oryp... File src/mainboard/system76/oryp5/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/47892/3/src/mainboard/system76/oryp... PS3, Line 87: subsystemid 0x1558 0x95e6 inherit
FSP integration guide only lists the registers with abbreviated names. […]
Ah, cursed FSP
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`?
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
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,
Jeremy got these magic numbers from the Windows driver, and putting them here happens to make the DM […]
I guess people put these here because the cim_verb_data format only allows groups of 4 DWORDs. Ugh, I don't like it but can't come up with a simple solution...
https://review.coreboot.org/c/coreboot/+/47892/2/src/mainboard/system76/oryp... File src/mainboard/system76/oryp5/romstage.c:
https://review.coreboot.org/c/coreboot/+/47892/2/src/mainboard/system76/oryp... PS2, Line 52: DRAM
It seems all of these are copied from the header (error including). […]
Thanks
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?
-
Angel Pons (Code Review)