From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Wed, 1 Aug 2012 04:41:41 +0200
Subject: [U-Boot] [PATCH v2 2/5] ehci-hcd: Boost transfer speed
In-Reply-To: <50183764.7000804@herbrechtsmeier.net>
References: <1468394961.845242.1343696760020.JavaMail.root@advansee.com>
	<50183764.7000804@herbrechtsmeier.net>
Message-ID: <201208010441.41751.marex@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Dear Stefan Herbrechtsmeier,

[...]

> >>> What do you mean by "partial USB transfers"? As seen from EHCI
> >>> users like
> >>> the MSC driver (usb_storage.c), USB transfers either succeed or
> >>> fail, but
> >>> they cannot be "segmented".
> >> 
> >> Segmented -- like multiple transfers being issues with small payload?
> 
> Right.
> 
> >> You can
> >> not put these together at the USB-level, since it's the issuing code
> >> that has to
> >> be fixed.
> 
> If the segmentation comes from the file system handling we can not avoid
> this.

If the FS code is shitty or the device is fragmented, noone can help that.

[...]

> >>> My code assumes that wMaxPacketSize is a power of 2. This is not
> >>> always
> >>> true for interrupt endpoints. Let's talk about these. Their
> >>> handling is
> >>> currently broken in U-Boot since their transfers are made
> >>> asynchronous
> >>> instead of periodic. Devices shouldn't care too much about that, as
> >>> long
> >>> as transfers do not exceed wMaxPacketSize, in which case my code
> >>> still
> >>> works because wMaxPacketSize always fits in a single qTD. Interrupt
> >>> transfers larger than wMaxPacketSize do not seem to be used by
> >>> U-Boot. If
> >>> they were used, the current code in U-Boot would have a timing
> >>> issue
> >>> because the asynchronous scheme would break the interval requested
> >>> by
> >>> devices, which could at worst make them fail in some way. So the
> >>> only
> >>> solution would be that such transfers be split by the caller of
> >>> submit_int_msg, in which case my code still works. What would you
> >>> think
> >>> about failing with an error message in submit_int_msg if length is
> >>> larger
> >>> than wMaxPacketSize? Marek, what do you think?
> >> 
> >> Let's do that ... I think the interrupt endpoint is only used for
> >> keyboard and
> >> if someone needs it for something else, the code will be there, just
> >> needing
> >> improvement. Comment and error message are OK.
> > 
> > OK. I have thought of another solution for this. You'll tell me which one
> > you prefer.
> > 
> > The ehci_submit_async code currently in U-Boot checks through
> > ehci_td_buffer that length fits in the single qTD reserved for data
> > payload only after work has begun, possibly after a SETUP transfer. With
> > my series, this is checked at the very beginning, before the allocation.
> > We could detect that wMaxPacketSize is not a power of 2 (e.g. with
> > __builtin_popcount), in which case the allocation for the data payload
> > would be restricted to 1 qTD like now, and there would be a check at the
> > very beginning to test if length fits in this qTD. In that way, there
> > could be several packets per interrupt transfer as long as it fits in a
> > single qTD, just like now, contrary to the limitation imposed by the
> > error in submit_int_msg. But I'm not sure it's a good idea to allow this
> > behavior.
> 
> I think this is not needed, as there is only one user (keyboard) with
> max size of 8 byte.

I agree, but for a different reason. Let's aim for a simple implementation first 
that doesn't change behavior and add this change later _if_ needed.

[...]

> >>> So we could perhaps issue a #error in ehci-hcd or in usb_storage if
> >>> CONFIG_SYS_MALLOC_LEN is not large enough, but I don't think it's a
> >>> good
> >>> 
> >>> idea because:
> >>>   - the threshold value would have to depend on runtime block sizes
> >>>   or
> >>> 
> >>> something, which could lead to a big worst worst case that would
> >>> almost
> >>> never happen in real life, so giving such an unrealistic heap size
> >>> constraint would be cumbersome,
> >> 
> >> #warning then?
> > 
> > With which limit if so?
> 
> I would expect more than 128kB as this is a common worst case (512 B
> block size).

Seems to be reasonable.

[..]