| Message ID | 20200622153050.23193-5-nsaenzjulienne@suse.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: xhci: Load Raspberry Pi 4 VL805's firmware | expand |
On 6/22/20 5:30 PM, Nicolas Saenz Julienne wrote: [...] > diff --git a/include/usb/xhci.h b/include/usb/xhci.h > index 1170c0ac69..7d34103fd5 100644 > --- a/include/usb/xhci.h > +++ b/include/usb/xhci.h > @@ -16,6 +16,7 @@ > #ifndef HOST_XHCI_H_ > #define HOST_XHCI_H_ > > +#include <reset.h> > #include <asm/types.h> > #include <asm/cache.h> > #include <asm/io.h> > @@ -1209,6 +1210,7 @@ struct xhci_ctrl { > #if CONFIG_IS_ENABLED(DM_USB) > struct udevice *dev; > #endif > + struct reset_ctl reset; Should all this reset logic be protected by if CONFIG_IS_ENABLED(DM_RESET) ? Otherwise the series looks good to me, thanks.
Hi Marek, On Mon, 2020-06-22 at 17:34 +0200, Marek Vasut wrote: > On 6/22/20 5:30 PM, Nicolas Saenz Julienne wrote: > [...] > > > diff --git a/include/usb/xhci.h b/include/usb/xhci.h > > index 1170c0ac69..7d34103fd5 100644 > > --- a/include/usb/xhci.h > > +++ b/include/usb/xhci.h > > @@ -16,6 +16,7 @@ > > #ifndef HOST_XHCI_H_ > > #define HOST_XHCI_H_ > > > > +#include <reset.h> > > #include <asm/types.h> > > #include <asm/cache.h> > > #include <asm/io.h> > > @@ -1209,6 +1210,7 @@ struct xhci_ctrl { > > #if CONFIG_IS_ENABLED(DM_USB) > > struct udevice *dev; > > #endif > > + struct reset_ctl reset; > > Should all this reset logic be protected by if CONFIG_IS_ENABLED(DM_RESET) ? As far as building the code there shouldn't be any issues, function/structures are defined in either case. That said I can see how there could be a runtime issue for the !CONFIG_IS_ENABLED(DM_RESET) case, as xhci_reset_hw() will return an error and make xchi_register() fail. So I can either protect everything with preprocessor conditionals, both in the struct as in xhci_register(), or catch -ENOTSUPP in xhci_register() and continue the registration as usual. I prefer the later as it adds less preprocessor churn, but I'll go the other way if you prefer it. > Otherwise the series looks good to me, thanks. Thanks! Regards, Nicolas -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 488 bytes Desc: This is a digitally signed message part URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200624/a72cb35a/attachment.sig>
On 6/24/20 12:55 PM, Nicolas Saenz Julienne wrote: > Hi Marek, Hi, > On Mon, 2020-06-22 at 17:34 +0200, Marek Vasut wrote: >> On 6/22/20 5:30 PM, Nicolas Saenz Julienne wrote: >> [...] >> >>> diff --git a/include/usb/xhci.h b/include/usb/xhci.h >>> index 1170c0ac69..7d34103fd5 100644 >>> --- a/include/usb/xhci.h >>> +++ b/include/usb/xhci.h >>> @@ -16,6 +16,7 @@ >>> #ifndef HOST_XHCI_H_ >>> #define HOST_XHCI_H_ >>> >>> +#include <reset.h> >>> #include <asm/types.h> >>> #include <asm/cache.h> >>> #include <asm/io.h> >>> @@ -1209,6 +1210,7 @@ struct xhci_ctrl { >>> #if CONFIG_IS_ENABLED(DM_USB) >>> struct udevice *dev; >>> #endif >>> + struct reset_ctl reset; >> >> Should all this reset logic be protected by if CONFIG_IS_ENABLED(DM_RESET) ? > > As far as building the code there shouldn't be any issues, function/structures > are defined in either case. > > That said I can see how there could be a runtime issue for the > !CONFIG_IS_ENABLED(DM_RESET) case, as xhci_reset_hw() will return an error and > make xchi_register() fail. > > So I can either protect everything with preprocessor conditionals, both in the > struct as in xhci_register(), or catch -ENOTSUPP in xhci_register() and > continue the registration as usual. I prefer the later as it adds less > preprocessor churn, but I'll go the other way if you prefer it. I agree, lets go with the later, thanks.
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index f446520528..108f4bd8cf 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -180,6 +180,8 @@ 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.c b/drivers/usb/host/xhci.c index ebd2954571..03b41cc855 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -190,6 +190,35 @@ static int xhci_start(struct xhci_hcor *hcor) return ret; } +/** + * 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) { + 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; +} + /** * Resets the XHCI Controller * @@ -1508,6 +1537,10 @@ 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 1170c0ac69..7d34103fd5 100644 --- a/include/usb/xhci.h +++ b/include/usb/xhci.h @@ -16,6 +16,7 @@ #ifndef HOST_XHCI_H_ #define HOST_XHCI_H_ +#include <reset.h> #include <asm/types.h> #include <asm/cache.h> #include <asm/io.h> @@ -1209,6 +1210,7 @@ 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;
Some atypical users of xhci might need to manually reset their xHCI controller before starting the HCD setup. Check if a reset controller device is available to the PCI bus and trigger a reset. Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne at suse.de> --- Changes since v3: - Move reset support to xchi core drivers/usb/host/xhci-mem.c | 2 ++ drivers/usb/host/xhci.c | 33 +++++++++++++++++++++++++++++++++ include/usb/xhci.h | 2 ++ 3 files changed, 37 insertions(+)