[U-Boot] [PATCH v2 1/2] usb: zynqmp: Add XHCI driver support

[U-Boot] [PATCH v2 1/2] usb: zynqmp: Add XHCI driver support

[Siva Durga Prasad Paladugu]

Added USB XHCI driver support for zynqmp.

Signed-off-by: Siva Durga Prasad Paladugu <sivadur@xilinx.com>
---
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

diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index f70f38c..645b990 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -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
diff --git a/drivers/usb/host/xhci-zynqmp.c b/drivers/usb/host/xhci-zynqmp.c
new file mode 100644
index 0000000..bb23afa
--- /dev/null
+++ b/drivers/usb/host/xhci-zynqmp.c
@@ -0,0 +1,147 @@
+/*
+ * Copyright 2015 Xilinx, Inc.
+ *
+ * Zynq USB HOST xHCI Controller
+ *
+ * Author: Siva Durga Prasad Paladugu<sivadur@xilinx.com>
+ *
+ * This file was reused from Freescale USB xHCI
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <usb.h>
+#include <asm-generic/errno.h>
+#include <asm/arch-zynqmp/hardware.h>
+#include <linux/compat.h>
+#include <linux/usb/xhci-zynqmp.h>
+#include <linux/usb/dwc3.h>
+#include "xhci.h"
+
+/* Declare global data pointer */
+DECLARE_GLOBAL_DATA_PTR;
+
+/* Default to the ZYNQMP XHCI defines */
+#define USB3_PWRCTL_CLK_CMD_MASK	0x3FE000
+#define USB3_PWRCTL_CLK_FREQ_MASK	0xFFC
+#define USB3_PHY_PARTIAL_RX_POWERON     BIT(6)
+#define USB3_PHY_RX_POWERON		BIT(14)
+#define USB3_PHY_TX_POWERON		BIT(15)
+#define USB3_PHY_TX_RX_POWERON	(USB3_PHY_RX_POWERON | USB3_PHY_TX_POWERON)
+#define USB3_PWRCTL_CLK_CMD_SHIFT   14
+#define USB3_PWRCTL_CLK_FREQ_SHIFT	22
+
+/* USBOTGSS_WRAPPER definitions */
+#define USBOTGSS_WRAPRESET	BIT(17)
+#define USBOTGSS_DMADISABLE BIT(16)
+#define USBOTGSS_STANDBYMODE_NO_STANDBY BIT(4)
+#define USBOTGSS_STANDBYMODE_SMRT		BIT(5)
+#define USBOTGSS_STANDBYMODE_SMRT_WKUP (0x3 << 4)
+#define USBOTGSS_IDLEMODE_NOIDLE BIT(2)
+#define USBOTGSS_IDLEMODE_SMRT BIT(3)
+#define USBOTGSS_IDLEMODE_SMRT_WKUP (0x3 << 2)
+
+/* USBOTGSS_IRQENABLE_SET_0 bit */
+#define USBOTGSS_COREIRQ_EN	BIT(1)
+
+/* USBOTGSS_IRQENABLE_SET_1 bits */
+#define USBOTGSS_IRQ_SET_1_IDPULLUP_FALL_EN	BIT(1)
+#define USBOTGSS_IRQ_SET_1_DISCHRGVBUS_FALL_EN	BIT(3)
+#define USBOTGSS_IRQ_SET_1_CHRGVBUS_FALL_EN	BIT(4)
+#define USBOTGSS_IRQ_SET_1_DRVVBUS_FALL_EN	BIT(5)
+#define USBOTGSS_IRQ_SET_1_IDPULLUP_RISE_EN	BIT(8)
+#define USBOTGSS_IRQ_SET_1_DISCHRGVBUS_RISE_EN	BIT(11)
+#define USBOTGSS_IRQ_SET_1_CHRGVBUS_RISE_EN	BIT(12)
+#define USBOTGSS_IRQ_SET_1_DRVVBUS_RISE_EN	BIT(13)
+#define USBOTGSS_IRQ_SET_1_OEVT_EN		BIT(16)
+#define USBOTGSS_IRQ_SET_1_DMADISABLECLR_EN	BIT(17)
+
+struct zynqmp_xhci {
+	struct xhci_hccr *hcd;
+	struct dwc3 *dwc3_reg;
+};
+
+static struct zynqmp_xhci zynqmp_xhci;
+
+unsigned long ctr_addr[] = {ZYNQMP_USB0_XHCI_BASEADDR,
+			    ZYNQMP_USB1_XHCI_BASEADDR};
+
+__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);
+
+	mdelay(200);
+
+	/* 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)
+{
+	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;
+	}
+
+	*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",
+	      (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;
+}
-- 
1.7.1

[U-Boot] [PATCH v2 2/2] usb: zynqmp: Enable USB XHCI support

[Siva Durga Prasad Paladugu]

Enable USB XHCI support for ZynqMP

Signed-off-by: Siva Durga Prasad Paladugu <sivadur@xilinx.com>
---
Changes for v2:
- None
---
 include/configs/xilinx_zynqmp.h |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/include/configs/xilinx_zynqmp.h b/include/configs/xilinx_zynqmp.h
index da87188..a9556d5 100644
--- a/include/configs/xilinx_zynqmp.h
+++ b/include/configs/xilinx_zynqmp.h
@@ -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
 
-- 
1.7.1

[U-Boot] [PATCH v2 1/2] usb: zynqmp: Add XHCI driver support

[Marek Vasut]

On Tuesday, September 01, 2015 at 12:31:02 PM, Siva Durga Prasad Paladugu wrote:
> Added USB XHCI driver support for zynqmp.
> 
> Signed-off-by: Siva Durga Prasad Paladugu <sivadur@xilinx.com>

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

[...]

> +unsigned long ctr_addr[] = {ZYNQMP_USB0_XHCI_BASEADDR,

static const void __iomem *ctl_addr[]

> +			    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.

> +__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);
> +
> +	mdelay(200);

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

> +	/* 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) +{
> +	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;
> +	}
> +
> +	*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.

> +	      (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;
> +}

[U-Boot] [PATCH v2 2/2] usb: zynqmp: Enable USB XHCI support

[Marek Vasut]

On Tuesday, September 01, 2015 at 12:31:03 PM, Siva Durga Prasad Paladugu wrote:
> Enable USB XHCI support for ZynqMP
> 
> Signed-off-by: Siva Durga Prasad Paladugu <sivadur@xilinx.com>
> ---
> Changes for v2:
> - None
> ---
>  include/configs/xilinx_zynqmp.h |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/include/configs/xilinx_zynqmp.h
> b/include/configs/xilinx_zynqmp.h index da87188..a9556d5 100644
> --- a/include/configs/xilinx_zynqmp.h
> +++ b/include/configs/xilinx_zynqmp.h
> @@ -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

Drop this space between # and define please.

> +#define CONFIG_USB_XHCI_ZYNQMP
> +
>  #define CONFIG_USB_DWC3
>  #define CONFIG_USB_DWC3_GADGET

Best regards,
Marek Vasut

[U-Boot] [PATCH v2 1/2] usb: zynqmp: Add XHCI driver support

[Siva Durga Prasad Paladugu]

HI Marek,

> -----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
> wrote:
> > Added USB XHCI driver support for zynqmp.
> >
> > Signed-off-by: Siva Durga Prasad Paladugu <sivadur@xilinx.com>
> 
> Hi, looks almost good, a few minor nits though ...
> 
> [...]
> 
> > +unsigned long ctr_addr[] = {ZYNQMP_USB0_XHCI_BASEADDR,
> 
> static const void __iomem *ctl_addr[]
> 
> > +			    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?

> The ideal way would be to obtain these information from DT though.
> 
> > +__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);
> > +
> > +	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.

> 
> > +	/* 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) +{
> > +	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;
> > +	}
> > +
> > +	*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.
OK

Thanks,
Siva
> 
> > +	      (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;
> > +}

[U-Boot] [PATCH v2 2/2] usb: zynqmp: Enable USB XHCI support

[Siva Durga Prasad Paladugu]

> -----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
> 
> On Tuesday, September 01, 2015 at 12:31:03 PM, Siva Durga Prasad Paladugu
> wrote:
> > Enable USB XHCI support for ZynqMP
> >
> > Signed-off-by: Siva Durga Prasad Paladugu <sivadur@xilinx.com>
> > ---
> > Changes for v2:
> > - None
> > ---
> >  include/configs/xilinx_zynqmp.h |    8 ++++++++
> >  1 files changed, 8 insertions(+), 0 deletions(-)
> >
> > diff --git a/include/configs/xilinx_zynqmp.h
> > b/include/configs/xilinx_zynqmp.h index da87188..a9556d5 100644
> > --- a/include/configs/xilinx_zynqmp.h
> > +++ b/include/configs/xilinx_zynqmp.h
> > @@ -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
> 
> Drop this space between # and define please.
Ok will do that in V3

Thanks,
Siva
> 
> > +#define CONFIG_USB_XHCI_ZYNQMP
> > +
> >  #define CONFIG_USB_DWC3
> >  #define CONFIG_USB_DWC3_GADGET
> 
> Best regards,
> Marek Vasut

[U-Boot] [PATCH v2 1/2] usb: zynqmp: Add XHCI driver support

[Marek Vasut]

On Tuesday, September 01, 2015 at 02:48:27 PM, Siva Durga Prasad Paladugu wrote:
> HI Marek,

Hi,

> > -----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
> > 
> > wrote:
> > > Added USB XHCI driver support for zynqmp.
> > > 
> > > Signed-off-by: Siva Durga Prasad Paladugu <sivadur@xilinx.com>
> > 
> > Hi, looks almost good, a few minor nits though ...
> > 
> > [...]
> > 
> > > +unsigned long ctr_addr[] = {ZYNQMP_USB0_XHCI_BASEADDR,
> > 
> > static const void __iomem *ctl_addr[]
> > 
> > > +			    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 that a problem ?

> 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.

> > The ideal way would be to obtain these information from DT though.
> > 
> > > +__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);
> > 
> > > +
> > > +	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 ?

[...]

[U-Boot] [PATCH v2 1/2] usb: zynqmp: Add XHCI driver support

[Siva Durga Prasad Paladugu]

Hi,

> -----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
> wrote:
> > HI Marek,
> 
> Hi,
> 
> > > -----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
> > >
> > > wrote:
> > > > Added USB XHCI driver support for zynqmp.
> > > >
> > > > Signed-off-by: Siva Durga Prasad Paladugu <sivadur@xilinx.com>
> > >
> > > Hi, looks almost good, a few minor nits though ...
> > >
> > > [...]
> > >
> > > > +unsigned long ctr_addr[] = {ZYNQMP_USB0_XHCI_BASEADDR,
> > >
> > > static const void __iomem *ctl_addr[]
> > >
> > > > +			    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 that a problem ?
That's not at all a problem.
> 
> > 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.

> 
> > > The ideal way would be to obtain these information from DT though.
> > >
> > > > +__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);
> > >
> > > > +
> > > > +	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 ?
I will just try with less delay and let you know. 
But generally how much delay do you think would be sufficient?

Thanks,
Siva
> 
> [...]

[U-Boot] [PATCH v2 1/2] usb: zynqmp: Add XHCI driver support

[Marek Vasut]

On Tuesday, September 01, 2015 at 03:37:12 PM, Siva Durga Prasad Paladugu wrote:
> Hi,
> 
> > -----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
> > 
> > wrote:
> > > HI Marek,
> > 
> > Hi,
> > 
> > > > -----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
> > > > 
> > > > wrote:
> > > > > Added USB XHCI driver support for zynqmp.
> > > > > 
> > > > > Signed-off-by: Siva Durga Prasad Paladugu <sivadur@xilinx.com>
> > > > 
> > > > Hi, looks almost good, a few minor nits though ...
> > > > 
> > > > [...]
> > > > 
> > > > > +unsigned long ctr_addr[] = {ZYNQMP_USB0_XHCI_BASEADDR,
> > > > 
> > > > static const void __iomem *ctl_addr[]
> > > > 
> > > > > +			    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 that a problem ?
> 
> That's not at all a problem.
> 
> > > 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?

> > > > The ideal way would be to obtain these information from DT though.
> > > > 
> > > > > +__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);
> > > > 
> > > > > +
> > > > > +	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 ?
> 
> 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 .

[U-Boot] [PATCH v2 1/2] usb: zynqmp: Add XHCI driver support

[Siva Durga Prasad Paladugu]

Hi,

> -----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
> 
> On Tuesday, September 01, 2015 at 03:37:12 PM, Siva Durga Prasad Paladugu
> wrote:
> > Hi,
> >
> > > -----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
> > >
> > > wrote:
> > > > HI Marek,
> > >
> > > Hi,
> > >
> > > > > -----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
> > > > >
> > > > > wrote:
> > > > > > Added USB XHCI driver support for zynqmp.
> > > > > >
> > > > > > Signed-off-by: Siva Durga Prasad Paladugu <sivadur@xilinx.com>
> > > > >
> > > > > Hi, looks almost good, a few minor nits though ...
> > > > >
> > > > > [...]
> > > > >
> > > > > > +unsigned long ctr_addr[] = {ZYNQMP_USB0_XHCI_BASEADDR,
> > > > >
> > > > > static const void __iomem *ctl_addr[]
> > > > >
> > > > > > +			    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 that a problem ?
> >
> > That's not at all a problem.
> >
> > > > 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?
I got your point now, Thanks for the clarification. Will make the changes accordingly.

Thanks,
Siva
> 
> > > > > The ideal way would be to obtain these information from DT though.
> > > > >
> > > > > > +__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);
> > > > >
> > > > > > +
> > > > > > +	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 ?
> >
> > 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 .