-
-
Notifications
You must be signed in to change notification settings - Fork 833
WIP: renesas_ra: Add (more) support for RA6M5 and RA2E1 #2164
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
base: main
Are you sure you want to change the base?
WIP: renesas_ra: Add (more) support for RA6M5 and RA2E1 #2164
Conversation
Currently missing only write and erase. And testing. That's important.
|
Still todo:
|
dragonmux
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done a review - very happy with the general direction you're managing to take this, there are only a few fairly small details we spotted that could do with being cleaned up/sorted.
| #define MF3_CF_BLOCK_SIZE (0x800U) | ||
| #define MF3_RA2A1_CF_BLOCK_SIZE (0x400U) | ||
| #define MF3_RA2A1_CF_BLOCK_SIZE (0x400U) // Contradicted by RA2A1 Ref Manual | ||
| #define MF3_DF_BLOCK_SIZE (0x400U) | ||
| #define MF3_CF_WRITE_SIZE (0x40U) | ||
| #define MF3_CF_WRITE_SIZE (0x8U) | ||
| #define MF3_RA2E1_CF_WRITE_SIZE (0x4U) | ||
| #define MF3_DF_WRITE_SIZE (0x1U) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're aware this is, on your part, a "following the style set before", however the parens here are not doing anything useful and can (should) be dropped please. Their presence doesn't follow any of the rest of the code base.
| /* See "Switching to Code Flash P/E Mode": §35.13.3.2 of the RA2E1 manual R01UH0852EJ0170. */ | ||
| /* Set PE/READ mode. Note that this is very similar to the RV40 mode control, | ||
| * just with a different target register address. This probably ought to be | ||
| * deduplicated, but getting it working is more important first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having this duplicated is fine - often de-dupe attempts result in more complex/spaghetti code, so this is more than fine with us.
| * implementation (i.e. ignoring FMS2) as that is what I have to test on | ||
| * currently. | ||
| * | ||
| * Might also need a 5 us delay here (unlikely given the speed at which this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please include a note of where exactly that 5µs delay might be needed as this may become relevant in the future. Otherwise, sound logic and reasoning 👍🏼
|
|
||
| if (!(target_mem32_read8(target, MF3_FSTATR1) & MF3_FSTATR1_FRDY) || | ||
| target_mem32_read16(target, MF3_FENTRYR) != 0) { | ||
| DEBUG_WARN("flash is not ready, may be hanging mid unfinished command due to something going wrong, " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Flash not 'flash please; this is a serious enough thing that we might suggest making it DEBUG_ERROR or, if possible to get it back through GDB at this moment in the protocol, a gdb_outf() call so the user definitely gets to see this.
| target_mem32_write16(target, MF3_FSARL, (const uint16_t)(addr & 0xffffU)); | ||
| target_mem32_write16(target, MF3_FSARH, (const uint16_t)((addr >> 16) & 0xffffU)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The const's in these casts do nothing and can (should) be dropped. Casting to uint16_t likewise has the same value effect as masking by 0xffffU so the mask can technically be dropped (though having that explicit is no bad thing).
| /* Code flash or data flash operation */ | ||
| const bool code_flash = addr < RENESAS_CF_END; | ||
|
|
||
| while (len) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's possible to express this as a for loop over len, keeping len and addr constant, it would be better - the compiler's able to generate better code for this and prove to itself the correctness of the loop for optimisation purposes. Adding/tracking an offset turns out to be ~free due to instruction set things the compiler can take advantage of.
| target_mem32_write16(target, MF3_FSARL, (const uint16_t)(dest & 0xffffU)); | ||
| target_mem32_write16(target, MF3_FSARH, (const uint16_t)((dest >> 16) & 0xffffU)); | ||
|
|
||
| while (len) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same query here, especially as src could be cast to a buffer pointer so you only have one cast to uint8_t to do and then everything else already uses the right pointer width for the remainder
| target_mem32_write16(target, MF3_FWBL0, *(const uint16_t *)src); | ||
| target_mem32_write16(target, MF3_FWBH0, *((const uint16_t *)src + 1U)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
((const uint16_t *)src)[0]/((const uint16_t *)src)[1] would we think be clearer here what you're intending to do.
Detailed description
Adding/expanding support for Renesas RA6M5, RA4M1, and RA2E1 microcontrollers.
Todo:
Your checklist for this pull request