From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Fri, 27 Jul 2012 16:01:11 +0200
Subject: [U-Boot] [PATCH 2/5] ehci-hcd: Boost transfer speed
In-Reply-To: <806388763.701359.1343397576841.JavaMail.root@advansee.com>
References: <806388763.701359.1343397576841.JavaMail.root@advansee.com>
Message-ID: <201207271601.11940.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 Beno?t Th?baudeau,

[...]

> > Ok, maybe you can please elaborate on this crazy calculation in here?
> > Or somehow
> > clarify it? Also, won't the macros in include/common.h help in a way?
> > (like
> > ROUND() etc).
> 
> I have already posted a v2 for this specific patch (only 2/5) that does
> exactly that. Please review only the latest versions of patches.
> 
> > I don't really graps what you're trying to calculate here, so maybe
> > even a
> > comment would help.
> 
> I'll do that.
> 
> > > +		else					/* page-aligned */
> > > +			qtd_count += (length + QT_BUFFER_CNT * 4096 - 1) /
> > > +					(QT_BUFFER_CNT * 4096);
> > 
> > Same here, also please avoid using those 4096 and such constants ...
> > maybe
> > #define them in ehci.h ?
> 
> Yes. It was already everywhere, so I went on the same way.

I'm not exactly proud of the state of the usb stack, you know :(

> Do you think
> using PAGE_SIZE from <linux/compat.h> would be fine since these 4096 are
> nothing more than page sizes?

Isn't that intel-specific?

> > > +	}
> > > +	qtd = memalign(USB_DMA_MINALIGN, qtd_count * sizeof(struct qTD));
> > 
> > So your code can alloc more than 5 qTDs ? How does it chain them
> > then? Into more
> > QHs ?
> 
> It's done in exactly the same way as for the original 3 qTDs, only with
> more qTDs, but still with 5 qt_buffers per qTD.

I'm starting to see what you're trying to do. That's really cool :)

[...]
> > > +			token = (toggle << 31) |
> > > +			    (xfr_bytes << 16) |
> > > +			    ((req == NULL ? 1 : 0) << 15) |
> > > +			    (0 << 12) |
> > > +			    (3 << 10) |
> > > +			    ((usb_pipein(pipe) ? 1 : 0) << 8) | (0x80 << 0);
> > 
> > If you could fix all this magic afterwards (not in these patches),
> > that'd be
> > great.
> 
> Do you only mean #defining all those values?

Yes, but let's do this in a subsequent patch. It can wait for later.

[...]

> Best regards,
> Beno?t Th?baudeau