| Message ID | 1379464155-13250-1-git-send-email-jwerner@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
Hi, On Tue, Sep 17, 2013 at 6:29 PM, Julius Werner <jwerner@chromium.org> wrote: > The EHCI controller has some very specific requirements for the USB 2.0 > port test modes, which were not closely followed in the initial test > mode commit. It demands that the host controller is completely shut down > (all ports suspended, Run/Stop bit unset) when activating test mode, and > will not work on an already enumerated port. > > This patch fixes that by introducing a new ehci_shutdown() function that > closely follows the procedure listed in EHCI 4.14. Also, when we have > such a function anyway, we might as well also use it in > usb_lowlevel_stop() to make the normal host controller shutdown cleaner. > Marek this seems like a bug fix - should we pick it up for the release? > > Signed-off-by: Julius Werner <jwerner@chromium.org> > --- > drivers/usb/host/ehci-hcd.c | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c > index fdad739..3143021 100644 > --- a/drivers/usb/host/ehci-hcd.c > +++ b/drivers/usb/host/ehci-hcd.c > @@ -190,6 +190,35 @@ out: > return ret; > } > > +static int ehci_shutdown(struct ehci_ctrl *ctrl) > Suggest a comment as to what this function does (e.g. a few lines from your commit message). > +{ > + int i, ret = 0; > nit: = 0 not needed I think > + uint32_t cmd, reg; > + > + cmd = ehci_readl(&ctrl->hcor->or_usbcmd); > + cmd &= ~(CMD_PSE | CMD_ASE); > + ehci_writel(&ctrl->hcor->or_usbcmd, cmd); > + ret = handshake(&ctrl->hcor->or_usbsts, STS_ASS | STS_PSS, 0, 8 * > 1000); > + > + if (!ret) { > + for (i = 0; i < CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS; i++) { > + reg = ehci_readl(&ctrl->hcor->or_portsc[i]); > + reg |= EHCI_PS_SUSP; > + ehci_writel(&ctrl->hcor->or_portsc[i], reg); > + } > + > + cmd &= ~CMD_RUN; > + ehci_writel(&ctrl->hcor->or_usbcmd, cmd); > + ret = handshake(&ctrl->hcor->or_usbsts, STS_HALT, STS_HALT, > + 8 * 1000); > + } > + > + if (ret) > + printf("EHCI failed to shut down host controller.\n"); > + > + return ret; > +} > + > static int ehci_td_buffer(struct qTD *td, void *buf, size_t sz) > { > uint32_t delta, next; > @@ -808,6 +837,7 @@ ehci_submit_root(struct usb_device *dev, unsigned long > pipe, void *buffer, > } > break; > case USB_PORT_FEAT_TEST: > + ehci_shutdown(ctrl); > If it cannot be shut down, I suppose there is nothing that can be done here. Should this function exit here, or not? > reg &= ~(0xf << 16); > reg |= ((le16_to_cpu(req->index) >> 8) & 0xf) << > 16; > ehci_writel(status_reg, reg); > @@ -878,6 +908,7 @@ unknown: > > int usb_lowlevel_stop(int index) > { > + ehci_shutdown(&ehcic[index]); > return ehci_hcd_stop(index); > } > > -- > 1.7.12.4 > > Regards, Simon
Dear Julius Werner, > The EHCI controller has some very specific requirements for the USB 2.0 > port test modes, which were not closely followed in the initial test > mode commit. It demands that the host controller is completely shut down > (all ports suspended, Run/Stop bit unset) when activating test mode, and > will not work on an already enumerated port. > > This patch fixes that by introducing a new ehci_shutdown() function that > closely follows the procedure listed in EHCI 4.14. Also, when we have > such a function anyway, we might as well also use it in > usb_lowlevel_stop() to make the normal host controller shutdown cleaner. > > Signed-off-by: Julius Werner <jwerner@chromium.org> > --- > drivers/usb/host/ehci-hcd.c | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c > index fdad739..3143021 100644 > --- a/drivers/usb/host/ehci-hcd.c > +++ b/drivers/usb/host/ehci-hcd.c > @@ -190,6 +190,35 @@ out: > return ret; > } > > +static int ehci_shutdown(struct ehci_ctrl *ctrl) > +{ > + int i, ret = 0; > + uint32_t cmd, reg; > + > + cmd = ehci_readl(&ctrl->hcor->or_usbcmd); > + cmd &= ~(CMD_PSE | CMD_ASE); > + ehci_writel(&ctrl->hcor->or_usbcmd, cmd); > + ret = handshake(&ctrl->hcor->or_usbsts, STS_ASS | STS_PSS, 0, 8 * 1000); Why 8 * 1000? It's not clear. > + > + if (!ret) { > + for (i = 0; i < CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS; i++) { > + reg = ehci_readl(&ctrl->hcor->or_portsc[i]); > + reg |= EHCI_PS_SUSP; > + ehci_writel(&ctrl->hcor->or_portsc[i], reg); > + } > + > + cmd &= ~CMD_RUN; > + ehci_writel(&ctrl->hcor->or_usbcmd, cmd); > + ret = handshake(&ctrl->hcor->or_usbsts, STS_HALT, STS_HALT, > + 8 * 1000); DTTO > + } > + > + if (ret) > + printf("EHCI failed to shut down host controller.\n"); puts() > + > + return ret; > +} > + > static int ehci_td_buffer(struct qTD *td, void *buf, size_t sz) > { > uint32_t delta, next; > @@ -808,6 +837,7 @@ ehci_submit_root(struct usb_device *dev, unsigned long > pipe, void *buffer, } > break; > case USB_PORT_FEAT_TEST: > + ehci_shutdown(ctrl); > reg &= ~(0xf << 16); > reg |= ((le16_to_cpu(req->index) >> 8) & 0xf) << 16; > ehci_writel(status_reg, reg); > @@ -878,6 +908,7 @@ unknown: > > int usb_lowlevel_stop(int index) > { > + ehci_shutdown(&ehcic[index]); > return ehci_hcd_stop(index); > } Best regards, Marek Vasut
> Why 8 * 1000? It's not clear.
I am not quite sure to be honest... it's been a while since I actually
wrote this patch. The EHCI spec gives no clear max for the
Periodic/Async Schedule disable (I assume not more than a frame,
though), and says the Run/Stop bit must halt within 16 microframes
(which would be 4.125ms). I guess 8 looked like a safe and not too
large value that still makes the line fit on 80 characters (and it's
not like most of the other timeouts in that file have a clear origin,
so it's in good company). What do you want me to change it to?
Dear Julius Werner, > > Why 8 * 1000? It's not clear. > > I am not quite sure to be honest... it's been a while since I actually > wrote this patch. The EHCI spec gives no clear max for the > Periodic/Async Schedule disable (I assume not more than a frame, > though), and says the Run/Stop bit must halt within 16 microframes > (which would be 4.125ms). I guess 8 looked like a safe and not too > large value that still makes the line fit on 80 characters (and it's > not like most of the other timeouts in that file have a clear origin, > so it's in good company). What do you want me to change it to? I would like to hear a convincing argument why 8 is the good value. The amount of "I guess" in the text above does not convince me. It would be also good if you didn't sprinkle duplicates of this magic value all around , but instead #define it somewhere and put a beefy comment above it. Best regards, Marek Vasut
Hello Marek, 8 seemed like a reasonable value. It is not the only possible one. As I said, the spec requires a minimum of 4.125 (125us to make sure the current microframe finishes, and 16 * 125us to wait for the 16 full microframes listed on page 20). 8 fulfills that constraint and leaves a little more tolerance, but you could also use 5, 16 or 100. I really don't care, so please just tell me what number you want to have if you don't like 8, because it just doesn't matter. It is no more or less magic than the 100ms used in most other handshake() calls (without any #define or explanation).
Hi Julius, On Thu, Sep 19, 2013 at 7:39 PM, Julius Werner <jwerner@chromium.org> wrote: > Hello Marek, > > 8 seemed like a reasonable value. It is not the only possible one. As > I said, the spec requires a minimum of 4.125 (125us to make sure the > current microframe finishes, and 16 * 125us to wait for the 16 full > microframes listed on page 20). 8 fulfills that constraint and leaves > a little more tolerance, but you could also use 5, 16 or 100. I really > don't care, so please just tell me what number you want to have if you > don't like 8, because it just doesn't matter. It is no more or less > magic than the 100ms used in most other handshake() calls (without any > #define or explanation). > It seems like you could do something like /* * This is the delay for ...the spec requires a minimum of ... */ #define SOME_SUITABLE_NAME 8000 at the top of the file and then use it twice in your function. Regards, Simon
Hi Simon, > It seems like you could do something like > > /* > * This is the delay for ...the spec requires a minimum of ... > */ > #define SOME_SUITABLE_NAME 8000 > > at the top of the file and then use it twice in your function. The file contains a dozen handshake() calls that have a literal "100 * 1000" in there, and none of them have any special meaning in regards to the spec... it's all just a random guess. I'd be happy to change the timeouts in my patch to 100 for consistency, but it really doesn't make sense to single this one out with a #define. (Maybe we should make it a #define DEFAULT_TIMEOUT 100000 for all of them, but not in this patch.)
Hi Julius, On Mon, Sep 23, 2013 at 8:55 PM, Julius Werner <jwerner@chromium.org> wrote: > Hi Simon, > > > It seems like you could do something like > > > > /* > > * This is the delay for ...the spec requires a minimum of ... > > */ > > #define SOME_SUITABLE_NAME 8000 > > > > at the top of the file and then use it twice in your function. > > The file contains a dozen handshake() calls that have a literal "100 * > 1000" in there, and none of them have any special meaning in regards > to the spec... it's all just a random guess. I'd be happy to change > the timeouts in my patch to 100 for consistency, but it really doesn't > make sense to single this one out with a #define. (Maybe we should > make it a #define DEFAULT_TIMEOUT 100000 for all of them, but not in > this patch.) > I guess we are trying to distinguish between how the code is and how we would like it to me. From that point of view your patch is independent of the existing code. Since you have a clear reason for the timeout you have chose, you can document that and set a good example for others! Regards, Simon
Dear Simon Glass, > Hi Julius, > > On Mon, Sep 23, 2013 at 8:55 PM, Julius Werner <jwerner@chromium.org> wrote: > > Hi Simon, > > > > > It seems like you could do something like > > > > > > /* > > > > > > * This is the delay for ...the spec requires a minimum of ... > > > */ > > > > > > #define SOME_SUITABLE_NAME 8000 > > > > > > at the top of the file and then use it twice in your function. > > > > The file contains a dozen handshake() calls that have a literal "100 * > > 1000" in there, and none of them have any special meaning in regards > > to the spec... it's all just a random guess. I'd be happy to change > > the timeouts in my patch to 100 for consistency, but it really doesn't > > make sense to single this one out with a #define. (Maybe we should > > make it a #define DEFAULT_TIMEOUT 100000 for all of them, but not in > > this patch.) > > I guess we are trying to distinguish between how the code is and how we > would like it to me. From that point of view your patch is independent of > the existing code. Since you have a clear reason for the timeout you have > chose, you can document that and set a good example for others! ACK on that. We should focus on improvement, not stagnate. Best regards, Marek Vasut
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index fdad739..3143021 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -190,6 +190,35 @@ out: return ret; } +static int ehci_shutdown(struct ehci_ctrl *ctrl) +{ + int i, ret = 0; + uint32_t cmd, reg; + + cmd = ehci_readl(&ctrl->hcor->or_usbcmd); + cmd &= ~(CMD_PSE | CMD_ASE); + ehci_writel(&ctrl->hcor->or_usbcmd, cmd); + ret = handshake(&ctrl->hcor->or_usbsts, STS_ASS | STS_PSS, 0, 8 * 1000); + + if (!ret) { + for (i = 0; i < CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS; i++) { + reg = ehci_readl(&ctrl->hcor->or_portsc[i]); + reg |= EHCI_PS_SUSP; + ehci_writel(&ctrl->hcor->or_portsc[i], reg); + } + + cmd &= ~CMD_RUN; + ehci_writel(&ctrl->hcor->or_usbcmd, cmd); + ret = handshake(&ctrl->hcor->or_usbsts, STS_HALT, STS_HALT, + 8 * 1000); + } + + if (ret) + printf("EHCI failed to shut down host controller.\n"); + + return ret; +} + static int ehci_td_buffer(struct qTD *td, void *buf, size_t sz) { uint32_t delta, next; @@ -808,6 +837,7 @@ ehci_submit_root(struct usb_device *dev, unsigned long pipe, void *buffer, } break; case USB_PORT_FEAT_TEST: + ehci_shutdown(ctrl); reg &= ~(0xf << 16); reg |= ((le16_to_cpu(req->index) >> 8) & 0xf) << 16; ehci_writel(status_reg, reg); @@ -878,6 +908,7 @@ unknown: int usb_lowlevel_stop(int index) { + ehci_shutdown(&ehcic[index]); return ehci_hcd_stop(index); }
The EHCI controller has some very specific requirements for the USB 2.0 port test modes, which were not closely followed in the initial test mode commit. It demands that the host controller is completely shut down (all ports suspended, Run/Stop bit unset) when activating test mode, and will not work on an already enumerated port. This patch fixes that by introducing a new ehci_shutdown() function that closely follows the procedure listed in EHCI 4.14. Also, when we have such a function anyway, we might as well also use it in usb_lowlevel_stop() to make the normal host controller shutdown cleaner. Signed-off-by: Julius Werner <jwerner@chromium.org> --- drivers/usb/host/ehci-hcd.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)