[PATCH] phy: qcom: qmp-combo: Move pipe_clk on/off to common

[PATCH] phy: qcom: qmp-combo: Move pipe_clk on/off to common

[Val Packett]

Keep the USB pipe clock working when the phy is in DP-only mode, because
the dwc controller still needs it for USB 2.0 over the same Type-C port.

Tested with the BenQ RD280UA monitor which has a downstream-facing port
for data passthrough that's manually switchable between USB 2 and 3,
corresponding to 4-lane and 2-lane DP respectively.

Note: the suspend/resume callbacks were already gating the enable/disable
of this clock only on init_count and not usb_init_count!

Signed-off-by: Val Packett <val@packett.cool>
---
o/

Just got my hands on a perfect test device for DP alt mode: a monitor with an
on-demand toggle between 2 and 4 lanes. (Started digging because I thought
I needed 4 lanes to use its full resolution and refresh rate, even though
it turned out to be the dpu adjusted mode clock check rejecting the modes,
patches for which are already posted.)

In [1] Konrad mentioned that "the hardware disagrees" with keeping the USB
PLL always on. I'm not sure what exactly was meant by disagreement there,
and I didn't find any specific code that touches that PLL in the driver,
so I decided to just try it anyway.

Before the changes, 4-lane mode would actually kill the USB 2.0 functionality
on the port, no recovery until reboot.

With this patch, I can switch the monitor between 4-lane and 2-lane modes
(with an unplug-replug cycle..) and the USB 2.0 devices attached through
the monitor keep working! (I verified the number of lanes used via dp_debug).

I'm sure it might not be that simple but from my limited and uninformed
understanding without any internal knowledge, the "sneaky workaround"
might actually be the intended way to do things?

Thanks,
~val

P.S. if I'm actually wrong and this is not acceptable for $reasons, the suspend
and resume callbacks would need to be changed to match the logic of having the
clk on depending on usb_init_count, not just the overall init_count.

[1]: https://lore.kernel.org/all/f21b7d52-4c3f-4e5b-bee7-f8b2945b5b02@oss.qualcomm.com/

---
 drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
index 7b5af30f1d02..c4bbd738eba1 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
@@ -3035,6 +3035,13 @@ static int qmp_combo_com_init(struct qmp_combo *qmp, bool force)
 	if (ret)
 		goto err_assert_reset;
 
+	/* In DP-only mode, the pipe clk is still required for USB2 */
+	ret = clk_prepare_enable(qmp->pipe_clk);
+	if (ret) {
+		dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
+		return ret;
+	}
+
 	qphy_setbits(com, QPHY_V3_DP_COM_POWER_DOWN_CTRL, SW_PWRDN);
 
 	/* override hardware control for reset of qmp phy */
@@ -3103,6 +3110,7 @@ static int qmp_combo_com_exit(struct qmp_combo *qmp, bool force)
 	reset_control_bulk_assert(cfg->num_resets, qmp->resets);
 
 	clk_bulk_disable_unprepare(qmp->num_clks, qmp->clks);
+	clk_disable_unprepare(qmp->pipe_clk);
 
 	regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
 
@@ -3205,12 +3213,6 @@ static int qmp_combo_usb_power_on(struct phy *phy)
 
 	qmp_configure(qmp->dev, serdes, cfg->serdes_tbl, cfg->serdes_tbl_num);
 
-	ret = clk_prepare_enable(qmp->pipe_clk);
-	if (ret) {
-		dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
-		return ret;
-	}
-
 	/* Tx, Rx, and PCS configurations */
 	qmp_configure_lane(qmp->dev, tx, cfg->tx_tbl, cfg->tx_tbl_num, 1);
 	qmp_configure_lane(qmp->dev, tx2, cfg->tx_tbl, cfg->tx_tbl_num, 2);
@@ -3254,8 +3256,6 @@ static int qmp_combo_usb_power_off(struct phy *phy)
 	struct qmp_combo *qmp = phy_get_drvdata(phy);
 	const struct qmp_phy_cfg *cfg = qmp->cfg;
 
-	clk_disable_unprepare(qmp->pipe_clk);
-
 	/* PHY reset */
 	qphy_setbits(qmp->pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
 
-- 
2.51.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH] phy: qcom: qmp-combo: Move pipe_clk on/off to common

[Konrad Dybcio]

On 9/27/25 11:17 AM, Val Packett wrote:
> Keep the USB pipe clock working when the phy is in DP-only mode, because
> the dwc controller still needs it for USB 2.0 over the same Type-C port.
> 
> Tested with the BenQ RD280UA monitor which has a downstream-facing port
> for data passthrough that's manually switchable between USB 2 and 3,
> corresponding to 4-lane and 2-lane DP respectively.
> 
> Note: the suspend/resume callbacks were already gating the enable/disable
> of this clock only on init_count and not usb_init_count!
> 
> Signed-off-by: Val Packett <val@packett.cool>
> ---
> o/
> 
> Just got my hands on a perfect test device for DP alt mode: a monitor with an
> on-demand toggle between 2 and 4 lanes. (Started digging because I thought
> I needed 4 lanes to use its full resolution and refresh rate, even though
> it turned out to be the dpu adjusted mode clock check rejecting the modes,
> patches for which are already posted.)
> 
> In [1] Konrad mentioned that "the hardware disagrees" with keeping the USB
> PLL always on. I'm not sure what exactly was meant by disagreement there,
> and I didn't find any specific code that touches that PLL in the driver,
> so I decided to just try it anyway.

So what I did was playing around with the RESET_OVRD settings, which
dictate what parts of the PHY (and their associated PLLs) are kept online..
but I totally forgot that there is a branch/gate clock in GCC that sits
inbetween!

> Before the changes, 4-lane mode would actually kill the USB 2.0 functionality
> on the port, no recovery until reboot.
> 
> With this patch, I can switch the monitor between 4-lane and 2-lane modes
> (with an unplug-replug cycle..) and the USB 2.0 devices attached through
> the monitor keep working! (I verified the number of lanes used via dp_debug).
> 
> I'm sure it might not be that simple but from my limited and uninformed
> understanding without any internal knowledge, the "sneaky workaround"
> might actually be the intended way to do things?

Normally the clock which you're enabling is sourced from the QMPPHY.
The other option (bar some debug outputs) is for it to be driven by
the 19.2 MHz always-on crystal (instead of $lots_of_mhz from the PHY).

For USB hosts without a USB3 phy connected to them, there's an option
to mux the controller's PIPE clock to be sourced from the UTMI clock
input. In those cases, the UTMI (and therefore PIPE) clock runs at..
well, 19.2 MHz!

(you can actually do that on USB3-phy-connected hosts too, at the cost
of.. USB3, probably)

So I'm not sure how much of that is well thought-out design and how
much is luck, but this ends up working for us anyway, with seemingly
no downsides.

At least that's my understanding of the situation.

> 
> Thanks,
> ~val
> 
> P.S. if I'm actually wrong and this is not acceptable for $reasons, the suspend
> and resume callbacks would need to be changed to match the logic of having the
> clk on depending on usb_init_count, not just the overall init_count.

The suspend logic is broken and unused anyway, but that's a nice catch,
the PIPE clock in question is even conveniently called "usb3_pipe" in DT

> 
> [1]: https://lore.kernel.org/all/f21b7d52-4c3f-4e5b-bee7-f8b2945b5b02@oss.qualcomm.com/
> 
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 7b5af30f1d02..c4bbd738eba1 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> @@ -3035,6 +3035,13 @@ static int qmp_combo_com_init(struct qmp_combo *qmp, bool force)
>  	if (ret)
>  		goto err_assert_reset;
>  
> +	/* In DP-only mode, the pipe clk is still required for USB2 */
> +	ret = clk_prepare_enable(qmp->pipe_clk);
> +	if (ret) {
> +		dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
> +		return ret;
> +	}
> +
>  	qphy_setbits(com, QPHY_V3_DP_COM_POWER_DOWN_CTRL, SW_PWRDN);
>  
>  	/* override hardware control for reset of qmp phy */
> @@ -3103,6 +3110,7 @@ static int qmp_combo_com_exit(struct qmp_combo *qmp, bool force)
>  	reset_control_bulk_assert(cfg->num_resets, qmp->resets);
>  
>  	clk_bulk_disable_unprepare(qmp->num_clks, qmp->clks);
> +	clk_disable_unprepare(qmp->pipe_clk);

Let's disable this one first, to preserve existing behavior (and it
makes sense logically - if the PHY doesn't have its clocks, it can't
really generate one either)

Great job finding this!

Konrad

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH] phy: qcom: qmp-combo: Move pipe_clk on/off to common

[Val Packett]

On 10/6/25 11:44 AM, Konrad Dybcio wrote:
> On 9/27/25 11:17 AM, Val Packett wrote:
>> Keep the USB pipe clock working when the phy is in DP-only mode, because
>> the dwc controller still needs it for USB 2.0 over the same Type-C port.
>> [..]
>>
>> In [1] Konrad mentioned that "the hardware disagrees" with keeping the USB
>> PLL always on. I'm not sure what exactly was meant by disagreement there,
>> and I didn't find any specific code that touches that PLL in the driver,
>> so I decided to just try it anyway.
> So what I did was playing around with the RESET_OVRD settings, which
> dictate what parts of the PHY (and their associated PLLs) are kept online..
> but I totally forgot that there is a branch/gate clock in GCC that sits
> inbetween!
>
>> [..]
>> I'm sure it might not be that simple but from my limited and uninformed
>> understanding without any internal knowledge, the "sneaky workaround"
>> might actually be the intended way to do things?
> Normally the clock which you're enabling is sourced from the QMPPHY.
> The other option (bar some debug outputs) is for it to be driven by
> the 19.2 MHz always-on crystal (instead of $lots_of_mhz from the PHY).
>
> For USB hosts without a USB3 phy connected to them, there's an option
> to mux the controller's PIPE clock to be sourced from the UTMI clock
> input. In those cases, the UTMI (and therefore PIPE) clock runs at..
> well, 19.2 MHz!
>
> (you can actually do that on USB3-phy-connected hosts too, at the cost
> of.. USB3, probably)
>
> So I'm not sure how much of that is well thought-out design and how
> much is luck, but this ends up working for us anyway, with seemingly
> no downsides.
>
> At least that's my understanding of the situation.

I wonder how Windows drivers handle this.

The ability to use the UTMI clock sounds more appropriate for when only 
a legacy USB2 device is plugged in and the entirety of QMPPHY is 
unnecessary and can be shut down to save power.

BTW I'm still seeing USB2 functionality die if I boot with the monitor 
cable *already* plugged in, but that sounds like a very different issue 
(the host controller starting to touch the bus before the PIPE clock is 
up? something something probe order?)

> The suspend logic is broken and unused anyway, but that's a nice catch,
> the PIPE clock in question is even conveniently called "usb3_pipe" in DT

Hmm. Is it unused? Oh, you mean the pm_runtime_forbid(), right.

Do you have any pointers about what exactly is broken there? I've been 
poking at the runtime PM stuff too 
(https://gitlab.com/Linaro/arm64-laptops/linux/-/issues/14 for USB), the 
PHYs are the biggest missing piece there overall..

>> [..]
>> @@ -3103,6 +3110,7 @@ static int qmp_combo_com_exit(struct qmp_combo *qmp, bool force)
>>   	reset_control_bulk_assert(cfg->num_resets, qmp->resets);
>>   
>>   	clk_bulk_disable_unprepare(qmp->num_clks, qmp->clks);
>> +	clk_disable_unprepare(qmp->pipe_clk);
> Let's disable this one first, to preserve existing behavior (and it
> makes sense logically - if the PHY doesn't have its clocks, it can't
> really generate one either)

Sure, nice catch. Will send a V2 with this fixed.

Thanks,
~val


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH] phy: qcom: qmp-combo: Move pipe_clk on/off to common

[Konrad Dybcio]

On 10/6/25 6:13 PM, Val Packett wrote:
> 
> On 10/6/25 11:44 AM, Konrad Dybcio wrote:
>> On 9/27/25 11:17 AM, Val Packett wrote:
>>> Keep the USB pipe clock working when the phy is in DP-only mode, because
>>> the dwc controller still needs it for USB 2.0 over the same Type-C port.
>>> [..]
>>>
>>> In [1] Konrad mentioned that "the hardware disagrees" with keeping the USB
>>> PLL always on. I'm not sure what exactly was meant by disagreement there,
>>> and I didn't find any specific code that touches that PLL in the driver,
>>> so I decided to just try it anyway.
>> So what I did was playing around with the RESET_OVRD settings, which
>> dictate what parts of the PHY (and their associated PLLs) are kept online..
>> but I totally forgot that there is a branch/gate clock in GCC that sits
>> inbetween!
>>
>>> [..]
>>> I'm sure it might not be that simple but from my limited and uninformed
>>> understanding without any internal knowledge, the "sneaky workaround"
>>> might actually be the intended way to do things?
>> Normally the clock which you're enabling is sourced from the QMPPHY.
>> The other option (bar some debug outputs) is for it to be driven by
>> the 19.2 MHz always-on crystal (instead of $lots_of_mhz from the PHY).
>>
>> For USB hosts without a USB3 phy connected to them, there's an option
>> to mux the controller's PIPE clock to be sourced from the UTMI clock
>> input. In those cases, the UTMI (and therefore PIPE) clock runs at..
>> well, 19.2 MHz!
>>
>> (you can actually do that on USB3-phy-connected hosts too, at the cost
>> of.. USB3, probably)
>>
>> So I'm not sure how much of that is well thought-out design and how
>> much is luck, but this ends up working for us anyway, with seemingly
>> no downsides.
>>
>> At least that's my understanding of the situation.
> 
> I wonder how Windows drivers handle this.
> 
> The ability to use the UTMI clock sounds more appropriate for when only a legacy USB2 device is plugged in and the entirety of QMPPHY is unnecessary and can be shut down to save power.

How would you hotplug a USB3 device then?

> BTW I'm still seeing USB2 functionality die if I boot with the monitor cable *already* plugged in, but that sounds like a very different issue (the host controller starting to touch the bus before the PIPE clock is up? something something probe order?)

Unclocked accesses would result in immediate restarts

We've had some coldplug woes in the past due to the ADSP Type-C handling..
does replugging (maybe more than once, maybe in a different orientation)
fix it for you?

>> The suspend logic is broken and unused anyway, but that's a nice catch,
>> the PIPE clock in question is even conveniently called "usb3_pipe" in DT
> 
> Hmm. Is it unused? Oh, you mean the pm_runtime_forbid(), right.
> 
> Do you have any pointers about what exactly is broken there? I've been poking at the runtime PM stuff too (https://gitlab.com/Linaro/arm64-laptops/linux/-/issues/14 for USB), the PHYs are the biggest missing piece there overall..

The PHYs likely sip power compared to other things.. (but of course fixing
this would be welcome as every drop counts)

I am not sure what needs fixing, but there exists a chance that because
of the relationship that we're talking about in this thread, the xhci
suspend could use some love first.. I think there was some work on that,
somewhere?

Konrad

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH] phy: qcom: qmp-combo: Move pipe_clk on/off to common

[Konrad Dybcio]

On 9/27/25 11:17 AM, Val Packett wrote:
> Keep the USB pipe clock working when the phy is in DP-only mode, because
> the dwc controller still needs it for USB 2.0 over the same Type-C port.
> 
> Tested with the BenQ RD280UA monitor which has a downstream-facing port
> for data passthrough that's manually switchable between USB 2 and 3,
> corresponding to 4-lane and 2-lane DP respectively.
> 
> Note: the suspend/resume callbacks were already gating the enable/disable
> of this clock only on init_count and not usb_init_count!
> 
> Signed-off-by: Val Packett <val@packett.cool>
> ---
> o/
> 
> Just got my hands on a perfect test device for DP alt mode: a monitor with an
> on-demand toggle between 2 and 4 lanes. (Started digging because I thought
> I needed 4 lanes to use its full resolution and refresh rate, even though
> it turned out to be the dpu adjusted mode clock check rejecting the modes,
> patches for which are already posted.)
> 
> In [1] Konrad mentioned that "the hardware disagrees" with keeping the USB
> PLL always on. I'm not sure what exactly was meant by disagreement there,
> and I didn't find any specific code that touches that PLL in the driver,
> so I decided to just try it anyway.
> 
> Before the changes, 4-lane mode would actually kill the USB 2.0 functionality
> on the port, no recovery until reboot.
> 
> With this patch, I can switch the monitor between 4-lane and 2-lane modes
> (with an unplug-replug cycle..) and the USB 2.0 devices attached through
> the monitor keep working! (I verified the number of lanes used via dp_debug).
> 
> I'm sure it might not be that simple but from my limited and uninformed
> understanding without any internal knowledge, the "sneaky workaround"
> might actually be the intended way to do things?

+Bjorn, Wesley I wrote a wall of text for you, hopefully it makes sense

I now think I have some more knowledge about the setup.

The PIPE clock is an *output* of the PHY that feeds back into GCC, which
provides a XO-PHY mux, which then has a gate/branch downstream of it.
(that's 100% sure)

That branch (e.g. GCC_USB3_PRIM_PHY_PIPE_CLK) then feeds into the
destination core (PCIe, DWC3..) (that's *almost* 100% sure.. it's
surprisingly difficult to fully confirm)

Both msm-5.10 (and older) and upstream decided to keep that clock within
the PHY device though, and that makes.. some.. sense, because you're
not really supposed to turn it off (at least on recent chips), so much so
as switch its source from PHY to XO. That of course needs to happen when
the PHY is going to be disabled, as otherwise the PIPE clock is absent..

I can't really find information about when that branch is supposed to
be *disabled*, but one can probably assume with reasonable confidence
that if the PHY is offline (which generally only happens if the controller
is offline), that may be disabled as well.


The only remaining question remains then, where does the PIPE clock
come from? It's most likely not derived from the DP PLL.

The PHY has some slow onboard always-on clocks (for USB4 wakeup and
USB3 autonomous mode), but those are waay too slow to be driving PIPE
(which runs at 125 MHz @ SS, 312.5 MHz @ SS+).. so only the USB PLL
remains.

But then, in DP-only mode, SW_USB3PHY_RESET is asserted. The
documentation uses some imprecise wording with regards to what that
entails, but there's not many options left other than it may be
leaving the PLL in tact, while halting other parts of the USB3 logic.
Conversely, usb3phy_(nocsr_)reset is explicitly promised to reset the
USB_PLL.


I think the PLL also happens to be disabled when entering either of
the aforementioned sleep modes, since the table tells me the required
RPMH level is 'retention', which surely isn't enough to feed a big PLL.


The solution of teaching DWC3 how to switch clock sources is a little
involved, since we (to my understanding) have to/should omit programming
the parts that are conditional upon superspeed capabilities, and doing
that at runtime would probably realistically require making it type-C
aware (I don't think we want to go that route plus micro-b/MHL edge cases
*sigh*), or we could count on the suspend/resume work making it magically
click into place (since the dwc3 core does quite some re-programming upon
any sort of sus/res today anyway, it seems). If the latter happens, we may
end up reverting this patch as it may then turn out no longer necessary.

Today, i think this commit resolves this immediate issue and I think it's
the fix we should take (bumping my request of reordering the clock disables
and resubmitting..).

On an interesting note, if we take for granted my hypothesis about the
soft-reset not impacting the USB_PLL, this seems like an oversight in
programming guides, because USB4 transition sequences explicitly mention
the names which refer to resets coming from GCC, while DP/USB mix-and-
matching explicitly suggests using the soft-resets, but that might have
gone unnoticed, since dp-only hasn't been much of a prime use case for
in the past, as most of these chips were targetting mobile.

Konrad

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy