Skip to content

ports: mimxrt: Switch to official GitHub SDK (WIP) #11516

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

imi415
Copy link
Contributor

@imi415 imi415 commented May 16, 2023

This (WIP) PR will replace nxp_sdk submodule with official mcux_sdk released here

The current nxp_sdk was used for both mpy and TinyUSB project, given the fact that TinyUSB project has switched to mcux_sdk already (At least for Kinetis, LPC55, and i.MXRT devices), and nxp_sdk drivers are kept on an fairly old version with limited devices supported, the mcux_sdk might be a better choice for existing and future devices.

The PR also updated the (Config Tool Generated) clock_config presets and pinmux table for RT1052 series, which was updated due to device macro names has changed in recent revisions.

The CMSIS core headers is nolonger shipped in official mcux_sdk, so mpy's CMSIS lib is used instead.

Currently tested platform:

  • MIMXRT1020EVK

Other EVKs only performed compilation tests, will be updated here after testing.

@github-actions
Copy link

github-actions bot commented May 16, 2023

Code size report:


@robert-hh
Copy link
Contributor

Thanks for the attempt. I had considered switching to the mcux_sdk a while ago, but refused so, since the mcux_sdk is huge (>807MB), containing many boards that are out of scope. And tests with the boards were not promising. They compiled, but did not work well. So switching requires each and every IO device to be tested again. I do not know how much testing of the peripherals you did with the 1020 EVK board.

@imi415
Copy link
Contributor Author

imi415 commented May 16, 2023

Thanks for the attempt. I had considered switching to the mcux_sdk a while ago, but refused so, since the mcux_sdk is huge (>807MB), containing many boards that are out of scope. And tests with the boards were not promising. They compiled, but did not work well. So switching requires each and every IO device to be tested again. I do not know how much testing of the peripherals you did with the 1020 EVK board.

Thanks @robert-hh , and I also agree that doing extensive tests with every boards is going to be slow and painful, and that's why this PR is still a WIP draft - and will still be a draft until all implemented drivers in machine modules are tested with every supported boards (there aren't much, are they?).

Since there's still not any automated and reproduceable method of doing so, I'll post my manual or (semi-)auto test result matrix and method/code (maybe in another repository) if anyone is interested.

@robert-hh
Copy link
Contributor

until all implemented drivers in machine modules are tested with every supported boards (there aren't much, are they?).

At the moment we have:

MIMXRT1010_EVK
MIMXRT1015_EVK
MIMXRT1020_EVK
MIMXRT1050_EVK(B)
MIMXRT1060_EVK(B)
MIMXRT1064_EVK(B)
MIMXRT1170_EVK
OLIMEX RT1010 (i.mx rt 1011)
Seeed Arch Mix (i.mx rt 1052)
Adafruit Metro M7 (i.mx rt 1011)
Teensy 4.0 (i.mx rt 1062)
Teensy 4.1 (i.mx rt 1062)
and there is another board used by @iabdalkader.

The MCU's are i.mx rt1011, 1015, 1021, 1052, 1062, 1064 and 1176, as far as I can tell. So yes, it's way less then the ~170 devices supported by the mcux_sdk.

@robert-hh
Copy link
Contributor

Since there's still not any automated and reproduceable method of doing so,

I once started with a kind of test shield for the EVK boards with some wiring and external components on it for some amount of automated testing, but abandoned that. It did not help much. I can support in testing, especially for the non-NXP boards.

@iabdalkader
Copy link
Contributor

I have been using 2.13.1 for a while with 60/62/64 with no problems, it build with MicroPython with one change just need to fix the macro names in machine_i2s.c.

@imi415
Copy link
Contributor Author

imi415 commented May 16, 2023

I once started with a kind of test shield for the EVK boards with some wiring and external components on it for some amount of automated testing, but abandoned that. It did not help much.

I think this is a great idea worth trying, an Arduino shield with testing peripherals

I can support in testing, especially for the non-NXP boards.

Thanks, and that would be great help.

@robert-hh
Copy link
Contributor

I have been using 2.13.1 for a while with 60/62/64 with no problems,

I had issues when trying to move from 2.10 to 2.11. All compiled well, but there were issues with the clock configuration. That was before support for I2S was added.

@iabdalkader
Copy link
Contributor

@robert-hh Speaking of clock config, those a very board-specific files (*clock_config.c/h) don't you think those files should be moved in board dirs, (i.e boards/$(BOARD)/clock_config.h/c ? Even if they are duplicated for some boards, but this would allow a board to override it.

@robert-hh
Copy link
Contributor

robert-hh commented May 16, 2023

don't you think those files should be moved in board dirs

That's what it was initially, but when more boards for a specific MCU were added, Damien asked me to move these files to the boards directory. Nowadays, I would consider a mcu/xxyy directory structure. Besides that, a board specific clock_config.c file could also be introduced using Makefile/mpconfigboard.mk variations.
But do not let us have too many changes at the same time. Git is pretty bad at handling changes to Makefile and mpconfigxxx.h files.

@robert-hh
Copy link
Contributor

I started a few build & run tests. Nothing special. MIXRT1176 and Olimex worked fine. Seeed Arch Mix requires the same change for mpconfigboard.h like you made for the MIMXRT1050_EVKB. I have attached the updated file. It's better if changes are just made at one place. For the MIMXRT1176 and the SEEED Arch Mix board I just tested, whether they compile & boot, show REPL, and whether the LAN interface works. It does. The max speed I got with the MIMXRT1170_EVKB was ~300 MBit/s at the 1G interface, streaming data via TCP from a Python script into netcat running on my 10 year old PC.

mpconfigboard.zip

@imi415
Copy link
Contributor Author

imi415 commented May 18, 2023

Thanks @robert-hh ,

I have just completed a prototype based on your idea about making a shield which could make the tests automated, the tricky parts are standard peripherals, like GPIO, UART, I2C, SPI, PWM etc., while the USB CDC, SDCard and Ethernet can be easily verified on the DUT itself. The boards are currently out for manufacturing, and I'll update the files in another repo after the operations and firmware of that shield are tested.

@robert-hh
Copy link
Contributor

I have just completed a prototype

Wow. That's fast. For UART I had just connections between RX and TX for a local loopback. Fir I2C I had added an external I2C DAC, which also supplied a varying analog voltage for the ADC. But I2C can also be tested with the I2S and the WM8960, or the Ethernet PHY, and for ADC you can also just use a resistor ladder. SPI is more tricky. I had implemented peripheral mode once, but it was rejected because first the API had to be defined (if ever). But for that an external device could be used as well. Alternatively, a small micro can be added to the test shield, which itself does not require many external components and which can act as a counterpart. Obviously that makes the set-up more complicated than just passive components or simple external devices.

@imi415
Copy link
Contributor Author

imi415 commented May 18, 2023

Alternatively, a small micro can be added to the test shield, which itself does not require many external components and which can act as a counterpart.

That's exactly what I actually did here. The firmware might be a tricky part, but we'll get there after the hardware is built.

@robert-hh
Copy link
Contributor

That looks excellent. Yes, the software might get some work. But it can grow step-by-step. It could have a kind of command interface e.g. connected to the UART at D0/D1, making it possible to control the operation.

@iabdalkader
Copy link
Contributor

but refused so, since the mcux_sdk is huge (>807MB), containing many boards that are out of scope

Would it make more sense to just mirror the used devices in a new repo, and just update them every now and then, something like:

https://github.com/micropython/stm32lib

Would also allow for the possibility of patching the SDK (if that's ever needed).

@robert-hh
Copy link
Contributor

Of course. I did similar before, until Hatach kindly updated his copy of the repository. But that would require to put that subset at a suitable URL and have it administered. For STM32 it is the MicroPyhton repository. But I have doubts that Damien is interested to do the administration. Obviously someone else could have a look at the versions and make PRs whenever needed.

@robert-hh
Copy link
Contributor

@imi415 I made short tests for
UART, I2C, SPI for the 1011, 1015, 1020, 1052, 1062 and 1176 devices.
I2S for 1020, 1052 and 1176. For 1062, my I2S adapter seems broken.
SDCARD with 1020, 1052, 1062 and 1176
Ethernet for 1020, 1052, 1062 and 1176
No tests yet for PWM, the various Pin modes, RTC.

All tests worked fine. I have to test further. The performance tests seemed to run a little bit slower.

@robert-hh
Copy link
Contributor

Testing PWM:
Passed for i.MX RT 1011, 1015, 1021, 1062, 1176
FAIL for i.MX RT 1052. The Quadtimer PWM works, the FLEXPWM one one. It works on the existing lib version. To be analyzed.

@robert-hh
Copy link
Contributor

About MIMXRT1052 PWM. The line:
CLOCK_SetMode(kCLOCK_ModeRun);
was missing at the end of MIMXRT1052_clock_config.c. With that line PWM works.

@robert-hh
Copy link
Contributor

@imi415 Did you make progress with your test shield? I could not open the Kicad files because the most recent KiCad I can run is v6.x. Seems like I need a new PC, or have to switch to Ubuntu.

@imi415
Copy link
Contributor Author

imi415 commented Jun 6, 2023

@imi415 Did you make progress with your test shield? I could not open the Kicad files because the most recent KiCad I can run is v6.x. Seems like I need a new PC, or have to switch to Ubuntu.

Hi Robert, I'm very grateful for the testing effort and sorry for the late response.

Yes, the project does requires KiCAD 7 to open. I have received the boards a week ago, the hardware thankfully works despite a few minor issues (saved by the SWM).

The firmware communicates with host using I2C and works like a I2C to GPIO expander but with more functionality. I have finished the I2C, GPIO, ADC/DAC part and still left SPI, UART, timer capture for PWM to go.

The firmware code is public however it is currently on a self-hosted instance out of GitHub, I'll put the repo on GitHub after finishing a working prototype.


ENET_Init(ENET, &g_handle, &enet_config, &buffConfig[0], hw_addr, source_clock);
ENET_SetCallback(&g_handle, eth_irq_handler, (void *)self);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed somewhere between v2.13 and v2.15, callback is in config struct.

@@ -157,7 +157,9 @@ void machine_rtc_start(void) {
// Do a basic init.
SNVS_LP_Init(SNVS);
// Disable all external Tamper
#if defined(FSL_FEATURE_SNVS_HAS_MULTIPLE_TAMPER) && (FSL_FEATURE_SNVS_HAS_MULTIPLE_TAMPER > 0)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some parts (RT1021) does not have SNVS tamper.

Copy link

codecov bot commented Jan 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.39%. Comparing base (5114f2c) to head (55d855d).

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #11516   +/-   ##
=======================================
  Coverage   98.39%   98.39%           
=======================================
  Files         161      161           
  Lines       21200    21200           
=======================================
  Hits        20860    20860           
  Misses        340      340           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mattytrentini
Copy link
Contributor

It would be good if git would allow subsets, but it does not.

Would shallow submodules help? At least they're possible (maybe we should use them for all submodules?):

https://git-scm.com/docs/git-submodule#Documentation/git-submodule.txt---no-recommend-shallow

@robert-hh
Copy link
Contributor

I do not see this file in the pull request

It is in the actual master branch and in the Makefile for MIMXRT. hal/fsl_lpuart.c was added later in #14041

I do not understand exactly why we cannot mitigate this in existing SDK support

Sure. It can be made as part of the move to more recent versions of the library. Only it has to be done. And since that part of the code is not broken, there is no urge.

@robert-hh
Copy link
Contributor

Would shallow submodules help?

I do not know. If we switch, it may be best to first just change the submodule source path and make sure all works fine with v2.10. And then progress the versions. At other submodules we often do not use the most recent commit.

@imi415
Copy link
Contributor Author

imi415 commented Nov 4, 2024

Hi @robert-hh,
It seems GitHub SDK release method is subject to change significantly, so let's see whether the new SDK repository has better way to integrate into MPy.

Thanks & BR,
Yilin Sun

@robert-hh
Copy link
Contributor

@imi415 Thank you for the note. If that results in a smaller repository for the MIMXRT devices it will help.

@andrewleech
Copy link
Contributor

I'm coming to this as a relatively new mimxrt user.

At work we're evaluating mimxrt1176 for use with LVGL as a potential standardised platform for ongoing use.
LVGL has good support from NXP but their drivers expect a significantly newer version of the SDK.

This started my efforts in hathach/nxp_driver#9 to automate the updating of the repo currently used here for NXP drivers but my preference really would be to use the official NXP repo.

Using their repo means one less thing that needs maintenance to pull in upstream improvements by NXP.

Keeping up to date with the official nxp repo would help us get support from NXP if needed; they're getting involved directly in other open source communities like zephyr, it'd be great to encourage their input here also.

The large repo size may be an issue in slowing CI down here having to pull a large amount each time, though as suggested shallow clone might help a lot here.

Large checkouts are painful for some users I know, though for most people you'll only need to do this once for their checkout and then never think of it again. Not everyone can afford large disks and download quotas certainly but toolchain downloads measured in GB are quite common these days, avoiding that comes at increased maintenance costs.

Is there a different downside to large submodules I'm overlooking?

In terms of upstream version support, I know NXP are still doing something quirky with regards to releases from their mcux package being different somehow to the GitHub master, the actual release packs on the GitHub page come from isolated commits outside of the main git tree.

I think the differences between the two are being reduced though, to the point I think it might be easier to migrate to newer upstream version than old.

For that it's worth, before I learned of this PR I had a test build for 1176 compiled against mcux 2.16 from GitHub with only a couple of hashdef changes to micropython, plus replacing hal/fsl_lpuart.c with the official one. I haven't had a chance to test the build on hardware though so I'm sure there's things I'm missing relating to the changes made here and mentioned above.

@robert-hh
Copy link
Contributor

As said, changing is possible. This PR is still in the draft mode, so it cannot be merged. But it will need anyhow a different approach to start, even with the huge submodule. It should first just switch the submodule, but not the version. and then progress, Changing library versions require a lot of testing.

replacing hal/fsl_lpuart.c with the official one

As far as I could tell when isolating hal/fsl_lpuart.c the only change to the library version was the handling of the line_idle signal. When you use the lib version of fsl_lpuart.c, the event IRQ_RXIDLE will happen way more often that it should. It works more like IRQ_RX. Maybe the most recent version was fixed.

@andrewleech
Copy link
Contributor

andrewleech commented Nov 5, 2024

Ok, as a first trial I've raised #16163 to demonstrate reducing the size of submodules (history) and will check how much impact that has on the nxp repo size.

Ok, performing a filtered clone locally doesn't really make much difference:

1.6G    mcux-sdk
1.4G    mcux-sdk (--filter=blob:none)

@robert-hh
Copy link
Contributor

robert-hh commented Nov 6, 2024

Thank, @andrewleech . In any case, I will start testing again with the mcux-sdk once the struggle with PWM is settled. PWM is surprisingly complicated.

@andrewleech
Copy link
Contributor

@imi415 & @robert-hh I've got a slightly updated copy of this PR pushed to #16235 ( still with your as the commit author @imi415 )

I had a chance to talk to Damien briefly and his preference was slightly towards keeping a cut down sdk repo, likely eventually moved into the micropython github project space. This pattern is used for stm and works pretty well really, particularly as there's such a dramatic difference in size.

I've improved my nxp_sdk repo update script a bit and used it to mirror the official sdk @ 2.11 for the included targets, so that MR can be used to test compatibility with the existing drivers & boards. It's also been rebased to current micropython master

I believe I've preserved the fsl_uart changes you've made recently for irq handling @robert-hh

I need a newer version of the sdk for use with lvgl so will update that as a separate commit in a new PR to test separately.

@robert-hh
Copy link
Contributor

Thanks. I will test it in the next days, hopefully today.

@robert-hh
Copy link
Contributor

I made a few tests. It compiles for all MCUs. I tested with boards for all MCUs except i.MX RT 1064, lacking a board. Boards used for the test: Adafruit Metro M7 (1011), MIMXRT1015_EVK, MIMXRT1020_EVK, MIMXRT1050_EVKB (1052, Hyoerflash), Seeed ArchMix(1052, SPI flash), Teensy 4.1 (1062), and MIMXRT1170_EVK.
I checked RAM availability, UART IRQ.RXidle, and LAN, where supported. It all worked with the previous test scripts besides LAN at the SEEED Arch Mix, which did not pick up IP address. Reason unknown. It works if before calling lan.active(True) lan.ifconfig("dhcp") is called. Changes to the files like eth.c are not related to this behavior. Strange!

@robert-hh
Copy link
Contributor

There is more trouble with the Seeed Arch Mix support. When the new firmware is loaded, I cannot connect to the device any more using JLink as it used to work. Only if I attempt to connect immediately after releasing the Reset button JLink connects. Less severe issues with MIMXRT1052_EVK. It connects but shows an error which did not exist before: Could not find core in Coresight setup. No problems with the other boards so far using JLink.
Besides that it is still possible to use serial upload of the firmware for the Arch Mix board, but that is tedious.

@andrewleech
Copy link
Contributor

How strange! That suggests the swd port / debug core is being disabled. Perhaps the charge in cmsis library being used? Or was the pinouts on those boards changed in the update?

@andrewleech
Copy link
Contributor

LAN at the SEEED Arch Mix, which did not pick up IP address. Reason unknown.

Was the clock config changed for that processor / board perhaps?

@robert-hh
Copy link
Contributor

Yes. That's the next I wanted to check. At first glance, looking through all changes, it looked ,mostly like different formatting. But there were also changes in the ENET section. I do not know why this was changed only for MIMXRT1052. I will revert that changes & try.

@robert-hh
Copy link
Contributor

Rolling back the clock changes made it work again. So I have to check if any of these changes were needed. Found one so far.

@andrewleech
Copy link
Contributor

Maybe those changes were simply copied from an offer SDK builder export that didn't have so the peripherals enabled?
It'd be good to document how/where the board profile files should come from to make it easier for people to add new boards.

That being said we may not want to rush into that too much, a person from NXP told me that more board support libraries were going to be pushed to their GitHub soon to reduce reliance on the web based builder. I was asking about the LVGL integration libraries but hopefully that would extend to there other things like board/clock/pins etc. Then at least the board profile here could include a link back to the upstream repo (if not include this directly from a submodule)

@robert-hh
Copy link
Contributor

Maybe those changes were simply copied from an other SDK builder export that didn't have so the peripherals enabled?

Maybe. So for the actual PR you should revert the changes to boards/MIMXRT1052_clock_config.c and boards/MIMXRT1052_clock_config.c, but delete for now the two lines below from boards/MIMXRT1052_clock_config.c (lines 368, 369). I have to check what they do and maybe rewrite them.

    /* Disable pfd offset. */
    CCM_ANALOG->PLL_SYS &= ~CCM_ANALOG_PLL_SYS_PFD_OFFSET_EN_MASK;

@andrewleech
Copy link
Contributor

@imi415 do you remember where your changes to the files in the board profiles came from?

@imi415
Copy link
Contributor Author

imi415 commented Nov 17, 2024

@imi415 do you remember where your changes to the files in the board profiles came from?

Board file changes came from "Config Tool" by importing source files and regenerating them. These files comes with YAML comments can be imported back to CT and they needs to be updated if SDK changed significantly. I suggest using Config Tool to import the source files, make modifications graphically, then copy the generated sources files back to keep YAML comments in sync with the code for future modifications.

@robert-hh
Copy link
Contributor

@imi415 That explains the way the file is generated, but not,

  • why the configuration did not match the boards supported by MicroPython, and
  • why the PR contains a changed configuration only for the MIMXRT1052 variant.

Did you create a configuration for a different board?

@imi415
Copy link
Contributor Author

imi415 commented Nov 17, 2024

@imi415 That explains the way the file is generated, but not,

  • why the configuration did not match the boards supported by MicroPython, and

Since the configuration is imported from the original MPY's source file, I can only assume there's a mismatch between the existing code and YAML settings. (file is modified manually after it was generated).

  • why the PR contains a changed configuration only for the MIMXRT1052 variant.

I was only able to test this setup on RT1052, and this is partially why this is still WIP.

Did you create a configuration for a different board?

No, there are no "board" settings for the tool when imported from existing source file.
I'll test the above issues on RT1052 again next week when I had the time, thanks.

@robert-hh
Copy link
Contributor

robert-hh commented Nov 17, 2024

The clock config file for the MicroPython RT1052 (Clocks V5.0) is identical to the file for RT1052 library v2.10. But clock_config.c for v2.10 is different to v2.11. Difference are in the setting of TRACE_CLK_ROOT.outFreq and TRACE_CLK selection, and configuration of PLL5 (Video PLL), which is not used. The actual setting for TRACE_CLK (source & divider) 132Mhz resp 117 MHz.

Edit: Looking into the data sheet, the maximum for ARM_TRACE_CLK is 70MHz (chapter 4.4.4, Table 41). Given that, both configurations are wrong, only 117Mhz is more close to 70Mhz. Setting the divider to 5 results in ~70Mhz, which works and allows JTAG to connect.

Edit2: On further reading, ARM_TRACE_CLK is TRACE_CLK_ROOT / 2, so 132MHz should be withing range as well......

@robert-hh
Copy link
Contributor

robert-hh commented Nov 17, 2024

Finally, it was only the missing call to CLOCK_SetMode(kCLOCK_ModeRun); at the end BOARD_BootClockRUN() which caused the problems. I can move that to board_init.c, leaving BOARD_BootClockRun() as generated by the "Config Tool".
Or I just do not include the changed clock_config.* file, like the versions for the other MCUs.,

@andrewleech
Copy link
Contributor

On a tangential note, @robert-hh I don't suppose you've previously worked with the DAC on these mimxrt parts by any chance?

@robert-hh
Copy link
Contributor

robert-hh commented Nov 27, 2024

I looked at a DAC functionality of the MIMXRT10xx devices, but I only found it as part of the ACMP unit, which has an internal 6 bit DAC. I did not see any way of getting the output signal of that DAC to a Pin, not even using XBAR. Do you know more?

Edit: Scanning through the data sheets again, DAC seems available at the 117x devices, ACMP is available at MIMXRT102x, -1052, -106x.

@andrewleech
Copy link
Contributor

andrewleech commented Nov 27, 2024

Ah, yes 1176 is the main cpu I'm working with right now for which i saw the dac driver in the SDK headers. I didn't realise it was the first series of the mimxrt family to include one.
I'll add a ticket to our internal backlog to write a driver for it.

Thanks for the info about the ACMP, I haven't looked at that peripheral myself. It's quite strange they provisioned dac hardware just for that without making it available on an output.

@andrewleech
Copy link
Contributor

andrewleech commented Jan 31, 2025

Finally, it was only the missing call to CLOCK_SetMode(kCLOCK_ModeRun); at the end BOARD_BootClockRUN() which caused the problems. I can move that to board_init.c, leaving BOARD_BootClockRun() as generated by the "Config Tool". Or I just do not include the changed clock_config.* file, like the versions for the other MCUs.,

I integrated this in my updated PR as a separate commit, moving this line to ports/mimxrt/board_init.c which seems to work fine for all boards... except MIMXRT1170_EVK. Seem the 1176 chip doesn't have this function in its fsl_clock.h unlike all the other chips in the range. I've ifdef'd around it now and it's looking pretty good to me

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.

7 participants