From mboxrd@z Thu Jan 1 00:00:00 1970 From: Florian Fainelli Subject: Re: [PATCH V3 4/6] usb: ohci-platform: Add support for Broadcom STB SoC's Date: Tue, 6 Nov 2018 15:44:21 -0800 Message-ID: <674c3275-de47-57f4-84aa-58b318aef67b@gmail.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: Al Cooper , Alan Stern , Al Cooper Cc: linux-kernel@vger.kernel.org, Alban Bedel , Alex Elder , Andrew Morton , Arnd Bergmann , Avi Fishman , bcm-kernel-feedback-list@broadcom.com, Bjorn Andersson , Chunfeng Yun , "David S. Miller" , devicetree@vger.kernel.org, Dmitry Osipenko , Greg Kroah-Hartman , "Gustavo A. R. Silva" , Hans de Goede , James Hogan , Jianguo Sun , Johan Hovold , Kees Cook , linux-usb@vger.kernel.org, Lu Baolu List-Id: devicetree@vger.kernel.org On 11/6/18 1:40 PM, Al Cooper wrote: > On 11/6/18 11:08 AM, Alan Stern wrote: >> On Mon, 5 Nov 2018, Al Cooper wrote: >> >>> Add support for Broadcom STB SoC's to the ohci platform driver. >>> >>> Signed-off-by: Al Cooper >>> --- >> >>> @@ -177,6 +189,8 @@ static int ohci_platform_probe(struct >>> platform_device *dev) >>>           ohci->flags |= OHCI_QUIRK_FRAME_NO; >>>       if (pdata->num_ports) >>>           ohci->num_ports = pdata->num_ports; >>> +    if (pdata->suspend_without_phy_exit) >>> +        hcd->suspend_without_phy_exit = 1; >> >> Sorry if I missed this in the earlier discussions...  Is there any >> possibility of adding a DT binding that could express this requirement, >> instead of putting it in the platform data? >> >> Alan Stern >> > > Alan, > > That was my original approach but internal review suggested that I use > pdata instead. Below is my original patch for: And the reason for that suggestion was really because it was percevied as encoding a driver behavior as a Device Tree property as opposed to describing something that was inherently and strictly a hardware behavior (therefore suitable for Device Tree). > [PATCH V3 2/6] usb: core: Add ability to skip phy exit on suspend and > init on resume > With this patch I can then use ohci_platform.c without any > modifications. Could you let me know what you think? > > Thanks > Al > > Add the ability to skip calling the PHY's exit routine on suspend > and the PHY's init routine on resume. This is to handle a USB PHY > that should have it's power_off function called on suspend but cannot > have it's exit function called because on exit it will disable the > PHY to the point where register accesses to the Host Controllers > using the PHY will be disabled and the host drivers will crash. > > This is enabled with the HCD flag "suspend_without_phy_exit" which > can be set from any HCD driver or from the device-tree property > "suspend-without-phy-exit". > > Signed-off-by: Al Cooper > --- >  drivers/usb/core/hcd.c  |  8 ++++---- >  drivers/usb/core/phy.c  | 21 +++++++++++++++------ >  drivers/usb/core/phy.h  |  9 ++++++--- >  include/linux/usb/hcd.h |  3 +++ >  4 files changed, 28 insertions(+), 13 deletions(-) > > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c > index 1c21955fe7c0..e67e4d6b3d21 100644 > --- a/drivers/usb/core/hcd.c > +++ b/drivers/usb/core/hcd.c > @@ -2263,7 +2263,7 @@ int hcd_bus_suspend(struct usb_device *rhdev, > pm_message_t msg) >          hcd->state = HC_STATE_SUSPENDED; > >          if (!PMSG_IS_AUTO(msg)) > -            usb_phy_roothub_suspend(hcd->self.sysdev, > +            usb_phy_roothub_suspend(hcd, >                          hcd->phy_roothub); > >          /* Did we race with a root-hub wakeup event? */ > @@ -2304,7 +2304,7 @@ int hcd_bus_resume(struct usb_device *rhdev, > pm_message_t msg) >      } > >      if (!PMSG_IS_AUTO(msg)) { > -        status = usb_phy_roothub_resume(hcd->self.sysdev, > +        status = usb_phy_roothub_resume(hcd, >                          hcd->phy_roothub); >          if (status) >              return status; > @@ -2347,7 +2347,7 @@ int hcd_bus_resume(struct usb_device *rhdev, > pm_message_t msg) >          } >      } else { >          hcd->state = old_state; > -        usb_phy_roothub_suspend(hcd->self.sysdev, hcd->phy_roothub); > +        usb_phy_roothub_suspend(hcd, hcd->phy_roothub); >          dev_dbg(&rhdev->dev, "bus %s fail, err %d\n", >                  "resume", status); >          if (status != -ESHUTDOWN) > @@ -2744,7 +2744,7 @@ int usb_add_hcd(struct usb_hcd *hcd, >      struct usb_device *rhdev; > >      if (!hcd->skip_phy_initialization && usb_hcd_is_primary_hcd(hcd)) { > -        hcd->phy_roothub = usb_phy_roothub_alloc(hcd->self.sysdev); > +        hcd->phy_roothub = usb_phy_roothub_alloc(hcd); >          if (IS_ERR(hcd->phy_roothub)) >              return PTR_ERR(hcd->phy_roothub); > > diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c > index 9879767452a2..0eb12566f1c3 100644 > --- a/drivers/usb/core/phy.c > +++ b/drivers/usb/core/phy.c > @@ -45,10 +45,11 @@ static int usb_phy_roothub_add_phy(struct device > *dev, int index, >      return 0; >  } > > -struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev) > +struct usb_phy_roothub *usb_phy_roothub_alloc(struct usb_hcd *hcd) >  { >      struct usb_phy_roothub *phy_roothub; >      int i, num_phys, err; > +    struct device *dev = hcd->self.sysdev; > >      if (!IS_ENABLED(CONFIG_GENERIC_PHY)) >          return NULL; > @@ -58,6 +59,9 @@ struct usb_phy_roothub *usb_phy_roothub_alloc(struct > device *dev) >      if (num_phys <= 0) >          return NULL; > > +    if (device_property_read_bool(dev, "suspend-without-phy-exit")) > +        hcd->suspend_without_phy_exit = 1; > + >      phy_roothub = devm_kzalloc(dev, sizeof(*phy_roothub), GFP_KERNEL); >      if (!phy_roothub) >          return ERR_PTR(-ENOMEM); > @@ -161,26 +165,30 @@ void usb_phy_roothub_power_off(struct > usb_phy_roothub *phy_roothub) >  } >  EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off); > > -int usb_phy_roothub_suspend(struct device *controller_dev, > +int usb_phy_roothub_suspend(struct usb_hcd *hcd, >                  struct usb_phy_roothub *phy_roothub) >  { > +    struct device *controller_dev = hcd->self.sysdev; > + >      usb_phy_roothub_power_off(phy_roothub); > >      /* keep the PHYs initialized so the device can wake up the system */ > -    if (device_may_wakeup(controller_dev)) > +    if (device_may_wakeup(controller_dev) || > hcd->suspend_without_phy_exit) >          return 0; > >      return usb_phy_roothub_exit(phy_roothub); >  } >  EXPORT_SYMBOL_GPL(usb_phy_roothub_suspend); > > -int usb_phy_roothub_resume(struct device *controller_dev, > +int usb_phy_roothub_resume(struct usb_hcd *hcd, >                 struct usb_phy_roothub *phy_roothub) >  { > +    struct device *controller_dev = hcd->self.sysdev; >      int err; > >      /* if the device can't wake up the system _exit was called */ > -    if (!device_may_wakeup(controller_dev)) { > +    if (!device_may_wakeup(controller_dev) && > +        !hcd->suspend_without_phy_exit) { >          err = usb_phy_roothub_init(phy_roothub); >          if (err) >              return err; > @@ -189,7 +197,8 @@ int usb_phy_roothub_resume(struct device > *controller_dev, >      err = usb_phy_roothub_power_on(phy_roothub); > >      /* undo _init if _power_on failed */ > -    if (err && !device_may_wakeup(controller_dev)) > +    if (err && !device_may_wakeup(controller_dev) > +        && !hcd->suspend_without_phy_exit) >          usb_phy_roothub_exit(phy_roothub); > >      return err; > diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h > index 88a3c037e9df..34293e11a917 100644 > --- a/drivers/usb/core/phy.h > +++ b/drivers/usb/core/phy.h > @@ -5,13 +5,16 @@ >   * Copyright (C) 2018 Martin Blumenstingl > >   */ > > +#include > +#include > + >  #ifndef __USB_CORE_PHY_H_ >  #define __USB_CORE_PHY_H_ > >  struct device; >  struct usb_phy_roothub; > > -struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev); > +struct usb_phy_roothub *usb_phy_roothub_alloc(struct usb_hcd *hcd); > >  int usb_phy_roothub_init(struct usb_phy_roothub *phy_roothub); >  int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub); > @@ -19,9 +22,9 @@ >  int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub); >  void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub); > > -int usb_phy_roothub_suspend(struct device *controller_dev, > +int usb_phy_roothub_suspend(struct usb_hcd *hcd, >                  struct usb_phy_roothub *phy_roothub); > -int usb_phy_roothub_resume(struct device *controller_dev, > +int usb_phy_roothub_resume(struct usb_hcd *hcd, >                 struct usb_phy_roothub *phy_roothub); > >  #endif /* __USB_CORE_PHY_H_ */ > diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h > index 97e2ddec18b1..87a104055b5e 100644 > --- a/include/linux/usb/hcd.h > +++ b/include/linux/usb/hcd.h > @@ -157,6 +157,9 @@ struct usb_hcd { >       */ >      unsigned        skip_phy_initialization:1; > > +    /* Some phys don't want the phy's exit/init called on > suspend/resume */ > +    unsigned        suspend_without_phy_exit:1; > + >      /* The next flag is a stopgap, to be removed when all the HCDs >       * support the new root-hub polling mechanism. */ >      unsigned        uses_new_polling:1; -- Florian