diff mbox

[v3] dmaengine: xgene-dma: Fix sparse wannings and coccinelle warnings

Message ID 1429212673-22254-1-git-send-email-rsahu@apm.com (mailing list archive)
State Superseded
Headers show

Commit Message

Rameshwar Prasad Sahu April 16, 2015, 7:31 p.m. UTC 
v3 changes:
	* Minor changes in length setting in DMA descriptor

v2 changes:
	* Code cleanup
	* Changed way of setting DMA descriptors for big-endian

This patch fixes compilation sparse warnings like incorrect type in assignment
(different base types), cast to restricted __le64, symbol
'__UNIQUE_ID_author__COUNTER__' has multiple initializers etc and
coccinelle warnings (No need to set .owner here. The core will do it.)

This patch is based on slave-dma / for-linus branch.
(commit: 9f2fd0dfa594d857fbdaeda523ff7a46f16567f5 [26/28]
	dmaengine: Add support for APM X-Gene SoC DMA engine driver)

Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: Rameshwar Prasad Sahu <rsahu@apm.com>
---
 drivers/dma/xgene-dma.c | 191 +++++++++++++++++++-----------------------------
 1 file changed, 74 insertions(+), 117 deletions(-)

--
1.8.2.1

--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Arnd Bergmann April 17, 2015, 8:49 a.m. UTC | #1 
On Friday 17 April 2015 01:01:13 Rameshwar Prasad Sahu wrote:
> v3 changes:
> 	* Minor changes in length setting in DMA descriptor
> 
> v2 changes:
> 	* Code cleanup
> 	* Changed way of setting DMA descriptors for big-endian
> 
> This patch fixes compilation sparse warnings like incorrect type in assignment
> (different base types), cast to restricted __le64, symbol
> '__UNIQUE_ID_author__COUNTER__' has multiple initializers etc and
> coccinelle warnings (No need to set .owner here. The core will do it.)
> 
> This patch is based on slave-dma / for-linus branch.
> (commit: 9f2fd0dfa594d857fbdaeda523ff7a46f16567f5 [26/28]
> 	dmaengine: Add support for APM X-Gene SoC DMA engine driver)
> 
> Reported-by: kbuild test robot <fengguang.wu@intel.com>
> Signed-off-by: Rameshwar Prasad Sahu <rsahu@apm.com>

Looks good now,

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

There is one small enhancement that you could still do and I'll shut up after
that ;-)

> 
> -static void *xgene_dma_lookup_ext8(u64 *desc, int idx)
> +static __le64 *xgene_dma_lookup_ext8(struct xgene_dma_desc_hw *desc, int idx)
>  {
> -	return (idx % 2) ? (desc + idx - 1) : (desc + idx + 1);
> +	switch (idx) {
> +	case 0:
> +		return &desc->m1;
> +	case 1:
> +		return &desc->m0;
> +	case 2:
> +		return &desc->m3;
> +	case 3:
> +		return &desc->m2;
> +	default:
> +		pr_err("Invalid dma descriptor index\n");
> +	}
> +
> +	return NULL;
>  }
> 
...

>  	/* Set 1st to 5th source addresses */
>  	for (i = 0; i < src_cnt; i++) {
>  		len = *nbytes;
> -		xgene_dma_set_src_buffer((i == 0) ? (desc1 + 8) :
> +		xgene_dma_set_src_buffer((i == 0) ? &desc1->m1 :
>  					 xgene_dma_lookup_ext8(desc2, i - 1),
>  					 &len, &src[i]);
> -		XGENE_DMA_DESC_MULTI_SET(desc1, scf[i], i);
> +		desc1->m2 |= cpu_to_le64((scf[i] << ((i + 1) * 8)));
>  	}

If you just unroll this loop, you get code that is smaller and easier to
understand:

	/* Set 1st to 5th source addresses */
	xgene_dma_set_src_buffer(&desc1->m1, &len, &src[0]);
	xgene_dma_set_src_buffer(&desc2->m0, &len, &src[1]);
	xgene_dma_set_src_buffer(&desc2->m3, &len, &src[2]);
	xgene_dma_set_src_buffer(&desc2->m2, &len, &src[3]);
	desc1->m2 |= cpu_to_le64(scf[0] | scf[1] << 8 | scf[2] << 16 | scf[3] << 24);

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rameshwar Prasad Sahu April 17, 2015, 9:11 a.m. UTC | #2 
Hi Arnd,

On Fri, Apr 17, 2015 at 2:19 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 17 April 2015 01:01:13 Rameshwar Prasad Sahu wrote:
>> v3 changes:
>>       * Minor changes in length setting in DMA descriptor
>>
>> v2 changes:
>>       * Code cleanup
>>       * Changed way of setting DMA descriptors for big-endian
>>
>> This patch fixes compilation sparse warnings like incorrect type in assignment
>> (different base types), cast to restricted __le64, symbol
>> '__UNIQUE_ID_author__COUNTER__' has multiple initializers etc and
>> coccinelle warnings (No need to set .owner here. The core will do it.)
>>
>> This patch is based on slave-dma / for-linus branch.
>> (commit: 9f2fd0dfa594d857fbdaeda523ff7a46f16567f5 [26/28]
>>       dmaengine: Add support for APM X-Gene SoC DMA engine driver)
>>
>> Reported-by: kbuild test robot <fengguang.wu@intel.com>
>> Signed-off-by: Rameshwar Prasad Sahu <rsahu@apm.com>
>
> Looks good now,
>
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
>
> There is one small enhancement that you could still do and I'll shut up after
> that ;-)
>
>>
>> -static void *xgene_dma_lookup_ext8(u64 *desc, int idx)
>> +static __le64 *xgene_dma_lookup_ext8(struct xgene_dma_desc_hw *desc, int idx)
>>  {
>> -     return (idx % 2) ? (desc + idx - 1) : (desc + idx + 1);
>> +     switch (idx) {
>> +     case 0:
>> +             return &desc->m1;
>> +     case 1:
>> +             return &desc->m0;
>> +     case 2:
>> +             return &desc->m3;
>> +     case 3:
>> +             return &desc->m2;
>> +     default:
>> +             pr_err("Invalid dma descriptor index\n");
>> +     }
>> +
>> +     return NULL;
>>  }
>>
> ...
>
>>       /* Set 1st to 5th source addresses */
>>       for (i = 0; i < src_cnt; i++) {
>>               len = *nbytes;
>> -             xgene_dma_set_src_buffer((i == 0) ? (desc1 + 8) :
>> +             xgene_dma_set_src_buffer((i == 0) ? &desc1->m1 :
>>                                        xgene_dma_lookup_ext8(desc2, i - 1),
>>                                        &len, &src[i]);
>> -             XGENE_DMA_DESC_MULTI_SET(desc1, scf[i], i);
>> +             desc1->m2 |= cpu_to_le64((scf[i] << ((i + 1) * 8)));
>>       }
>
> If you just unroll this loop, you get code that is smaller and easier to
> understand:
>
>         /* Set 1st to 5th source addresses */
>         xgene_dma_set_src_buffer(&desc1->m1, &len, &src[0]);
>         xgene_dma_set_src_buffer(&desc2->m0, &len, &src[1]);
>         xgene_dma_set_src_buffer(&desc2->m3, &len, &src[2]);
>         xgene_dma_set_src_buffer(&desc2->m2, &len, &src[3]);
>         desc1->m2 |= cpu_to_le64(scf[0] | scf[1] << 8 | scf[2] << 16 | scf[3] << 24);

Actually here, in run time src_cnt value can be 2 or 3 upto 5, so we
can't unroll it.


>
>         Arnd
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann April 17, 2015, 9:56 a.m. UTC | #3 
On Friday 17 April 2015 14:41:02 Rameshwar Sahu wrote:
> >>
> >> -static void *xgene_dma_lookup_ext8(u64 *desc, int idx)
> >> +static __le64 *xgene_dma_lookup_ext8(struct xgene_dma_desc_hw *desc, int idx)
> >>  {
> >> -     return (idx % 2) ? (desc + idx - 1) : (desc + idx + 1);
> >> +     switch (idx) {
> >> +     case 0:
> >> +             return &desc->m1;
> >> +     case 1:
> >> +             return &desc->m0;
> >> +     case 2:
> >> +             return &desc->m3;
> >> +     case 3:
> >> +             return &desc->m2;
> >> +     default:
> >> +             pr_err("Invalid dma descriptor index\n");
> >> +     }
> >> +
> >> +     return NULL;
> >>  }
> >>
> > ...
> >
> >>       /* Set 1st to 5th source addresses */
> >>       for (i = 0; i < src_cnt; i++) {
> >>               len = *nbytes;
> >> -             xgene_dma_set_src_buffer((i == 0) ? (desc1 + 8) :
> >> +             xgene_dma_set_src_buffer((i == 0) ? &desc1->m1 :
> >>                                        xgene_dma_lookup_ext8(desc2, i - 1),
> >>                                        &len, &src[i]);
> >> -             XGENE_DMA_DESC_MULTI_SET(desc1, scf[i], i);
> >> +             desc1->m2 |= cpu_to_le64((scf[i] << ((i + 1) * 8)));
> >>       }
> >
> > If you just unroll this loop, you get code that is smaller and easier to
> > understand:
> >
> >         /* Set 1st to 5th source addresses */
> >         xgene_dma_set_src_buffer(&desc1->m1, &len, &src[0]);
> >         xgene_dma_set_src_buffer(&desc2->m0, &len, &src[1]);
> >         xgene_dma_set_src_buffer(&desc2->m3, &len, &src[2]);
> >         xgene_dma_set_src_buffer(&desc2->m2, &len, &src[3]);
> >         desc1->m2 |= cpu_to_le64(scf[0] | scf[1] << 8 | scf[2] << 16 | scf[3] << 24);
> 
> Actually here, in run time src_cnt value can be 2 or 3 upto 5, so we
> can't unroll it.

I see, sorry for misreading the code. The patch is good then as it is.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vinod Koul April 17, 2015, 6:29 p.m. UTC | #4 
>  	/* Get DMA error interrupt */
> @@ -2076,7 +2035,6 @@ static struct platform_driver xgene_dma_driver = {
>  	.remove = xgene_dma_remove,
>  	.driver = {
>  		.name = "X-Gene-DMA",
> -		.owner = THIS_MODULE,
I have already applied a patch for this

>  		.of_match_table = xgene_dma_of_match_ptr,
>  	},
>  };
> @@ -2085,6 +2043,5 @@ module_platform_driver(xgene_dma_driver);
> 
>  MODULE_DESCRIPTION("APM X-Gene SoC DMA driver");
>  MODULE_AUTHOR("Rameshwar Prasad Sahu <rsahu@apm.com>");
> -MODULE_AUTHOR("Loc Ho <lho@apm.com>");
And why this?

Fixes looks good though
Rameshwar Prasad Sahu April 20, 2015, 3:08 a.m. UTC | #5 
Hi Vinod,

On Fri, Apr 17, 2015 at 11:59 PM, Vinod Koul <vinod.koul@intel.com> wrote:
>
>>       /* Get DMA error interrupt */
>> @@ -2076,7 +2035,6 @@ static struct platform_driver xgene_dma_driver = {
>>       .remove = xgene_dma_remove,
>>       .driver = {
>>               .name = "X-Gene-DMA",
>> -             .owner = THIS_MODULE,
> I have already applied a patch for this

Okay, I didn't pull latest git

>
>>               .of_match_table = xgene_dma_of_match_ptr,
>>       },
>>  };
>> @@ -2085,6 +2043,5 @@ module_platform_driver(xgene_dma_driver);
>>
>>  MODULE_DESCRIPTION("APM X-Gene SoC DMA driver");
>>  MODULE_AUTHOR("Rameshwar Prasad Sahu <rsahu@apm.com>");
>> -MODULE_AUTHOR("Loc Ho <lho@apm.com>");
> And why this?

I saw below warning reported by the kbuild robot test

drivers/dma/xgene-dma.c:2088:1: sparse: symbol
'__UNIQUE_ID_author__COUNTER__' has multiple initializers (originally
initialized at drivers/dma/xgene-dma.c:2087)
So, I kept only one author here.


>
> Fixes looks good though
>
> --
> ~Vinod
>>  MODULE_LICENSE("GPL");
>>  MODULE_VERSION("1.0");
>> --
>> 1.8.2.1
>>
>
> --
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vinod Koul April 27, 2015, 3:13 a.m. UTC | #6 
On Mon, Apr 20, 2015 at 08:38:18AM +0530, Rameshwar Sahu wrote:
> Hi Vinod,
>> >> @@ -2085,6 +2043,5 @@ module_platform_driver(xgene_dma_driver);
> >>
> >>  MODULE_DESCRIPTION("APM X-Gene SoC DMA driver");
> >>  MODULE_AUTHOR("Rameshwar Prasad Sahu <rsahu@apm.com>");
> >> -MODULE_AUTHOR("Loc Ho <lho@apm.com>");
> > And why this?
> 
> I saw below warning reported by the kbuild robot test
> 
> drivers/dma/xgene-dma.c:2088:1: sparse: symbol
> '__UNIQUE_ID_author__COUNTER__' has multiple initializers (originally
> initialized at drivers/dma/xgene-dma.c:2087)
> So, I kept only one author here.
No that is not right, sparse shouldn't have cribbed here.

Fengguang can we get the bot to ignore this please
Fengguang Wu April 27, 2015, 5:20 a.m. UTC | #7 
On Mon, Apr 27, 2015 at 08:43:15AM +0530, Vinod Koul wrote:
> On Mon, Apr 20, 2015 at 08:38:18AM +0530, Rameshwar Sahu wrote:
> > Hi Vinod,
> >> >> @@ -2085,6 +2043,5 @@ module_platform_driver(xgene_dma_driver);
> > >>
> > >>  MODULE_DESCRIPTION("APM X-Gene SoC DMA driver");
> > >>  MODULE_AUTHOR("Rameshwar Prasad Sahu <rsahu@apm.com>");
> > >> -MODULE_AUTHOR("Loc Ho <lho@apm.com>");
> > > And why this?
> > 
> > I saw below warning reported by the kbuild robot test
> > 
> > drivers/dma/xgene-dma.c:2088:1: sparse: symbol
> > '__UNIQUE_ID_author__COUNTER__' has multiple initializers (originally
> > initialized at drivers/dma/xgene-dma.c:2087)
> > So, I kept only one author here.
> No that is not right, sparse shouldn't have cribbed here.
> 
> Fengguang can we get the bot to ignore this please

OK, sorry for the noises! CC sparse maintainer btw.

Thanks,
Fengguang
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christopher Li June 12, 2015, 12:36 a.m. UTC | #8 
On Sun, Apr 26, 2015 at 10:20 PM, Fengguang Wu <fengguang.wu@intel.com> wrote:
>> >
>> > drivers/dma/xgene-dma.c:2088:1: sparse: symbol
>> > '__UNIQUE_ID_author__COUNTER__' has multiple initializers (originally
>> > initialized at drivers/dma/xgene-dma.c:2087)
>> > So, I kept only one author here.
>> No that is not right, sparse shouldn't have cribbed here.
>>
>> Fengguang can we get the bot to ignore this please

Sorry for the late reply.

That looks like the __COUNTER__ macro feature. It has been implemented in
sparse git tree already.

Chris
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fengguang Wu June 12, 2015, 1 a.m. UTC | #9 
On Thu, Jun 11, 2015 at 05:36:00PM -0700, Christopher Li wrote:
> n Sun, Apr 26, 2015 at 10:20 PM, Fengguang Wu <fengguang.wu@intel.com> wrote:
> >> >
> >> > drivers/dma/xgene-dma.c:2088:1: sparse: symbol
> >> > '__UNIQUE_ID_author__COUNTER__' has multiple initializers (originally
> >> > initialized at drivers/dma/xgene-dma.c:2087)
> >> > So, I kept only one author here.
> >> No that is not right, sparse shouldn't have cribbed here.
> >>
> >> Fengguang can we get the bot to ignore this please
> 
> Sorry for the late reply.
> 
> That looks like the __COUNTER__ macro feature. It has been implemented in
> sparse git tree already.

That's great, I'll fetch the latest sparse code from

git://git.kernel.org/pub/scm/devel/sparse/chrisl/sparse.git

Thanks,
Fengguang
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/dma/xgene-dma.c b/drivers/dma/xgene-dma.c
index aa61935..722c684 100755
--- a/drivers/dma/xgene-dma.c
+++ b/drivers/dma/xgene-dma.c
@@ -124,32 +124,8 @@ 
 #define XGENE_DMA_DESC_ELERR_POS		46
 #define XGENE_DMA_DESC_RTYPE_POS		56
 #define XGENE_DMA_DESC_LERR_POS			60
-#define XGENE_DMA_DESC_FLYBY_POS		4
 #define XGENE_DMA_DESC_BUFLEN_POS		48
 #define XGENE_DMA_DESC_HOENQ_NUM_POS		48
-
-#define XGENE_DMA_DESC_NV_SET(m)		\
-	(((u64 *)(m))[0] |= XGENE_DMA_DESC_NV_BIT)
-#define XGENE_DMA_DESC_IN_SET(m)		\
-	(((u64 *)(m))[0] |= XGENE_DMA_DESC_IN_BIT)
-#define XGENE_DMA_DESC_RTYPE_SET(m, v)		\
-	(((u64 *)(m))[0] |= ((u64)(v) << XGENE_DMA_DESC_RTYPE_POS))
-#define XGENE_DMA_DESC_BUFADDR_SET(m, v)	\
-	(((u64 *)(m))[0] |= (v))
-#define XGENE_DMA_DESC_BUFLEN_SET(m, v)		\
-	(((u64 *)(m))[0] |= ((u64)(v) << XGENE_DMA_DESC_BUFLEN_POS))
-#define XGENE_DMA_DESC_C_SET(m)			\
-	(((u64 *)(m))[1] |= XGENE_DMA_DESC_C_BIT)
-#define XGENE_DMA_DESC_FLYBY_SET(m, v)		\
-	(((u64 *)(m))[2] |= ((v) << XGENE_DMA_DESC_FLYBY_POS))
-#define XGENE_DMA_DESC_MULTI_SET(m, v, i)	\
-	(((u64 *)(m))[2] |= ((u64)(v) << (((i) + 1) * 8)))
-#define XGENE_DMA_DESC_DR_SET(m)		\
-	(((u64 *)(m))[2] |= XGENE_DMA_DESC_DR_BIT)
-#define XGENE_DMA_DESC_DST_ADDR_SET(m, v)	\
-	(((u64 *)(m))[3] |= (v))
-#define XGENE_DMA_DESC_H0ENQ_NUM_SET(m, v)	\
-	(((u64 *)(m))[3] |= ((u64)(v) << XGENE_DMA_DESC_HOENQ_NUM_POS))
 #define XGENE_DMA_DESC_ELERR_RD(m)		\
 	(((m) >> XGENE_DMA_DESC_ELERR_POS) & 0x3)
 #define XGENE_DMA_DESC_LERR_RD(m)		\
@@ -158,14 +134,7 @@ 
 	(((elerr) << 4) | (lerr))

 /* X-Gene DMA descriptor empty s/w signature */
-#define XGENE_DMA_DESC_EMPTY_INDEX		0
 #define XGENE_DMA_DESC_EMPTY_SIGNATURE		~0ULL
-#define XGENE_DMA_DESC_SET_EMPTY(m)		\
-	(((u64 *)(m))[XGENE_DMA_DESC_EMPTY_INDEX] =	\
-	 XGENE_DMA_DESC_EMPTY_SIGNATURE)
-#define XGENE_DMA_DESC_IS_EMPTY(m)		\
-	(((u64 *)(m))[XGENE_DMA_DESC_EMPTY_INDEX] ==	\
-	 XGENE_DMA_DESC_EMPTY_SIGNATURE)

 /* X-Gene DMA configurable parameters defines */
 #define XGENE_DMA_RING_NUM		512
@@ -184,7 +153,7 @@ 
 #define XGENE_DMA_XOR_ALIGNMENT		6	/* 64 Bytes */
 #define XGENE_DMA_MAX_XOR_SRC		5
 #define XGENE_DMA_16K_BUFFER_LEN_CODE	0x0
-#define XGENE_DMA_INVALID_LEN_CODE	0x7800
+#define XGENE_DMA_INVALID_LEN_CODE	0x7800000000000000ULL

 /* X-Gene DMA descriptor error codes */
 #define ERR_DESC_AXI			0x01
@@ -214,10 +183,10 @@ 
 #define ERR_DESC_SRC_INT		0xB

 /* X-Gene DMA flyby operation code */
-#define FLYBY_2SRC_XOR			0x8
-#define FLYBY_3SRC_XOR			0x9
-#define FLYBY_4SRC_XOR			0xA
-#define FLYBY_5SRC_XOR			0xB
+#define FLYBY_2SRC_XOR			0x80
+#define FLYBY_3SRC_XOR			0x90
+#define FLYBY_4SRC_XOR			0xA0
+#define FLYBY_5SRC_XOR			0xB0

 /* X-Gene DMA SW descriptor flags */
 #define XGENE_DMA_FLAG_64B_DESC		BIT(0)
@@ -238,10 +207,10 @@ 
 	dev_err(chan->dev, "%s: " fmt, chan->name, ##arg)

 struct xgene_dma_desc_hw {
-	u64 m0;
-	u64 m1;
-	u64 m2;
-	u64 m3;
+	__le64 m0;
+	__le64 m1;
+	__le64 m2;
+	__le64 m3;
 };

 enum xgene_dma_ring_cfgsize {
@@ -388,18 +357,11 @@  static bool is_pq_enabled(struct xgene_dma *pdma)
 	return !(val & XGENE_DMA_PQ_DISABLE_MASK);
 }

-static void xgene_dma_cpu_to_le64(u64 *desc, int count)
-{
-	int i;
-
-	for (i = 0; i < count; i++)
-		desc[i] = cpu_to_le64(desc[i]);
-}
-
-static u16 xgene_dma_encode_len(u32 len)
+static u64 xgene_dma_encode_len(size_t len)
 {
 	return (len < XGENE_DMA_MAX_BYTE_CNT) ?
-		len : XGENE_DMA_16K_BUFFER_LEN_CODE;
+		((u64)len << XGENE_DMA_DESC_BUFLEN_POS) :
+		XGENE_DMA_16K_BUFFER_LEN_CODE;
 }

 static u8 xgene_dma_encode_xor_flyby(u32 src_cnt)
@@ -424,34 +386,50 @@  static u32 xgene_dma_ring_desc_cnt(struct xgene_dma_ring *ring)
 	return XGENE_DMA_RING_DESC_CNT(ring_state);
 }

-static void xgene_dma_set_src_buffer(void *ext8, size_t *len,
+static void xgene_dma_set_src_buffer(__le64 *ext8, size_t *len,
 				     dma_addr_t *paddr)
 {
 	size_t nbytes = (*len < XGENE_DMA_MAX_BYTE_CNT) ?
 			*len : XGENE_DMA_MAX_BYTE_CNT;

-	XGENE_DMA_DESC_BUFADDR_SET(ext8, *paddr);
-	XGENE_DMA_DESC_BUFLEN_SET(ext8, xgene_dma_encode_len(nbytes));
+	*ext8 |= cpu_to_le64(*paddr);
+	*ext8 |= cpu_to_le64(xgene_dma_encode_len(nbytes));
 	*len -= nbytes;
 	*paddr += nbytes;
 }

-static void xgene_dma_invalidate_buffer(void *ext8)
+static void xgene_dma_invalidate_buffer(__le64 *ext8)
 {
-	XGENE_DMA_DESC_BUFLEN_SET(ext8, XGENE_DMA_INVALID_LEN_CODE);
+	*ext8 |= cpu_to_le64(XGENE_DMA_INVALID_LEN_CODE);
 }

-static void *xgene_dma_lookup_ext8(u64 *desc, int idx)
+static __le64 *xgene_dma_lookup_ext8(struct xgene_dma_desc_hw *desc, int idx)
 {
-	return (idx % 2) ? (desc + idx - 1) : (desc + idx + 1);
+	switch (idx) {
+	case 0:
+		return &desc->m1;
+	case 1:
+		return &desc->m0;
+	case 2:
+		return &desc->m3;
+	case 3:
+		return &desc->m2;
+	default:
+		pr_err("Invalid dma descriptor index\n");
+	}
+
+	return NULL;
 }

-static void xgene_dma_init_desc(void *desc, u16 dst_ring_num)
+static void xgene_dma_init_desc(struct xgene_dma_desc_hw *desc,
+				u16 dst_ring_num)
 {
-	XGENE_DMA_DESC_C_SET(desc); /* Coherent IO */
-	XGENE_DMA_DESC_IN_SET(desc);
-	XGENE_DMA_DESC_H0ENQ_NUM_SET(desc, dst_ring_num);
-	XGENE_DMA_DESC_RTYPE_SET(desc, XGENE_DMA_RING_OWNER_DMA);
+	desc->m0 |= cpu_to_le64(XGENE_DMA_DESC_IN_BIT);
+	desc->m0 |= cpu_to_le64((u64)XGENE_DMA_RING_OWNER_DMA <<
+				XGENE_DMA_DESC_RTYPE_POS);
+	desc->m1 |= cpu_to_le64(XGENE_DMA_DESC_C_BIT);
+	desc->m3 |= cpu_to_le64((u64)dst_ring_num <<
+				XGENE_DMA_DESC_HOENQ_NUM_POS);
 }

 static void xgene_dma_prep_cpy_desc(struct xgene_dma_chan *chan,
@@ -459,7 +437,7 @@  static void xgene_dma_prep_cpy_desc(struct xgene_dma_chan *chan,
 				    dma_addr_t dst, dma_addr_t src,
 				    size_t len)
 {
-	void *desc1, *desc2;
+	struct xgene_dma_desc_hw *desc1, *desc2;
 	int i;

 	/* Get 1st descriptor */
@@ -467,23 +445,21 @@  static void xgene_dma_prep_cpy_desc(struct xgene_dma_chan *chan,
 	xgene_dma_init_desc(desc1, chan->tx_ring.dst_ring_num);

 	/* Set destination address */
-	XGENE_DMA_DESC_DR_SET(desc1);
-	XGENE_DMA_DESC_DST_ADDR_SET(desc1, dst);
+	desc1->m2 |= cpu_to_le64(XGENE_DMA_DESC_DR_BIT);
+	desc1->m3 |= cpu_to_le64(dst);

 	/* Set 1st source address */
-	xgene_dma_set_src_buffer(desc1 + 8, &len, &src);
+	xgene_dma_set_src_buffer(&desc1->m1, &len, &src);

-	if (len <= 0) {
-		desc2 = NULL;
-		goto skip_additional_src;
-	}
+	if (!len)
+		return;

 	/*
 	 * We need to split this source buffer,
 	 * and need to use 2nd descriptor
 	 */
 	desc2 = &desc_sw->desc2;
-	XGENE_DMA_DESC_NV_SET(desc1);
+	desc1->m0 |= cpu_to_le64(XGENE_DMA_DESC_NV_BIT);

 	/* Set 2nd to 5th source address */
 	for (i = 0; i < 4 && len; i++)
@@ -496,12 +472,6 @@  static void xgene_dma_prep_cpy_desc(struct xgene_dma_chan *chan,

 	/* Updated flag that we have prepared 64B descriptor */
 	desc_sw->flags |= XGENE_DMA_FLAG_64B_DESC;
-
-skip_additional_src:
-	/* Hardware stores descriptor in little endian format */
-	xgene_dma_cpu_to_le64(desc1, 4);
-	if (desc2)
-		xgene_dma_cpu_to_le64(desc2, 4);
 }

 static void xgene_dma_prep_xor_desc(struct xgene_dma_chan *chan,
@@ -510,7 +480,7 @@  static void xgene_dma_prep_xor_desc(struct xgene_dma_chan *chan,
 				    u32 src_cnt, size_t *nbytes,
 				    const u8 *scf)
 {
-	void *desc1, *desc2;
+	struct xgene_dma_desc_hw *desc1, *desc2;
 	size_t len = *nbytes;
 	int i;

@@ -521,28 +491,24 @@  static void xgene_dma_prep_xor_desc(struct xgene_dma_chan *chan,
 	xgene_dma_init_desc(desc1, chan->tx_ring.dst_ring_num);

 	/* Set destination address */
-	XGENE_DMA_DESC_DR_SET(desc1);
-	XGENE_DMA_DESC_DST_ADDR_SET(desc1, *dst);
+	desc1->m2 |= cpu_to_le64(XGENE_DMA_DESC_DR_BIT);
+	desc1->m3 |= cpu_to_le64(*dst);

 	/* We have multiple source addresses, so need to set NV bit*/
-	XGENE_DMA_DESC_NV_SET(desc1);
+	desc1->m0 |= cpu_to_le64(XGENE_DMA_DESC_NV_BIT);

 	/* Set flyby opcode */
-	XGENE_DMA_DESC_FLYBY_SET(desc1, xgene_dma_encode_xor_flyby(src_cnt));
+	desc1->m2 |= cpu_to_le64(xgene_dma_encode_xor_flyby(src_cnt));

 	/* Set 1st to 5th source addresses */
 	for (i = 0; i < src_cnt; i++) {
 		len = *nbytes;
-		xgene_dma_set_src_buffer((i == 0) ? (desc1 + 8) :
+		xgene_dma_set_src_buffer((i == 0) ? &desc1->m1 :
 					 xgene_dma_lookup_ext8(desc2, i - 1),
 					 &len, &src[i]);
-		XGENE_DMA_DESC_MULTI_SET(desc1, scf[i], i);
+		desc1->m2 |= cpu_to_le64((scf[i] << ((i + 1) * 8)));
 	}

-	/* Hardware stores descriptor in little endian format */
-	xgene_dma_cpu_to_le64(desc1, 4);
-	xgene_dma_cpu_to_le64(desc2, 4);
-
 	/* Update meta data */
 	*nbytes = len;
 	*dst += XGENE_DMA_MAX_BYTE_CNT;
@@ -738,7 +704,7 @@  static int xgene_chan_xfer_request(struct xgene_dma_ring *ring,
  * xgene_chan_xfer_ld_pending - push any pending transactions to hw
  * @chan : X-Gene DMA channel
  *
- * LOCKING: must hold chan->desc_lock
+ * LOCKING: must hold chan->lock
  */
 static void xgene_chan_xfer_ld_pending(struct xgene_dma_chan *chan)
 {
@@ -808,7 +774,8 @@  static void xgene_dma_cleanup_descriptors(struct xgene_dma_chan *chan)
 		desc_hw = &ring->desc_hw[ring->head];

 		/* Check if this descriptor has been completed */
-		if (unlikely(XGENE_DMA_DESC_IS_EMPTY(desc_hw)))
+		if (unlikely(le64_to_cpu(desc_hw->m0) ==
+			     XGENE_DMA_DESC_EMPTY_SIGNATURE))
 			break;

 		if (++ring->head == ring->slots)
@@ -842,7 +809,7 @@  static void xgene_dma_cleanup_descriptors(struct xgene_dma_chan *chan)
 		iowrite32(-1, ring->cmd);

 		/* Mark this hw descriptor as processed */
-		XGENE_DMA_DESC_SET_EMPTY(desc_hw);
+		desc_hw->m0 = cpu_to_le64(XGENE_DMA_DESC_EMPTY_SIGNATURE);

 		xgene_dma_run_tx_complete_actions(chan, desc_sw);

@@ -889,7 +856,7 @@  static int xgene_dma_alloc_chan_resources(struct dma_chan *dchan)
  * @chan: X-Gene DMA channel
  * @list: the list to free
  *
- * LOCKING: must hold chan->desc_lock
+ * LOCKING: must hold chan->lock
  */
 static void xgene_dma_free_desc_list(struct xgene_dma_chan *chan,
 				     struct list_head *list)
@@ -900,15 +867,6 @@  static void xgene_dma_free_desc_list(struct xgene_dma_chan *chan,
 		xgene_dma_clean_descriptor(chan, desc);
 }

-static void xgene_dma_free_tx_desc_list(struct xgene_dma_chan *chan,
-					struct list_head *list)
-{
-	struct xgene_dma_desc_sw *desc, *_desc;
-
-	list_for_each_entry_safe(desc, _desc, list, node)
-		xgene_dma_clean_descriptor(chan, desc);
-}
-
 static void xgene_dma_free_chan_resources(struct dma_chan *dchan)
 {
 	struct xgene_dma_chan *chan = to_dma_chan(dchan);
@@ -985,7 +943,7 @@  fail:
 	if (!first)
 		return NULL;

-	xgene_dma_free_tx_desc_list(chan, &first->tx_list);
+	xgene_dma_free_desc_list(chan, &first->tx_list);
 	return NULL;
 }

@@ -1093,7 +1051,7 @@  fail:
 	if (!first)
 		return NULL;

-	xgene_dma_free_tx_desc_list(chan, &first->tx_list);
+	xgene_dma_free_desc_list(chan, &first->tx_list);
 	return NULL;
 }

@@ -1141,7 +1099,7 @@  fail:
 	if (!first)
 		return NULL;

-	xgene_dma_free_tx_desc_list(chan, &first->tx_list);
+	xgene_dma_free_desc_list(chan, &first->tx_list);
 	return NULL;
 }

@@ -1218,7 +1176,7 @@  fail:
 	if (!first)
 		return NULL;

-	xgene_dma_free_tx_desc_list(chan, &first->tx_list);
+	xgene_dma_free_desc_list(chan, &first->tx_list);
 	return NULL;
 }

@@ -1316,7 +1274,6 @@  static void xgene_dma_setup_ring(struct xgene_dma_ring *ring)
 {
 	void *ring_cfg = ring->state;
 	u64 addr = ring->desc_paddr;
-	void *desc;
 	u32 i, val;

 	ring->slots = ring->size / XGENE_DMA_RING_WQ_DESC_SIZE;
@@ -1358,8 +1315,10 @@  static void xgene_dma_setup_ring(struct xgene_dma_ring *ring)

 	/* Set empty signature to DMA Rx ring descriptors */
 	for (i = 0; i < ring->slots; i++) {
+		struct xgene_dma_desc_hw *desc;
+
 		desc = &ring->desc_hw[i];
-		XGENE_DMA_DESC_SET_EMPTY(desc);
+		desc->m0 = cpu_to_le64(XGENE_DMA_DESC_EMPTY_SIGNATURE);
 	}

 	/* Enable DMA Rx ring interrupt */
@@ -1895,9 +1854,9 @@  static int xgene_dma_get_resources(struct platform_device *pdev,

 	pdma->csr_dma = devm_ioremap(&pdev->dev, res->start,
 				     resource_size(res));
-	if (IS_ERR(pdma->csr_dma)) {
+	if (!pdma->csr_dma) {
 		dev_err(&pdev->dev, "Failed to ioremap csr region");
-		return PTR_ERR(pdma->csr_dma);
+		return -ENOMEM;
 	}

 	/* Get DMA ring csr region */
@@ -1909,9 +1868,9 @@  static int xgene_dma_get_resources(struct platform_device *pdev,

 	pdma->csr_ring =  devm_ioremap(&pdev->dev, res->start,
 				       resource_size(res));
-	if (IS_ERR(pdma->csr_ring)) {
+	if (!pdma->csr_ring) {
 		dev_err(&pdev->dev, "Failed to ioremap ring csr region");
-		return PTR_ERR(pdma->csr_ring);
+		return -ENOMEM;
 	}

 	/* Get DMA ring cmd csr region */
@@ -1923,9 +1882,9 @@  static int xgene_dma_get_resources(struct platform_device *pdev,

 	pdma->csr_ring_cmd = devm_ioremap(&pdev->dev, res->start,
 					  resource_size(res));
-	if (IS_ERR(pdma->csr_ring_cmd)) {
+	if (!pdma->csr_ring_cmd) {
 		dev_err(&pdev->dev, "Failed to ioremap ring cmd csr region");
-		return PTR_ERR(pdma->csr_ring_cmd);
+		return -ENOMEM;
 	}

 	/* Get efuse csr region */
@@ -1937,9 +1896,9 @@  static int xgene_dma_get_resources(struct platform_device *pdev,

 	pdma->csr_efuse = devm_ioremap(&pdev->dev, res->start,
 				       resource_size(res));
-	if (IS_ERR(pdma->csr_efuse)) {
+	if (!pdma->csr_efuse) {
 		dev_err(&pdev->dev, "Failed to ioremap efuse csr region");
-		return PTR_ERR(pdma->csr_efuse);
+		return -ENOMEM;
 	}

 	/* Get DMA error interrupt */
@@ -2076,7 +2035,6 @@  static struct platform_driver xgene_dma_driver = {
 	.remove = xgene_dma_remove,
 	.driver = {
 		.name = "X-Gene-DMA",
-		.owner = THIS_MODULE,
 		.of_match_table = xgene_dma_of_match_ptr,
 	},
 };
@@ -2085,6 +2043,5 @@  module_platform_driver(xgene_dma_driver);

 MODULE_DESCRIPTION("APM X-Gene SoC DMA driver");
 MODULE_AUTHOR("Rameshwar Prasad Sahu <rsahu@apm.com>");
-MODULE_AUTHOR("Loc Ho <lho@apm.com>");
 MODULE_LICENSE("GPL");
 MODULE_VERSION("1.0");