Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42373 )
Change subject: mainboard/hp: Add ProBook 6360b ......................................................................
Patch Set 5: Code-Review+1
(15 comments)
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:
I used autoport and moved the generated code into a variant. […]
Right. thanks. I'd drop the link from the commit message, though, since it will eventually rot away
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
It's not, I added a comment to reflect this.
Ack
https://review.coreboot.org/c/coreboot/+/42373/5/src/mainboard/hp/snb_ivb_la... File src/mainboard/hp/snb_ivb_laptops/Kconfig:
https://review.coreboot.org/c/coreboot/+/42373/5/src/mainboard/hp/snb_ivb_la... PS5, Line 30: default "probook_6360b" if BOARD_HP_PROBOOK_6360B nit: use alphabetical order?
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 t […]
Done
https://review.coreboot.org/c/coreboot/+/42373/2/src/mainboard/hp/snb_ivb_la... PS2, Line 37: SUPERIO_SMSC_LPC47N217
I think so, I took a picture of the board, the superio is near the docking connector. […]
Oh, it's on the board. Ok, then we can just keep this always on.
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
Yes it does
Then, I'd change this to "y"
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 */
Yes, they are based on the output of lsusb -t.
Ack
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 serial port is on the docking station, which I don't have.
Right, the chip is on the board, but the port isn't useful unless docked. It slows down boot quite a bit, so if you can figure out if we're docked at this point you could make this conditional. Or, disable serial port output in Kconfig
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 :=
The laptop has one DP port and one VGA, the docking station has two DP (and some analog ports). […]
You can physically inspect the mainboard to see if there are any DP switches. There should be some switches for VGA, because there's only one VGA output.
DP++ requires some additional circuitry on the mainboard, AFAIK.
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. […]
Done
https://review.coreboot.org/c/coreboot/+/42373/2/src/mainboard/hp/snb_ivb_la... PS2, Line 20: 0x3b
Autoport detected 1, 3, 4, 5. 0 and 1 have been checked manually. […]
Ok, please update the comment on the previous line
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
Yes, using lspci.
Note that the vendor firmware can swap the PCIe root port functions, you can check for that by looking at their PCI device IDs
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?
Done
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
Yes, I have checked them using radare2 on the EcThermalInit EFI module.
Hrm. Seems to be the same as the other Cougar Point boards. The `ec_fan_ctrl_value` might matter, but I've no idea. There's CB:41159 as well, if you need inspiration 😄
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
They are on the docking station.
Ack