Re: [PATCH] usb: dwc3: gadget: Avoid canceling current request for queuing error
Wesley Cheng wrote: > > > On 4/15/2021 12:28 PM, Thinh Nguyen wrote: >> Thinh Nguyen wrote: >>> Wesley Cheng wrote: >>>> >>>> >>>> On 4/14/2021 11:26 PM, Felipe Balbi wrote: >>>>> Wesley Cheng writes: >>>>> >>>>>> If an error is received when issuing a start or update transfer >>>>>> command, the error handler will stop all active requests (including >>>>>> the current USB request), and call dwc3_gadget_giveback() to notify >>>>>> function drivers of the requests which have been stopped. Avoid >>>>>> having to cancel the current request which is trying to be queued, as >>>>>> the function driver will handle the EP queue error accordingly. >>>>>> Simply unmap the request as it was done before, and allow previously >>>>>> started transfers to be cleaned up. >>>>>> >>>>>> Signed-off-by: Wesley Cheng >>>>>> --- >>>>>> drivers/usb/dwc3/gadget.c | 5 + >>>>>> 1 file changed, 5 insertions(+) >>>>>> >>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>>>>> index e1b04c97..4200775 100644 >>>>>> --- a/drivers/usb/dwc3/gadget.c >>>>>> +++ b/drivers/usb/dwc3/gadget.c >>>>>> @@ -1399,6 +1399,11 @@ static int __dwc3_gadget_kick_transfer(struct >>>>>> dwc3_ep *dep) >>>>>> if (ret == -EAGAIN) >>>>>> return ret; >>>>>> >>>>>> +/* Avoid canceling current request, as it has not been >>>>>> started */ >>>>>> +if (req->trb) >>>>>> +memset(req->trb, 0, sizeof(struct dwc3_trb)); >>>>> >>>>> we don't need a full memset. I think ensuring HWO bit is zero is enough. >>>>> >>>> Hi Felipe, >>>> >>>> Thanks for the input/review, will make this change to just clear the HWO. >>>> >>> >>> Make sure to increment the dequeue pointer also. I think you can use >>> dwc3_gadget_ep_skip_trbs(). >>> >> >> Nevermind. There maybe a problem with using dwc3_gadget_ep_skip_trbs(). >> >> Thinh >> > Hi Thinh, > > Thank you for your input. In this case (if kick transfer fails w/ an > error), would we still need to mess with the enqueue/dequeue pointers? > Not sure if my assumption is correct, but the TRB wouldn't have been > started, so we can use the same (failed) TRB for future requests, right? > > I think one thing I will need to update is to loop through num_trbs and > clear all HWO bits if the above is not needed. > No. We track and increment the TRB enqueue counter whenever we prepared one. What you need to do is the opposite as dwc3_gadget_ep_skip_trbs(). You'd need to decrement the enqueue counter of all the TRB prepared by the request. Not every request is the same, and we shouldn't rely on the function driver behavior to reuse/resubmit the same request. Also, we use __dwc3_gadget_kick_transfer() everywhere and not just when queuing new request. Make sure that your change only applies when queuing new request failed and not because of other scenarios. Thanks, Thinh
Re: [PATCH] usb: dwc3: gadget: Avoid canceling current request for queuing error
Thinh Nguyen wrote: > Wesley Cheng wrote: >> >> >> On 4/14/2021 11:26 PM, Felipe Balbi wrote: >>> Wesley Cheng writes: >>> >>>> If an error is received when issuing a start or update transfer >>>> command, the error handler will stop all active requests (including >>>> the current USB request), and call dwc3_gadget_giveback() to notify >>>> function drivers of the requests which have been stopped. Avoid >>>> having to cancel the current request which is trying to be queued, as >>>> the function driver will handle the EP queue error accordingly. >>>> Simply unmap the request as it was done before, and allow previously >>>> started transfers to be cleaned up. >>>> >>>> Signed-off-by: Wesley Cheng >>>> --- >>>> drivers/usb/dwc3/gadget.c | 5 + >>>> 1 file changed, 5 insertions(+) >>>> >>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>>> index e1b04c97..4200775 100644 >>>> --- a/drivers/usb/dwc3/gadget.c >>>> +++ b/drivers/usb/dwc3/gadget.c >>>> @@ -1399,6 +1399,11 @@ static int __dwc3_gadget_kick_transfer(struct >>>> dwc3_ep *dep) >>>>if (ret == -EAGAIN) >>>>return ret; >>>> >>>> + /* Avoid canceling current request, as it has not been started >>>> */ >>>> + if (req->trb) >>>> + memset(req->trb, 0, sizeof(struct dwc3_trb)); >>> >>> we don't need a full memset. I think ensuring HWO bit is zero is enough. >>> >> Hi Felipe, >> >> Thanks for the input/review, will make this change to just clear the HWO. >> > > Make sure to increment the dequeue pointer also. I think you can use > dwc3_gadget_ep_skip_trbs(). > Nevermind. There maybe a problem with using dwc3_gadget_ep_skip_trbs(). Thinh
Re: [PATCH] usb: dwc3: gadget: Avoid canceling current request for queuing error
Wesley Cheng wrote: > > > On 4/14/2021 11:26 PM, Felipe Balbi wrote: >> Wesley Cheng writes: >> >>> If an error is received when issuing a start or update transfer >>> command, the error handler will stop all active requests (including >>> the current USB request), and call dwc3_gadget_giveback() to notify >>> function drivers of the requests which have been stopped. Avoid >>> having to cancel the current request which is trying to be queued, as >>> the function driver will handle the EP queue error accordingly. >>> Simply unmap the request as it was done before, and allow previously >>> started transfers to be cleaned up. >>> >>> Signed-off-by: Wesley Cheng >>> --- >>> drivers/usb/dwc3/gadget.c | 5 + >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>> index e1b04c97..4200775 100644 >>> --- a/drivers/usb/dwc3/gadget.c >>> +++ b/drivers/usb/dwc3/gadget.c >>> @@ -1399,6 +1399,11 @@ static int __dwc3_gadget_kick_transfer(struct >>> dwc3_ep *dep) >>> if (ret == -EAGAIN) >>> return ret; >>> >>> + /* Avoid canceling current request, as it has not been started >>> */ >>> + if (req->trb) >>> + memset(req->trb, 0, sizeof(struct dwc3_trb)); >> >> we don't need a full memset. I think ensuring HWO bit is zero is enough. >> > Hi Felipe, > > Thanks for the input/review, will make this change to just clear the HWO. > Make sure to increment the dequeue pointer also. I think you can use dwc3_gadget_ep_skip_trbs(). BR, Thinh
Re: [Patch v4] usb: dwc3: add cancelled reasons for dwc3 requests
DWC3_REQUEST_STATUS_DEQUEUED);
>
> dep->flags &= ~DWC3_EP_WAIT_TRANSFER_COMPLETE;
>
> @@ -1848,7 +1864,7 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int
> value, int protocol)
> dwc3_stop_active_transfer(dep, true, true);
>
> list_for_each_entry_safe(req, tmp, >started_list, list)
> - dwc3_gadget_move_cancelled_request(req);
> + dwc3_gadget_move_cancelled_request(req,
> DWC3_REQUEST_STATUS_STALLED);
>
> if (dep->flags & DWC3_EP_END_TRANSFER_PENDING) {
> dep->flags |= DWC3_EP_PENDING_CLEAR_STALL;
> diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
> index 0cd281949970..77df4b6d6c13 100644
> --- a/drivers/usb/dwc3/gadget.h
> +++ b/drivers/usb/dwc3/gadget.h
> @@ -90,15 +90,17 @@ static inline void
> dwc3_gadget_move_started_request(struct dwc3_request *req)
> /**
> * dwc3_gadget_move_cancelled_request - move @req to the cancelled_list
> * @req: the request to be moved
> + * @reason: cancelled reason for the dwc3 request
> *
> * Caller should take care of locking. This function will move @req from its
> * current list to the endpoint's cancelled_list.
> */
> -static inline void dwc3_gadget_move_cancelled_request(struct dwc3_request
> *req)
> +static inline void dwc3_gadget_move_cancelled_request(struct dwc3_request
> *req,
> + unsigned int reason)
> {
> struct dwc3_ep *dep = req->dep;
>
> - req->status = DWC3_REQUEST_STATUS_CANCELLED;
> + req->status = reason;
> list_move_tail(>list, >cancelled_list);
> }
>
>
Reviewed-by: Thinh Nguyen
Thanks for the patch,
Thinh
Re: [PATCH v3 2/2] usb: dwc3: Fix DRD mode change sequence following programming guide
Wesley Cheng wrote: > > > On 3/6/2021 3:39 PM, Thinh Nguyen wrote: >> Wesley Cheng wrote: >>> >>> On 1/7/2021 5:51 PM, John Stultz wrote: >>>> In reviewing the previous patch, Thinh Nguyen pointed out that >>>> the DRD mode change sequence should be like the following when >>>> switching from host -> device according to the programming guide >>>> (for all DRD IPs): >>>> 1. Reset controller with GCTL.CoreSoftReset >>>> 2. Set GCTL.PrtCapDir(device) >>>> 3. Soft reset with DCTL.CSftRst >>>> 4. Then follow up with the initializing registers sequence >>>> >>>> The current code does: >>>> a. Soft reset with DCTL.CSftRst on driver probe >>>> b. Reset controller with GCTL.CoreSoftReset (added in previous >>>>patch) >>>> c. Set GCTL.PrtCapDir(device) >>>> d. < missing DCTL.CSftRst > >>>> e. Then follow up with initializing registers sequence >>>> >>>> So this patch adds the DCTL.CSftRst soft reset that was currently >>>> missing from the dwc3 mode switching. >>>> >>>> Cc: Felipe Balbi >>>> Cc: Tejas Joglekar >>>> Cc: Yang Fei >>>> Cc: YongQin Liu >>>> Cc: Andrzej Pietrasiewicz >>>> Cc: Thinh Nguyen >>>> Cc: Jun Li >>>> Cc: Mauro Carvalho Chehab >>>> Cc: Greg Kroah-Hartman >>>> Cc: linux-...@vger.kernel.org >>>> Signed-off-by: John Stultz >>>> --- >>>> Feedback would be appreciated. I'm a little worried I should be >>>> conditionalizing the DCTL.CSftRst on DRD mode controllers, but >>>> I'm really not sure what the right thing to do is for non-DRD >>>> mode controllers. >>>> --- >>>> drivers/usb/dwc3/core.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>>> index b6a6b90eb2d5..71f8b07ecb99 100644 >>>> --- a/drivers/usb/dwc3/core.c >>>> +++ b/drivers/usb/dwc3/core.c >>>> @@ -40,6 +40,8 @@ >>>> >>>> #define DWC3_DEFAULT_AUTOSUSPEND_DELAY5000 /* ms */ >>>> >>>> +static int dwc3_core_soft_reset(struct dwc3 *dwc); >>>> + >>>> /** >>>> * dwc3_get_dr_mode - Validates and sets dr_mode >>>> * @dwc: pointer to our context structure >>>> @@ -177,6 +179,7 @@ static void __dwc3_set_mode(struct work_struct *work) >>>> >>>>dwc3_set_prtcap(dwc, dwc->desired_dr_role); >>>> >>>> + dwc3_core_soft_reset(dwc); >>> Hi John/Thinh/Felipe, >>> >>> I actually added this change into my local branch, because we were >>> seeing an issue when switching from host mode --> peripheral mode. What >>> was happening was that the RXFIFO register did not update back to the >>> expected value for peripheral mode by the time >>> dwc3_gadget_init_out_endpoint() was executed. With the logic to >>> calculate the EP max packet limit based on RXFIFO reg, this caused all >>> EPs to be set with an EP max limit of 0. >>> >>> With this change, it seemed to help with the above issue. However, can >>> we consider moving the core soft reset outside the spinlock? At least >>> with our PHY init routines, we have some msleep() calls for waiting for >>> the PHYs to be ready, which will end up as a sleeping while atomic bug. >>> (not sure if PHY init is required to be called in atomic context) >>> >>> Thanks >>> Wesley Cheng >> >> Hi Wesley, >> >> Thanks for letting us know the issue you're having also. >> >> Yes, you need to wait a certain amount of time to synchronize with the >> PHY (at least 50ms for dwc_usb32 and dwc_usb31 v1.80a and above, and >> less for older versions). When removing the spinlock to use msleep(), >> just make sure that there's no race issue. BTW, how long does your setup >> need to msleep()? >> > Hi Thinh, > > Sorry for the late response. My mistake, its actually just a usleep() > for a less than 100uS (polling for a status bit change, so it will exit > early if possible). For this change, can we just move the > dwc3_core_soft_reset() outside of the spinlock? > > Thanks > Wesley Cheng > Hi Wesley, dwc3 can get notified at any time to queue a work to switch mode. So you need protect it from a potential race. I think you can use a mutex for this. Also, what status are you polling? Note that there's no status bit for GCTL.coresoftreset. For DCTL.CSFTRST, different controller versions behave differently. Use dwc3_core_soft_reset() for DCTL.CSFTRST to get the logic from there. 1 more thing, make sure that this flow only applies for DRD mode controller and not OTG from older DWC_usb3 IP. Thanks, Thinh
Re: [Patch v2] usb: dwc3: add cancelled reasons for dwc3 requests
Hi Ray,
Ray Chi wrote:
> Currently, when dwc3 handles request cancelled, dwc3 just returns
> -ECONNRESET for all requests. It will cause USB function drivers
> can't know if the requests are cancelled by other reasons.
>
> This patch will replace DWC3_REQUEST_STATUS_CANCELLED with the
> reasons below.
> - DWC3_REQUEST_STATUS_DISCONNECTED
> - DWC3_REQUEST_STATUS_DEQUEUED
> - DWC3_REQUEST_STATUS_STALLED
>
> Signed-off-by: Ray Chi
> ---
> drivers/usb/dwc3/core.h | 12 +++-
> drivers/usb/dwc3/gadget.c | 21 +
> drivers/usb/dwc3/gadget.h | 6 --
> 3 files changed, 28 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 4ca4b4be85e4..51a3eec2428a 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -908,11 +908,13 @@ struct dwc3_request {
> unsigned intremaining;
>
> unsigned intstatus;
> -#define DWC3_REQUEST_STATUS_QUEUED 0
> -#define DWC3_REQUEST_STATUS_STARTED 1
> -#define DWC3_REQUEST_STATUS_CANCELLED2
> -#define DWC3_REQUEST_STATUS_COMPLETED3
> -#define DWC3_REQUEST_STATUS_UNKNOWN -1
> +#define DWC3_REQUEST_STATUS_QUEUED 0
> +#define DWC3_REQUEST_STATUS_STARTED 1
> +#define DWC3_REQUEST_STATUS_DISCONNECTED 2
> +#define DWC3_REQUEST_STATUS_DEQUEUED 3
> +#define DWC3_REQUEST_STATUS_STALLED 4
> +#define DWC3_REQUEST_STATUS_COMPLETED5
> +#define DWC3_REQUEST_STATUS_UNKNOWN -1
>
> u8 epnum;
> struct dwc3_trb *trb;
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index e1442fc763e1..b11fd02604bb 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1402,7 +1402,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep
> *dep)
> dwc3_stop_active_transfer(dep, true, true);
>
> list_for_each_entry_safe(req, tmp, >started_list, list)
> - dwc3_gadget_move_cancelled_request(req);
> + dwc3_gadget_move_cancelled_request(req,
> DWC3_REQUEST_STATUS_DEQUEUED);
>
> /* If ep isn't started, then there's no end transfer pending */
> if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
> @@ -1732,7 +1732,19 @@ static void
> dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep)
>
> list_for_each_entry_safe(req, tmp, >cancelled_list, list) {
> dwc3_gadget_ep_skip_trbs(dep, req);
> - dwc3_gadget_giveback(dep, req, -ECONNRESET);
> + switch (req->status) {
> + case DWC3_REQUEST_STATUS_DISCONNECTED:
> + dwc3_gadget_giveback(dep, req, -ESHUTDOWN);
> + break;
> + case DWC3_REQUEST_STATUS_DEQUEUED:
> + dwc3_gadget_giveback(dep, req, -ECONNRESET);
> + break;
> + case DWC3_REQUEST_STATUS_STALLED:
> + dwc3_gadget_giveback(dep, req, -EPIPE);
> + break;
> + default:
Also giveback the request for default case here, but with -ECONNRESET
and a dev_err() print here indicating driver bug. The rest of the patch
looks good.
Thanks,
Thinh
> + break;
> + }
> }
> }
>
> @@ -1776,7 +1788,8 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
>* cancelled.
>*/
> list_for_each_entry_safe(r, t, >started_list, list)
> - dwc3_gadget_move_cancelled_request(r);
> + dwc3_gadget_move_cancelled_request(r,
> + DWC3_REQUEST_STATUS_DEQUEUED);
>
> dep->flags &= ~DWC3_EP_WAIT_TRANSFER_COMPLETE;
>
> @@ -1848,7 +1861,7 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int
> value, int protocol)
> dwc3_stop_active_transfer(dep, true, true);
>
> list_for_each_entry_safe(req, tmp, >started_list, list)
> - dwc3_gadget_move_cancelled_request(req);
> + dwc3_gadget_move_cancelled_request(req,
> DWC3_REQUEST_STATUS_STALLED);
>
> if (dep->flags & DWC3_EP_END_TRANSFER_PENDING) {
> dep->flags |= DWC3_EP_PENDING_CLEAR_STALL;
> diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
> index 0cd281949970..77df4b6d6c13 100644
> --- a/drivers/usb/dwc3/gadget.h
> +++ b/drivers/usb/dwc3/gadget.h
> @@ -90,15 +90,17 @@ static inline void
> dwc3_gadget_move_started_request(struct dwc3_request *req)
> /**
> * dwc3_gadget_move_cancelled_request - move @req to the cancelled_list
> * @req: the request to be moved
> + * @reason: cancelled reason for the dwc3 request
> *
> * Caller should take care of locking. This function will move @req from its
> * current list to the
Re: [PATCH] usb: dwc3: add cancelled reason for dwc3 requests
Felipe Balbi wrote: > Hi, > > Ray Chi writes: >> diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h >> index 0cd281949970..a23e85bd3933 100644 >> --- a/drivers/usb/dwc3/gadget.h >> +++ b/drivers/usb/dwc3/gadget.h >> @@ -56,6 +56,12 @@ struct dwc3; >> >> /* Frame/Microframe Number Mask */ >> #define DWC3_FRNUMBER_MASK 0x3fff >> + >> +/* Cancel reason for dwc3 request */ >> +#define DWC3_REQUEST_DEQUEUED -ECONNRESET /* Request get >> dequeued */ >> +#define DWC3_REQUEST_DISCONNECTED -ESHUTDOWN /* Device is >> disconnected/disabled */ >> +#define DWC3_REQUEST_STALL -EPIPE /* Bus or protocol error */ > > this is just obfuscation, pass the errors directly. Also, make sure > these are documented in the API. > Note: we don't have documentation for error codes for gadget side, only host. These are just the equivalent error codes from the host side. What we currently have for host: https://www.kernel.org/doc/html/latest/driver-api/usb/error-codes.html Maybe someone can work on that in a separate patch. Thanks, Thinh
Re: [PATCH] usb: dwc3: add cancelled reason for dwc3 requests
Hi Ray,
Ray Chi wrote:
> Currently, when dwc3 handles request cancelled, dwc3 just returns
> -ECONNRESET for all requests. It will cause USB class drivers can't
class drivers -> gadget driver or function driver.
> know if the requests are cancelled by other reasons.
>
> This patch will add the reason when the requests are cancelled.
>
> Signed-off-by: Ray Chi
> ---
> drivers/usb/dwc3/gadget.c | 6 +++---
> drivers/usb/dwc3/gadget.h | 10 +-
> 2 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index e1442fc763e1..cc65fc064078 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1402,7 +1402,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep
> *dep)
> dwc3_stop_active_transfer(dep, true, true);
>
> list_for_each_entry_safe(req, tmp, >started_list, list)
> - dwc3_gadget_move_cancelled_request(req);
> + dwc3_gadget_move_cancelled_request(req,
> DWC3_REQUEST_DEQUEUED);
>
> /* If ep isn't started, then there's no end transfer pending */
> if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
> @@ -1776,7 +1776,7 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
>* cancelled.
>*/
> list_for_each_entry_safe(r, t, >started_list, list)
> - dwc3_gadget_move_cancelled_request(r);
> + dwc3_gadget_move_cancelled_request(r,
> DWC3_REQUEST_DEQUEUED);
>
> dep->flags &= ~DWC3_EP_WAIT_TRANSFER_COMPLETE;
>
> @@ -1848,7 +1848,7 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int
> value, int protocol)
> dwc3_stop_active_transfer(dep, true, true);
>
> list_for_each_entry_safe(req, tmp, >started_list, list)
> - dwc3_gadget_move_cancelled_request(req);
> + dwc3_gadget_move_cancelled_request(req,
> DWC3_REQUEST_STALL);
>
> if (dep->flags & DWC3_EP_END_TRANSFER_PENDING) {
> dep->flags |= DWC3_EP_PENDING_CLEAR_STALL;
> diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
> index 0cd281949970..a23e85bd3933 100644
> --- a/drivers/usb/dwc3/gadget.h
> +++ b/drivers/usb/dwc3/gadget.h
> @@ -56,6 +56,12 @@ struct dwc3;
>
> /* Frame/Microframe Number Mask */
> #define DWC3_FRNUMBER_MASK 0x3fff
> +
> +/* Cancel reason for dwc3 request */
> +#define DWC3_REQUEST_DEQUEUED-ECONNRESET /* Request get
> dequeued */
> +#define DWC3_REQUEST_DISCONNECTED-ESHUTDOWN /* Device is
> disconnected/disabled */
> +#define DWC3_REQUEST_STALL -EPIPE /* Bus or protocol error */
> +
> /*
> -- */
>
> #define to_dwc3_request(r) (container_of(r, struct dwc3_request, request))
> @@ -90,15 +96,17 @@ static inline void
> dwc3_gadget_move_started_request(struct dwc3_request *req)
> /**
> * dwc3_gadget_move_cancelled_request - move @req to the cancelled_list
> * @req: the request to be moved
> + * @reason: cancelled reason for the dwc3 request
> *
> * Caller should take care of locking. This function will move @req from its
> * current list to the endpoint's cancelled_list.
> */
> -static inline void dwc3_gadget_move_cancelled_request(struct dwc3_request
> *req)
> +static inline void dwc3_gadget_move_cancelled_request(struct dwc3_request
> *req, int reason)
> {
> struct dwc3_ep *dep = req->dep;
>
> req->status = DWC3_REQUEST_STATUS_CANCELLED;
My suggestion previously was to replace DWC3_REQUEST_STATUS_CANCELLED
with more specific status:
DWC3_REQUEST_STATUS_DISCONNECTED
DWC3_REQUEST_STATUS_DEQUEUED
DWC3_REQUEST_STATUS_STALLED
and set req->status = reason;
> + req->request.status = reason;
Not to update this here as this doesn't do anything. Update
dwc3_gadget_ep_cleanup_cancelled_requests() to have a switch case and
provide the appropriate giveback error code base on the status above.
> list_move_tail(>list, >cancelled_list);
> }
>
>
BR,
Thinh
Re: [PATCH 2/2] usb: dwc3: gadget: Ignore EP queue requests during bus reset
Wesley Cheng wrote:
> Hi Thinh,
>
>
> On 3/19/2021 7:01 PM, Thinh Nguyen wrote:
>> Wesley Cheng wrote:
>>>
>>>
>>> On 3/19/2021 5:40 PM, Thinh Nguyen wrote:
>>>> Hi,
>>>>
>>>> Wesley Cheng wrote:
>>>>> The current dwc3_gadget_reset_interrupt() will stop any active
>>>>> transfers, but only addresses blocking of EP queuing for while we are
>>>>> coming from a disconnected scenario, i.e. after receiving the disconnect
>>>>> event. If the host decides to issue a bus reset on the device, the
>>>>> connected parameter will still be set to true, allowing for EP queuing
>>>>> to continue while we are disabling the functions. To avoid this, set the
>>>>> connected flag to false until the stop active transfers is complete.
>>>>>
>>>>> Signed-off-by: Wesley Cheng
>>>>> ---
>>>>> drivers/usb/dwc3/gadget.c | 9 +
>>>>> 1 file changed, 9 insertions(+)
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>>> index 6e14fdc..d5ed0f69 100644
>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>> @@ -3327,6 +3327,15 @@ static void dwc3_gadget_reset_interrupt(struct
>>>>> dwc3 *dwc)
>>>>> u32 reg;
>>>>>
>>>>> /*
>>>>> + * Ideally, dwc3_reset_gadget() would trigger the function
>>>>> + * drivers to stop any active transfers through ep disable.
>>>>> + * However, for functions which defer ep disable, such as mass
>>>>> + * storage, we will need to rely on the call to stop active
>>>>> + * transfers here, and avoid allowing of request queuing.
>>>>> + */
>>>>> + dwc->connected = false;
>>>>> +
>>>>> + /*
>>>>>* WORKAROUND: DWC3 revisions <1.88a have an issue which
>>>>>* would cause a missing Disconnect Event if there's a
>>>>>* pending Setup Packet in the FIFO.
>>>>>
>>>>
>>>> This doesn't look right. Did you have rebase issue with your local
>>>> change again?
>>>>
>>>> BR,
>>>> Thinh
>>>>
>>> Hi Thinh,
>>>
>>> This was rebased on Greg's usb-linus branch, which has commit
>>> f09ddcfcb8c5 ("usb: dwc3: gadget: Prevent EP queuing while stopping
>>> transfers") merged.
>>
>> Ah I see.
>>
>>>
>>> commit f09ddcfcb8c5 moved the dwc->connected = true to after we have
>>> finished stop active transfers. However, this change will also ensure
>>> that the connected flag is set to false to ensure that when we call stop
>>> active transfers, nothing can prepare TRBs. (previous commit only
>>> addresses the case where we get the reset interrupt when coming from a
>>> disconnected state)
>>>
>>
>> That still doesn't address this issue.
>>
>> Because:
>> 1) We're still protected by the spin_lock_irq*(), so this change doesn't
>> make any difference while handling an event.
>
> Thank you for the feedback. So it is true that we lock dwc->lock while
> handling EP/device events, but what these changes are trying to address
> is that during dwc3_stop_active_transfers() we will eventually call
> dwc3_gadget_giveback() to call the complete() functions registered by
> the function driver. Before we call the complete() callbacks, we unlock
> dwc->lock, so we are no longer protected, and if there was a pending ep
> queue from a function driver, that would allow it to acquire the lock
> and continue preparing the TRBs.
>
Ah I forgot about that.
>> 2) We don't enable the interrupt for END_TRANSFER command completion
>> when doing dwc3_stop_active_transfers(), the
>> DWC3_EP_END_TRANSFER_PENDING flag will not be set to prevent preparing
>> new requests.
>>
> Agreed. That is the reason for adding the check to dwc->connected in
> __dwc3_gadget_ep_queue()
>
> if (!dep->endpoint.desc || !dwc->pullups_connected || !dwc->connected) {
> dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
> dep->name);
> return -ESHUTDOWN;
>
>> We should do dwc->connected = true when we handle connection_done
>> interrupt instead. The END_TRANSFER command should complete before this.
>>
> So how this change will address the issue is:
>
> 1. IRQ handler will acquire dwc->lock
> 2. dwc3_gadget_reset_handler() sets dwc->connected = false
> 3. Call to dwc3_stop_active_transfers()
> ---> dwc3_gadget_giveback() releases dwc->lock
> 4. If there was a pending ep queue (waiting for dwc->lock) it can
> continue here
> 5. __dwc3_gadget_ep_queue() exits early due to dwc->connected = false
> 6. dwc3_gadget_giveback() re-acquires dwc->lock and continues
>
Ok. I thought this was for different issue. I thought you were trying to
solve an issue where a request is queued immediately after handling the
reset interrupt but before the END_TRANSFER command completion.
Thanks for the clarification.
BR,
Thinh
Re: [PATCH 2/2] usb: dwc3: gadget: Ignore EP queue requests during bus reset
Thinh Nguyen wrote:
> Wesley Cheng wrote:
>>
>>
>> On 3/19/2021 5:40 PM, Thinh Nguyen wrote:
>>> Hi,
>>>
>>> Wesley Cheng wrote:
>>>> The current dwc3_gadget_reset_interrupt() will stop any active
>>>> transfers, but only addresses blocking of EP queuing for while we are
>>>> coming from a disconnected scenario, i.e. after receiving the disconnect
>>>> event. If the host decides to issue a bus reset on the device, the
>>>> connected parameter will still be set to true, allowing for EP queuing
>>>> to continue while we are disabling the functions. To avoid this, set the
>>>> connected flag to false until the stop active transfers is complete.
>>>>
>>>> Signed-off-by: Wesley Cheng
>>>> ---
>>>> drivers/usb/dwc3/gadget.c | 9 +
>>>> 1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>> index 6e14fdc..d5ed0f69 100644
>>>> --- a/drivers/usb/dwc3/gadget.c
>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>> @@ -3327,6 +3327,15 @@ static void dwc3_gadget_reset_interrupt(struct dwc3
>>>> *dwc)
>>>>u32 reg;
>>>>
>>>>/*
>>>> + * Ideally, dwc3_reset_gadget() would trigger the function
>>>> + * drivers to stop any active transfers through ep disable.
>>>> + * However, for functions which defer ep disable, such as mass
>>>> + * storage, we will need to rely on the call to stop active
>>>> + * transfers here, and avoid allowing of request queuing.
>>>> + */
>>>> + dwc->connected = false;
>>>> +
>>>> + /*
>>>> * WORKAROUND: DWC3 revisions <1.88a have an issue which
>>>> * would cause a missing Disconnect Event if there's a
>>>> * pending Setup Packet in the FIFO.
>>>>
>>>
>>> This doesn't look right. Did you have rebase issue with your local
>>> change again?
>>>
>>> BR,
>>> Thinh
>>>
>> Hi Thinh,
>>
>> This was rebased on Greg's usb-linus branch, which has commit
>> f09ddcfcb8c5 ("usb: dwc3: gadget: Prevent EP queuing while stopping
>> transfers") merged.
>
> Ah I see.
>
>>
>> commit f09ddcfcb8c5 moved the dwc->connected = true to after we have
>> finished stop active transfers. However, this change will also ensure
>> that the connected flag is set to false to ensure that when we call stop
>> active transfers, nothing can prepare TRBs. (previous commit only
>> addresses the case where we get the reset interrupt when coming from a
>> disconnected state)
>>
>
> That still doesn't address this issue.
>
> Because:
> 1) We're still protected by the spin_lock_irq*(), so this change doesn't
> make any difference while handling an event.
> 2) We don't enable the interrupt for END_TRANSFER command completion
> when doing dwc3_stop_active_transfers(), the
> DWC3_EP_END_TRANSFER_PENDING flag will not be set to prevent preparing
> new requests.
>
> We should do dwc->connected = true when we handle connection_done
> interrupt instead. The END_TRANSFER command should complete before this.
>
> Thanks,
> Thinh
>
Just want to clarify, I was referring to your previous commit
f09ddcfcb8c5, we'd still need dwc->connected = false when handling reset
interrupt as you've done here.
BR,
Thinh
Re: [PATCH 2/2] usb: dwc3: gadget: Ignore EP queue requests during bus reset
Wesley Cheng wrote:
>
>
> On 3/19/2021 5:40 PM, Thinh Nguyen wrote:
>> Hi,
>>
>> Wesley Cheng wrote:
>>> The current dwc3_gadget_reset_interrupt() will stop any active
>>> transfers, but only addresses blocking of EP queuing for while we are
>>> coming from a disconnected scenario, i.e. after receiving the disconnect
>>> event. If the host decides to issue a bus reset on the device, the
>>> connected parameter will still be set to true, allowing for EP queuing
>>> to continue while we are disabling the functions. To avoid this, set the
>>> connected flag to false until the stop active transfers is complete.
>>>
>>> Signed-off-by: Wesley Cheng
>>> ---
>>> drivers/usb/dwc3/gadget.c | 9 +
>>> 1 file changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index 6e14fdc..d5ed0f69 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -3327,6 +3327,15 @@ static void dwc3_gadget_reset_interrupt(struct dwc3
>>> *dwc)
>>> u32 reg;
>>>
>>> /*
>>> +* Ideally, dwc3_reset_gadget() would trigger the function
>>> +* drivers to stop any active transfers through ep disable.
>>> +* However, for functions which defer ep disable, such as mass
>>> +* storage, we will need to rely on the call to stop active
>>> +* transfers here, and avoid allowing of request queuing.
>>> +*/
>>> + dwc->connected = false;
>>> +
>>> + /*
>>> * WORKAROUND: DWC3 revisions <1.88a have an issue which
>>> * would cause a missing Disconnect Event if there's a
>>> * pending Setup Packet in the FIFO.
>>>
>>
>> This doesn't look right. Did you have rebase issue with your local
>> change again?
>>
>> BR,
>> Thinh
>>
> Hi Thinh,
>
> This was rebased on Greg's usb-linus branch, which has commit
> f09ddcfcb8c5 ("usb: dwc3: gadget: Prevent EP queuing while stopping
> transfers") merged.
Ah I see.
>
> commit f09ddcfcb8c5 moved the dwc->connected = true to after we have
> finished stop active transfers. However, this change will also ensure
> that the connected flag is set to false to ensure that when we call stop
> active transfers, nothing can prepare TRBs. (previous commit only
> addresses the case where we get the reset interrupt when coming from a
> disconnected state)
>
That still doesn't address this issue.
Because:
1) We're still protected by the spin_lock_irq*(), so this change doesn't
make any difference while handling an event.
2) We don't enable the interrupt for END_TRANSFER command completion
when doing dwc3_stop_active_transfers(), the
DWC3_EP_END_TRANSFER_PENDING flag will not be set to prevent preparing
new requests.
We should do dwc->connected = true when we handle connection_done
interrupt instead. The END_TRANSFER command should complete before this.
Thanks,
Thinh
Re: [PATCH 2/2] usb: dwc3: gadget: Ignore EP queue requests during bus reset
Hi, Wesley Cheng wrote: > The current dwc3_gadget_reset_interrupt() will stop any active > transfers, but only addresses blocking of EP queuing for while we are > coming from a disconnected scenario, i.e. after receiving the disconnect > event. If the host decides to issue a bus reset on the device, the > connected parameter will still be set to true, allowing for EP queuing > to continue while we are disabling the functions. To avoid this, set the > connected flag to false until the stop active transfers is complete. > > Signed-off-by: Wesley Cheng > --- > drivers/usb/dwc3/gadget.c | 9 + > 1 file changed, 9 insertions(+) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 6e14fdc..d5ed0f69 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -3327,6 +3327,15 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 > *dwc) > u32 reg; > > /* > + * Ideally, dwc3_reset_gadget() would trigger the function > + * drivers to stop any active transfers through ep disable. > + * However, for functions which defer ep disable, such as mass > + * storage, we will need to rely on the call to stop active > + * transfers here, and avoid allowing of request queuing. > + */ > + dwc->connected = false; > + > + /* >* WORKAROUND: DWC3 revisions <1.88a have an issue which >* would cause a missing Disconnect Event if there's a >* pending Setup Packet in the FIFO. > This doesn't look right. Did you have rebase issue with your local change again? BR, Thinh
Re: [PATCH v3 1/2] usb: dwc3: Trigger a GCTL soft reset when switching modes in DRD
Wesley Cheng wrote: > > > On 3/8/2021 10:33 PM, Wesley Cheng wrote: >> >> >> On 3/8/2021 7:05 PM, Thinh Nguyen wrote: >>> Wesley Cheng wrote: >>>> >>>> On 3/6/2021 3:41 PM, Thinh Nguyen wrote: >>>>> Wesley Cheng wrote: >>>>>> On 1/8/2021 4:44 PM, Thinh Nguyen wrote: >>>>>>> Hi, >>>>>>> >>>>>>> John Stultz wrote: >>>>>>>> On Fri, Jan 8, 2021 at 4:26 AM Felipe Balbi wrote: >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> John Stultz writes: >>>>>>>>>> From: Yu Chen >>>>>>>>>> >>>>>>>>>> Just resending this, as discussion died out a bit and I'm not >>>>>>>>>> sure how to make further progress. See here for debug data that >>>>>>>>>> was requested last time around: >>>>>>>>>> >>>>>>>>>> https://urldefense.com/v3/__https://lore.kernel.org/lkml/calaqxlxdnaufjkx0an9xwwtfwvjmwigppy2aqsnj56yvnbu...@mail.gmail.com/__;!!A4F2R9G_pg!LNzuprAeg-O80SgolYkIkW4-ne-M-yLWCDUY9MygAIrQC398Z6gRJ9wnsnlqd3w$ >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> With the current dwc3 code on the HiKey960 we often see the >>>>>>>>>> COREIDLE flag get stuck off in __dwc3_gadget_start(), which >>>>>>>>>> seems to prevent the reset irq and causes the USB gadget to >>>>>>>>>> fail to initialize. >>>>>>>>>> >>>>>>>>>> We had seen occasional initialization failures with older >>>>>>>>>> kernels but with recent 5.x era kernels it seemed to be becoming >>>>>>>>>> much more common, so I dug back through some older trees and >>>>>>>>>> realized I dropped this quirk from Yu Chen during upstreaming >>>>>>>>>> as I couldn't provide a proper rational for it and it didn't >>>>>>>>>> seem to be necessary. I now realize I was wrong. >>>>>>>>>> >>>>>>>>>> After resubmitting the quirk, Thinh Nguyen pointed out that it >>>>>>>>>> shouldn't be a quirk at all and it is actually mentioned in the >>>>>>>>>> programming guide that it should be done when switching modes >>>>>>>>>> in DRD. >>>>>>>>>> >>>>>>>>>> So, to avoid these !COREIDLE lockups seen on HiKey960, this >>>>>>>>>> patch issues GCTL soft reset when switching modes if the >>>>>>>>>> controller is in DRD mode. >>>>>>>>>> >>>>>>>>>> Cc: Felipe Balbi >>>>>>>>>> Cc: Tejas Joglekar >>>>>>>>>> Cc: Yang Fei >>>>>>>>>> Cc: YongQin Liu >>>>>>>>>> Cc: Andrzej Pietrasiewicz >>>>>>>>>> Cc: Thinh Nguyen >>>>>>>>>> Cc: Jun Li >>>>>>>>>> Cc: Mauro Carvalho Chehab >>>>>>>>>> Cc: Greg Kroah-Hartman >>>>>>>>>> Cc: linux-...@vger.kernel.org >>>>>>>>>> Signed-off-by: Yu Chen >>>>>>>>>> Signed-off-by: John Stultz >>>>>>>>>> --- >>>>>>>>>> v2: >>>>>>>>>> * Rework to always call the GCTL soft reset in DRD mode, >>>>>>>>>> rather then using a quirk as suggested by Thinh Nguyen >>>>>>>>>> >>>>>>>>>> v3: >>>>>>>>>> * Move GCTL soft reset under the spinlock as suggested by >>>>>>>>>> Thinh Nguyen >>>>>>>>> Because this is such an invasive change, I would prefer that we get >>>>>>>>> Tested-By tags from a good fraction of the users before applying these >>>>>>>>> two changes. >>>>>>>> I'm happy to reach out to folks to try to get that. Though I'm >>>>>>>> wondering if it would be better to put it behind a dts quirk flag, as >>>>>>>> originally proposed
Re: [PATCH v2] usb: dwc3: gadget: Prevent EP queuing while stopping transfers
Wesley Cheng wrote:
> In the situations where the DWC3 gadget stops active transfers, once
> calling the dwc3_gadget_giveback(), there is a chance where a function
> driver can queue a new USB request in between the time where the dwc3
> lock has been released and re-aquired. This occurs after we've already
> issued an ENDXFER command. When the stop active transfers continues
> to remove USB requests from all dep lists, the newly added request will
> also be removed, while controller still has an active TRB for it.
> This can lead to the controller accessing an unmapped memory address.
>
> Fix this by ensuring parameters to prevent EP queuing are set before
> calling the stop active transfers API.
>
> Fixes: ae7e86108b12 ("usb: dwc3: Stop active transfers before halting the
> controller")
> Signed-off-by: Wesley Cheng
> ---
> Changes since V1:
> - Added Fixes tag to point to the commit this is addressing
>
> drivers/usb/dwc3/gadget.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 4780983..4d98fbf 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -783,8 +783,6 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
>
> trace_dwc3_gadget_ep_disable(dep);
>
> - dwc3_remove_requests(dwc, dep);
> -
> /* make sure HW endpoint isn't stalled */
> if (dep->flags & DWC3_EP_STALL)
> __dwc3_gadget_ep_set_halt(dep, 0, false);
> @@ -803,6 +801,8 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
> dep->endpoint.desc = NULL;
> }
>
> + dwc3_remove_requests(dwc, dep);
> +
> return 0;
> }
>
> @@ -1617,7 +1617,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep,
> struct dwc3_request *req)
> {
> struct dwc3 *dwc = dep->dwc;
>
> - if (!dep->endpoint.desc || !dwc->pullups_connected) {
> + if (!dep->endpoint.desc || !dwc->pullups_connected || !dwc->connected) {
> dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
> dep->name);
> return -ESHUTDOWN;
> @@ -2247,6 +2247,7 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int
> is_on)
> if (!is_on) {
> u32 count;
>
> + dwc->connected = false;
You want to set this before __dwc3_gadget_stop() right? Then you may
want to remove this same setting few lines below this. Otherwise, this
looks good.
Thanks,
Thinh
> /*
>* In the Synopsis DesignWare Cores USB3 Databook Rev. 3.30a
>* Section 4.1.8 Table 4-7, it states that for a
> device-initiated
> @@ -3329,8 +3330,6 @@ static void dwc3_gadget_reset_interrupt(struct dwc3
> *dwc)
> {
> u32 reg;
>
> - dwc->connected = true;
> -
> /*
>* WORKAROUND: DWC3 revisions <1.88a have an issue which
>* would cause a missing Disconnect Event if there's a
> @@ -3370,6 +3369,7 @@ static void dwc3_gadget_reset_interrupt(struct dwc3
> *dwc)
>* transfers."
>*/
> dwc3_stop_active_transfers(dwc);
> + dwc->connected = true;
>
> reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> reg &= ~DWC3_DCTL_TSTCTRL_MASK;
>
Re: [PATCH v3 1/2] usb: dwc3: Trigger a GCTL soft reset when switching modes in DRD
Wesley Cheng wrote: > > On 3/6/2021 3:41 PM, Thinh Nguyen wrote: >> Wesley Cheng wrote: >>> On 1/8/2021 4:44 PM, Thinh Nguyen wrote: >>>> Hi, >>>> >>>> John Stultz wrote: >>>>> On Fri, Jan 8, 2021 at 4:26 AM Felipe Balbi wrote: >>>>>> Hi, >>>>>> >>>>>> John Stultz writes: >>>>>>> From: Yu Chen >>>>>>> >>>>>>> Just resending this, as discussion died out a bit and I'm not >>>>>>> sure how to make further progress. See here for debug data that >>>>>>> was requested last time around: >>>>>>> >>>>>>> https://urldefense.com/v3/__https://lore.kernel.org/lkml/calaqxlxdnaufjkx0an9xwwtfwvjmwigppy2aqsnj56yvnbu...@mail.gmail.com/__;!!A4F2R9G_pg!LNzuprAeg-O80SgolYkIkW4-ne-M-yLWCDUY9MygAIrQC398Z6gRJ9wnsnlqd3w$ >>>>>>> >>>>>>> >>>>>>> With the current dwc3 code on the HiKey960 we often see the >>>>>>> COREIDLE flag get stuck off in __dwc3_gadget_start(), which >>>>>>> seems to prevent the reset irq and causes the USB gadget to >>>>>>> fail to initialize. >>>>>>> >>>>>>> We had seen occasional initialization failures with older >>>>>>> kernels but with recent 5.x era kernels it seemed to be becoming >>>>>>> much more common, so I dug back through some older trees and >>>>>>> realized I dropped this quirk from Yu Chen during upstreaming >>>>>>> as I couldn't provide a proper rational for it and it didn't >>>>>>> seem to be necessary. I now realize I was wrong. >>>>>>> >>>>>>> After resubmitting the quirk, Thinh Nguyen pointed out that it >>>>>>> shouldn't be a quirk at all and it is actually mentioned in the >>>>>>> programming guide that it should be done when switching modes >>>>>>> in DRD. >>>>>>> >>>>>>> So, to avoid these !COREIDLE lockups seen on HiKey960, this >>>>>>> patch issues GCTL soft reset when switching modes if the >>>>>>> controller is in DRD mode. >>>>>>> >>>>>>> Cc: Felipe Balbi >>>>>>> Cc: Tejas Joglekar >>>>>>> Cc: Yang Fei >>>>>>> Cc: YongQin Liu >>>>>>> Cc: Andrzej Pietrasiewicz >>>>>>> Cc: Thinh Nguyen >>>>>>> Cc: Jun Li >>>>>>> Cc: Mauro Carvalho Chehab >>>>>>> Cc: Greg Kroah-Hartman >>>>>>> Cc: linux-...@vger.kernel.org >>>>>>> Signed-off-by: Yu Chen >>>>>>> Signed-off-by: John Stultz >>>>>>> --- >>>>>>> v2: >>>>>>> * Rework to always call the GCTL soft reset in DRD mode, >>>>>>> rather then using a quirk as suggested by Thinh Nguyen >>>>>>> >>>>>>> v3: >>>>>>> * Move GCTL soft reset under the spinlock as suggested by >>>>>>> Thinh Nguyen >>>>>> Because this is such an invasive change, I would prefer that we get >>>>>> Tested-By tags from a good fraction of the users before applying these >>>>>> two changes. >>>>> I'm happy to reach out to folks to try to get that. Though I'm >>>>> wondering if it would be better to put it behind a dts quirk flag, as >>>>> originally proposed? >>>>> >>>>> https://urldefense.com/v3/__https://lore.kernel.org/lkml/20201021181803.79650-1-john.stu...@linaro.org/__;!!A4F2R9G_pg!LNzuprAeg-O80SgolYkIkW4-ne-M-yLWCDUY9MygAIrQC398Z6gRJ9wnRWITZfc$ >>>>> >>>>> >>>>> That way folks can enable it for devices as they need? >>>>> >>>>> Again, I'm not trying to force this in as-is, just mostly sending it >>>>> out again for discussion to understand what other approach might work. >>>>> >>>>> thanks >>>>> -john >>>> A quirk would imply something is broken/diverged from the design right? >>>> But it's not the case here, and at least this is needed for HiKey960. >>>> Also, I think Rob will be ok with not adding 1 more quirk to the dwc3 >>>> devicetree. :) >>>> >>>> BR, >>>> Thinh >>>> >>> Hi All, >>> >>> Sorry for jumping in, but I checked the SNPS v1.90a databook, and that >>> seemed to remove the requirement for the GCTL.softreset before writing >>> to PRTCAPDIR. Should we consider adding a controller version/IP check? >>> >> Hi Wesley, >> >> From what I see in the v1.90a databook and others, the flow remains the >> same. I need to check internally, but I'm not aware of the change. >> > Hi Thinh, > > Hmmm, can you help check the register description for the PRTCAPDIR on > your v1.90a databook? (Table 1-19 Fields for Register: GCTL (Continued) > Pg73) When we compared the sequence in the description there to the > previous versions it removed the GCTL.softreset. If it still shows up > on yours, then maybe my v1.90a isn't the final version? > > Thanks > Wesley Cheng > Hi Wesley, Actually your IP version type may use the newer flow. Can you print your DWC3_VER_TYPE? I still need to verify internally to know which versions need the update if any. Thanks, Thinh
Re: [PATCH v3 1/2] usb: dwc3: Trigger a GCTL soft reset when switching modes in DRD
Wesley Cheng wrote: > > On 1/8/2021 4:44 PM, Thinh Nguyen wrote: >> Hi, >> >> John Stultz wrote: >>> On Fri, Jan 8, 2021 at 4:26 AM Felipe Balbi wrote: >>>> Hi, >>>> >>>> John Stultz writes: >>>>> From: Yu Chen >>>>> >>>>> Just resending this, as discussion died out a bit and I'm not >>>>> sure how to make further progress. See here for debug data that >>>>> was requested last time around: >>>>> >>>>> https://urldefense.com/v3/__https://lore.kernel.org/lkml/calaqxlxdnaufjkx0an9xwwtfwvjmwigppy2aqsnj56yvnbu...@mail.gmail.com/__;!!A4F2R9G_pg!LNzuprAeg-O80SgolYkIkW4-ne-M-yLWCDUY9MygAIrQC398Z6gRJ9wnsnlqd3w$ >>>>> >>>>> >>>>> With the current dwc3 code on the HiKey960 we often see the >>>>> COREIDLE flag get stuck off in __dwc3_gadget_start(), which >>>>> seems to prevent the reset irq and causes the USB gadget to >>>>> fail to initialize. >>>>> >>>>> We had seen occasional initialization failures with older >>>>> kernels but with recent 5.x era kernels it seemed to be becoming >>>>> much more common, so I dug back through some older trees and >>>>> realized I dropped this quirk from Yu Chen during upstreaming >>>>> as I couldn't provide a proper rational for it and it didn't >>>>> seem to be necessary. I now realize I was wrong. >>>>> >>>>> After resubmitting the quirk, Thinh Nguyen pointed out that it >>>>> shouldn't be a quirk at all and it is actually mentioned in the >>>>> programming guide that it should be done when switching modes >>>>> in DRD. >>>>> >>>>> So, to avoid these !COREIDLE lockups seen on HiKey960, this >>>>> patch issues GCTL soft reset when switching modes if the >>>>> controller is in DRD mode. >>>>> >>>>> Cc: Felipe Balbi >>>>> Cc: Tejas Joglekar >>>>> Cc: Yang Fei >>>>> Cc: YongQin Liu >>>>> Cc: Andrzej Pietrasiewicz >>>>> Cc: Thinh Nguyen >>>>> Cc: Jun Li >>>>> Cc: Mauro Carvalho Chehab >>>>> Cc: Greg Kroah-Hartman >>>>> Cc: linux-...@vger.kernel.org >>>>> Signed-off-by: Yu Chen >>>>> Signed-off-by: John Stultz >>>>> --- >>>>> v2: >>>>> * Rework to always call the GCTL soft reset in DRD mode, >>>>> rather then using a quirk as suggested by Thinh Nguyen >>>>> >>>>> v3: >>>>> * Move GCTL soft reset under the spinlock as suggested by >>>>> Thinh Nguyen >>>> Because this is such an invasive change, I would prefer that we get >>>> Tested-By tags from a good fraction of the users before applying these >>>> two changes. >>> I'm happy to reach out to folks to try to get that. Though I'm >>> wondering if it would be better to put it behind a dts quirk flag, as >>> originally proposed? >>> >>> https://urldefense.com/v3/__https://lore.kernel.org/lkml/20201021181803.79650-1-john.stu...@linaro.org/__;!!A4F2R9G_pg!LNzuprAeg-O80SgolYkIkW4-ne-M-yLWCDUY9MygAIrQC398Z6gRJ9wnRWITZfc$ >>> >>> >>> That way folks can enable it for devices as they need? >>> >>> Again, I'm not trying to force this in as-is, just mostly sending it >>> out again for discussion to understand what other approach might work. >>> >>> thanks >>> -john >> A quirk would imply something is broken/diverged from the design right? >> But it's not the case here, and at least this is needed for HiKey960. >> Also, I think Rob will be ok with not adding 1 more quirk to the dwc3 >> devicetree. :) >> >> BR, >> Thinh >> > Hi All, > > Sorry for jumping in, but I checked the SNPS v1.90a databook, and that > seemed to remove the requirement for the GCTL.softreset before writing > to PRTCAPDIR. Should we consider adding a controller version/IP check? > Hi Wesley, From what I see in the v1.90a databook and others, the flow remains the same. I need to check internally, but I'm not aware of the change. BR, Thinh
Re: [PATCH v3 2/2] usb: dwc3: Fix DRD mode change sequence following programming guide
Wesley Cheng wrote: > > On 1/7/2021 5:51 PM, John Stultz wrote: >> In reviewing the previous patch, Thinh Nguyen pointed out that >> the DRD mode change sequence should be like the following when >> switching from host -> device according to the programming guide >> (for all DRD IPs): >> 1. Reset controller with GCTL.CoreSoftReset >> 2. Set GCTL.PrtCapDir(device) >> 3. Soft reset with DCTL.CSftRst >> 4. Then follow up with the initializing registers sequence >> >> The current code does: >> a. Soft reset with DCTL.CSftRst on driver probe >> b. Reset controller with GCTL.CoreSoftReset (added in previous >>patch) >> c. Set GCTL.PrtCapDir(device) >> d. < missing DCTL.CSftRst > >> e. Then follow up with initializing registers sequence >> >> So this patch adds the DCTL.CSftRst soft reset that was currently >> missing from the dwc3 mode switching. >> >> Cc: Felipe Balbi >> Cc: Tejas Joglekar >> Cc: Yang Fei >> Cc: YongQin Liu >> Cc: Andrzej Pietrasiewicz >> Cc: Thinh Nguyen >> Cc: Jun Li >> Cc: Mauro Carvalho Chehab >> Cc: Greg Kroah-Hartman >> Cc: linux-...@vger.kernel.org >> Signed-off-by: John Stultz >> --- >> Feedback would be appreciated. I'm a little worried I should be >> conditionalizing the DCTL.CSftRst on DRD mode controllers, but >> I'm really not sure what the right thing to do is for non-DRD >> mode controllers. >> --- >> drivers/usb/dwc3/core.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index b6a6b90eb2d5..71f8b07ecb99 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -40,6 +40,8 @@ >> >> #define DWC3_DEFAULT_AUTOSUSPEND_DELAY 5000 /* ms */ >> >> +static int dwc3_core_soft_reset(struct dwc3 *dwc); >> + >> /** >> * dwc3_get_dr_mode - Validates and sets dr_mode >> * @dwc: pointer to our context structure >> @@ -177,6 +179,7 @@ static void __dwc3_set_mode(struct work_struct *work) >> >> dwc3_set_prtcap(dwc, dwc->desired_dr_role); >> >> +dwc3_core_soft_reset(dwc); > Hi John/Thinh/Felipe, > > I actually added this change into my local branch, because we were > seeing an issue when switching from host mode --> peripheral mode. What > was happening was that the RXFIFO register did not update back to the > expected value for peripheral mode by the time > dwc3_gadget_init_out_endpoint() was executed. With the logic to > calculate the EP max packet limit based on RXFIFO reg, this caused all > EPs to be set with an EP max limit of 0. > > With this change, it seemed to help with the above issue. However, can > we consider moving the core soft reset outside the spinlock? At least > with our PHY init routines, we have some msleep() calls for waiting for > the PHYs to be ready, which will end up as a sleeping while atomic bug. > (not sure if PHY init is required to be called in atomic context) > > Thanks > Wesley Cheng Hi Wesley, Thanks for letting us know the issue you're having also. Yes, you need to wait a certain amount of time to synchronize with the PHY (at least 50ms for dwc_usb32 and dwc_usb31 v1.80a and above, and less for older versions). When removing the spinlock to use msleep(), just make sure that there's no race issue. BTW, how long does your setup need to msleep()? As we suggested and explained before, this change is required as part of the programming guide. It should be added to the driver. It's not a quirk. BR, Thinh
Re: [PATCH v6 3/4] usb: dwc3: Resize TX FIFOs to meet EP bursting requirements
Wesley Cheng wrote:
>
> On 1/26/2021 12:43 PM, Thinh Nguyen wrote:
>> Wesley Cheng wrote:
>>> On 1/22/2021 4:15 PM, Thinh Nguyen wrote:
>>>> Hi,
>>>>
>>>> Wesley Cheng wrote:
>>>>> Some devices have USB compositions which may require multiple endpoints
>>>>> that support EP bursting. HW defined TX FIFO sizes may not always be
>>>>> sufficient for these compositions. By utilizing flexible TX FIFO
>>>>> allocation, this allows for endpoints to request the required FIFO depth
>>>>> to
>>>>> achieve higher bandwidth. With some higher bMaxBurst configurations,
>>>>> using
>>>>> a larger TX FIFO size results in better TX throughput.
>>>>>
>>>>> By introducing the check_config() callback, the resizing logic can fetch
>>>>> the maximum number of endpoints used in the USB composition (can contain
>>>>> multiple configurations), which helps ensure that the resizing logic can
>>>>> fulfill the configuration(s), or return an error to the gadget layer
>>>>> otherwise during bind time.
>>>>>
>>>>> Signed-off-by: Wesley Cheng
>>>>> ---
>>>>> drivers/usb/dwc3/core.c | 2 +
>>>>> drivers/usb/dwc3/core.h | 8 ++
>>>>> drivers/usb/dwc3/ep0.c| 2 +
>>>>> drivers/usb/dwc3/gadget.c | 194
>>>>> ++
>>>>> 4 files changed, 206 insertions(+)
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>> index 6969196..e7fa6af 100644
>>>>> --- a/drivers/usb/dwc3/core.c
>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>> @@ -1284,6 +1284,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>>>> _thr_num_pkt_prd);
>>>>> device_property_read_u8(dev, "snps,tx-max-burst-prd",
>>>>> _max_burst_prd);
>>>>> + dwc->needs_fifo_resize = device_property_read_bool(dev,
>>>>> +"tx-fifo-resize");
>>>>>
>>>>> dwc->disable_scramble_quirk = device_property_read_bool(dev,
>>>>> "snps,disable_scramble_quirk");
>>>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>>>> index eec1cf4..983b2fd4 100644
>>>>> --- a/drivers/usb/dwc3/core.h
>>>>> +++ b/drivers/usb/dwc3/core.h
>>>>> @@ -1223,6 +1223,7 @@ struct dwc3 {
>>>>> unsignedis_utmi_l1_suspend:1;
>>>>> unsignedis_fpga:1;
>>>>> unsignedpending_events:1;
>>>>> + unsignedneeds_fifo_resize:1;
>>>> The prefix "need" sounds like a requirement, but I don't think it is the
>>>> case here. I think "do" would be a better prefix here.
>>>>
>>> Hi Thinh,
>>>
>>> Sure, that is true, since this may be an optional flag for certain
>>> platforms.
>>>
>>>>> unsignedpullups_connected:1;
>>>>> unsignedsetup_packet_pending:1;
>>>>> unsignedthree_stage_setup:1;
>>>>> @@ -1257,6 +1258,10 @@ struct dwc3 {
>>>>> unsigneddis_split_quirk:1;
>>>>>
>>>>> u16 imod_interval;
>>>>> +
>>>>> + int max_cfg_eps;
>>>>> + int last_fifo_depth;
>>>>> + int num_ep_resized;
>>>>> };
>>>> Please document these new fields.
>>>>
>>> Will do.
>>>
>>>>>
>>>>> #define INCRX_BURST_MODE 0
>>>>> @@ -1471,6 +1476,7 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep,
>>>>> unsigned int cmd,
>>>>> struct dwc3_gadget_ep_cmd_params *params);
>>>>> int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned int cmd,
>>>>> u32 param);
>>>>> +void dwc3_gadget_clear_tx_fifos(struct dwc3 *dwc);
>>>>> #else
>>>>> static inline int dwc3_gadget_init(struct dwc3 *dwc)
>>>>> { return 0; }
>>>>> @@ -1490,6 +1496,8 @@ static inl
Re: [PATCH v6 3/4] usb: dwc3: Resize TX FIFOs to meet EP bursting requirements
Wesley Cheng wrote:
>
> On 1/22/2021 4:15 PM, Thinh Nguyen wrote:
>> Hi,
>>
>> Wesley Cheng wrote:
>>> Some devices have USB compositions which may require multiple endpoints
>>> that support EP bursting. HW defined TX FIFO sizes may not always be
>>> sufficient for these compositions. By utilizing flexible TX FIFO
>>> allocation, this allows for endpoints to request the required FIFO depth to
>>> achieve higher bandwidth. With some higher bMaxBurst configurations, using
>>> a larger TX FIFO size results in better TX throughput.
>>>
>>> By introducing the check_config() callback, the resizing logic can fetch
>>> the maximum number of endpoints used in the USB composition (can contain
>>> multiple configurations), which helps ensure that the resizing logic can
>>> fulfill the configuration(s), or return an error to the gadget layer
>>> otherwise during bind time.
>>>
>>> Signed-off-by: Wesley Cheng
>>> ---
>>> drivers/usb/dwc3/core.c | 2 +
>>> drivers/usb/dwc3/core.h | 8 ++
>>> drivers/usb/dwc3/ep0.c| 2 +
>>> drivers/usb/dwc3/gadget.c | 194
>>> ++
>>> 4 files changed, 206 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index 6969196..e7fa6af 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -1284,6 +1284,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>> _thr_num_pkt_prd);
>>> device_property_read_u8(dev, "snps,tx-max-burst-prd",
>>> _max_burst_prd);
>>> + dwc->needs_fifo_resize = device_property_read_bool(dev,
>>> + "tx-fifo-resize");
>>>
>>> dwc->disable_scramble_quirk = device_property_read_bool(dev,
>>> "snps,disable_scramble_quirk");
>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>> index eec1cf4..983b2fd4 100644
>>> --- a/drivers/usb/dwc3/core.h
>>> +++ b/drivers/usb/dwc3/core.h
>>> @@ -1223,6 +1223,7 @@ struct dwc3 {
>>> unsignedis_utmi_l1_suspend:1;
>>> unsignedis_fpga:1;
>>> unsignedpending_events:1;
>>> + unsignedneeds_fifo_resize:1;
>> The prefix "need" sounds like a requirement, but I don't think it is the
>> case here. I think "do" would be a better prefix here.
>>
> Hi Thinh,
>
> Sure, that is true, since this may be an optional flag for certain
> platforms.
>
>>> unsignedpullups_connected:1;
>>> unsignedsetup_packet_pending:1;
>>> unsignedthree_stage_setup:1;
>>> @@ -1257,6 +1258,10 @@ struct dwc3 {
>>> unsigneddis_split_quirk:1;
>>>
>>> u16 imod_interval;
>>> +
>>> + int max_cfg_eps;
>>> + int last_fifo_depth;
>>> + int num_ep_resized;
>>> };
>> Please document these new fields.
>>
> Will do.
>
>>>
>>> #define INCRX_BURST_MODE 0
>>> @@ -1471,6 +1476,7 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep,
>>> unsigned int cmd,
>>> struct dwc3_gadget_ep_cmd_params *params);
>>> int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned int cmd,
>>> u32 param);
>>> +void dwc3_gadget_clear_tx_fifos(struct dwc3 *dwc);
>>> #else
>>> static inline int dwc3_gadget_init(struct dwc3 *dwc)
>>> { return 0; }
>>> @@ -1490,6 +1496,8 @@ static inline int dwc3_send_gadget_ep_cmd(struct
>>> dwc3_ep *dep, unsigned int cmd,
>>> static inline int dwc3_send_gadget_generic_command(struct dwc3 *dwc,
>>> int cmd, u32 param)
>>> { return 0; }
>>> +static inline void dwc3_gadget_clear_tx_fifos(struct dwc3 *dwc)
>>> +{ }
>>> #endif
>>>
>>> #if IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
>>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>>> index 8b668ef..4f216bd 100644
>>> --- a/drivers/usb/dwc3/ep0.c
>>> +++ b/drivers/usb/dwc3/ep0.c
>>> @@ -616,6 +616,8 @@ static int dwc3_ep0_set_config(struct dwc3 *dwc, struct
>>>
Re: [PATCH v6 3/4] usb: dwc3: Resize TX FIFOs to meet EP bursting requirements
Hi,
Wesley Cheng wrote:
> Some devices have USB compositions which may require multiple endpoints
> that support EP bursting. HW defined TX FIFO sizes may not always be
> sufficient for these compositions. By utilizing flexible TX FIFO
> allocation, this allows for endpoints to request the required FIFO depth to
> achieve higher bandwidth. With some higher bMaxBurst configurations, using
> a larger TX FIFO size results in better TX throughput.
>
> By introducing the check_config() callback, the resizing logic can fetch
> the maximum number of endpoints used in the USB composition (can contain
> multiple configurations), which helps ensure that the resizing logic can
> fulfill the configuration(s), or return an error to the gadget layer
> otherwise during bind time.
>
> Signed-off-by: Wesley Cheng
> ---
> drivers/usb/dwc3/core.c | 2 +
> drivers/usb/dwc3/core.h | 8 ++
> drivers/usb/dwc3/ep0.c| 2 +
> drivers/usb/dwc3/gadget.c | 194
> ++
> 4 files changed, 206 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 6969196..e7fa6af 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1284,6 +1284,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
> _thr_num_pkt_prd);
> device_property_read_u8(dev, "snps,tx-max-burst-prd",
> _max_burst_prd);
> + dwc->needs_fifo_resize = device_property_read_bool(dev,
> +"tx-fifo-resize");
>
> dwc->disable_scramble_quirk = device_property_read_bool(dev,
> "snps,disable_scramble_quirk");
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index eec1cf4..983b2fd4 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1223,6 +1223,7 @@ struct dwc3 {
> unsignedis_utmi_l1_suspend:1;
> unsignedis_fpga:1;
> unsignedpending_events:1;
> + unsignedneeds_fifo_resize:1;
The prefix "need" sounds like a requirement, but I don't think it is the
case here. I think "do" would be a better prefix here.
> unsignedpullups_connected:1;
> unsignedsetup_packet_pending:1;
> unsignedthree_stage_setup:1;
> @@ -1257,6 +1258,10 @@ struct dwc3 {
> unsigneddis_split_quirk:1;
>
> u16 imod_interval;
> +
> + int max_cfg_eps;
> + int last_fifo_depth;
> + int num_ep_resized;
> };
Please document these new fields.
>
> #define INCRX_BURST_MODE 0
> @@ -1471,6 +1476,7 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep,
> unsigned int cmd,
> struct dwc3_gadget_ep_cmd_params *params);
> int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned int cmd,
> u32 param);
> +void dwc3_gadget_clear_tx_fifos(struct dwc3 *dwc);
> #else
> static inline int dwc3_gadget_init(struct dwc3 *dwc)
> { return 0; }
> @@ -1490,6 +1496,8 @@ static inline int dwc3_send_gadget_ep_cmd(struct
> dwc3_ep *dep, unsigned int cmd,
> static inline int dwc3_send_gadget_generic_command(struct dwc3 *dwc,
> int cmd, u32 param)
> { return 0; }
> +static inline void dwc3_gadget_clear_tx_fifos(struct dwc3 *dwc)
> +{ }
> #endif
>
> #if IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 8b668ef..4f216bd 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -616,6 +616,8 @@ static int dwc3_ep0_set_config(struct dwc3 *dwc, struct
> usb_ctrlrequest *ctrl)
> return -EINVAL;
>
> case USB_STATE_ADDRESS:
> + dwc3_gadget_clear_tx_fifos(dwc);
> +
> ret = dwc3_ep0_delegate_req(dwc, ctrl);
> /* if the cfg matches and the cfg is non zero */
> if (cfg && (!ret || (ret == USB_GADGET_DELAYED_STATUS))) {
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 86f257f..26f9d64 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -615,6 +615,161 @@ static int dwc3_gadget_set_ep_config(struct dwc3_ep
> *dep, unsigned int action)
> static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
> bool interrupt);
>
> +static int dwc3_gadget_calc_tx_fifo_size(struct dwc3 *dwc, int mult)
Can you document what this function does?
> +{
> + int max_packet = 1024;
Maybe you can also document why you chose 1024 (e.g. applicable to
Enhanced SuperSpeed only?).
> + int fifo_size;
> + int mdwidth;
> +
> + mdwidth = DWC3_MDWIDTH(dwc->hwparams.hwparams0);
> + /* MDWIDTH is represented in bits, we need it in bytes */
> + mdwidth >>= 3;
mdwidth for DWC32 requires to read hwparams6
Re: usb: dwc3: gadget: skip pullup and set_speed after suspend
Hi, Daehwan Jung wrote: > Sometimes dwc3_gadget_pullup and dwc3_gadget_set_speed are called after > entering suspend. That's why it needs to check whether suspend > > 1. dwc3 sends disconnect uevent and turn off. (suspend) > 2. Platform side causes pullup or set_speed(e.g., adbd closes ffs node) > 3. It causes unexpected behavior like ITMON error. > > Signed-off-by: Daehwan Jung > --- > drivers/usb/dwc3/gadget.c | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index ee44321..d7d4202 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -2093,6 +2093,9 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int > is_on) > unsigned long flags; > int ret; > > + if (pm_runtime_suspended(dwc->dev)) > + return 0; > + > is_on = !!is_on; > > /* > @@ -2403,6 +2406,9 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g, > unsigned long flags; > u32 reg; > > + if (pm_runtime_suspended(dwc->dev)) > + return; > + > spin_lock_irqsave(>lock, flags); > reg = dwc3_readl(dwc->regs, DWC3_DCFG); > reg &= ~(DWC3_DCFG_SPEED_MASK); This is already addressed in Wesley Cheng's patches. Can you try the latest changes of DWC3 in Greg's usb-next branch? Thanks, Thinh
Re: [PATCH v3 1/2] usb: dwc3: Trigger a GCTL soft reset when switching modes in DRD
Hi, John Stultz wrote: > On Fri, Jan 8, 2021 at 4:26 AM Felipe Balbi wrote: >> >> Hi, >> >> John Stultz writes: >>> From: Yu Chen >>> >>> Just resending this, as discussion died out a bit and I'm not >>> sure how to make further progress. See here for debug data that >>> was requested last time around: >>> >>> https://urldefense.com/v3/__https://lore.kernel.org/lkml/calaqxlxdnaufjkx0an9xwwtfwvjmwigppy2aqsnj56yvnbu...@mail.gmail.com/__;!!A4F2R9G_pg!LNzuprAeg-O80SgolYkIkW4-ne-M-yLWCDUY9MygAIrQC398Z6gRJ9wnsnlqd3w$ >>> >>> >>> With the current dwc3 code on the HiKey960 we often see the >>> COREIDLE flag get stuck off in __dwc3_gadget_start(), which >>> seems to prevent the reset irq and causes the USB gadget to >>> fail to initialize. >>> >>> We had seen occasional initialization failures with older >>> kernels but with recent 5.x era kernels it seemed to be becoming >>> much more common, so I dug back through some older trees and >>> realized I dropped this quirk from Yu Chen during upstreaming >>> as I couldn't provide a proper rational for it and it didn't >>> seem to be necessary. I now realize I was wrong. >>> >>> After resubmitting the quirk, Thinh Nguyen pointed out that it >>> shouldn't be a quirk at all and it is actually mentioned in the >>> programming guide that it should be done when switching modes >>> in DRD. >>> >>> So, to avoid these !COREIDLE lockups seen on HiKey960, this >>> patch issues GCTL soft reset when switching modes if the >>> controller is in DRD mode. >>> >>> Cc: Felipe Balbi >>> Cc: Tejas Joglekar >>> Cc: Yang Fei >>> Cc: YongQin Liu >>> Cc: Andrzej Pietrasiewicz >>> Cc: Thinh Nguyen >>> Cc: Jun Li >>> Cc: Mauro Carvalho Chehab >>> Cc: Greg Kroah-Hartman >>> Cc: linux-...@vger.kernel.org >>> Signed-off-by: Yu Chen >>> Signed-off-by: John Stultz >>> --- >>> v2: >>> * Rework to always call the GCTL soft reset in DRD mode, >>> rather then using a quirk as suggested by Thinh Nguyen >>> >>> v3: >>> * Move GCTL soft reset under the spinlock as suggested by >>> Thinh Nguyen >> Because this is such an invasive change, I would prefer that we get >> Tested-By tags from a good fraction of the users before applying these >> two changes. > I'm happy to reach out to folks to try to get that. Though I'm > wondering if it would be better to put it behind a dts quirk flag, as > originally proposed? > > https://urldefense.com/v3/__https://lore.kernel.org/lkml/20201021181803.79650-1-john.stu...@linaro.org/__;!!A4F2R9G_pg!LNzuprAeg-O80SgolYkIkW4-ne-M-yLWCDUY9MygAIrQC398Z6gRJ9wnRWITZfc$ > > > That way folks can enable it for devices as they need? > > Again, I'm not trying to force this in as-is, just mostly sending it > out again for discussion to understand what other approach might work. > > thanks > -john A quirk would imply something is broken/diverged from the design right? But it's not the case here, and at least this is needed for HiKey960. Also, I think Rob will be ok with not adding 1 more quirk to the dwc3 devicetree. :) BR, Thinh
Re: [PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect
gre...@linuxfoundation.org wrote:
> On Fri, Jan 08, 2021 at 02:19:30AM +0000, Thinh Nguyen wrote:
>> Hi Wesley,
>>
>> Felipe Balbi wrote:
>>> Hi,
>>>
>>> Wesley Cheng writes:
>>>> +void composite_reset(struct usb_gadget *gadget)
>>>> +{
>>>> + /*
>>>> + * Section 1.4.13 Standard Downstream Port of the USB battery charging
>>>> + * specification v1.2 states that a device connected on a SDP shall only
>>>> + * draw at max 100mA while in a connected, but unconfigured state.
>>> The requirements are different, though. I think OTG spec has some extra
>>> requirements where only 8mA can be drawn max. You need to check for the
>>> otg flag. Moreover, USB3+ spec has units of 150mA meaning the device
>>> can't draw 100mA (IIRC).
>>>
>> We see issue with this patch series. For our device running at SSP, the
>> device couldn't recover from a port reset and remained in eSS.Inactive
>> state.
>>
>> This patch series is already in Greg's usb-testing. Please review and
>> help fix it.
> Should I just revert this? I'll be glad to drop it.
>
> thanks,
>
> greg k-h
Unless there's some other issue, let's not do so as it seems to be
related to my HW stability than any SW issue. Sorry for the noise.
BR,
Thinh
Re: [PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect
Jack Pham wrote:
> Hi Thinh,
>
> On Fri, Jan 08, 2021 at 02:19:30AM +0000, Thinh Nguyen wrote:
>> Hi Wesley,
>>
>> Felipe Balbi wrote:
>>> Hi,
>>>
>>> Wesley Cheng writes:
>>>> +void composite_reset(struct usb_gadget *gadget)
>>>> +{
>>>> + /*
>>>> + * Section 1.4.13 Standard Downstream Port of the USB battery charging
>>>> + * specification v1.2 states that a device connected on a SDP shall only
>>>> + * draw at max 100mA while in a connected, but unconfigured state.
>>> The requirements are different, though. I think OTG spec has some extra
>>> requirements where only 8mA can be drawn max. You need to check for the
>>> otg flag. Moreover, USB3+ spec has units of 150mA meaning the device
>>> can't draw 100mA (IIRC).
>>>
>> We see issue with this patch series. For our device running at SSP, the
>> device couldn't recover from a port reset and remained in eSS.Inactive
>> state.
>>
>> This patch series is already in Greg's usb-testing. Please review and
>> help fix it.
>>
>> We can see the failure once the patch "usb: gadget: configfs: Add a
>> specific configFS reset callback" is introduced.
> Hmm. Does your device use a legacy USB2 PHY (not generic PHY) driver,
> and does it implement the usb_phy_set_power() callback? Because
> otherwise this new configfs_composite_reset() callback would not have
> changed from the original behavior since the newly introduced (in patch
> 1/3) dwc3_gadget_vbus_draw() callback would simply be a no-op if
> dwc->usb2_phy is not present.
>
> If it does turn out to be something with your PHY driver's set_power(),
> it's still puzzling since it's directed to only the usb2_phy, so I'm
> curious how telling it to draw 100mA could affect SSP operation at all.
>
> Thanks,
> Jack
So, I ran some more tests. It seems like this new change affects some
timing in my setup that triggers this failure. I tried to add some
printouts to look into it further, but somehow that reduces the failure
rate significantly. This doesn't seem related to power drawing as it
doesn't update usb2_phy for SSP as you said.
After switching my HW setup, the problem seems to go away. I guess we
can ignore this since the code path looks to be the same as previous.
BR,
Thinh
Re: [PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect
Hi Wesley,
Felipe Balbi wrote:
> Hi,
>
> Wesley Cheng writes:
>> +void composite_reset(struct usb_gadget *gadget)
>> +{
>> +/*
>> + * Section 1.4.13 Standard Downstream Port of the USB battery charging
>> + * specification v1.2 states that a device connected on a SDP shall only
>> + * draw at max 100mA while in a connected, but unconfigured state.
> The requirements are different, though. I think OTG spec has some extra
> requirements where only 8mA can be drawn max. You need to check for the
> otg flag. Moreover, USB3+ spec has units of 150mA meaning the device
> can't draw 100mA (IIRC).
>
We see issue with this patch series. For our device running at SSP, the
device couldn't recover from a port reset and remained in eSS.Inactive
state.
This patch series is already in Greg's usb-testing. Please review and
help fix it.
We can see the failure once the patch "usb: gadget: configfs: Add a
specific configFS reset callback" is introduced.
Thanks,
Thinh
Re: [PATCH v2] usb: dwc3: Trigger a GCTL soft reset when switching modes in DRD
Hi,
Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen writes:
>> John Stultz wrote:
>>> static void __dwc3_set_mode(struct work_struct *work)
>>> {
>>> struct dwc3 *dwc = work_to_dwc(work);
>>> unsigned long flags;
>>> + int hw_mode;
>>> int ret;
>>> u32 reg;
>>>
>>> @@ -154,6 +168,11 @@ static void __dwc3_set_mode(struct work_struct *work)
>>> break;
>>> }
>>>
>>> + /* Execute a GCTL Core Soft Reset when switch mode in DRD*/
>>> + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
>>> + if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD)
>>> + dwc3_gctl_core_soft_reset(dwc);
>>> +
>> I think this should be done inside the spin_lock.
>>
>>> spin_lock_irqsave(>lock, flags);
>>>
>>> dwc3_set_prtcap(dwc, dwc->desired_dr_role);
>> The DRD mode change sequence should be like this if we want to switch
>> from host -> device according to the programming guide (for all DRD IPs):
>> 1. Reset controller with GCTL.CoreSoftReset
>> 2. Set GCTL.PrtCapDir(device)
>> 3. Soft reset with DCTL.CSftRst
>> 4. Then follow up with the initializing registers sequence
>>
>> However, from code review, with this patch, it follows this sequence:
>> a. Soft reset with DCTL.CSftRst on driver probe
>> b. Reset controller with GCTL.CoreSoftReset
>> c. Set GCTL.PrtCapDir(device)
>> d. < missing DCTL.CSftRst >
>> e. Then follow up with initializing registers sequence
>>
>> It may work, but it doesn't follow the programming guide.
>>
>> For device -> host, it should be fine because the xHCI driver will do
>> USBCMD.HCRST during initialization.
> The only reason why this is needed is because SNPS saves some die area
> by mapping some host and peripheral register to the same physical area
> in the die. I still think a full soft reset is unnecessary as we have
> been running this driver without that soft reset for several years now.
>
This isn't about whether to use GCTL or DCTL for Core Soft Reset (Please
correct me if I'm wrong because I think this is what you're referring
to). It's about the programming flow when switching modes.
Both step 1 and 3 are required. Because before step 1, if the host was
in normal working mode (e.g. attached to a device), then changing the
PrtCapDir directly to device mode is not advisable and HW may exhibit
unknown behavior. The proper way is to have step 1 through 4 in
sequence. Step 3 is required because some of the HW functionality is
dependent on the PrtCapDir setting before the driver intervention is
required for proper communication with the device. The HW may be in some
intermediate state while changing the PrtCapDir. So, it is required to
reset it to have a fresh start in device mode.
Though we may not see issues even if we eliminate the steps 1 and 3, it
is not advisable and we may have some impact under certain conditions.
One change needs to made for this patch is the driver needs to wait a
certain amount of time before clearing the GCTL.CoreSoftReset for the
PHY clock to start and stabilize.
BR,
Thinh
Re: [PATCH 2/2] usb: dwc3: gadget: Preserve UDC max speed setting
Wesley Cheng wrote:
>
> On 10/28/2020 5:43 PM, Thinh Nguyen wrote:
>> Hi,
>>
>> Wesley Cheng wrote:
>>> The USB gadget/UDC driver can restrict the DWC3 controller speed using
>>> dwc3_gadget_set_speed(). Store this setting into a variable, in order for
>>> this setting to persist across controller resets due to runtime PM.
>> Why do we need to do this? DCFG should persist unless we overwrite it.
>> The current PM shouldn't update the current speed setting.
>>
>> BR,
>> Thinh
>>
> Hi Thinh,
>
> During runtime PM suspend, the dwc3_suspend_common() will call
> dwc3_core_exit(). On some platforms they register the DWC3 reset
> control to the DWC3 core driver (otherwise could be handled in the DWC3
> glue drivers), which will be asserted here:
>
> static void dwc3_core_exit(struct dwc3 *dwc)
> {
> ...
> reset_control_assert(dwc->reset);
>
> From the SNPS databook (Table 2-2 Resets for Registers) it mentions that
> assertion of the reset signal will reset the DCFG register.
I see. There's a hard reset on some platforms.
>
> In addition to the above, with the change to allow runtime PM suspend
> during UDC unbind, we need a way to avoid writing to the DCFG during the
> UDC bind path. (if we entered suspend before re-binding the UDC) If we
> add an early exit based on the PM state (in
> dwc3_gadget_set_udc_speed()), then we basically ignore the max speed
> request from the UDC/gadget layer.
Then shouldn't we restore the speed setting when dwc3_gadget_resume()
instead of in dwc3_gadget_run_stop()?
>
> Since it looks like the DWC3 gadget driver doesn't like using
> synchronous PM runtime resumes, by going this route, we can allow the
> async runtime resume handler to do everything, such as writing the speed
> config and re-enabling the controller.
>
> Thanks
>
> Regards,
> Wesley Cheng
Thanks,
Thinh
Re: [PATCH 2/2] usb: dwc3: gadget: Preserve UDC max speed setting
Thinh Nguyen wrote:
> Wesley Cheng wrote:
>> On 10/28/2020 5:43 PM, Thinh Nguyen wrote:
>>> Hi,
>>>
>>> Wesley Cheng wrote:
>>>> The USB gadget/UDC driver can restrict the DWC3 controller speed using
>>>> dwc3_gadget_set_speed(). Store this setting into a variable, in order for
>>>> this setting to persist across controller resets due to runtime PM.
>>> Why do we need to do this? DCFG should persist unless we overwrite it.
>>> The current PM shouldn't update the current speed setting.
>>>
>>> BR,
>>> Thinh
>>>
>> Hi Thinh,
>>
>> During runtime PM suspend, the dwc3_suspend_common() will call
>> dwc3_core_exit(). On some platforms they register the DWC3 reset
>> control to the DWC3 core driver (otherwise could be handled in the DWC3
>> glue drivers), which will be asserted here:
>>
>> static void dwc3_core_exit(struct dwc3 *dwc)
>> {
>> ...
>> reset_control_assert(dwc->reset);
>>
>> From the SNPS databook (Table 2-2 Resets for Registers) it mentions that
>> assertion of the reset signal will reset the DCFG register.
> I see. There's a hard reset on some platforms.
>
>> In addition to the above, with the change to allow runtime PM suspend
>> during UDC unbind, we need a way to avoid writing to the DCFG during the
>> UDC bind path. (if we entered suspend before re-binding the UDC) If we
>> add an early exit based on the PM state (in
>> dwc3_gadget_set_udc_speed()), then we basically ignore the max speed
>> request from the UDC/gadget layer.
> Then shouldn't we restore the speed setting when dwc3_gadget_resume()
> instead of in dwc3_gadget_run_stop()?
Actually, ignore this comment. This is fine, and it may save some
register read/write operations. I was thinking of save/restore from
suspend and resume similar to hibernation.
Thanks,
Thinh
>
>> Since it looks like the DWC3 gadget driver doesn't like using
>> synchronous PM runtime resumes, by going this route, we can allow the
>> async runtime resume handler to do everything, such as writing the speed
>> config and re-enabling the controller.
>>
>> Thanks
>>
>> Regards,
>> Wesley Cheng
> Thanks,
> Thinh
Re: [PATCH 2/2] usb: dwc3: gadget: Preserve UDC max speed setting
Hi,
Wesley Cheng wrote:
> The USB gadget/UDC driver can restrict the DWC3 controller speed using
> dwc3_gadget_set_speed(). Store this setting into a variable, in order for
> this setting to persist across controller resets due to runtime PM.
Why do we need to do this? DCFG should persist unless we overwrite it.
The current PM shouldn't update the current speed setting.
BR,
Thinh
> Signed-off-by: Wesley Cheng
> ---
> drivers/usb/dwc3/core.h | 1 +
> drivers/usb/dwc3/gadget.c | 108 --
> 2 files changed, 58 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 2f04b3e42bf1..390d3deef0ba 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1119,6 +1119,7 @@ struct dwc3 {
> u32 nr_scratch;
> u32 u1u2;
> u32 maximum_speed;
> + u32 gadget_max_speed;
>
> u32 ip;
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 6364429b2122..1a93b92a5e6f 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1941,6 +1941,61 @@ static void dwc3_stop_active_transfers(struct dwc3
> *dwc)
> }
> }
>
> +static void __dwc3_gadget_set_speed(struct dwc3 *dwc)
> +{
> + u32 reg;
> +
> + reg = dwc3_readl(dwc->regs, DWC3_DCFG);
> + reg &= ~(DWC3_DCFG_SPEED_MASK);
> +
> + /*
> + * WORKAROUND: DWC3 revision < 2.20a have an issue
> + * which would cause metastability state on Run/Stop
> + * bit if we try to force the IP to USB2-only mode.
> + *
> + * Because of that, we cannot configure the IP to any
> + * speed other than the SuperSpeed
> + *
> + * Refers to:
> + *
> + * STAR#9000525659: Clock Domain Crossing on DCTL in
> + * USB 2.0 Mode
> + */
> + if (DWC3_VER_IS_PRIOR(DWC3, 220A) &&
> + !dwc->dis_metastability_quirk) {
> + reg |= DWC3_DCFG_SUPERSPEED;
> + } else {
> + switch (dwc->gadget_max_speed) {
> + case USB_SPEED_LOW:
> + reg |= DWC3_DCFG_LOWSPEED;
> + break;
> + case USB_SPEED_FULL:
> + reg |= DWC3_DCFG_FULLSPEED;
> + break;
> + case USB_SPEED_HIGH:
> + reg |= DWC3_DCFG_HIGHSPEED;
> + break;
> + case USB_SPEED_SUPER:
> + reg |= DWC3_DCFG_SUPERSPEED;
> + break;
> + case USB_SPEED_SUPER_PLUS:
> + if (DWC3_IP_IS(DWC3))
> + reg |= DWC3_DCFG_SUPERSPEED;
> + else
> + reg |= DWC3_DCFG_SUPERSPEED_PLUS;
> + break;
> + default:
> + dev_err(dwc->dev, "invalid speed (%d)\n",
> dwc->gadget_max_speed);
> +
> + if (DWC3_IP_IS(DWC3))
> + reg |= DWC3_DCFG_SUPERSPEED;
> + else
> + reg |= DWC3_DCFG_SUPERSPEED_PLUS;
> + }
> + }
> + dwc3_writel(dwc->regs, DWC3_DCFG, reg);
> +}
> +
> static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
> {
> u32 reg;
> @@ -1963,6 +2018,7 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int
> is_on, int suspend)
> if (dwc->has_hibernation)
> reg |= DWC3_DCTL_KEEP_CONNECT;
>
> + __dwc3_gadget_set_speed(dwc);
> dwc->pullups_connected = true;
> } else {
> reg &= ~DWC3_DCTL_RUN_STOP;
> @@ -2318,59 +2374,9 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g,
> {
> struct dwc3 *dwc = gadget_to_dwc(g);
> unsigned long flags;
> - u32 reg;
>
> spin_lock_irqsave(>lock, flags);
> - reg = dwc3_readl(dwc->regs, DWC3_DCFG);
> - reg &= ~(DWC3_DCFG_SPEED_MASK);
> -
> - /*
> - * WORKAROUND: DWC3 revision < 2.20a have an issue
> - * which would cause metastability state on Run/Stop
> - * bit if we try to force the IP to USB2-only mode.
> - *
> - * Because of that, we cannot configure the IP to any
> - * speed other than the SuperSpeed
> - *
> - * Refers to:
> - *
> - * STAR#9000525659: Clock Domain Crossing on DCTL in
> - * USB 2.0 Mode
> - */
> - if (DWC3_VER_IS_PRIOR(DWC3, 220A) &&
> - !dwc->dis_metastability_quirk) {
> - reg |= DWC3_DCFG_SUPERSPEED;
> - } else {
> - switch (speed) {
> - case USB_SPEED_LOW:
> - reg |= DWC3_DCFG_LOWSPEED;
> - break;
> - case USB_SPEED_FULL:
> - reg |= DWC3_DCFG_FULLSPEED;
> -
Re: [PATCH v2] usb: dwc3: Trigger a GCTL soft reset when switching modes in DRD
John Stultz wrote:
> From: Yu Chen
>
> With the current dwc3 code on the HiKey960 we often see the
> COREIDLE flag get stuck off in __dwc3_gadget_start(), which
> seems to prevent the reset irq and causes the USB gadget to
> fail to initialize.
>
> We had seen occasional initialization failures with older
> kernels but with recent 5.x era kernels it seemed to be becoming
> much more common, so I dug back through some older trees and
> realized I dropped this quirk from Yu Chen during upstreaming
> as I couldn't provide a proper rational for it and it didn't
> seem to be necessary. I now realize I was wrong.
>
> After resubmitting the quirk Thinh Nguyen pointed out that it
> shouldn't be a quirk and it is actually mentioned in the
> programming guide that it should be done when switching modes
> in DRD.
>
> So, to avoid these !COREIDLE lockups seen on HiKey960, this
> patch issues GCTL soft reset when switching modes if the
> controller is in DRD mode.
>
> Cc: Felipe Balbi
> Cc: Tejas Joglekar
> Cc: Yang Fei
> Cc: YongQin Liu
> Cc: Andrzej Pietrasiewicz
> Cc: Thinh Nguyen
> Cc: Jun Li
> Cc: Mauro Carvalho Chehab
> Cc: Greg Kroah-Hartman
> Cc: linux-...@vger.kernel.org
> Signed-off-by: Yu Chen
> Signed-off-by: John Stultz
> ---
> v2:
> * Rework to always call the GCTL soft reset in DRD mode,
> rather then using a quirk as suggested by Thinh Nguyen
>
> ---
> drivers/usb/dwc3/core.c | 19 +++
> 1 file changed, 19 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index bdf0925da6b6..ca94f3a2a83c 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -114,10 +114,24 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode)
> dwc->current_dr_role = mode;
> }
>
> +static void dwc3_gctl_core_soft_reset(struct dwc3 *dwc)
> +{
> + int reg;
> +
> + reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> + reg |= (DWC3_GCTL_CORESOFTRESET);
> + dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> +
> + reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> + reg &= ~(DWC3_GCTL_CORESOFTRESET);
> + dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> +}
> +
> static void __dwc3_set_mode(struct work_struct *work)
> {
> struct dwc3 *dwc = work_to_dwc(work);
> unsigned long flags;
> + int hw_mode;
> int ret;
> u32 reg;
>
> @@ -154,6 +168,11 @@ static void __dwc3_set_mode(struct work_struct *work)
> break;
> }
>
> + /* Execute a GCTL Core Soft Reset when switch mode in DRD*/
> + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
> + if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD)
> + dwc3_gctl_core_soft_reset(dwc);
> +
I think this should be done inside the spin_lock.
> spin_lock_irqsave(>lock, flags);
>
> dwc3_set_prtcap(dwc, dwc->desired_dr_role);
The DRD mode change sequence should be like this if we want to switch
from host -> device according to the programming guide (for all DRD IPs):
1. Reset controller with GCTL.CoreSoftReset
2. Set GCTL.PrtCapDir(device)
3. Soft reset with DCTL.CSftRst
4. Then follow up with the initializing registers sequence
However, from code review, with this patch, it follows this sequence:
a. Soft reset with DCTL.CSftRst on driver probe
b. Reset controller with GCTL.CoreSoftReset
c. Set GCTL.PrtCapDir(device)
d. < missing DCTL.CSftRst >
e. Then follow up with initializing registers sequence
It may work, but it doesn't follow the programming guide.
For device -> host, it should be fine because the xHCI driver will do
USBCMD.HCRST during initialization.
BR,
Thinh
Re: [RFC][PATCH] usb: dwc3: Add quirk to trigger a GCTL soft reset for Hisilicon Kirin Soc Platform
John Stultz wrote:
> From: Yu Chen
>
> With the current dwc3 code on the HiKey960 we often see the
> COREIDLE flag get stuck off in __dwc3_gadget_start(), which
> seems to prevent the reset irq and causes the USB gadget to
> fail to initialize.
>
> We had seen occasional initialization failures with older
> kernels but with recent 5.x era kernels it seemed to be becoming
> much more common, so I dug back through some older trees and
> realized I dropped this quirk from Yu Chen during upstreaming
> as I couldn't provide a proper rational for it and it didn't
> seem to be necessary. I now realize I was wrong.
>
> On the upside, I can now understand more why such a quirk is
> needed.
This shouldn't be a quirk. It's part of the programming guide when
switching mode in DRD. I don't know how we missed this.
>
> So to address a quirk in the DesignWare USB3 DRD Core of
> Hisilicon Kirin SoCs, this patch adds a quirk flag which
> executes a GCTL soft reset when we switch modes.
>
> Cc: Felipe Balbi
> Cc: Tejas Joglekar
> Cc: Yang Fei
> Cc: YongQin Liu
> Cc: Andrzej Pietrasiewicz
> Cc: Thinh Nguyen
> Cc: Jun Li
> Cc: Mauro Carvalho Chehab
> Cc: Rob Herring
> Cc: Greg Kroah-Hartman
> Cc: linux-...@vger.kernel.org
> Cc: devicet...@vger.kernel.org
> Signed-off-by: Yu Chen
> Signed-off-by: John Stultz
> ---
> .../devicetree/bindings/usb/dwc3.txt | 3 +++
> drivers/usb/dwc3/core.c | 19 +++
> drivers/usb/dwc3/core.h | 1 +
> 3 files changed, 23 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
> b/Documentation/devicetree/bindings/usb/dwc3.txt
> index 1aae2b6160c1..f435bae0f172 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -81,6 +81,9 @@ Optional properties:
> - snps,dis-split-quirk: when set, change the way URBs are handled by the
>driver. Needed to avoid -EPROTO errors with usbhid
>on some devices (Hikey 970).
> + - snps,gctl-reset-quirk: When set, execute a soft reset on mode changes.
> +Needed to avoid COREIDLE getting stuck off and the
> +gadget to fail to initialize (HiKey960).
> - snps,is-utmi-l1-suspend: true when DWC3 asserts output signal
> utmi_l1_suspend_n, false when asserts utmi_sleep_n
> - snps,hird-threshold: HIRD threshold
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index bdf0925da6b6..b138c67e3892 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -114,6 +114,19 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode)
> dwc->current_dr_role = mode;
> }
>
> +static void dwc3_gctl_core_soft_reset(struct dwc3 *dwc)
> +{
> + int reg;
> +
> + reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> + reg |= (DWC3_GCTL_CORESOFTRESET);
> + dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> +
> + reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> + reg &= ~(DWC3_GCTL_CORESOFTRESET);
> + dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> +}
> +
> static void __dwc3_set_mode(struct work_struct *work)
> {
> struct dwc3 *dwc = work_to_dwc(work);
> @@ -178,6 +191,10 @@ static void __dwc3_set_mode(struct work_struct *work)
> }
> break;
> case DWC3_GCTL_PRTCAP_DEVICE:
> + /* Execute a GCTL Core Soft Reset when switch mode */
> + if (dwc->gctl_reset_quirk)
> + dwc3_gctl_core_soft_reset(dwc);
> +
This should be done before dwc3_set_prtcap(), and this applies when
switching from device to host mode also. Make sure to check if the
controller is DRD before doing this.
> dwc3_event_buffers_setup(dwc);
>
> if (dwc->usb2_phy)
> @@ -1357,6 +1374,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>
> dwc->dis_split_quirk = device_property_read_bool(dev,
> "snps,dis-split-quirk");
> + dwc->gctl_reset_quirk = device_property_read_bool(dev,
> + "snps,gctl-reset-quirk");
>
> dwc->lpm_nyet_threshold = lpm_nyet_threshold;
> dwc->tx_de_emphasis = tx_de_emphasis;
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 74323b10a64a..993f243aedc8 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1252,6 +1252,7 @@ struct dwc3 {
> unsigneddis_metastability_quirk:1;
>
> unsigneddis_split_quirk:1;
> + unsignedgctl_reset_quirk:1;
>
> u16 imod_interval;
> };
BR,
Thinh
Re: [PATCH v2] usb: dwc3: core: fix a issue about clear connect state
Dejin Zheng wrote:
> According to Synopsys Programming Guide chapter 2.2 Register Resets,
> it cannot reset the DCTL register by setting DCTL.CSFTRST for core soft
> reset, if DWC3 controller as a slave device and stay connected with a usb
> host, then, while rebooting linux, it will fail to reinitialize dwc3 as a
> slave device when the DWC3 controller did not power off.
If you reboot the OS, wouldn't it go through the driver tear-down
sequence and clear the run_stop bit anyway?
However, I can see how this can be an issue.
> because the
> connection status is incorrect, so we also need to clear DCTL.RUN_STOP
> bit for disabling connect when doing core soft reset.
>
> Fixes: f59dcab176293b6 ("usb: dwc3: core: improve reset sequence")
> Signed-off-by: Dejin Zheng
> ---
> v1 -> v2:
> * modify some commit messages by Sergei's suggest, Thanks
> very much for Sergei's help!
>
> drivers/usb/dwc3/core.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 2eb34c8b4065..239636c454c2 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -256,6 +256,7 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc)
>
> reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> reg |= DWC3_DCTL_CSFTRST;
> + reg &= ~DWC3_DCTL_RUN_STOP;
> dwc3_writel(dwc->regs, DWC3_DCTL, reg);
>
> /*
There will still be other stale configuration in DCTL if you do this. I
think it's better to reset the other fields of DCTL to the default
(should be all 0s) instead of doing register read-modify-write as what
we're doing here. If not, at least we should use
dwc3_gadget_dctl_write_safe().
Thanks,
Thinh
Re: [PATCH v6 04/13] usb: dwc3: Add splitdisable quirk for Hisilicon Kirin Soc
Felipe Balbi wrote:
> Hi,
>
> Mauro Carvalho Chehab writes:
I tested here, together with the Hikey 970 phy RFC patches I sent
last week.
Without this patch, the USB HID driver receives -EPROTO from
submitted URBs, causing it to enter into an endless reset cycle
on every 500 ms, at the hid_io_error() logic.
Tested-by: Mauro Carvalho Chehab
If you prefer, I can re-submit this one with my SOB.
>>> Please do, but since you're changing device tree, I need Rob's acked-by.
>> Ok, I'll do that.
> thanks
>
Em Sat, 20 Apr 2019 14:40:10 +0800
Yu Chen escreveu:
> SPLIT_BOUNDARY_DISABLE should be set for DesignWare USB3 DRD Core
> of Hisilicon Kirin Soc when dwc3 core act as host.
>>> is this Kirin-specific or is this something that we should do a revision
>>> check?
>> I've no idea. I don't have any datasheets from this device.
> I see
>
>>> Why does it affect only Hikey kirin?
>> As John Stultz didn't re-submit this one (and looking at the DT
>> between Kirin 960 and 970 from the original Kernel 4.9 official
>> drivers), I suspect that only Kirin 970 requires this quirk.
>>
>> It could well be due to some Dwc3 revision, but it could also be due
>> to some differences at the USB part of the SoC, as there are a
> the reason I ask is that if it's caused by dwc3 revision, then we don't
> need the extra dt property, we can rely on a revision check. If it's
> something that can't be detected in runtime, then we need a property.
>
>> few other things different between hikey 960 and 970: it has a
>> different PHY driver, and there are also some differences at the
>> USB HUB which is connected into it.
>>
>> On both devices, the USB physical ports are actually connected
>> into a HUB. In the case of Hikey 970, the hub seems to be a
>> TI TUSB8041 4-Port Hub:
>>
>> $ lsusb
>> Bus 002 Device 002: ID 0451:8140 Texas Instruments, Inc. TUSB8041
>> 4-Port Hub
>> Bus 002 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
>> Bus 001 Device 004: ID 090c:1000 Silicon Motion, Inc. - Taiwan
>> (formerly Feiya Technology Corp.) Flash Drive
>> Bus 001 Device 003: ID 413c:301a Dell Computer Corp. Dell MS116 Optical
>> Mouse
>> Bus 001 Device 002: ID 0451:8142 Texas Instruments, Inc. TUSB8041
>> 4-Port Hub
>> Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
>>
>>> What's the dwc3 revision on
>>> that SoC (grep SNPSID /sys/kernel/debugfs/*dwc3/regdump)?
>> GSNPSID = 0x33313130
> This isn't even listed as a known revision in dwc3/core.h. Thinh, could
> the issue being described here caused by a known Erratum with this
> particular revision?
If you have a STAR issue number, then I may be able to look it up.
Btw, GNSPSID register is the ID for DWC_usb31 IP. For DWC_usb31 and
DWC_usb32, we need to read the version number and version type registers
(offset 0xc1a0 and 0xc1a4 respectively).
We need to add these registers to the register dump.
BR,
Thinh
>
> + reg = dwc3_readl(dwc->regs, DWC3_GUCTL3);
> + reg |= DWC3_GUCTL3_SPLITDISABLE;
> + dwc3_writel(dwc->regs, DWC3_GUCTL3, reg);
> + }
> +}
> +#else
> +#define dwc3_complete NULL
> #endif /* CONFIG_PM_SLEEP */
>
> static const struct dev_pm_ops dwc3_dev_pm_ops = {
> SET_SYSTEM_SLEEP_PM_OPS(dwc3_suspend, dwc3_resume)
> + .complete = dwc3_complete,
>>> why is this done on complete? Why can't it be done at the end of
>>> dwc3_resume()?
>> Again, no idea. I didn't actually tried to suspend/resume.
>>
>> Maybe the original author can shed a light on it.
> yeah, would be nice :-)
>
Re: [PATCH v3] usb: dwc3: Stop active transfers before halting the controller
e transfers.
> + */
> + dwc3_stop_active_transfers(dwc);
> + __dwc3_gadget_stop(dwc);
> +
> + count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
> + count &= DWC3_GEVNTCOUNT_MASK;
> + if (count > 0) {
> + dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count);
> + dwc->ev_buf->lpos = (dwc->ev_buf->lpos + count) %
> + dwc->ev_buf->length;
> + }
> + }
> +
> ret = dwc3_gadget_run_stop(dwc, is_on, false);
> spin_unlock_irqrestore(>lock, flags);
> + enable_irq(dwc->irq_gadget);
>
> return ret;
> }
> @@ -3100,6 +3145,8 @@ static void dwc3_gadget_reset_interrupt(struct dwc3
> *dwc)
> }
>
> dwc3_reset_gadget(dwc);
> + /* Stop any active/pending transfers when receiving bus reset */
> + dwc3_stop_active_transfers(dwc);
>
> reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> reg &= ~DWC3_DCTL_TSTCTRL_MASK;
Looks good to me.
Reviewed-by: Thinh Nguyen
Thanks,
Thinh
Re: [PATCH v2] usb: dwc3: Stop active transfers before halting the controller
Wesley Cheng wrote:
>
> On 9/1/2020 3:14 PM, Wesley Cheng wrote:
>>
>> On 8/29/2020 2:35 PM, Thinh Nguyen wrote:
>>> Wesley Cheng wrote:
>>>> In the DWC3 databook, for a device initiated disconnect or bus reset, the
>>>> driver is required to send dependxfer commands for any pending transfers.
>>>> In addition, before the controller can move to the halted state, the SW
>>>> needs to acknowledge any pending events. If the controller is not halted
>>>> properly, there is a chance the controller will continue accessing stale or
>>>> freed TRBs and buffers.
>>>>
>>>> Signed-off-by: Wesley Cheng
>>>>
>>>> ---
>>>> Changes in v2:
>>>> - Moved cleanup code to the pullup() API to differentiate between device
>>>>disconnect and hibernation.
>>>> - Added cleanup code to the bus reset case as well.
>>>> - Verified the move to pullup() did not reproduce the problen using the
>>>>same test sequence.
>>>>
>>>> Verified fix by adding a check for ETIMEDOUT during the run stop call.
>>>> Shell script writing to the configfs UDC file to trigger disconnect and
>>>> connect. Batch script to have PC execute data transfers over adb (ie adb
>>>> push) After a few iterations, we'd run into a scenario where the
>>>> controller wasn't halted. With the following change, no failed halts after
>>>> many iterations.
>>>> ---
>>>> drivers/usb/dwc3/ep0.c| 2 +-
>>>> drivers/usb/dwc3/gadget.c | 52 ++-
>>>> 2 files changed, 52 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>>>> index 59f2e8c31bd1..456aa87e8778 100644
>>>> --- a/drivers/usb/dwc3/ep0.c
>>>> +++ b/drivers/usb/dwc3/ep0.c
>>>> @@ -197,7 +197,7 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct
>>>> usb_request *request,
>>>>int ret;
>>>>
>>>>spin_lock_irqsave(>lock, flags);
>>>> - if (!dep->endpoint.desc) {
>>>> + if (!dep->endpoint.desc || !dwc->pullups_connected) {
>>>>dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
>>>>dep->name);
>>>>ret = -ESHUTDOWN;
>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>> index 3ab6f118c508..df8d89d6bdc9 100644
>>>> --- a/drivers/usb/dwc3/gadget.c
>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>> @@ -1516,7 +1516,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep
>>>> *dep, struct dwc3_request *req)
>>>> {
>>>>struct dwc3 *dwc = dep->dwc;
>>>>
>>>> - if (!dep->endpoint.desc) {
>>>> + if (!dep->endpoint.desc || !dwc->pullups_connected) {
>>>>dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
>>>>dep->name);
>>>>return -ESHUTDOWN;
>>>> @@ -1926,6 +1926,24 @@ static int dwc3_gadget_set_selfpowered(struct
>>>> usb_gadget *g,
>>>>return 0;
>>>> }
>>>>
>>>> +static void dwc3_stop_active_transfers(struct dwc3 *dwc)
>>>> +{
>>>> + u32 epnum;
>>>> +
>>>> + for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
>>>> + struct dwc3_ep *dep;
>>>> +
>>>> + dep = dwc->eps[epnum];
>>>> + if (!dep)
>>>> + continue;
>>>> +
>>>> + if (!(dep->flags & DWC3_EP_ENABLED))
>>>> + continue;
>>> Don't do the enabled check here. Let the dwc3_stop_active_transfer() do
>>> that checking.
>>>
>> Hi Thinh,
>>
>> Thanks for the detailed review, as always. Got it, we can allow that to
>> catch it based off the DWC3_EP_TRANSFER_STARTED.
>>
>>>> +
>>>> + dwc3_remove_requests(dwc, dep);
>>>> + }
>>>> +}
>>>> +
>>>> static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
>>>> {
>>>>u32 reg;
>>>> @@ -1994,9 +2012,39 @@ static int dwc3_gadget_pullup(struct usb_gadget *g,
>>>> int is_
Re: [PATCH v2] usb: dwc3: Stop active transfers before halting the controller
Wesley Cheng wrote:
> In the DWC3 databook, for a device initiated disconnect or bus reset, the
> driver is required to send dependxfer commands for any pending transfers.
> In addition, before the controller can move to the halted state, the SW
> needs to acknowledge any pending events. If the controller is not halted
> properly, there is a chance the controller will continue accessing stale or
> freed TRBs and buffers.
>
> Signed-off-by: Wesley Cheng
>
> ---
> Changes in v2:
> - Moved cleanup code to the pullup() API to differentiate between device
>disconnect and hibernation.
> - Added cleanup code to the bus reset case as well.
> - Verified the move to pullup() did not reproduce the problen using the
>same test sequence.
>
> Verified fix by adding a check for ETIMEDOUT during the run stop call.
> Shell script writing to the configfs UDC file to trigger disconnect and
> connect. Batch script to have PC execute data transfers over adb (ie adb
> push) After a few iterations, we'd run into a scenario where the
> controller wasn't halted. With the following change, no failed halts after
> many iterations.
> ---
> drivers/usb/dwc3/ep0.c| 2 +-
> drivers/usb/dwc3/gadget.c | 52 ++-
> 2 files changed, 52 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 59f2e8c31bd1..456aa87e8778 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -197,7 +197,7 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct
> usb_request *request,
> int ret;
>
> spin_lock_irqsave(>lock, flags);
> - if (!dep->endpoint.desc) {
> + if (!dep->endpoint.desc || !dwc->pullups_connected) {
> dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
> dep->name);
> ret = -ESHUTDOWN;
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 3ab6f118c508..df8d89d6bdc9 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1516,7 +1516,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep,
> struct dwc3_request *req)
> {
> struct dwc3 *dwc = dep->dwc;
>
> - if (!dep->endpoint.desc) {
> + if (!dep->endpoint.desc || !dwc->pullups_connected) {
> dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
> dep->name);
> return -ESHUTDOWN;
> @@ -1926,6 +1926,24 @@ static int dwc3_gadget_set_selfpowered(struct
> usb_gadget *g,
> return 0;
> }
>
> +static void dwc3_stop_active_transfers(struct dwc3 *dwc)
> +{
> + u32 epnum;
> +
> + for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
> + struct dwc3_ep *dep;
> +
> + dep = dwc->eps[epnum];
> + if (!dep)
> + continue;
> +
> + if (!(dep->flags & DWC3_EP_ENABLED))
> + continue;
Don't do the enabled check here. Let the dwc3_stop_active_transfer() do
that checking.
> +
> + dwc3_remove_requests(dwc, dep);
> + }
> +}
> +
> static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
> {
> u32 reg;
> @@ -1994,9 +2012,39 @@ static int dwc3_gadget_pullup(struct usb_gadget *g,
> int is_on)
> }
> }
>
> + /*
> + * Synchronize and disable any further event handling while controller
> + * is being enabled/disabled.
> + */
> + disable_irq(dwc->irq_gadget);
I think it's better to do dwc3_gadget_disable_irq(). This only stops
handling events. Although it's unlikely, the controller may still
generate events before it's halted.
> spin_lock_irqsave(>lock, flags);
> +
> + /* Controller is not halted until pending events are acknowledged */
> + if (!is_on) {
> + u32 reg;
> +
> + __dwc3_gadget_ep_disable(dwc->eps[0]);
> + __dwc3_gadget_ep_disable(dwc->eps[1]);
You can just do __dwc3_gadget_stop(), and do that after
dwc3_stop_active_transfers().
> +
> + /*
> + * The databook explicitly mentions for a device-initiated
> + * disconnect sequence, the SW needs to ensure that it ends any
> + * active transfers.
> + */
> + dwc3_stop_active_transfers(dwc);
> +
> + reg = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
> + reg &= DWC3_GEVNTCOUNT_MASK;
Can we use another variable "count" instead of reusing reg to make it a
little clearer?
> + if (reg > 0) {
> + dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), reg);
> + dwc->ev_buf->lpos = (dwc->ev_buf->lpos + reg) %
> + dwc->ev_buf->length;
> + }
> + }
> +
> ret = dwc3_gadget_run_stop(dwc, is_on, false);
> spin_unlock_irqrestore(>lock,
Re: [PATCH] usb: dwc3: Stop active transfers before halting the controller
Wesley Cheng wrote:
> On 8/19/2020 2:42 PM, Thinh Nguyen wrote:
>> Hi,
>>
>> Wesley Cheng wrote:
>>> In the DWC3 databook, for a device initiated disconnect, the driver is
>>> required to send dependxfer commands for any pending transfers.
>>> In addition, before the controller can move to the halted state, the SW
>>> needs to acknowledge any pending events. If the controller is not halted
>>> properly, there is a chance the controller will continue accessing stale or
>>> freed TRBs and buffers.
>>>
>>> Signed-off-by: Wesley Cheng
>>>
>>> ---
>>> Verified fix by adding a check for ETIMEDOUT during the run stop call.
>>> Shell script writing to the configfs UDC file to trigger disconnect and
>>> connect. Batch script to have PC execute data transfers over adb (ie adb
>>> push) After a few iterations, we'd run into a scenario where the
>>> controller wasn't halted. With the following change, no failed halts after
>>> many iterations.
Btw, we have some sysfs attributes to do soft-connect/disconnect also
/sys/class/udc//soft_connect
>>> ---
>>> drivers/usb/dwc3/ep0.c| 2 +-
>>> drivers/usb/dwc3/gadget.c | 59 +--
>>> 2 files changed, 57 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>>> index 59f2e8c31bd1..456aa87e8778 100644
>>> --- a/drivers/usb/dwc3/ep0.c
>>> +++ b/drivers/usb/dwc3/ep0.c
>>> @@ -197,7 +197,7 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct
>>> usb_request *request,
>>> int ret;
>>>
>>> spin_lock_irqsave(>lock, flags);
>>> - if (!dep->endpoint.desc) {
>>> + if (!dep->endpoint.desc || !dwc->pullups_connected) {
>>> dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
>>> dep->name);
>>> ret = -ESHUTDOWN;
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index 3ab6f118c508..1f981942d7f9 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -1516,7 +1516,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep
>>> *dep, struct dwc3_request *req)
>>> {
>>> struct dwc3 *dwc = dep->dwc;
>>>
>>> - if (!dep->endpoint.desc) {
>>> + if (!dep->endpoint.desc || !dwc->pullups_connected) {
>>> dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
>>> dep->name);
>>> return -ESHUTDOWN;
>>> @@ -1926,6 +1926,24 @@ static int dwc3_gadget_set_selfpowered(struct
>>> usb_gadget *g,
>>> return 0;
>>> }
>>>
>>> +static void dwc3_stop_active_transfers(struct dwc3 *dwc)
>>> +{
>>> + u32 epnum;
>>> +
>>> + for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
>>> + struct dwc3_ep *dep;
>>> +
>>> + dep = dwc->eps[epnum];
>>> + if (!dep)
>>> + continue;
>>> +
>>> + if (!(dep->flags & DWC3_EP_ENABLED))
>>> + continue;
>>> +
>>> + dwc3_remove_requests(dwc, dep);
>>> + }
>>> +}
>>> +
>>> static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
>>> {
>>> u32 reg;
>>> @@ -1950,16 +1968,37 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc,
>>> int is_on, int suspend)
>>>
>>> dwc->pullups_connected = true;
>>> } else {
>>> + dwc->pullups_connected = false;
>>> +
>>> + __dwc3_gadget_ep_disable(dwc->eps[0]);
>>> + __dwc3_gadget_ep_disable(dwc->eps[1]);
>> run_stop() function shouldn't be doing this. This is done in
>> dwc3_gadget_stop(). Also, if it's device-initiated disconnect, driver
>> needs to wait for control transfers to complete.
>>
> Hi Thinh ,
>
> Thanks for the feedback.
Thanks for the patch :)
>
> We already wait for the ep0state to move to the setup stage before
> running the run stop routine, but events can still be triggered until
> the controller is halted. (which is not started until we attempt to
> write to the DCTL register) The reasoning will be the same as the below
> comment.
>
Re: [PATCH] usb: dwc3: Stop active transfers before halting the controller
Hi,
Wesley Cheng wrote:
> In the DWC3 databook, for a device initiated disconnect, the driver is
> required to send dependxfer commands for any pending transfers.
> In addition, before the controller can move to the halted state, the SW
> needs to acknowledge any pending events. If the controller is not halted
> properly, there is a chance the controller will continue accessing stale or
> freed TRBs and buffers.
>
> Signed-off-by: Wesley Cheng
>
> ---
> Verified fix by adding a check for ETIMEDOUT during the run stop call.
> Shell script writing to the configfs UDC file to trigger disconnect and
> connect. Batch script to have PC execute data transfers over adb (ie adb
> push) After a few iterations, we'd run into a scenario where the
> controller wasn't halted. With the following change, no failed halts after
> many iterations.
> ---
> drivers/usb/dwc3/ep0.c| 2 +-
> drivers/usb/dwc3/gadget.c | 59 +--
> 2 files changed, 57 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 59f2e8c31bd1..456aa87e8778 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -197,7 +197,7 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct
> usb_request *request,
> int ret;
>
> spin_lock_irqsave(>lock, flags);
> - if (!dep->endpoint.desc) {
> + if (!dep->endpoint.desc || !dwc->pullups_connected) {
> dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
> dep->name);
> ret = -ESHUTDOWN;
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 3ab6f118c508..1f981942d7f9 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1516,7 +1516,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep,
> struct dwc3_request *req)
> {
> struct dwc3 *dwc = dep->dwc;
>
> - if (!dep->endpoint.desc) {
> + if (!dep->endpoint.desc || !dwc->pullups_connected) {
> dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
> dep->name);
> return -ESHUTDOWN;
> @@ -1926,6 +1926,24 @@ static int dwc3_gadget_set_selfpowered(struct
> usb_gadget *g,
> return 0;
> }
>
> +static void dwc3_stop_active_transfers(struct dwc3 *dwc)
> +{
> + u32 epnum;
> +
> + for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
> + struct dwc3_ep *dep;
> +
> + dep = dwc->eps[epnum];
> + if (!dep)
> + continue;
> +
> + if (!(dep->flags & DWC3_EP_ENABLED))
> + continue;
> +
> + dwc3_remove_requests(dwc, dep);
> + }
> +}
> +
> static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
> {
> u32 reg;
> @@ -1950,16 +1968,37 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int
> is_on, int suspend)
>
> dwc->pullups_connected = true;
> } else {
> + dwc->pullups_connected = false;
> +
> + __dwc3_gadget_ep_disable(dwc->eps[0]);
> + __dwc3_gadget_ep_disable(dwc->eps[1]);
run_stop() function shouldn't be doing this. This is done in
dwc3_gadget_stop(). Also, if it's device-initiated disconnect, driver
needs to wait for control transfers to complete.
> +
> + /*
> + * The databook explicitly mentions for a device-initiated
> + * disconnect sequence, the SW needs to ensure that it ends any
> + * active transfers.
> + */
> + dwc3_stop_active_transfers(dwc);
It shouldn't be done here. Maybe move this to the dwc3_gadget_pullup()
function. The run_stop() function can be called for other context beside
this (e.g. hibernation).
> +
> reg &= ~DWC3_DCTL_RUN_STOP;
>
> if (dwc->has_hibernation && !suspend)
> reg &= ~DWC3_DCTL_KEEP_CONNECT;
> -
> - dwc->pullups_connected = false;
> }
>
> dwc3_gadget_dctl_write_safe(dwc, reg);
>
> + /* Controller is not halted until pending events are acknowledged */
> + if (!is_on) {
> + reg = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
> + reg &= DWC3_GEVNTCOUNT_MASK;
> + if (reg > 0) {
> + dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), reg);
> + dwc->ev_buf->lpos = (dwc->ev_buf->lpos + reg) %
> + dwc->ev_buf->length;
> + }
> + }
> +
Driver should handle the events before clearing the run_stop bit, not
just acknowledging and ignoring them.
> do {
> reg = dwc3_readl(dwc->regs, DWC3_DSTS);
> reg &= DWC3_DSTS_DEVCTRLHLT;
> @@ -1994,9 +2033,15 @@ static int dwc3_gadget_pullup(struct usb_gadget *g,
> int is_on)
> }
> }
>
> +
Re: [RFC PATCH] usb: dwc3: fix maximum_speed check for usb2.0-only core
Hi,
Thinh Nguyen wrote:
> Hi,
>
> Chunfeng Yun wrote:
>> The maximum_speed will be USB_SPEED_SUPER_PLUS, but the
>> maximum_speed check for usb2.0-only core doesn't consider it,
>> so fix it, and move the ckeck into dwc3_check_params().
>>
>> Signed-off-by: Chunfeng Yun
>> ---
>> Note:
>>
>> When I look at the code, find that this may be a problem, but no
>> platform to test it.
>> ---
>>drivers/usb/dwc3/core.c | 14 +++---
>>1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 25c686a7..ffd5ab3 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -930,13 +930,6 @@ static int dwc3_core_init(struct dwc3 *dwc)
>> */
>> dwc3_writel(dwc->regs, DWC3_GUID, LINUX_VERSION_CODE);
>>
>> -/* Handle USB2.0-only core configuration */
>> -if (DWC3_GHWPARAMS3_SSPHY_IFC(dwc->hwparams.hwparams3) ==
>> -DWC3_GHWPARAMS3_SSPHY_IFC_DIS) {
>> -if (dwc->maximum_speed == USB_SPEED_SUPER)
>> -dwc->maximum_speed = USB_SPEED_HIGH;
>> -}
>> -
>> ret = dwc3_phy_setup(dwc);
>> if (ret)
>> goto err0;
>> @@ -1426,6 +1419,13 @@ static void dwc3_check_params(struct dwc3 *dwc)
>>
>> break;
>> }
>> +
>> +/* Handle USB2.0-only core configuration */
>> +if (DWC3_GHWPARAMS3_SSPHY_IFC(dwc->hwparams.hwparams3) ==
>> +DWC3_GHWPARAMS3_SSPHY_IFC_DIS) {
>> +if (dwc->maximum_speed > USB_SPEED_HIGH)
>> +dwc->maximum_speed = USB_SPEED_HIGH;
>> +}
>>}
>>
>>static int dwc3_probe(struct platform_device *pdev)
> Actually, the dwc->maximum_speed captures the maximum speed device
> property value. It maybe be set based on the phy's capability if there's
> no property specifying it (i.e. maximum_speed is USB_SPEED_UNKNOWN).
>
> So, this code should be removed. The fix should be in the check of
> dwc3_check_params(). If maximum_speed = USB_SPEED_UNKNOWN and the phy's
> capability is only up to highspeed, then set the maximum_speed to
> highspeed only.
>
Are you going into update and resend this patch? If not I can create one
and add your "Reported-by"
BR,
Thinh
Re: [RFC PATCH] usb: dwc3: fix maximum_speed check for usb2.0-only core
Hi,
Chunfeng Yun wrote:
> The maximum_speed will be USB_SPEED_SUPER_PLUS, but the
> maximum_speed check for usb2.0-only core doesn't consider it,
> so fix it, and move the ckeck into dwc3_check_params().
>
> Signed-off-by: Chunfeng Yun
> ---
> Note:
>
> When I look at the code, find that this may be a problem, but no
> platform to test it.
> ---
> drivers/usb/dwc3/core.c | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 25c686a7..ffd5ab3 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -930,13 +930,6 @@ static int dwc3_core_init(struct dwc3 *dwc)
>*/
> dwc3_writel(dwc->regs, DWC3_GUID, LINUX_VERSION_CODE);
>
> - /* Handle USB2.0-only core configuration */
> - if (DWC3_GHWPARAMS3_SSPHY_IFC(dwc->hwparams.hwparams3) ==
> - DWC3_GHWPARAMS3_SSPHY_IFC_DIS) {
> - if (dwc->maximum_speed == USB_SPEED_SUPER)
> - dwc->maximum_speed = USB_SPEED_HIGH;
> - }
> -
> ret = dwc3_phy_setup(dwc);
> if (ret)
> goto err0;
> @@ -1426,6 +1419,13 @@ static void dwc3_check_params(struct dwc3 *dwc)
>
> break;
> }
> +
> + /* Handle USB2.0-only core configuration */
> + if (DWC3_GHWPARAMS3_SSPHY_IFC(dwc->hwparams.hwparams3) ==
> + DWC3_GHWPARAMS3_SSPHY_IFC_DIS) {
> + if (dwc->maximum_speed > USB_SPEED_HIGH)
> + dwc->maximum_speed = USB_SPEED_HIGH;
> + }
> }
>
> static int dwc3_probe(struct platform_device *pdev)
Actually, the dwc->maximum_speed captures the maximum speed device
property value. It maybe be set based on the phy's capability if there's
no property specifying it (i.e. maximum_speed is USB_SPEED_UNKNOWN).
So, this code should be removed. The fix should be in the check of
dwc3_check_params(). If maximum_speed = USB_SPEED_UNKNOWN and the phy's
capability is only up to highspeed, then set the maximum_speed to
highspeed only.
Thanks,
Thinh
Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
Thinh Nguyen wrote: > Jun Li wrote: >> Hi >> >>> -Original Message- >>> From: Thinh Nguyen >>> Sent: 2020年5月19日 14:46 >>> To: Jun Li ; Felipe Balbi ; Jun Li >>> >>> Cc: John Stultz ; lkml >>> ; Yu >>> Chen ; Greg Kroah-Hartman >>> ; Rob >>> Herring ; Mark Rutland ; ShuFan >>> Lee >>> ; Heikki Krogerus ; >>> Suzuki K Poulose ; Chunfeng Yun >>> ; Hans de Goede ; Andy >>> Shevchenko >>> ; Valentin Schneider >>> ; >>> Jack Pham ; Linux USB List >>> ; open >>> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS >>> ; >>> Peter Chen >>> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared >>> by device >>> controller >>> >>> Thinh Nguyen wrote: >>>> Jun Li wrote: >>>>>> -Original Message- >>>>>> From: Felipe Balbi On Behalf Of Felipe Balbi >>>>>> Sent: 2020年5月16日 19:57 >>>>>> To: Jun Li ; Thinh Nguyen >>>>>> ; Jun Li >>>>>> Cc: John Stultz ; lkml >>>>>> ; Yu Chen ; Greg >>>>>> Kroah-Hartman ; Rob Herring >>>>>> ; Mark Rutland ; ShuFan >>>>>> Lee ; Heikki Krogerus >>>>>> ; >>>>>> Suzuki K Poulose ; Chunfeng Yun >>>>>> ; Hans de Goede ; >>>>>> Andy Shevchenko ; Valentin Schneider >>>>>> ; Jack Pham ; >>>>>> Linux USB List ; open list:OPEN FIRMWARE >>>>>> AND FLATTENED DEVICE TREE BINDINGS ; >>>>>> Peter Chen ; Thinh Nguyen >>>>>> >>>>>> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct >>>>>> cleared by device controller >>>>>> >>>>>> >>>>>> Hi, >>>>>> >>>>>> Jun Li writes: >>>>>>>>>> Hi Thinh, could you comment this? >>>>>>>>> You only need to wake up the usb2 phy when issuing the command >>>>>>>>> while running in highspeed or below. If you're running in SS or >>>>>>>>> higher, internally the controller does it for you for usb3 phy. >>>>>>>>> In Jun's case, it seems like it takes longer for his phy to wake up. >>>>>>>>> >>>>>>>>> IMO, in this case, I think it's fine to increase the command timeout. >>>>>>>> Is there an upper limit to this? Is 32k clock the slowest that can >>>>>>>> be fed to the PHY as a suspend clock? >>>>>>> Yes, 32K clock is the slowest, Per DWC3 document on Power Down >>>>>>> Scale (bits 31:19 of GCTL): >>>>>>> >>>>>>> "Power Down Scale (PwrDnScale) >>>>>>> The USB3 suspend_clk input replaces pipe3_rx_pclk as a clock source >>>>>>> to a small part of the USB3 controller that operates when the SS >>>>>>> PHY is in its lowest power (P3) state, and therefore does not provide a >>>>>>> clock. >>>>>>> The Power Down Scale field specifies how many suspend_clk periods >>>>>>> fit into a 16 kHz clock period. When performing the division, round >>>>>>> up the remainder. >>>>>>> For example, when using an 8-bit/16-bit/32-bit PHY and 25-MHz >>>>>>> Suspend clock, Power Down Scale = 25000 kHz/16 kHz = 13'd1563 >>>>>>> (rounder up) >>>>>>> Note: >>>>>>> - Minimum Suspend clock frequency is 32 kHz >>>>>>> - Maximum Suspend clock frequency is 125 MHz" >>>>>> Cool, now do we have an upper bound for how many clock cycles it >>>>>> takes to wake up the PHY? >>>>> My understanding is this ep command does not wake up the SS PHY, the >>>>> SS PHY still stays at P3 when execute this ep command. The time >>>>> required here is to wait controller complete something for this ep >>>>> command with 32K clock. >>>> Sorry I made a mistake. You're right. Just checked with one of the RTL >>>> engineers, and it doesn't need to wake up the phy. However, if it is >>>> eSS speed, it may take longer time as the command may be completing >>>> with the suspend clock. >>>> >>> What's the value for GCTL[7:6]? >> 2'b00 >> >> Thanks >> Li Jun > (Sorry for the delay reply) > > If it's 0, then the ram clock should be the same as the bus_clk, which > is odd since you mentioned that the suspend_clk is used instead while in P3. Just checked with the RTL engineer, even if GCTL[7:6] is set to 0, internally it can still run with suspend clock during P3. > Anyway, I was looking for a way maybe to improve the speed during > issuing a command. One way is to set GUSB3PIPECTL[17]=0, and it should > wakeup the phy anytime. I think Felipe suggested it. It's odd that it > doesn't work for you. I don't have other ideas beside increasing the > command timeout. > In any case, increasing the timeout should be fine with me. It maybe difficult to determine the max timeout base on the slowest clock rate and number of cycles. Different controller and controller versions behave differently and may have different number of clock cycles to complete a command. The RTL engineer recommended timeout to be at least 1ms (which maybe more than the polling rate of this patch). I'm fine with either the rate provided by this tested patch or higher. BR, Thinh
Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
Jun Li wrote: > Hi > >> -Original Message----- >> From: Thinh Nguyen >> Sent: 2020年5月19日 14:46 >> To: Jun Li ; Felipe Balbi ; Jun Li >> >> Cc: John Stultz ; lkml >> ; Yu >> Chen ; Greg Kroah-Hartman ; >> Rob >> Herring ; Mark Rutland ; ShuFan Lee >> ; Heikki Krogerus ; >> Suzuki K Poulose ; Chunfeng Yun >> ; Hans de Goede ; Andy >> Shevchenko >> ; Valentin Schneider ; >> Jack Pham ; Linux USB List >> ; open >> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS >> ; >> Peter Chen >> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared >> by device >> controller >> >> Thinh Nguyen wrote: >>> Jun Li wrote: >>>>> -Original Message- >>>>> From: Felipe Balbi On Behalf Of Felipe Balbi >>>>> Sent: 2020年5月16日 19:57 >>>>> To: Jun Li ; Thinh Nguyen >>>>> ; Jun Li >>>>> Cc: John Stultz ; lkml >>>>> ; Yu Chen ; Greg >>>>> Kroah-Hartman ; Rob Herring >>>>> ; Mark Rutland ; ShuFan >>>>> Lee ; Heikki Krogerus >>>>> ; >>>>> Suzuki K Poulose ; Chunfeng Yun >>>>> ; Hans de Goede ; >>>>> Andy Shevchenko ; Valentin Schneider >>>>> ; Jack Pham ; >>>>> Linux USB List ; open list:OPEN FIRMWARE >>>>> AND FLATTENED DEVICE TREE BINDINGS ; >>>>> Peter Chen ; Thinh Nguyen >>>>> >>>>> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct >>>>> cleared by device controller >>>>> >>>>> >>>>> Hi, >>>>> >>>>> Jun Li writes: >>>>>>>>> Hi Thinh, could you comment this? >>>>>>>> You only need to wake up the usb2 phy when issuing the command >>>>>>>> while running in highspeed or below. If you're running in SS or >>>>>>>> higher, internally the controller does it for you for usb3 phy. >>>>>>>> In Jun's case, it seems like it takes longer for his phy to wake up. >>>>>>>> >>>>>>>> IMO, in this case, I think it's fine to increase the command timeout. >>>>>>> Is there an upper limit to this? Is 32k clock the slowest that can >>>>>>> be fed to the PHY as a suspend clock? >>>>>> Yes, 32K clock is the slowest, Per DWC3 document on Power Down >>>>>> Scale (bits 31:19 of GCTL): >>>>>> >>>>>> "Power Down Scale (PwrDnScale) >>>>>> The USB3 suspend_clk input replaces pipe3_rx_pclk as a clock source >>>>>> to a small part of the USB3 controller that operates when the SS >>>>>> PHY is in its lowest power (P3) state, and therefore does not provide a >>>>>> clock. >>>>>> The Power Down Scale field specifies how many suspend_clk periods >>>>>> fit into a 16 kHz clock period. When performing the division, round >>>>>> up the remainder. >>>>>> For example, when using an 8-bit/16-bit/32-bit PHY and 25-MHz >>>>>> Suspend clock, Power Down Scale = 25000 kHz/16 kHz = 13'd1563 >>>>>> (rounder up) >>>>>> Note: >>>>>> - Minimum Suspend clock frequency is 32 kHz >>>>>> - Maximum Suspend clock frequency is 125 MHz" >>>>> Cool, now do we have an upper bound for how many clock cycles it >>>>> takes to wake up the PHY? >>>> My understanding is this ep command does not wake up the SS PHY, the >>>> SS PHY still stays at P3 when execute this ep command. The time >>>> required here is to wait controller complete something for this ep >>>> command with 32K clock. >>> Sorry I made a mistake. You're right. Just checked with one of the RTL >>> engineers, and it doesn't need to wake up the phy. However, if it is >>> eSS speed, it may take longer time as the command may be completing >>> with the suspend clock. >>> >> What's the value for GCTL[7:6]? > 2'b00 > > Thanks > Li Jun (Sorry for the delay reply) If it's 0, then the ram clock should be the same as the bus_clk, which is odd since you mentioned that the suspend_clk is used instead while in P3. Anyway, I was looking for a way maybe to improve the speed during issuing a command. One way is to set GUSB3PIPECTL[17]=0, and it should wakeup the phy anytime. I think Felipe suggested it. It's odd that it doesn't work for you. I don't have other ideas beside increasing the command timeout. Thanks, Thinh
Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
Thinh Nguyen wrote: > Jun Li wrote: >>> -Original Message- >>> From: Felipe Balbi On Behalf Of Felipe Balbi >>> Sent: 2020年5月16日 19:57 >>> To: Jun Li ; Thinh Nguyen ; Jun >>> Li >>> >>> Cc: John Stultz ; lkml >>> ; Yu >>> Chen ; Greg Kroah-Hartman >>> ; Rob >>> Herring ; Mark Rutland ; ShuFan >>> Lee >>> ; Heikki Krogerus ; >>> Suzuki K Poulose ; Chunfeng Yun >>> ; Hans de Goede ; Andy >>> Shevchenko >>> ; Valentin Schneider >>> ; >>> Jack Pham ; Linux USB List >>> ; open >>> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS >>> ; >>> Peter Chen ; Thinh Nguyen >>> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared >>> by device >>> controller >>> >>> >>> Hi, >>> >>> Jun Li writes: >>>>>>> Hi Thinh, could you comment this? >>>>>> You only need to wake up the usb2 phy when issuing the command >>>>>> while running in highspeed or below. If you're running in SS or >>>>>> higher, internally the controller does it for you for usb3 phy. In >>>>>> Jun's case, it seems like it takes longer for his phy to wake up. >>>>>> >>>>>> IMO, in this case, I think it's fine to increase the command timeout. >>>>> Is there an upper limit to this? Is 32k clock the slowest that can be >>>>> fed to the PHY as a suspend clock? >>>> Yes, 32K clock is the slowest, Per DWC3 document on Power Down Scale >>>> (bits 31:19 of GCTL): >>>> >>>> "Power Down Scale (PwrDnScale) >>>> The USB3 suspend_clk input replaces pipe3_rx_pclk as a clock source to >>>> a small part of the USB3 controller that operates when the SS PHY is >>>> in its lowest power (P3) state, and therefore does not provide a clock. >>>> The Power Down Scale field specifies how many suspend_clk periods fit >>>> into a 16 kHz clock period. When performing the division, round up the >>>> remainder. >>>> For example, when using an 8-bit/16-bit/32-bit PHY and 25-MHz Suspend >>>> clock, Power Down Scale = 25000 kHz/16 kHz = 13'd1563 (rounder up) >>>> Note: >>>> - Minimum Suspend clock frequency is 32 kHz >>>> - Maximum Suspend clock frequency is 125 MHz" >>> Cool, now do we have an upper bound for how many clock cycles it takes to >>> wake up >>> the PHY? >> My understanding is this ep command does not wake up the SS PHY, >> the SS PHY still stays at P3 when execute this ep command. The time >> required here is to wait controller complete something for this ep >> command with 32K clock. > Sorry I made a mistake. You're right. Just checked with one of the RTL > engineers, and it doesn't need to wake up the phy. However, if it is eSS > speed, it may take longer time as the command may be completing with the > suspend clock. > What's the value for GCTL[7:6]? BR, Thinh
Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
Jun Li wrote: >> -Original Message- >> From: Felipe Balbi On Behalf Of Felipe Balbi >> Sent: 2020年5月16日 19:57 >> To: Jun Li ; Thinh Nguyen ; Jun Li >> >> Cc: John Stultz ; lkml >> ; Yu >> Chen ; Greg Kroah-Hartman ; >> Rob >> Herring ; Mark Rutland ; ShuFan Lee >> ; Heikki Krogerus ; >> Suzuki K Poulose ; Chunfeng Yun >> ; Hans de Goede ; Andy >> Shevchenko >> ; Valentin Schneider ; >> Jack Pham ; Linux USB List >> ; open >> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS >> ; >> Peter Chen ; Thinh Nguyen >> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared >> by device >> controller >> >> >> Hi, >> >> Jun Li writes: >>>>>> Hi Thinh, could you comment this? >>>>> You only need to wake up the usb2 phy when issuing the command >>>>> while running in highspeed or below. If you're running in SS or >>>>> higher, internally the controller does it for you for usb3 phy. In >>>>> Jun's case, it seems like it takes longer for his phy to wake up. >>>>> >>>>> IMO, in this case, I think it's fine to increase the command timeout. >>>> Is there an upper limit to this? Is 32k clock the slowest that can be >>>> fed to the PHY as a suspend clock? >>> Yes, 32K clock is the slowest, Per DWC3 document on Power Down Scale >>> (bits 31:19 of GCTL): >>> >>> "Power Down Scale (PwrDnScale) >>> The USB3 suspend_clk input replaces pipe3_rx_pclk as a clock source to >>> a small part of the USB3 controller that operates when the SS PHY is >>> in its lowest power (P3) state, and therefore does not provide a clock. >>> The Power Down Scale field specifies how many suspend_clk periods fit >>> into a 16 kHz clock period. When performing the division, round up the >>> remainder. >>> For example, when using an 8-bit/16-bit/32-bit PHY and 25-MHz Suspend >>> clock, Power Down Scale = 25000 kHz/16 kHz = 13'd1563 (rounder up) >>> Note: >>> - Minimum Suspend clock frequency is 32 kHz >>> - Maximum Suspend clock frequency is 125 MHz" >> Cool, now do we have an upper bound for how many clock cycles it takes to >> wake up >> the PHY? > My understanding is this ep command does not wake up the SS PHY, > the SS PHY still stays at P3 when execute this ep command. The time > required here is to wait controller complete something for this ep > command with 32K clock. Sorry I made a mistake. You're right. Just checked with one of the RTL engineers, and it doesn't need to wake up the phy. However, if it is eSS speed, it may take longer time as the command may be completing with the suspend clock. BR, Thinh > >> Then we can just set the time to that upper bound. > Per my test with trace, the time is about 400us(~13 cycles). > > Thanks > Li Jun >> -- >> balbi
Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
Hi,
Jun Li wrote:
>> -Original Message-
>> From: Felipe Balbi On Behalf Of Felipe Balbi
>> Sent: 2020年5月15日 17:31
>> To: Jun Li
>> Cc: John Stultz ; lkml
>> ; Yu
>> Chen ; Greg Kroah-Hartman ;
>> Rob
>> Herring ; Mark Rutland ; ShuFan Lee
>> ; Heikki Krogerus ;
>> Suzuki K Poulose ; Chunfeng Yun
>> ; Hans de Goede ; Andy
>> Shevchenko
>> ; Valentin Schneider ;
>> Jack Pham ; Linux USB List
>> ; open
>> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
>> ;
>> Peter Chen ; Jun Li ; Thinh Nguyen
>>
>> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared
>> by device
>> controller
>>
>>
>> Hi,
>>
>> Jun Li writes:
>>>> @@ -397,12 +407,18 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep,
>>>> unsigned
>> cmd,
>>>> dwc3_gadget_ep_get_transfer_index(dep);
>>>> }
>>>>
>>>> - if (saved_config) {
>>>> + if (saved_hs_config) {
>>>> reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
>>>> - reg |= saved_config;
>>>> + reg |= saved_hs_config;
>>>> dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
>>>> }
>>>>
>>>> + if (saved_ss_config) {
>>>> + reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
>>>> + reg |= saved_ss_config;
>>>> + dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
>>>> + }
>>>> +
>>>> return ret;
>>>> }
>>> Unfortunately this way can't work, once the SS PHY enters P3, disable
>>> suspend_en can't force SS PHY exit P3, unless do this at the very
>>> beginning to prevent SS PHY entering P3(e.g. add "snps,dis_u3_susphy_quirk"
>>> for
>> test).
>>
>> It sounds like you have a quirky PHY.
> From what I got from the IC design, the behavior of DWC3_GUSB3PIPECTL_SUSPHY
> bit should be as what I said, not a quirky.
>
> Hi Thinh, could you comment this?
You only need to wake up the usb2 phy when issuing the command while
running in highspeed or below. If you're running in SS or higher,
internally the controller does it for you for usb3 phy. In Jun's case,
it seems like it takes longer for his phy to wake up.
IMO, in this case, I think it's fine to increase the command timeout.
BR,
Thinh
Re: [PATCH 3/3] usb: dwc3: gadget: Add support for disabling U1 and U2 entries
Hi, Anurag Kumar Vulisha wrote: > Hi Claus, > >> -Original Message- >> From: Claus H. Stovgaard [mailto:c...@phaseone.com] >> Sent: Tuesday, May 07, 2019 2:28 AM >> To: Thinh Nguyen ; Anurag Kumar Vulisha >> ; Greg Kroah-Hartman ; Rob >> Herring ; Mark Rutland ; Felipe >> Balbi >> >> Cc: linux-...@vger.kernel.org; devicet...@vger.kernel.org; linux- >> ker...@vger.kernel.org; v.anuragku...@gmail.com >> Subject: Re: [PATCH 3/3] usb: dwc3: gadget: Add support for disabling U1 and >> U2 >> entries >> >> Hi Thinh and Anurag >> >> On man, 2019-05-06 at 19:21 +, Thinh Nguyen wrote: >> >>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index >>>> a1b126f..4f0912c 100644 >>>> --- a/drivers/usb/dwc3/core.c >>>> +++ b/drivers/usb/dwc3/core.c >>>> @@ -1285,6 +1285,10 @@ static void dwc3_get_properties(struct dwc3 >>>> *dwc) >>>>"snps,dis_u2_susphy_quirk"); >>>>dwc->dis_enblslpm_quirk = device_property_read_bool(dev, >>>>"snps,dis_enblslpm_quirk"); >>>> + dwc->dis_u1_entry_quirk = device_property_read_bool(dev, >>>> + "snps,dis_u1_entry_quirk"); >>>> + dwc->dis_u2_entry_quirk = device_property_read_bool(dev, >>>> + "snps,dis_u2_entry_quirk"); >>> Please use "-" rather than "_" in the property names. >> I have thought about this feature over the weekend, and think the naming >> should be >> changed to something like "snps,bos-u1-exit-lat-in-us" >> and named the same in the code. And then be the value used by the >> get_config_params. E.g. the device-tree is used to set the values directly >> used for >> bUxdevExitLat instead of named something not related to exit latency. >> >> With this the name and function is a 1 to 1 match, and you can among others >> set it to >> 0 for optaining what Anurag wants. >> > Your suggestion looks good but the problem is the U1 and U2 exit latencies are > fixed values in dwc3 controller(can be found in HCSPARAMS3). Adding different > exit latencies may modify the U1SEL/U2SEL values sent from the host but the > real > dwc3 controller exit latencies are not getting changed. Because of this > reason I > had opted "snps,dis_u1_entry_quirk", so that the U1/U2 exit latency values > reported in BOS descriptor can be either be zero (when U1/U2 entries needs to > be > disabled) or non-zero value (reported in HCSPARAMS3) when U1/U2 states > allowed. > Based on this I think it is better if we can continue with > "snps,dis-u1-entry-quirk" > instead of the "snps,bos-u1-exit-lat-in-us". Please provide your opinion on > this. > >> Regarding the disabling of U1 / U2. I send this to Anurag >> https://urldefense.proofpoint.com/v2/url?u=https-3A__marc.info_-3Fl-3Dlinux-2Dusb-26m-3D155683299311954-26w-3D2=DwIGaQ=DPL6_X_6JkXFx7AXWqB0tg=u9FYoxKtyhjrGFcyixFYqTjw1ZX0VsG2d8FCmzkTY-w=MBQpZmX-jgrlpT68k5VR-4xv_DYb5UGUiD5objMqwpA=Ca-zBV5t26-ZFPbNAkD8K3F3lbc3CwUXNpAgnkVasg4= >> Here i created a configfs interface with the names "lpm_U1_disable" and >> "lpm_U2_disable" for controlling the DTCL of dwc3, and reject >> SET_FEATURE(U1/U2) >> >> Will send this in seperate patch tomorrow, in the hope that Anurags feature >> can >> become a way for controlling exit latency, and my patch become a way for >> disabling >> U1/U2 >> > I agree with your suggestion. When U1 and U2 entries are not allowed it is > always > better to report zero value for U1/U2 exit latencies in BOS descriptor (no > point in > reporting non-zero exit latency values when U1/U2 states are not allowed). > Along > with that changes for preventing the dwc3 controller from initiating or > accepting > U1/U2 requests are also required (since there are some host platforms where > sending > 0 exit latency doesn't work). Based on these observations I believe both your > patch > changes and my patch changes needs to be added. > > @Thinh Nguyen > Please provide your opinion on this > The 0 exit latency in the BOS descriptor doesn't mean that device doesn't support U1/U2 (however unrealistic it may seem). The exit latency values reported in the BOS decriptor are just recommended latency. The host controls over what they should be (host has its own U1/U2 exit latency). I don't think we should have a device property to set the device exit latency. If the gadget driver needs to know what the recommended latency to set in the BOS descriptor, we can have those values exported to some new fields in the usb_gadget structure. BR, Thinh
Re: [PATCH 3/3] usb: dwc3: gadget: Add support for disabling U1 and U2 entries
Hi Anurag,
Anurag Kumar Vulisha wrote:
> Gadget applications may have a requirement to disable the U1 and U2
> entry based on the usecase. For example, when performing performance
> benchmarking on mass storage gadget the U1 and U2 entries can be disabled.
> Another example is when periodic transfers like ISOC transfers are used
> with bInterval of 1 which doesn't require the link to enter into U1 or U2
> state (since ping is issued from host for every uframe interval). In this
> case the U1 and U2 entry can be disabled. This can be done by setting
> U1DevExitLat and U2DevExitLat values to 0 in the BOS descriptor. Host on
> seeing 0 value for U1DevExitLat and U2DevExitLat, it doesn't send SET_SEL
> commands to the gadget. Thus entry of U1 and U2 states can be avioded.
> This patch updates the same
I don't think we should assume that's the case for every host driver.
>
> Signed-off-by: Anurag Kumar Vulisha
> ---
> drivers/usb/dwc3/core.c | 4
> drivers/usb/dwc3/core.h | 4
> drivers/usb/dwc3/gadget.c | 19 +++
> drivers/usb/dwc3/gadget.h | 6 ++
> 4 files changed, 33 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index a1b126f..4f0912c 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1285,6 +1285,10 @@ static void dwc3_get_properties(struct dwc3 *dwc)
> "snps,dis_u2_susphy_quirk");
> dwc->dis_enblslpm_quirk = device_property_read_bool(dev,
> "snps,dis_enblslpm_quirk");
> + dwc->dis_u1_entry_quirk = device_property_read_bool(dev,
> + "snps,dis_u1_entry_quirk");
> + dwc->dis_u2_entry_quirk = device_property_read_bool(dev,
> + "snps,dis_u2_entry_quirk");
Please use "-" rather than "_" in the property names.
> dwc->dis_rxdet_inp3_quirk = device_property_read_bool(dev,
> "snps,dis_rxdet_inp3_quirk");
> dwc->dis_u2_freeclk_exists_quirk = device_property_read_bool(dev,
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 1528d39..fa398e2 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1015,6 +1015,8 @@ struct dwc3_scratchpad_array {
> * @dis_u2_susphy_quirk: set if we disable usb2 suspend phy
> * @dis_enblslpm_quirk: set if we clear enblslpm in GUSB2PHYCFG,
> * disabling the suspend signal to the PHY.
> + * @dis_u1_entry_quirk: set if link entering into U1 state needs to be
> disabled.
> + * @dis_u2_entry_quirk: set if link entering into U2 state needs to be
> disabled.
> * @dis_rxdet_inp3_quirk: set if we disable Rx.Detect in P3
> * @dis_u2_freeclk_exists_quirk : set if we clear u2_freeclk_exists
> * in GUSB2PHYCFG, specify that USB2 PHY doesn't
> @@ -1206,6 +1208,8 @@ struct dwc3 {
> unsigneddis_u3_susphy_quirk:1;
> unsigneddis_u2_susphy_quirk:1;
> unsigneddis_enblslpm_quirk:1;
> + unsigneddis_u1_entry_quirk:1;
> + unsigneddis_u2_entry_quirk:1;
> unsigneddis_rxdet_inp3_quirk:1;
> unsigneddis_u2_freeclk_exists_quirk:1;
> unsigneddis_del_phy_power_chg_quirk:1;
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index e293400..f2d3112 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2073,6 +2073,24 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
> return 0;
> }
>
> +static void dwc3_gadget_config_params(struct usb_gadget *g,
> + struct usb_dcd_config_params *params)
> +{
> + struct dwc3 *dwc = gadget_to_dwc(g);
> +
> + /* U1 Device exit Latency */
> + if (dwc->dis_u1_entry_quirk)
> + params->bU1devExitLat = 0;
> + else
> + params->bU1devExitLat = DWC3_DEFAULT_U1_DEV_EXIT_LAT;
> +
> + /* U2 Device exit Latency */
> + if (dwc->dis_u2_entry_quirk)
> + params->bU2DevExitLat = 0;
> + else
> + params->bU2DevExitLat = DWC3_DEFAULT_U2_DEV_EXIT_LAT;
> +}
> +
> static void dwc3_gadget_set_speed(struct usb_gadget *g,
> enum usb_device_speed speed)
> {
> @@ -2142,6 +2160,7 @@ static const struct usb_gadget_ops dwc3_gadget_ops = {
> .udc_start = dwc3_gadget_start,
> .udc_stop = dwc3_gadget_stop,
> .udc_set_speed = dwc3_gadget_set_speed,
> + .get_config_params = dwc3_gadget_config_params,
> };
>
> /*
> -- */
> diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h
> index 3ed738e..5faf4d1 100644
> --- a/drivers/usb/dwc3/gadget.h
> +++ b/drivers/usb/dwc3/gadget.h
> @@ -48,6 +48,12 @@ struct dwc3;
> /* DEPXFERCFG
Re: Linux Kernel Regression: HAPS quirk breaks PCIe on i.MX6QP
Thinh Nguyen wrote:
> Bjorn Helgaas wrote:
>> On Tue, Feb 05, 2019 at 08:38:58PM +0000, Thinh Nguyen wrote:
>>> Hi Bjorn,
>>>
>>> Bjorn Helgaas wrote:
>>>> On Fri, Feb 01, 2019 at 08:27:00PM +, Thinh Nguyen wrote:
>>>>> Lukas Wunner wrote:
>>>>>> On Thu, Jan 31, 2019 at 11:46:23PM +, Thinh Nguyen wrote:
>>>>>>> --- a/drivers/pci/quirks.c
>>>>>>> +++ b/drivers/pci/quirks.c
>>>>>>> @@ -629,6 +629,9 @@ static void quirk_synopsys_haps(struct pci_dev
>>>>>>> *pdev)
>>>>>>> {
>>>>>>> u32 class = pdev->class;
>>>>>>>
>>>>>>> + if (class != PCI_CLASS_SERIAL_USB_XHCI)
>>>>>>> + return;
>>>>>>> +
>>>>>>> switch (pdev->device) {
>>>>>>> case PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3:
>>>>>>> case PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3_AXI:
>>>>>> Please use DECLARE_PCI_FIXUP_CLASS_HEADER() instead.
>>>>> Sure. That's a better option. Can you test this with your setup?
>>>>>
>>>>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>>>>> index b0a413f3f7ca..f46e7de9e15d 100644
>>>>> --- a/drivers/pci/quirks.c
>>>>> +++ b/drivers/pci/quirks.c
>>>>> @@ -639,8 +639,8 @@ static void quirk_synopsys_haps(struct pci_dev *pdev)
>>>>> break;
>>>>> }
>>>>> }
>>>>> -DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SYNOPSYS, PCI_ANY_ID,
>>>>> -quirk_synopsys_haps);
>>>>> +DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_SYNOPSYS, PCI_ANY_ID,
>>>>> + PCI_CLASS_SERIAL_USB_XHCI, 0, quirk_synopsys_haps);
>>>>>
>>>>> /*
>>>>> * Let's make the southbridge information explicit instead of having to
>>>>>
>>>>>
>>>> Can we get a formal patch, including details about the issue (I assume
>>>> Synopsys released two different parts with Device ID 0xabcd) and a
>>>> signed-off-by?
>>>>
>>>> I'd like to get this into for-linus as soon as possible for v5.0.
>>> I already submitted a patch for this. Please review patch subject
>>> "[PATCH] PCI: Check for USB xHCI class for HAPS platform".
>> OK, maybe I'm missing something. I don't see it on the linux-pci list
>> [1] or the PCI patchwork [2] yet. Maybe it's still making its way
>> through the mailing list servers. If you have a URL to the archive or
>> patchwork, let me know.
>>
>> Bjorn
>>
>> [1]
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_linux-2Dpci_=DwIBAg=DPL6_X_6JkXFx7AXWqB0tg=u9FYoxKtyhjrGFcyixFYqTjw1ZX0VsG2d8FCmzkTY-w=gf8tJEWOt92UvWl-0yPhkFlhOXuG0fn-pG0zPdqsHv8=vQgsCLqN63Bd-d7ZMY5HPVmBa4Htnz64oO96cG-6pNA=
>> [2]
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.ozlabs.org_project_linux-2Dpci_list=DwIBAg=DPL6_X_6JkXFx7AXWqB0tg=u9FYoxKtyhjrGFcyixFYqTjw1ZX0VsG2d8FCmzkTY-w=gf8tJEWOt92UvWl-0yPhkFlhOXuG0fn-pG0zPdqsHv8=Tbm0SOa0CCxMxzGcwT6YJrYYzVEU6x7sR8JB9l82VGg=
After I removed some names in the CC list, it looks like it appears on
PCI patchwork now.
https://patchwork.ozlabs.org/patch/1037211/
Thanks,
Thinh
Re: Linux Kernel Regression: HAPS quirk breaks PCIe on i.MX6QP
Bjorn Helgaas wrote:
> On Tue, Feb 05, 2019 at 08:38:58PM +0000, Thinh Nguyen wrote:
>> Hi Bjorn,
>>
>> Bjorn Helgaas wrote:
>>> On Fri, Feb 01, 2019 at 08:27:00PM +, Thinh Nguyen wrote:
>>>> Lukas Wunner wrote:
>>>>> On Thu, Jan 31, 2019 at 11:46:23PM +, Thinh Nguyen wrote:
>>>>>> --- a/drivers/pci/quirks.c
>>>>>> +++ b/drivers/pci/quirks.c
>>>>>> @@ -629,6 +629,9 @@ static void quirk_synopsys_haps(struct pci_dev *pdev)
>>>>>> {
>>>>>> u32 class = pdev->class;
>>>>>>
>>>>>> + if (class != PCI_CLASS_SERIAL_USB_XHCI)
>>>>>> + return;
>>>>>> +
>>>>>> switch (pdev->device) {
>>>>>> case PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3:
>>>>>> case PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3_AXI:
>>>>> Please use DECLARE_PCI_FIXUP_CLASS_HEADER() instead.
>>>> Sure. That's a better option. Can you test this with your setup?
>>>>
>>>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>>>> index b0a413f3f7ca..f46e7de9e15d 100644
>>>> --- a/drivers/pci/quirks.c
>>>> +++ b/drivers/pci/quirks.c
>>>> @@ -639,8 +639,8 @@ static void quirk_synopsys_haps(struct pci_dev *pdev)
>>>> break;
>>>> }
>>>> }
>>>> -DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SYNOPSYS, PCI_ANY_ID,
>>>> -quirk_synopsys_haps);
>>>> +DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_SYNOPSYS, PCI_ANY_ID,
>>>> + PCI_CLASS_SERIAL_USB_XHCI, 0, quirk_synopsys_haps);
>>>>
>>>> /*
>>>> * Let's make the southbridge information explicit instead of having to
>>>>
>>>>
>>> Can we get a formal patch, including details about the issue (I assume
>>> Synopsys released two different parts with Device ID 0xabcd) and a
>>> signed-off-by?
>>>
>>> I'd like to get this into for-linus as soon as possible for v5.0.
>> I already submitted a patch for this. Please review patch subject
>> "[PATCH] PCI: Check for USB xHCI class for HAPS platform".
> OK, maybe I'm missing something. I don't see it on the linux-pci list
> [1] or the PCI patchwork [2] yet. Maybe it's still making its way
> through the mailing list servers. If you have a URL to the archive or
> patchwork, let me know.
>
> Bjorn
>
> [1]
> https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_linux-2Dpci_=DwIBAg=DPL6_X_6JkXFx7AXWqB0tg=u9FYoxKtyhjrGFcyixFYqTjw1ZX0VsG2d8FCmzkTY-w=gf8tJEWOt92UvWl-0yPhkFlhOXuG0fn-pG0zPdqsHv8=vQgsCLqN63Bd-d7ZMY5HPVmBa4Htnz64oO96cG-6pNA=
> [2]
> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.ozlabs.org_project_linux-2Dpci_list=DwIBAg=DPL6_X_6JkXFx7AXWqB0tg=u9FYoxKtyhjrGFcyixFYqTjw1ZX0VsG2d8FCmzkTY-w=gf8tJEWOt92UvWl-0yPhkFlhOXuG0fn-pG0zPdqsHv8=Tbm0SOa0CCxMxzGcwT6YJrYYzVEU6x7sR8JB9l82VGg=
>
That's odd. I sent it yesterday. I'll resend it then.
Thanks,
Thinh
Re: Linux Kernel Regression: HAPS quirk breaks PCIe on i.MX6QP
Hi Bjorn,
Bjorn Helgaas wrote:
> On Fri, Feb 01, 2019 at 08:27:00PM +0000, Thinh Nguyen wrote:
>> Lukas Wunner wrote:
>>> On Thu, Jan 31, 2019 at 11:46:23PM +, Thinh Nguyen wrote:
>>>> --- a/drivers/pci/quirks.c
>>>> +++ b/drivers/pci/quirks.c
>>>> @@ -629,6 +629,9 @@ static void quirk_synopsys_haps(struct pci_dev *pdev)
>>>> {
>>>> u32 class = pdev->class;
>>>>
>>>> + if (class != PCI_CLASS_SERIAL_USB_XHCI)
>>>> + return;
>>>> +
>>>> switch (pdev->device) {
>>>> case PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3:
>>>> case PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3_AXI:
>>> Please use DECLARE_PCI_FIXUP_CLASS_HEADER() instead.
>> Sure. That's a better option. Can you test this with your setup?
>>
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index b0a413f3f7ca..f46e7de9e15d 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -639,8 +639,8 @@ static void quirk_synopsys_haps(struct pci_dev *pdev)
>> break;
>> }
>> }
>> -DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SYNOPSYS, PCI_ANY_ID,
>> -quirk_synopsys_haps);
>> +DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_SYNOPSYS, PCI_ANY_ID,
>> + PCI_CLASS_SERIAL_USB_XHCI, 0, quirk_synopsys_haps);
>>
>> /*
>> * Let's make the southbridge information explicit instead of having to
>>
>>
> Can we get a formal patch, including details about the issue (I assume
> Synopsys released two different parts with Device ID 0xabcd) and a
> signed-off-by?
>
> I'd like to get this into for-linus as soon as possible for v5.0.
>
I already submitted a patch for this. Please review patch subject
"[PATCH] PCI: Check for USB xHCI class for HAPS platform".
Thanks,
Thinh
Re: Frequent dwc3 crashes on suspend or reboot since 5.0-rc1
John Stultz wrote:
> On Fri, Feb 1, 2019 at 4:46 PM Thinh Nguyen wrote:
>> John Stultz wrote:
>>> On Fri, Feb 1, 2019 at 4:18 PM John Stultz wrote:
>>> Bisecting the changes down, it seems like its due to commit
>>> fec9095bdef4e ("usb: dwc3: gadget: remove wait_end_transfer").
>>>
>>> It doesn't happen all the time, so I'll need to run some more testing,
>>> but so far I've not been able to trigger it backing out the patches to
>>> that point.
>> Yeah, it sounds like the same issue. You can review the discussion here:
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.spinics.net_lists_linux-2Dusb_msg176110.html=DwIBaQ=DPL6_X_6JkXFx7AXWqB0tg=u9FYoxKtyhjrGFcyixFYqTjw1ZX0VsG2d8FCmzkTY-w=NafAczsBrlszpsknN68eiuJOr8zYyB34O0R9NCF3-pc=ExaEPcrEUReFA79ZWRHG9MELb9eead_QxKiTu1ea8Eg=
> Unfortunately, merging in
> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_balbi_usb.git=DwIBaQ=DPL6_X_6JkXFx7AXWqB0tg=u9FYoxKtyhjrGFcyixFYqTjw1ZX0VsG2d8FCmzkTY-w=NafAczsBrlszpsknN68eiuJOr8zYyB34O0R9NCF3-pc=VLaXOrfcXp-6oljE2fC9rBEsPfYTZ6CxKqiQOhW_QKE=
> testing/next seems to trigger a different issue:
>
> [ 38.585141] OOM killer enabled.
> [ 38.585143] Restarting tasks ...
> [ 38.585874] [ cut here ]
> [ 38.585882] ep1out: request already in flight
> [ 38.585944] WARNING: CPU: 7 PID: 2545 at
> drivers/usb/dwc3/gadget.c:1430 dwc3_gadget_ep_queue+0x1d4/0x200
> [ 38.585946] Modules linked in:
> [ 38.585960] CPU: 7 PID: 2545 Comm: adbd Tainted: G S
> 5.0.0-rc4-00110-gad0e691 #509
> [ 38.585963] Hardware name: HiKey960 (DT)
> [ 38.585968] pstate: 80400085 (Nzcv daIf +PAN -UAO)
> [ 38.585972] pc : dwc3_gadget_ep_queue+0x1d4/0x200
> [ 38.585976] lr : dwc3_gadget_ep_queue+0x1d4/0x200
> [ 38.585978] sp : ff80149fbb40
It looks like the gadget driver tried to queue request twice.
> Let me know if you have other ideas to try!
>
>
Try to revert this patches series (3 patches) from Felipe testing/next
to see if that helps:
https://patchwork.kernel.org/patch/10764297/
Thinh
Re: Frequent dwc3 crashes on suspend or reboot since 5.0-rc1
Hi John,
John Stultz wrote:
> On Fri, Feb 1, 2019 at 4:18 PM John Stultz wrote:
>> Hey all,
>> Since the 5.0 merge window opened, I've been tripping on frequent
>> dwc3 crashes on reboot and suspend, which I've added an example to the
>> bottom of this mail.
>>
>> I've dug in a little bit and sort of have a sense of whats going on.
>>
>> In ffs_epfile_io():
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_torvalds_linux.git_tree_drivers_usb_gadget_function_f-5Ffs.c-23n1065=DwIBaQ=DPL6_X_6JkXFx7AXWqB0tg=u9FYoxKtyhjrGFcyixFYqTjw1ZX0VsG2d8FCmzkTY-w=Ikgcuoe1TJkip3EVA2Cce33perU7WerY9a24BCFW4DM=3gJjzpAGPdj79ROPvlM1ziRTY-4u6VRFRwKWbz5X_SA=
>>
>> The completion done is setup on the stack:
>> DECLARE_COMPLETION_ONSTACK(done);
>>
>> Then later we setup a request and queue it:
>> req->context =
>> ...
>> ret = usb_ep_queue(ep->ep, req, GFP_ATOMIC);
>>
>> Then wait for it:
>> if (unlikely(wait_for_completion_interruptible())) {
>> /*
>> * To avoid race condition with ffs_epfile_io_complete,
>> * dequeue the request first then check
>> * status. usb_ep_dequeue API should guarantee no race
>> * condition with req->complete callback.
>> */
>> usb_ep_dequeue(ep->ep, req);
>> interrupted = ep->status < 0;
>> }
>>
>> The problem is, that we end up being interrupted, supposedly dequeue
>> the request, and exit.
>>
>> But then (or in parallel) the irq triggers and we try calling
>> complete() on the context pointer which points to now random stack
>> space, which results in the panic.
>>
>> It seems like something is wrong with usb_ep_dequeue not really
>> stopping the irq from happening?
>>
>> If I revert all the changes to dwc3 back to 4.20, I don't see the issue.
>>
>> I'll do some bisection to try to narrow things down, but I wanted to
>> see if this was a known issue or if anyone had immediate ideas as to
>> what might be wrong.
> Bisecting the changes down, it seems like its due to commit
> fec9095bdef4e ("usb: dwc3: gadget: remove wait_end_transfer").
>
> It doesn't happen all the time, so I'll need to run some more testing,
> but so far I've not been able to trigger it backing out the patches to
> that point.
>
> thanks
> -john
>
Yeah, it sounds like the same issue. You can review the discussion here:
https://www.spinics.net/lists/linux-usb/msg176110.html
Thinh
Re: Frequent dwc3 crashes on suspend or reboot since 5.0-rc1
Hi John,
John Stultz wrote:
> Hey all,
> Since the 5.0 merge window opened, I've been tripping on frequent
> dwc3 crashes on reboot and suspend, which I've added an example to the
> bottom of this mail.
>
> I've dug in a little bit and sort of have a sense of whats going on.
>
> In ffs_epfile_io():
> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_torvalds_linux.git_tree_drivers_usb_gadget_function_f-5Ffs.c-23n1065=DwIBaQ=DPL6_X_6JkXFx7AXWqB0tg=u9FYoxKtyhjrGFcyixFYqTjw1ZX0VsG2d8FCmzkTY-w=a8TU-itM8GBG_EARYf2yM-kVfCzmaPkKDNAUFQHTe3Q=BQiVAFiViSlxVg5_LemED0x_47FLVUD43M7R6h6T8qk=
>
> The completion done is setup on the stack:
> DECLARE_COMPLETION_ONSTACK(done);
>
> Then later we setup a request and queue it:
> req->context =
> ...
> ret = usb_ep_queue(ep->ep, req, GFP_ATOMIC);
>
> Then wait for it:
> if (unlikely(wait_for_completion_interruptible())) {
> /*
> * To avoid race condition with ffs_epfile_io_complete,
> * dequeue the request first then check
> * status. usb_ep_dequeue API should guarantee no race
> * condition with req->complete callback.
> */
> usb_ep_dequeue(ep->ep, req);
> interrupted = ep->status < 0;
> }
>
> The problem is, that we end up being interrupted, supposedly dequeue
> the request, and exit.
>
> But then (or in parallel) the irq triggers and we try calling
> complete() on the context pointer which points to now random stack
> space, which results in the panic.
>
> It seems like something is wrong with usb_ep_dequeue not really
> stopping the irq from happening?
>
> If I revert all the changes to dwc3 back to 4.20, I don't see the issue.
>
> I'll do some bisection to try to narrow things down, but I wanted to
> see if this was a known issue or if anyone had immediate ideas as to
> what might be wrong.
>
I'm not sure if this is related, but can you try to test using Felipe's
testing/next branch? There is a fix to a race condition when the gadget
driver tries to dequeue requests.
See if you run into this issue again.
Thanks,
Thinh
Re: Linux Kernel Regression: HAPS quirk breaks PCIe on i.MX6QP
Hi Lukas,
Lukas Wunner wrote:
> On Thu, Jan 31, 2019 at 11:46:23PM +0000, Thinh Nguyen wrote:
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -629,6 +629,9 @@ static void quirk_synopsys_haps(struct pci_dev *pdev)
>> {
>> u32 class = pdev->class;
>>
>> + if (class != PCI_CLASS_SERIAL_USB_XHCI)
>> + return;
>> +
>> switch (pdev->device) {
>> case PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3:
>> case PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3_AXI:
> Please use DECLARE_PCI_FIXUP_CLASS_HEADER() instead.
Sure. That's a better option. Can you test this with your setup?
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index b0a413f3f7ca..f46e7de9e15d 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -639,8 +639,8 @@ static void quirk_synopsys_haps(struct pci_dev *pdev)
break;
}
}
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_SYNOPSYS, PCI_ANY_ID,
-quirk_synopsys_haps);
+DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_SYNOPSYS, PCI_ANY_ID,
+ PCI_CLASS_SERIAL_USB_XHCI, 0, quirk_synopsys_haps);
/*
* Let's make the southbridge information explicit instead of having to
Thanks,
Thinh
Re: Linux Kernel Regression: HAPS quirk breaks PCIe on i.MX6QP
Hi Lukas,
Lukas Hartmann wrote:
> Hi Thinh,
>
> this is in a taped out i.MX6QP 1.2GHz SoC, I also confirmed it with the
> 1.0GHz version, it has the same 0xabcd device id integrated.
I see.
> I will try your patch ASAP.
Thank you.
Thinh
>
>> On 1. Feb 2019, at 00:46, Thinh Nguyen wrote:
>>
>> Hi Lukas,
>>
>> Thinh Nguyen wrote:
>>> Bjorn Helgaas wrote:
>>>> [+cc linux-pci, linux-kernel]
>>>>
>>>>> On Thu, Jan 31, 2019 at 11:21 AM Lukas F. Hartmann
>>>>> wrote:
>>>>> Hi Thinh,
>>>>>
>>>>> I'm writing you because you're the author in this commit:
>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_torvalds_linux_commit_03e6742584af8866ba014874669d0440bed3a623=DwIBaQ=DPL6_X_6JkXFx7AXWqB0tg=VKipRJmm95P2RbIxyKKYrcUCOGNlQtjlV-5zhrVhIik=2sOrowYXlsC3rl0LfHQhIZzImag0jFZXGvR6NIQDsh8=oJoBWRE8_LGAYbX2alh6QnjYZTTmzcgLw4MtOMToNyo=
>>>>>
>>>>> This quirk workaround breaks the PCIe on i.MX6QP, at least on my test
>>>>> devices. The reason is because the Synposys PCIe IP in i.MX6QP has the
>>>>> same device ID as the HAPS USB3 controller you are targeting in the
>>>>> commit: 0xabcd.
>>>>>
>>>>> Definition:
>>>>>
>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_torvalds_linux_blob_master_include_linux_pci-5Fids.h-23L2364=DwIBaQ=DPL6_X_6JkXFx7AXWqB0tg=VKipRJmm95P2RbIxyKKYrcUCOGNlQtjlV-5zhrVhIik=2sOrowYXlsC3rl0LfHQhIZzImag0jFZXGvR6NIQDsh8=-FVCwe81XNjJsYHYk1w-kdAzSOLunTGeNc73azO2QYw=
>>>>>
>>>>> Here is a bit of lspci output on my i.MX6QP machine (without the problem):
>>>>>
>>>>> mntmn@reform:~/code/linux$ lspci -v
>>>>> 00:00.0 PCI bridge: Synopsys, Inc. Device abcd (rev 01) (prog-if 00
>>>>>
>>>>> [Normal decode])
>>>>>Flags: bus master, fast devsel, latency 0, IRQ 307
>>>>>Memory at 0100 (32-bit, non-prefetchable) [size=1M]
>>>>>Bus: primary=00, secondary=01, subordinate=ff, sec-latency=0
>>>>> ...
>>>>>
>>>>> The failure mode is that the PCIe controller shows up as a USB
>>>>> controller and my ath9k wireless PCIe card cannot be assigned the
>>>>> proper resources anymore (-ENOMEM even for very small BARs).
>>>>>
>>>>> Reverting this commit fixes the problem for me. I suggest to make the
>>>>> check more specific to the platform/chipset you are targeting.
>>>> So Synopsys apparently re-used Device ID 0xabcd? That's a pretty bad
>>>> problem.
>>>>
>>>> It looks like we merged 03e6742584af ("PCI: Override Synopsys USB 3.x
>>>> HAPS device class") for v5.0, and v5.0-final hasn't been released yet,
>>>> so if we don't hear from Thinh with a resolution, we can still revert
>>>> it.
>>>>
>>>> I set myself a reminder to revert this on Feb 11, but hopefully we'll
>>>> have a better resolution before then.
>>>>
>>>> Bjorn
>>>>
>>> This is really odd that the PID for the PCIe controller in i.MX6QP is
>>> the same as PID Synopsys use for USB controller. We use a different set
>>> of PIDs for PCIe controllers and track VID and PID in our releases.
>>>
>>> Is the Root Complex (00:00.0) part of the SoC? Or is this a Synopsys
>>> prototype connected to your board? If it is the latter, then the FPGA
>>> configuration needs to be updated to other PID.
>>>
Re: Linux Kernel Regression: HAPS quirk breaks PCIe on i.MX6QP
Hi Lukas,
Thinh Nguyen wrote:
> Bjorn Helgaas wrote:
>> [+cc linux-pci, linux-kernel]
>>
>> On Thu, Jan 31, 2019 at 11:21 AM Lukas F. Hartmann wrote:
>>> Hi Thinh,
>>>
>>> I'm writing you because you're the author in this commit:
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_torvalds_linux_commit_03e6742584af8866ba014874669d0440bed3a623=DwIBaQ=DPL6_X_6JkXFx7AXWqB0tg=VKipRJmm95P2RbIxyKKYrcUCOGNlQtjlV-5zhrVhIik=2sOrowYXlsC3rl0LfHQhIZzImag0jFZXGvR6NIQDsh8=oJoBWRE8_LGAYbX2alh6QnjYZTTmzcgLw4MtOMToNyo=
>>>
>>> This quirk workaround breaks the PCIe on i.MX6QP, at least on my test
>>> devices. The reason is because the Synposys PCIe IP in i.MX6QP has the
>>> same device ID as the HAPS USB3 controller you are targeting in the
>>> commit: 0xabcd.
>>>
>>> Definition:
>>>
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_torvalds_linux_blob_master_include_linux_pci-5Fids.h-23L2364=DwIBaQ=DPL6_X_6JkXFx7AXWqB0tg=VKipRJmm95P2RbIxyKKYrcUCOGNlQtjlV-5zhrVhIik=2sOrowYXlsC3rl0LfHQhIZzImag0jFZXGvR6NIQDsh8=-FVCwe81XNjJsYHYk1w-kdAzSOLunTGeNc73azO2QYw=
>>>
>>> Here is a bit of lspci output on my i.MX6QP machine (without the problem):
>>>
>>> mntmn@reform:~/code/linux$ lspci -v
>>> 00:00.0 PCI bridge: Synopsys, Inc. Device abcd (rev 01) (prog-if 00
>>>
>>> [Normal decode])
>>> Flags: bus master, fast devsel, latency 0, IRQ 307
>>> Memory at 0100 (32-bit, non-prefetchable) [size=1M]
>>> Bus: primary=00, secondary=01, subordinate=ff, sec-latency=0
>>> ...
>>>
>>> The failure mode is that the PCIe controller shows up as a USB
>>> controller and my ath9k wireless PCIe card cannot be assigned the
>>> proper resources anymore (-ENOMEM even for very small BARs).
>>>
>>> Reverting this commit fixes the problem for me. I suggest to make the
>>> check more specific to the platform/chipset you are targeting.
>> So Synopsys apparently re-used Device ID 0xabcd? That's a pretty bad
>> problem.
>>
>> It looks like we merged 03e6742584af ("PCI: Override Synopsys USB 3.x
>> HAPS device class") for v5.0, and v5.0-final hasn't been released yet,
>> so if we don't hear from Thinh with a resolution, we can still revert
>> it.
>>
>> I set myself a reminder to revert this on Feb 11, but hopefully we'll
>> have a better resolution before then.
>>
>> Bjorn
>>
> This is really odd that the PID for the PCIe controller in i.MX6QP is
> the same as PID Synopsys use for USB controller. We use a different set
> of PIDs for PCIe controllers and track VID and PID in our releases.
>
> Is the Root Complex (00:00.0) part of the SoC? Or is this a Synopsys
> prototype connected to your board? If it is the latter, then the FPGA
> configuration needs to be updated to other PID.
>
> We're investigating the workaround in case this is on a taped-out SoC.
Can you try to see if this will help?
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index b0a413f3f7ca..64623fbdd1e5 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -629,6 +629,9 @@ static void quirk_synopsys_haps(struct pci_dev *pdev)
{
u32 class = pdev->class;
+ if (class != PCI_CLASS_SERIAL_USB_XHCI)
+ return;
+
switch (pdev->device) {
case PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3:
case PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3_AXI:
Thanks,
Thinh
Re: Linux Kernel Regression: HAPS quirk breaks PCIe on i.MX6QP
Hi Lukas,
Bjorn Helgaas wrote:
> [+cc linux-pci, linux-kernel]
>
> On Thu, Jan 31, 2019 at 11:21 AM Lukas F. Hartmann wrote:
>> Hi Thinh,
>>
>> I'm writing you because you're the author in this commit:
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_torvalds_linux_commit_03e6742584af8866ba014874669d0440bed3a623=DwIBaQ=DPL6_X_6JkXFx7AXWqB0tg=VKipRJmm95P2RbIxyKKYrcUCOGNlQtjlV-5zhrVhIik=2sOrowYXlsC3rl0LfHQhIZzImag0jFZXGvR6NIQDsh8=oJoBWRE8_LGAYbX2alh6QnjYZTTmzcgLw4MtOMToNyo=
>>
>> This quirk workaround breaks the PCIe on i.MX6QP, at least on my test
>> devices. The reason is because the Synposys PCIe IP in i.MX6QP has the
>> same device ID as the HAPS USB3 controller you are targeting in the
>> commit: 0xabcd.
>>
>> Definition:
>>
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_torvalds_linux_blob_master_include_linux_pci-5Fids.h-23L2364=DwIBaQ=DPL6_X_6JkXFx7AXWqB0tg=VKipRJmm95P2RbIxyKKYrcUCOGNlQtjlV-5zhrVhIik=2sOrowYXlsC3rl0LfHQhIZzImag0jFZXGvR6NIQDsh8=-FVCwe81XNjJsYHYk1w-kdAzSOLunTGeNc73azO2QYw=
>>
>> Here is a bit of lspci output on my i.MX6QP machine (without the problem):
>>
>> mntmn@reform:~/code/linux$ lspci -v
>> 00:00.0 PCI bridge: Synopsys, Inc. Device abcd (rev 01) (prog-if 00
>>
>> [Normal decode])
>> Flags: bus master, fast devsel, latency 0, IRQ 307
>> Memory at 0100 (32-bit, non-prefetchable) [size=1M]
>> Bus: primary=00, secondary=01, subordinate=ff, sec-latency=0
>> ...
>>
>> The failure mode is that the PCIe controller shows up as a USB
>> controller and my ath9k wireless PCIe card cannot be assigned the
>> proper resources anymore (-ENOMEM even for very small BARs).
>>
>> Reverting this commit fixes the problem for me. I suggest to make the
>> check more specific to the platform/chipset you are targeting.
> So Synopsys apparently re-used Device ID 0xabcd? That's a pretty bad problem.
>
> It looks like we merged 03e6742584af ("PCI: Override Synopsys USB 3.x
> HAPS device class") for v5.0, and v5.0-final hasn't been released yet,
> so if we don't hear from Thinh with a resolution, we can still revert
> it.
>
> I set myself a reminder to revert this on Feb 11, but hopefully we'll
> have a better resolution before then.
>
> Bjorn
>
This is really odd that the PID for the PCIe controller in i.MX6QP is
the same as PID Synopsys use for USB controller. We use a different set
of PIDs for PCIe controllers and track VID and PID in our releases.
Is the Root Complex (00:00.0) part of the SoC? Or is this a Synopsys
prototype connected to your board? If it is the latter, then the FPGA
configuration needs to be updated to other PID.
We're investigating the workaround in case this is on a taped-out SoC.
Thanks,
Thinh
Re: [PATCH] usb: dwc3: gadget: fix miss isoc issue introduced by IRQ latency
Hi Zengtao, On 12/16/2018 5:45 PM, Zengtao (B) wrote: >> If it's a busy system, some times when we start an isoc transfer, >> the framenumber get from the event buffer may be already elasped, >> in this case, we will get all the packets dropped due to miss isoc. >> And we turn into transfer nothing, to fix this issue, we need to >> fix the framenumber to make sure that it's not out of date. >> >> Signed-off-by: Liang Shengjun >> Signed-off-by: Zeng Tao > There's a patch going upstream already doing this: > > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_balbi_usb.git_commit=DwIGaQ=DPL6_X_6JkXFx7AXWqB0tg=u9FYoxKtyhjrGFcyixFYqTjw1ZX0VsG2d8FCmzkTY-w=yGrN_JnamczN1G0wY4Th67vAfxSJEouaUgJdu0u6Av8=v4xeWOLyCSNoHxHS6kI8_bJfHgddFsPkqzgLFTJZOG8= > /?h > =next=d53701067f048b8b11635e964b6d3bd9a248c622 > Sorry, I think I missed to update to the latest version. But I think my patch is more efficient. Because I just sync the frame from the HW, it won't fail and there is no need to extra tries. What do you think about it? >>> the 14 bits you use for your check is not representative of the actual >>> micro-frame you're currently in. Thinh explained that in the >>> discussion we had until we came to the patch I pointed you to above. >>> Please look at the mailing list archives for details. >>> >> There are several issues with this patch. >> 1) Your frame elapsed time check is not based on interval but statically >> DWC3_EVENT_PRAM_SOFFN / 2. That's about 1 second. So it doesn't >> account for isoc transfers with large service interval of 1 sec or more. > > This is just for checking whether the Frame number is overflow or not. So > 1 second should a reason value. Why when there's another way to solve this issue without creating a new logic that only works within 1 second limit. > >> 2) This function __dwc3_gadget_target_frame_elapsed() should have >> comments for what it does. The name implies that this function checks >> for eframe > cframe, and not eframe > cframe + 1s. > eframe > cframe + 1s is used to deal with the Frame number overflow. Right. You need to provide comments for that. It's not obvious that DWC3_EVENT_PRAM_SOFFN / 2 is around 1 second. > >> 3) If this check fails, then it will do DWC3_ALIGN_FRAME() at least twice. >> The isoc transfer will start 1 more interval into the future than it needs >> to. >> > If the interval is one, 1 more interval should be more reasonable because the > core always fetch the trb one frame ahead. Did it fail with your setup when you test with interval of 1? It didn't fail from my testing setup. Your check applies for all intervals and not just interval of 1. Also, the databook doesn't mention this limitation that you have to do 2 intervals into the future if the service interval is 1. Regardless, if the __dwc3_gadget_target_frame_elapsed() passes for service interval of 1, then you'd only do DWC3_ALIGN_FRAME() once. This is different than what you said/wanted to do. > >> Also, I think the role of this check should be from the controller as it has >> more information and its own logic to decide if the selected future uframe >> has elapsed. > Yes, agree, but I think if the sw can be used to do the same thing and more > effective, Why not? > No. The SW cannot do the same thing. As Felipe mentioned previously, __dwc3_gadget_get_frame() can only get a 14-bit frame number. The controller keeps track of the frame number with at least 16 bits. It has more information and can work with larger service intervals. Can you test with Felipe's patches to see if they work for you? We can come back and review if there are still issues. Thanks, Thinh
Re: [PATCH] usb: dwc3: gadget: fix miss isoc issue introduced by IRQ latency
Hi Zengtao, On 12/14/2018 3:24 AM, Felipe Balbi wrote: > Hi, > > "Zengtao (B)" writes: >>> -Original Message- >>> From: Felipe Balbi [mailto:ba...@kernel.org] >>> Sent: Friday, December 14, 2018 4:52 PM >>> To: Zengtao (B) >>> Cc: liangshengjun ; Zengtao (B) >>> ; Greg Kroah-Hartman >>> ; linux-...@vger.kernel.org; >>> linux-kernel@vger.kernel.org >>> Subject: Re: [PATCH] usb: dwc3: gadget: fix miss isoc issue introduced by >>> IRQ latency >>> >>> * PGP Signed by an unknown key >>> >>> Zeng Tao writes: >>> If it's a busy system, some times when we start an isoc transfer, the framenumber get from the event buffer may be already elasped, in this case, we will get all the packets dropped due to miss isoc. And we turn into transfer nothing, to fix this issue, we need to fix the framenumber to make sure that it's not out of date. Signed-off-by: Liang Shengjun Signed-off-by: Zeng Tao >>> There's a patch going upstream already doing this: >>> >>> https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git/commit/?h >>> =next=d53701067f048b8b11635e964b6d3bd9a248c622 >>> >> Sorry, I think I missed to update to the latest version. But I think my >> patch is more efficient. Because I just sync the frame from the HW, it >> won't fail and there is no need to extra tries. >> >> What do you think about it? > the 14 bits you use for your check is not representative of the actual > micro-frame you're currently in. Thinh explained that in the discussion > we had until we came to the patch I pointed you to above. Please look at > the mailing list archives for details. > There are several issues with this patch. 1) Your frame elapsed time check is not based on interval but statically DWC3_EVENT_PRAM_SOFFN / 2. That's about 1 second. So it doesn't account for isoc transfers with large service interval of 1 sec or more. 2) This function __dwc3_gadget_target_frame_elapsed() should have comments for what it does. The name implies that this function checks for eframe > cframe, and not eframe > cframe + 1s. 3) If this check fails, then it will do DWC3_ALIGN_FRAME() at least twice. The isoc transfer will start 1 more interval into the future than it needs to. Also, I think the role of this check should be from the controller as it has more information and its own logic to decide if the selected future uframe has elapsed. BR, Thinh
