feat(d1): add smhc driver #299
Conversation
+ change *reset* function parameter to be `assert` instead of `deassert` which makes more sense for a user
- `get_cid` - `get_rca` - `select` - `set_wide_bus`
| } => { | ||
| tracing::trace!("setting buf len: {cnt}"); | ||
| // TODO: Safety? | ||
| unsafe { buf.as_vec_mut().set_len(*cnt) }; |
There was a problem hiding this comment.
One downside of using a FixedVec: having to manually set the length after you read the data.
| // By declaring them in this scope the descriptor memory will live long enough, | ||
| // however currently this is up to the user to guarantuee. | ||
| // Safety: a zero'ed descriptor is valid and will simply be ignored by the IDMAC. | ||
| let mut descriptors: [idmac::Descriptor; 16] = unsafe { core::mem::zeroed() }; |
There was a problem hiding this comment.
I'd like to improve this, but don't know if dynamically allocating memory for descriptors is the way to go.
There was a problem hiding this comment.
hmm...we could, maybe, have a pool of descriptors owned by the SMHC driver, using mnemos_bitpool? this could be similar to how we allocate descriptors in the regular DMAC code.
but, if this works for now, it seems fine to me.
There was a problem hiding this comment.
I'm going to look at the regular DMAC code and try to do things the same way for this internal DMAC.
There was a problem hiding this comment.
Hey @hawkw, could you point me in the right direction regarding the way descriptors are allocated for the regular DMAC? I don't find references to mnemos_bitpool in the code and from what I can see the two current DMAC users (uart + spim) have enough with 1 descriptor per transfer (whereas here the number of required descriptors depends on the size of the transfer).
There was a problem hiding this comment.
The code I was referring to is the DMAC channel allocator in d1_core::dmac, particularly the Dmac::claim_channel method and friends. I think a somewhat similar pattern could be used here, potentially.
I probably wouldn't worry about that right now though. If the current solution works, this might be worth saving for a subsequent PR.
There was a problem hiding this comment.
This is extremely cool, thanks for putting so much work into it! Overall, it looks great!
I commented on some questions and minor suggestions I had while reading through the code. In general, though, this is great work and fits into the idiom for mnemOS drivers really nicely!
| pub async fn register( | ||
| self, | ||
| kernel: &'static Kernel, | ||
| queued: usize, | ||
| ) -> Result<(), registry::RegistrationError> { | ||
| let rx = kernel | ||
| .registry() | ||
| .bind_konly::<SdmmcService>(queued) | ||
| .await? | ||
| .into_request_stream(queued) | ||
| .await; | ||
|
|
||
| kernel.spawn(self.run(rx)).await; | ||
| tracing::info!("SMHC driver task spawned"); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
this registers the SMHC controller as an instance of the SdmmcService. Is this correct for all the SMHC peripherals, or just SMHC0/SMHC2? would it make sesnse to have separate wrapper types around the Smhc struct for each separate SMHC peripheral, and put the actual service implementations on those wrappers, so that they can implement different services? or, does it make sense to use the SdmmcService for all the hardware devices controlled by the SMHC peripherals?
sorry if this is a dumb question! i'm not super familiar with the different types of SMHC device and whether it would make more sense to have different service interfaces for them or not...
There was a problem hiding this comment.
Difficult to answer right now. Once we get to implementing SDIO or MMC we should have a better idea if everything can be done in 1 SdmmcService or we need to make a split. At that point we can do some refactoring if necessary.
source/kernel/src/services/sdmmc.rs
Outdated
| } | ||
|
|
||
| /// Get the relative card address | ||
| pub async fn get_rca(&mut self) -> Result<u32, Error> { |
There was a problem hiding this comment.
i wonder if we should consider adding a Rca(u32) or RelativeCardAddress(u32) newtype, so that the user can only call methods like select and, set_wide_bus, read, etc, with real RCAs they obtained from this method, rather than random u32s that might not index a real card?
| } | ||
|
|
||
| /// A client for SDIO cards using the [`SdmmcService`]. | ||
| pub struct SdioClient { |
There was a problem hiding this comment.
let's maybe add
| pub struct SdioClient { | |
| #[allow(dead_code)] | |
| pub struct SdioClient { |
on this for now so that clippy stops whining about it?
| &mut self, | ||
| sector: u32, | ||
| blocks: usize, | ||
| buf: FixedVec<u8>, |
There was a problem hiding this comment.
if reads are always block-oriented, would it make sense to take a FixedVec<[u8; 512]> here, rather than a FixedVec<u8>? or do you think that just makes things more painful for the user of the API?
| buf: FixedVec<u8>, | ||
| ) -> Result<FixedVec<u8>, Error> { |
There was a problem hiding this comment.
also, not something to address in this PR, but something that i was thinking about doing for all driver impls that consume a buffer to read/write into/from could maybe return it in their error type if the read/write failed (the i2c driver could also do this). this way, the user of the API can continue reusing their allocation, even if a particular read/write op failed.
we could make this change later though.
|
I've been testing mostly with a lichee rv SOM, but have recently also started using a mango pi mq pro. |
Sounds good, thanks for testing with both boards! <3 |
| last, | ||
| buff_size, | ||
| buff_addr: unsafe { buf_ptr.add(index * DESCR_BUFF_SIZE) }, | ||
| next_desc: (!last).then_some(&descriptors[index + 1] as *const _ as _), |
There was a problem hiding this comment.
hmm, can this bounds check fail if we're on the last descriptor? should we maybe be using
| next_desc: (!last).then_some(&descriptors[index + 1] as *const _ as _), | |
| next_desc: (!last).then(|| &descriptors[index + 1] as *const _ as _), |
so that the indexing only occurs if last is false?
| if reset == BusReset::Assert { | ||
| w.$reset().clear_bit() | ||
| } else { | ||
| w.$reset().set_bit() | ||
| } |
There was a problem hiding this comment.
nit, take it or leave it: this could still be a one-liner if we wrote
| if reset == BusReset::Assert { | |
| w.$reset().clear_bit() | |
| } else { | |
| w.$reset().set_bit() | |
| } | |
| w.$reset.bit(reset == BusReset::Deassert); |
| if gating == BusGating::Mask { | ||
| w.$gating().clear_bit() | ||
| } else { | ||
| w.$gating().set_bit() | ||
| } |
There was a problem hiding this comment.
nit, take it or leave it: this could still be a one-liner if we wrote
| if gating == BusGating::Mask { | |
| w.$gating().clear_bit() | |
| } else { | |
| w.$gating().set_bit() | |
| } | |
| w.$gating.bit(gating == BusGating::Pass); |
| // Configure the burst size and TX/RX trigger level | ||
| self.smhc.smhc_fifoth.write(|w| { | ||
| w.tx_tl().variant(8); | ||
| w.rx_tl().variant(7); |
There was a problem hiding this comment.
minor nit: can we add what burst size and trigger levels were selecting to the comment? it's not immediately clear what "8" and "7" are saying here — is that the actual value of the trigger level or is it selecting something else?
| // Reset the FIFO | ||
| self.smhc.smhc_ctrl.modify(|_, w| w.fifo_rst().reset()); | ||
| while self.smhc.smhc_ctrl.read().fifo_rst().is_reset() { | ||
| core::hint::spin_loop(); | ||
| } |
There was a problem hiding this comment.
very minor nit, not actually important: we have very similar code in Smhc::init, would it make sense to factor this out into a
fn reset_fifo(regs: &smhc::RegisterBlock) {
// ...
}or something?
| w.dma_rst().set_bit(); | ||
| w | ||
| }); | ||
| while self.smhc.smhc_ctrl.read().dma_rst().bit_is_set() {} |
There was a problem hiding this comment.
should this loop have a spin loop hint in it, like we do elsewhere?
| while self.smhc.smhc_ctrl.read().dma_rst().bit_is_set() {} | |
| while self.smhc.smhc_ctrl.read().dma_rst().bit_is_set() { | |
| core::hint::spin_loop(); | |
| } |
| return Err(sdmmc::Error::Data); | ||
| } | ||
|
|
||
| tracing::debug!("Creating descriptor chain from buffer"); |
There was a problem hiding this comment.
nit, take it or leave it: maybe worth including the descriptor count?
| tracing::debug!("Creating descriptor chain from buffer"); | |
| tracing::debug!(cnt, "Creating descriptor chain from buffer"); |
| } | ||
| State::WaitForDma => { | ||
| // TODO: better way to check for RX and TX, | ||
| // *normal interrupt summary* does not seem to work |
There was a problem hiding this comment.
out of curiousity, what does this mean exactly?
There was a problem hiding this comment.
In the IDMAC interrupt status register has the normal interrupt summary as bit 8, which is supposed to be set when either of the RX or TX interrupt bits are set. That would be convenient to check if the operation we are waiting for has finished.
However, it doesn't work and the bit is never set.
Maybe the documentation of the interrupt enable register isn't complete in the manual version I have (v1.0).
If you have the newer version, could you let me know if SMHC_IDIE_REG has a bit 8 that can be set? 😄
There was a problem hiding this comment.
i checked v1.4 of the D1 manual, and the table for SMHC_IDIE_REG lists the following bits:
| Bit | Name |
|---|---|
| 0 | TX_INT_ENB |
| 1 | RX_INT_ENB |
| 2 | FERR_INT_ENB |
| 4 | DES_UNAVL_INT_ENB |
| 5 | ERR_SUM_INT_ENB |
Bit 3 and bits 6-31 are not listed in the table in that version of the manual. I assume this means that the "normal interrupt summary" bit is not actually real?
There was a problem hiding this comment.
Ah, it looks like v1.4 of the manual does discuss a NOR_INT_SUM ("Normal Interrupt Summary") bit, but it's bit 8 of the SMHC_IDST_REG register, rather than SMHC_IDIE_REG.
The documentation for that bit says the following:
NOR_INT_SUM (NIS)
Normal Interrupt Summary
Logical OR of the following:
IDST[0]: Transmit Interrupt
IDST[1]: Receive InterruptOnly unmasked bits affect this bit.
This is a sticky bit and must be cleared each time a corresponding
bit that causes NIS to be set is cleared.
Hope that's helpful!
There was a problem hiding this comment.
Ah okay, it's the same for the v1.0 manual. The IDST is the status register while the IDIE is the enable register.
Since the IDST talks about bit 8, but I don't see it getting set, I was wondering if it had to be enabled as well, and that the corresponding bit in the IDIE was missing in my manual version. But it doesn't seem so.
There was a problem hiding this comment.
Oh, I see. Hmm. The Allwinner docs are....not great... I wonder if they're trying to say that you need to be writing it back every time we read from the IDST reg to ensure it gets set on the next IRQ?
| } | ||
| State::WaitForDataTransfer => { | ||
| if smhc.smhc_mintsts.read().m_dtc_int().bit_is_set() { | ||
| smhc.smhc_rintsts.write(|w| w.dtc().set_bit()); |
There was a problem hiding this comment.
should this be a modify instead of a write? the write will set the register to only have the DTC bit set, is that what we want? or do we only want to touch that bit and leave the others unchanged?
if we do want to clear every other bit, a comment stating that could be nice...
There was a problem hiding this comment.
This is indeed a bit counterintuitive, but many of the interrupt registers in the D1 are "write 1 to clear".
Which means that this write sets only the DTC bit back to 0 and the rest are left untouched.
Using modify here would actually reset other bits that are also set, which is undesirable.
I'll add a general comment about it in the advance_isr function.
There was a problem hiding this comment.
cool, thank you! let's definitely add the comment.
in other drivers, we do generally use modify, as it will write back every bit that's currently 1 as a 1, which should be sufficient to clear the interrupt I think --- cc @jamesmunns
| mod idmac { | ||
| #[repr(C, align(4))] | ||
| #[derive(Default, Debug)] | ||
| pub struct Descriptor { |
There was a problem hiding this comment.
nit, TIOLI: this struct (and everything else in the idmac module) really only needs to be
| pub struct Descriptor { | |
| pub(super) struct Descriptor { |
since it's never re-exported...
I think my mango pi might already be busted DRAM only have internal ZQ!!
get_pmu_exist() = 4294967295
ddr_efuse_type: 0x0
[AUTO DEBUG] two rank and full DQ!
ddr_efuse_type: 0x0
[AUTO DEBUG] rank 0 row = 16
[AUTO DEBUG] rank 0 bank = 8
[AUTO DEBUG] rank 0 page size = 8 KB
[AUTO DEBUG] rank 1 row = 15
[AUTO DEBUG] rank 1 bank = 8
[AUTO DEBUG] rank 1 page size = 2 KB
rank1 config different from rank0
DRAM BOOT DRIVE INFO: %s
DRAM CLK = 792 MHz
DRAM Type = 3 (2:DDR2,3:DDR3)
DRAMC ZQ value: 0x7b7bfb
DRAM ODT value: 0x42.
ddr_efuse_type: 0x0
DRAM SIZE =4608 M
DRAM simple test FAIL.
fedcc098 != fedcba98 at address d0000000 |
welp. that seems bad... |
No luck trying to revive it. But in the meantime I've refactored the idmac code to also use a builder and bitfields. |
| const _RESERVED_0 = 1; | ||
|
|
||
| /// Disable interrupts on completion. | ||
| const CUR_TXRX_OVER_INT_DIS = 1; | ||
|
|
||
| /// When set to 1, this bit indicates that the buffers this descriptor points to | ||
| /// are the last data buffer. | ||
| const LAST_FLAG = 1; | ||
|
|
||
| /// When set to 1, this bit indicates that this descriptor contains | ||
| /// the first buffer of data. It must be set to 1 in the first DES. | ||
| const FIRST_FLAG = 1; | ||
|
|
||
| /// When set to 1, this bit indicates that the second address in the descriptor | ||
| /// is the next descriptor address. It must be set to 1. | ||
| const CHAIN_MOD = 1; | ||
|
|
||
| /// Bits 29:5 are reserved. | ||
| const _RESERVED_1 = 25; | ||
|
|
||
| /// When some errors happen in transfer, this bit will be set to 1 by the IDMAC. | ||
| const ERR_FLAG = 1; | ||
|
|
||
| /// When set to 1, this bit indicates that the descriptor is owned by the IDMAC. | ||
| /// When this bit is reset, it indicates that the descriptor is owned by the host. | ||
| /// This bit is cleared when the transfer is over. | ||
| const DES_OWN_FLAG = 1; |
There was a problem hiding this comment.
nit, take it or leave it: i would probably use bool for the various 1-bit flags here, rather than a single-bit numeric bitfield:
| const _RESERVED_0 = 1; | |
| /// Disable interrupts on completion. | |
| const CUR_TXRX_OVER_INT_DIS = 1; | |
| /// When set to 1, this bit indicates that the buffers this descriptor points to | |
| /// are the last data buffer. | |
| const LAST_FLAG = 1; | |
| /// When set to 1, this bit indicates that this descriptor contains | |
| /// the first buffer of data. It must be set to 1 in the first DES. | |
| const FIRST_FLAG = 1; | |
| /// When set to 1, this bit indicates that the second address in the descriptor | |
| /// is the next descriptor address. It must be set to 1. | |
| const CHAIN_MOD = 1; | |
| /// Bits 29:5 are reserved. | |
| const _RESERVED_1 = 25; | |
| /// When some errors happen in transfer, this bit will be set to 1 by the IDMAC. | |
| const ERR_FLAG = 1; | |
| /// When set to 1, this bit indicates that the descriptor is owned by the IDMAC. | |
| /// When this bit is reset, it indicates that the descriptor is owned by the host. | |
| /// This bit is cleared when the transfer is over. | |
| const DES_OWN_FLAG = 1; | |
| const _RESERVED_0 = 1; | |
| /// Disable interrupts on completion. | |
| const CUR_TXRX_OVER_INT_DIS = 1; | |
| /// When set to 1, this bit indicates that the buffers this descriptor points to | |
| /// are the last data buffer. | |
| const LAST_FLAG: bool; | |
| /// When set to 1, this bit indicates that this descriptor contains | |
| /// the first buffer of data. It must be set to 1 in the first DES. | |
| const FIRST_FLAG: bool; | |
| /// When set to 1, this bit indicates that the second address in the descriptor | |
| /// is the next descriptor address. It must be set to 1. | |
| const CHAIN_MOD bool; | |
| /// Bits 29:5 are reserved. | |
| const _RESERVED_1 = 25; | |
| /// When some errors happen in transfer, this bit will be set to 1 by the IDMAC. | |
| const ERR_FLAG: bool; | |
| /// When set to 1, this bit indicates that the descriptor is owned by the IDMAC. | |
| /// When this bit is reset, it indicates that the descriptor is owned by the host. | |
| /// This bit is cleared when the transfer is over. | |
| const DES_OWN_FLAG: bool; |
stylistically, i would also probably drop _FLAG from these names, but that's up to you.
|
|
||
| pub fn disable_irq(self, val: bool) -> Self { | ||
| Self { | ||
| cfg: self.cfg.with(Cfg::CUR_TXRX_OVER_INT_DIS, val as u32), | ||
| ..self | ||
| } | ||
| } | ||
|
|
||
| pub fn first(self, val: bool) -> Self { | ||
| Self { | ||
| cfg: self.cfg.with(Cfg::FIRST_FLAG, val as u32), | ||
| ..self | ||
| } | ||
| } | ||
|
|
||
| pub fn last(self, val: bool) -> Self { | ||
| Self { | ||
| cfg: self.cfg.with(Cfg::LAST_FLAG, val as u32), | ||
| ..self | ||
| } | ||
| } |
There was a problem hiding this comment.
since this builder type is only used in one place, i'm not convinced that we need methods for these...we could just have the code that builds the descriptor call cfg.with(Cfg::CUR_TXRX_OVER_INT_DIS, ...).with(Cfg::FIRST_FLAG, ...)...
There was a problem hiding this comment.
Do you mean adding them as parameters to DescriptorBuilder::build?
pub fn build(self, disable_irq: bool, first: bool, last: bool) -> Descriptor {
Descriptor {
configuration: self.cfg.with(...).with(...), // set all the required flags
...
}
}Only taking self in build feels nicer though imo.
There was a problem hiding this comment.
oh, i was thinking we would just call the with methods on the cfg bitfield directly in the main driver code, making the cfg field pub(super).
i'm fine with the current approach, or with exposing the bitfield to the main driver code and having it call with directly. i have a slight preference against the approach you described where they're all passed as parameters to build, because then we have a function that takes three bools and there's no way for a reader seeing that function call to determine what any of them mean. seeing .first(true) or .with(Cfg::FIRST_FLAG, true), on the other hand, the reader knows what field is being set.
There was a problem hiding this comment.
I'll keep it like it is for now then, we could always later change it to expose the cfg field.
source/kernel/src/services/sdmmc.rs
Outdated
| Response::Short { .. } => Err(Error::Response), | ||
| Response::Long(value) => { | ||
| tracing::trace!("CMD2 response: {value:?}"); | ||
| let mut bytes = [0u8; 16]; |
There was a problem hiding this comment.
Hey @hawkw, just wanted to check with you if you know of a better way to convert the [u32; 4] to u128?
There was a problem hiding this comment.
i mean, they're laid out the same in memory so ideally the conversion should be a no-op...we could do a bunch of shifts and OR them all together, but that almost certainly won't compile down to a no-op.
the most efficient solution would be to use unsafe code (either mem::transmute or a union) to guarantee that the conversion is a no-op, but we need to write the unsafe word to do that. if we were going to go down the unsafe route, i would prefer a union personally, since it doesn't allow totally unrestricted type punning. something like:
Response::Long(value) => {
tracing::trace!("CMD2 response: {value:?}");
union CidResponse {
value: [u16; 4],
cid: u128,
}
let rsp = CidResponse { value };
unsafe {
rsp.cid
}
}although, on the other hand, using mem::transmute is significantly shorter.
it's up to you whether you think it's worth writing unsafe code for this. i don't think there's any danger in using unsafe here, but it's not strictly necessary. it does seem unfortunate to do a bunch of memcpys when the [u32; 4] and u128 should be exactly the same in memory, though...
There was a problem hiding this comment.
I'll just go ahead and use your union example, since there's no clear "best" way of doing it.
|
The PR should be ready now. I went through the comments again and they all should have been resolved/applied. Let me know if I missed something or in case of other remarks! |
|
hmm, CI failure looks kinda weird, i've never seen it do that before: https://github.com/tosc-rs/mnemos/actions/runs/7008859631/job/19066205659?pr=299#step:7:876 |
source/kernel/src/services/sdmmc.rs
Outdated
| union CidResponse { | ||
| value: [u32; 4], | ||
| cid: u128, | ||
| } | ||
| let rsp = CidResponse { value }; | ||
| Ok(CardIdentification(unsafe { rsp.cid })) |
There was a problem hiding this comment.
thinking about this a little more: i wonder if it would make more sense to just change Response::Long from [u32; 4] to u128? get_cid is the only method where we actually use the long response, and it wants to use the response as a u128, so...maybe we should just represent the data that way everywhere. i don't know how other code we might add in the future would consume these responses, but we could always turn the u128 into smaller integers there...
There was a problem hiding this comment.
Looking at this again, it indeed makes sense to let the hardware specific driver do the conversion.
This is currently [u32; 4] because in the D1 you need to retrieve 4 32-bit registers to reconstruct the "long" response, but this shouldn't be part of the service interface.
There was a problem hiding this comment.
yeah, i think that's the right thing --- from the perspective of the hardware-agnostic SdmmcService, it's just a 128-bit number, i think. how that's constructed from register values or whatever is up to the driver implementation.
| /// Manufacturing date [19:8] | ||
| /// CRC7 checksum [7:1] | ||
| /// Not used, always 1 [0:0] | ||
| pub struct CardIdentification(u128); |
There was a problem hiding this comment.
might still be nice to do this, but maybe save it for a follow-up PR?
source/kernel/src/services/sdmmc.rs
Outdated
|
|
||
| /// Errors returned by the [`SdmmcService`] | ||
| #[derive(Debug, Eq, PartialEq)] | ||
| pub enum Error { |
There was a problem hiding this comment.
nit, not a blocker: it could be nice to add a Display impl for this error type. also, we should maybe add
| pub enum Error { | |
| #[non_exhaustive] | |
| pub enum Error { |
so that we can add more errors in the future?
| ErrorKind::Response => sdmmc::Error::Response, | ||
| ErrorKind::ResponseCrc => sdmmc::Error::Response, | ||
| ErrorKind::DataCrc => sdmmc::Error::Data, | ||
| ErrorKind::ResponseTimeout => sdmmc::Error::Timeout, | ||
| ErrorKind::DataTimeout => sdmmc::Error::Timeout, | ||
| ErrorKind::DataStarvationTimeout => sdmmc::Error::Timeout, | ||
| ErrorKind::FifoUnderrunOverflow => sdmmc::Error::Data, | ||
| ErrorKind::CommandBusyIllegalWrite => sdmmc::Error::Busy, | ||
| ErrorKind::DataStart => sdmmc::Error::Data, | ||
| ErrorKind::DataEnd => sdmmc::Error::Data, | ||
| ErrorKind::Dma => sdmmc::Error::Other, | ||
| ErrorKind::Other => sdmmc::Error::Other, |
There was a problem hiding this comment.
hmm, it's a bit unfortunate that some potentially useful information is lost here when converting these ErrorKinds into sdmmc::Errors. for example, the user cannot determine whether a sdmmc::Error::Timeout represents a ResponseTimeout, a DataTimeout, or a DataStarvationTimeout.
what do you think about changing the sdmmc::Error type to something like this:
pub struct Error {
kind: sdmmc::ErrorKind,
message: Option<&'static str>,
}(where sdmmc::ErrorKind is the enum currently named sdmmc::Error).
this way, users can still programmatically match on the enum, representing a standardized enumeration of possible kinds of errors that a SDMMC driver may encounter, but there's also the ability for the driver implementation to include extra human-readable information that can be printed out by the user, such as "response timeout" vs "data timeout", or disambiguating "data start" and "data end", etc?
There was a problem hiding this comment.
Yeah, I like the idea. It's unfortunate that this
return Err(ErrorKind::Response)needs to become something like this then
return Err(Error { kind: ErrorKind::Response, message: None })or
return Err(Error::new(ErrorKind::Response))although I guess the last option isn't too bad.
source/kernel/src/services/sdmmc.rs
Outdated
| match self | ||
| .cmd(Command { | ||
| index: 6, | ||
| argument: 0b10, |
There was a problem hiding this comment.
For this command, you can use either 0b00 as argument, to tell the card to switch to 1-bit width or 0b10 to switch to 4-bit width. I added a comment to clarify.
I guess this set_wide_bus function should have been set_bus_width though, with an argument that specifies the width.
If it's okay for you, I'd prefer to postpone this change to a future PR though.
There was a problem hiding this comment.
i think adding a comment is fine for now --- if you want to change the function to set_bus_width in a follow up, i'm fine with that too!
source/kernel/src/services/sdmmc.rs
Outdated
| .await? | ||
| { | ||
| Response::Short { value, .. } => { | ||
| tracing::trace!("CMD8 response: {value:#x}"); |
There was a problem hiding this comment.
take it or leave it, not a big deal: a similar trace is included in each of the methods that send a command to the card...what do you think about adding this to the cmd method, instead, like:
tracing::trace!("CMD{index}: response {value:#}");so that we don't have to repeat it in every other method?
There was a problem hiding this comment.
That would indeed be better. However, I guess you would have to match on the response in cmd then?
Or derive Debug on Response, which is probably the easiest solution.
There was a problem hiding this comment.
🤷♀️ if it's too much extra work, i'm fine leaving this as is!
source/kernel/src/services/sdmmc.rs
Outdated
| Response::Long(_) => return Err(Error::Response), | ||
| } | ||
|
|
||
| // TODO: wait some time (e.g., 1ms?) |
There was a problem hiding this comment.
we do have a globally available kernel timer...
There was a problem hiding this comment.
Is this using maitake::time::sleep(...).await?
|
PR #300 fixes the CI issues, so updating from |
+ smhc: use transmute to convert [u32;4] to u128
jspngh
force-pushed
the
jspngh/smhc-driver
branch
from
7eb2b26 to
091961b
Compare
There was a problem hiding this comment.
this looks wonderful! i commented on some last very minor style suggestions i noticed while skimming your latest changes, but none of them are actually important. please go ahead and merge whenever you feel like this is ready, i'm really excited to get it merged! :D
source/kernel/src/services/sdmmc.rs
Outdated
| ErrorKind::Data => write!(f, "data "), | ||
| ErrorKind::Buffer => write!(f, "buffer "), | ||
| ErrorKind::Timeout => write!(f, "timeout "), | ||
| ErrorKind::Other => write!(f, "other "), |
There was a problem hiding this comment.
tiny style nit, not a blocker: i try to avoid using the write! macro when no formatted arguments are interpolated into the string. it's up to you whether you actually care about this, though...
| ErrorKind::Data => write!(f, "data "), | |
| ErrorKind::Buffer => write!(f, "buffer "), | |
| ErrorKind::Timeout => write!(f, "timeout "), | |
| ErrorKind::Other => write!(f, "other "), | |
| ErrorKind::Data => f.write_str("data "), | |
| ErrorKind::Buffer => f.write_str("buffer "), | |
| ErrorKind::Timeout => f.write_str("timeout "), | |
| ErrorKind::Other => f.write_str("other "), |
source/kernel/src/services/sdmmc.rs
Outdated
| Response::Short { value, data: _ } => write!(f, "{:#x}", value), | ||
| Response::Long(value) => write!(f, "{:#x}", value), |
There was a problem hiding this comment.
tiny style nit, feel free to ignore me:
| Response::Short { value, data: _ } => write!(f, "{:#x}", value), | |
| Response::Long(value) => write!(f, "{:#x}", value), | |
| Response::Short { value, data: _ } => write!(f, "{value:#x}"), | |
| Response::Long(value) => write!(f, "{value:#x}"), |
source/kernel/src/services/sdmmc.rs
Outdated
| .request_oneshot(cmd, &self.reply) | ||
| .await | ||
| .map_err(|error| { | ||
| tracing::warn!(?error, "failed to send request to SD/MMC service"); |
There was a problem hiding this comment.
nit: it could maybe be nice to include the command here as well:
| tracing::warn!(?error, "failed to send request to SD/MMC service"); | |
| tracing::warn!(?error, "failed to send CMD{index} request to SD/MMC service"); |
source/kernel/src/services/sdmmc.rs
Outdated
| Error::from(ErrorKind::Other) // TODO | ||
| }) | ||
| .and_then(|resp| resp.body); | ||
| tracing::trace!("CMD{} response: {:?}", index, result); |
There was a problem hiding this comment.
extremely unimportant style nit, feel free to ignore this:
| tracing::trace!("CMD{} response: {:?}", index, result); | |
| tracing::trace!("CMD{index} response: {result:?}"); |
source/kernel/src/services/sdmmc.rs
Outdated
| .await? | ||
| { | ||
| Response::Short { value, .. } => { | ||
| if value == 0x1AA { |
There was a problem hiding this comment.
style nit, not a blocker: should 0x1AA maybe be a const, since it's also the command index?
source/kernel/src/services/sdmmc.rs
Outdated
| // The provider buffer should have space for the requested amount of data | ||
| if buf.capacity() < 512 * blocks { | ||
| return Err(Error::from(ErrorKind::Buffer)); | ||
| } | ||
|
|
||
| match self | ||
| .cmd(Command { | ||
| index: 18, | ||
| argument: sector, | ||
| options: HardwareOptions::None, | ||
| kind: CommandKind::Read(512 * blocks), |
There was a problem hiding this comment.
nit, not a blocker: maybe:
| // The provider buffer should have space for the requested amount of data | |
| if buf.capacity() < 512 * blocks { | |
| return Err(Error::from(ErrorKind::Buffer)); | |
| } | |
| match self | |
| .cmd(Command { | |
| index: 18, | |
| argument: sector, | |
| options: HardwareOptions::None, | |
| kind: CommandKind::Read(512 * blocks), | |
| const BLOCK_SIZE: usize = 512; | |
| let bytes = BLOCK_SIZE * blocks; | |
| // The provider buffer should have space for the requested amount of data | |
| if buf.capacity() < bytes { | |
| return Err(Error::from(ErrorKind::Buffer)); | |
| } | |
| match self | |
| .cmd(Command { | |
| index: 18, | |
| argument: sector, | |
| options: HardwareOptions::None, | |
| kind: CommandKind::Read(bytes), |
This PR adds support for SD card interaction on the Allwinner D1 platform.
It is split in an
sdmmckernel service and a driver for the SD/MMC host controller peripheral.Currently only SD card support is implemented, but this should allow for adding MMC and SDIO support later.
The interface between the kernel service and the driver aims to work on the specification/protocol level
and not contain any Allwinner D1 details, which should make it suitable for other platforms as well.
This PR also improves the
BusGatingResetRegistertrait inplatforms/allwinner-d1/d1-core/src/ccu.rs