Skip to contentSkip to conversation

USB: Fix EHCI to work with data cache enabled

PATCH New Marek Vasut 5 participants 15 messages Apr 9, 2012 – May 29, 2012

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover USB: Fix EHCI to work with data cache enabled

This patchset reworks the USB core and EHCI-HCD to work with data caches.

Marek Vasut (3):
USB: Drop ehci_alloc/ehci_free in ehci-hcd
USB: Drop cache flush bloat in EHCI-HCD
USB: Document the QH and qTD antics in EHCI-HCD

Puneet Saxena (1):
USB: Align buffers at cacheline

 common/cmd_usb.c            |    3 +-
 common/usb.c                |   22 ++--
 common/usb_hub.c            |   27 ++--
 common/usb_storage.c        |   59 +++++-----
 disk/part_dos.c             |    2 +-
 drivers/usb/host/ehci-hcd.c |  284 ++++++++++++++-----------------------------
 include/scsi.h              |    4 +-
 include/usb.h               |    4 +-
 8 files changed, 155 insertions(+), 250 deletions(-)
Cc Puneet Saxena <[email protected]>
Signature · 1 line
1.7.9.1
#1 lore raw

PATCH 1/4 USB: Align buffers at cacheline

From: Puneet Saxena <[email protected]>

This avoids cache-alignment warnings shown in console when a usb command is entered.

Whenever X bytes of unaligned buffer is invalidated, arm core invalidates X + Y bytes as per the cache line size and throws these warnings.

---
 common/cmd_usb.c     |    3 +-
 common/usb.c         |   22 +++++++++---------
 common/usb_hub.c     |   27 +++++++++++-----------
 common/usb_storage.c |   59 +++++++++++++++++++++++--------------------------
 disk/part_dos.c      |    2 +-
 include/scsi.h       |    4 ++-
 include/usb.h        |    4 ++-
 7 files changed, 62 insertions(+), 59 deletions(-)
common/cmd_usb.c +2 -1
index 9eba271..a8e3ae5 100644
@@ -150,7 +150,8 @@ void usb_display_class_sub(unsigned char dclass, unsigned char subclass,
 
 void usb_display_string(struct usb_device *dev, int index)
 {
-	char buffer[256];
+	ALLOC_CACHE_ALIGN_BUFFER(char, buffer, 256);
+
 	if (index != 0) {
 		if (usb_string(dev, index, &buffer[0], 256) > 0)
 			printf("String: \"%s\"", buffer);
common/usb.c +11 -11
index 1ec30bc..87ffb79 100644
@@ -169,7 +169,7 @@ int usb_control_msg(struct usb_device *dev, unsigned int pipe,
 			unsigned short value, unsigned short index,
 			void *data, unsigned short size, int timeout)
 {
-	struct devrequest setup_packet;
+	ALLOC_CACHE_ALIGN_BUFFER(struct devrequest, setup_packet, 1);
 
 	if ((timeout == 0) && (!asynch_allowed)) {
 		/* request for a asynch control pipe is not allowed */
@@ -177,17 +177,17 @@ int usb_control_msg(struct usb_device *dev, unsigned int pipe,
 	}
 
 	/* set setup command */
-	setup_packet.requesttype = requesttype;
-	setup_packet.request = request;
-	setup_packet.value = cpu_to_le16(value);
-	setup_packet.index = cpu_to_le16(index);
-	setup_packet.length = cpu_to_le16(size);
+	setup_packet->requesttype = requesttype;
+	setup_packet->request = request;
+	setup_packet->value = cpu_to_le16(value);
+	setup_packet->index = cpu_to_le16(index);
+	setup_packet->length = cpu_to_le16(size);
 	USB_PRINTF("usb_control_msg: request: 0x%X, requesttype: 0x%X, " \
 		   "value 0x%X index 0x%X length 0x%X\n",
 		   request, requesttype, value, index, size);
 	dev->status = USB_ST_NOT_PROC; /*not yet processed */
 
-	submit_control_msg(dev, pipe, data, size, &setup_packet);
+	submit_control_msg(dev, pipe, data, size, setup_packet);
 	if (timeout == 0)
 		return (int)size;
 
@@ -681,7 +681,7 @@ static int usb_string_sub(struct usb_device *dev, unsigned int langid,
  */
 int usb_string(struct usb_device *dev, int index, char *buf, size_t size)
 {
-	unsigned char mybuf[USB_BUFSIZ];
+	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, mybuf, USB_BUFSIZ);
 	unsigned char *tbuf;
 	int err;
 	unsigned int u, idx;
@@ -781,7 +781,7 @@ int usb_new_device(struct usb_device *dev)
 {
 	int addr, err;
 	int tmp;
-	unsigned char tmpbuf[USB_BUFSIZ];
+	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, tmpbuf, USB_BUFSIZ);
 
 	/* We still haven't set the Address yet */
 	addr = dev->devnum;
@@ -908,8 +908,8 @@ int usb_new_device(struct usb_device *dev)
 	le16_to_cpus(&dev->descriptor.idProduct);
 	le16_to_cpus(&dev->descriptor.bcdDevice);
 	/* only support for one config for now */
-	usb_get_configuration_no(dev, &tmpbuf[0], 0);
-	usb_parse_config(dev, &tmpbuf[0], 0);
+	usb_get_configuration_no(dev, tmpbuf, 0);
+	usb_parse_config(dev, tmpbuf, 0);
 	usb_set_maxpacket(dev);
 	/* we set the default configuration here */
 	if (usb_set_configuration(dev, dev->config.desc.bConfigurationValue)) {
common/usb_hub.c +14 -13
index e0edaad..f35ad95 100644
@@ -153,7 +153,7 @@ int hub_port_reset(struct usb_device *dev, int port,
 			unsigned short *portstat)
 {
 	int tries;
-	struct usb_port_status portsts;
+	ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1);
 	unsigned short portstatus, portchange;
 
 	USB_HUB_PRINTF("hub_port_reset: resetting port %d...\n", port);
@@ -162,13 +162,13 @@ int hub_port_reset(struct usb_device *dev, int port,
 		usb_set_port_feature(dev, port + 1, USB_PORT_FEAT_RESET);
 		mdelay(200);
 
-		if (usb_get_port_status(dev, port + 1, &portsts) < 0) {
+		if (usb_get_port_status(dev, port + 1, portsts) < 0) {
 			USB_HUB_PRINTF("get_port_status failed status %lX\n",
 					dev->status);
 			return -1;
 		}
-		portstatus = le16_to_cpu(portsts.wPortStatus);
-		portchange = le16_to_cpu(portsts.wPortChange);
+		portstatus = le16_to_cpu(portsts->wPortStatus);
+		portchange = le16_to_cpu(portsts->wPortChange);
 
 		USB_HUB_PRINTF("portstatus %x, change %x, %s\n",
 				portstatus, portchange,
@@ -206,19 +206,19 @@ int hub_port_reset(struct usb_device *dev, int port,
 void usb_hub_port_connect_change(struct usb_device *dev, int port)
 {
 	struct usb_device *usb;
-	struct usb_port_status portsts;
+	ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1);
 	unsigned short portstatus;
 
 	/* Check status */
-	if (usb_get_port_status(dev, port + 1, &portsts) < 0) {
+	if (usb_get_port_status(dev, port + 1, portsts) < 0) {
 		USB_HUB_PRINTF("get_port_status failed\n");
 		return;
 	}
 
-	portstatus = le16_to_cpu(portsts.wPortStatus);
+	portstatus = le16_to_cpu(portsts->wPortStatus);
 	USB_HUB_PRINTF("portstatus %x, change %x, %s\n",
 			portstatus,
-			le16_to_cpu(portsts.wPortChange),
+			le16_to_cpu(portsts->wPortChange),
 			portspeed(portstatus));
 
 	/* Clear the connection change status */
@@ -267,7 +267,8 @@ void usb_hub_port_connect_change(struct usb_device *dev, int port)
 static int usb_hub_configure(struct usb_device *dev)
 {
 	int i;
-	unsigned char buffer[USB_BUFSIZ], *bitmap;
+	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, USB_BUFSIZ);
+	unsigned char *bitmap;
 	struct usb_hub_descriptor *descriptor;
 	struct usb_hub_device *hub;
 #ifdef USB_HUB_DEBUG
@@ -389,16 +390,16 @@ static int usb_hub_configure(struct usb_device *dev)
 	usb_hub_power_on(hub);
 
 	for (i = 0; i < dev->maxchild; i++) {
-		struct usb_port_status portsts;
+		ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1);
 		unsigned short portstatus, portchange;
 
-		if (usb_get_port_status(dev, i + 1, &portsts) < 0) {
+		if (usb_get_port_status(dev, i + 1, portsts) < 0) {
 			USB_HUB_PRINTF("get_port_status failed\n");
 			continue;
 		}
 
-		portstatus = le16_to_cpu(portsts.wPortStatus);
-		portchange = le16_to_cpu(portsts.wPortChange);
+		portstatus = le16_to_cpu(portsts->wPortStatus);
+		portchange = le16_to_cpu(portsts->wPortChange);
 		USB_HUB_PRINTF("Port %d Status %X Change %X\n",
 				i + 1, portstatus, portchange);
 
common/usb_storage.c +28 -31

150 lines not on this page · raw message

disk/part_dos.c +1 -1
index b5bcb37..70211ee 100644
@@ -87,7 +87,7 @@ static int test_block_type(unsigned char *buffer)
 
 int test_part_dos (block_dev_desc_t *dev_desc)
 {
-	unsigned char buffer[dev_desc->blksz];
+	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, dev_desc->blksz);
 
 	if ((dev_desc->block_read(dev_desc->dev, 0, 1, (ulong *) buffer) != 1) ||
 	    (buffer[DOS_PART_MAGIC_OFFSET + 0] != 0x55) ||
include/scsi.h +3 -1
index c52759c..89ae45f 100644
@@ -26,7 +26,9 @@
 
 typedef struct SCSI_cmd_block{
 	unsigned char		cmd[16];					/* command				   */
-	unsigned char		sense_buf[64];		/* for request sense */
+	/* for request sense */
+	unsigned char		sense_buf[64]
+		__attribute__((aligned(ARCH_DMA_MINALIGN)));
 	unsigned char		status;						/* SCSI Status			 */
 	unsigned char		target;						/* Target ID				 */
 	unsigned char		lun;							/* Target LUN        */
include/usb.h +3 -1
index 48e4bcd..6da91e7 100644
@@ -109,7 +109,9 @@ struct usb_device {
 	int epmaxpacketout[16];		/* OUTput endpoint specific maximums */
 
 	int configno;			/* selected config number */
-	struct usb_device_descriptor descriptor; /* Device Descriptor */
+	/* Device Descriptor */
+	struct usb_device_descriptor descriptor
+		__attribute__((aligned(ARCH_DMA_MINALIGN)));
 	struct usb_config config; /* config descriptor */
 
 	int have_langid;		/* whether string_langid is valid yet */
Signature · 1 line
1.7.9.1
#2 lore raw

PATCH v2 1/4 USB: Align buffers at cacheline

From: Puneet Saxena <[email protected]>

This avoids cache-alignment warnings shown in console when a usb command is entered.

Whenever X bytes of unaligned buffer is invalidated, arm core invalidates X + Y bytes as per the cache line size and throws these warnings.

---
 common/cmd_usb.c     |    3 +-
 common/usb.c         |   22 +++++++++---------
 common/usb_hub.c     |   27 +++++++++++-----------
 common/usb_storage.c |   59 +++++++++++++++++++++++--------------------------
 disk/part_dos.c      |    6 ++--
 include/scsi.h       |    4 ++-
 include/usb.h        |    4 ++-
 7 files changed, 64 insertions(+), 61 deletions(-)

V2: Fixup part_dos.c, properly align all buffers!

common/cmd_usb.c +2 -1
index 9eba271..a8e3ae5 100644
@@ -150,7 +150,8 @@ void usb_display_class_sub(unsigned char dclass, unsigned char subclass,
 
 void usb_display_string(struct usb_device *dev, int index)
 {
-	char buffer[256];
+	ALLOC_CACHE_ALIGN_BUFFER(char, buffer, 256);
+
 	if (index != 0) {
 		if (usb_string(dev, index, &buffer[0], 256) > 0)
 			printf("String: \"%s\"", buffer);
common/usb.c +11 -11
index 1ec30bc..87ffb79 100644
@@ -169,7 +169,7 @@ int usb_control_msg(struct usb_device *dev, unsigned int pipe,
 			unsigned short value, unsigned short index,
 			void *data, unsigned short size, int timeout)
 {
-	struct devrequest setup_packet;
+	ALLOC_CACHE_ALIGN_BUFFER(struct devrequest, setup_packet, 1);
 
 	if ((timeout == 0) && (!asynch_allowed)) {
 		/* request for a asynch control pipe is not allowed */
@@ -177,17 +177,17 @@ int usb_control_msg(struct usb_device *dev, unsigned int pipe,
 	}
 
 	/* set setup command */
-	setup_packet.requesttype = requesttype;
-	setup_packet.request = request;
-	setup_packet.value = cpu_to_le16(value);
-	setup_packet.index = cpu_to_le16(index);
-	setup_packet.length = cpu_to_le16(size);
+	setup_packet->requesttype = requesttype;
+	setup_packet->request = request;
+	setup_packet->value = cpu_to_le16(value);
+	setup_packet->index = cpu_to_le16(index);
+	setup_packet->length = cpu_to_le16(size);
 	USB_PRINTF("usb_control_msg: request: 0x%X, requesttype: 0x%X, " \
 		   "value 0x%X index 0x%X length 0x%X\n",
 		   request, requesttype, value, index, size);
 	dev->status = USB_ST_NOT_PROC; /*not yet processed */
 
-	submit_control_msg(dev, pipe, data, size, &setup_packet);
+	submit_control_msg(dev, pipe, data, size, setup_packet);
 	if (timeout == 0)
 		return (int)size;
 
@@ -681,7 +681,7 @@ static int usb_string_sub(struct usb_device *dev, unsigned int langid,
  */
 int usb_string(struct usb_device *dev, int index, char *buf, size_t size)
 {
-	unsigned char mybuf[USB_BUFSIZ];
+	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, mybuf, USB_BUFSIZ);
 	unsigned char *tbuf;
 	int err;
 	unsigned int u, idx;
@@ -781,7 +781,7 @@ int usb_new_device(struct usb_device *dev)
 {
 	int addr, err;
 	int tmp;
-	unsigned char tmpbuf[USB_BUFSIZ];
+	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, tmpbuf, USB_BUFSIZ);
 
 	/* We still haven't set the Address yet */
 	addr = dev->devnum;
@@ -908,8 +908,8 @@ int usb_new_device(struct usb_device *dev)
 	le16_to_cpus(&dev->descriptor.idProduct);
 	le16_to_cpus(&dev->descriptor.bcdDevice);
 	/* only support for one config for now */
-	usb_get_configuration_no(dev, &tmpbuf[0], 0);
-	usb_parse_config(dev, &tmpbuf[0], 0);
+	usb_get_configuration_no(dev, tmpbuf, 0);
+	usb_parse_config(dev, tmpbuf, 0);
 	usb_set_maxpacket(dev);
 	/* we set the default configuration here */
 	if (usb_set_configuration(dev, dev->config.desc.bConfigurationValue)) {
common/usb_hub.c +14 -13
index e0edaad..f35ad95 100644
@@ -153,7 +153,7 @@ int hub_port_reset(struct usb_device *dev, int port,
 			unsigned short *portstat)
 {
 	int tries;
-	struct usb_port_status portsts;
+	ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1);
 	unsigned short portstatus, portchange;
 
 	USB_HUB_PRINTF("hub_port_reset: resetting port %d...\n", port);
@@ -162,13 +162,13 @@ int hub_port_reset(struct usb_device *dev, int port,
 		usb_set_port_feature(dev, port + 1, USB_PORT_FEAT_RESET);
 		mdelay(200);
 
-		if (usb_get_port_status(dev, port + 1, &portsts) < 0) {
+		if (usb_get_port_status(dev, port + 1, portsts) < 0) {
 			USB_HUB_PRINTF("get_port_status failed status %lX\n",
 					dev->status);
 			return -1;
 		}
-		portstatus = le16_to_cpu(portsts.wPortStatus);
-		portchange = le16_to_cpu(portsts.wPortChange);
+		portstatus = le16_to_cpu(portsts->wPortStatus);
+		portchange = le16_to_cpu(portsts->wPortChange);
 
 		USB_HUB_PRINTF("portstatus %x, change %x, %s\n",
 				portstatus, portchange,
@@ -206,19 +206,19 @@ int hub_port_reset(struct usb_device *dev, int port,
 void usb_hub_port_connect_change(struct usb_device *dev, int port)
 {
 	struct usb_device *usb;
-	struct usb_port_status portsts;
+	ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1);
 	unsigned short portstatus;
 
 	/* Check status */
-	if (usb_get_port_status(dev, port + 1, &portsts) < 0) {
+	if (usb_get_port_status(dev, port + 1, portsts) < 0) {
 		USB_HUB_PRINTF("get_port_status failed\n");
 		return;
 	}
 
-	portstatus = le16_to_cpu(portsts.wPortStatus);
+	portstatus = le16_to_cpu(portsts->wPortStatus);
 	USB_HUB_PRINTF("portstatus %x, change %x, %s\n",
 			portstatus,
-			le16_to_cpu(portsts.wPortChange),
+			le16_to_cpu(portsts->wPortChange),
 			portspeed(portstatus));
 
 	/* Clear the connection change status */
@@ -267,7 +267,8 @@ void usb_hub_port_connect_change(struct usb_device *dev, int port)
 static int usb_hub_configure(struct usb_device *dev)
 {
 	int i;
-	unsigned char buffer[USB_BUFSIZ], *bitmap;
+	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, USB_BUFSIZ);
+	unsigned char *bitmap;
 	struct usb_hub_descriptor *descriptor;
 	struct usb_hub_device *hub;
 #ifdef USB_HUB_DEBUG
@@ -389,16 +390,16 @@ static int usb_hub_configure(struct usb_device *dev)
 	usb_hub_power_on(hub);
 
 	for (i = 0; i < dev->maxchild; i++) {
-		struct usb_port_status portsts;
+		ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1);
 		unsigned short portstatus, portchange;
 
-		if (usb_get_port_status(dev, i + 1, &portsts) < 0) {
+		if (usb_get_port_status(dev, i + 1, portsts) < 0) {
 			USB_HUB_PRINTF("get_port_status failed\n");
 			continue;
 		}
 
-		portstatus = le16_to_cpu(portsts.wPortStatus);
-		portchange = le16_to_cpu(portsts.wPortChange);
+		portstatus = le16_to_cpu(portsts->wPortStatus);
+		portchange = le16_to_cpu(portsts->wPortChange);
 		USB_HUB_PRINTF("Port %d Status %X Change %X\n",
 				i + 1, portstatus, portchange);
 
common/usb_storage.c +28 -31

150 lines not on this page · raw message

disk/part_dos.c +3 -3
index b5bcb37..c028aaf 100644
@@ -87,7 +87,7 @@ static int test_block_type(unsigned char *buffer)
 
 int test_part_dos (block_dev_desc_t *dev_desc)
 {
-	unsigned char buffer[dev_desc->blksz];
+	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, dev_desc->blksz);
 
 	if ((dev_desc->block_read(dev_desc->dev, 0, 1, (ulong *) buffer) != 1) ||
 	    (buffer[DOS_PART_MAGIC_OFFSET + 0] != 0x55) ||
@@ -102,7 +102,7 @@ int test_part_dos (block_dev_desc_t *dev_desc)
 static void print_partition_extended (block_dev_desc_t *dev_desc, int ext_part_sector, int relative,
 							   int part_num)
 {
-	unsigned char buffer[dev_desc->blksz];
+	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, dev_desc->blksz);
 	dos_partition_t *pt;
 	int i;
 
@@ -166,7 +166,7 @@ static int get_partition_info_extended (block_dev_desc_t *dev_desc, int ext_part
 				 int relative, int part_num,
 				 int which_part, disk_partition_t *info)
 {
-	unsigned char buffer[dev_desc->blksz];
+	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, dev_desc->blksz);
 	dos_partition_t *pt;
 	int i;
 
include/scsi.h +3 -1
index c52759c..89ae45f 100644
@@ -26,7 +26,9 @@
 
 typedef struct SCSI_cmd_block{
 	unsigned char		cmd[16];					/* command				   */
-	unsigned char		sense_buf[64];		/* for request sense */
+	/* for request sense */
+	unsigned char		sense_buf[64]
+		__attribute__((aligned(ARCH_DMA_MINALIGN)));
 	unsigned char		status;						/* SCSI Status			 */
 	unsigned char		target;						/* Target ID				 */
 	unsigned char		lun;							/* Target LUN        */
include/usb.h +3 -1
index 48e4bcd..6da91e7 100644
@@ -109,7 +109,9 @@ struct usb_device {
 	int epmaxpacketout[16];		/* OUTput endpoint specific maximums */
 
 	int configno;			/* selected config number */
-	struct usb_device_descriptor descriptor; /* Device Descriptor */
+	/* Device Descriptor */
+	struct usb_device_descriptor descriptor
+		__attribute__((aligned(ARCH_DMA_MINALIGN)));
 	struct usb_config config; /* config descriptor */
 
 	int have_langid;		/* whether string_langid is valid yet */
Signature · 1 line
1.7.9.1
#3 lore raw

PATCH 2/4 USB: Drop ehci_alloc/ehci_free in ehci-hcd

In reply to Marek Vasut

These two functions were called only from ehci_submit_async(), therefore dissolve them as part of ehci_submit_async() to get rid of all those static variables.

---
 drivers/usb/host/ehci-hcd.c |  123 +++++++++++-------------------------------
 1 files changed, 32 insertions(+), 91 deletions(-)
drivers/usb/host/ehci-hcd.c +32 -91
index b6422d7..8ac0b44 100644
@@ -225,11 +225,6 @@ static int handshake(uint32_t *ptr, uint32_t mask, uint32_t done, int usec)
 	return -1;
 }
 
-static void ehci_free(void *p, size_t sz)
-{
-
-}
-
 static int ehci_reset(void)
 {
 	uint32_t cmd;
@@ -266,35 +261,6 @@ out:
 	return ret;
 }
 
-static void *ehci_alloc(size_t sz, size_t align)
-{
-	static struct QH qh __attribute__((aligned(32)));
-	static struct qTD td[3] __attribute__((aligned (32)));
-	static int ntds;
-	void *p;
-
-	switch (sz) {
-	case sizeof(struct QH):
-		p = &qh;
-		ntds = 0;
-		break;
-	case sizeof(struct qTD):
-		if (ntds == 3) {
-			debug("out of TDs\n");
-			return NULL;
-		}
-		p = &td[ntds];
-		ntds++;
-		break;
-	default:
-		debug("unknown allocation size\n");
-		return NULL;
-	}
-
-	memset(p, 0, sz);
-	return p;
-}
-
 static int ehci_td_buffer(struct qTD *td, void *buf, size_t sz)
 {
 	uint32_t addr, delta, next;
@@ -326,8 +292,10 @@ static int
 ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer,
 		   int length, struct devrequest *req)
 {
-	struct QH *qh;
-	struct qTD *td;
+	static struct QH qh __attribute__((aligned(32)));
+	static struct qTD qtd[3] __attribute__((aligned (32)));
+	int qtd_counter = 0;
+
 	volatile struct qTD *vtd;
 	unsigned long ts;
 	uint32_t *tdp;
@@ -346,12 +314,10 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer,
 		      le16_to_cpu(req->value), le16_to_cpu(req->value),
 		      le16_to_cpu(req->index));
 
-	qh = ehci_alloc(sizeof(struct QH), 32);
-	if (qh == NULL) {
-		debug("unable to allocate QH\n");
-		return -1;
-	}
-	qh->qh_link = cpu_to_hc32((uint32_t)&qh_list | QH_LINK_TYPE_QH);
+	memset(&qh, 0, sizeof(struct QH));
+	memset(qtd, 0, sizeof(qtd));
+
+	qh.qh_link = cpu_to_hc32((uint32_t)&qh_list | QH_LINK_TYPE_QH);
 	c = (usb_pipespeed(pipe) != USB_SPEED_HIGH &&
 	     usb_pipeendpoint(pipe) == 0) ? 1 : 0;
 	endpt = (8 << 28) |
@@ -362,85 +328,67 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer,
 	    (usb_pipespeed(pipe) << 12) |
 	    (usb_pipeendpoint(pipe) << 8) |
 	    (0 << 7) | (usb_pipedevice(pipe) << 0);
-	qh->qh_endpt1 = cpu_to_hc32(endpt);
+	qh.qh_endpt1 = cpu_to_hc32(endpt);
 	endpt = (1 << 30) |
 	    (dev->portnr << 23) |
 	    (dev->parent->devnum << 16) | (0 << 8) | (0 << 0);
-	qh->qh_endpt2 = cpu_to_hc32(endpt);
-	qh->qh_overlay.qt_next = cpu_to_hc32(QT_NEXT_TERMINATE);
+	qh.qh_endpt2 = cpu_to_hc32(endpt);
+	qh.qh_overlay.qt_next = cpu_to_hc32(QT_NEXT_TERMINATE);
 
-	td = NULL;
-	tdp = &qh->qh_overlay.qt_next;
+	tdp = &qh.qh_overlay.qt_next;
 
 	toggle =
 	    usb_gettoggle(dev, usb_pipeendpoint(pipe), usb_pipeout(pipe));
 
 	if (req != NULL) {
-		td = ehci_alloc(sizeof(struct qTD), 32);
-		if (td == NULL) {
-			debug("unable to allocate SETUP td\n");
-			goto fail;
-		}
-		td->qt_next = cpu_to_hc32(QT_NEXT_TERMINATE);
-		td->qt_altnext = cpu_to_hc32(QT_NEXT_TERMINATE);
+		qtd[qtd_counter].qt_next = cpu_to_hc32(QT_NEXT_TERMINATE);
+		qtd[qtd_counter].qt_altnext = cpu_to_hc32(QT_NEXT_TERMINATE);
 		token = (0 << 31) |
 		    (sizeof(*req) << 16) |
 		    (0 << 15) | (0 << 12) | (3 << 10) | (2 << 8) | (0x80 << 0);
-		td->qt_token = cpu_to_hc32(token);
-		if (ehci_td_buffer(td, req, sizeof(*req)) != 0) {
+		qtd[qtd_counter].qt_token = cpu_to_hc32(token);
+		if (ehci_td_buffer(&qtd[qtd_counter], req, sizeof(*req)) != 0) {
 			debug("unable construct SETUP td\n");
-			ehci_free(td, sizeof(*td));
 			goto fail;
 		}
-		*tdp = cpu_to_hc32((uint32_t) td);
-		tdp = &td->qt_next;
+		*tdp = cpu_to_hc32((uint32_t)&qtd[qtd_counter]);
+		tdp = &qtd[qtd_counter++].qt_next;
 		toggle = 1;
 	}
 
 	if (length > 0 || req == NULL) {
-		td = ehci_alloc(sizeof(struct qTD), 32);
-		if (td == NULL) {
-			debug("unable to allocate DATA td\n");
-			goto fail;
-		}
-		td->qt_next = cpu_to_hc32(QT_NEXT_TERMINATE);
-		td->qt_altnext = cpu_to_hc32(QT_NEXT_TERMINATE);
+		qtd[qtd_counter].qt_next = cpu_to_hc32(QT_NEXT_TERMINATE);
+		qtd[qtd_counter].qt_altnext = cpu_to_hc32(QT_NEXT_TERMINATE);
 		token = (toggle << 31) |
 		    (length << 16) |
 		    ((req == NULL ? 1 : 0) << 15) |
 		    (0 << 12) |
 		    (3 << 10) |
 		    ((usb_pipein(pipe) ? 1 : 0) << 8) | (0x80 << 0);
-		td->qt_token = cpu_to_hc32(token);
-		if (ehci_td_buffer(td, buffer, length) != 0) {
+		qtd[qtd_counter].qt_token = cpu_to_hc32(token);
+		if (ehci_td_buffer(&qtd[qtd_counter], buffer, length) != 0) {
 			debug("unable construct DATA td\n");
-			ehci_free(td, sizeof(*td));
 			goto fail;
 		}
-		*tdp = cpu_to_hc32((uint32_t) td);
-		tdp = &td->qt_next;
+		*tdp = cpu_to_hc32((uint32_t)&qtd[qtd_counter]);
+		tdp = &qtd[qtd_counter++].qt_next;
 	}
 
 	if (req != NULL) {
-		td = ehci_alloc(sizeof(struct qTD), 32);
-		if (td == NULL) {
-			debug("unable to allocate ACK td\n");
-			goto fail;
-		}
-		td->qt_next = cpu_to_hc32(QT_NEXT_TERMINATE);
-		td->qt_altnext = cpu_to_hc32(QT_NEXT_TERMINATE);
+		qtd[qtd_counter].qt_next = cpu_to_hc32(QT_NEXT_TERMINATE);
+		qtd[qtd_counter].qt_altnext = cpu_to_hc32(QT_NEXT_TERMINATE);
 		token = (toggle << 31) |
 		    (0 << 16) |
 		    (1 << 15) |
 		    (0 << 12) |
 		    (3 << 10) |
 		    ((usb_pipein(pipe) ? 0 : 1) << 8) | (0x80 << 0);
-		td->qt_token = cpu_to_hc32(token);
-		*tdp = cpu_to_hc32((uint32_t) td);
-		tdp = &td->qt_next;
+		qtd[qtd_counter].qt_token = cpu_to_hc32(token);
+		*tdp = cpu_to_hc32((uint32_t)&qtd[qtd_counter]);
+		tdp = &qtd[qtd_counter++].qt_next;
 	}
 
-	qh_list.qh_link = cpu_to_hc32((uint32_t) qh | QH_LINK_TYPE_QH);
+	qh_list.qh_link = cpu_to_hc32((uint32_t)&qh | QH_LINK_TYPE_QH);
 
 	/* Flush dcache */
 	ehci_flush_dcache(&qh_list);
@@ -462,7 +410,7 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer,
 
 	/* Wait for TDs to be processed. */
 	ts = get_timer(0);
-	vtd = td;
+	vtd = &qtd[qtd_counter - 1];
 	timeout = USB_TIMEOUT_MS(pipe);
 	do {
 		/* Invalidate dcache */
@@ -492,7 +440,7 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer,
 
 	qh_list.qh_link = cpu_to_hc32((uint32_t)&qh_list | QH_LINK_TYPE_QH);
 
-	token = hc32_to_cpu(qh->qh_overlay.qt_token);
+	token = hc32_to_cpu(qh.qh_overlay.qt_token);
 	if (!(token & 0x80)) {
 		debug("TOKEN=%#x\n", token);
 		switch (token & 0xfc) {
@@ -531,13 +479,6 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer,
 	return (dev->status != USB_ST_NOT_PROC) ? 0 : -1;
 
 fail:
-	td = (void *)hc32_to_cpu(qh->qh_overlay.qt_next);
-	while (td != (void *)QT_NEXT_TERMINATE) {
-		qh->qh_overlay.qt_next = td->qt_next;
-		ehci_free(td, sizeof(*td));
-		td = (void *)hc32_to_cpu(qh->qh_overlay.qt_next);
-	}
-	ehci_free(qh, sizeof(*qh));
 	return -1;
 }
 
Signature · 1 line
1.7.9.1
#4 lore raw

PATCH 3/4 USB: Drop cache flush bloat in EHCI-HCD

In reply to Marek Vasut

Discard the creepy cache flushing mechanisms in ehci-hcd.c and replace them with more straightforward flushing. In the new approach, the flushing takes place directly in ehci_submit_async() call instead of going through the QH list and flushing all members and buffers. This discards a lot of weird bit operations on the members of QH and qTD structures.

NOTE: Certainly, this flushes even qTDs which are possibly unused in some transactions, though the overhead of the previous code was much higher than is the overhead of flushing two more cache lines (which most probably aren't even cached).

---
 drivers/usb/host/ehci-hcd.c |  123 ++++++++-----------------------------------
 1 files changed, 23 insertions(+), 100 deletions(-)
drivers/usb/host/ehci-hcd.c +23 -100
index 8ac0b44..55fe067 100644
@@ -108,99 +108,6 @@ static struct descriptor {
 #define ehci_is_TDI()	(0)
 #endif
 
-#if defined(CONFIG_EHCI_DCACHE)
-/*
- * Routines to handle (flush/invalidate) the dcache for the QH and qTD
- * structures and data buffers. This is needed on platforms using this
- * EHCI support with dcache enabled.
- */
-static void flush_invalidate(u32 addr, int size, int flush)
-{
-	if (flush)
-		flush_dcache_range(addr, addr + size);
-	else
-		invalidate_dcache_range(addr, addr + size);
-}
-
-static void cache_qtd(struct qTD *qtd, int flush)
-{
-	u32 *ptr = (u32 *)qtd->qt_buffer[0];
-	int len = (qtd->qt_token & 0x7fff0000) >> 16;
-
-	flush_invalidate((u32)qtd, sizeof(struct qTD), flush);
-	if (ptr && len)
-		flush_invalidate((u32)ptr, len, flush);
-}
-
-
-static inline struct QH *qh_addr(struct QH *qh)
-{
-	return (struct QH *)((u32)qh & 0xffffffe0);
-}
-
-static void cache_qh(struct QH *qh, int flush)
-{
-	struct qTD *qtd;
-	struct qTD *next;
-	static struct qTD *first_qtd;
-
-	/*
-	 * Walk the QH list and flush/invalidate all entries
-	 */
-	while (1) {
-		flush_invalidate((u32)qh_addr(qh), sizeof(struct QH), flush);
-		if ((u32)qh & QH_LINK_TYPE_QH)
-			break;
-		qh = qh_addr(qh);
-		qh = (struct QH *)qh->qh_link;
-	}
-	qh = qh_addr(qh);
-
-	/*
-	 * Save first qTD pointer, needed for invalidating pass on this QH
-	 */
-	if (flush)
-		first_qtd = qtd = (struct qTD *)(*(u32 *)&qh->qh_overlay &
-						 0xffffffe0);
-	else
-		qtd = first_qtd;
-
-	/*
-	 * Walk the qTD list and flush/invalidate all entries
-	 */
-	while (1) {
-		if (qtd == NULL)
-			break;
-		cache_qtd(qtd, flush);
-		next = (struct qTD *)((u32)qtd->qt_next & 0xffffffe0);
-		if (next == qtd)
-			break;
-		qtd = next;
-	}
-}
-
-static inline void ehci_flush_dcache(struct QH *qh)
-{
-	cache_qh(qh, 1);
-}
-
-static inline void ehci_invalidate_dcache(struct QH *qh)
-{
-	cache_qh(qh, 0);
-}
-#else /* CONFIG_EHCI_DCACHE */
-/*
- *
- */
-static inline void ehci_flush_dcache(struct QH *qh)
-{
-}
-
-static inline void ehci_invalidate_dcache(struct QH *qh)
-{
-}
-#endif /* CONFIG_EHCI_DCACHE */
-
 void __ehci_powerup_fixup(uint32_t *status_reg, uint32_t *reg)
 {
 	mdelay(50);
@@ -263,12 +170,20 @@ out:
 
 static int ehci_td_buffer(struct qTD *td, void *buf, size_t sz)
 {
-	uint32_t addr, delta, next;
+	uint32_t delta, next;
+	uint32_t addr = (uint32_t)buf;
+	size_t rsz = roundup(sz, 32);
 	int idx;
 
-	addr = (uint32_t) buf;
+	if (sz != rsz)
+		debug("EHCI-HCD: Misaligned buffer size (%08x)\n", sz);
+
+	if (addr & ~31)
+		debug("EHCI-HCD: Misaligned buffer address (%p)\n", buf);
+
 	idx = 0;
 	while (idx < 5) {
+		flush_dcache_range(addr, addr + rsz);
 		td->qt_buffer[idx] = cpu_to_hc32(addr);
 		td->qt_buffer_hi[idx] = 0;
 		next = (addr + 4096) & ~4095;
@@ -317,6 +232,8 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer,
 	memset(&qh, 0, sizeof(struct QH));
 	memset(qtd, 0, sizeof(qtd));
 
+	toggle = usb_gettoggle(dev, usb_pipeendpoint(pipe), usb_pipeout(pipe));
+
 	qh.qh_link = cpu_to_hc32((uint32_t)&qh_list | QH_LINK_TYPE_QH);
 	c = (usb_pipespeed(pipe) != USB_SPEED_HIGH &&
 	     usb_pipeendpoint(pipe) == 0) ? 1 : 0;
@@ -337,9 +254,6 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer,
 
 	tdp = &qh.qh_overlay.qt_next;
 
-	toggle =
-	    usb_gettoggle(dev, usb_pipeendpoint(pipe), usb_pipeout(pipe));
-
 	if (req != NULL) {
 		qtd[qtd_counter].qt_next = cpu_to_hc32(QT_NEXT_TERMINATE);
 		qtd[qtd_counter].qt_altnext = cpu_to_hc32(QT_NEXT_TERMINATE);
@@ -391,7 +305,10 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer,
 	qh_list.qh_link = cpu_to_hc32((uint32_t)&qh | QH_LINK_TYPE_QH);
 
 	/* Flush dcache */
-	ehci_flush_dcache(&qh_list);
+	flush_dcache_range((uint32_t)&qh_list,
+		(uint32_t)&qh_list + sizeof(struct QH));
+	flush_dcache_range((uint32_t)&qh, (uint32_t)&qh + sizeof(struct QH));
+	flush_dcache_range((uint32_t)qtd, (uint32_t)qtd + sizeof(qtd));
 
 	usbsts = ehci_readl(&hcor->or_usbsts);
 	ehci_writel(&hcor->or_usbsts, (usbsts & 0x3f));
@@ -414,7 +331,13 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer,
 	timeout = USB_TIMEOUT_MS(pipe);
 	do {
 		/* Invalidate dcache */
-		ehci_invalidate_dcache(&qh_list);
+		invalidate_dcache_range((uint32_t)&qh_list,
+			(uint32_t)&qh_list + sizeof(struct QH));
+		invalidate_dcache_range((uint32_t)&qh,
+			(uint32_t)&qh + sizeof(struct QH));
+		invalidate_dcache_range((uint32_t)qtd,
+			(uint32_t)qtd + sizeof(qtd));
+
 		token = hc32_to_cpu(vtd->qt_token);
 		if (!(token & 0x80))
 			break;
Signature · 1 line
1.7.9.1
#5 lore raw

PATCH v2 3/4 USB: Drop cache flush bloat in EHCI-HCD

Discard the creepy cache flushing mechanisms in ehci-hcd.c and replace them with more straightforward flushing. In the new approach, the flushing takes place directly in ehci_submit_async() call instead of going through the QH list and flushing all members and buffers. This discards a lot of weird bit operations on the members of QH and qTD structures.

NOTE: Certainly, this flushes even qTDs which are possibly unused in some transactions, though the overhead of the previous code was much higher than is the overhead of flushing two more cache lines (which most probably aren't even cached).

---
 drivers/usb/host/ehci-hcd.c |  127 +++++++++----------------------------------
 1 files changed, 27 insertions(+), 100 deletions(-)

V2: Also flush the data buffer.
Fix typo in address alignment checking in ehci_td_buffer()

drivers/usb/host/ehci-hcd.c +27 -100
index 8ac0b44..9f0991c 100644
@@ -108,99 +108,6 @@ static struct descriptor {
 #define ehci_is_TDI()	(0)
 #endif
 
-#if defined(CONFIG_EHCI_DCACHE)
-/*
- * Routines to handle (flush/invalidate) the dcache for the QH and qTD
- * structures and data buffers. This is needed on platforms using this
- * EHCI support with dcache enabled.
- */
-static void flush_invalidate(u32 addr, int size, int flush)
-{
-	if (flush)
-		flush_dcache_range(addr, addr + size);
-	else
-		invalidate_dcache_range(addr, addr + size);
-}
-
-static void cache_qtd(struct qTD *qtd, int flush)
-{
-	u32 *ptr = (u32 *)qtd->qt_buffer[0];
-	int len = (qtd->qt_token & 0x7fff0000) >> 16;
-
-	flush_invalidate((u32)qtd, sizeof(struct qTD), flush);
-	if (ptr && len)
-		flush_invalidate((u32)ptr, len, flush);
-}
-
-
-static inline struct QH *qh_addr(struct QH *qh)
-{
-	return (struct QH *)((u32)qh & 0xffffffe0);
-}
-
-static void cache_qh(struct QH *qh, int flush)
-{
-	struct qTD *qtd;
-	struct qTD *next;
-	static struct qTD *first_qtd;
-
-	/*
-	 * Walk the QH list and flush/invalidate all entries
-	 */
-	while (1) {
-		flush_invalidate((u32)qh_addr(qh), sizeof(struct QH), flush);
-		if ((u32)qh & QH_LINK_TYPE_QH)
-			break;
-		qh = qh_addr(qh);
-		qh = (struct QH *)qh->qh_link;
-	}
-	qh = qh_addr(qh);
-
-	/*
-	 * Save first qTD pointer, needed for invalidating pass on this QH
-	 */
-	if (flush)
-		first_qtd = qtd = (struct qTD *)(*(u32 *)&qh->qh_overlay &
-						 0xffffffe0);
-	else
-		qtd = first_qtd;
-
-	/*
-	 * Walk the qTD list and flush/invalidate all entries
-	 */
-	while (1) {
-		if (qtd == NULL)
-			break;
-		cache_qtd(qtd, flush);
-		next = (struct qTD *)((u32)qtd->qt_next & 0xffffffe0);
-		if (next == qtd)
-			break;
-		qtd = next;
-	}
-}
-
-static inline void ehci_flush_dcache(struct QH *qh)
-{
-	cache_qh(qh, 1);
-}
-
-static inline void ehci_invalidate_dcache(struct QH *qh)
-{
-	cache_qh(qh, 0);
-}
-#else /* CONFIG_EHCI_DCACHE */
-/*
- *
- */
-static inline void ehci_flush_dcache(struct QH *qh)
-{
-}
-
-static inline void ehci_invalidate_dcache(struct QH *qh)
-{
-}
-#endif /* CONFIG_EHCI_DCACHE */
-
 void __ehci_powerup_fixup(uint32_t *status_reg, uint32_t *reg)
 {
 	mdelay(50);
@@ -263,12 +170,20 @@ out:
 
 static int ehci_td_buffer(struct qTD *td, void *buf, size_t sz)
 {
-	uint32_t addr, delta, next;
+	uint32_t delta, next;
+	uint32_t addr = (uint32_t)buf;
+	size_t rsz = roundup(sz, 32);
 	int idx;
 
-	addr = (uint32_t) buf;
+	if (sz != rsz)
+		debug("EHCI-HCD: Misaligned buffer size (%08x)\n", sz);
+
+	if (addr & 31)
+		debug("EHCI-HCD: Misaligned buffer address (%p)\n", buf);
+
 	idx = 0;
 	while (idx < 5) {
+		flush_dcache_range(addr, addr + rsz);
 		td->qt_buffer[idx] = cpu_to_hc32(addr);
 		td->qt_buffer_hi[idx] = 0;
 		next = (addr + 4096) & ~4095;
@@ -317,6 +232,8 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer,
 	memset(&qh, 0, sizeof(struct QH));
 	memset(qtd, 0, sizeof(qtd));
 
+	toggle = usb_gettoggle(dev, usb_pipeendpoint(pipe), usb_pipeout(pipe));
+
 	qh.qh_link = cpu_to_hc32((uint32_t)&qh_list | QH_LINK_TYPE_QH);
 	c = (usb_pipespeed(pipe) != USB_SPEED_HIGH &&
 	     usb_pipeendpoint(pipe) == 0) ? 1 : 0;
@@ -337,9 +254,6 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer,
 
 	tdp = &qh.qh_overlay.qt_next;
 
-	toggle =
-	    usb_gettoggle(dev, usb_pipeendpoint(pipe), usb_pipeout(pipe));
-
 	if (req != NULL) {
 		qtd[qtd_counter].qt_next = cpu_to_hc32(QT_NEXT_TERMINATE);
 		qtd[qtd_counter].qt_altnext = cpu_to_hc32(QT_NEXT_TERMINATE);
@@ -391,7 +305,10 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer,
 	qh_list.qh_link = cpu_to_hc32((uint32_t)&qh | QH_LINK_TYPE_QH);
 
 	/* Flush dcache */
-	ehci_flush_dcache(&qh_list);
+	flush_dcache_range((uint32_t)&qh_list,
+		(uint32_t)&qh_list + sizeof(struct QH));
+	flush_dcache_range((uint32_t)&qh, (uint32_t)&qh + sizeof(struct QH));
+	flush_dcache_range((uint32_t)qtd, (uint32_t)qtd + sizeof(qtd));
 
 	usbsts = ehci_readl(&hcor->or_usbsts);
 	ehci_writel(&hcor->or_usbsts, (usbsts & 0x3f));
@@ -414,13 +331,23 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer,
 	timeout = USB_TIMEOUT_MS(pipe);
 	do {
 		/* Invalidate dcache */
-		ehci_invalidate_dcache(&qh_list);
+		invalidate_dcache_range((uint32_t)&qh_list,
+			(uint32_t)&qh_list + sizeof(struct QH));
+		invalidate_dcache_range((uint32_t)&qh,
+			(uint32_t)&qh + sizeof(struct QH));
+		invalidate_dcache_range((uint32_t)qtd,
+			(uint32_t)qtd + sizeof(qtd));
+
 		token = hc32_to_cpu(vtd->qt_token);
 		if (!(token & 0x80))
 			break;
 		WATCHDOG_RESET();
 	} while (get_timer(ts) < timeout);
 
+	/* Invalidate the memory area occupied by buffer */
+	invalidate_dcache_range(((uint32_t)buffer & ~31),
+		((uint32_t)buffer & ~31) + roundup(length, 32));
+
 	/* Check that the TD processing happened */
 	if (token & 0x80) {
 		printf("EHCI timed out on TD - token=%#x\n", token);
Signature · 1 line
1.7.9.1
#6 lore raw

PATCH v2 3/4 USB: Drop cache flush bloat in EHCI-HCD

Hi Marek,

Quoted 11 lines · On Mon, 9 Apr 2012 06:47:31 +0200 Marek Vasut <[email protected]> wrote:

Discard the creepy cache flushing mechanisms in ehci-hcd.c and replace them with more straightforward flushing. In the new approach, the flushing takes place directly in ehci_submit_async() call instead of going through the QH list and flushing all members and buffers. This discards a lot of weird bit operations on the members of QH and qTD structures.

NOTE: Certainly, this flushes even qTDs which are possibly unused in some transactions, though the overhead of the previous code was much higher than is the overhead of flushing two more cache lines (which most probably aren't even cached).

Quoted 5 lines
---
 drivers/usb/host/ehci-hcd.c |  127 +++++++++----------------------------------
 1 files changed, 27 insertions(+), 100 deletions(-)

Unfortunately this patch breaks compiling for many powerpc boards, mpc512x, mpc83xx, mpc85xx and QorIQ Px based with USB support enabled.

Thanks,
Anatolij

#7 lore raw

PATCH v2 3/4 USB: Drop cache flush bloat in EHCI-HCD

Dear Anatolij Gustschin,

Quoted 21 lines

Hi Marek,

On Mon, 9 Apr 2012 06:47:31 +0200

Quoted 16 lines · Marek Vasut

Discard the creepy cache flushing mechanisms in ehci-hcd.c and replace them with more straightforward flushing. In the new approach, the flushing takes place directly in ehci_submit_async() call instead of going through the QH list and flushing all members and buffers. This discards a lot of weird bit operations on the members of QH and qTD structures.

NOTE: Certainly, this flushes even qTDs which are possibly unused in some transactions, though the overhead of the previous code was much higher than is the overhead of flushing two more cache lines (which most probably aren't even cached).

---

Quoted 6 lines
Quoted 3 lines
 drivers/usb/host/ehci-hcd.c |  127

+++++++++---------------------------------- 1 files changed, 27 insertions(+), 100 deletions(-)

Unfortunately this patch breaks compiling for many powerpc boards, mpc512x, mpc83xx, mpc85xx and QorIQ Px based with USB support enabled.

Because they have broken cache implementation, right? I mean, they have their own snooping methods, so they don't need the cache flushing at all, but then, if they don't, these methods (dcache_flush() etc) should be optimized to empty functions. Maybe we should implement them for these CPUs then? btw. I thought these compiled before, hm...

Quoted 2 lines

Thanks,
Anatolij

Best regards,
Marek Vasut

#8 lore raw

PATCH v2 3/4 USB: Drop cache flush bloat in EHCI-HCD

Hi Marek,

Quoted 4 lines · Marek Vasut
Quoted 4 lines
Quoted 3 lines
 drivers/usb/host/ehci-hcd.c |  127

+++++++++---------------------------------- 1 files changed, 27 insertions(+), 100 deletions(-)

Quoted 8 lines
Quoted 2 lines

Unfortunately this patch breaks compiling for many powerpc boards, mpc512x, mpc83xx, mpc85xx and QorIQ Px based with USB support enabled.

Because they have broken cache implementation, right? I mean, they have their own snooping methods, so they don't need the cache flushing at all, but then, if they don't, these methods (dcache_flush() etc) should be optimized to empty functions. Maybe we should implement them for these CPUs then? btw. I thought these compiled before, hm...

Now this patch has been applied at "master" branch, but the building for powerpc boards as Anatolij mentioned will be failed.
So Which platforms this patch applies to? If they all can not flush cache like the powerpc snooping methods?

Thanks!

Best Regards!

Liu Gang

#9 lore raw

PATCH v2 3/4 USB: Drop cache flush bloat in EHCI-HCD

Dear Liu Gang,

Quoted 16 lines

Hi Marek,

Quoted 4 lines · Marek Vasut
Quoted 4 lines
Quoted 3 lines
 drivers/usb/host/ehci-hcd.c |  127

+++++++++---------------------------------- 1 files changed, 27 insertions(+), 100 deletions(-)

Quoted 8 lines
Quoted 2 lines

Unfortunately this patch breaks compiling for many powerpc boards, mpc512x, mpc83xx, mpc85xx and QorIQ Px based with USB support enabled.

Because they have broken cache implementation, right? I mean, they have their own snooping methods, so they don't need the cache flushing at all, but then, if they don't, these methods (dcache_flush() etc) should be optimized to empty functions. Maybe we should implement them for these CPUs then? btw. I thought these compiled before, hm...

Quoted 4 lines

Now this patch has been applied at "master" branch, but the building for powerpc boards as Anatolij mentioned will be failed.
So Which platforms this patch applies to? If they all can not flush cache like the powerpc snooping methods?

I already submitted a patch for this issue

http://www.mail-archive.com/u-boot at lists.denx.de/msg84629.html

Quoted 5 lines

Thanks!

Best Regards!

Liu Gang

Best regards,
Marek Vasut

#10 lore raw

PATCH 4/4 USB: Document the QH and qTD antics in EHCI-HCD

In reply to Marek Vasut

The construction of QH and qTD lists in ehci_submit_async() call is cryptic business, add at least a bit of comments so if someone is reading it, he can at least reference the intel ehci manual (ehci-r10.pdf).

---
 drivers/usb/host/ehci-hcd.c |   38 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 38 insertions(+), 0 deletions(-)
drivers/usb/host/ehci-hcd.c +38 -0
index 55fe067..524bc41 100644
@@ -234,6 +234,16 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer,
 
 	toggle = usb_gettoggle(dev, usb_pipeendpoint(pipe), usb_pipeout(pipe));
 
+	/*
+	 * Setup QH (3.6 in ehci-r10.pdf)
+	 *
+	 *   qh_link ................. 03-00 H
+	 *   qh_endpt1 ............... 07-04 H
+	 *   qh_endpt2 ............... 0B-08 H
+	 * - qh_curtd
+	 *   qh_overlay.qt_next ...... 13-10 H
+	 * - qh_overlay.qt_altnext
+	 */
 	qh.qh_link = cpu_to_hc32((uint32_t)&qh_list | QH_LINK_TYPE_QH);
 	c = (usb_pipespeed(pipe) != USB_SPEED_HIGH &&
 	     usb_pipeendpoint(pipe) == 0) ? 1 : 0;
@@ -255,6 +265,15 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer,
 	tdp = &qh.qh_overlay.qt_next;
 
 	if (req != NULL) {
+		/*
+		 * Setup request qTD (3.5 in ehci-r10.pdf)
+		 *
+		 *   qt_next ................ 03-00 H
+		 *   qt_altnext ............. 07-04 H
+		 *   qt_token ............... 0B-08 H
+		 *
+		 *   [ buffer, buffer_hi ] loaded with "req".
+		 */
 		qtd[qtd_counter].qt_next = cpu_to_hc32(QT_NEXT_TERMINATE);
 		qtd[qtd_counter].qt_altnext = cpu_to_hc32(QT_NEXT_TERMINATE);
 		token = (0 << 31) |
@@ -265,12 +284,22 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer,
 			debug("unable construct SETUP td\n");
 			goto fail;
 		}
+		/* Update previous qTD! */
 		*tdp = cpu_to_hc32((uint32_t)&qtd[qtd_counter]);
 		tdp = &qtd[qtd_counter++].qt_next;
 		toggle = 1;
 	}
 
 	if (length > 0 || req == NULL) {
+		/*
+		 * Setup request qTD (3.5 in ehci-r10.pdf)
+		 *
+		 *   qt_next ................ 03-00 H
+		 *   qt_altnext ............. 07-04 H
+		 *   qt_token ............... 0B-08 H
+		 *
+		 *   [ buffer, buffer_hi ] loaded with "buffer".
+		 */
 		qtd[qtd_counter].qt_next = cpu_to_hc32(QT_NEXT_TERMINATE);
 		qtd[qtd_counter].qt_altnext = cpu_to_hc32(QT_NEXT_TERMINATE);
 		token = (toggle << 31) |
@@ -284,11 +313,19 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer,
 			debug("unable construct DATA td\n");
 			goto fail;
 		}
+		/* Update previous qTD! */
 		*tdp = cpu_to_hc32((uint32_t)&qtd[qtd_counter]);
 		tdp = &qtd[qtd_counter++].qt_next;
 	}
 
 	if (req != NULL) {
+		/*
+		 * Setup request qTD (3.5 in ehci-r10.pdf)
+		 *
+		 *   qt_next ................ 03-00 H
+		 *   qt_altnext ............. 07-04 H
+		 *   qt_token ............... 0B-08 H
+		 */
 		qtd[qtd_counter].qt_next = cpu_to_hc32(QT_NEXT_TERMINATE);
 		qtd[qtd_counter].qt_altnext = cpu_to_hc32(QT_NEXT_TERMINATE);
 		token = (toggle << 31) |
@@ -298,6 +335,7 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer,
 		    (3 << 10) |
 		    ((usb_pipein(pipe) ? 0 : 1) << 8) | (0x80 << 0);
 		qtd[qtd_counter].qt_token = cpu_to_hc32(token);
+		/* Update previous qTD! */
 		*tdp = cpu_to_hc32((uint32_t)&qtd[qtd_counter]);
 		tdp = &qtd[qtd_counter++].qt_next;
 	}
Signature · 1 line
1.7.9.1
#11 lore raw

PATCH 4/4 USB: Document the QH and qTD antics in EHCI-HCD

Quoted 3 lines · On Sunday 08 April 2012 23:09:52 Marek Vasut wrote:

The construction of QH and qTD lists in ehci_submit_async() call is cryptic business, add at least a bit of comments so if someone is reading it, he can at least reference the intel ehci manual (ehci-r10.pdf).

nice
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120427/8a5711a1/attachment.pgp>

#12 lore raw

cover USB: Fix EHCI to work with data cache enabled

In reply to Marek Vasut

Dear Marek Vasut,

Quoted 21 lines

This patchset reworks the USB core and EHCI-HCD to work with data caches.

Marek Vasut (3):
USB: Drop ehci_alloc/ehci_free in ehci-hcd
USB: Drop cache flush bloat in EHCI-HCD
USB: Document the QH and qTD antics in EHCI-HCD

Puneet Saxena (1):
USB: Align buffers at cacheline

 common/cmd_usb.c            |    3 +-
 common/usb.c                |   22 ++--
 common/usb_hub.c            |   27 ++--
 common/usb_storage.c        |   59 +++++-----
 disk/part_dos.c             |    2 +-
 drivers/usb/host/ehci-hcd.c |  284
++++++++++++++----------------------------- include/scsi.h              | 
  4 +-
 include/usb.h               |    4 +-
 8 files changed, 155 insertions(+), 250 deletions(-)
Quoted 1 line
Cc Puneet Saxena <[email protected]>

Puneet, can you please test this stuff? It should fix your cache misalignment issues. Mine are now gone and USB works with caches on too.

I'd like to queue this for -next.

Best regards,
Marek Vasut

#13 lore raw

cover USB: Fix EHCI to work with data cache enabled

Dear Marek,
After applying these 4 patches I don't see any alignment spew and it detects the
mass storage device perfectly though I see a warning "EHCI timed out on TD - token=0x80008c80"
consistently in each "usb reset".

Thanx & Regards,
Puneet

Quoted 23 lines · Marek Vasut

Dear Marek Vasut,

Quoted 21 lines

This patchset reworks the USB core and EHCI-HCD to work with data caches.

Marek Vasut (3):
USB: Drop ehci_alloc/ehci_free in ehci-hcd
USB: Drop cache flush bloat in EHCI-HCD
USB: Document the QH and qTD antics in EHCI-HCD

Puneet Saxena (1):
USB: Align buffers at cacheline

  common/cmd_usb.c            |    3 +-
  common/usb.c                |   22 ++--
  common/usb_hub.c            |   27 ++--
  common/usb_storage.c        |   59 +++++-----
  disk/part_dos.c             |    2 +-
  drivers/usb/host/ehci-hcd.c |  284
++++++++++++++----------------------------- include/scsi.h              |
   4 +-
  include/usb.h               |    4 +-
  8 files changed, 155 insertions(+), 250 deletions(-)
Quoted 8 lines
Quoted 1 line
Cc Puneet Saxena<[email protected]>

Puneet, can you please test this stuff? It should fix your cache misalignment issues. Mine are now gone and USB works with caches on too.

I'd like to queue this for -next.

Best regards,
Marek Vasut

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

#14 lore raw

cover USB: Fix EHCI to work with data cache enabled

Dear puneets,

Quoted 6 lines

Dear Marek,
After applying these 4 patches I don't see any alignment spew and it detects the
mass storage device perfectly though I see a warning "EHCI timed out on TD - token=0x80008c80"
consistently in each "usb reset".

That's something else (unrelated to this patchset), I saw it in your backlog too. Can you try debuging it on top of these patches? Because these patches fix a lot of other issues, so let's take them as a base for now. Also, did you really test dos partition table and vfat? I had to fix them to work with patches, you change to part_dos.c wasn't enough.

Quoted 43 lines

Thanx & Regards,
Puneet

Quoted 30 lines · Marek Vasut

Dear Marek Vasut,

Quoted 21 lines

This patchset reworks the USB core and EHCI-HCD to work with data caches.

Marek Vasut (3):
USB: Drop ehci_alloc/ehci_free in ehci-hcd
USB: Drop cache flush bloat in EHCI-HCD
USB: Document the QH and qTD antics in EHCI-HCD

Puneet Saxena (1):
USB: Align buffers at cacheline

  common/cmd_usb.c            |    3 +-
  common/usb.c                |   22 ++--
  common/usb_hub.c            |   27 ++--
  common/usb_storage.c        |   59 +++++-----
  disk/part_dos.c             |    2 +-
  drivers/usb/host/ehci-hcd.c |  284

++++++++++++++----------------------------- include/scsi.h |

Quoted 6 lines
   4 +-
  include/usb.h               |    4 +-
  8 files changed, 155 insertions(+), 250 deletions(-)
Cc Puneet Saxena<[email protected]>
Quoted 7 lines

Puneet, can you please test this stuff? It should fix your cache misalignment issues. Mine are now gone and USB works with caches on too.

I'd like to queue this for -next.

Best regards,
Marek Vasut

Quoted 8 lines

---------------------------------------------------------------------------
-------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message.
---------------------------------------------------------------------------
--------

Best regards,
Marek Vasut