Re: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support
Hi Felipe, On Wed, Jul 2, 2014 at 10:16 PM, Felipe Balbi ba...@ti.com wrote: Hi, On Sun, May 25, 2014 at 11:10:30PM +0530, sundeep subbaraya wrote: Hi Felipe, Please take a look at below about how this IP works: IN: req.buf --- DMA (transfers from ddr to IP buffer, raise DMA done interrupt and set Buffer ready to transfer data to Host)Host PC buffer sent interrupt OUT: Host PC---buffer ready interrupt---DMA (transfer from IP buffer to DDR,DMA done interrupt, set Buffer ready to receive next data from Host)--req.buf I written logic to call completion in DMA done handler because it works for both IN and OUT eps. But I see significant performance degradation (by copying a file to mass storage gadget). DMA can handle unaligned address too but it has ep max limit (say 512 for bulk). Hence it is SW job to split packets and to drive DMA till req.length completes. I feel polling for a while is faster than switching between interrupt handler back and forth rapidly. Moreover, DMA may or may not be present in IP based on user configuration at design time. I fixed all other comments expect this one if you do not agree for polling then I may have to create threads and do this stuff. What do you suggest? Can you resend your driver so we can review again everything you have fixed ? I guess you have no other way... polling it is. Sure. I will resend the driver. Thanks, Sundeep.B.S. cheers -- balbi -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support
Hi, On Sun, May 25, 2014 at 11:10:30PM +0530, sundeep subbaraya wrote: Hi Felipe, Please take a look at below about how this IP works: IN: req.buf --- DMA (transfers from ddr to IP buffer, raise DMA done interrupt and set Buffer ready to transfer data to Host)Host PC buffer sent interrupt OUT: Host PC---buffer ready interrupt---DMA (transfer from IP buffer to DDR,DMA done interrupt, set Buffer ready to receive next data from Host)--req.buf I written logic to call completion in DMA done handler because it works for both IN and OUT eps. But I see significant performance degradation (by copying a file to mass storage gadget). DMA can handle unaligned address too but it has ep max limit (say 512 for bulk). Hence it is SW job to split packets and to drive DMA till req.length completes. I feel polling for a while is faster than switching between interrupt handler back and forth rapidly. Moreover, DMA may or may not be present in IP based on user configuration at design time. I fixed all other comments expect this one if you do not agree for polling then I may have to create threads and do this stuff. What do you suggest? Can you resend your driver so we can review again everything you have fixed ? I guess you have no other way... polling it is. cheers -- balbi signature.asc Description: Digital signature
Re: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support
Hi Felipe, Please take a look at below about how this IP works: IN: req.buf --- DMA (transfers from ddr to IP buffer, raise DMA done interrupt and set Buffer ready to transfer data to Host)Host PC buffer sent interrupt OUT: Host PC---buffer ready interrupt---DMA (transfer from IP buffer to DDR,DMA done interrupt, set Buffer ready to receive next data from Host)--req.buf I written logic to call completion in DMA done handler because it works for both IN and OUT eps. But I see significant performance degradation (by copying a file to mass storage gadget). DMA can handle unaligned address too but it has ep max limit (say 512 for bulk). Hence it is SW job to split packets and to drive DMA till req.length completes. I feel polling for a while is faster than switching between interrupt handler back and forth rapidly. Moreover, DMA may or may not be present in IP based on user configuration at design time. I fixed all other comments expect this one if you do not agree for polling then I may have to create threads and do this stuff. What do you suggest? Thanks, Sundeep On Wed, Apr 30, 2014 at 10:24 PM, Felipe Balbi ba...@ti.com wrote: Hi, On Wed, Apr 23, 2014 at 09:57:52AM +0530, sundeep subbaraya wrote: I get the impression that the two of you are arguing past each other. It appears that Sundeep is talking about transferring data from the gadget driver's buffer to an internal buffer in the UDC hardware, but Felipe is talking about transferring data from the UDC to the host. As I understand it, Sundeep said that when the gadget driver queues a data-IN request, the UDC driver copies as much of the data buffer as possible into a hardware FIFO. If it succeeds in copying all the data into the FIFO then the request's completion routine gets called immediately, even though the data doesn't get sent from the FIFO to the host until the host asks for it. If only part of the data can be copied into the FIFO then the request is added to the ep's request queue before the usb_ep_queue() call returns. When space becomes available in the FIFO, the data will be copied and eventually sent to the host. When all the data has been copied to the FIFO, the request's completion routine will be called. there seems to be a slight problem with this approach: how will the IP know that even though you copied X bytes into the FIFO, it should wait for another Y bytes before shifting data to the wire ? How will it know that it shouldn't generate CRC yet because there's still data to be added ? No. IP does/need not know that it has to wait for Y bytes.We just write X bytes into HW buffer and count as X in buffer count register. IP generates CRC for bytes based on Count register and sends data to Host. Let us consider this scenario of bulk IN transfer: req.length = 5120 and wMaxPacketSize = 512, ep_queue is called once and is returned with status 0. In ep_queue this code snippet, if (xudc_write_fifo(ep, req) == 1) req = NULL; if(req != NULL) list_add_tail(req-queue, ep-queue); xudc_write_fifo does the following if HW buffers not busy: copies 512 bytes to HW buffer set count and ready registers so that IP can start data transfer to host changes req.actual to 512 and returns 0(if req.length wMaxPacketSize) and 1(if req.length wMaxPacketSize). you should return a proper error code, not 1. Since return is zero this request is added to queue. When data transfer to host is completed IP generates an interrupt. In the interrupt handler we again call write_fifo if request list is not empty. if (list_empty(ep-queue)) req = NULL; else req = list_entry(ep-queue.next, struct xusb_req, queue); if (!req) return; okay, this can be improved a bit though: if (list_empty(ep-queue)) return; req = list_first_entry(ep-queue, struct xusb_req, queue); if (ep-is_in) xudc_write_fifo(ep, req); else xudc_read_fifo(ep, req); This happens 10 times(since length 5120) and completion is called. ok. If there's no space in the FIFO yet, why copy data at all ? If HW buffers are busy(IP is still transferring previous data to Host from buffer) then xudc_write_fifo returns 0 without changing req.actual. When previous data transfer completes then Interrupt then again write_fifo from handler. ok, sounds good to me. Thus there never is any need for the gadget driver to queue the request again. Yes An incomplete transfer means the FIFO didn't have enough room when the request was submitted; it doesn't mean that the data didn't eventually get sent to the host. Exactly Alan,this is what I was trying to say. Probably I was not clear in explaining. I didnt see any harm this
Re: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support
Hi, On Mon, Apr 21, 2014 at 10:09 PM, Alan Stern st...@rowland.harvard.edu wrote: On Mon, 21 Apr 2014, Felipe Balbi wrote: Hi, On Fri, Apr 18, 2014 at 07:34:08PM +0530, sundeep subbaraya wrote: snip in ep_queue driver starts dma transfer from/to IP buffer to/from req-buf. If transfer is completed then request is not added to ep request queue and returns from ep_queue. If transfer is not completed (actual length) then request is added to queue and returns from ep_queue. This is wrong. Why wouldn't you give gadget driver the chance to decide if it needs to queue the request again or not ? When does gadget driver decides to queue the same request again? if -EBUSY is returned from ep_queue or req.status != 0 in completion routine? whenever it so decides. Different gadget drivers might have different requirements. The code is open and sits under drivers/usb/gadget/ why don't you have a read ? I get the impression that the two of you are arguing past each other. It appears that Sundeep is talking about transferring data from the gadget driver's buffer to an internal buffer in the UDC hardware, but Felipe is talking about transferring data from the UDC to the host. As I understand it, Sundeep said that when the gadget driver queues a data-IN request, the UDC driver copies as much of the data buffer as possible into a hardware FIFO. If it succeeds in copying all the data into the FIFO then the request's completion routine gets called immediately, even though the data doesn't get sent from the FIFO to the host until the host asks for it. If only part of the data can be copied into the FIFO then the request is added to the ep's request queue before the usb_ep_queue() call returns. When space becomes available in the FIFO, the data will be copied and eventually sent to the host. When all the data has been copied to the FIFO, the request's completion routine will be called. Thus there never is any need for the gadget driver to queue the request again. An incomplete transfer means the FIFO didn't have enough room when the request was submitted; it doesn't mean that the data didn't eventually get sent to the host. Exactly Alan,this is what I was trying to say. Probably I was not clear in explaining. I didnt see any harm this way and even this implementation is same like at91_udc.c. I have been reading mas_storage to understand when does gadget driver tries to enqueue a request again. Since different gadget drivers might have different requirements (agree with Felipe), wanted to know criteria for queuing a same request again. I will change this implementation as per Felipe comments and test with some of the gadgets. Thanks Alan and Felipe, Sundeep HTH, Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support
On Tue, 22 Apr 2014, sundeep subbaraya wrote: Hi, On Mon, Apr 21, 2014 at 10:09 PM, Alan Stern st...@rowland.harvard.edu wrote: On Mon, 21 Apr 2014, Felipe Balbi wrote: Hi, On Fri, Apr 18, 2014 at 07:34:08PM +0530, sundeep subbaraya wrote: snip in ep_queue driver starts dma transfer from/to IP buffer to/from req-buf. If transfer is completed then request is not added to ep request queue and returns from ep_queue. If transfer is not completed (actual length) then request is added to queue and returns from ep_queue. This is wrong. Why wouldn't you give gadget driver the chance to decide if it needs to queue the request again or not ? When does gadget driver decides to queue the same request again? if -EBUSY is returned from ep_queue or req.status != 0 in completion routine? whenever it so decides. Different gadget drivers might have different requirements. The code is open and sits under drivers/usb/gadget/ why don't you have a read ? I get the impression that the two of you are arguing past each other. It appears that Sundeep is talking about transferring data from the gadget driver's buffer to an internal buffer in the UDC hardware, but Felipe is talking about transferring data from the UDC to the host. As I understand it, Sundeep said that when the gadget driver queues a data-IN request, the UDC driver copies as much of the data buffer as possible into a hardware FIFO. If it succeeds in copying all the data into the FIFO then the request's completion routine gets called immediately, even though the data doesn't get sent from the FIFO to the host until the host asks for it. If only part of the data can be copied into the FIFO then the request is added to the ep's request queue before the usb_ep_queue() call returns. When space becomes available in the FIFO, the data will be copied and eventually sent to the host. When all the data has been copied to the FIFO, the request's completion routine will be called. Thus there never is any need for the gadget driver to queue the request again. An incomplete transfer means the FIFO didn't have enough room when the request was submitted; it doesn't mean that the data didn't eventually get sent to the host. Exactly Alan,this is what I was trying to say. Probably I was not clear in explaining. I didnt see any harm this way and even this implementation is same like at91_udc.c. I have been reading mas_storage to understand when does gadget driver tries to enqueue a request again. Since different gadget drivers might have different requirements (agree with Felipe), wanted to know criteria for queuing a same request again. I will change this implementation as per Felipe comments and test with some of the gadgets. Wait until you hear what Felipe has to say about this discussion. Maybe he will decide that the original implementation was correct. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support
Hi, On Tue, Apr 22, 2014 at 12:58:41PM +0530, sundeep subbaraya wrote: Hi, On Mon, Apr 21, 2014 at 10:09 PM, Alan Stern st...@rowland.harvard.edu wrote: On Mon, 21 Apr 2014, Felipe Balbi wrote: Hi, On Fri, Apr 18, 2014 at 07:34:08PM +0530, sundeep subbaraya wrote: snip in ep_queue driver starts dma transfer from/to IP buffer to/from req-buf. If transfer is completed then request is not added to ep request queue and returns from ep_queue. If transfer is not completed (actual length) then request is added to queue and returns from ep_queue. This is wrong. Why wouldn't you give gadget driver the chance to decide if it needs to queue the request again or not ? When does gadget driver decides to queue the same request again? if -EBUSY is returned from ep_queue or req.status != 0 in completion routine? whenever it so decides. Different gadget drivers might have different requirements. The code is open and sits under drivers/usb/gadget/ why don't you have a read ? I get the impression that the two of you are arguing past each other. It appears that Sundeep is talking about transferring data from the gadget driver's buffer to an internal buffer in the UDC hardware, but Felipe is talking about transferring data from the UDC to the host. As I understand it, Sundeep said that when the gadget driver queues a data-IN request, the UDC driver copies as much of the data buffer as possible into a hardware FIFO. If it succeeds in copying all the data into the FIFO then the request's completion routine gets called immediately, even though the data doesn't get sent from the FIFO to the host until the host asks for it. If only part of the data can be copied into the FIFO then the request is added to the ep's request queue before the usb_ep_queue() call returns. When space becomes available in the FIFO, the data will be copied and eventually sent to the host. When all the data has been copied to the FIFO, the request's completion routine will be called. there seems to be a slight problem with this approach: how will the IP know that even though you copied X bytes into the FIFO, it should wait for another Y bytes before shifting data to the wire ? How will it know that it shouldn't generate CRC yet because there's still data to be added ? If there's no space in the FIFO yet, why copy data at all ? Thus there never is any need for the gadget driver to queue the request again. An incomplete transfer means the FIFO didn't have enough room when the request was submitted; it doesn't mean that the data didn't eventually get sent to the host. Exactly Alan,this is what I was trying to say. Probably I was not clear in explaining. I didnt see any harm this way and even this implementation is same like at91_udc.c. I have been reading mas_storage to understand when does gadget driver tries to enqueue a request again. Since different gadget drivers might have different requirements (agree with Felipe), wanted to know criteria for queuing a same request again. I will change this implementation as per Felipe comments and test with some of the gadgets. Let's see, please help me understand the questions above. -- balbi signature.asc Description: Digital signature
Re: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support
Hi, On Fri, Apr 18, 2014 at 07:34:08PM +0530, sundeep subbaraya wrote: snip in ep_queue driver starts dma transfer from/to IP buffer to/from req-buf. If transfer is completed then request is not added to ep request queue and returns from ep_queue. If transfer is not completed (actual length) then request is added to queue and returns from ep_queue. This is wrong. Why wouldn't you give gadget driver the chance to decide if it needs to queue the request again or not ? When does gadget driver decides to queue the same request again? if -EBUSY is returned from ep_queue or req.status != 0 in completion routine? whenever it so decides. Different gadget drivers might have different requirements. The code is open and sits under drivers/usb/gadget/ why don't you have a read ? there are cases where ping-pong buffers both are busy. currently this driver adds request to queue only when buffers are busy. In normal cases request is processed without adding to queue. it shouldn't do that. You can't add requests by yourself. If you can't initiate transfers right now, let the HW NAK or something. do i need to have another local queue for requests waiting since buffers are busy? you should probably move the request to another queue, yes. Here's what dwc3 does: . gadget queues request . dwc3 puts request in a 'queued' list . HW sends XferNotReady event to notify it needs transfer requests . dwc3 moves requests to 'pending' list . dwc3 tells hw to consume 'pending' list if gadget driver queues another request, we accept it into 'queued' list and wait for another XferNotReady before moving it to 'pending' and consuming it. Or dequeue the request ? you could return -EAGAIN, if you wish. But don't add requests by yourself, this could end up in hard-to-debug scenarios. Remember you don't *own* the reqeusts, only gadget drivers do. You can keep the request around until you have space in your FIFOs to start it, but then, in that case, don't try to list_del() - start() - list_add(). when buffer processed interrupt occurs, handler starts dma if there is request in queue and calls complete call back (when actual == length) Hence answer is Yes for some transfers (start dma called from interrupt handler not from ep_queue). this also seems wrong(-ish). 1) as Paul pointed out, you can't rely on jiffies if you're calling this with IRQs disabled. 2) you don't really need to wait until DMA finishes its transaction before returning from start_dma(), just use the DMA completion IRQ, 3) I don't see anywhere any sanitizing of arguments, can your DMA really handle any alignment/unaligned addresses or should you make sure you're getting good arguments? You need to work on this a little bit, I guess. Yes am working on this and will be using seperate ep_ops for ep0 like drivers/usb/dwc3/gadget.c since ep0 has no dma and ping-pong buffers ok -- balbi signature.asc Description: Digital signature
Re: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support
On Mon, 21 Apr 2014, Felipe Balbi wrote: Hi, On Fri, Apr 18, 2014 at 07:34:08PM +0530, sundeep subbaraya wrote: snip in ep_queue driver starts dma transfer from/to IP buffer to/from req-buf. If transfer is completed then request is not added to ep request queue and returns from ep_queue. If transfer is not completed (actual length) then request is added to queue and returns from ep_queue. This is wrong. Why wouldn't you give gadget driver the chance to decide if it needs to queue the request again or not ? When does gadget driver decides to queue the same request again? if -EBUSY is returned from ep_queue or req.status != 0 in completion routine? whenever it so decides. Different gadget drivers might have different requirements. The code is open and sits under drivers/usb/gadget/ why don't you have a read ? I get the impression that the two of you are arguing past each other. It appears that Sundeep is talking about transferring data from the gadget driver's buffer to an internal buffer in the UDC hardware, but Felipe is talking about transferring data from the UDC to the host. As I understand it, Sundeep said that when the gadget driver queues a data-IN request, the UDC driver copies as much of the data buffer as possible into a hardware FIFO. If it succeeds in copying all the data into the FIFO then the request's completion routine gets called immediately, even though the data doesn't get sent from the FIFO to the host until the host asks for it. If only part of the data can be copied into the FIFO then the request is added to the ep's request queue before the usb_ep_queue() call returns. When space becomes available in the FIFO, the data will be copied and eventually sent to the host. When all the data has been copied to the FIFO, the request's completion routine will be called. Thus there never is any need for the gadget driver to queue the request again. An incomplete transfer means the FIFO didn't have enough room when the request was submitted; it doesn't mean that the data didn't eventually get sent to the host. HTH, Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support
Hi Felipe,
On Thu, Apr 17, 2014 at 8:31 PM, Felipe Balbi ba...@ti.com wrote:
On Thu, Apr 17, 2014 at 03:01:37PM +0530, sundeep subbaraya wrote:
Hi Felipe,
On Wed, Apr 16, 2014 at 11:32 PM, Felipe Balbi ba...@ti.com wrote:
Hi,
On Wed, Apr 16, 2014 at 04:09:28PM +0530, sundeep subbaraya wrote:
Hi Felipe,
On Thu, Apr 3, 2014 at 8:28 PM, Felipe Balbi ba...@ti.com wrote:
+static int start_dma(struct xusb_ep *ep, u32 src, u32 dst, u32 length)
please prepend this with xudc_, it makes tracing a lot easier.
+{
+ struct xusb_udc *udc;
+ int rc = 0;
+ unsigned long timeout;
+
+ udc = ep-udc;
+ /*
+ * Set the addresses in the DMA source and
+ * destination registers and then set the length
+ * into the DMA length register.
+ */
+ udc-write_fn(udc-base_address, XUSB_DMA_DSAR_ADDR_OFFSET, src);
+ udc-write_fn(udc-base_address, XUSB_DMA_DDAR_ADDR_OFFSET, dst);
+ udc-write_fn(udc-base_address, XUSB_DMA_LENGTH_OFFSET, length);
+
+ /*
+ * Wait till DMA transaction is complete and
+ * check whether the DMA transaction was
+ * successful.
+ */
+ while ((udc-read_fn(ep-udc-base_address +
XUSB_DMA_STATUS_OFFSET)
+ XUSB_DMA_DMASR_BUSY) == XUSB_DMA_DMASR_BUSY) {
+ timeout = jiffies + 1;
+
+ if (time_after(jiffies, timeout)) {
+ rc = -ETIMEDOUT;
+ goto clean;
+ }
+ }
don't you get an IRQ for DMA completion ? If you do, you could be using
wait_for_completion()
This function is called in interrupt context when buffer is ready or
free. It initiates DMA to transfer data from IP buffer to memory.
Hence it waits in busy loop till DMA completes
wait, so you start_dma() before your gadget driver asks you to ?
in ep_queue driver starts dma transfer from/to IP buffer to/from req-buf.
If transfer is completed then request is not added to ep request queue
and returns from ep_queue.
If transfer is not completed (actual length) then request is added
to queue and returns from ep_queue.
This is wrong. Why wouldn't you give gadget driver the chance to decide
if it needs to queue the request again or not ?
When does gadget driver decides to queue the same request again?
if -EBUSY is returned from ep_queue or req.status != 0 in completion
routine?
there are cases where ping-pong buffers both are busy. currently this driver
adds request to queue only when buffers are busy. In normal cases request is
processed without adding to queue.
do i need to have another local queue for requests waiting since
buffers are busy?
Or dequeue the request ?
when buffer processed interrupt occurs, handler starts dma if there is
request in queue and calls complete call back (when actual == length)
Hence answer is Yes for some transfers (start dma called from
interrupt handler not from ep_queue).
this also seems wrong(-ish).
1) as Paul pointed out, you can't rely on jiffies if you're calling this
with IRQs disabled.
2) you don't really need to wait until DMA finishes its transaction
before returning from start_dma(), just use the DMA completion IRQ,
3) I don't see anywhere any sanitizing of arguments, can your DMA really
handle any alignment/unaligned addresses or should you make sure you're
getting good arguments?
You need to work on this a little bit, I guess.
Yes am working on this and will be using seperate ep_ops for ep0 like
drivers/usb/dwc3/gadget.c since ep0 has no dma and ping-pong buffers
Thanks,
Sundeep.B.S.
--
balbi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support
Hi Felipe,
On Wed, Apr 16, 2014 at 11:32 PM, Felipe Balbi ba...@ti.com wrote:
Hi,
On Wed, Apr 16, 2014 at 04:09:28PM +0530, sundeep subbaraya wrote:
Hi Felipe,
On Thu, Apr 3, 2014 at 8:28 PM, Felipe Balbi ba...@ti.com wrote:
+static int start_dma(struct xusb_ep *ep, u32 src, u32 dst, u32 length)
please prepend this with xudc_, it makes tracing a lot easier.
+{
+ struct xusb_udc *udc;
+ int rc = 0;
+ unsigned long timeout;
+
+ udc = ep-udc;
+ /*
+ * Set the addresses in the DMA source and
+ * destination registers and then set the length
+ * into the DMA length register.
+ */
+ udc-write_fn(udc-base_address, XUSB_DMA_DSAR_ADDR_OFFSET, src);
+ udc-write_fn(udc-base_address, XUSB_DMA_DDAR_ADDR_OFFSET, dst);
+ udc-write_fn(udc-base_address, XUSB_DMA_LENGTH_OFFSET, length);
+
+ /*
+ * Wait till DMA transaction is complete and
+ * check whether the DMA transaction was
+ * successful.
+ */
+ while ((udc-read_fn(ep-udc-base_address +
XUSB_DMA_STATUS_OFFSET)
+ XUSB_DMA_DMASR_BUSY) == XUSB_DMA_DMASR_BUSY) {
+ timeout = jiffies + 1;
+
+ if (time_after(jiffies, timeout)) {
+ rc = -ETIMEDOUT;
+ goto clean;
+ }
+ }
don't you get an IRQ for DMA completion ? If you do, you could be using
wait_for_completion()
This function is called in interrupt context when buffer is ready or
free. It initiates DMA to transfer data from IP buffer to memory.
Hence it waits in busy loop till DMA completes
wait, so you start_dma() before your gadget driver asks you to ?
in ep_queue driver starts dma transfer from/to IP buffer to/from req-buf.
If transfer is completed then request is not added to ep request queue
and returns from ep_queue.
If transfer is not completed (actual length) then request is added
to queue and returns from ep_queue.
when buffer processed interrupt occurs, handler starts dma if there is
request in queue and calls complete call back (when actual == length)
Hence answer is Yes for some transfers (start dma called from
interrupt handler not from ep_queue).
Thanks,
Sundeep.B.S.
--
balbi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support
Hi,
On Thu, Apr 17, 2014 at 1:38 AM, Paul Zimmerman
paul.zimmer...@synopsys.com wrote:
From: linux-usb-ow...@vger.kernel.org
[mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of sundeep subbaraya
Sent: Wednesday, April 16, 2014 3:39 AM
Hi Felipe,
On Thu, Apr 3, 2014 at 8:28 PM, Felipe Balbi ba...@ti.com wrote:
+static int start_dma(struct xusb_ep *ep, u32 src, u32 dst, u32 length)
please prepend this with xudc_, it makes tracing a lot easier.
+{
+ struct xusb_udc *udc;
+ int rc = 0;
+ unsigned long timeout;
+
+ udc = ep-udc;
+ /*
+ * Set the addresses in the DMA source and
+ * destination registers and then set the length
+ * into the DMA length register.
+ */
+ udc-write_fn(udc-base_address, XUSB_DMA_DSAR_ADDR_OFFSET, src);
+ udc-write_fn(udc-base_address, XUSB_DMA_DDAR_ADDR_OFFSET, dst);
+ udc-write_fn(udc-base_address, XUSB_DMA_LENGTH_OFFSET, length);
+
+ /*
+ * Wait till DMA transaction is complete and
+ * check whether the DMA transaction was
+ * successful.
+ */
+ while ((udc-read_fn(ep-udc-base_address +
XUSB_DMA_STATUS_OFFSET)
+ XUSB_DMA_DMASR_BUSY) == XUSB_DMA_DMASR_BUSY) {
+ timeout = jiffies + 1;
+
+ if (time_after(jiffies, timeout)) {
+ rc = -ETIMEDOUT;
+ goto clean;
+ }
+ }
don't you get an IRQ for DMA completion ? If you do, you could be using
wait_for_completion()
This function is called in interrupt context when buffer is ready or
free. It initiates
DMA to transfer data from IP buffer to memory. Hence it waits in busy
loop till DMA
completes
If this function is called in interrupt context, then you can't use
jiffies for your timeout, since jiffies may not get updated while in
interrupt context.
Yes. It is obvious that this logic is buggy, will change this.
Thanks,
Sundeep.B.S.
--
Paul
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support
On Thu, Apr 17, 2014 at 03:01:37PM +0530, sundeep subbaraya wrote:
Hi Felipe,
On Wed, Apr 16, 2014 at 11:32 PM, Felipe Balbi ba...@ti.com wrote:
Hi,
On Wed, Apr 16, 2014 at 04:09:28PM +0530, sundeep subbaraya wrote:
Hi Felipe,
On Thu, Apr 3, 2014 at 8:28 PM, Felipe Balbi ba...@ti.com wrote:
+static int start_dma(struct xusb_ep *ep, u32 src, u32 dst, u32 length)
please prepend this with xudc_, it makes tracing a lot easier.
+{
+ struct xusb_udc *udc;
+ int rc = 0;
+ unsigned long timeout;
+
+ udc = ep-udc;
+ /*
+ * Set the addresses in the DMA source and
+ * destination registers and then set the length
+ * into the DMA length register.
+ */
+ udc-write_fn(udc-base_address, XUSB_DMA_DSAR_ADDR_OFFSET, src);
+ udc-write_fn(udc-base_address, XUSB_DMA_DDAR_ADDR_OFFSET, dst);
+ udc-write_fn(udc-base_address, XUSB_DMA_LENGTH_OFFSET, length);
+
+ /*
+ * Wait till DMA transaction is complete and
+ * check whether the DMA transaction was
+ * successful.
+ */
+ while ((udc-read_fn(ep-udc-base_address +
XUSB_DMA_STATUS_OFFSET)
+ XUSB_DMA_DMASR_BUSY) == XUSB_DMA_DMASR_BUSY) {
+ timeout = jiffies + 1;
+
+ if (time_after(jiffies, timeout)) {
+ rc = -ETIMEDOUT;
+ goto clean;
+ }
+ }
don't you get an IRQ for DMA completion ? If you do, you could be using
wait_for_completion()
This function is called in interrupt context when buffer is ready or
free. It initiates DMA to transfer data from IP buffer to memory.
Hence it waits in busy loop till DMA completes
wait, so you start_dma() before your gadget driver asks you to ?
in ep_queue driver starts dma transfer from/to IP buffer to/from req-buf.
If transfer is completed then request is not added to ep request queue
and returns from ep_queue.
If transfer is not completed (actual length) then request is added
to queue and returns from ep_queue.
This is wrong. Why wouldn't you give gadget driver the chance to decide
if it needs to queue the request again or not ?
when buffer processed interrupt occurs, handler starts dma if there is
request in queue and calls complete call back (when actual == length)
Hence answer is Yes for some transfers (start dma called from
interrupt handler not from ep_queue).
this also seems wrong(-ish).
1) as Paul pointed out, you can't rely on jiffies if you're calling this
with IRQs disabled.
2) you don't really need to wait until DMA finishes its transaction
before returning from start_dma(), just use the DMA completion IRQ,
3) I don't see anywhere any sanitizing of arguments, can your DMA really
handle any alignment/unaligned addresses or should you make sure you're
getting good arguments?
You need to work on this a little bit, I guess.
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support
Hi Felipe,
On Thu, Apr 3, 2014 at 8:28 PM, Felipe Balbi ba...@ti.com wrote:
+static int start_dma(struct xusb_ep *ep, u32 src, u32 dst, u32 length)
please prepend this with xudc_, it makes tracing a lot easier.
+{
+ struct xusb_udc *udc;
+ int rc = 0;
+ unsigned long timeout;
+
+ udc = ep-udc;
+ /*
+ * Set the addresses in the DMA source and
+ * destination registers and then set the length
+ * into the DMA length register.
+ */
+ udc-write_fn(udc-base_address, XUSB_DMA_DSAR_ADDR_OFFSET, src);
+ udc-write_fn(udc-base_address, XUSB_DMA_DDAR_ADDR_OFFSET, dst);
+ udc-write_fn(udc-base_address, XUSB_DMA_LENGTH_OFFSET, length);
+
+ /*
+ * Wait till DMA transaction is complete and
+ * check whether the DMA transaction was
+ * successful.
+ */
+ while ((udc-read_fn(ep-udc-base_address + XUSB_DMA_STATUS_OFFSET)
+ XUSB_DMA_DMASR_BUSY) == XUSB_DMA_DMASR_BUSY) {
+ timeout = jiffies + 1;
+
+ if (time_after(jiffies, timeout)) {
+ rc = -ETIMEDOUT;
+ goto clean;
+ }
+ }
don't you get an IRQ for DMA completion ? If you do, you could be using
wait_for_completion()
This function is called in interrupt context when buffer is ready or
free. It initiates
DMA to transfer data from IP buffer to memory. Hence it waits in busy
loop till DMA
completes
Thanks,
Sundeep.B.S.
--
balbi
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support
Hi,
On Wed, Apr 16, 2014 at 04:09:28PM +0530, sundeep subbaraya wrote:
Hi Felipe,
On Thu, Apr 3, 2014 at 8:28 PM, Felipe Balbi ba...@ti.com wrote:
+static int start_dma(struct xusb_ep *ep, u32 src, u32 dst, u32 length)
please prepend this with xudc_, it makes tracing a lot easier.
+{
+ struct xusb_udc *udc;
+ int rc = 0;
+ unsigned long timeout;
+
+ udc = ep-udc;
+ /*
+ * Set the addresses in the DMA source and
+ * destination registers and then set the length
+ * into the DMA length register.
+ */
+ udc-write_fn(udc-base_address, XUSB_DMA_DSAR_ADDR_OFFSET, src);
+ udc-write_fn(udc-base_address, XUSB_DMA_DDAR_ADDR_OFFSET, dst);
+ udc-write_fn(udc-base_address, XUSB_DMA_LENGTH_OFFSET, length);
+
+ /*
+ * Wait till DMA transaction is complete and
+ * check whether the DMA transaction was
+ * successful.
+ */
+ while ((udc-read_fn(ep-udc-base_address + XUSB_DMA_STATUS_OFFSET)
+ XUSB_DMA_DMASR_BUSY) == XUSB_DMA_DMASR_BUSY) {
+ timeout = jiffies + 1;
+
+ if (time_after(jiffies, timeout)) {
+ rc = -ETIMEDOUT;
+ goto clean;
+ }
+ }
don't you get an IRQ for DMA completion ? If you do, you could be using
wait_for_completion()
This function is called in interrupt context when buffer is ready or
free. It initiates DMA to transfer data from IP buffer to memory.
Hence it waits in busy loop till DMA completes
wait, so you start_dma() before your gadget driver asks you to ?
--
balbi
signature.asc
Description: Digital signature
RE: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support
From: linux-usb-ow...@vger.kernel.org
[mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of sundeep subbaraya
Sent: Wednesday, April 16, 2014 3:39 AM
Hi Felipe,
On Thu, Apr 3, 2014 at 8:28 PM, Felipe Balbi ba...@ti.com wrote:
+static int start_dma(struct xusb_ep *ep, u32 src, u32 dst, u32 length)
please prepend this with xudc_, it makes tracing a lot easier.
+{
+ struct xusb_udc *udc;
+ int rc = 0;
+ unsigned long timeout;
+
+ udc = ep-udc;
+ /*
+ * Set the addresses in the DMA source and
+ * destination registers and then set the length
+ * into the DMA length register.
+ */
+ udc-write_fn(udc-base_address, XUSB_DMA_DSAR_ADDR_OFFSET, src);
+ udc-write_fn(udc-base_address, XUSB_DMA_DDAR_ADDR_OFFSET, dst);
+ udc-write_fn(udc-base_address, XUSB_DMA_LENGTH_OFFSET, length);
+
+ /*
+ * Wait till DMA transaction is complete and
+ * check whether the DMA transaction was
+ * successful.
+ */
+ while ((udc-read_fn(ep-udc-base_address + XUSB_DMA_STATUS_OFFSET)
+ XUSB_DMA_DMASR_BUSY) == XUSB_DMA_DMASR_BUSY) {
+ timeout = jiffies + 1;
+
+ if (time_after(jiffies, timeout)) {
+ rc = -ETIMEDOUT;
+ goto clean;
+ }
+ }
don't you get an IRQ for DMA completion ? If you do, you could be using
wait_for_completion()
This function is called in interrupt context when buffer is ready or
free. It initiates
DMA to transfer data from IP buffer to memory. Hence it waits in busy
loop till DMA
completes
If this function is called in interrupt context, then you can't use
jiffies for your timeout, since jiffies may not get updated while in
interrupt context.
--
Paul
N�r��yb�X��ǧv�^�){.n�+{��^n�r���z���h��� �G���h�(�階�ݢj���m��z�ޖ���f���h���~�m�
Re: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support
Hi Michal,
On Thu, Apr 3, 2014 at 8:53 PM, Michal Simek mon...@monstr.eu wrote:
+struct xusb_udc {
+struct usb_gadget gadget;
+struct xusb_ep ep[8];
+struct usb_gadget_driver *driver;
+struct cmdbuf ch9cmd;
+u32 usb_state;
+u32 remote_wkp;
+unsigned int (*read_fn)(void __iomem *);
+void (*write_fn)(void __iomem *, u32, u32);
why do you need these to be function pointers ? Because of endianness ?
generic readl()/writel() already take care of that.
readl from asm-generic/io.h is converting value from little endian IO
to cpu endianness.
This IP exists also in big endian version.
It means we have to support all possible combinations.
IP little and reading it on little or big endian CPU
IP big and reading it on little or big endian CPU.
Look below.
+spin_lock_init(udc-lock);
+
+/* Check for IP endianness */
+udc-write_fn = xudc_write32_be;
+udc-read_fn = xudc_read32_be;
+udc-write_fn(udc-base_address, XUSB_TESTMODE_OFFSET, TEST_J);
+if ((udc-read_fn(udc-base_address + XUSB_TESTMODE_OFFSET))
+!= TEST_J) {
+udc-write_fn = xudc_write32;
+udc-read_fn = xudc_read32;
+}
hmm... isn't there a configuration register to check this out ?
Sundeep can tell us if there is any configuration register but
I don't think so in connection to my experience with Xilinx soft IPs.
Yes, there is no configuration register for endianess.
Thanks,
Sundeep.
Endian detection directly on IP itself came from my discussion
on drivers/spi/spi-xilinx.c that this is only one way how to do it.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP - KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support
+struct xusb_udc {
+struct usb_gadget gadget;
+struct xusb_ep ep[8];
+struct usb_gadget_driver *driver;
+struct cmdbuf ch9cmd;
+u32 usb_state;
+u32 remote_wkp;
+unsigned int (*read_fn)(void __iomem *);
+void (*write_fn)(void __iomem *, u32, u32);
why do you need these to be function pointers ? Because of endianness ?
generic readl()/writel() already take care of that.
readl from asm-generic/io.h is converting value from little endian IO
to cpu endianness.
This IP exists also in big endian version.
It means we have to support all possible combinations.
IP little and reading it on little or big endian CPU
IP big and reading it on little or big endian CPU.
Look below.
+spin_lock_init(udc-lock);
+
+/* Check for IP endianness */
+udc-write_fn = xudc_write32_be;
+udc-read_fn = xudc_read32_be;
+udc-write_fn(udc-base_address, XUSB_TESTMODE_OFFSET, TEST_J);
+if ((udc-read_fn(udc-base_address + XUSB_TESTMODE_OFFSET))
+!= TEST_J) {
+udc-write_fn = xudc_write32;
+udc-read_fn = xudc_read32;
+}
hmm... isn't there a configuration register to check this out ?
Sundeep can tell us if there is any configuration register but
I don't think so in connection to my experience with Xilinx soft IPs.
Endian detection directly on IP itself came from my discussion
on drivers/spi/spi-xilinx.c that this is only one way how to do it.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP - KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
signature.asc
Description: OpenPGP digital signature
