Re: [PATCH RESEND] usb: phy: mxs: Suggest passing "usbcore.autosuspend=-1" for mx23/mx28

[Peter Chen]

On Thu, Feb 25, 2016 at 02:08:44PM -0300, Fabio Estevam wrote:
> On Wed, Feb 24, 2016 at 10:20 PM, Peter Chen  wrote:
> 
> > Would you get any chances to test i.mx28 evk with an external HUB?
> > I just would like to know it is not a HUB issue which can't be
> > suspended.
> 
> Just connected an external USB hub on a mx28evk and did not see any issue.

So, it may be USB HUB's problem, it is so strange.

Would you please have a test just toggle portsc.suspendM at issued
board? Surely, you need to add bootargs to let it work beforehand.

The address for portsc is $USB_BASE + 0x184, suspendM is bit 7.

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND] usb: phy: mxs: Suggest passing "usbcore.autosuspend=-1" for mx23/mx28

[Fabio Estevam]

On Wed, Feb 24, 2016 at 10:20 PM, Peter Chen  wrote:

> Would you get any chances to test i.mx28 evk with an external HUB?
> I just would like to know it is not a HUB issue which can't be
> suspended.

Just connected an external USB hub on a mx28evk and did not see any issue.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND] usb: phy: mxs: Suggest passing "usbcore.autosuspend=-1" for mx23/mx28

[Peter Chen]

On Wed, Feb 24, 2016 at 11:23:01AM -0300, Fabio Estevam wrote:
> On Wed, Feb 24, 2016 at 10:29 AM, Felipe Balbi  wrote:
> 
> >> [2.814449] usb 1-1: new high-speed USB device number 2 using ci_hdrc
> >> [2.857129] fec 800f.ethernet eth0: Freescale FEC PHY driver
> >> [SMSC LAN8710/LAN8720] (mii_bus:phy_addr=800f.etherne:00, irq)
> >> [2.993879] hub 1-1:1.0: USB hub found
> >> [2.998414] hub 1-1:1.0: 2 ports detected
> >> [3.525108] usb 1-1: USB disconnect, device number 2
> >
> > Why is the device disconnecting ? This is quite odd. Returning -EBUSY on
> 
> Not sure either.
> 
> > your runtime suspend should be the same thing as disabling runtime pm.
> >
> > Wonder what it is that we're missing.
> 
> Peter, any ideas?

Would you get any chances to test i.mx28 evk with an external HUB?
I just would like to know it is not a HUB issue which can't be
suspended.

Currently, with chipidea/mxs phy runtime suspend disable for imx23/28
will keep the PHY is active, and related clock is on; And USB core
is runtime suspend enabled by default, so it will suspend bus, in theory,
it should not cause any problems.

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND] usb: phy: mxs: Suggest passing "usbcore.autosuspend=-1" for mx23/mx28

[Fabio Estevam]

On Wed, Feb 24, 2016 at 10:29 AM, Felipe Balbi  wrote:

>> [2.814449] usb 1-1: new high-speed USB device number 2 using ci_hdrc
>> [2.857129] fec 800f.ethernet eth0: Freescale FEC PHY driver
>> [SMSC LAN8710/LAN8720] (mii_bus:phy_addr=800f.etherne:00, irq)
>> [2.993879] hub 1-1:1.0: USB hub found
>> [2.998414] hub 1-1:1.0: 2 ports detected
>> [3.525108] usb 1-1: USB disconnect, device number 2
>
> Why is the device disconnecting ? This is quite odd. Returning -EBUSY on

Not sure either.

> your runtime suspend should be the same thing as disabling runtime pm.
>
> Wonder what it is that we're missing.

Peter, any ideas?
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND] usb: phy: mxs: Suggest passing "usbcore.autosuspend=-1" for mx23/mx28

[Felipe Balbi]

Hi,

Fabio Estevam  writes:
> On Wed, Feb 24, 2016 at 8:48 AM, Fabio Estevam  wrote:
>
>> and this is the result:
>
> I missed to post the first print. Here is the complete log:
>
> [2.375588] usbcore: registered new interface driver usb-storage
> [2.405944] ** Calling ci_hdrc_imx_runtime_suspend
> [2.412608] 8009.usb supply vbus not found, using dummy regulator
> [2.424703] ci_hdrc ci_hdrc.1: EHCI Host Controller
> [2.430689] ci_hdrc ci_hdrc.1: new USB bus registered, assigned bus number 
> 1
> [2.454341] ci_hdrc ci_hdrc.1: USB 2.0 started, EHCI 1.00
> [2.475947] hub 1-0:1.0: USB hub found
> [2.480321] hub 1-0:1.0: 1 port detected
> [2.499691] mousedev: PS/2 mouse device common for all mice
>
> .
>
> [2.814449] usb 1-1: new high-speed USB device number 2 using ci_hdrc
> [2.857129] fec 800f.ethernet eth0: Freescale FEC PHY driver
> [SMSC LAN8710/LAN8720] (mii_bus:phy_addr=800f.etherne:00, irq)
> [2.993879] hub 1-1:1.0: USB hub found
> [2.998414] hub 1-1:1.0: 2 ports detected
> [3.525108] usb 1-1: USB disconnect, device number 2

Why is the device disconnecting ? This is quite odd. Returning -EBUSY on
your runtime suspend should be the same thing as disabling runtime pm.

Wonder what it is that we're missing.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH RESEND] usb: phy: mxs: Suggest passing "usbcore.autosuspend=-1" for mx23/mx28

[Fabio Estevam]

On Wed, Feb 24, 2016 at 8:48 AM, Fabio Estevam  wrote:

> and this is the result:

I missed to post the first print. Here is the complete log:

[2.375588] usbcore: registered new interface driver usb-storage
[2.405944] ** Calling ci_hdrc_imx_runtime_suspend
[2.412608] 8009.usb supply vbus not found, using dummy regulator
[2.424703] ci_hdrc ci_hdrc.1: EHCI Host Controller
[2.430689] ci_hdrc ci_hdrc.1: new USB bus registered, assigned bus number 1
[2.454341] ci_hdrc ci_hdrc.1: USB 2.0 started, EHCI 1.00
[2.475947] hub 1-0:1.0: USB hub found
[2.480321] hub 1-0:1.0: 1 port detected
[2.499691] mousedev: PS/2 mouse device common for all mice

.

[2.814449] usb 1-1: new high-speed USB device number 2 using ci_hdrc
[2.857129] fec 800f.ethernet eth0: Freescale FEC PHY driver
[SMSC LAN8710/LAN8720] (mii_bus:phy_addr=800f.etherne:00, irq)
[2.993879] hub 1-1:1.0: USB hub found
[2.998414] hub 1-1:1.0: 2 ports detected
[3.525108] usb 1-1: USB disconnect, device number 2
[3.571144] usb usb1-port1: cannot reset (err = -32)
[3.576562] usb usb1-port1: cannot reset (err = -32)
[3.581823] usb usb1-port1: cannot reset (err = -32)
[3.587321] usb usb1-port1: cannot reset (err = -32)
[3.592577] usb usb1-port1: cannot reset (err = -32)
[3.597701] usb usb1-port1: Cannot enable. Maybe the USB cable is bad?
[3.605924] usb usb1-port1: cannot reset (err = -32)
[3.611184] usb usb1-port1: cannot reset (err = -32)
[3.616577] usb usb1-port1: cannot reset (err = -32)
[3.621830] usb usb1-port1: cannot reset (err = -32)
[3.627321] usb usb1-port1: cannot reset (err = -32)
[3.632332] usb usb1-port1: Cannot enable. Maybe the USB cable is bad?
[3.639697] usb usb1-port1: cannot reset (err = -32)
[3.645176] usb usb1-port1: cannot reset (err = -32)
[3.650430] usb usb1-port1: cannot reset (err = -32)
[3.655813] usb usb1-port1: cannot reset (err = -32)
[3.661064] usb usb1-port1: cannot reset (err = -32)
[3.666213] usb usb1-port1: Cannot enable. Maybe the USB cable is bad?
[3.673547] usb usb1-port1: cannot reset (err = -32)
[3.678929] usb usb1-port1: cannot reset (err = -32)
[3.684440] usb usb1-port1: cannot reset (err = -32)
[3.689699] usb usb1-port1: cannot reset (err = -32)
[3.695077] usb usb1-port1: cannot reset (err = -32)
[3.700087] usb usb1-port1: Cannot enable. Maybe the USB cable is bad?
[3.707183] usb usb1-port1: unable to enumerate USB device
[5.195598] ** Calling ci_hdrc_imx_runtime_suspend
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND] usb: phy: mxs: Suggest passing "usbcore.autosuspend=-1" for mx23/mx28

[Fabio Estevam]

On Wed, Feb 24, 2016 at 8:32 AM, Felipe Balbi  wrote:

> Then that's the problem. You should _always_ implement your runtime_pm
> callbacks otherwise driver model will assume you don't need to do
> ANYTHING for runtime pm and runtime suspend you unconditionally.
>
> As a test, try setting the flag but just returning -EBUSY from the top
> of runtime_suspend in chipidea.

Here are the changes as suggested:

--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -33,7 +33,8 @@ static const struct ci_hdrc_imx_platform_flag imx27_usb_data =

 static const struct ci_hdrc_imx_platform_flag imx28_usb_data = {
.flags = CI_HDRC_IMX28_WRITE_FIX |
-   CI_HDRC_TURN_VBUS_EARLY_ON,
+   CI_HDRC_TURN_VBUS_EARLY_ON |
+   CI_HDRC_SUPPORTS_RUNTIME_PM,
 };

 static const struct ci_hdrc_imx_platform_flag imx6q_usb_data = {
@@ -317,6 +318,10 @@ static int ci_hdrc_imx_runtime_suspend(struct device *dev)
struct ci_hdrc_imx_data *data = dev_get_drvdata(dev);
int ret;

+   pr_err("** Calling ci_hdrc_imx_runtime_suspend\n");
+   /* Quick test */
+   return -EBUSY;
+
if (data->in_lpm) {
WARN_ON(1);
return 0;

and this is the result:

[3.104883] hub 2-1:1.0: USB hub found
[3.109226] hub 2-1:1.0: 2 ports detected
[3.635138] usb 2-1: USB disconnect, device number 2
[3.656231] usb usb2-port1: cannot reset (err = -32)
[3.661500] usb usb2-port1: cannot reset (err = -32)
[3.667074] usb usb2-port1: cannot reset (err = -32)
[3.672329] usb usb2-port1: cannot reset (err = -32)
[3.677712] usb usb2-port1: cannot reset (err = -32)
[3.682722] usb usb2-port1: Cannot enable. Maybe the USB cable is bad?
[3.690996] usb usb2-port1: cannot reset (err = -32)
[3.696495] usb usb2-port1: cannot reset (err = -32)
[3.701745] usb usb2-port1: cannot reset (err = -32)
[3.707242] usb usb2-port1: cannot reset (err = -32)
[3.712495] usb usb2-port1: cannot reset (err = -32)
[3.717618] usb usb2-port1: Cannot enable. Maybe the USB cable is bad?
[3.725122] usb usb2-port1: cannot reset (err = -32)
[3.730380] usb usb2-port1: cannot reset (err = -32)
[3.735762] usb usb2-port1: cannot reset (err = -32)
[3.741015] usb usb2-port1: cannot reset (err = -32)
[3.746501] usb usb2-port1: cannot reset (err = -32)
[3.751510] usb usb2-port1: Cannot enable. Maybe the USB cable is bad?
[3.758981] usb usb2-port1: cannot reset (err = -32)
[3.764468] usb usb2-port1: cannot reset (err = -32)
[3.769730] usb usb2-port1: cannot reset (err = -32)
[3.775100] usb usb2-port1: cannot reset (err = -32)
[3.780351] usb usb2-port1: cannot reset (err = -32)
[3.785509] usb usb2-port1: Cannot enable. Maybe the USB cable is bad?
[3.792544] usb usb2-port1: unable to enumerate USB device
[5.195776] ** Calling ci_hdrc_imx_runtime_suspend
[6.057095] fec 800f.ethernet eth0: Link is Up - 100Mbps/Full - flow conx
[6.084367] Sending DHCP requests ., OK

.

[7.156524] uart-pl011 80074000.serial: no DMA platform data
[7.228624] VFS: Mounted root (nfs filesystem) readonly on device 0:14.
[7.240462] devtmpfs: mounted
[7.245817] Freeing unused kernel memory: 248K (c070a000 - c0748000)
[7.586576] ci_hdrc ci_hdrc.0: timeout waiting for 0800 in 11
[7.593451] ** Calling ci_hdrc_imx_runtime_suspend
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND] usb: phy: mxs: Suggest passing "usbcore.autosuspend=-1" for mx23/mx28

[Felipe Balbi]

Hi,

Fabio Estevam  writes:
> Hi Felipe,
>
> On Wed, Feb 24, 2016 at 7:47 AM, Felipe Balbi  wrote:
>
>> Then what DOES get called ? If we don't reach runtime_pm of the PHY,
>> we must reach runtime_pm of chipidea. In that case, detect _there_ if
>> you're running on one of the known broken ones and return -EBUSY or
>> something like that.
>
> We don't reach runtime_pm in chipidea either.
>
> The reason is that mx23/mx28 do not turn on the
> CI_HDRC_SUPPORTS_RUNTIME_PM flag in
> drivers/usb/chipidea/ci_hdrc_imx.c.

Then that's the problem. You should _always_ implement your runtime_pm
callbacks otherwise driver model will assume you don't need to do
ANYTHING for runtime pm and runtime suspend you unconditionally.

As a test, try setting the flag but just returning -EBUSY from the top
of runtime_suspend in chipidea.

> Passing "usbcore.autosuspend=-1" make things to work fine.

of course, this disables runtime_pm altogether. We can do the same thing
without needing that flag at all.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH RESEND] usb: phy: mxs: Suggest passing "usbcore.autosuspend=-1" for mx23/mx28

[Fabio Estevam]

Hi Felipe,

On Wed, Feb 24, 2016 at 7:47 AM, Felipe Balbi  wrote:

> Then what DOES get called ? If we don't reach runtime_pm of the PHY, we
> must reach runtime_pm of chipidea. In that case, detect _there_ if
> you're running on one of the known broken ones and return -EBUSY or
> something like that.

We don't reach runtime_pm in chipidea either.

The reason is that mx23/mx28 do not turn on the
CI_HDRC_SUPPORTS_RUNTIME_PM flag in
drivers/usb/chipidea/ci_hdrc_imx.c.

The errors are:

hub 1-1:1.0: USB hub found
hub 1-1:1.0: 2 ports detected
usb 1-1: USB disconnect, device number 2
usb usb1-port1: cannot reset (err = -32)
usb usb1-port1: cannot reset (err = -32)
usb usb1-port1: cannot reset (err = -32)
usb usb1-port1: cannot reset (err = -32)
usb usb1-port1: cannot reset (err = -32)
usb usb1-port1: Cannot enable. Maybe the USB cable is bad?

which come from drivers/usb/core/hub.c.

Passing "usbcore.autosuspend=-1" make things to work fine.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND] usb: phy: mxs: Suggest passing "usbcore.autosuspend=-1" for mx23/mx28

[Felipe Balbi]

Hi,

Fabio Estevam  writes:
> On Tue, Feb 23, 2016 at 11:44 PM, Peter Chen  wrote:
>
>> Just plug in an external USB HUB at i.mx28 evk host port without pass
>> ''usbcore.autosuspend=-1' at bootargs to see if it works.
>
> Well, the board I have is based on mx28 and has a USB hub onboard. It
> does not work without ''usbcore.autosuspend=-1".

Then what DOES get called ? If we don't reach runtime_pm of the PHY, we
must reach runtime_pm of chipidea. In that case, detect _there_ if
you're running on one of the known broken ones and return -EBUSY or
something like that.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH RESEND] usb: phy: mxs: Suggest passing "usbcore.autosuspend=-1" for mx23/mx28

[Fabio Estevam]

On Tue, Feb 23, 2016 at 11:44 PM, Peter Chen  wrote:

> Just plug in an external USB HUB at i.mx28 evk host port without pass
> ''usbcore.autosuspend=-1' at bootargs to see if it works.

Well, the board I have is based on mx28 and has a USB hub onboard. It
does not work without ''usbcore.autosuspend=-1".
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH RESEND] usb: phy: mxs: Suggest passing "usbcore.autosuspend=-1" for mx23/mx28

[Peter Chen]

 
> On Tue, Feb 23, 2016 at 10:20 PM, Peter Chen 
> wrote:
> 
> > Oh, you said it is called before.
> >
> > http://www.spinics.net/lists/linux-usb/msg136714.html
> 
> Yes, I have enabled runtime pm previously. Sorry for the confusion.
> 
> > If you make sure the neither .runtime_suspend at chipidea driver nor
> > mxs_phy_suspend at mxs driver are called, it means the USB HUB can
> > never be suspended, then the kernel command line is the only way.
> 
> Yes, passing 'usbcore.autosuspend=-1' is the only reliable method I see so 
> far.
> 
> > Care to test external USB HUB with i.mx28 evk? I would like to know if
> > it is the HUB's issue. Afaik, i.mx28 has no such bug when the bus is
> > suspended but the PHY is still active.
> 
> How do I test this?

Just plug in an external USB HUB at i.mx28 evk host port without pass 
''usbcore.autosuspend=-1' at bootargs to see if it works.

Peter
N�r��yb�X��ǧv�^�)޺{.n�+{��^n�r���z���h�&���G���h�(�階�ݢj"���m��z�ޖ���f���h���~�m�

Re: [PATCH RESEND] usb: phy: mxs: Suggest passing "usbcore.autosuspend=-1" for mx23/mx28

[Fabio Estevam]

On Tue, Feb 23, 2016 at 10:20 PM, Peter Chen  wrote:

> Oh, you said it is called before.
>
> http://www.spinics.net/lists/linux-usb/msg136714.html

Yes, I have enabled runtime pm previously. Sorry for the confusion.

> If you make sure the neither .runtime_suspend at chipidea driver
> nor mxs_phy_suspend at mxs driver are called, it means the USB HUB
> can never be suspended, then the kernel command line is the only way.

Yes, passing 'usbcore.autosuspend=-1' is the only reliable method I see so far.

> Care to test external USB HUB with i.mx28 evk? I would like to know if it
> is the HUB's issue. Afaik, i.mx28 has no such bug when the bus is
> suspended but the PHY is still active.

How do I test this?
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND] usb: phy: mxs: Suggest passing "usbcore.autosuspend=-1" for mx23/mx28

[Peter Chen]

On Tue, Feb 23, 2016 at 05:06:12PM -0300, Fabio Estevam wrote:
> On Tue, Feb 23, 2016 at 4:47 PM, Fabio Estevam  wrote:
> > Hi Peter,
> >
> > On Sun, Feb 21, 2016 at 10:59 PM, Peter Chen  wrote:
> >
> >> Fabio, Felipe is correct. The mx23 and mx28 should NOT call mxs phy's
> >> suspend API
> >> since the runtime suspend is not enabled for mx23 and mx28 at chipidea 
> >> driver.
> >> Would you please check if CI_HDRC_SUPPORTS_RUNTIME_PM is set wrongly
> >> for mx23/mx28 at ci_hdrc_imx.c? If it does not been set, would please add
> >> WARN_ON(1) at mxs_phy_suspend to show call stack?
> >
> > CI_HDRC_SUPPORTS_RUNTIME_PM is not set for mx23/mx28. I may have
> > enabled in earlier tests, but now I confirm it is not being set.
> >
> > I am running 4.1.13 with only this patch applied:
> >
> > --- a/drivers/usb/phy/phy-mxs-usb.c
> > +++ b/drivers/usb/phy/phy-mxs-usb.c
> > @@ -368,6 +368,9 @@ static int mxs_phy_suspend(struct usb_phy *x, int 
> > suspend)
> > low_speed_connection = mxs_phy_is_low_speed_connection(mxs_phy);
> > vbus_is_on = mxs_phy_get_vbus_status(mxs_phy);
> >
> > +   pr_err("** entering mxs_phy_suspend\n");
> > +   WARN_ON(1);
> > +
> > if (suspend) {
> > /*
> >  * FIXME: Do not power down RXPWD1PT1 bit for low speed
> >
> > .but I never see mxs_phy_suspend() getting called.
> 

Oh, you said it is called before.

http://www.spinics.net/lists/linux-usb/msg136714.html

> Actually the runtime suspend funcion is mxs_phy_system_suspend().
> 
> Also added a  WARN_ON(1) inside mxs_phy_system_suspend() and it is
> also never called.
> 

Yes, it is expected since the runtime pm is not enabled for PHY mxs
driver.

> This means runtime pm functions are not being called for mx28 as expected.
> 
> Is there a way for getting USB to work on this board with a USB hub
> without passing "usbcore.autosuspend=-1" in the kernel command line?

If you make sure the neither .runtime_suspend at chipidea driver
nor mxs_phy_suspend at mxs driver are called, it means the USB HUB
can never be suspended, then the kernel command line is the only way.

Care to test external USB HUB with i.mx28 evk? I would like to know if it
is the HUB's issue. Afaik, i.mx28 has no such bug when the bus is
suspended but the PHY is still active.

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND] usb: phy: mxs: Suggest passing "usbcore.autosuspend=-1" for mx23/mx28

[Fabio Estevam]

On Tue, Feb 23, 2016 at 4:47 PM, Fabio Estevam  wrote:
> Hi Peter,
>
> On Sun, Feb 21, 2016 at 10:59 PM, Peter Chen  wrote:
>
>> Fabio, Felipe is correct. The mx23 and mx28 should NOT call mxs phy's
>> suspend API
>> since the runtime suspend is not enabled for mx23 and mx28 at chipidea 
>> driver.
>> Would you please check if CI_HDRC_SUPPORTS_RUNTIME_PM is set wrongly
>> for mx23/mx28 at ci_hdrc_imx.c? If it does not been set, would please add
>> WARN_ON(1) at mxs_phy_suspend to show call stack?
>
> CI_HDRC_SUPPORTS_RUNTIME_PM is not set for mx23/mx28. I may have
> enabled in earlier tests, but now I confirm it is not being set.
>
> I am running 4.1.13 with only this patch applied:
>
> --- a/drivers/usb/phy/phy-mxs-usb.c
> +++ b/drivers/usb/phy/phy-mxs-usb.c
> @@ -368,6 +368,9 @@ static int mxs_phy_suspend(struct usb_phy *x, int suspend)
> low_speed_connection = mxs_phy_is_low_speed_connection(mxs_phy);
> vbus_is_on = mxs_phy_get_vbus_status(mxs_phy);
>
> +   pr_err("** entering mxs_phy_suspend\n");
> +   WARN_ON(1);
> +
> if (suspend) {
> /*
>  * FIXME: Do not power down RXPWD1PT1 bit for low speed
>
> .but I never see mxs_phy_suspend() getting called.

Actually the runtime suspend funcion is mxs_phy_system_suspend().

Also added a  WARN_ON(1) inside mxs_phy_system_suspend() and it is
also never called.

This means runtime pm functions are not being called for mx28 as expected.

Is there a way for getting USB to work on this board with a USB hub
without passing "usbcore.autosuspend=-1" in the kernel command line?
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND] usb: phy: mxs: Suggest passing "usbcore.autosuspend=-1" for mx23/mx28

[Fabio Estevam]

Hi Peter,

On Sun, Feb 21, 2016 at 10:59 PM, Peter Chen  wrote:

> Fabio, Felipe is correct. The mx23 and mx28 should NOT call mxs phy's
> suspend API
> since the runtime suspend is not enabled for mx23 and mx28 at chipidea driver.
> Would you please check if CI_HDRC_SUPPORTS_RUNTIME_PM is set wrongly
> for mx23/mx28 at ci_hdrc_imx.c? If it does not been set, would please add
> WARN_ON(1) at mxs_phy_suspend to show call stack?

CI_HDRC_SUPPORTS_RUNTIME_PM is not set for mx23/mx28. I may have
enabled in earlier tests, but now I confirm it is not being set.

I am running 4.1.13 with only this patch applied:

--- a/drivers/usb/phy/phy-mxs-usb.c
+++ b/drivers/usb/phy/phy-mxs-usb.c
@@ -368,6 +368,9 @@ static int mxs_phy_suspend(struct usb_phy *x, int suspend)
low_speed_connection = mxs_phy_is_low_speed_connection(mxs_phy);
vbus_is_on = mxs_phy_get_vbus_status(mxs_phy);

+   pr_err("** entering mxs_phy_suspend\n");
+   WARN_ON(1);
+
if (suspend) {
/*
 * FIXME: Do not power down RXPWD1PT1 bit for low speed

.but I never see mxs_phy_suspend() getting called.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND] usb: phy: mxs: Suggest passing "usbcore.autosuspend=-1" for mx23/mx28

[Peter Chen]

On Fri, Feb 19, 2016 at 11:05 PM, Fabio Estevam  wrote:
> On Fri, Feb 19, 2016 at 12:13 PM, Felipe Balbi  wrote:
>
>> h, okay. So you have another problem, actually. Seems like ci_hdrc
>> just shouldn't call your phy->suspend(), or your phy->suspend()
>> shouldn't do anything. How about below?
>>
>> @@ -370,6 +370,9 @@ static int mxs_phy_suspend(struct usb_phy *x, int 
>> suspend)
>> struct mxs_phy *mxs_phy = to_mxs_phy(x);
>> bool low_speed_connection, vbus_is_on;
>>
>> +   if (mxs_phy->data->flags & MXS_PHY_ABNORMAL_IN_SUSPEND)
>> +   return 0; /* or should we return -EBUSY ? */
>> +
>
> Tested with 0 and also -EBUSY. This code is called now, but it does
> not fix the problem. Same log:
>
> [3.005881] hub 1-1:1.0: USB hub found
> [3.010341] hub 1-1:1.0: 2 ports detected
> [3.536362] usb 1-1: USB disconnect, device number 2
> [3.582732] usb usb1-port1: cannot reset (err = -32)
> [3.588289] usb usb1-port1: cannot reset (err = -32)
> [3.593546] usb usb1-port1: cannot reset (err = -32)
> [3.598929] usb usb1-port1: cannot reset (err = -32)
> [3.604188] usb usb1-port1: cannot reset (err = -32)
> [3.609339] usb usb1-port1: Cannot enable. Maybe the USB cable is bad?
> --

Fabio, Felipe is correct. The mx23 and mx28 should NOT call mxs phy's
suspend API
since the runtime suspend is not enabled for mx23 and mx28 at chipidea driver.
Would you please check if CI_HDRC_SUPPORTS_RUNTIME_PM is set wrongly
for mx23/mx28 at ci_hdrc_imx.c? If it does not been set, would please add
WARN_ON(1) at mxs_phy_suspend to show call stack?


-- 
BR,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND] usb: phy: mxs: Suggest passing "usbcore.autosuspend=-1" for mx23/mx28

[Fabio Estevam]

On Fri, Feb 19, 2016 at 12:13 PM, Felipe Balbi  wrote:

> h, okay. So you have another problem, actually. Seems like ci_hdrc
> just shouldn't call your phy->suspend(), or your phy->suspend()
> shouldn't do anything. How about below?
>
> @@ -370,6 +370,9 @@ static int mxs_phy_suspend(struct usb_phy *x, int suspend)
> struct mxs_phy *mxs_phy = to_mxs_phy(x);
> bool low_speed_connection, vbus_is_on;
>
> +   if (mxs_phy->data->flags & MXS_PHY_ABNORMAL_IN_SUSPEND)
> +   return 0; /* or should we return -EBUSY ? */
> +

Tested with 0 and also -EBUSY. This code is called now, but it does
not fix the problem. Same log:

[3.005881] hub 1-1:1.0: USB hub found
[3.010341] hub 1-1:1.0: 2 ports detected
[3.536362] usb 1-1: USB disconnect, device number 2
[3.582732] usb usb1-port1: cannot reset (err = -32)
[3.588289] usb usb1-port1: cannot reset (err = -32)
[3.593546] usb usb1-port1: cannot reset (err = -32)
[3.598929] usb usb1-port1: cannot reset (err = -32)
[3.604188] usb usb1-port1: cannot reset (err = -32)
[3.609339] usb usb1-port1: Cannot enable. Maybe the USB cable is bad?
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND] usb: phy: mxs: Suggest passing "usbcore.autosuspend=-1" for mx23/mx28

[Felipe Balbi]

Hi,

Fabio Estevam  writes:
> On Fri, Feb 19, 2016 at 11:33 AM, Felipe Balbi  wrote:
>
>> alright, in which sense doesn't it help ? Which platform did you use ?
>
> USB devices are not recognized with your patch applied on a mx28 board
> with an on-board USB hub and if I remove 'usbcore.autosuspend=-1' from
> the kernel command line.
>
>> mx23 or mx28 ? Did you check that mxs_phy_runtime_idle() got called ?
>
> mxs_phy_runtime_idle() is not being called on my mx28 board.

h, okay. So you have another problem, actually. Seems like ci_hdrc
just shouldn't call your phy->suspend(), or your phy->suspend()
shouldn't do anything. How about below?

@@ -370,6 +370,9 @@ static int mxs_phy_suspend(struct usb_phy *x, int suspend)
struct mxs_phy *mxs_phy = to_mxs_phy(x);
bool low_speed_connection, vbus_is_on;
 
+   if (mxs_phy->data->flags & MXS_PHY_ABNORMAL_IN_SUSPEND)
+   return 0; /* or should we return -EBUSY ? */
+
low_speed_connection = mxs_phy_is_low_speed_connection(mxs_phy);
vbus_is_on = mxs_phy_get_vbus_status(mxs_phy);
 


-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH RESEND] usb: phy: mxs: Suggest passing "usbcore.autosuspend=-1" for mx23/mx28

[Fabio Estevam]

On Fri, Feb 19, 2016 at 11:33 AM, Felipe Balbi  wrote:

> alright, in which sense doesn't it help ? Which platform did you use ?

USB devices are not recognized with your patch applied on a mx28 board
with an on-board USB hub and if I remove 'usbcore.autosuspend=-1' from
the kernel command line.

> mx23 or mx28 ? Did you check that mxs_phy_runtime_idle() got called ?

mxs_phy_runtime_idle() is not being called on my mx28 board.

> Did you have any splats on dmesg ?

[2.788614] usb 1-1: new high-speed USB device number 2 using ci_hdrc
[2.837487] fec 800f.ethernet eth0: Freescale FEC PHY driver
[SMSC LAN8710/LAN8720] (mii_bus:phy_addr=800f.etherne:00, irq=
-1)
[2.968109] hub 1-1:1.0: USB hub found
[2.972876] hub 1-1:1.0: 2 ports detected
[3.499166] usb 1-1: USB disconnect, device number 2
[3.544724] usb usb1-port1: cannot reset (err = -32)
[3.550257] usb usb1-port1: cannot reset (err = -32)
[3.16] usb usb1-port1: cannot reset (err = -32)
[3.560903] usb usb1-port1: cannot reset (err = -32)
[3.566159] usb usb1-port1: cannot reset (err = -32)
[3.571306] usb usb1-port1: Cannot enable. Maybe the USB cable is bad?
[3.579584] usb usb1-port1: cannot reset (err = -32)
[3.584850] usb usb1-port1: cannot reset (err = -32)
[3.590368] usb usb1-port1: cannot reset (err = -32)
[3.595628] usb usb1-port1: cannot reset (err = -32)
[3.601014] usb usb1-port1: cannot reset (err = -32)
[3.606027] usb usb1-port1: Cannot enable. Maybe the USB cable is bad?
[3.613481] usb usb1-port1: cannot reset (err = -32)
[3.618868] usb usb1-port1: cannot reset (err = -32)
[3.624125] usb usb1-port1: cannot reset (err = -32)
[3.629615] usb usb1-port1: cannot reset (err = -32)
[3.634867] usb usb1-port1: cannot reset (err = -32)
[3.639988] usb usb1-port1: Cannot enable. Maybe the USB cable is bad?
[3.647249] usb usb1-port1: cannot reset (err = -32)
[3.652744] usb usb1-port1: cannot reset (err = -32)
[3.657999] usb usb1-port1: cannot reset (err = -32)
[3.663369] usb usb1-port1: cannot reset (err = -32)
[3.668856] usb usb1-port1: cannot reset (err = -32)
[3.673866] usb usb1-port1: Cannot enable. Maybe the USB cable is bad?
[3.680862] usb usb1-port1: unable to enumerate USB device
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND] usb: phy: mxs: Suggest passing "usbcore.autosuspend=-1" for mx23/mx28

[Felipe Balbi]

Hi,

Fabio Estevam  writes:
> Hi Felipe,
>
> On Fri, Feb 19, 2016 at 10:05 AM, Felipe Balbi  wrote:
>
>> how about detecting that you're running on mx23/mx28 and returning
>> -EBUSY on your runtime_idle implementation ? You already have the basics
>> done for it. Care to test below and tell me whether it helps ?
>
> Thanks for the suggestion. I tested it and unfortunately it does not
> help.

alright, in which sense doesn't it help ? Which platform did you use ?
mx23 or mx28 ? Did you check that mxs_phy_runtime_idle() got called ?
Did you have any splats on dmesg ?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH RESEND] usb: phy: mxs: Suggest passing "usbcore.autosuspend=-1" for mx23/mx28

[Fabio Estevam]

Hi Felipe,

On Fri, Feb 19, 2016 at 10:05 AM, Felipe Balbi  wrote:

> how about detecting that you're running on mx23/mx28 and returning
> -EBUSY on your runtime_idle implementation ? You already have the basics
> done for it. Care to test below and tell me whether it helps ?

Thanks for the suggestion. I tested it and unfortunately it does not help.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND] usb: phy: mxs: Suggest passing "usbcore.autosuspend=-1" for mx23/mx28

[Felipe Balbi]

Hi,

Fabio Estevam  writes:
> From: Fabio Estevam 
>
> On a mx28 board with a USB hub the following error is observed:
>
> hub 1-1:1.0: USB hub found
> hub 1-1:1.0: 2 ports detected
> usb 1-1: USB disconnect, device number 2
> usb usb1-port1: cannot reset (err = -32)
> usb usb1-port1: cannot reset (err = -32)
> usb usb1-port1: cannot reset (err = -32)
> usb usb1-port1: cannot reset (err = -32)
> usb usb1-port1: cannot reset (err = -32)
> usb usb1-port1: Cannot enable. Maybe the USB cable is bad?
>
> ,which is caused by a problem described by the MXS_PHY_ABNORMAL_IN_SUSPEND
> flag.

how about detecting that you're running on mx23/mx28 and returning
-EBUSY on your runtime_idle implementation ? You already have the basics
done for it. Care to test below and tell me whether it helps ?

modified   drivers/usb/phy/phy-mxs-usb.c
@@ -564,8 +564,23 @@ static int mxs_phy_system_resume(struct device *dev)
 }
 #endif /* CONFIG_PM_SLEEP */
 
-static SIMPLE_DEV_PM_OPS(mxs_phy_pm, mxs_phy_system_suspend,
-   mxs_phy_system_resume);
+#ifdef CONFIG_PM
+static int mxs_phy_runtime_idle(struct device *dev)
+{
+   struct mxs_phy *mxs_phy = dev_get_drvdata(dev);
+
+   if (mxs_phy->data->flags & MXS_PHY_ABNORMAL_IN_SUSPEND)
+   return -EBUSY;
+
+   return 0;
+}
+#endif /* CONFIG_PM */
+
+static const struct dev_pm_ops mxs_phy_pm = {
+   SET_SYSTEM_SLEEP_PM_OPS(mxs_phy_system_suspend,
+   mxs_phy_system_resume)
+   SET_RUNTIME_PM_OPS(NULL, NULL, mxs_phy_runtime_idle)
+};
 
 static struct platform_driver mxs_phy_driver = {
.probe = mxs_phy_probe,



-- 
balbi


signature.asc
Description: PGP signature

[PATCH RESEND] usb: phy: mxs: Suggest passing "usbcore.autosuspend=-1" for mx23/mx28

[Fabio Estevam]

From: Fabio Estevam 

On a mx28 board with a USB hub the following error is observed:

hub 1-1:1.0: USB hub found
hub 1-1:1.0: 2 ports detected
usb 1-1: USB disconnect, device number 2
usb usb1-port1: cannot reset (err = -32)
usb usb1-port1: cannot reset (err = -32)
usb usb1-port1: cannot reset (err = -32)
usb usb1-port1: cannot reset (err = -32)
usb usb1-port1: cannot reset (err = -32)
usb usb1-port1: Cannot enable. Maybe the USB cable is bad?

,which is caused by a problem described by the MXS_PHY_ABNORMAL_IN_SUSPEND
flag.

As we currently do not have a proper implementation for this issue, one
possible workaround is to pass "usbcore.autosuspend=-1" in the kernel
command line, so add this suggestion into the MXS_PHY_ABNORMAL_IN_SUSPEND
flag description.

Signed-off-by: Fabio Estevam 
Acked-by: Peter Chen 
---
 drivers/usb/phy/phy-mxs-usb.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/phy/phy-mxs-usb.c b/drivers/usb/phy/phy-mxs-usb.c
index 00bfea0..be46ab3 100644
--- a/drivers/usb/phy/phy-mxs-usb.c
+++ b/drivers/usb/phy/phy-mxs-usb.c
@@ -98,6 +98,10 @@
  * The PHY will be in messy if there is a wakeup after putting
  * bus to suspend (set portsc.suspendM) but before setting PHY to low
  * power mode (set portsc.phcd).
+ *
+ * To work around this problem on mx23/mx28 user should pass
+ * "usbcore.autosuspend=-1" in the kernel command line for now, as
+ * we do not have a proper fix for this flag in the kernel yet.
  */
 #define MXS_PHY_ABNORMAL_IN_SUSPENDBIT(1)
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html