Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix broken msg passing in wishbone master for (fix #692) #693

Merged
merged 2 commits into from
Nov 23, 2020
Merged

fix broken msg passing in wishbone master for (fix #692) #693

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bc80ea4
Add reproducer for #692
Nov 12, 2020
91b7790
Push wishbone master req msg one cycle earlier
Nov 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions vunit/vhdl/verification_components/run.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def encode(tb_cfg):


def gen_wb_tests(obj, *args):
for dat_width, num_cycles, strobe_prob, ack_prob, stall_prob in product(*args):
for dat_width, num_cycles, strobe_prob, ack_prob, stall_prob, slave_inst in product(*args):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would use slave_comb instead of slave_inst, since my understanding is that the difference between the two cases is whether the slave is combinatorial or registered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is to have possibility to skip wb slave VC instantiation and drive wishbone slave signals directly. It makes possible to test various corner cases - like here when specific set of acknowledge logic lead to crash on modelsim, but not ghdl. I thinks it's not feasible to add this corner cases reproducers to wishbone slave as it will obfuscate code. This is all about wb master VC bug.

tb_cfg = dict(
dat_width=dat_width,
# TODO remove fixed addr
Expand All @@ -31,6 +31,7 @@ def gen_wb_tests(obj, *args):
ack_prob=ack_prob,
stall_prob=stall_prob,
num_cycles=num_cycles,
slave_inst=slave_inst
)
config_name = encode(tb_cfg)
obj.add_config(name=config_name, generics=dict(encoded_tb_cfg=encode(tb_cfg)))
Expand Down Expand Up @@ -86,13 +87,17 @@ def gen_avalon_master_tests(obj, *args):

for test in TB_WISHBONE_SLAVE.get_tests():
# TODO strobe_prob not implemented in slave tb
gen_wb_tests(test, [8, 32], [1, 64], [1.0], [0.3, 1.0], [0.4, 0.0])
gen_wb_tests(test, [8, 32], [1, 64], [1.0], [0.3, 1.0], [0.4, 0.0], [True, ])


TB_WISHBONE_MASTER = LIB.test_bench("tb_wishbone_master")

for test in TB_WISHBONE_MASTER.get_tests():
gen_wb_tests(test, [8, 32], [1, 64], [0.3, 1.0], [0.3, 1.0], [0.4, 0.0])
if test.name == "slave comb ack":
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure to understand this. Why is the combinatorial version tested with one set of params but the other one is tested with two?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

slave comb ack test case doesn't need/cannot have random stimulus

gen_wb_tests(test, [32], [64], [1.0], [1.0], [0.0], [False, ])
else:
gen_wb_tests(test, [8, 32], [1, 64], [0.3, 1.0], [0.3, 1.0], [0.4, 0.0], [True, ])


TB_AXI_STREAM = LIB.test_bench("tb_axi_stream")

Expand Down
3 changes: 1 addition & 2 deletions vunit/vhdl/verification_components/src/wishbone_master.vhd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,11 +91,10 @@ begin
-- TODO why sel is not passed in msg for reading (present for writing)?
--sel <= pop_std_ulogic_vector(request_msg);
end if;
push(acknowledge_queue, request_msg);
wait until rising_edge(clk) and stall = '0';
stb <= '0';

push(acknowledge_queue, request_msg);

elsif msg_type = wait_until_idle_msg then
if cycle then
wait until not cycle;
Expand Down
53 changes: 36 additions & 17 deletions vunit/vhdl/verification_components/test/tb_wishbone_master.vhd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ architecture a of tb_wishbone_master is
strobe_prob : real;
ack_prob : real;
stall_prob : real;
slave_inst : boolean;
end record tb_cfg_t;

impure function decode(encoded_tb_cfg : string) return tb_cfg_t is
Expand All @@ -43,7 +44,8 @@ architecture a of tb_wishbone_master is
num_cycles => positive'value(get(encoded_tb_cfg, "num_cycles")),
strobe_prob => real'value(get(encoded_tb_cfg, "strobe_prob")),
ack_prob => real'value(get(encoded_tb_cfg, "ack_prob")),
stall_prob => real'value(get(encoded_tb_cfg, "stall_prob")));
stall_prob => real'value(get(encoded_tb_cfg, "stall_prob")),
slave_inst => boolean'value(get(encoded_tb_cfg, "slave_inst")));
end function decode;

constant tb_cfg : tb_cfg_t := decode(encoded_tb_cfg);
Expand Down Expand Up @@ -156,6 +158,11 @@ begin
wait for 20 ns;
end loop;

elsif run("slave comb ack") then
write_bus(net, bus_handle, 0, value);
wait until ack = '1' and rising_edge(clk);
wait for 20 ns;

end if;

info(tb_logger, "Done, quit...");
Expand All @@ -182,22 +189,34 @@ begin
ack => ack
);

dut_slave : entity work.wishbone_slave
generic map (
wishbone_slave => wishbone_slave
)
port map (
clk => clk,
adr => adr,
dat_i => dat_o,
dat_o => dat_i,
sel => sel,
cyc => cyc,
stb => stb,
we => we,
stall => stall,
ack => ack
);
slave_gen : if tb_cfg.slave_inst generate
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense for comb to be an option (generic) of the wishbone slave, instead of using a generate in the testbench?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as described in first comment IMHO: no, it would not

dut_slave : entity work.wishbone_slave
generic map (
wishbone_slave => wishbone_slave
)
port map (
clk => clk,
adr => adr,
dat_i => dat_o,
dat_o => dat_i,
sel => sel,
cyc => cyc,
stb => stb,
we => we,
stall => stall,
ack => ack
);
else generate
signal wr_r : std_ulogic;
begin
proc : process(clk) is begin
if rising_edge(clk) then
wr_r <= we and cyc and stb;
end if;
end process;
ack <= wr_r and not stall and cyc;
stall <= not wr_r;
end generate;

clk <= not clk after 5 ns;

Expand Down