Re: [PATCH V7 3/3] dma: add Qualcomm Technologies HIDMA channel driver

2015-12-17 Thread Sinan Kaya 
On 12/11/2015 4:35 AM, Vinod Koul wrote:
> On Thu, Dec 10, 2015 at 03:10:48PM -0500, Sinan Kaya wrote:
>> On 12/5/2015 3:00 AM, Vinod Koul wrote:
>>> On Wed, Dec 02, 2015 at 02:04:05PM -0500, Sinan Kaya wrote:
>>> You are missing the point. Channel can be paused, yes but the descriptor
>>> is in queue and is not paused. The descriptor running is paused, yes.
>>> There is subtle difference between these
> I'll follow your recommendation. PAUSE for the currently active
> descriptor and DMA_IN_PROGRESS for the rest.
>

 I'm now confused.

 I looked at several DMA driver implementations.

 1. They call dma_cookie_status function to see if the job is done.
 2. If done, they return right ahead.
 3. Otherwise, dma_cookie_status returns DMA_IN_PROGRESS.
 4. Next the code checks if the channel is paused and return value is
 DMA_IN_PROGRESS. The code changes return code to DMA_PAUSED.

 Whereas, I was returning paused first before even checking if the
 descriptor is done. Are you OK with the sequence 1..4 above?
>>>
>>> Yes am okay with this with slight change in 4.
>>>
>>> You should set to PAUSED only for current descriptor and not for the ones in
>>> queue
>>>
>>
>> OK. I'll post a new version with this. Is there any other comment that
>> needed to be addressed?
> 
> Looks okay to me
> 

Vinod, Mark;
Just posted a new series (V10) https://lkml.org/lkml/2015/12/17/447 that 
addresses
Mark Rutland and your concern. 

Can you please review the series and queue for inclusion if you are OK?
Sinan

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V7 3/3] dma: add Qualcomm Technologies HIDMA channel driver

2015-12-11 Thread Vinod Koul 
On Thu, Dec 10, 2015 at 03:10:48PM -0500, Sinan Kaya wrote:
> On 12/5/2015 3:00 AM, Vinod Koul wrote:
> > On Wed, Dec 02, 2015 at 02:04:05PM -0500, Sinan Kaya wrote:
> > You are missing the point. Channel can be paused, yes but the descriptor
> > is in queue and is not paused. The descriptor running is paused, yes.
> > There is subtle difference between these
> >>> I'll follow your recommendation. PAUSE for the currently active
> >>> descriptor and DMA_IN_PROGRESS for the rest.
> >>>
> >>
> >> I'm now confused.
> >>
> >> I looked at several DMA driver implementations.
> >>
> >> 1. They call dma_cookie_status function to see if the job is done.
> >> 2. If done, they return right ahead.
> >> 3. Otherwise, dma_cookie_status returns DMA_IN_PROGRESS.
> >> 4. Next the code checks if the channel is paused and return value is
> >> DMA_IN_PROGRESS. The code changes return code to DMA_PAUSED.
> >>
> >> Whereas, I was returning paused first before even checking if the
> >> descriptor is done. Are you OK with the sequence 1..4 above?
> > 
> > Yes am okay with this with slight change in 4.
> > 
> > You should set to PAUSED only for current descriptor and not for the ones in
> > queue
> > 
> 
> OK. I'll post a new version with this. Is there any other comment that
> needed to be addressed?

Looks okay to me

-- 
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V7 3/3] dma: add Qualcomm Technologies HIDMA channel driver

2015-12-10 Thread Sinan Kaya 
On 12/5/2015 3:00 AM, Vinod Koul wrote:
> On Wed, Dec 02, 2015 at 02:04:05PM -0500, Sinan Kaya wrote:
> You are missing the point. Channel can be paused, yes but the descriptor
> is in queue and is not paused. The descriptor running is paused, yes.
> There is subtle difference between these
>>> I'll follow your recommendation. PAUSE for the currently active
>>> descriptor and DMA_IN_PROGRESS for the rest.
>>>
>>
>> I'm now confused.
>>
>> I looked at several DMA driver implementations.
>>
>> 1. They call dma_cookie_status function to see if the job is done.
>> 2. If done, they return right ahead.
>> 3. Otherwise, dma_cookie_status returns DMA_IN_PROGRESS.
>> 4. Next the code checks if the channel is paused and return value is
>> DMA_IN_PROGRESS. The code changes return code to DMA_PAUSED.
>>
>> Whereas, I was returning paused first before even checking if the
>> descriptor is done. Are you OK with the sequence 1..4 above?
> 
> Yes am okay with this with slight change in 4.
> 
> You should set to PAUSED only for current descriptor and not for the ones in
> queue
> 

OK. I'll post a new version with this. Is there any other comment that
needed to be addressed?

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V7 3/3] dma: add Qualcomm Technologies HIDMA channel driver

2015-12-08 Thread Sinan Kaya 
On 12/5/2015 3:00 AM, Vinod Koul wrote:
> On Wed, Dec 02, 2015 at 02:04:05PM -0500, Sinan Kaya wrote:
> You are missing the point. Channel can be paused, yes but the descriptor
> is in queue and is not paused. The descriptor running is paused, yes.
> There is subtle difference between these
>>> I'll follow your recommendation. PAUSE for the currently active
>>> descriptor and DMA_IN_PROGRESS for the rest.
>>>
>>
>> I'm now confused.
>>
>> I looked at several DMA driver implementations.
>>
>> 1. They call dma_cookie_status function to see if the job is done.
>> 2. If done, they return right ahead.
>> 3. Otherwise, dma_cookie_status returns DMA_IN_PROGRESS.
>> 4. Next the code checks if the channel is paused and return value is
>> DMA_IN_PROGRESS. The code changes return code to DMA_PAUSED.
>>
>> Whereas, I was returning paused first before even checking if the
>> descriptor is done. Are you OK with the sequence 1..4 above?
> 
> Yes am okay with this with slight change in 4.
> 
> You should set to PAUSED only for current descriptor and not for the ones in
> queue
> 
OK, I'll work on it.

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V7 3/3] dma: add Qualcomm Technologies HIDMA channel driver

2015-12-04 Thread Vinod Koul 
On Wed, Dec 02, 2015 at 02:04:05PM -0500, Sinan Kaya wrote:
> >> > You are missing the point. Channel can be paused, yes but the descriptor
> >> > is in queue and is not paused. The descriptor running is paused, yes.
> >> > There is subtle difference between these
> > I'll follow your recommendation. PAUSE for the currently active
> > descriptor and DMA_IN_PROGRESS for the rest.
> > 
> 
> I'm now confused.
> 
> I looked at several DMA driver implementations.
> 
> 1. They call dma_cookie_status function to see if the job is done.
> 2. If done, they return right ahead.
> 3. Otherwise, dma_cookie_status returns DMA_IN_PROGRESS.
> 4. Next the code checks if the channel is paused and return value is
> DMA_IN_PROGRESS. The code changes return code to DMA_PAUSED.
> 
> Whereas, I was returning paused first before even checking if the
> descriptor is done. Are you OK with the sequence 1..4 above?

Yes am okay with this with slight change in 4.

You should set to PAUSED only for current descriptor and not for the ones in
queue

-- 
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V7 3/3] dma: add Qualcomm Technologies HIDMA channel driver

2015-12-02 Thread Sinan Kaya 
On 12/1/2015 4:16 PM, Sinan Kaya wrote:
 >>>
>  +static enum dma_status hidma_tx_status(struct dma_chan *dmach,
>  +dma_cookie_t cookie, struct dma_tx_state 
>  *txstate)
>  +{
>  +struct hidma_chan *mchan = to_hidma_chan(dmach);
>  +enum dma_status ret;
>  +
>  +if (mchan->paused)
>  +ret = DMA_PAUSED;
 >>>
 >>> This is not quite right. The status query is for a descriptor and NOT 
 >>> for
 >>> channel. Here if the descriptor queried was running then it would be 
 >>> paused
 >>> for the rest pending txn, it would be DMA_IN_PROGRESS.
>>> >>
>>> >> The channel can be paused by the hypervisor. If it is paused, then no
>>> >> other transaction will go through including the pending requests. That's
>>> >> why, I'm checking the state first.
>> > 
>> > You are missing the point. Channel can be paused, yes but the descriptor
>> > is in queue and is not paused. The descriptor running is paused, yes.
>> > There is subtle difference between these
> I'll follow your recommendation. PAUSE for the currently active
> descriptor and DMA_IN_PROGRESS for the rest.
> 

I'm now confused.

I looked at several DMA driver implementations.

1. They call dma_cookie_status function to see if the job is done.
2. If done, they return right ahead.
3. Otherwise, dma_cookie_status returns DMA_IN_PROGRESS.
4. Next the code checks if the channel is paused and return value is
DMA_IN_PROGRESS. The code changes return code to DMA_PAUSED.

Whereas, I was returning paused first before even checking if the
descriptor is done. Are you OK with the sequence 1..4 above?

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V7 3/3] dma: add Qualcomm Technologies HIDMA channel driver

2015-12-01 Thread Sinan Kaya 
On 12/1/2015 6:34 AM, Vinod Koul wrote:
> On Mon, Nov 30, 2015 at 03:06:36PM -0500, Sinan Kaya wrote:
>> I have split the debugfs support from this patch to its own patch. Any
>> other idea on how else you'd break the code? I can take one more step
>> and separate the lower layer from the OS layer by using stub functions
>> on the initial commit.
> 
> Yes the ll.c can be a separate patch and no need to add placeholders as
> makefile can be last
> 

ok. I'll create a new patch file with the Makefile and hidma_ll.c only.

>>>
 +static enum dma_status hidma_tx_status(struct dma_chan *dmach,
 +  dma_cookie_t cookie, struct dma_tx_state *txstate)
 +{
 +  struct hidma_chan *mchan = to_hidma_chan(dmach);
 +  enum dma_status ret;
 +
 +  if (mchan->paused)
 +  ret = DMA_PAUSED;
>>>
>>> This is not quite right. The status query is for a descriptor and NOT for
>>> channel. Here if the descriptor queried was running then it would be paused
>>> for the rest pending txn, it would be DMA_IN_PROGRESS.
>>
>> The channel can be paused by the hypervisor. If it is paused, then no
>> other transaction will go through including the pending requests. That's
>> why, I'm checking the state first.
> 
> You are missing the point. Channel can be paused, yes but the descriptor
> is in queue and is not paused. The descriptor running is paused, yes.
> There is subtle difference between these

I'll follow your recommendation. PAUSE for the currently active
descriptor and DMA_IN_PROGRESS for the rest.

> 
 +static dma_cookie_t hidma_tx_submit(struct dma_async_tx_descriptor *txd)
 +{
 +  struct hidma_chan *mchan = to_hidma_chan(txd->chan);
 +  struct hidma_dev *dmadev = mchan->dmadev;
 +  struct hidma_desc *mdesc;
 +  unsigned long irqflags;
 +  dma_cookie_t cookie;
 +
 +  if (!hidma_ll_isenabled(dmadev->lldev))
 +  return -ENODEV;
>>>
>>> is this not too late for this check, you should fail this on prepare
>>> method
>>
>> The channels can be paused by the hypervisor at runtime after the guest
>> OS boot. The client might have allocated the channels during guest
>> machine boot. If a channel is paused and client makes a request, client
>> will never get the callback. By placing this check, I'm looking at the
>> runtime status of the channel and rejecting the transmit request right
>> ahead.
> 
> In this case you have request already given to you, here the request is
> submitted to the pending queue, 

No, it is not. Putting into the pending queue is done after this check
not before.

I'm rejecting the queue request directly here as HIDMA channel is no
longer functional. I'd rather do active error handling rather than
passive error handling. Debugging of passive faults are much more difficult.

> if hypervisor has paused you, so the entire
> txn queue is paused. But from API semantics I would argue that this should be
> accepted and suffer the same fate as already submitted txns
> 



>>
>>>
>>>
 +static int hidma_alloc_chan_resources(struct dma_chan *dmach)
 +{
 +  struct hidma_chan *mchan = to_hidma_chan(dmach);
 +  struct hidma_dev *dmadev = mchan->dmadev;
 +  struct hidma_desc *mdesc, *tmp;
 +  unsigned long irqflags;
 +  LIST_HEAD(descs);
 +  unsigned int i;
 +  int rc = 0;
 +
 +  if (mchan->allocated)
 +  return 0;
 +
 +  /* Alloc descriptors for this channel */
 +  for (i = 0; i < dmadev->nr_descriptors; i++) {
 +  mdesc = kzalloc(sizeof(struct hidma_desc), GFP_KERNEL);
>>>
>>> GFP_NOWAIT pls, this can be called from atomic context
>>
>> I'll change it, but why would anyone allocate a channel in an interrupt
>> handler?
> 
> remember this is dmaengine, where people traditionally wanted to offload
> from cpu and were not allowed to sleep.
> 
> I think am okay with this one..
> 
>>
>>>
 +  if (!mdesc) {
 +  rc = -ENOMEM;
 +  break;
 +  }
 +  dma_async_tx_descriptor_init(>desc, dmach);
 +  mdesc->desc.flags = DMA_CTRL_ACK;
>>>
>>> what is the purpose of setting this flag here
>>
>> It means that the code only supports interrupt. Maybe, you can correct
>> me here. I couldn't see a very useful info about the DMA engine flags.
>> My understanding is that DMA_CTRL_ACK means user wants an interrupt
>> based callback.
> 
> Right user wants, you are not user though
> 

Fair enough.

>>
>> If DMA_CTRL_ACK is not specified, then user wants to poll for the
>> results. The current code only supports the interrupt based delivery for
>> callbacks. Polling support is another to-do in my list for small sized
>> transfers.
> 
> See the documentation for flags and usage. The point here is that user
> should set this. I know some old driver do this but that is not right
> 

I'll remove it.

>>
>>>
>>>
 +static int hidma_remove(struct platform_device *pdev)
 +{
 +  struct

Re: [PATCH V7 3/3] dma: add Qualcomm Technologies HIDMA channel driver

2015-12-01 Thread Vinod Koul 
On Mon, Nov 30, 2015 at 03:06:36PM -0500, Sinan Kaya wrote:
> I have split the debugfs support from this patch to its own patch. Any
> other idea on how else you'd break the code? I can take one more step
> and separate the lower layer from the OS layer by using stub functions
> on the initial commit.

Yes the ll.c can be a separate patch and no need to add placeholders as
makefile can be last

> > 
> >> +static enum dma_status hidma_tx_status(struct dma_chan *dmach,
> >> +  dma_cookie_t cookie, struct dma_tx_state *txstate)
> >> +{
> >> +  struct hidma_chan *mchan = to_hidma_chan(dmach);
> >> +  enum dma_status ret;
> >> +
> >> +  if (mchan->paused)
> >> +  ret = DMA_PAUSED;
> > 
> > This is not quite right. The status query is for a descriptor and NOT for
> > channel. Here if the descriptor queried was running then it would be paused
> > for the rest pending txn, it would be DMA_IN_PROGRESS.
> 
> The channel can be paused by the hypervisor. If it is paused, then no
> other transaction will go through including the pending requests. That's
> why, I'm checking the state first.

You are missing the point. Channel can be paused, yes but the descriptor
is in queue and is not paused. The descriptor running is paused, yes.
There is subtle difference between these

> >> +static dma_cookie_t hidma_tx_submit(struct dma_async_tx_descriptor *txd)
> >> +{
> >> +  struct hidma_chan *mchan = to_hidma_chan(txd->chan);
> >> +  struct hidma_dev *dmadev = mchan->dmadev;
> >> +  struct hidma_desc *mdesc;
> >> +  unsigned long irqflags;
> >> +  dma_cookie_t cookie;
> >> +
> >> +  if (!hidma_ll_isenabled(dmadev->lldev))
> >> +  return -ENODEV;
> > 
> > is this not too late for this check, you should fail this on prepare
> > method
> 
> The channels can be paused by the hypervisor at runtime after the guest
> OS boot. The client might have allocated the channels during guest
> machine boot. If a channel is paused and client makes a request, client
> will never get the callback. By placing this check, I'm looking at the
> runtime status of the channel and rejecting the transmit request right
> ahead.

In this case you have request already given to you, here the request is
submitted to the pending queue, if hypervisor has paused you, so the entire
txn queue is paused. But from API semantics I would argue that this should be
accepted and suffer the same fate as already submitted txns

> 
> > 
> > 
> >> +static int hidma_alloc_chan_resources(struct dma_chan *dmach)
> >> +{
> >> +  struct hidma_chan *mchan = to_hidma_chan(dmach);
> >> +  struct hidma_dev *dmadev = mchan->dmadev;
> >> +  struct hidma_desc *mdesc, *tmp;
> >> +  unsigned long irqflags;
> >> +  LIST_HEAD(descs);
> >> +  unsigned int i;
> >> +  int rc = 0;
> >> +
> >> +  if (mchan->allocated)
> >> +  return 0;
> >> +
> >> +  /* Alloc descriptors for this channel */
> >> +  for (i = 0; i < dmadev->nr_descriptors; i++) {
> >> +  mdesc = kzalloc(sizeof(struct hidma_desc), GFP_KERNEL);
> > 
> > GFP_NOWAIT pls, this can be called from atomic context
> 
> I'll change it, but why would anyone allocate a channel in an interrupt
> handler?

remember this is dmaengine, where people traditionally wanted to offload
from cpu and were not allowed to sleep.

I think am okay with this one..

> 
> > 
> >> +  if (!mdesc) {
> >> +  rc = -ENOMEM;
> >> +  break;
> >> +  }
> >> +  dma_async_tx_descriptor_init(>desc, dmach);
> >> +  mdesc->desc.flags = DMA_CTRL_ACK;
> > 
> > what is the purpose of setting this flag here
> 
> It means that the code only supports interrupt. Maybe, you can correct
> me here. I couldn't see a very useful info about the DMA engine flags.
> My understanding is that DMA_CTRL_ACK means user wants an interrupt
> based callback.

Right user wants, you are not user though

> 
> If DMA_CTRL_ACK is not specified, then user wants to poll for the
> results. The current code only supports the interrupt based delivery for
> callbacks. Polling support is another to-do in my list for small sized
> transfers.

See the documentation for flags and usage. The point here is that user
should set this. I know some old driver do this but that is not right

> 
> > 
> >> +static void hidma_free_chan_resources(struct dma_chan *dmach)
> >> +{
> >> +  struct hidma_chan *mchan = to_hidma_chan(dmach);
> >> +  struct hidma_dev *mdma = mchan->dmadev;
> >> +  struct hidma_desc *mdesc, *tmp;
> >> +  unsigned long irqflags;
> >> +  LIST_HEAD(descs);
> >> +
> >> +  if (!list_empty(>prepared) || !list_empty(>active) ||
> >> +  !list_empty(>completed)) {
> >> +  /*
> >> +   * We have unfinished requests waiting.
> >> +   * Terminate the request from the hardware.
> > 
> > that sounds as bug.. free should be called when txn are completed/aborted
> 
> Let me see what other drivers are doing...
> 
> > 
> >> +   */
> >> +

Re: [PATCH V7 3/3] dma: add Qualcomm Technologies HIDMA channel driver

2015-11-30 Thread Sinan Kaya 
On 11/30/2015 3:59 AM, Vinod Koul wrote:
> On Sun, Nov 22, 2015 at 09:28:25PM -0500, Sinan Kaya wrote:
>> This patch adds support for hidma engine. The driver consists
>> of two logical blocks. The DMA engine interface and the
>> low-level interface. The hardware only supports memcpy/memset
>> and this driver only support memcpy interface. HW and driver
>> doesn't support slave interface.
>>
>> Signed-off-by: Sinan Kaya 
>> ---
>>  .../devicetree/bindings/dma/qcom_hidma.txt |  18 +
>>  drivers/dma/qcom/Kconfig   |  10 +
>>  drivers/dma/qcom/Makefile  |   2 +
>>  drivers/dma/qcom/hidma.c   | 706 
>>  drivers/dma/qcom/hidma.h   | 157 
>>  drivers/dma/qcom/hidma_dbg.c   | 217 +
>>  drivers/dma/qcom/hidma_ll.c| 924 
>> +
>>  7 files changed, 2034 insertions(+)
> 
> This was one of the reasons for slow review of this. I started few times but
> large patches made this getting pushed back
> 
> Pls help reviewers by splitting your code which looking at this could have
> been done

I have split the debugfs support from this patch to its own patch. Any
other idea on how else you'd break the code? I can take one more step
and separate the lower layer from the OS layer by using stub functions
on the initial commit.

> 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
> 
> Do you need so many headers...?

probably not, let me see what I can do about them.

> 
>> +MODULE_PARM_DESC(event_channel_idx,
>> +"event channel index array for the notifications");
> 
> Can you explain this parameter in detail pls

OK. I added the following comment to the code.

/*
 * Each DMA channel is associated with an event channel for interrupt
 * delivery. The event channel index usually comes from the firmware through
 * ACPI/DT. When a HIDMA channel is executed in the guest machine
context (QEMU)
 * the device tree gets auto-generated based on the memory and IRQ resources
 * this driver uses on the host machine. Any device specific parameter
such as
 * event-channel gets ignored by the QEMU.
 * We are using this command line parameter to pass the event channel
index to
 * the guest machine.
 */
s

> 
>> +static void hidma_callback(void *data)
>> +{
>> +struct hidma_desc *mdesc = data;
>> +struct hidma_chan *mchan = to_hidma_chan(mdesc->desc.chan);
>> +struct dma_device *ddev = mchan->chan.device;
>> +struct hidma_dev *dmadev = to_hidma_dev(ddev);
>> +unsigned long irqflags;
>> +bool queued = false;
>> +
>> +spin_lock_irqsave(>lock, irqflags);
>> +if (mdesc->node.next) {
>> +/* Delete from the active list, add to completed list */
>> +list_move_tail(>node, >completed);
>> +queued = true;
>> +}
>> +spin_unlock_irqrestore(>lock, irqflags);
>> +
>> +hidma_process_completed(dmadev);
>> +
>> +if (queued) {
>> +pm_runtime_mark_last_busy(dmadev->ddev.dev);
>> +pm_runtime_put_autosuspend(dmadev->ddev.dev);
>> +}
>> +}
> 
> Callback is typically referred to client callback upon dma completion, can
> you explain what you are trying to do here

When a DMA transfer completes, the hidma_callback function in the
hidma.c file gets called from the lower layer (hidma_ll.c) in order to
move this request from active queue into the completed queue.

hidma_process_completed eventually calls the "client callback" in it.

The PM stuff is for guaranteeing that clocks are not turned off while HW
is working on it.

I grab the PM lock in issue pending and release it in the hidma_callback.

> 
>> +static int hidma_chan_init(struct hidma_dev *dmadev, u32 dma_sig)
>> +{
>> +struct hidma_chan *mchan;
>> +struct dma_device *ddev;
>> +
>> +mchan = devm_kzalloc(dmadev->ddev.dev, sizeof(*mchan), GFP_KERNEL);
>> +if (!mchan)
>> +return -ENOMEM;
>> +
>> +ddev = >ddev;
>> +mchan->dma_sig = dma_sig;
>> +mchan->dmadev = dmadev;
>> +mchan->chan.device = ddev;
>> +dma_cookie_init(>chan);
>> +
>> +INIT_LIST_HEAD(>free);
>> +INIT_LIST_HEAD(>prepared);
>> +INIT_LIST_HEAD(>active);
>> +INIT_LIST_HEAD(>completed);
>> +
>> +spin_lock_init(>lock);
>> +list_add_tail(>chan.device_node, >channels);
>> +dmadev->ddev.chancnt++;
> 
> Have you looked at vchan implementation and moving list handling to this
> layer ?

I'll add vchan support later on another revision. The original code had
vchan support in it. It became a terminology mess. So, I removed it for
the moment.

Using vchan, I can take multiple clients and serve them in a single
channel

Re: [PATCH V7 3/3] dma: add Qualcomm Technologies HIDMA channel driver

2015-11-30 Thread Vinod Koul 
On Sun, Nov 22, 2015 at 09:28:25PM -0500, Sinan Kaya wrote:
> This patch adds support for hidma engine. The driver consists
> of two logical blocks. The DMA engine interface and the
> low-level interface. The hardware only supports memcpy/memset
> and this driver only support memcpy interface. HW and driver
> doesn't support slave interface.
> 
> Signed-off-by: Sinan Kaya 
> ---
>  .../devicetree/bindings/dma/qcom_hidma.txt |  18 +
>  drivers/dma/qcom/Kconfig   |  10 +
>  drivers/dma/qcom/Makefile  |   2 +
>  drivers/dma/qcom/hidma.c   | 706 
>  drivers/dma/qcom/hidma.h   | 157 
>  drivers/dma/qcom/hidma_dbg.c   | 217 +
>  drivers/dma/qcom/hidma_ll.c| 924 
> +
>  7 files changed, 2034 insertions(+)

This was one of the reasons for slow review of this. I started few times but
large patches made this getting pushed back

Pls help reviewers by splitting your code which looking at this could have
been done

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

Do you need so many headers...?

> +MODULE_PARM_DESC(event_channel_idx,
> + "event channel index array for the notifications");

Can you explain this parameter in detail pls

> +static void hidma_callback(void *data)
> +{
> + struct hidma_desc *mdesc = data;
> + struct hidma_chan *mchan = to_hidma_chan(mdesc->desc.chan);
> + struct dma_device *ddev = mchan->chan.device;
> + struct hidma_dev *dmadev = to_hidma_dev(ddev);
> + unsigned long irqflags;
> + bool queued = false;
> +
> + spin_lock_irqsave(>lock, irqflags);
> + if (mdesc->node.next) {
> + /* Delete from the active list, add to completed list */
> + list_move_tail(>node, >completed);
> + queued = true;
> + }
> + spin_unlock_irqrestore(>lock, irqflags);
> +
> + hidma_process_completed(dmadev);
> +
> + if (queued) {
> + pm_runtime_mark_last_busy(dmadev->ddev.dev);
> + pm_runtime_put_autosuspend(dmadev->ddev.dev);
> + }
> +}

Callback is typically referred to client callback upon dma completion, can
you explain what you are trying to do here

> +static int hidma_chan_init(struct hidma_dev *dmadev, u32 dma_sig)
> +{
> + struct hidma_chan *mchan;
> + struct dma_device *ddev;
> +
> + mchan = devm_kzalloc(dmadev->ddev.dev, sizeof(*mchan), GFP_KERNEL);
> + if (!mchan)
> + return -ENOMEM;
> +
> + ddev = >ddev;
> + mchan->dma_sig = dma_sig;
> + mchan->dmadev = dmadev;
> + mchan->chan.device = ddev;
> + dma_cookie_init(>chan);
> +
> + INIT_LIST_HEAD(>free);
> + INIT_LIST_HEAD(>prepared);
> + INIT_LIST_HEAD(>active);
> + INIT_LIST_HEAD(>completed);
> +
> + spin_lock_init(>lock);
> + list_add_tail(>chan.device_node, >channels);
> + dmadev->ddev.chancnt++;

Have you looked at vchan implementation and moving list handling to this
layer ?

> + return 0;
> +}
> +
> +static void hidma_issue_pending(struct dma_chan *dmach)
> +{
> + struct hidma_chan *mchan = to_hidma_chan(dmach);
> + struct hidma_dev *dmadev = mchan->dmadev;
> +
> + /* PM will be released in hidma_callback function. */
> + pm_runtime_get_sync(dmadev->ddev.dev);

Oh no, this is not allowed. pm_runtime_get_sync() can sleep and
issue_pending can be invoked from atomic context.

> +static enum dma_status hidma_tx_status(struct dma_chan *dmach,
> + dma_cookie_t cookie, struct dma_tx_state *txstate)
> +{
> + struct hidma_chan *mchan = to_hidma_chan(dmach);
> + enum dma_status ret;
> +
> + if (mchan->paused)
> + ret = DMA_PAUSED;

This is not quite right. The status query is for a descriptor and NOT for
channel. Here if the descriptor queried was running then it would be paused
for the rest pending txn, it would be DMA_IN_PROGRESS.

> +static dma_cookie_t hidma_tx_submit(struct dma_async_tx_descriptor *txd)
> +{
> + struct hidma_chan *mchan = to_hidma_chan(txd->chan);
> + struct hidma_dev *dmadev = mchan->dmadev;
> + struct hidma_desc *mdesc;
> + unsigned long irqflags;
> + dma_cookie_t cookie;
> +
> + if (!hidma_ll_isenabled(dmadev->lldev))
> + return -ENODEV;

is this not too late for this check, you should fail this on prepare
method


> +static int hidma_alloc_chan_resources(struct dma_chan *dmach)
> +{
> + struct hidma_chan *mchan = to_hidma_chan(dmach);
> + struct hidma_dev *dmadev = mchan->dmadev;
> + struct hidma_desc *mdesc, *tmp;
> + unsigned long irqflags;
> + LIST_HEAD(descs);
> + unsigned int i;
> +

[PATCH V7 3/3] dma: add Qualcomm Technologies HIDMA channel driver

2015-11-22 Thread Sinan Kaya 
This patch adds support for hidma engine. The driver consists
of two logical blocks. The DMA engine interface and the
low-level interface. The hardware only supports memcpy/memset
and this driver only support memcpy interface. HW and driver
doesn't support slave interface.

Signed-off-by: Sinan Kaya 
---
 .../devicetree/bindings/dma/qcom_hidma.txt |  18 +
 drivers/dma/qcom/Kconfig   |  10 +
 drivers/dma/qcom/Makefile  |   2 +
 drivers/dma/qcom/hidma.c   | 706 
 drivers/dma/qcom/hidma.h   | 157 
 drivers/dma/qcom/hidma_dbg.c   | 217 +
 drivers/dma/qcom/hidma_ll.c| 924 +
 7 files changed, 2034 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/qcom_hidma.txt
 create mode 100644 drivers/dma/qcom/hidma.c
 create mode 100644 drivers/dma/qcom/hidma.h
 create mode 100644 drivers/dma/qcom/hidma_dbg.c
 create mode 100644 drivers/dma/qcom/hidma_ll.c

diff --git a/Documentation/devicetree/bindings/dma/qcom_hidma.txt 
b/Documentation/devicetree/bindings/dma/qcom_hidma.txt
new file mode 100644
index 000..d18c8fc
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/qcom_hidma.txt
@@ -0,0 +1,18 @@
+Qualcomm Technologies HIDMA Channel driver
+
+Required properties:
+- compatible: must contain "qcom,hidma-1.0"
+- reg: Addresses for the transfer and event channel
+- interrupts: Should contain the event interrupt
+- desc-count: Number of asynchronous requests this channel can handle
+- event-channel: The HW event channel completions will be delivered.
+Example:
+
+   hidma_24: dma-controller@0x5c05 {
+   compatible = "qcom,hidma-1.0";
+   reg = <0 0x5c05 0x0 0x1000>,
+ <0 0x5c0b 0x0 0x1000>;
+   interrupts = <0 389 0>;
+   desc-count = <10>;
+   event-channel = <4>;
+   };
diff --git a/drivers/dma/qcom/Kconfig b/drivers/dma/qcom/Kconfig
index ebe5b8c..5588e1c 100644
--- a/drivers/dma/qcom/Kconfig
+++ b/drivers/dma/qcom/Kconfig
@@ -17,3 +17,13 @@ config QCOM_HIDMA_MGMT
  start managing the channels. In a virtualized environment,
  the guest OS would run QCOM_HIDMA channel driver and the
  hypervisor would run the QCOM_HIDMA_MGMT management driver.
+
+config QCOM_HIDMA
+   tristate "Qualcomm Technologies HIDMA Channel support"
+   select DMA_ENGINE
+   help
+ Enable support for the Qualcomm Technologies HIDMA controller.
+ The HIDMA controller supports optimized buffer copies
+ (user to kernel, kernel to kernel, etc.).  It only supports
+ memcpy interface. The core is not intended for general
+ purpose slave DMA.
diff --git a/drivers/dma/qcom/Makefile b/drivers/dma/qcom/Makefile
index 1a5a96d..f702df1 100644
--- a/drivers/dma/qcom/Makefile
+++ b/drivers/dma/qcom/Makefile
@@ -1,2 +1,4 @@
 obj-$(CONFIG_QCOM_BAM_DMA) += bam_dma.o
 obj-$(CONFIG_QCOM_HIDMA_MGMT) += hidma_mgmt.o hidma_mgmt_sys.o
+obj-$(CONFIG_QCOM_HIDMA) +=  hdma.o
+hdma-objs:= hidma_ll.o hidma.o hidma_dbg.o
diff --git a/drivers/dma/qcom/hidma.c b/drivers/dma/qcom/hidma.c
new file mode 100644
index 000..c0c7f44
--- /dev/null
+++ b/drivers/dma/qcom/hidma.c
@@ -0,0 +1,706 @@
+/*
+ * Qualcomm Technologies HIDMA DMA engine interface
+ *
+ * Copyright (c) 2015, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+/*
+ * Copyright (C) Freescale Semicondutor, Inc. 2007, 2008.
+ * Copyright (C) Semihalf 2009
+ * Copyright (C) Ilya Yanok, Emcraft Systems 2010
+ * Copyright (C) Alexander Popov, Promcontroller 2014
+ *
+ * Written by Piotr Ziecik . Hardware description
+ * (defines, structures and comments) was taken from MPC5121 DMA driver
+ * written by Hongjun Chen .
+ *
+ * Approved as OSADL project by a majority of OSADL members and funded
+ * by OSADL membership fees in 2009;  for details see www.osadl.org.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A