Re: [PATCH] USB: Gadget: fsl driver pullup fix

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 5122 bytes --]

On Wed, Mar 19, 2014 at 02:23:59PM +0000, suresh.gupta@freescale.com wrote:
> 
> 
> > -----Original Message-----
> > From: Felipe Balbi [mailto:balbi@ti.com]
> > Sent: Saturday, March 15, 2014 7:05 AM
> > To: Gupta Suresh-B42813
> > Cc: balbi@ti.com; gregkh@linuxfoundation.org; linux-usb@vger.kernel.org;
> > linux-kernel@vger.kernel.org; Stefani Seibold
> > Subject: Re: [PATCH] USB: Gadget: fsl driver pullup fix
> > 
> > Hi,
> > 
> > (first of all, please fix your email client, we need the quotation marks.
> > See Documentation/email-clients.txt)
> > 
> > On Fri, Mar 14, 2014 at 08:53:24PM +0000, suresh.gupta@freescale.com
> > wrote:
> > > > On Thu, Mar 13, 2014 at 06:40:55PM +0530, Suresh Gupta wrote:
> > > > > Attached is a small fix for the fsl usb gadget driver. This fix
> > > > > the driver in a way that the usb device will be only "pulled up"
> > > > > on requests like other usb gadget drivers do.
> > > > > This is necessary, because the device information is not always
> > > > > available until an application is up and running which provides
> > > > > this datas.
> > > > >
> > > > > Signed-off-by: Stefani Seibold <stefani@seibold.net>
> > > > > Signed-off-by: Suresh Gupta <suresh.gupta@freescale.com>
> > > > > ---
> > > > >  drivers/usb/gadget/fsl_udc_core.c | 38
> > > > > +++++++++++++++++++++-----------------
> > > > >  1 file changed, 21 insertions(+), 17 deletions(-)
> > > > >
> > > > > diff --git a/drivers/usb/gadget/fsl_udc_core.c
> > > > > b/drivers/usb/gadget/fsl_udc_core.c
> > > > > index 35cb972..9a93727 100644
> > > > > --- a/drivers/usb/gadget/fsl_udc_core.c
> > > > > +++ b/drivers/usb/gadget/fsl_udc_core.c
> > > > > @@ -153,6 +153,21 @@ static inline void fsl_set_accessors(struct
> > > > > fsl_usb2_platform_data *pdata) {}
> > > > >
> > /********************************************************************
> > > > >   *	Internal Used Function
> > > > >
> > > > > ******************************************************************
> > > > > **/
> > > > > +static int can_pullup(struct fsl_udc *udc) {
> > > > > +	return udc->driver && udc->softconnect && udc->vbus_active; }
> > > > > +
> > > > > +static void set_pullup(struct fsl_udc *udc) {
> > > > > +	if (can_pullup(udc))
> > > > > +		fsl_writel((fsl_readl(&dr_regs->usbcmd) |
> > USB_CMD_RUN_STOP),
> > > > > +				&dr_regs->usbcmd);
> > > > > +	else
> > > > > +		fsl_writel((fsl_readl(&dr_regs->usbcmd) &
> > ~USB_CMD_RUN_STOP),
> > > > > +				&dr_regs->usbcmd);
> > > > > +}
> > > >
> > > > why is this a "fix", you just re-factored some code into
> > set_pullup().
> > > >
> > > [SuresH] I set udc->vbus_active and udc->softconnect to default value
> > > of 1 in struct_udc_setup. This was actual fix in this patch.  The
> > 
> > right, you see now why is it a problem to mix cleanups with fixes ? You
> > *never*, ever combine unrelated changes in a single patch. It makes it a
> > lot more difficult to see what you're actually changing. So, to start
> > with, this patch should (if it was correct) be split into two smaller
> > patches: one re-factoring the duplicated code into set_pullup() and
> > another which fixes vbus_active and softconnect flags.
> 
> Agreed, I will resend the patches.  
> 
> > 
> > But hang on, before you do that...
> > 
> > > can_pullup function return false when these values was not set and
> > > intern the code return without enabling the pullup and gadget
> > > controller stops.
> > 
> > So here's you mistake: the idea of can_pullup() (and thus, vbus_active
> > and softconnect flags) is to tell the driver "we're connet to a host,
> > it's safe to connect your pullups".
> > 
> > When you set both those flags to true, you're telling the driver that
> > you, indeed, are connected to a host. This might not be true if you first
> > boot up your platform, load all drivers and only after connect the cable.
> > In essence, you're lying to your driver and, as my mommy used to say,
> > "nobody likes a liar, my boy".
> > 
> > Curret situation isn't very good either since the driver is assuming that
> > cable is only plugged after driver is loaded, so it won't cope very well
> > with situation where cable is first plugged, then you apply power to your
> > board.
> > 
> > What you *really* need to do here is ask the HW for initial states of
> > those flags. During your probe() routine - as the name says - you probe
> > your HW to check its state (or to initialize its state), then you ask
> > "Hey IP, is VBUS above session valid threshold ?" Then you use the HW's
> > reply to initialize both flags, the way you want.
> 
> After your explanation and some code browsing, I think the exact place to
> set vbus_active is fsl_vbus_session which called on detecting vbus.
> Also fsl_pullup should return without doing anything if vbus_active is not set.
> Please suggest.
> 
> I do not get the usage of softconnect. Do I set softconnect also in
> fsl_vbus_session.  Please suggest.

looks like softconnect should be set when pullups are connected.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]