[PATCH 0/3] Refactor Aspeed USB vhub driver

[PATCH 0/3] Refactor Aspeed USB vhub driver

[Neal Liu]

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.

*** BLURB HERE ***

Neal Liu (3):
  usb: aspeed-vhub: add qualifier descriptor
  usb: aspeed-vhub: support remote wakeup feature
  usb: aspeed-vhub: fix ep0 OUT ack received wrong length issue

 drivers/usb/gadget/udc/aspeed-vhub/core.c |  3 ++
 drivers/usb/gadget/udc/aspeed-vhub/dev.c  | 18 ++++++--
 drivers/usb/gadget/udc/aspeed-vhub/ep0.c  |  7 ++++
 drivers/usb/gadget/udc/aspeed-vhub/hub.c  | 51 +++++++++++++++++++----
 drivers/usb/gadget/udc/aspeed-vhub/vhub.h |  2 +
 5 files changed, 68 insertions(+), 13 deletions(-)

-- 
2.25.1

[PATCH 1/3] usb: aspeed-vhub: add qualifier descriptor

[Neal Liu]

Support qualifier descriptor to pass USB30CV compliance test.

Signed-off-by: Neal Liu <neal_liu@aspeedtech.com>
---
 drivers/usb/gadget/udc/aspeed-vhub/hub.c  | 29 ++++++++++++++++++++---
 drivers/usb/gadget/udc/aspeed-vhub/vhub.h |  2 ++
 2 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
index b9960fdd8a51..d76f83bc7762 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;
 	}
@@ -417,10 +435,9 @@ enum std_req_rc ast_vhub_std_hub_request(struct ast_vhub_ep *ep,
 
 		/* GET/SET_CONFIGURATION */
 	case DeviceRequest | USB_REQ_GET_CONFIGURATION:
-		return ast_vhub_simple_reply(ep, 1);
+		return ast_vhub_simple_reply(ep, vhub->current_config);
 	case DeviceOutRequest | USB_REQ_SET_CONFIGURATION:
-		if (wValue != 1)
-			return std_req_stall;
+		vhub->current_config = wValue;
 		return std_req_complete;
 
 		/* GET_DESCRIPTOR */
@@ -428,6 +445,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 +1052,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..e6a11a22422a 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
+++ b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
@@ -419,12 +419,14 @@ struct ast_vhub {
 
 	/* Upstream bus speed captured at bus reset */
 	unsigned int			speed;
+	u8				current_config;
 
 	/* Standard USB Descriptors of the vhub. */
 	struct usb_device_descriptor	vhub_dev_desc;
 	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

Re: [PATCH 1/3] usb: aspeed-vhub: add qualifier descriptor

[Benjamin Herrenschmidt]

On Fri, 2021-11-26 at 19:09 +0800, Neal Liu wrote:
> 
> @@ -417,10 +435,9 @@ enum std_req_rc ast_vhub_std_hub_request(struct ast_vhub_ep *ep,
>  
>  		/* GET/SET_CONFIGURATION */
>  	case DeviceRequest | USB_REQ_GET_CONFIGURATION:
> -		return ast_vhub_simple_reply(ep, 1);
> +		return ast_vhub_simple_reply(ep, vhub->current_config);
>  	case DeviceOutRequest | USB_REQ_SET_CONFIGURATION:
> -		if (wValue != 1)
> -			return std_req_stall;
> +		vhub->current_config = wValue;
>  		return std_req_complete;

This is odd.. why should we support arbitrary SET_CONFIGURATION for
configs we don't support ?

Otherwise looks good.

Cheers,
Ben.

RE: [PATCH 1/3] usb: aspeed-vhub: add qualifier descriptor

[Neal Liu]

> -----Original Message-----
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Sent: Tuesday, November 30, 2021 7:41 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
> Cc: BMC-SW <BMC-SW@aspeedtech.com>
> Subject: Re: [PATCH 1/3] usb: aspeed-vhub: add qualifier descriptor
> 
> On Fri, 2021-11-26 at 19:09 +0800, Neal Liu wrote:
> >
> > @@ -417,10 +435,9 @@ enum std_req_rc ast_vhub_std_hub_request(struct
> > ast_vhub_ep *ep,
> >
> >  		/* GET/SET_CONFIGURATION */
> >  	case DeviceRequest | USB_REQ_GET_CONFIGURATION:
> > -		return ast_vhub_simple_reply(ep, 1);
> > +		return ast_vhub_simple_reply(ep, vhub->current_config);
> >  	case DeviceOutRequest | USB_REQ_SET_CONFIGURATION:
> > -		if (wValue != 1)
> > -			return std_req_stall;
> > +		vhub->current_config = wValue;
> >  		return std_req_complete;
> 
> This is odd.. why should we support arbitrary SET_CONFIGURATION for configs
> we don't support ?
> 
> Otherwise looks good.
> 
> Cheers,
> Ben.
> 
This is unnecessary...
I'll remove it in next patch.
Thanks

-Neal

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

[Neal Liu]

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(-)

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,
-- 
2.25.1

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

[Benjamin Herrenschmidt]

On Fri, 2021-11-26 at 19:09 +0800, Neal Liu wrote:
> Remote wakeup signaling will be automatically issued
> whenever any write commands has been received in suspend
> state.

> --- 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);

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 ?
 
> +	} 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);

This is unrelated to remote wakeup is it ? In which case it should
probably be a separate patch.

Cheers,
Ben.

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

[Neal Liu]

> -----Original Message-----
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Sent: Tuesday, November 30, 2021 7:46 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
> Cc: BMC-SW <BMC-SW@aspeedtech.com>
> Subject: Re: [PATCH 2/3] usb: aspeed-vhub: support remote wakeup feature
> 
> On Fri, 2021-11-26 at 19:09 +0800, Neal Liu wrote:
> > Remote wakeup signaling will be automatically issued whenever any
> > write commands has been received in suspend state.
> 
> > --- 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);
> 
> 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.

> > +	} 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);
> 
> This is unrelated to remote wakeup is it ? In which case it should probably be a
> separate patch.
> 
> Cheers,
> Ben.
> 
Yes, I'll separate this patch.
Thanks

-Neal

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

[Benjamin Herrenschmidt]

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.

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

[Neal Liu]

> -----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

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

[Neal Liu]

> -----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

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

[Benjamin Herrenschmidt]

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.

[PATCH 3/3] usb: aspeed-vhub: fix ep0 OUT ack received wrong length issue

[Neal Liu]

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

Signed-off-by: Neal Liu <neal_liu@aspeedtech.com>
---
 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..bea9cbb191a2 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;
 	}
+
+	/* HW 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

Re: [PATCH 3/3] usb: aspeed-vhub: fix ep0 OUT ack received wrong length issue

[Benjamin Herrenschmidt]

On Fri, 2021-11-26 at 19:09 +0800, Neal Liu wrote:
> 
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/ep0.c
> b/drivers/usb/gadget/udc/aspeed-vhub/ep0.c
> index 74ea36c19b1e..bea9cbb191a2 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;
>  	}
> +
> +	/* HW return wrong data len */
> +	if (len < ep->ep.maxpacket && len != remain) {
> +		EPDBG(ep, "using expected data len instead\n");
> +		len = remain;
> +	}
> +

Wow, that is a nasty hw bug ! Patch looks good, I had to swap some of
that logic back into my brain but it looks like it won't break any
normal case :-)

Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Cheers,
Ben.

RE: [PATCH 3/3] usb: aspeed-vhub: fix ep0 OUT ack received wrong length issue

[Neal Liu]

> -----Original Message-----
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Sent: Tuesday, November 30, 2021 7:49 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
> Cc: BMC-SW <BMC-SW@aspeedtech.com>
> Subject: Re: [PATCH 3/3] usb: aspeed-vhub: fix ep0 OUT ack received wrong
> length issue
> 
> On Fri, 2021-11-26 at 19:09 +0800, Neal Liu wrote:
> >
> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/ep0.c
> > b/drivers/usb/gadget/udc/aspeed-vhub/ep0.c
> > index 74ea36c19b1e..bea9cbb191a2 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;
> >  	}
> > +
> > +	/* HW return wrong data len */
> > +	if (len < ep->ep.maxpacket && len != remain) {
> > +		EPDBG(ep, "using expected data len instead\n");
> > +		len = remain;
> > +	}
> > +
> 
> Wow, that is a nasty hw bug ! Patch looks good, I had to swap some of that
> logic back into my brain but it looks like it won't break any normal case :-)
> 
> Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> Cheers,
> Ben.

Thanks for your review.
-Neal