94xhn opened a new pull request, #19415:
URL: https://github.com/apache/nuttx/pull/19415
drivers/spi/ice40: fix operator precedence in final clock cycle count
## Summary
* Why change is necessary (fix, update, new feature)? Bug fix: an
operator-precedence error causes `ice40_endwrite()` to clock out 8x more
finalization SPI bytes than intended.
* What functional part of the code is being changed? `ice40_endwrite()` in
`drivers/spi/ice40.c`, the loop that clocks out dummy bytes after the FPGA
bitstream to finish iCE40 configuration.
* How does the change exactly work (what will change and how)?
`ICE40_SPI_FINAL_CLK_CYCLES + 7 / 8` is parenthesized to
`(ICE40_SPI_FINAL_CLK_CYCLES + 7) / 8`.
In C, `/` binds tighter than `+`, so the unmodified expression parses as
`ICE40_SPI_FINAL_CLK_CYCLES + (7 / 8)` = `160 + 0` = `160`; the `+ 7 / 8` part
is a silent no-op. `ICE40_SPI_FINAL_CLK_CYCLES` is defined as `160` in
`include/nuttx/spi/ice40.h:54`. Each `SPI_SEND()` call sends one byte = 8 SPI
clock cycles (word size is configured to 8 bits via `SPI_SETBITS(spi, 8)` in
`ice40_configspi()`), so the unmodified loop runs 160 times and clocks out 160
× 8 = 1280 SPI clock cycles.
The macro name (`_CLK_CYCLES`, not `_BYTES`) together with the classic
`(n + 7) / 8` ceiling-division idiom used elsewhere in embedded code to convert
a bit/cycle count into a byte count make it clear the intent was `bytes_to_send
= ceil(ICE40_SPI_FINAL_CLK_CYCLES / 8) = (160 + 7) / 8 = 20` bytes, i.e.
exactly 160 clock cycles, matching the macro name. Adding the missing
parentheses restores that intended value (20 bytes / 160 cycles).
* Related [NuttX Issue](https://github.com/apache/nuttx/issues/19367)
reference if applicable. Fixes #19367, reported by @Zepp-Hanzj on 2026-07-08. I
independently verified the analysis in the issue against the current `master`
source before implementing this fix.
## Impact
* Is new feature added? Is existing feature changed? NO new feature.
Existing behavior of `ice40_endwrite()`'s configuration-finalization step
changes: it now clocks out 160 SPI clock cycles instead of 1280.
* Impact on user (will user need to adapt to change)? NO. Per Lattice's
iCE40 Programming and Configuration technical note (FPGA-TN-02001), only >= 49
extra `SPI_SCK` clock cycles after `CDONE` goes high are required to complete
configuration. Both the buggy (1280) and fixed (160) cycle counts are well
above that minimum, so this is not expected to change observable behavior on
hardware — it corrects a value that visibly contradicts both the macro's name
and the intent documented by the code shape, without being a functional
regression risk.
* Impact on build (will build process change)? NO.
* Impact on hardware (will arch(s) / board(s) / driver(s) change)? YES,
`drivers/spi/ice40.c` (iCE40 FPGA bitstream-loading SPI driver, used e.g. by
`boards/risc-v/esp32c3-legacy` iCE40 board support). Only the loop trip count
in the finalization step changes; no other logic, no ABI/API/Kconfig change.
* Impact on documentation (is update required / provided)? NO, no
user-visible behavior/documentation to update.
* Impact on security (any sort of implications)? NO.
* Impact on compatibility (backward/forward/interoperability)? NO.
* Anything else to consider or add? See "Testing" section below for an
important disclosure: I do not have access to real iCE-V-Wireless/iCE40
hardware, so this could not be verified on a physical target. Verification was
done with an isolated, faithful reproduction of the arithmetic/loop logic only.
## Testing
I do **not** have access to real iCE40/iCE-V-Wireless hardware, so I could
**not** produce a real-hardware build+runtime log for this change, which I
understand is normally expected for driver changes. Given that, here is exactly
what I *did* verify, so maintainers can judge whether it's sufficient or
whether hardware confirmation from someone with the board is needed before
merge:
* Build Host(s): Windows 11, gcc (MinGW-w64) via Git Bash, `gcc -std=c11
-Wall -Wextra -Werror`.
* Target(s): N/A — no physical iCE40 board available. Instead of a NuttX
board build, I wrote a standalone, non-NuttX host program that copies the exact
unmodified loop expression from `ice40_endwrite()` (before and after the fix),
the exact macro definition from `include/nuttx/spi/ice40.h`, and a mock
`SPI_SEND()` that counts bytes/clock cycles the same way the real one would (1
byte = 8 clock cycles, per `SPI_SETBITS(spi, 8)` in `ice40_configspi()`). This
isolates and proves the arithmetic/loop-trip-count discrepancy without needing
real SPI hardware, but it is **not** a substitute for an end-to-end hardware
test of `ice40_endwrite()` itself.
* I also confirmed the diff is a single line, that the changed line is
well under the 80-column limit, and that this is the only place
`ICE40_SPI_FINAL_CLK_CYCLES` is used in the tree (`grep -r
ICE40_SPI_FINAL_CLK_CYCLES`), so the fix cannot affect any other code path.
Reproduction steps (repository-independent, does not require NuttX build
system):
```
$ gcc -std=c11 -Wall -Wextra -Werror -o repro repro_ice40_clk_cycles.c
$ ./repro
```
Testing logs (this is the *reproduction program's* output, not a NuttX
board build/run log):
```
ICE40_SPI_FINAL_CLK_CYCLES = 160
Expression evaluation (matches C operator precedence):
buggy : ICE40_SPI_FINAL_CLK_CYCLES + 7 / 8 = 160 (parses as 160 +
(7/8) = 160 + 0)
fixed : (ICE40_SPI_FINAL_CLK_CYCLES + 7) / 8 = 20 (parses as (160+7)/8 =
167/8)
Simulated ice40_endwrite() finalization loop:
BEFORE FIX : 160 SPI_SEND() calls -> 1280 SPI clock cycles (intended:
160)
AFTER FIX : 20 SPI_SEND() calls -> 160 SPI clock cycles (intended: 160)
PASS: confirmed 8x discrepancy (160 vs 20 bytes sent) caused by C operator
precedence in the unmodified driver code, and confirmed the parenthesized
ceiling-division fix produces exactly ICE40_SPI_FINAL_CLK_CYCLES (160) clock
cycles as the macro name promises.
```
(exit code 0, compiled with `-Werror` and no warnings.)
<details>
<summary>Full source of the reproduction program (click to
expand)</summary>
```c
/*
* repro_ice40_clk_cycles.c
*
* Standalone reproduction for apache/nuttx issue #19367 /
drivers/spi/ice40.c
* operator-precedence bug in ice40_endwrite().
*
* Real (unmodified) source under test, apache/nuttx @ master,
* drivers/spi/ice40.c line 272:
*
* for (size_t i = 0; i < ICE40_SPI_FINAL_CLK_CYCLES + 7 / 8; i++)
* {
* SPI_SEND(dev->spi, 0xff);
* }
*
* ICE40_SPI_FINAL_CLK_CYCLES is defined in include/nuttx/spi/ice40.h:54
as:
*
* #define ICE40_SPI_FINAL_CLK_CYCLES 160
*
* Each call to SPI_SEND() transmits exactly one byte == 8 SPI clock cycles
* (this is how every other loop in the same file computes byte counts from
* a clock/bit budget, e.g. ice40_write() and the ICE_SPI_MAX_XFER chunking
* just above ice40_endwrite() in the same file operate on byte units).
*
* Build: gcc -std=c11 -Wall -Wextra -o repro repro_ice40_clk_cycles.c
* Run: ./repro
*/
#include <stdio.h>
#include <stddef.h>
#include <assert.h>
/* Verbatim from include/nuttx/spi/ice40.h:54 */
#define ICE40_SPI_FINAL_CLK_CYCLES 160
static long g_spi_send_calls;
static long g_spi_clock_cycles; /* 8 clock cycles per SPI_SEND() byte */
/* Mock of SPI_SEND(dev->spi, 0xff) from include/nuttx/spi/spi.h.
* Each invocation clocks out exactly one byte == 8 SPI clock cycles.
*/
static void mock_spi_send(unsigned char byte)
{
(void)byte;
g_spi_send_calls++;
g_spi_clock_cycles += 8;
}
/* Faithful copy of the loop body in ice40_endwrite(), drivers/spi/ice40.c,
* BEFORE the fix (line 272 verbatim, only SPI_SEND() replaced by the
mock).
*/
static void ice40_endwrite_loop_buggy(void)
{
for (size_t i = 0; i < ICE40_SPI_FINAL_CLK_CYCLES + 7 / 8; i++)
{
mock_spi_send(0xff);
}
}
/* Same loop AFTER the fix: parenthesize the ceiling-division. */
static void ice40_endwrite_loop_fixed(void)
{
for (size_t i = 0; i < (ICE40_SPI_FINAL_CLK_CYCLES + 7) / 8; i++)
{
mock_spi_send(0xff);
}
}
int main(void)
{
int status = 0;
size_t buggy_bound = ICE40_SPI_FINAL_CLK_CYCLES + 7 / 8;
size_t fixed_bound = (ICE40_SPI_FINAL_CLK_CYCLES + 7) / 8;
printf("ICE40_SPI_FINAL_CLK_CYCLES = %d\n\n",
ICE40_SPI_FINAL_CLK_CYCLES);
printf("Expression evaluation (matches C operator precedence):\n");
printf(" buggy : ICE40_SPI_FINAL_CLK_CYCLES + 7 / 8 = %lu "
"(parses as %d + (7/8) = %d + 0)\n",
(unsigned long)buggy_bound, ICE40_SPI_FINAL_CLK_CYCLES,
ICE40_SPI_FINAL_CLK_CYCLES);
printf(" fixed : (ICE40_SPI_FINAL_CLK_CYCLES + 7) / 8 = %lu "
"(parses as (%d+7)/8 = 167/8)\n\n",
(unsigned long)fixed_bound, ICE40_SPI_FINAL_CLK_CYCLES);
assert(buggy_bound == 160 && "unexpected buggy bound (repro invalid)");
assert(fixed_bound == 20 && "unexpected fixed bound (repro invalid)");
g_spi_send_calls = 0;
g_spi_clock_cycles = 0;
ice40_endwrite_loop_buggy();
long buggy_calls = g_spi_send_calls;
long buggy_cycles = g_spi_clock_cycles;
g_spi_send_calls = 0;
g_spi_clock_cycles = 0;
ice40_endwrite_loop_fixed();
long fixed_calls = g_spi_send_calls;
long fixed_cycles = g_spi_clock_cycles;
printf("Simulated ice40_endwrite() finalization loop:\n");
printf(" BEFORE FIX : %ld SPI_SEND() calls -> %ld SPI clock cycles"
" (intended: %d)\n",
buggy_calls, buggy_cycles, ICE40_SPI_FINAL_CLK_CYCLES);
printf(" AFTER FIX : %ld SPI_SEND() calls -> %ld SPI clock cycles"
" (intended: %d)\n\n",
fixed_calls, fixed_cycles, ICE40_SPI_FINAL_CLK_CYCLES);
if (buggy_calls != 160 || buggy_cycles != 1280)
{
printf("FAIL: expected the unmodified driver expression to produce "
"160 SPI_SEND calls / 1280 cycles\n");
status = 1;
}
if (fixed_calls != 20 || fixed_cycles != 160)
{
printf("FAIL: expected the fixed expression to produce 20 SPI_SEND "
"calls / 160 cycles (== ICE40_SPI_FINAL_CLK_CYCLES)\n");
status = 1;
}
if (status == 0)
{
printf("PASS: confirmed 8x discrepancy (160 vs 20 bytes sent) caused
"
"by C operator precedence in the unmodified driver code, and "
"confirmed the parenthesized ceiling-division fix produces "
"exactly ICE40_SPI_FINAL_CLK_CYCLES (160) clock cycles as "
"the macro name promises.\n");
}
return status;
}
```
</details>
I was not able to cross-compile or run `tools/nxstyle` cleanly in my
environment against this file (it reports a path-resolution error on
Windows/Git-Bash that also occurs on unmodified files, e.g.
`drivers/spi/spi.c`, so it is an environment/tooling limitation unrelated to
this change, not a style issue introduced by it). The changed line is 67
columns, within the 80-column limit, and matches surrounding indentation/style.
## PR verification Self-Check
* [x] This PR introduces only one functional change.
* [x] I have updated all required description fields above.
* [x] My PR adheres to Contributing
[Guidelines](https://github.com/apache/nuttx/blob/master/CONTRIBUTING.md) and
[Documentation](https://nuttx.apache.org/docs/latest/contributing/index.html)
(git commit title and message, coding standard, etc).
* [ ] My PR is still work in progress (not ready for review).
* [x] My PR is ready for review and can be safely merged into a codebase.
## Disclosure
I used Claude (Anthropic, model `claude-sonnet-5`, via Claude Code) to help
investigate this issue, write the fix, write the reproduction program, and
draft this PR description. I (the human author, credited in the `Signed-off-by`
trailer) reviewed the diff, the reasoning, and the reproduction output before
submitting. Per this repository's `CONTRIBUTING.md` §1.5, the commit carries an
`Assisted-by: Claude Code:claude-sonnet-5` trailer. I do not have physical
iCE40/iCE-V-Wireless hardware, so — as stated above in Testing — this fix has
not been verified on real hardware, only via an isolated logic reproduction;
I'm flagging this explicitly rather than implying hardware verification took
place.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
