[PATCH v2 2/4] usb: xhci-pci: Move reset logic out of XHCI core
Andre Przywara
andre.przywara at arm.com
Wed Apr 21 16:58:00 CEST 2021
On Wed, 21 Apr 2021 08:36:26 -0500
Samuel Holland <samuel at 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 <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.
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-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