ecp5: Memory pattern of Async FIFOs not mapping to BRAM

[gregdavill]

I've found an interesting bug recently with one of my designs. It's fundamentally related to the migen.fhdl.special emitting verilog code for memory blocks, but starts back in LiteX. Basically after a commit in May 2021 Async FIFOs created by LiteX may not be inferred to BRAM.

This is an intentional change on the Yosys front: YosysHQ/yosys#2965

This issue is mostly so others using LiteX have a workaround until this could be fixed in migen.

Example

Consider a large Async FIFO in LiteX

self.submodules.fifo = ClockDomainsRenamer({"read": "video","write": "sys"})(stream.AsyncFIFO([('data', 24)], 1024))

This becomes the following in the generated verilog

reg [25:0] storage_4[0:1023];
reg [9:0] memadr_10;
reg [9:0] memadr_11;
always @(posedge sys_clk) begin
	if (soc_asyncfifo_wrport_we)
		storage_4[soc_asyncfifo_wrport_adr] <= soc_asyncfifo_wrport_dat_w;
	memadr_10 <= soc_asyncfifo_wrport_adr;
end

always @(posedge video_clk) begin
	memadr_11 <= soc_asyncfifo_rdport_adr;
end

assign soc_asyncfifo_wrport_dat_r = storage_4[memadr_10];
assign soc_asyncfifo_rdport_dat_r = storage_4[memadr_11];

The pattern of having two clock domains, and registering memadr_11 on video_clk and the async read is what is causing issues.

It's been recommended (YosysHQ/yosys#2965) to switch this pattern to the following, note the reads are happening synchronously now.

reg [25:0] storage_4[0:1023];
reg [25:0] memdat_15;
reg [25:0] memdat_16;
always @(posedge sys_clk) begin
	if (soc_asyncfifo_wrport_we)
		storage_4[soc_asyncfifo_wrport_adr] <= soc_asyncfifo_wrport_dat_w;
	memdat_15 <= storage_4[soc_asyncfifo_wrport_adr];
end

always @(posedge video_clk) begin
	memdat_16 <= storage_4[soc_asyncfifo_rdport_adr];
end

assign soc_asyncfifo_wrport_dat_r = memdat_15;
assign soc_asyncfifo_rdport_dat_r = memdat_16;

Workaround

This is a small patch for migen.fhdl.special is working for me, but might be a bit too broad in it's selection for changing patterns, since as you can see from the above example it's also switching the sys_clk.read_port to a READ_FIRST mode.

diff --git a/migen/fhdl/specials.py b/migen/fhdl/specials.py
index 9344087..d1e3c78 100644
--- a/migen/fhdl/specials.py
+++ b/migen/fhdl/specials.py
@@ -330,6 +330,12 @@ class Memory(Special):
 
         adr_regs = {}
         data_regs = {}
+
+        clocks = [port.clock for port in memory.ports]
+        if clocks.count(clocks[0]) != len(clocks):
+            for port in memory.ports:
+                port.mode = READ_FIRST
+
         for port in memory.ports:
             if not port.async_read:
                 if port.mode == WRITE_FIRST:

[enjoy-digital]

Thanks for reporting it and the suggested fix @gregdavill, I'll look at it. In the long term I'd like LiteX to handle the FIFO/Ram implementations for several reasons:

• To remove the current limitations.
• To ease the use of PSRAM on iCE40, UltraRAM on Ultrascale, etc...
• And also to have more flexibility to update/fix this kind of issues.

Until this is done or Migen is updated, we can probably have a custom implementation of this in `litex.gen. I'll try to reproduce the issue and integrate your changes.

[enjoy-digital]

@gregdavill: Your workaround has been integrated with 8fbd1b8 and fd354c5. Can you do a test to verify it's behaving correctly for your use case?

Having the verilog memory generation now directly in LiteX will simplify specializing the memories and start progressively replace Migen's Memory.

[gregdavill]

Yep, this is now working for me.

Thanks!