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

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

[Rajesh Bhagat]

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

Sample Behaviour: Fallback to 16384 blocks once failure is seen on 32768.

=> usb write a0000000 0 10000;

USB write: device 0 block # 0, count 65536 ... usb_write: retry #2, xfer_blk 4096, smallblks 4096
usb_write: retry #2, xfer_blk 8192, smallblks 8192
usb_write: retry #2, xfer_blk 16384, smallblks 16384
usb_write: retry #2, xfer_blk 32768, smallblks 32768
EHCI timed out on TD - token=0x10008c80
usb_write: retry #1, xfer_blk 16384, smallblks 16384
usb_write: retry #2, xfer_blk 16384, smallblks 16384
usb_write: retry #2, xfer_blk 16384, smallblks 4096
65536 blocks write: OK

Rajesh Bhagat (2):
  common: usb_storage : Implement logic to calculate optimal usb maximum
    trasfer blocks
  common: usb_storage : Seperate optimal blocks logic calculation for
    read/write

 common/usb_storage.c | 66 +++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 47 insertions(+), 19 deletions(-)

-- 
2.6.2.198.g614a2ac

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

[Rajesh Bhagat]

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 adds an array of 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 andmax = 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>
---
 common/usb_storage.c |   54 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 11 deletions(-)

diff --git a/common/usb_storage.c b/common/usb_storage.c
index 7e6e52d..7b5ad07 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -101,16 +101,15 @@ struct us_data {
 };
 
 #ifdef CONFIG_USB_EHCI
-/*
- * The U-Boot EHCI driver can handle any transfer length as long as there is
- * enough free heap space left, but the SCSI READ(10) and WRITE(10) commands are
- * limited to 65535 blocks.
- */
-#define USB_MAX_XFER_BLK	65535
+#define USB_XFER_BLK_TBL_SZ		5
+static unsigned short usb_xfer_blk_tbl[5] = {4096, 8192, 16384, 32768, 65535};
 #else
-#define USB_MAX_XFER_BLK	20
+#define USB_XFER_BLK_TBL_SZ		1
+static unsigned short usb_xfer_blk_tbl[1] = {20};
 #endif
 
+static unsigned short USB_MAX_XFER_BLK;
+
 #ifndef CONFIG_BLK
 static struct us_data usb_stor[USB_MAX_STOR_DEV];
 #endif
@@ -1117,7 +1116,8 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,
 	unsigned short smallblks;
 	struct usb_device *udev;
 	struct us_data *ss;
-	int retry;
+	int retry, next = LOG2((USB_MAX_XFER_BLK + 1) / usb_xfer_blk_tbl[0]);
+	bool retry_flag = false;
 	ccb *srb = &usb_ccb;
 #ifdef CONFIG_BLK
 	struct blk_desc *block_dev;
@@ -1158,6 +1158,8 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,
 		else
 			smallblks = (unsigned short) blks;
 retry_it:
+		debug("usb_read: retry #%d, xfer_blk %hu, smallblks %hu\n",
+		      retry, USB_MAX_XFER_BLK, smallblks);
 		if (smallblks == USB_MAX_XFER_BLK)
 			usb_show_progress();
 		srb->datalen = block_dev->blksz * smallblks;
@@ -1165,14 +1167,26 @@ retry_it:
 		if (usb_read_10(srb, ss, start, smallblks)) {
 			debug("Read ERROR\n");
 			usb_request_sense(srb, ss);
-			if (retry--)
+			if (retry--) {
+				/* decrease the USB_MAX_XFER_BLK */
+				if (next >  0) {
+					smallblks = usb_xfer_blk_tbl[--next];
+					USB_MAX_XFER_BLK = smallblks;
+				}
+				retry_flag = true;
 				goto retry_it;
+			}
 			blkcnt -= blks;
 			break;
 		}
 		start += smallblks;
 		blks -= smallblks;
 		buf_addr += srb->datalen;
+
+		/* try to increase the USB_MAX_XFER_BLK */
+		if (next < USB_XFER_BLK_TBL_SZ  - 1)
+			if (!retry_flag && usb_xfer_blk_tbl[next + 1] <= blks)
+				USB_MAX_XFER_BLK = usb_xfer_blk_tbl[++next];
 	} while (blks != 0);
 	ss->flags &= ~USB_READY;
 
@@ -1199,7 +1213,8 @@ static unsigned long usb_stor_write(struct blk_desc *block_dev, lbaint_t blknr,
 	unsigned short smallblks;
 	struct usb_device *udev;
 	struct us_data *ss;
-	int retry;
+	int retry, next = LOG2((USB_MAX_XFER_BLK + 1) / usb_xfer_blk_tbl[0]);
+	bool retry_flag = false;
 	ccb *srb = &usb_ccb;
 #ifdef CONFIG_BLK
 	struct blk_desc *block_dev;
@@ -1244,6 +1259,8 @@ static unsigned long usb_stor_write(struct blk_desc *block_dev, lbaint_t blknr,
 		else
 			smallblks = (unsigned short) blks;
 retry_it:
+		debug("usb_write: retry #%d, xfer_blk %hu, smallblks %hu\n",
+		      retry, USB_MAX_XFER_BLK, smallblks);
 		if (smallblks == USB_MAX_XFER_BLK)
 			usb_show_progress();
 		srb->datalen = block_dev->blksz * smallblks;
@@ -1251,14 +1268,26 @@ retry_it:
 		if (usb_write_10(srb, ss, start, smallblks)) {
 			debug("Write ERROR\n");
 			usb_request_sense(srb, ss);
-			if (retry--)
+			if (retry--) {
+				/* decrease the USB_MAX_XFER_BLK */
+				if (next >  0) {
+					smallblks = usb_xfer_blk_tbl[--next];
+					USB_MAX_XFER_BLK = smallblks;
+				}
+				retry_flag = true;
 				goto retry_it;
+			}
 			blkcnt -= blks;
 			break;
 		}
 		start += smallblks;
 		blks -= smallblks;
 		buf_addr += srb->datalen;
+
+		/* try to increase the USB_MAX_XFER_BLK */
+		if (next < USB_XFER_BLK_TBL_SZ  - 1)
+			if (!retry_flag && usb_xfer_blk_tbl[next + 1] <= blks)
+				USB_MAX_XFER_BLK = usb_xfer_blk_tbl[++next];
 	} while (blks != 0);
 	ss->flags &= ~USB_READY;
 
@@ -1331,6 +1360,9 @@ int usb_storage_probe(struct usb_device *dev, unsigned int ifnum,
 		break;
 	}
 
+	/* Initialize the current transfer blocks to minimum value */
+	USB_MAX_XFER_BLK = usb_xfer_blk_tbl[0];
+
 	/*
 	 * We are expecting a minimum of 2 endpoints - in and out (bulk).
 	 * An optional interrupt is OK (necessary for CBI protocol).
-- 
1.7.9.5

[U-Boot] [PATCH 2/2] common: usb_storage : Seperate optimal blocks logic calculation for read/write

[Rajesh Bhagat]

Seperates optimal blocks logic calculation for read/write by defining
the usb_max_xfer_blk variable for usb_stor_read and usb_stor_write.

It has been observed, that USB devices are tend to be slower in write as
compared to read operation. Hence, should be handled in diffrent manner.

Signed-off-by: Sriram Dash <sriram.dash@nxp.com>
Signed-off-by: Rajesh Bhagat <rajesh.bhagat@nxp.com>
---
 common/usb_storage.c |   45 +++++++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/common/usb_storage.c b/common/usb_storage.c
index 7b5ad07..fb3c58a 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -108,7 +108,7 @@ static unsigned short usb_xfer_blk_tbl[5] = {4096, 8192, 16384, 32768, 65535};
 static unsigned short usb_xfer_blk_tbl[1] = {20};
 #endif
 
-static unsigned short USB_MAX_XFER_BLK;
+static unsigned short usb_read_xfer_blk, usb_write_xfer_blk;
 
 #ifndef CONFIG_BLK
 static struct us_data usb_stor[USB_MAX_STOR_DEV];
@@ -1116,7 +1116,7 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,
 	unsigned short smallblks;
 	struct usb_device *udev;
 	struct us_data *ss;
-	int retry, next = LOG2((USB_MAX_XFER_BLK + 1) / usb_xfer_blk_tbl[0]);
+	int retry, next = LOG2((usb_read_xfer_blk + 1) / usb_xfer_blk_tbl[0]);
 	bool retry_flag = false;
 	ccb *srb = &usb_ccb;
 #ifdef CONFIG_BLK
@@ -1153,14 +1153,14 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,
 		/* XXX need some comment here */
 		retry = 2;
 		srb->pdata = (unsigned char *)buf_addr;
-		if (blks > USB_MAX_XFER_BLK)
-			smallblks = USB_MAX_XFER_BLK;
+		if (blks > usb_read_xfer_blk)
+			smallblks = usb_read_xfer_blk;
 		else
 			smallblks = (unsigned short) blks;
 retry_it:
 		debug("usb_read: retry #%d, xfer_blk %hu, smallblks %hu\n",
-		      retry, USB_MAX_XFER_BLK, smallblks);
-		if (smallblks == USB_MAX_XFER_BLK)
+		      retry, usb_read_xfer_blk, smallblks);
+		if (smallblks == usb_read_xfer_blk)
 			usb_show_progress();
 		srb->datalen = block_dev->blksz * smallblks;
 		srb->pdata = (unsigned char *)buf_addr;
@@ -1168,10 +1168,10 @@ retry_it:
 			debug("Read ERROR\n");
 			usb_request_sense(srb, ss);
 			if (retry--) {
-				/* decrease the USB_MAX_XFER_BLK */
+				/* decrease the usb_read_xfer_blk */
 				if (next >  0) {
 					smallblks = usb_xfer_blk_tbl[--next];
-					USB_MAX_XFER_BLK = smallblks;
+					usb_read_xfer_blk = smallblks;
 				}
 				retry_flag = true;
 				goto retry_it;
@@ -1183,10 +1183,10 @@ retry_it:
 		blks -= smallblks;
 		buf_addr += srb->datalen;
 
-		/* try to increase the USB_MAX_XFER_BLK */
+		/* try to increase the usb_read_xfer_blk */
 		if (next < USB_XFER_BLK_TBL_SZ  - 1)
 			if (!retry_flag && usb_xfer_blk_tbl[next + 1] <= blks)
-				USB_MAX_XFER_BLK = usb_xfer_blk_tbl[++next];
+				usb_read_xfer_blk = usb_xfer_blk_tbl[++next];
 	} while (blks != 0);
 	ss->flags &= ~USB_READY;
 
@@ -1195,7 +1195,7 @@ retry_it:
 	      start, smallblks, buf_addr);
 
 	usb_disable_asynch(0); /* asynch transfer allowed */
-	if (blkcnt >= USB_MAX_XFER_BLK)
+	if (blkcnt >= usb_read_xfer_blk)
 		debug("\n");
 	return blkcnt;
 }
@@ -1213,7 +1213,7 @@ static unsigned long usb_stor_write(struct blk_desc *block_dev, lbaint_t blknr,
 	unsigned short smallblks;
 	struct usb_device *udev;
 	struct us_data *ss;
-	int retry, next = LOG2((USB_MAX_XFER_BLK + 1) / usb_xfer_blk_tbl[0]);
+	int retry, next = LOG2((usb_write_xfer_blk + 1) / usb_xfer_blk_tbl[0]);
 	bool retry_flag = false;
 	ccb *srb = &usb_ccb;
 #ifdef CONFIG_BLK
@@ -1254,14 +1254,14 @@ static unsigned long usb_stor_write(struct blk_desc *block_dev, lbaint_t blknr,
 		 */
 		retry = 2;
 		srb->pdata = (unsigned char *)buf_addr;
-		if (blks > USB_MAX_XFER_BLK)
-			smallblks = USB_MAX_XFER_BLK;
+		if (blks > usb_write_xfer_blk)
+			smallblks = usb_write_xfer_blk;
 		else
 			smallblks = (unsigned short) blks;
 retry_it:
 		debug("usb_write: retry #%d, xfer_blk %hu, smallblks %hu\n",
-		      retry, USB_MAX_XFER_BLK, smallblks);
-		if (smallblks == USB_MAX_XFER_BLK)
+		      retry, usb_write_xfer_blk, smallblks);
+		if (smallblks == usb_write_xfer_blk)
 			usb_show_progress();
 		srb->datalen = block_dev->blksz * smallblks;
 		srb->pdata = (unsigned char *)buf_addr;
@@ -1269,10 +1269,10 @@ retry_it:
 			debug("Write ERROR\n");
 			usb_request_sense(srb, ss);
 			if (retry--) {
-				/* decrease the USB_MAX_XFER_BLK */
+				/* decrease the usb_write_xfer_blk */
 				if (next >  0) {
 					smallblks = usb_xfer_blk_tbl[--next];
-					USB_MAX_XFER_BLK = smallblks;
+					usb_write_xfer_blk = smallblks;
 				}
 				retry_flag = true;
 				goto retry_it;
@@ -1284,10 +1284,10 @@ retry_it:
 		blks -= smallblks;
 		buf_addr += srb->datalen;
 
-		/* try to increase the USB_MAX_XFER_BLK */
+		/* try to increase the usb_write_xfer_blk */
 		if (next < USB_XFER_BLK_TBL_SZ  - 1)
 			if (!retry_flag && usb_xfer_blk_tbl[next + 1] <= blks)
-				USB_MAX_XFER_BLK = usb_xfer_blk_tbl[++next];
+				usb_write_xfer_blk = usb_xfer_blk_tbl[++next];
 	} while (blks != 0);
 	ss->flags &= ~USB_READY;
 
@@ -1295,7 +1295,7 @@ retry_it:
 	      PRIxPTR "\n", start, smallblks, buf_addr);
 
 	usb_disable_asynch(0); /* asynch transfer allowed */
-	if (blkcnt >= USB_MAX_XFER_BLK)
+	if (blkcnt >= usb_write_xfer_blk)
 		debug("\n");
 	return blkcnt;
 
@@ -1361,7 +1361,8 @@ int usb_storage_probe(struct usb_device *dev, unsigned int ifnum,
 	}
 
 	/* Initialize the current transfer blocks to minimum value */
-	USB_MAX_XFER_BLK = usb_xfer_blk_tbl[0];
+	usb_read_xfer_blk = usb_xfer_blk_tbl[0];
+	usb_write_xfer_blk = usb_xfer_blk_tbl[0];
 
 	/*
 	 * We are expecting a minimum of 2 endpoints - in and out (bulk).
-- 
1.7.9.5

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

[Marek Vasut]

On 05/30/2016 01:23 PM, Rajesh Bhagat wrote:
> 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 adds an array of 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 andmax = 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>
> ---
>  common/usb_storage.c |   54 ++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 43 insertions(+), 11 deletions(-)
> 
> diff --git a/common/usb_storage.c b/common/usb_storage.c
> index 7e6e52d..7b5ad07 100644
> --- a/common/usb_storage.c
> +++ b/common/usb_storage.c
> @@ -101,16 +101,15 @@ struct us_data {
>  };
>  
>  #ifdef CONFIG_USB_EHCI
> -/*
> - * The U-Boot EHCI driver can handle any transfer length as long as there is
> - * enough free heap space left, but the SCSI READ(10) and WRITE(10) commands are
> - * limited to 65535 blocks.
> - */
> -#define USB_MAX_XFER_BLK	65535
> +#define USB_XFER_BLK_TBL_SZ		5
> +static unsigned short usb_xfer_blk_tbl[5] = {4096, 8192, 16384, 32768, 65535};

You should stick to using one block less than the power of two, the
controllers react to that a bit better. Each value in this table can
then be calculated really trivially, it's:

(1 << (12 + n)) - 1

You also don't need such static data.

>  #else
> -#define USB_MAX_XFER_BLK	20
> +#define USB_XFER_BLK_TBL_SZ		1
> +static unsigned short usb_xfer_blk_tbl[1] = {20};
>  #endif
>  
> +static unsigned short USB_MAX_XFER_BLK;

This value is different on per-device basis, how can it be static data ?

>  #ifndef CONFIG_BLK
>  static struct us_data usb_stor[USB_MAX_STOR_DEV];
>  #endif
> @@ -1117,7 +1116,8 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,
>  	unsigned short smallblks;
>  	struct usb_device *udev;
>  	struct us_data *ss;
> -	int retry;
> +	int retry, next = LOG2((USB_MAX_XFER_BLK + 1) / usb_xfer_blk_tbl[0]);
> +	bool retry_flag = false;
>  	ccb *srb = &usb_ccb;
>  #ifdef CONFIG_BLK
>  	struct blk_desc *block_dev;
> @@ -1158,6 +1158,8 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,
>  		else
>  			smallblks = (unsigned short) blks;
>  retry_it:
> +		debug("usb_read: retry #%d, xfer_blk %hu, smallblks %hu\n",
> +		      retry, USB_MAX_XFER_BLK, smallblks);
>  		if (smallblks == USB_MAX_XFER_BLK)
>  			usb_show_progress();
>  		srb->datalen = block_dev->blksz * smallblks;
> @@ -1165,14 +1167,26 @@ retry_it:
>  		if (usb_read_10(srb, ss, start, smallblks)) {
>  			debug("Read ERROR\n");
>  			usb_request_sense(srb, ss);
> -			if (retry--)
> +			if (retry--) {
> +				/* decrease the USB_MAX_XFER_BLK */
> +				if (next >  0) {
> +					smallblks = usb_xfer_blk_tbl[--next];
> +					USB_MAX_XFER_BLK = smallblks;
> +				}
> +				retry_flag = true;
>  				goto retry_it;
> +			}
>  			blkcnt -= blks;
>  			break;
>  		}
>  		start += smallblks;
>  		blks -= smallblks;
>  		buf_addr += srb->datalen;
> +
> +		/* try to increase the USB_MAX_XFER_BLK */
> +		if (next < USB_XFER_BLK_TBL_SZ  - 1)
> +			if (!retry_flag && usb_xfer_blk_tbl[next + 1] <= blks)
> +				USB_MAX_XFER_BLK = usb_xfer_blk_tbl[++next];
>  	} while (blks != 0);
>  	ss->flags &= ~USB_READY;
>  
> @@ -1199,7 +1213,8 @@ static unsigned long usb_stor_write(struct blk_desc *block_dev, lbaint_t blknr,
>  	unsigned short smallblks;
>  	struct usb_device *udev;
>  	struct us_data *ss;
> -	int retry;
> +	int retry, next = LOG2((USB_MAX_XFER_BLK + 1) / usb_xfer_blk_tbl[0]);
> +	bool retry_flag = false;
>  	ccb *srb = &usb_ccb;
>  #ifdef CONFIG_BLK
>  	struct blk_desc *block_dev;
> @@ -1244,6 +1259,8 @@ static unsigned long usb_stor_write(struct blk_desc *block_dev, lbaint_t blknr,
>  		else
>  			smallblks = (unsigned short) blks;
>  retry_it:
> +		debug("usb_write: retry #%d, xfer_blk %hu, smallblks %hu\n",
> +		      retry, USB_MAX_XFER_BLK, smallblks);
>  		if (smallblks == USB_MAX_XFER_BLK)
>  			usb_show_progress();
>  		srb->datalen = block_dev->blksz * smallblks;
> @@ -1251,14 +1268,26 @@ retry_it:
>  		if (usb_write_10(srb, ss, start, smallblks)) {
>  			debug("Write ERROR\n");
>  			usb_request_sense(srb, ss);
> -			if (retry--)
> +			if (retry--) {
> +				/* decrease the USB_MAX_XFER_BLK */
> +				if (next >  0) {
> +					smallblks = usb_xfer_blk_tbl[--next];
> +					USB_MAX_XFER_BLK = smallblks;
> +				}
> +				retry_flag = true;
>  				goto retry_it;
> +			}
>  			blkcnt -= blks;
>  			break;
>  		}
>  		start += smallblks;
>  		blks -= smallblks;
>  		buf_addr += srb->datalen;
> +
> +		/* try to increase the USB_MAX_XFER_BLK */
> +		if (next < USB_XFER_BLK_TBL_SZ  - 1)
> +			if (!retry_flag && usb_xfer_blk_tbl[next + 1] <= blks)
> +				USB_MAX_XFER_BLK = usb_xfer_blk_tbl[++next];
>  	} while (blks != 0);
>  	ss->flags &= ~USB_READY;
>  
> @@ -1331,6 +1360,9 @@ int usb_storage_probe(struct usb_device *dev, unsigned int ifnum,
>  		break;
>  	}
>  
> +	/* Initialize the current transfer blocks to minimum value */
> +	USB_MAX_XFER_BLK = usb_xfer_blk_tbl[0];
> +
>  	/*
>  	 * We are expecting a minimum of 2 endpoints - in and out (bulk).
>  	 * An optional interrupt is OK (necessary for CBI protocol).
> 


-- 
Best regards,
Marek Vasut

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

[Rajesh Bhagat]

> -----Original Message-----
> From: Marek Vasut [mailto:marex at denx.de]
> Sent: Tuesday, May 31, 2016 5:12 AM
> 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 1/2] common: usb_storage : Implement logic to calculate
> optimal usb maximum trasfer blocks
> 
> On 05/30/2016 01:23 PM, Rajesh Bhagat wrote:
> > 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 adds an array of 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 andmax = 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>
> > ---
> >  common/usb_storage.c |   54
> ++++++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 43 insertions(+), 11 deletions(-)
> >
> > diff --git a/common/usb_storage.c b/common/usb_storage.c index
> > 7e6e52d..7b5ad07 100644
> > --- a/common/usb_storage.c
> > +++ b/common/usb_storage.c
> > @@ -101,16 +101,15 @@ struct us_data {  };
> >
> >  #ifdef CONFIG_USB_EHCI
> > -/*
> > - * The U-Boot EHCI driver can handle any transfer length as long as
> > there is
> > - * enough free heap space left, but the SCSI READ(10) and WRITE(10)
> > commands are
> > - * limited to 65535 blocks.
> > - */
> > -#define USB_MAX_XFER_BLK	65535
> > +#define USB_XFER_BLK_TBL_SZ		5
> > +static unsigned short usb_xfer_blk_tbl[5] = {4096, 8192, 16384,
> > +32768, 65535};
> 
> You should stick to using one block less than the power of two, the controllers react to
> that a bit better. 

Hello Marek,

I agree to above point, Will change the logic to use one block less than power of two
i.e. 4095, 8191... 65535

> Each value in this table can then be calculated really trivially, it's:
> 
> (1 << (12 + n)) - 1
> 

Prior to current implementation, I created USB_MIN_XFER_BLK=4096 and 
USB_MAX_XFER_BLK=65535 macros. And started the logic to multiple and divide 
by two to reach USB_MAX_XFER_BLK. Then thought, calculation would be costlier 
than indexing an array. 

Please comment. 

> You also don't need such static data.
> 

Let me share the background for keeping this variable, It is taken to store the 
last optimal value for next read/write operation. And these value is reset to minimum
when another USB device is connected.

Refer below log: 

1st time (Iterated over all the possible size)
=> mw 81000000 55555555 4000000; mw a0000000 aaaaaaaa 4000000; usb write a0000000 0 80000; usb read 81000000 0 80000; cmp.b a0000000 81000000 10000000;

USB write: device 0 block # 0, count 524288 ... usb_read: retry #2, xfer_blk 4096, smallblks 4096
usb_read: retry #2, xfer_blk 8192, smallblks 8192
usb_read: retry #2, xfer_blk 16384, smallblks 16384
usb_read: retry #2, xfer_blk 32768, smallblks 32768
usb_read: retry #2, xfer_blk 65535, smallblks 65535
usb_read: retry #2, xfer_blk 65535, smallblks 65535
usb_read: retry #2, xfer_blk 65535, smallblks 65535
usb_read: retry #2, xfer_blk 65535, smallblks 65535
usb_read: retry #2, xfer_blk 65535, smallblks 65535
usb_read: retry #2, xfer_blk 65535, smallblks 65535
usb_read: retry #2, xfer_blk 65535, smallblks 65535
usb_read: retry #2, xfer_blk 65535, smallblks 4103
524288 blocks write: OK

2nd time (It started form 65535 blocks as it is last optimal value)
=> mw 81000000 55555555 4000000; mw a0000000 aaaaaaaa 4000000; usb write a0000000 0 80000; usb read 81000000 0 80000; cmp.b a0000000 81000000 10000000;

USB write: device 0 block # 0, count 524288 ... usb_read: retry #2, xfer_blk 65535, smallblks 65535
usb_read: retry #2, xfer_blk 65535, smallblks 65535
usb_read: retry #2, xfer_blk 65535, smallblks 65535
usb_read: retry #2, xfer_blk 65535, smallblks 65535
usb_read: retry #2, xfer_blk 65535, smallblks 65535
usb_read: retry #2, xfer_blk 65535, smallblks 65535
usb_read: retry #2, xfer_blk 65535, smallblks 65535
usb_read: retry #2, xfer_blk 65535, smallblks 65535
usb_read: retry #2, xfer_blk 65535, smallblks 8
524288 blocks write: OK

And I observed this value is different for read/write, Hence second patch is sent for handle 
that situation. 

Best Regards,
Rajesh Bhagat 

> >  #else
> > -#define USB_MAX_XFER_BLK	20
> > +#define USB_XFER_BLK_TBL_SZ		1
> > +static unsigned short usb_xfer_blk_tbl[1] = {20};
> >  #endif
> >
> > +static unsigned short USB_MAX_XFER_BLK;
> 
> This value is different on per-device basis, how can it be static data ?
> 
> >  #ifndef CONFIG_BLK
> >  static struct us_data usb_stor[USB_MAX_STOR_DEV];  #endif @@ -1117,7
> > +1116,8 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev,
> lbaint_t blknr,
> >  	unsigned short smallblks;
> >  	struct usb_device *udev;
> >  	struct us_data *ss;
> > -	int retry;
> > +	int retry, next = LOG2((USB_MAX_XFER_BLK + 1) / usb_xfer_blk_tbl[0]);
> > +	bool retry_flag = false;
> >  	ccb *srb = &usb_ccb;
> >  #ifdef CONFIG_BLK
> >  	struct blk_desc *block_dev;
> > @@ -1158,6 +1158,8 @@ static unsigned long usb_stor_read(struct blk_desc
> *block_dev, lbaint_t blknr,
> >  		else
> >  			smallblks = (unsigned short) blks;
> >  retry_it:
> > +		debug("usb_read: retry #%d, xfer_blk %hu, smallblks %hu\n",
> > +		      retry, USB_MAX_XFER_BLK, smallblks);
> >  		if (smallblks == USB_MAX_XFER_BLK)
> >  			usb_show_progress();
> >  		srb->datalen = block_dev->blksz * smallblks; @@ -1165,14 +1167,26
> > @@ retry_it:
> >  		if (usb_read_10(srb, ss, start, smallblks)) {
> >  			debug("Read ERROR\n");
> >  			usb_request_sense(srb, ss);
> > -			if (retry--)
> > +			if (retry--) {
> > +				/* decrease the USB_MAX_XFER_BLK */
> > +				if (next >  0) {
> > +					smallblks = usb_xfer_blk_tbl[--next];
> > +					USB_MAX_XFER_BLK = smallblks;
> > +				}
> > +				retry_flag = true;
> >  				goto retry_it;
> > +			}
> >  			blkcnt -= blks;
> >  			break;
> >  		}
> >  		start += smallblks;
> >  		blks -= smallblks;
> >  		buf_addr += srb->datalen;
> > +
> > +		/* try to increase the USB_MAX_XFER_BLK */
> > +		if (next < USB_XFER_BLK_TBL_SZ  - 1)
> > +			if (!retry_flag && usb_xfer_blk_tbl[next + 1] <= blks)
> > +				USB_MAX_XFER_BLK = usb_xfer_blk_tbl[++next];
> >  	} while (blks != 0);
> >  	ss->flags &= ~USB_READY;
> >
> > @@ -1199,7 +1213,8 @@ static unsigned long usb_stor_write(struct blk_desc
> *block_dev, lbaint_t blknr,
> >  	unsigned short smallblks;
> >  	struct usb_device *udev;
> >  	struct us_data *ss;
> > -	int retry;
> > +	int retry, next = LOG2((USB_MAX_XFER_BLK + 1) / usb_xfer_blk_tbl[0]);
> > +	bool retry_flag = false;
> >  	ccb *srb = &usb_ccb;
> >  #ifdef CONFIG_BLK
> >  	struct blk_desc *block_dev;
> > @@ -1244,6 +1259,8 @@ static unsigned long usb_stor_write(struct blk_desc
> *block_dev, lbaint_t blknr,
> >  		else
> >  			smallblks = (unsigned short) blks;
> >  retry_it:
> > +		debug("usb_write: retry #%d, xfer_blk %hu, smallblks %hu\n",
> > +		      retry, USB_MAX_XFER_BLK, smallblks);
> >  		if (smallblks == USB_MAX_XFER_BLK)
> >  			usb_show_progress();
> >  		srb->datalen = block_dev->blksz * smallblks; @@ -1251,14 +1268,26
> > @@ retry_it:
> >  		if (usb_write_10(srb, ss, start, smallblks)) {
> >  			debug("Write ERROR\n");
> >  			usb_request_sense(srb, ss);
> > -			if (retry--)
> > +			if (retry--) {
> > +				/* decrease the USB_MAX_XFER_BLK */
> > +				if (next >  0) {
> > +					smallblks = usb_xfer_blk_tbl[--next];
> > +					USB_MAX_XFER_BLK = smallblks;
> > +				}
> > +				retry_flag = true;
> >  				goto retry_it;
> > +			}
> >  			blkcnt -= blks;
> >  			break;
> >  		}
> >  		start += smallblks;
> >  		blks -= smallblks;
> >  		buf_addr += srb->datalen;
> > +
> > +		/* try to increase the USB_MAX_XFER_BLK */
> > +		if (next < USB_XFER_BLK_TBL_SZ  - 1)
> > +			if (!retry_flag && usb_xfer_blk_tbl[next + 1] <= blks)
> > +				USB_MAX_XFER_BLK = usb_xfer_blk_tbl[++next];
> >  	} while (blks != 0);
> >  	ss->flags &= ~USB_READY;
> >
> > @@ -1331,6 +1360,9 @@ int usb_storage_probe(struct usb_device *dev, unsigned
> int ifnum,
> >  		break;
> >  	}
> >
> > +	/* Initialize the current transfer blocks to minimum value */
> > +	USB_MAX_XFER_BLK = usb_xfer_blk_tbl[0];
> > +
> >  	/*
> >  	 * We are expecting a minimum of 2 endpoints - in and out (bulk).
> >  	 * An optional interrupt is OK (necessary for CBI protocol).
> >
> 
> 
> --
> Best regards,
> Marek Vasut

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

[Marek Vasut]

On 05/31/2016 05:23 AM, Rajesh Bhagat wrote:
> 
> 
>> -----Original Message-----
>> From: Marek Vasut [mailto:marex at denx.de]
>> Sent: Tuesday, May 31, 2016 5:12 AM
>> 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 1/2] common: usb_storage : Implement logic to calculate
>> optimal usb maximum trasfer blocks
>>
>> On 05/30/2016 01:23 PM, Rajesh Bhagat wrote:
>>> 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 adds an array of 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 andmax = 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>
>>> ---
>>>  common/usb_storage.c |   54
>> ++++++++++++++++++++++++++++++++++++++++----------
>>>  1 file changed, 43 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/common/usb_storage.c b/common/usb_storage.c index
>>> 7e6e52d..7b5ad07 100644
>>> --- a/common/usb_storage.c
>>> +++ b/common/usb_storage.c
>>> @@ -101,16 +101,15 @@ struct us_data {  };
>>>
>>>  #ifdef CONFIG_USB_EHCI
>>> -/*
>>> - * The U-Boot EHCI driver can handle any transfer length as long as
>>> there is
>>> - * enough free heap space left, but the SCSI READ(10) and WRITE(10)
>>> commands are
>>> - * limited to 65535 blocks.
>>> - */
>>> -#define USB_MAX_XFER_BLK	65535
>>> +#define USB_XFER_BLK_TBL_SZ		5
>>> +static unsigned short usb_xfer_blk_tbl[5] = {4096, 8192, 16384,
>>> +32768, 65535};
>>
>> You should stick to using one block less than the power of two, the controllers react to
>> that a bit better. 
> 
> Hello Marek,
> 
> I agree to above point, Will change the logic to use one block less than power of two
> i.e. 4095, 8191... 65535
> 
>> Each value in this table can then be calculated really trivially, it's:
>>
>> (1 << (12 + n)) - 1
>>
> 
> Prior to current implementation, I created USB_MIN_XFER_BLK=4096 and 
> USB_MAX_XFER_BLK=65535 macros. And started the logic to multiple and divide 
> by two to reach USB_MAX_XFER_BLK. Then thought, calculation would be costlier 
> than indexing an array. 

Since the numbers are 16bit tops, they will most likely end up being
optimized to mov rN, #imm16 or mov rN, imm16 + sub rN, #1 . That's two
local instructions tops, which will always be at least as fast as any
memory fetch.

> Please comment. 
> 
>> You also don't need such static data.
>>
> 
> Let me share the background for keeping this variable, It is taken to store the 
> last optimal value for next read/write operation. And these value is reset to minimum
> when another USB device is connected.

Consider the scenario where you have two sticks plugged in and mix
reading from both. Your approach with static variable will fail miserably.

> Refer below log: 
> 
> 1st time (Iterated over all the possible size)
> => mw 81000000 55555555 4000000; mw a0000000 aaaaaaaa 4000000; usb write a0000000 0 80000; usb read 81000000 0 80000; cmp.b a0000000 81000000 10000000;
> 
> USB write: device 0 block # 0, count 524288 ... usb_read: retry #2, xfer_blk 4096, smallblks 4096
> usb_read: retry #2, xfer_blk 8192, smallblks 8192
> usb_read: retry #2, xfer_blk 16384, smallblks 16384
> usb_read: retry #2, xfer_blk 32768, smallblks 32768
> usb_read: retry #2, xfer_blk 65535, smallblks 65535
> usb_read: retry #2, xfer_blk 65535, smallblks 65535
> usb_read: retry #2, xfer_blk 65535, smallblks 65535
> usb_read: retry #2, xfer_blk 65535, smallblks 65535
> usb_read: retry #2, xfer_blk 65535, smallblks 65535
> usb_read: retry #2, xfer_blk 65535, smallblks 65535
> usb_read: retry #2, xfer_blk 65535, smallblks 65535
> usb_read: retry #2, xfer_blk 65535, smallblks 4103
> 524288 blocks write: OK
> 
> 2nd time (It started form 65535 blocks as it is last optimal value)
> => mw 81000000 55555555 4000000; mw a0000000 aaaaaaaa 4000000; usb write a0000000 0 80000; usb read 81000000 0 80000; cmp.b a0000000 81000000 10000000;
> 
> USB write: device 0 block # 0, count 524288 ... usb_read: retry #2, xfer_blk 65535, smallblks 65535
> usb_read: retry #2, xfer_blk 65535, smallblks 65535
> usb_read: retry #2, xfer_blk 65535, smallblks 65535
> usb_read: retry #2, xfer_blk 65535, smallblks 65535
> usb_read: retry #2, xfer_blk 65535, smallblks 65535
> usb_read: retry #2, xfer_blk 65535, smallblks 65535
> usb_read: retry #2, xfer_blk 65535, smallblks 65535
> usb_read: retry #2, xfer_blk 65535, smallblks 65535
> usb_read: retry #2, xfer_blk 65535, smallblks 8
> 524288 blocks write: OK
> 
> And I observed this value is different for read/write, Hence second patch is sent for handle 
> that situation. 
> 
> Best Regards,
> Rajesh Bhagat 
> 
>>>  #else
>>> -#define USB_MAX_XFER_BLK	20
>>> +#define USB_XFER_BLK_TBL_SZ		1
>>> +static unsigned short usb_xfer_blk_tbl[1] = {20};
>>>  #endif
>>>
>>> +static unsigned short USB_MAX_XFER_BLK;
>>
>> This value is different on per-device basis, how can it be static data ?
>>
>>>  #ifndef CONFIG_BLK
>>>  static struct us_data usb_stor[USB_MAX_STOR_DEV];  #endif @@ -1117,7
>>> +1116,8 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev,
>> lbaint_t blknr,
>>>  	unsigned short smallblks;
>>>  	struct usb_device *udev;
>>>  	struct us_data *ss;
>>> -	int retry;
>>> +	int retry, next = LOG2((USB_MAX_XFER_BLK + 1) / usb_xfer_blk_tbl[0]);
>>> +	bool retry_flag = false;
>>>  	ccb *srb = &usb_ccb;
>>>  #ifdef CONFIG_BLK
>>>  	struct blk_desc *block_dev;
>>> @@ -1158,6 +1158,8 @@ static unsigned long usb_stor_read(struct blk_desc
>> *block_dev, lbaint_t blknr,
>>>  		else
>>>  			smallblks = (unsigned short) blks;
>>>  retry_it:
>>> +		debug("usb_read: retry #%d, xfer_blk %hu, smallblks %hu\n",
>>> +		      retry, USB_MAX_XFER_BLK, smallblks);
>>>  		if (smallblks == USB_MAX_XFER_BLK)
>>>  			usb_show_progress();
>>>  		srb->datalen = block_dev->blksz * smallblks; @@ -1165,14 +1167,26
>>> @@ retry_it:
>>>  		if (usb_read_10(srb, ss, start, smallblks)) {
>>>  			debug("Read ERROR\n");
>>>  			usb_request_sense(srb, ss);
>>> -			if (retry--)
>>> +			if (retry--) {
>>> +				/* decrease the USB_MAX_XFER_BLK */
>>> +				if (next >  0) {
>>> +					smallblks = usb_xfer_blk_tbl[--next];
>>> +					USB_MAX_XFER_BLK = smallblks;
>>> +				}
>>> +				retry_flag = true;
>>>  				goto retry_it;
>>> +			}
>>>  			blkcnt -= blks;
>>>  			break;
>>>  		}
>>>  		start += smallblks;
>>>  		blks -= smallblks;
>>>  		buf_addr += srb->datalen;
>>> +
>>> +		/* try to increase the USB_MAX_XFER_BLK */
>>> +		if (next < USB_XFER_BLK_TBL_SZ  - 1)
>>> +			if (!retry_flag && usb_xfer_blk_tbl[next + 1] <= blks)
>>> +				USB_MAX_XFER_BLK = usb_xfer_blk_tbl[++next];
>>>  	} while (blks != 0);
>>>  	ss->flags &= ~USB_READY;
>>>
>>> @@ -1199,7 +1213,8 @@ static unsigned long usb_stor_write(struct blk_desc
>> *block_dev, lbaint_t blknr,
>>>  	unsigned short smallblks;
>>>  	struct usb_device *udev;
>>>  	struct us_data *ss;
>>> -	int retry;
>>> +	int retry, next = LOG2((USB_MAX_XFER_BLK + 1) / usb_xfer_blk_tbl[0]);
>>> +	bool retry_flag = false;
>>>  	ccb *srb = &usb_ccb;
>>>  #ifdef CONFIG_BLK
>>>  	struct blk_desc *block_dev;
>>> @@ -1244,6 +1259,8 @@ static unsigned long usb_stor_write(struct blk_desc
>> *block_dev, lbaint_t blknr,
>>>  		else
>>>  			smallblks = (unsigned short) blks;
>>>  retry_it:
>>> +		debug("usb_write: retry #%d, xfer_blk %hu, smallblks %hu\n",
>>> +		      retry, USB_MAX_XFER_BLK, smallblks);
>>>  		if (smallblks == USB_MAX_XFER_BLK)
>>>  			usb_show_progress();
>>>  		srb->datalen = block_dev->blksz * smallblks; @@ -1251,14 +1268,26
>>> @@ retry_it:
>>>  		if (usb_write_10(srb, ss, start, smallblks)) {
>>>  			debug("Write ERROR\n");
>>>  			usb_request_sense(srb, ss);
>>> -			if (retry--)
>>> +			if (retry--) {
>>> +				/* decrease the USB_MAX_XFER_BLK */
>>> +				if (next >  0) {
>>> +					smallblks = usb_xfer_blk_tbl[--next];
>>> +					USB_MAX_XFER_BLK = smallblks;
>>> +				}
>>> +				retry_flag = true;
>>>  				goto retry_it;
>>> +			}
>>>  			blkcnt -= blks;
>>>  			break;
>>>  		}
>>>  		start += smallblks;
>>>  		blks -= smallblks;
>>>  		buf_addr += srb->datalen;
>>> +
>>> +		/* try to increase the USB_MAX_XFER_BLK */
>>> +		if (next < USB_XFER_BLK_TBL_SZ  - 1)
>>> +			if (!retry_flag && usb_xfer_blk_tbl[next + 1] <= blks)
>>> +				USB_MAX_XFER_BLK = usb_xfer_blk_tbl[++next];
>>>  	} while (blks != 0);
>>>  	ss->flags &= ~USB_READY;
>>>
>>> @@ -1331,6 +1360,9 @@ int usb_storage_probe(struct usb_device *dev, unsigned
>> int ifnum,
>>>  		break;
>>>  	}
>>>
>>> +	/* Initialize the current transfer blocks to minimum value */
>>> +	USB_MAX_XFER_BLK = usb_xfer_blk_tbl[0];
>>> +
>>>  	/*
>>>  	 * We are expecting a minimum of 2 endpoints - in and out (bulk).
>>>  	 * An optional interrupt is OK (necessary for CBI protocol).
>>>
>>
>>
>> --
>> Best regards,
>> Marek Vasut


-- 
Best regards,
Marek Vasut

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

[Rajesh Bhagat]

> -----Original Message-----
> From: Marek Vasut [mailto:marex at denx.de]
> Sent: Tuesday, May 31, 2016 5:27 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 1/2] common: usb_storage : Implement logic to calculate
> optimal usb maximum trasfer blocks
> 
> On 05/31/2016 05:23 AM, Rajesh Bhagat wrote:
> >
> >
> >> -----Original Message-----
> >> From: Marek Vasut [mailto:marex at denx.de]
> >> Sent: Tuesday, May 31, 2016 5:12 AM
> >> 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 1/2] common: usb_storage : Implement logic to
> >> calculate optimal usb maximum trasfer blocks
> >>
> >> On 05/30/2016 01:23 PM, Rajesh Bhagat wrote:
> >>> 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 adds an array of 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 andmax = 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>
> >>> ---
> >>>  common/usb_storage.c |   54
> >> ++++++++++++++++++++++++++++++++++++++++----------
> >>>  1 file changed, 43 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/common/usb_storage.c b/common/usb_storage.c index
> >>> 7e6e52d..7b5ad07 100644
> >>> --- a/common/usb_storage.c
> >>> +++ b/common/usb_storage.c
> >>> @@ -101,16 +101,15 @@ struct us_data {  };
> >>>
> >>>  #ifdef CONFIG_USB_EHCI
> >>> -/*
> >>> - * The U-Boot EHCI driver can handle any transfer length as long as
> >>> there is
> >>> - * enough free heap space left, but the SCSI READ(10) and WRITE(10)
> >>> commands are
> >>> - * limited to 65535 blocks.
> >>> - */
> >>> -#define USB_MAX_XFER_BLK	65535
> >>> +#define USB_XFER_BLK_TBL_SZ		5
> >>> +static unsigned short usb_xfer_blk_tbl[5] = {4096, 8192, 16384,
> >>> +32768, 65535};
> >>
> >> You should stick to using one block less than the power of two, the
> >> controllers react to that a bit better.
> >
> > Hello Marek,
> >
> > I agree to above point, Will change the logic to use one block less
> > than power of two i.e. 4095, 8191... 65535
> >
> >> Each value in this table can then be calculated really trivially, it's:
> >>
> >> (1 << (12 + n)) - 1
> >>
> >
> > Prior to current implementation, I created USB_MIN_XFER_BLK=4096 and
> > USB_MAX_XFER_BLK=65535 macros. And started the logic to multiple and
> > divide by two to reach USB_MAX_XFER_BLK. Then thought, calculation
> > would be costlier than indexing an array.

Hello Marek, 

> 
> Since the numbers are 16bit tops, they will most likely end up being optimized to mov
> rN, #imm16 or mov rN, imm16 + sub rN, #1 . That's two local instructions tops,
> which will always be at least as fast as any memory fetch.
> 

Ok, I agree to your point. Will change the logic to use above formula as mentioned
by you in v2.

> > Please comment.
> >
> >> You also don't need such static data.
> >>
> >
> > Let me share the background for keeping this variable, It is taken to
> > store the last optimal value for next read/write operation. And these
> > value is reset to minimum when another USB device is connected.
> 
> Consider the scenario where you have two sticks plugged in and mix reading from
> both. Your approach with static variable will fail miserably.
> 

You are right, I missed it completely. Above logic would have been possible if at time 
of changing the current device "usb dev ..", I reset the value to minimum again as we
I have done when new device is connected. But "usb dev .." doesn?t seem to mapped 
to common/usb_storage.c code. Hence, it is difficult to retain value keeping a static 
variable. 

I will change the logic to start from minimum and try to go to maximum in every read/write
in v2.

Best Regards,
Rajesh Bhagat 

> > Refer below log:
> >
> > 1st time (Iterated over all the possible size) => mw 81000000 55555555
> > 4000000; mw a0000000 aaaaaaaa 4000000; usb write a0000000 0 80000; usb
> > read 81000000 0 80000; cmp.b a0000000 81000000 10000000;
> >
> > USB write: device 0 block # 0, count 524288 ... usb_read: retry #2,
> > xfer_blk 4096, smallblks 4096
> > usb_read: retry #2, xfer_blk 8192, smallblks 8192
> > usb_read: retry #2, xfer_blk 16384, smallblks 16384
> > usb_read: retry #2, xfer_blk 32768, smallblks 32768
> > usb_read: retry #2, xfer_blk 65535, smallblks 65535
> > usb_read: retry #2, xfer_blk 65535, smallblks 65535
> > usb_read: retry #2, xfer_blk 65535, smallblks 65535
> > usb_read: retry #2, xfer_blk 65535, smallblks 65535
> > usb_read: retry #2, xfer_blk 65535, smallblks 65535
> > usb_read: retry #2, xfer_blk 65535, smallblks 65535
> > usb_read: retry #2, xfer_blk 65535, smallblks 65535
> > usb_read: retry #2, xfer_blk 65535, smallblks 4103
> > 524288 blocks write: OK
> >
> > 2nd time (It started form 65535 blocks as it is last optimal value) =>
> > mw 81000000 55555555 4000000; mw a0000000 aaaaaaaa 4000000; usb write
> > a0000000 0 80000; usb read 81000000 0 80000; cmp.b a0000000 81000000
> > 10000000;
> >
> > USB write: device 0 block # 0, count 524288 ... usb_read: retry #2,
> > xfer_blk 65535, smallblks 65535
> > usb_read: retry #2, xfer_blk 65535, smallblks 65535
> > usb_read: retry #2, xfer_blk 65535, smallblks 65535
> > usb_read: retry #2, xfer_blk 65535, smallblks 65535
> > usb_read: retry #2, xfer_blk 65535, smallblks 65535
> > usb_read: retry #2, xfer_blk 65535, smallblks 65535
> > usb_read: retry #2, xfer_blk 65535, smallblks 65535
> > usb_read: retry #2, xfer_blk 65535, smallblks 65535
> > usb_read: retry #2, xfer_blk 65535, smallblks 8
> > 524288 blocks write: OK
> >
> > And I observed this value is different for read/write, Hence second
> > patch is sent for handle that situation.
> >
> > Best Regards,
> > Rajesh Bhagat
> >
> >>>  #else
> >>> -#define USB_MAX_XFER_BLK	20
> >>> +#define USB_XFER_BLK_TBL_SZ		1
> >>> +static unsigned short usb_xfer_blk_tbl[1] = {20};
> >>>  #endif
> >>>
> >>> +static unsigned short USB_MAX_XFER_BLK;
> >>
> >> This value is different on per-device basis, how can it be static data ?
> >>
> >>>  #ifndef CONFIG_BLK
> >>>  static struct us_data usb_stor[USB_MAX_STOR_DEV];  #endif @@
> >>> -1117,7
> >>> +1116,8 @@ static unsigned long usb_stor_read(struct blk_desc
> >>> +*block_dev,
> >> lbaint_t blknr,
> >>>  	unsigned short smallblks;
> >>>  	struct usb_device *udev;
> >>>  	struct us_data *ss;
> >>> -	int retry;
> >>> +	int retry, next = LOG2((USB_MAX_XFER_BLK + 1) /
> usb_xfer_blk_tbl[0]);
> >>> +	bool retry_flag = false;
> >>>  	ccb *srb = &usb_ccb;
> >>>  #ifdef CONFIG_BLK
> >>>  	struct blk_desc *block_dev;
> >>> @@ -1158,6 +1158,8 @@ static unsigned long usb_stor_read(struct
> >>> blk_desc
> >> *block_dev, lbaint_t blknr,
> >>>  		else
> >>>  			smallblks = (unsigned short) blks;
> >>>  retry_it:
> >>> +		debug("usb_read: retry #%d, xfer_blk %hu, smallblks %hu\n",
> >>> +		      retry, USB_MAX_XFER_BLK, smallblks);
> >>>  		if (smallblks == USB_MAX_XFER_BLK)
> >>>  			usb_show_progress();
> >>>  		srb->datalen = block_dev->blksz * smallblks; @@ -1165,14 +1167,26
> >>> @@ retry_it:
> >>>  		if (usb_read_10(srb, ss, start, smallblks)) {
> >>>  			debug("Read ERROR\n");
> >>>  			usb_request_sense(srb, ss);
> >>> -			if (retry--)
> >>> +			if (retry--) {
> >>> +				/* decrease the USB_MAX_XFER_BLK */
> >>> +				if (next >  0) {
> >>> +					smallblks = usb_xfer_blk_tbl[--next];
> >>> +					USB_MAX_XFER_BLK = smallblks;
> >>> +				}
> >>> +				retry_flag = true;
> >>>  				goto retry_it;
> >>> +			}
> >>>  			blkcnt -= blks;
> >>>  			break;
> >>>  		}
> >>>  		start += smallblks;
> >>>  		blks -= smallblks;
> >>>  		buf_addr += srb->datalen;
> >>> +
> >>> +		/* try to increase the USB_MAX_XFER_BLK */
> >>> +		if (next < USB_XFER_BLK_TBL_SZ  - 1)
> >>> +			if (!retry_flag && usb_xfer_blk_tbl[next + 1] <= blks)
> >>> +				USB_MAX_XFER_BLK = usb_xfer_blk_tbl[++next];
> >>>  	} while (blks != 0);
> >>>  	ss->flags &= ~USB_READY;
> >>>
> >>> @@ -1199,7 +1213,8 @@ static unsigned long usb_stor_write(struct
> >>> blk_desc
> >> *block_dev, lbaint_t blknr,
> >>>  	unsigned short smallblks;
> >>>  	struct usb_device *udev;
> >>>  	struct us_data *ss;
> >>> -	int retry;
> >>> +	int retry, next = LOG2((USB_MAX_XFER_BLK + 1) /
> usb_xfer_blk_tbl[0]);
> >>> +	bool retry_flag = false;
> >>>  	ccb *srb = &usb_ccb;
> >>>  #ifdef CONFIG_BLK
> >>>  	struct blk_desc *block_dev;
> >>> @@ -1244,6 +1259,8 @@ static unsigned long usb_stor_write(struct
> >>> blk_desc
> >> *block_dev, lbaint_t blknr,
> >>>  		else
> >>>  			smallblks = (unsigned short) blks;
> >>>  retry_it:
> >>> +		debug("usb_write: retry #%d, xfer_blk %hu, smallblks %hu\n",
> >>> +		      retry, USB_MAX_XFER_BLK, smallblks);
> >>>  		if (smallblks == USB_MAX_XFER_BLK)
> >>>  			usb_show_progress();
> >>>  		srb->datalen = block_dev->blksz * smallblks; @@ -1251,14 +1268,26
> >>> @@ retry_it:
> >>>  		if (usb_write_10(srb, ss, start, smallblks)) {
> >>>  			debug("Write ERROR\n");
> >>>  			usb_request_sense(srb, ss);
> >>> -			if (retry--)
> >>> +			if (retry--) {
> >>> +				/* decrease the USB_MAX_XFER_BLK */
> >>> +				if (next >  0) {
> >>> +					smallblks = usb_xfer_blk_tbl[--next];
> >>> +					USB_MAX_XFER_BLK = smallblks;
> >>> +				}
> >>> +				retry_flag = true;
> >>>  				goto retry_it;
> >>> +			}
> >>>  			blkcnt -= blks;
> >>>  			break;
> >>>  		}
> >>>  		start += smallblks;
> >>>  		blks -= smallblks;
> >>>  		buf_addr += srb->datalen;
> >>> +
> >>> +		/* try to increase the USB_MAX_XFER_BLK */
> >>> +		if (next < USB_XFER_BLK_TBL_SZ  - 1)
> >>> +			if (!retry_flag && usb_xfer_blk_tbl[next + 1] <= blks)
> >>> +				USB_MAX_XFER_BLK = usb_xfer_blk_tbl[++next];
> >>>  	} while (blks != 0);
> >>>  	ss->flags &= ~USB_READY;
> >>>
> >>> @@ -1331,6 +1360,9 @@ int usb_storage_probe(struct usb_device *dev,
> >>> unsigned
> >> int ifnum,
> >>>  		break;
> >>>  	}
> >>>
> >>> +	/* Initialize the current transfer blocks to minimum value */
> >>> +	USB_MAX_XFER_BLK = usb_xfer_blk_tbl[0];
> >>> +
> >>>  	/*
> >>>  	 * We are expecting a minimum of 2 endpoints - in and out (bulk).
> >>>  	 * An optional interrupt is OK (necessary for CBI protocol).
> >>>
> >>
> >>
> >> --
> >> Best regards,
> >> Marek Vasut
> 
> 
> --
> Best regards,
> Marek Vasut