[U-Boot-Users] Setting processor endianess for USB modules

[U-Boot-Users] Setting processor endianess for USB modules

[Christian Eggers]

Hello,

I've recognized that a lot of USB code in U-Boot uses the macros
swap_16() and swap_32() which are defined in usb.h. The behaviour
of the macros is controlled by the define LITTLEENDIAN.

Is there a good reason NOT to use the macros provided in
asm/byteorder.h (as in the appropriate code in Linux-2.4 )? 

I think that switching to these functions might be useful in order
to eliminate the need to use the LITTLEENDIAN define for
specifying the byteorder. It seems that LITTLEENDIAN is not
used outside the USB code.

regards

Christian Eggers

[U-Boot-Users] Setting processor endianess for USB modules

[Markus Klotzbücher]

Dear Christian,

"Christian Eggers" <ceggers@gmx.de> writes:

> I've recognized that a lot of USB code in U-Boot uses the macros
> swap_16() and swap_32() which are defined in usb.h. The behaviour
> of the macros is controlled by the define LITTLEENDIAN.
>
> Is there a good reason NOT to use the macros provided in
> asm/byteorder.h (as in the appropriate code in Linux-2.4 )? 

Largely not. But be carefull. Besides big and little endian CPUs we also
have controllers that operate in big or little endian (see
CFG_OHCI_BE_CONTROLLER), and then there are PCI controllers whose
registers need to be accessed as little endian
(CFG_OHCI_SWAP_REG_ACCESS).

But your right, there is no real for LITTLEENDIAN.

> I think that switching to these functions might be useful in order
> to eliminate the need to use the LITTLEENDIAN define for
> specifying the byteorder. It seems that LITTLEENDIAN is not
> used outside the USB code.

Right. Patches cleaning this up are more than welcome! Please note that
cleaning up USB drivers under the cpu/ directory is a waste of time
though. Boards using these drivers should be converted to use the
generic infrastructure in drivers/usb/ instead.

Best regards

Markus Klotzbuecher

--
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

[U-Boot-Users] Setting processor endianess for USB modules

[Christian Eggers]

Dear Markus,

I've made I new patch with git as requested.

regards
Christian


> Dear Christian,
> 
> "Christian Eggers" <ceggers@gmx.de> writes:
> 
> > I've recognized that a lot of USB code in U-Boot uses the macros
> > swap_16() and swap_32() which are defined in usb.h. The behaviour
> > of the macros is controlled by the define LITTLEENDIAN.
> >
> > Is there a good reason NOT to use the macros provided in
> > asm/byteorder.h (as in the appropriate code in Linux-2.4 )? 
> 
> Largely not. But be carefull. Besides big and little endian CPUs we also
> have controllers that operate in big or little endian (see
> CFG_OHCI_BE_CONTROLLER), and then there are PCI controllers whose
> registers need to be accessed as little endian
> (CFG_OHCI_SWAP_REG_ACCESS).
> 
> But your right, there is no real for LITTLEENDIAN.
> 
> > I think that switching to these functions might be useful in order
> > to eliminate the need to use the LITTLEENDIAN define for
> > specifying the byteorder. It seems that LITTLEENDIAN is not
> > used outside the USB code.
> 
> Right. Patches cleaning this up are more than welcome! Please note that
> cleaning up USB drivers under the cpu/ directory is a waste of time
> though. Boards using these drivers should be converted to use the
> generic infrastructure in drivers/usb/ instead.
> 
> Best regards
> 
> Markus Klotzbuecher


---
 common/usb.c         |   44 ++++++++++++++++++++++----------------------
 common/usb_kbd.c     |   11 ++++++-----
 common/usb_storage.c |   28 ++++++++++------------------
 3 files changed, 38 insertions(+), 45 deletions(-)

diff --git a/common/usb.c b/common/usb.c
index 2fa5254..7033c2a 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -48,6 +48,7 @@
 #include <command.h>
 #include <asm/processor.h>
 #include <linux/ctype.h>
+#include <asm/byteorder.h>
 
 #if defined(CONFIG_CMD_USB)
 
@@ -177,10 +178,10 @@ int usb_control_msg(struct usb_device *d
 	/* set setup command */
 	setup_packet.requesttype = requesttype;
 	setup_packet.request = request;
-	setup_packet.value = swap_16(value);
-	setup_packet.index = swap_16(index);
-	setup_packet.length = swap_16(size);
- 	USB_PRINTF("usb_control_msg: request: 0x%X, requesttype: 0x%X\nvalue 
0x%X index 0x%X length 0x%X\n",
+	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 */
 
@@ -300,7 +301,7 @@ int usb_parse_config(struct usb_device *
 		return -1;
 	}
 	memcpy(&dev->config, buffer, buffer[0]);
-	dev->config.wTotalLength = swap_16(dev->config.wTotalLength);
+	le16_to_cpus(&(dev->config.wTotalLength));
 	dev->config.no_of_if = 0;
 
 	index = dev->config.bLength;
@@ -329,8 +330,7 @@ int usb_parse_config(struct usb_device *
 				dev->config.if_desc[ifno].no_of_ep++; /* found an 
endpoint */
 				memcpy(&dev->config.if_desc[ifno].ep_desc[epno],
 					&buffer[index], buffer[index]);
-				dev-
>config.if_desc[ifno].ep_desc[epno].wMaxPacketSize =
-					swap_16(dev-
>config.if_desc[ifno].ep_desc[epno].wMaxPacketSize);
+				le16_to_cpus(&(dev-
>config.if_desc[ifno].ep_desc[epno].wMaxPacketSize));
 				USB_PRINTF("if %d, ep %d\n", ifno, epno);
 				break;
 			default:
@@ -413,7 +413,7 @@ int usb_get_configuration_no(struct usb_
 			printf("config descriptor too short (expected %i, got 
%i)\n",8,result);
 		return -1;
 	}
-	tmp=swap_16(config->wTotalLength);
+	tmp = le16_to_cpu(config->wTotalLength);
 
 	if (tmp > USB_BUFSIZ) {
 		USB_PRINTF("usb_get_configuration_no: failed to get descriptor - too 
long: %d\n",
@@ -816,10 +816,10 @@ int usb_new_device(struct usb_device *de
 		return 1;
 	}
 	/* correct le values */
-	dev->descriptor.bcdUSB=swap_16(dev->descriptor.bcdUSB);
-	dev->descriptor.idVendor=swap_16(dev->descriptor.idVendor);
-	dev->descriptor.idProduct=swap_16(dev->descriptor.idProduct);
-	dev->descriptor.bcdDevice=swap_16(dev->descriptor.bcdDevice);
+	le16_to_cpus(&dev->descriptor.bcdUSB);
+	le16_to_cpus(&dev->descriptor.idVendor);
+	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);
@@ -979,8 +979,8 @@ static int hub_port_reset(struct usb_dev
 			USB_HUB_PRINTF("get_port_status failed status %lX\n",dev-
>status);
 			return -1;
 		}
-		portstatus = swap_16(portsts.wPortStatus);
-		portchange = swap_16(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,
 			portstatus&(1<<USB_PORT_FEAT_LOWSPEED) ? "Low 
Speed" : "High Speed");
 		USB_HUB_PRINTF("STAT_C_CONNECTION = %d 
STAT_CONNECTION = %d  USB_PORT_STAT_ENABLE %d\n",
@@ -1024,8 +1024,8 @@ void usb_hub_port_connect_change(struct
 		return;
 	}
 
-	portstatus = swap_16(portsts.wPortStatus);
-	portchange = swap_16(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,
 		portstatus&(1<<USB_PORT_FEAT_LOWSPEED) ? "Low Speed" : "High 
Speed");
 
@@ -1099,7 +1099,7 @@ int usb_hub_configure(struct usb_device
 	}
 	memcpy((unsigned char *)&hub->desc,buffer,descriptor->bLength);
 	/* adjust 16bit values */
-	hub->desc.wHubCharacteristics=swap_16(descriptor->wHubCharacteristics);
+	hub->desc.wHubCharacteristics = le16_to_cpu(descriptor->wHubCharacteristics);
 	/* set the bitmap */
 	bitmap=(unsigned char *)&hub->desc.DeviceRemovable[0];
 	memset(bitmap,0xff,(USB_MAXCHILDREN+1+7)/8); /* devices not removable by 
default */
@@ -1161,11 +1161,11 @@ int usb_hub_configure(struct usb_device
 	}
 	hubsts = (struct usb_hub_status *)buffer;
 	USB_HUB_PRINTF("get_hub_status returned status %X, change %X\n",
-		swap_16(hubsts->wHubStatus),swap_16(hubsts->wHubChange));
+		le16_to_cpu(hubsts->wHubStatus),le16_to_cpu(hubsts->wHubChange));
 	USB_HUB_PRINTF("local power source is %s\n",
-		(swap_16(hubsts->wHubStatus) & HUB_STATUS_LOCAL_POWER) ? 
"lost (inactive)" : "good");
+		(le16_to_cpu(hubsts->wHubStatus) & HUB_STATUS_LOCAL_POWER) 
? "lost (inactive)" : "good");
 	USB_HUB_PRINTF("%sover-current condition exists\n",
-		(swap_16(hubsts->wHubStatus) & HUB_STATUS_OVERCURRENT) ? "" 
: "no ");
+		(le16_to_cpu(hubsts->wHubStatus) & HUB_STATUS_OVERCURRENT) 
? "" : "no ");
 	usb_hub_power_on(hub);
 	for (i = 0; i < dev->maxchild; i++) {
 		struct usb_port_status portsts;
@@ -1175,8 +1175,8 @@ int usb_hub_configure(struct usb_device
 			USB_HUB_PRINTF("get_port_status failed\n");
 			continue;
 		}
-		portstatus = swap_16(portsts.wPortStatus);
-		portchange = swap_16(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);
 		if (portchange & USB_PORT_STAT_C_CONNECTION) {
 			USB_HUB_PRINTF("port %d connection change\n", i + 1);
diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index 1703b23..f54c527 100644
--- a/common/usb_kbd.c
+++ b/common/usb_kbd.c
@@ -26,6 +26,7 @@
  */
 #include <common.h>
 #include <devices.h>
+#include <asm/byteorder.h>
 
 #ifdef CONFIG_USB_KEYBOARD
 
@@ -475,14 +476,14 @@ static int fetch_item(unsigned char *sta
 					break;
 				case 2:
 					if ((end - start) >= 2) {
-						item->data.u16 = swap_16((unsigned 
short *)start);
+						item->data.u16 = le16_to_cpu((unsigned 
short *)start);
 						start+=2;
 						return item->size;
 					}
 				case 3:
 					item->size++;
 					if ((end - start) >= 4) {
-						item->data.u32 = swap_32((unsigned 
long *)start);
+						item->data.u32 = le32_to_cpu((unsigned 
long *)start);
 						start+=4;
 						return item->size;
 					}
@@ -705,15 +706,15 @@ static int usb_kbd_get_hid_desc(struct u
 	}
 	index=head->bLength;
 	config=(struct usb_config_descriptor *)&buffer[0];
-	len=swap_16(config->wTotalLength);
+	len=le16_to_cpu(config->wTotalLength);
 	/* Ok the first entry must be a configuration entry, now process the others */
 	head=(struct usb_descriptor_header *)&buffer[index];
 	while(index+1 < len) {
 		if(head->bDescriptorType==USB_DT_HID) {
 			printf("HID desc found\n");
 			memcpy(&usb_kbd_hid_desc,&buffer[index],buffer[index]);
-			
usb_kbd_hid_desc.bcdHID=swap_16(usb_kbd_hid_desc.bcdHID);
-			
usb_kbd_hid_desc.wDescriptorLength=swap_16(usb_kbd_hid_desc.wDescriptorLength);
+			le16_to_cpus(&usb_kbd_hid_desc.bcdHID);
+			le16_to_cpus(&usb_kbd_hid_desc.wDescriptorLength);
 			usb_kbd_display_hid(&usb_kbd_hid_desc);
 			len=0;
 			break;
diff --git a/common/usb_storage.c b/common/usb_storage.c
index 7c08f95..2ca4721 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -52,6 +52,7 @@
 
 #include <common.h>
 #include <command.h>
+#include <asm/byteorder.h>
 #include <asm/processor.h>
 
 
@@ -474,9 +475,9 @@ int usb_stor_BBB_comdat(ccb *srb, struct
 	/* always OUT to the ep */
 	pipe = usb_sndbulkpipe(us->pusb_dev, us->ep_out);
 
-	cbw.dCBWSignature = swap_32(CBWSIGNATURE);
-	cbw.dCBWTag = swap_32(CBWTag++);
-	cbw.dCBWDataTransferLength = swap_32(srb->datalen);
+	cbw.dCBWSignature = cpu_to_le32(CBWSIGNATURE);
+	cbw.dCBWTag = cpu_to_le32(CBWTag++);
+	cbw.dCBWDataTransferLength = cpu_to_le32(srb->datalen);
 	cbw.bCBWFlags = (dir_in? CBWFLAGS_IN : CBWFLAGS_OUT);
 	cbw.bCBWLUN = srb->lun;
 	cbw.bCDBLength = srb->cmdlen;
@@ -692,14 +693,14 @@ int usb_stor_BBB_transport(ccb *srb, str
 	printf("\n");
 #endif
 	/* misuse pipe to get the residue */
-	pipe = swap_32(csw.dCSWDataResidue);
+	pipe = le32_to_cpu(csw.dCSWDataResidue);
 	if (pipe == 0 && srb->datalen != 0 && srb->datalen - data_actlen != 0)
 		pipe = srb->datalen - data_actlen;
-	if (CSWSIGNATURE != swap_32(csw.dCSWSignature)) {
+	if (CSWSIGNATURE != le32_to_cpu(csw.dCSWSignature)) {
 		USB_STOR_PRINTF("!CSWSIGNATURE\n");
 		usb_stor_BBB_reset(us);
 		return USB_STOR_TRANSPORT_FAILED;
-	} else if ((CBWTag - 1) != swap_32(csw.dCSWTag)) {
+	} else if ((CBWTag - 1) != le32_to_cpu(csw.dCSWTag)) {
 		USB_STOR_PRINTF("!Tag\n");
 		usb_stor_BBB_reset(us);
 		return USB_STOR_TRANSPORT_FAILED;
@@ -1222,18 +1223,9 @@ int usb_stor_get_info(struct usb_device
 	if(cap[0]>(0x200000 * 10)) /* greater than 10 GByte */
 		cap[0]>>=16;
 #endif
-#ifdef LITTLEENDIAN
-	cap[0] = ((unsigned long)(
-		(((unsigned long)(cap[0]) & (unsigned long)0x000000ffUL) << 24) |
-		(((unsigned long)(cap[0]) & (unsigned long)0x0000ff00UL) <<  8) |
-		(((unsigned long)(cap[0]) & (unsigned long)0x00ff0000UL) >>  8) |
-		(((unsigned long)(cap[0]) & (unsigned long)0xff000000UL) >> 24) ));
-	cap[1] = ((unsigned long)(
-		(((unsigned long)(cap[1]) & (unsigned long)0x000000ffUL) << 24) |
-		(((unsigned long)(cap[1]) & (unsigned long)0x0000ff00UL) <<  8) |
-		(((unsigned long)(cap[1]) & (unsigned long)0x00ff0000UL) >>  8) |
-		(((unsigned long)(cap[1]) & (unsigned long)0xff000000UL) >> 24) ));
-#endif
+	cap[0] = cpu_to_be32(cap[0]);
+	cap[1] = cpu_to_be32(cap[1]);
+	
 	/* this assumes bigendian! */
 	cap[0] += 1;
 	capacity = &cap[0];
-- 
1.4.3.4

[U-Boot-Users] Setting processor endianess for USB modules

[Wolfgang Denk]

In message <48348CDD.23520.310C8@ceggers.gmx.de> you wrote:
> 
> I've made I new patch with git as requested.

Unfortunately it's unusable as your mailer wrapped long lines:

> @@ -177,10 +178,10 @@ int usb_control_msg(struct usb_device *d
>  	/* set setup command */
>  	setup_packet.requesttype = requesttype;
>  	setup_packet.request = request;
> -	setup_packet.value = swap_16(value);
> -	setup_packet.index = swap_16(index);
> -	setup_packet.length = swap_16(size);
> - 	USB_PRINTF("usb_control_msg: request: 0x%X, requesttype: 0x%X\nvalue 
> 0x%X index 0x%X length 0x%X\n",
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^   Here.

> +	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",
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^   Here.

...
> @@ -329,8 +330,7 @@ int usb_parse_config(struct usb_device *
>  				dev->config.if_desc[ifno].no_of_ep++; /* found an 
> endpoint */
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^   Here.

And so on. Please fix your mailer (or even better use git-send-email)


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Q: How do you spell "onomatopoeia"?
A: The way it sounds.