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

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

Thanks,
Siva
> 
> [...]