esp32s3 USB0 register block issues

[simpkins]

I'm playing around with possibly implementing an embassy-usb driver for esp32s3, and while doing so I noticed a few things that look like typos or transcription errors in the esp32s3 SVD file (an also in the esp32s2 one too).

• The EPDISn field in the USB0.DIEPCTLn registers aren't named consistently: endpoints 0,1,2,4, and 6 call this D_EPDISN while endpoints 3 and 5 call this DI_EPDISn. (using a DI_ prefix instead of D_)

• The EPDISn fields in the USB0.DOEPCTLn registers appear to be listed as read-only. This seems likely wrong? The tinyusb esp code does set these fields, and the Synopsys USB docs provided for STMicro chips does document that these fields need to be set to disable OUT endpoints. (As far as I could tell Espressif only provides minimal high level documentation for their USB cores, so I'm going off STMicro docs).

• It's unfortunate that the USB0 FIFO ranges aren't listed in the SVD. It looks like the existing blocking (non-embassy) USB drivers end up accessing the FIFO by manually computing the address from the start of the USB0 register block. (reg_block + 0x1000 + fifo_num * 0x1000)

As an aside, the fact that all of the endpoint registers (DIEPCTLn, DOEPCTLn, DIEPINTn, DOEPINTn, etc) have different types for each endpoint makes the code much more awkward to use. The endpoint 0 registers are slightly different from the others, but it would be nice if endpoints 1 through 5 all used the same types.

Also, very minor, but I wonder if the USB0.GINTSTS.RXFLVI name was a typo in the SVD and should have been RXFLVL instead?

[simpkins]

Also, the MPS field looks incorrect in USB0.DIEPCTLn. I believe this should be bits 0:2 in diepctl0, but bits 0:10 in diepctl1 through diepctl6. However, it is listed as just 2 bits wide for all endpoints.

The DOEPCTLn registers to appear to be correct.

[simpkins]

I guess there are two separate aspects to this issue:

• obvious bugs that could be fixed with minimal changes (e.g., incorrect field bit widths, fields not being marked read-write)
• re-organization for easier use in Rust (e.g., changing all the endpoint registers to use the same type, and renaming their fields to be consistent, possibly turning them into an array)

I'm not sure how much appetite there is to do the larger cleanup. From a cursory search I didn't really see any downstream crates that seem to be using the USB0 register block much, so it possibly wouldn't disrupt too many things downstream. That said, I'm not sure how much you want to avoid deviating from the base SVD.

If we did clean things up I would suggest:

• Changing the DIEPTX1-4 registers to all use the same type, and turning them into an array
• Changing all of the device endpoint registers (DIEPCTLn, DIEPINTn, etc) to use the same type for endpoints 1-6. It would be nice group the endpoint registers into a sub-structure, and then make this into an array, as is done for the in_ep_reg and out_ep_reg fields of usb_dev_t in the soc/usb_struct.h files in the ESP-IDF.
• I haven't used the host-side registers as much, bit it looks like there are a number of host-side registers that could use the same treatment (e.g., HCINT0-7, HCTSIZ0-7, HCDMA0-7, ...)

[MabezDev]

Thanks for the issue @simpkins! We very much have an appetite to do the cleanup, particularly in this case where there isn't a driver to update. It's much nicer write drivers when the PAC is in a good spot.

We really appreciate the PR, and any subsequent PRs you may file (the clean up suggestions look good to me!). Most of the Rust team will be on vacation till next week, and we'll get to reviewing your PR then :).

[MabezDev]

Oh and seeing as you're interested in async USB in the end, cc: @dominazzz and embassy-rs/embassy#2494.

I also have a personal use for async USB coming up, so I'd be more than happy to help the both of you work through the issues :).

[simpkins]

Thanks for the feedback. I've got some code that is close to working (I think) for using embassy-usb on esp32s3; I'll try to make some more progress on it in the next few days and then upload it to github. It isn't really based on the embassy-stm32 implementation--I looked at how they did some stuff, but it is largely from scratch with a few different design choices. (I also looked at how tinyusb does things, plus the STMicro documentation, given that Espressif docs cite an NDA and don't include USB register documentation.)

I'll try to make a few more pull requests with SVD patches when I have made more progress.

Having a combined embassy-usb-driver implementation for both STM32 and ESP32 chips would be nice in the long run. That said, I think it would take quite a bit of refactoring to make this work with esp-pacs--the USB0 API exposed by esp-pacs is rather different than the one used by the embassy-stm32 ral, even though the register block layout is nearly the same.

As an aside, I did notice one more SVD bug: the PKTCNT fields in DOEPTSIZn is listed as just 1 bit, while I believe it should be 10 bits in endpoints 1-6. (At least, it's 10 bits in the STMicro docs.)

[MabezDev]

Having a combined embassy-usb-driver implementation for both STM32 and ESP32 chips would be nice in the long run. That said, I think it would take quite a bit of refactoring to make this work with esp-pacs--the USB0 API exposed by esp-pacs is rather different than the one used by the embassy-stm32 ral, even though the register block layout is nearly the same.

I agree, having completely different peripheral access mechanisms makes this probably more effort than it's currently worth. Let's just focus on getting the pac in good shape for your upcoming driver, merging the two can be a longer term stretch goal if we wish.

[MabezDev]

Closed via #213