[PATCH] usb: gadget: Fix double free of device descriptor pointers
From: Hemant Kumar
Upon driver unbind usb_free_all_descriptors() function frees all
speed descriptor pointers without setting them to NULL. In case
gadget speed changes (i.e from super speed plus to super speed)
after driver unbind only upto super speed descriptor pointers get
populated. Super speed plus desc still holds the stale (already
freed) pointer. Fix this issue by setting all descriptor pointers
to NULL after freeing them in usb_free_all_descriptors().
Signed-off-by: Hemant Kumar
Signed-off-by: Wesley Cheng
---
drivers/usb/gadget/config.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/usb/gadget/config.c b/drivers/usb/gadget/config.c
index 2d11535..8bb2577 100644
--- a/drivers/usb/gadget/config.c
+++ b/drivers/usb/gadget/config.c
@@ -194,9 +194,13 @@ EXPORT_SYMBOL_GPL(usb_assign_descriptors);
void usb_free_all_descriptors(struct usb_function *f)
{
usb_free_descriptors(f->fs_descriptors);
+ f->fs_descriptors = NULL;
usb_free_descriptors(f->hs_descriptors);
+ f->hs_descriptors = NULL;
usb_free_descriptors(f->ss_descriptors);
+ f->ss_descriptors = NULL;
usb_free_descriptors(f->ssp_descriptors);
+ f->ssp_descriptors = NULL;
}
EXPORT_SYMBOL_GPL(usb_free_all_descriptors);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Re: [PATCH] usb: dwc3: gadget: Avoid canceling current request for queuing error
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. Thanks Wesley Cheng -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH] usb: dwc3: gadget: Avoid canceling current request for queuing error
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. Thanks Wesley Cheng -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
[PATCH] usb: dwc3: gadget: Avoid canceling current request for queuing error
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)); + dwc3_gadget_del_and_unmap_request(dep, req, ret); + dwc3_stop_active_transfer(dep, true, true); list_for_each_entry_safe(req, tmp, >started_list, list) -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH] Revert "usb: dwc3: gadget: Prevent EP queuing while stopping transfers"
On 4/1/2021 4:55 AM, Martin Kepplinger wrote:
> This reverts commit 9de47c3737e0c0207beb03615b320cabe495.
>
> Signed-off-by: Martin Kepplinger
> ---
>
> I more or less blindly report:
> commit 9de47c ("usb: dwc3: gadget: Prevent EP queuing while stopping
> transfers") results in the below error every time I connect the type-c
> connector to the dwc3, configured with serial and ethernet gadgets.
>
> fyi, I apply the following to dwc3 on this port:
> dr_mode = "otg";
> snps,dis_u3_susphy_quirk;
> hnp-disable;
> srp-disable;
> adp-disable;
> usb-role-switch;
>
> v5.12-rc5 does not have this error so I'm not sure whether it's
> more appropriate to add something to dwc3 than reverting. I hope usb
> people to know better and maybe even see the problem.
>
> thanks,
>martin
>
Hi Martin,
This has been fixed with the below:
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/?h=usb-linus=5aef629704ad4d983ecf5c8a25840f16e45b6d59
Can you pull that in and give it a try?
Thanks
Wesley Cheng
>
>
>
> [ 51.148220] [ cut here ]
> [ 51.148241] dwc3 3810.usb: No resource for ep2in
> [ 51.148376] WARNING: CPU: 0 PID: 299 at drivers/usb/dwc3/gadget.c:360
> dwc3_send_gadget_ep_cmd+0x570/0x740 [dwc3]
> [ 51.148431] Modules linked in: aes_ce_ccm rfcomm algif_hash algif_skcipher
> af_alg bnep qmi_wwan cdc_wdm usbnet mii option usb_wwan usbserial caam_jr
> caamhash_desc caamalg_desc crypto_engine mousedev redpine_sdio redpine_91x
> bluetooth uas usb_storage mac80211 st_lsm6dsx_spi aes_ce_blk crypto_simd
> crct10dif_ce cfg80211 ghash_ce sha2_ce sha1_ce bq25890_charger s5k3l6xx
> snd_soc_wm8962 hi846 vcnl4000 leds_lm3560 edt_ft5x06 ofpart
> industrialio_triggered_buffer tps6598x st_lsm6dsx_i2c st_lsm6dsx kfifo_buf
> gnss_mtk typec mxc_mipi_csi2_yav gnss_serial mx6s_capture gnss
> videobuf2_dma_contig videobuf2_memops spi_nor v4l2_fwnode videobuf2_v4l2
> videobuf2_common snd_soc_gtm601 snd_soc_simple_card videodev mtd
> snd_soc_simple_card_utils mc snd_soc_fsl_sai imx_pcm_dma caam error
> snvs_pwrkey imx_sdma virt_dma snd_soc_core imx2_wdt snd_pcm_dmaengine snd_pcm
> watchdog snd_timer snd soundcore pwm_vibra rfkill_hks rfkill ledtrig_timer
> usb_f_acm u_serial usb_f_rndis g_multi usb_f_mass_storage u_ether
> [ 51.148759] libcomposite ledtrig_pattern fuse ip_tables x_tables ipv6
> xhci_plat_hcd xhci_hcd usbcore dwc3 cdns_mhdp_imx ulpi cdns_mhdp_drmcore
> udc_core imx_dcss roles usb_common phy_fsl_imx8mq_usb clk_bd718x7
> [ 51.148837] CPU: 0 PID: 299 Comm: irq/64-dwc3 Not tainted
> 5.11.11-librem5-00334-ge4c4ff3624e9 #218
> [ 51.148848] Hardware name: Purism Librem 5r4 (DT)
> [ 51.148854] pstate: 6085 (nZCv daIf -PAN -UAO -TCO BTYPE=--)
> [ 51.148863] pc : dwc3_send_gadget_ep_cmd+0x570/0x740 [dwc3]
> [ 51.148894] lr : dwc3_send_gadget_ep_cmd+0x570/0x740 [dwc3]
> [ 51.148924] sp : 800011cb3ac0
> [ 51.148929] x29: 800011cb3ac0 x28: 032a7b00
> [ 51.148942] x27: 0327da00 x26:
> [ 51.148954] x25: ffea x24: 0006
> [ 51.148967] x23: bee1c080 x22: 800011cb3b7c
> [ 51.148979] x21: 0406 x20: bf17
> [ 51.148992] x19: 0001 x18:
> [ 51.149004] x17: x16:
> [ 51.149016] x15: x14: 8000114512c0
> [ 51.149028] x13: 1698 x12: 0040
> [ 51.149040] x11: 80001151a6f8 x10: e000
> [ 51.149052] x9 : 8000100b2b7c x8 : 80001146a6f8
> [ 51.149065] x7 : 80001151a6f8 x6 :
> [ 51.149077] x5 : bf939948 x4 :
> [ 51.149089] x3 : 0027 x2 :
> [ 51.149101] x1 : x0 : 049ae040
> [ 51.149114] Call trace:
> [ 51.149120] dwc3_send_gadget_ep_cmd+0x570/0x740 [dwc3]
> [ 51.149150] __dwc3_gadget_ep_enable+0x288/0x4fc [dwc3]
> [ 51.149179] dwc3_gadget_ep_enable+0x6c/0x15c [dwc3]
> [ 51.149209] usb_ep_enable+0x48/0x110 [udc_core]
> [ 51.149251] rndis_set_alt+0x138/0x1c0 [usb_f_rndis]
> [ 51.149276] composite_setup+0x674/0x194c [libcomposite]
> [ 51.149314] dwc3_ep0_interrupt+0x9c4/0xb9c [dwc3]
> [ 51.149344] dwc3_thread_interrupt+0x8bc/0xe74 [dwc3]
> [ 51.149374] irq_thread_fn+0x38/0xb0
> [ 51.1
Re: [PATCH v3 2/2] usb: dwc3: Fix DRD mode change sequence following programming guide
On 3/29/2021 6:19 PM, Thinh Nguyen wrote: > 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_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()? >>> >> Hi Thinh, >> >> Sorry for the late response. My mist
Re: [PATCH v3 1/2] usb: dwc3: Trigger a GCTL soft reset when switching modes in DRD
On 3/29/2021 3:20 PM, John Stultz wrote: > On Mon, Mar 29, 2021 at 3:15 PM Wesley Cheng wrote: >> >> >> >> On 3/19/2021 4:09 PM, Thinh Nguyen wrote: >>> 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: >>>>>&g
Re: [PATCH v3 2/2] usb: dwc3: Fix DRD mode change sequence following programming guide
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_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()? > 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 -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v3 1/2] usb: dwc3: Trigger a GCTL soft reset when switching modes in DRD
On 3/19/2021 4:09 PM, Thinh Nguyen wrote: > 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
[PATCH] usb: dwc3: gadget: Clear DEP flags after stop transfers in ep disable
Ensure that dep->flags are cleared until after stop active transfers
is completed. Otherwise, the ENDXFER command will not be executed
during ep disable.
Fixes: f09ddcfcb8c5 ("usb: dwc3: gadget: Prevent EP queuing while stopping
transfers")
Reported-and-tested-by: Andy Shevchenko
Tested-by: Marek Szyprowski
Signed-off-by: Wesley Cheng
---
drivers/usb/dwc3/gadget.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index e1442fc..2c4d201 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -791,10 +791,6 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
reg &= ~DWC3_DALEPENA_EP(dep->number);
dwc3_writel(dwc->regs, DWC3_DALEPENA, reg);
- dep->stream_capable = false;
- dep->type = 0;
- dep->flags = 0;
-
/* Clear out the ep descriptors for non-ep0 */
if (dep->number > 1) {
dep->endpoint.comp_desc = NULL;
@@ -803,6 +799,10 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
dwc3_remove_requests(dwc, dep);
+ dep->stream_capable = false;
+ dep->type = 0;
+ dep->flags = 0;
+
return 0;
}
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Re: [PATCH v3] usb: dwc3: gadget: Prevent EP queuing while stopping transfers
On 3/23/2021 10:27 AM, Andy Shevchenko wrote: > On Tue, Mar 23, 2021 at 1:19 AM Wesley Cheng wrote: >> >> Hi Andy, >> >> On 3/22/2021 2:14 PM, Andy Shevchenko wrote: >>> On Mon, Mar 22, 2021 at 10:06 PM Wesley Cheng wrote: >>>> >>>> Hi Andy, >>>> >>>> On 3/22/2021 12:34 PM, Andy Shevchenko wrote: >>>>> On Mon, Mar 22, 2021 at 8:49 PM Wesley Cheng >>>>> wrote: >>>>>> >>>>>> Hi Andy, >>>>>> >>>>>> On 3/22/2021 5:48 AM, Andy Shevchenko wrote: >>>>>>> On Fri, Mar 12, 2021 at 2:01 AM 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. >>>>>>> >>>>>>> >>>>>>> commit f09ddcfcb8c569675066337adac2ac205113471f >>>>>>> Author: Wesley Cheng >>>>>>> Date: Thu Mar 11 15:59:02 2021 -0800 >>>>>>> >>>>>>>usb: dwc3: gadget: Prevent EP queuing while stopping transfers >>>>>>> >>>>>>> effectively broke my gadget setup. >>>>>>> >>>>>>> The output of the kernel (followed by non responsive state of USB >>>>>>> controller): >>>>>>> >>>>>>> [ 195.228586] using random self ethernet address >>>>>>> [ 195.233104] using random host ethernet address >>>>>>> [ 195.245306] usb0: HOST MAC aa:bb:cc:dd:ee:f2 >>>>>>> [ 195.249732] usb0: MAC aa:bb:cc:dd:ee:f1 >>>>>>> # [ 195.773594] IPv6: ADDRCONF(NETDEV_CHANGE): usb0: link becomes ready >>>>>>> [ 195.780585] [ cut here ] >>>>>>> [ 195.785217] dwc3 dwc3.0.auto: No resource for ep2in >>>>>>> [ 195.790162] WARNING: CPU: 0 PID: 217 at >>>>>>> drivers/usb/dwc3/gadget.c:360 dwc3_send_gadget_ep_cmd+0x4b9/0x670 >>>>>>> [ 195.799760] Modules linked in: usb_f_eem u_ether libcomposite >>>>>>> brcmfmac brcmutil mmc_block pwm_lpss_pci pwm_lps >>>>>>> s snd_sof_pci_intel_tng snd_sof_pci snd_sof_acpi_intel_byt >>>>>>> snd_sof_intel_ipc snd_sof_acpi snd_sof snd_sof_nocodec >>>>>>> spi_pxa2xx_platform snd_sof_xtensa_dsp spi_pxa2xx_pci >>>>>>> extcon_intel_mrfld intel_mrfld_adc sdhci_pci cqhci sdhci m >>>>>>> mc_core intel_mrfld_pwrbtn intel_soc_pmic_mrfld hci_uart btbcm btintel >>>>>>> [ 195.835604] CPU: 0 PID: 217 Comm: irq/16-dwc3 Not tainted >>>>>>> 5.12.0-rc4+ #60 >>>>>>> [ 195.842403] Hardware name: Intel Corporation Merrifield/BODEGA BAY, >>>>>>> BIOS 542 2015.01.21:18.19.48 >>>>>>> [ 195.851191] RIP: 0010:dwc3_send_gadget_ep_cmd+0x4b9/0x670 >>>>>>> [ 195.856608] Code: cd 00 00 00 44 89 44 24 20 48 89 4c 24 18 e8 ee >>>>>>> f7 e4 ff 48 8b 4c 24 18 4c 89 f2 48 c7 c7 b9 >>>>>>> ed 4f a0 48 89 c6 e8 ef 24 43 00 <0f> 0b 41 be ea ff ff ff 44 8b 44 24 >>>>>>> 20 e9 80 fc ff ff 41 83 fe 92 >>>>>>> [ 195.875381] RSP: :a53c00373ba8 EFLAGS: 00010086 >>>>>>> [ 195.880617] RAX: RBX: 1387 RCX: >>>>>>> dfff >>>>>>> [ 195.887755] RDX: dfff RSI: ffea RDI: >>>>>>> >>>>>>> [ 195.894893] RBP: 9ce8c8f2b028
Re: [PATCH v3] usb: dwc3: gadget: Prevent EP queuing while stopping transfers
On 3/23/2021 10:27 AM, Andy Shevchenko wrote: > On Tue, Mar 23, 2021 at 1:19 AM Wesley Cheng wrote: >> >> Hi Andy, >> >> On 3/22/2021 2:14 PM, Andy Shevchenko wrote: >>> On Mon, Mar 22, 2021 at 10:06 PM Wesley Cheng wrote: >>>> >>>> Hi Andy, >>>> >>>> On 3/22/2021 12:34 PM, Andy Shevchenko wrote: >>>>> On Mon, Mar 22, 2021 at 8:49 PM Wesley Cheng >>>>> wrote: >>>>>> >>>>>> Hi Andy, >>>>>> >>>>>> On 3/22/2021 5:48 AM, Andy Shevchenko wrote: >>>>>>> On Fri, Mar 12, 2021 at 2:01 AM 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. >>>>>>> >>>>>>> >>>>>>> commit f09ddcfcb8c569675066337adac2ac205113471f >>>>>>> Author: Wesley Cheng >>>>>>> Date: Thu Mar 11 15:59:02 2021 -0800 >>>>>>> >>>>>>>usb: dwc3: gadget: Prevent EP queuing while stopping transfers >>>>>>> >>>>>>> effectively broke my gadget setup. >>>>>>> >>>>>>> The output of the kernel (followed by non responsive state of USB >>>>>>> controller): >>>>>>> >>>>>>> [ 195.228586] using random self ethernet address >>>>>>> [ 195.233104] using random host ethernet address >>>>>>> [ 195.245306] usb0: HOST MAC aa:bb:cc:dd:ee:f2 >>>>>>> [ 195.249732] usb0: MAC aa:bb:cc:dd:ee:f1 >>>>>>> # [ 195.773594] IPv6: ADDRCONF(NETDEV_CHANGE): usb0: link becomes ready >>>>>>> [ 195.780585] [ cut here ] >>>>>>> [ 195.785217] dwc3 dwc3.0.auto: No resource for ep2in >>>>>>> [ 195.790162] WARNING: CPU: 0 PID: 217 at >>>>>>> drivers/usb/dwc3/gadget.c:360 dwc3_send_gadget_ep_cmd+0x4b9/0x670 >>>>>>> [ 195.799760] Modules linked in: usb_f_eem u_ether libcomposite >>>>>>> brcmfmac brcmutil mmc_block pwm_lpss_pci pwm_lps >>>>>>> s snd_sof_pci_intel_tng snd_sof_pci snd_sof_acpi_intel_byt >>>>>>> snd_sof_intel_ipc snd_sof_acpi snd_sof snd_sof_nocodec >>>>>>> spi_pxa2xx_platform snd_sof_xtensa_dsp spi_pxa2xx_pci >>>>>>> extcon_intel_mrfld intel_mrfld_adc sdhci_pci cqhci sdhci m >>>>>>> mc_core intel_mrfld_pwrbtn intel_soc_pmic_mrfld hci_uart btbcm btintel >>>>>>> [ 195.835604] CPU: 0 PID: 217 Comm: irq/16-dwc3 Not tainted >>>>>>> 5.12.0-rc4+ #60 >>>>>>> [ 195.842403] Hardware name: Intel Corporation Merrifield/BODEGA BAY, >>>>>>> BIOS 542 2015.01.21:18.19.48 >>>>>>> [ 195.851191] RIP: 0010:dwc3_send_gadget_ep_cmd+0x4b9/0x670 >>>>>>> [ 195.856608] Code: cd 00 00 00 44 89 44 24 20 48 89 4c 24 18 e8 ee >>>>>>> f7 e4 ff 48 8b 4c 24 18 4c 89 f2 48 c7 c7 b9 >>>>>>> ed 4f a0 48 89 c6 e8 ef 24 43 00 <0f> 0b 41 be ea ff ff ff 44 8b 44 24 >>>>>>> 20 e9 80 fc ff ff 41 83 fe 92 >>>>>>> [ 195.875381] RSP: :a53c00373ba8 EFLAGS: 00010086 >>>>>>> [ 195.880617] RAX: RBX: 1387 RCX: >>>>>>> dfff >>>>>>> [ 195.887755] RDX: dfff RSI: ffea RDI: >>>>>>> >>>>>>> [ 195.894893] RBP: 9ce8c8f2b028
Re: [PATCH] usb: gadget: Stall OS descriptor request for unsupported functions
On 3/22/2021 11:25 PM, Jack Pham wrote: > Hi Wesley, > > On Mon, Mar 22, 2021 at 06:50:17PM -0700, Wesley Cheng wrote: >> From: Chandana Kishori Chiluveru >> >> Hosts which request "OS descriptors" from gadgets do so during >> the enumeration phase and before the configuration is set with >> SET_CONFIGURATION. Composite driver supports OS descriptor >> handling in composite_setup function. This requires to pass >> signature field, vendor code, compatibleID and subCompatibleID >> from user space. >> >> For USB compositions that contain functions which don't implement os >> descriptors, Windows is sending vendor specific requests for os >> descriptors and composite driver handling this request with invalid >> data. With this invalid info host resetting the bus and never >> selecting the configuration and leading enumeration issue. >> >> Fix this by bailing out from the OS descriptor setup request >> handling if the functions does not have OS descriptors compatibleID. >> >> Signed-off-by: Chandana Kishori Chiluveru >> Signed-off-by: Wesley Cheng >> --- >> drivers/usb/gadget/composite.c | 6 ++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c >> index 72a9797..473edda6 100644 >> --- a/drivers/usb/gadget/composite.c >> +++ b/drivers/usb/gadget/composite.c >> @@ -1945,6 +1945,12 @@ composite_setup(struct usb_gadget *gadget, const >> struct usb_ctrlrequest *ctrl) >> buf[6] = w_index; >> /* Number of ext compat interfaces */ >> count = count_ext_compat(os_desc_cfg); >> +/* >> + * Bailout if device does not >> + * have ext_compat interfaces. >> + */ >> +if (count == 0) >> +break; >> buf[8] = count; >> count *= 24; /* 24 B/ext compat desc */ >> count += 16; /* header */ > > Do we still need this fix? IIRC we had this change in our downstream > kernel to fix the case when dynamically re-configuring ConfigFS, i.e. > changing the composition of functions wherein none of the interfaces > support OS Descriptors, so this causes count_ext_compat() to return > 0 and results in the issue described in $SUBJECT. > Hi Jack, You're correct. We can address this as well in the userspace perspective. > But I think this is more of a problem of an improperly configured > ConfigFS gadget. If userspace instead removes the config from the > gadget's os_desc subdirectory that should cause cdev->os_desc_config to > be set to NULL and hence composite_setup() should never enter this > handling at all, right? Sure, I'll go with fixing it in the userspace, since the support to stall the OS desc is already present in the composite driver as you mentioned. Thanks for the input. Thanks Wesley Cheng -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
[PATCH] usb: gadget: Stall OS descriptor request for unsupported functions
From: Chandana Kishori Chiluveru Hosts which request "OS descriptors" from gadgets do so during the enumeration phase and before the configuration is set with SET_CONFIGURATION. Composite driver supports OS descriptor handling in composite_setup function. This requires to pass signature field, vendor code, compatibleID and subCompatibleID from user space. For USB compositions that contain functions which don't implement os descriptors, Windows is sending vendor specific requests for os descriptors and composite driver handling this request with invalid data. With this invalid info host resetting the bus and never selecting the configuration and leading enumeration issue. Fix this by bailing out from the OS descriptor setup request handling if the functions does not have OS descriptors compatibleID. Signed-off-by: Chandana Kishori Chiluveru Signed-off-by: Wesley Cheng --- drivers/usb/gadget/composite.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c index 72a9797..473edda6 100644 --- a/drivers/usb/gadget/composite.c +++ b/drivers/usb/gadget/composite.c @@ -1945,6 +1945,12 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl) buf[6] = w_index; /* Number of ext compat interfaces */ count = count_ext_compat(os_desc_cfg); + /* +* Bailout if device does not +* have ext_compat interfaces. +*/ + if (count == 0) + break; buf[8] = count; count *= 24; /* 24 B/ext compat desc */ count += 16; /* header */ -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v3] usb: dwc3: gadget: Prevent EP queuing while stopping transfers
Hi Andy, On 3/22/2021 2:14 PM, Andy Shevchenko wrote: > On Mon, Mar 22, 2021 at 10:06 PM Wesley Cheng wrote: >> >> Hi Andy, >> >> On 3/22/2021 12:34 PM, Andy Shevchenko wrote: >>> On Mon, Mar 22, 2021 at 8:49 PM Wesley Cheng wrote: >>>> >>>> Hi Andy, >>>> >>>> On 3/22/2021 5:48 AM, Andy Shevchenko wrote: >>>>> On Fri, Mar 12, 2021 at 2:01 AM 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. >>>>> >>>>> >>>>> commit f09ddcfcb8c569675066337adac2ac205113471f >>>>> Author: Wesley Cheng >>>>> Date: Thu Mar 11 15:59:02 2021 -0800 >>>>> >>>>>usb: dwc3: gadget: Prevent EP queuing while stopping transfers >>>>> >>>>> effectively broke my gadget setup. >>>>> >>>>> The output of the kernel (followed by non responsive state of USB >>>>> controller): >>>>> >>>>> [ 195.228586] using random self ethernet address >>>>> [ 195.233104] using random host ethernet address >>>>> [ 195.245306] usb0: HOST MAC aa:bb:cc:dd:ee:f2 >>>>> [ 195.249732] usb0: MAC aa:bb:cc:dd:ee:f1 >>>>> # [ 195.773594] IPv6: ADDRCONF(NETDEV_CHANGE): usb0: link becomes ready >>>>> [ 195.780585] [ cut here ] >>>>> [ 195.785217] dwc3 dwc3.0.auto: No resource for ep2in >>>>> [ 195.790162] WARNING: CPU: 0 PID: 217 at >>>>> drivers/usb/dwc3/gadget.c:360 dwc3_send_gadget_ep_cmd+0x4b9/0x670 >>>>> [ 195.799760] Modules linked in: usb_f_eem u_ether libcomposite >>>>> brcmfmac brcmutil mmc_block pwm_lpss_pci pwm_lps >>>>> s snd_sof_pci_intel_tng snd_sof_pci snd_sof_acpi_intel_byt >>>>> snd_sof_intel_ipc snd_sof_acpi snd_sof snd_sof_nocodec >>>>> spi_pxa2xx_platform snd_sof_xtensa_dsp spi_pxa2xx_pci >>>>> extcon_intel_mrfld intel_mrfld_adc sdhci_pci cqhci sdhci m >>>>> mc_core intel_mrfld_pwrbtn intel_soc_pmic_mrfld hci_uart btbcm btintel >>>>> [ 195.835604] CPU: 0 PID: 217 Comm: irq/16-dwc3 Not tainted 5.12.0-rc4+ >>>>> #60 >>>>> [ 195.842403] Hardware name: Intel Corporation Merrifield/BODEGA BAY, >>>>> BIOS 542 2015.01.21:18.19.48 >>>>> [ 195.851191] RIP: 0010:dwc3_send_gadget_ep_cmd+0x4b9/0x670 >>>>> [ 195.856608] Code: cd 00 00 00 44 89 44 24 20 48 89 4c 24 18 e8 ee >>>>> f7 e4 ff 48 8b 4c 24 18 4c 89 f2 48 c7 c7 b9 >>>>> ed 4f a0 48 89 c6 e8 ef 24 43 00 <0f> 0b 41 be ea ff ff ff 44 8b 44 24 >>>>> 20 e9 80 fc ff ff 41 83 fe 92 >>>>> [ 195.875381] RSP: :a53c00373ba8 EFLAGS: 00010086 >>>>> [ 195.880617] RAX: RBX: 1387 RCX: >>>>> dfff >>>>> [ 195.887755] RDX: dfff RSI: ffea RDI: >>>>> >>>>> [ 195.894893] RBP: 9ce8c8f2b028 R08: a0732288 R09: >>>>> 9ffb >>>>> [ 195.902034] R10: e000 R11: 3fff R12: >>>>> 00041006 >>>>> [ 195.909170] R13: a53c00373c24 R14: 9ce8c11dadb0 R15: >>>>> 9ce8c2861700 >>>>> [ 195.916310] FS: () GS:9ce8fe20() >>>>> knlGS: >>>>> [ 195.924409] CS: 0010 DS: ES: CR0: 80050033 >>>>> [ 195.930161] CR2: f7f694a0 CR3: 38e0c000 CR4: >>>>> 001006f0 >>>>> [ 195.937300] Call Trace: >>&g
Re: [PATCH v3] usb: dwc3: gadget: Prevent EP queuing while stopping transfers
Hi Andy,
On 3/22/2021 12:34 PM, Andy Shevchenko wrote:
> On Mon, Mar 22, 2021 at 8:49 PM Wesley Cheng wrote:
>>
>> Hi Andy,
>>
>> On 3/22/2021 5:48 AM, Andy Shevchenko wrote:
>>> On Fri, Mar 12, 2021 at 2:01 AM 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.
>>>
>>>
>>> commit f09ddcfcb8c569675066337adac2ac205113471f
>>> Author: Wesley Cheng
>>> Date: Thu Mar 11 15:59:02 2021 -0800
>>>
>>>usb: dwc3: gadget: Prevent EP queuing while stopping transfers
>>>
>>> effectively broke my gadget setup.
>>>
>>> The output of the kernel (followed by non responsive state of USB
>>> controller):
>>>
>>> [ 195.228586] using random self ethernet address
>>> [ 195.233104] using random host ethernet address
>>> [ 195.245306] usb0: HOST MAC aa:bb:cc:dd:ee:f2
>>> [ 195.249732] usb0: MAC aa:bb:cc:dd:ee:f1
>>> # [ 195.773594] IPv6: ADDRCONF(NETDEV_CHANGE): usb0: link becomes ready
>>> [ 195.780585] [ cut here ]
>>> [ 195.785217] dwc3 dwc3.0.auto: No resource for ep2in
>>> [ 195.790162] WARNING: CPU: 0 PID: 217 at
>>> drivers/usb/dwc3/gadget.c:360 dwc3_send_gadget_ep_cmd+0x4b9/0x670
>>> [ 195.799760] Modules linked in: usb_f_eem u_ether libcomposite
>>> brcmfmac brcmutil mmc_block pwm_lpss_pci pwm_lps
>>> s snd_sof_pci_intel_tng snd_sof_pci snd_sof_acpi_intel_byt
>>> snd_sof_intel_ipc snd_sof_acpi snd_sof snd_sof_nocodec
>>> spi_pxa2xx_platform snd_sof_xtensa_dsp spi_pxa2xx_pci
>>> extcon_intel_mrfld intel_mrfld_adc sdhci_pci cqhci sdhci m
>>> mc_core intel_mrfld_pwrbtn intel_soc_pmic_mrfld hci_uart btbcm btintel
>>> [ 195.835604] CPU: 0 PID: 217 Comm: irq/16-dwc3 Not tainted 5.12.0-rc4+ #60
>>> [ 195.842403] Hardware name: Intel Corporation Merrifield/BODEGA BAY,
>>> BIOS 542 2015.01.21:18.19.48
>>> [ 195.851191] RIP: 0010:dwc3_send_gadget_ep_cmd+0x4b9/0x670
>>> [ 195.856608] Code: cd 00 00 00 44 89 44 24 20 48 89 4c 24 18 e8 ee
>>> f7 e4 ff 48 8b 4c 24 18 4c 89 f2 48 c7 c7 b9
>>> ed 4f a0 48 89 c6 e8 ef 24 43 00 <0f> 0b 41 be ea ff ff ff 44 8b 44 24
>>> 20 e9 80 fc ff ff 41 83 fe 92
>>> [ 195.875381] RSP: :a53c00373ba8 EFLAGS: 00010086
>>> [ 195.880617] RAX: RBX: 1387 RCX:
>>> dfff
>>> [ 195.887755] RDX: dfff RSI: ffea RDI:
>>>
>>> [ 195.894893] RBP: 9ce8c8f2b028 R08: a0732288 R09:
>>> 9ffb
>>> [ 195.902034] R10: e000 R11: 3fff R12:
>>> 00041006
>>> [ 195.909170] R13: a53c00373c24 R14: 9ce8c11dadb0 R15:
>>> 9ce8c2861700
>>> [ 195.916310] FS: () GS:9ce8fe20()
>>> knlGS:
>>> [ 195.924409] CS: 0010 DS: ES: CR0: 80050033
>>> [ 195.930161] CR2: f7f694a0 CR3: 38e0c000 CR4:
>>> 001006f0
>>> [ 195.937300] Call Trace:
>>> [ 195.939755] __dwc3_gadget_ep_enable+0x2d4/0x4e0
>>> [ 195.944393] ? dwc3_remove_requests.constprop.0+0x86/0x170
>>
>> Odd that this change would affect the USB enablment path, as they were
>> focused on the pullup disable path. Would you happen to have any
>> downstream changes on top of v5.12-rc4 we could review to see if they
>> are still required? (ie where is the dwc3_remove_requests() coming from
>> during ep enable)
>
> You may check my branch [1] on GH. Basically you may be interested in
> the commit:
> 0f86df1294ee7523060cc16eafaf4898c693eab0 REVERTME: usb: dwc3: gadget:
> skip endpoints ep[18]{in,out}
> Otherwise it's a clean v5.12-rc4 with a revert and another USB PHY
> sus
Re: [PATCH v3] usb: dwc3: gadget: Prevent EP queuing while stopping transfers
Hi Andy, On 3/22/2021 5:48 AM, Andy Shevchenko wrote: > On Fri, Mar 12, 2021 at 2:01 AM 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. > > > commit f09ddcfcb8c569675066337adac2ac205113471f > Author: Wesley Cheng > Date: Thu Mar 11 15:59:02 2021 -0800 > >usb: dwc3: gadget: Prevent EP queuing while stopping transfers > > effectively broke my gadget setup. > > The output of the kernel (followed by non responsive state of USB controller): > > [ 195.228586] using random self ethernet address > [ 195.233104] using random host ethernet address > [ 195.245306] usb0: HOST MAC aa:bb:cc:dd:ee:f2 > [ 195.249732] usb0: MAC aa:bb:cc:dd:ee:f1 > # [ 195.773594] IPv6: ADDRCONF(NETDEV_CHANGE): usb0: link becomes ready > [ 195.780585] [ cut here ] > [ 195.785217] dwc3 dwc3.0.auto: No resource for ep2in > [ 195.790162] WARNING: CPU: 0 PID: 217 at > drivers/usb/dwc3/gadget.c:360 dwc3_send_gadget_ep_cmd+0x4b9/0x670 > [ 195.799760] Modules linked in: usb_f_eem u_ether libcomposite > brcmfmac brcmutil mmc_block pwm_lpss_pci pwm_lps > s snd_sof_pci_intel_tng snd_sof_pci snd_sof_acpi_intel_byt > snd_sof_intel_ipc snd_sof_acpi snd_sof snd_sof_nocodec > spi_pxa2xx_platform snd_sof_xtensa_dsp spi_pxa2xx_pci > extcon_intel_mrfld intel_mrfld_adc sdhci_pci cqhci sdhci m > mc_core intel_mrfld_pwrbtn intel_soc_pmic_mrfld hci_uart btbcm btintel > [ 195.835604] CPU: 0 PID: 217 Comm: irq/16-dwc3 Not tainted 5.12.0-rc4+ #60 > [ 195.842403] Hardware name: Intel Corporation Merrifield/BODEGA BAY, > BIOS 542 2015.01.21:18.19.48 > [ 195.851191] RIP: 0010:dwc3_send_gadget_ep_cmd+0x4b9/0x670 > [ 195.856608] Code: cd 00 00 00 44 89 44 24 20 48 89 4c 24 18 e8 ee > f7 e4 ff 48 8b 4c 24 18 4c 89 f2 48 c7 c7 b9 > ed 4f a0 48 89 c6 e8 ef 24 43 00 <0f> 0b 41 be ea ff ff ff 44 8b 44 24 > 20 e9 80 fc ff ff 41 83 fe 92 > [ 195.875381] RSP: :a53c00373ba8 EFLAGS: 00010086 > [ 195.880617] RAX: RBX: 1387 RCX: > dfff > [ 195.887755] RDX: dfff RSI: ffea RDI: > > [ 195.894893] RBP: 9ce8c8f2b028 R08: a0732288 R09: > 9ffb > [ 195.902034] R10: e000 R11: 3fff R12: > 00041006 > [ 195.909170] R13: a53c00373c24 R14: 9ce8c11dadb0 R15: > 9ce8c2861700 > [ 195.916310] FS: () GS:9ce8fe20() > knlGS: > [ 195.924409] CS: 0010 DS: ES: CR0: 80050033 > [ 195.930161] CR2: f7f694a0 CR3: 38e0c000 CR4: > 001006f0 > [ 195.937300] Call Trace: > [ 195.939755] __dwc3_gadget_ep_enable+0x2d4/0x4e0 > [ 195.944393] ? dwc3_remove_requests.constprop.0+0x86/0x170 Odd that this change would affect the USB enablment path, as they were focused on the pullup disable path. Would you happen to have any downstream changes on top of v5.12-rc4 we could review to see if they are still required? (ie where is the dwc3_remove_requests() coming from during ep enable) > [ 195.949897] dwc3_gadget_ep_enable+0x5d/0x120 > [ 195.954274] usb_ep_enable+0x27/0x80 > [ 195.957869] gether_connect+0x32/0x1f0 [u_ether] > [ 195.962512] eem_set_alt+0x6d/0x140 [usb_f_eem] > [ 195.967061] composite_setup+0x224/0x1ba0 [libcomposite] > [ 195.972405] ? debug_dma_unmap_page+0x79/0x80 > [ 195.976782] ? configfs_composite_setup+0x6b/0x90 [libcomposite] > [ 195.982811] configfs_composite_setup+0x6b/0x90 [libcomposite] > [ 195.988668] dwc3_ep0_interrupt+0x459/0xa50 > [ 195.992869] dwc3_thread_interrupt+0x8e2/0xee0 > [ 195.997327] ? __schedule+0x237/0x6d0 > [ 196.001005] ? disable_irq_nosync+0x10/0x10 > [ 196.005200] irq_thread_fn+0x1b/0x60 > [ 196.008789] irq_thread+0xd6/0x170 > [ 196.012202] ? irq_thread_check_affinity+0x70/0x70 > [ 196.017004] ? irq_forced_thread_fn+0x70/0x70 > [ 196.021373] kthread+0x116/0x130 > [ 196.024617] ? kthread_create_worker_on_cpu+0x60/0x60 > [ 196.029680] ret_from_fork+0x22/0x30 > [ 1
Re: [PATCH 2/2] usb: dwc3: gadget: Ignore EP queue requests during bus reset
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.
> 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
Thanks
Wesley Cheng
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Re: [PATCH 2/2] usb: dwc3: gadget: Ignore EP queue requests during bus reset
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.
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)
Thanks
Wesley Cheng
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Re: [PATCH v3 1/2] usb: dwc3: Trigger a GCTL soft reset when switching modes in DRD
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? >>>>>>> >>>>>>> https://urldefense.com/v3/__https://lore.kernel.org/lkml/20201021181803.79650-1-john.stu...@linaro.org/__;!!A4F2R9G_pg!LNzuprAeg-O80SgolYkIkW4-ne-M-yLWCDUY9MygAIrQC398Z6gRJ9wnRWITZfc$ >>>>>>> >>>>>>> >
[PATCH 1/2] usb: dwc3: gadget: Avoid continuing preparing TRBs during teardown
Add checks similar to dwc3_gadget_ep_queue() before kicking off
transfers after getting an endpoint completion event. Since cleaning
up completed requests will momentarily unlock dwc->lock, there is a
chance for a sequence like pullup disable to be executed. This can
lead to preparing a TRB, which will be removed by the pullup disable
routine.
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 4a337f3..6e14fdc 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2912,6 +2912,11 @@ static void
dwc3_gadget_ep_cleanup_completed_requests(struct dwc3_ep *dep,
static bool dwc3_gadget_ep_should_continue(struct dwc3_ep *dep)
{
struct dwc3_request *req;
+ struct dwc3 *dwc = dep->dwc;
+
+ if (!dep->endpoint.desc || !dwc->pullups_connected ||
+ !dwc->connected)
+ return false;
if (!list_empty(>pending_list))
return true;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH 0/2] Fix allowing of ep queuing while stopping transfers
commit f09ddcfcb8c5 ("usb: dwc3: gadget: Prevent EP queuing while stopping
transfers") addressed an issue where the DWC3 gadget was still allowing EP
queuing to occur while the pullup disable routine was executing. This led to
a situation where the controller prepares a TRB, which will be unmapped by the
stop active transfer call.
In addition to the above, there are a few other places in the DWC3 gadget where
we need to block preparing of TRBs:
1. While the DWC3 gadget cleans up completed TRBs (during
dwc3_gadget_endpoint_trbs_complete()), DWC3 gadget giveback is utilized and
will release the dwc->lock. If a pullup disable call occurs while the
cleanup is happening, then there is a chance dwc3_gadget_ep_should_continue
will prepare a TRB, which will later on be unmapped by the stop active
transfer in the pullup disable path.
2. If we are in the CONFIGURED state and the host issues a bus RESET. In this
situation, the connected flag is still set to true while we stop active
transfers, which can lead to the same initial problem. Ideally, function
drivers would stop any pending usb requests through dwc3_reset_gadget()
using the EP disable call, but for some function drivers, this does not
occur synchronously in their disable() callback. These functions would rely
on the stop active transfers in the reset handler to issue the endxfer cmd.
Wesley Cheng (2):
usb: dwc3: gadget: Avoid continuing preparing TRBs during teardown
usb: dwc3: gadget: Ignore EP queue requests during bus reset
drivers/usb/dwc3/gadget.c | 14 ++
1 file changed, 14 insertions(+)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH 2/2] usb: dwc3: gadget: Ignore EP queue requests during bus reset
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. -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
[PATCH v3] usb: dwc3: gadget: Prevent EP queuing while stopping transfers
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 V2:
- Removed duplicate dwc->connected = false setting in pullup routine
Changes since V1:
- Added Fixes tag to point to the commit this is addressing
drivers/usb/dwc3/gadget.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 4780983..2c94cc9 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;
/*
* In the Synopsis DesignWare Cores USB3 Databook Rev. 3.30a
* Section 4.1.8 Table 4-7, it states that for a
device-initiated
@@ -2271,7 +2272,6 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int
is_on)
dwc->ev_buf->lpos = (dwc->ev_buf->lpos + count) %
dwc->ev_buf->length;
}
- dwc->connected = false;
} else {
__dwc3_gadget_start(dwc);
}
@@ -3329,8 +3329,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 +3368,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;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Re: [PATCH v2] usb: dwc3: gadget: Prevent EP queuing while stopping transfers
On 3/11/2021 3:18 PM, Thinh Nguyen wrote:
> 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
>
Hi Thinh,
Ah, good catch, must've missed that while porting this over from my
local branch. Will remove that connected false setting a few lines down.
Thanks
Wesley Cheng
>
>> /*
>> * 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;
>>
>
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v2] usb: dwc3: gadget: Prevent EP queuing while stopping transfers
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;
/*
* 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;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH] usb: dwc3: gadget: Prevent EP queuing while stopping transfers
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.
Signed-off-by: Wesley Cheng
---
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;
/*
* 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;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Re: [PATCH v3 1/2] usb: dwc3: Trigger a GCTL soft reset when switching modes in DRD
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? >>>>>> >>>>>> 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 > Hi Thinh, Sure, my DWC3_VER_TYPE output = 0x67612A2A Thanks Wesley Cheng -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v3 1/2] usb: dwc3: Trigger a GCTL soft reset when switching modes in DRD
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 -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v3 1/2] usb: dwc3: Trigger a GCTL soft reset when switching modes in DRD
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? Thanks Wesley Cheng -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v3 2/2] usb: dwc3: Fix DRD mode change sequence following programming guide
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 -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v7 4/5] usb: dwc3: dwc3-qcom: Enable tx-fifo-resize property by default
On 2/2/2021 8:23 AM, Bjorn Andersson wrote:
> On Thu 28 Jan 22:46 CST 2021, Wesley Cheng wrote:
>
>> In order to take advantage of the TX fifo resizing logic, manually add
>> these properties to the DWC3 child node by default. This will allow
>> the DWC3 gadget to resize the TX fifos for the IN endpoints, which
>> help with performance.
>>
>> Signed-off-by: Wesley Cheng
>> ---
>> drivers/usb/dwc3/dwc3-qcom.c | 10 ++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
>> index d803ee9..4ea6be3 100644
>> --- a/drivers/usb/dwc3/dwc3-qcom.c
>> +++ b/drivers/usb/dwc3/dwc3-qcom.c
>> @@ -564,6 +564,7 @@ static int dwc3_qcom_clk_init(struct dwc3_qcom *qcom,
>> int count)
>>
>> static const struct property_entry dwc3_qcom_acpi_properties[] = {
>> PROPERTY_ENTRY_STRING("dr_mode", "host"),
>> +PROPERTY_ENTRY_BOOL("tx-fifo-resize"),
>
> I checked the ACPI tables for Lenovo Miix 630, Yoga C630 and Flex 5G and
> neither one has this property specified. So while we could just add this
> here, it would have to be done in collaboration with the people who
> actually define these. And as said before, I believe we want this to
> always be enabled.
>
>> {}
>> };
>>
>> @@ -634,6 +635,7 @@ static int dwc3_qcom_of_register_core(struct
>> platform_device *pdev)
>> struct dwc3_qcom*qcom = platform_get_drvdata(pdev);
>> struct device_node *np = pdev->dev.of_node, *dwc3_np;
>> struct device *dev = >dev;
>> +struct property *prop;
>> int ret;
>>
>> dwc3_np = of_get_child_by_name(np, "dwc3");
>> @@ -642,6 +644,14 @@ static int dwc3_qcom_of_register_core(struct
>> platform_device *pdev)
>> return -ENODEV;
>> }
>>
>> +prop = kzalloc(sizeof(*prop), GFP_KERNEL);
>> +if (prop) {
>> +prop->name = "tx-fifo-resize";
>> +ret = of_add_property(dwc3_np, prop);
>
> Can't we come up with a way where the platform driver enables this on
> the core driver without modifying DT?
>
> Regards,
> Bjorn
Hi Bjorn,
Sorry for the late response. As you know, its a little difficult to
access the DWC3 core device during DWC3 qcom probe time, as the DWC3
core will likely return deferred probe due to the PHY devices not being
ready.
This is why I went with the approach to modify the DWC3 node here, so
that when the DWC3 core is eventually probed, it wouldn't miss this
property setting. If I tried to set this dynamically, say in
dwc3_qcom_vbus_override() (with proper NULL checks), then I'd miss this
setting for the first enumeration, but if cable plug out/in logic is
present, the setting would kick in on subsequent cable events.
Thanks
Wesley Cheng
>
>> +if (ret < 0)
>> +dev_info(dev, "unable to add tx-fifo-resize prop\n");
>> +}
>> +
>> ret = of_platform_populate(np, NULL, NULL, dev);
>> if (ret) {
>> dev_err(dev, "failed to register dwc3 core - %d\n", ret);
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Re: usb: dwc3: gadget: Change runtime pm function for DWC3 runtime suspend
On 2/15/2021 5:30 PM, Jung Daehwan wrote:
> Hello, Alan
>
> On Mon, Feb 15, 2021 at 12:41:45PM -0500, Alan Stern wrote:
>> On Mon, Feb 15, 2021 at 11:38:58AM +0900, Daehwan Jung wrote:
>>> It seems pm_runtime_put calls runtime_idle callback not runtime_suspend
>>> callback.
>>
>> How is this fact related to your patch?
>
> I think we should cause dwc3_runtime_suspend at the time.
> That's why I use pm_runtime_put_sync_suspend.
>
>>
>>> It's better to use pm_runtime_put_sync_suspend to allow DWC3 runtime
>>> suspend.
>>
>> Why do you think it is better? The advantage of pm_runtime_put is that
>> it allows the suspend to occur at a later time in a workqueue thread, so
>> the caller doesn't have to wait for the device to go into suspend.
>>
>
> We can assume DWC3 was already in suspend state if pm_runtime_get_sync
> returns 0. DWC3 resumes due to pm_rumtime_get_sync but it doesn't
> re-enter runtime_suspend but runtime_idle. pm_runtime_put decreases
> usage_count but doesn't cause runtime_suspend.
>
> 1. USB disconnected
> 2. UDC unbinded
> 3. DWC3 runtime suspend
> 4. UDC binded unexpectedly
> 5. DWC3 runtime resume (pm_runtime_get_sync)
> 6. DWC3 runtime idle (pm_runtime_put)
>-> DWC3 runtime suspend again (pm_runtime_put_sync_suspend)
>
> I've talked with Wesley in other patch.
>
> usbb: dwc3: gadget: skip pullup and set_speed after suspend
> patchwork.kernel.org/project/linux-usb/patch/163968-102424-1-git-send-email-dh10.j...@samsung.com
>
> @ Wesley
>
> I think We should guarantee DWC3 enters suspend again at the time.
> How do you think?
>
Hi Daehwan,
Even if we call runtime idle versus suspend, if the device is still in
the disconnected state, it should call the runtime PM suspend routine
after the autosuspend timer expires. As Alan mentioned already, this
allows not blocking the caller for the entire DWC3 suspend sequence to
execute. (DWC3 core will suspend other components as well, such as PHYs)
Also, for legitimate cases where pullup is actually called to start
enumeration from a suspended state, I'm not sure if the short duration
between RS set and re-suspend (due to your patch) is enough time for the
host to actually detect the device connected.
Thanks
Wesley Cheng
> Best Regards,
> Jung Daehwan
>
>> Alan Stern
>>
>
>
>>> Signed-off-by: Daehwan Jung
>>> ---
>>> drivers/usb/dwc3/gadget.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index aebcf8e..4a4b93b 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -2229,7 +2229,7 @@ static int dwc3_gadget_pullup(struct usb_gadget *g,
>>> int is_on)
>>> */
>>> ret = pm_runtime_get_sync(dwc->dev);
>>> if (!ret || ret < 0) {
>>> - pm_runtime_put(dwc->dev);
>>> + pm_runtime_put_sync_suspend(dwc->dev);
>>> return 0;
>>> }
>>>
>>> --
>>> 2.7.4
>>>
>>
>>
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Re: [PATCH v7 4/5] usb: dwc3: dwc3-qcom: Enable tx-fifo-resize property by default
On 1/29/2021 1:24 AM, Jack Pham wrote:
> Hi Wesley,
>
> On Thu, Jan 28, 2021 at 08:46:43PM -0800, Wesley Cheng wrote:
>> In order to take advantage of the TX fifo resizing logic, manually add
>> these properties to the DWC3 child node by default. This will allow
>> the DWC3 gadget to resize the TX fifos for the IN endpoints, which
>> help with performance.
>>
>> Signed-off-by: Wesley Cheng
>> ---
>> drivers/usb/dwc3/dwc3-qcom.c | 10 ++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
>> index d803ee9..4ea6be3 100644
>> --- a/drivers/usb/dwc3/dwc3-qcom.c
>> +++ b/drivers/usb/dwc3/dwc3-qcom.c
>> @@ -564,6 +564,7 @@ static int dwc3_qcom_clk_init(struct dwc3_qcom *qcom,
>> int count)
>>
>> static const struct property_entry dwc3_qcom_acpi_properties[] = {
>> PROPERTY_ENTRY_STRING("dr_mode", "host"),
>> +PROPERTY_ENTRY_BOOL("tx-fifo-resize"),
>> {}
>> };
>>
>> @@ -634,6 +635,7 @@ static int dwc3_qcom_of_register_core(struct
>> platform_device *pdev)
>> struct dwc3_qcom*qcom = platform_get_drvdata(pdev);
>> struct device_node *np = pdev->dev.of_node, *dwc3_np;
>> struct device *dev = >dev;
>> +struct property *prop;
>> int ret;
>>
>> dwc3_np = of_get_child_by_name(np, "dwc3");
>> @@ -642,6 +644,14 @@ static int dwc3_qcom_of_register_core(struct
>> platform_device *pdev)
>> return -ENODEV;
>> }
>>
>> +prop = kzalloc(sizeof(*prop), GFP_KERNEL);
>> +if (prop) {
>> +prop->name = "tx-fifo-resize";
>> +ret = of_add_property(dwc3_np, prop);
>> +if (ret < 0)
>> +dev_info(dev, "unable to add tx-fifo-resize prop\n");
>
> You'll need to kfree(prop) both in case of error here as well as in the
> driver's .remove() callback. Maybe easier to devm_kzalloc()?
Hi Jack,
Thanks for the catch, will fix this with the devm variant.
Hi Bjorn,
Just wanted to see what you thought about this approach? This way we
can just keep the dt binding w/o having to re-add it in the future, as
well as not needing to enable this property on every qcom platform with
dwc3.
Tested on my set up, and removed the change which added the property
from the DTSI node.
Thanks
Wesley Cheng
>
> Jack
>
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v7 5/5] dt-bindings: usb: dwc3: Update dwc3 TX fifo properties
Update the tx-fifo-resize property with a better description, while adding the tx-fifo-max-num, which is a new parameter allowing adjustments for the maximum number of packets the txfifo resizing logic can account for while resizing the endpoints. Signed-off-by: Wesley Cheng --- Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 15 +-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml index 2247da7..652b246 100644 --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml @@ -283,10 +283,21 @@ properties: maximum: 16 tx-fifo-resize: -description: Determines if the FIFO *has* to be reallocated -deprecated: true +description: Determines if the TX fifos can be dynamically resized depending + on the number of IN endpoints used and if bursting is supported. This + may help improve bandwidth on platforms with higher system latencies, as + increased fifo space allows for the controller to prefetch data into its + internal memory. type: boolean + tx-fifo-max-num: +description: Specifies the max number of packets the txfifo resizing logic + can account for when higher endpoint bursting is used. (bMaxBurst > 6) The + higher the number, the more fifo space the txfifo resizing logic will + allocate for that endpoint. +$ref: /schemas/types.yaml#/definitions/uint8 +minimum: 3 + snps,incr-burst-type-adjustment: description: Value for INCR burst type of GSBUSCFG0 register, undefined length INCR -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
[PATCH v7 2/5] usb: gadget: configfs: Check USB configuration before adding
Ensure that the USB gadget is able to support the configuration being
added based on the number of endpoints required from all interfaces. This
is for accounting for any bandwidth or space limitations.
Signed-off-by: Wesley Cheng
---
drivers/usb/gadget/configfs.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 0d56f33..e6de3ca5 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -1368,6 +1368,7 @@ static int configfs_composite_bind(struct usb_gadget
*gadget,
struct usb_function *f;
struct usb_function *tmp;
struct gadget_config_name *cn;
+ unsigned long ep_map = 0;
if (gadget_is_otg(gadget))
c->descriptors = otg_desc;
@@ -1397,7 +1398,28 @@ static int configfs_composite_bind(struct usb_gadget
*gadget,
list_add(>list, >func_list);
goto err_purge_funcs;
}
+ if (f->fs_descriptors) {
+ struct usb_descriptor_header **d;
+
+ d = f->fs_descriptors;
+ for (; *d; ++d) {
+ struct usb_endpoint_descriptor *ep;
+ int addr;
+
+ if ((*d)->bDescriptorType !=
USB_DT_ENDPOINT)
+ continue;
+
+ ep = (struct usb_endpoint_descriptor
*)*d;
+ addr = ((ep->bEndpointAddress & 0x80)
>> 3) |
+ (ep->bEndpointAddress & 0x0f);
+ set_bit(addr, _map);
+ }
+ }
}
+ ret = usb_gadget_check_config(cdev->gadget, ep_map);
+ if (ret)
+ goto err_purge_funcs;
+
usb_ep_autoconfig_reset(cdev->gadget);
}
if (cdev->use_os_string) {
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v7 4/5] usb: dwc3: dwc3-qcom: Enable tx-fifo-resize property by default
In order to take advantage of the TX fifo resizing logic, manually add
these properties to the DWC3 child node by default. This will allow
the DWC3 gadget to resize the TX fifos for the IN endpoints, which
help with performance.
Signed-off-by: Wesley Cheng
---
drivers/usb/dwc3/dwc3-qcom.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index d803ee9..4ea6be3 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -564,6 +564,7 @@ static int dwc3_qcom_clk_init(struct dwc3_qcom *qcom, int
count)
static const struct property_entry dwc3_qcom_acpi_properties[] = {
PROPERTY_ENTRY_STRING("dr_mode", "host"),
+ PROPERTY_ENTRY_BOOL("tx-fifo-resize"),
{}
};
@@ -634,6 +635,7 @@ static int dwc3_qcom_of_register_core(struct
platform_device *pdev)
struct dwc3_qcom*qcom = platform_get_drvdata(pdev);
struct device_node *np = pdev->dev.of_node, *dwc3_np;
struct device *dev = >dev;
+ struct property *prop;
int ret;
dwc3_np = of_get_child_by_name(np, "dwc3");
@@ -642,6 +644,14 @@ static int dwc3_qcom_of_register_core(struct
platform_device *pdev)
return -ENODEV;
}
+ prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+ if (prop) {
+ prop->name = "tx-fifo-resize";
+ ret = of_add_property(dwc3_np, prop);
+ if (ret < 0)
+ dev_info(dev, "unable to add tx-fifo-resize prop\n");
+ }
+
ret = of_platform_populate(np, NULL, NULL, dev);
if (ret) {
dev_err(dev, "failed to register dwc3 core - %d\n", ret);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v7 3/5] usb: dwc3: Resize TX FIFOs to meet EP bursting requirements
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 | 9 ++
drivers/usb/dwc3/core.h | 15
drivers/usb/dwc3/ep0.c| 2 +
drivers/usb/dwc3/gadget.c | 214 ++
4 files changed, 240 insertions(+)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 6969196..4f78370 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1238,6 +1238,7 @@ static void dwc3_get_properties(struct dwc3 *dwc)
u8 rx_max_burst_prd;
u8 tx_thr_num_pkt_prd;
u8 tx_max_burst_prd;
+ u8 tx_fifo_resize_max_num;
/* default to highest possible threshold */
lpm_nyet_threshold = 0xf;
@@ -1251,6 +1252,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
*/
hird_threshold = 12;
+ tx_fifo_resize_max_num = 6;
+
dwc->maximum_speed = usb_get_maximum_speed(dev);
dwc->dr_mode = usb_get_dr_mode(dev);
dwc->hsphy_mode = of_usb_get_phy_mode(dev->of_node);
@@ -1284,6 +1287,10 @@ 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->do_fifo_resize = device_property_read_bool(dev,
+ "tx-fifo-resize");
+ device_property_read_u8(dev, "tx-fifo-max-num",
+ _fifo_resize_max_num);
dwc->disable_scramble_quirk = device_property_read_bool(dev,
"snps,disable_scramble_quirk");
@@ -1349,6 +1356,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
dwc->tx_max_burst_prd = tx_max_burst_prd;
dwc->imod_interval = 0;
+
+ dwc->tx_fifo_resize_max_num = tx_fifo_resize_max_num;
}
/* check whether the core supports IMOD */
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index eec1cf4..411b904 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1007,6 +1007,7 @@ struct dwc3_scratchpad_array {
* @rx_max_burst_prd: max periodic ESS receive burst size
* @tx_thr_num_pkt_prd: periodic ESS transmit packet count
* @tx_max_burst_prd: max periodic ESS transmit burst size
+ * @tx_fifo_resize_max_num: max number of fifos allocated during txfifo resize
* @hsphy_interface: "utmi" or "ulpi"
* @connected: true when we're connected to a host, false otherwise
* @delayed_status: true when gadget driver asks for delayed status
@@ -1062,6 +1063,11 @@ struct dwc3_scratchpad_array {
* @dis_split_quirk: set to disable split boundary.
* @imod_interval: set the interrupt moderation interval in 250ns
* increments or 0 to disable.
+ * @max_cfg_eps: current max number of IN eps used across all USB configs.
+ * @last_fifo_depth: last fifo depth used to determine next fifo ram start
+ * address.
+ * @num_ep_resized: carries the current number endpoints which have had its tx
+ * fifo resized.
*/
struct dwc3 {
struct work_struct drd_work;
@@ -1210,6 +1216,7 @@ struct dwc3 {
u8 rx_max_burst_prd;
u8 tx_thr_num_pkt_prd;
u8 tx_max_burst_prd;
+ u8 tx_fifo_resize_max_num;
const char *hsphy_interface;
@@ -1223,6 +1230,7 @@ struct dwc3 {
unsignedis_utmi_l1_suspend:1;
unsignedis_fpga:1;
unsignedpending_events:1;
+ unsigneddo_fifo_resize:1;
unsignedpullups_connected:1;
unsignedsetup_packet_pending:1;
unsignedthree_stage_setup:1;
@@ -1257,6 +1265,10 @@ struct dwc3 {
unsigneddis_split_quirk:1;
u16 imod_interval;
+
+ int max_cfg_eps;
+ int last_fifo_depth;
+ int num_ep_resiz
[PATCH v7 1/5] usb: gadget: udc: core: Introduce check_config to verify USB configuration
Some UDCs may have constraints on how many high bandwidth endpoints it can
support in a certain configuration. This API allows for the composite
driver to pass down the total number of endpoints to the UDC so it can verify
it has the required resources to support the configuration.
Signed-off-by: Wesley Cheng
---
drivers/usb/gadget/udc/core.c | 25 +
include/linux/usb/gadget.h| 5 +
2 files changed, 30 insertions(+)
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 4173acd..81252e5 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -1003,6 +1003,31 @@ int usb_gadget_ep_match_desc(struct usb_gadget *gadget,
}
EXPORT_SYMBOL_GPL(usb_gadget_ep_match_desc);
+/**
+ * usb_gadget_check_config - checks if the UDC can support the number of eps
+ * @gadget: controller to check the USB configuration
+ * @ep_map: bitmap of endpoints being requested by a USB configuration
+ *
+ * Ensure that a UDC is able to support the number of endpoints within a USB
+ * configuration, and that there are no resource limitations to support all
+ * requested eps.
+ *
+ * Returns zero on success, else a negative errno.
+ */
+int usb_gadget_check_config(struct usb_gadget *gadget, unsigned long ep_map)
+{
+ int ret = 0;
+
+ if (!gadget->ops->check_config)
+ goto out;
+
+ ret = gadget->ops->check_config(gadget, ep_map);
+
+out:
+ return ret;
+}
+EXPORT_SYMBOL_GPL(usb_gadget_check_config);
+
/* - */
static void usb_gadget_state_work(struct work_struct *work)
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index ee04ef2..9fb69eb 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -328,6 +328,7 @@ struct usb_gadget_ops {
struct usb_ep *(*match_ep)(struct usb_gadget *,
struct usb_endpoint_descriptor *,
struct usb_ss_ep_comp_descriptor *);
+ int (*check_config)(struct usb_gadget *gadget, unsigned long
ep_map);
};
/**
@@ -607,6 +608,7 @@ int usb_gadget_connect(struct usb_gadget *gadget);
int usb_gadget_disconnect(struct usb_gadget *gadget);
int usb_gadget_deactivate(struct usb_gadget *gadget);
int usb_gadget_activate(struct usb_gadget *gadget);
+int usb_gadget_check_config(struct usb_gadget *gadget, unsigned long ep_map);
#else
static inline int usb_gadget_frame_number(struct usb_gadget *gadget)
{ return 0; }
@@ -630,6 +632,9 @@ static inline int usb_gadget_deactivate(struct usb_gadget
*gadget)
{ return 0; }
static inline int usb_gadget_activate(struct usb_gadget *gadget)
{ return 0; }
+static inline int usb_gadget_check_config(struct usb_gadget *gadget,
+ unsigned long ep_map)
+{ return 0; }
#endif /* CONFIG_USB_GADGET */
/*-*/
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v7 0/5] Re-introduce TX FIFO resize for larger EP bursting
Changes in V7: - Added a new property tx-fifo-max-num for limiting how much fifo space the resizing logic can allocate for endpoints with large burst values. This can differ across platforms, and tie in closely with overall system latency. - Added recommended checks for DWC32. - Added changes to set the tx-fifo-resize property from dwc3-qcom by default instead of modifying the current DTSI files. - Added comments on all APIs/variables introduced. - Updated the DWC3 YAML to include a better description of the tx-fifo-resize property and added an entry for tx-fifo-max-num. Changes in V6: - Rebased patches to usb-testing. - Renamed to PATCH series instead of RFC. - Checking for fs_descriptors instead of ss_descriptors for determining the endpoint count for a particular configuration. - Re-ordered patch series to fix patch dependencies. Changes in V5: - Added check_config() logic, which is used to communicate the number of EPs used in a particular configuration. Based on this, the DWC3 gadget driver has the ability to know the maximum number of eps utilized in all configs. This helps reduce unnecessary allocation to unused eps, and will catch fifo allocation issues at bind() time. - Fixed variable declaration to single line per variable, and reverse xmas. - Created a helper for fifo clearing, which is used by ep0.c Changes in V4: - Removed struct dwc3* as an argument for dwc3_gadget_resize_tx_fifos() - Removed WARN_ON(1) in case we run out of fifo space Changes in V3: - Removed "Reviewed-by" tags - Renamed series back to RFC - Modified logic to ensure that fifo_size is reset if we pass the minimum threshold. Tested with binding multiple FDs requesting 6 FIFOs. Changes in V2: - Modified TXFIFO resizing logic to ensure that each EP is reserved a FIFO. - Removed dev_dbg() prints and fixed typos from patches - Added some more description on the dt-bindings commit message Currently, there is no functionality to allow for resizing the TXFIFOs, and relying on the HW default setting for the TXFIFO depth. In most cases, the HW default is probably sufficient, but for USB compositions that contain multiple functions that require EP bursting, the default settings might not be enough. Also to note, the current SW will assign an EP to a function driver w/o checking to see if the TXFIFO size for that particular EP is large enough. (this is a problem if there are multiple HW defined values for the TXFIFO size) It is mentioned in the SNPS databook that a minimum of TX FIFO depth = 3 is required for an EP that supports bursting. Otherwise, there may be frequent occurences of bursts ending. For high bandwidth functions, such as data tethering (protocols that support data aggregation), mass storage, and media transfer protocol (over FFS), the bMaxBurst value can be large, and a bigger TXFIFO depth may prove to be beneficial in terms of USB throughput. (which can be associated to system access latency, etc...) It allows for a more consistent burst of traffic, w/o any interruptions, as data is readily available in the FIFO. With testing done using the mass storage function driver, the results show that with a larger TXFIFO depth, the bandwidth increased significantly. Test Parameters: - Platform: Qualcomm SM8150 - bMaxBurst = 6 - USB req size = 256kB - Num of USB reqs = 16 - USB Speed = Super-Speed - Function Driver: Mass Storage (w/ ramdisk) - Test Application: CrystalDiskMark Results: TXFIFO Depth = 3 max packets Test Case | Data Size | AVG tput (in MB/s) --- Sequential|1 GB x | Read |9 loops| 193.60 | | 195.86 | | 184.77 | | 193.60 --- TXFIFO Depth = 6 max packets Test Case | Data Size | AVG tput (in MB/s) --- Sequential|1 GB x | Read |9 loops| 287.35 | | 304.94 | | 289.64 | | 293.61 ------- Wesley Cheng (5): usb: gadget: udc: core: Introduce check_config to verify USB configuration usb: gadget: configfs: Check USB configuration before adding usb: dwc3: Resize TX FIFOs to meet EP bursting requirements usb: dwc3: dwc3-qcom: Enable tx-fifo-resize property by default dt-bindings: usb: dwc3: Update dwc3 TX fifo properties .../devicetree/bindings/usb/snps,dwc3.yaml | 15 +- drivers/usb/dwc3/core.c| 9 + drivers/usb/dwc3/core.h| 15 ++ drivers/usb/dwc3/dwc3-qcom.c | 10 + drivers/usb/dwc3/ep0.c | 2 + drivers/usb/dwc3/gadget.c | 214 + drivers/usb/gadget/configfs.c | 22 +++ drivers/usb/gadget/udc/core.c | 25 +++ include
Re: [PATCH v6 3/4] usb: dwc3: Resize TX FIFOs to meet EP bursting requirements
On 1/25/2021 9:15 PM, Bjorn Andersson wrote:
> On Mon 25 Jan 22:32 CST 2021, Wesley Cheng wrote:
>> On 1/25/2021 5:55 PM, Bjorn Andersson wrote:
>>> On Mon 25 Jan 19:14 CST 2021, Wesley Cheng wrote:
>>>
>>>>
>>>>
>>>> On 1/22/2021 9:12 AM, Bjorn Andersson wrote:
>>>>> On Thu 21 Jan 22:01 CST 2021, Wesley Cheng wrote:
>>>>>
>>>>
>>>> Hi Bjorn,
>>>>>
>>>>> Under what circumstances should we specify this? And in particular are
>>>>> there scenarios (in the Qualcomm platforms) where this must not be set?
>>>>> The TXFIFO dynamic allocation is actually a feature within the DWC3
>>>> controller, and isn't specifically for QCOM based platforms. It won't
>>>> do any harm functionally if this flag is not set, as this is meant for
>>>> enhancing performance/bandwidth.
>>>>
>>>>> In particular, the composition can be changed in runtime, so should we
>>>>> set this for all Qualcomm platforms?
>>>>>
>>>> Ideally yes, if we want to increase bandwith for situations where SS
>>>> endpoint bursting is set to a higher value.
>>>>
>>>>> And if that's the case, can we not just set it from the qcom driver?
>>>>>
>>>> Since this is a common DWC3 core feature, I think it would make more
>>>> sense to have it in DWC3 core instead of a vendor's DWC3 glue driver.
>>>>
>>>
>>> I don't have any objections to implementing it in the core driver, but
>>> my question is can we just skip the DT binding and just enable it from
>>> the vendor driver?
>>>
>>> Regards,
>>> Bjorn
>>>
>>
>> Hi Bjorn,
>>
>> I see. I think there are some designs which don't have a DWC3 glue
>> driver, so assuming there may be other platforms using this, there may
>> not always be a vendor driver to set this.
>>
>
> You mean that there are implementations of dwc3 without an associated
> glue driver that haven't yet realized that they need this feature?
>
> I would suggest then that we implement the core code necessary, we
> enable it from the Qualcomm glue layer and when someone realize that
> they need this without a glue driver it's going to be trivial to add the
> DT binding.
>>
> The alternative is that we're lugging around a requirement to specify
> this property in all past, present and future Qualcomm dts files - and
> then we'll need to hard code it for ACPI anyways.
>
Hi Bjorn,
Can we utilize the of_add_property() call to add the "tx-fifo-resize"
property from the dwc3_qcom_register_core() API? That way at least the
above concern would be addressed.
I'm not too familiar with the ACPI design, but I do see that the
dwc3-qcom does have an array carrying some DWC3 core properties. Looks
like we can add the tx-fifo-resize property here too.
static const struct property_entry dwc3_qcom_acpi_properties[] = {
PROPERTY_ENTRY_STRING("dr_mode", "host"),
{}
};
Thanks
Wesley Cheng
> Regards,
> Bjorn
>
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Re: [PATCH v6 3/4] usb: dwc3: Resize TX FIFOs to meet EP bursting requirements
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 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)
>>>> +{ }
>>>> #
Re: usb: dwc3: gadget: skip pullup and set_speed after suspend
On 1/21/2021 11:15 PM, Jung Daehwan wrote: > On Fri, Jan 22, 2021 03:32, Wesley cheng wrote: >> Hi Daehwan, >> >> If this is an unexpected event where userspace initiates the UDC bind >> sequence, then after the above sequence occurs, the DWC3 device should >> still be able to re-enter runtime suspend after the autosuspend timer >> expires. Since the cable is disconnected, the dwc->connected flag would >> still be false. Is this not happening in your situation? >> >> I'm just trying to understand what issue you're seeing other than the >> momentary transition from runtime suspend (due to cable disconnect) >> -->runtime resume (due to unexpected UDC bind) --> runtime suspend (due >> to nothing connected). >> >> Thanks >> Wesley cheng > > Hi Wesley, > > I don't know why but DWC3 device is not re-entering runtime-suspend in > my situation. I'm still debugging it. > Even if DWC3 re-enter runtime-suspend but it doesn't mean stopping gadget. > Are you stopping gadget manually in this case? Hi Daehwan, Sorry for the late response. So during the DWC3 runtime suspend path, we will execute dwc3_gadget_suspend() which should disable the gadget events and disable ep0 then clear RS bit. Then on runtime resume, the DWC3 will be re-enabled, and the RS bit set again. Thanks Wesley Cheng > > Best Regards, > Jung Daehwan > > -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v6 3/4] usb: dwc3: Resize TX FIFOs to meet EP bursting requirements
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
>> 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))) {
>&g
Re: [PATCH v6 3/4] usb: dwc3: Resize TX FIFOs to meet EP bursting requirements
On 1/25/2021 5:55 PM, Bjorn Andersson wrote: > On Mon 25 Jan 19:14 CST 2021, Wesley Cheng wrote: > >> >> >> On 1/22/2021 9:12 AM, Bjorn Andersson wrote: >>> On Thu 21 Jan 22:01 CST 2021, Wesley Cheng wrote: >>> >> >> Hi Bjorn, >>> >>> Under what circumstances should we specify this? And in particular are >>> there scenarios (in the Qualcomm platforms) where this must not be set? >>> The TXFIFO dynamic allocation is actually a feature within the DWC3 >> controller, and isn't specifically for QCOM based platforms. It won't >> do any harm functionally if this flag is not set, as this is meant for >> enhancing performance/bandwidth. >> >>> In particular, the composition can be changed in runtime, so should we >>> set this for all Qualcomm platforms? >>> >> Ideally yes, if we want to increase bandwith for situations where SS >> endpoint bursting is set to a higher value. >> >>> And if that's the case, can we not just set it from the qcom driver? >>> >> Since this is a common DWC3 core feature, I think it would make more >> sense to have it in DWC3 core instead of a vendor's DWC3 glue driver. >> > > I don't have any objections to implementing it in the core driver, but > my question is can we just skip the DT binding and just enable it from > the vendor driver? > > Regards, > Bjorn > Hi Bjorn, I see. I think there are some designs which don't have a DWC3 glue driver, so assuming there may be other platforms using this, there may not always be a vendor driver to set this. Thanks Wesley Cheng -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v6 1/4] usb: gadget: udc: core: Introduce check_config to verify USB configuration
On 1/21/2021 9:17 PM, Jack Pham wrote: > Hi Wesley, > > On Thu, Jan 21, 2021 at 08:01:37PM -0800, Wesley Cheng wrote: >> Some UDCs may have constraints on how many high bandwidth endpoints it can >> support in a certain configuration. This API allows for the composite >> driver to pass down the total number of endpoints to the UDC so it can verify >> it has the required resources to support the configuration. >> >> Signed-off-by: Wesley Cheng >> --- >> drivers/usb/gadget/udc/core.c | 9 + >> include/linux/usb/gadget.h| 2 ++ >> 2 files changed, 11 insertions(+) >> >> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c >> index 4173acd..469962f 100644 >> --- a/drivers/usb/gadget/udc/core.c >> +++ b/drivers/usb/gadget/udc/core.c >> @@ -1003,6 +1003,15 @@ int usb_gadget_ep_match_desc(struct usb_gadget >> *gadget, >> } >> EXPORT_SYMBOL_GPL(usb_gadget_ep_match_desc); >> >> +int usb_gadget_check_config(struct usb_gadget *gadget, unsigned long ep_map) > > You should probably add a kernel-doc for this function. > > Jack > Hi Jack, Sure, I'll update a bit more about how this API can be used. Thanks Wesley Cheng -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v6 3/4] usb: dwc3: Resize TX FIFOs to meet EP bursting requirements
On 1/22/2021 9:12 AM, Bjorn Andersson wrote: > On Thu 21 Jan 22:01 CST 2021, Wesley Cheng wrote: > Hi Bjorn, > > Under what circumstances should we specify this? And in particular are > there scenarios (in the Qualcomm platforms) where this must not be set? >The TXFIFO dynamic allocation is actually a feature within the DWC3 controller, and isn't specifically for QCOM based platforms. It won't do any harm functionally if this flag is not set, as this is meant for enhancing performance/bandwidth. > In particular, the composition can be changed in runtime, so should we > set this for all Qualcomm platforms? > Ideally yes, if we want to increase bandwith for situations where SS endpoint bursting is set to a higher value. > And if that's the case, can we not just set it from the qcom driver? > Since this is a common DWC3 core feature, I think it would make more sense to have it in DWC3 core instead of a vendor's DWC3 glue driver. Thanks Wesley Cheng > Regards, > Bjorn -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v6 1/4] usb: gadget: udc: core: Introduce check_config to verify USB configuration
On 1/22/2021 8:24 AM, Alan Stern wrote:
> On Thu, Jan 21, 2021 at 08:01:37PM -0800, Wesley Cheng wrote:
>> Some UDCs may have constraints on how many high bandwidth endpoints it can
>> support in a certain configuration. This API allows for the composite
>> driver to pass down the total number of endpoints to the UDC so it can verify
>> it has the required resources to support the configuration.
>>
>> Signed-off-by: Wesley Cheng
>
>
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -328,6 +328,7 @@ struct usb_gadget_ops {
>> struct usb_ep *(*match_ep)(struct usb_gadget *,
>> struct usb_endpoint_descriptor *,
>> struct usb_ss_ep_comp_descriptor *);
>> +int (*check_config)(struct usb_gadget *gadget, unsigned long
>> ep_map);
>> };
>>
>> /**
>> @@ -607,6 +608,7 @@ int usb_gadget_connect(struct usb_gadget *gadget);
>> int usb_gadget_disconnect(struct usb_gadget *gadget);
>> int usb_gadget_deactivate(struct usb_gadget *gadget);
>> int usb_gadget_activate(struct usb_gadget *gadget);
>> +int usb_gadget_check_config(struct usb_gadget *gadget, unsigned long
>> ep_map);
>> #else
>> static inline int usb_gadget_frame_number(struct usb_gadget *gadget)
>> { return 0; }
>
> Don't you also need an entry for the case where CONFIG_USB_GADGET isn't
> enabled?
>
> Alan Stern
>
Hi Alan,
Thanks for pointing that out. I missed that, and will add it to the
next rev.
Thanks
Wesley Cheng
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v6 0/4] Re-introduce TX FIFO resize for larger EP bursting
Changes in V6: - Rebased patches to usb-testing. - Renamed to PATCH series instead of RFC. - Checking for fs_descriptors instead of ss_descriptors for determining the endpoint count for a particular configuration. - Re-ordered patch series to fix patch dependencies. Changes in V5: - Added check_config() logic, which is used to communicate the number of EPs used in a particular configuration. Based on this, the DWC3 gadget driver has the ability to know the maximum number of eps utilized in all configs. This helps reduce unnecessary allocation to unused eps, and will catch fifo allocation issues at bind() time. - Fixed variable declaration to single line per variable, and reverse xmas. - Created a helper for fifo clearing, which is used by ep0.c Changes in V4: - Removed struct dwc3* as an argument for dwc3_gadget_resize_tx_fifos() - Removed WARN_ON(1) in case we run out of fifo space Changes in V3: - Removed "Reviewed-by" tags - Renamed series back to RFC - Modified logic to ensure that fifo_size is reset if we pass the minimum threshold. Tested with binding multiple FDs requesting 6 FIFOs. Changes in V2: - Modified TXFIFO resizing logic to ensure that each EP is reserved a FIFO. - Removed dev_dbg() prints and fixed typos from patches - Added some more description on the dt-bindings commit message Currently, there is no functionality to allow for resizing the TXFIFOs, and relying on the HW default setting for the TXFIFO depth. In most cases, the HW default is probably sufficient, but for USB compositions that contain multiple functions that require EP bursting, the default settings might not be enough. Also to note, the current SW will assign an EP to a function driver w/o checking to see if the TXFIFO size for that particular EP is large enough. (this is a problem if there are multiple HW defined values for the TXFIFO size) It is mentioned in the SNPS databook that a minimum of TX FIFO depth = 3 is required for an EP that supports bursting. Otherwise, there may be frequent occurences of bursts ending. For high bandwidth functions, such as data tethering (protocols that support data aggregation), mass storage, and media transfer protocol (over FFS), the bMaxBurst value can be large, and a bigger TXFIFO depth may prove to be beneficial in terms of USB throughput. (which can be associated to system access latency, etc...) It allows for a more consistent burst of traffic, w/o any interruptions, as data is readily available in the FIFO. With testing done using the mass storage function driver, the results show that with a larger TXFIFO depth, the bandwidth increased significantly. Test Parameters: - Platform: Qualcomm SM8150 - bMaxBurst = 6 - USB req size = 256kB - Num of USB reqs = 16 - USB Speed = Super-Speed - Function Driver: Mass Storage (w/ ramdisk) - Test Application: CrystalDiskMark Results: TXFIFO Depth = 3 max packets Test Case | Data Size | AVG tput (in MB/s) --- Sequential|1 GB x | Read |9 loops| 193.60 | | 195.86 | | 184.77 | | 193.60 --- TXFIFO Depth = 6 max packets Test Case | Data Size | AVG tput (in MB/s) --- Sequential|1 GB x | Read |9 loops| 287.35 | | 304.94 | | 289.64 | | 293.61 ------- Wesley Cheng (4): usb: gadget: udc: core: Introduce check_config to verify USB configuration usb: gadget: configfs: Check USB configuration before adding usb: dwc3: Resize TX FIFOs to meet EP bursting requirements arm64: boot: dts: qcom: sm8150: Enable dynamic TX FIFO resize logic arch/arm64/boot/dts/qcom/sm8150.dtsi | 1 + drivers/usb/dwc3/core.c | 2 + drivers/usb/dwc3/core.h | 8 ++ drivers/usb/dwc3/ep0.c | 2 + drivers/usb/dwc3/gadget.c| 194 +++ drivers/usb/gadget/configfs.c| 22 drivers/usb/gadget/udc/core.c| 9 ++ include/linux/usb/gadget.h | 2 + 8 files changed, 240 insertions(+) -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
[PATCH v6 1/4] usb: gadget: udc: core: Introduce check_config to verify USB configuration
Some UDCs may have constraints on how many high bandwidth endpoints it can
support in a certain configuration. This API allows for the composite
driver to pass down the total number of endpoints to the UDC so it can verify
it has the required resources to support the configuration.
Signed-off-by: Wesley Cheng
---
drivers/usb/gadget/udc/core.c | 9 +
include/linux/usb/gadget.h| 2 ++
2 files changed, 11 insertions(+)
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 4173acd..469962f 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -1003,6 +1003,15 @@ int usb_gadget_ep_match_desc(struct usb_gadget *gadget,
}
EXPORT_SYMBOL_GPL(usb_gadget_ep_match_desc);
+int usb_gadget_check_config(struct usb_gadget *gadget, unsigned long ep_map)
+{
+ if (!gadget->ops->check_config)
+ return 0;
+
+ return gadget->ops->check_config(gadget, ep_map);
+}
+EXPORT_SYMBOL_GPL(usb_gadget_check_config);
+
/* - */
static void usb_gadget_state_work(struct work_struct *work)
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index ee04ef2..8393fa8 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -328,6 +328,7 @@ struct usb_gadget_ops {
struct usb_ep *(*match_ep)(struct usb_gadget *,
struct usb_endpoint_descriptor *,
struct usb_ss_ep_comp_descriptor *);
+ int (*check_config)(struct usb_gadget *gadget, unsigned long
ep_map);
};
/**
@@ -607,6 +608,7 @@ int usb_gadget_connect(struct usb_gadget *gadget);
int usb_gadget_disconnect(struct usb_gadget *gadget);
int usb_gadget_deactivate(struct usb_gadget *gadget);
int usb_gadget_activate(struct usb_gadget *gadget);
+int usb_gadget_check_config(struct usb_gadget *gadget, unsigned long ep_map);
#else
static inline int usb_gadget_frame_number(struct usb_gadget *gadget)
{ return 0; }
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v6 2/4] usb: gadget: configfs: Check USB configuration before adding
Ensure that the USB gadget is able to support the configuration being
added based on the number of endpoints required from all interfaces. This
is for accounting for any bandwidth or space limitations.
Signed-off-by: Wesley Cheng
---
drivers/usb/gadget/configfs.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 0d56f33..e6de3ca5 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -1368,6 +1368,7 @@ static int configfs_composite_bind(struct usb_gadget
*gadget,
struct usb_function *f;
struct usb_function *tmp;
struct gadget_config_name *cn;
+ unsigned long ep_map = 0;
if (gadget_is_otg(gadget))
c->descriptors = otg_desc;
@@ -1397,7 +1398,28 @@ static int configfs_composite_bind(struct usb_gadget
*gadget,
list_add(>list, >func_list);
goto err_purge_funcs;
}
+ if (f->fs_descriptors) {
+ struct usb_descriptor_header **d;
+
+ d = f->fs_descriptors;
+ for (; *d; ++d) {
+ struct usb_endpoint_descriptor *ep;
+ int addr;
+
+ if ((*d)->bDescriptorType !=
USB_DT_ENDPOINT)
+ continue;
+
+ ep = (struct usb_endpoint_descriptor
*)*d;
+ addr = ((ep->bEndpointAddress & 0x80)
>> 3) |
+ (ep->bEndpointAddress & 0x0f);
+ set_bit(addr, _map);
+ }
+ }
}
+ ret = usb_gadget_check_config(cdev->gadget, ep_map);
+ if (ret)
+ goto err_purge_funcs;
+
usb_ep_autoconfig_reset(cdev->gadget);
}
if (cdev->use_os_string) {
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v6 3/4] usb: dwc3: Resize TX FIFOs to meet EP bursting requirements
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;
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;
};
#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)
+{
+ int max_packet = 1024;
+ int fifo_size;
+ int mdwidth;
+
+ mdwidth = DWC3_MDWIDTH(dwc->hwparams.hwparams0);
+ /* MDWIDTH is represented in bits, we need it in bytes */
+ mdwidth >>= 3;
+
+ fifo_size = mult * ((max_packet + mdwidth) / mdwidth) + 1;
+ return fifo_size;
+}
+
+void dwc3_gadget_clear_tx_fifos(struct dwc3 *dwc)
+{
+ struct dwc3_ep *dep;
+ int fifo_depth;
+ int size;
+ int num;
+
+ if (!dwc->needs_fifo_resize)
+ return;
+
+ /* Read ep0IN related TXFIFO size */
+ dep = dwc->eps[1];
+ size = dwc3_readl(dwc->regs, DWC3_GTXFIFOSIZ(0));
+ if (DWC3_IP_I
[PATCH v6 4/4] arm64: boot: dts: qcom: sm8150: Enable dynamic TX FIFO resize logic
Enable the flexible TX FIFO resize logic on SM8150. Using a larger TX FIFO SZ can help account for situations when system latency is greater than the USB bus transmission latency. Signed-off-by: Wesley Cheng --- arch/arm64/boot/dts/qcom/sm8150.dtsi | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi index 5270bda..c7706f4 100644 --- a/arch/arm64/boot/dts/qcom/sm8150.dtsi +++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi @@ -1569,6 +1569,7 @@ iommus = <_smmu 0x140 0>; snps,dis_u2_susphy_quirk; snps,dis_enblslpm_quirk; + tx-fifo-resize; phys = <_1_hsphy>, <_1_ssphy>; phy-names = "usb2-phy", "usb3-phy"; }; -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: usb: dwc3: gadget: skip pullup and set_speed after suspend
On 1/21/2021 12:13 AM, Jung Daehwan wrote: > On Wed, Jan 20, 2021 at 11:44:05PM -0800, Wesley Cheng wrote: >> >> >> On 1/20/2021 10:49 PM, Jung Daehwan wrote: >>> Hi, >>> >>> On Thu, Jan 21, 2021 at 01:00:32AM +, Thinh Nguyen wrote: >>>> 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 >>> >>> I checked Wesly Cheng's pathces but it's not same. >>> What I want to do for this patch is to avoid pullup from platform side. >>> (android in my case) >>> >>> It's possible that platform side tries to pullup by UDC_Store after usb is >>> already disconnected. >>> It can finally run controller and enable irq. >>> >>> I think we have to avoid it and other possible things related to platform >>> side. >>> >>> >> >> Hi Daehwan, >> >> I think what you're trying to do is to avoid the unnecessary runtime >> resume if the cable is disconnected and userspace attempts to >> bind/unbind the UDC. >> >> I'm not exactly sure what patches you've pulled in, but assuming you >> didn't pull in any of the recent suspend changes: >> >> usb: dwc3: gadget: Allow runtime suspend if UDC unbinded >> usb: dwc3: gadget: Preserve UDC max speed setting >> >> Please consider the following scenario: >> 1. USB connected >> 2. UDC unbinded >> 3. DWC3 will continue to stay in runtime active, since dwc->connected = >> true >> >> In this scenario, we should allow the DWC3 to enter runtime suspend, >> since runstop has been disabled. >> >> If you have pulled in the above changes, and adding your changes on top >> of it, then consider the following: >> 1. USB connected >> 2. UDC unbinded >> 3. DWC enters runtime suspend (due to the above changes) >> 4. UDC binded >> >> The check for pm_runtime_suspended() will block step#4 from re-enabling >> the runstop bit even if the USB cable is connected. >> >> Thanks >> Wesley Cheng >> >> -- >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, >> a Linux Foundation Collaborative Project >> > > Hi Wesley > > The check for runtime_suspended() will block re-enabling the runstop bit as > you said after pulling your patches in. > > UDC is contolled by userspace and it's possible UDC can be binded > unexpectedly. That's why I think it needs to handle it even if the > problem is from userspace. > > Below is an example in my environment. > > 1. USB disconnected > 2. UDC unbind
Re: usb: dwc3: gadget: skip pullup and set_speed after suspend
On 1/20/2021 10:49 PM, Jung Daehwan wrote: > Hi, > > On Thu, Jan 21, 2021 at 01:00:32AM +, Thinh Nguyen wrote: >> 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 > > I checked Wesly Cheng's pathces but it's not same. > What I want to do for this patch is to avoid pullup from platform side. > (android in my case) > > It's possible that platform side tries to pullup by UDC_Store after usb is > already disconnected. > It can finally run controller and enable irq. > > I think we have to avoid it and other possible things related to platform > side. > > Hi Daehwan, I think what you're trying to do is to avoid the unnecessary runtime resume if the cable is disconnected and userspace attempts to bind/unbind the UDC. I'm not exactly sure what patches you've pulled in, but assuming you didn't pull in any of the recent suspend changes: usb: dwc3: gadget: Allow runtime suspend if UDC unbinded usb: dwc3: gadget: Preserve UDC max speed setting Please consider the following scenario: 1. USB connected 2. UDC unbinded 3. DWC3 will continue to stay in runtime active, since dwc->connected = true In this scenario, we should allow the DWC3 to enter runtime suspend, since runstop has been disabled. If you have pulled in the above changes, and adding your changes on top of it, then consider the following: 1. USB connected 2. UDC unbinded 3. DWC enters runtime suspend (due to the above changes) 4. UDC binded The check for pm_runtime_suspended() will block step#4 from re-enabling the runstop bit even if the USB cable is connected. Thanks Wesley Cheng -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH 3/3] usb: gadget: configfs: Add a specific configFS reset callback
On 1/4/2021 7:45 AM, Greg KH wrote:
> On Tue, Dec 29, 2020 at 03:03:31PM -0800, Wesley Cheng wrote:
>> In order for configFS based USB gadgets to set the proper charge current
>> for bus reset scenarios, expose a separate reset callback to set the
>> current to 100mA based on the USB battery charging specification.
>>
>> Reviewed-by: Peter Chen
>> Signed-off-by: Wesley Cheng
>> ---
>> drivers/usb/gadget/configfs.c | 24 +++-
>> 1 file changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
>> index 56051bb97349..80ca7ff2fb97 100644
>> --- a/drivers/usb/gadget/configfs.c
>> +++ b/drivers/usb/gadget/configfs.c
>> @@ -1481,6 +1481,28 @@ static void configfs_composite_disconnect(struct
>> usb_gadget *gadget)
>> spin_unlock_irqrestore(>spinlock, flags);
>> }
>>
>> +static void configfs_composite_reset(struct usb_gadget *gadget)
>> +{
>> +struct usb_composite_dev *cdev;
>> +struct gadget_info *gi;
>> +unsigned long flags;
>> +
>> +cdev = get_gadget_data(gadget);
>> +if (!cdev)
>> +return;
>> +
>> +gi = container_of(cdev, struct gadget_info, cdev);
>> +spin_lock_irqsave(>spinlock, flags);
>> +cdev = get_gadget_data(gadget);
>> +if (!cdev || gi->unbind) {
>> +spin_unlock_irqrestore(>spinlock, flags);
>> +return;
>> +}
>> +
>> +composite_reset(gadget);
>> +spin_unlock_irqrestore(>spinlock, flags);
>> +}
>> +
>> static void configfs_composite_suspend(struct usb_gadget *gadget)
>> {
>> struct usb_composite_dev *cdev;
>> @@ -1530,7 +1552,7 @@ static const struct usb_gadget_driver
>> configfs_driver_template = {
>> .unbind = configfs_composite_unbind,
>>
>> .setup = configfs_composite_setup,
>> -.reset = configfs_composite_disconnect,
>> +.reset = configfs_composite_reset,
>> .disconnect = configfs_composite_disconnect,
>>
>> .suspend= configfs_composite_suspend,
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>
>
> So this changes the existing userspace functionality? What will break
> because of this now unexpected change?
>
> thanks,
>
> greg k-h
>
Hi Greg,
Happy new years! This wouldn't affect the userspace interaction with
configFS, as this is modifying the reset callback for the UDC core. The
reset callback is only executed during usb_gadget_udc_reset(), which is
specifically run when vendor UDC drivers (i.e. DWC3 gadget) receive a
USB bus reset interrupt. This is similar to the composite.c patch,
because for configFS based gadgets, they do not directly register the
USB composite ops and have their own routines.
Thanks
Wesley Cheng
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v2 0/2] Allow DWC3 runtime suspend if UDC is unbinded
Changes in v2: - Modified logic for executing the runtime PM resume. Using a sychronous get call to avoid race conditions. During the following scenario, the DWC3 runtime suspend routine is blocked as the connected flag is still true: 1. Enumerate device w/ host. 2. Gadget is unbinded - echo "" > /sys/kernel/config/usb_gadget/g1/UDC 3. Disconnect the USB cable (VBUS low) 4. No dwc3_gadget_disconnect_interrupt() seen (since controller is halted from step#1) 5. Runtime PM autosuspend fails due to "dwc->connected" being true (cleared in dwc3_gadget_disconnect_interrupt()) 6. Gadget binded - echo udc_name > /sys/kernel/config/usb_gadget/g1/UDC 7. No runtime suspend until cable is plugged in and out Technically, for device initiated disconnects, there is no active session/link with the host, so the DWC3 controller should be allowed to go into a low power state. Also, we need to now consider when re-binding the UDC, dwc3_gadget_set_speed() is executed before dwc3_gadget_pullup(), so if the DWC3 controller is suspended/disabled, while accessing the DCFG, that could result in bus timeouts, etc... Change the dwc3_gadget_set_speed() to save the speed being requested, and program it during dwc3_gadget_run_stop(), which is executed during PM runtime resume. If not, previous setting will be overridden as we execute a DWC3 controller reset during PM runtime resume. Wesley Cheng (2): usb: dwc3: gadget: Allow runtime suspend if UDC unbinded usb: dwc3: gadget: Preserve UDC max speed setting drivers/usb/dwc3/core.h | 1 + drivers/usb/dwc3/gadget.c | 121 ++ 2 files changed, 71 insertions(+), 51 deletions(-) -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
[PATCH v2 2/2] usb: dwc3: gadget: Preserve UDC max speed setting
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.
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 babf977cadc0..c145da1d8ba5 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;
@@ -2325,59 +2381,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;
- break;
- case USB_SPEED_HIGH:
- reg |= DWC3_DCFG_HIGHSPEED;
- break;
-
[PATCH 3/3] usb: gadget: configfs: Add a specific configFS reset callback
In order for configFS based USB gadgets to set the proper charge current
for bus reset scenarios, expose a separate reset callback to set the
current to 100mA based on the USB battery charging specification.
Reviewed-by: Peter Chen
Signed-off-by: Wesley Cheng
---
drivers/usb/gadget/configfs.c | 24 +++-
1 file changed, 23 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 56051bb97349..80ca7ff2fb97 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -1481,6 +1481,28 @@ static void configfs_composite_disconnect(struct
usb_gadget *gadget)
spin_unlock_irqrestore(>spinlock, flags);
}
+static void configfs_composite_reset(struct usb_gadget *gadget)
+{
+ struct usb_composite_dev *cdev;
+ struct gadget_info *gi;
+ unsigned long flags;
+
+ cdev = get_gadget_data(gadget);
+ if (!cdev)
+ return;
+
+ gi = container_of(cdev, struct gadget_info, cdev);
+ spin_lock_irqsave(>spinlock, flags);
+ cdev = get_gadget_data(gadget);
+ if (!cdev || gi->unbind) {
+ spin_unlock_irqrestore(>spinlock, flags);
+ return;
+ }
+
+ composite_reset(gadget);
+ spin_unlock_irqrestore(>spinlock, flags);
+}
+
static void configfs_composite_suspend(struct usb_gadget *gadget)
{
struct usb_composite_dev *cdev;
@@ -1530,7 +1552,7 @@ static const struct usb_gadget_driver
configfs_driver_template = {
.unbind = configfs_composite_unbind,
.setup = configfs_composite_setup,
- .reset = configfs_composite_disconnect,
+ .reset = configfs_composite_reset,
.disconnect = configfs_composite_disconnect,
.suspend= configfs_composite_suspend,
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v2 1/2] usb: dwc3: gadget: Allow runtime suspend if UDC unbinded
The DWC3 runtime suspend routine checks for the USB connected parameter to
determine if the controller can enter into a low power state. The
connected state is only set to false after receiving a disconnect event.
However, in the case of a device initiated disconnect (i.e. UDC unbind),
the controller is halted and a disconnect event is never generated. Set
the connected flag to false if issuing a device initiated disconnect to
allow the controller to be suspended.
Signed-off-by: Wesley Cheng
---
drivers/usb/dwc3/gadget.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 5d879b7606d5..babf977cadc0 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2012,6 +2012,17 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int
is_on)
}
}
+ /*
+* Check the return value for successful resume, or error. For a
+* successful resume, the DWC3 runtime PM resume routine will handle
+* the run stop sequence, so avoid duplicate operations here.
+*/
+ ret = pm_runtime_get_sync(dwc->dev);
+ if (!ret || ret < 0) {
+ pm_runtime_put(dwc->dev);
+ return 0;
+ }
+
/*
* Synchronize any pending event handling before executing the
controller
* halt routine.
@@ -2050,10 +2061,12 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int
is_on)
dwc->ev_buf->lpos = (dwc->ev_buf->lpos + count) %
dwc->ev_buf->length;
}
+ dwc->connected = false;
}
ret = dwc3_gadget_run_stop(dwc, is_on, false);
spin_unlock_irqrestore(>lock, flags);
+ pm_runtime_put(dwc->dev);
return ret;
}
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH 1/3] usb: dwc3: gadget: Introduce a DWC3 VBUS draw callback
Some devices support charging while in device mode. In these situations,
the USB gadget will notify the DWC3 gadget driver to modify the current
based on the enumeration and device state. The usb_phy_set_power() API
will allow external charger entities to adjust the charge current through
the notifier block.
Reviewed-by: Peter Chen
Signed-off-by: Wesley Cheng
---
drivers/usb/dwc3/gadget.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index c145da1d8ba5..69122f66978e 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2387,6 +2387,16 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g,
spin_unlock_irqrestore(>lock, flags);
}
+static int dwc3_gadget_vbus_draw(struct usb_gadget *g, unsigned int mA)
+{
+ struct dwc3 *dwc = gadget_to_dwc(g);
+
+ if (dwc->usb2_phy)
+ return usb_phy_set_power(dwc->usb2_phy, mA);
+
+ return 0;
+}
+
static const struct usb_gadget_ops dwc3_gadget_ops = {
.get_frame = dwc3_gadget_get_frame,
.wakeup = dwc3_gadget_wakeup,
@@ -2396,6 +2406,7 @@ static const struct usb_gadget_ops dwc3_gadget_ops = {
.udc_stop = dwc3_gadget_stop,
.udc_set_speed = dwc3_gadget_set_speed,
.get_config_params = dwc3_gadget_config_params,
+ .vbus_draw = dwc3_gadget_vbus_draw,
};
/* --
*/
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect
Add a specific composite reset API to differentiate between disconnect and
reset events. This is needed for adjusting the current draw accordingly
based on the USB battery charging specification. The device is only allowed
to draw the 500/900 mA (HS/SS) while in the CONFIGURED state, and only 100 mA
in the connected and UNCONFIGURED state.
Reviewed-by: Peter Chen
Signed-off-by: Wesley Cheng
---
drivers/usb/gadget/composite.c | 21 +++--
include/linux/usb/composite.h | 2 ++
2 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 05b176c82cc5..a41f7fe4b518 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -2036,7 +2036,7 @@ composite_setup(struct usb_gadget *gadget, const struct
usb_ctrlrequest *ctrl)
return value;
}
-void composite_disconnect(struct usb_gadget *gadget)
+static void __composite_disconnect(struct usb_gadget *gadget)
{
struct usb_composite_dev*cdev = get_gadget_data(gadget);
unsigned long flags;
@@ -2053,6 +2053,23 @@ void composite_disconnect(struct usb_gadget *gadget)
spin_unlock_irqrestore(>lock, flags);
}
+void composite_disconnect(struct usb_gadget *gadget)
+{
+ usb_gadget_vbus_draw(gadget, 0);
+ __composite_disconnect(gadget);
+}
+
+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.
+*/
+ usb_gadget_vbus_draw(gadget, 100);
+ __composite_disconnect(gadget);
+}
+
/*-*/
static ssize_t suspended_show(struct device *dev, struct device_attribute
*attr,
@@ -2373,7 +2390,7 @@ static const struct usb_gadget_driver
composite_driver_template = {
.unbind = composite_unbind,
.setup = composite_setup,
- .reset = composite_disconnect,
+ .reset = composite_reset,
.disconnect = composite_disconnect,
.suspend= composite_suspend,
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index 2040696d75b6..0d8a71471512 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -525,6 +525,8 @@ extern struct usb_string *usb_gstrings_attach(struct
usb_composite_dev *cdev,
extern int usb_string_ids_n(struct usb_composite_dev *c, unsigned n);
extern void composite_disconnect(struct usb_gadget *gadget);
+extern void composite_reset(struct usb_gadget *gadget);
+
extern int composite_setup(struct usb_gadget *gadget,
const struct usb_ctrlrequest *ctrl);
extern void composite_suspend(struct usb_gadget *gadget);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH 0/3] Add vbus draw support to DWC3
Some devices are connected to standard downstream ports (SDP) and draw current from them. The current rates are defined in the BC1.2 specification, and highlights the different charge rates depending on the device state. The DWC3 gadget does not currently have a mechanism to notify external drivers about how much current can be drawn. The current rates are notified by the USB gadget layer, and the DWC3 gadget will propagate this potentially to external charger drivers. Also, the USB gadget needs to be fixed to only allow 100mA current draw when receiving a bus reset from the host, as the BC1.2 specification states that this is the max current draw possible when in the connected and unconfigured state. Wesley Cheng (3): usb: dwc3: gadget: Introduce a DWC3 VBUS draw callback usb: gadget: composite: Split composite reset and disconnect usb: gadget: configfs: Add a specific configFS reset callback drivers/usb/dwc3/gadget.c | 11 +++ drivers/usb/gadget/composite.c | 21 +++-- drivers/usb/gadget/configfs.c | 24 +++- include/linux/usb/composite.h | 2 ++ 4 files changed, 55 insertions(+), 3 deletions(-) -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
[PATCH] usb: dwc3: gadget: Restart DWC3 gadget when enabling pullup
usb_gadget_deactivate/usb_gadget_activate does not execute the UDC start
operation, which may leave EP0 disabled and event IRQs disabled when
re-activating the function. Move the enabling/disabling of USB EP0 and
device event IRQs to be performed in the pullup routine.
Fixes: ae7e86108b12 ("usb: dwc3: Stop active transfers before halting the
controller")
Tested-by: Michael Tretter
Reported-by: Michael Tretter
Signed-off-by: Wesley Cheng
---
drivers/usb/dwc3/gadget.c | 14 +++---
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 5d879b7606d5..922c8b76e534 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1988,6 +1988,7 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int
is_on, int suspend)
static void dwc3_gadget_disable_irq(struct dwc3 *dwc);
static void __dwc3_gadget_stop(struct dwc3 *dwc);
+static int __dwc3_gadget_start(struct dwc3 *dwc);
static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
{
@@ -2050,6 +2051,8 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int
is_on)
dwc->ev_buf->lpos = (dwc->ev_buf->lpos + count) %
dwc->ev_buf->length;
}
+ } else {
+ __dwc3_gadget_start(dwc);
}
ret = dwc3_gadget_run_stop(dwc, is_on, false);
@@ -2224,10 +2227,6 @@ static int dwc3_gadget_start(struct usb_gadget *g,
}
dwc->gadget_driver = driver;
-
- if (pm_runtime_active(dwc->dev))
- __dwc3_gadget_start(dwc);
-
spin_unlock_irqrestore(>lock, flags);
return 0;
@@ -2253,13 +2252,6 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
unsigned long flags;
spin_lock_irqsave(>lock, flags);
-
- if (pm_runtime_suspended(dwc->dev))
- goto out;
-
- __dwc3_gadget_stop(dwc);
-
-out:
dwc->gadget_driver = NULL;
spin_unlock_irqrestore(>lock, flags);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH] usb: dwc3: gadget: Restart DWC3 gadget when enabling pullup
usb_gadget_deactivate/usb_gadget_activate does not execute the UDC start
operation, which may leave EP0 disabled and event IRQs disabled when
re-activating the function. Move the enabling/disabling of USB EP0 and
device event IRQs to be performed in the pullup routine.
Fixes: ae7e86108b12 ("usb: dwc3: Stop active transfers before halting the
controller")
Reported-by: Michael Tretter
Signed-off-by: Wesley Cheng
---
drivers/usb/dwc3/gadget.c | 14 +++---
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 5d879b7606d5..922c8b76e534 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1988,6 +1988,7 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int
is_on, int suspend)
static void dwc3_gadget_disable_irq(struct dwc3 *dwc);
static void __dwc3_gadget_stop(struct dwc3 *dwc);
+static int __dwc3_gadget_start(struct dwc3 *dwc);
static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
{
@@ -2050,6 +2051,8 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int
is_on)
dwc->ev_buf->lpos = (dwc->ev_buf->lpos + count) %
dwc->ev_buf->length;
}
+ } else {
+ __dwc3_gadget_start(dwc);
}
ret = dwc3_gadget_run_stop(dwc, is_on, false);
@@ -2224,10 +2227,6 @@ static int dwc3_gadget_start(struct usb_gadget *g,
}
dwc->gadget_driver = driver;
-
- if (pm_runtime_active(dwc->dev))
- __dwc3_gadget_start(dwc);
-
spin_unlock_irqrestore(>lock, flags);
return 0;
@@ -2253,13 +2252,6 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
unsigned long flags;
spin_lock_irqsave(>lock, flags);
-
- if (pm_runtime_suspended(dwc->dev))
- goto out;
-
- __dwc3_gadget_stop(dwc);
-
-out:
dwc->gadget_driver = NULL;
spin_unlock_irqrestore(>lock, flags);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Re: [PATCH v4] usb: dwc3: Stop active transfers before halting the controller
On 11/18/2020 2:14 AM, Michael Tretter wrote: > Hello, > > On Mon, 28 Sep 2020 17:20:59 -0700, 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. > > This patch causes a regression when using the uvc gadget with the dwc3 gadget > driver, which causes host to not be able to enumerate the USB device. > > The regression can be reproduced as follows: > > Configure the uvc gadget via configfs, which in the end binds to the driver > and calls dwc3_gadget_start. Start the uvc-gadget user space application, > which activates the function and the gadget and calls pullup enable. The UVC > Device is now detected by a USB host. > > Stop the uvc gadget application, which deactivates the gadget, calls pullup > disable and, thus, stops the dwc3 gadget. > Hi Michael, Thanks for the analysis. I think specifically, the f_uvc will use the usb_function_deactivate() API to disable the pullup (w/o calling udc_stop()) and using usb_function_activate() to do the opposite. These are triggered when an application opens/closes the V4L2 device. (your application) > Restart the uvc gadget application; the gadget is activated and pullup enable > is called, but the dwc3 gadget is not started. > Seems like the deactivate/activate calls avoid any UDC start stop operations, and only use the pullup executions to issue a soft disconnect. > The USB Host shows the following error messages and the USB device cannot be > enumerated. > > usb 3-1.1: Device not responding to setup address. > usb 3-1.1: Device not responding to setup address. > usb 3-1.1: device not accepting address 10, error -71 > usb 3-1.1: Device not responding to setup address. > usb 3-1.1: Device not responding to setup address. > usb 3-1.1: device not accepting address 11, error -71 > usb 3-1-port1: unable to enumerate USB device > >> >> Signed-off-by: Wesley Cheng >> Reviewed-by: Thinh Nguyen >> >> --- >> Changes in v4: >> - Updated comments to reference DWC3 databook sections and added direct >>quotes. >> - Changed the stop active transfer EP loop to use dwc->num_eps. >> - Moved to using dwc3_gadget_disable_irq/synchronize_irq instead of >>enable_irq/disable_irq for ensuring the interrupt handler is not pending. >> >> Changes in v3: >> - Removed DWC3_EP_ENABLED check from dwc3_gadget_stop_active_transfers() >>as dwc3_stop_active_transfer() has a check already in place. >> - Calling __dwc3_gadget_stop() which ensures that DWC3 interrupt events >>are cleared, and ep0 eps are cleared for the pullup disabled case. Not >>required to call __dwc3_gadget_start() on pullup enable, as the >>composite driver will execute udc_start() before calling pullup(). > > This change seems to be related to the regression. Maybe it is required to > call __dwc3_gadget_start() on pullup enable, but I am not sure, how this > should be handled. > > Michael > Definitely, in this situation, we would not call the udc_start() callback, which will affect the functionality after re-enabling the device. I wonder why the deactivate/activate routines don't explicitly need to start/stop the UDC? Thanks 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 | 66 ++- >> 2 files changed, 66 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 &
[PATCH 3/3] usb: gadget: configfs: Add a specific configFS reset callback
In order for configFS based USB gadgets to set the proper charge current
for bus reset scenarios, expose a separate reset callback to set the
current to 100mA based on the USB battery charging specification.
Signed-off-by: Wesley Cheng
---
drivers/usb/gadget/configfs.c | 24 +++-
1 file changed, 23 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 56051bb97349..80ca7ff2fb97 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -1481,6 +1481,28 @@ static void configfs_composite_disconnect(struct
usb_gadget *gadget)
spin_unlock_irqrestore(>spinlock, flags);
}
+static void configfs_composite_reset(struct usb_gadget *gadget)
+{
+ struct usb_composite_dev *cdev;
+ struct gadget_info *gi;
+ unsigned long flags;
+
+ cdev = get_gadget_data(gadget);
+ if (!cdev)
+ return;
+
+ gi = container_of(cdev, struct gadget_info, cdev);
+ spin_lock_irqsave(>spinlock, flags);
+ cdev = get_gadget_data(gadget);
+ if (!cdev || gi->unbind) {
+ spin_unlock_irqrestore(>spinlock, flags);
+ return;
+ }
+
+ composite_reset(gadget);
+ spin_unlock_irqrestore(>spinlock, flags);
+}
+
static void configfs_composite_suspend(struct usb_gadget *gadget)
{
struct usb_composite_dev *cdev;
@@ -1530,7 +1552,7 @@ static const struct usb_gadget_driver
configfs_driver_template = {
.unbind = configfs_composite_unbind,
.setup = configfs_composite_setup,
- .reset = configfs_composite_disconnect,
+ .reset = configfs_composite_reset,
.disconnect = configfs_composite_disconnect,
.suspend= configfs_composite_suspend,
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH 0/3] Add vbus draw support to DWC3
Some devices are connected to standard downstream ports (SDP) and draw current from them. The current rates are defined in the BC1.2 specification, and highlights the different charge rates depending on the device state. The DWC3 gadget does not currently have a mechanism to notify external drivers about how much current can be drawn. The current rates are notified by the USB gadget layer, and the DWC3 gadget will propagate this potentially to external charger drivers. Also, the USB gadget needs to be fixed to only allow 100mA current draw when receiving a bus reset from the host, as the BC1.2 specification states that this is the max current draw possible when in the connected and unconfigured state. Wesley Cheng (3): usb: dwc3: gadget: Introduce a DWC3 VBUS draw callback usb: gadget: composite: Split composite reset and disconnect usb: gadget: configfs: Add a specific configFS reset callback drivers/usb/dwc3/gadget.c | 11 +++ drivers/usb/gadget/composite.c | 21 +++-- drivers/usb/gadget/configfs.c | 24 +++- include/linux/usb/composite.h | 2 ++ 4 files changed, 55 insertions(+), 3 deletions(-) -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
[PATCH 1/3] usb: dwc3: gadget: Introduce a DWC3 VBUS draw callback
Some devices support charging while in device mode. In these situations,
the USB gadget will notify the DWC3 gadget driver to modify the current
based on the enumeration and device state. The usb_phy_set_power() API
will allow external charger entities to adjust the charge current through
the notifier block.
Signed-off-by: Wesley Cheng
---
drivers/usb/dwc3/gadget.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index c145da1d8ba5..69122f66978e 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2387,6 +2387,16 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g,
spin_unlock_irqrestore(>lock, flags);
}
+static int dwc3_gadget_vbus_draw(struct usb_gadget *g, unsigned int mA)
+{
+ struct dwc3 *dwc = gadget_to_dwc(g);
+
+ if (dwc->usb2_phy)
+ return usb_phy_set_power(dwc->usb2_phy, mA);
+
+ return 0;
+}
+
static const struct usb_gadget_ops dwc3_gadget_ops = {
.get_frame = dwc3_gadget_get_frame,
.wakeup = dwc3_gadget_wakeup,
@@ -2396,6 +2406,7 @@ static const struct usb_gadget_ops dwc3_gadget_ops = {
.udc_stop = dwc3_gadget_stop,
.udc_set_speed = dwc3_gadget_set_speed,
.get_config_params = dwc3_gadget_config_params,
+ .vbus_draw = dwc3_gadget_vbus_draw,
};
/* --
*/
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect
Add a specific composite reset API to differentiate between disconnect and
reset events. This is needed for adjusting the current draw accordingly
based on the USB battery charging specification. The device is only allowed
to draw the 500/900 mA (HS/SS) while in the CONFIGURED state, and only 100 mA
in the connected and UNCONFIGURED state.
Signed-off-by: Wesley Cheng
---
drivers/usb/gadget/composite.c | 21 +++--
include/linux/usb/composite.h | 2 ++
2 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 05b176c82cc5..a41f7fe4b518 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -2036,7 +2036,7 @@ composite_setup(struct usb_gadget *gadget, const struct
usb_ctrlrequest *ctrl)
return value;
}
-void composite_disconnect(struct usb_gadget *gadget)
+static void __composite_disconnect(struct usb_gadget *gadget)
{
struct usb_composite_dev*cdev = get_gadget_data(gadget);
unsigned long flags;
@@ -2053,6 +2053,23 @@ void composite_disconnect(struct usb_gadget *gadget)
spin_unlock_irqrestore(>lock, flags);
}
+void composite_disconnect(struct usb_gadget *gadget)
+{
+ usb_gadget_vbus_draw(gadget, 0);
+ __composite_disconnect(gadget);
+}
+
+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.
+*/
+ usb_gadget_vbus_draw(gadget, 100);
+ __composite_disconnect(gadget);
+}
+
/*-*/
static ssize_t suspended_show(struct device *dev, struct device_attribute
*attr,
@@ -2373,7 +2390,7 @@ static const struct usb_gadget_driver
composite_driver_template = {
.unbind = composite_unbind,
.setup = composite_setup,
- .reset = composite_disconnect,
+ .reset = composite_reset,
.disconnect = composite_disconnect,
.suspend= composite_suspend,
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index 2040696d75b6..0d8a71471512 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -525,6 +525,8 @@ extern struct usb_string *usb_gstrings_attach(struct
usb_composite_dev *cdev,
extern int usb_string_ids_n(struct usb_composite_dev *c, unsigned n);
extern void composite_disconnect(struct usb_gadget *gadget);
+extern void composite_reset(struct usb_gadget *gadget);
+
extern int composite_setup(struct usb_gadget *gadget,
const struct usb_ctrlrequest *ctrl);
extern void composite_suspend(struct usb_gadget *gadget);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v2 2/2] usb: dwc3: gadget: Preserve UDC max speed setting
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.
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 babf977cadc0..c145da1d8ba5 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;
@@ -2325,59 +2381,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;
- break;
- case USB_SPEED_HIGH:
- reg |= DWC3_DCFG_HIGHSPEED;
- break;
-
[PATCH v2 1/2] usb: dwc3: gadget: Allow runtime suspend if UDC unbinded
The DWC3 runtime suspend routine checks for the USB connected parameter to
determine if the controller can enter into a low power state. The
connected state is only set to false after receiving a disconnect event.
However, in the case of a device initiated disconnect (i.e. UDC unbind),
the controller is halted and a disconnect event is never generated. Set
the connected flag to false if issuing a device initiated disconnect to
allow the controller to be suspended.
Signed-off-by: Wesley Cheng
---
drivers/usb/dwc3/gadget.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 5d879b7606d5..babf977cadc0 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2012,6 +2012,17 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int
is_on)
}
}
+ /*
+* Check the return value for successful resume, or error. For a
+* successful resume, the DWC3 runtime PM resume routine will handle
+* the run stop sequence, so avoid duplicate operations here.
+*/
+ ret = pm_runtime_get_sync(dwc->dev);
+ if (!ret || ret < 0) {
+ pm_runtime_put(dwc->dev);
+ return 0;
+ }
+
/*
* Synchronize any pending event handling before executing the
controller
* halt routine.
@@ -2050,10 +2061,12 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int
is_on)
dwc->ev_buf->lpos = (dwc->ev_buf->lpos + count) %
dwc->ev_buf->length;
}
+ dwc->connected = false;
}
ret = dwc3_gadget_run_stop(dwc, is_on, false);
spin_unlock_irqrestore(>lock, flags);
+ pm_runtime_put(dwc->dev);
return ret;
}
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v2 0/2] Allow DWC3 runtime suspend if UDC is unbinded
Changes in v2: - Modified logic for executing the runtime PM resume. Using a sychronous get call to avoid race conditions. During the following scenario, the DWC3 runtime suspend routine is blocked as the connected flag is still true: 1. Enumerate device w/ host. 2. Gadget is unbinded - echo "" > /sys/kernel/config/usb_gadget/g1/UDC 3. Disconnect the USB cable (VBUS low) 4. No dwc3_gadget_disconnect_interrupt() seen (since controller is halted from step#1) 5. Runtime PM autosuspend fails due to "dwc->connected" being true (cleared in dwc3_gadget_disconnect_interrupt()) 6. Gadget binded - echo udc_name > /sys/kernel/config/usb_gadget/g1/UDC 7. No runtime suspend until cable is plugged in and out Technically, for device initiated disconnects, there is no active session/link with the host, so the DWC3 controller should be allowed to go into a low power state. Also, we need to now consider when re-binding the UDC, dwc3_gadget_set_speed() is executed before dwc3_gadget_pullup(), so if the DWC3 controller is suspended/disabled, while accessing the DCFG, that could result in bus timeouts, etc... Change the dwc3_gadget_set_speed() to save the speed being requested, and program it during dwc3_gadget_run_stop(), which is executed during PM runtime resume. If not, previous setting will be overridden as we execute a DWC3 controller reset during PM runtime resume. Wesley Cheng (2): usb: dwc3: gadget: Allow runtime suspend if UDC unbinded usb: dwc3: gadget: Preserve UDC max speed setting drivers/usb/dwc3/core.h | 1 + drivers/usb/dwc3/gadget.c | 121 ++ 2 files changed, 71 insertions(+), 51 deletions(-) -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH 1/2] usb: dwc3: gadget: Allow runtime suspend if UDC unbinded
On 11/3/2020 12:07 PM, Alan Stern wrote:
> On Tue, Nov 03, 2020 at 11:02:25AM -0800, Wesley Cheng wrote:
>>
>>
>> On 10/28/2020 6:07 PM, Alan Stern wrote:
>>>> --- a/drivers/usb/dwc3/gadget.c
>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>> @@ -1995,6 +1995,11 @@ static int dwc3_gadget_pullup(struct usb_gadget *g,
>>>> int is_on)
>>>>unsigned long flags;
>>>>int ret;
>>>>
>>>> + if (pm_runtime_suspended(dwc->dev)) {
>>>> + pm_request_resume(dwc->dev);
>>>> + return 0;
>>>> + }
>>>
>>> Isn't this racy? What happens if the controller was active but a
>>> runtime suspend occurs right here?
>>>
>>> Alan Stern
>>>
>>
>> Hi Alan,
>>
>> Ah, yes you're right. I was hoping that the PM runtime layer would be
>> utilizing the spinlock when reading out the runtime status, but even
>> then, we wouldn't be able to catch intermediate states with this API
>> (i.e. RPM_RESUMING or RPM_SUSPENDING)
>>
>> Tried a few different approaches, and came up with something like the
>> following:
>>
>> static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>> {
>> ...
>> ret = pm_runtime_get_sync(dwc->dev);
>> if (!ret) {
>> pm_runtime_put(dwc->dev);
>> return 0;
>> }
>> ...
>> pm_runtime_put(dwc->dev);
>>
>> return 0;
>> }
>>
>> I think this should be good to address your concern. The only way we
>> would be able to ensure that the runtime PM state doesn't enter
>> idle/suspend is if we increment the usage count for the duration we're
>> accessing the DWC3 registers. With the synchronous PM runtime resume
>> call, we can also ensure that no pending runtime suspends are executing
>> in parallel while running this code.
>
> That's correct.
>
>> The check for the zero return value would be for avoiding running the
>> DWC3 run stop sequence for the case where we executed the runtime PM
>> resume, as the DWC3 runtime PM resume routine will set the run stop bit
>> in there.
>
> If you need to add an explanation of this subtle point in your email
> message, then you should add a similar explanation as a comment in the
> code. And don't forget to check for ret < 0 (i.e., a resume error).
>
Hi Alan,
Got it, will do. Yes, I'll include the error conditions as well in the
actual change. Thanks again!
Thanks
Regards,
Wesley Cheng
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Re: [PATCH 1/2] usb: dwc3: gadget: Allow runtime suspend if UDC unbinded
On 10/28/2020 6:07 PM, Alan Stern wrote:
> On Wed, Oct 28, 2020 at 04:43:10PM -0700, Wesley Cheng wrote:
>> The DWC3 runtime suspend routine checks for the USB connected parameter to
>> determine if the controller can enter into a low power state. The
>> connected state is only set to false after receiving a disconnect event.
>> However, in the case of a device initiated disconnect (i.e. UDC unbind),
>> the controller is halted and a disconnect event is never generated. Set
>> the connected flag to false if issuing a device initiated disconnect to
>> allow the controller to be suspended.
>>
>> Signed-off-by: Wesley Cheng
>> ---
>> 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 5d879b7606d5..6364429b2122 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -1995,6 +1995,11 @@ static int dwc3_gadget_pullup(struct usb_gadget *g,
>> int is_on)
>> unsigned long flags;
>> int ret;
>>
>> +if (pm_runtime_suspended(dwc->dev)) {
>> +pm_request_resume(dwc->dev);
>> +return 0;
>> +}
>
> Isn't this racy? What happens if the controller was active but a
> runtime suspend occurs right here?
>
> Alan Stern
>
Hi Alan,
Ah, yes you're right. I was hoping that the PM runtime layer would be
utilizing the spinlock when reading out the runtime status, but even
then, we wouldn't be able to catch intermediate states with this API
(i.e. RPM_RESUMING or RPM_SUSPENDING)
Tried a few different approaches, and came up with something like the
following:
static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
{
...
ret = pm_runtime_get_sync(dwc->dev);
if (!ret) {
pm_runtime_put(dwc->dev);
return 0;
}
...
pm_runtime_put(dwc->dev);
return 0;
}
I think this should be good to address your concern. The only way we
would be able to ensure that the runtime PM state doesn't enter
idle/suspend is if we increment the usage count for the duration we're
accessing the DWC3 registers. With the synchronous PM runtime resume
call, we can also ensure that no pending runtime suspends are executing
in parallel while running this code.
The check for the zero return value would be for avoiding running the
DWC3 run stop sequence for the case where we executed the runtime PM
resume, as the DWC3 runtime PM resume routine will set the run stop bit
in there.
Thanks
Regards,
Wesley Cheng
>> +
>> is_on = !!is_on;
>>
>> /*
>> @@ -2050,6 +2055,7 @@ static int dwc3_gadget_pullup(struct usb_gadget *g,
>> int is_on)
>> dwc->ev_buf->lpos = (dwc->ev_buf->lpos + count) %
>> dwc->ev_buf->length;
>> }
>> +dwc->connected = false;
>> }
>>
>> ret = dwc3_gadget_run_stop(dwc, is_on, false);
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Re: [PATCH 2/2] usb: dwc3: gadget: Preserve UDC max speed setting
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.
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.
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
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH 1/2] usb: dwc3: gadget: Allow runtime suspend if UDC unbinded
The DWC3 runtime suspend routine checks for the USB connected parameter to
determine if the controller can enter into a low power state. The
connected state is only set to false after receiving a disconnect event.
However, in the case of a device initiated disconnect (i.e. UDC unbind),
the controller is halted and a disconnect event is never generated. Set
the connected flag to false if issuing a device initiated disconnect to
allow the controller to be suspended.
Signed-off-by: Wesley Cheng
---
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 5d879b7606d5..6364429b2122 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1995,6 +1995,11 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int
is_on)
unsigned long flags;
int ret;
+ if (pm_runtime_suspended(dwc->dev)) {
+ pm_request_resume(dwc->dev);
+ return 0;
+ }
+
is_on = !!is_on;
/*
@@ -2050,6 +2055,7 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int
is_on)
dwc->ev_buf->lpos = (dwc->ev_buf->lpos + count) %
dwc->ev_buf->length;
}
+ dwc->connected = false;
}
ret = dwc3_gadget_run_stop(dwc, is_on, false);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH 0/2] Allow DWC3 runtime suspend if UDC is unbinded
During the following scenario, the DWC3 runtime suspend routine is blocked as the connected flag is still true: 1. Enumerate device w/ host. 2. Gadget is unbinded - echo "" > /sys/kernel/config/usb_gadget/g1/UDC 3. Disconnect the USB cable (VBUS low) 4. No dwc3_gadget_disconnect_interrupt() seen (since controller is halted from step#1) 5. Runtime PM autosuspend fails due to "dwc->connected" being true (cleared in dwc3_gadget_disconnect_interrupt()) 6. Gadget binded - echo udc_name > /sys/kernel/config/usb_gadget/g1/UDC 7. No runtime suspend until cable is plugged in and out Technically, for device initiated disconnects, there is no active session/link with the host, so the DWC3 controller should be allowed to go into a low power state. Also, we need to now consider when re-binding the UDC, dwc3_gadget_set_speed() is executed before dwc3_gadget_pullup(), so if the DWC3 controller is suspended/disabled, while accessing the DCFG, that could result in bus timeouts, etc... Change the dwc3_gadget_set_speed() to save the speed being requested, and program it during dwc3_gadget_run_stop(), which is executed during PM runtime resume. If not, previous setting will be overridden as we execute a DWC3 controller reset during PM runtime resume. Wesley Cheng (2): usb: dwc3: gadget: Allow runtime suspend if UDC unbinded usb: dwc3: gadget: Preserve UDC max speed setting drivers/usb/dwc3/core.h | 1 + drivers/usb/dwc3/gadget.c | 114 +- 2 files changed, 64 insertions(+), 51 deletions(-) -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
[PATCH 2/2] usb: dwc3: gadget: Preserve UDC max speed setting
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.
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;
- break;
- case USB_SPEED_HIGH:
- reg |= DWC3_DCFG_HIGHSPEED;
- break;
-
Re: [PATCH v10 4/4] arm64: boot: dts: qcom: pm8150b: Add DTS node for PMIC VBUS booster
On 10/13/2020 8:03 AM, Rob Herring wrote:
> On Thu, Oct 08, 2020 at 04:59:34PM -0700, Wesley Cheng wrote:
>> Add the required DTS node for the USB VBUS output regulator, which is
>> available on PM8150B. This will provide the VBUS source to connected
>> peripherals.
>>
>> Signed-off-by: Wesley Cheng
>> ---
>> arch/arm64/boot/dts/qcom/pm8150b.dtsi | 6 ++
>> arch/arm64/boot/dts/qcom/sm8150-mtp.dts | 4
>> 2 files changed, 10 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/pm8150b.dtsi
>> b/arch/arm64/boot/dts/qcom/pm8150b.dtsi
>> index 2bf385f5a55a..49ea597cc0c5 100644
>> --- a/arch/arm64/boot/dts/qcom/pm8150b.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/pm8150b.dtsi
>> @@ -53,6 +53,12 @@ power-on@800 {
>> status = "disabled";
>> };
>>
>> +pm8150b_vbus: regulator@1100 {
>> +compatible = "qcom,pm8150b-vbus-reg";
>> +status = "disabled";
>> +reg = <0x1100>;
>> +};
>> +
>> pm8150b_typec: usb-typec@1500 {
>> compatible = "qcom,pm8150b-usb-typec";
>> status = "disabled";
>> diff --git a/arch/arm64/boot/dts/qcom/sm8150-mtp.dts
>> b/arch/arm64/boot/dts/qcom/sm8150-mtp.dts
>> index 6c6325c3af59..ba3b5b802954 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8150-mtp.dts
>> +++ b/arch/arm64/boot/dts/qcom/sm8150-mtp.dts
>> @@ -409,6 +409,10 @@ _mem_phy {
>> vdda-pll-max-microamp = <19000>;
>> };
>>
>> +_vbus {
>> +status = "okay";
>> +};
>
> Why aren't you enabling the TypeC node and providing a complete example?
>
Hi Rob,
I have another patch series which enables the type C node and adds QMP
PHY driver changes for setting the SS lane select MUX.
https://patchwork.kernel.org/project/linux-arm-msm/list/?series=361971
Just wanted to work on getting a PMIC based type C driver out there,
which can be utilized in designs where the QMP PHY lane select mux is
not going to be used. (ie using a FUSB340 as a lane select mux instead
of the QMP PHY mux)
Thanks
Regards,
Wesley Cheng
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Re: [PATCH v10 2/4] dt-bindings: usb: Add Qualcomm PMIC type C controller dt-binding
On 10/13/2020 8:00 AM, Rob Herring wrote: > On Thu, Oct 08, 2020 at 04:59:32PM -0700, Wesley Cheng wrote: >> Introduce the dt-binding for enabling USB type C orientation and role >> detection using the PM8150B. The driver will be responsible for receiving >> the interrupt at a state change on the CC lines, reading the >> orientation/role, and communicating this information to the remote >> clients, which can include a role switch node and a type C switch. >> >> Signed-off-by: Wesley Cheng >> --- >> .../bindings/usb/qcom,pmic-typec.yaml | 115 ++ >> 1 file changed, 115 insertions(+) >> create mode 100644 >> Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml >> >> diff --git a/Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml >> b/Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml >> new file mode 100644 >> index ..40e0a296f922 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml >> @@ -0,0 +1,115 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: "http://devicetree.org/schemas/usb/qcom,pmic-typec.yaml#; >> +$schema: "http://devicetree.org/meta-schemas/core.yaml#; >> + >> +title: Qualcomm PMIC based USB type C Detection Driver >> + >> +maintainers: >> + - Wesley Cheng >> + >> +description: | >> + Qualcomm PMIC Type C Detect >> + >> +properties: >> + compatible: >> +enum: >> + - qcom,pm8150b-usb-typec >> + >> + reg: >> +maxItems: 1 >> +description: Type C base address >> + >> + interrupts: >> +maxItems: 1 >> +description: CC change interrupt from PMIC >> + >> + port: >> +description: Remote endpoint connection to the DRD switch >> +type: object > > I don't understand what this is supposed to be. You'll have to expand > the example or provide a block diagram of what the connections/routing > looks like. > Hi Rob, The "port" node is going to be the connection to the usb role switch device, which will be listening for the USB type C port change events. (i.e handling USB role events, etc...) In previous patches, this was part of the connector node, which may not have made much sense, as the connector model is used to describe the HW connections within a design. The role switch endpoint is more of a SW interaction between drivers, thus the motivation to remove it from the connector node. I think the current usb-connector design is OK as it is, since the only component essentially involved in the SS path is the SS MUX that we've been discussing, and this is true among designs that are supporting SSUSB. Thanks Regards, Wesley Cheng -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
[PATCH v2 0/4] Enable USB type C support on SM8150
Changes in v2: - Added patch to fix a typo in dwc3_qcom_vbus_override_enable() - Modified dwc3_qcom_find_usb_connector_match() to search the child nodes for the connector device as well - Moved out the DRD switch remote endpoint from the connector node in the pm8150b_typec device This series adds support for setting of the orientation multiplexor within the QMP PHY based on the detection output from the PM8150B. It will also introduce a role switch in DWC3 QCOM, which is used so that the DWC3 QCOM glue can receive role switch change events, and set the vbus override accordingly. This event will then be propagated down to the DWC3 core driver, by the DWC3 QCOM getting a handle to the DWC3 core's role switch. Wesley Cheng (4): arm64: boot: dts: qcom: sm8150: Add nodes for PMIC based typec detection phy: qcom-qmp: Register as a typec switch for orientation detection usb: dwc3: dwc3-qcom: Find USB connector and register role switch usb: dwc3: dwc3-qcom: Fix typo in the dwc3 vbus override API arch/arm64/boot/dts/qcom/sm8150-mtp.dts | 40 +++- drivers/phy/qualcomm/Kconfig| 11 ++ drivers/phy/qualcomm/phy-qcom-qmp.c | 70 - drivers/usb/dwc3/dwc3-qcom.c| 128 ++-- 4 files changed, 239 insertions(+), 10 deletions(-) -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
[PATCH v2 4/4] usb: dwc3: dwc3-qcom: Fix typo in the dwc3 vbus override API
There was an extra character in the dwc3_qcom_vbus_override_enable()
function. Removed the extra character.
Signed-off-by: Wesley Cheng
---
drivers/usb/dwc3/dwc3-qcom.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 340760ef0e01..236afbfe01d9 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -107,7 +107,7 @@ static inline void dwc3_qcom_clrbits(void __iomem *base,
u32 offset, u32 val)
readl(base + offset);
}
-static void dwc3_qcom_vbus_overrride_enable(struct dwc3_qcom *qcom, bool
enable)
+static void dwc3_qcom_vbus_override_enable(struct dwc3_qcom *qcom, bool enable)
{
if (enable) {
dwc3_qcom_setbits(qcom->qscratch_base, QSCRATCH_SS_PHY_CTRL,
@@ -128,7 +128,7 @@ static int dwc3_qcom_vbus_notifier(struct notifier_block
*nb,
struct dwc3_qcom *qcom = container_of(nb, struct dwc3_qcom, vbus_nb);
/* enable vbus override for device mode */
- dwc3_qcom_vbus_overrride_enable(qcom, event);
+ dwc3_qcom_vbus_override_enable(qcom, event);
qcom->mode = event ? USB_DR_MODE_PERIPHERAL : USB_DR_MODE_HOST;
return NOTIFY_DONE;
@@ -140,7 +140,7 @@ static int dwc3_qcom_host_notifier(struct notifier_block
*nb,
struct dwc3_qcom *qcom = container_of(nb, struct dwc3_qcom, host_nb);
/* disable vbus override in host mode */
- dwc3_qcom_vbus_overrride_enable(qcom, !event);
+ dwc3_qcom_vbus_override_enable(qcom, !event);
qcom->mode = event ? USB_DR_MODE_HOST : USB_DR_MODE_PERIPHERAL;
return NOTIFY_DONE;
@@ -223,7 +223,7 @@ static int dwc3_qcom_usb_role_switch_set(struct
usb_role_switch *sw,
qcom->mode = (role == USB_ROLE_HOST) ? USB_DR_MODE_HOST :
USB_DR_MODE_PERIPHERAL;
- dwc3_qcom_vbus_overrride_enable(qcom, enable);
+ dwc3_qcom_vbus_override_enable(qcom, enable);
return 0;
}
@@ -750,7 +750,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
/* enable vbus override for device mode */
if (qcom->mode == USB_DR_MODE_PERIPHERAL)
- dwc3_qcom_vbus_overrride_enable(qcom, true);
+ dwc3_qcom_vbus_override_enable(qcom, true);
if (dwc3_qcom_find_usb_connector(pdev)) {
ret = dwc3_qcom_setup_role_switch(qcom);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v2 2/4] phy: qcom-qmp: Register as a typec switch for orientation detection
The lane select switch for USB typec orientation is within the USB QMP PHY.
the current device. It could be connected through an endpoint, to an
independent device handling the typec detection, ie the QCOM SPMI typec
driver.
Signed-off-by: Wesley Cheng
---
drivers/phy/qualcomm/Kconfig| 11 +
drivers/phy/qualcomm/phy-qcom-qmp.c | 70 +++--
2 files changed, 78 insertions(+), 3 deletions(-)
diff --git a/drivers/phy/qualcomm/Kconfig b/drivers/phy/qualcomm/Kconfig
index 928db510b86c..43f46a1b3db1 100644
--- a/drivers/phy/qualcomm/Kconfig
+++ b/drivers/phy/qualcomm/Kconfig
@@ -48,6 +48,17 @@ config PHY_QCOM_QMP
Enable this to support the QMP PHY transceiver that is used
with controllers such as PCIe, UFS, and USB on Qualcomm chips.
+if PHY_QCOM_QMP
+config PHY_QCOM_QMP_TYPEC
+ bool "Enable QCOM QMP PHY Type C Switch Support"
+ depends on PHY_QCOM_QMP=y && TYPEC=y || PHY_QCOM_QMP=m && TYPEC
+ help
+ Register a type C switch from the QMP PHY driver for type C
+ orientation support. This has dependencies with if the type C kernel
+ configuration is enabled or not. This support will not be present if
+ USB type C is disabled.
+endif
+
config PHY_QCOM_QUSB2
tristate "Qualcomm QUSB2 PHY Driver"
depends on OF && (ARCH_QCOM || COMPILE_TEST)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c
b/drivers/phy/qualcomm/phy-qcom-qmp.c
index 562053ce9455..29d8a3570328 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -19,6 +19,7 @@
#include
#include
#include
+#include
#include
@@ -66,6 +67,9 @@
/* QPHY_V3_PCS_MISC_CLAMP_ENABLE register bits */
#define CLAMP_EN BIT(0) /* enables i/o clamp_n */
+#define SW_PORTSELECT_VAL BIT(0)
+#define SW_PORTSELECT_MUX BIT(1)
+
#define PHY_INIT_COMPLETE_TIMEOUT 1
#define POWER_DOWN_DELAY_US_MIN10
#define POWER_DOWN_DELAY_US_MAX11
@@ -1845,6 +1849,8 @@ struct qmp_phy {
* @phy_initialized: indicate if PHY has been initialized
* @mode: current PHY mode
* @ufs_reset: optional UFS PHY reset handle
+ * @sw: typec switch for receiving orientation changes
+ * @orientation: carries current CC orientation
*/
struct qcom_qmp {
struct device *dev;
@@ -1864,6 +1870,8 @@ struct qcom_qmp {
enum phy_mode mode;
struct reset_control *ufs_reset;
+ struct typec_switch *sw;
+ enum typec_orientation orientation;
};
static inline void qphy_setbits(void __iomem *base, u32 offset, u32 val)
@@ -2485,6 +2493,7 @@ static int qcom_qmp_phy_com_init(struct qmp_phy *qphy)
void __iomem *pcs = qphy->pcs;
void __iomem *dp_com = qmp->dp_com;
int ret, i;
+ unsigned int val;
mutex_lock(>phy_mutex);
if (qmp->init_count++) {
@@ -2534,6 +2543,13 @@ static int qcom_qmp_phy_com_init(struct qmp_phy *qphy)
qphy_setbits(dp_com, QPHY_V3_DP_COM_PHY_MODE_CTRL,
USB3_MODE | DP_MODE);
+ if (cfg->is_dual_lane_phy) {
+ val = SW_PORTSELECT_MUX;
+ if (qmp->orientation == TYPEC_ORIENTATION_REVERSE)
+ val |= SW_PORTSELECT_VAL;
+ qphy_setbits(dp_com, QPHY_V3_DP_COM_TYPEC_CTRL, val);
+ }
+
/* bring both QMP USB and QMP DP PHYs PCS block out of reset */
qphy_clrbits(dp_com, QPHY_V3_DP_COM_RESET_OVRD_CTRL,
SW_DPPHY_RESET_MUX | SW_DPPHY_RESET |
@@ -2559,7 +2575,7 @@ static int qcom_qmp_phy_com_init(struct qmp_phy *qphy)
if (cfg->has_phy_com_ctrl) {
void __iomem *status;
- unsigned int mask, val;
+ unsigned int mask;
qphy_clrbits(serdes, cfg->regs[QPHY_COM_SW_RESET], SW_RESET);
qphy_setbits(serdes, cfg->regs[QPHY_COM_START_CONTROL],
@@ -3242,6 +3258,47 @@ static const struct dev_pm_ops qcom_qmp_phy_pm_ops = {
qcom_qmp_phy_runtime_resume, NULL)
};
+#if IS_ENABLED(CONFIG_PHY_QCOM_QMP_TYPEC)
+static int qcom_qmp_phy_typec_switch_set(struct typec_switch *sw,
+enum typec_orientation orientation)
+{
+ struct qcom_qmp *qmp = typec_switch_get_drvdata(sw);
+ struct qmp_phy *qphy = qmp->phys[0];
+
+ qmp->orientation = orientation;
+ if (qmp->phy_initialized) {
+ qcom_qmp_phy_disable(qphy->phy);
+ qcom_qmp_phy_enable(qphy->phy);
+ }
+
+ return 0;
+}
+
+static int qcom_qmp_phy_typec_switch_register(struct qcom_qmp *qmp)
+{
+ struct typec_switch_desc sw_desc;
+ struct device *dev = qmp-&
[PATCH v2 1/4] arm64: boot: dts: qcom: sm8150: Add nodes for PMIC based typec detection
Introduce required child nodes to enable the PMIC based USB type C driver.
This consits of connector and endpoint nodes to drivers, which manage the
type C mux and the USB role switch.
Signed-off-by: Wesley Cheng
---
arch/arm64/boot/dts/qcom/sm8150-mtp.dts | 40 -
1 file changed, 39 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/qcom/sm8150-mtp.dts
b/arch/arm64/boot/dts/qcom/sm8150-mtp.dts
index ba3b5b802954..06ad01dde080 100644
--- a/arch/arm64/boot/dts/qcom/sm8150-mtp.dts
+++ b/arch/arm64/boot/dts/qcom/sm8150-mtp.dts
@@ -413,6 +413,31 @@ _vbus {
status = "okay";
};
+_typec {
+ status = "okay";
+ connector {
+ compatible = "usb-c-connector";
+ power-role = "dual";
+ data-role = "dual";
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ port@1 {
+ reg = <1>;
+ usb3_data_ss: endpoint@0 {
+ remote-endpoint = <_ss_mux>;
+ };
+ };
+ };
+ };
+
+ port {
+ usb3_role: endpoint {
+ remote-endpoint = <_drd_switch>;
+ };
+ };
+};
+
_1_hsphy {
status = "okay";
vdda-pll-supply = <_usb_hs_core>;
@@ -424,12 +449,25 @@ _1_qmpphy {
status = "okay";
vdda-phy-supply = <_l3c_1p2>;
vdda-pll-supply = <_usb_ss_dp_core_1>;
+ orientation-switch;
+ port {
+ qmp_ss_mux: endpoint@0 {
+ remote-endpoint = <_data_ss>;
+ };
+ };
};
_1 {
status = "okay";
+ usb-role-switch;
+ port {
+ dwc3_drd_switch: endpoint@0 {
+ remote-endpoint = <_role>;
+ };
+ };
};
_1_dwc3 {
- dr_mode = "peripheral";
+ dr_mode = "otg";
+ usb-role-switch;
};
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v2 3/4] usb: dwc3: dwc3-qcom: Find USB connector and register role switch
If registering a USB typeC connector, the connector node may not be a child
of the DWC3 QCOM device. Utilize devcon graph search to lookup if any
remote endpoints contain the connector. If a connector is present, the
DWC3 QCOM will register a USB role switch to receive role change events, as
well as attain a reference to the DWC3 core role switch to pass the event
down.
Signed-off-by: Wesley Cheng
---
drivers/usb/dwc3/dwc3-qcom.c | 120 ++-
1 file changed, 118 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index e1e78e9824b1..340760ef0e01 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -19,6 +19,8 @@
#include
#include
#include
+#include
+#include
#include "core.h"
@@ -71,6 +73,9 @@ struct dwc3_qcom {
struct notifier_block vbus_nb;
struct notifier_block host_nb;
+ struct usb_role_switch *role_sw;
+ struct usb_role_switch *dwc3_drd_sw;
+
const struct dwc3_acpi_pdata *acpi_pdata;
enum usb_dr_modemode;
@@ -190,6 +195,73 @@ static int dwc3_qcom_register_extcon(struct dwc3_qcom
*qcom)
return 0;
}
+static int dwc3_qcom_usb_role_switch_set(struct usb_role_switch *sw,
+enum usb_role role)
+{
+ struct dwc3_qcom *qcom = usb_role_switch_get_drvdata(sw);
+ struct fwnode_handle *child;
+ bool enable = false;
+
+ if (!qcom->dwc3_drd_sw) {
+ child = device_get_next_child_node(qcom->dev, NULL);
+ if (child) {
+ qcom->dwc3_drd_sw =
usb_role_switch_find_by_fwnode(child);
+ fwnode_handle_put(child);
+ if (IS_ERR(qcom->dwc3_drd_sw)) {
+ qcom->dwc3_drd_sw = NULL;
+ return 0;
+ }
+ }
+ }
+
+ usb_role_switch_set_role(qcom->dwc3_drd_sw, role);
+
+ if (role == USB_ROLE_DEVICE)
+ enable = true;
+ else
+ enable = false;
+
+ qcom->mode = (role == USB_ROLE_HOST) ? USB_DR_MODE_HOST :
+ USB_DR_MODE_PERIPHERAL;
+ dwc3_qcom_vbus_overrride_enable(qcom, enable);
+ return 0;
+}
+
+static enum usb_role dwc3_qcom_usb_role_switch_get(struct usb_role_switch *sw)
+{
+ struct dwc3_qcom *qcom = usb_role_switch_get_drvdata(sw);
+ enum usb_role role;
+
+ switch (qcom->mode) {
+ case USB_DR_MODE_HOST:
+ role = USB_ROLE_HOST;
+ break;
+ case USB_DR_MODE_PERIPHERAL:
+ role = USB_ROLE_DEVICE;
+ break;
+ default:
+ role = USB_ROLE_DEVICE;
+ break;
+ }
+
+ return role;
+}
+
+static int dwc3_qcom_setup_role_switch(struct dwc3_qcom *qcom)
+{
+ struct usb_role_switch_desc dwc3_role_switch = {NULL};
+
+ dwc3_role_switch.fwnode = dev_fwnode(qcom->dev);
+ dwc3_role_switch.set = dwc3_qcom_usb_role_switch_set;
+ dwc3_role_switch.get = dwc3_qcom_usb_role_switch_get;
+ dwc3_role_switch.driver_data = qcom;
+ qcom->role_sw = usb_role_switch_register(qcom->dev, _role_switch);
+ if (IS_ERR(qcom->role_sw))
+ return PTR_ERR(qcom->role_sw);
+
+ return 0;
+}
+
static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
{
if (qcom->hs_phy_irq) {
@@ -540,6 +612,42 @@ static int dwc3_qcom_of_register_core(struct
platform_device *pdev)
return 0;
}
+static int dwc3_qcom_connector_check(struct fwnode_handle *fwnode)
+{
+ if (fwnode && (!fwnode_property_match_string(fwnode, "compatible",
+"gpio-usb-b-connector") ||
+ !fwnode_property_match_string(fwnode, "compatible",
+ "usb-c-connector")))
+ return 1;
+
+ return 0;
+}
+
+static void *dwc3_qcom_find_usb_connector_match(struct device_connection *con,
+ int ep, void *data)
+{
+ struct fwnode_handle *fwnode;
+
+ /* Check if the "connector" node is the parent of the remote endpoint */
+ if (dwc3_qcom_connector_check(con->fwnode))
+ return fwnode;
+
+ /* else, check if it is a child node */
+ fwnode = fwnode_get_named_child_node(con->fwnode, "connector");
+ if (dwc3_qcom_connector_check(fwnode))
+ return fwnode;
+
+ return 0;
+}
+
+static bool dwc3_qcom_find_usb_connector(struct platform_device *pdev)
+{
+ struct fwnode_handle *fwnode = pdev->dev.fwnode;
+
+ return fwnode_connection_find_match(fwnode, "connector", NULL,
+
[PATCH v10 2/4] dt-bindings: usb: Add Qualcomm PMIC type C controller dt-binding
Introduce the dt-binding for enabling USB type C orientation and role
detection using the PM8150B. The driver will be responsible for receiving
the interrupt at a state change on the CC lines, reading the
orientation/role, and communicating this information to the remote
clients, which can include a role switch node and a type C switch.
Signed-off-by: Wesley Cheng
---
.../bindings/usb/qcom,pmic-typec.yaml | 115 ++
1 file changed, 115 insertions(+)
create mode 100644 Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml
diff --git a/Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml
b/Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml
new file mode 100644
index ..40e0a296f922
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml
@@ -0,0 +1,115 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/usb/qcom,pmic-typec.yaml#;
+$schema: "http://devicetree.org/meta-schemas/core.yaml#;
+
+title: Qualcomm PMIC based USB type C Detection Driver
+
+maintainers:
+ - Wesley Cheng
+
+description: |
+ Qualcomm PMIC Type C Detect
+
+properties:
+ compatible:
+enum:
+ - qcom,pm8150b-usb-typec
+
+ reg:
+maxItems: 1
+description: Type C base address
+
+ interrupts:
+maxItems: 1
+description: CC change interrupt from PMIC
+
+ port:
+description: Remote endpoint connection to the DRD switch
+type: object
+
+properties:
+ endpoint:
+description: Connection to the DRD switch being used
+type: object
+
+ connector:
+$ref: /connector/usb-connector.yaml#
+description: Connector type for remote endpoints
+type: object
+
+properties:
+ compatible:
+enum:
+ - usb-c-connector
+
+ power-role: true
+ data-role: true
+
+ ports:
+description: Remote endpoint connections for type C paths
+type: object
+
+properties:
+ port@1:
+description: Remote endpoints for the Super Speed path
+type: object
+
+properties:
+ endpoint:
+description: Connection to USB type C mux node
+type: object
+
+required:
+ - compatible
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - connector
+ - port
+
+additionalProperties: false
+
+examples:
+ - |
+#include
+pm8150b {
+#address-cells = <1>;
+#size-cells = <0>;
+pm8150b_typec: usb-typec@1500 {
+compatible = "qcom,pm8150b-usb-typec";
+reg = <0x1500>;
+interrupts = <0x2 0x15 0x5 IRQ_TYPE_EDGE_RISING>;
+
+port {
+usb3_role: endpoint {
+remote-endpoint = <_drd_switch>;
+};
+};
+
+connector {
+compatible = "usb-c-connector";
+power-role = "dual";
+data-role = "dual";
+ports {
+#address-cells = <1>;
+#size-cells = <0>;
+port@0 {
+reg = <0>;
+};
+port@1 {
+reg = <1>;
+#address-cells = <1>;
+#size-cells = <0>;
+usb3_data_ss: endpoint {
+remote-endpoint = <_ss_mux>;
+};
+};
+};
+};
+};
+};
+...
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v10 1/4] usb: typec: Add QCOM PMIC typec detection driver
The QCOM SPMI typec driver handles the role and orientation detection, and
notifies client drivers using the USB role switch framework. It registers
as a typec port, so orientation can be communicated using the typec switch
APIs. The driver also attains a handle to the VBUS output regulator, so it
can enable/disable the VBUS source when acting as a host/device.
Signed-off-by: Wesley Cheng
Acked-by: Heikki Krogerus
Reviewed-by: Stephen Boyd
---
drivers/usb/typec/Kconfig | 12 ++
drivers/usb/typec/Makefile | 1 +
drivers/usb/typec/qcom-pmic-typec.c | 262
3 files changed, 275 insertions(+)
create mode 100644 drivers/usb/typec/qcom-pmic-typec.c
diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
index 559dd06117e7..63789cf88fce 100644
--- a/drivers/usb/typec/Kconfig
+++ b/drivers/usb/typec/Kconfig
@@ -73,6 +73,18 @@ config TYPEC_TPS6598X
If you choose to build this driver as a dynamically linked module, the
module will be called tps6598x.ko.
+config TYPEC_QCOM_PMIC
+ tristate "Qualcomm PMIC USB Type-C driver"
+ depends on ARCH_QCOM || COMPILE_TEST
+ help
+ Driver for supporting role switch over the Qualcomm PMIC. This will
+ handle the USB Type-C role and orientation detection reported by the
+ QCOM PMIC if the PMIC has the capability to handle USB Type-C
+ detection.
+
+ It will also enable the VBUS output to connected devices when a
+ DFP connection is made.
+
source "drivers/usb/typec/mux/Kconfig"
source "drivers/usb/typec/altmodes/Kconfig"
diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
index 7753a5c3cd46..cceffd987643 100644
--- a/drivers/usb/typec/Makefile
+++ b/drivers/usb/typec/Makefile
@@ -6,4 +6,5 @@ obj-$(CONFIG_TYPEC_TCPM)+= tcpm/
obj-$(CONFIG_TYPEC_UCSI) += ucsi/
obj-$(CONFIG_TYPEC_HD3SS3220) += hd3ss3220.o
obj-$(CONFIG_TYPEC_TPS6598X) += tps6598x.o
+obj-$(CONFIG_TYPEC_QCOM_PMIC) += qcom-pmic-typec.o
obj-$(CONFIG_TYPEC)+= mux/
diff --git a/drivers/usb/typec/qcom-pmic-typec.c
b/drivers/usb/typec/qcom-pmic-typec.c
new file mode 100644
index ..a0454a80c4a2
--- /dev/null
+++ b/drivers/usb/typec/qcom-pmic-typec.c
@@ -0,0 +1,262 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define TYPEC_MISC_STATUS 0xb
+#define CC_ATTACHEDBIT(0)
+#define CC_ORIENTATION BIT(1)
+#define SNK_SRC_MODE BIT(6)
+#define TYPEC_MODE_CFG 0x44
+#define TYPEC_DISABLE_CMD BIT(0)
+#define EN_SNK_ONLYBIT(1)
+#define EN_SRC_ONLYBIT(2)
+#define TYPEC_VCONN_CONTROL0x46
+#define VCONN_EN_SRC BIT(0)
+#define VCONN_EN_VAL BIT(1)
+#define TYPEC_EXIT_STATE_CFG 0x50
+#define SEL_SRC_UPPER_REF BIT(2)
+#define TYPEC_INTR_EN_CFG_10x5e
+#define TYPEC_INTR_EN_CFG_1_MASK GENMASK(7, 0)
+
+struct qcom_pmic_typec {
+ struct device *dev;
+ struct regmap *regmap;
+ u32 base;
+
+ struct typec_port *port;
+ struct usb_role_switch *role_sw;
+
+ struct regulator*vbus_reg;
+ boolvbus_enabled;
+};
+
+static void qcom_pmic_typec_enable_vbus_regulator(struct qcom_pmic_typec
+ *qcom_usb, bool enable)
+{
+ int ret;
+
+ if (enable == qcom_usb->vbus_enabled)
+ return;
+
+ if (enable) {
+ ret = regulator_enable(qcom_usb->vbus_reg);
+ if (ret)
+ return;
+ } else {
+ ret = regulator_disable(qcom_usb->vbus_reg);
+ if (ret)
+ return;
+ }
+ qcom_usb->vbus_enabled = enable;
+}
+
+static void qcom_pmic_typec_check_connection(struct qcom_pmic_typec *qcom_usb)
+{
+ enum typec_orientation orientation;
+ enum usb_role role;
+ unsigned int stat;
+ bool enable_vbus;
+
+ regmap_read(qcom_usb->regmap, qcom_usb->base + TYPEC_MISC_STATUS,
+ );
+
+ if (stat & CC_ATTACHED) {
+ orientation = (stat & CC_ORIENTATION) ?
+ TYPEC_ORIENTATION_REVERSE :
+ TYPEC_ORIENTATION_NORMAL;
+ typec_set_orientation(qcom_usb->port, orientation);
+
+ role = (stat & SNK_SRC_MODE) ? USB_ROLE_HOST : USB_ROLE_DEVICE;
+ if (role == USB_ROLE_HOST)
+ enable_vbus = true;
+ e
[PATCH v10 4/4] arm64: boot: dts: qcom: pm8150b: Add DTS node for PMIC VBUS booster
Add the required DTS node for the USB VBUS output regulator, which is
available on PM8150B. This will provide the VBUS source to connected
peripherals.
Signed-off-by: Wesley Cheng
---
arch/arm64/boot/dts/qcom/pm8150b.dtsi | 6 ++
arch/arm64/boot/dts/qcom/sm8150-mtp.dts | 4
2 files changed, 10 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/pm8150b.dtsi
b/arch/arm64/boot/dts/qcom/pm8150b.dtsi
index 2bf385f5a55a..49ea597cc0c5 100644
--- a/arch/arm64/boot/dts/qcom/pm8150b.dtsi
+++ b/arch/arm64/boot/dts/qcom/pm8150b.dtsi
@@ -53,6 +53,12 @@ power-on@800 {
status = "disabled";
};
+ pm8150b_vbus: regulator@1100 {
+ compatible = "qcom,pm8150b-vbus-reg";
+ status = "disabled";
+ reg = <0x1100>;
+ };
+
pm8150b_typec: usb-typec@1500 {
compatible = "qcom,pm8150b-usb-typec";
status = "disabled";
diff --git a/arch/arm64/boot/dts/qcom/sm8150-mtp.dts
b/arch/arm64/boot/dts/qcom/sm8150-mtp.dts
index 6c6325c3af59..ba3b5b802954 100644
--- a/arch/arm64/boot/dts/qcom/sm8150-mtp.dts
+++ b/arch/arm64/boot/dts/qcom/sm8150-mtp.dts
@@ -409,6 +409,10 @@ _mem_phy {
vdda-pll-max-microamp = <19000>;
};
+_vbus {
+ status = "okay";
+};
+
_1_hsphy {
status = "okay";
vdda-pll-supply = <_usb_hs_core>;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v10 0/4] Introduce PMIC based USB type C detection
Changes in v10: - Modified the type c dt-binding to remove the DRD switch node outside of the connector, as it is more of a SW entity, whereas the USB connector model focuses more on how the connector pins are connected in the HW design. The binding now matches what is specified in the usb-connector binding. - Change the fwnode to search for the remote endpoint referencing the usb role switch device in qcom-pmic-typec - Rename typec node from "typec" to "usb-typec" Changes in v9: - Fixed dt-binding to reference usb-connector from the 'connector' node, removed properties that didn't have further constraints (than specified in usb-connector.yaml), and make 'reg' a required property. - Moved vbus_reg get call into probe(), and will fail if the regulator is not available. - Removed some references from qcom_pmic_typec, as they were not needed after probe(). - Moved interrupt registration until after all used variables were initialized. Changes in v8: - Simplified some property definitions, and corrected the connector reference in the dt binding. Changes in v7: - Fixups in qcom-pmic-typec.c to remove uncesscary includes, printk formatting, and revising some logic operations. Changes in v6: - Removed qcom_usb_vbus-regulator.c and qcom,usb-vbus-regulator.yaml from the series as they have been merged on regulator.git - Added separate references to the usb-connector.yaml in qcom,pmic-typec.yaml instead of referencing the entire schema. Changes in v5: - Fix dt_binding_check warning/error in qcom,pmic-typec.yaml Changes in v4: - Modified qcom,pmic-typec binding to include the SS mux and the DRD remote endpoint nodes underneath port@1, which is assigned to the SSUSB path according to usb-connector - Added usb-connector reference to the typec dt-binding - Added tags to the usb type c and vbus nodes - Removed "qcom" tags from type c and vbus nodes - Modified Kconfig module name, and removed module alias from the typec driver Changes in v3: - Fix driver reference to match driver name in Kconfig for qcom_usb_vbus-regulator.c - Utilize regulator bitmap helpers for enable, disable and is enabled calls in qcom_usb_vbus-regulator.c - Use of_get_regulator_init_data() to initialize regulator init data, and to set constraints in qcom_usb_vbus-regulator.c - Remove the need for a local device structure in the vbus regulator driver Changes in v2: - Use devm_kzalloc() in qcom_pmic_typec_probe() - Add checks to make sure return value of typec_find_port_power_role() is valid - Added a VBUS output regulator driver, which will be used by the PMIC USB type c driver to enable/disable the source - Added logic to control vbus source from the PMIC type c driver when UFP/DFP is detected - Added dt-binding for this new regulator driver - Fixed Kconfig typec notation to match others - Leave type C block disabled until enabled by a platform DTS Add the required drivers for implementing type C orientation and role detection using the Qualcomm PMIC. Currently, PMICs such as the PM8150B have an integrated type C block, which can be utilized for this. This series adds the dt-binding, PMIC type C driver, and DTS nodes. The PMIC type C driver will register itself as a type C port w/ a registered type C switch for orientation, and will fetch a USB role switch handle for the role notifications. It will also have the ability to enable the VBUS output to any connected devices based on if the device is behaving as a UFP or DFP. Wesley Cheng (4): usb: typec: Add QCOM PMIC typec detection driver dt-bindings: usb: Add Qualcomm PMIC type C controller dt-binding arm64: boot: dts: qcom: pm8150b: Add node for USB type C block arm64: boot: dts: qcom: pm8150b: Add DTS node for PMIC VBUS booster .../bindings/usb/qcom,pmic-typec.yaml | 115 arch/arm64/boot/dts/qcom/pm8150b.dtsi | 13 + arch/arm64/boot/dts/qcom/sm8150-mtp.dts | 4 + drivers/usb/typec/Kconfig | 12 + drivers/usb/typec/Makefile| 1 + drivers/usb/typec/qcom-pmic-typec.c | 262 ++ 6 files changed, 407 insertions(+) create mode 100644 Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml create mode 100644 drivers/usb/typec/qcom-pmic-typec.c -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
[PATCH v10 3/4] arm64: boot: dts: qcom: pm8150b: Add node for USB type C block
The PM8150B has a dedicated USB type C block, which can be used for type C
orientation and role detection. Create the reference node to this type C
block for further use.
Signed-off-by: Wesley Cheng
Reviewed-by: Bjorn Andersson
---
arch/arm64/boot/dts/qcom/pm8150b.dtsi | 7 +++
1 file changed, 7 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/pm8150b.dtsi
b/arch/arm64/boot/dts/qcom/pm8150b.dtsi
index e112e8876db6..2bf385f5a55a 100644
--- a/arch/arm64/boot/dts/qcom/pm8150b.dtsi
+++ b/arch/arm64/boot/dts/qcom/pm8150b.dtsi
@@ -53,6 +53,13 @@ power-on@800 {
status = "disabled";
};
+ pm8150b_typec: usb-typec@1500 {
+ compatible = "qcom,pm8150b-usb-typec";
+ status = "disabled";
+ reg = <0x1500>;
+ interrupts = <0x2 0x15 0x5 IRQ_TYPE_EDGE_RISING>;
+ };
+
pm8150b_temp: temp-alarm@2400 {
compatible = "qcom,spmi-temp-alarm";
reg = <0x2400>;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v4] usb: dwc3: Stop active transfers before halting the controller
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
Reviewed-by: Thinh Nguyen
---
Changes in v4:
- Updated comments to reference DWC3 databook sections and added direct
quotes.
- Changed the stop active transfer EP loop to use dwc->num_eps.
- Moved to using dwc3_gadget_disable_irq/synchronize_irq instead of
enable_irq/disable_irq for ensuring the interrupt handler is not pending.
Changes in v3:
- Removed DWC3_EP_ENABLED check from dwc3_gadget_stop_active_transfers()
as dwc3_stop_active_transfer() has a check already in place.
- Calling __dwc3_gadget_stop() which ensures that DWC3 interrupt events
are cleared, and ep0 eps are cleared for the pullup disabled case. Not
required to call __dwc3_gadget_start() on pullup enable, as the
composite driver will execute udc_start() before calling pullup().
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 | 66 ++-
2 files changed, 66 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..5d879b7606d5 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,21 @@ 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 < dwc->num_eps; epnum++) {
+ struct dwc3_ep *dep;
+
+ dep = dwc->eps[epnum];
+ if (!dep)
+ continue;
+
+ dwc3_remove_requests(dwc, dep);
+ }
+}
+
static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
{
u32 reg;
@@ -1971,6 +1986,9 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int
is_on, int suspend)
return 0;
}
+static void dwc3_gadget_disable_irq(struct dwc3 *dwc);
+static void __dwc3_gadget_stop(struct dwc3 *dwc);
+
static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
{
struct dwc3 *dwc = gadget_to_dwc(g);
@@ -1994,7 +2012,46 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int
is_on)
}
}
+ /*
+* Synchronize any pending event handling before executing the
controller
+* halt routine.
+*/
+ if (!is_on) {
+ dwc3_gadget_disable_irq(dwc);
+ synchronize_irq(dwc->irq_gadget);
+ }
+
spin_lock_irqsave(>lock, flags);
+
+ if (!is_on) {
+ u32 count;
+
+ /*
+* In the Synopsis DesignWare Cores USB3 Databook Rev. 3.30a
+* Section 4.1.8 Table 4-7, it states that for a
device-initiated
+* disconnect, the SW needs to ensure that it sends "a
DEPENDXFER
+* comma
Re: [PATCH v3] usb: dwc3: Stop active transfers before halting the controller
On 9/24/2020 11:06 PM, Felipe Balbi wrote:
>
> Hi,
>
> Alan Stern writes:
>>>> Hence, the reason if there was already a pending IRQ triggered, the
>>>> dwc3_gadget_disable_irq() won't ensure the IRQ is handled. We can do
>>>> something like:
>>>> if (!is_on)
>>>>dwc3_gadget_disable_irq()
>>>> synchronize_irq()
>>>> spin_lock_irqsave()
>>>> if(!is_on) {
>>>> ...
>>>>
>>>> But the logic to only apply this on the pullup removal case is a little
>>>> messy. Also, from my understanding, the spin_lock_irqsave() will only
>>>> disable the local CPU IRQs, but not the interrupt line on the GIC, which
>>>> means other CPUs can handle it, unless we explicitly set the IRQ
>>>> affinity to CPUX.
>>>
>>> Yeah, the way I understand this can't really happen. But I'm open to
>>> being educated. Maybe Alan can explain if this is really possibility?
>>
Hi Felipe/Alan,
Thanks for the detailed explanations and inputs. Useful information to
have!
>> It depends on the details of the hardware, but yes, it is possible in
>> general for an interrupt handler to run after you have turned off the
>> device's interrupt-request line. For example:
>>
>> CPU A CPU B
>> --- --
>> Gets an IRQ from the device
>> Calls handler routine spin_lock_irq
>>spin_lock_irq Turns off the IRQ line
>>...spins... spin_unlock_irq
>>Rest of handler runs
>>spin_unlock_irq
>>
>> That's why we have synchronize_irq(). The usual pattern is something
>> like this:
>>
>> spin_lock_irq(>lock);
>> priv->disconnected = true;
>> my_disable_irq(priv);
>> spin_unlock_irq(>lock);
>> synchronize_irq(priv->irq);
>>
>> And of course this has to be done in a context that can sleep.
>>
>> Does this answer your question?
>
> It does, thank you Alan. It seems like we don't need a call to
> disable_irq(), only synchronize_irq() is enough, however it should be
> called with spinlocks released, not held.
>
I mean...I'm not against using the synchronize_irq() +
dwc3_gadget_disable_irq() route, since that will address the concern as
well. It was just with the disable/enable IRQ route, I didn't need to
explicitly check the is_on flag again, since I didn't need to worry
about overwriting the DEVTEN reg (for the pullup enable case). Will
include this on the next version.
Thanks
Wesley Cheng
> Thanks
>
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Re: [PATCH v3] usb: dwc3: Stop active transfers before halting the controller
On 9/6/2020 11:20 PM, Felipe Balbi wrote:
>
> Hi,
>
> Wesley Cheng writes:
>> 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) {
>
> this looks odd. If we don't have pullups connected, we shouldn't have a
> descriptor, likewise if we don't have a a description, we haven't been
> enumerated, therefore we shouldn't have pullups connected.
>
> What am I missing here?
>
Hi Felipe,
When we
echo "" > /sys/kernel/config/usb_gadget/g1/UDC
This triggers the usb_gadget_disconnect() routine to execute.
int usb_gadget_disconnect(struct usb_gadget *gadget)
{
...
ret = gadget->ops->pullup(gadget, 0);
if (!ret) {
gadget->connected = 0;
gadget->udc->driver->disconnect(gadget);
}
So it is possible that we've already disabled the pullup before running
the disable() callbacks in the function drivers. The disable()
callbacks usually are the ones responsible for calling usb_ep_disable(),
where we clear the desc field. This means there is a brief period where
the pullups_connected = 0, but we still have valid ep desc, as it has
not been disabled yet.
Also, for function drivers like mass storage, the fsg_disable() routine
defers the actual usb_ep_disable() call to the fsg_thread, so its not
always ensured that the disconnect() execution would result in the
usb_ep_disable() to occur synchronously.
>> @@ -1926,6 +1926,21 @@ 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++) {
>
> dwc3 knows the number of endpoints available in the HW. Use dwc->num_eps
> instead.
>
Sure, will do.
>> @@ -1971,6 +1986,8 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int
>> is_on, int suspend)
>> return 0;
>> }
>>
>> +static void __dwc3_gadget_stop(struct dwc3 *dwc);
>> +
>> static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>> {
>> struct dwc3 *dwc = gadget_to_dwc(g);
>> @@ -1994,9 +2011,37 @@ 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);
>
> why isn't dwc3_gadget_disable_irq() enough?
>
>> spin_lock_irqsave(>lock, flags);
>
> spin_lock_irqsave() will disable interrupts, why disable_irq() above?
>
In the discussion I had with Thinh, the concern was that with the newly
added code to override the lpos here, if the interrupt routine
(dwc3_check_event_buf()) runs, then it will reference the lpos for
copying the event buffer contents to the event cache, and potentially
process events. There is no locking in place, so it could be possible
to have both run in parallel.
Hence, the reason if there was already a pending IRQ triggered, the
dwc3_gadget_disable_irq() won't ensure the IRQ is handled. We can do
something like:
if (!is_on)
dwc3_gadget_disable_irq()
synchronize_irq()
spin_lock_irqsave()
if(!is_on) {
...
But the logic to only apply this on the pullup removal case is a little
messy. Also, from my understanding, the spin_lock_irqsave() will only
disable the local CPU IRQs, but not the interrupt line on the GIC, which
means other CPUs can handle it, unless we explicitly set the IRQ
affinity to CPUX.
>> +/* Controller is not halted until pending events are acknowledged */
>> +if (!is_on) {
>> +u32 count;
>> +
>> +/*
>> + * The databook explicitly mentions for a device-initiated
>> + * disconnect sequence, the SW needs to ensure that it ends any
>> + * active transfers.
>> + */
>
> make this a little better by mentioning the version and section of the
> databook you're reading. That makes it easier for future
> reference. Also, use an actual quote from the databook, along the lines
> of:
>
> /*
> * Synopsys DesignWare Cores USB3 Da
[PATCH v9 2/4] dt-bindings: usb: Add Qualcomm PMIC type C controller dt-binding
Introduce the dt-binding for enabling USB type C orientation and role
detection using the PM8150B. The driver will be responsible for receiving
the interrupt at a state change on the CC lines, reading the
orientation/role, and communicating this information to the remote
clients, which can include a role switch node and a type C switch.
Signed-off-by: Wesley Cheng
---
.../bindings/usb/qcom,pmic-typec.yaml | 108 ++
1 file changed, 108 insertions(+)
create mode 100644 Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml
diff --git a/Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml
b/Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml
new file mode 100644
index ..8582ab6a3cc4
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml
@@ -0,0 +1,108 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/usb/qcom,pmic-typec.yaml#;
+$schema: "http://devicetree.org/meta-schemas/core.yaml#;
+
+title: Qualcomm PMIC based USB type C Detection Driver
+
+maintainers:
+ - Wesley Cheng
+
+description: |
+ Qualcomm PMIC Type C Detect
+
+properties:
+ compatible:
+enum:
+ - qcom,pm8150b-usb-typec
+
+ reg:
+maxItems: 1
+description: Type C base address
+
+ interrupts:
+maxItems: 1
+description: CC change interrupt from PMIC
+
+ connector:
+$ref: /connector/usb-connector.yaml#
+description: Connector type for remote endpoints
+type: object
+
+properties:
+ compatible:
+enum:
+ - usb-c-connector
+
+ power-role: true
+ data-role: true
+
+ ports:
+description: Remote endpoint connections
+type: object
+
+properties:
+ port@1:
+description: Remote endpoints for the Super Speed path
+type: object
+
+properties:
+ endpoint@0:
+description: Connection to USB type C mux node
+type: object
+
+ endpoint@1:
+description: Connection to role switch node
+type: object
+
+required:
+ - compatible
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - connector
+
+additionalProperties: false
+
+examples:
+ - |
+#include
+pm8150b {
+#address-cells = <1>;
+#size-cells = <0>;
+pm8150b_typec: typec@1500 {
+compatible = "qcom,pm8150b-usb-typec";
+reg = <0x1500>;
+interrupts = <0x2 0x15 0x5 IRQ_TYPE_EDGE_RISING>;
+
+connector {
+compatible = "usb-c-connector";
+power-role = "dual";
+data-role = "dual";
+ports {
+#address-cells = <1>;
+#size-cells = <0>;
+port@0 {
+reg = <0>;
+};
+port@1 {
+reg = <1>;
+#address-cells = <1>;
+#size-cells = <0>;
+usb3_data_ss: endpoint@0 {
+reg = <0>;
+remote-endpoint = <_ss_mux>;
+};
+usb3_role: endpoint@1 {
+reg = <1>;
+remote-endpoint = <_drd_switch>;
+};
+};
+};
+};
+};
+};
+...
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v9 4/4] arm64: boot: dts: qcom: pm8150b: Add DTS node for PMIC VBUS booster
Add the required DTS node for the USB VBUS output regulator, which is
available on PM8150B. This will provide the VBUS source to connected
peripherals.
Signed-off-by: Wesley Cheng
---
arch/arm64/boot/dts/qcom/pm8150b.dtsi | 6 ++
arch/arm64/boot/dts/qcom/sm8150-mtp.dts | 4
2 files changed, 10 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/pm8150b.dtsi
b/arch/arm64/boot/dts/qcom/pm8150b.dtsi
index 053c659734a7..b49caa63cd4c 100644
--- a/arch/arm64/boot/dts/qcom/pm8150b.dtsi
+++ b/arch/arm64/boot/dts/qcom/pm8150b.dtsi
@@ -53,6 +53,12 @@ power-on@800 {
status = "disabled";
};
+ pm8150b_vbus: regulator@1100 {
+ compatible = "qcom,pm8150b-vbus-reg";
+ status = "disabled";
+ reg = <0x1100>;
+ };
+
pm8150b_typec: typec@1500 {
compatible = "qcom,pm8150b-usb-typec";
status = "disabled";
diff --git a/arch/arm64/boot/dts/qcom/sm8150-mtp.dts
b/arch/arm64/boot/dts/qcom/sm8150-mtp.dts
index 6c6325c3af59..ba3b5b802954 100644
--- a/arch/arm64/boot/dts/qcom/sm8150-mtp.dts
+++ b/arch/arm64/boot/dts/qcom/sm8150-mtp.dts
@@ -409,6 +409,10 @@ _mem_phy {
vdda-pll-max-microamp = <19000>;
};
+_vbus {
+ status = "okay";
+};
+
_1_hsphy {
status = "okay";
vdda-pll-supply = <_usb_hs_core>;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v9 0/4] Introduce PMIC based USB type C detection
Changes in v9: - Fixed dt-binding to reference usb-connector from the 'connector' node, removed properties that didn't have further constraints (than specified in usb-connector.yaml), and make 'reg' a required property. - Moved vbus_reg get call into probe(), and will fail if the regulator is not available. - Removed some references from qcom_pmic_typec, as they were not needed after probe(). - Moved interrupt registration until after all used variables were initialized. Changes in v8: - Simplified some property definitions, and corrected the connector reference in the dt binding. Changes in v7: - Fixups in qcom-pmic-typec.c to remove uncesscary includes, printk formatting, and revising some logic operations. Changes in v6: - Removed qcom_usb_vbus-regulator.c and qcom,usb-vbus-regulator.yaml from the series as they have been merged on regulator.git - Added separate references to the usb-connector.yaml in qcom,pmic-typec.yaml instead of referencing the entire schema. Changes in v5: - Fix dt_binding_check warning/error in qcom,pmic-typec.yaml Changes in v4: - Modified qcom,pmic-typec binding to include the SS mux and the DRD remote endpoint nodes underneath port@1, which is assigned to the SSUSB path according to usb-connector - Added usb-connector reference to the typec dt-binding - Added tags to the usb type c and vbus nodes - Removed "qcom" tags from type c and vbus nodes - Modified Kconfig module name, and removed module alias from the typec driver Changes in v3: - Fix driver reference to match driver name in Kconfig for qcom_usb_vbus-regulator.c - Utilize regulator bitmap helpers for enable, disable and is enabled calls in qcom_usb_vbus-regulator.c - Use of_get_regulator_init_data() to initialize regulator init data, and to set constraints in qcom_usb_vbus-regulator.c - Remove the need for a local device structure in the vbus regulator driver Changes in v2: - Use devm_kzalloc() in qcom_pmic_typec_probe() - Add checks to make sure return value of typec_find_port_power_role() is valid - Added a VBUS output regulator driver, which will be used by the PMIC USB type c driver to enable/disable the source - Added logic to control vbus source from the PMIC type c driver when UFP/DFP is detected - Added dt-binding for this new regulator driver - Fixed Kconfig typec notation to match others - Leave type C block disabled until enabled by a platform DTS Wesley Cheng (4): usb: typec: Add QCOM PMIC typec detection driver dt-bindings: usb: Add Qualcomm PMIC type C controller dt-binding arm64: boot: dts: qcom: pm8150b: Add node for USB type C block arm64: boot: dts: qcom: pm8150b: Add DTS node for PMIC VBUS booster .../bindings/usb/qcom,pmic-typec.yaml | 108 arch/arm64/boot/dts/qcom/pm8150b.dtsi | 13 + arch/arm64/boot/dts/qcom/sm8150-mtp.dts | 4 + drivers/usb/typec/Kconfig | 12 + drivers/usb/typec/Makefile| 1 + drivers/usb/typec/qcom-pmic-typec.c | 262 ++ 6 files changed, 400 insertions(+) create mode 100644 Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml create mode 100644 drivers/usb/typec/qcom-pmic-typec.c -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
[PATCH v9 3/4] arm64: boot: dts: qcom: pm8150b: Add node for USB type C block
The PM8150B has a dedicated USB type C block, which can be used for type C
orientation and role detection. Create the reference node to this type C
block for further use.
Signed-off-by: Wesley Cheng
Reviewed-by: Bjorn Andersson
---
arch/arm64/boot/dts/qcom/pm8150b.dtsi | 7 +++
1 file changed, 7 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/pm8150b.dtsi
b/arch/arm64/boot/dts/qcom/pm8150b.dtsi
index e112e8876db6..053c659734a7 100644
--- a/arch/arm64/boot/dts/qcom/pm8150b.dtsi
+++ b/arch/arm64/boot/dts/qcom/pm8150b.dtsi
@@ -53,6 +53,13 @@ power-on@800 {
status = "disabled";
};
+ pm8150b_typec: typec@1500 {
+ compatible = "qcom,pm8150b-usb-typec";
+ status = "disabled";
+ reg = <0x1500>;
+ interrupts = <0x2 0x15 0x5 IRQ_TYPE_EDGE_RISING>;
+ };
+
pm8150b_temp: temp-alarm@2400 {
compatible = "qcom,spmi-temp-alarm";
reg = <0x2400>;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v9 1/4] usb: typec: Add QCOM PMIC typec detection driver
The QCOM SPMI typec driver handles the role and orientation detection, and
notifies client drivers using the USB role switch framework. It registers
as a typec port, so orientation can be communicated using the typec switch
APIs. The driver also attains a handle to the VBUS output regulator, so it
can enable/disable the VBUS source when acting as a host/device.
Signed-off-by: Wesley Cheng
Acked-by: Heikki Krogerus
Reviewed-by: Stephen Boyd
---
drivers/usb/typec/Kconfig | 12 ++
drivers/usb/typec/Makefile | 1 +
drivers/usb/typec/qcom-pmic-typec.c | 262
3 files changed, 275 insertions(+)
create mode 100644 drivers/usb/typec/qcom-pmic-typec.c
diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
index 559dd06117e7..63789cf88fce 100644
--- a/drivers/usb/typec/Kconfig
+++ b/drivers/usb/typec/Kconfig
@@ -73,6 +73,18 @@ config TYPEC_TPS6598X
If you choose to build this driver as a dynamically linked module, the
module will be called tps6598x.ko.
+config TYPEC_QCOM_PMIC
+ tristate "Qualcomm PMIC USB Type-C driver"
+ depends on ARCH_QCOM || COMPILE_TEST
+ help
+ Driver for supporting role switch over the Qualcomm PMIC. This will
+ handle the USB Type-C role and orientation detection reported by the
+ QCOM PMIC if the PMIC has the capability to handle USB Type-C
+ detection.
+
+ It will also enable the VBUS output to connected devices when a
+ DFP connection is made.
+
source "drivers/usb/typec/mux/Kconfig"
source "drivers/usb/typec/altmodes/Kconfig"
diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
index 7753a5c3cd46..cceffd987643 100644
--- a/drivers/usb/typec/Makefile
+++ b/drivers/usb/typec/Makefile
@@ -6,4 +6,5 @@ obj-$(CONFIG_TYPEC_TCPM)+= tcpm/
obj-$(CONFIG_TYPEC_UCSI) += ucsi/
obj-$(CONFIG_TYPEC_HD3SS3220) += hd3ss3220.o
obj-$(CONFIG_TYPEC_TPS6598X) += tps6598x.o
+obj-$(CONFIG_TYPEC_QCOM_PMIC) += qcom-pmic-typec.o
obj-$(CONFIG_TYPEC)+= mux/
diff --git a/drivers/usb/typec/qcom-pmic-typec.c
b/drivers/usb/typec/qcom-pmic-typec.c
new file mode 100644
index ..9fc5e69d6e82
--- /dev/null
+++ b/drivers/usb/typec/qcom-pmic-typec.c
@@ -0,0 +1,262 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define TYPEC_MISC_STATUS 0xb
+#define CC_ATTACHEDBIT(0)
+#define CC_ORIENTATION BIT(1)
+#define SNK_SRC_MODE BIT(6)
+#define TYPEC_MODE_CFG 0x44
+#define TYPEC_DISABLE_CMD BIT(0)
+#define EN_SNK_ONLYBIT(1)
+#define EN_SRC_ONLYBIT(2)
+#define TYPEC_VCONN_CONTROL0x46
+#define VCONN_EN_SRC BIT(0)
+#define VCONN_EN_VAL BIT(1)
+#define TYPEC_EXIT_STATE_CFG 0x50
+#define SEL_SRC_UPPER_REF BIT(2)
+#define TYPEC_INTR_EN_CFG_10x5e
+#define TYPEC_INTR_EN_CFG_1_MASK GENMASK(7, 0)
+
+struct qcom_pmic_typec {
+ struct device *dev;
+ struct regmap *regmap;
+ u32 base;
+
+ struct typec_port *port;
+ struct usb_role_switch *role_sw;
+
+ struct regulator*vbus_reg;
+ boolvbus_enabled;
+};
+
+static void qcom_pmic_typec_enable_vbus_regulator(struct qcom_pmic_typec
+ *qcom_usb, bool enable)
+{
+ int ret;
+
+ if (enable == qcom_usb->vbus_enabled)
+ return;
+
+ if (enable) {
+ ret = regulator_enable(qcom_usb->vbus_reg);
+ if (ret)
+ return;
+ } else {
+ ret = regulator_disable(qcom_usb->vbus_reg);
+ if (ret)
+ return;
+ }
+ qcom_usb->vbus_enabled = enable;
+}
+
+static void qcom_pmic_typec_check_connection(struct qcom_pmic_typec *qcom_usb)
+{
+ enum typec_orientation orientation;
+ enum usb_role role;
+ unsigned int stat;
+ bool enable_vbus;
+
+ regmap_read(qcom_usb->regmap, qcom_usb->base + TYPEC_MISC_STATUS,
+ );
+
+ if (stat & CC_ATTACHED) {
+ orientation = (stat & CC_ORIENTATION) ?
+ TYPEC_ORIENTATION_REVERSE :
+ TYPEC_ORIENTATION_NORMAL;
+ typec_set_orientation(qcom_usb->port, orientation);
+
+ role = (stat & SNK_SRC_MODE) ? USB_ROLE_HOST : USB_ROLE_DEVICE;
+ if (role == USB_ROLE_HOST)
+ enable_vbus = true;
+ e
Re: [PATCH v8 4/4] arm64: boot: dts: qcom: pm8150b: Add DTS node for PMIC VBUS booster
On 8/30/2020 10:52 AM, Bjorn Andersson wrote:
> On Thu 20 Aug 07:47 UTC 2020, Wesley Cheng wrote:
>
>>
>>
>> On 8/12/2020 2:34 AM, Sergei Shtylyov wrote:
>>> Hello!
>>>
>>> On 12.08.2020 10:19, Wesley Cheng wrote:
>>>
>>>> Add the required DTS node for the USB VBUS output regulator, which is
>>>> available on PM8150B. This will provide the VBUS source to connected
>>>> peripherals.
>>>>
>>>> Signed-off-by: Wesley Cheng
>>>> ---
>>>> arch/arm64/boot/dts/qcom/pm8150b.dtsi | 6 ++
>>>> arch/arm64/boot/dts/qcom/sm8150-mtp.dts | 4
>>>> 2 files changed, 10 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/pm8150b.dtsi
>>>> b/arch/arm64/boot/dts/qcom/pm8150b.dtsi
>>>> index 053c659734a7..9e560c1ca30d 100644
>>>> --- a/arch/arm64/boot/dts/qcom/pm8150b.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/pm8150b.dtsi
>>>> @@ -53,6 +53,12 @@ power-on@800 {
>>>> status = "disabled";
>>>> };
>>>> + pm8150b_vbus: dcdc@1100 {
>>>
>>> s/dcdc/regulator/? What is "dcdc", anyway?
>>> The device nodes must have the generic names, according to the DT spec.
>>>
>>
>> Hi Sergei,
>>
>> Thanks for the comment!
>>
>> DCDC is the label that we use for the DC to DC converter block, since
>> the VBUS booster will output 5V to the connected devices. Would it make
>> more sense to have "dc-dc?"
>>
>
> At this level it's just a regulator at 0x1100, so it should be
> "regulator@1100". If you would like a more useful name in the running
> system you should be able to use the "regulator-name" property.
>
> Regards,
> Bjorn
>
Hi Bjorn,
Thanks for the suggestion. Sounds good, I will just use the "regulator"
name for now.
Thanks
Wesley
>> Thanks
>> Wesley
>>
>>>> + compatible = "qcom,pm8150b-vbus-reg";
>>>> + status = "disabled";
>>>> + reg = <0x1100>;
>>>> + };
>>>> +
>>>> pm8150b_typec: typec@1500 {
>>>> compatible = "qcom,pm8150b-usb-typec";
>>>> status = "disabled";
>>> [...]
>>>
>>> MBR, Sergei
>>
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
