A minimal Rust based sdmmc driver for odroidc4 #273

Cheng-Li1 · 2024-10-21T04:24:31Z

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:

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)
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.
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 · 2024-10-21T05:02:14Z

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

midnightveil

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

midnightveil · 2024-10-21T05:11:09Z

drivers/blk/sdmmc/sdmmc_protocol/sdmmc.rs

+    pub async fn read_block(self, blockcnt: u32, start_idx: u64, destination: u64) -> (Result<(), SdmmcHalError>, Option<SdmmcProtocol<'a, T>>) {
+        let mut cmd: SdmmcCmd;
+        let mut res: Result<(), SdmmcHalError>;
+        // TODO: Figure out a way to support cards with 4 KB sector size


They don't exist, only legacy SD cards support block sizes not 512; any recent (i.e. anything you can buy these days and probably the past however long) is mandated to only support 512. Refer to the SD card specification.

I agree with that, the comment should be deleted as sdcard can support block size specified by user, except legacy sdcard.

Other way around -- modern SD cards can only support 512 sectors, legacy ones can specify any between 1-2048 (depending on the card status info max & min read/write lengths)

My mistake.

midnightveil · 2024-10-21T05:11:33Z

drivers/blk/sdmmc/sdmmc_protocol/sdmmc.rs

+pub const MMC_RSP_R7: u32 = MMC_RSP_PRESENT | MMC_RSP_CRC | MMC_RSP_OPCODE;
+
+
+/// Program async Rust can be very dangerous if you do not know what is happening understand the hood


I'm not sure what the point of comments like this are.

Remind myself what I am doing is how dangerous it is😅, should adjust that.

midnightveil · 2024-10-21T05:11:52Z

drivers/blk/sdmmc/sdmmc_hal/meson/lib.rs

@@ -0,0 +1,3 @@
+#![no_std]  // Don't link the standard library


This is obvious.

midnightveil · 2024-10-21T05:15:15Z

drivers/blk/sdmmc/sdmmc_hal/meson/meson_gx_mmc.rs

+const CLK_CO_PHASE_270: u32 = 3 << 8;
+const CLK_TX_PHASE_000: u32 = 0 << 10;
+const CLK_TX_PHASE_180: u32 = 2 << 10;
+const CLK_ALWAYS_ON: u32 = 1 << 24;


What's the point of using rust with it's nice data type support (i.e. ability to write good versions of bitfield structs) and then just using raw bitshifts?

I am planning to use some kind of bit list structure on the protocol layer. On the hardware layer, as I need to directly write to the hardware registers, I am wondering if it is necessary to rely on another crate for those low-level operations as the hardware layer is almost directly from uboot, would this bare metal way make it easier to move from legacy C to Rust?

midnightveil · 2024-10-21T05:16:16Z

drivers/blk/sdmmc/sdmmc_hal/meson/meson_gx_mmc.rs

+
+// STATUS register masks and flags
+const STATUS_MASK: u32 = 0xFFFF;               // GENMASK(15, 0)
+const STATUS_ERR_MASK: u32 = 0x1FFF;           // GENMASK(12, 0)


This is surely less understandable than the C version... no?

(You could write a const fn that is the equivalent of GENMASK, or use full data type)

midnightveil · 2024-10-21T05:45:08Z

drivers/blk/sdmmc/src/main.rs

+
+const INTERRUPT: sel4_microkit::Channel = sel4_microkit::Channel::new(1);
+
+const SDCARD_SECTOR_SIZE: u32 = 512;


You've defined this twice?

midnightveil · 2024-10-21T05:45:44Z

drivers/blk/sdmmc/src/main.rs

+    let meson_hal: &mut MesonSdmmcRegisters = MesonSdmmcRegisters::new();
+    let mut protocol: SdmmcProtocol<'static, MesonSdmmcRegisters> = SdmmcProtocol::new(meson_hal);
+    let mut irq_to_enable = InterruptType::Success as u32 | InterruptType::Error as u32;
+    // Should always succeed, at least for odroid C4


midnightveil · 2024-10-21T05:47:35Z

drivers/blk/sdmmc/src/main.rs

+                            // TODO: The MAX_BLOCK_PER_TRANSFER is got by hackily get the defines in hardware layer which is wrong, check that to get properly from protocol layer
+                            request.count_to_do = core::cmp::min(request.count as u32, sdmmc_hal::meson_gx_mmc::MAX_BLOCK_PER_TRANSFER);
+                            if let Some(sdmmc) = self.sdmmc.take() {
+                                self.future = Some(Box::pin(sdmmc.write_block(request.count_to_do as u32, request.block_number as u64 + request.success_count as u64, request.io_or_offset + request.success_count as u64 * SDCARD_SECTOR_SIZE as u64)));


Wait why are we allocating in the command processing loop?

midnightveil · 2024-10-21T05:48:32Z

drivers/blk/sdmmc/src/sddf_blk/mod.rs

+    pub io_or_offset: u64,
+    pub block_number: u32,
+    pub count: u16,
+    // I suggest use u32 here and change the count to use u32 in sddf_blk


2 billion * 4096 bytes is a lot of data?

I think in more recent versions of sdcard and sd host, they try to support 32 bit block count? And setting the block count to 32 bit should not cause any issue for host or card that does not support 32 bit block count as well.

midnightveil · 2024-10-21T05:49:09Z

drivers/blk/sdmmc/src/sddf_helper.c

+
+uint8_t blk_enqueue_resp_helper(uint8_t status, uint16_t success, uint32_t id) {
+    // It would be better if we do not use int but use int8_t
+    if (blk_enqueue_resp(queue_handle, status, success, id) == 0) {


Why does this do the same as the inner functions?

It's most likely because bindgen can't convert the header and so we have to have some wrapper functions.

I'm not too much of a fan of this though, we should just be able to have an object that only includes <sddf/blk/queue.h> etc for the Rust bindings.

rust-lang/rust-bindgen#2335 ?

I have not had much luck with header-only APIs and bindgen in the past.

@Cheng-Li1 did you try bindgen before adding these wrappers?

No, I admit I have never used this before. But I guess if I use this bindgen, Ineed to keep track of the queue_handle in the Rust code?

What would be the problem with that?

Cheng-Li1 · 2024-10-21T06:20:06Z

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 · 2024-10-21T06:45:21Z

drivers/blk/sdmmc/sdmmc_hal/meson/meson_gx_mmc.rs

+        if clock_freq > 16000000 {
+            clk = SD_EMMC_CLKSRC_DIV2;
+            clk_src = CLK_SRC_DIV2;
+        } else {
+            clk = SD_EMMC_CLKSRC_24M;
+            clk_src = CLK_SRC_24M;
+        }


In Rust, if statements are expressions:

let (clk, clk_src) = if frequency > 16_000_000 { (SD_EMMC_CLKSRC_DIV2, CLK_SRC_DIV2) } else { (SD_EMMC_CLKSRC_24M, CLK_SRC_24M) };

midnightveil · 2024-10-21T06:47:03Z

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.

midnightveil · 2024-10-21T06:49:59Z

drivers/blk/sdmmc/src/main.rs

+}
+
+#[protection_domain(heap_size = 0x10000)]
+fn init() -> HandlerImpl<'static, MesonSdmmcRegisters> {


Nowhere in this initialisation process do you actually follow the specification for initialising the SD card?

I guess if you assume that U-Boot has done so already, but this isn't always the case, especially on platforms like the iMX8 where it doesn't have to boot off the SD card (or has multiple MMCs - eMMC and SD card). My driver does this.

That is what I have said in the note, as I cannot control the voltage. I will not be able to init the card by myself. If someone boots the system from an eMMC but want to use this driver to use sdcard, it will not work because I do not have any way to control the voltage to boot the sdcard.

I'm not talking about the voltages et al, I'm talking about the command sending flow.

There's a whole initialisation sequence for sending commands? Refer to my driver

Without it you can't initialise the SD layer on secondary MMC devices, even if the voltage is turned on (which at least for the imx8 it is).

(Also, hopefully this should support hotplug, where that's definitely necessary)

The init of the card is easy to implement, providing I can control the voltage in the hardware layer, which I cannot do right now. For reset, it is also easy to implement but unnecessary because the card is already in sdcard high speed mode, reset does not have any benefits as I know.

I'm not talking about the voltages et al, I'm talking about the command sending flow.

There's a whole initialisation sequence for sending commands? Refer to my driver

Without it you can't initialise the SD layer on secondary MMC devices, even if the voltage is turned on (which at least for the imx8 it is).

(Also, hopefully this should support hotplug, where that's definitely necessary)

No need to worry about hotplugging right now. I am pretty sure it would be supported in the future. If people really want to have reset even when doing it makes no obvious benefits, I can implement that as well.

reset even when doing it makes no obvious benefits

We should not be relying on U-Boot to reset the device properly. We should be doing device reset ourselves.

Sure, but let me provide a bit more context about this. The card setup function is always on my plan as to improve the read/write speed to above 100MB/s, a card setup would be definitely be needed. I was planning to add the card setup at the second iteration, when we have some way to change voltage. But since we do not want to completely rely on uboot set up the device for us, I can do it ahead. However, one thing to understand is that, whether I implement it or not we are all relying on uboot to set up the voltage, so we are still depending on uboot to set up the device, more or less.

Cheng-Li1 · 2024-10-21T06:50:03Z

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 · 2024-10-21T07:17:33Z

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 · 2024-10-21T07:49:07Z

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 · 2024-10-21T10:08:29Z

drivers/blk/sdmmc/support/targets/aarch64-sel4-microkit-minimal.json

I hope we don't need to store this in the repository, I'll talk to Nick but if we do it should not be in the driver code.

Maybe in board folder?

Ivan-Velickovic · 2024-10-21T10:13:35Z

drivers/blk/sdmmc/sdmmc_driver.mk

+	$(CC) -c $(CFLAGS) $< -o $@
+
+blk/sdmmc/meson/libsddfblk.a: blk/sdmmc/meson/sddf_helper.o |blk/sdmmc/meson
+	ar rcs $@ $<


Suggested change

ar rcs $@ $<

$(AR) rcs $@ $<

Ivan-Velickovic · 2024-10-21T10:15:38Z

drivers/blk/sdmmc/src/main.rs

+const INTERRUPT: sel4_microkit::Channel = sel4_microkit::Channel::new(1);
+
+const SDCARD_SECTOR_SIZE: u32 = 512;
+const SDDF_TRANSFER_SIZE: u32 = 4096;


Shouldn't be defined here, should be part of the bindings.

Ivan-Velickovic · 2024-10-21T10:16:04Z

drivers/blk/sdmmc/src/main.rs

+const SDDF_TRANSFER_SIZE: u32 = 4096;
+const SDDF_TO_REAL_SECTOR: u32 = SDDF_TRANSFER_SIZE / SDCARD_SECTOR_SIZE;
+
+const RETRY_CHANCE: u16 = 5;


Can you explain this retries thing?

When sdcard encounters an error, like time out, crc error or other kinds of error, the sdmmc driver should retry a few times before reporting back an error in case the error is only temporary. I think uboot does this as well.

Ivan-Velickovic · 2024-10-21T10:17:47Z

.gitignore

@@ -15,3 +15,5 @@ ci_build/
 .#*
 zig-out/
 .zig-cache/
+target/
+Cargo.lock


We should commit the lockfiles for reproducibility. Is that right @midnightveil ?

Yep. Though if we end up with a lock file per example it might bloat the git history/reposize a lot.

Ivan-Velickovic · 2024-10-21T10:19:22Z

drivers/blk/sdmmc/build.rs

Do we need this? Can we keep all the building to the Makefile?

This is the easiest way if we are building through cargo I believe (annoyingly), though you could build everything by invoking rustc directly (this would make it harder to pull in dependencies such as sel4-rust though)

https://public-docs.ferrocene.dev/main/user-manual/rustc/executable.html

(build.rs isn't the build script it just modifies how cargo builds. I think you can do most of this with environment variables but...)

In the rust-sel4 examples I don't see a build.rs for every crate

From memory when I used the rust-sel4 project, I don't think I needed a build.rs either

I suspect this is because Cheng wanted to link against the sddf helper wrappers -- It's only because it wants to modify the link paths to link against that binary.

I suspect most of the rust-sel4 projects don't explicitly link against C code in that way.

Yeah, I had to do pretty much the same thing in SMOS when linking against the sDDF and couldn't find a better way of doing it. IMO it's not a big deal to have it.

Ivan-Velickovic · 2024-10-21T11:15:08Z

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 · 2024-10-21T23:04:04Z

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 · 2024-10-21T23:25:58Z

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 · 2024-10-21T23:32:36Z

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.

alwin-joshy

Just a few inital comments. don't fully understand the async stuff yet so will need to play around with it a little.

alwin-joshy · 2024-10-21T06:12:29Z

drivers/blk/sdmmc/Cargo.toml

+edition = "2021"
+
+[dependencies]
+sel4-microkit = { git = "https://github.com/seL4/rust-sel4.git", rev = "d3790bfd15512e18659f9491c319867fabf9552d", package = "sel4-microkit" }


Should this be pinned to a particular commit?

I see the advantage that it means it will build more deterministically, but if we do end up including the Carfo.lock, I think we should just point it to main so there's less chance it gets out of date.

Also, I think the package = "sel4-microkit" isn't needed.

Right😂, package = "sel4-microkit" is not needed as cargo will look for the right package on its own. At that time when I am writing Cargo.toml I did not know.

alwin-joshy · 2024-10-21T06:12:55Z

drivers/blk/sdmmc/build.rs

+    // Specify the path where the C library is located
+    println!("cargo:rustc-link-search=native=./");
+
+    // Link the C library (static or dynamic). Adjust "static" or "dylib" as needed.


Comment could be improved

alwin-joshy · 2024-10-21T06:13:17Z

drivers/blk/sdmmc/build.rs

+    println!("cargo:rustc-link-lib=static=sddfblk");
+
+    // If you need to specify the include directory for C headers:
+    // println!("cargo:include=path/to/your/c/include");


Just delete it if you don't need it.

alwin-joshy · 2024-10-21T06:14:34Z

drivers/blk/sdmmc/sdmmc_driver.mk

+sdmmc_driver.elf: $(BUILD_DIR) blk/sdmmc/meson/libsddfblk.a
+	cp blk/sdmmc/meson/libsddfblk.a $(abspath ${SDMMC_DRIVER_DIR})
+	@cd $(abspath ${SDMMC_DRIVER_DIR}) && \
+	echo "Building sdmmc_driver.elf for board $(MICROKIT_BOARD)..." && \


Probably don't want debug prints in the Make recipe

alwin-joshy · 2024-10-21T06:40:47Z

drivers/blk/sdmmc/sdmmc_hal/meson/meson_gx_mmc.rs

+const IRQ_TXD_ERR: u32 = 1 << 8;
+const IRQ_DESC_ERR: u32 = 1 << 9;
+const IRQ_RESP_ERR: u32 = 1 << 10;
+// Equivalent to (IRQ_RXD_ERR_MASK | IRQ_TXD_ERR | IRQ_DESC_ERR | IRQ_RESP_ERR)


Unnecessary comment

alwin-joshy · 2024-10-21T23:32:23Z

drivers/blk/sdmmc/build.rs

Yeah, I had to do pretty much the same thing in SMOS when linking against the sDDF and couldn't find a better way of doing it. IMO it's not a big deal to have it.

alwin-joshy · 2024-10-21T23:36:49Z

drivers/blk/sdmmc/sdmmc_protocol/sdmmc.rs

+
+/// Program async Rust can be very dangerous if you do not know what is happening understand the hood
+/// Power up and power off cannot be properly implemented if I do not have access to control gpio/ regulator and timer
+pub trait SdmmcHardware {


It's probably so that a concrete implementation can implement a susbet of the full functionality of the trait, right?

Not clear if this is something that we actually want or not...

alwin-joshy · 2024-10-21T23:37:38Z

drivers/blk/sdmmc/sdmmc_protocol/sdmmc.rs

+    }
+
+    // TODO: Change it to use the sleep function provided by the hardware layer
+    let mut retry: u32 = 1000000;


This is a lot of retries... Is there a particular reason you chose this number?

This is a busy poll retry, we could poll infinitely if we trust the device to be correct. I will add a comment to explain this.

Not clear if this is something that we actually want or not...

From the designer's perspective, not all the functions defined in traits will need to be implemented for the driver to work. There will be a function to ask for the hardware layer to provide all of its abilities and the protocol layer to determine if those satisfy the minimal requirement to get the driver working and if so, protocol layer try to achieve the maximum speed possible based on the capabilities provided. I would hope to have more clarification on this one.

midnightveil · 2024-10-22T00:04:06Z

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 · 2024-10-22T00:25:52Z

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.

…m impacting my fileio compiling Signed-off-by: Cheng <[email protected]>

Signed-off-by: Cheng <[email protected]>

Signed-off-by: Ivan-Velickovic <[email protected]>

Signed-off-by: Cheng <[email protected]>

…ersion of sdmmc library on my repo(temporary and only for testing) Signed-off-by: Cheng <[email protected]>

Signed-off-by: Cheng <[email protected]>

Cheng-Li1 requested review from Ivan-Velickovic, alwin-joshy, erichchan999 and midnightveil October 21, 2024 04:58

midnightveil reviewed Oct 21, 2024

View reviewed changes

Cheng-Li1 force-pushed the sdmmc_rust branch from a8080c6 to 228a8c8 Compare October 21, 2024 06:04

midnightveil reviewed Oct 21, 2024

View reviewed changes

Ivan-Velickovic reviewed Oct 21, 2024

View reviewed changes

alwin-joshy reviewed Oct 21, 2024

View reviewed changes

Cheng-Li1 force-pushed the sdmmc_rust branch from 8393993 to 5e13b25 Compare October 22, 2024 05:52

Cheng-Li1 added 2 commits October 22, 2024 16:54

SDMMC: Recreated this branch for breaking changes in network subsyste…

f8e8385

…m impacting my fileio compiling Signed-off-by: Cheng <[email protected]>

sdmmc: experimentally implement polling based on interrupt

c55e256

Signed-off-by: Cheng <[email protected]>

Cheng-Li1 and others added 6 commits October 22, 2024 16:54

sdmmc: Can only get the first interrupt

7fb0931

Signed-off-by: Cheng <[email protected]>

sdmmc: Interrupt is working, but the code definitely need some polishing

3f7f8ff

Signed-off-by: Cheng <[email protected]>

sdmmc: Add support for blk example on odroidc4

4c40e85

Signed-off-by: Cheng <[email protected]>

sdmmc: some clean up before pull request

aec5d6b

Signed-off-by: Cheng <[email protected]>

ci: add odroidc4 for blk example

fe41ae9

Signed-off-by: Ivan-Velickovic <[email protected]>

sdmmc: update to latest sddf

0581e17

Signed-off-by: Cheng <[email protected]>

Cheng-Li1 force-pushed the sdmmc_rust branch from 5e13b25 to 0581e17 Compare October 22, 2024 05:55

Cheng-Li1 added 5 commits December 2, 2024 12:19

sdmmc: Update blk_benchmark client and point sdmmc driver to latest v…

e61cda3

…ersion of sdmmc library on my repo(temporary and only for testing) Signed-off-by: Cheng <[email protected]>

sdmmc: upload the files I forgot to upload

52ca07e

Signed-off-by: Cheng <[email protected]>

Update rev

14de216

Signed-off-by: Cheng <[email protected]>

sdmmc: update rev

a2be6e1

Signed-off-by: Cheng <[email protected]>

sdmmc: change the delay to spinning

fdd240f

Signed-off-by: Cheng <[email protected]>

		pub const MMC_RSP_R7: u32 = MMC_RSP_PRESENT \| MMC_RSP_CRC \| MMC_RSP_OPCODE;


		/// Program async Rust can be very dangerous if you do not know what is happening understand the hood

		@@ -0,0 +1,3 @@
		#![no_std] // Don't link the standard library


		const INTERRUPT: sel4_microkit::Channel = sel4_microkit::Channel::new(1);

		const SDCARD_SECTOR_SIZE: u32 = 512;

A minimal Rust based sdmmc driver for odroidc4 #273

Are you sure you want to change the base?

A minimal Rust based sdmmc driver for odroidc4 #273

Conversation

Cheng-Li1 commented Oct 21, 2024 • edited Loading

alwin-joshy commented Oct 21, 2024

midnightveil left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cheng-Li1 commented Oct 21, 2024

Choose a reason for hiding this comment

midnightveil commented Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

midnightveil Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cheng-Li1 commented Oct 21, 2024

Cheng-Li1 commented Oct 21, 2024

midnightveil commented Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

midnightveil Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

midnightveil Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivan-Velickovic commented Oct 21, 2024

Cheng-Li1 commented Oct 21, 2024

Cheng-Li1 commented Oct 21, 2024

Cheng-Li1 commented Oct 21, 2024

alwin-joshy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

midnightveil commented Oct 22, 2024

Cheng-Li1 commented Oct 22, 2024

Cheng-Li1 commented Oct 21, 2024 •

edited

Loading

midnightveil left a comment •

edited

Loading

midnightveil commented Oct 21, 2024 •

edited

Loading

midnightveil Oct 21, 2024 •

edited

Loading

midnightveil commented Oct 21, 2024 •

edited

Loading

midnightveil Oct 21, 2024 •

edited

Loading

midnightveil Oct 21, 2024 •

edited

Loading