[PATCH] USB: Gadget: fsl driver pullup fix

[PATCH] USB: Gadget: fsl driver pullup fix

[Suresh Gupta]

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);
+}
+
 /*-----------------------------------------------------------------
  * done() - retire a request; caller blocked irqs
  * @status : request status to be set, only works when
@@ -283,6 +298,8 @@ static int dr_controller_setup(struct fsl_udc *udc)
 	tmp &= ~USB_CMD_RUN_STOP;
 	fsl_writel(tmp, &dr_regs->usbcmd);
 
+	set_pullup(udc);
+
 	tmp = fsl_readl(&dr_regs->usbcmd);
 	tmp |= USB_CMD_CTRL_RESET;
 	fsl_writel(tmp, &dr_regs->usbcmd);
@@ -1168,11 +1185,6 @@ static int fsl_wakeup(struct usb_gadget *gadget)
 	return 0;
 }
 
-static int can_pullup(struct fsl_udc *udc)
-{
-	return udc->driver && udc->softconnect && udc->vbus_active;
-}
-
 /* Notify controller that VBUS is powered, Called by whatever
    detects VBUS sessions */
 static int fsl_vbus_session(struct usb_gadget *gadget, int is_active)
@@ -1184,12 +1196,7 @@ static int fsl_vbus_session(struct usb_gadget *gadget, int is_active)
 	spin_lock_irqsave(&udc->lock, flags);
 	VDBG("VBUS %s", is_active ? "on" : "off");
 	udc->vbus_active = (is_active != 0);
-	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);
+	set_pullup(udc);
 	spin_unlock_irqrestore(&udc->lock, flags);
 	return 0;
 }
@@ -1220,12 +1227,7 @@ static int fsl_pullup(struct usb_gadget *gadget, int is_on)
 
 	udc = container_of(gadget, struct fsl_udc, gadget);
 	udc->softconnect = (is_on != 0);
-	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);
+	set_pullup(udc);
 
 	return 0;
 }
@@ -2286,6 +2288,8 @@ static int __init struct_udc_setup(struct fsl_udc *udc,
 	udc->usb_state = USB_STATE_POWERED;
 	udc->ep0_dir = 0;
 	udc->remote_wakeup = 0;	/* default to 0 on reset */
+	udc->vbus_active = 1;
+	udc->softconnect = 1;
 
 	return 0;
 }
-- 
1.8.4.1

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

[Felipe Balbi]

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

Hi,

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

-- 
balbi

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

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

[suresh.gupta]

Hi,

-----Original Message-----
From: Felipe Balbi [mailto:balbi@ti.com] 
Sent: Thursday, March 13, 2014 8:55 PM
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,

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 can_pullup function return false when these values was not set 
and intern the code return without enabling the pullup and gadget controller stops. 

--
Thanks
SuresH

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

[Felipe Balbi]

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

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.

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.

cheers

-- 
balbi

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

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

[suresh.gupta]

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

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

[Felipe Balbi]

[-- 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 --]