2021-12-08 10:05:57

by Neal Liu

[permalink] [raw]
Subject: [PATCH v3 0/4] Refactor Aspeed USB vhub driver

These patch series include 2 parts. One is adding more features
to pass USB30CV compliance test, the other is fixing hw issues.
More detail descriptions are included below patchsets.

Change since v2:
- Add more description in changelog.
- Fix remote wakeup issue patch and make it more configurable.

Change since v1:
- Remove unnecessary configs for SET_CONFIGURATION.
- Separate supporting test mode to new patch.

*** BLURB HERE ***

Neal Liu (4):
usb: aspeed-vhub: add qualifier descriptor
usb: aspeed-vhub: fix remote wakeup failure in iKVM use case
usb: aspeed-vhub: fix ep0 OUT ack received wrong length issue
usb: aspeed-vhub: support test mode feature

drivers/usb/gadget/udc/aspeed-vhub/dev.c | 19 +++++++--
drivers/usb/gadget/udc/aspeed-vhub/ep0.c | 7 ++++
drivers/usb/gadget/udc/aspeed-vhub/epn.c | 5 +++
drivers/usb/gadget/udc/aspeed-vhub/hub.c | 47 ++++++++++++++++++++---
drivers/usb/gadget/udc/aspeed-vhub/vhub.h | 1 +
5 files changed, 69 insertions(+), 10 deletions(-)

--
2.25.1



2021-12-08 10:06:03

by Neal Liu

[permalink] [raw]
Subject: [PATCH v3 2/4] usb: aspeed-vhub: fix remote wakeup failure in iKVM use case

Signaling remote wakeup if an emulated USB device has any activity
if the device is allowed by host.

Signed-off-by: Neal Liu <[email protected]>
---
drivers/usb/gadget/udc/aspeed-vhub/epn.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
index 917892ca8753..ccc239b5cc17 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
@@ -381,6 +381,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,
--
2.25.1


2021-12-08 10:06:11

by Neal Liu

[permalink] [raw]
Subject: [PATCH v3 3/4] usb: aspeed-vhub: fix ep0 OUT ack received wrong length issue

If multiple devices in vhub are enumerated simultaneously, ep0 OUT
ack might received wrong data length. Using expected data length
instead.

Acked-by: Benjamin Herrenschmidt <[email protected]>
Signed-off-by: Neal Liu <[email protected]>
---
drivers/usb/gadget/udc/aspeed-vhub/ep0.c | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/drivers/usb/gadget/udc/aspeed-vhub/ep0.c b/drivers/usb/gadget/udc/aspeed-vhub/ep0.c
index 74ea36c19b1e..b4cf46249fea 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/ep0.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/ep0.c
@@ -251,6 +251,13 @@ static void ast_vhub_ep0_do_receive(struct ast_vhub_ep *ep, struct ast_vhub_req
len = remain;
rc = -EOVERFLOW;
}
+
+ /* Hardware return wrong data len */
+ if (len < ep->ep.maxpacket && len != remain) {
+ EPDBG(ep, "using expected data len instead\n");
+ len = remain;
+ }
+
if (len && req->req.buf)
memcpy(req->req.buf + req->req.actual, ep->buf, len);
req->req.actual += len;
--
2.25.1


2021-12-08 10:06:14

by Neal Liu

[permalink] [raw]
Subject: [PATCH v3 1/4] usb: aspeed-vhub: add qualifier descriptor

USB3 Command Verifier (USB3CV) is the official tool for
USB3 Hub and Device Framework testing.

A high-speed capable device that has different device information
for full-speed and high-speed must have a Device Qualifier Descriptor.

This patch is to support device qualifier to pass
USB3CV - Chapter 9 Test [USB 2 devices] - Device Qualifier Tests.

Signed-off-by: Neal Liu <[email protected]>
---
drivers/usb/gadget/udc/aspeed-vhub/hub.c | 24 +++++++++++++++++++++++
drivers/usb/gadget/udc/aspeed-vhub/vhub.h | 1 +
2 files changed, 25 insertions(+)

diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
index b9960fdd8a51..93f27a745760 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
@@ -68,6 +68,18 @@ static const struct usb_device_descriptor ast_vhub_dev_desc = {
.bNumConfigurations = 1,
};

+static const struct usb_qualifier_descriptor ast_vhub_qual_desc = {
+ .bLength = 0xA,
+ .bDescriptorType = USB_DT_DEVICE_QUALIFIER,
+ .bcdUSB = cpu_to_le16(0x0200),
+ .bDeviceClass = USB_CLASS_HUB,
+ .bDeviceSubClass = 0,
+ .bDeviceProtocol = 0,
+ .bMaxPacketSize0 = 64,
+ .bNumConfigurations = 1,
+ .bRESERVED = 0,
+};
+
/*
* Configuration descriptor: same comments as above
* regarding handling USB1 mode.
@@ -271,9 +283,11 @@ static int ast_vhub_rep_desc(struct ast_vhub_ep *ep,
BUILD_BUG_ON(dsize > sizeof(vhub->vhub_dev_desc));
BUILD_BUG_ON(USB_DT_DEVICE_SIZE >= AST_VHUB_EP0_MAX_PACKET);
break;
+ case USB_DT_OTHER_SPEED_CONFIG:
case USB_DT_CONFIG:
dsize = AST_VHUB_CONF_DESC_SIZE;
memcpy(ep->buf, &vhub->vhub_conf_desc, dsize);
+ ((u8 *)ep->buf)[1] = desc_type;
BUILD_BUG_ON(dsize > sizeof(vhub->vhub_conf_desc));
BUILD_BUG_ON(AST_VHUB_CONF_DESC_SIZE >= AST_VHUB_EP0_MAX_PACKET);
break;
@@ -283,6 +297,10 @@ static int ast_vhub_rep_desc(struct ast_vhub_ep *ep,
BUILD_BUG_ON(dsize > sizeof(vhub->vhub_hub_desc));
BUILD_BUG_ON(AST_VHUB_HUB_DESC_SIZE >= AST_VHUB_EP0_MAX_PACKET);
break;
+ case USB_DT_DEVICE_QUALIFIER:
+ dsize = sizeof(vhub->vhub_qual_desc);
+ memcpy(ep->buf, &vhub->vhub_qual_desc, dsize);
+ break;
default:
return std_req_stall;
}
@@ -428,6 +446,8 @@ enum std_req_rc ast_vhub_std_hub_request(struct ast_vhub_ep *ep,
switch (wValue >> 8) {
case USB_DT_DEVICE:
case USB_DT_CONFIG:
+ case USB_DT_DEVICE_QUALIFIER:
+ case USB_DT_OTHER_SPEED_CONFIG:
return ast_vhub_rep_desc(ep, wValue >> 8,
wLength);
case USB_DT_STRING:
@@ -1033,6 +1053,10 @@ static int ast_vhub_init_desc(struct ast_vhub *vhub)
else
ret = ast_vhub_str_alloc_add(vhub, &ast_vhub_strings);

+ /* Initialize vhub Qualifier Descriptor. */
+ memcpy(&vhub->vhub_qual_desc, &ast_vhub_qual_desc,
+ sizeof(vhub->vhub_qual_desc));
+
return ret;
}

diff --git a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
index 87a5dea12d3c..6b9dfa6e10eb 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
+++ b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
@@ -425,6 +425,7 @@ struct ast_vhub {
struct ast_vhub_full_cdesc vhub_conf_desc;
struct usb_hub_descriptor vhub_hub_desc;
struct list_head vhub_str_desc;
+ struct usb_qualifier_descriptor vhub_qual_desc;
};

/* Standard request handlers result codes */
--
2.25.1


2021-12-08 10:06:22

by Neal Liu

[permalink] [raw]
Subject: [PATCH v3 4/4] usb: aspeed-vhub: support test mode feature

Support aspeed usb vhub set feature to test mode.

Signed-off-by: Neal Liu <[email protected]>
---
drivers/usb/gadget/udc/aspeed-vhub/dev.c | 19 +++++++++++++++----
drivers/usb/gadget/udc/aspeed-vhub/hub.c | 23 +++++++++++++++++------
2 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/gadget/udc/aspeed-vhub/dev.c b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
index d918e8b2af3c..b0dfca43fbdc 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/dev.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
@@ -110,15 +110,26 @@ 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;
+ 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 93f27a745760..65cd4e46f031 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
@@ -212,17 +212,28 @@ 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");
+ 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,
--
2.25.1


2021-12-08 12:21:53

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH v3 0/4] Refactor Aspeed USB vhub driver

On Wed, Dec 08, 2021 at 06:05:41PM +0800, Neal Liu wrote:
> These patch series include 2 parts. One is adding more features
> to pass USB30CV compliance test, the other is fixing hw issues.
> More detail descriptions are included below patchsets.
>
> Change since v2:
> - Add more description in changelog.
> - Fix remote wakeup issue patch and make it more configurable.
>
> Change since v1:
> - Remove unnecessary configs for SET_CONFIGURATION.
> - Separate supporting test mode to new patch.
>
> *** BLURB HERE ***

Blurb is missing :(

2021-12-09 00:09:50

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: [PATCH v3 1/4] usb: aspeed-vhub: add qualifier descriptor

On Wed, 2021-12-08 at 18:05 +0800, Neal Liu wrote:
> USB3 Command Verifier (USB3CV) is the official tool for
> USB3 Hub and Device Framework testing.
>
> A high-speed capable device that has different device information
> for full-speed and high-speed must have a Device Qualifier
> Descriptor.
>
> This patch is to support device qualifier to pass
> USB3CV - Chapter 9 Test [USB 2 devices] - Device Qualifier Tests.
>
> Signed-off-by: Neal Liu <[email protected]>
> ---

Acked-by: Benjamin Herrenschmidt <[email protected]>

> drivers/usb/gadget/udc/aspeed-vhub/hub.c | 24
> +++++++++++++++++++++++
> drivers/usb/gadget/udc/aspeed-vhub/vhub.h | 1 +
> 2 files changed, 25 insertions(+)
>
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> index b9960fdd8a51..93f27a745760 100644
> --- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> @@ -68,6 +68,18 @@ static const struct usb_device_descriptor
> ast_vhub_dev_desc = {
> .bNumConfigurations = 1,
> };
>
> +static const struct usb_qualifier_descriptor ast_vhub_qual_desc = {
> + .bLength = 0xA,
> + .bDescriptorType = USB_DT_DEVICE_QUALIFIER,
> + .bcdUSB = cpu_to_le16(0x0200),
> + .bDeviceClass = USB_CLASS_HUB,
> + .bDeviceSubClass = 0,
> + .bDeviceProtocol = 0,
> + .bMaxPacketSize0 = 64,
> + .bNumConfigurations = 1,
> + .bRESERVED = 0,
> +};
> +
> /*
> * Configuration descriptor: same comments as above
> * regarding handling USB1 mode.
> @@ -271,9 +283,11 @@ static int ast_vhub_rep_desc(struct ast_vhub_ep
> *ep,
> BUILD_BUG_ON(dsize > sizeof(vhub->vhub_dev_desc));
> BUILD_BUG_ON(USB_DT_DEVICE_SIZE >=
> AST_VHUB_EP0_MAX_PACKET);
> break;
> + case USB_DT_OTHER_SPEED_CONFIG:
> case USB_DT_CONFIG:
> dsize = AST_VHUB_CONF_DESC_SIZE;
> memcpy(ep->buf, &vhub->vhub_conf_desc, dsize);
> + ((u8 *)ep->buf)[1] = desc_type;
> BUILD_BUG_ON(dsize > sizeof(vhub->vhub_conf_desc));
> BUILD_BUG_ON(AST_VHUB_CONF_DESC_SIZE >=
> AST_VHUB_EP0_MAX_PACKET);
> break;
> @@ -283,6 +297,10 @@ static int ast_vhub_rep_desc(struct ast_vhub_ep
> *ep,
> BUILD_BUG_ON(dsize > sizeof(vhub->vhub_hub_desc));
> BUILD_BUG_ON(AST_VHUB_HUB_DESC_SIZE >=
> AST_VHUB_EP0_MAX_PACKET);
> break;
> + case USB_DT_DEVICE_QUALIFIER:
> + dsize = sizeof(vhub->vhub_qual_desc);
> + memcpy(ep->buf, &vhub->vhub_qual_desc, dsize);
> + break;
> default:
> return std_req_stall;
> }
> @@ -428,6 +446,8 @@ enum std_req_rc ast_vhub_std_hub_request(struct
> ast_vhub_ep *ep,
> switch (wValue >> 8) {
> case USB_DT_DEVICE:
> case USB_DT_CONFIG:
> + case USB_DT_DEVICE_QUALIFIER:
> + case USB_DT_OTHER_SPEED_CONFIG:
> return ast_vhub_rep_desc(ep, wValue >> 8,
> wLength);
> case USB_DT_STRING:
> @@ -1033,6 +1053,10 @@ static int ast_vhub_init_desc(struct ast_vhub
> *vhub)
> else
> ret = ast_vhub_str_alloc_add(vhub, &ast_vhub_strings);
>
> + /* Initialize vhub Qualifier Descriptor. */
> + memcpy(&vhub->vhub_qual_desc, &ast_vhub_qual_desc,
> + sizeof(vhub->vhub_qual_desc));
> +
> return ret;
> }
>
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> index 87a5dea12d3c..6b9dfa6e10eb 100644
> --- a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> @@ -425,6 +425,7 @@ struct ast_vhub {
> struct ast_vhub_full_cdesc vhub_conf_desc;
> struct usb_hub_descriptor vhub_hub_desc;
> struct list_head vhub_str_desc;
> + struct usb_qualifier_descriptor vhub_qual_desc;
> };
>
> /* Standard request handlers result codes */


2021-12-09 00:10:58

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: [PATCH v3 2/4] usb: aspeed-vhub: fix remote wakeup failure in iKVM use case

On Wed, 2021-12-08 at 18:05 +0800, Neal Liu wrote:
> Signaling remote wakeup if an emulated USB device has any activity
> if the device is allowed by host.
>
> Signed-off-by: Neal Liu <[email protected]>

I still think it should fundamentally be the device making that
decision, but since they don't, this is an acceptable workaround, but
please, don't write the MMIO on every EP queue. Either keep track of
the bus being suspended, or turn on the AUTO bit in HW when wakeup_en
is set.

Cheers,
Ben.

> ---
> drivers/usb/gadget/udc/aspeed-vhub/epn.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> index 917892ca8753..ccc239b5cc17 100644
> --- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> @@ -381,6 +381,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,


2021-12-09 00:11:37

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: [PATCH v3 4/4] usb: aspeed-vhub: support test mode feature

On Wed, 2021-12-08 at 18:05 +0800, Neal Liu wrote:
> Support aspeed usb vhub set feature to test mode.
>
> Signed-off-by: Neal Liu <[email protected]>

Acked-by: Benjamin Herrenschmidt <[email protected]>

> ---
> drivers/usb/gadget/udc/aspeed-vhub/dev.c | 19 +++++++++++++++----
> drivers/usb/gadget/udc/aspeed-vhub/hub.c | 23 +++++++++++++++++-----
> -
> 2 files changed, 32 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> index d918e8b2af3c..b0dfca43fbdc 100644
> --- a/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> @@ -110,15 +110,26 @@ 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;
> + 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 93f27a745760..65cd4e46f031 100644
> --- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> @@ -212,17 +212,28 @@ 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");
> + 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,


2021-12-09 02:37:21

by Neal Liu

[permalink] [raw]
Subject: RE: [PATCH v3 2/4] usb: aspeed-vhub: fix remote wakeup failure in iKVM use case

> -----Original Message-----
> From: Benjamin Herrenschmidt <[email protected]>
> Sent: Thursday, December 9, 2021 8:05 AM
> To: Neal Liu <[email protected]>; Felipe Balbi <[email protected]>;
> Greg Kroah-Hartman <[email protected]>; Joel Stanley
> <[email protected]>; Andrew Jeffery <[email protected]>; Cai Huoqing
> <[email protected]>; Tao Ren <[email protected]>; Julia Lawall
> <[email protected]>; kernel test robot <[email protected]>; Sasha Levin
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]
> Cc: BMC-SW <[email protected]>
> Subject: Re: [PATCH v3 2/4] usb: aspeed-vhub: fix remote wakeup failure in
> iKVM use case
>
> On Wed, 2021-12-08 at 18:05 +0800, Neal Liu wrote:
> > Signaling remote wakeup if an emulated USB device has any activity if
> > the device is allowed by host.
> >
> > Signed-off-by: Neal Liu <[email protected]>
>
> I still think it should fundamentally be the device making that decision, but
> since they don't, this is an acceptable workaround, but please, don't write the
> MMIO on every EP queue. Either keep track of the bus being suspended, or
> turn on the AUTO bit in HW when wakeup_en is set.
>
> Cheers,
> Ben.
>

I'm confused. Signaling Wakeup when wakeup_en is set if it has any ep activities is not exactly what you said?
wakeup_en is set only if host allows this device have wakeup capability and bus being suspended.
Normal ep activities would not write the MMIO because wakeup_en is not set.
Thanks

-Neal

> > ---
> > drivers/usb/gadget/udc/aspeed-vhub/epn.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > index 917892ca8753..ccc239b5cc17 100644
> > --- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > @@ -381,6 +381,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,

2021-12-09 02:40:02

by Neal Liu

[permalink] [raw]
Subject: RE: [PATCH v3 0/4] Refactor Aspeed USB vhub driver

> -----Original Message-----
> From: Greg Kroah-Hartman <[email protected]>
> Sent: Wednesday, December 8, 2021 8:22 PM
> To: Neal Liu <[email protected]>
> Cc: Felipe Balbi <[email protected]>; Joel Stanley <[email protected]>; Andrew
> Jeffery <[email protected]>; Cai Huoqing <[email protected]>; Tao Ren
> <[email protected]>; Julia Lawall <[email protected]>; kernel test
> robot <[email protected]>; Sasha Levin <[email protected]>;
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; BMC-SW <[email protected]>
> Subject: Re: [PATCH v3 0/4] Refactor Aspeed USB vhub driver
>
> On Wed, Dec 08, 2021 at 06:05:41PM +0800, Neal Liu wrote:
> > These patch series include 2 parts. One is adding more features to
> > pass USB30CV compliance test, the other is fixing hw issues.
> > More detail descriptions are included below patchsets.
> >
> > Change since v2:
> > - Add more description in changelog.
> > - Fix remote wakeup issue patch and make it more configurable.
> >
> > Change since v1:
> > - Remove unnecessary configs for SET_CONFIGURATION.
> > - Separate supporting test mode to new patch.
> >
> > *** BLURB HERE ***
>
> Blurb is missing :(

I would remove this comment if new patch is necessary.
Thanks

-Neal

2021-12-13 14:01:40

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH v3 2/4] usb: aspeed-vhub: fix remote wakeup failure in iKVM use case

On Wed, Dec 08, 2021 at 06:05:43PM +0800, Neal Liu wrote:
> Signaling remote wakeup if an emulated USB device has any activity
> if the device is allowed by host.
>
> Signed-off-by: Neal Liu <[email protected]>
> ---
> drivers/usb/gadget/udc/aspeed-vhub/epn.c | 5 +++++
> 1 file changed, 5 insertions(+)

What commit does this fix?

Does it need to go to stable kernels?

Should it be independent of this patch series that adds new features?

thanks,

greg k-h

2021-12-14 02:41:19

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: [PATCH v3 2/4] usb: aspeed-vhub: fix remote wakeup failure in iKVM use case

On Thu, 2021-12-09 at 02:37 +0000, Neal Liu wrote:
> I'm confused. Signaling Wakeup when wakeup_en is set if it has any ep
> activities is not exactly what you said?
>
> wakeup_en is set only if host allows this device have wakeup
> capability and bus being suspended.
>
> Normal ep activities would not write the MMIO because wakeup_en is
> not set.

Hrm... I didn't think wakeup_en was limited to the bus being suspended,
but maybe I misremember, it's been a while.

Cheers,
Ben.



2021-12-20 02:23:08

by Neal Liu

[permalink] [raw]
Subject: RE: [PATCH v3 2/4] usb: aspeed-vhub: fix remote wakeup failure in iKVM use case

> -----Original Message-----
> From: Benjamin Herrenschmidt <[email protected]>
> Sent: Tuesday, December 14, 2021 10:36 AM
> To: Neal Liu <[email protected]>; Felipe Balbi <[email protected]>;
> Greg Kroah-Hartman <[email protected]>; Joel Stanley
> <[email protected]>; Andrew Jeffery <[email protected]>; Cai Huoqing
> <[email protected]>; Tao Ren <[email protected]>; Julia Lawall
> <[email protected]>; kernel test robot <[email protected]>; Sasha Levin
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]
> Cc: BMC-SW <[email protected]>
> Subject: Re: [PATCH v3 2/4] usb: aspeed-vhub: fix remote wakeup failure in
> iKVM use case
>
> On Thu, 2021-12-09 at 02:37 +0000, Neal Liu wrote:
> > I'm confused. Signaling Wakeup when wakeup_en is set if it has any ep
> > activities is not exactly what you said?
> >
> > wakeup_en is set only if host allows this device have wakeup
> > capability and bus being suspended.
> >
> > Normal ep activities would not write the MMIO because wakeup_en is not
> > set.
>
> Hrm... I didn't think wakeup_en was limited to the bus being suspended, but
> maybe I misremember, it's been a while.
>
> Cheers,
> Ben.
>

wakeup_en is only set in the case of host set USB_DEVICE_REMOTE_WAKEUP feature to vhub devices.
I think this behavior only occurs during host is going to suspend, and set this feature to any device which can wakeup itself before sleep.
Thanks

-Neal


2021-12-27 02:07:09

by Neal Liu

[permalink] [raw]
Subject: RE: [PATCH v3 2/4] usb: aspeed-vhub: fix remote wakeup failure in iKVM use case

> -----Original Message-----
> From: Neal Liu
> Sent: Monday, December 20, 2021 10:23 AM
> To: Benjamin Herrenschmidt <[email protected]>; Felipe Balbi
> <[email protected]>; Greg Kroah-Hartman <[email protected]>; Joel
> Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Cai Huoqing
> <[email protected]>; Tao Ren <[email protected]>; Julia Lawall
> <[email protected]>; kernel test robot <[email protected]>; Sasha Levin
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]
> Cc: BMC-SW <[email protected]>
> Subject: RE: [PATCH v3 2/4] usb: aspeed-vhub: fix remote wakeup failure in
> iKVM use case
>
> > -----Original Message-----
> > From: Benjamin Herrenschmidt <[email protected]>
> > Sent: Tuesday, December 14, 2021 10:36 AM
> > To: Neal Liu <[email protected]>; Felipe Balbi
> > <[email protected]>; Greg Kroah-Hartman <[email protected]>;
> > Joel Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Cai
> > Huoqing <[email protected]>; Tao Ren <[email protected]>; Julia
> > Lawall <[email protected]>; kernel test robot <[email protected]>;
> > Sasha Levin <[email protected]>; [email protected];
> > [email protected]; [email protected];
> > [email protected]
> > Cc: BMC-SW <[email protected]>
> > Subject: Re: [PATCH v3 2/4] usb: aspeed-vhub: fix remote wakeup
> > failure in iKVM use case
> >
> > On Thu, 2021-12-09 at 02:37 +0000, Neal Liu wrote:
> > > I'm confused. Signaling Wakeup when wakeup_en is set if it has any
> > > ep activities is not exactly what you said?
> > >
> > > wakeup_en is set only if host allows this device have wakeup
> > > capability and bus being suspended.
> > >
> > > Normal ep activities would not write the MMIO because wakeup_en is
> > > not set.
> >
> > Hrm... I didn't think wakeup_en was limited to the bus being
> > suspended, but maybe I misremember, it's been a while.
> >
> > Cheers,
> > Ben.
> >
>
> wakeup_en is only set in the case of host set USB_DEVICE_REMOTE_WAKEUP
> feature to vhub devices.
> I think this behavior only occurs during host is going to suspend, and set this
> feature to any device which can wakeup itself before sleep.
> Thanks
>
> -Neal
>
Would you like to test it if you have some free time.
Please feel free for any feedback.

-Neal

2022-01-10 06:29:23

by Neal Liu

[permalink] [raw]
Subject: RE: [PATCH v3 2/4] usb: aspeed-vhub: fix remote wakeup failure in iKVM use case

> -----Original Message-----
> From: Neal Liu
> Sent: Monday, December 27, 2021 10:07 AM
> To: Benjamin Herrenschmidt <[email protected]>; Felipe Balbi
> <[email protected]>; Greg Kroah-Hartman <[email protected]>; Joel
> Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Cai Huoqing
> <[email protected]>; Tao Ren <[email protected]>; Julia Lawall
> <[email protected]>; kernel test robot <[email protected]>; Sasha Levin
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]
> Cc: BMC-SW <[email protected]>
> Subject: RE: [PATCH v3 2/4] usb: aspeed-vhub: fix remote wakeup failure in
> iKVM use case
>
> > -----Original Message-----
> > From: Neal Liu
> > Sent: Monday, December 20, 2021 10:23 AM
> > To: Benjamin Herrenschmidt <[email protected]>; Felipe Balbi
> > <[email protected]>; Greg Kroah-Hartman <[email protected]>;
> > Joel Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Cai
> > Huoqing <[email protected]>; Tao Ren <[email protected]>; Julia
> > Lawall <[email protected]>; kernel test robot <[email protected]>;
> > Sasha Levin <[email protected]>; [email protected];
> > [email protected]; [email protected];
> > [email protected]
> > Cc: BMC-SW <[email protected]>
> > Subject: RE: [PATCH v3 2/4] usb: aspeed-vhub: fix remote wakeup
> > failure in iKVM use case
> >
> > > -----Original Message-----
> > > From: Benjamin Herrenschmidt <[email protected]>
> > > Sent: Tuesday, December 14, 2021 10:36 AM
> > > To: Neal Liu <[email protected]>; Felipe Balbi
> > > <[email protected]>; Greg Kroah-Hartman <[email protected]>;
> > > Joel Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Cai
> > > Huoqing <[email protected]>; Tao Ren <[email protected]>;
> > > Julia Lawall <[email protected]>; kernel test robot
> > > <[email protected]>; Sasha Levin <[email protected]>;
> > > [email protected]; [email protected];
> > > [email protected];
> > > [email protected]
> > > Cc: BMC-SW <[email protected]>
> > > Subject: Re: [PATCH v3 2/4] usb: aspeed-vhub: fix remote wakeup
> > > failure in iKVM use case
> > >
> > > On Thu, 2021-12-09 at 02:37 +0000, Neal Liu wrote:
> > > > I'm confused. Signaling Wakeup when wakeup_en is set if it has any
> > > > ep activities is not exactly what you said?
> > > >
> > > > wakeup_en is set only if host allows this device have wakeup
> > > > capability and bus being suspended.
> > > >
> > > > Normal ep activities would not write the MMIO because wakeup_en is
> > > > not set.
> > >
> > > Hrm... I didn't think wakeup_en was limited to the bus being
> > > suspended, but maybe I misremember, it's been a while.
> > >
> > > Cheers,
> > > Ben.
> > >
> >
> > wakeup_en is only set in the case of host set
> USB_DEVICE_REMOTE_WAKEUP
> > feature to vhub devices.
> > I think this behavior only occurs during host is going to suspend, and
> > set this feature to any device which can wakeup itself before sleep.
> > Thanks
> >
> > -Neal
> >
> Would you like to test it if you have some free time.
> Please feel free for any feedback.
>
> -Neal

Gentle ping for this patch.
Thanks