[PATCH] USB: initialize or shutdown PHY when add or remove host controller

[PATCH] USB: initialize or shutdown PHY when add or remove host controller

[Chao Xie]

Some controller need software to initialize PHY before add
host controller, and shut down PHY after remove host controller.
Add the generic code for these controllers so they do not need
do it in its own host controller driver.

Signed-off-by: Chao Xie <chao.xie@marvell.com>
---
 drivers/usb/core/hcd.c |   19 ++++++++++++++++++-
 1 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index d53547d..b26196b 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -43,6 +43,7 @@
 
 #include <linux/usb.h>
 #include <linux/usb/hcd.h>
+#include <linux/usb/phy.h>
 
 #include "usb.h"
 
@@ -2531,12 +2532,22 @@ int usb_add_hcd(struct usb_hcd *hcd,
 	 */
 	set_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
 
+	/* Initialize the PHY before other hardware operation. */
+	if (hcd->phy) {
+		retval = usb_phy_init(hcd->phy);
+		if (retval) {
+			dev_err(hcd->self.controller,
+				"can't initialize phy\n");
+			goto err_hcd_driver_setup;
+		}
+	}
+
 	/* "reset" is misnamed; its role is now one-time init. the controller
 	 * should already have been reset (and boot firmware kicked off etc).
 	 */
 	if (hcd->driver->reset && (retval = hcd->driver->reset(hcd)) < 0) {
 		dev_err(hcd->self.controller, "can't setup\n");
-		goto err_hcd_driver_setup;
+		goto err_hcd_driver_init_phy;
 	}
 	hcd->rh_pollable = 1;
 
@@ -2608,6 +2619,9 @@ err_hcd_driver_start:
 	if (usb_hcd_is_primary_hcd(hcd) && hcd->irq > 0)
 		free_irq(irqnum, hcd);
 err_request_irq:
+err_hcd_driver_init_phy:
+	if (hcd->phy)
+		usb_phy_shutdown(hcd->phy);
 err_hcd_driver_setup:
 err_set_rh_speed:
 	usb_put_dev(hcd->self.root_hub);
@@ -2674,6 +2688,9 @@ void usb_remove_hcd(struct usb_hcd *hcd)
 			free_irq(hcd->irq, hcd);
 	}
 
+	if (hcd->phy)
+		usb_phy_shutdown(hcd->phy);
+
 	usb_put_dev(hcd->self.root_hub);
 	usb_deregister_bus(&hcd->self);
 	hcd_buffer_destroy(hcd);
-- 
1.7.4.1

[PATCH] USB: initialize or shutdown PHY when add or remove host controller

[Chao Xie]

Some controller need software to initialize PHY before add
host controller, and shut down PHY after remove host controller.
Add the generic code for these controllers so they do not need
do it in its own host controller driver.

Signed-off-by: Chao Xie <chao.xie@marvell.com>
---
 drivers/usb/core/hcd.c |   19 ++++++++++++++++++-
 1 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index d53547d..b26196b 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -43,6 +43,7 @@
 
 #include <linux/usb.h>
 #include <linux/usb/hcd.h>
+#include <linux/usb/phy.h>
 
 #include "usb.h"
 
@@ -2531,12 +2532,22 @@ int usb_add_hcd(struct usb_hcd *hcd,
 	 */
 	set_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
 
+	/* Initialize the PHY before other hardware operation. */
+	if (hcd->phy) {
+		retval = usb_phy_init(hcd->phy);
+		if (retval) {
+			dev_err(hcd->self.controller,
+				"can't initialize phy\n");
+			goto err_hcd_driver_setup;
+		}
+	}
+
 	/* "reset" is misnamed; its role is now one-time init. the controller
 	 * should already have been reset (and boot firmware kicked off etc).
 	 */
 	if (hcd->driver->reset && (retval = hcd->driver->reset(hcd)) < 0) {
 		dev_err(hcd->self.controller, "can't setup\n");
-		goto err_hcd_driver_setup;
+		goto err_hcd_driver_init_phy;
 	}
 	hcd->rh_pollable = 1;
 
@@ -2608,6 +2619,9 @@ err_hcd_driver_start:
 	if (usb_hcd_is_primary_hcd(hcd) && hcd->irq > 0)
 		free_irq(irqnum, hcd);
 err_request_irq:
+err_hcd_driver_init_phy:
+	if (hcd->phy)
+		usb_phy_shutdown(hcd->phy);
 err_hcd_driver_setup:
 err_set_rh_speed:
 	usb_put_dev(hcd->self.root_hub);
@@ -2674,6 +2688,9 @@ void usb_remove_hcd(struct usb_hcd *hcd)
 			free_irq(hcd->irq, hcd);
 	}
 
+	if (hcd->phy)
+		usb_phy_shutdown(hcd->phy);
+
 	usb_put_dev(hcd->self.root_hub);
 	usb_deregister_bus(&hcd->self);
 	hcd_buffer_destroy(hcd);
-- 
1.7.4.1

[PATCH] USB: initialize or shutdown PHY when add or remove host controller

[Felipe Balbi]

Hi,

On Tue, Jun 18, 2013 at 03:15:01AM -0400, Chao Xie wrote:
> Some controller need software to initialize PHY before add
> host controller, and shut down PHY after remove host controller.
> Add the generic code for these controllers so they do not need
> do it in its own host controller driver.
> 
> Signed-off-by: Chao Xie <chao.xie@marvell.com>
> ---
>  drivers/usb/core/hcd.c |   19 ++++++++++++++++++-
>  1 files changed, 18 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index d53547d..b26196b 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -43,6 +43,7 @@
>  
>  #include <linux/usb.h>
>  #include <linux/usb/hcd.h>
> +#include <linux/usb/phy.h>
>  
>  #include "usb.h"
>  
> @@ -2531,12 +2532,22 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  	 */
>  	set_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
>  
> +	/* Initialize the PHY before other hardware operation. */
> +	if (hcd->phy) {

this looks wrong for two reasons:

a) you're not grabbing the PHY here.

	You can't just assume another entity grabbed your PHY for you.

b) usb_get_phy() returns an error number

	so the proper check would be !IS_ERR()

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130618/5970adbc/attachment.sig>

Re: [PATCH] USB: initialize or shutdown PHY when add or remove host controller

[Felipe Balbi]

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

Hi,

On Tue, Jun 18, 2013 at 03:15:01AM -0400, Chao Xie wrote:
> Some controller need software to initialize PHY before add
> host controller, and shut down PHY after remove host controller.
> Add the generic code for these controllers so they do not need
> do it in its own host controller driver.
> 
> Signed-off-by: Chao Xie <chao.xie@marvell.com>
> ---
>  drivers/usb/core/hcd.c |   19 ++++++++++++++++++-
>  1 files changed, 18 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index d53547d..b26196b 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -43,6 +43,7 @@
>  
>  #include <linux/usb.h>
>  #include <linux/usb/hcd.h>
> +#include <linux/usb/phy.h>
>  
>  #include "usb.h"
>  
> @@ -2531,12 +2532,22 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  	 */
>  	set_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
>  
> +	/* Initialize the PHY before other hardware operation. */
> +	if (hcd->phy) {

this looks wrong for two reasons:

a) you're not grabbing the PHY here.

	You can't just assume another entity grabbed your PHY for you.

b) usb_get_phy() returns an error number

	so the proper check would be !IS_ERR()

-- 
balbi

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

[PATCH] USB: initialize or shutdown PHY when add or remove host controller

[Roger Quadros]

On 06/18/2013 11:01 AM, Felipe Balbi wrote:
> Hi,
> 
> On Tue, Jun 18, 2013 at 03:15:01AM -0400, Chao Xie wrote:
>> Some controller need software to initialize PHY before add
>> host controller, and shut down PHY after remove host controller.
>> Add the generic code for these controllers so they do not need
>> do it in its own host controller driver.
>>
>> Signed-off-by: Chao Xie <chao.xie@marvell.com>
>> ---
>>  drivers/usb/core/hcd.c |   19 ++++++++++++++++++-
>>  1 files changed, 18 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>> index d53547d..b26196b 100644
>> --- a/drivers/usb/core/hcd.c
>> +++ b/drivers/usb/core/hcd.c
>> @@ -43,6 +43,7 @@
>>  
>>  #include <linux/usb.h>
>>  #include <linux/usb/hcd.h>
>> +#include <linux/usb/phy.h>
>>  
>>  #include "usb.h"
>>  
>> @@ -2531,12 +2532,22 @@ int usb_add_hcd(struct usb_hcd *hcd,
>>  	 */
>>  	set_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
>>  
>> +	/* Initialize the PHY before other hardware operation. */
>> +	if (hcd->phy) {
> 
> this looks wrong for two reasons:
> 
> a) you're not grabbing the PHY here.
> 
> 	You can't just assume another entity grabbed your PHY for you.

Isn't that done in the controller drivers e.g. ehci-fsl.c, ohci-omap, etc?
If the controllers don't want HCD core to manage the PHY they can just set it
to some error code.

cheers,
-roger

Re: [PATCH] USB: initialize or shutdown PHY when add or remove host controller

[Roger Quadros]

On 06/18/2013 11:01 AM, Felipe Balbi wrote:
> Hi,
> 
> On Tue, Jun 18, 2013 at 03:15:01AM -0400, Chao Xie wrote:
>> Some controller need software to initialize PHY before add
>> host controller, and shut down PHY after remove host controller.
>> Add the generic code for these controllers so they do not need
>> do it in its own host controller driver.
>>
>> Signed-off-by: Chao Xie <chao.xie@marvell.com>
>> ---
>>  drivers/usb/core/hcd.c |   19 ++++++++++++++++++-
>>  1 files changed, 18 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>> index d53547d..b26196b 100644
>> --- a/drivers/usb/core/hcd.c
>> +++ b/drivers/usb/core/hcd.c
>> @@ -43,6 +43,7 @@
>>  
>>  #include <linux/usb.h>
>>  #include <linux/usb/hcd.h>
>> +#include <linux/usb/phy.h>
>>  
>>  #include "usb.h"
>>  
>> @@ -2531,12 +2532,22 @@ int usb_add_hcd(struct usb_hcd *hcd,
>>  	 */
>>  	set_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
>>  
>> +	/* Initialize the PHY before other hardware operation. */
>> +	if (hcd->phy) {
> 
> this looks wrong for two reasons:
> 
> a) you're not grabbing the PHY here.
> 
> 	You can't just assume another entity grabbed your PHY for you.

Isn't that done in the controller drivers e.g. ehci-fsl.c, ohci-omap, etc?
If the controllers don't want HCD core to manage the PHY they can just set it
to some error code.

cheers,
-roger

[PATCH] USB: initialize or shutdown PHY when add or remove host controller

[Felipe Balbi]

HI,

On Tue, Jun 18, 2013 at 11:23:59AM +0300, Roger Quadros wrote:
> On 06/18/2013 11:01 AM, Felipe Balbi wrote:
> > Hi,
> > 
> > On Tue, Jun 18, 2013 at 03:15:01AM -0400, Chao Xie wrote:
> >> Some controller need software to initialize PHY before add
> >> host controller, and shut down PHY after remove host controller.
> >> Add the generic code for these controllers so they do not need
> >> do it in its own host controller driver.
> >>
> >> Signed-off-by: Chao Xie <chao.xie@marvell.com>
> >> ---
> >>  drivers/usb/core/hcd.c |   19 ++++++++++++++++++-
> >>  1 files changed, 18 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> >> index d53547d..b26196b 100644
> >> --- a/drivers/usb/core/hcd.c
> >> +++ b/drivers/usb/core/hcd.c
> >> @@ -43,6 +43,7 @@
> >>  
> >>  #include <linux/usb.h>
> >>  #include <linux/usb/hcd.h>
> >> +#include <linux/usb/phy.h>
> >>  
> >>  #include "usb.h"
> >>  
> >> @@ -2531,12 +2532,22 @@ int usb_add_hcd(struct usb_hcd *hcd,
> >>  	 */
> >>  	set_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
> >>  
> >> +	/* Initialize the PHY before other hardware operation. */
> >> +	if (hcd->phy) {
> > 
> > this looks wrong for two reasons:
> > 
> > a) you're not grabbing the PHY here.
> > 
> > 	You can't just assume another entity grabbed your PHY for you.
> 
> Isn't that done in the controller drivers e.g. ehci-fsl.c, ohci-omap, etc?

right, and what I'm saying is that it should all be re-factored into
ehci-hcd core :-)

> If the controllers don't want HCD core to manage the PHY they can just set it
> to some error code.

they shouldn't have the choice, otherwise it'll be a bit of a PITA to
maintain the code. ehci core tries to grab the PHY, if it's not there,
try to continue anyway. Assume it's not needed.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130618/0ef4a89f/attachment.sig>

Re: [PATCH] USB: initialize or shutdown PHY when add or remove host controller

[Felipe Balbi]

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

HI,

On Tue, Jun 18, 2013 at 11:23:59AM +0300, Roger Quadros wrote:
> On 06/18/2013 11:01 AM, Felipe Balbi wrote:
> > Hi,
> > 
> > On Tue, Jun 18, 2013 at 03:15:01AM -0400, Chao Xie wrote:
> >> Some controller need software to initialize PHY before add
> >> host controller, and shut down PHY after remove host controller.
> >> Add the generic code for these controllers so they do not need
> >> do it in its own host controller driver.
> >>
> >> Signed-off-by: Chao Xie <chao.xie@marvell.com>
> >> ---
> >>  drivers/usb/core/hcd.c |   19 ++++++++++++++++++-
> >>  1 files changed, 18 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> >> index d53547d..b26196b 100644
> >> --- a/drivers/usb/core/hcd.c
> >> +++ b/drivers/usb/core/hcd.c
> >> @@ -43,6 +43,7 @@
> >>  
> >>  #include <linux/usb.h>
> >>  #include <linux/usb/hcd.h>
> >> +#include <linux/usb/phy.h>
> >>  
> >>  #include "usb.h"
> >>  
> >> @@ -2531,12 +2532,22 @@ int usb_add_hcd(struct usb_hcd *hcd,
> >>  	 */
> >>  	set_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
> >>  
> >> +	/* Initialize the PHY before other hardware operation. */
> >> +	if (hcd->phy) {
> > 
> > this looks wrong for two reasons:
> > 
> > a) you're not grabbing the PHY here.
> > 
> > 	You can't just assume another entity grabbed your PHY for you.
> 
> Isn't that done in the controller drivers e.g. ehci-fsl.c, ohci-omap, etc?

right, and what I'm saying is that it should all be re-factored into
ehci-hcd core :-)

> If the controllers don't want HCD core to manage the PHY they can just set it
> to some error code.

they shouldn't have the choice, otherwise it'll be a bit of a PITA to
maintain the code. ehci core tries to grab the PHY, if it's not there,
try to continue anyway. Assume it's not needed.

-- 
balbi

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

[PATCH] USB: initialize or shutdown PHY when add or remove host controller

[Roger Quadros]

On 06/18/2013 11:24 AM, Felipe Balbi wrote:
> HI,
> 
> On Tue, Jun 18, 2013 at 11:23:59AM +0300, Roger Quadros wrote:
>> On 06/18/2013 11:01 AM, Felipe Balbi wrote:
>>> Hi,
>>>
>>> On Tue, Jun 18, 2013 at 03:15:01AM -0400, Chao Xie wrote:
>>>> Some controller need software to initialize PHY before add
>>>> host controller, and shut down PHY after remove host controller.
>>>> Add the generic code for these controllers so they do not need
>>>> do it in its own host controller driver.
>>>>
>>>> Signed-off-by: Chao Xie <chao.xie@marvell.com>
>>>> ---
>>>>  drivers/usb/core/hcd.c |   19 ++++++++++++++++++-
>>>>  1 files changed, 18 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>>>> index d53547d..b26196b 100644
>>>> --- a/drivers/usb/core/hcd.c
>>>> +++ b/drivers/usb/core/hcd.c
>>>> @@ -43,6 +43,7 @@
>>>>  
>>>>  #include <linux/usb.h>
>>>>  #include <linux/usb/hcd.h>
>>>> +#include <linux/usb/phy.h>
>>>>  
>>>>  #include "usb.h"
>>>>  
>>>> @@ -2531,12 +2532,22 @@ int usb_add_hcd(struct usb_hcd *hcd,
>>>>  	 */
>>>>  	set_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
>>>>  
>>>> +	/* Initialize the PHY before other hardware operation. */
>>>> +	if (hcd->phy) {
>>>
>>> this looks wrong for two reasons:
>>>
>>> a) you're not grabbing the PHY here.
>>>
>>> 	You can't just assume another entity grabbed your PHY for you.
>>
>> Isn't that done in the controller drivers e.g. ehci-fsl.c, ohci-omap, etc?
> 
> right, and what I'm saying is that it should all be re-factored into
> ehci-hcd core :-)
> 
>> If the controllers don't want HCD core to manage the PHY they can just set it
>> to some error code.
> 
> they shouldn't have the choice, otherwise it'll be a bit of a PITA to
> maintain the code. ehci core tries to grab the PHY, if it's not there,
> try to continue anyway. Assume it's not needed.
> 

OK fine, but ehci-omap is a weird case as it needs a slightly different
sequence as to when PHY is initialized depending on which mode it is. (Transceiver
or transceiver-less). please see this fix.
http://www.spinics.net/lists/stable/msg12106.html

All I'm saying as that ehci-omap needs a way to tell hcd core that it needs PHY
handling for itself.

cheers,
-roger

Re: [PATCH] USB: initialize or shutdown PHY when add or remove host controller

[Roger Quadros]

On 06/18/2013 11:24 AM, Felipe Balbi wrote:
> HI,
> 
> On Tue, Jun 18, 2013 at 11:23:59AM +0300, Roger Quadros wrote:
>> On 06/18/2013 11:01 AM, Felipe Balbi wrote:
>>> Hi,
>>>
>>> On Tue, Jun 18, 2013 at 03:15:01AM -0400, Chao Xie wrote:
>>>> Some controller need software to initialize PHY before add
>>>> host controller, and shut down PHY after remove host controller.
>>>> Add the generic code for these controllers so they do not need
>>>> do it in its own host controller driver.
>>>>
>>>> Signed-off-by: Chao Xie <chao.xie@marvell.com>
>>>> ---
>>>>  drivers/usb/core/hcd.c |   19 ++++++++++++++++++-
>>>>  1 files changed, 18 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>>>> index d53547d..b26196b 100644
>>>> --- a/drivers/usb/core/hcd.c
>>>> +++ b/drivers/usb/core/hcd.c
>>>> @@ -43,6 +43,7 @@
>>>>  
>>>>  #include <linux/usb.h>
>>>>  #include <linux/usb/hcd.h>
>>>> +#include <linux/usb/phy.h>
>>>>  
>>>>  #include "usb.h"
>>>>  
>>>> @@ -2531,12 +2532,22 @@ int usb_add_hcd(struct usb_hcd *hcd,
>>>>  	 */
>>>>  	set_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
>>>>  
>>>> +	/* Initialize the PHY before other hardware operation. */
>>>> +	if (hcd->phy) {
>>>
>>> this looks wrong for two reasons:
>>>
>>> a) you're not grabbing the PHY here.
>>>
>>> 	You can't just assume another entity grabbed your PHY for you.
>>
>> Isn't that done in the controller drivers e.g. ehci-fsl.c, ohci-omap, etc?
> 
> right, and what I'm saying is that it should all be re-factored into
> ehci-hcd core :-)
> 
>> If the controllers don't want HCD core to manage the PHY they can just set it
>> to some error code.
> 
> they shouldn't have the choice, otherwise it'll be a bit of a PITA to
> maintain the code. ehci core tries to grab the PHY, if it's not there,
> try to continue anyway. Assume it's not needed.
> 

OK fine, but ehci-omap is a weird case as it needs a slightly different
sequence as to when PHY is initialized depending on which mode it is. (Transceiver
or transceiver-less). please see this fix.
http://www.spinics.net/lists/stable/msg12106.html

All I'm saying as that ehci-omap needs a way to tell hcd core that it needs PHY
handling for itself.

cheers,
-roger

[PATCH] USB: initialize or shutdown PHY when add or remove host controller

[Felipe Balbi]

Hi,

On Tue, Jun 18, 2013 at 11:34:08AM +0300, Roger Quadros wrote:
> >>> On Tue, Jun 18, 2013 at 03:15:01AM -0400, Chao Xie wrote:
> >>>> Some controller need software to initialize PHY before add
> >>>> host controller, and shut down PHY after remove host controller.
> >>>> Add the generic code for these controllers so they do not need
> >>>> do it in its own host controller driver.
> >>>>
> >>>> Signed-off-by: Chao Xie <chao.xie@marvell.com>
> >>>> ---
> >>>>  drivers/usb/core/hcd.c |   19 ++++++++++++++++++-
> >>>>  1 files changed, 18 insertions(+), 1 deletions(-)
> >>>>
> >>>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> >>>> index d53547d..b26196b 100644
> >>>> --- a/drivers/usb/core/hcd.c
> >>>> +++ b/drivers/usb/core/hcd.c
> >>>> @@ -43,6 +43,7 @@
> >>>>  
> >>>>  #include <linux/usb.h>
> >>>>  #include <linux/usb/hcd.h>
> >>>> +#include <linux/usb/phy.h>
> >>>>  
> >>>>  #include "usb.h"
> >>>>  
> >>>> @@ -2531,12 +2532,22 @@ int usb_add_hcd(struct usb_hcd *hcd,
> >>>>  	 */
> >>>>  	set_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
> >>>>  
> >>>> +	/* Initialize the PHY before other hardware operation. */
> >>>> +	if (hcd->phy) {
> >>>
> >>> this looks wrong for two reasons:
> >>>
> >>> a) you're not grabbing the PHY here.
> >>>
> >>> 	You can't just assume another entity grabbed your PHY for you.
> >>
> >> Isn't that done in the controller drivers e.g. ehci-fsl.c, ohci-omap, etc?
> > 
> > right, and what I'm saying is that it should all be re-factored into
> > ehci-hcd core :-)
> > 
> >> If the controllers don't want HCD core to manage the PHY they can just set it
> >> to some error code.
> > 
> > they shouldn't have the choice, otherwise it'll be a bit of a PITA to
> > maintain the code. ehci core tries to grab the PHY, if it's not there,
> > try to continue anyway. Assume it's not needed.
> > 
> 
> OK fine, but ehci-omap is a weird case as it needs a slightly different
> sequence as to when PHY is initialized depending on which mode it is. (Transceiver
> or transceiver-less). please see this fix.
> http://www.spinics.net/lists/stable/msg12106.html
> 
> All I'm saying as that ehci-omap needs a way to tell hcd core that it needs PHY
> handling for itself.

why don't you do that always ? Meaning, why don't you *always* take PHY
out of suspend ? If PHY is suspended, you can't wakeup unless you have
(in OMAP case) pad wakeup working, right ?

Moreover, if you can suspend the PHY and still wakup, that's something
we need to teach the PHY layer about. Currently it doesn't know anything
about such wakeup capable PHYs ;-)

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130618/311b1b74/attachment.sig>

Re: [PATCH] USB: initialize or shutdown PHY when add or remove host controller

[Felipe Balbi]

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

Hi,

On Tue, Jun 18, 2013 at 11:34:08AM +0300, Roger Quadros wrote:
> >>> On Tue, Jun 18, 2013 at 03:15:01AM -0400, Chao Xie wrote:
> >>>> Some controller need software to initialize PHY before add
> >>>> host controller, and shut down PHY after remove host controller.
> >>>> Add the generic code for these controllers so they do not need
> >>>> do it in its own host controller driver.
> >>>>
> >>>> Signed-off-by: Chao Xie <chao.xie@marvell.com>
> >>>> ---
> >>>>  drivers/usb/core/hcd.c |   19 ++++++++++++++++++-
> >>>>  1 files changed, 18 insertions(+), 1 deletions(-)
> >>>>
> >>>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> >>>> index d53547d..b26196b 100644
> >>>> --- a/drivers/usb/core/hcd.c
> >>>> +++ b/drivers/usb/core/hcd.c
> >>>> @@ -43,6 +43,7 @@
> >>>>  
> >>>>  #include <linux/usb.h>
> >>>>  #include <linux/usb/hcd.h>
> >>>> +#include <linux/usb/phy.h>
> >>>>  
> >>>>  #include "usb.h"
> >>>>  
> >>>> @@ -2531,12 +2532,22 @@ int usb_add_hcd(struct usb_hcd *hcd,
> >>>>  	 */
> >>>>  	set_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
> >>>>  
> >>>> +	/* Initialize the PHY before other hardware operation. */
> >>>> +	if (hcd->phy) {
> >>>
> >>> this looks wrong for two reasons:
> >>>
> >>> a) you're not grabbing the PHY here.
> >>>
> >>> 	You can't just assume another entity grabbed your PHY for you.
> >>
> >> Isn't that done in the controller drivers e.g. ehci-fsl.c, ohci-omap, etc?
> > 
> > right, and what I'm saying is that it should all be re-factored into
> > ehci-hcd core :-)
> > 
> >> If the controllers don't want HCD core to manage the PHY they can just set it
> >> to some error code.
> > 
> > they shouldn't have the choice, otherwise it'll be a bit of a PITA to
> > maintain the code. ehci core tries to grab the PHY, if it's not there,
> > try to continue anyway. Assume it's not needed.
> > 
> 
> OK fine, but ehci-omap is a weird case as it needs a slightly different
> sequence as to when PHY is initialized depending on which mode it is. (Transceiver
> or transceiver-less). please see this fix.
> http://www.spinics.net/lists/stable/msg12106.html
> 
> All I'm saying as that ehci-omap needs a way to tell hcd core that it needs PHY
> handling for itself.

why don't you do that always ? Meaning, why don't you *always* take PHY
out of suspend ? If PHY is suspended, you can't wakeup unless you have
(in OMAP case) pad wakeup working, right ?

Moreover, if you can suspend the PHY and still wakup, that's something
we need to teach the PHY layer about. Currently it doesn't know anything
about such wakeup capable PHYs ;-)

-- 
balbi

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

[PATCH] USB: initialize or shutdown PHY when add or remove host controller

[Roger Quadros]

On 06/18/2013 11:37 AM, Felipe Balbi wrote:
> Hi,
> 
> On Tue, Jun 18, 2013 at 11:34:08AM +0300, Roger Quadros wrote:
>>>>> On Tue, Jun 18, 2013 at 03:15:01AM -0400, Chao Xie wrote:
>>>>>> Some controller need software to initialize PHY before add
>>>>>> host controller, and shut down PHY after remove host controller.
>>>>>> Add the generic code for these controllers so they do not need
>>>>>> do it in its own host controller driver.
>>>>>>
>>>>>> Signed-off-by: Chao Xie <chao.xie@marvell.com>
>>>>>> ---
>>>>>>  drivers/usb/core/hcd.c |   19 ++++++++++++++++++-
>>>>>>  1 files changed, 18 insertions(+), 1 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>>>>>> index d53547d..b26196b 100644
>>>>>> --- a/drivers/usb/core/hcd.c
>>>>>> +++ b/drivers/usb/core/hcd.c
>>>>>> @@ -43,6 +43,7 @@
>>>>>>  
>>>>>>  #include <linux/usb.h>
>>>>>>  #include <linux/usb/hcd.h>
>>>>>> +#include <linux/usb/phy.h>
>>>>>>  
>>>>>>  #include "usb.h"
>>>>>>  
>>>>>> @@ -2531,12 +2532,22 @@ int usb_add_hcd(struct usb_hcd *hcd,
>>>>>>  	 */
>>>>>>  	set_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
>>>>>>  
>>>>>> +	/* Initialize the PHY before other hardware operation. */
>>>>>> +	if (hcd->phy) {
>>>>>
>>>>> this looks wrong for two reasons:
>>>>>
>>>>> a) you're not grabbing the PHY here.
>>>>>
>>>>> 	You can't just assume another entity grabbed your PHY for you.
>>>>
>>>> Isn't that done in the controller drivers e.g. ehci-fsl.c, ohci-omap, etc?
>>>
>>> right, and what I'm saying is that it should all be re-factored into
>>> ehci-hcd core :-)
>>>
>>>> If the controllers don't want HCD core to manage the PHY they can just set it
>>>> to some error code.
>>>
>>> they shouldn't have the choice, otherwise it'll be a bit of a PITA to
>>> maintain the code. ehci core tries to grab the PHY, if it's not there,
>>> try to continue anyway. Assume it's not needed.
>>>
>>
>> OK fine, but ehci-omap is a weird case as it needs a slightly different
>> sequence as to when PHY is initialized depending on which mode it is. (Transceiver
>> or transceiver-less). please see this fix.
>> http://www.spinics.net/lists/stable/msg12106.html
>>
>> All I'm saying as that ehci-omap needs a way to tell hcd core that it needs PHY
>> handling for itself.
> 
> why don't you do that always ? Meaning, why don't you *always* take PHY
> out of suspend ? If PHY is suspended, you can't wakeup unless you have
> (in OMAP case) pad wakeup working, right ?
> 

Maybe I wasn't clear before. This is nothing about wakeup and e always take PHY out of suspend. 
The problem is when to take it out of suspend relative to when EHCI controller starts.
Let me clarify.

In Transceiver mode we need this.

- bring phy out of reset
- start EHCI controller

Whereas for Transceiver-less mode we need this.

- start EHCI controller
- bring phy out of reset

If there is some way to signal this behaviour to the HCD core, it should be good enough.

cheers,
-roger

Re: [PATCH] USB: initialize or shutdown PHY when add or remove host controller

[Roger Quadros]

On 06/18/2013 11:37 AM, Felipe Balbi wrote:
> Hi,
> 
> On Tue, Jun 18, 2013 at 11:34:08AM +0300, Roger Quadros wrote:
>>>>> On Tue, Jun 18, 2013 at 03:15:01AM -0400, Chao Xie wrote:
>>>>>> Some controller need software to initialize PHY before add
>>>>>> host controller, and shut down PHY after remove host controller.
>>>>>> Add the generic code for these controllers so they do not need
>>>>>> do it in its own host controller driver.
>>>>>>
>>>>>> Signed-off-by: Chao Xie <chao.xie@marvell.com>
>>>>>> ---
>>>>>>  drivers/usb/core/hcd.c |   19 ++++++++++++++++++-
>>>>>>  1 files changed, 18 insertions(+), 1 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>>>>>> index d53547d..b26196b 100644
>>>>>> --- a/drivers/usb/core/hcd.c
>>>>>> +++ b/drivers/usb/core/hcd.c
>>>>>> @@ -43,6 +43,7 @@
>>>>>>  
>>>>>>  #include <linux/usb.h>
>>>>>>  #include <linux/usb/hcd.h>
>>>>>> +#include <linux/usb/phy.h>
>>>>>>  
>>>>>>  #include "usb.h"
>>>>>>  
>>>>>> @@ -2531,12 +2532,22 @@ int usb_add_hcd(struct usb_hcd *hcd,
>>>>>>  	 */
>>>>>>  	set_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
>>>>>>  
>>>>>> +	/* Initialize the PHY before other hardware operation. */
>>>>>> +	if (hcd->phy) {
>>>>>
>>>>> this looks wrong for two reasons:
>>>>>
>>>>> a) you're not grabbing the PHY here.
>>>>>
>>>>> 	You can't just assume another entity grabbed your PHY for you.
>>>>
>>>> Isn't that done in the controller drivers e.g. ehci-fsl.c, ohci-omap, etc?
>>>
>>> right, and what I'm saying is that it should all be re-factored into
>>> ehci-hcd core :-)
>>>
>>>> If the controllers don't want HCD core to manage the PHY they can just set it
>>>> to some error code.
>>>
>>> they shouldn't have the choice, otherwise it'll be a bit of a PITA to
>>> maintain the code. ehci core tries to grab the PHY, if it's not there,
>>> try to continue anyway. Assume it's not needed.
>>>
>>
>> OK fine, but ehci-omap is a weird case as it needs a slightly different
>> sequence as to when PHY is initialized depending on which mode it is. (Transceiver
>> or transceiver-less). please see this fix.
>> http://www.spinics.net/lists/stable/msg12106.html
>>
>> All I'm saying as that ehci-omap needs a way to tell hcd core that it needs PHY
>> handling for itself.
> 
> why don't you do that always ? Meaning, why don't you *always* take PHY
> out of suspend ? If PHY is suspended, you can't wakeup unless you have
> (in OMAP case) pad wakeup working, right ?
> 

Maybe I wasn't clear before. This is nothing about wakeup and e always take PHY out of suspend. 
The problem is when to take it out of suspend relative to when EHCI controller starts.
Let me clarify.

In Transceiver mode we need this.

- bring phy out of reset
- start EHCI controller

Whereas for Transceiver-less mode we need this.

- start EHCI controller
- bring phy out of reset

If there is some way to signal this behaviour to the HCD core, it should be good enough.

cheers,
-roger

[PATCH] USB: initialize or shutdown PHY when add or remove host controller

[Felipe Balbi]

Hi,

On Tue, Jun 18, 2013 at 11:45:05AM +0300, Roger Quadros wrote:
> >>>>> this looks wrong for two reasons:
> >>>>>
> >>>>> a) you're not grabbing the PHY here.
> >>>>>
> >>>>> 	You can't just assume another entity grabbed your PHY for you.
> >>>>
> >>>> Isn't that done in the controller drivers e.g. ehci-fsl.c, ohci-omap, etc?
> >>>
> >>> right, and what I'm saying is that it should all be re-factored into
> >>> ehci-hcd core :-)
> >>>
> >>>> If the controllers don't want HCD core to manage the PHY they can just set it
> >>>> to some error code.
> >>>
> >>> they shouldn't have the choice, otherwise it'll be a bit of a PITA to
> >>> maintain the code. ehci core tries to grab the PHY, if it's not there,
> >>> try to continue anyway. Assume it's not needed.
> >>>
> >>
> >> OK fine, but ehci-omap is a weird case as it needs a slightly different
> >> sequence as to when PHY is initialized depending on which mode it is. (Transceiver
> >> or transceiver-less). please see this fix.
> >> http://www.spinics.net/lists/stable/msg12106.html
> >>
> >> All I'm saying as that ehci-omap needs a way to tell hcd core that it needs PHY
> >> handling for itself.
> > 
> > why don't you do that always ? Meaning, why don't you *always* take PHY
> > out of suspend ? If PHY is suspended, you can't wakeup unless you have
> > (in OMAP case) pad wakeup working, right ?
> > 
> 
> Maybe I wasn't clear before. This is nothing about wakeup and e always take PHY out of suspend. 
> The problem is when to take it out of suspend relative to when EHCI controller starts.
> Let me clarify.
> 
> In Transceiver mode we need this.
> 
> - bring phy out of reset
> - start EHCI controller
> 
> Whereas for Transceiver-less mode we need this.
> 
> - start EHCI controller
> - bring phy out of reset
> 
> If there is some way to signal this behaviour to the HCD core, it
> should be good enough.

alright, now I get it. That's quite messed up that it has to be this way
:-p

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130618/fc604c6d/attachment.sig>

Re: [PATCH] USB: initialize or shutdown PHY when add or remove host controller

[Felipe Balbi]

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

Hi,

On Tue, Jun 18, 2013 at 11:45:05AM +0300, Roger Quadros wrote:
> >>>>> this looks wrong for two reasons:
> >>>>>
> >>>>> a) you're not grabbing the PHY here.
> >>>>>
> >>>>> 	You can't just assume another entity grabbed your PHY for you.
> >>>>
> >>>> Isn't that done in the controller drivers e.g. ehci-fsl.c, ohci-omap, etc?
> >>>
> >>> right, and what I'm saying is that it should all be re-factored into
> >>> ehci-hcd core :-)
> >>>
> >>>> If the controllers don't want HCD core to manage the PHY they can just set it
> >>>> to some error code.
> >>>
> >>> they shouldn't have the choice, otherwise it'll be a bit of a PITA to
> >>> maintain the code. ehci core tries to grab the PHY, if it's not there,
> >>> try to continue anyway. Assume it's not needed.
> >>>
> >>
> >> OK fine, but ehci-omap is a weird case as it needs a slightly different
> >> sequence as to when PHY is initialized depending on which mode it is. (Transceiver
> >> or transceiver-less). please see this fix.
> >> http://www.spinics.net/lists/stable/msg12106.html
> >>
> >> All I'm saying as that ehci-omap needs a way to tell hcd core that it needs PHY
> >> handling for itself.
> > 
> > why don't you do that always ? Meaning, why don't you *always* take PHY
> > out of suspend ? If PHY is suspended, you can't wakeup unless you have
> > (in OMAP case) pad wakeup working, right ?
> > 
> 
> Maybe I wasn't clear before. This is nothing about wakeup and e always take PHY out of suspend. 
> The problem is when to take it out of suspend relative to when EHCI controller starts.
> Let me clarify.
> 
> In Transceiver mode we need this.
> 
> - bring phy out of reset
> - start EHCI controller
> 
> Whereas for Transceiver-less mode we need this.
> 
> - start EHCI controller
> - bring phy out of reset
> 
> If there is some way to signal this behaviour to the HCD core, it
> should be good enough.

alright, now I get it. That's quite messed up that it has to be this way
:-p

-- 
balbi

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

[PATCH] USB: initialize or shutdown PHY when add or remove host controller

[Chao Xie]

On Tue, Jun 18, 2013 at 4:48 PM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Tue, Jun 18, 2013 at 11:45:05AM +0300, Roger Quadros wrote:
>> >>>>> this looks wrong for two reasons:
>> >>>>>
>> >>>>> a) you're not grabbing the PHY here.
>> >>>>>
>> >>>>>         You can't just assume another entity grabbed your PHY for you.
>> >>>>
>> >>>> Isn't that done in the controller drivers e.g. ehci-fsl.c, ohci-omap, etc?
>> >>>
>> >>> right, and what I'm saying is that it should all be re-factored into
>> >>> ehci-hcd core :-)
>> >>>
>> >>>> If the controllers don't want HCD core to manage the PHY they can just set it
>> >>>> to some error code.
>> >>>
>> >>> they shouldn't have the choice, otherwise it'll be a bit of a PITA to
>> >>> maintain the code. ehci core tries to grab the PHY, if it's not there,
>> >>> try to continue anyway. Assume it's not needed.
>> >>>
>> >>
>> >> OK fine, but ehci-omap is a weird case as it needs a slightly different
>> >> sequence as to when PHY is initialized depending on which mode it is. (Transceiver
>> >> or transceiver-less). please see this fix.
>> >> http://www.spinics.net/lists/stable/msg12106.html
>> >>
>> >> All I'm saying as that ehci-omap needs a way to tell hcd core that it needs PHY
>> >> handling for itself.
>> >
>> > why don't you do that always ? Meaning, why don't you *always* take PHY
>> > out of suspend ? If PHY is suspended, you can't wakeup unless you have
>> > (in OMAP case) pad wakeup working, right ?
>> >
>>
>> Maybe I wasn't clear before. This is nothing about wakeup and e always take PHY out of suspend.
>> The problem is when to take it out of suspend relative to when EHCI controller starts.
>> Let me clarify.
>>
>> In Transceiver mode we need this.
>>
>> - bring phy out of reset
>> - start EHCI controller
>>
>> Whereas for Transceiver-less mode we need this.
>>
>> - start EHCI controller
>> - bring phy out of reset
>>
>> If there is some way to signal this behaviour to the HCD core, it
>> should be good enough.
>
> alright, now I get it. That's quite messed up that it has to be this way
> :-p
>
It depends on the host controler's driver ehci-xxx to get the phy and
set it to hcd->phy.
If some controller do not want HCD or EHCI-HCD to do the phy
initialization and shutdown, just do not set hcd->phy, and it will be
NULL.
If the host controller driver successlly get the phy, it can set
hcd->phy, or it need return false in its probe.


> --
> balbi

Re: [PATCH] USB: initialize or shutdown PHY when add or remove host controller

[Chao Xie]

On Tue, Jun 18, 2013 at 4:48 PM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Tue, Jun 18, 2013 at 11:45:05AM +0300, Roger Quadros wrote:
>> >>>>> this looks wrong for two reasons:
>> >>>>>
>> >>>>> a) you're not grabbing the PHY here.
>> >>>>>
>> >>>>>         You can't just assume another entity grabbed your PHY for you.
>> >>>>
>> >>>> Isn't that done in the controller drivers e.g. ehci-fsl.c, ohci-omap, etc?
>> >>>
>> >>> right, and what I'm saying is that it should all be re-factored into
>> >>> ehci-hcd core :-)
>> >>>
>> >>>> If the controllers don't want HCD core to manage the PHY they can just set it
>> >>>> to some error code.
>> >>>
>> >>> they shouldn't have the choice, otherwise it'll be a bit of a PITA to
>> >>> maintain the code. ehci core tries to grab the PHY, if it's not there,
>> >>> try to continue anyway. Assume it's not needed.
>> >>>
>> >>
>> >> OK fine, but ehci-omap is a weird case as it needs a slightly different
>> >> sequence as to when PHY is initialized depending on which mode it is. (Transceiver
>> >> or transceiver-less). please see this fix.
>> >> http://www.spinics.net/lists/stable/msg12106.html
>> >>
>> >> All I'm saying as that ehci-omap needs a way to tell hcd core that it needs PHY
>> >> handling for itself.
>> >
>> > why don't you do that always ? Meaning, why don't you *always* take PHY
>> > out of suspend ? If PHY is suspended, you can't wakeup unless you have
>> > (in OMAP case) pad wakeup working, right ?
>> >
>>
>> Maybe I wasn't clear before. This is nothing about wakeup and e always take PHY out of suspend.
>> The problem is when to take it out of suspend relative to when EHCI controller starts.
>> Let me clarify.
>>
>> In Transceiver mode we need this.
>>
>> - bring phy out of reset
>> - start EHCI controller
>>
>> Whereas for Transceiver-less mode we need this.
>>
>> - start EHCI controller
>> - bring phy out of reset
>>
>> If there is some way to signal this behaviour to the HCD core, it
>> should be good enough.
>
> alright, now I get it. That's quite messed up that it has to be this way
> :-p
>
It depends on the host controler's driver ehci-xxx to get the phy and
set it to hcd->phy.
If some controller do not want HCD or EHCI-HCD to do the phy
initialization and shutdown, just do not set hcd->phy, and it will be
NULL.
If the host controller driver successlly get the phy, it can set
hcd->phy, or it need return false in its probe.


> --
> balbi

[PATCH] USB: initialize or shutdown PHY when add or remove host controller

[Alan Stern]

On Tue, 18 Jun 2013, Felipe Balbi wrote:

> HI,
> 
> On Tue, Jun 18, 2013 at 11:23:59AM +0300, Roger Quadros wrote:
> > On 06/18/2013 11:01 AM, Felipe Balbi wrote:
> > > Hi,
> > > 
> > > On Tue, Jun 18, 2013 at 03:15:01AM -0400, Chao Xie wrote:
> > >> Some controller need software to initialize PHY before add
> > >> host controller, and shut down PHY after remove host controller.
> > >> Add the generic code for these controllers so they do not need
> > >> do it in its own host controller driver.
> > >>
> > >> Signed-off-by: Chao Xie <chao.xie@marvell.com>
> > >> ---
> > >>  drivers/usb/core/hcd.c |   19 ++++++++++++++++++-
> > >>  1 files changed, 18 insertions(+), 1 deletions(-)
> > >>
> > >> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> > >> index d53547d..b26196b 100644
> > >> --- a/drivers/usb/core/hcd.c
> > >> +++ b/drivers/usb/core/hcd.c
> > >> @@ -43,6 +43,7 @@
> > >>  
> > >>  #include <linux/usb.h>
> > >>  #include <linux/usb/hcd.h>
> > >> +#include <linux/usb/phy.h>
> > >>  
> > >>  #include "usb.h"
> > >>  
> > >> @@ -2531,12 +2532,22 @@ int usb_add_hcd(struct usb_hcd *hcd,
> > >>  	 */
> > >>  	set_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
> > >>  
> > >> +	/* Initialize the PHY before other hardware operation. */
> > >> +	if (hcd->phy) {
> > > 
> > > this looks wrong for two reasons:
> > > 
> > > a) you're not grabbing the PHY here.
> > > 
> > > 	You can't just assume another entity grabbed your PHY for you.
> > 
> > Isn't that done in the controller drivers e.g. ehci-fsl.c, ohci-omap, etc?
> 
> right, and what I'm saying is that it should all be re-factored into
> ehci-hcd core :-)
> 
> > If the controllers don't want HCD core to manage the PHY they can just set it
> > to some error code.
> 
> they shouldn't have the choice, otherwise it'll be a bit of a PITA to
> maintain the code. ehci core tries to grab the PHY, if it's not there,
> try to continue anyway. Assume it's not needed.

Felipe, are all these issues straightened out to your satisfaction?  I
can't tell from the email thread.

Looking through the EHCI glue source files, there appears to be a 
variety of different ways of getting the PHY.  It's not at all clear 
that they can be moved into the ehci-hcd core (or even better, the USB 
core -- which is what Chao is trying to do).

Given that the glue module has to be responsible for getting the PHY,
it should also be responsible for error checking.  So the code added to
hcd.c doesn't need to apply an IS_ERR check; it can simply assume that
if hcd->phy is NULL then either there is no software-controllable PHY
or else the HCD doesn't want the core to manage it.

Alan Stern

Re: [PATCH] USB: initialize or shutdown PHY when add or remove host controller

[Alan Stern]

On Tue, 18 Jun 2013, Felipe Balbi wrote:

> HI,
> 
> On Tue, Jun 18, 2013 at 11:23:59AM +0300, Roger Quadros wrote:
> > On 06/18/2013 11:01 AM, Felipe Balbi wrote:
> > > Hi,
> > > 
> > > On Tue, Jun 18, 2013 at 03:15:01AM -0400, Chao Xie wrote:
> > >> Some controller need software to initialize PHY before add
> > >> host controller, and shut down PHY after remove host controller.
> > >> Add the generic code for these controllers so they do not need
> > >> do it in its own host controller driver.
> > >>
> > >> Signed-off-by: Chao Xie <chao.xie@marvell.com>
> > >> ---
> > >>  drivers/usb/core/hcd.c |   19 ++++++++++++++++++-
> > >>  1 files changed, 18 insertions(+), 1 deletions(-)
> > >>
> > >> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> > >> index d53547d..b26196b 100644
> > >> --- a/drivers/usb/core/hcd.c
> > >> +++ b/drivers/usb/core/hcd.c
> > >> @@ -43,6 +43,7 @@
> > >>  
> > >>  #include <linux/usb.h>
> > >>  #include <linux/usb/hcd.h>
> > >> +#include <linux/usb/phy.h>
> > >>  
> > >>  #include "usb.h"
> > >>  
> > >> @@ -2531,12 +2532,22 @@ int usb_add_hcd(struct usb_hcd *hcd,
> > >>  	 */
> > >>  	set_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
> > >>  
> > >> +	/* Initialize the PHY before other hardware operation. */
> > >> +	if (hcd->phy) {
> > > 
> > > this looks wrong for two reasons:
> > > 
> > > a) you're not grabbing the PHY here.
> > > 
> > > 	You can't just assume another entity grabbed your PHY for you.
> > 
> > Isn't that done in the controller drivers e.g. ehci-fsl.c, ohci-omap, etc?
> 
> right, and what I'm saying is that it should all be re-factored into
> ehci-hcd core :-)
> 
> > If the controllers don't want HCD core to manage the PHY they can just set it
> > to some error code.
> 
> they shouldn't have the choice, otherwise it'll be a bit of a PITA to
> maintain the code. ehci core tries to grab the PHY, if it's not there,
> try to continue anyway. Assume it's not needed.

Felipe, are all these issues straightened out to your satisfaction?  I
can't tell from the email thread.

Looking through the EHCI glue source files, there appears to be a 
variety of different ways of getting the PHY.  It's not at all clear 
that they can be moved into the ehci-hcd core (or even better, the USB 
core -- which is what Chao is trying to do).

Given that the glue module has to be responsible for getting the PHY,
it should also be responsible for error checking.  So the code added to
hcd.c doesn't need to apply an IS_ERR check; it can simply assume that
if hcd->phy is NULL then either there is no software-controllable PHY
or else the HCD doesn't want the core to manage it.

Alan Stern

[PATCH] USB: initialize or shutdown PHY when add or remove host controller

[Felipe Balbi]

Hi,

On Tue, Jun 18, 2013 at 10:53:26AM -0400, Alan Stern wrote:
> > > If the controllers don't want HCD core to manage the PHY they can just set it
> > > to some error code.
> > 
> > they shouldn't have the choice, otherwise it'll be a bit of a PITA to
> > maintain the code. ehci core tries to grab the PHY, if it's not there,
> > try to continue anyway. Assume it's not needed.
> 
> Felipe, are all these issues straightened out to your satisfaction?  I
> can't tell from the email thread.
> 
> Looking through the EHCI glue source files, there appears to be a 
> variety of different ways of getting the PHY.  It's not at all clear 
> that they can be moved into the ehci-hcd core (or even better, the USB 
> core -- which is what Chao is trying to do).

yeah, Roger brought up a big problem with OMAP's EHCI depending on the
mode so, at least for now, we should keep phy_get and, in case of EHCI
OMAP, phy_init in the glue :-(

Roger has all the details, and they're also in the list archives, but
basically, depending on the mode, PHY *must* be initialized at a
particular point.

> Given that the glue module has to be responsible for getting the PHY,
> it should also be responsible for error checking.  So the code added to
> hcd.c doesn't need to apply an IS_ERR check; it can simply assume that
> if hcd->phy is NULL then either there is no software-controllable PHY
> or else the HCD doesn't want the core to manage it.

makes sense to me, add the requirement to:

if (IS_ERR(hcd->phy))
	hcd->phy = NULL;

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130618/fa95cad7/attachment-0001.sig>

Re: [PATCH] USB: initialize or shutdown PHY when add or remove host controller

[Felipe Balbi]

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

Hi,

On Tue, Jun 18, 2013 at 10:53:26AM -0400, Alan Stern wrote:
> > > If the controllers don't want HCD core to manage the PHY they can just set it
> > > to some error code.
> > 
> > they shouldn't have the choice, otherwise it'll be a bit of a PITA to
> > maintain the code. ehci core tries to grab the PHY, if it's not there,
> > try to continue anyway. Assume it's not needed.
> 
> Felipe, are all these issues straightened out to your satisfaction?  I
> can't tell from the email thread.
> 
> Looking through the EHCI glue source files, there appears to be a 
> variety of different ways of getting the PHY.  It's not at all clear 
> that they can be moved into the ehci-hcd core (or even better, the USB 
> core -- which is what Chao is trying to do).

yeah, Roger brought up a big problem with OMAP's EHCI depending on the
mode so, at least for now, we should keep phy_get and, in case of EHCI
OMAP, phy_init in the glue :-(

Roger has all the details, and they're also in the list archives, but
basically, depending on the mode, PHY *must* be initialized at a
particular point.

> Given that the glue module has to be responsible for getting the PHY,
> it should also be responsible for error checking.  So the code added to
> hcd.c doesn't need to apply an IS_ERR check; it can simply assume that
> if hcd->phy is NULL then either there is no software-controllable PHY
> or else the HCD doesn't want the core to manage it.

makes sense to me, add the requirement to:

if (IS_ERR(hcd->phy))
	hcd->phy = NULL;

-- 
balbi

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

[PATCH] USB: initialize or shutdown PHY when add or remove host controller

[Alan Stern]

On Tue, 18 Jun 2013, Felipe Balbi wrote:

> yeah, Roger brought up a big problem with OMAP's EHCI depending on the
> mode so, at least for now, we should keep phy_get and, in case of EHCI
> OMAP, phy_init in the glue :-(
> 
> Roger has all the details, and they're also in the list archives, but
> basically, depending on the mode, PHY *must* be initialized at a
> particular point.

Right.  Which means the core shouldn't be involved, since the OMAP PHY
initialization has to be done at a non-standard time.  (Unless we 
decide to add a flag for this special case...)

> > Given that the glue module has to be responsible for getting the PHY,
> > it should also be responsible for error checking.  So the code added to
> > hcd.c doesn't need to apply an IS_ERR check; it can simply assume that
> > if hcd->phy is NULL then either there is no software-controllable PHY
> > or else the HCD doesn't want the core to manage it.
> 
> makes sense to me, add the requirement to:
> 
> if (IS_ERR(hcd->phy))
> 	hcd->phy = NULL;

Actually, in the IS_ERR case, most glue drivers just fail the probe.  
But for any that want to continue on, we would have to add this
requirement.

Alan Stern

Re: [PATCH] USB: initialize or shutdown PHY when add or remove host controller

[Alan Stern]

On Tue, 18 Jun 2013, Felipe Balbi wrote:

> yeah, Roger brought up a big problem with OMAP's EHCI depending on the
> mode so, at least for now, we should keep phy_get and, in case of EHCI
> OMAP, phy_init in the glue :-(
> 
> Roger has all the details, and they're also in the list archives, but
> basically, depending on the mode, PHY *must* be initialized at a
> particular point.

Right.  Which means the core shouldn't be involved, since the OMAP PHY
initialization has to be done at a non-standard time.  (Unless we 
decide to add a flag for this special case...)

> > Given that the glue module has to be responsible for getting the PHY,
> > it should also be responsible for error checking.  So the code added to
> > hcd.c doesn't need to apply an IS_ERR check; it can simply assume that
> > if hcd->phy is NULL then either there is no software-controllable PHY
> > or else the HCD doesn't want the core to manage it.
> 
> makes sense to me, add the requirement to:
> 
> if (IS_ERR(hcd->phy))
> 	hcd->phy = NULL;

Actually, in the IS_ERR case, most glue drivers just fail the probe.  
But for any that want to continue on, we would have to add this
requirement.

Alan Stern