| Message ID | 1417783941-2418-4-git-send-email-lyz@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> From: Yunzhi Li [mailto:lyz@rock-chips.com] > Sent: Friday, December 05, 2014 4:52 AM > > Get PHY parameters from devicetree and power off usb PHY during > system suspend. > > Signed-off-by: Yunzhi Li <lyz@rock-chips.com> > --- > > drivers/usb/dwc2/gadget.c | 33 ++++++++++++--------------------- > drivers/usb/dwc2/platform.c | 34 ++++++++++++++++++++++++++++++++++ > 2 files changed, 46 insertions(+), 21 deletions(-) > > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > index 200168e..2601c61 100644 > --- a/drivers/usb/dwc2/gadget.c > +++ b/drivers/usb/dwc2/gadget.c > @@ -3410,8 +3410,6 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq) > { > struct device *dev = hsotg->dev; > struct s3c_hsotg_plat *plat = dev->platform_data; > - struct phy *phy; > - struct usb_phy *uphy; > struct s3c_hsotg_ep *eps; > int epnum; > int ret; > @@ -3421,30 +3419,23 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq) > hsotg->phyif = GUSBCFG_PHYIF16; > > /* > - * Attempt to find a generic PHY, then look for an old style > - * USB PHY, finally fall back to pdata > + * If platform probe couldn't find a generic PHY or an old style > + * USB PHY, fall back to pdata > */ > - phy = devm_phy_get(dev, "usb2-phy"); > - if (IS_ERR(phy)) { > - uphy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2); > - if (IS_ERR(uphy)) { > - /* Fallback for pdata */ > - plat = dev_get_platdata(dev); > - if (!plat) { > - dev_err(dev, > - "no platform data or transceiver defined\n"); > - return -EPROBE_DEFER; > - } > - hsotg->plat = plat; > - } else > - hsotg->uphy = uphy; > - } else { > - hsotg->phy = phy; > + if (IS_ERR_OR_NULL(hsotg->phy) && IS_ERR_OR_NULL(hsotg->uphy)) { > + plat = dev_get_platdata(dev); > + if (!plat) { > + dev_err(dev, > + "no platform data or transceiver defined\n"); > + return -EPROBE_DEFER; > + } > + hsotg->plat = plat; > + } else if (hsotg->phy) { You have changed the behavior here. Previously, the driver would work even if there were no phys or pdata defined. Now it will return -EPROBE_DEFER instead. Are you sure that won't break any existing platforms? > /* > * If using the generic PHY framework, check if the PHY bus > * width is 8-bit and set the phyif appropriately. > */ > - if (phy_get_bus_width(phy) == 8) > + if (phy_get_bus_width(hsotg->phy) == 8) > hsotg->phyif = GUSBCFG_PHYIF8; > } > > diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c > index 6a795aa..739d14f 100644 > --- a/drivers/usb/dwc2/platform.c > +++ b/drivers/usb/dwc2/platform.c > @@ -155,6 +155,8 @@ static int dwc2_driver_probe(struct platform_device *dev) > struct dwc2_core_params defparams; > struct dwc2_hsotg *hsotg; > struct resource *res; > + struct phy *phy; > + struct usb_phy *uphy; > int retval; > int irq; > > @@ -212,6 +214,24 @@ static int dwc2_driver_probe(struct platform_device *dev) > > hsotg->dr_mode = of_usb_get_dr_mode(dev->dev.of_node); > > + /* > + * Attempt to find a generic PHY, then look for an old style > + * USB PHY > + */ > + phy = devm_phy_get(&dev->dev, "usb2-phy"); > + if (IS_ERR(phy)) { > + hsotg->phy = NULL; > + uphy = devm_usb_get_phy(&dev->dev, USB_PHY_TYPE_USB2); > + if (IS_ERR(uphy)) > + hsotg->uphy = NULL; > + else > + hsotg->uphy = uphy; > + } else { > + hsotg->phy = phy; > + phy_power_on(hsotg->phy); > + phy_init(hsotg->phy); > + } > + > spin_lock_init(&hsotg->lock); > mutex_init(&hsotg->init_mutex); > retval = dwc2_gadget_init(hsotg, irq); > @@ -233,6 +253,14 @@ static int __maybe_unused dwc2_suspend(struct device *dev) > > if (dwc2_is_device_mode(dwc2)) > ret = s3c_hsotg_suspend(dwc2); > + else { Kernel style says that both branches of the if() should have braces here. > + if (dwc2->lx_state == DWC2_L0) > + return 0; > + if (dwc2->phy) { > + phy_exit(dwc2->phy); > + phy_power_off(dwc2->phy); > + } Minor nit: no need to test dwc2->phy for NULL here, the phy functions handle a NULL pointer just fine. > + } > return ret; > } > > @@ -243,6 +271,12 @@ static int __maybe_unused dwc2_resume(struct device *dev) > > if (dwc2_is_device_mode(dwc2)) > ret = s3c_hsotg_resume(dwc2); > + else { Braces. > + if (dwc2->phy) { > + phy_power_on(dwc2->phy); > + phy_init(dwc2->phy); > + } > + } > return ret; > } >
Hi Paul: Thank you for replying. On 2014/12/6 3:04, Paul Zimmerman wrote: >> From: Yunzhi Li [mailto:lyz@rock-chips.com] >> Sent: Friday, December 05, 2014 4:52 AM >> >> Get PHY parameters from devicetree and power off usb PHY during >> system suspend. >> >> Signed-off-by: Yunzhi Li <lyz@rock-chips.com> >> --- >> >> drivers/usb/dwc2/gadget.c | 33 ++++++++++++--------------------- >> drivers/usb/dwc2/platform.c | 34 ++++++++++++++++++++++++++++++++++ >> 2 files changed, 46 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c >> index 200168e..2601c61 100644 >> --- a/drivers/usb/dwc2/gadget.c >> +++ b/drivers/usb/dwc2/gadget.c >> [...] >> >> /* >> - * Attempt to find a generic PHY, then look for an old style >> - * USB PHY, finally fall back to pdata >> + * If platform probe couldn't find a generic PHY or an old style >> + * USB PHY, fall back to pdata >> */ >> - phy = devm_phy_get(dev, "usb2-phy"); >> - if (IS_ERR(phy)) { >> - uphy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2); >> - if (IS_ERR(uphy)) { >> - /* Fallback for pdata */ >> - plat = dev_get_platdata(dev); >> - if (!plat) { >> - dev_err(dev, >> - "no platform data or transceiver defined\n"); >> - return -EPROBE_DEFER; >> - } >> - hsotg->plat = plat; >> - } else >> - hsotg->uphy = uphy; >> - } else { >> - hsotg->phy = phy; >> + if (IS_ERR_OR_NULL(hsotg->phy) && IS_ERR_OR_NULL(hsotg->uphy)) { >> + plat = dev_get_platdata(dev); >> + if (!plat) { >> + dev_err(dev, >> + "no platform data or transceiver defined\n"); >> + return -EPROBE_DEFER; >> + } >> + hsotg->plat = plat; >> + } else if (hsotg->phy) { > You have changed the behavior here. Previously, the driver would work > even if there were no phys or pdata defined. Now it will return > -EPROBE_DEFER instead. Are you sure that won't break any existing > platforms? > I don't really catch your meaning. Could you please point out where is the difference? Thanks . In previous version, if there were no phy or usb_phy defined, devm_phy_get() and devm_usb_get_phy() both failed, then driver falls back to find pdata, if no pdata defined dev_get_platdata() returns NULL, then dwc2_gadget_init returns -EPROBE_DEFER and the dwc2_driver_probe fail. So I think, for the dwc2 driver without my patch, when CONFIG_USB_DWC2_PERIPHERAL or CONFIG_USB_DWC2_DUAL_ROLE selected, if there were no phys or pdata defined, the driver probe will also return -EPROBE_DEFER. And when only CONFIG_USB_DWC2_HOST selected, the previous version driver didn't care any thing about phy, in my patch it will try to find a phy when platform driver probe, but do not fail probe if there isn't a phy node.
> From: Yunzhi Li [mailto:lyz@rock-chips.com] > Sent: Sunday, December 07, 2014 7:58 PM > > On 2014/12/6 3:04, Paul Zimmerman wrote: > >> From: Yunzhi Li [mailto:lyz@rock-chips.com] > >> Sent: Friday, December 05, 2014 4:52 AM > >> > >> Get PHY parameters from devicetree and power off usb PHY during > >> system suspend. > >> > >> Signed-off-by: Yunzhi Li <lyz@rock-chips.com> > >> --- > >> > >> drivers/usb/dwc2/gadget.c | 33 ++++++++++++--------------------- > >> drivers/usb/dwc2/platform.c | 34 ++++++++++++++++++++++++++++++++++ > >> 2 files changed, 46 insertions(+), 21 deletions(-) > >> > >> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > >> index 200168e..2601c61 100644 > >> --- a/drivers/usb/dwc2/gadget.c > >> +++ b/drivers/usb/dwc2/gadget.c > >> [...] > >> > >> /* > >> - * Attempt to find a generic PHY, then look for an old style > >> - * USB PHY, finally fall back to pdata > >> + * If platform probe couldn't find a generic PHY or an old style > >> + * USB PHY, fall back to pdata > >> */ > >> - phy = devm_phy_get(dev, "usb2-phy"); > >> - if (IS_ERR(phy)) { > >> - uphy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2); > >> - if (IS_ERR(uphy)) { > >> - /* Fallback for pdata */ > >> - plat = dev_get_platdata(dev); > >> - if (!plat) { > >> - dev_err(dev, > >> - "no platform data or transceiver defined\n"); > >> - return -EPROBE_DEFER; > >> - } > >> - hsotg->plat = plat; > >> - } else > >> - hsotg->uphy = uphy; > >> - } else { > >> - hsotg->phy = phy; > >> + if (IS_ERR_OR_NULL(hsotg->phy) && IS_ERR_OR_NULL(hsotg->uphy)) { > >> + plat = dev_get_platdata(dev); > >> + if (!plat) { > >> + dev_err(dev, > >> + "no platform data or transceiver defined\n"); > >> + return -EPROBE_DEFER; > >> + } > >> + hsotg->plat = plat; > >> + } else if (hsotg->phy) { > > You have changed the behavior here. Previously, the driver would work > > even if there were no phys or pdata defined. Now it will return > > -EPROBE_DEFER instead. Are you sure that won't break any existing > > platforms? > > > I don't really catch your meaning. Could you please point out where is > the difference? Thanks . Yeah, sorry, I misread the patch. I think your new version is fine.
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index 200168e..2601c61 100644 --- a/drivers/usb/dwc2/gadget.c +++ b/drivers/usb/dwc2/gadget.c @@ -3410,8 +3410,6 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq) { struct device *dev = hsotg->dev; struct s3c_hsotg_plat *plat = dev->platform_data; - struct phy *phy; - struct usb_phy *uphy; struct s3c_hsotg_ep *eps; int epnum; int ret; @@ -3421,30 +3419,23 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq) hsotg->phyif = GUSBCFG_PHYIF16; /* - * Attempt to find a generic PHY, then look for an old style - * USB PHY, finally fall back to pdata + * If platform probe couldn't find a generic PHY or an old style + * USB PHY, fall back to pdata */ - phy = devm_phy_get(dev, "usb2-phy"); - if (IS_ERR(phy)) { - uphy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2); - if (IS_ERR(uphy)) { - /* Fallback for pdata */ - plat = dev_get_platdata(dev); - if (!plat) { - dev_err(dev, - "no platform data or transceiver defined\n"); - return -EPROBE_DEFER; - } - hsotg->plat = plat; - } else - hsotg->uphy = uphy; - } else { - hsotg->phy = phy; + if (IS_ERR_OR_NULL(hsotg->phy) && IS_ERR_OR_NULL(hsotg->uphy)) { + plat = dev_get_platdata(dev); + if (!plat) { + dev_err(dev, + "no platform data or transceiver defined\n"); + return -EPROBE_DEFER; + } + hsotg->plat = plat; + } else if (hsotg->phy) { /* * If using the generic PHY framework, check if the PHY bus * width is 8-bit and set the phyif appropriately. */ - if (phy_get_bus_width(phy) == 8) + if (phy_get_bus_width(hsotg->phy) == 8) hsotg->phyif = GUSBCFG_PHYIF8; } diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c index 6a795aa..739d14f 100644 --- a/drivers/usb/dwc2/platform.c +++ b/drivers/usb/dwc2/platform.c @@ -155,6 +155,8 @@ static int dwc2_driver_probe(struct platform_device *dev) struct dwc2_core_params defparams; struct dwc2_hsotg *hsotg; struct resource *res; + struct phy *phy; + struct usb_phy *uphy; int retval; int irq; @@ -212,6 +214,24 @@ static int dwc2_driver_probe(struct platform_device *dev) hsotg->dr_mode = of_usb_get_dr_mode(dev->dev.of_node); + /* + * Attempt to find a generic PHY, then look for an old style + * USB PHY + */ + phy = devm_phy_get(&dev->dev, "usb2-phy"); + if (IS_ERR(phy)) { + hsotg->phy = NULL; + uphy = devm_usb_get_phy(&dev->dev, USB_PHY_TYPE_USB2); + if (IS_ERR(uphy)) + hsotg->uphy = NULL; + else + hsotg->uphy = uphy; + } else { + hsotg->phy = phy; + phy_power_on(hsotg->phy); + phy_init(hsotg->phy); + } + spin_lock_init(&hsotg->lock); mutex_init(&hsotg->init_mutex); retval = dwc2_gadget_init(hsotg, irq); @@ -233,6 +253,14 @@ static int __maybe_unused dwc2_suspend(struct device *dev) if (dwc2_is_device_mode(dwc2)) ret = s3c_hsotg_suspend(dwc2); + else { + if (dwc2->lx_state == DWC2_L0) + return 0; + if (dwc2->phy) { + phy_exit(dwc2->phy); + phy_power_off(dwc2->phy); + } + } return ret; } @@ -243,6 +271,12 @@ static int __maybe_unused dwc2_resume(struct device *dev) if (dwc2_is_device_mode(dwc2)) ret = s3c_hsotg_resume(dwc2); + else { + if (dwc2->phy) { + phy_power_on(dwc2->phy); + phy_init(dwc2->phy); + } + } return ret; }
Get PHY parameters from devicetree and power off usb PHY during system suspend. Signed-off-by: Yunzhi Li <lyz@rock-chips.com> --- drivers/usb/dwc2/gadget.c | 33 ++++++++++++--------------------- drivers/usb/dwc2/platform.c | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 21 deletions(-)