[PATCH] EHCI: Add support for 64 bit capability

[Sven Schnelle]

Signed-off-by: Sven Schnelle 
---
 src/usb-ehci.c |    6 ++----
 src/usb-ehci.h |    9 ++++++---
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/src/usb-ehci.c b/src/usb-ehci.c
index 4cb3e6a..8267a63 100644
--- a/src/usb-ehci.c
+++ b/src/usb-ehci.c
@@ -334,10 +334,6 @@ ehci_init(struct pci_device *pci, int busid, struct pci_device *comppci)
     u32 baseaddr = pci_config_readl(bdf, PCI_BASE_ADDRESS_0);
     struct ehci_caps *caps = (void*)(baseaddr & PCI_BASE_ADDRESS_MEM_MASK);
     u32 hcc_params = readl(&caps->hccparams);
-    if (hcc_params & HCC_64BIT_ADDR) {
-        dprintf(1, "No support for 64bit EHCI\n");
-        return -1;
-    }

struct usb_ehci_s *cntl = malloc_tmphigh(sizeof(*cntl));
     if (!cntl) {
@@ -349,6 +345,8 @@ ehci_init(struct pci_device *pci, int busid, struct pci_device *comppci)
     cntl->usb.pci = pci;
     cntl->usb.type = USB_TYPE_EHCI;
     cntl->caps = caps;
+    if (hcc_params & HCC_64BIT_ADDR)
+	cntl->regs->ctrldssegment = 0;
     cntl->regs = (void*)caps + readb(&caps->caplength);

dprintf(1, "EHCI init on dev %02x:%02x.%x (regs=%p)\n"
diff --git a/src/usb-ehci.h b/src/usb-ehci.h
index b295c78..924bbd8 100644
--- a/src/usb-ehci.h
+++ b/src/usb-ehci.h
@@ -102,7 +102,7 @@ struct ehci_qh {
     u32 alt_next;
     u32 token;
     u32 buf[5];
-    // u32 buf_hi[5];
+    u32 buf_hi[5];
 } PACKED;

#define QH_CONTROL       (1 << 27)
@@ -140,8 +140,11 @@ struct ehci_qtd {
     u32 alt_next;
     u32 token;
     u32 buf[5];
-    //u32 buf_hi[5];
-} PACKED;
+    u32 buf_hi[5];
+    /* keep struct size a multiple of 32 Bytes, as we're allocating
+       arrays. Without this padding, the second qtd would have the
+       wrong alignment. */
+} PACKED __aligned(32);

#define QTD_TOGGLE      (1 << 31)
 #define QTD_LENGTH_SHIFT 16

[Kevin O'Connor]

On Mon, Jun 11, 2012 at 10:57:30PM +0200, Sven Schnelle wrote:
> Signed-off-by: Sven Schnelle

Thanks.  See my comments below.
> --- a/src/usb-ehci.c
> +++ b/src/usb-ehci.c
> @@ -334,10 +334,6 @@ ehci_init(struct pci_device *pci, int busid, struct pci_device *comppci)
>      u32 baseaddr = pci_config_readl(bdf, PCI_BASE_ADDRESS_0);
>      struct ehci_caps *caps = (void*)(baseaddr & PCI_BASE_ADDRESS_MEM_MASK);
>      u32 hcc_params = readl(&caps->hccparams);
>
> if (hcc_params & HCC_64BIT_ADDR) {
>    dprintf(1, "No support for 64bit EHCI\n");
>    return -1;
>
> }
>
> struct usb_ehci_s *cntl = malloc_tmphigh(sizeof(*cntl));
> if (!cntl) {
>
> @@ -349,6 +345,8 @@ ehci_init(struct pci_device *pci, int busid, struct pci_device *comppci)
>      cntl->usb.pci = pci;
>      cntl->usb.type = USB_TYPE_EHCI;
>      cntl->caps = caps;
> if (hcc_params & HCC_64BIT_ADDR)
> cntl->regs->ctrldssegment = 0;

SeaBIOS uses all spaces (no tabs).

[...]
> --- a/src/usb-ehci.h
> +++ b/src/usb-ehci.h
> @@ -102,7 +102,7 @@ struct ehci_qh {
>      u32 alt_next;
>      u32 token;
>      u32 buf[5];
> // u32 buf_hi[5];
> u32 buf_hi[5];
>
> } PACKED;

The ehci spec requires the entire structure not span a 4K boundary.
There's nothing preventing one of the allocaters from doing this.  The
easiest solution is probably to increase EHCI_QH_ALIGN to 128.
> #define QH_CONTROL       (1 << 27)
> @@ -140,8 +140,11 @@ struct ehci_qtd {
>      u32 alt_next;
>      u32 token;
>      u32 buf[5];
> //u32 buf_hi[5];
>
> -} PACKED;
> u32 buf_hi[5];
> /* keep struct size a multiple of 32 Bytes, as we're allocating
>   arrays. Without this padding, the second qtd would have the
>   wrong alignment. */
>
> +} PACKED __aligned(32);

Same as above.  Easist solution is likely to increase EHCI_QTD_ALIGN
to 64.  (Also, it should probably read __aligned(EHCI_QTD_ALIGN).)

-Kevin

[Sven Schnelle]

Hi Kevin,

Kevin O'Connor  writes:
> On Mon, Jun 11, 2012 at 10:57:30PM +0200, Sven Schnelle wrote:
> > Signed-off-by: Sven Schnelle
>
> Thanks.  See my comments below.
> > --- a/src/usb-ehci.c
> > +++ b/src/usb-ehci.c
> > @@ -334,10 +334,6 @@ ehci_init(struct pci_device *pci, int busid, struct pci_device *comppci)
> >      u32 baseaddr = pci_config_readl(bdf, PCI_BASE_ADDRESS_0);
> >      struct ehci_caps *caps = (void*)(baseaddr & PCI_BASE_ADDRESS_MEM_MASK);
> >      u32 hcc_params = readl(&caps->hccparams);
> >
> > if (hcc_params & HCC_64BIT_ADDR) {
> >    dprintf(1, "No support for 64bit EHCI\n");
> >    return -1;
> >
> > }
> >
> > struct usb_ehci_s *cntl = malloc_tmphigh(sizeof(*cntl));
> > if (!cntl) {
> >
> > @@ -349,6 +345,8 @@ ehci_init(struct pci_device *pci, int busid, struct pci_device *comppci)
> >      cntl->usb.pci = pci;
> >      cntl->usb.type = USB_TYPE_EHCI;
> >      cntl->caps = caps;
> > if (hcc_params & HCC_64BIT_ADDR)
> > cntl->regs->ctrldssegment = 0;
>
> SeaBIOS uses all spaces (no tabs).
>
> [...]
> > --- a/src/usb-ehci.h
> > +++ b/src/usb-ehci.h
> > @@ -102,7 +102,7 @@ struct ehci_qh {
> >      u32 alt_next;
> >      u32 token;
> >      u32 buf[5];
> > // u32 buf_hi[5];
> > u32 buf_hi[5];
> >
> > } PACKED;
>
> The ehci spec requires the entire structure not span a 4K boundary.
> There's nothing preventing one of the allocaters from doing this.  The
> easiest solution is probably to increase EHCI_QH_ALIGN to 128.
> > #define QH_CONTROL       (1 << 27)
> > @@ -140,8 +140,11 @@ struct ehci_qtd {
> >      u32 alt_next;
> >      u32 token;
> >      u32 buf[5];
> > //u32 buf_hi[5];
> >
> > -} PACKED;
> > u32 buf_hi[5];
> > /* keep struct size a multiple of 32 Bytes, as we're allocating
> >   arrays. Without this padding, the second qtd would have the
> >   wrong alignment. */
> >
> > +} PACKED __aligned(32);
>
> Same as above.  Easist solution is likely to increase EHCI_QTD_ALIGN
> to 64.  (Also, it should probably read __aligned(EHCI_QTD_ALIGN).)

Thanks Kevin. I've updated my patch according to your suggestion.

commit b126073fe6ca3997d68f1a348f90f701a1efb83d
Author: Sven Schnelle 
Date:   Mon Jun 11 22:57:17 2012 +0200

EHCI: Add support for 64 bit capability

Signed-off-by: Sven Schnelle

diff --git a/src/usb-ehci.c b/src/usb-ehci.c
index 4cb3e6a..3c0be13 100644
--- a/src/usb-ehci.c
+++ b/src/usb-ehci.c
@@ -334,10 +334,6 @@ ehci_init(struct pci_device *pci, int busid, struct pci_device *comppci)
     u32 baseaddr = pci_config_readl(bdf, PCI_BASE_ADDRESS_0);
     struct ehci_caps *caps = (void*)(baseaddr & PCI_BASE_ADDRESS_MEM_MASK);
     u32 hcc_params = readl(&caps->hccparams);
-    if (hcc_params & HCC_64BIT_ADDR) {
-        dprintf(1, "No support for 64bit EHCI\n");
-        return -1;
-    }

struct usb_ehci_s *cntl = malloc_tmphigh(sizeof(*cntl));
     if (!cntl) {
@@ -349,6 +345,8 @@ ehci_init(struct pci_device *pci, int busid, struct pci_device *comppci)
     cntl->usb.pci = pci;
     cntl->usb.type = USB_TYPE_EHCI;
     cntl->caps = caps;
+    if (hcc_params & HCC_64BIT_ADDR)
+        cntl->regs->ctrldssegment = 0;
     cntl->regs = (void*)caps + readb(&caps->caplength);

dprintf(1, "EHCI init on dev %02x:%02x.%x (regs=%p)\n"
diff --git a/src/usb-ehci.h b/src/usb-ehci.h
index b295c78..5bc443b 100644
--- a/src/usb-ehci.h
+++ b/src/usb-ehci.h
@@ -90,7 +90,7 @@ struct ehci_regs {
 #define PORT_RWC_BITS   (PORT_CSC | PORT_PEC | PORT_OCC)

-#define EHCI_QH_ALIGN 64 // Can't span a 4K boundary, so increase to 64
+#define EHCI_QH_ALIGN 128 // Can't span a 4K boundary, so increase to 64

struct ehci_qh {
     u32 next;
@@ -102,7 +102,7 @@ struct ehci_qh {
     u32 alt_next;
     u32 token;
     u32 buf[5];
-    // u32 buf_hi[5];
+    u32 buf_hi[5];
 } PACKED;

#define QH_CONTROL       (1 << 27)
@@ -140,8 +140,11 @@ struct ehci_qtd {
     u32 alt_next;
     u32 token;
     u32 buf[5];
-    //u32 buf_hi[5];
-} PACKED;
+    u32 buf_hi[5];
+    /* keep struct size a multiple of 32 Bytes, as we're allocating
+       arrays. Without this padding, the second qtd would have the
+       wrong alignment. */
+} PACKED __aligned(EHCI_QH_ALIGN);

#define QTD_TOGGLE      (1 << 31)
 #define QTD_LENGTH_SHIFT 16

[Sven Schnelle]

Hi Kevin,
Sven Schnelle  writes:
> Kevin O'Connor  writes:
> > On Mon, Jun 11, 2012 at 10:57:30PM +0200, Sven Schnelle wrote:
> > > Signed-off-by: Sven Schnelle
> >
> > Thanks.  See my comments below.
> > [..]
>
> Thanks Kevin. I've updated my patch according to your suggestion.

That patch wasn't complete, sorry. I've attached another version ;)

commit 4b16e4f24a5ff99aa3169e59c233f700223c5b51
Author: Sven Schnelle 
Date:   Mon Jun 11 22:57:17 2012 +0200

EHCI: Add support for 64 bit capability

Signed-off-by: Sven Schnelle

diff --git a/src/usb-ehci.c b/src/usb-ehci.c
index 4cb3e6a..3c0be13 100644
--- a/src/usb-ehci.c
+++ b/src/usb-ehci.c
@@ -334,10 +334,6 @@ ehci_init(struct pci_device *pci, int busid, struct pci_device *comppci)
     u32 baseaddr = pci_config_readl(bdf, PCI_BASE_ADDRESS_0);
     struct ehci_caps *caps = (void*)(baseaddr & PCI_BASE_ADDRESS_MEM_MASK);
     u32 hcc_params = readl(&caps->hccparams);
-    if (hcc_params & HCC_64BIT_ADDR) {
-        dprintf(1, "No support for 64bit EHCI\n");
-        return -1;
-    }

struct usb_ehci_s *cntl = malloc_tmphigh(sizeof(*cntl));
     if (!cntl) {
@@ -349,6 +345,8 @@ ehci_init(struct pci_device *pci, int busid, struct pci_device *comppci)
     cntl->usb.pci = pci;
     cntl->usb.type = USB_TYPE_EHCI;
     cntl->caps = caps;
+    if (hcc_params & HCC_64BIT_ADDR)
+        cntl->regs->ctrldssegment = 0;
     cntl->regs = (void*)caps + readb(&caps->caplength);

dprintf(1, "EHCI init on dev %02x:%02x.%x (regs=%p)\n"
diff --git a/src/usb-ehci.h b/src/usb-ehci.h
index b295c78..9baec98 100644
--- a/src/usb-ehci.h
+++ b/src/usb-ehci.h
@@ -90,7 +90,7 @@ struct ehci_regs {
 #define PORT_RWC_BITS   (PORT_CSC | PORT_PEC | PORT_OCC)

-#define EHCI_QH_ALIGN 64 // Can't span a 4K boundary, so increase to 64
+#define EHCI_QH_ALIGN 128 // Can't span a 4K boundary, so increase to 128

struct ehci_qh {
     u32 next;
@@ -102,8 +102,8 @@ struct ehci_qh {
     u32 alt_next;
     u32 token;
     u32 buf[5];
-    // u32 buf_hi[5];
-} PACKED;
+    u32 buf_hi[5];
+} PACKED __aligned(EHCI_QH_ALIGN);

#define QH_CONTROL       (1 << 27)
 #define QH_MAXPACKET_SHIFT 16
@@ -133,15 +133,18 @@ struct ehci_qh {
 #define EHCI_PTR_QH             0x0002

-#define EHCI_QTD_ALIGN 32
+#define EHCI_QTD_ALIGN 64

struct ehci_qtd {
     u32 qtd_next;
     u32 alt_next;
     u32 token;
     u32 buf[5];
-    //u32 buf_hi[5];
-} PACKED;
+    u32 buf_hi[5];
+    /* keep struct size a multiple of 32 Bytes, as we're allocating
+       arrays. Without this padding, the second qtd would have the
+       wrong alignment. */
+} PACKED __aligned(EHCI_QTD_ALIGN);

#define QTD_TOGGLE      (1 << 31)
 #define QTD_LENGTH_SHIFT 16

[Kevin O'Connor]

On Tue, Jun 12, 2012 at 09:20:23AM +0200, Sven Schnelle wrote:
[...]
> That patch wasn't complete, sorry. I've attached another version ;)

[...]
> struct ehci_qh {
>      u32 next;
> @@ -102,8 +102,8 @@ struct ehci_qh {
>      u32 alt_next;
>      u32 token;
>      u32 buf[5];
> // u32 buf_hi[5];
>
> -} PACKED;
> u32 buf_hi[5];
>
> +} PACKED __aligned(EHCI_QH_ALIGN);

I don't think the ehci_qh needs to be padded to 128 bytes as it isn't
allocated in arrays.  Not specifiying the alignment allows the other
fields in the ehci_pipe struct to use that pad space instead of it
being wasted.  Let me know if I've missed something.

How about the below?

-Kevin
> From 4fe78a3714ce14957fdf184d90dd815c8415c4b3 Mon Sep 17 00:00:00 2001

From: Sven Schnelle 
Date: Tue, 12 Jun 2012 09:20:23 +0200
Subject: [PATCH] EHCI: Add support for 64 bit capability
To: [e-mail]

Signed-off-by: Sven Schnelle 
Signed-off-by: Kevin O'Connor 
---
 src/usb-ehci.c |    6 ++----
 src/usb-ehci.h |   13 ++++++++-----
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/src/usb-ehci.c b/src/usb-ehci.c
index 4cb3e6a..3c0be13 100644
--- a/src/usb-ehci.c
+++ b/src/usb-ehci.c
@@ -334,10 +334,6 @@ ehci_init(struct pci_device *pci, int busid, struct pci_device *comppci)
     u32 baseaddr = pci_config_readl(bdf, PCI_BASE_ADDRESS_0);
     struct ehci_caps *caps = (void*)(baseaddr & PCI_BASE_ADDRESS_MEM_MASK);
     u32 hcc_params = readl(&caps->hccparams);
-    if (hcc_params & HCC_64BIT_ADDR) {
-        dprintf(1, "No support for 64bit EHCI\n");
-        return -1;
-    }

struct usb_ehci_s *cntl = malloc_tmphigh(sizeof(*cntl));
     if (!cntl) {
@@ -349,6 +345,8 @@ ehci_init(struct pci_device *pci, int busid, struct pci_device *comppci)
     cntl->usb.pci = pci;
     cntl->usb.type = USB_TYPE_EHCI;
     cntl->caps = caps;
+    if (hcc_params & HCC_64BIT_ADDR)
+        cntl->regs->ctrldssegment = 0;
     cntl->regs = (void*)caps + readb(&caps->caplength);

dprintf(1, "EHCI init on dev %02x:%02x.%x (regs=%p)\n"
diff --git a/src/usb-ehci.h b/src/usb-ehci.h
index b295c78..32e4109 100644
--- a/src/usb-ehci.h
+++ b/src/usb-ehci.h
@@ -90,7 +90,7 @@ struct ehci_regs {
 #define PORT_RWC_BITS   (PORT_CSC | PORT_PEC | PORT_OCC)

-#define EHCI_QH_ALIGN 64 // Can't span a 4K boundary, so increase to 64
+#define EHCI_QH_ALIGN 128 // Can't span a 4K boundary, so increase from 32

struct ehci_qh {
     u32 next;
@@ -102,7 +102,7 @@ struct ehci_qh {
     u32 alt_next;
     u32 token;
     u32 buf[5];
-    // u32 buf_hi[5];
+    u32 buf_hi[5];
 } PACKED;

#define QH_CONTROL       (1 << 27)
@@ -133,15 +133,18 @@ struct ehci_qh {
 #define EHCI_PTR_QH             0x0002

-#define EHCI_QTD_ALIGN 32
+#define EHCI_QTD_ALIGN 64 // Can't span a 4K boundary, so increase from 32

struct ehci_qtd {
     u32 qtd_next;
     u32 alt_next;
     u32 token;
     u32 buf[5];
-    //u32 buf_hi[5];
-} PACKED;
+    u32 buf_hi[5];
+    /* keep struct size a multiple of 64 bytes, as we're allocating
+       arrays. Without this padding, the second qtd could have the
+       wrong alignment. */
+} PACKED __aligned(EHCI_QTD_ALIGN);

#define QTD_TOGGLE      (1 << 31)
 #define QTD_LENGTH_SHIFT 16

[Sven Schnelle]

Kevin O'Connor  writes:
> On Tue, Jun 12, 2012 at 09:20:23AM +0200, Sven Schnelle wrote:
> [...]
> > That patch wasn't complete, sorry. I've attached another version ;)
>
> [...]
> > struct ehci_qh {
> >      u32 next;
> > @@ -102,8 +102,8 @@ struct ehci_qh {
> >      u32 alt_next;
> >      u32 token;
> >      u32 buf[5];
> > // u32 buf_hi[5];
> >
> > -} PACKED;
> > u32 buf_hi[5];
> >
> > +} PACKED __aligned(EHCI_QH_ALIGN);
>
> I don't think the ehci_qh needs to be padded to 128 bytes as it isn't
> allocated in arrays.  Not specifiying the alignment allows the other
> fields in the ehci_pipe struct to use that pad space instead of it
> being wasted.  Let me know if I've missed something.

You didn't miss anything. it's ok to remove that padding.
> How about the below?
> [..]

Looks ok to me.

Regards
Sven

[Kevin O'Connor]

On Tue, Jun 12, 2012 at 03:40:38PM +0200, Sven Schnelle wrote:
> Kevin O'Connor  writes:
> > On Tue, Jun 12, 2012 at 09:20:23AM +0200, Sven Schnelle wrote:
> > [...]
> > > That patch wasn't complete, sorry. I've attached another version ;)
> >
> > [...]
> > > struct ehci_qh {
> > >      u32 next;
> > > @@ -102,8 +102,8 @@ struct ehci_qh {
> > >      u32 alt_next;
> > >      u32 token;
> > >      u32 buf[5];
> > > // u32 buf_hi[5];
> > >
> > > -} PACKED;
> > > u32 buf_hi[5];
> > >
> > > +} PACKED __aligned(EHCI_QH_ALIGN);
> >
> > I don't think the ehci_qh needs to be padded to 128 bytes as it isn't
> > allocated in arrays.  Not specifiying the alignment allows the other
> > fields in the ehci_pipe struct to use that pad space instead of it
> > being wasted.  Let me know if I've missed something.
>
> You didn't miss anything. it's ok to remove that padding.
> > How about the below?
> > [..]
>
> Looks ok to me.

Thanks - I committed it.

-Kevin