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.

Sign up for GitHub

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

Jump to bottom

boards: add a dedicated Qemu variant based on MPS2_AN385 #11284

Closed
wants to merge 1 commit into from
Closed

boards: add a dedicated Qemu variant based on MPS2_AN385 #11284

wants to merge 1 commit into from

Conversation

nashif
Copy link
Member

@nashif nashif commented Nov 10, 2018 •
edited by pfalcon

This is the result of issues we had in #10556 related to running in Qemu.

Edit by @pfalcon: specific pointer: #10556 (comment)

@nashif nashif requested a review from andyross November 10, 2018 17:14
@codecov-io
Copy link

codecov-io commented Nov 10, 2018 •
edited

Codecov Report

Merging #11284 into master will decrease coverage by 0.09%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #11284     +/-   ##
=========================================
- Coverage   48.22%   48.13%   -0.1%     
=========================================
  Files         279      279             
  Lines       43285    43272     -13     
  Branches    10357    10356      -1     
=========================================
- Hits        20875    20827     -48     
- Misses      18270    18302     +32     
- Partials     4140     4143      +3
Impacted Files Coverage Δ
subsys/logging/log_output.c 62.38% <0%> (-10%) ⬇️
subsys/logging/log_core.c 70.85% <0%> (-4.69%) ⬇️
misc/printk.c 81.81% <0%> (-3.13%) ⬇️
include/logging/log_msg.h 89.09% <0%> (-1.82%) ⬇️
include/logging/log_core.h 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73562dc...f72f3ab. Read the comment docs.

andyross
andyross approved these changes Nov 11, 2018
View reviewed changes
Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

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

This seems better. FWIW: you could eliminate some of the pasted hardware stuff, as AFAIK there's no qemu emulation support for stuff like gpio and i2c (but... maybe I'm wrong, no idea).

galak
galak previously requested changes Nov 11, 2018
View reviewed changes
Copy link
Collaborator

@galak galak left a comment

Choose a reason for hiding this comment

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

Rather than duplicating this, why not just add a second defconfig, and Kconfig to the MPS2_AN385 for qemu.

@nashif nashif force-pushed the qemu_mps2 branch 3 times, most recently from 79f3d5d to d585bfe Compare November 11, 2018 14:07
@nashif
Copy link
Member Author

nashif commented Nov 11, 2018

This seems better. FWIW: you could eliminate some of the pasted hardware stuff, as AFAIK there's no qemu emulation support for stuff like gpio and i2c (but... maybe I'm wrong, no idea).

fixed now

@nashif
Copy link
Member Author

nashif commented Nov 11, 2018

Rather than duplicating this, why not just add a second defconfig, and Kconfig to the MPS2_AN385 for qemu.

It is not going to be a duplication, all files have been changed for the qemu target now, this is going to be similar to qemu_riscv32

@nashif
Copy link
Member Author

nashif commented Nov 11, 2018

@galak please revisit

@nashif
Copy link
Member Author

nashif commented Nov 12, 2018

@galak ping

pfalcon
pfalcon reviewed Nov 13, 2018
View reviewed changes
boards/arm/qemu_mps2/qemu_mps2.dts

#include <arm/armv7-m.dtsi>

/ {
Copy link
Contributor

@pfalcon pfalcon Nov 13, 2018 •
edited

Choose a reason for hiding this comment

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

So, why we duplicate all this, instead of #include'ing it?

Copy link
Member Author

Choose a reason for hiding this comment

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

including it from where?

Copy link
Contributor

Choose a reason for hiding this comment

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

From real mps_an385 DTS.

Copy link
Member Author

Choose a reason for hiding this comment

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

it is not an exact copy, we have removed anything that does not apply to running in qemu.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, seems like we're just eager to complicate our own life here. If the only (?) problem with mps2_an385 vs qemu variant of it is the need to disable tickless mode, why do more work than that?

@nashif nashif dismissed galak’s stale review November 16, 2018 18:09

please re-review

@nashif nashif force-pushed the qemu_mps2 branch 2 times, most recently from fc19ffd to 3bffae9 Compare November 20, 2018 23:33
@zephyrbot
Copy link
Collaborator

Found the following issues, please fix and resubmit:

License/Copyright issues

  • boards/arm/qemu_mps2/dts_fixup.h missing license.
  • boards/arm/qemu_mps2/dts_fixup.h missing copyright.
  • boards/arm/qemu_mps2/qemu_mps2.dts missing license.
  • boards/arm/qemu_mps2/qemu_mps2.dts missing copyright.

@nashif
boards: add a dedicated Qemu variant based on MPS2_AN385
f72f3ab 
We need to be able to change the board definition for usage in Qemu
and apply workarounds based on the board, so it does not make sense to
change a HW model for Qemu usage, thus creating this alias.

Signed-off-by: Anas Nashif <anas.nashif@intel.com>
@nashif nashif closed this Jan 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pfalcon pfalcon pfalcon left review comments

@andyross andyross andyross approved these changes

@andrewboie andrewboie andrewboie approved these changes

@galak galak galak left review comments

@MaureenHelm MaureenHelm Awaiting requested review from MaureenHelm

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@nashif @codecov-io @zephyrbot @andyross @pfalcon @andrewboie @galak