[U-Boot] [PATCH] usb: ehci: Fix test mode for connected ports

[U-Boot] [PATCH] usb: ehci: Fix test mode for connected ports

[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);
+
+	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);
 }
 
-- 
1.7.12.4

[U-Boot] [PATCH] usb: ehci: Fix test mode for connected ports

[Simon Glass]

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

[U-Boot] [PATCH] usb: ehci: Fix test mode for connected ports

[Marek Vasut]

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

[U-Boot] [PATCH] usb: ehci: Fix test mode for connected ports

[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?

[U-Boot] [PATCH] usb: ehci: Fix test mode for connected ports

[Marek Vasut]

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

[U-Boot] [PATCH] usb: ehci: Fix test mode for connected ports

[Julius Werner]

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).

[U-Boot] [PATCH] usb: ehci: Fix test mode for connected ports

[Simon Glass]

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

[U-Boot] [PATCH] usb: ehci: Fix test mode for connected ports

[Julius Werner]

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.)

[U-Boot] [PATCH] usb: ehci: Fix test mode for connected ports

[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!

Regards,
Simon

[U-Boot] [PATCH] usb: ehci: Fix test mode for connected ports

[Marek Vasut]

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

[U-Boot] [PATCH v2] usb: ehci: Fix test mode for connected ports

[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 | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index fdad739..3ae04c0 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -36,6 +36,12 @@
 #define CONFIG_USB_MAX_CONTROLLER_COUNT 1
 #endif
 
+/*
+ * EHCI spec page 20 says that the HC may take up to 16 uFrames (= 4ms) to halt.
+ * Let's time out after 8 to have a little safety margin on top of that.
+ */
+#define HCHALT_TIMEOUT (8 * 1000)
+
 static struct ehci_ctrl ehcic[CONFIG_USB_MAX_CONTROLLER_COUNT];
 
 #define ALIGN_END_ADDR(type, ptr, size)			\
@@ -190,6 +196,36 @@ 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,
+		100 * 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,
+			HCHALT_TIMEOUT);
+	}
+
+	if (ret)
+		puts("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 +844,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 +915,7 @@ unknown:
 
 int usb_lowlevel_stop(int index)
 {
+	ehci_shutdown(&ehcic[index]);
 	return ehci_hcd_stop(index);
 }
 
-- 
1.7.12.4

[U-Boot] [PATCH v2] usb: ehci: Fix test mode for connected ports

[Simon Glass]

On Tue, Sep 24, 2013 at 11:53 AM, 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.
>
> Signed-off-by: Julius Werner <jwerner@chromium.org>

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH v2] usb: ehci: Fix test mode for connected ports

[Marek Vasut]

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>
> ---

Applied, thanks

Best regards,
Marek Vasut