Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dev quad ad77681 #1490

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from
Draft

Dev quad ad77681 #1490

wants to merge 18 commits into from

Conversation

sarpadi
Copy link
Contributor

@sarpadi sarpadi commented Oct 15, 2024

HDL for PMB00004 board

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)

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

sarpadi and others added 18 commits October 10, 2024 14:31
The SPI_CLK needs to be the reference clk divided by 2.

Signed-off-by: PopPaul2021 <[email protected]>
Rename the project and update the files that are affected.

Signed-off-by: Pop Ioan Daniel <[email protected]>
@sarpadi sarpadi requested a review from dlech October 18, 2024 09:11
@sarpadi
Copy link
Contributor Author

sarpadi commented Oct 18, 2024

Hi @dlech. This PR includes changes to the axi_spi_engine IP core that aim to provide FIFO mode support for multiple SDI lines. This means that we have to add extra registers and we also had to move some of the existing ones to get a better usage of address space. Any feedback would be nice.

https://github.com/analogdevicesinc/hdl/pull/1490/files#diff-d1274cfe2e206aa66a0ecd3da04b3e62fc5fad9e12029b34b226c6f91454d34d
https://github.com/analogdevicesinc/hdl/pull/1490/files#diff-05633a28329dff91191debd876bef07f0693c441d2af021d4b8c82e3f0d44aff

@dlech
Copy link
Collaborator

dlech commented Oct 21, 2024

This means that we have to add extra registers and we also had to move some of the existing ones to get a better usage of address space.

Then we will need to bump the version of the IP block to 2.0.0 so that we don't end up with existing drivers trying to poke the wrong registers. (Existing drivers already check for a major version of 1.)

This PR includes changes to the axi_spi_engine IP core that aim to provide FIFO mode support for multiple SDI lines.

Hmm... what advantage is there to adding more FIFOs vs. putting all data received in a single FIFO? I suppose it makes sense for the PEEK registers, but am not familiar with why those are needed. It isn't something we are using in the Linux kernel driver.

For offload support, will there also be 8 different DMA streams? It seems like we would want just one DMA stream where every n words are sent at the same time.


Taking a step back from this specific use case, here is a broader view of the needs I have seen in the various chips I have looked at so far. Even if we don't implement all of these use cases right away, I think it will be useful to have them in mind while working on this.

Multiple SDO lines

We should also consider multiple SDO lines for DAC chips. Basically everything for multiple SDI lines also applies for multiple SDO lines. I.e, if we end up rearranging registers, we should make sure we reserve space for new SDO registers as well.

Data channels vs. dual/quad SPI

We also need to consider how data is distributed on the lines. I've seen two different ways.

  1. The case that looks like what is being done in this PR where each ADC channel has it's own dedicated where data is shifted out. I'm not aware of any off-the-shelf SPI controllers that actually support something like this, so if anyone knows of one, please let me know.

    image

  2. There are also other chips, like AD4030 that take a single data value and split it up between multiple SDI lines, e.g. even bits on one and odd bits on the other. It is rare, but I've seen at least one off-the-shelf SPI controller that looks like it supports this.

    image

  3. There are even chips like AD4630 that do both at the same time! (8 SDI lines, 2 channels, 4 lines per channel)

    image

So, at the very least, we need to make a distinction between the number of physical SDI lines and the number of channels that can be read at the same time.

Full-duplex vs. half-duplex (traditional dual/quad SPI)

Some chips, like AD3552R have parallel bidirectional SDIO lines instead of dedicated separate SDI and SDO lines. These act like the conventional dual/quad SPI controllers that are a bit more common.

Bit order for split words

Most chips follow the defacto standards and put the most significant bit on the lowest numbered SDIO line. AD3552R decided to do it in the opposite order, so we also need to take this into consideration as a configurable parameter.

image

Selecting configuration at runtime vs. at compile time

Some chips need to use single SDI/SDO lines for reading/writing registers and then switch to multiple SDI/SDO lines to read/write sample data. As seen in the screenshot above for AD3552R, we may even need to do both in the same SPI message.

So we will likely need to modify the SPI engine instructions to add a per transfer selection of how many SDI/SDO lines to use for an individual transfer.

Linux kernel considerations

  1. There is already a per-transfer setting for number of SDI/SDO lines that would work to support the suggested new SPI Engine instruction.
  2. The kernel already supports transfers where a single word is split up between multiple SDI/SDO lines.
  3. However, it does not support transfers where each SDI/SDO line contains an independent data channel.
  4. Any HDL compile-time setting that is used for these features needs to be made available to read via a memory mapped register (if it isn't already) so that we can set the proper flags require by the Linux SPI framework to indicate what the controller is actually capable of.

In summary, I think the most import things to consider up front are:

  1. Separating the concepts of the number of physical SDI lines from the number of data channels and number of lines per channel. Ideally, the number of physical lines would be an HDL compile-time setting while the use of those lines would be runtime configurable on a per-transfer basis.
  2. Considering what the offload DMA layout for multiple parallel channels would look like and use that same logic with a single FIFO rather than having multiple FIFOs.

@dlech
Copy link
Collaborator

dlech commented Oct 29, 2024

In my previous comments, I didn't realize that the problem we are trying to solve is reading/writing data to multiple separate chips at the same time. So, I've done some more research on this and found some interesting "prior art".

There are a number of specialized SPI controllers for SPI flash memory that support connecting to multiple flash memory chips in series or in parallel. The parallel case is the most similar to what we are trying to do here. So if we can emulate those controllers, that would be best in terms of coming up with a software interface that works well with other controllers too.

One that is well documented is the "Generic QSPI Controller" on Zynq Ultrascale.

image
https://docs.amd.com/r/en-US/ug1085-zynq-ultrascale-trm/Two-SPI-Flash-Memories-with-Separate-Buses-Dual-Parallel

It is basically a single SPI controller with two completely separate SPI buses that allow controlling them individually or at the same time. Here is what it make available for configuration (by the way, this is configuration per-transfer).

image
https://docs.amd.com/r/en-US/ug1085-zynq-ultrascale-trm/Generic-Command-FIFO-20-bit-Width-and-32-bit-Depth

The key features I see here are:

  1. In addition to being able to select which chip select to use, there is also a selection of which bus to use. This is what would be needed to allow configuring registers on each individual chip (lower or upper bus) and then read data from both at the same time (both busses).
  2. There is a "striping" feature that specifies how the data is sent to each bus when both busses are enabled. This controller says it can only stripe each byte though, not each word, so unfortunately, wouldn't work with >8-bit chips without having to unscramble the data. Of course, we could extend this so something that works better for our use cases.

As a 2nd data point, STM32 has a OCTISPI controller that can be operated in a dual-quad mode similar to this. It also has a single FIFO data register and does 1-byte interleaving/striping of the data (i.e. even bytes go to one chip and odd bytes go to other chip).


In summary, this doesn't really change any of my suggestions in the first reply. But it does make it more clear the direction we should be aiming for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants