This patch has the driver for the Passthrough DMA engines in AMD SoC. Changes to use the Generic DMA interface driver (https://reviews.freebsd.org/D25316) is removed for now.
Diff Detail
Diff Detail
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 32088 Build 29608: arc lint + arc unit
Event Timeline
Comment Actions
Thanks!
Some general comments: Usually, you would just put this in sys/dev/ptdma (and sys/modules/ptdma) rather than adding a directory level.
Questions: Is there a public datasheet for this part? Is it present in any shipped/public products?
Code style deviates quite strongly from https://www.freebsd.org/cgi/man.cgi?query=style&sektion=9 but I think there are bigger picture things to look at here first.
| sys/dev/dmaengine/ptdma/ptdma.c | ||
|---|---|---|
| 81–86 ↗ | (On Diff #73511) | Non-static prototypes should go in a header instead. |
| 177 ↗ | (On Diff #73511) | atomic_set doesn't do what you want, see below. |
| 360–361 ↗ | (On Diff #73511) | Why does this push through a second queue and context switch instead of just dispatching directly to the hardware? I think this is more complicated / slow than it needs to be. |
| 446 ↗ | (On Diff #73511) | This can just be an ordinary mutex. There's no need for it to be a spinlock. |
| 491–497 ↗ | (On Diff #73511) | You don't need a second queue here. You've already registered pt_interrupt_handler as an ithread, which runs in more or less normal context and can take ordinary mutexes. You don't need to defer work to an additional queue. |
| 938–943 ↗ | (On Diff #73511) | I don't think it's safe to set softc values and expect to see them in attach, as any drivers may be invoked to probe a device. Just do the PCI lookup again in attach, I think. |
| 963 ↗ | (On Diff #73511) | First, atomic_set doesn't do what you think — it ORs the value with 0. (That'd be atomic_store.) But second, this is unnecessary — you can rely on the softc being initially zeroed by the bus/device subsystem. |
Comment Actions
Thanks @cem for taking time to review the code.
Regarding the directory structure. During development I had the generic driver also. So, I placed this driver in sys/dev/dmaengine/ptdma. Now, that we have another thread running regarding that generic DMA driver, I have moved this driver to sys/dev/ptdma (and sys/modules/ptdma) as mentioned.
Currently, we don't have the specification for this device publicly available. If there is any question, I will try to give answers for the same. Please let me know.
Regarding style changes. I tried to develop the code considering most of the guidelines. I have gone through the code again and addressed the possible/missed style changes. Please let me know if you still see anything not correct.
| sys/dev/dmaengine/ptdma/ptdma.c | ||
|---|---|---|
| 81–86 ↗ | (On Diff #73511) | Removed here. Already having extern declarations in header file. |
| 177 ↗ | (On Diff #73511) | Removed. |
| 360–361 ↗ | (On Diff #73511) | This comment is not yet addressed. We have designed this way, so that the client doesn't wait for each request to complete before submitting the next request. Please let me know your comments here. Anyway, I will do some tests to see if we are seeing improvement if we remove the separate process to handle the request. This needs some changes to our test client as well. |
| 491–497 ↗ | (On Diff #73511) | Removed the intr_workqueue and directly calling ptdma_intr_work now. I am setting up the interrupt handler using regular bus_setup_intr only. But looks like it bus_setup_intr internally uses ithread constructs. Please correct me if I am wrong. If my understanding above is right, any interrupt handler registered using bus_setup_intr will run in normal context, rather than interrupt context? |
| 938–943 ↗ | (On Diff #73511) | Done the changes. But a question, the driver's softc is specific to this driver and will not be exposed to other drivers right? |
Comment Actions
- AMD Passthrough DMA Engine driver v2 patch
Addressed review comments and other minor changes
Comment Actions
Thank you for writing it!
Regarding the directory structure. During development I had the generic driver also. So, I placed this driver in sys/dev/dmaengine/ptdma. Now, that we have another thread running regarding that generic DMA driver, I have moved this driver to sys/dev/ptdma (and sys/modules/ptdma) as mentioned.
Currently, we don't have the specification for this device publicly available. If there is any question, I will try to give answers for the same. Please let me know.
If the specification can be made available, even under NDA, that would be helpful.
Regarding style changes. I tried to develop the code considering most of the guidelines. I have gone through the code again and addressed the possible/missed style changes. Please let me know if you still see anything not correct.
I can take a style pass later but just some examples: multiline comments should start with a line of just /*; function definitions (but not declarations) should have type/qualifiers on a line by itself, and then the function name is the first character of the next line. Eg.,
static int
my_foo(int a, void *p)
{| sys/dev/dmaengine/ptdma/ptdma.c | ||
|---|---|---|
| 360–361 ↗ | (On Diff #73511) | Surely multiple requests can be dispatched to the hardware without waiting for earlier requests to complete? The idea was to do dispatch in the caller context, but the completion is still asynchronous. If the hardware can only perform one operation at a time, then I’m not sure of its utility on any modern amd64 device. |
| 491–497 ↗ | (On Diff #73511) | Bus_setup_intr has two callbacks: “filter” (optional), which runs in interrupt context, and “ithread,” which run in normal context. Most drivers should use ithread-style interrupt handling. |
| 938–943 ↗ | (On Diff #73511) | Right. But the memory may be freed and reallocated between probe and attach, I think. (FreeBSD probe is a bidding process where multiple drivers are asked if they support a given bus device. Each driver sees an appropriate sized softc during probe. Only the “winning” driver attaches.) |
Comment Actions
Hi @cem, I am working with my team here to understand the NDA process. I will keep you updated here. To understand things better, Is there any specific purpose you are looking the spec for? Are you looking to use them in any specific products? If so, can you please give some more details.
If you prefer a mail communication on this subject, I would do that. Please let me know your opinion
| sys/dev/dmaengine/ptdma/ptdma.c | ||
|---|---|---|
| 360–361 ↗ | (On Diff #73511) | As per the current design, multiple request can be queued from the test application without waiting for the previous request to complete. But the queued requests are handled sequentially with in the DMA driver. Let me know your comments. |
| 491–497 ↗ | (On Diff #73511) | Thanks for clarifying |
| 938–943 ↗ | (On Diff #73511) | Thanks for clarifying. |
Comment Actions
Fine @cem. No issues.
Regarding the NDA process. Can you please give some details about the necessity of the spec? So, that I can take it to my team here. Are you looking the spec for review alone (or) do you look forward to use the same in any products?