atf: rk3399: fix PL330 DMA controller initialization #9285
+188
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mainline TF-A doesn't initialize SGRF_SOC_CON8-CON15 which control DMAC0 and DMAC1 configuration. Without this, the PL330 DMA controllers cannot be accessed from Linux and PERIPH_ID reads as 0x0. The fix configures SGRF registers to set DMAC manager threads to running state and non-secure mode, then pulses DMAC reset to apply the new configuration. Based on community reverse-engineering: - https://lists.denx.de/pipermail/u-boot/2023-April/514267.html - https://gist.github.com/CrystalGamma/a68333fa4c9fda7eb6c09d30ad4937fe Tested on Helios64 (RK3399). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
iav
requested review from
ColorfulRhino,
HeyMeco,
SuperKali,
Tonymac32,
ahoneybun,
amazingfate,
igorpecovnik,
joekhoobyar,
paolosabatino and
prahal
as code owners
Contributor
📝 WalkthroughWalkthroughThis PR introduces DMAC SGRF initialization for RK3399 SoCs. It adds configuration logic to set DMAC0/DMAC1 security and manager states via SGRF registers, then pulses their reset to apply the settings, enabling proper DMA controller access from Linux. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
02
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@patch/atf/atf-rockchip64/v2.13/1003-rk3399-fix-pl330-dmac-sgrf-init.patch`:
- Around line 36-67: The patch uses undefined macros CRU_DMAC0_RST,
CRU_DMAC1_RST, CRU_DMAC0_RST_RLS and CRU_DMAC1_RST_RLS when writing to
CRU_SOFTRST_CON(10); add these macro definitions to secure.h following the
existing write-mask pattern: base bit positions BIT(12) and BIT(13) for
DMAC0/DMAC1 respectively, define the *_RST macro as the bit OR'ed with that bit
shifted into the upper 16-bit write-mask, and define the *_RST_RLS macro as only
the upper 16-bit mask (the bit shifted left 16) so mmio_write_32(CRU_BASE +
CRU_SOFTRST_CON(10), ...) pulses the reset correctly.
In `@patch/atf/atf-rockchip64/v2.14/1003-rk3399-fix-pl330-dmac-sgrf-init.patch`:
- Around line 60-67: Add the missing CRU_DMAC reset macro definitions to
secure.h: define CRU_DMAC0_RST and CRU_DMAC1_RST as the assert values that set
the bit and its write-mask (BIT(pos) | BIT(16 + pos)) for DMAC0 (pos 3) and
DMAC1 (pos 4), and define CRU_DMAC0_RST_RLS and CRU_DMAC1_RST_RLS as the
deassert write-mask-only values (BIT(16 + pos)) for the same positions; use the
existing BIT() macro so the new symbols CRU_DMAC0_RST, CRU_DMAC1_RST,
CRU_DMAC0_RST_RLS, and CRU_DMAC1_RST_RLS are available for the reset sequence in
the patch.
patch/atf/atf-rockchip64/v2.13/1003-rk3399-fix-pl330-dmac-sgrf-init.patch
Show resolved
Hide resolved
Comment on lines
+60
to
+67
| + /* Assert DMAC0 and DMAC1 reset */ | ||
| + mmio_write_32(CRU_BASE + CRU_SOFTRST_CON(10), | ||
| + CRU_DMAC0_RST | CRU_DMAC1_RST); | ||
| + /* Small delay to ensure reset is recognized */ | ||
| + udelay(1); | ||
| + /* Deassert DMAC0 and DMAC1 reset */ | ||
| + mmio_write_32(CRU_BASE + CRU_SOFTRST_CON(10), | ||
| + CRU_DMAC0_RST_RLS | CRU_DMAC1_RST_RLS); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the specific CRU_DMAC macros mentioned in the review
echo "=== Searching for CRU_DMAC macro definitions ==="
rg -i "CRU_DMAC0_RST|CRU_DMAC1_RST|CRU_DMAC0_RST_RLS|CRU_DMAC1_RST_RLS" -t c -t h --no-heading
echo ""
echo "=== Checking the patch file itself ==="
cat patch/atf/atf-rockchip64/v2.14/1003-rk3399-fix-pl330-dmac-sgrf-init.patch | head -100Repository: armbian/build
Length of output: 3836
🏁 Script executed:
#!/bin/bash
# Search for CRU_BASE definition and other CRU macros
echo "=== Searching for CRU_BASE and CRU_SOFTRST_CON ==="
rg -i "CRU_BASE|CRU_SOFTRST_CON" -t c -t h --no-heading -A2
echo ""
echo "=== Search for any RST or DMAC patterns in headers ==="
fd -e h -type f | xargs grep -l "CRU\|RST\|DMAC" 2>/dev/null | head -20
echo ""
echo "=== Check if these macros appear in any ATF platform includes ==="
fd -e h -type f -path "*rockchip*" -o -path "*rk3399*" | head -20Repository: armbian/build
Length of output: 589
🏁 Script executed:
#!/bin/bash
# Search more carefully for CRU definitions
echo "=== Find all rockchip-related header files ==="
find . -path "*rockchip*" -name "*.h" 2>/dev/null | head -30
echo ""
echo "=== Search for CRU in all .h files ==="
rg "CRU_BASE|CRU_SOFTRST|DMAC0|DMAC1" --type h --no-heading
echo ""
echo "=== Check the directory structure of rk3399 ==="
find . -path "*rk3399*" -type d 2>/dev/null
echo ""
echo "=== Check if there's a base ATF tree in this repo ==="
ls -la plat/rockchip/rk3399/drivers/secure/ 2>/dev/null || echo "Directory not found"Repository: armbian/build
Length of output: 387
🏁 Script executed:
#!/bin/bash
# This is the Armbian build repo with patches. Let's check what other patches exist
# for the same file to see if macros are defined elsewhere
echo "=== List all patches in this directory ==="
ls -la patch/atf/atf-rockchip64/v2.14/ | head -20
echo ""
echo "=== Check if there's a v2.13 version for comparison ==="
ls -la patch/atf/atf-rockchip64/v2.13/ 2>/dev/null | grep -E "1003|dmac|cru" || echo "No v2.13 found or no matching patches"
echo ""
echo "=== Look for any other patches that might define CRU macros ==="
grep -r "CRU_DMAC\|#define.*CRU" patch/atf/ 2>/dev/null | head -20
echo ""
echo "=== Check if the base ATF includes CRU definitions ==="
find . -name "*.h" -path "*/rk3399/*" 2>/dev/null | xargs grep -l "CRU" 2>/dev/nullRepository: armbian/build
Length of output: 2260
🏁 Script executed:
#!/bin/bash
# Compare v2.13 and v2.14 versions of the same patch
echo "=== Compare v2.13 and v2.14 1003 patches ==="
diff patch/atf/atf-rockchip64/v2.13/1003-rk3399-fix-pl330-dmac-sgrf-init.patch \
patch/atf/atf-rockchip64/v2.14/1003-rk3399-fix-pl330-dmac-sgrf-init.patch || true
echo ""
echo "=== Check what defines CRU_BASE in ATF or patches ==="
grep -r "define CRU_BASE" patch/ 2>/dev/null
echo ""
echo "=== Check for similar reset patterns in other patches ==="
grep -r "CRU_SOFTRST_CON\|mmio_write_32.*CRU" patch/atf/ 2>/dev/null | head -10Repository: armbian/build
Length of output: 703
🏁 Script executed:
#!/bin/bash
# Check the v2.13 patch in detail to see if it defines the CRU_DMAC macros
echo "=== View v2.13 patch header section ==="
cat patch/atf/atf-rockchip64/v2.13/1003-rk3399-fix-pl330-dmac-sgrf-init.patch | head -80
echo ""
echo "=== Check if CRU_DMAC macros are defined in v2.13 ==="
grep "define CRU_DMAC" patch/atf/atf-rockchip64/v2.13/1003-rk3399-fix-pl330-dmac-sgrf-init.patch || echo "NOT FOUND in v2.13"
echo ""
echo "=== Check if CRU_BASE is in any rockchip ATF patch ==="
grep -r "define CRU_BASE" patch/atf/atf-rockchip64/ || echo "CRU_BASE not defined in any patch"
echo ""
echo "=== Look for PLAT_RK_XLAT_TABLE or platform include in secure.h context ==="
cat patch/atf/atf-rockchip64/v2.14/1003-rk3399-fix-pl330-dmac-sgrf-init.patch | grep -A 100 "secure.h"Repository: armbian/build
Length of output: 6220
🏁 Script executed:
#!/bin/bash
# Check what's in the original ATF secure.h before the patch to understand the includes
echo "=== Check full secure.h diff in patch ==="
cat patch/atf/atf-rockchip64/v2.14/1003-rk3399-fix-pl330-dmac-sgrf-init.patch | grep -A 50 "secure.h"
echo ""
echo "=== Search for CRU_BASE definition across all patches ==="
grep -r "CRU_BASE" patch/ 2>/dev/null | head -5
echo ""
echo "=== Check secure.c includes to understand where CRU_BASE should come from ==="
head -50 patch/atf/atf-rockchip64/v2.14/1003-rk3399-fix-pl330-dmac-sgrf-init.patch | tail -40Repository: armbian/build
Length of output: 5210
🏁 Script executed:
#!/bin/bash
# Look at rk3328 patch to see how CRU_BASE and related are handled there
echo "=== Check rk3328 patch for CRU macro definitions ==="
cat patch/atf/atf-rockchip64/v2.14/1000-rk3328-add-efuse-initialization-in-ATF.patch | grep -E "^(\+#define|diff|---|\+\+\+)" | head -40
echo ""
echo "=== Look for where CRU_BASE might be defined (check full rk3328 patch header) ==="
head -100 patch/atf/atf-rockchip64/v2.14/1000-rk3328-add-efuse-initialization-in-ATF.patch | tail -60Repository: armbian/build
Length of output: 3537
🏁 Script executed:
#!/bin/bash
# Check if platform_def.h or soc.h exists and what they contain
echo "=== Search for platform_def.h or soc.h references ==="
grep -r "platform_def\|soc.h" patch/atf/ 2>/dev/null | head -10
echo ""
echo "=== Look for any header files that might define CRU constants ==="
grep -r "#include.*platform_def\|#include.*soc.h" patch/atf/ | head -5
echo ""
echo "=== Check what includes are in the rk3399 secure.c file being patched ==="
grep -A 30 "^diff.*secure.c" patch/atf/atf-rockchip64/v2.14/1003-rk3399-fix-pl330-dmac-sgrf-init.patch | grep -E "^\+#include|^\+\s*$" | head -20Repository: armbian/build
Length of output: 2263
🏁 Script executed:
#!/bin/bash
# Check the actual includes in secure.c before the patch
echo "=== Extract the beginning of secure.c from the patch context ==="
cat patch/atf/atf-rockchip64/v2.14/1003-rk3399-fix-pl330-dmac-sgrf-init.patch | head -200 | tail -100
echo ""
echo "=== Search for any reference to where CRU_BASE comes from ==="
grep -B 5 "CRU_BASE" patch/atf/atf-rockchip64/v2.14/1003-rk3399-fix-pl330-dmac-sgrf-init.patch | head -20Repository: armbian/build
Length of output: 4345
🏁 Script executed:
#!/bin/bash
# Try a different approach - check if the patch shows the file context before the changes
echo "=== Get raw patch content around includes ==="
cat patch/atf/atf-rockchip64/v2.14/1003-rk3399-fix-pl330-dmac-sgrf-init.patch | sed -n '/^diff.*secure.c/,/^---/p' | head -50
echo ""
echo "=== Check if there are any other v2.x patches with CRU_DMAC definitions ==="
find patch/atf/atf-rockchip64/ -name "*.patch" -exec grep -l "CRU_DMAC.*#define\|#define.*CRU_DMAC" {} \;
echo ""
echo "=== Search for rockchip soc.h references which might define these ==="
grep -r "soc.h" patch/atf/atf-rockchip64/ | head -5Repository: armbian/build
Length of output: 638
🏁 Script executed:
#!/bin/bash
# Let's verify by checking TF-A mainline documentation or typical patterns
# First, let's see what the actual structure of CRU reset operations should be
echo "=== Check for CRU reset patterns in Rockchip ATF commits/documentation ==="
grep -r "CRU.*RST\|SOFTRST_CON" patch/atf/ 2>/dev/null | grep -v ".patch:" | head -10
echo ""
echo "=== Look for any macro definitions that look like reset bit masks ==="
grep -r "BIT.*<<\|<< 16\|WMSK\|write.*mask" patch/atf/atf-rockchip64/ 2>/dev/null | grep -E "(define|#define)" | head -10
echo ""
echo "=== Check the full secure.h patch diff to see all new definitions ==="
sed -n '/^diff.*secure.h/,/^--$/p' patch/atf/atf-rockchip64/v2.14/1003-rk3399-fix-pl330-dmac-sgrf-init.patch | tail -50Repository: armbian/build
Length of output: 1344
🏁 Script executed:
#!/bin/bash
# Final verification - search for where these CRU reset constants might be defined
# in a standard TF-A distribution
echo "=== Check if ATF v2.14 source code exists locally ==="
find . -name "*.c" -o -name "*.h" | xargs grep -l "CRU_DMAC0_RST\|CRU_SOFTRST_CON" 2>/dev/null | head -5
echo ""
echo "=== Look for what CRU_SOFTRST_CON macro looks like if it exists ==="
grep -r "CRU_SOFTRST_CON" . 2>/dev/null | grep -v ".patch" | head -5
echo ""
echo "=== Check if platform_def.h or soc.h exists in actual source ==="
find . -path "*rk3399*" -name "platform_def.h" -o -path "*rk3399*" -name "soc.h" 2>/dev/null
echo ""
echo "=== Check the git repo to see base state ==="
git ls-files "*rk3399*secure*" 2>/dev/null | head -10Repository: armbian/build
Length of output: 291
Missing CRU_DMAC reset macro definitions will cause compilation failure.
The reset sequence uses CRU_DMAC0_RST, CRU_DMAC1_RST, CRU_DMAC0_RST_RLS, and CRU_DMAC1_RST_RLS, but these macros are not defined in this patch or elsewhere in the codebase. They must be added to secure.h.
For RK3399 CRU soft reset registers (which use write-mask in bits 16-31), add these definitions to secure.h:
Suggested macro definitions
/* CRU_SOFTRST_CON10: DMAC0 bit 3, DMAC1 bit 4 (verify bit positions against RK3399 TRM) */
`#define` CRU_DMAC0_RST (BIT(3) | BIT(16 + 3)) /* assert with write-mask */
`#define` CRU_DMAC1_RST (BIT(4) | BIT(16 + 4))
`#define` CRU_DMAC0_RST_RLS BIT(16 + 3) /* deassert: mask only, value 0 */
`#define` CRU_DMAC1_RST_RLS BIT(16 + 4)🤖 Prompt for AI Agents
In `@patch/atf/atf-rockchip64/v2.14/1003-rk3399-fix-pl330-dmac-sgrf-init.patch`
around lines 60 - 67, Add the missing CRU_DMAC reset macro definitions to
secure.h: define CRU_DMAC0_RST and CRU_DMAC1_RST as the assert values that set
the bit and its write-mask (BIT(pos) | BIT(16 + pos)) for DMAC0 (pos 3) and
DMAC1 (pos 4), and define CRU_DMAC0_RST_RLS and CRU_DMAC1_RST_RLS as the
deassert write-mask-only values (BIT(16 + pos)) for the same positions; use the
existing BIT() macro so the new symbols CRU_DMAC0_RST, CRU_DMAC1_RST,
CRU_DMAC0_RST_RLS, and CRU_DMAC1_RST_RLS are available for the reset sequence in
the patch.
Contributor
Author
|
@coderabbitai The macros File: #define CRU_DMAC0_RST BIT_WITH_WMSK(3)
/* reset release*/
#define CRU_DMAC0_RST_RLS WMSK_BIT(3)
#define CRU_DMAC1_RST BIT_WITH_WMSK(4)
/* reset release*/
#define CRU_DMAC1_RST_RLS WMSK_BIT(4)And #include <soc.h>So no additional definitions are needed — the patch uses existing infrastructure. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
The use of This means the patch only needs to add the DMAC-specific SGRF configuration macros ( (_/) ✏️ Learnings added
|
This was referenced Jan 23, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fix PL330 DMA controllers (DMAC0 and DMAC1) not initializing on RK3399 boards when using mainline TF-A.
Problem: Linux kernel shows errors on boot:
The AMBA PERIPH_ID register reads as 0x0 instead of 0x00241330, so the PL330 driver cannot identify the device.
Root cause: Mainline TF-A doesn't initialize SGRF_SOC_CON8-CON15 registers which control DMAC security configuration. Without this, the DMA controllers are inaccessible from non-secure world (Linux).
Changes
ATF patch for RK3399 (
plat/rockchip/rk3399/drivers/secure/secure.c):Configure SGRF registers for DMAC0 and DMAC1:
Pulse DMAC reset after SGRF configuration:
The SGRF configuration defines DMAC state after reset, so we must reset the DMAC controllers for the new settings to take effect.
Results
After fix:
References
Test plan
Notes
🤖 This fix was developed by Claude (Anthropic AI assistant). Human tested on Helios64 hardware.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.