Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level hardware interface

2016-04-30 Thread Sinan Kaya 
On 4/28/2016 3:30 PM, Sinan Kaya wrote:
> On 4/26/2016 12:24 PM, Vinod Koul wrote:
>>> +
 +   dev_err(lldev->dev, "error 0x%x, resetting...\n",
 +   cause);
>> right justify this and others as well please
>>
> 
> Can you please point me to other lines that need to be fixed please? It looks 
> good
> to me though it doesn't to you. I want to make sure that I'm touching the 
> right ones.
> You seem to prefer a tab following an open parenthesis in your code. Do you 
> want me to
> add a tab for every single multi-line such that it comes this for instance?
> 
> 
>   ret = readl_poll_timeout(lldev->trca + HIDMA_TRCA_CTRLSTS_REG, val,
>   HIDMA_CH_STATE(val) == 
> HIDMA_CH_DISABLED, 
>   1000, 1); 
> 
> instead of
> 
>   ret = readl_poll_timeout(lldev->trca + HIDMA_TRCA_CTRLSTS_REG, val,
>HIDMA_CH_STATE(val) == HIDMA_CH_DISABLED, 1000,
>1); 
> 
> 
 +int hidma_ll_resume(struct hidma_lldev *lldev)
 +{
 +  return hidma_ll_enable(lldev);
 +}
>>
>> why do we need this empty function, use hidma_ll_enable.

 hidma_ll_enable is a common function that gets called from multiple 
 places. 
 hidma_ll_resume and hidma_ll_pause is used by the OS interface for pausing
 and resuming the DMA channel. 
>> is there a reason why we can't have the code in resume and that being called
>> internally as well as externally?
>>
> 
> I'll have the pause code in OS layer call hidma_ll_enable and get rid of pause
> function here.
> 


I posted v18 along with all the style corrections and other things you asked me.
Please let me know if there is anything outstanding.



-- 
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

Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level hardware interface

2016-04-30 Thread Sinan Kaya 
On 4/28/2016 3:30 PM, Sinan Kaya wrote:
> On 4/26/2016 12:24 PM, Vinod Koul wrote:
>>> +
 +   dev_err(lldev->dev, "error 0x%x, resetting...\n",
 +   cause);
>> right justify this and others as well please
>>
> 
> Can you please point me to other lines that need to be fixed please? It looks 
> good
> to me though it doesn't to you. I want to make sure that I'm touching the 
> right ones.
> You seem to prefer a tab following an open parenthesis in your code. Do you 
> want me to
> add a tab for every single multi-line such that it comes this for instance?
> 
> 
>   ret = readl_poll_timeout(lldev->trca + HIDMA_TRCA_CTRLSTS_REG, val,
>   HIDMA_CH_STATE(val) == 
> HIDMA_CH_DISABLED, 
>   1000, 1); 
> 
> instead of
> 
>   ret = readl_poll_timeout(lldev->trca + HIDMA_TRCA_CTRLSTS_REG, val,
>HIDMA_CH_STATE(val) == HIDMA_CH_DISABLED, 1000,
>1); 
> 
> 
 +int hidma_ll_resume(struct hidma_lldev *lldev)
 +{
 +  return hidma_ll_enable(lldev);
 +}
>>
>> why do we need this empty function, use hidma_ll_enable.

 hidma_ll_enable is a common function that gets called from multiple 
 places. 
 hidma_ll_resume and hidma_ll_pause is used by the OS interface for pausing
 and resuming the DMA channel. 
>> is there a reason why we can't have the code in resume and that being called
>> internally as well as externally?
>>
> 
> I'll have the pause code in OS layer call hidma_ll_enable and get rid of pause
> function here.
> 


I posted v18 along with all the style corrections and other things you asked me.
Please let me know if there is anything outstanding.



-- 
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

Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level hardware interface

2016-04-28 Thread Sinan Kaya 
On 4/26/2016 12:24 PM, Vinod Koul wrote:
>> +
>> > +   dev_err(lldev->dev, "error 0x%x, resetting...\n",
>> > +   cause);
> right justify this and others as well please
> 

Can you please point me to other lines that need to be fixed please? It looks 
good
to me though it doesn't to you. I want to make sure that I'm touching the right 
ones.
You seem to prefer a tab following an open parenthesis in your code. Do you 
want me to
add a tab for every single multi-line such that it comes this for instance?


ret = readl_poll_timeout(lldev->trca + HIDMA_TRCA_CTRLSTS_REG, val,
HIDMA_CH_STATE(val) == 
HIDMA_CH_DISABLED, 
1000, 1); 

instead of

ret = readl_poll_timeout(lldev->trca + HIDMA_TRCA_CTRLSTS_REG, val,
 HIDMA_CH_STATE(val) == HIDMA_CH_DISABLED, 1000,
 1); 


 > >> +int hidma_ll_resume(struct hidma_lldev *lldev)
 > >> +{
 > >> + return hidma_ll_enable(lldev);
 > >> +}
>>> > > 
>>> > > why do we need this empty function, use hidma_ll_enable.
>> > 
>> > hidma_ll_enable is a common function that gets called from multiple 
>> > places. 
>> > hidma_ll_resume and hidma_ll_pause is used by the OS interface for pausing
>> > and resuming the DMA channel. 
> is there a reason why we can't have the code in resume and that being called
> internally as well as externally?
> 

I'll have the pause code in OS layer call hidma_ll_enable and get rid of pause
function here.

-- 
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

Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level hardware interface

2016-04-28 Thread Sinan Kaya 
On 4/26/2016 12:24 PM, Vinod Koul wrote:
>> +
>> > +   dev_err(lldev->dev, "error 0x%x, resetting...\n",
>> > +   cause);
> right justify this and others as well please
> 

Can you please point me to other lines that need to be fixed please? It looks 
good
to me though it doesn't to you. I want to make sure that I'm touching the right 
ones.
You seem to prefer a tab following an open parenthesis in your code. Do you 
want me to
add a tab for every single multi-line such that it comes this for instance?


ret = readl_poll_timeout(lldev->trca + HIDMA_TRCA_CTRLSTS_REG, val,
HIDMA_CH_STATE(val) == 
HIDMA_CH_DISABLED, 
1000, 1); 

instead of

ret = readl_poll_timeout(lldev->trca + HIDMA_TRCA_CTRLSTS_REG, val,
 HIDMA_CH_STATE(val) == HIDMA_CH_DISABLED, 1000,
 1); 


 > >> +int hidma_ll_resume(struct hidma_lldev *lldev)
 > >> +{
 > >> + return hidma_ll_enable(lldev);
 > >> +}
>>> > > 
>>> > > why do we need this empty function, use hidma_ll_enable.
>> > 
>> > hidma_ll_enable is a common function that gets called from multiple 
>> > places. 
>> > hidma_ll_resume and hidma_ll_pause is used by the OS interface for pausing
>> > and resuming the DMA channel. 
> is there a reason why we can't have the code in resume and that being called
> internally as well as externally?
> 

I'll have the pause code in OS layer call hidma_ll_enable and get rid of pause
function here.

-- 
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

Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level hardware interface

2016-04-26 Thread Vinod Koul 
On Tue, Apr 26, 2016 at 11:04:55AM -0400, Sinan Kaya wrote:
> On 4/25/2016 11:28 PM, Vinod Koul wrote:
> >> +
> >> +  /* reset the channel for recovery */
> >> +  if (hidma_ll_setup(lldev)) {
> > 
> > should this be done in ISR?
> 
> I created a new tasklet called rst_task and posted the code there. 

sounds better

> 
>  /*
> + * Abort all transactions and perform a reset.
> + */
> +static void hidma_ll_abort(unsigned long arg)
> +{
> +   u8 err_code = HIDMA_EVRE_STATUS_ERROR;
> +   u8 err_info = 0xFF;
> +
> +   dev_err(lldev->dev, "error 0x%x, resetting...\n",
> +   cause);

right justify this and others as well please

> >> +int hidma_ll_resume(struct hidma_lldev *lldev)
> >> +{
> >> +  return hidma_ll_enable(lldev);
> >> +}
> > 
> > why do we need this empty function, use hidma_ll_enable.
> 
> hidma_ll_enable is a common function that gets called from multiple places. 
> hidma_ll_resume and hidma_ll_pause is used by the OS interface for pausing
> and resuming the DMA channel. 

is there a reason why we can't have the code in resume and that being called
internally as well as externally?

> >> +/*
> >> + * Note that even though we stop this channel
> >> + * if there is a pending transaction in flight
> >> + * it will complete and follow the callback.
> >> + * This request will prevent further requests
> >> + * to be made.
> > 
> > Why the odd formating?
> 
> aligned to 75 characters.

This seems to be 50 chars!

-- 
~Vinod

Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level hardware interface

2016-04-26 Thread Vinod Koul 
On Tue, Apr 26, 2016 at 11:04:55AM -0400, Sinan Kaya wrote:
> On 4/25/2016 11:28 PM, Vinod Koul wrote:
> >> +
> >> +  /* reset the channel for recovery */
> >> +  if (hidma_ll_setup(lldev)) {
> > 
> > should this be done in ISR?
> 
> I created a new tasklet called rst_task and posted the code there. 

sounds better

> 
>  /*
> + * Abort all transactions and perform a reset.
> + */
> +static void hidma_ll_abort(unsigned long arg)
> +{
> +   u8 err_code = HIDMA_EVRE_STATUS_ERROR;
> +   u8 err_info = 0xFF;
> +
> +   dev_err(lldev->dev, "error 0x%x, resetting...\n",
> +   cause);

right justify this and others as well please

> >> +int hidma_ll_resume(struct hidma_lldev *lldev)
> >> +{
> >> +  return hidma_ll_enable(lldev);
> >> +}
> > 
> > why do we need this empty function, use hidma_ll_enable.
> 
> hidma_ll_enable is a common function that gets called from multiple places. 
> hidma_ll_resume and hidma_ll_pause is used by the OS interface for pausing
> and resuming the DMA channel. 

is there a reason why we can't have the code in resume and that being called
internally as well as externally?

> >> +/*
> >> + * Note that even though we stop this channel
> >> + * if there is a pending transaction in flight
> >> + * it will complete and follow the callback.
> >> + * This request will prevent further requests
> >> + * to be made.
> > 
> > Why the odd formating?
> 
> aligned to 75 characters.

This seems to be 50 chars!

-- 
~Vinod

Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level hardware interface

2016-04-26 Thread Sinan Kaya 
On 4/26/2016 11:10 AM, Andy Shevchenko wrote:
> On Tue, Apr 26, 2016 at 6:04 PM, Sinan Kaya  wrote:
>> On 4/25/2016 11:28 PM, Vinod Koul wrote:
>>> On Mon, Apr 11, 2016 at 10:21:11AM -0400, Sinan Kaya wrote:
> 
 +while (cause) {
 +if ((cause & BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS)) ||
 +(cause & BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)) ||
 +(cause & BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS)) ||
 +(cause & BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS)) ||
 +(cause & BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS))) {
>>>
>>> Switch please
>>
>> Cause is a combined status register. Let's say it contains 0x41. I need to 
>> check
>> if bit 0 or bit 6 is set in this value for each case condition. The value is 
>> not 0x40
>> or 0x1.
>>
>> I created macro like this instead.
>>
>> +#define HIDMA_IS_ERR_INTERRUPT(cause)  \
>> +   (cause & BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS))   || \
>> +   (cause & BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)) || \
>> +   (cause & BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS))   || \
>> +   (cause & BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS))|| \
>> +   (cause & BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS))
> 
> This looks overheaded.
> 
> #define HIDMA_XXX (BIT(a) | BIT (b) ... BIT(n))
> 
>>
>> and replaced the if statement as follows
>>
>> if (HIDMA_IS_ERR_INTERRUPT(cause)) {
> 
> if (cause & HIDMA_XXX) {
> 

This is even better.

-- 
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

Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level hardware interface

2016-04-26 Thread Sinan Kaya 
On 4/26/2016 11:10 AM, Andy Shevchenko wrote:
> On Tue, Apr 26, 2016 at 6:04 PM, Sinan Kaya  wrote:
>> On 4/25/2016 11:28 PM, Vinod Koul wrote:
>>> On Mon, Apr 11, 2016 at 10:21:11AM -0400, Sinan Kaya wrote:
> 
 +while (cause) {
 +if ((cause & BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS)) ||
 +(cause & BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)) ||
 +(cause & BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS)) ||
 +(cause & BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS)) ||
 +(cause & BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS))) {
>>>
>>> Switch please
>>
>> Cause is a combined status register. Let's say it contains 0x41. I need to 
>> check
>> if bit 0 or bit 6 is set in this value for each case condition. The value is 
>> not 0x40
>> or 0x1.
>>
>> I created macro like this instead.
>>
>> +#define HIDMA_IS_ERR_INTERRUPT(cause)  \
>> +   (cause & BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS))   || \
>> +   (cause & BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)) || \
>> +   (cause & BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS))   || \
>> +   (cause & BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS))|| \
>> +   (cause & BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS))
> 
> This looks overheaded.
> 
> #define HIDMA_XXX (BIT(a) | BIT (b) ... BIT(n))
> 
>>
>> and replaced the if statement as follows
>>
>> if (HIDMA_IS_ERR_INTERRUPT(cause)) {
> 
> if (cause & HIDMA_XXX) {
> 

This is even better.

-- 
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

Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level hardware interface

2016-04-26 Thread Andy Shevchenko 
On Tue, Apr 26, 2016 at 6:04 PM, Sinan Kaya  wrote:
> On 4/25/2016 11:28 PM, Vinod Koul wrote:
>> On Mon, Apr 11, 2016 at 10:21:11AM -0400, Sinan Kaya wrote:

>>> +while (cause) {
>>> +if ((cause & BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS)) ||
>>> +(cause & BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)) ||
>>> +(cause & BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS)) ||
>>> +(cause & BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS)) ||
>>> +(cause & BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS))) {
>>
>> Switch please
>
> Cause is a combined status register. Let's say it contains 0x41. I need to 
> check
> if bit 0 or bit 6 is set in this value for each case condition. The value is 
> not 0x40
> or 0x1.
>
> I created macro like this instead.
>
> +#define HIDMA_IS_ERR_INTERRUPT(cause)  \
> +   (cause & BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS))   || \
> +   (cause & BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)) || \
> +   (cause & BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS))   || \
> +   (cause & BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS))|| \
> +   (cause & BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS))

This looks overheaded.

#define HIDMA_XXX (BIT(a) | BIT (b) ... BIT(n))

>
> and replaced the if statement as follows
>
> if (HIDMA_IS_ERR_INTERRUPT(cause)) {

if (cause & HIDMA_XXX) {

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level hardware interface

2016-04-26 Thread Andy Shevchenko 
On Tue, Apr 26, 2016 at 6:04 PM, Sinan Kaya  wrote:
> On 4/25/2016 11:28 PM, Vinod Koul wrote:
>> On Mon, Apr 11, 2016 at 10:21:11AM -0400, Sinan Kaya wrote:

>>> +while (cause) {
>>> +if ((cause & BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS)) ||
>>> +(cause & BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)) ||
>>> +(cause & BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS)) ||
>>> +(cause & BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS)) ||
>>> +(cause & BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS))) {
>>
>> Switch please
>
> Cause is a combined status register. Let's say it contains 0x41. I need to 
> check
> if bit 0 or bit 6 is set in this value for each case condition. The value is 
> not 0x40
> or 0x1.
>
> I created macro like this instead.
>
> +#define HIDMA_IS_ERR_INTERRUPT(cause)  \
> +   (cause & BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS))   || \
> +   (cause & BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)) || \
> +   (cause & BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS))   || \
> +   (cause & BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS))|| \
> +   (cause & BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS))

This looks overheaded.

#define HIDMA_XXX (BIT(a) | BIT (b) ... BIT(n))

>
> and replaced the if statement as follows
>
> if (HIDMA_IS_ERR_INTERRUPT(cause)) {

if (cause & HIDMA_XXX) {

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level hardware interface

2016-04-26 Thread Sinan Kaya 
On 4/25/2016 11:28 PM, Vinod Koul wrote:
> On Mon, Apr 11, 2016 at 10:21:11AM -0400, Sinan Kaya wrote:
> 
>> + * HIDMA is not aware of IOMMU presence since it follows the DMA API. All
>> + * IOMMU latency will be built into the data movement time. By the time
>> + * interrupt happens, IOMMU lookups + data movement has already taken place.
> 
> Do you mean dmaengine API or dma mapping API here? Where is you IOMMU
> located wrt to dma controller?

I mean DMA API in this context. The code works on dma_addr_t. That means the 
caller
takes care of DMA mapping before coming to this code. This code is not involved 
in
any of the DMA mapping calls or any other IOMMU related operations. All of this 
is abstracted from this code.

The IOMMU is right in front of this device. That's why, all IOMMU operations 
are 
inclusive into the data transfers. 

> 
>> + *
>> + * While the first read in a typical PCI endpoint ISR flushes all 
>> outstanding
>> + * requests traditionally to the destination, this concept does not apply
>> + * here for this HW.
>> + */
>> +static void hidma_ll_int_handler_internal(struct hidma_lldev *lldev)
>> +{
>> +u32 status;
>> +u32 enable;
>> +u32 cause;
>> +
>> +/*
>> + * Fine tuned for this HW...
>> + *
>> + * This ISR has been designed for this particular hardware. Relaxed
>> + * read and write accessors are used for performance reasons due to
>> + * interrupt delivery guarantees. Do not copy this code blindly and
>> + * expect that to work.
>> + */
>> +status = readl_relaxed(lldev->evca + HIDMA_EVCA_IRQ_STAT_REG);
>> +enable = readl_relaxed(lldev->evca + HIDMA_EVCA_IRQ_EN_REG);
>> +cause = status & enable;
>> +
>> +while (cause) {
>> +if ((cause & BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS)) ||
>> +(cause & BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)) ||
>> +(cause & BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS)) ||
>> +(cause & BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS)) ||
>> +(cause & BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS))) {
> 
> Switch please

Cause is a combined status register. Let's say it contains 0x41. I need to check
if bit 0 or bit 6 is set in this value for each case condition. The value is 
not 0x40 
or 0x1. 

I created macro like this instead.

+#define HIDMA_IS_ERR_INTERRUPT(cause)  \
+   (cause & BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS))   || \
+   (cause & BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)) || \
+   (cause & BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS))   || \
+   (cause & BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS))|| \
+   (cause & BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS))

and replaced the if statement as follows

if (HIDMA_IS_ERR_INTERRUPT(cause)) {

> 
>> +u8 err_code = HIDMA_EVRE_STATUS_ERROR;
>> +u8 err_info = 0xFF;
>> +
>> +/* Clear out pending interrupts */
>> +writel(cause, lldev->evca + HIDMA_EVCA_IRQ_CLR_REG);
>> +
>> +dev_err(lldev->dev, "error 0x%x, resetting...\n",
>> +cause);
>> +
>> +hidma_cleanup_pending_tre(lldev, err_info, err_code);
>> +
>> +/* reset the channel for recovery */
>> +if (hidma_ll_setup(lldev)) {
> 
> should this be done in ISR?

I created a new tasklet called rst_task and posted the code there. 

 /*
+ * Abort all transactions and perform a reset.
+ */
+static void hidma_ll_abort(unsigned long arg)
+{
+   u8 err_code = HIDMA_EVRE_STATUS_ERROR;
+   u8 err_info = 0xFF;
+
+   dev_err(lldev->dev, "error 0x%x, resetting...\n",
+   cause);
+
+   hidma_cleanup_pending_tre(lldev, err_info, err_code);
+
+   /* reset the channel for recovery */
+   if (hidma_ll_setup(lldev)) {
+   dev_err(lldev->dev,
+   "channel reinitialize failed after error\n");
+   return;
+   }
+   hidma_ll_control_irq(lldev, ENABLE_IRQS);
+}

+   if (HIDMA_IS_ERR_INTERRUPT(cause)) {
/* Clear out pending interrupts */
writel(cause, lldev->evca + HIDMA_EVCA_IRQ_CLR_REG);

-   dev_err(lldev->dev, "error 0x%x, resetting...\n",
-   cause);
-
-   hidma_cleanup_pending_tre(lldev, err_info, err_code);
-
-   /* reset the channel for recovery */
-   if (hidma_ll_setup(lldev)) {
-   dev_err(lldev->dev,
-   "channel reinitialize failed after 
error\n");
-   return;
-   }
-   hidma_ll_enable_irq(lldev, ENABLE_IRQS);
+   tasklet_schedule(>rst_task);
 



> 
>> +int hidma_ll_resume(struct hidma_lldev *lldev)
>> +{
>> +return

Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level hardware interface

2016-04-26 Thread Sinan Kaya 
On 4/25/2016 11:28 PM, Vinod Koul wrote:
> On Mon, Apr 11, 2016 at 10:21:11AM -0400, Sinan Kaya wrote:
> 
>> + * HIDMA is not aware of IOMMU presence since it follows the DMA API. All
>> + * IOMMU latency will be built into the data movement time. By the time
>> + * interrupt happens, IOMMU lookups + data movement has already taken place.
> 
> Do you mean dmaengine API or dma mapping API here? Where is you IOMMU
> located wrt to dma controller?

I mean DMA API in this context. The code works on dma_addr_t. That means the 
caller
takes care of DMA mapping before coming to this code. This code is not involved 
in
any of the DMA mapping calls or any other IOMMU related operations. All of this 
is abstracted from this code.

The IOMMU is right in front of this device. That's why, all IOMMU operations 
are 
inclusive into the data transfers. 

> 
>> + *
>> + * While the first read in a typical PCI endpoint ISR flushes all 
>> outstanding
>> + * requests traditionally to the destination, this concept does not apply
>> + * here for this HW.
>> + */
>> +static void hidma_ll_int_handler_internal(struct hidma_lldev *lldev)
>> +{
>> +u32 status;
>> +u32 enable;
>> +u32 cause;
>> +
>> +/*
>> + * Fine tuned for this HW...
>> + *
>> + * This ISR has been designed for this particular hardware. Relaxed
>> + * read and write accessors are used for performance reasons due to
>> + * interrupt delivery guarantees. Do not copy this code blindly and
>> + * expect that to work.
>> + */
>> +status = readl_relaxed(lldev->evca + HIDMA_EVCA_IRQ_STAT_REG);
>> +enable = readl_relaxed(lldev->evca + HIDMA_EVCA_IRQ_EN_REG);
>> +cause = status & enable;
>> +
>> +while (cause) {
>> +if ((cause & BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS)) ||
>> +(cause & BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)) ||
>> +(cause & BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS)) ||
>> +(cause & BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS)) ||
>> +(cause & BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS))) {
> 
> Switch please

Cause is a combined status register. Let's say it contains 0x41. I need to check
if bit 0 or bit 6 is set in this value for each case condition. The value is 
not 0x40 
or 0x1. 

I created macro like this instead.

+#define HIDMA_IS_ERR_INTERRUPT(cause)  \
+   (cause & BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS))   || \
+   (cause & BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)) || \
+   (cause & BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS))   || \
+   (cause & BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS))|| \
+   (cause & BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS))

and replaced the if statement as follows

if (HIDMA_IS_ERR_INTERRUPT(cause)) {

> 
>> +u8 err_code = HIDMA_EVRE_STATUS_ERROR;
>> +u8 err_info = 0xFF;
>> +
>> +/* Clear out pending interrupts */
>> +writel(cause, lldev->evca + HIDMA_EVCA_IRQ_CLR_REG);
>> +
>> +dev_err(lldev->dev, "error 0x%x, resetting...\n",
>> +cause);
>> +
>> +hidma_cleanup_pending_tre(lldev, err_info, err_code);
>> +
>> +/* reset the channel for recovery */
>> +if (hidma_ll_setup(lldev)) {
> 
> should this be done in ISR?

I created a new tasklet called rst_task and posted the code there. 

 /*
+ * Abort all transactions and perform a reset.
+ */
+static void hidma_ll_abort(unsigned long arg)
+{
+   u8 err_code = HIDMA_EVRE_STATUS_ERROR;
+   u8 err_info = 0xFF;
+
+   dev_err(lldev->dev, "error 0x%x, resetting...\n",
+   cause);
+
+   hidma_cleanup_pending_tre(lldev, err_info, err_code);
+
+   /* reset the channel for recovery */
+   if (hidma_ll_setup(lldev)) {
+   dev_err(lldev->dev,
+   "channel reinitialize failed after error\n");
+   return;
+   }
+   hidma_ll_control_irq(lldev, ENABLE_IRQS);
+}

+   if (HIDMA_IS_ERR_INTERRUPT(cause)) {
/* Clear out pending interrupts */
writel(cause, lldev->evca + HIDMA_EVCA_IRQ_CLR_REG);

-   dev_err(lldev->dev, "error 0x%x, resetting...\n",
-   cause);
-
-   hidma_cleanup_pending_tre(lldev, err_info, err_code);
-
-   /* reset the channel for recovery */
-   if (hidma_ll_setup(lldev)) {
-   dev_err(lldev->dev,
-   "channel reinitialize failed after 
error\n");
-   return;
-   }
-   hidma_ll_enable_irq(lldev, ENABLE_IRQS);
+   tasklet_schedule(>rst_task);
 



> 
>> +int hidma_ll_resume(struct hidma_lldev *lldev)
>> +{
>> +return

Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level hardware interface

2016-04-25 Thread Vinod Koul 
On Mon, Apr 11, 2016 at 10:21:11AM -0400, Sinan Kaya wrote:

> + * HIDMA is not aware of IOMMU presence since it follows the DMA API. All
> + * IOMMU latency will be built into the data movement time. By the time
> + * interrupt happens, IOMMU lookups + data movement has already taken place.

Do you mean dmaengine API or dma mapping API here? Where is you IOMMU
located wrt to dma controller?

> + *
> + * While the first read in a typical PCI endpoint ISR flushes all outstanding
> + * requests traditionally to the destination, this concept does not apply
> + * here for this HW.
> + */
> +static void hidma_ll_int_handler_internal(struct hidma_lldev *lldev)
> +{
> + u32 status;
> + u32 enable;
> + u32 cause;
> +
> + /*
> +  * Fine tuned for this HW...
> +  *
> +  * This ISR has been designed for this particular hardware. Relaxed
> +  * read and write accessors are used for performance reasons due to
> +  * interrupt delivery guarantees. Do not copy this code blindly and
> +  * expect that to work.
> +  */
> + status = readl_relaxed(lldev->evca + HIDMA_EVCA_IRQ_STAT_REG);
> + enable = readl_relaxed(lldev->evca + HIDMA_EVCA_IRQ_EN_REG);
> + cause = status & enable;
> +
> + while (cause) {
> + if ((cause & BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS)) ||
> + (cause & BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)) ||
> + (cause & BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS)) ||
> + (cause & BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS)) ||
> + (cause & BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS))) {

Switch please

> + u8 err_code = HIDMA_EVRE_STATUS_ERROR;
> + u8 err_info = 0xFF;
> +
> + /* Clear out pending interrupts */
> + writel(cause, lldev->evca + HIDMA_EVCA_IRQ_CLR_REG);
> +
> + dev_err(lldev->dev, "error 0x%x, resetting...\n",
> + cause);
> +
> + hidma_cleanup_pending_tre(lldev, err_info, err_code);
> +
> + /* reset the channel for recovery */
> + if (hidma_ll_setup(lldev)) {

should this be done in ISR?

> +int hidma_ll_resume(struct hidma_lldev *lldev)
> +{
> + return hidma_ll_enable(lldev);
> +}

why do we need this empty function, use hidma_ll_enable.

> +bool hidma_ll_isenabled(struct hidma_lldev *lldev)
> +{
> + u32 val;
> +
> + val = readl(lldev->trca + HIDMA_TRCA_CTRLSTS_REG);
> + lldev->trch_state = HIDMA_CH_STATE(val);
> + val = readl(lldev->evca + HIDMA_EVCA_CTRLSTS_REG);
> + lldev->evch_state = HIDMA_CH_STATE(val);
> +
> + /* both channels have to be enabled before calling this function */
> + if (((lldev->trch_state == HIDMA_CH_ENABLED) ||
> +  (lldev->trch_state == HIDMA_CH_RUNNING)) &&
> + ((lldev->evch_state == HIDMA_CH_ENABLED) ||
> +  (lldev->evch_state == HIDMA_CH_RUNNING)))
> + return true;

hmmm this looks hard to read, why not do:

is_chan_enabled(state)
{
switch (state) {
case HIDMA_CH_ENABLED:
case HIDMA_CH_RUNNING:
return true;
default:
return false;
}

and then :

if (is_chan_enabled(lldev->trch_state) &&
is_chan_enabled(lldev->evch_state))


> +void hidma_ll_start(struct hidma_lldev *lldev)
> +{
> + hidma_ll_hw_start(lldev);
> +}

Another dummy :(

> +/*
> + * Note that even though we stop this channel
> + * if there is a pending transaction in flight
> + * it will complete and follow the callback.
> + * This request will prevent further requests
> + * to be made.

Why the odd formating?

> +int hidma_ll_uninit(struct hidma_lldev *lldev)
> +{
> + int rc = 0;
> + u32 val;
> +
> + if (!lldev)
> + return -ENODEV;
> +
> + if (lldev->initialized) {
> + u32 required_bytes;
> +
> + lldev->initialized = 0;
> +
> + required_bytes = sizeof(struct hidma_tre) * lldev->nr_tres;
> + tasklet_kill(>task);
> + memset(lldev->trepool, 0, required_bytes);
> + lldev->trepool = NULL;
> + lldev->pending_tre_count = 0;
> + lldev->tre_write_offset = 0;
> +
> + rc = hidma_ll_reset(lldev);
> +
> + /*
> +  * Clear all pending interrupts again.
> +  * Otherwise, we observe reset complete interrupts.
> +  */
> + val = readl(lldev->evca + HIDMA_EVCA_IRQ_STAT_REG);
> + writel(val, lldev->evca + HIDMA_EVCA_IRQ_CLR_REG);
> + hidma_ll_enable_irq(lldev, 0);

uninit enables irq?

-- 
~Vinod

Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level hardware interface

2016-04-25 Thread Vinod Koul 
On Mon, Apr 11, 2016 at 10:21:11AM -0400, Sinan Kaya wrote:

> + * HIDMA is not aware of IOMMU presence since it follows the DMA API. All
> + * IOMMU latency will be built into the data movement time. By the time
> + * interrupt happens, IOMMU lookups + data movement has already taken place.

Do you mean dmaengine API or dma mapping API here? Where is you IOMMU
located wrt to dma controller?

> + *
> + * While the first read in a typical PCI endpoint ISR flushes all outstanding
> + * requests traditionally to the destination, this concept does not apply
> + * here for this HW.
> + */
> +static void hidma_ll_int_handler_internal(struct hidma_lldev *lldev)
> +{
> + u32 status;
> + u32 enable;
> + u32 cause;
> +
> + /*
> +  * Fine tuned for this HW...
> +  *
> +  * This ISR has been designed for this particular hardware. Relaxed
> +  * read and write accessors are used for performance reasons due to
> +  * interrupt delivery guarantees. Do not copy this code blindly and
> +  * expect that to work.
> +  */
> + status = readl_relaxed(lldev->evca + HIDMA_EVCA_IRQ_STAT_REG);
> + enable = readl_relaxed(lldev->evca + HIDMA_EVCA_IRQ_EN_REG);
> + cause = status & enable;
> +
> + while (cause) {
> + if ((cause & BIT(HIDMA_IRQ_TR_CH_INVALID_TRE_BIT_POS)) ||
> + (cause & BIT(HIDMA_IRQ_TR_CH_TRE_RD_RSP_ER_BIT_POS)) ||
> + (cause & BIT(HIDMA_IRQ_EV_CH_WR_RESP_BIT_POS)) ||
> + (cause & BIT(HIDMA_IRQ_TR_CH_DATA_RD_ER_BIT_POS)) ||
> + (cause & BIT(HIDMA_IRQ_TR_CH_DATA_WR_ER_BIT_POS))) {

Switch please

> + u8 err_code = HIDMA_EVRE_STATUS_ERROR;
> + u8 err_info = 0xFF;
> +
> + /* Clear out pending interrupts */
> + writel(cause, lldev->evca + HIDMA_EVCA_IRQ_CLR_REG);
> +
> + dev_err(lldev->dev, "error 0x%x, resetting...\n",
> + cause);
> +
> + hidma_cleanup_pending_tre(lldev, err_info, err_code);
> +
> + /* reset the channel for recovery */
> + if (hidma_ll_setup(lldev)) {

should this be done in ISR?

> +int hidma_ll_resume(struct hidma_lldev *lldev)
> +{
> + return hidma_ll_enable(lldev);
> +}

why do we need this empty function, use hidma_ll_enable.

> +bool hidma_ll_isenabled(struct hidma_lldev *lldev)
> +{
> + u32 val;
> +
> + val = readl(lldev->trca + HIDMA_TRCA_CTRLSTS_REG);
> + lldev->trch_state = HIDMA_CH_STATE(val);
> + val = readl(lldev->evca + HIDMA_EVCA_CTRLSTS_REG);
> + lldev->evch_state = HIDMA_CH_STATE(val);
> +
> + /* both channels have to be enabled before calling this function */
> + if (((lldev->trch_state == HIDMA_CH_ENABLED) ||
> +  (lldev->trch_state == HIDMA_CH_RUNNING)) &&
> + ((lldev->evch_state == HIDMA_CH_ENABLED) ||
> +  (lldev->evch_state == HIDMA_CH_RUNNING)))
> + return true;

hmmm this looks hard to read, why not do:

is_chan_enabled(state)
{
switch (state) {
case HIDMA_CH_ENABLED:
case HIDMA_CH_RUNNING:
return true;
default:
return false;
}

and then :

if (is_chan_enabled(lldev->trch_state) &&
is_chan_enabled(lldev->evch_state))


> +void hidma_ll_start(struct hidma_lldev *lldev)
> +{
> + hidma_ll_hw_start(lldev);
> +}

Another dummy :(

> +/*
> + * Note that even though we stop this channel
> + * if there is a pending transaction in flight
> + * it will complete and follow the callback.
> + * This request will prevent further requests
> + * to be made.

Why the odd formating?

> +int hidma_ll_uninit(struct hidma_lldev *lldev)
> +{
> + int rc = 0;
> + u32 val;
> +
> + if (!lldev)
> + return -ENODEV;
> +
> + if (lldev->initialized) {
> + u32 required_bytes;
> +
> + lldev->initialized = 0;
> +
> + required_bytes = sizeof(struct hidma_tre) * lldev->nr_tres;
> + tasklet_kill(>task);
> + memset(lldev->trepool, 0, required_bytes);
> + lldev->trepool = NULL;
> + lldev->pending_tre_count = 0;
> + lldev->tre_write_offset = 0;
> +
> + rc = hidma_ll_reset(lldev);
> +
> + /*
> +  * Clear all pending interrupts again.
> +  * Otherwise, we observe reset complete interrupts.
> +  */
> + val = readl(lldev->evca + HIDMA_EVCA_IRQ_STAT_REG);
> + writel(val, lldev->evca + HIDMA_EVCA_IRQ_CLR_REG);
> + hidma_ll_enable_irq(lldev, 0);

uninit enables irq?

-- 
~Vinod

[PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level hardware interface

2016-04-11 Thread Sinan Kaya 
This patch implements the hardware hooks for the HIDMA channel driver.

The main functions of interest are:
- hidma_ll_init
- hidma_ll_request
- hidma_ll_queue_request
- hidma_ll_hw_start

OS layer calls the hidma_ll_init function during probe to set up the
hardware. At this moment, the number of supported descriptors are also
given. On each request, a descriptor is allocated from the free pool and
filled in with the transfer parameters. Multiple requests can be queued
into the hardware via the OS interface. When client is ready for requests
to be executed, start method is called.

Completions are delivered via callbacks via tasklet.

Signed-off-by: Sinan Kaya 
---
 drivers/dma/qcom/Makefile   |   2 +
 drivers/dma/qcom/hidma.h|  20 +-
 drivers/dma/qcom/hidma_ll.c | 900 
 3 files changed, 912 insertions(+), 10 deletions(-)
 create mode 100644 drivers/dma/qcom/hidma_ll.c

diff --git a/drivers/dma/qcom/Makefile b/drivers/dma/qcom/Makefile
index bfea699..6bf9267 100644
--- a/drivers/dma/qcom/Makefile
+++ b/drivers/dma/qcom/Makefile
@@ -1,3 +1,5 @@
 obj-$(CONFIG_QCOM_BAM_DMA) += bam_dma.o
 obj-$(CONFIG_QCOM_HIDMA_MGMT) += hdma_mgmt.o
 hdma_mgmt-objs  := hidma_mgmt.o hidma_mgmt_sys.o
+obj-$(CONFIG_QCOM_HIDMA) +=  hdma.o
+hdma-objs:= hidma_ll.o hidma.o
diff --git a/drivers/dma/qcom/hidma.h b/drivers/dma/qcom/hidma.h
index 231e306..c5eea65 100644
--- a/drivers/dma/qcom/hidma.h
+++ b/drivers/dma/qcom/hidma.h
@@ -1,7 +1,7 @@
 /*
  * Qualcomm Technologies HIDMA data structures
  *
- * Copyright (c) 2014, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2014-2016, 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
@@ -20,13 +20,13 @@
 #include 
 #include 
 
-#define TRE_SIZE   32 /* each TRE is 32 bytes  */
-#define TRE_CFG_IDX0
-#define TRE_LEN_IDX1
-#define TRE_SRC_LOW_IDX2
-#define TRE_SRC_HI_IDX 3
-#define TRE_DEST_LOW_IDX   4
-#define TRE_DEST_HI_IDX5
+#define HIDMA_TRE_SIZE 32 /* each TRE is 32 bytes  */
+#define HIDMA_TRE_CFG_IDX  0
+#define HIDMA_TRE_LEN_IDX  1
+#define HIDMA_TRE_SRC_LOW_IDX  2
+#define HIDMA_TRE_SRC_HI_IDX   3
+#define HIDMA_TRE_DEST_LOW_IDX 4
+#define HIDMA_TRE_DEST_HI_IDX  5
 
 struct hidma_tx_status {
u8 err_info;/* error record in this transfer*/
@@ -37,13 +37,13 @@ struct hidma_tre {
atomic_t allocated; /* if this channel is allocated */
bool queued;/* flag whether this is pending */
u16 status; /* status   */
-   u32 chidx;  /* index of the tre */
+   u32 idx;/* index of the tre */
u32 dma_sig;/* signature of the tre */
const char *dev_name;   /* name of the device   */
void (*callback)(void *data);   /* requester callback   */
void *data; /* Data associated with this channel*/
struct hidma_lldev *lldev;  /* lldma device pointer */
-   u32 tre_local[TRE_SIZE / sizeof(u32) + 1]; /* TRE local copy*/
+   u32 tre_local[HIDMA_TRE_SIZE / sizeof(u32) + 1]; /* TRE local copy  */
u32 tre_index;  /* the offset where this was written*/
u32 int_flags;  /* interrupt flags  */
 };
diff --git a/drivers/dma/qcom/hidma_ll.c b/drivers/dma/qcom/hidma_ll.c
new file mode 100644
index 000..443a5a2
--- /dev/null
+++ b/drivers/dma/qcom/hidma_ll.c
@@ -0,0 +1,900 @@
+/*
+ * Qualcomm Technologies HIDMA DMA engine low level code
+ *
+ * Copyright (c) 2015-2016, 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.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "hidma.h"
+
+#define HIDMA_EVRE_SIZE16  /* each EVRE is 16 
bytes */
+
+#define HIDMA_TRCA_CTRLSTS_REG 0x000
+#define HIDMA_TRCA_RING_LOW_REG0x008
+#define HIDMA_TRCA_RING_HIGH_REG   0x00C

[PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level hardware interface

2016-04-11 Thread Sinan Kaya 
This patch implements the hardware hooks for the HIDMA channel driver.

The main functions of interest are:
- hidma_ll_init
- hidma_ll_request
- hidma_ll_queue_request
- hidma_ll_hw_start

OS layer calls the hidma_ll_init function during probe to set up the
hardware. At this moment, the number of supported descriptors are also
given. On each request, a descriptor is allocated from the free pool and
filled in with the transfer parameters. Multiple requests can be queued
into the hardware via the OS interface. When client is ready for requests
to be executed, start method is called.

Completions are delivered via callbacks via tasklet.

Signed-off-by: Sinan Kaya 
---
 drivers/dma/qcom/Makefile   |   2 +
 drivers/dma/qcom/hidma.h|  20 +-
 drivers/dma/qcom/hidma_ll.c | 900 
 3 files changed, 912 insertions(+), 10 deletions(-)
 create mode 100644 drivers/dma/qcom/hidma_ll.c

diff --git a/drivers/dma/qcom/Makefile b/drivers/dma/qcom/Makefile
index bfea699..6bf9267 100644
--- a/drivers/dma/qcom/Makefile
+++ b/drivers/dma/qcom/Makefile
@@ -1,3 +1,5 @@
 obj-$(CONFIG_QCOM_BAM_DMA) += bam_dma.o
 obj-$(CONFIG_QCOM_HIDMA_MGMT) += hdma_mgmt.o
 hdma_mgmt-objs  := hidma_mgmt.o hidma_mgmt_sys.o
+obj-$(CONFIG_QCOM_HIDMA) +=  hdma.o
+hdma-objs:= hidma_ll.o hidma.o
diff --git a/drivers/dma/qcom/hidma.h b/drivers/dma/qcom/hidma.h
index 231e306..c5eea65 100644
--- a/drivers/dma/qcom/hidma.h
+++ b/drivers/dma/qcom/hidma.h
@@ -1,7 +1,7 @@
 /*
  * Qualcomm Technologies HIDMA data structures
  *
- * Copyright (c) 2014, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2014-2016, 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
@@ -20,13 +20,13 @@
 #include 
 #include 
 
-#define TRE_SIZE   32 /* each TRE is 32 bytes  */
-#define TRE_CFG_IDX0
-#define TRE_LEN_IDX1
-#define TRE_SRC_LOW_IDX2
-#define TRE_SRC_HI_IDX 3
-#define TRE_DEST_LOW_IDX   4
-#define TRE_DEST_HI_IDX5
+#define HIDMA_TRE_SIZE 32 /* each TRE is 32 bytes  */
+#define HIDMA_TRE_CFG_IDX  0
+#define HIDMA_TRE_LEN_IDX  1
+#define HIDMA_TRE_SRC_LOW_IDX  2
+#define HIDMA_TRE_SRC_HI_IDX   3
+#define HIDMA_TRE_DEST_LOW_IDX 4
+#define HIDMA_TRE_DEST_HI_IDX  5
 
 struct hidma_tx_status {
u8 err_info;/* error record in this transfer*/
@@ -37,13 +37,13 @@ struct hidma_tre {
atomic_t allocated; /* if this channel is allocated */
bool queued;/* flag whether this is pending */
u16 status; /* status   */
-   u32 chidx;  /* index of the tre */
+   u32 idx;/* index of the tre */
u32 dma_sig;/* signature of the tre */
const char *dev_name;   /* name of the device   */
void (*callback)(void *data);   /* requester callback   */
void *data; /* Data associated with this channel*/
struct hidma_lldev *lldev;  /* lldma device pointer */
-   u32 tre_local[TRE_SIZE / sizeof(u32) + 1]; /* TRE local copy*/
+   u32 tre_local[HIDMA_TRE_SIZE / sizeof(u32) + 1]; /* TRE local copy  */
u32 tre_index;  /* the offset where this was written*/
u32 int_flags;  /* interrupt flags  */
 };
diff --git a/drivers/dma/qcom/hidma_ll.c b/drivers/dma/qcom/hidma_ll.c
new file mode 100644
index 000..443a5a2
--- /dev/null
+++ b/drivers/dma/qcom/hidma_ll.c
@@ -0,0 +1,900 @@
+/*
+ * Qualcomm Technologies HIDMA DMA engine low level code
+ *
+ * Copyright (c) 2015-2016, 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.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "hidma.h"
+
+#define HIDMA_EVRE_SIZE16  /* each EVRE is 16 
bytes */
+
+#define HIDMA_TRCA_CTRLSTS_REG 0x000
+#define HIDMA_TRCA_RING_LOW_REG0x008
+#define HIDMA_TRCA_RING_HIGH_REG   0x00C
+#define