usb: zynqmp: Add XHCI driver support

PATCH Superseded Siva Durga Prasad Paladugu 2 participants 10 messages Sep 1, 2015

Subsystems (3)

USB Arm/Zynq architecture USB xHCI driver

lore

Share & tools

lore

Siva Durga Prasad Paladugu

author

2015-09-01 10:31 UTC #0 lore raw

PATCH v2 1/2 usb: zynqmp: Add XHCI driver support

Added USB XHCI driver support for zynqmp.

Signed-off-by Siva Durga Prasad Paladugu <[email protected]>

---
Changes for v2:
- Moved all from xhci-zynqmp.h to .c file as
per review comment
- Removed ad-hoc function zynqmp_xhci_core_exit()
as per review comment

---
 drivers/usb/host/Makefile      |    1 +
 drivers/usb/host/xhci-zynqmp.c |  147 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 148 insertions(+), 0 deletions(-)
 create mode 100644 drivers/usb/host/xhci-zynqmp.c

drivers/usb/host/Makefile +1 -0

index f70f38c..645b990 100644
@@ -54,6 +54,7 @@ obj-$(CONFIG_USB_EHCI_ZYNQ) += ehci-zynq.o
 # xhci
 obj-$(CONFIG_USB_XHCI) += xhci.o xhci-mem.o xhci-ring.o
 obj-$(CONFIG_USB_XHCI_DWC3) += xhci-dwc3.o
+obj-$(CONFIG_USB_XHCI_ZYNQMP) += xhci-zynqmp.o
 obj-$(CONFIG_USB_XHCI_KEYSTONE) += xhci-keystone.o
 obj-$(CONFIG_USB_XHCI_EXYNOS) += xhci-exynos5.o
 obj-$(CONFIG_USB_XHCI_FSL) += xhci-fsl.o

drivers/usb/host/xhci-zynqmp.c new file +147 -0

147 lines not on this page · raw message

Signature · 1 line

1.7.1

Siva Durga Prasad Paladugu

author

2015-09-01 10:31 UTC #1 lore raw

PATCH v2 2/2 usb: zynqmp: Enable USB XHCI support

Enable USB XHCI support for ZynqMP

Signed-off-by Siva Durga Prasad Paladugu <[email protected]>

---
Changes for v2:
- None

---
 include/configs/xilinx_zynqmp.h |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

include/configs/xilinx_zynqmp.h +8 -0

index da87188..a9556d5 100644
@@ -109,6 +109,14 @@
 #define CONFIG_SYS_LOAD_ADDR		0x8000000
 
 #if defined(CONFIG_ZYNQMP_USB)
+#define CONFIG_USB_XHCI_DWC3
+#define CONFIG_USB_XHCI
+#define CONFIG_USB_MAX_CONTROLLER_COUNT         1
+#define CONFIG_SYS_USB_XHCI_MAX_ROOT_PORTS      2
+# define CONFIG_CMD_USB
+# define CONFIG_USB_STORAGE
+#define CONFIG_USB_XHCI_ZYNQMP
+
 #define CONFIG_USB_DWC3
 #define CONFIG_USB_DWC3_GADGET

Signature · 1 line

1.7.1

Siva Durga Prasad Paladugu

author

2015-09-01 12:49 UTC #3 lore raw

PATCH v2 2/2 usb: zynqmp: Enable USB XHCI support

Quoted 28 lines

-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Tuesday, September 01, 2015 5:09 PM
To: Siva Durga Prasad Paladugu
Cc: u-boot at lists.denx.de; Siva Durga Prasad Paladugu
Subject: Re: [PATCH v2 2/2] usb: zynqmp: Enable USB XHCI support
Quoted 17 lines · Siva Durga Prasad Paladugu
Enable USB XHCI support for ZynqMP
Signed-off-by Siva Durga Prasad Paladugu <[email protected]>
---
Changes for v2:
- None
---
 include/configs/xilinx_zynqmp.h |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
(quoted diff) +0 -0
b/include/configs/xilinx_zynqmp.h index da87188..a9556d5 100644
include/configs/xilinx_zynqmp.h +0 -0
@@ -109,6 +109,14 @@
 #define CONFIG_SYS_LOAD_ADDR		0x8000000
 

Quoted 9 lines

include/configs/xilinx_zynqmp.h line 117

Quoted 7 lines

include/configs/xilinx_zynqmp.h +6 -0

 #if defined(CONFIG_ZYNQMP_USB)
+#define CONFIG_USB_XHCI_DWC3
+#define CONFIG_USB_XHCI
+#define CONFIG_USB_MAX_CONTROLLER_COUNT         1
+#define CONFIG_SYS_USB_XHCI_MAX_ROOT_PORTS      2
+# define CONFIG_CMD_USB
+# define CONFIG_USB_STORAGE

Drop this space between # and define please.

Ok will do that in V3

Thanks,
Siva

Quoted 8 lines

include/configs/xilinx_zynqmp.h line 121
Quoted 5 lines
include/configs/xilinx_zynqmp.h +2 -0
+#define CONFIG_USB_XHCI_ZYNQMP
+
 #define CONFIG_USB_DWC3
 #define CONFIG_USB_DWC3_GADGET
Best regards,
Marek Vasut

Marek Vasut

2015-09-01 11:38 UTC #4 lore raw

PATCH v2 1/2 usb: zynqmp: Add XHCI driver support

In reply to Siva Durga Prasad Paladugu

Quoted 3 lines · Siva Durga Prasad Paladugu

Added USB XHCI driver support for zynqmp.
Signed-off-by Siva Durga Prasad Paladugu <[email protected]>

Hi, looks almost good, a few minor nits though ...

[...]

Quoted 1 line

+unsigned long ctr_addr[] = {ZYNQMP_USB0_XHCI_BASEADDR,

static const void __iomem *ctl_addr[]

drivers/usb/host/xhci-zynqmp.c line 68

Quoted 1 line

drivers/usb/host/xhci-zynqmp.c new file +1 -0
+			    ZYNQMP_USB1_XHCI_BASEADDR};

I guess you can define something like CONFIG_ZYNQMP_XHCI_LIST { address ... } in your board config file and then use static const unsigned long ctl_addr[] = CONFIG_ZYNQMP... ; This will cover board which only use one controller. The ideal way would be to obtain these information from DT though.

Quoted 4 lines

drivers/usb/host/xhci-zynqmp.c new file +4 -0

+__weak int __board_usb_init(int index, enum usb_init_type init)
+{
+	return 0;
+}

drivers/usb/host/xhci-zynqmp.c line 83

Quoted 10 lines

drivers/usb/host/xhci-zynqmp.c new file +10 -0

+
+void usb_phy_reset(struct dwc3 *dwc3_reg)
+{
+	/* Assert USB3 PHY reset */
+	setbits_le32(&dwc3_reg->g_usb3pipectl[0], DWC3_GUSB3PIPECTL_PHYSOFTRST);
+
+	/* Assert USB2 PHY reset */
+	setbits_le32(&dwc3_reg->g_usb2phycfg, DWC3_GUSB2PHYCFG_PHYSOFTRST);
+
+	mdelay(200);

That's some lazy crappy controller. Is this long delay needed ?

Quoted 38 lines

drivers/usb/host/xhci-zynqmp.c new file +24 -0

+	/* Clear USB3 PHY reset */
+	clrbits_le32(&dwc3_reg->g_usb3pipectl[0], DWC3_GUSB3PIPECTL_PHYSOFTRST);
+
+	/* Clear USB2 PHY reset */
+	clrbits_le32(&dwc3_reg->g_usb2phycfg, DWC3_GUSB2PHYCFG_PHYSOFTRST);
+}
+
+static int zynqmp_xhci_core_init(struct zynqmp_xhci *zynqmp_xhci)
+{
+	int ret = 0;
+
+	ret = dwc3_core_init(zynqmp_xhci->dwc3_reg);
+	if (ret) {
+		debug("%s:failed to initialize core\n", __func__);
+		return ret;
+	}
+
+	/* We are hard-coding DWC3 core to Host Mode */
+	dwc3_set_mode(zynqmp_xhci->dwc3_reg, DWC3_GCTL_PRTCAP_HOST);
+
+	return ret;
+}
+
+int xhci_hcd_init(int index, struct xhci_hccr **hccr, struct xhci_hcor

**hcor) +{

drivers/usb/host/xhci-zynqmp.c new file +13 -0

+	struct zynqmp_xhci *ctx = &zynqmp_xhci;
+	int ret = 0;
+
+	ctx->hcd = (struct xhci_hccr *)ctr_addr[index];
+	ctx->dwc3_reg = (struct dwc3 *)((char *)(ctx->hcd) + DWC3_REG_OFFSET);
+
+	ret = board_usb_init(index, USB_INIT_HOST);
+	if (ret != 0) {
+		puts("Failed to initialize board for USB\n");
+		return ret;
+	}
+
+	ret = zynqmp_xhci_core_init(ctx);

drivers/usb/host/xhci-zynqmp.c line 132

Quoted 10 lines

drivers/usb/host/xhci-zynqmp.c new file +10 -0

+	if (ret < 0) {
+		puts("Failed to initialize xhci\n");
+		return ret;
+	}
+
+	*hccr = (struct xhci_hccr *)ctx->hcd;
+	*hcor = (struct xhci_hcor *)((uint32_t) *hccr
+				+ HC_LENGTH(xhci_readl(&(*hccr)->cr_capbase)));
+
+	debug("zynqmp-xhci: init hccr %x and hcor %x hc_length %d\n",

Use %p in the formating string to print pointers and drop the type casts.

Quoted 15 lines

drivers/usb/host/xhci-zynqmp.c new file +15 -0

+	      (uint32_t)*hccr, (uint32_t)*hcor,
+	      (uint32_t)HC_LENGTH(xhci_readl(&(*hccr)->cr_capbase)));
+
+	return ret;
+}
+
+void xhci_hcd_stop(int index)
+{
+	/*
+	 * Currently zynqmp socs do not support PHY shutdown from
+	 * sw. But this support may be added in future socs.
+	 */
+
+	return 0;
+}

Siva Durga Prasad Paladugu

author

2015-09-01 12:48 UTC #5 lore raw

PATCH v2 1/2 usb: zynqmp: Add XHCI driver support

HI Marek,

Quoted 19 lines

-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Tuesday, September 01, 2015 5:09 PM
To: Siva Durga Prasad Paladugu
Cc: u-boot at lists.denx.de; Siva Durga Prasad Paladugu; monstr at monstr.eu
Subject: Re: [PATCH v2 1/2] usb: zynqmp: Add XHCI driver support
Quoted 3 lines · Siva Durga Prasad Paladugu
Added USB XHCI driver support for zynqmp.
Signed-off-by Siva Durga Prasad Paladugu <[email protected]>
Hi, looks almost good, a few minor nits though ...
[...]
Quoted 1 line
+unsigned long ctr_addr[] = {ZYNQMP_USB0_XHCI_BASEADDR,

Quoted 8 lines

static const void __iomem *ctl_addr[]
drivers/usb/host/xhci-zynqmp.c line 68
Quoted 1 line
drivers/usb/host/xhci-zynqmp.c new file +1 -0
+			    ZYNQMP_USB1_XHCI_BASEADDR};
I guess you can define something like CONFIG_ZYNQMP_XHCI_LIST { address ... } in your board config file and then use static const unsigned long ctl_addr[] = CONFIG_ZYNQMP... ; This will cover board which only use one controller.

Yeah DT is the ideal way,
For now, I can modify it to be like this
static const void __iomem *ctl_addr[] = { ZYNQMP_USB0_XHCI_BASEADDR,

					ZYNQMP_USB1_XHCI_BASEADDR};

But to define a macro in board config file, I may have to include hardware.h, where iam defining all base addresses of the IP's into the board config file just for this.

Is it fine if I can keep as I mentioned above?

Quoted 14 lines

The ideal way would be to obtain these information from DT though.
drivers/usb/host/xhci-zynqmp.c
Quoted 11 lines
drivers/usb/host/xhci-zynqmp.c new file +11 -0
+__weak int __board_usb_init(int index, enum usb_init_type init) {
+	return 0;
+}
+
+void usb_phy_reset(struct dwc3 *dwc3_reg) {
+	/* Assert USB3 PHY reset */
+	setbits_le32(&dwc3_reg->g_usb3pipectl[0],
+DWC3_GUSB3PIPECTL_PHYSOFTRST);
+
+	/* Assert USB2 PHY reset */
+	setbits_le32(&dwc3_reg->g_usb2phycfg,
DWC3_GUSB2PHYCFG_PHYSOFTRST);

Quoted 4 lines

Review
Quoted 2 lines
(quoted diff) +2 -0
+
+	mdelay(200);
That's some lazy crappy controller. Is this long delay needed ?

Yeah can you just keep it as is for some time.
This is how we tested on our emulation platforms.
I will anyway modify it at later point of time.

Quoted 48 lines

drivers/usb/host/xhci-zynqmp.c

Quoted 7 lines

drivers/usb/host/xhci-zynqmp.c new file +6 -0

+	/* Clear USB3 PHY reset */
+	clrbits_le32(&dwc3_reg->g_usb3pipectl[0],
+DWC3_GUSB3PIPECTL_PHYSOFTRST);
+
+	/* Clear USB2 PHY reset */
+	clrbits_le32(&dwc3_reg->g_usb2phycfg,

DWC3_GUSB2PHYCFG_PHYSOFTRST);

drivers/usb/host/xhci-zynqmp.c

Quoted 13 lines

drivers/usb/host/xhci-zynqmp.c new file +13 -0

+}
+
+static int zynqmp_xhci_core_init(struct zynqmp_xhci *zynqmp_xhci) {
+	int ret = 0;
+
+	ret = dwc3_core_init(zynqmp_xhci->dwc3_reg);
+	if (ret) {
+		debug("%s:failed to initialize core\n", __func__);
+		return ret;
+	}
+
+	/* We are hard-coding DWC3 core to Host Mode */
+	dwc3_set_mode(zynqmp_xhci->dwc3_reg,

DWC3_GCTL_PRTCAP_HOST);

Quoted 26 lines

drivers/usb/host/xhci-zynqmp.c new file +6 -0

+
+	return ret;
+}
+
+int xhci_hcd_init(int index, struct xhci_hccr **hccr, struct
+xhci_hcor

**hcor) +{

drivers/usb/host/xhci-zynqmp.c new file +19 -0

+	struct zynqmp_xhci *ctx = &zynqmp_xhci;
+	int ret = 0;
+
+	ctx->hcd = (struct xhci_hccr *)ctr_addr[index];
+	ctx->dwc3_reg = (struct dwc3 *)((char *)(ctx->hcd) +
+DWC3_REG_OFFSET);
+
+	ret = board_usb_init(index, USB_INIT_HOST);
+	if (ret != 0) {
+		puts("Failed to initialize board for USB\n");
+		return ret;
+	}
+
+	ret = zynqmp_xhci_core_init(ctx);
+	if (ret < 0) {
+		puts("Failed to initialize xhci\n");
+		return ret;
+	}
+

Quoted 8 lines

Review
Quoted 6 lines
drivers/usb/host/xhci-zynqmp.c new file +3 -0
+	*hccr = (struct xhci_hccr *)ctx->hcd;
+	*hcor = (struct xhci_hcor *)((uint32_t) *hccr
+				+ HC_LENGTH(xhci_readl(&(*hccr)-
cr_capbase)));
(quoted diff) +2 -0
+
+	debug("zynqmp-xhci: init hccr %x and hcor %x hc_length %d\n",
Use %p in the formating string to print pointers and drop the type casts.

Thanks,
Siva

Quoted 16 lines

drivers/usb/host/xhci-zynqmp.c new file +15 -0

+	      (uint32_t)*hccr, (uint32_t)*hcor,
+	      (uint32_t)HC_LENGTH(xhci_readl(&(*hccr)->cr_capbase)));
+
+	return ret;
+}
+
+void xhci_hcd_stop(int index)
+{
+	/*
+	 * Currently zynqmp socs do not support PHY shutdown from
+	 * sw. But this support may be added in future socs.
+	 */
+
+	return 0;
+}

Marek Vasut

2015-09-01 13:20 UTC #6 lore raw

PATCH v2 1/2 usb: zynqmp: Add XHCI driver support

Quoted 1 line · Siva Durga Prasad Paladugu

HI Marek,

Hi,

Quoted 29 lines

Quoted 20 lines
-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Tuesday, September 01, 2015 5:09 PM
To: Siva Durga Prasad Paladugu
Cc: u-boot at lists.denx.de; Siva Durga Prasad Paladugu; monstr at monstr.eu
Subject: Re: [PATCH v2 1/2] usb: zynqmp: Add XHCI driver support
On Tuesday, September 01, 2015 at 12:31:02 PM, Siva Durga Prasad Paladugu
Quoted 3 lines · wrote:
Added USB XHCI driver support for zynqmp.
Signed-off-by Siva Durga Prasad Paladugu <[email protected]>
Hi, looks almost good, a few minor nits though ...
[...]
Quoted 1 line
+unsigned long ctr_addr[] = {ZYNQMP_USB0_XHCI_BASEADDR,
Quoted 8 lines
static const void __iomem *ctl_addr[]
drivers/usb/host/xhci-zynqmp.c line 68
Quoted 1 line
drivers/usb/host/xhci-zynqmp.c new file +1 -0
+			    ZYNQMP_USB1_XHCI_BASEADDR};
I guess you can define something like CONFIG_ZYNQMP_XHCI_LIST { address ... } in your board config file and then use static const unsigned long ctl_addr[] = CONFIG_ZYNQMP... ; This will cover board which only use one controller.

Quoted 8 lines

Yeah DT is the ideal way,
For now, I can modify it to be like this
static const void __iomem *ctl_addr[] = { ZYNQMP_USB0_XHCI_BASEADDR,
					ZYNQMP_USB1_XHCI_BASEADDR};
But to define a macro in board config file, I may have to include hardware.h, where iam defining all base addresses of the IP's into the board config file just for this.

Is that a problem ?

Quoted 1 line

Is it fine if I can keep as I mentioned above?

I am not very fond of it, since this is broken for boards which don't use both controllers.

Quoted 16 lines

Quoted 16 lines
The ideal way would be to obtain these information from DT though.
drivers/usb/host/xhci-zynqmp.c
Quoted 11 lines
drivers/usb/host/xhci-zynqmp.c new file +11 -0
+__weak int __board_usb_init(int index, enum usb_init_type init) {
+	return 0;
+}
+
+void usb_phy_reset(struct dwc3 *dwc3_reg) {
+	/* Assert USB3 PHY reset */
+	setbits_le32(&dwc3_reg->g_usb3pipectl[0],
+DWC3_GUSB3PIPECTL_PHYSOFTRST);
+
+	/* Assert USB2 PHY reset */
+	setbits_le32(&dwc3_reg->g_usb2phycfg,
DWC3_GUSB2PHYCFG_PHYSOFTRST);

Quoted 8 lines

Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+
+	mdelay(200);
That's some lazy crappy controller. Is this long delay needed ?
Yeah can you just keep it as is for some time.
This is how we tested on our emulation platforms.
I will anyway modify it at later point of time.

Why can't you modify it now ? 200mS is just too long in my opinion, what's the reason for such a long delay ?

[...]

Siva Durga Prasad Paladugu

author

2015-09-01 13:37 UTC #7 lore raw

PATCH v2 1/2 usb: zynqmp: Add XHCI driver support

Hi,

Quoted 49 lines

-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Tuesday, September 01, 2015 6:50 PM
To: Siva Durga Prasad Paladugu
Cc: u-boot at lists.denx.de; monstr at monstr.eu
Subject: Re: [PATCH v2 1/2] usb: zynqmp: Add XHCI driver support
Quoted 1 line · Siva Durga Prasad Paladugu
HI Marek,
Hi,
Quoted 36 lines
Quoted 22 lines
-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Tuesday, September 01, 2015 5:09 PM
To: Siva Durga Prasad Paladugu
Cc: u-boot at lists.denx.de; Siva Durga Prasad Paladugu; monstr at monstr.eu
Subject: Re: [PATCH v2 1/2] usb: zynqmp: Add XHCI driver support
On Tuesday, September 01, 2015 at 12:31:02 PM, Siva Durga Prasad Paladugu
Quoted 3 lines · wrote:
Added USB XHCI driver support for zynqmp.
Signed-off-by Siva Durga Prasad Paladugu <[email protected]>
Hi, looks almost good, a few minor nits though ...
[...]
Quoted 1 line
+unsigned long ctr_addr[] = {ZYNQMP_USB0_XHCI_BASEADDR,
Quoted 8 lines
static const void __iomem *ctl_addr[]
drivers/usb/host/xhci-zynqmp.c line 68
Quoted 1 line
drivers/usb/host/xhci-zynqmp.c new file +1 -0
+			    ZYNQMP_USB1_XHCI_BASEADDR};
I guess you can define something like CONFIG_ZYNQMP_XHCI_LIST { address ... } in your board config file and then use static const unsigned long ctl_addr[] = CONFIG_ZYNQMP... ; This will cover board which only use one controller.
Yeah DT is the ideal way,
For now, I can modify it to be like this static const void __iomem *ctl_addr[] = { ZYNQMP_USB0_XHCI_BASEADDR,
					ZYNQMP_USB1_XHCI_BASEADDR};

Quoted 5 lines

Quoted 3 lines
But to define a macro in board config file, I may have to include hardware.h, where iam defining all base addresses of the IP's into the board config file just for this.
Is that a problem ?

That's not at all a problem.

Quoted 5 lines

Quoted 2 lines
Is it fine if I can keep as I mentioned above?
I am not very fond of it, since this is broken for boards which don't use both controllers.

Can you tell me, how it will be broken for boards which don't use two controllers,
we are anyway specifying MAX controller count in board config and only those initialized base on that.

Quoted 22 lines

Quoted 22 lines
Quoted 21 lines
The ideal way would be to obtain these information from DT though.
drivers/usb/host/xhci-zynqmp.c
Quoted 11 lines
drivers/usb/host/xhci-zynqmp.c new file +11 -0
+__weak int __board_usb_init(int index, enum usb_init_type init) {
+	return 0;
+}
+
+void usb_phy_reset(struct dwc3 *dwc3_reg) {
+	/* Assert USB3 PHY reset */
+	setbits_le32(&dwc3_reg->g_usb3pipectl[0],
+DWC3_GUSB3PIPECTL_PHYSOFTRST);
+
+	/* Assert USB2 PHY reset */
+	setbits_le32(&dwc3_reg->g_usb2phycfg,
DWC3_GUSB2PHYCFG_PHYSOFTRST);
Review
Quoted 2 lines
(quoted diff) +2 -0
+
+	mdelay(200);
That's some lazy crappy controller. Is this long delay needed ?

Quoted 6 lines

Quoted 3 lines
Yeah can you just keep it as is for some time.
This is how we tested on our emulation platforms.
I will anyway modify it at later point of time.
Why can't you modify it now ? 200mS is just too long in my opinion, what's the reason for such a long delay ?

I will just try with less delay and let you know. But generally how much delay do you think would be sufficient?

Thanks,
Siva

Quoted 2 lines

[...]

Marek Vasut

2015-09-01 13:58 UTC #8 lore raw

PATCH v2 1/2 usb: zynqmp: Add XHCI driver support

Quoted 61 lines · Siva Durga Prasad Paladugu

Hi,
Quoted 49 lines
-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Tuesday, September 01, 2015 6:50 PM
To: Siva Durga Prasad Paladugu
Cc: u-boot at lists.denx.de; monstr at monstr.eu
Subject: Re: [PATCH v2 1/2] usb: zynqmp: Add XHCI driver support
On Tuesday, September 01, 2015 at 02:48:27 PM, Siva Durga Prasad Paladugu
Quoted 1 line · wrote:
HI Marek,
Hi,
Quoted 35 lines
Quoted 22 lines
-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Tuesday, September 01, 2015 5:09 PM
To: Siva Durga Prasad Paladugu
Cc: u-boot at lists.denx.de; Siva Durga Prasad Paladugu; monstr at monstr.eu
Subject: Re: [PATCH v2 1/2] usb: zynqmp: Add XHCI driver support
On Tuesday, September 01, 2015 at 12:31:02 PM, Siva Durga Prasad Paladugu
Quoted 3 lines · wrote:
Added USB XHCI driver support for zynqmp.
Signed-off-by Siva Durga Prasad Paladugu <[email protected]>
Hi, looks almost good, a few minor nits though ...
[...]
Quoted 1 line
+unsigned long ctr_addr[] = {ZYNQMP_USB0_XHCI_BASEADDR,
Quoted 8 lines
static const void __iomem *ctl_addr[]
drivers/usb/host/xhci-zynqmp.c line 68
Quoted 1 line
drivers/usb/host/xhci-zynqmp.c new file +1 -0
+			    ZYNQMP_USB1_XHCI_BASEADDR};
I guess you can define something like CONFIG_ZYNQMP_XHCI_LIST { address ... } in your board config file and then use static const unsigned long ctl_addr[] = CONFIG_ZYNQMP... ; This will cover board which only use one controller.
Yeah DT is the ideal way,
For now, I can modify it to be like this static const void __iomem *ctl_addr[] = { ZYNQMP_USB0_XHCI_BASEADDR,
Quoted 7 lines
Quoted 5 lines
					ZYNQMP_USB1_XHCI_BASEADDR};
But to define a macro in board config file, I may have to include hardware.h, where iam defining all base addresses of the IP's into the board config file just for this.
Is that a problem ?
That's not at all a problem.

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
Is it fine if I can keep as I mentioned above?
I am not very fond of it, since this is broken for boards which don't use both controllers.
Can you tell me, how it will be broken for boards which don't use two controllers, we are anyway specifying MAX controller count in board config and only those initialized base on that.

How do you configure this thing to use only controller #1 and not controller #0?

Quoted 25 lines

Quoted 25 lines
Quoted 24 lines
Quoted 16 lines
The ideal way would be to obtain these information from DT though.
drivers/usb/host/xhci-zynqmp.c
Quoted 11 lines
drivers/usb/host/xhci-zynqmp.c new file +11 -0
+__weak int __board_usb_init(int index, enum usb_init_type init) {
+	return 0;
+}
+
+void usb_phy_reset(struct dwc3 *dwc3_reg) {
+	/* Assert USB3 PHY reset */
+	setbits_le32(&dwc3_reg->g_usb3pipectl[0],
+DWC3_GUSB3PIPECTL_PHYSOFTRST);
+
+	/* Assert USB2 PHY reset */
+	setbits_le32(&dwc3_reg->g_usb2phycfg,
DWC3_GUSB2PHYCFG_PHYSOFTRST);
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+
+	mdelay(200);
That's some lazy crappy controller. Is this long delay needed ?
Yeah can you just keep it as is for some time.
This is how we tested on our emulation platforms.
I will anyway modify it at later point of time.

Quoted 5 lines

Quoted 2 lines
Why can't you modify it now ? 200mS is just too long in my opinion, what's the reason for such a long delay ?
I will just try with less delay and let you know. But generally how much delay do you think would be sufficient?

I don't know your controller, check the datasheet ;-) For reset, it's usually tens of uSec .

Siva Durga Prasad Paladugu

author

2015-09-01 15:58 UTC #9 lore raw

PATCH v2 1/2 usb: zynqmp: Add XHCI driver support

Hi,

Quoted 76 lines

-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Tuesday, September 01, 2015 7:28 PM
To: Siva Durga Prasad Paladugu
Cc: u-boot at lists.denx.de; monstr at monstr.eu
Subject: Re: [PATCH v2 1/2] usb: zynqmp: Add XHCI driver support
Quoted 67 lines · Siva Durga Prasad Paladugu
Hi,
Quoted 50 lines
-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Tuesday, September 01, 2015 6:50 PM
To: Siva Durga Prasad Paladugu
Cc: u-boot at lists.denx.de; monstr at monstr.eu
Subject: Re: [PATCH v2 1/2] usb: zynqmp: Add XHCI driver support
On Tuesday, September 01, 2015 at 02:48:27 PM, Siva Durga Prasad Paladugu
Quoted 1 line · wrote:
HI Marek,
Hi,
Quoted 35 lines
Quoted 22 lines
-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Tuesday, September 01, 2015 5:09 PM
To: Siva Durga Prasad Paladugu
Cc: u-boot at lists.denx.de; Siva Durga Prasad Paladugu; monstr at monstr.eu
Subject: Re: [PATCH v2 1/2] usb: zynqmp: Add XHCI driver support
On Tuesday, September 01, 2015 at 12:31:02 PM, Siva Durga Prasad Paladugu
Quoted 3 lines · wrote:
Added USB XHCI driver support for zynqmp.
Signed-off-by Siva Durga Prasad Paladugu <[email protected]>
Hi, looks almost good, a few minor nits though ...
[...]
Quoted 1 line
+unsigned long ctr_addr[] = {ZYNQMP_USB0_XHCI_BASEADDR,
Quoted 8 lines
static const void __iomem *ctl_addr[]
drivers/usb/host/xhci-zynqmp.c line 68
Quoted 1 line
drivers/usb/host/xhci-zynqmp.c new file +1 -0
+			    ZYNQMP_USB1_XHCI_BASEADDR};
I guess you can define something like CONFIG_ZYNQMP_XHCI_LIST { address ... } in your board config file and then use static const unsigned long ctl_addr[] = CONFIG_ZYNQMP... ; This will cover board which only use one controller.
Yeah DT is the ideal way,
For now, I can modify it to be like this static const void __iomem *ctl_addr[] = { ZYNQMP_USB0_XHCI_BASEADDR,
Quoted 7 lines
Quoted 5 lines
					ZYNQMP_USB1_XHCI_BASEADDR};
But to define a macro in board config file, I may have to include hardware.h, where iam defining all base addresses of the IP's into the board config file just for this.
Is that a problem ?
That's not at all a problem.
Quoted 4 lines
Quoted 1 line
Is it fine if I can keep as I mentioned above?
I am not very fond of it, since this is broken for boards which don't use both controllers.

Quoted 6 lines

Quoted 3 lines
Can you tell me, how it will be broken for boards which don't use two controllers, we are anyway specifying MAX controller count in board config and only those initialized base on that.
How do you configure this thing to use only controller #1 and not controller #0?

I got your point now, Thanks for the clarification. Will make the changes accordingly.

Thanks,
Siva

Quoted 34 lines

Quoted 26 lines
Quoted 26 lines
Quoted 25 lines
Quoted 17 lines
The ideal way would be to obtain these information from DT though.
drivers/usb/host/xhci-zynqmp.c
Quoted 11 lines
drivers/usb/host/xhci-zynqmp.c new file +11 -0
+__weak int __board_usb_init(int index, enum usb_init_type init) {
+	return 0;
+}
+
+void usb_phy_reset(struct dwc3 *dwc3_reg) {
+	/* Assert USB3 PHY reset */
+	setbits_le32(&dwc3_reg->g_usb3pipectl[0],
+DWC3_GUSB3PIPECTL_PHYSOFTRST);
+
+	/* Assert USB2 PHY reset */
+	setbits_le32(&dwc3_reg->g_usb2phycfg,
DWC3_GUSB2PHYCFG_PHYSOFTRST);
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+
+	mdelay(200);
That's some lazy crappy controller. Is this long delay needed ?
Yeah can you just keep it as is for some time.
This is how we tested on our emulation platforms.
I will anyway modify it at later point of time.
Quoted 5 lines
Quoted 2 lines
Why can't you modify it now ? 200mS is just too long in my opinion, what's the reason for such a long delay ?
I will just try with less delay and let you know. But generally how much delay do you think would be sufficient?
I don't know your controller, check the datasheet ;-) For reset, it's usually tens of uSec .