Re: [microblaze-uclinux] [PATCHv2] [RFC] Xilinx MPMC SDMA subsystem
On Wednesday 28 April 2010 09:13:06 you wrote: > Hi Sergey and Steven, > > On Tue, Apr 27, 2010 at 8:29 PM, Steven J. Magnani > > wrote: > > On Wed, 2010-04-28 at 02:06 +0400, Sergey Temerkhanov wrote: > >> This is the 2nd version of Xilinx MPMC LocalLink SDMA subsystem > >> > >> Changelog v2: > >> * Changed the functions and struct definition prefix from sdma_ to > >> xllsdma_ * Platform bus bindings and various changes by Steven J. > >> Magnani. * Moved source files from arch/powerpc/sysdev to global > >> locations and added CONFIG_XLLSDMA option. > >> > >> Regards, Sergey Temerkhanov, > >> Cifronic ZAO > >> > >> diff -r baced9e29ab5 drivers/dma/Kconfig > >> --- a/drivers/dma/Kconfig Tue Apr 27 20:48:50 2010 +0400 > >> +++ b/drivers/dma/Kconfig Wed Apr 28 02:00:51 2010 +0400 > >> @@ -97,6 +97,14 @@ > >> Support the TXx9 SoC internal DMA controller. This can be > >> integrated in chips such as the Toshiba TX4927/38/39. > >> > >> +config XLLSDMA > > I'd prefer XILINX_LLSDMA. XLLSDMA could be ambiguous in the global > namespace (for a human reader), particularly as it is something that > few people will actually see. I've changed it to XILINX_SDMA in the current version. > > >> + bool "Xilinx MPMC DMA support" > >> + depends on XILINX_VIRTEX || MICROBLAZE > >> + select DMA_ENGINE > >> + help > >> + Support fot Xilinx MPMC LocalLink SDMA. Virtex FPGA family > >> + has it integrated or fabric-based. > >> + > > > > fot --> for > > > > Since the xllsdma driver provides services to other drivers - not to > > userland - I think this would be better as a "silent" option, selected > > automatically when something like ll_temac or the forthcoming Xilinx DMA > > engine is selected. If we do it that way, note that XLLSDMA is > > independent of DMA_ENGINE so drivers/Makefile will need to be patched so > > that the dma subdirectory is always "y". > > Agreed. However, looking at this code, I don't see anything that > actually uses DMA_ENGINE here. Am I missing something? It's because the appropriate line in drivers/Makefile is obj-$(CONFIG_DMA_ENGINE) += dma/ instead of obj-$(CONFIG_DMADEVICES) += dma/ > > >> diff -r baced9e29ab5 drivers/dma/Makefile > >> --- a/drivers/dma/MakefileTue Apr 27 20:48:50 2010 +0400 > >> +++ b/drivers/dma/MakefileWed Apr 28 02:00:51 2010 +0400 > >> @@ -10,3 +10,4 @@ > >> obj-$(CONFIG_AT_HDMAC) += at_hdmac.o > >> obj-$(CONFIG_MX3_IPU) += ipu/ > >> obj-$(CONFIG_TXX9_DMAC) += txx9dmac.o > >> +obj-$(CONFIG_XLLSDMA) += xllsdma.o > >> diff -r baced9e29ab5 drivers/dma/xllsdma.c > >> --- /dev/null Thu Jan 01 00:00:00 1970 + > >> +++ b/drivers/dma/xllsdma.c Wed Apr 28 02:00:51 2010 +0400 > >> @@ -0,0 +1,887 @@ > >> +/* > >> + * SDMA subsystem support for Xilinx MPMC. > >> + * > >> + * Author: Sergey Temerkhanov > >> + * Platform Bus by Steven J. Magnani > >> + * > >> + * Copyright (c) 2008-2010 Cifronic ZAO > >> + * > >> + * This program is free software; you can redistribute it and/or > >> modify it + * under the terms of the GNU General Public License as > >> published by the + * Free Software Foundation; either version 2 of the > >> License, or (at your + * option) any later version. > >> + * > >> + */ > >> + > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> + > >> +#include > >> +#include > >> + > >> +#define DRV_VERSION "0.1.0" > > Irrelevant, can be dropped > > >> +#define DRV_NAME "sdma" > > Used only once, drop. > > >> + > >> +MODULE_AUTHOR("Sergey Temerkhanov "); > >> +MODULE_DESCRIPTION("Xilinx SDMA driver"); > >> +MODULE_LICENSE("GPL"); > >> +MODULE_VERSION(DRV_VERSION); > >> + > >> +LIST_HEAD(mpmc_devs); > >> +DEFINE_MUTEX(mpmc_devs_lock); > >> + > >> +void xllsdma_tx_irq_enable(struct xllsdma_device *sdma) > >> +{ > >> + u32 tx_cr; > >> + unsigned long flags; > >> + > >> + BUG_ON(sdma->tx_irq == NO_IRQ); > >> + > >> + spin_lock_irqsave(&sdma->lock, flags); > >> + tx_cr = xllsdma_tx_in32(sdma, XLLSDMA_CR); > >> + xllsdma_tx_out32(sdma, XLLSDMA_CR, tx_cr | XLLSDMA_CR_IRQ_EN); > >> + spin_unlock_irqrestore(&sdma->lock, flags); > > This pattern is used a lot. Might be worth while to implement > xllsdma_tx_* variants of setbits32, clrbits32 and clrsetbits32. > > Also, there are a lot of these little functions; really trivial and > small. Would it be better to have them as static inlines in the > header file instead of exported globals? Well, I can do this but it will require moving of register definitions to xllsdma.h > > >> +} > >> +EXPORT_SYMBOL_GPL(xllsdma_tx_irq_enable); > >> + > >> +void xllsdma_rx_irq_enable(struct xllsdma_device *sdma) > >> +{ > >> + u32 rx_cr; > >> + unsigned long flags; > >> + > >> + BUG_ON(sdma->rx_irq == NO_IRQ); > >> + > >> + spin_lock_irqsave(&sdma->lock, flags); > >>
Re: [microblaze-uclinux] [PATCHv2] [RFC] Xilinx MPMC SDMA subsystem
Grant - Thanks for the feedback. My responses are inline. --Steve On Tue, 2010-04-27 at 23:13 -0600, Grant Likely wrote: > Hi Sergey and Steven, > > On Tue, Apr 27, 2010 at 8:29 PM, Steven J. Magnani > wrote: > > On Wed, 2010-04-28 at 02:06 +0400, Sergey Temerkhanov wrote: > >> This is the 2nd version of Xilinx MPMC LocalLink SDMA subsystem > >> [snip] > >> diff -r baced9e29ab5 drivers/dma/Kconfig > >> --- a/drivers/dma/Kconfig Tue Apr 27 20:48:50 2010 +0400 > >> +++ b/drivers/dma/Kconfig Wed Apr 28 02:00:51 2010 +0400 > >> @@ -97,6 +97,14 @@ > >> Support the TXx9 SoC internal DMA controller. This can be > >> integrated in chips such as the Toshiba TX4927/38/39. > >> > >> +config XLLSDMA > > I'd prefer XILINX_LLSDMA. XLLSDMA could be ambiguous in the global > namespace (for a human reader), particularly as it is something that > few people will actually see. > > >> + bool "Xilinx MPMC DMA support" > >> + depends on XILINX_VIRTEX || MICROBLAZE > >> + select DMA_ENGINE > >> + help > >> + Support fot Xilinx MPMC LocalLink SDMA. Virtex FPGA family > >> + has it integrated or fabric-based. > >> + > > > > fot --> for > > > > Since the xllsdma driver provides services to other drivers - not to > > userland - > > I think this would be better as a "silent" option, selected automatically > > when > > something like ll_temac or the forthcoming Xilinx DMA engine is selected. > > If we do it that way, note that XLLSDMA is independent of DMA_ENGINE so > > drivers/Makefile will need to be patched so that the dma subdirectory is > > always "y". > > Agreed. However, looking at this code, I don't see anything that > actually uses DMA_ENGINE here. Am I missing something? This is a low-level driver that provides services to ll_temac and the (still in the works) DMA engine driver. There's no real relationship between this driver and DMA_ENGINE, and the still-in-the-works driver will come later after we stabilize the interface of this one. > >> +++ b/drivers/dma/xllsdma.c Wed Apr 28 02:00:51 2010 +0400 [snip] > >> + > >> +static int __devinit mpmc_of_probe(struct of_device *op, > >> + const struct of_device_id *match) > >> +{ > >> + int err = mpmc_init(&op->dev); > >> + if (err) > >> + return err; > >> + > >> + of_platform_bus_probe(op->node, xllsdma_of_match, &op->dev); > >> + return 0; > >> +} > > Okay, I think I've figured out what is bothering me > > While this *does* work; it really is the long way to go about things. > Doing it this way requires going back out to the driver model to tell > it things and trigger a probe on things that *this* driver needs, and > *this* driver already knows about. It doesn't need to be this > complex. > > Rather than register a bunch more of_platform devices, do something > like this instead: > > +static int __devinit mpmc_of_probe(struct of_device *op, > + const struct of_device_id *match) > +{ > + struct mpmc_device *mpmc; > + struct device_node *node; > + > + mpmc = mpmc_init(&op->dev); > + if (!mpmc) > + return -ENODEV; > + > + for_each_child_of_node(op->node, node) { > + xllsdma_of_init(mpmc, node); > + } > + return 0; > +} > > You do *not* need to register a separate struct device for each DMA > channel sub node (at least with regard to the driver model; I don't > know about the dma subsystem). If you *want* a struct device, then > xllsdma_of_init() is free to register one, but it does not need to be > on the of_platform_bus, and this driver should not require a probe > step for each DMA channel. The DMA engine driver (and ll_temac, I imagine) will gain access to this driver via xllsdma_find_device(). They should not need a struct device. [snip] > >> +#else/* CONFIG_OF */ > > Why else? It is perfectly valid to have both of_platform and platform > bus bindings. That being said, this split will become unnecessary in > the very near future. I've eliminated of_platform_bus_type, and > automatically moved all users of it over to the platform bus (without > driver changes). Agreed. It would help to have an idea of the timeline for the convergence. I haven't been following any OF-related discussions on LKML; I'll try to pay more attention. > > However, current powerpc and microblaze code makes CONFIG_OF > manditory. What condition will compile in the platform bus > attachment? At the moment, none in mainline. See my next comment. > > >> +/*--- > >> + * Platform bus attachment > >> + */ > >> + > >> +static __devexit int xllsdma_plat_remove(struct platform_device *pdev) > >> +{ > >> + xllsdma_cleanup(&pdev->dev); > >> + return 0; > >> +} > >> + > >> +static int __devinit xllsdma_plat_probe(struct platform_device *pdev) > >> +{ > >> + struct resource *rx_irq, *tx_irq, *mem; > >> +
Re: [microblaze-uclinux] [PATCHv2] [RFC] Xilinx MPMC SDMA subsystem
Hi Sergey and Steven, On Tue, Apr 27, 2010 at 8:29 PM, Steven J. Magnani wrote: > On Wed, 2010-04-28 at 02:06 +0400, Sergey Temerkhanov wrote: >> This is the 2nd version of Xilinx MPMC LocalLink SDMA subsystem >> >> Changelog v2: >> * Changed the functions and struct definition prefix from sdma_ to xllsdma_ >> * Platform bus bindings and various changes by Steven J. Magnani. >> * Moved source files from arch/powerpc/sysdev to global locations and added >> CONFIG_XLLSDMA option. >> >> Regards, Sergey Temerkhanov, >> Cifronic ZAO >> >> diff -r baced9e29ab5 drivers/dma/Kconfig >> --- a/drivers/dma/Kconfig Tue Apr 27 20:48:50 2010 +0400 >> +++ b/drivers/dma/Kconfig Wed Apr 28 02:00:51 2010 +0400 >> @@ -97,6 +97,14 @@ >> Support the TXx9 SoC internal DMA controller. This can be >> integrated in chips such as the Toshiba TX4927/38/39. >> >> +config XLLSDMA I'd prefer XILINX_LLSDMA. XLLSDMA could be ambiguous in the global namespace (for a human reader), particularly as it is something that few people will actually see. >> + bool "Xilinx MPMC DMA support" >> + depends on XILINX_VIRTEX || MICROBLAZE >> + select DMA_ENGINE >> + help >> + Support fot Xilinx MPMC LocalLink SDMA. Virtex FPGA family >> + has it integrated or fabric-based. >> + > > fot --> for > > Since the xllsdma driver provides services to other drivers - not to userland > - > I think this would be better as a "silent" option, selected automatically when > something like ll_temac or the forthcoming Xilinx DMA engine is selected. > If we do it that way, note that XLLSDMA is independent of DMA_ENGINE so > drivers/Makefile will need to be patched so that the dma subdirectory is > always "y". Agreed. However, looking at this code, I don't see anything that actually uses DMA_ENGINE here. Am I missing something? >> diff -r baced9e29ab5 drivers/dma/Makefile >> --- a/drivers/dma/Makefile Tue Apr 27 20:48:50 2010 +0400 >> +++ b/drivers/dma/Makefile Wed Apr 28 02:00:51 2010 +0400 >> @@ -10,3 +10,4 @@ >> obj-$(CONFIG_AT_HDMAC) += at_hdmac.o >> obj-$(CONFIG_MX3_IPU) += ipu/ >> obj-$(CONFIG_TXX9_DMAC) += txx9dmac.o >> +obj-$(CONFIG_XLLSDMA) += xllsdma.o >> diff -r baced9e29ab5 drivers/dma/xllsdma.c >> --- /dev/null Thu Jan 01 00:00:00 1970 + >> +++ b/drivers/dma/xllsdma.c Wed Apr 28 02:00:51 2010 +0400 >> @@ -0,0 +1,887 @@ >> +/* >> + * SDMA subsystem support for Xilinx MPMC. >> + * >> + * Author: Sergey Temerkhanov >> + * Platform Bus by Steven J. Magnani >> + * >> + * Copyright (c) 2008-2010 Cifronic ZAO >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms of the GNU General Public License as published by the >> + * Free Software Foundation; either version 2 of the License, or (at your >> + * option) any later version. >> + * >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> +#include >> + >> +#define DRV_VERSION "0.1.0" Irrelevant, can be dropped >> +#define DRV_NAME "sdma" Used only once, drop. >> + >> +MODULE_AUTHOR("Sergey Temerkhanov "); >> +MODULE_DESCRIPTION("Xilinx SDMA driver"); >> +MODULE_LICENSE("GPL"); >> +MODULE_VERSION(DRV_VERSION); >> + >> +LIST_HEAD(mpmc_devs); >> +DEFINE_MUTEX(mpmc_devs_lock); >> + >> +void xllsdma_tx_irq_enable(struct xllsdma_device *sdma) >> +{ >> + u32 tx_cr; >> + unsigned long flags; >> + >> + BUG_ON(sdma->tx_irq == NO_IRQ); >> + >> + spin_lock_irqsave(&sdma->lock, flags); >> + tx_cr = xllsdma_tx_in32(sdma, XLLSDMA_CR); >> + xllsdma_tx_out32(sdma, XLLSDMA_CR, tx_cr | XLLSDMA_CR_IRQ_EN); >> + spin_unlock_irqrestore(&sdma->lock, flags); This pattern is used a lot. Might be worth while to implement xllsdma_tx_* variants of setbits32, clrbits32 and clrsetbits32. Also, there are a lot of these little functions; really trivial and small. Would it be better to have them as static inlines in the header file instead of exported globals? >> +} >> +EXPORT_SYMBOL_GPL(xllsdma_tx_irq_enable); >> + >> +void xllsdma_rx_irq_enable(struct xllsdma_device *sdma) >> +{ >> + u32 rx_cr; >> + unsigned long flags; >> + >> + BUG_ON(sdma->rx_irq == NO_IRQ); >> + >> + spin_lock_irqsave(&sdma->lock, flags); >> + rx_cr = xllsdma_rx_in32(sdma, XLLSDMA_CR); >> + xllsdma_rx_out32(sdma, XLLSDMA_CR, rx_cr | XLLSDMA_CR_IRQ_EN); >> + spin_unlock_irqrestore(&sdma->lock, flags); >> +} >> +EXPORT_SYMBOL_GPL(xllsdma_rx_irq_enable); >> + >> +void xllsdma_tx_irq_disable(struct xllsdma_device *sdma) >> +{ >> + u32 tx_cr; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&sdma->lock, flags); >> + tx_cr = xllsdma_tx_in32(sdma, XLLSDMA_CR); >> + xllsdma_tx_out32(sdma, XLLSDMA_CR, tx_cr & ~XLLSDMA_CR_IRQ_EN); >> + spin_unlock_irqrestore(&sdma->lock, flags); >> +} >> +EXPORT_SYMBOL_GPL(xllsdm
Re: [microblaze-uclinux] [PATCHv2] [RFC] Xilinx MPMC SDMA subsystem
On Wed, 2010-04-28 at 02:06 +0400, Sergey Temerkhanov wrote: > This is the 2nd version of Xilinx MPMC LocalLink SDMA subsystem > > Changelog v2: > * Changed the functions and struct definition prefix from sdma_ to xllsdma_ > * Platform bus bindings and various changes by Steven J. Magnani. > * Moved source files from arch/powerpc/sysdev to global locations and added > CONFIG_XLLSDMA option. > > Regards, Sergey Temerkhanov, > Cifronic ZAO > > diff -r baced9e29ab5 drivers/dma/Kconfig > --- a/drivers/dma/Kconfig Tue Apr 27 20:48:50 2010 +0400 > +++ b/drivers/dma/Kconfig Wed Apr 28 02:00:51 2010 +0400 > @@ -97,6 +97,14 @@ > Support the TXx9 SoC internal DMA controller. This can be > integrated in chips such as the Toshiba TX4927/38/39. > > +config XLLSDMA > + bool "Xilinx MPMC DMA support" > + depends on XILINX_VIRTEX || MICROBLAZE > + select DMA_ENGINE > + help > + Support fot Xilinx MPMC LocalLink SDMA. Virtex FPGA family > + has it integrated or fabric-based. > + fot --> for Since the xllsdma driver provides services to other drivers - not to userland - I think this would be better as a "silent" option, selected automatically when something like ll_temac or the forthcoming Xilinx DMA engine is selected. If we do it that way, note that XLLSDMA is independent of DMA_ENGINE so drivers/Makefile will need to be patched so that the dma subdirectory is always "y". > > config DMA_ENGINE > bool > > @@ -133,3 +141,5 @@ > DMA Device driver. > > endif > + > + Some extra white space crept in at the end of the file. > diff -r baced9e29ab5 drivers/dma/Makefile > --- a/drivers/dma/MakefileTue Apr 27 20:48:50 2010 +0400 > +++ b/drivers/dma/MakefileWed Apr 28 02:00:51 2010 +0400 > @@ -10,3 +10,4 @@ > obj-$(CONFIG_AT_HDMAC) += at_hdmac.o > obj-$(CONFIG_MX3_IPU) += ipu/ > obj-$(CONFIG_TXX9_DMAC) += txx9dmac.o > +obj-$(CONFIG_XLLSDMA) += xllsdma.o > diff -r baced9e29ab5 drivers/dma/xllsdma.c > --- /dev/null Thu Jan 01 00:00:00 1970 + > +++ b/drivers/dma/xllsdma.c Wed Apr 28 02:00:51 2010 +0400 > @@ -0,0 +1,887 @@ > +/* > + * SDMA subsystem support for Xilinx MPMC. > + * > + * Author: Sergey Temerkhanov > + * Platform Bus by Steven J. Magnani > + * > + * Copyright (c) 2008-2010 Cifronic ZAO > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the > + * Free Software Foundation; either version 2 of the License, or (at your > + * option) any later version. > + * > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > + > +#define DRV_VERSION "0.1.0" > +#define DRV_NAME "sdma" > + > +MODULE_AUTHOR("Sergey Temerkhanov "); > +MODULE_DESCRIPTION("Xilinx SDMA driver"); > +MODULE_LICENSE("GPL"); > +MODULE_VERSION(DRV_VERSION); > + > +LIST_HEAD(mpmc_devs); > +DEFINE_MUTEX(mpmc_devs_lock); > + > +enum { > + XLLSDMA_TX_REGS = 0x00, /* TX channel registers beginning */ > + XLLSDMA_RX_REGS = 0x20, /* RX channel registers beginning */ > + XLLSDMA_DMACR = 0x40, /* DMA control register */ > + > + XLLSDMA_NDESCR = 0x00, /* Next descriptor address */ > + XLLSDMA_BUFA= 0x04, /* Current buffer address */ > + XLLSDMA_BUFL= 0x08, /* Current buffer length */ > + XLLSDMA_CDESCR = 0x0C, /* Current descriptor address */ > + XLLSDMA_TDESCR = 0x10, /* Tail descriptor address */ > + XLLSDMA_CR = 0x14, /* Channel control */ > + XLLSDMA_IRQ = 0x18, /* Interrupt register */ > + XLLSDMA_SR = 0x1C, /* Status */ > +}; > + > +enum { > + XLLSDMA_CR_IRQ_TIMEOUT_MSK = (0xFF << 24),/* Interrupt coalesce > timeout */ > + XLLSDMA_CR_IRQ_THRESHOLD_MSK = (0xFF << 16),/* Interrupt coalesce > count */ > + XLLSDMA_CR_MSB_ADDR_MSK = (0xF << 12), /* MSB for 36 bit > addressing */ > + XLLSDMA_CR_APP_EN = (1 << 11), /* Application data mask enable > */ > + XLLSDMA_CR_1_BIT_CNT = (1 << 10), /* All interrupt counters are > 1-bit */ > + XLLSDMA_CR_INT_ON_END = (1 << 9), /* Interrupt-on-end */ > + XLLSDMA_CR_LD_IRQ_CNT = (1 << 8), /* Load IRQ_COUNT */ > + XLLSDMA_CR_IRQ_EN = (1 << 7), /* Master interrupt enable */ > + XLLSDMA_CR_IRQ_ERROR = (1 << 2), /* Error interrupt enable */ > + XLLSDMA_CR_IRQ_TIMEOUT= (1 << 1), /* Coalesce timeout interrupt > enable */ > + XLLSDMA_CR_IRQ_THRESHOLD = (1 << 0), /* Coalesce threshold interrupt > enable */ > + > + XLLSDMA_CR_IRQ_ALL= XLLSDMA_CR_IRQ_EN | XLLSDMA_CR_IRQ_ERROR | > + XLLSDMA_CR_IRQ_TIMEOUT | > XLLSDMA_CR_IRQ_THRESHOLD, > + > + XLLSDMA_CR_IRQ_TIMEOUT_SH = 24, > + XLLSDMA_CR_IRQ_THRESHOLD_SH = 16, > + XLLSDMA_CR_MSB_ADDR_SH = 12,