[U-Boot] [PATCH 1/2] ush: ehci: initialize altnext pointers in QH

[U-Boot] [PATCH 1/2] ush: ehci: initialize altnext pointers in QH

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

A QH's overlay QTD altnext pointer should be explicitly marked invalid
so that the EHCI controller knows to look at the QTD next pointer. Update
the driver to do this.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/usb/host/ehci-hcd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 17187caed482..7068b762d8a4 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -395,6 +395,7 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer,
 		QH_ENDPT2_UFCMASK(0) | QH_ENDPT2_UFSMASK(0);
 	qh->qh_endpt2 = cpu_to_hc32(endpt);
 	qh->qh_overlay.qt_next = cpu_to_hc32(QT_NEXT_TERMINATE);
+	qh->qh_overlay.qt_altnext = cpu_to_hc32(QT_NEXT_TERMINATE);
 
 	tdp = &qh->qh_overlay.qt_next;
 
@@ -1186,6 +1187,7 @@ create_int_queue(struct usb_device *dev, unsigned long pipe, int queuesize,
 			qh->qh_link = QH_LINK_TERMINATE;
 
 		qh->qh_overlay.qt_next = (uint32_t)td;
+		qh->qh_overlay.qt_altnext = QT_NEXT_TERMINATE;
 		qh->qh_endpt1 = (0 << 28) | /* No NAK reload (ehci 4.9) */
 			(usb_maxpacket(dev, pipe) << 16) | /* MPS */
 			(1 << 14) |
-- 
1.8.1.5

[U-Boot] [PATCH 2/2] usb: ehci: fully align interrupt QHs/QTDs

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

These data structures are passed to cache-flushing routines, and hence
must be conform to both the USB the cache-flusing alignment requirements.
That means aligning to USB_DMA_MINALIGN. This is important on systems
where cache lines are >32 bytes.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/usb/host/ehci-hcd.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 7068b762d8a4..6017090ebeec 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1162,14 +1162,16 @@ create_int_queue(struct usb_device *dev, unsigned long pipe, int queuesize,
 		debug("ehci intr queue: out of memory\n");
 		goto fail1;
 	}
-	result->first = memalign(32, sizeof(struct QH) * queuesize);
+	result->first = memalign(USB_DMA_MINALIGN,
+				 sizeof(struct QH) * queuesize);
 	if (!result->first) {
 		debug("ehci intr queue: out of memory\n");
 		goto fail2;
 	}
 	result->current = result->first;
 	result->last = result->first + queuesize - 1;
-	result->tds = memalign(32, sizeof(struct qTD) * queuesize);
+	result->tds = memalign(USB_DMA_MINALIGN,
+			       sizeof(struct qTD) * queuesize);
 	if (!result->tds) {
 		debug("ehci intr queue: out of memory\n");
 		goto fail3;
-- 
1.8.1.5

[U-Boot] [PATCH 2/2] usb: ehci: fully align interrupt QHs/QTDs

[Marek Vasut]

On Thursday, February 06, 2014 at 09:13:06 PM, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> These data structures are passed to cache-flushing routines, and hence
> must be conform to both the USB the cache-flusing alignment requirements.
> That means aligning to USB_DMA_MINALIGN. This is important on systems
> where cache lines are >32 bytes.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

Acked-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

[U-Boot] [PATCH 2/2] usb: ehci: fully align interrupt QHs/QTDs

[Stephen Warren]

On 02/06/2014 07:53 PM, Marek Vasut wrote:
> On Thursday, February 06, 2014 at 09:13:06 PM, Stephen Warren wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> These data structures are passed to cache-flushing routines, and hence
>> must be conform to both the USB the cache-flusing alignment requirements.
>> That means aligning to USB_DMA_MINALIGN. This is important on systems
>> where cache lines are >32 bytes.
>>
>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> 
> Acked-by: Marek Vasut <marex@denx.de>

Oh, I assumed you would be applying this?

[U-Boot] [PATCH 2/2] usb: ehci: fully align interrupt QHs/QTDs

[Marek Vasut]

On Friday, February 07, 2014 at 07:48:06 AM, Stephen Warren wrote:
> On 02/06/2014 07:53 PM, Marek Vasut wrote:
> > On Thursday, February 06, 2014 at 09:13:06 PM, Stephen Warren wrote:
> >> From: Stephen Warren <swarren@nvidia.com>
> >> 
> >> These data structures are passed to cache-flushing routines, and hence
> >> must be conform to both the USB the cache-flusing alignment
> >> requirements. That means aligning to USB_DMA_MINALIGN. This is
> >> important on systems where cache lines are >32 bytes.
> >> 
> >> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> > 
> > Acked-by: Marek Vasut <marex@denx.de>
> 
> Oh, I assumed you would be applying this?

Yes I would , but the previous patch still needs some discussion.

Best regards,
Marek Vasut

[U-Boot] [PATCH 2/2] usb: ehci: fully align interrupt QHs/QTDs

[Stephen Warren]

On 02/07/2014 06:48 AM, Marek Vasut wrote:
> On Friday, February 07, 2014 at 07:48:06 AM, Stephen Warren wrote:
>> On 02/06/2014 07:53 PM, Marek Vasut wrote:
>>> On Thursday, February 06, 2014 at 09:13:06 PM, Stephen Warren wrote:
>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>
>>>> These data structures are passed to cache-flushing routines, and hence
>>>> must be conform to both the USB the cache-flusing alignment
>>>> requirements. That means aligning to USB_DMA_MINALIGN. This is
>>>> important on systems where cache lines are >32 bytes.
>>>>
>>>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>>>
>>> Acked-by: Marek Vasut <marex@denx.de>
>>
>> Oh, I assumed you would be applying this?
> 
> Yes I would , but the previous patch still needs some discussion.

Oh OK. The patches don't depend on each-other in any way, so you can go
ahead and apply this now if you want. I should have actually sent them
separately rather than as a series.

[U-Boot] [PATCH 2/2] usb: ehci: fully align interrupt QHs/QTDs

[Marek Vasut]

On Friday, February 07, 2014 at 05:24:27 PM, Stephen Warren wrote:
> On 02/07/2014 06:48 AM, Marek Vasut wrote:
> > On Friday, February 07, 2014 at 07:48:06 AM, Stephen Warren wrote:
> >> On 02/06/2014 07:53 PM, Marek Vasut wrote:
> >>> On Thursday, February 06, 2014 at 09:13:06 PM, Stephen Warren wrote:
> >>>> From: Stephen Warren <swarren@nvidia.com>
> >>>> 
> >>>> These data structures are passed to cache-flushing routines, and hence
> >>>> must be conform to both the USB the cache-flusing alignment
> >>>> requirements. That means aligning to USB_DMA_MINALIGN. This is
> >>>> important on systems where cache lines are >32 bytes.
> >>>> 
> >>>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> >>> 
> >>> Acked-by: Marek Vasut <marex@denx.de>
> >> 
> >> Oh, I assumed you would be applying this?
> > 
> > Yes I would , but the previous patch still needs some discussion.
> 
> Oh OK. The patches don't depend on each-other in any way, so you can go
> ahead and apply this now if you want. I should have actually sent them
> separately rather than as a series.

Yeah, applied V2 of 1 and 2/2 now.

Best regards,
Marek Vasut

[U-Boot] [PATCH 2/2] usb: ehci: fully align interrupt QHs/QTDs

[Stephen Warren]

On 02/08/2014 07:01 AM, Marek Vasut wrote:
> On Friday, February 07, 2014 at 05:24:27 PM, Stephen Warren wrote:
>> On 02/07/2014 06:48 AM, Marek Vasut wrote:
>>> On Friday, February 07, 2014 at 07:48:06 AM, Stephen Warren wrote:
>>>> On 02/06/2014 07:53 PM, Marek Vasut wrote:
>>>>> On Thursday, February 06, 2014 at 09:13:06 PM, Stephen Warren wrote:
>>>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>>>
>>>>>> These data structures are passed to cache-flushing routines, and hence
>>>>>> must be conform to both the USB the cache-flusing alignment
>>>>>> requirements. That means aligning to USB_DMA_MINALIGN. This is
>>>>>> important on systems where cache lines are >32 bytes.
>>>>>>
>>>>>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>>>>>
>>>>> Acked-by: Marek Vasut <marex@denx.de>
>>>>
>>>> Oh, I assumed you would be applying this?
>>>
>>> Yes I would , but the previous patch still needs some discussion.
>>
>> Oh OK. The patches don't depend on each-other in any way, so you can go
>> ahead and apply this now if you want. I should have actually sent them
>> separately rather than as a series.
> 
> Yeah, applied V2 of 1 and 2/2 now.

Great, thanks. I assume these will be pulled into 2014.04?

[U-Boot] [PATCH 1/2] ush: ehci: initialize altnext pointers in QH

[Marek Vasut]

On Thursday, February 06, 2014 at 09:13:05 PM, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> A QH's overlay QTD altnext pointer should be explicitly marked invalid
> so that the EHCI controller knows to look at the QTD next pointer. Update
> the driver to do this.

Can you please add a reference to the spec where this is stated ?

> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  drivers/usb/host/ehci-hcd.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index 17187caed482..7068b762d8a4 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -395,6 +395,7 @@ ehci_submit_async(struct usb_device *dev, unsigned long
> pipe, void *buffer, QH_ENDPT2_UFCMASK(0) | QH_ENDPT2_UFSMASK(0);
>  	qh->qh_endpt2 = cpu_to_hc32(endpt);
>  	qh->qh_overlay.qt_next = cpu_to_hc32(QT_NEXT_TERMINATE);
> +	qh->qh_overlay.qt_altnext = cpu_to_hc32(QT_NEXT_TERMINATE);
> 
>  	tdp = &qh->qh_overlay.qt_next;
> 
> @@ -1186,6 +1187,7 @@ create_int_queue(struct usb_device *dev, unsigned
> long pipe, int queuesize, qh->qh_link = QH_LINK_TERMINATE;
> 
>  		qh->qh_overlay.qt_next = (uint32_t)td;
> +		qh->qh_overlay.qt_altnext = QT_NEXT_TERMINATE;

So next is td and altnext is terminate here ?

>  		qh->qh_endpt1 = (0 << 28) | /* No NAK reload (ehci 4.9) */
>  			(usb_maxpacket(dev, pipe) << 16) | /* MPS */
>  			(1 << 14) |

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/2] ush: ehci: initialize altnext pointers in QH

[Stephen Warren]

On 02/06/2014 07:53 PM, Marek Vasut wrote:
> On Thursday, February 06, 2014 at 09:13:05 PM, Stephen Warren wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> A QH's overlay QTD altnext pointer should be explicitly marked invalid
>> so that the EHCI controller knows to look at the QTD next pointer. Update
>> the driver to do this.
> 
> Can you please add a reference to the spec where this is stated ?

Section 4.10.2 "Advance Queue". Are you simply asking me to repost with
that added to the commit description, or justify the change?

FWIW, the situation is that the QH set up by ehci_submit_async() does
not have the active or halt bits set the first time around, so after the
"Fetch Queue Head" state, the "Advance Queue" state is entered, and this
is when the T bits of the Alternate Next qTD Pointer and Next qTD
Pointer are checked. I guess since I can download the PDF from Intel's
website without agreeing to any kind of license, I can quote it:

If the field Bytes to Transfer is not zero and the T-bit in the
Alternate Next qTD Pointer is set to zero, then the host controller uses
the Alternate Next qTD Pointer. Otherwise, the host controller uses the
Next qTD Pointer. If Next qTD Pointer?s T-bit is set to a one, then the
host controller exits this state and uses the horizontal pointer to the
next schedule data structure.

>> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c

>> @@ -1186,6 +1187,7 @@ create_int_queue(struct usb_device *dev, unsigned
>> long pipe, int queuesize, qh->qh_link = QH_LINK_TERMINATE;
>>
>>  		qh->qh_overlay.qt_next = (uint32_t)td;
>> +		qh->qh_overlay.qt_altnext = QT_NEXT_TERMINATE;
> 
> So next is td and altnext is terminate here ?

Yes. TERMINATE really just means INVALID, so the EHCI controller gets
the pointer from qt_next rather than qt_altnext.

[U-Boot] [PATCH 1/2] ush: ehci: initialize altnext pointers in QH

[Marek Vasut]

On Friday, February 07, 2014 at 07:47:14 AM, Stephen Warren wrote:
> On 02/06/2014 07:53 PM, Marek Vasut wrote:
> > On Thursday, February 06, 2014 at 09:13:05 PM, Stephen Warren wrote:
> >> From: Stephen Warren <swarren@nvidia.com>
> >> 
> >> A QH's overlay QTD altnext pointer should be explicitly marked invalid
> >> so that the EHCI controller knows to look at the QTD next pointer.
> >> Update the driver to do this.
> > 
> > Can you please add a reference to the spec where this is stated ?
> 
> Section 4.10.2 "Advance Queue". Are you simply asking me to repost with
> that added to the commit description, or justify the change?

Justify it. Of course, clear and helpful commit message like so would be very 
nice, so feel free to repost this please.

> FWIW, the situation is that the QH set up by ehci_submit_async() does
> not have the active or halt bits set the first time around, so after the
> "Fetch Queue Head" state, the "Advance Queue" state is entered, and this
> is when the T bits of the Alternate Next qTD Pointer and Next qTD
> Pointer are checked. I guess since I can download the PDF from Intel's
> website without agreeing to any kind of license, I can quote it:
> 
> If the field Bytes to Transfer is not zero and the T-bit in the
> Alternate Next qTD Pointer is set to zero, then the host controller uses
> the Alternate Next qTD Pointer. Otherwise, the host controller uses the
> Next qTD Pointer. If Next qTD Pointer?s T-bit is set to a one, then the
> host controller exits this state and uses the horizontal pointer to the
> next schedule data structure.
> 
> >> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> >> 
> >> @@ -1186,6 +1187,7 @@ create_int_queue(struct usb_device *dev, unsigned
> >> long pipe, int queuesize, qh->qh_link = QH_LINK_TERMINATE;
> >> 
> >>  		qh->qh_overlay.qt_next = (uint32_t)td;
> >> 
> >> +		qh->qh_overlay.qt_altnext = QT_NEXT_TERMINATE;
> > 
> > So next is td and altnext is terminate here ?
> 
> Yes. TERMINATE really just means INVALID, so the EHCI controller gets
> the pointer from qt_next rather than qt_altnext.

Hmmmm ... not really clear or self-explaining, is it. If you have time to 
produce a RFC to clean this up, I'm all for checking it.

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/2] ush: ehci: initialize altnext pointers in QH

[Stephen Warren]

On 02/07/2014 06:50 AM, Marek Vasut wrote:
> On Friday, February 07, 2014 at 07:47:14 AM, Stephen Warren wrote:
>> On 02/06/2014 07:53 PM, Marek Vasut wrote:
>>> On Thursday, February 06, 2014 at 09:13:05 PM, Stephen Warren wrote:
>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>
>>>> A QH's overlay QTD altnext pointer should be explicitly marked invalid
>>>> so that the EHCI controller knows to look at the QTD next pointer.
>>>> Update the driver to do this.
...
>>>> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
>>>>
>>>> @@ -1186,6 +1187,7 @@ create_int_queue(struct usb_device *dev, unsigned
>>>> long pipe, int queuesize, qh->qh_link = QH_LINK_TERMINATE;
>>>>
>>>>  		qh->qh_overlay.qt_next = (uint32_t)td;
>>>>
>>>> +		qh->qh_overlay.qt_altnext = QT_NEXT_TERMINATE;
>>>
>>> So next is td and altnext is terminate here ?
>>
>> Yes. TERMINATE really just means INVALID, so the EHCI controller gets
>> the pointer from qt_next rather than qt_altnext.
> 
> Hmmmm ... not really clear or self-explaining, is it. If you have time to 
> produce a RFC to clean this up, I'm all for checking it.

Looking at section 3.5.1 "Next qTD Pointer", this bit is in fact named
Terminate (hence it's referred to as the T bit), so this naming seems to
be ingrained in the EHCI specification, so I'm uninclined to change it
in U-Boot.