[RFC v5 0/2] WIP: add usb keyboard driver

[RFC v5 0/2] WIP: add usb keyboard driver

[Peter Mamonov]

Changes since v4:
	- usb: ehci-hcd: detect submit_*_msg() functions re-entrance
	- common: clock: introduce non interruptible version of mdelay() function
	- usb: ehci-hcd: use mdelay_non_inerruptible to avoid re-entrance

Peter Mamonov (5):
	usb: ehci-hcd: port periodic transactions implementation
	usb: ehci-hcd: detect re-entrance
	common: clock: introduce mdelay_non_interruptible()
	usb: ehci-hcd: use mdelay_non_inerruptible()
	input: port usb keyboard driver from the u-boot

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/5] usb: ehci-hcd: port periodic transactions implementation from the u-boot

[Peter Mamonov]

Signed-off-by: Peter Mamonov <pmamonov@gmail.com>
---
 drivers/usb/host/ehci-hcd.c | 400 +++++++++++++++++++++++++++++++++++++++++++-
 drivers/usb/host/ehci.h     |  15 +-
 2 files changed, 413 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index c1c48f4..14bb7d9 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -48,6 +48,18 @@ struct ehci_priv {
 	int (*init)(void *drvdata);
 	int (*post_init)(void *drvdata);
 	void *drvdata;
+	int periodic_schedules;
+	struct QH *periodic_queue;
+	uint32_t *periodic_list;
+};
+
+struct int_queue {
+	int elementsize;
+	unsigned long pipe;
+	struct QH *first;
+	struct QH *current;
+	struct QH *last;
+	struct qTD *tds;
 };
 
 #define to_ehci(ptr) container_of(ptr, struct ehci_priv, host)
@@ -769,6 +781,8 @@ static int ehci_init(struct usb_host *host)
 	uint32_t reg;
 	uint32_t cmd;
 	int ret = 0;
+	struct QH *periodic;
+	int i;
 
 	ehci_halt(ehci);
 
@@ -799,6 +813,44 @@ static int ehci_init(struct usb_host *host)
 		ehci_writel(&ehci->hcor->or_asynclistaddr, ba);
 	}
 
+	/*
+	 * Set up periodic list
+	 * Step 1: Parent QH for all periodic transfers.
+	 */
+	ehci->periodic_schedules = 0;
+	periodic = ehci->periodic_queue;
+	memset(periodic, 0, sizeof(*periodic));
+	periodic->qh_link = cpu_to_hc32(QH_LINK_TERMINATE);
+	periodic->qt_next = cpu_to_hc32(QT_NEXT_TERMINATE);
+	periodic->qt_altnext = cpu_to_hc32(QT_NEXT_TERMINATE);
+
+	/*
+	 * Step 2: Setup frame-list: Every microframe, USB tries the same list.
+	 *         In particular, device specifications on polling frequency
+	 *         are disregarded. Keyboards seem to send NAK/NYet reliably
+	 *         when polled with an empty buffer.
+	 *
+	 *         Split Transactions will be spread across microframes using
+	 *         S-mask and C-mask.
+	 */
+	if (ehci->periodic_list == NULL)
+		/*
+		 * FIXME: this memory chunk have to be 4k aligned AND
+		 * reside in coherent memory. Current implementation of
+		 * dma_alloc_coherent() allocates PAGE_SIZE aligned memory chunks.
+		 * PAGE_SIZE less then 4k will break this code.
+		 */
+		ehci->periodic_list = dma_alloc_coherent(1024 * 4,
+							 DMA_ADDRESS_BROKEN);
+	for (i = 0; i < 1024; i++) {
+		ehci->periodic_list[i] = cpu_to_hc32((unsigned long)periodic
+						| QH_LINK_TYPE_QH);
+	}
+
+	/* Set periodic list base address */
+	ehci_writel(&ehci->hcor->or_periodiclistbase,
+		(unsigned long)ehci->periodic_list);
+
 	reg = ehci_readl(&ehci->hccr->cr_hcsparams);
 	descriptor.hub.bNbrPorts = HCS_N_PORTS(reg);
 
@@ -870,15 +922,359 @@ submit_control_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
 }
 
 static int
+disable_periodic(struct ehci_priv *ehci)
+{
+	uint32_t cmd;
+	struct ehci_hcor *hcor = ehci->hcor;
+	int ret;
+
+	cmd = ehci_readl(&hcor->or_usbcmd);
+	cmd &= ~CMD_PSE;
+	ehci_writel(&hcor->or_usbcmd, cmd);
+
+	ret = handshake((uint32_t *)&hcor->or_usbsts,
+			STS_PSS, 0, 100 * 1000);
+	if (ret < 0) {
+		printf("EHCI failed: timeout when disabling periodic list\n");
+		return -ETIMEDOUT;
+	}
+	return 0;
+}
+
+#define NEXT_QH(qh) (struct QH *)((0xa0000000 | (unsigned long)hc32_to_cpu((qh)->qh_link)) & ~0x1f)
+
+static int
+enable_periodic(struct ehci_priv *ehci)
+{
+	uint32_t cmd;
+	struct ehci_hcor *hcor = ehci->hcor;
+	int ret;
+	uint64_t start;
+
+	cmd = ehci_readl(&hcor->or_usbcmd);
+	cmd |= CMD_PSE;
+	ehci_writel(&hcor->or_usbcmd, cmd);
+
+	ret = handshake((uint32_t *)&hcor->or_usbsts,
+			STS_PSS, STS_PSS, 100 * 1000);
+	if (ret < 0) {
+		printf("EHCI failed: timeout when enabling periodic list\n");
+		return -ETIMEDOUT;
+	}
+
+	start = get_time_ns();
+	while(!is_timeout_non_interruptible(start, 1 * MSECOND));
+
+	return 0;
+}
+
+static inline u8 ehci_encode_speed(enum usb_device_speed speed)
+{
+	#define QH_HIGH_SPEED	2
+	#define QH_FULL_SPEED	0
+	#define QH_LOW_SPEED	1
+	if (speed == USB_SPEED_HIGH)
+		return QH_HIGH_SPEED;
+	if (speed == USB_SPEED_LOW)
+		return QH_LOW_SPEED;
+	return QH_FULL_SPEED;
+}
+
+static void ehci_update_endpt2_dev_n_port(struct usb_device *udev,
+					  struct QH *qh)
+{
+	struct usb_device *ttdev;
+	int parent_devnum;
+
+	if (udev->speed != USB_SPEED_LOW && udev->speed != USB_SPEED_FULL)
+		return;
+
+	/*
+	 * For full / low speed devices we need to get the devnum and portnr of
+	 * the tt, so of the first upstream usb-2 hub, there may be usb-1 hubs
+	 * in the tree before that one!
+	 */
+
+	ttdev = udev;
+	while (ttdev->parent && ttdev->parent->speed != USB_SPEED_HIGH)
+		ttdev = ttdev->parent;
+	if (!ttdev->parent)
+		return;
+	parent_devnum = ttdev->parent->devnum;
+
+	qh->qh_endpt2 |= cpu_to_hc32(QH_ENDPT2_PORTNUM(ttdev->portnr) |
+				     QH_ENDPT2_HUBADDR(parent_devnum));
+}
+
+static struct int_queue *ehci_create_int_queue(struct usb_device *dev,
+			unsigned long pipe, int queuesize, int elementsize,
+			void *buffer, int interval)
+{
+	struct usb_host *host = dev->host;
+	struct ehci_priv *ehci = to_ehci(host);
+	struct int_queue *result = NULL;
+	uint32_t i, toggle;
+	struct QH *list = ehci->periodic_queue;
+
+	/*
+	 * Interrupt transfers requiring several transactions are not supported
+	 * because bInterval is ignored.
+	 *
+	 * Also, ehci_submit_async() relies on wMaxPacketSize being a power of 2
+	 * <= PKT_ALIGN if several qTDs are required, while the USB
+	 * specification does not constrain this for interrupt transfers. That
+	 * means that ehci_submit_async() would support interrupt transfers
+	 * requiring several transactions only as long as the transfer size does
+	 * not require more than a single qTD.
+	 */
+	if (elementsize > usb_maxpacket(dev, pipe)) {
+		dev_err(&dev->dev,
+			"%s: xfers requiring several transactions are not supported.\n",
+			__func__);
+		return NULL;
+	}
+
+	debug("Enter create_int_queue\n");
+	if (usb_pipetype(pipe) != PIPE_INTERRUPT) {
+		dev_dbg(&dev->dev,
+			"non-interrupt pipe (type=%lu)",
+			usb_pipetype(pipe));
+		return NULL;
+	}
+
+	/* limit to 4 full pages worth of data -
+	 * we can safely fit them in a single TD,
+	 * no matter the alignment
+	 */
+	if (elementsize >= 16384) {
+		dev_dbg(&dev->dev,
+			"too large elements for interrupt transfers\n");
+		return NULL;
+	}
+
+	result = xzalloc(sizeof(*result));
+	result->elementsize = elementsize;
+	result->pipe = pipe;
+	result->first = dma_alloc_coherent(sizeof(struct QH) * queuesize,
+					   DMA_ADDRESS_BROKEN);
+	result->current = result->first;
+	result->last = result->first + queuesize - 1;
+	result->tds = dma_alloc_coherent(sizeof(struct qTD) * queuesize,
+					 DMA_ADDRESS_BROKEN);
+	memset(result->first, 0, sizeof(struct QH) * queuesize);
+	memset(result->tds, 0, sizeof(struct qTD) * queuesize);
+
+	toggle = usb_gettoggle(dev, usb_pipeendpoint(pipe), usb_pipeout(pipe));
+
+	for (i = 0; i < queuesize; i++) {
+		struct QH *qh = result->first + i;
+		struct qTD *td = result->tds + i;
+		void **buf = &qh->buffer;
+
+		qh->qh_link = cpu_to_hc32((unsigned long)(qh+1) | QH_LINK_TYPE_QH);
+		if (i == queuesize - 1)
+			qh->qh_link = cpu_to_hc32(QH_LINK_TERMINATE);
+
+		qh->qt_next = cpu_to_hc32((unsigned long)td);
+		qh->qt_altnext = cpu_to_hc32(QT_NEXT_TERMINATE);
+		qh->qh_endpt1 =
+			cpu_to_hc32((0 << 28) | /* No NAK reload (ehci 4.9) */
+			(usb_maxpacket(dev, pipe) << 16) | /* MPS */
+			(1 << 14) |
+			QH_ENDPT1_EPS(ehci_encode_speed(dev->speed)) |
+			(usb_pipeendpoint(pipe) << 8) | /* Endpoint Number */
+			(usb_pipedevice(pipe) << 0));
+		qh->qh_endpt2 = cpu_to_hc32((1 << 30) | /* 1 Tx per mframe */
+			(1 << 0)); /* S-mask: microframe 0 */
+		if (dev->speed == USB_SPEED_LOW ||
+				dev->speed == USB_SPEED_FULL) {
+			/* C-mask: microframes 2-4 */
+			qh->qh_endpt2 |= cpu_to_hc32((0x1c << 8));
+		}
+		ehci_update_endpt2_dev_n_port(dev, qh);
+
+		td->qt_next = cpu_to_hc32(QT_NEXT_TERMINATE);
+		td->qt_altnext = cpu_to_hc32(QT_NEXT_TERMINATE);
+		dev_dbg(&dev->dev,
+			"communication direction is '%s'\n",
+			usb_pipein(pipe) ? "in" : "out");
+		td->qt_token = cpu_to_hc32(
+			QT_TOKEN_DT(toggle) |
+			(elementsize << 16) |
+			((usb_pipein(pipe) ? 1 : 0) << 8) | /* IN/OUT token */
+			0x80); /* active */
+		td->qt_buffer[0] =
+		    cpu_to_hc32((unsigned long)buffer + i * elementsize);
+		td->qt_buffer[1] =
+		    cpu_to_hc32((td->qt_buffer[0] + 0x1000) & ~0xfff);
+		td->qt_buffer[2] =
+		    cpu_to_hc32((td->qt_buffer[0] + 0x2000) & ~0xfff);
+		td->qt_buffer[3] =
+		    cpu_to_hc32((td->qt_buffer[0] + 0x3000) & ~0xfff);
+		td->qt_buffer[4] =
+		    cpu_to_hc32((td->qt_buffer[0] + 0x4000) & ~0xfff);
+
+		*buf = buffer + i * elementsize;
+		toggle ^= 1;
+	}
+
+	if (ehci->periodic_schedules > 0) {
+		if (disable_periodic(ehci) < 0) {
+			dev_err(&dev->dev,
+				"FATAL: periodic should never fail, but did");
+			goto fail3;
+		}
+	}
+
+	/* hook up to periodic list */
+	result->last->qh_link = list->qh_link;
+	list->qh_link = cpu_to_hc32((unsigned long)result->first | QH_LINK_TYPE_QH);
+
+	if (enable_periodic(ehci) < 0) {
+		dev_err(&dev->dev,
+			"FATAL: periodic should never fail, but did");
+		goto fail3;
+	}
+	ehci->periodic_schedules++;
+
+	dev_dbg(&dev->dev, "Exit create_int_queue\n");
+	return result;
+fail3:
+	dma_free_coherent(result->tds, 0, sizeof(struct qTD) * queuesize);
+	dma_free_coherent(result->first, 0, sizeof(struct QH) * queuesize);
+	free(result);
+	return NULL;
+}
+
+static void *ehci_poll_int_queue(struct usb_device *dev,
+				  struct int_queue *queue)
+{
+	struct QH *cur = queue->current;
+	struct qTD *cur_td;
+	uint32_t token, toggle;
+	unsigned long pipe = queue->pipe;
+
+	/* depleted queue */
+	if (cur == NULL) {
+		dev_dbg(&dev->dev, "Exit poll_int_queue with completed queue\n");
+		return NULL;
+	}
+	/* still active */
+	cur_td = &queue->tds[queue->current - queue->first];
+	token = hc32_to_cpu(cur_td->qt_token);
+	if (QT_TOKEN_GET_STATUS(token) & QT_TOKEN_STATUS_ACTIVE) {
+		dev_dbg(&dev->dev,
+			"Exit poll_int_queue with no completed intr transfer. token is %x\n",
+			token);
+		return NULL;
+	}
+
+	toggle = QT_TOKEN_GET_DT(token);
+	usb_settoggle(dev, usb_pipeendpoint(pipe), usb_pipeout(pipe), toggle);
+
+	if (!(cur->qh_link & QH_LINK_TERMINATE))
+		queue->current++;
+	else
+		queue->current = NULL;
+
+	dev_dbg(&dev->dev,
+		"Exit poll_int_queue with completed intr transfer. token is %x at %p (first at %p)\n",
+		token, cur, queue->first);
+	return cur->buffer;
+}
+
+static int ehci_destroy_int_queue(struct usb_device *dev,
+				   struct int_queue *queue)
+{
+	int result = -EINVAL;
+	struct usb_host *host = dev->host;
+	struct ehci_priv *ehci = to_ehci(host);
+	struct QH *cur = ehci->periodic_queue;
+	uint64_t start;
+
+	if (disable_periodic(ehci) < 0) {
+		dev_err(&dev->dev,
+			"FATAL: periodic should never fail, but did\n");
+		goto out;
+	}
+	ehci->periodic_schedules--;
+
+	start = get_time_ns();
+	while (!(cur->qh_link & cpu_to_hc32(QH_LINK_TERMINATE))) {
+		dev_dbg(&dev->dev, 
+			"considering %p, with qh_link %x\n",
+			cur, cur->qh_link);
+		if (NEXT_QH(cur) == queue->first) {
+			dev_dbg(&dev->dev,
+				"found candidate. removing from chain\n");
+			cur->qh_link = queue->last->qh_link;
+			result = 0;
+			break;
+		}
+		cur = NEXT_QH(cur);
+		if (is_timeout_non_interruptible(start, 500 * MSECOND)) {
+			dev_err(&dev->dev,
+				"Timeout destroying interrupt endpoint queue\n");
+			result = -ETIMEDOUT;
+			goto out;
+		}
+	}
+
+	if (ehci->periodic_schedules > 0) {
+		result = enable_periodic(ehci);
+		if (result < 0)
+			dev_err(&dev->dev,
+				"FATAL: periodic should never fail, but did");
+	}
+
+out:
+	free(queue->tds);
+	free(queue->first);
+	free(queue);
+
+	return result;
+}
+
+static int
 submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
 	       int length, int interval)
 {
 	struct usb_host *host = dev->host;
 	struct ehci_priv *ehci = to_ehci(host);
+	struct int_queue *queue;
+	uint64_t start;
+	void *backbuffer;
+	int result = 0, ret;
 
 	dev_dbg(ehci->dev, "dev=%p, pipe=%lu, buffer=%p, length=%d, interval=%d",
 	      dev, pipe, buffer, length, interval);
-	return -1;
+
+	queue = ehci_create_int_queue(dev, pipe, 1, length, buffer, interval);
+	if (!queue)
+		return -EINVAL;
+
+	start = get_time_ns();
+	while ((backbuffer = ehci_poll_int_queue(dev, queue)) == NULL)
+		if (is_timeout_non_interruptible(start, 
+						 USB_CNTL_TIMEOUT * MSECOND)) {
+			dev_err(&dev->dev,
+				"Timeout poll on interrupt endpoint\n");
+			result = -ETIMEDOUT;
+			break;
+		}
+
+	if (backbuffer != buffer) {
+		dev_err(&dev->dev,
+			"got wrong buffer back (%p instead of %p)\n",
+			backbuffer, buffer);
+		return -EINVAL;
+	}
+
+	ret = ehci_destroy_int_queue(dev, queue);
+	if (ret < 0)
+		return ret;
+
+	return result;
 }
 
 static int ehci_detect(struct device_d *dev)
@@ -913,6 +1309,8 @@ int ehci_register(struct device_d *dev, struct ehci_data *data)
 
 	ehci->qh_list = dma_alloc_coherent(sizeof(struct QH) * NUM_TD,
 					   DMA_ADDRESS_BROKEN);
+	ehci->periodic_queue = dma_alloc_coherent(sizeof(struct QH),
+						  DMA_ADDRESS_BROKEN);
 	ehci->td = dma_alloc_coherent(sizeof(struct qTD) * NUM_TD,
 				      DMA_ADDRESS_BROKEN);
 
diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index d71d056..39de763 100644
--- a/drivers/usb/host/ehci.h
+++ b/drivers/usb/host/ehci.h
@@ -20,6 +20,15 @@
 
 #include <io.h>
 
+#define QH_ENDPT1_EPS(x)	(((x) & 0x3) << 12)	/* Endpoint Speed */
+#define QH_ENDPT2_PORTNUM(x)	(((x) & 0x7f) << 23)	/* Port Number */
+#define QH_ENDPT2_HUBADDR(x)	(((x) & 0x7f) << 16)	/* Hub Address */
+
+#define QT_TOKEN_DT(x)		(((x) & 0x1) << 31)	/* Data Toggle */
+#define QT_TOKEN_GET_STATUS(x)	(((x) >> 0) & 0xff)
+#define QT_TOKEN_STATUS_ACTIVE	0x80
+#define QT_TOKEN_GET_DT(x)	(((x) >> 31) & 0x1)
+
 #if !defined(CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS)
 #define CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS	16
 #endif
@@ -51,6 +60,7 @@ struct ehci_hcor {
 #define CMD_RUN		(1 << 0)		/* start/stop HC */
 	uint32_t or_usbsts;
 #define	STD_ASS		(1 << 15)
+#define STS_PSS         (1 << 14)
 #define STS_HALT	(1 << 12)
 	uint32_t or_usbintr;
 	uint32_t or_frindex;
@@ -148,7 +158,10 @@ struct QH {
 	 * Add dummy fill value to make the size of this struct
 	 * aligned to 32 bytes
 	 */
-	uint8_t fill[16];
+	union {
+		uint8_t fill[16];
+		void* buffer;
+	};
 };
 
 #endif /* USB_EHCI_H */
-- 
2.1.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/5] usb: ehci-hcd: port periodic transactions implementation from the u-boot

[Sascha Hauer]

On Mon, Sep 21, 2015 at 02:30:45PM +0300, Peter Mamonov wrote:
> Signed-off-by: Peter Mamonov <pmamonov@gmail.com>
> ---
>  drivers/usb/host/ehci-hcd.c | 400 +++++++++++++++++++++++++++++++++++++++++++-
>  drivers/usb/host/ehci.h     |  15 +-
>  2 files changed, 413 insertions(+), 2 deletions(-)
> 
>  }
>  
>  static int
> +disable_periodic(struct ehci_priv *ehci)
> +{
> +	uint32_t cmd;
> +	struct ehci_hcor *hcor = ehci->hcor;
> +	int ret;
> +
> +	cmd = ehci_readl(&hcor->or_usbcmd);
> +	cmd &= ~CMD_PSE;
> +	ehci_writel(&hcor->or_usbcmd, cmd);
> +
> +	ret = handshake((uint32_t *)&hcor->or_usbsts,
> +			STS_PSS, 0, 100 * 1000);
> +	if (ret < 0) {
> +		printf("EHCI failed: timeout when disabling periodic list\n");
> +		return -ETIMEDOUT;
> +	}
> +	return 0;
> +}
> +
> +#define NEXT_QH(qh) (struct QH *)((0xa0000000 | (unsigned long)hc32_to_cpu((qh)->qh_link)) & ~0x1f)

Ok, here's the reason it doesn't work on Arm. The 0xa0000000 is probably
due to the cached/uncached address conversion on Mips. Without the
0a0000000 this patch works fine on Arm.

> +
> +static int
> +enable_periodic(struct ehci_priv *ehci)
> +{
> +	uint32_t cmd;
> +	struct ehci_hcor *hcor = ehci->hcor;
> +	int ret;
> +	uint64_t start;
> +
> +	cmd = ehci_readl(&hcor->or_usbcmd);
> +	cmd |= CMD_PSE;
> +	ehci_writel(&hcor->or_usbcmd, cmd);
> +
> +	ret = handshake((uint32_t *)&hcor->or_usbsts,
> +			STS_PSS, STS_PSS, 100 * 1000);
> +	if (ret < 0) {
> +		printf("EHCI failed: timeout when enabling periodic list\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	start = get_time_ns();
> +	while(!is_timeout_non_interruptible(start, 1 * MSECOND));

mdelay_non_interruptible?

> +static int ehci_destroy_int_queue(struct usb_device *dev,
> +				   struct int_queue *queue)
> +{
> +	int result = -EINVAL;
> +	struct usb_host *host = dev->host;
> +	struct ehci_priv *ehci = to_ehci(host);
> +	struct QH *cur = ehci->periodic_queue;
> +	uint64_t start;
> +
> +	if (disable_periodic(ehci) < 0) {
> +		dev_err(&dev->dev,
> +			"FATAL: periodic should never fail, but did\n");
> +		goto out;
> +	}
> +	ehci->periodic_schedules--;
> +
> +	start = get_time_ns();
> +	while (!(cur->qh_link & cpu_to_hc32(QH_LINK_TERMINATE))) {
> +		dev_dbg(&dev->dev, 
> +			"considering %p, with qh_link %x\n",
> +			cur, cur->qh_link);
> +		if (NEXT_QH(cur) == queue->first) {
> +			dev_dbg(&dev->dev,
> +				"found candidate. removing from chain\n");
> +			cur->qh_link = queue->last->qh_link;
> +			result = 0;
> +			break;
> +		}
> +		cur = NEXT_QH(cur);
> +		if (is_timeout_non_interruptible(start, 500 * MSECOND)) {
> +			dev_err(&dev->dev,
> +				"Timeout destroying interrupt endpoint queue\n");
> +			result = -ETIMEDOUT;
> +			goto out;
> +		}
> +	}
> +
> +	if (ehci->periodic_schedules > 0) {
> +		result = enable_periodic(ehci);
> +		if (result < 0)
> +			dev_err(&dev->dev,
> +				"FATAL: periodic should never fail, but did");
> +	}
> +
> +out:
> +	free(queue->tds);
> +	free(queue->first);

You have to use dma_free_coherent here.

> +	free(queue);
> +
> +	return result;
> +}
> +
>  submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
>  	       int length, int interval)
>  {
>  	struct usb_host *host = dev->host;
>  	struct ehci_priv *ehci = to_ehci(host);
> +	struct int_queue *queue;
> +	uint64_t start;
> +	void *backbuffer;
> +	int result = 0, ret;
>  
>  	dev_dbg(ehci->dev, "dev=%p, pipe=%lu, buffer=%p, length=%d, interval=%d",
>  	      dev, pipe, buffer, length, interval);
> -	return -1;
> +
> +	queue = ehci_create_int_queue(dev, pipe, 1, length, buffer, interval);
> +	if (!queue)
> +		return -EINVAL;
> +
> +	start = get_time_ns();
> +	while ((backbuffer = ehci_poll_int_queue(dev, queue)) == NULL)
> +		if (is_timeout_non_interruptible(start, 
> +						 USB_CNTL_TIMEOUT * MSECOND)) {
> +			dev_err(&dev->dev,
> +				"Timeout poll on interrupt endpoint\n");
> +			result = -ETIMEDOUT;
> +			break;
> +		}
> +
> +	if (backbuffer != buffer) {
> +		dev_err(&dev->dev,
> +			"got wrong buffer back (%p instead of %p)\n",
> +			backbuffer, buffer);
> +		return -EINVAL;

Shouldn't we call ehci_destroy_int_queue() here aswell? Otherwise we
leak the memory allocated from ehci_create_int_queue().

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/5] usb: ehci-hcd: detect re-entrance

[Peter Mamonov]

Signed-off-by: Peter Mamonov <pmamonov@gmail.com>
---
 drivers/usb/host/ehci-hcd.c | 43 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 38 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 14bb7d9..133f42e 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -51,6 +51,8 @@ struct ehci_priv {
 	int periodic_schedules;
 	struct QH *periodic_queue;
 	uint32_t *periodic_list;
+	int sem;
+	struct device_d *usedby;
 };
 
 struct int_queue {
@@ -135,6 +137,16 @@ static struct descriptor {
 
 #define ehci_is_TDI()	(ehci->flags & EHCI_HAS_TT)
 
+#define ehci_reentrance_detect(ehci) \
+	if (ehci->sem) \
+		dev_err(&dev->dev, "%s: re-entrance %d (%s:%s)\n", \
+			__func__, \
+			ehci->sem, \
+			ehci->usedby->driver->name, \
+			ehci->usedby->name); \
+	ehci->sem++; \
+	ehci->usedby = &dev->dev;
+
 static int handshake(uint32_t *ptr, uint32_t mask, uint32_t done, int usec)
 {
 	uint32_t result;
@@ -893,12 +905,18 @@ submit_bulk_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
 {
 	struct usb_host *host = dev->host;
 	struct ehci_priv *ehci = to_ehci(host);
+	int ret;
+
+	ehci_reentrance_detect(ehci);
 
 	if (usb_pipetype(pipe) != PIPE_BULK) {
 		dev_dbg(ehci->dev, "non-bulk pipe (type=%lu)", usb_pipetype(pipe));
+		ehci->sem--;
 		return -1;
 	}
-	return ehci_submit_async(dev, pipe, buffer, length, NULL);
+	ret = ehci_submit_async(dev, pipe, buffer, length, NULL);
+	ehci->sem--;
+	return ret;
 }
 
 static int
@@ -907,6 +925,9 @@ submit_control_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
 {
 	struct usb_host *host = dev->host;
 	struct ehci_priv *ehci = to_ehci(host);
+	int ret;
+
+	ehci_reentrance_detect(ehci);
 
 	if (usb_pipetype(pipe) != PIPE_CONTROL) {
 		dev_dbg(ehci->dev, "non-control pipe (type=%lu)", usb_pipetype(pipe));
@@ -916,9 +937,13 @@ submit_control_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
 	if (usb_pipedevice(pipe) == ehci->rootdev) {
 		if (ehci->rootdev == 0)
 			dev->speed = USB_SPEED_HIGH;
-		return ehci_submit_root(dev, pipe, buffer, length, setup);
+		ret = ehci_submit_root(dev, pipe, buffer, length, setup);
+		ehci->sem--;
+		return ret;
 	}
-	return ehci_submit_async(dev, pipe, buffer, length, setup);
+	ret = ehci_submit_async(dev, pipe, buffer, length, setup);
+	ehci->sem--;
+	return ret;
 }
 
 static int
@@ -1246,12 +1271,16 @@ submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
 	void *backbuffer;
 	int result = 0, ret;
 
+	ehci_reentrance_detect(ehci);
+
 	dev_dbg(ehci->dev, "dev=%p, pipe=%lu, buffer=%p, length=%d, interval=%d",
 	      dev, pipe, buffer, length, interval);
 
 	queue = ehci_create_int_queue(dev, pipe, 1, length, buffer, interval);
-	if (!queue)
+	if (!queue) {
+		ehci->sem--;
 		return -EINVAL;
+	}
 
 	start = get_time_ns();
 	while ((backbuffer = ehci_poll_int_queue(dev, queue)) == NULL)
@@ -1267,13 +1296,17 @@ submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
 		dev_err(&dev->dev,
 			"got wrong buffer back (%p instead of %p)\n",
 			backbuffer, buffer);
+		ehci->sem--;
 		return -EINVAL;
 	}
 
 	ret = ehci_destroy_int_queue(dev, queue);
-	if (ret < 0)
+	if (ret < 0) {
+		ehci->sem--;
 		return ret;
+	}
 
+	ehci->sem--;
 	return result;
 }
 
-- 
2.1.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/5] common: clock: introduce mdelay_non_interruptible()

[Peter Mamonov]

Signed-off-by: Peter Mamonov <pmamonov@gmail.com>
---
 common/clock.c   | 8 ++++++++
 include/clock.h  | 1 +
 include/common.h | 1 +
 3 files changed, 10 insertions(+)

diff --git a/common/clock.c b/common/clock.c
index 35c9e6c..51cf9e4 100644
--- a/common/clock.c
+++ b/common/clock.c
@@ -202,6 +202,14 @@ void mdelay(unsigned long msecs)
 }
 EXPORT_SYMBOL(mdelay);
 
+void mdelay_non_interruptible(unsigned long msecs)
+{
+	uint64_t start = get_time_ns();
+
+	while(!is_timeout_non_interruptible(start, msecs * MSECOND));
+}
+EXPORT_SYMBOL(mdelay_non_interruptible);
+
 int init_clock(struct clocksource *cs)
 {
 	current_clock = cs;
diff --git a/include/clock.h b/include/clock.h
index 691befc..68d71d7 100644
--- a/include/clock.h
+++ b/include/clock.h
@@ -38,6 +38,7 @@ int is_timeout_non_interruptible(uint64_t start_ns, uint64_t time_offset_ns);
 
 void ndelay(unsigned long nsecs);
 void mdelay(unsigned long msecs);
+void mdelay_non_interruptible (unsigned long msecs);
 
 #define SECOND ((uint64_t)(1000 * 1000 * 1000))
 #define MSECOND ((uint64_t)(1000 * 1000))
diff --git a/include/common.h b/include/common.h
index 553a7f4..c576908 100644
--- a/include/common.h
+++ b/include/common.h
@@ -72,6 +72,7 @@ void __noreturn poweroff(void);
 /* lib_$(ARCH)/time.c */
 void	udelay (unsigned long);
 void	mdelay (unsigned long);
+void	mdelay_non_interruptible (unsigned long);
 
 /* lib_generic/crc32.c */
 uint32_t crc32(uint32_t, const void*, unsigned int);
-- 
2.1.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 4/5] usb: ehci-hcd: use mdelay_non_inerruptible()

[Peter Mamonov]

Signed-off-by: Peter Mamonov <pmamonov@gmail.com>
---
 drivers/usb/host/ehci-hcd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 133f42e..4a8bb58 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -684,7 +684,7 @@ ehci_submit_root(struct usb_device *dev, unsigned long pipe, void *buffer,
 				 * root
 				 */
 				ehci_powerup_fixup(ehci);
-				mdelay(50);
+				mdelay_non_interruptible(50);
 				ehci->portreset |= 1 << port;
 				/* terminate the reset */
 				ehci_writel(status_reg, reg & ~EHCI_PS_PR);
@@ -747,7 +747,7 @@ ehci_submit_root(struct usb_device *dev, unsigned long pipe, void *buffer,
 		goto unknown;
 	}
 
-	mdelay(1);
+	mdelay_non_interruptible(1);
 	len = min3(srclen, (int)le16_to_cpu(req->length), length);
 	if (srcptr != NULL && len > 0)
 		memcpy(buffer, srcptr, len);
@@ -889,7 +889,7 @@ static int ehci_init(struct usb_host *host)
 	ehci_writel(&ehci->hcor->or_configflag, cmd);
 	/* unblock posted write */
 	cmd = ehci_readl(&ehci->hcor->or_usbcmd);
-	mdelay(5);
+	mdelay_non_interruptible(5);
 
 	ehci->rootdev = 0;
 
-- 
2.1.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 5/5] input: port usb keyboard driver from the u-boot

[Peter Mamonov]

Signed-off-by: Peter Mamonov <pmamonov@gmail.com>
---
 drivers/input/Kconfig   |   7 +
 drivers/input/Makefile  |   1 +
 drivers/input/usb_kbd.c | 416 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 424 insertions(+)
 create mode 100644 drivers/input/usb_kbd.c

diff --git a/drivers/input/Kconfig b/drivers/input/Kconfig
index b4e86fd..24a5d10 100644
--- a/drivers/input/Kconfig
+++ b/drivers/input/Kconfig
@@ -46,4 +46,11 @@ config KEYBOARD_TWL6030
 	help
 	  Say Y here if you want to use TWL6030 power button as a key.
 
+config KEYBOARD_USB
+	bool "USB keyboard"
+	depends on USB_HOST
+	select POLLER
+	help
+	  This driver implements support for usb keyboard.
+
 endmenu
diff --git a/drivers/input/Makefile b/drivers/input/Makefile
index 2143336..40b898c 100644
--- a/drivers/input/Makefile
+++ b/drivers/input/Makefile
@@ -1,3 +1,4 @@
+obj-$(CONFIG_KEYBOARD_USB) += usb_kbd.o
 obj-$(CONFIG_KEYBOARD_GPIO) += gpio_keys.o
 obj-$(CONFIG_KEYBOARD_TWL6030) += twl6030_pwrbtn.o
 obj-$(CONFIG_KEYBOARD_IMX_KEYPAD) += imx_keypad.o
diff --git a/drivers/input/usb_kbd.c b/drivers/input/usb_kbd.c
new file mode 100644
index 0000000..db1ba90
--- /dev/null
+++ b/drivers/input/usb_kbd.c
@@ -0,0 +1,416 @@
+/*
+ * USB keyboard driver for barebox
+ *
+ * (C) Copyright 2001 Denis Peter, MPL AG Switzerland
+ * (C) Copyright 2015 Peter Mamonov
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <common.h>
+#include <init.h>
+#include <clock.h>
+#include <poller.h>
+#include <usb/usb.h>
+#include <string.h>
+#include <dma.h>
+#include <kfifo.h>
+
+#define USB_KBD_FIFO_SIZE	50
+
+#define REPEAT_RATE	40		/* 40msec -> 25cps */
+#define REPEAT_DELAY	10		/* 10 x REPEAT_RATE = 400msec */
+
+#define NUM_LOCK	0x53
+#define CAPS_LOCK	0x39
+#define SCROLL_LOCK	0x47
+
+/* Modifier bits */
+#define LEFT_CNTR	(1 << 0)
+#define LEFT_SHIFT	(1 << 1)
+#define LEFT_ALT	(1 << 2)
+#define LEFT_GUI	(1 << 3)
+#define RIGHT_CNTR	(1 << 4)
+#define RIGHT_SHIFT	(1 << 5)
+#define RIGHT_ALT	(1 << 6)
+#define RIGHT_GUI	(1 << 7)
+
+/* Size of the keyboard buffer */
+#define USB_KBD_BUFFER_LEN	0x20
+
+/* Keyboard maps */
+static const unsigned char usb_kbd_numkey[] = {
+	'1', '2', '3', '4', '5', '6', '7', '8', '9', '0',
+	'\r', 0x1b, '\b', '\t', ' ', '-', '=', '[', ']',
+	'\\', '#', ';', '\'', '`', ',', '.', '/'
+};
+static const unsigned char usb_kbd_numkey_shifted[] = {
+	'!', '@', '#', '$', '%', '^', '&', '*', '(', ')',
+	'\r', 0x1b, '\b', '\t', ' ', '_', '+', '{', '}',
+	'|', '~', ':', '"', '~', '<', '>', '?'
+};
+
+static const unsigned char usb_kbd_num_keypad[] = {
+	'/', '*', '-', '+', '\r',
+	'1', '2', '3', '4', '5', '6', '7', '8', '9', '0',
+	'.', 0, 0, 0, '='
+};
+
+/*
+ * map arrow keys to ^F/^B ^N/^P, can't really use the proper
+ * ANSI sequence for arrow keys because the queuing code breaks
+ * when a single keypress expands to 3 queue elements
+ */
+static const unsigned char usb_kbd_arrow[] = {
+	0x6, 0x2, 0xe, 0x10
+};
+
+/*
+ * NOTE: It's important for the NUM, CAPS, SCROLL-lock bits to be in this
+ *       order. See usb_kbd_setled() function!
+ */
+#define USB_KBD_NUMLOCK		(1 << 0)
+#define USB_KBD_CAPSLOCK	(1 << 1)
+#define USB_KBD_SCROLLLOCK	(1 << 2)
+#define USB_KBD_CTRL		(1 << 3)
+
+#define USB_KBD_LEDMASK		\
+	(USB_KBD_NUMLOCK | USB_KBD_CAPSLOCK | USB_KBD_SCROLLLOCK)
+
+/*
+ * USB Keyboard reports are 8 bytes in boot protocol.
+ * Appendix B of HID Device Class Definition 1.11
+ */
+#define USB_KBD_BOOT_REPORT_SIZE 8
+
+struct usb_kbd_pdata;
+
+struct usb_kbd_pdata {
+	uint64_t	last_report;
+	uint64_t	last_poll;
+	uint8_t		*new;
+	uint8_t		old[USB_KBD_BOOT_REPORT_SIZE];
+	uint32_t	repeat_delay;
+	uint8_t		flags;
+	struct poller_struct	poller;
+	struct usb_device	*usbdev;
+	struct console_device	cdev;
+	struct kfifo		*recv_fifo;
+	int		lock;
+	unsigned long	intpipe;
+	int		intpktsize;
+	int		intinterval;
+	struct usb_endpoint_descriptor *ep;
+	int (*do_poll)(struct usb_kbd_pdata *);
+};
+
+static int usb_kbd_int_poll(struct usb_kbd_pdata *data)
+{
+	return usb_submit_int_msg(data->usbdev, data->intpipe, data->new,
+				  data->intpktsize, data->intinterval);
+}
+
+static int usb_kbd_cnt_poll(struct usb_kbd_pdata *data)
+{
+	struct usb_interface *iface = &data->usbdev->config.interface[0];
+
+	return usb_get_report(data->usbdev, iface->desc.bInterfaceNumber,
+			      1, 0, data->new, USB_KBD_BOOT_REPORT_SIZE);
+}
+
+#define CAPITAL_MASK	0x20
+/* Translate the scancode in ASCII */
+static int usb_kbd_translate(struct usb_kbd_pdata *data, unsigned char scancode,
+				unsigned char modifier, int pressed)
+{
+	int keycode = 0;
+
+	/* Key released */
+	if (pressed == 0) {
+		data->repeat_delay = 0;
+		return 0;
+	}
+
+	if (pressed == 2) {
+		data->repeat_delay++;
+		if (data->repeat_delay < REPEAT_DELAY)
+			return 0;
+
+		data->repeat_delay = REPEAT_DELAY;
+	}
+
+	/* Alphanumeric values */
+	if ((scancode > 3) && (scancode <= 0x1d)) {
+		keycode = scancode - 4 + 'a';
+
+		if (data->flags & USB_KBD_CAPSLOCK)
+			keycode &= ~CAPITAL_MASK;
+
+		if (modifier & (LEFT_SHIFT | RIGHT_SHIFT)) {
+			/* Handle CAPSLock + Shift pressed simultaneously */
+			if (keycode & CAPITAL_MASK)
+				keycode &= ~CAPITAL_MASK;
+			else
+				keycode |= CAPITAL_MASK;
+		}
+	}
+
+	if ((scancode > 0x1d) && (scancode < 0x3a)) {
+		/* Shift pressed */
+		if (modifier & (LEFT_SHIFT | RIGHT_SHIFT))
+			keycode = usb_kbd_numkey_shifted[scancode - 0x1e];
+		else
+			keycode = usb_kbd_numkey[scancode - 0x1e];
+	}
+
+	/* Arrow keys */
+	if ((scancode >= 0x4f) && (scancode <= 0x52))
+		keycode = usb_kbd_arrow[scancode - 0x4f];
+
+	/* Numeric keypad */
+	if ((scancode >= 0x54) && (scancode <= 0x67))
+		keycode = usb_kbd_num_keypad[scancode - 0x54];
+
+	if (data->flags & USB_KBD_CTRL)
+		keycode = scancode - 0x3;
+
+	if (pressed == 1) {
+		if (scancode == NUM_LOCK) {
+			data->flags ^= USB_KBD_NUMLOCK;
+			return 1;
+		}
+
+		if (scancode == CAPS_LOCK) {
+			data->flags ^= USB_KBD_CAPSLOCK;
+			return 1;
+		}
+		if (scancode == SCROLL_LOCK) {
+			data->flags ^= USB_KBD_SCROLLLOCK;
+			return 1;
+		}
+	}
+
+	/* Report keycode if any */
+	if (keycode) {
+		pr_debug("%s: key pressed: '%c'\n", __FUNCTION__, keycode);
+		kfifo_put(data->recv_fifo, (u_char*)&keycode, sizeof(keycode));
+	}
+
+	return 0;
+}
+
+static uint32_t usb_kbd_service_key(struct usb_kbd_pdata *data, int i, int up)
+{
+	uint32_t res = 0;
+	uint8_t *new;
+	uint8_t *old;
+
+	if (up) {
+		new = data->old;
+		old = data->new;
+	} else {
+		new = data->new;
+		old = data->old;
+	}
+
+	if ((old[i] > 3) &&
+	    (memscan(new + 2, old[i], USB_KBD_BOOT_REPORT_SIZE - 2) ==
+			new + USB_KBD_BOOT_REPORT_SIZE)) {
+		res |= usb_kbd_translate(data, old[i], data->new[0], up);
+	}
+
+	return res;
+}
+
+static void usb_kbd_setled(struct usb_kbd_pdata *data)
+{
+	struct usb_device *usbdev = data->usbdev;
+	struct usb_interface *iface = &usbdev->config.interface[0];
+	uint8_t leds = (uint8_t)(data->flags & USB_KBD_LEDMASK);
+
+	usb_control_msg(usbdev, usb_sndctrlpipe(usbdev, 0),
+		USB_REQ_SET_REPORT, USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+		0x200, iface->desc.bInterfaceNumber, &leds, 1, USB_CNTL_TIMEOUT);
+}
+
+
+static int usb_kbd_process(struct usb_kbd_pdata *data)
+{
+	int i, res = 0;
+
+	/* No combo key pressed */
+	if (data->new[0] == 0x00)
+		data->flags &= ~USB_KBD_CTRL;
+	/* Left or Right Ctrl pressed */
+	else if ((data->new[0] == LEFT_CNTR) || (data->new[0] == RIGHT_CNTR))
+		data->flags |= USB_KBD_CTRL;
+
+	for (i = 2; i < USB_KBD_BOOT_REPORT_SIZE; i++) {
+		res |= usb_kbd_service_key(data, i, 0);
+		res |= usb_kbd_service_key(data, i, 1);
+	}
+
+	/* Key is still pressed */
+	if ((data->new[2] > 3) && (data->old[2] == data->new[2]))
+		res |= usb_kbd_translate(data, data->new[2], data->new[0], 2);
+
+	if (res == 1)
+		usb_kbd_setled(data);
+
+	return 1;
+}
+
+static void usb_kbd_poll(struct poller_struct *poller)
+{
+	struct usb_kbd_pdata *data = container_of(poller,
+						  struct usb_kbd_pdata, poller);
+	struct usb_device *usbdev = data->usbdev;
+	int diff, tout;
+
+	if (data->lock)
+		return;
+	data->lock = 1;
+
+	if (0 > data->do_poll(data)) {
+		/* exit and lock forever */
+		dev_err(&usbdev->dev,
+			"usb_submit_int_msg() failed. Keyboard disconnect?\n");
+		return;
+	}
+	diff = memcmp(data->old, data->new, USB_KBD_BOOT_REPORT_SIZE);
+	tout = get_time_ns() > data->last_report + REPEAT_RATE * MSECOND;
+	if (diff || tout) {
+		data->last_report = get_time_ns();
+		if (diff) {
+			pr_debug("%s: old report: %016llx\n",
+				__func__,
+				*((volatile uint64_t *)data->old));
+			pr_debug("%s: new report: %016llx\n\n",
+				__func__,
+				*((volatile uint64_t *)data->new));
+		}
+		usb_kbd_process(data);
+		memcpy(data->old, data->new, USB_KBD_BOOT_REPORT_SIZE);
+	}
+
+	data->lock = 0;
+}
+
+static int usb_kbd_getc(struct console_device *cdev)
+{
+	int code = 0;
+	struct usb_kbd_pdata *data = container_of(cdev, struct usb_kbd_pdata, cdev);
+
+	kfifo_get(data->recv_fifo, (u_char*)&code, sizeof(int));
+	return code;
+}
+
+static int usb_kbd_tstc(struct console_device *cdev)
+{
+	struct usb_kbd_pdata *data = container_of(cdev, struct usb_kbd_pdata, cdev);
+
+	return (kfifo_len(data->recv_fifo) == 0) ? 0 : 1;
+}
+
+static int usb_kbd_probe(struct usb_device *usbdev,
+			 const struct usb_device_id *id)
+{
+	int ret;
+	struct usb_interface *iface = &usbdev->config.interface[0];
+	struct usb_kbd_pdata *data;
+	struct console_device *cdev;
+
+	dev_info(&usbdev->dev, "USB keyboard found\n");
+	dev_dbg(&usbdev->dev, "Debug enabled\n");
+	ret = usb_set_protocol(usbdev, iface->desc.bInterfaceNumber, 0);
+	if (ret < 0)
+		return ret;
+
+	ret = usb_set_idle(usbdev, iface->desc.bInterfaceNumber, REPEAT_RATE / 4, 0);
+	if (ret < 0)
+		return ret;
+
+	data = xzalloc(sizeof(struct usb_kbd_pdata));
+	usbdev->drv_data = data;
+	data->recv_fifo = kfifo_alloc(USB_KBD_FIFO_SIZE);
+	data->new = dma_alloc_coherent(USB_KBD_BOOT_REPORT_SIZE, NULL);
+
+	data->usbdev = usbdev;
+	data->last_report = get_time_ns();
+
+	data->ep = &iface->ep_desc[0];
+	data->intpipe = usb_rcvintpipe(usbdev, data->ep->bEndpointAddress);
+	data->intpktsize = min(usb_maxpacket(usbdev, data->intpipe),
+			       USB_KBD_BOOT_REPORT_SIZE);
+	data->intinterval = data->ep->bInterval;
+	/* test polling via interrupt endpoint */
+	data->do_poll = usb_kbd_int_poll;
+	ret = data->do_poll(data);
+	if (ret < 0) {
+		/* fall back to polling via control enpoint */
+		data->do_poll = usb_kbd_cnt_poll;
+		usb_set_idle(usbdev,
+			     iface->desc.bInterfaceNumber, 0, 0);
+		ret = data->do_poll(data);
+		if (ret < 0) {
+			/* no luck */
+			kfifo_free(data->recv_fifo);
+			dma_free_coherent(data->new, 0,
+					  USB_KBD_BOOT_REPORT_SIZE);
+			free(data);
+			return ret;
+		} else
+			dev_dbg(&usbdev->dev, "poll keyboard via cont ep\n");
+	} else
+		dev_dbg(&usbdev->dev, "poll keyboard via int ep\n");
+
+	cdev = &data->cdev;
+	usbdev->dev.type_data = cdev;
+	cdev->dev = &usbdev->dev;
+	cdev->tstc = usb_kbd_tstc;
+	cdev->getc = usb_kbd_getc;
+
+	console_register(cdev);
+	console_set_active(cdev, CONSOLE_STDIN);
+
+	data->poller.func = usb_kbd_poll;
+	return poller_register(&data->poller);
+}
+
+static void usb_kbd_disconnect(struct usb_device *usbdev)
+{
+	struct usb_kbd_pdata *data = usbdev->drv_data;
+
+	poller_unregister(&data->poller);
+	console_unregister(&data->cdev);
+	kfifo_free(data->recv_fifo);
+	dma_free_coherent(data->new, 0, USB_KBD_BOOT_REPORT_SIZE);
+	free(data);
+}
+
+static struct usb_device_id usb_kbd_usb_ids[] = {
+	{ USB_INTERFACE_INFO(3, 1, 1) }, // usb keyboard
+	{ }
+};
+
+static struct usb_driver usb_kbd_driver = {
+	.name =		"usb-keyboard",
+	.id_table =	usb_kbd_usb_ids,
+	.probe =	usb_kbd_probe,
+	.disconnect =	usb_kbd_disconnect,
+};
+
+static int __init usb_kbd_init(void)
+{
+	return usb_driver_register(&usb_kbd_driver);
+}
+device_initcall(usb_kbd_init);
-- 
2.1.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v6 0/5] input: add usb keyboard driver

[Peter Mamonov]

Changes since v5:
	- Address issues reported here: 
	  http://lists.infradead.org/pipermail/barebox/2015-September/024726.html

	- input: usb_kbd: set idle time to minimum and limit polling rate

	  Idle time is the amount of time, during which the usb device will wait before
	  replying to the host interrupt packet. We don't want to waste that time, 
	  so keep it as small as possible. In particular this resolves the problem of 
	  very poor tftp performance, when the usb keyboard is enabled.

Peter Mamonov (5):
	common: clock: introduce mdelay_non_interruptible()
	usb: ehci-hcd: port periodic transactions implementation
	usb: ehci-hcd: detect re-entrance
	usb: ehci-hcd: use mdelay_non_inerruptible()
	input: port usb keyboard driver from the u-boot

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 4/5] usb: ehci-hcd: use mdelay_non_inerruptible()

[Peter Mamonov]

Signed-off-by: Peter Mamonov <pmamonov@gmail.com>
---
 drivers/usb/host/ehci-hcd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index d6df7b8..03d6150 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -684,7 +684,7 @@ ehci_submit_root(struct usb_device *dev, unsigned long pipe, void *buffer,
 				 * root
 				 */
 				ehci_powerup_fixup(ehci);
-				mdelay(50);
+				mdelay_non_interruptible(50);
 				ehci->portreset |= 1 << port;
 				/* terminate the reset */
 				ehci_writel(status_reg, reg & ~EHCI_PS_PR);
@@ -747,7 +747,7 @@ ehci_submit_root(struct usb_device *dev, unsigned long pipe, void *buffer,
 		goto unknown;
 	}
 
-	mdelay(1);
+	mdelay_non_interruptible(1);
 	len = min3(srclen, (int)le16_to_cpu(req->length), length);
 	if (srcptr != NULL && len > 0)
 		memcpy(buffer, srcptr, len);
@@ -889,7 +889,7 @@ static int ehci_init(struct usb_host *host)
 	ehci_writel(&ehci->hcor->or_configflag, cmd);
 	/* unblock posted write */
 	cmd = ehci_readl(&ehci->hcor->or_usbcmd);
-	mdelay(5);
+	mdelay_non_interruptible(5);
 
 	ehci->rootdev = 0;
 
-- 
2.1.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/5] usb: ehci-hcd: use mdelay_non_inerruptible()

[Jean-Christophe PLAGNIOL-VILLARD]

On 18:58 Tue 22 Sep     , Peter Mamonov wrote:
> Signed-off-by: Peter Mamonov <pmamonov@gmail.com>
> ---
>  drivers/usb/host/ehci-hcd.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index d6df7b8..03d6150 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -684,7 +684,7 @@ ehci_submit_root(struct usb_device *dev, unsigned long pipe, void *buffer,
>  				 * root
>  				 */
>  				ehci_powerup_fixup(ehci);
> -				mdelay(50);
> +				mdelay_non_interruptible(50);
>  				ehci->portreset |= 1 << port;
>  				/* terminate the reset */
>  				ehci_writel(status_reg, reg & ~EHCI_PS_PR);
> @@ -747,7 +747,7 @@ ehci_submit_root(struct usb_device *dev, unsigned long pipe, void *buffer,
>  		goto unknown;
>  	}
>  
> -	mdelay(1);
> +	mdelay_non_interruptible(1);
>  	len = min3(srclen, (int)le16_to_cpu(req->length), length);
>  	if (srcptr != NULL && len > 0)
>  		memcpy(buffer, srcptr, len);
> @@ -889,7 +889,7 @@ static int ehci_init(struct usb_host *host)
>  	ehci_writel(&ehci->hcor->or_configflag, cmd);
>  	/* unblock posted write */
>  	cmd = ehci_readl(&ehci->hcor->or_usbcmd);
> -	mdelay(5);
> +	mdelay_non_interruptible(5);
why do you need that much non interruptible delau?
>  
>  	ehci->rootdev = 0;
>  
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/5] usb: ehci-hcd: use mdelay_non_inerruptible()

[Peter Mamonov]

On Wed, 7 Oct 2015 15:47:03 +0200
Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:

> On 18:58 Tue 22 Sep     , Peter Mamonov wrote:
> > Signed-off-by: Peter Mamonov <pmamonov@gmail.com>
> > ---
> >  drivers/usb/host/ehci-hcd.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/usb/host/ehci-hcd.c
> > b/drivers/usb/host/ehci-hcd.c index d6df7b8..03d6150 100644
> > --- a/drivers/usb/host/ehci-hcd.c
> > +++ b/drivers/usb/host/ehci-hcd.c
> > @@ -684,7 +684,7 @@ ehci_submit_root(struct usb_device *dev,
> > unsigned long pipe, void *buffer,
> >  				 * root
> >  				 */
> >  				ehci_powerup_fixup(ehci);
> > -				mdelay(50);
> > +				mdelay_non_interruptible(50);
> >  				ehci->portreset |= 1 << port;
> >  				/* terminate the reset */
> >  				ehci_writel(status_reg, reg &
> > ~EHCI_PS_PR); @@ -747,7 +747,7 @@ ehci_submit_root(struct
> > usb_device *dev, unsigned long pipe, void *buffer, goto unknown;
> >  	}
> >  
> > -	mdelay(1);
> > +	mdelay_non_interruptible(1);
> >  	len = min3(srclen, (int)le16_to_cpu(req->length), length);
> >  	if (srcptr != NULL && len > 0)
> >  		memcpy(buffer, srcptr, len);
> > @@ -889,7 +889,7 @@ static int ehci_init(struct usb_host *host)
> >  	ehci_writel(&ehci->hcor->or_configflag, cmd);
> >  	/* unblock posted write */
> >  	cmd = ehci_readl(&ehci->hcor->or_usbcmd);
> > -	mdelay(5);
> > +	mdelay_non_interruptible(5);
> why do you need that much non interruptible delau?

Non-interruptible delays are needed here to prevent ehci_* functions
re-entrance. The re-entrance occurs during a usb bus scan, after
detection of a usb keyboard. As soon as a USB keyboard is detected, it's
driver starts the poller, which interferes with the process of usb bus scan.
However that last one delay may be interruptible.


> >  
> >  	ehci->rootdev = 0;
> >  
> > -- 
> > 2.1.4
> > 
> > 
> > _______________________________________________
> > barebox mailing list
> > barebox@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/barebox


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/5] usb: ehci-hcd: use mdelay_non_inerruptible()

[Jean-Christophe PLAGNIOL-VILLARD]

On 17:40 Wed 07 Oct     , Peter Mamonov wrote:
> On Wed, 7 Oct 2015 15:47:03 +0200
> Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:
> 
> > On 18:58 Tue 22 Sep     , Peter Mamonov wrote:
> > > Signed-off-by: Peter Mamonov <pmamonov@gmail.com>
> > > ---
> > >  drivers/usb/host/ehci-hcd.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/usb/host/ehci-hcd.c
> > > b/drivers/usb/host/ehci-hcd.c index d6df7b8..03d6150 100644
> > > --- a/drivers/usb/host/ehci-hcd.c
> > > +++ b/drivers/usb/host/ehci-hcd.c
> > > @@ -684,7 +684,7 @@ ehci_submit_root(struct usb_device *dev,
> > > unsigned long pipe, void *buffer,
> > >  				 * root
> > >  				 */
> > >  				ehci_powerup_fixup(ehci);
> > > -				mdelay(50);
> > > +				mdelay_non_interruptible(50);
> > >  				ehci->portreset |= 1 << port;
> > >  				/* terminate the reset */
> > >  				ehci_writel(status_reg, reg &
> > > ~EHCI_PS_PR); @@ -747,7 +747,7 @@ ehci_submit_root(struct
> > > usb_device *dev, unsigned long pipe, void *buffer, goto unknown;
> > >  	}
> > >  
> > > -	mdelay(1);
> > > +	mdelay_non_interruptible(1);
> > >  	len = min3(srclen, (int)le16_to_cpu(req->length), length);
> > >  	if (srcptr != NULL && len > 0)
> > >  		memcpy(buffer, srcptr, len);
> > > @@ -889,7 +889,7 @@ static int ehci_init(struct usb_host *host)
> > >  	ehci_writel(&ehci->hcor->or_configflag, cmd);
> > >  	/* unblock posted write */
> > >  	cmd = ehci_readl(&ehci->hcor->or_usbcmd);
> > > -	mdelay(5);
> > > +	mdelay_non_interruptible(5);
> > why do you need that much non interruptible delau?
> 
> Non-interruptible delays are needed here to prevent ehci_* functions
> re-entrance. The re-entrance occurs during a usb bus scan, after
> detection of a usb keyboard. As soon as a USB keyboard is detected, it's
> driver starts the poller, which interferes with the process of usb bus scan.
> However that last one delay may be interruptible.

my problem is as soon as you start a usb control msg you block everything

nothing else can run in barebox

can slow down barebox boot

I do think we need a real mdelay_non_interruptible feature but just forbidden
to recall usb control msg.
But the rest of barebox can run durring those 5ms

Best Regards,
J.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/5] usb: ehci-hcd: use mdelay_non_inerruptible()

[Peter Mamonov]

On Wed, 7 Oct 2015 17:40:24 +0200
Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:

> On 17:40 Wed 07 Oct     , Peter Mamonov wrote:
> > On Wed, 7 Oct 2015 15:47:03 +0200
> > Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:
> > 
> > > On 18:58 Tue 22 Sep     , Peter Mamonov wrote:
> > > > Signed-off-by: Peter Mamonov <pmamonov@gmail.com>
> > > > ---
> > > >  drivers/usb/host/ehci-hcd.c | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/usb/host/ehci-hcd.c
> > > > b/drivers/usb/host/ehci-hcd.c index d6df7b8..03d6150 100644
> > > > --- a/drivers/usb/host/ehci-hcd.c
> > > > +++ b/drivers/usb/host/ehci-hcd.c
> > > > @@ -684,7 +684,7 @@ ehci_submit_root(struct usb_device *dev,
> > > > unsigned long pipe, void *buffer,
> > > >  				 * root
> > > >  				 */
> > > >  				ehci_powerup_fixup(ehci);
> > > > -				mdelay(50);
> > > > +				mdelay_non_interruptible(50);
> > > >  				ehci->portreset |= 1 << port;
> > > >  				/* terminate the reset */
> > > >  				ehci_writel(status_reg, reg &
> > > > ~EHCI_PS_PR); @@ -747,7 +747,7 @@ ehci_submit_root(struct
> > > > usb_device *dev, unsigned long pipe, void *buffer, goto unknown;
> > > >  	}
> > > >  
> > > > -	mdelay(1);
> > > > +	mdelay_non_interruptible(1);
> > > >  	len = min3(srclen, (int)le16_to_cpu(req->length),
> > > > length); if (srcptr != NULL && len > 0)
> > > >  		memcpy(buffer, srcptr, len);
> > > > @@ -889,7 +889,7 @@ static int ehci_init(struct usb_host *host)
> > > >  	ehci_writel(&ehci->hcor->or_configflag, cmd);
> > > >  	/* unblock posted write */
> > > >  	cmd = ehci_readl(&ehci->hcor->or_usbcmd);
> > > > -	mdelay(5);
> > > > +	mdelay_non_interruptible(5);
> > > why do you need that much non interruptible delau?
> > 
> > Non-interruptible delays are needed here to prevent ehci_* functions
> > re-entrance. The re-entrance occurs during a usb bus scan, after
> > detection of a usb keyboard. As soon as a USB keyboard is detected,
> > it's driver starts the poller, which interferes with the process of
> > usb bus scan. However that last one delay may be interruptible.
> 
> my problem is as soon as you start a usb control msg you block
> everything
> 
> nothing else can run in barebox
> 
> can slow down barebox boot
> 
> I do think we need a real mdelay_non_interruptible feature but just
> forbidden to recall usb control msg.
> But the rest of barebox can run durring those 5ms

Well, I think it can be done by returning -EAGAIN on re-entrance
detection in ehci_* functions [1], and skipping the poll in the keyboard
driver.

By the way, could you clarify: do you really experience
considerable barebox slowdown or you make an assumtion based on the
code analysis?

[1]
http://lists.infradead.org/pipermail/barebox/2015-September/024715.html

Regards,
Peter

> 
> Best Regards,
> J.


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/5] usb: ehci-hcd: use mdelay_non_inerruptible()

[Sascha Hauer]

On Wed, Oct 07, 2015 at 07:52:21PM +0300, Peter Mamonov wrote:
> On Wed, 7 Oct 2015 17:40:24 +0200
> > > Non-interruptible delays are needed here to prevent ehci_* functions
> > > re-entrance. The re-entrance occurs during a usb bus scan, after
> > > detection of a usb keyboard. As soon as a USB keyboard is detected,
> > > it's driver starts the poller, which interferes with the process of
> > > usb bus scan. However that last one delay may be interruptible.
> > 
> > my problem is as soon as you start a usb control msg you block
> > everything
> > 
> > nothing else can run in barebox
> > 
> > can slow down barebox boot
> > 
> > I do think we need a real mdelay_non_interruptible feature but just
> > forbidden to recall usb control msg.
> > But the rest of barebox can run durring those 5ms
> 
> Well, I think it can be done by returning -EAGAIN on re-entrance
> detection in ehci_* functions [1], and skipping the poll in the keyboard
> driver.

Could you tell us what you have done to get re-entrance problems? I have
just replaced the mdelay_non_interruptible with regular mdelay in the
ehci driver and didn't notice any problems. Could you verify the
_non_interruptible is still needed?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/5] usb: ehci-hcd: use mdelay_non_inerruptible()

[Peter Mamonov]

On Mon, 12 Oct 2015 09:00:21 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> On Wed, Oct 07, 2015 at 07:52:21PM +0300, Peter Mamonov wrote:
> > On Wed, 7 Oct 2015 17:40:24 +0200
> > > > Non-interruptible delays are needed here to prevent ehci_*
> > > > functions re-entrance. The re-entrance occurs during a usb bus
> > > > scan, after detection of a usb keyboard. As soon as a USB
> > > > keyboard is detected, it's driver starts the poller, which
> > > > interferes with the process of usb bus scan. However that last
> > > > one delay may be interruptible.
> > > 
> > > my problem is as soon as you start a usb control msg you block
> > > everything
> > > 
> > > nothing else can run in barebox
> > > 
> > > can slow down barebox boot
> > > 
> > > I do think we need a real mdelay_non_interruptible feature but
> > > just forbidden to recall usb control msg.
> > > But the rest of barebox can run durring those 5ms
> > 
> > Well, I think it can be done by returning -EAGAIN on re-entrance
> > detection in ehci_* functions [1], and skipping the poll in the
> > keyboard driver.
> 
> Could you tell us what you have done to get re-entrance problems? I
> have just replaced the mdelay_non_interruptible with regular mdelay
> in the ehci driver and didn't notice any problems. Could you verify
> the _non_interruptible is still needed?

If I revert the "usb: ehci-hcd: use mdelay_non_interruptible()" patch,
while leaving the "usb: ehci-hcd: detect re-entrance" applied, I get
the following messages after running the "usb" command:

barebox:/ usb                                                         
USB: scanning bus for devices...                                                
Bus 001 Device 001: ID 0000:0000 EHCI Host Controller                           
Bus 001 Device 002: ID 0424:2514                                                
Bus 001 Device 003: ID 413c:2112 Dell USB Wired Multimedia Keybo                
usb-keyboard usb1-0-0: USB keyboard found                                       
Bus 001 Device 004: ID 8564:1000 Mass Storage Device                            
Using index 0 for the new disk                                                  
usb-keyboard usb1-0-0: submit_int_msg: re-entrance 1 (usb-hub:usb1)             
usb-keyboard usb1-0-0: submit_int_msg: re-entrance 1 (usb-hub:usb1)             
usb-keyboard usb1-0-0: submit_int_msg: re-entrance 1 (usb-keyboard:usb1-0-0)    
usb-keyboard usb1-0-0: submit_int_msg: re-entrance 1 (usb-keyboard:usb1-0-0)    
usb-keyboard usb1-0-0: submit_int_msg: re-entrance 1 (usb-hub:usb1)             
usb-keyboard usb1-0-0: submit_int_msg: re-entrance 1 (usb-keyboard:usb1-0-0)    
usb-keyboard usb1-0-0: submit_int_msg: re-entrance 1 (usb-keyboard:usb1-0-0)    
Bus 001 Device 005: ID 0424:2514                                                
5 USB Device(s) found                                                       

According to the debug messages: submit_int_msg() is called by the usb-keyboard driver (from the poller function),
while "usb-hub" driver is executing submit_control_msg() (which calls mdelay() and poller_call() subsequently).
This occurs several times, until usb bus scan is completed.
Probably the problem can be reproduced by adding more usb devices.

> 
> Sascha
> 


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/5] usb: ehci-hcd: use mdelay_non_inerruptible()

[Sascha Hauer]

On Mon, Oct 12, 2015 at 02:43:25PM +0300, Peter Mamonov wrote:
> On Mon, 12 Oct 2015 09:00:21 +0200
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > On Wed, Oct 07, 2015 at 07:52:21PM +0300, Peter Mamonov wrote:
> > > On Wed, 7 Oct 2015 17:40:24 +0200
> > > > > Non-interruptible delays are needed here to prevent ehci_*
> > > > > functions re-entrance. The re-entrance occurs during a usb bus
> > > > > scan, after detection of a usb keyboard. As soon as a USB
> > > > > keyboard is detected, it's driver starts the poller, which
> > > > > interferes with the process of usb bus scan. However that last
> > > > > one delay may be interruptible.
> > > > 
> > > > my problem is as soon as you start a usb control msg you block
> > > > everything
> > > > 
> > > > nothing else can run in barebox
> > > > 
> > > > can slow down barebox boot
> > > > 
> > > > I do think we need a real mdelay_non_interruptible feature but
> > > > just forbidden to recall usb control msg.
> > > > But the rest of barebox can run durring those 5ms
> > > 
> > > Well, I think it can be done by returning -EAGAIN on re-entrance
> > > detection in ehci_* functions [1], and skipping the poll in the
> > > keyboard driver.
> > 
> > Could you tell us what you have done to get re-entrance problems? I
> > have just replaced the mdelay_non_interruptible with regular mdelay
> > in the ehci driver and didn't notice any problems. Could you verify
> > the _non_interruptible is still needed?
> 
> If I revert the "usb: ehci-hcd: use mdelay_non_interruptible()" patch,
> while leaving the "usb: ehci-hcd: detect re-entrance" applied, I get
> the following messages after running the "usb" command:
> 
> barebox:/ usb                                                         
> USB: scanning bus for devices...                                                
> Bus 001 Device 001: ID 0000:0000 EHCI Host Controller                           
> Bus 001 Device 002: ID 0424:2514                                                
> Bus 001 Device 003: ID 413c:2112 Dell USB Wired Multimedia Keybo                
> usb-keyboard usb1-0-0: USB keyboard found                                       
> Bus 001 Device 004: ID 8564:1000 Mass Storage Device                            
> Using index 0 for the new disk                                                  
> usb-keyboard usb1-0-0: submit_int_msg: re-entrance 1 (usb-hub:usb1)             
> usb-keyboard usb1-0-0: submit_int_msg: re-entrance 1 (usb-hub:usb1)             
> usb-keyboard usb1-0-0: submit_int_msg: re-entrance 1 (usb-keyboard:usb1-0-0)    
> usb-keyboard usb1-0-0: submit_int_msg: re-entrance 1 (usb-keyboard:usb1-0-0)    
> usb-keyboard usb1-0-0: submit_int_msg: re-entrance 1 (usb-hub:usb1)             
> usb-keyboard usb1-0-0: submit_int_msg: re-entrance 1 (usb-keyboard:usb1-0-0)    
> usb-keyboard usb1-0-0: submit_int_msg: re-entrance 1 (usb-keyboard:usb1-0-0)    
> Bus 001 Device 005: ID 0424:2514                                                
> 5 USB Device(s) found                                                       
> 
> According to the debug messages: submit_int_msg() is called by the usb-keyboard driver (from the poller function),
> while "usb-hub" driver is executing submit_control_msg() (which calls mdelay() and poller_call() subsequently).
> This occurs several times, until usb bus scan is completed.
> Probably the problem can be reproduced by adding more usb devices.

I wonder why this does not happen here. I finally could reproduce this
by adding some additional delays to the ehci driver.

We should probably move the reentrance detection to the generic usb
layer to usb_submit_int_msg, usb_control_msg and usb_bulk_msg. This
would avoid running into the same problems in the other usb host
drivers. If I understand the issue correctly we could just use regular
*delay in the host drivers when we detect reentrancy correctly, right?
I mean we don't have to print an error message when we detect
reentrance, but instead just consider that a case that can happen.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/5] usb: ehci-hcd: use mdelay_non_inerruptible()

[Peter Mamonov]

On Mon, 12 Oct 2015 15:44:01 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> On Mon, Oct 12, 2015 at 02:43:25PM +0300, Peter Mamonov wrote:
> > On Mon, 12 Oct 2015 09:00:21 +0200
> > Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > 
> > > On Wed, Oct 07, 2015 at 07:52:21PM +0300, Peter Mamonov wrote:
> > > > On Wed, 7 Oct 2015 17:40:24 +0200
> > > > > > Non-interruptible delays are needed here to prevent ehci_*
> > > > > > functions re-entrance. The re-entrance occurs during a usb
> > > > > > bus scan, after detection of a usb keyboard. As soon as a
> > > > > > USB keyboard is detected, it's driver starts the poller,
> > > > > > which interferes with the process of usb bus scan. However
> > > > > > that last one delay may be interruptible.
> > > > > 
> > > > > my problem is as soon as you start a usb control msg you block
> > > > > everything
> > > > > 
> > > > > nothing else can run in barebox
> > > > > 
> > > > > can slow down barebox boot
> > > > > 
> > > > > I do think we need a real mdelay_non_interruptible feature but
> > > > > just forbidden to recall usb control msg.
> > > > > But the rest of barebox can run durring those 5ms
> > > > 
> > > > Well, I think it can be done by returning -EAGAIN on re-entrance
> > > > detection in ehci_* functions [1], and skipping the poll in the
> > > > keyboard driver.
> > > 
> > > Could you tell us what you have done to get re-entrance problems?
> > > I have just replaced the mdelay_non_interruptible with regular
> > > mdelay in the ehci driver and didn't notice any problems. Could
> > > you verify the _non_interruptible is still needed?
> > 
> > If I revert the "usb: ehci-hcd: use mdelay_non_interruptible()"
> > patch, while leaving the "usb: ehci-hcd: detect re-entrance"
> > applied, I get the following messages after running the "usb"
> > command:
> > 
> > barebox:/
> > usb USB: scanning bus for
> > devices... Bus 001 Device 001: ID 0000:0000 EHCI Host
> > Controller Bus 001 Device 002: ID
> > 0424:2514 Bus 001 Device 003: ID 413c:2112 Dell USB Wired
> > Multimedia Keybo usb-keyboard usb1-0-0: USB keyboard
> > found Bus 001 Device 004: ID 8564:1000 Mass Storage
> > Device Using index 0 for the new
> > disk usb-keyboard usb1-0-0: submit_int_msg: re-entrance 1
> > (usb-hub:usb1) usb-keyboard usb1-0-0: submit_int_msg: re-entrance 1
> > (usb-hub:usb1) usb-keyboard usb1-0-0: submit_int_msg: re-entrance 1
> > (usb-keyboard:usb1-0-0) usb-keyboard usb1-0-0: submit_int_msg:
> > re-entrance 1 (usb-keyboard:usb1-0-0) usb-keyboard usb1-0-0:
> > submit_int_msg: re-entrance 1 (usb-hub:usb1) usb-keyboard usb1-0-0:
> > submit_int_msg: re-entrance 1 (usb-keyboard:usb1-0-0) usb-keyboard
> > usb1-0-0: submit_int_msg: re-entrance 1 (usb-keyboard:usb1-0-0) Bus
> > 001 Device 005: ID 0424:2514 5 USB Device(s)
> > found                                                       
> > 
> > According to the debug messages: submit_int_msg() is called by the
> > usb-keyboard driver (from the poller function), while "usb-hub"
> > driver is executing submit_control_msg() (which calls mdelay() and
> > poller_call() subsequently). This occurs several times, until usb
> > bus scan is completed. Probably the problem can be reproduced by
> > adding more usb devices.
> 
> I wonder why this does not happen here. I finally could reproduce this
> by adding some additional delays to the ehci driver.
> 
> We should probably move the reentrance detection to the generic usb
> layer to usb_submit_int_msg, usb_control_msg and usb_bulk_msg. This
> would avoid running into the same problems in the other usb host
> drivers. If I understand the issue correctly we could just use regular
> *delay in the host drivers when we detect reentrancy correctly, right?
> I mean we don't have to print an error message when we detect
> reentrance, but instead just consider that a case that can happen.

We can return -EAGAIN on reentrance detection in usb_* functions, but
the usb device driver should distinguish this case and do sane
things, i.e. it should not fail, but skip the action. This only
concerns the drivers that use pollers, i.e. for example usb storage
driver is not affected, since it doesn't use pollers.

> 
> Sascha
> 


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/5] usb: ehci-hcd: use mdelay_non_inerruptible()

[Sascha Hauer]

On Tue, Oct 13, 2015 at 01:37:33PM +0300, Peter Mamonov wrote:
> On Mon, 12 Oct 2015 15:44:01 +0200
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > On Mon, Oct 12, 2015 at 02:43:25PM +0300, Peter Mamonov wrote:
> > > On Mon, 12 Oct 2015 09:00:21 +0200
> > > Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > > 
> > > > On Wed, Oct 07, 2015 at 07:52:21PM +0300, Peter Mamonov wrote:
> > > > > On Wed, 7 Oct 2015 17:40:24 +0200
> > > > > > > Non-interruptible delays are needed here to prevent ehci_*
> > > > > > > functions re-entrance. The re-entrance occurs during a usb
> > > > > > > bus scan, after detection of a usb keyboard. As soon as a
> > > > > > > USB keyboard is detected, it's driver starts the poller,
> > > > > > > which interferes with the process of usb bus scan. However
> > > > > > > that last one delay may be interruptible.
> > > > > > 
> > > > > > my problem is as soon as you start a usb control msg you block
> > > > > > everything
> > > > > > 
> > > > > > nothing else can run in barebox
> > > > > > 
> > > > > > can slow down barebox boot
> > > > > > 
> > > > > > I do think we need a real mdelay_non_interruptible feature but
> > > > > > just forbidden to recall usb control msg.
> > > > > > But the rest of barebox can run durring those 5ms
> > > > > 
> > > > > Well, I think it can be done by returning -EAGAIN on re-entrance
> > > > > detection in ehci_* functions [1], and skipping the poll in the
> > > > > keyboard driver.
> > > > 
> > > > Could you tell us what you have done to get re-entrance problems?
> > > > I have just replaced the mdelay_non_interruptible with regular
> > > > mdelay in the ehci driver and didn't notice any problems. Could
> > > > you verify the _non_interruptible is still needed?
> > > 
> > > If I revert the "usb: ehci-hcd: use mdelay_non_interruptible()"
> > > patch, while leaving the "usb: ehci-hcd: detect re-entrance"
> > > applied, I get the following messages after running the "usb"
> > > command:
> > > 
> > > barebox:/
> > > usb USB: scanning bus for
> > > devices... Bus 001 Device 001: ID 0000:0000 EHCI Host
> > > Controller Bus 001 Device 002: ID
> > > 0424:2514 Bus 001 Device 003: ID 413c:2112 Dell USB Wired
> > > Multimedia Keybo usb-keyboard usb1-0-0: USB keyboard
> > > found Bus 001 Device 004: ID 8564:1000 Mass Storage
> > > Device Using index 0 for the new
> > > disk usb-keyboard usb1-0-0: submit_int_msg: re-entrance 1
> > > (usb-hub:usb1) usb-keyboard usb1-0-0: submit_int_msg: re-entrance 1
> > > (usb-hub:usb1) usb-keyboard usb1-0-0: submit_int_msg: re-entrance 1
> > > (usb-keyboard:usb1-0-0) usb-keyboard usb1-0-0: submit_int_msg:
> > > re-entrance 1 (usb-keyboard:usb1-0-0) usb-keyboard usb1-0-0:
> > > submit_int_msg: re-entrance 1 (usb-hub:usb1) usb-keyboard usb1-0-0:
> > > submit_int_msg: re-entrance 1 (usb-keyboard:usb1-0-0) usb-keyboard
> > > usb1-0-0: submit_int_msg: re-entrance 1 (usb-keyboard:usb1-0-0) Bus
> > > 001 Device 005: ID 0424:2514 5 USB Device(s)
> > > found                                                       
> > > 
> > > According to the debug messages: submit_int_msg() is called by the
> > > usb-keyboard driver (from the poller function), while "usb-hub"
> > > driver is executing submit_control_msg() (which calls mdelay() and
> > > poller_call() subsequently). This occurs several times, until usb
> > > bus scan is completed. Probably the problem can be reproduced by
> > > adding more usb devices.
> > 
> > I wonder why this does not happen here. I finally could reproduce this
> > by adding some additional delays to the ehci driver.
> > 
> > We should probably move the reentrance detection to the generic usb
> > layer to usb_submit_int_msg, usb_control_msg and usb_bulk_msg. This
> > would avoid running into the same problems in the other usb host
> > drivers. If I understand the issue correctly we could just use regular
> > *delay in the host drivers when we detect reentrancy correctly, right?
> > I mean we don't have to print an error message when we detect
> > reentrance, but instead just consider that a case that can happen.
> 
> We can return -EAGAIN on reentrance detection in usb_* functions, but
> the usb device driver should distinguish this case and do sane
> things, i.e. it should not fail, but skip the action.

Yes, that sounds like a sane approach to me.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/5] usb: ehci-hcd: use mdelay_non_inerruptible()

[Jean-Christophe PLAGNIOL-VILLARD]

On 19:52 Wed 07 Oct     , Peter Mamonov wrote:
> On Wed, 7 Oct 2015 17:40:24 +0200
> Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:
> 
> > On 17:40 Wed 07 Oct     , Peter Mamonov wrote:
> > > On Wed, 7 Oct 2015 15:47:03 +0200
> > > Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:
> > > 
> > > > On 18:58 Tue 22 Sep     , Peter Mamonov wrote:
> > > > > Signed-off-by: Peter Mamonov <pmamonov@gmail.com>
> > > > > ---
> > > > >  drivers/usb/host/ehci-hcd.c | 6 +++---
> > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/usb/host/ehci-hcd.c
> > > > > b/drivers/usb/host/ehci-hcd.c index d6df7b8..03d6150 100644
> > > > > --- a/drivers/usb/host/ehci-hcd.c
> > > > > +++ b/drivers/usb/host/ehci-hcd.c
> > > > > @@ -684,7 +684,7 @@ ehci_submit_root(struct usb_device *dev,
> > > > > unsigned long pipe, void *buffer,
> > > > >  				 * root
> > > > >  				 */
> > > > >  				ehci_powerup_fixup(ehci);
> > > > > -				mdelay(50);
> > > > > +				mdelay_non_interruptible(50);
> > > > >  				ehci->portreset |= 1 << port;
> > > > >  				/* terminate the reset */
> > > > >  				ehci_writel(status_reg, reg &
> > > > > ~EHCI_PS_PR); @@ -747,7 +747,7 @@ ehci_submit_root(struct
> > > > > usb_device *dev, unsigned long pipe, void *buffer, goto unknown;
> > > > >  	}
> > > > >  
> > > > > -	mdelay(1);
> > > > > +	mdelay_non_interruptible(1);
> > > > >  	len = min3(srclen, (int)le16_to_cpu(req->length),
> > > > > length); if (srcptr != NULL && len > 0)
> > > > >  		memcpy(buffer, srcptr, len);
> > > > > @@ -889,7 +889,7 @@ static int ehci_init(struct usb_host *host)
> > > > >  	ehci_writel(&ehci->hcor->or_configflag, cmd);
> > > > >  	/* unblock posted write */
> > > > >  	cmd = ehci_readl(&ehci->hcor->or_usbcmd);
> > > > > -	mdelay(5);
> > > > > +	mdelay_non_interruptible(5);
> > > > why do you need that much non interruptible delau?
> > > 
> > > Non-interruptible delays are needed here to prevent ehci_* functions
> > > re-entrance. The re-entrance occurs during a usb bus scan, after
> > > detection of a usb keyboard. As soon as a USB keyboard is detected,
> > > it's driver starts the poller, which interferes with the process of
> > > usb bus scan. However that last one delay may be interruptible.
> > 
> > my problem is as soon as you start a usb control msg you block
> > everything
> > 
> > nothing else can run in barebox
> > 
> > can slow down barebox boot
> > 
> > I do think we need a real mdelay_non_interruptible feature but just
> > forbidden to recall usb control msg.
> > But the rest of barebox can run durring those 5ms
> 
> Well, I think it can be done by returning -EAGAIN on re-entrance
> detection in ehci_* functions [1], and skipping the poll in the keyboard
> driver.
> 
> By the way, could you clarify: do you really experience
> considerable barebox slowdown or you make an assumtion based on the
> code analysis?

I've wrote similar code for barebox nearly 1 year ago and did notice it

Best Regards,
J.
> 
> [1]
> http://lists.infradead.org/pipermail/barebox/2015-September/024715.html
> 
> Regards,
> Peter
> 
> > 
> > Best Regards,
> > J.
> 

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/5] usb: ehci-hcd: use mdelay_non_inerruptible()

[Antony Pavlov]

On Tue, 13 Oct 2015 04:04:28 +0200
Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:

> On 19:52 Wed 07 Oct     , Peter Mamonov wrote:
> > On Wed, 7 Oct 2015 17:40:24 +0200
> > Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:
> > 
> > > On 17:40 Wed 07 Oct     , Peter Mamonov wrote:
> > > > On Wed, 7 Oct 2015 15:47:03 +0200
> > > > Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:
> > > > 
> > > > > On 18:58 Tue 22 Sep     , Peter Mamonov wrote:
> > > > > > Signed-off-by: Peter Mamonov <pmamonov@gmail.com>
> > > > > > ---
> > > > > >  drivers/usb/host/ehci-hcd.c | 6 +++---
> > > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/usb/host/ehci-hcd.c
> > > > > > b/drivers/usb/host/ehci-hcd.c index d6df7b8..03d6150 100644
> > > > > > --- a/drivers/usb/host/ehci-hcd.c
> > > > > > +++ b/drivers/usb/host/ehci-hcd.c
> > > > > > @@ -684,7 +684,7 @@ ehci_submit_root(struct usb_device *dev,
> > > > > > unsigned long pipe, void *buffer,
> > > > > >  				 * root
> > > > > >  				 */
> > > > > >  				ehci_powerup_fixup(ehci);
> > > > > > -				mdelay(50);
> > > > > > +				mdelay_non_interruptible(50);
> > > > > >  				ehci->portreset |= 1 << port;
> > > > > >  				/* terminate the reset */
> > > > > >  				ehci_writel(status_reg, reg &
> > > > > > ~EHCI_PS_PR); @@ -747,7 +747,7 @@ ehci_submit_root(struct
> > > > > > usb_device *dev, unsigned long pipe, void *buffer, goto unknown;
> > > > > >  	}
> > > > > >  
> > > > > > -	mdelay(1);
> > > > > > +	mdelay_non_interruptible(1);
> > > > > >  	len = min3(srclen, (int)le16_to_cpu(req->length),
> > > > > > length); if (srcptr != NULL && len > 0)
> > > > > >  		memcpy(buffer, srcptr, len);
> > > > > > @@ -889,7 +889,7 @@ static int ehci_init(struct usb_host *host)
> > > > > >  	ehci_writel(&ehci->hcor->or_configflag, cmd);
> > > > > >  	/* unblock posted write */
> > > > > >  	cmd = ehci_readl(&ehci->hcor->or_usbcmd);
> > > > > > -	mdelay(5);
> > > > > > +	mdelay_non_interruptible(5);
> > > > > why do you need that much non interruptible delau?
> > > > 
> > > > Non-interruptible delays are needed here to prevent ehci_* functions
> > > > re-entrance. The re-entrance occurs during a usb bus scan, after
> > > > detection of a usb keyboard. As soon as a USB keyboard is detected,
> > > > it's driver starts the poller, which interferes with the process of
> > > > usb bus scan. However that last one delay may be interruptible.
> > > 
> > > my problem is as soon as you start a usb control msg you block
> > > everything
> > > 
> > > nothing else can run in barebox
> > > 
> > > can slow down barebox boot
> > > 
> > > I do think we need a real mdelay_non_interruptible feature but just
> > > forbidden to recall usb control msg.
> > > But the rest of barebox can run durring those 5ms
> > 
> > Well, I think it can be done by returning -EAGAIN on re-entrance
> > detection in ehci_* functions [1], and skipping the poll in the keyboard
> > driver.
> > 
> > By the way, could you clarify: do you really experience
> > considerable barebox slowdown or you make an assumtion based on the
> > code analysis?
> 
> I've wrote similar code for barebox nearly 1 year ago and did notice it

So you have not tested Peter's code.
Could you please test latest next branch and sand a word on slowdown?


> Best Regards,
> J.
> > 
> > [1]
> > http://lists.infradead.org/pipermail/barebox/2015-September/024715.html
> > 
> > Regards,
> > Peter
> > 
> > > 
> > > Best Regards,
> > > J.
> > 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox


-- 
-- 
Best regards,
  Antony Pavlov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox