[U-Boot] [PATCH v5 2/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, June 14, 2016 5:41 PM
> To: Rajesh Bhagat <rajesh.bhagat at nxp.com>; u-boot at lists.denx.de
> Cc: sjg at chromium.org; york sun <york.sun at nxp.com>; Sriram Dash
> <sriram.dash at nxp.com>; Rajesh Bhagat <rajesh.bhagat at freescale.com>
> Subject: Re: [PATCH v5 2/2] common: usb_storage : Implement logic to calculate
> optimal usb maximum trasfer blocks
> 
> On 06/14/2016 05:41 AM, Rajesh Bhagat wrote:
> >
> >
> >> -----Original Message-----
> >> From: Marek Vasut [mailto:marex at denx.de]
> >> Sent: Monday, June 13, 2016 7:07 PM
> >> To: Rajesh Bhagat <rajesh.bhagat at nxp.com>; u-boot at lists.denx.de
> >> Cc: sjg at chromium.org; york sun <york.sun at nxp.com>; Sriram Dash
> >> <sriram.dash at nxp.com>; Rajesh Bhagat <rajesh.bhagat at 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 at 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 at nxp.com>
> >>> Signed-off-by: Rajesh Bhagat <rajesh.bhagat at 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.
> 

Will take care in v6. Yes it does, as it is keeping the logic intact for size to be less than blks and
USB_MAX_XFER_BLK both.

Best Regards,
Rajesh Bhagat

> 
> --
> Best regards,
> Marek Vasut