usb: dwc2: add SPLIT transaction support
Subsystems (2)
cover 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(-)
Signature · 1 line
2.1.4
PATCH 1/6 usb: dwc2: avoid out of bounds access
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
--- drivers/usb/host/dwc2.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/usb/host/dwc2.c +4 -3
@@ -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),
Signature · 1 line
2.1.4
PATCH 1/6 usb: dwc2: avoid out of bounds access
Quoted 5 lines · Stefan Br?ns
flush_dcache_rangemay access data afterpriv->aligned_bufferend if len >DWC2_DATA_BUF_SIZE.
memcpy may access data after buffer end if done > 0Signed-off-by Stefan Br?ns <[email protected]>
Best regards,
Marek Vasut
PATCH 1/6 usb: dwc2: avoid out of bounds access
In reply to Stefan BrünsQuoted 3 lines · Stefan Br?ns
flush_dcache_rangemay access data afterpriv->aligned_bufferend if len >DWC2_DATA_BUF_SIZE.
memcpy may access data after buffer end if done > 0
Uggh; icky bug:-(
drivers/usb/host/dwc2.c int chunk_msg(struct dwc2_priv *priv, struct usb_device *dev, · line 826
Quoted 7 lines
drivers/usb/host/dwc2.c +1
-2@@ -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?
PATCH 1/6 usb: dwc2: avoid out of bounds access
Quoted 9 lines · Stephen Warren
Quoted 3 lines · Stefan Br?ns
flush_dcache_rangemay access data afterpriv->aligned_bufferend if len >DWC2_DATA_BUF_SIZE.
memcpy may access data after buffer end if done > 0Acked-by Stephen Warren <[email protected]>Uggh; icky bug:-(
Quoted 8 lines
drivers/usb/host/dwc2.c int chunk_msg(struct dwc2_priv *priv, struct · line 829
Quoted 8 lines
drivers/usb/host/dwc2.c +0
-2@@ -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);
Quoted 3 lines
drivers/usb/host/dwc2.c int chunk_msg(struct dwc2_priv *priv, struct usb_device *dev, · line 826
Quoted 1 line
drivers/usb/host/dwc2.c +1
-0+if (!in && xfer_len) {Do zero-length memcpy or
flush_dcache_rangeactually cause an issue?
I believe they should not, based on how they are implemented.
Best regards,
Marek Vasut
PATCH 1/6 usb: dwc2: avoid out of bounds access
Quoted 5 lines · On Wednesday 16 December 2015 11:29:14 Marek Vasut wrote:
Quoted 3 lines
drivers/usb/host/dwc2.c int chunk_msg(struct dwc2_priv *priv, struct usb_device *dev, · line 826
Quoted 1 line
drivers/usb/host/dwc2.c +1
-0+if (!in && xfer_len) {Do zero-length memcpy or
flush_dcache_rangeactually 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
Signature · 3 lines
Stefan Br?ns / Bergstra?e 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 work: +49 2405 49936-424
PATCH 2/6 usb: dwc2: Handle NAK during CONTROL DATA and STATUS stage
In reply to Stefan BrünsA 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.
--- drivers/usb/host/dwc2.c | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-)
drivers/usb/host/dwc2.c +24 -13
@@ -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;
Signature · 1 line
2.1.4
PATCH 2/6 usb: dwc2: Handle NAK during CONTROL DATA and STATUS stage
Quoted 35 lines · Stefan Br?ns
A function is allowed to return NAKs during the DATA stage to control data flow control. NAKs during the STATUS stage signal the function is still processing the request.
Signed-off-by Stefan Br?ns <[email protected]>--- drivers/usb/host/dwc2.c | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-)drivers/usb/host/dwc2.c +5
-3@@ -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 {
drivers/usb/host/dwc2.c static int _submit_control_msg(struct dwc2_priv · line 925
Quoted 10 lines
drivers/usb/host/dwc2.c +10
-0+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.
drivers/usb/host/dwc2.c static int _submit_control_msg(struct dwc2_priv *priv, struct usb_device *dev, · line 933
Quoted 11 lines
drivers/usb/host/dwc2.c +5
-3if (ret)return ret; - act_len = dev->act_len; - }/* End of DATA stage */ -else + status_direction =usb_pipeout (pipe); + }else { +/* No-data CONTROL always ends with an IN transaction */ + status_direction =1 ; act_len =0 ; + }
Otherwise:
PATCH 2/6 usb: dwc2: Handle NAK during CONTROL DATA and STATUS stage
In reply to Stefan BrünsQuoted 3 lines · Stefan Br?ns
A function is allowed to return NAKs during the DATA stage to control data flow control. NAKs during the STATUS stage signal the function is still processing the request.
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.
drivers/usb/host/dwc2.c static int _submit_control_msg(struct dwc2_priv *priv, struct usb_device *dev, · line 912
Quoted 9 lines
drivers/usb/host/dwc2.c +2
-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 */
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?
drivers/usb/host/dwc2.c static int _submit_control_msg(struct dwc2_priv *priv, struct usb_device *dev, · line 914
Quoted 4 lines
drivers/usb/host/dwc2.c +1
-2pid =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...
drivers/usb/host/dwc2.c static int _submit_control_msg(struct dwc2_priv *priv, struct usb_device *dev, · line 924
Quoted 10 lines
drivers/usb/host/dwc2.c +10
-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;
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.
drivers/usb/host/dwc2.c static int _submit_control_msg(struct dwc2_priv *priv, struct usb_device *dev, · line 942
Quoted 9 lines
drivers/usb/host/dwc2.c +4
-2pid =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?
PATCH 2/6 usb: dwc2: Handle NAK during CONTROL DATA and STATUS stage
Quoted 10 lines · On Tuesday 15 December 2015 20:07:26 Stephen Warren wrote:
Quoted 3 lines · Stefan Br?ns
A function is allowed to return NAKs during the DATA stage to control data flow control. NAKs during the STATUS stage signal the function is still processing the request.
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
Quoted 12 lines
Quoted 12 lines
drivers/usb/host/dwc2.c +1
-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) {
Quoted 7 lines
drivers/usb/host/dwc2.c static int _submit_control_msg(struct dwc2_priv *priv, struct usb_device *dev, · line 912
Quoted 1 line
drivers/usb/host/dwc2.c +1
-0+/* 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.
Quoted 8 lines
drivers/usb/host/dwc2.c static int _submit_control_msg(struct dwc2_priv *priv, struct usb_device *dev, · line 914
Quoted 5 lines
drivers/usb/host/dwc2.c +1
-2pid =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...
Quoted 14 lines
drivers/usb/host/dwc2.c static int _submit_control_msg(struct dwc2_priv *priv, struct usb_device *dev, · line 924
Quoted 10 lines
drivers/usb/host/dwc2.c +10
-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;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.
Quoted 9 lines
Quoted 9 lines
drivers/usb/host/dwc2.c +4
-2pid =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 );
Quoted 5 lines
drivers/usb/host/dwc2.c static int _submit_control_msg(struct dwc2_priv *priv, struct usb_device *dev, · line 927
Quoted 3 lines
drivers/usb/host/dwc2.c +0
-0if (ret)return ret;Shouldn't that last loop have a timeout too?
Correct, but see above.
Kind regards,
Stefan
Signature · 3 lines
Stefan Br?ns / Bergstra?e 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 work: +49 2405 49936-424
PATCH 3/6 usb: dwc2: determine TT hub address and port for split transactions
In reply to Stefan BrünsStart 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
--- drivers/usb/host/dwc2.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+)
drivers/usb/host/dwc2.c +60 -0
@@ -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
Signature · 1 line
2.1.4
PATCH 3/6 usb: dwc2: determine TT hub address and port for split transactions
Quoted 5 lines · Stefan Br?ns
Start split and complete split tokens need the hub address and the downstream port of the first HS hub (device view).
Code copied fromhost/ehci_hcd.cSigned-off-by Stefan Br?ns <[email protected]>
Best regards,
Marek Vasut
PATCH 3/6 usb: dwc2: determine TT hub address and port for split transactions
In reply to Stefan BrünsQuoted 3 lines · Stefan Br?ns
Start split and complete split tokens need the hub address and the downstream port of the first HS hub (device view).
Code copied fromhost/ehci_hcd.c
Rather than cut/paste this, shouldn't you create a common function that can be shared between all users?
PATCH 3/6 usb: dwc2: determine TT hub address and port for split transactions
Quoted 7 lines · On Tuesday 15 December 2015 20:17:32 Stephen Warren wrote:
Quoted 3 lines · Stefan Br?ns
Start split and complete split tokens need the hub address and the downstream port of the first HS hub (device view).
Code copied fromhost/ehci_hcd.cRather 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
Signature · 3 lines
Stefan Br?ns / Bergstra?e 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 work: +49 2405 49936-424
PATCH 1/1 usb: Move determination of TT hub address/port into seperate function
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.
---
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(-)
common/usb.c +54 -0
@@ -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 */
drivers/usb/host/ehci-hcd.c +5 -45
61 lines not on this page · raw message
drivers/usb/musb-new/musb_host.c +5 -3
@@ -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 }
drivers/usb/musb-new/usb-compat.h +0 -53
65 lines not on this page · raw message
include/usb.h +10 -0
@@ -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 *
Signature · 1 line
2.1.4
PATCH 1/1 usb: Move determination of TT hub address/port into seperate function
Quoted 41 lines · Stefan Br?ns
Start split and complete split tokens need the hub address and the downstream port of the first HS hub (device view).
The core of the function was duplicated in both host/ehci_hcd and
musb-new/usb-compat.h.Signed-off-by Stefan Br?ns <[email protected]>---
Obsoletes [PATCH 3/6] usb: dwc2: determine TT hub address and port for split transactionscommon/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(-)common/usb.c +13
-0@@ -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 .
common/usb.c bool usb_device_has_child_on_port(struct usb_device · line 1225
Quoted 10 lines
common/usb.c +10
-0+*/ + 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");
common/usb.c bool usb_device_has_child_on_port(struct usb_device · line 1226
Quoted 1 line
common/usb.c +1
-0+ *hub_address = *hub_port =0 ;
Please avoid this construct: foo = bar = 1234; , it's really confusing and error prone.
Otherwise looks good!
PATCH 1/1 usb: Move determination of TT hub address/port into seperate function
In reply to Stefan BrünsHi,
Quoted 7 lines · Stefan Br?ns
Start split and complete split tokens need the hub address and the downstream port of the first HS hub (device view).
The core of the function was duplicated in both host/ehci_hcd and
musb-new/usb-compat.h.Signed-off-by Stefan Br?ns <[email protected]>
Thanks for working on this, I think I've spotted one small bug though, see comments inline.
Quoted 41 lines
---
Obsoletes [PATCH 3/6] usb: dwc2: determine TT hub address and port for split transactionscommon/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(-)common/usb.c +23
-0@@ -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" );
common/usb.c bool usb_device_has_child_on_port(struct usb_device *parent, int port) · line 1235
Quoted 10 lines
common/usb.c +10
-0+ *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.
common/usb.c bool usb_device_has_child_on_port(struct usb_device *parent, int port) · line 1247
Quoted 12 lines
common/usb.c +12
-0+} +#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.
Quoted 66 lines
common/usb.c +9
-0+ return; + } + +printf ("Error: Cannot find high speed parent of usb-1 device\n" ); + *hub_address = *hub_port =0 ; +} +#endif + +/* EOF */ drivers/usb/host/ehci-hcd.c +2
-42@@ -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;
drivers/usb/host/ehci-hcd.c static inline u8 ehci_encode_speed(enum usb_device_speed speed) · line 294
Quoted 10 lines
drivers/usb/host/ehci-hcd.c +3
-3-#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>
Quoted 30 lines
drivers/usb/musb-new/usb-compat.h +0
-22@@ -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 ;
drivers/usb/musb-new/usb-compat.h static inline int usb_hcd_unmap_urb_for_dma(struct usb_hcd *hcd, · line 102
Quoted 10 lines
drivers/usb/musb-new/usb-compat.h +0
-10- } - - 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".
Quoted 18 lines
drivers/usb/musb-new/usb-compat.h +0
-11static 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 ;
drivers/usb/musb-new/usb-compat.h static inline struct usb_device *usb_dev_get_parent(struct usb_device *udev) · line 152
Quoted 10 lines
drivers/usb/musb-new/usb-compat.h +0
-10- -/* 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).
Quoted 12 lines
drivers/usb/musb-new/usb-compat.h +0
-0static inline struct usb_device *usb_dev_get_parent (struct usb_device *dev) {return dev->parent;drivers/usb/musb-new/musb_host.c +0
-1@@ -2092,9 +2092,11 @@ int musb_urb_enqueue( }#else if (tt_needed (musb, urb->dev)) { -u16 hub_port =find_tt (urb->dev);
drivers/usb/musb-new/musb_host.c int musb_urb_enqueue( · line 2102
Quoted 10 lines
drivers/usb/musb-new/musb_host.c +5
-2- 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.
Quoted 12 lines
include/usb.h +3
-0@@ -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
include/usb.h int legacy_hub_port_reset(struct usb_device *dev, int port, · line 889
Quoted 10 lines
include/usb.h +7
-0+* 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
PATCH 1/1 usb: Move determination of TT hub address/port into seperate function
Quoted 6 lines · On Friday 18 December 2015 11:00:19 Hans de Goede wrote:
Hi,
Quoted 3 lines · Stefan Br?ns
Start split and complete split tokens need the hub address and the downstream port of the first HS hub (device view).
Quoted 7 lines
Quoted 4 lines
The core of the function was duplicated in both host/ehci_hcd and
musb-new/usb-compat.h.Signed-off-by Stefan Br?ns <[email protected]>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.
Quoted 2 lines
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
Signature · 3 lines
Stefan Br?ns / Bergstra?e 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 work: +49 2405 49936-424
PATCH 1/1 usb: Move determination of TT hub address/port into seperate function
Hi,
Quoted 10 lines · Stefan Bruens
Quoted 9 lines · On Friday 18 December 2015 11:00:19 Hans de Goede wrote:
Hi,
Quoted 6 lines · Stefan Br?ns
Start split and complete split tokens need the hub address and the downstream port of the first HS hub (device view).
The core of the function was duplicated in both host/ehci_hcd and
musb-new/usb-compat.h.
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Stefan Br?ns <[email protected]>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:
Quoted 10 lines
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.
Quoted 8 lines
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.
Quoted 1 line
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
PATCH v2 1/2 usb: Move determination of TT hub address/port into seperate function
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.
---
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(-)
common/usb.c +54 -0
@@ -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 */
drivers/usb/host/ehci-hcd.c +5 -45
61 lines not on this page · raw message
drivers/usb/musb-new/musb_host.c +5 -3
@@ -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 }
drivers/usb/musb-new/usb-compat.h +0 -53
65 lines not on this page · raw message
include/usb.h +12 -0
@@ -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 *
Signature · 1 line
2.1.4
PATCH v2 2/2 usb: musb: Fix hub port number for SPLIT transactions
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.
--- drivers/usb/musb-new/musb_host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/musb-new/musb_host.c +1 -1
@@ -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 }
Signature · 1 line
2.1.4
PATCH v2 2/2 usb: musb: Fix hub port number for SPLIT transactions
Hi,
Quoted 30 lines · Stefan Br?ns
The ifdef'ed Linux kernel code uses the 1 based port number, whereas U-Boot puts a 0 based port number into the register. The reason the 0 based port number apparently works can probably be taken from the USB 2.0 spec:
8.4.2.2 Start-Split Transaction Token
... The host must correctly set the port field for single and multiple TT hub implementations. A single TT hub implementation *may ignore* the port field.Actually, as far as I unterstand, a multi TT hub defaults to single TT (bAlternateSetting: 0) until switched via SetInterface, so even "port 42" would work.
The change was verified by hardcoding the port number to a wrong value, SPLIT transactions kept working (although using a DWC2 instead of MUSB). Tested hubs are the RPi onboard SMC9514 and an external "05e3:0608 Genesys Logic, Inc. USB-2.0 4-Port HUB". The former is a multi TT hub, the latter single TT only.
Signed-off-by Stefan Br?ns <[email protected]>--- drivers/usb/musb-new/musb_host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/usb/musb-new/musb_host.c +0
-0
drivers/usb/musb-new/musb_host.c int musb_urb_enqueue( · line 2102
Quoted 8 lines
drivers/usb/musb-new/musb_host.c +1
-1@@ -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:
Regards,
Hans
PATCH v2 2/2 usb: musb: Fix hub port number for SPLIT transactions
Quoted 29 lines · Hans de Goede
Hi,
Quoted 26 lines · Stefan Br?ns
The ifdef'ed Linux kernel code uses the 1 based port number, whereas U-Boot puts a 0 based port number into the register. The reason the 0 based port number apparently works can probably be taken from the USB 2.0 spec:
8.4.2.2 Start-Split Transaction Token
... The host must correctly set the port field for single and multiple TT hub implementations. A single TT hub implementation *may ignore* the port field.Actually, as far as I unterstand, a multi TT hub defaults to single TT (bAlternateSetting: 0) until switched via SetInterface, so even "port 42" would work.
The change was verified by hardcoding the port number to a wrong value, SPLIT transactions kept working (although using a DWC2 instead of MUSB). Tested hubs are the RPi onboard SMC9514 and an external "05e3:0608 Genesys Logic, Inc. USB-2.0 4-Port HUB". The former is a multi TT hub, the latter single TT only.
Signed-off-by Stefan Br?ns <[email protected]>---
drivers/usb/musb-new/musb_host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 8 lines
drivers/usb/musb-new/musb_host.c int musb_urb_enqueue( · line 2098
Quoted 8 lines
(quoted diff) +0
-0
b/drivers/usb/musb-new/musb_host.cindex fc4ba62..5c028d4 100644drivers/usb/musb-new/musb_host.c +0
-0@@ -2096,7 +2096,7 @@ int musb_urb_enqueue(uint8_t hubaddr =0 ; usb_find_usb2_hub_address_port(udev, &hubaddr,
&portnr)
Quoted 9 lines
Quoted 9 lines
drivers/usb/musb-new/musb_host.c +1
-1qh->h_addr_reg = hubaddr; - qh->h_port_reg = portnr -1 ; + qh->h_port_reg = portnr;} #endif
Quoted 7 lines
Quoted 1 line
}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 <[email protected]>Tested-by Hans de Goede <[email protected]>
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
PATCH v2 2/2 usb: musb: Fix hub port number for SPLIT transactions
Quoted 10 lines · On Monday 21 December 2015 21:27:01 Marek Vasut wrote:
Quoted 5 lines · Hans de Goede
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 <[email protected]>Tested-by Hans de Goede <[email protected]>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
Signature · 3 lines
Stefan Br?ns / Bergstra?e 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 work: +49 2405 49936-424
PATCH v2 2/2 usb: musb: Fix hub port number for SPLIT transactions
Quoted 16 lines · Stefan Bruens
Quoted 9 lines · On Monday 21 December 2015 21:27:01 Marek Vasut wrote:
Quoted 5 lines · Hans de Goede
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 <[email protected]>Tested-by Hans de Goede <[email protected]>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:
Quoted 6 lines
1. Dynamic allocation of configuration descriptor (1 patch)
2. Factor out function for hub address/port, fix musb port number (2 patches) 3. Fix out-of-bound access in DWC (1 patch)These four patches are the first 4 patches from the old 7 patch series, so have been reviewed.
That's fine, thanks!
Best regards,
Marek Vasut
PATCH v3 1/2 usb: Move determination of TT hub address/port into seperate function
In reply to Hans de GoedeStart 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.
---
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(-)
common/usb.c +56 -0
@@ -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 */
drivers/usb/host/ehci-hcd.c +5 -45
61 lines not on this page · raw message
drivers/usb/musb-new/musb_host.c +6 -3
@@ -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 }
drivers/usb/musb-new/usb-compat.h +0 -53
65 lines not on this page · raw message
include/usb.h +12 -0
@@ -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 *
Signature · 1 line
2.1.4
PATCH v3 1/2 usb: Move determination of TT hub address/port into seperate function
Hi,
Quoted 11 lines · Stefan Br?ns
Start split and complete split tokens need the hub address and the downstream port of the first HS hub (device view).
The core of the function was duplicated in both host/ehci_hcd and
musb-new/usb-compat.h.Signed-off-by Stefan Br?ns <[email protected]>---
v2: - renamed function tousb_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
Quoted 254 lines
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(-)common/usb.c +56
-0@@ -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 */ drivers/usb/host/ehci-hcd.c +5
-45@@ -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 intdrivers/usb/musb-new/musb_host.c +6
-3@@ -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 }drivers/usb/musb-new/usb-compat.h +0
-53@@ -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;include/usb.h +12
-0@@ -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 *
PATCH v3 1/2 usb: Move determination of TT hub address/port into seperate function
Hi,
Quoted 33 lines · Hans de Goede
Hi,
Quoted 11 lines · Stefan Br?ns
Start split and complete split tokens need the hub address and the downstream port of the first HS hub (device view).
The core of the function was duplicated in both host/ehci_hcd and
musb-new/usb-compat.h.Signed-off-by Stefan Br?ns <[email protected]>---
v2: - renamed function tousb_find_usb2_hub_address_port()
- put musb port numbering change into seperate patch
v3: - do one assignement per lineThanks, 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:
(quoted diff) +2
-1@@ -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;
Quoted 1 line
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:
Regards,
Hans
Quoted 260 lines
Regards,
Hans
Quoted 254 lines
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(-)common/usb.c +56
-0@@ -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 */ drivers/usb/host/ehci-hcd.c +5
-45@@ -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 intdrivers/usb/musb-new/musb_host.c +6
-3@@ -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 }drivers/usb/musb-new/usb-compat.h +0
-53@@ -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;include/usb.h +12
-0@@ -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 *
PATCH 4/6 usb: dwc2: add helper function for setting SPLIT HC registers
In reply to Stefan Brüns--- drivers/usb/host/dwc2.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)
drivers/usb/host/dwc2.c +18 -1
@@ -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 */
Signature · 1 line
2.1.4
PATCH 4/6 usb: dwc2: add helper function for setting SPLIT HC registers
Quoted 1 line · Stefan Br?ns
Signed-off-by Stefan Br?ns <[email protected]>
Commit message would be nice, otherwise:
Best regards,
Marek Vasut
PATCH 4/6 usb: dwc2: add helper function for setting SPLIT HC registers
In reply to Stefan BrünsQuoted 1 line · Stefan Br?ns
Signed-off-by Stefan Br?ns <[email protected]>
Commit description?
PATCH 5/6 usb: dwc2: add support for SPLIT transactions
In reply to Stefan BrünsIn 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.
--- drivers/usb/host/dwc2.c | 96 ++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 83 insertions(+), 13 deletions(-)
drivers/usb/host/dwc2.c +83 -13
@@ -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;
Signature · 1 line
2.1.4
PATCH 5/6 usb: dwc2: add support for SPLIT transactions
Quoted 10 lines · Stefan Br?ns
In contrast to non-SPLIT transfers each transaction has to be submitted as an individual chunk. Handling of ACK/NAk/NYET handshakes depends on transaction (non-SPLIT/SSPLIT/CSPLIT), thus inline the HCINT flag handling.
Signed-off-by Stefan Br?ns <[email protected]>--- drivers/usb/host/dwc2.c | 96++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 83 insertions(+), 13 deletions(-)
Best regards,
Marek Vasut
PATCH 5/6 usb: dwc2: add support for SPLIT transactions
In reply to Stefan BrünsQuoted 10 lines · Stefan Br?ns
In contrast to non-SPLIT transfers each transaction has to be submitted as an individual chunk. Handling of ACK/NAk/NYET handshakes depends on transaction (non-SPLIT/SSPLIT/CSPLIT), thus inline the HCINT flag handling.
drivers/usb/host/dwc2.c +1
-2int 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)
...
drivers/usb/host/dwc2.c int chunk_msg(struct dwc2_priv *priv, struct usb_device *dev, · line 872
Quoted 8 lines
drivers/usb/host/dwc2.c +8
-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 ;
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:-)
drivers/usb/host/dwc2.c int chunk_msg(struct dwc2_priv *priv, struct usb_device *dev, · line 879
Quoted 3 lines
drivers/usb/host/dwc2.c +3
-0+/* 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.
Quoted 1 line
do {
...
drivers/usb/host/dwc2.c int chunk_msg(struct dwc2_priv *priv, struct usb_device *dev, · line 1009
Quoted 2 lines
drivers/usb/host/dwc2.c +1
-1- }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,
PATCH 5/6 usb: dwc2: add support for SPLIT transactions
Quoted 16 lines · On Tuesday 15 December 2015 20:36:02 Stephen Warren wrote:
Quoted 12 lines · Stefan Br?ns
In contrast to non-SPLIT transfers each transaction has to be submitted as an individual chunk. Handling of ACK/NAk/NYET handshakes depends on transaction (non-SPLIT/SSPLIT/CSPLIT), thus inline the HCINT flag handling.
drivers/usb/host/dwc2.c +1
-2int 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)...
Quoted 15 lines
drivers/usb/host/dwc2.c int chunk_msg(struct dwc2_priv *priv, struct usb_device *dev, · line 872
Quoted 8 lines
drivers/usb/host/dwc2.c +8
-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 ;Rather than inlining all this stuff into
chunk_msg, I had always intended to move the body ofchunk_msg()into a new function e.g.split_msg()thatchunk_msg()called repeatedly for each chunk, withsplit_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
Signature · 3 lines
Stefan Br?ns / Bergstra?e 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 work: +49 2405 49936-424
PATCH 5/6 usb: dwc2: add support for SPLIT transactions
Quoted 33 lines · Stefan Bruens
Quoted 16 lines · On Tuesday 15 December 2015 20:36:02 Stephen Warren wrote:
Quoted 12 lines · Stefan Br?ns
In contrast to non-SPLIT transfers each transaction has to be submitted as an individual chunk. Handling of ACK/NAk/NYET handshakes depends on transaction (non-SPLIT/SSPLIT/CSPLIT), thus inline the HCINT flag handling.
drivers/usb/host/dwc2.c +1
-2int 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)...
Quoted 15 lines
drivers/usb/host/dwc2.c int chunk_msg(struct dwc2_priv *priv, struct usb_device *dev, · line 872
Quoted 8 lines
drivers/usb/host/dwc2.c +8
-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 ;Rather than inlining all this stuff into
chunk_msg, I had always intended to move the body ofchunk_msg()into a new function e.g.split_msg()thatchunk_msg()called repeatedly for each chunk, withsplit_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.
Quoted 2 lines
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
PATCH 6/6 usb: dwc2: remove no longer used wait_for_chhltd()
In reply to Stefan Brüns--- drivers/usb/host/dwc2.c | 34 ---------------------------------- 1 file changed, 34 deletions(-)
drivers/usb/host/dwc2.c +0 -34
@@ -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 ,
Signature · 1 line
2.1.4
PATCH 6/6 usb: dwc2: remove no longer used wait_for_chhltd()
Quoted 1 line · Stefan Br?ns
Signed-off-by Stefan Br?ns <[email protected]>
Best regards,
Marek Vasut
PATCH 6/6 usb: dwc2: remove no longer used wait_for_chhltd()
In reply to Stefan BrünsQuoted 1 line · Stefan Br?ns
Signed-off-by Stefan Br?ns <[email protected]>
Commit description?
cover usb: dwc2: add SPLIT transaction support
In reply to Stefan BrünsQuoted 10 lines · Stefan Br?ns
First two patches are preparatory work. 3rd and 4th introduce helper functions. Patch 5 contains the core changes. Patch 6 is cleanup.
The patch has been tested on RPi 1 B with several full speed and low speed devices. All devices enumerated successfully, with good output from "usb info". High speed devices, e.g. the SMSC9514 ethernet and mass storage devices still work.
Neither
CONFIG_DM_USBnor 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:
Best regards,
Marek Vasut
cover usb: dwc2: add SPLIT transaction support
In reply to Stefan BrünsQuoted 10 lines · Stefan Br?ns
First two patches are preparatory work. 3rd and 4th introduce helper functions. Patch 5 contains the core changes. Patch 6 is cleanup.
The patch has been tested on RPi 1 B with several full speed and low speed devices. All devices enumerated successfully, with good output from "usb info". High speed devices, e.g. the SMSC9514 ethernet and mass storage devices still work.
Neither
CONFIG_DM_USBnor LS/FS devices directly connected to the root hub/hc have been tested, although both should be handled by the patches.
The series,
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.