rust-osdev / uart_16550

Minimal support for uart_16550 serial output.
MIT License
33 stars 24 forks source link

Add support for memory mapped UARTs #15

Closed remimimimimi closed 3 years ago

remimimimimi commented 3 years ago

Unresolved questions:

phil-opp commented 3 years ago

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 commented 3 years ago

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 = []
  2. 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 = []
  3. 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 = []
  4. Due #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.
```toml
[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 commented 3 years ago

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 https://github.com/remimimimi/uart_16550/pull/1 (a PR against your PR).

phil-opp commented 3 years ago

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