-----Original Message----- From: Marek Vasut [mailto:marex@denx.de] Sent: Monday, June 13, 2016 7:07 PM To: Rajesh Bhagat rajesh.bhagat@nxp.com; u-boot@lists.denx.de Cc: sjg@chromium.org; york sun york.sun@nxp.com; Sriram Dash sriram.dash@nxp.com; Rajesh Bhagat rajesh.bhagat@freescale.com Subject: Re: [PATCH v5 2/2] common: usb_storage : Implement logic to calculate optimal usb maximum trasfer blocks
On 06/13/2016 06:03 AM, Rajesh Bhagat 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 v5:
- None
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 | 67
++++++++++++++++++++++++++++++++++++++++++++++---
include/usb.h | 1 + 2 files changed, 63 insertions(+), 5 deletions(-)
diff --git a/common/usb_storage.c b/common/usb_storage.c index f060637..9b09412 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)
Hello Marek,
Parenthesis around variables are missing.
Will take care in v6.
#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 pos == 0 , the condition below will be true (size will be 2047), so this extra ternary is unnecessary.
Will take care in v6. Will remove the extra ternary operator used, as 2047 will follow the below check (size < USB_MIN_XFER_BLK) and code will work fine.
- 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;Why don't you clamp the size to min(blks, USB_MAX_XFER_BLK) instead ?
Do you mean below statement?
if (size > min(blks, 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 @@ -1128,6 +1171,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; @@ -1167,26 +1211,36 @@ 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;retry_it:
if (smallblks == USB_MAX_XFER_BLK)
debug("usb_%s: retry #%d, cur_xfer_blks %hu, smallblks %hu\n",USB_STOR_OP_TYPE, retry, udev->cur_xfer_blks,smallblks);if (smallblks == udev->cur_xfer_blks) usb_show_progress();
if (retry--)
if (retry--) {if (!dec_cur_xfer_blks(udev))smallblks = udev->cur_xfer_blks;retry_flag = true; goto retry_it;} blkcnt -= blks; break;if (!retry_flag && !inc_cur_xfer_blks(udev, blks))smallblks = udev->cur_xfer_blks;@@ -1195,7 +1249,7 @@ retry_it: USB_STOR_OP_TYPE, 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;
} @@ -1282,6 +1336,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;
Best Regards, Rajesh Bhagat
-- Best regards, Marek Vasut