Introduce tuner transaction interface #9

sunjin-choi · 2025-07-09T21:19:46Z

Summary

add tuner_txn_if with combined request/response handshake
implement tuner_ctrl_txn_adapter to bridge old arbiter logic
hook search PHY through the new interface and adapter
align interface fields with naming conventions (val/rdy)
drop generated verible.filelist

Testing

cmake .. (fails: Cannot find file containing module)

https://chatgpt.com/codex/tasks/task_e_686daf84a7a4832db0ec3091ef578c99

gemini-code-assist

Summary of Changes

Hello @sunjin-choi, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly refactors the tuner control mechanism by introducing a new, more abstract transaction interface (tuner_txn_if). This change aims to simplify the interaction between tuner controllers and the physical layer, moving from a multi-step arbiter logic to a single request/response handshake. An adapter module is provided to ensure compatibility with legacy arbiter interfaces, facilitating a smoother transition and cleaner design for future tuner operations.

Highlights

New Transaction Interface: Introduced tuner_txn_if, a new SystemVerilog interface designed for a combined request/response handshake, simplifying tuning operations with val/rdy naming conventions.
Interface Adapter: Implemented tuner_ctrl_txn_adapter to bridge the new tuner_txn_if with the existing tuner_ctrl_arb_if, allowing for a phased migration and encapsulating complex handshaking logic.
PHY Integration: Refactored tuner_search_phy to directly utilize the new tuner_txn_if, removing its internal arbiter state machine and streamlining its control flow for tuning and measurement.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request introduces a new transaction-based interface (tuner_txn_if) and an adapter module to simplify the control logic in tuner_search_phy, improving modularity and readability. A critical syntax error was identified in lib/verilog/tuner/tuner_search_phy.sv due to missing end keywords, which needs to be addressed. Additionally, a medium-severity comment suggests adding a timeout mechanism to the tuner_ctrl_txn_adapter module to prevent potential deadlocks.

gemini-code-assist · 2025-07-09T21:23:05Z

lib/verilog/tuner/tuner_search_phy.sv

-  // Majority-Vote
-  /*assign pwr_incremented = (i_dig_pwr_detected > pwr_detected_track_window[0]);*/
+      pwr_det_track   <= txn_if.meas_power;
+      ring_tune_track <= ring_tune;


The always_ff block starting on line 304 and the else if block starting on line 314 are missing their closing end keywords, which is causing a syntax error and build failure.

pwr_det_track <= txn_if.meas_power; ring_tune_track <= ring_tune; end end

gemini-code-assist · 2025-07-09T21:23:05Z

lib/verilog/tuner/tuner_ctrl_txn_adapter.sv

+      IDLE: if (txn_if.val) state_next = WAIT_TUNE;
+      WAIT_TUNE: if (ctrl_if.get_ctrl_tune_ack(CHANNEL)) state_next = WAIT_COMMIT;
+      WAIT_COMMIT: if (ctrl_if.get_ctrl_commit_ack(CHANNEL)) state_next = RESP;
+      RESP: if (txn_if.fire()) state_next = IDLE;


The state machine could potentially get stuck in WAIT_TUNE or WAIT_COMMIT states if the expected ack signals from ctrl_if are not asserted. Consider adding a timeout mechanism to handle such scenarios and prevent potential deadlocks.

Copilot

Pull Request Overview

This PR introduces a new transaction interface (tuner_txn_if) for tuning operations with a combined request/response handshake pattern, replacing the previous arbiter-based approach. The changes simplify the controller logic by consolidating micro handshaking into a single adapter module.

Adds a new tuner_txn_if interface with val/rdy handshake semantics
Implements tuner_ctrl_txn_adapter to bridge between new interface and existing arbiter logic
Refactors tuner_search_phy to use the simplified transaction interface

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
lib/verilog/tuner/tuner_txn_if.sv	New transaction interface with handshake protocol and modports
lib/verilog/tuner/tuner_ctrl_txn_adapter.sv	Adapter module implementing state machine to bridge interfaces
lib/verilog/tuner/tuner_search_phy.sv	Refactored to use new transaction interface, removing complex state logic
lib/verilog/tuner/tuner_phy.sv	Integration of new interface and adapter into existing hierarchy

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-08-26T05:01:33Z

lib/verilog/tuner/tuner_ctrl_txn_adapter.sv

+);
+  import tuner_phy_pkg::*;
+
+  typedef enum logic [1:0] {IDLE, WAIT_TUNE, WAIT_COMMIT, RESP} state_e;


The state enumeration uses 4 states but only declares 2 bits, which limits future extensibility. Consider using logic [2:0] to provide room for additional states or use an explicit width declaration that matches the actual number of states needed.

Suggested change

typedef enum logic [1:0] {IDLE, WAIT_TUNE, WAIT_COMMIT, RESP} state_e;

typedef enum logic [2:0] {IDLE, WAIT_TUNE, WAIT_COMMIT, RESP} state_e;

Copilot · 2025-08-26T05:01:33Z

lib/verilog/tuner/tuner_search_phy.sv

+    end else if (search_refresh) begin
      ring_tune <= i_dig_ring_tune_start;
-    end
-    else if (tune_compute) begin
+    end else if (txn_if.fire()) begin


[nitpick] The else if statements should be formatted consistently. Line 260 uses end else if while line 262 uses end else if - consider using the same spacing pattern throughout the conditional chain.

Copilot · 2025-08-26T05:01:33Z

lib/verilog/tuner/tuner_search_phy.sv

+      txn_valid <= 1'b1;
+    end
+    else if (txn_if.fire()) begin
+      txn_valid <= ~search_active_done;


[nitpick] Using bitwise negation ~ on a single-bit signal is less clear than logical negation !. Consider using !search_active_done for better readability.

Suggested change

txn_valid <= ~search_active_done;

txn_valid <= !search_active_done;

Refine tuner transaction interface naming

fa05002

sunjin-choi added the codex label Jul 9, 2025 — with ChatGPT Codex Connector

gemini-code-assist bot reviewed Jul 9, 2025

View reviewed changes

sunjin-choi requested a review from Copilot August 25, 2025 10:06

This comment was marked as outdated.

Sign in to view

sunjin-choi requested a review from Copilot August 26, 2025 05:00

Copilot AI reviewed Aug 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Introduce tuner transaction interface #9

Introduce tuner transaction interface #9

Uh oh!

sunjin-choi commented Jul 9, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot Jul 9, 2025

Uh oh!

gemini-code-assist bot Jul 9, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Aug 26, 2025

Uh oh!

Copilot AI Aug 26, 2025

Uh oh!

Copilot AI Aug 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	typedef enum logic [1:0] {IDLE, WAIT_TUNE, WAIT_COMMIT, RESP} state_e;
	typedef enum logic [2:0] {IDLE, WAIT_TUNE, WAIT_COMMIT, RESP} state_e;

	txn_valid <= ~search_active_done;
	txn_valid <= !search_active_done;

Introduce tuner transaction interface #9

Are you sure you want to change the base?

Introduce tuner transaction interface #9

Uh oh!

Conversation

sunjin-choi commented Jul 9, 2025

Summary

Testing

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Aug 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 26, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants