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

target: Renesas: support for flash types MF3 and MF4 #1723

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

cfr-mir
Copy link

@cfr-mir cfr-mir commented Jan 11, 2024

Detailed description

This change implements support for flashing Renesas processors with flash type MF3 and MF4.
I have fixed "native" platform build by moving all Renesas support to be optional ENABLE_RENESAS=1.
This is only tests on a RA2L1 processor, as this is the only one I have access to.

NOTE:
At the moment writing the .IDCode segment 0x1010010, is disabled. Writing a wrong value here bricks the processor beyond repair.
For the boldhearted! remove all

	if (f->start >= 0x1010000 && f->start<=0x1010100)
		return true;

Your checklist for this pull request

  • I've read the Code of Conduct
  • I've read the guidelines for contributing to this repository
  • It builds for hardware native (make PROBE_HOST=native)
  • It builds as BMDA (make PROBE_HOST=hosted)
  • I've tested it to the best of my ability.
  • My commit messages provide a useful short description of what the commits do

Closing issues

fixes #1243

@dragonmux dragonmux added this to the v2.0 release milestone Jan 11, 2024
@dragonmux dragonmux added Enhancement General project improvement New Target New debug target labels Jan 11, 2024
@dragonmux
Copy link
Member

dragonmux commented Jan 11, 2024

Before we go nuts reviewing this, please run clang-format on your contribution as it does not follow the code style for this code base.

We're a bit uneasy with the Flash interfaces being split out into their own files like this, especially with what the renesas.c implementation is having to do - and as the existing support is left in that file, which means it's inconsistent. Please can you give us your rationale behind this split?

The enable variable you're defining in src/Makefile is inconsistent - named ENABLE_RENESAS_MF4_FLASH in one place but ENABLE_RENESAS in another.

@cfr-mir
Copy link
Author

cfr-mir commented Jan 12, 2024

clang-format, I didn't think of that.
Inconsistencies should of course be solved. I will do that.

The rationale to split in two files, is that the renesas_ra.c is already about 1000 lines and pretty hard to navigate. So adding another 1000 lines would add to the confusion.
I my world. I would be beaten up for creating files that large :-), so I think - old habits.

Your repo, you rules :-) If you insist. I will make renesas_ra 2000 lines.

I guess a solution would be to have individual files, for the two kind of processors, having their own probe function. That would follow you style, but it would also duplicate some of the probe code, hence larger flash image.

I am pretty sure main doesn't build with PROBE_HOST=native. After a rebase on main, I get:

/usr/lib/gcc/arm-none-eabi/10.3.1/../../../arm-none-eabi/bin/ld: region rom overflowed by 8 bytes

When I build, without Renesas support, which now exclude renesas_ra.c.

@dragonmux
Copy link
Member

dragonmux commented Jan 12, 2024

Your compiler is too old - main builds just fine if you use GCC 12.1.Rel1 from ARM - please upgrade your compiler to fix that problem. It's the same reason you thought you had to gate the Renesas code (which.. we'd like a compelling reason for that too ideally).

Re splitting the files - the problem isn't necessarily the split, it's the inconsistency created by the split how it's done here - you've split the Flash routines out for one processor type but not both, renesas.c could have all the probe code and Flash registration logic calling out into two seperate files - renesas_rv40.c with the Flash routines for those parts and renesas_mf4.c with the Flash routines for those and that would principally be fine. It would be consistent which is what matters to us. We don't think having it all in one file is bad though for this due to something else we need to talk about:

The code you're PR'ing here appears to be adapted from Renesas' MIT-licensed BSPs and other Renesas repos, and you're ascribing the copyright and licensing to us which is a violation of their licensing. Their license headers must be kept intact. The code also needs considerable clean-up, including BSP macros eliminating as those aren't relevant to BMD nor helpful to maintain.

There's also some timing stuff that got copied in which doesn't fit with the code base at all nor how it's organised and needs addressing. If you really need a µs delay timing function, something should be added to src/include/platform_support.h along side platform_delay, and to the platform-specific timing backends like src/platforms/common/stm32/timing_stm32.c. It will probably need to be a busy loop in most cases, but this gives you the correct way to calibrate the loop to create a microsecond delay for each platform.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement General project improvement New Target New debug target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Renesas RA2A1 (MF3 Flash architecture)
2 participants