[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 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 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 ?

[...]