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

DMA Generic Transfer Start Synchronization #1341

Merged
merged 6 commits into from
Jul 16, 2024
Merged

DMA Generic Transfer Start Synchronization #1341

merged 6 commits into from
Jul 16, 2024

Conversation

podgori
Copy link
Contributor

@podgori podgori commented May 28, 2024

PR Description

This PR adds a generic sync port used for transfer start synchronization. The signal used for synchronization is assumed to be synchronous with the interface clock which needs to be triggered by the sync signal.

On the receive AXI Streaming interface, the user can choose to receive the synchronization signal on TUSER instead of 'sync',
which is the default setting, configurable through the AXIS_TUSER_SYNC synthesis parameter.

The fifo_wr interface has the SYNC signal removed, since it is now a generic synchronization port, applicable to all interfaces. Therefore, all affected IPs and Projects had to be updated to address this change.

Tested this change in simulation. Please refer to: https://github.com/analogdevicesinc/testbenches/tree/pluto_phaser_tx_sync

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

Copy link
Contributor

@gastmaier gastmaier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The resulting cpack2->dma connection is weird, because you renamed on the DMAC side but not on CPACK, only popped of the intf.

I would rename it from packed_fifo_wr_sync to packed_sync on cpack, and on the tcl files
to:

ad_connect  <inst>_pack/packed_sync <inst>_dma/sync

instead of:

ad_connect  <inst>_pack/packed_fifo_wr_sync <inst>_dma/sync

I synthesized pulsar_adc/coraz7s and cn0540/de10nano with the PR rebased to main without issues.
I also rebased #1332 onto yours and I got only a few merge conflicts that I was able to easily resolve.


the suggested tb also works rebased to main

library/axi_dmac/axi_dmac_regmap.v Show resolved Hide resolved
library/axi_dmac/axi_dmac_ip.tcl Outdated Show resolved Hide resolved
docs/regmap/adi_regmap_dmac.txt Show resolved Hide resolved
@gastmaier
Copy link
Contributor

From the CI run, can you ensure these projects were already failing?

fmcomms5.zc702
fmcomms5.zcu102
fmcomms5.zc706
adrv9009zu11eg.adrv2crr_fmcxmwbr1
adrv9009zu11eg.adrv2crr_fmcomms8

@podgori
Copy link
Contributor Author

podgori commented May 31, 2024

V2:

  • renamed packed_fifo_wr_sync into packed_sync
  • updated all affected projects by the above change
  • updated all projects which had the cpack - dma sync connection hidden, but did not have the SYNC_TRANSFER_START parameter set (exception: ad7606x which is in progress)
  • fixed indentation by removing tabs

gastmaier
gastmaier previously approved these changes Jun 4, 2024
@SRaus
Copy link
Collaborator

SRaus commented Jun 7, 2024

From the CI run, can you ensure these projects were already failing?

fmcomms5.zc702
fmcomms5.zcu102
fmcomms5.zc706
adrv9009zu11eg.adrv2crr_fmcxmwbr1
adrv9009zu11eg.adrv2crr_fmcomms8

You can compare the PR results with the latest main results (or any release branch) anytime.
image

acostina
acostina previously approved these changes Jun 14, 2024
@podgori
Copy link
Contributor Author

podgori commented Jul 2, 2024

V2.1: Updated the Optimization Mode on fmcomms8.a10soc in order to improve timing; rebased on the latest changes on main.

gastmaier
gastmaier previously approved these changes Jul 2, 2024
Copy link
Contributor

@gastmaier gastmaier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!
I believe we could squash into a single commit when merging since the intermediary commits are non-functional, just make sure to update the message body to properly describe the changes.

@gastmaier gastmaier mentioned this pull request Jul 8, 2024
12 tasks
acostina
acostina previously approved these changes Jul 8, 2024
This commit adds a generic 'sync' port used for transfer start.
The signal used for synchronization is assumed to be synchronous
with the interface clock.

On the receive AXI Streaming interface, the user can choose to
receive the synchronization signal on TUSER instead of 'sync',
which is the default setting, configurable through the
AXIS_TUSER_SYNC synthesis parameter.

Signed-off-by: Ionut Podgoreanu <[email protected]>
@podgori
Copy link
Contributor Author

podgori commented Jul 10, 2024

V2.2: Some minor edits to improve the build results; also rebased on the latest changes on main.

  • ad7606x: enabled SYNC_TRANSFER_START and updated the sync connection;
  • adrv9026: updated the sync connection;
  • adrv9009zu11eg: tied adc_sync_in to GND;
  • fmcomms5: tied tdd_sync to GND;
  • jupiter_sdr: tied tdd_sync to GND.

@podgori podgori merged commit 9ca41b7 into main Jul 16, 2024
1 of 3 checks passed
@podgori podgori deleted the dma_tx_sync branch July 16, 2024 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants