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

[PATCH V2] 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 |   24 +++++++++++++++++++++++-
 1 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index d53547d..301c639 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -40,9 +40,11 @@
 #include <linux/platform_device.h>
 #include <linux/workqueue.h>
 #include <linux/pm_runtime.h>
+#include <linux/err.h>
 
 #include <linux/usb.h>
 #include <linux/usb/hcd.h>
+#include <linux/usb/phy.h>
 
 #include "usb.h"
 
@@ -2531,12 +2533,26 @@ int usb_add_hcd(struct usb_hcd *hcd,
 	 */
 	set_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
 
+	/* In case hcd->phy contains the error code. */
+	if (IS_ERR(hcd->phy))
+		hcd->phy = NULL;
+
+	/* 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 +2624,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 +2693,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

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

[Greg KH]

On Tue, Jun 18, 2013 at 10:31:20PM -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.

Why?  What breaks if we add this patch, and what gets fixed?  I'm
guessing you can then remove code?

What out-of-tree code now works properly?  Or gets broken?

we need more info here please...

thanks,

greg k-h

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

[Chao Xie]

On Wed, Jun 19, 2013 at 10:48 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Tue, Jun 18, 2013 at 10:31:20PM -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.
>
> Why?  What breaks if we add this patch, and what gets fixed?  I'm
> guessing you can then remove code?
>
> What out-of-tree code now works properly?  Or gets broken?
>
> we need more info here please...
>
The patch does not fix any bug.
Some echi-xxx driver will need initialize the phy before it do
usb_add_hcd, and shut down phy after
do usb_remove_hcd, and i did a patch for ehci-mv.c to do above thing.
Alan and Felipe comments on my patch, and they think it is a peice of
generic code, and it can be
moved to hcd to handle it, so other ehci-xxx will not do the same thing again.
So i add the patch to add the generic code in hcd to handle phy
initialization and shut down.

> thanks,
>
> greg k-h

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

[Roger Quadros]

Hi Chao,

On 06/19/2013 05:31 AM, 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 |   24 +++++++++++++++++++++++-
>  1 files changed, 23 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index d53547d..301c639 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -40,9 +40,11 @@
>  #include <linux/platform_device.h>
>  #include <linux/workqueue.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/err.h>
>  
>  #include <linux/usb.h>
>  #include <linux/usb/hcd.h>
> +#include <linux/usb/phy.h>
>  
>  #include "usb.h"
>  
> @@ -2531,12 +2533,26 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  	 */
>  	set_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
>  
> +	/* In case hcd->phy contains the error code. */
> +	if (IS_ERR(hcd->phy))
> +		hcd->phy = NULL;
> +
> +	/* 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 +2624,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 +2693,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);
> 

I still think that we shouldn't do this because it adds more confusion and is not
still a generic enough solution.

1) It is better for the piece of code that does usb_phy_get() to do usb_phy_init/shutdown,
else there will be lot of confusion. (Felipe pointed this out earlier).

2) There is no standard way of getting phy for different controllers. It is mostly platform
dependent and it is best to leave this to the controller drivers. (Pointed out by Alan).

3) Controllers can have multiple PHYs. e.g. ehci-omap has one PHY per port and it supports
3 ports. This is also platform specific and difficult to handle generically.

4) Controllers can have specific timing requirements as to when the PHY is initialized relative
to the controller being initialized. I've pointed OMAP specific stuff in the earlier patch.

Considering all these points, I think we should leave things as they are. It isn't that hard
for controllers to manage phy_init() and phy_shutdown(), and there is not much code space saved
when compared to the complexity it creates if we move them to HCD layer.

cheers,
-roger

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

[Chao Xie]

On Wed, Jun 19, 2013 at 3:51 PM, Roger Quadros <rogerq@ti.com> wrote:
> Hi Chao,
>
> On 06/19/2013 05:31 AM, 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 |   24 +++++++++++++++++++++++-
>>  1 files changed, 23 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>> index d53547d..301c639 100644
>> --- a/drivers/usb/core/hcd.c
>> +++ b/drivers/usb/core/hcd.c
>> @@ -40,9 +40,11 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/workqueue.h>
>>  #include <linux/pm_runtime.h>
>> +#include <linux/err.h>
>>
>>  #include <linux/usb.h>
>>  #include <linux/usb/hcd.h>
>> +#include <linux/usb/phy.h>
>>
>>  #include "usb.h"
>>
>> @@ -2531,12 +2533,26 @@ int usb_add_hcd(struct usb_hcd *hcd,
>>        */
>>       set_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
>>
>> +     /* In case hcd->phy contains the error code. */
>> +     if (IS_ERR(hcd->phy))
>> +             hcd->phy = NULL;
>> +
>> +     /* 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 +2624,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 +2693,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);
>>
>
> I still think that we shouldn't do this because it adds more confusion and is not
> still a generic enough solution.
>
> 1) It is better for the piece of code that does usb_phy_get() to do usb_phy_init/shutdown,
> else there will be lot of confusion. (Felipe pointed this out earlier).
>
> 2) There is no standard way of getting phy for different controllers. It is mostly platform
> dependent and it is best to leave this to the controller drivers. (Pointed out by Alan).
>
> 3) Controllers can have multiple PHYs. e.g. ehci-omap has one PHY per port and it supports
> 3 ports. This is also platform specific and difficult to handle generically.
>
> 4) Controllers can have specific timing requirements as to when the PHY is initialized relative
> to the controller being initialized. I've pointed OMAP specific stuff in the earlier patch.
>
> Considering all these points, I think we should leave things as they are. It isn't that hard
> for controllers to manage phy_init() and phy_shutdown(), and there is not much code space saved
> when compared to the complexity it creates if we move them to HCD layer.
>
In fact, the PHY setting and handling is related to platform or SOC,
and for different SOC they can
have same EHCI HCD but they PHY handling can be different.
Omap'a case is the example, and i think some other vendors may have
silimar cases.
>From above point, It is better to leave the PHY initialization and
shutdown to be done by each echi-xxx driver.

So Alan and Felipe
What are your ideas about it?

> cheers,
> -roger

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

[Felipe Balbi]

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

Hi,

On Thu, Jun 20, 2013 at 08:53:05AM +0800, Chao Xie wrote:
> >> @@ -2674,6 +2693,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);
> >>
> >
> > I still think that we shouldn't do this because it adds more confusion and is not
> > still a generic enough solution.
> >
> > 1) It is better for the piece of code that does usb_phy_get() to do usb_phy_init/shutdown,
> > else there will be lot of confusion. (Felipe pointed this out earlier).
> >
> > 2) There is no standard way of getting phy for different controllers. It is mostly platform
> > dependent and it is best to leave this to the controller drivers. (Pointed out by Alan).
> >
> > 3) Controllers can have multiple PHYs. e.g. ehci-omap has one PHY per port and it supports
> > 3 ports. This is also platform specific and difficult to handle generically.
> >
> > 4) Controllers can have specific timing requirements as to when the PHY is initialized relative
> > to the controller being initialized. I've pointed OMAP specific stuff in the earlier patch.
> >
> > Considering all these points, I think we should leave things as they are. It isn't that hard
> > for controllers to manage phy_init() and phy_shutdown(), and there is not much code space saved
> > when compared to the complexity it creates if we move them to HCD layer.
> >
> In fact, the PHY setting and handling is related to platform or SOC,
> and for different SOC they can
> have same EHCI HCD but they PHY handling can be different.
> Omap'a case is the example, and i think some other vendors may have
> silimar cases.
> From above point, It is better to leave the PHY initialization and
> shutdown to be done by each echi-xxx driver.
> 
> So Alan and Felipe
> What are your ideas about it?

If we have so many exceptions, then sure. But eventually, the common
case should be added generically with a flag so that non-generic cases
(like OMAP) can request to handle the PHY by themselves.

Alan ?

-- 
balbi

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

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

[Alan Stern]

On Thu, 20 Jun 2013, Felipe Balbi wrote:

> > In fact, the PHY setting and handling is related to platform or SOC,
> > and for different SOC they can
> > have same EHCI HCD but they PHY handling can be different.
> > Omap'a case is the example, and i think some other vendors may have
> > silimar cases.
> > From above point, It is better to leave the PHY initialization and
> > shutdown to be done by each echi-xxx driver.
> > 
> > So Alan and Felipe
> > What are your ideas about it?
> 
> If we have so many exceptions, then sure. But eventually, the common
> case should be added generically with a flag so that non-generic cases
> (like OMAP) can request to handle the PHY by themselves.
> 
> Alan ?

I don't have very strong feelings about this; Felipe has much more
experience with these things.

However, when the common case is added into the core, the simplest way
to indicate that the HCD wants to handle the PHY(s) by itself will be
to leave hcd->phy set to NULL or an ERR_PTR value.

One important thing that hasn't been pointed out yet: When we move
these calls into the core, the same patch must also remove those calls
from the glue drivers that currently do set hcd->phy.  And it must make
sure that the glue drivers which handle the PHY by themselves do not
set hcd->phy.

Alan Stern

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

[Chao Xie]

On Fri, Jun 21, 2013 at 1:25 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, 20 Jun 2013, Felipe Balbi wrote:
>
>> > In fact, the PHY setting and handling is related to platform or SOC,
>> > and for different SOC they can
>> > have same EHCI HCD but they PHY handling can be different.
>> > Omap'a case is the example, and i think some other vendors may have
>> > silimar cases.
>> > From above point, It is better to leave the PHY initialization and
>> > shutdown to be done by each echi-xxx driver.
>> >
>> > So Alan and Felipe
>> > What are your ideas about it?
>>
>> If we have so many exceptions, then sure. But eventually, the common
>> case should be added generically with a flag so that non-generic cases
>> (like OMAP) can request to handle the PHY by themselves.
>>
>> Alan ?
>
> I don't have very strong feelings about this; Felipe has much more
> experience with these things.
>
> However, when the common case is added into the core, the simplest way
> to indicate that the HCD wants to handle the PHY(s) by itself will be
> to leave hcd->phy set to NULL or an ERR_PTR value.
>
> One important thing that hasn't been pointed out yet: When we move
> these calls into the core, the same patch must also remove those calls
> from the glue drivers that currently do set hcd->phy.  And it must make
> sure that the glue drivers which handle the PHY by themselves do not
> set hcd->phy.
>

>From device point of view, EHCI is a standlone component. It has the
standard sepcification, so each
SOC vendor has EHCI HCD need to follow the standards. Then we have
common EHCI HCD driver.
The PHY is outside of EHCI component, each SOC vendor may have
different PHY implementation. Then
we have PHY driver.
The EHCI glue driver ehci-xxx works like a SOC depended driver. It is
its duty to handle the'
relationship between the EHCI HCD driver and PHY driver.
It is same as clk, irq requested by ehci-xxx driver.

So i think add a flag and use usb_get_phy() is not very good.
It is bette to make ehci-xxx to do the phy getting and EHCI HCD
initialize it and shut down as the patch did, or let ehci-xxx to
handle the PHY as Roger said.

Based on the generic work is not too much, and does not look so
meaningful. I suggest that let to echi-xxx
do it.

> Alan Stern
>

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

[Chao Xie]

correct for irq. irq number is get by gludriver, while irq is
requested by EHCO HCD.

On Fri, Jun 21, 2013 at 9:07 AM, Chao Xie <xiechao.mail@gmail.com> wrote:
> On Fri, Jun 21, 2013 at 1:25 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> On Thu, 20 Jun 2013, Felipe Balbi wrote:
>>
>>> > In fact, the PHY setting and handling is related to platform or SOC,
>>> > and for different SOC they can
>>> > have same EHCI HCD but they PHY handling can be different.
>>> > Omap'a case is the example, and i think some other vendors may have
>>> > silimar cases.
>>> > From above point, It is better to leave the PHY initialization and
>>> > shutdown to be done by each echi-xxx driver.
>>> >
>>> > So Alan and Felipe
>>> > What are your ideas about it?
>>>
>>> If we have so many exceptions, then sure. But eventually, the common
>>> case should be added generically with a flag so that non-generic cases
>>> (like OMAP) can request to handle the PHY by themselves.
>>>
>>> Alan ?
>>
>> I don't have very strong feelings about this; Felipe has much more
>> experience with these things.
>>
>> However, when the common case is added into the core, the simplest way
>> to indicate that the HCD wants to handle the PHY(s) by itself will be
>> to leave hcd->phy set to NULL or an ERR_PTR value.
>>
>> One important thing that hasn't been pointed out yet: When we move
>> these calls into the core, the same patch must also remove those calls
>> from the glue drivers that currently do set hcd->phy.  And it must make
>> sure that the glue drivers which handle the PHY by themselves do not
>> set hcd->phy.
>>
>
> From device point of view, EHCI is a standlone component. It has the
> standard sepcification, so each
> SOC vendor has EHCI HCD need to follow the standards. Then we have
> common EHCI HCD driver.
> The PHY is outside of EHCI component, each SOC vendor may have
> different PHY implementation. Then
> we have PHY driver.
> The EHCI glue driver ehci-xxx works like a SOC depended driver. It is
> its duty to handle the'
> relationship between the EHCI HCD driver and PHY driver.
> It is same as clk, irq requested by ehci-xxx driver.
>
> So i think add a flag and use usb_get_phy() is not very good.
> It is bette to make ehci-xxx to do the phy getting and EHCI HCD
> initialize it and shut down as the patch did, or let ehci-xxx to
> handle the PHY as Roger said.
>
> Based on the generic work is not too much, and does not look so
> meaningful. I suggest that let to echi-xxx
> do it.
>
>> Alan Stern
>>

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

[Felipe Balbi]

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

Hi,

On Fri, Jun 21, 2013 at 09:07:59AM +0800, Chao Xie wrote:
> On Fri, Jun 21, 2013 at 1:25 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Thu, 20 Jun 2013, Felipe Balbi wrote:
> >
> >> > In fact, the PHY setting and handling is related to platform or SOC,
> >> > and for different SOC they can
> >> > have same EHCI HCD but they PHY handling can be different.
> >> > Omap'a case is the example, and i think some other vendors may have
> >> > silimar cases.
> >> > From above point, It is better to leave the PHY initialization and
> >> > shutdown to be done by each echi-xxx driver.
> >> >
> >> > So Alan and Felipe
> >> > What are your ideas about it?
> >>
> >> If we have so many exceptions, then sure. But eventually, the common
> >> case should be added generically with a flag so that non-generic cases
> >> (like OMAP) can request to handle the PHY by themselves.
> >>
> >> Alan ?
> >
> > I don't have very strong feelings about this; Felipe has much more
> > experience with these things.
> >
> > However, when the common case is added into the core, the simplest way
> > to indicate that the HCD wants to handle the PHY(s) by itself will be
> > to leave hcd->phy set to NULL or an ERR_PTR value.
> >
> > One important thing that hasn't been pointed out yet: When we move
> > these calls into the core, the same patch must also remove those calls
> > from the glue drivers that currently do set hcd->phy.  And it must make
> > sure that the glue drivers which handle the PHY by themselves do not
> > set hcd->phy.
> >
> 
> From device point of view, EHCI is a standlone component. It has the
> standard sepcification, so each
> SOC vendor has EHCI HCD need to follow the standards. Then we have
> common EHCI HCD driver.
> The PHY is outside of EHCI component, each SOC vendor may have
> different PHY implementation. Then
> we have PHY driver.
> The EHCI glue driver ehci-xxx works like a SOC depended driver. It is
> its duty to handle the'
> relationship between the EHCI HCD driver and PHY driver.

that's not entirely true. We build abstractions layers so that the
commonalities can be written generically. Just look at the amount of
code I removed on v3.10 merge window by moving all other UDC drivers to
use generic constructs I introduced earlier.

It just so happens that OMAP's EHCI has two different working modes
which mandates different ways to handle the PHY, one is pretty much the
generic way (power up EHCI, then power up PHY) the other is inverted
(PHY, then EHCI), that's the only reason (as of today) we're having this
thread.

> It is same as clk, irq requested by ehci-xxx driver.

clocks could be handled generically in some cases, we have pm_clk_add()
for a reason ;-)

Also, clock handling can be hidden under pm_runtime callbacks (say,
clk_enable() on ->runtime_resume(), clk_disable() on
->runtime_suspend()). IRQ is actually handled by usbcore, you just pass
a handler which, in most cases, is the normal ehci_irq() handler.

But we'll get to those later, let's focus on PHY for now.

> So i think add a flag and use usb_get_phy() is not very good.

Alan was talking about use hcd->phy as that flag, no flag would be
added. But why isn't it very good ? you didn't mention your resoning.

> It is bette to make ehci-xxx to do the phy getting and EHCI HCD
> initialize it and shut down as the patch did, or let ehci-xxx to
> handle the PHY as Roger said.

right, so this is what Alan suggested:

ehci-xxx.c does usb_get_phy() (or any of those variants) and sets the
returned pointer to hcd->phy. From that point on, ehci-hcd will play
with the phy, resuming and suspending at the proper locations, asking
the phy to enable wakeup capabilities and the like.

In fact, because of that, I was just considering if I should protect
usb_phy* against NULL pointers, just to make EHCI's life easier, I mean:

static inline int usb_phy_set_suspend(struct usb_phy *phy, int suspend)
{
	if (!phy)
		return 0;

	return phy->suspend(phy, suspend);
}

> Based on the generic work is not too much, and does not look so
> meaningful. I suggest that let to echi-xxx
> do it.

we'll end up with a boilerplate code in every single ehci-xxx doing
exactly the same thing. By building the common case in ehci-hcd, we can
make sure to focus efforts wrt power consumption, proper use of the phy
layer, etc in a single location which (almost) everybody shares.

The other bits which are non-generic, can use ehci-hcd as a reference to
build their own stuff.

my 2 cents

-- 
balbi

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

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

[Chao Xie]

On Tue, Jun 25, 2013 at 3:45 AM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Fri, Jun 21, 2013 at 09:07:59AM +0800, Chao Xie wrote:
>> On Fri, Jun 21, 2013 at 1:25 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> > On Thu, 20 Jun 2013, Felipe Balbi wrote:
>> >
>> >> > In fact, the PHY setting and handling is related to platform or SOC,
>> >> > and for different SOC they can
>> >> > have same EHCI HCD but they PHY handling can be different.
>> >> > Omap'a case is the example, and i think some other vendors may have
>> >> > silimar cases.
>> >> > From above point, It is better to leave the PHY initialization and
>> >> > shutdown to be done by each echi-xxx driver.
>> >> >
>> >> > So Alan and Felipe
>> >> > What are your ideas about it?
>> >>
>> >> If we have so many exceptions, then sure. But eventually, the common
>> >> case should be added generically with a flag so that non-generic cases
>> >> (like OMAP) can request to handle the PHY by themselves.
>> >>
>> >> Alan ?
>> >
>> > I don't have very strong feelings about this; Felipe has much more
>> > experience with these things.
>> >
>> > However, when the common case is added into the core, the simplest way
>> > to indicate that the HCD wants to handle the PHY(s) by itself will be
>> > to leave hcd->phy set to NULL or an ERR_PTR value.
>> >
>> > One important thing that hasn't been pointed out yet: When we move
>> > these calls into the core, the same patch must also remove those calls
>> > from the glue drivers that currently do set hcd->phy.  And it must make
>> > sure that the glue drivers which handle the PHY by themselves do not
>> > set hcd->phy.
>> >
>>
>> From device point of view, EHCI is a standlone component. It has the
>> standard sepcification, so each
>> SOC vendor has EHCI HCD need to follow the standards. Then we have
>> common EHCI HCD driver.
>> The PHY is outside of EHCI component, each SOC vendor may have
>> different PHY implementation. Then
>> we have PHY driver.
>> The EHCI glue driver ehci-xxx works like a SOC depended driver. It is
>> its duty to handle the'
>> relationship between the EHCI HCD driver and PHY driver.
>
> that's not entirely true. We build abstractions layers so that the
> commonalities can be written generically. Just look at the amount of
> code I removed on v3.10 merge window by moving all other UDC drivers to
> use generic constructs I introduced earlier.
>
> It just so happens that OMAP's EHCI has two different working modes
> which mandates different ways to handle the PHY, one is pretty much the
> generic way (power up EHCI, then power up PHY) the other is inverted
> (PHY, then EHCI), that's the only reason (as of today) we're having this
> thread.
>
>> It is same as clk, irq requested by ehci-xxx driver.
>
> clocks could be handled generically in some cases, we have pm_clk_add()
> for a reason ;-)
>
> Also, clock handling can be hidden under pm_runtime callbacks (say,
> clk_enable() on ->runtime_resume(), clk_disable() on
> ->runtime_suspend()). IRQ is actually handled by usbcore, you just pass
> a handler which, in most cases, is the normal ehci_irq() handler.
>
> But we'll get to those later, let's focus on PHY for now.
>
clock is another story, and i know that OMAP has full system to handle
the clock with PM runtime,
i would like to discuss it when one day you want to do it.

>> So i think add a flag and use usb_get_phy() is not very good.
>
> Alan was talking about use hcd->phy as that flag, no flag would be
> added. But why isn't it very good ? you didn't mention your resoning.
>
I maybe understand something wrong.
Using hcd->phy as a flag to indicates whether the gule driver need
EHCI HCD to help
phy operation, such as initialization and shutdown, i think it is fine.
If add another member as a flag in EHCI HCD to indicates the PHY
differences of each echi-xxx.c driver,
and handle them in EHCI HCD, i think that is not very good. Because as
you said that make
common part into EHCI HCD is the target, but this member will import
all the differences to EHCI HCD.
It is better to let the ehci-xxx.c driver to handle the differences if
it does not fit EHCI HCD's requirment
for common PHY handling just as this patch did.


>> It is bette to make ehci-xxx to do the phy getting and EHCI HCD
>> initialize it and shut down as the patch did, or let ehci-xxx to
>> handle the PHY as Roger said.
>
> right, so this is what Alan suggested:
>
> ehci-xxx.c does usb_get_phy() (or any of those variants) and sets the
> returned pointer to hcd->phy. From that point on, ehci-hcd will play
> with the phy, resuming and suspending at the proper locations, asking
> the phy to enable wakeup capabilities and the like.
>
> In fact, because of that, I was just considering if I should protect
> usb_phy* against NULL pointers, just to make EHCI's life easier, I mean:
>
> static inline int usb_phy_set_suspend(struct usb_phy *phy, int suspend)
> {
>         if (!phy)
>                 return 0;
>
>         return phy->suspend(phy, suspend);
> }
>
This patch does not include the suspending/resumeing. It is great that you are
woking at it.

>> Based on the generic work is not too much, and does not look so
>> meaningful. I suggest that let to echi-xxx
>> do it.
>
> we'll end up with a boilerplate code in every single ehci-xxx doing
> exactly the same thing. By building the common case in ehci-hcd, we can
> make sure to focus efforts wrt power consumption, proper use of the phy
> layer, etc in a single location which (almost) everybody shares.
>
> The other bits which are non-generic, can use ehci-hcd as a reference to
> build their own stuff.
>
> my 2 cents
>
OK. I understand. I am not very fimilar with PHY suspending/resuming.
I hope that i can see the patch move all PHY handling to EHCI HCD
including suspending/resuming, so
i can change our ehci driver to fit it and continuing to push the USB
patches ;-)

> --
> balbi

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

[Felipe Balbi]

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

Hi,

On Tue, Jun 25, 2013 at 09:25:30AM +0800, Chao Xie wrote:
> >> It is same as clk, irq requested by ehci-xxx driver.
> >
> > clocks could be handled generically in some cases, we have pm_clk_add()
> > for a reason ;-)
> >
> > Also, clock handling can be hidden under pm_runtime callbacks (say,
> > clk_enable() on ->runtime_resume(), clk_disable() on
> > ->runtime_suspend()). IRQ is actually handled by usbcore, you just pass
> > a handler which, in most cases, is the normal ehci_irq() handler.
> >
> > But we'll get to those later, let's focus on PHY for now.
> >
> clock is another story, and i know that OMAP has full system to handle
> the clock with PM runtime,
> i would like to discuss it when one day you want to do it.

sure, anytime.

> >> So i think add a flag and use usb_get_phy() is not very good.
> >
> > Alan was talking about use hcd->phy as that flag, no flag would be
> > added. But why isn't it very good ? you didn't mention your resoning.
> >
> I maybe understand something wrong.
> Using hcd->phy as a flag to indicates whether the gule driver need
> EHCI HCD to help
> phy operation, such as initialization and shutdown, i think it is fine.
> If add another member as a flag in EHCI HCD to indicates the PHY
> differences of each echi-xxx.c driver,
> and handle them in EHCI HCD, i think that is not very good. Because as

no argument there :-)

> you said that make
> common part into EHCI HCD is the target, but this member will import
> all the differences to EHCI HCD.

oh no, by 'flag' I meant something to tell ehci-hcd that we want to
handle PHY by ourselves, but as Alan pointed out, we don't need a
separate flag.

IOW, I didn't mean to cater for OMAP's peculiarities in the generic code
:-)

> It is better to let the ehci-xxx.c driver to handle the differences if
> it does not fit EHCI HCD's requirment
> for common PHY handling just as this patch did.

right :-)

> >> It is bette to make ehci-xxx to do the phy getting and EHCI HCD
> >> initialize it and shut down as the patch did, or let ehci-xxx to
> >> handle the PHY as Roger said.
> >
> > right, so this is what Alan suggested:
> >
> > ehci-xxx.c does usb_get_phy() (or any of those variants) and sets the
> > returned pointer to hcd->phy. From that point on, ehci-hcd will play
> > with the phy, resuming and suspending at the proper locations, asking
> > the phy to enable wakeup capabilities and the like.
> >
> > In fact, because of that, I was just considering if I should protect
> > usb_phy* against NULL pointers, just to make EHCI's life easier, I mean:
> >
> > static inline int usb_phy_set_suspend(struct usb_phy *phy, int suspend)
> > {
> >         if (!phy)
> >                 return 0;
> >
> >         return phy->suspend(phy, suspend);
> > }
> >
> This patch does not include the suspending/resumeing. It is great that you are
> woking at it.

yeah, I'll add that part so that ehci-hcd doesn't have to add if
(hcd->phy) all over the place.

> >> Based on the generic work is not too much, and does not look so
> >> meaningful. I suggest that let to echi-xxx
> >> do it.
> >
> > we'll end up with a boilerplate code in every single ehci-xxx doing
> > exactly the same thing. By building the common case in ehci-hcd, we can
> > make sure to focus efforts wrt power consumption, proper use of the phy
> > layer, etc in a single location which (almost) everybody shares.
> >
> > The other bits which are non-generic, can use ehci-hcd as a reference to
> > build their own stuff.
> >
> > my 2 cents
> >
> OK. I understand. I am not very fimilar with PHY suspending/resuming.
> I hope that i can see the patch move all PHY handling to EHCI HCD
> including suspending/resuming, so
> i can change our ehci driver to fit it and continuing to push the USB
> patches ;-)

suspend/resume is usually very tricky, so I'd rather leave it for later.

For now, let's just build enough ground-work as to make it easier to
think about suspend/resume later :-)

Meaning that we can just add the bare minimum (init on probe and
shutdown on remove) and add more support as we go :-)

cheers

-- 
balbi

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

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

[Felipe Balbi]

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

On Thu, Jun 20, 2013 at 01:25:31PM -0400, Alan Stern wrote:
> On Thu, 20 Jun 2013, Felipe Balbi wrote:
> 
> > > In fact, the PHY setting and handling is related to platform or SOC,
> > > and for different SOC they can
> > > have same EHCI HCD but they PHY handling can be different.
> > > Omap'a case is the example, and i think some other vendors may have
> > > silimar cases.
> > > From above point, It is better to leave the PHY initialization and
> > > shutdown to be done by each echi-xxx driver.
> > > 
> > > So Alan and Felipe
> > > What are your ideas about it?
> > 
> > If we have so many exceptions, then sure. But eventually, the common
> > case should be added generically with a flag so that non-generic cases
> > (like OMAP) can request to handle the PHY by themselves.
> > 
> > Alan ?
> 
> I don't have very strong feelings about this; Felipe has much more
> experience with these things.
> 
> However, when the common case is added into the core, the simplest way
> to indicate that the HCD wants to handle the PHY(s) by itself will be
> to leave hcd->phy set to NULL or an ERR_PTR value.
> 
> One important thing that hasn't been pointed out yet: When we move
> these calls into the core, the same patch must also remove those calls
> from the glue drivers that currently do set hcd->phy.  And it must make
> sure that the glue drivers which handle the PHY by themselves do not
> set hcd->phy.

perfect summary. Perhaps Roger could already work on private PHY handle
for ehci-omap.c and later we can start moving generic case to usbcore
without having to touch ehci-omap.c at all. Roger, any commetns ?

-- 
balbi

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

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

[Roger Quadros]

On 06/24/2013 10:36 PM, Felipe Balbi wrote:
> On Thu, Jun 20, 2013 at 01:25:31PM -0400, Alan Stern wrote:
>> On Thu, 20 Jun 2013, Felipe Balbi wrote:
>>
>>>> In fact, the PHY setting and handling is related to platform or SOC,
>>>> and for different SOC they can
>>>> have same EHCI HCD but they PHY handling can be different.
>>>> Omap'a case is the example, and i think some other vendors may have
>>>> silimar cases.
>>>> From above point, It is better to leave the PHY initialization and
>>>> shutdown to be done by each echi-xxx driver.
>>>>
>>>> So Alan and Felipe
>>>> What are your ideas about it?
>>>
>>> If we have so many exceptions, then sure. But eventually, the common
>>> case should be added generically with a flag so that non-generic cases
>>> (like OMAP) can request to handle the PHY by themselves.
>>>
>>> Alan ?
>>
>> I don't have very strong feelings about this; Felipe has much more
>> experience with these things.
>>
>> However, when the common case is added into the core, the simplest way
>> to indicate that the HCD wants to handle the PHY(s) by itself will be
>> to leave hcd->phy set to NULL or an ERR_PTR value.
>>
>> One important thing that hasn't been pointed out yet: When we move
>> these calls into the core, the same patch must also remove those calls
>> from the glue drivers that currently do set hcd->phy.  And it must make
>> sure that the glue drivers which handle the PHY by themselves do not
>> set hcd->phy.
> 
> perfect summary. Perhaps Roger could already work on private PHY handle
> for ehci-omap.c and later we can start moving generic case to usbcore
> without having to touch ehci-omap.c at all. Roger, any commetns ?
> 

This looks fine to me. I don't have anything to add.

cheers,
-roger

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

[Felipe Balbi]

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

On Tue, Jun 25, 2013 at 04:37:11PM +0300, Roger Quadros wrote:
> On 06/24/2013 10:36 PM, Felipe Balbi wrote:
> > On Thu, Jun 20, 2013 at 01:25:31PM -0400, Alan Stern wrote:
> >> On Thu, 20 Jun 2013, Felipe Balbi wrote:
> >>
> >>>> In fact, the PHY setting and handling is related to platform or SOC,
> >>>> and for different SOC they can
> >>>> have same EHCI HCD but they PHY handling can be different.
> >>>> Omap'a case is the example, and i think some other vendors may have
> >>>> silimar cases.
> >>>> From above point, It is better to leave the PHY initialization and
> >>>> shutdown to be done by each echi-xxx driver.
> >>>>
> >>>> So Alan and Felipe
> >>>> What are your ideas about it?
> >>>
> >>> If we have so many exceptions, then sure. But eventually, the common
> >>> case should be added generically with a flag so that non-generic cases
> >>> (like OMAP) can request to handle the PHY by themselves.
> >>>
> >>> Alan ?
> >>
> >> I don't have very strong feelings about this; Felipe has much more
> >> experience with these things.
> >>
> >> However, when the common case is added into the core, the simplest way
> >> to indicate that the HCD wants to handle the PHY(s) by itself will be
> >> to leave hcd->phy set to NULL or an ERR_PTR value.
> >>
> >> One important thing that hasn't been pointed out yet: When we move
> >> these calls into the core, the same patch must also remove those calls
> >> from the glue drivers that currently do set hcd->phy.  And it must make
> >> sure that the glue drivers which handle the PHY by themselves do not
> >> set hcd->phy.
> > 
> > perfect summary. Perhaps Roger could already work on private PHY handle
> > for ehci-omap.c and later we can start moving generic case to usbcore
> > without having to touch ehci-omap.c at all. Roger, any commetns ?
> > 
> 
> This looks fine to me. I don't have anything to add.

thanks :-)

-- 
balbi

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