Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Renesas ra family support #1391

Merged
merged 32 commits into from
Mar 17, 2023
Merged

Renesas ra family support #1391

merged 32 commits into from
Mar 17, 2023

Conversation

perigoso
Copy link
Collaborator

@perigoso perigoso commented Mar 9, 2022

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.

@perigoso perigoso requested a review from hathach March 9, 2022 13:12
@perigoso perigoso added the Porting Adding new mcu/board/os support label Mar 9, 2022
@perigoso perigoso requested a review from kkitayam March 9, 2022 14:13
@perigoso perigoso force-pushed the renesas-ra branch 2 times, most recently from 3d5b8be to 8948cf5 Compare March 9, 2022 16:28
@perigoso
Copy link
Collaborator Author

perigoso commented Mar 9, 2022

Not sure what is wrong with the Rx build, I would appreciate if someone pick this up for me

@perigoso perigoso force-pushed the renesas-ra branch 2 times, most recently from bce460a to c826f0b Compare March 9, 2022 18:17
@Wini-Buh
Copy link
Contributor

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

@perigoso
Copy link
Collaborator Author

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.

@perigoso
Copy link
Collaborator Author

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

@kkitayam
Copy link
Collaborator

@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.

@perigoso
Copy link
Collaborator Author

perigoso commented Mar 21, 2022

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

@kkitayam
Copy link
Collaborator

kkitayam commented Mar 21, 2022

@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.

@perigoso
Copy link
Collaborator Author

@perigoso perigoso force-pushed the renesas-ra branch 2 times, most recently from 6584523 to fc5cdbc Compare March 21, 2022 15:32
@perigoso
Copy link
Collaborator Author

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

@perigoso
Copy link
Collaborator Author

hopefully nothing broke, but I tested cdc_msc and cdc_dual_ports and they behaved as expected so I think I'm good.

@perigoso perigoso requested a review from kkitayam March 21, 2022 16:04
@kkitayam
Copy link
Collaborator

@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.

@perigoso
Copy link
Collaborator Author

@kkitayam did you ever get to test this? not in a rush or trying to pressure, just eager

@kkitayam
Copy link
Collaborator

Sorry for the late reply. Not yet tested.

@perigoso
Copy link
Collaborator Author

ah, that fixed the rx build, wonderful! thank you
hoping that means everything still works in hardware

@kkitayam
Copy link
Collaborator

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
Copy link
Collaborator Author

perigoso commented Mar 8, 2023

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

@@ -156,28 +106,6 @@ typedef struct
//--------------------------------------------------------------------+
static hcd_data_t _hcd;

static uint32_t disable_interrupt(void)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you still remember why this disable_interrupt()/enable_interrupt() is dropped ? Or is it not needed in the first place ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has moved to the link_rx.h file, as it is specific to the RX family

@hathach
Copy link
Owner

hathach commented Mar 8, 2023

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.

static inline void link_int_enable(uint8_t rhport)
{
(void) rhport;
NVIC_EnableIRQ(TU_IRQn);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

everything look great, though I don't understand where is TU_IRQn is used, I couldn't find where it got used to setup USB interrupt, maybe I miss something.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see these lines
These mcus have a dynamic linking between peripheral interrupts and the vector table, their IDE generates these files, this is derived from the generated files so there is no proper define "linking" them together

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, I see thank you, it is a dynamic IRQ binding. I may try to change it to CFG_TUD_*

@hathach
Copy link
Owner

hathach commented Mar 12, 2023

@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.

Screenshot from 2023-03-12 16-30-16

@perigoso
Copy link
Collaborator Author

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.

I don't remember where I got the IP name, I might have misread something, renaming things back is fine by me

@hathach
Copy link
Owner

hathach commented Mar 16, 2023

I don't remember where I got the IP name, I might have misread something, renaming things back is fine by me

Thanks, I will do the rename, maybe calling it rusb2, most of renesas mcu use this IP I think.

Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superb! Thank you @perigoso very much for generalizing the renesas driver and sorry for the huge delay. I was too busy last time (around last Lunar New Year), and lots of stuffs happening making this fall off my radar. Though that is still hugely my bad.

I made some changes but should not cause any issue since I tested with both RA and RX boards

  • Add ra4m1_ek board, which I have. I also have an ra6m1_ek but that can wait for next PR. Note bsp for RA can be refactor to reduce code a bit but can wait for next PR as well
  • changes supported in readme to only ra4m1 and ra4m3 which are tested. I think we should only state supported MCU when it is tested.
  • rename usb-ip from link to rusb2 (they seem to also have usb 3 supported but only on CPU)
  • rename to freertos portable to make it clear that path is to portable source

@hathach hathach merged commit 6b84a29 into hathach:master Mar 17, 2023
@perigoso perigoso deleted the renesas-ra branch March 17, 2023 11:39
@perigoso
Copy link
Collaborator Author

@hathach No worries, thank you for picking this up, very happy to see this merged :)

you might want to rename the LINK_REG macro too, on the implementation files, since that was based on the Link name for the core, and now may cause confusion for readers without context

I will take this opportunity to point you at another one of my PR that you might have also missed, if you have a little time to help with that #892

@hathach
Copy link
Owner

hathach commented Mar 18, 2023

@perigoso thank you very much, I have already rename everything from LINK_REG to RUSB2_REG, let me know if you spot any leftover. I have another ra6m1 board, but running out of time. Will come back to this port later on.

PS: will also review the other SAM3 pr as soon as I could.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Porting Adding new mcu/board/os support
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants