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

[Mattijs Korpershoek <mkorpershoek@kernel.org>]

On Wed, Jul 22, 2026 at 14:01, sashiko-bot@kernel.org wrote:

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

I have not observed any issues during my testing on a SA8775p board with
SCMI firmware (that's compatible with this driver)

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

I have not observed any issues during my testing on a SA8775p board with
SCMI firmware (that's compatible with this driver)

>
>> +}
> [ ... ]
>> @@ -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?

This is unrelated to the patch I've submitted. Should this be send out
as a seperate fix?

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