Re: [PATCH] phy: qcom: qmp-combo: Leave the USB PCS alone in DP-only mode

[Val Packett]

On 10/22/26 1:52 PM, Joonhoe Kim wrote:
> In DP-only mode (DisplayPort pin assignments C and E) all four lanes
> belong to the DisplayPort PHY and the USB PCS stays powered down. The
> USB controller can still exit and re-init its PHY in this state, e.g.
> across a system suspend that loses power. qmp_combo_usb_init() then
> programs the USB serdes, lane and PCS tables over the DisplayPort
> configuration and times out waiting for PHYSTATUS, and
> qmp_combo_usb_exit() resets and powers down a PCS that is not in use.
>
> In DP-only mode only take and drop the common reference, but keep
> counting the USB user, so that qmp_combo_reconfigure_phy() powers the
> USB PCS up when the port leaves DP-only mode.
>
> On a Lenovo Y700 Gen 5 (TB323FU, Kaanapali) with a USB-C monitor
> in pin assignment C, resume from s2idle failed with "phy initialization
> timed-out", the dwc3 resume returned -110 and the following xHCI resume
> hung the system.
Good catch. I don't think I've seen system hangs from this on Hamoa, but 
I've had USB stop working on the ports where 4xDP+USB2 was left 
connected during suspend..
> Fixes: 896277138c13 ("phy: qcom: qmp-combo: register a typec mux to change the QMPPHY_MODE")
> Signed-off-by: Joonhoe Kim <26rote@xxxxxxxxx>
> ---
> Tested on a 7.3-rc4 based tree, where the mux switch itself still drops
> usb_init_count when it enters DP-only mode, so the tested change skipped
> the count as well. This version follows the reworked mux path in
> phy-next and is build-tested only.
>
>   drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 18 +++++++++++-------
>   1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 187f0cd11f6c..6e3406d519d2 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> @@ -4459,10 +4459,12 @@ static int qmp_combo_usb_init(struct phy *phy)
>   	if (ret)
>   		return ret;
>   
> -	ret = qmp_combo_usb_power_on(phy);
> -	if (ret) {
> -		qmp_combo_com_exit(qmp, false);
> -		return ret;
> +	if (qmp->qmpphy_mode != QMPPHY_MODE_DP_ONLY) {
> +		ret = qmp_combo_usb_power_on(phy);
> +		if (ret) {
> +			qmp_combo_com_exit(qmp, false);
> +			return ret;
> +		}
> [..]

The USB4 chunk above (juuuust before the start of this diff's context) 
does a similar thing in a different style: it increments both init_count 
and usb_init_count and early-returns, skipping over qmp_combo_com_init 
as well.

I think both USB4 and DP should be written similarly but should 
qmp_combo_com_init be skipped here? No in both cases or does USB4 skip 
it for a good reason?

(I'm especially wondering about the impact of that on the USB HS PHY 
clock, since it's actually sourced from here without a properly modeled 
dependency right now…)


Thanks,
~val