Hi Zengtao, On 3/22/2018 1:36 PM, Zengtao (B) wrote: > Hi Minas: > >> -----Original Message----- >> From: Minas Harutyunyan [mailto:Minas.Harutyunyan@xxxxxxxxxxxx] >> Sent: Thursday, March 22, 2018 4:06 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/21/2018 2:45 PM, Zengtao (B) wrote: >>> Hi Minas: >>> >>>> -----Original Message----- >>>> From: Minas Harutyunyan [mailto:Minas.Harutyunyan@xxxxxxxxxxxx] >>>> Sent: Wednesday, March 21, 2018 4:08 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/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. >>>>> >>> >>> How do you ensure enough gap between the producer and consumer? >>> The producer fill new desc at random time, and when clearing the >>> DEV_DMA_L bit of the last desc in the chain, interrupts is disabled >>> and we may delay or miss the BNA interrupt. >>> Linux is not a real time system, so I think we should consider such corner >> cases. >>> >> Such corner case considered in driver by handling BNA and restarting transfers >> based on remaining requests in queue, if any. >> > BNA interrupt will report an -EIO error to the function in your 3rd patch, so I think > this is not an IO error and what about returning -ERESTART(you have mentioned in > another mail). > > And the restart transfer the remaining requests in queue in the NAK interrupt, right? > Not agree with you. On BNA restart is performing internally in dwc2 and function driver should be not aware about it. If dwc2 will return -ERESTART it can mean that function driver should restart its internal flow. Based on Filipe feedback, I think, 0 status code with correct actual size (in this case =0) will be best solution. See my feedback in another email thread about status codes. Will send shortly. >> If it happen then there is no index gap, because function driver not able to queue >> new requests on time. If no any requests in queue then >> *list* of descriptors can't be created at all. >> Time gap between 2 new subsequent requests should be similar to interval. If >> function driver can't do it then using descriptor DMA mode not be reasonable, >> buffer DMA mode should be used instead. >> >>>>> 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. >>>>> >>> >>> It is not mandatary for the function driver to queuing new request in >>> the complete handler. we can't depend on the function driver to keep the gap. >>> >>>>> >>>>>>> >>>>> >>>>>>>> >>>>> >>>>>>>> +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. >>>> >>> >>> Then what about adding some assert here to catch the core case? >> >> If both interrupt will be asserted simultanously then we can't be here because >> XferComplete will be ignored and this function will not called. >> >>> >>>>> >>>>>>> >>>>> >>>>>>> >>>>> >>>>>>>> - 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. >>>> >>> >>> Maybe you have misunderstood me, I mean there is no need to add the tmp >> variable. >>> Directly update the hs_ep->target_frame using "hs_ep->target_frame = >>> dwc2_hsotg_read_frameno(hsotg); " instead of "hs_ep->target_frame = >>> tmp" >>> In both branches. >>> >> >> No, if target frame number will be stored by "hs_ep->target_frame = >> dwc2_hsotg_read_frameno(hsotg);" then after calling >> dwc2_hsotg_complete_request(), function driver can enqueue new request >> which increments target_frame by one more additional interval. This is why >> "hs_ep->target_frame = tmp" performing after calling complete request. >> > > Then I got it ,thank you, It 's a bit difficult to understand. But the function driver > maybe queue more than one request in the complete callback, and target_frame > will increase more the one interval? > Please review dwc2_hsotg_ep_queue() function. Only in case if "hs_ep->target_frame != TARGET_FRAME_INITIAL" fill_isoc will called and target_frame will be incremented inside fill_isoc. This is why dwc2 keep target_frame equal to TARGET_FRAME_INITIAL before calling request completion function and then change to actual frame number. Just, FYI. Initially implementation was done as you suggested, but after testing we saw that IN packet go to USB bus instead of (N+1) frame in (N+2), where N is frame number when NAK asserted. > >>>>> >>>>>>> >>>>> >>>>>>>> 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_majordo >>>> >> mo-2Dinfo.html&d=DwIGaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=6z9Al9FrHR_Zqb >>>> >> btSAsD16pvOL2S3XHxQnSzq8kusyI&m=WfGU1k4UBRap5Y7jA2LB6Q9JV5b8FXNr >>>> >> mgg3ZvnV7V4&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
