[PORT] Add GD32VF103 support and Sipeed Longan Nano Board support #959
Conversation
KarlK90
force-pushed
the
gd32vf103-support-tiny-usb
branch
2 times, most recently
from
500faeb
to
b33ebd2
Compare
|
sorry for late review, I have been late with other works. will try this out asap. |
|
No Problem and thank you for helping with the debugging! 🙂 I hope you and your families are safe and healthy. |
There was a problem hiding this comment.
Sorry for keeping you waiting, I have been overwhelmed with other paid work and non-work (real life) issue that we are all facing. Anyway, I finally manged to review this PR, it is much easier than I thought, you are doing a really great work !! Though there are only couple of minor feedbacks
- nuclei-sdk is overkill, please use the simpler firmware library from gigadevice for low level mcu. Since the stack doesn't need anything else
- please add the ST license to the
synopsys_common.h
PS: You probably need to rebase/merge from master to resolve the conflict, there are a couple of PR merged previously.
| [submodule "hw/mcu/gd/nuclei-sdk"] | ||
| path = hw/mcu/gd/nuclei-sdk | ||
| url = https://github.com/Nuclei-Software/nuclei-sdk.git |
There was a problem hiding this comment.
nuclei-sdk is too overkill as submodule for just low level mcu driver. Furthermore, TinyUSB shouldn't reply on other project to provide hal layer. It is best to just use the mcu driver from the vendor, I think https://github.com/GigaDevice-Semiconductor/GD32VF103_Firmware_Library would be a much better sub-module, since we only need a basic one.
There was a problem hiding this comment.
I completely agree that the nuclei-sdk is overkill for this port, but I had chosen it over the multiple other repositories of the GD32VF firmware on github because their repository is the most up to date one and actively maintained by nucleisys which provided the RV32 core IP. Furthermore there is a bug in the GD32VF startup code in all libaries that throws an compilation error if -Wundef and -Werror is used with GCC, for which I had opened a PR. Therefore I would vote to keep this despite being more then necessary, what do you think?
The other repositories:
https://github.com/riscv-mcu/GD32VF103_Firmware_Library (Semi-official, contains a note to use nuclei-sdk, last activity in October 2020)
https://github.com/GigaDevice-Semiconductor/GD32VF103_Firmware_Library (Is this official?, last real activity in November 2020)
There was a problem hiding this comment.
I pull the nuclei and did a quick check, the https://github.com/GigaDevice-Semiconductor/GD32VF103_Firmware_Library is more updated in term of the mcu driver. The last activities you said in 2020 is update the mcu lib to v1.1. while nuclei still uses the v1.0 version. nuclei is more maintained by it is mostly for their platform, tinyusb is only interested in low level mcu driver (clock, register def). Therefore I think it make more sense to move to the said repo.
There was a problem hiding this comment.
I'm a bit confused the latest firmware library from GigaDevice is 1.1.2 from June 2021 but can only be found on http://www.gd32mcu.com/en/download/0?kw=GD32VF1 so all the repos are outdated. Judging from the activity in repositories all but nuclei-sdk seem to be unmaintained, as no pull request are merged or answered.
I hope that I don't bother you with this discussion, but I think that nuclei-sdk is the better choice from a maintenance perspective and I would happily open a PR to update the supplied firmware in nuclei-sdk. If that is not an option at all, I'll switch to the suggested https://github.com/GigaDevice-Semiconductor/GD32VF103_Firmware_Library repository.
There was a problem hiding this comment.
I am not bothered at all, I have looked a bit more closer. To be honest, at first I thought Nuclei is a 3rd party, but look like they are IP creator similar to ARM. And Giga put Nuclei's N200 risc-v core into their silicon. Furthermore, Nuclei seems to spend much more effort on the software/firmware side than Giga does. And I could see why you prefer to go with their sdk. Feel free to correct me if I get it wrong.
https://github.com/Nuclei-Software/NMSIS is indeed a really great repo which is set to do what CMSIS does for ARM. I think we could actually depend on this nmsis. However, I still don't want to force any other users to use nuclei sdk just to run the usb stack. They may already be on another platform/framework, that could cause an conflict. Unless there is something that we would really need from sdk that we couldn't write it ourself.
To sum up, do you think it is good enough to just add 2 submodules
- semi official https://github.com/riscv-mcu/GD32VF103_Firmware_Library for low level mcu, I don't mind that it is old v1.0, mcu driver doesn't change much. And USB does not use much of them.
- https://github.com/Nuclei-Software/NMSIS for providing abstraction for risc-v core for eclic enabled if needed.
This is actually very similar to stm32 with an cmisis and driver combo, https://github.com/hathach/tinyusb/blob/master/hw/bsp/stm32f103bluepill/board.mk#L2
Note: when integrating tinyusb to an application, you can freely use nuclei, and have your own board setup, just changes the include path to the corresponding. The idea is if we use an smaller set of files (mcu lib), it would be more portable.
There was a problem hiding this comment.
I am not bothered at all, I have looked a bit more closer.
That is good to hear!
You are totally right about Nuclei, they provided the N200/BumbleBee Core for the GD32VF103 to GigaDevice and kind of co-developed the firmware library. I'm not sure why GigaDevice doesn't show a bit more effort with their firmware, it is a bit frustrating.
I still don't want to force any other users to use nuclei sdk just to run the usb stack. They may already be on another platform/framework, that could cause an conflict. Unless there is something that we would really need from sdk that we couldn't write it ourself.
I'm totally with you that we don't want conflicts in other projects. I think you are referring to namespace conflicts, like multiple definitions of the same symbol, but correct me here? My point is that the parts that we include and actively use in the compilation of tinyusb from nuclei-sdk is really just the NMSIS library and the gd32vf103 firmware library with modifications from nuclei to play along nicely with NMSIS (see below). The other header and source files related to different socs are not included in the build, it is just the bare minimum. So the conflicts should not increase by using nuclei-sdk and should be the same as using the separate NMSIS and GD32VF103 firmware repositories.
On the other hand I had started to change the current port to use the two independent repositories as suggested and ran into trouble. The NMSIS library wants specific defines that describe the capabilities of the GD32VF103. Unfortunately we can't just use this file because the GD32VF103 doesn't have a FPU and has other IRQs defined. So we would have to provide this block ourselfs in a separate header file before including nmsis_core.h in the dcd driver.
But by doing so we have a board specific header-file in a rather generic driver, which is not good. The nuclei-sdk provides these defines in gd32vf103.h which is different from the gd32vf103.h that is supplied with the firmware library. And they actually modified the firmware library in other parts to play a long nicely with NMSIS framework. The GD32VF103 firmware library is really just meant to be used on its own. There maybe even more conflicts that but I haven't explored that way further.
Given the above reasons and addressing your concerns about the conflicts could a dependency on just nuclei-sdk be the better option? What do you think?
There was a problem hiding this comment.
That is good to hear!
You know this much better than me, there is obvious reason why you prefer the nuclei sdk
You are totally right about Nuclei, they provided the N200/BumbleBee Core for the GD32VF103 to GigaDevice and kind of co-developed the firmware library. I'm not sure why GigaDevice doesn't show a bit more effort with their firmware, it is a bit frustrating.
Yeah, I figured it out, Giga doesn't spend much effort like ST do with their stm32
But by doing so we have a board specific header-file in a rather generic driver, which is not good. The
nuclei-sdkprovides these defines in gd32vf103.h which is different from the gd32vf103.h that is supplied with the firmware library. And they actually modified the firmware library in other parts to play a long nicely with NMSIS framework. The GD32VF103 firmware library is really just meant to be used on its own. There maybe even more conflicts that but I haven't explored that way further.Given the above reasons and addressing your concerns about the conflicts could a dependency on just
nuclei-sdkbe the better option? What do you think?
My ultimate goal is not having usb stack to depend on nuclei-sdk to run. User should just compile it with any GD32VF103_Firmware_Library variant, especially one downloaded from the Giga site since that is what user will do. With that in mind, in the fact the nuclei has to modify the library to get it work with nmsis, I think we actually have to change the approach a bit.
- We can keep using nuclei-sdk, and use it for running example only ie. bsp folder.
- However we shouldn't use any of nuclei-sdk and/or nmsis within the
srcfolder, i.e thedcd_synopsys.c. The reason is it will force user to use the modified firmware lib from nuclei and not be able to use official lib downloaded from Giga website. Even it is not well maintained, it is still what most user would use (unfortunately).
There was a problem hiding this comment.
That sounds good to me! I'll change the implementation to meet these constraints, maybe the core code in src can even be implemented without any reliance on any gd32vf103 firmawre library or nmsis/nuclei-sdk.
There was a problem hiding this comment.
Thanks for patiently following my suggestion, yeah, the example bsp is mostly for demonstrating/testing and maintaining. If nuclei-sdk make it easier for us, no problem at all, I wasn't sure before and kind of worry it will become an dependency due to the nmsis. If we could get it dcd_synopsys neutral then that would be no problem.
There was a problem hiding this comment.
Perfect! I have cleaned up and fixed a bug with the systick timer not beeing reloaded. I decided to go with nuclei-sdk inside bsp for obvious reasons ;-). Let me know what you think!
Thank you, same to you. Sorry for late response again. |
Without having __riscv_flen defined we get multiple warinings. But defining it causes the startup code to contain floating point instructions. This results in a exception right after booting. See startup_gd32vf103.S lines 289-294 should open a PR at nuclei sdk
This reverts commit 5e0c2e1.
KarlK90
force-pushed
the
gd32vf103-support-tiny-usb
branch
from
b33ebd2
to
3eb54d8
Compare
|
I have rebased on the latest master and pushed my changes! Thanks for your review and no need to apologize, I hope everything is well 🙂 If you find the time, then I'm curios what you find out about the strange errors I had with the demos and tinyuf2 bootloader. |
KarlK90
changed the title
| #elif CFG_TUSB_MCU == OPT_MCU_GD32VF103 | ||
| #define STM32F1_SYNOPSYS | ||
| #include "gd32vf103.h" | ||
| #include "nmsis_core.h" |
There was a problem hiding this comment.
I am not familiar with riscv and I am not sure if nuclei nmsis is as universal used as arm cmsis. If we didn't use it much e.g only enable/disable IRQ, maybe it is best to just use the low level code from giga firmware. That will allow user to run tinyusb with bare metal without the need for nuclei sdk.
There was a problem hiding this comment.
The whole risc-v ecosystem is really young and because of the decentralized and open nature of the ISA I doubt that something universal as CMSIS is going to emerge. So NMSIS is largely Nuclei specific as the processor core have vendor specific extensions but very well designed in my opinion. It also provides a CMSIS compatibility layer. Although for this port very few parts are actually used.
There was a problem hiding this comment.
looking more at nmsis, I think it is a great repo. I will leave to you to decide whether we should use nmsis here. Note that, anything used in this dcd file will be considered as tinyusb dependency.
There was a problem hiding this comment.
That sounds good! See my other comment for more rationale.
There was a problem hiding this comment.
I actually take this back, given the fact that nmsis requires firmware lib modification and couldn't work with official lib downloaded from Giga. Since it is simple enough, probably only ECLIC_EnableIRQ/DisableIRQ(), do you think we could do it with register level, I actually have to go this low for some of mcu ports to stay independent enough.
Sum up: we shouldn't use function that require firmware modification to work, even though it is good, it is still not officially supported by Giga. We may change this in the future though.
There was a problem hiding this comment.
This is absolutely possible! Enabling and Disabeling the IRQs is a simple setting and clearing the MIE bit in the mstatus register. I'll figure out if we go with the gd32vf103 firmware library provided functions or some inline asm.
There was a problem hiding this comment.
Oh except for actually enabling and disabling a specific IRQ, but this is rather easy to do.
There was a problem hiding this comment.
Thank you, we only need to specifically enable/disable the USB IRQ only, and don't need a generic function like NVIC. And bit set/clear with correct mask and register would do the trick hopefully
There was a problem hiding this comment.
Please have a look at c6d495d this is the commit that removes any external dependency for the core code. Are you fine with helper functions being directly in the driver code?
I will look at that, though I think firstly we need to make sure all stock examples in this repo run well with vf103 first. Since it is new port, it may has bug/issue. Therefore it is best to test against known-good example first. Once it is confirmed working well with stock example, we will move on with external one such as tinyuf2. |
|
Thank you and I totally agree with the sentiment. Maybe this PR would have been the better place to add errors I already encountered with the stock examples: |
The core of tinyusb must be as independent as possible, we previously relied on nuclei-sdk or the GD32VF103 firmware library for the synopsys driver to work with the GD32VF103. Fortunatly we needed very few parts from them so we implement them here.
This controller family only supports USB FS with four endpoints
We use the linker files provided by nuclei-sdk instead
Instead of using the HAL functions we can just use the defines from the board support for the longan nano that comes with the nuclei-sdk. Also we move some includes and defines to the header file.
Only 48MHz, 72MHz, 96MHz and 120 MHz system clocks derived from an external crystal are suitable for the usb peripheral, as the internal oscillator is not stable enough. Also the usb-prescaler only supports division by 1 (48MHZ), 1.5(72MHz), 2(96MHz) and 2.5(120Mhz). 120Mhz is also out of spec and not added here.
Forgot to reload the systick timer in the irq handler
The GD32VF103 family only has USB-OTG peripherals.
As the peripheral is the same as on the STM32F1 and STM32F4 lines we do the same.
|
@KarlK90 everything look great with latest commit, however, when I tried to flash your pr to my longan nano board, it does not seems to run at all. Maybe I haven't done it correctly,
However with both
|
Great that my changes look good. I think the flashing problem is connected to the buggy GigaDevice DFU bootloader that reports a transfer size of 2048 bytes, PS: that buggy bootloader set me on the adventure to add tinyusb and tinyuf2 support 😉 |
|
Ah you are right, upgrading to dfu-util 0.10 and it successfully flash the device (even tried to force the upload-size with 1024 on 0.9 just to test, but it doesn't help). Thanks for suggestion, I will wire up to use it with jlink for faster developement. The cdc_msc example have the cdc echo fine, however, the msc does not, I hook it into analyzer. Look like there is some issue when host read 4k of data. Notice that the data is corrupted with 0x55AA repeatedly somehow. Host give up and decide device isn't properly formatted. It is probably buffer/endpoints config/overflow issue etc .. which is typical for a new port based on existing dcd driver. I will spend sometime this week (or so) to poke at GDVF103 manual to see if I could spot any issue. Hopefully it would be only an minor change. PS: It is not important but the blink timer is a bit slow, it should toggle 1000ms when connected, 250ms when not. We could look into this later on just to make it consistent with other ports.
|
The systick timer is driven by the AHB bus divided by 4, set the correct reload value to generate a timer irq every ms.
|
Good to hear that the upgrade solved the problem with the flashing. You are right about the timer, I set the wrong reload value. The systick timer is driven by the AHB clock divided by 4. On the current implementation the AHB clock should equal the system clock (72Mhz by default). I quickly pushed a fix, hopefully that corrects the blinking frequency. Thank you for the debugging effort, you clearly have tools and knowledge that I lack 💪. The tinyusb HID examples worked fine from what I can remember. I had ported the stm32/synopsis otg driver in chibios to the GD32VF103 (just renaming registers) but it worked fine with HID device applications as well. I don't know if this is of any help but here it is anyways. |
|
yeah, both HID and CDC with example config only send up to 64 bytes per fifo, this issue probably only happens when doing large buffer transferring back to back, may involve race condition. I will try to troubleshoot it and post more update when available. |
There was a problem hiding this comment.
@KarlK90 Thank you very much for the PR and your patient. I have spent an hour or so trying to see if I could spot any obvious issue. Unfortunately, I couldn't, it may need a overall revise of the dcd. which I don't have the bandwidth to do at the moment. It is relatively normal for an new port, I have to do something similar with esp32 (also using synopsys IP). Therefore I think it is best to merge this to master and then file an issue so that other people can get on helping to resolve this. I also made a couple of changes to this PR mostly to get passed CI build
- Move the bsp to its own familay, also rename board to
sipeed_longan_nano, that would help to add more boards with similar mcu. - Change default CROSS_COMPILE to
riscv-none-embed-installed with xpack, mainly is for CI, since that is toolchain used by fomu (another existing risc-v port). there is also comment for nuclei and embecosm as well in the family.mk if user want to change.
PS: #1022 is filed for the msc issue.
|
@hathach Thank you for you continued effort and merging this PR, the changes look good! Hopefully the bug with the dcd and this particular peripheral is not to complicated so it can be resolved in the future. I'll try to poke at it a bit, maybe I can find something. |
|
I am the one to say thanks, will definitely try to solve it in the future. |
Describe the PR
This is a rough first draft to support the GD32VF103 RISC-V MCU by GigaDevice. It uses the same Synopsys USB OTG full-speed peripheral that is found on the STMF105 and STM32F107. I have added the nuclei-sdk that adds support for the GD32VF103. I chose to extract register definitions for the OTG peripheral from the stm32f105.h headers and put them into synopsys_common.h in order to not rename all registers in the sources, which is hacky and another solutions should probably be found. So this needs clean up, atm this PR is meant to help debugging with adafruit/tinyuf2#131.
Additional files for development:
Toolchain
I can highly recommend the pre-built toolchains from embecosm.
Datasheets and SVD files
GD32VF103 Datasheet
GD32VF103 User Manual
GD32VF103 SVD (remove .txt)
These files except the SVD were downloaded from GigaDevice. GigaDevice has given all registers different names for the USB peripheral but it is the same as found on the others mcus. So an SVD file for the STM32F105/STM32F107 should give the familiar STM32 names. Seggers J-Link and RTT support the GD32VF103 out of the box.
The Longan Nano has a broken out JTAG Debug Header, I have enabled the USART peripheral on this header as well it is the T0 and R0 pins.