usb: dwc3: gadget: Skip resizing EP's TX FIFO if already resized
Subsystems (3)
PATCH 1/1 usb: dwc3: gadget: Skip resizing EP's TX FIFO if already resized
Some functions may dynamically enable and disable their endpoints regularly throughout their operation, particularly when Set Interface is employed to switch between Alternate Settings. For instance the UAC2 function has its respective endpoints for playback & capture associated with AltSetting 1, in which case those endpoints would not get enabled until the host activates the AltSetting. And they conversely become disabled when the interfaces' AltSetting 0 is chosen.
With the DWC3 FIFO resizing algorithm recently added, every usb_ep_enable() call results in a call to resize that EP's TXFIFO, but if the same endpoint is enabled again and again, this incorrectly leads to FIFO RAM allocation exhaustion as the mechanism did not account for the possibility that endpoints can be re-enabled many times.
Example log splat:
dwc3 a600000.dwc3: Fifosize(3717) > RAM size(3462) ep3in depth:217973127 configfs-gadget gadget: u_audio_start_capture:521 Error! dwc3 a600000.dwc3: request 000000000be13e18 was not queued to ep3in
This is easily fixed by bailing out of dwc3_gadget_resize_tx_fifos() if an endpoint is already resized, avoiding the calculation error resulting from accumulating the EP's FIFO depth repeatedly.
--- drivers/usb/dwc3/gadget.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/usb/dwc3/gadget.c +4 -0
@@ -747,6 +747,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep)if (!usb_endpoint_dir_in (dep->endpoint.desc) || dep->number <=1 )return 0 ; +/* bail if already resized */ +if (dwc3_readl (dwc->regs,DWC3_GTXFIFOSIZ (dep->number >>1 ))) +return 0 ; + ram1_depth =DWC3_RAM1_DEPTH (dwc->hwparams.hwparams7); if ((dep->endpoint.maxburst > 1 &&
Signature · 1 line
2.24.0
Hi,
Quoted 37 lines · Jack Pham
Some functions may dynamically enable and disable their endpoints regularly throughout their operation, particularly when Set Interface is employed to switch between Alternate Settings. For instance the UAC2 function has its respective endpoints for playback & capture associated with AltSetting 1, in which case those endpoints would not get enabled until the host activates the AltSetting. And they conversely become disabled when the interfaces' AltSetting 0 is chosen.
With the DWC3 FIFO resizing algorithm recently added, every
usb_ep_enable()call results in a call to resize that EP's TXFIFO, but if the same endpoint is enabled again and again, this incorrectly leads to FIFO RAM allocation exhaustion as the mechanism did not account for the possibility that endpoints can be re-enabled many times.Example log splat:
dwc3 a600000.dwc3: Fifosize(3717) > RAM size(3462) ep3in depth:217973127 configfs-gadget gadget: u_audio_start_capture:521 Error! dwc3 a600000.dwc3: request 000000000be13e18 was not queued to ep3inThis is easily fixed by bailing out of
dwc3_gadget_resize_tx_fifos()if an endpoint is already resized, avoiding the calculation error resulting from accumulating the EP's FIFO depth repeatedly.Fixes 9f607a309fbe9 ("usb: dwc3: Resize TX FIFOs to meet EP bursting requirements")Signed-off-by Jack Pham <[email protected]>--- drivers/usb/dwc3/gadget.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/usb/dwc3/gadget.c +0
-0
drivers/usb/dwc3/gadget.c static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) · line 753
Quoted 7 lines
drivers/usb/dwc3/gadget.c +4
-0@@ -747,6 +747,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep)if (!usb_endpoint_dir_in (dep->endpoint.desc) || dep->number <=1 )return 0 ; +/* bail if already resized */ +if (dwc3_readl (dwc->regs,DWC3_GTXFIFOSIZ (dep->number >>1 ))) +return 0 ; +
heh, not to say "I told you so", but...
That being said, your test is not very good. The whole idea for resizing the FIFOs is that in some applications we only use e.g. 2 endpoints and there is considerable FIFO space left unused.
The goal is to use that unused FIFO space to squeeze more throughput out of the pipe, since it amortizes SW latency.
This patch is essentially the same as reverting the original commit :-)
Signature · 1 line
balbi
Hi Felipe,
Quoted 49 lines · Felipe Balbi
Quoted 37 lines · Jack Pham
Some functions may dynamically enable and disable their endpoints regularly throughout their operation, particularly when Set Interface is employed to switch between Alternate Settings. For instance the UAC2 function has its respective endpoints for playback & capture associated with AltSetting 1, in which case those endpoints would not get enabled until the host activates the AltSetting. And they conversely become disabled when the interfaces' AltSetting 0 is chosen.
With the DWC3 FIFO resizing algorithm recently added, every
usb_ep_enable()call results in a call to resize that EP's TXFIFO, but if the same endpoint is enabled again and again, this incorrectly leads to FIFO RAM allocation exhaustion as the mechanism did not account for the possibility that endpoints can be re-enabled many times.Example log splat:
dwc3 a600000.dwc3: Fifosize(3717) > RAM size(3462) ep3in depth:217973127 configfs-gadget gadget: u_audio_start_capture:521 Error! dwc3 a600000.dwc3: request 000000000be13e18 was not queued to ep3inThis is easily fixed by bailing out of
dwc3_gadget_resize_tx_fifos()if an endpoint is already resized, avoiding the calculation error resulting from accumulating the EP's FIFO depth repeatedly.Fixes 9f607a309fbe9 ("usb: dwc3: Resize TX FIFOs to meet EP bursting requirements")Signed-off-by Jack Pham <[email protected]>--- drivers/usb/dwc3/gadget.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/usb/dwc3/gadget.c +0
-0drivers/usb/dwc3/gadget.c static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) · line 753
Quoted 7 lines
drivers/usb/dwc3/gadget.c +4
-0@@ -747,6 +747,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep)if (!usb_endpoint_dir_in (dep->endpoint.desc) || dep->number <=1 )return 0 ; +/* bail if already resized */ +if (dwc3_readl (dwc->regs,DWC3_GTXFIFOSIZ (dep->number >>1 ))) +return 0 ; +heh, not to say "I told you so", but...
Quoted 8 lines
That being said, your test is not very good. The whole idea for resizing the FIFOs is that in some applications we only use e.g. 2 endpoints and there is considerable FIFO space left unused.
The goal is to use that unused FIFO space to squeeze more throughput out of the pipe, since it amortizes SW latency.
This patch is essentially the same as reverting the original commit :-)
No, it's not quite the same as nullifying the resizing algorithm. This patch is predicated on a key part of the resizing algorithm where dwc3_gadget_clear_tx_fifos() occurs upon receiving Set_Configuration in ep0.c. Which means that each new connection starts off with a blank slate with all the GTXFIFOSIZ(n) registers cleared. Then each EP gets resized one at a time when usb_ep_enable() is called.
The problem this patch is fixing is avoiding *re-resizing*, the idea being that if an EP was already resized once during a session (from Set Configuration until the next reset or disconnect), then it should be good to go even if it gets disabled and re-enabled again. Since we lack any boolean state variable in struct dwc3_ep reflecting whether it had already been resized, re-reading the GTXFIFOSIZ register is the next best equivalent. Note also that this check occurs after the if (!dwc->do_fifo_resize) check so this is applicable only if the entire "tx-fifo-resize" mechanism is enabled.
Jack
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Hi Jack,
Quoted 31 lines · Jack Pham
Quoted 15 lines
drivers/usb/dwc3/gadget.c static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) · line 753
Quoted 12 lines
drivers/usb/dwc3/gadget.c +4
-0@@ -747,6 +747,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep)if (!usb_endpoint_dir_in (dep->endpoint.desc) || dep->number <=1 )return 0 ; +/* bail if already resized */ +if (dwc3_readl (dwc->regs,DWC3_GTXFIFOSIZ (dep->number >>1 ))) +return 0 ; +heh, not to say "I told you so", but...
Quoted 8 lines
That being said, your test is not very good. The whole idea for resizing the FIFOs is that in some applications we only use e.g. 2 endpoints and there is considerable FIFO space left unused.
The goal is to use that unused FIFO space to squeeze more throughput out of the pipe, since it amortizes SW latency.
This patch is essentially the same as reverting the original commit :-)
No, it's not quite the same as nullifying the resizing algorithm. This patch is predicated on a key part of the resizing algorithm where
dwc3_gadget_clear_tx_fifos()occurs upon receivingSet_Configurationinep0.c. Which means that each new connection starts off with a blank slate with all the GTXFIFOSIZ(n) registers cleared. Then each EP gets resized one at a time whenusb_ep_enable()is called.
Quoted 4 lines
The problem this patch is fixing is avoiding *re-resizing*, the idea being that if an EP was already resized once during a session (from Set Configuration until the next reset or disconnect), then it should be good to go even if it gets disabled and re-enabled again.
that's not a safe assumption, though. What happens in cases where Configuration 1 is wildly different from Configuration 2? Say Config 1 is a mass storage device and Config 2 is a collection of several CDC interfaces?
Quoted 2 lines
Since we lack any boolean state variable in struct
dwc3_epreflecting whether it had already been resized, re-reading the GTXFIFOSIZ register
it might be a better idea to introduce such a flag and make the intention clear. But in any case, I still think the assumption you're making is not very good.
Quoted 3 lines
is the next best equivalent. Note also that this check occurs after the if (!dwc->do_fifo_resize) check so this is applicable only if the entire "tx-fifo-resize" mechanism is enabled.
Right, that's fine :-)
Signature · 1 line
balbi
Quoted 40 lines · Felipe Balbi
Hi Jack,
Quoted 31 lines · Jack Pham
Quoted 15 lines
drivers/usb/dwc3/gadget.c static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) · line 753
Quoted 12 lines
drivers/usb/dwc3/gadget.c +4
-0@@ -747,6 +747,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep)if (!usb_endpoint_dir_in (dep->endpoint.desc) || dep->number <=1 )return 0 ; +/* bail if already resized */ +if (dwc3_readl (dwc->regs,DWC3_GTXFIFOSIZ (dep->number >>1 ))) +return 0 ; +heh, not to say "I told you so", but...
Quoted 8 lines
That being said, your test is not very good. The whole idea for resizing the FIFOs is that in some applications we only use e.g. 2 endpoints and there is considerable FIFO space left unused.
The goal is to use that unused FIFO space to squeeze more throughput out of the pipe, since it amortizes SW latency.
This patch is essentially the same as reverting the original commit :-)
No, it's not quite the same as nullifying the resizing algorithm. This patch is predicated on a key part of the resizing algorithm where
dwc3_gadget_clear_tx_fifos()occurs upon receivingSet_Configurationinep0.c. Which means that each new connection starts off with a blank slate with all the GTXFIFOSIZ(n) registers cleared. Then each EP gets resized one at a time whenusb_ep_enable()is called.Quoted 4 lines
The problem this patch is fixing is avoiding *re-resizing*, the idea being that if an EP was already resized once during a session (from Set Configuration until the next reset or disconnect), then it should be good to go even if it gets disabled and re-enabled again.
Quoted 4 lines
that's not a safe assumption, though. What happens in cases where Configuration 1 is wildly different from Configuration 2? Say Config 1 is a mass storage device and Config 2 is a collection of several CDC interfaces?
The resizing algorithm operates only on one Configuration at a time. If a new Configuration is selected by the host, the algorithm starts over again, so all of the endpoints in the new config will need to be evaluated and resized again. The number of total endpoints in a configuration (including across multiple Alt Settings) is taken into account to ensure there is enough room to guarantee each IN endpoint will have a minimum amount of FIFO space, but also allowing an opportunity for those endpoints who could benefit (based on their MaxBurst setting) to be allocated more.
Quoted 6 lines
Quoted 2 lines
Since we lack any boolean state variable in struct
dwc3_epreflecting whether it had already been resized, re-reading the GTXFIFOSIZ registerit might be a better idea to introduce such a flag and make the intention clear. But in any case, I still think the assumption you're making is not very good.
IMHO I think it's pretty straightforward, as the GTXFIFOSIZ(n) register must have a valid value for the EP to be operational. The assumption is if it's 0, that means it was cleared by dwc3_gadget_clear_tx_fifos() and we haven't yet visited this EP for resizing. So the check within this resize routine is basically a combination of the following:
if (dwc->do_fifo_resize && DWC3_GTXFIFOSIZ(N) != 0)
// this IN EP has been resized
But I hear you about using an explicit flag as it would make it crystal clear to the reader. I can prepare something for V2.
Quoted 5 lines
Quoted 3 lines
is the next best equivalent. Note also that this check occurs after the if (!dwc->do_fifo_resize) check so this is applicable only if the entire "tx-fifo-resize" mechanism is enabled.
Right, that's fine :-)
Jack
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Thinh Nguyen
maintainerQuoted 38 lines · Jack Pham
Some functions may dynamically enable and disable their endpoints regularly throughout their operation, particularly when Set Interface is employed to switch between Alternate Settings. For instance the UAC2 function has its respective endpoints for playback & capture associated with AltSetting 1, in which case those endpoints would not get enabled until the host activates the AltSetting. And they conversely become disabled when the interfaces' AltSetting 0 is chosen.
With the DWC3 FIFO resizing algorithm recently added, every
usb_ep_enable()call results in a call to resize that EP's TXFIFO, but if the same endpoint is enabled again and again, this incorrectly leads to FIFO RAM allocation exhaustion as the mechanism did not account for the possibility that endpoints can be re-enabled many times.Example log splat:
dwc3 a600000.dwc3: Fifosize(3717) > RAM size(3462) ep3in depth:217973127 configfs-gadget gadget: u_audio_start_capture:521 Error! dwc3 a600000.dwc3: request 000000000be13e18 was not queued to ep3inThis is easily fixed by bailing out of
dwc3_gadget_resize_tx_fifos()if an endpoint is already resized, avoiding the calculation error resulting from accumulating the EP's FIFO depth repeatedly.Fixes 9f607a309fbe9 ("usb: dwc3: Resize TX FIFOs to meet EP bursting requirements")Signed-off-by Jack Pham <[email protected]>--- drivers/usb/dwc3/gadget.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/usb/dwc3/gadget.c +0
-0
drivers/usb/dwc3/gadget.c static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) · line 756
Quoted 10 lines
drivers/usb/dwc3/gadget.c +4
-0@@ -747,6 +747,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep)if (!usb_endpoint_dir_in (dep->endpoint.desc) || dep->number <=1 )return 0 ; +/* bail if already resized */ +if (dwc3_readl (dwc->regs,DWC3_GTXFIFOSIZ (dep->number >>1 ))) +return 0 ; + ram1_depth =DWC3_RAM1_DEPTH (dwc->hwparams.hwparams7); if ((dep->endpoint.maxburst > 1 &&
This seems like a workaround more than a fix. As previously pointed out, there will be problems when there are multiple alternate setting interface [2]. If we're doing this way, are we properly allocating the fifo size for the alternate setting that requires the most fifo size and not just the first alt-setting 0? Also different alt-setting can have different endpoints, the logic handling this may not be simple.
There are a few review comments for Wesley. Hopefully they get resolved eventually.
[1]
https://lore.kernel.org/linux-usb/[email protected]/
[2]
https://lore.kernel.org/linux-usb/[email protected]/
Thanks,
Thinh
Quoted 41 lines · Thinh Nguyen
Quoted 40 lines · Jack Pham
Some functions may dynamically enable and disable their endpoints regularly throughout their operation, particularly when Set Interface is employed to switch between Alternate Settings. For instance the UAC2 function has its respective endpoints for playback & capture associated with AltSetting 1, in which case those endpoints would not get enabled until the host activates the AltSetting. And they conversely become disabled when the interfaces' AltSetting 0 is chosen.
With the DWC3 FIFO resizing algorithm recently added, every
usb_ep_enable()call results in a call to resize that EP's TXFIFO, but if the same endpoint is enabled again and again, this incorrectly leads to FIFO RAM allocation exhaustion as the mechanism did not account for the possibility that endpoints can be re-enabled many times.Example log splat:
dwc3 a600000.dwc3: Fifosize(3717) > RAM size(3462) ep3in depth:217973127 configfs-gadget gadget: u_audio_start_capture:521 Error! dwc3 a600000.dwc3: request 000000000be13e18 was not queued to ep3inThis is easily fixed by bailing out of
dwc3_gadget_resize_tx_fifos()if an endpoint is already resized, avoiding the calculation error resulting from accumulating the EP's FIFO depth repeatedly.Fixes 9f607a309fbe9 ("usb: dwc3: Resize TX FIFOs to meet EP bursting requirements")Signed-off-by Jack Pham <[email protected]>--- drivers/usb/dwc3/gadget.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/usb/dwc3/gadget.c +0
-0@@ -747,6 +747,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep)if (!usb_endpoint_dir_in (dep->endpoint.desc) || dep->number <=1 )return 0 ;
Quoted 8 lines
drivers/usb/dwc3/gadget.c static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) · line 756
Quoted 8 lines
drivers/usb/dwc3/gadget.c +4
-0+/* bail if already resized */ +if (dwc3_readl (dwc->regs,DWC3_GTXFIFOSIZ (dep->number >>1 ))) +return 0 ; + ram1_depth =DWC3_RAM1_DEPTH (dwc->hwparams.hwparams7); if ((dep->endpoint.maxburst > 1 &&
Hi Thinh,
Quoted 6 lines
This seems like a workaround more than a fix. As previously pointed out, there will be problems when there are multiple alternate setting interface [2]. If we're doing this way, are we properly allocating the fifo size for the alternate setting that requires the most fifo size and not just the first alt-setting 0? Also different alt-setting can have
Each alt interface will call usb_ep_autoconfig() which should be assigned different endpoint numbers. This would mean that if alt intf#0 gets selected, and EP is enabled, then we will resize the TXFIFO and map that FIFO to the physical EP. Then when/if the host requests the other alt intf#1, and that calls EP enable, then the logic will then attempt to resize based on the parameters, and again map that FIFO to the physical EP. (since we call autoconfig on all interfaces, they should be assigned different endpoints)
I agree that there is currently a limitation because we are going to reserve at minimum 1 FIFO for BOTH alt interfaces, even though there is only 1 interface active at a time. The missing logic that we might be missing is seeing how we can re-purpose the FIFO that is being disabled.
However, I think Jack's fix here would be applicable to the improvement
in logic to re-use/re-assign FIFO space allocated by disabled EPs also.
Quoted 4 lines
different endpoints, the logic handling this may not be simple.
There are a few review comments for Wesley. Hopefully they get resolved eventually.
As mentioned above, there is a lot of considerations we need to make when looking at the amount of combinations that can be done for a USB configuration. We obviously want to see if we can find a way to re-allocate FIFO space, but it gets complicated if we run into a "fragmented" situation where the RAM associated to the EP being re-allocated is in between 2 that are active.
Thanks
Wesley Cheng
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Thinh Nguyen
maintainerQuoted 62 lines · Wesley Cheng
Quoted 41 lines · Thinh Nguyen
Quoted 40 lines · Jack Pham
Some functions may dynamically enable and disable their endpoints regularly throughout their operation, particularly when Set Interface is employed to switch between Alternate Settings. For instance the UAC2 function has its respective endpoints for playback & capture associated with AltSetting 1, in which case those endpoints would not get enabled until the host activates the AltSetting. And they conversely become disabled when the interfaces' AltSetting 0 is chosen.
With the DWC3 FIFO resizing algorithm recently added, every
usb_ep_enable()call results in a call to resize that EP's TXFIFO, but if the same endpoint is enabled again and again, this incorrectly leads to FIFO RAM allocation exhaustion as the mechanism did not account for the possibility that endpoints can be re-enabled many times.Example log splat:
dwc3 a600000.dwc3: Fifosize(3717) > RAM size(3462) ep3in depth:217973127 configfs-gadget gadget: u_audio_start_capture:521 Error! dwc3 a600000.dwc3: request 000000000be13e18 was not queued to ep3inThis is easily fixed by bailing out of
dwc3_gadget_resize_tx_fifos()if an endpoint is already resized, avoiding the calculation error resulting from accumulating the EP's FIFO depth repeatedly.Fixes 9f607a309fbe9 ("usb: dwc3: Resize TX FIFOs to meet EP bursting requirements")Signed-off-by Jack Pham <[email protected]>--- drivers/usb/dwc3/gadget.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/usb/dwc3/gadget.c +0
-0@@ -747,6 +747,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep)if (!usb_endpoint_dir_in (dep->endpoint.desc) || dep->number <=1 )return 0 ;Quoted 8 lines
drivers/usb/dwc3/gadget.c static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) · line 756
Quoted 8 lines
drivers/usb/dwc3/gadget.c +4
-0+/* bail if already resized */ +if (dwc3_readl (dwc->regs,DWC3_GTXFIFOSIZ (dep->number >>1 ))) +return 0 ; + ram1_depth =DWC3_RAM1_DEPTH (dwc->hwparams.hwparams7); if ((dep->endpoint.maxburst > 1 &&Hi Thinh,
Quoted 6 lines
This seems like a workaround more than a fix. As previously pointed out, there will be problems when there are multiple alternate setting interface [2]. If we're doing this way, are we properly allocating the fifo size for the alternate setting that requires the most fifo size and not just the first alt-setting 0? Also different alt-setting can have
Quoted 8 lines
Each alt interface will call
usb_ep_autoconfig()which should be assigned different endpoint numbers. This would mean that if alt intf#0 gets selected, and EP is enabled, then we will resize the TXFIFO and map that FIFO to the physical EP. Then when/if the host requests the other alt intf#1, and that calls EP enable, then the logic will then attempt to resize based on the parameters, and again map that FIFO to the physical EP. (since we call autoconfig on all interfaces, they should be assigned different endpoints)
That's not true. Different alt-settings of an interface can share endpoint numbers. This is often the case for UASP driver where alt-setting 0 is for BOT protocol and alt-setting 1 is UASP. When we switch alt-setting, we disable the current endpoints and enable the old/new ones.
Quoted 8 lines
I agree that there is currently a limitation because we are going to reserve at minimum 1 FIFO for BOTH alt interfaces, even though there is only 1 interface active at a time. The missing logic that we might be missing is seeing how we can re-purpose the FIFO that is being disabled.
However, I think Jack's fix here would be applicable to the improvement
in logic to re-use/re-assign FIFO space allocated by disabled EPs also.
Improvement is always great. I just hope we don't just stop where we are now. Since you're working on this feature at the moment, it would be good to also resolve some of the outstanding issues as Jack's fix seems to be incomplete.
Quoted 5 lines
Quoted 4 lines
different endpoints, the logic handling this may not be simple.
There are a few review comments for Wesley. Hopefully they get resolved eventually.
Quoted 7 lines
As mentioned above, there is a lot of considerations we need to make when looking at the amount of combinations that can be done for a USB configuration. We obviously want to see if we can find a way to re-allocate FIFO space, but it gets complicated if we run into a "fragmented" situation where the RAM associated to the EP being re-allocated is in between 2 that are active.
I'd like to have this feature added, and it would be great if it can overcome some of the current limitations. At the moment, if this feature is enabled, it may improve some applications, but it may also cause regression for some. As I noted, the fix may not be simple, but I hope this feature can work for various applications and not just a limited few.
Thanks,
Thinh
Quoted 63 lines · Thinh Nguyen
Quoted 62 lines · Wesley Cheng
Quoted 41 lines · Thinh Nguyen
Quoted 40 lines · Jack Pham
Some functions may dynamically enable and disable their endpoints regularly throughout their operation, particularly when Set Interface is employed to switch between Alternate Settings. For instance the UAC2 function has its respective endpoints for playback & capture associated with AltSetting 1, in which case those endpoints would not get enabled until the host activates the AltSetting. And they conversely become disabled when the interfaces' AltSetting 0 is chosen.
With the DWC3 FIFO resizing algorithm recently added, every
usb_ep_enable()call results in a call to resize that EP's TXFIFO, but if the same endpoint is enabled again and again, this incorrectly leads to FIFO RAM allocation exhaustion as the mechanism did not account for the possibility that endpoints can be re-enabled many times.Example log splat:
dwc3 a600000.dwc3: Fifosize(3717) > RAM size(3462) ep3in depth:217973127 configfs-gadget gadget: u_audio_start_capture:521 Error! dwc3 a600000.dwc3: request 000000000be13e18 was not queued to ep3inThis is easily fixed by bailing out of
dwc3_gadget_resize_tx_fifos()if an endpoint is already resized, avoiding the calculation error resulting from accumulating the EP's FIFO depth repeatedly.Fixes 9f607a309fbe9 ("usb: dwc3: Resize TX FIFOs to meet EP bursting requirements")Signed-off-by Jack Pham <[email protected]>--- drivers/usb/dwc3/gadget.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/usb/dwc3/gadget.c +0
-0@@ -747,6 +747,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep)if (!usb_endpoint_dir_in (dep->endpoint.desc) || dep->number <=1 )return 0 ;Quoted 8 lines
drivers/usb/dwc3/gadget.c static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) · line 756
Quoted 8 lines
drivers/usb/dwc3/gadget.c +4
-0+/* bail if already resized */ +if (dwc3_readl (dwc->regs,DWC3_GTXFIFOSIZ (dep->number >>1 ))) +return 0 ; + ram1_depth =DWC3_RAM1_DEPTH (dwc->hwparams.hwparams7); if ((dep->endpoint.maxburst > 1 &&Hi Thinh,
Quoted 6 lines
This seems like a workaround more than a fix. As previously pointed out, there will be problems when there are multiple alternate setting interface [2]. If we're doing this way, are we properly allocating the fifo size for the alternate setting that requires the most fifo size and not just the first alt-setting 0? Also different alt-setting can have
Quoted 8 lines
Quoted 8 lines
Each alt interface will call
usb_ep_autoconfig()which should be assigned different endpoint numbers. This would mean that if alt intf#0 gets selected, and EP is enabled, then we will resize the TXFIFO and map that FIFO to the physical EP. Then when/if the host requests the other alt intf#1, and that calls EP enable, then the logic will then attempt to resize based on the parameters, and again map that FIFO to the physical EP. (since we call autoconfig on all interfaces, they should be assigned different endpoints)
Hi Thinh,
Quoted 7 lines
That's not true. Different alt-settings of an interface can share endpoint numbers. This is often the case for UASP driver where alt-setting 0 is for BOT protocol and alt-setting 1 is UASP. When we switch alt-setting, we disable the current endpoints and enable the old/new ones.
Thanks for pointing that use case out. Maybe we can consider seeing if we can walk through all alternate interfaces for a particular function, and resize for the largest setting? That might be a possible improvement made to the check_config() function. Let me start makign the changes for this and verifying it.
Quoted 9 lines
Quoted 8 lines
I agree that there is currently a limitation because we are going to reserve at minimum 1 FIFO for BOTH alt interfaces, even though there is only 1 interface active at a time. The missing logic that we might be missing is seeing how we can re-purpose the FIFO that is being disabled.
However, I think Jack's fix here would be applicable to the improvement
in logic to re-use/re-assign FIFO space allocated by disabled EPs also.
Quoted 5 lines
Improvement is always great. I just hope we don't just stop where we are now. Since you're working on this feature at the moment, it would be good to also resolve some of the outstanding issues as Jack's fix seems to be incomplete.
If we implement the improvement mentioned above, I think Jack's fix will be applicable there as well. If we resize for the largest alternate interface, then there would be no reason for us to resize again.
Quoted 13 lines
Quoted 5 lines
Quoted 4 lines
different endpoints, the logic handling this may not be simple.
There are a few review comments for Wesley. Hopefully they get resolved eventually.
Quoted 7 lines
As mentioned above, there is a lot of considerations we need to make when looking at the amount of combinations that can be done for a USB configuration. We obviously want to see if we can find a way to re-allocate FIFO space, but it gets complicated if we run into a "fragmented" situation where the RAM associated to the EP being re-allocated is in between 2 that are active.
Quoted 6 lines
I'd like to have this feature added, and it would be great if it can overcome some of the current limitations. At the moment, if this feature is enabled, it may improve some applications, but it may also cause regression for some. As I noted, the fix may not be simple, but I hope this feature can work for various applications and not just a limited few.
Agreed, there are some use cases that we may not consider in our platform, so I appreciate the input.
Thanks
Wesley Cheng
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Thinh Nguyen
maintainerQuoted 85 lines · Wesley Cheng
Quoted 63 lines · Thinh Nguyen
Quoted 62 lines · Wesley Cheng
Quoted 41 lines · Thinh Nguyen
Quoted 40 lines · Jack Pham
Some functions may dynamically enable and disable their endpoints regularly throughout their operation, particularly when Set Interface is employed to switch between Alternate Settings. For instance the UAC2 function has its respective endpoints for playback & capture associated with AltSetting 1, in which case those endpoints would not get enabled until the host activates the AltSetting. And they conversely become disabled when the interfaces' AltSetting 0 is chosen.
With the DWC3 FIFO resizing algorithm recently added, every
usb_ep_enable()call results in a call to resize that EP's TXFIFO, but if the same endpoint is enabled again and again, this incorrectly leads to FIFO RAM allocation exhaustion as the mechanism did not account for the possibility that endpoints can be re-enabled many times.Example log splat:
dwc3 a600000.dwc3: Fifosize(3717) > RAM size(3462) ep3in depth:217973127 configfs-gadget gadget: u_audio_start_capture:521 Error! dwc3 a600000.dwc3: request 000000000be13e18 was not queued to ep3inThis is easily fixed by bailing out of
dwc3_gadget_resize_tx_fifos()if an endpoint is already resized, avoiding the calculation error resulting from accumulating the EP's FIFO depth repeatedly.Fixes 9f607a309fbe9 ("usb: dwc3: Resize TX FIFOs to meet EP bursting requirements")Signed-off-by Jack Pham <[email protected]>--- drivers/usb/dwc3/gadget.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/usb/dwc3/gadget.c +0
-0@@ -747,6 +747,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep)if (!usb_endpoint_dir_in (dep->endpoint.desc) || dep->number <=1 )return 0 ;Quoted 8 lines
drivers/usb/dwc3/gadget.c static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) · line 756
Quoted 8 lines
drivers/usb/dwc3/gadget.c +4
-0+/* bail if already resized */ +if (dwc3_readl (dwc->regs,DWC3_GTXFIFOSIZ (dep->number >>1 ))) +return 0 ; + ram1_depth =DWC3_RAM1_DEPTH (dwc->hwparams.hwparams7); if ((dep->endpoint.maxburst > 1 &&Hi Thinh,
Quoted 6 lines
This seems like a workaround more than a fix. As previously pointed out, there will be problems when there are multiple alternate setting interface [2]. If we're doing this way, are we properly allocating the fifo size for the alternate setting that requires the most fifo size and not just the first alt-setting 0? Also different alt-setting can have
Quoted 8 lines
Quoted 8 lines
Each alt interface will call
usb_ep_autoconfig()which should be assigned different endpoint numbers. This would mean that if alt intf#0 gets selected, and EP is enabled, then we will resize the TXFIFO and map that FIFO to the physical EP. Then when/if the host requests the other alt intf#1, and that calls EP enable, then the logic will then attempt to resize based on the parameters, and again map that FIFO to the physical EP. (since we call autoconfig on all interfaces, they should be assigned different endpoints)Hi Thinh,
Quoted 7 lines
That's not true. Different alt-settings of an interface can share endpoint numbers. This is often the case for UASP driver where alt-setting 0 is for BOT protocol and alt-setting 1 is UASP. When we switch alt-setting, we disable the current endpoints and enable the old/new ones.
Quoted 6 lines
Thanks for pointing that use case out. Maybe we can consider seeing if we can walk through all alternate interfaces for a particular function, and resize for the largest setting? That might be a possible improvement made to the
check_config()function. Let me start makign the changes for this and verifying it.
Thanks!
Currently the gadget configures early and informs the gadget driver of how many endpoints are available, which doesn't leave much room for the gadget to do optimization/reconfiguration.
If there's an option for the composite layer to inform the controller driver of the entire configuration, then we can take advantage of more dwc3 controller capability/flexibility (not just resizing txfifo).
Quoted 15 lines
Quoted 9 lines
Quoted 8 lines
I agree that there is currently a limitation because we are going to reserve at minimum 1 FIFO for BOTH alt interfaces, even though there is only 1 interface active at a time. The missing logic that we might be missing is seeing how we can re-purpose the FIFO that is being disabled.
However, I think Jack's fix here would be applicable to the improvement
in logic to re-use/re-assign FIFO space allocated by disabled EPs also.Quoted 5 lines
Improvement is always great. I just hope we don't just stop where we are now. Since you're working on this feature at the moment, it would be good to also resolve some of the outstanding issues as Jack's fix seems to be incomplete.
Quoted 4 lines
If we implement the improvement mentioned above, I think Jack's fix will be applicable there as well. If we resize for the largest alternate interface, then there would be no reason for us to resize again.
As long as you have the above as part of your roadmap, I don't mind Jack's fix for now. Depends on your implementation, Jack's fix may not be applicable. Regardless, ultimately it's up to Felipe. :)
Quoted 20 lines
Quoted 13 lines
Quoted 5 lines
Quoted 4 lines
different endpoints, the logic handling this may not be simple.
There are a few review comments for Wesley. Hopefully they get resolved eventually.
Quoted 7 lines
As mentioned above, there is a lot of considerations we need to make when looking at the amount of combinations that can be done for a USB configuration. We obviously want to see if we can find a way to re-allocate FIFO space, but it gets complicated if we run into a "fragmented" situation where the RAM associated to the EP being re-allocated is in between 2 that are active.
Quoted 6 lines
I'd like to have this feature added, and it would be great if it can overcome some of the current limitations. At the moment, if this feature is enabled, it may improve some applications, but it may also cause regression for some. As I noted, the fix may not be simple, but I hope this feature can work for various applications and not just a limited few.
Quoted 3 lines
Agreed, there are some use cases that we may not consider in our platform, so I appreciate the input.
(Sorry for the delay response. I was on vacation.)
BR,
Thinh
Quoted 118 lines · Thinh Nguyen
Quoted 85 lines · Wesley Cheng
Quoted 63 lines · Thinh Nguyen
Quoted 62 lines · Wesley Cheng
Quoted 41 lines · Thinh Nguyen
Quoted 40 lines · Jack Pham
Some functions may dynamically enable and disable their endpoints regularly throughout their operation, particularly when Set Interface is employed to switch between Alternate Settings. For instance the UAC2 function has its respective endpoints for playback & capture associated with AltSetting 1, in which case those endpoints would not get enabled until the host activates the AltSetting. And they conversely become disabled when the interfaces' AltSetting 0 is chosen.
With the DWC3 FIFO resizing algorithm recently added, every
usb_ep_enable()call results in a call to resize that EP's TXFIFO, but if the same endpoint is enabled again and again, this incorrectly leads to FIFO RAM allocation exhaustion as the mechanism did not account for the possibility that endpoints can be re-enabled many times.Example log splat:
dwc3 a600000.dwc3: Fifosize(3717) > RAM size(3462) ep3in depth:217973127 configfs-gadget gadget: u_audio_start_capture:521 Error! dwc3 a600000.dwc3: request 000000000be13e18 was not queued to ep3inThis is easily fixed by bailing out of
dwc3_gadget_resize_tx_fifos()if an endpoint is already resized, avoiding the calculation error resulting from accumulating the EP's FIFO depth repeatedly.Fixes 9f607a309fbe9 ("usb: dwc3: Resize TX FIFOs to meet EP bursting requirements")Signed-off-by Jack Pham <[email protected]>--- drivers/usb/dwc3/gadget.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/usb/dwc3/gadget.c +0
-0@@ -747,6 +747,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep)if (!usb_endpoint_dir_in (dep->endpoint.desc) || dep->number <=1 )return 0 ;Quoted 8 lines
drivers/usb/dwc3/gadget.c static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) · line 756
Quoted 8 lines
drivers/usb/dwc3/gadget.c +4
-0+/* bail if already resized */ +if (dwc3_readl (dwc->regs,DWC3_GTXFIFOSIZ (dep->number >>1 ))) +return 0 ; + ram1_depth =DWC3_RAM1_DEPTH (dwc->hwparams.hwparams7); if ((dep->endpoint.maxburst > 1 &&Hi Thinh,
Quoted 6 lines
This seems like a workaround more than a fix. As previously pointed out, there will be problems when there are multiple alternate setting interface [2]. If we're doing this way, are we properly allocating the fifo size for the alternate setting that requires the most fifo size and not just the first alt-setting 0? Also different alt-setting can have
Quoted 8 lines
Quoted 8 lines
Each alt interface will call
usb_ep_autoconfig()which should be assigned different endpoint numbers. This would mean that if alt intf#0 gets selected, and EP is enabled, then we will resize the TXFIFO and map that FIFO to the physical EP. Then when/if the host requests the other alt intf#1, and that calls EP enable, then the logic will then attempt to resize based on the parameters, and again map that FIFO to the physical EP. (since we call autoconfig on all interfaces, they should be assigned different endpoints)Hi Thinh,
Quoted 7 lines
That's not true. Different alt-settings of an interface can share endpoint numbers. This is often the case for UASP driver where alt-setting 0 is for BOT protocol and alt-setting 1 is UASP. When we switch alt-setting, we disable the current endpoints and enable the old/new ones.
Quoted 6 lines
Thanks for pointing that use case out. Maybe we can consider seeing if we can walk through all alternate interfaces for a particular function, and resize for the largest setting? That might be a possible improvement made to the
check_config()function. Let me start makign the changes for this and verifying it.Thanks!
Currently the gadget configures early and informs the gadget driver of how many endpoints are available, which doesn't leave much room for the gadget to do optimization/reconfiguration.
If there's an option for the composite layer to inform the controller driver of the entire configuration, then we can take advantage of more dwc3 controller capability/flexibility (not just resizing txfifo).
Quoted 15 lines
Quoted 14 lines
Quoted 8 lines
I agree that there is currently a limitation because we are going to reserve at minimum 1 FIFO for BOTH alt interfaces, even though there is only 1 interface active at a time. The missing logic that we might be missing is seeing how we can re-purpose the FIFO that is being disabled.
However, I think Jack's fix here would be applicable to the improvement
in logic to re-use/re-assign FIFO space allocated by disabled EPs also.Improvement is always great. I just hope we don't just stop where we are now. Since you're working on this feature at the moment, it would be good to also resolve some of the outstanding issues as Jack's fix seems to be incomplete.
Quoted 6 lines
Quoted 4 lines
If we implement the improvement mentioned above, I think Jack's fix will be applicable there as well. If we resize for the largest alternate interface, then there would be no reason for us to resize again.
As long as you have the above as part of your roadmap, I don't mind Jack's fix for now.
Thanks for your input as always Thinh. Does the patch still look ok in its current state? Last time I had responded to Felipe that I would try to entertain adding an explicit flag to keep track of whether an EP had been resized yet or not. When trying to implement this as another DWC3_EP_* bit for dep->flags we'd then need to retain this flag across ep_disable/enable calls, so it looks a tiny bit cumbersome because dep->flags isn't neatly 0 anymore :-P.
So ep_disable() would need to look something like this:
static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep) {
...
dep->stream_capable = false; dep->type = 0;
- dep->flags = 0;
+ dep->flags &= DWC3_EP_TXFIFO_RESIZED;
return 0;
}
The flag would be initially set in dwc3_gadget_resize_tx_fifos() and also would need to be checked there as well to avoid re-resizing. It would then get cleared in dwc3_gadget_clear_tx_fifos().
Is this still preferable to the current patch with just the single register read?
Quoted 2 lines
Depends on your implementation, Jack's fix may not be applicable. Regardless, ultimately it's up to Felipe. :)
Felipe, what do you think? Wesley is already working on improving the scheme to better handle endpoints that get activated only when switching AltSettings. But I'd still like to get my patch in as an incremental fix though since it does take care of an existing issue with disabling and re-enabling endpoints within the same Configuration & AltSetting.
Thanks,
Jack
Quoted 24 lines
Quoted 24 lines
Quoted 14 lines
Quoted 6 lines
Quoted 5 lines
different endpoints, the logic handling this may not be simple.
There are a few review comments for Wesley. Hopefully they get resolved eventually.
Quoted 7 lines
As mentioned above, there is a lot of considerations we need to make when looking at the amount of combinations that can be done for a USB configuration. We obviously want to see if we can find a way to re-allocate FIFO space, but it gets complicated if we run into a "fragmented" situation where the RAM associated to the EP being re-allocated is in between 2 that are active.
Quoted 6 lines
I'd like to have this feature added, and it would be great if it can overcome some of the current limitations. At the moment, if this feature is enabled, it may improve some applications, but it may also cause regression for some. As I noted, the fix may not be simple, but I hope this feature can work for various applications and not just a limited few.
Agreed, there are some use cases that we may not consider in our platform, so I appreciate the input.
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Thinh Nguyen
maintainerQuoted 123 lines · Jack Pham
Quoted 122 lines · Thinh Nguyen
Quoted 85 lines · Wesley Cheng
Quoted 63 lines · Thinh Nguyen
Quoted 62 lines · Wesley Cheng
Quoted 41 lines · Thinh Nguyen
Quoted 40 lines · Jack Pham
Some functions may dynamically enable and disable their endpoints regularly throughout their operation, particularly when Set Interface is employed to switch between Alternate Settings. For instance the UAC2 function has its respective endpoints for playback & capture associated with AltSetting 1, in which case those endpoints would not get enabled until the host activates the AltSetting. And they conversely become disabled when the interfaces' AltSetting 0 is chosen.
With the DWC3 FIFO resizing algorithm recently added, every
usb_ep_enable()call results in a call to resize that EP's TXFIFO, but if the same endpoint is enabled again and again, this incorrectly leads to FIFO RAM allocation exhaustion as the mechanism did not account for the possibility that endpoints can be re-enabled many times.Example log splat:
dwc3 a600000.dwc3: Fifosize(3717) > RAM size(3462) ep3in depth:217973127 configfs-gadget gadget: u_audio_start_capture:521 Error! dwc3 a600000.dwc3: request 000000000be13e18 was not queued to ep3inThis is easily fixed by bailing out of
dwc3_gadget_resize_tx_fifos()if an endpoint is already resized, avoiding the calculation error resulting from accumulating the EP's FIFO depth repeatedly.Fixes 9f607a309fbe9 ("usb: dwc3: Resize TX FIFOs to meet EP bursting requirements")Signed-off-by Jack Pham <[email protected]>--- drivers/usb/dwc3/gadget.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/usb/dwc3/gadget.c +0
-0@@ -747,6 +747,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep)if (!usb_endpoint_dir_in (dep->endpoint.desc) || dep->number <=1 )return 0 ;Quoted 8 lines
drivers/usb/dwc3/gadget.c static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) · line 756
Quoted 8 lines
drivers/usb/dwc3/gadget.c +4
-0+/* bail if already resized */ +if (dwc3_readl (dwc->regs,DWC3_GTXFIFOSIZ (dep->number >>1 ))) +return 0 ; + ram1_depth =DWC3_RAM1_DEPTH (dwc->hwparams.hwparams7); if ((dep->endpoint.maxburst > 1 &&Hi Thinh,
Quoted 6 lines
This seems like a workaround more than a fix. As previously pointed out, there will be problems when there are multiple alternate setting interface [2]. If we're doing this way, are we properly allocating the fifo size for the alternate setting that requires the most fifo size and not just the first alt-setting 0? Also different alt-setting can have
Quoted 8 lines
Quoted 8 lines
Each alt interface will call
usb_ep_autoconfig()which should be assigned different endpoint numbers. This would mean that if alt intf#0 gets selected, and EP is enabled, then we will resize the TXFIFO and map that FIFO to the physical EP. Then when/if the host requests the other alt intf#1, and that calls EP enable, then the logic will then attempt to resize based on the parameters, and again map that FIFO to the physical EP. (since we call autoconfig on all interfaces, they should be assigned different endpoints)Hi Thinh,
Quoted 7 lines
That's not true. Different alt-settings of an interface can share endpoint numbers. This is often the case for UASP driver where alt-setting 0 is for BOT protocol and alt-setting 1 is UASP. When we switch alt-setting, we disable the current endpoints and enable the old/new ones.
Quoted 6 lines
Thanks for pointing that use case out. Maybe we can consider seeing if we can walk through all alternate interfaces for a particular function, and resize for the largest setting? That might be a possible improvement made to the
check_config()function. Let me start makign the changes for this and verifying it.Thanks!
Currently the gadget configures early and informs the gadget driver of how many endpoints are available, which doesn't leave much room for the gadget to do optimization/reconfiguration.
If there's an option for the composite layer to inform the controller driver of the entire configuration, then we can take advantage of more dwc3 controller capability/flexibility (not just resizing txfifo).
Quoted 19 lines
Quoted 9 lines
Quoted 8 lines
I agree that there is currently a limitation because we are going to reserve at minimum 1 FIFO for BOTH alt interfaces, even though there is only 1 interface active at a time. The missing logic that we might be missing is seeing how we can re-purpose the FIFO that is being disabled.
However, I think Jack's fix here would be applicable to the improvement
in logic to re-use/re-assign FIFO space allocated by disabled EPs also.Quoted 5 lines
Improvement is always great. I just hope we don't just stop where we are now. Since you're working on this feature at the moment, it would be good to also resolve some of the outstanding issues as Jack's fix seems to be incomplete.
If we implement the improvement mentioned above, I think Jack's fix will be applicable there as well. If we resize for the largest alternate interface, then there would be no reason for us to resize again.
Quoted 5 lines
Quoted 2 lines
As long as you have the above as part of your roadmap, I don't mind Jack's fix for now.
Thanks for your input as always Thinh. Does the patch still look ok in its current state? Last time I had responded to Felipe that I would try
TX endpoints should have non-zero GTXFIFOSIZ. Using the register as a flag to check whether it's been resized is not ok. Also, what happened after resizing the txfifo? Do you restore its previous default value?
Quoted 13 lines
to entertain adding an explicit flag to keep track of whether an EP had been resized yet or not. When trying to implement this as another DWC3_EP_* bit for
dep->flagswe'd then need to retain this flag across ep_disable/enable calls, so it looks a tiny bit cumbersome becausedep->flagsisn't neatly 0 anymore :-P.So
ep_disable()would need to look something like this:static int
__dwc3_gadget_ep_disable(struct dwc3_ep *dep){
...dep->stream_capable = false; dep->type = 0;
Review
Quoted 2 lines
(quoted diff) +1
-1- dep->flags = 0; + dep->flags &= DWC3_EP_TXFIFO_RESIZED;
I think you mean this?dep->flags &= ~DWC3_EP_TXFIFO_RESIZED;
Quoted 11 lines
return 0;}
The flag would be initially set in
dwc3_gadget_resize_tx_fifos()and also would need to be checked there as well to avoid re-resizing. It would then get cleared indwc3_gadget_clear_tx_fifos().Is this still preferable to the current patch with just the single register read?
What if we change the configuration from the configfs? Would the flag persist? If that's the case, that doesn't look right.
Thanks,
Thinh
Thinh Nguyen
maintainerQuoted 142 lines · Thinh Nguyen
Quoted 123 lines · Jack Pham
Quoted 122 lines · Thinh Nguyen
Quoted 85 lines · Wesley Cheng
Quoted 63 lines · Thinh Nguyen
Quoted 62 lines · Wesley Cheng
Quoted 41 lines · Thinh Nguyen
Quoted 40 lines · Jack Pham
Some functions may dynamically enable and disable their endpoints regularly throughout their operation, particularly when Set Interface is employed to switch between Alternate Settings. For instance the UAC2 function has its respective endpoints for playback & capture associated with AltSetting 1, in which case those endpoints would not get enabled until the host activates the AltSetting. And they conversely become disabled when the interfaces' AltSetting 0 is chosen.
With the DWC3 FIFO resizing algorithm recently added, every
usb_ep_enable()call results in a call to resize that EP's TXFIFO, but if the same endpoint is enabled again and again, this incorrectly leads to FIFO RAM allocation exhaustion as the mechanism did not account for the possibility that endpoints can be re-enabled many times.Example log splat:
dwc3 a600000.dwc3: Fifosize(3717) > RAM size(3462) ep3in depth:217973127 configfs-gadget gadget: u_audio_start_capture:521 Error! dwc3 a600000.dwc3: request 000000000be13e18 was not queued to ep3inThis is easily fixed by bailing out of
dwc3_gadget_resize_tx_fifos()if an endpoint is already resized, avoiding the calculation error resulting from accumulating the EP's FIFO depth repeatedly.Fixes 9f607a309fbe9 ("usb: dwc3: Resize TX FIFOs to meet EP bursting requirements")Signed-off-by Jack Pham <[email protected]>--- drivers/usb/dwc3/gadget.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/usb/dwc3/gadget.c +0
-0@@ -747,6 +747,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep)if (!usb_endpoint_dir_in (dep->endpoint.desc) || dep->number <=1 )return 0 ;Quoted 8 lines
drivers/usb/dwc3/gadget.c static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) · line 756
Quoted 8 lines
drivers/usb/dwc3/gadget.c +4
-0+/* bail if already resized */ +if (dwc3_readl (dwc->regs,DWC3_GTXFIFOSIZ (dep->number >>1 ))) +return 0 ; + ram1_depth =DWC3_RAM1_DEPTH (dwc->hwparams.hwparams7); if ((dep->endpoint.maxburst > 1 &&Hi Thinh,
Quoted 6 lines
This seems like a workaround more than a fix. As previously pointed out, there will be problems when there are multiple alternate setting interface [2]. If we're doing this way, are we properly allocating the fifo size for the alternate setting that requires the most fifo size and not just the first alt-setting 0? Also different alt-setting can have
Quoted 8 lines
Quoted 8 lines
Each alt interface will call
usb_ep_autoconfig()which should be assigned different endpoint numbers. This would mean that if alt intf#0 gets selected, and EP is enabled, then we will resize the TXFIFO and map that FIFO to the physical EP. Then when/if the host requests the other alt intf#1, and that calls EP enable, then the logic will then attempt to resize based on the parameters, and again map that FIFO to the physical EP. (since we call autoconfig on all interfaces, they should be assigned different endpoints)Hi Thinh,
Quoted 7 lines
That's not true. Different alt-settings of an interface can share endpoint numbers. This is often the case for UASP driver where alt-setting 0 is for BOT protocol and alt-setting 1 is UASP. When we switch alt-setting, we disable the current endpoints and enable the old/new ones.
Quoted 6 lines
Thanks for pointing that use case out. Maybe we can consider seeing if we can walk through all alternate interfaces for a particular function, and resize for the largest setting? That might be a possible improvement made to the
check_config()function. Let me start makign the changes for this and verifying it.Thanks!
Currently the gadget configures early and informs the gadget driver of how many endpoints are available, which doesn't leave much room for the gadget to do optimization/reconfiguration.
If there's an option for the composite layer to inform the controller driver of the entire configuration, then we can take advantage of more dwc3 controller capability/flexibility (not just resizing txfifo).
Quoted 19 lines
Quoted 9 lines
Quoted 8 lines
I agree that there is currently a limitation because we are going to reserve at minimum 1 FIFO for BOTH alt interfaces, even though there is only 1 interface active at a time. The missing logic that we might be missing is seeing how we can re-purpose the FIFO that is being disabled.
However, I think Jack's fix here would be applicable to the improvement
in logic to re-use/re-assign FIFO space allocated by disabled EPs also.Quoted 5 lines
Improvement is always great. I just hope we don't just stop where we are now. Since you're working on this feature at the moment, it would be good to also resolve some of the outstanding issues as Jack's fix seems to be incomplete.
If we implement the improvement mentioned above, I think Jack's fix will be applicable there as well. If we resize for the largest alternate interface, then there would be no reason for us to resize again.
Quoted 5 lines
Quoted 2 lines
As long as you have the above as part of your roadmap, I don't mind Jack's fix for now.
Thanks for your input as always Thinh. Does the patch still look ok in its current state? Last time I had responded to Felipe that I would try
TX endpoints should have non-zero GTXFIFOSIZ. Using the register as a flag to check whether it's been resized is not ok. Also, what happened after resizing the txfifo? Do you restore its previous default value?
Quoted 8 lines
to entertain adding an explicit flag to keep track of whether an EP had been resized yet or not. When trying to implement this as another DWC3_EP_* bit for
dep->flagswe'd then need to retain this flag across ep_disable/enable calls, so it looks a tiny bit cumbersome becausedep->flagsisn't neatly 0 anymore :-P.So
ep_disable()would need to look something like this:
Quoted 11 lines
Review
Quoted 7 lines
static int
__dwc3_gadget_ep_disable(struct dwc3_ep *dep){
...dep->stream_capable = false; dep->type = 0;(quoted diff) +1
-1- dep->flags = 0; + dep->flags &= DWC3_EP_TXFIFO_RESIZED;I think you mean this?
dep->flags&= ~DWC3_EP_TXFIFO_RESIZED;
nvm, ignore this comment. Your intention isdep->flags &= DWC3_EP_TXFIFO_RESIZED;
Thinh
Thinh Nguyen
maintainerQuoted 142 lines · Thinh Nguyen
Quoted 123 lines · Jack Pham
Quoted 122 lines · Thinh Nguyen
Quoted 85 lines · Wesley Cheng
Quoted 63 lines · Thinh Nguyen
Quoted 62 lines · Wesley Cheng
Quoted 41 lines · Thinh Nguyen
Quoted 40 lines · Jack Pham
Some functions may dynamically enable and disable their endpoints regularly throughout their operation, particularly when Set Interface is employed to switch between Alternate Settings. For instance the UAC2 function has its respective endpoints for playback & capture associated with AltSetting 1, in which case those endpoints would not get enabled until the host activates the AltSetting. And they conversely become disabled when the interfaces' AltSetting 0 is chosen.
With the DWC3 FIFO resizing algorithm recently added, every
usb_ep_enable()call results in a call to resize that EP's TXFIFO, but if the same endpoint is enabled again and again, this incorrectly leads to FIFO RAM allocation exhaustion as the mechanism did not account for the possibility that endpoints can be re-enabled many times.Example log splat:
dwc3 a600000.dwc3: Fifosize(3717) > RAM size(3462) ep3in depth:217973127 configfs-gadget gadget: u_audio_start_capture:521 Error! dwc3 a600000.dwc3: request 000000000be13e18 was not queued to ep3inThis is easily fixed by bailing out of
dwc3_gadget_resize_tx_fifos()if an endpoint is already resized, avoiding the calculation error resulting from accumulating the EP's FIFO depth repeatedly.Fixes 9f607a309fbe9 ("usb: dwc3: Resize TX FIFOs to meet EP bursting requirements")Signed-off-by Jack Pham <[email protected]>--- drivers/usb/dwc3/gadget.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/usb/dwc3/gadget.c +0
-0@@ -747,6 +747,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep)if (!usb_endpoint_dir_in (dep->endpoint.desc) || dep->number <=1 )return 0 ;Quoted 8 lines
drivers/usb/dwc3/gadget.c static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) · line 756
Quoted 8 lines
drivers/usb/dwc3/gadget.c +4
-0+/* bail if already resized */ +if (dwc3_readl (dwc->regs,DWC3_GTXFIFOSIZ (dep->number >>1 ))) +return 0 ; + ram1_depth =DWC3_RAM1_DEPTH (dwc->hwparams.hwparams7); if ((dep->endpoint.maxburst > 1 &&Hi Thinh,
Quoted 6 lines
This seems like a workaround more than a fix. As previously pointed out, there will be problems when there are multiple alternate setting interface [2]. If we're doing this way, are we properly allocating the fifo size for the alternate setting that requires the most fifo size and not just the first alt-setting 0? Also different alt-setting can have
Quoted 8 lines
Quoted 8 lines
Each alt interface will call
usb_ep_autoconfig()which should be assigned different endpoint numbers. This would mean that if alt intf#0 gets selected, and EP is enabled, then we will resize the TXFIFO and map that FIFO to the physical EP. Then when/if the host requests the other alt intf#1, and that calls EP enable, then the logic will then attempt to resize based on the parameters, and again map that FIFO to the physical EP. (since we call autoconfig on all interfaces, they should be assigned different endpoints)Hi Thinh,
Quoted 7 lines
That's not true. Different alt-settings of an interface can share endpoint numbers. This is often the case for UASP driver where alt-setting 0 is for BOT protocol and alt-setting 1 is UASP. When we switch alt-setting, we disable the current endpoints and enable the old/new ones.
Quoted 6 lines
Thanks for pointing that use case out. Maybe we can consider seeing if we can walk through all alternate interfaces for a particular function, and resize for the largest setting? That might be a possible improvement made to the
check_config()function. Let me start makign the changes for this and verifying it.Thanks!
Currently the gadget configures early and informs the gadget driver of how many endpoints are available, which doesn't leave much room for the gadget to do optimization/reconfiguration.
If there's an option for the composite layer to inform the controller driver of the entire configuration, then we can take advantage of more dwc3 controller capability/flexibility (not just resizing txfifo).
Quoted 19 lines
Quoted 9 lines
Quoted 8 lines
I agree that there is currently a limitation because we are going to reserve at minimum 1 FIFO for BOTH alt interfaces, even though there is only 1 interface active at a time. The missing logic that we might be missing is seeing how we can re-purpose the FIFO that is being disabled.
However, I think Jack's fix here would be applicable to the improvement
in logic to re-use/re-assign FIFO space allocated by disabled EPs also.Quoted 5 lines
Improvement is always great. I just hope we don't just stop where we are now. Since you're working on this feature at the moment, it would be good to also resolve some of the outstanding issues as Jack's fix seems to be incomplete.
If we implement the improvement mentioned above, I think Jack's fix will be applicable there as well. If we resize for the largest alternate interface, then there would be no reason for us to resize again.
Quoted 5 lines
Quoted 2 lines
As long as you have the above as part of your roadmap, I don't mind Jack's fix for now.
Thanks for your input as always Thinh. Does the patch still look ok in its current state? Last time I had responded to Felipe that I would try
TX endpoints should have non-zero GTXFIFOSIZ. Using the register as a flag to check whether it's been resized is not ok. Also, what happened after resizing the txfifo? Do you restore its previous default value?
Quoted 8 lines
to entertain adding an explicit flag to keep track of whether an EP had been resized yet or not. When trying to implement this as another DWC3_EP_* bit for
dep->flagswe'd then need to retain this flag across ep_disable/enable calls, so it looks a tiny bit cumbersome becausedep->flagsisn't neatly 0 anymore :-P.So
ep_disable()would need to look something like this:
Quoted 11 lines
Review
Quoted 7 lines
static int
__dwc3_gadget_ep_disable(struct dwc3_ep *dep){
...dep->stream_capable = false; dep->type = 0;(quoted diff) +1
-1- dep->flags = 0; + dep->flags &= DWC3_EP_TXFIFO_RESIZED;I think you mean this?
dep->flags&= ~DWC3_EP_TXFIFO_RESIZED;
nvm, ignore this comment. Your intention isdep->flags &= DWC3_EP_TXFIFO_RESIZED;
Thinh
Quoted 127 lines · Thinh Nguyen
Quoted 126 lines · Jack Pham
Quoted 124 lines · Thinh Nguyen
Quoted 85 lines · Wesley Cheng
Quoted 63 lines · Thinh Nguyen
Quoted 62 lines · Wesley Cheng
Quoted 41 lines · Thinh Nguyen
Quoted 40 lines · Jack Pham
Some functions may dynamically enable and disable their endpoints regularly throughout their operation, particularly when Set Interface is employed to switch between Alternate Settings. For instance the UAC2 function has its respective endpoints for playback & capture associated with AltSetting 1, in which case those endpoints would not get enabled until the host activates the AltSetting. And they conversely become disabled when the interfaces' AltSetting 0 is chosen.
With the DWC3 FIFO resizing algorithm recently added, every
usb_ep_enable()call results in a call to resize that EP's TXFIFO, but if the same endpoint is enabled again and again, this incorrectly leads to FIFO RAM allocation exhaustion as the mechanism did not account for the possibility that endpoints can be re-enabled many times.Example log splat:
dwc3 a600000.dwc3: Fifosize(3717) > RAM size(3462) ep3in depth:217973127 configfs-gadget gadget: u_audio_start_capture:521 Error! dwc3 a600000.dwc3: request 000000000be13e18 was not queued to ep3inThis is easily fixed by bailing out of
dwc3_gadget_resize_tx_fifos()if an endpoint is already resized, avoiding the calculation error resulting from accumulating the EP's FIFO depth repeatedly.Fixes 9f607a309fbe9 ("usb: dwc3: Resize TX FIFOs to meet EP bursting requirements")Signed-off-by Jack Pham <[email protected]>--- drivers/usb/dwc3/gadget.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/usb/dwc3/gadget.c +0
-0@@ -747,6 +747,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep)if (!usb_endpoint_dir_in (dep->endpoint.desc) || dep->number <=1 )return 0 ;Quoted 8 lines
drivers/usb/dwc3/gadget.c static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) · line 756
Quoted 8 lines
drivers/usb/dwc3/gadget.c +4
-0+/* bail if already resized */ +if (dwc3_readl (dwc->regs,DWC3_GTXFIFOSIZ (dep->number >>1 ))) +return 0 ; + ram1_depth =DWC3_RAM1_DEPTH (dwc->hwparams.hwparams7); if ((dep->endpoint.maxburst > 1 &&Hi Thinh,
Quoted 6 lines
This seems like a workaround more than a fix. As previously pointed out, there will be problems when there are multiple alternate setting interface [2]. If we're doing this way, are we properly allocating the fifo size for the alternate setting that requires the most fifo size and not just the first alt-setting 0? Also different alt-setting can have
Quoted 8 lines
Quoted 8 lines
Each alt interface will call
usb_ep_autoconfig()which should be assigned different endpoint numbers. This would mean that if alt intf#0 gets selected, and EP is enabled, then we will resize the TXFIFO and map that FIFO to the physical EP. Then when/if the host requests the other alt intf#1, and that calls EP enable, then the logic will then attempt to resize based on the parameters, and again map that FIFO to the physical EP. (since we call autoconfig on all interfaces, they should be assigned different endpoints)Hi Thinh,
Quoted 7 lines
That's not true. Different alt-settings of an interface can share endpoint numbers. This is often the case for UASP driver where alt-setting 0 is for BOT protocol and alt-setting 1 is UASP. When we switch alt-setting, we disable the current endpoints and enable the old/new ones.
Quoted 6 lines
Thanks for pointing that use case out. Maybe we can consider seeing if we can walk through all alternate interfaces for a particular function, and resize for the largest setting? That might be a possible improvement made to the
check_config()function. Let me start makign the changes for this and verifying it.Thanks!
Currently the gadget configures early and informs the gadget driver of how many endpoints are available, which doesn't leave much room for the gadget to do optimization/reconfiguration.
If there's an option for the composite layer to inform the controller driver of the entire configuration, then we can take advantage of more dwc3 controller capability/flexibility (not just resizing txfifo).
Quoted 15 lines
Quoted 9 lines
Quoted 8 lines
I agree that there is currently a limitation because we are going to reserve at minimum 1 FIFO for BOTH alt interfaces, even though there is only 1 interface active at a time. The missing logic that we might be missing is seeing how we can re-purpose the FIFO that is being disabled.
However, I think Jack's fix here would be applicable to the improvement
in logic to re-use/re-assign FIFO space allocated by disabled EPs also.Quoted 5 lines
Improvement is always great. I just hope we don't just stop where we are now. Since you're working on this feature at the moment, it would be good to also resolve some of the outstanding issues as Jack's fix seems to be incomplete.
Quoted 4 lines
If we implement the improvement mentioned above, I think Jack's fix will be applicable there as well. If we resize for the largest alternate interface, then there would be no reason for us to resize again.
As long as you have the above as part of your roadmap, I don't mind Jack's fix for now.
Quoted 6 lines
Quoted 2 lines
Thanks for your input as always Thinh. Does the patch still look ok in its current state? Last time I had responded to Felipe that I would try
TX endpoints should have non-zero GTXFIFOSIZ. Using the register as a flag to check whether it's been resized is not ok. Also, what happened after resizing the txfifo? Do you restore its previous default value?
The choice to use the resizing algorithm is a fixed setting determined by device property. So if it is enabled for a particular platform instance, it means we don't intend to use any of the default values. All the registers will get cleared to 0 upon every Set Configuration so that each EP enable call thereafter will be starting from a clean slate.
As you correctly pointed out however, this doesn't quite work well for AltSettings where EPs may come and go well after the Set Config. This will be where future improvements will hopefully address.
Quoted 8 lines
Quoted 8 lines
to entertain adding an explicit flag to keep track of whether an EP had been resized yet or not. When trying to implement this as another DWC3_EP_* bit for
dep->flagswe'd then need to retain this flag across ep_disable/enable calls, so it looks a tiny bit cumbersome becausedep->flagsisn't neatly 0 anymore :-P.So
ep_disable()would need to look something like this:
Quoted 10 lines
Review
Quoted 7 lines
static int
__dwc3_gadget_ep_disable(struct dwc3_ep *dep){
...dep->stream_capable = false; dep->type = 0;(quoted diff) +1
-1- dep->flags = 0; + dep->flags &= DWC3_EP_TXFIFO_RESIZED;I think you mean this?
dep->flags&= ~DWC3_EP_TXFIFO_RESIZED;
No I do mean AND without any inverting, since if that flag was set we need to keep it still set after disabling.
Quoted 7 lines
Quoted 7 lines
return 0;}
The flag would be initially set in
dwc3_gadget_resize_tx_fifos()and also would need to be checked there as well to avoid re-resizing. It would then get cleared indwc3_gadget_clear_tx_fifos().
Quoted 6 lines
Quoted 3 lines
Is this still preferable to the current patch with just the single register read?
What if we change the configuration from the configfs? Would the flag persist? If that's the case, that doesn't look right.
Yes, it might still be set, but this is only a transient state. It will eventually get cleared at the time the GTXFIFOSIZ is reset back to 0 during dwc3_gadget_clear_tx_fifos() when the next Set Config (in dwc3_ep0_set_config()) is received during enumeration in the new configuration.
I can go ahead with V2 of this patch using this flag to be more clear.
Jack
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Thinh Nguyen
maintainerQuoted 135 lines · Jack Pham
Quoted 127 lines · Thinh Nguyen
Quoted 126 lines · Jack Pham
Quoted 124 lines · Thinh Nguyen
Quoted 85 lines · Wesley Cheng
Quoted 63 lines · Thinh Nguyen
Quoted 62 lines · Wesley Cheng
Quoted 41 lines · Thinh Nguyen
Quoted 40 lines · Jack Pham
Some functions may dynamically enable and disable their endpoints regularly throughout their operation, particularly when Set Interface is employed to switch between Alternate Settings. For instance the UAC2 function has its respective endpoints for playback & capture associated with AltSetting 1, in which case those endpoints would not get enabled until the host activates the AltSetting. And they conversely become disabled when the interfaces' AltSetting 0 is chosen.
With the DWC3 FIFO resizing algorithm recently added, every
usb_ep_enable()call results in a call to resize that EP's TXFIFO, but if the same endpoint is enabled again and again, this incorrectly leads to FIFO RAM allocation exhaustion as the mechanism did not account for the possibility that endpoints can be re-enabled many times.Example log splat:
dwc3 a600000.dwc3: Fifosize(3717) > RAM size(3462) ep3in depth:217973127 configfs-gadget gadget: u_audio_start_capture:521 Error! dwc3 a600000.dwc3: request 000000000be13e18 was not queued to ep3inThis is easily fixed by bailing out of
dwc3_gadget_resize_tx_fifos()if an endpoint is already resized, avoiding the calculation error resulting from accumulating the EP's FIFO depth repeatedly.Fixes 9f607a309fbe9 ("usb: dwc3: Resize TX FIFOs to meet EP bursting requirements")Signed-off-by Jack Pham <[email protected]>--- drivers/usb/dwc3/gadget.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/usb/dwc3/gadget.c +0
-0@@ -747,6 +747,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep)if (!usb_endpoint_dir_in (dep->endpoint.desc) || dep->number <=1 )return 0 ;Quoted 8 lines
drivers/usb/dwc3/gadget.c static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) · line 756
Quoted 8 lines
drivers/usb/dwc3/gadget.c +4
-0+/* bail if already resized */ +if (dwc3_readl (dwc->regs,DWC3_GTXFIFOSIZ (dep->number >>1 ))) +return 0 ; + ram1_depth =DWC3_RAM1_DEPTH (dwc->hwparams.hwparams7); if ((dep->endpoint.maxburst > 1 &&Hi Thinh,
Quoted 6 lines
This seems like a workaround more than a fix. As previously pointed out, there will be problems when there are multiple alternate setting interface [2]. If we're doing this way, are we properly allocating the fifo size for the alternate setting that requires the most fifo size and not just the first alt-setting 0? Also different alt-setting can have
Quoted 8 lines
Quoted 8 lines
Each alt interface will call
usb_ep_autoconfig()which should be assigned different endpoint numbers. This would mean that if alt intf#0 gets selected, and EP is enabled, then we will resize the TXFIFO and map that FIFO to the physical EP. Then when/if the host requests the other alt intf#1, and that calls EP enable, then the logic will then attempt to resize based on the parameters, and again map that FIFO to the physical EP. (since we call autoconfig on all interfaces, they should be assigned different endpoints)Hi Thinh,
Quoted 7 lines
That's not true. Different alt-settings of an interface can share endpoint numbers. This is often the case for UASP driver where alt-setting 0 is for BOT protocol and alt-setting 1 is UASP. When we switch alt-setting, we disable the current endpoints and enable the old/new ones.
Quoted 6 lines
Thanks for pointing that use case out. Maybe we can consider seeing if we can walk through all alternate interfaces for a particular function, and resize for the largest setting? That might be a possible improvement made to the
check_config()function. Let me start makign the changes for this and verifying it.Thanks!
Currently the gadget configures early and informs the gadget driver of how many endpoints are available, which doesn't leave much room for the gadget to do optimization/reconfiguration.
If there's an option for the composite layer to inform the controller driver of the entire configuration, then we can take advantage of more dwc3 controller capability/flexibility (not just resizing txfifo).
Quoted 15 lines
Quoted 9 lines
Quoted 8 lines
I agree that there is currently a limitation because we are going to reserve at minimum 1 FIFO for BOTH alt interfaces, even though there is only 1 interface active at a time. The missing logic that we might be missing is seeing how we can re-purpose the FIFO that is being disabled.
However, I think Jack's fix here would be applicable to the improvement
in logic to re-use/re-assign FIFO space allocated by disabled EPs also.Quoted 5 lines
Improvement is always great. I just hope we don't just stop where we are now. Since you're working on this feature at the moment, it would be good to also resolve some of the outstanding issues as Jack's fix seems to be incomplete.
Quoted 4 lines
If we implement the improvement mentioned above, I think Jack's fix will be applicable there as well. If we resize for the largest alternate interface, then there would be no reason for us to resize again.
As long as you have the above as part of your roadmap, I don't mind Jack's fix for now.
Quoted 6 lines
Quoted 2 lines
Thanks for your input as always Thinh. Does the patch still look ok in its current state? Last time I had responded to Felipe that I would try
TX endpoints should have non-zero GTXFIFOSIZ. Using the register as a flag to check whether it's been resized is not ok. Also, what happened after resizing the txfifo? Do you restore its previous default value?
Quoted 5 lines
The choice to use the resizing algorithm is a fixed setting determined by device property. So if it is enabled for a particular platform instance, it means we don't intend to use any of the default values. All the registers will get cleared to 0 upon every Set Configuration so that each EP enable call thereafter will be starting from a clean slate.
Some fields of GTXFIFOSIZ may not get cleared. Depends on the controller version, we introduce different fields (as the case for DWC32 and DWC31). So this may not apply for all the time. I just noticed that the entire GTXFIFOSIZ was written with 0. Please only write to the specific fields with proper macros.
Quoted 41 lines
As you correctly pointed out however, this doesn't quite work well for AltSettings where EPs may come and go well after the Set Config. This will be where future improvements will hopefully address.
Quoted 8 lines
Quoted 8 lines
to entertain adding an explicit flag to keep track of whether an EP had been resized yet or not. When trying to implement this as another DWC3_EP_* bit for
dep->flagswe'd then need to retain this flag across ep_disable/enable calls, so it looks a tiny bit cumbersome becausedep->flagsisn't neatly 0 anymore :-P.So
ep_disable()would need to look something like this:Quoted 10 lines
Review
Quoted 7 lines
static int
__dwc3_gadget_ep_disable(struct dwc3_ep *dep){
...dep->stream_capable = false; dep->type = 0;(quoted diff) +1
-1- dep->flags = 0; + dep->flags &= DWC3_EP_TXFIFO_RESIZED;I think you mean this?
dep->flags&= ~DWC3_EP_TXFIFO_RESIZED;No I do mean AND without any inverting, since if that flag was set we need to keep it still set after disabling.
Quoted 7 lines
Quoted 7 lines
return 0;}
The flag would be initially set in
dwc3_gadget_resize_tx_fifos()and also would need to be checked there as well to avoid re-resizing. It would then get cleared indwc3_gadget_clear_tx_fifos().Quoted 6 lines
Quoted 3 lines
Is this still preferable to the current patch with just the single register read?
What if we change the configuration from the configfs? Would the flag persist? If that's the case, that doesn't look right.
Quoted 8 lines
Yes, it might still be set, but this is only a transient state. It will eventually get cleared at the time the GTXFIFOSIZ is reset back to 0 during
dwc3_gadget_clear_tx_fifos()when the next Set Config (indwc3_ep0_set_config()) is received during enumeration in the new configuration.I can go ahead with V2 of this patch using this flag to be more clear.
I think using a flag is clearer also.
Thanks,
Thinh
Quoted 12 lines · Thinh Nguyen
Quoted 10 lines · Jack Pham
Quoted 3 lines · Thinh Nguyen
TX endpoints should have non-zero GTXFIFOSIZ. Using the register as a flag to check whether it's been resized is not ok. Also, what happened after resizing the txfifo? Do you restore its previous default value?
The choice to use the resizing algorithm is a fixed setting determined by device property. So if it is enabled for a particular platform instance, it means we don't intend to use any of the default values. All the registers will get cleared to 0 upon every Set Configuration so that each EP enable call thereafter will be starting from a clean slate.
Quoted 5 lines
Some fields of GTXFIFOSIZ may not get cleared. Depends on the controller version, we introduce different fields (as the case for DWC32 and DWC31). So this may not apply for all the time. I just noticed that the entire GTXFIFOSIZ was written with 0. Please only write to the specific fields with proper macros.
Hmm, I thought Wesley's original change already takes care to do that:
void dwc3_gadget_clear_tx_fifos(struct dwc3 *dwc)
{
...
/* Clear existing TXFIFO for all IN eps except ep0 */
for (num = 3; num < min_t(int, dwc->num_eps, DWC3_ENDPOINTS_NUM);
num += 2) {
dep = dwc->eps[num];
/* Don't change TXFRAMNUM on usb31 version */
size = DWC3_IP_IS(DWC3) ? 0 :
dwc3_readl(dwc->regs, DWC3_GTXFIFOSIZ(num >> 1)) &
DWC31_GTXFIFOSIZ_TXFRAMNUM;
dwc3_writel(dwc->regs, DWC3_GTXFIFOSIZ(num >> 1), size);
dep->flags &= ~DWC3_EP_TXFIFO_RESIZED;
}
dwc->num_ep_resized = 0;
}Just before the write, the read is masked with the TXFRAMNUM bit in case of !DWC3, i.e. DWC31 or DWC32. The rest would be 0'ed out. Sorry if my previous reply implied the entire register was written as 0.
Quoted 4 lines
Quoted 2 lines
I can go ahead with V2 of this patch using this flag to be more clear.
I think using a flag is clearer also.
Thanks, and appreciate the reviewed-by you already replied with for v2!
Jack
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Thinh Nguyen
maintainerQuoted 13 lines · Jack Pham
Quoted 12 lines · Thinh Nguyen
Quoted 10 lines · Jack Pham
Quoted 3 lines · Thinh Nguyen
TX endpoints should have non-zero GTXFIFOSIZ. Using the register as a flag to check whether it's been resized is not ok. Also, what happened after resizing the txfifo? Do you restore its previous default value?
The choice to use the resizing algorithm is a fixed setting determined by device property. So if it is enabled for a particular platform instance, it means we don't intend to use any of the default values. All the registers will get cleared to 0 upon every Set Configuration so that each EP enable call thereafter will be starting from a clean slate.
Quoted 8 lines
Quoted 5 lines
Some fields of GTXFIFOSIZ may not get cleared. Depends on the controller version, we introduce different fields (as the case for DWC32 and DWC31). So this may not apply for all the time. I just noticed that the entire GTXFIFOSIZ was written with 0. Please only write to the specific fields with proper macros.
Hmm, I thought Wesley's original change already takes care to do that:
I was referring to the field such as TXFRAMNUM that the check "if (GTXFIFOSIZ)" may not work because this field may be non-zero by default.
Quoted 11 lines
void
dwc3_gadget_clear_tx_fifos(struct dwc3 *dwc)
{
.../* Clear existing TXFIFO for all IN eps except ep0 */ for (num = 3; num < min_t(int, dwc->num_eps, DWC3_ENDPOINTS_NUM); num += 2) { dep = dwc->eps[num]; /* Don't change TXFRAMNUM on usb31 version */ size = DWC3_IP_IS(DWC3) ? 0 : dwc3_readl(dwc->regs, DWC3_GTXFIFOSIZ(num >> 1)) & DWC31_GTXFIFOSIZ_TXFRAMNUM;
I think it's better to use masks such as below since it's self-documented, but it's probably fine as is.
size &= ~(DWC3x_GTXFIFOSIZ_TXFSADDR(~0) | DWC3x_GTXFIFOSIZ_TXFDEP(~0));
Quoted 11 lines
dwc3_writel(dwc->regs, DWC3_GTXFIFOSIZ(num >> 1), size); dep->flags &= ~DWC3_EP_TXFIFO_RESIZED; } dwc->num_ep_resized = 0; }Just before the write, the read is masked with the TXFRAMNUM bit in case of !DWC3, i.e. DWC31 or DWC32. The rest would be 0'ed out. Sorry if my previous reply implied the entire register was written as 0.
For DWC32, the field TXFRAMNUM is replaced with something else, but the TXFSADDR and TXFDEP are unchanged since DWC31. Check that we don't unintentionally overwrite bit(15) of GTXFIFOSIZ for DWC32 and DWC31 when updating txfifo.
Thanks,
Thinh