[RFC] Xilinx MPMC SDMA subsystem
in [Kernel]
|
From: Steven J. Magnani on 29 Mar 2010 11:50 On Fri, 2010-03-26 at 17:53 -0600, Grant Likely wrote: > I've not got time to review this patch right now, but Sergey and > Steven, you both posted MPMC drivers on the same day; Steven on the > microblaze list and Sergey on the powerpc list. Can you two please > coordinate and figure out how to mork toward a single driver that will > meet both your needs? I don't want to have 2 drivers (3 if you count > the ll_temac driver) in mainline for the same hardware interface. > I don't think we'll end up with a single driver. A MPMC DMA Engine driver is useful only on "loopback" SDMA ports. Sergey's code looks like a nice generic interface to Xilinx SDMA HW that could be used by the xlldma and ll_temac drivers, for instance. Both of those will get smaller, but won't go away. For this to be useful to me, it would need to be located somewhere more accessible than arch/powerpc and it would need to have initialization methods that don't depend on OF. In my build I would have platform code that binds to the xlldma platform attachment, which would call Sergey's SDMA code to assign it the proper resources. Any objections to having Sergey's code live in drivers/dma, and putting sdma.h out in include/linux? Might need to tweak the file/function names some to head off namespace issues. Or is there some other strategy for managing Xilinx-related drivers common to both Microblaze and PowerPC? Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Grant Likely on 29 Mar 2010 12:00 On Mon, Mar 29, 2010 at 9:42 AM, Steven J. Magnani <steve(a)digidescorp.com> wrote: > On Fri, 2010-03-26 at 17:53 -0600, Grant Likely wrote: >> I've not got time to review this patch right now, but Sergey and >> Steven, you both posted MPMC drivers on the same day; Steven on the >> microblaze list and Sergey on the powerpc list. �Can you two please >> coordinate and figure out how to mork toward a single driver that will >> meet both your needs? �I don't want to have 2 drivers (3 if you count >> the ll_temac driver) in mainline for the same hardware interface. >> > > I don't think we'll end up with a single driver. A MPMC DMA Engine > driver is useful only on "loopback" SDMA ports. Sergey's code looks like > a nice generic interface to Xilinx SDMA HW that could be used by the > xlldma and ll_temac drivers, for instance. Both of those will get > smaller, but won't go away. > > For this to be useful to me, it would need to be located somewhere more > accessible than arch/powerpc and it would need to have initialization > methods that don't depend on OF. In my build I would have platform code > that binds to the xlldma platform attachment, which would call Sergey's > SDMA code to assign it the proper resources. That should be fine. > Any objections to having Sergey's code live in drivers/dma, and putting > sdma.h out in include/linux? Might need to tweak the file/function names > some to head off namespace issues. Or is there some other strategy for > managing Xilinx-related drivers common to both Microblaze and PowerPC? I have no objections. This sounds like a good plan. g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Sergey Temerkhanov on 29 Mar 2010 16:20 On Monday 29 March 2010 19:56:15 Grant Likely wrote: > On Mon, Mar 29, 2010 at 9:42 AM, Steven J. Magnani > > <steve(a)digidescorp.com> wrote: > > On Fri, 2010-03-26 at 17:53 -0600, Grant Likely wrote: > >> I've not got time to review this patch right now, but Sergey and > >> Steven, you both posted MPMC drivers on the same day; Steven on the > >> microblaze list and Sergey on the powerpc list. Can you two please > >> coordinate and figure out how to mork toward a single driver that will > >> meet both your needs? I don't want to have 2 drivers (3 if you count > >> the ll_temac driver) in mainline for the same hardware interface. > > > > I don't think we'll end up with a single driver. A MPMC DMA Engine > > driver is useful only on "loopback" SDMA ports. Sergey's code looks like > > a nice generic interface to Xilinx SDMA HW that could be used by the > > xlldma and ll_temac drivers, for instance. Both of those will get > > smaller, but won't go away. Yes, it's like having IBM EMAC driver and MAL layer or something > > > > For this to be useful to me, it would need to be located somewhere more > > accessible than arch/powerpc and it would need to have initialization > > methods that don't depend on OF. In my build I would have platform code > > that binds to the xlldma platform attachment, which would call Sergey's > > SDMA code to assign it the proper resources. > > That should be fine. Well, I'll look at my old code for the platform interface bindings. I remember it worked well on arch/ppc with my other drivers. > > > Any objections to having Sergey's code live in drivers/dma, and putting > > sdma.h out in include/linux? Might need to tweak the file/function names > > some to head off namespace issues. Or is there some other strategy for > > managing Xilinx-related drivers common to both Microblaze and PowerPC? > > I have no objections. This sounds like a good plan. Or we can put Xilinx-related headers to, i.e., include/linux/xilinx. There might be some other candidates for this. > > g. > Regards, Sergey Temerkhanov, Cifronic ZAO -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Steven J. Magnani on 20 Apr 2010 12:40 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! www.digidescorp.com Earthling, return my space modulator!" #include <standard.disclaimer>
From: Sergey Temerkhanov on 27 Apr 2010 12:50 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! > www.digidescorp.com Earthling, return my space modulator!" > > #include <standard.disclaimer> > Regards, Sergey Temerkhanov, Cifronic ZAO. -- Regards, Sergey Temerkhanov, Cifronic ZAO -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|
Pages: 1 Prev: [PATCH 2/12] arch/s390/kernel: Add missing unlock Next: of: check for IS_ERR() |