Add patch to fix Kingston eMMC compatibility with ODROID-N2 #3619
Conversation
Testing shows that the 24MHz cap is problematic for Kingston. Bumping this cap to 40 MHz in #3319 fixed Kingston but reintroduced issues with some other eMMCs that the frequency cap was supposed to fix. This commit adds another patch that selects a different clock for eMMC. While it's still unclear why Kingston fails with the frequencies of slightly below 24 MHz (see patch message why it's below), using 24 MHz from xtal seems to be fine and hopefully shouldn't introduce regressions for old eMMCs. Alternative approach would be using the same clock source but setting the frequency cap to 25 MHz. This results in exact 25 MHz from PLL which is even the defined frequency for some eMMC modes, Kingston eMMC works fine with it and the difference from the previously used value wouldn't be too high, but with 24 MHz we're even closer. refs #3250, refs #3227
📝 WalkthroughWalkthroughThe changes in this pull request involve a patch to the Meson GX MMC driver, specifically modifying the clock configuration to utilize the crystal oscillator (xtal) as the clock source for frequencies up to 24 MHz. This adjustment addresses issues with imprecise clock rates that affected communication with certain Kingston eMMCs, ensuring a precise 24 MHz frequency. The implementation includes a conditional check in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MesonGX_MMC_Driver
participant Clock_Source
User->>MesonGX_MMC_Driver: Request clock configuration
MesonGX_MMC_Driver->>Clock_Source: Check requested frequency
alt Frequency > 24 MHz
Clock_Source-->>MesonGX_MMC_Driver: Set clock source to xtal
else
Clock_Source-->>MesonGX_MMC_Driver: Use previous clock source
end
MesonGX_MMC_Driver-->>User: Clock configuration applied
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: 1
🧹 Outside diff range and nitpick comments (2)
buildroot-external/board/hardkernel/patches/uboot/0002-mmc-meson-gx-use-xtal-as-clock-for-frequencies-24MHz.patch (2)
44-44: Consider addressing the TODO commentThe comment "TOFIX This should use the proper clock taken from DT" remains unchanged. Consider creating a follow-up task to address this TODO, as it might be important for proper device tree support.
Would you like me to create a GitHub issue to track this TODO item?
48-53: Fix indentation of the new comment blockThe new comment block is indented with one space less than the surrounding code. Please adjust the indentation to match the rest of the function for consistency.
Apply this diff to fix the indentation:
- /* - * With f_max=24M, we almost always end up with 24M anyway and - * granularity between 24M and 12M isn't needed, so try to use - * xtal clock source directly. - */ + /* + * With f_max=24M, we almost always end up with 24M anyway and + * granularity between 24M and 12M isn't needed, so try to use + * xtal clock source directly. + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- buildroot-external/board/hardkernel/patches/uboot/0002-mmc-meson-gx-use-xtal-as-clock-for-frequencies-24MHz.patch (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
buildroot-external/board/hardkernel/patches/uboot/0002-mmc-meson-gx-use-xtal-as-clock-for-frequencies-24MHz.patch (3)
9-33: Well-documented commit messageThe commit message provides a comprehensive explanation of the issue, the proposed solution, and its rationale. It also acknowledges potential future improvements, which is valuable for maintainers and other developers.
46-53: Improved code readability with named constantThe replacement of the magic number
16000000with the named constantSD_EMMC_CLKSRC_24Mimproves code readability and maintainability. This change is appreciated.
1-56: Overall assessment: Patch looks good with minor improvements suggestedThe patch successfully implements the intended fix for the Kingston eMMC compatibility issue with ODROID-N2. The changes are well-documented, minimal, and focused. The use of a named constant improves code readability.
A few minor improvements have been suggested:
- Correcting the commit date in the patch header.
- Addressing the existing TODO comment about using the proper clock from the device tree.
- Fixing the indentation of the new comment block.
Once these minor issues are addressed, the patch should be ready for merging.
| From 77ec70c4d73c772b7da0d007946f697f7d27a7a7 Mon Sep 17 00:00:00 2001 | ||
| From: =?UTF-8?q?Jan=20=C4=8Cerm=C3=A1k?= <sairon@sairon.cz> | ||
| Date: Thu, 10 Oct 2024 19:06:24 +0200 | ||
| Subject: [PATCH] mmc: meson-gx: use xtal as clock for frequencies <= 24MHz | ||
| MIME-Version: 1.0 | ||
| Content-Type: text/plain; charset=UTF-8 | ||
| Content-Transfer-Encoding: 8bit | ||
|
|
There was a problem hiding this comment.
Incorrect commit date in the patch header
The commit date is set to October 10, 2024, which is in the future. This could cause issues with version control and should be corrected to the current date.
Please update the commit date to the current date:
-Date: Thu, 10 Oct 2024 19:06:24 +0200
+Date: <current_date_and_time>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| From 77ec70c4d73c772b7da0d007946f697f7d27a7a7 Mon Sep 17 00:00:00 2001 | |
| From: =?UTF-8?q?Jan=20=C4=8Cerm=C3=A1k?= <sairon@sairon.cz> | |
| Date: Thu, 10 Oct 2024 19:06:24 +0200 | |
| Subject: [PATCH] mmc: meson-gx: use xtal as clock for frequencies <= 24MHz | |
| MIME-Version: 1.0 | |
| Content-Type: text/plain; charset=UTF-8 | |
| Content-Transfer-Encoding: 8bit | |
| From 77ec70c4d73c772b7da0d007946f697f7d27a7a7 Mon Sep 17 00:00:00 2001 | |
| From: =?UTF-8?q?Jan=20=C4=8Cerm=C3=A1k?= <sairon@sairon.cz> | |
| Date: <current_date_and_time> | |
| Subject: [PATCH] mmc: meson-gx: use xtal as clock for frequencies <= 24MHz | |
| MIME-Version: 1.0 | |
| Content-Type: text/plain; charset=UTF-8 | |
| Content-Transfer-Encoding: 8bit | |
|
Unfortunately, this doesn't work as good as I initially thought 😢 I will try few more things and run some automated test to test the patch works reliably the next time. |
Testing shows that the 24MHz cap is problematic for Kingston. Bumping this cap to 40 MHz in #3319 fixed Kingston but reintroduced issues with some other eMMCs that the frequency cap was supposed to fix.
This commit adds another patch that selects a different clock for eMMC. While it's still unclear why Kingston fails with the frequencies of slightly below 24 MHz (see patch message why it's below), using 24 MHz from xtal seems to be fine and hopefully shouldn't introduce regressions for old eMMCs.
Alternative approach would be using the same clock source but setting the frequency cap to 25 MHz. This results in exact 25 MHz from PLL which is even the defined frequency for some eMMC modes, Kingston eMMC works fine with it and the difference from the previously used value wouldn't be too high, but with 24 MHz we're even closer.
refs #3250, refs #3227
Summary by CodeRabbit