On 06/14/2016 05:41 AM, Rajesh Bhagat wrote: > > >> -----Original Message----- >> From: Marek Vasut [mailto:ma...@denx.de] >> Sent: Monday, June 13, 2016 7:07 PM >> To: Rajesh Bhagat <rajesh.bha...@nxp.com>; u-boot@lists.denx.de >> Cc: s...@chromium.org; york sun <york....@nxp.com>; Sriram Dash >> <sriram.d...@nxp.com>; Rajesh Bhagat <rajesh.bha...@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.bha...@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.d...@nxp.com> >>> Signed-off-by: Rajesh Bhagat <rajesh.bha...@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;
Yes, if that makes sense. -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
