Attention is currently required from: Matt DeVillier, Nicholas Chin.
Joel Linn 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:
(38 comments)
File Documentation/mainboard/hp/pro_3500_series.md:
PS2:
This file needs to be added to `Documentation/mainboard/index. […]
Done
PS2:
Please reflow all lines to 72 columns (though I do see many documentation files that use 80 columns)
Done
https://review.coreboot.org/c/coreboot/+/81368/comment/67592b37_5f07965f : PS2, Line 6:
Remove trailing space
Done
https://review.coreboot.org/c/coreboot/+/81368/comment/b2198f8a_553b4ab2 : 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
Done
https://review.coreboot.org/c/coreboot/+/81368/comment/05007656_fb3d907f : PS2, Line 87: ITE8779e
Probably should be either `IT8779E` or `ITE IT8779E`
Done
File Documentation/mainboard/hp/pro_3500_series_flash.jpg:
PS2:
The red is a bit difficult to read here. […]
Done
File Documentation/mainboard/hp/pro_3500_series_jumper.jpg:
PS2:
Same here, probably could be reduced in size
Done
File src/mainboard/hp/pro_3500_series/Kconfig:
PS2:
Needs an SPDX license identifier
Done
https://review.coreboot.org/c/coreboot/+/81368/comment/957b2a78_51652ed1 : 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
Done
https://review.coreboot.org/c/coreboot/+/81368/comment/30695057_6b3bcc22 : PS2, Line 22: string
Remove to avoid redefining types, as it is already declared in other shared Kconfig files
Done
https://review.coreboot.org/c/coreboot/+/81368/comment/4542ed21_bd02ea02 : PS2, Line 26: string
Remove
Done
https://review.coreboot.org/c/coreboot/+/81368/comment/9868f38f_3ca866bd : PS2, Line 30: string
Remove
Done
https://review.coreboot.org/c/coreboot/+/81368/comment/1fd28bbd_b97f9626 : PS2, Line 34: int
Remove
Done
https://review.coreboot.org/c/coreboot/+/81368/comment/f0ace531_22615316 : PS2, Line 37: # FIXME: check this
An EHCI debugger isn't necessary to determine this value, as all it does is indicate which EHCI cont […]
Done Thanks for explaining!
https://review.coreboot.org/c/coreboot/+/81368/comment/fa86f3df_fc5b6a5e : PS2, Line 38: int
Remove
Done
File src/mainboard/hp/pro_3500_series/Kconfig.name:
PS2:
Needs an SPDX license identifier
Done
File src/mainboard/hp/pro_3500_series/Makefile.mk:
PS2:
Needs an SPDX license identifier
Done
File src/mainboard/hp/pro_3500_series/acpi/ec.asl:
PS2:
Should contain […]
Done
File src/mainboard/hp/pro_3500_series/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/81368/comment/a70f06f1_bbc98be5 : PS2, Line 6: /* FIXME: check this function. */
if this is from autoport and you've verified the temps, can probably remove the comment
Done
File src/mainboard/hp/pro_3500_series/common_defines.h:
https://review.coreboot.org/c/coreboot/+/81368/comment/1a8d884e_b151a5de : PS2, Line 8: #define IT8772F_BASE 0x2e : #define EC_DEV PNP_DEV(IT8772F_BASE, IT8772F_EC) : #define GPIO_DEV PNP_DEV(IT8772F_BASE, IT8772F_GPIO)
tab align for readability
Done
File src/mainboard/hp/pro_3500_series/devicetree.cb:
PS2:
Needs an SPDX license identifier
Done
https://review.coreboot.org/c/coreboot/+/81368/comment/cc11ea4f_cc464dac : PS2, Line 3: register "gpu_cpu_backlight" = "0x00000000" : register "gpu_dp_b_hotplug" = "4" : register "gpu_dp_c_hotplug" = "0" : register "gpu_dp_d_hotplug" = "0" : 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"
these values look very suspect - the fixme comment might be applicable here
Done
https://review.coreboot.org/c/coreboot/+/81368/comment/bcaad051_1849b7db : PS2, Line 22: register "gen2_dec" = "0x00000000" : register "gen3_dec" = "0x00000000" : register "gen4_dec" = "0x00000000"
Can be removed
Done
https://review.coreboot.org/c/coreboot/+/81368/comment/c90975d2_1f46171b : PS2, Line 35: # Management Engine Interface 1
The device ref aliases make some of these comments redundant, so they should be removed
Done
https://review.coreboot.org/c/coreboot/+/81368/comment/aec14809_806e82f6 : PS2, Line 36: subsystemid 0x103c 0x2abf
All of these use the same subsystemid, so you can add the line `subsystemid 0x103c 0x2abf inherit` r […]
Done
https://review.coreboot.org/c/coreboot/+/81368/comment/4253192d_4679ca54 : 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 anyt […]
Done
https://review.coreboot.org/c/coreboot/+/81368/comment/f00f3d78_c5e60dec : PS2, Line 78: #register "gpio_set1" = "0x00" : #register "gpio_set2" = "0xff" : #register "gpio_set3" = "0x00" : #register "gpio_set4" = "0x00" : #register "gpio_set5" = "0x00"
can be removed since commented out
Done
https://review.coreboot.org/c/coreboot/+/81368/comment/be5afd2e_a61c6afc : 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 […]
Done
File src/mainboard/hp/pro_3500_series/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/81368/comment/19b12777_e0f7f7da : PS2, Line 3: /* SPDX-License-Identifier: GPL-2.0-only */
The license identifier should be the first line
Done
File src/mainboard/hp/pro_3500_series/early_init.c:
https://review.coreboot.org/c/coreboot/+/81368/comment/e6f97e26_83cd5a7d : PS2, Line 2:
Nit: Remove extra blank line
Done
https://review.coreboot.org/c/coreboot/+/81368/comment/640d2741_370a52c3 : PS2, Line 18: FIXME: Unknown current: RCBA(0x350c)=0x350c
`struct southbridge_usb_port` is defined in `sb/intel/bd82x6x/pch.h`: […]
Done
https://review.coreboot.org/c/coreboot/+/81368/comment/1bc03ec2_7d1e8b46 : PS2, Line 33: 0x1400
Could be replaced with defines from `southbridge/intel/bd82x6x/pch. […]
Done
https://review.coreboot.org/c/coreboot/+/81368/comment/9052e966_b8f948f2 : PS2, Line 33: PCI_DEV(0, 0x1f, 0)
Could be replaced with `PCH_LPC_DEV` from `southbridge/intel/bd82x6x/pch. […]
Done
https://review.coreboot.org/c/coreboot/+/81368/comment/6963552d_fe2271d7 : PS2, Line 34: pci_write_config16(PCI_DEV(0, 0x1f, 0), 0x80, 0x0000); :
Could be removed, all zeros is the default
Done
https://review.coreboot.org/c/coreboot/+/81368/comment/0c899f5c_eb78e31a : PS2, Line 42: // ite_kill_watchdog(GPIO_DEV);
remove commented line
Done
File src/mainboard/hp/pro_3500_series/mainboard.c:
https://review.coreboot.org/c/coreboot/+/81368/comment/705f0a30_f824b53e : PS2, Line 9: * FIXME: untested. */
tested? handler needed at all?
Well the board boots with libgfxinit but so do others and they keep it as well. I guess if you configure to not use libgfxinit it's required?
File src/superio/ite/common/early_serial.c:
PS2:
Yes will move them to the previous changeset in the chain as it was already suggested there.
Done
File src/superio/ite/common/ite.h:
PS2:
Same as early_serial. […]
Done
-
Joel Linn (Code Review)