[XL] Change in coreboot[main]: mb/protectli/vault_adl_p: Add initial support for VP6630/VP6650/VP6670

[Michał Żygowski (Code Review)]

Attention is currently required from: Felix Singer, Maciej Pijanowski, Nico Huber, Piotr Król.

Michał Żygowski has posted comments on this change by Michał Żygowski. ( https://review.coreboot.org/c/coreboot/+/80501?usp=email )

Change subject: mb/protectli/vault_adl_p: Add initial support for VP6630/VP6650/VP6670
......................................................................

Patch Set 8:

(13 comments)

File src/mainboard/protectli/vault_adl_p/Kconfig:

https://review.coreboot.org/c/coreboot/+/80501/comment/dc5c9f40_04af61bb?usp... :
PS7, Line 38: 	default 0
> Isn't this the global default anyway?

Yes, no idea what it is doing here. Removed

File src/mainboard/protectli/vault_adl_p/bootblock.c:

https://review.coreboot.org/c/coreboot/+/80501/comment/bd6713cc_32ec8958?usp... :
PS7, Line 41: polarity << pin
> I will look into ITE datasheets I have at hand and see how much can be shared. […]

Prepared a patch with a generic ITE SIo GPIO driver: CB:83355

File src/mainboard/protectli/vault_adl_p/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/80501/comment/1be68e07_38f419a9?usp... :
PS7, Line 20: 	device cpu_cluster 0 on end
> Equal to chipset devicetree, please remove

Done

https://review.coreboot.org/c/coreboot/+/80501/comment/f7ae0b9e_2099b687?usp... :
PS7, Line 46: 			register "tcss_ports[0]" = "TCSS_PORT_DEFAULT(OC_SKIP)" # 5G module
            : 			register "tcss_ports[1]" = "TCSS_PORT_DEFAULT(OC_SKIP)" # USB2.0 + USB 3.0 column with RJ45
            : 			register "tcss_ports[2]" = "TCSS_PORT_DEFAULT(OC_SKIP)" # USB Type-C no TBT
            :
>

Done

https://review.coreboot.org/c/coreboot/+/80501/comment/3f7a463e_71abb6fa?usp... :
PS7, Line 93: 			register "usb2_ports[0]" = "USB2_PORT_SHORT(OC_SKIP)" # USB-A 2.0 USB1
            : 			register "usb2_ports[1]" = "USB2_PORT_SHORT(OC_SKIP)" # USB-A 3.0 USB1
            : 			register "usb2_ports[2]" = "USB2_PORT_SHORT(OC_SKIP)" # 5G
            : 			register "usb2_ports[3]" = "USB2_PORT_SHORT(OC_SKIP)" # USB-A 3.0 internal
            : 			register "usb2_ports[4]" = "USB2_PORT_EMPTY"
            : 			register "usb2_ports[5]" = "USB2_PORT_SHORT(OC_SKIP)" # USB-A 2.0 USB2
            : 			register "usb2_ports[6]" = "USB2_PORT_EMPTY"
            : 			register "usb2_ports[7]" = "USB2_PORT_TYPE_C(OC_SKIP)" # USB Type-C no TBT
            : 			register "usb2_ports[8]" = "USB2_PORT_SHORT(OC_SKIP)" # USB-A 2.0 USB2
            :
> Removes ports 4 and 6 too. […]

Done

https://review.coreboot.org/c/coreboot/+/80501/comment/2a88d5b9_33be2ebd?usp... :
PS7, Line 103: 			register "usb2_ports[10]" = "USB2_PORT_EMPTY"
             : 			register "usb2_ports[11]" = "USB2_PORT_EMPTY"
             : 			register "usb2_ports[12]" = "USB2_PORT_EMPTY"
             : 			register "usb2_ports[13]" = "USB2_PORT_EMPTY"
             : 			register "usb2_ports[14]" = "USB2_PORT_EMPTY"
             : 			register "usb2_ports[15]" = "USB2_PORT_EMPTY"
             : 
             : 			register "usb3_ports[0]" = "USB3_PORT_EMPTY"
             : 			register "usb3_ports[1]" = "USB3_PORT_EMPTY"
             : 			register "usb3_ports[2]" = "USB3_PORT_EMPTY"
             : 			register "usb3_ports[3]" = "USB3_PORT_EMPTY"
             : 			register "usb3_ports[4]" = "USB3_PORT_EMPTY"
             : 			register "usb3_ports[5]" = "USB3_PORT_EMPTY"
             : 			register "usb3_ports[6]" = "USB3_PORT_EMPTY"
             : 			register "usb3_ports[7]" = "USB3_PORT_EMPTY"
             : 			register "usb3_ports[8]" = "USB3_PORT_EMPTY"
             : 			register "usb3_ports[9]" = "USB3_PORT_EMPTY"
> It does, yes. Please remove. […]

Done

https://review.coreboot.org/c/coreboot/+/80501/comment/b13c1b49_2cb06263?usp... :
PS7, Line 193: 		device ref heci1 on end
> Equal to chipset devicetree, please remove.

Done

https://review.coreboot.org/c/coreboot/+/80501/comment/df78af6f_af331a6b?usp... :
PS7, Line 206: PCIE_RP_CLK_SRC_UNUSED
> Given that you enable one clock per port, it doesn't seem unused. Or […]

I have added the appropriate CLK_SRC pins.

https://review.coreboot.org/c/coreboot/+/80501/comment/8daac9dd_23a523cd?usp... :
PS7, Line 317: 			end
> This got me curious. […]

Usually the PC80 TPM was on LPC bus. ESPI is an LPC successor, so I leave the TPM there. We may move it under SPI device, but then the hierarchy in ACPI will cause the TPM to depend on the SPI device 1f.5. Typically the TPM device should live under _SB directly, so I wonder where it should really be placed.

https://review.coreboot.org/c/coreboot/+/80501/comment/b6e90c5f_2397cd8f?usp... :
PS7, Line 320: 		device ref p2sb hidden end
> Equal to chipset devicetree, please remove.

Done

File src/mainboard/protectli/vault_adl_p/gpio.c:

https://review.coreboot.org/c/coreboot/+/80501/comment/b80f7616_dcaca785?usp... :
PS7, Line 16: static const struct pad_config gpio_table[] = {
> Remove comments for pads not connected.

Done

File src/mainboard/protectli/vault_adl_p/mainboard.c:

https://review.coreboot.org/c/coreboot/+/80501/comment/ac1aac6b_cc3fef05?usp... :
PS7, Line 29: }
> Déjà vu. Shouldn't this be shared with `die. […]

Exported it to a header file as static inline function

https://review.coreboot.org/c/coreboot/+/80501/comment/438f898b_f9f617e0?usp... :
PS7, Line 51: 	}
> Another déjà vu. Please share code where possible. For instance, this could […]

Haven't touched it yet. Will update it soon with another patch.