RE: [PATCH 1/7 v1] USB: EHCI: Add Marvell Host Controller driver

[Neil Zhang <zhangwm@xxxxxxxxxxx>]

> -----Original Message-----
> From: Alan Stern [mailto:stern@xxxxxxxxxxxxxxxxxxx]
> Sent: 2011年11月25日 0:16
> To: Neil Zhang
> Cc: balbi@xxxxxx; gregkh@xxxxxxx; linux-usb@xxxxxxxxxxxxxxx; Chao Xie
> Subject: Re: [PATCH 1/7 v1] USB: EHCI: Add Marvell Host Controller
> driver
> 
> On Thu, 24 Nov 2011, Neil Zhang wrote:
> 
> > This patch adds support for EHCI compliant HSUSB Host controller
> found
> > on Marvell Socs.
> >
> > It fits both OTG and SPH controller on marvell Socs, including
> > PXA920/MMP2/MMP3/MGx.
> >
> > Signed-off-by: Neil Zhang <zhangwm@xxxxxxxxxxx>
> 
> > --- a/drivers/usb/host/Kconfig
> > +++ b/drivers/usb/host/Kconfig
> > @@ -194,6 +194,30 @@ config USB_EHCI_S5P
> >         help
> >  	 Enable support for the S5P SOC's on-chip EHCI controller.
> >
> > +config USB_EHCI_PXA
> > +	bool "EHCI support for PXA USB SPH and OTG controller"
> > +	depends on USB_EHCI_HCD
> > +	select USB_EHCI_ROOT_HUB_TT
> > +	---help---
> > +	  Enables support for PXA USB SPH and OTG controller. SPH is a
> > +	  single port host, and it can only be EHCI host. OTG is
> controller
> > +	  that can switch to be host mode.
> 
> This help message is confusing.  If CONFIG_USB_EHCI_PXA enables support
> for the PXA USB OTG controller, why do you also need
> CONFIG_USB_EHCI_PXA_U2O?
> 
> Maybe the message should say something simpler, like "Enables EHCI
> support for the PXA's USB controller".  Or maybe this config option
> doesn't need to exist at all?
> 
> > +
> > +config USB_EHCI_PXA_U2O
> > +	bool "EHCI support for PXA USB OTG controller"
> > +	depends on USB_EHCI_PXA
> > +	---help---
> > +	   Enables support for OTG controller. OTG is controller
> > +	   that can switch to be host mode.
> > +
> > +config USB_EHCI_PXA_U2H_HSIC
> > +	bool "EHCI support for PXA USB SPH of HSIC"
> > +	depends on USB_EHCI_PXA
> > +	---help---
> > +	  Enables support for PXA USB SPH HSIC. SPH is a
> > +	  single port host, and it can only be EHCI host. HSIC can only
> > +	  run in High Speed.
> 
> This also is confusing.  Is PXA USB SPH the same as or different from
> PXA USB SPH of HSIC?
> 

Thanks for your remind. 
Actually, we should only leave USB_EHCI_PXA here, move USB_EHCI_PXA_U2O 
and USB_EHCI_PXA_U2H_HSIC into our platform's Kconfig.

> > +
> >  config USB_W90X900_EHCI
> >  	bool "W90X900(W90P910) EHCI support"
> >  	depends on USB_EHCI_HCD && ARCH_W90X900
> > diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-
> hcd.c
> > index 3ff9f82..4eb19f8 100644
> > --- a/drivers/usb/host/ehci-hcd.c
> 
> > +++ b/drivers/usb/host/ehci-hcd.c
> 
> ...
> 
> > +static const struct hc_driver mv_ehci_hc_driver = {
> > +	.description = hcd_name,
> > +	.product_desc = "Marvell EHCI",
> > +	.hcd_priv_size = sizeof(struct ehci_hcd),
> > +
> > +	/*
> > +	 * generic hardware linkage
> > +	 */
> > +	.irq = ehci_irq,
> > +	.flags = HCD_MEMORY | HCD_USB2,
> > +
> > +	/*
> > +	 * basic lifecycle operations
> > +	 */
> > +	.reset = mv_ehci_reset,
> > +	.start = ehci_run,
> > +	.stop = ehci_stop,
> > +	.shutdown = ehci_shutdown,
> > +
> > +	/*
> > +	 * managing i/o requests and associated device resources
> > +	 */
> > +	.urb_enqueue = ehci_urb_enqueue,
> > +	.urb_dequeue = ehci_urb_dequeue,
> > +	.endpoint_disable = ehci_endpoint_disable,
> > +
> > +	/*
> > +	 * scheduling support
> > +	 */
> > +	.get_frame_number = ehci_get_frame,
> > +
> > +	/*
> > +	 * root hub support
> > +	 */
> > +	.hub_status_data = ehci_hub_status_data,
> > +	.hub_control = ehci_hub_control,
> > +	.bus_suspend = ehci_bus_suspend,
> > +	.bus_resume = ehci_bus_resume,
> > +};
> 
> What happened to endpoint_reset and clear_tt_buffer_complete?

I will add them, thanks.

> 
> Alan Stern

?韬{.n?????%??檩??w?{.n???{炳???骅w*jg????????G??⒏⒎?:+v????????????＂??????