dmaengine: fsl-qdma: add NXP Layerscape qDMA engine driver support
PATCH v3 2/6 dmaengine: fsl-qdma: add NXP Layerscape qDMA engine driver support
Review
Quoted 4 lines · Wen He
config
FSL_RAID
tristate "Freescale RAID engine Support"(quoted diff) +1
-1- depends on FSL_SOC && !ASYNC_TX_ENABLE_CHANNEL_SWITCH + depends on FSL_SOC && !ASYNC_TX_ENABLE_CHANNEL_SWITCH
Why this change?
Quoted 10 lines
+/*
+ * Driver for NXP Layerscape Queue direct memory access controller (qDMA)
+ *
+ * Copyright 2017 NXP
+ *
+ * Author:
+ * Jiaheng Fan <[email protected]>
+ * Wen He <[email protected]>
+ *
+ * SPDX-License-Identifier: GPL-2.0+
this needs to be:
//SPDX-License-Identifier: GPL-2.0+
//Copyright 2017 NXP
and at top of the file
Review
Quoted 4 lines
(quoted diff) +4
-0+ +#define FSL_QDMA_CQIDR_CQT 0xff000000 +#define FSL_QDMA_CQIDR_SQPE 0x800000 +#define FSL_QDMA_CQIDR_SQT 0x8000
BIT and GENMASK() for these and rest of them please
Review
Quoted 4 lines
(quoted diff) +4
-0+static inline u64 +qdma_ccdf_addr_get64(const struct fsl_qdma_format *ccdf) +{ + return le64_to_cpu(ccdf->data) & 0xffffffffffLLU;
^^^^^
Do you want something like INT_MAX, if not GENMASK() pls for the constant
Review
Quoted 13 lines
(quoted diff) +13
-0+} + +static inline void +qdma_desc_addr_set64(struct fsl_qdma_format *ccdf, u64 addr) +{ + ccdf->addr_hi = upper_32_bits(addr); + ccdf->addr_lo = cpu_to_le32(lower_32_bits(addr)); +} + +static inline u64 +qdma_ccdf_get_queue(const struct fsl_qdma_format *ccdf) +{ + return ccdf->cfg8b_w1 & 0xff;
USHRT_MAX
Review
Quoted 8 lines
(quoted diff) +8
-0+static void fsl_qdma_comp_fill_memcpy(struct fsl_qdma_comp *fsl_comp, + dma_addr_t dst, dma_addr_t src, u32 len) +{ + struct fsl_qdma_format *ccdf, *csgf_desc, *csgf_src, *csgf_dest; + struct fsl_qdma_sdf *sdf; + struct fsl_qdma_ddf *ddf; + + ccdf = (struct fsl_qdma_format *)fsl_comp->virt_addr;
why are doing a cast away from void *?
Review
Quoted 3 lines
(quoted diff) +3
-0+ csgf_desc = (struct fsl_qdma_format *)fsl_comp->virt_addr + 1; + csgf_src = (struct fsl_qdma_format *)fsl_comp->virt_addr + 2; + csgf_dest = (struct fsl_qdma_format *)fsl_comp->virt_addr + 3;
why not ccdf + 1 and so on?
Review
Quoted 8 lines
(quoted diff) +8
-0+ sdf = (struct fsl_qdma_sdf *)fsl_comp->virt_addr + 4; + ddf = (struct fsl_qdma_ddf *)fsl_comp->virt_addr + 5; + + memset(fsl_comp->virt_addr, 0, FSL_QDMA_BASE_BUFFER_SIZE); + /* Head Command Descriptor(Frame Descriptor) */ + qdma_desc_addr_set64(ccdf, fsl_comp->bus_addr + 16); + qdma_ccdf_set_format(ccdf, qdma_ccdf_get_offset(ccdf)); + qdma_ccdf_set_ser(ccdf, qdma_ccdf_get_status(ccdf));
empty line after each logical block to help read the code better
Review
Quoted 7 lines
(quoted diff) +7
-0+static int fsl_qdma_pre_request_enqueue_desc(struct fsl_qdma_queue *queue) +{ + struct fsl_qdma_comp *comp_temp; + int i; + + for (i = 0; i < queue->n_cq; i++) { + comp_temp = kzalloc(sizeof(*comp_temp), GFP_KERNEL);
GFP_KERNEL here
Review
Quoted 4 lines
(quoted diff) +4
-0+ if (!comp_temp) + return -ENOMEM; + comp_temp->virt_addr = dma_pool_alloc(queue->comp_pool, + GFP_NOWAIT,
GFP_NOWAIT here ??
this seems to be called in probe right?
Quoted 9 lines
(quoted diff) +9
-0+static struct fsl_qdma_comp *fsl_qdma_request_enqueue_desc( + struct fsl_qdma_chan *fsl_chan, + unsigned int dst_nents, + unsigned int src_nents) +{ + struct fsl_qdma_comp *comp_temp; + struct fsl_qdma_sg *sg_block; + struct fsl_qdma_queue *queue = fsl_chan->queue; + unsigned long flags;
Review
Quoted 10 lines
(quoted diff) +10
-0+ unsigned int dst_sg_entry_block, src_sg_entry_block, sg_entry_total, i; + + spin_lock_irqsave(&queue->queue_lock, flags); + if (list_empty(&queue->comp_free)) { + spin_unlock_irqrestore(&queue->queue_lock, flags); + comp_temp = kzalloc(sizeof(*comp_temp), GFP_KERNEL); + if (!comp_temp) + return NULL; + comp_temp->virt_addr = dma_pool_alloc(queue->comp_pool, + GFP_NOWAIT,
here too! this is called from prep_ context
Quoted 7 lines
(quoted diff) +7
-0+ &comp_temp->bus_addr); + if (!comp_temp->virt_addr) { + kfree(comp_temp); + return NULL; + } + + } else {
Review
Quoted 10 lines
(quoted diff) +10
-0+ comp_temp = list_first_entry(&queue->comp_free, + struct fsl_qdma_comp, + list); + list_del(&comp_temp->list); + spin_unlock_irqrestore(&queue->queue_lock, flags); + } + + if (dst_nents != 0) + dst_sg_entry_block = dst_nents / + (FSL_QDMA_EXPECT_SG_ENTRY_NUM - 1) + 1;
DIV_ROUND_UP or something. PLs check kernel has defines for these sort of things...
Quoted 14 lines
(quoted diff) +14
-0+ else + dst_sg_entry_block = 0; + + if (src_nents != 0) + src_sg_entry_block = src_nents / + (FSL_QDMA_EXPECT_SG_ENTRY_NUM - 1) + 1; + else + src_sg_entry_block = 0; + + sg_entry_total = dst_sg_entry_block + src_sg_entry_block; + if (sg_entry_total) { + sg_block = kzalloc(sizeof(*sg_block) * + sg_entry_total, + GFP_KERNEL);
Review
Quoted 10 lines
(quoted diff) +10
-0+ if (!sg_block) { + dma_pool_free(queue->comp_pool, + comp_temp->virt_addr, + comp_temp->bus_addr); + return NULL; + } + comp_temp->sg_block = sg_block; + for (i = 0; i < sg_entry_total; i++) { + sg_block->virt_addr = dma_pool_alloc(queue->sg_pool, + GFP_NOWAIT,
i guess these are copy paster errors. And this code should then be in helper and used everywhere...
Quoted 24 lines
(quoted diff) +24
-0+static struct fsl_qdma_queue *fsl_qdma_alloc_queue_resources( + struct platform_device *pdev, + unsigned int queue_num) +{ + struct fsl_qdma_queue *queue_head, *queue_temp; + int ret, len, i; + unsigned int queue_size[FSL_QDMA_QUEUE_MAX]; + + if (queue_num > FSL_QDMA_QUEUE_MAX) + queue_num = FSL_QDMA_QUEUE_MAX; + len = sizeof(*queue_head) * queue_num; + queue_head = devm_kzalloc(&pdev->dev, len, GFP_KERNEL); + if (!queue_head) + return NULL; + + ret = device_property_read_u32_array(&pdev->dev, "queue-sizes", + queue_size, queue_num); + if (ret) { + dev_err(&pdev->dev, "Can't get queue-sizes.\n"); + return NULL; + } + + for (i = 0; i < queue_num; i++) { + if (queue_size[i] > FSL_QDMA_CIRCULAR_DESC_SIZE_MAX
Review
Quoted 10 lines
(quoted diff) +10
-0+ || queue_size[i] < FSL_QDMA_CIRCULAR_DESC_SIZE_MIN) { + dev_err(&pdev->dev, "Get wrong queue-sizes.\n"); + return NULL; + } + queue_temp = queue_head + i; + queue_temp->cq = dma_alloc_coherent(&pdev->dev, + sizeof(struct fsl_qdma_format) * + queue_size[i], + &queue_temp->bus_addr, + GFP_KERNEL);
so we allocated here
Review
Quoted 13 lines
(quoted diff) +13
-0+ if (!queue_temp->cq) + return NULL; + queue_temp->n_cq = queue_size[i]; + queue_temp->id = i; + queue_temp->virt_head = queue_temp->cq; + queue_temp->virt_tail = queue_temp->cq; + /* + * The dma pool for queue command buffer + */ + queue_temp->comp_pool = dma_pool_create("comp_pool", + &pdev->dev, + FSL_QDMA_BASE_BUFFER_SIZE, + 16, 0);
and here
Review
Quoted 7 lines
(quoted diff) +7
-0+ if (!queue_temp->comp_pool) { + dma_free_coherent(&pdev->dev, + sizeof(struct fsl_qdma_format) * + queue_size[i], + queue_temp->cq, + queue_temp->bus_addr); + return NULL;
and freed here on error
Quoted 6 lines
(quoted diff) +6
-0+ } + /* + * The dma pool for queue command buffer + */ + queue_temp->sg_pool = dma_pool_create("sg_pool", + &pdev->dev,
Review
Quoted 10 lines
(quoted diff) +10
-0+ FSL_QDMA_EXPECT_SG_ENTRY_NUM * 16, + 64, 0); + if (!queue_temp->sg_pool) { + dma_free_coherent(&pdev->dev, + sizeof(struct fsl_qdma_format) * + queue_size[i], + queue_temp->cq, + queue_temp->bus_addr); + dma_pool_destroy(queue_temp->comp_pool); + return NULL;
and repeated same here, maybe you should add goto's and have common error handling rather these everywhere..
Quoted 23 lines
(quoted diff) +23
-0+static struct fsl_qdma_queue *fsl_qdma_prep_status_queue( + struct platform_device *pdev) +{ + struct device_node *np = pdev->dev.of_node; + struct fsl_qdma_queue *status_head; + unsigned int status_size; + int ret; + + ret = of_property_read_u32(np, "status-sizes", &status_size); + if (ret) { + dev_err(&pdev->dev, "Can't get status-sizes.\n"); + return NULL; + } + if (status_size > FSL_QDMA_CIRCULAR_DESC_SIZE_MAX + || status_size < FSL_QDMA_CIRCULAR_DESC_SIZE_MIN) { + dev_err(&pdev->dev, "Get wrong status_size.\n"); + return NULL; + } + status_head = devm_kzalloc(&pdev->dev, sizeof(*status_head), + GFP_KERNEL); + if (!status_head) + return NULL; +
Review
Quoted 10 lines
(quoted diff) +10
-0+ /* + * Buffer for queue command + */ + status_head->cq = dma_alloc_coherent(&pdev->dev, + sizeof(struct fsl_qdma_format) * + status_size, + &status_head->bus_addr, + GFP_KERNEL); + if (!status_head->cq) + return NULL;
empty line here...
Review
Quoted 4 lines
(quoted diff) +4
-0+static enum dma_status fsl_qdma_tx_status(struct dma_chan *chan, + dma_cookie_t cookie, struct dma_tx_state *txstate) +{ + return dma_cookie_status(chan, cookie, txstate);
why not set this to dma_cookie_status()