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

[suresh.gupta@xxxxxxxxxxxxx]

> -----Original Message-----
> From: Felipe Balbi [mailto:balbi@xxxxxx]
> Sent: Saturday, March 15, 2014 7:05 AM
> To: Gupta Suresh-B42813
> Cc: balbi@xxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; 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@xxxxxxxxxxxxx
> 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@xxxxxxxxxxx>
> > > > Signed-off-by: Suresh Gupta <suresh.gupta@xxxxxxxxxxxxx>
> > > > ---
> > > >  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.

Thanks
SuresH
--
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/