Skip to content

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 jfng force-pushed the fix-csr-wb-bridge branch from 9b70b70 to c3af6bd Compare March 24, 2020 18:21
@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.

2 participants