About XHCI_TD_PAGE_SIZE.

[Kohji Okuno]

From: Hans Petter Selasky <hps at selasky.org>
Subject: Re: About XHCI_TD_PAGE_SIZE.
Date: Mon, 22 Dec 2014 10:10:46 +0100
Message-ID: <5497E016.7020809 at selasky.org>

> On 12/22/14 02:38, Kohji Okuno wrote:
>> From: Hans Petter Selasky <hps at selasky.org>
>> Subject: Re: About XHCI_TD_PAGE_SIZE.
>> Date: Sat, 20 Dec 2014 10:30:36 +0100
>>
>>> On 12/17/14 05:42, Nidal Khalil wrote:
>>>> I agree. Thanks
>>>>
>>>> Nidal
>>>> On Dec 16, 2014 6:25 PM, "Kohji Okuno" <okuno.kohji at jp.panasonic.com>
>>>> wrote:
>>>>
>>>>> Hi Hans,
>>>>>
>>>>> If we use PAGE_SIZE as USB_PAGE_SIZE, we should use PAGE_SIZE as
>>>>> XHCI_TD_PAGE_SIZE, too. I think.
>>>>>
>>>>> As you know, one TRB can use 1~64kB for the transfer length.
>>>>>
>>>
>>> Hi,
>>>
>>> We currently only check if 4K pages are supported by the hardware. If you
>>> change the value of XHCI_TD_PAGE_SIZE, you will also need to change the
>>> checks
>>> other places. You know that PAGE_SIZE is not a constant?
>>>
>>> Do you have a complete patch?
>>>
>>> --HPS
>>>
>>
>> Hi,
>>
>> XHCI_TD_PAGE_SIZE is used at only 2-points.
>>
>> 1. use as XHCI_TD_PAYLOAD_MAX (XHCI_TD_PAGE_NBUF) in xhci.h
>>     We sholud change as the following, I think.
>>
>> - #define XHCI_TD_PAGE_NBUF	17	/* units, room enough for 64Kbytes */
>> - #define XHCI_TD_PAGE_SIZE	4096	/* bytes */
>> - #define XHCI_TD_PAYLOAD_MAX	(XHCI_TD_PAGE_SIZE * (XHCI_TD_PAGE_NBUF - 1))
>> + #define XHCI_TD_PAYLOAD_MAX	(64*1024)	/* bytes */	
>> + #define XHCI_TD_PAGE_SIZE	PAGE_SIZE	/* bytes */
>> + /* units, room enough for 64Kbytes */
>> + #define XHCI_TD_PAGE_NBUF	(XHCI_TD_PAYLOAD_MAX/XHCI_TD_PAGE_SIZE + 1)
>>
>> 2. use as the maximum length of TRB.
>>     If PAGE_SIZE is 8kB, buf_res.length may be 8kB.
>>     But, we can set 1B~64kB for length of TRB. This is the spcification
>>     of xHCI. So, we don't need change this point.
>>
>> xhci.c
>> 1807                                 /* check for maximum length */
>> 1808                                 if (buf_res.length > XHCI_TD_PAGE_SIZE)
>> 1809 buf_res.length = XHCI_TD_PAGE_SIZE;
>>
> 
> Hi Kohji,
> 
> I see your points. By doing this you save some memory in the descriptor layout
> for 8K page size - right?
> 
> BTW: Do you think the following check is OK, or should it be extended to check
> also for 8K?
> 
>         if (!(XREAD4(sc, oper, XHCI_PAGESIZE) & XHCI_PAGESIZE_4K)) {
>                 device_printf(sc->sc_bus.parent, "Controller does "
>                     "not support 4K page size.\n");
>                 return (USB_ERR_IOERROR);
>         }
> 
> I guess your patch is more in the direction of optimisation. Is it very
> urgent? Or is it fine if I handle it the beginning of January?
> 
> Thank you for your input and review!
> 
> --HPS
> 

Hi HPS,

> I see your points. By doing this you save some memory in the
> descriptor layout for 8K page size - right?

Yes. In addition, we may reduce the size of data which a xhci
controller fetches.


> I guess your patch is more in the direction of optimisation. Is it very
> urgent? Or is it fine if I handle it the beginning of January?

No, it isn't urgent. It's OK in the next month.

In an optimisation, we should reduce the number of LINK_TRB, too.
I heard  from a LSI engineer that,
Generally xhci controler has the cache of TRB array. But, LINK_TRB
may make the cache miss.

Unfortunately, I don't know about XHCI_PAGESIZE. If I can hear from a
LSI engineer, I will inform you.

Best regards,
 Kohji Okuno