Repository navigation
Conversation
Signed-off-by: Ayushman a-ayushman@ti.com
PR Summary by QodoAdd mirror_input + spi_10mhz AM261x examples; tighten Flow Control labels and SPI timing
AI Description
Diagram
High-Level Assessment
Files changed (92)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
461a2a3 to
a8bf168
Compare
2607cc5 to
bb486a3
Compare
signed off by: Ayushman <a-ayushman@ti.com>
bb486a3 to
a69f359
Compare
pratheesh
left a comment
pratheesh
left a comment
There was a problem hiding this comment.
Thanks for this. The Flow Control jump-target work is a good addition. I generated and tested several designs against this head (a69f359) and found some problems I think should be fixed before merge. They're listed most severe first.
How I checked: SysConfig 1.28.0 CLI against the PR-head .metadata/product.json (AM261x_ZFG, icss_m1_pru0), with the generated asm assembled using clpru 2.3.3. I checked loop behaviour on the TI pru-simulator (cycle-accurate hardware-LOOP model) but have not tested it on silicon.
1. <Loop>_end "break" behaves like continue on finite loops
Loop_0_end: is emitted at the same address as endloop_0. A JMP to the LOOP end address triggers the hardware loop-back. I simulated the exact generated asm with a 5-count loop whose branch jumps to Loop_0_end. It still ran the body 5 times and then halted, where a real break would run it once. Infinite loops are fine, because their _end comes after a QBA and no hardware loop is involved.
Suggested fix: place the break target at an address other than endloop_N (for example one instruction past a trampoline), then confirm on silicon.
2. <Loop>_start re-arms the LOOP, so the documented "continue" pattern never terminates
The label sits between LDI and LOOP. The code comment says a jump there "resumes with the current counter value", but that isn't what happens: re-executing LOOP reloads the count from the register. The conditional_block/flow_control docs tell users to end both branches with JMP Loop_0_start. I simulated that pattern with GPI high and got 714 body entries in 5000 steps with no halt. Also, if the counter register gets reallocated inside the body, the count would be garbage.
3. The dropdown offers targets that are never emitted, and validation accepts them
getValidJumpTargets() lists _start/_end for every /pru_blocks/ instance. But addToPruRegisterAllocationSummary drops 0-cycle entries for blocks with an empty opCode (Memory Variable, LUT). A design with jumpTarget = "Mem_Var_start" passes SysConfig validation, and then clpru fails with E0300 undefined symbol Mem_Var_start. The docs say validation prevents exactly this failure. Unconnected blocks, which pushInstruction never visits, are probably affected the same way.
4. Breaking change: legacy Flow Control designs no longer load
opCode is now a getter-only hidden config. On this head, main's examples/empty/.../example.syscfg (which sets opCode = "HALT") fails with Error: cannot set 'opCode' to 'HALT'. The repo examples were migrated by deleting that line, which also changes HALT into JMP sysconfig_generated_end plus a label in main.asm. User designs have no migration path. Could the old setter be accepted and mapped, or could a migration note be added?
5. SPI Data Setup Time units change silently from cycles to ns
Existing designs will get a much shorter setup time: a value of 10 used to mean 10 cycles and now means 10 ns, which is 2 cycles at 200 MHz. CS Setup/Hold were labelled ns but were previously passed through as raw cycles. This PR fixes that bug, but it also changes behaviour (500 used to mean 2.5 µs and now means 500 ns). This needs a release note or migration.
6. The spi_10mhz example contradicts its README
- The syscfg has SCLK high=16, low=6, but the README says 14/6.
- PRU0 (the controller) leaves
pruClkFreqat its default of 200 while PRU1 sets 225. That gives SCLK = 9.09 MHz at 200 or 10.23 MHz at 225, never 10 MHz. - The caption says "1 bit = 10ns" (should be 100 ns).
- The README block table now says "SPI Transfer up to 7.69 MHz", which contradicts this 10 MHz SPI Transfer example.
7. If_Else_N_start comes after the input computations
The docs say "jump to an If/Else's _start to re-evaluate it". A jump there compares stale registers that may already have been reallocated, and it does not re-read the inputs.
8. branchTerminates ORs paths (from reading the code; not tested)
It accepts a branch if any next/data-consumer path reaches Flow Control, and it treats data-flow edges as control flow. The docstring says every path must reach it.
9. Small defects
- uart_tx_op: the input port displayName is
"output1"(copy-paste error). - spi_write docs still say MODE0 needs a minimum High of 2 for 25 MHz, but validation now allows High=1 (7 cycles, 28.57 MHz).
- mirror_input readme: the title is "SPI Transfer at 10MHz", it says
Loop_0_startwhere the config usesstartloop_0, and it references a pin table that doesn't exist. - The mirror R5F syscfg still has leftover SPI pinmux entries (PRU1 GPIO5/6/11, PRU0 GPIO2/7/9).
- The mirror main.c has dead AM64x/AM243x RTU/TX loads in an AM261x-only example.
10. Scope
The title says "mirror input example", but the PR also includes:
- the Flow Control/label redesign
- the SPI timing rework
- an AM261x CI SDK bump from 10.02.00.15 to 26.00.00.06.STS (mcelf boot images and
freertos.*libs, which breaks older SDKs; the README doesn't state the new minimum SDK) - new ZNC and ZFG_400 device variants
Splitting these into separate PRs would make review and bisecting much easier. The PR description is also empty.
Note: CI is green, but that only shows the shipped examples compile. None of them uses a finite-loop jump or a declaration-block jump target, which is where items 1 to 3 appear. The qodo bot also flagged #4. Its claim that the repo examples still set opCode is out of date at this head.
| //push <LoopName>_start label between LDI and LOOP so a Flow Control | ||
| //jump back here resumes with the current counter value instead of | ||
| //resetting it (jumping to before the LDI would reset the count) | ||
| addToPruRegisterAllocationSummary(`${instanceName}_start`, "0", instance, instanceName, 0); |
There was a problem hiding this comment.
Re-executing LOOP reloads the count from the register, so a jump to <Loop>_start re-arms the loop instead of resuming with the current count. The comment above is incorrect, and the documented JMP Loop_0_start continue pattern never terminates in simulation (see item 2 in the review summary).
There was a problem hiding this comment.
fixed the documentation for this
JMP to loop start will act as continue only for infinite loops
| displayName: `${inst.$name} (loop start)` | ||
| }); | ||
| } | ||
| // <LoopName>_end: exposed early-exit target, distinct from |
There was a problem hiding this comment.
For finite loops, <Loop>_end resolves to the same address as endloop_N. A JMP there triggers the hardware loop-back, so it acts as continue, not break. In simulation a 5-count loop still ran 5 iterations (item 1).
There was a problem hiding this comment.
fixed: loop wont emit end label
for breaking out of the loop, we jump to the next block's start (connected next to loop)
| * targets, not block entry/exit points). | ||
| * @returns {{name: string, displayName: string}[]} | ||
| */ | ||
| function getValidJumpTargets() { |
There was a problem hiding this comment.
This lists _start/_end for every /pru_blocks/ instance, but blocks with an empty opCode (Memory Variable, LUT) never emit the label. jumpTarget = "Mem_Var_start" passes validation and then fails in clpru with E0300 undefined symbol (item 3).
There was a problem hiding this comment.
fixed validation for this
| * branches must recursively terminate the same way. | ||
| * Returns an error message, or null if the branch terminates correctly. | ||
| */ | ||
| function branchTerminates(headInst, ownerName, branchLabel) { |
There was a problem hiding this comment.
From reading the code: walk accepts the branch if any reachable next/data-consumer path hits Flow Control, and it treats output1 data edges as control flow. The docstring says every path must terminate (item 8).
| ports: (inst) => { | ||
| const dataBits = inst.dataBits; | ||
| const inputType = dataBits > 32 ? "input64" : "input32"; | ||
| const portName = inputType === "input64" ? "in64" : "output1" |
There was a problem hiding this comment.
The input port displayName falls back to "output1". Looks like a copy-paste from uart_rx_op; should this be "input1"/"in32"?
a1248924
changed the title
2 participants
No description provided.