>-----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
