[PATCH linux-next v2 0/1] usb: gadget: udc: atmel: Update endpoint allocation scheme

[PATCH linux-next v2 0/1] usb: gadget: udc: atmel: Update endpoint allocation scheme

[cristian.birsan at microchip.com]

From: Cristian Birsan <cristian.birsan@microchip.com>

Hi,

This patch updates the usb endpoint allocation scheme for atmel usba
driver to make sure all endpoints are allocated in order. This requirement
comes from the datasheet of the controller.

The allocation scheme is decided by fifo_mode parameter. For fifo_mode = 0
the driver tries to autoconfigure the endpoints fifo size. All other modes
contain fixed configurations optimized for different purposes. The idea is
somehow similar with the approach used on musb driver.

Please let me know if you have any comments or suggestions.

Changes since v1:
	- Minor reworks based on received fedback


Kind regards,
Cristian

Cristian Birsan (1):
  usb: gadget: udc: atmel: Update endpoint allocation scheme

 drivers/usb/gadget/udc/Kconfig          |  14 ++
 drivers/usb/gadget/udc/atmel_usba_udc.c | 236 +++++++++++++++++++++++++++-----
 drivers/usb/gadget/udc/atmel_usba_udc.h |  10 +-
 3 files changed, 227 insertions(+), 33 deletions(-)

-- 
2.7.4

[PATCH linux-next v2 0/1] usb: gadget: udc: atmel: Update endpoint allocation scheme

[cristian.birsan]

From: Cristian Birsan <cristian.birsan@microchip.com>

Hi,

This patch updates the usb endpoint allocation scheme for atmel usba
driver to make sure all endpoints are allocated in order. This requirement
comes from the datasheet of the controller.

The allocation scheme is decided by fifo_mode parameter. For fifo_mode = 0
the driver tries to autoconfigure the endpoints fifo size. All other modes
contain fixed configurations optimized for different purposes. The idea is
somehow similar with the approach used on musb driver.

Please let me know if you have any comments or suggestions.

Changes since v1:
	- Minor reworks based on received fedback


Kind regards,
Cristian

Cristian Birsan (1):
  usb: gadget: udc: atmel: Update endpoint allocation scheme

 drivers/usb/gadget/udc/Kconfig          |  14 ++
 drivers/usb/gadget/udc/atmel_usba_udc.c | 236 +++++++++++++++++++++++++++-----
 drivers/usb/gadget/udc/atmel_usba_udc.h |  10 +-
 3 files changed, 227 insertions(+), 33 deletions(-)

-- 
2.7.4

[PATCH linux-next v2 1/1] usb: gadget: udc: atmel: Update endpoint allocation scheme

[cristian.birsan at microchip.com]

From: Cristian Birsan <cristian.birsan@microchip.com>

Update atmel udc driver with a new enpoint allocation scheme. The data
sheet requires that all endpoints are allocated in order.

Signed-off-by: Cristian Birsan <cristian.birsan@microchip.com>
---
 drivers/usb/gadget/udc/Kconfig          |  14 ++
 drivers/usb/gadget/udc/atmel_usba_udc.c | 236 +++++++++++++++++++++++++++-----
 drivers/usb/gadget/udc/atmel_usba_udc.h |  10 +-
 3 files changed, 227 insertions(+), 33 deletions(-)

diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
index 658b8da..4b69f28 100644
--- a/drivers/usb/gadget/udc/Kconfig
+++ b/drivers/usb/gadget/udc/Kconfig
@@ -60,6 +60,20 @@ config USB_ATMEL_USBA
 	  USBA is the integrated high-speed USB Device controller on
 	  the AT32AP700x, some AT91SAM9 and AT91CAP9 processors from Atmel.
 
+	  The fifo_mode parameter is used to select endpoint allocation mode.
+	  fifo_mode = 0 is used to let the driver autoconfigure the endpoints.
+	  In this case 2 banks are allocated for isochronous endpoints and
+	  only one bank is allocated for the rest of the endpoints.
+
+	  fifo_mode = 1 is a generic maximum fifo size (1024 bytes) configuration
+	  allowing the usage of ep1 - ep6
+
+	  fifo_mode = 2 is a generic performance maximum fifo size (1024 bytes)
+	  configuration allowing the usage of ep1 - ep3
+
+	  fifo_mode = 3 is a balanced performance configuration allowing the
+	  the usage of ep1 - ep8
+
 config USB_BCM63XX_UDC
 	tristate "Broadcom BCM63xx Peripheral Controller"
 	depends on BCM63XX
diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
index 12c7687..11bbce2 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.c
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
@@ -20,6 +20,7 @@
 #include <linux/mfd/syscon.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
+#include <linux/ctype.h>
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
 #include <linux/usb/atmel_usba_udc.h>
@@ -318,6 +319,91 @@ static inline void usba_cleanup_debugfs(struct usba_udc *udc)
 }
 #endif
 
+static ushort fifo_mode;
+
+/* "modprobe ... fifo_mode=1" etc */
+module_param(fifo_mode, ushort, 0x0);
+MODULE_PARM_DESC(fifo_mode, "Endpoint configuration mode");
+
+/* mode 0 - uses autoconfig */
+
+/* mode 1 - fits in 8KB, generic max fifo configuration */
+static struct usba_fifo_cfg mode_1_cfg[] = {
+{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
+{ .hw_ep_num = 1, .fifo_size = 1024,	.nr_banks = 2, },
+{ .hw_ep_num = 2, .fifo_size = 1024,	.nr_banks = 1, },
+{ .hw_ep_num = 3, .fifo_size = 1024,	.nr_banks = 1, },
+{ .hw_ep_num = 4, .fifo_size = 1024,	.nr_banks = 1, },
+{ .hw_ep_num = 5, .fifo_size = 1024,	.nr_banks = 1, },
+{ .hw_ep_num = 6, .fifo_size = 1024,	.nr_banks = 1, },
+};
+
+/* mode 2 - fits in 8KB, performance max fifo configuration */
+static struct usba_fifo_cfg mode_2_cfg[] = {
+{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
+{ .hw_ep_num = 1, .fifo_size = 1024,	.nr_banks = 3, },
+{ .hw_ep_num = 2, .fifo_size = 1024,	.nr_banks = 2, },
+{ .hw_ep_num = 3, .fifo_size = 1024,	.nr_banks = 2, },
+};
+
+/* mode 3 - fits in 8KB, mixed fifo configuration */
+static struct usba_fifo_cfg mode_3_cfg[] = {
+{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
+{ .hw_ep_num = 1, .fifo_size = 1024,	.nr_banks = 2, },
+{ .hw_ep_num = 2, .fifo_size = 512,	.nr_banks = 2, },
+{ .hw_ep_num = 3, .fifo_size = 512,	.nr_banks = 2, },
+{ .hw_ep_num = 4, .fifo_size = 512,	.nr_banks = 2, },
+{ .hw_ep_num = 5, .fifo_size = 512,	.nr_banks = 2, },
+{ .hw_ep_num = 6, .fifo_size = 512,	.nr_banks = 2, },
+};
+
+/* mode 4 - fits in 8KB, custom fifo configuration */
+static struct usba_fifo_cfg mode_4_cfg[] = {
+{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
+{ .hw_ep_num = 1, .fifo_size = 512,	.nr_banks = 2, },
+{ .hw_ep_num = 2, .fifo_size = 512,	.nr_banks = 2, },
+{ .hw_ep_num = 3, .fifo_size = 8,	.nr_banks = 2, },
+{ .hw_ep_num = 4, .fifo_size = 512,	.nr_banks = 2, },
+{ .hw_ep_num = 5, .fifo_size = 512,	.nr_banks = 2, },
+{ .hw_ep_num = 6, .fifo_size = 16,	.nr_banks = 2, },
+{ .hw_ep_num = 7, .fifo_size = 8,	.nr_banks = 2, },
+{ .hw_ep_num = 8, .fifo_size = 8,	.nr_banks = 2, },
+};
+/* Add additional configurations here */
+
+int usba_config_fifo_table(struct usba_udc *udc)
+{
+	int n;
+
+	switch (fifo_mode) {
+	default:
+		fifo_mode = 0;
+	case 0:
+		udc->fifo_cfg = NULL;
+		n = 0;
+		break;
+	case 1:
+		udc->fifo_cfg = mode_1_cfg;
+		n = ARRAY_SIZE(mode_1_cfg);
+		break;
+	case 2:
+		udc->fifo_cfg = mode_2_cfg;
+		n = ARRAY_SIZE(mode_2_cfg);
+		break;
+	case 3:
+		udc->fifo_cfg = mode_3_cfg;
+		n = ARRAY_SIZE(mode_3_cfg);
+		break;
+	case 4:
+		udc->fifo_cfg = mode_4_cfg;
+		n = ARRAY_SIZE(mode_4_cfg);
+		break;
+	}
+	DBG(DBG_HW, "Setup fifo_mode %d\n", fifo_mode);
+
+	return n;
+}
+
 static inline u32 usba_int_enb_get(struct usba_udc *udc)
 {
 	return udc->int_enb_cache;
@@ -543,24 +629,17 @@ usba_ep_enable(struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
 	ep->is_isoc = 0;
 	ep->is_in = 0;
 
-	if (maxpacket <= 8)
-		ept_cfg = USBA_BF(EPT_SIZE, USBA_EPT_SIZE_8);
-	else
-		/* LSB is bit 1, not 0 */
-		ept_cfg = USBA_BF(EPT_SIZE, fls(maxpacket - 1) - 3);
-
-	DBG(DBG_HW, "%s: EPT_SIZE = %lu (maxpacket = %lu)\n",
+	DBG(DBG_ERR, "%s: EPT_CFG = 0x%lx (maxpacket = %lu)\n",
 			ep->ep.name, ept_cfg, maxpacket);
 
 	if (usb_endpoint_dir_in(desc)) {
 		ep->is_in = 1;
-		ept_cfg |= USBA_EPT_DIR_IN;
+		ep->ept_cfg |= USBA_EPT_DIR_IN;
 	}
 
 	switch (usb_endpoint_type(desc)) {
 	case USB_ENDPOINT_XFER_CONTROL:
-		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_CONTROL);
-		ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_ONE);
+		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_CONTROL);
 		break;
 	case USB_ENDPOINT_XFER_ISOC:
 		if (!ep->can_isoc) {
@@ -578,24 +657,15 @@ usba_ep_enable(struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
 			return -EINVAL;
 
 		ep->is_isoc = 1;
-		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_ISO);
+		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_ISO);
+		ep->ept_cfg |= USBA_BF(NB_TRANS, nr_trans);
 
-		/*
-		 * Do triple-buffering on high-bandwidth iso endpoints.
-		 */
-		if (nr_trans > 1 && ep->nr_banks == 3)
-			ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_TRIPLE);
-		else
-			ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_DOUBLE);
-		ept_cfg |= USBA_BF(NB_TRANS, nr_trans);
 		break;
 	case USB_ENDPOINT_XFER_BULK:
-		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_BULK);
-		ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_DOUBLE);
+		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_BULK);
 		break;
 	case USB_ENDPOINT_XFER_INT:
-		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_INT);
-		ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_DOUBLE);
+		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_INT);
 		break;
 	}
 
@@ -604,7 +674,7 @@ usba_ep_enable(struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
 	ep->ep.desc = desc;
 	ep->ep.maxpacket = maxpacket;
 
-	usba_ep_writel(ep, CFG, ept_cfg);
+	usba_ep_writel(ep, CFG, ep->ept_cfg);
 	usba_ep_writel(ep, CTL_ENB, USBA_EPT_ENABLE);
 
 	if (ep->can_dma) {
@@ -1006,12 +1076,81 @@ static int atmel_usba_start(struct usb_gadget *gadget,
 		struct usb_gadget_driver *driver);
 static int atmel_usba_stop(struct usb_gadget *gadget);
 
+static struct usb_ep *atmel_usba_match_ep(
+		struct usb_gadget		*gadget,
+		struct usb_endpoint_descriptor	*desc,
+		struct usb_ss_ep_comp_descriptor *ep_comp
+)
+{
+	struct usb_ep	*_ep;
+	struct usba_ep *ep;
+
+	/* Look at endpoints until an unclaimed one looks usable */
+	list_for_each_entry(_ep, &gadget->ep_list, ep_list) {
+		if (usb_gadget_ep_match_desc(gadget, _ep, desc, ep_comp))
+			goto found_ep;
+	}
+	/* Fail */
+	return NULL;
+
+found_ep:
+
+	if (fifo_mode == 0) {
+		/* Optimize hw fifo size based on ep type and other info */
+		ep = to_usba_ep(_ep);
+
+		switch (usb_endpoint_type(desc)) {
+
+		case USB_ENDPOINT_XFER_CONTROL:
+			break;
+
+		case USB_ENDPOINT_XFER_ISOC:
+			ep->fifo_size = 1024;
+			ep->nr_banks = 2;
+			break;
+
+		case USB_ENDPOINT_XFER_BULK:
+			ep->fifo_size = 512;
+			ep->nr_banks = 1;
+			break;
+
+		case USB_ENDPOINT_XFER_INT:
+			if (desc->wMaxPacketSize == 0)
+				ep->fifo_size =
+				    roundup_pow_of_two(_ep->maxpacket_limit);
+			else
+				ep->fifo_size =
+				    roundup_pow_of_two(le16_to_cpu(desc->wMaxPacketSize));
+			ep->nr_banks = 1;
+			break;
+		}
+
+		/* It might be a little bit late to set this */
+		usb_ep_set_maxpacket_limit(&ep->ep, ep->fifo_size);
+
+		/* Generate ept_cfg basd on FIFO size and number of banks */
+		if (ep->fifo_size  <= 8)
+			ep->ept_cfg = USBA_BF(EPT_SIZE, USBA_EPT_SIZE_8);
+		else
+			/* LSB is bit 1, not 0 */
+			ep->ept_cfg =
+				USBA_BF(EPT_SIZE, fls(ep->fifo_size - 1) - 3);
+
+		ep->ept_cfg |= USBA_BF(BK_NUMBER, ep->nr_banks);
+
+		ep->udc->configured_ep++;
+	}
+
+return _ep;
+}
+
 static const struct usb_gadget_ops usba_udc_ops = {
 	.get_frame		= usba_udc_get_frame,
 	.wakeup			= usba_udc_wakeup,
 	.set_selfpowered	= usba_udc_set_selfpowered,
 	.udc_start		= atmel_usba_start,
 	.udc_stop		= atmel_usba_stop,
+	.match_ep		= atmel_usba_match_ep,
 };
 
 static struct usb_endpoint_descriptor usba_ep0_desc = {
@@ -1678,7 +1817,8 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
 	}
 
 	if (status & USBA_END_OF_RESET) {
-		struct usba_ep *ep0;
+		struct usba_ep *ep0, *ep;
+		int i, n;
 
 		usba_writel(udc, INT_CLR, USBA_END_OF_RESET);
 		generate_bias_pulse(udc);
@@ -1717,6 +1857,16 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
 		if (!(usba_ep_readl(ep0, CFG) & USBA_EPT_MAPPED))
 			dev_dbg(&udc->pdev->dev,
 				 "ODD: EP0 configuration is invalid!\n");
+
+		/* Preallocate other endpoints */
+		n = fifo_mode ? udc->num_ep : udc->configured_ep;
+		for (i = 1; i < n; i++) {
+			ep = &udc->usba_ep[i];
+			usba_ep_writel(ep, CFG, ep->ept_cfg);
+			if (!(usba_ep_readl(ep, CFG) & USBA_EPT_MAPPED))
+				dev_dbg(&udc->pdev->dev,
+				 "ODD: EP%d configuration is invalid!\n", i);
+		}
 	}
 
 	spin_unlock(&udc->lock);
@@ -1864,6 +2014,9 @@ static int atmel_usba_stop(struct usb_gadget *gadget)
 	if (gpio_is_valid(udc->vbus_pin))
 		disable_irq(gpio_to_irq(udc->vbus_pin));
 
+	if (fifo_mode == 0)
+		udc->configured_ep = 1;
+
 	usba_stop(udc);
 
 	udc->driver = NULL;
@@ -1931,9 +2084,13 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
 						&flags);
 	udc->vbus_pin_inverted = (flags & OF_GPIO_ACTIVE_LOW) ? 1 : 0;
 
-	pp = NULL;
-	while ((pp = of_get_next_child(np, pp)))
-		udc->num_ep++;
+	if (fifo_mode == 0) {
+		pp = NULL;
+		while ((pp = of_get_next_child(np, pp)))
+			udc->num_ep++;
+		udc->configured_ep = 1;
+	} else
+		udc->num_ep = usba_config_fifo_table(udc);
 
 	eps = devm_kzalloc(&pdev->dev, sizeof(struct usba_ep) * udc->num_ep,
 			   GFP_KERNEL);
@@ -1946,7 +2103,7 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
 
 	pp = NULL;
 	i = 0;
-	while ((pp = of_get_next_child(np, pp))) {
+	while ((pp = of_get_next_child(np, pp)) && i < udc->num_ep) {
 		ep = &eps[i];
 
 		ret = of_property_read_u32(pp, "reg", &val);
@@ -1954,21 +2111,21 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
 			dev_err(&pdev->dev, "of_probe: reg error(%d)\n", ret);
 			goto err;
 		}
-		ep->index = val;
+		ep->index = fifo_mode ? udc->fifo_cfg[i].hw_ep_num : val;
 
 		ret = of_property_read_u32(pp, "atmel,fifo-size", &val);
 		if (ret) {
 			dev_err(&pdev->dev, "of_probe: fifo-size error(%d)\n", ret);
 			goto err;
 		}
-		ep->fifo_size = val;
+		ep->fifo_size = fifo_mode ? udc->fifo_cfg[i].fifo_size : val;
 
 		ret = of_property_read_u32(pp, "atmel,nb-banks", &val);
 		if (ret) {
 			dev_err(&pdev->dev, "of_probe: nb-banks error(%d)\n", ret);
 			goto err;
 		}
-		ep->nr_banks = val;
+		ep->nr_banks = fifo_mode ? udc->fifo_cfg[i].nr_banks : val;
 
 		ep->can_dma = of_property_read_bool(pp, "atmel,can-dma");
 		ep->can_isoc = of_property_read_bool(pp, "atmel,can-isoc");
@@ -2000,6 +2157,21 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
 		ep->ep.caps.dir_in = true;
 		ep->ep.caps.dir_out = true;
 
+		if (fifo_mode != 0) {
+			/*
+			 * Generate ept_cfg based on FIFO size and
+			 * banks number
+			 */
+			if (ep->fifo_size  <= 8)
+				ep->ept_cfg = USBA_BF(EPT_SIZE, USBA_EPT_SIZE_8);
+			else
+				/* LSB is bit 1, not 0 */
+				ep->ept_cfg =
+				  USBA_BF(EPT_SIZE, fls(ep->fifo_size - 1) - 3);
+
+			ep->ept_cfg |= USBA_BF(BK_NUMBER, ep->nr_banks);
+		}
+
 		if (i)
 			list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
 
diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.h b/drivers/usb/gadget/udc/atmel_usba_udc.h
index b03b2eb..9551b70 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.h
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.h
@@ -275,6 +275,12 @@ struct usba_dma_desc {
 	u32 ctrl;
 };
 
+struct usba_fifo_cfg {
+	u8			hw_ep_num;
+	u16			fifo_size;
+	u8			nr_banks;
+};
+
 struct usba_ep {
 	int					state;
 	void __iomem				*ep_regs;
@@ -293,7 +299,7 @@ struct usba_ep {
 	unsigned int				can_isoc:1;
 	unsigned int				is_isoc:1;
 	unsigned int				is_in:1;
-
+	unsigned long				ept_cfg;
 #ifdef CONFIG_USB_GADGET_DEBUG_FS
 	u32					last_dma_status;
 	struct dentry				*debugfs_dir;
@@ -338,6 +344,8 @@ struct usba_udc {
 	int vbus_pin;
 	int vbus_pin_inverted;
 	int num_ep;
+	int configured_ep;
+	struct usba_fifo_cfg *fifo_cfg;
 	struct clk *pclk;
 	struct clk *hclk;
 	struct usba_ep *usba_ep;
-- 
2.7.4

[PATCH linux-next v2 1/1] usb: gadget: udc: atmel: Update endpoint allocation scheme

[cristian.birsan]

From: Cristian Birsan <cristian.birsan@microchip.com>

Update atmel udc driver with a new enpoint allocation scheme. The data
sheet requires that all endpoints are allocated in order.

Signed-off-by: Cristian Birsan <cristian.birsan@microchip.com>
---
 drivers/usb/gadget/udc/Kconfig          |  14 ++
 drivers/usb/gadget/udc/atmel_usba_udc.c | 236 +++++++++++++++++++++++++++-----
 drivers/usb/gadget/udc/atmel_usba_udc.h |  10 +-
 3 files changed, 227 insertions(+), 33 deletions(-)

diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
index 658b8da..4b69f28 100644
--- a/drivers/usb/gadget/udc/Kconfig
+++ b/drivers/usb/gadget/udc/Kconfig
@@ -60,6 +60,20 @@ config USB_ATMEL_USBA
 	  USBA is the integrated high-speed USB Device controller on
 	  the AT32AP700x, some AT91SAM9 and AT91CAP9 processors from Atmel.
 
+	  The fifo_mode parameter is used to select endpoint allocation mode.
+	  fifo_mode = 0 is used to let the driver autoconfigure the endpoints.
+	  In this case 2 banks are allocated for isochronous endpoints and
+	  only one bank is allocated for the rest of the endpoints.
+
+	  fifo_mode = 1 is a generic maximum fifo size (1024 bytes) configuration
+	  allowing the usage of ep1 - ep6
+
+	  fifo_mode = 2 is a generic performance maximum fifo size (1024 bytes)
+	  configuration allowing the usage of ep1 - ep3
+
+	  fifo_mode = 3 is a balanced performance configuration allowing the
+	  the usage of ep1 - ep8
+
 config USB_BCM63XX_UDC
 	tristate "Broadcom BCM63xx Peripheral Controller"
 	depends on BCM63XX
diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
index 12c7687..11bbce2 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.c
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
@@ -20,6 +20,7 @@
 #include <linux/mfd/syscon.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
+#include <linux/ctype.h>
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
 #include <linux/usb/atmel_usba_udc.h>
@@ -318,6 +319,91 @@ static inline void usba_cleanup_debugfs(struct usba_udc *udc)
 }
 #endif
 
+static ushort fifo_mode;
+
+/* "modprobe ... fifo_mode=1" etc */
+module_param(fifo_mode, ushort, 0x0);
+MODULE_PARM_DESC(fifo_mode, "Endpoint configuration mode");
+
+/* mode 0 - uses autoconfig */
+
+/* mode 1 - fits in 8KB, generic max fifo configuration */
+static struct usba_fifo_cfg mode_1_cfg[] = {
+{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
+{ .hw_ep_num = 1, .fifo_size = 1024,	.nr_banks = 2, },
+{ .hw_ep_num = 2, .fifo_size = 1024,	.nr_banks = 1, },
+{ .hw_ep_num = 3, .fifo_size = 1024,	.nr_banks = 1, },
+{ .hw_ep_num = 4, .fifo_size = 1024,	.nr_banks = 1, },
+{ .hw_ep_num = 5, .fifo_size = 1024,	.nr_banks = 1, },
+{ .hw_ep_num = 6, .fifo_size = 1024,	.nr_banks = 1, },
+};
+
+/* mode 2 - fits in 8KB, performance max fifo configuration */
+static struct usba_fifo_cfg mode_2_cfg[] = {
+{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
+{ .hw_ep_num = 1, .fifo_size = 1024,	.nr_banks = 3, },
+{ .hw_ep_num = 2, .fifo_size = 1024,	.nr_banks = 2, },
+{ .hw_ep_num = 3, .fifo_size = 1024,	.nr_banks = 2, },
+};
+
+/* mode 3 - fits in 8KB, mixed fifo configuration */
+static struct usba_fifo_cfg mode_3_cfg[] = {
+{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
+{ .hw_ep_num = 1, .fifo_size = 1024,	.nr_banks = 2, },
+{ .hw_ep_num = 2, .fifo_size = 512,	.nr_banks = 2, },
+{ .hw_ep_num = 3, .fifo_size = 512,	.nr_banks = 2, },
+{ .hw_ep_num = 4, .fifo_size = 512,	.nr_banks = 2, },
+{ .hw_ep_num = 5, .fifo_size = 512,	.nr_banks = 2, },
+{ .hw_ep_num = 6, .fifo_size = 512,	.nr_banks = 2, },
+};
+
+/* mode 4 - fits in 8KB, custom fifo configuration */
+static struct usba_fifo_cfg mode_4_cfg[] = {
+{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
+{ .hw_ep_num = 1, .fifo_size = 512,	.nr_banks = 2, },
+{ .hw_ep_num = 2, .fifo_size = 512,	.nr_banks = 2, },
+{ .hw_ep_num = 3, .fifo_size = 8,	.nr_banks = 2, },
+{ .hw_ep_num = 4, .fifo_size = 512,	.nr_banks = 2, },
+{ .hw_ep_num = 5, .fifo_size = 512,	.nr_banks = 2, },
+{ .hw_ep_num = 6, .fifo_size = 16,	.nr_banks = 2, },
+{ .hw_ep_num = 7, .fifo_size = 8,	.nr_banks = 2, },
+{ .hw_ep_num = 8, .fifo_size = 8,	.nr_banks = 2, },
+};
+/* Add additional configurations here */
+
+int usba_config_fifo_table(struct usba_udc *udc)
+{
+	int n;
+
+	switch (fifo_mode) {
+	default:
+		fifo_mode = 0;
+	case 0:
+		udc->fifo_cfg = NULL;
+		n = 0;
+		break;
+	case 1:
+		udc->fifo_cfg = mode_1_cfg;
+		n = ARRAY_SIZE(mode_1_cfg);
+		break;
+	case 2:
+		udc->fifo_cfg = mode_2_cfg;
+		n = ARRAY_SIZE(mode_2_cfg);
+		break;
+	case 3:
+		udc->fifo_cfg = mode_3_cfg;
+		n = ARRAY_SIZE(mode_3_cfg);
+		break;
+	case 4:
+		udc->fifo_cfg = mode_4_cfg;
+		n = ARRAY_SIZE(mode_4_cfg);
+		break;
+	}
+	DBG(DBG_HW, "Setup fifo_mode %d\n", fifo_mode);
+
+	return n;
+}
+
 static inline u32 usba_int_enb_get(struct usba_udc *udc)
 {
 	return udc->int_enb_cache;
@@ -543,24 +629,17 @@ usba_ep_enable(struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
 	ep->is_isoc = 0;
 	ep->is_in = 0;
 
-	if (maxpacket <= 8)
-		ept_cfg = USBA_BF(EPT_SIZE, USBA_EPT_SIZE_8);
-	else
-		/* LSB is bit 1, not 0 */
-		ept_cfg = USBA_BF(EPT_SIZE, fls(maxpacket - 1) - 3);
-
-	DBG(DBG_HW, "%s: EPT_SIZE = %lu (maxpacket = %lu)\n",
+	DBG(DBG_ERR, "%s: EPT_CFG = 0x%lx (maxpacket = %lu)\n",
 			ep->ep.name, ept_cfg, maxpacket);
 
 	if (usb_endpoint_dir_in(desc)) {
 		ep->is_in = 1;
-		ept_cfg |= USBA_EPT_DIR_IN;
+		ep->ept_cfg |= USBA_EPT_DIR_IN;
 	}
 
 	switch (usb_endpoint_type(desc)) {
 	case USB_ENDPOINT_XFER_CONTROL:
-		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_CONTROL);
-		ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_ONE);
+		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_CONTROL);
 		break;
 	case USB_ENDPOINT_XFER_ISOC:
 		if (!ep->can_isoc) {
@@ -578,24 +657,15 @@ usba_ep_enable(struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
 			return -EINVAL;
 
 		ep->is_isoc = 1;
-		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_ISO);
+		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_ISO);
+		ep->ept_cfg |= USBA_BF(NB_TRANS, nr_trans);
 
-		/*
-		 * Do triple-buffering on high-bandwidth iso endpoints.
-		 */
-		if (nr_trans > 1 && ep->nr_banks == 3)
-			ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_TRIPLE);
-		else
-			ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_DOUBLE);
-		ept_cfg |= USBA_BF(NB_TRANS, nr_trans);
 		break;
 	case USB_ENDPOINT_XFER_BULK:
-		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_BULK);
-		ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_DOUBLE);
+		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_BULK);
 		break;
 	case USB_ENDPOINT_XFER_INT:
-		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_INT);
-		ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_DOUBLE);
+		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_INT);
 		break;
 	}
 
@@ -604,7 +674,7 @@ usba_ep_enable(struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
 	ep->ep.desc = desc;
 	ep->ep.maxpacket = maxpacket;
 
-	usba_ep_writel(ep, CFG, ept_cfg);
+	usba_ep_writel(ep, CFG, ep->ept_cfg);
 	usba_ep_writel(ep, CTL_ENB, USBA_EPT_ENABLE);
 
 	if (ep->can_dma) {
@@ -1006,12 +1076,81 @@ static int atmel_usba_start(struct usb_gadget *gadget,
 		struct usb_gadget_driver *driver);
 static int atmel_usba_stop(struct usb_gadget *gadget);
 
+static struct usb_ep *atmel_usba_match_ep(
+		struct usb_gadget		*gadget,
+		struct usb_endpoint_descriptor	*desc,
+		struct usb_ss_ep_comp_descriptor *ep_comp
+)
+{
+	struct usb_ep	*_ep;
+	struct usba_ep *ep;
+
+	/* Look at endpoints until an unclaimed one looks usable */
+	list_for_each_entry(_ep, &gadget->ep_list, ep_list) {
+		if (usb_gadget_ep_match_desc(gadget, _ep, desc, ep_comp))
+			goto found_ep;
+	}
+	/* Fail */
+	return NULL;
+
+found_ep:
+
+	if (fifo_mode == 0) {
+		/* Optimize hw fifo size based on ep type and other info */
+		ep = to_usba_ep(_ep);
+
+		switch (usb_endpoint_type(desc)) {
+
+		case USB_ENDPOINT_XFER_CONTROL:
+			break;
+
+		case USB_ENDPOINT_XFER_ISOC:
+			ep->fifo_size = 1024;
+			ep->nr_banks = 2;
+			break;
+
+		case USB_ENDPOINT_XFER_BULK:
+			ep->fifo_size = 512;
+			ep->nr_banks = 1;
+			break;
+
+		case USB_ENDPOINT_XFER_INT:
+			if (desc->wMaxPacketSize == 0)
+				ep->fifo_size =
+				    roundup_pow_of_two(_ep->maxpacket_limit);
+			else
+				ep->fifo_size =
+				    roundup_pow_of_two(le16_to_cpu(desc->wMaxPacketSize));
+			ep->nr_banks = 1;
+			break;
+		}
+
+		/* It might be a little bit late to set this */
+		usb_ep_set_maxpacket_limit(&ep->ep, ep->fifo_size);
+
+		/* Generate ept_cfg basd on FIFO size and number of banks */
+		if (ep->fifo_size  <= 8)
+			ep->ept_cfg = USBA_BF(EPT_SIZE, USBA_EPT_SIZE_8);
+		else
+			/* LSB is bit 1, not 0 */
+			ep->ept_cfg =
+				USBA_BF(EPT_SIZE, fls(ep->fifo_size - 1) - 3);
+
+		ep->ept_cfg |= USBA_BF(BK_NUMBER, ep->nr_banks);
+
+		ep->udc->configured_ep++;
+	}
+
+return _ep;
+}
+
 static const struct usb_gadget_ops usba_udc_ops = {
 	.get_frame		= usba_udc_get_frame,
 	.wakeup			= usba_udc_wakeup,
 	.set_selfpowered	= usba_udc_set_selfpowered,
 	.udc_start		= atmel_usba_start,
 	.udc_stop		= atmel_usba_stop,
+	.match_ep		= atmel_usba_match_ep,
 };
 
 static struct usb_endpoint_descriptor usba_ep0_desc = {
@@ -1678,7 +1817,8 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
 	}
 
 	if (status & USBA_END_OF_RESET) {
-		struct usba_ep *ep0;
+		struct usba_ep *ep0, *ep;
+		int i, n;
 
 		usba_writel(udc, INT_CLR, USBA_END_OF_RESET);
 		generate_bias_pulse(udc);
@@ -1717,6 +1857,16 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
 		if (!(usba_ep_readl(ep0, CFG) & USBA_EPT_MAPPED))
 			dev_dbg(&udc->pdev->dev,
 				 "ODD: EP0 configuration is invalid!\n");
+
+		/* Preallocate other endpoints */
+		n = fifo_mode ? udc->num_ep : udc->configured_ep;
+		for (i = 1; i < n; i++) {
+			ep = &udc->usba_ep[i];
+			usba_ep_writel(ep, CFG, ep->ept_cfg);
+			if (!(usba_ep_readl(ep, CFG) & USBA_EPT_MAPPED))
+				dev_dbg(&udc->pdev->dev,
+				 "ODD: EP%d configuration is invalid!\n", i);
+		}
 	}
 
 	spin_unlock(&udc->lock);
@@ -1864,6 +2014,9 @@ static int atmel_usba_stop(struct usb_gadget *gadget)
 	if (gpio_is_valid(udc->vbus_pin))
 		disable_irq(gpio_to_irq(udc->vbus_pin));
 
+	if (fifo_mode == 0)
+		udc->configured_ep = 1;
+
 	usba_stop(udc);
 
 	udc->driver = NULL;
@@ -1931,9 +2084,13 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
 						&flags);
 	udc->vbus_pin_inverted = (flags & OF_GPIO_ACTIVE_LOW) ? 1 : 0;
 
-	pp = NULL;
-	while ((pp = of_get_next_child(np, pp)))
-		udc->num_ep++;
+	if (fifo_mode == 0) {
+		pp = NULL;
+		while ((pp = of_get_next_child(np, pp)))
+			udc->num_ep++;
+		udc->configured_ep = 1;
+	} else
+		udc->num_ep = usba_config_fifo_table(udc);
 
 	eps = devm_kzalloc(&pdev->dev, sizeof(struct usba_ep) * udc->num_ep,
 			   GFP_KERNEL);
@@ -1946,7 +2103,7 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
 
 	pp = NULL;
 	i = 0;
-	while ((pp = of_get_next_child(np, pp))) {
+	while ((pp = of_get_next_child(np, pp)) && i < udc->num_ep) {
 		ep = &eps[i];
 
 		ret = of_property_read_u32(pp, "reg", &val);
@@ -1954,21 +2111,21 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
 			dev_err(&pdev->dev, "of_probe: reg error(%d)\n", ret);
 			goto err;
 		}
-		ep->index = val;
+		ep->index = fifo_mode ? udc->fifo_cfg[i].hw_ep_num : val;
 
 		ret = of_property_read_u32(pp, "atmel,fifo-size", &val);
 		if (ret) {
 			dev_err(&pdev->dev, "of_probe: fifo-size error(%d)\n", ret);
 			goto err;
 		}
-		ep->fifo_size = val;
+		ep->fifo_size = fifo_mode ? udc->fifo_cfg[i].fifo_size : val;
 
 		ret = of_property_read_u32(pp, "atmel,nb-banks", &val);
 		if (ret) {
 			dev_err(&pdev->dev, "of_probe: nb-banks error(%d)\n", ret);
 			goto err;
 		}
-		ep->nr_banks = val;
+		ep->nr_banks = fifo_mode ? udc->fifo_cfg[i].nr_banks : val;
 
 		ep->can_dma = of_property_read_bool(pp, "atmel,can-dma");
 		ep->can_isoc = of_property_read_bool(pp, "atmel,can-isoc");
@@ -2000,6 +2157,21 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
 		ep->ep.caps.dir_in = true;
 		ep->ep.caps.dir_out = true;
 
+		if (fifo_mode != 0) {
+			/*
+			 * Generate ept_cfg based on FIFO size and
+			 * banks number
+			 */
+			if (ep->fifo_size  <= 8)
+				ep->ept_cfg = USBA_BF(EPT_SIZE, USBA_EPT_SIZE_8);
+			else
+				/* LSB is bit 1, not 0 */
+				ep->ept_cfg =
+				  USBA_BF(EPT_SIZE, fls(ep->fifo_size - 1) - 3);
+
+			ep->ept_cfg |= USBA_BF(BK_NUMBER, ep->nr_banks);
+		}
+
 		if (i)
 			list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
 
diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.h b/drivers/usb/gadget/udc/atmel_usba_udc.h
index b03b2eb..9551b70 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.h
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.h
@@ -275,6 +275,12 @@ struct usba_dma_desc {
 	u32 ctrl;
 };
 
+struct usba_fifo_cfg {
+	u8			hw_ep_num;
+	u16			fifo_size;
+	u8			nr_banks;
+};
+
 struct usba_ep {
 	int					state;
 	void __iomem				*ep_regs;
@@ -293,7 +299,7 @@ struct usba_ep {
 	unsigned int				can_isoc:1;
 	unsigned int				is_isoc:1;
 	unsigned int				is_in:1;
-
+	unsigned long				ept_cfg;
 #ifdef CONFIG_USB_GADGET_DEBUG_FS
 	u32					last_dma_status;
 	struct dentry				*debugfs_dir;
@@ -338,6 +344,8 @@ struct usba_udc {
 	int vbus_pin;
 	int vbus_pin_inverted;
 	int num_ep;
+	int configured_ep;
+	struct usba_fifo_cfg *fifo_cfg;
 	struct clk *pclk;
 	struct clk *hclk;
 	struct usba_ep *usba_ep;
-- 
2.7.4

[PATCH linux-next v2 1/1] usb: gadget: udc: atmel: Update endpoint allocation scheme

[Nicolas Ferre]

Le 23/01/2017 ? 15:45, cristian.birsan at microchip.com a ?crit :
> From: Cristian Birsan <cristian.birsan@microchip.com>
> 
> Update atmel udc driver with a new enpoint allocation scheme. The data
> sheet requires that all endpoints are allocated in order.

Pieces of information from the cover letter could have been added here.

> Signed-off-by: Cristian Birsan <cristian.birsan@microchip.com>
> ---
>  drivers/usb/gadget/udc/Kconfig          |  14 ++
>  drivers/usb/gadget/udc/atmel_usba_udc.c | 236 +++++++++++++++++++++++++++-----
>  drivers/usb/gadget/udc/atmel_usba_udc.h |  10 +-
>  3 files changed, 227 insertions(+), 33 deletions(-)

It can be good to run ./scripts/checkpatch.pl --strict as well. It would
have told you about some of the alignment and braces issues. I ran it as
some the code has looked weird to me once or twice...


> diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
> index 658b8da..4b69f28 100644
> --- a/drivers/usb/gadget/udc/Kconfig
> +++ b/drivers/usb/gadget/udc/Kconfig
> @@ -60,6 +60,20 @@ config USB_ATMEL_USBA
>  	  USBA is the integrated high-speed USB Device controller on
>  	  the AT32AP700x, some AT91SAM9 and AT91CAP9 processors from Atmel.
>  
> +	  The fifo_mode parameter is used to select endpoint allocation mode.
> +	  fifo_mode = 0 is used to let the driver autoconfigure the endpoints.
> +	  In this case 2 banks are allocated for isochronous endpoints and

You mean that 2 banks can do isochronous xfers if needed, but they can
do other types as well, right? So maybe rephrase the sentence.

> +	  only one bank is allocated for the rest of the endpoints.
> +
> +	  fifo_mode = 1 is a generic maximum fifo size (1024 bytes) configuration
> +	  allowing the usage of ep1 - ep6
> +
> +	  fifo_mode = 2 is a generic performance maximum fifo size (1024 bytes)
> +	  configuration allowing the usage of ep1 - ep3
> +
> +	  fifo_mode = 3 is a balanced performance configuration allowing the
> +	  the usage of ep1 - ep8
> +
>  config USB_BCM63XX_UDC
>  	tristate "Broadcom BCM63xx Peripheral Controller"
>  	depends on BCM63XX
> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
> index 12c7687..11bbce2 100644
> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
> @@ -20,6 +20,7 @@
>  #include <linux/mfd/syscon.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
> +#include <linux/ctype.h>
>  #include <linux/usb/ch9.h>
>  #include <linux/usb/gadget.h>
>  #include <linux/usb/atmel_usba_udc.h>
> @@ -318,6 +319,91 @@ static inline void usba_cleanup_debugfs(struct usba_udc *udc)
>  }
>  #endif
>  
> +static ushort fifo_mode;
> +
> +/* "modprobe ... fifo_mode=1" etc */

No need for this comment.

> +module_param(fifo_mode, ushort, 0x0);
> +MODULE_PARM_DESC(fifo_mode, "Endpoint configuration mode");
> +
> +/* mode 0 - uses autoconfig */
> +
> +/* mode 1 - fits in 8KB, generic max fifo configuration */
> +static struct usba_fifo_cfg mode_1_cfg[] = {
> +{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
> +{ .hw_ep_num = 1, .fifo_size = 1024,	.nr_banks = 2, },
> +{ .hw_ep_num = 2, .fifo_size = 1024,	.nr_banks = 1, },
> +{ .hw_ep_num = 3, .fifo_size = 1024,	.nr_banks = 1, },
> +{ .hw_ep_num = 4, .fifo_size = 1024,	.nr_banks = 1, },
> +{ .hw_ep_num = 5, .fifo_size = 1024,	.nr_banks = 1, },
> +{ .hw_ep_num = 6, .fifo_size = 1024,	.nr_banks = 1, },
> +};
> +
> +/* mode 2 - fits in 8KB, performance max fifo configuration */
> +static struct usba_fifo_cfg mode_2_cfg[] = {
> +{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
> +{ .hw_ep_num = 1, .fifo_size = 1024,	.nr_banks = 3, },
> +{ .hw_ep_num = 2, .fifo_size = 1024,	.nr_banks = 2, },
> +{ .hw_ep_num = 3, .fifo_size = 1024,	.nr_banks = 2, },
> +};
> +
> +/* mode 3 - fits in 8KB, mixed fifo configuration */
> +static struct usba_fifo_cfg mode_3_cfg[] = {
> +{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
> +{ .hw_ep_num = 1, .fifo_size = 1024,	.nr_banks = 2, },
> +{ .hw_ep_num = 2, .fifo_size = 512,	.nr_banks = 2, },
> +{ .hw_ep_num = 3, .fifo_size = 512,	.nr_banks = 2, },
> +{ .hw_ep_num = 4, .fifo_size = 512,	.nr_banks = 2, },
> +{ .hw_ep_num = 5, .fifo_size = 512,	.nr_banks = 2, },
> +{ .hw_ep_num = 6, .fifo_size = 512,	.nr_banks = 2, },
> +};
> +
> +/* mode 4 - fits in 8KB, custom fifo configuration */
> +static struct usba_fifo_cfg mode_4_cfg[] = {
> +{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
> +{ .hw_ep_num = 1, .fifo_size = 512,	.nr_banks = 2, },
> +{ .hw_ep_num = 2, .fifo_size = 512,	.nr_banks = 2, },
> +{ .hw_ep_num = 3, .fifo_size = 8,	.nr_banks = 2, },
> +{ .hw_ep_num = 4, .fifo_size = 512,	.nr_banks = 2, },
> +{ .hw_ep_num = 5, .fifo_size = 512,	.nr_banks = 2, },
> +{ .hw_ep_num = 6, .fifo_size = 16,	.nr_banks = 2, },
> +{ .hw_ep_num = 7, .fifo_size = 8,	.nr_banks = 2, },
> +{ .hw_ep_num = 8, .fifo_size = 8,	.nr_banks = 2, },
> +};
> +/* Add additional configurations here */
> +
> +int usba_config_fifo_table(struct usba_udc *udc)

Isn't is static?

> +{
> +	int n;
> +
> +	switch (fifo_mode) {
> +	default:
> +		fifo_mode = 0;
> +	case 0:
> +		udc->fifo_cfg = NULL;
> +		n = 0;
> +		break;
> +	case 1:
> +		udc->fifo_cfg = mode_1_cfg;
> +		n = ARRAY_SIZE(mode_1_cfg);
> +		break;
> +	case 2:
> +		udc->fifo_cfg = mode_2_cfg;
> +		n = ARRAY_SIZE(mode_2_cfg);
> +		break;
> +	case 3:
> +		udc->fifo_cfg = mode_3_cfg;
> +		n = ARRAY_SIZE(mode_3_cfg);
> +		break;
> +	case 4:
> +		udc->fifo_cfg = mode_4_cfg;
> +		n = ARRAY_SIZE(mode_4_cfg);
> +		break;
> +	}
> +	DBG(DBG_HW, "Setup fifo_mode %d\n", fifo_mode);
> +
> +	return n;
> +}
> +
>  static inline u32 usba_int_enb_get(struct usba_udc *udc)
>  {
>  	return udc->int_enb_cache;
> @@ -543,24 +629,17 @@ usba_ep_enable(struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
>  	ep->is_isoc = 0;
>  	ep->is_in = 0;
>  
> -	if (maxpacket <= 8)
> -		ept_cfg = USBA_BF(EPT_SIZE, USBA_EPT_SIZE_8);
> -	else
> -		/* LSB is bit 1, not 0 */
> -		ept_cfg = USBA_BF(EPT_SIZE, fls(maxpacket - 1) - 3);
> -
> -	DBG(DBG_HW, "%s: EPT_SIZE = %lu (maxpacket = %lu)\n",
> +	DBG(DBG_ERR, "%s: EPT_CFG = 0x%lx (maxpacket = %lu)\n",
>  			ep->ep.name, ept_cfg, maxpacket);
>  
>  	if (usb_endpoint_dir_in(desc)) {
>  		ep->is_in = 1;
> -		ept_cfg |= USBA_EPT_DIR_IN;
> +		ep->ept_cfg |= USBA_EPT_DIR_IN;
>  	}
>  
>  	switch (usb_endpoint_type(desc)) {
>  	case USB_ENDPOINT_XFER_CONTROL:
> -		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_CONTROL);
> -		ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_ONE);
> +		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_CONTROL);
>  		break;
>  	case USB_ENDPOINT_XFER_ISOC:
>  		if (!ep->can_isoc) {
> @@ -578,24 +657,15 @@ usba_ep_enable(struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
>  			return -EINVAL;
>  
>  		ep->is_isoc = 1;
> -		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_ISO);
> +		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_ISO);
> +		ep->ept_cfg |= USBA_BF(NB_TRANS, nr_trans);
>  
> -		/*
> -		 * Do triple-buffering on high-bandwidth iso endpoints.
> -		 */
> -		if (nr_trans > 1 && ep->nr_banks == 3)
> -			ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_TRIPLE);
> -		else
> -			ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_DOUBLE);
> -		ept_cfg |= USBA_BF(NB_TRANS, nr_trans);
>  		break;
>  	case USB_ENDPOINT_XFER_BULK:
> -		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_BULK);
> -		ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_DOUBLE);
> +		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_BULK);
>  		break;
>  	case USB_ENDPOINT_XFER_INT:
> -		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_INT);
> -		ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_DOUBLE);
> +		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_INT);
>  		break;
>  	}
>  
> @@ -604,7 +674,7 @@ usba_ep_enable(struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
>  	ep->ep.desc = desc;
>  	ep->ep.maxpacket = maxpacket;
>  
> -	usba_ep_writel(ep, CFG, ept_cfg);
> +	usba_ep_writel(ep, CFG, ep->ept_cfg);
>  	usba_ep_writel(ep, CTL_ENB, USBA_EPT_ENABLE);
>  
>  	if (ep->can_dma) {
> @@ -1006,12 +1076,81 @@ static int atmel_usba_start(struct usb_gadget *gadget,
>  		struct usb_gadget_driver *driver);
>  static int atmel_usba_stop(struct usb_gadget *gadget);
>  
> +static struct usb_ep *atmel_usba_match_ep(

I find the indentation of this funciton wreird...

> +		struct usb_gadget		*gadget,

No need for a tab here and align with open parenthesis,

> +		struct usb_endpoint_descriptor	*desc,
> +		struct usb_ss_ep_comp_descriptor *ep_comp
> +)

This parenthesis seems weird to me: put it after the last parameter.

> +{
> +	struct usb_ep	*_ep;
> +	struct usba_ep *ep;
> +
> +	/* Look at endpoints until an unclaimed one looks usable */
> +	list_for_each_entry(_ep, &gadget->ep_list, ep_list) {
> +		if (usb_gadget_ep_match_desc(gadget, _ep, desc, ep_comp))
> +			goto found_ep;
> +	}
> +	/* Fail */
> +	return NULL;
> +
> +found_ep:
> +
> +	if (fifo_mode == 0) {
> +		/* Optimize hw fifo size based on ep type and other info */
> +		ep = to_usba_ep(_ep);
> +
> +		switch (usb_endpoint_type(desc)) {
> +
> +		case USB_ENDPOINT_XFER_CONTROL:
> +			break;
> +
> +		case USB_ENDPOINT_XFER_ISOC:
> +			ep->fifo_size = 1024;
> +			ep->nr_banks = 2;
> +			break;
> +
> +		case USB_ENDPOINT_XFER_BULK:
> +			ep->fifo_size = 512;
> +			ep->nr_banks = 1;
> +			break;
> +
> +		case USB_ENDPOINT_XFER_INT:
> +			if (desc->wMaxPacketSize == 0)
> +				ep->fifo_size =
> +				    roundup_pow_of_two(_ep->maxpacket_limit);
> +			else
> +				ep->fifo_size =
> +				    roundup_pow_of_two(le16_to_cpu(desc->wMaxPacketSize));
> +			ep->nr_banks = 1;
> +			break;
> +		}
> +
> +		/* It might be a little bit late to set this */

Is it too late or not? This comment is frightening... We may need some
feedback from the USB guys here...

> +		usb_ep_set_maxpacket_limit(&ep->ep, ep->fifo_size);
> +
> +		/* Generate ept_cfg basd on FIFO size and number of banks */
> +		if (ep->fifo_size  <= 8)
> +			ep->ept_cfg = USBA_BF(EPT_SIZE, USBA_EPT_SIZE_8);
> +		else
> +			/* LSB is bit 1, not 0 */
> +			ep->ept_cfg =
> +				USBA_BF(EPT_SIZE, fls(ep->fifo_size - 1) - 3);
> +
> +		ep->ept_cfg |= USBA_BF(BK_NUMBER, ep->nr_banks);
> +		ep->udc->configured_ep++;

is it more than a boolean value?

> +	}
> +
> +return _ep;

Indentation issue here ^

> +}
> +
>  static const struct usb_gadget_ops usba_udc_ops = {
>  	.get_frame		= usba_udc_get_frame,
>  	.wakeup			= usba_udc_wakeup,
>  	.set_selfpowered	= usba_udc_set_selfpowered,
>  	.udc_start		= atmel_usba_start,
>  	.udc_stop		= atmel_usba_stop,
> +	.match_ep		= atmel_usba_match_ep,
>  };
>  
>  static struct usb_endpoint_descriptor usba_ep0_desc = {
> @@ -1678,7 +1817,8 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
>  	}
>  
>  	if (status & USBA_END_OF_RESET) {
> -		struct usba_ep *ep0;
> +		struct usba_ep *ep0, *ep;
> +		int i, n;
>  
>  		usba_writel(udc, INT_CLR, USBA_END_OF_RESET);
>  		generate_bias_pulse(udc);
> @@ -1717,6 +1857,16 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
>  		if (!(usba_ep_readl(ep0, CFG) & USBA_EPT_MAPPED))
>  			dev_dbg(&udc->pdev->dev,
>  				 "ODD: EP0 configuration is invalid!\n");
> +
> +		/* Preallocate other endpoints */
> +		n = fifo_mode ? udc->num_ep : udc->configured_ep;
> +		for (i = 1; i < n; i++) {
> +			ep = &udc->usba_ep[i];
> +			usba_ep_writel(ep, CFG, ep->ept_cfg);
> +			if (!(usba_ep_readl(ep, CFG) & USBA_EPT_MAPPED))
> +				dev_dbg(&udc->pdev->dev,
> +				 "ODD: EP%d configuration is invalid!\n", i);

I know that it's the case for the EP0 just above and that we do it like
this in the current driver but maybe we can do more than just having a
debug message.

> +		}
>  	}
>  
>  	spin_unlock(&udc->lock);
> @@ -1864,6 +2014,9 @@ static int atmel_usba_stop(struct usb_gadget *gadget)
>  	if (gpio_is_valid(udc->vbus_pin))
>  		disable_irq(gpio_to_irq(udc->vbus_pin));
>  
> +	if (fifo_mode == 0)
> +		udc->configured_ep = 1;
> +
>  	usba_stop(udc);
>  
>  	udc->driver = NULL;
> @@ -1931,9 +2084,13 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
>  						&flags);
>  	udc->vbus_pin_inverted = (flags & OF_GPIO_ACTIVE_LOW) ? 1 : 0;
>  
> -	pp = NULL;
> -	while ((pp = of_get_next_child(np, pp)))
> -		udc->num_ep++;
> +	if (fifo_mode == 0) {
> +		pp = NULL;
> +		while ((pp = of_get_next_child(np, pp)))
> +			udc->num_ep++;
> +		udc->configured_ep = 1;
> +	} else

Braces here

> +		udc->num_ep = usba_config_fifo_table(udc);
>  
>  	eps = devm_kzalloc(&pdev->dev, sizeof(struct usba_ep) * udc->num_ep,
>  			   GFP_KERNEL);
> @@ -1946,7 +2103,7 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
>  
>  	pp = NULL;
>  	i = 0;
> -	while ((pp = of_get_next_child(np, pp))) {
> +	while ((pp = of_get_next_child(np, pp)) && i < udc->num_ep) {
>  		ep = &eps[i];
>  
>  		ret = of_property_read_u32(pp, "reg", &val);
> @@ -1954,21 +2111,21 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
>  			dev_err(&pdev->dev, "of_probe: reg error(%d)\n", ret);
>  			goto err;
>  		}
> -		ep->index = val;
> +		ep->index = fifo_mode ? udc->fifo_cfg[i].hw_ep_num : val;
>  
>  		ret = of_property_read_u32(pp, "atmel,fifo-size", &val);
>  		if (ret) {
>  			dev_err(&pdev->dev, "of_probe: fifo-size error(%d)\n", ret);
>  			goto err;
>  		}
> -		ep->fifo_size = val;
> +		ep->fifo_size = fifo_mode ? udc->fifo_cfg[i].fifo_size : val;

No, this is where I would like to see that the value from the table is
still validated agains the "max value" that is stored in HW description/DT.
I know that all of our product are able to handle 1024 fifo size, but
it's more a savfe check...

>  
>  		ret = of_property_read_u32(pp, "atmel,nb-banks", &val);
>  		if (ret) {
>  			dev_err(&pdev->dev, "of_probe: nb-banks error(%d)\n", ret);
>  			goto err;
>  		}
> -		ep->nr_banks = val;
> +		ep->nr_banks = fifo_mode ? udc->fifo_cfg[i].nr_banks : val;

Here however, it is pretty important to check this value. For instance,
notice that the at91sam9x5 cannot fullfil the "mode 2" configuration: we
can't silentely ignore the HW specifications given by DT.

>  		ep->can_dma = of_property_read_bool(pp, "atmel,can-dma");
>  		ep->can_isoc = of_property_read_bool(pp, "atmel,can-isoc");
> @@ -2000,6 +2157,21 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
>  		ep->ep.caps.dir_in = true;
>  		ep->ep.caps.dir_out = true;
>  
> +		if (fifo_mode != 0) {
> +			/*
> +			 * Generate ept_cfg based on FIFO size and
> +			 * banks number
> +			 */
> +			if (ep->fifo_size  <= 8)
> +				ep->ept_cfg = USBA_BF(EPT_SIZE, USBA_EPT_SIZE_8);
> +			else
> +				/* LSB is bit 1, not 0 */
> +				ep->ept_cfg =
> +				  USBA_BF(EPT_SIZE, fls(ep->fifo_size - 1) - 3);
> +
> +			ep->ept_cfg |= USBA_BF(BK_NUMBER, ep->nr_banks);
> +		}
> +
>  		if (i)
>  			list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
>  
> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.h b/drivers/usb/gadget/udc/atmel_usba_udc.h
> index b03b2eb..9551b70 100644
> --- a/drivers/usb/gadget/udc/atmel_usba_udc.h
> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.h
> @@ -275,6 +275,12 @@ struct usba_dma_desc {
>  	u32 ctrl;
>  };
>  
> +struct usba_fifo_cfg {
> +	u8			hw_ep_num;
> +	u16			fifo_size;
> +	u8			nr_banks;
> +};
> +
>  struct usba_ep {
>  	int					state;
>  	void __iomem				*ep_regs;
> @@ -293,7 +299,7 @@ struct usba_ep {
>  	unsigned int				can_isoc:1;
>  	unsigned int				is_isoc:1;
>  	unsigned int				is_in:1;
> -
> +	unsigned long				ept_cfg;
>  #ifdef CONFIG_USB_GADGET_DEBUG_FS
>  	u32					last_dma_status;
>  	struct dentry				*debugfs_dir;
> @@ -338,6 +344,8 @@ struct usba_udc {
>  	int vbus_pin;
>  	int vbus_pin_inverted;
>  	int num_ep;
> +	int configured_ep;
> +	struct usba_fifo_cfg *fifo_cfg;
>  	struct clk *pclk;
>  	struct clk *hclk;
>  	struct usba_ep *usba_ep;
> 

Regards,
-- 
Nicolas Ferre

Re: [PATCH linux-next v2 1/1] usb: gadget: udc: atmel: Update endpoint allocation scheme

[Nicolas Ferre]

Le 23/01/2017 à 15:45, cristian.birsan@microchip.com a écrit :
> From: Cristian Birsan <cristian.birsan@microchip.com>
> 
> Update atmel udc driver with a new enpoint allocation scheme. The data
> sheet requires that all endpoints are allocated in order.

Pieces of information from the cover letter could have been added here.

> Signed-off-by: Cristian Birsan <cristian.birsan@microchip.com>
> ---
>  drivers/usb/gadget/udc/Kconfig          |  14 ++
>  drivers/usb/gadget/udc/atmel_usba_udc.c | 236 +++++++++++++++++++++++++++-----
>  drivers/usb/gadget/udc/atmel_usba_udc.h |  10 +-
>  3 files changed, 227 insertions(+), 33 deletions(-)

It can be good to run ./scripts/checkpatch.pl --strict as well. It would
have told you about some of the alignment and braces issues. I ran it as
some the code has looked weird to me once or twice...


> diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
> index 658b8da..4b69f28 100644
> --- a/drivers/usb/gadget/udc/Kconfig
> +++ b/drivers/usb/gadget/udc/Kconfig
> @@ -60,6 +60,20 @@ config USB_ATMEL_USBA
>  	  USBA is the integrated high-speed USB Device controller on
>  	  the AT32AP700x, some AT91SAM9 and AT91CAP9 processors from Atmel.
>  
> +	  The fifo_mode parameter is used to select endpoint allocation mode.
> +	  fifo_mode = 0 is used to let the driver autoconfigure the endpoints.
> +	  In this case 2 banks are allocated for isochronous endpoints and

You mean that 2 banks can do isochronous xfers if needed, but they can
do other types as well, right? So maybe rephrase the sentence.

> +	  only one bank is allocated for the rest of the endpoints.
> +
> +	  fifo_mode = 1 is a generic maximum fifo size (1024 bytes) configuration
> +	  allowing the usage of ep1 - ep6
> +
> +	  fifo_mode = 2 is a generic performance maximum fifo size (1024 bytes)
> +	  configuration allowing the usage of ep1 - ep3
> +
> +	  fifo_mode = 3 is a balanced performance configuration allowing the
> +	  the usage of ep1 - ep8
> +
>  config USB_BCM63XX_UDC
>  	tristate "Broadcom BCM63xx Peripheral Controller"
>  	depends on BCM63XX
> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
> index 12c7687..11bbce2 100644
> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
> @@ -20,6 +20,7 @@
>  #include <linux/mfd/syscon.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
> +#include <linux/ctype.h>
>  #include <linux/usb/ch9.h>
>  #include <linux/usb/gadget.h>
>  #include <linux/usb/atmel_usba_udc.h>
> @@ -318,6 +319,91 @@ static inline void usba_cleanup_debugfs(struct usba_udc *udc)
>  }
>  #endif
>  
> +static ushort fifo_mode;
> +
> +/* "modprobe ... fifo_mode=1" etc */

No need for this comment.

> +module_param(fifo_mode, ushort, 0x0);
> +MODULE_PARM_DESC(fifo_mode, "Endpoint configuration mode");
> +
> +/* mode 0 - uses autoconfig */
> +
> +/* mode 1 - fits in 8KB, generic max fifo configuration */
> +static struct usba_fifo_cfg mode_1_cfg[] = {
> +{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
> +{ .hw_ep_num = 1, .fifo_size = 1024,	.nr_banks = 2, },
> +{ .hw_ep_num = 2, .fifo_size = 1024,	.nr_banks = 1, },
> +{ .hw_ep_num = 3, .fifo_size = 1024,	.nr_banks = 1, },
> +{ .hw_ep_num = 4, .fifo_size = 1024,	.nr_banks = 1, },
> +{ .hw_ep_num = 5, .fifo_size = 1024,	.nr_banks = 1, },
> +{ .hw_ep_num = 6, .fifo_size = 1024,	.nr_banks = 1, },
> +};
> +
> +/* mode 2 - fits in 8KB, performance max fifo configuration */
> +static struct usba_fifo_cfg mode_2_cfg[] = {
> +{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
> +{ .hw_ep_num = 1, .fifo_size = 1024,	.nr_banks = 3, },
> +{ .hw_ep_num = 2, .fifo_size = 1024,	.nr_banks = 2, },
> +{ .hw_ep_num = 3, .fifo_size = 1024,	.nr_banks = 2, },
> +};
> +
> +/* mode 3 - fits in 8KB, mixed fifo configuration */
> +static struct usba_fifo_cfg mode_3_cfg[] = {
> +{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
> +{ .hw_ep_num = 1, .fifo_size = 1024,	.nr_banks = 2, },
> +{ .hw_ep_num = 2, .fifo_size = 512,	.nr_banks = 2, },
> +{ .hw_ep_num = 3, .fifo_size = 512,	.nr_banks = 2, },
> +{ .hw_ep_num = 4, .fifo_size = 512,	.nr_banks = 2, },
> +{ .hw_ep_num = 5, .fifo_size = 512,	.nr_banks = 2, },
> +{ .hw_ep_num = 6, .fifo_size = 512,	.nr_banks = 2, },
> +};
> +
> +/* mode 4 - fits in 8KB, custom fifo configuration */
> +static struct usba_fifo_cfg mode_4_cfg[] = {
> +{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
> +{ .hw_ep_num = 1, .fifo_size = 512,	.nr_banks = 2, },
> +{ .hw_ep_num = 2, .fifo_size = 512,	.nr_banks = 2, },
> +{ .hw_ep_num = 3, .fifo_size = 8,	.nr_banks = 2, },
> +{ .hw_ep_num = 4, .fifo_size = 512,	.nr_banks = 2, },
> +{ .hw_ep_num = 5, .fifo_size = 512,	.nr_banks = 2, },
> +{ .hw_ep_num = 6, .fifo_size = 16,	.nr_banks = 2, },
> +{ .hw_ep_num = 7, .fifo_size = 8,	.nr_banks = 2, },
> +{ .hw_ep_num = 8, .fifo_size = 8,	.nr_banks = 2, },
> +};
> +/* Add additional configurations here */
> +
> +int usba_config_fifo_table(struct usba_udc *udc)

Isn't is static?

> +{
> +	int n;
> +
> +	switch (fifo_mode) {
> +	default:
> +		fifo_mode = 0;
> +	case 0:
> +		udc->fifo_cfg = NULL;
> +		n = 0;
> +		break;
> +	case 1:
> +		udc->fifo_cfg = mode_1_cfg;
> +		n = ARRAY_SIZE(mode_1_cfg);
> +		break;
> +	case 2:
> +		udc->fifo_cfg = mode_2_cfg;
> +		n = ARRAY_SIZE(mode_2_cfg);
> +		break;
> +	case 3:
> +		udc->fifo_cfg = mode_3_cfg;
> +		n = ARRAY_SIZE(mode_3_cfg);
> +		break;
> +	case 4:
> +		udc->fifo_cfg = mode_4_cfg;
> +		n = ARRAY_SIZE(mode_4_cfg);
> +		break;
> +	}
> +	DBG(DBG_HW, "Setup fifo_mode %d\n", fifo_mode);
> +
> +	return n;
> +}
> +
>  static inline u32 usba_int_enb_get(struct usba_udc *udc)
>  {
>  	return udc->int_enb_cache;
> @@ -543,24 +629,17 @@ usba_ep_enable(struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
>  	ep->is_isoc = 0;
>  	ep->is_in = 0;
>  
> -	if (maxpacket <= 8)
> -		ept_cfg = USBA_BF(EPT_SIZE, USBA_EPT_SIZE_8);
> -	else
> -		/* LSB is bit 1, not 0 */
> -		ept_cfg = USBA_BF(EPT_SIZE, fls(maxpacket - 1) - 3);
> -
> -	DBG(DBG_HW, "%s: EPT_SIZE = %lu (maxpacket = %lu)\n",
> +	DBG(DBG_ERR, "%s: EPT_CFG = 0x%lx (maxpacket = %lu)\n",
>  			ep->ep.name, ept_cfg, maxpacket);
>  
>  	if (usb_endpoint_dir_in(desc)) {
>  		ep->is_in = 1;
> -		ept_cfg |= USBA_EPT_DIR_IN;
> +		ep->ept_cfg |= USBA_EPT_DIR_IN;
>  	}
>  
>  	switch (usb_endpoint_type(desc)) {
>  	case USB_ENDPOINT_XFER_CONTROL:
> -		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_CONTROL);
> -		ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_ONE);
> +		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_CONTROL);
>  		break;
>  	case USB_ENDPOINT_XFER_ISOC:
>  		if (!ep->can_isoc) {
> @@ -578,24 +657,15 @@ usba_ep_enable(struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
>  			return -EINVAL;
>  
>  		ep->is_isoc = 1;
> -		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_ISO);
> +		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_ISO);
> +		ep->ept_cfg |= USBA_BF(NB_TRANS, nr_trans);
>  
> -		/*
> -		 * Do triple-buffering on high-bandwidth iso endpoints.
> -		 */
> -		if (nr_trans > 1 && ep->nr_banks == 3)
> -			ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_TRIPLE);
> -		else
> -			ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_DOUBLE);
> -		ept_cfg |= USBA_BF(NB_TRANS, nr_trans);
>  		break;
>  	case USB_ENDPOINT_XFER_BULK:
> -		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_BULK);
> -		ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_DOUBLE);
> +		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_BULK);
>  		break;
>  	case USB_ENDPOINT_XFER_INT:
> -		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_INT);
> -		ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_DOUBLE);
> +		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_INT);
>  		break;
>  	}
>  
> @@ -604,7 +674,7 @@ usba_ep_enable(struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
>  	ep->ep.desc = desc;
>  	ep->ep.maxpacket = maxpacket;
>  
> -	usba_ep_writel(ep, CFG, ept_cfg);
> +	usba_ep_writel(ep, CFG, ep->ept_cfg);
>  	usba_ep_writel(ep, CTL_ENB, USBA_EPT_ENABLE);
>  
>  	if (ep->can_dma) {
> @@ -1006,12 +1076,81 @@ static int atmel_usba_start(struct usb_gadget *gadget,
>  		struct usb_gadget_driver *driver);
>  static int atmel_usba_stop(struct usb_gadget *gadget);
>  
> +static struct usb_ep *atmel_usba_match_ep(

I find the indentation of this funciton wreird...

> +		struct usb_gadget		*gadget,

No need for a tab here and align with open parenthesis,

> +		struct usb_endpoint_descriptor	*desc,
> +		struct usb_ss_ep_comp_descriptor *ep_comp
> +)

This parenthesis seems weird to me: put it after the last parameter.

> +{
> +	struct usb_ep	*_ep;
> +	struct usba_ep *ep;
> +
> +	/* Look at endpoints until an unclaimed one looks usable */
> +	list_for_each_entry(_ep, &gadget->ep_list, ep_list) {
> +		if (usb_gadget_ep_match_desc(gadget, _ep, desc, ep_comp))
> +			goto found_ep;
> +	}
> +	/* Fail */
> +	return NULL;
> +
> +found_ep:
> +
> +	if (fifo_mode == 0) {
> +		/* Optimize hw fifo size based on ep type and other info */
> +		ep = to_usba_ep(_ep);
> +
> +		switch (usb_endpoint_type(desc)) {
> +
> +		case USB_ENDPOINT_XFER_CONTROL:
> +			break;
> +
> +		case USB_ENDPOINT_XFER_ISOC:
> +			ep->fifo_size = 1024;
> +			ep->nr_banks = 2;
> +			break;
> +
> +		case USB_ENDPOINT_XFER_BULK:
> +			ep->fifo_size = 512;
> +			ep->nr_banks = 1;
> +			break;
> +
> +		case USB_ENDPOINT_XFER_INT:
> +			if (desc->wMaxPacketSize == 0)
> +				ep->fifo_size =
> +				    roundup_pow_of_two(_ep->maxpacket_limit);
> +			else
> +				ep->fifo_size =
> +				    roundup_pow_of_two(le16_to_cpu(desc->wMaxPacketSize));
> +			ep->nr_banks = 1;
> +			break;
> +		}
> +
> +		/* It might be a little bit late to set this */

Is it too late or not? This comment is frightening... We may need some
feedback from the USB guys here...

> +		usb_ep_set_maxpacket_limit(&ep->ep, ep->fifo_size);
> +
> +		/* Generate ept_cfg basd on FIFO size and number of banks */
> +		if (ep->fifo_size  <= 8)
> +			ep->ept_cfg = USBA_BF(EPT_SIZE, USBA_EPT_SIZE_8);
> +		else
> +			/* LSB is bit 1, not 0 */
> +			ep->ept_cfg =
> +				USBA_BF(EPT_SIZE, fls(ep->fifo_size - 1) - 3);
> +
> +		ep->ept_cfg |= USBA_BF(BK_NUMBER, ep->nr_banks);
> +		ep->udc->configured_ep++;

is it more than a boolean value?

> +	}
> +
> +return _ep;

Indentation issue here ^

> +}
> +
>  static const struct usb_gadget_ops usba_udc_ops = {
>  	.get_frame		= usba_udc_get_frame,
>  	.wakeup			= usba_udc_wakeup,
>  	.set_selfpowered	= usba_udc_set_selfpowered,
>  	.udc_start		= atmel_usba_start,
>  	.udc_stop		= atmel_usba_stop,
> +	.match_ep		= atmel_usba_match_ep,
>  };
>  
>  static struct usb_endpoint_descriptor usba_ep0_desc = {
> @@ -1678,7 +1817,8 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
>  	}
>  
>  	if (status & USBA_END_OF_RESET) {
> -		struct usba_ep *ep0;
> +		struct usba_ep *ep0, *ep;
> +		int i, n;
>  
>  		usba_writel(udc, INT_CLR, USBA_END_OF_RESET);
>  		generate_bias_pulse(udc);
> @@ -1717,6 +1857,16 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
>  		if (!(usba_ep_readl(ep0, CFG) & USBA_EPT_MAPPED))
>  			dev_dbg(&udc->pdev->dev,
>  				 "ODD: EP0 configuration is invalid!\n");
> +
> +		/* Preallocate other endpoints */
> +		n = fifo_mode ? udc->num_ep : udc->configured_ep;
> +		for (i = 1; i < n; i++) {
> +			ep = &udc->usba_ep[i];
> +			usba_ep_writel(ep, CFG, ep->ept_cfg);
> +			if (!(usba_ep_readl(ep, CFG) & USBA_EPT_MAPPED))
> +				dev_dbg(&udc->pdev->dev,
> +				 "ODD: EP%d configuration is invalid!\n", i);

I know that it's the case for the EP0 just above and that we do it like
this in the current driver but maybe we can do more than just having a
debug message.

> +		}
>  	}
>  
>  	spin_unlock(&udc->lock);
> @@ -1864,6 +2014,9 @@ static int atmel_usba_stop(struct usb_gadget *gadget)
>  	if (gpio_is_valid(udc->vbus_pin))
>  		disable_irq(gpio_to_irq(udc->vbus_pin));
>  
> +	if (fifo_mode == 0)
> +		udc->configured_ep = 1;
> +
>  	usba_stop(udc);
>  
>  	udc->driver = NULL;
> @@ -1931,9 +2084,13 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
>  						&flags);
>  	udc->vbus_pin_inverted = (flags & OF_GPIO_ACTIVE_LOW) ? 1 : 0;
>  
> -	pp = NULL;
> -	while ((pp = of_get_next_child(np, pp)))
> -		udc->num_ep++;
> +	if (fifo_mode == 0) {
> +		pp = NULL;
> +		while ((pp = of_get_next_child(np, pp)))
> +			udc->num_ep++;
> +		udc->configured_ep = 1;
> +	} else

Braces here

> +		udc->num_ep = usba_config_fifo_table(udc);
>  
>  	eps = devm_kzalloc(&pdev->dev, sizeof(struct usba_ep) * udc->num_ep,
>  			   GFP_KERNEL);
> @@ -1946,7 +2103,7 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
>  
>  	pp = NULL;
>  	i = 0;
> -	while ((pp = of_get_next_child(np, pp))) {
> +	while ((pp = of_get_next_child(np, pp)) && i < udc->num_ep) {
>  		ep = &eps[i];
>  
>  		ret = of_property_read_u32(pp, "reg", &val);
> @@ -1954,21 +2111,21 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
>  			dev_err(&pdev->dev, "of_probe: reg error(%d)\n", ret);
>  			goto err;
>  		}
> -		ep->index = val;
> +		ep->index = fifo_mode ? udc->fifo_cfg[i].hw_ep_num : val;
>  
>  		ret = of_property_read_u32(pp, "atmel,fifo-size", &val);
>  		if (ret) {
>  			dev_err(&pdev->dev, "of_probe: fifo-size error(%d)\n", ret);
>  			goto err;
>  		}
> -		ep->fifo_size = val;
> +		ep->fifo_size = fifo_mode ? udc->fifo_cfg[i].fifo_size : val;

No, this is where I would like to see that the value from the table is
still validated agains the "max value" that is stored in HW description/DT.
I know that all of our product are able to handle 1024 fifo size, but
it's more a savfe check...

>  
>  		ret = of_property_read_u32(pp, "atmel,nb-banks", &val);
>  		if (ret) {
>  			dev_err(&pdev->dev, "of_probe: nb-banks error(%d)\n", ret);
>  			goto err;
>  		}
> -		ep->nr_banks = val;
> +		ep->nr_banks = fifo_mode ? udc->fifo_cfg[i].nr_banks : val;

Here however, it is pretty important to check this value. For instance,
notice that the at91sam9x5 cannot fullfil the "mode 2" configuration: we
can't silentely ignore the HW specifications given by DT.

>  		ep->can_dma = of_property_read_bool(pp, "atmel,can-dma");
>  		ep->can_isoc = of_property_read_bool(pp, "atmel,can-isoc");
> @@ -2000,6 +2157,21 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
>  		ep->ep.caps.dir_in = true;
>  		ep->ep.caps.dir_out = true;
>  
> +		if (fifo_mode != 0) {
> +			/*
> +			 * Generate ept_cfg based on FIFO size and
> +			 * banks number
> +			 */
> +			if (ep->fifo_size  <= 8)
> +				ep->ept_cfg = USBA_BF(EPT_SIZE, USBA_EPT_SIZE_8);
> +			else
> +				/* LSB is bit 1, not 0 */
> +				ep->ept_cfg =
> +				  USBA_BF(EPT_SIZE, fls(ep->fifo_size - 1) - 3);
> +
> +			ep->ept_cfg |= USBA_BF(BK_NUMBER, ep->nr_banks);
> +		}
> +
>  		if (i)
>  			list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
>  
> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.h b/drivers/usb/gadget/udc/atmel_usba_udc.h
> index b03b2eb..9551b70 100644
> --- a/drivers/usb/gadget/udc/atmel_usba_udc.h
> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.h
> @@ -275,6 +275,12 @@ struct usba_dma_desc {
>  	u32 ctrl;
>  };
>  
> +struct usba_fifo_cfg {
> +	u8			hw_ep_num;
> +	u16			fifo_size;
> +	u8			nr_banks;
> +};
> +
>  struct usba_ep {
>  	int					state;
>  	void __iomem				*ep_regs;
> @@ -293,7 +299,7 @@ struct usba_ep {
>  	unsigned int				can_isoc:1;
>  	unsigned int				is_isoc:1;
>  	unsigned int				is_in:1;
> -
> +	unsigned long				ept_cfg;
>  #ifdef CONFIG_USB_GADGET_DEBUG_FS
>  	u32					last_dma_status;
>  	struct dentry				*debugfs_dir;
> @@ -338,6 +344,8 @@ struct usba_udc {
>  	int vbus_pin;
>  	int vbus_pin_inverted;
>  	int num_ep;
> +	int configured_ep;
> +	struct usba_fifo_cfg *fifo_cfg;
>  	struct clk *pclk;
>  	struct clk *hclk;
>  	struct usba_ep *usba_ep;
> 

Regards,
-- 
Nicolas Ferre

[PATCH linux-next v2 1/1] usb: gadget: udc: atmel: Update endpoint allocation scheme

[Felipe Balbi]

Nicolas Ferre <nicolas.ferre@atmel.com> writes:

> Le 23/01/2017 ? 15:45, cristian.birsan at microchip.com a ?crit :
>> From: Cristian Birsan <cristian.birsan@microchip.com>
>> 
>> Update atmel udc driver with a new enpoint allocation scheme. The data
>> sheet requires that all endpoints are allocated in order.
>
> Pieces of information from the cover letter could have been added here.

yup, that's what I did:

commit 741d2558bf0aa8da9c0834ad43e1b9a1b16aa515
Author: Cristian Birsan <cristian.birsan@microchip.com>
Date:   Mon Jan 23 16:45:59 2017 +0200

    usb: gadget: udc: atmel: Update endpoint allocation scheme
    
    This patch updates the usb endpoint allocation scheme for atmel usba
    driver to make sure all endpoints are allocated in order. This
    requirement comes from the datasheet of the controller.
    
    The allocation scheme is decided by fifo_mode parameter. For fifo_mode =
    0 the driver tries to autoconfigure the endpoints fifo size. All other
    modes contain fixed configurations optimized for different purposes. The
    idea is somehow similar with the approach used on musb driver.
    
    Signed-off-by: Cristian Birsan <cristian.birsan@microchip.com>
    Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>

But seems like we will need a fixup patch during v4.11-rc?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170127/2e9ab65b/attachment.sig>

Re: [PATCH linux-next v2 1/1] usb: gadget: udc: atmel: Update endpoint allocation scheme

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1367 bytes --]

Nicolas Ferre <nicolas.ferre@atmel.com> writes:

> Le 23/01/2017 à 15:45, cristian.birsan@microchip.com a écrit :
>> From: Cristian Birsan <cristian.birsan@microchip.com>
>> 
>> Update atmel udc driver with a new enpoint allocation scheme. The data
>> sheet requires that all endpoints are allocated in order.
>
> Pieces of information from the cover letter could have been added here.

yup, that's what I did:

commit 741d2558bf0aa8da9c0834ad43e1b9a1b16aa515
Author: Cristian Birsan <cristian.birsan@microchip.com>
Date:   Mon Jan 23 16:45:59 2017 +0200

    usb: gadget: udc: atmel: Update endpoint allocation scheme
    
    This patch updates the usb endpoint allocation scheme for atmel usba
    driver to make sure all endpoints are allocated in order. This
    requirement comes from the datasheet of the controller.
    
    The allocation scheme is decided by fifo_mode parameter. For fifo_mode =
    0 the driver tries to autoconfigure the endpoints fifo size. All other
    modes contain fixed configurations optimized for different purposes. The
    idea is somehow similar with the approach used on musb driver.
    
    Signed-off-by: Cristian Birsan <cristian.birsan@microchip.com>
    Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>

But seems like we will need a fixup patch during v4.11-rc?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[PATCH linux-next v2 1/1] usb: gadget: udc: atmel: Update endpoint allocation scheme

[Cristian Birsan]

I will send a fixup patch with updates based on the comments received from Nicolas.

On 01/26/2017 05:02 PM, Nicolas Ferre wrote:
> Le 23/01/2017 ? 15:45, cristian.birsan at microchip.com a ?crit :
>> From: Cristian Birsan <cristian.birsan@microchip.com>
>>
>> Update atmel udc driver with a new enpoint allocation scheme. The data
>> sheet requires that all endpoints are allocated in order.
> 
> Pieces of information from the cover letter could have been added here.
>

Ack. I'll remove the cover letter.

>> Signed-off-by: Cristian Birsan <cristian.birsan@microchip.com>
>> ---
>>  drivers/usb/gadget/udc/Kconfig          |  14 ++
>>  drivers/usb/gadget/udc/atmel_usba_udc.c | 236 +++++++++++++++++++++++++++-----
>>  drivers/usb/gadget/udc/atmel_usba_udc.h |  10 +-
>>  3 files changed, 227 insertions(+), 33 deletions(-)
> 
> It can be good to run ./scripts/checkpatch.pl --strict as well. It would
> have told you about some of the alignment and braces issues. I ran it as
> some the code has looked weird to me once or twice...
> 
> 

Ack.

>> diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
>> index 658b8da..4b69f28 100644
>> --- a/drivers/usb/gadget/udc/Kconfig
>> +++ b/drivers/usb/gadget/udc/Kconfig
>> @@ -60,6 +60,20 @@ config USB_ATMEL_USBA
>>  	  USBA is the integrated high-speed USB Device controller on
>>  	  the AT32AP700x, some AT91SAM9 and AT91CAP9 processors from Atmel.
>>  
>> +	  The fifo_mode parameter is used to select endpoint allocation mode.
>> +	  fifo_mode = 0 is used to let the driver autoconfigure the endpoints.
>> +	  In this case 2 banks are allocated for isochronous endpoints and
> 
> You mean that 2 banks can do isochronous xfers if needed, but they can
> do other types as well, right? So maybe rephrase the sentence.
> 

For fifo_mode = 0 (autoconfigure case) the driver allocates 2 banks only for
isochronous endpoints as required by the reference manual. For other endpoints
it will allocate only 1 bank because it cannot know in advance the number of
endpoints and the size of them, especially if the configfs is used to create
the gadget. The idea is to support as many endpoints as we can in this mode.

>> +	  only one bank is allocated for the rest of the endpoints.
>> +
>> +	  fifo_mode = 1 is a generic maximum fifo size (1024 bytes) configuration
>> +	  allowing the usage of ep1 - ep6
>> +
>> +	  fifo_mode = 2 is a generic performance maximum fifo size (1024 bytes)
>> +	  configuration allowing the usage of ep1 - ep3
>> +
>> +	  fifo_mode = 3 is a balanced performance configuration allowing the
>> +	  the usage of ep1 - ep8
>> +
>>  config USB_BCM63XX_UDC
>>  	tristate "Broadcom BCM63xx Peripheral Controller"
>>  	depends on BCM63XX
>> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
>> index 12c7687..11bbce2 100644
>> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
>> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
>> @@ -20,6 +20,7 @@
>>  #include <linux/mfd/syscon.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/regmap.h>
>> +#include <linux/ctype.h>
>>  #include <linux/usb/ch9.h>
>>  #include <linux/usb/gadget.h>
>>  #include <linux/usb/atmel_usba_udc.h>
>> @@ -318,6 +319,91 @@ static inline void usba_cleanup_debugfs(struct usba_udc *udc)
>>  }
>>  #endif
>>  
>> +static ushort fifo_mode;
>> +
>> +/* "modprobe ... fifo_mode=1" etc */
> 
> No need for this comment.
>
 
Ack. I'll remove it.

>> +module_param(fifo_mode, ushort, 0x0);
>> +MODULE_PARM_DESC(fifo_mode, "Endpoint configuration mode");
>> +
>> +/* mode 0 - uses autoconfig */
>> +
>> +/* mode 1 - fits in 8KB, generic max fifo configuration */
>> +static struct usba_fifo_cfg mode_1_cfg[] = {
>> +{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
>> +{ .hw_ep_num = 1, .fifo_size = 1024,	.nr_banks = 2, },
>> +{ .hw_ep_num = 2, .fifo_size = 1024,	.nr_banks = 1, },
>> +{ .hw_ep_num = 3, .fifo_size = 1024,	.nr_banks = 1, },
>> +{ .hw_ep_num = 4, .fifo_size = 1024,	.nr_banks = 1, },
>> +{ .hw_ep_num = 5, .fifo_size = 1024,	.nr_banks = 1, },
>> +{ .hw_ep_num = 6, .fifo_size = 1024,	.nr_banks = 1, },
>> +};
>> +
>> +/* mode 2 - fits in 8KB, performance max fifo configuration */
>> +static struct usba_fifo_cfg mode_2_cfg[] = {
>> +{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
>> +{ .hw_ep_num = 1, .fifo_size = 1024,	.nr_banks = 3, },
>> +{ .hw_ep_num = 2, .fifo_size = 1024,	.nr_banks = 2, },
>> +{ .hw_ep_num = 3, .fifo_size = 1024,	.nr_banks = 2, },
>> +};
>> +
>> +/* mode 3 - fits in 8KB, mixed fifo configuration */
>> +static struct usba_fifo_cfg mode_3_cfg[] = {
>> +{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
>> +{ .hw_ep_num = 1, .fifo_size = 1024,	.nr_banks = 2, },
>> +{ .hw_ep_num = 2, .fifo_size = 512,	.nr_banks = 2, },
>> +{ .hw_ep_num = 3, .fifo_size = 512,	.nr_banks = 2, },
>> +{ .hw_ep_num = 4, .fifo_size = 512,	.nr_banks = 2, },
>> +{ .hw_ep_num = 5, .fifo_size = 512,	.nr_banks = 2, },
>> +{ .hw_ep_num = 6, .fifo_size = 512,	.nr_banks = 2, },
>> +};
>> +
>> +/* mode 4 - fits in 8KB, custom fifo configuration */
>> +static struct usba_fifo_cfg mode_4_cfg[] = {
>> +{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
>> +{ .hw_ep_num = 1, .fifo_size = 512,	.nr_banks = 2, },
>> +{ .hw_ep_num = 2, .fifo_size = 512,	.nr_banks = 2, },
>> +{ .hw_ep_num = 3, .fifo_size = 8,	.nr_banks = 2, },
>> +{ .hw_ep_num = 4, .fifo_size = 512,	.nr_banks = 2, },
>> +{ .hw_ep_num = 5, .fifo_size = 512,	.nr_banks = 2, },
>> +{ .hw_ep_num = 6, .fifo_size = 16,	.nr_banks = 2, },
>> +{ .hw_ep_num = 7, .fifo_size = 8,	.nr_banks = 2, },
>> +{ .hw_ep_num = 8, .fifo_size = 8,	.nr_banks = 2, },
>> +};
>> +/* Add additional configurations here */
>> +
>> +int usba_config_fifo_table(struct usba_udc *udc)
> 
> Isn't is static?
> 

Ack. I'm going to make it static.

>> +{
>> +	int n;
>> +
>> +	switch (fifo_mode) {
>> +	default:
>> +		fifo_mode = 0;
>> +	case 0:
>> +		udc->fifo_cfg = NULL;
>> +		n = 0;
>> +		break;
>> +	case 1:
>> +		udc->fifo_cfg = mode_1_cfg;
>> +		n = ARRAY_SIZE(mode_1_cfg);
>> +		break;
>> +	case 2:
>> +		udc->fifo_cfg = mode_2_cfg;
>> +		n = ARRAY_SIZE(mode_2_cfg);
>> +		break;
>> +	case 3:
>> +		udc->fifo_cfg = mode_3_cfg;
>> +		n = ARRAY_SIZE(mode_3_cfg);
>> +		break;
>> +	case 4:
>> +		udc->fifo_cfg = mode_4_cfg;
>> +		n = ARRAY_SIZE(mode_4_cfg);
>> +		break;
>> +	}
>> +	DBG(DBG_HW, "Setup fifo_mode %d\n", fifo_mode);
>> +
>> +	return n;
>> +}
>> +
>>  static inline u32 usba_int_enb_get(struct usba_udc *udc)
>>  {
>>  	return udc->int_enb_cache;
>> @@ -543,24 +629,17 @@ usba_ep_enable(struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
>>  	ep->is_isoc = 0;
>>  	ep->is_in = 0;
>>  
>> -	if (maxpacket <= 8)
>> -		ept_cfg = USBA_BF(EPT_SIZE, USBA_EPT_SIZE_8);
>> -	else
>> -		/* LSB is bit 1, not 0 */
>> -		ept_cfg = USBA_BF(EPT_SIZE, fls(maxpacket - 1) - 3);
>> -
>> -	DBG(DBG_HW, "%s: EPT_SIZE = %lu (maxpacket = %lu)\n",
>> +	DBG(DBG_ERR, "%s: EPT_CFG = 0x%lx (maxpacket = %lu)\n",
>>  			ep->ep.name, ept_cfg, maxpacket);
>>  
>>  	if (usb_endpoint_dir_in(desc)) {
>>  		ep->is_in = 1;
>> -		ept_cfg |= USBA_EPT_DIR_IN;
>> +		ep->ept_cfg |= USBA_EPT_DIR_IN;
>>  	}
>>  
>>  	switch (usb_endpoint_type(desc)) {
>>  	case USB_ENDPOINT_XFER_CONTROL:
>> -		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_CONTROL);
>> -		ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_ONE);
>> +		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_CONTROL);
>>  		break;
>>  	case USB_ENDPOINT_XFER_ISOC:
>>  		if (!ep->can_isoc) {
>> @@ -578,24 +657,15 @@ usba_ep_enable(struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
>>  			return -EINVAL;
>>  
>>  		ep->is_isoc = 1;
>> -		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_ISO);
>> +		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_ISO);
>> +		ep->ept_cfg |= USBA_BF(NB_TRANS, nr_trans);
>>  
>> -		/*
>> -		 * Do triple-buffering on high-bandwidth iso endpoints.
>> -		 */
>> -		if (nr_trans > 1 && ep->nr_banks == 3)
>> -			ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_TRIPLE);
>> -		else
>> -			ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_DOUBLE);
>> -		ept_cfg |= USBA_BF(NB_TRANS, nr_trans);
>>  		break;
>>  	case USB_ENDPOINT_XFER_BULK:
>> -		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_BULK);
>> -		ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_DOUBLE);
>> +		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_BULK);
>>  		break;
>>  	case USB_ENDPOINT_XFER_INT:
>> -		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_INT);
>> -		ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_DOUBLE);
>> +		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_INT);
>>  		break;
>>  	}
>>  
>> @@ -604,7 +674,7 @@ usba_ep_enable(struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
>>  	ep->ep.desc = desc;
>>  	ep->ep.maxpacket = maxpacket;
>>  
>> -	usba_ep_writel(ep, CFG, ept_cfg);
>> +	usba_ep_writel(ep, CFG, ep->ept_cfg);
>>  	usba_ep_writel(ep, CTL_ENB, USBA_EPT_ENABLE);
>>  
>>  	if (ep->can_dma) {
>> @@ -1006,12 +1076,81 @@ static int atmel_usba_start(struct usb_gadget *gadget,
>>  		struct usb_gadget_driver *driver);
>>  static int atmel_usba_stop(struct usb_gadget *gadget);
>>  
>> +static struct usb_ep *atmel_usba_match_ep(
> 
> I find the indentation of this funciton wreird...
> 
>> +		struct usb_gadget		*gadget,
> 
> No need for a tab here and align with open parenthesis,
> 

Ack.

>> +		struct usb_endpoint_descriptor	*desc,
>> +		struct usb_ss_ep_comp_descriptor *ep_comp
>> +)
> 
> This parenthesis seems weird to me: put it after the last parameter.
>

Ack.
 
>> +{
>> +	struct usb_ep	*_ep;
>> +	struct usba_ep *ep;
>> +
>> +	/* Look at endpoints until an unclaimed one looks usable */
>> +	list_for_each_entry(_ep, &gadget->ep_list, ep_list) {
>> +		if (usb_gadget_ep_match_desc(gadget, _ep, desc, ep_comp))
>> +			goto found_ep;
>> +	}
>> +	/* Fail */
>> +	return NULL;
>> +
>> +found_ep:
>> +
>> +	if (fifo_mode == 0) {
>> +		/* Optimize hw fifo size based on ep type and other info */
>> +		ep = to_usba_ep(_ep);
>> +
>> +		switch (usb_endpoint_type(desc)) {
>> +
>> +		case USB_ENDPOINT_XFER_CONTROL:
>> +			break;
>> +
>> +		case USB_ENDPOINT_XFER_ISOC:
>> +			ep->fifo_size = 1024;
>> +			ep->nr_banks = 2;
>> +			break;
>> +
>> +		case USB_ENDPOINT_XFER_BULK:
>> +			ep->fifo_size = 512;
>> +			ep->nr_banks = 1;
>> +			break;
>> +
>> +		case USB_ENDPOINT_XFER_INT:
>> +			if (desc->wMaxPacketSize == 0)
>> +				ep->fifo_size =
>> +				    roundup_pow_of_two(_ep->maxpacket_limit);
>> +			else
>> +				ep->fifo_size =
>> +				    roundup_pow_of_two(le16_to_cpu(desc->wMaxPacketSize));
>> +			ep->nr_banks = 1;
>> +			break;
>> +		}
>> +
>> +		/* It might be a little bit late to set this */
> 
> Is it too late or not? This comment is frightening... We may need some
> feedback from the USB guys here...
>

If someone from USB can comment a bit on this topic I will be grateful.

The idea here is to optimize the endpoint size used by usb gadgets even if they are created
at runtime using configfs. Calling usb_ep_set_maxpacket_limit() in probe is too early for
the size of the endpoint used by the above gadget function to be known. Setting this
value here it might be a bit late only if the above layer is already using internally the
maxpacket value configured in probe. From my tests on CDC ACM, RNDIS and HID this is not
a problem. I added the message as a warning.

>> +		usb_ep_set_maxpacket_limit(&ep->ep, ep->fifo_size);
>> +
>> +		/* Generate ept_cfg basd on FIFO size and number of banks */
>> +		if (ep->fifo_size  <= 8)
>> +			ep->ept_cfg = USBA_BF(EPT_SIZE, USBA_EPT_SIZE_8);
>> +		else
>> +			/* LSB is bit 1, not 0 */
>> +			ep->ept_cfg =
>> +				USBA_BF(EPT_SIZE, fls(ep->fifo_size - 1) - 3);
>> +
>> +		ep->ept_cfg |= USBA_BF(BK_NUMBER, ep->nr_banks);
>> +		ep->udc->configured_ep++;
> 
> is it more than a boolean value?
> 

Yes. It is more than a boolean value. It counts the number of configured
endpoints by atmel_usba_match_ep() function that is used as autoconfigure
mechanism.

>> +	}
>> +
>> +return _ep;
> 
> Indentation issue here ^
> 

Ack.

>> +}
>> +
>>  static const struct usb_gadget_ops usba_udc_ops = {
>>  	.get_frame		= usba_udc_get_frame,
>>  	.wakeup			= usba_udc_wakeup,
>>  	.set_selfpowered	= usba_udc_set_selfpowered,
>>  	.udc_start		= atmel_usba_start,
>>  	.udc_stop		= atmel_usba_stop,
>> +	.match_ep		= atmel_usba_match_ep,
>>  };
>>  
>>  static struct usb_endpoint_descriptor usba_ep0_desc = {
>> @@ -1678,7 +1817,8 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
>>  	}
>>  
>>  	if (status & USBA_END_OF_RESET) {
>> -		struct usba_ep *ep0;
>> +		struct usba_ep *ep0, *ep;
>> +		int i, n;
>>  
>>  		usba_writel(udc, INT_CLR, USBA_END_OF_RESET);
>>  		generate_bias_pulse(udc);
>> @@ -1717,6 +1857,16 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
>>  		if (!(usba_ep_readl(ep0, CFG) & USBA_EPT_MAPPED))
>>  			dev_dbg(&udc->pdev->dev,
>>  				 "ODD: EP0 configuration is invalid!\n");
>> +
>> +		/* Preallocate other endpoints */
>> +		n = fifo_mode ? udc->num_ep : udc->configured_ep;
>> +		for (i = 1; i < n; i++) {
>> +			ep = &udc->usba_ep[i];
>> +			usba_ep_writel(ep, CFG, ep->ept_cfg);
>> +			if (!(usba_ep_readl(ep, CFG) & USBA_EPT_MAPPED))
>> +				dev_dbg(&udc->pdev->dev,
>> +				 "ODD: EP%d configuration is invalid!\n", i);
> 
> I know that it's the case for the EP0 just above and that we do it like
> this in the current driver but maybe we can do more than just having a
> debug message.
> 

As you said the debug message follows the pattern that was already in the driver.
I never stumbled upon this corner case but for now I would like to keep the print
here to avoid any silent failure. If the USBA_EPT_MAPPED flag is not raised by
hardware it means that internal memory allocation in the controller went wrong (most
of the time due to not enough memory, caused by too many / large endpoints). It's
hard to imagine a solution that can fix this situation without reinitializing the
usba controller. This can be fixed in a later patch if it's needed.

>> +		}
>>  	}
>>  
>>  	spin_unlock(&udc->lock);
>> @@ -1864,6 +2014,9 @@ static int atmel_usba_stop(struct usb_gadget *gadget)
>>  	if (gpio_is_valid(udc->vbus_pin))
>>  		disable_irq(gpio_to_irq(udc->vbus_pin));
>>  
>> +	if (fifo_mode == 0)
>> +		udc->configured_ep = 1;
>> +
>>  	usba_stop(udc);
>>  
>>  	udc->driver = NULL;
>> @@ -1931,9 +2084,13 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
>>  						&flags);
>>  	udc->vbus_pin_inverted = (flags & OF_GPIO_ACTIVE_LOW) ? 1 : 0;
>>  
>> -	pp = NULL;
>> -	while ((pp = of_get_next_child(np, pp)))
>> -		udc->num_ep++;
>> +	if (fifo_mode == 0) {
>> +		pp = NULL;
>> +		while ((pp = of_get_next_child(np, pp)))
>> +			udc->num_ep++;
>> +		udc->configured_ep = 1;
>> +	} else
> 
> Braces here
> 

Ack. Will fix.

>> +		udc->num_ep = usba_config_fifo_table(udc);
>>  
>>  	eps = devm_kzalloc(&pdev->dev, sizeof(struct usba_ep) * udc->num_ep,
>>  			   GFP_KERNEL);
>> @@ -1946,7 +2103,7 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
>>  
>>  	pp = NULL;
>>  	i = 0;
>> -	while ((pp = of_get_next_child(np, pp))) {
>> +	while ((pp = of_get_next_child(np, pp)) && i < udc->num_ep) {
>>  		ep = &eps[i];
>>  
>>  		ret = of_property_read_u32(pp, "reg", &val);
>> @@ -1954,21 +2111,21 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
>>  			dev_err(&pdev->dev, "of_probe: reg error(%d)\n", ret);
>>  			goto err;
>>  		}
>> -		ep->index = val;
>> +		ep->index = fifo_mode ? udc->fifo_cfg[i].hw_ep_num : val;
>>  
>>  		ret = of_property_read_u32(pp, "atmel,fifo-size", &val);
>>  		if (ret) {
>>  			dev_err(&pdev->dev, "of_probe: fifo-size error(%d)\n", ret);
>>  			goto err;
>>  		}
>> -		ep->fifo_size = val;
>> +		ep->fifo_size = fifo_mode ? udc->fifo_cfg[i].fifo_size : val;
> 
> No, this is where I would like to see that the value from the table is
> still validated agains the "max value" that is stored in HW description/DT.
> I know that all of our product are able to handle 1024 fifo size, but
> it's more a savfe check...
>

Ack. I'll validate against "max value".
 
>>  
>>  		ret = of_property_read_u32(pp, "atmel,nb-banks", &val);
>>  		if (ret) {
>>  			dev_err(&pdev->dev, "of_probe: nb-banks error(%d)\n", ret);
>>  			goto err;
>>  		}
>> -		ep->nr_banks = val;
>> +		ep->nr_banks = fifo_mode ? udc->fifo_cfg[i].nr_banks : val;
> 
> Here however, it is pretty important to check this value. For instance,
> notice that the at91sam9x5 cannot fullfil the "mode 2" configuration: we
> can't silentely ignore the HW specifications given by DT.
> 

Ack. Good catch. Although at91sam9x5 has a slightly different name for the usb
device controller in the reference manual, it is basically the same ip. 

>>  		ep->can_dma = of_property_read_bool(pp, "atmel,can-dma");
>>  		ep->can_isoc = of_property_read_bool(pp, "atmel,can-isoc");
>> @@ -2000,6 +2157,21 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
>>  		ep->ep.caps.dir_in = true;
>>  		ep->ep.caps.dir_out = true;
>>  
>> +		if (fifo_mode != 0) {
>> +			/*
>> +			 * Generate ept_cfg based on FIFO size and
>> +			 * banks number
>> +			 */
>> +			if (ep->fifo_size  <= 8)
>> +				ep->ept_cfg = USBA_BF(EPT_SIZE, USBA_EPT_SIZE_8);
>> +			else
>> +				/* LSB is bit 1, not 0 */
>> +				ep->ept_cfg =
>> +				  USBA_BF(EPT_SIZE, fls(ep->fifo_size - 1) - 3);
>> +
>> +			ep->ept_cfg |= USBA_BF(BK_NUMBER, ep->nr_banks);
>> +		}
>> +
>>  		if (i)
>>  			list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
>>  
>> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.h b/drivers/usb/gadget/udc/atmel_usba_udc.h
>> index b03b2eb..9551b70 100644
>> --- a/drivers/usb/gadget/udc/atmel_usba_udc.h
>> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.h
>> @@ -275,6 +275,12 @@ struct usba_dma_desc {
>>  	u32 ctrl;
>>  };
>>  
>> +struct usba_fifo_cfg {
>> +	u8			hw_ep_num;
>> +	u16			fifo_size;
>> +	u8			nr_banks;
>> +};
>> +
>>  struct usba_ep {
>>  	int					state;
>>  	void __iomem				*ep_regs;
>> @@ -293,7 +299,7 @@ struct usba_ep {
>>  	unsigned int				can_isoc:1;
>>  	unsigned int				is_isoc:1;
>>  	unsigned int				is_in:1;
>> -
>> +	unsigned long				ept_cfg;
>>  #ifdef CONFIG_USB_GADGET_DEBUG_FS
>>  	u32					last_dma_status;
>>  	struct dentry				*debugfs_dir;
>> @@ -338,6 +344,8 @@ struct usba_udc {
>>  	int vbus_pin;
>>  	int vbus_pin_inverted;
>>  	int num_ep;
>> +	int configured_ep;
>> +	struct usba_fifo_cfg *fifo_cfg;
>>  	struct clk *pclk;
>>  	struct clk *hclk;
>>  	struct usba_ep *usba_ep;
>>
> 
> Regards,
> 

Re: [PATCH linux-next v2 1/1] usb: gadget: udc: atmel: Update endpoint allocation scheme

[Cristian Birsan]

I will send a fixup patch with updates based on the comments received from Nicolas.

On 01/26/2017 05:02 PM, Nicolas Ferre wrote:
> Le 23/01/2017 à 15:45, cristian.birsan@microchip.com a écrit :
>> From: Cristian Birsan <cristian.birsan@microchip.com>
>>
>> Update atmel udc driver with a new enpoint allocation scheme. The data
>> sheet requires that all endpoints are allocated in order.
> 
> Pieces of information from the cover letter could have been added here.
>

Ack. I'll remove the cover letter.

>> Signed-off-by: Cristian Birsan <cristian.birsan@microchip.com>
>> ---
>>  drivers/usb/gadget/udc/Kconfig          |  14 ++
>>  drivers/usb/gadget/udc/atmel_usba_udc.c | 236 +++++++++++++++++++++++++++-----
>>  drivers/usb/gadget/udc/atmel_usba_udc.h |  10 +-
>>  3 files changed, 227 insertions(+), 33 deletions(-)
> 
> It can be good to run ./scripts/checkpatch.pl --strict as well. It would
> have told you about some of the alignment and braces issues. I ran it as
> some the code has looked weird to me once or twice...
> 
> 

Ack.

>> diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
>> index 658b8da..4b69f28 100644
>> --- a/drivers/usb/gadget/udc/Kconfig
>> +++ b/drivers/usb/gadget/udc/Kconfig
>> @@ -60,6 +60,20 @@ config USB_ATMEL_USBA
>>  	  USBA is the integrated high-speed USB Device controller on
>>  	  the AT32AP700x, some AT91SAM9 and AT91CAP9 processors from Atmel.
>>  
>> +	  The fifo_mode parameter is used to select endpoint allocation mode.
>> +	  fifo_mode = 0 is used to let the driver autoconfigure the endpoints.
>> +	  In this case 2 banks are allocated for isochronous endpoints and
> 
> You mean that 2 banks can do isochronous xfers if needed, but they can
> do other types as well, right? So maybe rephrase the sentence.
> 

For fifo_mode = 0 (autoconfigure case) the driver allocates 2 banks only for
isochronous endpoints as required by the reference manual. For other endpoints
it will allocate only 1 bank because it cannot know in advance the number of
endpoints and the size of them, especially if the configfs is used to create
the gadget. The idea is to support as many endpoints as we can in this mode.

>> +	  only one bank is allocated for the rest of the endpoints.
>> +
>> +	  fifo_mode = 1 is a generic maximum fifo size (1024 bytes) configuration
>> +	  allowing the usage of ep1 - ep6
>> +
>> +	  fifo_mode = 2 is a generic performance maximum fifo size (1024 bytes)
>> +	  configuration allowing the usage of ep1 - ep3
>> +
>> +	  fifo_mode = 3 is a balanced performance configuration allowing the
>> +	  the usage of ep1 - ep8
>> +
>>  config USB_BCM63XX_UDC
>>  	tristate "Broadcom BCM63xx Peripheral Controller"
>>  	depends on BCM63XX
>> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
>> index 12c7687..11bbce2 100644
>> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
>> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
>> @@ -20,6 +20,7 @@
>>  #include <linux/mfd/syscon.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/regmap.h>
>> +#include <linux/ctype.h>
>>  #include <linux/usb/ch9.h>
>>  #include <linux/usb/gadget.h>
>>  #include <linux/usb/atmel_usba_udc.h>
>> @@ -318,6 +319,91 @@ static inline void usba_cleanup_debugfs(struct usba_udc *udc)
>>  }
>>  #endif
>>  
>> +static ushort fifo_mode;
>> +
>> +/* "modprobe ... fifo_mode=1" etc */
> 
> No need for this comment.
>
 
Ack. I'll remove it.

>> +module_param(fifo_mode, ushort, 0x0);
>> +MODULE_PARM_DESC(fifo_mode, "Endpoint configuration mode");
>> +
>> +/* mode 0 - uses autoconfig */
>> +
>> +/* mode 1 - fits in 8KB, generic max fifo configuration */
>> +static struct usba_fifo_cfg mode_1_cfg[] = {
>> +{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
>> +{ .hw_ep_num = 1, .fifo_size = 1024,	.nr_banks = 2, },
>> +{ .hw_ep_num = 2, .fifo_size = 1024,	.nr_banks = 1, },
>> +{ .hw_ep_num = 3, .fifo_size = 1024,	.nr_banks = 1, },
>> +{ .hw_ep_num = 4, .fifo_size = 1024,	.nr_banks = 1, },
>> +{ .hw_ep_num = 5, .fifo_size = 1024,	.nr_banks = 1, },
>> +{ .hw_ep_num = 6, .fifo_size = 1024,	.nr_banks = 1, },
>> +};
>> +
>> +/* mode 2 - fits in 8KB, performance max fifo configuration */
>> +static struct usba_fifo_cfg mode_2_cfg[] = {
>> +{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
>> +{ .hw_ep_num = 1, .fifo_size = 1024,	.nr_banks = 3, },
>> +{ .hw_ep_num = 2, .fifo_size = 1024,	.nr_banks = 2, },
>> +{ .hw_ep_num = 3, .fifo_size = 1024,	.nr_banks = 2, },
>> +};
>> +
>> +/* mode 3 - fits in 8KB, mixed fifo configuration */
>> +static struct usba_fifo_cfg mode_3_cfg[] = {
>> +{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
>> +{ .hw_ep_num = 1, .fifo_size = 1024,	.nr_banks = 2, },
>> +{ .hw_ep_num = 2, .fifo_size = 512,	.nr_banks = 2, },
>> +{ .hw_ep_num = 3, .fifo_size = 512,	.nr_banks = 2, },
>> +{ .hw_ep_num = 4, .fifo_size = 512,	.nr_banks = 2, },
>> +{ .hw_ep_num = 5, .fifo_size = 512,	.nr_banks = 2, },
>> +{ .hw_ep_num = 6, .fifo_size = 512,	.nr_banks = 2, },
>> +};
>> +
>> +/* mode 4 - fits in 8KB, custom fifo configuration */
>> +static struct usba_fifo_cfg mode_4_cfg[] = {
>> +{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
>> +{ .hw_ep_num = 1, .fifo_size = 512,	.nr_banks = 2, },
>> +{ .hw_ep_num = 2, .fifo_size = 512,	.nr_banks = 2, },
>> +{ .hw_ep_num = 3, .fifo_size = 8,	.nr_banks = 2, },
>> +{ .hw_ep_num = 4, .fifo_size = 512,	.nr_banks = 2, },
>> +{ .hw_ep_num = 5, .fifo_size = 512,	.nr_banks = 2, },
>> +{ .hw_ep_num = 6, .fifo_size = 16,	.nr_banks = 2, },
>> +{ .hw_ep_num = 7, .fifo_size = 8,	.nr_banks = 2, },
>> +{ .hw_ep_num = 8, .fifo_size = 8,	.nr_banks = 2, },
>> +};
>> +/* Add additional configurations here */
>> +
>> +int usba_config_fifo_table(struct usba_udc *udc)
> 
> Isn't is static?
> 

Ack. I'm going to make it static.

>> +{
>> +	int n;
>> +
>> +	switch (fifo_mode) {
>> +	default:
>> +		fifo_mode = 0;
>> +	case 0:
>> +		udc->fifo_cfg = NULL;
>> +		n = 0;
>> +		break;
>> +	case 1:
>> +		udc->fifo_cfg = mode_1_cfg;
>> +		n = ARRAY_SIZE(mode_1_cfg);
>> +		break;
>> +	case 2:
>> +		udc->fifo_cfg = mode_2_cfg;
>> +		n = ARRAY_SIZE(mode_2_cfg);
>> +		break;
>> +	case 3:
>> +		udc->fifo_cfg = mode_3_cfg;
>> +		n = ARRAY_SIZE(mode_3_cfg);
>> +		break;
>> +	case 4:
>> +		udc->fifo_cfg = mode_4_cfg;
>> +		n = ARRAY_SIZE(mode_4_cfg);
>> +		break;
>> +	}
>> +	DBG(DBG_HW, "Setup fifo_mode %d\n", fifo_mode);
>> +
>> +	return n;
>> +}
>> +
>>  static inline u32 usba_int_enb_get(struct usba_udc *udc)
>>  {
>>  	return udc->int_enb_cache;
>> @@ -543,24 +629,17 @@ usba_ep_enable(struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
>>  	ep->is_isoc = 0;
>>  	ep->is_in = 0;
>>  
>> -	if (maxpacket <= 8)
>> -		ept_cfg = USBA_BF(EPT_SIZE, USBA_EPT_SIZE_8);
>> -	else
>> -		/* LSB is bit 1, not 0 */
>> -		ept_cfg = USBA_BF(EPT_SIZE, fls(maxpacket - 1) - 3);
>> -
>> -	DBG(DBG_HW, "%s: EPT_SIZE = %lu (maxpacket = %lu)\n",
>> +	DBG(DBG_ERR, "%s: EPT_CFG = 0x%lx (maxpacket = %lu)\n",
>>  			ep->ep.name, ept_cfg, maxpacket);
>>  
>>  	if (usb_endpoint_dir_in(desc)) {
>>  		ep->is_in = 1;
>> -		ept_cfg |= USBA_EPT_DIR_IN;
>> +		ep->ept_cfg |= USBA_EPT_DIR_IN;
>>  	}
>>  
>>  	switch (usb_endpoint_type(desc)) {
>>  	case USB_ENDPOINT_XFER_CONTROL:
>> -		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_CONTROL);
>> -		ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_ONE);
>> +		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_CONTROL);
>>  		break;
>>  	case USB_ENDPOINT_XFER_ISOC:
>>  		if (!ep->can_isoc) {
>> @@ -578,24 +657,15 @@ usba_ep_enable(struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
>>  			return -EINVAL;
>>  
>>  		ep->is_isoc = 1;
>> -		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_ISO);
>> +		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_ISO);
>> +		ep->ept_cfg |= USBA_BF(NB_TRANS, nr_trans);
>>  
>> -		/*
>> -		 * Do triple-buffering on high-bandwidth iso endpoints.
>> -		 */
>> -		if (nr_trans > 1 && ep->nr_banks == 3)
>> -			ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_TRIPLE);
>> -		else
>> -			ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_DOUBLE);
>> -		ept_cfg |= USBA_BF(NB_TRANS, nr_trans);
>>  		break;
>>  	case USB_ENDPOINT_XFER_BULK:
>> -		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_BULK);
>> -		ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_DOUBLE);
>> +		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_BULK);
>>  		break;
>>  	case USB_ENDPOINT_XFER_INT:
>> -		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_INT);
>> -		ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_DOUBLE);
>> +		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_INT);
>>  		break;
>>  	}
>>  
>> @@ -604,7 +674,7 @@ usba_ep_enable(struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
>>  	ep->ep.desc = desc;
>>  	ep->ep.maxpacket = maxpacket;
>>  
>> -	usba_ep_writel(ep, CFG, ept_cfg);
>> +	usba_ep_writel(ep, CFG, ep->ept_cfg);
>>  	usba_ep_writel(ep, CTL_ENB, USBA_EPT_ENABLE);
>>  
>>  	if (ep->can_dma) {
>> @@ -1006,12 +1076,81 @@ static int atmel_usba_start(struct usb_gadget *gadget,
>>  		struct usb_gadget_driver *driver);
>>  static int atmel_usba_stop(struct usb_gadget *gadget);
>>  
>> +static struct usb_ep *atmel_usba_match_ep(
> 
> I find the indentation of this funciton wreird...
> 
>> +		struct usb_gadget		*gadget,
> 
> No need for a tab here and align with open parenthesis,
> 

Ack.

>> +		struct usb_endpoint_descriptor	*desc,
>> +		struct usb_ss_ep_comp_descriptor *ep_comp
>> +)
> 
> This parenthesis seems weird to me: put it after the last parameter.
>

Ack.
 
>> +{
>> +	struct usb_ep	*_ep;
>> +	struct usba_ep *ep;
>> +
>> +	/* Look at endpoints until an unclaimed one looks usable */
>> +	list_for_each_entry(_ep, &gadget->ep_list, ep_list) {
>> +		if (usb_gadget_ep_match_desc(gadget, _ep, desc, ep_comp))
>> +			goto found_ep;
>> +	}
>> +	/* Fail */
>> +	return NULL;
>> +
>> +found_ep:
>> +
>> +	if (fifo_mode == 0) {
>> +		/* Optimize hw fifo size based on ep type and other info */
>> +		ep = to_usba_ep(_ep);
>> +
>> +		switch (usb_endpoint_type(desc)) {
>> +
>> +		case USB_ENDPOINT_XFER_CONTROL:
>> +			break;
>> +
>> +		case USB_ENDPOINT_XFER_ISOC:
>> +			ep->fifo_size = 1024;
>> +			ep->nr_banks = 2;
>> +			break;
>> +
>> +		case USB_ENDPOINT_XFER_BULK:
>> +			ep->fifo_size = 512;
>> +			ep->nr_banks = 1;
>> +			break;
>> +
>> +		case USB_ENDPOINT_XFER_INT:
>> +			if (desc->wMaxPacketSize == 0)
>> +				ep->fifo_size =
>> +				    roundup_pow_of_two(_ep->maxpacket_limit);
>> +			else
>> +				ep->fifo_size =
>> +				    roundup_pow_of_two(le16_to_cpu(desc->wMaxPacketSize));
>> +			ep->nr_banks = 1;
>> +			break;
>> +		}
>> +
>> +		/* It might be a little bit late to set this */
> 
> Is it too late or not? This comment is frightening... We may need some
> feedback from the USB guys here...
>

If someone from USB can comment a bit on this topic I will be grateful.

The idea here is to optimize the endpoint size used by usb gadgets even if they are created
at runtime using configfs. Calling usb_ep_set_maxpacket_limit() in probe is too early for
the size of the endpoint used by the above gadget function to be known. Setting this
value here it might be a bit late only if the above layer is already using internally the
maxpacket value configured in probe. From my tests on CDC ACM, RNDIS and HID this is not
a problem. I added the message as a warning.

>> +		usb_ep_set_maxpacket_limit(&ep->ep, ep->fifo_size);
>> +
>> +		/* Generate ept_cfg basd on FIFO size and number of banks */
>> +		if (ep->fifo_size  <= 8)
>> +			ep->ept_cfg = USBA_BF(EPT_SIZE, USBA_EPT_SIZE_8);
>> +		else
>> +			/* LSB is bit 1, not 0 */
>> +			ep->ept_cfg =
>> +				USBA_BF(EPT_SIZE, fls(ep->fifo_size - 1) - 3);
>> +
>> +		ep->ept_cfg |= USBA_BF(BK_NUMBER, ep->nr_banks);
>> +		ep->udc->configured_ep++;
> 
> is it more than a boolean value?
> 

Yes. It is more than a boolean value. It counts the number of configured
endpoints by atmel_usba_match_ep() function that is used as autoconfigure
mechanism.

>> +	}
>> +
>> +return _ep;
> 
> Indentation issue here ^
> 

Ack.

>> +}
>> +
>>  static const struct usb_gadget_ops usba_udc_ops = {
>>  	.get_frame		= usba_udc_get_frame,
>>  	.wakeup			= usba_udc_wakeup,
>>  	.set_selfpowered	= usba_udc_set_selfpowered,
>>  	.udc_start		= atmel_usba_start,
>>  	.udc_stop		= atmel_usba_stop,
>> +	.match_ep		= atmel_usba_match_ep,
>>  };
>>  
>>  static struct usb_endpoint_descriptor usba_ep0_desc = {
>> @@ -1678,7 +1817,8 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
>>  	}
>>  
>>  	if (status & USBA_END_OF_RESET) {
>> -		struct usba_ep *ep0;
>> +		struct usba_ep *ep0, *ep;
>> +		int i, n;
>>  
>>  		usba_writel(udc, INT_CLR, USBA_END_OF_RESET);
>>  		generate_bias_pulse(udc);
>> @@ -1717,6 +1857,16 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
>>  		if (!(usba_ep_readl(ep0, CFG) & USBA_EPT_MAPPED))
>>  			dev_dbg(&udc->pdev->dev,
>>  				 "ODD: EP0 configuration is invalid!\n");
>> +
>> +		/* Preallocate other endpoints */
>> +		n = fifo_mode ? udc->num_ep : udc->configured_ep;
>> +		for (i = 1; i < n; i++) {
>> +			ep = &udc->usba_ep[i];
>> +			usba_ep_writel(ep, CFG, ep->ept_cfg);
>> +			if (!(usba_ep_readl(ep, CFG) & USBA_EPT_MAPPED))
>> +				dev_dbg(&udc->pdev->dev,
>> +				 "ODD: EP%d configuration is invalid!\n", i);
> 
> I know that it's the case for the EP0 just above and that we do it like
> this in the current driver but maybe we can do more than just having a
> debug message.
> 

As you said the debug message follows the pattern that was already in the driver.
I never stumbled upon this corner case but for now I would like to keep the print
here to avoid any silent failure. If the USBA_EPT_MAPPED flag is not raised by
hardware it means that internal memory allocation in the controller went wrong (most
of the time due to not enough memory, caused by too many / large endpoints). It's
hard to imagine a solution that can fix this situation without reinitializing the
usba controller. This can be fixed in a later patch if it's needed.

>> +		}
>>  	}
>>  
>>  	spin_unlock(&udc->lock);
>> @@ -1864,6 +2014,9 @@ static int atmel_usba_stop(struct usb_gadget *gadget)
>>  	if (gpio_is_valid(udc->vbus_pin))
>>  		disable_irq(gpio_to_irq(udc->vbus_pin));
>>  
>> +	if (fifo_mode == 0)
>> +		udc->configured_ep = 1;
>> +
>>  	usba_stop(udc);
>>  
>>  	udc->driver = NULL;
>> @@ -1931,9 +2084,13 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
>>  						&flags);
>>  	udc->vbus_pin_inverted = (flags & OF_GPIO_ACTIVE_LOW) ? 1 : 0;
>>  
>> -	pp = NULL;
>> -	while ((pp = of_get_next_child(np, pp)))
>> -		udc->num_ep++;
>> +	if (fifo_mode == 0) {
>> +		pp = NULL;
>> +		while ((pp = of_get_next_child(np, pp)))
>> +			udc->num_ep++;
>> +		udc->configured_ep = 1;
>> +	} else
> 
> Braces here
> 

Ack. Will fix.

>> +		udc->num_ep = usba_config_fifo_table(udc);
>>  
>>  	eps = devm_kzalloc(&pdev->dev, sizeof(struct usba_ep) * udc->num_ep,
>>  			   GFP_KERNEL);
>> @@ -1946,7 +2103,7 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
>>  
>>  	pp = NULL;
>>  	i = 0;
>> -	while ((pp = of_get_next_child(np, pp))) {
>> +	while ((pp = of_get_next_child(np, pp)) && i < udc->num_ep) {
>>  		ep = &eps[i];
>>  
>>  		ret = of_property_read_u32(pp, "reg", &val);
>> @@ -1954,21 +2111,21 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
>>  			dev_err(&pdev->dev, "of_probe: reg error(%d)\n", ret);
>>  			goto err;
>>  		}
>> -		ep->index = val;
>> +		ep->index = fifo_mode ? udc->fifo_cfg[i].hw_ep_num : val;
>>  
>>  		ret = of_property_read_u32(pp, "atmel,fifo-size", &val);
>>  		if (ret) {
>>  			dev_err(&pdev->dev, "of_probe: fifo-size error(%d)\n", ret);
>>  			goto err;
>>  		}
>> -		ep->fifo_size = val;
>> +		ep->fifo_size = fifo_mode ? udc->fifo_cfg[i].fifo_size : val;
> 
> No, this is where I would like to see that the value from the table is
> still validated agains the "max value" that is stored in HW description/DT.
> I know that all of our product are able to handle 1024 fifo size, but
> it's more a savfe check...
>

Ack. I'll validate against "max value".
 
>>  
>>  		ret = of_property_read_u32(pp, "atmel,nb-banks", &val);
>>  		if (ret) {
>>  			dev_err(&pdev->dev, "of_probe: nb-banks error(%d)\n", ret);
>>  			goto err;
>>  		}
>> -		ep->nr_banks = val;
>> +		ep->nr_banks = fifo_mode ? udc->fifo_cfg[i].nr_banks : val;
> 
> Here however, it is pretty important to check this value. For instance,
> notice that the at91sam9x5 cannot fullfil the "mode 2" configuration: we
> can't silentely ignore the HW specifications given by DT.
> 

Ack. Good catch. Although at91sam9x5 has a slightly different name for the usb
device controller in the reference manual, it is basically the same ip. 

>>  		ep->can_dma = of_property_read_bool(pp, "atmel,can-dma");
>>  		ep->can_isoc = of_property_read_bool(pp, "atmel,can-isoc");
>> @@ -2000,6 +2157,21 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
>>  		ep->ep.caps.dir_in = true;
>>  		ep->ep.caps.dir_out = true;
>>  
>> +		if (fifo_mode != 0) {
>> +			/*
>> +			 * Generate ept_cfg based on FIFO size and
>> +			 * banks number
>> +			 */
>> +			if (ep->fifo_size  <= 8)
>> +				ep->ept_cfg = USBA_BF(EPT_SIZE, USBA_EPT_SIZE_8);
>> +			else
>> +				/* LSB is bit 1, not 0 */
>> +				ep->ept_cfg =
>> +				  USBA_BF(EPT_SIZE, fls(ep->fifo_size - 1) - 3);
>> +
>> +			ep->ept_cfg |= USBA_BF(BK_NUMBER, ep->nr_banks);
>> +		}
>> +
>>  		if (i)
>>  			list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
>>  
>> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.h b/drivers/usb/gadget/udc/atmel_usba_udc.h
>> index b03b2eb..9551b70 100644
>> --- a/drivers/usb/gadget/udc/atmel_usba_udc.h
>> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.h
>> @@ -275,6 +275,12 @@ struct usba_dma_desc {
>>  	u32 ctrl;
>>  };
>>  
>> +struct usba_fifo_cfg {
>> +	u8			hw_ep_num;
>> +	u16			fifo_size;
>> +	u8			nr_banks;
>> +};
>> +
>>  struct usba_ep {
>>  	int					state;
>>  	void __iomem				*ep_regs;
>> @@ -293,7 +299,7 @@ struct usba_ep {
>>  	unsigned int				can_isoc:1;
>>  	unsigned int				is_isoc:1;
>>  	unsigned int				is_in:1;
>> -
>> +	unsigned long				ept_cfg;
>>  #ifdef CONFIG_USB_GADGET_DEBUG_FS
>>  	u32					last_dma_status;
>>  	struct dentry				*debugfs_dir;
>> @@ -338,6 +344,8 @@ struct usba_udc {
>>  	int vbus_pin;
>>  	int vbus_pin_inverted;
>>  	int num_ep;
>> +	int configured_ep;
>> +	struct usba_fifo_cfg *fifo_cfg;
>>  	struct clk *pclk;
>>  	struct clk *hclk;
>>  	struct usba_ep *usba_ep;
>>
> 
> Regards,
> 

[PATCH linux-next v2 1/1] usb: gadget: udc: atmel: Update endpoint allocation scheme

[Nicolas Ferre]

Le 27/01/2017 ? 14:47, Cristian Birsan a ?crit :
> I will send a fixup patch with updates based on the comments received from Nicolas.
> 
> On 01/26/2017 05:02 PM, Nicolas Ferre wrote:
>> Le 23/01/2017 ? 15:45, cristian.birsan at microchip.com a ?crit :
>>> From: Cristian Birsan <cristian.birsan@microchip.com>
>>>
>>> Update atmel udc driver with a new enpoint allocation scheme. The data
>>> sheet requires that all endpoints are allocated in order.
>>
>> Pieces of information from the cover letter could have been added here.
>>
> 
> Ack. I'll remove the cover letter.
> 
>>> Signed-off-by: Cristian Birsan <cristian.birsan@microchip.com>
>>> ---
>>>  drivers/usb/gadget/udc/Kconfig          |  14 ++
>>>  drivers/usb/gadget/udc/atmel_usba_udc.c | 236 +++++++++++++++++++++++++++-----
>>>  drivers/usb/gadget/udc/atmel_usba_udc.h |  10 +-
>>>  3 files changed, 227 insertions(+), 33 deletions(-)
>>
>> It can be good to run ./scripts/checkpatch.pl --strict as well. It would
>> have told you about some of the alignment and braces issues. I ran it as
>> some the code has looked weird to me once or twice...
>>
>>
> 
> Ack.
> 
>>> diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
>>> index 658b8da..4b69f28 100644
>>> --- a/drivers/usb/gadget/udc/Kconfig
>>> +++ b/drivers/usb/gadget/udc/Kconfig
>>> @@ -60,6 +60,20 @@ config USB_ATMEL_USBA
>>>  	  USBA is the integrated high-speed USB Device controller on
>>>  	  the AT32AP700x, some AT91SAM9 and AT91CAP9 processors from Atmel.
>>>  
>>> +	  The fifo_mode parameter is used to select endpoint allocation mode.
>>> +	  fifo_mode = 0 is used to let the driver autoconfigure the endpoints.
>>> +	  In this case 2 banks are allocated for isochronous endpoints and
>>
>> You mean that 2 banks can do isochronous xfers if needed, but they can
>> do other types as well, right? So maybe rephrase the sentence.
>>
> 
> For fifo_mode = 0 (autoconfigure case) the driver allocates 2 banks only for
> isochronous endpoints as required by the reference manual. For other endpoints
> it will allocate only 1 bank because it cannot know in advance the number of
> endpoints and the size of them, especially if the configfs is used to create
> the gadget. The idea is to support as many endpoints as we can in this mode.

You know that I know this. It was just a remark about the wording of
your sentence. "are allocated for isochronous" can be turned into "are
allocated so that they could be use as isochronous endpoints". Not a big
deal though.


>>> +	  only one bank is allocated for the rest of the endpoints.
>>> +
>>> +	  fifo_mode = 1 is a generic maximum fifo size (1024 bytes) configuration
>>> +	  allowing the usage of ep1 - ep6
>>> +
>>> +	  fifo_mode = 2 is a generic performance maximum fifo size (1024 bytes)
>>> +	  configuration allowing the usage of ep1 - ep3
>>> +
>>> +	  fifo_mode = 3 is a balanced performance configuration allowing the
>>> +	  the usage of ep1 - ep8
>>> +
>>>  config USB_BCM63XX_UDC
>>>  	tristate "Broadcom BCM63xx Peripheral Controller"
>>>  	depends on BCM63XX
>>> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
>>> index 12c7687..11bbce2 100644
>>> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
>>> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
>>> @@ -20,6 +20,7 @@
>>>  #include <linux/mfd/syscon.h>
>>>  #include <linux/platform_device.h>
>>>  #include <linux/regmap.h>
>>> +#include <linux/ctype.h>
>>>  #include <linux/usb/ch9.h>
>>>  #include <linux/usb/gadget.h>
>>>  #include <linux/usb/atmel_usba_udc.h>
>>> @@ -318,6 +319,91 @@ static inline void usba_cleanup_debugfs(struct usba_udc *udc)
>>>  }
>>>  #endif
>>>  
>>> +static ushort fifo_mode;
>>> +
>>> +/* "modprobe ... fifo_mode=1" etc */
>>
>> No need for this comment.
>>
>  
> Ack. I'll remove it.
> 
>>> +module_param(fifo_mode, ushort, 0x0);
>>> +MODULE_PARM_DESC(fifo_mode, "Endpoint configuration mode");
>>> +
>>> +/* mode 0 - uses autoconfig */
>>> +
>>> +/* mode 1 - fits in 8KB, generic max fifo configuration */
>>> +static struct usba_fifo_cfg mode_1_cfg[] = {
>>> +{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
>>> +{ .hw_ep_num = 1, .fifo_size = 1024,	.nr_banks = 2, },
>>> +{ .hw_ep_num = 2, .fifo_size = 1024,	.nr_banks = 1, },
>>> +{ .hw_ep_num = 3, .fifo_size = 1024,	.nr_banks = 1, },
>>> +{ .hw_ep_num = 4, .fifo_size = 1024,	.nr_banks = 1, },
>>> +{ .hw_ep_num = 5, .fifo_size = 1024,	.nr_banks = 1, },
>>> +{ .hw_ep_num = 6, .fifo_size = 1024,	.nr_banks = 1, },
>>> +};
>>> +
>>> +/* mode 2 - fits in 8KB, performance max fifo configuration */
>>> +static struct usba_fifo_cfg mode_2_cfg[] = {
>>> +{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
>>> +{ .hw_ep_num = 1, .fifo_size = 1024,	.nr_banks = 3, },
>>> +{ .hw_ep_num = 2, .fifo_size = 1024,	.nr_banks = 2, },
>>> +{ .hw_ep_num = 3, .fifo_size = 1024,	.nr_banks = 2, },
>>> +};
>>> +
>>> +/* mode 3 - fits in 8KB, mixed fifo configuration */
>>> +static struct usba_fifo_cfg mode_3_cfg[] = {
>>> +{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
>>> +{ .hw_ep_num = 1, .fifo_size = 1024,	.nr_banks = 2, },
>>> +{ .hw_ep_num = 2, .fifo_size = 512,	.nr_banks = 2, },
>>> +{ .hw_ep_num = 3, .fifo_size = 512,	.nr_banks = 2, },
>>> +{ .hw_ep_num = 4, .fifo_size = 512,	.nr_banks = 2, },
>>> +{ .hw_ep_num = 5, .fifo_size = 512,	.nr_banks = 2, },
>>> +{ .hw_ep_num = 6, .fifo_size = 512,	.nr_banks = 2, },
>>> +};
>>> +
>>> +/* mode 4 - fits in 8KB, custom fifo configuration */
>>> +static struct usba_fifo_cfg mode_4_cfg[] = {
>>> +{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
>>> +{ .hw_ep_num = 1, .fifo_size = 512,	.nr_banks = 2, },
>>> +{ .hw_ep_num = 2, .fifo_size = 512,	.nr_banks = 2, },
>>> +{ .hw_ep_num = 3, .fifo_size = 8,	.nr_banks = 2, },
>>> +{ .hw_ep_num = 4, .fifo_size = 512,	.nr_banks = 2, },
>>> +{ .hw_ep_num = 5, .fifo_size = 512,	.nr_banks = 2, },
>>> +{ .hw_ep_num = 6, .fifo_size = 16,	.nr_banks = 2, },
>>> +{ .hw_ep_num = 7, .fifo_size = 8,	.nr_banks = 2, },
>>> +{ .hw_ep_num = 8, .fifo_size = 8,	.nr_banks = 2, },
>>> +};
>>> +/* Add additional configurations here */
>>> +
>>> +int usba_config_fifo_table(struct usba_udc *udc)
>>
>> Isn't is static?
>>
> 
> Ack. I'm going to make it static.
> 
>>> +{
>>> +	int n;
>>> +
>>> +	switch (fifo_mode) {
>>> +	default:
>>> +		fifo_mode = 0;
>>> +	case 0:
>>> +		udc->fifo_cfg = NULL;
>>> +		n = 0;
>>> +		break;
>>> +	case 1:
>>> +		udc->fifo_cfg = mode_1_cfg;
>>> +		n = ARRAY_SIZE(mode_1_cfg);
>>> +		break;
>>> +	case 2:
>>> +		udc->fifo_cfg = mode_2_cfg;
>>> +		n = ARRAY_SIZE(mode_2_cfg);
>>> +		break;
>>> +	case 3:
>>> +		udc->fifo_cfg = mode_3_cfg;
>>> +		n = ARRAY_SIZE(mode_3_cfg);
>>> +		break;
>>> +	case 4:
>>> +		udc->fifo_cfg = mode_4_cfg;
>>> +		n = ARRAY_SIZE(mode_4_cfg);
>>> +		break;
>>> +	}
>>> +	DBG(DBG_HW, "Setup fifo_mode %d\n", fifo_mode);
>>> +
>>> +	return n;
>>> +}
>>> +
>>>  static inline u32 usba_int_enb_get(struct usba_udc *udc)
>>>  {
>>>  	return udc->int_enb_cache;
>>> @@ -543,24 +629,17 @@ usba_ep_enable(struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
>>>  	ep->is_isoc = 0;
>>>  	ep->is_in = 0;
>>>  
>>> -	if (maxpacket <= 8)
>>> -		ept_cfg = USBA_BF(EPT_SIZE, USBA_EPT_SIZE_8);
>>> -	else
>>> -		/* LSB is bit 1, not 0 */
>>> -		ept_cfg = USBA_BF(EPT_SIZE, fls(maxpacket - 1) - 3);
>>> -
>>> -	DBG(DBG_HW, "%s: EPT_SIZE = %lu (maxpacket = %lu)\n",
>>> +	DBG(DBG_ERR, "%s: EPT_CFG = 0x%lx (maxpacket = %lu)\n",
>>>  			ep->ep.name, ept_cfg, maxpacket);
>>>  
>>>  	if (usb_endpoint_dir_in(desc)) {
>>>  		ep->is_in = 1;
>>> -		ept_cfg |= USBA_EPT_DIR_IN;
>>> +		ep->ept_cfg |= USBA_EPT_DIR_IN;
>>>  	}
>>>  
>>>  	switch (usb_endpoint_type(desc)) {
>>>  	case USB_ENDPOINT_XFER_CONTROL:
>>> -		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_CONTROL);
>>> -		ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_ONE);
>>> +		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_CONTROL);
>>>  		break;
>>>  	case USB_ENDPOINT_XFER_ISOC:
>>>  		if (!ep->can_isoc) {
>>> @@ -578,24 +657,15 @@ usba_ep_enable(struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
>>>  			return -EINVAL;
>>>  
>>>  		ep->is_isoc = 1;
>>> -		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_ISO);
>>> +		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_ISO);
>>> +		ep->ept_cfg |= USBA_BF(NB_TRANS, nr_trans);
>>>  
>>> -		/*
>>> -		 * Do triple-buffering on high-bandwidth iso endpoints.
>>> -		 */
>>> -		if (nr_trans > 1 && ep->nr_banks == 3)
>>> -			ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_TRIPLE);
>>> -		else
>>> -			ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_DOUBLE);
>>> -		ept_cfg |= USBA_BF(NB_TRANS, nr_trans);
>>>  		break;
>>>  	case USB_ENDPOINT_XFER_BULK:
>>> -		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_BULK);
>>> -		ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_DOUBLE);
>>> +		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_BULK);
>>>  		break;
>>>  	case USB_ENDPOINT_XFER_INT:
>>> -		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_INT);
>>> -		ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_DOUBLE);
>>> +		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_INT);
>>>  		break;
>>>  	}
>>>  
>>> @@ -604,7 +674,7 @@ usba_ep_enable(struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
>>>  	ep->ep.desc = desc;
>>>  	ep->ep.maxpacket = maxpacket;
>>>  
>>> -	usba_ep_writel(ep, CFG, ept_cfg);
>>> +	usba_ep_writel(ep, CFG, ep->ept_cfg);
>>>  	usba_ep_writel(ep, CTL_ENB, USBA_EPT_ENABLE);
>>>  
>>>  	if (ep->can_dma) {
>>> @@ -1006,12 +1076,81 @@ static int atmel_usba_start(struct usb_gadget *gadget,
>>>  		struct usb_gadget_driver *driver);
>>>  static int atmel_usba_stop(struct usb_gadget *gadget);
>>>  
>>> +static struct usb_ep *atmel_usba_match_ep(
>>
>> I find the indentation of this funciton wreird...
>>
>>> +		struct usb_gadget		*gadget,
>>
>> No need for a tab here and align with open parenthesis,
>>
> 
> Ack.
> 
>>> +		struct usb_endpoint_descriptor	*desc,
>>> +		struct usb_ss_ep_comp_descriptor *ep_comp
>>> +)
>>
>> This parenthesis seems weird to me: put it after the last parameter.
>>
> 
> Ack.
>  
>>> +{
>>> +	struct usb_ep	*_ep;
>>> +	struct usba_ep *ep;
>>> +
>>> +	/* Look at endpoints until an unclaimed one looks usable */
>>> +	list_for_each_entry(_ep, &gadget->ep_list, ep_list) {
>>> +		if (usb_gadget_ep_match_desc(gadget, _ep, desc, ep_comp))
>>> +			goto found_ep;
>>> +	}
>>> +	/* Fail */
>>> +	return NULL;
>>> +
>>> +found_ep:
>>> +
>>> +	if (fifo_mode == 0) {
>>> +		/* Optimize hw fifo size based on ep type and other info */
>>> +		ep = to_usba_ep(_ep);
>>> +
>>> +		switch (usb_endpoint_type(desc)) {
>>> +
>>> +		case USB_ENDPOINT_XFER_CONTROL:
>>> +			break;
>>> +
>>> +		case USB_ENDPOINT_XFER_ISOC:
>>> +			ep->fifo_size = 1024;
>>> +			ep->nr_banks = 2;
>>> +			break;
>>> +
>>> +		case USB_ENDPOINT_XFER_BULK:
>>> +			ep->fifo_size = 512;
>>> +			ep->nr_banks = 1;
>>> +			break;
>>> +
>>> +		case USB_ENDPOINT_XFER_INT:
>>> +			if (desc->wMaxPacketSize == 0)
>>> +				ep->fifo_size =
>>> +				    roundup_pow_of_two(_ep->maxpacket_limit);
>>> +			else
>>> +				ep->fifo_size =
>>> +				    roundup_pow_of_two(le16_to_cpu(desc->wMaxPacketSize));
>>> +			ep->nr_banks = 1;
>>> +			break;
>>> +		}
>>> +
>>> +		/* It might be a little bit late to set this */
>>
>> Is it too late or not? This comment is frightening... We may need some
>> feedback from the USB guys here...
>>
> 
> If someone from USB can comment a bit on this topic I will be grateful.
> 
> The idea here is to optimize the endpoint size used by usb gadgets even if they are created
> at runtime using configfs. Calling usb_ep_set_maxpacket_limit() in probe is too early for
> the size of the endpoint used by the above gadget function to be known. Setting this
> value here it might be a bit late only if the above layer is already using internally the
> maxpacket value configured in probe. From my tests on CDC ACM, RNDIS and HID this is not
> a problem. I added the message as a warning.

Ok, indeed can be good to have feedback from Felipe or Greg.

>>> +		usb_ep_set_maxpacket_limit(&ep->ep, ep->fifo_size);
>>> +
>>> +		/* Generate ept_cfg basd on FIFO size and number of banks */
>>> +		if (ep->fifo_size  <= 8)
>>> +			ep->ept_cfg = USBA_BF(EPT_SIZE, USBA_EPT_SIZE_8);
>>> +		else
>>> +			/* LSB is bit 1, not 0 */
>>> +			ep->ept_cfg =
>>> +				USBA_BF(EPT_SIZE, fls(ep->fifo_size - 1) - 3);
>>> +
>>> +		ep->ept_cfg |= USBA_BF(BK_NUMBER, ep->nr_banks);
>>> +		ep->udc->configured_ep++;
>>
>> is it more than a boolean value?
>>
> 
> Yes. It is more than a boolean value. It counts the number of configured
> endpoints by atmel_usba_match_ep() function that is used as autoconfigure
> mechanism.
> 
>>> +	}
>>> +
>>> +return _ep;
>>
>> Indentation issue here ^
>>
> 
> Ack.
> 
>>> +}
>>> +
>>>  static const struct usb_gadget_ops usba_udc_ops = {
>>>  	.get_frame		= usba_udc_get_frame,
>>>  	.wakeup			= usba_udc_wakeup,
>>>  	.set_selfpowered	= usba_udc_set_selfpowered,
>>>  	.udc_start		= atmel_usba_start,
>>>  	.udc_stop		= atmel_usba_stop,
>>> +	.match_ep		= atmel_usba_match_ep,
>>>  };
>>>  
>>>  static struct usb_endpoint_descriptor usba_ep0_desc = {
>>> @@ -1678,7 +1817,8 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
>>>  	}
>>>  
>>>  	if (status & USBA_END_OF_RESET) {
>>> -		struct usba_ep *ep0;
>>> +		struct usba_ep *ep0, *ep;
>>> +		int i, n;
>>>  
>>>  		usba_writel(udc, INT_CLR, USBA_END_OF_RESET);
>>>  		generate_bias_pulse(udc);
>>> @@ -1717,6 +1857,16 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
>>>  		if (!(usba_ep_readl(ep0, CFG) & USBA_EPT_MAPPED))
>>>  			dev_dbg(&udc->pdev->dev,
>>>  				 "ODD: EP0 configuration is invalid!\n");
>>> +
>>> +		/* Preallocate other endpoints */
>>> +		n = fifo_mode ? udc->num_ep : udc->configured_ep;
>>> +		for (i = 1; i < n; i++) {
>>> +			ep = &udc->usba_ep[i];
>>> +			usba_ep_writel(ep, CFG, ep->ept_cfg);
>>> +			if (!(usba_ep_readl(ep, CFG) & USBA_EPT_MAPPED))
>>> +				dev_dbg(&udc->pdev->dev,
>>> +				 "ODD: EP%d configuration is invalid!\n", i);
>>
>> I know that it's the case for the EP0 just above and that we do it like
>> this in the current driver but maybe we can do more than just having a
>> debug message.
>>
> 
> As you said the debug message follows the pattern that was already in the driver.
> I never stumbled upon this corner case but for now I would like to keep the print
> here to avoid any silent failure. If the USBA_EPT_MAPPED flag is not raised by
> hardware it means that internal memory allocation in the controller went wrong (most
> of the time due to not enough memory, caused by too many / large endpoints). It's
> hard to imagine a solution that can fix this situation without reinitializing the
> usba controller. This can be fixed in a later patch if it's needed.

dev_dbg is pretty much silent all the time. dev_warn() can certainly be
considered. Then in addition to this warning, a return value that could
signal the issue on upper layers is certainly a good idea. But as you
said, certainly the topic of another patch/patch series.

> 
>>> +		}
>>>  	}
>>>  
>>>  	spin_unlock(&udc->lock);
>>> @@ -1864,6 +2014,9 @@ static int atmel_usba_stop(struct usb_gadget *gadget)
>>>  	if (gpio_is_valid(udc->vbus_pin))
>>>  		disable_irq(gpio_to_irq(udc->vbus_pin));
>>>  
>>> +	if (fifo_mode == 0)
>>> +		udc->configured_ep = 1;
>>> +
>>>  	usba_stop(udc);
>>>  
>>>  	udc->driver = NULL;
>>> @@ -1931,9 +2084,13 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
>>>  						&flags);
>>>  	udc->vbus_pin_inverted = (flags & OF_GPIO_ACTIVE_LOW) ? 1 : 0;
>>>  
>>> -	pp = NULL;
>>> -	while ((pp = of_get_next_child(np, pp)))
>>> -		udc->num_ep++;
>>> +	if (fifo_mode == 0) {
>>> +		pp = NULL;
>>> +		while ((pp = of_get_next_child(np, pp)))
>>> +			udc->num_ep++;
>>> +		udc->configured_ep = 1;
>>> +	} else
>>
>> Braces here
>>
> 
> Ack. Will fix.
> 
>>> +		udc->num_ep = usba_config_fifo_table(udc);
>>>  
>>>  	eps = devm_kzalloc(&pdev->dev, sizeof(struct usba_ep) * udc->num_ep,
>>>  			   GFP_KERNEL);
>>> @@ -1946,7 +2103,7 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
>>>  
>>>  	pp = NULL;
>>>  	i = 0;
>>> -	while ((pp = of_get_next_child(np, pp))) {
>>> +	while ((pp = of_get_next_child(np, pp)) && i < udc->num_ep) {
>>>  		ep = &eps[i];
>>>  
>>>  		ret = of_property_read_u32(pp, "reg", &val);
>>> @@ -1954,21 +2111,21 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
>>>  			dev_err(&pdev->dev, "of_probe: reg error(%d)\n", ret);
>>>  			goto err;
>>>  		}
>>> -		ep->index = val;
>>> +		ep->index = fifo_mode ? udc->fifo_cfg[i].hw_ep_num : val;
>>>  
>>>  		ret = of_property_read_u32(pp, "atmel,fifo-size", &val);
>>>  		if (ret) {
>>>  			dev_err(&pdev->dev, "of_probe: fifo-size error(%d)\n", ret);
>>>  			goto err;
>>>  		}
>>> -		ep->fifo_size = val;
>>> +		ep->fifo_size = fifo_mode ? udc->fifo_cfg[i].fifo_size : val;
>>
>> No, this is where I would like to see that the value from the table is
>> still validated agains the "max value" that is stored in HW description/DT.
>> I know that all of our product are able to handle 1024 fifo size, but
>> it's more a savfe check...
>>
> 
> Ack. I'll validate against "max value".
>  
>>>  
>>>  		ret = of_property_read_u32(pp, "atmel,nb-banks", &val);
>>>  		if (ret) {
>>>  			dev_err(&pdev->dev, "of_probe: nb-banks error(%d)\n", ret);
>>>  			goto err;
>>>  		}
>>> -		ep->nr_banks = val;
>>> +		ep->nr_banks = fifo_mode ? udc->fifo_cfg[i].nr_banks : val;
>>
>> Here however, it is pretty important to check this value. For instance,
>> notice that the at91sam9x5 cannot fullfil the "mode 2" configuration: we
>> can't silentely ignore the HW specifications given by DT.
>>
> 
> Ack. Good catch. Although at91sam9x5 has a slightly different name for the usb
> device controller in the reference manual, it is basically the same ip. 
> 
>>>  		ep->can_dma = of_property_read_bool(pp, "atmel,can-dma");
>>>  		ep->can_isoc = of_property_read_bool(pp, "atmel,can-isoc");
>>> @@ -2000,6 +2157,21 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
>>>  		ep->ep.caps.dir_in = true;
>>>  		ep->ep.caps.dir_out = true;
>>>  
>>> +		if (fifo_mode != 0) {
>>> +			/*
>>> +			 * Generate ept_cfg based on FIFO size and
>>> +			 * banks number
>>> +			 */
>>> +			if (ep->fifo_size  <= 8)
>>> +				ep->ept_cfg = USBA_BF(EPT_SIZE, USBA_EPT_SIZE_8);
>>> +			else
>>> +				/* LSB is bit 1, not 0 */
>>> +				ep->ept_cfg =
>>> +				  USBA_BF(EPT_SIZE, fls(ep->fifo_size - 1) - 3);
>>> +
>>> +			ep->ept_cfg |= USBA_BF(BK_NUMBER, ep->nr_banks);
>>> +		}
>>> +
>>>  		if (i)
>>>  			list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
>>>  
>>> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.h b/drivers/usb/gadget/udc/atmel_usba_udc.h
>>> index b03b2eb..9551b70 100644
>>> --- a/drivers/usb/gadget/udc/atmel_usba_udc.h
>>> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.h
>>> @@ -275,6 +275,12 @@ struct usba_dma_desc {
>>>  	u32 ctrl;
>>>  };
>>>  
>>> +struct usba_fifo_cfg {
>>> +	u8			hw_ep_num;
>>> +	u16			fifo_size;
>>> +	u8			nr_banks;
>>> +};
>>> +
>>>  struct usba_ep {
>>>  	int					state;
>>>  	void __iomem				*ep_regs;
>>> @@ -293,7 +299,7 @@ struct usba_ep {
>>>  	unsigned int				can_isoc:1;
>>>  	unsigned int				is_isoc:1;
>>>  	unsigned int				is_in:1;
>>> -
>>> +	unsigned long				ept_cfg;
>>>  #ifdef CONFIG_USB_GADGET_DEBUG_FS
>>>  	u32					last_dma_status;
>>>  	struct dentry				*debugfs_dir;
>>> @@ -338,6 +344,8 @@ struct usba_udc {
>>>  	int vbus_pin;
>>>  	int vbus_pin_inverted;
>>>  	int num_ep;
>>> +	int configured_ep;
>>> +	struct usba_fifo_cfg *fifo_cfg;
>>>  	struct clk *pclk;
>>>  	struct clk *hclk;
>>>  	struct usba_ep *usba_ep;

Regards,

-- 
Nicolas Ferre

Re: [PATCH linux-next v2 1/1] usb: gadget: udc: atmel: Update endpoint allocation scheme

[Nicolas Ferre]

Le 27/01/2017 à 14:47, Cristian Birsan a écrit :
> I will send a fixup patch with updates based on the comments received from Nicolas.
> 
> On 01/26/2017 05:02 PM, Nicolas Ferre wrote:
>> Le 23/01/2017 à 15:45, cristian.birsan@microchip.com a écrit :
>>> From: Cristian Birsan <cristian.birsan@microchip.com>
>>>
>>> Update atmel udc driver with a new enpoint allocation scheme. The data
>>> sheet requires that all endpoints are allocated in order.
>>
>> Pieces of information from the cover letter could have been added here.
>>
> 
> Ack. I'll remove the cover letter.
> 
>>> Signed-off-by: Cristian Birsan <cristian.birsan@microchip.com>
>>> ---
>>>  drivers/usb/gadget/udc/Kconfig          |  14 ++
>>>  drivers/usb/gadget/udc/atmel_usba_udc.c | 236 +++++++++++++++++++++++++++-----
>>>  drivers/usb/gadget/udc/atmel_usba_udc.h |  10 +-
>>>  3 files changed, 227 insertions(+), 33 deletions(-)
>>
>> It can be good to run ./scripts/checkpatch.pl --strict as well. It would
>> have told you about some of the alignment and braces issues. I ran it as
>> some the code has looked weird to me once or twice...
>>
>>
> 
> Ack.
> 
>>> diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
>>> index 658b8da..4b69f28 100644
>>> --- a/drivers/usb/gadget/udc/Kconfig
>>> +++ b/drivers/usb/gadget/udc/Kconfig
>>> @@ -60,6 +60,20 @@ config USB_ATMEL_USBA
>>>  	  USBA is the integrated high-speed USB Device controller on
>>>  	  the AT32AP700x, some AT91SAM9 and AT91CAP9 processors from Atmel.
>>>  
>>> +	  The fifo_mode parameter is used to select endpoint allocation mode.
>>> +	  fifo_mode = 0 is used to let the driver autoconfigure the endpoints.
>>> +	  In this case 2 banks are allocated for isochronous endpoints and
>>
>> You mean that 2 banks can do isochronous xfers if needed, but they can
>> do other types as well, right? So maybe rephrase the sentence.
>>
> 
> For fifo_mode = 0 (autoconfigure case) the driver allocates 2 banks only for
> isochronous endpoints as required by the reference manual. For other endpoints
> it will allocate only 1 bank because it cannot know in advance the number of
> endpoints and the size of them, especially if the configfs is used to create
> the gadget. The idea is to support as many endpoints as we can in this mode.

You know that I know this. It was just a remark about the wording of
your sentence. "are allocated for isochronous" can be turned into "are
allocated so that they could be use as isochronous endpoints". Not a big
deal though.


>>> +	  only one bank is allocated for the rest of the endpoints.
>>> +
>>> +	  fifo_mode = 1 is a generic maximum fifo size (1024 bytes) configuration
>>> +	  allowing the usage of ep1 - ep6
>>> +
>>> +	  fifo_mode = 2 is a generic performance maximum fifo size (1024 bytes)
>>> +	  configuration allowing the usage of ep1 - ep3
>>> +
>>> +	  fifo_mode = 3 is a balanced performance configuration allowing the
>>> +	  the usage of ep1 - ep8
>>> +
>>>  config USB_BCM63XX_UDC
>>>  	tristate "Broadcom BCM63xx Peripheral Controller"
>>>  	depends on BCM63XX
>>> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
>>> index 12c7687..11bbce2 100644
>>> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
>>> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
>>> @@ -20,6 +20,7 @@
>>>  #include <linux/mfd/syscon.h>
>>>  #include <linux/platform_device.h>
>>>  #include <linux/regmap.h>
>>> +#include <linux/ctype.h>
>>>  #include <linux/usb/ch9.h>
>>>  #include <linux/usb/gadget.h>
>>>  #include <linux/usb/atmel_usba_udc.h>
>>> @@ -318,6 +319,91 @@ static inline void usba_cleanup_debugfs(struct usba_udc *udc)
>>>  }
>>>  #endif
>>>  
>>> +static ushort fifo_mode;
>>> +
>>> +/* "modprobe ... fifo_mode=1" etc */
>>
>> No need for this comment.
>>
>  
> Ack. I'll remove it.
> 
>>> +module_param(fifo_mode, ushort, 0x0);
>>> +MODULE_PARM_DESC(fifo_mode, "Endpoint configuration mode");
>>> +
>>> +/* mode 0 - uses autoconfig */
>>> +
>>> +/* mode 1 - fits in 8KB, generic max fifo configuration */
>>> +static struct usba_fifo_cfg mode_1_cfg[] = {
>>> +{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
>>> +{ .hw_ep_num = 1, .fifo_size = 1024,	.nr_banks = 2, },
>>> +{ .hw_ep_num = 2, .fifo_size = 1024,	.nr_banks = 1, },
>>> +{ .hw_ep_num = 3, .fifo_size = 1024,	.nr_banks = 1, },
>>> +{ .hw_ep_num = 4, .fifo_size = 1024,	.nr_banks = 1, },
>>> +{ .hw_ep_num = 5, .fifo_size = 1024,	.nr_banks = 1, },
>>> +{ .hw_ep_num = 6, .fifo_size = 1024,	.nr_banks = 1, },
>>> +};
>>> +
>>> +/* mode 2 - fits in 8KB, performance max fifo configuration */
>>> +static struct usba_fifo_cfg mode_2_cfg[] = {
>>> +{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
>>> +{ .hw_ep_num = 1, .fifo_size = 1024,	.nr_banks = 3, },
>>> +{ .hw_ep_num = 2, .fifo_size = 1024,	.nr_banks = 2, },
>>> +{ .hw_ep_num = 3, .fifo_size = 1024,	.nr_banks = 2, },
>>> +};
>>> +
>>> +/* mode 3 - fits in 8KB, mixed fifo configuration */
>>> +static struct usba_fifo_cfg mode_3_cfg[] = {
>>> +{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
>>> +{ .hw_ep_num = 1, .fifo_size = 1024,	.nr_banks = 2, },
>>> +{ .hw_ep_num = 2, .fifo_size = 512,	.nr_banks = 2, },
>>> +{ .hw_ep_num = 3, .fifo_size = 512,	.nr_banks = 2, },
>>> +{ .hw_ep_num = 4, .fifo_size = 512,	.nr_banks = 2, },
>>> +{ .hw_ep_num = 5, .fifo_size = 512,	.nr_banks = 2, },
>>> +{ .hw_ep_num = 6, .fifo_size = 512,	.nr_banks = 2, },
>>> +};
>>> +
>>> +/* mode 4 - fits in 8KB, custom fifo configuration */
>>> +static struct usba_fifo_cfg mode_4_cfg[] = {
>>> +{ .hw_ep_num = 0, .fifo_size = 64,	.nr_banks = 1, },
>>> +{ .hw_ep_num = 1, .fifo_size = 512,	.nr_banks = 2, },
>>> +{ .hw_ep_num = 2, .fifo_size = 512,	.nr_banks = 2, },
>>> +{ .hw_ep_num = 3, .fifo_size = 8,	.nr_banks = 2, },
>>> +{ .hw_ep_num = 4, .fifo_size = 512,	.nr_banks = 2, },
>>> +{ .hw_ep_num = 5, .fifo_size = 512,	.nr_banks = 2, },
>>> +{ .hw_ep_num = 6, .fifo_size = 16,	.nr_banks = 2, },
>>> +{ .hw_ep_num = 7, .fifo_size = 8,	.nr_banks = 2, },
>>> +{ .hw_ep_num = 8, .fifo_size = 8,	.nr_banks = 2, },
>>> +};
>>> +/* Add additional configurations here */
>>> +
>>> +int usba_config_fifo_table(struct usba_udc *udc)
>>
>> Isn't is static?
>>
> 
> Ack. I'm going to make it static.
> 
>>> +{
>>> +	int n;
>>> +
>>> +	switch (fifo_mode) {
>>> +	default:
>>> +		fifo_mode = 0;
>>> +	case 0:
>>> +		udc->fifo_cfg = NULL;
>>> +		n = 0;
>>> +		break;
>>> +	case 1:
>>> +		udc->fifo_cfg = mode_1_cfg;
>>> +		n = ARRAY_SIZE(mode_1_cfg);
>>> +		break;
>>> +	case 2:
>>> +		udc->fifo_cfg = mode_2_cfg;
>>> +		n = ARRAY_SIZE(mode_2_cfg);
>>> +		break;
>>> +	case 3:
>>> +		udc->fifo_cfg = mode_3_cfg;
>>> +		n = ARRAY_SIZE(mode_3_cfg);
>>> +		break;
>>> +	case 4:
>>> +		udc->fifo_cfg = mode_4_cfg;
>>> +		n = ARRAY_SIZE(mode_4_cfg);
>>> +		break;
>>> +	}
>>> +	DBG(DBG_HW, "Setup fifo_mode %d\n", fifo_mode);
>>> +
>>> +	return n;
>>> +}
>>> +
>>>  static inline u32 usba_int_enb_get(struct usba_udc *udc)
>>>  {
>>>  	return udc->int_enb_cache;
>>> @@ -543,24 +629,17 @@ usba_ep_enable(struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
>>>  	ep->is_isoc = 0;
>>>  	ep->is_in = 0;
>>>  
>>> -	if (maxpacket <= 8)
>>> -		ept_cfg = USBA_BF(EPT_SIZE, USBA_EPT_SIZE_8);
>>> -	else
>>> -		/* LSB is bit 1, not 0 */
>>> -		ept_cfg = USBA_BF(EPT_SIZE, fls(maxpacket - 1) - 3);
>>> -
>>> -	DBG(DBG_HW, "%s: EPT_SIZE = %lu (maxpacket = %lu)\n",
>>> +	DBG(DBG_ERR, "%s: EPT_CFG = 0x%lx (maxpacket = %lu)\n",
>>>  			ep->ep.name, ept_cfg, maxpacket);
>>>  
>>>  	if (usb_endpoint_dir_in(desc)) {
>>>  		ep->is_in = 1;
>>> -		ept_cfg |= USBA_EPT_DIR_IN;
>>> +		ep->ept_cfg |= USBA_EPT_DIR_IN;
>>>  	}
>>>  
>>>  	switch (usb_endpoint_type(desc)) {
>>>  	case USB_ENDPOINT_XFER_CONTROL:
>>> -		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_CONTROL);
>>> -		ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_ONE);
>>> +		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_CONTROL);
>>>  		break;
>>>  	case USB_ENDPOINT_XFER_ISOC:
>>>  		if (!ep->can_isoc) {
>>> @@ -578,24 +657,15 @@ usba_ep_enable(struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
>>>  			return -EINVAL;
>>>  
>>>  		ep->is_isoc = 1;
>>> -		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_ISO);
>>> +		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_ISO);
>>> +		ep->ept_cfg |= USBA_BF(NB_TRANS, nr_trans);
>>>  
>>> -		/*
>>> -		 * Do triple-buffering on high-bandwidth iso endpoints.
>>> -		 */
>>> -		if (nr_trans > 1 && ep->nr_banks == 3)
>>> -			ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_TRIPLE);
>>> -		else
>>> -			ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_DOUBLE);
>>> -		ept_cfg |= USBA_BF(NB_TRANS, nr_trans);
>>>  		break;
>>>  	case USB_ENDPOINT_XFER_BULK:
>>> -		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_BULK);
>>> -		ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_DOUBLE);
>>> +		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_BULK);
>>>  		break;
>>>  	case USB_ENDPOINT_XFER_INT:
>>> -		ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_INT);
>>> -		ept_cfg |= USBA_BF(BK_NUMBER, USBA_BK_NUMBER_DOUBLE);
>>> +		ep->ept_cfg |= USBA_BF(EPT_TYPE, USBA_EPT_TYPE_INT);
>>>  		break;
>>>  	}
>>>  
>>> @@ -604,7 +674,7 @@ usba_ep_enable(struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
>>>  	ep->ep.desc = desc;
>>>  	ep->ep.maxpacket = maxpacket;
>>>  
>>> -	usba_ep_writel(ep, CFG, ept_cfg);
>>> +	usba_ep_writel(ep, CFG, ep->ept_cfg);
>>>  	usba_ep_writel(ep, CTL_ENB, USBA_EPT_ENABLE);
>>>  
>>>  	if (ep->can_dma) {
>>> @@ -1006,12 +1076,81 @@ static int atmel_usba_start(struct usb_gadget *gadget,
>>>  		struct usb_gadget_driver *driver);
>>>  static int atmel_usba_stop(struct usb_gadget *gadget);
>>>  
>>> +static struct usb_ep *atmel_usba_match_ep(
>>
>> I find the indentation of this funciton wreird...
>>
>>> +		struct usb_gadget		*gadget,
>>
>> No need for a tab here and align with open parenthesis,
>>
> 
> Ack.
> 
>>> +		struct usb_endpoint_descriptor	*desc,
>>> +		struct usb_ss_ep_comp_descriptor *ep_comp
>>> +)
>>
>> This parenthesis seems weird to me: put it after the last parameter.
>>
> 
> Ack.
>  
>>> +{
>>> +	struct usb_ep	*_ep;
>>> +	struct usba_ep *ep;
>>> +
>>> +	/* Look at endpoints until an unclaimed one looks usable */
>>> +	list_for_each_entry(_ep, &gadget->ep_list, ep_list) {
>>> +		if (usb_gadget_ep_match_desc(gadget, _ep, desc, ep_comp))
>>> +			goto found_ep;
>>> +	}
>>> +	/* Fail */
>>> +	return NULL;
>>> +
>>> +found_ep:
>>> +
>>> +	if (fifo_mode == 0) {
>>> +		/* Optimize hw fifo size based on ep type and other info */
>>> +		ep = to_usba_ep(_ep);
>>> +
>>> +		switch (usb_endpoint_type(desc)) {
>>> +
>>> +		case USB_ENDPOINT_XFER_CONTROL:
>>> +			break;
>>> +
>>> +		case USB_ENDPOINT_XFER_ISOC:
>>> +			ep->fifo_size = 1024;
>>> +			ep->nr_banks = 2;
>>> +			break;
>>> +
>>> +		case USB_ENDPOINT_XFER_BULK:
>>> +			ep->fifo_size = 512;
>>> +			ep->nr_banks = 1;
>>> +			break;
>>> +
>>> +		case USB_ENDPOINT_XFER_INT:
>>> +			if (desc->wMaxPacketSize == 0)
>>> +				ep->fifo_size =
>>> +				    roundup_pow_of_two(_ep->maxpacket_limit);
>>> +			else
>>> +				ep->fifo_size =
>>> +				    roundup_pow_of_two(le16_to_cpu(desc->wMaxPacketSize));
>>> +			ep->nr_banks = 1;
>>> +			break;
>>> +		}
>>> +
>>> +		/* It might be a little bit late to set this */
>>
>> Is it too late or not? This comment is frightening... We may need some
>> feedback from the USB guys here...
>>
> 
> If someone from USB can comment a bit on this topic I will be grateful.
> 
> The idea here is to optimize the endpoint size used by usb gadgets even if they are created
> at runtime using configfs. Calling usb_ep_set_maxpacket_limit() in probe is too early for
> the size of the endpoint used by the above gadget function to be known. Setting this
> value here it might be a bit late only if the above layer is already using internally the
> maxpacket value configured in probe. From my tests on CDC ACM, RNDIS and HID this is not
> a problem. I added the message as a warning.

Ok, indeed can be good to have feedback from Felipe or Greg.

>>> +		usb_ep_set_maxpacket_limit(&ep->ep, ep->fifo_size);
>>> +
>>> +		/* Generate ept_cfg basd on FIFO size and number of banks */
>>> +		if (ep->fifo_size  <= 8)
>>> +			ep->ept_cfg = USBA_BF(EPT_SIZE, USBA_EPT_SIZE_8);
>>> +		else
>>> +			/* LSB is bit 1, not 0 */
>>> +			ep->ept_cfg =
>>> +				USBA_BF(EPT_SIZE, fls(ep->fifo_size - 1) - 3);
>>> +
>>> +		ep->ept_cfg |= USBA_BF(BK_NUMBER, ep->nr_banks);
>>> +		ep->udc->configured_ep++;
>>
>> is it more than a boolean value?
>>
> 
> Yes. It is more than a boolean value. It counts the number of configured
> endpoints by atmel_usba_match_ep() function that is used as autoconfigure
> mechanism.
> 
>>> +	}
>>> +
>>> +return _ep;
>>
>> Indentation issue here ^
>>
> 
> Ack.
> 
>>> +}
>>> +
>>>  static const struct usb_gadget_ops usba_udc_ops = {
>>>  	.get_frame		= usba_udc_get_frame,
>>>  	.wakeup			= usba_udc_wakeup,
>>>  	.set_selfpowered	= usba_udc_set_selfpowered,
>>>  	.udc_start		= atmel_usba_start,
>>>  	.udc_stop		= atmel_usba_stop,
>>> +	.match_ep		= atmel_usba_match_ep,
>>>  };
>>>  
>>>  static struct usb_endpoint_descriptor usba_ep0_desc = {
>>> @@ -1678,7 +1817,8 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
>>>  	}
>>>  
>>>  	if (status & USBA_END_OF_RESET) {
>>> -		struct usba_ep *ep0;
>>> +		struct usba_ep *ep0, *ep;
>>> +		int i, n;
>>>  
>>>  		usba_writel(udc, INT_CLR, USBA_END_OF_RESET);
>>>  		generate_bias_pulse(udc);
>>> @@ -1717,6 +1857,16 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
>>>  		if (!(usba_ep_readl(ep0, CFG) & USBA_EPT_MAPPED))
>>>  			dev_dbg(&udc->pdev->dev,
>>>  				 "ODD: EP0 configuration is invalid!\n");
>>> +
>>> +		/* Preallocate other endpoints */
>>> +		n = fifo_mode ? udc->num_ep : udc->configured_ep;
>>> +		for (i = 1; i < n; i++) {
>>> +			ep = &udc->usba_ep[i];
>>> +			usba_ep_writel(ep, CFG, ep->ept_cfg);
>>> +			if (!(usba_ep_readl(ep, CFG) & USBA_EPT_MAPPED))
>>> +				dev_dbg(&udc->pdev->dev,
>>> +				 "ODD: EP%d configuration is invalid!\n", i);
>>
>> I know that it's the case for the EP0 just above and that we do it like
>> this in the current driver but maybe we can do more than just having a
>> debug message.
>>
> 
> As you said the debug message follows the pattern that was already in the driver.
> I never stumbled upon this corner case but for now I would like to keep the print
> here to avoid any silent failure. If the USBA_EPT_MAPPED flag is not raised by
> hardware it means that internal memory allocation in the controller went wrong (most
> of the time due to not enough memory, caused by too many / large endpoints). It's
> hard to imagine a solution that can fix this situation without reinitializing the
> usba controller. This can be fixed in a later patch if it's needed.

dev_dbg is pretty much silent all the time. dev_warn() can certainly be
considered. Then in addition to this warning, a return value that could
signal the issue on upper layers is certainly a good idea. But as you
said, certainly the topic of another patch/patch series.

> 
>>> +		}
>>>  	}
>>>  
>>>  	spin_unlock(&udc->lock);
>>> @@ -1864,6 +2014,9 @@ static int atmel_usba_stop(struct usb_gadget *gadget)
>>>  	if (gpio_is_valid(udc->vbus_pin))
>>>  		disable_irq(gpio_to_irq(udc->vbus_pin));
>>>  
>>> +	if (fifo_mode == 0)
>>> +		udc->configured_ep = 1;
>>> +
>>>  	usba_stop(udc);
>>>  
>>>  	udc->driver = NULL;
>>> @@ -1931,9 +2084,13 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
>>>  						&flags);
>>>  	udc->vbus_pin_inverted = (flags & OF_GPIO_ACTIVE_LOW) ? 1 : 0;
>>>  
>>> -	pp = NULL;
>>> -	while ((pp = of_get_next_child(np, pp)))
>>> -		udc->num_ep++;
>>> +	if (fifo_mode == 0) {
>>> +		pp = NULL;
>>> +		while ((pp = of_get_next_child(np, pp)))
>>> +			udc->num_ep++;
>>> +		udc->configured_ep = 1;
>>> +	} else
>>
>> Braces here
>>
> 
> Ack. Will fix.
> 
>>> +		udc->num_ep = usba_config_fifo_table(udc);
>>>  
>>>  	eps = devm_kzalloc(&pdev->dev, sizeof(struct usba_ep) * udc->num_ep,
>>>  			   GFP_KERNEL);
>>> @@ -1946,7 +2103,7 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
>>>  
>>>  	pp = NULL;
>>>  	i = 0;
>>> -	while ((pp = of_get_next_child(np, pp))) {
>>> +	while ((pp = of_get_next_child(np, pp)) && i < udc->num_ep) {
>>>  		ep = &eps[i];
>>>  
>>>  		ret = of_property_read_u32(pp, "reg", &val);
>>> @@ -1954,21 +2111,21 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
>>>  			dev_err(&pdev->dev, "of_probe: reg error(%d)\n", ret);
>>>  			goto err;
>>>  		}
>>> -		ep->index = val;
>>> +		ep->index = fifo_mode ? udc->fifo_cfg[i].hw_ep_num : val;
>>>  
>>>  		ret = of_property_read_u32(pp, "atmel,fifo-size", &val);
>>>  		if (ret) {
>>>  			dev_err(&pdev->dev, "of_probe: fifo-size error(%d)\n", ret);
>>>  			goto err;
>>>  		}
>>> -		ep->fifo_size = val;
>>> +		ep->fifo_size = fifo_mode ? udc->fifo_cfg[i].fifo_size : val;
>>
>> No, this is where I would like to see that the value from the table is
>> still validated agains the "max value" that is stored in HW description/DT.
>> I know that all of our product are able to handle 1024 fifo size, but
>> it's more a savfe check...
>>
> 
> Ack. I'll validate against "max value".
>  
>>>  
>>>  		ret = of_property_read_u32(pp, "atmel,nb-banks", &val);
>>>  		if (ret) {
>>>  			dev_err(&pdev->dev, "of_probe: nb-banks error(%d)\n", ret);
>>>  			goto err;
>>>  		}
>>> -		ep->nr_banks = val;
>>> +		ep->nr_banks = fifo_mode ? udc->fifo_cfg[i].nr_banks : val;
>>
>> Here however, it is pretty important to check this value. For instance,
>> notice that the at91sam9x5 cannot fullfil the "mode 2" configuration: we
>> can't silentely ignore the HW specifications given by DT.
>>
> 
> Ack. Good catch. Although at91sam9x5 has a slightly different name for the usb
> device controller in the reference manual, it is basically the same ip. 
> 
>>>  		ep->can_dma = of_property_read_bool(pp, "atmel,can-dma");
>>>  		ep->can_isoc = of_property_read_bool(pp, "atmel,can-isoc");
>>> @@ -2000,6 +2157,21 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
>>>  		ep->ep.caps.dir_in = true;
>>>  		ep->ep.caps.dir_out = true;
>>>  
>>> +		if (fifo_mode != 0) {
>>> +			/*
>>> +			 * Generate ept_cfg based on FIFO size and
>>> +			 * banks number
>>> +			 */
>>> +			if (ep->fifo_size  <= 8)
>>> +				ep->ept_cfg = USBA_BF(EPT_SIZE, USBA_EPT_SIZE_8);
>>> +			else
>>> +				/* LSB is bit 1, not 0 */
>>> +				ep->ept_cfg =
>>> +				  USBA_BF(EPT_SIZE, fls(ep->fifo_size - 1) - 3);
>>> +
>>> +			ep->ept_cfg |= USBA_BF(BK_NUMBER, ep->nr_banks);
>>> +		}
>>> +
>>>  		if (i)
>>>  			list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
>>>  
>>> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.h b/drivers/usb/gadget/udc/atmel_usba_udc.h
>>> index b03b2eb..9551b70 100644
>>> --- a/drivers/usb/gadget/udc/atmel_usba_udc.h
>>> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.h
>>> @@ -275,6 +275,12 @@ struct usba_dma_desc {
>>>  	u32 ctrl;
>>>  };
>>>  
>>> +struct usba_fifo_cfg {
>>> +	u8			hw_ep_num;
>>> +	u16			fifo_size;
>>> +	u8			nr_banks;
>>> +};
>>> +
>>>  struct usba_ep {
>>>  	int					state;
>>>  	void __iomem				*ep_regs;
>>> @@ -293,7 +299,7 @@ struct usba_ep {
>>>  	unsigned int				can_isoc:1;
>>>  	unsigned int				is_isoc:1;
>>>  	unsigned int				is_in:1;
>>> -
>>> +	unsigned long				ept_cfg;
>>>  #ifdef CONFIG_USB_GADGET_DEBUG_FS
>>>  	u32					last_dma_status;
>>>  	struct dentry				*debugfs_dir;
>>> @@ -338,6 +344,8 @@ struct usba_udc {
>>>  	int vbus_pin;
>>>  	int vbus_pin_inverted;
>>>  	int num_ep;
>>> +	int configured_ep;
>>> +	struct usba_fifo_cfg *fifo_cfg;
>>>  	struct clk *pclk;
>>>  	struct clk *hclk;
>>>  	struct usba_ep *usba_ep;

Regards,

-- 
Nicolas Ferre

[PATCH linux-next v2 1/1] usb: gadget: udc: atmel: Update endpoint allocation scheme

[Felipe Balbi]

Hi,

Cristian Birsan <cristian.birsan@microchip.com> writes:
>>> +		/* It might be a little bit late to set this */
>> 
>> Is it too late or not? This comment is frightening... We may need some
>> feedback from the USB guys here...
>>
>
> If someone from USB can comment a bit on this topic I will be grateful.

it is not too late :-) Just look at the implementation of
usb_ep_autoconfig_ss(). That said, I haven't considered if gadget
drivers might want this value to be valid before calling
usb_ep_autoconfig_ss(). I can't see why they would, but still; just a
little warning.

struct usb_ep *usb_ep_autoconfig_ss(
	struct usb_gadget		*gadget,
	struct usb_endpoint_descriptor	*desc,
	struct usb_ss_ep_comp_descriptor *ep_comp
)
{
	struct usb_ep	*ep;
	u8		type;

	type = desc->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK;

	if (gadget->ops->match_ep) {
		ep = gadget->ops->match_ep(gadget, desc, ep_comp);
		if (ep)
			goto found_ep;
	}

	/* Second, look at endpoints until an unclaimed one looks usable */
	list_for_each_entry (ep, &gadget->ep_list, ep_list) {
		if (usb_gadget_ep_match_desc(gadget, ep, desc, ep_comp))
			goto found_ep;
	}

	/* Fail */
	return NULL;
found_ep:

	/*
	 * If the protocol driver hasn't yet decided on wMaxPacketSize
	 * and wants to know the maximum possible, provide the info.
	 */
	if (desc->wMaxPacketSize == 0)
		desc->wMaxPacketSize = cpu_to_le16(ep->maxpacket_limit);

	/* report address */
	desc->bEndpointAddress &= USB_DIR_IN;
	if (isdigit(ep->name[2])) {
		u8 num = simple_strtoul(&ep->name[2], NULL, 10);
		desc->bEndpointAddress |= num;
	} else if (desc->bEndpointAddress & USB_DIR_IN) {
		if (++gadget->in_epnum > 15)
			return NULL;
		desc->bEndpointAddress = USB_DIR_IN | gadget->in_epnum;
	} else {
		if (++gadget->out_epnum > 15)
			return NULL;
		desc->bEndpointAddress |= gadget->out_epnum;
	}

	/* report (variable) full speed bulk maxpacket */
	if ((type == USB_ENDPOINT_XFER_BULK) && !ep_comp) {
		int size = ep->maxpacket_limit;

		/* min() doesn't work on bitfields with gcc-3.5 */
		if (size > 64)
			size = 64;
		desc->wMaxPacketSize = cpu_to_le16(size);
	}

	ep->address = desc->bEndpointAddress;
	ep->desc = NULL;
	ep->comp_desc = NULL;
	ep->claimed = true;
	return ep;
}

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170127/fa9be2a4/attachment.sig>

Re: [PATCH linux-next v2 1/1] usb: gadget: udc: atmel: Update endpoint allocation scheme

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2329 bytes --]


Hi,

Cristian Birsan <cristian.birsan@microchip.com> writes:
>>> +		/* It might be a little bit late to set this */
>> 
>> Is it too late or not? This comment is frightening... We may need some
>> feedback from the USB guys here...
>>
>
> If someone from USB can comment a bit on this topic I will be grateful.

it is not too late :-) Just look at the implementation of
usb_ep_autoconfig_ss(). That said, I haven't considered if gadget
drivers might want this value to be valid before calling
usb_ep_autoconfig_ss(). I can't see why they would, but still; just a
little warning.

struct usb_ep *usb_ep_autoconfig_ss(
	struct usb_gadget		*gadget,
	struct usb_endpoint_descriptor	*desc,
	struct usb_ss_ep_comp_descriptor *ep_comp
)
{
	struct usb_ep	*ep;
	u8		type;

	type = desc->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK;

	if (gadget->ops->match_ep) {
		ep = gadget->ops->match_ep(gadget, desc, ep_comp);
		if (ep)
			goto found_ep;
	}

	/* Second, look at endpoints until an unclaimed one looks usable */
	list_for_each_entry (ep, &gadget->ep_list, ep_list) {
		if (usb_gadget_ep_match_desc(gadget, ep, desc, ep_comp))
			goto found_ep;
	}

	/* Fail */
	return NULL;
found_ep:

	/*
	 * If the protocol driver hasn't yet decided on wMaxPacketSize
	 * and wants to know the maximum possible, provide the info.
	 */
	if (desc->wMaxPacketSize == 0)
		desc->wMaxPacketSize = cpu_to_le16(ep->maxpacket_limit);

	/* report address */
	desc->bEndpointAddress &= USB_DIR_IN;
	if (isdigit(ep->name[2])) {
		u8 num = simple_strtoul(&ep->name[2], NULL, 10);
		desc->bEndpointAddress |= num;
	} else if (desc->bEndpointAddress & USB_DIR_IN) {
		if (++gadget->in_epnum > 15)
			return NULL;
		desc->bEndpointAddress = USB_DIR_IN | gadget->in_epnum;
	} else {
		if (++gadget->out_epnum > 15)
			return NULL;
		desc->bEndpointAddress |= gadget->out_epnum;
	}

	/* report (variable) full speed bulk maxpacket */
	if ((type == USB_ENDPOINT_XFER_BULK) && !ep_comp) {
		int size = ep->maxpacket_limit;

		/* min() doesn't work on bitfields with gcc-3.5 */
		if (size > 64)
			size = 64;
		desc->wMaxPacketSize = cpu_to_le16(size);
	}

	ep->address = desc->bEndpointAddress;
	ep->desc = NULL;
	ep->comp_desc = NULL;
	ep->claimed = true;
	return ep;
}

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]