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

-----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 [off-list ref]; Joel Stanley
[off-list ref]; Andrew Jeffery [off-list ref]; Cai Huoqing
[off-list ref]; Tao Ren [off-list ref]; Julia Lawall
[off-list ref]; kernel test robot [off-list ref]; Sasha Levin
[off-list ref]; 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:

quoted

quoted

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.

quoted

In this case, d->wakeup_en is not used.
Set VHUB_CTRL_AUTO_REMOTE_WAKEUP to enable HW automatically

signaling

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel