Skip to contentSkip to conversation

USB cleanup

PATCH New Marek Vasut 2 participants 10 messages Feb 14, 2012

lore
Share & tools
lore

Marek Vasut

author
#0 lore raw

cover USB cleanup

This series introduces various cleanups in order to better split the USB subsystem components.

Marek Vasut (4):
USB: Staticize internal functions
USB: Separate out USB hub driver
USB: Make struct devrequest setup_packet local
USB: Staticize usb_storage.c

 common/Makefile      |    2 +-
 common/usb.c         |  448 ++---------------------------------------------
 common/usb_hub.c     |  483 ++++++++++++++++++++++++++++++++++++++++++++++++++
 common/usb_storage.c |   39 ++--
 include/usb.h        |    8 +
 5 files changed, 522 insertions(+), 458 deletions(-)
 create mode 100644 common/usb_hub.c
Signature · 1 line
1.7.8.3
author
#1 lore raw

PATCH 1/4 USB: Staticize internal functions

Signed-off-by Marek Vasut <[email protected]>
Cc Remy Bohmer <[email protected]>
---
 common/usb.c |   45 +++++++++++++++++++++++----------------------
 1 files changed, 23 insertions(+), 22 deletions(-)
common/usb.c +23 -22
index 63a11c8..de31212 100644
@@ -80,10 +80,10 @@ char usb_started; /* flag for the started/stopped USB status */
 /**********************************************************************
  * some forward declerations...
  */
-void usb_scan_devices(void);
+static void usb_scan_devices(void);
 
-int usb_hub_probe(struct usb_device *dev, int ifnum);
-void usb_hub_reset(void);
+static int usb_hub_probe(struct usb_device *dev, int ifnum);
+static void usb_hub_reset(void);
 static int hub_port_reset(struct usb_device *dev, int port,
 			  unsigned short *portstat);
 

@@ -316,7 +316,7 @@ usb_set_maxpacket_ep(struct usb_device *dev, int if_idx, int ep_idx)
 /*
  * set the max packed value of all endpoints in the given configuration
  */
-int usb_set_maxpacket(struct usb_device *dev)
+static int usb_set_maxpacket(struct usb_device *dev)
 {
 	int i, ii;
 

@@ -331,7 +331,8 @@ int usb_set_maxpacket(struct usb_device *dev)
  * Parse the config, located in buffer, and fills the dev->config structure.
  * Note that all little/big endian swapping are done automatically.
  */
-int usb_parse_config(struct usb_device *dev, unsigned char *buffer, int cfgno)
+static int usb_parse_config(struct usb_device *dev,
+			unsigned char *buffer, int cfgno)
 {
 	struct usb_descriptor_header *head;
 	int index, ifno, epno, curr_if_num;

@@ -450,7 +451,7 @@ int usb_clear_halt(struct usb_device *dev, int pipe)
 /**********************************************************************
  * get_descriptor type
  */
-int usb_get_descriptor(struct usb_device *dev, unsigned char type,
+static int usb_get_descriptor(struct usb_device *dev, unsigned char type,
 			unsigned char index, void *buf, int size)
 {
 	int res;

@@ -500,7 +501,7 @@ int usb_get_configuration_no(struct usb_device *dev,
  * set address of a device to the value in dev->devnum.
  * This can only be done by addressing the device via the default address (0)
  */
-int usb_set_address(struct usb_device *dev)
+static int usb_set_address(struct usb_device *dev)
 {
 	int res;
 

@@ -553,7 +554,7 @@ int usb_set_interface(struct usb_device *dev, int interface, int alternate)
 /********************************************************************
  * set configuration number to configuration
  */
-int usb_set_configuration(struct usb_device *dev, int configuration)
+static int usb_set_configuration(struct usb_device *dev, int configuration)
 {
 	int res;
 	USB_PRINTF("set configuration %d\n", configuration);

@@ -616,7 +617,7 @@ int usb_get_class_descriptor(struct usb_device *dev, int ifnum,
 /********************************************************************
  * get string index in buffer
  */
-int usb_get_string(struct usb_device *dev, unsigned short langid,
+static int usb_get_string(struct usb_device *dev, unsigned short langid,
 		   unsigned char index, void *buf, int size)
 {
 	int i;

@@ -764,7 +765,7 @@ struct usb_device *usb_get_dev_index(int index)
 /* returns a pointer of a new device structure or NULL, if
  * no device struct is available
  */
-struct usb_device *usb_alloc_new_device(void)
+static struct usb_device *usb_alloc_new_device(void)
 {
 	int i;
 	USB_PRINTF("New Device %d\n", dev_index);

@@ -790,7 +791,7 @@ struct usb_device *usb_alloc_new_device(void)
  *
  * Returns 0 for success, != 0 for error.
  */
-int usb_new_device(struct usb_device *dev)
+static int usb_new_device(struct usb_device *dev)
 {
 	int addr, err;
 	int tmp;

@@ -954,7 +955,7 @@ int usb_new_device(struct usb_device *dev)
 }
 
 /* build device Tree  */
-void usb_scan_devices(void)
+static void usb_scan_devices(void)
 {
 	int i;
 	struct usb_device *dev;

@@ -988,42 +989,42 @@ static struct usb_hub_device hub_dev[USB_MAX_HUB];
 static int usb_hub_index;
 
 
-int usb_get_hub_descriptor(struct usb_device *dev, void *data, int size)
+static int usb_get_hub_descriptor(struct usb_device *dev, void *data, int size)
 {
 	return usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
 		USB_REQ_GET_DESCRIPTOR, USB_DIR_IN | USB_RT_HUB,
 		USB_DT_HUB << 8, 0, data, size, USB_CNTL_TIMEOUT);
 }
 
-int usb_clear_hub_feature(struct usb_device *dev, int feature)
+static int usb_clear_hub_feature(struct usb_device *dev, int feature)
 {
 	return usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
 				USB_REQ_CLEAR_FEATURE, USB_RT_HUB, feature,
 				0, NULL, 0, USB_CNTL_TIMEOUT);
 }
 
-int usb_clear_port_feature(struct usb_device *dev, int port, int feature)
+static int usb_clear_port_feature(struct usb_device *dev, int port, int feature)
 {
 	return usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
 				USB_REQ_CLEAR_FEATURE, USB_RT_PORT, feature,
 				port, NULL, 0, USB_CNTL_TIMEOUT);
 }
 
-int usb_set_port_feature(struct usb_device *dev, int port, int feature)
+static int usb_set_port_feature(struct usb_device *dev, int port, int feature)
 {
 	return usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
 				USB_REQ_SET_FEATURE, USB_RT_PORT, feature,
 				port, NULL, 0, USB_CNTL_TIMEOUT);
 }
 
-int usb_get_hub_status(struct usb_device *dev, void *data)
+static int usb_get_hub_status(struct usb_device *dev, void *data)
 {
 	return usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
 			USB_REQ_GET_STATUS, USB_DIR_IN | USB_RT_HUB, 0, 0,
 			data, sizeof(struct usb_hub_status), USB_CNTL_TIMEOUT);
 }
 
-int usb_get_port_status(struct usb_device *dev, int port, void *data)
+static int usb_get_port_status(struct usb_device *dev, int port, void *data)
 {
 	return usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
 			USB_REQ_GET_STATUS, USB_DIR_IN | USB_RT_PORT, 0, port,

@@ -1046,12 +1047,12 @@ static void usb_hub_power_on(struct usb_hub_device *hub)
 	}
 }
 
-void usb_hub_reset(void)
+static void usb_hub_reset(void)
 {
 	usb_hub_index = 0;
 }
 
-struct usb_hub_device *usb_hub_allocate(void)
+static struct usb_hub_device *usb_hub_allocate(void)
 {
 	if (usb_hub_index < USB_MAX_HUB)
 		return &hub_dev[usb_hub_index++];

@@ -1187,7 +1188,7 @@ void usb_hub_port_connect_change(struct usb_device *dev, int port)
 }
 
 
-int usb_hub_configure(struct usb_device *dev)
+static int usb_hub_configure(struct usb_device *dev)
 {
 	int i;
 	unsigned char buffer[USB_BUFSIZ], *bitmap;

@@ -1370,7 +1371,7 @@ int usb_hub_configure(struct usb_device *dev)
 	return 0;
 }
 
-int usb_hub_probe(struct usb_device *dev, int ifnum)
+static int usb_hub_probe(struct usb_device *dev, int ifnum)
 {
 	struct usb_interface *iface;
 	struct usb_endpoint_descriptor *ep;
Signature · 1 line
1.7.8.3
author
#3 lore raw

PATCH 2/4 USB: Separate out USB hub driver

In reply to Marek Vasut
Signed-off-by Marek Vasut <[email protected]>
Cc Remy Bohmer <[email protected]>
---
 common/Makefile  |    2 +-
 common/usb.c     |  432 +------------------------------------------------
 common/usb_hub.c |  483 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/usb.h    |    8 +
 4 files changed, 494 insertions(+), 431 deletions(-)
 create mode 100644 common/usb_hub.c
common/Makefile +1 -1
index 2d9ae8c..2a31c62 100644
@@ -157,7 +157,7 @@ COBJS-$(CONFIG_CMD_UNIVERSE) += cmd_universe.o
 COBJS-$(CONFIG_CMD_UNZIP) += cmd_unzip.o
 ifdef CONFIG_CMD_USB
 COBJS-y += cmd_usb.o
-COBJS-y += usb.o
+COBJS-y += usb.o usb_hub.o
 COBJS-$(CONFIG_USB_STORAGE) += usb_storage.o
 endif
 COBJS-$(CONFIG_CMD_XIMG) += cmd_ximg.o
common/usb.c +2 -430

454 lines not on this page · raw message

common/usb_hub.c new file +483 -0

483 lines not on this page · raw message

include/usb.h +8 -0
index 06170cd..9b74050 100644
@@ -367,4 +367,12 @@ struct usb_hub_device {
 	struct usb_hub_descriptor desc;
 };
 
+int usb_hub_probe(struct usb_device *dev, int ifnum);
+void usb_hub_reset(void);
+int hub_port_reset(struct usb_device *dev, int port,
+			  unsigned short *portstat);
+
+struct usb_device *usb_alloc_new_device(void);
+int usb_new_device(struct usb_device *dev);
+
 #endif /*_USB_H_ */
Signature · 1 line
1.7.8.3
author
#5 lore raw

PATCH 2/4 USB: Separate out USB hub driver

Quoted 1 line

i don't really see the point, but looks simple enough

You'll see soon enough, for now, consider it as a preparation for CONFIG_USB_MULTI.

M

Quoted 3 lines
Acked-by Mike Frysinger <[email protected]>

-mike

author
#6 lore raw

PATCH 3/4 USB: Make struct devrequest setup_packet local

In reply to Marek Vasut
Signed-off-by Marek Vasut <[email protected]>
Cc Remy Bohmer <[email protected]>
---
 common/usb.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
common/usb.c +2 -1
index 81dcbe2..3c9ede4 100644
@@ -73,7 +73,6 @@ static struct usb_device usb_dev[USB_MAX_DEVICE];
 static int dev_index;
 static int running;
 static int asynch_allowed;
-static struct devrequest setup_packet;
 
 char usb_started; /* flag for the started/stopped USB status */
 

@@ -180,6 +179,8 @@ 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;
+
 	if ((timeout == 0) && (!asynch_allowed)) {
 		/* request for a asynch control pipe is not allowed */
 		return -1;
Signature · 1 line
1.7.8.3
author
#8 lore raw

PATCH 4/4 USB: Staticize usb_storage.c

In reply to Marek Vasut

Also, make usb_stor_buf local.

Signed-off-by Marek Vasut <[email protected]>
Cc Remy Bohmer <[email protected]>
---
 common/usb_storage.c |   39 +++++++++++++++++++--------------------
 1 files changed, 19 insertions(+), 20 deletions(-)
common/usb_storage.c +19 -20
index de84c8d..1446fb0 100644
@@ -79,7 +79,6 @@ static const unsigned char us_direction[256/8] = {
 };
 #define US_DIRECTION(x) ((us_direction[x>>3] >> (x & 7)) & 1)
 
-static unsigned char usb_stor_buf[512];
 static ccb usb_ccb;
 
 /*

@@ -164,15 +163,14 @@ static struct us_data usb_stor[USB_MAX_STOR_DEV];
 #define USB_STOR_TRANSPORT_FAILED -1
 #define USB_STOR_TRANSPORT_ERROR  -2
 
-int usb_stor_get_info(struct usb_device *dev, struct us_data *us,
+static int usb_stor_get_info(struct usb_device *dev, struct us_data *us,
 		      block_dev_desc_t *dev_desc);
-int usb_storage_probe(struct usb_device *dev, unsigned int ifnum,
+static int usb_storage_probe(struct usb_device *dev, unsigned int ifnum,
 		      struct us_data *ss);
-unsigned long usb_stor_read(int device, unsigned long blknr,
+static unsigned long usb_stor_read(int device, unsigned long blknr,
 			    unsigned long blkcnt, void *buffer);
-unsigned long usb_stor_write(int device, unsigned long blknr,
+static unsigned long usb_stor_write(int device, unsigned long blknr,
 			     unsigned long blkcnt, const void *buffer);
-struct usb_device * usb_get_dev_index(int index);
 void uhci_show_temp_int_td(void);
 
 #ifdef CONFIG_PARTITIONS

@@ -182,7 +180,7 @@ block_dev_desc_t *usb_stor_get_dev(int index)
 }
 #endif
 
-void usb_show_progress(void)
+static void usb_show_progress(void)
 {
 	debug(".");
 }

@@ -233,9 +231,6 @@ int usb_stor_scan(int mode)
 	unsigned char i;
 	struct usb_device *dev;
 
-	/* GJ */
-	memset(usb_stor_buf, 0, sizeof(usb_stor_buf));
-
 	if (mode == 1)
 		printf("       scanning bus for storage devices... ");
 

@@ -493,7 +488,7 @@ static int usb_stor_CB_reset(struct us_data *us)
  * Set up the command for a BBB device. Note that the actual SCSI
  * command is copied into cbw.CBWCDB.
  */
-int usb_stor_BBB_comdat(ccb *srb, struct us_data *us)
+static int usb_stor_BBB_comdat(ccb *srb, struct us_data *us)
 {
 	int result;
 	int actlen;

@@ -541,7 +536,7 @@ int usb_stor_BBB_comdat(ccb *srb, struct us_data *us)
 /* FIXME: we also need a CBI_command which sets up the completion
  * interrupt, and waits for it
  */
-int usb_stor_CB_comdat(ccb *srb, struct us_data *us)
+static int usb_stor_CB_comdat(ccb *srb, struct us_data *us)
 {
 	int result = 0;
 	int dir_in, retry;

@@ -610,7 +605,7 @@ int usb_stor_CB_comdat(ccb *srb, struct us_data *us)
 }
 
 
-int usb_stor_CBI_get_status(ccb *srb, struct us_data *us)
+static int usb_stor_CBI_get_status(ccb *srb, struct us_data *us)
 {
 	int timeout;
 

@@ -658,7 +653,7 @@ int usb_stor_CBI_get_status(ccb *srb, struct us_data *us)
 #define USB_TRANSPORT_NOT_READY_RETRY 10
 
 /* clear a stall on an endpoint - special for BBB devices */
-int usb_stor_BBB_clear_endpt_stall(struct us_data *us, __u8 endpt)
+static int usb_stor_BBB_clear_endpt_stall(struct us_data *us, __u8 endpt)
 {
 	int result;
 

@@ -669,7 +664,7 @@ int usb_stor_BBB_clear_endpt_stall(struct us_data *us, __u8 endpt)
 	return result;
 }
 
-int usb_stor_BBB_transport(ccb *srb, struct us_data *us)
+static int usb_stor_BBB_transport(ccb *srb, struct us_data *us)
 {
 	int result, retry;
 	int dir_in;

@@ -790,7 +785,7 @@ again:
 	return result;
 }
 
-int usb_stor_CB_transport(ccb *srb, struct us_data *us)
+static int usb_stor_CB_transport(ccb *srb, struct us_data *us)
 {
 	int result, status;
 	ccb *psrb;

@@ -1042,7 +1037,7 @@ static void usb_bin_fixup(struct usb_device_descriptor descriptor,
 }
 #endif /* CONFIG_USB_BIN_FIXUP */
 
-unsigned long usb_stor_read(int device, unsigned long blknr,
+static unsigned long usb_stor_read(int device, unsigned long blknr,
 			    unsigned long blkcnt, void *buffer)
 {
 	unsigned long start, blks, buf_addr;

@@ -1118,7 +1113,7 @@ retry_it:
 	return blkcnt;
 }
 
-unsigned long usb_stor_write(int device, unsigned long blknr,
+static unsigned long usb_stor_write(int device, unsigned long blknr,
 				unsigned long blkcnt, const void *buffer)
 {
 	unsigned long start, blks, buf_addr;

@@ -1199,7 +1194,7 @@ retry_it:
 }
 
 /* Probe to see if a new device is actually a Storage device */
-int usb_storage_probe(struct usb_device *dev, unsigned int ifnum,
+static int usb_storage_probe(struct usb_device *dev, unsigned int ifnum,
 		      struct us_data *ss)
 {
 	struct usb_interface *iface;

@@ -1339,13 +1334,17 @@ int usb_storage_probe(struct usb_device *dev, unsigned int ifnum,
 	return 1;
 }
 
-int usb_stor_get_info(struct usb_device *dev, struct us_data *ss,
+static int usb_stor_get_info(struct usb_device *dev, struct us_data *ss,
 		      block_dev_desc_t *dev_desc)
 {
 	unsigned char perq, modi;
 	unsigned long cap[2];
 	unsigned long *capacity, *blksz;
 	ccb *pccb = &usb_ccb;
+	unsigned char usb_stor_buf[512];
+
+	/* GJ */
+	memset(usb_stor_buf, 0, sizeof(usb_stor_buf));
 
 	pccb->pdata = usb_stor_buf;
Signature · 1 line
1.7.8.3
#9 lore raw

PATCH 4/4 USB: Staticize usb_storage.c

Quoted 1 line · On Monday 13 February 2012 23:58:19 Marek Vasut wrote:

Also, make usb_stor_buf local.

probably should get split out into its own patch

Quoted 16 lines
common/usb_storage.c +0 -0

int usb_stor_scan(int mode)
...

common/usb_storage.c +1 -4
 	unsigned char i;
 	struct usb_device *dev;
 
-	/* GJ */
-	memset(usb_stor_buf, 0, sizeof(usb_stor_buf));
-
 	if (mode == 1)
 		printf("       scanning bus for storage devices... ");
...
-int usb_stor_get_info(struct usb_device *dev, struct us_data *ss,
+static int usb_stor_get_info(struct usb_device *dev, struct us_data *ss,

common/usb_storage.c

Quoted 10 lines
common/usb_storage.c +4 -0
 		      block_dev_desc_t *dev_desc)
 {
 	unsigned char perq, modi;
 	unsigned long cap[2];
 	unsigned long *capacity, *blksz;
 	ccb *pccb = &usb_ccb;
+	unsigned char usb_stor_buf[512];
+
+	/* GJ */
+	memset(usb_stor_buf, 0, sizeof(usb_stor_buf));

before, the code would do:

usb_stor_scan() {
	memset(usb_stor_buf)
	for (...) {
		usb_stor_get_info() {
			... use usb_stor_buf ...
		}
	}
}

now the new code calls memset multiple times. in a cursory reading of the code, i'd say just drop the memset altogether. the get_info func will return if the usb_inquiry call failed and the buffer contents don't matter. if usb_query worked, then the buffer should contain valid data and the memset was pointless.

looking a bit more, i think 512 is too big. it makes sense when it's a global dumping ground that random parts of the code would use. but this buffer is only used with usb_inquiry (which in reality is the SCSI INQUIRY command), and that should require just 36 bytes.

so my suggestion would be:
- drop usb_stor_buf changes from this patch and make a dedicated one
- drop the useless GJ comment
- drop the useless memset
- shrink the buffer to 36 bytes
-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/20120214/08b18e72/attachment.pgp>