Attention is currently required from: amersel. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57085 )
Change subject: [wip]mb/dell: add Dell Optiplex XE2 ......................................................................
Patch Set 2:
(33 comments)
Patchset:
PS2: Nice!
File src/mainboard/dell/optiplex_xe2/Kconfig:
https://review.coreboot.org/c/coreboot/+/57085/comment/b44e68fa_fc3594f9 PS2, Line 6: select CPU_INTEL_HASWELL Should be selected by NORTHBRIDGE_INTEL_HASWELL already
https://review.coreboot.org/c/coreboot/+/57085/comment/ce5e6465_6e02ce0a PS2, Line 11: # FIXME: check this Drop the comment?
https://review.coreboot.org/c/coreboot/+/57085/comment/e28bb971_443e8985 PS2, Line 17: TSC_MONOTONIC_TIMER huh?
https://review.coreboot.org/c/coreboot/+/57085/comment/6329dea5_beda79d3 PS2, Line 21: #config GFX_GMA_CPU_VARIANT : # string : # default "Normal" Please drop
https://review.coreboot.org/c/coreboot/+/57085/comment/efc767e5_b8491fad PS2, Line 37: config VGA_BIOS_FILE : string : default "pci8086,0412.rom" : : config VGA_BIOS_ID : string : default "8086,0412" This depends on the installed processor, please remove
https://review.coreboot.org/c/coreboot/+/57085/comment/3cce9ed4_0a5e6dff PS2, Line 45: config MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID : hex : default 0x5c1 : : config MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID : hex : default 0x1028 No longer makes a difference, please remove
https://review.coreboot.org/c/coreboot/+/57085/comment/e284ed6b_2f6c1cb3 PS2, Line 53: #config MAX_CPUS : # int : # default 8 No longer needed, can be removed
https://review.coreboot.org/c/coreboot/+/57085/comment/e24d9363_e2590aaa PS2, Line 57: #config USBDEBUG_HCD_INDEX # FIXME: check this : # int : # default 2 If untested, just remove this
File src/mainboard/dell/optiplex_xe2/bootblock.c:
https://review.coreboot.org/c/coreboot/+/57085/comment/3957e251_b849aa7b PS2, Line 16: Trailing whitespace
https://review.coreboot.org/c/coreboot/+/57085/comment/7b417a7d_256f19ce PS2, Line 17: // Wait for acknowledgement message from EC Please use tabs for alignment
https://review.coreboot.org/c/coreboot/+/57085/comment/d1c71086_482bae11 PS2, Line 44: : Same two things as above
https://review.coreboot.org/c/coreboot/+/57085/comment/f5f3cce4_4c13edf8 PS2, Line 59: : Same two things as above
https://review.coreboot.org/c/coreboot/+/57085/comment/cff7ff6d_7c87df28 PS2, Line 62: : Same two things as above
https://review.coreboot.org/c/coreboot/+/57085/comment/f808fba8_204db6c8 PS2, Line 107: : Same two things as above
https://review.coreboot.org/c/coreboot/+/57085/comment/8224fb07_3b560817 PS2, Line 110: : Same two things as above
https://review.coreboot.org/c/coreboot/+/57085/comment/2a5109d7_6daeeea5 PS2, Line 113: } Please ensure files end with a newline
File src/mainboard/dell/optiplex_xe2/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/57085/comment/279802d7_701412bf PS2, Line 8: #register "c1_acpower" = "1" : #register "c1_battery" = "1" : #register "c2_acpower" = "3" : #register "c2_battery" = "3" : #register "c3_acpower" = "5" : #register "c3_battery" = "5" You can remove this
https://review.coreboot.org/c/coreboot/+/57085/comment/e5d33bd2_42b65312 PS2, Line 14: 0x0 nit: 0
https://review.coreboot.org/c/coreboot/+/57085/comment/b76a5b4f_515c4c7f PS2, Line 24: # register "sata_ahci" = "1" You can remove this too
https://review.coreboot.org/c/coreboot/+/57085/comment/454f204d_07566af3 PS2, Line 25: register "sata_port_map" = "0x63" Uhhh, really? This value enables an inexistent SATA port. If unsure, `0x3f` enables all SATA ports.
https://review.coreboot.org/c/coreboot/+/57085/comment/f2bceb95_6fb6d7e9 PS2, Line 27: subsystemid 0x1028 0x05c1 You can inherit the subsystemid (see other mainboards, look for the `inherit` keyword)
https://review.coreboot.org/c/coreboot/+/57085/comment/1cbdd19a_7508ea67 PS2, Line 53: device pci 1c.2 off # PCIe Port #3 : end I'd appreciate if you could place the `end` keywords in the previous line (see mb/asrock/b85m_pro4 for an example of what I mean)
https://review.coreboot.org/c/coreboot/+/57085/comment/7d216469_da3718c0 PS2, Line 68: LPC bridge PCI-LPC bridge Just `LPC bridge` is enough
https://review.coreboot.org/c/coreboot/+/57085/comment/75a6a7f1_3291d975 PS2, Line 94: chip drivers/pc80/tpm : device pnp 0c31.0 on end : end tabs, please
https://review.coreboot.org/c/coreboot/+/57085/comment/a4cf3bb6_2443dd7e PS2, Line 98: Unsupported PCI device 8086:2822 I'd drop this part
https://review.coreboot.org/c/coreboot/+/57085/comment/8a4a169e_174c63c4 PS2, Line 109: Host bridge Host bridge One instance of `Host bridge` in the comment is enough
https://review.coreboot.org/c/coreboot/+/57085/comment/dbd9f9d7_40d98f40 PS2, Line 109: device pci 00.0 on end # Host bridge Host bridge : device pci 01.0 on end # PCIe Bridge for discrete graphics : device pci 02.0 on end # Internal graphics VGA controller : device pci 03.0 on end # Mini-HD audio Please move these devices right above `chip southbridge/intel/lynxpoint`
File src/mainboard/dell/optiplex_xe2/gma-mainboard.ads:
https://review.coreboot.org/c/coreboot/+/57085/comment/b8a132f3_24709d8b PS2, Line 20: LVDS, : eDP Most likely not. You can replace this with `others => Disabled`
File src/mainboard/dell/optiplex_xe2/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/57085/comment/16247c30_e7857395 PS2, Line 22: nit: drop empty line
File src/mainboard/dell/optiplex_xe2/romstage.c:
https://review.coreboot.org/c/coreboot/+/57085/comment/781ae5a4_5eddb011 PS2, Line 9: trailing whitespace
https://review.coreboot.org/c/coreboot/+/57085/comment/bf341f4e_15704e3d PS2, Line 19: //RCBA_RMW_REG_32(FD, ~0, PCH_DISABLE_ALWAYS), Remove this please
https://review.coreboot.org/c/coreboot/+/57085/comment/1365ff45_e7f94fea PS2, Line 58: // void mainboard_romstage_entry(unsigned long bist) : // { : // struct pei_data pei_data = { : // .pei_version = PEI_VERSION, : // .mchbar = (uintptr_t)DEFAULT_MCHBAR, : // .dmibar = (uintptr_t)DEFAULT_DMIBAR, : // .epbar = DEFAULT_EPBAR, : // .pciexbar = DEFAULT_PCIEXBAR, : // .smbusbar = SMBUS_IO_BASE, : // .wdbbar = 0x4000000, : // .wdbsize = 0x1000, : // .hpet_address = HPET_ADDR, : // .rcba = (uintptr_t)DEFAULT_RCBA, : // .pmbase = DEFAULT_PMBASE, : // .gpiobase = DEFAULT_GPIOBASE, : // .temp_mmio_base = 0xfed08000, : // .system_type = 1, /* desktop/server, FIXME: check this */ : // .tseg_size = CONFIG_SMM_TSEG_SIZE, : // .spd_addresses = { 0xa0, 0xa2, 0xa4, 0xa6 }, /* FIXME: check this */ : // .ec_present = 0, : // .dimm_channel0_disabled = 0, /* FIXME: leave channel 0 enabled */ : // .dimm_channel1_disabled = 0, /* FIXME: leave channel 1 enabled */ : // .max_ddr3_freq = 1600, : // .usb2_ports = { : // /* Length, Enable, OCn#, Location */ : // { 0x0040, 1, USB_OC_PIN_SKIP, USB_PORT_BACK_PANEL }, : // { 0x0040, 1, USB_OC_PIN_SKIP, USB_PORT_BACK_PANEL }, : // { 0x0040, 1, 1, USB_PORT_BACK_PANEL }, : // { 0x0040, 1, 2, USB_PORT_BACK_PANEL }, : // { 0x0040, 1, 3, USB_PORT_BACK_PANEL }, : // { 0x0040, 1, 3, USB_PORT_BACK_PANEL }, : // { 0x0040, 1, 0, USB_PORT_BACK_PANEL }, : // { 0x0040, 1, 0, USB_PORT_BACK_PANEL }, : // { 0x0040, 1, 4, USB_PORT_BACK_PANEL }, : // { 0x0040, 1, 4, USB_PORT_BACK_PANEL }, : // { 0x0040, 1, 5, USB_PORT_BACK_PANEL }, : // { 0x0040, 1, 5, USB_PORT_BACK_PANEL }, : // { 0x0040, 1, 6, USB_PORT_BACK_PANEL }, : // { 0x0040, 1, 7, USB_PORT_BACK_PANEL }, : // }, : // .usb3_ports = { : // { 1, 6 }, : // { 1, 7 }, : // { 1, USB_OC_PIN_SKIP }, : // { 1, USB_OC_PIN_SKIP }, : // { 1, 1 }, : // { 1, 2 }, : // }, : // }; : : // struct romstage_params romstage_params = { : // .pei_data = &pei_data, : // .gpio_map = &mainboard_gpio_map, : // .rcba_config = rcba_config, : // .bist = bist, : // }; : : // romstage_common(&romstage_params); : // } This will never be needed again
-
Angel Pons (Code Review)