[PATCH 0/3] Enable ISOCH IN handling for AM335x

[PATCH 0/3] Enable ISOCH IN handling for AM335x

[George Cherian]

This series enables the ISOCH IN handling for AM335x HOST mode.
With this series webcam devices are now supported under AM335x
MUSB HOST with CPPI DMA enabled.

Patch 1 - Enable basic ISOCH IN handling
Patch 2 - Make CPPI aware of hb transfers
Patch 3 - Using hrtimer based polling for tx empty leads to hang , fixes it


George Cherian (3):
  usb: musb: musb_host: Enable ISOCH IN handling for AM335x host
  usb: musb: musb_cppi41: Make CPPI aware of high bandwidth transfers
  usb: musb: musb_cppi41: Handle ISOCH differently and not use the
    hrtimer.

 drivers/usb/musb/musb_cppi41.c | 67 +++++++++++++++++++++++++++++++++++++++++-
 drivers/usb/musb/musb_host.c   | 29 ++++++++++++++++--
 2 files changed, 92 insertions(+), 4 deletions(-)

-- 
1.8.1

[PATCH 1/3] usb: musb: musb_host: Enable ISOCH IN handling for AM335x host

[George Cherian]

Enable the isochrounous IN handling for AM335x HOST.
Reprogram CPPI to receive consecutive ISOCH frames in the same URB.

Signed-off-by: George Cherian <george.cherian@ti.com>
---
 drivers/usb/musb/musb_host.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index ed45572..5b6482c 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -1689,9 +1689,11 @@ void musb_host_rx(struct musb *musb, u8 epnum)
 			| MUSB_RXCSR_H_AUTOREQ
 			| MUSB_RXCSR_AUTOCLEAR
 			| MUSB_RXCSR_RXPKTRDY);
+
 		musb_writew(hw_ep->regs, MUSB_RXCSR, val);
 
-#if defined(CONFIG_USB_INVENTRA_DMA) || defined(CONFIG_USB_UX500_DMA)
+#if defined(CONFIG_USB_INVENTRA_DMA) || defined(CONFIG_USB_UX500_DMA) || \
+	defined(CONFIG_USB_TI_CPPI41_DMA)
 		if (usb_pipeisoc(pipe)) {
 			struct usb_iso_packet_descriptor *d;
 
@@ -1706,8 +1708,28 @@ void musb_host_rx(struct musb *musb, u8 epnum)
 
 			if (++qh->iso_idx >= urb->number_of_packets)
 				done = true;
-			else
+			else {
+#if defined(CONFIG_USB_TI_CPPI41_DMA)
+				struct dma_controller   *c;
+				dma_addr_t *buf;
+				u32 length, ret;
+
+				c = musb->dma_controller;
+				buf = (void *)
+					urb->iso_frame_desc[qh->iso_idx].offset
+					+ (u32)urb->transfer_dma;
+
+				length =
+					urb->iso_frame_desc[qh->iso_idx].length;
+
+				val |= MUSB_RXCSR_DMAENAB;
+				musb_writew(hw_ep->regs, MUSB_RXCSR, val);
+
+				ret = c->channel_program(dma, qh->maxpacket,
+						0, (u32) buf, length);
+#endif
 				done = false;
+			}
 
 		} else  {
 		/* done if urb buffer is full or short packet is recd */
@@ -1747,7 +1769,8 @@ void musb_host_rx(struct musb *musb, u8 epnum)
 		}
 
 		/* we are expecting IN packets */
-#if defined(CONFIG_USB_INVENTRA_DMA) || defined(CONFIG_USB_UX500_DMA)
+#if defined(CONFIG_USB_INVENTRA_DMA) || defined(CONFIG_USB_UX500_DMA) || \
+	defined(CONFIG_USB_TI_CPPI41_DMA)
 		if (dma) {
 			struct dma_controller	*c;
 			u16			rx_count;
-- 
1.8.1

Re: [PATCH 1/3] usb: musb: musb_host: Enable ISOCH IN handling for AM335x host

[Sergei Shtylyov]

Hello.

On 24-01-2014 18:14, George Cherian wrote:

> Enable the isochrounous IN handling for AM335x HOST.
> Reprogram CPPI to receive consecutive ISOCH frames in the same URB.

    Sigh, I knew CPPI ISO path was broken for years but didn't have time to 
fix it. :-(

> Signed-off-by: George Cherian <george.cherian@ti.com>
> ---
>   drivers/usb/musb/musb_host.c | 29 ++++++++++++++++++++++++++---
>   1 file changed, 26 insertions(+), 3 deletions(-)

> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> index ed45572..5b6482c 100644
> --- a/drivers/usb/musb/musb_host.c
> +++ b/drivers/usb/musb/musb_host.c
> @@ -1689,9 +1689,11 @@ void musb_host_rx(struct musb *musb, u8 epnum)
>   			| MUSB_RXCSR_H_AUTOREQ
>   			| MUSB_RXCSR_AUTOCLEAR
>   			| MUSB_RXCSR_RXPKTRDY);
> +

    Not needed.

>   		musb_writew(hw_ep->regs, MUSB_RXCSR, val);
>
> -#if defined(CONFIG_USB_INVENTRA_DMA) || defined(CONFIG_USB_UX500_DMA)
> +#if defined(CONFIG_USB_INVENTRA_DMA) || defined(CONFIG_USB_UX500_DMA) || \
> +	defined(CONFIG_USB_TI_CPPI41_DMA)

    The DaVinci CPPI 3.0 should probably also be included here...

>   		if (usb_pipeisoc(pipe)) {
>   			struct usb_iso_packet_descriptor *d;
>
> @@ -1706,8 +1708,28 @@ void musb_host_rx(struct musb *musb, u8 epnum)
>
>   			if (++qh->iso_idx >= urb->number_of_packets)
>   				done = true;
> -			else
> +			else {

    If you're making the *else* arm use {}, you should make all the other arms 
of *if* statement also have them -- see Documentation/CodingStyle.

WBR, Sergei

Re: [PATCH 1/3] usb: musb: musb_host: Enable ISOCH IN handling for AM335x host

[George Cherian]

On 1/24/2014 11:13 PM, Sergei Shtylyov wrote:
> Hello.
>
> On 24-01-2014 18:14, George Cherian wrote:
>
>> Enable the isochrounous IN handling for AM335x HOST.
>> Reprogram CPPI to receive consecutive ISOCH frames in the same URB.
>
>    Sigh, I knew CPPI ISO path was broken for years but didn't have 
> time to fix it. :-(
>
>> Signed-off-by: George Cherian <george.cherian@ti.com>
>> --- git rebase
>>   drivers/usb/musb/musb_host.c | 29 ++++++++++++++++++++++++++---
>>   1 file changed, 26 insertions(+), 3 deletions(-)
>
>> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
>> index ed45572..5b6482c 100644
>> --- a/drivers/usb/musb/musb_host.c
>> +++ b/drivers/usb/musb/musb_host.c
>> @@ -1689,9 +1689,11 @@ void musb_host_rx(struct musb *musb, u8 epnum)
>>               | MUSB_RXCSR_H_AUTOREQ
>>               | MUSB_RXCSR_AUTOCLEAR
>>               | MUSB_RXCSR_RXPKTRDY);
>> +
>
>    Not needed.
>
>>           musb_writew(hw_ep->regs, MUSB_RXCSR, val);
>>
>> -#if defined(CONFIG_USB_INVENTRA_DMA) || defined(CONFIG_USB_UX500_DMA)
>> +#if defined(CONFIG_USB_INVENTRA_DMA) || 
>> defined(CONFIG_USB_UX500_DMA) || \
>> +    defined(CONFIG_USB_TI_CPPI41_DMA)
>
>    The DaVinci CPPI 3.0 should probably also be included here...
>
Should'nt that be a separate patch as this one is specific to AM335X.
>>           if (usb_pipeisoc(pipe)) {
>>               struct usb_iso_packet_descriptor *d;
>>
>> @@ -1706,8 +1708,28 @@ void musb_host_rx(struct musb *musb, u8 epnum)
>>
>>               if (++qh->iso_idx >= urb->number_of_packets)
>>                   done = true;
>> -            else
>> +            else {
>
>    If you're making the *else* arm use {}, you should make all the 
> other arms of *if* statement also have them -- see 
> Documentation/CodingStyle.

Will fix in v2!!!
>
> WBR, Sergei
>


-- 
-George

Re: [PATCH 1/3] usb: musb: musb_host: Enable ISOCH IN handling for AM335x host

[Sergei Shtylyov]

Hello.

On 27-01-2014 13:21, George Cherian wrote:

>>> Enable the isochrounous IN handling for AM335x HOST.
>>> Reprogram CPPI to receive consecutive ISOCH frames in the same URB.

>>    Sigh, I knew CPPI ISO path was broken for years but didn't have time to
>> fix it. :-(

>>> Signed-off-by: George Cherian <george.cherian@ti.com>
>>> --- git rebase
>>>   drivers/usb/musb/musb_host.c | 29 ++++++++++++++++++++++++++---
>>>   1 file changed, 26 insertions(+), 3 deletions(-)

>>> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
>>> index ed45572..5b6482c 100644
>>> --- a/drivers/usb/musb/musb_host.c
>>> +++ b/drivers/usb/musb/musb_host.c
>>> @@ -1689,9 +1689,11 @@ void musb_host_rx(struct musb *musb, u8 epnum)
[...]
>>>           musb_writew(hw_ep->regs, MUSB_RXCSR, val);
>>>
>>> -#if defined(CONFIG_USB_INVENTRA_DMA) || defined(CONFIG_USB_UX500_DMA)
>>> +#if defined(CONFIG_USB_INVENTRA_DMA) || defined(CONFIG_USB_UX500_DMA) || \
>>> +    defined(CONFIG_USB_TI_CPPI41_DMA)

>>    The DaVinci CPPI 3.0 should probably also be included here...

> Should'nt that be a separate patch as this one is specific to AM335X.

    Of course.

WBR, Sergei

[PATCH 2/3] usb: musb: musb_cppi41: Make CPPI aware of high bandwidth transfers

[George Cherian]

Enable CPPI to handle high bandwidth transfers, especially to support
webcam captures. Use a single bd to get the whole of the data in case of
high bandwidth transfers.

Signed-off-by: George Cherian <george.cherian@ti.com>
---
 drivers/usb/musb/musb_cppi41.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c
index f889296..39ee516 100644
--- a/drivers/usb/musb/musb_cppi41.c
+++ b/drivers/usb/musb/musb_cppi41.c
@@ -448,12 +448,25 @@ static int cppi41_dma_channel_program(struct dma_channel *channel,
 				dma_addr_t dma_addr, u32 len)
 {
 	int ret;
+	struct cppi41_dma_channel *cppi41_channel = channel->private_data;
+	int hb_mult = 0;
 
 	BUG_ON(channel->status == MUSB_DMA_STATUS_UNKNOWN ||
 		channel->status == MUSB_DMA_STATUS_BUSY);
 
+	if (is_host_active(cppi41_channel->controller->musb)) {
+		if (cppi41_channel->is_tx)
+			hb_mult = cppi41_channel->hw_ep->out_qh->hb_mult;
+		else
+			hb_mult = cppi41_channel->hw_ep->in_qh->hb_mult;
+	}
+
 	channel->status = MUSB_DMA_STATUS_BUSY;
 	channel->actual_len = 0;
+
+	if (hb_mult)
+		packet_sz = hb_mult * (packet_sz & 0x7FF);
+
 	ret = cppi41_configure_channel(channel, packet_sz, mode, dma_addr, len);
 	if (!ret)
 		channel->status = MUSB_DMA_STATUS_FREE;
-- 
1.8.1

[PATCH 3/3] usb: musb: musb_cppi41: Handle ISOCH differently and not use the hrtimer.

[George Cherian]

In case of ISOCH transfers the hrtimer workaround for the hardware issue
is not very reliable. Instead of checking musb_is_tx_fifo_empty() in hrtimer
routine, schedule a completion work and check the same in completion work.

Signed-off-by: George Cherian <george.cherian@ti.com>
---
 drivers/usb/musb/musb_cppi41.c | 54 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c
index 39ee516..8075562 100644
--- a/drivers/usb/musb/musb_cppi41.c
+++ b/drivers/usb/musb/musb_cppi41.c
@@ -39,6 +39,7 @@ struct cppi41_dma_channel {
 	u32 transferred;
 	u32 packet_sz;
 	struct list_head tx_check;
+	struct work_struct dma_completion;
 };
 
 #define MUSB_DMA_NUM_CHANNELS 15
@@ -112,6 +113,18 @@ static bool musb_is_tx_fifo_empty(struct musb_hw_ep *hw_ep)
 	return true;
 }
 
+static bool is_isoc(struct musb_hw_ep *hw_ep, bool in)
+{
+	if (in && hw_ep->in_qh) {
+		if (hw_ep->in_qh->type == USB_ENDPOINT_XFER_ISOC)
+			return true;
+	} else if (hw_ep->out_qh) {
+		if (hw_ep->out_qh->type == USB_ENDPOINT_XFER_ISOC)
+			return true;
+	}
+	return false;
+}
+
 static void cppi41_dma_callback(void *private_data);
 
 static void cppi41_trans_done(struct cppi41_dma_channel *cppi41_channel)
@@ -165,6 +178,32 @@ static void cppi41_trans_done(struct cppi41_dma_channel *cppi41_channel)
 	}
 }
 
+static void cppi_trans_done_work(struct work_struct *work)
+{
+	unsigned long flags;
+	struct cppi41_dma_channel *cppi41_channel =
+		container_of(work, struct cppi41_dma_channel, dma_completion);
+	struct cppi41_dma_controller *controller = cppi41_channel->controller;
+	struct musb *musb = controller->musb;
+	struct musb_hw_ep *hw_ep = cppi41_channel->hw_ep;
+	bool empty;
+
+	if (!cppi41_channel->is_tx && is_isoc(hw_ep, 1)) {
+			spin_lock_irqsave(&musb->lock, flags);
+			cppi41_trans_done(cppi41_channel);
+			spin_unlock_irqrestore(&musb->lock, flags);
+	} else {
+		empty = musb_is_tx_fifo_empty(hw_ep);
+		if (empty) {
+			spin_lock_irqsave(&musb->lock, flags);
+			cppi41_trans_done(cppi41_channel);
+			spin_unlock_irqrestore(&musb->lock, flags);
+		} else {
+			schedule_work(&cppi41_channel->dma_completion);
+		}
+	}
+}
+
 static enum hrtimer_restart cppi41_recheck_tx_req(struct hrtimer *timer)
 {
 	struct cppi41_dma_controller *controller;
@@ -228,6 +267,14 @@ static void cppi41_dma_callback(void *private_data)
 			transferred < cppi41_channel->packet_sz)
 		cppi41_channel->prog_len = 0;
 
+	if (!cppi41_channel->is_tx) {
+		if (is_isoc(hw_ep, 1))
+			schedule_work(&cppi41_channel->dma_completion);
+		else
+			cppi41_trans_done(cppi41_channel);
+		goto out;
+	}
+
 	empty = musb_is_tx_fifo_empty(hw_ep);
 	if (empty) {
 		cppi41_trans_done(cppi41_channel);
@@ -264,6 +311,10 @@ static void cppi41_dma_callback(void *private_data)
 				goto out;
 			}
 		}
+		if (is_isoc(hw_ep, 0)) {
+			schedule_work(&cppi41_channel->dma_completion);
+			goto out;
+		}
 		list_add_tail(&cppi41_channel->tx_check,
 				&controller->early_tx_list);
 		if (!hrtimer_active(&controller->early_tx)) {
@@ -620,7 +671,8 @@ static int cppi41_dma_controller_start(struct cppi41_dma_controller *controller)
 		cppi41_channel->port_num = port;
 		cppi41_channel->is_tx = is_tx;
 		INIT_LIST_HEAD(&cppi41_channel->tx_check);
-
+		INIT_WORK(&cppi41_channel->dma_completion,
+			  cppi_trans_done_work);
 		musb_dma = &cppi41_channel->channel;
 		musb_dma->private_data = cppi41_channel;
 		musb_dma->status = MUSB_DMA_STATUS_FREE;
-- 
1.8.1

Re: [PATCH 3/3] usb: musb: musb_cppi41: Handle ISOCH differently and not use the hrtimer.

[Sergei Shtylyov]

Hello.

On 24-01-2014 18:14, George Cherian wrote:

> In case of ISOCH transfers the hrtimer workaround for the hardware issue
> is not very reliable. Instead of checking musb_is_tx_fifo_empty() in hrtimer
> routine, schedule a completion work and check the same in completion work.

> Signed-off-by: George Cherian <george.cherian@ti.com>
> ---
>   drivers/usb/musb/musb_cppi41.c | 54 +++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 53 insertions(+), 1 deletion(-)

> diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c
> index 39ee516..8075562 100644
> --- a/drivers/usb/musb/musb_cppi41.c
> +++ b/drivers/usb/musb/musb_cppi41.c
[...]
> @@ -165,6 +178,32 @@ static void cppi41_trans_done(struct cppi41_dma_channel *cppi41_channel)
>   	}
>   }
>
> +static void cppi_trans_done_work(struct work_struct *work)
> +{
> +	unsigned long flags;
> +	struct cppi41_dma_channel *cppi41_channel =
> +		container_of(work, struct cppi41_dma_channel, dma_completion);
> +	struct cppi41_dma_controller *controller = cppi41_channel->controller;
> +	struct musb *musb = controller->musb;
> +	struct musb_hw_ep *hw_ep = cppi41_channel->hw_ep;
> +	bool empty;
> +
> +	if (!cppi41_channel->is_tx && is_isoc(hw_ep, 1)) {
> +			spin_lock_irqsave(&musb->lock, flags);
> +			cppi41_trans_done(cppi41_channel);
> +			spin_unlock_irqrestore(&musb->lock, flags);

    This *if* arm is clearly over-indented by one tab. Compare with below 
*else* arm.

> +	} else {
> +		empty = musb_is_tx_fifo_empty(hw_ep);
> +		if (empty) {
> +			spin_lock_irqsave(&musb->lock, flags);
> +			cppi41_trans_done(cppi41_channel);
> +			spin_unlock_irqrestore(&musb->lock, flags);
> +		} else {
> +			schedule_work(&cppi41_channel->dma_completion);
> +		}
> +	}
> +}
> +

WBR, Sergei

Re: [PATCH 3/3] usb: musb: musb_cppi41: Handle ISOCH differently and not use the hrtimer.

[George Cherian]

On 1/24/2014 11:16 PM, Sergei Shtylyov wrote:
> Hello.
>
> On 24-01-2014 18:14, George Cherian wrote:
>
>> In case of ISOCH transfers the hrtimer workaround for the hardware issue
>> is not very reliable. Instead of checking musb_is_tx_fifo_empty() in 
>> hrtimer
>> routine, schedule a completion work and check the same in completion 
>> work.
>
>> Signed-off-by: George Cherian <george.cherian@ti.com>
>> ---
>>   drivers/usb/musb/musb_cppi41.c | 54 
>> +++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 53 insertions(+), 1 deletion(-)
>
>> diff --git a/drivers/usb/musb/musb_cppi41.c 
>> b/drivers/usb/musb/musb_cppi41.c
>> index 39ee516..8075562 100644
>> --- a/drivers/usb/musb/musb_cppi41.c
>> +++ b/drivers/usb/musb/musb_cppi41.c
> [...]
>> @@ -165,6 +178,32 @@ static void cppi41_trans_done(struct 
>> cppi41_dma_channel *cppi41_channel)
>>       }
>>   }
>>
>> +static void cppi_trans_done_work(struct work_struct *work)
>> +{
>> +    unsigned long flags;
>> +    struct cppi41_dma_channel *cppi41_channel =
>> +        container_of(work, struct cppi41_dma_channel, dma_completion);
>> +    struct cppi41_dma_controller *controller = 
>> cppi41_channel->controller;
>> +    struct musb *musb = controller->musb;
>> +    struct musb_hw_ep *hw_ep = cppi41_channel->hw_ep;
>> +    bool empty;
>> +
>> +    if (!cppi41_channel->is_tx && is_isoc(hw_ep, 1)) {
>> +            spin_lock_irqsave(&musb->lock, flags);
>> +            cppi41_trans_done(cppi41_channel);
>> +            spin_unlock_irqrestore(&musb->lock, flags);
>
>    This *if* arm is clearly over-indented by one tab. Compare with 
> below *else* arm.

Will Fix in v2


>
>> +    } else {
>> +        empty = musb_is_tx_fifo_empty(hw_ep);
>> +        if (empty) {
>> +            spin_lock_irqsave(&musb->lock, flags);
>> +            cppi41_trans_done(cppi41_channel);
>> +            spin_unlock_irqrestore(&musb->lock, flags);
>> +        } else {
>> + schedule_work(&cppi41_channel->dma_completion);
>> +        }
>> +    }
>> +}
>> +
>
> WBR, Sergei
>


-- 
-George