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

Add support for memory mapped UARTs #15

Merged
merged 15 commits into from
Jun 6, 2021
Merged

Conversation

remimimimimi
Copy link
Contributor

@remimimimimi remimimimimi commented May 13, 2021

Unresolved questions:

  • Feature naming
  • Do we need atomics?
    • Atomic ordering

@phil-opp
Copy link
Member

Thanks for the PR!

I don't think that reusing the same SerialPort type for two very different things (MMIO vs port IO) is a good idea. There are even differences in the function signatures, e.g. the new method takes a pointer instead of a port number. I would prefer to introduce the MMIO-based port functionality through a separate MmioSerialPort type.

Regarding the features, I would prefer to keep the current stable and nightly features if possible. If necessary, we can add an additional mmio feature. We might need some trickery for the x86_64 dependency and its features, but it should be possible to implment this via target-specific dependencies somehow.

@remimimimimi
Copy link
Contributor Author

remimimimimi commented May 14, 2021

Thanks for the PR!

Regarding the features, I would prefer to keep the current stable and nightly features if possible. If necessary, we can add an additional mmio feature. We might need some trickery for the x86_64 dependency and its features, but it should be possible to implment this via target-specific dependencies somehow.

I don't see other way to solve this problem besides these variants.

  1. Add port, mmio_stable, mmio_nightly features and stay with stable nightly, but this kinda ugly.
[features]
default = [ "port", "nightly" ]
port = []
stable = [ "x86_64/external_asm" ]
nightly = [ "x86_64/inline_asm" ]
mmio_stable = []
mmio_nightly = []
  1. Small api break but this will make it more logic.
[features]
default = [ "port_nightly" ]
port_stable = [ "x86_64/external_asm" ]
port_nightly = [ "x86_64/inline_asm" ]
mmio_stable = []
mmio_nightly = []
  1. Stay only with stable or nightly version of mmio. So there's no api break but less choice.
[features]
default = [ "port_nightly" ]
port_stable = [ "x86_64/external_asm" ]
port_nightly = [ "x86_64/inline_asm" ]
mmio = []
  1. Due SerialPort::new() no longer requires nightly #16 we can also do this
[dependencies]
x86_64 = { version = "0.14.0", default-features = false, features = ["instructions", "x86_64/external_asm"], optional = true }

[features]
default = [ "port"]
port = ["x86_64"]
mmio = [] 
# Can combined with mmio
stable = [] # Noop for port feature 
nightly = [] # Noop for port feature 

There's issue about target-specific features but it opened since 2015, so this can be perfect version but nowadays it impossible.

[target.'cfg(target = "x86_64"'.features]
stable = ["x86_64/external_asm"]
nightly = ["x86_64/inline_asm"]
[target.'cfg(target = "riscv"'.features]
stable = []
nightly = []

@phil-opp phil-opp mentioned this pull request May 16, 2021
This enables the `MmioSerialPort` type unconditionally on all architectures. The `SerialPort` type is only enabled on `x86_64`.
@phil-opp
Copy link
Member

I think we can simplify this by enabling the x86_64 dependency only on target_arch = x86_64 targets. Then we don't need any cargo features for disabling it. I prepared a patch to implement this in remimimimimi#1 (a PR against your PR).

remimimimimi and others added 2 commits May 16, 2021 17:56
Simplify cargo features by making `x86_64` a target-specific dependency
README.md Outdated Show resolved Hide resolved
@remimimimimi remimimimimi requested a review from phil-opp May 26, 2021 18:54
Copy link
Contributor Author

@remimimimimi remimimimimi left a comment

Choose a reason for hiding this comment

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

Resolve requested changes

@phil-opp phil-opp changed the title Add riscv support through mmio Add support for memory mapped UARTs Jun 6, 2021
Copy link
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

Thanks for the updates and sorry for the delay! Looks good to me overall, I have a few suggestions for improvements, but I'll implement them in a follow-up PR.

@phil-opp phil-opp merged commit c1578b3 into rust-osdev:master Jun 6, 2021
phil-opp added a commit that referenced this pull request Jun 6, 2021
@phil-opp
Copy link
Member

phil-opp commented Jun 6, 2021

Follow-up changes done in #18. Published as v0.2.15.

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.

2 participants