| Message ID | 1541453493-34981-5-git-send-email-alcooperx@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add XHCI, EHCI and OHCI support for Broadcom STB SoS's | expand |
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 <alcooperx@gmail.com> > --- > @@ -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
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 <alcooperx@gmail.com> >> --- > >> @@ -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: [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 <alcooperx@gmail.com> --- 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 <martin.blumenstingl@googlemail.com> */ +#include <linux/usb.h> +#include <linux/usb/hcd.h> + #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;
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 <alcooperx@gmail.com> >>> --- >> >>> @@ -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 <alcooperx@gmail.com> > --- > 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 > <martin.blumenstingl@googlemail.com> > */ > > +#include <linux/usb.h> > +#include <linux/usb/hcd.h> > + > #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;
On Tue, 6 Nov 2018, Florian Fainelli wrote: > 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 <alcooperx@gmail.com> > >>> --- > >> > >>> @@ -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). Right. The best way to approach this problem is to identify and characterize the hardware behavior which makes this override necessary. Then _that_ can be added to DT, since it will be a property of the hardware rather than of the driver. > > 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. What's special about this PHY? Why does the exit function mess the PHY up? Or to put it another way, why doesn't the exit function mess up other PHYs in the same way? For that matter, can we change the code so that suspend doesn't call the exit function for _any_ PHY? Will just calling the power_off function be good enough? If not, then why not? Alan Stern
On 11/7/18 10:23 AM, Alan Stern wrote: > On Tue, 6 Nov 2018, Florian Fainelli wrote: > >> 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 <alcooperx@gmail.com> >>>>> --- >>>> >>>>> @@ -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). > > Right. The best way to approach this problem is to identify and > characterize the hardware behavior which makes this override necessary. > Then _that_ can be added to DT, since it will be a property of the > hardware rather than of the driver. > >>> 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. > > What's special about this PHY? Why does the exit function mess the PHY > up? Or to put it another way, why doesn't the exit function mess up > other PHYs in the same way? > > For that matter, can we change the code so that suspend doesn't call > the exit function for _any_ PHY? Will just calling the power_off > function be good enough? If not, then why not? > > Alan Stern > In our USB hardware the USB PHY supplies a clock for the EHCI/OHCI and XHCI host controllers and if the PHY is totally shut down the EHCI, OHCI and XHCI registers will cause an exception if accessed and cause the EHCI, OHCI and XHCI drivers to crash. There is always talk of fixing this in the hardware by adding an aux clock that will takeover when the PHY clock is shut down, but this hasn't happened yet. It seems like "exit on suspend" still makes sense on systems that don't have this problem (additional power savings?) so removing the exit on suspend for all systems is not a good idea. Al
On Wed, 7 Nov 2018, Al Cooper wrote: > On 11/7/18 10:23 AM, Alan Stern wrote: > > On Tue, 6 Nov 2018, Florian Fainelli wrote: > > > >> 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 <alcooperx@gmail.com> > >>>>> --- > >>>> > >>>>> @@ -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). > > > > Right. The best way to approach this problem is to identify and > > characterize the hardware behavior which makes this override necessary. > > Then _that_ can be added to DT, since it will be a property of the > > hardware rather than of the driver. > > > >>> 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. > > > > What's special about this PHY? Why does the exit function mess the PHY > > up? Or to put it another way, why doesn't the exit function mess up > > other PHYs in the same way? > > > > For that matter, can we change the code so that suspend doesn't call > > the exit function for _any_ PHY? Will just calling the power_off > > function be good enough? If not, then why not? > > > > Alan Stern > > > > In our USB hardware the USB PHY supplies a clock for the EHCI/OHCI and > XHCI host controllers and if the PHY is totally shut down the EHCI, OHCI > and XHCI registers will cause an exception if accessed and cause the > EHCI, OHCI and XHCI drivers to crash. There is always talk of fixing > this in the hardware by adding an aux clock that will takeover when the > PHY clock is shut down, but this hasn't happened yet. It seems like > "exit on suspend" still makes sense on systems that don't have this > problem (additional power savings?) so removing the exit on suspend for > all systems is not a good idea. Then in theory you should be able to add a Device Tree property which says that the PHY provides a clock for the USB host controller. That is strictly a property of the hardware; it has nothing to do with the driver. Therefore it is appropriate for DT. Wouldn't this solve your issue? Alan Stern
On 11/7/18 8:27 AM, Alan Stern wrote: > On Wed, 7 Nov 2018, Al Cooper wrote: > >> On 11/7/18 10:23 AM, Alan Stern wrote: >>> On Tue, 6 Nov 2018, Florian Fainelli wrote: >>> >>>> 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 <alcooperx@gmail.com> >>>>>>> --- >>>>>> >>>>>>> @@ -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). >>> >>> Right. The best way to approach this problem is to identify and >>> characterize the hardware behavior which makes this override necessary. >>> Then _that_ can be added to DT, since it will be a property of the >>> hardware rather than of the driver. >>> >>>>> 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. >>> >>> What's special about this PHY? Why does the exit function mess the PHY >>> up? Or to put it another way, why doesn't the exit function mess up >>> other PHYs in the same way? >>> >>> For that matter, can we change the code so that suspend doesn't call >>> the exit function for _any_ PHY? Will just calling the power_off >>> function be good enough? If not, then why not? >>> >>> Alan Stern >>> >> >> In our USB hardware the USB PHY supplies a clock for the EHCI/OHCI and >> XHCI host controllers and if the PHY is totally shut down the EHCI, OHCI >> and XHCI registers will cause an exception if accessed and cause the >> EHCI, OHCI and XHCI drivers to crash. There is always talk of fixing >> this in the hardware by adding an aux clock that will takeover when the >> PHY clock is shut down, but this hasn't happened yet. It seems like >> "exit on suspend" still makes sense on systems that don't have this >> problem (additional power savings?) so removing the exit on suspend for >> all systems is not a good idea. > > Then in theory you should be able to add a Device Tree property which > says that the PHY provides a clock for the USB host controller. That > is strictly a property of the hardware; it has nothing to do with the > driver. Therefore it is appropriate for DT. The very compatible string that is being allocated/defined for this controller carries that information already, that is, if you probe a "brcm,bcm7445-ohci" compatible then that means the controller has a dependency on the PHY to supply its clock. Adding a property vs. keying on the compatible string makes sense if you know there is at least a second consumer of that property (unless we make it a broadcom specific property, in which case, it really is redundant with the compatible string). Anyway, my grudge with that property was the name chosen initially, which included an action to be performed by an implementation as opposed to something purely descriptive. E.g: 'phy-supplies-clock' might be okay. > > Wouldn't this solve your issue? It would not change much except that there is no need to much with ohci-platform.c anymore, but ultimately that property needs to be read by ohci-hcd.c and acted on, which would likely lead to the same amount of changes as those present in patch #2 currently.
On 11/7/18 12:29 PM, Florian Fainelli wrote: > On 11/7/18 8:27 AM, Alan Stern wrote: >> On Wed, 7 Nov 2018, Al Cooper wrote: >> >>> On 11/7/18 10:23 AM, Alan Stern wrote: >>>> On Tue, 6 Nov 2018, Florian Fainelli wrote: >>>> >>>>> 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 <alcooperx@gmail.com> >>>>>>>> --- >>>>>>> >>>>>>>> @@ -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). >>>> >>>> Right. The best way to approach this problem is to identify and >>>> characterize the hardware behavior which makes this override necessary. >>>> Then _that_ can be added to DT, since it will be a property of the >>>> hardware rather than of the driver. >>>> >>>>>> 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. >>>> >>>> What's special about this PHY? Why does the exit function mess the PHY >>>> up? Or to put it another way, why doesn't the exit function mess up >>>> other PHYs in the same way? >>>> >>>> For that matter, can we change the code so that suspend doesn't call >>>> the exit function for _any_ PHY? Will just calling the power_off >>>> function be good enough? If not, then why not? >>>> >>>> Alan Stern >>>> >>> >>> In our USB hardware the USB PHY supplies a clock for the EHCI/OHCI and >>> XHCI host controllers and if the PHY is totally shut down the EHCI, OHCI >>> and XHCI registers will cause an exception if accessed and cause the >>> EHCI, OHCI and XHCI drivers to crash. There is always talk of fixing >>> this in the hardware by adding an aux clock that will takeover when the >>> PHY clock is shut down, but this hasn't happened yet. It seems like >>> "exit on suspend" still makes sense on systems that don't have this >>> problem (additional power savings?) so removing the exit on suspend for >>> all systems is not a good idea. >> >> Then in theory you should be able to add a Device Tree property which >> says that the PHY provides a clock for the USB host controller. That >> is strictly a property of the hardware; it has nothing to do with the >> driver. Therefore it is appropriate for DT. > > The very compatible string that is being allocated/defined for this > controller carries that information already, that is, if you probe a > "brcm,bcm7445-ohci" compatible then that means the controller has a > dependency on the PHY to supply its clock. > > Adding a property vs. keying on the compatible string makes sense if you > know there is at least a second consumer of that property (unless we > make it a broadcom specific property, in which case, it really is > redundant with the compatible string). > > Anyway, my grudge with that property was the name chosen initially, > which included an action to be performed by an implementation as opposed > to something purely descriptive. E.g: 'phy-supplies-clock' might be okay. > >> >> Wouldn't this solve your issue? > > It would not change much except that there is no need to much with > ohci-platform.c anymore, but ultimately that property needs to be read > by ohci-hcd.c and acted on, which would likely lead to the same amount > of changes as those present in patch #2 currently. > We also need this functionality in the EHCI and XHCI drivers and it's not the ohci-hcd.c module that needs to know, it's the core/phy.c module called from core/hcd.c. Al
On 11/7/2018 9:40 AM, Al Cooper wrote: > On 11/7/18 12:29 PM, Florian Fainelli wrote: >> On 11/7/18 8:27 AM, Alan Stern wrote: >>> On Wed, 7 Nov 2018, Al Cooper wrote: >>> >>>> On 11/7/18 10:23 AM, Alan Stern wrote: >>>>> On Tue, 6 Nov 2018, Florian Fainelli wrote: >>>>> >>>>>> 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 <alcooperx@gmail.com> >>>>>>>>> --- >>>>>>>> >>>>>>>>> @@ -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). >>>>> >>>>> Right. The best way to approach this problem is to identify and >>>>> characterize the hardware behavior which makes this override >>>>> necessary. >>>>> Then _that_ can be added to DT, since it will be a property of the >>>>> hardware rather than of the driver. >>>>> >>>>>>> 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. >>>>> >>>>> What's special about this PHY? Why does the exit function mess the >>>>> PHY >>>>> up? Or to put it another way, why doesn't the exit function mess up >>>>> other PHYs in the same way? >>>>> >>>>> For that matter, can we change the code so that suspend doesn't call >>>>> the exit function for _any_ PHY? Will just calling the power_off >>>>> function be good enough? If not, then why not? >>>>> >>>>> Alan Stern >>>>> >>>> >>>> In our USB hardware the USB PHY supplies a clock for the EHCI/OHCI and >>>> XHCI host controllers and if the PHY is totally shut down the EHCI, >>>> OHCI >>>> and XHCI registers will cause an exception if accessed and cause the >>>> EHCI, OHCI and XHCI drivers to crash. There is always talk of fixing >>>> this in the hardware by adding an aux clock that will takeover when the >>>> PHY clock is shut down, but this hasn't happened yet. It seems like >>>> "exit on suspend" still makes sense on systems that don't have this >>>> problem (additional power savings?) so removing the exit on suspend for >>>> all systems is not a good idea. >>> >>> Then in theory you should be able to add a Device Tree property which >>> says that the PHY provides a clock for the USB host controller. That >>> is strictly a property of the hardware; it has nothing to do with the >>> driver. Therefore it is appropriate for DT. >> >> The very compatible string that is being allocated/defined for this >> controller carries that information already, that is, if you probe a >> "brcm,bcm7445-ohci" compatible then that means the controller has a >> dependency on the PHY to supply its clock. >> >> Adding a property vs. keying on the compatible string makes sense if you >> know there is at least a second consumer of that property (unless we >> make it a broadcom specific property, in which case, it really is >> redundant with the compatible string). >> >> Anyway, my grudge with that property was the name chosen initially, >> which included an action to be performed by an implementation as opposed >> to something purely descriptive. E.g: 'phy-supplies-clock' might be okay. >> >>> >>> Wouldn't this solve your issue? >> >> It would not change much except that there is no need to much with >> ohci-platform.c anymore, but ultimately that property needs to be read >> by ohci-hcd.c and acted on, which would likely lead to the same amount >> of changes as those present in patch #2 currently. >> > We also need this functionality in the EHCI and XHCI drivers and it's > not the ohci-hcd.c module that needs to know, it's the core/phy.c module > called from core/hcd.c. So in that regard the Device Tree property would actually scale a bit better in that you would no longer need to modify the various *hci-plat*.c files, if that is the way to go, then sure.
On Mon, Nov 12, 2018 at 6:37 PM Rob Herring <robh@kernel.org> wrote: > > On Wed, Nov 07, 2018 at 10:11:59AM -0800, Florian Fainelli wrote: > > On 11/7/2018 9:40 AM, Al Cooper wrote: > > > On 11/7/18 12:29 PM, Florian Fainelli wrote: > > >> On 11/7/18 8:27 AM, Alan Stern wrote: > > >>> On Wed, 7 Nov 2018, Al Cooper wrote: > > >>> > > >>>> On 11/7/18 10:23 AM, Alan Stern wrote: > > >>>>> On Tue, 6 Nov 2018, Florian Fainelli wrote: > > >>>>> > > >>>>>> 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 <alcooperx@gmail.com> > > >>>>>>>>> --- > > >>>>>>>> > > >>>>>>>>> @@ -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). > > >>>>> > > >>>>> Right. The best way to approach this problem is to identify and > > >>>>> characterize the hardware behavior which makes this override > > >>>>> necessary. > > >>>>> Then _that_ can be added to DT, since it will be a property of the > > >>>>> hardware rather than of the driver. > > >>>>> > > >>>>>>> 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. > > >>>>> > > >>>>> What's special about this PHY? Why does the exit function mess the > > >>>>> PHY > > >>>>> up? Or to put it another way, why doesn't the exit function mess up > > >>>>> other PHYs in the same way? > > >>>>> > > >>>>> For that matter, can we change the code so that suspend doesn't call > > >>>>> the exit function for _any_ PHY? Will just calling the power_off > > >>>>> function be good enough? If not, then why not? > > >>>>> > > >>>>> Alan Stern > > >>>>> > > >>>> > > >>>> In our USB hardware the USB PHY supplies a clock for the EHCI/OHCI and > > >>>> XHCI host controllers and if the PHY is totally shut down the EHCI, > > >>>> OHCI > > >>>> and XHCI registers will cause an exception if accessed and cause the > > >>>> EHCI, OHCI and XHCI drivers to crash. There is always talk of fixing > > >>>> this in the hardware by adding an aux clock that will takeover when the > > >>>> PHY clock is shut down, but this hasn't happened yet. It seems like > > >>>> "exit on suspend" still makes sense on systems that don't have this > > >>>> problem (additional power savings?) so removing the exit on suspend for > > >>>> all systems is not a good idea. > > >>> > > >>> Then in theory you should be able to add a Device Tree property which > > >>> says that the PHY provides a clock for the USB host controller. That > > >>> is strictly a property of the hardware; it has nothing to do with the > > >>> driver. Therefore it is appropriate for DT. > > >> > > >> The very compatible string that is being allocated/defined for this > > >> controller carries that information already, that is, if you probe a > > >> "brcm,bcm7445-ohci" compatible then that means the controller has a > > >> dependency on the PHY to supply its clock. > > >> > > >> Adding a property vs. keying on the compatible string makes sense if you > > >> know there is at least a second consumer of that property (unless we > > >> make it a broadcom specific property, in which case, it really is > > >> redundant with the compatible string). > > >> > > >> Anyway, my grudge with that property was the name chosen initially, > > >> which included an action to be performed by an implementation as opposed > > >> to something purely descriptive. E.g: 'phy-supplies-clock' might be okay. > > >> > > >>> > > >>> Wouldn't this solve your issue? > > >> > > >> It would not change much except that there is no need to much with > > >> ohci-platform.c anymore, but ultimately that property needs to be read > > >> by ohci-hcd.c and acted on, which would likely lead to the same amount > > >> of changes as those present in patch #2 currently. > > >> > > > We also need this functionality in the EHCI and XHCI drivers and it's > > > not the ohci-hcd.c module that needs to know, it's the core/phy.c module > > > called from core/hcd.c. > > > > So in that regard the Device Tree property would actually scale a bit > > better in that you would no longer need to modify the various > > *hci-plat*.c files, if that is the way to go, then sure. > > Sounds like the phy needs to be a clock provider to the USB controller. > Maybe that's a bit of overkill, but would be the most accurate. > > Otherwise, my preference is using the compatible string. IOW, we already > have properties to handle this. If you don't want to use them, then use > compatible rather than inventing something new and custom. My previous commit used the compatible string to set the HCD flag instead of a device tree property, but Alan Stern ask me to use a DT binding instead. This has the advantage of having the code in one place in the phy.c module instead of having to add it to the OHCI, EHCI and XHCI platform drivers. > > At least last time I looked, there's a lack of support in the PHY API to > handle various handshakes needed between phys and controllers like this. > It's fairly easy for the controller to fetch info from a phy node, but > the opposite is not so easy. It seems to me some API for controllers to > set flags in the phy driver is needed. The Generic PHY subsystem allows the PHY provider to export phy_power_on/phy_power_off and phy_init/phy_exit functions for use by the PHY consumer. The exact functionality of these routines seems to vary among the different PHY provider drivers. The Broadcom USB PHY driver expects the phy_power_on/phy_power_off to be used on suspend/resume and the phy_init/phy_exit to be used by the consumer driver's probe/remove routines. The new USB PHY consumer code does not do this, instead it calls both power_off AND exit for both suspend (not wakable) and remove. If you look at other PHY provider/consumer implementations in the kernel tree you'll find examples of both these methods but there are more examples that behave like the Broadcom PHY. What I was trying to do was to use a DT property to tell the USB PHY consumer code which method to use. I guess the problem is that the current generic PHY api only allows the consumer to put the PHY in 1 of 3 states, EXIT, OFF or ON. The current USB PHY consumer code is using OFF for "suspend with wakeup" and EXIT for "suspend without wakeup". The Broadcom PHY driver wants OFF for either "suspend with wakeup" or "suspend without wakeup" and EXIT when there are no consumers using the PHY. If the PHY API allowed for 4 states, EXIT, OFF_NOT_WAKABLE, OFF_WAKABLE and ON then the individual PHY provider could hide the differences, but this seems hard to retrofit into the PHY subsystem. Al > > Rob >
diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c index 65a1c3fdc88c..f425168238e6 100644 --- a/drivers/usb/host/ohci-platform.c +++ b/drivers/usb/host/ohci-platform.c @@ -22,6 +22,7 @@ #include <linux/module.h> #include <linux/err.h> #include <linux/of.h> +#include <linux/of_device.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> #include <linux/reset.h> @@ -99,12 +100,23 @@ static int ohci_platform_probe(struct platform_device *dev) if (usb_disabled()) return -ENODEV; - /* - * Use reasonable defaults so platforms don't have to provide these - * with DT probing on ARM. - */ - if (!pdata) - pdata = &ohci_platform_defaults; + if (!pdata) { + const struct usb_ohci_pdata *match_pdata; + + match_pdata = of_device_get_match_data(&dev->dev); + if (match_pdata) { + pdata = devm_kmemdup(&dev->dev, match_pdata, + sizeof(*pdata), GFP_KERNEL); + if (!pdata) + return -ENOMEM; + } else { + /* + * Use reasonable defaults so platforms don't have + * to provide these with DT probing on ARM. + */ + pdata = &ohci_platform_defaults; + } + } err = dma_coerce_mask_and_coherent(&dev->dev, DMA_BIT_MASK(32)); if (err) @@ -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; #ifndef CONFIG_USB_OHCI_BIG_ENDIAN_MMIO if (ohci->flags & OHCI_QUIRK_BE_MMIO) { @@ -305,10 +319,18 @@ static int ohci_platform_resume(struct device *dev) } #endif /* CONFIG_PM_SLEEP */ +static const struct usb_ohci_pdata ohci_plat_brcm_bcm7445_ohci = { + .power_on = ohci_platform_power_on, + .power_suspend = ohci_platform_power_off, + .power_off = ohci_platform_power_off, + .suspend_without_phy_exit = 1, +}; + static const struct of_device_id ohci_platform_ids[] = { { .compatible = "generic-ohci", }, { .compatible = "cavium,octeon-6335-ohci", }, { .compatible = "ti,ohci-omap3", }, + { .compatible = "brcm,bcm7445-ohci", &ohci_plat_brcm_bcm7445_ohci}, { } }; MODULE_DEVICE_TABLE(of, ohci_platform_ids); diff --git a/include/linux/usb/ohci_pdriver.h b/include/linux/usb/ohci_pdriver.h index 7eb16cf587ee..16b24ea1e3bb 100644 --- a/include/linux/usb/ohci_pdriver.h +++ b/include/linux/usb/ohci_pdriver.h @@ -35,6 +35,7 @@ struct usb_ohci_pdata { unsigned big_endian_desc:1; unsigned big_endian_mmio:1; unsigned no_big_frame_no:1; + unsigned suspend_without_phy_exit:1; unsigned int num_ports; /* Turn on all power and clocks */
Add support for Broadcom STB SoC's to the ohci platform driver. Signed-off-by: Al Cooper <alcooperx@gmail.com> --- drivers/usb/host/ohci-platform.c | 34 ++++++++++++++++++++++++++++------ include/linux/usb/ohci_pdriver.h | 1 + 2 files changed, 29 insertions(+), 6 deletions(-)