| Message ID | 1402056736-12674-3-git-send-email-gautam.vivek@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> 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.
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.
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
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.
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 ?
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 --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 */
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(+)