drivers: usb: dwc2: esp32: Add support #85600
Conversation
raffarost
force-pushed
the
feature/usb_otg
branch
from
4353128 to
5843cdc
Compare
|
The following west manifest projects have changed revision in this Pull Request:
⛔ DNM label due to: 1 project with PR revision Note: This message is automatically posted and updated by the Manifest GitHub Action. |
zephyrbot
added
manifest
manifest-hal_espressif
DNM (manifest)
raffarost
force-pushed
the
feature/usb_otg
branch
from
5843cdc to
a3360fa
Compare
zephyrbot
added
area: USB
zephyrbot
requested review from
andyross,
ceolin,
dcpleung,
jfischer-no,
kartben,
LucasTambor,
marekmatej,
nashif,
sylvioalves,
tmon-nordic,
uLipe and
wmrsouza
raffarost
force-pushed
the
feature/usb_otg
branch
from
a3360fa to
2d10bca
Compare
| static void udc_dwc2_irq_enable_func_##n(const struct device *dev) \ | ||
| { \ | ||
| IRQ_CONNECT(DT_INST_IRQN(n), \ | ||
| DT_INST_IRQ(n, priority), \ | ||
| udc_dwc2_isr_handler, \ | ||
| DEVICE_DT_INST_GET(n), \ | ||
| DW_IRQ_FLAGS(n)); \ | ||
| \ | ||
| irq_enable(DT_INST_IRQN(n)); \ | ||
| } \ | ||
| \ | ||
| static void udc_dwc2_irq_disable_func_##n(const struct device *dev) \ | ||
| { \ | ||
| irq_disable(DT_INST_IRQN(n)); \ | ||
| } \ | ||
| UDC_DWC2_IRQ_DT_INST_DEFINE(n) \ |
There was a problem hiding this comment.
Looks like a stray change that was not justified in the commit message. Anyway, I do not agree with it, you should use irq_enable()/irq_disable().
There was a problem hiding this comment.
for the time being we are using Espressif's interrupt allocator
I thought about turning it into a quirk API call, but it would be probably disliked as a solution
do you have a different suggestion?
There was a problem hiding this comment.
I do not understand the problem with irq_enable/irq_disable. How was it accepted not to use generic API for your platform? Are you working on fixing it?
There was a problem hiding this comment.
I did some checking, and the current behavior for ESP Xtensa chips is: Zephyr's API is used to control non-muxed interrupt lines (such as wifi, systimer). Most peripherals such as USB allow routing sources through an interrupt matrix and for this, the interrupt controller API is used. I'm not sure how much this deviates from the standard implementation, but I believe significant changes would be needed to otherwise accommodate this.
There was a problem hiding this comment.
@jfischer-no, for esp32 device, we still need the irq calls in there, otherwise it is no-go for now. Would you review it again?
drivers/usb/udc/udc_dwc2.c
Outdated
| */ | ||
| #define UDC_DWC2_GRXFSIZ_FS_DEFAULT (15U + 512U/4U) | ||
| #define UDC_DWC2_GRXFSIZ_FS_DEFAULT (15U + 2 * (64U/4U + 1)) |
There was a problem hiding this comment.
Largest endpoint MPS is not 64.
There was a problem hiding this comment.
can you help figuring out how to properly size the FIFO?
our device has FIFO depth of 200 words, and I couldn't figure out how to properly downsize it
There was a problem hiding this comment.
Is your PHY supporting only Full-Speed?
There was a problem hiding this comment.
yes, initially we're adding support to ESP32S3, with integrated FS transceiver
There was a problem hiding this comment.
The current FIFO assignment code is completely broken in the DWC2 driver IMHO. I think it cannot be really fixed without providing the driver with information about available device configurations (how many endpoints, what type and with what maximum packet size). Such information would be necessary upfront because once the device is enabled, the RX fifo cannot be resized (and similarly once IN endpoint is enabled, the TX fifo cannot be resized).
Only if such full information was somehow exposed, then the driver could determine how much space it can assign to each endpoint. If there would be not enough FIFO to facilitate all endpoints, the driver could then decide to e.g. enable thresholding only for IN endpoints, or for all endpoints.
This is not that much hitting issue for current Nordic Semiconductor devices because the FIFO is large enough for most projects. However, with smaller SPRAM configurations, this is much more pressing issue (like here for esp32).
If you have an idea how to modify the stack to enable the driver to know this information, I would gladly review it.
There was a problem hiding this comment.
@tmon-nordic, your suggestion is super valid but I think it could be handled in a separate PR for now. Would you review it again and let us know whether you think re-work is needed? We do want this to move forward.
samples/subsys/usb/cdc_acm/boards/esp32s3_devkitm_procpu.overlay
Outdated
Show resolved
Hide resolved
|
@jfischer-no |
raffarost
force-pushed
the
feature/usb_otg
branch
from
2d10bca to
f764216
Compare
raffarost
force-pushed
the
feature/usb_otg
branch
from
f764216 to
c11da1c
Compare
raffarost
requested review from
jfischer-no and
tmon-nordic
and removed request for
tmon-nordic
raffarost
force-pushed
the
feature/usb_otg
branch
from
c11da1c to
2b61eef
Compare
raffarost
force-pushed
the
feature/usb_otg
branch
from
2b61eef to
19a3b46
Compare
Update hal_espressif to add USB-OTG support. Signed-off-by: Raffael Rostagno <[email protected]>
Add USB device driver support for ESP32 devices with USB-OTG peripheral. Signed-off-by: Raffael Rostagno <[email protected]>
Due to smaller FIFO buffer, some devices might require limiting MPS (max packet size) to allow proper FIFO sizing at driver's init. We propose creating a new setting (MPS) for the device caps to allow the vendor to inform the driver about the maximum supported packet size. FIFO sizing formula is adjusted to match DWC2 programming guide recommendation: GRXFSIZ = 13 + 1 + 2 × (Largest-EPsize/4 + 1) + 2 × EPOUTnum If MPS is not set at dwc2_quirk_caps() the driver should behave as previous versions and won't affect other vendors. Signed-off-by: Raffael Rostagno <[email protected]>
Add USB-OTG peripheral support to ESP32S3. Signed-off-by: Raffael Rostagno <[email protected]>
Label zephyr_udc0 is necessary to run samples but it must remain disabled as USB/JTAG shares the same phy and pins and won't work for builds that don't use USB. Signed-off-by: Raffael Rostagno <[email protected]>
Add samples for HID-keyboard and mass storage. Signed-off-by: Raffael Rostagno <[email protected]>
raffarost
force-pushed
the
feature/usb_otg
branch
from
19a3b46 to
4323bc8
Compare
Add USB device driver support for ESP32 devices with USB-OTG peripheral.