Message-ID: <f32bdc06-d76b-44f7-8738-2032669e793c@oss.qualcomm.com>
Date: Tue, 2 Dec 2025 12:07:01 +0530
From: Sriram Dash <sriram.dash@....qualcomm.com>
To: Bjorn Andersson <andersson@...nel.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Wesley Cheng <quic_wcheng@...cinc.com>,
        Thinh Nguyen <Thinh.Nguyen@...opsys.com>, jack.pham@....qualcomm.com,
        faisal.hassan@....qualcomm.com, krishna.kurapati@....qualcomm.com,
        linux-arm-msm@...r.kernel.org, linux-usb@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        Konrad Dybcio <konradybcio@...nel.org>,
        Shazad Hussain <shazad.hussain@....qualcomm.com>
Subject: Re: [PATCH 2/2] usb: dwc3: qcom: Support firmware-managed resource
 states for power management

On 12/2/2025 10:51 AM, Bjorn Andersson wrote:
> On Thu, Nov 27, 2025 at 04:01:45PM +0530, Sriram Dash wrote:
>> Add support for firmware-managed resource states in the
>> Qualcomm DWC3 USB controller driver. On platforms
>> like sa8255p, where controller resources are abstracted
>> and managed collectively by firmware, the driver communicates
>> power management transitions using dedicated resource state
>> levels via dev_pm_opp_set_level().
>>
>> Macros are introduced to represent key lifecycle events:
>> initialization, system and runtime suspend/resume, and exit.
>> The driver sets the appropriate resource state during probe,
>> remove, suspend, and resume operations, enabling bulk ON/OFF
>> transitions of grouped resources according to the
>> controller's operational state.
>>
>> Signed-off-by: Sriram Dash <sriram.dash@....qualcomm.com>
>> Co-developed-by: Shazad Hussain <shazad.hussain@....qualcomm.com>
>> Signed-off-by: Shazad Hussain <shazad.hussain@....qualcomm.com>
>> ---
>>  drivers/usb/dwc3/dwc3-qcom.c | 97 ++++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 88 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
>> index 9ac75547820d..9615ca6cfcae 100644
>> --- a/drivers/usb/dwc3/dwc3-qcom.c
>> +++ b/drivers/usb/dwc3/dwc3-qcom.c
>> @@ -13,6 +13,8 @@
>>  #include <linux/kernel.h>
>>  #include <linux/interconnect.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/pm_domain.h>
>> +#include <linux/pm_opp.h>
>>  #include <linux/phy/phy.h>
>>  #include <linux/usb/of.h>
>>  #include <linux/reset.h>
>> @@ -85,10 +87,48 @@ struct dwc3_qcom {
>>  	struct icc_path		*icc_path_apps;
>>  
>>  	enum usb_role		current_role;
>> +	bool			fw_managed;
>>  };
>>  
>>  #define to_dwc3_qcom(d) container_of((d), struct dwc3_qcom, dwc)
>>  
>> +/*
>> + * QCOM DWC3 USB Controller: Firmware-Managed Resource State Levels
>> + *
>> + * On select Qualcomm platforms, the USB controller’s power-related
>> + * resources including GDSC, reset lines, clocks, and interconnects
>> + * are managed collectively by system firmware via SCMI. The driver
>> + * signals the controller’s operational state to firmware using these
>> + * levels, each mapped to a specific power management transition or
>> + * lifecycle event:
>> + *
>> + * DWC3_QCOM_FW_MANAGED_INIT
> Both power and performance states are typically...states...
> But these are actions/transitions between states.
>
>
> The purpose of doing firmware assisted resource management (like done in
> ACPI) is that it abstracts away the power management aspects from the OS
> implementation, here we instead seems to complicate the OS
> implementation.
>
>> + *	Enable GDSC, Assert and Deassert Resets, and turn ON all clocks
>> + *	and interconnects.
>> + *
>> + * DWC3_QCOM_FW_MANAGED_SYSTEM_RESUME
>> + *	Enable GDSC and turn ON all clocks and interconnects.
>> + *
>> + * DWC3_QCOM_FW_MANAGED_RUNTIME_RESUME
>> + *	Turn ON all clocks and interconnects.
>> + *
>> + * DWC3_QCOM_FW_MANAGED_EXIT
>> + *	Turn OFF all clocks and interconnects, Assert reset and disable GDSC.
>> + *
>> + * DWC3_QCOM_FW_MANAGED_SYSTEM_SUSPEND
>> + *	Turn OFF all clocks and interconnects and disable GDSC.
>> + *
>> + * DWC3_QCOM_FW_MANAGED_RUNTIME_SUSPEND
>> + *	Turn OFF clocks and interconnects.
>> + */
>> +
>> +#define DWC3_QCOM_FW_MANAGED_INIT			1
>> +#define DWC3_QCOM_FW_MANAGED_SYSTEM_RESUME		2
>> +#define DWC3_QCOM_FW_MANAGED_RUNTIME_RESUME		3
> Given that dwc3_core_probe() calls pm_runtime_forbid(), do we actually
> hit these states, or are you in practice only hitting some "D0" and "D3"
> states?
>
> Could this be simplified to match what we would need here for an ACPI
> system?


Hi Bjorn,

thanks for the comments.

You’re right that the wording in the comment makes these look like
explicit “do X/Y/Z now” transitions rather than passive states. The
intention is not to expose an imperative sequence to firmware, but to
advertise a small set of abstract “resource configurations” that
correspond to specific OS power‑management contexts in the driver.

On sa8255p, the USB controller and its associated resources (GDSC,
clocks, interconnects, resets) are grouped behind a single
firmware‑managed perf domain. From the driver’s perspective we only have
a few meaningful configurations:

initial bring‑up during probe,
system suspend / system resume,
runtime suspend / runtime resume (planned once runtime PM is enabled), and
final shutdown on remove.

The levels are meant to encode which phase of the lifecycle we are in,
so that firmware can choose an internal representation that matches its
own notion of “D0‑like”, “temporarily suspended” or “off / removed”,
including any differences in how aggressively it can drop power, assert
resets, or preserve context.

You are correct that INIT and the various RESUME levels are all “on” in
the sense that the controller ends up operational, and similarly EXIT /
SUSPEND variants are “off / not actively used”. Today the driver does
not try to model these as strict D0/D3/D3hot/D3cold equivalents, because:

INIT may require a more complete bring‑up after boot, where firmware
might need to perform extra initialization compared to a resume from a
prior suspended state.
SYSTEM_* vs RUNTIME_* are tied to the OS‑level PM entry points
(dwc3_qcom_suspend() is used for both system and runtime suspend;
runtime is currently forbidden but it is planned later). The distinction
gives firmware the option to use different policies for system sleep vs
runtime idle, including wake‑capability and context‑retention.
That said, I agree that the current comment over‑specifies the concrete
actions (“Enable GDSC, Assert and Deassert Resets…”) and makes the
interface look more complicated than it actually is.

We can reword it to describe the effective resource state, without
prescribing exactly how the firmware should sequence GDSC, resets and
clocks. However, I’d still like to keep the separation between system
and runtime paths so that we don’t have to extend the protocol again
when runtime PM is enabled.

/*
 * QCOM DWC3 USB Controller: Firmware-Managed Resource State Levels
 *
 * On select Qualcomm platforms, the USB controller’s power-related
 * resources (such as GDSC, reset lines, clocks, and interconnects)
 * are managed collectively by system firmware. The driver reports
 * the controller’s lifecycle and power-management context using the
 * following abstract resource state levels. The exact sequencing and
 * choice of underlying resources for each level is left to firmware.
 *
 * DWC3_QCOM_FW_MANAGED_INIT
 *    Controller is initialized after probe and brought into a fully
 *    operational state suitable for normal use.
 *
 * DWC3_QCOM_FW_MANAGED_SYSTEM_RESUME
 *    Controller returns from system suspend to a fully operational
 *    state suitable for normal use.
 *
 * DWC3_QCOM_FW_MANAGED_RUNTIME_RESUME
 *    Controller returns from runtime suspend to an operational state
 *    sufficient for runtime activity.
 *
 * DWC3_QCOM_FW_MANAGED_EXIT
 *    Controller is shut down as part of driver removal and may be put
 *    into a fully powered-off state with no requirement for retention.
 *
 * DWC3_QCOM_FW_MANAGED_SYSTEM_SUSPEND
 *    Controller is quiesced for system suspend; resources may be
 *    reduced or powered down according to platform policy.
 *
 * DWC3_QCOM_FW_MANAGED_RUNTIME_SUSPEND
 *    Controller is quiesced for runtime suspend; a lower-power state
 *    is entered while allowing a later runtime resume.
 */
#define DWC3_QCOM_FW_MANAGED_INIT            1
#define DWC3_QCOM_FW_MANAGED_SYSTEM_RESUME        2
#define DWC3_QCOM_FW_MANAGED_RUNTIME_RESUME        3
#define DWC3_QCOM_FW_MANAGED_EXIT            8
#define DWC3_QCOM_FW_MANAGED_SYSTEM_SUSPEND        9
#define DWC3_QCOM_FW_MANAGED_RUNTIME_SUSPEND        10


Let me know if this is OK.



> Regards,
> Bjorn
>
>> +#define DWC3_QCOM_FW_MANAGED_EXIT			8
>> +#define DWC3_QCOM_FW_MANAGED_SYSTEM_SUSPEND		9
>> +#define DWC3_QCOM_FW_MANAGED_RUNTIME_SUSPEND		10
>> +
>>  static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
>>  {
>>  	u32 reg;
>> @@ -335,7 +375,7 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
>>  		dwc3_qcom_enable_port_interrupts(&qcom->ports[i]);
>>  }
>>  
>> -static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
>> +static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup, pm_message_t msg)
>>  {
>>  	u32 val;
>>  	int i, ret;
>> @@ -348,6 +388,13 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
>>  		if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
>>  			dev_err(qcom->dev, "port-%d HS-PHY not in L2\n", i + 1);
>>  	}
>> +	if (qcom->fw_managed) {
>> +		if (PMSG_IS_AUTO(msg))
>> +			dev_pm_opp_set_level(qcom->dev, DWC3_QCOM_FW_MANAGED_RUNTIME_SUSPEND);
>> +		else
>> +			dev_pm_opp_set_level(qcom->dev, DWC3_QCOM_FW_MANAGED_SYSTEM_SUSPEND);
>> +	}
>> +
>>  	clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
>>  
>>  	ret = dwc3_qcom_interconnect_disable(qcom);
>> @@ -369,7 +416,7 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
>>  	return 0;
>>  }
>>  
>> -static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
>> +static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup, pm_message_t msg)
>>  {
>>  	int ret;
>>  	int i;
>> @@ -380,6 +427,18 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
>>  	if (dwc3_qcom_is_host(qcom) && wakeup)
>>  		dwc3_qcom_disable_interrupts(qcom);
>>  
>> +	if (qcom->fw_managed) {
>> +		if (PMSG_IS_AUTO(msg))
>> +			ret = dev_pm_opp_set_level(qcom->dev, DWC3_QCOM_FW_MANAGED_RUNTIME_RESUME);
>> +		else
>> +			ret = dev_pm_opp_set_level(qcom->dev, DWC3_QCOM_FW_MANAGED_SYSTEM_RESUME);
>> +
>> +		if (ret < 0) {
>> +			dev_err(qcom->dev, "Failed to Resume fw managed device\n");
>> +			return ret;
>> +		}
>> +	}
>> +
>>  	ret = clk_bulk_prepare_enable(qcom->num_clocks, qcom->clks);
>>  	if (ret < 0)
>>  		return ret;
>> @@ -624,10 +683,18 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>>  
>>  	qcom->dev = &pdev->dev;
>>  
>> +	qcom->fw_managed = device_get_match_data(dev);
>> +	if (qcom->fw_managed) {
>> +		ret = dev_pm_opp_set_level(qcom->dev, DWC3_QCOM_FW_MANAGED_INIT);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +
>>  	qcom->resets = devm_reset_control_array_get_optional_exclusive(dev);
>>  	if (IS_ERR(qcom->resets)) {
>> -		return dev_err_probe(&pdev->dev, PTR_ERR(qcom->resets),
>> -				     "failed to get resets\n");
>> +		dev_err_probe(&pdev->dev, PTR_ERR(qcom->resets),
>> +			      "failed to get resets\n");
>> +		goto resources_off;
>>  	}
>>  
>>  	ret = devm_clk_bulk_get_all(&pdev->dev, &qcom->clks);
>> @@ -638,7 +705,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>>  	ret = reset_control_assert(qcom->resets);
>>  	if (ret) {
>>  		dev_err(&pdev->dev, "failed to assert resets, err=%d\n", ret);
>> -		return ret;
>> +		goto resources_off;
>>  	}
>>  
>>  	usleep_range(10, 1000);
>> @@ -727,6 +794,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>>  clk_disable:
>>  	clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
>>  
>> +resources_off:
>> +	if (qcom->fw_managed)
>> +		dev_pm_opp_set_level(qcom->dev, DWC3_QCOM_FW_MANAGED_EXIT);
>> +
>>  	return ret;
>>  }
>>  
>> @@ -739,6 +810,10 @@ static void dwc3_qcom_remove(struct platform_device *pdev)
>>  		return;
>>  
>>  	dwc3_core_remove(&qcom->dwc);
>> +
>> +	if (qcom->fw_managed)
>> +		dev_pm_opp_set_level(qcom->dev, DWC3_QCOM_FW_MANAGED_EXIT);
>> +
>>  	clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
>>  	dwc3_qcom_interconnect_exit(qcom);
>>  
>> @@ -756,7 +831,7 @@ static int dwc3_qcom_pm_suspend(struct device *dev)
>>  	if (ret)
>>  		return ret;
>>  
>> -	ret = dwc3_qcom_suspend(qcom, wakeup);
>> +	ret = dwc3_qcom_suspend(qcom, wakeup, PMSG_SUSPEND);
>>  	if (ret)
>>  		return ret;
>>  
>> @@ -772,7 +847,7 @@ static int dwc3_qcom_pm_resume(struct device *dev)
>>  	bool wakeup = device_may_wakeup(dev);
>>  	int ret;
>>  
>> -	ret = dwc3_qcom_resume(qcom, wakeup);
>> +	ret = dwc3_qcom_resume(qcom, wakeup, PMSG_RESUME);
>>  	if (ret)
>>  		return ret;
>>  
>> @@ -809,7 +884,7 @@ static int dwc3_qcom_runtime_suspend(struct device *dev)
>>  	if (ret)
>>  		return ret;
>>  
>> -	return dwc3_qcom_suspend(qcom, true);
>> +	return dwc3_qcom_suspend(qcom, true, PMSG_AUTO_SUSPEND);
>>  }
>>  
>>  static int dwc3_qcom_runtime_resume(struct device *dev)
>> @@ -818,7 +893,7 @@ static int dwc3_qcom_runtime_resume(struct device *dev)
>>  	struct dwc3_qcom *qcom = to_dwc3_qcom(dwc);
>>  	int ret;
>>  
>> -	ret = dwc3_qcom_resume(qcom, true);
>> +	ret = dwc3_qcom_resume(qcom, true, PMSG_AUTO_RESUME);
>>  	if (ret)
>>  		return ret;
>>  
>> @@ -839,6 +914,10 @@ static const struct dev_pm_ops dwc3_qcom_dev_pm_ops = {
>>  };
>>  
>>  static const struct of_device_id dwc3_qcom_of_match[] = {
>> +	{
>> +		.compatible	= "qcom,snps-dwc3-fw-managed",
>> +		.data		= (void *)true,
>> +	},
>>  	{ .compatible = "qcom,snps-dwc3" },
>>  	{ }
>>  };
>>
>> -- 
>> 2.34.1
>>

Powered by blists - more mailing lists