RE: [PATCHv8 2/2] dma: Add Freescale eDMA engine driver support
> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Thursday, January 09, 2014 8:19 PM
> To: Lu Jingchang-B35083
> Cc: vinod.k...@intel.com; dan.j.willi...@intel.com; shawn@linaro.org;
> pawel.m...@arm.com; mark.rutl...@arm.com; swar...@wwwdotorg.org; linux-
> ker...@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
> devicet...@vger.kernel.org
> Subject: Re: [PATCHv8 2/2] dma: Add Freescale eDMA engine driver support
>
> On Thursday 09 January 2014 11:44:46 Jingchang Lu wrote:
> > >
> > > On Thursday 09 January 2014, Jingchang Lu wrote:
> > > > > > > +sai2: sai@40031000 {
> > > > > > > + compatible = "fsl,vf610-sai";
> > > > > > > + reg = <0x40031000 0x1000>;
> > > > > > > + interrupts = <0 86 0x04>;
> > > > > > > + clocks = <&clks VF610_CLK_SAI2>;
> > > > > > > + clock-names = "sai";
> > > > > > > + dma-names = "tx", "rx";
> > > > > > > + dmas = <&edma0 VF610_EDMA_DMAMUX0
> VF610_EDMA0_MUX0_SAI2_TX>,
> > > > > > > + <&edma0 VF610_EDMA_DMAMUX0
> VF610_EDMA0_MUX0_SAI2_RX>;
> > > > > > > + status = "disabled";
> > > > > > > +};
> > > > >
> > > > > It seems wrong to have macros defined like
> VF610_EDMA0_MUX0_SAI2_TX,
> > > > > in particular in the example. These should just be literal
> numbers.
> > > > [Lu Jingchang-b35083]
> > > > This eDMA engineer requires two specifiers, one is a mux group id,
> the
> > > other is a request source id.
> > > > The VF610_EDMA0_MUX0_SAI2_TX is sai's tx dma request source id, it
> is
> > > defined as a literal number.
> > > > There are totally more than 100 request source id, I have them
> macros
> > > defined to make it referenced
> > > > easily and clearly, just like some clock binding done.
> > > > The macros are defined in include/dt-bindings/dma/vf610-edma.h.
> > >
> > > The clock bindings are special because the macros there tend to be
> made
> > > up for controllers that just have a bunch of clocks at random
> register
> > > locations.
> > >
> > > This is not the case for DMA bindings (or some of the more regular
> clock
> > > controllers), so there is absolutely no reason to define those macros
> > > in a header file, it just adds artificial dependencies between the
> > > driver, SoC support and the binding.
> > >
> > > If the numbers are the same as the ones provided in the data sheet,
> > > just use the numbers and remove the macros.
> >
> > Yes, the request source id is defined in the data sheet.
> > each eDMA controller has two muxes, some SoCs have two eDMA controllers
> > while others have only one. The 1st eDMA's muxes are named mux0 and
> mux1,
> > while the 2nd eDMA controller's muxes are named MUX2 and MUX3 in the
> datasheet,
> > and I index them in each eDMA controller from 0 in the driver to handle
> them commonly.
>
> Right. I don't object to the use of the macros for the muxes, that seems
> fine and appropriate given your description.
>
> > I defines the macro tending to give the info from the macro name, such
> as
> > for the VF610_EDMA0_MUX0_SAI2_TX request source, implying the eDMA
> engine id 0,
> > and the mux id should be 0. While VF610_EDMA1_MUX1_FTM0_CH0 implying
> eDMA engine id 1,
> > mux 1. The request sources are shared between the two eDMA controller
> but need the user
> > to select one manually when reference.
>
> Note that the dma binding allows you to actually specify both engines
> in this case. A slave device can have
>
> dma-names = "rx", "rx", "tx", "tx";
> dmas = <&edma0 0 12> <&edma1 0 12> <&edma0 0 13> <&edma1 0 13>;
>
> and the requesting a channel from the slave driver will find
> the first available one. In theory we could have some load-balancing
> as well, but that has not been implemented.
[Lu Jingchang-b35083]
Yes, the slave can specify mixed dma channel and the eDMA driver can allocate
channels to it properly. But just as you said, the driver doesn't implement
load-balanceing between eDMA controller, this need the user to balance them
in dts and slave driver manually. And on our LS-1 SoC, there is only one eDMA
controller.
For the slave request id macros definitions, I thin
Re: [PATCHv8 2/2] dma: Add Freescale eDMA engine driver support
On Thursday 09 January 2014 11:44:46 Jingchang Lu wrote:
> >
> > On Thursday 09 January 2014, Jingchang Lu wrote:
> > > > > > +sai2: sai@40031000 {
> > > > > > + compatible = "fsl,vf610-sai";
> > > > > > + reg = <0x40031000 0x1000>;
> > > > > > + interrupts = <0 86 0x04>;
> > > > > > + clocks = <&clks VF610_CLK_SAI2>;
> > > > > > + clock-names = "sai";
> > > > > > + dma-names = "tx", "rx";
> > > > > > + dmas = <&edma0 VF610_EDMA_DMAMUX0 VF610_EDMA0_MUX0_SAI2_TX>,
> > > > > > + <&edma0 VF610_EDMA_DMAMUX0 VF610_EDMA0_MUX0_SAI2_RX>;
> > > > > > + status = "disabled";
> > > > > > +};
> > > >
> > > > It seems wrong to have macros defined like VF610_EDMA0_MUX0_SAI2_TX,
> > > > in particular in the example. These should just be literal numbers.
> > > [Lu Jingchang-b35083]
> > > This eDMA engineer requires two specifiers, one is a mux group id, the
> > other is a request source id.
> > > The VF610_EDMA0_MUX0_SAI2_TX is sai's tx dma request source id, it is
> > defined as a literal number.
> > > There are totally more than 100 request source id, I have them macros
> > defined to make it referenced
> > > easily and clearly, just like some clock binding done.
> > > The macros are defined in include/dt-bindings/dma/vf610-edma.h.
> >
> > The clock bindings are special because the macros there tend to be made
> > up for controllers that just have a bunch of clocks at random register
> > locations.
> >
> > This is not the case for DMA bindings (or some of the more regular clock
> > controllers), so there is absolutely no reason to define those macros
> > in a header file, it just adds artificial dependencies between the
> > driver, SoC support and the binding.
> >
> > If the numbers are the same as the ones provided in the data sheet,
> > just use the numbers and remove the macros.
>
> Yes, the request source id is defined in the data sheet.
> each eDMA controller has two muxes, some SoCs have two eDMA controllers
> while others have only one. The 1st eDMA's muxes are named mux0 and mux1,
> while the 2nd eDMA controller's muxes are named MUX2 and MUX3 in the
> datasheet,
> and I index them in each eDMA controller from 0 in the driver to handle them
> commonly.
Right. I don't object to the use of the macros for the muxes, that seems
fine and appropriate given your description.
> I defines the macro tending to give the info from the macro name, such as
> for the VF610_EDMA0_MUX0_SAI2_TX request source, implying the eDMA engine id
> 0,
> and the mux id should be 0. While VF610_EDMA1_MUX1_FTM0_CH0 implying eDMA
> engine id 1,
> mux 1. The request sources are shared between the two eDMA controller but
> need the user
> to select one manually when reference.
Note that the dma binding allows you to actually specify both engines
in this case. A slave device can have
dma-names = "rx", "rx", "tx", "tx";
dmas = <&edma0 0 12> <&edma1 0 12> <&edma0 0 13> <&edma1 0 13>;
and the requesting a channel from the slave driver will find
the first available one. In theory we could have some load-balancing
as well, but that has not been implemented.
> > You are right, your code is actually correct on all combinations
> > of big-endian and little-endian ARM CPUs. However, I would argue that
> > it's unusual style, and not portable to other architectures (e.g. arm64)
> > because the definition of readl() is highly architecture dependent.
> > It would also be problematic if the arm definition has to change
> > in some form and this driver is overlooked.
> [Lu Jingchang-b35083]
> Yes, these definitions are highly depending on the arm32 arch. This device is
> only
> integrated in our arm32 SoCs currently, so I have only considered arm32.
> In arm32, it seems that the ioread32 and readl are the same in arm32,
> I will try your recommendation below to simplify the caller.
Ok, thanks. I generally insist on writing drivers in a portable way
if possible because they can serve as examples to other people, and
hardware often gets reused on other parts. I would expect that it's
possible that freescale eventually creates powerpc or arm64 devices
with the same edma controller, even if you are not currently aware
of any such products.
> > BTW, I noticed that fsl_edma_set_tcd_params() is calling edma_writew()
> > and edma_writel() without an endian-swap, so I assume it is still
> > broken on big-endian CPUs, and likely also on big-endian eDMA engines.
> The values written in fsl_edma_set_tcd_params() are pre-swapped in
> fill_tcd_params().
> The eDMA support hardware SGs, but the eDMA engine's sg format is required
> the same as
> the eDMA Controller's endian, so the SGs in memory to be loaded automatically
> by the eDMA
> engine also required swapped properly. So they should be swapped specifically
> here.
Ah, I see now. I had noticed that before but then forgotten about it.
> > > > > > +static bool fsl_edma_filter_fn(struct dma_chan *chan, void *mux)
> > > > > > +{
> > > > > > +return (c
RE: [PATCHv8 2/2] dma: Add Freescale eDMA engine driver support
> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Thursday, January 09, 2014 6:43 PM
> To: Lu Jingchang-B35083
> Cc: vinod.k...@intel.com; dan.j.willi...@intel.com; shawn@linaro.org;
> pawel.m...@arm.com; mark.rutl...@arm.com; swar...@wwwdotorg.org; linux-
> ker...@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
> devicet...@vger.kernel.org
> Subject: Re: [PATCHv8 2/2] dma: Add Freescale eDMA engine driver support
>
> On Thursday 09 January 2014, Jingchang Lu wrote:
> > > > > +sai2: sai@40031000 {
> > > > > + compatible = "fsl,vf610-sai";
> > > > > + reg = <0x40031000 0x1000>;
> > > > > + interrupts = <0 86 0x04>;
> > > > > + clocks = <&clks VF610_CLK_SAI2>;
> > > > > + clock-names = "sai";
> > > > > + dma-names = "tx", "rx";
> > > > > + dmas = <&edma0 VF610_EDMA_DMAMUX0 VF610_EDMA0_MUX0_SAI2_TX>,
> > > > > + <&edma0 VF610_EDMA_DMAMUX0 VF610_EDMA0_MUX0_SAI2_RX>;
> > > > > + status = "disabled";
> > > > > +};
> > >
> > > It seems wrong to have macros defined like VF610_EDMA0_MUX0_SAI2_TX,
> > > in particular in the example. These should just be literal numbers.
> > [Lu Jingchang-b35083]
> > This eDMA engineer requires two specifiers, one is a mux group id, the
> other is a request source id.
> > The VF610_EDMA0_MUX0_SAI2_TX is sai's tx dma request source id, it is
> defined as a literal number.
> > There are totally more than 100 request source id, I have them macros
> defined to make it referenced
> > easily and clearly, just like some clock binding done.
> > The macros are defined in include/dt-bindings/dma/vf610-edma.h.
>
> The clock bindings are special because the macros there tend to be made
> up for controllers that just have a bunch of clocks at random register
> locations.
>
> This is not the case for DMA bindings (or some of the more regular clock
> controllers), so there is absolutely no reason to define those macros
> in a header file, it just adds artificial dependencies between the
> driver, SoC support and the binding.
>
> If the numbers are the same as the ones provided in the data sheet,
> just use the numbers and remove the macros.
Yes, the request source id is defined in the data sheet.
each eDMA controller has two muxes, some SoCs have two eDMA controllers
while others have only one. The 1st eDMA's muxes are named mux0 and mux1,
while the 2nd eDMA controller's muxes are named MUX2 and MUX3 in the datasheet,
and I index them in each eDMA controller from 0 in the driver to handle them
commonly.
I defines the macro tending to give the info from the macro name, such as
for the VF610_EDMA0_MUX0_SAI2_TX request source, implying the eDMA engine id 0,
and the mux id should be 0. While VF610_EDMA1_MUX1_FTM0_CH0 implying eDMA
engine id 1,
mux 1. The request sources are shared between the two eDMA controller but need
the user
to select one manually when reference.
I think this could ease the dma reference for the eDMA controller, or I will
remove them.
Thanks!
>
> > > > > +/*
> > > > > + * copying of ARM read{b,w,l) and write{b,w,l) macro definations
> > > > > + * except for doing default swap of cpu_to_le32, the bytes swap
> > > > > + * is done depending on eDMA controller's endian defination,
> > > > > + * which may be big-endian or little-endian.
> > > > > + */
> > > > > +static inline u16 edma_readw(void __iomem *addr)
> > > > > +{
> > > > > + u16 __v = (__force u16) __raw_readw(addr);
> > > > > +
> > > > > + __iormb();
> > > > > + return __v;
> > > > > +}
> > >
> > > The comment doesn't seem to match the implementation: You don't
> > > actually do swaps here at all, which means this will fail when
> > > your kernel is running in big-endian mode. Just use the regular
> > > readw() etc, or use ioread16/ioread16be depending on the flag,
> > > and get rid of your EDMA_SWAP macros.
> > >
> > > No need to come up with your own scheme when the problem has
> > > been solved already elsewhere.
> > The working scenario for this device is, the cpu running in little-
> endian mode,
> > While the eDMA module running in little- or big-endian mode. This
> device is currently
> > running on ARM architecture only, and I notice that ARM's little-endian
> readl/writel
> > t
Re: [PATCHv8 2/2] dma: Add Freescale eDMA engine driver support
On Thursday 09 January 2014, Jingchang Lu wrote:
> > > > +sai2: sai@40031000 {
> > > > + compatible = "fsl,vf610-sai";
> > > > + reg = <0x40031000 0x1000>;
> > > > + interrupts = <0 86 0x04>;
> > > > + clocks = <&clks VF610_CLK_SAI2>;
> > > > + clock-names = "sai";
> > > > + dma-names = "tx", "rx";
> > > > + dmas = <&edma0 VF610_EDMA_DMAMUX0 VF610_EDMA0_MUX0_SAI2_TX>,
> > > > + <&edma0 VF610_EDMA_DMAMUX0 VF610_EDMA0_MUX0_SAI2_RX>;
> > > > + status = "disabled";
> > > > +};
> >
> > It seems wrong to have macros defined like VF610_EDMA0_MUX0_SAI2_TX,
> > in particular in the example. These should just be literal numbers.
> [Lu Jingchang-b35083]
> This eDMA engineer requires two specifiers, one is a mux group id, the other
> is a request source id.
> The VF610_EDMA0_MUX0_SAI2_TX is sai's tx dma request source id, it is defined
> as a literal number.
> There are totally more than 100 request source id, I have them macros defined
> to make it referenced
> easily and clearly, just like some clock binding done.
> The macros are defined in include/dt-bindings/dma/vf610-edma.h.
The clock bindings are special because the macros there tend to be made
up for controllers that just have a bunch of clocks at random register
locations.
This is not the case for DMA bindings (or some of the more regular clock
controllers), so there is absolutely no reason to define those macros
in a header file, it just adds artificial dependencies between the
driver, SoC support and the binding.
If the numbers are the same as the ones provided in the data sheet,
just use the numbers and remove the macros.
> > > > +/*
> > > > + * copying of ARM read{b,w,l) and write{b,w,l) macro definations
> > > > + * except for doing default swap of cpu_to_le32, the bytes swap
> > > > + * is done depending on eDMA controller's endian defination,
> > > > + * which may be big-endian or little-endian.
> > > > + */
> > > > +static inline u16 edma_readw(void __iomem *addr)
> > > > +{
> > > > + u16 __v = (__force u16) __raw_readw(addr);
> > > > +
> > > > + __iormb();
> > > > + return __v;
> > > > +}
> >
> > The comment doesn't seem to match the implementation: You don't
> > actually do swaps here at all, which means this will fail when
> > your kernel is running in big-endian mode. Just use the regular
> > readw() etc, or use ioread16/ioread16be depending on the flag,
> > and get rid of your EDMA_SWAP macros.
> >
> > No need to come up with your own scheme when the problem has
> > been solved already elsewhere.
> The working scenario for this device is, the cpu running in little-endian
> mode,
> While the eDMA module running in little- or big-endian mode. This device is
> currently
> running on ARM architecture only, and I notice that ARM's little-endian
> readl/writel
> treats all value as little-endian. This is not matching our scenario here. So
> I removed
> the default cpu_to_le32() in the readl(), and put it in EDMA_SWAP32() to
> satisfy the
> required.
You are right, your code is actually correct on all combinations
of big-endian and little-endian ARM CPUs. However, I would argue that
it's unusual style, and not portable to other architectures (e.g. arm64)
because the definition of readl() is highly architecture dependent.
It would also be problematic if the arm definition has to change
in some form and this driver is overlooked.
I would still recommend doing an implementation like
static inline u16 edma_readw(struct fsl_edma_engine *edma, u16 val, unsigned
long offset)
{
if (edma->big_endian)
iowrite16be(val, edma->membase + offset);
else
iowrite16(val, edma->membase + offset);
}
This would simplify the callers that now can replace
cur_addr = EDMA_SWAP32(fsl_chan->edma, edma_readl(addr +
EDMA_TCD_SADDR(ch)));
with
cur_addr = edma_readl(fsl_chan->edma, EDMA_TCD_SADDR(ch));
which IMHO is much easier to read. For accessing muxbase, you
don't have to use your own version, because all accesses are
single-byte.
BTW, I noticed that fsl_edma_set_tcd_params() is calling edma_writew()
and edma_writel() without an endian-swap, so I assume it is still
broken on big-endian CPUs, and likely also on big-endian eDMA engines.
> > > > +static irqreturn_t fsl_edma_irq_handler(int irq, void *dev_id)
> > > > +{
> > > > + if (fsl_edma_tx_handler(irq, dev_id) == IRQ_HANDLED)
> > > > + return IRQ_HANDLED;
> > > > +
> > > > + return fsl_edma_err_handler(irq, dev_id);
> > > > +}
> >
> > Does this do the right thing if both completion and error are
> > signalled at the same time? It seems you'd miss the error interrupt.
> I think the error would occur rarely, so if the transmission irq entered,
> it will return directly after handling. If there is really an error occur,
> the interrupt will be raised again without transmission interrupt flag, then
> the irq handler would execute fsl_
RE: [PATCHv8 2/2] dma: Add Freescale eDMA engine driver support
> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Wednesday, January 08, 2014 6:54 PM
> To: Lu Jingchang-B35083
> Cc: vinod.k...@intel.com; dan.j.willi...@intel.com; shawn@linaro.org;
> pawel.m...@arm.com; mark.rutl...@arm.com; swar...@wwwdotorg.org; linux-
> ker...@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
> devicet...@vger.kernel.org
> Subject: Re: [PATCHv8 2/2] dma: Add Freescale eDMA engine driver support
>
> On Wednesday 08 January 2014 10:24:38 Jingchang Lu wrote:
>
> > > +- clocks : Phandle of the DMA channel group block clock of the eDMA
> > > module
> > > +- clock-names : The channel group block clock names
>
> Turn these around and first define the required names, then state
> that 'clocks' should contain none clock specifier for each clock
> name.
I will turn them around, thanks.
>
> > > +sai2: sai@40031000 {
> > > + compatible = "fsl,vf610-sai";
> > > + reg = <0x40031000 0x1000>;
> > > + interrupts = <0 86 0x04>;
> > > + clocks = <&clks VF610_CLK_SAI2>;
> > > + clock-names = "sai";
> > > + dma-names = "tx", "rx";
> > > + dmas = <&edma0 VF610_EDMA_DMAMUX0 VF610_EDMA0_MUX0_SAI2_TX>,
> > > + <&edma0 VF610_EDMA_DMAMUX0 VF610_EDMA0_MUX0_SAI2_RX>;
> > > + status = "disabled";
> > > +};
>
> It seems wrong to have macros defined like VF610_EDMA0_MUX0_SAI2_TX,
> in particular in the example. These should just be literal numbers.
[Lu Jingchang-b35083]
This eDMA engineer requires two specifiers, one is a mux group id, the other is
a request source id.
The VF610_EDMA0_MUX0_SAI2_TX is sai's tx dma request source id, it is defined
as a literal number.
There are totally more than 100 request source id, I have them macros defined
to make it referenced
easily and clearly, just like some clock binding done.
The macros are defined in include/dt-bindings/dma/vf610-edma.h.
Thanks.
>
> It's ok to have macros for the VF610_EDMA_DMAMUX0, but I suspect
> that the possible values are just '0' and '1', which means a literal
> would be just as easy here.
>
> > > +struct fsl_edma_hw_tcd {
> > > + u32 saddr;
> > > + u16 soff;
> > > + u16 attr;
> > > + u32 nbytes;
> > > + u32 slast;
> > > + u32 daddr;
> > > + u16 doff;
> > > + u16 citer;
> > > + u32 dlast_sga;
> > > + u16 csr;
> > > + u16 biter;
> > > +} __packed;
>
> The structure is packed already, and marking it __packed
> will just mean that the compiler can't access the members
> atomically. Please remove the __packed keyword.
Ok, I will remove it, thanks.
>
> > > +/* bytes swapping according to eDMA controller's endian */
> > > +#define EDMA_SWAP16(e, v)(__force u16)(e->big_endian ?
> > > cpu_to_be16(v) : cpu_to_le16(v))
> > > +#define EDMA_SWAP32(e, v)(__force u32)(e->big_endian ?
> > > cpu_to_be32(v) : cpu_to_le32(v))
>
> Maybe use inline functions for these?
>
> > > +/*
> > > + * copying of ARM read{b,w,l) and write{b,w,l) macro definations
> > > + * except for doing default swap of cpu_to_le32, the bytes swap
> > > + * is done depending on eDMA controller's endian defination,
> > > + * which may be big-endian or little-endian.
> > > + */
> > > +static inline u16 edma_readw(void __iomem *addr)
> > > +{
> > > + u16 __v = (__force u16) __raw_readw(addr);
> > > +
> > > + __iormb();
> > > + return __v;
> > > +}
>
> The comment doesn't seem to match the implementation: You don't
> actually do swaps here at all, which means this will fail when
> your kernel is running in big-endian mode. Just use the regular
> readw() etc, or use ioread16/ioread16be depending on the flag,
> and get rid of your EDMA_SWAP macros.
>
> No need to come up with your own scheme when the problem has
> been solved already elsewhere.
The working scenario for this device is, the cpu running in little-endian mode,
While the eDMA module running in little- or big-endian mode. This device is
currently
running on ARM architecture only, and I notice that ARM's little-endian
readl/writel
treats all value as little-endian. This is not matching our scenario here. So I
removed
the default cpu_to_le32() in the readl(), and put it in EDMA_SWAP32() to
satisfy the
required.
Thanks.
>
> > > +static irqreturn_t fsl_edma_irq_handler(int irq, void *dev_id)
> > &g
Re: [PATCHv8 2/2] dma: Add Freescale eDMA engine driver support
On Wednesday 08 January 2014 10:24:38 Jingchang Lu wrote:
> > +- clocks : Phandle of the DMA channel group block clock of the eDMA
> > module
> > +- clock-names : The channel group block clock names
Turn these around and first define the required names, then state
that 'clocks' should contain none clock specifier for each clock
name.
> > +sai2: sai@40031000 {
> > + compatible = "fsl,vf610-sai";
> > + reg = <0x40031000 0x1000>;
> > + interrupts = <0 86 0x04>;
> > + clocks = <&clks VF610_CLK_SAI2>;
> > + clock-names = "sai";
> > + dma-names = "tx", "rx";
> > + dmas = <&edma0 VF610_EDMA_DMAMUX0 VF610_EDMA0_MUX0_SAI2_TX>,
> > + <&edma0 VF610_EDMA_DMAMUX0 VF610_EDMA0_MUX0_SAI2_RX>;
> > + status = "disabled";
> > +};
It seems wrong to have macros defined like VF610_EDMA0_MUX0_SAI2_TX,
in particular in the example. These should just be literal numbers.
It's ok to have macros for the VF610_EDMA_DMAMUX0, but I suspect
that the possible values are just '0' and '1', which means a literal
would be just as easy here.
> > +struct fsl_edma_hw_tcd {
> > + u32 saddr;
> > + u16 soff;
> > + u16 attr;
> > + u32 nbytes;
> > + u32 slast;
> > + u32 daddr;
> > + u16 doff;
> > + u16 citer;
> > + u32 dlast_sga;
> > + u16 csr;
> > + u16 biter;
> > +} __packed;
The structure is packed already, and marking it __packed
will just mean that the compiler can't access the members
atomically. Please remove the __packed keyword.
> > +/* bytes swapping according to eDMA controller's endian */
> > +#define EDMA_SWAP16(e, v) (__force u16)(e->big_endian ?
> > cpu_to_be16(v) : cpu_to_le16(v))
> > +#define EDMA_SWAP32(e, v) (__force u32)(e->big_endian ?
> > cpu_to_be32(v) : cpu_to_le32(v))
Maybe use inline functions for these?
> > +/*
> > + * copying of ARM read{b,w,l) and write{b,w,l) macro definations
> > + * except for doing default swap of cpu_to_le32, the bytes swap
> > + * is done depending on eDMA controller's endian defination,
> > + * which may be big-endian or little-endian.
> > + */
> > +static inline u16 edma_readw(void __iomem *addr)
> > +{
> > + u16 __v = (__force u16) __raw_readw(addr);
> > +
> > + __iormb();
> > + return __v;
> > +}
The comment doesn't seem to match the implementation: You don't
actually do swaps here at all, which means this will fail when
your kernel is running in big-endian mode. Just use the regular
readw() etc, or use ioread16/ioread16be depending on the flag,
and get rid of your EDMA_SWAP macros.
No need to come up with your own scheme when the problem has
been solved already elsewhere.
> > +static irqreturn_t fsl_edma_irq_handler(int irq, void *dev_id)
> > +{
> > + if (fsl_edma_tx_handler(irq, dev_id) == IRQ_HANDLED)
> > + return IRQ_HANDLED;
> > +
> > + return fsl_edma_err_handler(irq, dev_id);
> > +}
Does this do the right thing if both completion and error are
signalled at the same time? It seems you'd miss the error interrupt.
> > +static bool fsl_edma_filter_fn(struct dma_chan *chan, void *mux)
> > +{
> > +return (chan->chan_id / DMAMUX_NR) == (u32)mux;
> > +}
> > +
> > +static struct dma_chan *fsl_edma_xlate(struct of_phandle_args *dma_spec,
> > + struct of_dma *ofdma)
> > +{
> > + struct dma_chan *chan;
> > + dma_cap_mask_t mask;
> > +
> > + if (dma_spec->args_count != 2)
> > + return NULL;
> > +
> > + dma_cap_zero(mask);
> > + dma_cap_set(DMA_SLAVE, mask);
> > + dma_cap_set(DMA_CYCLIC, mask);
> > + chan = dma_request_channel(mask, fsl_edma_filter_fn, (void
> > *)dma_spec->args[0]);
> > + if (chan)
> > + fsl_edma_chan_mux(to_fsl_edma_chan(chan), dma_spec->args[1],
> > true);
> > + return chan;
> > +}
Please remove the filter function now and use dma_get_slave_chan
with the correct channel as an argument. No need to walk all
available channels in the system and introduce bugs by not
ignoring other dma engines.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv8 2/2] dma: Add Freescale eDMA engine driver support
Hi, Vinod, Mark and other maintainers,
Could you please help review this Freescale eDMA driver and the dts binding.
Many thanks!
Best Regards,
Jingchang
> -Original Message-
> From: Jingchang Lu [mailto:b35...@freescale.com]
> Sent: Thursday, January 02, 2014 2:52 PM
> To: vinod.k...@intel.com
> Cc: dan.j.willi...@intel.com; shawn@linaro.org; pawel.m...@arm.com;
> mark.rutl...@arm.com; swar...@wwwdotorg.org; linux-kernel@vger.kernel.org;
> linux-arm-ker...@lists.infradead.org; devicet...@vger.kernel.org; Lu
> Jingchang-B35083; Wang Huan-B18965
> Subject: [PATCHv8 2/2] dma: Add Freescale eDMA engine driver support
>
> Add Freescale enhanced direct memory(eDMA) controller support.
> This module can be found on Vybrid and LS-1 SoCs.
>
> Signed-off-by: Alison Wang
> Signed-off-by: Jingchang Lu
> ---
> changes in v8:
> change the edma driver according eDMA dts change.
> add big-endian and little-endian handling.
>
> no changes in v4 ~ v7.
>
> changes in v3:
> add vf610 edma dt-bindings namespace with prefix VF610_*.
>
> changes in v2:
> using generic dma-channels property instead of fsl,dma-channels.
>
> Documentation/devicetree/bindings/dma/fsl-edma.txt | 67 ++
> drivers/dma/Kconfig| 10 +
> drivers/dma/Makefile | 1 +
> drivers/dma/fsl-edma.c | 939
> +
> 4 files changed, 1017 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/dma/fsl-edma.txt
> create mode 100644 drivers/dma/fsl-edma.c
>
> diff --git a/Documentation/devicetree/bindings/dma/fsl-edma.txt
> b/Documentation/devicetree/bindings/dma/fsl-edma.txt
> new file mode 100644
> index 000..3ff6603
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/fsl-edma.txt
> @@ -0,0 +1,67 @@
> +* Freescale enhanced Direct Memory Access(eDMA) Controller
> +
> + The eDMA channels have multiplex capability by programmble memory-
> mapped
> +register. All channels are split into two groups, called DMAMUX0 and
> DMAMUX1,
> +specific DMA request source can only be multiplexed by any channel of
> certain
> +group, DMAMUX0 or DMAMUX1, but not both.
> +
> +* eDMA Controller
> +Required properties:
> +- compatible :
> + - "fsl,vf610-edma" for eDMA used similar to that on Vybrid vf610
> SoC
> +- reg : Specifies base physical address(s) and size of the eDMA
> registers.
> + The 1st region is eDMA control register's address and size.
> + The 2nd and the 3rd regions are programmable channel multiplexing
> + control register's address and size.
> +- interrupts : Should contain eDMA interrupt
> +- interrupt-names : Should be "edma-tx" for transmission interrupt and
> + "edma-err" for error interrupt
> +- #dma-cells : Must be <2>.
> + The 1st cell specifies the DMAMUX(0 for DMAMUX0 and 1 for DMAMUX1).
> + Specific request source can only be multiplexed by specific
> channels
> + group called DMAMUX.
> + The 2nd cell specifies the request source(slot).
> + See include/dt-bindings/dma/-edma.h for all the supported
> + request sources.
> +- dma-channels : Number of channels supported by the controller
> +- clocks : Phandle of the DMA channel group block clock of the eDMA
> module
> +- clock-names : The channel group block clock names
> +
> +
> +Examples:
> +
> +edma0: dma-controller@40018000 {
> + #dma-cells = <2>;
> + compatible = "fsl,vf610-edma";
> + reg = <0x40018000 0x2000>,
> + <0x40024000 0x1000>,
> + <0x40025000 0x1000>;
> + interrupts = <0 8 0x04>,
> + <0 9 0x04>;
> + interrupt-names = "edma-tx", "edma-err";
> + dma-channels = <32>;
> + clocks = <&clks VF610_CLK_DMAMUX0>,
> + <&clks VF610_CLK_DMAMUX1>;
> + clock-names = "dmamux0", "dmamux1";
> +};
> +
> +
> +* DMA clients
> +DMA client drivers that uses the DMA function must use the format
> described
> +in the dma.txt file, using a two-cell specifier for each channel: the
> 1st
> +specifies the channel group(DMAMUX) in which this request can be
> multiplexed,
> +and the 2nd specifies the request source.
> +
> +Examples:
> +
> +sai2: sai@40031000 {
> + compatible = "fsl,vf610-sai";
> + reg = <0x40031000 0x1000>;
> + interrupts = <0 86 0x04>;
> + clocks = <&clks VF610_CLK_SAI2>;
> + clock-names = "sai";
> + dma-names = "tx", "rx&quo
[PATCHv8 2/2] dma: Add Freescale eDMA engine driver support
Add Freescale enhanced direct memory(eDMA) controller support.
This module can be found on Vybrid and LS-1 SoCs.
Signed-off-by: Alison Wang
Signed-off-by: Jingchang Lu
---
changes in v8:
change the edma driver according eDMA dts change.
add big-endian and little-endian handling.
no changes in v4 ~ v7.
changes in v3:
add vf610 edma dt-bindings namespace with prefix VF610_*.
changes in v2:
using generic dma-channels property instead of fsl,dma-channels.
Documentation/devicetree/bindings/dma/fsl-edma.txt | 67 ++
drivers/dma/Kconfig| 10 +
drivers/dma/Makefile | 1 +
drivers/dma/fsl-edma.c | 939 +
4 files changed, 1017 insertions(+)
create mode 100644 Documentation/devicetree/bindings/dma/fsl-edma.txt
create mode 100644 drivers/dma/fsl-edma.c
diff --git a/Documentation/devicetree/bindings/dma/fsl-edma.txt
b/Documentation/devicetree/bindings/dma/fsl-edma.txt
new file mode 100644
index 000..3ff6603
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/fsl-edma.txt
@@ -0,0 +1,67 @@
+* Freescale enhanced Direct Memory Access(eDMA) Controller
+
+ The eDMA channels have multiplex capability by programmble memory-mapped
+register. All channels are split into two groups, called DMAMUX0 and DMAMUX1,
+specific DMA request source can only be multiplexed by any channel of certain
+group, DMAMUX0 or DMAMUX1, but not both.
+
+* eDMA Controller
+Required properties:
+- compatible :
+ - "fsl,vf610-edma" for eDMA used similar to that on Vybrid vf610 SoC
+- reg : Specifies base physical address(s) and size of the eDMA registers.
+ The 1st region is eDMA control register's address and size.
+ The 2nd and the 3rd regions are programmable channel multiplexing
+ control register's address and size.
+- interrupts : Should contain eDMA interrupt
+- interrupt-names : Should be "edma-tx" for transmission interrupt and
+ "edma-err" for error interrupt
+- #dma-cells : Must be <2>.
+ The 1st cell specifies the DMAMUX(0 for DMAMUX0 and 1 for DMAMUX1).
+ Specific request source can only be multiplexed by specific channels
+ group called DMAMUX.
+ The 2nd cell specifies the request source(slot).
+ See include/dt-bindings/dma/-edma.h for all the supported
+ request sources.
+- dma-channels : Number of channels supported by the controller
+- clocks : Phandle of the DMA channel group block clock of the eDMA module
+- clock-names : The channel group block clock names
+
+
+Examples:
+
+edma0: dma-controller@40018000 {
+ #dma-cells = <2>;
+ compatible = "fsl,vf610-edma";
+ reg = <0x40018000 0x2000>,
+ <0x40024000 0x1000>,
+ <0x40025000 0x1000>;
+ interrupts = <0 8 0x04>,
+ <0 9 0x04>;
+ interrupt-names = "edma-tx", "edma-err";
+ dma-channels = <32>;
+ clocks = <&clks VF610_CLK_DMAMUX0>,
+ <&clks VF610_CLK_DMAMUX1>;
+ clock-names = "dmamux0", "dmamux1";
+};
+
+
+* DMA clients
+DMA client drivers that uses the DMA function must use the format described
+in the dma.txt file, using a two-cell specifier for each channel: the 1st
+specifies the channel group(DMAMUX) in which this request can be multiplexed,
+and the 2nd specifies the request source.
+
+Examples:
+
+sai2: sai@40031000 {
+ compatible = "fsl,vf610-sai";
+ reg = <0x40031000 0x1000>;
+ interrupts = <0 86 0x04>;
+ clocks = <&clks VF610_CLK_SAI2>;
+ clock-names = "sai";
+ dma-names = "tx", "rx";
+ dmas = <&edma0 VF610_EDMA_DMAMUX0 VF610_EDMA0_MUX0_SAI2_TX>,
+ <&edma0 VF610_EDMA_DMAMUX0 VF610_EDMA0_MUX0_SAI2_RX>;
+ status = "disabled";
+};
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index c10eb89..7989c8c 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -336,6 +336,16 @@ config K3_DMA
Support the DMA engine for Hisilicon K3 platform
devices.
+config FSL_EDMA
+ tristate "Freescale eDMA engine support"
+ depends on OF
+ select DMA_ENGINE
+ select DMA_VIRTUAL_CHANNELS
+ help
+ Support the Freescale eDMA engine with DMAMUXs multiplexing
+ DMA request sources(slot) on eDMA engine channels.
+ This module can be found on Freescale Vybrid and LS-1 SoCs.
+
config DMA_ENGINE
bool
diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
index 0ce2da9..68422af 100644
--- a/drivers/dma/Makefile
+++ b/drivers/dma/Makefile
@@ -42,3 +42,4 @@ obj-$(CONFIG_MMP_PDMA) += mmp_pdma.o
obj-$(CONFIG_DMA_JZ4740) += dma-jz4740.o
obj-$(CONFIG_TI_CPPI41) += cppi41.o
obj-$(CONFIG_K3_DMA) += k3dma.o
+obj-$(CONFIG_FSL_EDMA) += fsl-edma.o
diff --git a/drivers/dma/fsl-edma.c b/drivers/dma/fsl-edma.c
new file mode 100644
index 000..9dc27b4
--- /dev/null
+++ b/drivers/dma/fsl-edma.c
@@ -0,0 +1,939 @@
+/*
+ * drivers/dma/fsl-edma.c
+ *
+ * Cop
