Tearing down DMA transfer setup after DMA client has finished

Tearing down DMA transfer setup after DMA client has finished

[Mason]

Hello,

On my platform, setting up a DMA transfer is a two-step process:

1) configure the "switch box" to connect a device to a memory channel
2) configure the transfer details (address, size, command)

When the transfer is done, the sbox setup can be torn down,
and the DMA driver can start another transfer.

The current software architecture for my NFC (NAND Flash controller)
driver is as follows (for one DMA transfer).

  sg_init_one
  dma_map_sg
  dmaengine_prep_slave_sg
  dmaengine_submit
  dma_async_issue_pending
  configure_NFC_transfer
  wait_for_IRQ_from_DMA_engine // via DMA_PREP_INTERRUPT
  wait_for_NFC_idle
  dma_unmap_sg


The problem is that the DMA driver tears down the sbox setup
as soon as it receives the IRQ. However, when writing to the
device, the interrupt only means "I have pushed all data from
memory to the memory channel". These data have not reached
the device yet, and may still be "in flight". Thus the sbox
setup can only be torn down after the NFC is idle.

How do I call back into the DMA driver after wait_for_NFC_idle,
to request sbox tear down?

The new architecture would become:

  sg_init_one
  dma_map_sg
  dmaengine_prep_slave_sg
  dmaengine_submit
  dma_async_issue_pending
  configure_NFC_transfer
  wait_for_IRQ_from_DMA_engine // via DMA_PREP_INTERRUPT
  wait_for_NFC_idle
  request_sbox_tear_down /*** HOW TO DO THAT ***/
  dma_unmap_sg


As far as I can tell, my NFC driver should call dmaengine_synchronize ??
(In other words request_sbox_tear_down == dmaengine_synchronize)

So the DMA driver should implement the device_synchronize hook,
and tear the sbox down in that function.

Is that correct? Or am I on the wrong track?

Regards.

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Mason <slash.tmp@free.fr> writes:

> Hello,
>
> On my platform, setting up a DMA transfer is a two-step process:
>
> 1) configure the "switch box" to connect a device to a memory channel
> 2) configure the transfer details (address, size, command)
>
> When the transfer is done, the sbox setup can be torn down,
> and the DMA driver can start another transfer.
>
> The current software architecture for my NFC (NAND Flash controller)
> driver is as follows (for one DMA transfer).
>
>   sg_init_one
>   dma_map_sg
>   dmaengine_prep_slave_sg
>   dmaengine_submit
>   dma_async_issue_pending
>   configure_NFC_transfer
>   wait_for_IRQ_from_DMA_engine // via DMA_PREP_INTERRUPT
>   wait_for_NFC_idle
>   dma_unmap_sg
>
> The problem is that the DMA driver tears down the sbox setup
> as soon as it receives the IRQ. However, when writing to the
> device, the interrupt only means "I have pushed all data from
> memory to the memory channel". These data have not reached
> the device yet, and may still be "in flight". Thus the sbox
> setup can only be torn down after the NFC is idle.
>
> How do I call back into the DMA driver after wait_for_NFC_idle,
> to request sbox tear down?
>
> The new architecture would become:
>
>   sg_init_one
>   dma_map_sg
>   dmaengine_prep_slave_sg
>   dmaengine_submit
>   dma_async_issue_pending
>   configure_NFC_transfer
>   wait_for_IRQ_from_DMA_engine // via DMA_PREP_INTERRUPT
>   wait_for_NFC_idle
>   request_sbox_tear_down /*** HOW TO DO THAT ***/
>   dma_unmap_sg
>
> As far as I can tell, my NFC driver should call dmaengine_synchronize ??
> (In other words request_sbox_tear_down == dmaengine_synchronize)
>
> So the DMA driver should implement the device_synchronize hook,
> and tear the sbox down in that function.
>
> Is that correct? Or am I on the wrong track?

dmaengine_synchronize() is not meant for this.  See the documentation:

/**
 * dmaengine_synchronize() - Synchronize DMA channel termination
 * @chan: The channel to synchronize
 *
 * Synchronizes to the DMA channel termination to the current context. When this
 * function returns it is guaranteed that all transfers for previously issued
 * descriptors have stopped and and it is safe to free the memory assoicated
 * with them. Furthermore it is guaranteed that all complete callback functions
 * for a previously submitted descriptor have finished running and it is safe to
 * free resources accessed from within the complete callbacks.
 *
 * The behavior of this function is undefined if dma_async_issue_pending() has
 * been called between dmaengine_terminate_async() and this function.
 *
 * This function must only be called from non-atomic context and must not be
 * called from within a complete callback of a descriptor submitted on the same
 * channel.
 */

This is for use after a dmaengine_terminate_async() call to wait for the
dma engine to finish whatever it was doing.  This is not the problem
here.  Your problem is that the dma engine interrupt fires before the
transfer is actually complete.  Although you get an indication from the
target device when it has received all the data, there is no way to make
the dma driver wait for this.

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Mason]

On 23/11/2016 13:13, Måns Rullgård wrote:

> Mason wrote:
> 
>> On my platform, setting up a DMA transfer is a two-step process:
>>
>> 1) configure the "switch box" to connect a device to a memory channel
>> 2) configure the transfer details (address, size, command)
>>
>> When the transfer is done, the sbox setup can be torn down,
>> and the DMA driver can start another transfer.
>>
>> The current software architecture for my NFC (NAND Flash controller)
>> driver is as follows (for one DMA transfer).
>>
>>   sg_init_one
>>   dma_map_sg
>>   dmaengine_prep_slave_sg
>>   dmaengine_submit
>>   dma_async_issue_pending
>>   configure_NFC_transfer
>>   wait_for_IRQ_from_DMA_engine // via DMA_PREP_INTERRUPT
>>   wait_for_NFC_idle
>>   dma_unmap_sg
>>
>> The problem is that the DMA driver tears down the sbox setup
>> as soon as it receives the IRQ. However, when writing to the
>> device, the interrupt only means "I have pushed all data from
>> memory to the memory channel". These data have not reached
>> the device yet, and may still be "in flight". Thus the sbox
>> setup can only be torn down after the NFC is idle.
>>
>> How do I call back into the DMA driver after wait_for_NFC_idle,
>> to request sbox tear down?
>>
>> The new architecture would become:
>>
>>   sg_init_one
>>   dma_map_sg
>>   dmaengine_prep_slave_sg
>>   dmaengine_submit
>>   dma_async_issue_pending
>>   configure_NFC_transfer
>>   wait_for_IRQ_from_DMA_engine // via DMA_PREP_INTERRUPT
>>   wait_for_NFC_idle
>>   request_sbox_tear_down /*** HOW TO DO THAT ***/
>>   dma_unmap_sg
>>
>> As far as I can tell, my NFC driver should call dmaengine_synchronize ??
>> (In other words request_sbox_tear_down == dmaengine_synchronize)
>>
>> So the DMA driver should implement the device_synchronize hook,
>> and tear the sbox down in that function.
>>
>> Is that correct? Or am I on the wrong track?
> 
> dmaengine_synchronize() is not meant for this.  See the documentation:
> 
> /**
>  * dmaengine_synchronize() - Synchronize DMA channel termination
>  * @chan: The channel to synchronize
>  *
>  * Synchronizes to the DMA channel termination to the current context. When this
>  * function returns it is guaranteed that all transfers for previously issued
>  * descriptors have stopped and and it is safe to free the memory assoicated
>  * with them. Furthermore it is guaranteed that all complete callback functions
>  * for a previously submitted descriptor have finished running and it is safe to
>  * free resources accessed from within the complete callbacks.
>  *
>  * The behavior of this function is undefined if dma_async_issue_pending() has
>  * been called between dmaengine_terminate_async() and this function.
>  *
>  * This function must only be called from non-atomic context and must not be
>  * called from within a complete callback of a descriptor submitted on the same
>  * channel.
>  */
> 
> This is for use after a dmaengine_terminate_async() call to wait for the
> dma engine to finish whatever it was doing.  This is not the problem
> here.  Your problem is that the dma engine interrupt fires before the
> transfer is actually complete.  Although you get an indication from the
> target device when it has received all the data, there is no way to make
> the dma driver wait for this.

Hello Mans,

I'm confused. Are you saying there is no solution to my problem
within the existing DMA framework?

In its current form, the tangox-dma.c driver will fail randomly
for writes to a device (SATA, NFC).

Maybe an extra hook can be added to the DMA framework?

I'd like to hear from the framework's maintainers. Perhaps they
can provide some guidance.

Regards.

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Mason <slash.tmp@free.fr> writes:

> On 23/11/2016 13:13, Måns Rullgård wrote:
>
>> Mason wrote:
>> 
>>> On my platform, setting up a DMA transfer is a two-step process:
>>>
>>> 1) configure the "switch box" to connect a device to a memory channel
>>> 2) configure the transfer details (address, size, command)
>>>
>>> When the transfer is done, the sbox setup can be torn down,
>>> and the DMA driver can start another transfer.
>>>
>>> The current software architecture for my NFC (NAND Flash controller)
>>> driver is as follows (for one DMA transfer).
>>>
>>>   sg_init_one
>>>   dma_map_sg
>>>   dmaengine_prep_slave_sg
>>>   dmaengine_submit
>>>   dma_async_issue_pending
>>>   configure_NFC_transfer
>>>   wait_for_IRQ_from_DMA_engine // via DMA_PREP_INTERRUPT
>>>   wait_for_NFC_idle
>>>   dma_unmap_sg
>>>
>>> The problem is that the DMA driver tears down the sbox setup
>>> as soon as it receives the IRQ. However, when writing to the
>>> device, the interrupt only means "I have pushed all data from
>>> memory to the memory channel". These data have not reached
>>> the device yet, and may still be "in flight". Thus the sbox
>>> setup can only be torn down after the NFC is idle.
>>>
>>> How do I call back into the DMA driver after wait_for_NFC_idle,
>>> to request sbox tear down?
>>>
>>> The new architecture would become:
>>>
>>>   sg_init_one
>>>   dma_map_sg
>>>   dmaengine_prep_slave_sg
>>>   dmaengine_submit
>>>   dma_async_issue_pending
>>>   configure_NFC_transfer
>>>   wait_for_IRQ_from_DMA_engine // via DMA_PREP_INTERRUPT
>>>   wait_for_NFC_idle
>>>   request_sbox_tear_down /*** HOW TO DO THAT ***/
>>>   dma_unmap_sg
>>>
>>> As far as I can tell, my NFC driver should call dmaengine_synchronize ??
>>> (In other words request_sbox_tear_down == dmaengine_synchronize)
>>>
>>> So the DMA driver should implement the device_synchronize hook,
>>> and tear the sbox down in that function.
>>>
>>> Is that correct? Or am I on the wrong track?
>> 
>> dmaengine_synchronize() is not meant for this.  See the documentation:
>> 
>> /**
>>  * dmaengine_synchronize() - Synchronize DMA channel termination
>>  * @chan: The channel to synchronize
>>  *
>>  * Synchronizes to the DMA channel termination to the current context. When this
>>  * function returns it is guaranteed that all transfers for previously issued
>>  * descriptors have stopped and and it is safe to free the memory assoicated
>>  * with them. Furthermore it is guaranteed that all complete callback functions
>>  * for a previously submitted descriptor have finished running and it is safe to
>>  * free resources accessed from within the complete callbacks.
>>  *
>>  * The behavior of this function is undefined if dma_async_issue_pending() has
>>  * been called between dmaengine_terminate_async() and this function.
>>  *
>>  * This function must only be called from non-atomic context and must not be
>>  * called from within a complete callback of a descriptor submitted on the same
>>  * channel.
>>  */
>> 
>> This is for use after a dmaengine_terminate_async() call to wait for the
>> dma engine to finish whatever it was doing.  This is not the problem
>> here.  Your problem is that the dma engine interrupt fires before the
>> transfer is actually complete.  Although you get an indication from the
>> target device when it has received all the data, there is no way to make
>> the dma driver wait for this.
>
> Hello Mans,
>
> I'm confused. Are you saying there is no solution to my problem
> within the existing DMA framework?
>
> In its current form, the tangox-dma.c driver will fail randomly
> for writes to a device (SATA, NFC).
>
> Maybe an extra hook can be added to the DMA framework?
>
> I'd like to hear from the framework's maintainers. Perhaps they
> can provide some guidance.

You could have the dma descriptor callback wait for the receiving device
to finish.  Bear in mind this runs from a tasklet, so it's not allowed
to sleep.

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Mason]

On 23/11/2016 18:21, Måns Rullgård wrote:

> Mason writes:
> 
>> On 23/11/2016 13:13, Måns Rullgård wrote:
>>
>>> Mason wrote:
>>>
>>>> On my platform, setting up a DMA transfer is a two-step process:
>>>>
>>>> 1) configure the "switch box" to connect a device to a memory channel
>>>> 2) configure the transfer details (address, size, command)
>>>>
>>>> When the transfer is done, the sbox setup can be torn down,
>>>> and the DMA driver can start another transfer.
>>>>
>>>> The current software architecture for my NFC (NAND Flash controller)
>>>> driver is as follows (for one DMA transfer).
>>>>
>>>>   sg_init_one
>>>>   dma_map_sg
>>>>   dmaengine_prep_slave_sg
>>>>   dmaengine_submit
>>>>   dma_async_issue_pending
>>>>   configure_NFC_transfer
>>>>   wait_for_IRQ_from_DMA_engine // via DMA_PREP_INTERRUPT
>>>>   wait_for_NFC_idle
>>>>   dma_unmap_sg
>>>>
>>>> The problem is that the DMA driver tears down the sbox setup
>>>> as soon as it receives the IRQ. However, when writing to the
>>>> device, the interrupt only means "I have pushed all data from
>>>> memory to the memory channel". These data have not reached
>>>> the device yet, and may still be "in flight". Thus the sbox
>>>> setup can only be torn down after the NFC is idle.
>>>>
>>>> How do I call back into the DMA driver after wait_for_NFC_idle,
>>>> to request sbox tear down?
>>>>
>>>> The new architecture would become:
>>>>
>>>>   sg_init_one
>>>>   dma_map_sg
>>>>   dmaengine_prep_slave_sg
>>>>   dmaengine_submit
>>>>   dma_async_issue_pending
>>>>   configure_NFC_transfer
>>>>   wait_for_IRQ_from_DMA_engine // via DMA_PREP_INTERRUPT
>>>>   wait_for_NFC_idle
>>>>   request_sbox_tear_down /*** HOW TO DO THAT ***/
>>>>   dma_unmap_sg
>>>>
>>>> As far as I can tell, my NFC driver should call dmaengine_synchronize ??
>>>> (In other words request_sbox_tear_down == dmaengine_synchronize)
>>>>
>>>> So the DMA driver should implement the device_synchronize hook,
>>>> and tear the sbox down in that function.
>>>>
>>>> Is that correct? Or am I on the wrong track?
>>>
>>> dmaengine_synchronize() is not meant for this.  See the documentation:
>>>
>>> /**
>>>  * dmaengine_synchronize() - Synchronize DMA channel termination
>>>  * @chan: The channel to synchronize
>>>  *
>>>  * Synchronizes to the DMA channel termination to the current context. When this
>>>  * function returns it is guaranteed that all transfers for previously issued
>>>  * descriptors have stopped and and it is safe to free the memory assoicated
>>>  * with them. Furthermore it is guaranteed that all complete callback functions
>>>  * for a previously submitted descriptor have finished running and it is safe to
>>>  * free resources accessed from within the complete callbacks.
>>>  *
>>>  * The behavior of this function is undefined if dma_async_issue_pending() has
>>>  * been called between dmaengine_terminate_async() and this function.
>>>  *
>>>  * This function must only be called from non-atomic context and must not be
>>>  * called from within a complete callback of a descriptor submitted on the same
>>>  * channel.
>>>  */
>>>
>>> This is for use after a dmaengine_terminate_async() call to wait for the
>>> dma engine to finish whatever it was doing.  This is not the problem
>>> here.  Your problem is that the dma engine interrupt fires before the
>>> transfer is actually complete.  Although you get an indication from the
>>> target device when it has received all the data, there is no way to make
>>> the dma driver wait for this.
>>
>> Hello Mans,
>>
>> I'm confused. Are you saying there is no solution to my problem
>> within the existing DMA framework?
>>
>> In its current form, the tangox-dma.c driver will fail randomly
>> for writes to a device (SATA, NFC).
>>
>> Maybe an extra hook can be added to the DMA framework?
>>
>> I'd like to hear from the framework's maintainers. Perhaps they
>> can provide some guidance.
> 
> You could have the dma descriptor callback wait for the receiving device
> to finish.  Bear in mind this runs from a tasklet, so it's not allowed
> to sleep.

Thanks for the suggestion, but I don't think it works :-(

This is my DMA desc callback:

static void tango_dma_callback(void *arg)
{
	printk("%s from %pf\n", __func__, __builtin_return_address(0));
	mdelay(10000);
	printk("DONE FAKE SPINNING\n");
	complete(arg);
}

I also added
    printk("%s from %pf\n", __func__, __builtin_return_address(0));
after tangox_dma_pchan_detach(pchan);

And I get this output:

[   35.085854] SETUP DMA
[   35.088272] START NAND TRANSFER
[   35.091670] tangox_dma_pchan_start from tangox_dma_irq
[   35.096882] tango_dma_callback from vchan_complete
[   45.102513] DONE FAKE SPINNING

So the IRQ rolls in, the ISR calls tangox_dma_pchan_start,
which calls tangox_dma_pchan_detach to tear down the sbox
setup; and only sometime later does the DMA framework call
my callback function.

So far, the work-arounds I've tested are:

1) delay sbox tear-down by 10 µs in tangox_dma_pchan_detach.
2) statically setup sbox in probe, and never touch it henceforth.

WA1 is fragile, it might break for devices other than NFC.
WA2 is what I used when I wrote the NFC driver.

Can tangox_dma_irq() be changed to have the framework call
the client's callback *before* tangox_dma_pchan_start?

(Thinking out loud) The DMA_PREP_INTERRUPT requests that the
DMA framework invoke the callback from tasklet context,
maybe a different flag DMA_PREP_INTERRUPT_EX can request
calling the call-back directly from within the ISR?

(Looking at existing flags) Could I use DMA_CTRL_ACK?
Description sounds like some kind hand-shake between
client and dmaengine.

Grepping for DMA_PREP_INTERRUPT, I don't see where the framework
checks that flag to spawn the tasklet? Or is that up to each
driver individually?

Regards.

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Mason <slash.tmp@free.fr> writes:

>>> I'm confused. Are you saying there is no solution to my problem
>>> within the existing DMA framework?
>>>
>>> In its current form, the tangox-dma.c driver will fail randomly
>>> for writes to a device (SATA, NFC).
>>>
>>> Maybe an extra hook can be added to the DMA framework?
>>>
>>> I'd like to hear from the framework's maintainers. Perhaps they
>>> can provide some guidance.
>> 
>> You could have the dma descriptor callback wait for the receiving device
>> to finish.  Bear in mind this runs from a tasklet, so it's not allowed
>> to sleep.
>
> Thanks for the suggestion, but I don't think it works :-(
>
> This is my DMA desc callback:
>
> static void tango_dma_callback(void *arg)
> {
> 	printk("%s from %pf\n", __func__, __builtin_return_address(0));
> 	mdelay(10000);
> 	printk("DONE FAKE SPINNING\n");
> 	complete(arg);
> }
>
> I also added
>     printk("%s from %pf\n", __func__, __builtin_return_address(0));
> after tangox_dma_pchan_detach(pchan);
>
> And I get this output:
>
> [   35.085854] SETUP DMA
> [   35.088272] START NAND TRANSFER
> [   35.091670] tangox_dma_pchan_start from tangox_dma_irq
> [   35.096882] tango_dma_callback from vchan_complete
> [   45.102513] DONE FAKE SPINNING
>
> So the IRQ rolls in, the ISR calls tangox_dma_pchan_start,
> which calls tangox_dma_pchan_detach to tear down the sbox
> setup; and only sometime later does the DMA framework call
> my callback function.

Yes, I realised this soon after I said it.  The dma driver could be
rearranged to make it work though.

> So far, the work-arounds I've tested are:
>
> 1) delay sbox tear-down by 10 µs in tangox_dma_pchan_detach.
> 2) statically setup sbox in probe, and never touch it henceforth.
>
> WA1 is fragile, it might break for devices other than NFC.
> WA2 is what I used when I wrote the NFC driver.
>
> Can tangox_dma_irq() be changed to have the framework call
> the client's callback *before* tangox_dma_pchan_start?
>
> (Thinking out loud) The DMA_PREP_INTERRUPT requests that the
> DMA framework invoke the callback from tasklet context,
> maybe a different flag DMA_PREP_INTERRUPT_EX can request
> calling the call-back directly from within the ISR?
>
> (Looking at existing flags) Could I use DMA_CTRL_ACK?
> Description sounds like some kind hand-shake between
> client and dmaengine.
>
> Grepping for DMA_PREP_INTERRUPT, I don't see where the framework
> checks that flag to spawn the tasklet? Or is that up to each
> driver individually?

Those flags all have defined meanings and abusing them for other things
is a bad idea.  As far as possible, device drivers should work with any
dma driver.

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Mason]

On 24/11/2016 15:17, Måns Rullgård wrote:

> Mason wrote:
> 
>> [   35.085854] SETUP DMA
>> [   35.088272] START NAND TRANSFER
>> [   35.091670] tangox_dma_pchan_start from tangox_dma_irq
>> [   35.096882] tango_dma_callback from vchan_complete
>> [   45.102513] DONE FAKE SPINNING
>>
>> So the IRQ rolls in, the ISR calls tangox_dma_pchan_start,
>> which calls tangox_dma_pchan_detach to tear down the sbox
>> setup; and only sometime later does the DMA framework call
>> my callback function.
> 
> Yes, I realised this soon after I said it.  The dma driver could be
> rearranged to make it work though.

There is a way to make the tasklet run and invoke the callback
before the interrupt service routine proceeds? Can you say more
about this?


>> So far, the work-arounds I've tested are:
>>
>> 1) delay sbox tear-down by 10 µs in tangox_dma_pchan_detach.
>> 2) statically setup sbox in probe, and never touch it henceforth.
>>
>> WA1 is fragile, it might break for devices other than NFC.
>> WA2 is what I used when I wrote the NFC driver.
>>
>> Can tangox_dma_irq() be changed to have the framework call
>> the client's callback *before* tangox_dma_pchan_start?
>>
>> (Thinking out loud) The DMA_PREP_INTERRUPT requests that the
>> DMA framework invoke the callback from tasklet context,
>> maybe a different flag DMA_PREP_INTERRUPT_EX can request
>> calling the call-back directly from within the ISR?
>>
>> (Looking at existing flags) Could I use DMA_CTRL_ACK?
>> Description sounds like some kind hand-shake between
>> client and dmaengine.
>>
>> Grepping for DMA_PREP_INTERRUPT, I don't see where the framework
>> checks that flag to spawn the tasklet? Or is that up to each
>> driver individually?
> 
> Those flags all have defined meanings and abusing them for other things
> is a bad idea.  As far as possible, device drivers should work with any
> dma driver.

I was asking about introducing a new flag, not abusing existing
flags. (I don't understand the semantics of DMA_CTRL_ACK.)

(FWIW, both the NFC and the MBUS agent are custom designs,
not third-party IP blocks.)

Regards.

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Mason <slash.tmp@free.fr> writes:

> On 24/11/2016 15:17, Måns Rullgård wrote:
>
>> Mason wrote:
>> 
>>> [   35.085854] SETUP DMA
>>> [   35.088272] START NAND TRANSFER
>>> [   35.091670] tangox_dma_pchan_start from tangox_dma_irq
>>> [   35.096882] tango_dma_callback from vchan_complete
>>> [   45.102513] DONE FAKE SPINNING
>>>
>>> So the IRQ rolls in, the ISR calls tangox_dma_pchan_start,
>>> which calls tangox_dma_pchan_detach to tear down the sbox
>>> setup; and only sometime later does the DMA framework call
>>> my callback function.
>> 
>> Yes, I realised this soon after I said it.  The dma driver could be
>> rearranged to make it work though.
>
> There is a way to make the tasklet run and invoke the callback
> before the interrupt service routine proceeds?

No, but it would be possible to defer the teardown to the tasklet.
Having said that, I'm not sure it's such a great idea since the tasklet
could be held up for an arbitrary length of time waiting for the target
to finish.

>>> So far, the work-arounds I've tested are:
>>>
>>> 1) delay sbox tear-down by 10 µs in tangox_dma_pchan_detach.
>>> 2) statically setup sbox in probe, and never touch it henceforth.
>>>
>>> WA1 is fragile, it might break for devices other than NFC.
>>> WA2 is what I used when I wrote the NFC driver.
>>>
>>> Can tangox_dma_irq() be changed to have the framework call
>>> the client's callback *before* tangox_dma_pchan_start?
>>>
>>> (Thinking out loud) The DMA_PREP_INTERRUPT requests that the
>>> DMA framework invoke the callback from tasklet context,
>>> maybe a different flag DMA_PREP_INTERRUPT_EX can request
>>> calling the call-back directly from within the ISR?
>>>
>>> (Looking at existing flags) Could I use DMA_CTRL_ACK?
>>> Description sounds like some kind hand-shake between
>>> client and dmaengine.
>>>
>>> Grepping for DMA_PREP_INTERRUPT, I don't see where the framework
>>> checks that flag to spawn the tasklet? Or is that up to each
>>> driver individually?
>> 
>> Those flags all have defined meanings and abusing them for other things
>> is a bad idea.  As far as possible, device drivers should work with any
>> dma driver.
>
> I was asking about introducing a new flag, not abusing existing
> flags. (I don't understand the semantics of DMA_CTRL_ACK.)

This needs more than a new flag anyhow.

> (FWIW, both the NFC and the MBUS agent are custom designs,
> not third-party IP blocks.)

Sure, but who knows what will be in the next chip?

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Vinod Koul]

On Wed, Nov 23, 2016 at 11:25:44AM +0100, Mason wrote:
> Hello,
> 
> On my platform, setting up a DMA transfer is a two-step process:
> 
> 1) configure the "switch box" to connect a device to a memory channel
> 2) configure the transfer details (address, size, command)
> 
> When the transfer is done, the sbox setup can be torn down,
> and the DMA driver can start another transfer.
> 
> The current software architecture for my NFC (NAND Flash controller)
> driver is as follows (for one DMA transfer).
> 
>   sg_init_one
>   dma_map_sg
>   dmaengine_prep_slave_sg
>   dmaengine_submit
>   dma_async_issue_pending
>   configure_NFC_transfer
>   wait_for_IRQ_from_DMA_engine // via DMA_PREP_INTERRUPT
>   wait_for_NFC_idle
>   dma_unmap_sg

Looking at thread and discussion now, first thinking would be to ensure
the transaction is completed properly and then isr fired. You may need
to talk to your HW designers to find a way for that. It is quite common
that DMA controllers will fire and complete whereas the transaction is
still in flight.

If that is not doable, then since you claim this is custom part which
other vendors wont use (hope we are wrong down the line), then we can
have a custom api,

foo_sbox_configure(bool enable, ...);

This can be invoked from NFC driver when required for configuration and
teardown. For very specific cases where people need some specific
configuration we do allow custom APIs.

Only problem with that would be it wont be a generic solution and you
seem to be fine with that.


-- 
~Vinod

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Vinod Koul <vinod.koul@intel.com> writes:

> On Wed, Nov 23, 2016 at 11:25:44AM +0100, Mason wrote:
>> Hello,
>> 
>> On my platform, setting up a DMA transfer is a two-step process:
>> 
>> 1) configure the "switch box" to connect a device to a memory channel
>> 2) configure the transfer details (address, size, command)
>> 
>> When the transfer is done, the sbox setup can be torn down,
>> and the DMA driver can start another transfer.
>> 
>> The current software architecture for my NFC (NAND Flash controller)
>> driver is as follows (for one DMA transfer).
>> 
>>   sg_init_one
>>   dma_map_sg
>>   dmaengine_prep_slave_sg
>>   dmaengine_submit
>>   dma_async_issue_pending
>>   configure_NFC_transfer
>>   wait_for_IRQ_from_DMA_engine // via DMA_PREP_INTERRUPT
>>   wait_for_NFC_idle
>>   dma_unmap_sg
>
> Looking at thread and discussion now, first thinking would be to ensure
> the transaction is completed properly and then isr fired. You may need
> to talk to your HW designers to find a way for that. It is quite common
> that DMA controllers will fire and complete whereas the transaction is
> still in flight.

The hardware is what it is, and it has been deployed in some form or
other for years.

> If that is not doable, then since you claim this is custom part which
> other vendors wont use (hope we are wrong down the line), then we can
> have a custom api,
>
> foo_sbox_configure(bool enable, ...);
>
> This can be invoked from NFC driver when required for configuration and
> teardown. For very specific cases where people need some specific
> configuration we do allow custom APIs.
>
> Only problem with that would be it wont be a generic solution and you
> seem to be fine with that.

The same DMA unit is also used for SATA, which is an off the shelf
Designware controller with an in-kernel driver.  This interrupt timing
glitch can actually explain some intermittent errors I've observed with
it.

One possible solution is to add a new function for device drivers to
call when their end is complete.  Existing DMA drivers would simply do
nothing, and device drivers could have this call added whenever the need
arises.

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Russell King - ARM Linux]

On Fri, Nov 25, 2016 at 10:25:49AM +0530, Vinod Koul wrote:
> Looking at thread and discussion now, first thinking would be to ensure
> the transaction is completed properly and then isr fired. You may need
> to talk to your HW designers to find a way for that. It is quite common
> that DMA controllers will fire and complete whereas the transaction is
> still in flight.
> 
> If that is not doable, then since you claim this is custom part which
> other vendors wont use (hope we are wrong down the line), then we can
> have a custom api,
> 
> foo_sbox_configure(bool enable, ...);
> 
> This can be invoked from NFC driver when required for configuration and
> teardown. For very specific cases where people need some specific
> configuration we do allow custom APIs.
> 
> Only problem with that would be it wont be a generic solution and you
> seem to be fine with that.

Isn't this just the same problem as PL08x or any other system which
has multiple requests from devices, but only a limited number of
hardware channels - so you have to route the request signals to the
appropriate hardware channels according to the requests queued up?

If so, no new "custom" APIs are required, it's already able to be
solved within the DMA engine drivers...

(We also have more complex situations already supported, such as
PL08x with a FPGA routing on three of its request signals.)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: Tearing down DMA transfer setup after DMA client has finished

[Mason]

On 25/11/2016 05:55, Vinod Koul wrote:

> On Wed, Nov 23, 2016 at 11:25:44AM +0100, Mason wrote:
>
>> On my platform, setting up a DMA transfer is a two-step process:
>>
>> 1) configure the "switch box" to connect a device to a memory channel
>> 2) configure the transfer details (address, size, command)
>>
>> When the transfer is done, the sbox setup can be torn down,
>> and the DMA driver can start another transfer.
>>
>> The current software architecture for my NFC (NAND Flash controller)
>> driver is as follows (for one DMA transfer).
>>
>>   sg_init_one
>>   dma_map_sg
>>   dmaengine_prep_slave_sg
>>   dmaengine_submit
>>   dma_async_issue_pending
>>   configure_NFC_transfer
>>   wait_for_IRQ_from_DMA_engine // via DMA_PREP_INTERRUPT
>>   wait_for_NFC_idle
>>   dma_unmap_sg
> 
> Looking at thread and discussion now, first thinking would be to ensure
> the transaction is completed properly and then isr fired. You may need
> to talk to your HW designers to find a way for that. It is quite common
> that DMA controllers will fire and complete whereas the transaction is
> still in flight.

It seems there is a disconnect between what Linux expects - an IRQ
when the transfer is complete - and the quirks of this HW :-(

On this system, there are MBUS "agents" connected via a "switch box".
An agent fires an IRQ when it has dealt with its *half* of the transfer.

SOURCE_AGENT <---> SBOX <---> DESTINATION_AGENT

Here are the steps for a transfer, in the general case:

1) setup the sbox to connect SOURCE TO DEST
2) configure source to send N bytes
3) configure dest to receive N bytes

When SOURCE_AGENT has sent N bytes, it fires an IRQ
When DEST_AGENT has received N bytes, it fires an IRQ
The sbox connection can be torn down only when the destination
agent has received all bytes.
(And the twist is that some agents do not have an IRQ line.)

The system provides 3 RAM-to-sbox agents (read channels)
and 3 sbox-to-RAM agents (write channels).

The NAND Flash controller read and write agents do not have
IRQ lines.

So for a NAND-to-memory transfer (read from device)
- nothing happens when the NFC has finished sending N bytes to the sbox
- the write channel fires an IRQ when it has received N bytes

In that case, one IRQ fires when the transfer is complete,
like Linux expects.

For a memory-to-NAND transfer (write to device)
- the read channel fires an IRQ when it has sent N bytes
- the NFC driver is supposed to poll the NFC to determine
when the controller has finished writing N bytes

In that case, the IRQ does not indicate that the transfer
is complete, merely that the sending half has finished
its part.

For a memory-to-memory transfer (memcpy)
- the read channel fires an IRQ when it has sent N bytes
- the write channel fires an IRQ when it has received N bytes

So you actually get two IRQs in that case, which I don't
think Linux (or the current DMA driver) expects.

I'm not sure how we're supposed to handle this kind of HW
in Linux? (That's why I started this thread.)


> If that is not doable, then since you claim this is custom part which
> other vendors won't use (hope we are wrong down the line),

I'm not sure how to interpret "you claim this is custom part".
Do you mean I may be wrong, that it is not custom?
I don't know if other vendors may have HW with the same
quirky behavior. What do you mean about being wrong down
the line?

> then we can have a custom api,
> 
> foo_sbox_configure(bool enable, ...);
> 
> This can be invoked from NFC driver when required for configuration and
> teardown. For very specific cases where people need some specific
> configuration we do allow custom APIs.

I don't think that would work. The fundamental issue is
that Linux expects a single IRQ to indicate "transfer
complete". And the driver (as written) starts a new
transfer as soon as the IRQ fires.

But the HW may generate 0, 1, or even 2 IRQs for a single
transfer. And when there is a single IRQ, it may not
indicate "transfer complete" (as seen above).

> Only problem with that would be it wont be a generic solution
> and you seem to be fine with that.

I think it is possible to have a generic solution:
Right now, the callback is called from tasklet context.
If we can have a new flag to have the callback invoked
directly from the ISR, then the driver for the client
device can do what is required.

For example, the NFC driver waits for the IRQ from the
memory agent, and then polls the controller itself.

I can whip up a proof-of-concept if it's better to
illustrate with a patch?

Regards.

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Russell King - ARM Linux <linux@armlinux.org.uk> writes:

> On Fri, Nov 25, 2016 at 10:25:49AM +0530, Vinod Koul wrote:
>> Looking at thread and discussion now, first thinking would be to ensure
>> the transaction is completed properly and then isr fired. You may need
>> to talk to your HW designers to find a way for that. It is quite common
>> that DMA controllers will fire and complete whereas the transaction is
>> still in flight.
>> 
>> If that is not doable, then since you claim this is custom part which
>> other vendors wont use (hope we are wrong down the line), then we can
>> have a custom api,
>> 
>> foo_sbox_configure(bool enable, ...);
>> 
>> This can be invoked from NFC driver when required for configuration and
>> teardown. For very specific cases where people need some specific
>> configuration we do allow custom APIs.
>> 
>> Only problem with that would be it wont be a generic solution and you
>> seem to be fine with that.
>
> Isn't this just the same problem as PL08x or any other system which
> has multiple requests from devices, but only a limited number of
> hardware channels - so you have to route the request signals to the
> appropriate hardware channels according to the requests queued up?
>
> If so, no new "custom" APIs are required, it's already able to be
> solved within the DMA engine drivers...

That isn't the problem.  The multiplexing of many devices on a limited
number of hardware channels is working fine.  The problem is that (some)
client devices need the routing to remain for some time after the dma
interrupt signals completion.  I'd characterise this hardware as broken,
but there's nothing we can do about that.

The fix has to provide some way for the dma driver to delay reusing a
hardware channel until the client device indicates completion.  If only
a short delay (a few bus cycles) is needed, it is probably acceptable to
rework the driver such that the descriptor completion callback can do
the necessary waiting (e.g. by busy-polling a device status register).
If the delay can be longer, some other method needs to be devised.

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Mason <slash.tmp@free.fr> writes:

> On 25/11/2016 05:55, Vinod Koul wrote:
>
>> On Wed, Nov 23, 2016 at 11:25:44AM +0100, Mason wrote:
>>
>>> On my platform, setting up a DMA transfer is a two-step process:
>>>
>>> 1) configure the "switch box" to connect a device to a memory channel
>>> 2) configure the transfer details (address, size, command)
>>>
>>> When the transfer is done, the sbox setup can be torn down,
>>> and the DMA driver can start another transfer.
>>>
>>> The current software architecture for my NFC (NAND Flash controller)
>>> driver is as follows (for one DMA transfer).
>>>
>>>   sg_init_one
>>>   dma_map_sg
>>>   dmaengine_prep_slave_sg
>>>   dmaengine_submit
>>>   dma_async_issue_pending
>>>   configure_NFC_transfer
>>>   wait_for_IRQ_from_DMA_engine // via DMA_PREP_INTERRUPT
>>>   wait_for_NFC_idle
>>>   dma_unmap_sg
>> 
>> Looking at thread and discussion now, first thinking would be to ensure
>> the transaction is completed properly and then isr fired. You may need
>> to talk to your HW designers to find a way for that. It is quite common
>> that DMA controllers will fire and complete whereas the transaction is
>> still in flight.
>
> It seems there is a disconnect between what Linux expects - an IRQ
> when the transfer is complete - and the quirks of this HW :-(
>
> On this system, there are MBUS "agents" connected via a "switch box".
> An agent fires an IRQ when it has dealt with its *half* of the transfer.
>
> SOURCE_AGENT <---> SBOX <---> DESTINATION_AGENT
>
> Here are the steps for a transfer, in the general case:
>
> 1) setup the sbox to connect SOURCE TO DEST
> 2) configure source to send N bytes
> 3) configure dest to receive N bytes
>
> When SOURCE_AGENT has sent N bytes, it fires an IRQ
> When DEST_AGENT has received N bytes, it fires an IRQ
> The sbox connection can be torn down only when the destination
> agent has received all bytes.
> (And the twist is that some agents do not have an IRQ line.)
>
> The system provides 3 RAM-to-sbox agents (read channels)
> and 3 sbox-to-RAM agents (write channels).
>
> The NAND Flash controller read and write agents do not have
> IRQ lines.
>
> So for a NAND-to-memory transfer (read from device)
> - nothing happens when the NFC has finished sending N bytes to the sbox
> - the write channel fires an IRQ when it has received N bytes
>
> In that case, one IRQ fires when the transfer is complete,
> like Linux expects.
>
> For a memory-to-NAND transfer (write to device)
> - the read channel fires an IRQ when it has sent N bytes
> - the NFC driver is supposed to poll the NFC to determine
> when the controller has finished writing N bytes
>
> In that case, the IRQ does not indicate that the transfer
> is complete, merely that the sending half has finished
> its part.

When does your NAND controller signal completion?  When it has received
the DMA data, or only when it has finished the actual write operation?

> I think it is possible to have a generic solution:
> Right now, the callback is called from tasklet context.
> If we can have a new flag to have the callback invoked
> directly from the ISR, then the driver for the client
> device can do what is required.

No, that won't work.  The callback shouldn't run in interrupt context.

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Russell King - ARM Linux]

On Fri, Nov 25, 2016 at 01:07:05PM +0000, Måns Rullgård wrote:
> Russell King - ARM Linux <linux@armlinux.org.uk> writes:
> 
> > On Fri, Nov 25, 2016 at 10:25:49AM +0530, Vinod Koul wrote:
> >> Looking at thread and discussion now, first thinking would be to ensure
> >> the transaction is completed properly and then isr fired. You may need
> >> to talk to your HW designers to find a way for that. It is quite common
> >> that DMA controllers will fire and complete whereas the transaction is
> >> still in flight.
> >> 
> >> If that is not doable, then since you claim this is custom part which
> >> other vendors wont use (hope we are wrong down the line), then we can
> >> have a custom api,
> >> 
> >> foo_sbox_configure(bool enable, ...);
> >> 
> >> This can be invoked from NFC driver when required for configuration and
> >> teardown. For very specific cases where people need some specific
> >> configuration we do allow custom APIs.
> >> 
> >> Only problem with that would be it wont be a generic solution and you
> >> seem to be fine with that.
> >
> > Isn't this just the same problem as PL08x or any other system which
> > has multiple requests from devices, but only a limited number of
> > hardware channels - so you have to route the request signals to the
> > appropriate hardware channels according to the requests queued up?
> >
> > If so, no new "custom" APIs are required, it's already able to be
> > solved within the DMA engine drivers...
> 
> That isn't the problem.  The multiplexing of many devices on a limited
> number of hardware channels is working fine.  The problem is that (some)
> client devices need the routing to remain for some time after the dma
> interrupt signals completion.  I'd characterise this hardware as broken,
> but there's nothing we can do about that.
> 
> The fix has to provide some way for the dma driver to delay reusing a
> hardware channel until the client device indicates completion.  If only
> a short delay (a few bus cycles) is needed, it is probably acceptable to
> rework the driver such that the descriptor completion callback can do
> the necessary waiting (e.g. by busy-polling a device status register).
> If the delay can be longer, some other method needs to be devised.

What I understood from the original mail is:

| The problem is that the DMA driver tears down the sbox setup
| as soon as it receives the IRQ. However, when writing to the
| device, the interrupt only means "I have pushed all data from
| memory to the memory channel". These data have not reached
| the device yet, and may still be "in flight". Thus the sbox
| setup can only be torn down after the NFC is idle.

The interrupt comes in after it's read the the last data from memory,
but the data is still sitting in the engine's buffers and has not yet
been passed to the device.

It sounds like the DMA engine buffers the data on its way to the device,
and it's not clear from the description whether that is done in
response to a request from the device or whether the data is prefetched.
IOW, what the lifetime of the data in the dma engine is.

It seems odd that the DMA engine provides no way to know whether the
channel still contains data that is in-flight to the device, whether
by interrupt (from the descriptions the IRQ is way too early) or by
polling some status register within the DMA engine itself.

If the delay is predictable, why not use a delayed workqueue or a
hrtimer to wait a period after the IRQ before completing the DMA
transaction?  If it's not predictable and you haven't some status
register in the DMA engine hardware that indicates whether there's
remaining data, then the design really is screwed up, and I don't
think there's a reasonable solution to the problem - anything
would be a horrid hack that would be specific to this SoC.

It would be unfair to augment the API and add the burden on everyone
for the new API when 99.999% of the world doesn't require it.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Russell King - ARM Linux <linux@armlinux.org.uk> writes:

> On Fri, Nov 25, 2016 at 01:07:05PM +0000, Måns Rullgård wrote:
>> Russell King - ARM Linux <linux@armlinux.org.uk> writes:
>> 
>> > On Fri, Nov 25, 2016 at 10:25:49AM +0530, Vinod Koul wrote:
>> >> Looking at thread and discussion now, first thinking would be to ensure
>> >> the transaction is completed properly and then isr fired. You may need
>> >> to talk to your HW designers to find a way for that. It is quite common
>> >> that DMA controllers will fire and complete whereas the transaction is
>> >> still in flight.
>> >> 
>> >> If that is not doable, then since you claim this is custom part which
>> >> other vendors wont use (hope we are wrong down the line), then we can
>> >> have a custom api,
>> >> 
>> >> foo_sbox_configure(bool enable, ...);
>> >> 
>> >> This can be invoked from NFC driver when required for configuration and
>> >> teardown. For very specific cases where people need some specific
>> >> configuration we do allow custom APIs.
>> >> 
>> >> Only problem with that would be it wont be a generic solution and you
>> >> seem to be fine with that.
>> >
>> > Isn't this just the same problem as PL08x or any other system which
>> > has multiple requests from devices, but only a limited number of
>> > hardware channels - so you have to route the request signals to the
>> > appropriate hardware channels according to the requests queued up?
>> >
>> > If so, no new "custom" APIs are required, it's already able to be
>> > solved within the DMA engine drivers...
>> 
>> That isn't the problem.  The multiplexing of many devices on a limited
>> number of hardware channels is working fine.  The problem is that (some)
>> client devices need the routing to remain for some time after the dma
>> interrupt signals completion.  I'd characterise this hardware as broken,
>> but there's nothing we can do about that.
>> 
>> The fix has to provide some way for the dma driver to delay reusing a
>> hardware channel until the client device indicates completion.  If only
>> a short delay (a few bus cycles) is needed, it is probably acceptable to
>> rework the driver such that the descriptor completion callback can do
>> the necessary waiting (e.g. by busy-polling a device status register).
>> If the delay can be longer, some other method needs to be devised.
>
> What I understood from the original mail is:
>
> | The problem is that the DMA driver tears down the sbox setup
> | as soon as it receives the IRQ. However, when writing to the
> | device, the interrupt only means "I have pushed all data from
> | memory to the memory channel". These data have not reached
> | the device yet, and may still be "in flight". Thus the sbox
> | setup can only be torn down after the NFC is idle.
>
> The interrupt comes in after it's read the the last data from memory,
> but the data is still sitting in the engine's buffers and has not yet
> been passed to the device.
>
> It sounds like the DMA engine buffers the data on its way to the device,
> and it's not clear from the description whether that is done in
> response to a request from the device or whether the data is prefetched.
> IOW, what the lifetime of the data in the dma engine is.

It's not clear from the information I have exactly when the interrupt
fires, only that it appears to be somewhat too early.

> It seems odd that the DMA engine provides no way to know whether the
> channel still contains data that is in-flight to the device, whether
> by interrupt (from the descriptions the IRQ is way too early) or by
> polling some status register within the DMA engine itself.
>
> If the delay is predictable, why not use a delayed workqueue or a
> hrtimer to wait a period after the IRQ before completing the DMA
> transaction?  If it's not predictable and you haven't some status
> register in the DMA engine hardware that indicates whether there's
> remaining data, then the design really is screwed up, and I don't
> think there's a reasonable solution to the problem - anything
> would be a horrid hack that would be specific to this SoC.

This would hardly be the first screwed up hardware design.  There is a
completion indicator, just not in the dma engine.  For some idiotic
reason, the designers put this responsibility on the client devices and
their respective drivers.

> It would be unfair to augment the API and add the burden on everyone
> for the new API when 99.999% of the world doesn't require it.

I don't think making this particular dma driver wait for the descriptor
callback to return before reusing a channel quite amounts to a horrid
hack.  It certainly wouldn't burden anyone other than the poor drivers
for devices connected to it, all of which are specific to Sigma AFAIK.

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Russell King - ARM Linux]

On Fri, Nov 25, 2016 at 01:50:35PM +0000, Måns Rullgård wrote:
> Russell King - ARM Linux <linux@armlinux.org.uk> writes:
> > It would be unfair to augment the API and add the burden on everyone
> > for the new API when 99.999% of the world doesn't require it.
> 
> I don't think making this particular dma driver wait for the descriptor
> callback to return before reusing a channel quite amounts to a horrid
> hack.  It certainly wouldn't burden anyone other than the poor drivers
> for devices connected to it, all of which are specific to Sigma AFAIK.

Except when you stop to think that delaying in a tasklet is exactly
the same as randomly delaying in an interrupt handler - the tasklet
runs on the return path back to the parent context of an interrupt
handler.  Even if you sleep in the tasklet, you're sleeping on behalf
of the currently executing thread - if it's a RT thread, you effectively
destroy the RT-ness of the thread.  Let's hope no one cares about RT
performance on that hardware...

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Russell King - ARM Linux <linux@armlinux.org.uk> writes:

> On Fri, Nov 25, 2016 at 01:50:35PM +0000, Måns Rullgård wrote:
>> Russell King - ARM Linux <linux@armlinux.org.uk> writes:
>> > It would be unfair to augment the API and add the burden on everyone
>> > for the new API when 99.999% of the world doesn't require it.
>> 
>> I don't think making this particular dma driver wait for the descriptor
>> callback to return before reusing a channel quite amounts to a horrid
>> hack.  It certainly wouldn't burden anyone other than the poor drivers
>> for devices connected to it, all of which are specific to Sigma AFAIK.
>
> Except when you stop to think that delaying in a tasklet is exactly
> the same as randomly delaying in an interrupt handler - the tasklet
> runs on the return path back to the parent context of an interrupt
> handler.  Even if you sleep in the tasklet, you're sleeping on behalf
> of the currently executing thread - if it's a RT thread, you effectively
> destroy the RT-ness of the thread.  Let's hope no one cares about RT
> performance on that hardware...

That's why I suggested to do this only if the needed delay is known to
be no more than a few bus cycles.  The completion callback is currently
the only post-transfer interaction we have between the dma and device
drivers.  To handle an arbitrarily long delay, some new interface will
be required.

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Mason]

On 25/11/2016 12:57, Måns Rullgård wrote:

> The same DMA unit is also used for SATA, which is an off the shelf
> Designware controller with an in-kernel driver.  This interrupt timing
> glitch can actually explain some intermittent errors I've observed with
> it.

FWIW, newer chips embed an AHCI controller, with a dedicated
memory channel.

FWIW2, the HW dev said memory channels are "almost free", and he
would have no problem giving each device their own private channel
read/write pair.

Regards.

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Mason <slash.tmp@free.fr> writes:

> On 25/11/2016 12:57, Måns Rullgård wrote:
>
>> The same DMA unit is also used for SATA, which is an off the shelf
>> Designware controller with an in-kernel driver.  This interrupt timing
>> glitch can actually explain some intermittent errors I've observed with
>> it.
>
> FWIW, newer chips embed an AHCI controller, with a dedicated
> memory channel.
>
> FWIW2, the HW dev said memory channels are "almost free", and he
> would have no problem giving each device their own private channel
> read/write pair.

We still need to deal with the existing hardware.

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Russell King - ARM Linux]

On Fri, Nov 25, 2016 at 02:03:20PM +0000, Måns Rullgård wrote:
> Russell King - ARM Linux <linux@armlinux.org.uk> writes:
> 
> > On Fri, Nov 25, 2016 at 01:50:35PM +0000, Måns Rullgård wrote:
> >> Russell King - ARM Linux <linux@armlinux.org.uk> writes:
> >> > It would be unfair to augment the API and add the burden on everyone
> >> > for the new API when 99.999% of the world doesn't require it.
> >> 
> >> I don't think making this particular dma driver wait for the descriptor
> >> callback to return before reusing a channel quite amounts to a horrid
> >> hack.  It certainly wouldn't burden anyone other than the poor drivers
> >> for devices connected to it, all of which are specific to Sigma AFAIK.
> >
> > Except when you stop to think that delaying in a tasklet is exactly
> > the same as randomly delaying in an interrupt handler - the tasklet
> > runs on the return path back to the parent context of an interrupt
> > handler.  Even if you sleep in the tasklet, you're sleeping on behalf
> > of the currently executing thread - if it's a RT thread, you effectively
> > destroy the RT-ness of the thread.  Let's hope no one cares about RT
> > performance on that hardware...
> 
> That's why I suggested to do this only if the needed delay is known to
> be no more than a few bus cycles.  The completion callback is currently
> the only post-transfer interaction we have between the dma and device
> drivers.  To handle an arbitrarily long delay, some new interface will
> be required.

And now we're back at the point I made a few emails ago about undue
burden which is just about quoted above...

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: Tearing down DMA transfer setup after DMA client has finished

[Mason]

On 25/11/2016 14:11, Måns Rullgård wrote:

> Mason writes:
> 
>> It seems there is a disconnect between what Linux expects - an IRQ
>> when the transfer is complete - and the quirks of this HW :-(
>>
>> On this system, there are MBUS "agents" connected via a "switch box".
>> An agent fires an IRQ when it has dealt with its *half* of the transfer.
>>
>> SOURCE_AGENT <---> SBOX <---> DESTINATION_AGENT
>>
>> Here are the steps for a transfer, in the general case:
>>
>> 1) setup the sbox to connect SOURCE TO DEST
>> 2) configure source to send N bytes
>> 3) configure dest to receive N bytes
>>
>> When SOURCE_AGENT has sent N bytes, it fires an IRQ
>> When DEST_AGENT has received N bytes, it fires an IRQ
>> The sbox connection can be torn down only when the destination
>> agent has received all bytes.
>> (And the twist is that some agents do not have an IRQ line.)
>>
>> The system provides 3 RAM-to-sbox agents (read channels)
>> and 3 sbox-to-RAM agents (write channels).
>>
>> The NAND Flash controller read and write agents do not have
>> IRQ lines.
>>
>> So for a NAND-to-memory transfer (read from device)
>> - nothing happens when the NFC has finished sending N bytes to the sbox
>> - the write channel fires an IRQ when it has received N bytes
>>
>> In that case, one IRQ fires when the transfer is complete,
>> like Linux expects.
>>
>> For a memory-to-NAND transfer (write to device)
>> - the read channel fires an IRQ when it has sent N bytes
>> - the NFC driver is supposed to poll the NFC to determine
>> when the controller has finished writing N bytes
>>
>> In that case, the IRQ does not indicate that the transfer
>> is complete, merely that the sending half has finished
>> its part.
> 
> When does your NAND controller signal completion?  When it has received
> the DMA data, or only when it has finished the actual write operation?

The NAND controller provides a STATUS register.
Bit 31 is the CMD_READY bit.
This bit goes to 0 when the controller is busy, and to 1
when the controller is ready to accept the next command.

The NFC driver is doing:

	res = wait_for_completion_timeout(&tx_done, HZ);
	if (res > 0)
		err = readl_poll_timeout(addr, val, val & CMD_READY, 0, 1000);

So basically, sleep until the memory agent IRQ falls,
then spin until the controller is idle.

Did you see that adding a 10 µs delay at the start of
tangox_dma_pchan_detach() makes the system no longer
fail (passes an mtd_speedtest).

>> I think it is possible to have a generic solution:
>> Right now, the callback is called from tasklet context.
>> If we can have a new flag to have the callback invoked
>> directly from the ISR, then the driver for the client
>> device can do what is required.
> 
> No, that won't work.  The callback shouldn't run in interrupt context.

What if the callback only spun for, at most, 10 µs ?

	readl_poll_timeout(addr, val, val & CMD_READY, 0, 10);

Regards.

Re: Tearing down DMA transfer setup after DMA client has finished

[Mason]

On 25/11/2016 15:12, Måns Rullgård wrote:

> Mason writes:
> 
>> On 25/11/2016 12:57, Måns Rullgård wrote:
>>
>>> The same DMA unit is also used for SATA, which is an off the shelf
>>> Designware controller with an in-kernel driver.  This interrupt timing
>>> glitch can actually explain some intermittent errors I've observed with
>>> it.
>>
>> FWIW, newer chips embed an AHCI controller, with a dedicated
>> memory channel.
>>
>> FWIW2, the HW dev said memory channels are "almost free", and he
>> would have no problem giving each device their own private channel
>> read/write pair.
> 
> We still need to deal with the existing hardware.

Can you confirm that your MBUS driver, in its current form,
does not support memcpy-type transfers, which generate two
IRQs (one from send agent, one from receive agent)?

Do you plan to support that, or is it just too quirky?

Regards.

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Mason <slash.tmp@free.fr> writes:

> On 25/11/2016 14:11, Måns Rullgård wrote:
>
>> Mason writes:
>> 
>>> It seems there is a disconnect between what Linux expects - an IRQ
>>> when the transfer is complete - and the quirks of this HW :-(
>>>
>>> On this system, there are MBUS "agents" connected via a "switch box".
>>> An agent fires an IRQ when it has dealt with its *half* of the transfer.
>>>
>>> SOURCE_AGENT <---> SBOX <---> DESTINATION_AGENT
>>>
>>> Here are the steps for a transfer, in the general case:
>>>
>>> 1) setup the sbox to connect SOURCE TO DEST
>>> 2) configure source to send N bytes
>>> 3) configure dest to receive N bytes
>>>
>>> When SOURCE_AGENT has sent N bytes, it fires an IRQ
>>> When DEST_AGENT has received N bytes, it fires an IRQ
>>> The sbox connection can be torn down only when the destination
>>> agent has received all bytes.
>>> (And the twist is that some agents do not have an IRQ line.)
>>>
>>> The system provides 3 RAM-to-sbox agents (read channels)
>>> and 3 sbox-to-RAM agents (write channels).
>>>
>>> The NAND Flash controller read and write agents do not have
>>> IRQ lines.
>>>
>>> So for a NAND-to-memory transfer (read from device)
>>> - nothing happens when the NFC has finished sending N bytes to the sbox
>>> - the write channel fires an IRQ when it has received N bytes
>>>
>>> In that case, one IRQ fires when the transfer is complete,
>>> like Linux expects.
>>>
>>> For a memory-to-NAND transfer (write to device)
>>> - the read channel fires an IRQ when it has sent N bytes
>>> - the NFC driver is supposed to poll the NFC to determine
>>> when the controller has finished writing N bytes
>>>
>>> In that case, the IRQ does not indicate that the transfer
>>> is complete, merely that the sending half has finished
>>> its part.
>> 
>> When does your NAND controller signal completion?  When it has received
>> the DMA data, or only when it has finished the actual write operation?
>
> The NAND controller provides a STATUS register.
> Bit 31 is the CMD_READY bit.
> This bit goes to 0 when the controller is busy, and to 1
> when the controller is ready to accept the next command.
>
> The NFC driver is doing:
>
> 	res = wait_for_completion_timeout(&tx_done, HZ);
> 	if (res > 0)
> 		err = readl_poll_timeout(addr, val, val & CMD_READY, 0, 1000);
>
> So basically, sleep until the memory agent IRQ falls,
> then spin until the controller is idle.

This doesn't answer my question.  Waiting for the entire operation to
finish isn't necessary.  The dma driver only needs to wait until all the
data has been received by the nand controller, not until the controller
is completely finished with the command.  Does the nand controller
provide an indication for completion of the dma independently of the
progress of the write command?  The dma glue Sigma added to the
Designware sata controller does this.

> Did you see that adding a 10 µs delay at the start of
> tangox_dma_pchan_detach() makes the system no longer
> fail (passes an mtd_speedtest).

Yes, but maybe that's much longer than is actually necessary.

>>> I think it is possible to have a generic solution:
>>> Right now, the callback is called from tasklet context.
>>> If we can have a new flag to have the callback invoked
>>> directly from the ISR, then the driver for the client
>>> device can do what is required.
>> 
>> No, that won't work.  The callback shouldn't run in interrupt context.
>
> What if the callback only spun for, at most, 10 µs ?
>
> 	readl_poll_timeout(addr, val, val & CMD_READY, 0, 10);

That's far too long to wait in interrupt of tasklet context.

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Russell King - ARM Linux <linux@armlinux.org.uk> writes:

> On Fri, Nov 25, 2016 at 02:03:20PM +0000, Måns Rullgård wrote:
>> Russell King - ARM Linux <linux@armlinux.org.uk> writes:
>> 
>> > On Fri, Nov 25, 2016 at 01:50:35PM +0000, Måns Rullgård wrote:
>> >> Russell King - ARM Linux <linux@armlinux.org.uk> writes:
>> >> > It would be unfair to augment the API and add the burden on everyone
>> >> > for the new API when 99.999% of the world doesn't require it.
>> >> 
>> >> I don't think making this particular dma driver wait for the descriptor
>> >> callback to return before reusing a channel quite amounts to a horrid
>> >> hack.  It certainly wouldn't burden anyone other than the poor drivers
>> >> for devices connected to it, all of which are specific to Sigma AFAIK.
>> >
>> > Except when you stop to think that delaying in a tasklet is exactly
>> > the same as randomly delaying in an interrupt handler - the tasklet
>> > runs on the return path back to the parent context of an interrupt
>> > handler.  Even if you sleep in the tasklet, you're sleeping on behalf
>> > of the currently executing thread - if it's a RT thread, you effectively
>> > destroy the RT-ness of the thread.  Let's hope no one cares about RT
>> > performance on that hardware...
>> 
>> That's why I suggested to do this only if the needed delay is known to
>> be no more than a few bus cycles.  The completion callback is currently
>> the only post-transfer interaction we have between the dma and device
>> drivers.  To handle an arbitrarily long delay, some new interface will
>> be required.
>
> And now we're back at the point I made a few emails ago about undue
> burden which is just about quoted above...

So what do you suggest?  Stick our heads in the sand and pretend
everything is perfect?

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Mason <slash.tmp@free.fr> writes:

> On 25/11/2016 15:12, Måns Rullgård wrote:
>
>> Mason writes:
>> 
>>> On 25/11/2016 12:57, Måns Rullgård wrote:
>>>
>>>> The same DMA unit is also used for SATA, which is an off the shelf
>>>> Designware controller with an in-kernel driver.  This interrupt timing
>>>> glitch can actually explain some intermittent errors I've observed with
>>>> it.
>>>
>>> FWIW, newer chips embed an AHCI controller, with a dedicated
>>> memory channel.
>>>
>>> FWIW2, the HW dev said memory channels are "almost free", and he
>>> would have no problem giving each device their own private channel
>>> read/write pair.
>> 
>> We still need to deal with the existing hardware.
>
> Can you confirm that your MBUS driver, in its current form,
> does not support memcpy-type transfers, which generate two
> IRQs (one from send agent, one from receive agent)?

It does not.

> Do you plan to support that, or is it just too quirky?

I hadn't planned on doing that, but I'm ruling it out entirely.

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Russell King - ARM Linux]

On Fri, Nov 25, 2016 at 02:40:21PM +0000, Måns Rullgård wrote:
> Russell King - ARM Linux <linux@armlinux.org.uk> writes:
> 
> > On Fri, Nov 25, 2016 at 02:03:20PM +0000, Måns Rullgård wrote:
> >> Russell King - ARM Linux <linux@armlinux.org.uk> writes:
> >> 
> >> > On Fri, Nov 25, 2016 at 01:50:35PM +0000, Måns Rullgård wrote:
> >> >> Russell King - ARM Linux <linux@armlinux.org.uk> writes:
> >> >> > It would be unfair to augment the API and add the burden on everyone
> >> >> > for the new API when 99.999% of the world doesn't require it.
> >> >> 
> >> >> I don't think making this particular dma driver wait for the descriptor
> >> >> callback to return before reusing a channel quite amounts to a horrid
> >> >> hack.  It certainly wouldn't burden anyone other than the poor drivers
> >> >> for devices connected to it, all of which are specific to Sigma AFAIK.
> >> >
> >> > Except when you stop to think that delaying in a tasklet is exactly
> >> > the same as randomly delaying in an interrupt handler - the tasklet
> >> > runs on the return path back to the parent context of an interrupt
> >> > handler.  Even if you sleep in the tasklet, you're sleeping on behalf
> >> > of the currently executing thread - if it's a RT thread, you effectively
> >> > destroy the RT-ness of the thread.  Let's hope no one cares about RT
> >> > performance on that hardware...
> >> 
> >> That's why I suggested to do this only if the needed delay is known to
> >> be no more than a few bus cycles.  The completion callback is currently
> >> the only post-transfer interaction we have between the dma and device
> >> drivers.  To handle an arbitrarily long delay, some new interface will
> >> be required.
> >
> > And now we're back at the point I made a few emails ago about undue
> > burden which is just about quoted above...
> 
> So what do you suggest?  Stick our heads in the sand and pretend
> everything is perfect?

Look, if you're going to be arsey, don't be surprised if I start getting
the urge to repeat previous comments.

Let's try and keep this on a technical basis for once, rather than
decending into insults.

So, wind back to my original email where I started talking about PL08x
already doing something along these lines.  Before a DMA user can make
use of a DMA channel, it has to be requested.  Once a DMA user has
finished, it can free up the channel.

What this means is that there's already a solution here - but it depends
how many DMA channels and how many active DMA users there are.  It's
entirely possible to set the mapping up when a DMA user requests a
DMA channel, leave it setup, and only tear it down when the channel
is eventually freed.

At that point, there's no need to spin-wait or sleep to delay the
tear-down of the channel - and I'd suggest that approach _until_
such time that there are more users than there are DMA channels.  This
has minimal overhead, it doesn't screw up RT threads (which include
IRQ threads), and it doesn't spread the maintanence burden across
drivers with a new custom API just for one SoC.

If (or when) the number of active users exceeds the number of hardware
DMA channels, then there's a decision to be made:

1) either limit the number of peripherals that we support DMA on for
   the SoC.
2) add the delay or API as necessary and switch to dynamic channel
   allocation to incoming requests.

Until that point is reached, there's no point inventing new APIs for
something that isn't actually a problem yet.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: Tearing down DMA transfer setup after DMA client has finished

[Mason]

On 25/11/2016 15:17, Russell King - ARM Linux wrote:
> On Fri, Nov 25, 2016 at 02:03:20PM +0000, Måns Rullgård wrote:
>> Russell King - ARM Linux <linux@armlinux.org.uk> writes:
>>
>>> On Fri, Nov 25, 2016 at 01:50:35PM +0000, Måns Rullgård wrote:
>>>> Russell King - ARM Linux <linux@armlinux.org.uk> writes:
>>>>> It would be unfair to augment the API and add the burden on everyone
>>>>> for the new API when 99.999% of the world doesn't require it.
>>>>
>>>> I don't think making this particular dma driver wait for the descriptor
>>>> callback to return before reusing a channel quite amounts to a horrid
>>>> hack.  It certainly wouldn't burden anyone other than the poor drivers
>>>> for devices connected to it, all of which are specific to Sigma AFAIK.
>>>
>>> Except when you stop to think that delaying in a tasklet is exactly
>>> the same as randomly delaying in an interrupt handler - the tasklet
>>> runs on the return path back to the parent context of an interrupt
>>> handler.  Even if you sleep in the tasklet, you're sleeping on behalf
>>> of the currently executing thread - if it's a RT thread, you effectively
>>> destroy the RT-ness of the thread.  Let's hope no one cares about RT
>>> performance on that hardware...
>>
>> That's why I suggested to do this only if the needed delay is known to
>> be no more than a few bus cycles.  The completion callback is currently
>> the only post-transfer interaction we have between the dma and device
>> drivers.  To handle an arbitrarily long delay, some new interface will
>> be required.
> 
> And now we're back at the point I made a few emails ago about undue
> burden which is just about quoted above...

I've had several talks with the HW dev, and I don't think they
anticipated the need to mux the 3 channels. In their minds,
customers would choose at most 3 devices to support, and
assign one channel to each device statically.

In fact, in tango4, supported devices are:
A) NAND Flash controllers 0 and 1
NB: the upstream driver only uses controller 0
B) IDE or SATA controllers 0 and 1
C) a few crypto HW blocks which do not work as expected (unused)

Customers typically use 1 channel for NAND, maybe 1 for SATA,
and 1 channel remains unused.

I understand the desire to solve the general case in the
driver, but actual use-cases are much more trivial.

Regards.

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Russell King - ARM Linux <linux@armlinux.org.uk> writes:

> On Fri, Nov 25, 2016 at 02:40:21PM +0000, Måns Rullgård wrote:
>> Russell King - ARM Linux <linux@armlinux.org.uk> writes:
>> 
>> > On Fri, Nov 25, 2016 at 02:03:20PM +0000, Måns Rullgård wrote:
>> >> Russell King - ARM Linux <linux@armlinux.org.uk> writes:
>> >> 
>> >> > On Fri, Nov 25, 2016 at 01:50:35PM +0000, Måns Rullgård wrote:
>> >> >> Russell King - ARM Linux <linux@armlinux.org.uk> writes:
>> >> >> > It would be unfair to augment the API and add the burden on everyone
>> >> >> > for the new API when 99.999% of the world doesn't require it.
>> >> >> 
>> >> >> I don't think making this particular dma driver wait for the descriptor
>> >> >> callback to return before reusing a channel quite amounts to a horrid
>> >> >> hack.  It certainly wouldn't burden anyone other than the poor drivers
>> >> >> for devices connected to it, all of which are specific to Sigma AFAIK.
>> >> >
>> >> > Except when you stop to think that delaying in a tasklet is exactly
>> >> > the same as randomly delaying in an interrupt handler - the tasklet
>> >> > runs on the return path back to the parent context of an interrupt
>> >> > handler.  Even if you sleep in the tasklet, you're sleeping on behalf
>> >> > of the currently executing thread - if it's a RT thread, you effectively
>> >> > destroy the RT-ness of the thread.  Let's hope no one cares about RT
>> >> > performance on that hardware...
>> >> 
>> >> That's why I suggested to do this only if the needed delay is known to
>> >> be no more than a few bus cycles.  The completion callback is currently
>> >> the only post-transfer interaction we have between the dma and device
>> >> drivers.  To handle an arbitrarily long delay, some new interface will
>> >> be required.
>> >
>> > And now we're back at the point I made a few emails ago about undue
>> > burden which is just about quoted above...
>> 
>> So what do you suggest?  Stick our heads in the sand and pretend
>> everything is perfect?
>
> Look, if you're going to be arsey, don't be surprised if I start getting
> the urge to repeat previous comments.
>
> Let's try and keep this on a technical basis for once, rather than
> decending into insults.

You're the one who constantly insults people.  I'd be happy for you to
stop.

> So, wind back to my original email where I started talking about PL08x
> already doing something along these lines.  Before a DMA user can make
> use of a DMA channel, it has to be requested.  Once a DMA user has
> finished, it can free up the channel.
>
> What this means is that there's already a solution here - but it depends
> how many DMA channels and how many active DMA users there are.  It's
> entirely possible to set the mapping up when a DMA user requests a
> DMA channel, leave it setup, and only tear it down when the channel
> is eventually freed.
>
> At that point, there's no need to spin-wait or sleep to delay the
> tear-down of the channel - and I'd suggest that approach _until_
> such time that there are more users than there are DMA channels.  This
> has minimal overhead, it doesn't screw up RT threads (which include
> IRQ threads), and it doesn't spread the maintanence burden across
> drivers with a new custom API just for one SoC.

I never suggested a custom API for one SoC.

> If (or when) the number of active users exceeds the number of hardware
> DMA channels, then there's a decision to be made:
>
> 1) either limit the number of peripherals that we support DMA on for
>    the SoC.

I don't think people would like being forced to choose between, say,
SATA and NAND flash.

> 2) add the delay or API as necessary and switch to dynamic channel
>    allocation to incoming requests.

A fixed delay doesn't seem right.  Since we don't know the exact amount
required, we'll need to make a guess and make it conservative enough
that it never ends up being too short.  This will most likely end up
delaying things far more than is actually necessary.

The reality of the situation is that the current dmaengine api doesn't
adequately cover all real hardware situations.  You seem to be of the
opinion that fixing this is an "undue burden."

> Until that point is reached, there's no point inventing new APIs for
> something that isn't actually a problem yet.

We're already at that point.  The hardware has many more devices than
physical channels.

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Mason <slash.tmp@free.fr> writes:

> On 25/11/2016 15:17, Russell King - ARM Linux wrote:
>> On Fri, Nov 25, 2016 at 02:03:20PM +0000, Måns Rullgård wrote:
>>> Russell King - ARM Linux <linux@armlinux.org.uk> writes:
>>>
>>>> On Fri, Nov 25, 2016 at 01:50:35PM +0000, Måns Rullgård wrote:
>>>>> Russell King - ARM Linux <linux@armlinux.org.uk> writes:
>>>>>> It would be unfair to augment the API and add the burden on everyone
>>>>>> for the new API when 99.999% of the world doesn't require it.
>>>>>
>>>>> I don't think making this particular dma driver wait for the descriptor
>>>>> callback to return before reusing a channel quite amounts to a horrid
>>>>> hack.  It certainly wouldn't burden anyone other than the poor drivers
>>>>> for devices connected to it, all of which are specific to Sigma AFAIK.
>>>>
>>>> Except when you stop to think that delaying in a tasklet is exactly
>>>> the same as randomly delaying in an interrupt handler - the tasklet
>>>> runs on the return path back to the parent context of an interrupt
>>>> handler.  Even if you sleep in the tasklet, you're sleeping on behalf
>>>> of the currently executing thread - if it's a RT thread, you effectively
>>>> destroy the RT-ness of the thread.  Let's hope no one cares about RT
>>>> performance on that hardware...
>>>
>>> That's why I suggested to do this only if the needed delay is known to
>>> be no more than a few bus cycles.  The completion callback is currently
>>> the only post-transfer interaction we have between the dma and device
>>> drivers.  To handle an arbitrarily long delay, some new interface will
>>> be required.
>> 
>> And now we're back at the point I made a few emails ago about undue
>> burden which is just about quoted above...
>
> I've had several talks with the HW dev, and I don't think they
> anticipated the need to mux the 3 channels. In their minds,
> customers would choose at most 3 devices to support, and
> assign one channel to each device statically.
>
> In fact, in tango4, supported devices are:
> A) NAND Flash controllers 0 and 1
> NB: the upstream driver only uses controller 0
> B) IDE or SATA controllers 0 and 1
> C) a few crypto HW blocks which do not work as expected (unused)
>
> Customers typically use 1 channel for NAND, maybe 1 for SATA,
> and 1 channel remains unused.

The hardware has two sata controllers, and I have a board that uses both.

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Mason]

On 25/11/2016 16:12, Måns Rullgård wrote:

> Mason writes:
> 
>> I've had several talks with the HW dev, and I don't think they
>> anticipated the need to mux the 3 channels. In their minds,
>> customers would choose at most 3 devices to support, and
>> assign one channel to each device statically.
>>
>> In fact, in tango4, supported devices are:
>> A) NAND Flash controllers 0 and 1
>> NB: the upstream driver only uses controller 0
>> B) IDE or SATA controllers 0 and 1
>> C) a few crypto HW blocks which do not work as expected (unused)
>>
>> Customers typically use 1 channel for NAND, maybe 1 for SATA,
>> and 1 channel remains unused.
> 
> The hardware has two sata controllers, and I have a board that uses both.

I don't have the tango3 client devices in mind, but
1 NAND + 2 SATA works out alright for 3 channels, right?

Regards.

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Mason <slash.tmp@free.fr> writes:

> On 25/11/2016 16:12, Måns Rullgård wrote:
>
>> Mason writes:
>> 
>>> I've had several talks with the HW dev, and I don't think they
>>> anticipated the need to mux the 3 channels. In their minds,
>>> customers would choose at most 3 devices to support, and
>>> assign one channel to each device statically.
>>>
>>> In fact, in tango4, supported devices are:
>>> A) NAND Flash controllers 0 and 1
>>> NB: the upstream driver only uses controller 0
>>> B) IDE or SATA controllers 0 and 1
>>> C) a few crypto HW blocks which do not work as expected (unused)
>>>
>>> Customers typically use 1 channel for NAND, maybe 1 for SATA,
>>> and 1 channel remains unused.
>> 
>> The hardware has two sata controllers, and I have a board that uses both.
>
> I don't have the tango3 client devices in mind, but
> 1 NAND + 2 SATA works out alright for 3 channels, right?

There are only two usable channels.

Besides, your 3.4 kernel allocates the channels dynamically, sort of,
but since it has a completely custom api, this particular timing issue
doesn't arise there.

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Mason]

On 25/11/2016 15:37, Måns Rullgård wrote:

> Mason writes:
> 
>> On 25/11/2016 14:11, Måns Rullgård wrote:
>>
>>> Mason writes:
>>>
>>>> It seems there is a disconnect between what Linux expects - an IRQ
>>>> when the transfer is complete - and the quirks of this HW :-(
>>>>
>>>> On this system, there are MBUS "agents" connected via a "switch box".
>>>> An agent fires an IRQ when it has dealt with its *half* of the transfer.
>>>>
>>>> SOURCE_AGENT <---> SBOX <---> DESTINATION_AGENT
>>>>
>>>> Here are the steps for a transfer, in the general case:
>>>>
>>>> 1) setup the sbox to connect SOURCE TO DEST
>>>> 2) configure source to send N bytes
>>>> 3) configure dest to receive N bytes
>>>>
>>>> When SOURCE_AGENT has sent N bytes, it fires an IRQ
>>>> When DEST_AGENT has received N bytes, it fires an IRQ
>>>> The sbox connection can be torn down only when the destination
>>>> agent has received all bytes.
>>>> (And the twist is that some agents do not have an IRQ line.)
>>>>
>>>> The system provides 3 RAM-to-sbox agents (read channels)
>>>> and 3 sbox-to-RAM agents (write channels).
>>>>
>>>> The NAND Flash controller read and write agents do not have
>>>> IRQ lines.
>>>>
>>>> So for a NAND-to-memory transfer (read from device)
>>>> - nothing happens when the NFC has finished sending N bytes to the sbox
>>>> - the write channel fires an IRQ when it has received N bytes
>>>>
>>>> In that case, one IRQ fires when the transfer is complete,
>>>> like Linux expects.
>>>>
>>>> For a memory-to-NAND transfer (write to device)
>>>> - the read channel fires an IRQ when it has sent N bytes
>>>> - the NFC driver is supposed to poll the NFC to determine
>>>> when the controller has finished writing N bytes
>>>>
>>>> In that case, the IRQ does not indicate that the transfer
>>>> is complete, merely that the sending half has finished
>>>> its part.
>>>
>>> When does your NAND controller signal completion?  When it has received
>>> the DMA data, or only when it has finished the actual write operation?
>>
>> The NAND controller provides a STATUS register.
>> Bit 31 is the CMD_READY bit.
>> This bit goes to 0 when the controller is busy, and to 1
>> when the controller is ready to accept the next command.
>>
>> The NFC driver is doing:
>>
>> 	res = wait_for_completion_timeout(&tx_done, HZ);
>> 	if (res > 0)
>> 		err = readl_poll_timeout(addr, val, val & CMD_READY, 0, 1000);
>>
>> So basically, sleep until the memory agent IRQ falls,
>> then spin until the controller is idle.
> 
> This doesn't answer my question.  Waiting for the entire operation to
> finish isn't necessary.  The dma driver only needs to wait until all the
> data has been received by the nand controller, not until the controller
> is completely finished with the command.  Does the nand controller
> provide an indication for completion of the dma independently of the
> progress of the write command?  The dma glue Sigma added to the
> Designware sata controller does this.

I called the HW dev. He told me the NFC block does not have
buffers to store the incoming data; so they remain in the
MBUS FIFOs until the NFC consumes them, i.e. when it has
finished writing them to a NAND chip, which could take
a "long time" when writing to a slow chip.

So the answer to your question is: "the NAND controller
signals completion only when it has finished the actual
write operation."

>> Did you see that adding a 10 µs delay at the start of
>> tangox_dma_pchan_detach() makes the system no longer
>> fail (passes an mtd_speedtest).
> 
> Yes, but maybe that's much longer than is actually necessary.

I could instrument my spin loop to record how long we had
to wait between the IRQ and CMD_READY.

Regards.

Re: Tearing down DMA transfer setup after DMA client has finished

[Mason]

[ Nothing new added below.
  Vinod, was the description of my HW's quirks clear enough?
  Is there a way to write a driver within the existing framework?
  How can I get that HW block supported upstream?
  Regards. ]

On 25/11/2016 13:46, Mason wrote:

> On 25/11/2016 05:55, Vinod Koul wrote:
> 
>> On Wed, Nov 23, 2016 at 11:25:44AM +0100, Mason wrote:
>>
>>> On my platform, setting up a DMA transfer is a two-step process:
>>>
>>> 1) configure the "switch box" to connect a device to a memory channel
>>> 2) configure the transfer details (address, size, command)
>>>
>>> When the transfer is done, the sbox setup can be torn down,
>>> and the DMA driver can start another transfer.
>>>
>>> The current software architecture for my NFC (NAND Flash controller)
>>> driver is as follows (for one DMA transfer).
>>>
>>>   sg_init_one
>>>   dma_map_sg
>>>   dmaengine_prep_slave_sg
>>>   dmaengine_submit
>>>   dma_async_issue_pending
>>>   configure_NFC_transfer
>>>   wait_for_IRQ_from_DMA_engine // via DMA_PREP_INTERRUPT
>>>   wait_for_NFC_idle
>>>   dma_unmap_sg
>>
>> Looking at thread and discussion now, first thinking would be to ensure
>> the transaction is completed properly and then isr fired. You may need
>> to talk to your HW designers to find a way for that. It is quite common
>> that DMA controllers will fire and complete whereas the transaction is
>> still in flight.
> 
> It seems there is a disconnect between what Linux expects - an IRQ
> when the transfer is complete - and the quirks of this HW :-(
> 
> On this system, there are MBUS "agents" connected via a "switch box".
> An agent fires an IRQ when it has dealt with its *half* of the transfer.
> 
> SOURCE_AGENT <---> SBOX <---> DESTINATION_AGENT
> 
> Here are the steps for a transfer, in the general case:
> 
> 1) setup the sbox to connect SOURCE TO DEST
> 2) configure source to send N bytes
> 3) configure dest to receive N bytes
> 
> When SOURCE_AGENT has sent N bytes, it fires an IRQ
> When DEST_AGENT has received N bytes, it fires an IRQ
> The sbox connection can be torn down only when the destination
> agent has received all bytes.
> (And the twist is that some agents do not have an IRQ line.)
> 
> The system provides 3 RAM-to-sbox agents (read channels)
> and 3 sbox-to-RAM agents (write channels).
> 
> The NAND Flash controller read and write agents do not have
> IRQ lines.
> 
> So for a NAND-to-memory transfer (read from device)
> - nothing happens when the NFC has finished sending N bytes to the sbox
> - the write channel fires an IRQ when it has received N bytes
> 
> In that case, one IRQ fires when the transfer is complete,
> like Linux expects.
> 
> For a memory-to-NAND transfer (write to device)
> - the read channel fires an IRQ when it has sent N bytes
> - the NFC driver is supposed to poll the NFC to determine
> when the controller has finished writing N bytes
> 
> In that case, the IRQ does not indicate that the transfer
> is complete, merely that the sending half has finished
> its part.
> 
> For a memory-to-memory transfer (memcpy)
> - the read channel fires an IRQ when it has sent N bytes
> - the write channel fires an IRQ when it has received N bytes
> 
> So you actually get two IRQs in that case, which I don't
> think Linux (or the current DMA driver) expects.
> 
> I'm not sure how we're supposed to handle this kind of HW
> in Linux? (That's why I started this thread.)
> 
> 
>> If that is not doable, then since you claim this is custom part which
>> other vendors won't use (hope we are wrong down the line),
> 
> I'm not sure how to interpret "you claim this is custom part".
> Do you mean I may be wrong, that it is not custom?
> I don't know if other vendors may have HW with the same
> quirky behavior. What do you mean about being wrong down
> the line?
> 
>> then we can have a custom api,
>>
>> foo_sbox_configure(bool enable, ...);
>>
>> This can be invoked from NFC driver when required for configuration and
>> teardown. For very specific cases where people need some specific
>> configuration we do allow custom APIs.
> 
> I don't think that would work. The fundamental issue is
> that Linux expects a single IRQ to indicate "transfer
> complete". And the driver (as written) starts a new
> transfer as soon as the IRQ fires.
> 
> But the HW may generate 0, 1, or even 2 IRQs for a single
> transfer. And when there is a single IRQ, it may not
> indicate "transfer complete" (as seen above).
> 
>> Only problem with that would be it wont be a generic solution
>> and you seem to be fine with that.
> 
> I think it is possible to have a generic solution:
> Right now, the callback is called from tasklet context.
> If we can have a new flag to have the callback invoked
> directly from the ISR, then the driver for the client
> device can do what is required.
> 
> For example, the NFC driver waits for the IRQ from the
> memory agent, and then polls the controller itself.
> 
> I can whip up a proof-of-concept if it's better to
> illustrate with a patch?

Re: Tearing down DMA transfer setup after DMA client has finished

[Vinod Koul]

On Tue, Nov 29, 2016 at 07:25:02PM +0100, Mason wrote:

Sorry I was away for a week in meeting with laptop down.

> [ Nothing new added below.
>   Vinod, was the description of my HW's quirks clear enough?

Yes

>   Is there a way to write a driver within the existing framework?

I think so, looking back at comments from Russell, I do tend to agree with
that. Is there a specfic reason why sbox can't be tied to alloc and free
channels?

>   How can I get that HW block supported upstream?
>   Regards. ]
> 
> On 25/11/2016 13:46, Mason wrote:
> 
> > On 25/11/2016 05:55, Vinod Koul wrote:
> > 
> >> On Wed, Nov 23, 2016 at 11:25:44AM +0100, Mason wrote:
> >>
> >>> On my platform, setting up a DMA transfer is a two-step process:
> >>>
> >>> 1) configure the "switch box" to connect a device to a memory channel
> >>> 2) configure the transfer details (address, size, command)
> >>>
> >>> When the transfer is done, the sbox setup can be torn down,
> >>> and the DMA driver can start another transfer.
> >>>
> >>> The current software architecture for my NFC (NAND Flash controller)
> >>> driver is as follows (for one DMA transfer).
> >>>
> >>>   sg_init_one
> >>>   dma_map_sg
> >>>   dmaengine_prep_slave_sg
> >>>   dmaengine_submit
> >>>   dma_async_issue_pending
> >>>   configure_NFC_transfer
> >>>   wait_for_IRQ_from_DMA_engine // via DMA_PREP_INTERRUPT
> >>>   wait_for_NFC_idle
> >>>   dma_unmap_sg
> >>
> >> Looking at thread and discussion now, first thinking would be to ensure
> >> the transaction is completed properly and then isr fired. You may need
> >> to talk to your HW designers to find a way for that. It is quite common
> >> that DMA controllers will fire and complete whereas the transaction is
> >> still in flight.
> > 
> > It seems there is a disconnect between what Linux expects - an IRQ
> > when the transfer is complete - and the quirks of this HW :-(
> > 
> > On this system, there are MBUS "agents" connected via a "switch box".
> > An agent fires an IRQ when it has dealt with its *half* of the transfer.
> > 
> > SOURCE_AGENT <---> SBOX <---> DESTINATION_AGENT
> > 
> > Here are the steps for a transfer, in the general case:
> > 
> > 1) setup the sbox to connect SOURCE TO DEST
> > 2) configure source to send N bytes
> > 3) configure dest to receive N bytes
> > 
> > When SOURCE_AGENT has sent N bytes, it fires an IRQ
> > When DEST_AGENT has received N bytes, it fires an IRQ
> > The sbox connection can be torn down only when the destination
> > agent has received all bytes.
> > (And the twist is that some agents do not have an IRQ line.)
> > 
> > The system provides 3 RAM-to-sbox agents (read channels)
> > and 3 sbox-to-RAM agents (write channels).
> > 
> > The NAND Flash controller read and write agents do not have
> > IRQ lines.
> > 
> > So for a NAND-to-memory transfer (read from device)
> > - nothing happens when the NFC has finished sending N bytes to the sbox
> > - the write channel fires an IRQ when it has received N bytes
> > 
> > In that case, one IRQ fires when the transfer is complete,
> > like Linux expects.
> > 
> > For a memory-to-NAND transfer (write to device)
> > - the read channel fires an IRQ when it has sent N bytes
> > - the NFC driver is supposed to poll the NFC to determine
> > when the controller has finished writing N bytes
> > 
> > In that case, the IRQ does not indicate that the transfer
> > is complete, merely that the sending half has finished
> > its part.
> > 
> > For a memory-to-memory transfer (memcpy)
> > - the read channel fires an IRQ when it has sent N bytes
> > - the write channel fires an IRQ when it has received N bytes
> > 
> > So you actually get two IRQs in that case, which I don't
> > think Linux (or the current DMA driver) expects.
> > 
> > I'm not sure how we're supposed to handle this kind of HW
> > in Linux? (That's why I started this thread.)
> > 
> > 
> >> If that is not doable, then since you claim this is custom part which
> >> other vendors won't use (hope we are wrong down the line),
> > 
> > I'm not sure how to interpret "you claim this is custom part".
> > Do you mean I may be wrong, that it is not custom?
> > I don't know if other vendors may have HW with the same
> > quirky behavior. What do you mean about being wrong down
> > the line?
> > 
> >> then we can have a custom api,
> >>
> >> foo_sbox_configure(bool enable, ...);
> >>
> >> This can be invoked from NFC driver when required for configuration and
> >> teardown. For very specific cases where people need some specific
> >> configuration we do allow custom APIs.
> > 
> > I don't think that would work. The fundamental issue is
> > that Linux expects a single IRQ to indicate "transfer
> > complete". And the driver (as written) starts a new
> > transfer as soon as the IRQ fires.
> > 
> > But the HW may generate 0, 1, or even 2 IRQs for a single
> > transfer. And when there is a single IRQ, it may not
> > indicate "transfer complete" (as seen above).
> > 
> >> Only problem with that would be it wont be a generic solution
> >> and you seem to be fine with that.
> > 
> > I think it is possible to have a generic solution:
> > Right now, the callback is called from tasklet context.
> > If we can have a new flag to have the callback invoked
> > directly from the ISR, then the driver for the client
> > device can do what is required.
> > 
> > For example, the NFC driver waits for the IRQ from the
> > memory agent, and then polls the controller itself.
> > 
> > I can whip up a proof-of-concept if it's better to
> > illustrate with a patch?
> 

-- 
~Vinod

Re: Tearing down DMA transfer setup after DMA client has finished

[Mason]

On 06/12/2016 06:12, Vinod Koul wrote:

> On Tue, Nov 29, 2016 at 07:25:02PM +0100, Mason wrote:
> 
>> Is there a way to write a driver within the existing framework?
> 
> I think so, looking back at comments from Russell, I do tend to agree with
> that. Is there a specific reason why sbox can't be tied to alloc and free
> channels?

Here's a recap of the situation.

The "SBOX+MBUS" HW is used in several iterations of the tango SoC:

tango3
  2 memory channels available
  6 devices ("clients"?) may request an MBUS channel

tango4 (one more channel)
  3 memory channels available
  7 devices may request an MBUS channel :
    NFC0, NFC1, SATA0, SATA1, memcpy, (IDE0, IDE1)

Notes:
The current NFC driver supports only one controller.
IDE is mostly obsolete at this point.

tango5 (SATA gets own dedicated MBUS channel pair)
  3 memory channels available
  5 devices may request an MBUS channel :
    NFC0, NFC1, memcpy, (IDE0, IDE1)


If I understand the current DMA driver (written by Mans), client
drivers are instructed to use a specific channel in the DT, and
the DMA driver muxes access to that channel. The DMA driver
manages a per-channel queue of outstanding DMA transfer requests,
and a new transfer is started friom within the DMA ISR
(modulo the fact that the interrupt does not signal completion
of the transfer, as explained else-thread).

What you're proposing, Vinod, is to make a channel exclusive
to a driver, as long as the driver has not explicitly released
the channel, via dma_release_channel(), right?

Regards.

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Mason <slash.tmp@free.fr> writes:

> On 06/12/2016 06:12, Vinod Koul wrote:
>
>> On Tue, Nov 29, 2016 at 07:25:02PM +0100, Mason wrote:
>> 
>>> Is there a way to write a driver within the existing framework?
>> 
>> I think so, looking back at comments from Russell, I do tend to agree with
>> that. Is there a specific reason why sbox can't be tied to alloc and free
>> channels?
>
> Here's a recap of the situation.
>
> The "SBOX+MBUS" HW is used in several iterations of the tango SoC:
>
> tango3
>   2 memory channels available
>   6 devices ("clients"?) may request an MBUS channel
>
> tango4 (one more channel)
>   3 memory channels available
>   7 devices may request an MBUS channel :
>     NFC0, NFC1, SATA0, SATA1, memcpy, (IDE0, IDE1)
>
> Notes:
> The current NFC driver supports only one controller.

I consider that a bug.

> IDE is mostly obsolete at this point.
>
> tango5 (SATA gets own dedicated MBUS channel pair)
>   3 memory channels available
>   5 devices may request an MBUS channel :
>     NFC0, NFC1, memcpy, (IDE0, IDE1)

Some of the chip variants can also use this DMA engine for PCI devices.

> If I understand the current DMA driver (written by Mans), client
> drivers are instructed to use a specific channel in the DT, and
> the DMA driver muxes access to that channel.

Almost.  The DT indicates the sbox ID of each device.  The driver
multiplexes requests from all devices across all channels.

> The DMA driver manages a per-channel queue of outstanding DMA transfer
> requests, and a new transfer is started friom within the DMA ISR
> (modulo the fact that the interrupt does not signal completion of the
> transfer, as explained else-thread).

We need to somehow let the device driver signal the dma driver when a
transfer has been fully completed.  Currently the only post-transfer
interaction between the dma engine and the device driver is through the
descriptor callback, which is not suitable for this purpose.

This is starting to look like one of those situations where someone just
needs to implement a solution, or we'll be forever bickering about
hypotheticals.

> What you're proposing, Vinod, is to make a channel exclusive
> to a driver, as long as the driver has not explicitly released
> the channel, via dma_release_channel(), right?

That's not going to work very well.  Device drivers typically request
dma channels in their probe functions or when the device is opened.
This means that reserving one of the few channels there will inevitably
make some other device fail to operate.

Doing a request/release per transfer really doesn't fit with the
intended usage of the dmaengine api.  For starters, what should a driver
do if all the channels are currently busy?

Since the hardware actually does support multiplexing the dma channels,
I think it would be misguided to deliberately cripple the software
support in order to shoehorn it into an incomplete model of how hardware
ought to work.  While I agree it would be nicer if all hardware actually
did work that way, this isn't the reality we're living in.

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Mason]

On 06/12/2016 14:14, Måns Rullgård wrote:

> Mason wrote:
> 
>> On 06/12/2016 06:12, Vinod Koul wrote:
>>
>>> On Tue, Nov 29, 2016 at 07:25:02PM +0100, Mason wrote:
>>>
>>>> Is there a way to write a driver within the existing framework?
>>>
>>> I think so, looking back at comments from Russell, I do tend to agree with
>>> that. Is there a specific reason why sbox can't be tied to alloc and free
>>> channels?
>>
>> Here's a recap of the situation.
>>
>> The "SBOX+MBUS" HW is used in several iterations of the tango SoC:
>>
>> tango3
>>   2 memory channels available
>>   6 devices ("clients"?) may request an MBUS channel
>>
>> tango4 (one more channel)
>>   3 memory channels available
>>   7 devices may request an MBUS channel :
>>     NFC0, NFC1, SATA0, SATA1, memcpy, (IDE0, IDE1)
>>
>> Notes:
>> The current NFC driver supports only one controller.
> 
> I consider that a bug.

Meh. The two controller blocks share the I/O pins to the outside
world, so it's not possible to have two concurrent accesses.
Moreover, the current NAND framework does not currently support
such a setup. (I discussed this with the maintainer.)


>> IDE is mostly obsolete at this point.
>>
>> tango5 (SATA gets own dedicated MBUS channel pair)
>>   3 memory channels available
>>   5 devices may request an MBUS channel :
>>     NFC0, NFC1, memcpy, (IDE0, IDE1)
> 
> Some of the chip variants can also use this DMA engine for PCI devices.

Note: PCI support was dropped with tango4.


>> If I understand the current DMA driver (written by Mans), client
>> drivers are instructed to use a specific channel in the DT, and
>> the DMA driver muxes access to that channel.
> 
> Almost.  The DT indicates the sbox ID of each device.  The driver
> multiplexes requests from all devices across all channels.

Thanks for pointing that out. I misremembered the DT.
So a client's DT node specifies the client's SBOX port.
And the DMA node specifies all available MBUS channels.

So when an interrupt fires, the DMA driver (re)uses that
channel for the next transfer in line?


>> The DMA driver manages a per-channel queue of outstanding DMA transfer
>> requests, and a new transfer is started from within the DMA ISR
>> (modulo the fact that the interrupt does not signal completion of the
>> transfer, as explained else-thread).
> 
> We need to somehow let the device driver signal the dma driver when a
> transfer has been fully completed.  Currently the only post-transfer
> interaction between the dma engine and the device driver is through the
> descriptor callback, which is not suitable for this purpose.

The callback is called from vchan_complete() right?
Is that running from interrupt context?

What's the relationship between vchan_complete() and
tangox_dma_irq() -- does one call the other? Are they
asynchronous?


> This is starting to look like one of those situations where someone just
> needs to implement a solution, or we'll be forever bickering about
> hypotheticals.

I can give that a shot (if you're busy with real work).


>> What you're proposing, Vinod, is to make a channel exclusive
>> to a driver, as long as the driver has not explicitly released
>> the channel, via dma_release_channel(), right?
> 
> That's not going to work very well.  Device drivers typically request
> dma channels in their probe functions or when the device is opened.
> This means that reserving one of the few channels there will inevitably
> make some other device fail to operate.

This is true for tango3. Less so for tango4. And no longer
an issue for tango5.


> Doing a request/release per transfer really doesn't fit with the
> intended usage of the dmaengine api.  For starters, what should a driver
> do if all the channels are currently busy?

Why can't we queue channel requests the same way we queue
transfer requests?


> Since the hardware actually does support multiplexing the dma channels,
> I think it would be misguided to deliberately cripple the software
> support in order to shoehorn it into an incomplete model of how hardware
> ought to work.  While I agree it would be nicer if all hardware actually
> did work that way, this isn't the reality we're living in.

I agree with you that it would be nice to have a general solution,
since the HW supports it.

Regards.

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Mason <slash.tmp@free.fr> writes:

> On 06/12/2016 14:14, Måns Rullgård wrote:
>
>> Mason wrote:
>> 
>>> On 06/12/2016 06:12, Vinod Koul wrote:
>>>
>>>> On Tue, Nov 29, 2016 at 07:25:02PM +0100, Mason wrote:
>>>>
>>>>> Is there a way to write a driver within the existing framework?
>>>>
>>>> I think so, looking back at comments from Russell, I do tend to agree with
>>>> that. Is there a specific reason why sbox can't be tied to alloc and free
>>>> channels?
>>>
>>> Here's a recap of the situation.
>>>
>>> The "SBOX+MBUS" HW is used in several iterations of the tango SoC:
>>>
>>> tango3
>>>   2 memory channels available
>>>   6 devices ("clients"?) may request an MBUS channel
>>>
>>> tango4 (one more channel)
>>>   3 memory channels available
>>>   7 devices may request an MBUS channel :
>>>     NFC0, NFC1, SATA0, SATA1, memcpy, (IDE0, IDE1)
>>>
>>> Notes:
>>> The current NFC driver supports only one controller.
>> 
>> I consider that a bug.
>
> Meh. The two controller blocks share the I/O pins to the outside
> world, so it's not possible to have two concurrent accesses.

OK, you failed to mention that part.  Why are there two controllers at
all if only one or the other can be used?

>>> If I understand the current DMA driver (written by Mans), client
>>> drivers are instructed to use a specific channel in the DT, and
>>> the DMA driver muxes access to that channel.
>> 
>> Almost.  The DT indicates the sbox ID of each device.  The driver
>> multiplexes requests from all devices across all channels.
>
> Thanks for pointing that out. I misremembered the DT.
> So a client's DT node specifies the client's SBOX port.
> And the DMA node specifies all available MBUS channels.
>
> So when an interrupt fires, the DMA driver (re)uses that
> channel for the next transfer in line?

Correct.

>>> The DMA driver manages a per-channel queue of outstanding DMA transfer
>>> requests, and a new transfer is started from within the DMA ISR
>>> (modulo the fact that the interrupt does not signal completion of the
>>> transfer, as explained else-thread).
>> 
>> We need to somehow let the device driver signal the dma driver when a
>> transfer has been fully completed.  Currently the only post-transfer
>> interaction between the dma engine and the device driver is through the
>> descriptor callback, which is not suitable for this purpose.
>
> The callback is called from vchan_complete() right?
> Is that running from interrupt context?

It runs from a tasklet which is almost the same thing.

> What's the relationship between vchan_complete() and
> tangox_dma_irq() -- does one call the other? Are they
> asynchronous?
>
>> This is starting to look like one of those situations where someone just
>> needs to implement a solution, or we'll be forever bickering about
>> hypotheticals.
>
> I can give that a shot (if you're busy with real work).

I have an idea I'd like to try out over the weekend.  If I don't come
back with something by next week, go for it.

>>> What you're proposing, Vinod, is to make a channel exclusive
>>> to a driver, as long as the driver has not explicitly released
>>> the channel, via dma_release_channel(), right?
>> 
>> That's not going to work very well.  Device drivers typically request
>> dma channels in their probe functions or when the device is opened.
>> This means that reserving one of the few channels there will inevitably
>> make some other device fail to operate.
>
> This is true for tango3. Less so for tango4. And no longer
> an issue for tango5.
>
>> Doing a request/release per transfer really doesn't fit with the
>> intended usage of the dmaengine api.  For starters, what should a driver
>> do if all the channels are currently busy?
>
> Why can't we queue channel requests the same way we queue
> transfer requests?

That's in effect what we're doing.  Calling it by another name doesn't
really solve anything.

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Mason]

On 06/12/2016 16:34, Måns Rullgård wrote:

> Mason writes:
> 
>> Meh. The two controller blocks share the I/O pins to the outside
>> world, so it's not possible to have two concurrent accesses.
> 
> OK, you failed to mention that part.  Why are there two controllers at
> all if only one or the other can be used?

I'd have to ask the HW designer what types of use-cases he had
in mind. Perhaps looking at what is *not* shared provides clues.
Configuration registers are duplicated, meaning it is possible
to decide "channel A for chip0, channel B for chip1" if there
are two NAND chips in the system (as is the case on dev boards).
In that case, it is unnecessary to rewrite the chip parameters
every time the driver switches chips. (I don't think the perf
impact is even measurable.)

The ECC engines are duplicated, but I don't know how long it
takes to run the BCH algorithm in HW vs performing I/O over
a slow 8-bit bus.


>> The callback is called from vchan_complete() right?
>> Is that running from interrupt context?
> 
> It runs from a tasklet which is almost the same thing.

I'll read up on tasklets tomorrow.


>> I can give that a shot (if you're busy with real work).
> 
> I have an idea I'd like to try out over the weekend.  If I don't come
> back with something by next week, go for it.

I do have my plate full this week, with XHCI and AHCI :-)


>> Why can't we queue channel requests the same way we queue
>> transfer requests?
> 
> That's in effect what we're doing.  Calling it by another name doesn't
> really solve anything.

Hmmm... the difference is that "tear down" would be explicit
in the release function.

Current implementation for single transfer:

dmaengine_prep_slave_sg()
dmaengine_submit()
dma_async_issue_pending()	/* A */
wait for read channel IRQ	/* B */
spin until NFC idle		/* C */

Setup SBOX route and program MBUS transfer happen in A.
The SBOX route is torn down a little before B (thus before C).


With the proposed implementation, where request_chan
sets up the route and release_chan tears it down:

request_chan()			/* X */
dmaengine_prep_slave_sg()
dmaengine_submit()
dma_async_issue_pending()	/* A */
wait for read channel IRQ	/* B */
spin until NFC idle		/* C */
release_chan()			/* Y */

Now, the SBOX route is setup in X.
(If no MBUS channel are available, thread is put to sleep
until one becomes available.)
Program MBUS transfer in A.
When IRQ falls, cannot start new transfer yet.
vchan_complete() will at some point run the client callback.
The client driver can now spin however long until NFC idle.
In Y, we release the channel, thus calling back into the
DMA driver, at which point a new transfer can be started.

What did I get wrong in this pseudo-code?


I do see one problem with the approach:
It's no big deal for me to convert the NFC driver to
do it that way, but the Synopsys (I think) SATA driver
in tango3 and tango4 is shared across multiple SoCs,
and they probably call request_chan() only in the
probe function, as you mentioned at some point.

http://lxr.free-electrons.com/source/drivers/ata/sata_dwc_460ex.c#L366

BTW, can someone explain what the DMA_CTRL_ACK flag means?


Regards.

Re: Tearing down DMA transfer setup after DMA client has finished

[Vinod Koul]

On Tue, Dec 06, 2016 at 01:42:31PM +0100, Mason wrote:
> On 06/12/2016 06:12, Vinod Koul wrote:
> 
> > On Tue, Nov 29, 2016 at 07:25:02PM +0100, Mason wrote:
> > 
> >> Is there a way to write a driver within the existing framework?
> > 
> > I think so, looking back at comments from Russell, I do tend to agree with
> > that. Is there a specific reason why sbox can't be tied to alloc and free
> > channels?
> 
> Here's a recap of the situation.
> 
> The "SBOX+MBUS" HW is used in several iterations of the tango SoC:

btw is SBOX setup dependent upon the peripheral connected to?

> 
> tango3
>   2 memory channels available
>   6 devices ("clients"?) may request an MBUS channel

But only 2 can get a channel at any time..

> 
> tango4 (one more channel)
>   3 memory channels available
>   7 devices may request an MBUS channel :
>     NFC0, NFC1, SATA0, SATA1, memcpy, (IDE0, IDE1)

Same here

Only thing is users shouldn't hold on to channel and freeup when not in use.

> Notes:
> The current NFC driver supports only one controller.
> IDE is mostly obsolete at this point.
> 
> tango5 (SATA gets own dedicated MBUS channel pair)
>   3 memory channels available
>   5 devices may request an MBUS channel :
>     NFC0, NFC1, memcpy, (IDE0, IDE1)
> 
> 
> If I understand the current DMA driver (written by Mans), client
> drivers are instructed to use a specific channel in the DT, and
> the DMA driver muxes access to that channel. The DMA driver
> manages a per-channel queue of outstanding DMA transfer requests,
> and a new transfer is started friom within the DMA ISR
> (modulo the fact that the interrupt does not signal completion
> of the transfer, as explained else-thread).
> 
> What you're proposing, Vinod, is to make a channel exclusive
> to a driver, as long as the driver has not explicitly released
> the channel, via dma_release_channel(), right?

Precisely, but yes the downside of that is concurrent access are limited, but
am not sure if driver implements virtual channels and allows that..

Thanks
-- 
~Vinod

Re: Tearing down DMA transfer setup after DMA client has finished

[Vinod Koul]

On Tue, Dec 06, 2016 at 01:14:20PM +0000, Måns Rullgård wrote:
> 
> That's not going to work very well.  Device drivers typically request
> dma channels in their probe functions or when the device is opened.
> This means that reserving one of the few channels there will inevitably
> make some other device fail to operate.

No that doesnt make sense at all, you should get a channel only when you
want to use it and not in probe!

-- 
~Vinod

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Vinod Koul <vinod.koul@intel.com> writes:

> On Tue, Dec 06, 2016 at 01:42:31PM +0100, Mason wrote:
>> On 06/12/2016 06:12, Vinod Koul wrote:
>> 
>> > On Tue, Nov 29, 2016 at 07:25:02PM +0100, Mason wrote:
>> > 
>> >> Is there a way to write a driver within the existing framework?
>> > 
>> > I think so, looking back at comments from Russell, I do tend to agree with
>> > that. Is there a specific reason why sbox can't be tied to alloc and free
>> > channels?
>> 
>> Here's a recap of the situation.
>> 
>> The "SBOX+MBUS" HW is used in several iterations of the tango SoC:
>
> btw is SBOX setup dependent upon the peripheral connected to?

The sbox is basically a crossbar that connects each of a number of input
ports to any of a number of output ports.  A few of the inputs and
outputs are dma channels reading or writing to memory while the rest are
peripheral devices.  To perform a mem-to-device transfer, you pick a dma
read channel, program the sbox to connect it to the chosen device, and
finally program the dma channel with address and size to transfer.

>> tango3
>>   2 memory channels available
>>   6 devices ("clients"?) may request an MBUS channel
>
> But only 2 can get a channel at any time..
>
>> tango4 (one more channel)
>>   3 memory channels available
>>   7 devices may request an MBUS channel :
>>     NFC0, NFC1, SATA0, SATA1, memcpy, (IDE0, IDE1)
>
> Same here
>
> Only thing is users shouldn't hold on to channel and freeup when not in use.
>
>> Notes:
>> The current NFC driver supports only one controller.
>> IDE is mostly obsolete at this point.
>> 
>> tango5 (SATA gets own dedicated MBUS channel pair)
>>   3 memory channels available
>>   5 devices may request an MBUS channel :
>>     NFC0, NFC1, memcpy, (IDE0, IDE1)
>> 
>> 
>> If I understand the current DMA driver (written by Mans), client
>> drivers are instructed to use a specific channel in the DT, and
>> the DMA driver muxes access to that channel. The DMA driver
>> manages a per-channel queue of outstanding DMA transfer requests,
>> and a new transfer is started friom within the DMA ISR
>> (modulo the fact that the interrupt does not signal completion
>> of the transfer, as explained else-thread).
>> 
>> What you're proposing, Vinod, is to make a channel exclusive
>> to a driver, as long as the driver has not explicitly released
>> the channel, via dma_release_channel(), right?
>
> Precisely, but yes the downside of that is concurrent access are
> limited, but am not sure if driver implements virtual channels and
> allows that..

My driver implements virtual channels.  The problem is that the physical
dma channels signal completion slightly too soon, at least with
mem-to-device transfers.  Apparently we need to keep the sbox routing
until the peripheral indicates that it has actually received all the
data.

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Vinod Koul <vinod.koul@intel.com> writes:

> On Tue, Dec 06, 2016 at 01:14:20PM +0000, Måns Rullgård wrote:
>> 
>> That's not going to work very well.  Device drivers typically request
>> dma channels in their probe functions or when the device is opened.
>> This means that reserving one of the few channels there will inevitably
>> make some other device fail to operate.
>
> No that doesnt make sense at all, you should get a channel only when you
> want to use it and not in probe!

Tell that to just about every single driver ever written.

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Vinod Koul]

On Wed, Dec 07, 2016 at 04:44:55PM +0000, Måns Rullgård wrote:
> Vinod Koul <vinod.koul@intel.com> writes:
> >> 
> >> What you're proposing, Vinod, is to make a channel exclusive
> >> to a driver, as long as the driver has not explicitly released
> >> the channel, via dma_release_channel(), right?
> >
> > Precisely, but yes the downside of that is concurrent access are
> > limited, but am not sure if driver implements virtual channels and
> > allows that..
> 
> My driver implements virtual channels.  The problem is that the physical
> dma channels signal completion slightly too soon, at least with
> mem-to-device transfers.  Apparently we need to keep the sbox routing
> until the peripheral indicates that it has actually received all the
> data.

So do we need concurrent accesses by all clients.

-- 
~Vinod

Re: Tearing down DMA transfer setup after DMA client has finished

[Vinod Koul]

On Wed, Dec 07, 2016 at 04:45:58PM +0000, Måns Rullgård wrote:
> Vinod Koul <vinod.koul@intel.com> writes:
> 
> > On Tue, Dec 06, 2016 at 01:14:20PM +0000, Måns Rullgård wrote:
> >> 
> >> That's not going to work very well.  Device drivers typically request
> >> dma channels in their probe functions or when the device is opened.
> >> This means that reserving one of the few channels there will inevitably
> >> make some other device fail to operate.
> >
> > No that doesnt make sense at all, you should get a channel only when you
> > want to use it and not in probe!
> 
> Tell that to just about every single driver ever written.

Not really, few do yes which is wrong but not _all_ do that.

-- 
~Vinod

Re: Tearing down DMA transfer setup after DMA client has finished

[Mason]

On 08/12/2016 11:39, Vinod Koul wrote:

> On Wed, Dec 07, 2016 at 04:45:58PM +0000, Måns Rullgård wrote:
>
>> Vinod Koul <vinod.koul@intel.com> writes:
>>
>>> On Tue, Dec 06, 2016 at 01:14:20PM +0000, Måns Rullgård wrote:
>>>>
>>>> That's not going to work very well.  Device drivers typically request
>>>> dma channels in their probe functions or when the device is opened.
>>>> This means that reserving one of the few channels there will inevitably
>>>> make some other device fail to operate.
>>>
>>> No that doesn't make sense at all, you should get a channel only when you
>>> want to use it and not in probe!
>>
>> Tell that to just about every single driver ever written.
> 
> Not really, few do yes which is wrong but not _all_ do that.

Vinod,

Could you explain something to me in layman's terms?

I have a NAND Flash Controller driver that depends on the
DMA driver under discussion.

Suppose I move the dma_request_chan() call from the driver's
probe function, to the actual DMA transfer function.

I would want dma_request_chan() to put the calling thread
to sleep until a channel becomes available (possibly with
a timeout value).

But Maxime told me dma_request_chan() will just return
-EBUSY if no channels are available.

Am I supposed to busy wait in my driver's DMA function
until a channel becomes available?

I don't understand how the multiplexing of few memory
channels to many clients is supposed to happen efficiently?

Regards.

Re: Tearing down DMA transfer setup after DMA client has finished

[Geert Uytterhoeven]

On Thu, Dec 8, 2016 at 11:54 AM, Mason <slash.tmp@free.fr> wrote:
> On 08/12/2016 11:39, Vinod Koul wrote:
>> On Wed, Dec 07, 2016 at 04:45:58PM +0000, Måns Rullgård wrote:
>>> Vinod Koul <vinod.koul@intel.com> writes:
>>>> On Tue, Dec 06, 2016 at 01:14:20PM +0000, Måns Rullgård wrote:
>>>>> That's not going to work very well.  Device drivers typically request
>>>>> dma channels in their probe functions or when the device is opened.
>>>>> This means that reserving one of the few channels there will inevitably
>>>>> make some other device fail to operate.
>>>>
>>>> No that doesn't make sense at all, you should get a channel only when you
>>>> want to use it and not in probe!
>>>
>>> Tell that to just about every single driver ever written.
>>
>> Not really, few do yes which is wrong but not _all_ do that.
>
> Vinod,
>
> Could you explain something to me in layman's terms?
>
> I have a NAND Flash Controller driver that depends on the
> DMA driver under discussion.
>
> Suppose I move the dma_request_chan() call from the driver's
> probe function, to the actual DMA transfer function.
>
> I would want dma_request_chan() to put the calling thread
> to sleep until a channel becomes available (possibly with
> a timeout value).
>
> But Maxime told me dma_request_chan() will just return
> -EBUSY if no channels are available.
>
> Am I supposed to busy wait in my driver's DMA function
> until a channel becomes available?

Can you fall back to PIO if requesting a channel fails?

Alternatively, dma_request_chan() could always succeed, and
dmaengine_prep_slave_sg() could fail if the channel is currently not
available due to a limitation on the number of active channels, and
the driver could fall back to PIO for that transfer.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Vinod Koul <vinod.koul@intel.com> writes:

> On Wed, Dec 07, 2016 at 04:44:55PM +0000, Måns Rullgård wrote:
>> Vinod Koul <vinod.koul@intel.com> writes:
>> >> 
>> >> What you're proposing, Vinod, is to make a channel exclusive
>> >> to a driver, as long as the driver has not explicitly released
>> >> the channel, via dma_release_channel(), right?
>> >
>> > Precisely, but yes the downside of that is concurrent access are
>> > limited, but am not sure if driver implements virtual channels and
>> > allows that..
>> 
>> My driver implements virtual channels.  The problem is that the physical
>> dma channels signal completion slightly too soon, at least with
>> mem-to-device transfers.  Apparently we need to keep the sbox routing
>> until the peripheral indicates that it has actually received all the
>> data.
>
> So do we need concurrent accesses by all clients.

Depends on what people do with the chips.

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Vinod Koul <vinod.koul@intel.com> writes:

> On Wed, Dec 07, 2016 at 04:45:58PM +0000, Måns Rullgård wrote:
>> Vinod Koul <vinod.koul@intel.com> writes:
>> 
>> > On Tue, Dec 06, 2016 at 01:14:20PM +0000, Måns Rullgård wrote:
>> >> 
>> >> That's not going to work very well.  Device drivers typically request
>> >> dma channels in their probe functions or when the device is opened.
>> >> This means that reserving one of the few channels there will inevitably
>> >> make some other device fail to operate.
>> >
>> > No that doesnt make sense at all, you should get a channel only when you
>> > want to use it and not in probe!
>> 
>> Tell that to just about every single driver ever written.
>
> Not really, few do yes which is wrong but not _all_ do that.

Every driver I ever looked at does.  Name one you consider "correct."

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> On Thu, Dec 8, 2016 at 11:54 AM, Mason <slash.tmp@free.fr> wrote:
>> On 08/12/2016 11:39, Vinod Koul wrote:
>>> On Wed, Dec 07, 2016 at 04:45:58PM +0000, Måns Rullgård wrote:
>>>> Vinod Koul <vinod.koul@intel.com> writes:
>>>>> On Tue, Dec 06, 2016 at 01:14:20PM +0000, Måns Rullgård wrote:
>>>>>> That's not going to work very well.  Device drivers typically request
>>>>>> dma channels in their probe functions or when the device is opened.
>>>>>> This means that reserving one of the few channels there will inevitably
>>>>>> make some other device fail to operate.
>>>>>
>>>>> No that doesn't make sense at all, you should get a channel only when you
>>>>> want to use it and not in probe!
>>>>
>>>> Tell that to just about every single driver ever written.
>>>
>>> Not really, few do yes which is wrong but not _all_ do that.
>>
>> Vinod,
>>
>> Could you explain something to me in layman's terms?
>>
>> I have a NAND Flash Controller driver that depends on the
>> DMA driver under discussion.
>>
>> Suppose I move the dma_request_chan() call from the driver's
>> probe function, to the actual DMA transfer function.
>>
>> I would want dma_request_chan() to put the calling thread
>> to sleep until a channel becomes available (possibly with
>> a timeout value).
>>
>> But Maxime told me dma_request_chan() will just return
>> -EBUSY if no channels are available.
>>
>> Am I supposed to busy wait in my driver's DMA function
>> until a channel becomes available?
>
> Can you fall back to PIO if requesting a channel fails?
>
> Alternatively, dma_request_chan() could always succeed, and
> dmaengine_prep_slave_sg() could fail if the channel is currently not
> available due to a limitation on the number of active channels, and
> the driver could fall back to PIO for that transfer.

Why are we debating this nonsense?  There is an easy fix that doesn't
require changing the semantics of existing functions or falling back to
slow pio.

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Geert Uytterhoeven]

On Thu, Dec 8, 2016 at 12:44 PM, Måns Rullgård <mans@mansr.com> wrote:
> Vinod Koul <vinod.koul@intel.com> writes:
>> On Wed, Dec 07, 2016 at 04:45:58PM +0000, Måns Rullgård wrote:
>>> Vinod Koul <vinod.koul@intel.com> writes:
>>> > On Tue, Dec 06, 2016 at 01:14:20PM +0000, Måns Rullgård wrote:
>>> >> That's not going to work very well.  Device drivers typically request
>>> >> dma channels in their probe functions or when the device is opened.
>>> >> This means that reserving one of the few channels there will inevitably
>>> >> make some other device fail to operate.
>>> >
>>> > No that doesnt make sense at all, you should get a channel only when you
>>> > want to use it and not in probe!
>>>
>>> Tell that to just about every single driver ever written.
>>
>> Not really, few do yes which is wrong but not _all_ do that.
>
> Every driver I ever looked at does.  Name one you consider "correct."

I'm far from claiming that drivers/tty/serial/sh-sci.c is perfect, but it does
request DMA channels at open time, not at probe time.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: Tearing down DMA transfer setup after DMA client has finished

[Geert Uytterhoeven]

Hi Måns,

On Thu, Dec 8, 2016 at 12:47 PM, Måns Rullgård <mans@mansr.com> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>> On Thu, Dec 8, 2016 at 11:54 AM, Mason <slash.tmp@free.fr> wrote:
>>> On 08/12/2016 11:39, Vinod Koul wrote:
>>>> On Wed, Dec 07, 2016 at 04:45:58PM +0000, Måns Rullgård wrote:
>>>>> Vinod Koul <vinod.koul@intel.com> writes:
>>>>>> On Tue, Dec 06, 2016 at 01:14:20PM +0000, Måns Rullgård wrote:
>>>>>>> That's not going to work very well.  Device drivers typically request
>>>>>>> dma channels in their probe functions or when the device is opened.
>>>>>>> This means that reserving one of the few channels there will inevitably
>>>>>>> make some other device fail to operate.
>>>>>>
>>>>>> No that doesn't make sense at all, you should get a channel only when you
>>>>>> want to use it and not in probe!
>>>>>
>>>>> Tell that to just about every single driver ever written.
>>>>
>>>> Not really, few do yes which is wrong but not _all_ do that.
>>>
>>> Vinod,
>>>
>>> Could you explain something to me in layman's terms?
>>>
>>> I have a NAND Flash Controller driver that depends on the
>>> DMA driver under discussion.
>>>
>>> Suppose I move the dma_request_chan() call from the driver's
>>> probe function, to the actual DMA transfer function.
>>>
>>> I would want dma_request_chan() to put the calling thread
>>> to sleep until a channel becomes available (possibly with
>>> a timeout value).
>>>
>>> But Maxime told me dma_request_chan() will just return
>>> -EBUSY if no channels are available.
>>>
>>> Am I supposed to busy wait in my driver's DMA function
>>> until a channel becomes available?
>>
>> Can you fall back to PIO if requesting a channel fails?
>>
>> Alternatively, dma_request_chan() could always succeed, and
>> dmaengine_prep_slave_sg() could fail if the channel is currently not
>> available due to a limitation on the number of active channels, and
>> the driver could fall back to PIO for that transfer.
>
> Why are we debating this nonsense?  There is an easy fix that doesn't
> require changing the semantics of existing functions or falling back to
> slow pio.

You still want to fall back to PIO if the DMA engine is not available at all
(e.g. DMA engine driver not compiled in, or module not loaded).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: Tearing down DMA transfer setup after DMA client has finished

[Mason]

On 08/12/2016 13:03, Geert Uytterhoeven wrote:

> Måns Rullgård wrote:
>
>> Geert Uytterhoeven writes:
>>
>>> Can you fall back to PIO if requesting a channel fails?
>>
>> Why are we debating this nonsense?  There is an easy fix that doesn't
>> require changing the semantics of existing functions or falling back to
>> slow pio.
> 
> You still want to fall back to PIO if the DMA engine is not available
> at all (e.g. DMA engine driver not compiled in, or module not loaded).

FWIW, the ECC engine is tied to the DMA engine. So PIO means
not only taking a hit from tying up the CPU for a slooow
transfer, but also a huge hit if ECC must be computed in SW.
(A 100x perf degradation is not unlikely.)

Regards.

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> On Thu, Dec 8, 2016 at 12:44 PM, Måns Rullgård <mans@mansr.com> wrote:
>> Vinod Koul <vinod.koul@intel.com> writes:
>>> On Wed, Dec 07, 2016 at 04:45:58PM +0000, Måns Rullgård wrote:
>>>> Vinod Koul <vinod.koul@intel.com> writes:
>>>> > On Tue, Dec 06, 2016 at 01:14:20PM +0000, Måns Rullgård wrote:
>>>> >> That's not going to work very well.  Device drivers typically request
>>>> >> dma channels in their probe functions or when the device is opened.
>>>> >> This means that reserving one of the few channels there will inevitably
>>>> >> make some other device fail to operate.
>>>> >
>>>> > No that doesnt make sense at all, you should get a channel only when you
>>>> > want to use it and not in probe!
>>>>
>>>> Tell that to just about every single driver ever written.
>>>
>>> Not really, few do yes which is wrong but not _all_ do that.
>>
>> Every driver I ever looked at does.  Name one you consider "correct."
>
> I'm far from claiming that drivers/tty/serial/sh-sci.c is perfect, but
> it does request DMA channels at open time, not at probe time.

In the part quoted above, I said most drivers request dma channels in
their probe or open functions.  For the purposes of this discussion,
that distinction is irrelevant.  In either case, the channel is held
indefinitely.  If this wasn't the correct way to use the dmaengine,
there would be no need for the virt-dma helpers which are specifically
designed for cases the one currently at hand.

The only problem we have is that nobody envisioned hardware where the
dma engine indicates completion slightly too soon.  I suspect there's a
fifo or such somewhere, and the interrupt is triggered when the last
byte has been placed in the fifo rather than when it has been removed
which would have been more correct.

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> Hi Måns,
>
> On Thu, Dec 8, 2016 at 12:47 PM, Måns Rullgård <mans@mansr.com> wrote:
>> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>>> On Thu, Dec 8, 2016 at 11:54 AM, Mason <slash.tmp@free.fr> wrote:
>>>> On 08/12/2016 11:39, Vinod Koul wrote:
>>>>> On Wed, Dec 07, 2016 at 04:45:58PM +0000, Måns Rullgård wrote:
>>>>>> Vinod Koul <vinod.koul@intel.com> writes:
>>>>>>> On Tue, Dec 06, 2016 at 01:14:20PM +0000, Måns Rullgård wrote:
>>>>>>>> That's not going to work very well.  Device drivers typically request
>>>>>>>> dma channels in their probe functions or when the device is opened.
>>>>>>>> This means that reserving one of the few channels there will inevitably
>>>>>>>> make some other device fail to operate.
>>>>>>>
>>>>>>> No that doesn't make sense at all, you should get a channel only when you
>>>>>>> want to use it and not in probe!
>>>>>>
>>>>>> Tell that to just about every single driver ever written.
>>>>>
>>>>> Not really, few do yes which is wrong but not _all_ do that.
>>>>
>>>> Vinod,
>>>>
>>>> Could you explain something to me in layman's terms?
>>>>
>>>> I have a NAND Flash Controller driver that depends on the
>>>> DMA driver under discussion.
>>>>
>>>> Suppose I move the dma_request_chan() call from the driver's
>>>> probe function, to the actual DMA transfer function.
>>>>
>>>> I would want dma_request_chan() to put the calling thread
>>>> to sleep until a channel becomes available (possibly with
>>>> a timeout value).
>>>>
>>>> But Maxime told me dma_request_chan() will just return
>>>> -EBUSY if no channels are available.
>>>>
>>>> Am I supposed to busy wait in my driver's DMA function
>>>> until a channel becomes available?
>>>
>>> Can you fall back to PIO if requesting a channel fails?
>>>
>>> Alternatively, dma_request_chan() could always succeed, and
>>> dmaengine_prep_slave_sg() could fail if the channel is currently not
>>> available due to a limitation on the number of active channels, and
>>> the driver could fall back to PIO for that transfer.
>>
>> Why are we debating this nonsense?  There is an easy fix that doesn't
>> require changing the semantics of existing functions or falling back to
>> slow pio.
>
> You still want to fall back to PIO if the DMA engine is not available at all
> (e.g. DMA engine driver not compiled in, or module not loaded).

That's a choice for each device driver to make.  Some devices don't have
a pio mode at all.

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Geert Uytterhoeven]

On Thu, Dec 8, 2016 at 1:20 PM, Måns Rullgård <mans@mansr.com> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>> On Thu, Dec 8, 2016 at 12:44 PM, Måns Rullgård <mans@mansr.com> wrote:
>>> Vinod Koul <vinod.koul@intel.com> writes:
>>>> On Wed, Dec 07, 2016 at 04:45:58PM +0000, Måns Rullgård wrote:
>>>>> Vinod Koul <vinod.koul@intel.com> writes:
>>>>> > On Tue, Dec 06, 2016 at 01:14:20PM +0000, Måns Rullgård wrote:
>>>>> >> That's not going to work very well.  Device drivers typically request
>>>>> >> dma channels in their probe functions or when the device is opened.
>>>>> >> This means that reserving one of the few channels there will inevitably
>>>>> >> make some other device fail to operate.
>>>>> >
>>>>> > No that doesnt make sense at all, you should get a channel only when you
>>>>> > want to use it and not in probe!
>>>>>
>>>>> Tell that to just about every single driver ever written.
>>>>
>>>> Not really, few do yes which is wrong but not _all_ do that.
>>>
>>> Every driver I ever looked at does.  Name one you consider "correct."
>>
>> I'm far from claiming that drivers/tty/serial/sh-sci.c is perfect, but
>> it does request DMA channels at open time, not at probe time.
>
> In the part quoted above, I said most drivers request dma channels in
> their probe or open functions.  For the purposes of this discussion,
> that distinction is irrelevant.  In either case, the channel is held
> indefinitely.  If this wasn't the correct way to use the dmaengine,
> there would be no need for the virt-dma helpers which are specifically
> designed for cases the one currently at hand.

Sorry, I mainly read Vinod's "not in probe", and missed your "or when the
device is opened".

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: Tearing down DMA transfer setup after DMA client has finished

[Mason]

On 08/12/2016 13:20, Måns Rullgård wrote:

> The only problem we have is that nobody envisioned hardware where the
> dma engine indicates completion slightly too soon.  I suspect there's a
> fifo or such somewhere, and the interrupt is triggered when the last
> byte has been placed in the fifo rather than when it has been removed
> which would have been more correct.

As I (tried to) explain here:
https://marc.info/?l=dmaengine&m=148007808418242&w=2

A *read* MBUS agent raises its IRQ when it is safe for the memory
to be overwritten (i.e. every byte has been pushed into the pipe).

A *write* MBUS agent raises its IRQ when it is safe for another
agent to read any one of the transferred bytes.

The issue comes from the fact that, for a memory-to-device transfer,
the system will receive the read agent's IRQ, but most devices
(NFC, SATA) don't have an IRQ line to signal that their part of the
operation is complete.

As I explained, if one sets up a memory-to-memory DMA copy, then
the system will actually receive *two* IRQs.

Regards.

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Mason <slash.tmp@free.fr> writes:

> On 08/12/2016 13:20, Måns Rullgård wrote:
>
>> The only problem we have is that nobody envisioned hardware where the
>> dma engine indicates completion slightly too soon.  I suspect there's a
>> fifo or such somewhere, and the interrupt is triggered when the last
>> byte has been placed in the fifo rather than when it has been removed
>> which would have been more correct.
>
> As I (tried to) explain here:
> https://marc.info/?l=dmaengine&m=148007808418242&w=2
>
> A *read* MBUS agent raises its IRQ when it is safe for the memory
> to be overwritten (i.e. every byte has been pushed into the pipe).
>
> A *write* MBUS agent raises its IRQ when it is safe for another
> agent to read any one of the transferred bytes.
>
> The issue comes from the fact that, for a memory-to-device transfer,
> the system will receive the read agent's IRQ, but most devices
> (NFC, SATA) don't have an IRQ line to signal that their part of the
> operation is complete.

SATA does, actually.  Nevertheless, it's an unusual design.

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Mason]

On 08/12/2016 13:44, Måns Rullgård wrote:

> Mason <slash.tmp@free.fr> writes:
> 
>> On 08/12/2016 13:20, Måns Rullgård wrote:
>>
>>> The only problem we have is that nobody envisioned hardware where the
>>> dma engine indicates completion slightly too soon.  I suspect there's a
>>> fifo or such somewhere, and the interrupt is triggered when the last
>>> byte has been placed in the fifo rather than when it has been removed
>>> which would have been more correct.
>>
>> As I (tried to) explain here:
>> https://marc.info/?l=dmaengine&m=148007808418242&w=2
>>
>> A *read* MBUS agent raises its IRQ when it is safe for the memory
>> to be overwritten (i.e. every byte has been pushed into the pipe).
>>
>> A *write* MBUS agent raises its IRQ when it is safe for another
>> agent to read any one of the transferred bytes.
>>
>> The issue comes from the fact that, for a memory-to-device transfer,
>> the system will receive the read agent's IRQ, but most devices
>> (NFC, SATA) don't have an IRQ line to signal that their part of the
>> operation is complete.
> 
> SATA does, actually.  Nevertheless, it's an unusual design.

Thanks, I was mistaken about the SATA controller.

On tango3 (and also tango4, I assume)

IRQ 41 = Serial ATA #0
IRQ 42 = Serial ATA DMA #0
IRQ 54 = Serial ATA #1
IRQ 55 = Serial ATA DMA #1

But in the end, whether there is a device interrupt (SATA)
or not (NFC), for a memory-to-device transfer, the DMA
driver will get the read agent notification (which should
be ignored) and the client driver should either spin until
idle (NFC) or wait for its completion IRQ (SATA).

Correct?

Regards.

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Mason <slash.tmp@free.fr> writes:

> On 08/12/2016 13:44, Måns Rullgård wrote:
>
>> Mason <slash.tmp@free.fr> writes:
>> 
>>> On 08/12/2016 13:20, Måns Rullgård wrote:
>>>
>>>> The only problem we have is that nobody envisioned hardware where the
>>>> dma engine indicates completion slightly too soon.  I suspect there's a
>>>> fifo or such somewhere, and the interrupt is triggered when the last
>>>> byte has been placed in the fifo rather than when it has been removed
>>>> which would have been more correct.
>>>
>>> As I (tried to) explain here:
>>> https://marc.info/?l=dmaengine&m=148007808418242&w=2
>>>
>>> A *read* MBUS agent raises its IRQ when it is safe for the memory
>>> to be overwritten (i.e. every byte has been pushed into the pipe).
>>>
>>> A *write* MBUS agent raises its IRQ when it is safe for another
>>> agent to read any one of the transferred bytes.
>>>
>>> The issue comes from the fact that, for a memory-to-device transfer,
>>> the system will receive the read agent's IRQ, but most devices
>>> (NFC, SATA) don't have an IRQ line to signal that their part of the
>>> operation is complete.
>> 
>> SATA does, actually.  Nevertheless, it's an unusual design.
>
> Thanks, I was mistaken about the SATA controller.
>
> On tango3 (and also tango4, I assume)
>
> IRQ 41 = Serial ATA #0
> IRQ 42 = Serial ATA DMA #0
> IRQ 54 = Serial ATA #1
> IRQ 55 = Serial ATA DMA #1
>
> But in the end, whether there is a device interrupt (SATA)
> or not (NFC), for a memory-to-device transfer, the DMA
> driver will get the read agent notification (which should
> be ignored) and the client driver should either spin until
> idle (NFC) or wait for its completion IRQ (SATA).
>
> Correct?

Yes, and when the client device is finished, the driver needs to signal
this to the dma driver so it can reuse the channel.  It's this last
piece that's missing.

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Vinod Koul]

On Thu, Dec 08, 2016 at 11:44:53AM +0000, Måns Rullgård wrote:
> Vinod Koul <vinod.koul@intel.com> writes:
> 
> > On Wed, Dec 07, 2016 at 04:45:58PM +0000, Måns Rullgård wrote:
> >> Vinod Koul <vinod.koul@intel.com> writes:
> >> 
> >> > On Tue, Dec 06, 2016 at 01:14:20PM +0000, Måns Rullgård wrote:
> >> >> 
> >> >> That's not going to work very well.  Device drivers typically request
> >> >> dma channels in their probe functions or when the device is opened.
> >> >> This means that reserving one of the few channels there will inevitably
> >> >> make some other device fail to operate.
> >> >
> >> > No that doesnt make sense at all, you should get a channel only when you
> >> > want to use it and not in probe!
> >> 
> >> Tell that to just about every single driver ever written.
> >
> > Not really, few do yes which is wrong but not _all_ do that.
> 
> Every driver I ever looked at does.  Name one you consider "correct."

That only tells me you haven't looked enough and want to rant!

FWIW look at ALSA-dmaengine lib, thereby every audio driver that uses it. I
could find other examples and go on and on, but that's besides the point and
looks like you don't want to listen to people telling you something..

-- 
~Vinod

Re: Tearing down DMA transfer setup after DMA client has finished

[Mason]

On 08/12/2016 16:40, Vinod Koul wrote:

> FWIW look at ALSA-dmaengine lib, thereby every audio driver that uses it. I
> could find other examples and go on and on, but that's besides the point and
> looks like you don't want to listen to people telling you something..

Hello Vinod,

FWIW, your system clock seems to be 10 minutes ahead ;-)

I am willing to learn. I have a few outstanding questions
in the thread. Could you have a look at them?

Regards.

Re: Tearing down DMA transfer setup after DMA client has finished

[Vinod Koul]

On Thu, Dec 08, 2016 at 12:20:30PM +0000, Måns Rullgård wrote:
> >
> > I'm far from claiming that drivers/tty/serial/sh-sci.c is perfect, but
> > it does request DMA channels at open time, not at probe time.
> 
> In the part quoted above, I said most drivers request dma channels in
> their probe or open functions.  For the purposes of this discussion,
> that distinction is irrelevant.  In either case, the channel is held
> indefinitely.  

And the answer was it is wrong and not _all_ do that!!

> If this wasn't the correct way to use the dmaengine,
> there would be no need for the virt-dma helpers which are specifically
> designed for cases the one currently at hand.

That is incorrect.

virt-dma helps to have multiple request from various clients. For many
controllers which implement a SW mux, they can transfer data for one client
and then next transfer can be for some other one.

This allows better utilization of dma channels and helps in case where we
have fewer dma channels than users. This was _not_ developed to let people
grab channels in probe.

> The only problem we have is that nobody envisioned hardware where the
> dma engine indicates completion slightly too soon.  I suspect there's a
> fifo or such somewhere, and the interrupt is triggered when the last
> byte has been placed in the fifo rather than when it has been removed
> which would have been more correct.

That is pretty common hardware optimization but usually hardware shows up
with flush commands to let in flight transactions be completed.

One other example of this implementations has already been pointed in this
thread

-- 
~Vinod

Re: Tearing down DMA transfer setup after DMA client has finished

[Vinod Koul]

On Thu, Dec 08, 2016 at 04:43:53PM +0100, Mason wrote:
> On 08/12/2016 16:40, Vinod Koul wrote:
> 
> > FWIW look at ALSA-dmaengine lib, thereby every audio driver that uses it. I
> > could find other examples and go on and on, but that's besides the point and
> > looks like you don't want to listen to people telling you something..
> 
> Hello Vinod,
> 
> FWIW, your system clock seems to be 10 minutes ahead ;-)

Ah thanks for pointing, fixed now :)
> 
> I am willing to learn. I have a few outstanding questions
> in the thread. Could you have a look at them?

Sure thing, I have replied/ing to few things, feel free to ping me if I miss
something today..

-- 
~Vinod

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Vinod Koul <vinod.koul@intel.com> writes:

> On Thu, Dec 08, 2016 at 12:20:30PM +0000, Måns Rullgård wrote:
>> >
>> > I'm far from claiming that drivers/tty/serial/sh-sci.c is perfect, but
>> > it does request DMA channels at open time, not at probe time.
>> 
>> In the part quoted above, I said most drivers request dma channels in
>> their probe or open functions.  For the purposes of this discussion,
>> that distinction is irrelevant.  In either case, the channel is held
>> indefinitely.  
>
> And the answer was it is wrong and not _all_ do that!!

Show me one that doesn't.

>> If this wasn't the correct way to use the dmaengine,
>> there would be no need for the virt-dma helpers which are specifically
>> designed for cases the one currently at hand.
>
> That is incorrect.
>
> virt-dma helps to have multiple request from various clients. For many
> controllers which implement a SW mux, they can transfer data for one client
> and then next transfer can be for some other one.
>
> This allows better utilization of dma channels and helps in case where we
> have fewer dma channels than users.

Which is *exactly* the situation we have here.

I have no idea what you're arguing for or against any more.  Perhaps
you're just arguing.

>> The only problem we have is that nobody envisioned hardware where the
>> dma engine indicates completion slightly too soon.  I suspect there's a
>> fifo or such somewhere, and the interrupt is triggered when the last
>> byte has been placed in the fifo rather than when it has been removed
>> which would have been more correct.
>
> That is pretty common hardware optimization but usually hardware shows up
> with flush commands to let in flight transactions be completed.

Well, this hardware seems to lack that particular feature.  Pretending
otherwise isn't helping.

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Vinod Koul]

On Thu, Dec 08, 2016 at 11:54:51AM +0100, Mason wrote:
> On 08/12/2016 11:39, Vinod Koul wrote:
> 
> > On Wed, Dec 07, 2016 at 04:45:58PM +0000, Måns Rullgård wrote:
> >
> >> Vinod Koul <vinod.koul@intel.com> writes:
> >>
> >>> On Tue, Dec 06, 2016 at 01:14:20PM +0000, Måns Rullgård wrote:
> >>>>
> >>>> That's not going to work very well.  Device drivers typically request
> >>>> dma channels in their probe functions or when the device is opened.
> >>>> This means that reserving one of the few channels there will inevitably
> >>>> make some other device fail to operate.
> >>>
> >>> No that doesn't make sense at all, you should get a channel only when you
> >>> want to use it and not in probe!
> >>
> >> Tell that to just about every single driver ever written.
> > 
> > Not really, few do yes which is wrong but not _all_ do that.
> 
> Vinod,
> 
> Could you explain something to me in layman's terms?
> 
> I have a NAND Flash Controller driver that depends on the
> DMA driver under discussion.
> 
> Suppose I move the dma_request_chan() call from the driver's
> probe function, to the actual DMA transfer function.
> 
> I would want dma_request_chan() to put the calling thread
> to sleep until a channel becomes available (possibly with
> a timeout value).
> 
> But Maxime told me dma_request_chan() will just return
> -EBUSY if no channels are available.

That is correct

> Am I supposed to busy wait in my driver's DMA function
> until a channel becomes available?

If someone else is using the channels then the bust wait will not help, so
in this case you should fall back to PIO, few drivers do that

> I don't understand how the multiplexing of few memory
> channels to many clients is supposed to happen efficiently?

To make it efficient, disregarding your Sbox HW issue, the solution is
virtual channels. You can delink physical channels and virtual channels. If
one has SW controlled MUX then a channel can service any client. For few
controllers request lines are hard wired so they cant use any channel. But
if you dont have this restriction then driver can queue up many transactions
from different controllers.

-- 
~Vinod

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Vinod Koul <vinod.koul@intel.com> writes:

> On Thu, Dec 08, 2016 at 11:44:53AM +0000, Måns Rullgård wrote:
>> Vinod Koul <vinod.koul@intel.com> writes:
>> 
>> > On Wed, Dec 07, 2016 at 04:45:58PM +0000, Måns Rullgård wrote:
>> >> Vinod Koul <vinod.koul@intel.com> writes:
>> >> 
>> >> > On Tue, Dec 06, 2016 at 01:14:20PM +0000, Måns Rullgård wrote:
>> >> >> 
>> >> >> That's not going to work very well.  Device drivers typically request
>> >> >> dma channels in their probe functions or when the device is opened.
>> >> >> This means that reserving one of the few channels there will inevitably
>> >> >> make some other device fail to operate.
>> >> >
>> >> > No that doesnt make sense at all, you should get a channel only when you
>> >> > want to use it and not in probe!
>> >> 
>> >> Tell that to just about every single driver ever written.
>> >
>> > Not really, few do yes which is wrong but not _all_ do that.
>> 
>> Every driver I ever looked at does.  Name one you consider "correct."
>
> That only tells me you haven't looked enough and want to rant!
>
> FWIW look at ALSA-dmaengine lib, thereby every audio driver that uses it. I
> could find other examples and go on and on, but that's besides the point and
> looks like you don't want to listen to people telling you something..

ALSA is a particularly poor example.  ALSA drivers request a dma channel
at some point between the device being opened and playback (or capture)
starting.  They then set up a cyclic transfer that runs continuously
until playback is stopped.  It's a completely different model of
operation than we're talking about here.

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Vinod Koul <vinod.koul@intel.com> writes:

> To make it efficient, disregarding your Sbox HW issue, the solution is
> virtual channels. You can delink physical channels and virtual channels. If
> one has SW controlled MUX then a channel can service any client. For few
> controllers request lines are hard wired so they cant use any channel. But
> if you dont have this restriction then driver can queue up many transactions
> from different controllers.

Have you been paying attention at all?  This exactly what the driver
ALREADY DOES.

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Vinod Koul]

On Thu, Dec 08, 2016 at 04:48:18PM +0000, Måns Rullgård wrote:
> Vinod Koul <vinod.koul@intel.com> writes:
> 
> > To make it efficient, disregarding your Sbox HW issue, the solution is
> > virtual channels. You can delink physical channels and virtual channels. If
> > one has SW controlled MUX then a channel can service any client. For few
> > controllers request lines are hard wired so they cant use any channel. But
> > if you dont have this restriction then driver can queue up many transactions
> > from different controllers.
> 
> Have you been paying attention at all?  This exactly what the driver
> ALREADY DOES.

And have you read what the question was?

-- 
~Vinod

Re: Tearing down DMA transfer setup after DMA client has finished

[Sebastian Frias]

On 09/12/16 07:59, Vinod Koul wrote:
> On Thu, Dec 08, 2016 at 04:48:18PM +0000, Måns Rullgård wrote:
>> Vinod Koul <vinod.koul@intel.com> writes:
>>
>>> To make it efficient, disregarding your Sbox HW issue, the solution is
>>> virtual channels. You can delink physical channels and virtual channels. If
>>> one has SW controlled MUX then a channel can service any client. For few
>>> controllers request lines are hard wired so they cant use any channel. But
>>> if you dont have this restriction then driver can queue up many transactions
>>> from different controllers.
>>
>> Have you been paying attention at all?  This exactly what the driver
>> ALREADY DOES.
> 
> And have you read what the question was?
> 

I think many people appreciate the quick turn around time and responsiveness of
knowledgeable people making constructive remarks in this thread, but it looks we
are slowly drifting away from the main problem.

If we had to sum up the discussion, the current DMA API/framework in Linux seems
to lack a way to properly handle this HW (or if it has a way, the information got
lost somewhere).

What concrete solution do you propose?

Alternatively, one can think of the current issue (i.e.: the fact that the IRQ
arrives "too soon") in a different way.
Instead of thinking the IRQ indicates "transfer complete", it is indicating "ready
to accept another command", which in practice (and with proper API support) can
translate into efficient queuing of DMA operations.

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Sebastian Frias <sf84@laposte.net> writes:

> On 09/12/16 07:59, Vinod Koul wrote:
>> On Thu, Dec 08, 2016 at 04:48:18PM +0000, Måns Rullgård wrote:
>>> Vinod Koul <vinod.koul@intel.com> writes:
>>>
>>>> To make it efficient, disregarding your Sbox HW issue, the solution is
>>>> virtual channels. You can delink physical channels and virtual channels. If
>>>> one has SW controlled MUX then a channel can service any client. For few
>>>> controllers request lines are hard wired so they cant use any channel. But
>>>> if you dont have this restriction then driver can queue up many transactions
>>>> from different controllers.
>>>
>>> Have you been paying attention at all?  This exactly what the driver
>>> ALREADY DOES.
>> 
>> And have you read what the question was?

I wrote the driver.  I think I know what Mason and I are asking.

> I think many people appreciate the quick turn around time and
> responsiveness of knowledgeable people making constructive remarks in
> this thread, but it looks we are slowly drifting away from the main
> problem.
>
> If we had to sum up the discussion, the current DMA API/framework in
> Linux seems to lack a way to properly handle this HW (or if it has a
> way, the information got lost somewhere).
>
> What concrete solution do you propose?
>
> Alternatively, one can think of the current issue (i.e.: the fact that
> the IRQ arrives "too soon") in a different way.  Instead of thinking
> the IRQ indicates "transfer complete", it is indicating "ready to
> accept another command", which in practice (and with proper API
> support) can translate into efficient queuing of DMA operations.

For multiple back to back transfers to the same peripheral, it is indeed
a slight optimisation.  What's apparently lacking is some way of doing a
full flush 

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[1Måns Rullgård]

Sebastian Frias <sf84@laposte.net> writes:

> On 09/12/16 07:59, Vinod Koul wrote:
>> On Thu, Dec 08, 2016 at 04:48:18PM +0000, Måns Rullgård wrote:
>>> Vinod Koul <vinod.koul@intel.com> writes:
>>>
>>>> To make it efficient, disregarding your Sbox HW issue, the solution is
>>>> virtual channels. You can delink physical channels and virtual channels. If
>>>> one has SW controlled MUX then a channel can service any client. For few
>>>> controllers request lines are hard wired so they cant use any channel. But
>>>> if you dont have this restriction then driver can queue up many transactions
>>>> from different controllers.
>>>
>>> Have you been paying attention at all?  This exactly what the driver
>>> ALREADY DOES.
>> 
>> And have you read what the question was?

I wrote the driver.  I think I know what Mason and I are asking.

> I think many people appreciate the quick turn around time and
> responsiveness of knowledgeable people making constructive remarks in
> this thread, but it looks we are slowly drifting away from the main
> problem.
>
> If we had to sum up the discussion, the current DMA API/framework in
> Linux seems to lack a way to properly handle this HW (or if it has a
> way, the information got lost somewhere).
>
> What concrete solution do you propose?
>
> Alternatively, one can think of the current issue (i.e.: the fact that
> the IRQ arrives "too soon") in a different way.  Instead of thinking
> the IRQ indicates "transfer complete", it is indicating "ready to
> accept another command", which in practice (and with proper API
> support) can translate into efficient queuing of DMA operations.

For multiple back to back transfers to the same peripheral, it is indeed
a slight optimisation.  What's apparently lacking is some way of doing a
full flush at the end of a series of transfers, before switching the
channel to another device.

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Vinod Koul]

On Fri, Dec 09, 2016 at 11:25:57AM +0100, Sebastian Frias wrote:
> 
> What concrete solution do you propose?

I have already proposed two solutions.

A) Request a channel only when you need it. Obviously we can't do virtual
channels with this (though we should still use virt-channels framework).
The sbox setup and teardown can be done as part of channel request and
freeup. PL08x already does this.

Downside is that we can only have as many consumers at a time as channels.

I have not heard any technical reason for not doing this apart from drivers
grab the channel at probe, which is incorrect and needs to be fixed
irrespective of the problem at hand.

This is my preferred option.

B) Create a custom driver specific API. This API for example:
	sbox_setup(bool enable, ...)
can be called by client to explicitly setup and clear up the sbox setting.

This way we can have transactions muxed.

I have not heard any arguments on why we shouldn't do this except Russell's
comment that A) solves this.

> Alternatively, one can think of the current issue (i.e.: the fact that the IRQ
> arrives "too soon") in a different way.
> Instead of thinking the IRQ indicates "transfer complete", it is indicating "ready
> to accept another command", which in practice (and with proper API support) can
> translate into efficient queuing of DMA operations.

That IMO is a better understanding of this issue. But based on discussion, I
think the issue is that submitting next transaction cannot be done until
sbox is setup (as a consequence torn down for previous). Tear down can be
down only when client knows that transfer is done, from dma controller data
has been pushed and in-flight.


-- 
~Vinod

Re: Tearing down DMA transfer setup after DMA client has finished

[Måns Rullgård]

Vinod Koul <vinod.koul@intel.com> writes:

> On Fri, Dec 09, 2016 at 11:25:57AM +0100, Sebastian Frias wrote:
>> 
>> What concrete solution do you propose?
>
> I have already proposed two solutions.
>
> A) Request a channel only when you need it. Obviously we can't do virtual
> channels with this (though we should still use virt-channels framework).
> The sbox setup and teardown can be done as part of channel request and
> freeup. PL08x already does this.
>
> Downside is that we can only have as many consumers at a time as channels.
>
> I have not heard any technical reason for not doing this apart from drivers
> grab the channel at probe, which is incorrect and needs to be fixed
> irrespective of the problem at hand.
>
> This is my preferred option.

Sorry, but this is not acceptable.

> B) Create a custom driver specific API. This API for example:
> 	sbox_setup(bool enable, ...)
> can be called by client to explicitly setup and clear up the sbox setting.
>
> This way we can have transactions muxed.
>
> I have not heard any arguments on why we shouldn't do this except Russell's
> comment that A) solves this.

Driver-specific interfaces are not the solution.  That way lies chaos
and madness.

This would all be so much easier if you all would just shut up for a
moment and let me fix it properly.

-- 
Måns Rullgård

Re: Tearing down DMA transfer setup after DMA client has finished

[Mason]

On 09/12/2016 18:17, Vinod Koul wrote:

> On Fri, Dec 09, 2016 at 11:25:57AM +0100, Sebastian Frias wrote:
>>
>> What concrete solution do you propose?
> 
> I have already proposed two solutions.
> 
> A) Request a channel only when you need it. Obviously we can't do virtual
> channels with this (though we should still use virt-channels framework).
> The sbox setup and teardown can be done as part of channel request and
> freeup. PL08x already does this.
> 
> Downside is that we can only have as many consumers at a time as channels.
> 
> I have not heard any technical reason for not doing this apart from drivers
> grab the channel at probe, which is incorrect and needs to be fixed
> irrespective of the problem at hand.
> 
> This is my preferred option.

There is one important drawback with this solution. If a driver calls
dma_request_chan() when no channels are currently available, it will
get -EBUSY. If there were a flag in dma_request_chan to be put to
sleep (with timeout) until a channel is available, then it would
work. But busy waiting in the client driver is a waste of power.

Regards.

Re: Tearing down DMA transfer setup after DMA client has finished

[Vinod Koul]

On Fri, Dec 09, 2016 at 05:28:01PM +0000, Måns Rullgård wrote:
> Vinod Koul <vinod.koul@intel.com> writes:
> 
> > On Fri, Dec 09, 2016 at 11:25:57AM +0100, Sebastian Frias wrote:
> >> 
> >> What concrete solution do you propose?
> >
> > I have already proposed two solutions.
> >
> > A) Request a channel only when you need it. Obviously we can't do virtual
> > channels with this (though we should still use virt-channels framework).
> > The sbox setup and teardown can be done as part of channel request and
> > freeup. PL08x already does this.
> >
> > Downside is that we can only have as many consumers at a time as channels.
> >
> > I have not heard any technical reason for not doing this apart from drivers
> > grab the channel at probe, which is incorrect and needs to be fixed
> > irrespective of the problem at hand.
> >
> > This is my preferred option.
> 
> Sorry, but this is not acceptable.

without outlining why..

> 
> > B) Create a custom driver specific API. This API for example:
> > 	sbox_setup(bool enable, ...)
> > can be called by client to explicitly setup and clear up the sbox setting.
> >
> > This way we can have transactions muxed.
> >
> > I have not heard any arguments on why we shouldn't do this except Russell's
> > comment that A) solves this.
> 
> Driver-specific interfaces are not the solution.  That way lies chaos
> and madness.

Yes fair enough. So would API change which 99% world doesnt need.

> This would all be so much easier if you all would just shut up for a
> moment and let me fix it properly.

Oh please go away, noone is asking you to reply!

-- 
~Vinod

Re: Tearing down DMA transfer setup after DMA client has finished

[Vinod Koul]

On Fri, Dec 09, 2016 at 06:34:15PM +0100, Mason wrote:
> On 09/12/2016 18:17, Vinod Koul wrote:
> 
> > On Fri, Dec 09, 2016 at 11:25:57AM +0100, Sebastian Frias wrote:
> >>
> >> What concrete solution do you propose?
> > 
> > I have already proposed two solutions.
> > 
> > A) Request a channel only when you need it. Obviously we can't do virtual
> > channels with this (though we should still use virt-channels framework).
> > The sbox setup and teardown can be done as part of channel request and
> > freeup. PL08x already does this.
> > 
> > Downside is that we can only have as many consumers at a time as channels.
> > 
> > I have not heard any technical reason for not doing this apart from drivers
> > grab the channel at probe, which is incorrect and needs to be fixed
> > irrespective of the problem at hand.
> > 
> > This is my preferred option.
> 
> There is one important drawback with this solution. If a driver calls
> dma_request_chan() when no channels are currently available, it will
> get -EBUSY. If there were a flag in dma_request_chan to be put to
> sleep (with timeout) until a channel is available, then it would
> work. But busy waiting in the client driver is a waste of power.

Right, but in that case the fallback would be PIO mode, and if that is
not availble (IIRC some f your devices don't) then reject the usage with
EAGAIN.


-- 
~Vinod

Re: Tearing down DMA transfer setup after DMA client has finished

[Vinod Koul]

On Fri, Dec 09, 2016 at 11:26:06PM +0530, Vinod Koul wrote:
> On Fri, Dec 09, 2016 at 06:34:15PM +0100, Mason wrote:
> > On 09/12/2016 18:17, Vinod Koul wrote:
> > 
> > > On Fri, Dec 09, 2016 at 11:25:57AM +0100, Sebastian Frias wrote:
> > >>
> > >> What concrete solution do you propose?
> > > 
> > > I have already proposed two solutions.
> > > 
> > > A) Request a channel only when you need it. Obviously we can't do virtual
> > > channels with this (though we should still use virt-channels framework).
> > > The sbox setup and teardown can be done as part of channel request and
> > > freeup. PL08x already does this.
> > > 
> > > Downside is that we can only have as many consumers at a time as channels.
> > > 
> > > I have not heard any technical reason for not doing this apart from drivers
> > > grab the channel at probe, which is incorrect and needs to be fixed
> > > irrespective of the problem at hand.
> > > 
> > > This is my preferred option.
> > 
> > There is one important drawback with this solution. If a driver calls
> > dma_request_chan() when no channels are currently available, it will
> > get -EBUSY. If there were a flag in dma_request_chan to be put to
> > sleep (with timeout) until a channel is available, then it would
> > work. But busy waiting in the client driver is a waste of power.
> 
> Right, but in that case the fallback would be PIO mode, and if that is
> not availble (IIRC some f your devices don't) then reject the usage with
> EAGAIN.

Alternatively I can think of one more way.

If there is fixed delay or maximum delay predicted between ISR being
fired and transaction being completed from client, then we can use that
magic value and degrade the performance a bit but make a simpler system
than other two suggestions.

The idea here is that typically the subsequent transaction should be
issued as soon as possible, best case being in the ISR. But we can
degrade that performance a bit and issue in the tasklet. But that can be
done after introducing a delay, that too only in the case where new sbox
configuration is different from previous one (so performance degrade is
only on the switch and not for txn for same setup). You can possible
optimize even further by issuing in ISR for same sbox setup and issuing
in tasklet if configuration is different.

Yes this is bit iffy and adds more burden on driver, but lets us get
away with decent performance and being able to handle the hardware
condition.

Would that work for your case...?

-- 
~Vinod

Re: Tearing down DMA transfer setup after DMA client has finished

[Mason]

[ Dropping Mans to preserve his peace-of-mind ]

On 09/12/2016 18:56, Vinod Koul wrote:
> On Fri, Dec 09, 2016 at 06:34:15PM +0100, Mason wrote:
>> On 09/12/2016 18:17, Vinod Koul wrote:
>>
>>> On Fri, Dec 09, 2016 at 11:25:57AM +0100, Sebastian Frias wrote:
>>>>
>>>> What concrete solution do you propose?
>>>
>>> I have already proposed two solutions.
>>>
>>> A) Request a channel only when you need it. Obviously we can't do virtual
>>> channels with this (though we should still use virt-channels framework).
>>> The sbox setup and teardown can be done as part of channel request and
>>> freeup. PL08x already does this.
>>>
>>> Downside is that we can only have as many consumers at a time as channels.
>>>
>>> I have not heard any technical reason for not doing this apart from drivers
>>> grab the channel at probe, which is incorrect and needs to be fixed
>>> irrespective of the problem at hand.
>>>
>>> This is my preferred option.
>>
>> There is one important drawback with this solution. If a driver calls
>> dma_request_chan() when no channels are currently available, it will
>> get -EBUSY. If there were a flag in dma_request_chan to be put to
>> sleep (with timeout) until a channel is available, then it would
>> work. But busy waiting in the client driver is a waste of power.
> 
> Right, but in that case the fallback would be PIO mode, and if that is
> not availble (IIRC some f your devices don't) then reject the usage with
> EAGAIN.

Maybe I'm missing something, but I don't see how that would help.
Take the NAND Flash controller driver, for instance. PIO is not
an option, because the ECC engine is tied to DMA.

And failing with -EAGAIN doesn't help the busy looping situation.
The caller should be put on some kind of queue to wait for a
"channel ready" event.

Regards.

Re: Tearing down DMA transfer setup after DMA client has finished

[Vinod Koul]

On Fri, Dec 09, 2016 at 07:23:17PM +0100, Mason wrote:
> [ Dropping Mans to preserve his peace-of-mind ]
> 
> On 09/12/2016 18:56, Vinod Koul wrote:
> > On Fri, Dec 09, 2016 at 06:34:15PM +0100, Mason wrote:
> >> On 09/12/2016 18:17, Vinod Koul wrote:
> >>
> >>> On Fri, Dec 09, 2016 at 11:25:57AM +0100, Sebastian Frias wrote:
> >>>>
> >>>> What concrete solution do you propose?
> >>>
> >>> I have already proposed two solutions.
> >>>
> >>> A) Request a channel only when you need it. Obviously we can't do virtual
> >>> channels with this (though we should still use virt-channels framework).
> >>> The sbox setup and teardown can be done as part of channel request and
> >>> freeup. PL08x already does this.
> >>>
> >>> Downside is that we can only have as many consumers at a time as channels.
> >>>
> >>> I have not heard any technical reason for not doing this apart from drivers
> >>> grab the channel at probe, which is incorrect and needs to be fixed
> >>> irrespective of the problem at hand.
> >>>
> >>> This is my preferred option.
> >>
> >> There is one important drawback with this solution. If a driver calls
> >> dma_request_chan() when no channels are currently available, it will
> >> get -EBUSY. If there were a flag in dma_request_chan to be put to
> >> sleep (with timeout) until a channel is available, then it would
> >> work. But busy waiting in the client driver is a waste of power.
> > 
> > Right, but in that case the fallback would be PIO mode, and if that is
> > not availble (IIRC some f your devices don't) then reject the usage with
> > EAGAIN.
> 
> Maybe I'm missing something, but I don't see how that would help.
> Take the NAND Flash controller driver, for instance. PIO is not
> an option, because the ECC engine is tied to DMA.
> 
> And failing with -EAGAIN doesn't help the busy looping situation.
> The caller should be put on some kind of queue to wait for a
> "channel ready" event.

So if you go down this route then we have do a bit of engineering for this
solutions to solve the hardware issue.

Can you tell me the clients for this controller and channels available?

In this case possibly we can dedicate one for NAND and keep the ones with
PIO mode dynamic in nature.. But yes it is not an elegant one.

-- 
~Vinod

Re: Tearing down DMA transfer setup after DMA client has finished

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 907 bytes --]

On Fri, Dec 09, 2016 at 07:23:17PM +0100, Mason wrote:
> On 09/12/2016 18:56, Vinod Koul wrote:

> > Right, but in that case the fallback would be PIO mode, and if that is
> > not availble (IIRC some f your devices don't) then reject the usage with
> > EAGAIN.

> Maybe I'm missing something, but I don't see how that would help.
> Take the NAND Flash controller driver, for instance. PIO is not
> an option, because the ECC engine is tied to DMA.

> And failing with -EAGAIN doesn't help the busy looping situation.
> The caller should be put on some kind of queue to wait for a
> "channel ready" event.

Even without the tie into ECC being an issue it seems like falling back
to PIO could produce poor results at the system level - it's likely that
the PIO would be very expensive and take longer than waiting would've
done, increasing the burden on the CPU and slowing things down overall
for userspace.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 484 bytes --]