diff mbox

[usb-next,v10,3/8] usb: core: add a wrapper for the USB PHYs on the HCD

Message ID e9ef724f-0a62-cd35-9e0a-82643bde4a16@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Roger Quadros March 22, 2018, 12:41 p.m. UTC
On 22/03/18 10:10, Chunfeng Yun wrote:
> Hi,
> On Wed, 2018-03-21 at 13:30 +0200, Roger Quadros wrote:
>> Martin,
>>
>> On 21/03/18 00:01, Martin Blumenstingl wrote:
>>> Hi Roger, Hi Chunfeng,
>>>
>>> On Tue, Mar 20, 2018 at 1:04 PM, Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
>>>> Hi Martin & Roger:
>>>>
>>>> On Mon, 2018-03-19 at 17:12 +0100, Martin Blumenstingl wrote:
>>>>> Hi Roger,
>>>>>
>>>>> On Mon, Mar 19, 2018 at 9:49 AM, Roger Quadros <rogerq@ti.com> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 19/03/18 00:29, Martin Blumenstingl wrote:
>>>>>>> Hi Roger,
>>>>>>>
>>>>>>> On Fri, Mar 16, 2018 at 3:32 PM, Roger Quadros <rogerq@ti.com> wrote:
>>>>>>>> +some TI folks
>>>>>>>>
>>>>>>>> Hi Martin,
>>>>>>>>
>>>>>>>> On 18/02/18 20:44, Martin Blumenstingl wrote:
>>>>>>>>> Many SoC platforms have separate devices for the USB PHY which are
>>>>>>>>> registered through the generic PHY framework. These PHYs have to be
>>>>>>>>> enabled to make the USB controller actually work. They also have to be
>>>>>>>>> disabled again on shutdown/suspend.
>>>>>>>>>
>>>>>>>>> Currently (at least) the following HCI platform drivers are using custom
>>>>>>>>> code to obtain all PHYs via devicetree for the roothub/controller and
>>>>>>>>> disable/enable them when required:
>>>>>>>>> - ehci-platform.c has ehci_platform_power_{on,off}
>>>>>>>>> - xhci-mtk.c has xhci_mtk_phy_{init,exit,power_on,power_off}
>>>>>>>>> - ohci-platform.c has ohci_platform_power_{on,off}
>>>>>>>>>
>>>>>>>>> With this new wrapper the USB PHYs can be specified directly in the
>>>>>>>>> USB controller's devicetree node (just like on the drivers listed
>>>>>>>>> above). This allows SoCs like the Amlogic Meson GXL family to operate
>>>>>>>>> correctly once this is wired up correctly. These SoCs use a dwc3
>>>>>>>>> controller and require all USB PHYs to be initialized (if one of the USB
>>>>>>>>> PHYs it not initialized then none of USB port works at all).
>>>>>>>>>
>>>>>>>>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>>>>>>>>> Tested-by: Yixun Lan <yixun.lan@amlogic.com>
>>>>>>>>> Cc: Neil Armstrong <narmstrong@baylibre.com>
>>>>>>>>> Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
>>>>>>>>
>>>>>>>> This patch is breaking low power cases on TI SoCs when USB is in host mode.
>>>>>>>> I'll explain why below.
>>>>>>> based on your explanation and reading the TI PHY drivers I am assuming
>>>>>>> that the affected SoCs are using the "phy-omap-usb2" driver
>>>>>>>
>>>>>> yes and phy-ti-pipe3 as well i.e. "ti,phy-usb3" and "ti,omap-usb3"
>>>>> I missed that, thanks
>>>>>
>>>>>>>>> ---
>>>>>>>>>  drivers/usb/core/Makefile |   2 +-
>>>>>>>>>  drivers/usb/core/phy.c    | 158 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>>  drivers/usb/core/phy.h    |   7 ++
>>>>>>>>>  3 files changed, 166 insertions(+), 1 deletion(-)
>>>>>>>>>  create mode 100644 drivers/usb/core/phy.c
>>>>>>>>>  create mode 100644 drivers/usb/core/phy.h
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
>>>>>>>>> index 92c9cefb4317..18e874b0441e 100644
>>>>>>>>> --- a/drivers/usb/core/Makefile
>>>>>>>>> +++ b/drivers/usb/core/Makefile
>>>>>>>>> @@ -6,7 +6,7 @@
>>>>>>>>>  usbcore-y := usb.o hub.o hcd.o urb.o message.o driver.o
>>>>>>>>>  usbcore-y += config.o file.o buffer.o sysfs.o endpoint.o
>>>>>>>>>  usbcore-y += devio.o notify.o generic.o quirks.o devices.o
>>>>>>>>> -usbcore-y += port.o
>>>>>>>>> +usbcore-y += phy.o port.o
>>>>>>>>>
>>>>>>>>>  usbcore-$(CONFIG_OF)         += of.o
>>>>>>>>>  usbcore-$(CONFIG_USB_PCI)            += hcd-pci.o
>>>>>>>>> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
>>>>>>>>> new file mode 100644
>>>>>>>>> index 000000000000..09b7c43c0ea4
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/drivers/usb/core/phy.c
>>>>>>>>> @@ -0,0 +1,158 @@
>>>>>>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>>>>>>> +/*
>>>>>>>>> + * A wrapper for multiple PHYs which passes all phy_* function calls to
>>>>>>>>> + * multiple (actual) PHY devices. This is comes handy when initializing
>>>>>>>>> + * all PHYs on a HCD and to keep them all in the same state.
>>>>>>>>> + *
>>>>>>>>> + * Copyright (C) 2018 Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>>>>>>>>> + */
>>>>>>>>> +
>>>>>>>>> +#include <linux/device.h>
>>>>>>>>> +#include <linux/list.h>
>>>>>>>>> +#include <linux/phy/phy.h>
>>>>>>>>> +#include <linux/of.h>
>>>>>>>>> +
>>>>>>>>> +#include "phy.h"
>>>>>>>>> +
>>>>>>>>> +struct usb_phy_roothub {
>>>>>>>>> +     struct phy              *phy;
>>>>>>>>> +     struct list_head        list;
>>>>>>>>> +};
>>>>>>>>> +
>>>>>>>>> +static struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
>>>>>>>>> +{
>>>>>>>>> +     struct usb_phy_roothub *roothub_entry;
>>>>>>>>> +
>>>>>>>>> +     roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
>>>>>>>>> +     if (!roothub_entry)
>>>>>>>>> +             return ERR_PTR(-ENOMEM);
>>>>>>>>> +
>>>>>>>>> +     INIT_LIST_HEAD(&roothub_entry->list);
>>>>>>>>> +
>>>>>>>>> +     return roothub_entry;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +static int usb_phy_roothub_add_phy(struct device *dev, int index,
>>>>>>>>> +                                struct list_head *list)
>>>>>>>>> +{
>>>>>>>>> +     struct usb_phy_roothub *roothub_entry;
>>>>>>>>> +     struct phy *phy = devm_of_phy_get_by_index(dev, dev->of_node, index);
>>>>>>>>> +
>>>>>>>>> +     if (IS_ERR_OR_NULL(phy)) {
>>>>>>>>> +             if (!phy || PTR_ERR(phy) == -ENODEV)
>>>>>>>>> +                     return 0;
>>>>>>>>> +             else
>>>>>>>>> +                     return PTR_ERR(phy);
>>>>>>>>> +     }
>>>>>>>>> +
>>>>>>>>> +     roothub_entry = usb_phy_roothub_alloc(dev);
>>>>>>>>> +     if (IS_ERR(roothub_entry))
>>>>>>>>> +             return PTR_ERR(roothub_entry);
>>>>>>>>> +
>>>>>>>>> +     roothub_entry->phy = phy;
>>>>>>>>> +
>>>>>>>>> +     list_add_tail(&roothub_entry->list, list);
>>>>>>>>> +
>>>>>>>>> +     return 0;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
>>>>>>>>> +{
>>>>>>>>> +     struct usb_phy_roothub *phy_roothub;
>>>>>>>>> +     struct usb_phy_roothub *roothub_entry;
>>>>>>>>> +     struct list_head *head;
>>>>>>>>> +     int i, num_phys, err;
>>>>>>>>> +
>>>>>>>>> +     num_phys = of_count_phandle_with_args(dev->of_node, "phys",
>>>>>>>>> +                                           "#phy-cells");
>>>>>>>>> +     if (num_phys <= 0)
>>>>>>>>> +             return NULL;
>>>>>>>>> +
>>>>>>>>> +     phy_roothub = usb_phy_roothub_alloc(dev);
>>>>>>>>> +     if (IS_ERR(phy_roothub))
>>>>>>>>> +             return phy_roothub;
>>>>>>>>> +
>>>>>>>>> +     for (i = 0; i < num_phys; i++) {
>>>>>>>>> +             err = usb_phy_roothub_add_phy(dev, i, &phy_roothub->list);
>>>>>>>>> +             if (err)
>>>>>>>>> +                     goto err_out;
>>>>>>>>> +     }
>>>>>>>>> +
>>>>>>>>> +     head = &phy_roothub->list;
>>>>>>>>> +
>>>>>>>>> +     list_for_each_entry(roothub_entry, head, list) {
>>>>>>>>> +             err = phy_init(roothub_entry->phy);
>>>>>>>>
>>>>>>>> The phy_init() function actually enables the PHY clocks.
>>>>>>>> It should be moved to the usb_phy_roothub_exit() routine just before calling phy_power_on().
>>>>>>> do you mean that phy_init should be moved to usb_phy_roothub_power_on
>>>>>>> (just before phy_power_on is called within usb_phy_roothub_power_on)?
>>>>>>>
>>>>>>
>>>>>> Yes.
>>>>>>
>>>>>>> an earlier version of my patch did exactly this, but it caused
>>>>>>> problems during a suspend/resume cycle on Mediatek devices
>>>>>>> Chunfeng Yun reported that issue here [0], quote from that mail for
>>>>>>> easier reading:
>>>>>>> "In order to keep link state on mt8173, we just power off all phys(not
>>>>>>> exit) when system enter suspend, then power on them again (needn't
>>>>>>> init, otherwise device will be disconnected) when system resume, this
>>>>>>> can avoid re-enumerating device."
>>>>>>>
>>>>>>>>> +             if (err)
>>>>>>>>> +                     goto err_exit_phys;
>>>>>>>>> +     }
>>>>>>>>> +
>>>>>>>>> +     return phy_roothub;
>>>>>>>>> +
>>>>>>>>> +err_exit_phys:
>>>>>>>>> +     list_for_each_entry_continue_reverse(roothub_entry, head, list)
>>>>>>>>> +             phy_exit(roothub_entry->phy);
>>>>>>>>> +
>>>>>>>>> +err_out:
>>>>>>>>> +     return ERR_PTR(err);
>>>>>>>>> +}
>>>>>>>>> +EXPORT_SYMBOL_GPL(usb_phy_roothub_init);
>>>>>>>>> +
>>>>>>>>> +int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub)
>>>>>>>>> +{
>>>>>>>>> +     struct usb_phy_roothub *roothub_entry;
>>>>>>>>> +     struct list_head *head;
>>>>>>>>> +     int err, ret = 0;
>>>>>>>>> +
>>>>>>>>> +     if (!phy_roothub)
>>>>>>>>> +             return 0;
>>>>>>>>> +
>>>>>>>>> +     head = &phy_roothub->list;
>>>>>>>>> +
>>>>>>>>> +     list_for_each_entry(roothub_entry, head, list) {
>>>>>>>>> +             err = phy_exit(roothub_entry->phy);
>>>>>>>>> +             if (err)
>>>>>>>>> +                     ret = ret;
>>>>>>>>> +     }
>>>>>>>>
>>>>>>>> phy_exit() should be moved to usb_phy_roothub_poweroff() just after calling phy_power_off().
>>>>>>> if I understood Chunfeng Yun correctly this will require
>>>>>>> re-enumeration of the USB devices after a suspend/resume cycle on
>>>>>>> Mediatek SoCs
>>>>>>>
>>>>>>
>>>>>> OK. I suppose that there are 2 cases
>>>>>> 1) Mediatek's case: USB controller context retained across suspend/resume.
>>>>>> Remote wakeup probably required.
>>>>>> No re-enumeration preferred after resume. phy_exit()/phy_init() must not be called
>>>>>> during suspend/resume to keep PHY link active.
>>>>> Documentation/devicetree/bindings/usb/mediatek,mtu3.txt indeed shows
>>>>> that the parent of the USB controller can be marked as "wakeup-source"
>>>>>
>>>>>> 2) TI's case: low power important: USB context is lost, OK to re-enumerate.
>>>>>> phy_exit()/phy_init() must be called during suspend/resume.
>>>>> ACK
>>>>>
>>>>>>>> With that there is nothing else being done here. Shouldn't we be doing the equivalent of
>>>>>>>> usb_phy_roothub_del_phy() and usb_phy_roothub_free() here?
>>>>>>>>
>>>>>>>>> +
>>>>>>>>> +     return ret;
>>>>>>>>> +}
>>>>>>>>> +EXPORT_SYMBOL_GPL(usb_phy_roothub_exit);
>>>>>>>>> +
>>>>>>>>> +int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub)
>>>>>>>>> +{
>>>>>>>>> +     struct usb_phy_roothub *roothub_entry;
>>>>>>>>> +     struct list_head *head;
>>>>>>>>> +     int err;
>>>>>>>>> +
>>>>>>>>> +     if (!phy_roothub)
>>>>>>>>> +             return 0;
>>>>>>>>> +
>>>>>>>>> +     head = &phy_roothub->list;
>>>>>>>>> +
>>>>>>>>> +     list_for_each_entry(roothub_entry, head, list) {
>>>>>>>>> +             err = phy_power_on(roothub_entry->phy);
>>>>>>>>> +             if (err)
>>>>>>>>> +                     goto err_out;
>>>>>>>>> +     }
>>>>>>>>> +
>>>>>>>>> +     return 0;
>>>>>>>>> +
>>>>>>>>> +err_out:
>>>>>>>>> +     list_for_each_entry_continue_reverse(roothub_entry, head, list)
>>>>>>>>> +             phy_power_off(roothub_entry->phy);
>>>>>>>>> +
>>>>>>>>> +     return err;
>>>>>>>>> +}
>>>>>>>>> +EXPORT_SYMBOL_GPL(usb_phy_roothub_power_on);
>>>>>>>>> +
>>>>>>>>> +void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub)
>>>>>>>>> +{
>>>>>>>>> +     struct usb_phy_roothub *roothub_entry;
>>>>>>>>> +
>>>>>>>>> +     if (!phy_roothub)
>>>>>>>>> +             return;
>>>>>>>>> +
>>>>>>>>> +     list_for_each_entry_reverse(roothub_entry, &phy_roothub->list, list)
>>>>>>>>> +             phy_power_off(roothub_entry->phy);
>>>>>>>>
>>>>>>>> Not doing the phy_exit() here leaves the clocks enabled on our SoC and
>>>>>>>> we're no longer able to reach low power states on system suspend.
>>>>>>> I'm not sure where this problem should be solved:
>>>>>>> - set skip_phy_initialization in struct usb_hcd to 1 for the affected
>>>>>>> TI platforms
>>>>>>
>>>>>> Many TI platforms are affected, omap5*, dra7*, am43*
>>>>>>
>>>>>>> - fix this in the usb_phy_roothub code
>>>>>>
>>>>>> I'd vote for fixing it in the usb_phy_roothub code. How?
>>>>>> How about using the device_can_wakeup() to decide if we should call phy_exit()/init() or not?
>>>>>> If the USB device can't wakeup the system there is no point in keeping it powered/clocked right?
>>>>> @Chunfeng: can you confirm Roger's idea that we could call phy_exit if
>>>>> the controller is *NOT* marked as "wakeup-source"?
>>>>> I am also not sure if it would work, since the "wakeup-source"
>>>>> property is defined on the USB controller's parent node in case of the
>>>>> Mediatek MTU3 (Mediatek USB3.0 DRD) controller
>>>>>
>>>> Very sorry, I forgot that MTU3 & xHCI drivers always set them as wakeup
>>>> devices by device_init_wakeup(dev, true),but not dependent on
>>>> "wakeup-source" property, so maybe we can use device_can_wakeup() to
>>>> decide whether call phy_exit()/init() or not.
>>> the 64-bit Amlogic Meson GXL/GXM SoCs don't support suspend/resume
>>> yet, so I cannot test this
>>> based on this suggestion I threw up two patches which are *compile
>>> tested only* based on Greg's usb-next branch
>>> you can find them here: [0] (as well as attached to this mail)
>>>
>>> @Chunfeng: can you please test this on one of your Mediatek SoCs?
>>> @Roger: can you please test this on a TI SoC?
>>>
>>> (apologies in advance if these patches don't work)
>>>
>>> please note that I won't have access to my computer until Saturday.
>>> if these patches need to be rewritten/replaced/etc. then feel free to
>>> send your own version to the list
>>
>> Had to do the following to build
>>
>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>> index 6d4a419..2884607 100644
>> --- a/drivers/usb/core/hcd.c
>> +++ b/drivers/usb/core/hcd.c
>> @@ -2262,7 +2262,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->phy_roothub);
>> +			usb_phy_roothub_suspend(&rhdev->dev, hcd->phy_roothub);
> 
> Try to use hcd->self.controller instead of &rhdev->dev;

Actually it should be hcd->self.sysdev.
> 
>>  
>>  		/* Did we race with a root-hub wakeup event? */
>>  		if (rhdev->do_remote_wakeup) {
>> @@ -2302,7 +2302,7 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
>>  	}
>>  
>>  	if (!PMSG_IS_AUTO(msg)) {
>> -		status = usb_phy_roothub_resume(hcd->phy_roothub);
>> +		status = usb_phy_roothub_resume(&rhdev->dev, hcd->phy_roothub);
> ditto
>>  		if (status)
>>  			return status;
>>  	}
>> @@ -2344,7 +2344,7 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
>>  		}
>>  	} else {
>>  		hcd->state = old_state;
>> -		usb_phy_roothub_suspend(hcd->phy_roothub);
>> +		usb_phy_roothub_suspend(&rhdev->dev, hcd->phy_roothub);
> ditto
>>  		dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
>>  				"resume", status);
>>  		if (status != -ESHUTDOWN)
>>
>>
>> And the following to fix runtime issues
>>
>> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
>> index 2eca371..8598906 100644
>> --- a/drivers/usb/core/phy.c
>> +++ b/drivers/usb/core/phy.c
>> @@ -32,9 +32,9 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index,
>>  			return PTR_ERR(phy);
>>  	}
>>  
>> -	roothub_entry = usb_phy_roothub_alloc(dev);
>> -	if (IS_ERR(roothub_entry))
>> -		return PTR_ERR(roothub_entry);
>> +	roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
>> +	if (!roothub_entry)
>> +		return -ENOMEM;
>>  
>>  	roothub_entry->phy = phy;
>>  
>> @@ -162,7 +162,7 @@ int usb_phy_roothub_suspend(struct device *dev,
>>  	usb_phy_roothub_power_off(phy_roothub);
>>  
>>  	/* keep the PHYs initialized so the device can wake up the system */
>> -	if (device_can_wakeup(dev))
>> +	if (device_may_wakeup(dev))
> It's ok

I had to additionally fix usb_phy_roothub_resume() from
	if (device_can_wakeup(dev))
to
	if (!device_may_wakeup(dev))

>>  		return 0;
>>  
>>  	return usb_phy_roothub_exit(phy_roothub);
>>
>>
>> Here are my obervations
>> - if wakeup is disabled it works fine as expected, phy_exit() is called and I'm able to reach
>> low power states.
>>
>> - if wakeup is enabled (/sys/bus/usb/device/usb2/power/wakeup), then hcd_bus_suspend() is never called
>> and so phy_power_off won't be called either.
>>
>> This means that the device_may_wakeup() check is redundant. Sorry for suggesting this.
> You maybe use a wrong device.

Yes, after using the correct device I don't see the problem.

Can you please try the below patch on top of the 2 Patches that Martin sent and the new patch you sent?
Once you confirm it works for you I can send the 2 patches officially.

Comments

Chunfeng Yun (云春峰) March 23, 2018, 3:19 a.m. UTC | #1
On Thu, 2018-03-22 at 14:41 +0200, Roger Quadros wrote:
> On 22/03/18 10:10, Chunfeng Yun wrote:
> > Hi,
> > On Wed, 2018-03-21 at 13:30 +0200, Roger Quadros wrote:
> >> Martin,
> >>
> >> On 21/03/18 00:01, Martin Blumenstingl wrote:
> >>> Hi Roger, Hi Chunfeng,
> >>>
> >>> On Tue, Mar 20, 2018 at 1:04 PM, Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
> >>>> Hi Martin & Roger:
> >>>>
> >>>> On Mon, 2018-03-19 at 17:12 +0100, Martin Blumenstingl wrote:
> >>>>> Hi Roger,
> >>>>>
> >>>>> On Mon, Mar 19, 2018 at 9:49 AM, Roger Quadros <rogerq@ti.com> wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> On 19/03/18 00:29, Martin Blumenstingl wrote:
> >>>>>>> Hi Roger,
> >>>>>>>
> >>>>>>> On Fri, Mar 16, 2018 at 3:32 PM, Roger Quadros <rogerq@ti.com> wrote:
> >>>>>>>> +some TI folks
> >>>>>>>>
> >>>>>>>> Hi Martin,
> >>>>>>>>
> >>>>>>>> On 18/02/18 20:44, Martin Blumenstingl wrote:
> >>>>>>>>> Many SoC platforms have separate devices for the USB PHY which are
> >>>>>>>>> registered through the generic PHY framework. These PHYs have to be
> >>>>>>>>> enabled to make the USB controller actually work. They also have to be
> >>>>>>>>> disabled again on shutdown/suspend.
> >>>>>>>>>
> >>>>>>>>> Currently (at least) the following HCI platform drivers are using custom
> >>>>>>>>> code to obtain all PHYs via devicetree for the roothub/controller and
> >>>>>>>>> disable/enable them when required:
> >>>>>>>>> - ehci-platform.c has ehci_platform_power_{on,off}
> >>>>>>>>> - xhci-mtk.c has xhci_mtk_phy_{init,exit,power_on,power_off}
> >>>>>>>>> - ohci-platform.c has ohci_platform_power_{on,off}
> >>>>>>>>>
> >>>>>>>>> With this new wrapper the USB PHYs can be specified directly in the
> >>>>>>>>> USB controller's devicetree node (just like on the drivers listed
> >>>>>>>>> above). This allows SoCs like the Amlogic Meson GXL family to operate
> >>>>>>>>> correctly once this is wired up correctly. These SoCs use a dwc3
> >>>>>>>>> controller and require all USB PHYs to be initialized (if one of the USB
> >>>>>>>>> PHYs it not initialized then none of USB port works at all).
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> >>>>>>>>> Tested-by: Yixun Lan <yixun.lan@amlogic.com>
> >>>>>>>>> Cc: Neil Armstrong <narmstrong@baylibre.com>
> >>>>>>>>> Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
> >>>>>>>>
> >>>>>>>> This patch is breaking low power cases on TI SoCs when USB is in host mode.
> >>>>>>>> I'll explain why below.
> >>>>>>> based on your explanation and reading the TI PHY drivers I am assuming
> >>>>>>> that the affected SoCs are using the "phy-omap-usb2" driver
> >>>>>>>
> >>>>>> yes and phy-ti-pipe3 as well i.e. "ti,phy-usb3" and "ti,omap-usb3"
> >>>>> I missed that, thanks
> >>>>>
> >>>>>>>>> ---
> >>>>>>>>>  drivers/usb/core/Makefile |   2 +-
> >>>>>>>>>  drivers/usb/core/phy.c    | 158 ++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>>>>>  drivers/usb/core/phy.h    |   7 ++
> >>>>>>>>>  3 files changed, 166 insertions(+), 1 deletion(-)
> >>>>>>>>>  create mode 100644 drivers/usb/core/phy.c
> >>>>>>>>>  create mode 100644 drivers/usb/core/phy.h
> >>>>>>>>>
> >>>>>>>>> diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
> >>>>>>>>> index 92c9cefb4317..18e874b0441e 100644
> >>>>>>>>> --- a/drivers/usb/core/Makefile
> >>>>>>>>> +++ b/drivers/usb/core/Makefile
> >>>>>>>>> @@ -6,7 +6,7 @@
> >>>>>>>>>  usbcore-y := usb.o hub.o hcd.o urb.o message.o driver.o
> >>>>>>>>>  usbcore-y += config.o file.o buffer.o sysfs.o endpoint.o
> >>>>>>>>>  usbcore-y += devio.o notify.o generic.o quirks.o devices.o
> >>>>>>>>> -usbcore-y += port.o
> >>>>>>>>> +usbcore-y += phy.o port.o
> >>>>>>>>>
> >>>>>>>>>  usbcore-$(CONFIG_OF)         += of.o
> >>>>>>>>>  usbcore-$(CONFIG_USB_PCI)            += hcd-pci.o
> >>>>>>>>> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
> >>>>>>>>> new file mode 100644
> >>>>>>>>> index 000000000000..09b7c43c0ea4
> >>>>>>>>> --- /dev/null
> >>>>>>>>> +++ b/drivers/usb/core/phy.c
> >>>>>>>>> @@ -0,0 +1,158 @@
> >>>>>>>>> +// SPDX-License-Identifier: GPL-2.0+
> >>>>>>>>> +/*
> >>>>>>>>> + * A wrapper for multiple PHYs which passes all phy_* function calls to
> >>>>>>>>> + * multiple (actual) PHY devices. This is comes handy when initializing
> >>>>>>>>> + * all PHYs on a HCD and to keep them all in the same state.
> >>>>>>>>> + *
> >>>>>>>>> + * Copyright (C) 2018 Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> >>>>>>>>> + */
> >>>>>>>>> +
> >>>>>>>>> +#include <linux/device.h>
> >>>>>>>>> +#include <linux/list.h>
> >>>>>>>>> +#include <linux/phy/phy.h>
> >>>>>>>>> +#include <linux/of.h>
> >>>>>>>>> +
> >>>>>>>>> +#include "phy.h"
> >>>>>>>>> +
> >>>>>>>>> +struct usb_phy_roothub {
> >>>>>>>>> +     struct phy              *phy;
> >>>>>>>>> +     struct list_head        list;
> >>>>>>>>> +};
> >>>>>>>>> +
> >>>>>>>>> +static struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
> >>>>>>>>> +{
> >>>>>>>>> +     struct usb_phy_roothub *roothub_entry;
> >>>>>>>>> +
> >>>>>>>>> +     roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
> >>>>>>>>> +     if (!roothub_entry)
> >>>>>>>>> +             return ERR_PTR(-ENOMEM);
> >>>>>>>>> +
> >>>>>>>>> +     INIT_LIST_HEAD(&roothub_entry->list);
> >>>>>>>>> +
> >>>>>>>>> +     return roothub_entry;
> >>>>>>>>> +}
> >>>>>>>>> +
> >>>>>>>>> +static int usb_phy_roothub_add_phy(struct device *dev, int index,
> >>>>>>>>> +                                struct list_head *list)
> >>>>>>>>> +{
> >>>>>>>>> +     struct usb_phy_roothub *roothub_entry;
> >>>>>>>>> +     struct phy *phy = devm_of_phy_get_by_index(dev, dev->of_node, index);
> >>>>>>>>> +
> >>>>>>>>> +     if (IS_ERR_OR_NULL(phy)) {
> >>>>>>>>> +             if (!phy || PTR_ERR(phy) == -ENODEV)
> >>>>>>>>> +                     return 0;
> >>>>>>>>> +             else
> >>>>>>>>> +                     return PTR_ERR(phy);
> >>>>>>>>> +     }
> >>>>>>>>> +
> >>>>>>>>> +     roothub_entry = usb_phy_roothub_alloc(dev);
> >>>>>>>>> +     if (IS_ERR(roothub_entry))
> >>>>>>>>> +             return PTR_ERR(roothub_entry);
> >>>>>>>>> +
> >>>>>>>>> +     roothub_entry->phy = phy;
> >>>>>>>>> +
> >>>>>>>>> +     list_add_tail(&roothub_entry->list, list);
> >>>>>>>>> +
> >>>>>>>>> +     return 0;
> >>>>>>>>> +}
> >>>>>>>>> +
> >>>>>>>>> +struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
> >>>>>>>>> +{
> >>>>>>>>> +     struct usb_phy_roothub *phy_roothub;
> >>>>>>>>> +     struct usb_phy_roothub *roothub_entry;
> >>>>>>>>> +     struct list_head *head;
> >>>>>>>>> +     int i, num_phys, err;
> >>>>>>>>> +
> >>>>>>>>> +     num_phys = of_count_phandle_with_args(dev->of_node, "phys",
> >>>>>>>>> +                                           "#phy-cells");
> >>>>>>>>> +     if (num_phys <= 0)
> >>>>>>>>> +             return NULL;
> >>>>>>>>> +
> >>>>>>>>> +     phy_roothub = usb_phy_roothub_alloc(dev);
> >>>>>>>>> +     if (IS_ERR(phy_roothub))
> >>>>>>>>> +             return phy_roothub;
> >>>>>>>>> +
> >>>>>>>>> +     for (i = 0; i < num_phys; i++) {
> >>>>>>>>> +             err = usb_phy_roothub_add_phy(dev, i, &phy_roothub->list);
> >>>>>>>>> +             if (err)
> >>>>>>>>> +                     goto err_out;
> >>>>>>>>> +     }
> >>>>>>>>> +
> >>>>>>>>> +     head = &phy_roothub->list;
> >>>>>>>>> +
> >>>>>>>>> +     list_for_each_entry(roothub_entry, head, list) {
> >>>>>>>>> +             err = phy_init(roothub_entry->phy);
> >>>>>>>>
> >>>>>>>> The phy_init() function actually enables the PHY clocks.
> >>>>>>>> It should be moved to the usb_phy_roothub_exit() routine just before calling phy_power_on().
> >>>>>>> do you mean that phy_init should be moved to usb_phy_roothub_power_on
> >>>>>>> (just before phy_power_on is called within usb_phy_roothub_power_on)?
> >>>>>>>
> >>>>>>
> >>>>>> Yes.
> >>>>>>
> >>>>>>> an earlier version of my patch did exactly this, but it caused
> >>>>>>> problems during a suspend/resume cycle on Mediatek devices
> >>>>>>> Chunfeng Yun reported that issue here [0], quote from that mail for
> >>>>>>> easier reading:
> >>>>>>> "In order to keep link state on mt8173, we just power off all phys(not
> >>>>>>> exit) when system enter suspend, then power on them again (needn't
> >>>>>>> init, otherwise device will be disconnected) when system resume, this
> >>>>>>> can avoid re-enumerating device."
> >>>>>>>
> >>>>>>>>> +             if (err)
> >>>>>>>>> +                     goto err_exit_phys;
> >>>>>>>>> +     }
> >>>>>>>>> +
> >>>>>>>>> +     return phy_roothub;
> >>>>>>>>> +
> >>>>>>>>> +err_exit_phys:
> >>>>>>>>> +     list_for_each_entry_continue_reverse(roothub_entry, head, list)
> >>>>>>>>> +             phy_exit(roothub_entry->phy);
> >>>>>>>>> +
> >>>>>>>>> +err_out:
> >>>>>>>>> +     return ERR_PTR(err);
> >>>>>>>>> +}
> >>>>>>>>> +EXPORT_SYMBOL_GPL(usb_phy_roothub_init);
> >>>>>>>>> +
> >>>>>>>>> +int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub)
> >>>>>>>>> +{
> >>>>>>>>> +     struct usb_phy_roothub *roothub_entry;
> >>>>>>>>> +     struct list_head *head;
> >>>>>>>>> +     int err, ret = 0;
> >>>>>>>>> +
> >>>>>>>>> +     if (!phy_roothub)
> >>>>>>>>> +             return 0;
> >>>>>>>>> +
> >>>>>>>>> +     head = &phy_roothub->list;
> >>>>>>>>> +
> >>>>>>>>> +     list_for_each_entry(roothub_entry, head, list) {
> >>>>>>>>> +             err = phy_exit(roothub_entry->phy);
> >>>>>>>>> +             if (err)
> >>>>>>>>> +                     ret = ret;
> >>>>>>>>> +     }
> >>>>>>>>
> >>>>>>>> phy_exit() should be moved to usb_phy_roothub_poweroff() just after calling phy_power_off().
> >>>>>>> if I understood Chunfeng Yun correctly this will require
> >>>>>>> re-enumeration of the USB devices after a suspend/resume cycle on
> >>>>>>> Mediatek SoCs
> >>>>>>>
> >>>>>>
> >>>>>> OK. I suppose that there are 2 cases
> >>>>>> 1) Mediatek's case: USB controller context retained across suspend/resume.
> >>>>>> Remote wakeup probably required.
> >>>>>> No re-enumeration preferred after resume. phy_exit()/phy_init() must not be called
> >>>>>> during suspend/resume to keep PHY link active.
> >>>>> Documentation/devicetree/bindings/usb/mediatek,mtu3.txt indeed shows
> >>>>> that the parent of the USB controller can be marked as "wakeup-source"
> >>>>>
> >>>>>> 2) TI's case: low power important: USB context is lost, OK to re-enumerate.
> >>>>>> phy_exit()/phy_init() must be called during suspend/resume.
> >>>>> ACK
> >>>>>
> >>>>>>>> With that there is nothing else being done here. Shouldn't we be doing the equivalent of
> >>>>>>>> usb_phy_roothub_del_phy() and usb_phy_roothub_free() here?
> >>>>>>>>
> >>>>>>>>> +
> >>>>>>>>> +     return ret;
> >>>>>>>>> +}
> >>>>>>>>> +EXPORT_SYMBOL_GPL(usb_phy_roothub_exit);
> >>>>>>>>> +
> >>>>>>>>> +int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub)
> >>>>>>>>> +{
> >>>>>>>>> +     struct usb_phy_roothub *roothub_entry;
> >>>>>>>>> +     struct list_head *head;
> >>>>>>>>> +     int err;
> >>>>>>>>> +
> >>>>>>>>> +     if (!phy_roothub)
> >>>>>>>>> +             return 0;
> >>>>>>>>> +
> >>>>>>>>> +     head = &phy_roothub->list;
> >>>>>>>>> +
> >>>>>>>>> +     list_for_each_entry(roothub_entry, head, list) {
> >>>>>>>>> +             err = phy_power_on(roothub_entry->phy);
> >>>>>>>>> +             if (err)
> >>>>>>>>> +                     goto err_out;
> >>>>>>>>> +     }
> >>>>>>>>> +
> >>>>>>>>> +     return 0;
> >>>>>>>>> +
> >>>>>>>>> +err_out:
> >>>>>>>>> +     list_for_each_entry_continue_reverse(roothub_entry, head, list)
> >>>>>>>>> +             phy_power_off(roothub_entry->phy);
> >>>>>>>>> +
> >>>>>>>>> +     return err;
> >>>>>>>>> +}
> >>>>>>>>> +EXPORT_SYMBOL_GPL(usb_phy_roothub_power_on);
> >>>>>>>>> +
> >>>>>>>>> +void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub)
> >>>>>>>>> +{
> >>>>>>>>> +     struct usb_phy_roothub *roothub_entry;
> >>>>>>>>> +
> >>>>>>>>> +     if (!phy_roothub)
> >>>>>>>>> +             return;
> >>>>>>>>> +
> >>>>>>>>> +     list_for_each_entry_reverse(roothub_entry, &phy_roothub->list, list)
> >>>>>>>>> +             phy_power_off(roothub_entry->phy);
> >>>>>>>>
> >>>>>>>> Not doing the phy_exit() here leaves the clocks enabled on our SoC and
> >>>>>>>> we're no longer able to reach low power states on system suspend.
> >>>>>>> I'm not sure where this problem should be solved:
> >>>>>>> - set skip_phy_initialization in struct usb_hcd to 1 for the affected
> >>>>>>> TI platforms
> >>>>>>
> >>>>>> Many TI platforms are affected, omap5*, dra7*, am43*
> >>>>>>
> >>>>>>> - fix this in the usb_phy_roothub code
> >>>>>>
> >>>>>> I'd vote for fixing it in the usb_phy_roothub code. How?
> >>>>>> How about using the device_can_wakeup() to decide if we should call phy_exit()/init() or not?
> >>>>>> If the USB device can't wakeup the system there is no point in keeping it powered/clocked right?
> >>>>> @Chunfeng: can you confirm Roger's idea that we could call phy_exit if
> >>>>> the controller is *NOT* marked as "wakeup-source"?
> >>>>> I am also not sure if it would work, since the "wakeup-source"
> >>>>> property is defined on the USB controller's parent node in case of the
> >>>>> Mediatek MTU3 (Mediatek USB3.0 DRD) controller
> >>>>>
> >>>> Very sorry, I forgot that MTU3 & xHCI drivers always set them as wakeup
> >>>> devices by device_init_wakeup(dev, true),but not dependent on
> >>>> "wakeup-source" property, so maybe we can use device_can_wakeup() to
> >>>> decide whether call phy_exit()/init() or not.
> >>> the 64-bit Amlogic Meson GXL/GXM SoCs don't support suspend/resume
> >>> yet, so I cannot test this
> >>> based on this suggestion I threw up two patches which are *compile
> >>> tested only* based on Greg's usb-next branch
> >>> you can find them here: [0] (as well as attached to this mail)
> >>>
> >>> @Chunfeng: can you please test this on one of your Mediatek SoCs?
> >>> @Roger: can you please test this on a TI SoC?
> >>>
> >>> (apologies in advance if these patches don't work)
> >>>
> >>> please note that I won't have access to my computer until Saturday.
> >>> if these patches need to be rewritten/replaced/etc. then feel free to
> >>> send your own version to the list
> >>
> >> Had to do the following to build
> >>
> >> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> >> index 6d4a419..2884607 100644
> >> --- a/drivers/usb/core/hcd.c
> >> +++ b/drivers/usb/core/hcd.c
> >> @@ -2262,7 +2262,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->phy_roothub);
> >> +			usb_phy_roothub_suspend(&rhdev->dev, hcd->phy_roothub);
> > 
> > Try to use hcd->self.controller instead of &rhdev->dev;
> 
> Actually it should be hcd->self.sysdev.
I find that xhci-plat.c enables wakeup by hcd->self.controller which is
different from hcd->self.sysdev sometimes.

And hcd->self.controller is the same as hcd->self.sysdev on MTK's
platform.

> > 
> >>  
> >>  		/* Did we race with a root-hub wakeup event? */
> >>  		if (rhdev->do_remote_wakeup) {
> >> @@ -2302,7 +2302,7 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
> >>  	}
> >>  
> >>  	if (!PMSG_IS_AUTO(msg)) {
> >> -		status = usb_phy_roothub_resume(hcd->phy_roothub);
> >> +		status = usb_phy_roothub_resume(&rhdev->dev, hcd->phy_roothub);
> > ditto
> >>  		if (status)
> >>  			return status;
> >>  	}
> >> @@ -2344,7 +2344,7 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
> >>  		}
> >>  	} else {
> >>  		hcd->state = old_state;
> >> -		usb_phy_roothub_suspend(hcd->phy_roothub);
> >> +		usb_phy_roothub_suspend(&rhdev->dev, hcd->phy_roothub);
> > ditto
> >>  		dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
> >>  				"resume", status);
> >>  		if (status != -ESHUTDOWN)
> >>
> >>
> >> And the following to fix runtime issues
> >>
> >> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
> >> index 2eca371..8598906 100644
> >> --- a/drivers/usb/core/phy.c
> >> +++ b/drivers/usb/core/phy.c
> >> @@ -32,9 +32,9 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index,
> >>  			return PTR_ERR(phy);
> >>  	}
> >>  
> >> -	roothub_entry = usb_phy_roothub_alloc(dev);
> >> -	if (IS_ERR(roothub_entry))
> >> -		return PTR_ERR(roothub_entry);
> >> +	roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
> >> +	if (!roothub_entry)
> >> +		return -ENOMEM;
> >>  
> >>  	roothub_entry->phy = phy;
> >>  
> >> @@ -162,7 +162,7 @@ int usb_phy_roothub_suspend(struct device *dev,
> >>  	usb_phy_roothub_power_off(phy_roothub);
> >>  
> >>  	/* keep the PHYs initialized so the device can wake up the system */
> >> -	if (device_can_wakeup(dev))
> >> +	if (device_may_wakeup(dev))
> > It's ok
> 
> I had to additionally fix usb_phy_roothub_resume() from
> 	if (device_can_wakeup(dev))
> to
> 	if (!device_may_wakeup(dev))
> 
> >>  		return 0;
> >>  
> >>  	return usb_phy_roothub_exit(phy_roothub);
> >>
> >>
> >> Here are my obervations
> >> - if wakeup is disabled it works fine as expected, phy_exit() is called and I'm able to reach
> >> low power states.
> >>
> >> - if wakeup is enabled (/sys/bus/usb/device/usb2/power/wakeup), then hcd_bus_suspend() is never called
> >> and so phy_power_off won't be called either.
> >>
> >> This means that the device_may_wakeup() check is redundant. Sorry for suggesting this.
> > You maybe use a wrong device.
> 
> Yes, after using the correct device I don't see the problem.
> 
> Can you please try the below patch on top of the 2 Patches that Martin sent and the new patch you sent?
> Once you confirm it works for you I can send the 2 patches officially.
It works on MTK's platform, I'll test it again when official patches are
sent out.

Thanks a lot
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 6d4a419..04cc453 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2262,7 +2262,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->phy_roothub);
> +			usb_phy_roothub_suspend(hcd->self.sysdev, hcd->phy_roothub);
>  
>  		/* Did we race with a root-hub wakeup event? */
>  		if (rhdev->do_remote_wakeup) {
> @@ -2302,7 +2302,7 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
>  	}
>  
>  	if (!PMSG_IS_AUTO(msg)) {
> -		status = usb_phy_roothub_resume(hcd->phy_roothub);
> +		status = usb_phy_roothub_resume(hcd->self.sysdev, hcd->phy_roothub);
>  		if (status)
>  			return status;
>  	}
> @@ -2344,7 +2344,7 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
>  		}
>  	} else {
>  		hcd->state = old_state;
> -		usb_phy_roothub_suspend(hcd->phy_roothub);
> +		usb_phy_roothub_suspend(hcd->self.sysdev, hcd->phy_roothub);
>  		dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
>  				"resume", status);
>  		if (status != -ESHUTDOWN)
> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
> index 2eca371..25fe729 100644
> --- a/drivers/usb/core/phy.c
> +++ b/drivers/usb/core/phy.c
> @@ -32,9 +32,9 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index,
>  			return PTR_ERR(phy);
>  	}
>  
> -	roothub_entry = usb_phy_roothub_alloc(dev);
> -	if (IS_ERR(roothub_entry))
> -		return PTR_ERR(roothub_entry);
> +	roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
> +	if (!roothub_entry)
> +		return -ENOMEM;
>  
>  	roothub_entry->phy = phy;
>  
> @@ -162,7 +162,7 @@ int usb_phy_roothub_suspend(struct device *dev,
>  	usb_phy_roothub_power_off(phy_roothub);
>  
>  	/* keep the PHYs initialized so the device can wake up the system */
> -	if (device_can_wakeup(dev))
> +	if (device_may_wakeup(dev))
>  		return 0;
>  
>  	return usb_phy_roothub_exit(phy_roothub);
> @@ -175,7 +175,7 @@ int usb_phy_roothub_resume(struct device *dev,
>  	int err;
>  
>  	/* if the device can't wake up the system _exit was called */
> -	if (device_can_wakeup(dev)) {
> +	if (!device_may_wakeup(dev)) {
>  		err = usb_phy_roothub_init(phy_roothub);
>  		if (err)
>  			return err;
diff mbox

Patch

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 6d4a419..04cc453 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2262,7 +2262,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->phy_roothub);
+			usb_phy_roothub_suspend(hcd->self.sysdev, hcd->phy_roothub);
 
 		/* Did we race with a root-hub wakeup event? */
 		if (rhdev->do_remote_wakeup) {
@@ -2302,7 +2302,7 @@  int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
 	}
 
 	if (!PMSG_IS_AUTO(msg)) {
-		status = usb_phy_roothub_resume(hcd->phy_roothub);
+		status = usb_phy_roothub_resume(hcd->self.sysdev, hcd->phy_roothub);
 		if (status)
 			return status;
 	}
@@ -2344,7 +2344,7 @@  int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
 		}
 	} else {
 		hcd->state = old_state;
-		usb_phy_roothub_suspend(hcd->phy_roothub);
+		usb_phy_roothub_suspend(hcd->self.sysdev, hcd->phy_roothub);
 		dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
 				"resume", status);
 		if (status != -ESHUTDOWN)
diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
index 2eca371..25fe729 100644
--- a/drivers/usb/core/phy.c
+++ b/drivers/usb/core/phy.c
@@ -32,9 +32,9 @@  static int usb_phy_roothub_add_phy(struct device *dev, int index,
 			return PTR_ERR(phy);
 	}
 
-	roothub_entry = usb_phy_roothub_alloc(dev);
-	if (IS_ERR(roothub_entry))
-		return PTR_ERR(roothub_entry);
+	roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
+	if (!roothub_entry)
+		return -ENOMEM;
 
 	roothub_entry->phy = phy;
 
@@ -162,7 +162,7 @@  int usb_phy_roothub_suspend(struct device *dev,
 	usb_phy_roothub_power_off(phy_roothub);
 
 	/* keep the PHYs initialized so the device can wake up the system */
-	if (device_can_wakeup(dev))
+	if (device_may_wakeup(dev))
 		return 0;
 
 	return usb_phy_roothub_exit(phy_roothub);
@@ -175,7 +175,7 @@  int usb_phy_roothub_resume(struct device *dev,
 	int err;
 
 	/* if the device can't wake up the system _exit was called */
-	if (device_can_wakeup(dev)) {
+	if (!device_may_wakeup(dev)) {
 		err = usb_phy_roothub_init(phy_roothub);
 		if (err)
 			return err;