On Tue, Sep 11, 2012 at 03:08:20PM +0530, Pratyush Anand wrote: > On 9/11/2012 2:39 PM, Felipe Balbi wrote: > >On Tue, Sep 11, 2012 at 12:29:36PM +0530, Pratyush Anand wrote: > >>On 9/10/2012 6:49 PM, Felipe Balbi wrote: > >>>On Mon, Sep 10, 2012 at 06:45:06PM +0530, Pratyush Anand wrote: > >>>>Hi Felip, > >>>> > >>>>Thanks for the clarifications. > >>>> > >>>>On 9/10/2012 6:29 PM, Felipe Balbi wrote: > >>>>>>If my understanding is correct, then I might need to modify dwc3 > >>>>>>>driver a bit. only first TRB of the service interval should have > >>>>>>>TRBCTL as ISOC_FIRST rest should have TRBCTL as ISOC. > >>>>>Why ? IIRC, ISOC_FIRST was a hint to the internal packet scheduler to > >>>>>give higher priority to the isochronous packet, right ? Does it make any > >>>>>difference for your use case ? > >>>> > >>>>Databook says: > >>>> > >>>> > >>>>"The first TRB in a Buffer Descriptor must have the TRBCTL field set > >>>>to the “Isochronous-First” type while all others have this field set > >>>>to “Isochronous”." > >>> > >>>aaa true :-) I had forgotten about that extra bit of information when > >>>using chained TRBs :-s My bad. Please send a patch. > >>> > >>>>So I thought to modify the code. As of now, ISOC IN does not work > >>>>fully with either All Isochronous-First or Isochronous First + > >>>>Isochronous implementation (in case of more than one TRB for one > >>>>service interval). > >>>>May be I am doing some mistake in my code. :( > >>>>I am debugging. Will get back with my observations. > >>> > >>>I think you're perfectly right at the need for ISOCHRONOUS_FIRST flag. > >>>My bad. > >>> > >> > >>Hummm.. It works with "Isochronous First + Isochronous" > >>implementation. But, code needs many modification.. > >> > >>We have one trb and one trb_dma per "struct dwc3_request". We take > >>lot of decisions on the basis of req->trb value. These all will > >>change in case of SG. We will not have only one TRB per "struct > >>usb_request". > >> > >>I am going to keep array for trb and trb_dma and then to manage all > >>these with backward compatibility (non SG case). What do you say? > >> > >>struct dwc3_trb *trb[DWC3_TRB_NUM]; > >>dma_addr_t trb_dma[DWC3_TRB_NUM]; > > > >that doesn't look right :-( > > > >Isn't something like below enough ? > > Probably not ;) > It will handle only one part, ie to manage > DWC3_TRBCTL_ISOCHRONOUS_FIRST & DWC3_TRBCTL_ISOCHRONOUS. > > But, problem is that we have single trb associated with each struct > dwc3_trb. In case of SG, we will prepare req->request.num_mapped_sgs > number of TRBs against one request (ie one struct dwc3_trb). > > Each TRBs of SG need to be updated into core cache. During cleanup, > we need to check status of each TRB of SG. > > How can we manage if we have only one "struct dwc3_trb *trb" > associated with struct dwc3_trb. Well, you iterate over each entrie on the scatterlist and you know that TRBs are physically consecutive. Granted, it would be a lot easier if we have one request per-TRB, but this isn't always the case and we need to treat that inside the driver. Looks like dwc3_gadget_cleanup_done_reqs() needs some re-factoring and needs to learn about chained TRBs. Currently it only works if no TRBs fail, I guess... Still, I don't think adding that array to the endpoint is the best solution. Maybe it's enough to just add some check for req->request.num_mapped_sgs and if that's greater than zero, then we need to act differently... I'd start by looking at cleaning up dwc3_gadget_cleanup_done_reqs() since that's a bit obfuscated IMHO. -- balbi
Attachment:
signature.asc
Description: Digital signature
