Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 16 Sep 2020 12:16:07 -0700
From:   Matthias Kaehlcke <mka@chromium.org>
To:     Peter Chen <peter.chen@nxp.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Rob Herring <robh+dt@kernel.org>,
        Frank Rowand <frowand.list@gmail.com>,
        Alan Stern <stern@rowland.harvard.edu>,
        Krzysztof Kozlowski <krzk@kernel.org>,
        Bastien Nocera <hadess@hadess.net>,
        Ravi Chandra Sadineni <ravisadineni@chromium.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        Stephen Boyd <swboyd@chromium.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        Douglas Anderson <dianders@chromium.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "Alexander A. Klimov" <grandmaster@al2klimov.de>,
        Masahiro Yamada <masahiroy@kernel.org>
Subject: Re: [PATCH 2/2] USB: misc: Add onboard_usb_hub driver
Message-ID: <20200916191607.GB3560556@google.com>
References: <20200914112716.1.I248292623d3d0f6a4f0c5bc58478ca3c0062b49a@changeid>
 <20200914112716.2.I7c9a1f1d6ced41dd8310e8a03da666a32364e790@changeid>
 <20200915025426.GA17450@b29397-desktop>
 <20200915050207.GF2022397@google.com>
 <AM7PR04MB715735A8A102F3EC9041EA328B200@AM7PR04MB7157.eurprd04.prod.outlook.com>
 <20200915230345.GF2771744@google.com>
 <20200916081821.GA14376@b29397-desktop>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20200916081821.GA14376@b29397-desktop>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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://www.spinics.net/lists/linux-usb/msg199406.html

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://lore.kernel.org/patchwork/patch/1305894/), however I now wonder
if it should evaluate the xHCI wakeup flag instead of its own.

Thanks

Matthias