[PATCH linux dev-5.4 v2 0/3] aspeed-g6: enable usb support

[PATCH linux dev-5.4 v2 0/3] aspeed-g6: enable usb support

[rentao.bupt]

From: Tao Ren <rentao.bupt@gmail.com>

The patch series aims at enabling USB Host and Gadget support on AST2600
platforms. I'm targeting openbmc tree mainly for some early feedback and
more widespread testing. I'm planning to upstream the patches after
5.6-rc1.

Patch #1 moves hardcoded vhub attributes (number of downstream ports and
endpoints) to "struct ast_hub_config" which is then attached to "struct
of_device_id". By doing this, it will be easier to enable ast2600 vhub
which supports more ports and endpoints.

Patch #2 enables AST2600 support in aspeed-vhub gadget driver.

Patch #3 adds USB devices and according pin groups in aspeed-g6 dtsi.

Tao Ren (3):
  usb: gadget: aspeed: read vhub config from of_device_id
  usb: gadget: aspeed: add ast2600 vhub support
  ARM: dts: aspeed-g6: add usb functions

 arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi   |  25 +++++
 arch/arm/boot/dts/aspeed-g6.dtsi           |  43 ++++++++
 drivers/usb/gadget/udc/aspeed-vhub/Kconfig |   4 +-
 drivers/usb/gadget/udc/aspeed-vhub/core.c  | 108 ++++++++++++++-------
 drivers/usb/gadget/udc/aspeed-vhub/dev.c   |  30 ++++--
 drivers/usb/gadget/udc/aspeed-vhub/epn.c   |   4 +-
 drivers/usb/gadget/udc/aspeed-vhub/hub.c   |  22 +++--
 drivers/usb/gadget/udc/aspeed-vhub/vhub.h  |  21 ++--
 8 files changed, 185 insertions(+), 72 deletions(-)

-- 
2.17.1

[PATCH linux dev-5.4 v2 1/3] usb: gadget: aspeed: read vhub config from of_device_id

[rentao.bupt]

From: Tao Ren <rentao.bupt@gmail.com>

The patch moves hardcoded vhub attributes (maximum downstream ports and
generic endpoints) to "ast_vhub_config" structure which is attached to
struct of_device_id. By doing this, it will be very straightforward to
enable support of AST2600 vhub which has different number of downstream
ports and endpoints.

Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
---
 Changes in v2:
  - this is the first version. It's given v2 to align with the version
    of patch series.


 drivers/usb/gadget/udc/aspeed-vhub/core.c | 100 ++++++++++++++--------
 drivers/usb/gadget/udc/aspeed-vhub/dev.c  |  30 +++++--
 drivers/usb/gadget/udc/aspeed-vhub/epn.c  |   4 +-
 drivers/usb/gadget/udc/aspeed-vhub/hub.c  |  22 +++--
 drivers/usb/gadget/udc/aspeed-vhub/vhub.h |  21 ++---
 5 files changed, 107 insertions(+), 70 deletions(-)

diff --git a/drivers/usb/gadget/udc/aspeed-vhub/core.c b/drivers/usb/gadget/udc/aspeed-vhub/core.c
index 90b134d5dca9..9efbfdffad30 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/core.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/core.c
@@ -32,6 +32,29 @@
 
 #include "vhub.h"
 
+struct ast_vhub_config {
+	u32 max_ports;	/* max number of downstream ports */
+	u32 max_epns;	/* max number of generic endpoints */
+};
+
+static const struct ast_vhub_config ast2400_config = {
+	.max_ports = 5,
+	.max_epns = 15,
+};
+
+static const struct of_device_id ast_vhub_dt_ids[] = {
+	{
+		.compatible = "aspeed,ast2400-usb-vhub",
+		.data = &ast2400_config,
+	},
+	{
+		.compatible = "aspeed,ast2500-usb-vhub",
+		.data = &ast2400_config,
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ast_vhub_dt_ids);
+
 void ast_vhub_done(struct ast_vhub_ep *ep, struct ast_vhub_req *req,
 		   int status)
 {
@@ -99,7 +122,7 @@ static irqreturn_t ast_vhub_irq(int irq, void *data)
 {
 	struct ast_vhub *vhub = data;
 	irqreturn_t iret = IRQ_NONE;
-	u32 istat;
+	u32 i, istat;
 
 	/* Stale interrupt while tearing down */
 	if (!vhub->ep0_bufs)
@@ -121,10 +144,10 @@ static irqreturn_t ast_vhub_irq(int irq, void *data)
 
 	/* Handle generic EPs first */
 	if (istat & VHUB_IRQ_EP_POOL_ACK_STALL) {
-		u32 i, ep_acks = readl(vhub->regs + AST_VHUB_EP_ACK_ISR);
+		u32 ep_acks = readl(vhub->regs + AST_VHUB_EP_ACK_ISR);
 		writel(ep_acks, vhub->regs + AST_VHUB_EP_ACK_ISR);
 
-		for (i = 0; ep_acks && i < AST_VHUB_NUM_GEN_EPs; i++) {
+		for (i = 0; ep_acks && i < vhub->max_epns; i++) {
 			u32 mask = VHUB_EP_IRQ(i);
 			if (ep_acks & mask) {
 				ast_vhub_epn_ack_irq(&vhub->epns[i]);
@@ -134,21 +157,11 @@ static irqreturn_t ast_vhub_irq(int irq, void *data)
 	}
 
 	/* Handle device interrupts */
-	if (istat & (VHUB_IRQ_DEVICE1 |
-		     VHUB_IRQ_DEVICE2 |
-		     VHUB_IRQ_DEVICE3 |
-		     VHUB_IRQ_DEVICE4 |
-		     VHUB_IRQ_DEVICE5)) {
-		if (istat & VHUB_IRQ_DEVICE1)
-			ast_vhub_dev_irq(&vhub->ports[0].dev);
-		if (istat & VHUB_IRQ_DEVICE2)
-			ast_vhub_dev_irq(&vhub->ports[1].dev);
-		if (istat & VHUB_IRQ_DEVICE3)
-			ast_vhub_dev_irq(&vhub->ports[2].dev);
-		if (istat & VHUB_IRQ_DEVICE4)
-			ast_vhub_dev_irq(&vhub->ports[3].dev);
-		if (istat & VHUB_IRQ_DEVICE5)
-			ast_vhub_dev_irq(&vhub->ports[4].dev);
+	for (i = 0; i < vhub->max_ports; i++) {
+		u32 dev_mask = VHUB_IRQ_DEVICE1 << i;
+
+		if (istat & dev_mask)
+			ast_vhub_dev_irq(&vhub->ports[i].dev);
 	}
 
 	/* Handle top-level vHub EP0 interrupts */
@@ -182,7 +195,7 @@ static irqreturn_t ast_vhub_irq(int irq, void *data)
 
 void ast_vhub_init_hw(struct ast_vhub *vhub)
 {
-	u32 ctrl;
+	u32 ctrl, port_mask, epn_mask;
 
 	UDCDBG(vhub,"(Re)Starting HW ...\n");
 
@@ -222,15 +235,20 @@ void ast_vhub_init_hw(struct ast_vhub *vhub)
 	}
 
 	/* Reset all devices */
-	writel(VHUB_SW_RESET_ALL, vhub->regs + AST_VHUB_SW_RESET);
+	port_mask = GENMASK(vhub->max_ports, 1);
+	writel(VHUB_SW_RESET_ROOT_HUB |
+	       VHUB_SW_RESET_DMA_CONTROLLER |
+	       VHUB_SW_RESET_EP_POOL |
+	       port_mask, vhub->regs + AST_VHUB_SW_RESET);
 	udelay(1);
 	writel(0, vhub->regs + AST_VHUB_SW_RESET);
 
 	/* Disable and cleanup EP ACK/NACK interrupts */
+	epn_mask = GENMASK(vhub->max_epns - 1, 0);
 	writel(0, vhub->regs + AST_VHUB_EP_ACK_IER);
 	writel(0, vhub->regs + AST_VHUB_EP_NACK_IER);
-	writel(VHUB_EP_IRQ_ALL, vhub->regs + AST_VHUB_EP_ACK_ISR);
-	writel(VHUB_EP_IRQ_ALL, vhub->regs + AST_VHUB_EP_NACK_ISR);
+	writel(epn_mask, vhub->regs + AST_VHUB_EP_ACK_ISR);
+	writel(epn_mask, vhub->regs + AST_VHUB_EP_NACK_ISR);
 
 	/* Default settings for EP0, enable HW hub EP1 */
 	writel(0, vhub->regs + AST_VHUB_EP0_CTRL);
@@ -273,7 +291,7 @@ static int ast_vhub_remove(struct platform_device *pdev)
 		return 0;
 
 	/* Remove devices */
-	for (i = 0; i < AST_VHUB_NUM_PORTS; i++)
+	for (i = 0; i < vhub->max_ports; i++)
 		ast_vhub_del_dev(&vhub->ports[i].dev);
 
 	spin_lock_irqsave(&vhub->lock, flags);
@@ -295,7 +313,7 @@ static int ast_vhub_remove(struct platform_device *pdev)
 	if (vhub->ep0_bufs)
 		dma_free_coherent(&pdev->dev,
 				  AST_VHUB_EP0_MAX_PACKET *
-				  (AST_VHUB_NUM_PORTS + 1),
+				  (vhub->max_ports + 1),
 				  vhub->ep0_bufs,
 				  vhub->ep0_bufs_dma);
 	vhub->ep0_bufs = NULL;
@@ -309,11 +327,30 @@ static int ast_vhub_probe(struct platform_device *pdev)
 	struct ast_vhub *vhub;
 	struct resource *res;
 	int i, rc = 0;
+	const struct of_device_id *ofdid;
+	struct ast_vhub_config *config;
 
 	vhub = devm_kzalloc(&pdev->dev, sizeof(*vhub), GFP_KERNEL);
 	if (!vhub)
 		return -ENOMEM;
 
+	ofdid = of_match_node(ast_vhub_dt_ids, pdev->dev.of_node);
+	if (!ofdid)
+		return -EINVAL;
+	config = ofdid->data;
+
+	vhub->max_ports = config->max_ports;
+	vhub->ports = devm_kcalloc(&pdev->dev, vhub->max_ports,
+				   sizeof(*vhub->ports), GFP_KERNEL);
+	if (!vhub->ports)
+		return -ENOMEM;
+
+	vhub->max_epns = config->max_epns;
+	vhub->epns = devm_kcalloc(&pdev->dev, vhub->max_epns,
+				  sizeof(*vhub->epns), GFP_KERNEL);
+	if (!vhub->epns)
+		return -ENOMEM;
+
 	spin_lock_init(&vhub->lock);
 	vhub->pdev = pdev;
 
@@ -366,7 +403,7 @@ static int ast_vhub_probe(struct platform_device *pdev)
 	 */
 	vhub->ep0_bufs = dma_alloc_coherent(&pdev->dev,
 					    AST_VHUB_EP0_MAX_PACKET *
-					    (AST_VHUB_NUM_PORTS + 1),
+					    (vhub->max_ports + 1),
 					    &vhub->ep0_bufs_dma, GFP_KERNEL);
 	if (!vhub->ep0_bufs) {
 		dev_err(&pdev->dev, "Failed to allocate EP0 DMA buffers\n");
@@ -380,7 +417,7 @@ static int ast_vhub_probe(struct platform_device *pdev)
 	ast_vhub_init_ep0(vhub, &vhub->ep0, NULL);
 
 	/* Init devices */
-	for (i = 0; i < AST_VHUB_NUM_PORTS && rc == 0; i++)
+	for (i = 0; i < vhub->max_ports && rc == 0; i++)
 		rc = ast_vhub_init_dev(vhub, i);
 	if (rc)
 		goto err;
@@ -400,17 +437,6 @@ static int ast_vhub_probe(struct platform_device *pdev)
 	return rc;
 }
 
-static const struct of_device_id ast_vhub_dt_ids[] = {
-	{
-		.compatible = "aspeed,ast2400-usb-vhub",
-	},
-	{
-		.compatible = "aspeed,ast2500-usb-vhub",
-	},
-	{ }
-};
-MODULE_DEVICE_TABLE(of, ast_vhub_dt_ids);
-
 static struct platform_driver ast_vhub_driver = {
 	.probe		= ast_vhub_probe,
 	.remove		= ast_vhub_remove,
diff --git a/drivers/usb/gadget/udc/aspeed-vhub/dev.c b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
index 4008e7a51188..f9b762951121 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/dev.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
@@ -77,7 +77,7 @@ static void ast_vhub_dev_enable(struct ast_vhub_dev *d)
 	writel(d->ep0.buf_dma, d->regs + AST_VHUB_DEV_EP0_DATA);
 
 	/* Clear stall on all EPs */
-	for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++) {
+	for (i = 0; i < d->max_epns; i++) {
 		struct ast_vhub_ep *ep = d->epns[i];
 
 		if (ep && (ep->epn.stalled || ep->epn.wedged)) {
@@ -137,7 +137,7 @@ static int ast_vhub_ep_feature(struct ast_vhub_dev *d,
 	     is_set ? "SET" : "CLEAR", ep_num, wValue);
 	if (ep_num == 0)
 		return std_req_complete;
-	if (ep_num >= AST_VHUB_NUM_GEN_EPs || !d->epns[ep_num - 1])
+	if (ep_num >= d->max_epns || !d->epns[ep_num - 1])
 		return std_req_stall;
 	if (wValue != USB_ENDPOINT_HALT)
 		return std_req_driver;
@@ -181,7 +181,7 @@ static int ast_vhub_ep_status(struct ast_vhub_dev *d,
 
 	DDBG(d, "GET_STATUS(ep%d)\n", ep_num);
 
-	if (ep_num >= AST_VHUB_NUM_GEN_EPs)
+	if (ep_num >= d->max_epns)
 		return std_req_stall;
 	if (ep_num != 0) {
 		ep = d->epns[ep_num - 1];
@@ -299,7 +299,7 @@ static void ast_vhub_dev_nuke(struct ast_vhub_dev *d)
 {
 	unsigned int i;
 
-	for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++) {
+	for (i = 0; i < d->max_epns; i++) {
 		if (!d->epns[i])
 			continue;
 		ast_vhub_nuke(d->epns[i], -ESHUTDOWN);
@@ -416,10 +416,10 @@ static struct usb_ep *ast_vhub_udc_match_ep(struct usb_gadget *gadget,
 	 * that will allow the generic code to use our
 	 * assigned address.
 	 */
-	for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++)
+	for (i = 0; i < d->max_epns; i++)
 		if (d->epns[i] == NULL)
 			break;
-	if (i >= AST_VHUB_NUM_GEN_EPs)
+	if (i >= d->max_epns)
 		return NULL;
 	addr = i + 1;
 
@@ -526,6 +526,7 @@ void ast_vhub_del_dev(struct ast_vhub_dev *d)
 
 	usb_del_gadget_udc(&d->gadget);
 	device_unregister(d->port_dev);
+	kfree(d->epns);
 }
 
 static void ast_vhub_dev_release(struct device *dev)
@@ -546,14 +547,25 @@ int ast_vhub_init_dev(struct ast_vhub *vhub, unsigned int idx)
 
 	ast_vhub_init_ep0(vhub, &d->ep0, d);
 
+	/*
+	 * A USB device can have up to 30 endpoints besides control
+	 * endpoint 0.
+	 */
+	d->max_epns = min(vhub->max_epns, 30);
+	d->epns = kcalloc(d->max_epns, sizeof(*d->epns), GFP_KERNEL);
+	if (!d->epns)
+		return -ENOMEM;
+
 	/*
 	 * The UDC core really needs us to have separate and uniquely
 	 * named "parent" devices for each port so we create a sub device
 	 * here for that purpose
 	 */
 	d->port_dev = kzalloc(sizeof(struct device), GFP_KERNEL);
-	if (!d->port_dev)
-		return -ENOMEM;
+	if (!d->port_dev) {
+		rc = -ENOMEM;
+		goto fail_alloc;
+	}
 	device_initialize(d->port_dev);
 	d->port_dev->release = ast_vhub_dev_release;
 	d->port_dev->parent = parent;
@@ -584,6 +596,8 @@ int ast_vhub_init_dev(struct ast_vhub *vhub, unsigned int idx)
 	device_del(d->port_dev);
  fail_add:
 	put_device(d->port_dev);
+ fail_alloc:
+	kfree(d->epns);
 
 	return rc;
 }
diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
index 7475c74aa5c5..0bd6b20435b8 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
@@ -800,10 +800,10 @@ struct ast_vhub_ep *ast_vhub_alloc_epn(struct ast_vhub_dev *d, u8 addr)
 
 	/* Find a free one (no device) */
 	spin_lock_irqsave(&vhub->lock, flags);
-	for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++)
+	for (i = 0; i < vhub->max_epns; i++)
 		if (vhub->epns[i].dev == NULL)
 			break;
-	if (i >= AST_VHUB_NUM_GEN_EPs) {
+	if (i >= vhub->max_epns) {
 		spin_unlock_irqrestore(&vhub->lock, flags);
 		return NULL;
 	}
diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
index 19b3517e04c0..aa1c127e9f2f 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
@@ -133,10 +133,9 @@ static const struct ast_vhub_full_cdesc {
 
 #define AST_VHUB_HUB_DESC_SIZE	(USB_DT_HUB_NONVAR_SIZE + 2)
 
-static const struct usb_hub_descriptor ast_vhub_hub_desc = {
+static struct usb_hub_descriptor ast_vhub_hub_desc = {
 	.bDescLength			= AST_VHUB_HUB_DESC_SIZE,
 	.bDescriptorType		= USB_DT_HUB,
-	.bNbrPorts			= AST_VHUB_NUM_PORTS,
 	.wHubCharacteristics		= cpu_to_le16(HUB_CHAR_NO_LPSM),
 	.bPwrOn2PwrGood			= 10,
 	.bHubContrCurrent		= 0,
@@ -504,7 +503,7 @@ static void ast_vhub_wake_work(struct work_struct *work)
 	 * we let the normal host wake path deal with it later.
 	 */
 	spin_lock_irqsave(&vhub->lock, flags);
-	for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
+	for (i = 0; i < vhub->max_ports; i++) {
 		struct ast_vhub_port *p = &vhub->ports[i];
 
 		if (!(p->status & USB_PORT_STAT_SUSPEND))
@@ -587,7 +586,7 @@ static enum std_req_rc ast_vhub_set_port_feature(struct ast_vhub_ep *ep,
 	struct ast_vhub *vhub = ep->vhub;
 	struct ast_vhub_port *p;
 
-	if (port == 0 || port > AST_VHUB_NUM_PORTS)
+	if (port == 0 || port > vhub->max_ports)
 		return std_req_stall;
 	port--;
 	p = &vhub->ports[port];
@@ -630,7 +629,7 @@ static enum std_req_rc ast_vhub_clr_port_feature(struct ast_vhub_ep *ep,
 	struct ast_vhub *vhub = ep->vhub;
 	struct ast_vhub_port *p;
 
-	if (port == 0 || port > AST_VHUB_NUM_PORTS)
+	if (port == 0 || port > vhub->max_ports)
 		return std_req_stall;
 	port--;
 	p = &vhub->ports[port];
@@ -676,7 +675,7 @@ static enum std_req_rc ast_vhub_get_port_stat(struct ast_vhub_ep *ep,
 	struct ast_vhub *vhub = ep->vhub;
 	u16 stat, chg;
 
-	if (port == 0 || port > AST_VHUB_NUM_PORTS)
+	if (port == 0 || port > vhub->max_ports)
 		return std_req_stall;
 	port--;
 
@@ -757,7 +756,7 @@ void ast_vhub_hub_suspend(struct ast_vhub *vhub)
 	 * Forward to unsuspended ports without changing
 	 * their connection status.
 	 */
-	for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
+	for (i = 0; i < vhub->max_ports; i++) {
 		struct ast_vhub_port *p = &vhub->ports[i];
 
 		if (!(p->status & USB_PORT_STAT_SUSPEND))
@@ -780,7 +779,7 @@ void ast_vhub_hub_resume(struct ast_vhub *vhub)
 	 * Forward to unsuspended ports without changing
 	 * their connection status.
 	 */
-	for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
+	for (i = 0; i < vhub->max_ports; i++) {
 		struct ast_vhub_port *p = &vhub->ports[i];
 
 		if (!(p->status & USB_PORT_STAT_SUSPEND))
@@ -814,7 +813,7 @@ void ast_vhub_hub_reset(struct ast_vhub *vhub)
 	 * Clear all port status, disable gadgets and "suspend"
 	 * them. They will be woken up by a port reset.
 	 */
-	for (i = 0; i < AST_VHUB_NUM_PORTS; i++) {
+	for (i = 0; i < vhub->max_ports; i++) {
 		struct ast_vhub_port *p = &vhub->ports[i];
 
 		/* Only keep the connected flag */
@@ -838,5 +837,10 @@ void ast_vhub_init_hub(struct ast_vhub *vhub)
 {
 	vhub->speed = USB_SPEED_UNKNOWN;
 	INIT_WORK(&vhub->wake_work, ast_vhub_wake_work);
+
+	/*
+	 * Fixup number of ports in hub descriptor.
+	 */
+	ast_vhub_hub_desc.bNbrPorts = vhub->max_ports;
 }
 
diff --git a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
index 761919e220d3..11414548cd32 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
+++ b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
@@ -76,17 +76,9 @@
 #define VHUB_SW_RESET_DEVICE2			(1 << 2)
 #define VHUB_SW_RESET_DEVICE1			(1 << 1)
 #define VHUB_SW_RESET_ROOT_HUB			(1 << 0)
-#define VHUB_SW_RESET_ALL			(VHUB_SW_RESET_EP_POOL | \
-						 VHUB_SW_RESET_DMA_CONTROLLER | \
-						 VHUB_SW_RESET_DEVICE5 | \
-						 VHUB_SW_RESET_DEVICE4 | \
-						 VHUB_SW_RESET_DEVICE3 | \
-						 VHUB_SW_RESET_DEVICE2 | \
-						 VHUB_SW_RESET_DEVICE1 | \
-						 VHUB_SW_RESET_ROOT_HUB)
+
 /* EP ACK/NACK IRQ masks */
 #define VHUB_EP_IRQ(n)				(1 << (n))
-#define VHUB_EP_IRQ_ALL				0x7fff	/* 15 EPs */
 
 /* USB status reg */
 #define VHUB_USBSTS_HISPEED			(1 << 27)
@@ -210,8 +202,6 @@
  *                                      *
  ****************************************/
 
-#define AST_VHUB_NUM_GEN_EPs	15	/* Generic non-0 EPs */
-#define AST_VHUB_NUM_PORTS	5	/* vHub ports */
 #define AST_VHUB_EP0_MAX_PACKET	64	/* EP0's max packet size */
 #define AST_VHUB_EPn_MAX_PACKET	1024	/* Generic EPs max packet size */
 #define AST_VHUB_DESCS_COUNT	256	/* Use 256 descriptor mode (valid
@@ -358,7 +348,8 @@ struct ast_vhub_dev {
 
 	/* Endpoint structures */
 	struct ast_vhub_ep		ep0;
-	struct ast_vhub_ep		*epns[AST_VHUB_NUM_GEN_EPs];
+	struct ast_vhub_ep		**epns;
+	u32				max_epns;
 
 };
 #define to_ast_dev(__g) container_of(__g, struct ast_vhub_dev, gadget)
@@ -393,10 +384,12 @@ struct ast_vhub {
 	bool				ep1_stalled : 1;
 
 	/* Per-port info */
-	struct ast_vhub_port		ports[AST_VHUB_NUM_PORTS];
+	struct ast_vhub_port		*ports;
+	u32				max_ports;
 
 	/* Generic EP data structures */
-	struct ast_vhub_ep		epns[AST_VHUB_NUM_GEN_EPs];
+	struct ast_vhub_ep		*epns;
+	u32				max_epns;
 
 	/* Upstream bus is suspended ? */
 	bool				suspended : 1;
-- 
2.17.1

[PATCH linux dev-5.4 v2 2/3] usb: gadget: aspeed: add ast2600 vhub support

[rentao.bupt]

From: Tao Ren <rentao.bupt@gmail.com>

Add AST2600 support in aspeed-vhub driver. There are 3 major differences
between AST2500 and AST2600 vhub:
  - AST2600 supports 7 downstream ports while AST2500 supports 5.
  - AST2600 supports 21 generic endpoints while AST2500 supports 15.
  - EP0 data buffer's 8-byte DMA alignment restriction is removed from
    AST2600.

Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
---
 Changes in v2:
  - all the ports/endpoints handling logic are moved to patch #1.

 drivers/usb/gadget/udc/aspeed-vhub/Kconfig | 4 ++--
 drivers/usb/gadget/udc/aspeed-vhub/core.c  | 9 +++++++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/udc/aspeed-vhub/Kconfig b/drivers/usb/gadget/udc/aspeed-vhub/Kconfig
index 83ba8a2eb6af..605500b19cf3 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/Kconfig
+++ b/drivers/usb/gadget/udc/aspeed-vhub/Kconfig
@@ -4,5 +4,5 @@ config USB_ASPEED_VHUB
 	depends on ARCH_ASPEED || COMPILE_TEST
 	depends on USB_LIBCOMPOSITE
 	help
-	  USB peripheral controller for the Aspeed AST2500 family
-	  SoCs supporting the "vHub" functionality and USB2.0
+	  USB peripheral controller for the Aspeed AST2400, AST2500 and
+	  AST2600 family SoCs supporting the "vHub" functionality and USB2.0
diff --git a/drivers/usb/gadget/udc/aspeed-vhub/core.c b/drivers/usb/gadget/udc/aspeed-vhub/core.c
index 9efbfdffad30..8caa6f6bd3d5 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/core.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/core.c
@@ -42,6 +42,11 @@ static const struct ast_vhub_config ast2400_config = {
 	.max_epns = 15,
 };
 
+static const struct ast_vhub_config ast2600_config = {
+	.max_ports = 7,
+	.max_epns = 21,
+};
+
 static const struct of_device_id ast_vhub_dt_ids[] = {
 	{
 		.compatible = "aspeed,ast2400-usb-vhub",
@@ -51,6 +56,10 @@ static const struct of_device_id ast_vhub_dt_ids[] = {
 		.compatible = "aspeed,ast2500-usb-vhub",
 		.data = &ast2400_config,
 	},
+	{
+		.compatible = "aspeed,ast2600-usb-vhub",
+		.data = &ast2600_config,
+	},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, ast_vhub_dt_ids);
-- 
2.17.1

[PATCH linux dev-5.4 v2 3/3] ARM: dts: aspeed-g6: add usb functions

[rentao.bupt]

From: Tao Ren <rentao.bupt@gmail.com>

Add USB components and according pin groups in aspeed-g6 dtsi.

Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
---
 Changes in v2:
  - fixed vhub reg size in aspeed-g6.dtsi.

 arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi | 25 ++++++++++++++
 arch/arm/boot/dts/aspeed-g6.dtsi         | 43 ++++++++++++++++++++++++
 2 files changed, 68 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi b/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi
index 045ce66ca876..7028e21bdd98 100644
--- a/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi
+++ b/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi
@@ -1112,6 +1112,31 @@
 		groups = "UART9";
 	};
 
+	pinctrl_usb2ah_default: usb2ah_default {
+		function = "USB2AH";
+		groups = "USBA";
+	};
+
+	pinctrl_usb2ad_default: usb2ad_default {
+		function = "USB2AD";
+		groups = "USBA";
+	};
+
+	pinctrl_usb2bh_default: usb2bh_default {
+		function = "USB2BH";
+		groups = "USBB";
+	};
+
+	pinctrl_usb2bd_default: usb2bd_default {
+		function = "USB2BD";
+		groups = "USBB";
+	};
+
+	pinctrl_usb11bhid_default: usb11bhid_default {
+		function = "USB11BHID";
+		groups = "USBB";
+	};
+
 	pinctrl_vb_default: vb_default {
 		function = "VB";
 		groups = "VB";
diff --git a/arch/arm/boot/dts/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed-g6.dtsi
index 99cc7d7ced4d..5b3bd626f62a 100644
--- a/arch/arm/boot/dts/aspeed-g6.dtsi
+++ b/arch/arm/boot/dts/aspeed-g6.dtsi
@@ -245,6 +245,49 @@
 			status = "disabled";
 		};
 
+		ehci0: usb@1e6a1000 {
+			compatible = "aspeed,ast2600-ehci", "generic-ehci";
+			reg = <0x1e6a1000 0x100>;
+			interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&syscon ASPEED_CLK_GATE_USBPORT1CLK>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_usb2ah_default>;
+			status = "disabled";
+		};
+
+		ehci1: usb@1e6a3000 {
+			compatible = "aspeed,ast2600-ehci", "generic-ehci";
+			reg = <0x1e6a3000 0x100>;
+			interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&syscon ASPEED_CLK_GATE_USBPORT2CLK>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_usb2bh_default>;
+			status = "disabled";
+		};
+
+		uhci: usb@1e6b0000 {
+			compatible = "aspeed,ast2600-uhci", "generic-uhci";
+			reg = <0x1e6b0000 0x100>;
+			interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
+			#ports = <2>;
+			clocks = <&syscon ASPEED_CLK_GATE_USBUHCICLK>;
+			status = "disabled";
+			/*
+			 * No default pinmux, it will follow EHCI, use an
+			 * explicit pinmux override if EHCI is not enabled.
+			 */
+		};
+
+		vhub: usb-vhub@1e6a0000 {
+			compatible = "aspeed,ast2600-usb-vhub";
+			reg = <0x1e6a0000 0x350>;
+			interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&syscon ASPEED_CLK_GATE_USBPORT1CLK>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_usb2ad_default>;
+			status = "disabled";
+		};
+
 		apb {
 			compatible = "simple-bus";
 			#address-cells = <1>;
-- 
2.17.1

Re: [PATCH linux dev-5.4 v2 1/3] usb: gadget: aspeed: read vhub config from of_device_id

[Andrew Jeffery]

On Thu, 23 Jan 2020, at 18:19, rentao.bupt@gmail.com wrote:
> From: Tao Ren <rentao.bupt@gmail.com>
> 
> The patch moves hardcoded vhub attributes (maximum downstream ports and
> generic endpoints) to "ast_vhub_config" structure which is attached to
> struct of_device_id. By doing this, it will be very straightforward to
> enable support of AST2600 vhub which has different number of downstream
> ports and endpoints.
> 
> Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> ---
>  Changes in v2:
>   - this is the first version. It's given v2 to align with the version
>     of patch series.
> 
> 
>  drivers/usb/gadget/udc/aspeed-vhub/core.c | 100 ++++++++++++++--------
>  drivers/usb/gadget/udc/aspeed-vhub/dev.c  |  30 +++++--
>  drivers/usb/gadget/udc/aspeed-vhub/epn.c  |   4 +-
>  drivers/usb/gadget/udc/aspeed-vhub/hub.c  |  22 +++--
>  drivers/usb/gadget/udc/aspeed-vhub/vhub.h |  21 ++---
>  5 files changed, 107 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/core.c 
> b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> index 90b134d5dca9..9efbfdffad30 100644
> --- a/drivers/usb/gadget/udc/aspeed-vhub/core.c
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> @@ -32,6 +32,29 @@
>  
>  #include "vhub.h"
>  
> +struct ast_vhub_config {
> +	u32 max_ports;	/* max number of downstream ports */
> +	u32 max_epns;	/* max number of generic endpoints */
> +};
> +
> +static const struct ast_vhub_config ast2400_config = {
> +	.max_ports = 5,
> +	.max_epns = 15,
> +};
> +
> +static const struct of_device_id ast_vhub_dt_ids[] = {
> +	{
> +		.compatible = "aspeed,ast2400-usb-vhub",
> +		.data = &ast2400_config,
> +	},
> +	{
> +		.compatible = "aspeed,ast2500-usb-vhub",
> +		.data = &ast2400_config,
> +	},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ast_vhub_dt_ids);
> +

Is there a reason to move the table from where it was? It's not a problem,
just makes the diff a bit noisier.

>  void ast_vhub_done(struct ast_vhub_ep *ep, struct ast_vhub_req *req,
>  		   int status)
>  {
> @@ -99,7 +122,7 @@ static irqreturn_t ast_vhub_irq(int irq, void *data)
>  {
>  	struct ast_vhub *vhub = data;
>  	irqreturn_t iret = IRQ_NONE;
> -	u32 istat;
> +	u32 i, istat;
>  
>  	/* Stale interrupt while tearing down */
>  	if (!vhub->ep0_bufs)
> @@ -121,10 +144,10 @@ static irqreturn_t ast_vhub_irq(int irq, void *data)
>  
>  	/* Handle generic EPs first */
>  	if (istat & VHUB_IRQ_EP_POOL_ACK_STALL) {
> -		u32 i, ep_acks = readl(vhub->regs + AST_VHUB_EP_ACK_ISR);
> +		u32 ep_acks = readl(vhub->regs + AST_VHUB_EP_ACK_ISR);
>  		writel(ep_acks, vhub->regs + AST_VHUB_EP_ACK_ISR);
>  
> -		for (i = 0; ep_acks && i < AST_VHUB_NUM_GEN_EPs; i++) {
> +		for (i = 0; ep_acks && i < vhub->max_epns; i++) {
>  			u32 mask = VHUB_EP_IRQ(i);
>  			if (ep_acks & mask) {
>  				ast_vhub_epn_ack_irq(&vhub->epns[i]);
> @@ -134,21 +157,11 @@ static irqreturn_t ast_vhub_irq(int irq, void *data)
>  	}
>  
>  	/* Handle device interrupts */
> -	if (istat & (VHUB_IRQ_DEVICE1 |
> -		     VHUB_IRQ_DEVICE2 |
> -		     VHUB_IRQ_DEVICE3 |
> -		     VHUB_IRQ_DEVICE4 |
> -		     VHUB_IRQ_DEVICE5)) {
> -		if (istat & VHUB_IRQ_DEVICE1)
> -			ast_vhub_dev_irq(&vhub->ports[0].dev);
> -		if (istat & VHUB_IRQ_DEVICE2)
> -			ast_vhub_dev_irq(&vhub->ports[1].dev);
> -		if (istat & VHUB_IRQ_DEVICE3)
> -			ast_vhub_dev_irq(&vhub->ports[2].dev);
> -		if (istat & VHUB_IRQ_DEVICE4)
> -			ast_vhub_dev_irq(&vhub->ports[3].dev);
> -		if (istat & VHUB_IRQ_DEVICE5)
> -			ast_vhub_dev_irq(&vhub->ports[4].dev);
> +	for (i = 0; i < vhub->max_ports; i++) {
> +		u32 dev_mask = VHUB_IRQ_DEVICE1 << i;
> +
> +		if (istat & dev_mask)
> +			ast_vhub_dev_irq(&vhub->ports[i].dev);
>  	}
>  
>  	/* Handle top-level vHub EP0 interrupts */
> @@ -182,7 +195,7 @@ static irqreturn_t ast_vhub_irq(int irq, void *data)
>  
>  void ast_vhub_init_hw(struct ast_vhub *vhub)
>  {
> -	u32 ctrl;
> +	u32 ctrl, port_mask, epn_mask;
>  
>  	UDCDBG(vhub,"(Re)Starting HW ...\n");
>  
> @@ -222,15 +235,20 @@ void ast_vhub_init_hw(struct ast_vhub *vhub)
>  	}
>  
>  	/* Reset all devices */
> -	writel(VHUB_SW_RESET_ALL, vhub->regs + AST_VHUB_SW_RESET);
> +	port_mask = GENMASK(vhub->max_ports, 1);
> +	writel(VHUB_SW_RESET_ROOT_HUB |
> +	       VHUB_SW_RESET_DMA_CONTROLLER |
> +	       VHUB_SW_RESET_EP_POOL |
> +	       port_mask, vhub->regs + AST_VHUB_SW_RESET);
>  	udelay(1);
>  	writel(0, vhub->regs + AST_VHUB_SW_RESET);
>  
>  	/* Disable and cleanup EP ACK/NACK interrupts */
> +	epn_mask = GENMASK(vhub->max_epns - 1, 0);
>  	writel(0, vhub->regs + AST_VHUB_EP_ACK_IER);
>  	writel(0, vhub->regs + AST_VHUB_EP_NACK_IER);
> -	writel(VHUB_EP_IRQ_ALL, vhub->regs + AST_VHUB_EP_ACK_ISR);
> -	writel(VHUB_EP_IRQ_ALL, vhub->regs + AST_VHUB_EP_NACK_ISR);
> +	writel(epn_mask, vhub->regs + AST_VHUB_EP_ACK_ISR);
> +	writel(epn_mask, vhub->regs + AST_VHUB_EP_NACK_ISR);
>  
>  	/* Default settings for EP0, enable HW hub EP1 */
>  	writel(0, vhub->regs + AST_VHUB_EP0_CTRL);
> @@ -273,7 +291,7 @@ static int ast_vhub_remove(struct platform_device *pdev)
>  		return 0;
>  
>  	/* Remove devices */
> -	for (i = 0; i < AST_VHUB_NUM_PORTS; i++)
> +	for (i = 0; i < vhub->max_ports; i++)
>  		ast_vhub_del_dev(&vhub->ports[i].dev);
>  
>  	spin_lock_irqsave(&vhub->lock, flags);
> @@ -295,7 +313,7 @@ static int ast_vhub_remove(struct platform_device *pdev)
>  	if (vhub->ep0_bufs)
>  		dma_free_coherent(&pdev->dev,
>  				  AST_VHUB_EP0_MAX_PACKET *
> -				  (AST_VHUB_NUM_PORTS + 1),
> +				  (vhub->max_ports + 1),
>  				  vhub->ep0_bufs,
>  				  vhub->ep0_bufs_dma);
>  	vhub->ep0_bufs = NULL;
> @@ -309,11 +327,30 @@ static int ast_vhub_probe(struct platform_device *pdev)
>  	struct ast_vhub *vhub;
>  	struct resource *res;
>  	int i, rc = 0;
> +	const struct of_device_id *ofdid;
> +	struct ast_vhub_config *config;
>  
>  	vhub = devm_kzalloc(&pdev->dev, sizeof(*vhub), GFP_KERNEL);
>  	if (!vhub)
>  		return -ENOMEM;
>  
> +	ofdid = of_match_node(ast_vhub_dt_ids, pdev->dev.of_node);
> +	if (!ofdid)
> +		return -EINVAL;
> +	config = ofdid->data;
> +
> +	vhub->max_ports = config->max_ports;
> +	vhub->ports = devm_kcalloc(&pdev->dev, vhub->max_ports,
> +				   sizeof(*vhub->ports), GFP_KERNEL);
> +	if (!vhub->ports)
> +		return -ENOMEM;
> +
> +	vhub->max_epns = config->max_epns;
> +	vhub->epns = devm_kcalloc(&pdev->dev, vhub->max_epns,
> +				  sizeof(*vhub->epns), GFP_KERNEL);
> +	if (!vhub->epns)
> +		return -ENOMEM;
> +
>  	spin_lock_init(&vhub->lock);
>  	vhub->pdev = pdev;
>  
> @@ -366,7 +403,7 @@ static int ast_vhub_probe(struct platform_device *pdev)
>  	 */
>  	vhub->ep0_bufs = dma_alloc_coherent(&pdev->dev,
>  					    AST_VHUB_EP0_MAX_PACKET *
> -					    (AST_VHUB_NUM_PORTS + 1),
> +					    (vhub->max_ports + 1),
>  					    &vhub->ep0_bufs_dma, GFP_KERNEL);
>  	if (!vhub->ep0_bufs) {
>  		dev_err(&pdev->dev, "Failed to allocate EP0 DMA buffers\n");
> @@ -380,7 +417,7 @@ static int ast_vhub_probe(struct platform_device *pdev)
>  	ast_vhub_init_ep0(vhub, &vhub->ep0, NULL);
>  
>  	/* Init devices */
> -	for (i = 0; i < AST_VHUB_NUM_PORTS && rc == 0; i++)
> +	for (i = 0; i < vhub->max_ports && rc == 0; i++)
>  		rc = ast_vhub_init_dev(vhub, i);
>  	if (rc)
>  		goto err;
> @@ -400,17 +437,6 @@ static int ast_vhub_probe(struct platform_device *pdev)
>  	return rc;
>  }
>  
> -static const struct of_device_id ast_vhub_dt_ids[] = {
> -	{
> -		.compatible = "aspeed,ast2400-usb-vhub",
> -	},
> -	{
> -		.compatible = "aspeed,ast2500-usb-vhub",
> -	},
> -	{ }
> -};
> -MODULE_DEVICE_TABLE(of, ast_vhub_dt_ids);
> -
>  static struct platform_driver ast_vhub_driver = {
>  	.probe		= ast_vhub_probe,
>  	.remove		= ast_vhub_remove,
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/dev.c 
> b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> index 4008e7a51188..f9b762951121 100644
> --- a/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> @@ -77,7 +77,7 @@ static void ast_vhub_dev_enable(struct ast_vhub_dev 
> *d)
>  	writel(d->ep0.buf_dma, d->regs + AST_VHUB_DEV_EP0_DATA);
>  
>  	/* Clear stall on all EPs */
> -	for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++) {
> +	for (i = 0; i < d->max_epns; i++) {
>  		struct ast_vhub_ep *ep = d->epns[i];
>  
>  		if (ep && (ep->epn.stalled || ep->epn.wedged)) {
> @@ -137,7 +137,7 @@ static int ast_vhub_ep_feature(struct ast_vhub_dev *d,
>  	     is_set ? "SET" : "CLEAR", ep_num, wValue);
>  	if (ep_num == 0)
>  		return std_req_complete;
> -	if (ep_num >= AST_VHUB_NUM_GEN_EPs || !d->epns[ep_num - 1])
> +	if (ep_num >= d->max_epns || !d->epns[ep_num - 1])
>  		return std_req_stall;
>  	if (wValue != USB_ENDPOINT_HALT)
>  		return std_req_driver;
> @@ -181,7 +181,7 @@ static int ast_vhub_ep_status(struct ast_vhub_dev *d,
>  
>  	DDBG(d, "GET_STATUS(ep%d)\n", ep_num);
>  
> -	if (ep_num >= AST_VHUB_NUM_GEN_EPs)
> +	if (ep_num >= d->max_epns)
>  		return std_req_stall;
>  	if (ep_num != 0) {
>  		ep = d->epns[ep_num - 1];
> @@ -299,7 +299,7 @@ static void ast_vhub_dev_nuke(struct ast_vhub_dev *d)
>  {
>  	unsigned int i;
>  
> -	for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++) {
> +	for (i = 0; i < d->max_epns; i++) {
>  		if (!d->epns[i])
>  			continue;
>  		ast_vhub_nuke(d->epns[i], -ESHUTDOWN);
> @@ -416,10 +416,10 @@ static struct usb_ep 
> *ast_vhub_udc_match_ep(struct usb_gadget *gadget,
>  	 * that will allow the generic code to use our
>  	 * assigned address.
>  	 */
> -	for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++)
> +	for (i = 0; i < d->max_epns; i++)
>  		if (d->epns[i] == NULL)
>  			break;
> -	if (i >= AST_VHUB_NUM_GEN_EPs)
> +	if (i >= d->max_epns)
>  		return NULL;
>  	addr = i + 1;
>  
> @@ -526,6 +526,7 @@ void ast_vhub_del_dev(struct ast_vhub_dev *d)
>  
>  	usb_del_gadget_udc(&d->gadget);
>  	device_unregister(d->port_dev);
> +	kfree(d->epns);
>  }
>  
>  static void ast_vhub_dev_release(struct device *dev)
> @@ -546,14 +547,25 @@ int ast_vhub_init_dev(struct ast_vhub *vhub, 
> unsigned int idx)
>  
>  	ast_vhub_init_ep0(vhub, &d->ep0, d);
>  
> +	/*
> +	 * A USB device can have up to 30 endpoints besides control
> +	 * endpoint 0.
> +	 */
> +	d->max_epns = min(vhub->max_epns, 30);
> +	d->epns = kcalloc(d->max_epns, sizeof(*d->epns), GFP_KERNEL);
> +	if (!d->epns)
> +		return -ENOMEM;
> +
>  	/*
>  	 * The UDC core really needs us to have separate and uniquely
>  	 * named "parent" devices for each port so we create a sub device
>  	 * here for that purpose
>  	 */
>  	d->port_dev = kzalloc(sizeof(struct device), GFP_KERNEL);
> -	if (!d->port_dev)
> -		return -ENOMEM;
> +	if (!d->port_dev) {
> +		rc = -ENOMEM;
> +		goto fail_alloc;
> +	}
>  	device_initialize(d->port_dev);
>  	d->port_dev->release = ast_vhub_dev_release;
>  	d->port_dev->parent = parent;
> @@ -584,6 +596,8 @@ int ast_vhub_init_dev(struct ast_vhub *vhub, 
> unsigned int idx)
>  	device_del(d->port_dev);
>   fail_add:
>  	put_device(d->port_dev);
> + fail_alloc:
> +	kfree(d->epns);
>  
>  	return rc;
>  }
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c 
> b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> index 7475c74aa5c5..0bd6b20435b8 100644
> --- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> @@ -800,10 +800,10 @@ struct ast_vhub_ep *ast_vhub_alloc_epn(struct 
> ast_vhub_dev *d, u8 addr)
>  
>  	/* Find a free one (no device) */
>  	spin_lock_irqsave(&vhub->lock, flags);
> -	for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++)
> +	for (i = 0; i < vhub->max_epns; i++)
>  		if (vhub->epns[i].dev == NULL)
>  			break;
> -	if (i >= AST_VHUB_NUM_GEN_EPs) {
> +	if (i >= vhub->max_epns) {
>  		spin_unlock_irqrestore(&vhub->lock, flags);
>  		return NULL;
>  	}
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c 
> b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> index 19b3517e04c0..aa1c127e9f2f 100644
> --- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> @@ -133,10 +133,9 @@ static const struct ast_vhub_full_cdesc {
>  
>  #define AST_VHUB_HUB_DESC_SIZE	(USB_DT_HUB_NONVAR_SIZE + 2)
>  
> -static const struct usb_hub_descriptor ast_vhub_hub_desc = {
> +static struct usb_hub_descriptor ast_vhub_hub_desc = {

This seems unfortunate, though we only have one on the SoC... is
it worth dynamically allocating it? Or adding a comment?

Andrew

Re: [PATCH linux dev-5.4 v2 2/3] usb: gadget: aspeed: add ast2600 vhub support

[Andrew Jeffery]

On Thu, 23 Jan 2020, at 18:19, rentao.bupt@gmail.com wrote:
> From: Tao Ren <rentao.bupt@gmail.com>
> 
> Add AST2600 support in aspeed-vhub driver. There are 3 major differences
> between AST2500 and AST2600 vhub:
>   - AST2600 supports 7 downstream ports while AST2500 supports 5.
>   - AST2600 supports 21 generic endpoints while AST2500 supports 15.
>   - EP0 data buffer's 8-byte DMA alignment restriction is removed from
>     AST2600.
> 
> Signed-off-by: Tao Ren <rentao.bupt@gmail.com>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

Re: [PATCH linux dev-5.4 v2 3/3] ARM: dts: aspeed-g6: add usb functions

[Andrew Jeffery]

On Thu, 23 Jan 2020, at 18:19, rentao.bupt@gmail.com wrote:
> From: Tao Ren <rentao.bupt@gmail.com>
> 
> Add USB components and according pin groups in aspeed-g6 dtsi.
> 
> Signed-off-by: Tao Ren <rentao.bupt@gmail.com>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

Re: [PATCH linux dev-5.4 v2 1/3] usb: gadget: aspeed: read vhub config from of_device_id

[Tao Ren]

On Fri, Jan 24, 2020 at 10:39:45AM +1030, Andrew Jeffery wrote:
> 
> 
> On Thu, 23 Jan 2020, at 18:19, rentao.bupt@gmail.com wrote:
> > From: Tao Ren <rentao.bupt@gmail.com>
> > 
> > The patch moves hardcoded vhub attributes (maximum downstream ports and
> > generic endpoints) to "ast_vhub_config" structure which is attached to
> > struct of_device_id. By doing this, it will be very straightforward to
> > enable support of AST2600 vhub which has different number of downstream
> > ports and endpoints.
> > 
> > Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> > ---
> >  Changes in v2:
> >   - this is the first version. It's given v2 to align with the version
> >     of patch series.
> > 
> > 
> >  drivers/usb/gadget/udc/aspeed-vhub/core.c | 100 ++++++++++++++--------
> >  drivers/usb/gadget/udc/aspeed-vhub/dev.c  |  30 +++++--
> >  drivers/usb/gadget/udc/aspeed-vhub/epn.c  |   4 +-
> >  drivers/usb/gadget/udc/aspeed-vhub/hub.c  |  22 +++--
> >  drivers/usb/gadget/udc/aspeed-vhub/vhub.h |  21 ++---
> >  5 files changed, 107 insertions(+), 70 deletions(-)
> > 
> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/core.c 
> > b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > index 90b134d5dca9..9efbfdffad30 100644
> > --- a/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > @@ -32,6 +32,29 @@
> >  
> >  #include "vhub.h"
> >  
> > +struct ast_vhub_config {
> > +	u32 max_ports;	/* max number of downstream ports */
> > +	u32 max_epns;	/* max number of generic endpoints */
> > +};
> > +
> > +static const struct ast_vhub_config ast2400_config = {
> > +	.max_ports = 5,
> > +	.max_epns = 15,
> > +};
> > +
> > +static const struct of_device_id ast_vhub_dt_ids[] = {
> > +	{
> > +		.compatible = "aspeed,ast2400-usb-vhub",
> > +		.data = &ast2400_config,
> > +	},
> > +	{
> > +		.compatible = "aspeed,ast2500-usb-vhub",
> > +		.data = &ast2400_config,
> > +	},
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, ast_vhub_dt_ids);
> > +
> 
> Is there a reason to move the table from where it was? It's not a problem,
> just makes the diff a bit noisier.

I move the table before ast_vhub_probe() because it is referenced in the
function.

> 
> >  void ast_vhub_done(struct ast_vhub_ep *ep, struct ast_vhub_req *req,
> >  		   int status)
> >  {
> > @@ -99,7 +122,7 @@ static irqreturn_t ast_vhub_irq(int irq, void *data)
> >  {
> >  	struct ast_vhub *vhub = data;
> >  	irqreturn_t iret = IRQ_NONE;
> > -	u32 istat;
> > +	u32 i, istat;
> >  
> >  	/* Stale interrupt while tearing down */
> >  	if (!vhub->ep0_bufs)
> > @@ -121,10 +144,10 @@ static irqreturn_t ast_vhub_irq(int irq, void *data)
> >  
> >  	/* Handle generic EPs first */
> >  	if (istat & VHUB_IRQ_EP_POOL_ACK_STALL) {
> > -		u32 i, ep_acks = readl(vhub->regs + AST_VHUB_EP_ACK_ISR);
> > +		u32 ep_acks = readl(vhub->regs + AST_VHUB_EP_ACK_ISR);
> >  		writel(ep_acks, vhub->regs + AST_VHUB_EP_ACK_ISR);
> >  
> > -		for (i = 0; ep_acks && i < AST_VHUB_NUM_GEN_EPs; i++) {
> > +		for (i = 0; ep_acks && i < vhub->max_epns; i++) {
> >  			u32 mask = VHUB_EP_IRQ(i);
> >  			if (ep_acks & mask) {
> >  				ast_vhub_epn_ack_irq(&vhub->epns[i]);
> > @@ -134,21 +157,11 @@ static irqreturn_t ast_vhub_irq(int irq, void *data)
> >  	}
> >  
> >  	/* Handle device interrupts */
> > -	if (istat & (VHUB_IRQ_DEVICE1 |
> > -		     VHUB_IRQ_DEVICE2 |
> > -		     VHUB_IRQ_DEVICE3 |
> > -		     VHUB_IRQ_DEVICE4 |
> > -		     VHUB_IRQ_DEVICE5)) {
> > -		if (istat & VHUB_IRQ_DEVICE1)
> > -			ast_vhub_dev_irq(&vhub->ports[0].dev);
> > -		if (istat & VHUB_IRQ_DEVICE2)
> > -			ast_vhub_dev_irq(&vhub->ports[1].dev);
> > -		if (istat & VHUB_IRQ_DEVICE3)
> > -			ast_vhub_dev_irq(&vhub->ports[2].dev);
> > -		if (istat & VHUB_IRQ_DEVICE4)
> > -			ast_vhub_dev_irq(&vhub->ports[3].dev);
> > -		if (istat & VHUB_IRQ_DEVICE5)
> > -			ast_vhub_dev_irq(&vhub->ports[4].dev);
> > +	for (i = 0; i < vhub->max_ports; i++) {
> > +		u32 dev_mask = VHUB_IRQ_DEVICE1 << i;
> > +
> > +		if (istat & dev_mask)
> > +			ast_vhub_dev_irq(&vhub->ports[i].dev);
> >  	}
> >  
> >  	/* Handle top-level vHub EP0 interrupts */
> > @@ -182,7 +195,7 @@ static irqreturn_t ast_vhub_irq(int irq, void *data)
> >  
> >  void ast_vhub_init_hw(struct ast_vhub *vhub)
> >  {
> > -	u32 ctrl;
> > +	u32 ctrl, port_mask, epn_mask;
> >  
> >  	UDCDBG(vhub,"(Re)Starting HW ...\n");
> >  
> > @@ -222,15 +235,20 @@ void ast_vhub_init_hw(struct ast_vhub *vhub)
> >  	}
> >  
> >  	/* Reset all devices */
> > -	writel(VHUB_SW_RESET_ALL, vhub->regs + AST_VHUB_SW_RESET);
> > +	port_mask = GENMASK(vhub->max_ports, 1);
> > +	writel(VHUB_SW_RESET_ROOT_HUB |
> > +	       VHUB_SW_RESET_DMA_CONTROLLER |
> > +	       VHUB_SW_RESET_EP_POOL |
> > +	       port_mask, vhub->regs + AST_VHUB_SW_RESET);
> >  	udelay(1);
> >  	writel(0, vhub->regs + AST_VHUB_SW_RESET);
> >  
> >  	/* Disable and cleanup EP ACK/NACK interrupts */
> > +	epn_mask = GENMASK(vhub->max_epns - 1, 0);
> >  	writel(0, vhub->regs + AST_VHUB_EP_ACK_IER);
> >  	writel(0, vhub->regs + AST_VHUB_EP_NACK_IER);
> > -	writel(VHUB_EP_IRQ_ALL, vhub->regs + AST_VHUB_EP_ACK_ISR);
> > -	writel(VHUB_EP_IRQ_ALL, vhub->regs + AST_VHUB_EP_NACK_ISR);
> > +	writel(epn_mask, vhub->regs + AST_VHUB_EP_ACK_ISR);
> > +	writel(epn_mask, vhub->regs + AST_VHUB_EP_NACK_ISR);
> >  
> >  	/* Default settings for EP0, enable HW hub EP1 */
> >  	writel(0, vhub->regs + AST_VHUB_EP0_CTRL);
> > @@ -273,7 +291,7 @@ static int ast_vhub_remove(struct platform_device *pdev)
> >  		return 0;
> >  
> >  	/* Remove devices */
> > -	for (i = 0; i < AST_VHUB_NUM_PORTS; i++)
> > +	for (i = 0; i < vhub->max_ports; i++)
> >  		ast_vhub_del_dev(&vhub->ports[i].dev);
> >  
> >  	spin_lock_irqsave(&vhub->lock, flags);
> > @@ -295,7 +313,7 @@ static int ast_vhub_remove(struct platform_device *pdev)
> >  	if (vhub->ep0_bufs)
> >  		dma_free_coherent(&pdev->dev,
> >  				  AST_VHUB_EP0_MAX_PACKET *
> > -				  (AST_VHUB_NUM_PORTS + 1),
> > +				  (vhub->max_ports + 1),
> >  				  vhub->ep0_bufs,
> >  				  vhub->ep0_bufs_dma);
> >  	vhub->ep0_bufs = NULL;
> > @@ -309,11 +327,30 @@ static int ast_vhub_probe(struct platform_device *pdev)
> >  	struct ast_vhub *vhub;
> >  	struct resource *res;
> >  	int i, rc = 0;
> > +	const struct of_device_id *ofdid;
> > +	struct ast_vhub_config *config;
> >  
> >  	vhub = devm_kzalloc(&pdev->dev, sizeof(*vhub), GFP_KERNEL);
> >  	if (!vhub)
> >  		return -ENOMEM;
> >  
> > +	ofdid = of_match_node(ast_vhub_dt_ids, pdev->dev.of_node);
> > +	if (!ofdid)
> > +		return -EINVAL;
> > +	config = ofdid->data;
> > +
> > +	vhub->max_ports = config->max_ports;
> > +	vhub->ports = devm_kcalloc(&pdev->dev, vhub->max_ports,
> > +				   sizeof(*vhub->ports), GFP_KERNEL);
> > +	if (!vhub->ports)
> > +		return -ENOMEM;
> > +
> > +	vhub->max_epns = config->max_epns;
> > +	vhub->epns = devm_kcalloc(&pdev->dev, vhub->max_epns,
> > +				  sizeof(*vhub->epns), GFP_KERNEL);
> > +	if (!vhub->epns)
> > +		return -ENOMEM;
> > +
> >  	spin_lock_init(&vhub->lock);
> >  	vhub->pdev = pdev;
> >  
> > @@ -366,7 +403,7 @@ static int ast_vhub_probe(struct platform_device *pdev)
> >  	 */
> >  	vhub->ep0_bufs = dma_alloc_coherent(&pdev->dev,
> >  					    AST_VHUB_EP0_MAX_PACKET *
> > -					    (AST_VHUB_NUM_PORTS + 1),
> > +					    (vhub->max_ports + 1),
> >  					    &vhub->ep0_bufs_dma, GFP_KERNEL);
> >  	if (!vhub->ep0_bufs) {
> >  		dev_err(&pdev->dev, "Failed to allocate EP0 DMA buffers\n");
> > @@ -380,7 +417,7 @@ static int ast_vhub_probe(struct platform_device *pdev)
> >  	ast_vhub_init_ep0(vhub, &vhub->ep0, NULL);
> >  
> >  	/* Init devices */
> > -	for (i = 0; i < AST_VHUB_NUM_PORTS && rc == 0; i++)
> > +	for (i = 0; i < vhub->max_ports && rc == 0; i++)
> >  		rc = ast_vhub_init_dev(vhub, i);
> >  	if (rc)
> >  		goto err;
> > @@ -400,17 +437,6 @@ static int ast_vhub_probe(struct platform_device *pdev)
> >  	return rc;
> >  }
> >  
> > -static const struct of_device_id ast_vhub_dt_ids[] = {
> > -	{
> > -		.compatible = "aspeed,ast2400-usb-vhub",
> > -	},
> > -	{
> > -		.compatible = "aspeed,ast2500-usb-vhub",
> > -	},
> > -	{ }
> > -};
> > -MODULE_DEVICE_TABLE(of, ast_vhub_dt_ids);
> > -
> >  static struct platform_driver ast_vhub_driver = {
> >  	.probe		= ast_vhub_probe,
> >  	.remove		= ast_vhub_remove,
> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/dev.c 
> > b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> > index 4008e7a51188..f9b762951121 100644
> > --- a/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> > @@ -77,7 +77,7 @@ static void ast_vhub_dev_enable(struct ast_vhub_dev 
> > *d)
> >  	writel(d->ep0.buf_dma, d->regs + AST_VHUB_DEV_EP0_DATA);
> >  
> >  	/* Clear stall on all EPs */
> > -	for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++) {
> > +	for (i = 0; i < d->max_epns; i++) {
> >  		struct ast_vhub_ep *ep = d->epns[i];
> >  
> >  		if (ep && (ep->epn.stalled || ep->epn.wedged)) {
> > @@ -137,7 +137,7 @@ static int ast_vhub_ep_feature(struct ast_vhub_dev *d,
> >  	     is_set ? "SET" : "CLEAR", ep_num, wValue);
> >  	if (ep_num == 0)
> >  		return std_req_complete;
> > -	if (ep_num >= AST_VHUB_NUM_GEN_EPs || !d->epns[ep_num - 1])
> > +	if (ep_num >= d->max_epns || !d->epns[ep_num - 1])
> >  		return std_req_stall;
> >  	if (wValue != USB_ENDPOINT_HALT)
> >  		return std_req_driver;
> > @@ -181,7 +181,7 @@ static int ast_vhub_ep_status(struct ast_vhub_dev *d,
> >  
> >  	DDBG(d, "GET_STATUS(ep%d)\n", ep_num);
> >  
> > -	if (ep_num >= AST_VHUB_NUM_GEN_EPs)
> > +	if (ep_num >= d->max_epns)
> >  		return std_req_stall;
> >  	if (ep_num != 0) {
> >  		ep = d->epns[ep_num - 1];
> > @@ -299,7 +299,7 @@ static void ast_vhub_dev_nuke(struct ast_vhub_dev *d)
> >  {
> >  	unsigned int i;
> >  
> > -	for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++) {
> > +	for (i = 0; i < d->max_epns; i++) {
> >  		if (!d->epns[i])
> >  			continue;
> >  		ast_vhub_nuke(d->epns[i], -ESHUTDOWN);
> > @@ -416,10 +416,10 @@ static struct usb_ep 
> > *ast_vhub_udc_match_ep(struct usb_gadget *gadget,
> >  	 * that will allow the generic code to use our
> >  	 * assigned address.
> >  	 */
> > -	for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++)
> > +	for (i = 0; i < d->max_epns; i++)
> >  		if (d->epns[i] == NULL)
> >  			break;
> > -	if (i >= AST_VHUB_NUM_GEN_EPs)
> > +	if (i >= d->max_epns)
> >  		return NULL;
> >  	addr = i + 1;
> >  
> > @@ -526,6 +526,7 @@ void ast_vhub_del_dev(struct ast_vhub_dev *d)
> >  
> >  	usb_del_gadget_udc(&d->gadget);
> >  	device_unregister(d->port_dev);
> > +	kfree(d->epns);
> >  }
> >  
> >  static void ast_vhub_dev_release(struct device *dev)
> > @@ -546,14 +547,25 @@ int ast_vhub_init_dev(struct ast_vhub *vhub, 
> > unsigned int idx)
> >  
> >  	ast_vhub_init_ep0(vhub, &d->ep0, d);
> >  
> > +	/*
> > +	 * A USB device can have up to 30 endpoints besides control
> > +	 * endpoint 0.
> > +	 */
> > +	d->max_epns = min(vhub->max_epns, 30);
> > +	d->epns = kcalloc(d->max_epns, sizeof(*d->epns), GFP_KERNEL);
> > +	if (!d->epns)
> > +		return -ENOMEM;
> > +
> >  	/*
> >  	 * The UDC core really needs us to have separate and uniquely
> >  	 * named "parent" devices for each port so we create a sub device
> >  	 * here for that purpose
> >  	 */
> >  	d->port_dev = kzalloc(sizeof(struct device), GFP_KERNEL);
> > -	if (!d->port_dev)
> > -		return -ENOMEM;
> > +	if (!d->port_dev) {
> > +		rc = -ENOMEM;
> > +		goto fail_alloc;
> > +	}
> >  	device_initialize(d->port_dev);
> >  	d->port_dev->release = ast_vhub_dev_release;
> >  	d->port_dev->parent = parent;
> > @@ -584,6 +596,8 @@ int ast_vhub_init_dev(struct ast_vhub *vhub, 
> > unsigned int idx)
> >  	device_del(d->port_dev);
> >   fail_add:
> >  	put_device(d->port_dev);
> > + fail_alloc:
> > +	kfree(d->epns);
> >  
> >  	return rc;
> >  }
> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c 
> > b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > index 7475c74aa5c5..0bd6b20435b8 100644
> > --- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
> > @@ -800,10 +800,10 @@ struct ast_vhub_ep *ast_vhub_alloc_epn(struct 
> > ast_vhub_dev *d, u8 addr)
> >  
> >  	/* Find a free one (no device) */
> >  	spin_lock_irqsave(&vhub->lock, flags);
> > -	for (i = 0; i < AST_VHUB_NUM_GEN_EPs; i++)
> > +	for (i = 0; i < vhub->max_epns; i++)
> >  		if (vhub->epns[i].dev == NULL)
> >  			break;
> > -	if (i >= AST_VHUB_NUM_GEN_EPs) {
> > +	if (i >= vhub->max_epns) {
> >  		spin_unlock_irqrestore(&vhub->lock, flags);
> >  		return NULL;
> >  	}
> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c 
> > b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > index 19b3517e04c0..aa1c127e9f2f 100644
> > --- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > @@ -133,10 +133,9 @@ static const struct ast_vhub_full_cdesc {
> >  
> >  #define AST_VHUB_HUB_DESC_SIZE	(USB_DT_HUB_NONVAR_SIZE + 2)
> >  
> > -static const struct usb_hub_descriptor ast_vhub_hub_desc = {
> > +static struct usb_hub_descriptor ast_vhub_hub_desc = {
> 
> This seems unfortunate, though we only have one on the SoC... is
> it worth dynamically allocating it? Or adding a comment?
> 
> Andrew

According to the comment at the beginning of the file (line 39-47), we
may customize more descriptors in the future. Adding comments involves
little change in this case, because by allocating the descriptors, we
will also need a function to free the descriptors when ast_vhub_remove
is called. Anyways I'm fine with either way.

There is another option which is to fixup descriptors in request_desc
callback, like ast_vhub_patch_dev_desc_usb1() in the file. But I don't
like the approach personally.

Which way do you prefer?


Cheers,

Tao

Re: [PATCH linux dev-5.4 v2 3/3] ARM: dts: aspeed-g6: add usb functions

[Tao Ren]

On Fri, Jan 24, 2020 at 10:42:15AM +1030, Andrew Jeffery wrote:
> 
> 
> On Thu, 23 Jan 2020, at 18:19, rentao.bupt@gmail.com wrote:
> > From: Tao Ren <rentao.bupt@gmail.com>
> > 
> > Add USB components and according pin groups in aspeed-g6 dtsi.
> > 
> > Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> 
> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

Thank you for the quick review Andrew!


Cheers,

Tao

Re: [PATCH linux dev-5.4 v2 1/3] usb: gadget: aspeed: read vhub config from of_device_id

[Andrew Jeffery]

On Fri, 24 Jan 2020, at 11:53, Tao Ren wrote:
> On Fri, Jan 24, 2020 at 10:39:45AM +1030, Andrew Jeffery wrote:
> > > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c 
> > > b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > > index 19b3517e04c0..aa1c127e9f2f 100644
> > > --- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > > +++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > > @@ -133,10 +133,9 @@ static const struct ast_vhub_full_cdesc {
> > >  
> > >  #define AST_VHUB_HUB_DESC_SIZE	(USB_DT_HUB_NONVAR_SIZE + 2)
> > >  
> > > -static const struct usb_hub_descriptor ast_vhub_hub_desc = {
> > > +static struct usb_hub_descriptor ast_vhub_hub_desc = {
> > 
> > This seems unfortunate, though we only have one on the SoC... is
> > it worth dynamically allocating it? Or adding a comment?
> > 
> > Andrew
> 
> According to the comment at the beginning of the file (line 39-47), we
> may customize more descriptors in the future. Adding comments involves
> little change in this case, because by allocating the descriptors, we
> will also need a function to free the descriptors when ast_vhub_remove
> is called. Anyways I'm fine with either way.
> 
> There is another option which is to fixup descriptors in request_desc
> callback, like ast_vhub_patch_dev_desc_usb1() in the file. But I don't
> like the approach personally.
> 
> Which way do you prefer?

I'm not wedded to doing anything different from what you've already got
in the patch - we don't have hardware that requires a different solution at
this point. We can always fix the driver if that changes. The approach just
felt a bit icky, but I can live with that :)

Andrew

Re: [PATCH linux dev-5.4 v2 1/3] usb: gadget: aspeed: read vhub config from of_device_id

[Tao Ren]

On Tue, Jan 28, 2020 at 11:27:57AM +1030, Andrew Jeffery wrote:
> 
> 
> On Fri, 24 Jan 2020, at 11:53, Tao Ren wrote:
> > On Fri, Jan 24, 2020 at 10:39:45AM +1030, Andrew Jeffery wrote:
> > > > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c 
> > > > b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > > > index 19b3517e04c0..aa1c127e9f2f 100644
> > > > --- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > > > +++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> > > > @@ -133,10 +133,9 @@ static const struct ast_vhub_full_cdesc {
> > > >  
> > > >  #define AST_VHUB_HUB_DESC_SIZE	(USB_DT_HUB_NONVAR_SIZE + 2)
> > > >  
> > > > -static const struct usb_hub_descriptor ast_vhub_hub_desc = {
> > > > +static struct usb_hub_descriptor ast_vhub_hub_desc = {
> > > 
> > > This seems unfortunate, though we only have one on the SoC... is
> > > it worth dynamically allocating it? Or adding a comment?
> > > 
> > > Andrew
> > 
> > According to the comment at the beginning of the file (line 39-47), we
> > may customize more descriptors in the future. Adding comments involves
> > little change in this case, because by allocating the descriptors, we
> > will also need a function to free the descriptors when ast_vhub_remove
> > is called. Anyways I'm fine with either way.
> > 
> > There is another option which is to fixup descriptors in request_desc
> > callback, like ast_vhub_patch_dev_desc_usb1() in the file. But I don't
> > like the approach personally.
> > 
> > Which way do you prefer?
> 
> I'm not wedded to doing anything different from what you've already got
> in the patch - we don't have hardware that requires a different solution at
> this point. We can always fix the driver if that changes. The approach just
> felt a bit icky, but I can live with that :)
> 
> Andrew

Thanks Andrew. I just sent out patch v3 which adds some comments for the
hub descriptor; 2 compile warnings are also fixed in v3. Please kindly
review when you have bandwidth.

Cheers,

Tao

Re: [PATCH linux dev-5.4 v2 0/3] aspeed-g6: enable usb support

[Joel Stanley]

On Thu, 23 Jan 2020 at 07:50, <rentao.bupt@gmail.com> wrote:
>
> From: Tao Ren <rentao.bupt@gmail.com>
>
> The patch series aims at enabling USB Host and Gadget support on AST2600
> platforms. I'm targeting openbmc tree mainly for some early feedback and
> more widespread testing. I'm planning to upstream the patches after
> 5.6-rc1.
>
> Patch #1 moves hardcoded vhub attributes (number of downstream ports and
> endpoints) to "struct ast_hub_config" which is then attached to "struct
> of_device_id". By doing this, it will be easier to enable ast2600 vhub
> which supports more ports and endpoints.
>
> Patch #2 enables AST2600 support in aspeed-vhub gadget driver.
>
> Patch #3 adds USB devices and according pin groups in aspeed-g6 dtsi.

I have put these in the openbmc tree. Please send the changes upstream
for review asap (I would do it now) so we can get Ben's feedback on
them.

Cheers,

Joel

>
> Tao Ren (3):
>   usb: gadget: aspeed: read vhub config from of_device_id
>   usb: gadget: aspeed: add ast2600 vhub support
>   ARM: dts: aspeed-g6: add usb functions
>
>  arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi   |  25 +++++
>  arch/arm/boot/dts/aspeed-g6.dtsi           |  43 ++++++++
>  drivers/usb/gadget/udc/aspeed-vhub/Kconfig |   4 +-
>  drivers/usb/gadget/udc/aspeed-vhub/core.c  | 108 ++++++++++++++-------
>  drivers/usb/gadget/udc/aspeed-vhub/dev.c   |  30 ++++--
>  drivers/usb/gadget/udc/aspeed-vhub/epn.c   |   4 +-
>  drivers/usb/gadget/udc/aspeed-vhub/hub.c   |  22 +++--
>  drivers/usb/gadget/udc/aspeed-vhub/vhub.h  |  21 ++--
>  8 files changed, 185 insertions(+), 72 deletions(-)
>
> --
> 2.17.1
>

Re: [PATCH linux dev-5.4 v2 0/3] aspeed-g6: enable usb support

[Tao Ren]

On Fri, Jan 31, 2020 at 04:00:29AM +0000, Joel Stanley wrote:
> On Thu, 23 Jan 2020 at 07:50, <rentao.bupt@gmail.com> wrote:
> >
> > From: Tao Ren <rentao.bupt@gmail.com>
> >
> > The patch series aims at enabling USB Host and Gadget support on AST2600
> > platforms. I'm targeting openbmc tree mainly for some early feedback and
> > more widespread testing. I'm planning to upstream the patches after
> > 5.6-rc1.
> >
> > Patch #1 moves hardcoded vhub attributes (number of downstream ports and
> > endpoints) to "struct ast_hub_config" which is then attached to "struct
> > of_device_id". By doing this, it will be easier to enable ast2600 vhub
> > which supports more ports and endpoints.
> >
> > Patch #2 enables AST2600 support in aspeed-vhub gadget driver.
> >
> > Patch #3 adds USB devices and according pin groups in aspeed-g6 dtsi.
> 
> I have put these in the openbmc tree. Please send the changes upstream
> for review asap (I would do it now) so we can get Ben's feedback on
> them.
> 
> Cheers,
> 
> Joel

Thanks Joel. I've already sent the patch series to upstream.


Cheers,

Tao