RE: [EXT] Re: [PATCH] dmaengine: fsl-qdma: fixed the source/destination descriptior format

[Peng Ma]

>-----Original Message-----
>From: Vinod Koul <vkoul@xxxxxxxxxx>
>Sent: 2019å4æ29æ 13:16
>To: Peng Ma <peng.ma@xxxxxxx>
>Cc: dan.j.williams@xxxxxxxxx; Leo Li <leoyang.li@xxxxxxx>;
>dmaengine@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
>Subject: Re: [EXT] Re: [PATCH] dmaengine: fsl-qdma: fixed the
>source/destination descriptior format
>
>Caution: EXT Email
>
>On 28-04-19, 02:00, Peng Ma wrote:
>> Hi Vinod,
>>
>> Thanks your comments.
>> Please see my comments inline.
>>
>> Best Regards,
>> Peng
>>
>> >-----Original Message-----
>> >From: Vinod Koul <vkoul@xxxxxxxxxx>
>> >Sent: 2019å4æ26æ 19:51
>> >To: Peng Ma <peng.ma@xxxxxxx>
>> >Cc: dan.j.williams@xxxxxxxxx; Leo Li <leoyang.li@xxxxxxx>;
>> >dmaengine@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
>> >Subject: [EXT] Re: [PATCH] dmaengine: fsl-qdma: fixed the
>> >source/destination descriptior format
>> >
>> >Caution: EXT Email
>> >
>> >On 19-04-19, 08:46, Peng Ma wrote:
>> >> CMD of Source/Destination descriptior format should be lower of
>> >
>> >s/descriptior/descriptor
>> >
>> [Peng Ma] Got it.
>> >> struct fsl_qdma_engine number data address.
>> >>
>> >> Signed-off-by: Peng Ma <peng.ma@xxxxxxx>
>> >> ---
>> >>  drivers/dma/fsl-qdma.c |   29 ++++++++++++++++++-----------
>> >>  1 files changed, 18 insertions(+), 11 deletions(-)
>> >>
>> >> diff --git a/drivers/dma/fsl-qdma.c b/drivers/dma/fsl-qdma.c index
>> >> aa1d0ae..542765a 100644
>> >> --- a/drivers/dma/fsl-qdma.c
>> >> +++ b/drivers/dma/fsl-qdma.c
>> >> @@ -113,6 +113,7 @@
>> >>  /* Field definition for Descriptor offset */
>> >>  #define QDMA_CCDF_STATUS             20
>> >>  #define QDMA_CCDF_OFFSET             20
>> >> +#define QDMA_SDDF_CMD(x)             (((u64)(x)) << 32)
>> >>
>> >>  /* Field definition for safe loop count*/
>> >>  #define FSL_QDMA_HALT_COUNT          1500
>> >> @@ -214,6 +215,12 @@ struct fsl_qdma_engine {
>> >>
>> >>  };
>> >>
>> >> +static inline void
>> >> +qdma_sddf_set_cmd(struct fsl_qdma_format *sddf, u32 val) {
>> >> +     sddf->data = QDMA_SDDF_CMD(val); }
>> >> +
>> >>  static inline u64
>> >>  qdma_ccdf_addr_get64(const struct fsl_qdma_format *ccdf)  { @@
>> >-341,6
>> >> +348,7 @@ static void fsl_qdma_free_chan_resources(struct dma_chan
>> >> *chan)  static void fsl_qdma_comp_fill_memcpy(struct fsl_qdma_comp
>> >*fsl_comp,
>> >>                                     dma_addr_t dst,
>dma_addr_t
>> >src,
>> >> u32 len)  {
>> >> +     u32 cmd;
>> >>       struct fsl_qdma_format *sdf, *ddf;
>> >>       struct fsl_qdma_format *ccdf, *csgf_desc, *csgf_src,
>> >> *csgf_dest;
>> >>
>> >> @@ -353,6 +361,7 @@ static void fsl_qdma_comp_fill_memcpy(struct
>> >> fsl_qdma_comp *fsl_comp,
>> >>
>> >>       memset(fsl_comp->virt_addr, 0,
>> >FSL_QDMA_COMMAND_BUFFER_SIZE);
>> >>       memset(fsl_comp->desc_virt_addr, 0,
>> >> FSL_QDMA_DESCRIPTOR_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)); @@
>> >> -369,14 +378,14 @@ static void fsl_qdma_comp_fill_memcpy(struct
>> >fsl_qdma_comp *fsl_comp,
>> >>       /* This entry is the last entry. */
>> >>       qdma_csgf_set_f(csgf_dest, len);
>> >>       /* Descriptor Buffer */
>> >> -     sdf->data =
>> >> -             cpu_to_le64(FSL_QDMA_CMD_RWTTYPE <<
>> >> -                         FSL_QDMA_CMD_RWTTYPE_OFFSET);
>> >> -     ddf->data =
>> >> -             cpu_to_le64(FSL_QDMA_CMD_RWTTYPE <<
>> >> -                         FSL_QDMA_CMD_RWTTYPE_OFFSET);
>> >> -     ddf->data |=
>> >> -             cpu_to_le64(FSL_QDMA_CMD_LWC <<
>> >FSL_QDMA_CMD_LWC_OFFSET);
>> >> +     cmd = cpu_to_le32(FSL_QDMA_CMD_RWTTYPE <<
>> >> +                       FSL_QDMA_CMD_RWTTYPE_OFFSET);
>> >> +     qdma_sddf_set_cmd(sdf, cmd);
>> >> +
>> >> +     cmd = cpu_to_le32(FSL_QDMA_CMD_RWTTYPE <<
>> >> +                       FSL_QDMA_CMD_RWTTYPE_OFFSET);
>> >> +     cmd |= cpu_to_le32(FSL_QDMA_CMD_LWC <<
>> >FSL_QDMA_CMD_LWC_OFFSET);
>> >> +     qdma_sddf_set_cmd(ddf, cmd);
>> >>  }
>> >>
>> >>  /*
>> >> @@ -701,10 +710,8 @@ static irqreturn_t fsl_qdma_error_handler(int
>> >> irq, void *dev_id)
>> >>
>> >>       intr = qdma_readl(fsl_qdma, status + FSL_QDMA_DEDR);
>> >>
>> >> -     if (intr) {
>> >> +     if (intr)
>> >>               dev_err(fsl_qdma->dma_dev.dev, "DMA transaction
>> >error!\n");
>> >> -             return IRQ_NONE;
>> >> -     }
>> >
>> >this seems unrelated can you explain?
>> >
>> [Peng Ma] This is an added improvement. When an error occurs we should
>clean the error reg then to return.
>> I forgot to explain it on comments. Should I add this changed to the
>comments?
>
>Yes and you should make it a separate patch. A patch should do only 1 thing!
>
[Peng Ma] OK, Got it, thanks.
Best Regards,
Peng
>--
>~Vinod