[PATCH v2 2/4] usb: xhci-pci: Move reset logic out of XHCI core

[Andre Przywara <andre.przywara@arm.com>]

On Sat, 17 Apr 2021 09:20:57 -0500
Samuel Holland <samuel@sholland.org> wrote:

Hi,

> Resetting an XHCI controller inside xhci_register undoes any register
> setup performed by the platform driver. And at least on the Allwinner
> H6, resetting the XHCI controller also resets the PHY, which prevents
> the controller from working. That means the controller must be taken out
> of reset before initializing the PHY, which must be done before calling
> xhci_register.
> 
> The logic in the XHCI core was added to support the Raspberry Pi 4
> (although this was not mentioned in the commit log!), which uses the
> xhci-pci platform driver. Move the reset logic to the platform driver,
> where it belongs, and where it cannot interfere with other platform
> drivers.
> 
> This also fixes a failure to call reset_free if xhci_register failed.
> 
> Fixes: 0b80371b350e ("usb: xhci: Add reset controller support")
> Signed-off-by: Samuel Holland <samuel@sholland.org>

From my research it looks like no other board should be affected by the
change, and they have been no reports from the people I asked a few
weeks ago.
I'd say we should merge it, to expose it to a wider audience, and keep
an eye on bug reports.

Acked-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>  drivers/usb/host/xhci-mem.c |  2 --
>  drivers/usb/host/xhci-pci.c | 51 ++++++++++++++++++++++++++++++++++---
>  drivers/usb/host/xhci.c     | 35 -------------------------
>  include/usb/xhci.h          |  2 --
>  4 files changed, 47 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 1c11c2e7e0..0d9da62bab 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -180,8 +180,6 @@ void xhci_cleanup(struct xhci_ctrl *ctrl)
>  	xhci_free_virt_devices(ctrl);
>  	free(ctrl->erst.entries);
>  	free(ctrl->dcbaa);
> -	if (reset_valid(&ctrl->reset))
> -		reset_free(&ctrl->reset);
>  	memset(ctrl, '\0', sizeof(struct xhci_ctrl));
>  }
>  
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index aaa243f291..ea8e8f3211 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -10,9 +10,14 @@
>  #include <init.h>
>  #include <log.h>
>  #include <pci.h>
> +#include <reset.h>
>  #include <usb.h>
>  #include <usb/xhci.h>
>  
> +struct xhci_pci_plat {
> +	struct reset_ctl reset;
> +};
> +
>  static int xhci_pci_init(struct udevice *dev, struct xhci_hccr **ret_hccr,
>  			 struct xhci_hcor **ret_hcor)
>  {
> @@ -45,15 +50,53 @@ static int xhci_pci_init(struct udevice *dev, struct xhci_hccr **ret_hccr,
>  
>  static int xhci_pci_probe(struct udevice *dev)
>  {
> +	struct xhci_pci_plat = dev_get_plat(dev);
>  	struct xhci_hccr *hccr;
>  	struct xhci_hcor *hcor;
>  	int ret;
>  
> +	ret = reset_get_by_index(dev, 0, &plat->reset);
> +	if (ret && ret != -ENOENT && ret != -ENOTSUPP) {
> +		dev_err(dev, "failed to get reset\n");
> +		return ret;
> +	}
> +
> +	if (reset_valid(&plat->reset)) {
> +		ret = reset_assert(&plat->reset);
> +		if (ret)
> +			goto err_reset;
> +
> +		ret = reset_deassert(&plat->reset);
> +		if (ret)
> +			goto err_reset;
> +	}
> +
>  	ret = xhci_pci_init(dev, &hccr, &hcor);
>  	if (ret)
> -		return ret;
> +		goto err_reset;
> +
> +	ret = xhci_register(dev, hccr, hcor);
> +	if (ret)
> +		goto err_reset;
> +
> +	return 0;
> +
> +err_reset:
> +	if (reset_valid(&plat->reset))
> +		reset_free(&plat->reset);
> +
> +	return ret;
> +}
> +
> +static int xhci_pci_remove(struct udevice *dev)
> +{
> +	struct xhci_pci_plat = dev_get_plat(dev);
>  
> -	return xhci_register(dev, hccr, hcor);
> +	xhci_deregister(dev);
> +	if (reset_valid(&plat->reset))
> +		reset_free(&plat->reset);
> +
> +	return 0;
>  }
>  
>  static const struct udevice_id xhci_pci_ids[] = {
> @@ -65,10 +108,10 @@ U_BOOT_DRIVER(xhci_pci) = {
>  	.name	= "xhci_pci",
>  	.id	= UCLASS_USB,
>  	.probe = xhci_pci_probe,
> -	.remove = xhci_deregister,
> +	.remove	= xhci_pci_remove,
>  	.of_match = xhci_pci_ids,
>  	.ops	= &xhci_usb_ops,
> -	.plat_auto	= sizeof(struct usb_plat),
> +	.plat_auto	= sizeof(struct xhci_pci_plat),
>  	.priv_auto	= sizeof(struct xhci_ctrl),
>  	.flags	= DM_FLAG_OS_PREPARE | DM_FLAG_ALLOC_PRIV_DMA,
>  };
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index d27ac01c83..452dacc0af 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -188,37 +188,6 @@ static int xhci_start(struct xhci_hcor *hcor)
>  	return ret;
>  }
>  
> -#if CONFIG_IS_ENABLED(DM_USB)
> -/**
> - * Resets XHCI Hardware
> - *
> - * @param ctrl	pointer to host controller
> - * @return 0 if OK, or a negative error code.
> - */
> -static int xhci_reset_hw(struct xhci_ctrl *ctrl)
> -{
> -	int ret;
> -
> -	ret = reset_get_by_index(ctrl->dev, 0, &ctrl->reset);
> -	if (ret && ret != -ENOENT && ret != -ENOTSUPP) {
> -		dev_err(ctrl->dev, "failed to get reset\n");
> -		return ret;
> -	}
> -
> -	if (reset_valid(&ctrl->reset)) {
> -		ret = reset_assert(&ctrl->reset);
> -		if (ret)
> -			return ret;
> -
> -		ret = reset_deassert(&ctrl->reset);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	return 0;
> -}
> -#endif
> -
>  /**
>   * Resets the XHCI Controller
>   *
> @@ -1534,10 +1503,6 @@ int xhci_register(struct udevice *dev, struct xhci_hccr *hccr,
>  
>  	ctrl->dev = dev;
>  
> -	ret = xhci_reset_hw(ctrl);
> -	if (ret)
> -		goto err;
> -
>  	/*
>  	 * XHCI needs to issue a Address device command to setup
>  	 * proper device context structures, before it can interact
> diff --git a/include/usb/xhci.h b/include/usb/xhci.h
> index 8d95e213b0..01e63cf0fc 100644
> --- a/include/usb/xhci.h
> +++ b/include/usb/xhci.h
> @@ -17,7 +17,6 @@
>  #define HOST_XHCI_H_
>  
>  #include <phys2bus.h>
> -#include <reset.h>
>  #include <asm/types.h>
>  #include <asm/cache.h>
>  #include <asm/io.h>
> @@ -1200,7 +1199,6 @@ struct xhci_ctrl {
>  #if CONFIG_IS_ENABLED(DM_USB)
>  	struct udevice *dev;
>  #endif
> -	struct reset_ctl reset;
>  	struct xhci_hccr *hccr;	/* R/O registers, not need for volatile */
>  	struct xhci_hcor *hcor;
>  	struct xhci_doorbell_array *dba;