Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36685 )
Change subject: src/mb/intel/coffeelake_rvp: Add mainboard for CML-S RVP8 ......................................................................
Patch Set 10: Code-Review+1
(9 comments)
Patch Set 9:
anything needs to do with this patch?
Other than two comments left by Arthur, I think it's good.
https://review.coreboot.org/c/coreboot/+/36685/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36685/3//COMMIT_MSG@10 PS3, Line 10: CML-U RVP to RVPU to avoid confusing
OKAY. […]
Done
https://review.coreboot.org/c/coreboot/+/36685/3/src/mainboard/intel/coffeel... File src/mainboard/intel/coffeelake_rvp/Kconfig:
https://review.coreboot.org/c/coreboot/+/36685/3/src/mainboard/intel/coffeel... PS3, Line 64: : config DEVICETREE : string : default "variants/$(CONFIG_VARIANT_DIR)/devicetree.cb"
okay. will do.
Done
https://review.coreboot.org/c/coreboot/+/36685/6/src/mainboard/intel/coffeel... File src/mainboard/intel/coffeelake_rvp/Kconfig:
https://review.coreboot.org/c/coreboot/+/36685/6/src/mainboard/intel/coffeel... PS6, Line 18: || BOARD_INTEL_COFFEELAKE_RVP8
good catch.
Done
https://review.coreboot.org/c/coreboot/+/36685/6/src/mainboard/intel/coffeel... PS6, Line 47: || BOARD_INTEL_COMETLAKE_RVP8
good catch. done.
Done
https://review.coreboot.org/c/coreboot/+/36685/3/src/mainboard/intel/coffeel... File src/mainboard/intel/coffeelake_rvp/chromeos_32MB.fmd:
https://review.coreboot.org/c/coreboot/+/36685/3/src/mainboard/intel/coffeel... PS3, Line 5: SI_GBE@0x81000 0x2000
Your comment is correct. Will revise it.
Arthur, is this correct now?
https://review.coreboot.org/c/coreboot/+/36685/3/src/mainboard/intel/coffeel... File src/mainboard/intel/coffeelake_rvp/variants/cml_s/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/36685/3/src/mainboard/intel/coffeel... PS3, Line 164:
newline at the end.
Is this still applicable?
https://review.coreboot.org/c/coreboot/+/36685/10/src/mainboard/intel/coffee... File src/mainboard/intel/coffeelake_rvp/variants/cml_s/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/36685/10/src/mainboard/intel/coffee... PS10, Line 73: register "PcieRpEnable[0]" = "0" I think you can omit disabled PCIe root ports, as the default value is zero
https://review.coreboot.org/c/coreboot/+/36685/10/src/mainboard/intel/coffee... PS10, Line 91: register "PcieRpEnable[4]" = "1" Maybe add a comment that these are for the Thunderbolt controller
https://review.coreboot.org/c/coreboot/+/36685/10/src/mainboard/intel/coffee... PS10, Line 151: Tunderbolt typo: missing "h" in Thunderbolt