linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] usb: gadget: fsl: check vbus presence on probe
@ 2014-04-24  8:54 Paul Fertser
  2014-04-30 16:06 ` Felipe Balbi
  0 siblings, 1 reply; 11+ messages in thread
From: Paul Fertser @ 2014-04-24  8:54 UTC (permalink / raw)
  To: Li Yang, Felipe Balbi; +Cc: linux-usb, linux-kernel, Paul Fertser

If VBUS is already present during the driver initialisation, the
corresponding IRQ never fires, so there is no way the gadget can get
enumerated.

This patch is real-life tested on an i.MX25 board with VBUS constantly
hooked up.

Signed-off-by: Paul Fertser <fercerpav@gmail.com>
---
 drivers/usb/gadget/fsl_udc_core.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c
index a2f26cd..b4b1516 100644
--- a/drivers/usb/gadget/fsl_udc_core.c
+++ b/drivers/usb/gadget/fsl_udc_core.c
@@ -2501,6 +2501,11 @@ static int __init fsl_udc_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_del_udc;
 
+	/* Now let it settle a bit and sense VBUS */
+	msleep_interruptible(1);
+	if (fsl_readl(&dr_regs->otgsc) & OTGSC_STS_B_SESSION_VALID)
+		udc_controller->vbus_active = 1;
+
 	create_proc_file();
 	return 0;
 
-- 
1.7.10


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] usb: gadget: fsl: check vbus presence on probe
  2014-04-24  8:54 [PATCH] usb: gadget: fsl: check vbus presence on probe Paul Fertser
@ 2014-04-30 16:06 ` Felipe Balbi
  2014-04-30 19:27   ` Paul Fertser
  0 siblings, 1 reply; 11+ messages in thread
From: Felipe Balbi @ 2014-04-30 16:06 UTC (permalink / raw)
  To: Paul Fertser; +Cc: Li Yang, Felipe Balbi, linux-usb, linux-kernel

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

On Thu, Apr 24, 2014 at 12:54:13PM +0400, Paul Fertser wrote:
> If VBUS is already present during the driver initialisation, the

s/initialisation/initialization

> corresponding IRQ never fires, so there is no way the gadget can get
> enumerated.
> 
> This patch is real-life tested on an i.MX25 board with VBUS constantly
> hooked up.
> 
> Signed-off-by: Paul Fertser <fercerpav@gmail.com>
> ---
>  drivers/usb/gadget/fsl_udc_core.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c
> index a2f26cd..b4b1516 100644
> --- a/drivers/usb/gadget/fsl_udc_core.c
> +++ b/drivers/usb/gadget/fsl_udc_core.c
> @@ -2501,6 +2501,11 @@ static int __init fsl_udc_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_del_udc;
>  
> +	/* Now let it settle a bit and sense VBUS */
> +	msleep_interruptible(1);
> +	if (fsl_readl(&dr_regs->otgsc) & OTGSC_STS_B_SESSION_VALID)
> +		udc_controller->vbus_active = 1;

good fix, should this go to stable ?

-- 
balbi

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] usb: gadget: fsl: check vbus presence on probe
  2014-04-30 16:06 ` Felipe Balbi
@ 2014-04-30 19:27   ` Paul Fertser
  2014-04-30 20:12     ` Felipe Balbi
  0 siblings, 1 reply; 11+ messages in thread
From: Paul Fertser @ 2014-04-30 19:27 UTC (permalink / raw)
  To: Felipe Balbi; +Cc: Li Yang, linux-usb, linux-kernel

Hello,

Thank you for the review.

On Wed, Apr 30, 2014 at 11:06:25AM -0500, Felipe Balbi wrote:
> On Thu, Apr 24, 2014 at 12:54:13PM +0400, Paul Fertser wrote:
> > If VBUS is already present during the driver initialisation, the
> 
> s/initialisation/initialization

If I'm understanding [1] properly, both spelling variants are correct.

> > +	/* Now let it settle a bit and sense VBUS */
> > +	msleep_interruptible(1);
> > +	if (fsl_readl(&dr_regs->otgsc) & OTGSC_STS_B_SESSION_VALID)
> > +		udc_controller->vbus_active = 1;
> 
> good fix, should this go to stable ?

I was reluctant to Cc stable because I'm not sure which versions need
to be fixed, as I've seen an earlier version (3.0 something or a bit
before that probably) working without this (no idea why); also I'm not
sure about that msleep as it's deduced solely from experimenting with
a particular i.MX model, I hoped someone from Freescale would comment
on it. I've seen your review [2] on a related patch and my fix should
have been obvious to Suresh (and trivial to test on different boards),
yet he didn't implement it, that worries me a bit.

[1] https://en.wikipedia.org/wiki/American_and_British_English_spelling_differences#-ise.2C_-ize_.28-isation.2C_-ization.29
[2] https://patchwork.kernel.org/patch/3822321/
-- 
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@gmail.com

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] usb: gadget: fsl: check vbus presence on probe
  2014-04-30 19:27   ` Paul Fertser
@ 2014-04-30 20:12     ` Felipe Balbi
  2014-05-01 14:27       ` suresh.gupta
  0 siblings, 1 reply; 11+ messages in thread
From: Felipe Balbi @ 2014-04-30 20:12 UTC (permalink / raw)
  To: Paul Fertser; +Cc: Felipe Balbi, Li Yang, linux-usb, linux-kernel, suresh.gupta

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

+Suresh

(top-posting, yeah yeah :-)

On Wed, Apr 30, 2014 at 11:27:45PM +0400, Paul Fertser wrote:
> Hello,
> 
> Thank you for the review.
> 
> On Wed, Apr 30, 2014 at 11:06:25AM -0500, Felipe Balbi wrote:
> > On Thu, Apr 24, 2014 at 12:54:13PM +0400, Paul Fertser wrote:
> > > If VBUS is already present during the driver initialisation, the
> > 
> > s/initialisation/initialization
> 
> If I'm understanding [1] properly, both spelling variants are correct.
> 
> > > +	/* Now let it settle a bit and sense VBUS */
> > > +	msleep_interruptible(1);
> > > +	if (fsl_readl(&dr_regs->otgsc) & OTGSC_STS_B_SESSION_VALID)
> > > +		udc_controller->vbus_active = 1;
> > 
> > good fix, should this go to stable ?
> 
> I was reluctant to Cc stable because I'm not sure which versions need
> to be fixed, as I've seen an earlier version (3.0 something or a bit
> before that probably) working without this (no idea why); also I'm not
> sure about that msleep as it's deduced solely from experimenting with
> a particular i.MX model, I hoped someone from Freescale would comment
> on it. I've seen your review [2] on a related patch and my fix should
> have been obvious to Suresh (and trivial to test on different boards),
> yet he didn't implement it, that worries me a bit.
> 
> [1] https://en.wikipedia.org/wiki/American_and_British_English_spelling_differences#-ise.2C_-ize_.28-isation.2C_-ization.29
> [2] https://patchwork.kernel.org/patch/3822321/
> -- 
> Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
> mailto:fercerpav@gmail.com

-- 
balbi

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] usb: gadget: fsl: check vbus presence on probe
  2014-04-30 20:12     ` Felipe Balbi
@ 2014-05-01 14:27       ` suresh.gupta
  2014-05-08 14:30         ` suresh.gupta
  0 siblings, 1 reply; 11+ messages in thread
From: suresh.gupta @ 2014-05-01 14:27 UTC (permalink / raw)
  To: balbi, Paul Fertser; +Cc: LeoLi, linux-usb, linux-kernel

Give me some time (actually some days), I will try this and update you.

> -----Original Message-----
> From: Felipe Balbi [mailto:balbi@ti.com]
> Sent: Thursday, May 01, 2014 1:42 AM
> To: Paul Fertser
> Cc: Felipe Balbi; Li Yang-Leo-R58472; linux-usb@vger.kernel.org; linux-
> kernel@vger.kernel.org; Gupta Suresh-B42813
> Subject: Re: [PATCH] usb: gadget: fsl: check vbus presence on probe
> 
> +Suresh
> 
> (top-posting, yeah yeah :-)
> 
> On Wed, Apr 30, 2014 at 11:27:45PM +0400, Paul Fertser wrote:
> > Hello,
> >
> > Thank you for the review.
> >
> > On Wed, Apr 30, 2014 at 11:06:25AM -0500, Felipe Balbi wrote:
> > > On Thu, Apr 24, 2014 at 12:54:13PM +0400, Paul Fertser wrote:
> > > > If VBUS is already present during the driver initialisation, the
> > >
> > > s/initialisation/initialization
> >
> > If I'm understanding [1] properly, both spelling variants are correct.
> >
> > > > +	/* Now let it settle a bit and sense VBUS */
> > > > +	msleep_interruptible(1);
> > > > +	if (fsl_readl(&dr_regs->otgsc) & OTGSC_STS_B_SESSION_VALID)
> > > > +		udc_controller->vbus_active = 1;
> > >
> > > good fix, should this go to stable ?
> >
> > I was reluctant to Cc stable because I'm not sure which versions need
> > to be fixed, as I've seen an earlier version (3.0 something or a bit
> > before that probably) working without this (no idea why); also I'm not
> > sure about that msleep as it's deduced solely from experimenting with
> > a particular i.MX model, I hoped someone from Freescale would comment
> > on it. I've seen your review [2] on a related patch and my fix should
> > have been obvious to Suresh (and trivial to test on different boards),
> > yet he didn't implement it, that worries me a bit.
> >
> > [1]
> > https://en.wikipedia.org/wiki/American_and_British_English_spelling_di
> > fferences#-ise.2C_-ize_.28-isation.2C_-ization.29
> > [2] https://patchwork.kernel.org/patch/3822321/
> > --
> > Be free, use free (http://www.gnu.org/philosophy/free-sw.html)
> software!
> > mailto:fercerpav@gmail.com
> 
> --
> balbi

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] usb: gadget: fsl: check vbus presence on probe
  2014-05-01 14:27       ` suresh.gupta
@ 2014-05-08 14:30         ` suresh.gupta
  2014-05-08 15:38           ` Paul Fertser
  0 siblings, 1 reply; 11+ messages in thread
From: suresh.gupta @ 2014-05-08 14:30 UTC (permalink / raw)
  To: suresh.gupta, balbi, Paul Fertser; +Cc: LeoLi, linux-usb, linux-kernel

Hi,

As per my limited knowledge, the purpose of OTGSC_STS_B_SESSION_VALID bit is to tell either VBUS is above the B session valid threshold and which comes only Host is attached.
And Host might be attach after system bootup or after driver initialization. So putting this check in probe will not help much.

> -----Original Message-----
> From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-
> owner@vger.kernel.org] On Behalf Of suresh.gupta@freescale.com
> Sent: Thursday, May 01, 2014 7:58 PM
> To: balbi@ti.com; Paul Fertser
> Cc: Li Yang-Leo-R58472; linux-usb@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: RE: [PATCH] usb: gadget: fsl: check vbus presence on probe
> 
> Give me some time (actually some days), I will try this and update you.
> 
> > -----Original Message-----
> > From: Felipe Balbi [mailto:balbi@ti.com]
> > Sent: Thursday, May 01, 2014 1:42 AM
> > To: Paul Fertser
> > Cc: Felipe Balbi; Li Yang-Leo-R58472; linux-usb@vger.kernel.org;
> > linux- kernel@vger.kernel.org; Gupta Suresh-B42813
> > Subject: Re: [PATCH] usb: gadget: fsl: check vbus presence on probe
> >
> > +Suresh
> >
> > (top-posting, yeah yeah :-)
> >
> > On Wed, Apr 30, 2014 at 11:27:45PM +0400, Paul Fertser wrote:
> > > Hello,
> > >
> > > Thank you for the review.
> > >
> > > On Wed, Apr 30, 2014 at 11:06:25AM -0500, Felipe Balbi wrote:
> > > > On Thu, Apr 24, 2014 at 12:54:13PM +0400, Paul Fertser wrote:
> > > > > If VBUS is already present during the driver initialisation, the
> > > >
> > > > s/initialisation/initialization
> > >
> > > If I'm understanding [1] properly, both spelling variants are
> correct.
> > >
> > > > > +	/* Now let it settle a bit and sense VBUS */
> > > > > +	msleep_interruptible(1);
> > > > > +	if (fsl_readl(&dr_regs->otgsc) & OTGSC_STS_B_SESSION_VALID)
> > > > > +		udc_controller->vbus_active = 1;
> > > >
> > > > good fix, should this go to stable ?
> > >
> > > I was reluctant to Cc stable because I'm not sure which versions
> > > need to be fixed, as I've seen an earlier version (3.0 something or
> > > a bit before that probably) working without this (no idea why); also
> > > I'm not sure about that msleep as it's deduced solely from
> > > experimenting with a particular i.MX model, I hoped someone from
> > > Freescale would comment on it. I've seen your review [2] on a
> > > related patch and my fix should have been obvious to Suresh (and
> > > trivial to test on different boards), yet he didn't implement it,
> that worries me a bit.
> > >
> > > [1]
> > > https://en.wikipedia.org/wiki/American_and_British_English_spelling_
> > > di
> > > fferences#-ise.2C_-ize_.28-isation.2C_-ization.29
> > > [2] https://patchwork.kernel.org/patch/3822321/
> > > --
> > > Be free, use free (http://www.gnu.org/philosophy/free-sw.html)
> > software!
> > > mailto:fercerpav@gmail.com
> >
> > --
> > balbi
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] usb: gadget: fsl: check vbus presence on probe
  2014-05-08 14:30         ` suresh.gupta
@ 2014-05-08 15:38           ` Paul Fertser
  2014-05-08 18:42             ` suresh.gupta
  0 siblings, 1 reply; 11+ messages in thread
From: Paul Fertser @ 2014-05-08 15:38 UTC (permalink / raw)
  To: suresh.gupta; +Cc: balbi, LeoLi, linux-usb, linux-kernel

Hi,

On Thu, May 08, 2014 at 02:30:39PM +0000, suresh.gupta@freescale.com wrote:
> As per my limited knowledge, the purpose of
> OTGSC_STS_B_SESSION_VALID bit is to tell either VBUS is above the B
> session valid threshold and which comes only Host is attached.

Yes, that matches the datasheet I've read.

> And Host might be attach after system bootup or after driver
> initialization. So putting this check in probe will not help much.

If the host is attached after the driver was initialised, the
interrupt will trigger and the driver will get notified that VBUS
appeared and everything will go smooth, at least that's how it should
work (I do not have any board handy to real-life check that, but
AFAICT that's the intent of the current code).

I actually do not have any iMX demoboards at all, I've only got some
custom-designed i.MX25 boards where I can't control VBUS, it's
permanently pulled up.

But I was fixing the problem that was clearly, 100% reproducibly
happening when VBUS was applied before the interrupt was
configured. So what exactly do you mean here? Do you mean this check
I've added doesn't fix the bug? Or do you mean this bug should be
fixed somehow else? Or do you mean there's no bug in the first place
and my board doesn't work because of something else?

-- 
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@gmail.com

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] usb: gadget: fsl: check vbus presence on probe
  2014-05-08 15:38           ` Paul Fertser
@ 2014-05-08 18:42             ` suresh.gupta
  2014-05-08 20:14               ` Paul Fertser
  0 siblings, 1 reply; 11+ messages in thread
From: suresh.gupta @ 2014-05-08 18:42 UTC (permalink / raw)
  To: Paul Fertser, balbi; +Cc: LeoLi, linux-usb, linux-kernel



> -----Original Message-----
> From: Paul Fertser [mailto:fercerpav@gmail.com]
> Sent: Thursday, May 08, 2014 9:09 PM
> To: Gupta Suresh-B42813
> Cc: balbi@ti.com; Li Yang-Leo-R58472; linux-usb@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH] usb: gadget: fsl: check vbus presence on probe
> 
> Hi,
> 
> On Thu, May 08, 2014 at 02:30:39PM +0000, suresh.gupta@freescale.com
> wrote:
> > As per my limited knowledge, the purpose of OTGSC_STS_B_SESSION_VALID
> > bit is to tell either VBUS is above the B session valid threshold and
> > which comes only Host is attached.
> 
> Yes, that matches the datasheet I've read.
> 
> > And Host might be attach after system bootup or after driver
> > initialization. So putting this check in probe will not help much.
> 
> If the host is attached after the driver was initialised, the interrupt
> will trigger and the driver will get notified that VBUS appeared and
> everything will go smooth, at least that's how it should work (I do not
> have any board handy to real-life check that, but AFAICT that's the
> intent of the current code).

If is go through the code flow starting from usb_composite_probe the sequence is
usb_composite_probe->usb_gadget_probe_driver->udc_bind_to_driver->
udc_bind_to_driver->usb_gadget_udc_start->fsl_udc_start->usbcmd=RUN
udc_bind_to_driver->usb_gadget_connect->fsl_pullup

The function fsl_pullup make usbcmd=STOP if my fix is not there and if controller 
is stopped we do not get any interrupt. 
 
> 
> I actually do not have any iMX demoboards at all, I've only got some
> custom-designed i.MX25 boards where I can't control VBUS, it's
> permanently pulled up.
> 
> But I was fixing the problem that was clearly, 100% reproducibly
> happening when VBUS was applied before the interrupt was configured. So

Wait a minute, are you using OTG. If your VBUS is permanently pulled up, that 
means you are only Gadget(I might miss something) then why you use OTG mode.

> what exactly do you mean here? Do you mean this check I've added doesn't
> fix the bug? Or do you mean this bug should be fixed somehow else? Or do

What expertly my concern is, probe is not proper place to check VBUS valid.
I think we should wait to hear what Balbi has to say. 

> you mean there's no bug in the first place and my board doesn't work
> because of something else?
> 
> --
> Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
> mailto:fercerpav@gmail.com

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] usb: gadget: fsl: check vbus presence on probe
  2014-05-08 18:42             ` suresh.gupta
@ 2014-05-08 20:14               ` Paul Fertser
  2014-05-09  9:07                 ` suresh.gupta
  0 siblings, 1 reply; 11+ messages in thread
From: Paul Fertser @ 2014-05-08 20:14 UTC (permalink / raw)
  To: suresh.gupta; +Cc: balbi, LeoLi, linux-usb, linux-kernel

On Thu, May 08, 2014 at 06:42:53PM +0000, suresh.gupta@freescale.com wrote:
> > > And Host might be attach after system bootup or after driver
> > > initialization. So putting this check in probe will not help much.
> > 
> > If the host is attached after the driver was initialised, the interrupt
> > will trigger and the driver will get notified that VBUS appeared and
> > everything will go smooth, at least that's how it should work (I do not
> > have any board handy to real-life check that, but AFAICT that's the
> > intent of the current code).
> 
> If is go through the code flow starting from usb_composite_probe the sequence is
> usb_composite_probe->usb_gadget_probe_driver->udc_bind_to_driver->
> udc_bind_to_driver->usb_gadget_udc_start->fsl_udc_start->usbcmd=RUN
> udc_bind_to_driver->usb_gadget_connect->fsl_pullup
> 
> The function fsl_pullup make usbcmd=STOP if my fix is not there and if controller 
> is stopped we do not get any interrupt. 

Are you really sure we can't get async VBUS state change notifications
until controller has USB_CMD_RUN_STOP bit set (and the same bit
actually controls internal 1.5k dataline pullup)? If yes, I guess it
means we need to check VBUS state _every_ time we set that bit to sync
the vbus_active variable with the actual hardware state (unless an
external OTG PHY is used and VBUS pad state is irrelevant)?

> > I actually do not have any iMX demoboards at all, I've only got some
> > custom-designed i.MX25 boards where I can't control VBUS, it's
> > permanently pulled up.
> > 
> > But I was fixing the problem that was clearly, 100% reproducibly
> > happening when VBUS was applied before the interrupt was configured. So
> 
> Wait a minute, are you using OTG. If your VBUS is permanently pulled up, that 
> means you are only Gadget(I might miss something) then why you use OTG mode.

I'm using FSL_USB2_DR_DEVICE mode (FSL_USB2_PHY_UTMI phy_mode), so the
OTG controller should always work in the device mode only.

> > what exactly do you mean here? Do you mean this check I've added doesn't
> > fix the bug? Or do you mean this bug should be fixed somehow else? Or do
> 
> What expertly my concern is, probe is not proper place to check VBUS valid.
> I think we should wait to hear what Balbi has to say. 

Yes, I hope he can understand both of us :)

-- 
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@gmail.com

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH] usb: gadget: fsl: check vbus presence on probe
  2014-05-08 20:14               ` Paul Fertser
@ 2014-05-09  9:07                 ` suresh.gupta
  2014-05-09 11:18                   ` Paul Fertser
  0 siblings, 1 reply; 11+ messages in thread
From: suresh.gupta @ 2014-05-09  9:07 UTC (permalink / raw)
  To: Paul Fertser, balbi; +Cc: LeoLi, linux-usb, linux-kernel



> -----Original Message-----
> From: Paul Fertser [mailto:fercerpav@gmail.com]
> Sent: Friday, May 09, 2014 1:44 AM
> To: Gupta Suresh-B42813
> Cc: balbi@ti.com; Li Yang-Leo-R58472; linux-usb@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH] usb: gadget: fsl: check vbus presence on probe
> 
> On Thu, May 08, 2014 at 06:42:53PM +0000, suresh.gupta@freescale.com
> wrote:
> > > > And Host might be attach after system bootup or after driver
> > > > initialization. So putting this check in probe will not help much.
> > >
> > > If the host is attached after the driver was initialised, the
> > > interrupt will trigger and the driver will get notified that VBUS
> > > appeared and everything will go smooth, at least that's how it
> > > should work (I do not have any board handy to real-life check that,
> > > but AFAICT that's the intent of the current code).
> >
> > If is go through the code flow starting from usb_composite_probe the
> > sequence is
> > usb_composite_probe->usb_gadget_probe_driver->udc_bind_to_driver->
> > udc_bind_to_driver->usb_gadget_udc_start->fsl_udc_start->usbcmd=RUN
> > udc_bind_to_driver->usb_gadget_connect->fsl_pullup
> >
> > The function fsl_pullup make usbcmd=STOP if my fix is not there and if
> > controller is stopped we do not get any interrupt.
> 
> Are you really sure we can't get async VBUS state change notifications
> until controller has USB_CMD_RUN_STOP bit set (and the same bit actually
> controls internal 1.5k dataline pullup)? If yes, I guess it means we need
> to check VBUS state _every_ time we set that bit to sync the vbus_active
> variable with the actual hardware state (unless an external OTG PHY is
> used and VBUS pad state is irrelevant)?

There will be no interrupts if USB_CMD_RUN_STOP is not set but status get change.
I doubt, my previous patch (252455c40316009cc791f00338ee2e367d2d2739) make any difference in your device working.
Can you please remove my patch and check either your device work or not.

We are using two different configs(yours OTG and mine gadget only) that why I did not face the issue.
I remember Balbi suggest me to check hardware to initialize vbus_active. But I am not sure, the proper place to set vbus_active that why I did not implement it.

Balbi can help us to decide which is the proper place to set vbus_active :).

Thanks
      
> 
> > > I actually do not have any iMX demoboards at all, I've only got some
> > > custom-designed i.MX25 boards where I can't control VBUS, it's
> > > permanently pulled up.
> > >
> > > But I was fixing the problem that was clearly, 100% reproducibly
> > > happening when VBUS was applied before the interrupt was configured.
> > > So
> >
> > Wait a minute, are you using OTG. If your VBUS is permanently pulled
> > up, that means you are only Gadget(I might miss something) then why you
> use OTG mode.
> 
> I'm using FSL_USB2_DR_DEVICE mode (FSL_USB2_PHY_UTMI phy_mode), so the
> OTG controller should always work in the device mode only.
> 
> > > what exactly do you mean here? Do you mean this check I've added
> > > doesn't fix the bug? Or do you mean this bug should be fixed somehow
> > > else? Or do
> >
> > What expertly my concern is, probe is not proper place to check VBUS
> valid.
> > I think we should wait to hear what Balbi has to say.
> 
> Yes, I hope he can understand both of us :)
> 
> --
> Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
> mailto:fercerpav@gmail.com

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] usb: gadget: fsl: check vbus presence on probe
  2014-05-09  9:07                 ` suresh.gupta
@ 2014-05-09 11:18                   ` Paul Fertser
  0 siblings, 0 replies; 11+ messages in thread
From: Paul Fertser @ 2014-05-09 11:18 UTC (permalink / raw)
  To: suresh.gupta; +Cc: balbi, LeoLi, linux-usb, linux-kernel

Hi,

On Fri, May 09, 2014 at 09:07:00AM +0000, suresh.gupta@freescale.com wrote:
> > Are you really sure we can't get async VBUS state change notifications
> > until controller has USB_CMD_RUN_STOP bit set (and the same bit actually
> > controls internal 1.5k dataline pullup)? If yes, I guess it means we need
> > to check VBUS state _every_ time we set that bit to sync the vbus_active
> > variable with the actual hardware state (unless an external OTG PHY is
> > used and VBUS pad state is irrelevant)?
> 
> There will be no interrupts if USB_CMD_RUN_STOP is not set but
> status get change.  I doubt, my previous patch
> (252455c40316009cc791f00338ee2e367d2d2739) make any difference in
> your device working.  Can you please remove my patch and check
> either your device work or not.

I wasn't using your patch actually as I'm running 3.10 + OpenWrt
patches, not current kernel HEAD.

Will there be no interrupts without CMD_RUN_STOP as well even if an
external PHY/transceiver is used? How do you expect
fsl_vbus_session(..., 1) to ever get called then?

> We are using two different configs(yours OTG and mine gadget only)
> that why I did not face the issue.

How do you tell I'm using OTG config and what does it mean given I'm
using FSL_USB2_DR_DEVICE? And I'm not using any external OTG
transceiver. And if you're using "gadget-only" config (what does it
actually mean?) what and when was actually setting vbus_active=1 for
you?

And BTW, I looked through all the places USB_CMD_RUN_STOP is set and
it looks like a real mess. E.g. why do you call dr_controller_run
unconditionally (without external transceiver) from fsl_udc_start()
and thus set CMD_RUN_STOP in there when afaict the gadget framework
expects you to activate the pull up only after fsl_pullup (via
usb_gadget_connect) is called? What and when is supposed to initialise
interrupts when an external transceiver is used? Was "OTG mode" (which
I'm not using afaict) tested ever at all? How can it work given the
current code? If it's not possible to track VBUS state changes in
"stop" mode why aren't you checking it explicitly every time after
entering "run" mode?

So far the current fsl_udc_core.c code looks like a broken omap_udc.c
copycat to me. Don't you think everything that deals with external PHY
should be removed first, then all the init/deinit/start/stop carefully
reviewed, restructured, reordered according to the gadget framework
requirements, common parts factored out, tested on different hardware,
then external transceiver support reintroduced in a clean and obvious
manner?

Given the history of your patches I see on patchwork, no, you do not
think so. I admit I'm not nearly an expert in Linux UDC/gadget
framework but your responses so far are confusing me to no end.

Balbi, as a stop-gap measure I will try to test and send a v2 patch
that would sense VBUS at the end of dr_controller_run(). I should
also probably send another one marking the driver BROKEN in Kconfig :/

-- 
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@gmail.com

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2014-05-09 11:18 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-04-24  8:54 [PATCH] usb: gadget: fsl: check vbus presence on probe Paul Fertser
2014-04-30 16:06 ` Felipe Balbi
2014-04-30 19:27   ` Paul Fertser
2014-04-30 20:12     ` Felipe Balbi
2014-05-01 14:27       ` suresh.gupta
2014-05-08 14:30         ` suresh.gupta
2014-05-08 15:38           ` Paul Fertser
2014-05-08 18:42             ` suresh.gupta
2014-05-08 20:14               ` Paul Fertser
2014-05-09  9:07                 ` suresh.gupta
2014-05-09 11:18                   ` Paul Fertser

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).