enjoy-digital / litedram

Small footprint and configurable DRAM core
Other
365 stars 115 forks source link

LiteDRAMDMAWriter sinks data when not enabled #356

Open DaveBerkeley opened 2 months ago

DaveBerkeley commented 2 months ago

The sink Stream in LiteDRAMDMAWriter has the 'ready' flag set by default.

If with_csr is set, shouldn't the 'sink.ready' flag only be set if the 'enable' bit is set in the CSR?

At the moment you can connect a Stream to the DMA and it will accept data even when the DMA is not enabled. This seems wrong.

I want to be able to set the 'base'/'length' of the transfer, pointing to an address in DRAM. Then set 'enable'. It would also be good to be able to use the sink Stream's 'last' flag to terminate the transfer and generate an event. So you can wait for a packet to complete, without having to know in advance the number of bytes to be transferred. The transfer should stop & generate an event when the 'last' word is transferred, or the 'offset' number of words have been transferred, whichever happens first.

The logic can be fixed as follows :

diff --git a/litedram/frontend/dma.py b/litedram/frontend/dma.py
index 3f31455..f367013 100644
--- a/litedram/frontend/dma.py
+++ b/litedram/frontend/dma.py
@@ -251,16 +251,16 @@ class LiteDRAMDMAWriter(Module, AutoCSR):
         self.submodules.fsm = fsm
         self.comb += fsm.reset.eq(~self._enable.storage)
         fsm.act("IDLE",
-            self.sink.ready.eq(1),
+            self.sink.ready.eq(0),
             NextValue(offset, 0),
             NextState("RUN"),
         )
         fsm.act("RUN",
             self._sink.valid.eq(self.sink.valid),
-            self._sink.last.eq(offset == (length - 1)),
+            self._sink.last.eq((offset == (length - 1)) | self.sink.last),
             self._sink.address.eq(base + offset),
             self._sink.data.eq(self.sink.data),
-            self.sink.ready.eq(self._sink.ready),
+            self.sink.ready.eq(self._sink.ready & self._enable.storage & ~self._done.status),
             If(self.sink.valid & self.sink.ready,
                 NextValue(offset, offset + 1),
                 If(self._sink.last,
DaveBerkeley commented 2 months ago

I've got a fix for the first flag too. Not sure how to proceed. What would be a good way of testing the code? How should I go about writing unit tests for this?

In order to avoid breakages in other people's code it might be worth adding a defaulted parameter to the constructors of the LiteDRAMDMAReader/Writer classes.

I'm trying to write code that allows me to interface my Streams library https://github.com/DaveBerkeley/streams to the DMA interface of a LiteX SoC. The idea is to be able to send packets of data across a stream from RISC-V ram space and receive packets on a DMA Writer, with an interrupt when the packet completes. The first/last/ready flags all need some changes to enable this.

The aim is to make a vector processor in Amaranth HDL that can be driven by a pair of DMA controllers from the RISC-V.

DaveBerkeley commented 2 months ago

I'm working on a pull-request for this

https://github.com/DaveBerkeley/litedram/blob/issues_356/litedram/frontend/dma.py

It works well with my Amaranth / Streams interface. But I need a way to check backward compatibility.