Re: [RFC v2] usb: xhci: add Immediate Data Transfer support

[Felipe Balbi]

Hi,

Nicolas Saenz Julienne <nsaenzjulienne@xxxxxxx> writes:
>> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> > index 005e65922608..dec62f7f5dc8 100644
>> > --- a/drivers/usb/host/xhci.c
>> > +++ b/drivers/usb/host/xhci.c
>> > @@ -1238,6 +1238,21 @@ EXPORT_SYMBOL_GPL(xhci_resume);
>> >  
>> >  /*-------------------------------------------------------------------------
>> > */
>> >  
>> > +/*
>> > + * Bypass the DMA mapping if URB is suitable for Immediate Transfer (IDT),
>> > + * we'll copy the actual data into the TRB address register. This is
>> > limited to
>> > + * transfers up to 8 bytes on output endpoints of any kind with
>> > wMaxPacketSize
>> > + * >= 8 bytes.
>> > + */
>> > +static int xhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
>> > +				gfp_t mem_flags)
>> > +{
>> > +	if (xhci_urb_suitable_for_idt(urb))
>> > +		return 0;
>> > +
>> > +	return usb_hcd_map_urb_for_dma(hcd, urb, mem_flags);
>> > +}
>> 
>> don't you need a matching unmap_urb_for_dma()??
>
> Not really as every DMA mapping sets a matching URB flag to track it. For
> example when usb_hcd_map_urb_for_dma() uses dma_map_single() it will set
> URB_DMA_MAP_SINGLE in urb->transfer_flags, later on unmap_urb_for_dma() will
> catch it and unmap it. As I bypass the mapping altogether there are no
> flags set, so unmap_urb_for_dma() won't have any effect.
>
> I could still add it for clarity, and well, I guess it'll save some
> instructions on the IDT suitable side.

thanks for the clarification.

>> > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>> > index 652dc36e3012..9d77b0901ab7 100644
>> > --- a/drivers/usb/host/xhci.h
>> > +++ b/drivers/usb/host/xhci.h
>> > @@ -1295,6 +1295,8 @@ enum xhci_setup_dev {
>> >  #define TRB_IOC			(1<<5)
>> >  /* The buffer pointer contains immediate data */
>> >  #define TRB_IDT			(1<<6)
>> > +/* TDs smaller than this might use IDT */
>> 
>> Technically, "TDs at most this" since you're 8 itself is an allowed

heh, I made a mess on this sentence, but I guess you got the gist of it.

cheers

-- 
balbi

Attachment: signature.asc
Description: PGP signature