Hi,
Thanks for quick review
> You should mention (at least in the commit message) why this is needed.
> And please move this into a separate function, this indentation is
> becoming mad here
There is not much documented about it, and all I see is GIC crash
if I keep it open for S700. Would figure out more details about it and
update in next version.
.
>
> > + for (i = 0; i < od->nr_pchans; i++) {
> > + pchan = &od->pchans[i];
> > + chan_irq_pending = pchan_readl(pchan,
> > + OWL_DMAX_INT_CTL) &
> > + pchan_readl(pchan,
> > + OWL_DMAX_INT_STATUS)
> > + ;
> > +
> > + /* Dummy read to ensure OWL_DMA_IRQ_PD0 value is
> > + * updated
> > + */
> > + dma_readl(od, OWL_DMA_IRQ_PD0);
> >
> > - global_irq_pending = dma_readl(od, OWL_DMA_IRQ_PD0);
> > + global_irq_pending = dma_readl(od,
> > + OWL_DMA_IRQ_PD0);
> >
> > - if (chan_irq_pending && !(global_irq_pending & BIT(i))) {
> > - dev_dbg(od->dma.dev,
> > - "global and channel IRQ pending match err\n");
> > + if (chan_irq_pending && !(global_irq_pending &
> > + BIT(i))) {
> > + dev_dbg(od->dma.dev,
> > + "global and channel IRQ pending match err\n");
> >
> > - /* Clear IRQ status for this pchan */
> > - pchan_update(pchan, OWL_DMAX_INT_STATUS,
> > - 0xff, false);
> > + /* Clear IRQ status for this pchan */
> > + pchan_update(pchan, OWL_DMAX_INT_STATUS,
> > + 0xff, false);
> >
> > - /* Update global IRQ pending */
> > - pending |= BIT(i);
> > + /* Update global IRQ pending */
> > + pending |= BIT(i);
> > + }
> > }
> > }
> >
> > @@ -720,6 +743,7 @@ static int owl_dma_resume(struct dma_chan *chan)
> >
> > static u32 owl_dma_getbytes_chan(struct owl_dma_vchan *vchan)
> > {
> > + struct owl_dma *od = to_owl_dma(vchan->vc.chan.device);
> > struct owl_dma_pchan *pchan;
> > struct owl_dma_txd *txd;
> > struct owl_dma_lli *lli;
> > @@ -741,9 +765,15 @@ static u32 owl_dma_getbytes_chan(struct owl_dma_vchan *vchan)
> > list_for_each_entry(lli, &txd->lli_list, node) {
> > /* Start from the next active node */
> > if (lli->phys == next_lli_phy) {
> > - list_for_each_entry(lli, &txd->lli_list, node)
> > - bytes += lli->hw[OWL_DMADESC_FLEN] &
> > - GENMASK(19, 0);
> > + list_for_each_entry(lli, &txd->lli_list, node) {
> > + if (od->devid == S700_DMA)
> > + bytes +=
> > + lli->hw[OWL_DMADESC_FLEN];
> > + else
> > + bytes +=
> > + lli->hw[OWL_DMADESC_FLEN] &
> > + GENMASK(19, 0);
>
> You should have an accessor for getting the frame len, that should avoid
> the insane wrapping here. Or factor this out into a helper function.
> Alternatively revert the if statement and continue, that saves you one
> level of indentation.
>
> I guess flen is limited to 20 bits anyway, so you might want to apply
> the 20-bit mask unconditionally.
Actually, on S700 flen uses 24 bits , so we should not use 20-bit mask.
For accessor function, shall this be okay ?
+static u32 llc_hw_flen(struct owl_dma *od,
+ struct owl_dma_lli *lli)
+{
+ u32 bit_mask;
+
+ if (od->devid == S700_DMA)
+ bit_mask = 23;
+ else
+ bit_mask = 19;
+
+ return lli->hw[OWL_DMADESC_FLEN] & GENMASK(bit_mask, 0);
+
+}
Thanks
Amit
