Angel Pons 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 5:
(5 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
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. Then you can append these sentences:
Redrivers are not protocol-aware, which makes them relatively simple. However, their effectiveness is limited, and may not work at all in some scenarios.
And then, go on explaining the retimers. This helps one see the differences between a redriver and a retimer, and why using the latter can be necessary.
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 signal. Or am I mistaken?
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 66: * Store (_SB.PCI0.GTXS (power_gpio), Local0) nit: use ASL 2.0 instead of `Store`
https://review.coreboot.org/c/coreboot/+/44918/5/src/drivers/intel/usb4/reti... PS5, Line 118: dev->chip_info
config_of(dev)
Is it a fatal error (i.e. booting can never succeed) to have a NULL config here? I don't think so.
Remember that `config_of` will die if either `dev` or `dev->chip_info` are NULL, which should never be the case but isn't a reason to cause a boot failure here. But do check that neither is NULL here.
Refer to CB:44476 and CB:46046 for a related discussion.