Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31603 )
Change subject: mainboard: Add ASRock H110M-DVS ......................................................................
Patch Set 2:
(10 comments)
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/Kconfig File src/mainboard/asrock/h110m/Kconfig:
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/Kconfig@1... PS2, Line 16: TSC_MONOTONIC_TIMER already selected in the skylake soc Kconfig
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/Kconfig@2... PS2, Line 20: HAVE_SMI_HANDLER already selected in the skylake soc Kconfig
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/Kconfig@2... PS2, Line 24: choice : prompt "Processor family" : default USE_SOC_INTEL_SKYLAKE : : config USE_SOC_INTEL_SKYLAKE : bool "Skylake" : select SOC_INTEL_SKYLAKE : : config USE_SOC_INTEL_KABYLAKE : bool "Kabylake" : select SOC_INTEL_KABYLAKE : : endchoice is making this distinction really needed? you end up using the kabylake FSP anyway iirc.
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/Kconfig@4... PS2, Line 45: : config VARIANT_DIR : string : default "dvs" if BOARD_ASROCK_H110M_DVS remove until you add variants.
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/Kconfig@7... PS2, Line 73: BOARD_ASROCK_H110M_DVS not needed.
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/Kconfig@7... PS2, Line 78: BOARD_ASROCK_H110M_DVS not needed.
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/Kconfig@8... PS2, Line 84: config DIMM_SPD_SIZE : int : default 512 #DDR4 not the scope of your patch but I don't think it ought to be defined here. Afaik the fsp has size expectation on the array passed and that is not board specific.
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/variants/... File src/mainboard/asrock/h110m/variants/baseboard/devicetree.cb:
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/variants/... PS2, Line 38: # EC host command ranges are in 0x800-0x8ff & 0x200-0x20f : register "gen1_dec" = "0x00fc0801 I don't see a device decoding this range.
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/variants/... PS2, Line 156: register "pirqa_routing" = "0x0b" : register "pirqb_routing" = "0x0a" : register "pirqc_routing" = "0x0b" : register "pirqd_routing" = "0x0b" : register "pirqe_routing" = "0x0b" : register "pirqf_routing" = "0x0b" : register "pirqg_routing" = "0x0b" : register "pirqh_routing" = "0x0b" already done at line 60.
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/variants/... File src/mainboard/asrock/h110m/variants/dvs/overridetree.cb:
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/variants/... PS2, Line 1: ## Why use an override tree for a single board? do you intend to add variants?