Am263x: PRU-I2S - #8
Conversation
Pending Items: |
96bf0b7 to
a27184b
Compare
|
Hey @rajul-bhambay, what is TDM4? I assume something to do with time-domain multiplexing, instead of a typo for TDA4? |
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). |
26e87e1 to
29b5691
Compare
| @@ -0,0 +1,186 @@ | |||
| %%{ | |||
There was a problem hiding this comment.
I don't think we need this file. Can we remove?
| @@ -0,0 +1,77 @@ | |||
| %%{ | |||
There was a problem hiding this comment.
I don't think we need this file. Can we remove?
| @@ -0,0 +1,52 @@ | |||
| /* | |||
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 | |||
There was a problem hiding this comment.
Update copyright year in all files
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) |
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)*/ |
There was a problem hiding this comment.
I assume we have tested both modes. Can you confirm?
| */ | ||
|
|
||
|
|
||
| void i2s_i2c_io_expander(void) |
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); |
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 @@ | |||
| /* | |||
There was a problem hiding this comment.
Can we move this to SysConfig?
|
Can we merge commit? Don't think we need 4 |
cd4d050 to
172a2d0
Compare
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 | |||
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> |
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" |
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 \ |
There was a problem hiding this comment.
Next line is not really warranted here?
| pSwipAttrs->rxPin[i].pinNum = temp8b; | ||
| } | ||
|
|
||
| /* TBD: SoC PAD address lookup */ |
There was a problem hiding this comment.
Remove if not relevant anymore
| @@ -0,0 +1,200 @@ | |||
| /* | |||
| * Copyright (c) 2021, Texas Instruments Incorporated | |||
| @@ -0,0 +1,280 @@ | |||
| /* | |||
| * Copyright (C) 2021 Texas Instruments Incorporated | |||
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. | |||
There was a problem hiding this comment.
Add firmware design document in md format here under firmware/I2S/docs
There was a problem hiding this comment.
@rajul-bhambay should we have a seperate design document for this if its covered in the readme ?
034f165 to
52e4c10
Compare
|
@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
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.
|
/review |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| 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
| /* 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
8ab787e to
d9175ef
Compare
|
Persistent review updated to latest commit d9175ef |
| 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
| .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
|
Persistent review updated to latest commit 248af3d |
| .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
248af3d to
2dd5549
Compare
|
Persistent review updated to latest commit ca2034c |
| 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: |
There was a problem hiding this comment.
1. Uppercase opcodes in pru_i2s_main.asm 📘 Rule violation ⚙ Maintainability
PRU assembly instruction mnemonics are written in uppercase (for example LSR, LDI, LBBO, ADD) in the newly added firmware. This violates the required opcode casing convention and reduces consistency/readability.
Agent Prompt
## Issue description
New PRU assembly uses uppercase instruction mnemonics, violating the required lowercase opcode convention.
## Issue Context
Maintaining consistent opcode casing improves readability and reduces review friction across firmware sources.
## Fix Focus Areas
- examples/pru_i2s/firmware/I2S/pru_i2s_main.asm[45-71]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| /* 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.
2. Static globals in header 🐞 Bug ⚙ Maintainability
examples/pru_i2s/include/pru_i2s_drv.h defines driver state (gPruI2sDrvInit, gPruI2sObject, gPruI2sConfig, etc.) as static, so every .c including this header allocates its own private copy. This bloats RAM/flash and can cause state divergence or confusion as the project grows (and is already included by multiple translation units in this PR).
Agent Prompt
### Issue description
`pru_i2s_drv.h` contains `static` definitions for driver global state. Because the header is included by multiple `.c` files, each translation unit gets its own copy, wasting memory and risking future state divergence.
### Issue Context
This driver is used by the diagnostic app and app-config code, both of which include `pru_i2s_drv.h`.
### Fix Focus Areas
- Move all driver state/storage definitions (init flags, config tables, objects, debug buffers) from the header into the implementation file and keep them `static` there.
- In the header, keep only type definitions, function prototypes, and (if absolutely required) `extern` declarations.
### Fix Focus Areas (code locations)
- examples/pru_i2s/include/pru_i2s_drv.h[262-326]
- examples/pru_i2s/driver/pru_i2s_drv.c[1-120]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Persistent review updated to latest commit b92802e |
- TDM functionality - I2S functionality Fixes: PINDSW-8487,9295 Signed-off-by: Rajul Bhambay <r-bhambay@ti.com>
- Build fix Fixes: PINDSW-8487,9295 Signed-off-by: Rajul Bhambay <r-bhambay@ti.com>
- Build fix Fixes: PINDSW-8487,9295 Signed-off-by: Rajul Bhambay <r-bhambay@ti.com>
PR Summary by QodoAdd PRU-based I2S and TDM4 audio support for AM26x
AI Description
Diagram
High-Level Assessment
Files changed (82)
|
Code Review by Qodo
1. Firmware copy failures ignored
|
|
Code review by qodo was updated up to the latest commit e9fcd7d |
|
Code review by qodo was updated up to the latest commit c56130b |
|
Code review by qodo was updated up to the latest commit b3ef566 |
| ; 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.
1. Startup misdetects tx errors 🐞 Bug ☼ Reliability
The firmware reads and branches on err_stat before registers are initialized, so an indeterminate cold-start value can falsely enter TX recovery. That recovery clears the configured transmit buffer, destroying audio queued through PRUI2S_write() before the PRU starts.
Agent Prompt
## Issue description
The I2S TX firmware tests `err_stat` before initializing the register that contains it. A cold startup can therefore be mistaken for an underflow or frame-sync recovery and erase a transmit buffer prepared by the host.
## Issue Context
`err_stat` aliases `r27.b2`, while the general register clear occurs only after the recovery branch. Preserve intentional restart recovery only when the error state comes from a validated persistent source; otherwise initialize it before testing it.
## Fix Focus Areas
- examples/pru_i2s/firmware/I2S/pru_i2s_main.asm[45-73]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
need to discuss about this comment as this might be a big change if implemented @rajul-bhambay
|
Code review by qodo was updated up to the latest commit d967a77 |
- Fixed qodo comments Fixes: PINDSW-8487,9295 Signed-off-by: Rajul Bhambay <r-bhambay@ti.com>
|
Code review by qodo was updated up to the latest commit 79aff20 |
6 participants
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