Hi Sergey,
I've only just started using this in earnest, sorry for not getting back
to you sooner. It's a nice encapsulation of the MPMC/SDMA functionality,
thanks for posting it.
In order to integrate this into my system, I refactored the bus
attachment code and added hooks for platform bus. I also removed some
dead code, reformatted some things to satisfy checkpatch, tweaked
#includes to fix Microblaze compilation, and fixed a potential bug where
sdma_set_coalesce() could return without releasing a spinlock. I also
optimized the sdma_desc_* functions by moving any byte swapping from
runtime to compile-time.
Some more controversial changes / items for discussion:
1. I dropped setting the tail descriptor in the sdma_[rt]x_init()
functions since that would start DMA, which is not what I think we want.
2. I made RX and TX interrupts optional. There are use cases (DMAing
while atomic) in which interrupts are not necessary. The DMA engine only
needs RX interrupts. There is an (obscure) mode in which it might also
want TX interrupts, and in that case it's only interested in error
interrupts - normal "done" interrupts are of no interest whatsoever.
Rather than try to adapt the sdma driver to fit that case, I think I
will drop that mode from the DMA engine driver.
2A. I will need, but haven't added yet, methods to know if a SDMA
channel has RX and TX IRQ resources. I'm assuming that a simple inline
accessor is preferred over snooping struct sdma directly.
3. I changed the user[4] field of struct sdma_desc to individually-named
fields app1 - app4, to match the MPMC datasheet. I found user[0]
confusing and already had to fix a bug where I had coded user[0]
thinking it was app0, when I really should have specified stat_ctl.
4. Why have sdma_[rt]x_submit() return a value if it is always zero?
5. I would like to see the 'virt' and 'flags' fields removed from struct
sdma_desc and SDMA_ALIGNMENT reduced from 0x40 to 0x20. Neither field is
used in the sdma driver itself. I understand why 'virt' is there, but
having it in the struct will make the DMA engine driver less efficient.
Because the DMA engine operates on 'loopback' SDMA channels it always
allocates descriptors in pairs. Also the DMA engine framework already
provides storage for the 'virt' pointer. Having a larger-than-necessary
structure would force the DMA engine to do larger allocations from its
DMA pool - instead of 64 bytes per dual descriptor, it would have to
allocate 128.
6. I'm concerned that there is no concept of "allocating" a channel,
something like a sdma_device_get() / sdma_device_put() pair that would
prevent concurrent access to a SDMA device by removing the device from
consideration by sdma_find_device().
7. In that same vein, I'm curious about the need for a list of
sdma_clients. Is there a use case for this in your systems?
8. It would probably make sense to have sdma_init() fail with -EEXIST if
a SDMA device with the specified phandle already exists (-1 being an
exception).
9. I didn't resolve the issue of what to name the files / API, assuming
'sdma' is a little too generic for things that are now publicly visible.
If we have to change it, some suggestions are 'mpmcsdma' (long, but
precise), 'xildma', 'xsdma', or 'xdma' (also perhaps too generic).
As time permits, I'll work on refactoring the DMA engine driver to use
the sdma driver - I'll post change requests for anything else I need
rather than modifying the sdma code directly.
Regards,
------------------------------------------------------------------------
Steven J. Magnani "I claim this network for MARS!
http://www.digidescorp.com Earthling, return my space modulator!"
#include <standard.disclaimer>
On Tuesday 20 April 2010 20:29:55 Steven J. Magnani wrote:
> Hi Sergey,
>
> I've only just started using this in earnest, sorry for not getting back
> to you sooner. It's a nice encapsulation of the MPMC/SDMA functionality,
> thanks for posting it.
>
> In order to integrate this into my system, I refactored the bus
> attachment code and added hooks for platform bus. I also removed some
> dead code, reformatted some things to satisfy checkpatch, tweaked
> #includes to fix Microblaze compilation, and fixed a potential bug where
> sdma_set_coalesce() could return without releasing a spinlock. I also
> optimized the sdma_desc_* functions by moving any byte swapping from
> runtime to compile-time.
Well, it looks good.
>
> Some more controversial changes / items for discussion:
>
> 1. I dropped setting the tail descriptor in the sdma_[rt]x_init()
> functions since that would start DMA, which is not what I think we want.
>
Needs some testing, I think. Back in 2008, AFAIR, I've had some problems with
this approach, but I don't remember exactly if it was the cause.
> 2. I made RX and TX interrupts optional. There are use cases (DMAing
> while atomic) in which interrupts are not necessary. The DMA engine only
> needs RX interrupts. There is an (obscure) mode in which it might also
> want TX interrupts, and in that case it's only interested in error
> interrupts - normal "done" interrupts are of no interest whatsoever.
> Rather than try to adapt the sdma driver to fit that case, I think I
> will drop that mode from the DMA engine driver.
Looks good too.
>
> 2A. I will need, but haven't added yet, methods to know if a SDMA
> channel has RX and TX IRQ resources. I'm assuming that a simple inline
> accessor is preferred over snooping struct sdma directly.
I would suggest sdma_has_[r|t]x().
>
> 3. I changed the user[4] field of struct sdma_desc to individually-named
> fields app1 - app4, to match the MPMC datasheet. I found user[0]
> confusing and already had to fix a bug where I had coded user[0]
> thinking it was app0, when I really should have specified stat_ctl.
It doesn't really matter as these fields have different meaning in different
applications and one has to decode it appropriately. If it helps to write more
understandable code, so be it.
>
> 4. Why have sdma_[rt]x_submit() return a value if it is always zero?
I don't remember exactly why I've coded this, but now I think that return
value isn't needed for these functions too.
>
> 5. I would like to see the 'virt' and 'flags' fields removed from struct
> sdma_desc and SDMA_ALIGNMENT reduced from 0x40 to 0x20. Neither field is
> used in the sdma driver itself. I understand why 'virt' is there, but
> having it in the struct will make the DMA engine driver less efficient.
> Because the DMA engine operates on 'loopback' SDMA channels it always
> allocates descriptors in pairs. Also the DMA engine framework already
> provides storage for the 'virt' pointer. Having a larger-than-necessary
> structure would force the DMA engine to do larger allocations from its
> DMA pool - instead of 64 bytes per dual descriptor, it would have to
> allocate 128.
The 'virt' and 'flags' fields are there specially for users. The 'virt' is
intended for the pointer to the structure associated with the buffer (maybe,
'virt' should be called 'priv' instead), and 'flags' is there to determine
which data type 'virt' is pointing to.
>
> 6. I'm concerned that there is no concept of "allocating" a channel,
> something like a sdma_device_get() / sdma_device_put() pair that would
> prevent concurrent access to a SDMA device by removing the device from
> consideration by sdma_find_device().
>
> 7. In that same vein, I'm curious about the need for a list of
> sdma_clients. Is there a use case for this in your systems?
>
I've added the list of clients rather recently to support several of my
drivers which implement the separate descriptor rings and char devices for RX
and TX (there are some custom IP cores for DSP developed by our company which
need this functionality).
> 8. It would probably make sense to have sdma_init() fail with -EEXIST if
> a SDMA device with the specified phandle already exists (-1 being an
> exception).
Maybe this check is needed but 'linux,phandle' is an automatic property added
by the device tree compiler and I doubt that the FDT code with invalid
phandles will even compile.
>
> 9. I didn't resolve the issue of what to name the files / API, assuming
> 'sdma' is a little too generic for things that are now publicly visible.
> If we have to change it, some suggestions are 'mpmcsdma' (long, but
> precise), 'xildma', 'xsdma', or 'xdma' (also perhaps too generic).
>
Maybe, 'xllsdma' would be good.
> As time permits, I'll work on refactoring the DMA engine driver to use
> the sdma driver - I'll post change requests for anything else I need
> rather than modifying the sdma code directly.
>
Feel free to contact me directly or through the mailing list.
> Regards,
> ------------------------------------------------------------------------
> Steven J. Magnani "I claim this network for MARS!
> http://www.digidescorp.com Earthling, return my space modulator!"
>
> #include <standard.disclaimer>
>
Regards, Sergey Temerkhanov,
Cifronic ZAO.
--
Regards, Sergey Temerkhanov,
Cifronic ZAO