Skip to content
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

[µTVM] Add support for mps2_an521 board #7813

Merged
merged 6 commits into from Apr 13, 2021
Merged

Conversation

gromero
Copy link
Contributor

@gromero gromero commented Apr 9, 2021

Hi,

Could the following patchset be reviewed please?

It enables the Arm MPS2-AN521 board to be used on microTVM:

TARGET = tvm.target.target.micro("mps2_an521")
BOARD = "mps2_an521-qemu"

It also adds to micro_tflite.py an example on how to run Zephyr demo in apps/ using that new target.

To use the MPS2-AN521 target it's necessary the newest Zephyr 2.5.0 branch [0] that contains the following fixes:

drivers: uart: uart_cmsdk_apb: fix interrupt handling
zephyrproject-rtos/zephyr@ec0aa83

drivers: uart: uart_cmsdk_apb: Fix uart_irq_is_pending
zephyrproject-rtos/zephyr@bd0fe17

boards: arm: mps2-an521: Fix DT memory regions
zephyrproject-rtos/zephyr@fe3f71b

It's also necessary to have qemu-system-arm available in the environment (reachable from $PATH).

Thanks,
Gustavo

[0] https://github.com/zephyrproject-rtos/zephyr/commits/v2.5-branch

Some boards supported by Zephyr that run emulated by default, i.e. their
.yaml config file sets the field "simulation: qemu", don't have the
prefix "qemu_" on their names, so µTVM can't currently recognize it as
an emulated target to properly use the QEMU transporter (instead of the
serial port) to open a session against it. Such a boards usually have
real physical (hardware) counterparts, being specific boards and not
generic or "fake" ones simply tied to a CPU type of interest.

That commit allows the µTVM user to explicitly inform that µTVM needs
to use the QEMU transporter to open a session against a given board by
adding the suffix "-qemu" to the board name. That is necessary because
for boards that don't have the name prefixed by "qemu_" and even though
run emulated by default on Zephyr there is no easy way to detect them,
since it's not possible to determine it by looking at any Cmake
generated file or by using the `west` command to query that info.

The case where the board is emulated by default but has the prefix
"qemu_" in its board name is already handled by the current code.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
This commit adds a new µTVM target to support the Arm reference board
MPS2-AN521, which is based upon a Cortex-m33 core.

For more details about that board, please see:
http://developer.arm.com/tools-and-software/development-boards/fpga-prototyping-boards/mps2

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
This commit adds an example on how to run the Zephyr demo under apps/
using as a target the Arm mps2_an521 board, which is emulated by default
on Zephyr. The example is added to the tutorial script micro_tflite.py,
where other examples for other targets exist.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
@gromero
Copy link
Contributor Author

gromero commented Apr 9, 2021

@mehrdadh
Copy link
Member

mehrdadh commented Apr 9, 2021

@gromero Thanks for this PR!
For clarification, is Zephyr 2.5.0 branch [0] different than 2.5.0?

@mehrdadh
Copy link
Member

mehrdadh commented Apr 9, 2021

@gromero there is a lint issue. You can run make format in the main directory and it will fix it assuming you have clang-format-10.

Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

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

@gromero thanks for this pr, mostly looks good! you can also use docker/lint.sh which will use the same docker container used in regression.

@@ -108,7 +108,18 @@ def __init__(
f"project_dir supplied to ZephyrCompiler does not exist: {project_dir}"
)

if "qemu" in board:
Copy link
Contributor

Choose a reason for hiding this comment

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

can you simplify: self._is_qemu = "qemu" in board

Copy link
Contributor Author

@gromero gromero Apr 9, 2021

Choose a reason for hiding this comment

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

@areusch I can but I'm actually trimming off based on that check too. If I simplify like you suggest it would be:

diff --git a/python/tvm/micro/contrib/zephyr.py b/python/tvm/micro/contrib/zephyr.py
index ed66ce111..e772fa610 100644
--- a/python/tvm/micro/contrib/zephyr.py
+++ b/python/tvm/micro/contrib/zephyr.py
@@ -108,15 +108,14 @@ class ZephyrCompiler(tvm.micro.Compiler):
                 f"project_dir supplied to ZephyrCompiler does not exist: {project_dir}"
             )
 
-        if "qemu" in board:
-            self._qemu = True
+        self._qemu = "qemu" in board
 
-            # For Zephyr boards that run emulated by default, i.e. "simulation: qemu" is set in
-            # the board's .yaml config file, and which their names don't have the prefix "qemu_", a
-            # suffix "-qemu" is used in microTVM to inform that the QEMU transporter has to be used.
-            # So the suffix needs to be trimmed off before the board name is passed to Zephyr.
-            if "-qemu" in board:
-                board = board.replace("-qemu", "")
+        # For Zephyr boards that run emulated by default, i.e. "simulation: qemu" is set in
+        # the board's .yaml config file, and which their names don't have the prefix "qemu_", a
+        # suffix "-qemu" is used in microTVM to inform that the QEMU transporter has to be used.
+        # So the suffix needs to be trimmed off before the board name is passed to Zephyr.
+        if "-qemu" in board:
+            board = board.replace("-qemu", "")
 
         self._board = board

i.e. to keep the same result I would have to check for "-qemu" anyway below in order to trim off the suffix from the board name. Am I following your suggestion correctly? If so, I still prefer checking for the need of trimming off only when "qemu" is in board name, so the first version.

Copy link
Contributor

Choose a reason for hiding this comment

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

your diff looks right to me. I think this is one of those cases where there is not much CPU time to be saved, and in particular since this is Python, the self._is_qemu lookup could cost more time than the substring check. in these cases, I just favor the easiest code to read/debug, rather than adding e.g. if self._is_qemu and "-qemu" in board (you would the have to explain the self._is_qemu--is this intending to capture some other criteria, which may shift over time to be more than just "qemu" in board?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Over time I think that code will be updated to take care of FVP emulation too (about to be added to Zephyr, for Cortex M-55 + Ethos U-55, i.e. for MPS3 boards which are not supported by QEMU).

That said, I was actually not thinking of saving CPU cycles (I don't think there is a difference in performance between the two forms), nor about adding FVP support in the future, rather I just think that the first form is better to read/debug because it makes explicit that the trimming off applies only for a specific case where the suffix is present in the board name :) In the second form that dependency is "weak" and one maybe need to go back to the first if to realize that the second if will never be taken if the first is not taken. Anyway, since the result is the same, I'm sending a new version with the change suggested. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah okay. your logic makes sense, but I guess now we have a passing PR and I think they're pretty similar. let's merge this.

@gromero
Copy link
Contributor Author

gromero commented Apr 9, 2021

@maheshambule @areusch Thanks a lot for the hint on how to check lint locally. I was not aware of them : ) make format actually suggested more than 200 changes, which really seemed too picky and not necessary, so I ended up using just docker/bash.sh tlcpack/ci-lint:v0.62 ./tests/scripts/task_lint.s and it made the magic.

@gromero
Copy link
Contributor Author

gromero commented Apr 9, 2021

@gromero Thanks for this PR!
For clarification, is Zephyr 2.5.0 branch [0] different than 2.5.0?

Hi @mehrdadh ! Yes, they are different. The v2.5.0 is a tag and so a cut of master when the release happened. The v2.5-branch is a maintenance branch where the backports land (the great majority are fixes) for 2.5.0 and, afaik, it's considered stable at any time. I pointed out that this PR requires the v2.5-branch because the fixes I've listed (that are necessary for the MPS2-AN521) landed in master after the "cut" for the v2.5.0 release, so I had to backport them to Zephyr 2.5.0, hence they are present in v2.5-branch only.

Fix lint accordingly to the CI error.

Please, discard this commit message when squashing it to commit:

[µTVM] Zephyr: Allow user inform if a board is emulated

in the series.

Thanks,
Gustavo
Satisfy lint about boolean expression format.

Please, discard this commit message when squashing it to commit:

[µTVM] Zephyr: Allow user inform if a board is emulated

in the series.

Thanks,
Gustavo
@areusch
Copy link
Contributor

areusch commented Apr 12, 2021

@maheshambule @areusch Thanks a lot for the hint on how to check lint locally. I was not aware of them : ) make format actually suggested more than 200 changes, which really seemed too picky and not necessary, so I ended up using just docker/bash.sh tlcpack/ci-lint:v0.62 ./tests/scripts/task_lint.s and it made the magic.

yeah so this is why you should prefer docker/lint.sh (or equivalent docker/bash.sh) for linting operations--the output of the lint tool is extremely sensitive to the version of e.g. cpplint, pylint, etc that you are using. the docker container removes that issue from consideration.

Address suggestion from Andrew in the review.

Also updates the comment about suffix being trimmed off.

Please discard that commit message when squashing into commit:

[µTVM] Zephyr: Allow user inform if a board is emulated

in the patch series.

Thanks,
Gustavo
@areusch areusch merged commit 9782e6e into apache:main Apr 13, 2021
@areusch
Copy link
Contributor

areusch commented Apr 13, 2021

thanks @gromero , the PR is now merged!

@gromero
Copy link
Contributor Author

gromero commented Apr 13, 2021

thanks @gromero , the PR is now merged!

@areusch Thanks a lot for the reviews and for merging it!

areusch added a commit to areusch/tvm that referenced this pull request Apr 15, 2021
areusch added a commit to areusch/tvm that referenced this pull request Apr 15, 2021
mehrdadh pushed a commit to mehrdadh/tvm that referenced this pull request Apr 22, 2021
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
* [µTVM] Zephyr: Allow user inform if a board is emulated

Some boards supported by Zephyr that run emulated by default, i.e. their
.yaml config file sets the field "simulation: qemu", don't have the
prefix "qemu_" on their names, so µTVM can't currently recognize it as
an emulated target to properly use the QEMU transporter (instead of the
serial port) to open a session against it. Such a boards usually have
real physical (hardware) counterparts, being specific boards and not
generic or "fake" ones simply tied to a CPU type of interest.

That commit allows the µTVM user to explicitly inform that µTVM needs
to use the QEMU transporter to open a session against a given board by
adding the suffix "-qemu" to the board name. That is necessary because
for boards that don't have the name prefixed by "qemu_" and even though
run emulated by default on Zephyr there is no easy way to detect them,
since it's not possible to determine it by looking at any Cmake
generated file or by using the `west` command to query that info.

The case where the board is emulated by default but has the prefix
"qemu_" in its board name is already handled by the current code.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>

* [µTVM] Add new target mps2_an521

This commit adds a new µTVM target to support the Arm reference board
MPS2-AN521, which is based upon a Cortex-m33 core.

For more details about that board, please see:
http://developer.arm.com/tools-and-software/development-boards/fpga-prototyping-boards/mps2

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>

* [µTVM] Add an example for the mps2_an521 board

This commit adds an example on how to run the Zephyr demo under apps/
using as a target the Arm mps2_an521 board, which is emulated by default
on Zephyr. The example is added to the tutorial script micro_tflite.py,
where other examples for other targets exist.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>

* Fix lint

Fix lint accordingly to the CI error.

* Satisfy lint

Satisfy lint about boolean expression format.

* Address suggestion from Andrew

Address suggestion from Andrew in the review.

Also updates the comment about suffix being trimmed off.

Thanks,
Gustavo
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
* [µTVM] Zephyr: Allow user inform if a board is emulated

Some boards supported by Zephyr that run emulated by default, i.e. their
.yaml config file sets the field "simulation: qemu", don't have the
prefix "qemu_" on their names, so µTVM can't currently recognize it as
an emulated target to properly use the QEMU transporter (instead of the
serial port) to open a session against it. Such a boards usually have
real physical (hardware) counterparts, being specific boards and not
generic or "fake" ones simply tied to a CPU type of interest.

That commit allows the µTVM user to explicitly inform that µTVM needs
to use the QEMU transporter to open a session against a given board by
adding the suffix "-qemu" to the board name. That is necessary because
for boards that don't have the name prefixed by "qemu_" and even though
run emulated by default on Zephyr there is no easy way to detect them,
since it's not possible to determine it by looking at any Cmake
generated file or by using the `west` command to query that info.

The case where the board is emulated by default but has the prefix
"qemu_" in its board name is already handled by the current code.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>

* [µTVM] Add new target mps2_an521

This commit adds a new µTVM target to support the Arm reference board
MPS2-AN521, which is based upon a Cortex-m33 core.

For more details about that board, please see:
http://developer.arm.com/tools-and-software/development-boards/fpga-prototyping-boards/mps2

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>

* [µTVM] Add an example for the mps2_an521 board

This commit adds an example on how to run the Zephyr demo under apps/
using as a target the Arm mps2_an521 board, which is emulated by default
on Zephyr. The example is added to the tutorial script micro_tflite.py,
where other examples for other targets exist.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>

* Fix lint

Fix lint accordingly to the CI error.

* Satisfy lint

Satisfy lint about boolean expression format.

* Address suggestion from Andrew

Address suggestion from Andrew in the review.

Also updates the comment about suffix being trimmed off.

Thanks,
Gustavo
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
* [µTVM] Zephyr: Allow user inform if a board is emulated

Some boards supported by Zephyr that run emulated by default, i.e. their
.yaml config file sets the field "simulation: qemu", don't have the
prefix "qemu_" on their names, so µTVM can't currently recognize it as
an emulated target to properly use the QEMU transporter (instead of the
serial port) to open a session against it. Such a boards usually have
real physical (hardware) counterparts, being specific boards and not
generic or "fake" ones simply tied to a CPU type of interest.

That commit allows the µTVM user to explicitly inform that µTVM needs
to use the QEMU transporter to open a session against a given board by
adding the suffix "-qemu" to the board name. That is necessary because
for boards that don't have the name prefixed by "qemu_" and even though
run emulated by default on Zephyr there is no easy way to detect them,
since it's not possible to determine it by looking at any Cmake
generated file or by using the `west` command to query that info.

The case where the board is emulated by default but has the prefix
"qemu_" in its board name is already handled by the current code.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>

* [µTVM] Add new target mps2_an521

This commit adds a new µTVM target to support the Arm reference board
MPS2-AN521, which is based upon a Cortex-m33 core.

For more details about that board, please see:
http://developer.arm.com/tools-and-software/development-boards/fpga-prototyping-boards/mps2

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>

* [µTVM] Add an example for the mps2_an521 board

This commit adds an example on how to run the Zephyr demo under apps/
using as a target the Arm mps2_an521 board, which is emulated by default
on Zephyr. The example is added to the tutorial script micro_tflite.py,
where other examples for other targets exist.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>

* Fix lint

Fix lint accordingly to the CI error.

* Satisfy lint

Satisfy lint about boolean expression format.

* Address suggestion from Andrew

Address suggestion from Andrew in the review.

Also updates the comment about suffix being trimmed off.

Thanks,
Gustavo
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request May 6, 2021
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request May 11, 2021
* [µTVM] Zephyr: Allow user inform if a board is emulated

Some boards supported by Zephyr that run emulated by default, i.e. their
.yaml config file sets the field "simulation: qemu", don't have the
prefix "qemu_" on their names, so µTVM can't currently recognize it as
an emulated target to properly use the QEMU transporter (instead of the
serial port) to open a session against it. Such a boards usually have
real physical (hardware) counterparts, being specific boards and not
generic or "fake" ones simply tied to a CPU type of interest.

That commit allows the µTVM user to explicitly inform that µTVM needs
to use the QEMU transporter to open a session against a given board by
adding the suffix "-qemu" to the board name. That is necessary because
for boards that don't have the name prefixed by "qemu_" and even though
run emulated by default on Zephyr there is no easy way to detect them,
since it's not possible to determine it by looking at any Cmake
generated file or by using the `west` command to query that info.

The case where the board is emulated by default but has the prefix
"qemu_" in its board name is already handled by the current code.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>

* [µTVM] Add new target mps2_an521

This commit adds a new µTVM target to support the Arm reference board
MPS2-AN521, which is based upon a Cortex-m33 core.

For more details about that board, please see:
http://developer.arm.com/tools-and-software/development-boards/fpga-prototyping-boards/mps2

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>

* [µTVM] Add an example for the mps2_an521 board

This commit adds an example on how to run the Zephyr demo under apps/
using as a target the Arm mps2_an521 board, which is emulated by default
on Zephyr. The example is added to the tutorial script micro_tflite.py,
where other examples for other targets exist.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>

* Fix lint

Fix lint accordingly to the CI error.

* Satisfy lint

Satisfy lint about boolean expression format.

* Address suggestion from Andrew

Address suggestion from Andrew in the review.

Also updates the comment about suffix being trimmed off.

Thanks,
Gustavo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants