drivers: udc: rework TXFF config in udc_dwc2 driver by zjian-zhang · Pull Request #95654 · zephyrproject-rtos/zephyr

zjian-zhang · 2025-09-08T11:08:59Z

when endpoint enable, search for all txfifos and assigne the smallest acceptable txfifo to IN endpoint, update the corresponding txfunm and txff bitmap. For example, TXFF1 depth is 256B, TXFF2 depth is 64B, TXFF3 depth is 512B, EP1 mps is 64B, and EP2 mps is 256B. Then, search and select the smallest available TXFF in order, so EP1 selects TXFF2 and EP2 selects TXFF1.

Fixes #95500

tmon-nordic · 2025-09-08T13:24:02Z

+		if (!(priv->txf_set & BIT(i)) && (dep <= priv->max_txfifo_depth[i])) {
+			LOG_DBG("Find new TX FIFO %u for ep 0x%02x with depth %d", i, cfg->addr,
+				priv->max_txfifo_depth[i]);
+
+			cfg->txff_num = i;
+			return 0;
+		}


This will select the first fifo that matches. Not sure if it is looking too much into future, but should it actually always try to select the smallest TxFIFO that is capable of handling the endpoint?

So for example when there is an application with IN endpoint 1 with max packet size 64, and IN endpoint 2 with max packet size 512 that the endpoint 1 will get the smaller TxFIFO even if it is enabled first.

Yes, the method you proposed is better. For example, TXFF1 depth is 256B, TXFF2 depth is 64B, TXFF3 depth is 512B, EP1 mps is 64B, and EP2 mps is 256B. Then, search and select the smallest available TXFF in order, so EP1 selects TXFF2 and EP2 selects TXFF1. The new submission has been modified according to this method.

Please update commit message to reflect the new approach.

jfischer-no · 2025-09-08T13:58:02Z

This commit is not acceptable. Please avoid making this kind of change in the future. The existing code already follows coding style guidelines. For all your further commits, keep the style consistent with the USB drivers/subsystem code. Note that clangformat fails to format code properly, so you will need to fix it afterwards.

The existing code does not conform to the coding style as checked by ${ZEPHYR_BASE}/scripts/checkpatch.pl. I am proceeding with this commit using --no-verify for this instance, as the contribution guidelines state that contributors are not expected to fix style issues in code they are not otherwise modifying.

jfischer-no · 2025-09-08T14:01:30Z

When the TXFFn.depth assigned to EPn is not met, search for other
available TXFFs in order and maintain the corresponding relationship
by the new txffnum variable.

I cannot find TXFFn in the databook. What are you referring to with TXFF and what are you trying to fix?

This refers to INEPnTxFDep field in DIEPTXFx registers. Then the actual fifo to use is assigned by setting TxFNum field in DIEPCTLx registers.

tmon-nordic · 2025-09-12T11:41:26Z

+
+			/* Do not allocate TxFIFO outside the SPRAM */
+			if (*txfaddr + reqdep > priv->dfifodepth) {
+				/* TODO: reduce RX FIFO for new TX FIFO */


Any plans on how this TODO could be implemented? Is it even possible to resize RxFIFO after enabling device?

sorry that I didn't consider it thoroughly. The timing for resizing may not be right. I add TODO here is that the UDC_DWC2_GRXFSIZ_HS_DEFAULT is 0x314, while our total DIFO depth is 0x400, leaving no more space for two IN EP with mps 512, which will return error. I see the comment that set UDC_DWC2_GRXFSIZ_HS_DEFAULT to receive 3 * 1024 packets(supports high-bandwidth EPs) . I wonder if we should add a kconfig option to identify high-bandwidth support, so as to reserve space for TXFF?

I don't think adding Kconfig options is a solution for this. There are some approaches to pass Maximum Packet Size (#92831, #94766) to driver so it can be used to make better decision, but so far none are merged.

Thank you for updating us on the progress. We hope to receive support for this as soon as possible.

when endpoint enable, search for all txfifos and assigne the smallest acceptable txfifo to IN endpoint, update the corresponding txfunm and txff bitmap. Signed-off-by: zjian zhang <zjian_zhang@realsil.com.cn>

sonarqubecloud · 2025-09-16T08:23:49Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

tmon-nordic · 2025-09-16T11:48:42Z

+	*txfaddr = usb_dwc2_get_gnptxfsiz_nptxfdep(gnptxfsiz) +
+		   usb_dwc2_get_gnptxfsiz_nptxfstaddr(gnptxfsiz);
+
+	for (uint8_t i = 1U; i < priv->numdeveps; i++) {


If return; in dwc2_unset_unused_fifo() is deliberate action to make this function simpler (no need to handle gaps that arise from alternate setting changes), then the loop here should go from highest to lowest, just like it does in dwc2_unset_unused_fifo().

Please let me know if I missed something in the analysis, I do think that going from highest to lowest will work, but I am not certain.

Yes, from highest to lowest does indeed reduce the loop, as long as the last enabled EP is found, the next txfaddr will be calculated.Because the order of EP enable always starts from EP1. Thank you very much for the reminder.

But the reason why I have been like this is because of the previous EP re-enable situation(#92520) . If EP1, EP2, and EP3 have already been enabled, and EP2 is re-enable, the loop from highest to lowest will make an error. I am also puzzled about why EP2 re-enable happens. @jfischer-no Could you please help me check if this modification is useful for this situation?

When there are endpoints 1, 2, 3, 4, 5, 6, 7, 8 (enabled in order) if the loop goes from lowest to highest, then wouldn't it essentially re-use the same fifo starting locations for endpoints 2, 3, 4, 5, 6, 7, 8?

If this goes from highest to lowest, then the addresses will be sequential and "the gap problem" is completely avoided.

For FIFO re-use, wouldn't it be enough to have if (txfnum && (priv->txf_set & BIT(txfnum))) { /* handle fifo re-use */ } before the loop?

The above situations are all during configuration stage without EP transmission. If other EPs are transmitting, changing Txffaddr may cause problems. Do you have any other better ideas?

The problem is that you cannot re-arrange other endpoints at will (endpoint may be armed with data and in middle of large transfer using DMA). Therefore I think the simple solution of "actually free only highest assigned number" + "reiterate if needed" should be acceptable, at least initially.

The "deactivated, but not freed" endpoints would be subject to reuse currently assigned fifos (including start + depth) if they were not actually freed due to some other endpoint having assigned fifo with higher starting address.

Do you mean this way:

EP2 disable&re-enable:

disable EP2： since it is not the last IN EP with the highest assigned txffaddr, DIEPCTL2.txfnum and bitmap are retained in dwc2_unset_dedicated_fifo.

re-enable EP2： because EP3 is enabled and has the highest assigned txffaddr so return; in dwc2_unset_unused_fifo . though EP2 tmp ->stat. enabled is 0, it still has DIEPCTL.txfnum, so continue to use TXFFj addr and depth in dwc2_update_txfadr_txfdep: if ((txfnum && (priv->txf_set & BIT(txfnum))) and won't affect other EPs transfer.

EP3 disable&re-enable:

disable EP3: since it is last IN EP with the highest assigned txffaddr, clear DIEPCTL3.txfnum and bitmap in dwc2_unset_dedicated_fifo.

re-enable EP3: becase EP3 txfnum and bitmap all clear, re-assign TXFF in` dwc2_uupdate_txfadr_txfdep just like before.

if so then i need todo:

add loop in dwc2_unset_dedicated_fifo go from highest to lowest, like it does in dwc2_unset_unused_fifo() and check if it is the highest txffaddr.

and add flow to get highest txffaddr in dwc2_update_txfadr_txfdep no matter whether loop goes from lowest to highest or from highest to lowest (in case of alternate setting: EP2 enable and then EP1 enable, EP1 will have highest txffaddr)

But since TXFF has not been reassigned twice, it can be considered fixed after first allocation, and even if EP is disabled or re-enabled, this TXFF will be reused afterwards. Perhaps we can refer to Linux's fixed params for TXFF addr and depth, and then use EP.txffnum to select.
As for the gap, the entire FIFO is mapping after core init, there should be no difference that unused positions at the end or in the middle, as long as the EP txffaddr do not overlap with each other.

Do you mean this way: 1. EP2 disable&re-enable [...] 2. EP3 disable&re-enable [...]

Yes.

Perhaps we can refer to Linux's fixed params for TXFF addr and depth

This would be so much simpler IMHO. The only problem is that the user would have to adapt the devicetree to match what addresses the "endpoint fixup" will come up with (it is currently impossible to application to specify exactly which endpoint, the "fixup" always assigns it at runtime).

As for the gap, the entire FIFO is mapping after core init, there should be no difference that unused positions at the end or in the middle, as long as the EP txffaddr do not overlap with each other.

Yes, this is true. Unused portions are not a functional problem (just potentially wasted resources), only overlaps are a real problem.

sylvioalves · 2025-09-30T16:13:10Z

-			LOG_WRN("Some of the FIFOs higher than %u are set, %lx",
-				ep_idx, priv->txf_set & ~BIT_MASK(ep_idx));
-			return 0;
+		if (priv->txf_set & BIT_MASK(txfnum)) {


In #96804, I added a check to guarantee that current endpoint was not being tested. It sounds this is not needed anymore, right?

sylvioalves · 2025-09-30T16:13:15Z

 		}

 		/* Set FIFO depth (32-bit words) and address */
-		dwc2_set_txf(dev, ep_idx - 1, txfdep, txfaddr);
+		dwc2_set_txf(dev, txfnum - 1, txfdep, txfaddr);


Is there any possibilty for txfnum = 0? If so, some check could be addressed.

Ctrl EP0 usually remains activated after being enabled, so TXFF0 is fixed to CTRP EP0, and assign other TXFF to IN EP1,EP2...

sylvioalves · 2025-09-30T16:13:24Z

+
+			/* Do not allocate TxFIFO outside the SPRAM */
+			if (*txfaddr + reqdep > priv->dfifodepth) {
+				return -ENOMEM;


Should you also check for *txfdep as well?

if (*txfaddr + *txfdep > priv->dfifodepth) { return -ENOMEM; }

sylvioalves · 2025-09-30T16:14:27Z

Tested with ESP32-S3. Working good.

github-actions · 2025-12-09T00:40:41Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

zephyrbot added the area: USB Universal Serial Bus label Sep 8, 2025

zephyrbot requested review from jfischer-no and tmon-nordic September 8, 2025 11:09

zephyrbot assigned jfischer-no Sep 8, 2025

tmon-nordic reviewed Sep 8, 2025

View reviewed changes

jfischer-no requested changes Sep 8, 2025

View reviewed changes

zjian-zhang force-pushed the usb_txff branch from 2591f54 to d3f2895 Compare September 10, 2025 02:40

zjian-zhang requested review from jfischer-no and tmon-nordic September 11, 2025 07:09

tmon-nordic reviewed Sep 12, 2025

View reviewed changes

drivers: udc: rework TXFF config in udc_dwc2 driver

682c773

when endpoint enable, search for all txfifos and assigne the smallest acceptable txfifo to IN endpoint, update the corresponding txfunm and txff bitmap. Signed-off-by: zjian zhang <zjian_zhang@realsil.com.cn>

zjian-zhang force-pushed the usb_txff branch from d3f2895 to 682c773 Compare September 16, 2025 07:55

zjian-zhang requested a review from tmon-nordic September 16, 2025 08:32

tmon-nordic reviewed Sep 16, 2025

View reviewed changes

tmon-nordic requested review from marekmatej, raffarost and sylvioalves September 17, 2025 04:45

jean12332 mentioned this pull request Sep 18, 2025

add usb fifo config in dts for dwc2 USB driver #96206

Open

tmon-nordic mentioned this pull request Sep 30, 2025

udc_dwc2: fix off-by-one in TX FIFO unset check #96804

Merged

sylvioalves reviewed Sep 30, 2025

View reviewed changes

github-actions Bot added the Stale label Dec 9, 2025

github-actions Bot closed this Dec 23, 2025

tmon-nordic mentioned this pull request Mar 17, 2026

dts: bindings: usb: Specify fifo sizes in devicetree #99510

Open

Conversation

zjian-zhang commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jean12332 Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sonarqubecloud Bot commented Sep 16, 2025

Quality Gate passed

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmon-nordic Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jean12332 Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jean12332 Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sylvioalves Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sylvioalves commented Sep 30, 2025

Uh oh!

github-actions Bot commented Dec 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

zjian-zhang commented Sep 8, 2025 •

edited

Loading

jean12332 Sep 10, 2025 •

edited

Loading

tmon-nordic Sep 17, 2025 •

edited

Loading

jean12332 Sep 17, 2025 •

edited

Loading

jean12332 Sep 18, 2025 •

edited

Loading

sylvioalves Sep 30, 2025 •

edited

Loading