[U-Boot] [PATCH 0/6] usb: dwc2: add SPLIT transaction support

[U-Boot] [PATCH 0/6] usb: dwc2: add SPLIT transaction support

[Stefan Brüns]

First two patches are preparatory work. 3rd and 4th introduce helper
functions. Patch 5 contains the core changes. Patch 6 is cleanup.

The patch has been tested on RPi 1 B with several full speed and low speed
devices. All devices enumerated successfully, with good output from
"usb info". High speed devices, e.g. the SMSC9514 ethernet and mass storage
devices still work.

Neither CONFIG_DM_USB nor LS/FS devices directly connected to the root hub/hc
have been tested, although both should be handled by the patches.

Stefan Br?ns (6):
  usb: dwc2: avoid out of bounds access
  usb: dwc2: Handle NAK during CONTROL DATA and STATUS stage
  usb: dwc2: determine TT hub address and port for split transactions
  usb: dwc2: add helper function for setting SPLIT HC registers
  usb: dwc2: add support for SPLIT transactions
  usb: dwc2: remove no longer used wait_for_chhltd()

 drivers/usb/host/dwc2.c | 240 +++++++++++++++++++++++++++++++++++-------------
 1 file changed, 178 insertions(+), 62 deletions(-)

-- 
2.1.4

[U-Boot] [PATCH 1/6] usb: dwc2: avoid out of bounds access

[Stefan Brüns]

flush_dcache_range may access data after priv->aligned_buffer end if
len > DWC2_DATA_BUF_SIZE.
memcpy may access data after buffer end if done > 0

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 drivers/usb/host/dwc2.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
index 541c0f9..5ef6deb 100644
--- a/drivers/usb/host/dwc2.c
+++ b/drivers/usb/host/dwc2.c
@@ -823,12 +823,13 @@ int chunk_msg(struct dwc2_priv *priv, struct usb_device *dev,
 		       (*pid << DWC2_HCTSIZ_PID_OFFSET),
 		       &hc_regs->hctsiz);
 
-		if (!in) {
-			memcpy(priv->aligned_buffer, (char *)buffer + done, len);
+		if (!in && xfer_len) {
+			memcpy(priv->aligned_buffer, (char *)buffer + done,
+			       xfer_len);
 
 			flush_dcache_range((unsigned long)priv->aligned_buffer,
 				(unsigned long)((void *)priv->aligned_buffer +
-				roundup(len, ARCH_DMA_MINALIGN)));
+				roundup(xfer_len, ARCH_DMA_MINALIGN)));
 		}
 
 		writel(phys_to_bus((unsigned long)priv->aligned_buffer),
-- 
2.1.4

[U-Boot] [PATCH 1/6] usb: dwc2: avoid out of bounds access

[Marek Vasut]

On Sunday, December 13, 2015 at 05:17:53 AM, Stefan Br?ns wrote:
> flush_dcache_range may access data after priv->aligned_buffer end if
> len > DWC2_DATA_BUF_SIZE.
> memcpy may access data after buffer end if done > 0
> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>

Acked-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/6] usb: dwc2: avoid out of bounds access

[Stephen Warren]

On 12/12/2015 09:17 PM, Stefan Br?ns wrote:
> flush_dcache_range may access data after priv->aligned_buffer end if
> len > DWC2_DATA_BUF_SIZE.
> memcpy may access data after buffer end if done > 0

Acked-by: Stephen Warren <swarren@wwwdotorg.org>

Uggh; icky bug:-(

> @@ -823,12 +823,13 @@ int chunk_msg(struct dwc2_priv *priv, struct usb_device *dev,
>  		       (*pid << DWC2_HCTSIZ_PID_OFFSET),
>  		       &hc_regs->hctsiz);
>  
> -		if (!in) {
> -			memcpy(priv->aligned_buffer, (char *)buffer + done, len);
> +		if (!in && xfer_len) {

Do zero-length memcpy or flush_dcache_range actually cause an issue?

[U-Boot] [PATCH 1/6] usb: dwc2: avoid out of bounds access

[Marek Vasut]

On Wednesday, December 16, 2015 at 03:58:48 AM, Stephen Warren wrote:
> On 12/12/2015 09:17 PM, Stefan Br?ns wrote:
> > flush_dcache_range may access data after priv->aligned_buffer end if
> > len > DWC2_DATA_BUF_SIZE.
> > memcpy may access data after buffer end if done > 0
> 
> Acked-by: Stephen Warren <swarren@wwwdotorg.org>
> 
> Uggh; icky bug:-(
> 
> > @@ -823,12 +823,13 @@ int chunk_msg(struct dwc2_priv *priv, struct
> > usb_device *dev,
> > 
> >  		       (*pid << DWC2_HCTSIZ_PID_OFFSET),
> >  		       &hc_regs->hctsiz);
> > 
> > -		if (!in) {
> > -			memcpy(priv->aligned_buffer, (char *)buffer + done, 
len);
> > +		if (!in && xfer_len) {
> 
> Do zero-length memcpy or flush_dcache_range actually cause an issue?

I believe they should not, based on how they are implemented.

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/6] usb: dwc2: avoid out of bounds access

[Stefan Bruens]

On Wednesday 16 December 2015 11:29:14 Marek Vasut wrote:
> > > +		if (!in && xfer_len) {
> > 
> > Do zero-length memcpy or flush_dcache_range actually cause an issue?
> 
> I believe they should not, based on how they are implemented.

I think that's correct, it is just a minor optimization.

Kind regards,

Stefan

-- 
Stefan Br?ns  /  Bergstra?e 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019
work: +49 2405 49936-424

[U-Boot] [PATCH 2/6] usb: dwc2: Handle NAK during CONTROL DATA and STATUS stage

[Stefan Brüns]

A function is allowed to return NAKs during the DATA stage to control
data flow control. NAKs during the STATUS stage signal the function
is still processing the request.

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 drivers/usb/host/dwc2.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
index 5ef6deb..db3acd4 100644
--- a/drivers/usb/host/dwc2.c
+++ b/drivers/usb/host/dwc2.c
@@ -892,8 +892,8 @@ static int _submit_control_msg(struct dwc2_priv *priv, struct usb_device *dev,
 {
 	int devnum = usb_pipedevice(pipe);
 	int pid, ret, act_len;
-	/* For CONTROL endpoint pid should start with DATA1 */
 	int status_direction;
+	unsigned long timeout;
 
 	if (devnum == priv->root_hub_devnum) {
 		dev->status = 0;
@@ -907,26 +907,37 @@ static int _submit_control_msg(struct dwc2_priv *priv, struct usb_device *dev,
 	if (ret)
 		return ret;
 
+	timeout = get_timer(0) + USB_TIMEOUT_MS(pipe);
 	if (buffer) {
+		/* DATA stage */
 		pid = DWC2_HC_PID_DATA1;
-		ret = chunk_msg(priv, dev, pipe, &pid, usb_pipein(pipe), buffer,
-				len, false);
+		act_len = 0;
+		do {
+			if (get_timer(0) > timeout) {
+				printf("Timeout during CONTROL DATA stage\n");
+				return -ETIMEDOUT;
+			}
+			ret = chunk_msg(priv, dev, pipe, &pid, usb_pipein(pipe),
+					buffer, len, false);
+			act_len += dev->act_len;
+			buffer += dev->act_len;
+			len -= dev->act_len;
+		} while (len && (ret == -EAGAIN));
 		if (ret)
 			return ret;
-		act_len = dev->act_len;
-	} /* End of DATA stage */
-	else
+		status_direction = usb_pipeout(pipe);
+	} else {
+		/* No-data CONTROL always ends with an IN transaction */
+		status_direction = 1;
 		act_len = 0;
+	}
 
 	/* STATUS stage */
-	if ((len == 0) || usb_pipeout(pipe))
-		status_direction = 1;
-	else
-		status_direction = 0;
-
 	pid = DWC2_HC_PID_DATA1;
-	ret = chunk_msg(priv, dev, pipe, &pid, status_direction,
-			priv->status_buffer, 0, false);
+	do {
+		ret = chunk_msg(priv, dev, pipe, &pid, status_direction,
+				priv->status_buffer, 0, false);
+	} while (ret == -EAGAIN);
 	if (ret)
 		return ret;
 
-- 
2.1.4

[U-Boot] [PATCH 2/6] usb: dwc2: Handle NAK during CONTROL DATA and STATUS stage

[Marek Vasut]

On Sunday, December 13, 2015 at 05:17:54 AM, Stefan Br?ns wrote:
> A function is allowed to return NAKs during the DATA stage to control
> data flow control. NAKs during the STATUS stage signal the function
> is still processing the request.
> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> ---
>  drivers/usb/host/dwc2.c | 37 ++++++++++++++++++++++++-------------
>  1 file changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
> index 5ef6deb..db3acd4 100644
> --- a/drivers/usb/host/dwc2.c
> +++ b/drivers/usb/host/dwc2.c
> @@ -892,8 +892,8 @@ static int _submit_control_msg(struct dwc2_priv *priv,
> struct usb_device *dev, {
>  	int devnum = usb_pipedevice(pipe);
>  	int pid, ret, act_len;
> -	/* For CONTROL endpoint pid should start with DATA1 */
>  	int status_direction;
> +	unsigned long timeout;
> 
>  	if (devnum == priv->root_hub_devnum) {
>  		dev->status = 0;
> @@ -907,26 +907,37 @@ static int _submit_control_msg(struct dwc2_priv
> *priv, struct usb_device *dev, if (ret)
>  		return ret;
> 
> +	timeout = get_timer(0) + USB_TIMEOUT_MS(pipe);
>  	if (buffer) {
> +		/* DATA stage */
>  		pid = DWC2_HC_PID_DATA1;
> -		ret = chunk_msg(priv, dev, pipe, &pid, usb_pipein(pipe), buffer,
> -				len, false);
> +		act_len = 0;
> +		do {
> +			if (get_timer(0) > timeout) {
> +				printf("Timeout during CONTROL DATA stage\n");
> +				return -ETIMEDOUT;
> +			}
> +			ret = chunk_msg(priv, dev, pipe, &pid, usb_pipein(pipe),
> +					buffer, len, false);
> +			act_len += dev->act_len;
> +			buffer += dev->act_len;
> +			len -= dev->act_len;
> +		} while (len && (ret == -EAGAIN));

I can't say I'm a big fan of these conditions ^ . Can you rework a bit so it's
more obvious what the code does ? The condition in the while (cond) is really
inobvious at a first glance.

>  		if (ret)
>  			return ret;
> -		act_len = dev->act_len;
> -	} /* End of DATA stage */
> -	else
> +		status_direction = usb_pipeout(pipe);
> +	} else {
> +		/* No-data CONTROL always ends with an IN transaction */
> +		status_direction = 1;
>  		act_len = 0;
> +	}

Otherwise:
Acked-by: Marek Vasut <marex@denx.de>

[U-Boot] [PATCH 2/6] usb: dwc2: Handle NAK during CONTROL DATA and STATUS stage

[Stephen Warren]

On 12/12/2015 09:17 PM, Stefan Br?ns wrote:
> A function is allowed to return NAKs during the DATA stage to control
> data flow control. NAKs during the STATUS stage signal the function
> is still processing the request.

For my own education, do you have a link to the part of the spec that
states that? I'd naively expect the control stage to give a NAK, but
once a control transaction was accepted, the function would have to deal
with it without NAKs? Still, I don't think this change would cause any
issue either way.

> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c

> @@ -907,26 +907,37 @@ static int _submit_control_msg(struct dwc2_priv *priv, struct usb_device *dev,
>  	if (ret)
>  		return ret;
>  
> +	timeout = get_timer(0) + USB_TIMEOUT_MS(pipe);
>  	if (buffer) {
> +		/* DATA stage */

I'd suggest putting that new comment right before the "timeout = ..."
line, since that's the start of DATA stage processing.

If you're adding comments for the stages, perhaps add one at the start
of the CONTROL stage too?

>  		pid = DWC2_HC_PID_DATA1;
> -		ret = chunk_msg(priv, dev, pipe, &pid, usb_pipein(pipe), buffer,
> -				len, false);
> +		act_len = 0;

I don't think you need that assignment because...

> +		do {
> +			if (get_timer(0) > timeout) {
> +				printf("Timeout during CONTROL DATA stage\n");
> +				return -ETIMEDOUT;
> +			}
> +			ret = chunk_msg(priv, dev, pipe, &pid, usb_pipein(pipe),
> +					buffer, len, false);
> +			act_len += dev->act_len;
> +			buffer += dev->act_len;
> +			len -= dev->act_len;

Shouldn't those all be = not += or -=-, just like in the original code?
There's no chunking loop here, so the entire length either happens in
one go or not at all.

>  	pid = DWC2_HC_PID_DATA1;
> -	ret = chunk_msg(priv, dev, pipe, &pid, status_direction,
> -			priv->status_buffer, 0, false);
> +	do {
> +		ret = chunk_msg(priv, dev, pipe, &pid, status_direction,
> +				priv->status_buffer, 0, false);
> +	} while (ret == -EAGAIN);
>  	if (ret)
>  		return ret;

Shouldn't that last loop have a timeout too?

[U-Boot] [PATCH 2/6] usb: dwc2: Handle NAK during CONTROL DATA and STATUS stage

[Stefan Bruens]

On Tuesday 15 December 2015 20:07:26 Stephen Warren wrote:
> On 12/12/2015 09:17 PM, Stefan Br?ns wrote:
> > A function is allowed to return NAKs during the DATA stage to control
> > data flow control. NAKs during the STATUS stage signal the function
> > is still processing the request.
> 
> For my own education, do you have a link to the part of the spec that
> states that? I'd naively expect the control stage to give a NAK, but
> once a control transaction was accepted, the function would have to deal
> with it without NAKs? Still, I don't think this change would cause any
> issue either way.

"8.5.3 Control Transfers"
"The Data stage, if present, of a control transfer consists of one or more IN 
or OUT transactions and follows the same protocol rules as bulk transfers."
This can also be inferred from the flow charts/state diagrams which state NAKs 
and stalls are not allowed for for "control *setup* transaction" (emphasize 
mine), e.g. Figure 8-31.

"8.5.3.1 Reporting Status Results"
"NAK indicates that the function is still processing the command and that the 
host should continue the Status stage."

I have at least one USB LS mouse which responds with NAKs during control 
transfers, Sigrok LA traces available here:
http://sigrok.org/gitweb/?p=sigrok-dumps.git;a=tree;f=usb/setup

> > diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
> > 
> > @@ -907,26 +907,37 @@ static int _submit_control_msg(struct dwc2_priv
> > *priv, struct usb_device *dev,> 
> >  	if (ret)
> >  	
> >  		return ret;
> > 
> > +	timeout = get_timer(0) + USB_TIMEOUT_MS(pipe);
> > 
> >  	if (buffer) {
> > 
> > +		/* DATA stage */
> 
> I'd suggest putting that new comment right before the "timeout = ..."
> line, since that's the start of DATA stage processing.
> 
> If you're adding comments for the stages, perhaps add one at the start
> of the CONTROL stage too?

Good idea, will do.
 
> >  		pid = DWC2_HC_PID_DATA1;
> > 
> > -		ret = chunk_msg(priv, dev, pipe, &pid, usb_pipein(pipe), buffer,
> > -				len, false);
> > +		act_len = 0;
> 
> I don't think you need that assignment because...
> 
> > +		do {
> > +			if (get_timer(0) > timeout) {
> > +				printf("Timeout during CONTROL DATA stage\n");
> > +				return -ETIMEDOUT;
> > +			}
> > +			ret = chunk_msg(priv, dev, pipe, &pid, usb_pipein(pipe),
> > +					buffer, len, false);
> > +			act_len += dev->act_len;
> > +			buffer += dev->act_len;
> > +			len -= dev->act_len;
> 
> Shouldn't those all be = not += or -=-, just like in the original code?
> There's no chunking loop here, so the entire length either happens in
> one go or not at all.

No, as each NAK will cause a return from chunk_msg. I see e.g. the following 
interrupt flags in combination with CHHLTD (for GET_DEVICE_DESCRIPTOR):
- SETUP (SSPLIT) -> ACK
  SETUP (CSPLIT) -> NYET NYET ACK
- DATA IN (SSPLIT) > ACK
  DATA IN (CSPLIT) > NYET NYET NACK
- DATA IN (SSPLIT) > ACK
  DATA IN (CSPLIT) > NYET NYET ACK
- DATA IN (SSPLIT) > ACK
  DATA IN (CSPLIT) > NYET NYET NACK
- DATA IN (SSPLIT) > ACK
  DATA IN (CSPLIT) > NYET NYET ACK
- STATUS (SSPLIT) -> ACK
  STATUS (CSPLIT) -> NYET NYET ACK

On the first DATA-IN ACK, 8 bytes are returned, the second returns the final 
9th byte.

The NAK handling could be moved into the loop in chunk_msg, but this would 
break INTERRUPT transactions.

A different possibility is to move the timeout check into the chunk_msg loop, 
i.e.
---
xfer_timeout = get_timer(0) + USB_TIMEOUT_MS(pipe);
do {
  ...
  ret = wait_for_bit(CHHLTD, timeout=1ms)
  if (ret == -EINTR)
     break;
  if (get_timer(0) > xfer_timeout) {
     printf("Timeout\n");
     ret = -ETIMEDOUT;
     break;
  }
  ...
} while ((done < len) && !stop_transfer);
---

This would also simplify both the INTERRUPT and CONTROL submit functions, and 
BULK submit would finally honour the specified timeout.


> >  	pid = DWC2_HC_PID_DATA1;
> > 
> > -	ret = chunk_msg(priv, dev, pipe, &pid, status_direction,
> > -			priv->status_buffer, 0, false);
> > +	do {
> > +		ret = chunk_msg(priv, dev, pipe, &pid, status_direction,
> > +				priv->status_buffer, 0, false);
> > +	} while (ret == -EAGAIN);
> > 
> >  	if (ret)
> >  	
> >  		return ret;
> 
> Shouldn't that last loop have a timeout too?

Correct, but see above.

Kind regards,

Stefan

-- 
Stefan Br?ns  /  Bergstra?e 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019
work: +49 2405 49936-424

[U-Boot] [PATCH 3/6] usb: dwc2: determine TT hub address and port for split transactions

[Stefan Brüns]

Start split and complete split tokens need the hub address and the
downstream port of the first HS hub (device view).
Code copied from host/ehci_hcd.c

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 drivers/usb/host/dwc2.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
index db3acd4..2d97546 100644
--- a/drivers/usb/host/dwc2.c
+++ b/drivers/usb/host/dwc2.c
@@ -404,6 +404,66 @@ static void dwc_otg_core_init(struct dwc2_core_regs *regs)
 }
 
 /*
+ * Searches for the first HS hub above the given device. If a
+ * HS hub is found, the hub address and the port the device is
+ * connected to is return, as required for SPLIT transactions
+ *
+ * @param: udev full speed or low speed device
+ */
+#ifdef CONFIG_DM_USB
+static void dwc_find_hub_address_port(struct usb_device *udev,
+				      uint8_t *hub_address, uint8_t *hub_port)
+{
+	struct udevice *parent;
+	struct usb_device *uparent, *ttdev;
+
+	/*
+	 * When called from usb-uclass.c: usb_scan_device() udev->dev points
+	 * to the parent udevice, not the actual udevice belonging to the
+	 * udev as the device is not instantiated yet. So when searching
+	 * for the first usb-2 parent start with udev->dev not
+	 * udev->dev->parent .
+	 */
+	ttdev = udev;
+	parent = udev->dev;
+	uparent = dev_get_parent_priv(parent);
+
+	while (uparent->speed != USB_SPEED_HIGH) {
+		struct udevice *dev = parent;
+
+		if (device_get_uclass_id(dev->parent) != UCLASS_USB_HUB) {
+			printf("dwc2: Error cannot find high speed parent of usb-1 device\n");
+			*hub_address = *hub_port = 0;
+			return;
+		}
+
+		ttdev = dev_get_parent_priv(dev);
+		parent = dev->parent;
+		uparent = dev_get_parent_priv(parent);
+	}
+	*hub_address = uparent->devnum;
+	*hub_port = ttdev->portnr;
+}
+#else
+static void dwc_find_hub_address_port(struct usb_device *udev,
+				      uint8_t *hub_address, uint8_t *hub_port)
+{
+	/* Find out the nearest parent which is high speed */
+	while (udev->parent->parent != NULL)
+		if (udev->parent->speed != USB_SPEED_HIGH) {
+			udev = udev->parent;
+		} else {
+			*hub_address = udev->parent->devnum;
+			*hub_port = udev->portnr;
+			return;
+		}
+
+	printf("dwc2: Error cannot find high speed parent of usb-1 device\n");
+	*hub_address = *hub_port = 0;
+}
+#endif
+
+/*
  * Prepares a host channel for transferring packets to/from a specific
  * endpoint. The HCCHARn register is set up with the characteristics specified
  * in _hc. Host channel interrupts that may need to be serviced while this
-- 
2.1.4

[U-Boot] [PATCH 3/6] usb: dwc2: determine TT hub address and port for split transactions

[Marek Vasut]

On Sunday, December 13, 2015 at 05:17:55 AM, Stefan Br?ns wrote:
> Start split and complete split tokens need the hub address and the
> downstream port of the first HS hub (device view).
> Code copied from host/ehci_hcd.c
> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>

Acked-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

[U-Boot] [PATCH 3/6] usb: dwc2: determine TT hub address and port for split transactions

[Stephen Warren]

On 12/12/2015 09:17 PM, Stefan Br?ns wrote:
> Start split and complete split tokens need the hub address and the
> downstream port of the first HS hub (device view).
> Code copied from host/ehci_hcd.c

Rather than cut/paste this, shouldn't you create a common function that
can be shared between all users?

[U-Boot] [PATCH 3/6] usb: dwc2: determine TT hub address and port for split transactions

[Stefan Bruens]

On Tuesday 15 December 2015 20:17:32 Stephen Warren wrote:
> On 12/12/2015 09:17 PM, Stefan Br?ns wrote:
> > Start split and complete split tokens need the hub address and the
> > downstream port of the first HS hub (device view).
> > Code copied from host/ehci_hcd.c
> 
> Rather than cut/paste this, shouldn't you create a common function that
> can be shared between all users?

Good idea, as the code is also duplicated in musb-new.

Any suggestions where to put the new function? For CONFIG_DM_USB there is 
usb_uclass.c. Move it into usb_uclass.c and leave the non DM variants in place 
(same name, but declared static)?

Kind regards,

Stefan

-- 
Stefan Br?ns  /  Bergstra?e 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019
work: +49 2405 49936-424

[U-Boot] [PATCH] usb: Move determination of TT hub address/port into seperate function

[Stefan Brüns]

Start split and complete split tokens need the hub address and the
downstream port of the first HS hub (device view).

The core of the function was duplicated in both host/ehci_hcd and
musb-new/usb-compat.h.

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
Obsoletes [PATCH 3/6] usb: dwc2: determine TT hub address and port for split transactions

 common/usb.c                      | 54 +++++++++++++++++++++++++++++++++++++++
 drivers/usb/host/ehci-hcd.c       | 50 ++++--------------------------------
 drivers/usb/musb-new/musb_host.c  |  8 +++---
 drivers/usb/musb-new/usb-compat.h | 53 --------------------------------------
 include/usb.h                     | 10 ++++++++
 5 files changed, 74 insertions(+), 101 deletions(-)

diff --git a/common/usb.c b/common/usb.c
index 700bfc3..1d0a151 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -1200,4 +1200,58 @@ bool usb_device_has_child_on_port(struct usb_device *parent, int port)
 #endif
 }
 
+#ifdef CONFIG_DM_USB
+void usb_find_hub_address_port(struct usb_device *udev,
+			       uint8_t *hub_address, uint8_t *hub_port)
+{
+	struct udevice *parent;
+	struct usb_device *uparent, *ttdev;
+
+	/*
+	 * When called from usb-uclass.c: usb_scan_device() udev->dev points
+	 * to the parent udevice, not the actual udevice belonging to the
+	 * udev as the device is not instantiated yet. So when searching
+	 * for the first usb-2 parent start with udev->dev not
+	 * udev->dev->parent .
+	 */
+	ttdev = udev;
+	parent = udev->dev;
+	uparent = dev_get_parent_priv(parent);
+
+	while (uparent->speed != USB_SPEED_HIGH) {
+		struct udevice *dev = parent;
+
+		if (device_get_uclass_id(dev->parent) != UCLASS_USB_HUB) {
+			printf("Error: Cannot find high speed parent of usb-1 device\n");
+			*hub_address = *hub_port = 0;
+			return;
+		}
+
+		ttdev = dev_get_parent_priv(dev);
+		parent = dev->parent;
+		uparent = dev_get_parent_priv(parent);
+	}
+	*hub_address = uparent->devnum;
+	*hub_port = ttdev->portnr;
+}
+#else
+void usb_find_hub_address_port(struct usb_device *udev,
+			       uint8_t *hub_address, uint8_t *hub_port)
+{
+	/* Find out the nearest parent which is high speed */
+	while (udev->parent->parent != NULL)
+		if (udev->parent->speed != USB_SPEED_HIGH) {
+			udev = udev->parent;
+		} else {
+			*hub_address = udev->parent->devnum;
+			*hub_port = udev->portnr;
+			return;
+		}
+
+	printf("Error: Cannot find high speed parent of usb-1 device\n");
+	*hub_address = *hub_port = 0;
+}
+#endif
+
+
 /* EOF */
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index c85dbce..0a279e7 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -279,56 +279,16 @@ static inline u8 ehci_encode_speed(enum usb_device_speed speed)
 static void ehci_update_endpt2_dev_n_port(struct usb_device *udev,
 					  struct QH *qh)
 {
-	struct usb_device *ttdev;
-	int parent_devnum;
+	uint8_t portnr = 0;
+	uint8_t hubaddr = 0;
 
 	if (udev->speed != USB_SPEED_LOW && udev->speed != USB_SPEED_FULL)
 		return;
 
-	/*
-	 * For full / low speed devices we need to get the devnum and portnr of
-	 * the tt, so of the first upstream usb-2 hub, there may be usb-1 hubs
-	 * in the tree before that one!
-	 */
-#ifdef CONFIG_DM_USB
-	/*
-	 * When called from usb-uclass.c: usb_scan_device() udev->dev points
-	 * to the parent udevice, not the actual udevice belonging to the
-	 * udev as the device is not instantiated yet. So when searching
-	 * for the first usb-2 parent start with udev->dev not
-	 * udev->dev->parent .
-	 */
-	struct udevice *parent;
-	struct usb_device *uparent;
-
-	ttdev = udev;
-	parent = udev->dev;
-	uparent = dev_get_parent_priv(parent);
-
-	while (uparent->speed != USB_SPEED_HIGH) {
-		struct udevice *dev = parent;
-
-		if (device_get_uclass_id(dev->parent) != UCLASS_USB_HUB) {
-			printf("ehci: Error cannot find high-speed parent of usb-1 device\n");
-			return;
-		}
-
-		ttdev = dev_get_parent_priv(dev);
-		parent = dev->parent;
-		uparent = dev_get_parent_priv(parent);
-	}
-	parent_devnum = uparent->devnum;
-#else
-	ttdev = udev;
-	while (ttdev->parent && ttdev->parent->speed != USB_SPEED_HIGH)
-		ttdev = ttdev->parent;
-	if (!ttdev->parent)
-		return;
-	parent_devnum = ttdev->parent->devnum;
-#endif
+	usb_find_hub_address_port(udev, &hubaddr, &portnr)
 
-	qh->qh_endpt2 |= cpu_to_hc32(QH_ENDPT2_PORTNUM(ttdev->portnr) |
-				     QH_ENDPT2_HUBADDR(parent_devnum));
+	qh->qh_endpt2 |= cpu_to_hc32(QH_ENDPT2_PORTNUM(portnr) |
+				     QH_ENDPT2_HUBADDR(hubaddr));
 }
 
 static int
diff --git a/drivers/usb/musb-new/musb_host.c b/drivers/usb/musb-new/musb_host.c
index 40b9c66..352fa8c 100644
--- a/drivers/usb/musb-new/musb_host.c
+++ b/drivers/usb/musb-new/musb_host.c
@@ -2092,9 +2092,11 @@ int musb_urb_enqueue(
 			}
 #else
 			if (tt_needed(musb, urb->dev)) {
-				u16 hub_port = find_tt(urb->dev);
-				qh->h_addr_reg = (u8) (hub_port >> 8);
-				qh->h_port_reg = (u8) (hub_port & 0xff);
+				uint8_t portnr = 0;
+				uint8_t hubaddr = 0;
+				usb_find_hub_address_port(udev, &hubaddr, &portnr)
+				qh->h_addr_reg = hubaddr;
+				qh->h_port_reg = portnr;
 			}
 #endif
 		}
diff --git a/drivers/usb/musb-new/usb-compat.h b/drivers/usb/musb-new/usb-compat.h
index 1c41e2a..760bd78 100644
--- a/drivers/usb/musb-new/usb-compat.h
+++ b/drivers/usb/musb-new/usb-compat.h
@@ -68,38 +68,6 @@ static inline int usb_hcd_unmap_urb_for_dma(struct usb_hcd *hcd,
 }
 
 #ifdef CONFIG_DM_USB
-static inline u16 find_tt(struct usb_device *udev)
-{
-	struct udevice *parent;
-	struct usb_device *uparent, *ttdev;
-
-	/*
-	 * When called from usb-uclass.c: usb_scan_device() udev->dev points
-	 * to the parent udevice, not the actual udevice belonging to the
-	 * udev as the device is not instantiated yet. So when searching
-	 * for the first usb-2 parent start with udev->dev not
-	 * udev->dev->parent .
-	 */
-	ttdev = udev;
-	parent = udev->dev;
-	uparent = dev_get_parent_priv(parent);
-
-	while (uparent->speed != USB_SPEED_HIGH) {
-		struct udevice *dev = parent;
-
-		if (device_get_uclass_id(dev->parent) != UCLASS_USB_HUB) {
-			printf("musb: Error cannot find high speed parent of usb-1 device\n");
-			return 0;
-		}
-
-		ttdev = dev_get_parent_priv(dev);
-		parent = dev->parent;
-		uparent = dev_get_parent_priv(parent);
-	}
-
-	return (uparent->devnum << 8) | (ttdev->portnr - 1);
-}
-
 static inline struct usb_device *usb_dev_get_parent(struct usb_device *udev)
 {
 	struct udevice *parent = udev->dev->parent;
@@ -129,27 +97,6 @@ static inline struct usb_device *usb_dev_get_parent(struct usb_device *udev)
 	return NULL;
 }
 #else
-static inline u16 find_tt(struct usb_device *dev)
-{
-	u8 chid;
-	u8 hub;
-
-	/* Find out the nearest parent which is high speed */
-	while (dev->parent->parent != NULL)
-		if (dev->parent->speed != USB_SPEED_HIGH)
-			dev = dev->parent;
-		else
-			break;
-
-	/* determine the port address at that hub */
-	hub = dev->parent->devnum;
-	for (chid = 0; chid < USB_MAXCHILDREN; chid++)
-		if (dev->parent->children[chid] == dev)
-			break;
-
-	return (hub << 8) | chid;
-}
-
 static inline struct usb_device *usb_dev_get_parent(struct usb_device *dev)
 {
 	return dev->parent;
diff --git a/include/usb.h b/include/usb.h
index 55b9268..6e12876 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -874,6 +874,16 @@ int legacy_hub_port_reset(struct usb_device *dev, int port,
 
 int hub_port_reset(struct udevice *dev, int port, unsigned short *portstat);
 
+/*
+ * Searches for the first HS hub above the given device. If a
+ * HS hub is found, the hub address and the port the device is
+ * connected to is return, as required for SPLIT transactions
+ *
+ * @param: udev full speed or low speed device
+ */
+void usb_find_hub_address_port(struct usb_device *udev,
+			       uint8_t *hub_address, uint8_t *hub_port);
+
 /**
  * usb_alloc_new_device() - Allocate a new device
  *
-- 
2.1.4

[U-Boot] [PATCH] usb: Move determination of TT hub address/port into seperate function

[Marek Vasut]

On Friday, December 18, 2015 at 02:11:41 AM, Stefan Br?ns wrote:
> Start split and complete split tokens need the hub address and the
> downstream port of the first HS hub (device view).
> 
> The core of the function was duplicated in both host/ehci_hcd and
> musb-new/usb-compat.h.
> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> ---
> Obsoletes [PATCH 3/6] usb: dwc2: determine TT hub address and port for
> split transactions
> 
>  common/usb.c                      | 54
> +++++++++++++++++++++++++++++++++++++++ drivers/usb/host/ehci-hcd.c      
> | 50 ++++-------------------------------- drivers/usb/musb-new/musb_host.c
>  |  8 +++---
>  drivers/usb/musb-new/usb-compat.h | 53
> -------------------------------------- include/usb.h                     |
> 10 ++++++++
>  5 files changed, 74 insertions(+), 101 deletions(-)
> 
> diff --git a/common/usb.c b/common/usb.c
> index 700bfc3..1d0a151 100644
> --- a/common/usb.c
> +++ b/common/usb.c
> @@ -1200,4 +1200,58 @@ bool usb_device_has_child_on_port(struct usb_device
> *parent, int port) #endif
>  }
> 
> +#ifdef CONFIG_DM_USB
> +void usb_find_hub_address_port(struct usb_device *udev,
> +			       uint8_t *hub_address, uint8_t *hub_port)
> +{
> +	struct udevice *parent;
> +	struct usb_device *uparent, *ttdev;
> +
> +	/*
> +	 * When called from usb-uclass.c: usb_scan_device() udev->dev points
> +	 * to the parent udevice, not the actual udevice belonging to the
> +	 * udev as the device is not instantiated yet. So when searching
> +	 * for the first usb-2 parent start with udev->dev not
> +	 * udev->dev->parent .
> +	 */
> +	ttdev = udev;
> +	parent = udev->dev;
> +	uparent = dev_get_parent_priv(parent);
> +
> +	while (uparent->speed != USB_SPEED_HIGH) {
> +		struct udevice *dev = parent;
> +
> +		if (device_get_uclass_id(dev->parent) != UCLASS_USB_HUB) {
> +			printf("Error: Cannot find high speed parent of usb-1 
device\n");
> +			*hub_address = *hub_port = 0;

Please avoid this construct: foo = bar = 1234; , it's really confusing and error 
prone.

Otherwise looks good!

Reviewed-by: Marek Vasut <marex@denx.de>

[U-Boot] [PATCH] usb: Move determination of TT hub address/port into seperate function

[Hans de Goede]

Hi,

On 18-12-15 02:11, Stefan Br?ns wrote:
> Start split and complete split tokens need the hub address and the
> downstream port of the first HS hub (device view).
>
> The core of the function was duplicated in both host/ehci_hcd and
> musb-new/usb-compat.h.
>
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>

Thanks for working on this, I think I've spotted one small bug though, see comments inline.

> ---
> Obsoletes [PATCH 3/6] usb: dwc2: determine TT hub address and port for split transactions
>
>   common/usb.c                      | 54 +++++++++++++++++++++++++++++++++++++++
>   drivers/usb/host/ehci-hcd.c       | 50 ++++--------------------------------
>   drivers/usb/musb-new/musb_host.c  |  8 +++---
>   drivers/usb/musb-new/usb-compat.h | 53 --------------------------------------
>   include/usb.h                     | 10 ++++++++
>   5 files changed, 74 insertions(+), 101 deletions(-)
>
> diff --git a/common/usb.c b/common/usb.c
> index 700bfc3..1d0a151 100644
> --- a/common/usb.c
> +++ b/common/usb.c
> @@ -1200,4 +1200,58 @@ bool usb_device_has_child_on_port(struct usb_device *parent, int port)
>   #endif
>   }
>
> +#ifdef CONFIG_DM_USB
> +void usb_find_hub_address_port(struct usb_device *udev,
> +			       uint8_t *hub_address, uint8_t *hub_port)
> +{
> +	struct udevice *parent;
> +	struct usb_device *uparent, *ttdev;
> +
> +	/*
> +	 * When called from usb-uclass.c: usb_scan_device() udev->dev points
> +	 * to the parent udevice, not the actual udevice belonging to the
> +	 * udev as the device is not instantiated yet. So when searching
> +	 * for the first usb-2 parent start with udev->dev not
> +	 * udev->dev->parent .
> +	 */
> +	ttdev = udev;
> +	parent = udev->dev;
> +	uparent = dev_get_parent_priv(parent);
> +
> +	while (uparent->speed != USB_SPEED_HIGH) {
> +		struct udevice *dev = parent;
> +
> +		if (device_get_uclass_id(dev->parent) != UCLASS_USB_HUB) {
> +			printf("Error: Cannot find high speed parent of usb-1 device\n");
> +			*hub_address = *hub_port = 0;
> +			return;
> +		}
> +
> +		ttdev = dev_get_parent_priv(dev);
> +		parent = dev->parent;
> +		uparent = dev_get_parent_priv(parent);
> +	}
> +	*hub_address = uparent->devnum;
> +	*hub_port = ttdev->portnr;

This is correct to replace the ehci-code, but lets take a closer look at
the musb-new code below.


> +}
> +#else
> +void usb_find_hub_address_port(struct usb_device *udev,
> +			       uint8_t *hub_address, uint8_t *hub_port)
> +{
> +	/* Find out the nearest parent which is high speed */
> +	while (udev->parent->parent != NULL)
> +		if (udev->parent->speed != USB_SPEED_HIGH) {
> +			udev = udev->parent;
> +		} else {
> +			*hub_address = udev->parent->devnum;
> +			*hub_port = udev->portnr;

Same here.

> +			return;
> +		}
> +
> +	printf("Error: Cannot find high speed parent of usb-1 device\n");
> +	*hub_address = *hub_port = 0;
> +}
> +#endif
> +
> +
>   /* EOF */
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index c85dbce..0a279e7 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -279,56 +279,16 @@ static inline u8 ehci_encode_speed(enum usb_device_speed speed)
>   static void ehci_update_endpt2_dev_n_port(struct usb_device *udev,
>   					  struct QH *qh)
>   {
> -	struct usb_device *ttdev;
> -	int parent_devnum;
> +	uint8_t portnr = 0;
> +	uint8_t hubaddr = 0;
>
>   	if (udev->speed != USB_SPEED_LOW && udev->speed != USB_SPEED_FULL)
>   		return;
>
> -	/*
> -	 * For full / low speed devices we need to get the devnum and portnr of
> -	 * the tt, so of the first upstream usb-2 hub, there may be usb-1 hubs
> -	 * in the tree before that one!
> -	 */
> -#ifdef CONFIG_DM_USB
> -	/*
> -	 * When called from usb-uclass.c: usb_scan_device() udev->dev points
> -	 * to the parent udevice, not the actual udevice belonging to the
> -	 * udev as the device is not instantiated yet. So when searching
> -	 * for the first usb-2 parent start with udev->dev not
> -	 * udev->dev->parent .
> -	 */
> -	struct udevice *parent;
> -	struct usb_device *uparent;
> -
> -	ttdev = udev;
> -	parent = udev->dev;
> -	uparent = dev_get_parent_priv(parent);
> -
> -	while (uparent->speed != USB_SPEED_HIGH) {
> -		struct udevice *dev = parent;
> -
> -		if (device_get_uclass_id(dev->parent) != UCLASS_USB_HUB) {
> -			printf("ehci: Error cannot find high-speed parent of usb-1 device\n");
> -			return;
> -		}
> -
> -		ttdev = dev_get_parent_priv(dev);
> -		parent = dev->parent;
> -		uparent = dev_get_parent_priv(parent);
> -	}
> -	parent_devnum = uparent->devnum;
> -#else
> -	ttdev = udev;
> -	while (ttdev->parent && ttdev->parent->speed != USB_SPEED_HIGH)
> -		ttdev = ttdev->parent;
> -	if (!ttdev->parent)
> -		return;
> -	parent_devnum = ttdev->parent->devnum;
> -#endif
> +	usb_find_hub_address_port(udev, &hubaddr, &portnr)
>
> -	qh->qh_endpt2 |= cpu_to_hc32(QH_ENDPT2_PORTNUM(ttdev->portnr) |
> -				     QH_ENDPT2_HUBADDR(parent_devnum));
> +	qh->qh_endpt2 |= cpu_to_hc32(QH_ENDPT2_PORTNUM(portnr) |
> +				     QH_ENDPT2_HUBADDR(hubaddr));
>   }
>
>   static int

<note moved the changes to musb_host.c from here to behind other changes to make
  my comments easier to read>

> diff --git a/drivers/usb/musb-new/usb-compat.h b/drivers/usb/musb-new/usb-compat.h
> index 1c41e2a..760bd78 100644
> --- a/drivers/usb/musb-new/usb-compat.h
> +++ b/drivers/usb/musb-new/usb-compat.h
> @@ -68,38 +68,6 @@ static inline int usb_hcd_unmap_urb_for_dma(struct usb_hcd *hcd,
>   }
>
>   #ifdef CONFIG_DM_USB
> -static inline u16 find_tt(struct usb_device *udev)
> -{
> -	struct udevice *parent;
> -	struct usb_device *uparent, *ttdev;
> -
> -	/*
> -	 * When called from usb-uclass.c: usb_scan_device() udev->dev points
> -	 * to the parent udevice, not the actual udevice belonging to the
> -	 * udev as the device is not instantiated yet. So when searching
> -	 * for the first usb-2 parent start with udev->dev not
> -	 * udev->dev->parent .
> -	 */
> -	ttdev = udev;
> -	parent = udev->dev;
> -	uparent = dev_get_parent_priv(parent);
> -
> -	while (uparent->speed != USB_SPEED_HIGH) {
> -		struct udevice *dev = parent;
> -
> -		if (device_get_uclass_id(dev->parent) != UCLASS_USB_HUB) {
> -			printf("musb: Error cannot find high speed parent of usb-1 device\n");
> -			return 0;
> -		}
> -
> -		ttdev = dev_get_parent_priv(dev);
> -		parent = dev->parent;
> -		uparent = dev_get_parent_priv(parent);
> -	}
> -
> -	return (uparent->devnum << 8) | (ttdev->portnr - 1);
> -}
> -

Here in the original musb_new code "ttdev->portnr - 1" is used rather then "just "ttdev->portnr".

>   static inline struct usb_device *usb_dev_get_parent(struct usb_device *udev)
>   {
>   	struct udevice *parent = udev->dev->parent;
> @@ -129,27 +97,6 @@ static inline struct usb_device *usb_dev_get_parent(struct usb_device *udev)
>   	return NULL;
>   }
>   #else
> -static inline u16 find_tt(struct usb_device *dev)
> -{
> -	u8 chid;
> -	u8 hub;
> -
> -	/* Find out the nearest parent which is high speed */
> -	while (dev->parent->parent != NULL)
> -		if (dev->parent->speed != USB_SPEED_HIGH)
> -			dev = dev->parent;
> -		else
> -			break;
> -
> -	/* determine the port address at that hub */
> -	hub = dev->parent->devnum;
> -	for (chid = 0; chid < USB_MAXCHILDREN; chid++)
> -		if (dev->parent->children[chid] == dev)
> -			break;
> -
> -	return (hub << 8) | chid;
> -}
> -

And here for the non DM code the same in a convoluted way (the index is
returned which starts at 0, where as portnr-s start at 1).

>   static inline struct usb_device *usb_dev_get_parent(struct usb_device *dev)
>   {
>   	return dev->parent;
 > diff --git a/drivers/usb/musb-new/musb_host.c b/drivers/usb/musb-new/musb_host.c
 > index 40b9c66..352fa8c 100644
 > --- a/drivers/usb/musb-new/musb_host.c
 > +++ b/drivers/usb/musb-new/musb_host.c
 > @@ -2092,9 +2092,11 @@ int musb_urb_enqueue(
 > }
 > #else
 > if (tt_needed(musb, urb->dev)) {
 > - u16 hub_port = find_tt(urb->dev);
 > - qh->h_addr_reg = (u8) (hub_port >> 8);
 > - qh->h_port_reg = (u8) (hub_port & 0xff);
 > + uint8_t portnr = 0;
 > + uint8_t hubaddr = 0;
 > + usb_find_hub_address_port(udev, &hubaddr, &portnr)
 > + qh->h_addr_reg = hubaddr;
 > + qh->h_port_reg = portnr;
 > }
 > #endif
 > }

So here "qh->h_port_reg = portnr" should be "qh->h_port_reg = portnr - 1"
to compensate for the portnr returned by the new usb_find_hub_address_port
being one higher then the one returned by the old find_tt helper.

> diff --git a/include/usb.h b/include/usb.h
> index 55b9268..6e12876 100644
> --- a/include/usb.h
> +++ b/include/usb.h
> @@ -874,6 +874,16 @@ int legacy_hub_port_reset(struct usb_device *dev, int port,
>
>   int hub_port_reset(struct udevice *dev, int port, unsigned short *portstat);
>
> +/*
> + * Searches for the first HS hub above the given device. If a
> + * HS hub is found, the hub address and the port the device is
> + * connected to is return, as required for SPLIT transactions
> + *
> + * @param: udev full speed or low speed device
> + */
> +void usb_find_hub_address_port(struct usb_device *udev,
> +			       uint8_t *hub_address, uint8_t *hub_port);
> +
>   /**
>    * usb_alloc_new_device() - Allocate a new device
>    *
>

This function is usb-2 controller / hub specific, maybe rename it to:
"usb_find_usb2_hub_address_port" to reflect this ?

Regards,

Hans

[U-Boot] [PATCH] usb: Move determination of TT hub address/port into seperate function

[Stefan Bruens]

On Friday 18 December 2015 11:00:19 Hans de Goede wrote:
> Hi,
> 
> On 18-12-15 02:11, Stefan Br?ns wrote:
> > Start split and complete split tokens need the hub address and the
> > downstream port of the first HS hub (device view).
> > 
> > The core of the function was duplicated in both host/ehci_hcd and
> > musb-new/usb-compat.h.
> > 
> > Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> 
> Thanks for working on this, I think I've spotted one small bug though, see
> comments inline.

Ah, sorry for not mentioning this.

Yes, this changes the musb code, but this was on purpose. Rationale:

The ifdef'ed Linux kernel code uses the 1 based port number, whereas U-Boot 
puts a 0 based port number into the register. The reason the 0 based port 
number apparently works can probably be taken from the USB 2.0 spec:

8.4.2.2 Start-Split Transaction Token
... The host must correctly set the port field for single and multiple TT hub 
implementations. A single TT hub implementation *may ignore* the port field.

Actually, as far as I unterstand, a multi TT hub defaults to single TT 
(bAlternateSetting: 0) until switched via SetInterface, so even "port 42" 
would work.

I have somewhat verified this assumption by hardcoding the port number and 
split transactions still work. Used hubs are the RPi onboard SMC9514 and an 
external "05e3:0608 Genesys Logic, Inc. USB-2.0 4-Port HUB". The former is a 
multi TT hub, the latter single TT only.

I have no board with musb, but I think a 0 based port number is wrong.
 
> This function is usb-2 controller / hub specific, maybe rename it to:
> "usb_find_usb2_hub_address_port" to reflect this ?

Yes, sounds reasonable.

Kind regards,

Stefan
	


-- 
Stefan Br?ns  /  Bergstra?e 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019
work: +49 2405 49936-424

[U-Boot] [PATCH] usb: Move determination of TT hub address/port into seperate function

[Hans de Goede]

Hi,

On 19-12-15 18:17, Stefan Bruens wrote:
> On Friday 18 December 2015 11:00:19 Hans de Goede wrote:
>> Hi,
>>
>> On 18-12-15 02:11, Stefan Br?ns wrote:
>>> Start split and complete split tokens need the hub address and the
>>> downstream port of the first HS hub (device view).
>>>
>>> The core of the function was duplicated in both host/ehci_hcd and
>>> musb-new/usb-compat.h.
>>>
>>> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
>>
>> Thanks for working on this, I think I've spotted one small bug though, see
>> comments inline.
>
> Ah, sorry for not mentioning this.
>
> Yes, this changes the musb code, but this was on purpose.

IMHO that is not how this should be dealt with, if what we're currently
doing for musb is wrong (I do not know if it is), then fixing this does
not belong in a patch which is only moving code around. Such a fix
clearly belongs in a separate follow-up patch, and then you can use
everything you've just typed:

 > Rationale:
>
> The ifdef'ed Linux kernel code uses the 1 based port number, whereas U-Boot
> puts a 0 based port number into the register. The reason the 0 based port
> number apparently works can probably be taken from the USB 2.0 spec:
>
> 8.4.2.2 Start-Split Transaction Token
> ... The host must correctly set the port field for single and multiple TT hub
> implementations. A single TT hub implementation *may ignore* the port field.
>
> Actually, as far as I unterstand, a multi TT hub defaults to single TT
> (bAlternateSetting: 0) until switched via SetInterface, so even "port 42"
> would work.
>
> I have somewhat verified this assumption by hardcoding the port number and
> split transactions still work. Used hubs are the RPi onboard SMC9514 and an
> external "05e3:0608 Genesys Logic, Inc. USB-2.0 4-Port HUB". The former is a
> multi TT hub, the latter single TT only.

As a commit msg for such a separate patch. As a general rule of thumb never
make 2 separate / independent changes in a single commit just because they
happen to touch overlapping lines of code.

> I have no board with musb, but I think a 0 based port number is wrong.

I've a board with musb, and I plan to test your patch as soon as a new version
which re-adds the -1 for musb is posted. If you decided to do a second patch to
remove the -1, I can then test that afterwards, with as an added bonus that if
things break I can also tell you if it is the new shared helper which breaks
things, or the removing of the -1 :)

Regards,

Hans

[U-Boot] [PATCH 1/2 v2] usb: Move determination of TT hub address/port into seperate function

[Stefan Brüns]

Start split and complete split tokens need the hub address and the
downstream port of the first HS hub (device view).

The core of the function was duplicated in both host/ehci_hcd and
musb-new/usb-compat.h.

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
v2: - renamed function to usb_find_usb2_hub_address_port()
    - put musb port numbering change into seperate patch

 common/usb.c                      | 54 +++++++++++++++++++++++++++++++++++++++
 drivers/usb/host/ehci-hcd.c       | 50 ++++--------------------------------
 drivers/usb/musb-new/musb_host.c  |  8 +++---
 drivers/usb/musb-new/usb-compat.h | 53 --------------------------------------
 include/usb.h                     | 12 +++++++++
 5 files changed, 76 insertions(+), 101 deletions(-)

diff --git a/common/usb.c b/common/usb.c
index cc79695..eecdf58 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -1213,4 +1213,58 @@ bool usb_device_has_child_on_port(struct usb_device *parent, int port)
 #endif
 }
 
+#ifdef CONFIG_DM_USB
+void usb_find_usb2_hub_address_port(struct usb_device *udev,
+			       uint8_t *hub_address, uint8_t *hub_port)
+{
+	struct udevice *parent;
+	struct usb_device *uparent, *ttdev;
+
+	/*
+	 * When called from usb-uclass.c: usb_scan_device() udev->dev points
+	 * to the parent udevice, not the actual udevice belonging to the
+	 * udev as the device is not instantiated yet. So when searching
+	 * for the first usb-2 parent start with udev->dev not
+	 * udev->dev->parent .
+	 */
+	ttdev = udev;
+	parent = udev->dev;
+	uparent = dev_get_parent_priv(parent);
+
+	while (uparent->speed != USB_SPEED_HIGH) {
+		struct udevice *dev = parent;
+
+		if (device_get_uclass_id(dev->parent) != UCLASS_USB_HUB) {
+			printf("Error: Cannot find high speed parent of usb-1 device\n");
+			*hub_address = *hub_port = 0;
+			return;
+		}
+
+		ttdev = dev_get_parent_priv(dev);
+		parent = dev->parent;
+		uparent = dev_get_parent_priv(parent);
+	}
+	*hub_address = uparent->devnum;
+	*hub_port = ttdev->portnr;
+}
+#else
+void usb_find_usb2_hub_address_port(struct usb_device *udev,
+			       uint8_t *hub_address, uint8_t *hub_port)
+{
+	/* Find out the nearest parent which is high speed */
+	while (udev->parent->parent != NULL)
+		if (udev->parent->speed != USB_SPEED_HIGH) {
+			udev = udev->parent;
+		} else {
+			*hub_address = udev->parent->devnum;
+			*hub_port = udev->portnr;
+			return;
+		}
+
+	printf("Error: Cannot find high speed parent of usb-1 device\n");
+	*hub_address = *hub_port = 0;
+}
+#endif
+
+
 /* EOF */
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index c85dbce..af025de 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -279,56 +279,16 @@ static inline u8 ehci_encode_speed(enum usb_device_speed speed)
 static void ehci_update_endpt2_dev_n_port(struct usb_device *udev,
 					  struct QH *qh)
 {
-	struct usb_device *ttdev;
-	int parent_devnum;
+	uint8_t portnr = 0;
+	uint8_t hubaddr = 0;
 
 	if (udev->speed != USB_SPEED_LOW && udev->speed != USB_SPEED_FULL)
 		return;
 
-	/*
-	 * For full / low speed devices we need to get the devnum and portnr of
-	 * the tt, so of the first upstream usb-2 hub, there may be usb-1 hubs
-	 * in the tree before that one!
-	 */
-#ifdef CONFIG_DM_USB
-	/*
-	 * When called from usb-uclass.c: usb_scan_device() udev->dev points
-	 * to the parent udevice, not the actual udevice belonging to the
-	 * udev as the device is not instantiated yet. So when searching
-	 * for the first usb-2 parent start with udev->dev not
-	 * udev->dev->parent .
-	 */
-	struct udevice *parent;
-	struct usb_device *uparent;
-
-	ttdev = udev;
-	parent = udev->dev;
-	uparent = dev_get_parent_priv(parent);
-
-	while (uparent->speed != USB_SPEED_HIGH) {
-		struct udevice *dev = parent;
-
-		if (device_get_uclass_id(dev->parent) != UCLASS_USB_HUB) {
-			printf("ehci: Error cannot find high-speed parent of usb-1 device\n");
-			return;
-		}
-
-		ttdev = dev_get_parent_priv(dev);
-		parent = dev->parent;
-		uparent = dev_get_parent_priv(parent);
-	}
-	parent_devnum = uparent->devnum;
-#else
-	ttdev = udev;
-	while (ttdev->parent && ttdev->parent->speed != USB_SPEED_HIGH)
-		ttdev = ttdev->parent;
-	if (!ttdev->parent)
-		return;
-	parent_devnum = ttdev->parent->devnum;
-#endif
+	usb_find_usb2_hub_address_port(udev, &hubaddr, &portnr)
 
-	qh->qh_endpt2 |= cpu_to_hc32(QH_ENDPT2_PORTNUM(ttdev->portnr) |
-				     QH_ENDPT2_HUBADDR(parent_devnum));
+	qh->qh_endpt2 |= cpu_to_hc32(QH_ENDPT2_PORTNUM(portnr) |
+				     QH_ENDPT2_HUBADDR(hubaddr));
 }
 
 static int
diff --git a/drivers/usb/musb-new/musb_host.c b/drivers/usb/musb-new/musb_host.c
index 40b9c66..fc4ba62 100644
--- a/drivers/usb/musb-new/musb_host.c
+++ b/drivers/usb/musb-new/musb_host.c
@@ -2092,9 +2092,11 @@ int musb_urb_enqueue(
 			}
 #else
 			if (tt_needed(musb, urb->dev)) {
-				u16 hub_port = find_tt(urb->dev);
-				qh->h_addr_reg = (u8) (hub_port >> 8);
-				qh->h_port_reg = (u8) (hub_port & 0xff);
+				uint8_t portnr = 0;
+				uint8_t hubaddr = 0;
+				usb_find_usb2_hub_address_port(udev, &hubaddr, &portnr)
+				qh->h_addr_reg = hubaddr;
+				qh->h_port_reg = portnr - 1;
 			}
 #endif
 		}
diff --git a/drivers/usb/musb-new/usb-compat.h b/drivers/usb/musb-new/usb-compat.h
index 1c41e2a..760bd78 100644
--- a/drivers/usb/musb-new/usb-compat.h
+++ b/drivers/usb/musb-new/usb-compat.h
@@ -68,38 +68,6 @@ static inline int usb_hcd_unmap_urb_for_dma(struct usb_hcd *hcd,
 }
 
 #ifdef CONFIG_DM_USB
-static inline u16 find_tt(struct usb_device *udev)
-{
-	struct udevice *parent;
-	struct usb_device *uparent, *ttdev;
-
-	/*
-	 * When called from usb-uclass.c: usb_scan_device() udev->dev points
-	 * to the parent udevice, not the actual udevice belonging to the
-	 * udev as the device is not instantiated yet. So when searching
-	 * for the first usb-2 parent start with udev->dev not
-	 * udev->dev->parent .
-	 */
-	ttdev = udev;
-	parent = udev->dev;
-	uparent = dev_get_parent_priv(parent);
-
-	while (uparent->speed != USB_SPEED_HIGH) {
-		struct udevice *dev = parent;
-
-		if (device_get_uclass_id(dev->parent) != UCLASS_USB_HUB) {
-			printf("musb: Error cannot find high speed parent of usb-1 device\n");
-			return 0;
-		}
-
-		ttdev = dev_get_parent_priv(dev);
-		parent = dev->parent;
-		uparent = dev_get_parent_priv(parent);
-	}
-
-	return (uparent->devnum << 8) | (ttdev->portnr - 1);
-}
-
 static inline struct usb_device *usb_dev_get_parent(struct usb_device *udev)
 {
 	struct udevice *parent = udev->dev->parent;
@@ -129,27 +97,6 @@ static inline struct usb_device *usb_dev_get_parent(struct usb_device *udev)
 	return NULL;
 }
 #else
-static inline u16 find_tt(struct usb_device *dev)
-{
-	u8 chid;
-	u8 hub;
-
-	/* Find out the nearest parent which is high speed */
-	while (dev->parent->parent != NULL)
-		if (dev->parent->speed != USB_SPEED_HIGH)
-			dev = dev->parent;
-		else
-			break;
-
-	/* determine the port address at that hub */
-	hub = dev->parent->devnum;
-	for (chid = 0; chid < USB_MAXCHILDREN; chid++)
-		if (dev->parent->children[chid] == dev)
-			break;
-
-	return (hub << 8) | chid;
-}
-
 static inline struct usb_device *usb_dev_get_parent(struct usb_device *dev)
 {
 	return dev->parent;
diff --git a/include/usb.h b/include/usb.h
index 198ecbc..2539364 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -875,6 +875,18 @@ int legacy_hub_port_reset(struct usb_device *dev, int port,
 
 int hub_port_reset(struct udevice *dev, int port, unsigned short *portstat);
 
+/*
+ * usb_find_usb2_hub_address_port() - Get hub address and port for TT setting
+ *
+ * Searches for the first HS hub above the given device. If a
+ * HS hub is found, the hub address and the port the device is
+ * connected to is return, as required for SPLIT transactions
+ *
+ * @param: udev full speed or low speed device
+ */
+void usb_find_usb2_hub_address_port(struct usb_device *udev,
+				    uint8_t *hub_address, uint8_t *hub_port);
+
 /**
  * usb_alloc_new_device() - Allocate a new device
  *
-- 
2.1.4

[U-Boot] [PATCH 2/2 v2] usb: musb: Fix hub port number for SPLIT transactions

[Stefan Brüns]

The ifdef'ed Linux kernel code uses the 1 based port number, whereas U-Boot
puts a 0 based port number into the register. The reason the 0 based port
number apparently works can probably be taken from the USB 2.0 spec:

8.4.2.2 Start-Split Transaction Token
... The host must correctly set the port field for single and multiple TT
hub implementations. A single TT hub implementation *may ignore* the port
field.

Actually, as far as I unterstand, a multi TT hub defaults to single TT
(bAlternateSetting: 0) until switched via SetInterface, so even "port 42"
would work.

The change was verified by hardcoding the port number to a wrong value,
SPLIT transactions kept working (although using a DWC2 instead of MUSB).
Tested hubs are the RPi onboard SMC9514 and an external "05e3:0608
Genesys Logic, Inc. USB-2.0 4-Port HUB". The former is a multi TT hub,
the latter single TT only.

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 drivers/usb/musb-new/musb_host.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/musb-new/musb_host.c b/drivers/usb/musb-new/musb_host.c
index fc4ba62..5c028d4 100644
--- a/drivers/usb/musb-new/musb_host.c
+++ b/drivers/usb/musb-new/musb_host.c
@@ -2096,7 +2096,7 @@ int musb_urb_enqueue(
 				uint8_t hubaddr = 0;
 				usb_find_usb2_hub_address_port(udev, &hubaddr, &portnr)
 				qh->h_addr_reg = hubaddr;
-				qh->h_port_reg = portnr - 1;
+				qh->h_port_reg = portnr;
 			}
 #endif
 		}
-- 
2.1.4

[U-Boot] [PATCH 2/2 v2] usb: musb: Fix hub port number for SPLIT transactions

[Hans de Goede]

Hi,

On 19-12-15 21:16, Stefan Br?ns wrote:
> The ifdef'ed Linux kernel code uses the 1 based port number, whereas U-Boot
> puts a 0 based port number into the register. The reason the 0 based port
> number apparently works can probably be taken from the USB 2.0 spec:
>
> 8.4.2.2 Start-Split Transaction Token
> ... The host must correctly set the port field for single and multiple TT
> hub implementations. A single TT hub implementation *may ignore* the port
> field.
>
> Actually, as far as I unterstand, a multi TT hub defaults to single TT
> (bAlternateSetting: 0) until switched via SetInterface, so even "port 42"
> would work.
>
> The change was verified by hardcoding the port number to a wrong value,
> SPLIT transactions kept working (although using a DWC2 instead of MUSB).
> Tested hubs are the RPi onboard SMC9514 and an external "05e3:0608
> Genesys Logic, Inc. USB-2.0 4-Port HUB". The former is a multi TT hub,
> the latter single TT only.
>
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> ---
>   drivers/usb/musb-new/musb_host.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/musb-new/musb_host.c b/drivers/usb/musb-new/musb_host.c
> index fc4ba62..5c028d4 100644
> --- a/drivers/usb/musb-new/musb_host.c
> +++ b/drivers/usb/musb-new/musb_host.c
> @@ -2096,7 +2096,7 @@ int musb_urb_enqueue(
>   				uint8_t hubaddr = 0;
>   				usb_find_usb2_hub_address_port(udev, &hubaddr, &portnr)
>   				qh->h_addr_reg = hubaddr;
> -				qh->h_port_reg = portnr - 1;
> +				qh->h_port_reg = portnr;
>   			}
>   #endif
>   		}
>

This patch needs to be re-spun to apply on top of the compile fixed [patch 1/2]
of this set. With that done this patch is:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

[U-Boot] [PATCH 2/2 v2] usb: musb: Fix hub port number for SPLIT transactions

[Marek Vasut]

On Monday, December 21, 2015 at 08:33:31 PM, Hans de Goede wrote:
> Hi,
> 
> On 19-12-15 21:16, Stefan Br?ns wrote:
> > The ifdef'ed Linux kernel code uses the 1 based port number, whereas
> > U-Boot puts a 0 based port number into the register. The reason the 0
> > based port number apparently works can probably be taken from the USB
> > 2.0 spec:
> > 
> > 8.4.2.2 Start-Split Transaction Token
> > ... The host must correctly set the port field for single and multiple TT
> > hub implementations. A single TT hub implementation *may ignore* the port
> > field.
> > 
> > Actually, as far as I unterstand, a multi TT hub defaults to single TT
> > (bAlternateSetting: 0) until switched via SetInterface, so even "port 42"
> > would work.
> > 
> > The change was verified by hardcoding the port number to a wrong value,
> > SPLIT transactions kept working (although using a DWC2 instead of MUSB).
> > Tested hubs are the RPi onboard SMC9514 and an external "05e3:0608
> > Genesys Logic, Inc. USB-2.0 4-Port HUB". The former is a multi TT hub,
> > the latter single TT only.
> > 
> > Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> > ---
> > 
> >   drivers/usb/musb-new/musb_host.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/musb-new/musb_host.c
> > b/drivers/usb/musb-new/musb_host.c index fc4ba62..5c028d4 100644
> > --- a/drivers/usb/musb-new/musb_host.c
> > +++ b/drivers/usb/musb-new/musb_host.c
> > @@ -2096,7 +2096,7 @@ int musb_urb_enqueue(
> > 
> >   				uint8_t hubaddr = 0;
> >   				usb_find_usb2_hub_address_port(udev, &hubaddr, 
&portnr)
> >   				qh->h_addr_reg = hubaddr;
> > 
> > -				qh->h_port_reg = portnr - 1;
> > +				qh->h_port_reg = portnr;
> > 
> >   			}
> >   
> >   #endif
> >   
> >   		}
> 
> This patch needs to be re-spun to apply on top of the compile fixed [patch
> 1/2] of this set. With that done this patch is:
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Hans de Goede <hdegoede@redhat.com>

OK, would you, Stefan, please resend the patchset with the necessary Reviewed-by
etc so I can pick it ? This thread is a chaos and I am lost. 

Thanks!

Best regards,
Marek Vasut

[U-Boot] [PATCH 2/2 v2] usb: musb: Fix hub port number for SPLIT transactions

[Stefan Bruens]

On Monday 21 December 2015 21:27:01 Marek Vasut wrote:
> On Monday, December 21, 2015 at 08:33:31 PM, Hans de Goede wrote:
> > This patch needs to be re-spun to apply on top of the compile fixed [patch
> > 1/2] of this set. With that done this patch is:
> > 
> > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > Tested-by: Hans de Goede <hdegoede@redhat.com>
> 
> OK, would you, Stefan, please resend the patchset with the necessary
> Reviewed-by etc so I can pick it ? This thread is a chaos and I am lost.
> 
Hi,

is it okay if I send just the patches which are cleanup/bugfix work, as I am 
currently reworking the DWC split support? Currently this is:

1. Dynamic allocation of configuration descriptor (1 patch)
2. Factor out function for hub address/port, fix musb port number (2 patches)
3. Fix out-of-bound access in DWC (1 patch)

These four patches are the first 4 patches from the old 7 patch series, so 
have been reviewed.

Kind regards,

Stefan


-- 
Stefan Br?ns  /  Bergstra?e 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019
work: +49 2405 49936-424

[U-Boot] [PATCH 2/2 v2] usb: musb: Fix hub port number for SPLIT transactions

[Marek Vasut]

On Tuesday, December 22, 2015 at 12:02:44 AM, Stefan Bruens wrote:
> On Monday 21 December 2015 21:27:01 Marek Vasut wrote:
> > On Monday, December 21, 2015 at 08:33:31 PM, Hans de Goede wrote:
> > > This patch needs to be re-spun to apply on top of the compile fixed
> > > [patch 1/2] of this set. With that done this patch is:
> > > 
> > > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > > Tested-by: Hans de Goede <hdegoede@redhat.com>
> > 
> > OK, would you, Stefan, please resend the patchset with the necessary
> > Reviewed-by etc so I can pick it ? This thread is a chaos and I am lost.
> 
> Hi,
> 
> is it okay if I send just the patches which are cleanup/bugfix work, as I
> am currently reworking the DWC split support? Currently this is:
> 
> 1. Dynamic allocation of configuration descriptor (1 patch)
> 2. Factor out function for hub address/port, fix musb port number (2
> patches) 3. Fix out-of-bound access in DWC (1 patch)
> 
> These four patches are the first 4 patches from the old 7 patch series, so
> have been reviewed.

That's fine, thanks!

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/2 v3] usb: Move determination of TT hub address/port into seperate function

[Stefan Brüns]

Start split and complete split tokens need the hub address and the
downstream port of the first HS hub (device view).

The core of the function was duplicated in both host/ehci_hcd and
musb-new/usb-compat.h.

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
v2: - renamed function to usb_find_usb2_hub_address_port()
    - put musb port numbering change into seperate patch
v3: - do one assignement per line

 common/usb.c                      | 56 +++++++++++++++++++++++++++++++++++++++
 drivers/usb/host/ehci-hcd.c       | 50 ++++------------------------------
 drivers/usb/musb-new/musb_host.c  |  9 ++++---
 drivers/usb/musb-new/usb-compat.h | 53 ------------------------------------
 include/usb.h                     | 12 +++++++++
 5 files changed, 79 insertions(+), 101 deletions(-)

diff --git a/common/usb.c b/common/usb.c
index cc79695..41efe27 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -1213,4 +1213,60 @@ bool usb_device_has_child_on_port(struct usb_device *parent, int port)
 #endif
 }
 
+#ifdef CONFIG_DM_USB
+void usb_find_usb2_hub_address_port(struct usb_device *udev,
+			       uint8_t *hub_address, uint8_t *hub_port)
+{
+	struct udevice *parent;
+	struct usb_device *uparent, *ttdev;
+
+	/*
+	 * When called from usb-uclass.c: usb_scan_device() udev->dev points
+	 * to the parent udevice, not the actual udevice belonging to the
+	 * udev as the device is not instantiated yet. So when searching
+	 * for the first usb-2 parent start with udev->dev not
+	 * udev->dev->parent .
+	 */
+	ttdev = udev;
+	parent = udev->dev;
+	uparent = dev_get_parent_priv(parent);
+
+	while (uparent->speed != USB_SPEED_HIGH) {
+		struct udevice *dev = parent;
+
+		if (device_get_uclass_id(dev->parent) != UCLASS_USB_HUB) {
+			printf("Error: Cannot find high speed parent of usb-1 device\n");
+			*hub_address = 0;
+			*hub_port = 0;
+			return;
+		}
+
+		ttdev = dev_get_parent_priv(dev);
+		parent = dev->parent;
+		uparent = dev_get_parent_priv(parent);
+	}
+	*hub_address = uparent->devnum;
+	*hub_port = ttdev->portnr;
+}
+#else
+void usb_find_usb2_hub_address_port(struct usb_device *udev,
+			       uint8_t *hub_address, uint8_t *hub_port)
+{
+	/* Find out the nearest parent which is high speed */
+	while (udev->parent->parent != NULL)
+		if (udev->parent->speed != USB_SPEED_HIGH) {
+			udev = udev->parent;
+		} else {
+			*hub_address = udev->parent->devnum;
+			*hub_port = udev->portnr;
+			return;
+		}
+
+	printf("Error: Cannot find high speed parent of usb-1 device\n");
+	*hub_address = 0;
+	*hub_port = 0;
+}
+#endif
+
+
 /* EOF */
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index c85dbce..af025de 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -279,56 +279,16 @@ static inline u8 ehci_encode_speed(enum usb_device_speed speed)
 static void ehci_update_endpt2_dev_n_port(struct usb_device *udev,
 					  struct QH *qh)
 {
-	struct usb_device *ttdev;
-	int parent_devnum;
+	uint8_t portnr = 0;
+	uint8_t hubaddr = 0;
 
 	if (udev->speed != USB_SPEED_LOW && udev->speed != USB_SPEED_FULL)
 		return;
 
-	/*
-	 * For full / low speed devices we need to get the devnum and portnr of
-	 * the tt, so of the first upstream usb-2 hub, there may be usb-1 hubs
-	 * in the tree before that one!
-	 */
-#ifdef CONFIG_DM_USB
-	/*
-	 * When called from usb-uclass.c: usb_scan_device() udev->dev points
-	 * to the parent udevice, not the actual udevice belonging to the
-	 * udev as the device is not instantiated yet. So when searching
-	 * for the first usb-2 parent start with udev->dev not
-	 * udev->dev->parent .
-	 */
-	struct udevice *parent;
-	struct usb_device *uparent;
-
-	ttdev = udev;
-	parent = udev->dev;
-	uparent = dev_get_parent_priv(parent);
-
-	while (uparent->speed != USB_SPEED_HIGH) {
-		struct udevice *dev = parent;
-
-		if (device_get_uclass_id(dev->parent) != UCLASS_USB_HUB) {
-			printf("ehci: Error cannot find high-speed parent of usb-1 device\n");
-			return;
-		}
-
-		ttdev = dev_get_parent_priv(dev);
-		parent = dev->parent;
-		uparent = dev_get_parent_priv(parent);
-	}
-	parent_devnum = uparent->devnum;
-#else
-	ttdev = udev;
-	while (ttdev->parent && ttdev->parent->speed != USB_SPEED_HIGH)
-		ttdev = ttdev->parent;
-	if (!ttdev->parent)
-		return;
-	parent_devnum = ttdev->parent->devnum;
-#endif
+	usb_find_usb2_hub_address_port(udev, &hubaddr, &portnr)
 
-	qh->qh_endpt2 |= cpu_to_hc32(QH_ENDPT2_PORTNUM(ttdev->portnr) |
-				     QH_ENDPT2_HUBADDR(parent_devnum));
+	qh->qh_endpt2 |= cpu_to_hc32(QH_ENDPT2_PORTNUM(portnr) |
+				     QH_ENDPT2_HUBADDR(hubaddr));
 }
 
 static int
diff --git a/drivers/usb/musb-new/musb_host.c b/drivers/usb/musb-new/musb_host.c
index 40b9c66..0125679 100644
--- a/drivers/usb/musb-new/musb_host.c
+++ b/drivers/usb/musb-new/musb_host.c
@@ -2092,9 +2092,12 @@ int musb_urb_enqueue(
 			}
 #else
 			if (tt_needed(musb, urb->dev)) {
-				u16 hub_port = find_tt(urb->dev);
-				qh->h_addr_reg = (u8) (hub_port >> 8);
-				qh->h_port_reg = (u8) (hub_port & 0xff);
+				uint8_t portnr = 0;
+				uint8_t hubaddr = 0;
+				usb_find_usb2_hub_address_port(udev, &hubaddr,
+							       &portnr);
+				qh->h_addr_reg = hubaddr;
+				qh->h_port_reg = portnr - 1;
 			}
 #endif
 		}
diff --git a/drivers/usb/musb-new/usb-compat.h b/drivers/usb/musb-new/usb-compat.h
index 1c41e2a..760bd78 100644
--- a/drivers/usb/musb-new/usb-compat.h
+++ b/drivers/usb/musb-new/usb-compat.h
@@ -68,38 +68,6 @@ static inline int usb_hcd_unmap_urb_for_dma(struct usb_hcd *hcd,
 }
 
 #ifdef CONFIG_DM_USB
-static inline u16 find_tt(struct usb_device *udev)
-{
-	struct udevice *parent;
-	struct usb_device *uparent, *ttdev;
-
-	/*
-	 * When called from usb-uclass.c: usb_scan_device() udev->dev points
-	 * to the parent udevice, not the actual udevice belonging to the
-	 * udev as the device is not instantiated yet. So when searching
-	 * for the first usb-2 parent start with udev->dev not
-	 * udev->dev->parent .
-	 */
-	ttdev = udev;
-	parent = udev->dev;
-	uparent = dev_get_parent_priv(parent);
-
-	while (uparent->speed != USB_SPEED_HIGH) {
-		struct udevice *dev = parent;
-
-		if (device_get_uclass_id(dev->parent) != UCLASS_USB_HUB) {
-			printf("musb: Error cannot find high speed parent of usb-1 device\n");
-			return 0;
-		}
-
-		ttdev = dev_get_parent_priv(dev);
-		parent = dev->parent;
-		uparent = dev_get_parent_priv(parent);
-	}
-
-	return (uparent->devnum << 8) | (ttdev->portnr - 1);
-}
-
 static inline struct usb_device *usb_dev_get_parent(struct usb_device *udev)
 {
 	struct udevice *parent = udev->dev->parent;
@@ -129,27 +97,6 @@ static inline struct usb_device *usb_dev_get_parent(struct usb_device *udev)
 	return NULL;
 }
 #else
-static inline u16 find_tt(struct usb_device *dev)
-{
-	u8 chid;
-	u8 hub;
-
-	/* Find out the nearest parent which is high speed */
-	while (dev->parent->parent != NULL)
-		if (dev->parent->speed != USB_SPEED_HIGH)
-			dev = dev->parent;
-		else
-			break;
-
-	/* determine the port address at that hub */
-	hub = dev->parent->devnum;
-	for (chid = 0; chid < USB_MAXCHILDREN; chid++)
-		if (dev->parent->children[chid] == dev)
-			break;
-
-	return (hub << 8) | chid;
-}
-
 static inline struct usb_device *usb_dev_get_parent(struct usb_device *dev)
 {
 	return dev->parent;
diff --git a/include/usb.h b/include/usb.h
index 198ecbc..2539364 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -875,6 +875,18 @@ int legacy_hub_port_reset(struct usb_device *dev, int port,
 
 int hub_port_reset(struct udevice *dev, int port, unsigned short *portstat);
 
+/*
+ * usb_find_usb2_hub_address_port() - Get hub address and port for TT setting
+ *
+ * Searches for the first HS hub above the given device. If a
+ * HS hub is found, the hub address and the port the device is
+ * connected to is return, as required for SPLIT transactions
+ *
+ * @param: udev full speed or low speed device
+ */
+void usb_find_usb2_hub_address_port(struct usb_device *udev,
+				    uint8_t *hub_address, uint8_t *hub_port);
+
 /**
  * usb_alloc_new_device() - Allocate a new device
  *
-- 
2.1.4

[U-Boot] [PATCH 1/2 v3] usb: Move determination of TT hub address/port into seperate function

[Hans de Goede]

Hi,

On 19-12-15 21:26, Stefan Br?ns wrote:
> Start split and complete split tokens need the hub address and the
> downstream port of the first HS hub (device view).
>
> The core of the function was duplicated in both host/ehci_hcd and
> musb-new/usb-compat.h.
>
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> ---
> v2: - renamed function to usb_find_usb2_hub_address_port()
>      - put musb port numbering change into seperate patch
> v3: - do one assignement per line

Thanks, I've tried to test this, but it seems that musb-new support in
current master is broken in host mode when using hubs even when not
adding your patch, so I need to debug that first...

Note that your patch for musb-new does not even compile, you need this
compile fix:

@@ -2094,7 +2094,8 @@ int musb_urb_enqueue(
  			if (tt_needed(musb, urb->dev)) {
  				uint8_t portnr = 0;
  				uint8_t hubaddr = 0;
-				usb_find_usb2_hub_address_port(udev, &hubaddr,
+				usb_find_usb2_hub_address_port(urb->dev,
+							       &hubaddr,
  							       &portnr);
  				qh->h_addr_reg = hubaddr;
  				qh->h_port_reg = portnr - 1;

But with that still things do not work (as said current master seems broken atm).

Regards,

Hans


>
>   common/usb.c                      | 56 +++++++++++++++++++++++++++++++++++++++
>   drivers/usb/host/ehci-hcd.c       | 50 ++++------------------------------
>   drivers/usb/musb-new/musb_host.c  |  9 ++++---
>   drivers/usb/musb-new/usb-compat.h | 53 ------------------------------------
>   include/usb.h                     | 12 +++++++++
>   5 files changed, 79 insertions(+), 101 deletions(-)
>
> diff --git a/common/usb.c b/common/usb.c
> index cc79695..41efe27 100644
> --- a/common/usb.c
> +++ b/common/usb.c
> @@ -1213,4 +1213,60 @@ bool usb_device_has_child_on_port(struct usb_device *parent, int port)
>   #endif
>   }
>
> +#ifdef CONFIG_DM_USB
> +void usb_find_usb2_hub_address_port(struct usb_device *udev,
> +			       uint8_t *hub_address, uint8_t *hub_port)
> +{
> +	struct udevice *parent;
> +	struct usb_device *uparent, *ttdev;
> +
> +	/*
> +	 * When called from usb-uclass.c: usb_scan_device() udev->dev points
> +	 * to the parent udevice, not the actual udevice belonging to the
> +	 * udev as the device is not instantiated yet. So when searching
> +	 * for the first usb-2 parent start with udev->dev not
> +	 * udev->dev->parent .
> +	 */
> +	ttdev = udev;
> +	parent = udev->dev;
> +	uparent = dev_get_parent_priv(parent);
> +
> +	while (uparent->speed != USB_SPEED_HIGH) {
> +		struct udevice *dev = parent;
> +
> +		if (device_get_uclass_id(dev->parent) != UCLASS_USB_HUB) {
> +			printf("Error: Cannot find high speed parent of usb-1 device\n");
> +			*hub_address = 0;
> +			*hub_port = 0;
> +			return;
> +		}
> +
> +		ttdev = dev_get_parent_priv(dev);
> +		parent = dev->parent;
> +		uparent = dev_get_parent_priv(parent);
> +	}
> +	*hub_address = uparent->devnum;
> +	*hub_port = ttdev->portnr;
> +}
> +#else
> +void usb_find_usb2_hub_address_port(struct usb_device *udev,
> +			       uint8_t *hub_address, uint8_t *hub_port)
> +{
> +	/* Find out the nearest parent which is high speed */
> +	while (udev->parent->parent != NULL)
> +		if (udev->parent->speed != USB_SPEED_HIGH) {
> +			udev = udev->parent;
> +		} else {
> +			*hub_address = udev->parent->devnum;
> +			*hub_port = udev->portnr;
> +			return;
> +		}
> +
> +	printf("Error: Cannot find high speed parent of usb-1 device\n");
> +	*hub_address = 0;
> +	*hub_port = 0;
> +}
> +#endif
> +
> +
>   /* EOF */
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index c85dbce..af025de 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -279,56 +279,16 @@ static inline u8 ehci_encode_speed(enum usb_device_speed speed)
>   static void ehci_update_endpt2_dev_n_port(struct usb_device *udev,
>   					  struct QH *qh)
>   {
> -	struct usb_device *ttdev;
> -	int parent_devnum;
> +	uint8_t portnr = 0;
> +	uint8_t hubaddr = 0;
>
>   	if (udev->speed != USB_SPEED_LOW && udev->speed != USB_SPEED_FULL)
>   		return;
>
> -	/*
> -	 * For full / low speed devices we need to get the devnum and portnr of
> -	 * the tt, so of the first upstream usb-2 hub, there may be usb-1 hubs
> -	 * in the tree before that one!
> -	 */
> -#ifdef CONFIG_DM_USB
> -	/*
> -	 * When called from usb-uclass.c: usb_scan_device() udev->dev points
> -	 * to the parent udevice, not the actual udevice belonging to the
> -	 * udev as the device is not instantiated yet. So when searching
> -	 * for the first usb-2 parent start with udev->dev not
> -	 * udev->dev->parent .
> -	 */
> -	struct udevice *parent;
> -	struct usb_device *uparent;
> -
> -	ttdev = udev;
> -	parent = udev->dev;
> -	uparent = dev_get_parent_priv(parent);
> -
> -	while (uparent->speed != USB_SPEED_HIGH) {
> -		struct udevice *dev = parent;
> -
> -		if (device_get_uclass_id(dev->parent) != UCLASS_USB_HUB) {
> -			printf("ehci: Error cannot find high-speed parent of usb-1 device\n");
> -			return;
> -		}
> -
> -		ttdev = dev_get_parent_priv(dev);
> -		parent = dev->parent;
> -		uparent = dev_get_parent_priv(parent);
> -	}
> -	parent_devnum = uparent->devnum;
> -#else
> -	ttdev = udev;
> -	while (ttdev->parent && ttdev->parent->speed != USB_SPEED_HIGH)
> -		ttdev = ttdev->parent;
> -	if (!ttdev->parent)
> -		return;
> -	parent_devnum = ttdev->parent->devnum;
> -#endif
> +	usb_find_usb2_hub_address_port(udev, &hubaddr, &portnr)
>
> -	qh->qh_endpt2 |= cpu_to_hc32(QH_ENDPT2_PORTNUM(ttdev->portnr) |
> -				     QH_ENDPT2_HUBADDR(parent_devnum));
> +	qh->qh_endpt2 |= cpu_to_hc32(QH_ENDPT2_PORTNUM(portnr) |
> +				     QH_ENDPT2_HUBADDR(hubaddr));
>   }
>
>   static int
> diff --git a/drivers/usb/musb-new/musb_host.c b/drivers/usb/musb-new/musb_host.c
> index 40b9c66..0125679 100644
> --- a/drivers/usb/musb-new/musb_host.c
> +++ b/drivers/usb/musb-new/musb_host.c
> @@ -2092,9 +2092,12 @@ int musb_urb_enqueue(
>   			}
>   #else
>   			if (tt_needed(musb, urb->dev)) {
> -				u16 hub_port = find_tt(urb->dev);
> -				qh->h_addr_reg = (u8) (hub_port >> 8);
> -				qh->h_port_reg = (u8) (hub_port & 0xff);
> +				uint8_t portnr = 0;
> +				uint8_t hubaddr = 0;
> +				usb_find_usb2_hub_address_port(udev, &hubaddr,
> +							       &portnr);
> +				qh->h_addr_reg = hubaddr;
> +				qh->h_port_reg = portnr - 1;
>   			}
>   #endif
>   		}
> diff --git a/drivers/usb/musb-new/usb-compat.h b/drivers/usb/musb-new/usb-compat.h
> index 1c41e2a..760bd78 100644
> --- a/drivers/usb/musb-new/usb-compat.h
> +++ b/drivers/usb/musb-new/usb-compat.h
> @@ -68,38 +68,6 @@ static inline int usb_hcd_unmap_urb_for_dma(struct usb_hcd *hcd,
>   }
>
>   #ifdef CONFIG_DM_USB
> -static inline u16 find_tt(struct usb_device *udev)
> -{
> -	struct udevice *parent;
> -	struct usb_device *uparent, *ttdev;
> -
> -	/*
> -	 * When called from usb-uclass.c: usb_scan_device() udev->dev points
> -	 * to the parent udevice, not the actual udevice belonging to the
> -	 * udev as the device is not instantiated yet. So when searching
> -	 * for the first usb-2 parent start with udev->dev not
> -	 * udev->dev->parent .
> -	 */
> -	ttdev = udev;
> -	parent = udev->dev;
> -	uparent = dev_get_parent_priv(parent);
> -
> -	while (uparent->speed != USB_SPEED_HIGH) {
> -		struct udevice *dev = parent;
> -
> -		if (device_get_uclass_id(dev->parent) != UCLASS_USB_HUB) {
> -			printf("musb: Error cannot find high speed parent of usb-1 device\n");
> -			return 0;
> -		}
> -
> -		ttdev = dev_get_parent_priv(dev);
> -		parent = dev->parent;
> -		uparent = dev_get_parent_priv(parent);
> -	}
> -
> -	return (uparent->devnum << 8) | (ttdev->portnr - 1);
> -}
> -
>   static inline struct usb_device *usb_dev_get_parent(struct usb_device *udev)
>   {
>   	struct udevice *parent = udev->dev->parent;
> @@ -129,27 +97,6 @@ static inline struct usb_device *usb_dev_get_parent(struct usb_device *udev)
>   	return NULL;
>   }
>   #else
> -static inline u16 find_tt(struct usb_device *dev)
> -{
> -	u8 chid;
> -	u8 hub;
> -
> -	/* Find out the nearest parent which is high speed */
> -	while (dev->parent->parent != NULL)
> -		if (dev->parent->speed != USB_SPEED_HIGH)
> -			dev = dev->parent;
> -		else
> -			break;
> -
> -	/* determine the port address at that hub */
> -	hub = dev->parent->devnum;
> -	for (chid = 0; chid < USB_MAXCHILDREN; chid++)
> -		if (dev->parent->children[chid] == dev)
> -			break;
> -
> -	return (hub << 8) | chid;
> -}
> -
>   static inline struct usb_device *usb_dev_get_parent(struct usb_device *dev)
>   {
>   	return dev->parent;
> diff --git a/include/usb.h b/include/usb.h
> index 198ecbc..2539364 100644
> --- a/include/usb.h
> +++ b/include/usb.h
> @@ -875,6 +875,18 @@ int legacy_hub_port_reset(struct usb_device *dev, int port,
>
>   int hub_port_reset(struct udevice *dev, int port, unsigned short *portstat);
>
> +/*
> + * usb_find_usb2_hub_address_port() - Get hub address and port for TT setting
> + *
> + * Searches for the first HS hub above the given device. If a
> + * HS hub is found, the hub address and the port the device is
> + * connected to is return, as required for SPLIT transactions
> + *
> + * @param: udev full speed or low speed device
> + */
> +void usb_find_usb2_hub_address_port(struct usb_device *udev,
> +				    uint8_t *hub_address, uint8_t *hub_port);
> +
>   /**
>    * usb_alloc_new_device() - Allocate a new device
>    *
>

[U-Boot] [PATCH 1/2 v3] usb: Move determination of TT hub address/port into seperate function

[Hans de Goede]

Hi,

On 20-12-15 20:12, Hans de Goede wrote:
> Hi,
>
> On 19-12-15 21:26, Stefan Br?ns wrote:
>> Start split and complete split tokens need the hub address and the
>> downstream port of the first HS hub (device view).
>>
>> The core of the function was duplicated in both host/ehci_hcd and
>> musb-new/usb-compat.h.
>>
>> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
>> ---
>> v2: - renamed function to usb_find_usb2_hub_address_port()
>>      - put musb port numbering change into seperate patch
>> v3: - do one assignement per line
>
> Thanks, I've tried to test this, but it seems that musb-new support in
> current master is broken in host mode when using hubs even when not
> adding your patch, so I need to debug that first...
>
> Note that your patch for musb-new does not even compile, you need this
> compile fix:
>
> @@ -2094,7 +2094,8 @@ int musb_urb_enqueue(
>               if (tt_needed(musb, urb->dev)) {
>                   uint8_t portnr = 0;
>                   uint8_t hubaddr = 0;
> -                usb_find_usb2_hub_address_port(udev, &hubaddr,
> +                usb_find_usb2_hub_address_port(urb->dev,
> +                                   &hubaddr,
>                                      &portnr);
>                   qh->h_addr_reg = hubaddr;
>                   qh->h_port_reg = portnr - 1;
>
> But with that still things do not work (as said current master seems broken atm).

Ok, so this is caused by the addition of drivermodel support to
the keyboard layer. I've just send a patch selecting DM_KEYBOARD for
sunxi and that fixes usb-kbds not working at all with current master.

With that fixed + the above compile fix, things work.

So with the above compile fix this patch is:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

>
> Regards,
>
> Hans
>
>
>>
>>   common/usb.c                      | 56 +++++++++++++++++++++++++++++++++++++++
>>   drivers/usb/host/ehci-hcd.c       | 50 ++++------------------------------
>>   drivers/usb/musb-new/musb_host.c  |  9 ++++---
>>   drivers/usb/musb-new/usb-compat.h | 53 ------------------------------------
>>   include/usb.h                     | 12 +++++++++
>>   5 files changed, 79 insertions(+), 101 deletions(-)
>>
>> diff --git a/common/usb.c b/common/usb.c
>> index cc79695..41efe27 100644
>> --- a/common/usb.c
>> +++ b/common/usb.c
>> @@ -1213,4 +1213,60 @@ bool usb_device_has_child_on_port(struct usb_device *parent, int port)
>>   #endif
>>   }
>>
>> +#ifdef CONFIG_DM_USB
>> +void usb_find_usb2_hub_address_port(struct usb_device *udev,
>> +                   uint8_t *hub_address, uint8_t *hub_port)
>> +{
>> +    struct udevice *parent;
>> +    struct usb_device *uparent, *ttdev;
>> +
>> +    /*
>> +     * When called from usb-uclass.c: usb_scan_device() udev->dev points
>> +     * to the parent udevice, not the actual udevice belonging to the
>> +     * udev as the device is not instantiated yet. So when searching
>> +     * for the first usb-2 parent start with udev->dev not
>> +     * udev->dev->parent .
>> +     */
>> +    ttdev = udev;
>> +    parent = udev->dev;
>> +    uparent = dev_get_parent_priv(parent);
>> +
>> +    while (uparent->speed != USB_SPEED_HIGH) {
>> +        struct udevice *dev = parent;
>> +
>> +        if (device_get_uclass_id(dev->parent) != UCLASS_USB_HUB) {
>> +            printf("Error: Cannot find high speed parent of usb-1 device\n");
>> +            *hub_address = 0;
>> +            *hub_port = 0;
>> +            return;
>> +        }
>> +
>> +        ttdev = dev_get_parent_priv(dev);
>> +        parent = dev->parent;
>> +        uparent = dev_get_parent_priv(parent);
>> +    }
>> +    *hub_address = uparent->devnum;
>> +    *hub_port = ttdev->portnr;
>> +}
>> +#else
>> +void usb_find_usb2_hub_address_port(struct usb_device *udev,
>> +                   uint8_t *hub_address, uint8_t *hub_port)
>> +{
>> +    /* Find out the nearest parent which is high speed */
>> +    while (udev->parent->parent != NULL)
>> +        if (udev->parent->speed != USB_SPEED_HIGH) {
>> +            udev = udev->parent;
>> +        } else {
>> +            *hub_address = udev->parent->devnum;
>> +            *hub_port = udev->portnr;
>> +            return;
>> +        }
>> +
>> +    printf("Error: Cannot find high speed parent of usb-1 device\n");
>> +    *hub_address = 0;
>> +    *hub_port = 0;
>> +}
>> +#endif
>> +
>> +
>>   /* EOF */
>> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
>> index c85dbce..af025de 100644
>> --- a/drivers/usb/host/ehci-hcd.c
>> +++ b/drivers/usb/host/ehci-hcd.c
>> @@ -279,56 +279,16 @@ static inline u8 ehci_encode_speed(enum usb_device_speed speed)
>>   static void ehci_update_endpt2_dev_n_port(struct usb_device *udev,
>>                         struct QH *qh)
>>   {
>> -    struct usb_device *ttdev;
>> -    int parent_devnum;
>> +    uint8_t portnr = 0;
>> +    uint8_t hubaddr = 0;
>>
>>       if (udev->speed != USB_SPEED_LOW && udev->speed != USB_SPEED_FULL)
>>           return;
>>
>> -    /*
>> -     * For full / low speed devices we need to get the devnum and portnr of
>> -     * the tt, so of the first upstream usb-2 hub, there may be usb-1 hubs
>> -     * in the tree before that one!
>> -     */
>> -#ifdef CONFIG_DM_USB
>> -    /*
>> -     * When called from usb-uclass.c: usb_scan_device() udev->dev points
>> -     * to the parent udevice, not the actual udevice belonging to the
>> -     * udev as the device is not instantiated yet. So when searching
>> -     * for the first usb-2 parent start with udev->dev not
>> -     * udev->dev->parent .
>> -     */
>> -    struct udevice *parent;
>> -    struct usb_device *uparent;
>> -
>> -    ttdev = udev;
>> -    parent = udev->dev;
>> -    uparent = dev_get_parent_priv(parent);
>> -
>> -    while (uparent->speed != USB_SPEED_HIGH) {
>> -        struct udevice *dev = parent;
>> -
>> -        if (device_get_uclass_id(dev->parent) != UCLASS_USB_HUB) {
>> -            printf("ehci: Error cannot find high-speed parent of usb-1 device\n");
>> -            return;
>> -        }
>> -
>> -        ttdev = dev_get_parent_priv(dev);
>> -        parent = dev->parent;
>> -        uparent = dev_get_parent_priv(parent);
>> -    }
>> -    parent_devnum = uparent->devnum;
>> -#else
>> -    ttdev = udev;
>> -    while (ttdev->parent && ttdev->parent->speed != USB_SPEED_HIGH)
>> -        ttdev = ttdev->parent;
>> -    if (!ttdev->parent)
>> -        return;
>> -    parent_devnum = ttdev->parent->devnum;
>> -#endif
>> +    usb_find_usb2_hub_address_port(udev, &hubaddr, &portnr)
>>
>> -    qh->qh_endpt2 |= cpu_to_hc32(QH_ENDPT2_PORTNUM(ttdev->portnr) |
>> -                     QH_ENDPT2_HUBADDR(parent_devnum));
>> +    qh->qh_endpt2 |= cpu_to_hc32(QH_ENDPT2_PORTNUM(portnr) |
>> +                     QH_ENDPT2_HUBADDR(hubaddr));
>>   }
>>
>>   static int
>> diff --git a/drivers/usb/musb-new/musb_host.c b/drivers/usb/musb-new/musb_host.c
>> index 40b9c66..0125679 100644
>> --- a/drivers/usb/musb-new/musb_host.c
>> +++ b/drivers/usb/musb-new/musb_host.c
>> @@ -2092,9 +2092,12 @@ int musb_urb_enqueue(
>>               }
>>   #else
>>               if (tt_needed(musb, urb->dev)) {
>> -                u16 hub_port = find_tt(urb->dev);
>> -                qh->h_addr_reg = (u8) (hub_port >> 8);
>> -                qh->h_port_reg = (u8) (hub_port & 0xff);
>> +                uint8_t portnr = 0;
>> +                uint8_t hubaddr = 0;
>> +                usb_find_usb2_hub_address_port(udev, &hubaddr,
>> +                                   &portnr);
>> +                qh->h_addr_reg = hubaddr;
>> +                qh->h_port_reg = portnr - 1;
>>               }
>>   #endif
>>           }
>> diff --git a/drivers/usb/musb-new/usb-compat.h b/drivers/usb/musb-new/usb-compat.h
>> index 1c41e2a..760bd78 100644
>> --- a/drivers/usb/musb-new/usb-compat.h
>> +++ b/drivers/usb/musb-new/usb-compat.h
>> @@ -68,38 +68,6 @@ static inline int usb_hcd_unmap_urb_for_dma(struct usb_hcd *hcd,
>>   }
>>
>>   #ifdef CONFIG_DM_USB
>> -static inline u16 find_tt(struct usb_device *udev)
>> -{
>> -    struct udevice *parent;
>> -    struct usb_device *uparent, *ttdev;
>> -
>> -    /*
>> -     * When called from usb-uclass.c: usb_scan_device() udev->dev points
>> -     * to the parent udevice, not the actual udevice belonging to the
>> -     * udev as the device is not instantiated yet. So when searching
>> -     * for the first usb-2 parent start with udev->dev not
>> -     * udev->dev->parent .
>> -     */
>> -    ttdev = udev;
>> -    parent = udev->dev;
>> -    uparent = dev_get_parent_priv(parent);
>> -
>> -    while (uparent->speed != USB_SPEED_HIGH) {
>> -        struct udevice *dev = parent;
>> -
>> -        if (device_get_uclass_id(dev->parent) != UCLASS_USB_HUB) {
>> -            printf("musb: Error cannot find high speed parent of usb-1 device\n");
>> -            return 0;
>> -        }
>> -
>> -        ttdev = dev_get_parent_priv(dev);
>> -        parent = dev->parent;
>> -        uparent = dev_get_parent_priv(parent);
>> -    }
>> -
>> -    return (uparent->devnum << 8) | (ttdev->portnr - 1);
>> -}
>> -
>>   static inline struct usb_device *usb_dev_get_parent(struct usb_device *udev)
>>   {
>>       struct udevice *parent = udev->dev->parent;
>> @@ -129,27 +97,6 @@ static inline struct usb_device *usb_dev_get_parent(struct usb_device *udev)
>>       return NULL;
>>   }
>>   #else
>> -static inline u16 find_tt(struct usb_device *dev)
>> -{
>> -    u8 chid;
>> -    u8 hub;
>> -
>> -    /* Find out the nearest parent which is high speed */
>> -    while (dev->parent->parent != NULL)
>> -        if (dev->parent->speed != USB_SPEED_HIGH)
>> -            dev = dev->parent;
>> -        else
>> -            break;
>> -
>> -    /* determine the port address at that hub */
>> -    hub = dev->parent->devnum;
>> -    for (chid = 0; chid < USB_MAXCHILDREN; chid++)
>> -        if (dev->parent->children[chid] == dev)
>> -            break;
>> -
>> -    return (hub << 8) | chid;
>> -}
>> -
>>   static inline struct usb_device *usb_dev_get_parent(struct usb_device *dev)
>>   {
>>       return dev->parent;
>> diff --git a/include/usb.h b/include/usb.h
>> index 198ecbc..2539364 100644
>> --- a/include/usb.h
>> +++ b/include/usb.h
>> @@ -875,6 +875,18 @@ int legacy_hub_port_reset(struct usb_device *dev, int port,
>>
>>   int hub_port_reset(struct udevice *dev, int port, unsigned short *portstat);
>>
>> +/*
>> + * usb_find_usb2_hub_address_port() - Get hub address and port for TT setting
>> + *
>> + * Searches for the first HS hub above the given device. If a
>> + * HS hub is found, the hub address and the port the device is
>> + * connected to is return, as required for SPLIT transactions
>> + *
>> + * @param: udev full speed or low speed device
>> + */
>> +void usb_find_usb2_hub_address_port(struct usb_device *udev,
>> +                    uint8_t *hub_address, uint8_t *hub_port);
>> +
>>   /**
>>    * usb_alloc_new_device() - Allocate a new device
>>    *
>>

[U-Boot] [PATCH 4/6] usb: dwc2: add helper function for setting SPLIT HC registers

[Stefan Brüns]

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 drivers/usb/host/dwc2.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
index 2d97546..6496fcf 100644
--- a/drivers/usb/host/dwc2.c
+++ b/drivers/usb/host/dwc2.c
@@ -495,10 +495,27 @@ static void dwc_otg_hc_init(struct dwc2_core_regs *regs, uint8_t hc_num,
 	 */
 	writel(hcchar, &hc_regs->hcchar);
 
-	/* Program the HCSPLIT register for SPLITs */
+	/* Program the HCSPLIT register, default to no SPLIT */
 	writel(0, &hc_regs->hcsplt);
 }
 
+static void dwc_otg_hc_init_split(struct dwc2_core_regs *regs,
+				  uint8_t hc_num, uint8_t hub_devnum,
+				  uint8_t hub_port, uint8_t complete_split)
+{
+	struct dwc2_hc_regs *hc_regs = &regs->hc_regs[hc_num];
+	uint32_t hcsplt = 0;
+
+	hcsplt = DWC2_HCSPLT_SPLTENA;
+	hcsplt |= hub_devnum << DWC2_HCSPLT_HUBADDR_OFFSET;
+	hcsplt |= hub_port << DWC2_HCSPLT_PRTADDR_OFFSET;
+	if (complete_split)
+		hcsplt |= 1 << DWC2_HCSPLT_COMPSPLT_OFFSET;
+
+	/* Program the HCSPLIT register for SPLITs */
+	writel(hcsplt, &hc_regs->hcsplt);
+}
+
 /*
  * DWC2 to USB API interface
  */
-- 
2.1.4

[U-Boot] [PATCH 4/6] usb: dwc2: add helper function for setting SPLIT HC registers

[Marek Vasut]

On Sunday, December 13, 2015 at 05:17:56 AM, Stefan Br?ns wrote:
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>

Commit message would be nice, otherwise:

Acked-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

[U-Boot] [PATCH 4/6] usb: dwc2: add helper function for setting SPLIT HC registers

[Stephen Warren]

On 12/12/2015 09:17 PM, Stefan Br?ns wrote:
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>

Commit description?

Acked-by: Stephen Warren <swarren@wwwdotorg.org>

[U-Boot] [PATCH 5/6] usb: dwc2: add support for SPLIT transactions

[Stefan Brüns]

In contrast to non-SPLIT transfers each transaction has to be submitted
as an individual chunk. Handling of ACK/NAk/NYET handshakes depends on
transaction (non-SPLIT/SSPLIT/CSPLIT), thus inline the HCINT flag handling.

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 drivers/usb/host/dwc2.c | 96 ++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 83 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
index 6496fcf..0bf3ee5 100644
--- a/drivers/usb/host/dwc2.c
+++ b/drivers/usb/host/dwc2.c
@@ -848,8 +848,7 @@ static int dwc2_eptype[] = {
 };
 
 int chunk_msg(struct dwc2_priv *priv, struct usb_device *dev,
-	      unsigned long pipe, int *pid, int in, void *buffer, int len,
-	      bool ignore_ack)
+	      unsigned long pipe, int *pid, int in, void *buffer, int len)
 {
 	struct dwc2_core_regs *regs = priv->regs;
 	struct dwc2_hc_regs *hc_regs = &regs->hc_regs[DWC2_HC_CHANNEL];
@@ -863,23 +862,50 @@ int chunk_msg(struct dwc2_priv *priv, struct usb_device *dev,
 	uint32_t xfer_len;
 	uint32_t num_packets;
 	int stop_transfer = 0;
+	uint32_t hctsiz;
+	uint32_t hcint;
+	uint32_t hcint_rem;
+	uint8_t do_split = 0;
+	uint8_t complete_split = 0;
+	uint8_t start_again = 0;
+	uint8_t hub_addr = 0;
+	uint8_t hub_port = 0;
 
 	debug("%s: msg: pipe %lx pid %d in %d len %d\n", __func__, pipe, *pid,
 	      in, len);
 
+	/* Initialize channel */
+	dwc_otg_hc_init(regs, DWC2_HC_CHANNEL, dev, devnum, ep, in,
+			eptype, max);
+
+	/* Check if the target is a FS/LS device behind a HS hub */
+	if (dev->speed != USB_SPEED_HIGH) {
+		uint32_t hprt0 = readl(&regs->hprt0);
+		if ((hprt0 & DWC2_HPRT0_PRTSPD_MASK) ==
+			DWC2_HPRT0_PRTSPD_HIGH) {
+			do_split = 1;
+			dwc_find_hub_address_port(dev, &hub_addr, &hub_port);
+		}
+	}
+
 	do {
-		/* Initialize channel */
-		dwc_otg_hc_init(regs, DWC2_HC_CHANNEL, dev, devnum, ep, in,
-				eptype, max);
+		/* Clear old interrupt conditions for this host channel. */
+		writel(0x3fff, &hc_regs->hcint);
+
+		if (do_split)
+			dwc_otg_hc_init_split(regs, DWC2_HC_CHANNEL, hub_addr,
+					      hub_port, complete_split);
 
 		xfer_len = len - done;
+		if (do_split && xfer_len > max)
+			xfer_len = max;
 		if (xfer_len > CONFIG_DWC2_MAX_TRANSFER_SIZE)
 			xfer_len = CONFIG_DWC2_MAX_TRANSFER_SIZE - max + 1;
 		if (xfer_len > DWC2_DATA_BUF_SIZE)
 			xfer_len = DWC2_DATA_BUF_SIZE - max + 1;
 
 		/* Make sure that xfer_len is a multiple of max packet size. */
-		if (xfer_len > 0) {
+		if (xfer_len > max) {
 			num_packets = (xfer_len + max - 1) / max;
 			if (num_packets > CONFIG_DWC2_MAX_PACKET_COUNT) {
 				num_packets = CONFIG_DWC2_MAX_PACKET_COUNT;
@@ -918,10 +944,54 @@ int chunk_msg(struct dwc2_priv *priv, struct usb_device *dev,
 				(1 << DWC2_HCCHAR_MULTICNT_OFFSET) |
 				DWC2_HCCHAR_CHEN);
 
-		ret = wait_for_chhltd(regs, &sub, pid, ignore_ack);
+		ret = wait_for_bit(&hc_regs->hcint, DWC2_HCINT_CHHLTD, true);
 		if (ret)
 			break;
 
+		hcint = readl(&hc_regs->hcint);
+		hcint_rem = hcint & ~(DWC2_HCINT_XFERCOMP | DWC2_HCINT_CHHLTD);
+
+		hctsiz = readl(&hc_regs->hctsiz);
+		sub = (hctsiz & DWC2_HCTSIZ_XFERSIZE_MASK) >>
+			DWC2_HCTSIZ_XFERSIZE_OFFSET;
+		*pid = (hctsiz & DWC2_HCTSIZ_PID_MASK) >>
+			DWC2_HCTSIZ_PID_OFFSET;
+
+		start_again = 0;
+		if (complete_split) {
+			complete_split = 0;
+			if (hcint_rem & DWC2_HCINT_NYET) {
+				hcint_rem &= ~DWC2_HCINT_NYET;
+				start_again = 1;
+			}
+		} else if (do_split) {
+			if (hcint_rem & DWC2_HCINT_NAK) {
+				hcint_rem &= ~DWC2_HCINT_NAK;
+				/* should never happen, as there are no
+				 * concurrent transactions */
+				printf("TT busy\n");
+				ret = -EINVAL;
+			} else if (hcint_rem & DWC2_HCINT_ACK) {
+				complete_split = 1;
+				start_again = 1;
+			}
+		}
+
+		if (start_again) {
+			sub = 0;
+			xfer_len = 0;
+		}
+
+		if (hcint_rem & ~(DWC2_HCINT_NAK | DWC2_HCINT_FRMOVRUN |
+			DWC2_HCINT_ACK)) {
+			debug("%s: Error (HCINT=%08x)\n", __func__, hcint);
+			ret = -EINVAL;
+			break;
+		} else if (hcint_rem & (DWC2_HCINT_NAK | DWC2_HCINT_FRMOVRUN)) {
+			ret = -EAGAIN;
+			break;
+		}
+
 		if (in) {
 			xfer_len -= sub;
 
@@ -930,13 +1000,13 @@ int chunk_msg(struct dwc2_priv *priv, struct usb_device *dev,
 				roundup(xfer_len, ARCH_DMA_MINALIGN)));
 
 			memcpy(buffer + done, priv->aligned_buffer, xfer_len);
-			if (sub)
+			if (sub) /* short transfer/ZLP */
 				stop_transfer = 1;
 		}
 
 		done += xfer_len;
 
-	} while ((done < len) && !stop_transfer);
+	} while (((done < len) && !stop_transfer) || start_again);
 
 	writel(0, &hc_regs->hcintmsk);
 	writel(0xFFFFFFFF, &hc_regs->hcint);
@@ -960,7 +1030,7 @@ int _submit_bulk_msg(struct dwc2_priv *priv, struct usb_device *dev,
 	}
 
 	return chunk_msg(priv, dev, pipe, &priv->bulk_data_toggle[devnum][ep],
-			 usb_pipein(pipe), buffer, len, true);
+			 usb_pipein(pipe), buffer, len);
 }
 
 static int _submit_control_msg(struct dwc2_priv *priv, struct usb_device *dev,
@@ -980,7 +1050,7 @@ static int _submit_control_msg(struct dwc2_priv *priv, struct usb_device *dev,
 	}
 
 	pid = DWC2_HC_PID_SETUP;
-	ret = chunk_msg(priv, dev, pipe, &pid, 0, setup, 8, true);
+	ret = chunk_msg(priv, dev, pipe, &pid, 0, setup, 8);
 	if (ret)
 		return ret;
 
@@ -995,7 +1065,7 @@ static int _submit_control_msg(struct dwc2_priv *priv, struct usb_device *dev,
 				return -ETIMEDOUT;
 			}
 			ret = chunk_msg(priv, dev, pipe, &pid, usb_pipein(pipe),
-					buffer, len, false);
+					buffer, len);
 			act_len += dev->act_len;
 			buffer += dev->act_len;
 			len -= dev->act_len;
@@ -1013,7 +1083,7 @@ static int _submit_control_msg(struct dwc2_priv *priv, struct usb_device *dev,
 	pid = DWC2_HC_PID_DATA1;
 	do {
 		ret = chunk_msg(priv, dev, pipe, &pid, status_direction,
-				priv->status_buffer, 0, false);
+				priv->status_buffer, 0);
 	} while (ret == -EAGAIN);
 	if (ret)
 		return ret;
-- 
2.1.4

[U-Boot] [PATCH 5/6] usb: dwc2: add support for SPLIT transactions

[Marek Vasut]

On Sunday, December 13, 2015 at 05:17:57 AM, Stefan Br?ns wrote:
> In contrast to non-SPLIT transfers each transaction has to be submitted
> as an individual chunk. Handling of ACK/NAk/NYET handshakes depends on
> transaction (non-SPLIT/SSPLIT/CSPLIT), thus inline the HCINT flag handling.
> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> ---
>  drivers/usb/host/dwc2.c | 96
> ++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 83
> insertions(+), 13 deletions(-)
> 

Reviewed-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

[U-Boot] [PATCH 5/6] usb: dwc2: add support for SPLIT transactions

[Stephen Warren]

On 12/12/2015 09:17 PM, Stefan Br?ns wrote:
> In contrast to non-SPLIT transfers each transaction has to be submitted
> as an individual chunk. Handling of ACK/NAk/NYET handshakes depends on
> transaction (non-SPLIT/SSPLIT/CSPLIT), thus inline the HCINT flag handling.

> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c

>  int chunk_msg(struct dwc2_priv *priv, struct usb_device *dev,
> -	      unsigned long pipe, int *pid, int in, void *buffer, int len,
> -	      bool ignore_ack)
> +	      unsigned long pipe, int *pid, int in, void *buffer, int len)
...
> +	uint32_t hctsiz;
> +	uint32_t hcint;
> +	uint32_t hcint_rem;
> +	uint8_t do_split = 0;
> +	uint8_t complete_split = 0;
> +	uint8_t start_again = 0;
> +	uint8_t hub_addr = 0;
> +	uint8_t hub_port = 0;

Rather than inlining all this stuff into chunk_msg, I had always
intended to move the body of chunk_msg() into a new function e.g.
split_msg() that chunk_msg() called repeatedly for each chunk, with
split_msg() either performing just a single transaction, or performing
both a start/complete-split. That would keep the functions a bit simpler
and more focused.

Still, you've implemented this, so you get to choose how it works:-)

> +	/* Initialize channel */
> +	dwc_otg_hc_init(regs, DWC2_HC_CHANNEL, dev, devnum, ep, in,
> +			eptype, max);

I'd suggest calling that for each transaction. Sure there's some
redundancy, but I think it's better than duplicating the write of 0x3fff
to hc_regs->hc_int ther and within the loop here. If you disagree, I'd
suggest at least pulling that write out of dwc_otg_hc_init() so it's
only in one place.

>  	do {
...
> -	} while ((done < len) && !stop_transfer);
> +	} while (((done < len) && !stop_transfer) || start_again);

Perhaps just clear start_again if stop_transfer is set, or something
like that, to avoid the more complex condition?

Overall this looks reasonable though so,
Acked-by: Stephen Warren <swarren@wwwdotorg.org>

[U-Boot] [PATCH 5/6] usb: dwc2: add support for SPLIT transactions

[Stefan Bruens]

On Tuesday 15 December 2015 20:36:02 Stephen Warren wrote:
> On 12/12/2015 09:17 PM, Stefan Br?ns wrote:
> > In contrast to non-SPLIT transfers each transaction has to be submitted
> > as an individual chunk. Handling of ACK/NAk/NYET handshakes depends on
> > transaction (non-SPLIT/SSPLIT/CSPLIT), thus inline the HCINT flag
> > handling.
> > 
> > diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
> > 
> >  int chunk_msg(struct dwc2_priv *priv, struct usb_device *dev,
> > 
> > -	      unsigned long pipe, int *pid, int in, void *buffer, int len,
> > -	      bool ignore_ack)
> > +	      unsigned long pipe, int *pid, int in, void *buffer, int len)
> 
> ...
> 
> > +	uint32_t hctsiz;
> > +	uint32_t hcint;
> > +	uint32_t hcint_rem;
> > +	uint8_t do_split = 0;
> > +	uint8_t complete_split = 0;
> > +	uint8_t start_again = 0;
> > +	uint8_t hub_addr = 0;
> > +	uint8_t hub_port = 0;
> 
> Rather than inlining all this stuff into chunk_msg, I had always
> intended to move the body of chunk_msg() into a new function e.g.
> split_msg() that chunk_msg() called repeatedly for each chunk, with
> split_msg() either performing just a single transaction, or performing
> both a start/complete-split. That would keep the functions a bit simpler
> and more focused.

I will try to restructure the code a little bit to move the body of the loop 
to a seperate function ...

Kind regards,

Stefan

-- 
Stefan Br?ns  /  Bergstra?e 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019
work: +49 2405 49936-424

[U-Boot] [PATCH 5/6] usb: dwc2: add support for SPLIT transactions

[Marek Vasut]

On Sunday, December 20, 2015 at 01:17:05 AM, Stefan Bruens wrote:
> On Tuesday 15 December 2015 20:36:02 Stephen Warren wrote:
> > On 12/12/2015 09:17 PM, Stefan Br?ns wrote:
> > > In contrast to non-SPLIT transfers each transaction has to be submitted
> > > as an individual chunk. Handling of ACK/NAk/NYET handshakes depends on
> > > transaction (non-SPLIT/SSPLIT/CSPLIT), thus inline the HCINT flag
> > > handling.
> > > 
> > > diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
> > > 
> > >  int chunk_msg(struct dwc2_priv *priv, struct usb_device *dev,
> > > 
> > > -	      unsigned long pipe, int *pid, int in, void *buffer, int len,
> > > -	      bool ignore_ack)
> > > +	      unsigned long pipe, int *pid, int in, void *buffer, int len)
> > 
> > ...
> > 
> > > +	uint32_t hctsiz;
> > > +	uint32_t hcint;
> > > +	uint32_t hcint_rem;
> > > +	uint8_t do_split = 0;
> > > +	uint8_t complete_split = 0;
> > > +	uint8_t start_again = 0;
> > > +	uint8_t hub_addr = 0;
> > > +	uint8_t hub_port = 0;
> > 
> > Rather than inlining all this stuff into chunk_msg, I had always
> > intended to move the body of chunk_msg() into a new function e.g.
> > split_msg() that chunk_msg() called repeatedly for each chunk, with
> > split_msg() either performing just a single transaction, or performing
> > both a start/complete-split. That would keep the functions a bit simpler
> > and more focused.
> 
> I will try to restructure the code a little bit to move the body of the
> loop to a seperate function ...

Please send it as a whole series, this constant in-reply-to stuff is a complete
mess and it is impossible to track the patches.

Thanks

Best regards,
Marek Vasut

[U-Boot] [PATCH 6/6] usb: dwc2: remove no longer used wait_for_chhltd()

[Stefan Brüns]

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 drivers/usb/host/dwc2.c | 34 ----------------------------------
 1 file changed, 34 deletions(-)

diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
index 0bf3ee5..baf78d8 100644
--- a/drivers/usb/host/dwc2.c
+++ b/drivers/usb/host/dwc2.c
@@ -806,40 +806,6 @@ static int dwc_otg_submit_rh_msg(struct dwc2_priv *priv, struct usb_device *dev,
 	return stat;
 }
 
-int wait_for_chhltd(struct dwc2_core_regs *regs, uint32_t *sub, int *toggle,
-		    bool ignore_ack)
-{
-	uint32_t hcint_comp_hlt_ack = DWC2_HCINT_XFERCOMP | DWC2_HCINT_CHHLTD;
-	struct dwc2_hc_regs *hc_regs = &regs->hc_regs[DWC2_HC_CHANNEL];
-	int ret;
-	uint32_t hcint, hctsiz;
-
-	ret = wait_for_bit(&hc_regs->hcint, DWC2_HCINT_CHHLTD, true);
-	if (ret)
-		return ret;
-
-	hcint = readl(&hc_regs->hcint);
-	if (hcint & (DWC2_HCINT_NAK | DWC2_HCINT_FRMOVRUN))
-		return -EAGAIN;
-	if (ignore_ack)
-		hcint &= ~DWC2_HCINT_ACK;
-	else
-		hcint_comp_hlt_ack |= DWC2_HCINT_ACK;
-	if (hcint != hcint_comp_hlt_ack) {
-		debug("%s: Error (HCINT=%08x)\n", __func__, hcint);
-		return -EINVAL;
-	}
-
-	hctsiz = readl(&hc_regs->hctsiz);
-	*sub = (hctsiz & DWC2_HCTSIZ_XFERSIZE_MASK) >>
-		DWC2_HCTSIZ_XFERSIZE_OFFSET;
-	*toggle = (hctsiz & DWC2_HCTSIZ_PID_MASK) >> DWC2_HCTSIZ_PID_OFFSET;
-
-	debug("%s: sub=%u toggle=%d\n", __func__, *sub, *toggle);
-
-	return 0;
-}
-
 static int dwc2_eptype[] = {
 	DWC2_HCCHAR_EPTYPE_ISOC,
 	DWC2_HCCHAR_EPTYPE_INTR,
-- 
2.1.4

[U-Boot] [PATCH 6/6] usb: dwc2: remove no longer used wait_for_chhltd()

[Marek Vasut]

On Sunday, December 13, 2015 at 05:17:58 AM, Stefan Br?ns wrote:
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>

Acked-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

[U-Boot] [PATCH 6/6] usb: dwc2: remove no longer used wait_for_chhltd()

[Stephen Warren]

On 12/12/2015 09:17 PM, Stefan Br?ns wrote:
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>

Commit description?

Acked-by: Stephen Warren <swarren@wwwdotorg.org>

[U-Boot] [PATCH 0/6] usb: dwc2: add SPLIT transaction support

[Marek Vasut]

On Sunday, December 13, 2015 at 05:17:52 AM, Stefan Br?ns wrote:
> First two patches are preparatory work. 3rd and 4th introduce helper
> functions. Patch 5 contains the core changes. Patch 6 is cleanup.
> 
> The patch has been tested on RPi 1 B with several full speed and low speed
> devices. All devices enumerated successfully, with good output from
> "usb info". High speed devices, e.g. the SMSC9514 ethernet and mass storage
> devices still work.
> 
> Neither CONFIG_DM_USB nor LS/FS devices directly connected to the root
> hub/hc have been tested, although both should be handled by the patches.

On SoCFPGA Cyclone V (SoCkit), with CONFIG_DM_USB enabled:

Tested-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

[U-Boot] [PATCH 0/6] usb: dwc2: add SPLIT transaction support

[Stephen Warren]

On 12/12/2015 09:17 PM, Stefan Br?ns wrote:
> First two patches are preparatory work. 3rd and 4th introduce helper
> functions. Patch 5 contains the core changes. Patch 6 is cleanup.
> 
> The patch has been tested on RPi 1 B with several full speed and low speed
> devices. All devices enumerated successfully, with good output from
> "usb info". High speed devices, e.g. the SMSC9514 ethernet and mass storage
> devices still work.
> 
> Neither CONFIG_DM_USB nor LS/FS devices directly connected to the root hub/hc
> have been tested, although both should be handled by the patches.

The series,
Tested-by: Stephen Warren <swarren@wwwdotorg.org>

It's not perfect with the USB keyboards that I have (there are lots of
rapid extra keyboard repeats), but it basically does work and so I think
warrants the Tested-by. The extra repeats might be due to the strange
hacks I put into dwc2.c:_submit_int_msg() to try and handle the NAKing
of HID requests until the polling period has expired.