[coreboot-gerrit] [S] Change in coreboot[main]: drivers/intel/pmc_mux/conn: Copy ACPI _PLD property from USB port to mux

[Emilie Roberts (Code Review)]

Attention is currently required from: Paul Menzel, Won Chung.

Emilie Roberts has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81089?usp=email )

Change subject: drivers/intel/pmc_mux/conn: Copy ACPI _PLD property from USB port to mux
......................................................................

Patch Set 7:

(3 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/81089/comment/d0d005b7_478e1716 :
PS5, Line 9: Copy ACPI _PLD values from USB ports to corresponding USB muxes.
> @[e-mail] […]

The general case: to change the USB role, we need to modify `/sys/class/usb_role/CONX-role-switch`. USB ports are enumerated on the system as `/sys/class/portY`. Where X and Y are integers. Currently X and Y have no direct correlation - they are identical on many machines but not all. If you want to modify CONX for portY you need to know something about the system, run some manually plugging/unplugging tests, or examine the ACPI tables.

This change will create a symlink `/sys/class/typec/portY/usb-role-switch` so that userspace/scripts can modify the usb role without knowing ACPI topology for the device.

Specific DbC case: we are enabling ADB over DbC debugging. To use DbC, the usb port role needs to be in host mode. For many devices, X and Y are identical but not for all. We did a method to be able to know which `/sys/class/usb_role/CONX-role-switch` corresponds to which USB port.

https://review.coreboot.org/c/coreboot/+/81089/comment/6a3440a0_c71326b2 :
PS5, Line 12: TEST=emerge-${BOARD} coreboot
> I believe she checked ACPI tables correctly generated on dut.

Yes, I tested this patch on my mithrax (brya) chromebook. I found that `/sys/class/usb_role/CONX-role-switch` did not correspond to `/sys/class/portX` but was pointing to the other port.

See below but I think this patch might be correct but revealing a bad assumption in `intel_pmc_mux.c` where usb ports are simply enumerated sequentially in a loop.

Patchset:

PS5:
> I just checked on mithrax and it appears to be that port0 is on the right side and port1 is on the l […]

When I do that grep on mithrax, for port0, I see `physical_location/panel:right`. For port1 I see `physical_location/panel:left`.

Plugging into the left-hand port indeed creates `/sys/class/typec/port0-partner` and the right-hand port creates `sys/class/typec/port0-partner`.

So it is possible this patch is "correct" and the other typec parsing code is making an sequential assumption. For example `ectool usbpdmuxinfo` shows me `USB=1` for port0 when the left hand port is plugged in, and for port1 when the right hand port is plugged in. Seems like they are swapped.

Possibly the bug is here in the code in `intel_pmc_mux.c` which is doing a simple loop and registering the ports sequentially with `i++`?

```
    for (i = 0; i < pmc->num_ports; i++) {
    	fwnode = device_get_next_child_node(pmc->dev, fwnode);
    	if (!fwnode)
    		break;

ret = pmc_usb_register_port(pmc, i, fwnode);
    	if (ret) {
    		fwnode_handle_put(fwnode);
    		goto err_remove_ports;
    	}

pmc_mux_port_debugfs_init(&pmc->port[i]);
    }
```