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

[sashiko-bot@kernel.org]

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]?

> +
> +/* 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?

-- 
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