Skip to content
This repository has been archived by the owner on May 6, 2020. It is now read-only.

RISC-V support #3

Closed
wants to merge 5 commits into from
Closed

RISC-V support #3

wants to merge 5 commits into from

Conversation

ilya-epifanov
Copy link

@ilya-epifanov ilya-epifanov commented Apr 1, 2020

I know it's a lot of code duplication, so I made it in two commits. One which copies the async-cortex-m and another one with the necessary adjustments.

It works just fine on an RV32IMAC VexRiscv softcore

@mkroening
Copy link
Contributor

Since the changes are small adjustments regarding architecture specific bits, would it make sense to incorporate these changes via Cargo features? That way any general changes would not have to be doubled and the architecture specific differences would be easier to compare. The crate could then be renamed to async-embedded for example, to not contain links to the architecture in the name.

Then again this is a Proof of Concept and there might be some proper abstractions in the works, so I'm not sure if that would be worth it.

@ilya-epifanov
Copy link
Author

@mwkroening I ended up using features anyway, to select a suitable WFI/E/SEV implementation, since RISC-V doesn't define a standard way to do SEV

}
}

#[cfg(feature = "wait-nop")]
Copy link
Author

Choose a reason for hiding this comment

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

@mwkroening @japaric I can move ARM implementations here, too. The rest of the crate is generic and works both for ARM and for RISC-V.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like where this goes, but I have no authority here, so someone else would need to review this.

…Added several RISC-V implementations of `WFE/SEV`, as RISC-V leaves implementation of these to SoC designers.
@ilya-epifanov
Copy link
Author

ilya-epifanov commented Apr 27, 2020

@mwkroening @Disasm please take a look.
The ARM portion still runs without problems on an nRF52-DK.
The RISC-V part runs smoothly on a VexRiscV with default-features = false, features = ["isa-riscv", "riscv-wait-wfi-single-hart"]

pub mod task;
pub mod unsync;

#[cfg(feature = "isa-cortex-m")]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather replace this with architecture autodetection (with built-in cfgs or build.rs)

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately, we can't do that on stable yet. I need to conditionally depend on cortex-m, ccortex-m-udf and riscv crates. The fix for rust-lang/cargo#2524 has landed in nightly-2020-02-23.

Copy link
Author

Choose a reason for hiding this comment

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

I mean, I can change the #[cfg(...)] stuff but the user would still have to specify features = ["isa-..."] when depending on the crate.

Copy link
Author

Choose a reason for hiding this comment

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

Once rust-lang/cargo#7914 hits stable, we can remove these features

Copy link
Contributor

Choose a reason for hiding this comment

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

Strange, this solution works for me just fine:

[target.'cfg(target_arch = "arm")'.dependencies]
cortex-m = "*"

[target.'cfg(target_arch = "riscv32")'.dependencies]
riscv = "*"

Copy link
Author

Choose a reason for hiding this comment

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

Yep, it does. Please see the next commit

Copy link
Author

Choose a reason for hiding this comment

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

Neither master nor this branch work on stable, but both work on beta and nightly just fine.

@@ -43,7 +41,7 @@ pub async fn r#yield() {
self.yielded = true;
// wake ourselves
cx.waker().wake_by_ref();
asm::sev();
unsafe { crate::signal_event_ready(); }
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this is not required anymore after my fix: https://github.com/ferrous-systems/async-on-embedded/pull/2/files#diff-22ab3da1c5b4e44f95ba9b8000eda448R116
However, sev is required when task is woken from an interrupt context.

Copy link
Author

Choose a reason for hiding this comment

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

Well, your fix included the sev call so I kept it

cortex-m = "0.6.2"
cortex-m-udf = { path = "../cortex-m-udf" }

[target.'cfg(target_arch = "riscv32")'.dependencies]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[target.'cfg(target_arch = "riscv32")'.dependencies]
[target.'cfg(any(target_arch = "riscv32", target_arch = "riscv64"))'.dependencies]

Maybe it's a good idea to add riscv64 too :)

Copy link
Author

Choose a reason for hiding this comment

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

Done. No way for me to test it though

async-embedded/Cargo.toml Outdated Show resolved Hide resolved
Co-Authored-By: Vadim Kaushan <admin@disasm.info>
@ilya-epifanov
Copy link
Author

@Disasm could you please sign this one off? At least the RISC-V part.

@Disasm
Copy link
Contributor

Disasm commented Apr 30, 2020

@ilya-epifanov what do you mean? Just to note: I'm not a maintainer of this repo and I don't even have a push access to your PR-branch.

@ilya-epifanov
Copy link
Author

@Disasm I mean if this PR gets an approval from the most active reviewer, the maintainers will have a better idea of what's going on here 😀

@Disasm
Copy link
Contributor

Disasm commented Apr 30, 2020

Well, if there are no objections, I think this PR is ready to get merged!

@ilya-epifanov
Copy link
Author

@skade could you please take a look?

@japaric
Copy link
Contributor

japaric commented May 5, 2020

@ilya-epifanov thanks for the PR. For the time being, we (Ferrous) do not intend to continue working on this repository. Instead, we'd like the embedded community to continue to explore async/await on different embedded platforms and try out different things. For more details, check out our latest blog post.

Of course, you are more than welcome to continue working on a fork of this repo! You have made quite remarkable progress to make this work on RISCV so please keep going.

@Disasm
Copy link
Contributor

Disasm commented May 5, 2020

@japaric That's unfortunate. Could we move this repo to the rust-embedded-community org?

@jamesmunns
Copy link

We've created https://github.com/rust-embedded-community/async-on-embedded/, could you please resubmit your PR there? Thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants