[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 at 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;
> > +}