On 4/21/21 5:36 AM, Andre Przywara wrote:
> On Sat, 17 Apr 2021 09:20:57 -0500
> Samuel Holland <sam...@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 <sam...@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.przyw...@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-sam...@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;
>
