RE: [PATCH 2/3] usb: aspeed-vhub: support remote wakeup feature

[Neal Liu <neal_liu@aspeedtech.com>]

> -----Original Message-----
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Sent: Wednesday, December 1, 2021 7:38 AM
> To: Neal Liu <neal_liu@aspeedtech.com>; Felipe Balbi <balbi@kernel.org>;
> Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Joel Stanley
> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Cai Huoqing
> <caihuoqing@baidu.com>; Tao Ren <rentao.bupt@gmail.com>; Julia Lawall
> <julia.lawall@inria.fr>; kernel test robot <lkp@intel.com>; Sasha Levin
> <sashal@kernel.org>; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-aspeed@lists.ozlabs.org
> Subject: Re: [PATCH 2/3] usb: aspeed-vhub: support remote wakeup feature
> 
> On Tue, 2021-11-30 at 09:47 +0000, Neal Liu wrote:
> > > Should this  be controlled by d->wakeup_en ? IE, we have a feature
> > > for the host to enable/disable remote wakeup, should we honor it ?
> >
> > For KVM usage, remote keyboard packet would be sent if user wants to do
> remote wakeup.
> > In this case, d->wakeup_en is not used.
> > Set VHUB_CTRL_AUTO_REMOTE_WAKEUP to enable HW automatically
> signaling
> > wakeup if any packet would be transferred.
> 
> Sorry, I don't fully understand your explanation here.
> 
> Normally, a USB device will do remote wakeup if it's instructed to do so via the
> appropriate feature being set, which is what wakeup_en reflects. I hadn't
> originally plumbed it in, I forgot why, I think something was either not properly
> documented or not working when I wrote that driver.
> 
> You seem to want to override the behaviour and always send a remote wakeup
> packet no matter what. I am not sure this is desirable for all use cases, and
> might be something we want to make configurable, no ?
> 
> I'm trying to understand your sentence, you seem to imply that the only use
> case here is "KVM" (as in remote USB on a server system) which I can probably
> agree with... mostly.
> 
> And you say in that case, we should always do remote wakeup whenever an
> emulated USB device has any activity (keyboard or otherwise), regardless of
> whether the server has enabled the feature or not.
> 
> Am I correct ? What's the rationale here ?
> 
> Cheers,
> Ben.
> 

Let's me describe more details for our hardware behavior and hope you understand.

HUB00[3]: MANUAL_REMOTE_WAKEUP
HUB00[4]: AUTO_REMOTE_WAKEUP

Set HUB00[3] implies USB device will do remote wakeup if any write command to vhub register.
Set HUB00[4] implies USB device will do remote wakeup. It can only be set in suspend state.

For current design, d->wakeup_en only controls whether HUB00[4] can be set through usb_gadget_ops.wakeup().
If some applications (take KVM as example) want to wakeup host by sending a packet, it won't go through sb_gadget_ops.wakeup().
We enable HUB00[3] to fix this problem. It won't override above mentioned behavior.
If host has enabled the USB_DEVICE_REMOTE_WAKEUP feature, it has 2 ways to wakeup host.
1. set srp 1 (/sys/device/platform/xxxxxxxxx/udc/xxxxxx/srp)
2. emulated device has activity
If host has disabled the USB_DEVICE_REMOTE_WAKEUP feature, these 2 ways still cannot wakeup host even if USB bus is in resume state.
Thanks

-Neal