Re: [PATCH V3 4/6] usb: ohci-platform: Add support for Broadcom STB SoC's

[Florian Fainelli]

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@xxxxxxxxx>
>>> ---
>>
>>> @@ -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@xxxxxxxxx>
> ---
> Â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@xxxxxxxxxxxxxx>
> Â */
> 
> +#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;


-- 
Florian