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.

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

csr.wishbone: fix WishboneCSRBridge cycle counter. #13

Merged
merged 1 commit into from Mar 26, 2020

Conversation

jfng
Copy link
Member

@jfng jfng commented Mar 24, 2020

Before this commit, a CSR could be read two times in a single WB transaction.

Repro:

from nmigen import *
from nmigen.back.pysim import *

from nmigen_soc import csr
from nmigen_soc.memory import MemoryMap
from nmigen_soc.csr.wishbone import WishboneCSRBridge


csr_bus = csr.Interface(addr_width=2, data_width=8)
csr_bus.memory_map = MemoryMap(addr_width=2, data_width=8)

dut = WishboneCSRBridge(csr_bus, data_width=32)

def process():
    yield dut.wb_bus.adr.eq(0x0)
    yield dut.wb_bus.cyc.eq(1)
    yield dut.wb_bus.stb.eq(1)
    yield dut.wb_bus.sel.eq(0b0001)
    yield
    while not (yield dut.wb_bus.ack):
        yield
    yield dut.wb_bus.cyc.eq(0)
    yield dut.wb_bus.stb.eq(0)

sim = Simulator(dut)
sim.add_clock(1e-6)
sim.add_sync_process(process)
with sim.write_vcd("repro.vcd"):
    sim.run()

bad

After this commit, only one read occurs:

ok

@codecov
Copy link

codecov bot commented Mar 24, 2020

Codecov Report

Merging #13 into master will not change coverage by %.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #13   +/-   ##
=======================================
  Coverage   99.63%   99.63%           
=======================================
  Files           4        4           
  Lines         552      552           
  Branches      127      127           
=======================================
  Hits          550      550           
  Misses          1        1           
  Partials        1        1           
Impacted Files Coverage Δ
nmigen_soc/csr/wishbone.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 967a65f...c3af6bd. Read the comment docs.

@whitequark
Copy link
Member

Thanks. If it's not hard, could you add a test?

@jfng
Copy link
Member Author

jfng commented Mar 24, 2020

Done, I think. I believe that the bug was able to hide because the test suite deasserted STB on the same cycle as ACK, instead of 1 cycle later. See:

wb_bad

@jfng jfng merged commit 425692a into amaranth-lang:master Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants