[microTVM][Zephyr] Add MIMXRT1050 board support #9068
Conversation
mehrdadh
requested review from
comaniac,
jroesch,
junrushao,
kparzysz-quic,
masahi,
tqchen,
vinx13,
yzhliu,
zhiics and
ZihengJiang
as code owners
| @@ -180,6 +184,9 @@ def _get_device_args(options): | |||
| if flash_runner == "openocd": | |||
| return _get_openocd_device_args(options) | |||
|
|
|||
| if flash_runner == "jlink": | |||
| return [] | |||
There was a problem hiding this comment.
is there no arg to specify serial?
There was a problem hiding this comment.
it works without specifying the --serial assuming there's only once device. We should have the serial though, I'll add it.
There was a problem hiding this comment.
I realized that _get_device_args is only used with NRF board. So we don't need to have Jlink support in this function.
|
@mehrdadh Hi. Please change the PR title from microtvm to microTVM so it matches what we've been using after we moved away from the name variant with mu char. Otherwise you know it will land in the tree as microtvm. Otherwise looks good considering Andrew's comments, although I'm not yet able to test microTVM on that NXP board. |
mehrdadh
changed the title
|
@gromero thanks, done! |
|
@mehrdadh Hi. I took a second look at this change and tested it on a RT 1050 EVK board. It looks good. I just have a few additional comments besides Andrew's comment. I'd like to get rid of the following annoying warning that happens afaics only currently on that NXP board: This is caused because NXP code in Zephyr seems to honor more the I think it's possible to avoid it (in
The one I prefer is 3. and I think it's ok because it's present in GCC 7.5 packaged by Ubuntu 18.04 (our reference environment, used for the RVM, right?). @areusch wdyt? Speaking of RVM, should you add the new board to RVM Zephyr's You'll need to tweak it a bit to add the board property to Finally, that board also fails Cheers. |
|
@gromero I just ran this PR on RVM with log-level=DEBUG and I didn't see as far as the test-config.json I recommend that we reuse board.json file and add extra information such as vid/pid to board.json file to have a single source. We could do that in a separate PR. |
|
@gromero i don't think we can use the C++ attribute since we are in C. I'm okay with solution 1 you proposed, since |
|
@gromero since it wasn't seen in the RVM, perhaps we can make this change in a follow-on PR? |
hmr I got that warning on my local environment, however I'm also able to reproduce it in the RVM, look: I'm of course passing
All right. I was just concerned about not having a way to test the board with the RVM - I had to take care of it myself to test that PR in the RVM. |
It just happens that we can use :) But I agree that 1. will be a better fit since I can't explain why |
@areusch Well, as I pointed out to @mehrdadh I do see it happening in the RVM. Of course the warning is not observed in the upstream CI and it will affect mostly the folks experimenting microTVM on that specific NXP board - and us. So since we have even already discussed a solution for it (solution 1) and we understand well what's going on I'm ok to fix it in a follow-on PR. |
| @@ -545,6 +546,10 @@ def _find_openocd_serial_port(cls, options): | |||
|
|
|||
| return ports[0].device | |||
|
|
|||
| @classmethod | |||
| def _find_jlink_serial_port(cls, options): | |||
| return cls._find_openocd_serial_port(options) | |||
There was a problem hiding this comment.
@mehrdadh I think that ideally _find_jlink_serial_port() should not be made merely a wrapper for _find_openocd_serial_port(). I understand that the code for the later works well for finding a serial for Jlink too, but openocd is not really involved on detecting the serial port in the end. It looks in Zephyr's CMake configs and goes for a detection using usb module. Hence I think that _find_openocd_serial_port() should be made a generic code (and renamed to something like _find_serial_port()) and then be used for both "openocd" and "jlink" flash runners.
_find_openocd_serial_port() even contains debug message which cites openocd which is confusing because under the hood for the board in question JLinkExe will be used to flash the fw, not openocd.
The ProjectOption openocd_serial with also contains "openocd" also becomes incorrect since that option also applies now to jlink flash runner, so ideally it should be adapted too I think.
There was a problem hiding this comment.
Thanks for extra investigation on this. I agree that this is a bit confusing. We could do a refactor here in a follow on PR.
* main: Fix flaky NMS test by making sure scores are unique (apache#9140) [Relay] Merge analysis/context_analysis.cc and transforms/device_annotation.cc (apache#9038) [LLVM] Make changes needed for opaque pointers (apache#9138) Arm(R) Ethos(TM)-U NPU codegen integration (apache#8849) [CI] Split Integration tests out of first phase of pipeline (apache#9128) [Meta Schedule][M3b] Runner (apache#9111) Fix Google Mock differences between Ubuntu 18.04 and 16.04 (apache#9141) [TIR] add loop partition hint pragma (apache#9121) fix things (apache#9146) [Meta Schedule][M3a] SearchStrategy (apache#9132) [Frontend][PyTorch] support for quantized conv_transpose2d op (apache#9133) [UnitTest] Parametrized test_conv2d_int8_intrinsics (apache#9143) [OpenCL] Remove redundant visit statement in CodeGen. (apache#9144) [BYOC] support arbitrary input dims for add/mul/relu of dnnl c_src codegen (apache#9127) [Relay][ConvertLayout] Support for qnn.conv2d_transpose (apache#9139) add nn.global_avgpool to fq2i (apache#9137) [UnitTests] Enable minimum testing on Vulkan target in CI (apache#9093) [Torch] Support returning quantized weights and bias for BYOC use cases (apache#9135) [Relay] Prepare for new plan_devices.cc (part II) (apache#9130) [microTVM][Zephyr] Add MIMXRT1050 board support (apache#9068)
* add target support * fix ci issue
* add target support * fix ci issue
This PR:
imxrt1060micro target toimxrt10xxwhich used for imxrt10 family.This should merge after #9026