Date: Mon, 22 Dec 2014 18:31:57 +0900 (JST) From: Kohji Okuno <okuno.kohji@jp.panasonic.com> To: hps@selasky.org Cc: freebsd-usb@freebsd.org Subject: Re: About XHCI_TD_PAGE_SIZE. Message-ID: <20141222.183157.253796126986510571.okuno.kohji@jp.panasonic.com> In-Reply-To: <5497E016.7020809@selasky.org> References: <549541BC.6070505@selasky.org> <20141222.103833.2105768702793613386.okuno.kohji@jp.panasonic.com> <5497E016.7020809@selasky.org>
next in thread | previous in thread | raw e-mail | index | archive | help
From: Hans Petter Selasky <hps@selasky.org>
Subject: Re: About XHCI_TD_PAGE_SIZE.
Date: Mon, 22 Dec 2014 10:10:46 +0100
Message-ID: <5497E016.7020809@selasky.org>
> On 12/22/14 02:38, Kohji Okuno wrote:
>> From: Hans Petter Selasky <hps@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@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141222.183157.253796126986510571.okuno.kohji>