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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Incorrect power domain sequencing in `d3_to_d0` and `d0_to_d3` transitions, violating hardware constraints.

Pre-existing issues:
- [High] Unbalanced clock prepare/enable and disable/unprepare, leading to `pipe_clk` enable count leak.
--

--- Patch [2]: [PATCH v3 2/2] phy: qcom: qmp-usb: Add support for SA8255P ---
commit 2b06432a4f84c161e8379370587f792f91bca968
Author: Sriram Dash <quic_sriramd@quicinc.com>
Subject: phy: qcom: qmp-usb: Add support for SA8255P

The Qualcomm automotive SA8255p SoC relies on firmware to configure
platform resources, including clocks, interconnects and TLMM.
This commit adds support for the SA8255p SoC in the QMP USB driver by
using SCMI power protocols to manage platform resources via runtime PM
framework APIs. Two sets of resources are modeled as "core" and "transfer"
power domain suppliers.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
> index c5507168e1354..881f4ce23241e 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,
>  };
>  
> +static int qmp_usb_domain_attach(struct qmp_usb *qmp)
> +{
> +	struct dev_pm_domain_attach_data pd_data = {
> +		.pd_flags       = PD_FLAG_NO_DEV_LINK,
> +		.pd_names       = (const char *[]) { "transfer", "core" },
> +		.num_pd_names   = 2,
> +	};
[ ... ]
> +/* 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]);

[Severity: High]
Does resuming the 'transfer' domain before the 'core' domain cause hardware
bus hangs?

The commit message states that the "core" domain controls the GDSC (power),
while the "transfer" domain controls clocks. It appears this sequence turns
on the clocks before enabling power to the logic.

> +	if (ret)
> +		pm_runtime_put_sync(qmp->pd_list->pd_devs[0]);
> +
> +	return ret;
> +}
> +
> +/* 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, does suspending the 'core' domain before the 'transfer' domain
cause issues?

This sequence disables the GDSC (power) before disabling the clocks, which
might also violate hardware constraints and lead to timeouts or freezes.

> +}
[ ... ]
> @@ -2126,14 +2234,23 @@ static int __maybe_unused qmp_usb_runtime_resume(struct device *dev)
>  		return 0;
>  	}

[Severity: High]
This is a pre-existing issue, but does this early bailout lead to a pipe_clk
enable count leak?

The code right above this addition in qmp_usb_runtime_resume() is:

	if (!qmp->phy->init_count) {
		dev_vdbg(dev, "PHY not initialized, bailing out\n");
		return 0;
	}

During the standard PHY lifecycle, this check appears to cause an unbalanced
clock prepare/enable and disable/unprepare state:

1. phy_init() starts with init_count == 0, so qmp_usb_runtime_resume() skips
   enabling clocks.
2. After phy_init() completes, init_count is 1, so phy_pm_runtime_put() causes
   qmp_usb_runtime_suspend() to disable pipe_clk, triggering a clock framework
   underflow warning since it was never enabled.
3. Conversely, during phy_exit(), init_count is 1, so qmp_usb_runtime_resume()
   enables pipe_clk. Then qmp_usb_exit() decrements init_count to 0.
4. Finally, phy_pm_runtime_put() triggers qmp_usb_runtime_suspend() (which
   has a similar bailout check), leaving pipe_clk permanently enabled and
   preventing the system from hitting deep sleep states.

Can this state tracking be fixed to ensure the clocks are properly matched
during suspend and resume?

>  
> +	ret = qmp_usb_d1_to_d0(qmp);
> +	if (ret < 0) {
> +		dev_err(qmp->dev, "Failed to transition to d0 state\n");
> +		return ret;
> +	}
> +
>  	ret = clk_bulk_prepare_enable(qmp->num_clks, qmp->clks);
> -	if (ret)
> +	if (ret) {
> +		qmp_usb_d0_to_d1(qmp);
>  		return ret;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722-phy-qcom-qmp-usb-8255-v3-0-e789070b48f7@kernel.org?part=2

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