Change in coreboot[master]: drivers/intel/usb4: Update driver to support Retimer firmware upgrade

[John Zhao (Code Review)]

Attention is currently required from: Tim Wawrzynczak, Duncan Laurie.
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52712 )

Change subject: drivers/intel/usb4: Update driver to support Retimer firmware upgrade
......................................................................

Patch Set 4:

(15 comments)

Patchset:

PS3:
> Thanks for your patience John!! […]

Thanks.

File src/drivers/intel/usb4/retimer/chip.h:

https://review.coreboot.org/c/coreboot/+/52712/comment/21caec30_8a2a1691 
PS2, Line 14: uint8_t dfp_num;
> This could be dropped and the code could instead loop through DFP_NUM_MAX and ignore entries that ha […]

Ack

File src/drivers/intel/usb4/retimer/retimer.c:

https://review.coreboot.org/c/coreboot/+/52712/comment/4926628f_57d2e843 
PS3, Line 27: usb4_retimer_iteration_num
> USB4_RETIMER_ITERATION_NUM

Ack

https://review.coreboot.org/c/coreboot/+/52712/comment/7d1a7953_23338980 
PS3, Line 28: usb4_retimer_poll_cycle
> USB4_RETIMER_POLL_CYCLE_MS

Ack

https://review.coreboot.org/c/coreboot/+/52712/comment/6d56b648_ac9480e9 
PS3, Line 29: match
> suggestion: rename this to `expected_return_value` or similar

Sure, "expected_value" is used.

https://review.coreboot.org/c/coreboot/+/52712/comment/f84f217a_f6dee249 
PS3, Line 32: uint8_t
> nit: const

Ack

https://review.coreboot.org/c/coreboot/+/52712/comment/3628ec84_2685c00e 
PS3, Line 38: acpigen_write_return_integer
> this takes a `uint64_t` as an arg, so it will write out as a QWORD of 0xFFFFFFFFFFFFFFFF, will the k […]

Yes, it is agreed kernel will properly interpret the returned value -1.

https://review.coreboot.org/c/coreboot/+/52712/comment/8a713e88_75323917 
PS3, Line 98: match = USB_PD_MUX_NONE;
            : 	usb4_retimer_execute_ec_cmd(port, USB_RETIMER_FW_UPDATE_GET_MUX, match);
            : 
            : 	/*
            : 	 * Suspend PD
            : 	 * Command: USB_RETIMER_FW_UPDATE_SUSPEND_PD
            : 	 * Expect return value: 0
            : 	 */
            : 	match = 0;
            : 	usb4_retimer_execute_ec_cmd(port, USB_RETIMER_FW_UPDATE_SUSPEND_PD, match);
            : 
            : 	/*
            : 	 * Set MUX USB Mode
            : 	 * Command: USB_RETIMER_FW_UPDATE_SUSPEND_PD
            : 	 * Expect return value: USB_PD_MUX_USB_ENABLED
            : 	 */
            : 	match = USB_PD_MUX_USB_ENABLED;
            : 	usb4_retimer_execute_ec_cmd(port, USB_RETIMER_FW_UPDATE_SET_USB, match);
            : 
            : 	/*
            : 	 * Set MUX Safe Mode
            : 	 * Command: USB_RETIMER_FW_UPDATE_SET_SAFE
            : 	 * Expect return value: USB_PD_MUX_SAFE_MODE
            : 	 */
            : 	match = USB_PD_MUX_SAFE_MODE;
            : 	usb4_retimer_execute_ec_cmd(port, USB_RETIMER_FW_UPDATE_SET_SAFE, match);
            : 
            : 	/*
            : 	 * Set MUX TBT Mode
            : 	 * Command: USB_RETIMER_FW_UPDATE_SET_TBT
            : 	 * Expect return value: USB_PD_MUX_USB4_ENABLED or USB_PD_MUX_TBT_COMPAT_ENABLED
            : 	 */
            : 	match = USB_PD_MUX_USB4_ENABLED;
            : 	usb4_retimer_execute_ec_cmd(port, USB_RETIMER_FW_UPDATE_SET_TBT, match);
            : }
> chrome EC can handle these commands to RFWU ?

Yes. Whenever a command set is sent to RFWU, EC will be triggered to process accordingly for those online and offline sequential operations.

https://review.coreboot.org/c/coreboot/+/52712/comment/10c66666_8e38cf1d 
PS3, Line 141: Resuem
> Resume

Ack

https://review.coreboot.org/c/coreboot/+/52712/comment/fd5c7882_1f3aa6bc 
PS3, Line 171: poewr
> power

Ack

https://review.coreboot.org/c/coreboot/+/52712/comment/3b212939_0c7f9265 
PS3, Line 200: snprintf(pwr, sizeof(pwr), "HR.DFP%d%c%s", port, '.', "PWR");
> suggestion:  […]

Ack

https://review.coreboot.org/c/coreboot/+/52712/comment/bc218123_343c2b53 
PS3, Line 230: snprintf(pwr, sizeof(pwr), "HR.DFP%d%c%s", port, '.', "PWR");
> snprintf(pwr, sizeof(pwr), "HR.DFP%1d. […]

Ack

https://review.coreboot.org/c/coreboot/+/52712/comment/8c4c4d59_6f250c43 
PS3, Line 322: LOCAL1_OP
> nit: start with LOCAL0_OP

Ack

https://review.coreboot.org/c/coreboot/+/52712/comment/1f8608a3_5493d7c1 
PS3, Line 353: 0xf
> ACPI_STATUS_DEVICE_ALL_ON

Ack

https://review.coreboot.org/c/coreboot/+/52712/comment/6fcf4cb5_1aa20023 
PS3, Line 363: UPC_TYPE_PROPRIETARY
> why PROPRIETARY?

There seems no specific PLD for usb4 in the latest ACPI 6.4 and the framework acpi_pld_fill_usb does not have address property for usb4. So here just simply refer to UPC_TYPE_PROPRIETARY and late update its features like shape and visibility.