Message-ID: <20200917004758.GD3560556@google.com>
Date:   Wed, 16 Sep 2020 17:47:58 -0700
From:   Matthias Kaehlcke <mka@...omium.org>
To:     Peter Chen <peter.chen@....com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        Alan Stern <stern@...land.harvard.edu>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Bastien Nocera <hadess@...ess.net>,
        Ravi Chandra Sadineni <ravisadineni@...omium.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        Stephen Boyd <swboyd@...omium.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Douglas Anderson <dianders@...omium.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Alexander A. Klimov" <grandmaster@...klimov.de>,
        Masahiro Yamada <masahiroy@...nel.org>
Subject: Re: [PATCH 2/2] USB: misc: Add onboard_usb_hub driver

On Thu, Sep 17, 2020 at 12:27:29AM +0000, Peter Chen wrote:
> 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&amp;data=02%7C01%7Cpeter.chen%40nxp.com%7C02c4cc75e26a47d0224d08d85a74f945%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637358805725394858&amp;sdata=cjZhSmQiXVJoLsN5PjFACsLwsikH%2BeRTztPhsckJFNs%3D&amp;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&amp;data=02%7C01%7Cpeter.chen%40nxp.com%7C02c4cc75e26a47d0224d08d85a74f945%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637358805725394858&amp;sdata=6IjiiHJql%2FW4vzDla9q3qdfiiOzOQy1Vk7ryUhKOOTc%3D&amp;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.

Ok, thanks. So if I understand correctly the onboard hub driver should
check the wakeup state of the xHCI to determine if remote wakeup is
enabled for the controller (after all it doesn't know anything about
the platform device). Wakeup might not work properly if it is disabled
for the platform device, but it's the responsability of the board
software/config to make sure it is enabled (possibly this could be done
by making the dwc3-qcom driver understand the 'wakeup-source' property,
as the xhci-mtk driver does).

Powered by blists - more mailing lists