Re: [PATCH v3 01/10] dmaengine: Actions: get rid of bit fields from dma descriptor

[Amit Tomer]

Hi,

Thanks for having a look.

On Wed, Jun 3, 2020 at 12:52 PM Manivannan Sadhasivam
<manivannan.sadhasivam@xxxxxxxxxx> wrote:
> Individual comments for these enums?

I was expecting this comment , and thought these fields are self explanatory
But if you prefer to have description about it, I would have it in next version.

> >+enum owl_dmadesc_offsets {
> >+      OWL_DMADESC_NEXT_LLI = 0,
> >+      OWL_DMADESC_SADDR,
> >+      OWL_DMADESC_DADDR,
> >+      OWL_DMADESC_FLEN,
> >+      OWL_DMADESC_SRC_STRIDE,
> >+      OWL_DMADESC_DST_STRIDE,
> >+      OWL_DMADESC_CTRLA,
> >+      OWL_DMADESC_CTRLB,
> >+      OWL_DMADESC_CONST_NUM,
> >+      OWL_DMADESC_SIZE
> > };
> >
> > /**
> >@@ -153,7 +144,7 @@ struct owl_dma_lli_hw {
> >  * @node: node for txd's lli_list
> >  */
> > struct owl_dma_lli {
> >-      struct  owl_dma_lli_hw  hw;
> >+      u32                     hw[OWL_DMADESC_SIZE];
> >       dma_addr_t              phys;
> >       struct list_head        node;
> > };
> >@@ -320,6 +311,11 @@ static inline u32 llc_hw_ctrlb(u32 int_ctl)
> >       return ctl;
> > }
> >
> >+static u32 llc_hw_flen(struct owl_dma_lli *lli)
> >+{
> >+      return lli->hw[OWL_DMADESC_FLEN] & GENMASK(19, 0);
> >+}
> >+
> > static void owl_dma_free_lli(struct owl_dma *od,
> >                            struct owl_dma_lli *lli)
> > {
> >@@ -351,8 +347,9 @@ static struct owl_dma_lli *owl_dma_add_lli(struct
> >owl_dma_txd *txd,
> >               list_add_tail(&next->node, &txd->lli_list);
> >
> >       if (prev) {
> >-              prev->hw.next_lli = next->phys;
> >-              prev->hw.ctrla |= llc_hw_ctrla(OWL_DMA_MODE_LME, 0);
> >+              prev->hw[OWL_DMADESC_NEXT_LLI] = next->phys;
> >+              prev->hw[OWL_DMADESC_CTRLA] |=
> >+                                      llc_hw_ctrla(OWL_DMA_MODE_LME, 0);
> >       }
> >
> >       return next;
> >@@ -365,8 +362,7 @@ static inline int owl_dma_cfg_lli(struct
> >owl_dma_vchan *vchan,
> >                                 struct dma_slave_config *sconfig,
> >                                 bool is_cyclic)
> > {
> >-      struct owl_dma_lli_hw *hw = &lli->hw;
> >-      u32 mode;
> >+      u32 mode, ctrlb;
> >
> >       mode = OWL_DMA_MODE_PW(0);
> >
> >@@ -407,22 +403,22 @@ static inline int owl_dma_cfg_lli(struct
> >owl_dma_vchan *vchan,
> >               return -EINVAL;
> >       }
> >
> >-      hw->next_lli = 0; /* One link list by default */
> >-      hw->saddr = src;
> >-      hw->daddr = dst;
> >-
> >-      hw->fcnt = 1; /* Frame count fixed as 1 */
> >-      hw->flen = len; /* Max frame length is 1MB */
> >-      hw->src_stride = 0;
> >-      hw->dst_stride = 0;
> >-      hw->ctrla = llc_hw_ctrla(mode,
> >-                               OWL_DMA_LLC_SAV_LOAD_NEXT |
> >-                               OWL_DMA_LLC_DAV_LOAD_NEXT);
> >+      lli->hw[OWL_DMADESC_CTRLA] = llc_hw_ctrla(mode,
> >+                                                OWL_DMA_LLC_SAV_LOAD_NEXT |
> >+                                                OWL_DMA_LLC_DAV_LOAD_NEXT);
> >
> >       if (is_cyclic)
> >-              hw->ctrlb = llc_hw_ctrlb(OWL_DMA_INTCTL_BLOCK);
> >+              ctrlb = llc_hw_ctrlb(OWL_DMA_INTCTL_BLOCK);
> >       else
> >-              hw->ctrlb = llc_hw_ctrlb(OWL_DMA_INTCTL_SUPER_BLOCK);
> >+              ctrlb = llc_hw_ctrlb(OWL_DMA_INTCTL_SUPER_BLOCK);
> >+
> >+      lli->hw[OWL_DMADESC_NEXT_LLI] = 0;
>
> Again, please preserve the old comments.

Sure, would do it.
>
> >+      lli->hw[OWL_DMADESC_SADDR] = src;
> >+      lli->hw[OWL_DMADESC_DADDR] = dst;
> >+      lli->hw[OWL_DMADESC_SRC_STRIDE] = 0;
> >+      lli->hw[OWL_DMADESC_DST_STRIDE] = 0;
> >+      lli->hw[OWL_DMADESC_FLEN] = len | FCNT_VAL << 20;
>
> Please explain what you're doing here.

Actually , in next the patch 2/10 there is comment that explains a bit
about it.

        /*
         * S700 put flen and fcnt at offset 0x0c and 0x1c respectively,
         * whereas S900 put flen and fcnt at offset 0x0c.
         */

Shall I add more details to it in the next patch 02/10 ?

Thanks
-Amit.