targets: clock: change default core clock to 528M

[liugang-gavin]

Summary of changes

change the clock config of the RT1050 to set the default core clock to
528M

Signed-off-by: timwang [email protected]

Impact of changes

Set the default core speed of i.mxrt1050 to 528MHz which matchs the industrial grade requirement.

Migration actions required

Documentation

No documentation is required.

Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[X] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

---

Test results

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

---

Reviewers

---

[ciarmcom]

@liugang-gavin, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

[adbridge]

@liugang-gavin please fill in the rest of the PR header as required. Thanks

[liugang-gavin]

Updated the header of PR, thanks!

[0xc0170]

CI started

[mbed-ci]

Test run: FAILED

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

Failed test jobs:

• jenkins-ci/mbed-os-ci_example-test-lts
• jenkins-ci/mbed-os-ci_exporter-lts

[0xc0170]

I restarted the CI

[mbed-ci]

Test run: SUCCESS

Summary: 10 of 10 test jobs passed
Build number : 2
Build artifacts

[0xc0170]

@artokin Please review

[artokin]

Don't know why this change is needed, therefore forwarding review request also to @fredlee12001 and @JarkkoPaso .

[fredlee12001]

Change Requests comes from Hexing.
In order to match industrial grade requirements.

[multiplemonomials]

Can I just say, in retrospect it's kind of appalling that this pull request was merged? It clearly was not reviewed by anyone familiar with how power and clocking works in Mbed on the MIMXRT105x processor! It created not one, not two, but three different serious issues!

The first issue is simply that this is a globally breaking change made without any appropriate release notes or documentation updates. After this PR, everyone using the MIMXRT is going to see a 12% decrease in processor speed, everywhere. Yes, some people (using the MIMXRT105xxC chips) need this change for correct functionality, but others do not, and probably wouldn't be expecting an Mbed OS update, which does not say anything in the release notes, to hurt their performance! Not to mention, this change didn't need to be applied to everyone. It could have been hidden behind an option, so only people with the lower speed chips would need to decrease the frequency.

The second issue is, this PR does not correctly update the voltage regulator settings. When running at 600MHz, the chip needs a higher core voltage, but when running at 528MHz, it should use a lower one to save power. Since fsl_clock_config.c was generated by MCUXpresso Config Tools for 600MHz, it contains code to set the core voltage for 600MHz. These lines should have been removed, and would have been if Config Tools had been used to make the update instead of just making an ad-hoc change to the file. However, it's still there, meaning that all MIMXRT users are going to see higher power consumption from their processors than is needed.

The third, and most serious, issue is that this PR does not update the additional clock code located in specific.c. I'll admit that the original setup here is a bit weird -- fsl_clock_config.c is used to set the initial clock frequency on bootup, but specific.c in the same folder is responsible for slowing down/disabling things in the clock tree when deep sleep is entered, and then reenabling them after. That file was not touched by this MR, meaning that when an MIMXRT105x exits deep sleep for the first time, its core clock increases to 600MHz. This means that any MIMXRT105xxC chips, after deep sleeping, will be running at too high of a frequency and have the potential for random crashes. Additionally, Mbed peripheral drivers (e.g. UARTs) may end up with the wrong idea of what clock frequency they are running at, causing undefined behavior. Basically it's a huge mess and makes deep sleep virtually unusable.

Basically, the fact that this PR was green-lighted with no one saying anything is a huge red flag to me about Mbed's QA and code review process, and indicates that likely no one knowledgeable has been reviewing MIMXRT driver MRs for some time.

If anyone is looking for a fix for this issue, note that I am working on one on my fork, mbed-ce/mbed-os.