[BUG] SPI Engine sleep instructruction time is affected by word size
dlech opened this issue · 4 comments
Describe the bug
I have code that generates the following SPI Engine instructions:
[ 45.198984] ad4695 spi0.0: CMD: 0x2103 CPHA=1, CPOL=1
[ 45.199002] ad4695 spi0.0: CMD: 0x10fe assert CS0
[ 45.199014] ad4695 spi0.0: CMD: 0x2007 prescalar=8 (80 MHz clk / 8 = 10 MHz)
[ 45.199025] ad4695 spi0.0: CMD: 0x2208 bits_per_word=8
[ 45.199035] ad4695 spi0.0: CMD: 0x0100 write xfer
[ 45.199045] ad4695 spi0.0: CMD: 0x10ff deassert CS
[ 45.199055] ad4695 spi0.0: CMD: 0x3104 sleep 4 ticks
[ 45.199065] ad4695 spi0.0: CMD: 0x10fe assert CS0
[ 45.199075] ad4695 spi0.0: CMD: 0x2210 bits_per_word=16
[ 45.199085] ad4695 spi0.0: CMD: 0x0100 write xfer
[ 45.199095] ad4695 spi0.0: CMD: 0x10ff deassert CS
[ 45.199104] ad4695 spi0.0: CMD: 0x3104 sleep 4 ticks
[ 45.199114] ad4695 spi0.0: CMD: 0x10fe assert CS0
[ 45.199124] ad4695 spi0.0: CMD: 0x0200 read xfer
[ 45.199134] ad4695 spi0.0: CMD: 0x10ff deassert CS
[ 45.199144] ad4695 spi0.0: CMD: 0x2000 prescalar=0
[ 45.199153] ad4695 spi0.0: CMD: 0x3001 sync=1
According to the formula from http://analogdevicesinc.github.io/hdl/library/spi_engine/instruction-format.html
The two sleeps should be 2 + 4 * ((7 + 1) * 2) / 160 MHz = 412.5 ns
When measuring with a logic analyzer, the first delay looks correct (428 ns is approx. 412.5 ns sleep + 12.5 ns for the next chip assert instruction), but the second delay is much shorter (276 ns).
Expected behavior
The sleep time should always be the same no mater what the bits_per_word is.
Desktop (please complete the following information):
- Project name and used carrier board:
ad469x_fmc/zedEVAL-AD4696FMCZ + ZedBoard - Used Software: Linux
- Tool version: Vivado/2023.2
- HDL Release version: commit 3743439
- Software Release version: N/A
The key thing that appears to trigger the bug is setting 0x2210 bits_per_word=16 between the two sleep instructions. If we remove this and leave everything else the same, we don't see the bug.
I am a bit suspicious of this line:
The counter is shared by the sleep timer so it seems like this could affect the count based on the word size during a sleep instruction.
I "fixed" the bug with this hack. Clearly not the right way ™️ to fix it, but I don't really know what I'm doing when it comes to HDL. 😄
diff --git a/library/spi_engine/spi_engine_execution/spi_engine_execution.v b/library/spi_engine/spi_engine_execution/spi_engine_execution.v
index d046d4dfd..503239fac 100644
--- a/library/spi_engine/spi_engine_execution/spi_engine_execution.v
+++ b/library/spi_engine/spi_engine_execution/spi_engine_execution.v
@@ -268,7 +268,7 @@ module spi_engine_execution #(
if (idle == 1'b1 || (cs_sleep_counter_compare && !cs_sleep_repeat && inst_d1 == CMD_CHIPSELECT)) begin
counter <= 'h00;
end else if (clk_div_last == 1'b1 && wait_for_io == 1'b0) begin
- if (bit_counter == word_length) begin
+ if (bit_counter == word_length && inst_d1 != CMD_MISC) begin
counter <= (counter & BIT_COUNTER_CLEAR) + (transfer_active ? 'h1 : (2**BIT_COUNTER_WIDTH)) + BIT_COUNTER_CARRY;
end else begin
counter <= counter + (transfer_active ? 'h1 : (2**BIT_COUNTER_WIDTH));I used CMD_MISC because the sleep command is one of the subcommands of MISC. But this if branch should probably be disabled at other times as well. There aren't any comments in the code though, so it is hard to tell what the original intention was.
Hi David, can you also test it with this branch?
https://github.com/analogdevicesinc/hdl/tree/spi_sleep_fix
I tested it and it fixes the sleep timing issue. 👍
And it doesn't appear to break data transfers either.