[U-Boot] [PATCH v5 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: Monday, November 16, 2015 11:39 AM
> To: Siva Durga Prasad Paladugu
> Cc: u-boot at lists.denx.de; Siva Durga Prasad Paladugu
> Subject: Re: [PATCH v5 1/2] usb: zynqmp: Add XHCI driver support
> 
> On Monday, November 16, 2015 at 06:59:13 AM, Siva Durga Prasad Paladugu
> wrote:
> > From: Siva Durga Prasad Paladugu <siva.durga.paladugu at xilinx.com>
> >
> > Added USB XHCI driver support for zynqmp.
> >
> > Signed-off-by: Siva Durga Prasad Paladugu <sivadur at xilinx.com>
> 
> Hi!
> 
> [...]
> 
> > +static struct zynqmp_xhci zynqmp_xhci;
> > +
> > +unsigned long ctr_addr[] = CONFIG_ZYNQMP_XHCI_LIST;
> 
> Still no DM/DT support on Xilinx parts, huh ? :)
We have this support for Zynq but not for ZynqMP yet as this is new and this 
patch is related to ZynqMP.
> 
> [...]
> 
> > +int xhci_hcd_init(int index, struct xhci_hccr **hccr, struct
> > +xhci_hcor
> > **hcor) +{
> > +	struct zynqmp_xhci *ctx = &zynqmp_xhci;
> > +	int ret = 0;
> > +
> > +	if (index < 0 || index >= ARRAY_SIZE(ctr_addr))
> > +		return -EINVAL;
> > +
> > +	ctx->hcd = (struct xhci_hccr *)ctr_addr[index];
> > +	ctx->dwc3_reg = (struct dwc3 *)((char *)(ctx->hcd) +
> > +DWC3_REG_OFFSET);
> 
> Should be void *, not char * I think. Also, the parenthesis around ctx->hcd
> are not needed.
I think keeping char* would be good for easy understanding and readability.
> 
> > +	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)));
> 
> You want to use uintptr_t for the same of 64bit parts.
Ok
> 
> > +	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)));
> 
> We have %p for printing pointers, so drop the casts.
> 
> Also, since you're using HC_LENGTH() here twice, you might as well put it's
> value into temporary variable and use it in both places, it might make the
> code slightly more readable.

Ok, will take care in next version

Regards,
Siva

> 
> > +	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;
> > +}
> > diff --git a/include/configs/xilinx_zynqmp_ep.h
> > b/include/configs/xilinx_zynqmp_ep.h index ed6023a..1845622 100644
> > --- a/include/configs/xilinx_zynqmp_ep.h
> > +++ b/include/configs/xilinx_zynqmp_ep.h
> > @@ -23,6 +23,8 @@
> >  #define CONFIG_SYS_I2C_ZYNQ
> >  #define CONFIG_ZYNQ_EEPROM
> >  #define CONFIG_AHCI
> > +#define CONFIG_ZYNQMP_XHCI_LIST {ZYNQMP_USB0_XHCI_BASEADDR,
> \
> > +				 ZYNQMP_USB1_XHCI_BASEADDR}
> >
> >  #include <configs/xilinx_zynqmp.h>