Fix issue #120: Add HW spinlock usage examples (assembly/C) in academy/spinlock#142
Open
pratheesh wants to merge 2 commits into
Open
Fix issue #120: Add HW spinlock usage examples (assembly/C) in academy/spinlock#142pratheesh wants to merge 2 commits into
pratheesh wants to merge 2 commits into
Conversation
Code Review by Qodo
Context used 1. Busy-wait worst-case undocumented
|
PR Summary by QodoAdd PRU HW spinlock lab examples (C + assembly) under academy/spinlock WalkthroughsDescription• Add PRU-local HW spinlock usage examples in both C and pure assembly. • Provide C-callable acquire/release helpers built on XIN/XOUT broadside access. • Document spinlock semantics, build steps, and correct acquire/release patterns. Diagramgraph TD
A["PRU0 firmware"] --> B["main.c"] --> C["spinlock.asm"] --> D{{"HW spinlock (XID 0x90)"}}
E["PRU1 firmware"] --> B --> F{{"Debug GPO pins"}}
G["spinlock_example.asm"] --> D
H["readme.md"] --> B
High-Level AssessmentThe following are alternative approaches to this PR: 1. Implement acquire/release directly in C via __xin/__xout intrinsics
2. Provide a small reusable spinlock library API (local + remote subsystem variants)
Recommendation: Keep the current approach: a minimal asm helper for C plus a pure-asm macro example is the best teaching fit and matches the TRM semantics (R1.b0 id, status in R1.b3 bit0, XID 0x90). If this pattern spreads to more labs, consider adding an optional reusable library wrapper later. File ChangesEnhancement (3)
Documentation (1)
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+51
to
+65
| ### Assembly | ||
|
|
||
| The spinlock is accessed by reading and writing to the memory-mapped address. The following assembly snippet shows acquiring lock number 0: | ||
|
|
||
| ``` | ||
| LBBO &r0, r0, CSL_SPINLOCK0_BASE, 4 ; Read the spinlock register (lock 0) | ||
| QBEQ acquire, r0, 0 ; If lock is free (0), we got it | ||
| SPINLOOP: ; Otherwise, spin | ||
| LBBO &r0, r0, CSL_SPINLOCK0_BASE, 4 | ||
| QBNE SPINLOOP, r0, 0 | ||
| acquire: | ||
| ; ... critical section ... | ||
| ; Release the lock by writing 0 | ||
| SBCO &r0, r0, CSL_SPINLOCK0_BASE, 4, 1 | ||
| ``` |
There was a problem hiding this comment.
1. xin/xout spinlock example missing 📎 Requirement gap ≡ Correctness
The new spinlock lab demonstrates memory-mapped polling (LBBO/SBCO and volatile pointer loops) rather than the required local spinlock accelerator flow using XIN polling for acquisition status and XOUT for release. This fails the requirement to provide documented, working PRU Assembly and C examples specifically showing the __xin/__xout-based mechanism and register-byte ID/status usage.
Agent Prompt
## Issue description
The spinlock examples use memory-mapped loads/stores and do not demonstrate the required local spinlock accelerator access using XIN/XOUT (and `__xin`/`__xout`). The C example also relies on `SPINLOCK0_BASE` but does not provide a working, self-contained definition/selection, so it is not clearly buildable as-is.
## Issue Context
Compliance requires two working examples (PRU Assembly and C) demonstrating HW spinlock acquire (XIN-based polling on local spinlock XID) and release (XOUT), including documentation/comments mapping: local spinlock XID, spinlock ID byte location, valid ID range, and how the acquisition status is checked.
## Fix Focus Areas
- academy/spinlock/readme.md[51-87]
- academy/spinlock/spinlock_example.p[1-35]
- academy/spinlock/spinlock_example.c[15-76]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1
to
+10
| /* | ||
| * Spinlock Example - C Version | ||
| * | ||
| * This example shows how to use the hardware spinlock module for mutual | ||
| * exclusion between PRU cores or between PRU and host. | ||
| * | ||
| * Note: This example uses direct register access. If your device provides | ||
| * a Chip Support Layer (CSL) with spinlock APIs, you may prefer to use | ||
| * those instead for better portability. | ||
| */ |
There was a problem hiding this comment.
3. Missing ti file headers 📘 Rule violation ⚙ Maintainability
The new spinlock_example.c and spinlock_example.p sources do not include the required header fields including the file name and a Texas Instruments copyright statement. This reduces traceability and violates the repository’s required legal/header convention.
Agent Prompt
## Issue description
New source files are missing required headers: explicit file name and Texas Instruments copyright statement.
## Issue Context
Repository compliance requires standardized headers for traceability and legal compliance.
## Fix Focus Areas
- academy/spinlock/spinlock_example.c[1-14]
- academy/spinlock/spinlock_example.p[1-14]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 035d859 |
Comment on lines
+59
to
+65
| acquire_lock0: | ||
| ; Atomic test-and-set: a read of the lock register attempts to take it. | ||
| lbbo &LOCK_VAL, SPINLOCK_BASE_REG, LOCK0_OFFSET, 4 | ||
| ; read != 0 -> lock was already held, keep spinning. | ||
| ; read == 0 -> lock is now OWNED by this PRU, fall through. | ||
| qbne acquire_lock0, LOCK_VAL, 0 | ||
|
|
There was a problem hiding this comment.
1. Busy-wait loop lacks bound 📘 Rule violation ☼ Reliability
The assembly acquire loop can spin forever because it has no timeout or max-iteration termination condition. This can cause non-deterministic firmware behavior and a hard hang if the lock is never released.
Agent Prompt
## Issue description
The PRU assembly busy-wait acquire loop has no explicit termination bound, so it can run indefinitely.
## Issue Context
Compliance requires deterministic loops with termination conditions/bounds checking in PRU assembly.
## Fix Focus Areas
- academy/spinlock/spinlock_example.asm[59-65]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+37
to
+40
| ; definitions | ||
| .asg R2, SPINLOCK_BASE_REG ; holds the Spinlock module base address | ||
| .asg R3, LOCK_VAL ; scratch for the value read from / written to the lock | ||
|
|
There was a problem hiding this comment.
2. Uses .asg register aliases 📘 Rule violation ⚙ Maintainability
The assembly example allocates registers via .asg aliases instead of using .struct/.sassign. This increases the risk of register overlap/corruption and violates the required structured register mapping convention.
Agent Prompt
## Issue description
PRU assembly code must map registers using `.struct` and `.sassign`, rather than `.asg` aliases to raw `R*` registers.
## Issue Context
Structured register allocation reduces overlap/corruption bugs and improves maintainability.
## Fix Focus Areas
- academy/spinlock/spinlock_example.asm[37-40]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 3164531 |
Comment on lines
+1
to
+16
| /* | ||
| * SPDX-License-Identifier: BSD-3-Clause | ||
| * Copyright (C) 2024-2026 Texas Instruments Incorporated - http://www.ti.com/ | ||
| * | ||
| * Spinlock Example - C Version (PRU_ICSSG local hardware spinlock) | ||
| * | ||
| * Demonstrates mutual exclusion between PRU0 and PRU1 using the PRU-local | ||
| * hardware spinlock, accessed over the broadside interface (XIN/XOUT). The | ||
| * low-level acquire/release are implemented in spinlock.asm; this file shows | ||
| * the usage pattern. | ||
| * | ||
| * Each PRU repeatedly takes the same spinlock, drives a debug GPO high while it | ||
| * holds the lock, waits a (per-core different) number of cycles, then releases | ||
| * and drives the debug GPO low. Probing the two debug pins shows that the two | ||
| * cores never hold the lock at the same time. | ||
| */ |
There was a problem hiding this comment.
1. Main.c header missing filename 📘 Rule violation ⚙ Maintainability
academy/spinlock/main.c has a copyright/SPDX header but does not denote the file name in the header section. This violates the required source file header convention and reduces traceability.
Agent Prompt
## Issue description
`academy/spinlock/main.c` is missing the required header field that denotes the file name.
## Issue Context
The compliance checklist requires each source file header to include both the file name and the TI copyright statement.
## Fix Focus Areas
- academy/spinlock/main.c[1-16]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+35
to
+36
| /* TODO: pinmux the debug signals (see the GPIO lab) and update the shift to */ | ||
| /* match your board. PRU0 is selected via -DPRU0 in the makefile. */ |
There was a problem hiding this comment.
2. todo marker in main.c 📘 Rule violation ⚙ Maintainability
academy/spinlock/main.c introduces a TODO marker, which is disallowed by the repository marker standard. This makes tracking inconsistent and violates the required pending-work marker convention.
Agent Prompt
## Issue description
The code introduces a `TODO` marker; the repository standard requires `FIXME` instead.
## Issue Context
Use `FIXME` for pending work markers to keep consistency across the codebase.
## Fix Focus Areas
- academy/spinlock/main.c[35-36]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+28
to
+29
| uint8_t spinlock_acquire(uint8_t flag_id); | ||
| void spinlock_release(uint8_t flag_id); |
There was a problem hiding this comment.
3. Multiple spaces in main.c 📘 Rule violation ⚙ Maintainability
academy/spinlock/main.c contains multiple consecutive spaces within declarations for alignment (for example between void and spinlock_release). This violates the whitespace policy that restricts whitespace to single spaces and newlines.
Agent Prompt
## Issue description
The file uses multiple consecutive spaces for alignment, but the whitespace policy requires single spaces only.
## Issue Context
This applies to changed content in source files; alignment should be done without introducing multiple spaces (use one space and rely on formatting tools/settings if needed).
## Fix Focus Areas
- academy/spinlock/main.c[28-29]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+29
to
+51
| ;****************************************************************************** | ||
| ; Macro: M_SPINLOCK_ACQUIRE | ||
| ; Acquire the spinlock and busy-wait until successful. | ||
| ; R1.b0 - Input : spinlock id (set to SPINLOCK_ID) | ||
| ; R1.b3 - Output: acquisition status (bit 0: 0=failed, 1=acquired) | ||
| ; Best case 3 cycles; worst case unbounded (busy-wait). | ||
| ;****************************************************************************** | ||
| M_SPINLOCK_ACQUIRE .macro | ||
| .newblock | ||
| LDI R1.b0, SPINLOCK_ID ; lock id (0-63) | ||
| $1: | ||
| XIN INT_SPIN_XID, &R1.b3, 1 ; attempt acquire; status in R1.b3 | ||
| QBBC $1, R1.b3, 0 ; bit 0 clear -> not acquired, retry | ||
| .endm | ||
|
|
||
| ;****************************************************************************** | ||
| ; Macro: M_SPINLOCK_RELEASE | ||
| ; Release the spinlock. Must follow M_SPINLOCK_ACQUIRE (lock id is still | ||
| ; in R1.b0). 2 cycles. | ||
| ;****************************************************************************** | ||
| M_SPINLOCK_RELEASE .macro | ||
| XOUT INT_SPIN_XID, &R1.b3, 1 ; release lock held in R1.b0 | ||
| .endm |
There was a problem hiding this comment.
4. Macros missing required docs 📘 Rule violation ⚙ Maintainability
The new PRU assembly macros M_SPINLOCK_ACQUIRE and M_SPINLOCK_RELEASE are documented but do not include all mandatory fields (pseudo code and registers modified). This reduces reviewability and can hide register-clobber and timing assumptions.
Agent Prompt
## Issue description
PRU assembly macro documentation is missing mandatory fields (pseudo code, registers modified).
## Issue Context
The compliance checklist requires PRU macros/functions to document pseudo code, peak/worst-case cycle budget, and registers modified.
## Fix Focus Areas
- academy/spinlock/spinlock_example.asm[29-51]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+49
to
+52
| MOV R1.b0, R14.b0 ; lock id -> R1.b0 (presented to accel) | ||
| XIN INT_SPIN_XID, &R1.b3, 1 ; status returned in R1.b3 (bit0=acquired) | ||
| MOV R14.b0, R1.b3 ; return status | ||
| JMP r3.w2 |
There was a problem hiding this comment.
5. Acquire return not boolean 🐞 Bug ≡ Correctness
spinlock_acquire() claims to return 0/1 but actually returns the raw R1.b3 status byte; main.c spins on result != 1, so it can keep spinning even when bit0 indicates the lock is acquired (if any other status bits are set). This makes the example’s acquire loop depend on an exact byte value instead of the documented “bit0=success” contract.
Agent Prompt
## Issue description
`spinlock_acquire()` is documented as returning `1` when acquired and `0` otherwise, but it currently returns the entire status byte from `R1.b3`. The C example (`main.c`) and README usage pattern compare the return value to `1`, so the loop is only correct if the hardware always returns exactly `0x00` or `0x01`.
## Issue Context
The README and the pure-assembly macro example both treat only **bit 0** as meaningful (they test the bit), not the entire byte.
## Fix Focus Areas
- academy/spinlock/spinlock.asm[48-52]
- academy/spinlock/main.c[58-63]
## Suggested fix
1. Normalize the return value in `spinlock_acquire()` to `0`/`1` by masking `R1.b3` to bit0 before moving into `R14.b0` (e.g., `AND` with `0x01`).
2. (Optional hardening) Update the C loop to test bit0 (`(spinlock_acquire(...) & 1) != 0`) so the usage pattern matches the documented semantics even if the helper is reused elsewhere.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Closes TexasInstruments#120. Demonstrates the PRU_ICSSG local hardware spinlock accessed over the broadside interface (XIN/XOUT), as recommended in the issue: - lock id (0-63) in R1.b0 - acquire: XIN 0x90, &R1.b3, 1 -> status in R1.b3 bit 0 (1 = acquired) - release: XOUT 0x90, &R1.b3, 1 (same id in R1.b0) Files: - spinlock.asm: C-callable spinlock_acquire()/spinlock_release(). - main.c: C example - PRU0/PRU1 contend for one lock, scope via debug GPO. - spinlock_example.asm: pure-asm example with M_SPINLOCK_ACQUIRE/RELEASE macros. - readme.md: usage, semantics, and build notes. All sources assemble/compile with clpru --silicon_version=3 (ti-cgt-pru 2.3.3); main.c builds for both PRU0 and PRU1. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
pratheesh
force-pushed
the
fix/issue-120-spinlock-examples
branch
from
3164531 to
f22b51c
Compare
|
Code review by qodo was updated up to the latest commit f22b51c |
Comment on lines
+8
to
+21
| The PRU_ICSSG includes a hardware spinlock accelerator with **64 ownership flags** | ||
| (lock id `0`–`63`). A PRU core reaches it over the **broadside interface** using the | ||
| `XIN`/`XOUT` instructions — there is no memory-mapped load/store on the critical path, | ||
| so acquire/release are only a couple of cycles. | ||
|
|
||
| - The broadside device id (XID) **`0x90` (= 144)** selects the spinlock in the | ||
| **local** PRU subsystem. A different XID is needed to reach a spinlock in another | ||
| PRU subsystem. | ||
| - The **lock id** to operate on is placed in **`R1.b0`**. | ||
| - **Acquire** (`XIN 0x90, &R1.b3, 1`): the accelerator returns the acquisition status | ||
| in **`R1.b3`** — **bit 0 = 1** means the lock is now owned by this core, **bit 0 = 0** | ||
| means it is held by someone else, so retry. A single `XIN` is one attempt; busy-wait | ||
| until you get a 1. | ||
| - **Release** (`XOUT 0x90, &R1.b3, 1`): with the same lock id still in `R1.b0`, frees |
There was a problem hiding this comment.
1. Busy-wait worst-case undocumented 📎 Requirement gap ☼ Reliability
The README describes spinning until acquisition succeeds but does not explicitly document that the busy-wait can be unbounded in the worst case, as required. This can mislead users about timing/determinism implications of the blocking acquire loop.
Agent Prompt
## Issue description
The documentation explains the status bit and that the PRU should busy-wait, but it does not explicitly state the worst-case acquisition time is unbounded.
## Issue Context
PR Compliance requires documenting busy-wait semantics including best/worst-case timing characteristics and how acquisition success is detected.
## Fix Focus Areas
- academy/spinlock/readme.md[8-22]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+35
to
+66
| ;****************************************************************************** | ||
| ; uint8_t spinlock_acquire(uint8_t flag_id); | ||
| ; | ||
| ; One non-blocking attempt to acquire spinlock 'flag_id'. | ||
| ; Returns 1 if the lock was acquired, 0 if it is held by someone else. | ||
| ; | ||
| ; Calling convention (PRU C/C++ compiler): the uint8_t argument arrives in | ||
| ; R14.b0 and the uint8_t return value is passed back in R14.b0. The return | ||
| ; address is in r3.w2. | ||
| ;****************************************************************************** | ||
| .sect ".text:spinlock_acquire" | ||
| .clink | ||
| .global spinlock_acquire | ||
| spinlock_acquire: | ||
| MOV R1.b0, R14.b0 ; lock id -> R1.b0 (presented to accel) | ||
| XIN INT_SPIN_XID, &R1.b3, 1 ; status returned in R1.b3 (bit0=acquired) | ||
| MOV R14.b0, R1.b3 ; return status | ||
| JMP r3.w2 | ||
|
|
||
| ;****************************************************************************** | ||
| ; void spinlock_release(uint8_t flag_id); | ||
| ; | ||
| ; Release spinlock 'flag_id'. Only the owner should release, and it must do | ||
| ; so promptly. | ||
| ;****************************************************************************** | ||
| .sect ".text:spinlock_release" | ||
| .clink | ||
| .global spinlock_release | ||
| spinlock_release: | ||
| MOV R1.b0, R14.b0 ; lock id -> R1.b0 | ||
| XOUT INT_SPIN_XID, &R1.b3, 1 ; release the lock | ||
| JMP r3.w2 |
There was a problem hiding this comment.
2. spinlock.asm missing required metadata 📘 Rule violation ⚙ Maintainability
The new assembly functions spinlock_acquire/spinlock_release are added without the mandatory function metadata (pseudo code, peak/worst-case cycles, and registers modified). This reduces reviewability and can hide timing/register-clobber assumptions in a low-level synchronization primitive.
Agent Prompt
## Issue description
The assembly functions added in `spinlock.asm` do not include the required metadata fields: pseudo code, peak/worst-case cycle count, and registers modified.
## Issue Context
The compliance checklist requires standardized documentation for PRU assembly functions/macros to prevent timing and register-corruption bugs.
## Fix Focus Areas
- academy/spinlock/spinlock.asm[35-66]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Add the required PRU assembly function metadata (pseudocode, registers modified, peak cycle count) to spinlock_acquire and spinlock_release, matching the repo convention (e.g. multicore_scheduler pwm_toggle.asm). Documents timing and register-clobber assumptions for the low-level synchronization primitive. Comments only; assembles with clpru 2.3.3. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
Added the required PRU assembly function metadata to |
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #120.
Adds HW spinlock usage examples (assembly and C) under
academy/spinlock/.Approach
Uses the PRU_ICSSG local hardware spinlock accessed over the broadside interface (
XIN/XOUT), per the recommendation in #120. The spinlock accelerator owns 64 ownership flags (lock id 0–63), reached with broadside device id0x90(= 144) for the local PRU subsystem:R1.b0.XIN 0x90, &R1.b3, 1→ acquisition status inR1.b3bit 0 (1= acquired). OneXINis one attempt; busy-wait until acquired.XOUT 0x90, &R1.b3, 1with the same lock id still inR1.b0.A spinlock provides mutual exclusion only (not a mailbox); the README covers the correct signalling pattern (guard a shared buffer + raise a system event).
Files
spinlock.asm— C-callablespinlock_acquire()/spinlock_release()built on the broadside primitives.main.c— C example: PRU0 and PRU1 contend for one lock and drive a per-core debug GPO while holding it, so ownership is observable on a scope.spinlock_example.asm— pure-assembly example usingM_SPINLOCK_ACQUIRE/M_SPINLOCK_RELEASEmacros.readme.md— semantics, usage pattern, and build notes.Validation
All sources assemble/compile with
clpru --silicon_version=3(ti-cgt-pru 2.3.3);main.cbuilds for both PRU0 (-DPRU0) and PRU1.Notes
Squashed to a single commit. An earlier revision of this PR used the SoC-level memory-mapped Spinlock module (
0x2A000000); it has been reworked to the broadside local-spinlock approach requested in the issue.