Re: [PATCH v2 2/2] phy: qcom: qmp-usb: Add support for SA8255P

[Mattijs Korpershoek <mkorpershoek@kernel.org>]

On Thu, Jul 09, 2026 at 15:06, sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Incorrect power domain sequencing during d0/d3 transitions risks NoC errors and bus hangs.
> --
>
> --- Patch [2]: [PATCH v2 2/2] phy: qcom: qmp-usb: Add support for SA8255P ---
> commit c32bbfd817c3f9fca4d2982ea37be36354501cc7
> Author: Sriram Dash <quic_sriramd@quicinc.com>
> Subject: phy: qcom: qmp-usb: Add support for SA8255P
>
> This commit adds support for the SA8255P SoC to the QMP USB PHY driver. 
> It introduces firmware-managed resources via SCMI using power protocols,
> managing state transitions (d0, d1, d3) through the "usb_core" and
> "usb_transfer" power domains.
>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
>> index c5507168e1354..d085c1ad7fb5c 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
>
> [ ... ]
>
>> @@ -1865,6 +1890,77 @@ static const struct qmp_phy_cfg glymur_usb3_uniphy_cfg = {
>>  	.regs			= qmp_v7_usb3phy_regs_layout,
>>  };
>
> [ ... ]
>
>> +/* d3_to_d0 transition by turning on all the suppliers */
>> +static int qmp_usb_d3_to_d0(struct qmp_usb *qmp)
>> +{
>> +	int ret;
>> +
>> +	if (!qmp->pd_list)
>> +		return 0;
>> +
>> +	ret = pm_runtime_resume_and_get(qmp->pd_list->pd_devs[0]);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = pm_runtime_resume_and_get(qmp->pd_list->pd_devs[1]);
>> +	if (ret)
>> +		pm_runtime_put_sync(qmp->pd_list->pd_devs[0]);
>> +
>> +	return ret;
>> +}
>
> [Severity: High]
> Does this sequence power up the domains in the wrong order?
>
> The commit message notes that "usb_core" (which maps to pd_devs[1] based on 
> the qmp_usb_domain_attach order) controls the GDSC, while "usb_transfer" 
> (pd_devs[0]) controls clocks and interconnects.
>
> Enabling clocks before the GDSC foundational power domain is enabled typically
> results in unpowered register access on Qualcomm SoCs, which could trigger a
> Network-on-Chip (NoC) error and bus hang. Should pd_devs[1] be resumed before
> pd_devs[0]?

I did not observe any bus errors during my testing. I tried to
insert/remove the phy driver and also could detect a flash drive using a
couple of more patches on other USB drivers.

So I'll keep it this way for v3.

>
>> +
>> +/* d0_to_d3 transition by turning off all the suppliers */
>> +static void qmp_usb_d0_to_d3(struct qmp_usb *qmp)
>> +{
>> +	if (!qmp->pd_list)
>> +		return;
>> +
>> +	pm_runtime_put_sync(qmp->pd_list->pd_devs[1]);
>> +	pm_runtime_put_sync(qmp->pd_list->pd_devs[0]);
>> +}
>
> [Severity: High]
> Similarly, could this cause a bus hang during the d0 to d3 transition?
>
> Putting "usb_core" (pd_devs[1]) before "usb_transfer" (pd_devs[0]) disables 
> the GDSC before the clocks. Shouldn't the GDSC be disabled last to prevent 
> unpowered hardware clock access?

When running modprobe -r, I did not observe any bus hang on my board.
I'll keep this way for v3.

>
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260709-phy-qcom-qmp-usb-8255-v2-0-3b6c26a45788@kernel.org?part=2

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