Patchwork [U-Boot,v5,2/8] usb: ehci-marvell: add support for second USB controller

login
register
mail settings
Submitter Sascha Silbe
Date June 25, 2013, 9:27 p.m.
Message ID <1372195668-25496-3-git-send-email-t-uboot@infra-silbe.de>
Download mbox | patch
Permalink /patch/254417/
State New
Delegated to: Prafulla Wadaskar
Headers show

Comments

Sascha Silbe - June 25, 2013, 9:27 p.m.
From: Sascha Silbe <sascha-pgp@silbe.org>

Marvell 88AP510 (Armada 510, dove) has two separate USB
controllers. Use the index parameter that already gets passed in to
calculate the base address of the controller.

Signed-off-by: Sascha Silbe <t-uboot@infra-silbe.de>
---
 v4->v5: no changes

 drivers/usb/host/ehci-marvell.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)
Wolfgang Denk - June 26, 2013, 2:04 p.m.
Dear Sascha Silbe,

In message <1372195668-25496-3-git-send-email-t-uboot@infra-silbe.de> you wrote:
> From: Sascha Silbe <sascha-pgp@silbe.org>
> 
> Marvell 88AP510 (Armada 510, dove) has two separate USB
> controllers. Use the index parameter that already gets passed in to
> calculate the base address of the controller.
...
> -#define rdl(off)	readl(MVUSB0_BASE + (off))
> -#define wrl(off, val)	writel((val), MVUSB0_BASE + (off))
> +#define rdl(base, off)		readl((base) + (off))
> +#define wrl(base, off, val)	writel((val), (base) + (off))

Instead of extending this, can we eventually clean this up and use C
structs instead?  U-Boot does not allow device accesses through a
based plus offset notation.

>  	u32 size, base, attrib;
> +#ifdef MVUSB1_BASE
> +	u32 usb_base = (index == 0) ? MVUSB0_BASE : MVUSB1_BASE;
> +#else
> +	u32 usb_base = MVUSB0_BASE;
> +#endif

Can we please also avoid this #ifdef's ?  Eventually you can use
something like "base_0 + index * sizeof(struct usb_something)" ?


Best regards,

Wolfgang Denk
Sascha Silbe - June 28, 2013, 10:34 p.m.
Dear Wolfgang Denk,

Wolfgang Denk <wd@denx.de> writes:

>> -#define rdl(off)	readl(MVUSB0_BASE + (off))
>> -#define wrl(off, val)	writel((val), MVUSB0_BASE + (off))
>> +#define rdl(base, off)		readl((base) + (off))
>> +#define wrl(base, off, val)	writel((val), (base) + (off))
>
> Instead of extending this, can we eventually clean this up and use C
> structs instead?  U-Boot does not allow device accesses through a
> based plus offset notation.

Thanks for the review. I've given the clean-up a stab today, as a
separate patch that the CuBox support series can build on.


>>  	u32 size, base, attrib;
>> +#ifdef MVUSB1_BASE
>> +	u32 usb_base = (index == 0) ? MVUSB0_BASE : MVUSB1_BASE;
>> +#else
>> +	u32 usb_base = MVUSB0_BASE;
>> +#endif
>
> Can we please also avoid this #ifdef's ?  Eventually you can use
> something like "base_0 + index * sizeof(struct usb_something)" ?

The two USB host controllers on Dove are separate entities at
different base offsets (0x50000 vs. 0x51000). We could fill up the
register struct to have a size of 0x1000, but then the next SoC to be
supported could come up with a different offset.

Sascha
Marek Vasut - June 29, 2013, 10:26 p.m.
Dear Sascha Silbe,

> Dear Wolfgang Denk,
> 
> Wolfgang Denk <wd@denx.de> writes:
> >> -#define rdl(off)	readl(MVUSB0_BASE + (off))
> >> -#define wrl(off, val)	writel((val), MVUSB0_BASE + (off))
> >> +#define rdl(base, off)		readl((base) + (off))
> >> +#define wrl(base, off, val)	writel((val), (base) + (off))
> > 
> > Instead of extending this, can we eventually clean this up and use C
> > structs instead?  U-Boot does not allow device accesses through a
> > based plus offset notation.
> 
> Thanks for the review. I've given the clean-up a stab today, as a
> separate patch that the CuBox support series can build on.
> 
> >>  	u32 size, base, attrib;
> >> 
> >> +#ifdef MVUSB1_BASE
> >> +	u32 usb_base = (index == 0) ? MVUSB0_BASE : MVUSB1_BASE;
> >> +#else
> >> +	u32 usb_base = MVUSB0_BASE;
> >> +#endif
> > 
> > Can we please also avoid this #ifdef's ?  Eventually you can use
> > something like "base_0 + index * sizeof(struct usb_something)" ?
> 
> The two USB host controllers on Dove are separate entities at
> different base offsets (0x50000 vs. 0x51000). We could fill up the
> register struct to have a size of 0x1000, but then the next SoC to be
> supported could come up with a different offset.

Check drivers/usb/host/ehci-mxs.c for handling of such a case ;-)

Best regards,
Marek Vasut

Patch

diff --git a/drivers/usb/host/ehci-marvell.c b/drivers/usb/host/ehci-marvell.c
index 2b73e4a..d2cf026 100644
--- a/drivers/usb/host/ehci-marvell.c
+++ b/drivers/usb/host/ehci-marvell.c
@@ -28,7 +28,9 @@ 
 #include "ehci.h"
 #include <asm/arch/cpu.h>
 
-#if defined(CONFIG_KIRKWOOD)
+#if defined(CONFIG_DOVE)
+#include <asm/arch/dove.h>
+#elif defined(CONFIG_KIRKWOOD)
 #include <asm/arch/kirkwood.h>
 #elif defined(CONFIG_ORION5X)
 #include <asm/arch/orion5x.h>
@@ -36,8 +38,8 @@ 
 
 DECLARE_GLOBAL_DATA_PTR;
 
-#define rdl(off)	readl(MVUSB0_BASE + (off))
-#define wrl(off, val)	writel((val), MVUSB0_BASE + (off))
+#define rdl(base, off)		readl((base) + (off))
+#define wrl(base, off, val)	writel((val), (base) + (off))
 
 #define USB_WINDOW_CTRL(i)	(0x320 + ((i) << 4))
 #define USB_WINDOW_BASE(i)	(0x324 + ((i) << 4))
@@ -46,10 +48,15 @@  DECLARE_GLOBAL_DATA_PTR;
 /*
  * USB 2.0 Bridge Address Decoding registers setup
  */
-static void usb_brg_adrdec_setup(void)
+static void usb_brg_adrdec_setup(int index)
 {
 	int i;
 	u32 size, base, attrib;
+#ifdef MVUSB1_BASE
+	u32 usb_base = (index == 0) ? MVUSB0_BASE : MVUSB1_BASE;
+#else
+	u32 usb_base = MVUSB0_BASE;
+#endif
 
 	for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {
 
@@ -76,13 +83,13 @@  static void usb_brg_adrdec_setup(void)
 		size = gd->bd->bi_dram[i].size;
 		base = gd->bd->bi_dram[i].start;
 		if ((size) && (attrib))
-			wrl(USB_WINDOW_CTRL(i),
-				MVCPU_WIN_CTRL_DATA(size, USB_TARGET_DRAM,
-					attrib, MVCPU_WIN_ENABLE));
+			wrl(usb_base, USB_WINDOW_CTRL(i),
+			    MVCPU_WIN_CTRL_DATA(size, USB_TARGET_DRAM,
+						attrib, MVCPU_WIN_ENABLE));
 		else
-			wrl(USB_WINDOW_CTRL(i), MVCPU_WIN_DISABLE);
+			wrl(usb_base, USB_WINDOW_CTRL(i), MVCPU_WIN_DISABLE);
 
-		wrl(USB_WINDOW_BASE(i), base);
+		wrl(usb_base, USB_WINDOW_BASE(i), base);
 	}
 }
 
@@ -92,9 +99,15 @@  static void usb_brg_adrdec_setup(void)
  */
 int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor)
 {
-	usb_brg_adrdec_setup();
+#ifdef MVUSB1_BASE
+	u32 usb_base = (index == 0) ? MVUSB0_BASE : MVUSB1_BASE;
+#else
+	u32 usb_base = MVUSB0_BASE;
+#endif
 
-	*hccr = (struct ehci_hccr *)(MVUSB0_BASE + 0x100);
+	usb_brg_adrdec_setup(index);
+
+	*hccr = (struct ehci_hccr *)(usb_base + 0x100);
 	*hcor = (struct ehci_hcor *)((uint32_t) *hccr
 			+ HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase)));