From: Amit Tomer <amittomer25@gmail.com>
To: "André Przywara" <andre.przywara@arm.com>
Cc: vkoul@kernel.org, "Andreas Färber" <afaerber@suse.de>,
"Manivannan Sadhasivam" <manivannan.sadhasivam@linaro.org>,
dan.j.williams@intel.com, cristian.ciocaltea@gmail.com,
dmaengine@vger.kernel.org,
linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
linux-actions@lists.infradead.org
Subject: Re: [PATCH RFC 2/8] dmaengine: Actions: Add support for S700 DMA engine
Date: Wed, 6 May 2020 18:24:28 +0530 [thread overview]
Message-ID: <CABHD4K9mqpcO7jo4NQov__8jEGhAJr2o8JTiX1N+Z=zb9vG0OQ@mail.gmail.com> (raw)
In-Reply-To: <1c285ad4-a366-db08-e4e8-c2e1437cc505@arm.com>
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
next prev parent reply other threads:[~2020-05-06 12:55 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1588761371-9078-1-git-send-email-amittomer25@gmail.com>
2020-05-06 10:36 ` [PATCH RFC 1/8] dmaengine: Actions: get rid of bit fields from dma descriptor Amit Singh Tomar
2020-05-10 15:51 ` Manivannan Sadhasivam
2020-05-11 10:45 ` Amit Tomer
2020-05-11 11:20 ` Manivannan Sadhasivam
2020-05-11 11:44 ` André Przywara
2020-05-11 12:04 ` Manivannan Sadhasivam
2020-05-11 12:48 ` André Przywara
2020-05-11 15:29 ` Manivannan Sadhasivam
2020-05-06 10:36 ` [PATCH RFC 2/8] dmaengine: Actions: Add support for S700 DMA engine Amit Singh Tomar
2020-05-06 11:12 ` André Przywara
2020-05-06 12:54 ` Amit Tomer [this message]
2020-05-06 13:04 ` André Przywara
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CABHD4K9mqpcO7jo4NQov__8jEGhAJr2o8JTiX1N+Z=zb9vG0OQ@mail.gmail.com' \
--to=amittomer25@gmail.com \
--cc=afaerber@suse.de \
--cc=andre.przywara@arm.com \
--cc=cristian.ciocaltea@gmail.com \
--cc=dan.j.williams@intel.com \
--cc=dmaengine@vger.kernel.org \
--cc=linux-actions@lists.infradead.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=manivannan.sadhasivam@linaro.org \
--cc=vkoul@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).