On Fri, Sep 13, 2024 at 09:20:45AM +0800, Peter Chen wrote:
> On 24-09-12 12:51:49, Xu Yang wrote:
> > Currently, the deivice controller has below limitations:
> > 1. can't generate short packet interrupt if IOC not set in dTD. So if one
> > request span more than one dTDs and only the last dTD set IOC, the usb
> > request will pending there if no more data comes.
> > 2. the controller can't accurately deliver data to differtent usb requests
> > in some cases due to short packet. For example: one usb request span 3
> > dTDs, then if the controller received a short packet the next packet
> > will go to 2nd dTD of current request rather than the first dTD of next
> > request.
> >
>
> Are there any IP errata for it?
No. It's decided by hw IP design. This old design may not suit current
requirements.
>
> > To let the device controller work properly, one usb request should only
> > correspond to one dTD. Then every dTD will set IOC. In theory, each dTD
> > support up to 20KB data transfer if the offset is 0. Due to we cannot
> > predetermine the offset, this will limit the usb request length to max
> > 16KB. This should be fine since most of the user transfer data based on
> > this size policy.
> >
> > Although these limitations found on OUT eps, we can put the request to IN
> > eps too, this will benefit the following patches.
>
> Since IN endpoints have not found the problem, please limit the changes
> only for OUT endpoints.
This 1st patch is mainly used to serve the 2nd patch which may impact
both IN and OUT eps. Because it's hard to judge whether a request is
suit for transfer if it spans more dTDs. So it's needed for both eps.
I've looked through all function drivers, all of them use 16KB as max
request size. If one future function driver does use a larger request
size, they can adjust it according to this error log too.
>
> >
> > Signed-off-by: Xu Yang <xu.yang_2@xxxxxxx>
> > ---
> > drivers/usb/chipidea/ci.h | 1 +
> > drivers/usb/chipidea/udc.c | 5 +++++
> > 2 files changed, 6 insertions(+)
> >
> > diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
> > index 2a38e1eb6546..f8deccfc8713 100644
> > --- a/drivers/usb/chipidea/ci.h
> > +++ b/drivers/usb/chipidea/ci.h
> > @@ -25,6 +25,7 @@
> > #define TD_PAGE_COUNT 5
> > #define CI_HDRC_PAGE_SIZE 4096ul /* page size for TD's */
> > #define ENDPT_MAX 32
> > +#define CI_MAX_REQ_SIZE (4 * CI_HDRC_PAGE_SIZE)
> > #define CI_MAX_BUF_SIZE (TD_PAGE_COUNT * CI_HDRC_PAGE_SIZE)
> >
> > /******************************************************************************
> > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> > index b1a1be6439b6..5d9369d01065 100644
> > --- a/drivers/usb/chipidea/udc.c
> > +++ b/drivers/usb/chipidea/udc.c
> > @@ -969,6 +969,11 @@ static int _ep_queue(struct usb_ep *ep, struct usb_request *req,
> > return -EMSGSIZE;
> > }
> >
> > + if (hwreq->req.length > CI_MAX_REQ_SIZE) {
> > + dev_err(hwep->ci->dev, "request length too big (max 16KB)\n");
> > + return -EMSGSIZE;
> > + }
> > +
>
> Since this IP is used by many vendors, it may fix by others.
> I prefer add flag like CI_HDRC_SHORT_PACKET_NOT_SUPPORT,
> and set in imx platform file.
Okay, I can limit the impact to only imx platform.
Thanks,
Xu Yang
