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 <[email protected]>
Signed-off-by: Rameshwar Prasad Sahu <[email protected]>
---
drivers/dma/xgene-dma.c | 191 +++++++++++++++++++-----------------------------
1 file changed, 74 insertions(+), 117 deletions(-)
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 <[email protected]>");
-MODULE_AUTHOR("Loc Ho <[email protected]>");
MODULE_LICENSE("GPL");
MODULE_VERSION("1.0");
--
1.8.2.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 <[email protected]>
> Signed-off-by: Rameshwar Prasad Sahu <[email protected]>
Looks good now,
Reviewed-by: Arnd Bergmann <[email protected]>
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
Hi Arnd,
On Fri, Apr 17, 2015 at 2:19 PM, Arnd Bergmann <[email protected]> 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 <[email protected]>
>> Signed-off-by: Rameshwar Prasad Sahu <[email protected]>
>
> Looks good now,
>
> Reviewed-by: Arnd Bergmann <[email protected]>
>
> 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
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
> /* 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 <[email protected]>");
> -MODULE_AUTHOR("Loc Ho <[email protected]>");
And why this?
Fixes looks good though
--
~Vinod
> MODULE_LICENSE("GPL");
> MODULE_VERSION("1.0");
> --
> 1.8.2.1
>
--
Hi Vinod,
On Fri, Apr 17, 2015 at 11:59 PM, Vinod Koul <[email protected]> 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 <[email protected]>");
>> -MODULE_AUTHOR("Loc Ho <[email protected]>");
> 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
>>
>
> --
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 <[email protected]>");
> >> -MODULE_AUTHOR("Loc Ho <[email protected]>");
> > 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
--
~Vinod
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 <[email protected]>");
> > >> -MODULE_AUTHOR("Loc Ho <[email protected]>");
> > > 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
On Sun, Apr 26, 2015 at 10:20 PM, Fengguang Wu <[email protected]> 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
On Thu, Jun 11, 2015 at 05:36:00PM -0700, Christopher Li wrote:
> n Sun, Apr 26, 2015 at 10:20 PM, Fengguang Wu <[email protected]> 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