On 20-09-16 12:16:07, Matthias Kaehlcke wrote: > Hi Peter, > > On Wed, Sep 16, 2020 at 08:19:07AM +0000, Peter Chen wrote: > > On 20-09-15 16:03:45, Matthias Kaehlcke wrote: > > > Hi Peter, > > > > > > On Tue, Sep 15, 2020 at 07:05:38AM +0000, Peter Chen wrote: > > > > > > > > > > > + hub->cfg.power_off_in_suspend = > > > > > of_property_read_bool(dev->of_node, "power-off-in-suspend"); > > > > > > > + hub->cfg.wakeup_source = of_property_read_bool(dev->of_node, > > > > > > > +"wakeup-source"); > > > > > > > > > > > > Do you really need these two properties? If the device (and its > > > > > > children if existed) has wakeup enabled, you keep power in suspend, > > > > > > otherwise, you could close it, any exceptions? > > > > > > > > > > That would work for my use case, but I'm not sure it's a universally good > > > > > configuration. > > > > > > > > > > I don't have a specific USB device in mind, but you could have a device that > > > > > shouldn't lose it's context during suspend or keep operating autonomously (e.g. > > > > > a sensor with a large buffer collecting samples). Not sure if something like this > > > > > exists in the real though. > > > > > > > > > > I'm not an expert, but it seems there are USB controllers with wakeup support > > > > > which is always enabled. A board with such a controller then couldn't have a > > > > > policy to power down the hub regardless of wakeup capable devices being > > > > > connected. > > > > > > > > > > > > > Whether or not it is a wakeup_source, it could get through its or its children's > > > > /sys/../power/wakeup value, you have already used usb_wakeup_enabled_descendants > > > > to know it. > > > > > > I conceptually agree, but in practice there are some conflicting details: > > > > > > wakeup for the hubs on my system is by default disabled, yet USB wakeup works > > > regardless, so the flag doesn't really provide useful information. I guess we > > > could still use it if there is no better way, but it doesn't seem ideal. > > > > > > Similar for udev->bus->controller, according to sysfs it doesn't even have wakeup > > > support. Please let me know if there is a reliable way to check if wakeup is > > > enabled on the controller of a device. > > > > Then, how could your code work, you use usb_wakeup_enabled_descendants > > to get if HUB or the descendants under the HUB has wakeup enabled? > > Doing just that would not allow to switch the hub off when wakeup enabled > descendants are connected, which might be desirable in some configurations. > > > If you use dwc3, you need to enable xhci-plat.c's wakeup entry if your > > system needs xHCI connect/disconnect wakeup event. I have one pending > > patch to do it: > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Flinux-usb%2Fmsg199406.html&data=02%7C01%7Cpeter.chen%40nxp.com%7C02c4cc75e26a47d0224d08d85a74f945%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637358805725394858&sdata=cjZhSmQiXVJoLsN5PjFACsLwsikH%2BeRTztPhsckJFNs%3D&reserved=0 > > Thanks, my system has indeed a dwc3(-qcom) controller, your patch adds > the missing wakeup entry to sysfs. So it seems your patch should solve > my problem (sharp timing!), however you mention specifically the 'xHCI > connect/disconnect wakeup event', so I wonder if the xHCI wakeup flag > isn't applicable to other wakeup events. I know the dwc3-qcom platform > device has its own wakeup flag. The driver currently enables wakeup > interrupts unconditionally, I sent a patch to change that > (https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fpatchwork%2Fpatch%2F1305894%2F&data=02%7C01%7Cpeter.chen%40nxp.com%7C02c4cc75e26a47d0224d08d85a74f945%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637358805725394858&sdata=6IjiiHJql%2FW4vzDla9q3qdfiiOzOQy1Vk7ryUhKOOTc%3D&reserved=0), however I now wonder > if it should evaluate the xHCI wakeup flag instead of its own. > You may need both (glue & xhci), it depends on system design, and usually, these two kinds of wakeup setting isn't conflict. -- Thanks, Peter Chen
