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