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

Samuel Holland samuel at sholland.org
Wed Apr 21 15:36:26 CEST 2021


On 4/21/21 5:36 AM, Andre Przywara wrote:
> On Sat, 17 Apr 2021 09:20:57 -0500
> Samuel Holland <samuel at 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 at 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 at arm.com>

Thanks!

Note that after a pending device tree change[1], this patch won't be
strictly necessary for this platform anymore, since the DWC3 node will
no longer have a resets property. However I still think it's a good
change to make.

Regards,
Samuel

[1]:
https://lore.kernel.org/linux-sunxi/20210421042834.27309-3-samuel@sholland.org/

> 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;
> 



More information about the U-Boot mailing list