Am263x: PRU-I2S#8
Open
rajul-bhambay wants to merge 3 commits intomainfrom
Open
Conversation
Collaborator
Author
Pending Items: |
rajul-bhambay
force-pushed
the
a0497643_PINDSW-8487_PRU_I2S
branch
2 times, most recently
from
96bf0b7 to
a27184b
Compare
Collaborator
|
Hey @rajul-bhambay, what is TDM4? I assume something to do with time-domain multiplexing, instead of a typo for TDA4? |
Collaborator
Author
Hey @nsaulnier-ti , TDM4 (time division multiplexing 4 channels) is a mode of communication of audio signals. We have 2 modes, I2S and TDM4/TDM8 which is suppported by the TI's TAS6424Q1 coded (class-D amplifier). |
rajul-bhambay
force-pushed
the
a0497643_PINDSW-8487_PRU_I2S
branch
2 times, most recently
from
26e87e1 to
29b5691
Compare
dhavaljk
requested changes
Jun 24, 2025
| @@ -0,0 +1,186 @@ | |||
| %%{ | |||
Collaborator
There was a problem hiding this comment.
I don't think we need this file. Can we remove?
| @@ -0,0 +1,77 @@ | |||
| %%{ | |||
Collaborator
There was a problem hiding this comment.
I don't think we need this file. Can we remove?
| @@ -0,0 +1,52 @@ | |||
| /* | |||
Collaborator
There was a problem hiding this comment.
I don't think we need this file. Can we remove?
| @@ -0,0 +1,280 @@ | |||
| /* | |||
| * Copyright (C) 2021 Texas Instruments Incorporated | |||
Collaborator
There was a problem hiding this comment.
Update copyright year in all files
Collaborator
There was a problem hiding this comment.
Move this file to board sub-folder as it is not a core example function
| /* Function Definitions */ | ||
| /* ========================================================================== */ | ||
|
|
||
| int32_t TCA6416_open(TCA6416_Config *config, const TCA6416_Params *params) |
Collaborator
There was a problem hiding this comment.
Do we really need this code now? Is this not supported in MCU+ SDK SysConfig under IOEXP module?
| #define TEST_PRUI2S0_IDX ( 0 ) /* Test PRU I2S 0 index */ | ||
| #define TEST_PRUI2S1_IDX ( 1 ) /* Test PRU I2S 1 index */ | ||
|
|
||
| #define TDM4 ( 1 ) /* TDM4 mode (change to 0 for I2S)*/ |
Collaborator
There was a problem hiding this comment.
I assume we have tested both modes. Can you confirm?
| */ | ||
|
|
||
|
|
||
| void i2s_i2c_io_expander(void) |
Collaborator
There was a problem hiding this comment.
Let's review if this can be removed - by using SysConfig
| /* debug, increment ISR count */ | ||
| gPruI2s1RxIsrCnt++; | ||
| /* debug, drive GPIO high */ | ||
| GPIO_pinWriteHigh(CONFIG_GPIO_DEBUG1_BASE_ADDR, CONFIG_GPIO_DEBUG1_PIN); |
Collaborator
There was a problem hiding this comment.
Can we put this GPIO code under some debug macro? Else it adds unnecessary latency to ISRs
| @@ -0,0 +1,200 @@ | |||
| /* | |||
Collaborator
There was a problem hiding this comment.
Can we move this to SysConfig?
Collaborator
|
Can we merge commit? Don't think we need 4 |
dhavaljk
requested review from
dhavaljk,
nilabhanand-ti,
nsaulnier-ti,
pratheesh-ti and
tleyrer
rajul-bhambay
force-pushed
the
a0497643_PINDSW-8487_PRU_I2S
branch
3 times, most recently
from
cd4d050 to
172a2d0
Compare
pratheesh-ti
requested changes
Jun 27, 2025
Collaborator
pratheesh-ti
left a comment
pratheesh-ti
left a comment
There was a problem hiding this comment.
Complete the documentation changes to merge
| @@ -0,0 +1,2692 @@ | |||
| /* | |||
| * Copyright (C) 2021 Texas Instruments Incorporated | |||
Collaborator
There was a problem hiding this comment.
Check the copyright year
|
|
||
| #include <stdint.h> | ||
| #include <drivers/hw_include/cslr.h> | ||
| #include <drivers\hw_include\am263x\cslr_iomux.h> |
Collaborator
There was a problem hiding this comment.
Be consistent with use of "/" vs ""
| #include <kernel/dpl/SemaphoreP.h> | ||
| #include <drivers\hw_include\am263px\cslr_intr_r5fss0_core0.h> | ||
| #include <drivers\pinmux\am263x\pinmux.h> | ||
| //#include "ti_drivers_config.h" |
Collaborator
There was a problem hiding this comment.
Remove if not relevant
| #define PRUI2S_PRU_INTC_SYSEVT2_IDX ( 2 ) /* I2S error system event index */ | ||
|
|
||
| /* Number INTC channels per PRU */ | ||
| #define PRUI2S_PRUICSS_INTC_NUM_CHANNELS_PER_PRU \ |
Collaborator
There was a problem hiding this comment.
Next line is not really warranted here?
| pSwipAttrs->rxPin[i].pinNum = temp8b; | ||
| } | ||
|
|
||
| /* TBD: SoC PAD address lookup */ |
Collaborator
There was a problem hiding this comment.
Remove if not relevant anymore
Collaborator
There was a problem hiding this comment.
This file seems to redefine a bunch of functions definted in pruicss driver in MCU+ SDK. Is this intentional?
| @@ -0,0 +1,200 @@ | |||
| /* | |||
| * Copyright (c) 2021, Texas Instruments Incorporated | |||
| @@ -0,0 +1,280 @@ | |||
| /* | |||
| * Copyright (C) 2021 Texas Instruments Incorporated | |||
Collaborator
There was a problem hiding this comment.
Move this file to board sub-folder as it is not a core example function
| @@ -0,0 +1,113 @@ | |||
| ; | |||
| ; Copyright (c) 2021, Texas Instruments Incorporated | |||
| ; All rights reserved. | |||
Collaborator
There was a problem hiding this comment.
Add firmware design document in md format here under firmware/I2S/docs
| * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. | ||
| */ | ||
|
|
||
| #include <stdio.h> |
Collaborator
There was a problem hiding this comment.
Add Readme.md following other examples in open-pru, explaining the examples and other details
rajul-bhambay
force-pushed
the
a0497643_PINDSW-8487_PRU_I2S
branch
from
034f165 to
52e4c10
Compare
pratheesh-ti
previously approved these changes
Jul 11, 2025
Collaborator
|
@dhavaljk @manojKoppolu @rajul-bhambay let's discuss the overall structure of this project and the source folder, perhaps on Wednesday's meeting? I am trying to understand the logic of the overall project structure (e.g., not actually including any PRU code under examples, keeping it all under source), and function of different folders (especially .meta). If we want to keep the proposed structure of "source" [1], we would need to move the files a bit: source
[1] https://confluence.itg.ti.com/display/ProcSW/OpenPRU+Repository+Structure+Proposal |
nsaulnier-ti
requested changes
Jul 15, 2025
Collaborator
nsaulnier-ti
left a comment
nsaulnier-ti
left a comment
There was a problem hiding this comment.
I want to understand the logic behind the folder structures before we merge this in. Best to discuss verbally in the team meeting.
Collaborator
|
/review |
|
Persistent review updated to latest commit 4eff401 |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+157
to
+183
| INITIAL_STATE: | ||
| ; start iterating | ||
| ; wait until the input BCLK is low | ||
| ; then branch to wait for the rising edge | ||
| QBBS INITIAL_STATE, r31, I2S_INSTANCE_BCLK_PIN | ||
|
|
||
| ;Wait until there are 4 transitions of FS H-L-H-L-H | ||
| INITIAL_STATE_FS: | ||
| ; start iterating | ||
| ; wait until the input FS is low | ||
| ; then branch to wait for the rising edge | ||
| QBBS INITIAL_STATE_FS, r31, I2S_INSTANCE_FS_PIN | ||
|
|
||
| INITIAL_STATE_FS1: | ||
| ; wait until the input FS is high | ||
| QBBC INITIAL_STATE_FS1, r31, I2S_INSTANCE_FS_PIN | ||
|
|
||
| INITIAL_STATE_FS2: | ||
| ; wait until the input FS is low | ||
| QBBS INITIAL_STATE_FS2, r31, I2S_INSTANCE_FS_PIN | ||
|
|
||
| BCLK_RISING_EDGE_HIGH1: | ||
| QBBS BCLK_RISING_EDGE_HIGH1, r31, I2S_INSTANCE_BCLK_PIN | ||
|
|
||
| BCLK_RISING_EDGE_LOW: | ||
| ; wait until we see a high value | ||
| QBBC BCLK_RISING_EDGE_LOW, r31, I2S_INSTANCE_BCLK_PIN |
There was a problem hiding this comment.
1. qbbs wait loops unbounded 📘 Rule violation ⛯ Reliability
The firmware contains self-branching wait loops (e.g., waiting for BCLK/FS transitions) with no explicit termination/timeout condition. This can lead to runaway execution if the expected signal never arrives.
Agent Prompt
## Issue description
Signal-wait loops can run forever if BCLK/FS is missing or stuck.
## Issue Context
Several labels (`INITIAL_STATE`, `INITIAL_STATE_FS*`, `BCLK_RISING_EDGE_*`) busy-wait using self-branches without any bounded termination.
## Fix Focus Areas
- examples/pru_i2s/firmware/I2S/pru_i2s_main.asm[157-183]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+42
to
+53
| main: | ||
| .if $isdefed("I2S_TX") | ||
| ;err_stat = err_stat >> 1. Remove the Rx Overflow error bit. | ||
| LSR err_stat, err_stat, 1 | ||
| ;If there was an underflow/FrameSync Error, clearup thr Tx PingPong Buffer space | ||
| ;err_stat is already initialized during normal operation. | ||
| ; FS_ERROR UNDERFLOW_ERROR | ||
| ; 0 0 No Error | ||
| ; 0 1 Under Flow error | ||
| ; 1 0 FS Error | ||
| ; 1 1 Both errors | ||
| QBEQ CONTIUNE_INIT, err_stat, 0 |
There was a problem hiding this comment.
2. err_stat used before init 📘 Rule violation ⛯ Reliability
Startup code uses err_stat before the register-clearing ZERO &r0, 128 executes. This violates the requirement to explicitly initialize register state at startup and may cause unpredictable control flow on cold boot.
Agent Prompt
## Issue description
`err_stat` is used prior to the global register clear/initialization.
## Issue Context
The register clear occurs at `CONTIUNE_INIT`, but `err_stat` is shifted and compared earlier.
## Fix Focus Areas
- examples/pru_i2s/firmware/I2S/pru_i2s_main.asm[42-74]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1
to
+3
| ; | ||
| ; Copyright (C) 2024-2025 Texas Instruments Incorporated | ||
| ; |
There was a problem hiding this comment.
3. Missing file-name header fields 📘 Rule violation ⚙ Maintainability
Several newly-added source/header files include a TI copyright header but do not state the file name in the header. This violates the required file header content for traceability/legal compliance.
Agent Prompt
## Issue description
File headers are missing an explicit file-name line.
## Issue Context
Compliance requires each source/header to include both the file name and TI copyright.
## Fix Focus Areas
- examples/pru_i2s/firmware/I2S/pru_i2s_main.asm[1-3]
- examples/pru_i2s/firmware/I2S/pru_i2s_interface.h[1-3]
- examples/pru_i2s/firmware/I2S/pru_i2s_regs.h[1-3]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+42
to
+46
| main: | ||
| .if $isdefed("I2S_TX") | ||
| ;err_stat = err_stat >> 1. Remove the Rx Overflow error bit. | ||
| LSR err_stat, err_stat, 1 | ||
| ;If there was an underflow/FrameSync Error, clearup thr Tx PingPong Buffer space |
There was a problem hiding this comment.
4. main label/opcodes mis-cased 📘 Rule violation ⚙ Maintainability
The PRU assembly uses a lowercase label (main:) and uppercase opcodes (e.g., LSR, LDI) instead of the required casing conventions. This violates PRU assembly naming/style rules and reduces consistency/searchability.
Agent Prompt
## Issue description
PRU assembly label/opcode casing does not match required conventions.
## Issue Context
Compliance expects uppercase labels and lowercase opcodes for consistency.
## Fix Focus Areas
- examples/pru_i2s/firmware/I2S/pru_i2s_main.asm[42-46]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+114
to
+167
| if ((pSwipAttrs->pruInstId == PRUICSS_PRU0) && ((*gPruFwImageInfo)[0].pPruImemImg != NULL)) | ||
| { | ||
| /* Update base address */ | ||
| pSwipAttrs->baseAddr = (uint32_t)AddrTranslateP_getLocalAddr(pSwipAttrs->baseAddr); | ||
|
|
||
| /* Parse info in PRU image #0, update SW IP info */ | ||
| status = PRUI2S_getPruFwImageInfo(&((*gPruFwImageInfo)[0]), pSwipAttrs); | ||
| if (status != SystemP_SUCCESS) | ||
| { | ||
| retStatus = PRUI2S_DRV_SERR_INIT; | ||
| break; | ||
| } | ||
|
|
||
| /* Set FW image pointer */ | ||
| pObj->pPruFwImageInfo = &((*gPruFwImageInfo)[0]); | ||
| } | ||
| else if ((pSwipAttrs->pruInstId == PRUICSS_PRU1) && ((*gPruFwImageInfo)[1].pPruImemImg != NULL)) | ||
| { | ||
| /* Update base address */ | ||
| pSwipAttrs->baseAddr = (uint32_t)AddrTranslateP_getLocalAddr(pSwipAttrs->baseAddr); | ||
|
|
||
| /* Parse info in PRU image #1, update SW IP info */ | ||
| status = PRUI2S_getPruFwImageInfo(&((*gPruFwImageInfo)[1]), pSwipAttrs); | ||
| if (status != SystemP_SUCCESS) | ||
| { | ||
| retStatus = PRUI2S_DRV_SERR_INIT; | ||
| break; | ||
| } | ||
|
|
||
| /* Set FW image pointer */ | ||
| pObj->pPruFwImageInfo = &((*gPruFwImageInfo)[1]); | ||
| } | ||
| else | ||
| { | ||
| /* Error, no PRU FW image for selected core type */ | ||
| retStatus = PRUI2S_DRV_SERR_INIT_FWIMG; | ||
| break; | ||
| } | ||
|
|
||
| /* Check SW IP parameters */ | ||
| status = PRUI2S_checkSwipParams(pSwipAttrs); | ||
| if (status != SystemP_SUCCESS) | ||
| { | ||
| retStatus = PRUI2S_DRV_SERR_INIT; | ||
| break; | ||
| } | ||
|
|
||
| gPruI2sDrvNumValidCfg++; | ||
| } | ||
|
|
||
| *pNumValidCfg = gPruI2sDrvNumValidCfg; | ||
| if (retStatus == PRUI2S_DRV_SOK) | ||
| { | ||
| gPruI2sDrvInit = TRUE; |
There was a problem hiding this comment.
5. Init count out-of-bounds 🐞 Bug ✓ Correctness
PRUI2S_init increments gPruI2sDrvNumValidCfg without resetting it when initialization fails and is retried. Since PRUI2S_open trusts gPruI2sDrvNumValidCfg for bounds checking, repeated failed init attempts can allow out-of-bounds indexing into gPruI2sConfig.
Agent Prompt
## Issue description
`PRUI2S_init()` increments `gPruI2sDrvNumValidCfg` but does not reset it at the start of init attempts. If init fails (so `gPruI2sDrvInit` stays FALSE) and init is retried, the count can accumulate across attempts. `PRUI2S_open()` uses this count for bounds checking and then indexes `gPruI2sConfig[index]`, so an inflated count can allow out-of-bounds access.
## Issue Context
This is an error-recovery path bug: it may not show up during successful init, but it breaks retry logic and can turn a recoverable init failure into memory corruption.
## Fix Focus Areas
- examples/pru_i2s/driver/pru_i2s_drv.c[61-173]
- examples/pru_i2s/driver/pru_i2s_drv.c[284-290]
## Suggested fix
- At the beginning of `PRUI2S_init()` (inside the `gPruI2sDrvInit == FALSE` block), set `gPruI2sDrvNumValidCfg = 0;`.
- Add defensive bounds in `PRUI2S_open()` as well (e.g., `index < PRU_I2S_NUM_CONFIG` AND `index < gPruI2sDrvNumValidCfg`).
- Optionally, if init fails after constructing per-instance mutexes, consider cleaning up those constructed mutexes before returning to avoid leaking/duplicating state across retries.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Review Summary by QodoPRU I2S Driver and Firmware Implementation for AM263X and AM261X
WalkthroughsDescription• Complete PRU I2S driver implementation with comprehensive API for initialization, configuration, and data transfer operations supporting both interleaved and non-interleaved data formats • PRU firmware implementation for both I2S and TDM4 modes with assembly code, register definitions, and compiled binary images for PRU0 and PRU1 • Diagnostic application demonstrating PRU I2S Tx/Rx loopback with dual instance support, interrupt handlers, and semaphore-based synchronization • TCA6416 I2C IO expander driver for GPIO pin configuration with thread-safe operations • Interrupt management infrastructure including PRU-ARM event mappings, INTC configuration, and host interrupt notifications • FreeRTOS application support for AM263X and AM261X boards with main entry points and system initialization • Build infrastructure including linker command files, makefiles, and project specifications for both firmware and application compilation • Product metadata updates to include PRU component references Diagramflowchart LR
A["PRU I2S Driver<br/>pru_i2s_drv.c/h"] --> B["Firmware Layer<br/>I2S & TDM4 Modes"]
B --> C["PRU0/PRU1<br/>Binary Images"]
A --> D["Interrupt Management<br/>INTC Mapping"]
A --> E["Application<br/>Diagnostic App"]
E --> F["Board Support<br/>IO Expander"]
E --> G["FreeRTOS<br/>AM263X/AM261X"]
H["Data Structures<br/>Buffers & Config"] --> A
H --> E
File Changes1. examples/pru_i2s/driver/pru_i2s_drv.c
|
|
Persistent review updated to latest commit 35cb893 |
Comment on lines
+109
to
+146
| .if $isdefed("I2S_TX") | ||
| ;read the Tx buffer address provided by host. This should be in Shared memory | ||
| ; point to the PING audio buffer | ||
| LDI scratchreg0, I2S_TX_BUF_PING_ADD | ||
| LBBO &tx_buffer_address, scratchreg0, 0, 4 | ||
| MOV tx_ping_buffer_address, tx_buffer_address | ||
| .endif | ||
|
|
||
| ;read the buffer size provided by host. buffer size is ping+pong | ||
| LDI scratchreg0, I2S_PING_PONG_BUFSIZE_ADD | ||
| LBBO &tx_buf_size, scratchreg0, 0, 2 | ||
| ;Shift right by 1 to get PING/PONG size buffer size | ||
| LSR tx_buf_size, tx_buf_size, 1 | ||
| .if $isdefed("I2S_RX") | ||
| MOV rx_buf_size, tx_buf_size | ||
| ;Rx buffer size is one less than PING/PONG buffer size | ||
| SUB rx_buf_size, rx_buf_size, 1 | ||
|
|
||
| ;read the Rx buffer address provided by host | ||
| LDI scratchreg0, I2S_RX_BUF_PING_ADD | ||
| LBBO &rx_buffer_address, scratchreg0, 0, 4 | ||
| MOV rx_ping_buffer_address, rx_buffer_address | ||
| LDI rx_sd_counter, I2S_SAMPLES_PER_CHANNEL | ||
| .endif | ||
|
|
||
| ;Initialize the end address of the Tx/Rx buffers | ||
| .if $isdefed("I2S_TX") | ||
| ADD tx_buffer_address_end, tx_buffer_address, tx_buf_size | ||
| .endif | ||
| .if $isdefed("I2S_RX") | ||
| ADD rx_buffer_address_end, rx_buffer_address, rx_buf_size | ||
| .endif | ||
|
|
||
| ;Initialize the PONG buffer addresses | ||
| .if $isdefed("I2S_TX") | ||
| MOV tx_pong_buffer_address, tx_buffer_address_end | ||
| .endif | ||
| .if $isdefed("I2S_RX") |
There was a problem hiding this comment.
1. tx_buffer_address lacks bounds check 📘 Rule violation ⛨ Security
The PRU firmware reads host-provided buffer addresses/sizes and performs LBBO/SBBO using them without validating that the effective address range stays within an allowed region. This can lead to out-of-bounds PRU memory accesses if the host provides a bad address or size.
Agent Prompt
## Issue description
The firmware consumes host-provided buffer base addresses and sizes and uses them for PRU memory accesses without validating that the full `[base, base+size)` range lies within an allowed shared-memory region.
## Issue Context
This PRU firmware reads `I2S_TX_BUF_PING_ADD`, `I2S_PING_PONG_BUFSIZE_ADD`, and `I2S_RX_BUF_PING_ADD`, then computes `*_buffer_address_end` and later performs `LBBO`/`SBBO` based on those computed pointers.
## Fix Focus Areas
- examples/pru_i2s/firmware/I2S/pru_i2s_main.asm[109-146]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+117
to
+132
| ;read the buffer size provided by host. buffer size is ping+pong | ||
| LDI scratchreg0, I2S_PING_PONG_BUFSIZE_ADD | ||
| LBBO &tx_buf_size, scratchreg0, 0, 2 | ||
| ;Shift right by 1 to get PING/PONG size buffer size | ||
| LSR tx_buf_size, tx_buf_size, 1 | ||
| .if $isdefed("I2S_RX") | ||
| MOV rx_buf_size, tx_buf_size | ||
| ;Rx buffer size is one less than PING/PONG buffer size | ||
| SUB rx_buf_size, rx_buf_size, 1 | ||
|
|
||
| ;read the Rx buffer address provided by host | ||
| LDI scratchreg0, I2S_RX_BUF_PING_ADD | ||
| LBBO &rx_buffer_address, scratchreg0, 0, 4 | ||
| MOV rx_ping_buffer_address, rx_buffer_address | ||
| LDI rx_sd_counter, I2S_SAMPLES_PER_CHANNEL | ||
| .endif |
There was a problem hiding this comment.
2. rx_buf_size underflow risk 📘 Rule violation ≡ Correctness
The firmware derives rx_buf_size from the host-provided ping-pong size and then does `SUB rx_buf_size, rx_buf_size, 1` without checking for 0/1 values. This can underflow and make rx_buffer_address_end invalid, causing out-of-bounds accesses.
Agent Prompt
## Issue description
`rx_buf_size` is computed as `tx_buf_size - 1` without validating that `tx_buf_size` is large enough, which can underflow and break bounds calculations.
## Issue Context
`tx_buf_size` is loaded from `I2S_PING_PONG_BUFSIZE_ADD` and right-shifted by 1 to get single-buffer size; `rx_buf_size` is then set to that value and decremented.
## Fix Focus Areas
- examples/pru_i2s/firmware/I2S/pru_i2s_main.asm[117-132]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+213
to
+266
| /* Open drivers to open the UART driver for console */ | ||
| Drivers_open(); | ||
| Board_driversOpen(); | ||
|
|
||
| DebugP_log("PRU I2S test started ...\r\n"); | ||
| DebugP_log("Build timestamp : %s %s\r\n", __DATE__, __TIME__); | ||
|
|
||
| /* Debug, configure GPIO */ | ||
| #ifdef SOC_AM263X | ||
| GPIO_setDirMode(CONFIG_GPIO_DEBUG0_BASE_ADDR, CONFIG_GPIO_DEBUG0_PIN, CONFIG_GPIO_DEBUG0_DIR); | ||
| GPIO_pinWriteHigh(CONFIG_GPIO_DEBUG0_BASE_ADDR, CONFIG_GPIO_DEBUG0_PIN); | ||
| GPIO_pinWriteLow(CONFIG_GPIO_DEBUG0_BASE_ADDR, CONFIG_GPIO_DEBUG0_PIN); | ||
| GPIO_setDirMode(CONFIG_GPIO_DEBUG1_BASE_ADDR, CONFIG_GPIO_DEBUG1_PIN, CONFIG_GPIO_DEBUG1_DIR); | ||
| GPIO_pinWriteHigh(CONFIG_GPIO_DEBUG1_BASE_ADDR, CONFIG_GPIO_DEBUG1_PIN); | ||
| GPIO_pinWriteLow(CONFIG_GPIO_DEBUG1_BASE_ADDR, CONFIG_GPIO_DEBUG1_PIN); | ||
| #endif | ||
| /* Configure I2C IO Expander, | ||
| route PRUn signals to HSEC */ | ||
| #ifdef SOC_AM263X | ||
| i2s_i2c_io_expander(); | ||
| #endif | ||
| /* Construct semaphores */ | ||
| status = SemaphoreP_constructBinary(&gPruI2s0TxSemObj, 0); | ||
| if (status == SystemP_FAILURE) | ||
| { | ||
| DebugP_log("ERROR: SemaphoreP_constructBinary() gPruI2s0TxSemObj\r\n"); | ||
| return; | ||
| } | ||
| status = SemaphoreP_constructBinary(&gPruI2s1RxSemObj, 0); | ||
| if (status == SystemP_FAILURE) | ||
| { | ||
| DebugP_log("ERROR: SemaphoreP_constructBinary() gPruI2s1RxSemObj\r\n"); | ||
| return; | ||
| } | ||
|
|
||
| /* Initialize PRU I2S driver */ | ||
| status = PRUI2S_init(&numValidCfg, &gPruFwImageInfo); | ||
| if (status == PRUI2S_DRV_SERR_INIT_FWIMG) | ||
| { | ||
| DebugP_log("WARNING: PRUI2S_init() no FW image for configuration\r\n"); | ||
| } | ||
| else if (status != PRUI2S_DRV_SOK) | ||
| { | ||
| DebugP_log("ERROR: PRUI2S_init()\r\n"); | ||
| return; | ||
| } | ||
| DebugP_assert(numValidCfg != 0); | ||
|
|
||
| if (((numValidCfg-1) < TEST_PRUI2S0_IDX) || | ||
| ((numValidCfg-1) < TEST_PRUI2S1_IDX)) | ||
| { | ||
| DebugP_log("ERROR: Invalid test configuration()\r\n"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
3. Diagnostic early returns skip cleanup 📘 Rule violation ☼ Reliability
The diagnostic application returns early on multiple error paths after opening drivers/constructing resources, without consistently closing PRU-I2S handles, destructing semaphores, or calling Board_driversClose()/Drivers_close(). This can leak resources and leave the system in a partially initialized state.
Agent Prompt
## Issue description
Error paths in `pru_i2s_diagnostic_main()` return early without consistently cleaning up opened drivers, PRUI2S instances, and semaphores.
## Issue Context
The function calls `Drivers_open()`/`Board_driversOpen()` and constructs semaphores, then has multiple `return;` statements on failures.
## Fix Focus Areas
- examples/pru_i2s/pru_i2s_app/pru_i2s_diagnostic.c[213-266]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+343
to
+370
| /* Lock instance */ | ||
| SemaphoreP_pend(pObj->pruInstLock, SystemP_WAIT_FOREVER); | ||
|
|
||
| if ((pObj->isOpen == FALSE) && (pObj->pruIcssHandle == NULL)) | ||
| { | ||
| /* Copy parameters to Object */ | ||
| if (pPrms != NULL) | ||
| { | ||
| pObj->prms = *pPrms; | ||
| } | ||
| else | ||
| { | ||
| PRUI2S_paramsInit(&pObj->prms); | ||
| } | ||
|
|
||
| /* Check parameters are valid */ | ||
| status = PRUI2S_checkOpenParams(pSwipAttrs, &pObj->prms); | ||
| } | ||
|
|
||
| if (status == SystemP_SUCCESS) | ||
| { | ||
| /* | ||
| * Construct interrupts | ||
| */ | ||
|
|
||
| /* Use pObj->prms consistently to avoid NULL dereference */ | ||
| if (pObj->prms.i2sTxCallbackFxn != NULL) | ||
| { |
There was a problem hiding this comment.
4. Open allows double-open 🐞 Bug ☼ Reliability
PRUI2S_open() does not return an error when the instance is already open; it skips parameter validation but still proceeds to construct interrupts and reinitialize PRU/INTC/FW, risking duplicated HWIs and corrupted instance state.
Agent Prompt
### Issue description
`PRUI2S_open()` can be called multiple times for the same `index` without failing. When `pObj->isOpen == TRUE`, it skips parameter copy/validation but still proceeds to (re)construct interrupts and reinitialize PRU state.
### Issue Context
This can lead to duplicate Hwi objects, unexpected interrupt behavior, and inconsistent PRU/INTC state.
### Fix Focus Areas
- examples/pru_i2s/driver/pru_i2s_drv.c[343-372]
- examples/pru_i2s/driver/pru_i2s_drv.c[446-468]
### What to change
- After acquiring `pObj->pruInstLock`, add a guard:
- If `pObj->isOpen == TRUE` (and/or `pObj->pruIcssHandle != NULL`), release the lock and return `NULL` (or a defined error via a separate API).
- Ensure failure paths don’t call `PRUI2S_close()` on a partially-open object if you choose to return early for already-open.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+65
to
+74
| INCLUDES_common := \ | ||
| -I${CG_TOOL_ROOT}/include/c \ | ||
| -I${MCU_PLUS_SDK_PATH}/source \ | ||
| -I${OPEN_PRU_PATH}/source \ | ||
| -I${OPEN_PRU_PATH}/examples/pru_i2s_diagnostic \ | ||
| -I${MCU_PLUS_SDK_PATH}/source/kernel/freertos/FreeRTOS-Kernel/include \ | ||
| -I${MCU_PLUS_SDK_PATH}/source/kernel/freertos/portable/TI_ARM_CLANG/ARM_CR5F \ | ||
| -I${MCU_PLUS_SDK_PATH}/source/kernel/freertos/config/am263x/r5f \ | ||
| t-I${MCU_PLUS_SDK_PATH}/source/pru_io/driver | ||
| -Igenerated \ |
There was a problem hiding this comment.
5. Am263x makefile include typo 🐞 Bug ≡ Correctness
The AM263x MCU+ app makefile contains an invalid include flag line (t-I...) and is missing the trailing continuation backslash, which will break makefile parsing and/or omit required include paths.
Agent Prompt
### Issue description
The AM263x ti-arm-clang makefile has a malformed include path entry: `t-I${MCU_PLUS_SDK_PATH}/source/pru_io/driver` (note the leading `t-`) and it lacks a trailing `\` line continuation. This will break `INCLUDES_common` and likely fail the build.
### Issue Context
This makefile is used to build the `am263x-cc/r5fss0-0_freertos` diagnostic app.
### Fix Focus Areas
- examples/pru_i2s/pru_i2s_app/am263x-cc/r5fss0-0_freertos/ti-arm-clang/makefile[65-74]
### What to change
- Replace `t-I${MCU_PLUS_SDK_PATH}/source/pru_io/driver` with a valid include entry:
- `-I${MCU_PLUS_SDK_PATH}/source/pru_io/driver \`
- Ensure the line ends with `\` if more include paths follow.
- Verify the `-I${OPEN_PRU_PATH}/examples/...` include directory is correct for this repo layout (it currently points to `examples/pru_i2s_diagnostic`).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+95
to
+113
| am261x_mcuplus: | ||
| # am261x-lp - Build driver library | ||
| $(MAKE) -C lib/am261x/r5f/ti-arm-clang $(ARGUMENTS_MCUPLUS) | ||
| # am261x-lp - R5F application | ||
| $(MAKE) -C pru_i2s_app/am261x-lp/r5fss0-0_freertos/ti-arm-clang $(ARGUMENTS_MCUPLUS) | ||
|
|
||
| am263x: | ||
| # am263x-cc - I2S protocol | ||
| $(MAKE) -C firmware/I2S/pru0_tx/am263x-cc/icssm0-pru0_fw/ti-pru-cgt $(ARGUMENTS_PRU) | ||
| $(MAKE) -C firmware/I2S/pru1_rx/am263x-cc/icssm0-pru1_fw/ti-pru-cgt $(ARGUMENTS_PRU) | ||
| # am263x-cc - TDM4 protocol | ||
| $(MAKE) -C firmware/TDM4/pru0_tx/am263x-cc/icssm0-pru0_fw/ti-pru-cgt $(ARGUMENTS_PRU) | ||
| $(MAKE) -C firmware/TDM4/pru1_rx/am263x-cc/icssm0-pru1_fw/ti-pru-cgt $(ARGUMENTS_PRU) | ||
|
|
||
| am263x_mcuplus: | ||
| # am263x-cc - Build driver library | ||
| $(MAKE) -C lib/am263x/r5f/ti-arm-clang $(ARGUMENTS_MCUPLUS) | ||
| # am263x-cc - R5F application | ||
| $(MAKE) -C pru_i2s_app/am263x-cc/r5fss0-0_freertos/ti-arm-clang $(ARGUMENTS_MCUPLUS) |
There was a problem hiding this comment.
6. Missing lib build directory 🐞 Bug ☼ Reliability
examples/pru_i2s/makefile invokes $(MAKE) -C lib/am261x/... and lib/am263x/... for *_mcuplus targets, but the PR does not include any corresponding lib/ directories in the repository checkout, so these targets will fail.
Agent Prompt
### Issue description
The top-level `examples/pru_i2s/makefile` defines `am261x_mcuplus` and `am263x_mcuplus` targets that run `make` in `lib/am261x/...` and `lib/am263x/...`, but the repo does not contain these directories in the current PR.
### Issue Context
This will break `make all` for pru_i2s whenever `DEVICE_NON_PRU` includes `*_mcuplus`.
### Fix Focus Areas
- examples/pru_i2s/makefile[95-113]
### What to change
- Either:
1) Add the missing `examples/pru_i2s/lib/...` (or `lib/...`) directories and their makefiles as intended, OR
2) Update the `-C` paths to the correct existing location(s) (e.g., build steps under `pru_i2s_app/...` or MCU+ SDK paths), OR
3) Remove/guard the *_mcuplus targets until the library build infrastructure is present.
- If these are expected to be generated, add the generation step before invoking `make -C lib/...`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+231
to
+266
| /* Set output port register address - needed for next read */ | ||
| I2C_Transaction_init(&i2cTransaction); | ||
| buffer[0] = TCA6416_REG_OUTPUT_PORT_0 + port; | ||
| i2cTransaction.writeBuf = buffer; | ||
| i2cTransaction.writeCount = 1U; | ||
| i2cTransaction.targetAddress = i2cAddress; | ||
| status += I2C_transfer(config->i2cHandle, &i2cTransaction); | ||
|
|
||
| /* Read config register value */ | ||
| I2C_Transaction_init(&i2cTransaction); | ||
| i2cTransaction.readBuf = buffer; | ||
| i2cTransaction.readCount = 1; | ||
| i2cTransaction.targetAddress = i2cAddress; | ||
| status += I2C_transfer(config->i2cHandle, &i2cTransaction); | ||
|
|
||
| /* Set output or input mode to particular IO pin - read/modify/write */ | ||
| I2C_Transaction_init(&i2cTransaction); | ||
| if(TCA6416_OUT_STATE_HIGH == state) | ||
| { | ||
| buffer[1] = buffer[0] | (0x01 << portPin); | ||
| } | ||
| else | ||
| { | ||
| buffer[1] = buffer[0] & ~(0x01 << portPin); | ||
| } | ||
| buffer[0] = TCA6416_REG_OUTPUT_PORT_0 + port; | ||
| i2cTransaction.writeBuf = buffer; | ||
| i2cTransaction.writeCount = 2; | ||
| i2cTransaction.targetAddress = i2cAddress; | ||
| status += I2C_transfer(config->i2cHandle, &i2cTransaction); | ||
|
|
||
| SemaphoreP_post(&config->lockObj); | ||
| } | ||
|
|
||
| return (status); | ||
| } |
There was a problem hiding this comment.
7. I2c status accumulation bug 🐞 Bug ≡ Correctness
TCA6416_setOutput() uses status += I2C_transfer(...) and continues after failures, which can hide errors, produce invalid return codes, and perform read/modify/write based on invalid register data.
Agent Prompt
### Issue description
`TCA6416_setOutput()` uses `status += I2C_transfer(...)` for multiple transfers and proceeds regardless of intermediate failures. This can mask failures and cause incorrect read/modify/write behavior.
### Issue Context
The same file’s `TCA6416_config()` uses the safer pattern: compare each transfer result to `SystemP_SUCCESS` and set `status = SystemP_FAILURE` on error.
### Fix Focus Areas
- examples/pru_i2s/pru_i2s_app/board/ioexp_tca6416.c[231-266]
- examples/pru_i2s/pru_i2s_app/board/ioexp_tca6416.c[158-196]
### What to change
- Replace `status += I2C_transfer(...)` with explicit checks:
- `status = I2C_transfer(...); if (status != SystemP_SUCCESS) { ... }`
- On failure, stop further transactions and still `SemaphoreP_post()` before returning.
- Keep return values consistent with the rest of the driver (`SystemP_SUCCESS`/`SystemP_FAILURE`).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Review Summary by QodoAdd PRU I2S driver with firmware, diagnostic application, and multi-platform support for AM261x and AM263x
WalkthroughsDescription• Complete PRU I2S driver implementation with full API supporting initialization, open/close, read/write operations, and interrupt handling • Dual-mode firmware support for both I2S and TDM4 audio streaming with configurable channels and sampling rates • Comprehensive diagnostic application demonstrating real-time full-duplex audio streaming with ping-pong buffer management • PRU firmware binaries for both PRU0 (TX) and PRU1 (RX) cores with register definitions and bit-field macros • TCA6416 IO expander driver for board-level pin control and configuration • Support for AM261x LaunchPad and AM263x Control Card platforms with SOC-specific configurations • FreeRTOS application framework with interrupt-driven ISR handling and semaphore-based task synchronization • PRUICSS interrupt mapping and controller initialization for system event handling • Build system integration with makefile updates for example compilation Diagramflowchart LR
A["PRU I2S Driver<br/>pru_i2s_drv.c"] --> B["Driver API<br/>pru_i2s_drv.h"]
C["I2S Firmware<br/>pru_i2s_main.asm"] --> D["I2S Binaries<br/>pru_i2s_pru0/1_array.h"]
E["TDM4 Firmware<br/>pru_i2s_main.asm"] --> F["TDM4 Binaries<br/>pru_i2s_tdm4_pru0/1_array.h"]
B --> G["Diagnostic App<br/>pru_i2s_diagnostic.c"]
D --> G
F --> G
G --> H["AM261x/AM263x<br/>FreeRTOS Apps"]
I["IO Expander<br/>ioexp_tca6416.c"] --> H
J["Config & Syscfg<br/>pru_i2s_config.h"] --> H
File Changes1. examples/pru_i2s/driver/pru_i2s_drv.c
|
|
Persistent review updated to latest commit 52124f1 |
Comment on lines
+533
to
+633
| DebugP_log("[PRUI2S_open] Constructing interrupts...\r\n"); | ||
| if (pObj->prms.i2sTxCallbackFxn != NULL) | ||
| { | ||
| DebugP_log("[PRUI2S_open] Constructing Tx interrupt (intNum=%d)\r\n", pSwipAttrs->i2sTxHostIntNum); | ||
| HwiP_Params_init(&hwiPrms); | ||
| hwiPrms.intNum = pSwipAttrs->i2sTxHostIntNum; | ||
| hwiPrms.callback = pObj->prms.i2sTxCallbackFxn; | ||
| hwiPrms.priority = pObj->prms.i2sTxIntrPri; | ||
| hwiPrms.args = (void *)pCfg; | ||
| status = HwiP_construct(&pObj->i2sTxHwiObj, &hwiPrms); | ||
| if (status == SystemP_SUCCESS) | ||
| { | ||
| pObj->i2sTxHwiHandle = &pObj->i2sTxHwiObj; | ||
| DebugP_log("[PRUI2S_open] Tx interrupt constructed successfully\r\n"); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| DebugP_log("[PRUI2S_open] Tx callback is NULL, skipping Tx interrupt\r\n"); | ||
| } | ||
|
|
||
| if (status == SystemP_SUCCESS && pObj->prms.i2sRxCallbackFxn != NULL) | ||
| { | ||
| DebugP_log("[PRUI2S_open] Constructing Rx interrupt (intNum=%d)\r\n", pSwipAttrs->i2sRxHostIntNum); | ||
| HwiP_Params_init(&hwiPrms); | ||
| hwiPrms.intNum = pSwipAttrs->i2sRxHostIntNum; | ||
| hwiPrms.callback = pObj->prms.i2sRxCallbackFxn; | ||
| hwiPrms.priority = pObj->prms.i2sRxIntrPri; | ||
| hwiPrms.args = (void *)pCfg; | ||
| status = HwiP_construct(&pObj->i2sRxHwiObj, &hwiPrms); | ||
| if (status == SystemP_SUCCESS) | ||
| { | ||
| pObj->i2sRxHwiHandle = &pObj->i2sRxHwiObj; | ||
| DebugP_log("[PRUI2S_open] Rx interrupt constructed successfully\r\n"); | ||
| } | ||
| } | ||
| else if (status == SystemP_SUCCESS) | ||
| { | ||
| DebugP_log("[PRUI2S_open] Rx callback is NULL, skipping Rx interrupt\r\n"); | ||
| } | ||
|
|
||
| if (status == SystemP_SUCCESS && pObj->prms.i2sErrCallbackFxn != NULL) | ||
| { | ||
| DebugP_log("[PRUI2S_open] Constructing Error interrupt (intNum=%d)\r\n", pSwipAttrs->i2sErrHostIntNum); | ||
| HwiP_Params_init(&hwiPrms); | ||
| hwiPrms.intNum = pSwipAttrs->i2sErrHostIntNum; | ||
| hwiPrms.callback = pObj->prms.i2sErrCallbackFxn; | ||
| hwiPrms.priority = pObj->prms.i2sErrIntrPri; | ||
| hwiPrms.args = (void *)pCfg; | ||
| status = HwiP_construct(&pObj->i2sErrHwiObj, &hwiPrms); | ||
| if (status == SystemP_SUCCESS) | ||
| { | ||
| pObj->i2sErrHwiHandle = &pObj->i2sErrHwiObj; | ||
| DebugP_log("[PRUI2S_open] Error interrupt constructed successfully\r\n"); | ||
| } | ||
| } | ||
| else if (status == SystemP_SUCCESS) | ||
| { | ||
| DebugP_log("[PRUI2S_open] Error callback is NULL, skipping Error interrupt\r\n"); | ||
| } | ||
| } | ||
|
|
||
| if (status == SystemP_SUCCESS) | ||
| { | ||
| /* Initialize PRU */ | ||
| DebugP_log("[PRUI2S_open] Initializing PRU/FW/buffers...\r\n"); | ||
| DebugP_log("[PRUI2S_open] Calling PRUI2S_initPru...\r\n"); | ||
| status = PRUI2S_initPru(pCfg); | ||
| DebugP_log("[PRUI2S_open] PRUI2S_initPru returned: %d\r\n", status); | ||
|
|
||
| /* Initialize PRU I2S FW */ | ||
| if (status == SystemP_SUCCESS) | ||
| { | ||
| DebugP_log("[PRUI2S_open] Calling PRUI2S_initFw...\r\n"); | ||
| status = PRUI2S_initFw(pCfg); | ||
| DebugP_log("[PRUI2S_open] PRUI2S_initFw returned: %d\r\n", status); | ||
| } | ||
|
|
||
| /* Initialize ping/pong buffers */ | ||
| if (status == SystemP_SUCCESS) | ||
| { | ||
| DebugP_log("[PRUI2S_open] Calling PRUI2S_initPpBufs...\r\n"); | ||
| status = PRUI2S_initPpBufs(pCfg); | ||
| DebugP_log("[PRUI2S_open] PRUI2S_initPpBufs returned: %d\r\n", status); | ||
| } | ||
| } | ||
|
|
||
| if (status == SystemP_SUCCESS) | ||
| { | ||
| DebugP_log("[PRUI2S_open] Instance %d successfully opened\r\n", index); | ||
| pObj->isOpen = TRUE; | ||
| handle = (PRUI2S_Handle)pCfg; | ||
| } | ||
|
|
||
| /* Unlock instance */ | ||
| DebugP_log("[PRUI2S_open] Unlocking instance %d\r\n", index); | ||
| SemaphoreP_post(pObj->pruInstLock); | ||
| DebugP_log("[PRUI2S_open] EXIT: returning handle=%08X\r\n", (uint32_t)handle); | ||
|
|
||
| return handle; | ||
| } |
There was a problem hiding this comment.
1. prui2s_open() missing error cleanup 📘 Rule violation ☼ Reliability
PRUI2S_open() constructs interrupt objects and can later fail, returning NULL without destructing previously-constructed HWIs. This can leak resources and leave partially-initialized interrupt state.
Agent Prompt
## Issue description
`PRUI2S_open()` can fail after constructing one or more `HwiP` objects, but it returns without destructing the successfully-constructed interrupts.
## Issue Context
This violates the requirement to clean up all partially-acquired resources on error paths.
## Fix Focus Areas
- examples/pru_i2s/driver/pru_i2s_drv.c[533-633]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+31
to
+36
| ; .sect ".text:main" | ||
| .clink | ||
| .global main | ||
|
|
||
| .include "pru_i2s_interface.h" | ||
| .include "pru_i2s_regs.h" |
There was a problem hiding this comment.
2. Tdm4 main missing .retain 📘 Rule violation ☼ Reliability
firmware/TDM4/pru_i2s_main.asm lacks the required .retain and .retainrefs directives for assembly-based .out generation. This risks dropped sections/references and incorrect output images.
Agent Prompt
## Issue description
The TDM4 PRU assembly main file is missing `.retain`/`.retainrefs`, which are required to ensure correct `.out` generation.
## Issue Context
This is a common requirement for PRU assembly-only projects so the linker does not drop sections.
## Fix Focus Areas
- examples/pru_i2s/firmware/TDM4/pru_i2s_main.asm[31-36]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+646
to
+663
| int32_t PRUI2S_close( | ||
| PRUI2S_Handle handle | ||
| ) | ||
| { | ||
| PRUI2S_Config *pCfg; | ||
| PRUI2S_Object *pObj; | ||
| PRUI2S_SwipAttrs *pSwipAttrs; | ||
| int32_t status = SystemP_SUCCESS; | ||
| int32_t retStatus = PRUI2S_DRV_SOK; | ||
|
|
||
| /* Get pointers */ | ||
| pCfg = (PRUI2S_Config *)handle; | ||
| pObj = pCfg->object; | ||
| pSwipAttrs = pCfg->attrs; | ||
|
|
||
| /* Lock instance */ | ||
| SemaphoreP_pend(pObj->pruInstLock, SystemP_WAIT_FOREVER); | ||
|
|
There was a problem hiding this comment.
3. Null handle dereference 🐞 Bug ☼ Reliability
Multiple public PRU I2S APIs (e.g., PRUI2S_close/enable/clearInt/write/read) cast and dereference handle (and pIoBuf) without NULL checks, which can crash on error-path cleanup or caller mistakes. For example, PRUI2S_close() immediately dereferences handle to access pCfg->object.
Agent Prompt
### Issue description
Public PRU I2S APIs dereference `handle` (and sometimes `pIoBuf`) without NULL checks, causing NULL-pointer crashes.
### Issue Context
`PRUI2S_open()` returns `NULL` on failure, so defensive checks in APIs like `PRUI2S_close`, `PRUI2S_enable`, `PRUI2S_clearInt`, `PRUI2S_write`, and `PRUI2S_read` prevent cleanup-path crashes and make the API safer.
### Fix Focus Areas
- examples/pru_i2s/driver/pru_i2s_drv.c[646-770]
- examples/pru_i2s/driver/pru_i2s_drv.c[780-904]
- examples/pru_i2s/driver/pru_i2s_drv.c[919-981]
- examples/pru_i2s/driver/pru_i2s_drv.c[1427-1471]
### Suggested change
- At the top of each public API, add:
- `if (handle == NULL) return PRUI2S_DRV_SERR_INV_PRMS;`
- For buffer APIs: `if (pIoBuf == NULL || pIoBuf->ioBufAddr == NULL) return PRUI2S_DRV_SERR_INV_PRMS;`
- Consider also validating `pObj->pruIcssHandle != NULL` where it is required for a call.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1685
to
+1703
| /* Check ping/pong buffer size */ | ||
| if ((status == SystemP_SUCCESS) && | ||
| (pPrms->pingPongBufSz == 0)) | ||
| { | ||
| status = SystemP_FAILURE; | ||
| } | ||
|
|
||
| /* Check I2S error parameters */ | ||
| if ((status == SystemP_SUCCESS) && (pPrms->i2sErrCallbackFxn != NULL)) | ||
| { | ||
| /* Check I2S error interrupt priority */ | ||
| if (pPrms->i2sErrIntrPri > MAX_VIM_INTR_PRI_VAL) | ||
| { | ||
| status = SystemP_FAILURE; | ||
| } | ||
| } | ||
|
|
||
| return status; | ||
| } |
There was a problem hiding this comment.
4. Odd buffer size allowed 🐞 Bug ≡ Correctness
PRUI2S_checkOpenParams() only rejects pingPongBufSz == 0, but PRUI2S_write() rejects odd sizes while PRUI2S_read() performs memcpy using pingPongBufSz/2 without validating even/non-zero. This can lead to inconsistent behavior where writes fail but reads still run with truncated/incorrect half-buffer sizing.
Agent Prompt
### Issue description
The driver inconsistently validates `pingPongBufSz`: open() allows odd sizes, write() rejects them, read() does not reject them.
### Issue Context
Both read and write derive half-buffer size via `pingPongBufSz/2`. Ping/pong implies the total size should be divisible by 2.
### Fix Focus Areas
- examples/pru_i2s/driver/pru_i2s_drv.c[1618-1703]
- examples/pru_i2s/driver/pru_i2s_drv.c[832-904]
- examples/pru_i2s/driver/pru_i2s_drv.c[919-981]
### Suggested change
- Enforce `pingPongBufSz != 0` and `(pingPongBufSz % 2) == 0` in `PRUI2S_checkOpenParams()` so invalid sizes are rejected at open.
- Mirror the same validation in `PRUI2S_read()` for defense-in-depth (as already done in `PRUI2S_write()`).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
5
to
14
| "documentationPath": "../docs", | ||
| "includePaths": [ | ||
| "../source", | ||
| "C:/ti/mcu_plus_sdk/source/sysconfig", | ||
| ], | ||
| "components": [ | ||
|
|
||
| "/open_pru", | ||
| ], | ||
| "devices": [ | ||
| "AM64x", |
There was a problem hiding this comment.
5. Invalid json trailing commas 🐞 Bug ≡ Correctness
.metadata/product.json contains trailing commas in arrays (e.g., includePaths/devices/components), which makes the file invalid strict JSON and can break any tooling that parses it as JSON. The PR adds an includePaths entry that retains this invalid trailing comma pattern.
Agent Prompt
### Issue description
`.metadata/product.json` is not valid strict JSON due to trailing commas in arrays.
### Issue Context
Many JSON parsers used by build/config tooling reject trailing commas.
### Fix Focus Areas
- .metadata/product.json[5-21]
### Suggested change
- Remove trailing commas after the last element in `includePaths`, `components`, `devices` (and anywhere else in the file).
- If the consuming tooling expects JSON5, consider renaming to `.json5` and documenting it; otherwise keep strict JSON compliance.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
manojKoppolu
requested changes
Apr 13, 2026
Member
manojKoppolu
left a comment
manojKoppolu
left a comment
There was a problem hiding this comment.
Rajul,
Please rebase your PR and do force push to trigger CI make build
rajul-bhambay
force-pushed
the
a0497643_PINDSW-8487_PRU_I2S
branch
from
52124f1 to
7211e79
Compare
Review Summary by QodoPRU I2S Driver and Firmware Implementation with Dual-Mode Audio Support
WalkthroughsDescription• Complete PRU I2S driver implementation with comprehensive API support for initialization, open/close, read/write operations, and interrupt management • Dual-mode firmware support for both standard I2S and TDM4 audio interfaces with separate PRU0 (transmit) and PRU1 (receive) implementations • Full-duplex and half-duplex audio streaming capabilities with ping-pong buffer management and real-time interrupt handling • Diagnostic application demonstrating real-time audio streaming with multi-channel support, error detection, and optional loopback functionality • Platform support for AM261x LaunchPad and AM263x Control Card with FreeRTOS integration • TCA6416 IO expander driver for pin control and configuration on supported platforms • Comprehensive firmware register definitions, pin mappings, and linker configurations for both I2S and TDM4 modes • Pre-compiled firmware binary arrays for immediate deployment to PRU cores Diagramflowchart LR
A["PRU I2S Driver<br/>pru_i2s_drv.c/h"] --> B["Firmware Layer<br/>I2S & TDM4 Modes"]
B --> C["PRU0 TX<br/>Firmware Binary"]
B --> D["PRU1 RX<br/>Firmware Binary"]
A --> E["Application<br/>pru_i2s_diagnostic.c"]
E --> F["Platform Support<br/>AM261x/AM263x"]
F --> G["IO Expander<br/>TCA6416"]
E --> H["FreeRTOS<br/>Integration"]
File Changes1. examples/pru_i2s/driver/pru_i2s_drv.c
|
Code Review by QodoNew Review StartedThis review has been superseded by a new analysisⓘ The new review experience is currently in Beta. Learn more |
rajul-bhambay
force-pushed
the
a0497643_PINDSW-8487_PRU_I2S
branch
from
7211e79 to
c4e4775
Compare
Review Summary by QodoPRU I2S Driver and Firmware Implementation for AM261x and AM263x
WalkthroughsDescription• Complete PRU I2S driver implementation with full API support for initialization, open/close, read/write operations • Support for both I2S and TDM4 modes with interleaved and non-interleaved buffer management • Comprehensive PRU firmware implementation in assembly for Tx/Rx operations with ping-pong buffer handling • Diagnostic application with real-time audio streaming, ISR-based interrupt handling, and error tracking • TCA6416 IO expander driver for hardware pin configuration • Platform support for AM261x LaunchPad and AM263x Control Card with FreeRTOS integration • Firmware binary arrays for PRU0 (Tx) and PRU1 (Rx) in both I2S and TDM4 modes • Complete build infrastructure with makefiles, linker scripts, and project specifications • ROV (Runtime Object View) configuration for FreeRTOS debugging support Diagramflowchart LR
A["PRU I2S Driver<br/>pru_i2s_drv.c"] --> B["Public API<br/>pru_i2s_drv.h"]
C["I2S Firmware<br/>pru_i2s_main.asm"] --> D["I2S Binary Arrays<br/>pru_i2s_pru0/1_array.h"]
E["TDM4 Firmware<br/>pru_i2s_main.asm"] --> F["TDM4 Binary Arrays<br/>pru_i2s_tdm4_pru0/1_array.h"]
B --> G["Diagnostic App<br/>pru_i2s_diagnostic.c"]
D --> G
F --> G
H["IO Expander<br/>ioexp_tca6416.c"] --> G
I["App Config<br/>pru_i2s_app_config.c"] --> G
G --> J["FreeRTOS Main<br/>main.c"]
K["Firmware Registers<br/>icss_pru_i2s_fw.h"] --> C
K --> E
L["Pin Config<br/>pru_i2s_interface.h"] --> C
L --> E
File Changes1. examples/pru_i2s/driver/pru_i2s_drv.c
|
Code Review by QodoNew Review StartedThis review has been superseded by a new analysisⓘ The new review experience is currently in Beta. Learn more |
rajul-bhambay
force-pushed
the
a0497643_PINDSW-8487_PRU_I2S
branch
from
c4e4775 to
8ab787e
Compare
Review Summary by QodoPRU I2S Driver and Firmware Implementation with Real-Time Audio Streaming Support
WalkthroughsDescription• Implements complete PRU I2S driver with full lifecycle management (initialization, open/close, read/write operations) • Provides both interleaved and non-interleaved I/O buffer operations for Tx/Rx data handling with interrupt management • Adds comprehensive PRU I2S diagnostic application with real-time audio streaming over I2S/TDM interfaces • Implements ping-pong buffer management for continuous data flow with full-duplex, Tx-only, and Rx-only modes • Includes PRU firmware implementations for both I2S and TDM4 modes with assembly-level audio data transmission/reception • Provides pre-compiled firmware binary arrays for PRU0 (transmit) and PRU1 (receive) for both I2S and TDM4 modes • Adds TCA6416 IO expander driver for hardware pin control and configuration • Includes platform-specific configurations for AM261x LaunchPad and AM263x Control Card • Provides complete build infrastructure with makefiles, linker scripts, and project specifications • Adds FreeRTOS ROV (Runtime Object View) configuration for debugging support Diagramflowchart LR
A["PRU I2S Driver<br/>pru_i2s_drv.c/h"] --> B["Driver API<br/>Init/Open/Close<br/>Read/Write"]
C["PRU Firmware<br/>I2S & TDM4 Modes"] --> D["Firmware Binaries<br/>PRU0/PRU1 Arrays"]
E["Diagnostic App<br/>pru_i2s_diagnostic.c"] --> F["Audio Streaming<br/>Ping-Pong Buffers<br/>ISR Handling"]
B --> F
D --> F
G["Platform Config<br/>AM261x/AM263x"] --> B
H["IO Expander<br/>TCA6416 Driver"] --> F
File Changes1. examples/pru_i2s/driver/pru_i2s_drv.c
|
|
Persistent review updated to latest commit 8ab787e |
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
Comment on lines
+60
to
+68
| LDI scratchreg0, I2S_TX_BUF_PING_ADD | ||
| LBBO &tx_ping_buffer_address, scratchreg0, 0, 4 | ||
| LDI scratchreg0, I2S_PING_PONG_BUFSIZE_ADD | ||
| LBBO &tx_buf_size, scratchreg0, 0, 2 | ||
| ADD tx_buffer_address_end, tx_ping_buffer_address, tx_buf_size | ||
| ZERO_TX_PING_PONG: | ||
| SBBO &ch0_data_tx, tx_ping_buffer_address, 0, 4 | ||
| ADD tx_ping_buffer_address, tx_ping_buffer_address, 0x4 | ||
| QBGT ZERO_TX_PING_PONG, tx_ping_buffer_address, tx_buffer_address_end |
There was a problem hiding this comment.
1. zero_tx_ping_pong loop compares wrong 📘 Rule violation ≡ Correctness
The buffer-clear loop branches using QBGT with operands ordered such that the loop can become non-terminating and write past the intended end address when tx_ping_buffer_address exceeds tx_buffer_address_end. This can cause runaway out-of-bounds stores into arbitrary memory.
Agent Prompt
## Issue description
`ZERO_TX_PING_PONG` uses a `QBGT` compare/operand ordering that can cause non-terminating looping after the pointer passes the computed end address, resulting in out-of-bounds `SBBO`.
## Issue Context
This loop is used to clear the TX ping/pong buffer after certain error conditions; the termination condition must stop at the buffer end.
## Fix Focus Areas
- examples/pru_i2s/firmware/I2S/pru_i2s_main.asm[60-68]
- examples/pru_i2s/firmware/TDM4/pru_i2s_main.asm[59-67]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+7
to
+8
| export OPEN_PRU_PATH?=$(abspath ../../../../../../../..) | ||
| include $(OPEN_PRU_PATH)/imports.mak |
There was a problem hiding this comment.
2. open_pru_path uses fixed abspath 📘 Rule violation ⚙ Maintainability
The firmware makefile sets OPEN_PRU_PATH using $(abspath ../../../../../../../..) which depends on the invocation working directory. This can break portability when make is invoked from a different directory.
Agent Prompt
## Issue description
The makefile computes `OPEN_PRU_PATH` relative to the invocation directory, which is not portable.
## Issue Context
The compliance requirement expects root derivation from the makefile location (for example using `$(lastword $(MAKEFILE_LIST))` and `$(dir ...)`).
## Fix Focus Areas
- examples/pru_i2s/firmware/I2S/pru0_tx/am263x-cc/icssm0-pru0_fw/ti-pru-cgt/makefile[7-8]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+33
to
+40
| .if $isdefed("SOC_AM263X") | ||
| .if $isdefed("PRU0") | ||
| I2S_INSTANCE_BCLK_PIN .set 6 ; PRG0_PRU0_GPOx | ||
| I2S_INSTANCE_BCLK_PIN_POS .set 64 ; 2 << I2S_INSTANCE_BCLK_PIN | ||
|
|
||
| I2S_INSTANCE_FS_PIN .set 1 | ||
| I2S_INSTANCE_FS_PIN_POS .set 2 ; 2 << I2S_INSTANCE_FS_PIN | ||
| .else |
There was a problem hiding this comment.
3. pru_i2s_interface.h missing header guard 📘 Rule violation ☼ Reliability
The assembly header is included via .include but has no four-underscore $isdefed guard, so multiple inclusion can redefine symbols and cause build errors. This violates the required assembly header guard pattern.
Agent Prompt
## Issue description
The assembly header `pru_i2s_interface.h` lacks the required include guard, risking symbol redefinition on multiple `.include` paths.
## Issue Context
The repository guideline requires a four-underscore `$isdefed`-based guard (for example `____pru_i2s_interface_h`).
## Fix Focus Areas
- examples/pru_i2s/firmware/I2S/pru_i2s_interface.h[1-40]
- examples/pru_i2s/firmware/TDM4/pru_i2s_interface.h[1-40]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+960
to
+1016
| /* Get pointers */ | ||
| pCfg = (PRUI2S_Config *)handle; | ||
| pObj = pCfg->object; | ||
| pSwipAttrs = pCfg->attrs; | ||
|
|
||
| /* Get number of Tx I2S */ | ||
| numTxI2s = pSwipAttrs->numTxI2s; | ||
|
|
||
| /* Check IO buffer parameter */ | ||
| for (i = 0; i < numTxI2s; i++) | ||
| { | ||
| if ((pIoBufC->ioBufLAddr[i] == NULL) || | ||
| (pIoBufC->ioBufRAddr[i] == NULL)) | ||
| { | ||
| retStatus = PRUI2S_DRV_SERR_INV_PRMS; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (retStatus == PRUI2S_DRV_SOK) | ||
| { | ||
| /* Calculate bytes per slot, number of channels, number of slots */ | ||
| bytesPerSlot = pSwipAttrs->bitsPerSlot / 8; | ||
| numTxCh = 2*numTxI2s; | ||
| numSlots = pObj->prms.pingPongBufSz / (numTxCh * bytesPerSlot); | ||
|
|
||
| if (pSwipAttrs->bitsPerSlot == PRUI2S_BITS_PER_SLOT_16) | ||
| { | ||
| /* Interleave Left channel data */ | ||
| for (i = 0; i < numTxI2s; i++) | ||
| { | ||
| pSrc16b = (int16_t *)pIoBufC->ioBufLAddr[i]; | ||
| pDst16b = (int16_t *)pObj->txPingPongBuf; | ||
| pDst16b += i; | ||
|
|
||
| for (j = 0; j < numSlots; j++) | ||
| { | ||
| *pDst16b = *pSrc16b; | ||
| pSrc16b++; | ||
| pDst16b += numTxCh; | ||
| } | ||
| } | ||
|
|
||
| /* Interleave Right channel data */ | ||
| for (i = 0; i < numTxI2s; i++) | ||
| { | ||
| pSrc16b = (int16_t *)pIoBufC->ioBufRAddr[i]; | ||
| pDst16b = (int16_t *)pObj->txPingPongBuf; | ||
| pDst16b += numTxI2s+i; | ||
| for (j = 0; j < numSlots; j++) | ||
| { | ||
| *pDst16b = *pSrc16b; | ||
| pSrc16b++; | ||
| pDst16b += numTxCh; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
4. Writec breaks ping/pong 🐞 Bug ≡ Correctness
PRUI2S_writeC() always writes interleaved samples to the base of txPingPongBuf and never uses FW_REG_TX_PING_PONG_SEL nor sets FW_REG_TX_PING_PONG_STAT. This breaks the firmware ping/pong contract and can cause the PRU to consume the wrong half-buffer or never see the buffer as “full”.
Agent Prompt
### Issue description
`PRUI2S_writeC()` writes to `txPingPongBuf` without honoring the firmware-selected ping/pong half-buffer and without setting the FW “buffer full” status bit. This violates the same FW contract that `PRUI2S_write()` follows.
### Issue Context
The firmware exposes `FW_REG_TX_PING_PONG_SEL` and `FW_REG_TX_PING_PONG_STAT` and the interleaved write API already uses them. The conversion write API should follow the same handshake, but write interleaved output into the selected half-buffer.
### Fix Focus Areas
- examples/pru_i2s/driver/pru_i2s_drv.c[942-1051]
- examples/pru_i2s/driver/pru_i2s_drv.c[756-827]
### What to change
- Read `FW_REG_TX_PING_PONG_SEL` to determine ping vs pong.
- Compute `dstBase = (uint8_t*)txPingPongBuf + (sel==PONG ? size : 0)` where `size = pingPongBufSz/2`.
- Interleave into `dstBase` (not the base of the whole ping+pong region).
- Set the corresponding bit in `FW_REG_TX_PING_PONG_STAT` (ping or pong) after the write completes.
- Add the same buffer size validation used in `PRUI2S_write()` (even/non-zero) and bounds checks for the selected half-buffer.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1085
to
+1145
| /* Get pointers */ | ||
| pCfg = (PRUI2S_Config *)handle; | ||
| pObj = pCfg->object; | ||
| pSwipAttrs = pCfg->attrs; | ||
|
|
||
| /* Get number of Rx I2S */ | ||
| numRxI2s = pSwipAttrs->numRxI2s; | ||
|
|
||
| /* Check IO buffer parameter */ | ||
| for (i = 0; i < numRxI2s; i++) | ||
| { | ||
| if ((pIoBufC->ioBufLAddr[i] == NULL) || | ||
| (pIoBufC->ioBufRAddr[i] == NULL)) | ||
| { | ||
| retStatus = PRUI2S_DRV_SERR_INV_PRMS; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (retStatus == PRUI2S_DRV_SOK) | ||
| { | ||
| /* Calculate bytes per slot, number of channels, number of slots */ | ||
| bytesPerSlot = pSwipAttrs->bitsPerSlot / 8; | ||
| numRxCh = 2*numRxI2s; | ||
| numSlots = pObj->prms.pingPongBufSz / (numRxCh * bytesPerSlot); | ||
|
|
||
| if (pSwipAttrs->bitsPerSlot == PRUI2S_BITS_PER_SLOT_16) | ||
| { | ||
| /* Interleave Left channel data */ | ||
| for (i = 0; i < numRxI2s; i++) | ||
| { | ||
| pSrc16b = (int16_t *)pObj->rxPingPongBuf; | ||
| pSrc16b += i; | ||
| pDst16b = (int16_t *)pIoBufC->ioBufLAddr[i]; | ||
| for (j = 0; j < numSlots; j++) | ||
| { | ||
| *pDst16b = *pSrc16b; | ||
| pSrc16b += numRxCh; | ||
| pDst16b++; | ||
| } | ||
| } | ||
|
|
||
| /* Interleave Right channel data */ | ||
| for (i = 0; i < numRxI2s; i++) | ||
| { | ||
| pSrc16b = (int16_t *)pObj->rxPingPongBuf; | ||
| pSrc16b += numRxI2s+i; | ||
| pDst16b = (int16_t *)pIoBufC->ioBufRAddr[i]; | ||
| for (j = 0; j < numSlots; j++) | ||
| { | ||
| *pDst16b = *pSrc16b; | ||
| pSrc16b += numRxCh; | ||
| pDst16b++; | ||
| } | ||
| } | ||
| } | ||
| else if (pSwipAttrs->bitsPerSlot == PRUI2S_BITS_PER_SLOT_32) | ||
| { | ||
| /* Interleave Left channel data */ | ||
| for (i = 0; i < numRxI2s; i++) | ||
| { |
There was a problem hiding this comment.
5. Readc breaks ping/pong 🐞 Bug ≡ Correctness
PRUI2S_readC() always reads from the base of rxPingPongBuf and never uses FW_REG_RX_PING_PONG_SEL nor clears FW_REG_RX_PING_PONG_STAT. This can return stale/wrong audio data and can prevent firmware/host from advancing ping/pong ownership correctly.
Agent Prompt
### Issue description
`PRUI2S_readC()` reads from `rxPingPongBuf` without honoring the firmware-selected ping/pong half-buffer and does not clear the FW “buffer empty/full” status bit. This violates the same FW contract that `PRUI2S_read()` follows.
### Issue Context
Firmware exposes `FW_REG_RX_PING_PONG_SEL` and `FW_REG_RX_PING_PONG_STAT`. The non-conversion read API already uses these registers.
### Fix Focus Areas
- examples/pru_i2s/driver/pru_i2s_drv.c[1067-1174]
- examples/pru_i2s/driver/pru_i2s_drv.c[843-905]
### What to change
- Read `FW_REG_RX_PING_PONG_SEL` and compute `srcBase = (uint8_t*)rxPingPongBuf + (sel==PONG ? size : 0)`.
- De-interleave from `srcBase` into the caller’s L/R buffers.
- Clear the corresponding bit in `FW_REG_RX_PING_PONG_STAT` (ping or pong) after the read completes.
- Add the same buffer size validation and bounds checks used by `PRUI2S_read()`/`PRUI2S_write()` to avoid out-of-range half-buffer access.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1536
to
+1573
| } | ||
|
|
||
| /* Check Tx interrupt callback function */ | ||
| if ((status == SystemP_SUCCESS) && (pPrms->i2sTxCallbackFxn == NULL)) | ||
| { | ||
| status = SystemP_FAILURE; | ||
| } | ||
|
|
||
| /* Check Tx ping/pong buffer base address. | ||
| Tx ping/pong buffer is expected to be in ICSS SHMEM. | ||
| Note: Absolute buffer address validation is delegated to application | ||
| during PRUI2S_open() when full PRUICSS handle context is available. */ | ||
| if ((status == SystemP_SUCCESS) && (pPrms->txPingPongBaseAddr == 0)) | ||
| { | ||
| status = SystemP_FAILURE; | ||
| } | ||
| } | ||
|
|
||
| /* Check Rx I2S parameters */ | ||
| if ((status == SystemP_SUCCESS) && (pSwipAttrs->numRxI2s > 0)) | ||
| { | ||
| /* Check Rx interrupt priority */ | ||
| if (pPrms->i2sRxIntrPri > MAX_VIM_INTR_PRI_VAL) | ||
| { | ||
| status = SystemP_FAILURE; | ||
| } | ||
|
|
||
| /* Check Rx interrupt callback function */ | ||
| if ((status == SystemP_SUCCESS) && (pPrms->i2sRxCallbackFxn == NULL)) | ||
| { | ||
| status = SystemP_FAILURE; | ||
| } | ||
|
|
||
| /* Check Rx ping/pong buffer base address */ | ||
| if ((status == SystemP_SUCCESS) && (pPrms->rxPingPongBaseAddr == 0)) | ||
| { | ||
| status = SystemP_FAILURE; | ||
| } |
There was a problem hiding this comment.
6. Open(null) always fails 🐞 Bug ≡ Correctness
PRUI2S_open() claims NULL parameters use defaults, but the defaults set Tx/Rx callbacks to NULL and PRUI2S_checkOpenParams() rejects NULL callbacks when Tx/Rx is enabled. As a result PRUI2S_open(index, NULL) will fail for any Tx/Rx-capable instance.
Agent Prompt
### Issue description
`PRUI2S_open(index, NULL)` is documented to use defaults, but defaults set callbacks to NULL and the open-param validation rejects NULL callbacks when Tx/Rx is enabled. This makes the NULL/defaults path non-functional.
### Issue Context
- `PRUI2S_paramsInit()` sets callbacks to NULL.
- `PRUI2S_checkOpenParams()` requires callbacks for Tx/Rx.
### Fix Focus Areas
- examples/pru_i2s/driver/pru_i2s_drv.c[343-364]
- examples/pru_i2s/driver/pru_i2s_drv.c[380-472]
- examples/pru_i2s/driver/pru_i2s_drv.c[1522-1594]
### What to change (pick one consistent design)
Option A (most common): Allow polling/no-interrupt mode
- In `PRUI2S_checkOpenParams()`, do not fail when Tx/Rx callback is NULL; instead only construct/enable interrupts when a callback is provided.
- Update docs/comments to clarify behavior.
Option B: Require explicit params
- Change `PRUI2S_open()` docstring and behavior to reject `pPrms==NULL` (or provide non-NULL default callbacks).
Also consider returning a driver-specific error code instead of generic `SystemP_FAILURE` for easier debugging.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+262
to
+326
| /* Used to check status and initialization */ | ||
| static Bool gPruI2sDrvInit = FALSE; | ||
|
|
||
| /* Number of valid configurations */ | ||
| static uint8_t gPruI2sDrvNumValidCfg = 0; | ||
|
|
||
| /* PRU I2S objects */ | ||
| static PRUI2S_Object gPruI2sObject[PRU_I2S_MAX_NUM_INST]; | ||
|
|
||
| /* PRU I2S SW IP attributes - Minimal configuration | ||
| * All INTC, GPIO, and pinmux now managed by SysConfig. | ||
| * Only essential base configuration remains here. | ||
| */ | ||
| /* NOTE: This array is initialized with default values. | ||
| * Applications must call PRUI2S_setUserConfig() to configure ICSS instance and PRU core | ||
| * before calling PRUI2S_init(). | ||
| */ | ||
| static PRUI2S_SwipAttrs gPruI2sSwipAttrs[PRU_I2S_MAX_NUM_INST] = | ||
| { | ||
| /* Configuration 0 - Set by PRUI2S_setUserConfig() */ | ||
| { | ||
| .baseAddr = 0, /* Set by PRUI2S_setUserConfig() based on ICSS instance */ | ||
| .icssInstId = 0, /* Set by PRUI2S_setUserConfig() */ | ||
| .pruInstId = PRUICSS_PRU0, /* Set by PRUI2S_setUserConfig() */ | ||
| .numTxI2s = 0, /* Detected from firmware at runtime */ | ||
| .numRxI2s = 0, /* Detected from firmware at runtime */ | ||
| .sampFreq = 0, /* Detected from firmware at runtime */ | ||
| .bitsPerSlot = 0, /* Detected from firmware at runtime */ | ||
| .i2sTxHostIntNum = 0, /* Detected from firmware at runtime */ | ||
| .i2sRxHostIntNum = 0, /* Detected from firmware at runtime */ | ||
| .i2sErrHostIntNum = 0, /* Detected from firmware at runtime */ | ||
| .i2sTxIcssIntcSysEvt = 0, /* Detected from firmware at runtime */ | ||
| .i2sRxIcssIntcSysEvt = 0, /* Detected from firmware at runtime */ | ||
| .i2sErrIcssIntcSysEvt = 0, /* Detected from firmware at runtime */ | ||
| }, | ||
| /* Configuration 1 - Set by PRUI2S_setUserConfig() */ | ||
| { | ||
| .baseAddr = 0, /* Set by PRUI2S_setUserConfig() based on ICSS instance */ | ||
| .icssInstId = 0, /* Set by PRUI2S_setUserConfig() */ | ||
| .pruInstId = PRUICSS_PRU1, /* Set by PRUI2S_setUserConfig() */ | ||
| .numTxI2s = 0, /* Detected from firmware at runtime */ | ||
| .numRxI2s = 0, /* Detected from firmware at runtime */ | ||
| .sampFreq = 0, /* Detected from firmware at runtime */ | ||
| .bitsPerSlot = 0, /* Detected from firmware at runtime */ | ||
| .i2sTxHostIntNum = 0, /* Detected from firmware at runtime */ | ||
| .i2sRxHostIntNum = 0, /* Detected from firmware at runtime */ | ||
| .i2sErrHostIntNum = 0, /* Detected from firmware at runtime */ | ||
| .i2sTxIcssIntcSysEvt = 0, /* Detected from firmware at runtime */ | ||
| .i2sRxIcssIntcSysEvt = 0, /* Detected from firmware at runtime */ | ||
| .i2sErrIcssIntcSysEvt = 0, /* Detected from firmware at runtime */ | ||
| } | ||
| }; | ||
|
|
||
| /* PRU I2S configurations */ | ||
| static PRUI2S_Config gPruI2sConfig[PRU_I2S_NUM_CONFIG] = | ||
| { | ||
| { | ||
| &gPruI2sObject[0], | ||
| &gPruI2sSwipAttrs[0] | ||
| }, | ||
| { | ||
| &gPruI2sObject[1], | ||
| &gPruI2sSwipAttrs[1] | ||
| } | ||
| }; |
There was a problem hiding this comment.
7. Globals defined in header 🐞 Bug ⚙ Maintainability
pru_i2s_drv.h defines mutable driver globals (objects/config/state) as static variables in the header, so every translation unit that includes it allocates its own private copy. This wastes memory and makes driver state management error-prone (the app will have a separate unused copy from the driver implementation).
Agent Prompt
### Issue description
The public header `pru_i2s_drv.h` defines mutable driver state as `static` globals. Every `.c` file including this header gets its own private copy, causing memory bloat and making the driver architecture fragile.
### Issue Context
Driver state should live in a single compilation unit (typically `pru_i2s_drv.c`) with `extern` declarations (or accessor APIs) exposed via the header.
### Fix Focus Areas
- examples/pru_i2s/include/pru_i2s_drv.h[262-326]
- examples/pru_i2s/driver/pru_i2s_drv.c[1-200]
### What to change
- In `pru_i2s_drv.h`: remove the `static` definitions of driver globals/config tables; keep only type definitions and function prototypes.
- In `pru_i2s_drv.c`: add the actual definitions for these globals.
- If any of these need to be visible to other modules, declare them as `extern` in the header (preferably avoid exposing internals; provide getter APIs instead).
- Ensure only one definition exists across the build (no per-TU copies).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
- TDM functionality - I2S functionality Fixes: PINDSW-8487,9295 Signed-off-by: Rajul Bhambay <r-bhambay@ti.com>
rajul-bhambay
force-pushed
the
a0497643_PINDSW-8487_PRU_I2S
branch
from
8ab787e to
d9175ef
Compare
|
Persistent review updated to latest commit d9175ef |
Comment on lines
+324
to
+336
| gPruIcssHandle = PRUICSS_open(CONFIG_PRU_ICSS0); | ||
| if(gPruIcssHandle == NULL) | ||
| { | ||
| DebugP_log("\r\n ERROR: PRUICSS_open failed - NULL handle returned\n"); | ||
| DebugP_assert(0); | ||
| } | ||
|
|
||
| /* Disable PRU0 core */ | ||
| status = PRUICSS_disableCore(gPruIcssHandle, pru0_id); | ||
| DebugP_assert(SystemP_SUCCESS == status); | ||
|
|
||
| /* Disable PRU1 core */ | ||
| status = PRUICSS_disableCore(gPruIcssHandle, pru1_id); |
There was a problem hiding this comment.
1. pruicss_open() not closed 📘 Rule violation ☼ Reliability
The diagnostic app opens a PRUICSS_Handle via PRUICSS_open() but does not close it during cleanup, risking resource leaks and inconsistent hardware state across runs. This violates the requirement to always close/cleanup resources on all paths.
Agent Prompt
## Issue description
`PRUICSS_open()` allocates a PRUICSS handle but the app cleanup path never closes it, which can leak resources and leave PRU-ICSS in an unexpected state.
## Issue Context
The app already has a structured `cleanup:` section and closes PRUI2S instances plus `Board_driversClose()` / `Drivers_close()`, so adding PRUICSS handle closure fits the existing pattern.
## Fix Focus Areas
- examples/pru_i2s/pru_i2s_app/pru_i2s_diagnostic.c[324-336]
- examples/pru_i2s/pru_i2s_app/pru_i2s_diagnostic.c[1415-1426]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+42
to
+73
| main: | ||
| .if $isdefed("I2S_TX") | ||
| ;err_stat = err_stat >> 1. Remove the Rx Overflow error bit. | ||
| LSR err_stat, err_stat, 1 | ||
| ;If there was an underflow/FrameSync Error, clearup thr Tx PingPong Buffer space | ||
| ;err_stat is already initialized during normal operation. | ||
| ; FS_ERROR UNDERFLOW_ERROR | ||
| ; 0 0 No Error | ||
| ; 0 1 Under Flow error | ||
| ; 1 0 FS Error | ||
| ; 1 1 Both errors | ||
| QBEQ CONTIUNE_INIT, err_stat, 0 | ||
| ;If set, this means underflow has happened. | ||
| ;Below Initializations can be avoided but after power on, registers | ||
| ;may contain random addresses and may result in accessing illegal addresses. | ||
| LDI ch0_data_tx, 0x0 | ||
| LDI scratchreg0, 0x0 | ||
| LDI tx_buf_size, 0x0 | ||
| LDI scratchreg0, I2S_TX_BUF_PING_ADD | ||
| LBBO &tx_ping_buffer_address, scratchreg0, 0, 4 | ||
| LDI scratchreg0, I2S_PING_PONG_BUFSIZE_ADD | ||
| LBBO &tx_buf_size, scratchreg0, 0, 2 | ||
| ADD tx_buffer_address_end, tx_ping_buffer_address, tx_buf_size | ||
| ZERO_TX_PING_PONG: | ||
| SBBO &ch0_data_tx, tx_ping_buffer_address, 0, 4 | ||
| ADD tx_ping_buffer_address, tx_ping_buffer_address, 0x4 | ||
| QBGT ZERO_TX_PING_PONG, tx_ping_buffer_address, tx_buffer_address_end | ||
| .endif | ||
|
|
||
| CONTIUNE_INIT: | ||
| ;Clear registers R0-R29. 4*30=120 bytes | ||
| ZERO &r0, 128 |
There was a problem hiding this comment.
2. err_stat used before zero 📘 Rule violation ≡ Correctness
The firmware uses err_stat immediately on entry (shift/branch logic) before clearing/initializing registers, which can make startup behavior nondeterministic. This violates the requirement to clear/initialize registers at startup.
Agent Prompt
## Issue description
`err_stat` is read/modified before the firmware clears registers, so its value at cold start is undefined.
## Issue Context
Startup determinism is required; register zeroing should occur before any logic that depends on register contents.
## Fix Focus Areas
- examples/pru_i2s/firmware/I2S/pru_i2s_main.asm[42-73]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+65
to
+74
| .if $isdefed("PRU0") | ||
| .word 0x10000 ; TX_PING_PONG_BUF_ADDR | ||
| .else | ||
| .word 0x10200 ; TX_PING_PONG_BUF_ADDR | ||
| .endif | ||
| .if $isdefed("NUMBER_OF_TX_3") | ||
| .short 264 ; PING_PONG_BUF_SZ | ||
| .else | ||
| .short 256 ; PING_PONG_BUF_SZ | ||
| .endif |
There was a problem hiding this comment.
3. Hardcoded buffer addresses in fw_regs.asm 📘 Rule violation ⚙ Maintainability
fw_regs.asm encodes buffer addresses and sizes as numeric literals (e.g., 0x10000, 256, 264) rather than named constants, reducing maintainability and increasing risk of boundary mistakes. This violates the no-magic boundary numbers requirement.
Agent Prompt
## Issue description
`fw_regs.asm` uses numeric literals for buffer base addresses and sizes, making it hard to audit/change safely.
## Issue Context
Define these as named constants/macros (or include them from a single header) and use the names in `.word`/`.short` emissions.
## Fix Focus Areas
- examples/pru_i2s/firmware/I2S/fw_regs.asm[65-74]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+33
to
+66
| ; define registers | ||
| .if $isdefed("I2S_PROFILE_FW") | ||
| prux_cycle_cnt_0 .set r0 | ||
| prux_cycle_cnt_1 .set r1 | ||
| scratchreg_cycle_cnt .set r2 | ||
| prux_cycle_cnt .set r3 | ||
| cnt_buffer_address .set r4 | ||
| cnt_buffer_address_1 .set r5 | ||
| .endif | ||
|
|
||
| ; Pin mask registers | ||
| .if !$isdefed("I2S_RX") | ||
| i2s_instance_fs_pin_pos .set r11 | ||
| .else | ||
| i2s_instance_fs_pin_pos .set r7 | ||
| i2s_instance1_sd_rx_pin_pos .set r8 | ||
| i2s_instance2_sd_rx_pin_pos .set r9 | ||
| .endif | ||
|
|
||
| .if !$isdefed("I2S_RX") | ||
| ;In case of Tx only, use some of the Rx registers. | ||
| ; tx_buffer_address keeps track of where we are in the audio buffer | ||
| tx_buffer_address .set r0 | ||
| tx_buffer_address_end .set r1 | ||
| tx_ping_buffer_address .set r2 | ||
| tx_pong_buffer_address .set r3 | ||
| ; ch0_data_tx is 16/32 bit audio sample | ||
| ch0_data_tx .set r4 | ||
| ch1_data_tx .set r5 | ||
| ch2_data_tx .set r6 | ||
| r30_value .set r7 | ||
| r30_value_1 .set r8 | ||
| r30_value_2 .set r9 | ||
| .endif |
There was a problem hiding this comment.
4. Direct register aliases in pru_i2s_regs.h 📘 Rule violation ⚙ Maintainability
The new register header maps functionality to raw PRU registers using .set/.asg aliases (e.g., tx_buffer_address .set r0), rather than using .struct/.sassign-based allocation. This violates the structured register allocation requirement.
Agent Prompt
## Issue description
Register roles are allocated via direct `rN` aliases, increasing the risk of accidental overlap and making audits harder.
## Issue Context
Introduce a `.struct` describing the register layout and assign it using `.sassign`, then reference fields instead of raw `rN` aliases.
## Fix Focus Areas
- examples/pru_i2s/firmware/I2S/pru_i2s_regs.h[33-66]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1694
to
+1765
| if (pSwipAttrs->numTxI2s > 0) | ||
| { | ||
| /* Write Tx ping/pong buffer address */ | ||
| HW_WR_REG32(pSwipAttrs->baseAddr + FW_REG_TX_PING_PONG_BUF_ADDR, pObj->prms.txPingPongBaseAddr); | ||
| } | ||
|
|
||
| if (pSwipAttrs->numRxI2s > 0) | ||
| { | ||
| /* Rx buffer address passed from application is expected to be the target address | ||
| (either ICSS local address or absolute system address). | ||
| This is now application/SysConfig responsibility to provide correctly. | ||
| Firmware will use this address as-is. */ | ||
| tmpRxPpBufAddr = pObj->prms.rxPingPongBaseAddr; | ||
|
|
||
| /* Write Rx ping/pong buffer address */ | ||
| HW_WR_REG32(pSwipAttrs->baseAddr + FW_REG_RX_PING_PONG_BUF_ADDR, tmpRxPpBufAddr); | ||
| } | ||
|
|
||
| /* Write ping/pong buffer size */ | ||
| HW_WR_REG16(pSwipAttrs->baseAddr + FW_REG_PING_PONG_BUF_SZ, pObj->prms.pingPongBufSz); | ||
|
|
||
| /* TBD: other FW reg init */ | ||
|
|
||
| return SystemP_SUCCESS; | ||
| } | ||
|
|
||
| /* Initializes PRU I2S ping/pong buffers */ | ||
| static int32_t PRUI2S_initPpBufs( | ||
| PRUI2S_Config *pCfg | ||
| ) | ||
| { | ||
| PRUI2S_Object *pObj; | ||
| PRUI2S_SwipAttrs *pSwipAttrs; | ||
| const PRUICSS_HwAttrs *pPruIcssHwAttrs; | ||
| int32_t status = SystemP_SUCCESS; | ||
|
|
||
|
|
||
| /* Get pointers */ | ||
| pObj = pCfg->object; | ||
| pSwipAttrs = pCfg->attrs; | ||
|
|
||
|
|
||
| if (pSwipAttrs->numTxI2s > 0) | ||
| { | ||
| /* Get PRUICSS hardware attributes from the handle */ | ||
| pPruIcssHwAttrs = (PRUICSS_HwAttrs *)(pObj->pruIcssHandle->hwAttrs); | ||
|
|
||
| if (pPruIcssHwAttrs != NULL) | ||
| { | ||
| uint32_t pruDramBase = 0; | ||
|
|
||
|
|
||
| /* Get PRU-specific DRAM base address from hardware attributes - device/instance agnostic */ | ||
| /* The PRUICSS handle provides the actual DRAM mapping for any PRU on any device */ | ||
| if (pSwipAttrs->pruInstId == PRUICSS_PRU0) | ||
| { | ||
| pruDramBase = (uint32_t)pPruIcssHwAttrs->pru0DramBase; | ||
| } | ||
| else if (pSwipAttrs->pruInstId == PRUICSS_PRU1) | ||
| { | ||
| pruDramBase = (uint32_t)pPruIcssHwAttrs->pru1DramBase; | ||
| } | ||
| else | ||
| { | ||
| status = SystemP_FAILURE; | ||
| } | ||
|
|
||
| if ((status == SystemP_SUCCESS) && (pruDramBase != 0)) | ||
| { | ||
| /* Calculate Tx buffer address using PRU DRAM base + application offset */ | ||
| pObj->txPingPongBuf = (void *)(pruDramBase + pObj->prms.txPingPongBaseAddr); | ||
| } |
There was a problem hiding this comment.
5. Tx buffer address mismatch 🐞 Bug ≡ Correctness
The API/documentation treats PRUI2S_Params.txPingPongBaseAddr as an absolute CPU address (example casts a pointer), but the driver computes the CPU pointer as (pruDramBase + txPingPongBaseAddr) while firmware is programmed with the raw txPingPongBaseAddr. This inconsistency can make firmware and CPU access different memory regions and break TX data transfer.
Agent Prompt
### Issue description
`txPingPongBaseAddr` has inconsistent semantics between the public API docs, the CPU-side pointer calculation, and the firmware register programming. This can cause the firmware and CPU to use different addresses for the same ping/pong buffer.
### Issue Context
- Header example sets `txPingPongBaseAddr = (uint32_t)txBuffer` (looks like absolute address).
- Driver computes CPU pointer as `pruDramBase + txPingPongBaseAddr` (treats as offset).
- Driver programs firmware with raw `txPingPongBaseAddr`.
### Fix Focus Areas
- examples/pru_i2s/include/pru_i2s_drv.h[164-180]
- examples/pru_i2s/driver/pru_i2s_drv.c[1694-1714]
- examples/pru_i2s/driver/pru_i2s_drv.c[1720-1765]
### What to change
Choose ONE consistent contract and apply it everywhere:
- **Option A (recommended if FW expects PRU-local offsets):**
- Rename/comment `txPingPongBaseAddr` to `txPingPongBaseOffset`
- Update header example to pass an offset within PRU DRAM
- Keep `pObj->txPingPongBuf = pruDramBase + offset` and write the same offset to firmware (if FW interprets it as PRU-local)
- Add range validation that offset+size fits PRU DRAM
- **Option B (absolute address contract):**
- Treat `txPingPongBaseAddr` as absolute CPU/SoC address
- Set `pObj->txPingPongBuf = AddrTranslateP_getLocalAddr(txPingPongBaseAddr)` (or equivalent)
- Program firmware with the address format it actually uses (may require translating to PRU-view address)
Also update PRUI2S_checkOpenParams() to validate the chosen contract (alignment, range, even size).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
6
to
12
| "includePaths": [ | ||
| "../source", | ||
| "C:/ti/mcu_plus_sdk/source/sysconfig", | ||
| ], | ||
| "components": [ | ||
|
|
||
| "/open_pru", | ||
| ], |
There was a problem hiding this comment.
6. Hardcoded sdk include path 🐞 Bug ☼ Reliability
.metadata/product.json adds a Windows-specific absolute include path (C:/ti/...), which will break builds on non-Windows hosts and in CI environments where that path does not exist. This is a portability/build blocker for the repo.
Agent Prompt
### Issue description
A Windows-only absolute include path was added to `.metadata/product.json`, breaking portability and CI.
### Issue Context
`includePaths` contains `C:/ti/mcu_plus_sdk/source/sysconfig`.
### Fix Focus Areas
- .metadata/product.json[5-12]
### What to change
- Remove the absolute path.
- Replace with a repo-relative path (if the headers are in-repo), or use a variable-based mechanism supported by the build system (e.g., environment variable or a path relative to an SDK import root already defined in `imports.mak`).
- Ensure includePaths remains valid on Linux/macOS and Windows without local customization.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Persistent review updated to latest commit 248af3d |
Comment on lines
+109
to
+154
| .if $isdefed("I2S_TX") | ||
| ;read the Tx buffer address provided by host. This should be in Shared memory | ||
| ; point to the PING audio buffer | ||
| LDI scratchreg0, I2S_TX_BUF_PING_ADD | ||
| LBBO &tx_buffer_address, scratchreg0, 0, 4 | ||
| MOV tx_ping_buffer_address, tx_buffer_address | ||
| .endif | ||
|
|
||
| ;read the buffer size provided by host. buffer size is ping+pong | ||
| LDI scratchreg0, I2S_PING_PONG_BUFSIZE_ADD | ||
| LBBO &tx_buf_size, scratchreg0, 0, 2 | ||
| ;Shift right by 1 to get PING/PONG size buffer size | ||
| LSR tx_buf_size, tx_buf_size, 1 | ||
| .if $isdefed("I2S_RX") | ||
| MOV rx_buf_size, tx_buf_size | ||
| ;Rx buffer size is one less than PING/PONG buffer size | ||
| SUB rx_buf_size, rx_buf_size, 1 | ||
|
|
||
| ;read the Rx buffer address provided by host | ||
| LDI scratchreg0, I2S_RX_BUF_PING_ADD | ||
| LBBO &rx_buffer_address, scratchreg0, 0, 4 | ||
| MOV rx_ping_buffer_address, rx_buffer_address | ||
| LDI rx_sd_counter, I2S_SAMPLES_PER_CHANNEL | ||
| .endif | ||
|
|
||
| ;Initialize the end address of the Tx/Rx buffers | ||
| .if $isdefed("I2S_TX") | ||
| ADD tx_buffer_address_end, tx_buffer_address, tx_buf_size | ||
| .endif | ||
| .if $isdefed("I2S_RX") | ||
| ADD rx_buffer_address_end, rx_buffer_address, rx_buf_size | ||
| .endif | ||
|
|
||
| ;Initialize the PONG buffer addresses | ||
| .if $isdefed("I2S_TX") | ||
| MOV tx_pong_buffer_address, tx_buffer_address_end | ||
| .endif | ||
| .if $isdefed("I2S_RX") | ||
| MOV rx_pong_buffer_address, rx_buffer_address_end | ||
| ADD rx_pong_buffer_address, rx_pong_buffer_address, 1 | ||
| .endif | ||
|
|
||
| .if $isdefed("I2S_TX") | ||
| ;Read the first 8/12 bytes from Ping buffer | ||
| LBBO &ch0_data_tx, tx_buffer_address, 0, BYTES_TO_LOAD | ||
| ADD tx_buffer_address, tx_buffer_address, BYTES_TO_LOAD |
There was a problem hiding this comment.
1. tx_buffer_address lacks bounds checks 📘 Rule violation ⛨ Security
The PRU firmware reads host-provided buffer address/size values and immediately uses them for LBBO and end-address arithmetic without validating the range or checking for overflow. This can cause out-of-bounds reads/writes or unintended memory access if the host provides invalid values or if address + size wraps.
Agent Prompt
## Issue description
The PRU firmware uses host-provided buffer addresses/sizes for `LBBO` and end-address calculations without validating the range or checking for `address + size` overflow.
## Issue Context
Compliance requires bounds checks for computed memory accesses in PRU assembly to prevent out-of-bounds access and memory corruption.
## Fix Focus Areas
- examples/pru_i2s/firmware/I2S/pru_i2s_main.asm[109-156]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+303
to
+305
| * Get PRUI2S SW IP attributes for use in application, e.g. | ||
| * - # Tx I2S | ||
| * - # Rx I2S |
There was a problem hiding this comment.
2. e.g./i.e. in comments 📘 Rule violation ⚙ Maintainability
New documentation/comments use e.g. and i.e. instead of the approved phrases. This reduces documentation consistency and violates the required wording standard.
Agent Prompt
## Issue description
Comments/documentation introduce disallowed abbreviations `e.g.` and `i.e.`.
## Issue Context
Compliance requires using spelled-out equivalents: use `for example` instead of `e.g.`, and `that is` instead of `i.e.`.
## Fix Focus Areas
- examples/pru_i2s/driver/pru_i2s_drv.c[303-305]
- examples/pru_i2s/driver/pru_i2s_drv.c[743-746]
- examples/pru_i2s/include/pru_i2s_drv.h[409-416]
- examples/pru_i2s/pru_i2s_app/pru_i2s_diagnostic.c[96-100]
- examples/pru_i2s/README.md[375-395]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+35
to
+41
| .sect ".text:main" | ||
| .clink | ||
| .global main | ||
|
|
||
| .include "pru_i2s_interface.h" | ||
| .include "pru_i2s_regs.h" | ||
|
|
There was a problem hiding this comment.
3. Tabs in pru assembly 📘 Rule violation ⚙ Maintainability
The added PRU assembly uses tab characters for indentation/alignment instead of the required whitespace rules. This can create inconsistent formatting across environments and violates the indentation/whitespace standard.
Agent Prompt
## Issue description
PRU assembly uses tab characters for indentation/alignment.
## Issue Context
Compliance requires 4-space indentation and disallows tabs in assembly sources.
## Fix Focus Areas
- examples/pru_i2s/firmware/I2S/pru_i2s_main.asm[35-41]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1
to
+4
| ; | ||
| ; Copyright (C) 2024-2025 Texas Instruments Incorporated | ||
| ; | ||
| ; Redistribution and use in source and binary forms, with or without |
There was a problem hiding this comment.
4. Assembly header missing filename 📘 Rule violation ⚙ Maintainability
The new PRU assembly sources include TI copyright text but do not include the required file name in the source header. This violates the required source header format for attribution and identification.
Agent Prompt
## Issue description
PRU assembly sources are missing the file name in the header comment block.
## Issue Context
Compliance requires each source/header file header to include the file name and TI attribution.
## Fix Focus Areas
- examples/pru_i2s/firmware/I2S/pru_i2s_main.asm[1-4]
- examples/pru_i2s/firmware/TDM4/pru_i2s_main.asm[1-4]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| $(OBJDIR)/%.obj %.obj: %.asm | ||
| @echo 'Building file: "$^"' | ||
| @echo 'Invoking: PRU Compiler' | ||
| "$(CGT_TI_PRU_PATH)/bin/clpru" -DPRU0 -DSLICE0 -v4 --define=SOC_AM263X --define=PRU0 --define=I2S_TX --define=NUMBER_OF_TX_3 --define=I2S_TX_DETECT_UNDERFLOW --include_path="${CG_TOOL_ROOT}/include" --include_path="${MCU_PLUS_SDK_PATH}/source" --include_path="${OPEN_PRU_PATH}/source" --include_path="${OPEN_PRU_PATH}/examples/pru_i2s/firmware/include" --include_path="${OPEN_PRU_PATH}/examples/pru_i2s/firmware/I2S" --define=_DEBUG_=1 -g --diag_warning=225 --diag_wrap=off --display_error_number --endian=little --preproc_with_compile --preproc_dependency="$(basename $(<F)).d_raw" $^ |
There was a problem hiding this comment.
5. clpru line exceeds 129 📘 Rule violation ⚙ Maintainability
A new makefile line containing the clpru invocation exceeds the hard 129-character column limit. This reduces readability and violates the repository’s source column-limit standard.
Agent Prompt
## Issue description
A makefile command line exceeds the 129-character hard limit.
## Issue Context
Compliance requires non-markdown/non-text source files to stay within a hard maximum of 129 characters per line.
## Fix Focus Areas
- examples/pru_i2s/firmware/I2S/pru0_tx/am263x-cc/icssm0-pru0_fw/ti-pru-cgt/makefile[63-63]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+476
to
+519
| /* Construct interrupts */ | ||
| if (pObj->prms.i2sTxCallbackFxn != NULL) | ||
| { | ||
| HwiP_Params_init(&hwiPrms); | ||
| hwiPrms.intNum = pSwipAttrs->i2sTxHostIntNum; | ||
| hwiPrms.callback = pObj->prms.i2sTxCallbackFxn; | ||
| hwiPrms.priority = pObj->prms.i2sTxIntrPri; | ||
| hwiPrms.args = (void *)pCfg; | ||
| status = HwiP_construct(&pObj->i2sTxHwiObj, &hwiPrms); | ||
| if (status == SystemP_SUCCESS) | ||
| { | ||
| pObj->i2sTxHwiHandle = &pObj->i2sTxHwiObj; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| } | ||
|
|
||
| if (status == SystemP_SUCCESS && pObj->prms.i2sRxCallbackFxn != NULL) | ||
| { | ||
| HwiP_Params_init(&hwiPrms); | ||
| hwiPrms.intNum = pSwipAttrs->i2sRxHostIntNum; | ||
| hwiPrms.callback = pObj->prms.i2sRxCallbackFxn; | ||
| hwiPrms.priority = pObj->prms.i2sRxIntrPri; | ||
| hwiPrms.args = (void *)pCfg; | ||
| status = HwiP_construct(&pObj->i2sRxHwiObj, &hwiPrms); | ||
| if (status == SystemP_SUCCESS) | ||
| { | ||
| pObj->i2sRxHwiHandle = &pObj->i2sRxHwiObj; | ||
| } | ||
| } | ||
| else if (status == SystemP_SUCCESS) | ||
| { | ||
| } | ||
|
|
||
| if (status == SystemP_SUCCESS && pObj->prms.i2sErrCallbackFxn != NULL) | ||
| { | ||
| HwiP_Params_init(&hwiPrms); | ||
| hwiPrms.intNum = pSwipAttrs->i2sErrHostIntNum; | ||
| hwiPrms.callback = pObj->prms.i2sErrCallbackFxn; | ||
| hwiPrms.priority = pObj->prms.i2sErrIntrPri; | ||
| hwiPrms.args = (void *)pCfg; | ||
| status = HwiP_construct(&pObj->i2sErrHwiObj, &hwiPrms); | ||
| if (status == SystemP_SUCCESS) |
There was a problem hiding this comment.
6. Host irq numbers unset 🐞 Bug ≡ Correctness
PRUI2S_open() constructs HwiP interrupts using pSwipAttrs->i2s*HostIntNum, but these fields are never populated (remain 0 from the header defaults), so callbacks won't run on the intended PRU events. This breaks Tx/Rx/error interrupt handling and any semaphore-driven streaming loops.
Agent Prompt
### Issue description
`PRUI2S_open()` constructs HwiP interrupts with `pSwipAttrs->i2sTxHostIntNum/i2sRxHostIntNum/i2sErrHostIntNum`, but these values are never initialized and default to `0`, so interrupts are bound to the wrong CPU interrupt.
### Issue Context
- Firmware image parsing populates **system event numbers**, not CPU/VIM interrupt numbers.
- The driver must either (a) be provided host interrupt numbers from SysConfig/board config, or (b) derive them from the INTC init configuration.
### Fix Focus Areas
- examples/pru_i2s/driver/pru_i2s_drv.c[474-522]
- examples/pru_i2s/driver/pru_i2s_drv.c[1399-1442]
- examples/pru_i2s/include/pru_i2s_drv.h[227-326]
### Suggested fix
- Add explicit configuration for host interrupt numbers (e.g., in `PRUI2S_Params`, or via a new `PRUI2S_setHostIntNums()` API) and validate them in `PRUI2S_checkOpenParams()`.
- Ensure the driver’s *single* SwipAttrs instance is updated before `HwiP_construct()`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
- Build fix Fixes: PINDSW-8487,9295 Signed-off-by: Rajul Bhambay <r-bhambay@ti.com>
rajul-bhambay
force-pushed
the
a0497643_PINDSW-8487_PRU_I2S
branch
from
248af3d to
2dd5549
Compare
- Build fix Fixes: PINDSW-8487,9295 Signed-off-by: Rajul Bhambay <r-bhambay@ti.com>
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
5 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.
User description
Fixes: PINDSW-8487
PR Type
Enhancement
Description
• Complete PRU I2S driver implementation with comprehensive API for initialization, open/close, read/write operations
• Support for both I2S and TDM4 modes with interleaved/non-interleaved data format conversion
• PRU firmware binaries and assembly code for both TX and RX operations on PRU0 and PRU1
• Interrupt handling with callback mechanisms for Tx, Rx, and error events
• Hardware abstraction layer for PRU firmware loading, ICSS INTC configuration, and GPIO setup
• Diagnostic application with FreeRTOS support for AM263X and AM261X platforms
• TCA6416 IO expander driver for hardware configuration
• Build system integration with makefiles, linker scripts, and project configurations
• Runtime Object View (ROV) configuration for debugging support
Diagram Walkthrough
File Walkthrough
20 files
pru_i2s_drv.c
Complete PRU I2S driver implementation with hardware abstractionsource/pru_i2s/driver/pru_i2s_drv.c
• Complete PRU I2S driver implementation with initialization,
open/close, read/write operations
• Support for both interleaved and
non-interleaved data formats with conversion functions
• Interrupt
handling for Tx, Rx, and error events with callback mechanisms
•
Hardware abstraction for PRU firmware loading, ICSS INTC
configuration, and GPIO setup
main.c
FreeRTOS main application for AM263x PRU I2S diagnosticexamples/pru_i2s_diagnostic/single_channel/am263x-cc/r5fss0-0_freertos/main.c
• FreeRTOS main application entry point for AM263x platform
• Task
creation and scheduler initialization for PRU I2S diagnostic example
•
System and board initialization with proper task stack allocation
pru_i2s_drv.h
PRU I2S Driver Header Implementationsource/pru_i2s/include/pru_i2s_drv.h
• Complete PRU I2S driver header file with comprehensive API
definitions
• Defines data structures for configuration, parameters,
and I/O buffers
• Includes function declarations for driver
initialization, open/close, read/write operations
• Contains
hardware-specific constants and register definitions for AM263X and
AM261X SoCs
pru_i2s_diagnostic.c
PRU I2S Diagnostic Application Implementationexamples/pru_i2s_diagnostic/pru_i2s_diagnostic.c
• Complete diagnostic application for PRU I2S functionality testing
•
Implements Tx/Rx interrupt handlers with error handling and statistics
tracking
• Supports both TDM and I2S modes with configurable firmware
selection
• Includes I2C IO expander configuration for AM263X hardware
setup
icss_pru_i2s_fw.h
TDM4 Firmware Register Definitionssource/pru_i2s/firmware/TDM4/icss_pru_i2s_fw.h
• Firmware register definitions and bit field mappings for TDM4 mode
•
Defines register offsets, addresses, and bit field masks for PRU I2S
firmware
• Includes ping-pong buffer control and error status register
definitions
icss_pru_i2s_fw.h
I2S Firmware Register Definitionssource/pru_i2s/firmware/I2S/icss_pru_i2s_fw.h
• Firmware register definitions and bit field mappings for standard
I2S mode
• Identical structure to TDM4 version with same register
layout and definitions
• Provides firmware interface constants for I2S
protocol implementation
ioexp_tca6416.c
TCA6416 IO Expander Driver Implementationexamples/pru_i2s_diagnostic/board/ioexp_tca6416.c
• I2C IO expander driver implementation for TCA6416 chip
• Provides
functions for opening, configuring, and controlling GPIO pins
•
Includes I2C transaction handling and register read/write operations
pru_i2s_interface.h
New I2S firmware interface header with pin configurationssource/pru_i2s/firmware/I2S/pru_i2s_interface.h
• Added new assembly header file defining I2S interface constants and
pin configurations
• Includes conditional compilation for different
SoCs (AM263X, AM261X) and PRU instances
• Defines pin mappings, buffer
addresses, error positions, and interrupt event numbers
• Contains
configuration for both TX and RX I2S instances with different pin
assignments
pru_i2s_interface.h
New TDM4 firmware interface header with TDM-specific configurationssource/pru_i2s/firmware/TDM4/pru_i2s_interface.h
• Added TDM4 variant of I2S interface header with similar structure to
I2S version
• Includes same SoC and PRU conditional compilation
support
• Defines TDM-specific constants like
TDM_CHANNELSandMAX_TDM_CHANNELS• Contains different
BYTES_TO_LOADvalue (2) andsamples per channel (16) for TDM mode
data.h
I2S diagnostic example data definitions and buffer configurationsexamples/pru_i2s_diagnostic/data.h
• Added comprehensive data definitions for I2S diagnostic example
•
Defines buffer sizes, constants for audio processing, and error
handling
• Includes ping-pong buffer configurations and memory layout
definitions
• Contains pre-initialized TX buffer data with test
patterns
ioexp_tca6416.h
TCA6416 IO expander driver header with complete APIexamples/pru_i2s_diagnostic/board/ioexp_tca6416.h
• Added complete TCA6416 IO expander driver header file
• Defines API
functions for opening, configuring, and controlling IO expander
•
Includes data structures for configuration parameters and attributes
•
Provides constants for input/output modes and pin states
pru_i2s_tdm4_pru0_array.h
PRU0 TDM4 firmware binary arraysource/pru_i2s/firmware/TDM4/pru_i2s_tdm4_pru0_array.h
• Added compiled PRU0 firmware binary array for TDM4 mode
• Contains
both instruction array (
pru_prupru_i2s0_image_0_0) and data array•
Provides pre-compiled firmware ready for loading into PRU0
pru_i2s_pru0_array.h
PRU0 I2S firmware binary arraysource/pru_i2s/firmware/I2S/pru_i2s_pru0_array.h
• Added compiled PRU0 firmware binary array for standard I2S mode
•
Contains instruction and data arrays for PRU0 I2S firmware
• Provides
pre-compiled firmware binary ready for deployment
pru_i2s_tdm4_pru1_array.h
PRU1 TDM4 firmware binary arraysource/pru_i2s/firmware/TDM4/pru_i2s_tdm4_pru1_array.h
• Added compiled PRU1 firmware binary array for TDM4 mode
• Contains
instruction array and data array for PRU1 TDM4 firmware
• Provides
pre-compiled firmware for PRU1 in TDM4 configuration
pru_i2s_pru1_array.h
PRU1 I2S firmware binary arraysource/pru_i2s/firmware/I2S/pru_i2s_pru1_array.h
• Added compiled PRU1 firmware binary array for standard I2S mode
•
Contains instruction and data arrays for PRU1 I2S firmware
• Provides
pre-compiled firmware binary for PRU1 deployment
pru_i2s_regs.h
TDM4 PRU register definitions and mappingssource/pru_i2s/firmware/TDM4/pru_i2s_regs.h
• Added TDM4-specific register definitions and assignments
• Defines
PRU register mappings for TX/RX operations, counters, and buffers
•
Includes conditional compilation for different I2S modes (TX/RX)
•
Contains register assignments for ping-pong buffers and status
tracking
pru_i2s_regs.h
I2S PRU register definitions and mappingssource/pru_i2s/firmware/I2S/pru_i2s_regs.h
• Added I2S-specific register definitions and assignments
• Defines
PRU register mappings similar to TDM4 but for standard I2S
• Includes
register assignments for audio data, buffers, and control
• Contains
conditional compilation for TX/RX modes and profiling
main.c
AM261X I2S diagnostic main application with FreeRTOSexamples/pru_i2s_diagnostic/single_channel/am261x-lp/r5fss0-0_freertos/main.c
• Added main application file for AM261X LaunchPad I2S diagnostic
•
Implements FreeRTOS-based main function with task creation
• Calls
pru_i2s_diagnostic_mainfunction and handles task management•
Includes standard system and board initialization
fw_regs.asm
I2S firmware register initialization assemblysource/pru_i2s/firmware/I2S/fw_regs.asm
• Added firmware register initialization assembly file for I2S
•
Defines firmware register section with configuration values
• Includes
conditional compilation for different TX configurations
• Sets up pin
numbers, buffer addresses, and system event numbers
fw_regs.asm
TDM4 firmware register initialization assemblysource/pru_i2s/firmware/TDM4/fw_regs.asm
• Added firmware register initialization assembly file for TDM4
•
Similar structure to I2S version with TDM4-specific configurations
•
Defines register section with buffer addresses and pin assignments
•
Includes conditional compilation for different modes and PRU instances
8 files
pru_i2s_pruss_intc_mapping.h
PRUSS interrupt controller mapping definitions and macrossource/pru_i2s/include/pru_i2s_pruss_intc_mapping.h
• PRUSS interrupt controller mapping definitions and constants
•
System event, channel, and host interrupt mapping macros
• INTC
initialization data structure template with proper MISRA C compliance
syscfg_c.rov.xs
ROV configuration for AM261x FreeRTOS debuggingexamples/pru_i2s_diagnostic/single_channel/am261x-lp/r5fss0-0_freertos/ti-arm-clang/syscfg_c.rov.xs
• Runtime Object View (ROV) configuration file for debugging support
•
FreeRTOS ROV integration for AM261x platform
product.json
Product Metadata Configuration Update.metadata/product.json
• Added include path for sysconfig directory
• Added "/open_pru"
component to the components list
pru_i2s_master_icss.cmd
I2S PRU firmware linker command filesource/pru_i2s/firmware/I2S/pru_i2s_master_icss.cmd
• Added linker command file for I2S PRU firmware
• Defines memory
layout for ICSSG PRU with instruction and data memory sections
•
Includes peripheral memory mappings and section allocations
•
Configures firmware registers, output samples, and debug buffer
sections
pru_i2s_master_icss.cmd
TDM4 PRU firmware linker command filesource/pru_i2s/firmware/TDM4/pru_i2s_master_icss.cmd
• Added linker command file for TDM4 PRU firmware
• Identical
structure to I2S linker file with same memory layout
• Defines ICSSG
memory sections and peripheral mappings
• Configures section
allocations for TDM4 firmware variant
syscfg_c.rov.xs
ROV configuration for FreeRTOS debuggingexamples/pru_i2s_diagnostic/single_channel/am263x-cc/r5fss0-0_freertos/ti-arm-clang/syscfg_c.rov.xs
• Added Runtime Object View (ROV) configuration file
• Defines ROV
files for FreeRTOS debugging support
• Simple configuration file for
debugging tools integration
makefile_ccs_bootimage_gen
Boot image generation makefile with security featuresexamples/pru_i2s_diagnostic/single_channel/am261x-lp/r5fss0-0_freertos/ti-arm-clang/makefile_ccs_bootimage_gen
• Added comprehensive makefile for boot image generation
• Defines
build targets for different image formats (appimage, MCELF, signed)
•
Includes multi-core image generation and XIP support
• Contains
security features like image signing and encryption
makefile
Added I2S diagnostic example to build systemexamples/makefile
• Added
pru_i2s_diagnosticto the list of subdirectories to build•
Simple one-line addition to include new example in build system
37 files