[WIP]am261x: pru i2c: Add PRU I2C Master firmware and R5F example#137
Open
achala-ti wants to merge 2 commits into
Open
[WIP]am261x: pru i2c: Add PRU I2C Master firmware and R5F example#137achala-ti wants to merge 2 commits into
achala-ti wants to merge 2 commits into
Conversation
achala-ti
commented
May 4, 2026
achala-ti
commented
Collaborator
- Add I2C firmware files
- Create PRU0/PRU1 build projects for ICSS_M0
- Add R5F example
PR Summary by Qodoam261x: Add PRU I2C master firmware example and wire into build Description
Diagram
High-Level Assessment
Files changed (14)
|
Code Review by Qodo
1. pru_i2c_macro.h header filename mismatch
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+1
to
+2
| export OPEN_PRU_PATH?=$(abspath ../../../../../..) | ||
| include $(OPEN_PRU_PATH)/imports.mak |
There was a problem hiding this comment.
1. open_pru_path uses fixed abspath 📘 Rule violation ☼ Reliability
The new makefiles compute OPEN_PRU_PATH using a fixed relative $(abspath ../../../../../..) which can break when invoked from a different working directory. This violates the requirement to derive the repo root from the current makefile location for reliable includes.
Agent Prompt
## Issue description
Makefiles set `OPEN_PRU_PATH` using `$(abspath ../../../../../..)` which depends on the directory `make` is invoked from, causing fragile includes.
## Issue Context
The compliance requirement asks to derive the repository root relative to the current makefile path (e.g., using `$(dir $(lastword $(MAKEFILE_LIST)))`).
## Fix Focus Areas
- examples/pru_i2c/firmware/am261x-lp/icss_m0_pru0_fw/ti-pru-cgt/makefile[1-2]
- examples/pru_i2c/firmware/am261x-lp/icss_m0_pru1_fw/ti-pru-cgt/makefile[1-2]
- examples/pru_i2c/example/am261x-lp/r5fss0-0_freertos/ti-arm-clang/makefile[5-7]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+37
to
+46
| .if !$defined("__icss_i2c_macros_h") | ||
| __icss_i2c_macros_h .set 1 | ||
|
|
||
| ;************************************* includes ************************************* | ||
|
|
||
| ;;.include "icss_constant_defines.inc" | ||
| ;;.include "icss_cfg_regs.h" | ||
| ;;.include "icss_iep_regs.h" | ||
| ;;.include "icss_intc_regs.inc" | ||
| ;;;.include "endat_icss_reg_defs.h" |
There was a problem hiding this comment.
2. Nonstandard pru header guard 📘 Rule violation ⚙ Maintainability
The new PRU assembly include file uses a noncompliant header guard symbol and mechanism. This risks multiple-include issues and violates the required four-underscore $isdefed guard pattern.
Agent Prompt
## Issue description
`pru_i2c_macro.h` uses a nonstandard header guard (`$defined` + `__icss_i2c_macros_h`) instead of the required `$isdefed` + four-underscore guard symbol.
## Issue Context
Assembly include files may be included from multiple compilation units/macros; guard consistency prevents multiple-include errors.
## Fix Focus Areas
- examples/pru_i2c/firmware/pru_i2c_macro.h[37-46]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+58
to
+66
| .asg R1, TEMP_REG1 ; temporary register 1 | ||
| .asg R2, TEMP_REG2 ; temporary register 2 | ||
| .asg R3, TEMP_REG3 ; temporary register 3 | ||
| .asg R4, TEMP_REG4 ; temporary register 4 | ||
| .asg R5, TEMP_REG5 ; temporary register 5 | ||
| .asg R6, TEMP_REG6 ; temporary register 6 | ||
| .asg R7, TEMP_REG7 ; temporary register 7 | ||
| .asg R8, TEMP_REG8 ; temporary register 8 | ||
|
|
There was a problem hiding this comment.
3. Direct .asg register aliases 📘 Rule violation ☼ Reliability
The new PRU firmware defines working registers via direct PRU register aliasing (e.g., `.asg R1, TEMP_REG1`). This violates the requirement to prefer structured register mappings (.struct/.sassign) to reduce overlap/corruption risk.
Agent Prompt
## Issue description
PRU registers are assigned via `.asg` aliases (e.g., `TEMP_REG1` -> `R1`) instead of using a `.struct` + `.sassign` register map.
## Issue Context
Structured register mapping helps prevent accidental overlap/corruption and improves maintainability when macros/functions evolve.
## Fix Focus Areas
- examples/pru_i2c/firmware/pru_i2c_main.asm[58-66]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+54
to
+75
| ;************************************************************************************ | ||
| ; | ||
| ; Macro: ENABLE_XIN_XOUT_SHITFTING | ||
| ; | ||
| ; Enable support of shifting during XIN/XOUT operation | ||
| ; | ||
| ; PEAK cycles: | ||
| ; 3 cycles | ||
| ; Pseudo code: | ||
| ; ICSS_CFG_SPPC[0-7] |= 0x02 | ||
| ; | ||
| ; Parameters: | ||
| ; None | ||
| ; | ||
| ; Returns: | ||
| ; None | ||
| ; | ||
| ENABLE_XIN_XOUT_SHITFTING .macro | ||
| LBCO &TEMP_REG1, ICSS_CFG_CONST, ICSS_CFG_SPPC, 4 | ||
| OR TEMP_REG1.b0, TEMP_REG1.b0, 0x02 | ||
| SBCO &TEMP_REG1, ICSS_CFG_CONST, ICSS_CFG_SPPC, 4 | ||
| .endm |
There was a problem hiding this comment.
4. Macro docs omit modified registers 📘 Rule violation ⚙ Maintainability
New PRU assembly macros document pseudocode and peak cycles but do not document which registers they modify. This violates the required macro documentation standard and increases the risk of register-corruption regressions.
Agent Prompt
## Issue description
Macro documentation is missing the required "registers modified" list.
## Issue Context
This firmware uses many macros that touch shared working registers; documenting clobbers is required to prevent subtle corruption when macros are composed.
## Fix Focus Areas
- examples/pru_i2c/firmware/pru_i2c_macro.h[54-75]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1
to
+10
| /* | ||
| * Copyright (C) 2024 Texas Instruments Incorporated | ||
| * | ||
| * Redistribution and use in source and binary forms, with or without | ||
| * modification, are permitted provided that the following conditions | ||
| * are met: | ||
| * | ||
| * Redistributions of source code must retain the above copyright | ||
| * notice, this list of conditions and the following disclaimer. | ||
| * |
There was a problem hiding this comment.
5. Files missing filename header 📘 Rule violation ⚙ Maintainability
New source/header files include a TI copyright block but do not include a header denoting the file name. This violates the requirement that every source/header file include both file-name header and TI copyright statement.
Agent Prompt
## Issue description
New files are missing a header line that denotes the file name.
## Issue Context
Compliance requires both a file-name header and a TI copyright statement in every source/header file.
## Fix Focus Areas
- examples/pru_i2c/example/pru_i2c_example.c[1-10]
- examples/pru_i2c/example/am261x-lp/r5fss0-0_freertos/main.c[1-12]
- examples/pru_i2c/example/pru_i2c_example.h[1-12]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+34
to
+40
| typedef struct I2c_Params_s | ||
| { | ||
| PRUICSS_Handle pruicssHandle; | ||
| /**< PRU-ICSS Handle for which the Tamagawa driver will be based on */ | ||
| uint32_t baudRate; | ||
|
|
||
| } I2c_Params; No newline at end of file |
There was a problem hiding this comment.
6. Missing newline at eof 📘 Rule violation ⚙ Maintainability
pru_i2c_example.h is missing a trailing newline at end-of-file. This violates the source formatting requirement for consistent EOL handling and can cause tooling/diff issues.
Agent Prompt
## Issue description
The header file is missing a newline at end-of-file.
## Issue Context
Some compilers, linters, and patch tools expect LF-terminated final lines; missing EOL can create diff noise and portability issues.
## Fix Focus Areas
- examples/pru_i2c/example/pru_i2c_example.h[34-40]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+138
to
+144
| I2c_pruicssInit(); | ||
|
|
||
| while(1) | ||
| { | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
7. High-priority busy loop 🐞 Bug ☼ Reliability
The highest-priority FreeRTOS task calls pru_i2c_main(), which then spins forever in an empty while(1) without any blocking/yield, starving the idle task and all lower-priority tasks. This can break system behavior (timers/idle processing) and wastes CPU/power when running the example.
Agent Prompt
### Issue description
`pru_i2c_main()` runs in the highest-priority FreeRTOS task and contains a tight `while(1)` with no blocking call. This keeps the task runnable forever and prevents the idle task (and any lower-priority tasks) from executing.
### Issue Context
This is an example app, but as written it will consume 100% CPU and can destabilize the system if any other tasks/timers/idle-time work are expected.
### Fix Focus Areas
- examples/pru_i2c/example/pru_i2c_example.c[128-149]
- examples/pru_i2c/example/am261x-lp/r5fss0-0_freertos/main.c[40-75]
### Suggested fix
- Add a blocking call inside the loop, e.g. `vTaskDelay(pdMS_TO_TICKS(1));` (or an appropriate period) so the scheduler can run idle/other tasks.
- Optionally lower the task priority and/or restructure so `freertos_main()` creates worker tasks and then deletes itself, matching the comment in `main.c`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
achala-ti
changed the title
achala-ti
force-pushed
the
a0502729_PRU_I2C
branch
from
26b38ed to
641d03e
Compare
|
Code review by qodo was updated up to the latest commit 641d03e |
Comment on lines
+71
to
+75
| ENABLE_XIN_XOUT_SHITFTING .macro | ||
| LBCO &TEMP_REG1, ICSS_CFG_CONST, ICSS_CFG_SPPC, 4 | ||
| OR TEMP_REG1.b0, TEMP_REG1.b0, 0x02 | ||
| SBCO &TEMP_REG1, ICSS_CFG_CONST, ICSS_CFG_SPPC, 4 | ||
| .endm |
There was a problem hiding this comment.
1. Uppercase opcodes in macros 📘 Rule violation ⚙ Maintainability
New PRU assembly code uses uppercase instruction mnemonics (e.g., LBCO, OR, SBCO) instead of the required lowercase style. This violates the opcode casing consistency requirement for modified code.
Agent Prompt
## Issue description
Instruction opcodes in new/modified PRU assembly are written in uppercase, violating the required lowercase opcode style.
## Issue Context
The style rule requires consistent lowercase mnemonics for readability and review consistency.
## Fix Focus Areas
- examples/pru_i2c/firmware/pru_i2c_macro.h[71-75]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+96
to
+103
| I2C_WAIT_FOR_IEP_CMP .macro | ||
|
|
||
| WAIT_FOR_INT?: | ||
| LBCO &TEMP_REG1, ICSS_IEP_CONST, ICSS_IEP_CMP_STATUS_REG, 4 | ||
| QBBC WAIT_FOR_INT?, TEMP_REG1, PRU0_IEP_CMP_STATUS_BIT | ||
| ;;;;QBBC WAIT_FOR_INT?, R31, 31 | ||
|
|
||
| .endm |
There was a problem hiding this comment.
2. Pru1 iep compare mismatch 🐞 Bug ≡ Correctness
I2C_WAIT_FOR_IEP_CMP always polls PRU0_IEP_CMP_STATUS_BIT, while I2C_SETUP_IEP_COUNTER unconditionally programs PRU0_IEP_CMP_REG but conditionally enables PRU1 compare events; in PRU1 builds this can prevent any IEP compare event from being observed and the firmware can spin forever in the wait loop.
Agent Prompt
### Issue description
PRU1 firmware builds define `PRU1`, but `I2C_WAIT_FOR_IEP_CMP` always waits on `PRU0_IEP_CMP_STATUS_BIT`, and `I2C_SETUP_IEP_COUNTER` always writes the compare value to `PRU0_IEP_CMP_REG` while enabling PRU1 compare events. This mismatch can block PRU1 execution permanently in the wait loop.
### Issue Context
- PRU0/PRU1 builds are distinguished by preprocessor defines (`-DPRU0` vs `-DPRU1`) in the CCS projects.
- `PRU0_IEP_CMP_STATUS_BIT` and `PRU1_IEP_CMP_STATUS_BIT` are defined distinctly in `pru_i2c_main.asm`.
### Fix Focus Areas
- examples/pru_i2c/firmware/pru_i2c_macro.h[96-103]
- examples/pru_i2c/firmware/pru_i2c_macro.h[740-800]
- examples/pru_i2c/firmware/pru_i2c_main.asm[74-79]
### Suggested change
1. Update `I2C_WAIT_FOR_IEP_CMP` to select `PRU0_IEP_CMP_STATUS_BIT` vs `PRU1_IEP_CMP_STATUS_BIT` based on `$defined("PRU0")/$defined("PRU1")` (similar to `I2C_IEP_INTC_CLEAR_EVENT`).
2. Update `I2C_SETUP_IEP_COUNTER` to program `PRU0_IEP_CMP_REG` for PRU0 builds and `PRU1_IEP_CMP_REG` for PRU1 builds, and ensure the enabled compare event matches the programmed compare register.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+39
to
+44
| # Defines (pass instance specific definitions to the program) | ||
| # see --define or -D in 'PRU Optimizing C/C++ Compiler User's Guide' | ||
| # For example, to define PRU0, ICSSG1, SOC_AM243X, and set _DEBUG_=1: | ||
| # -DPRU0 -DICSSG1 --define=SOC_AM243X -D_DEBUG_=1 | ||
| DFLAGS := | ||
|
|
There was a problem hiding this comment.
3. Makefiles omit pru defines 🐞 Bug ≡ Correctness
The new ti-pru-cgt makefiles leave DFLAGS empty even though the firmware uses
$defined("PRU0")/$defined("PRU1") to select core-specific behavior, so make builds will not set
these symbols unless the caller injects them externally.
Agent Prompt
### Issue description
The PRU I2C firmware uses `$defined("PRU0")` / `$defined("PRU1")` conditionals to choose PRU-core-specific behavior (IEP status bits, compare registers, interrupt bits). However, both new makefiles set `DFLAGS :=` empty, and the shared build rules only pass whatever `DFLAGS` contains to `clpru`.
### Issue Context
- CCS projects pass `-DPRU0`/`-DPRU1`, but the provided makefiles currently do not.
- `pru_rules.mak` compiles with `$(DFLAGS)` as provided by the core makefile.
### Fix Focus Areas
- examples/pru_i2c/firmware/am261x-lp/icss_m0_pru0_fw/ti-pru-cgt/makefile[39-44]
- examples/pru_i2c/firmware/am261x-lp/icss_m0_pru1_fw/ti-pru-cgt/makefile[39-44]
- pru_rules.mak[126-130]
### Suggested change
- In the PRU0 makefile, set:
- `DFLAGS := -DPRU0 -DSLICE0`
- In the PRU1 makefile, set:
- `DFLAGS := -DPRU1 -DSLICE1`
- If other symbols are required to match CCS builds, add them here as well, but at minimum the PRU0/PRU1 defines should be present so the firmware compiles with the intended core-specific branches.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
- Add I2C firmware - Create PRU0/PRU1 build projects for ICSS_M0 - Configure makefiles and projectspec for open-pru structure Signed-off-by: Achala Ram <a-ram@ti.com>
achala-ti
force-pushed
the
a0502729_PRU_I2C
branch
from
641d03e to
9a16e00
Compare
|
Code review by qodo was updated up to the latest commit 9a16e00 |
| " | ||
|
|
||
| postBuildStep=" | ||
| $(CG_TOOL_ROOT)/bin/hexpru --diag_wrap=off --array --array:name_prefix=PRU0Firmware -o ../pru0_load_bin.h ${BuildArtifactFileBaseName}.out; |
There was a problem hiding this comment.
1. postbuildstep line not wrapped 📘 Rule violation ⚙ Maintainability
The postBuildStep command is added as a single long line rather than being wrapped/split using a supported continuation mechanism. This reduces readability and violates the project configuration line-wrapping requirement.
Agent Prompt
## Issue description
A very long `postBuildStep` command is written on a single line in the project configuration instead of being wrapped/split using a supported continuation mechanism.
## Issue Context
PR Compliance ID 22 expects long command attributes (such as post-build steps) to be wrapped/split to meet repository line-length expectations and keep project specs readable.
## Fix Focus Areas
- examples/pru_i2c/firmware/am261x-lp/icss_m0_pru0_fw/ti-pru-cgt/example.projectspec[56-58]
- examples/pru_i2c/firmware/am261x-lp/icss_m0_pru1_fw/ti-pru-cgt/example.projectspec[56-58]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| FIRMWARE_VERSION_BUILD .set 0x00 | ||
|
|
||
| ; macro for indicating version of firmware | ||
| I2C_FIRMWARE_VERSION .set ((FIRMWARE_VERSION_RELEASE << 31) | (FIRMWARE_VERSION_MAJOR << 24) | (FIRMWARE_VERSION_MINOR << 16) | (FIRMWARE_VERSION_BUILD << 0)) |
There was a problem hiding this comment.
2. i2c_firmware_version line too long 📘 Rule violation ⚙ Maintainability
The I2C_FIRMWARE_VERSION .set expression is written as a single very long source line, exceeding the repository column limits. This harms readability and violates the line-length requirement.
Agent Prompt
## Issue description
A single source line exceeds the repository column limits.
## Issue Context
Long expressions should be wrapped/split (for example by using intermediate `.set` symbols) to stay within the soft/hard line-length limits.
## Fix Focus Areas
- examples/pru_i2c/firmware/pru_i2c_firmware_version.h[50-51]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+15
to
+20
| PAGE 1: | ||
| /* Data RAMs */ | ||
| /* 8 KB PRU Data RAM 0 */ | ||
| PRU0_DMEM_0 : org = 0x00000000 len = 0x00002000 | ||
| /* 8 KB PRU Data RAM 1 */ | ||
| PRU1_DMEM_1 : org = 0x00002000 len = 0x00002000 |
There was a problem hiding this comment.
3. Pru1 linker dmem swapped 🐞 Bug ≡ Correctness
The PRU1 linker.cmd maps PRU0_DMEM_0 at 0x0 and PRU1_DMEM_1 at 0x2000, but PRU1 sections are allocated into PRU1_DMEM_1, so PRU1 stack/BSS can end up in the wrong local RAM alias. This differs from other AM261x PRU1 linker scripts in the repo (which place PRU1_DMEM_1 at 0x0), and can lead to PRU1 firmware malfunction or memory corruption.
Agent Prompt
## Issue description
`examples/pru_i2c/firmware/am261x-lp/icss_m0_pru1_fw/ti-pru-cgt/linker.cmd` uses the same DMEM origin layout as PRU0 (DMEM0 at 0x0, DMEM1 at 0x2000). For PRU1 builds, the repo’s existing AM261x PRU1 linker scripts map PRU1’s local DMEM (PRU1_DMEM_1) at 0x0 and PRU0_DMEM_0 at 0x2000.
This mismatch can cause PRU1 `.stack/.bss/.data/...` (allocated to `PRU1_DMEM_1`) to land in the wrong address range.
## Issue Context
The same-repo reference implementation for AM261x PRU1 (e.g., CRC firmware) shows the expected mapping.
## Fix Focus Areas
- examples/pru_i2c/firmware/am261x-lp/icss_m0_pru1_fw/ti-pru-cgt/linker.cmd[15-21]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Hi @achala-ti — nice work on the PRU I2C master. One thing that's blocking the am261x CI here: the "Verify all makefiles were built" step fails with "makefile never built" for the two Root cause: there's no Two small changes fix it:
The am261x:
# am261x-lp
$(MAKE) -C firmware/am261x-lp/icss_m0_pru0_fw/ti-pru-cgt $(ARGUMENTS_PRU)
$(MAKE) -C firmware/am261x-lp/icss_m0_pru1_fw/ti-pru-cgt $(ARGUMENTS_PRU)Verified locally with GNU Make dry-runs: am261x recurses into both firmware leaf makefiles (each producing a Happy to send this as a patch/PR against your branch if useful — let me know. (Separately, the PR body mentions an R5F example but the current diff looks firmware-only — is the R5F example still coming?) |
The am261x "Verify all makefiles were built" CI step failed with "makefile never built" for the two pru_i2c PRU firmware makefiles, because nothing in the top-level make recursed into them. - Add examples/pru_i2c/makefile aggregator (modeled on examples/empty), scoped to am261x with an am261x target that builds the pru0/pru1 ti-pru-cgt firmware. Non-am261x devices hit the SUPPORTED_PROCESSORS prebuild check and skip cleanly, so other device builds are unaffected. - Add pru_i2c to examples/makefile SUBDIRS so the top-level make recurses into it. Verified with GNU Make dry-runs: am261x recurses into both firmware leaf makefiles (each producing a .out, satisfying the verify step); am263x prints "does not have a build option" and builds nothing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment on lines
+31
to
+35
| ;************************************************************************************ | ||
| ; File: icss_i2c_macro.h | ||
| ; | ||
| ; Brief: This file contains ICSS I2C macros definations | ||
| ;************************************************************************************ |
There was a problem hiding this comment.
2. pru_i2c_macro.h header filename mismatch 📘 Rule violation ⚙ Maintainability
The file header comment states a different filename (icss_i2c_macro.h) than the actual file (pru_i2c_macro.h). This breaks the required formatting standard for accurate file-name headers.
Agent Prompt
## Issue description
The file header comment lists the wrong filename, which violates the formatting/header requirements.
## Issue Context
The repository formatting standard requires source files to contain a header denoting the correct file name.
## Fix Focus Areas
- examples/pru_i2c/firmware/pru_i2c_macro.h[31-35]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+58
to
+85
| .asg R1, TEMP_REG1 ; temporary register 1 | ||
| .asg R2, TEMP_REG2 ; temporary register 2 | ||
| .asg R3, TEMP_REG3 ; temporary register 3 | ||
| .asg R4, TEMP_REG4 ; temporary register 4 | ||
| .asg R5, TEMP_REG5 ; temporary register 5 | ||
| .asg R6, TEMP_REG6 ; temporary register 6 | ||
| .asg R7, TEMP_REG7 ; temporary register 7 | ||
| .asg R8, TEMP_REG8 ; temporary register 8 | ||
|
|
||
| .asg R20, IEP_COUNTER_NEXT_VAL1 | ||
| .asg R21, IEP_COUNTER_NEXT_VAL2 | ||
| .asg R22, I2C_GLOBAL_FREQ_REG | ||
|
|
||
|
|
||
|
|
||
|
|
||
| PRU0_IEP_CMP_REG .set ICSS_IEP_CMP0_REG | ||
| PRU1_IEP_CMP_REG .set ICSS_IEP_CMP1_REG | ||
| PRU0_IEP_CMP_ENABLE_BIT .set 1 | ||
| PRU1_IEP_CMP_ENABLE_BIT .set 2 | ||
| PRU0_IEP_CMP_STATUS_BIT .set 0 | ||
| PRU1_IEP_CMP_STATUS_BIT .set 1 | ||
|
|
||
|
|
||
| ; Bank ids for Xfer instructions | ||
| BANK0 .set 10 | ||
| BANK1 .set 11 | ||
| BANK2 .set 12 |
There was a problem hiding this comment.
3. pru_i2c_main.asm register aliasing 📘 Rule violation ⚙ Maintainability
The new PRU assembly introduces direct register aliasing via .asg/.set rather than structured allocation. This increases the risk of register overlap/corruption and reduces reviewability.
Agent Prompt
## Issue description
Direct register aliasing is used (`.asg` and `.set`) instead of structured register allocation.
## Issue Context
The compliance guideline prefers `.struct`/`.sassign` to reduce register overlap and improve maintainability.
## Fix Focus Areas
- examples/pru_i2c/firmware/pru_i2c_main.asm[58-85]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+78
to
+103
| ;************************************************************************************ | ||
| ; | ||
| ; Macro: I2C_WAIT_FOR_IEP_CMP | ||
| ; | ||
| ; wait until the IEP CMP Event triggers and interrupt | ||
| ; | ||
| ; PEAK cycles: | ||
| ; NA | ||
| ; Pseudo code: | ||
| ; while(r31.t31==0) | ||
| ; | ||
| ; Parameters: | ||
| ; None | ||
| ; | ||
| ; Returns: | ||
| ; None | ||
| ; | ||
| ;************************************************************************************ | ||
| I2C_WAIT_FOR_IEP_CMP .macro | ||
|
|
||
| WAIT_FOR_INT?: | ||
| LBCO &TEMP_REG1, ICSS_IEP_CONST, ICSS_IEP_CMP_STATUS_REG, 4 | ||
| QBBC WAIT_FOR_INT?, TEMP_REG1, PRU0_IEP_CMP_STATUS_BIT | ||
| ;;;;QBBC WAIT_FOR_INT?, R31, 31 | ||
|
|
||
| .endm |
There was a problem hiding this comment.
4. i2c_wait_for_iep_cmp lacks timeout 📘 Rule violation ☼ Reliability
The macro busy-waits in a loop without an explicit termination/timeout, and its documentation does not provide a real peak cycle budget nor list modified registers. This risks deadlock if the event never occurs and violates required macro documentation.
Agent Prompt
## Issue description
`I2C_WAIT_FOR_IEP_CMP` performs an unbounded busy-wait loop and its documentation is incomplete (no real peak cycle budget, no modified-register list).
## Issue Context
Compliance requires PRU loops to have explicit termination/bounds checks and requires macros to document peak cycle budget and modified registers.
## Fix Focus Areas
- examples/pru_i2c/firmware/pru_i2c_macro.h[78-103]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1375
to
+1380
| LOOPBACK_COPY_DATA: | ||
| LBBO &R15.b1, R11, R15.b2, 1 | ||
| SBBO &R15.b1, R12, R15.b2, 1 | ||
| ADD R15.b2, R15.b2, 1 | ||
| QBGE LOOPBACK_COPY_DATA_RETURN, R15.b3, R15.b2 | ||
| STATE_TASK_OVER |
There was a problem hiding this comment.
5. loopback_copy_data unchecked buffer access 📘 Rule violation ⛨ Security
The loopback copy performs LBBO/SBBO using a loop-derived offset (R15.b2) without verifying the offset is in-bounds before each access and without validating the requested count against the actual TX/RX buffer size. This can lead to out-of-bounds reads/writes if configuration is invalid or corrupted.
Agent Prompt
## Issue description
`LOOPBACK_COPY_DATA` reads/writes bytes from TX/RX buffers using an incrementing offset without checking bounds before the memory accesses, and without validating the requested length against the buffer capacity.
## Issue Context
PRU firmware must prevent out-of-bounds memory access and ensure loop-derived offsets are validated before accessing memory.
## Fix Focus Areas
- examples/pru_i2c/firmware/pru_i2c_main.asm[1357-1380]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+96
to
+103
| I2C_WAIT_FOR_IEP_CMP .macro | ||
|
|
||
| WAIT_FOR_INT?: | ||
| LBCO &TEMP_REG1, ICSS_IEP_CONST, ICSS_IEP_CMP_STATUS_REG, 4 | ||
| QBBC WAIT_FOR_INT?, TEMP_REG1, PRU0_IEP_CMP_STATUS_BIT | ||
| ;;;;QBBC WAIT_FOR_INT?, R31, 31 | ||
|
|
||
| .endm |
There was a problem hiding this comment.
6. Wrong cmp status bit 🐞 Bug ≡ Correctness
I2C_WAIT_FOR_IEP_CMP always polls PRU0_IEP_CMP_STATUS_BIT, so a PRU1 firmware build can wait on the wrong compare-status bit and never observe its own CMP event. This can deadlock the PRU1 task loop at startup.
Agent Prompt
### Issue description
`I2C_WAIT_FOR_IEP_CMP` polls `PRU0_IEP_CMP_STATUS_BIT` unconditionally. For PRU1, the CMP status bit is different (PRU1 bit), so PRU1 may spin forever waiting for an event that will never set the polled bit.
### Issue Context
`pru_i2c_main.asm` defines distinct status bits for PRU0 vs PRU1, and the firmware has separate PRU0/PRU1 build projects.
### Fix Focus Areas
- examples/pru_i2c/firmware/pru_i2c_macro.h[96-103]
- examples/pru_i2c/firmware/pru_i2c_main.asm[74-79]
### Suggested fix
Make `I2C_WAIT_FOR_IEP_CMP` choose the correct status bit based on `$defined("PRU0")/$defined("PRU1")` (mirroring the pattern already used in `I2C_IEP_INTC_CLEAR_EVENT`), or parameterize the macro to accept the status-bit constant.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+784
to
+788
| loop4?: | ||
| ;;ADD IEP_COUNTER_NEXT_VAL1, IEP_COUNTER_NEXT_VAL1, I2C_GLOBAL_FREQ_REG.w2 | ||
| ;;ADC IEP_COUNTER_NEXT_VAL2, IEP_COUNTER_NEXT_VAL2, 0x00 | ||
| SBCO &IEP_COUNTER_NEXT_VAL1, ICSS_IEP_CONST, PRU0_IEP_CMP_REG, 8 | ||
|
|
There was a problem hiding this comment.
7. Wrong cmp register programmed 🐞 Bug ≡ Correctness
I2C_SETUP_IEP_COUNTER writes the initial compare value to PRU0_IEP_CMP_REG unconditionally, so PRU1 never programs PRU1_IEP_CMP_REG. This can prevent PRU1 compare events from firing on the intended channel.
Agent Prompt
### Issue description
`I2C_SETUP_IEP_COUNTER` always writes compare values to `PRU0_IEP_CMP_REG`. When building for PRU1, the compare register is `PRU1_IEP_CMP_REG`, so PRU1’s compare channel may never be configured.
### Issue Context
`pru_i2c_main.asm` maps `PRU0_IEP_CMP_REG` to `ICSS_IEP_CMP0_REG` and `PRU1_IEP_CMP_REG` to `ICSS_IEP_CMP1_REG`.
### Fix Focus Areas
- examples/pru_i2c/firmware/pru_i2c_macro.h[740-802]
- examples/pru_i2c/firmware/pru_i2c_main.asm[74-79]
### Suggested fix
Conditionalize the `SBCO` target register in `I2C_SETUP_IEP_COUNTER` the same way `I2C_IEP_INTC_CLEAR_EVENT` does:
- if PRU0: write `PRU0_IEP_CMP_REG`
- if PRU1: write `PRU1_IEP_CMP_REG`
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+768
to
+776
| LDI TEMP_REG3.w0, ICSS_I2C_CONFIG_MEMORY | ||
| LBCO &IEP_COUNTER_NEXT_VAL1, ICSS_DMEM0_CONST, TEMP_REG3.w0, 8 | ||
| LBCO &TEMP_REG2, ICSS_IEP_CONST, ICSS_IEP_COUNT_REG, 8 | ||
|
|
||
| ;;;debug code | ||
| ;;.if $defined("DEBUG_CODE") | ||
| LDI IEP_COUNTER_NEXT_VAL1, 0x9c3 | ||
| LDI IEP_COUNTER_NEXT_VAL2, 0x0 | ||
| ;;.endif |
There was a problem hiding this comment.
8. Unconditional iep value override 🐞 Bug ≡ Correctness
I2C_SETUP_IEP_COUNTER unconditionally overwrites IEP_COUNTER_NEXT_VAL1/2 with 0x9c3/0, discarding the values loaded from DMEM. This forces initial compare scheduling toward the 100kHz constant and can produce incorrect timing relative to the configured bus frequency.
Agent Prompt
### Issue description
`I2C_SETUP_IEP_COUNTER` loads `IEP_COUNTER_NEXT_VAL1/2` from DMEM, but then immediately overwrites them with hardcoded values (`0x9c3`, `0x0`) because the intended `.if $defined("DEBUG_CODE")` guard is commented out.
### Issue Context
`0x9c3` matches the 100kHz increment constant in `pru_i2c_interface.h`, but the firmware supports multiple frequencies and uses `I2C_GLOBAL_FREQ_REG` to select among them.
### Fix Focus Areas
- examples/pru_i2c/firmware/pru_i2c_macro.h[768-776]
- examples/pru_i2c/firmware/pru_i2c_interface.h[175-193]
### Suggested fix
Either:
1) Restore the debug guard (uncomment/fix the `.if $defined("DEBUG_CODE")` / `.endif` so the hardcoded load only happens in debug builds), or
2) Remove the hardcoded overwrite entirely and initialize the compare value from DMEM/current IEP count + configured increment.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.