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,
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
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
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);
> >>
