Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44918 )
Change subject: drivers/intel/usb4: Add driver for USB4 retimer device ......................................................................
Patch Set 6:
(13 comments)
https://review.coreboot.org/c/coreboot/+/44918/5/Documentation/drivers/retim... File Documentation/drivers/retimer.md:
https://review.coreboot.org/c/coreboot/+/44918/5/Documentation/drivers/retim... PS5, Line 4: grow
increase
Done
https://review.coreboot.org/c/coreboot/+/44918/5/Documentation/drivers/retim... PS5, Line 13: A redriver is a device that boosts the high-frequency content of a signal in
nit: I'd place the `redriver` section before the `retimer` one. […]
Done
https://review.coreboot.org/c/coreboot/+/44918/5/Documentation/drivers/retim... PS5, Line 35: replacing the GPIO with the appropriate pin and polarity.
In this case, "active high" means the retimer will be reset when the GPIO outputs a logic high signa […]
Sorry, will reword, it's a "power" GPIO, not a reset GPIO, so when it's active, it's switching on power to the retimer.
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... File src/drivers/intel/usb4/retimer/Kconfig:
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... PS5, Line 3: help
depends on HAVE_ACPI_TABLES
Done
https://review.coreboot.org/c/coreboot/+/44918/1/src/drivers/intel/usb4/reti... File src/drivers/intel/usb4/retimer/retimer.c:
https://review.coreboot.org/c/coreboot/+/44918/1/src/drivers/intel/usb4/reti... PS1, Line 38: 0x07 Also this needs to be 0xF, bit 0 only indicates there are further bits set in the field (why it is set up that way though, IDK).
https://review.coreboot.org/c/coreboot/+/44918/1/src/drivers/intel/usb4/reti... PS1, Line 40: 0x01 Likewise this needs to be 0x3
https://review.coreboot.org/c/coreboot/+/44918/1/src/drivers/intel/usb4/reti... PS1, Line 82: /* : * If (Local0 == 0) { : * // Turn power off : * _SB.PCI0.CTXS (power_gpio) : * } : */ : acpigen_write_if_lequal_op_int(LOCAL0_OP, 0); : acpigen_disable_tx_gpio(power_gpio); : acpigen_pop_len(); : : /* : * If (Local0 == 1) { : * // Turn power on : * _SB.PCI0.STXS (power_gpio) : * } : */ : acpigen_write_if_lequal_op_int(LOCAL0_OP, 1); : acpigen_enable_tx_gpio(power_gpio); : acpigen_pop_len();
yes, there's acpigen_write_else(), but the result (Local0) is coming from the GTXS Method, which rea […]
I'll just switch to Else.
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... File src/drivers/intel/usb4/retimer/retimer.c:
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... PS5, Line 7: device/i2c_simple.h
I think left over from when I was under the impression the retimer was attached to smbus based on th […]
Done
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... PS5, Line 65: / Read power gpio into Local0
I don't think it hurts but could probably be skipped.
Done
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... PS5, Line 66: * Store (_SB.PCI0.GTXS (power_gpio), Local0)
nit: use ASL 2. […]
Done
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... PS5, Line 87: Local0
Yeah, that's right.
Done
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... PS5, Line 118: dev->chip_info
Is it a fatal error (i.e. booting can never succeed) to have a NULL config here? I don't think so. […]
I see, thanks for the pointers, Angel. However, you'll never run into this code if there is a valid chip driver (and hence even an empty chip_info, not NULL). IOW, the only way to get NULL is to add a chip driver that doesn't exist, and that should cause sconfig to throw an error during the build. Unless I'm misunderstanding something?
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... PS5, Line 118: struct
const
Done