Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33328 )
Change subject: mainboard: Add support for ASUS P8Z77-M PRO desktop mainboard ......................................................................
Patch Set 11:
(28 comments)
https://review.coreboot.org/#/c/33328/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33328/6//COMMIT_MSG@10 PS6, Line 10: Signed-off-by: Vlado Cibic vladocb@protonmail.com We generally have a newline before the Signed-off-by line.
https://review.coreboot.org/#/c/33328/11/src/mainboard/asus/p8z77-m_pro/Kcon... File src/mainboard/asus/p8z77-m_pro/Kconfig:
https://review.coreboot.org/#/c/33328/11/src/mainboard/asus/p8z77-m_pro/Kcon... PS11, Line 35: # select HAVE_IFD_BIN # uncomment if needed : # select HAVE_ME_BIN # uncomment if needed remove
https://review.coreboot.org/#/c/33328/11/src/mainboard/asus/p8z77-m_pro/Kcon... PS11, Line 49: his is used by libgfxinit. This is untrue.
https://review.coreboot.org/#/c/33328/11/src/mainboard/asus/p8z77-m_pro/Kcon... PS11, Line 45: : config INTEL_GMA_ADD_VBT : def_bool y : : # This is used by libgfxinit. Comment it if you prefer to use VGA OpROMs : config INTEL_GMA_VBT_FILE : string : default "src/mainboard/$(MAINBOARDDIR)/data.vbt" "select INTEL_GMA_HAVE_VBT", does both these things.
https://review.coreboot.org/#/c/33328/11/src/mainboard/asus/p8z77-m_pro/Kcon... PS11, Line 54: # Set this to use your IGP as primary display instead of the GPU : config ONBOARD_VGA_IS_PRIMARY : def_bool n No need to redefine this. You can do this in make nconfig
https://review.coreboot.org/#/c/33328/11/src/mainboard/asus/p8z77-m_pro/Kcon... PS11, Line 66: # This board works with both Native RAM Init + MRC blob. Use NRI as default. : config USE_NATIVE_RAMINIT : def_bool y No need to redefine this here.
https://review.coreboot.org/#/c/33328/11/src/mainboard/asus/p8z77-m_pro/Kcon... PS11, Line 70: : # We dont use USB2.0 (EHCI) debugging. Uncomment it if you need. Just leaving it in here does not hurt.
https://review.coreboot.org/#/c/33328/11/src/mainboard/asus/p8z77-m_pro/Kcon... PS11, Line 81: config USE_OPTION_TABLE : def_bool y Why override the default?
https://review.coreboot.org/#/c/33328/11/src/mainboard/asus/p8z77-m_pro/Kcon... PS11, Line 84: config IFD_BIN_PATH : string : default "3rdparty/blobs/mainboard/$(MAINBOARDDIR)/descriptor.bin" : : config ME_BIN_PATH : string : default "3rdparty/blobs/mainboard/$(MAINBOARDDIR)/me.bin" Already default, remove.
https://review.coreboot.org/#/c/33328/11/src/mainboard/asus/p8z77-m_pro/Kcon... PS11, Line 92: # : # Uncomment if you need to partial-write parts of the bios image : # : #config IFD_SECTION : # string : # default "0x00000000:00000fff" : # : #config IFD_BIOS_SECTION : # string : # default "0x00180000:007fffff" : # : #config IFD_ME_SECTION : # string : # default "0x00001000:0017ffff" Those seem unused in the code.
https://review.coreboot.org/#/c/33328/11/src/mainboard/asus/p8z77-m_pro/Kcon... PS11, Line 108: config GENERATE_SMBIOS_TABLES : def_bool y no need to redefine this.
https://review.coreboot.org/#/c/33328/11/src/mainboard/asus/p8z77-m_pro/Kcon... PS11, Line 124: DIMM_MAX unused by the sandybridge code afaict. Remove
https://review.coreboot.org/#/c/33328/11/src/mainboard/asus/p8z77-m_pro/Kcon... PS11, Line 128: # This board has a power button, it does not power-on via a : # jumper as other devboards. : config POWER_BUTTON_DEFAULT_ENABLE : def_bool y There seems to be no code with this option. Remove?
https://review.coreboot.org/#/c/33328/11/src/mainboard/asus/p8z77-m_pro/Kcon... PS11, Line 132: : if USE_NATIVE_RAMINIT : config MMCONF_BASE_ADDRESS : hex : default 0xf8000000 : help : Set MMCONF_BASE_ADDRESS to 0xf8000000. It was already done for some : boards, but not all. The Sandy Bridge chipset code assumes 64 pci : buses behind MMCONF. See: : src/northbridge/sandybridge/bootblock.c:bootblock_northbridge_init() : Therefore, only 64MiB of physical address space is required. : Increasing the address allows to use additional 128MiB of MMIO space : to use the Intel IGP and a PEG at the same time. Previously it wasn't : possible to use both at the same time, as two 256MiB areas won't fit : into MMIO space. : endif # MRC requires fixed MMCONF address at 0xf0000000 It was made common such that the bootblocks are compatible between the 2 codepaths. I'd increase 'register "pci_mmio_size"' in the devicetree.cb if you need more mmiospace, but it is quite surprising you're not having enough since the default is 2048M.
https://review.coreboot.org/#/c/33328/6/src/mainboard/asus/p8z77-m_pro/Makef... File src/mainboard/asus/p8z77-m_pro/Makefile.inc:
https://review.coreboot.org/#/c/33328/6/src/mainboard/asus/p8z77-m_pro/Makef... PS6, Line 18: : # as we using libgfxinit, we need to include this Remove this comment, as it is clear from the code.
https://review.coreboot.org/#/c/33328/6/src/mainboard/asus/p8z77-m_pro/cmos.... File src/mainboard/asus/p8z77-m_pro/cmos.layout:
https://review.coreboot.org/#/c/33328/6/src/mainboard/asus/p8z77-m_pro/cmos.... PS6, Line 63: hyper_threading unused option in the code.
https://review.coreboot.org/#/c/33328/6/src/mainboard/asus/p8z77-m_pro/cmos.... PS6, Line 63: 421 1 e 8 hyper_threading : : # usb3_xxxx are just used when using MRC blob to init ram/usb/pci. : # Not used when using Native RAM Init. : 422 2 e 9 usb3_mode : 424 1 e 10 usb3_preOS_drv : 425 1 e 11 usb3_streams : 426 1 e 12 usb_powered_on_s5 Please use the same formatting (without tabs) as the rest?
https://review.coreboot.org/#/c/33328/6/src/mainboard/asus/p8z77-m_pro/cmos.... PS6, Line 165: src/cpu/intel/hyperthreading/intel_sibling.c:intel_sibling_init() the model_206ax code does not use this.
https://review.coreboot.org/#/c/33328/10/src/mainboard/asus/p8z77-m_pro/devi... File src/mainboard/asus/p8z77-m_pro/devicetree.cb:
https://review.coreboot.org/#/c/33328/10/src/mainboard/asus/p8z77-m_pro/devi... PS10, Line 25: register "gpu_panel_port_select" = "0" : register "gpu_panel_power_backlight_off_delay" = "0" : register "gpu_panel_power_backlight_on_delay" = "0" : register "gpu_panel_power_cycle_delay" = "4" : register "gpu_panel_power_down_delay" = "0" : register "gpu_panel_power_up_delay" = "0" : register "gpu_pch_backlight" = "0x00000000" remove, default is 0.
https://review.coreboot.org/#/c/33328/10/src/mainboard/asus/p8z77-m_pro/devi... PS10, Line 49: register "docking_supported" = "0 remove, default is 0.
https://review.coreboot.org/#/c/33328/10/src/mainboard/asus/p8z77-m_pro/devi... PS10, Line 51: register "gen2_dec" = "0x00000000" : register "gen3_dec" = "0x00000000" remove, default is 0.
https://review.coreboot.org/#/c/33328/10/src/mainboard/asus/p8z77-m_pro/devi... PS10, Line 100: device pci 1f.0 on # LPC bridge PCI-LPC bridge Configure your superio below here.
https://review.coreboot.org/#/c/33328/8/src/mainboard/asus/p8z77-m_pro/mainb... File src/mainboard/asus/p8z77-m_pro/mainboard.c:
https://review.coreboot.org/#/c/33328/8/src/mainboard/asus/p8z77-m_pro/mainb... PS8, Line 19: #include <northbridge/intel/sandybridge/sandybridge.h> : #include <southbridge/intel/bd82x6x/pch.h> : #include <device/pci_type.h> : #include <device/pci_ops.h> unused?
https://review.coreboot.org/#/c/33328/4/src/mainboard/asus/p8z77-m_pro/romst... File src/mainboard/asus/p8z77-m_pro/romstage.c:
https://review.coreboot.org/#/c/33328/4/src/mainboard/asus/p8z77-m_pro/romst... PS4, Line 84: mainboard_config_superio In romstage you just want the serial console to work. All other things are better done via the devicetree/ramstage.
https://review.coreboot.org/#/c/33328/8/src/mainboard/asus/p8z77-m_pro/romst... File src/mainboard/asus/p8z77-m_pro/romstage.c:
https://review.coreboot.org/#/c/33328/8/src/mainboard/asus/p8z77-m_pro/romst... PS8, Line 34: #if CONFIG(USE_NATIVE_RAMINIT) The codepaths can coexist without CPP
https://review.coreboot.org/#/c/33328/8/src/mainboard/asus/p8z77-m_pro/romst... PS8, Line 49: 0x000c0291u Passing an int to a function expecting an unsigned int is well defined behaviour, there is a trivial conversion. I'd prefer not to needlessly clutter the notation.
https://review.coreboot.org/#/c/33328/8/src/mainboard/asus/p8z77-m_pro/romst... PS8, Line 139: (uint8_t) It is expected C behaviour to discard higher bits, so no need to do that explicitly
https://review.coreboot.org/#/c/33328/8/src/mainboard/asus/p8z77-m_pro/romst... PS8, Line 288: (uint16_t) it is expected C behaviour to remove higher bits. No need to do that explicitly.