[PATCH] dmaengine: fsl-edma: fixup reg offset and hw S/G support in big-endian model

[PATCH] dmaengine: fsl-edma: fixup reg offset and hw S/G support in big-endian model

[Jingchang Lu]

The offset of all 8-/16-bit register in big-endian eDMA model are
swapped in a 32-bit size opposite those in the little-endian model.

The hardware Scatter/Gather requires the subsequent TCDs in memory
to be auto loaded should retain the same endian as the core independent
of the register endian model, the dma engine will do the swap if need.

Signed-off-by: Jingchang Lu <jingchang.lu@freescale.com>
---
 drivers/dma/fsl-edma.c | 104 +++++++++++++++++++++++++++++++------------------
 1 file changed, 66 insertions(+), 38 deletions(-)

diff --git a/drivers/dma/fsl-edma.c b/drivers/dma/fsl-edma.c
index 3c5711d..499af8f 100644
--- a/drivers/dma/fsl-edma.c
+++ b/drivers/dma/fsl-edma.c
@@ -177,14 +177,20 @@ struct fsl_edma_engine {
 /*
  * R/W functions for big- or little-endian registers
  * the eDMA controller's endian is independent of the CPU core's endian.
+ * for the big-endian IP module, the offset for 8-bit or 16-bit register
+ * should also be swapped oposite to that in little-endian IP.
  */
 
 static u16 edma_readw(struct fsl_edma_engine *edma, void __iomem *addr)
 {
-	if (edma->big_endian)
-		return ioread16be(addr);
-	else
+	u32 dst;
+	/* swap the reg offset for these in big-endian mode */
+	if (edma->big_endian) {
+		dst = ((u32)addr & ~0x3) | (((u32)addr & 0x3) ^ 0x2);
+		return ioread16be((void __iomem *)dst);
+	} else {
 		return ioread16(addr);
+	}
 }
 
 static u32 edma_readl(struct fsl_edma_engine *edma, void __iomem *addr)
@@ -197,15 +203,26 @@ static u32 edma_readl(struct fsl_edma_engine *edma, void __iomem *addr)
 
 static void edma_writeb(struct fsl_edma_engine *edma, u8 val, void __iomem *addr)
 {
-	iowrite8(val, addr);
+	u32 dst;
+	/* swap the reg offset for these in big-endian mode */
+	if (edma->big_endian) {
+		dst = ((u32)addr & ~0x3) | (((u32)addr & 0x3) ^ 0x3);
+		iowrite8(val, (void __iomem *)dst);
+	} else {
+		iowrite8(val, addr);
+	}
 }
 
 static void edma_writew(struct fsl_edma_engine *edma, u16 val, void __iomem *addr)
 {
-	if (edma->big_endian)
-		iowrite16be(val, addr);
-	else
+	u32 dst;
+	/* swap the reg offset for these in big-endian mode */
+	if (edma->big_endian) {
+		dst = ((u32)addr & ~0x3) | (((u32)addr & 0x3) ^ 0x2);
+		iowrite16be(val, (void __iomem *)dst);
+	} else {
 		iowrite16(val, addr);
+	}
 }
 
 static void edma_writel(struct fsl_edma_engine *edma, u32 val, void __iomem *addr)
@@ -256,11 +273,10 @@ static void fsl_edma_chan_mux(struct fsl_edma_chan *fsl_chan,
 	muxaddr = fsl_chan->edma->muxbase[ch / chans_per_mux];
 
 	if (enable)
-		edma_writeb(fsl_chan->edma,
-				EDMAMUX_CHCFG_ENBL | EDMAMUX_CHCFG_SOURCE(slot),
+		writeb(EDMAMUX_CHCFG_ENBL | EDMAMUX_CHCFG_SOURCE(slot),
 				muxaddr + ch_off);
 	else
-		edma_writeb(fsl_chan->edma, EDMAMUX_CHCFG_DIS, muxaddr + ch_off);
+		writeb(EDMAMUX_CHCFG_DIS, muxaddr + ch_off);
 }
 
 static unsigned int fsl_edma_get_tcd_attr(enum dma_slave_buswidth addr_width)
@@ -433,21 +449,26 @@ static void fsl_edma_set_tcd_params(struct fsl_edma_chan *fsl_chan,
 	u32 ch = fsl_chan->vchan.chan.chan_id;
 
 	/*
-	 * TCD parameters have been swapped in fill_tcd_params(),
-	 * so just write them to registers in the cpu endian here
+	 * TCD parameters should be swapped according the eDMA
+	 * engine requirement.
 	 */
-	writew(0, addr + EDMA_TCD_CSR(ch));
-	writel(src, addr + EDMA_TCD_SADDR(ch));
-	writel(dst, addr + EDMA_TCD_DADDR(ch));
-	writew(attr, addr + EDMA_TCD_ATTR(ch));
-	writew(soff, addr + EDMA_TCD_SOFF(ch));
-	writel(nbytes, addr + EDMA_TCD_NBYTES(ch));
-	writel(slast, addr + EDMA_TCD_SLAST(ch));
-	writew(citer, addr + EDMA_TCD_CITER(ch));
-	writew(biter, addr + EDMA_TCD_BITER(ch));
-	writew(doff, addr + EDMA_TCD_DOFF(ch));
-	writel(dlast_sga, addr + EDMA_TCD_DLAST_SGA(ch));
-	writew(csr, addr + EDMA_TCD_CSR(ch));
+	edma_writew(fsl_chan->edma, 0, addr + EDMA_TCD_CSR(ch));
+	edma_writel(fsl_chan->edma, src, addr + EDMA_TCD_SADDR(ch));
+	edma_writel(fsl_chan->edma, dst, addr + EDMA_TCD_DADDR(ch));
+
+	edma_writew(fsl_chan->edma, attr, addr + EDMA_TCD_ATTR(ch));
+	edma_writew(fsl_chan->edma, soff, addr + EDMA_TCD_SOFF(ch));
+
+	edma_writel(fsl_chan->edma, nbytes, addr + EDMA_TCD_NBYTES(ch));
+	edma_writel(fsl_chan->edma, slast, addr + EDMA_TCD_SLAST(ch));
+
+	edma_writew(fsl_chan->edma, citer, addr + EDMA_TCD_CITER(ch));
+	edma_writew(fsl_chan->edma, biter, addr + EDMA_TCD_BITER(ch));
+	edma_writew(fsl_chan->edma, doff, addr + EDMA_TCD_DOFF(ch));
+
+	edma_writel(fsl_chan->edma, dlast_sga, addr + EDMA_TCD_DLAST_SGA(ch));
+
+	edma_writew(fsl_chan->edma, csr, addr + EDMA_TCD_CSR(ch));
 }
 
 static void fill_tcd_params(struct fsl_edma_engine *edma,
@@ -459,20 +480,27 @@ static void fill_tcd_params(struct fsl_edma_engine *edma,
 	u16 csr = 0;
 
 	/*
-	 * eDMA hardware SGs require the TCD parameters stored in memory
-	 * the same endian as the eDMA module so that they can be loaded
-	 * automatically by the engine
+	 * eDMA hardware SGs requires the TCDs to be auto loaded
+	 * in the same endian as the core whenver the eDAM engine's
+	 * register endian. So we don't swap the value, waitting
+	 * for fsl_set_tcd_params doing the swap.
 	 */
-	edma_writel(edma, src, &(tcd->saddr));
-	edma_writel(edma, dst, &(tcd->daddr));
-	edma_writew(edma, attr, &(tcd->attr));
-	edma_writew(edma, EDMA_TCD_SOFF_SOFF(soff), &(tcd->soff));
-	edma_writel(edma, EDMA_TCD_NBYTES_NBYTES(nbytes), &(tcd->nbytes));
-	edma_writel(edma, EDMA_TCD_SLAST_SLAST(slast), &(tcd->slast));
-	edma_writew(edma, EDMA_TCD_CITER_CITER(citer), &(tcd->citer));
-	edma_writew(edma, EDMA_TCD_DOFF_DOFF(doff), &(tcd->doff));
-	edma_writel(edma, EDMA_TCD_DLAST_SGA_DLAST_SGA(dlast_sga), &(tcd->dlast_sga));
-	edma_writew(edma, EDMA_TCD_BITER_BITER(biter), &(tcd->biter));
+	writel(src, &(tcd->saddr));
+	writel(dst, &(tcd->daddr));
+
+	writew(attr, &(tcd->attr));
+
+	writew(EDMA_TCD_SOFF_SOFF(soff), &(tcd->soff));
+
+	writel(EDMA_TCD_NBYTES_NBYTES(nbytes), &(tcd->nbytes));
+	writel(EDMA_TCD_SLAST_SLAST(slast), &(tcd->slast));
+
+	writew(EDMA_TCD_CITER_CITER(citer), &(tcd->citer));
+	writew(EDMA_TCD_DOFF_DOFF(doff), &(tcd->doff));
+
+	writel(EDMA_TCD_DLAST_SGA_DLAST_SGA(dlast_sga), &(tcd->dlast_sga));
+
+	writew(EDMA_TCD_BITER_BITER(biter), &(tcd->biter));
 	if (major_int)
 		csr |= EDMA_TCD_CSR_INT_MAJOR;
 
@@ -482,7 +510,7 @@ static void fill_tcd_params(struct fsl_edma_engine *edma,
 	if (enable_sg)
 		csr |= EDMA_TCD_CSR_E_SG;
 
-	edma_writew(edma, csr, &(tcd->csr));
+	writew(csr, &(tcd->csr));
 }
 
 static struct fsl_edma_desc *fsl_edma_alloc_desc(struct fsl_edma_chan *fsl_chan,
-- 
1.8.0

[PATCH] dmaengine: fsl-edma: fixup reg offset and hw S/G support in big-endian model

[Russell King - ARM Linux]

On Tue, Sep 23, 2014 at 05:15:19PM +0800, Jingchang Lu wrote:
>  static u16 edma_readw(struct fsl_edma_engine *edma, void __iomem *addr)
>  {
> -	if (edma->big_endian)
> -		return ioread16be(addr);
> -	else
> +	u32 dst;

This should be unsigned long, if it needs to exist.

> +	/* swap the reg offset for these in big-endian mode */
> +	if (edma->big_endian) {
> +		dst = ((u32)addr & ~0x3) | (((u32)addr & 0x3) ^ 0x2);

This can be simplified to:

		addr = (void __iomem *)((unsigned long)addr ^ 2);

Ditto for all the other cases.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

[PATCH] dmaengine: fsl-edma: fixup reg offset and hw S/G support in big-endian model

[Arnd Bergmann]

On Tuesday 23 September 2014 17:15:19 Jingchang Lu wrote:
> @@ -459,20 +480,27 @@ static void fill_tcd_params(struct fsl_edma_engine *edma,
>         u16 csr = 0;
>  
>         /*
> -        * eDMA hardware SGs require the TCD parameters stored in memory
> -        * the same endian as the eDMA module so that they can be loaded
> -        * automatically by the engine
> +        * eDMA hardware SGs requires the TCDs to be auto loaded
> +        * in the same endian as the core whenver the eDAM engine's
> +        * register endian. So we don't swap the value, waitting
> +        * for fsl_set_tcd_params doing the swap.
>          */

This makes no sense: how would the eDMA hardware know what endianess
the CPU is using to write this data? Have you tried this running on
the same hardware with both big-endian and little-endian kernels?

Also, you access this as little-endian data unconditionally rather
than cpu-endian as the comment suggests, maybe that is what you meant
here?

> -       edma_writel(edma, src, &(tcd->saddr));
> -       edma_writel(edma, dst, &(tcd->daddr));
> -       edma_writew(edma, attr, &(tcd->attr));
> -       edma_writew(edma, EDMA_TCD_SOFF_SOFF(soff), &(tcd->soff));
> -       edma_writel(edma, EDMA_TCD_NBYTES_NBYTES(nbytes), &(tcd->nbytes));
> -       edma_writel(edma, EDMA_TCD_SLAST_SLAST(slast), &(tcd->slast));
> -       edma_writew(edma, EDMA_TCD_CITER_CITER(citer), &(tcd->citer));
> -       edma_writew(edma, EDMA_TCD_DOFF_DOFF(doff), &(tcd->doff));
> -       edma_writel(edma, EDMA_TCD_DLAST_SGA_DLAST_SGA(dlast_sga), &(tcd->dlast_sga));
> -       edma_writew(edma, EDMA_TCD_BITER_BITER(biter), &(tcd->biter));
> +       writel(src, &(tcd->saddr));
> +       writel(dst, &(tcd->daddr));
> +
> +       writew(attr, &(tcd->attr));

You seem to have another preexisting bug: The tcd pointer actually does not
point to mmio registers here at all, but instead points to memory that
has been returned from dma_pool_alloc. It is not valid to use writel()
on such memory, that is only meant to work on real MMIO registers.

You should use regular assignments to the registers here, e.g.
	
	tcd->saddr = cpu_to_le32(src);
	tcd->daddr = cpu_to_le32(dst);
	...

	Arnd

[PATCH] dmaengine: fsl-edma: fixup reg offset and hw S/G support in big-endian model

[Jingchang Lu]

________________________________________
From: Arnd Bergmann <arnd@arndb.de>
Sent: Tuesday, September 23, 2014 8:30 PM
To: linux-arm-kernel at lists.infradead.org
Cc: Lu Jingchang-B35083; vinod.koul at intel.com; dmaengine at vger.kernel.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH] dmaengine: fsl-edma: fixup reg offset and hw S/G support in big-endian model

On Tuesday 23 September 2014 17:15:19 Jingchang Lu wrote:
> @@ -459,20 +480,27 @@ static void fill_tcd_params(struct fsl_edma_engine *edma,
>         u16 csr = 0;
>
>         /*
> -        * eDMA hardware SGs require the TCD parameters stored in memory
> -        * the same endian as the eDMA module so that they can be loaded
> -        * automatically by the engine
> +        * eDMA hardware SGs requires the TCDs to be auto loaded
> +        * in the same endian as the core whenver the eDAM engine's
> +        * register endian. So we don't swap the value, waitting
> +        * for fsl_set_tcd_params doing the swap.
>          */

This makes no sense: how would the eDMA hardware know what endianess
the CPU is using to write this data? Have you tried this running on
the same hardware with both big-endian and little-endian kernels?

Also, you access this as little-endian data unconditionally rather
than cpu-endian as the comment suggests, maybe that is what you meant
here?

I will rewrite this comment, thanks.

> -       edma_writel(edma, src, &(tcd->saddr));
> -       edma_writel(edma, dst, &(tcd->daddr));
> -       edma_writew(edma, attr, &(tcd->attr));
> -       edma_writew(edma, EDMA_TCD_SOFF_SOFF(soff), &(tcd->soff));
> -       edma_writel(edma, EDMA_TCD_NBYTES_NBYTES(nbytes), &(tcd->nbytes));
> -       edma_writel(edma, EDMA_TCD_SLAST_SLAST(slast), &(tcd->slast));
> -       edma_writew(edma, EDMA_TCD_CITER_CITER(citer), &(tcd->citer));
> -       edma_writew(edma, EDMA_TCD_DOFF_DOFF(doff), &(tcd->doff));
> -       edma_writel(edma, EDMA_TCD_DLAST_SGA_DLAST_SGA(dlast_sga), &(tcd->dlast_sga));
> -       edma_writew(edma, EDMA_TCD_BITER_BITER(biter), &(tcd->biter));
> +       writel(src, &(tcd->saddr));
> +       writel(dst, &(tcd->daddr));
> +
> +       writew(attr, &(tcd->attr));

You seem to have another preexisting bug: The tcd pointer actually does not
point to mmio registers here at all, but instead points to memory that
has been returned from dma_pool_alloc. It is not valid to use writel()
on such memory, that is only meant to work on real MMIO registers.

You should use regular assignments to the registers here, e.g.

        tcd->saddr = cpu_to_le32(src);
        tcd->daddr = cpu_to_le32(dst);
        ...

        Arnd

I will reconsider this setting. BTW, why writel() can't be used for memory location since it's still mapped and registers space is
also memory mapped? Thanks.

Best Regards,
Jingchang

[PATCH] dmaengine: fsl-edma: fixup reg offset and hw S/G support in big-endian model

[Jingchang Lu]

_______________________________________
From: Russell King - ARM Linux <linux@arm.linux.org.uk>
Sent: Tuesday, September 23, 2014 6:37 PM
To: Lu Jingchang-B35083
Cc: vinod.koul at intel.com; dmaengine at vger.kernel.org; linux-kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH] dmaengine: fsl-edma: fixup reg offset and hw S/G   support in big-endian model

On Tue, Sep 23, 2014 at 05:15:19PM +0800, Jingchang Lu wrote:
>  static u16 edma_readw(struct fsl_edma_engine *edma, void __iomem *addr)
>  {
> -     if (edma->big_endian)
> -             return ioread16be(addr);
> -     else
> +     u32 dst;

This should be unsigned long, if it needs to exist.

> +     /* swap the reg offset for these in big-endian mode */
> +     if (edma->big_endian) {
> +             dst = ((u32)addr & ~0x3) | (((u32)addr & 0x3) ^ 0x2);

This can be simplified to:

                addr = (void __iomem *)((unsigned long)addr ^ 2);

Ditto for all the other cases.

I will do that, Thanks.

Best Regards,
Jingchang

[PATCH] dmaengine: fsl-edma: fixup reg offset and hw S/G support in big-endian model

[Arnd Bergmann]

On Tuesday 23 September 2014 17:18:43 Jingchang Lu wrote:
> I will reconsider this setting. BTW, why writel() can't be used
> for memory location since it's still mapped and registers space is
> also memory mapped? Thanks.

You cannot rely on an '__iomem' token to actually be a pointer, even
if that is currently the case on ARM.

On other architectures, the bits in the pointer can encode something
completely different, e.g. it may be the physical address on
architectures that have instructions to load/store on that directly,
and ioremap() in that case returns an identity mapping.

If you install sparse and build with 'make C=1', you will in fact
get a warning about the code when you assign a regular pointer to
an __iomem pointer or vice versa.

	Arnd