dts: bindings: usb: Specify fifo sizes in devicetree #99510
Conversation
tmon-nordic
requested review from
carlescufi,
jfischer-no,
josuah and
raffarost
zephyrbot
added
platform: nRF
zephyrbot
requested review from
anangl,
bjarki-andreasen,
gdengi,
jaz1-nordic,
kl-cruz,
lstnl,
magp-nordic,
masz-nordic and
nika-nordic
|
|
||
| usb0: usb@ffb30000 { | ||
| compatible = "snps,dwc2"; | ||
| reg = <0xffb30000 0xffff>; |
There was a problem hiding this comment.
@nashif I found fifo information in Cyclone V HPS Register Address Map and Definitions but there the usb0 is at 0xFFB00000 and not 0xFFB30000. Is it a bug in zephyr devicetree or am I looking at wrong place?
|
@raffarost Can you provide the devicetree values from esp32-s3? |
|
@jean12332 can you verify whether this also fixes #95500? |
hi @tmon-nordic, |
What is actual GDFIFOCFG register value? The upper 16 bits show how many locations are available for fifos, the lower 16 bits show total SPRAM size. When DMA is used, some locations are used for storing DMA pointers (this is where the difference between the upper 16 bits and lower 16 bits come from). GHWCFG3 upper 16 bits report available DFIFO Depth (after reset should be the same as upper 16 bits from GDFIFOCFG register). |
tmon-nordic
force-pushed
the
dwc2-fifos
branch
from
81ea271 to
7c6577c
Compare
zephyrbot
added
area: Xtensa
tmon-nordic
force-pushed
the
dwc2-fifos
branch
from
7c6577c to
e95f5bb
Compare
tmon-nordic
force-pushed
the
dwc2-fifos
branch
from
e95f5bb to
73d7277
Compare
GDFIFOCFG = 0x00c8 0100 (200 and 256 words) |
Therefore the entries added in this PR should work. Could you please run some USB application on esp32-s3 with this PR with enabled asserts to see if there isn't any surprise there? (the only potential surprise would be that some power-on reset value would be lower than specified value; if that's the case then assert should catch it). |
I tested As you mentioned, total words should be 200 instead? If TX fifo is too small, maybe reducing the number of endpoints? Not sure what's best. |
Yes, it should be total 200. The higher locations are used by endpoint controller. I don't know why my brain didn't realize that 64 + 32 + 40 + 40 + 40 + 40 = 256 and not 200. Hopefully, I added the assert...
With such low amount of SPRAM, I think it makes sense to sacrifice on control IN endpoint and then to give 3 endpoints 32 locations (to be able to hold 2 bulk packets each) and then the remaining one 24 locations. would be my take for generic configuration. Can you verify if such configuration works? |
Tested on two samples, LGTM. |
| static struct udc_ep_config ep_cfg_in[DT_INST_PROP(n, num_in_eps)]; \ | ||
| \ | ||
| BUILD_ASSERT(DT_INST_PROP_LEN(n, g_tx_fifo_size) == \ | ||
| (DT_INST_PROP(n, ghwcfg4) & USB_DWC2_GHWCFG4_INEPS_MASK) >>\ |
There was a problem hiding this comment.
I've started to use macros for extracting things out of ghwcfg# here if that helps, maybe I can put them on drivers/usb/common/usb_dwc2_hw.h as a separate PR.
#define UHC_DWC2_CHAN_REG(base, chan_idx) \
((struct usb_dwc2_host_chan *)(((mem_addr_t)(base)) + 0x500UL + ((chan_idx) * 0x20UL)))
#define UHC_DWC2_FIFODEPTH(config) \
((uint32_t)(((config)->ghwcfg3 & USB_DWC2_GHWCFG3_DFIFODEPTH_MASK) >> \
USB_DWC2_GHWCFG3_DFIFODEPTH_POS))
#define UHC_DWC2_NUMHSTCHNL(config) \
((uint32_t)(((config)->ghwcfg2 & USB_DWC2_GHWCFG2_NUMHSTCHNL_MASK) >> \
USB_DWC2_GHWCFG2_NUMHSTCHNL_POS))
#define UHC_DWC2_OTGARCH(config) \
((uint32_t)(((config)->ghwcfg2 & USB_DWC2_GHWCFG2_OTGARCH_MASK) >> \
USB_DWC2_GHWCFG2_OTGARCH_POS))
#define UHC_DWC2_HSPHYTYPE(config) \
((uint32_t)(((config)->ghwcfg2 & USB_DWC2_GHWCFG2_HSPHYTYPE_MASK) >> \
USB_DWC2_GHWCFG2_HSPHYTYPE_POS))
#define UHC_DWC2_FSPHYTYPE(config) \
((uint32_t)(((config)->ghwcfg2 & USB_DWC2_GHWCFG2_FSPHYTYPE_MASK) >> \
USB_DWC2_GHWCFG2_FSPHYTYPE_POS))
#define UHC_DWC2_PHYDATAWIDTH(config) \
((uint32_t)(((config)->ghwcfg4 & USB_DWC2_GHWCFG4_PHYDATAWIDTH_MASK) >> \
USB_DWC2_GHWCFG4_PHYDATAWIDTH_POS))There was a problem hiding this comment.
Your macros rely on there being ghwcfg members inside config structure. For BUILD_ASSERT it would be necessary to have a macro that takes in just the ghwcfg value directly.
While we do have the getters and setters for register fields, these are really functions - which implies that they cannot be used in BUILD_ASSERT.
I am not sure in how many places the macro would be used. If you are about to move to drivers/usb/common/usb_dwc2_hw.h then I think it owuld make sense to have the macro take ghwcfg member and not config (and not include UHC prefix because at least OTGARCH, HSPHYTYPE, FSPHYTYPE and PHYDATAWIDTH are not exclusive for host mode).
Add g-rx-fifo-size, g-np-tx-fifo-size and g-tx-fifo-size required properties to snps,dwc2 compatible. Property names are the same as used in Linux because Zephyr aims for devicetree source compatibility with other operating systems. Specifying fifo sizes in devicetree greatly reduces necessary driver complexity and allows application developer to adjust the sizes if necessary to best utilize underlying hardware. Signed-off-by: Tomasz Moń <[email protected]>
Dynamically allocating fifo sizes leads to very complex code that yields
subpar results. DWC2 controller has very specific requirements related
to configured FIFO sizes that significantly increase the complexity:
* each fifo has specific upper bound on both size and starting
location (fixed for each silicon)
* assigned locations must be contiguous
* assigned locations cannot be changed on-the-fly
Zephyr tried to dynamically assign fifos using simple but essentially
broken algorithm that:
* assumed 1-to-1 endpoint number to TxFIFO mapping
DWC2 controller can be configured with non-contiguous IN endpoint
numbers, but TxFIFOs are always contiguous.
* allocated minimum required space for each endpoint
There is no benefit in using less SPRAM than hardware has available,
but using only 128 locations per bulk endpoint significantly limits
maximum transfer rate when using DMA. At least 256 locations allows
DMA to load next packet data before previous packet is transmitted
on the bus. Actual performance degradation depends on clock rates
and latencies. MSC on nRF54H20 using bulk endpoints with only 128
locations can achieve no more than 15.6 MB/s throughput, while with
256 locations speeds up to 36.9 MB/s are possible (running otherwise
identical software).
* used one-size-fits-all defaults for RxFIFO
Remedied with some upper limits calculated at runtime, but the limit
was not taking into account complete configuration.
* was unable to effectively handle multiple alternate settings
This did lead to set interface failures where e.g. HID interface
with wMaxPacketSize > 64 was not using the highest numbered IN
endpoint within active configuration.
Instead of trying to come up with bandaid for each issue that would
significantly complicate the code, just shift the responsibility towards
firmware developer. While for many typical applications board defaults
are perfectly fine, the application developer may want to come up with
specific devicetree overrides best suited for their use case.
Signed-off-by: Tomasz Moń <[email protected]>
tmon-nordic
force-pushed
the
dwc2-fifos
branch
from
73d7277 to
bc89220
Compare
|
5 participants
Add g-rx-fifo-size, g-np-tx-fifo-size and g-tx-fifo-size required properties to snps,dwc2 compatible. Property names are the same as used in Linux because Zephyr aims for devicetree source compatibility with other operating systems.
Specifying fifo sizes in devicetree greatly reduces necessary driver complexity and allows application developer to adjust the sizes if necessary to best utilize underlying hardware.
Implements #96206