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