Attention is currently required from: Joel Linn, Matt DeVillier.
Nicholas Chin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81368?usp=email )
Change subject: mb/hp: Add Pro 3500 series (Sandy/Ivy Bridge) ......................................................................
Patch Set 2:
(32 comments)
Patchset:
PS2: Welcome to coreboot! Thanks for the port. We hope to see many more from you.
I left a bunch of comments, some of which are probably a bit nitpicky, so don't feel like you need to fix them if you don't want to. A bunch of things are also just the result of autoport not really being maintained and omitting things that are now required on all files, like license identifiers.
File Documentation/mainboard/hp/pro_3500_series.md:
PS2: Please reflow all lines to 72 columns (though I do see many documentation files that use 80 columns)
PS2: This file needs to be added to `Documentation/mainboard/index.html` so that it will actually show up in the generated html
https://review.coreboot.org/c/coreboot/+/81368/comment/b3996ab9_dca32968 : PS2, Line 6: Remove trailing space
https://review.coreboot.org/c/coreboot/+/81368/comment/86903322_3d84105c : PS2, Line 12: With disabled ME, the SuperIO will not get CPU temperatures via PECI and therefore the automatic fan control will not increase the fan speed. Reflow to 72 characters
https://review.coreboot.org/c/coreboot/+/81368/comment/2fe6385e_37f42ff5 : PS2, Line 87: ITE8779e Probably should be either `IT8779E` or `ITE IT8779E`
File Documentation/mainboard/hp/pro_3500_series_flash.jpg:
PS2: Also, would you mind running the image through an image optimizer website (such as https://jpeg-optimizer.com/)? Images sizes do add up in the repo, and usually there isn't a need for the maximum possible clarity. See https://review.coreboot.org/c/coreboot/+/67818/comment/e9e39fa9_a5451496/
PS2: The red is a bit difficult to read here.
By the way, the pinout of this header is already documented on https://doc.coreboot.org/tutorial/flashing_firmware/ext_standalone.html#hp-b... (source file `Documentation/tutorial/flashing_firmware/ext_standalone.md`), though this image is still useful for helping locate the flash.
File Documentation/mainboard/hp/pro_3500_series_jumper.jpg:
PS2: Same here, probably could be reduced in size
File src/mainboard/hp/pro_3500_series/Kconfig:
PS2: Needs an SPDX license identifier
https://review.coreboot.org/c/coreboot/+/81368/comment/584e7498_6a6979d1 : PS2, Line 5: select BOARD_ROMSIZE_KB_8192 : select HAVE_ACPI_RESUME : select HAVE_ACPI_TABLES : select INTEL_INT15 : select MAINBOARD_HAS_LIBGFXINIT : select INTEL_GMA_HAVE_VBT : select MAINBOARD_USES_IFD_GBE_REGION : select NORTHBRIDGE_INTEL_SANDYBRIDGE : select SERIRQ_CONTINUOUS_MODE : select SOUTHBRIDGE_INTEL_BD82X6X : select SUPERIO_ITE_IT8772F : select USE_NATIVE_RAMINIT Please sort these alphabetically
https://review.coreboot.org/c/coreboot/+/81368/comment/c5d0110f_62fef45e : PS2, Line 22: string Remove to avoid redefining types, as it is already declared in other shared Kconfig files
https://review.coreboot.org/c/coreboot/+/81368/comment/a8dd24a1_3e89bcfb : PS2, Line 26: string Remove
https://review.coreboot.org/c/coreboot/+/81368/comment/dadb879d_47271d5a : PS2, Line 30: string Remove
https://review.coreboot.org/c/coreboot/+/81368/comment/bd2e9fb7_5be23227 : PS2, Line 34: int Remove
https://review.coreboot.org/c/coreboot/+/81368/comment/1cf80794_e10c447f : PS2, Line 38: int Remove
File src/mainboard/hp/pro_3500_series/Kconfig.name:
PS2: Needs an SPDX license identifier
File src/mainboard/hp/pro_3500_series/Makefile.mk:
PS2: Needs an SPDX license identifier
File src/mainboard/hp/pro_3500_series/acpi/ec.asl:
PS2: Should contain ``` /* SPDX-License-Identifier: CC-PDDC */
/* Please update the license if adding licensable material. */ ``` Refer to commit cf4722d317
File src/mainboard/hp/pro_3500_series/devicetree.cb:
PS2: Needs an SPDX license identifier
https://review.coreboot.org/c/coreboot/+/81368/comment/0a17bcc0_0bbc96b3 : PS2, Line 22: register "gen2_dec" = "0x00000000" : register "gen3_dec" = "0x00000000" : register "gen4_dec" = "0x00000000" Can be removed
https://review.coreboot.org/c/coreboot/+/81368/comment/f7504312_f3fbbb71 : PS2, Line 35: # Management Engine Interface 1 The device ref aliases make some of these comments redundant, so they should be removed
https://review.coreboot.org/c/coreboot/+/81368/comment/034ddbec_0bff7ebd : PS2, Line 36: subsystemid 0x103c 0x2abf All of these use the same subsystemid, so you can add the line `subsystemid 0x103c 0x2abf inherit` right after the `device domain 0x0 on` line, and all subsequent devices will inherit it. Then all the individual subsystemid lines can be dropped. See `mb/hp/elitebook_820_g1/devicetree.cb` for an example of this.
https://review.coreboot.org/c/coreboot/+/81368/comment/321aaddf_632f0a74 : PS2, Line 38: device ref mei2 off # Management Engine Interface 2 : end Typically the `end` is moved to the same line as the `device`, after the on/off, if there isn't anything nested within it to make the devicetree cleaner.
https://review.coreboot.org/c/coreboot/+/81368/comment/9e67b35f_cf5a45ba : PS2, Line 150: device ref host_bridge on # Host bridge Host bridge : subsystemid 0x103c 0x2abf : end : device ref peg10 on # PEG : subsystemid 0x103c 0x2abf : end : device ref igd on # iGPU : subsystemid 0x103c 0x2abf : end I guess this just comes down to style and maybe organization, but typically these devices are placed before the southbridge chip entry, right after `device domain 0x0 on`. Just a quirk of how autoport operates.
File src/mainboard/hp/pro_3500_series/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/81368/comment/9a8fb56c_eb7e7f1e : PS2, Line 3: /* SPDX-License-Identifier: GPL-2.0-only */ The license identifier should be the first line
File src/mainboard/hp/pro_3500_series/early_init.c:
https://review.coreboot.org/c/coreboot/+/81368/comment/13cc2732_3d30a3f6 : PS2, Line 2: Nit: Remove extra blank line
https://review.coreboot.org/c/coreboot/+/81368/comment/edcb8c2a_7a4b7d81 : PS2, Line 33: 0x1400 Could be replaced with defines from `southbridge/intel/bd82x6x/pch.h` (line 140 ish) to more clearly show what's being enabled.
https://review.coreboot.org/c/coreboot/+/81368/comment/74aeddc8_04fb8a13 : PS2, Line 33: PCI_DEV(0, 0x1f, 0) Could be replaced with `PCH_LPC_DEV` from `southbridge/intel/bd82x6x/pch.h`
https://review.coreboot.org/c/coreboot/+/81368/comment/6f9cb3f5_2f391040 : PS2, Line 34: pci_write_config16(PCI_DEV(0, 0x1f, 0), 0x80, 0x0000); : Could be removed, all zeros is the default
File src/superio/ite/common/early_serial.c:
PS2: I would split the changes to src/superio/ite/common into a separate patch
File src/superio/ite/common/ite.h:
PS2: Same as early_serial.c, split into a separate patch