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

LiteDRAMDMAWriter sinks data when not enabled #356

Open
DaveBerkeley opened this issue Apr 24, 2024 · 2 comments
Open

LiteDRAMDMAWriter sinks data when not enabled #356

DaveBerkeley opened this issue Apr 24, 2024 · 2 comments
Labels

Comments

@DaveBerkeley
Copy link

DaveBerkeley commented Apr 24, 2024

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
Copy link
Author

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants