Incorrect behavior in case of HRESP=1

[Thomasb81]

Hello

Regarding the testcase:

DUT=ahb_template SIM=icarus TIMEPREC=1ps TIMEUNIT=1ns WAVES=1 RANDOM_SEED=1 pytest tests/test_ahb_lite_sram.py::test_ahb_lite_sram[data_width0]

On the waveform we can observe that slave_hresp is high for a single cycle. This behavior does not match with spec.

AHB spec (ARM IHI 0033B.b) chapter 5.1. In case of error mention that HRESP is driven high for 2 cycles, HREADYOUT is low on first cycle and high on second. On last cycle of HRESP high, HTRAN must be drive to IDLE.

The waveform provided in the spec depict a slave wait state before returning the error.

To be note that in case of support of exclusive access (HEXCL / ahb5) , the HEXOKAY signal should also be handle.

[Thomasb81]

I am sorry, the fix is incorrect.

HTRAN should be IDLE when slave_hready = 1 and slave_hresp=1. but I see HTRANS put idle when slave_hready = 0 and slave_hresp=1. Also 1 slave wait state has been added, is that expected ?

[aignacio]

Hey @Thomasb81, can you try to get the latest version (v0.1.9)? on my side I don't see this issue both verilator / icarus, as you can see below:

If you are running icarus, try to add something like below at the monitor fork.

if self.bus.htrans.value.is_resolvable and self.bus.hready.value.is_resolvable:
                if self.bus.htrans.value == 0 and self.bus.hready == 0:
                    raise AssertionError("AHB PROTOCOL VIOLATION - TEST")

[Thomasb81]

I should have mix something...

The waveform show the second transaction that return an error.

icarus:

With icarus HTRANS never go to IDLE, it take 2'bzz

verilator:

[aignacio]

technically you should see htrans going from htrans = NSEQ to htrans = IDLE only if there is a pipeline transaction ongoing, on pip_mode = False, there's no overlap.

[Thomasb81]

The point is with an rtl dut,

My first write access at address 0x4 of the value 0x1 does not work ... I have to dis-align clock and bus data...

[aignacio]

Sorry, didn't understand your point, are you saying there's an issue with your design? maybe you can share the RTL.

[Thomasb81]

Yes I design a register decoder. Actually I can't share the code.

Here is a part of rtl code.

wire trans_req = hready & hsel & htrans[1];
wire ahb_read_req = trans_req & ~hwrite;
wire ahb_write_req = trans_req & hwrite;

always @(posedge hclk or negedge hresetn) begin : delay_haddr
    if (~hresetn) begin
        addr_reg <= {(ADDR_WIDTH){1'b0}}; 
    end
    else if (ahb_write_req|ahb_read_req) begin
        addr_reg <= haddr[ADDR_WIDTH-1:0];
    end
    else begin
        addr_reg <= {(ADDR_WIDTH){1'b0}}; 
    end
end

    always @(posedge hclk or negedge hresetn) begin : delay_write
        if (~hresetn) begin
            write_en_reg <= 1'b0;
        end
        else if (ahb_write_req) begin
            write_en_reg  <= ahb_write_req;
    end
    else begin
        write_en_reg <= 1'b0;   
    end
    end

assign addr = addr_reg[ADDR_WIDTH-1:0];

delay_write work but delay_haddr don't... Because there is nothing at offset 0x0, the error at 1 signal manage hreadyout and resp to toggle for the error response.

Snippet of my test:

    ahb_lite_master = AHBLiteMaster(
        AHBBus.from_prefix(dut,
                           prefix=None,

                          ),
        dut.hclk,
        dut.hresetn,
        def_val="0",
    )
    resp = await ahb_lite_master.write(0x4, 0x1)
    assert resp[0]['resp'] == AHBResp.OKAY

verilator is even worst, none othe clocked process work.

[Thomasb81]

technically you should see htrans going from htrans = NSEQ to htrans = IDLE only if there is a pipeline transaction ongoing, on pip_mode = False, there's no overlap.

I disagree with that.

it is mandatory to complete the error transfert end with an IDLE to "flush" the pipeline ...

[aignacio]

Hey @Thomasb81, let me clarify my point here, as you highlighted there, it's written the following: "...two-cycle response provides sufficient time for the master to cancel this next access and drive HTRANS[1:0] to IDLE before the start of the next transfer" Thus, moving HTRANS from NSEQR to IDLE is just to indicate the operation B in the image below is cancelled, as also mentioned by the specification, this is due to the pipeline nature of the AHB protocol once by the time the slave set HREADY == HIGH it is already accepting the address phase B.

As I said, for 2x or more transactions with pipeline mode set (pip=True), this is required to cancel the operation B (which will retry again after), however, for non-pipeline transactions there is no point on changing HTRANS once address phase and data phase are not overlapping. Another way of thinking this is the following, if I have a single AHB transaction where the slave returns error, the data phase could have all floating the MOSI (master output slave input) signals but HWDATA, as the only one that matters is the HWDATA (in case of writes). Hope this helps to clarify.

[aignacio]

I was reviewing again the ARM's forum and actually the first wait state (T1-T2) is not mandatory. Currently the AHB slave is behaving like the ARM's waveforms but in the future I can revert back to remove this wait state as the only obligation from a slave PoV is the 2-cycle error response (1st HREADY==LOW && HRESP == ERROR 2nd HREADY==HIGH && HRESP == ERROR).

[Thomasb81]

I was reviewing again the ARM's forum and actually the first wait state (T1-T2) is not mandatory. Currently the AHB slave is behaving like the ARM's waveforms but in the future I can revert back to remove this wait state as the only obligation from a slave PoV is the 2-cycle error response (1st HREADY==LOW && HRESP == ERROR 2nd HREADY==HIGH && HRESP == ERROR).

That's what the comment below the diagram say:

See chapter 3.6 dedicated to wait state.

[aignacio]

I was reviewing again the ARM's forum and actually the first wait state (T1-T2) is not mandatory. Currently the AHB slave is behaving like the ARM's waveforms but in the future I can revert back to remove this wait state as the only obligation from a slave PoV is the 2-cycle error response (1st HREADY==LOW && HRESP == ERROR 2nd HREADY==HIGH && HRESP == ERROR).

That's what the comment below the diagram say:

See chapter 3.6 dedicated to wait state.

Exactly, it not mandatory but it's implementation specific. Also, I reviewed some ARM's code/docs and seems to be aligned.

[aignacio]

Hello

Regarding the testcase:

DUT=ahb_template SIM=icarus TIMEPREC=1ps TIMEUNIT=1ns WAVES=1 RANDOM_SEED=1 pytest tests/test_ahb_lite_sram.py::test_ahb_lite_sram[data_width0]

On the waveform we can observe that slave_hresp is high for a single cycle. This behavior does not match with spec.

AHB spec (ARM IHI 0033B.b) chapter 5.1. In case of error mention that HRESP is driven high for 2 cycles, HREADYOUT is low on first cycle and high on second. On last cycle of HRESP high, HTRAN must be drive to IDLE.

The waveform provided in the spec depict a slave wait state before returning the error.

To be note that in case of support of exclusive access (HEXCL / ahb5) , the HEXOKAY signal should also be handle.

Coming back to the first message, the only thing wrong I denote is the HRESP == OKAY during the first cycle of the two cycle response. Anyway, I already change the VIP response and current solution is spec aligned.

[Thomasb81]

By the way https://github.com/aignacio/cocotbext-ahb/blob/ef7bb85ddf6dc5be87fee67e05dbcb9b3801a480/cocotbext/ahb/ahb_master.py#L30

This does not make sense. VIP should alway drive a valid value. When highz is involve in logic with 4 states value (0,1,z,x) it leads to x that propagate.

On AHB it makes non-sence to drive signal at highz.

[aignacio]

Sorry @Thomasb81 but I disagree with your comment based on my personal experience, commercial VIPs from different vendors such as Cadence, Synospsys... have all default values to either Z / X intentionally to help to diagnose designs with bugs when X / Z is propagated. It only drive values when it is mandatory/required to, and a proper design should not sample input values when it is not suppose to.

[Thomasb81]

Coming back to the first message, the only thing wrong I denote is the HRESP == OKAY during the first cycle of the two cycle response. Anyway, I already change the VIP response and current solution is spec aligned.

Yes, but has the AHBLiteSlave has a bac-pressure mechanism that control hreadyout output (hreadyout on Slave of figure 4-2). The AHBLiteMaster should have a similar mechanism to control hready output (hready on Master and Slave of figure 4-2) This mechanism is currently missing, unless we should use the custom method for this purpose ?

Sorry @Thomasb81 but I disagree with your comment based on my personal experience, commercial VIPs from different vendors such as Cadence, Synospsys... have all default values to either Z / X intentionally to help to diagnose designs with bugs when X / Z is propagated. It only drive values when it is mandatory/required to, and a proper design should not sample input values when it is not suppose to.

I can agree with that. But due to pipe line nature of this protocol, some key control signal can't take 'z'. like hsel and htrans, that are use to decode a starting address-phase. Currently when we simulate with icarus with AHBLiteMaster and def_val="Z", X are propagde during the dataphase, if a single transfert is request.

[aignacio]

Coming back to the first message, the only thing wrong I denote is the HRESP == OKAY during the first cycle of the two cycle response. Anyway, I already change the VIP response and current solution is spec aligned.

Yes, but has the AHBLiteSlave has a bac-pressure mechanism that control hreadyout output (hreadyout on Slave of figure 4-2). The AHBLiteMaster should have a similar mechanism to control hready output (hready on Master and Slave of figure 4-2) This mechanism is currently missing, unless we should use the custom method for this purpose ?

Sorry @Thomasb81 but I disagree with your comment based on my personal experience, commercial VIPs from different vendors such as Cadence, Synospsys... have all default values to either Z / X intentionally to help to diagnose designs with bugs when X / Z is propagated. It only drive values when it is mandatory/required to, and a proper design should not sample input values when it is not suppose to.

I can agree with that. But due to pipe line nature of this protocol, some key control signal can't take 'z'. like hsel and htrans, that are use to decode a starting address-phase. Currently when we simulate with icarus with AHBLiteMaster and def_val="Z", X are propagde during the dataphase, if a single transfert is request.

For the first point I agree there should be a way to control hready_in from the master side. As this is not a mandatory signal for master and it is associated with the bus matrix interconnect to the slave, I decided to drive it high by default, typically the interconnect will mirror the slave hreadyout but I did not want to model this as it makes more sense to create a different class (maybe AHBLiteMasterInterconnect). For now what I would suggest is to implement outside the VIP (throughout your testbench, using force -> dut.hready_in ) this behavior as you intend once it would take me some work to have it fully working, i.e model interconnect broadasting slave back-pressure.

And on the second point, it is hard to comment without checking waveforms or the entire design, the VIP is suppose to only drive HWDATA with something during data phase, otherwise something is not correct. I understand you cannot share your code but feel free to reach me out by e-mail if I can help you somehow (anderson@aignacio.com). :thumbsup: