library/spi_engine: SDO Extension upgrade #1808

caosjr · 2025-07-01T18:54:48Z

PR Description

Upgrade SPI engine to support up to 8 lanes. For that, two new registers were inserted into Configuration Write Instruction, they are responsible for setting the SDI and SDO lane masks. Each bit represents a lane.
This PR also removes register 8'h3b from the SPI Engine regmap.
The testbench for this modification is here: analogdevicesinc/testbenches#240

PR Type

Bug fix (change that fixes an issue)
New feature (change that adds new functionality)
Breaking change (has dependencies in other repos or will cause CI to fail)
Documentation

PR Checklist

I have followed the code style guidelines
I have performed a self-review of changes
I have compiled all hdl projects and libraries affected by this PR
I have tested in hardware affected projects, at least on relevant boards
I have commented my code, at least hard-to-understand parts
I have signed off all commits from this PR
I have updated the documentation (wiki pages, ReadMe files, Copyright etc)
I have not introduced new Warnings/Critical Warnings on compilation
I have added new hdl testbenches or updated existing ones

LBFFilho

Overall this seems headed in a nice direction, and thanks for also doing a lot of small fixes and improvements to the existing code along the way.

About the version: is this going to be a minor version or a major version bump? I understand that the removed register was not used anywhere (it was broken even), but technically we're breaking anything that relied on it. Also changing the behavior of the SDI & SDO FIFOs.

Also, please check timing on ad4052/de10nano just to be sure if it's all good.

docs/library/spi_engine/spi_engine_interconnect.rst

docs/regmap/adi_regmap_spi_engine.txt

docs/library/spi_engine/spi_engine_interconnect.rst

library/spi_engine/axi_spi_engine/axi_spi_engine.v

library/spi_engine/spi_engine_execution/spi_engine_execution.v

LBFFilho · 2025-07-02T14:34:35Z

library/spi_engine/spi_engine_execution/spi_engine_execution.v

@@ -69,7 +70,7 @@ module spi_engine_execution #(

  input echo_sclk,
  output reg sclk,
-  output reg sdo,
+  output reg [NUM_OF_SDI-1:0] sdo,


Since now this applies to both sdi and sdo, please rename it to reflect the change.

Similar to my other comment, does it make sense to always have NUM_OF_SDI and NUM_OF_SDO the same? There are chips that have multiple SDO lines (so multiple SPI on the SPI Engine) but only one SDI line (so only one SDO line on the SPI Engine).

I talked to Sergiu and he agreed that having a single parameter for the number of lanes of SDI/SDO is better. The number of active lanes for SDI/SDO could be controlled on the software side by changing its respective lane mask, I am implementing a new register to have a separate mask for the SDI and SDO.

library/spi_engine/spi_engine_execution/spi_engine_execution_shiftreg.v

library/spi_engine/spi_engine_execution/spi_engine_execution_shiftreg_data_assemble.v

docs/library/spi_engine/instruction-format.rst

docs/library/spi_engine/axi_spi_engine.rst

docs/library/spi_engine/instruction-format.rst

dlech · 2025-07-14T20:39:23Z

library/spi_engine/spi_engine_execution/spi_engine_execution.v

@@ -69,7 +70,7 @@ module spi_engine_execution #(

  input echo_sclk,
  output reg sclk,
-  output reg sdo,
+  output reg [NUM_OF_SDI-1:0] sdo,


Similar to my other comment, does it make sense to always have NUM_OF_SDI and NUM_OF_SDO the same? There are chips that have multiple SDO lines (so multiple SPI on the SPI Engine) but only one SDI line (so only one SDO line on the SPI Engine).

* Extend SDO support to 8 (symmetrical with SDI support); * Update SDI to use asymmetrical FIFO; * Insert symmetrical FIFO for the SDO; * Insert SDI lane mask configuration instruction to reg 3'b011; * Insert SDO lane mask configuration instruction to reg 3'b100; * Insert offload active interface for interconnect and execution; * Remove register 8'h3b from spi engine regmap; * Improvements on the critical paths of the execution module. Prefetching on offload work iff all lanes are active. Signed-off-by: Carlos Souza <[email protected]>

* Update documentation to include the changes done for supporting more than one SDO lane. * Update the register map; * Insert SDI/SDO lane mask registers; * Update Configuration Write Instruction; * Update parameter from NUM_OF_SDI to NUM_OF_SDIO. Signed-off-by: Carlos Souza <[email protected]>

There is a different register for SDI lane mask and SDO lane mask; Update the NUM_OF_SDI parameter to NUM_OF_SDIO parameter to reflect that both SDI and SDO are symmetrical. The control over them is through the SDI/SDO lane masks. Inserts a ready signal for when the valid_indices has finished its inner logic. This avoid the possible issue where the latency of the command is smaller than this logic. This could only happen in the FIFO mode. Signed-off-by: Carlos Souza <[email protected]>

Update NUM_OF_SDI to NUM_OF_SDIO in the projects documentations. The variable was updated on the spi_engine library since SDI and SDO have symmetrical sizes. Signed-off-by: Carlos Souza <[email protected]>

Updated the projects to use NUM_OF_SDIO parameter. It is the same project, no functionality has changed. Signed-off-by: Carlos Souza <[email protected]>

dlech

I tried testing this with the ad738x_fmc/zed project. It worked with the default NUM_OF_SDI=1 but trying to use FIFO mode fails waiting for the SYNC command when I build with NUM_OF_SDI=4. I might need some help troubleshooting the HDL for that.

dlech · 2025-08-14T00:31:06Z

docs/library/spi_engine/instruction-format.rst

+
+.. _spi_engine sdi-lane-mask-register:
+
+SDI Lane Mask Register


Is this implemented yet? It looks like only SDO Lane mask is implemented so far.

I have tested the ad738x_fmc with these modifications on my testbench and it is working fine. I just need to set the SDO_LANE_MASK to 8'b1. The testbench is here: https://github.com/analogdevicesinc/testbenches/tree/spi_n_lanes. Could you take a look on it? I hope it helps you out.

The timeout from writing to this register (0x3) is fixed with the latest update.

dlech · 2025-08-14T00:34:10Z

library/spi_engine/spi_engine_execution/spi_engine_execution.v

+      clk_div        <= DEFAULT_CLK_DIV;
+      word_length    <= DATA_WIDTH;
+      left_aligned   <= 0;
+      sdo_lane_mask  <= ALL_ACTIVE_LANE_MASK;


I don't think we are going to be able to make this work without it being a breaking change. Setting the default sdo_lane_mask to ALL_ACTIVE_LANE_MASK basically breaks all FIFO mode transfers for existing users since before FIFO mode only ever used one lane. But setting it to 1 would break existing users that used multiple lanes with the offload. If we call this v2.0, then the old drivers will fail to load with a useful error message and the updated driver can be made to always program the number of lanes so that we don't have to set a default.

I had this discussion with Laez (@LBFFilho) and Sergiu, it was still on debate if that would break or not. ALL_ACTIVE_LANE_MASK is ((2^NUM_OF_SDIO) - 1). That would only be a problem for situations where the number of active lanes in the beginning is different from the NUM_OF_SDIO. @sarpadi what do you think about that?

library/spi_engine/axi_spi_engine/axi_spi_engine.v

dlech · 2025-08-14T23:28:47Z

docs/library/spi_engine/instruction-format.rst

+SDO Lane Mask Register
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+This register configures the SDO mask that defines which lanes are active


Is SDO from the point of view of the SPI controller or from the peripheral?

It is from the SPI controller view.

This doesn't seem to match how it is implemented. I was testing with AD7386-6 which has 4 SDO lines on the ADC. so from the controller point of view, I would be expecting to enable 4 SDI lines on the controller.

In the driver, I am doing a write command to register 0x4 rather than 0x3 to enable the multiple SDI (controller)/SDO (peripheral) lanes at the same time.

In a previous commit those addresses were reversed, that is what may have happened. This has been fixed for a while, could you update your branch to the latest commit, please?

dlech · 2025-08-14T23:40:13Z

I rebuilt the ad738x_fmc/zed project with NUM_OF_SDIO=4 again today (with a change to bump the version to 2.0.0) and changed the driver init to not try to use the unimplemented SDI mask register and made it a bit farther (didn't hit the timeout waiting for SYNC).

But the DMA buffer isn't getting filled correctly. I'm guessing this has to do with the SDI mask not being implemented yet? I though maybe the SDO mask was for reading from an ADC, but maybe you are just using it for a DAC so far?

dlech · 2025-08-15T00:29:02Z

But the DMA buffer isn't getting filled correctly.

I spoke a bit too soon. I found a mistake in my code and fixed it and got a bit farther. But now I'm seeing some occasional weird glitches like this:

I'm guessing that there are some timing constraint issues and sometimes we are dropping a bit while deserializing.

And if I run too many times, it eventually breaks the SPI engine and I get timeouts waiting for SYNC again. So will likely need to do some HDL debugging to figure this out.

dlech · 2025-08-15T20:41:27Z

I continued to refine the software today and apparently changed something that made it a bit happier.

The glitch went away, but now I've noticed that I'm not getting any data on 2 out of the 4 SDOs for some reason. Still investigating if this is an issue with the software, the eval board wiring or an HDL problem.

Also the data on the FIFO doesn't seem to be coming out in the expected order. Still investigating that as well.

dlech · 2025-08-15T21:31:59Z

library/spi_engine/axi_spi_engine/axi_spi_engine.v

+    end
+  end
+
+  //the current logic is considering that there is only one active lane in the set of lanes


Do I understand right that FIFO mode only supports one active lane currently? In other words, we can only enable multiple lanes when using the offload?

The current implementation allows the programmer to use a single active lane or all active lanes. According to @sarpadi , there is no use case for other masks for the SDO lanes. When in OFFLOAD mode, we are considering that the programmer is activating all lanes for the SDO lanes.

This limitation is not affecting the SDI side for FIFO or OFFLOAD modes. You just need to be careful for not changing the lane mask while executing the OFFLOAD mode.

dlech · 2025-08-15T21:57:12Z

The glitch went away, but now I've noticed that I'm not getting any data on 2 out of the 4 SDOs for some reason. Still investigating if this is an issue with the software, the eval board wiring or an HDL problem.

Rebooting seems to have solved this. Not sure what could have triggered it.

Also the data on the FIFO doesn't seem to be coming out in the expected order. Still investigating that as well.

The FIFO seems to struggle with multi-word reads. Here is an example where I am attempting to read 4 16-bit words repeatedly.

The first SPI message goes just fine.

The second SPI message has a long pause between the 3rd and 4th word for some reason.

Then the 3rd attempt has a long pause before reading the first two words and then just gets stuck forever and never reads the last two words.

fix DATA_WIDTH register. Signed-off-by: Carlos Souza <[email protected]>

It was missing the sdi_lane_mask register. That was not affecting execution because the only logic requiring this register is on the axi_spi_engine.v Signed-off-by: Carlos Souza <[email protected]>

LBFFilho reviewed Jul 2, 2025

View reviewed changes

docs/library/spi_engine/instruction-format.rst Outdated Show resolved Hide resolved

LBFFilho requested changes Jul 2, 2025

View reviewed changes

docs/library/spi_engine/instruction-format.rst Outdated Show resolved Hide resolved

LBFFilho reviewed Jul 2, 2025

View reviewed changes

docs/library/spi_engine/instruction-format.rst Outdated Show resolved Hide resolved

LBFFilho reviewed Jul 2, 2025

View reviewed changes

docs/library/spi_engine/instruction-format.rst Outdated Show resolved Hide resolved

caosjr force-pushed the sdo_extension_spi_engine branch 3 times, most recently from 8a2552e to 15eaefd Compare July 4, 2025 14:44

caosjr changed the title ~~library/spi_engine: Extend SDO support for the SPI Engine~~ library/spi_engine: SDO Extension upgrade Jul 7, 2025

caosjr force-pushed the sdo_extension_spi_engine branch 4 times, most recently from 4300d4c to 6d23286 Compare July 10, 2025 13:53

dlech reviewed Jul 14, 2025

View reviewed changes

caosjr force-pushed the sdo_extension_spi_engine branch from df039b8 to ce44e94 Compare August 5, 2025 19:36

caosjr marked this pull request as ready for review August 7, 2025 12:59

caosjr requested review from IuliaCMoldovan, gastmaier, StancaPop, sarpadi, podgori, ladace and PIoandan as code owners August 7, 2025 12:59

caosjr force-pushed the sdo_extension_spi_engine branch 2 times, most recently from 149396a to ab91a18 Compare August 7, 2025 15:14

caosjr requested review from acostina, AndreiGrozav and PopPaul2021 as code owners August 7, 2025 15:14

caosjr force-pushed the sdo_extension_spi_engine branch 2 times, most recently from cb4785c to 0ff6ae5 Compare August 7, 2025 15:19

caosjr force-pushed the sdo_extension_spi_engine branch 2 times, most recently from 644dbe7 to 1d4f513 Compare August 7, 2025 15:30

caosjr mentioned this pull request Aug 7, 2025

testbenches/ip/spi_engine: Upgrade spi_engine testbench for SDO extension analogdevicesinc/testbenches#240

Open

13 tasks

sarpadi requested a review from dlech August 11, 2025 14:02

caosjr added 5 commits August 12, 2025 10:17

docs/projects: update documentation of projects

3774b7f

Update NUM_OF_SDI to NUM_OF_SDIO in the projects documentations. The variable was updated on the spi_engine library since SDI and SDO have symmetrical sizes. Signed-off-by: Carlos Souza <[email protected]>

projects: updated NUM_OF_SDI parameter to NUM_OF_SDIO

15deab1

Updated the projects to use NUM_OF_SDIO parameter. It is the same project, no functionality has changed. Signed-off-by: Carlos Souza <[email protected]>

caosjr force-pushed the sdo_extension_spi_engine branch from f1c8844 to 15deab1 Compare August 12, 2025 13:52

dlech reviewed Aug 14, 2025

View reviewed changes

library/spi_engine/axi_spi_engine/axi_spi_engine.v Show resolved Hide resolved

dlech reviewed Aug 14, 2025

View reviewed changes

dlech reviewed Aug 15, 2025

View reviewed changes

caosjr added 2 commits August 15, 2025 23:00

docs/library/spi_engine: fix regmap

ea4146c

fix DATA_WIDTH register. Signed-off-by: Carlos Souza <[email protected]>

library/spi_engine: fix execution module

8c90fc3

It was missing the sdi_lane_mask register. That was not affecting execution because the only logic requiring this register is on the axi_spi_engine.v Signed-off-by: Carlos Souza <[email protected]>

caosjr marked this pull request as draft August 18, 2025 17:29


		.. _spi_engine sdi-lane-mask-register:

		SDI Lane Mask Register

library/spi_engine: SDO Extension upgrade #1808

Are you sure you want to change the base?

library/spi_engine: SDO Extension upgrade #1808

Uh oh!

Conversation

caosjr commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Description

PR Type

PR Checklist

Uh oh!

LBFFilho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlech left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caosjr Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caosjr Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlech commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlech commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlech commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlech commented Aug 15, 2025

Uh oh!

Uh oh!

caosjr commented Jul 1, 2025 •

edited

Loading

dlech left a comment •

edited

Loading

caosjr Aug 18, 2025 •

edited

Loading

caosjr Aug 18, 2025 •

edited

Loading

dlech commented Aug 14, 2025 •

edited

Loading

dlech commented Aug 15, 2025 •

edited

Loading

dlech commented Aug 15, 2025 •

edited

Loading