Fall back to max eMMC clock to fix Kingston eMMC compat with ODROID-N2 #3665
Conversation
Replace the original patch that limits the frequency to 24 MHz with a more sophisticated version that first tries to initialize the eMMC at this limited frequency, and if it fails, then tries disabling the limit to use one defined in the upstream driver. While still hacky, this seems to finally fix the Kingston eMMC issues, and with the current implementation, the initialization should appear the same as on older HAOS versions, as 24 MHz is used first. Better solution that addresses the root cause on some eMMCs (128 GB Microns?) would be still nice though... refs #3250, refs #3227
📝 WalkthroughWalkthroughThe pull request introduces changes to the MMC driver for the Meson GX platform in U-Boot, specifically to limit the maximum clock frequency for eMMC cards during initialization. A new boolean flag, Changes
Sequence Diagram(s)sequenceDiagram
participant Driver
participant MMC
participant eMMC
Driver->>MMC: Initialize
MMC->>eMMC: Set clock frequency
alt meson_gx_f_max_hack is true
MMC->>eMMC: Limit clock to 24 MHz
else
MMC->>eMMC: Set clock to requested frequency
end
eMMC-->>MMC: Initialization complete
MMC-->>Driver: Return status
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
buildroot-external/board/hardkernel/patches/uboot/0001-HACK-mmc-meson-gx-limit-f_max-to-24-MHz-on-the-first.patch (6)
1-26: Consider enhancing the commit message documentation.While the commit message explains the approach well, it would be helpful to:
- Document specific Kingston eMMC models affected
- Explain why Kingston chips work better at 52 MHz
- Consider removing "HACK" from the subject if this is intended as a permanent solution
31-38: Add documentation for the meson_gx_f_max_hack flag.The flag's purpose and impact should be documented in the code. Consider adding a comment explaining:
- Why it's enabled by default
- Which devices/cards are affected
- The performance implications
cfg->b_max = 511; /* max 512 - 1 blocks */ cfg->name = dev->name; + /* Enable 24MHz frequency limit by default for better compatibility. + * This may be disabled later if card initialization fails. + * Known to affect: Kingston eMMC cards on ODROID-N2 + */ + mmc->meson_gx_f_max_hack = true; + mmc->priv = pdata;
44-54: Consider improving the clock limit implementation.The clock limiting logic is correct but could be more robust:
- Consider using a named constant for 24MHz instead of magic number
- Add debug logging to help troubleshoot timing issues
+#define MESON_GX_SAFE_CLOCK_HZ 24000000 + /* Apply 24 MHz limit that fixes issues with some cards on meson. */ - if (mmc->meson_gx_f_max_hack && clock > 24000000) - clock = 24000000; + if (mmc->meson_gx_f_max_hack && clock > MESON_GX_SAFE_CLOCK_HZ) { + debug("mmc: limiting clock from %d Hz to %d Hz\n", + clock, MESON_GX_SAFE_CLOCK_HZ); + clock = MESON_GX_SAFE_CLOCK_HZ; + }
59-66: Enhance error reporting and handling.The error handling could be improved to:
- Provide more detailed error information
- Log the actual error code
- Mention that a retry will be attempted at higher frequency
if (err && mmc->meson_gx_f_max_hack) { /* Some eMMCs (namely Kingston) do not initialize at limited frequency. */ - printf("Card failed to initialize at %d Hz, disabling meson_gx hack.\n", - mmc->clock); + printf("MMC initialization failed (err=%d) at %d Hz\n", + err, mmc->clock); + printf("Retrying with maximum supported frequency...\n"); mmc->meson_gx_f_max_hack = false; err = mmc_select_mode_and_width(mmc, mmc->card_caps); + if (err) + printf("MMC initialization failed again (err=%d)\n", err); }
73-79: Document the meson_gx_f_max_hack field.Consider adding documentation for the new field to explain its purpose and behavior.
enum bus_mode user_speed_mode; /* input speed mode from user */ - bool meson_gx_f_max_hack; + /* When true, limits initial clock to 24MHz for compatibility. + * This is automatically disabled and retried if initialization fails. + */ + bool meson_gx_f_max_hack;
1-81: Consider a more robust long-term solution.While this implementation provides a working solution for Kingston eMMC compatibility, consider these architectural improvements for the future:
- Make the initial frequency limit configurable through device tree or U-Boot environment variables
- Implement card vendor/model detection to apply the hack selectively
- Consider adding support for other frequency steps in the fallback sequence
This would provide more flexibility and better maintainability for future compatibility issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
buildroot-external/board/hardkernel/patches/uboot/0001-HACK-mmc-meson-gx-limit-f_max-to-24-MHz-on-the-first.patch(1 hunks)buildroot-external/board/hardkernel/patches/uboot/0001-HACK-mmc-meson-gx-limit-to-24MHz.patch(0 hunks)
💤 Files with no reviewable changes (1)
- buildroot-external/board/hardkernel/patches/uboot/0001-HACK-mmc-meson-gx-limit-to-24MHz.patch
|
I ran an automated test where the ODROID cold-booted over 100 times in a row without issues. Now I'm running another one where I'm leaving it to boot to the system and run a |
Replace the original patch that limits the frequency to 24 MHz with a more sophisticated version that first tries to initialize the eMMC at this limited frequency, and if it fails, then tries disabling the limit to use one defined in the upstream driver.
While still hacky, this seems to finally fix the Kingston eMMC issues, and with the current implementation, the initialization should appear the same as on older HAOS versions, as 24 MHz is used first. Better solution that addresses the root cause on some eMMCs (128 GB Microns?) would be still nice though...
refs #3250, refs #3227
Summary by CodeRabbit
New Features
Bug Fixes