usb: usdc: BL61x Device HS USB #97750
Conversation
VynDragon
force-pushed
the
bl61x_usb
branch
4 times, most recently
from
662579f to
f771808
Compare
josuah
left a comment
josuah
left a comment
There was a problem hiding this comment.
I wish to do another round of review when I get back home, also run USB3CV compliance tool which will be good at spotting bugs.
Though it sounds like ready for review to me.
Thank you!
|
Zephyr host not ready yet either... (>_<') |
|
Thanks for the review.
What would be the proper naming scheme? should I name this driver 'udc' instead of usb and the host one 'uhc'? |
|
I think you are right, to follow the UDC (USB Device Controller) acronym, |
VynDragon
force-pushed
the
bl61x_usb
branch
2 times, most recently
from
da3285c to
e7f5f3b
Compare
|
And one other caveat before this is ready, this will need to add a new board, because the current bl61x board ai-m62-12f has NO usb at all going out the module so it cant support USB. Liekly this will be sipeed m0s, but it could also be added to qcc744_evk if it's merged very fast |
|
Ah no wait it does have them... |
zephyrbot
added
area: RISCV
zephyrbot
requested review from
edersondisouza,
fkokosinski,
jfischer-no,
katsuster,
kgugala,
masz-nordic,
mgielda and
npitre
|
I think the next step is basically wait that the maintainers' review queue clears out. If it takes long and "ping" don't help, the workaround is |
i didnt know about that and manually git fetched and cherry picked PR |
|
ok did all the ones that worked i think and a few more |
drivers/usb/udc/udc_bflb_bl61x.c
Outdated
| #define USB_BL61X_CAP_ISO 1 | ||
| #define USB_BL61X_CAP_BULK 2 | ||
| #define USB_BL61X_CAP_INT 3 |
There was a problem hiding this comment.
What are these values? They are used by bitwise ORing the values, but USB_BL61X_CAP_ISO | USB_BL61X_CAP_BULK == USB_BL61X_CAP_INT. Is this intentional or oversight?
There was a problem hiding this comment.
It is intentional, but i suspect its just the type from https://github.com/torvalds/linux/blob/dd72c8fcf6d35de5d6d976f20dc1ae84ce7af08b/include/uapi/linux/usb/ch9.h#L441 so it might be wrong...
Additional 'documentation' since there is no documentation:
https://android.googlesource.com/kernel/msm/+/android-msm-swift-3.18-marshmallow-mr1-wear-release/drivers/usb/gadget/udc/fotg210-udc.c
https://github.com/cherry-embedded/CherryUSB/blob/master/port/bouffalolab/usb_dc_bl.c
There was a problem hiding this comment.
https://github.com/cherry-embedded/CherryUSB/blob/master/port/bouffalolab/usb_dc_bl.c
Do not use it for any references, or copy any code or logic from there. This project is absolutely unreliable regarding code source and licensing.
There was a problem hiding this comment.
It's the same code as from the bouffalolab SDK, the author of cherryUSB worked for bouffalolab for quite a while (like 4 years at least, until fairly recently).
There was a problem hiding this comment.
anyway, fixed that issue by removing that function entirely, it was usign the wrong thing anyway.
There was a problem hiding this comment.
In general it looks good, there are usual style and code structure issues that needs to be fixed, but...
Looking at the next commit I was curious about "It is similar to FOTG210". I started to look at the BL61X manual, and there does not seem to be one, the same appears to be for FOTG210 IP. I see some rumors that the BL61X SoC uses FOTG210 IP. Comparing fotg210-udc.[c,h] from the Linux kernel with your code and usb_v2_reg.h from the bouffalolab HAL, it seems to be FOTG210 USB controller IP. In that case, the right way is to implement platform/vendor independent driver for FOTG210 based controllers and use vendor specific quirks for platform specific parts like clock configuration or pin configuration.
Please use drivers/usb/common/usb_dwc2_hw.h, drivers/usb/udc/udc_dwc2.[c,h], and drivers/usb/udc/udc_dwc2_vendor_quirks.h as the reference.
Rename udc_bflb_bl61x.c to udc_fotg210.c and use udc_fotg210/fotg210 as namespace. Add common header file drivers/usb/common/usb_fotg210_hw.h. Use definition from usb_fotg210_hw.h in udc_fotg210.c. Move BL61X specific platform code to drivers/usb/udc/udc_fotg210_vendor_quirks.h.
The problem is where to get register names for drivers/usb/common/usb_fotg210_hw.h, ideally they should have similar names as in the official FOTG210 manual/databook. I have no idea where to get them. Faraday Technology seems to be the original author of the Linux driver but these files are GPL-2.0+ only and definitions there can not be copied. So this need to be solved first.
I am also curious where did you get all the info to write this driver?
There was a problem hiding this comment.
it is similar to fotg210, there are some significant differences on what interrupts work and dont work, and it uses an entirely different DMA system, if you look at the linux driver for fotg210 it has some significant differences mostly around the control logic due to what is and what isnt available (or doesnt work as it should, i tried to apply the advantages of the version in the linux kernel in many places) in the bflb version. So i dont think it's possible to make a generic fotg210 driver from this, as it's pretty much guaranteed to be inadapted for whatever else could use it.
I am also curious where did you get all the info to write this driver?
apache-2 bouffalo SDK, author of cherryUSB did work for them, so code is available both in cherryUSB and the bouffalosdk, and you can also observe the same code under proprietary license in the QCC744 SDK. The bouffalolab port also has a line to talk to bouffalolab but this driver predates it.
There was a problem hiding this comment.
it is similar to fotg210, there are some significant differences on what interrupts work and dont work, and it uses an entirely different DMA system, if you look at the linux driver for fotg210 it has some significant differences mostly around the control logic due to what is and what isnt available
That is not a reason to give up. There are more similarities than differences. Some parts can be handled by vendor quirks. VDMA in the driver with a comment until there will be another platform that could use this driver.
apache-2 bouffalo SDK, author of cherryUSB did work for them
That makes it unacceptable for me. You need to write the code from scratch.
There was a problem hiding this comment.
That makes it unacceptable for me. You need to write the code from scratch.
The reference code. I should have quoted 'I am also curious where did you get all the info to write this driver?' instead. Obviously i've written the entire thing submitted here... If you're going to complain about the register writes being the same, there isn't 5 different way to do the same operations with the same hardware.
That is not a reason to give up. There are more similarities than differences. Some parts can be handled by vendor quirks. VDMA in the driver with a comment until there will be another platform that could use this driver.
Yes it is until I get docs for the IP, and even then this is re-scoping the PR in a huge way.
VynDragon
force-pushed
the
bl61x_usb
branch
3 times, most recently
from
013b081 to
a84cca4
Compare
Adds a device mode driver for BL61x Signed-off-by: Camille BAUD <[email protected]>
Adds the binding to enable the USB controller Signed-off-by: Camille BAUD <[email protected]>
This is necessary to have a board testing the USB driver, however the board doesn't have a USB plug for it, but the pins are available Signed-off-by: Camille BAUD <[email protected]>
|
|
Converting back to draft because some major bugs have been found and we're struggling to fix them (what with not having docs...). |
5 participants
It does device at high-speed speeds.
Caveat: controller does host, this does not do host.