Re: [PATCH] dmaengine: cppi41: Fix cppi41_dma_prep_slave_sg() when idle

[Peter Ujfalusi <peter.ujfalusi@ti.com>]

On 23/10/2019 23.18, Tony Lindgren wrote:
> * Peter Ujfalusi <peter.ujfalusi@ti.com> [191023 19:55]:
>> On 10/23/19 10:18 PM, Tony Lindgren wrote:
>>> We'd have to allow dma consumer driver call pm_runtime_get_sync()
>>> on the dma device. Something similar maybe to what we have
>>> for phy_pm_runtime_get_sync(). Or just get the device handle for
>>> dma so the consumer can call pm_runtime_get_sync() on it.
>>
>> How much a pm_runtime_get_sync(dmadev) is different when it is issued by
>> the client driver compared to when the dma driver issues it for it's own
>> device?
> 
> Well the consumer device could call pm_runtime_get_sync(dmadev)
> when the USB cable is connected for example, and then call
> pm_runtime_pu(dmadev) when let's say the USB cable is disconnected.

And the USB cable connect/disconnect is handled in interrupt -> you need
to call pm_runtime_get_sync(dmadev) from interrupt context and need to
mark the dmadev to pm_runtime_irq_safe()

> Without using pm_runtime_irq_safe() we currently don't have a
> clear path for doing this where the pm_runtime_get_sync(dmadev)
> may sleep.
> 
>> But I still fail to see the difference between the events before this
>> patch and with the case when there is a 100ms delay between prep_sg and
>> issue_pending.
>>
>> Before this patch:
>>
>> prep_sg()
>> issue_pending() <- runtime_get() /  put_autosuspend()
>> 		   _not_ starting transfer
>> runtime_resume() <- starts the transfer
>>
>> With this patch and than 100ms delay between prep_sg and issue_pending:
>>
>> prep_sg() <- runtime_get() /  put_autosuspend()
>> runtime_resume() <- not starting transfer
>> issue_pending() <- runtime_get() /  put_autosuspend()
>> 		   starts the transfer
>>
>> With this patch, but more than 100ms delay in between:
>>
>> prep_sg() <- runtime_get() /  put_autosuspend()
>> runtime_resume() <- not starting transfer
>>> 100ms delay
>> runtime_suspend()
>> issue_pending() <- runtime_get() /  put_autosuspend()
>> 		   _not_ starting transfer
>> runtime_resume() <- starts the transfer
>>
>> pm_runtime_get_sync() in issue_pending would be the solution to avoid
>> delayed execution, but the usb driver should not assume that DMA is
>> completed as soon as issue_pending returned.
> 
> Oh I see. Yes the consumer driver would need to check for
> the completed dma transfer in all cases. The delay issues
> should not currently happen in the musb_ep_program() problem
> case as it gets called from IRQ context.

the cppi41 driver solely relies on irq to check is the transfer is
completed (based on the cookie status).
So yeah, musb have no other choice than trust that the transfer is done
in a timely manner.

> And no, adding pm_runtime_get_sync() to issue_pending is not
> a solution. There may be clocks and regulators that need to
> be powered up, and we don't want to use pm_runtime_irq_safe()
> because of the permanent use count on the parent.

I think the only way to handle this is to keep the DMA enabled as long
as the USB cable is connected.
Either to introduce dma_pm_runtime_get_sync(struct dma_chan *c) and
dma_pm_runtime_put(struct dma_chan *c) or some better name.

It's use would be optional, but for USB you would call them for cable
connect the get_sync (from a work) and put it on disconnect.

The driver internally would not need to be changed, I think this patch
could be removed as well.

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki