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