PulSAR LVDS #1273

PIoandan · 2024-02-09T09:09:42Z

PR Description

Add support for fast LVDS PulSAR AD762x/AD796x ADC.

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

projects/pulsar_lvds_adc/zed/Makefile

sarpadi · 2024-03-19T08:07:36Z

library/axi_pulsar_lvds/axi_pulsar_lvds.v

+  parameter ADC_DATA_WIDTH = 16,
+  parameter BITS_PER_SAMPLE = 32
+) (
+  input                     delay_clk,


please minimum amount of whitespaces to align signals
output [31:0] adc_data, => output [31:0] adc_data,

this aplies to most wire/reg declarations as well

sarpadi · 2024-03-19T08:16:23Z

library/axi_pulsar_lvds/axi_pulsar_lvds.v

+  // processor read interface
+
+  always @(negedge up_rstn or posedge up_clk) begin
+    if (up_rstn == 0) begin


change line to
if (!up_rstn) begin

sarpadi · 2024-03-19T08:17:34Z

library/axi_pulsar_lvds/axi_pulsar_lvds_channel.v

+
+  // adc interface
+
+  input                  adc_clk,


use fewer whitespaces for alignment

sarpadi · 2024-03-19T08:19:28Z

library/axi_pulsar_lvds/axi_pulsar_lvds_if.v

+
+  // delay interface
+
+  input             up_clk,


use fewer whitespaces

sarpadi · 2024-03-19T08:23:13Z

library/axi_pulsar_lvds/axi_pulsar_lvds.v

+    .DELAY_REFCLK_FREQUENCY (DELAY_REFCLK_FREQUENCY),
+    .IODELAY_CTRL (IODELAY_CTRL),
+    .ADC_DATA_WIDTH (ADC_DATA_WIDTH)
+


remove blank line

sarpadi · 2024-03-19T08:23:36Z

projects/pulsar_lvds_adc/common/pulsar_lvds_adc_bd.tcl

+ad_ip_parameter reference_clkgen CONFIG.VCO_DIV 1
+ad_ip_parameter reference_clkgen CONFIG.VCO_MUL 10
+ad_ip_parameter reference_clkgen CONFIG.CLK0_DIV 6
+#ad_ip_parameter reference_clkgen CONFIG.CLK1_DIV 4


remove commented line

sarpadi · 2024-03-19T08:24:02Z

projects/pulsar_lvds_adc/zed/ad7626_system_top.v

+
+module system_top (
+
+  inout       [14:0]      ddr_addr,


too many whitespaces used for alignment

sarpadi · 2024-03-19T08:24:24Z

projects/pulsar_lvds_adc/zed/ad7626_system_top.v

+
+  // internal signals
+
+  wire  [63:0]  gpio_i;


too many whitespaces used for alignment

projects/pulsar_lvds_adc/zed/system_project.tcl

PIoandan · 2024-03-21T07:38:29Z

Made the requested changes.

Signed-off-by: Ioan-daniel Pop <Pop.Ioan-daniel@analog.com>

bia1708 · 2024-03-26T12:40:15Z

RetriggerCI

gastmaier

Minor comments
Tested synthesize on both 16/18 resolutions.
No critical warnings

gastmaier · 2024-03-26T17:40:24Z

projects/pulsar_lvds_adc/common/pulsar_lvds_adc_bd.tcl

+ad_cpu_interconnect 0x44A30000 axi_pulsar_lvds_dma
+ad_cpu_interconnect 0x44A60000 axi_pwm_gen
+ad_cpu_interconnect 0x44a80000 reference_clkgen
+# interconnect (adc)


Add blank line

gastmaier · 2024-03-26T17:46:19Z

projects/pulsar_lvds_adc/common/pulsar_lvds_adc_bd.tcl

+ad_cpu_interconnect 0x44A00000 axi_pulsar_lvds
+ad_cpu_interconnect 0x44A30000 axi_pulsar_lvds_dma
+ad_cpu_interconnect 0x44A60000 axi_pwm_gen
+ad_cpu_interconnect 0x44a80000 reference_clkgen


Uppercase this hex value, or lower case the others hex values.

gastmaier · 2024-03-26T17:46:59Z

projects/pulsar_lvds_adc/common/pulsar_lvds_adc_bd.tcl

+set ADC_DATA_WIDTH [expr {$RESOLUTION_16_18N == 1 ? 16 : 18}]
+set BITS_PER_SAMPLE [expr {$RESOLUTION_16_18N == 1 ? 16 : 32}]
+
+# ltc2387


Add blank line

gastmaier · 2024-03-26T17:48:00Z

projects/pulsar_lvds_adc/Readme.md

+How to use over-writable parameters from the environment:
+```
+hdl/projects/pulsar_lvds_adc/zed> make RESOLUTION_16_18N=0
+RESOLUTION_16_18N  - Defines the resolution of the ADC: 0 - 18 BITS, 1 - 16 BITS.


Missing closing ```

gastmaier · 2024-03-26T17:54:31Z

projects/pulsar_lvds_adc/common/pulsar_lvds_adc_bd.tcl

+set BITS_PER_SAMPLE [expr {$RESOLUTION_16_18N == 1 ? 16 : 32}]
+
+# ltc2387
+create_bd_port -dir I ref_clk


This port doesn't seem to be used.

Deleted it for now.

gastmaier · 2024-03-26T17:58:14Z

projects/pulsar_lvds_adc/common/pulsar_lvds_adc_bd.tcl

+ad_connect d_n        axi_pulsar_lvds/d_n
+
+ad_connect reference_clkgen/clk_0    axi_pulsar_lvds_dma/fifo_wr_clk
+ad_connect axi_pulsar_lvds/adc_valid axi_pulsar_lvds_dma/fifo_wr_en


Why isn't adc_valid , adc_data , and adc_dovf a single fifo_wr interface?

gastmaier · 2024-03-26T18:04:32Z

library/axi_pulsar_lvds/axi_pulsar_lvds.v

+`timescale 1ns/100ps
+
+module axi_pulsar_lvds #(
+


Remove extra line

gastmaier · 2024-03-26T18:16:05Z

library/axi_pulsar_lvds/axi_pulsar_lvds_ip.tcl

+  "axi_pulsar_lvds.v" ]
+
+adi_ip_properties axi_pulsar_lvds
+


You could pack adc_valid, adc_data, adc_dovf into a fifo_wr interface, so when you connect to the axi_dmac/fifo_wr(slave) it doesn't raise unnecessary warnings.

Suggested change

adi_add_bus "fifo_wr" "master" \

"analog.com:interface:fifo_wr_rtl:1.0" \

"analog.com:interface:fifo_wr:1.0" \

{ \

{"adc_valid" "EN"} \

{"adc_data" "DATA"} \

{"adc_dovf " "OVERFLOW"} \

}

adi_add_bus_clock "fifo_wr_clk" "fifo_wr"

gastmaier · 2024-03-26T18:37:41Z

library/axi_pulsar_lvds/axi_pulsar_lvds.v

+  // dma interface
+
+  output        adc_valid,
+  output [31:0] adc_data,


Couldn't this be

Suggested change

output [31:0] adc_data,

output [BITS_PER_SAMPLE-1:0] adc_data,

gastmaier · 2024-03-26T18:39:00Z

library/axi_pulsar_lvds/axi_pulsar_lvds_channel.v

+
+  output        adc_enable,
+  output        adc_valid,
+  output [31:0] adc_data,


Couldn't this be

Suggested change

output [31:0] adc_data,

output [BITS_PER_SAMPLE-1:0] adc_data,

Signed-off-by: Ioan-daniel Pop <Pop.Ioan-daniel@analog.com>

PopPaul2021

Looks good to me!

PopPaul2021 · 2024-02-21T11:41:48Z

library/axi_pulsar_lvds/axi_pulsar_lvds.v

+    .DELAY_REFCLK_FREQUENCY (DELAY_REFCLK_FREQUENCY),
+    .IODELAY_CTRL (IODELAY_CTRL),
+    .ADC_DATA_WIDTH (ADC_DATA_WIDTH)
+


There is an extra empty line here.

PopPaul2021 · 2024-02-21T11:43:39Z

projects/pulsar_lvds_adc/common/pulsar_lvds_adc_bd.tcl

+ad_ip_parameter reference_clkgen CONFIG.VCO_DIV 1
+ad_ip_parameter reference_clkgen CONFIG.VCO_MUL 10
+ad_ip_parameter reference_clkgen CONFIG.CLK0_DIV 6
+#ad_ip_parameter reference_clkgen CONFIG.CLK1_DIV 4


This line can be deleted.

changes done

PIoandan requested review from acostina, AndreiGrozav and PopPaul2021 as code owners February 9, 2024 09:09

gastmaier self-requested a review February 9, 2024 11:21

PIoandan force-pushed the pulsar_lvds_dev branch from a3f975d to aa6832b Compare February 21, 2024 08:34

PIoandan force-pushed the pulsar_lvds_dev branch from 4eca858 to a1c6b5a Compare March 19, 2024 07:09

sarpadi self-requested a review March 19, 2024 07:46

sarpadi reviewed Mar 19, 2024

View reviewed changes

PIoandan force-pushed the pulsar_lvds_dev branch 3 times, most recently from fb7b120 to fa02ebd Compare March 21, 2024 07:36

PIoandan requested a review from sarpadi March 21, 2024 07:38

PIoandan added 2 commits March 21, 2024 16:22

library/axi_pulsar_lvds

96b5f19

Signed-off-by: Ioan-daniel Pop <Pop.Ioan-daniel@analog.com>

project/pulsar_lvds_adc

e224aa7

Signed-off-by: Ioan-daniel Pop <Pop.Ioan-daniel@analog.com>

PIoandan force-pushed the pulsar_lvds_dev branch from fa02ebd to e224aa7 Compare March 21, 2024 14:24

sarpadi previously approved these changes Mar 22, 2024

View reviewed changes

gastmaier previously requested changes Mar 26, 2024

View reviewed changes

Requested changes

9b258f3

Signed-off-by: Ioan-daniel Pop <Pop.Ioan-daniel@analog.com>

PIoandan dismissed sarpadi’s stale review via 9b258f3 April 12, 2024 12:57

PIoandan requested review from sarpadi and gastmaier April 12, 2024 12:57

sarpadi approved these changes Apr 15, 2024

View reviewed changes

PopPaul2021 approved these changes Apr 15, 2024

View reviewed changes

sarpadi merged commit ab4ea30 into main Apr 16, 2024
1 of 3 checks passed

sarpadi deleted the pulsar_lvds_dev branch April 16, 2024 08:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PulSAR LVDS #1273

PulSAR LVDS #1273

PIoandan commented Feb 9, 2024 •

edited

Loading

sarpadi Mar 19, 2024

sarpadi Mar 19, 2024

sarpadi Mar 19, 2024

sarpadi Mar 19, 2024

sarpadi Mar 19, 2024

sarpadi Mar 19, 2024

sarpadi Mar 19, 2024

sarpadi Mar 19, 2024

PIoandan commented Mar 21, 2024

bia1708 commented Mar 26, 2024

gastmaier left a comment

gastmaier Mar 26, 2024

PIoandan Apr 15, 2024

gastmaier Mar 26, 2024

PIoandan Apr 15, 2024

gastmaier Mar 26, 2024

PIoandan Apr 15, 2024

gastmaier Mar 26, 2024

PIoandan Apr 15, 2024

gastmaier Mar 26, 2024

PIoandan Apr 15, 2024

gastmaier Mar 26, 2024

gastmaier Mar 26, 2024

PIoandan Apr 15, 2024

gastmaier Mar 26, 2024

PIoandan Apr 15, 2024

gastmaier Mar 26, 2024

PIoandan Apr 15, 2024

gastmaier Mar 26, 2024

PIoandan Apr 15, 2024

PopPaul2021 left a comment

PopPaul2021 Feb 21, 2024

PopPaul2021 Feb 21, 2024

+adi_add_bus "fifo_wr" "master" \
+        "analog.com:interface:fifo_wr_rtl:1.0" \
+        "analog.com:interface:fifo_wr:1.0" \
+        { \
+                {"adc_valid" "EN"} \
+                {"adc_data" "DATA"} \
+                {"adc_dovf  " "OVERFLOW"} \
+        }
+adi_add_bus_clock "fifo_wr_clk" "fifo_wr"

	output [31:0] adc_data,
	output [BITS_PER_SAMPLE-1:0] adc_data,

		`timescale 1ns/100ps

		module axi_pulsar_lvds #(

PulSAR LVDS #1273

PulSAR LVDS #1273

Conversation

PIoandan commented Feb 9, 2024 • edited Loading

PR Description

PR Type

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIoandan commented Mar 21, 2024

bia1708 commented Mar 26, 2024

gastmaier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PopPaul2021 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIoandan commented Feb 9, 2024 •

edited

Loading