[U-Boot] [PATCH 0/6] usb: dwc2: add SPLIT transaction support
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(-)
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),
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
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?
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
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
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;
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 */
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;- }
Otherwise: Acked-by: Marek Vasut marex@denx.de
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?
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
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
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
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?
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
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 *
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
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;elsebreak;- /* 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
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
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
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 *
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 }
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; }
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
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
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
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:
- Dynamic allocation of configuration descriptor (1 patch)
- 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
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 *
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; }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;elsebreak;- /* 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
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; }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;elsebreak;- /* 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
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 = ®s->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 */
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
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
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 = ®s->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(®s->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;
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
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
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
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
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 = ®s->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,
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
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
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
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.
participants (5)
-
Hans de Goede -
Marek Vasut -
Stefan Bruens -
Stefan Brüns
-
Stephen Warren