Re: [PATCH v2 2/3] usb: dwc3: xilinx: re-assert resets on ZynqMP init error paths

["Pandey, Radhey Shyam" <radheys@amd.com>]

On 9/23/2026 2:43 PM, Philipp Zabel wrote:
> On Di, 2026-09-22 at 23:51 +0530, Radhey Shyam Pandey wrote:
>> If reset deassert or PHY setup fails partway through
>> dwc3_xlnx_init_zynqmp(), re-assert any resets that were already
>> released before unwinding the PHY. Use fall-through error labels so
>> unwind matches how far init progressed, for both USB2 and USB3 paths.
>>
>> Save the ZynqMP reset handles in driver private data so later probe
>> teardown can re-assert released resets.
>>
>> Fixes: 84770f028fab ("usb: dwc3: Add driver for Xilinx platforms")
>> Cc: stable@vger.kernel.org
>> Assisted-by: Claude:claude-opus-5
>> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
>> ---
>> Changes in v2:
>> - Split out of the combined five patch series; see patch 1.
>> - Reordered ahead of the platform-data cleanups.
>> - Added Cc: stable.
>> - No functional change to the patch itself.
>>
>>   drivers/usb/dwc3/dwc3-xilinx.c | 49 +++++++++++++++++++++-------------
>>   1 file changed, 30 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-xilinx.c
>> index 8c63e02575f1..d18d3e364381 100644
>> --- a/drivers/usb/dwc3/dwc3-xilinx.c
>> +++ b/drivers/usb/dwc3/dwc3-xilinx.c
>> @@ -48,6 +48,10 @@ struct dwc3_xlnx {
>>   	void __iomem			*regs;
>>   	int				(*pltfm_init)(struct dwc3_xlnx *data);
>>   	struct phy			*usb3_phy;
>> +	struct reset_control		*usb_crst;
>> +	struct reset_control		*usb_hibrst;
>> +	struct reset_control		*usb_apbrst;
>> +	bool				usb_resets_released;
>>   };
>>   
>>   static void dwc3_xlnx_mask_phy_rst(struct dwc3_xlnx *priv_data, bool mask)
>> @@ -112,7 +116,6 @@ static int dwc3_xlnx_init_versal(struct dwc3_xlnx *priv_data)
>>   static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
>>   {
>>   	struct device		*dev = priv_data->dev;
>> -	struct reset_control	*crst, *hibrst, *apbrst;
>>   	struct gpio_desc	*reset_gpio;
>>   	int			ret = 0;
>>   
>> @@ -124,25 +127,25 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
>>   		goto err;
>>   	}
>>   
>> -	crst = devm_reset_control_get_exclusive(dev, "usb_crst");
>> -	if (IS_ERR(crst)) {
>> -		ret = PTR_ERR(crst);
>> +	priv_data->usb_crst = devm_reset_control_get_exclusive(dev, "usb_crst");
>> +	if (IS_ERR(priv_data->usb_crst)) {
>> +		ret = PTR_ERR(priv_data->usb_crst);
>>   		dev_err_probe(dev, ret,
>>   			      "failed to get core reset signal\n");
>>   		goto err;
>>   	}
>>   
>> -	hibrst = devm_reset_control_get_exclusive(dev, "usb_hibrst");
>> -	if (IS_ERR(hibrst)) {
>> -		ret = PTR_ERR(hibrst);
>> +	priv_data->usb_hibrst = devm_reset_control_get_exclusive(dev, "usb_hibrst");
>> +	if (IS_ERR(priv_data->usb_hibrst)) {
>> +		ret = PTR_ERR(priv_data->usb_hibrst);
>>   		dev_err_probe(dev, ret,
>>   			      "failed to get hibernation reset signal\n");
>>   		goto err;
>>   	}
>>   
>> -	apbrst = devm_reset_control_get_exclusive(dev, "usb_apbrst");
>> -	if (IS_ERR(apbrst)) {
>> -		ret = PTR_ERR(apbrst);
>> +	priv_data->usb_apbrst = devm_reset_control_get_exclusive(dev, "usb_apbrst");
>> +	if (IS_ERR(priv_data->usb_apbrst)) {
>> +		ret = PTR_ERR(priv_data->usb_apbrst);
>>   		dev_err_probe(dev, ret,
>>   			      "failed to get APB reset signal\n");
>>   		goto err;
>> @@ -156,19 +159,19 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
>>   	 * absent.
>>   	 */
>>   	if (priv_data->usb3_phy) {
>> -		ret = reset_control_assert(crst);
>> +		ret = reset_control_assert(priv_data->usb_crst);
>>   		if (ret < 0) {
>>   			dev_err(dev, "Failed to assert core reset\n");
>>   			goto err;
>>   		}
>>   
>> -		ret = reset_control_assert(hibrst);
>> +		ret = reset_control_assert(priv_data->usb_hibrst);
>>   		if (ret < 0) {
>>   			dev_err(dev, "Failed to assert hibernation reset\n");
>>   			goto err;
>>   		}
>>   
>> -		ret = reset_control_assert(apbrst);
>> +		ret = reset_control_assert(priv_data->usb_apbrst);
>>   		if (ret < 0) {
>>   			dev_err(dev, "Failed to assert APB reset\n");
>>   			goto err;
>> @@ -179,7 +182,7 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
>>   	if (ret < 0)
>>   		goto err;
>>   
>> -	ret = reset_control_deassert(apbrst);
>> +	ret = reset_control_deassert(priv_data->usb_apbrst);
>>   	if (ret < 0) {
>>   		dev_err(dev, "Failed to release APB reset\n");
>>   		goto err_phy_exit;
>> @@ -195,21 +198,21 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
>>   		writel(PIPE_CLK_DESELECT, priv_data->regs + XLNX_USB_FPD_PIPE_CLK);
>>   	}
>>   
>> -	ret = reset_control_deassert(crst);
>> +	ret = reset_control_deassert(priv_data->usb_crst);
>>   	if (ret < 0) {
>>   		dev_err(dev, "Failed to release core reset\n");
>> -		goto err_phy_exit;
>> +		goto err_apbrst_assert;
>>   	}
>>   
>> -	ret = reset_control_deassert(hibrst);
>> +	ret = reset_control_deassert(priv_data->usb_hibrst);
>>   	if (ret < 0) {
>>   		dev_err(dev, "Failed to release hibernation reset\n");
>> -		goto err_phy_exit;
>> +		goto err_crst_assert;
>>   	}
>>   
>>   	ret = phy_power_on(priv_data->usb3_phy);
>>   	if (ret < 0)
>> -		goto err_phy_exit;
>> +		goto err_hibrst_assert;
>>   
>>   	/* ulpi reset via gpio-modepin or gpio-framework driver */
>>   	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> 
> Are the resets asserted if this fails?

YEs, if reset_gpio fails resets are asserted.

> 
> Same question about if probe fails after pltfm_init() succeeded.
Valid point. In this patch, only the init_zynqmp() error handling is 
fixed; probe failure handling is addressed in the subsequent patch.

> 
>> @@ -226,10 +229,18 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
>>   
>>   	dwc3_xlnx_set_coherency(priv_data, XLNX_USB_TRAFFIC_ROUTE_CONFIG);
>>   
>> +	priv_data->usb_resets_released = true;
>> +
>>   	return 0;
>>   
>>   err_phy_power_off:
>>   	phy_power_off(priv_data->usb3_phy);
>> +err_hibrst_assert:
>> +	reset_control_assert(priv_data->usb_hibrst);
>> +err_crst_assert:
>> +	reset_control_assert(priv_data->usb_crst);
>> +err_apbrst_assert:
>> +	reset_control_assert(priv_data->usb_apbrst);
>>   err_phy_exit:
>>   	phy_exit(priv_data->usb3_phy);
>>   err:
> 
> Rather than storing usb_resets_released state, and then having
> conditional teardown in the next patch, this could be handled via
> devm_add_action_or_reset.
I agree on it, it simplifies the error handling and will spin v3.

Thanks,
Radhey