Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42373 )
Change subject: mainboard/hp: Add ProBook 6360b ......................................................................
Patch Set 2:
(17 comments)
There are no schematics, so I can't verify everything. Hence, dumping settings while running the vendor firmware and then using autoport would help with at least the PCH GPIO settings and Azalia verbs.
https://review.coreboot.org/c/coreboot/+/42373/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42373/2//COMMIT_MSG@8 PS2, Line 8: Please mention how you did this. Looks like it was copied from 8460p and changed a bit?
https://review.coreboot.org/c/coreboot/+/42373/2//COMMIT_MSG@27 PS2, Line 27: TPM Does SeaBIOS show any options to configure the TPM?
https://review.coreboot.org/c/coreboot/+/42373/2/src/mainboard/hp/snb_ivb_la... File src/mainboard/hp/snb_ivb_laptops/Kconfig:
https://review.coreboot.org/c/coreboot/+/42373/2/src/mainboard/hp/snb_ivb_la... PS2, Line 65: default 1 if BOARD_HP_6360B Is this tested?
https://review.coreboot.org/c/coreboot/+/42373/2/src/mainboard/hp/snb_ivb_la... File src/mainboard/hp/snb_ivb_laptops/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/42373/2/src/mainboard/hp/snb_ivb_la... PS2, Line 25: BOARD_HP_6360B minor: Since this is a ProBook and not an EliteBook, I would not use the same Kconfig symbol style to avoid confusion. Instead, I'd use `BOARD_HP_PROBOOK_6360B`
https://review.coreboot.org/c/coreboot/+/42373/2/src/mainboard/hp/snb_ivb_la... PS2, Line 37: SUPERIO_SMSC_LPC47N217 Is this true?
https://review.coreboot.org/c/coreboot/+/42373/2/src/mainboard/hp/snb_ivb_la... File src/mainboard/hp/snb_ivb_laptops/variants/6360b/board_info.txt:
https://review.coreboot.org/c/coreboot/+/42373/2/src/mainboard/hp/snb_ivb_la... PS2, Line 6: n Does internal flashing with flashrom work, once coreboot is running?
https://review.coreboot.org/c/coreboot/+/42373/2/src/mainboard/hp/snb_ivb_la... File src/mainboard/hp/snb_ivb_laptops/variants/6360b/early_init.c:
https://review.coreboot.org/c/coreboot/+/42373/2/src/mainboard/hp/snb_ivb_la... PS2, Line 13: { 1, 1, 0 }, /* left front */ Are these comments correct?
https://review.coreboot.org/c/coreboot/+/42373/2/src/mainboard/hp/snb_ivb_la... PS2, Line 31: lpc47n217_enable_serial(SERIAL_DEV, CONFIG_TTYS0_BASE); The board does not seem to have any serial port
https://review.coreboot.org/c/coreboot/+/42373/2/src/mainboard/hp/snb_ivb_la... File src/mainboard/hp/snb_ivb_laptops/variants/6360b/gma-mainboard.ads:
https://review.coreboot.org/c/coreboot/+/42373/2/src/mainboard/hp/snb_ivb_la... PS2, Line 11: ports : constant Port_List := Are all of these video ports accessible?
https://review.coreboot.org/c/coreboot/+/42373/2/src/mainboard/hp/snb_ivb_la... File src/mainboard/hp/snb_ivb_laptops/variants/6360b/gpio.c:
PS2: Where does this come from? Ideally, you would want to use autoport to generate this file
https://review.coreboot.org/c/coreboot/+/42373/2/src/mainboard/hp/snb_ivb_la... File src/mainboard/hp/snb_ivb_laptops/variants/6360b/hda_verb.c:
PS2: Where does this come from?
https://review.coreboot.org/c/coreboot/+/42373/2/src/mainboard/hp/snb_ivb_la... File src/mainboard/hp/snb_ivb_laptops/variants/6360b/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/42373/2/src/mainboard/hp/snb_ivb_la... PS2, Line 7: subsystemid 0x103c 0x161c inherit This subsystem ID is wrong. It is for the 8460P
https://review.coreboot.org/c/coreboot/+/42373/2/src/mainboard/hp/snb_ivb_la... PS2, Line 20: 0x3b This mask enables ports 0, 1, 3, 4, 5
https://review.coreboot.org/c/coreboot/+/42373/2/src/mainboard/hp/snb_ivb_la... PS2, Line 22: device pci 1c.0 on end # PCIe Port #1 Are PCIe ports described correctly?
https://review.coreboot.org/c/coreboot/+/42373/2/src/mainboard/hp/snb_ivb_la... PS2, Line 38: register "ec_data_port" = "0x60" Why are there spaces to indent this?
https://review.coreboot.org/c/coreboot/+/42373/2/src/mainboard/hp/snb_ivb_la... PS2, Line 38: register "ec_data_port" = "0x60" : register "ec_cmd_port" = "0x64" : register "ec_ctrl_reg" = "0xca" : register "ec_fan_ctrl_value" = "0x6e" : device pnp ff.1 off end Did you check the EC settings?
https://review.coreboot.org/c/coreboot/+/42373/2/src/mainboard/hp/snb_ivb_la... PS2, Line 44: chip superio/smsc/lpc47n217 : device pnp 4e.3 on # Parallel : io 0x60 = 0x378 : irq 0x70 = 7 : end : device pnp 4e.4 on # COM1 : io 0x60 = 0x3f8 : irq 0x70 = 4 : end : device pnp 4e.5 off end # COM2 : end I can't see any of these on the board