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? >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? >>>> >>>>>> >>>> >>>>>>> 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 > ��.n��������+%������w��{.n�����{���)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
