Hi Zengtao, On 3/21/2018 6:17 AM, Zengtao (B) wrote: >> -----Original Message----- > >> From: linux-usb-owner@xxxxxxxxxxxxxxx > >> [mailto:linux-usb-owner@xxxxxxxxxxxxxxx] On Behalf Of Minas Harutyunyan > >> Sent: Tuesday, March 20, 2018 10:40 PM > >> To: Zengtao (B) <prime.zeng@xxxxxxxxxxxxx>; Minas Harutyunyan > >> <Minas.Harutyunyan@xxxxxxxxxxxx>; John Youn <John.Youn@xxxxxxxxxxxx>; > >> Felipe Balbi <balbi@xxxxxxxxxx>; Greg Kroah-Hartman > >> <gregkh@xxxxxxxxxxxxxxxxxxx>; linux-usb@xxxxxxxxxxxxxxx > >> Subject: Re: [PATCH v1 2/3] usb: dwc2: Change ISOC DDMA flow > >> > >> Hi Zengtao, > >> > >> On 3/20/2018 6:01 AM, Zengtao (B) wrote: > >>> Hi Minas: > >>> > >>> > >>> > >>> A few minor comments: > >>> > >>> > >>> > >>>> -----Original Message----- > >>> > >>>> From: linux-usb-owner@xxxxxxxxxxxxxxx > >>> > >>>> [mailto:linux-usb-owner@xxxxxxxxxxxxxxx] On Behalf Of Minas > >>>> Harutyunyan > >>> > >>>> Sent: Saturday, March 17, 2018 5:10 PM > >>> > >>>> To: John Youn <John.Youn@xxxxxxxxxxxx>; Felipe Balbi > >>>> <balbi@xxxxxxxxxx>; > >>> > >>>> Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; > >>>> linux-usb@xxxxxxxxxxxxxxx > >>> > >>>> Cc: Minas Harutyunyan <Minas.Harutyunyan@xxxxxxxxxxxx> > >>> > >>>> Subject: [PATCH v1 2/3] usb: dwc2: Change ISOC DDMA flow > >>> > >>>> > >>> > >>>> Changed existing two descriptor-chain flow to one chain. > >>> > >>>> > >>> > >>>> In two-chain implementation BNA interrupt used for switching between > >>>> two > >>> > >>>> chains. BNA interrupt asserted because of returning to beginning of > >>>> the chain > >>> > >>>> based on L-bit of last descriptor. > >>> > >>>> > >>> > >>>> Because of that we lose packets. This issue resolved by using one desc-chain. > >>> > >>>> > >>> > >>>> Removed all staff related to two desc-chain flow from DDMA ISOC > >>>> related > >>> > >>>> functions. > >>> > >>>> > >>> > >>>> Removed request length checking from dwc2_gadget_fill_isoc_desc() > >> function. > >>> > >>>> Request length checking added to dwc2_hsotg_ep_queue() function. If > >>>> request > >>> > >>>> length greater than descriptor limits then request not added to queue. > >>> > >>>> Additional checking done for High Bandwidth ISOC OUT's which not > >>>> supported by > >>> > >>>> driver. In > >>> > >>>> dwc2_gadget_fill_isoc_desc() function also checked desc-chain status > >>>> (full or not) > >>> > >>>> to avoid of reusing not yet processed descriptors. > >>> > >>>> > >>> > >>>> In dwc2_gadget_start_isoc_ddma() function creation of desc-chain > >>>> always > >>> > >>>> started from descriptor 0. Before filling descriptors, they were > >>>> initialized by > >>> > >>>> HOST BUSY status. > >>> > >>>> > >>> > >>>> In dwc2_gadget_complete_isoc_request_ddma() added checking for > >>>> desc-chain > >>> > >>>> rollover. Also added checking completion status. > >>> > >>>> Request completed successfully if DEV_DMA_STS is DEV_DMA_STS_SUCC, > >>> > >>>> otherwise complete with -ETIMEDOUT. > >>> > >>>> > >>> > >>>> Actually removed dwc2_gadget_start_next_isoc_ddma() function because > >>>> now > >>> > >>>> driver use only one desc-chain and instead that function added > >>> > >>>> dwc2_gadget_handle_isoc_bna() function for handling BNA interrupts. > >>> > >>>> > >>> > >>>> Handling BNA interrupt done by flushing TxFIFOs for OUT EPs, > >>>> completing > >>> > >>>> request with -EIO and resetting desc-chain number and target frame to > >>>> initial > >>> > >>>> values for restarting transfers. > >>> > >>>> > >>> > >>>> On handling NAK request completed with -ENODATA. Incremented target > >>>> frame > >>> > >>>> to allow fill desc chain and start transfers. > >>> > >>>> In DDMA mode avoided of frame number incrementing, because tracking > >>>> of > >>> > >>>> frame number performed in dwc2_gadget_fill_isoc_desc() function. > >>> > >>>> > >>> > >>>> When core assert XferCompl along with BNA, we should ignore XferCompl > >>>> in > >>> > >>>> dwc2_hsotg_epint() function. > >>> > >>>> > >>> > >>>> On BNA interrupt replaced dwc2_gadget_start_next_isoc_ddma() by above > >>> > >>>> mentioned BNA handler. > >>> > >>>> > >>> > >>>> In dwc2_hsotg_ep_enable() function added sanity check of bInterval > >>>> for ISOC IN > >>> > >>>> in DDMA mode, because HW not supported EP's with bInterval more than 12. > >>> > >>>> > >>> > >>>> Signed-off-by: Minas Harutyunyan <hminas@xxxxxxxxxxxx> > >>> > >>>> --- > >>> > >>>> drivers/usb/dwc2/core.h | 2 - > >>> > >>>> drivers/usb/dwc2/gadget.c | 235 > >>>> ++++++++++++++++++++++------------------------ > >>> > >>>> 2 files changed, 113 insertions(+), 124 deletions(-) > >>> > >>>> > >>> > >>>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index > >>> > >>>> d83be5651f87..093d078adaf4 100644 > >>> > >>>> --- a/drivers/usb/dwc2/core.h > >>> > >>>> +++ b/drivers/usb/dwc2/core.h > >>> > >>>> @@ -178,7 +178,6 @@ struct dwc2_hsotg_req; > >>> > >>>> * @desc_list_dma: The DMA address of descriptor chain currently in use. > >>> > >>>> * @desc_list: Pointer to descriptor DMA chain head currently in use. > >>> > >>>> * @desc_count: Count of entries within the DMA descriptor chain of EP. > >>> > >>>> - * @isoc_chain_num: Number of ISOC chain currently in use - either 0 or 1. > >>> > >>>> * @next_desc: index of next free descriptor in the ISOC chain under > >>>> SW > >>> > >>>> control. > >>> > >>>> * @total_data: The total number of data bytes done. > >>> > >>>> * @fifo_size: The size of the FIFO (for periodic IN endpoints) @@ > >>>> -231,7 > >>> > >>>> +230,6 @@ struct dwc2_hsotg_ep { > >>> > >>>> struct dwc2_dma_desc *desc_list; > >>> > >>>> u8 desc_count; > >>> > >>>> > >>> > >>>> - unsigned char isoc_chain_num; > >>> > >>>> unsigned int next_desc; > >>> > >>>> > >>> > >>>> char name[10]; > >>> > >>>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > >>>> index > >>> > >>>> c231321656f9..1b9c84cb58fb 100644 > >>> > >>>> --- a/drivers/usb/dwc2/gadget.c > >>> > >>>> +++ b/drivers/usb/dwc2/gadget.c > >>> > >>>> @@ -793,9 +793,7 @@ static void > >>> > >>>> dwc2_gadget_config_nonisoc_xfer_ddma(struct dwc2_hsotg_ep *hs_ep, > >>> > >>>> * @dma_buff: usb requests dma buffer. > >>> > >>>> * @len: usb request transfer length. > >>> > >>>> * > >>> > >>>> - * Finds out index of first free entry either in the bottom or up > >>>> half of > >>> > >>>> - * descriptor chain depend on which is under SW control and not > >>>> processed > >>> > >>>> - * by HW. Then fills that descriptor with the data of the arrived > >>>> usb request, > >>> > >>>> + * Fills next free descriptor with the data of the arrived usb > >>>> + request, > >>> > >>>> * frame info, sets Last and IOC bits increments next_desc. If > >>>> filled > >>> > >>>> * descriptor is not the first one, removes L bit from the previous > >>>> descriptor > >>> > >>>> * status. > >>> > >>>> @@ -810,34 +808,17 @@ static int dwc2_gadget_fill_isoc_desc(struct > >>> > >>>> dwc2_hsotg_ep *hs_ep, > >>> > >>>> u32 mask = 0; > >>> > >>>> > >>> > >>>> maxsize = dwc2_gadget_get_desc_params(hs_ep, &mask); > >>> > >>>> - if (len > maxsize) { > >>> > >>>> - dev_err(hsotg->dev, "wrong len %d\n", len); > >>> > >>>> - return -EINVAL; > >>> > >>>> - } > >>> > >>>> - > >>> > >>>> - /* > >>> > >>>> - * If SW has already filled half of chain, then return and wait for > >>> > >>>> - * the other chain to be processed by HW. > >>> > >>>> - */ > >>> > >>>> - if (hs_ep->next_desc == MAX_DMA_DESC_NUM_GENERIC / 2) > >>> > >>>> - return -EBUSY; > >>> > >>>> > >>> > >>>> - /* Increment frame number by interval for IN */ > >>> > >>>> - if (hs_ep->dir_in) > >>> > >>>> - dwc2_gadget_incr_frame_num(hs_ep); > >>> > >>>> - > >>> > >>>> - index = (MAX_DMA_DESC_NUM_GENERIC / 2) * hs_ep->isoc_chain_num + > >>> > >>>> - hs_ep->next_desc; > >>> > >>>> + index = hs_ep->next_desc; > >>> > >>>> + desc = &hs_ep->desc_list[index]; > >>> > >>>> > >>> > >>>> - /* Sanity check of calculated index */ > >>> > >>>> - if ((hs_ep->isoc_chain_num && index > MAX_DMA_DESC_NUM_GENERIC) > >>> > >>>> || > >>> > >>>> - (!hs_ep->isoc_chain_num && index > > >>> > >>>> MAX_DMA_DESC_NUM_GENERIC / 2)) { > >>> > >>>> - dev_err(hsotg->dev, "wrong index %d for iso chain\n", index); > >>> > >>>> - return -EINVAL; > >>> > >>>> + /* Check if descriptor chain full */ > >>> > >>>> + if ((desc->status >> DEV_DMA_BUFF_STS_SHIFT) == > >>> > >>>> + DEV_DMA_BUFF_STS_HREADY) { > >>> > >>>> + dev_dbg(hsotg->dev, "%s: desc chain full\n", __func__); > >>> > >>>> + return 1; > >>> > >>>> } > >>> > >>>> > >>> > >>>> - desc = &hs_ep->desc_list[index]; > >>> > >>>> - > >>> > >>>> /* Clear L bit of previous desc if more than one entries in the > >>>> chain */ > >>> > >>>> if (hs_ep->next_desc) > >>> > >>>> hs_ep->desc_list[index - 1].status &= ~DEV_DMA_L; @@ -865,8 > >>> > >>> > >>> > >>> When changing the status of the desc, How to sync the SW with the HW? > >>> > >>> Since the HW and SW is working on the same desc synchronously. > >> > >> Not clear for what you mean: HW and SW working on same desc synchronously? > >> Descriptor list is like Producer(SW)/Consumer(HW) list. > >> Index management allow always keep gap between Producer and Consumer > >> indexes. Producer index should be always more than Consumer index. In some > >> cases when Consumer index achieved Producer index, which mean no more > >> ready descriptor to process then BNA interrupt will be asserted, i.e SW can't > >> enough bandwidth to submit new requests. > >> > > > > About my question, > > I mean when you clear the DEV_DMA_L bit of last desc in the chain, the HW may be > > accessing this particular desc at the same time, so perhaps the DEV_DMA_L bit > > clear action is not sync to the HW, and the HW can't work as we want. > > Core always fetching 1 descriptor based on: 1. ISOC IN - one uf before target uf. 2. ISOC OUT - RxFIFO NonEmpty, EP enabled. As I mentioned before, between producer and consumer indexes should be enough gap. If not, then BNA will be asserted, EP will be disabled and will need to restart transfers from scratch based on IN-NAK (ISOC IN) or OUT-EPDis (ISOC OUT) interrupts. > > Per your comment, you mentioned the producer and consumer, but > > How do you do the synchronization between the producer and consumer? > Actually, as soon as consumer complete some desc then dwc2 complete appropriate producers request to function driver then function driver queuing new request. So, initial gap will be kept. > > >>> > >>>> > >>>> +846,14 @@ static int dwc2_gadget_fill_isoc_desc(struct dwc2_hsotg_ep > >>> > >>>> *hs_ep, > >>> > >>>> desc->status &= ~DEV_DMA_BUFF_STS_MASK; > >>> > >>>> desc->status |= (DEV_DMA_BUFF_STS_HREADY << > >>> > >>>> DEV_DMA_BUFF_STS_SHIFT); > >>> > >>>> > >>> > >>>> + /* Increment frame number by interval for IN */ > >>> > >>>> + if (hs_ep->dir_in) > >>> > >>>> + dwc2_gadget_incr_frame_num(hs_ep); > >>> > >>>> + > >>> > >>>> /* Update index of last configured entry in the chain */ > >>> > >>>> hs_ep->next_desc++; > >>> > >>>> + if (hs_ep->next_desc >= MAX_DMA_DESC_NUM_GENERIC) > >>> > >>>> + hs_ep->next_desc = 0; > >>> > >>>> > >>> > >>>> return 0; > >>> > >>>> } > >>> > >>>> @@ -875,11 +862,8 @@ static int dwc2_gadget_fill_isoc_desc(struct > >>> > >>>> dwc2_hsotg_ep *hs_ep, > >>> > >>>> * dwc2_gadget_start_isoc_ddma - start isochronous transfer in DDMA > >>> > >>>> * @hs_ep: The isochronous endpoint. > >>> > >>>> * > >>> > >>>> - * Prepare first descriptor chain for isochronous endpoints. > >>>> Afterwards > >>> > >>>> + * Prepare descriptor chain for isochronous endpoints. Afterwards > >>> > >>>> * write DMA address to HW and enable the endpoint. > >>> > >>>> - * > >>> > >>>> - * Switch between descriptor chains via isoc_chain_num to give SW > >>>> opportunity > >>> > >>>> - * to prepare second descriptor chain while first one is being processed by > >> HW. > >>> > >>>> */ > >>> > >>>> static void dwc2_gadget_start_isoc_ddma(struct dwc2_hsotg_ep *hs_ep) > >>> > >>>> { @@ -890,19 +874,27 @@ static void > >>>> dwc2_gadget_start_isoc_ddma(struct > >>> > >>>> dwc2_hsotg_ep *hs_ep) > >>> > >>>> u32 dma_reg; > >>> > >>>> u32 depctl; > >>> > >>>> u32 ctrl; > >>> > >>>> + struct dwc2_dma_desc *desc; > >>> > >>>> > >>> > >>>> if (list_empty(&hs_ep->queue)) { > >>> > >>>> dev_dbg(hsotg->dev, "%s: No requests in queue\n", __func__); > >>> > >>>> return; > >>> > >>>> } > >>> > >>>> > >>> > >>>> + /* Initialize descriptor chain by Host Busy status */ > >>> > >>>> + for (ret = 0; ret < MAX_DMA_DESC_NUM_GENERIC; ret++) { > >>> > >>>> + desc = &hs_ep->desc_list[ret]; > >>> > >>>> + desc->status = 0; > >>> > >>>> + desc->status |= (DEV_DMA_BUFF_STS_HBUSY > >>> > >>>> + << DEV_DMA_BUFF_STS_SHIFT); > >>> > >>>> + } > >>> > >>>> + > >>> > >>> > >>> > >>> Ret is not a good naming as the loop counter. > >>> > >> > >> Could be, but it just reuse existing variable instead to define new one. > >> > > > > From the code reader's perspective, it is a bit strange. > Ok, will declare new variable. > > >>> > >>> > >>>> + hs_ep->next_desc = 0; > >>> > >>>> list_for_each_entry_safe(hs_req, treq, &hs_ep->queue, queue) { > >>> > >>> > >>> > >>> Do we really need safe function here? We have already have the spinlock. > >>> > >> I left existing list_for_each_entry_safe() not of part of this patch. > >> > >>> > >>> > >>>> ret = dwc2_gadget_fill_isoc_desc(hs_ep, hs_req->req.dma, > >>> > >>>> hs_req->req.length); > >>> > >>>> - if (ret) { > >>> > >>>> - dev_dbg(hsotg->dev, "%s: desc chain full\n", __func__); > >>> > >>>> + if (ret) > >>> > >>>> break; > >>> > >>>> - } > >>> > >>>> } > >>> > >>>> > >>> > >>>> depctl = hs_ep->dir_in ? DIEPCTL(index) : DOEPCTL(index); @@ -914,10 > >>> > >>>> +906,6 @@ static void dwc2_gadget_start_isoc_ddma(struct > >>>> +dwc2_hsotg_ep > >>> > >>>> *hs_ep) > >>> > >>>> ctrl = dwc2_readl(hsotg->regs + depctl); > >>> > >>>> ctrl |= DXEPCTL_EPENA | DXEPCTL_CNAK; > >>> > >>>> dwc2_writel(ctrl, hsotg->regs + depctl); > >>> > >>>> - > >>> > >>>> - /* Switch ISOC descriptor chain number being processed by SW*/ > >>> > >>>> - hs_ep->isoc_chain_num = (hs_ep->isoc_chain_num ^ 1) & 0x1; > >>> > >>>> - hs_ep->next_desc = 0; > >>> > >>>> } > >>> > >>>> > >>> > >>>> /** > >>> > >>>> @@ -1291,6 +1279,9 @@ static int dwc2_hsotg_ep_queue(struct usb_ep > >>>> *ep, > >>> > >>>> struct usb_request *req, > >>> > >>>> struct dwc2_hsotg *hs = hs_ep->parent; > >>> > >>>> bool first; > >>> > >>>> int ret; > >>> > >>>> + u32 maxsize = 0; > >>> > >>>> + u32 mask = 0; > >>> > >>>> + > >>> > >>>> > >>> > >>>> dev_dbg(hs->dev, "%s: req %p: %d@%p, noi=%d, zero=%d, snok=%d\n", > >>> > >>>> ep->name, req, req->length, req->buf, req->no_interrupt, @@ -1308,6 > >>> > >>>> +1299,24 @@ static int dwc2_hsotg_ep_queue(struct usb_ep *ep, struct > >>> > >>>> usb_request *req, > >>> > >>>> req->actual = 0; > >>> > >>>> req->status = -EINPROGRESS; > >>> > >>>> > >>> > >>>> + /* In DDMA mode for ISOC's don't queue request if length greater > >>> > >>>> + * than descriptor limits. > >>> > >>>> + */ > >>> > >>>> + if (using_desc_dma(hs) && hs_ep->isochronous) { > >>> > >>>> + maxsize = dwc2_gadget_get_desc_params(hs_ep, &mask); > >>> > >>>> + if (hs_ep->dir_in && req->length > maxsize) { > >>> > >>>> + dev_err(hs->dev, "wrong length %d (maxsize=%d)\n", > >>> > >>>> + req->length, maxsize); > >>> > >>>> + return -EINVAL; > >>> > >>>> + } > >>> > >>>> + /* ISOC OUT high bandwidth not supported */ > >>> > >>>> + if (!hs_ep->dir_in && req->length > hs_ep->ep.maxpacket) { > >>> > >>>> + dev_err(hs->dev, "ISOC OUT: wrong length %d (mps=%d)\n", > >>> > >>>> + req->length, hs_ep->ep.maxpacket); > >>> > >>>> + return -EINVAL; > >>> > >>>> + } > >>> > >>>> + } > >>> > >>>> + > >>> > >>>> ret = dwc2_hsotg_handle_unaligned_buf_start(hs, hs_ep, hs_req); > >>> > >>>> if (ret) > >>> > >>>> return ret; > >>> > >>>> @@ -1330,7 +1339,7 @@ static int dwc2_hsotg_ep_queue(struct usb_ep > >>>> *ep, > >>> > >>>> struct usb_request *req, > >>> > >>>> > >>> > >>>> /* > >>> > >>>> * Handle DDMA isochronous transfers separately - just add new entry > >>> > >>>> - * to the half of descriptor chain that is not processed by HW. > >>> > >>>> + * to the descriptor chain. > >>> > >>>> * Transfer will be started once SW gets either one of NAK or > >>> > >>>> * OutTknEpDis interrupts. > >>> > >>>> */ > >>> > >>>> @@ -1338,9 +1347,9 @@ static int dwc2_hsotg_ep_queue(struct usb_ep > >>>> *ep, > >>> > >>>> struct usb_request *req, > >>> > >>>> hs_ep->target_frame != TARGET_FRAME_INITIAL) { > >>> > >>>> ret = dwc2_gadget_fill_isoc_desc(hs_ep, hs_req->req.dma, > >>> > >>>> hs_req->req.length); > >>> > >>>> - if (ret) > >>> > >>>> - dev_dbg(hs->dev, "%s: ISO desc chain full\n", __func__); > >>> > >>>> - > >>> > >>>> + if (ret < 0) > >>> > >>>> + dev_dbg(hs->dev, "%s: failed to fill isoc desc\n", > >>> > >>>> + __func__); > >>> > >>> > >>> > >>> dwc2_gadget_fill_isoc_desc will never return a negative value, and at > >>> the same > >>> > >>> time I think there is no need to check the return value, we can work > >>> properly even > >>> > >>> the desc chain is full. > >>> > >> You are rigth. ret is 0 or 1. So, no need to change existing code. Yes, we can > >> continue work properly even if the desc chain full. It just debug message if (ret != > >> 0). Do you have objection? > >> > > > > Since we already have debug message inside dwc2_gadget_fill_isoc_desc, so here > > we don't need additional debug message, what do you think of it? > > Agree with you, will remove. > > > >> > >>> > >>> > >>>> return 0; > >>> > >>>> } > >>> > >>>> > >>> > >>>> @@ -2011,10 +2020,9 @@ static void > >> dwc2_hsotg_complete_request(struct > >>> > >>>> dwc2_hsotg *hsotg, > >>> > >>>> * @hs_ep: The endpoint the request was on. > >>> > >>>> * > >>> > >>>> * Get first request from the ep queue, determine descriptor on > >>>> which > >>> > >>>> complete > >>> > >>>> - * happened. SW based on isoc_chain_num discovers which half of the > >>> > >>>> descriptor > >>> > >>>> - * chain is currently in use by HW, adjusts dma_address and > >>>> calculates index > >>> > >>>> - * of completed descriptor based on the value of DEPDMA register. > >>>> Update > >>> > >>>> actual > >>> > >>>> - * length of request, giveback to gadget. > >>> > >>>> + * happened. SW discovers which descriptor currently in use by HW, > >>> > >>>> + adjusts > >>> > >>>> + * dma_address and calculates index of completed descriptor based on > >>> > >>>> + the value > >>> > >>>> + * of DEPDMA register. Update actual length of request, giveback to gadget. > >>> > >>>> */ > >>> > >>>> static void dwc2_gadget_complete_isoc_request_ddma(struct > >>>> dwc2_hsotg_ep > >>> > >>>> *hs_ep) { @@ -2037,82 +2045,55 @@ static void > >>> > >>>> dwc2_gadget_complete_isoc_request_ddma(struct dwc2_hsotg_ep *hs_ep) > >>> > >>>> > >>> > >>>> dma_addr = hs_ep->desc_list_dma; > >>> > >>>> > >>> > >>>> - /* > >>> > >>>> - * If lower half of descriptor chain is currently use by SW, > >>> > >>>> - * that means higher half is being processed by HW, so shift > >>> > >>>> - * DMA address to higher half of descriptor chain. > >>> > >>>> - */ > >>> > >>>> - if (!hs_ep->isoc_chain_num) > >>> > >>>> - dma_addr += sizeof(struct dwc2_dma_desc) * > >>> > >>>> - (MAX_DMA_DESC_NUM_GENERIC / 2); > >>> > >>>> - > >>> > >>>> dma_reg = hs_ep->dir_in ? DIEPDMA(hs_ep->index) : > >>> > >>>> DOEPDMA(hs_ep->index); > >>> > >>>> depdma = dwc2_readl(hsotg->regs + dma_reg); > >>> > >>>> > >>> > >>>> index = (depdma - dma_addr) / sizeof(struct dwc2_dma_desc) - 1; > >>> > >>>> - desc_sts = hs_ep->desc_list[index].status; > >>> > >>>> + /* Check descriptor chain rollover */ > >>> > >>>> + if (index < 0) > >>> > >>>> + index = MAX_DMA_DESC_NUM_GENERIC - 1; > >>> > >>>> > >>> > >>> > >>> > >>> I don't understand here, why setting the index to > >>> > >>> MAX_DMA_DESC_NUM_GENERIC - 1 when the chain is rollover? > >>> > >>> If the chain is rollover, the index should point to the latest > >>> finished desc, > >>> > >>> and all the finished descs should be processed. And even the chain is > >>> rollover, > >>> > >>> the count of descs in chain is maybe less then > >> MAX_DMA_DESC_NUM_GENERIC. > >>> > >> If depdma point to first desc in list, that mean the last processed desc was last > >> in the chain. In this case index will be negative. > >> In case of desc count less than MAX_DMA_DESC_NUM_GENERIC then rollover > >> can be happen because of L-bit set. In this case by processing first desc will be > >> asserted BNA interrupt but not XferComplete interrupt. > >> > > > > Can we reach here in BNA interrupt, or can we have both BNA and XferComplete > > Interrupts asserted at the same time? > > Your conclusion is based on the following assumption: > > 1. BNA interrupt handle flow can't go into here. > > 2. when the execution flow going to here, only because of XferComplete interrupt. > > 3. when the XferComplete is asserted, BNA interrupt can't be asserted. > > In case of both interrupt asserted then XferComplete ignored. See dwc2_hsotg_epint() function. Actually, in my test cases with bInterval=1 it happen on last descriptor and no any new requests from function driver, because EP disabled with some latency in core BNA also asserted. But its corner case and can be ignored. > >>> > >>> > >>>> - mask = hs_ep->dir_in ? DEV_DMA_ISOC_TX_NBYTES_MASK : > >>> > >>>> - DEV_DMA_ISOC_RX_NBYTES_MASK; > >>> > >>>> - ureq->actual = ureq->length - > >>> > >>>> - ((desc_sts & mask) >> DEV_DMA_ISOC_NBYTES_SHIFT); > >>> > >>>> - > >>> > >>>> - /* Adjust actual length for ISOC Out if length is not align of 4 */ > >>> > >>>> - if (!hs_ep->dir_in && ureq->length & 0x3) > >>> > >>>> - ureq->actual += 4 - (ureq->length & 0x3); > >>> > >>>> + desc_sts = hs_ep->desc_list[index].status; > >>> > >>>> + /* Check completion status */ > >>> > >>>> + if ((desc_sts & DEV_DMA_STS_MASK) >> DEV_DMA_STS_SHIFT == > >>> > >>>> + DEV_DMA_STS_SUCC) { > >>> > >>>> + mask = hs_ep->dir_in ? DEV_DMA_ISOC_TX_NBYTES_MASK : > >>> > >>>> + DEV_DMA_ISOC_RX_NBYTES_MASK; > >>> > >>>> + ureq->actual = ureq->length - > >>> > >>>> + ((desc_sts & mask) >> > >>> > >>>> + DEV_DMA_ISOC_NBYTES_SHIFT); > >>> > >>>> + > >>> > >>>> + /* Adjust actual len for ISOC Out if len is not align of 4 */ > >>> > >>>> + if (!hs_ep->dir_in && ureq->length & 0x3) > >>> > >>>> + ureq->actual += 4 - (ureq->length & 0x3); > >>> > >>>> > >>> > >>>> - dwc2_hsotg_complete_request(hsotg, hs_ep, hs_req, 0); > >>> > >>>> + dwc2_hsotg_complete_request(hsotg, hs_ep, hs_req, 0); > >>> > >>>> + } else { > >>> > >>>> + dwc2_hsotg_complete_request(hsotg, hs_ep, hs_req, -ETIMEDOUT); > >>> > >>>> + } > >>> > >>>> } > >>> > >>>> > >>> > >>>> /* > >>> > >>>> - * dwc2_gadget_start_next_isoc_ddma - start next isoc request, if any. > >>> > >>>> - * @hs_ep: The isochronous endpoint to be re-enabled. > >>> > >>>> + * dwc2_gadget_handle_isoc_bna - handle BNA interrupt for ISOC. > >>> > >>>> + * @hs_ep: The isochronous endpoint. > >>> > >>>> * > >>> > >>>> - * If ep has been disabled due to last descriptor servicing (IN endpoint) or > >>> > >>>> - * BNA (OUT endpoint) check the status of other half of descriptor chain that > >>> > >>>> - * was under SW control till HW was busy and restart the endpoint if needed. > >>> > >>>> + * Complete request with -EIO. > >>> > >>>> + * If EP ISOC OUT then need to flush RX FIFO to remove source of BNA > >>> > >>>> + * interrupt. Reset target frame and next_desc to allow to start > >>> > >>>> + * ISOC's on NAK interrupt for IN direction or on OUTTKNEPDIS > >>> > >>>> + * interrupt for OUT direction. > >>> > >>>> */ > >>> > >>>> -static void dwc2_gadget_start_next_isoc_ddma(struct dwc2_hsotg_ep > >>> > >>>> *hs_ep) > >>> > >>>> +static void dwc2_gadget_handle_isoc_bna(struct dwc2_hsotg_ep *hs_ep) > >>> > >>>> { > >>> > >>>> struct dwc2_hsotg *hsotg = hs_ep->parent; > >>> > >>>> - u32 depctl; > >>> > >>>> - u32 dma_reg; > >>> > >>>> - u32 ctrl; > >>> > >>>> - u32 dma_addr = hs_ep->desc_list_dma; > >>> > >>>> - unsigned char index = hs_ep->index; > >>> > >>>> - > >>> > >>>> - dma_reg = hs_ep->dir_in ? DIEPDMA(index) : DOEPDMA(index); > >>> > >>>> - depctl = hs_ep->dir_in ? DIEPCTL(index) : DOEPCTL(index); > >>> > >>>> > >>> > >>>> - ctrl = dwc2_readl(hsotg->regs + depctl); > >>> > >>>> + if (!hs_ep->dir_in) > >>> > >>>> + dwc2_flush_rx_fifo(hsotg); > >>> > >>>> + dwc2_hsotg_complete_request(hsotg, hs_ep, get_ep_head(hs_ep), > >>> > >>>> + -EIO); > >>> > >>>> > >>> > >>>> - /* > >>> > >>>> - * EP was disabled if HW has processed last descriptor or BNA was set. > >>> > >>>> - * So restart ep if SW has prepared new descriptor chain in ep_queue > >>> > >>>> - * routine while HW was busy. > >>> > >>>> - */ > >>> > >>>> - if (!(ctrl & DXEPCTL_EPENA)) { > >>> > >>>> - if (!hs_ep->next_desc) { > >>> > >>>> - dev_dbg(hsotg->dev, "%s: No more ISOC requests\n", > >>> > >>>> - __func__); > >>> > >>>> - return; > >>> > >>>> - } > >>> > >>>> - > >>> > >>>> - dma_addr += sizeof(struct dwc2_dma_desc) * > >>> > >>>> - (MAX_DMA_DESC_NUM_GENERIC / 2) * > >>> > >>>> - hs_ep->isoc_chain_num; > >>> > >>>> - dwc2_writel(dma_addr, hsotg->regs + dma_reg); > >>> > >>>> - > >>> > >>>> - ctrl |= DXEPCTL_EPENA | DXEPCTL_CNAK; > >>> > >>>> - dwc2_writel(ctrl, hsotg->regs + depctl); > >>> > >>>> - > >>> > >>>> - /* Switch ISOC descriptor chain number being processed by SW*/ > >>> > >>>> - hs_ep->isoc_chain_num = (hs_ep->isoc_chain_num ^ 1) & 0x1; > >>> > >>>> - hs_ep->next_desc = 0; > >>> > >>>> - > >>> > >>>> - dev_dbg(hsotg->dev, "%s: Restarted isochronous endpoint\n", > >>> > >>>> - __func__); > >>> > >>>> - } > >>> > >>>> + hs_ep->target_frame = TARGET_FRAME_INITIAL; > >>> > >>>> + hs_ep->next_desc = 0; > >>> > >>>> } > >>> > >>>> > >>> > >>>> /** > >>> > >>>> @@ -2816,18 +2797,25 @@ static void dwc2_gadget_handle_nak(struct > >>> > >>>> dwc2_hsotg_ep *hs_ep) { > >>> > >>>> struct dwc2_hsotg *hsotg = hs_ep->parent; > >>> > >>>> int dir_in = hs_ep->dir_in; > >>> > >>>> + u32 tmp; > >>> > >>>> > >>> > >>>> if (!dir_in || !hs_ep->isochronous) > >>> > >>>> return; > >>> > >>>> > >>> > >>>> if (hs_ep->target_frame == TARGET_FRAME_INITIAL) { > >>> > >>>> - hs_ep->target_frame = dwc2_hsotg_read_frameno(hsotg); > >>> > >>>> > >>> > >>>> + tmp = dwc2_hsotg_read_frameno(hsotg); > >>> > >>> > >>> > >>> I think there is no need to introduce tmp, and the original is all right. > >>> > >> No, tmp required. Reading current frame number should be done as soon as > >> possible. This is why it's stored in tmp and then used for below cases: > >> 1. DDMA mode. On dwc2_hsotg_complete_request() function driver > >> immediatly queued new request and as result target_frame incremented by > >> interval. > >> 2. BDMA mode. tmp required if interval > 1. > >>> > > > > So for both DDMA or BDMA mode, hs_ep->target_frame should be updated the > > Current frame number in HW. > > The original code can cover both branch, right? > > Original code doesn't increment here target frame number (performing in fill_isoc) to start transfers from next interval and doesn't complete request. > >>> > >>>> if (using_desc_dma(hsotg)) { > >>> > >>>> + dwc2_hsotg_complete_request(hsotg, hs_ep, > >>> > >>>> + get_ep_head(hs_ep), > >>> > >>>> + -ENODATA); > >>> > >>>> + hs_ep->target_frame = tmp; > >>> > >>>> + dwc2_gadget_incr_frame_num(hs_ep); > >>> > >>>> dwc2_gadget_start_isoc_ddma(hs_ep); > >>> > >>>> return; > >>> > >>>> } > >>> > >>>> > >>> > >>>> + hs_ep->target_frame = tmp; > >>> > >>>> if (hs_ep->interval > 1) { > >>> > >>>> u32 ctrl = dwc2_readl(hsotg->regs + > >>> > >>>> DIEPCTL(hs_ep->index)); > >>> > >>>> @@ -2843,7 +2831,8 @@ static void dwc2_gadget_handle_nak(struct > >>> > >>>> dwc2_hsotg_ep *hs_ep) > >>> > >>>> get_ep_head(hs_ep), 0); > >>> > >>>> } > >>> > >>>> > >>> > >>>> - dwc2_gadget_incr_frame_num(hs_ep); > >>> > >>>> + if (!using_desc_dma(hsotg)) > >>> > >>>> + dwc2_gadget_incr_frame_num(hs_ep); > >>> > >>>> } > >>> > >>>> > >>> > >>>> /** > >>> > >>>> @@ -2901,9 +2890,9 @@ static void dwc2_hsotg_epint(struct dwc2_hsotg > >>> > >>>> *hsotg, unsigned int idx, > >>> > >>>> > >>> > >>>> /* In DDMA handle isochronous requests separately */ > >>> > >>>> if (using_desc_dma(hsotg) && hs_ep->isochronous) { > >>> > >>>> - dwc2_gadget_complete_isoc_request_ddma(hs_ep); > >>> > >>>> - /* Try to start next isoc request */ > >>> > >>>> - dwc2_gadget_start_next_isoc_ddma(hs_ep); > >>> > >>>> + /* XferCompl set along with BNA */ > >>> > >>>> + if (!(ints & DXEPINT_BNAINTR)) > >>> > >>>> + dwc2_gadget_complete_isoc_request_ddma(hs_ep); > >>> > >>>> } else if (dir_in) { > >>> > >>>> /* > >>> > >>>> * We get OutDone from the FIFO, so we only @@ -2978,15 > >>> > >>>> +2967,8 @@ static void dwc2_hsotg_epint(struct dwc2_hsotg *hsotg, > >> unsigned > >>> > >>>> int idx, > >>> > >>>> > >>> > >>>> if (ints & DXEPINT_BNAINTR) { > >>> > >>>> dev_dbg(hsotg->dev, "%s: BNA interrupt\n", __func__); > >>> > >>>> - > >>> > >>>> - /* > >>> > >>>> - * Try to start next isoc request, if any. > >>> > >>>> - * Sometimes the endpoint remains enabled after BNA interrupt > >>> > >>>> - * assertion, which is not expected, hence we can enter here > >>> > >>>> - * couple of times. > >>> > >>>> - */ > >>> > >>>> if (hs_ep->isochronous) > >>> > >>>> - dwc2_gadget_start_next_isoc_ddma(hs_ep); > >>> > >>>> + dwc2_gadget_handle_isoc_bna(hs_ep); > >>> > >>>> } > >>> > >>>> > >>> > >>>> if (dir_in && !hs_ep->isochronous) { > >>> > >>>> @@ -3791,6 +3773,7 @@ static int dwc2_hsotg_ep_enable(struct usb_ep > >> *ep, > >>> > >>>> unsigned int dir_in; > >>> > >>>> unsigned int i, val, size; > >>> > >>>> int ret = 0; > >>> > >>>> + unsigned char ep_type; > >>> > >>>> > >>> > >>>> dev_dbg(hsotg->dev, > >>> > >>>> "%s: ep %s: a 0x%02x, attr 0x%02x, mps 0x%04x, intr %d\n", @@ > >>> > >>>> -3809,6 +3792,15 @@ static int dwc2_hsotg_ep_enable(struct usb_ep *ep, > >>> > >>>> return -EINVAL; > >>> > >>>> } > >>> > >>>> > >>> > >>>> + ep_type = desc->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK; > >>> > >>>> + /* ISOC DDMA supported bInterval up to 12 */ > >>> > >>>> + if (using_desc_dma(hsotg) && ep_type == USB_ENDPOINT_XFER_ISOC && > >>> > >>>> + dir_in && desc->bInterval > 12) { > >>> > >>>> + dev_err(hsotg->dev, > >>> > >>>> + "%s: ISOC IN: bInterval>12 not supported!\n", __func__); > >>> > >>>> + return -EINVAL; > >>> > >>>> + } > >>> > >>>> + > >>> > >>>> mps = usb_endpoint_maxp(desc); > >>> > >>>> mc = usb_endpoint_maxp_mult(desc); > >>> > >>>> > >>> > >>>> @@ -3852,14 +3844,13 @@ static int dwc2_hsotg_ep_enable(struct usb_ep > >>> > >>>> *ep, > >>> > >>>> hs_ep->halted = 0; > >>> > >>>> hs_ep->interval = desc->bInterval; > >>> > >>>> > >>> > >>>> - switch (desc->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) { > >>> > >>>> + switch (ep_type) { > >>> > >>>> case USB_ENDPOINT_XFER_ISOC: > >>> > >>>> epctrl |= DXEPCTL_EPTYPE_ISO; > >>> > >>>> epctrl |= DXEPCTL_SETEVENFR; > >>> > >>>> hs_ep->isochronous = 1; > >>> > >>>> hs_ep->interval = 1 << (desc->bInterval - 1); > >>> > >>>> hs_ep->target_frame = TARGET_FRAME_INITIAL; > >>> > >>>> - hs_ep->isoc_chain_num = 0; > >>> > >>>> hs_ep->next_desc = 0; > >>> > >>>> if (dir_in) { > >>> > >>>> hs_ep->periodic = 1; > >>> > >>>> -- > >>> > >>>> 2.11.0 > >>> > >>>> > >>> > >>>> -- > >>> > >>>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body > >> of > >>> > >>>> a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at > >>> > >>>> > >> https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordo > >> mo-2Dinfo.html&d=DwIGaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=6z9Al9FrHR_Zqb > >> btSAsD16pvOL2S3XHxQnSzq8kusyI&m=aPXXRmF_CWLjH-UpIejOss2qsaYVAMO- > >> oeSoNd1iTmg&s=RtquFAV3zcz956KK1FiPPWA43kJl9InZKrc3jWQR59s&e= > >>> > >>> > >> > >> Thank you for review. > >> > >> Minas > >> -- > >> To unsubscribe from this list: send the line "unsubscribe linux-usb" in > >> the body of a message to majordomo@xxxxxxxxxxxxxxx > >> More majordomo info at https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIGaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=6z9Al9FrHR_ZqbbtSAsD16pvOL2S3XHxQnSzq8kusyI&m=WfGU1k4UBRap5Y7jA2LB6Q9JV5b8FXNrmgg3ZvnV7V4&s=ZtDDEAhbXXQqBblOSfwtEDMXZqRRC3_6wLgCJREr2EQ&e= > > Thanks, Minas -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
