diff mbox

[2/4] usb: host: xhci-plat: Add support to get PHYs

Message ID 1402056736-12674-3-git-send-email-gautam.vivek@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Vivek Gautam June 6, 2014, 12:12 p.m. UTC
The host controller by itself may sometimes need to handle PHY
and/or calibrate some of the PHY settings to get full support out
of the PHY controller.
The PHY core provides a calibration funtionality now to do so.
Therefore, facilitate getting the two possible PHY types for
xhci controller, viz. USB 2.0 type (UTMI+) and USB 3.0 type (PIPE3)
from its parent.

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
---
 drivers/usb/host/xhci-plat.c |   24 ++++++++++++++++++++++++
 drivers/usb/host/xhci.h      |    3 +++
 2 files changed, 27 insertions(+)

Comments

Julius Werner June 9, 2014, 8:22 p.m. UTC | #1
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 9ffecd5..453d89e 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1582,6 +1582,9 @@ struct xhci_hcd {
>         u32                     port_status_u0;
>  /* Compliance Mode Timer Triggered every 2 seconds */
>  #define COMP_MODE_RCVRY_MSECS 2000
> +       /* phys for the controller */
> +       struct phy              *phy2_gen;
> +       struct phy              *phy3_gen;
>  };

I don't think adding new variables here and restricting most of this
logic to xhci-plat.c (in the next patch) is the best way to do it.
There's no conceptual reason why other host controllers (e.g. xhci-pci
or even EHCI) could not have a similar need to tune their PHY after
reset. PHYs are universal to all host controllers.

There is already a 'phy' member in struct usb_hcd which I think is
mostly unused right now. I think it would be much less
confusing/redundant to reuse that member for this purpose (you could
still set it up from xhci_plat_probe(), and then call it from
hcd_bus_resume() or something like that). Since XHCI host controllers
already conveniently have two struct usb_hcd (one for 2.0 and one for
3.0), you can cleanly store references to your two PHYs in there.
Vivek Gautam June 24, 2014, 6:10 a.m. UTC | #2
Hi,


On Tue, Jun 10, 2014 at 1:52 AM, Julius Werner <jwerner@chromium.org> wrote:
>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>> index 9ffecd5..453d89e 100644
>> --- a/drivers/usb/host/xhci.h
>> +++ b/drivers/usb/host/xhci.h
>> @@ -1582,6 +1582,9 @@ struct xhci_hcd {
>>         u32                     port_status_u0;
>>  /* Compliance Mode Timer Triggered every 2 seconds */
>>  #define COMP_MODE_RCVRY_MSECS 2000
>> +       /* phys for the controller */
>> +       struct phy              *phy2_gen;
>> +       struct phy              *phy3_gen;
>>  };
>
> I don't think adding new variables here and restricting most of this
> logic to xhci-plat.c (in the next patch) is the best way to do it.
> There's no conceptual reason why other host controllers (e.g. xhci-pci
> or even EHCI) could not have a similar need to tune their PHY after
> reset. PHYs are universal to all host controllers.
>
> There is already a 'phy' member in struct usb_hcd which I think is
> mostly unused right now. I think it would be much less
> confusing/redundant to reuse that member for this purpose (you could
> still set it up from xhci_plat_probe(), and then call it from
> hcd_bus_resume() or something like that). Since XHCI host controllers
> already conveniently have two struct usb_hcd (one for 2.0 and one for
> 3.0), you can cleanly store references to your two PHYs in there.

Ok, i will look into the suggested approach, and raise flag in case i
have doubts.
Sergei Shtylyov June 24, 2014, 10:34 p.m. UTC | #3
Hello.

On 06/10/2014 12:22 AM, Julius Werner wrote:

>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>> index 9ffecd5..453d89e 100644
>> --- a/drivers/usb/host/xhci.h
>> +++ b/drivers/usb/host/xhci.h
>> @@ -1582,6 +1582,9 @@ struct xhci_hcd {
>>          u32                     port_status_u0;
>>   /* Compliance Mode Timer Triggered every 2 seconds */
>>   #define COMP_MODE_RCVRY_MSECS 2000
>> +       /* phys for the controller */
>> +       struct phy              *phy2_gen;
>> +       struct phy              *phy3_gen;
>>   };

> I don't think adding new variables here and restricting most of this
> logic to xhci-plat.c (in the next patch) is the best way to do it.

    Indeed.

> There's no conceptual reason why other host controllers (e.g. xhci-pci
> or even EHCI) could not have a similar need to tune their PHY after
> reset. PHYs are universal to all host controllers.

> There is already a 'phy' member in struct usb_hcd which I think is
> mostly unused right now. I think it would be much less
> confusing/redundant to reuse that member for this purpose (you could
> still set it up from xhci_plat_probe(), and then call it from
> hcd_bus_resume() or something like that).

    That member has type 'struct usb_phy *' while here we have 'struct phy *' 
-- feel the difference.
    I have already tried adding 'struct phy *gen_phy' to 'struct usb_hcd', 
however Greg wasn't eager to pick that up so far. Here's the last posting of 
my patch:

http://marc.info/?l=linux-usb&m=140145917506582

WBR, Sergei
Vivek Gautam June 25, 2014, 5:49 a.m. UTC | #4
HI Sergei,


On Wed, Jun 25, 2014 at 4:04 AM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> Hello.
>
>
> On 06/10/2014 12:22 AM, Julius Werner wrote:
>
>>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>>> index 9ffecd5..453d89e 100644
>>> --- a/drivers/usb/host/xhci.h
>>> +++ b/drivers/usb/host/xhci.h
>>> @@ -1582,6 +1582,9 @@ struct xhci_hcd {
>>>          u32                     port_status_u0;
>>>   /* Compliance Mode Timer Triggered every 2 seconds */
>>>   #define COMP_MODE_RCVRY_MSECS 2000
>>> +       /* phys for the controller */
>>> +       struct phy              *phy2_gen;
>>> +       struct phy              *phy3_gen;
>>>   };
>
>
>> I don't think adding new variables here and restricting most of this
>> logic to xhci-plat.c (in the next patch) is the best way to do it.
>
>
>    Indeed.
>
>
>> There's no conceptual reason why other host controllers (e.g. xhci-pci
>> or even EHCI) could not have a similar need to tune their PHY after
>> reset. PHYs are universal to all host controllers.
>
>
>> There is already a 'phy' member in struct usb_hcd which I think is
>> mostly unused right now. I think it would be much less
>> confusing/redundant to reuse that member for this purpose (you could
>> still set it up from xhci_plat_probe(), and then call it from
>> hcd_bus_resume() or something like that).
>
>
>    That member has type 'struct usb_phy *' while here we have 'struct phy *'
> -- feel the difference.
>    I have already tried adding 'struct phy *gen_phy' to 'struct usb_hcd',
> however Greg wasn't eager to pick that up so far. Here's the last posting of
> my patch:
>
> http://marc.info/?l=linux-usb&m=140145917506582

Thanks for pointing to the patch, i have also been tracking this patch.
I will try to rework using this patch and post the relevant patches.
Vivek Gautam June 25, 2014, 8:44 a.m. UTC | #5
Hi,


On Wed, Jun 25, 2014 at 4:04 AM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> Hello.
>
>
> On 06/10/2014 12:22 AM, Julius Werner wrote:
>
>>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>>> index 9ffecd5..453d89e 100644
>>> --- a/drivers/usb/host/xhci.h
>>> +++ b/drivers/usb/host/xhci.h
>>> @@ -1582,6 +1582,9 @@ struct xhci_hcd {
>>>          u32                     port_status_u0;
>>>   /* Compliance Mode Timer Triggered every 2 seconds */
>>>   #define COMP_MODE_RCVRY_MSECS 2000
>>> +       /* phys for the controller */
>>> +       struct phy              *phy2_gen;
>>> +       struct phy              *phy3_gen;
>>>   };
>
>
>> I don't think adding new variables here and restricting most of this
>> logic to xhci-plat.c (in the next patch) is the best way to do it.
>
>
>    Indeed.
>
>
>> There's no conceptual reason why other host controllers (e.g. xhci-pci
>> or even EHCI) could not have a similar need to tune their PHY after
>> reset. PHYs are universal to all host controllers.
>
>
>> There is already a 'phy' member in struct usb_hcd which I think is
>> mostly unused right now. I think it would be much less
>> confusing/redundant to reuse that member for this purpose (you could
>> still set it up from xhci_plat_probe(), and then call it from
>> hcd_bus_resume() or something like that).
>
>
>    That member has type 'struct usb_phy *' while here we have 'struct phy *'
> -- feel the difference.
>    I have already tried adding 'struct phy *gen_phy' to 'struct usb_hcd',

So the 'struct phy *' available in the usb_hcd is requested in usb_add_hcd().
This is requested with the constant string 'usb' :
           struct phy *phy = phy_get(hcd->self.controller, "usb");

This can get the phy with string 'usb' only if, either the host
controller has a device node wherein the phys are given.
Even in this case one can't give same constant string for two
different phys, UTMI+ and PIPE3 phy, isn't it ?

Or, the other way can be when host gets a lookup table to look into to
find the relevant phys, something like
Heikki has suggested:
usb: dwc3: host: convey the PHYs to xhci
(https://lkml.org/lkml/2014/6/5/585) and related patch series.

So if we use this second approach, we would need to override the
'phy_get()' that has been done in usb_add_hcd()
in xhci_plat_probe(), and then use them in later operations.

am i getting the things correctly ?
Sergei Shtylyov July 3, 2014, 10:39 p.m. UTC | #6
Hello.

On 06/25/2014 12:44 PM, Vivek Gautam wrote:

>>>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>>>> index 9ffecd5..453d89e 100644
>>>> --- a/drivers/usb/host/xhci.h
>>>> +++ b/drivers/usb/host/xhci.h
>>>> @@ -1582,6 +1582,9 @@ struct xhci_hcd {
>>>>           u32                     port_status_u0;
>>>>    /* Compliance Mode Timer Triggered every 2 seconds */
>>>>    #define COMP_MODE_RCVRY_MSECS 2000
>>>> +       /* phys for the controller */
>>>> +       struct phy              *phy2_gen;
>>>> +       struct phy              *phy3_gen;
>>>>    };

>>> I don't think adding new variables here and restricting most of this
>>> logic to xhci-plat.c (in the next patch) is the best way to do it.

>>     Indeed.

    Actually, I haven't given enough thought to this...

>>> There's no conceptual reason why other host controllers (e.g. xhci-pci
>>> or even EHCI) could not have a similar need to tune their PHY after
>>> reset. PHYs are universal to all host controllers.

>>> There is already a 'phy' member in struct usb_hcd which I think is
>>> mostly unused right now. I think it would be much less
>>> confusing/redundant to reuse that member for this purpose (you could
>>> still set it up from xhci_plat_probe(), and then call it from
>>> hcd_bus_resume() or something like that).

>>     That member has type 'struct usb_phy *' while here we have 'struct phy *'
>> -- feel the difference.
>>     I have already tried adding 'struct phy *gen_phy' to 'struct usb_hcd',

> So the 'struct phy *' available in the usb_hcd is requested in usb_add_hcd().
> This is requested with the constant string 'usb' :
>             struct phy *phy = phy_get(hcd->self.controller, "usb");

> This can get the phy with string 'usb' only if, either the host
> controller has a device node wherein the phys are given.

    Yes, that was the plan. Currently, the PHY driver for which this was 
written can be probed from the device tree only.

> Even in this case one can't give same constant string for two
> different phys, UTMI+ and PIPE3 phy, isn't it ?

    Yes, you're right. I didn't really consider the case of some host needing 
2 distinct PHY.

> Or, the other way can be when host gets a lookup table to look into to
> find the relevant phys, something like
> Heikki has suggested:
> usb: dwc3: host: convey the PHYs to xhci
> (https://lkml.org/lkml/2014/6/5/585) and related patch series.

    Well, AFAIK the look-up table method is already provided by the current 
PHY core for non-DT use cases; this doesn't seem to have changed with that 
patch set, does it?

> So if we use this second approach, we would need to override the
> 'phy_get()' that has been done in usb_add_hcd()
> in xhci_plat_probe(), and then use them in later operations.

    I guess it'd probably be simpler to ignore the 'gen_phy' member of 'struct 
usb_hcd' in your case (if it gets eventually added).

WBR, Sergei
diff mbox

Patch

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 29d8adb..e7145b5 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -17,6 +17,7 @@ 
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
+#include <linux/phy/phy.h>
 
 #include "xhci.h"
 #include "xhci-mvebu.h"
@@ -183,6 +184,29 @@  static int xhci_plat_probe(struct platform_device *pdev)
 	}
 
 	/*
+	 * Get possile USB 2.0 type PHY (UTMI+), as well as
+	 * USB 3.0 type PHY (PIPE3).
+	 * The host controller may need to handle PHYs by itself too
+	 * sometimes, so as to calibrate it based on the need.
+	 */
+	xhci->phy2_gen = devm_phy_get(&pdev->dev, "usb2-phy");
+	if (IS_ERR(xhci->phy2_gen)) {
+		ret = PTR_ERR(xhci->phy2_gen);
+		if (ret != -ENOSYS && ret != -ENODEV) {
+			dev_err(&pdev->dev, "no usb2 phy configured\n");
+			return ret;
+		}
+	}
+	xhci->phy3_gen = devm_phy_get(&pdev->dev, "usb3-phy");
+	if (IS_ERR(xhci->phy3_gen)) {
+		ret = PTR_ERR(xhci->phy3_gen);
+		if (ret != -ENOSYS && ret != -ENODEV) {
+			dev_err(&pdev->dev, "no usb3 phy configured\n");
+			return ret;
+		}
+	}
+
+	/*
 	 * Set the xHCI pointer before xhci_plat_setup() (aka hcd_driver.reset)
 	 * is called by usb_add_hcd().
 	 */
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 9ffecd5..453d89e 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1582,6 +1582,9 @@  struct xhci_hcd {
 	u32			port_status_u0;
 /* Compliance Mode Timer Triggered every 2 seconds */
 #define COMP_MODE_RCVRY_MSECS 2000
+	/* phys for the controller */
+	struct phy		*phy2_gen;
+	struct phy		*phy3_gen;
 };
 
 /* convert between an HCD pointer and the corresponding EHCI_HCD */