hathach
Renesas ra family support
Describe the PR
This PR generalizes the previously named renesas usba driver that supported the renesas RX family of MCUs into the link driver, named after the name of the usb core according to renesas (LINK core), it also adds support for the cortex m family RA.
Along comes the support for one of the evaluation boards ek-ra4m3
The is also a small change to the way the freertos port is handled in the build system, as it was necessary to use the port provided by renesas
Additional context Most device examples were tested successfully on the ek-ra4m3, no host was tested other than compilation. No tests were done on the RX family, regression testing is necessary but I do not have access to boards of that family, and have no easy way of installing the toolchain.
Will have access to a ek-ra4m2 and ek-ra6m2 soon, but no tests were done so far. I expect all RA devices to work, but I have to note that two of the mcus in this family support USB HS on this same core (RA6M3, RA6M5), I have not done any work on that though.
Not sure what is wrong with the Rx build, I would appreciate if someone pick this up for me
Why do you made the the missing register definition so complicated ?
The following definition would do it for your case without doing changes umpteen times across the whole usb driver file:
#if CFG_TUSB_MCU == OPT_MCU_RAXXX
#include "bsp_api.h"
#include "renesas.h"
#define USB0 R_USB_FS0_BASE
#else
#include "iodefine.h"
#endif
The following definition would do it for your case without doing changes umpteen times across the whole usb driver file
If you looked closer you would see that it's not quite as simple as that, the register definitions are not compatible, that's why I created a local register definition.
I didn't complicate anything, the fundamental driver did not change, only the registers.
the interrups are also handled different in those families, thats why that was abstracted, I tried to a similar approach to the dwc2 driver to keep things consistent, it could have been kept all in the driver files if we wanted
@perigoso
Thank you for your great work!
If possible, could you please keep the indentation of link_dcd.c and link_hcd.c? I would like to see differences other than indentation and symbol names, but it is difficult, I seem to miss them. I don't mind changing the indentation, but would you separate the pull requests.
Hm, that might be hard the way i did the commits, i kind of messed up, if you look at the rename commit for instance you'll see what i mean
I'll try to redo this more cleanly when i find some time
@perigoso
I see. Could you share the settings file for clang-format? I will temporarily apply it to master branch on my local environment, and try to generate diff file except indentation changes.
rebased everything, now there should be no style changes.
It was quite a bit of work to redo everything properly but it was worth it
hopefully nothing broke, but I tested cdc_msc and cdc_dual_ports and they behaved as expected so I think I'm good.
@perigoso
Thank you very much for your work! It is very helpful for me to review changes. I will test this PR on my boards.
@kkitayam did you ever get to test this? not in a rush or trying to pressure, just eager
ah, that fixed the rx build, wonderful! thank you hoping that means everything still works in hardware
I have tested following device examples on RX65N target board with WIndows 10.
| example | result | note |
|---|---|---|
| cdc_msc | ✔️ | |
| cdc_dual_ports | ✔️ | |
| cdc_msc_freertos | ❌ | Enumeration failed |
| hid_composite | ❌ | Enumeration OK, but the behavior is not correct. |
| msc_dual_lun | ✔️ | |
| net_lwip_webserver | ❌ | Enumeration probably OK, but the behavior is not correct. |
| uac2_headset | ❌ | Enumeration OK, but no audio data recorded |
| video_capture | ❌ | Enumeration OK, but no video data recorded |
| webusb_serial | ✔️ |
It seems that bulk transfers work fine but Isochroous and interrupt transfers. I'll see if there are any problems.
@perigoso
Thanks for updating! I have confirmed that these examples work fine except cdc_msc_freertos.
Also, I confirmed that cdc_msc_freertos didn't work at the HEAD of the master branch either. So, this issue is not caused by this PR.
The host driver will test at a later date.
I confirmed that cdc_msc_hid works with a low-speed mouse and a super-speed USB HUB.
@Wini-Buh
If you don't mind, could you comment in terms of big-endian or CCRX?
@kkitayam
I had a look into the code and did a quick test with the CCRX toolchain in big and little endian mode. After doing the quick hacks on the code (add the correct #pragma directive and add __evenaccess keyword, see the description below), the CDC test was successful.
Unfortunately the code as it is, is not endian independent. For the CCRX we could solve this with a #pragma directive. But to be more independent of the toolchain (and invest into the future), we could use solution like it is found in the header file iodefine.h, which is provided by Renesas if using the GCC toolchain (see the example code fragment below). This way it should be possible to do get along without the #pragma directive.
typedef struct st_usb0 {
union {
unsigned short WORD;
struct {
#ifdef __RX_LITTLE_ENDIAN__
unsigned short USBE : 1;
unsigned short : 3;
unsigned short DPRPU : 1;
unsigned short DRPD : 1;
unsigned short DCFM : 1;
unsigned short : 3;
unsigned short SCKE : 1;
unsigned short : 5;
#else
unsigned short : 5;
unsigned short SCKE : 1;
unsigned short : 3;
unsigned short DCFM : 1;
unsigned short DRPD : 1;
unsigned short DPRPU : 1;
unsigned short : 3;
unsigned short USBE : 1;
#endif
} BIT;
} SYSCFG;
Another small problem is the definition of the LINK_REG. To make sure the CCRX accesses the defined registers correctly, the keyword __evenaccess is required (this extension guarantees access in the size of the target variable).
#if defined(__CCRX__)
#define LINK_REG ((LINK_REG_t __evenaccess*)LINK_REG_BASE)
#else
#define LINK_REG ((LINK_REG_t*)LINK_REG_BASE)
#endif
I hope this info is useful for you.
@Wini-Buh Thank you very much for testing and useful comments! I understood we need more modifications to work with gcc for big-endian.
My understanding for the current implementation (dcd_usba) of master branch, it supports big-endian with only CCRX. Because of using TU_ATTR_BIT_FIELD_ORDER_BEGIN for endianness. It is a #pragma. Is this correct?
I think it is better to keep the features that the current implementation of master branch has. So, we should keep support big-endian with CCRX. Of course, I think it is more better to support big-endian with gcc. But, I don't mind big-endian with gcc would be supported in another PR and not in this PR.
After doing the quick hacks on the code (add the correct #pragma directive and add __evenaccess keyword, see the description below), the CDC test was successful.
I understood __evenaccess keyword needs to src/portable/renesas/link/link_rx.h. But, It seems that #pragma is already added src/portable/renesas/link/link_type.h. It has TU_ATTR_BIT_FIELD_ORDER_BEGIN directive. If you don't mind, would you give me more information regarding correct #pragma?
@kkitayam
My understanding for the current implementation (
dcd_usba) of master branch, it supports big-endian with only CCRX. Because of using TU_ATTR_BIT_FIELD_ORDER_BEGIN for endianness. It is a#pragma. Is this correct?
Yes
I think it is better to keep the features that the current implementation of master branch has. So, we should keep support big-endian with CCRX. Of course, I think it is more better to support big-endian with gcc. But, I don't mind big-endian with gcc would be supported in another PR and not in this PR.
OK
I understood
__evenaccesskeyword needs to src/portable/renesas/link/link_rx.h. But, It seems that#pragmais already added src/portable/renesas/link/link_type.h. It has TU_ATTR_BIT_FIELD_ORDER_BEGIN directive. If you don't mind, would you give me more information regarding correct#pragma?
First of all, I did the test with an old CCRX toolchain version which I normally not use anymore, sorry for that.
So the #pragma is already correctly set. The correct #pragma is _Pragma("bit_order right") (that's already correct defined in the file tusb_compiler.h). So everything is fine here. Only the __evenaccess keyword needs to be added. With that everything is working fine.
Sorry again for the mistake with the old toolchain.
If i understood correctly commit d55743b2af30d780b4bf2b93cc5ee152f0d2a74d is all that was needed?
if so this PR is ready, I would just like the good to go from @hathach (I understand you are busy right now, don't worry)
Sorry for late response. I am busy for family reasons.
@Wini-Buh Thank you for comments. I understood we need __evenaccess keyword.
@perigoso Thank you for updating this PR to add the keyword. I think this PR is ready for merge to master branch!
@hathach do you think you could review this? It should be relatively clean, I just don't want to have this become stale and then have to rework it again
@perigoso I am so sorry for the massive delay, Unfortunately, this matter slipped off my radar and the notification got lost amidst others. I admit that my lack of familiarity with Renesas MCUs make me not able to follow up on this matter more closely.
I am gonna try to sync this with master, review and pull out my boards to test, hopefully we could merge this soon as it should be.
Sorry again
Hi @hathach , unfortunately I no longer have access to the renesas development boards (job change), but I'm happy to see this being picked up
hopefully it's not too much work, the more conflicting change might be the RTOS changes
Hi @hathach , unfortunately I no longer have access to the renesas development boards (job change), but I'm happy to see this being picked up
hopefully it's not too much work, the more conflicting change might be the RTOS changes
Thank you, RTOS change is minor comparing to the whole PR. It is totally my fault, I am trying to fix/merge this as soon as I could :)
PS sorry to hear that you have to change your job if that is unexpected.
@perigoso I have made some minor commits and I think we are really close to merge this. I only have an question regarding the IRQn number used by RA chip as mentioned above. That is ok if you don't remember that, we can merge it as it is for now and worry about that later on.
Another thing is the name of USB IP, I did a quick search on renesas page. As far as I can tell, Link is not the name of the controller, it seems to be used in place for connectivity, there is also sata, pcie link. If you don't mind I would suggest to rename the usbip back to simply renesas ? ofc, I will do that myself.
