[U-Boot] [PATCH v4 0/2] common: usb_storage : Implement logic to calculate optimal

[U-Boot] [PATCH v4 0/2] common: usb_storage : Implement logic to calculate optimal

[Rajesh Bhagat]

Performs code cleanup by making common function for usb_stor_read/write 
and implements the logic to calculate the optimal usb maximum trasfer blocks
instead of sending USB_MAX_XFER_BLK blocks which is 65535 and 20 in case
of EHCI and other USB protocols respectively.

Rajesh Bhagat (2):
  common: usb_storage: Make common function for    
    usb_stor_read/usb_stor_write
  common: usb_storage : Implement logic to calculate optimal      usb
    maximum trasfer blocks

 common/usb_storage.c | 200 ++++++++++++++++++++++++++++-----------------------
 include/usb.h        |   1 +
 2 files changed, 110 insertions(+), 91 deletions(-)

-- 
2.6.2.198.g614a2ac

[U-Boot] [PATCH v4 1/2] common: usb_storage: Make common function for usb_stor_read/usb_stor_write

[Rajesh Bhagat]

Performs code cleanup by making common function for usb_stor_read/
usb_stor_write. Currently only difference in these fucntions is call
to usb_read_10/usb_write_10 scsi commands.

Signed-off-by: Rajesh Bhagat <rajesh.bhagat@nxp.com>
---
Changes in v4: 
 - Adds code to make common function for read/write

 common/usb_storage.c | 132 ++++++++++++++++++---------------------------------
 1 file changed, 46 insertions(+), 86 deletions(-)

diff --git a/common/usb_storage.c b/common/usb_storage.c
index 7e6e52d..5efdc90 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -1104,12 +1104,20 @@ static void usb_bin_fixup(struct usb_device_descriptor descriptor,
 }
 #endif /* CONFIG_USB_BIN_FIXUP */
 
+#define USB_STOR_OPERATION	is_write ? "write" : "read"
+#define USB_STOR_OPERATION_FUNC(srb, ss, start, smallblks)		 \
+				is_write ?				 \
+				usb_write_10(srb, ss, start, smallblks) :\
+				usb_read_10(srb, ss, start, smallblks)
+
 #ifdef CONFIG_BLK
-static unsigned long usb_stor_read(struct udevice *dev, lbaint_t blknr,
-				   lbaint_t blkcnt, void *buffer)
+static unsigned long usb_stor_read_write(struct udevice *dev, lbaint_t blknr,
+					 lbaint_t blkcnt, const void *buffer,
+					 bool is_write)
 #else
-static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,
-				   lbaint_t blkcnt, void *buffer)
+static unsigned long usb_stor_read_write(struct blk_desc *block_dev,
+					 lbaint_t blknr, lbaint_t blkcnt,
+					 const void *buffer, bool is_write)
 #endif
 {
 	lbaint_t start, blks;
@@ -1129,9 +1137,9 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,
 #ifdef CONFIG_BLK
 	block_dev = dev_get_uclass_platdata(dev);
 	udev = dev_get_parent_priv(dev_get_parent(dev));
-	debug("\nusb_read: udev %d\n", block_dev->devnum);
+	debug("\nusb_%s: udev %d\n", USB_STOR_OPERATION, block_dev->devnum);
 #else
-	debug("\nusb_read: udev %d\n", block_dev->devnum);
+	debug("\nusb_%s: udev %d\n", USB_STOR_OPERATION, block_dev->devnum);
 	udev = usb_dev_desc[block_dev->devnum].priv;
 	if (!udev) {
 		debug("%s: No device\n", __func__);
@@ -1146,11 +1154,14 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,
 	start = blknr;
 	blks = blkcnt;
 
-	debug("\nusb_read: dev %d startblk " LBAF ", blccnt " LBAF " buffer %"
-	      PRIxPTR "\n", block_dev->devnum, start, blks, buf_addr);
+	debug("\nusb_%s: dev %d startblk " LBAF ", blccnt " LBAF " buffer %"
+	      PRIxPTR "\n", USB_STOR_OPERATION, block_dev->devnum, start, blks,
+	      buf_addr);
 
 	do {
-		/* XXX need some comment here */
+		/* If read/write fails retry for max retry count else
+		 * return with number of blocks written successfully.
+		 */
 		retry = 2;
 		srb->pdata = (unsigned char *)buf_addr;
 		if (blks > USB_MAX_XFER_BLK)
@@ -1162,8 +1173,8 @@ retry_it:
 			usb_show_progress();
 		srb->datalen = block_dev->blksz * smallblks;
 		srb->pdata = (unsigned char *)buf_addr;
-		if (usb_read_10(srb, ss, start, smallblks)) {
-			debug("Read ERROR\n");
+		if (USB_STOR_OPERATION_FUNC(srb, ss, start, smallblks)) {
+			debug("%s ERROR\n", USB_STOR_OPERATION);
 			usb_request_sense(srb, ss);
 			if (retry--)
 				goto retry_it;
@@ -1176,9 +1187,9 @@ retry_it:
 	} while (blks != 0);
 	ss->flags &= ~USB_READY;
 
-	debug("usb_read: end startblk " LBAF
+	debug("usb_%s: end startblk " LBAF
 	      ", blccnt %x buffer %" PRIxPTR "\n",
-	      start, smallblks, buf_addr);
+	      USB_STOR_OPERATION, start, smallblks, buf_addr);
 
 	usb_disable_asynch(0); /* asynch transfer allowed */
 	if (blkcnt >= USB_MAX_XFER_BLK)
@@ -1186,6 +1197,24 @@ retry_it:
 	return blkcnt;
 }
 
+
+#ifdef CONFIG_BLK
+static unsigned long usb_stor_read(struct udevice *dev, lbaint_t blknr,
+				   lbaint_t blkcnt, void *buffer)
+#else
+static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,
+				   lbaint_t blkcnt, void *buffer)
+#endif
+{
+	return usb_stor_read_write(
+#ifdef CONFIG_BLK
+				   dev,
+#else
+				   block_dev,
+#endif
+				   blknr, blkcnt, buffer, false);
+}
+
 #ifdef CONFIG_BLK
 static unsigned long usb_stor_write(struct udevice *dev, lbaint_t blknr,
 				    lbaint_t blkcnt, const void *buffer)
@@ -1194,82 +1223,13 @@ static unsigned long usb_stor_write(struct blk_desc *block_dev, lbaint_t blknr,
 				    lbaint_t blkcnt, const void *buffer)
 #endif
 {
-	lbaint_t start, blks;
-	uintptr_t buf_addr;
-	unsigned short smallblks;
-	struct usb_device *udev;
-	struct us_data *ss;
-	int retry;
-	ccb *srb = &usb_ccb;
+	return usb_stor_read_write(
 #ifdef CONFIG_BLK
-	struct blk_desc *block_dev;
-#endif
-
-	if (blkcnt == 0)
-		return 0;
-
-	/* Setup  device */
-#ifdef CONFIG_BLK
-	block_dev = dev_get_uclass_platdata(dev);
-	udev = dev_get_parent_priv(dev_get_parent(dev));
-	debug("\nusb_read: udev %d\n", block_dev->devnum);
+				   dev,
 #else
-	debug("\nusb_read: udev %d\n", block_dev->devnum);
-	udev = usb_dev_desc[block_dev->devnum].priv;
-	if (!udev) {
-		debug("%s: No device\n", __func__);
-		return 0;
-	}
+				   block_dev,
 #endif
-	ss = (struct us_data *)udev->privptr;
-
-	usb_disable_asynch(1); /* asynch transfer not allowed */
-
-	srb->lun = block_dev->lun;
-	buf_addr = (uintptr_t)buffer;
-	start = blknr;
-	blks = blkcnt;
-
-	debug("\nusb_write: dev %d startblk " LBAF ", blccnt " LBAF " buffer %"
-	      PRIxPTR "\n", block_dev->devnum, start, blks, buf_addr);
-
-	do {
-		/* If write fails retry for max retry count else
-		 * return with number of blocks written successfully.
-		 */
-		retry = 2;
-		srb->pdata = (unsigned char *)buf_addr;
-		if (blks > USB_MAX_XFER_BLK)
-			smallblks = USB_MAX_XFER_BLK;
-		else
-			smallblks = (unsigned short) blks;
-retry_it:
-		if (smallblks == USB_MAX_XFER_BLK)
-			usb_show_progress();
-		srb->datalen = block_dev->blksz * smallblks;
-		srb->pdata = (unsigned char *)buf_addr;
-		if (usb_write_10(srb, ss, start, smallblks)) {
-			debug("Write ERROR\n");
-			usb_request_sense(srb, ss);
-			if (retry--)
-				goto retry_it;
-			blkcnt -= blks;
-			break;
-		}
-		start += smallblks;
-		blks -= smallblks;
-		buf_addr += srb->datalen;
-	} while (blks != 0);
-	ss->flags &= ~USB_READY;
-
-	debug("usb_write: end startblk " LBAF ", blccnt %x buffer %"
-	      PRIxPTR "\n", start, smallblks, buf_addr);
-
-	usb_disable_asynch(0); /* asynch transfer allowed */
-	if (blkcnt >= USB_MAX_XFER_BLK)
-		debug("\n");
-	return blkcnt;
-
+				   blknr, blkcnt, buffer, true);
 }
 
 /* Probe to see if a new device is actually a Storage device */
-- 
2.6.2.198.g614a2ac

[U-Boot] [PATCH v4 2/2] common: usb_storage : Implement logic to calculate optimal usb maximum trasfer blocks

[Rajesh Bhagat]

From: Rajesh Bhagat <rajesh.bhagat@freescale.com>

Implements the logic to calculate the optimal usb maximum trasfer blocks
instead of sending USB_MAX_XFER_BLK blocks which is 65535 and 20 in case
of EHCI and other USB protocols respectively.

It defines USB_MIN_XFER_BLK/USB_MAX_XFER_BLK trasfer blocks that should
be checked for success starting from minimum to maximum, and rest of the
read/write are performed with that optimal value. It tries to increase/
decrease the blocks in follwing scenarios:

1.decrease blocks: when read/write for a particular number of blocks
fails.
2. increase blocks: when read/write for a particular number of blocks
pass and amount left to trasfer is greater than current number of
blocks.

Currently changes are done for EHCI where min = 4096 and max = 65535
is taken. And for other cases code is left unchanged by keeping min
= max = 20.

Signed-off-by: Sriram Dash <sriram.dash@nxp.com>
Signed-off-by: Rajesh Bhagat <rajesh.bhagat@nxp.com>
---
Changes in v4:
 - Adds udev paramater in dec/inc_cur_xfer_blks function and adds 
   sanity check on it.
 - Changes type of pos varible to unsigned int in dec/inc_cur_xfer_blks
 - Removes usage of pos varible from usb_stor_read/write
 
Changes in v3:
 - Adds cur_xfer_blks in struct usb_device to retain values
 - Adds functions dec/inc_cur_xfer_blks to remove code duplication
 - Moves check from macro to calling functions

Changes in v2:
 - Removes table to store blocks and use formula (1 << (12 + n)) - 1
 - Adds logic to start from minimum, go to maximum in each read/write

 common/usb_storage.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++----
 include/usb.h        |  1 +
 2 files changed, 64 insertions(+), 5 deletions(-)

diff --git a/common/usb_storage.c b/common/usb_storage.c
index 5efdc90..e109dfb 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -106,11 +106,16 @@ struct us_data {
  * enough free heap space left, but the SCSI READ(10) and WRITE(10) commands are
  * limited to 65535 blocks.
  */
+#define USB_MIN_XFER_BLK	4095
 #define USB_MAX_XFER_BLK	65535
 #else
+#define USB_MIN_XFER_BLK	20
 #define USB_MAX_XFER_BLK	20
 #endif
 
+#define GET_CUR_XFER_BLKS(blks)	(LOG2((blks + 1) / (USB_MIN_XFER_BLK + 1)))
+#define CALC_CUR_XFER_BLKS(pos)	((1 << (12 + pos)) - 1)
+
 #ifndef CONFIG_BLK
 static struct us_data usb_stor[USB_MAX_STOR_DEV];
 #endif
@@ -141,6 +146,44 @@ static void usb_show_progress(void)
 	debug(".");
 }
 
+static int dec_cur_xfer_blks(struct usb_device *udev)
+{
+	/* decrease the cur_xfer_blks */
+	unsigned int pos;
+	unsigned short size;
+
+	if (!udev)
+		return -EINVAL;
+
+	pos = GET_CUR_XFER_BLKS(udev->cur_xfer_blks);
+	size  = pos ? CALC_CUR_XFER_BLKS(pos - 1) : 0;
+
+	if (size < USB_MIN_XFER_BLK)
+		return -EINVAL;
+
+	udev->cur_xfer_blks = size;
+	return 0;
+}
+
+static int inc_cur_xfer_blks(struct usb_device *udev, lbaint_t blks)
+{
+	/* try to increase the cur_xfer_blks */
+	unsigned int pos;
+	unsigned short size;
+
+	if (!udev)
+		return -EINVAL;
+
+	pos = GET_CUR_XFER_BLKS(udev->cur_xfer_blks);
+	size = CALC_CUR_XFER_BLKS(pos + 1);
+
+	if (size > blks || size > USB_MAX_XFER_BLK)
+		return -EINVAL;
+
+	udev->cur_xfer_blks = size;
+	return 0;
+}
+
 /*******************************************************************************
  * show info on storage devices; 'usb start/init' must be invoked earlier
  * as we only retrieve structures populated during devices initialization
@@ -1126,6 +1169,7 @@ static unsigned long usb_stor_read_write(struct blk_desc *block_dev,
 	struct usb_device *udev;
 	struct us_data *ss;
 	int retry;
+	bool retry_flag = false;
 	ccb *srb = &usb_ccb;
 #ifdef CONFIG_BLK
 	struct blk_desc *block_dev;
@@ -1164,26 +1208,37 @@ static unsigned long usb_stor_read_write(struct blk_desc *block_dev,
 		 */
 		retry = 2;
 		srb->pdata = (unsigned char *)buf_addr;
-		if (blks > USB_MAX_XFER_BLK)
-			smallblks = USB_MAX_XFER_BLK;
+		if (blks > udev->cur_xfer_blks)
+			smallblks = udev->cur_xfer_blks;
 		else
 			smallblks = (unsigned short) blks;
 retry_it:
-		if (smallblks == USB_MAX_XFER_BLK)
+		debug("usb_%s: retry #%d, cur_xfer_blks %hu, smallblks %hu\n",
+		      USB_STOR_OPERATION, retry, udev->cur_xfer_blks,
+		      smallblks);
+		if (smallblks == udev->cur_xfer_blks)
 			usb_show_progress();
 		srb->datalen = block_dev->blksz * smallblks;
 		srb->pdata = (unsigned char *)buf_addr;
 		if (USB_STOR_OPERATION_FUNC(srb, ss, start, smallblks)) {
 			debug("%s ERROR\n", USB_STOR_OPERATION);
 			usb_request_sense(srb, ss);
-			if (retry--)
+			if (retry--) {
+				if (!dec_cur_xfer_blks(udev))
+					smallblks = udev->cur_xfer_blks;
+				retry_flag = true;
 				goto retry_it;
+			}
 			blkcnt -= blks;
 			break;
 		}
 		start += smallblks;
 		blks -= smallblks;
 		buf_addr += srb->datalen;
+
+		if (!retry_flag && !inc_cur_xfer_blks(udev, blks))
+			smallblks = udev->cur_xfer_blks;
+
 	} while (blks != 0);
 	ss->flags &= ~USB_READY;
 
@@ -1192,7 +1247,7 @@ retry_it:
 	      USB_STOR_OPERATION, start, smallblks, buf_addr);
 
 	usb_disable_asynch(0); /* asynch transfer allowed */
-	if (blkcnt >= USB_MAX_XFER_BLK)
+	if (blkcnt >= udev->cur_xfer_blks)
 		debug("\n");
 	return blkcnt;
 }
@@ -1291,6 +1346,9 @@ int usb_storage_probe(struct usb_device *dev, unsigned int ifnum,
 		break;
 	}
 
+	/* Initialize the current transfer blocks to minimum value */
+	dev->cur_xfer_blks = USB_MIN_XFER_BLK;
+
 	/*
 	 * We are expecting a minimum of 2 endpoints - in and out (bulk).
 	 * An optional interrupt is OK (necessary for CBI protocol).
diff --git a/include/usb.h b/include/usb.h
index 02a0ccd..b815816 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -153,6 +153,7 @@ struct usb_device {
 	struct udevice *dev;		/* Pointer to associated device */
 	struct udevice *controller_dev;	/* Pointer to associated controller */
 #endif
+	unsigned short cur_xfer_blks;   /* Current maximum transfer blocks */
 };
 
 struct int_queue;
-- 
2.6.2.198.g614a2ac

[U-Boot] [PATCH v4 1/2] common: usb_storage: Make common function for usb_stor_read/usb_stor_write

[Marek Vasut]

On 06/09/2016 08:32 AM, Rajesh Bhagat wrote:
> Performs code cleanup by making common function for usb_stor_read/
> usb_stor_write. Currently only difference in these fucntions is call
> to usb_read_10/usb_write_10 scsi commands.
> 
> Signed-off-by: Rajesh Bhagat <rajesh.bhagat@nxp.com>
> ---
> Changes in v4: 
>  - Adds code to make common function for read/write
> 
>  common/usb_storage.c | 132 ++++++++++++++++++---------------------------------
>  1 file changed, 46 insertions(+), 86 deletions(-)
> 
> diff --git a/common/usb_storage.c b/common/usb_storage.c
> index 7e6e52d..5efdc90 100644
> --- a/common/usb_storage.c
> +++ b/common/usb_storage.c
> @@ -1104,12 +1104,20 @@ static void usb_bin_fixup(struct usb_device_descriptor descriptor,
>  }
>  #endif /* CONFIG_USB_BIN_FIXUP */
>  
> +#define USB_STOR_OPERATION	is_write ? "write" : "read"
> +#define USB_STOR_OPERATION_FUNC(srb, ss, start, smallblks)		 \
> +				is_write ?				 \
> +				usb_write_10(srb, ss, start, smallblks) :\
> +				usb_read_10(srb, ss, start, smallblks)

Turn this into a function. The macro is obviously missing even the most
basic parenthesis, but that's beyond the point.

> +
>  #ifdef CONFIG_BLK
> -static unsigned long usb_stor_read(struct udevice *dev, lbaint_t blknr,
> -				   lbaint_t blkcnt, void *buffer)
> +static unsigned long usb_stor_read_write(struct udevice *dev, lbaint_t blknr,
> +					 lbaint_t blkcnt, const void *buffer,
> +					 bool is_write)
>  #else
> -static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,
> -				   lbaint_t blkcnt, void *buffer)
> +static unsigned long usb_stor_read_write(struct blk_desc *block_dev,
> +					 lbaint_t blknr, lbaint_t blkcnt,
> +					 const void *buffer, bool is_write)
>  #endif
>  {
>  	lbaint_t start, blks;
> @@ -1129,9 +1137,9 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,
>  #ifdef CONFIG_BLK
>  	block_dev = dev_get_uclass_platdata(dev);
>  	udev = dev_get_parent_priv(dev_get_parent(dev));
> -	debug("\nusb_read: udev %d\n", block_dev->devnum);
> +	debug("\nusb_%s: udev %d\n", USB_STOR_OPERATION, block_dev->devnum);
>  #else
> -	debug("\nusb_read: udev %d\n", block_dev->devnum);
> +	debug("\nusb_%s: udev %d\n", USB_STOR_OPERATION, block_dev->devnum);
>  	udev = usb_dev_desc[block_dev->devnum].priv;
>  	if (!udev) {
>  		debug("%s: No device\n", __func__);
> @@ -1146,11 +1154,14 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,
>  	start = blknr;
>  	blks = blkcnt;
>  
> -	debug("\nusb_read: dev %d startblk " LBAF ", blccnt " LBAF " buffer %"
> -	      PRIxPTR "\n", block_dev->devnum, start, blks, buf_addr);
> +	debug("\nusb_%s: dev %d startblk " LBAF ", blccnt " LBAF " buffer %"
> +	      PRIxPTR "\n", USB_STOR_OPERATION, block_dev->devnum, start, blks,
> +	      buf_addr);
>  
>  	do {
> -		/* XXX need some comment here */

Multi-line comment goes this way:
/*
 * foo
 * bar
 */
It's in the coding style document too, please read it.

> +		/* If read/write fails retry for max retry count else
> +		 * return with number of blocks written successfully.
> +		 */
>  		retry = 2;
>  		srb->pdata = (unsigned char *)buf_addr;
>  		if (blks > USB_MAX_XFER_BLK)
> @@ -1162,8 +1173,8 @@ retry_it:
>  			usb_show_progress();
>  		srb->datalen = block_dev->blksz * smallblks;
>  		srb->pdata = (unsigned char *)buf_addr;
> -		if (usb_read_10(srb, ss, start, smallblks)) {
> -			debug("Read ERROR\n");
> +		if (USB_STOR_OPERATION_FUNC(srb, ss, start, smallblks)) {
> +			debug("%s ERROR\n", USB_STOR_OPERATION);
>  			usb_request_sense(srb, ss);
>  			if (retry--)
>  				goto retry_it;
> @@ -1176,9 +1187,9 @@ retry_it:
>  	} while (blks != 0);
>  	ss->flags &= ~USB_READY;
>  
> -	debug("usb_read: end startblk " LBAF
> +	debug("usb_%s: end startblk " LBAF
>  	      ", blccnt %x buffer %" PRIxPTR "\n",
> -	      start, smallblks, buf_addr);
> +	      USB_STOR_OPERATION, start, smallblks, buf_addr);
>  
>  	usb_disable_asynch(0); /* asynch transfer allowed */
>  	if (blkcnt >= USB_MAX_XFER_BLK)
> @@ -1186,6 +1197,24 @@ retry_it:
>  	return blkcnt;
>  }
>  
> +
> +#ifdef CONFIG_BLK
> +static unsigned long usb_stor_read(struct udevice *dev, lbaint_t blknr,
> +				   lbaint_t blkcnt, void *buffer)
> +#else
> +static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,
> +				   lbaint_t blkcnt, void *buffer)
> +#endif
> +{
> +	return usb_stor_read_write(
> +#ifdef CONFIG_BLK

Rename the variable to be always dev and drop this ifdef ... please,
review the patches before submitting to remove such stupid omissions
from them, it's wasting both my time and yours.

> +				   dev,
> +#else
> +				   block_dev,
> +#endif
> +				   blknr, blkcnt, buffer, false);
> +}
> +
>  #ifdef CONFIG_BLK
>  static unsigned long usb_stor_write(struct udevice *dev, lbaint_t blknr,
>  				    lbaint_t blkcnt, const void *buffer)
> @@ -1194,82 +1223,13 @@ static unsigned long usb_stor_write(struct blk_desc *block_dev, lbaint_t blknr,
>  				    lbaint_t blkcnt, const void *buffer)
>  #endif
>  {
> -	lbaint_t start, blks;
> -	uintptr_t buf_addr;
> -	unsigned short smallblks;
> -	struct usb_device *udev;
> -	struct us_data *ss;
> -	int retry;
> -	ccb *srb = &usb_ccb;
> +	return usb_stor_read_write(
>  #ifdef CONFIG_BLK
> -	struct blk_desc *block_dev;
> -#endif
> -
> -	if (blkcnt == 0)
> -		return 0;
> -
> -	/* Setup  device */
> -#ifdef CONFIG_BLK
> -	block_dev = dev_get_uclass_platdata(dev);
> -	udev = dev_get_parent_priv(dev_get_parent(dev));
> -	debug("\nusb_read: udev %d\n", block_dev->devnum);
> +				   dev,
>  #else
> -	debug("\nusb_read: udev %d\n", block_dev->devnum);
> -	udev = usb_dev_desc[block_dev->devnum].priv;
> -	if (!udev) {
> -		debug("%s: No device\n", __func__);
> -		return 0;
> -	}
> +				   block_dev,
>  #endif
> -	ss = (struct us_data *)udev->privptr;
> -
> -	usb_disable_asynch(1); /* asynch transfer not allowed */
> -
> -	srb->lun = block_dev->lun;
> -	buf_addr = (uintptr_t)buffer;
> -	start = blknr;
> -	blks = blkcnt;
> -
> -	debug("\nusb_write: dev %d startblk " LBAF ", blccnt " LBAF " buffer %"
> -	      PRIxPTR "\n", block_dev->devnum, start, blks, buf_addr);
> -
> -	do {
> -		/* If write fails retry for max retry count else
> -		 * return with number of blocks written successfully.
> -		 */
> -		retry = 2;
> -		srb->pdata = (unsigned char *)buf_addr;
> -		if (blks > USB_MAX_XFER_BLK)
> -			smallblks = USB_MAX_XFER_BLK;
> -		else
> -			smallblks = (unsigned short) blks;
> -retry_it:
> -		if (smallblks == USB_MAX_XFER_BLK)
> -			usb_show_progress();
> -		srb->datalen = block_dev->blksz * smallblks;
> -		srb->pdata = (unsigned char *)buf_addr;
> -		if (usb_write_10(srb, ss, start, smallblks)) {
> -			debug("Write ERROR\n");
> -			usb_request_sense(srb, ss);
> -			if (retry--)
> -				goto retry_it;
> -			blkcnt -= blks;
> -			break;
> -		}
> -		start += smallblks;
> -		blks -= smallblks;
> -		buf_addr += srb->datalen;
> -	} while (blks != 0);
> -	ss->flags &= ~USB_READY;
> -
> -	debug("usb_write: end startblk " LBAF ", blccnt %x buffer %"
> -	      PRIxPTR "\n", start, smallblks, buf_addr);
> -
> -	usb_disable_asynch(0); /* asynch transfer allowed */
> -	if (blkcnt >= USB_MAX_XFER_BLK)
> -		debug("\n");
> -	return blkcnt;
> -
> +				   blknr, blkcnt, buffer, true);
>  }
>  
>  /* Probe to see if a new device is actually a Storage device */
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v4 1/2] common: usb_storage: Make common function for usb_stor_read/usb_stor_write

[Rajesh Bhagat]

> -----Original Message-----
> From: Marek Vasut [mailto:marex at denx.de]
> Sent: Thursday, June 09, 2016 8:21 PM
> To: Rajesh Bhagat <rajesh.bhagat@nxp.com>; u-boot at lists.denx.de
> Cc: sjg at chromium.org; york sun <york.sun@nxp.com>; Sriram Dash
> <sriram.dash@nxp.com>
> Subject: Re: [PATCH v4 1/2] common: usb_storage: Make common function for
> usb_stor_read/usb_stor_write
> 
> On 06/09/2016 08:32 AM, Rajesh Bhagat wrote:
> > Performs code cleanup by making common function for usb_stor_read/
> > usb_stor_write. Currently only difference in these fucntions is call
> > to usb_read_10/usb_write_10 scsi commands.
> >
> > Signed-off-by: Rajesh Bhagat <rajesh.bhagat@nxp.com>
> > ---
> > Changes in v4:
> >  - Adds code to make common function for read/write
> >
> >  common/usb_storage.c | 132
> > ++++++++++++++++++---------------------------------
> >  1 file changed, 46 insertions(+), 86 deletions(-)
> >
> > diff --git a/common/usb_storage.c b/common/usb_storage.c index
> > 7e6e52d..5efdc90 100644
> > --- a/common/usb_storage.c
> > +++ b/common/usb_storage.c
> > @@ -1104,12 +1104,20 @@ static void usb_bin_fixup(struct
> > usb_device_descriptor descriptor,  }  #endif /* CONFIG_USB_BIN_FIXUP
> > */
> >
> > +#define USB_STOR_OPERATION	is_write ? "write" : "read"
> > +#define USB_STOR_OPERATION_FUNC(srb, ss, start, smallblks)		 \
> > +				is_write ?				 \
> > +				usb_write_10(srb, ss, start, smallblks) :\
> > +				usb_read_10(srb, ss, start, smallblks)
> 
> Turn this into a function. The macro is obviously missing even the most basic
> parenthesis, but that's beyond the point.
> 

Ok, I will change it into a function in v5. 

> > +
> >  #ifdef CONFIG_BLK
> > -static unsigned long usb_stor_read(struct udevice *dev, lbaint_t blknr,
> > -				   lbaint_t blkcnt, void *buffer)
> > +static unsigned long usb_stor_read_write(struct udevice *dev, lbaint_t blknr,
> > +					 lbaint_t blkcnt, const void *buffer,
> > +					 bool is_write)
> >  #else
> > -static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,
> > -				   lbaint_t blkcnt, void *buffer)
> > +static unsigned long usb_stor_read_write(struct blk_desc *block_dev,
> > +					 lbaint_t blknr, lbaint_t blkcnt,
> > +					 const void *buffer, bool is_write)
> >  #endif
> >  {
> >  	lbaint_t start, blks;
> > @@ -1129,9 +1137,9 @@ static unsigned long usb_stor_read(struct
> > blk_desc *block_dev, lbaint_t blknr,  #ifdef CONFIG_BLK
> >  	block_dev = dev_get_uclass_platdata(dev);
> >  	udev = dev_get_parent_priv(dev_get_parent(dev));
> > -	debug("\nusb_read: udev %d\n", block_dev->devnum);
> > +	debug("\nusb_%s: udev %d\n", USB_STOR_OPERATION, block_dev->devnum);
> >  #else
> > -	debug("\nusb_read: udev %d\n", block_dev->devnum);
> > +	debug("\nusb_%s: udev %d\n", USB_STOR_OPERATION, block_dev->devnum);
> >  	udev = usb_dev_desc[block_dev->devnum].priv;
> >  	if (!udev) {
> >  		debug("%s: No device\n", __func__); @@ -1146,11 +1154,14 @@ static
> > unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,
> >  	start = blknr;
> >  	blks = blkcnt;
> >
> > -	debug("\nusb_read: dev %d startblk " LBAF ", blccnt " LBAF " buffer %"
> > -	      PRIxPTR "\n", block_dev->devnum, start, blks, buf_addr);
> > +	debug("\nusb_%s: dev %d startblk " LBAF ", blccnt " LBAF " buffer %"
> > +	      PRIxPTR "\n", USB_STOR_OPERATION, block_dev->devnum, start, blks,
> > +	      buf_addr);
> >
> >  	do {
> > -		/* XXX need some comment here */
> 
> Multi-line comment goes this way:
> /*
>  * foo
>  * bar
>  */
> It's in the coding style document too, please read it.
> 

My apologies, I took it from usb_stor_write function and din't noticed the coding style issue
in the existing code. Will correct it in v5.

> > +		/* If read/write fails retry for max retry count else
> > +		 * return with number of blocks written successfully.
> > +		 */
> >  		retry = 2;
> >  		srb->pdata = (unsigned char *)buf_addr;
> >  		if (blks > USB_MAX_XFER_BLK)
> > @@ -1162,8 +1173,8 @@ retry_it:
> >  			usb_show_progress();
> >  		srb->datalen = block_dev->blksz * smallblks;
> >  		srb->pdata = (unsigned char *)buf_addr;
> > -		if (usb_read_10(srb, ss, start, smallblks)) {
> > -			debug("Read ERROR\n");
> > +		if (USB_STOR_OPERATION_FUNC(srb, ss, start, smallblks)) {
> > +			debug("%s ERROR\n", USB_STOR_OPERATION);
> >  			usb_request_sense(srb, ss);
> >  			if (retry--)
> >  				goto retry_it;
> > @@ -1176,9 +1187,9 @@ retry_it:
> >  	} while (blks != 0);
> >  	ss->flags &= ~USB_READY;
> >
> > -	debug("usb_read: end startblk " LBAF
> > +	debug("usb_%s: end startblk " LBAF
> >  	      ", blccnt %x buffer %" PRIxPTR "\n",
> > -	      start, smallblks, buf_addr);
> > +	      USB_STOR_OPERATION, start, smallblks, buf_addr);
> >
> >  	usb_disable_asynch(0); /* asynch transfer allowed */
> >  	if (blkcnt >= USB_MAX_XFER_BLK)
> > @@ -1186,6 +1197,24 @@ retry_it:
> >  	return blkcnt;
> >  }
> >
> > +
> > +#ifdef CONFIG_BLK
> > +static unsigned long usb_stor_read(struct udevice *dev, lbaint_t blknr,
> > +				   lbaint_t blkcnt, void *buffer) #else static unsigned long
> > +usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,
> > +				   lbaint_t blkcnt, void *buffer) #endif {
> > +	return usb_stor_read_write(
> > +#ifdef CONFIG_BLK
> 
> Rename the variable to be always dev and drop this ifdef ... please, review the
> patches before submitting to remove such stupid omissions from them, it's wasting
> both my time and yours.
> 

I understand your concern, but my intent was to keep the exiting prototypes unchanged, may
be difference of thought process. Will take care in v5. 

Best Regards,
Rajesh Bhagat 


> > +				   dev,
> > +#else
> > +				   block_dev,
> > +#endif
> > +				   blknr, blkcnt, buffer, false); }
> > +
> >  #ifdef CONFIG_BLK
> >  static unsigned long usb_stor_write(struct udevice *dev, lbaint_t blknr,
> >  				    lbaint_t blkcnt, const void *buffer) @@ -1194,82
> +1223,13 @@
> > static unsigned long usb_stor_write(struct blk_desc *block_dev, lbaint_t blknr,
> >  				    lbaint_t blkcnt, const void *buffer)  #endif  {
> > -	lbaint_t start, blks;
> > -	uintptr_t buf_addr;
> > -	unsigned short smallblks;
> > -	struct usb_device *udev;
> > -	struct us_data *ss;
> > -	int retry;
> > -	ccb *srb = &usb_ccb;
> > +	return usb_stor_read_write(
> >  #ifdef CONFIG_BLK
> > -	struct blk_desc *block_dev;
> > -#endif
> > -
> > -	if (blkcnt == 0)
> > -		return 0;
> > -
> > -	/* Setup  device */
> > -#ifdef CONFIG_BLK
> > -	block_dev = dev_get_uclass_platdata(dev);
> > -	udev = dev_get_parent_priv(dev_get_parent(dev));
> > -	debug("\nusb_read: udev %d\n", block_dev->devnum);
> > +				   dev,
> >  #else
> > -	debug("\nusb_read: udev %d\n", block_dev->devnum);
> > -	udev = usb_dev_desc[block_dev->devnum].priv;
> > -	if (!udev) {
> > -		debug("%s: No device\n", __func__);
> > -		return 0;
> > -	}
> > +				   block_dev,
> >  #endif
> > -	ss = (struct us_data *)udev->privptr;
> > -
> > -	usb_disable_asynch(1); /* asynch transfer not allowed */
> > -
> > -	srb->lun = block_dev->lun;
> > -	buf_addr = (uintptr_t)buffer;
> > -	start = blknr;
> > -	blks = blkcnt;
> > -
> > -	debug("\nusb_write: dev %d startblk " LBAF ", blccnt " LBAF " buffer %"
> > -	      PRIxPTR "\n", block_dev->devnum, start, blks, buf_addr);
> > -
> > -	do {
> > -		/* If write fails retry for max retry count else
> > -		 * return with number of blocks written successfully.
> > -		 */
> > -		retry = 2;
> > -		srb->pdata = (unsigned char *)buf_addr;
> > -		if (blks > USB_MAX_XFER_BLK)
> > -			smallblks = USB_MAX_XFER_BLK;
> > -		else
> > -			smallblks = (unsigned short) blks;
> > -retry_it:
> > -		if (smallblks == USB_MAX_XFER_BLK)
> > -			usb_show_progress();
> > -		srb->datalen = block_dev->blksz * smallblks;
> > -		srb->pdata = (unsigned char *)buf_addr;
> > -		if (usb_write_10(srb, ss, start, smallblks)) {
> > -			debug("Write ERROR\n");
> > -			usb_request_sense(srb, ss);
> > -			if (retry--)
> > -				goto retry_it;
> > -			blkcnt -= blks;
> > -			break;
> > -		}
> > -		start += smallblks;
> > -		blks -= smallblks;
> > -		buf_addr += srb->datalen;
> > -	} while (blks != 0);
> > -	ss->flags &= ~USB_READY;
> > -
> > -	debug("usb_write: end startblk " LBAF ", blccnt %x buffer %"
> > -	      PRIxPTR "\n", start, smallblks, buf_addr);
> > -
> > -	usb_disable_asynch(0); /* asynch transfer allowed */
> > -	if (blkcnt >= USB_MAX_XFER_BLK)
> > -		debug("\n");
> > -	return blkcnt;
> > -
> > +				   blknr, blkcnt, buffer, true);
> >  }
> >
> >  /* Probe to see if a new device is actually a Storage device */
> >
> 
> 
> --
> Best regards,
> Marek Vasut

[U-Boot] [PATCH v4 2/2] common: usb_storage : Implement logic to calculate optimal usb maximum trasfer blocks

[Simon Glass]

On 9 June 2016 at 00:32, Rajesh Bhagat <rajesh.bhagat@nxp.com> wrote:
> From: Rajesh Bhagat <rajesh.bhagat@freescale.com>
>
> Implements the logic to calculate the optimal usb maximum trasfer blocks
> instead of sending USB_MAX_XFER_BLK blocks which is 65535 and 20 in case
> of EHCI and other USB protocols respectively.
>
> It defines USB_MIN_XFER_BLK/USB_MAX_XFER_BLK trasfer blocks that should
> be checked for success starting from minimum to maximum, and rest of the
> read/write are performed with that optimal value. It tries to increase/
> decrease the blocks in follwing scenarios:
>
> 1.decrease blocks: when read/write for a particular number of blocks
> fails.
> 2. increase blocks: when read/write for a particular number of blocks
> pass and amount left to trasfer is greater than current number of
> blocks.
>
> Currently changes are done for EHCI where min = 4096 and max = 65535
> is taken. And for other cases code is left unchanged by keeping min
> = max = 20.
>
> Signed-off-by: Sriram Dash <sriram.dash@nxp.com>
> Signed-off-by: Rajesh Bhagat <rajesh.bhagat@nxp.com>
> ---
> Changes in v4:
>  - Adds udev paramater in dec/inc_cur_xfer_blks function and adds
>    sanity check on it.
>  - Changes type of pos varible to unsigned int in dec/inc_cur_xfer_blks
>  - Removes usage of pos varible from usb_stor_read/write
>
> Changes in v3:
>  - Adds cur_xfer_blks in struct usb_device to retain values
>  - Adds functions dec/inc_cur_xfer_blks to remove code duplication
>  - Moves check from macro to calling functions
>
> Changes in v2:
>  - Removes table to store blocks and use formula (1 << (12 + n)) - 1
>  - Adds logic to start from minimum, go to maximum in each read/write
>
>  common/usb_storage.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++----
>  include/usb.h        |  1 +
>  2 files changed, 64 insertions(+), 5 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

I'm not keen on the 'unsigned short' for the size. Perhaps add a
comment to your 65535 #define that it cannot go higher?