Message-ID: <20150722045700.GO23525@localhost>
Date:	Wed, 22 Jul 2015 10:27:00 +0530
From:	Vinod Koul <vinod.koul@...el.com>
To:	Andy Shevchenko <andy.shevchenko@...il.com>
Cc:	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Lee Jones <lee.jones@...aro.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	dmaengine <dmaengine@...r.kernel.org>,
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	Jarkko Nikula <jarkko.nikula@...ux.intel.com>,
	"Wysocki, Rafael J" <rafael.j.wysocki@...el.com>,
	Michael Turquette <mturquette@...libre.com>
Subject: Re: [PATCH v5 7/8] dmaengine: add a driver for Intel integrated DMA
 64-bit

On Tue, Jul 21, 2015 at 10:08:28AM +0300, Andy Shevchenko wrote:
> On Tue, Jul 21, 2015 at 7:50 AM, Vinod Koul <vinod.koul@...el.com> wrote:
> > On Mon, Jul 20, 2015 at 11:46:28AM +0300, Andy Shevchenko wrote:
> >> >> +static void idma64_chan_init(struct idma64 *idma64, struct idma64_chan *idma64c)
> >> >> +{
> >> >> +     u32 cfghi = IDMA64C_CFGH_SRC_PER(1) | IDMA64C_CFGH_DST_PER(0);
> >> >> +     u32 cfglo = 0;
> >> >> +
> >> >> +     /* Enforce FIFO drain when channel is suspended */
> >> >> +     cfglo |= IDMA64C_CFGL_CH_DRAIN;
> >> >> +
> >> >> +     /* Set default burst alignment */
> >> >> +     cfglo |= IDMA64C_CFGL_DST_BURST_ALIGN | IDMA64C_CFGL_SRC_BURST_ALIGN;
> >> >> +
> >> >> +     channel_writel(idma64c, CFG_LO, cfglo);
> >> >> +     channel_writel(idma64c, CFG_HI, cfghi);
> >> >> +
> >> >> +     /* Enable interrupts */
> >> >> +     channel_set_bit(idma64, MASK(XFER), idma64c->mask);
> >> >> +     channel_set_bit(idma64, MASK(ERROR), idma64c->mask);
> >> >> +
> >> >> +     /*
> >> >> +      * Enforce the controller to be turned on.
> >> >> +      *
> >> >> +      * The iDMA is turned off in ->probe() and looses context during system
> >> >> +      * suspend / resume cycle. That's why we have to enable it each time we
> >> >> +      * use it.
> >> >> +      */
> >> >> +     idma64_on(idma64);
> >> > would it be better that you do this in resume and runtime_resume cycle. That
> >> > way it need not be called for every channel init
> >>
> >> Mika, I don't remember details here, but this piece came from you. Can
> >> you shed a light?
> >>
> >> My understanding that DMA IP is private to the host controller and has
> >> the same power rail. Thus, there is no need to do separate power
> >> management for it, which makes things more complicated for no profit.
> >> It is also needed for time period from probe till first transfer
> >> (otherwise we have to check the status of DMA anyway and enable it if
> >> required), which currently remains DMA off.
> > so who does runtime management of this power rail and whosoever does that
> > how do they ensure the dma is not active at that time?
> 
> The host controller driver knows this and it makes it so.
> Anyway, even if we introduce PM callbacks in this driver how to solve
> the issue to run very first transfer in a neat way?

And how does it know?

In PM callback case, your runtime resume should enable the controller

> >> >> +static size_t idma64_desc_size(struct idma64_desc *desc, unsigned int active)
> >> >> +{
> >> >> +     size_t bytes = 0;
> >> >> +     unsigned int i;
> >> >> +
> >> >> +     for (i = active; i < desc->ndesc; i++)
> >> >> +             bytes += desc->hw[i].len;
> >> > it can help for the non active case to have this size stored in desc while
> >> > preparing, that gets rid of runtime calculation for that case
> >>
> >> I don't see much improvement, since it's a rare cases, but waste of
> >> memory. If you consider that should be done I can implement it.
> > The users of timing information will invoke this in non sleeping context so
> > it will help in using one more variable, maybe a good trade off. Also if
> > this is a rare case do you have use of this calculation and code?
> 
> I have no calculations done, but the residue check is used only in one
> case for now, i.e. in the UART driver (see 8250_dma.c). That code
> serializes requests, so it will not ask for pending descriptor size.
> The case when we start the DMA and immediately ask for residue is
> unlike (when UART RX timeout happens).
> 
> >> >> +static int idma64_terminate_all(struct dma_chan *chan)
> >> >> +{
> >> >> +     struct idma64_chan *idma64c = to_idma64_chan(chan);
> >> >> +     unsigned long flags;
> >> >> +     LIST_HEAD(head);
> >> >> +
> >> >> +     spin_lock_irqsave(&idma64c->vchan.lock, flags);
> >> >> +     idma64_stop_transfer(idma64c);
> >> > I dont think this is the right method for terminate. Can you check, it
> >> > might be that we have to suspend the channel before terminating an active
> >> > one. For non active case this should be okay
> >>
> >> Do you mean hardware can become into wrong state?
> > Yes at least that was true for 32bit version of this IP
> 
> Can you elaborate what exactly is happening? We can do a test then and
> check. For now on we experienced no problems.

While you are doing transfers, try terminating. After that doesnt next
transaction on same channel work well, if not then please try suspending
first before terminating

> >> Only what can actually happen is the data loss which is in DMA FIFO,
> >> but we already know we would like to terminate the transfer we don't
> >> care about any data loss since that.
> > The terminate flow expect you to suspend the channel first and then terminate
> 
> Hm... I didn't see anything in the specification. Can you point out
> where it's described?

I dont have specs to point, but we did see this issue on 32bit BYT
iDMA and designer told us to follow this flow

> >> >> +static const struct dev_pm_ops idma64_dev_pm_ops = {
> >> >> +     SET_LATE_SYSTEM_SLEEP_PM_OPS(idma64_suspend_late, idma64_resume_early)
> >> > any reason why late ops?
> >>
> >> We already discussed this several times. There is no dependency
> >> between DMA and its users during PM cycles. We have to be sure that
> >> device (host controller) is quiescent before we stop DMA.
> > And the realities change! you should be looking at deferred probing for this
> > rather than using late ops.
> 
> > Clients should be suspended first which means
> > they should be probed last and in the probe ensure all the resources (dma)
> > are available if not defer the probe.
> 
> Ah, deferred probe here makes no sense. We have set probing ordering
> in the MFD driver (for our drivers).
> Looks like we may use normal calls.

Most of the cases where people use last ops doesnt warrant them so checking
is always a good idea :)

-- 
~Vinod

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists