-
Notifications
You must be signed in to change notification settings - Fork 3k
Disable MPU on RT1050 due to memory map #9036
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
Conversation
Disable the MPU on the RT1050 since this target has a memory map that is incompatible with the default MPU driver.
Thank you. Can we get more information on the MPU driver. |
Hi @mmahadevan108 porting details on the MPU are here: The MPU was enabled by default for devices with a standard ARMv7-M and ARMv8-M MPU and with a compatible memory map. This target should not have had the MPU enabled since its memory map is not compatible, so this PR turns it off. |
MPU on MXRT was already enabled via the below code: Do we need to tie this in with the MPU HAL implementation? |
Yes please. We do not need to touch that init code (no changes there for now), and wait for MPU custom implementation, do we? |
@cmonr Heya, is 5.11.1 the first planned 5.11 release? If not, I think it would be good to have this in the "first" (or even RC) release as it means the target basically doesn't work. |
CI started |
Sorry, false start on starting CI for this PR. Still needs review. |
@unsignedint 5.11.1 will be the first patch release, though we're looking to get 5.11.0 out ASAP. Part of that is that we're now on our third RC, and at this point only fixes that are critical are coming in. We're still waiting to hear back from NXP, and in order to get enough testing for this last RC, we need to make it now (you can actually find the PR here: #9064). As soon as that PR is in, we have a flood of 5.11.1 PRs that will need testing, and I expect that this will be coming in shortly. |
@cmonr OK cool. I mean, how do you define a "critical fix"... this target doesn't work on 5.11? :) But I'm only half serious, with Christmas coming up fast we can wait till 5.11.1. But, to be honest it seems like these releases are coming too fast and furiously; I respect that its a hard job to test all these targets, but from my perspective there is still no working "official" mbed release with Ethernet support on the i.MX RT1050 which really sucks. (The feature didn't make it in 5.10.4 and 5.11 is dead-on-arrival until this patch lands). As a user, I am trying to sell mbed to our team. Official support and some stability (as opposed to a build off master) for say Ethernet, which is critical for our project, has an aura about it that bosses and leads like :) Ironically, one of the reasons I suggest mbed is great is the unit tests etc, but the MPU changes with this release (and related issues across numerous targets) doesn't support that. Just some well meaning comments, I hope you don't take that the wrong way and keep up the good work :) |
@unsignedint Are you kidding me? I love this kind of feedback! There are certainly things I'd like to see improve and get better, and we only have so much bandwidth to get work done, but seeing comments like yours helps validates things that our users themselves are facing. I've forwarded your comments to other groups. There might not be much we can do at the moment, but it wouldn't hurt to ask. Also, I'm not certain on when 5.11.1, but if history holds, it might be released a week after 5.11.0 is out. I'd need to verify that though. |
Oh, and I didn't answer your original question.
After a certain point, we have to stop generating RCs and switch over to creating the release itself. A critical fix is generally something that affects an entire feature or multiple targets (generally a family or more), or something considerably more larger. The MPU feature has definitely been a headache this release, as we almost considered disabling it by default: #8335 (comment). We want to introduce features that make the OS overall more secure and stable, but that might mean making one or two targets unstable in the short term. Of course, we'll do our best to make sure that doesn't happen, but we're only human. Fwiw, I can definitely say that we're putting focus on more stability in the near future, but I think that's about all I can say on the subject for now. |
@mmahadevan108 Can you approve or any work still needs to be done here (assuming that new MPU implementation comes in a new PR) ? |
I am fine disabling this feature for now. Can you please share plans to enable MPU support for MXRT in future? |
This will have to come from either @c1728p9 or @kjbracey-arm |
CI started |
Test run: FAILEDSummary: 1 of 11 test jobs failed Failed test jobs:
|
Ok, I think it's time to file an issue around this test: It's failed for the third time in recent memory, and reruns have succeeded. CI restarted for |
Bringing this in. Note: This still may end up being pushed out to 5.11.1. We're currently sorting out if an RC4 will be made, but at least this target should work in |
Description
Disable the MPU on the RT1050 since this target has a memory map that is incompatible with the default MPU driver.
fixes #9024
Pull request type