On 08-07-20, 05:45, Koehrer Mathias (ETAS/EES-SL) wrote:
> Extend NXP QDMA driver to check transmission errors
>
> The NXP QDMA driver (fsl-qdma.c) does not check the status bits
> that indicate if a DMA transfer has been completed successfully.
> This patch extends the driver to do exactly this.
This is v3, pls do mention what changes b/w each revision in cover
letter or the after the marker after s-o-b line
>
> Signed-off-by: Mathias Koehrer <mathias.koehrer@xxxxxxxx>
> ---
> Index: linux-5.4/drivers/dma/fsl-qdma.c
> ===================================================================
> --- linux-5.4.orig/drivers/dma/fsl-qdma.c
> +++ linux-5.4/drivers/dma/fsl-qdma.c
> @@ -56,7 +56,7 @@
>
> /* Registers for bit and genmask */
> #define FSL_QDMA_CQIDR_SQT BIT(15)
> -#define QDMA_CCDF_FOTMAT BIT(29)
> +#define QDMA_CCDF_FORMAT BIT(29)
> #define QDMA_CCDF_SER BIT(30)
> #define QDMA_SG_FIN BIT(30)
> #define QDMA_SG_LEN_MASK GENMASK(29, 0)
> @@ -110,11 +110,23 @@
> #define FSL_QDMA_CMD_DSEN_OFFSET 19
> #define FSL_QDMA_CMD_LWC_OFFSET 16
>
> +/* Field definition for Descriptor status */
> +#define QDMA_CCDF_STATUS_RTE BIT(5)
> +#define QDMA_CCDF_STATUS_WTE BIT(4)
> +#define QDMA_CCDF_STATUS_CDE BIT(2)
> +#define QDMA_CCDF_STATUS_SDE BIT(1)
> +#define QDMA_CCDF_STATUS_DDE BIT(0)
> +#define QDMA_CCDF_STATUS_MASK (QDMA_CCDF_STATUS_RTE | \
> + QDMA_CCDF_STATUS_WTE | \
> + QDMA_CCDF_STATUS_CDE | \
> + QDMA_CCDF_STATUS_SDE | \
> + QDMA_CCDF_STATUS_DDE)
> +
> /* Field definition for Descriptor offset */
> -#define QDMA_CCDF_STATUS 20
> #define QDMA_CCDF_OFFSET 20
> #define QDMA_SDDF_CMD(x) (((u64)(x)) << 32)
>
> +
redundant empty line
> /* Field definition for safe loop count*/
> #define FSL_QDMA_HALT_COUNT 1500
> #define FSL_QDMA_MAX_SIZE 16385
> @@ -243,13 +255,13 @@ qdma_ccdf_get_offset(const struct fsl_qd
> static inline void
> qdma_ccdf_set_format(struct fsl_qdma_format *ccdf, int offset)
> {
> - ccdf->cfg = cpu_to_le32(QDMA_CCDF_FOTMAT | offset);
> + ccdf->cfg = cpu_to_le32(QDMA_CCDF_FORMAT | (offset << QDMA_CCDF_OFFSET));
> }
>
> static inline int
> qdma_ccdf_get_status(const struct fsl_qdma_format *ccdf)
> {
> - return (le32_to_cpu(ccdf->status) & QDMA_CCDF_MASK) >> QDMA_CCDF_STATUS;
> + return (le32_to_cpu(ccdf->status) & QDMA_CCDF_STATUS_MASK);
> }
>
> static inline void
> @@ -618,6 +630,7 @@ fsl_qdma_queue_transfer_complete(struct
> {
> bool duplicate;
> u32 reg, i, count;
> + u8 completion_status;
> struct fsl_qdma_queue *temp_queue;
> struct fsl_qdma_format *status_addr;
> struct fsl_qdma_comp *fsl_comp = NULL;
> @@ -677,6 +690,8 @@ fsl_qdma_queue_transfer_complete(struct
> }
> list_del(&fsl_comp->list);
>
> + completion_status = qdma_ccdf_get_status(status_addr);
> +
> reg = qdma_readl(fsl_qdma, block + FSL_QDMA_BSQMR);
> reg |= FSL_QDMA_BSQMR_DI;
> qdma_desc_addr_set64(status_addr, 0x0);
> @@ -686,6 +701,24 @@ fsl_qdma_queue_transfer_complete(struct
> qdma_writel(fsl_qdma, reg, block + FSL_QDMA_BSQMR);
> spin_unlock(&temp_queue->queue_lock);
>
> + /* The completion_status is evaluated here (outside of spin lock) */
> + if (completion_status) {
> + if (completion_status & QDMA_CCDF_STATUS_WTE) {
> + /* Write transaction error */
> + fsl_comp->vdesc.tx_result.result = DMA_TRANS_WRITE_FAILED;
> + }
this brace should be with else if line. See
Documentation/process/coding-style.rst and do run
./scripts/checkpatch.pl --strict for this patch (i see few other style
issues)
> + else if (completion_status & QDMA_CCDF_STATUS_RTE) {
> + /* Read transaction error */
> + fsl_comp->vdesc.tx_result.result = DMA_TRANS_READ_FAILED;
> + }
> + else {
> + /* Command/source/destination description error */
> + fsl_comp->vdesc.tx_result.result = DMA_TRANS_ABORTED;
> + dev_err(fsl_qdma->dma_dev.dev, "DMA status descriptor error %x\n",
> + completion_status);
> + }
So completion status always mean error? when does it indicate success,
when not set?
> + }
> +
> spin_lock(&fsl_comp->qchan->vchan.lock);
> vchan_cookie_complete(&fsl_comp->vdesc);
> fsl_comp->qchan->status = DMA_COMPLETE;
> @@ -704,7 +737,14 @@ static irqreturn_t fsl_qdma_error_handle
> intr = qdma_readl(fsl_qdma, status + FSL_QDMA_DEDR);
>
> if (intr)
> - dev_err(fsl_qdma->dma_dev.dev, "DMA transaction error!\n");
> + {
style error!
> + unsigned int decfdw0r = qdma_readl(fsl_qdma, status + FSL_QDMA_DECFDW0R);
> + unsigned int decfdw1r = qdma_readl(fsl_qdma, status + FSL_QDMA_DECFDW1R);
> + unsigned int decfdw2r = qdma_readl(fsl_qdma, status + FSL_QDMA_DECFDW2R);
> + unsigned int decfdw3r = qdma_readl(fsl_qdma, status + FSL_QDMA_DECFDW3R);
> + dev_err(fsl_qdma->dma_dev.dev, "DMA transaction error! (%x: %x-%x-%x-%x)\n",
> + intr, decfdw0r, decfdw1r, decfdw2r, decfdw3r);
> + }
>
> qdma_writel(fsl_qdma, FSL_QDMA_DEDR_CLEAR, status + FSL_QDMA_DEDR);
> return IRQ_HANDLED;
--
~Vinod
