Re: [PATCH v4 6/7] usb: dwc3: qcom: Support firmware-managed resource states for power management

[Faisal Hassan <faisal.hassan@oss.qualcomm.com>]

On 02-11-2026 11:50 am, Peter Chen wrote:
> On 26-10-29 00:29:36, Faisal Hassan wrote:
>> Add support for firmware-managed resource handling in the Qualcomm
>> DWC3 USB controller driver. On platforms such as SA8255P, controller
>> resources are abstracted and managed collectively by firmware, and are
>> exposed to Linux through SCMI-backed power domains.
>>
>> On SA8255P, resources such as clocks, regulators, resets, TLMM and
>> interconnects are controlled by firmware. The driver communicates with
>> firmware through two power domains:
>>
>>    - "power": controls power supply (GDSC) to the USB controller
>>    - "bus": controls clocks and interconnects used for data transfer.
>>      Register accesses through the controller require the "power"
>>      domain to be enabled.
>>
>> The two domains are voted on and off directly from the PM callbacks
>> through dwc3_qcom_domains_get() and dwc3_qcom_domains_put(). Runtime
>> PM usage counting keeps each domain enabled for as long as it is
>> required; the driver does not attempt to track or infer any aggregate
>> device power state, nor does it assume what state firmware or the
>> bootloader left the domains in before probe.
>>
>> Runtime suspend/resume vote only the "bus" domain, since the "power"
>> domain must remain enabled for hardware accesses that can occur while
>> runtime suspended. System suspend/resume vote both domains.
>>
>> If system suspend is entered while the device is already runtime
>> suspended, only the "power" vote is still held, so system suspend
>> drops just that one. System resume then votes both domains back on,
>> which keeps the votes balanced across the cycle.
>>
>> The domains are attached with PD_FLAG_NO_DEV_LINK because runtime
>> suspend and system suspend require different supplier states. Runtime
>> suspend releases only the "bus" domain while retaining the "power"
>> domain, whereas system suspend releases both domains. Ordinary
>> device-links would couple both suppliers to the same consumer runtime
>> PM state and therefore cannot express the required policy. The driver
>> consequently manages the runtime PM references for the attached domains
>> explicitly.
>>
>> These votes are taken during probe, remove, suspend and resume,
>> enabling coordinated management of grouped resources according to the
>> controller's operational requirements.
>>
>> Enable firmware-managed resource handling for the
>> "qcom,sa8255p-dwc3" compatible while maintaining backward
>> compatibility with existing platforms that use direct resource
>> control.
>>
>> pm_runtime_put_sync() drops the domain usage count even when it
>> returns an error, so failures to vote a domain off are only reported.
>> Failing suspend on them would leave the driver thinking the votes are
>> still held and unbalance the next resume/suspend pair.
>>
>> While here, assert the controller reset in dwc3_qcom_remove() and on
>> the probe error-unwind path once the controller is powered down. These
>> paths are shared by every qcom,snps-dwc3 platform, not only the
>> firmware-managed ones added here. That is safe for existing platforms,
>> because remove() already fully powers the controller down and a probe
>> failure already leaves it unclocked.

Hi Peter,

Thanks for the review.
> 
> Since you do controller reset for all platforms, would you please split this
> patch for:
> 
> - Add controller reset for all qcom platforms
> - Add firmware-based resource management for auto SoCs
> 

Ok, I will move the reset on remove() and on the probe error path into 
its own patch, ahead of the firmware-managed one.

>>   
>> +/*
>> + * Firmware-managed resource handling
>> + *
>> + * On platforms where clocks, interconnects, resets and the GDSC for the USB
>> + * controller are owned and sequenced by firmware, access to those resources
>> + * is exposed to Linux as two power domains:
>> + * - pd_list->pd_devs[0] ("power"): the GDSC supplying the controller
>> + * - pd_list->pd_devs[1] ("bus"): the clocks and interconnects used for data
>> + *   transfer, which additionally require "power" to be on for register
>> + *   accesses to succeed
>> + *
>> + * The two domains are voted on/off directly from each PM callback that needs
>> + * them; there's no attempt to track or name an aggregate device power state,
>> + * since the actual state lives in firmware and is queried by voting through
>> + * runtime PM, not by mirroring it in the driver.
>> + */
>> +
>> +/**
>> + * dwc3_qcom_domain_detach() - Detach power domains
>> + * @qcom: Pointer to the dwc3_qcom structure
>> + *
>> + * Detaches all power domains.
>> + */
>> +static void dwc3_qcom_domain_detach(struct dwc3_qcom *qcom)
>> +{
>> +	if (qcom->pd_list)
>> +		dev_pm_domain_detach_list(qcom->pd_list);
>> +}
>> +
>> +/**
>> + * dwc3_qcom_domain_attach() - Attach power domains
>> + * @qcom: Pointer to the dwc3_qcom structure
>> + *
>> + * Attaches power domains for firmware managed resource handling.
>> + * Returns 0 on success, negative error code on failure.
>> + */
>> +static int dwc3_qcom_domain_attach(struct dwc3_qcom *qcom)
>> +{
>> +	struct dev_pm_domain_attach_data pd_data = {
>> +		.pd_flags	= PD_FLAG_NO_DEV_LINK,
>> +		.pd_names	= (const char*[]) { "power", "bus" },
>> +		.num_pd_names	= 2,
>> +	};
>> +	struct device *dev = qcom->dev;
>> +	int ret;
>> +
>> +	ret = dev_pm_domain_attach_list(dev, &pd_data, &qcom->pd_list);
> 
> Using devm_pm_domain_attach_list, the dwc3_qcom_domain_detach is not needed
> any more.
> 

Ok, I will switch to devm_pm_domain_attach_list() and drop 
dwc3_qcom_domain_detach().

>> +	if (ret != pd_data.num_pd_names) {
>> +		dev_err(dev, "domain attach failed (%d)\n", ret);
>> +		return ret < 0 ? ret : -ENODEV;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * dwc3_qcom_domains_get() - Vote the firmware-managed domains on
>> + * @qcom: Pointer to the dwc3_qcom structure
>> + * @bus_only: If true, leave the power domain untouched and only vote bus
>> + *
>> + * Votes runtime PM "on" for the domains this call is responsible for. Each
>> + * call has exactly one matching dwc3_qcom_domains_put() call with the same
>> + * @bus_only value on the corresponding disable path; runtime PM's own usage
>> + * counting is what keeps power on for as long as bus needs it, without the
>> + * driver tracking or assuming any prior domain state itself.
>> + * Returns 0 on success, negative error code on failure.
>> + */
>> +static int dwc3_qcom_domains_get(struct dwc3_qcom *qcom, bool bus_only)
>> +{
>> +	struct device *power_dev = qcom->pd_list->pd_devs[0];
>> +	struct device *bus_dev = qcom->pd_list->pd_devs[1];
>> +	int ret;
>> +
>> +	if (!bus_only) {
>> +		ret = pm_runtime_resume_and_get(power_dev);
>> +		if (ret) {
>> +			dev_err(qcom->dev, "failed to enable power domain: %d\n", ret);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	ret = pm_runtime_resume_and_get(bus_dev);
>> +	if (ret) {
>> +		dev_err(qcom->dev, "failed to enable bus domain: %d\n", ret);
>> +		if (!bus_only)
>> +			pm_runtime_put_sync(power_dev);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * dwc3_qcom_domains_put() - Vote the firmware-managed domains off
>> + * @qcom: Pointer to the dwc3_qcom structure
>> + * @bus_only: If true, leave the power domain untouched and only vote bus
>> + *
>> + * Releases the votes taken by the matching dwc3_qcom_domains_get() call.
>> + * Bus is released before power since bus register accesses require power
>> + * to still be on.
>> + *
>> + * pm_runtime_put_sync() drops the usage count even when it returns an error,
>> + * so the votes are released regardless and errors are only reported. Treating
>> + * them as failures would make callers believe the votes are still held and
>> + * unbalance the next dwc3_qcom_domains_get()/put() pair.
>> + */
>> +static void dwc3_qcom_domains_put(struct dwc3_qcom *qcom, bool bus_only)
>> +{
>> +	struct device *power_dev = qcom->pd_list->pd_devs[0];
>> +	struct device *bus_dev = qcom->pd_list->pd_devs[1];
>> +	int ret;
>> +
>> +	ret = pm_runtime_put_sync(bus_dev);
>> +	if (ret < 0)
>> +		dev_warn(qcom->dev, "failed to disable bus domain: %d\n", ret);
>> +
>> +	if (bus_only)
>> +		return;
>> +
>> +	ret = pm_runtime_put_sync(power_dev);
>> +	if (ret < 0)
>> +		dev_warn(qcom->dev, "failed to disable power domain: %d\n", ret);
>> +}
>> +
>>   static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
>>   {
>>   	u32 reg;
>> @@ -380,8 +510,20 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, pm_message_t msg)
>>   	int i, ret;
>>   	bool wakeup;
>>   
>> -	if (qcom->is_suspended)
>> +	if (qcom->is_suspended) {
>> +		/*
>> +		 * Runtime suspend dropped only the bus domain. System suspend
>> +		 * must drop the power domain too, so that system resume, which
>> +		 * votes both domains back on, stays balanced. The usage count
>> +		 * is dropped even if this fails, so don't fail the suspend.
>> +		 */
>> +		if (qcom->fw_managed && !PMSG_IS_AUTO(msg)) {
>> +			ret = pm_runtime_put_sync(qcom->pd_list->pd_devs[0]);
>> +			if (ret < 0)
>> +				dev_warn(qcom->dev, "failed to disable power domain: %d\n", ret);
>> +		}
>>   		return 0;
>> +	}
>>   
>>   	/*
>>   	 * For runtime suspend, always enable wakeup.
>> @@ -397,11 +539,17 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, pm_message_t msg)
>>   		if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
>>   			dev_err(qcom->dev, "port-%d HS-PHY not in L2\n", i + 1);
>>   	}
>> -	clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
>>   
>> -	ret = dwc3_qcom_interconnect_disable(qcom);
>> -	if (ret)
>> -		dev_warn(qcom->dev, "failed to disable interconnect: %d\n", ret);
>> +	if (!qcom->fw_managed) {
>> +		clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
>> +
>> +		ret = dwc3_qcom_interconnect_disable(qcom);
>> +		if (ret)
>> +			dev_warn(qcom->dev, "failed to disable interconnect: %d\n", ret);
>> +	} else {
>> +		/* Runtime suspend keeps power voted on; system suspend drops both. */
>> +		dwc3_qcom_domains_put(qcom, PMSG_IS_AUTO(msg));
>> +	}
>>   
>>   	/*
>>   	 * The role is stable during suspend as role switching is done from a
>> @@ -456,13 +604,22 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, pm_message_t msg)
>>   	 */
>>   	dwc3_qcom_disable_interrupts(qcom);
>>   
>> -	ret = clk_bulk_prepare_enable(qcom->num_clocks, qcom->clks);
>> -	if (ret < 0)
>> -		return ret;
>> +	if (!qcom->fw_managed) {
>> +		ret = clk_bulk_prepare_enable(qcom->num_clocks, qcom->clks);
>> +		if (ret < 0)
>> +			return ret;
>>   
>> -	ret = dwc3_qcom_interconnect_enable(qcom);
>> -	if (ret)
>> -		dev_warn(qcom->dev, "failed to enable interconnect: %d\n", ret);
>> +		ret = dwc3_qcom_interconnect_enable(qcom);
>> +		if (ret)
>> +			dev_warn(qcom->dev, "failed to enable interconnect: %d\n", ret);
>> +	} else {
>> +		/* Runtime resume re-votes bus only; system resume votes both. */
>> +		ret = dwc3_qcom_domains_get(qcom, PMSG_IS_AUTO(msg));
>> +		if (ret) {
>> +			dev_err(qcom->dev, "failed to enable power domains: %d\n", ret);
>> +			return ret;
>> +		}
>> +	}
>>   
>>   	/* Clear existing events from PHY related to L2 in/out */
>>   	for (i = 0; i < qcom->num_ports; i++) {
>> @@ -699,16 +856,21 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>>   	if (priv_data)
>>   		qcom->broken_suspend = priv_data->broken_suspend;
>>   
>> +	if (priv_data && priv_data->fw_managed)
>> +		qcom->fw_managed = priv_data->fw_managed;
>> +
>>   	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");
>>   	}
>>   
>> -	ret = devm_clk_bulk_get_all(&pdev->dev, &qcom->clks);
>> -	if (ret < 0)
>> -		return dev_err_probe(dev, ret, "failed to get clocks\n");
>> -	qcom->num_clocks = ret;
>> +	if (!qcom->fw_managed) {
>> +		ret = devm_clk_bulk_get_all(&pdev->dev, &qcom->clks);
>> +		if (ret < 0)
>> +			return dev_err_probe(dev, ret, "failed to get clocks\n");
>> +		qcom->num_clocks = ret;
>> +	}
>>   
>>   	ret = reset_control_assert(qcom->resets);
>>   	if (ret) {
>> @@ -724,9 +886,33 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>>   		return ret;
>>   	}
>>   
>> -	ret = clk_bulk_prepare_enable(qcom->num_clocks, qcom->clks);
>> -	if (ret < 0)
>> -		return ret;
>> +	if (!qcom->fw_managed) {
>> +		ret = clk_bulk_prepare_enable(qcom->num_clocks, qcom->clks);
>> +		if (ret < 0) {
>> +			reset_control_assert(qcom->resets);
>> +			return ret;
>> +		}
>> +	} else {
>> +		ret = dwc3_qcom_domain_attach(qcom);
>> +		if (ret) {
>> +			dev_err(dev, "Failed to attach domains (%d).\n", ret);
>> +			reset_control_assert(qcom->resets);
>> +			return ret;
>> +		}
>> +
>> +		/*
>> +		 * pm_runtime_resume_and_get() is correct here regardless of
>> +		 * whatever state firmware or the bootloader left the domains
>> +		 * in beforehand.
>> +		 */
>> +		ret = dwc3_qcom_domains_get(qcom, false /* bus_only */);
>> +		if (ret) {
>> +			dev_err(dev, "Failed to enable power domains (%d)\n", ret);
>> +			dwc3_qcom_domain_detach(qcom);
>> +			reset_control_assert(qcom->resets);
>> +			return ret;
>> +		}
>> +	}
>>   
>>   	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>   	if (!r) {
>> @@ -790,9 +976,11 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>>   		goto clk_disable;
>>   	}
>>   
>> -	ret = dwc3_qcom_interconnect_init(qcom);
>> -	if (ret)
>> -		goto remove_core;
>> +	if (!qcom->fw_managed) {
>> +		ret = dwc3_qcom_interconnect_init(qcom);
>> +		if (ret)
>> +			goto remove_core;
>> +	}
>>   
>>   	wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source");
>>   	device_init_wakeup(&pdev->dev, wakeup_source);
>> @@ -804,7 +992,14 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>>   remove_core:
>>   	dwc3_core_remove(&qcom->dwc);
>>   clk_disable:
>> -	clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
>> +	if (!qcom->fw_managed) {
>> +		clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
>> +	} else {
>> +		dwc3_qcom_domains_put(qcom, false /* bus_only */);
>> +		dwc3_qcom_domain_detach(qcom);
>> +	}
>> +
>> +	reset_control_assert(qcom->resets);
>>   
>>   	return ret;
>>   }
>> @@ -817,9 +1012,18 @@ static void dwc3_qcom_remove(struct platform_device *pdev)
>>   	if (pm_runtime_resume_and_get(qcom->dev) < 0)
>>   		return;
>>   
>> +	/* Also disables runtime PM and marks the device suspended. */
>>   	dwc3_core_remove(&qcom->dwc);
>> -	clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
>> -	dwc3_qcom_interconnect_exit(qcom);
>> +
>> +	if (!qcom->fw_managed) {
>> +		clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
>> +		dwc3_qcom_interconnect_exit(qcom);
>> +	} else {
>> +		dwc3_qcom_domains_put(qcom, false /* bus_only */);
>> +		dwc3_qcom_domain_detach(qcom);
>> +	}
>> +
>> +	reset_control_assert(qcom->resets);
>>   
>>   	pm_runtime_put_noidle(qcom->dev);
>>   }
> 
> There are so many judgements for qcom->fw_managed during the main flow, since
> there are only resource management difference, is it possible hide the differences
> for the main flow, and add some qcom USB resource APIs for them, eg:
> 
> static void dwc3_qcom_clk_disable(struct dwc3_qcom *qcom, bool bus_only)
> {
> 	if (!qcom->fw_managed)
> 		clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
> 	else
> 		dwc3_qcom_domains_put(qcom, is_all);
> }
> 
> For calling dwc3_qcom_interconnect_init(qcom), if it is fw_managed, just boil out.
> 

Ok, I will add resource enable/disable helpers that wrap either the 
clocks and interconnect or the power domain votes, so the suspend, 
resume, probe and remove paths no longer check fw_managed. 
dwc3_qcom_interconnect_init() will return early on firmware-managed 
platforms, as you suggest.

Thanks,
Faisal