[U-Boot] [PATCH 1/7] usb: rockchip: fix command failed on host side due to missing data
Alberto Panizzo
alberto at amarulasolutions.com
Wed Jul 4 10:11:33 UTC 2018
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 <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.
>
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 *)&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?
Yes
>
> 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?
>
It is possible to reuse more the code yes.
> One more remark - shouldn't we set content of g_next_csw in the
> rockusb_complete() ?
This no. because CSW cotains data of the generating request (like tag)
and status is not something related to USB bus (being able to complete
transfer) but status is related to the ability to complete the
issued command.
This way if I will construct the csw block on rockusb_complete() function
I will need to store anyway somewere the same data generated on
rockusb_tx_write_csw_on_complete() function call.
>
> > }
> >
> > static void cb_write_lba(struct usb_ep *ep, struct usb_request *req)
>
>
Best Regards,
Alberto Panizzo
--
Presidente CDA
Amarula Solutions SRL Via le Canevare 30 31100 Treviso Italy
CTO - Co-Founder
Amarula Solutions BV Cruquiuskade 47 Amsterdam 1018 AM The Netherlands
Phone. +31(0)851119171 Fax. +31(0)204106211 www.amarulasolutions.com
More information about the U-Boot
mailing list