| Message ID | 20210417142059.45337-3-samuel@sholland.org |
|---|---|
| State | Superseded |
| Delegated to: | Andre Przywara |
| Headers | show |
| Series | Allwinner H6 USB3 support | expand |
On Sat, 17 Apr 2021 09:20:57 -0500 Samuel Holland <samuel@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@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@arm.com> 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;
On 4/21/21 5:36 AM, Andre Przywara wrote: > On Sat, 17 Apr 2021 09:20:57 -0500 > Samuel Holland <samuel@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@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@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; >
On Wed, 21 Apr 2021 08:36:26 -0500 Samuel Holland <samuel@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@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@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@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; > > >
On Sat, Apr 17, 2021 at 10:21 PM Samuel Holland <samuel@sholland.org> wrote: > > 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@sholland.org> > --- > 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(-) > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
On 7/5/21 10:04 AM, Bin Meng wrote: > On Sat, Apr 17, 2021 at 10:21 PM Samuel Holland <samuel@sholland.org> wrote: >> >> 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@sholland.org> >> --- >> 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(-) >> > > Reviewed-by: Bin Meng <bmeng.cn@gmail.com> So shall we apply this whole thing for 2021.10 ?
On Mon, Jul 5, 2021 at 4:19 PM Marek Vasut <marex@denx.de> wrote: > > On 7/5/21 10:04 AM, Bin Meng wrote: > > On Sat, Apr 17, 2021 at 10:21 PM Samuel Holland <samuel@sholland.org> wrote: > >> > >> 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@sholland.org> > >> --- > >> 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(-) > >> > > > > Reviewed-by: Bin Meng <bmeng.cn@gmail.com> > > So shall we apply this whole thing for 2021.10 ? Yes. Andre wanted to get this in 2021.07 which is too late. Regards, Bin
On Mon, 5 Jul 2021 16:38:29 +0800 Bin Meng <bmeng.cn@gmail.com> wrote: Hi, > On Mon, Jul 5, 2021 at 4:19 PM Marek Vasut <marex@denx.de> wrote: > > > > On 7/5/21 10:04 AM, Bin Meng wrote: > > > On Sat, Apr 17, 2021 at 10:21 PM Samuel Holland <samuel@sholland.org> wrote: > > >> > > >> 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@sholland.org> > > >> --- > > >> 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(-) > > >> > > > > > > Reviewed-by: Bin Meng <bmeng.cn@gmail.com> > > > > So shall we apply this whole thing for 2021.10 ? > > Yes. Andre wanted to get this in 2021.07 which is too late. Ah, sorry, I didn't mean into this release, but into the 2021.10 merge window. I was preparing the sunxi patches for the PR, so stumbled upon this. So I'd be grateful if you could push this into the MW, I can then finish up the sunxi side of things (patch 4/4). Thanks! Andre
Hi Andre, On Mon, Jul 5, 2021 at 5:07 PM Andre Przywara <andre.przywara@arm.com> wrote: > > On Mon, 5 Jul 2021 16:38:29 +0800 > Bin Meng <bmeng.cn@gmail.com> wrote: > > Hi, > > > On Mon, Jul 5, 2021 at 4:19 PM Marek Vasut <marex@denx.de> wrote: > > > > > > On 7/5/21 10:04 AM, Bin Meng wrote: > > > > On Sat, Apr 17, 2021 at 10:21 PM Samuel Holland <samuel@sholland.org> wrote: > > > >> > > > >> 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@sholland.org> > > > >> --- > > > >> 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(-) > > > >> > > > > > > > > Reviewed-by: Bin Meng <bmeng.cn@gmail.com> > > > > > > So shall we apply this whole thing for 2021.10 ? > > > > Yes. Andre wanted to get this in 2021.07 which is too late. > > Ah, sorry, I didn't mean into this release, but into the 2021.10 merge > window. I was preparing the sunxi patches for the PR, so stumbled upon > this. Ah, 2021.01 is not a problem. > > So I'd be grateful if you could push this into the MW, I can then > finish up the sunxi side of things (patch 4/4). Marek can pick up this soon. Regards, Bin
On 7/5/21 10:38 AM, Bin Meng wrote: > On Mon, Jul 5, 2021 at 4:19 PM Marek Vasut <marex@denx.de> wrote: >> >> On 7/5/21 10:04 AM, Bin Meng wrote: >>> On Sat, Apr 17, 2021 at 10:21 PM Samuel Holland <samuel@sholland.org> wrote: >>>> >>>> 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@sholland.org> >>>> --- >>>> 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(-) >>>> >>> >>> Reviewed-by: Bin Meng <bmeng.cn@gmail.com> >> >> So shall we apply this whole thing for 2021.10 ? > > Yes. Andre wanted to get this in 2021.07 which is too late. Applied to usb/next, thanks
On 7/5/21 11:18 AM, Bin Meng wrote: > Hi Andre, > > On Mon, Jul 5, 2021 at 5:07 PM Andre Przywara <andre.przywara@arm.com> wrote: >> >> On Mon, 5 Jul 2021 16:38:29 +0800 >> Bin Meng <bmeng.cn@gmail.com> wrote: >> >> Hi, >> >>> On Mon, Jul 5, 2021 at 4:19 PM Marek Vasut <marex@denx.de> wrote: >>>> >>>> On 7/5/21 10:04 AM, Bin Meng wrote: >>>>> On Sat, Apr 17, 2021 at 10:21 PM Samuel Holland <samuel@sholland.org> wrote: >>>>>> >>>>>> 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@sholland.org> >>>>>> --- >>>>>> 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(-) >>>>>> >>>>> >>>>> Reviewed-by: Bin Meng <bmeng.cn@gmail.com> >>>> >>>> So shall we apply this whole thing for 2021.10 ? >>> >>> Yes. Andre wanted to get this in 2021.07 which is too late. >> >> Ah, sorry, I didn't mean into this release, but into the 2021.10 merge >> window. I was preparing the sunxi patches for the PR, so stumbled upon >> this. > > Ah, 2021.01 is not a problem. > >> >> So I'd be grateful if you could push this into the MW, I can then >> finish up the sunxi side of things (patch 4/4). > > Marek can pick up this soon. U-Boot CI seems to fail on this, please recheck that.
On Mon, 5 Jul 2021 12:45:59 +0200 Marek Vasut <marex@denx.de> wrote: Hi, > On 7/5/21 11:18 AM, Bin Meng wrote: > > Hi Andre, > > > > On Mon, Jul 5, 2021 at 5:07 PM Andre Przywara <andre.przywara@arm.com> wrote: > >> > >> On Mon, 5 Jul 2021 16:38:29 +0800 > >> Bin Meng <bmeng.cn@gmail.com> wrote: > >> > >> Hi, > >> > >>> On Mon, Jul 5, 2021 at 4:19 PM Marek Vasut <marex@denx.de> wrote: > >>>> > >>>> On 7/5/21 10:04 AM, Bin Meng wrote: > >>>>> On Sat, Apr 17, 2021 at 10:21 PM Samuel Holland <samuel@sholland.org> wrote: > >>>>>> > >>>>>> 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@sholland.org> > >>>>>> --- > >>>>>> 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(-) > >>>>>> > >>>>> > >>>>> Reviewed-by: Bin Meng <bmeng.cn@gmail.com> > >>>> > >>>> So shall we apply this whole thing for 2021.10 ? > >>> > >>> Yes. Andre wanted to get this in 2021.07 which is too late. > >> > >> Ah, sorry, I didn't mean into this release, but into the 2021.10 merge > >> window. I was preparing the sunxi patches for the PR, so stumbled upon > >> this. > > > > Ah, 2021.01 is not a problem. > > > >> > >> So I'd be grateful if you could push this into the MW, I can then > >> finish up the sunxi side of things (patch 4/4). > > > > Marek can pick up this soon. > > U-Boot CI seems to fail on this, please recheck that. Ouch, thanks for the heads up! It's xhci-pci.c failing, as used by the RPi4, for instance. I will send a v3 ASAP. Cheers, Andre
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;
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@sholland.org> --- 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(-)