Hi,
Thanks for having a look.
On Wed, Jun 3, 2020 at 12:52 PM Manivannan Sadhasivam
<manivannan.sadhasi...@linaro.org> 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.
