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

[BUG] AD9361 Xilinx axi_ad9361_lvds_if #916

Open
AlexLao512 opened this issue Apr 9, 2022 · 3 comments · May be fixed by #1460
Open

[BUG] AD9361 Xilinx axi_ad9361_lvds_if #916

AlexLao512 opened this issue Apr 9, 2022 · 3 comments · May be fixed by #1460
Labels

Comments

@AlexLao512
Copy link

Seems commit 83d3bde from years ago completely breaks RX on the AD9361. Prior commits of this file works perfectly for us, but the changes in this commit breaks it. When I look at the rx_frame signals in a logic analyze at runtime, the case statements for delineation can not possibly work. This is an RTL bug, tool and software versions should not matter here.

image

Used Software: No-OS
Tool version: Vivado 2019.1.3
HDL Release version hdl_2019_r2
Software Release version 2019_r2

@AlexLao512 AlexLao512 added the bug label Apr 9, 2022
@dmitrii-galantsev
Copy link

Affirmative, can observe in my setup as well. 👎

@johnathan-convertino-afrl
Copy link

johnathan-convertino-afrl commented Dec 21, 2023

Change lines 222 to 229 in hdl/library/axi_ad9361/xilinx
/axi_ad9361_lvds_if.v to the snippet below.

always @(posedge l_clk) begin
    rx_error_r1 <= ~(({rx_frame, rx_frame_s} == 4'b1100) || ({rx_frame, rx_frame_s} == 4'b0011));
    rx_error_r2 <= ~(({rx_frame, rx_frame_s} == 4'b1111) || ({rx_frame, rx_frame_s} == 4'b1100) ||
                     ({rx_frame, rx_frame_s} == 4'b0000) || ({rx_frame, rx_frame_s} == 4'b0011));
  end

  always @(posedge l_clk) begin
      case ({rx_r1_mode, rx_frame, rx_frame_s})

A few years back I ran into this issue and fixed it for my own HDL project. The original code using rx_frame_s only makes zero sense, as its only a 2 bit vector being compared to a 4 bit vector. Fix is based on the previous code. I also changed the order of the case statement as well.

Wish I had documented the changes better, my commits don't reflect the reasons. It was bundled with other changes involved in arria10 timing. Lesson, make better commits.

My file can be found at: https://github.com/johnathan-convertino-afrl/hdl/blob/hdl_quartus_pro_2023_r1/library/axi_ad9361/axi_ad9361_lvds_if.v

@IuliaCMoldovan
Copy link
Contributor

Hi all,

Thank you for letting us know, and apologies for the very late reply; somehow we missed this. We don't really track the GitHub issues, as most of the inquiries and comments come through our EngineerZone forum.
This will be fixed in the future commits.

Best regards,
Iulia

gastmaier added a commit that referenced this issue Sep 12, 2024
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 added a commit that referenced this issue Sep 17, 2024
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 linked a pull request Sep 17, 2024 that will close this issue
12 tasks
gastmaier added a commit that referenced this issue Nov 26, 2024
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants