Skip to content

Fix CDC issue: Synchronize i_hsb_stat[0] in bootp module#109

Open
umairsiddiqui-digitek wants to merge 1 commit into
nvidia-holoscan:mainfrom
umairsiddiqui-digitek:verilog_bootp_missing_sync
Open

Fix CDC issue: Synchronize i_hsb_stat[0] in bootp module#109
umairsiddiqui-digitek wants to merge 1 commit into
nvidia-holoscan:mainfrom
umairsiddiqui-digitek:verilog_bootp_missing_sync

Conversation

@umairsiddiqui-digitek

@umairsiddiqui-digitek umairsiddiqui-digitek commented Jul 3, 2026

Copy link
Copy Markdown

Hi,

I noticed a Clock Domain Crossing (CDC) issue where the signal i_hsb_stat in the bootp.sv module is being used without proper synchronization.

Details:

Currently, only bit-0 (i_hsb_stat[0]) is connected in bootp.sv (Line 330).
 

assign hsb_stat = {7'h0, ptp_en_sync_rx};

This net is driven by the output signal o_ptp_en_sync_rx from the ptp_top block, as seen in ptp_top.sv (Line 58).
 

output o_ptp_en_sync_rx, //Enable Sync RX

Problem:
The i_hsb_stat[0] signal is generated in the ptp_top domain clocked by i_ptp_clk. However, the bootp block operates on i_hif_clk. Using this signal directly without synchronization can lead to metastability.

Solution:
This PR adds the necessary synchronization logic to safely cross i_hsb_stat[0] over to the i_hif_clk domain before it is consumed by the bootp block.

Regards,

Summary by CodeRabbit

  • Bug Fixes
    • Improved BOOTP vendor data reliability by using a clock-synchronized status signal, reducing the risk of inconsistent or unstable data being exposed across clock domains.

  // i_hsb_stat[0] is generated in ptp_top.sv @ i_ptp_clk, 
  // therefore, in bootp block it need to be sync on i_hif_clk 

Signed-off-by: umairsiddiqui-digitek <41374313+umairsiddiqui-digitek@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7a0eaa8b-8925-4af8-b84f-feabe86cb70d

📥 Commits

Reviewing files that changed from the base of the PR and between fc2a6cf and 3de79b7.

📒 Files selected for processing (1)
  • fpga/nv_hsb_ip/bootp/bootp.sv

Walkthrough

The BOOTP module in bootp.sv now synchronizes the i_hsb_stat input into the i_clk clock domain using a new data_sync instance, producing i_hsb_stat_sync. The bootp_vend_data field is updated to use this synchronized signal instead of the raw i_hsb_stat.

Changes

BOOTP clock-domain sync

Layer / File(s) Summary
Add status synchronizer and update usage
fpga/nv_hsb_ip/bootp/bootp.sv
Instantiates a data_sync block to synchronize i_hsb_stat into i_clk (i_hsb_stat_sync) with reset handling, and updates bootp_vend_data to use i_hsb_stat_sync instead of i_hsb_stat.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Related Issues

None found.

Related PRs

None found.

Suggested labels: fpga, hardware

Suggested reviewers: None identified.

Poem: 🐰 A signal once raced through the clock's domain, / Now synced and steady, no more strain. / Through data_sync it hops in line, / bootp_vend_data reads it fine. / A tiny fix, a rabbit's cheer! 🎉

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly states the CDC fix and the BOOTP module synchronization change.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

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.

1 participant