Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sdio: add sdh peripherals and modify sdh_demo #15

Merged
merged 2 commits into from
Dec 27, 2024

Conversation

NanaHigh
Copy link
Member

Also fix some code issues.
Tested on M1s Dock, is works fine.
Ok

@luojia65 luojia65 changed the title Nanahigh/sdio sdio: add sdh peripherals and modify sdh_demo Dec 27, 2024
Copy link
Member

@luojia65 luojia65 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good overall, with only minor style adjustments needed.

Comment on lines 243 to 244
/// Set this bit to zero.
ZERO = 0,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Set this bit to zero.
ZERO = 0,
/// Set this bit to zero.
None = 0,

Should we consider using None to indicate that no automatic command is enabled?

@@ -321,22 +330,23 @@ pub struct Command(u16);

/// CMD Type.
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
pub enum CmdType {
Other,
pub enum SDHCmdType {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub enum SDHCmdType {
pub enum CommandType {

I suggest using CommandType as the enum name, as the module name sdio already implies it pertains to an SDH command.

Comment on lines 3613 to 3618
while !self.sdh.software_reset.read().is_reset_all_finished() {
// Wait for software reset finished.
unsafe {
asm!("nop");
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
while !self.sdh.software_reset.read().is_reset_all_finished() {
// Wait for software reset finished.
unsafe {
asm!("nop");
}
}
// Wait for software reset finished.
while !self.sdh.software_reset.read().is_reset_all_finished() {
core::hint::spin_loop()
}

Use core::hint::spin_loop() for busy-wait loops. This function provides appropriate hint instructions for each architecture to temporarily reduce the instruction retirement rate, improving energy efficiency.

Comment on lines 3620 to 3615
// GLB_Set_SDH_CLK.
let mut tmp_val = glb.sdh_config.read(); // GLB_REG_SDH_CLK_EN.
tmp_val = tmp_val.disable_sdh_clk();
unsafe {
glb.sdh_config.write(tmp_val);
}
tmp_val = glb.sdh_config.read();
tmp_val = tmp_val.set_sdh_clk_sel(0); // GLB_REG_SDH_CLK_SEL.
tmp_val = tmp_val.set_sdh_clk_div_len(7); // GLB_REG_SDH_CLK_DIV.
unsafe {
glb.sdh_config.write(tmp_val);
}
tmp_val = glb.sdh_config.read();
tmp_val = tmp_val.enable_sdh_clk(); // GLB_REG_SDH_CLK_EN.
unsafe {
glb.sdh_config.write(tmp_val);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are multiple writes to the sdh_config register. Will the code function correctly if we retain only the last write?

Comment on lines 3638 to 3633
// SDH_Ctrl_Init.
let mut tmp_val = self.sdh.clock_control.read();
tmp_val = tmp_val.set_sd_clk_freq(0); // SDH_SD_FREQ_SEL_LO.
tmp_val = tmp_val.set_sd_clk_freq_upper(0); // SDH_SD_FREQ_SEL_HI.
tmp_val = tmp_val.set_clk_gen_mode(ClkGenMode::DividedClk); // SDH_CLK_GEN_SEL.
tmp_val = tmp_val.enable_internal_clk(); // SDH_INT_CLK_EN.
tmp_val = tmp_val.enable_sd_clk(); // SDH_SD_CLK_EN.
unsafe {
self.sdh.clock_control.write(tmp_val);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer modify function on RW registers to shorten the code size.

Also fix some code issues.

Signed-off-by: Yu Chongbing <[email protected]>
@NanaHigh
Copy link
Member Author

I have completed the adjustment in latest commit. @luojia65

Copy link
Member

@luojia65 luojia65 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@luojia65 luojia65 merged commit d1d212d into rustsbi:main Dec 27, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants