A minimal Rust based sdmmc driver for odroidc4

[Cheng-Li1]

This pull request integrates the SDMMC driver for Lions OS into the SDDF. The driver has been tested with the existing file system and is capable of performing read/write operations. It serves as a foundation for further protocol layer enhancements and performance optimizations.

Driver structure: sdmmc_hal(SDMMC Hardware (Physical/Platform) Layer): Interacts with the specific hardware registers of the controller to send and receive commands and data. It is hardware-specific and may only be reusable with similar hardware platforms (e.g., hardware layer for odroid C4 may only be reused for other Amlogic devices).

sdmmc_protocol(SDMMC Protocol Layer): Implements the SD card and eMMC protocols, handling tasks such as card initialization, data transfer setup, and command management. It does not interact directly with hardware registers and can be reused across all platforms using SDMMC host controllers. However, it provides a framework of interfaces that a sdmmc_hal should implement. This protocol layer is written in safe Rust, ensuring memory safety without the use of unsafe blocks.

main.rs: Contains the event loop that orchestrates the driver, leveraging both sdmmc_hal and sdmmc_protocol. This is where OS-specific logic is integrated with the SDMMC driver.

Note:

1. The SDMMC driver is functional and capable of block-level read/write operations. The protocol layer has been implemented minimally, providing the essential functionality required for data transfers. (not include card initialization, since without UHS-I support, resetting the card only results in slower speed)

2. Future work depends on enabling voltage control for UHS-I speed modes, which requires a voltage regulator or GPIO driver. These components are not yet available, so the driver currently operates without UHS-I support.

3. This pull request represents the first iteration of the SDMMC driver, with planned enhancements for improved protocol handling and performance optimization in upcoming versions.

[alwin-joshy]

One quick thing, you should run cargo fmt on it.

[Cheng-Li1]

This is a review of just local behaviour.

Thoughts on overall design I'll do probably later but my thoughts is that this is a leaky abstraction and doesn't benefit from Async rust at all in its current state and would be easier without it - the part where you want to manually implement a future is where you are reading the done/not done registers, not in the big state machine where you want an async fn

I don't agree with the suggestion to implement the entire state machine as the future. In Rust, a future is designed to return Poll::Pending or Poll::Ready(result), and once it returns a result, it is considered complete. Putting the logic of returning results inside state machine is too messy and make debugging much harder. My design should be easier to understand, debug, and add new features than a big state machine.

[midnightveil]

I don't agree with the suggestion to implement the entire state machine as the future. In Rust, a future is designed to return Poll::Pending or Poll::Ready(result), and once it returns a result, it is considered complete. Putting the logic of returning results inside state machine is too messy and make debugging much harder. My design should be easier to understand, debug, and add new features than a big state machine.

If I'm not mistaken... this is what I was saying was bad? You'd written a state machine inside the future, rather than using the syntax sugar of async fn. The manual impl Future should only be necessary when you need to block waiting for the registers to change.

You've written one for the whole sending a command thing.

[Cheng-Li1]

Hello, I can see a lot of reviews are about the comments on the code, which I did not pay much attention to. I am fixing those now. Thanks for the input.

[Cheng-Li1]

If I'm not mistaken... this is what I was saying was bad? You'd written a state machine inside the future, rather than using the syntax sugar of async fn. The manual impl Future should only be necessary when you need to block waiting for the registers to change.

You've written one for the whole sending a command thing.

I am a bit confused by your claim. As I know, one would need to implement a future object as the basic building block for other async functions that call it. I would hope for more clarification on this one. My understanding is, while you can build complex async functions by composing other async functions, at the end of the chain, there must be some base futures that are not created using async fn and await.

[midnightveil]

I am a bit confused by your claim. As I know, one would need to implement a future object as the basic building block for other async functions that call it. I would hope for more clarification on this one. My understanding is, while you can build complex async functions by composing other async functions, at the end of the chain, there must be some base futures that are not created using async fn and await.

Yes, but what I'm saying is you've implemented:

impl Future for SdMmcFuture {
    fn poll(...) {
        match state {
            StateA => {
                // do stuffA
                return Pending;
            },
            StateB => {
                // do stuffB
                return Pending;
            },
            StateC => {
                // do stuffC
                return Done(error or OK)
            }
    }
}

When it would make more sense for

// these don't /really/ need to be async, but if you need to send CMD5 for APP_CMDs before..
async fn send_command() { ... }
async fn get_response() { ... }

struct IrqWaiter;
impl Future for IrqWater {
    fn poll() {
        if (look at registers show done) {
            return Done
        else if (show error) {
            return Done with error
        } else {
            return Pending;
        }
    }
}

fn wait_for_irq(...) {
    return IrqWaiter { };
}

async fn mmc_send_command(...) {
    send_command(...).await?;
    wait_for_irq().await?;
    get_response().await?;
}

This was roughly how my existing driver worked?

[Ivan-Velickovic]

Hmm, doesn't seem to compile for me:

error: failed to run custom build command for `sel4-sys v0.1.0 (https://github.com/seL4/rust-sel4.git?rev=d3790bfd15512e18659f9491c319867fabf9552d#d3790bfd)`

Caused by:
  process didn't exit successfully: `/Users/ivanv/ts/sddf/examples/blk/build/blk/sdmmc/meson/debug/build/sel4-sys-08eeb9ebbd413fc3/build-script-main` (exit status: 101)
  --- stdout
  cargo:rerun-if-env-changed=SEL4_INCLUDE_DIRS
  cargo:rerun-if-changed=/Users/ivanv/ts/microkit-sdk-1.4.1/board/odroidc4/debug/include
  cargo:rerun-if-env-changed=SEL4_INCLUDE_DIRS
  cargo:rerun-if-changed=/Users/ivanv/ts/microkit-sdk-1.4.1/board/odroidc4/debug/include
  cargo:rerun-if-env-changed=SEL4_INCLUDE_DIRS
  cargo:rerun-if-changed=/Users/ivanv/ts/microkit-sdk-1.4.1/board/odroidc4/debug/include
  cargo:rerun-if-env-changed=SEL4_INCLUDE_DIRS
  cargo:rerun-if-changed=/Users/ivanv/ts/microkit-sdk-1.4.1/board/odroidc4/debug/include
  cargo:rerun-if-env-changed=SEL4_INCLUDE_DIRS
  cargo:rerun-if-changed=/Users/ivanv/ts/microkit-sdk-1.4.1/board/odroidc4/debug/include
  cargo:rerun-if-env-changed=SEL4_INCLUDE_DIRS
  cargo:rerun-if-changed=/Users/ivanv/ts/microkit-sdk-1.4.1/board/odroidc4/debug/include

  --- stderr
  thread 'main' panicked at /Users/ivanv/.cargo/git/checkouts/rust-sel4-a4edf3b9624837c3/d3790bf/crates/sel4/sys/build/c.rs:69:10:
  called `Result::unwrap()` on an `Err` value: ClangDiagnostic("error: version 'minimal' in target triple 'aarch64-sel4-microkit-minimal' is invalid\n")
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
make[1]: *** [sdmmc_driver.elf] Error 101
make: *** [/Users/ivanv/ts/sddf/examples/blk/build/loader.img] Error 2

[Cheng-Li1]

called Result::unwrap() on an Err value: ClangDiagnostic("error: version 'minimal' in target triple 'aarch64-sel4-microkit-minimal' is invalid\n")

Hi, Ivan. It looks like the build script of sel4 crate pass target 'aarch64-sel4-microkit-minimal' (which is suppose to be the Rust program's target) to clang, which causes this error. Could you try building this example https://github.com/seL4/rust-microkit-demo to see if it builds? I want to narrow done the problem because the example builds on my machine.

[Cheng-Li1]

When it would make more sense for

// these don't /really/ need to be async, but if you need to send CMD5 for APP_CMDs before..
async fn send_command() { ... }
async fn get_response() { ... }

struct IrqWaiter;
impl Future for IrqWater {
  fn poll() {
      if (look at registers show done) {
          return Done
      else if (show error) {
          return Done with error
      } else {
          return Pending;
      }
  }
}

fn wait_for_irq(...) {
  return IrqWaiter { };
}

async fn mmc_send_command(...) {
  send_command(...).await?;
    wait_for_irq().await?;
    get_response().await?;
}

Hello, let me summarise the suggestion in case there is any misunderstanding. So you suggest I adopt a different kind of base future design that is simpler and requires a function from the hardware layer that checks if a command has finished or not. Let me explain my thought: I think there is no need for a simpler base future as the current one that encapsulates send command and receive response logic is already simple enough. Right now, the receive_response function checks if the cmd has finished, if so return the result. There is no need to separate another function to specifically check if the cmd has finished or not. Personally, I think whether the future is designed in your proposed way or my way is more like a design choice and does not have much influence on performance or correctness.

[Cheng-Li1]

Personally, I think whether the future is designed in your proposed way or my way is more like a design choice and does not have much influence on performance or correctness.

But I do agree to put the whole send cmd and receive response logic into a separate async function like fn send_cmd_and_receive_resp can make the code look better.

[midnightveil]

Personally, I think whether the future is designed in your proposed way or my way is more like a design choice and does not have much influence on performance or correctness.

This is the whole point, to make a good design? (Which then should be performant and correct). There are a lot of ways to do things badly that can still be performant or appear correct.

[Cheng-Li1]

This is the whole point, to make a good design? (Which then should be performant and correct). There are a lot of ways to do things badly that can still be performant or appear correct.

Exactly, I definitely do not tolerate any possible wrong code that appears right or designs that are simply bad or unscalable. I understand right now, despite my efforts, there are a lot of places in my code that may have such issues. I will keep working on those.