[U-Boot] [PATCH v6 0/2] common: usb_storage : Implement logic to calculate optimal
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 | 213 +++++++++++++++++++++++---------------------------- include/usb.h | 1 + 2 files changed, 98 insertions(+), 116 deletions(-)
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 v6: - Removes USB_STOR_OP_TYPE macro and adds __func__ in debug prints - Unifies usb_read_10/usb_write_10 functions to usb_read_write_10
Changes in v5: - Converts USB_STOR_OPERATION_FUNC macro to a function - Corrects the multi line comment accroding to coding style - Renames variable to dev to remove code duplication
Changes in v4: - Adds code to make common function for read/write
common/usb_storage.c | 147 +++++++++++++-------------------------------------- 1 file changed, 36 insertions(+), 111 deletions(-)
diff --git a/common/usb_storage.c b/common/usb_storage.c index 7e6e52d..41e5aa3 100644 --- a/common/usb_storage.c +++ b/common/usb_storage.c @@ -1046,11 +1046,11 @@ static int usb_read_capacity(ccb *srb, struct us_data *ss) return -1; }
-static int usb_read_10(ccb *srb, struct us_data *ss, unsigned long start, - unsigned short blocks) +static int usb_read_write_10(ccb *srb, struct us_data *ss, unsigned long start, + unsigned short blocks, bool is_write) { memset(&srb->cmd[0], 0, 12); - srb->cmd[0] = SCSI_READ10; + srb->cmd[0] = is_write ? SCSI_WRITE10 : SCSI_READ10; srb->cmd[1] = srb->lun << 5; srb->cmd[2] = ((unsigned char) (start >> 24)) & 0xff; srb->cmd[3] = ((unsigned char) (start >> 16)) & 0xff; @@ -1059,28 +1059,10 @@ static int usb_read_10(ccb *srb, struct us_data *ss, unsigned long start, srb->cmd[7] = ((unsigned char) (blocks >> 8)) & 0xff; srb->cmd[8] = (unsigned char) blocks & 0xff; srb->cmdlen = 12; - debug("read10: start %lx blocks %x\n", start, blocks); + debug("%s: start %lx blocks %x\n", __func__, start, blocks); return ss->transport(srb, ss); }
-static int usb_write_10(ccb *srb, struct us_data *ss, unsigned long start, - unsigned short blocks) -{ - memset(&srb->cmd[0], 0, 12); - srb->cmd[0] = SCSI_WRITE10; - srb->cmd[1] = srb->lun << 5; - srb->cmd[2] = ((unsigned char) (start >> 24)) & 0xff; - srb->cmd[3] = ((unsigned char) (start >> 16)) & 0xff; - srb->cmd[4] = ((unsigned char) (start >> 8)) & 0xff; - srb->cmd[5] = ((unsigned char) (start)) & 0xff; - srb->cmd[7] = ((unsigned char) (blocks >> 8)) & 0xff; - srb->cmd[8] = (unsigned char) blocks & 0xff; - srb->cmdlen = 12; - debug("write10: start %lx blocks %x\n", start, blocks); - return ss->transport(srb, ss); -} - - #ifdef CONFIG_USB_BIN_FIXUP /* * Some USB storage devices queried for SCSI identification data respond with @@ -1105,11 +1087,13 @@ static void usb_bin_fixup(struct usb_device_descriptor descriptor, #endif /* CONFIG_USB_BIN_FIXUP */
#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 +1113,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("\n%s: udev %d\n", __func__, block_dev->devnum); #else - debug("\nusb_read: udev %d\n", block_dev->devnum); + debug("\n%s: udev %d\n", __func__, block_dev->devnum); udev = usb_dev_desc[block_dev->devnum].priv; if (!udev) { debug("%s: No device\n", __func__); @@ -1146,11 +1130,15 @@ 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("\n%s: dev %d startblk " LBAF ", blccnt " LBAF " buffer %" + PRIxPTR "\n", __func__, 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 +1150,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_read_write_10(srb, ss, start, smallblks, is_write)) { + debug("%s ERROR\n", __func__); usb_request_sense(srb, ss); if (retry--) goto retry_it; @@ -1176,9 +1164,9 @@ retry_it: } while (blks != 0); ss->flags &= ~USB_READY;
- debug("usb_read: end startblk " LBAF + debug("%s: end startblk " LBAF ", blccnt %x buffer %" PRIxPTR "\n", - start, smallblks, buf_addr); + __func__, start, smallblks, buf_addr);
usb_disable_asynch(0); /* asynch transfer allowed */ if (blkcnt >= USB_MAX_XFER_BLK) @@ -1186,90 +1174,27 @@ retry_it: return blkcnt; }
+ #ifdef CONFIG_BLK -static unsigned long usb_stor_write(struct udevice *dev, lbaint_t blknr, - lbaint_t blkcnt, const void *buffer) +static unsigned long usb_stor_read(struct udevice *dev, lbaint_t blknr, + lbaint_t blkcnt, void *buffer) #else -static unsigned long usb_stor_write(struct blk_desc *block_dev, lbaint_t blknr, - lbaint_t blkcnt, const void *buffer) +static unsigned long usb_stor_read(struct blk_desc *dev, lbaint_t blknr, + lbaint_t blkcnt, 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; -#ifdef CONFIG_BLK - struct blk_desc *block_dev; -#endif - - if (blkcnt == 0) - return 0; + return usb_stor_read_write(dev, blknr, blkcnt, buffer, false); +}
- /* 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); +static unsigned long usb_stor_write(struct udevice *dev, lbaint_t blknr, + lbaint_t blkcnt, const void *buffer) #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; - } +static unsigned long usb_stor_write(struct blk_desc *dev, lbaint_t blknr, + lbaint_t blkcnt, const void *buffer) #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; - +{ + return usb_stor_read_write(dev, blknr, blkcnt, buffer, true); }
/* Probe to see if a new device is actually a Storage device */
On 06/15/2016 09:00 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 v6:
- Removes USB_STOR_OP_TYPE macro and adds __func__ in debug prints
- Unifies usb_read_10/usb_write_10 functions to usb_read_write_10
Changes in v5:
- Converts USB_STOR_OPERATION_FUNC macro to a function
- Corrects the multi line comment accroding to coding style
- Renames variable to dev to remove code duplication
Changes in v4:
- Adds code to make common function for read/write
common/usb_storage.c | 147 +++++++++++++-------------------------------------- 1 file changed, 36 insertions(+), 111 deletions(-)
diff --git a/common/usb_storage.c b/common/usb_storage.c index 7e6e52d..41e5aa3 100644 --- a/common/usb_storage.c +++ b/common/usb_storage.c @@ -1046,11 +1046,11 @@ static int usb_read_capacity(ccb *srb, struct us_data *ss) return -1; }
-static int usb_read_10(ccb *srb, struct us_data *ss, unsigned long start,
unsigned short blocks)+static int usb_read_write_10(ccb *srb, struct us_data *ss, unsigned long start,
unsigned short blocks, bool is_write){ memset(&srb->cmd[0], 0, 12);
- srb->cmd[0] = SCSI_READ10;
- srb->cmd[0] = is_write ? SCSI_WRITE10 : SCSI_READ10; srb->cmd[1] = srb->lun << 5; srb->cmd[2] = ((unsigned char) (start >> 24)) & 0xff; srb->cmd[3] = ((unsigned char) (start >> 16)) & 0xff;
@@ -1059,28 +1059,10 @@ static int usb_read_10(ccb *srb, struct us_data *ss, unsigned long start, srb->cmd[7] = ((unsigned char) (blocks >> 8)) & 0xff; srb->cmd[8] = (unsigned char) blocks & 0xff; srb->cmdlen = 12;
- debug("read10: start %lx blocks %x\n", start, blocks);
- debug("%s: start %lx blocks %x\n", __func__, start, blocks); return ss->transport(srb, ss);
}
-static int usb_write_10(ccb *srb, struct us_data *ss, unsigned long start,
unsigned short blocks)-{
- memset(&srb->cmd[0], 0, 12);
- srb->cmd[0] = SCSI_WRITE10;
- srb->cmd[1] = srb->lun << 5;
- srb->cmd[2] = ((unsigned char) (start >> 24)) & 0xff;
- srb->cmd[3] = ((unsigned char) (start >> 16)) & 0xff;
- srb->cmd[4] = ((unsigned char) (start >> 8)) & 0xff;
- srb->cmd[5] = ((unsigned char) (start)) & 0xff;
- srb->cmd[7] = ((unsigned char) (blocks >> 8)) & 0xff;
- srb->cmd[8] = (unsigned char) blocks & 0xff;
- srb->cmdlen = 12;
- debug("write10: start %lx blocks %x\n", start, blocks);
- return ss->transport(srb, ss);
-}
This stuff should be in a separate patch, otherwise it's not bisectable. Looks pretty OK otherwise.
[...]
-----Original Message----- From: Marek Vasut [mailto:marex@denx.de] Sent: Thursday, June 16, 2016 3:33 AM 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 Subject: Re: [PATCH v6 1/2] common: usb_storage: Make common function for usb_stor_read/usb_stor_write
On 06/15/2016 09:00 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 v6:
- Removes USB_STOR_OP_TYPE macro and adds __func__ in debug prints
- Unifies usb_read_10/usb_write_10 functions to usb_read_write_10
Changes in v5:
- Converts USB_STOR_OPERATION_FUNC macro to a function
- Corrects the multi line comment accroding to coding style
- Renames variable to dev to remove code duplication
Changes in v4:
- Adds code to make common function for read/write
common/usb_storage.c | 147 +++++++++++++-------------------------------------- 1 file changed, 36 insertions(+), 111 deletions(-)
diff --git a/common/usb_storage.c b/common/usb_storage.c index 7e6e52d..41e5aa3 100644 --- a/common/usb_storage.c +++ b/common/usb_storage.c @@ -1046,11 +1046,11 @@ static int usb_read_capacity(ccb *srb, struct us_data
*ss)
return -1; }
-static int usb_read_10(ccb *srb, struct us_data *ss, unsigned long start,
unsigned short blocks)+static int usb_read_write_10(ccb *srb, struct us_data *ss, unsigned long start,
unsigned short blocks, bool is_write){ memset(&srb->cmd[0], 0, 12);
- srb->cmd[0] = SCSI_READ10;
- srb->cmd[0] = is_write ? SCSI_WRITE10 : SCSI_READ10; srb->cmd[1] = srb->lun << 5; srb->cmd[2] = ((unsigned char) (start >> 24)) & 0xff; srb->cmd[3] = ((unsigned char) (start >> 16)) & 0xff; @@ -1059,28
+1059,10 @@ static int usb_read_10(ccb *srb, struct us_data *ss, unsigned long
start,
srb->cmd[7] = ((unsigned char) (blocks >> 8)) & 0xff; srb->cmd[8] = (unsigned char) blocks & 0xff; srb->cmdlen = 12;
- debug("read10: start %lx blocks %x\n", start, blocks);
- debug("%s: start %lx blocks %x\n", __func__, start, blocks); return ss->transport(srb, ss);
}
-static int usb_write_10(ccb *srb, struct us_data *ss, unsigned long start,
unsigned short blocks)-{
- memset(&srb->cmd[0], 0, 12);
- srb->cmd[0] = SCSI_WRITE10;
- srb->cmd[1] = srb->lun << 5;
- srb->cmd[2] = ((unsigned char) (start >> 24)) & 0xff;
- srb->cmd[3] = ((unsigned char) (start >> 16)) & 0xff;
- srb->cmd[4] = ((unsigned char) (start >> 8)) & 0xff;
- srb->cmd[5] = ((unsigned char) (start)) & 0xff;
- srb->cmd[7] = ((unsigned char) (blocks >> 8)) & 0xff;
- srb->cmd[8] = (unsigned char) blocks & 0xff;
- srb->cmdlen = 12;
- debug("write10: start %lx blocks %x\n", start, blocks);
- return ss->transport(srb, ss);
-}
Hello Marek,
This stuff should be in a separate patch, otherwise it's not bisectable. Looks pretty OK otherwise.
Will take care in v7.
Best Regards, Rajesh Bhagat
[...]
-- Best regards, Marek Vasut
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 Reviewed-by: Simon Glass sjg@chromium.org --- Changes in v6: - Adds paranthesis around macro variables - Removes extra ternary operator from dec_cur_xfer_blks - Clamps the size to min(blks, USB_MAX_XFER_BLK) in inc_cur_xfer_blks
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 | 66 ++++++++++++++++++++++++++++++++++++++++++++++++---- include/usb.h | 1 + 2 files changed, 62 insertions(+), 5 deletions(-)
diff --git a/common/usb_storage.c b/common/usb_storage.c index 41e5aa3..0856052 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 = CALC_CUR_XFER_BLKS(pos - 1); + + 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 > min(blks, (lbaint_t)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 @@ -1102,6 +1145,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; @@ -1141,26 +1185,35 @@ 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("%s: retry #%d, cur_xfer_blks %hu, smallblks %hu\n", + __func__, 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_read_write_10(srb, ss, start, smallblks, is_write)) { debug("%s ERROR\n", __func__); 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;
@@ -1169,7 +1222,7 @@ retry_it: __func__, 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; } @@ -1256,6 +1309,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;
participants (2)
-
Marek Vasut -
Rajesh Bhagat