[U-Boot] [PATCH 1/7] usb: rockchip: fix command failed on host side due to missing data

[Lukasz Majewski]

Hi Alberto,

> Two consecutive rockusb_tx_write without waiting for request complete
> do results in transfer reset of first request and thus no or
> incomplete data transfer. This because rockusb_tx_write do use just
> une request to keep serialization.
> 
> So calls like:
> rockusb_tx_write_str(emmc_id);
> rockusb_tx_write_csw(cbw->tag, cbw->data_transfer_length, CSW_GOOD);
> 
> was succeeding only when DEBUG was defined because the time spent
> printing debug info was enough for request to complete.

Serialization by printf..... 

> 
> This patch add a way to postpone sending csw after first
> rockusb_tx_write is completed (indeed inside rockusb_complete) fixing
> execution of: $ rkdeveloptool rfi
> when DEBUG is not defined.
> 
> Signed-off-by: Alberto Panizzo <alberto at amarulasolutions.com>
> ---
>  arch/arm/include/asm/arch-rockchip/f_rockusb.h |  1 +
>  drivers/usb/gadget/f_rockusb.c                 | 37
> ++++++++++++++++++++++---- 2 files changed, 33 insertions(+), 5
> deletions(-)
> 
> diff --git a/arch/arm/include/asm/arch-rockchip/f_rockusb.h
> b/arch/arm/include/asm/arch-rockchip/f_rockusb.h index
> 0b62771..f5cad8e 100644 ---
> a/arch/arm/include/asm/arch-rockchip/f_rockusb.h +++
> b/arch/arm/include/asm/arch-rockchip/f_rockusb.h @@ -124,6 +124,7 @@
> struct f_rockusb { int reboot_flag;
>  	void *buf;
>  	void *buf_head;
> +	struct bulk_cs_wrap *next_csw;
>  };
>  
>  /* init rockusb device, tell rockusb which device you want to
> read/write*/ diff --git a/drivers/usb/gadget/f_rockusb.c
> b/drivers/usb/gadget/f_rockusb.c index b8833d0..a39ad51 100644
> --- a/drivers/usb/gadget/f_rockusb.c
> +++ b/drivers/usb/gadget/f_rockusb.c
> @@ -97,6 +97,7 @@ static struct usb_gadget_strings *rkusb_strings[] =
> { 
>  static struct f_rockusb *rockusb_func;
>  static void rx_handler_command(struct usb_ep *ep, struct usb_request
> *req); +static int rockusb_tx_write(const char *buffer, unsigned int
> buffer_size); static int rockusb_tx_write_csw(u32 tag, int residue,
> u8 status, int size); 
>  struct f_rockusb *get_rkusb(void)
> @@ -136,11 +137,22 @@ struct usb_endpoint_descriptor *hs)
>  
>  static void rockusb_complete(struct usb_ep *ep, struct usb_request
> *req) {
> +	struct f_rockusb *f_rkusb = get_rkusb();
>  	int status = req->status;
>  
> -	if (!status)
> -		return;
> -	debug("status: %d ep '%s' trans: %d\n", status, ep->name,
> req->actual);
> +	if (status)
> +		debug("status: %d ep '%s' trans: %d\n",
> +		      status, ep->name, req->actual);
> +
> +	/* Send Command Status on previous transfer complete */
> +	if (f_rkusb->next_csw) {
		     ^^^^^^^^ - isn't this a bit misleading? We send
		     the status for the previous transfer.

> +#ifdef DEBUG
> +		printcsw((char *)f_rkusb->next_csw);
> +#endif
> +		rockusb_tx_write((char *)f_rkusb->next_csw,
> +				 USB_BULK_CS_WRAP_LEN);
> +	}
> +	f_rkusb->next_csw = NULL;
>  }
>  
>  /* config the rockusb device*/
> @@ -388,6 +400,21 @@ static int rockusb_tx_write_csw(u32 tag, int
> residue, u8 status, int size) return rockusb_tx_write((char *)csw,
> size); }
>  
> +struct bulk_cs_wrap g_next_csw;

You have added the pointer to struct bulk_cs_wrap_g to struct
f_rockusb, and here we do have global definition.

Two issues with cache; alignment and padding.

Maybe it would be better to allocate it and store pointer int struct
f_rockusb ?

> +static void rockusb_tx_write_csw_on_complete(u32 tag, int residue,
> u8 status) +{
> +	struct f_rockusb *f_rkusb = get_rkusb();
> +
> +	g_next_csw.signature = cpu_to_le32(USB_BULK_CS_SIG);
> +	g_next_csw.tag = tag;
> +	g_next_csw.residue = cpu_to_be32(residue);
> +	g_next_csw.status = status;
> +#ifdef DEBUG
> +	printcsw((char *)&g_next_csw);
> +#endif
> +	f_rkusb->next_csw = &g_next_csw;
> +}
> +
>  static unsigned int rx_bytes_expected(struct usb_ep *ep)
>  {
>  	struct f_rockusb *f_rkusb = get_rkusb();
> @@ -501,8 +528,8 @@ static void cb_read_storage_id(struct usb_ep *ep,
> struct usb_request *req) printf("read storage id\n");
>  	memcpy((char *)cbw, req->buf, USB_BULK_CB_WRAP_LEN);
>  	rockusb_tx_write_str(emmc_id);
> -	rockusb_tx_write_csw(cbw->tag, cbw->data_transfer_length,
> CSW_GOOD,
> -			     USB_BULK_CS_WRAP_LEN);
> +	rockusb_tx_write_csw_on_complete(cbw->tag,
> cbw->data_transfer_length,
> +					 CSW_GOOD);

It seems like you are preparing the content of the csw structure to be
ready when the completion is called. Am I right?

What I'm concerned about - with your patch we do have two functions
with almost the same code - namely rockusb_tx_write_csw() and
rockusb_tx_write_csw_on_complete(). 

Would it be possible to unify (reuse) the code?

One more remark - shouldn't we set content of g_next_csw in the
rockusb_complete() ?

>  }
>  
>  static void cb_write_lba(struct usb_ep *ep, struct usb_request *req)




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180703/ad42e197/attachment.sig>