Usdhc1 support#186
Conversation
|
This is dependent on tacertain/imxrt-usdhc#1 |
mciantyre
left a comment
There was a problem hiding this comment.
We can put the uSDHC clock setup in the BSP. What's the reason for picking that 198MHz frequency? Back when I did my driver testing, I was at 4MHz for a root clock, eventually divided down to under 400KHz for the functional clock. But it's been a while since I thought about SD cards, so I don't remember the protocol requirements right now.
We can take the pin muxing too. Anything else that needs to happen? High-Z or pulls on the pins? Otherwise I think we're using whatever the reset states are.
I've explored and tested a driver like this before. If I deeply reviewed your option, is there anything novel that I might learn?
From a high-level review, here are my notes:
The SD card state machine and the transport -- how the I/O happens -- are coupled. I've seen this in other drivers, meaning everyone writes the same CMD0 → CMD8 → ACMD41 → CMD2 → CMD3 → CMD9 → CMD7 kind of thing in their driver. Although incomplete, my approach tries to separate the state machine from the transport. It's a bigger lift, but if it works, I think we benefit more of the embedded Rust ecosystem.
I'm not sure why many blocks of the implementation are unsafe. If it's superficial, I would be concerned that it's hiding the meaningful unsafe blocks.
My driver's a prototype, so I'm not integrating it into this BSP anytime soon. Let's figure out an approach where folks can select their own uSDHC driver. Your example can become an example in your driver package.
|
Thanks for the high-level comments. My responses: 1/ On the clock, the cards support up to 25MHz for normal speed or 50MHz for high speed, so that's why I started with the (almost) 200MHz clock. 2/ On the state-machine separation, that was indeed something I saw in your code and I agree it's a good idea. I will be adopting it. 3/ On the unsafe blocks, I'm still in the "barely reformed C programmer" camp when it comes to Rust, so I'm still sometimes getting stuff to work without completely absorbing all the implications. I will dig in more here. 4/ Overall, I doubt you'd learn anything. The only reason to review it is if you thought you wanted to pull it into imxrt-rs. And it would of course help me reform more of my C instincts! But my goal is to be helpful, not to make work for you. Thanks again for the response. |
|
On the pins, I'm just taking what imxrt-iomuxc prepare sets up for usdhc, which seems fine. Though it looks like my driver might want to switch DAT3 to pull-up after card detection |
78f41fd to
76a51eb
Compare
| ral::modify_reg!(ral::ccm, ccm, CSCMR1, USDHC1_CLK_SEL: 0); // PLL2_PFD2 | ||
| ral::modify_reg!(ral::ccm, ccm, CSCDR1, USDHC1_PODF: 1); // divide by 2 |
There was a problem hiding this comment.
It would be nice to see these written in terms of imxrt-hal ccm helper calls, like
| ral::modify_reg!(ral::ccm, ccm, CSCMR1, USDHC1_CLK_SEL: 0); // PLL2_PFD2 | |
| ral::modify_reg!(ral::ccm, ccm, CSCDR1, USDHC1_PODF: 1); // divide by 2 | |
| ccm::usdhc_clk::set_selection::<1>(ccm, ccm::usdhc_clk::Selection::Pll2Pfd2); | |
| ccm::usdhc_clk::set_divider::<1>(ccm, 2); |
That would fit in more with how this module already looks. But that implementation detail could come later, once supported by imxrt-hal.
| Add USDHC1 clock configuration and expose the USDHC1 peripheral instance | ||
| in board resources. The BSP configures the USDHC1 root clock to 198 MHz | ||
| (PLL2_PFD2 / 2) and publishes this as `USDHC1_FREQUENCY`. Pin muxing and | ||
| driver initialization are left to the caller. |
There was a problem hiding this comment.
We're allowed to change clock details. Let's just note that the root clock is configured.
| Add USDHC1 clock configuration and expose the USDHC1 peripheral instance | |
| in board resources. The BSP configures the USDHC1 root clock to 198 MHz | |
| (PLL2_PFD2 / 2) and publishes this as `USDHC1_FREQUENCY`. Pin muxing and | |
| driver initialization are left to the caller. | |
| Add USDHC1 clock configuration and expose the USDHC1 peripheral instance | |
| in board resources. The BSP configures the USDHC1 root clock and publishes the | |
| frequency as `USDHC1_FREQUENCY`. Pin muxing and | |
| driver initialization are left to the caller. |
Configure USDHC1 root clock and expose the peripheral: - Configure USDHC1 root clock (PLL2_PFD2 / 2 = 198 MHz) - Add USDHC1 clock gate to CLOCK_GATES - Add USDHC1_FREQUENCY constant - Add usdhc1 instance to board Resources
76a51eb to
994205d
Compare
Integrate the imxrt-usdhc driver crate into the BSP: