Skip to content

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

Merged
merged 1 commit into from
Dec 12, 2018
Merged

Conversation

c1728p9
Copy link
Contributor

@c1728p9 c1728p9 commented Dec 10, 2018

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

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Disable the MPU on the RT1050 since this target has a memory map that
is incompatible with the default MPU driver.
@c1728p9 c1728p9 changed the title Disable MPU RT1050 due to memory map Disable MPU on RT1050 due to memory map Dec 10, 2018
@cmonr cmonr requested a review from a team December 10, 2018 16:20
@mmahadevan108
Copy link
Contributor

Thank you. Can we get more information on the MPU driver.

@c1728p9
Copy link
Contributor Author

c1728p9 commented Dec 10, 2018

Hi @mmahadevan108 porting details on the MPU are here:
https://github.com/ARMmbed/mbed-os-5-docs/blob/development/docs/porting/target/mpu.md

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.

@mmahadevan108
Copy link
Contributor

mmahadevan108 commented Dec 10, 2018

MPU on MXRT was already enabled via the below code:
https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_NXP/TARGET_MCUXpresso_MCUS/TARGET_MIMXRT1050/TARGET_EVK/mbed_overrides.c#L28

Do we need to tie this in with the MPU HAL implementation?

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 11, 2018

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?

@unsignedint
Copy link

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

@cmonr
Copy link
Contributor

cmonr commented Dec 11, 2018

CI started

@cmonr
Copy link
Contributor

cmonr commented Dec 11, 2018

Sorry, false start on starting CI for this PR. Still needs review.

@cmonr
Copy link
Contributor

cmonr commented Dec 11, 2018

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

@unsignedint
Copy link

@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 :)

@cmonr
Copy link
Contributor

cmonr commented Dec 12, 2018

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

@cmonr
Copy link
Contributor

cmonr commented Dec 12, 2018

Oh, and I didn't answer your original question.

I mean, how do you define a "critical fix".

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.

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 12, 2018

@mmahadevan108 Can you approve or any work still needs to be done here (assuming that new MPU implementation comes in a new PR) ?

@mmahadevan108
Copy link
Contributor

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

@cmonr
Copy link
Contributor

cmonr commented Dec 12, 2018

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

@cmonr
Copy link
Contributor

cmonr commented Dec 12, 2018

CI started

@mbed-ci
Copy link

mbed-ci commented Dec 12, 2018

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test

@cmonr
Copy link
Contributor

cmonr commented Dec 12, 2018

Ok, I think it's time to file an issue around this test: K82F iar_arm / K82F-IAR.tests-mbed_hal-qspi.qspi frequency setting test

It's failed for the third time in recent memory, and reruns have succeeded.

CI restarted for jenkins-ci/greentea-test

@cmonr
Copy link
Contributor

cmonr commented Dec 12, 2018

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

@cmonr cmonr merged commit 4e52240 into ARMmbed:master Dec 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken i.MX RT1050-EVK target on 5.11 after MPU API merge?
6 participants