lkml.org 
[lkml]   [2014]   [Mar]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH] USB: Gadget: fsl driver pullup fix
Date


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

Thanks
SuresH


\
 
 \ /
  Last update: 2014-03-19 16:01    [W:0.064 / U:16.172 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site