On Wed, 21 Apr 2021 08:36:26 -0500 Samuel Holland <sam...@sholland.org> wrote:
Hi, > 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. Yes, I saw that, but those DT changes won't make it into U-Boot for a while, so we should go ahead with those patches anyway. This would also allow us the get the PHY driver tested already. And I think we need another U-Boot patch, to teach it about the new(ly used) compatible string? I don't find "allwinner,sun50i-h6-dwc3" anywhere in the current U-Boot code. Cheers, Andre > > [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; > > >