Re: [U-Boot] [PATCH 1/7] usb: rockchip: fix command failed on host side due to missing data
Hi Lukasz,
On Tue, Jul 03, 2018 at 11:24:34PM +0200, Lukasz Majewski wrote:
> 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
> > ---
> > 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.
>
This is part of the rockusb protocol: every commands issued by the
workstation will have back an optional payload request + this CSW
control block.
Commands are serialized by workstation: on rockusb you do not have
a real parallel execution of multiple commands.
Thus the simple caching of next CSW block to send is enough to
bring back things working.
The other possibility is to have a different _complete function
to send back the CSW request as the write_lba is doing.
> > +#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 ?
>
Well, not really because rockusb_tx_write will not use directly our
g_next_csw, but copy its data into rockusb_func->in_req->buf
which is indeed allocated aligned for DMA.
Style wide I agree to replace the global pointer with alloc/free sequences
> > +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 *)_next_csw);
> > +#endif
> > + f_rkusb->next_csw = _next_csw;
> > +}
> > +
> > static unsigned int rx_bytes_expected(struct usb_ep *ep)
> > {
> > struct f_rockusb *f_rkusb = get_rkusb();
> > @@
Re: [U-Boot] [PATCH 1/7] usb: rockchip: fix command failed on host side due to missing data
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
> ---
> 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 *)_next_csw);
> +#endif
> + f_rkusb->next_csw = _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
[U-Boot] [PATCH 1/7] usb: rockchip: fix command failed on host side due to missing data
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.
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
---
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) {
+#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;
+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 *)_next_csw);
+#endif
+ f_rkusb->next_csw = _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);
}
static void cb_write_lba(struct usb_ep *ep, struct usb_request *req)
--
2.7.4
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
