Re: [PATCH 2/2] USB: misc: Add onboard_usb_hub driver

[Peter Chen]

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