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

[Thinh Nguyen]

On Mon, Sep 28, 2026, Radhey Shyam Pandey wrote:
> If reset deassert or PHY setup fails partway through
> dwc3_xlnx_init_zynqmp(), re-assert the resets that were already
> released before unwinding the PHY.
> 
> Only undo the reset cycle the function actually performed. The cycle is
> skipped when no usb3-phy is present, because asserting the core resets
> can break a USB3 configuration that is live but missing from the device
> tree, so the error path skips it for the same reason. Asserting an
> already asserted reset is harmless, so one block covers however far the
> deasserts progressed.
> 
> 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@xxxxxxxxxxxxxxx
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xxxxxxx>
> ---
> Changes in v3:
> - Dropped the usb_resets_released field.  Nothing in this patch read it;
>   it only existed for the conditional teardown in patch 3, which no
>   longer needs it.
> - Collapsed the three cascading assert labels into one err_reset_assert
>   block guarded on priv_data->usb3_phy.  v2 asserted unconditionally,
>   which would have put a no-PHY board into reset that init never
>   asserted; the guard now matches the init-time cycle.
> 
> 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 | 55 ++++++++++++++++++++++------------
>  1 file changed, 36 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-xilinx.c
> index 8c63e02575f1..d2315573b3d3 100644
> --- a/drivers/usb/dwc3/dwc3-xilinx.c
> +++ b/drivers/usb/dwc3/dwc3-xilinx.c
> @@ -48,6 +48,9 @@ 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;
>  };
>  
>  static void dwc3_xlnx_mask_phy_rst(struct dwc3_xlnx *priv_data, bool mask)
> @@ -112,7 +115,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 +126,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 +158,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 +181,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 +197,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_reset_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_reset_assert;
>  	}
>  
>  	ret = phy_power_on(priv_data->usb3_phy);
>  	if (ret < 0)
> -		goto err_phy_exit;
> +		goto err_reset_assert;
>  
>  	/* ulpi reset via gpio-modepin or gpio-framework driver */
>  	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> @@ -230,6 +232,21 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
>  
>  err_phy_power_off:
>  	phy_power_off(priv_data->usb3_phy);
> +err_reset_assert:
> +	/*
> +	 * Undo only the reset cycle performed above.  That cycle is skipped
> +	 * when there is no usb3-phy.  The deasserts stay unconditional so a
> +	 * core the bootloader left in reset still comes up, but they are
> +	 * deliberately not paired with an assert here: resetting a USB3
> +	 * configuration that is live but missing from the device tree could
> +	 * break it.  Asserting an already asserted reset is harmless, so one
> +	 * block covers however far the cycle's deasserts got.
> +	 */

This is overly verbose. I'd reserve the comment in the commit message
only, although I suppose having it doesn't hurt..

> +	if (priv_data->usb3_phy) {
> +		reset_control_assert(priv_data->usb_hibrst);
> +		reset_control_assert(priv_data->usb_crst);
> +		reset_control_assert(priv_data->usb_apbrst);
> +	}
>  err_phy_exit:
>  	phy_exit(priv_data->usb3_phy);
>  err:
> -- 
> 2.44.4
> 

Acked-by: Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx>

BR,
Thinh