| Message ID | 20211126110954.2677627-3-neal_liu@aspeedtech.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] usb: aspeed-vhub: add qualifier descriptor | expand |
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.
> -----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
> -----Original Message----- > From: Neal Liu > Sent: Thursday, December 2, 2021 11:03 AM > To: Benjamin Herrenschmidt <benh@kernel.crashing.org>; 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 > Cc: BMC-SW <BMC-SW@aspeedtech.com> > Subject: 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 <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[4]: MANUAL_REMOTE_WAKEUP > HUB00[3]: AUTO_REMOTE_WAKEUP Correct bit number. > > 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 I also have another solution which you might be more acceptable without enabling HUB00[3]. I think I will resent this patch if you preferred. diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c b/drivers/usb/gadget/udc/aspeed-vhub/epn.c index 99b0a12d4dc0..1e0ac742c29b 100644 --- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c +++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c @@ -400,6 +400,11 @@ static int ast_vhub_epn_queue(struct usb_ep* u_ep, struct usb_request *u_req, } else u_req->dma = 0; + if (ep->dev->wakeup_en) { + EPVDBG(ep, "Wakeup host first\n"); + ast_vhub_hub_wake_all(vhub); + } + EPVDBG(ep, "enqueue req @%p\n", req); EPVDBG(ep, " l=%d dma=0x%x zero=%d noshort=%d noirq=%d is_in=%d\n", u_req->length, (u32)u_req->dma, u_req->zero, Thanks -Neal
On Thu, 2021-12-02 at 03:03 +0000, Neal Liu wrote: > > > 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 So what you are saying is that currently, the various gadgets aren't calling usb_gadget_wakeup() ? Ie. it should be a gadget policy to decide when to wake-up I suppose, but it's true that nothing in the core nor the existing gadgets seem to handle that. I think what you propose is a band-aid. The real problem is that the gadget drivers should trigger wakeups (or the core should do so on activity). That said, for now, I don't object to adding that "auto" bit, but I would prefer if that behaviour was use configurable. Cheers, Ben.
diff --git a/drivers/usb/gadget/udc/aspeed-vhub/core.c b/drivers/usb/gadget/udc/aspeed-vhub/core.c index 7a635c499777..122ee7ef0b03 100644 --- a/drivers/usb/gadget/udc/aspeed-vhub/core.c +++ b/drivers/usb/gadget/udc/aspeed-vhub/core.c @@ -240,6 +240,9 @@ void ast_vhub_init_hw(struct ast_vhub *vhub) if (vhub->force_usb1) ctrl |= VHUB_CTRL_FULL_SPEED_ONLY; + /* Enable auto remote wakeup */ + ctrl |= VHUB_CTRL_AUTO_REMOTE_WAKEUP; + ctrl |= VHUB_CTRL_UPSTREAM_CONNECT; writel(ctrl, vhub->regs + AST_VHUB_CTRL); diff --git a/drivers/usb/gadget/udc/aspeed-vhub/dev.c b/drivers/usb/gadget/udc/aspeed-vhub/dev.c index d918e8b2af3c..4462f4b73b04 100644 --- a/drivers/usb/gadget/udc/aspeed-vhub/dev.c +++ b/drivers/usb/gadget/udc/aspeed-vhub/dev.c @@ -110,15 +110,25 @@ static int ast_vhub_dev_feature(struct ast_vhub_dev *d, u16 wIndex, u16 wValue, bool is_set) { + u32 val; + DDBG(d, "%s_FEATURE(dev val=%02x)\n", is_set ? "SET" : "CLEAR", wValue); - if (wValue != USB_DEVICE_REMOTE_WAKEUP) - return std_req_driver; + if (wValue == USB_DEVICE_REMOTE_WAKEUP) { + d->wakeup_en = is_set; + return std_req_complete; - d->wakeup_en = is_set; + } else if (wValue == USB_DEVICE_TEST_MODE) { + val = readl(d->vhub->regs + AST_VHUB_CTRL); + val &= ~GENMASK(10, 8); + val |= VHUB_CTRL_SET_TEST_MODE((wIndex >> 8) & 0x7); + writel(val, d->vhub->regs + AST_VHUB_CTRL); - return std_req_complete; + return std_req_complete; + } + + return std_req_driver; } static int ast_vhub_ep_feature(struct ast_vhub_dev *d, diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c b/drivers/usb/gadget/udc/aspeed-vhub/hub.c index d76f83bc7762..ac589d3ba7a2 100644 --- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c +++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c @@ -212,17 +212,27 @@ static int ast_vhub_hub_dev_feature(struct ast_vhub_ep *ep, u16 wIndex, u16 wValue, bool is_set) { + u32 val; + EPDBG(ep, "%s_FEATURE(dev val=%02x)\n", is_set ? "SET" : "CLEAR", wValue); - if (wValue != USB_DEVICE_REMOTE_WAKEUP) - return std_req_stall; + if (wValue == USB_DEVICE_REMOTE_WAKEUP) { + ep->vhub->wakeup_en = is_set; + EPDBG(ep, "Hub remote wakeup %s\n", + is_set ? "enabled" : "disabled"); + return std_req_complete; - ep->vhub->wakeup_en = is_set; - EPDBG(ep, "Hub remote wakeup %s\n", - is_set ? "enabled" : "disabled"); + } else if (wValue == USB_DEVICE_TEST_MODE) { + val = readl(ep->vhub->regs + AST_VHUB_CTRL); + val &= ~GENMASK(10, 8); + val |= VHUB_CTRL_SET_TEST_MODE((wIndex >> 8) & 0x7); + writel(val, ep->vhub->regs + AST_VHUB_CTRL); - return std_req_complete; + return std_req_complete; + } + + return std_req_stall; } static int ast_vhub_hub_ep_feature(struct ast_vhub_ep *ep,
Remote wakeup signaling will be automatically issued whenever any write commands has been received in suspend state. Signed-off-by: Neal Liu <neal_liu@aspeedtech.com> --- drivers/usb/gadget/udc/aspeed-vhub/core.c | 3 +++ drivers/usb/gadget/udc/aspeed-vhub/dev.c | 18 ++++++++++++++---- drivers/usb/gadget/udc/aspeed-vhub/hub.c | 22 ++++++++++++++++------ 3 files changed, 33 insertions(+), 10 deletions(-)