Re: [PATCH v2 5/7] dmaengine: dw: platform: power on device on shutdown

[Shevchenko, Andriy]

On Thu, 2015-11-26 at 19:58 +0200, Andy Shevchenko wrote:
> On Thu, 2015-11-26 at 23:11 +0530, Vinod Koul wrote:
> > On Thu, Nov 26, 2015 at 07:24:48PM +0200, Andy Shevchenko wrote:
> > > On Thu, 2015-11-26 at 22:31 +0530, Vinod Koul wrote:
> > > > On Thu, Nov 26, 2015 at 05:19:11PM +0200, Andy Shevchenko
> > > > wrote:
> > > > > We have to call dw_dma_disable() to stop any ongoing
> > > > > transfer.
> > > > 
> > > > Ok
> > > > 
> > > > > On some platforms we can't do that since DMA device is
> > > > > powered
> > > > > off.
> > > > 
> > > > Umm, you said we have ongoing transfer which means DMA should
> > > > be
> > > > on..!!
> > > 
> > > Yes, that's true for HSW/BDW and non-affected BYT/CHT.
> > 
> > Can you please explain even when DMA is in use how can device be
> > powered
> > off? That does not sound right to me. 
> 
> It can't, but the problem is we can't distinguish that in this
> routine!
> We simple do *not* know the actual power state of DMA.
> 
> These calls *ensures* that DMA is powered on. Yes, the call to
> dw_dma_off() when it used to be powered off sounds silly, I agree,
> though I see no upstreamable (non-hackish) solution for that.
> 
> Previously I proposed to remove .shutdown hook completely, you were
> objecting.

And second approach with PMC involved was also rejected by you since it
was too hackish, which I completely agree with.

> 
> > Is this on GP DMA on BYT/CHT or
> > something else?
> 
> Correct. Affected platforms are BYT-T and some or all of BSW/CHT
> depending on firmware in use.
> 
> > Â
> > > Like I mentioned here is no possibility to know which platform we
> > > run
> > > on.
> > > 
> > > Would you like to test this on a real device? We can provide you
> > > a
> > > login.
> > > 
> > > > 
> > > > > Moreover we have no
> > > > > possibility at that point to check if the platform is
> > > > > affected
> > > > > or
> > > > > not. That's
> > > > > why we call pm_runtime_get_sync() / pm_runtime_put()
> > > > > unconditionally. On the
> > > > > other hand we can't use pm_runtime_suspended() because
> > > > > runtime
> > > > > PM
> > > > > framework is
> > > > > not fully used by the driver.
> > > > 
> > > > Shouldn't that be fixed?
> > > 
> > > Do you have any solution how?
> > > 
> > > Rough approach is to turn on it on channel allocation and turn
> > > off
> > > on
> > > freeing resources. The obvious downside of this solution is power
> > > consumption of idling device.
> > 
> > But in that case, the clients should not hold ref of dma chan when
> > idle and
> > allocate only when required which is a resonable expectation
> 
> There is not the case for few drivers. At least for us it's spi-
> pxa2xx
> one. It requires channels on its ->probe() stage. Jarkko is Cc'ed
> here,
> you may ask him as he is our maintainer for the SPI.
> 

-- 
Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.