RE: [PATCH 2/9][v2]usb:fsl:otg: Add support to add/remove usb host driver

[Ramneek Mehresh]

> -----Original Message-----
> From: Mehresh Ramneek-B31383
> Sent: Monday, April 20, 2015 10:05 PM
> To: 'Alan Stern'
> Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; balbi@xxxxxx;
> gregkh@xxxxxxxxxxxxxxxxxxx
> Subject: RE: [PATCH 2/9][v2]usb:fsl:otg: Add support to add/remove usb
> host driver
> 
> 
> 
> > -----Original Message-----
> > From: Alan Stern [mailto:stern@xxxxxxxxxxxxxxxxxxx]
> > Sent: Tuesday, April 07, 2015 9:23 PM
> > To: Mehresh Ramneek-B31383
> > Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx;
> > balbi@xxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx
> > Subject: Re: [PATCH 2/9][v2]usb:fsl:otg: Add support to add/remove usb
> > host driver
> >
> > On Tue, 7 Apr 2015, Ramneek Mehresh wrote:
> >
> > > Add workqueue to add/remove host driver (outside interrupt context)
> > > upon each id change
> >
> > This patch needs to be fixed.  See below.
> >
> Thanks Alan.  I'll correct the patch.
> 
> > > Signed-off-by: Ramneek Mehresh <ramneek.mehresh@xxxxxxxxxxxxx>
> > > ---
> > >  drivers/usb/host/ehci-fsl.c | 107
> > ++++++++++++++++++++++++++++++++++++--------
> > >  drivers/usb/host/ehci.h     |   1 -
> > >  2 files changed, 88 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/drivers/usb/host/ehci-fsl.c
> > > b/drivers/usb/host/ehci-fsl.c index ab4eee3..9c91fbe 100644
> > > --- a/drivers/usb/host/ehci-fsl.c
> > > +++ b/drivers/usb/host/ehci-fsl.c
> > > @@ -33,6 +33,58 @@
> > >
> > >  #include "ehci-fsl.h"
> > >
> > > +struct ehci_fsl {
> > > +	struct ehci_hcd ehci;
> > > +
> > > +#ifdef CONFIG_PM
> > > +	/* Saved USB PHY settings, need to restore after deep sleep. */
> > > +	u32 usb_ctrl;
> > > +#if defined(CONFIG_FSL_USB2_OTG) ||
> > defined(CONFIG_FSL_USB2_OTG_MODULE)
> > > +	struct work_struct change_hcd_work; #endif #endif
> > > +
> > > +	/* store current hcd state for otg;
> > > +	 * have_hcd is true when host drv al already part of otg framework,
> > > +	 * otherwise false;
> > > +	 * hcd_add is true when otg framework wants to add host
> > > +	 * drv as part of otg;flase when it wants to remove it
> > > +	 */
> > > +	unsigned have_hcd:1;
> > > +	unsigned hcd_add:1;
> > > +};
> >
> > This is the wrong way to connect platform-private data with the
> > ehci_hcd structure.  The right way is to use an ehci_driver_overrides
> > structure, like in ehci-omap.c and quite a few other EHCI platform drivers.
> >
>
Hi Alan, I checked again...we are not connecting ehci_hcd structure here with 
platform-private data. We are just having a wrapper on it to have some additional 
Information that can be used for ehci fsl driver. This additional information is 
private to ehci fsl driver, and is mainly used for otg driver functionality and to
save/restore fsl regsiters during deep-sleep 

> > > +#if defined(CONFIG_FSL_USB2_OTG) ||
> > > +defined(CONFIG_FSL_USB2_OTG_MODULE)
> > > +static struct ehci_fsl *hcd_to_ehci_fsl(struct usb_hcd *hcd) {
> > > +	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
> > > +
> > > +	return container_of(ehci, struct ehci_fsl, ehci); }
> >
> > This is bound to fail no matter what, because the ehci_hcd structure
> > is already embedded in the usb_hcd structure.  It can't be embedded in
> > an ehci_fsl structure too.
> >
> > (Of course, this won't matter when you switch to the approved way of
> > connecting private data.)
> >
> Will correct
> > > +static void do_change_hcd(struct work_struct *work) {
> > > +	struct ehci_fsl *ehci_fsl = container_of(work, struct ehci_fsl,
> > > +					change_hcd_work);
> > > +	struct ehci_hcd *ehci = &ehci_fsl->ehci;
> > > +	struct usb_hcd *hcd = ehci_to_hcd(ehci);
> > > +
> > > +	void __iomem *non_ehci = hcd->regs;
> > > +	int retval;
> > > +
> > > +	if (ehci_fsl->hcd_add && !ehci_fsl->have_hcd) {
> > > +		writel(USBMODE_CM_HOST, non_ehci +
> > FSL_SOC_USB_USBMODE);
> > > +		/* host, gadget and otg share same int line */
> > > +		retval = usb_add_hcd(hcd, hcd->irq, IRQF_SHARED);
> > > +		if (retval == 0)
> > > +			ehci_fsl->have_hcd = 1;
> > > +	} else if (!ehci_fsl->hcd_add && ehci_fsl->have_hcd) {
> > > +		usb_remove_hcd(hcd);
> > > +		ehci_fsl->have_hcd = 0;
> > > +	}
> > > +}
> >
> > Does this really work right?  What about things like
> > device_wakeup_enable()?  Don't you need to disable the controller when
> > leaving host mode and enable it when entering host mode?
> >
> Yes, this does work, it's tested. Yes, controller needs to be disabled when
> switching the mode from Host to gadget and vice-versa. Only one driver can
> have control of the hardware at a time. that's why call to usb_remove_hcd()
> is made. Gadget driver configures the controller in gadget mode, and takes
> control of the hardware
> > Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/