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? 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://www.spinics.net/lists/linux-usb/msg199406.html > > > If the onboard HUB needs to reflect wakeup signal, it should not power off its regulator. > > > > For another property power-off-in-suspend, I think it is also a user option, > > but not a hardware feature. > > Ok, I think you are suggesting a sysfs attribute instead of a DT property, that > sounds good to me. Yes. -- Thanks, Peter Chen
