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

axi_ad9361: Fixup LVDS RX_FRAME (#916) #1460

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

axi_ad9361: Fixup LVDS RX_FRAME (#916) #1460

wants to merge 1 commit into from

Conversation

gastmaier
Copy link
Contributor

@gastmaier gastmaier commented Sep 17, 2024

PR Description

The rx_error_r* on the axi_ad9361/xilinx/axi_ad9361_lvds_if.v were not implemented properly.
the author intended it to be the inverse of all valid frames combinations {rx_frame, rx_frame_s} but only compared against rx_frame_s.

Checking against all valid frames is also unnecessary, since checking for unequal DDR output values have the same practical effect.
It is also worth noting that at 2RX mode, a frame fulfills the 2 channels in 4 clock cycles, while {rx_frame, rx_frame_s} looks back only 2 clock cycles.

image
UG-570, p95

This commit changes rx_error to be a "link stable signal".

Relevant information:
AD9361:
operates in DDR
Linux/no-os drivers set (or leave default):

  • reg 0x010 value 0xC8, bit 2 -> Rx Frame Pulse Mode : 50% duty cycle
  • reg 0x011 value 0x00, bit 2 -> Invert Rx Frame : don't invert

AXI_AD9361:

rx_r1_mode is ADC_COMMON 0x0011[2] R1_MODE with:

  • clear: 2 channels
  • set: 1 channel

status is ADC_COMMON 0x0017 [0] STATUS with:

  • clear : error on intf

Aims to fix #916, but does not try to fix the frame skew/unequal ddr output observed in the issue author first screenshot, however, the prior logic may resolve into the error signal never asserting, entering runtime without properly aligning, somewhere around ad9361_conv.c#ad9361_dig_tune_delay.
An unaligned frame results into the following Linux boot log message:

SAMPL CLK: 61440000 tuning: RX
  0:1:2:3:4:5:6:7:8:9:a:b:c:d:e:f:
0:# # # # # # # # # # # # # # # #
1:# # # # # # # # # # # # # # # #
ad9361 spi0.0: ad9361_dig_tune_delay: Tuning RX FAILED!

Tested on hardware in loopback with single channel, dual channel, single tone, dual tone, and BISPs.
Updates only the xilinx side, since the 83d3bde only edited it, but the error logic could be added to intel also.

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

The rx_error_r* registers were miss-implemented,
they intended to be the negate of valid {rx_frame, rx_frame_s}
but only took rx_frame_s into check.
As an error for the current adc_data is also improper, because
a 2Rx takes 4 clock cycles to fulfil, while {rx_frame, rx_frame_s}
only look back 2 clock cycles.

Change the logic to have rx_error solely as a link stable signal.

Relevant information:
AD9361 operates in DDR.
AD9361 operating register values according to drivers:
reg 0x010 value 0xC8[2] -> Rx Frame Pulse Mode : 50% duty cycle
reg 0x011 value 0x00[2] -> Invert Rx Frame : don't

rx_r1_mode is ADC_COMMON 0x0011[2] R1_MODE with:
clear: 2 channels
set: 1 channel

Signed-off-by: Jorge Marques <[email protected]>
@gastmaier gastmaier marked this pull request as ready for review September 18, 2024 15:33
@AndreiGrozav
Copy link
Contributor

Can you increment the year for the last change in the license header?
The code looks good.

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.

[BUG] AD9361 Xilinx axi_ad9361_lvds_if
2 participants