Skip to content

drivers: udc: rework TXFF config in udc_dwc2 driver #95654

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
zjian-zhang wants to merge 1 commit into from
Closed

drivers: udc: rework TXFF config in udc_dwc2 driver #95654

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
179 changes: 132 additions & 47 deletions drivers/usb/udc/udc_dwc2.c
View file
Open in desktop
Copy link
Copy Markdown
Contributor

@jfischer-no jfischer-no Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@zjian-zhang zjian-zhang Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@jfischer-no jfischer-no Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

@tmon-nordic tmon-nordic Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
Expand Up @@ -1345,17 +1345,115 @@ static void dwc2_unset_unused_fifo(const struct device *dev)
struct udc_dwc2_data *const priv = udc_get_private(dev);
struct udc_ep_config *tmp;

for (uint8_t i = priv->ineps - 1U; i > 0; i--) {
for (uint8_t i = priv->numdeveps - 1U; i > 0; i--) {
tmp = udc_get_ep_cfg(dev, i | USB_EP_DIR_IN);

if (tmp->stat.enabled && (priv->txf_set & BIT(i))) {
if (tmp == NULL) {
continue;
}

mem_addr_t diepctl_reg = dwc2_get_dxepctl_reg(dev, tmp->addr);
uint32_t diepctl = sys_read32(diepctl_reg);
uint32_t txfnum = usb_dwc2_get_depctl_txfnum(diepctl);

if (tmp->stat.enabled && (priv->txf_set & BIT(txfnum))) {
Comment thread
tmon-nordic marked this conversation as resolved.
return;
}

if (!tmp->stat.enabled && (priv->txf_set & BIT(i))) {
priv->txf_set &= ~BIT(i);
if (!tmp->stat.enabled && (priv->txf_set & BIT(txfnum))) {
LOG_DBG("Unset unused FIFO %u", txfnum);
priv->txf_set &= ~BIT(txfnum);
diepctl &= ~USB_DWC2_DEPCTL_TXFNUM_MASK;
sys_write32(diepctl, diepctl_reg);
}
}
}

static int dwc2_update_txfaddr_txfdep(const struct device *dev,
struct udc_ep_config *const cfg, uint32_t reqdep,
uint32_t *txfaddr, uint32_t *txfdep)
{
struct udc_dwc2_data *const priv = udc_get_private(dev);
struct usb_dwc2_reg *const base = dwc2_get_base(dev);
uint32_t gnptxfsiz = sys_read32((mem_addr_t)&base->gnptxfsiz);
struct udc_ep_config *tmp;
int ret = 0;

*txfaddr = usb_dwc2_get_gnptxfsiz_nptxfdep(gnptxfsiz) +
usb_dwc2_get_gnptxfsiz_nptxfstaddr(gnptxfsiz);

for (uint8_t i = 1U; i < priv->numdeveps; i++) {
Copy link
Copy Markdown
Contributor

@tmon-nordic tmon-nordic Sep 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Copy Markdown
Contributor

@tmon-nordic tmon-nordic Sep 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

@jean12332 jean12332 Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

@jean12332 jean12332 Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

@tmon-nordic tmon-nordic Sep 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown

@jean12332 jean12332 Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

@tmon-nordic tmon-nordic Sep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

@jean12332 jean12332 Sep 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean this way:

  1. 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.
  1. 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)

Copy link
Copy Markdown

@jean12332 jean12332 Sep 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@tmon-nordic tmon-nordic Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

tmp = udc_get_ep_cfg(dev, i | USB_EP_DIR_IN);

if (tmp == NULL) {
continue;
}

mem_addr_t diepctl_reg = dwc2_get_dxepctl_reg(dev, tmp->addr);
uint32_t diepctl = sys_read32(diepctl_reg);
uint32_t txfnum = usb_dwc2_get_depctl_txfnum(diepctl);

if (tmp->stat.enabled && (priv->txf_set & BIT(txfnum))) {
if (cfg->addr != tmp->addr) {
*txfaddr = dwc2_get_txfdep(dev, txfnum - 1) +
dwc2_get_txfaddr(dev, txfnum - 1);
} else {
uint32_t curaddr = dwc2_get_txfaddr(dev, txfnum - 1);
uint32_t curdep = dwc2_get_txfdep(dev, txfnum - 1);

LOG_DBG("ep 0x%02x re-enable", cfg->addr);
if (*txfaddr != curaddr || reqdep > curdep) {
LOG_ERR("FIFO%u cannot be reused", txfnum);
ret = -ENOMEM;
}
break;
}

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

@sylvioalves sylvioalves Sep 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you also check for *txfdep as well?

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

}
}
}

LOG_DBG("Find new TX FIFO addr 0x%04x", *txfaddr);
*txfdep = reqdep;
return ret;
}

static int dwc2_select_dedicated_fifo(const struct device *dev,
const uint32_t dep, const uint32_t addr,
uint32_t *txfnum)
{
struct udc_dwc2_data *const priv = udc_get_private(dev);
uint16_t depth_tmp = UINT16_MAX;
int ret = -ENOMEM;
struct udc_ep_config *tmp;

for (uint8_t i = 1; i < priv->numdeveps; i++) {
tmp = udc_get_ep_cfg(dev, i | USB_EP_DIR_IN);

if (tmp == NULL) {
continue;
}

if (!(priv->txf_set & BIT(i)) && (dep <= priv->max_txfifo_depth[i])) {
LOG_DBG("Find new TX FIFO%u with depth %d",
i, priv->max_txfifo_depth[i]);

if (depth_tmp > priv->max_txfifo_depth[i]) {
/* Select the smallest available TX FIFO */
depth_tmp = priv->max_txfifo_depth[i];
*txfnum = i;
}
ret = 0;
}
}

if (ret) {
LOG_ERR("No available TX FIFO");
}
return ret;
}

/*
Expand All @@ -1373,9 +1471,12 @@ static int dwc2_set_dedicated_fifo(const struct device *dev,
uint32_t reqdep;
uint32_t txfaddr;
uint32_t txfdep;
uint32_t txfnum;
uint32_t tmp;
int ret;

/* Keep everything but FIFO number */
txfnum = usb_dwc2_get_depctl_txfnum(*diepctl);
tmp = *diepctl & ~USB_DWC2_DEPCTL_TXFNUM_MASK;

reqdep = DIV_ROUND_UP(udc_mps_ep_size(cfg), 4U);
Expand All @@ -1387,44 +1488,30 @@ static int dwc2_set_dedicated_fifo(const struct device *dev,
}

if (priv->dynfifosizing) {
if (priv->txf_set & ~BIT_MASK(ep_idx)) {
if (priv->txf_set & ~BIT_MASK(txfnum)) {
dwc2_unset_unused_fifo(dev);
}

if ((ep_idx - 1) != 0U) {
txfaddr = dwc2_get_txfdep(dev, ep_idx - 2) +
dwc2_get_txfaddr(dev, ep_idx - 2);
} else {
txfaddr = priv->rxfifo_depth +
MIN(UDC_DWC2_FIFO0_DEPTH, priv->max_txfifo_depth[0]);
}
ret = dwc2_update_txfaddr_txfdep(dev, cfg, reqdep, &txfaddr, &txfdep);

if (priv->txf_set & BIT(ep_idx)) {
uint32_t curaddr;

curaddr = dwc2_get_txfaddr(dev, ep_idx - 1);
txfdep = dwc2_get_txfdep(dev, ep_idx - 1);
if (txfaddr != curaddr || reqdep > txfdep) {
LOG_ERR("FIFO%u cannot be reused, new addr 0x%04x depth %u",
ep_idx, txfaddr, reqdep);
return -ENOMEM;
}
} else {
txfdep = reqdep;
if (ret) {
return -ENOMEM;
}

/* Make sure to not set TxFIFO greater than hardware allows */
if (txfdep > priv->max_txfifo_depth[ep_idx]) {
return -ENOMEM;
}
if (cfg->stat.enabled && (priv->txf_set & BIT(txfnum))) {
LOG_DBG("ep 0x%02x is re-enabled FIFO%u", cfg->addr, txfnum);
} else {
/* Search for the smallest acceptable fifo */
ret = dwc2_select_dedicated_fifo(dev, reqdep, txfaddr, &txfnum);

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

/* 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);
Copy link
Copy Markdown
Contributor

@sylvioalves sylvioalves Sep 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

@jean12332 jean12332 Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else {
txfdep = dwc2_get_txfdep(dev, ep_idx - 1);
txfaddr = dwc2_get_txfaddr(dev, ep_idx - 1);
Expand All @@ -1433,16 +1520,17 @@ static int dwc2_set_dedicated_fifo(const struct device *dev,
return -ENOMEM;
}

txfnum = ep_idx;
LOG_DBG("Reuse FIFO%u addr 0x%08x depth %u", ep_idx, txfaddr, txfdep);
}

/* Assign FIFO to the IN endpoint */
*diepctl = tmp | usb_dwc2_set_depctl_txfnum(ep_idx);
priv->txf_set |= BIT(ep_idx);
dwc2_flush_tx_fifo(dev, ep_idx);
*diepctl = tmp | usb_dwc2_set_depctl_txfnum(txfnum);
priv->txf_set |= BIT(txfnum);
dwc2_flush_tx_fifo(dev, txfnum);

LOG_INF("Set FIFO%u (ep 0x%02x) addr 0x%04x depth %u size %u",
ep_idx, cfg->addr, txfaddr, txfdep, dwc2_ftx_avail(dev, ep_idx));
LOG_INF("Set FIFO%u (ep 0x%02x) addr 0x%04x depth %u",
txfnum, cfg->addr, txfaddr, txfdep);

return 0;
}
Expand Down Expand Up @@ -1563,8 +1651,9 @@ static int udc_dwc2_ep_activate(const struct device *dev,
}

for (uint8_t i = 1U; i < priv->ineps; i++) {
LOG_DBG("DIEPTXF%u %08x DIEPCTL%u %08x",
i, sys_read32((mem_addr_t)&base->dieptxf[i - 1U]), i, dxepctl);
LOG_DBG("DIEPCTL%u %08x DIEPTXF%u %08x DTXFSTS%u %u",
i, dxepctl, i, sys_read32((mem_addr_t)&base->dieptxf[i - 1]),
i, dwc2_ftx_avail(dev, i));
}

return 0;
Expand All @@ -1575,22 +1664,18 @@ static int dwc2_unset_dedicated_fifo(const struct device *dev,
uint32_t *const diepctl)
{
struct udc_dwc2_data *const priv = udc_get_private(dev);
uint8_t ep_idx = USB_EP_GET_IDX(cfg->addr);
uint32_t txfnum = usb_dwc2_get_depctl_txfnum(*diepctl);

/* Clear FIFO number field */
*diepctl &= ~USB_DWC2_DEPCTL_TXFNUM_MASK;

if (priv->dynfifosizing) {
if (priv->txf_set & ~BIT_MASK(ep_idx)) {
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)) {
Copy link
Copy Markdown
Contributor

@sylvioalves sylvioalves Sep 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

dwc2_set_txf(dev, txfnum - 1, 0, 0);
}

dwc2_set_txf(dev, ep_idx - 1, 0, 0);
}

priv->txf_set &= ~BIT(ep_idx);
priv->txf_set &= ~BIT(txfnum);

return 0;
}
Expand Down Expand Up @@ -2218,7 +2303,7 @@ static int udc_dwc2_init_controller(const struct device *dev)
LOG_DBG("RX FIFO size %u bytes", priv->rxfifo_depth * 4);
for (uint8_t i = 1U; i < priv->ineps; i++) {
LOG_DBG("TX FIFO%u depth %u addr %u",
i, priv->max_txfifo_depth[i], dwc2_get_txfaddr(dev, i));
i, priv->max_txfifo_depth[i], dwc2_get_txfaddr(dev, i - 1));
}

if (udc_ep_enable_internal(dev, USB_CONTROL_EP_OUT,
Expand Down