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!
--
~Vinod
