axi_burst_unwrap: Remove overly pessimistic assertion

[Scheremo]

This PR fixes a bug in the axi_burst_unwrap module, where the id_queue managing a write burst's outstanding W beats receives decrement requests even if the W channel transfer was not handshaked.

[Scheremo]

I think the same technical bug is in the handling of B channel responses, but it practically never triggers because most AXI modules in this repo always keep b_ready high. If we can get this change reviewed for the W channel, I can also fix it for the B channel.

[niwis]

Hi, thanks a lot for pointing this out! However, there is a path from w_cnt_req to w_cnt_gnt through the id_queue module, which means that the proposed change introduces a loop. Would moving w_cnt_dec into the handshake condition instead of w_cnt_req solve the issue?

[Scheremo]

Hey Nils, You're right about the loop between req and gnt, I didn't notice this before.
I'll check your proposed solution on my end, but it should be "more correct" :).

[Scheremo]

Okay, so this looked better to begin with, until I figured out the real real issue at hand.
Since w_cnt_req is only gated by w_valid, it might be issued when a master is trying (unsuccessfully) to launch a new W transaction, i.e. aw_valid and w_valid are both high, but aw_ready is not. In this case, the assertion further down in the code triggers, since the id_queue gnt is given, without the data being valid, because the w_valid used for issuing the request stems from a transaction that has not yet been handshaked.
I'm thinking maybe instead of

      WReady: begin
        if (act_req.w_valid) begin

We need

      WReady: begin
        if (act_req.w_valid & ("We are not in a half-way AW handshake" )) begin

Since the id_queue takes a cycle to latch a new transaction anyway. What do you think, @niwis ?

EDIT: Alternatively I think we can also disable the assertion further down in the code, as this case seems valid to me.

[Scheremo]

Okay, so this looked better to begin with, until I figured out the real real issue at hand. Since w_cnt_req is only gated by w_valid, it might be issued when a master is trying (unsuccessfully) to launch a new W transaction, i.e. aw_valid and w_valid are both high, but aw_ready is not. In this case, the assertion further down in the code triggers, since the id_queue gnt is given, without the data being valid, because the w_valid used for issuing the request stems from a transaction that has not yet been handshaked. I'm thinking maybe instead of

      WReady: begin
        if (act_req.w_valid) begin

We need

      WReady: begin
        if (act_req.w_valid & ("We are not in a half-way AW handshake" )) begin

Since the id_queue takes a cycle to latch a new transaction anyway. What do you think, @niwis ?

EDIT: Alternatively I think we can also disable the assertion further down in the code, as this case seems valid to me.

I think I solved the underlying issue now; by introducing an cnt_empty_o signal to the burst counters, this PR avoids requesting burst counts of outstanding write transactions iff no write transactions are in flight.

If I understand correctly, functionally this only causes a change if the AXI slave of this module handshakes w_ready without a write transaction in flight, as in this case invalid data might be passed through. This is legal behaviour as far as I interpret the AXI spec, but is not commonly done in PULP AXI IPs.

The edge case this change prevents is the w counter being requested if there are no more outstanding transactions queued up, and the AW Channel handshake for the next transaction has not been done, but the w_valid for the first beat of the next transaction on the W channel is already asserted. Adding this change helps avoid the warning "Invalid output at ID queue, read not granted!", since now the FSM awaits that the w counter is non-empty.

[niwis]

I think I understand the issue now. You're absolutely right that the AW beat is required to process the W/B streams, and that there should be backpressure on W/B while waiting for the AW handshake. From my recollection, this is the purpose of cnt_gnt: If the ID queue does not contain an entry for the requested id (yet), the W/B channels are stalled. Since the validity of the ID queues output is checked with cnt_gnt, I don't see the need for the assertion and think that it can be safely removed.

While I like the idea of checking the counters before accessing the ID queue, I see two potential hazards:

1. The fact that any counter is allocated does not necessarily mean that the AW handshake of the current transaction has occurred (a counter could be allocated to another ID, triggering the same assertion for the requested ID. Although W beats are ordered and B beats are unlikely to arrive before their AW, but who knows?).
2. Would this possibly add a cycle of delay if AW and W arrive simultaneously, since we now need to wait for the counter update?

tl;dr: It seems to me like the preferred solution is removing the assertion, which I don't think is necessary here. At the same time, I don't think that this change breaks anything, so I'm not opposed to it if I'm missing something :)

[Scheremo]

I agree with @niwis on this - all preconditions for forwarding W beats are correct in the current version. The assertion, however, is overly pessimistic.
As the minimal change to get to a state where no erroneous assertions are triggered, we should remove the assertion rather than change the module's behaviour.