Skip to content

QSPI_XIP_CTRL: Fix missing default assignment for HREADYOUT in st_rw state#4162

Open
ashnaaseth2325-oss wants to merge 1 commit intoThe-OpenROAD-Project:masterfrom
ashnaaseth2325-oss:fix/qspi-hreadyout-st-rw
Open

QSPI_XIP_CTRL: Fix missing default assignment for HREADYOUT in st_rw state#4162
ashnaaseth2325-oss wants to merge 1 commit intoThe-OpenROAD-Project:masterfrom
ashnaaseth2325-oss:fix/qspi-hreadyout-st-rw

Conversation

@ashnaaseth2325-oss
Copy link
Copy Markdown
Contributor

1. SUMMARY

This PR fixes a missing default assignment for HREADYOUT in the st_rw state, which could cause it to hold a stale value. The change ensures correct ready signaling when no active transfer is present.
Primary changes are in flow/designs/src/chameleon/IPs/QSPI_XIP_CTRL.v within the QSPI_XIP_CTRL module.


2. FIX

// Before
else if(HTRANS[1] & HSEL & HREADY & ~c_hit) HREADYOUT <= 1'b0;
// else HREADYOUT <= 1'b1;

// After
else if(HTRANS[1] & HSEL & HREADY & ~c_hit) HREADYOUT <= 1'b0;
else HREADYOUT <= 1'b1;

3. VERIFICATION

Simulated AHB transactions covering hit, miss, and idle conditions in st_rw. Previously, HREADYOUT could remain low after a stall; after the fix, it correctly returns to 1 when no transfer is active. Behavior is now consistent with protocol expectations and other FSM states.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request uncomments an assignment for the HREADYOUT signal in the st_rw state of the QSPI_XIP_CTRL module. The reviewer suggests adding a default case to the case statement to ensure HREADYOUT is driven to a safe state (1'b1) in the event of an undefined state, which prevents potential bus stalls.

st_rw : if(HTRANS[1] & HSEL & HREADY & c_hit) HREADYOUT <= 1'b1;
else if(HTRANS[1] & HSEL & HREADY & ~c_hit) HREADYOUT <= 1'b0;
//else HREADYOUT <= 1'b1;
else HREADYOUT <= 1'b1;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The case (state) statement (starting at line 90) is missing a default branch. Although the FSM state transitions are handled by a default value for nstate, the HREADYOUT signal is assigned within a clocked case block. It is a best practice in RTL design to include a default case for all control logic to ensure that signals are driven to a known safe state if the state register ever enters an undefined value (e.g., due to a glitch or SEU). For an AHB-Lite slave, the safe state for HREADYOUT is typically 1'b1 to prevent a permanent bus stall.

                            else HREADYOUT <= 1'b1;
                default :   HREADYOUT <= 1'b1;

@ashnaaseth2325-oss ashnaaseth2325-oss marked this pull request as draft April 18, 2026 18:23
In st_rw, HREADYOUT was not assigned when no active AHB transfer
occurred, causing it to hold a stale 0. Add the missing else clause
to drive HREADYOUT high, consistent with st_idle behavior.

Signed-off-by: ashnaaseth2325-oss <ashnaaseth2325@gmail.com>
Signed-off-by: Ashnaa Seth <ashnaaseth2325@gmail.com>
@ashnaaseth2325-oss ashnaaseth2325-oss force-pushed the fix/qspi-hreadyout-st-rw branch from 12892e1 to f53e97e Compare April 18, 2026 18:23
@ashnaaseth2325-oss ashnaaseth2325-oss marked this pull request as ready for review April 18, 2026 18:24
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