Re: [RESEND x2][PATCH 0/3] dwc2 fixes for edge cases on hikey
On 10/12/2017 10:06 PM, John Stultz wrote: > On Thu, Oct 12, 2017 at 12:59 AM, Minas Harutyunyan > wrote: >> >> 1. Vardan's patch fixing issue when dwc2 switched from host to device >> mode. It's allow to make functional device after reconnecting without >> tracking UDC state. > > While I'm sure Vardan's patch is useful, I worry that its resolving a > different issue then what I'm trying to address, as it doesn't seem to > help the problems I'm seeing. > >> 2. I suppose that your patch "[RESEND x2][PATCH 1/3] usb: dwc2: Improve >> gadget state disconnection handling" not a good way to set correct UDC >> state. You added calling device mode functions dwc2_hsotg_disconnect() >> and dwc2_hsotg_core_init_disconnected() while core in Host mode and as >> result additional unwanted "mode mismatch" interrupts will be asserted. > > Apologies, I'm not sure I'm understanding you here. Forgive me if I'm > misinterpreting your feedback. > > So, the "usb: dwc2: Improve gadget state disconnection handling" isn't > itself doing the UDC state handling. > > Personally I see it as improving a previously applied fix > (dad3f793f20f - usb: dwc2: Make sure we disconnect the gadget state). > So instead of calling dwc2_hsotg_disconnect() in > dwc2_conn_id_status_change() when transitioning INTO device/B mode, > which was added due to earlier problems with state tracking (as when > we unplug the gadget cable, nothing else triggers the hsotg_disconnect > code), I'm instead suggesting we call dwc2_hsotg_disconnect() when we > transition into host/A mode. > > This only allows us to do proper UDC state handling later, since we > properly run the disconnect code for device when we switch into host > mode. > > If I'm understanding you, you seem to be objecting to this, as calling > dwc2_hsotg_disconnect() while we are transitioning to host mode can > cause "mode mismatch" interrupts. I've not seen this in practice with > this patch, but you know the logic better and it could be possible. > > Now, I'm of course open to other approaches, but it seems that we need > *something* to call dwc2_hsotg_disconnect() when the otg cable is > removed (which currently just doesn't happen). The earlier patch > calling dwc2_hsotg_disconnect() when we are entering device/B mode > avoids the state tracking warnings but, doesn't seem correct (nor does > it allow for things like proper UDC state handling). > >> 3. Function dwc2_conn_id_status_change() called when connector ID status >> changed. This interrupt asserted only when A-plug connected or >> disconnected. Connecting/disconnecting B-plug doesn't assert this interrupt. > > Ok. What I'm seeing may be somewhat hardware specific then, as on > HiKey, we have a switch that enables a on-board USB hub when the OTG > plug is removed. This may be the root of the issue, but I guess I'm > at a loss for how things should be handled here. > > When the b-plug is disconnected, we need to do something to signal to > the core that we aren't connected, no? > > And it seems that your point that the conn_id_status_change logic only > runs on the A-plug connect/disconnect mirrors the usage for the B plug > (ie: if A is disconnected, we enter B mode, thus if A is connected, > shouldn't we disable/disconnect B mode?). I suspect there's something > more subtle to your statement though, so if you could expand a bit so > I could better understand I'd appreciate it. > > thanks > -john > Hi John Stultz, On b-plug disconnect should asserted GOTGINT.SesEndDet interrupt. According previously sent by you register dump (GHWCFG2 = 0x23affc70) your core OTG_MODE=0. Bellow fragment from programming guide on Device disconnect: "7.3Device Disconnection The device session ends when the USB cable is disconnected or if the VBUS is switched off by the Host. The device disconnect flow varies depending on the value of the OTG_MODE configuration parameter. When OTG_MODE = 0,1, or 3 When OTG_MODE is set to 0,1, or 3, the device disconnect flow is as follows: 1. When the USB cable is unplugged or when the VBUS is switched off by the Host, the Device core trigger GINTSTS.OTGInt [bit 2] interrupt bit. 2. When the device application detects GINTSTS.OTGInt interrupt, it checks that the GOTGINT.SesEndDet (Session End Detected) bit is set to 1’b1." So, you should receive and handle "Session End Detected". In function dwc2_handle_otg_intr() on this interrupt (in device mode) calling dwc2_hsotg_disconnect() function. By adding your patch "[PATCH 3/3] usb: dwc2: Fix UDC state tracking" state changed to not attached as required. Thanks, Minas
Re: [RESEND x2][PATCH 0/3] dwc2 fixes for edge cases on hikey
On 10/17/2017 1:34 AM, John Stultz wrote: > On Mon, Oct 16, 2017 at 1:36 AM, Minas Harutyunyan > wrote: >> On b-plug disconnect should asserted GOTGINT.SesEndDet interrupt. >> According previously sent by you register dump (GHWCFG2 = 0x23affc70) >> your core OTG_MODE=0. >> Bellow fragment from programming guide on Device disconnect: >> >> "7.3Device Disconnection >> The device session ends when the USB cable is disconnected or if the >> VBUS is switched off by the Host. The >> device disconnect flow varies depending on the value of the OTG_MODE >> configuration parameter. >> >> When OTG_MODE = 0,1, or 3 >> When OTG_MODE is set to 0,1, or 3, the device disconnect flow is as follows: >> 1. When the USB cable is unplugged or when the VBUS is switched off by >> the Host, the Device core >> trigger GINTSTS.OTGInt [bit 2] interrupt bit. >> 2. When the device application detects GINTSTS.OTGInt interrupt, it >> checks that the >> GOTGINT.SesEndDet (Session End Detected) bit is set to 1’b1." >> >> So, you should receive and handle "Session End Detected". In function >> dwc2_handle_otg_intr() on this interrupt (in device mode) calling >> dwc2_hsotg_disconnect() function. By adding your patch "[PATCH 3/3] usb: >> dwc2: Fix UDC state tracking" state changed to not attached as required. > > > So, on the HiKey board (using 4.14-rc5 + Vardan's patch), I'm not > seeing the GOTGINT_SES_END_DET in dwc2_handle_otg_intr() when I remove > the USB OTG cable. > > In fact, I'm not seeing any calls to dwc2_handle_otg_intr()... which > seems... odd maybe? Any clues as to what might be going wrong then? > > thanks > -john > Hi John Stultz, So, on Hikey board on unplug B connector GOTGINT.SesEndDet interrupt not asserted, instead asserted GINTSTS_CONIDSTSCHNG. Please, confirm. In this case without your patch "[PATCH 1/3] usb: dwc2: Improve gadget state disconnection handling" but by applying your patch "[PATCH 3/3] usb: dwc2: Fix UDC state tracking": 1. On B plug connect UDC state will be set to "configured" 2. On B plug disconnect - "not attached". Is it Ok for you? Meantime, I'll check with HW team why GOTGINT.SesEndDet interrupt not asserted on unplug B connector. Thanks, Minas
Re: [PATCH 1/3 v2] usb: dwc2: Improve gadget state disconnection handling
On 10/24/2017 1:33 AM, John Stultz wrote:
> In the earlier commit dad3f793f20f ("usb: dwc2: Make sure we
> disconnect the gadget state"), I was trying to fix up the
> fact that we somehow weren't disconnecting the gadget state,
> so that when the OTG port was plugged in the second time we
> would get warnings about the state tracking being wrong.
>
> (This seems to be due to a quirk of the HiKey board where
> we do not ever get any otg interrupts, particularly the session
> end detected signal. Instead we only see status change
> interrupt.)
>
> The fix there was somewhat simple, as it just made sure to
> call dwc2_hsotg_disconnect() before we connected things up
> in OTG mode, ensuring the state handling didn't throw errors.
>
> But in looking at a different issue I was seeing with UDC
> state handling, I realized that it would be much better
> to call dwc2_hsotg_disconnect when we get the state change
> signal moving to host mode.
>
> Thus, this patch removes the earlier disconnect call I added
> and moves it (and the needed locking) to the host mode
> transition.
>
> Cc: Wei Xu
> Cc: Guodong Xu
> Cc: Amit Pundir
> Cc: YongQin Liu
> Cc: John Youn
> Cc: Minas Harutyunyan
> Cc: Douglas Anderson
> Cc: Chen Yu
> Cc: Felipe Balbi
> Cc: Greg Kroah-Hartman
> Cc: linux-...@vger.kernel.org
> Signed-off-by: John Stultz
> ---
> v2: Remove the extra dwc2_hsotg_core_init_disconnected() call
> I had added, as suggested by Minas.
> ---
> drivers/usb/dwc2/hcd.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index c263114..9bd60ec 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -3277,7 +3277,6 @@ static void dwc2_conn_id_status_change(struct
> work_struct *work)
> dwc2_core_init(hsotg, false);
> dwc2_enable_global_interrupts(hsotg);
> spin_lock_irqsave(&hsotg->lock, flags);
> - dwc2_hsotg_disconnect(hsotg);
> dwc2_hsotg_core_init_disconnected(hsotg, false);
> spin_unlock_irqrestore(&hsotg->lock, flags);
> dwc2_hsotg_core_connect(hsotg);
> @@ -3296,8 +3295,12 @@ static void dwc2_conn_id_status_change(struct
> work_struct *work)
> if (count > 250)
> dev_err(hsotg->dev,
> "Connection id status change timed out\n");
> - hsotg->op_state = OTG_STATE_A_HOST;
>
> + spin_lock_irqsave(&hsotg->lock, flags);
> + dwc2_hsotg_disconnect(hsotg);
> + spin_unlock_irqrestore(&hsotg->lock, flags);
> +
> + hsotg->op_state = OTG_STATE_A_HOST;
> /* Initialize the Core for Host mode */
> dwc2_core_init(hsotg, false);
> dwc2_enable_global_interrupts(hsotg);
>
This patch is required for the HiKey platform, because the assertion of
the "Connector ID status change" interrupt is different: asserting on B
connector unplug and goes to Host mode.
No any side effect on SNPS HAPS-DX platform where ConnIDStsChng
assertion is correct.
Acked-by: Minas Harutyunyan
Tested-by: Minas Harutyunyan
Thanks,
Minas
Re: [RESEND x2][PATCH 0/3] dwc2 fixes for edge cases on hikey
On 10/24/2017 12:41 AM, John Stultz wrote: > On Mon, Oct 23, 2017 at 2:19 AM, Minas Harutyunyan > wrote: >> Could you please verify on your setup follow patches: >> 1. Vardan's patch. >> 2. Patch for TOUTCAL&USBTRDTIM programming (new version see below). >> 4. Your patch 2/3 to avoid "Mode Mismatch" interrupts. >> 5. Your patch 3/3 to set udc state to "not attached". >> 6. Your patch 1/3, but remove dwc2_hsotg_core_init_disconnected() >> function call from Host starting brnch, keep *only* >> dwc2_hsotg_disconnect() to change UDC state to "not attached". > > So yes, this set does seem to work ok for me. Though neither Vardan's > patch or the TOUTCAL/USBTRDTIM patch seem to have much effect either > way (I need to do more testing just to be sure, but for the use cases > I've had trouble with they don't seem to do much). > > I'm happy to rework my earlier patch #1/3 to remove > dwc2_hsotg_core_init_disconnected() and resend. > > thanks > -john > Hi John Stultz, 1. Vardan's patch required for cases when core switching from host mode to device mode. On host disconnect hsotg->lx_state set to DWC2_L2 as result dwc2_hsotg_enqueue_setup() failed because enqueuing should be done in DWC2_L0 state. This patch set lx_state to DWC2_L0 before enqueuing setup transfer. 2. TOUTCAL&USBTRDTIM patch adding missing USBCFG programming in host mode. These fields even if was programmed in device mode (see function dwc2_hsotg_core_init_disconnected()) will be resetting to POR values after core soft reset applied. Programming of these fields to correct values allow fix issues with lot of transaction errors due to timeouts and turnarrounds on USB bus. Previously on TOUTCAL patch you wrote: "So while using this patch and the earlier one from Vardan, I don't see the "Transaction Error" and " ChHltd set, but reason is unknown" messages". Thanks, Minas
[PATCH] usb: dwc2: host: Setting TOUTCAL and USBTRDTIM fields in host mode
Added missing GUSBCFG programming in host mode.
These fields even if was programmed in device mode (in function
dwc2_hsotg_core_init_disconnected()) will be resetting to POR values
after core soft reset applied.
So, each time when switching to host mode required to set these fields
to correct values. It's allow fix issues with lot of transaction errors
due to timeouts and turnarrounds on USB bus.
Signed-off-by: Minas Harutyunyan
---
drivers/usb/dwc2/hcd.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index f4ef159b538e..1f65464055b9 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -2311,10 +2311,17 @@ static int dwc2_core_init(struct dwc2_hsotg *hsotg,
bool initial_setup)
*/
static void dwc2_core_host_init(struct dwc2_hsotg *hsotg)
{
- u32 hcfg, hfir, otgctl;
+ u32 hcfg, hfir, otgctl, usbcfg, val;
dev_dbg(hsotg->dev, "%s(%p)\n", __func__, hsotg);
+ /* Set HS/FS Timeout Calibration and USBTrdTim */
+ usbcfg = dwc2_readl(hsotg->regs + GUSBCFG);
+ usbcfg &= ~(GUSBCFG_TOUTCAL_MASK | GUSBCFG_USBTRDTIM_MASK);
+ val = (hsotg->phyif == GUSBCFG_PHYIF8) ? 9 : 5;
+ usbcfg |= (GUSBCFG_TOUTCAL(7) | (val << GUSBCFG_USBTRDTIM_SHIFT));
+ dwc2_writel(usbcfg, hsotg->regs + GUSBCFG);
+
/* Restart the Phy Clock */
dwc2_writel(0, hsotg->regs + PCGCTL);
--
2.11.0
Re: [PATCH] usb: dwc2: host: fix isoc urb actual length
Hi, On 11/6/2017 12:46 PM, William Wu wrote: > The actual_length in dwc2_hcd_urb structure is used > to indicate the total data length transferred so far, > but in dwc2_update_isoc_urb_state(), it just updates > the actual_length of isoc frame, and don't update the > urb actual_length at the same time, this will cause > device drivers working error which depend on the urb > actual_length. > > we can easily find this issue if use an USB camera， > the userspace use libusb to get USB data from kernel > via devio driver.In usb devio driver, processcompl() > function will process urb complete and copy data to > userspace depending on urb actual_length. > > Let's update the urb actual_length if the isoc frame > is valid. > > Signed-off-by: William Wu > --- > drivers/usb/dwc2/hcd_intr.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c > index 28a8210..01b1e13 100644 > --- a/drivers/usb/dwc2/hcd_intr.c > +++ b/drivers/usb/dwc2/hcd_intr.c > @@ -580,6 +580,7 @@ static enum dwc2_halt_status dwc2_update_isoc_urb_state( > frame_desc->status = 0; > frame_desc->actual_length = dwc2_get_actual_xfer_length(hsotg, > chan, chnum, qtd, halt_status, NULL); > + urb->actual_length += frame_desc->actual_length; > break; > case DWC2_HC_XFER_FRAME_OVERRUN: > urb->error_count++; > @@ -599,6 +600,7 @@ static enum dwc2_halt_status dwc2_update_isoc_urb_state( > frame_desc->status = -EPROTO; > frame_desc->actual_length = dwc2_get_actual_xfer_length(hsotg, > chan, chnum, qtd, halt_status, NULL); > + urb->actual_length += frame_desc->actual_length; > > /* Skip whole frame */ > if (chan->qh->do_split && > According urb description in usb.h urb->actual_length used for non-iso transfers: "@actual_length: This is read in non-iso completion functions, and ... * ISO transfer status is reported in the status and actual_length fields * of the iso_frame_desc array, " Thanks, Minas
Re: [PATCH] usb: dwc2: host: fix isoc urb actual length
Hi William, On 11/6/2017 2:08 PM, wlf wrote: > Hi Minas, > > 在 2017年11月06日 17:28, Minas Harutyunyan 写道: >> Hi, >> >> On 11/6/2017 12:46 PM, William Wu wrote: >>> The actual_length in dwc2_hcd_urb structure is used >>> to indicate the total data length transferred so far, >>> but in dwc2_update_isoc_urb_state(), it just updates >>> the actual_length of isoc frame, and don't update the >>> urb actual_length at the same time, this will cause >>> device drivers working error which depend on the urb >>> actual_length. >>> >>> we can easily find this issue if use an USB camera， >>> the userspace use libusb to get USB data from kernel >>> via devio driver.In usb devio driver, processcompl() >>> function will process urb complete and copy data to >>> userspace depending on urb actual_length. >>> >>> Let's update the urb actual_length if the isoc frame >>> is valid. >>> >>> Signed-off-by: William Wu >>> --- >>> drivers/usb/dwc2/hcd_intr.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c >>> index 28a8210..01b1e13 100644 >>> --- a/drivers/usb/dwc2/hcd_intr.c >>> +++ b/drivers/usb/dwc2/hcd_intr.c >>> @@ -580,6 +580,7 @@ static enum dwc2_halt_status dwc2_update_isoc_urb_state( >>> frame_desc->status = 0; >>> frame_desc->actual_length = >>> dwc2_get_actual_xfer_length(hsotg, >>> chan, chnum, qtd, halt_status, >>> NULL); >>> + urb->actual_length += frame_desc->actual_length; >>> break; >>> case DWC2_HC_XFER_FRAME_OVERRUN: >>> urb->error_count++; >>> @@ -599,6 +600,7 @@ static enum dwc2_halt_status dwc2_update_isoc_urb_state( >>> frame_desc->status = -EPROTO; >>> frame_desc->actual_length = >>> dwc2_get_actual_xfer_length(hsotg, >>> chan, chnum, qtd, halt_status, >>> NULL); >>> + urb->actual_length += frame_desc->actual_length; >>> >>> /* Skip whole frame */ >>> if (chan->qh->do_split && >>> >> According urb description in usb.h urb->actual_length used for non-iso >> transfers: >> >> "@actual_length: This is read in non-iso completion functions, and ... >> >> * ISO transfer status is reported in the status and actual_length fields >> * of the iso_frame_desc array, " > Yes, urb->actual_length is used for non-iso transfers. And for ISO > transfer, the most > usb class drivers can only use iso frame status and actual_length to > handle usb data, > like: kernel v4.4 drivers/media/usb/uvc/uvc_video.c > > But the usb devio driver really need the urb actual_length in the > processcompl() if > handle ISO transfer, just as I mentioned in the commit message. > > And I also found the same issue on raspberrypi board: > https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_raspberrypi_linux_issues_903&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=6z9Al9FrHR_ZqbbtSAsD16pvOL2S3XHxQnSzq8kusyI&m=v5ECJPMqgVerj8G74QqsTJ7jt-qDVbCVIq8jq_t245s&s=ArX8DL6fZL4p3dsOY5AstGDRguIB_c1GDuuhf49-ev8&e= > > So do you think we need to fix the usb devio driver, rather than fix dwc2? > I think maybe we can use urb actual length for ISO transfer, it seems that > don't cause any side-effect. Agree, should be no any side-effect. > >> >> Thanks, >> Minas >> >> >> >> >> >> >
Re: [PATCH] usb: dwc2: disable erroneous overcurrent condition
On 10/16/2017 8:40 PM, Marek Vasut wrote:
> On 10/16/2017 03:57 PM, Dinh Nguyen wrote:
>> For the case where an external VBUS is used, we should enable the external
>> VBUS comparator in the driver. This would prevent an unnecessary
>> overcurrent error which would then disable the host port.
>>
>> This patch uses the standard 'disable-over-current' binding to allow of the
>> option of disabling the over-current condition.
>>
>> Signed-off-by: Dinh Nguyen
>
> Reviewed-by: Marek Vasut
>
> Similar patch was in U-Boot for two years now and it does the trick indeed.
>
>> ---
>> drivers/usb/dwc2/core.h | 4
>> drivers/usb/dwc2/hcd.c| 5 +
>> drivers/usb/dwc2/params.c | 3 +++
>> 3 files changed, 12 insertions(+)
>>
>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>> index 8367d4f9..730d7eb 100644
>> --- a/drivers/usb/dwc2/core.h
>> +++ b/drivers/usb/dwc2/core.h
>> @@ -395,6 +395,9 @@ enum dwc2_ep0_state {
>>* (default when phy_type is UTMI+ or ULPI)
>>* 1 - 6 MHz
>>* (default when phy_type is Full Speed)
>> + * @oc_disable: Flag to disable overcurrent condition.
>> + * 0 - Allow overcurrent condition to get detected
>> + * 1 - Disable overcurrent condtion to get detected
>>* @ts_dline: Enable Term Select Dline pulsing
>>* 0 - No (default)
>>* 1 - Yes
>> @@ -492,6 +495,7 @@ struct dwc2_core_params {
>> bool dma_desc_fs_enable;
>> bool host_support_fs_ls_low_power;
>> bool host_ls_low_power_phy_clk;
>> +bool oc_disable;
>>
>> u8 host_channels;
>> u16 host_rx_fifo_size;
>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>> index c263114..5e20336 100644
>> --- a/drivers/usb/dwc2/hcd.c
>> +++ b/drivers/usb/dwc2/hcd.c
>> @@ -213,6 +213,11 @@ static int dwc2_hs_phy_init(struct dwc2_hsotg *hsotg,
>> bool select_phy)
>> usbcfg &= ~(GUSBCFG_PHYIF16 | GUSBCFG_DDRSEL);
>> if (hsotg->params.phy_ulpi_ddr)
>> usbcfg |= GUSBCFG_DDRSEL;
>> +
>> +/* Set external VBUS indicator as needed. */
>> +if (hsotg->params.oc_disable)
>> +usbcfg |= (GUSBCFG_ULPI_INT_VBUS_IND |
>> + GUSBCFG_INDICATORPASSTHROUGH);
>> break;
>> case DWC2_PHY_TYPE_PARAM_UTMI:
>> /* UTMI+ interface */
>> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
>> index a3ffe97..39e02cd 100644
>> --- a/drivers/usb/dwc2/params.c
>> +++ b/drivers/usb/dwc2/params.c
>> @@ -335,6 +335,9 @@ static void dwc2_get_device_properties(struct dwc2_hsotg
>> *hsotg)
>> num);
>> }
>> }
>> +
>> +if (of_find_property(hsotg->dev->of_node, "disable-over-current", NULL))
>> +p->oc_disable = true;
>> }
>>
>> static void dwc2_check_param_otg_cap(struct dwc2_hsotg *hsotg)
>>
>
>
Hi John Youn, I checked with HW team - patch is OK.
Acked-by: Minas Harutyunyan
Re: [RESEND x2][PATCH 0/3] dwc2 fixes for edge cases on hikey
On 10/17/2017 12:41 PM, Minas Harutyunyan wrote:
> On 10/17/2017 1:34 AM, John Stultz wrote:
>> On Mon, Oct 16, 2017 at 1:36 AM, Minas Harutyunyan
>> wrote:
>>> On b-plug disconnect should asserted GOTGINT.SesEndDet interrupt.
>>> According previously sent by you register dump (GHWCFG2 = 0x23affc70)
>>> your core OTG_MODE=0.
>>> Bellow fragment from programming guide on Device disconnect:
>>>
>>> "7.3Device Disconnection
>>> The device session ends when the USB cable is disconnected or if the
>>> VBUS is switched off by the Host. The
>>> device disconnect flow varies depending on the value of the OTG_MODE
>>> configuration parameter.
>>>
>>> When OTG_MODE = 0,1, or 3
>>> When OTG_MODE is set to 0,1, or 3, the device disconnect flow is as follows:
>>> 1. When the USB cable is unplugged or when the VBUS is switched off by
>>> the Host, the Device core
>>> trigger GINTSTS.OTGInt [bit 2] interrupt bit.
>>> 2. When the device application detects GINTSTS.OTGInt interrupt, it
>>> checks that the
>>> GOTGINT.SesEndDet (Session End Detected) bit is set to 1’b1."
>>>
>>> So, you should receive and handle "Session End Detected". In function
>>> dwc2_handle_otg_intr() on this interrupt (in device mode) calling
>>> dwc2_hsotg_disconnect() function. By adding your patch "[PATCH 3/3] usb:
>>> dwc2: Fix UDC state tracking" state changed to not attached as required.
>>
>>
>> So, on the HiKey board (using 4.14-rc5 + Vardan's patch), I'm not
>> seeing the GOTGINT_SES_END_DET in dwc2_handle_otg_intr() when I remove
>> the USB OTG cable.
>>
>> In fact, I'm not seeing any calls to dwc2_handle_otg_intr()... which
>> seems... odd maybe? Any clues as to what might be going wrong then?
>>
>> thanks
>> -john
>>
> Hi John Stultz,
> So, on Hikey board on unplug B connector GOTGINT.SesEndDet interrupt not
> asserted, instead asserted GINTSTS_CONIDSTSCHNG. Please, confirm.
>
> In this case without your patch "[PATCH 1/3] usb: dwc2: Improve gadget
> state disconnection handling" but by applying your patch "[PATCH 3/3]
> usb: dwc2: Fix UDC state tracking":
> 1. On B plug connect UDC state will be set to "configured"
> 2. On B plug disconnect - "not attached".
> Is it Ok for you?
>
> Meantime, I'll check with HW team why GOTGINT.SesEndDet interrupt not
> asserted on unplug B connector.
>
> Thanks,
> Minas
>
Hi John Stultz,
Could you please apply this patch. Please not apply your patch series
"[PATCH 0/3] dwc2 fixes for edge cases on hikey" to check only below patch.
If you confirm that this patch fix your issue with "Transaction Error"
and " ChHltd set, but reason is unknown" I'll submit to LKML as final patch.
Then can be applied your patch series, without patch 1/3 (changing in
connector id status change) to correctly set UDC states.
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index
f4ef159b538e..7da22152df68 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -331,6 +331,9 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg)
usbcfg = dwc2_readl(hsotg->regs + GUSBCFG);
usbcfg &= ~(GUSBCFG_HNPCAP | GUSBCFG_SRPCAP);
+ /* Set HS/FS Timeout Calibration */
+ usbcfg |= GUSBCFG_TOUTCAL(7);
+
switch (hsotg->hw_params.op_mode) {
case GHWCFG2_OP_MODE_HNP_SRP_CAPABLE:
if (hsotg->params.otg_cap ==
Thanks,
Minas
Re: [PATCH v2] usb: dwc3: Prevent indefinite sleep in _dwc3_set_mode during suspend/resume
Hi Filipe, On 3/19/2018 5:53 PM, Minas Harutyunyan wrote: > Hi, > > On 3/19/2018 3:36 PM, Minas Harutyunyan wrote: >> Hi, >> >> On 3/19/2018 12:55 PM, Felipe Balbi wrote: >>> >>> Hi, >>> >>> Minas Harutyunyan writes: >>>>>>>>> Thanks for picking this for -next. >>>>>>>>> Is it better to have this in v4.16-rc fixes? >>>>>>>>> and also stable? v4.12+ >>>>>>>> >>>>>>>> Well, there was no "Fixes: foobar" or "Cc: stable" lines in the commit >>>>>>>> log ;-) >>>>>>>> >>>>>>>> The best we can do now, is wait for -rc1 and manually send the commit >>>>>>>> to >>>>>>>> stable. >>>>>>>> >>>>>>> >>>>>>> That's fine. Thanks. >>>>>>> >>>>>> >>>>>> Same issue seen in dwc3_gadget_ep_dequeue() function where also used >>>>>> wait_event_lock_irq() - as result infinite loop. >>>>> >>>>> how did this happen? During rmmod dwc3? Or, perhaps, after you unloaded >>>>> a gadget driver? >>>>> >>>> No, not during rmmod's. >>>> We using our internal USB testing tool. Test case; ISOC OUT, transfer >>>> size N frames. When host starts ISOC OUT traffic then the dwc3 based on >>>> "Transfer not ready" event in frame F starts transfers staring from >>>> frame F+4 (for bInterval=1) as result 4 requests, which already queued >>>> on device side, remain incomplete. Function driver on some timeout >>>> trying dequeue these 4 requests (without disabling EP) to complete test. >>>> For IN ISOC's these requests completed on MISSED ISOC event, but for >>>> ISOC OUT required call dequeue on some timeout. >>> >>> okay >>> >>>>>> Actually to fix this issue I updated condition of wait function >>>>>> from: >>>>>> !(dep->flags & DWC3_EP_END_TRANSFER_PENDING) >>>>>> to: >>>>>> !(dep->flags & DWC3_EP_END_TRANSFER_PENDING & DWC3_EP_ENABLED) >>>>> >>>>> you're not fixing anything. You're, essentially, removing the entire >>>>> end transfer pending logic. >>>> yes, you are right, but how to overcome this infinite loop? Replace >>>> wait_event_lock_irq() by wait_event_interruptible_lock_irq_timeout()? >>> >>> The best way here would be to figure why we're missing command complete >>> IRQ in those cases. According to documentation, we *should* receive that >>> interrupt, so why is it missing? >>> >> >> Additional info on test. Core configuration is HS only mode, test speed >> HS, core version v2.90a. Maybe it will help to understand cause of issue. >> BTW, currently to pass above describe ISOC OUT test we just commented >> wait_event_lock_irq() in dwc3_gadget_ep_dequeue() function and >> successfully received request completion in function driver. >> Thanks, >> Minas >> > > One more info: while function driver call dequeue, host periodically > send control read command to get status of test from function - test In > Progress or Finished. > Thanks, > Minas > Your last dwc3 patch series allow us to successfully dequeuing remaining requests without falling in to infinite loop. Thank you, Minas
Re: [PATCH v4] usb: dwc2: dwc2_vbus_supply_init: fix error check
Hi Heiko,
On 4/10/2018 4:28 PM, Heiko Stuebner wrote:
> Am Montag, 26. März 2018, 11:00:01 CEST schrieb Tomeu Vizoso:
>> devm_regulator_get_optional returns -ENODEV if the regulator isn't
>> there, so if that's the case we have to make sure not to leave -ENODEV
>> in the regulator pointer.
>>
>> Also, make sure we return 0 in that case, but correctly propagate any
>> other errors. Also propagate the error from _dwc2_hcd_start.
>>
>> Fixes: 531ef5ebea96 ("usb: dwc2: add support for host mode external vbus
>> supply")
>> Cc: Amelie Delaunay
>> Signed-off-by: Tomeu Vizoso
>
> The patch that gets fixed here also breaks display-output on dwc2-based
> Rockchip devices (likely even more), probably due to making the regulator
> framework hickup.
>
Could you please elaborate what mean "breaks display-output".
On which Kernel version you apply this patch?
Thanks,
Minas
> With this patch applied, apart from not seeing the NULL-ptr, I also get
> display output on my rk3288-veycron Chromebook again, so
>
> Tested-by: Heiko Stuebner
>
>
>> v2: Only overwrite the error in the pointer after checking it (Heiko
>> Stübner )
>> v3: Remove hunks that shouldn't be in this patch
>> v4: Don't overwrite the error code before returning it (kbuild test
>> robot )
>> ---
>> drivers/usb/dwc2/hcd.c | 13 -
>> 1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>> index 190f95964000..c51b73b3e048 100644
>> --- a/drivers/usb/dwc2/hcd.c
>> +++ b/drivers/usb/dwc2/hcd.c
>> @@ -358,9 +358,14 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg)
>>
>> static int dwc2_vbus_supply_init(struct dwc2_hsotg *hsotg)
>> {
>> +int ret;
>> +
>> hsotg->vbus_supply = devm_regulator_get_optional(hsotg->dev, "vbus");
>> -if (IS_ERR(hsotg->vbus_supply))
>> -return 0;
>> +if (IS_ERR(hsotg->vbus_supply)) {
>> +ret = PTR_ERR(hsotg->vbus_supply);
>> +hsotg->vbus_supply = NULL;
>> +return ret == -ENODEV ? 0 : ret;
>> +}
>>
>> return regulator_enable(hsotg->vbus_supply);
>> }
>> @@ -4342,9 +4347,7 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)
>>
>> spin_unlock_irqrestore(&hsotg->lock, flags);
>>
>> -dwc2_vbus_supply_init(hsotg);
>> -
>> -return 0;
>> +return dwc2_vbus_supply_init(hsotg);
>> }
>>
>> /*
>>
>
>
>
Re: [PATCH v4] usb: dwc2: dwc2_vbus_supply_init: fix error check
Hi Heiko,
On 4/10/2018 7:37 PM, Heiko Stübner wrote:
> Am Dienstag, 10. April 2018, 15:52:25 CEST schrieb Minas Harutyunyan:
>> Hi Heiko,
>>
>> On 4/10/2018 4:28 PM, Heiko Stuebner wrote:
>>> Am Montag, 26. März 2018, 11:00:01 CEST schrieb Tomeu Vizoso:
>>>> devm_regulator_get_optional returns -ENODEV if the regulator isn't
>>>> there, so if that's the case we have to make sure not to leave -ENODEV
>>>> in the regulator pointer.
>>>>
>>>> Also, make sure we return 0 in that case, but correctly propagate any
>>>> other errors. Also propagate the error from _dwc2_hcd_start.
>>>>
>>>> Fixes: 531ef5ebea96 ("usb: dwc2: add support for host mode external vbus
>>>> supply") Cc: Amelie Delaunay
>>>> Signed-off-by: Tomeu Vizoso
>>>
>>> The patch that gets fixed here also breaks display-output on dwc2-based
>>> Rockchip devices (likely even more), probably due to making the regulator
>>> framework hickup.
>>
>> Could you please elaborate what mean "breaks display-output".
>> On which Kernel version you apply this patch?
>
> I think I may have written that poorly. _Without_ this patch I get
> display breakage on the most recent torvalds/master (merge-window)
> where "usb: dwc2: add support for host mode external vbus supply" is
> applied and this patch fixes the issue.
>
> "breaks display output" means both hdmi + edp output are missing
> also including the backlight staying off.
>
> The patch we're fixing here, causes a null-pointer dereference in the
> regulator framework, which seems to also cause issues when other
> regulators are enabled, which I think is what I'm seeing here.
>
>
Thank you for clarification. Earlier you added "reviewed" tag, this
feedback can assumed as "tested" tag.
Thanks,
Minas
> Heiko
>
>>
>> Thanks,
>> Minas
>>
>>> With this patch applied, apart from not seeing the NULL-ptr, I also get
>>> display output on my rk3288-veycron Chromebook again, so
>>>
>>> Tested-by: Heiko Stuebner
>>>
>>>> v2: Only overwrite the error in the pointer after checking it (Heiko
>>>>
>>>> Stübner )
>>>>
>>>> v3: Remove hunks that shouldn't be in this patch
>>>> v4: Don't overwrite the error code before returning it (kbuild test
>>>>
>>>> robot )
>>>>
>>>> ---
>>>>
>>>>drivers/usb/dwc2/hcd.c | 13 -
>>>>1 file changed, 8 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>>>> index 190f95964000..c51b73b3e048 100644
>>>> --- a/drivers/usb/dwc2/hcd.c
>>>> +++ b/drivers/usb/dwc2/hcd.c
>>>> @@ -358,9 +358,14 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg
>>>> *hsotg)>>
>>>>static int dwc2_vbus_supply_init(struct dwc2_hsotg *hsotg)
>>>>{
>>>>
>>>> + int ret;
>>>> +
>>>>
>>>>hsotg->vbus_supply = devm_regulator_get_optional(hsotg->dev,
>>>> "vbus");
>>>>
>>>> - if (IS_ERR(hsotg->vbus_supply))
>>>> - return 0;
>>>> + if (IS_ERR(hsotg->vbus_supply)) {
>>>> + ret = PTR_ERR(hsotg->vbus_supply);
>>>> + hsotg->vbus_supply = NULL;
>>>> + return ret == -ENODEV ? 0 : ret;
>>>> + }
>>>>
>>>>return regulator_enable(hsotg->vbus_supply);
>>>>
>>>>}
>>>>
>>>> @@ -4342,9 +4347,7 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)
>>>>
>>>>spin_unlock_irqrestore(&hsotg->lock, flags);
>>>>
>>>> - dwc2_vbus_supply_init(hsotg);
>>>> -
>>>> - return 0;
>>>> + return dwc2_vbus_supply_init(hsotg);
>>>>
>>>>}
>>>>
>>>>/*
>
>
>
Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"
Hi Maynard,
On 12/7/2018 3:09 AM, Maynard CABIENTE wrote:
> Hi Minas,
>
> I tried your new patch on top of the other 2 patches for a couple of days now
> and I do not see the issue that Marek encountered. Of course, I did not see
> it also on the original two patches you created. I also do not see the
> original FIFO map warning issue that I have with all 3 patches intact.
>
> However, I do have 2 issues that I'm not certain if this was created by these
> patches or already there in the beginning (prior to applying all your 3
> patches). Again my system is an Altera Cyclone V SoC FPGA running on linux
> kernel 4.14.44. We are using both USB HID and mass storage gadget. The 2
> issues may occur when USB mass storage is in use.
>
> First issue I am encountering is (sometimes continuous) multiple USB resets
> occurring when using the USB mass storage gadget. When I used a USB sniffer
> (Ellisys) to see the cause of the USB reset, I am seeing an out-of-order USB
> mass storage transfer. That is, the transfer is supposed to follow (1)
> Command Block Wrapper (CBW), (2) Data, (3) Command Status Wrapper (CSW). When
> the USB reset occur, I see either one of the following prior to the reset:
>
> - Data comes in first before the CBW
> - Request Sense CBW, Data then no CSW followed by a Test Unit Ready CBW and
> Data with no CSW again
> - CBW, CSW and then Data
>
> Second issue that I see is a deadlock then my system will reboot. This
> happens when I re-enable the USB mass storage gadget after disabling it
> before.
>
> The two issues may not be related to your changes. I will revert back all
> your patches to see if the issue is present already. However, I will hit my
> original problem when I don't apply at least 2 of your patches, which is the
> WARNING: CPU: 0 PID: 0 at ../drivers/usb/dwc2/gadget.c:300
> dwc2_hsotg_init_fifo+0x34/0x1b4.
>
> I will get back to you once I test it without your patches.
>
> Regards,
> Maynard
>
Thank you for testing.
Issue which described looks like not related to these patches. For that
issue please open another mail thread with more details. We will work on it.
Thanks,
Minas
> On Tuesday, December 04, 2018 7:34 AM, Minas Harutyunyan wrote:
>> On 11/23/2018 6:43 PM, Dan Carpenter wrote:
>>> Ugh... We also had a long thread about the v2 patch but it turns out
>>> the list was not CC'd. I should have checked for that.
>>>
>>> You have to pass a flag to say if the caller holds the lock or not...
>>>
>>> regards,
>>> dan carpenter
>>>
>>
>> Hi Dan, Marek, Maynard,
>>
>> Could you please apply bellow patch over follow patches:
>>
>> dccf1bad4be7 usb: dwc2: Disable all EP's on disconnect
>> 6f774b501928 usb: dwc2: Fix ep disable spinlock flow.
>>
>> Please review and test. Feedback is appreciated :-)
>>
>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index
>> 8eb25e3597b0..db438d13c36a 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -3165,8 +3165,6 @@ static void kill_all_requests(struct dwc2_hsotg
>> *hsotg,
>> dwc2_hsotg_txfifo_flush(hsotg, ep->fifo_index);
>>}
>>
>> -static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
>> -
>>/**
>> * dwc2_hsotg_disconnect - disconnect service
>> * @hsotg: The device state.
>> @@ -3185,12 +3183,13 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg
>> *hsotg)
>> hsotg->connected = 0;
>> hsotg->test_mode = 0;
>>
>> - /* all endpoints should be shutdown */
>> for (ep = 0; ep < hsotg->num_of_eps; ep++) {
>> if (hsotg->eps_in[ep])
>> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>> + kill_all_requests(hsotg, hsotg->eps_in[ep],
>> + -ESHUTDOWN);
>> if (hsotg->eps_out[ep])
>> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>> + kill_all_requests(hsotg, hsotg->eps_out[ep],
>> + -ESHUTDOWN);
>> }
>>
>> call_gadget(hsotg, disconnect); @@ -3234,6 +3233,8 @@ static void
>> dwc2_hsotg_irq_fifoempty(struct dwc2_hsotg *hsotg, bool periodic)
>> GINTSTS_PTXFEMP | \
>> GINTSTS_RXFLVL)
>>
>> +static int dwc2_hsotg_ep_disable(struct
Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"
Hi Marek,
On 12/6/2018 7:04 PM, Marek Szyprowski wrote:
> Dear Minas,
>
> On 2018-12-04 13:34, Minas Harutyunyan wrote:
>> On 11/23/2018 6:43 PM, Dan Carpenter wrote:
>>> Ugh... We also had a long thread about the v2 patch but it turns out
>>> the list was not CC'd. I should have checked for that.
>>>
>>> You have to pass a flag to say if the caller holds the lock or not...
>>>
>>> regards,
>>> dan carpenter
>>>
>> Hi Dan, Marek, Maynard,
>>
>> Could you please apply bellow patch over follow patches:
>>
>> dccf1bad4be7 usb: dwc2: Disable all EP's on disconnect
>> 6f774b501928 usb: dwc2: Fix ep disable spinlock flow.
>>
>> Please review and test. Feedback is appreciated :-)
>
> Okay, I finally managed to find some time to check this. Your diff is
> mangled, so I had to manually apply it. Frankly, it is very similar to
> the revert I proposed. I've checked it on my test machines and it fixes
> the issues. I'm not very happy about the unlock/lock design, but it
> should be safe in this case and doesn't make the code even more complex.
> Feel free to add a following tag to the final patch:
>
> Tested-by: Marek Szyprowski
Thanks for testing.
>
>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>> index 8eb25e3597b0..db438d13c36a 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -3165,8 +3165,6 @@ static void kill_all_requests(struct dwc2_hsotg
>> *hsotg,
>> dwc2_hsotg_txfifo_flush(hsotg, ep->fifo_index);
>>}
>>
>> -static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
>> -
>>/**
>> * dwc2_hsotg_disconnect - disconnect service
>> * @hsotg: The device state.
>> @@ -3185,12 +3183,13 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg)
>> hsotg->connected = 0;
>> hsotg->test_mode = 0;
>>
>> - /* all endpoints should be shutdown */
>> for (ep = 0; ep < hsotg->num_of_eps; ep++) {
>> if (hsotg->eps_in[ep])
>> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>> + kill_all_requests(hsotg, hsotg->eps_in[ep],
>> + -ESHUTDOWN);
>> if (hsotg->eps_out[ep])
>> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>> + kill_all_requests(hsotg, hsotg->eps_out[ep],
>> + -ESHUTDOWN);
>> }
>>
>> call_gadget(hsotg, disconnect);
>> @@ -3234,6 +3233,8 @@ static void dwc2_hsotg_irq_fifoempty(struct
>> dwc2_hsotg *hsotg, bool periodic)
>> GINTSTS_PTXFEMP | \
>> GINTSTS_RXFLVL)
>>
>> +static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
>> +
>>/**
>> * dwc2_hsotg_core_init - issue softreset to the core
>> * @hsotg: The device state
>> @@ -3258,12 +3259,14 @@ void dwc2_hsotg_core_init_disconnected(struct
>> dwc2_hsotg *hsotg,
>> return;
>> } else {
>> /* all endpoints should be shutdown */
>> + spin_unlock(&hsotg->lock);
>> for (ep = 1; ep < hsotg->num_of_eps; ep++) {
>> if (hsotg->eps_in[ep])
>>
>> dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>> if (hsotg->eps_out[ep])
>>
>> dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>> }
>> + spin_lock(&hsotg->lock);
>> }
>>
>> /*
>> @@ -4072,7 +4075,6 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>> unsigned long flags;
>> u32 epctrl_reg;
>> u32 ctrl;
>> - int locked;
>>
>> dev_dbg(hsotg->dev, "%s(ep %p)\n", __func__, ep);
>>
>> @@ -4088,7 +4090,7 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>>
>> epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
>>
>> - locked = spin_trylock_irqsave(&hsotg->lock, flags);
>> + spin_lock_irqsave(&hsotg->lock, flags);
>>
>> ctrl = dwc2_readl(hsotg, epctrl_reg);
>>
>> @@ -4112,8 +4114,7 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>> hs_ep->fifo_index = 0;
>> hs_ep->fifo_size = 0;
>>
>> - if (locked)
>> - spin_unlock_irqrestore(&hsotg->lock, flags);
>> + spin_unlock_irqrestore(&hsotg->lock, flags);
>>
>> return 0;
>>}
>>
>> Thanks,
>> Minas
>>
>>
> Best regards
>
Thanks,
Minas
Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"
Hi Filipe, My patch dccf1bad4be7eaa096c1f3697bd37883f9a08ecb "usb: dwc2: Disable all EP's on disconnect" applied to 4.20-rc1. I need to update this patch. What I should do. There are 2 options: 1. Ack Marek Szyprowski [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect" then submit final fixed version of patch? 2. Or submit new patch to fix existing "usb: dwc2: Disable all EP's on disconnect". Please advise. Thanks, Minas
Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"
Hi Dan,
On 12/7/2018 2:16 PM, Dan Carpenter wrote:
> On Wed, Dec 05, 2018 at 12:52:22PM +0000, Minas Harutyunyan wrote:
>> Hi,
>>
>> On 12/4/2018 5:29 PM, Dan Carpenter wrote:
>>> On Tue, Dec 04, 2018 at 12:34:08PM +, Minas Harutyunyan wrote:
>>>> @@ -3185,12 +3183,13 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg
>>>> *hsotg)
>>>>hsotg->connected = 0;
>>>>hsotg->test_mode = 0;
>>>>
>>>> - /* all endpoints should be shutdown */
>>>>for (ep = 0; ep < hsotg->num_of_eps; ep++) {
>>>>if (hsotg->eps_in[ep])
>>>> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>>>> + kill_all_requests(hsotg, hsotg->eps_in[ep],
>>>> + -ESHUTDOWN);
>>>>if (hsotg->eps_out[ep])
>>>> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>>>> + kill_all_requests(hsotg, hsotg->eps_out[ep],
>>>> + -ESHUTDOWN);
>>>
>>>
>>> Should this part be in a separate patch?
>>>
>>> I'm not trying to be rhetorical at all. I literally don't know the
>>> code very well. Hopefully the full commit message will explain it.
>>>
>>
>> Actually, this fragment of patch revert changes from V2 and keep
>> untouched dwc2_hsotg_disconnect() function.
>>
>
> To me it feels like there are two issues. The first is this change, and
> the second is fixing the lockdep warning.
>
>
>>>>}
>>>>
>>>>call_gadget(hsotg, disconnect);
>>>> @@ -3234,6 +3233,8 @@ static void dwc2_hsotg_irq_fifoempty(struct
>>>> dwc2_hsotg *hsotg, bool periodic)
>>>>GINTSTS_PTXFEMP | \
>>>>GINTSTS_RXFLVL)
>>>>
>>>> +static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
>>>> +
>>>> /**
>>>> * dwc2_hsotg_core_init - issue softreset to the core
>>>> * @hsotg: The device state
>>>> @@ -3258,12 +3259,14 @@ void dwc2_hsotg_core_init_disconnected(struct
>>>> dwc2_hsotg *hsotg,
>>>>return;
>>>>} else {
>>>>/* all endpoints should be shutdown */
>>>> + spin_unlock(&hsotg->lock);
>>>>for (ep = 1; ep < hsotg->num_of_eps; ep++) {
>>>>if (hsotg->eps_in[ep])
>>>>
>>>> dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>>>>if (hsotg->eps_out[ep])
>>>>
>>>> dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>>>>}
>>>> + spin_lock(&hsotg->lock);
>>>>}
>>>>
>>>>/*
>>>
>>> The idea here is that this is the only caller which is holding the
>>> lock and we drop it here and take it again inside dwc2_hsotg_ep_disable().
>>> I don't know the code very well and can't totally swear that this
>>> doesn't introduce a small race condition...
>>>
>> Above fragment of patch allow to keep untouched dwc2_hsotg_ep_disble()
>> function also, without changing spin_lock/_unlock stuff inside function.
>>
>> My approach here minimally update code to add any races. Just in
>> dwc2_hsotg_core_init_disconnected() function on USB reset interrupt
>> perform disabling all EP's. Because on USB reset interrupt, called from
>> interrupt
>> handler with acquired lock and dwc2_hsotg_ep_disble() function (without
>> changes) acquire lock, just need to unlock lock to avoid any troubles.
>>
>
> Yes. I understand that. I just don't like it.
>
> Although your patch is more "minimal" in that it touches fewer lines of
> code it's actually more complicated because we have to verify that it's
> safe to drop the lock.
>
>
>>> Another option would be to introduce a new function which takes the lock
>>> and change all the other callers instead. To me that would be easier to
>>> review... See below for how it might look:
>>>
>>> regards,
>>> dan carpenter
>
Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"
Hi Dan,
On 12/7/2018 3:20 PM, Minas Harutyunyan wrote:
> Hi Dan,
>
> On 12/7/2018 2:16 PM, Dan Carpenter wrote:
>> On Wed, Dec 05, 2018 at 12:52:22PM +, Minas Harutyunyan wrote:
>>> Hi,
>>>
>>> On 12/4/2018 5:29 PM, Dan Carpenter wrote:
>>>> On Tue, Dec 04, 2018 at 12:34:08PM +, Minas Harutyunyan wrote:
>>>>> @@ -3185,12 +3183,13 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg
>>>>> *hsotg)
>>>>> hsotg->connected = 0;
>>>>> hsotg->test_mode = 0;
>>>>>
>>>>> - /* all endpoints should be shutdown */
>>>>> for (ep = 0; ep < hsotg->num_of_eps; ep++) {
>>>>> if (hsotg->eps_in[ep])
>>>>> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>>>>> + kill_all_requests(hsotg, hsotg->eps_in[ep],
>>>>> + -ESHUTDOWN);
>>>>> if (hsotg->eps_out[ep])
>>>>> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>>>>> + kill_all_requests(hsotg, hsotg->eps_out[ep],
>>>>> + -ESHUTDOWN);
>>>>
>>>>
>>>> Should this part be in a separate patch?
>>>>
>>>> I'm not trying to be rhetorical at all. I literally don't know the
>>>> code very well. Hopefully the full commit message will explain it.
>>>>
>>>
>>> Actually, this fragment of patch revert changes from V2 and keep
>>> untouched dwc2_hsotg_disconnect() function.
>>>
>>
>> To me it feels like there are two issues. The first is this change, and
>> the second is fixing the lockdep warning.
>>
>>
>>>>> }
>>>>>
>>>>> call_gadget(hsotg, disconnect);
>>>>> @@ -3234,6 +3233,8 @@ static void dwc2_hsotg_irq_fifoempty(struct
>>>>> dwc2_hsotg *hsotg, bool periodic)
>>>>> GINTSTS_PTXFEMP | \
>>>>> GINTSTS_RXFLVL)
>>>>>
>>>>> +static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
>>>>> +
>>>>> /**
>>>>> * dwc2_hsotg_core_init - issue softreset to the core
>>>>> * @hsotg: The device state
>>>>> @@ -3258,12 +3259,14 @@ void dwc2_hsotg_core_init_disconnected(struct
>>>>> dwc2_hsotg *hsotg,
>>>>> return;
>>>>> } else {
>>>>> /* all endpoints should be shutdown */
>>>>> + spin_unlock(&hsotg->lock);
>>>>> for (ep = 1; ep < hsotg->num_of_eps; ep++) {
>>>>> if (hsotg->eps_in[ep])
>>>>>
>>>>> dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>>>>> if (hsotg->eps_out[ep])
>>>>>
>>>>> dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>>>>> }
>>>>> + spin_lock(&hsotg->lock);
>>>>> }
>>>>>
>>>>> /*
>>>>
>>>> The idea here is that this is the only caller which is holding the
>>>> lock and we drop it here and take it again inside dwc2_hsotg_ep_disable().
>>>> I don't know the code very well and can't totally swear that this
>>>> doesn't introduce a small race condition...
>>>>
>>> Above fragment of patch allow to keep untouched dwc2_hsotg_ep_disble()
>>> function also, without changing spin_lock/_unlock stuff inside function.
>>>
>>> My approach here minimally update code to add any races. Just in
>>> dwc2_hsotg_core_init_disconnected() function on USB reset interrupt
>>> perform disabling all EP's. Because on USB reset interrupt, called from
>>> interrupt
>>> handler with acquired lock and dwc2_hsotg_ep_disble() function (without
>>> changes) acquire lock, just need to unlock lock to avoid any troubles.
>>>
>>
>> Yes. I understand that. I just don't like it.
>>
>> Although your patch is more "mini
Re: [PATCH 1/1] usb: dwc2: disable power_down on Amlogic devices
On 12/9/2018 11:01 PM, Martin Blumenstingl wrote:
> Disable power_down by setting the parameter to
> DWC2_POWER_DOWN_PARAM_NONE. This fixes a problem on various Amlogic
> Meson SoCs where USB devices are only recognized when plugged in before
> booting Linux. A hot-plugged USB device was not detected even though the
> device got power (my USB thumb drive for example has an LED which lit
> up).
>
> A similar fix was implemented for Rockchip SoCs in commit c216765d3a1def
> ("usb: dwc2: disable power_down on rockchip devices"). That commit
> suggests that a change in the dwc2 driver is the cause because the
> default value for the "hibernate" parameter (which then got renamed to
> "power_down" to support other modes) was changed in the v4.17 merge
> window with:
> commit 6d23ee9caa6790 ("Merge tag 'usb-for-v4.17' of
> git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb into usb-testing").
>
> Cc: # 4.19
> Suggested-by: Christian Hewitt
> Signed-off-by: Martin Blumenstingl
Acked-by: Minas Harutyunyan
> ---
> drivers/usb/dwc2/params.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
> index 7c1b6938f212..38c813b1d203 100644
> --- a/drivers/usb/dwc2/params.c
> +++ b/drivers/usb/dwc2/params.c
> @@ -111,6 +111,7 @@ static void dwc2_set_amlogic_params(struct dwc2_hsotg
> *hsotg)
> p->phy_type = DWC2_PHY_TYPE_PARAM_UTMI;
> p->ahbcfg = GAHBCFG_HBSTLEN_INCR8 <<
> GAHBCFG_HBSTLEN_SHIFT;
> + p->power_down = DWC2_POWER_DOWN_PARAM_NONE;
> }
>
> static void dwc2_set_amcc_params(struct dwc2_hsotg *hsotg)
>
Re: [PATCH] usb: dwc2: gadget: fix ISOC frame overflow handling
Hi John,
On 12/18/2018 6:35 PM, John Keeping wrote:
> Hi Minas,
>
> On Fri, 14 Dec 2018 09:00:08 +0000
> Minas Harutyunyan wrote:
>> First of all, sorry for delayed answer.
>> Looks like similar issue seen by Andrzej Pietrasiewicz
>> : "dwc2 isochronous transfers issues". Same
>> feedback provided to Andrzej.
>>
>> I run tests on 4.20.0-rc4 in DDMA. By default IN ISOC traffic failed
>> because of BNA interrupts. It's happen because UAC2 requests count
>> set by default to 2. Our core and driver can't work in DDMA with
>> descriptor list length equal to 2. It's not possible on time prepare
>> next descriptors to avoid BNA interrupt.
>>
>> By changing UAC2_DEF_REQ_NUM to 4 all audio gadget tests passed
>> smoothly. Could you please apply this patch and run tests in DDMA
>> mode:
>>
>> diff --git a/drivers/usb/gadget/function/u_uac2.h
>> b/drivers/usb/gadget/function/u_uac2.h
>> index 8362ee572e1e..5e649259ab76 100644
>> --- a/drivers/usb/gadget/function/u_uac2.h
>> +++ b/drivers/usb/gadget/function/u_uac2.h
>> @@ -21,7 +21,7 @@
>>#define UAC2_DEF_CCHMASK 0x3
>>#define UAC2_DEF_CSRATE 64000
>>#define UAC2_DEF_CSSIZE 2
>> -#define UAC2_DEF_REQ_NUM 2
>> +#define UAC2_DEF_REQ_NUM 4
>>
>>struct f_uac2_opts {
>> struct usb_function_instancefunc_inst;
>>
>>
>> If it will OK on your side also then will switch to BDMA mode and
>> debug.
>
> With DDMA enabled, I see the following error after the stream has been
> running for a while (anything from a few seconds to a few minutes):
>
> -- >8 --
> [ 1798.836322] dwc2 ff58.usb: dwc2_hsotg_ep_disable: called for ep0
> [ 1798.836329] dwc2 ff58.usb: dwc2_hsotg_ep_disable: called for ep0
> [ 1798.851092] dwc2 ff58.usb: new device is high-speed
> [ 1798.924461] dwc2 ff58.usb: new device is high-speed
> [ 1798.982887] dwc2 ff58.usb: new address 25
> [ 1799.002463] configfs-gadget gadget: high-speed config #1: config
> -- 8< --
>
> On the host side (Linux 4.18.16 Intel xHCI), I see this logged at the
> same time:
>
> -- >8 --
> [1735740.716242] retire_capture_urb: usb 1-2.2.7: frame 0 active: -71
> [1735740.716990] retire_capture_urb: usb 1-2.2.7: frame 0 active: -71
> [1735740.717906] retire_capture_urb: usb 1-2.2.7: frame 0 active: -71
> [1735740.718905] retire_capture_urb: usb 1-2.2.7: frame 0 active: -71
> [1735740.719916] retire_capture_urb: usb 1-2.2.7: frame 0 active: -71
> [1735740.720032] usb 1-2.2-port7: disabled by hub (EMI?), re-enabling...
> [1735740.720036] usb 1-2.2.7: USB disconnect, device number 28
> [1735740.937500] usb 1-2.2.7: new high-speed USB device number 29 using
> xhci_hcd
> -- 8< --
>
> The device does then enumerate and works for a period of time before the
> same error happens again.
>
From logs not clear who initiate disconnect: host or device. More
probably host, after some errors in retire_capture_urb initiate
disconnect. Do you have any idea?
Can't you connect device to host on same platform?
If root cause of disconnect by host is device issue, i.e. not able to
send to host data packets in time then I need device side dmesg log with
debug enabled. USB trace around the disconnect will help to debug.
Thanks,
Minas
>
> Regards,
> John
>
Re: [PATCH] usb: dwc2: gadget: fix ISOC frame overflow handling
Hi John,
On 11/13/2018 2:46 AM, John Keeping wrote:
> Hi Minas,
>
> On Mon, 12 Nov 2018 08:53:36 +0000
> Minas Harutyunyan wrote:
>> On 11/9/2018 10:43 PM, John Keeping wrote:
>>> On Fri, 9 Nov 2018 14:36:36 +
>>> Minas Harutyunyan wrote:
>>>
>>>> On 11/9/2018 12:43 PM, Minas Harutyunyan wrote:
>>>>> Hi John,
>>>>>
>>>>> On 11/8/2018 9:37 PM, John Keeping wrote:
>>>>>> Hi Minas,
>>>>>>
>>>>>> On Mon, 5 Nov 2018 08:28:07 +
>>>>>> Minas Harutyunyan wrote:
>>>>>>
>>>>>>> On 10/23/2018 5:43 PM, John Keeping wrote:
>>>>>>>> By clearing the overrun flag as soon as the target frame is
>>>>>>>> next incremented, we can end up incrementing the target frame
>>>>>>>> more than expected in dwc2_gadget_handle_ep_disabled() when the
>>>>>>>> endpoint's interval is greater than 1. This happens if the
>>>>>>>> target frame has just wrapped at the point when the endpoint is
>>>>>>>> disabled and the frame number has not yet done so.
>>>>>>>>
>>>>>>>> Instead, wait until the frame number also wraps and then clear
>>>>>>>> the overrun flag.
>>>>>>>>
>>>>>>>> Signed-off-by: John Keeping
>>>>>>>> ---
>>>>>>>> drivers/usb/dwc2/gadget.c | 2 +-
>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/usb/dwc2/gadget.c
>>>>>>>> b/drivers/usb/dwc2/gadget.c index 2d6d2c8244de..8da2c052dfa1
>>>>>>>> 100644 --- a/drivers/usb/dwc2/gadget.c
>>>>>>>> +++ b/drivers/usb/dwc2/gadget.c
>>>>>>>> @@ -117,7 +117,7 @@ static inline void
>>>>>>>> dwc2_gadget_incr_frame_num(struct dwc2_hsotg_ep *hs_ep) if
>>>>>>>> (hs_ep->target_frame > DSTS_SOFFN_LIMIT)
>>>>>>>> { hs_ep->frame_overrun = true; hs_ep->target_frame &=
>>>>>>>> DSTS_SOFFN_LIMIT;
>>>>>>>> - } else {
>>>>>>>> + } else if (hs_ep->parent->frame_number <
>>>>>>>> hs_ep->target_frame) { hs_ep->frame_overrun = false;
>>>>>>>>}
>>>>>>>> }
>>>>>>>>
>>>>>>> Did you tested mentioned by you scenario? If you see issue can
>>>>>>> you provide debug log and point the issue line in the log.
>>>>>>
>>>>>> It only reproduces very occasionally so it's difficult to capture
>>>>>> a full debug log containing the error.
>>>>>>
>>>>>> I applied this patch to capture logging specifically around this
>>>>>> scenario:
>>>>>>
>>>>>> -- >8 --
>>>>>> diff --git a/drivers/usb/dwc2/gadget.c
>>>>>> b/drivers/usb/dwc2/gadget.c index 220c0f9b89b0..3770b9d3b523
>>>>>> 100644 --- a/drivers/usb/dwc2/gadget.c
>>>>>> +++ b/drivers/usb/dwc2/gadget.c
>>>>>> @@ -2722,13 +2722,20 @@ static void
>>>>>> dwc2_gadget_handle_ep_disabled(struct dwc2_hsotg_ep *hs_ep) }
>>>>>>
>>>>>> do {
>>>>>> + unsigned int target_frame = hs_ep->target_frame;
>>>>>> + bool frame_overrun = hs_ep->frame_overrun;
>>>>>> +
>>>>>> hs_req = get_ep_head(hs_ep);
>>>>>> if (hs_req)
>>>>>> dwc2_hsotg_complete_request(hsotg,
>>>>>> hs_ep, hs_req, -ENODATA);
>>>>>> +
>>>>>> dwc2_gadget_incr_frame_num(hs_ep);
>>>>>> /* Update current frame number value. */
>>>>>> hsotg->frame_number =
>>>>>> dwc2_hsotg_read_frameno(hsotg); +
>>>>>> + dev_warn(hsotg->dev, "%s: expiring request
>>>>>> frame_number=0x%04x target_frame=0x%04x overrun=%u\n",
>>>>>> +__func__, hsotg-&g
Re: [PATCH] usb: dwc2: gadget: fix ISOC frame overflow handling
Hi John,
On 12/21/2018 8:05 PM, John Keeping wrote:
> Hi Minas,
>
> On Wed, 19 Dec 2018 14:09:01 +0000
> Minas Harutyunyan wrote:
>
>> On 12/18/2018 6:35 PM, John Keeping wrote:
>>> Hi Minas,
>>>
>>> On Fri, 14 Dec 2018 09:00:08 +
>>> Minas Harutyunyan wrote:
>>>> First of all, sorry for delayed answer.
>>>> Looks like similar issue seen by Andrzej Pietrasiewicz
>>>> : "dwc2 isochronous transfers issues". Same
>>>> feedback provided to Andrzej.
>>>>
>>>> I run tests on 4.20.0-rc4 in DDMA. By default IN ISOC traffic
>>>> failed because of BNA interrupts. It's happen because UAC2
>>>> requests count set by default to 2. Our core and driver can't work
>>>> in DDMA with descriptor list length equal to 2. It's not possible
>>>> on time prepare next descriptors to avoid BNA interrupt.
>>>>
>>>> By changing UAC2_DEF_REQ_NUM to 4 all audio gadget tests passed
>>>> smoothly. Could you please apply this patch and run tests in DDMA
>>>> mode:
>>>>
>>>> diff --git a/drivers/usb/gadget/function/u_uac2.h
>>>> b/drivers/usb/gadget/function/u_uac2.h
>>>> index 8362ee572e1e..5e649259ab76 100644
>>>> --- a/drivers/usb/gadget/function/u_uac2.h
>>>> +++ b/drivers/usb/gadget/function/u_uac2.h
>>>> @@ -21,7 +21,7 @@
>>>> #define UAC2_DEF_CCHMASK 0x3
>>>> #define UAC2_DEF_CSRATE 64000
>>>> #define UAC2_DEF_CSSIZE 2
>>>> -#define UAC2_DEF_REQ_NUM 2
>>>> +#define UAC2_DEF_REQ_NUM 4
>>>>
>>>> struct f_uac2_opts {
>>>>struct usb_function_instancefunc_inst;
>>>>
>>>>
>>>> If it will OK on your side also then will switch to BDMA mode and
>>>> debug.
>>>
>>> With DDMA enabled, I see the following error after the stream has
>>> been running for a while (anything from a few seconds to a few
>>> minutes):
>>> -- >8 --
>>> [ 1798.836322] dwc2 ff58.usb: dwc2_hsotg_ep_disable: called for
>>> ep0 [ 1798.836329] dwc2 ff58.usb: dwc2_hsotg_ep_disable: called
>>> for ep0 [ 1798.851092] dwc2 ff58.usb: new device is high-speed
>>> [ 1798.924461] dwc2 ff58.usb: new device is high-speed
>>> [ 1798.982887] dwc2 ff58.usb: new address 25
>>> [ 1799.002463] configfs-gadget gadget: high-speed config #1: config
>>> -- 8< --
>>>
>>> On the host side (Linux 4.18.16 Intel xHCI), I see this logged at
>>> the same time:
>>>
>>> -- >8 --
>>> [1735740.716242] retire_capture_urb: usb 1-2.2.7: frame 0 active:
>>> -71 [1735740.716990] retire_capture_urb: usb 1-2.2.7: frame 0
>>> active: -71 [1735740.717906] retire_capture_urb: usb 1-2.2.7: frame
>>> 0 active: -71 [1735740.718905] retire_capture_urb: usb 1-2.2.7:
>>> frame 0 active: -71 [1735740.719916] retire_capture_urb: usb
>>> 1-2.2.7: frame 0 active: -71 [1735740.720032] usb 1-2.2-port7:
>>> disabled by hub (EMI?), re-enabling... [1735740.720036] usb
>>> 1-2.2.7: USB disconnect, device number 28 [1735740.937500] usb
>>> 1-2.2.7: new high-speed USB device number 29 using xhci_hcd -- 8< --
>>>
>>> The device does then enumerate and works for a period of time
>>> before the same error happens again.
>>>
>> From logs not clear who initiate disconnect: host or device. More
>> probably host, after some errors in retire_capture_urb initiate
>> disconnect. Do you have any idea?
>> Can't you connect device to host on same platform?
>> If root cause of disconnect by host is device issue, i.e. not able to
>> send to host data packets in time then I need device side dmesg log
>> with debug enabled. USB trace around the disconnect will help to
>> debug.
>
> I remember running a test with a hardware USB analyzer and which showed
> an isochronous packet with an incorrect length (much larger than it
> should have been) when the failure occurred.
>
> I don't have any logs from that test and I'm out of the office at the
> moment, but I will capture logs when I'm back in January.
I think, if you enable debug prints, you will see BNA interrupts. So, my
recommendation is to increase more UAC2_DEF_REQ_NUM until BNA will
disappear. Per me value of UAC2_DEF_REQ_NUM is platform's latency dependent.
Thanks,
Minas
>
>
> Thanks,
> John
>
Re: [PATCH v3 05/14] usb: dwc2: Update port suspend/resume function definitions.
On 4/8/2021 1:45 PM, Artur Petrosyan wrote:
> Earlier "dwc2_port_suspend()" and "dwc2_port_resume()" functions
> were implemented without proper description and host or device mode
> difference.
>
> - Added "dwc2_port_suspend" and "dwc2_port_resume" functions to
>"core.h" header file.
>
> - Updated function description in documentation.
>
> Signed-off-by: Artur Petrosyan
Acked-by: Minas Harutyunyan
> ---
> Changes in v3:
> - None
> Changes in v2:
> - None
>
> drivers/usb/dwc2/core.h | 4
> drivers/usb/dwc2/hcd.c | 25 +++--
> 2 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 39037709a2ad..b7d99cf9e84c 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -1470,6 +1470,8 @@ void dwc2_hcd_connect(struct dwc2_hsotg *hsotg);
> void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force);
> void dwc2_hcd_start(struct dwc2_hsotg *hsotg);
> int dwc2_core_init(struct dwc2_hsotg *hsotg, bool initial_setup);
> +void dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex);
> +void dwc2_port_resume(struct dwc2_hsotg *hsotg);
> int dwc2_backup_host_registers(struct dwc2_hsotg *hsotg);
> int dwc2_restore_host_registers(struct dwc2_hsotg *hsotg);
> int dwc2_host_enter_hibernation(struct dwc2_hsotg *hsotg);
> @@ -1493,6 +1495,8 @@ static inline void dwc2_hcd_start(struct dwc2_hsotg
> *hsotg) {}
> static inline void dwc2_hcd_remove(struct dwc2_hsotg *hsotg) {}
> static inline int dwc2_core_init(struct dwc2_hsotg *hsotg, bool
> initial_setup)
> { return 0; }
> +static inline void dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex) {}
> +static inline void dwc2_port_resume(struct dwc2_hsotg *hsotg) {}
> static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg)
> { return 0; }
> static inline int dwc2_backup_host_registers(struct dwc2_hsotg *hsotg)
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index dd0362e07444..f4247a66c2b2 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -56,8 +56,6 @@
> #include "core.h"
> #include "hcd.h"
>
> -static void dwc2_port_resume(struct dwc2_hsotg *hsotg);
> -
> /*
>* =
>* Host Core Layer Functions
> @@ -3277,8 +3275,16 @@ static int dwc2_host_is_b_hnp_enabled(struct
> dwc2_hsotg *hsotg)
> return hcd->self.b_hnp_enable;
> }
>
> -/* Must NOT be called with interrupt disabled or spinlock held */
> -static void dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex)
> +/**
> + * dwc2_port_suspend() - Put controller in suspend mode for host.
> + *
> + * @hsotg: Programming view of the DWC_otg controller
> + * @windex: The control request wIndex field
> + *
> + * This function is for entering Host mode suspend.
> + * Must NOT be called with interrupt disabled or spinlock held.
> + */
> +void dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex)
> {
> unsigned long flags;
> u32 hprt0;
> @@ -3328,8 +3334,15 @@ static void dwc2_port_suspend(struct dwc2_hsotg
> *hsotg, u16 windex)
> }
> }
>
> -/* Must NOT be called with interrupt disabled or spinlock held */
> -static void dwc2_port_resume(struct dwc2_hsotg *hsotg)
> +/**
> + * dwc2_port_resume() - Exit controller from suspend mode for host.
> + *
> + * @hsotg: Programming view of the DWC_otg controller
> + *
> + * This function is for exiting Host mode suspend.
> + * Must NOT be called with interrupt disabled or spinlock held.
> + */
> +void dwc2_port_resume(struct dwc2_hsotg *hsotg)
> {
> unsigned long flags;
> u32 hprt0;
>
Re: [PATCH v3 06/14] usb: dwc2: Add enter partial power down when port is suspended
On 4/8/2021 1:45 PM, Artur Petrosyan wrote:
> Adds flow of entering Partial Power Down in
> "dwc2_port_suspend()" function when core receives suspend.
>
> NOTE: Switch case statement is used for hibernation partial
> power down and clock gating mode determination. In this patch
> only Partial Power Down is implemented the Hibernation and
> clock gating implementations are planned to be added.
>
> Signed-off-by: Artur Petrosyan
Acked-by: Minas Harutyunyan
> ---
> Changes in v3:
> - None
> Changes in v2:
> - None
>
> drivers/usb/dwc2/core.h | 5 +++--
> drivers/usb/dwc2/hcd.c | 48 ++---
> 2 files changed, 34 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index b7d99cf9e84c..76807abd753b 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -1470,7 +1470,7 @@ void dwc2_hcd_connect(struct dwc2_hsotg *hsotg);
> void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force);
> void dwc2_hcd_start(struct dwc2_hsotg *hsotg);
> int dwc2_core_init(struct dwc2_hsotg *hsotg, bool initial_setup);
> -void dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex);
> +int dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex);
> void dwc2_port_resume(struct dwc2_hsotg *hsotg);
> int dwc2_backup_host_registers(struct dwc2_hsotg *hsotg);
> int dwc2_restore_host_registers(struct dwc2_hsotg *hsotg);
> @@ -1495,7 +1495,8 @@ static inline void dwc2_hcd_start(struct dwc2_hsotg
> *hsotg) {}
> static inline void dwc2_hcd_remove(struct dwc2_hsotg *hsotg) {}
> static inline int dwc2_core_init(struct dwc2_hsotg *hsotg, bool
> initial_setup)
> { return 0; }
> -static inline void dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex) {}
> +static inline int dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex)
> +{ return 0; }
> static inline void dwc2_port_resume(struct dwc2_hsotg *hsotg) {}
> static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg)
> { return 0; }
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index f4247a66c2b2..e7fb0d5940bc 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -3281,15 +3281,18 @@ static int dwc2_host_is_b_hnp_enabled(struct
> dwc2_hsotg *hsotg)
>* @hsotg: Programming view of the DWC_otg controller
>* @windex: The control request wIndex field
>*
> + * Return: non-zero if failed to enter suspend mode for host.
> + *
>* This function is for entering Host mode suspend.
>* Must NOT be called with interrupt disabled or spinlock held.
>*/
> -void dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex)
> +int dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex)
> {
> unsigned long flags;
> u32 hprt0;
> u32 pcgctl;
> u32 gotgctl;
> + int ret = 0;
>
> dev_dbg(hsotg->dev, "%s()\n", __func__);
>
> @@ -3302,22 +3305,31 @@ void dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16
> windex)
> hsotg->op_state = OTG_STATE_A_SUSPEND;
> }
>
> - hprt0 = dwc2_read_hprt0(hsotg);
> - hprt0 |= HPRT0_SUSP;
> - dwc2_writel(hsotg, hprt0, HPRT0);
> -
> - hsotg->bus_suspended = true;
> -
> - /*
> - * If power_down is supported, Phy clock will be suspended
> - * after registers are backuped.
> - */
> - if (!hsotg->params.power_down) {
> - /* Suspend the Phy Clock */
> - pcgctl = dwc2_readl(hsotg, PCGCTL);
> - pcgctl |= PCGCTL_STOPPCLK;
> - dwc2_writel(hsotg, pcgctl, PCGCTL);
> - udelay(10);
> + switch (hsotg->params.power_down) {
> + case DWC2_POWER_DOWN_PARAM_PARTIAL:
> + ret = dwc2_enter_partial_power_down(hsotg);
> + if (ret)
> + dev_err(hsotg->dev,
> + "enter partial_power_down failed.\n");
> + break;
> + case DWC2_POWER_DOWN_PARAM_HIBERNATION:
> + case DWC2_POWER_DOWN_PARAM_NONE:
> + default:
> + hprt0 = dwc2_read_hprt0(hsotg);
> + hprt0 |= HPRT0_SUSP;
> + dwc2_writel(hsotg, hprt0, HPRT0);
> + hsotg->bus_suspended = true;
> + /*
> + * If power_down is supported, Phy clock will be suspended
> + * after registers are backuped.
> + */
> + if (!hsotg->params.power_down) {
> + /* Suspend the Phy Clock */
> + pcgctl = dwc2_readl(hsotg, PCGCTL);
> + pcgctl |= PCGCTL_STOPPCLK;
> + dwc2_writel(hsotg, pcgctl, PCGCTL);
> + udelay(10);
> + }
> }
>
> /* For HNP the bus must be suspended for at least 200ms */
> @@ -3332,6 +3344,8 @@ void dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16
> windex)
> } else {
> spin_unlock_irqrestore(&hsotg->lock, flags);
> }
> +
> + return ret;
> }
>
> /**
>
Re: [PATCH v3 08/14] usb: dwc2: Add exit partial power down when port reset is asserted
On 4/8/2021 1:45 PM, Artur Petrosyan wrote:
> Adds Partial Power Down exiting flow when set port feature
> reset is received in suspended state.
>
> Signed-off-by: Artur Petrosyan
Acked-by: Minas Harutyunyan
> ---
> Changes in v3:
> - None
> Changes in v2:
> - None
>
> drivers/usb/dwc2/hcd.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 720354df014b..7c7496719152 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -3694,6 +3694,15 @@ static int dwc2_hcd_hub_control(struct dwc2_hsotg
> *hsotg, u16 typereq,
> if (hsotg->params.power_down ==
> DWC2_POWER_DOWN_PARAM_HIBERNATION &&
> hsotg->hibernated)
> dwc2_exit_hibernation(hsotg, 0, 1, 1);
> +
> + if (hsotg->in_ppd) {
> + retval = dwc2_exit_partial_power_down(hsotg, 1,
> + true);
> + if (retval)
> + dev_err(hsotg->dev,
> + "exit partial_power_down
> failed\n");
> + }
> +
> hprt0 = dwc2_read_hprt0(hsotg);
> dev_dbg(hsotg->dev,
> "SetPortFeature - USB_PORT_FEAT_RESET\n");
>
Re: [PATCH v3 07/14] usb: dwc2: Add exit partial power down when port is resumed
On 4/8/2021 1:45 PM, Artur Petrosyan wrote:
> Added flow of exiting Partial Power Down in
> "dwc2_port_resume()" function when core receives resume.
>
> NOTE: Switch case statement is used for hibernation partial
> power down and clock gating mode determination. In this patch
> only Partial Power Down is implemented the Hibernation and
> clock gating implementations are planned to be added.
>
> Signed-off-by: Artur Petrosyan
Acked-by: Minas Harutyunyan
> ---
> Changes in v3:
> - None
> Changes in v2:
> - None
>
> drivers/usb/dwc2/core.h | 5 ++--
> drivers/usb/dwc2/hcd.c | 61 ++---
> 2 files changed, 42 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 76807abd753b..5a7850482e57 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -1471,7 +1471,7 @@ void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool
> force);
> void dwc2_hcd_start(struct dwc2_hsotg *hsotg);
> int dwc2_core_init(struct dwc2_hsotg *hsotg, bool initial_setup);
> int dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex);
> -void dwc2_port_resume(struct dwc2_hsotg *hsotg);
> +int dwc2_port_resume(struct dwc2_hsotg *hsotg);
> int dwc2_backup_host_registers(struct dwc2_hsotg *hsotg);
> int dwc2_restore_host_registers(struct dwc2_hsotg *hsotg);
> int dwc2_host_enter_hibernation(struct dwc2_hsotg *hsotg);
> @@ -1497,7 +1497,8 @@ static inline int dwc2_core_init(struct dwc2_hsotg
> *hsotg, bool initial_setup)
> { return 0; }
> static inline int dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex)
> { return 0; }
> -static inline void dwc2_port_resume(struct dwc2_hsotg *hsotg) {}
> +static inline int dwc2_port_resume(struct dwc2_hsotg *hsotg)
> +{ return 0; }
> static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg)
> { return 0; }
> static inline int dwc2_backup_host_registers(struct dwc2_hsotg *hsotg)
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index e7fb0d5940bc..720354df014b 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -3353,44 +3353,61 @@ int dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16
> windex)
>*
>* @hsotg: Programming view of the DWC_otg controller
>*
> + * Return: non-zero if failed to exit suspend mode for host.
> + *
>* This function is for exiting Host mode suspend.
>* Must NOT be called with interrupt disabled or spinlock held.
>*/
> -void dwc2_port_resume(struct dwc2_hsotg *hsotg)
> +int dwc2_port_resume(struct dwc2_hsotg *hsotg)
> {
> unsigned long flags;
> u32 hprt0;
> u32 pcgctl;
> + int ret = 0;
>
> spin_lock_irqsave(&hsotg->lock, flags);
>
> - /*
> - * If power_down is supported, Phy clock is already resumed
> - * after registers restore.
> - */
> - if (!hsotg->params.power_down) {
> - pcgctl = dwc2_readl(hsotg, PCGCTL);
> - pcgctl &= ~PCGCTL_STOPPCLK;
> - dwc2_writel(hsotg, pcgctl, PCGCTL);
> + switch (hsotg->params.power_down) {
> + case DWC2_POWER_DOWN_PARAM_PARTIAL:
> + ret = dwc2_exit_partial_power_down(hsotg, 0, true);
> + if (ret)
> + dev_err(hsotg->dev,
> + "exit partial_power_down failed.\n");
> + break;
> + case DWC2_POWER_DOWN_PARAM_HIBERNATION:
> + case DWC2_POWER_DOWN_PARAM_NONE:
> + default:
> + /*
> + * If power_down is supported, Phy clock is already resumed
> + * after registers restore.
> + */
> + if (!hsotg->params.power_down) {
> + pcgctl = dwc2_readl(hsotg, PCGCTL);
> + pcgctl &= ~PCGCTL_STOPPCLK;
> + dwc2_writel(hsotg, pcgctl, PCGCTL);
> + spin_unlock_irqrestore(&hsotg->lock, flags);
> + msleep(20);
> + spin_lock_irqsave(&hsotg->lock, flags);
> + }
> +
> + hprt0 = dwc2_read_hprt0(hsotg);
> + hprt0 |= HPRT0_RES;
> + hprt0 &= ~HPRT0_SUSP;
> + dwc2_writel(hsotg, hprt0, HPRT0);
> spin_unlock_irqrestore(&hsotg->lock, flags);
> - msleep(20);
> +
> + msleep(USB_RESUME_TIMEOUT);
> +
> spin_lock_irqsave(&hsotg->lock, flags);
> + hprt0 = dwc2_read_hprt0(hsotg);
> + hprt0 &= ~(HPRT0_RES | HPRT0_SUSP);
> + dwc2_writel(hsotg, hprt0, HPRT
Re: [PATCH v3 10/14] usb: dwc2: Allow exit partial power down in urb enqueue
On 4/8/2021 1:45 PM, Artur Petrosyan wrote:
> When core is in partial power down state and an external
> hub is connected, upper layer sends URB enqueue request,
> which results in port reset issue.
>
> Added exit from partial power down state to avoid port
> reset issue and process upper layer request correctly.
>
> Signed-off-by: Artur Petrosyan
Acked-by: Minas Harutyunyan
> ---
> Changes in v3:
> - None
> Changes in v2:
> - None
>
> drivers/usb/dwc2/hcd.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 9529e9839961..cb52bc41bfb8 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -4633,6 +4633,13 @@ static int _dwc2_hcd_urb_enqueue(struct usb_hcd *hcd,
> struct urb *urb,
> dwc2_dump_urb_info(hcd, urb, "urb_enqueue");
> }
>
> + if (hsotg->in_ppd) {
> + retval = dwc2_exit_partial_power_down(hsotg, 0, true);
> + if (retval)
> + dev_err(hsotg->dev,
> + "exit partial_power_down failed\n");
> + }
> +
> if (!ep)
> return -EINVAL;
>
>
Re: [PATCH v3 12/14] usb: dwc2: Update partial power down entering by system suspend
On 4/8/2021 1:45 PM, Artur Petrosyan wrote:
> With current implementation the port power is being disabled,
> which is not required by the programming guide. Also, if there
> is a system which works only in "DWC2_POWER_DOWN_PARAM_NONE"
> (clock gating) mode the current implementation does not set
> Gate hclk bit in pcgctl register.
>
> Rearranges and updates the implementation of entering to partial
> power down power saving mode when PC is suspended to get
> rid of many "if" statements and removes disabling of port power.
>
> NOTE: Switch case statement is used for hibernation partial
> power down and clock gating mode determination. In this patch
> only Partial Power Down is implemented the Hibernation and
> clock gating implementations are planned to be added.
>
> Signed-off-by: Artur Petrosyan
Acked-by: Minas Harutyunyan
> ---
> Changes in v3:
> - None
> Changes in v2:
> - None
>
> drivers/usb/dwc2/hcd.c | 53 ++
> 1 file changed, 18 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index cb52bc41bfb8..34030bafdff4 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -4367,8 +4367,6 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
> struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
> unsigned long flags;
> int ret = 0;
> - u32 hprt0;
> - u32 pcgctl;
>
> spin_lock_irqsave(&hsotg->lock, flags);
>
> @@ -4384,47 +4382,32 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
> if (hsotg->op_state == OTG_STATE_B_PERIPHERAL)
> goto unlock;
>
> - if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL ||
> - hsotg->flags.b.port_connect_status == 0)
> + if (hsotg->bus_suspended)
> goto skip_power_saving;
>
> - /*
> - * Drive USB suspend and disable port Power
> - * if usb bus is not suspended.
> - */
> - if (!hsotg->bus_suspended) {
> - hprt0 = dwc2_read_hprt0(hsotg);
> - if (hprt0 & HPRT0_CONNSTS) {
> - hprt0 |= HPRT0_SUSP;
> - if (hsotg->params.power_down ==
> DWC2_POWER_DOWN_PARAM_PARTIAL)
> - hprt0 &= ~HPRT0_PWR;
> - dwc2_writel(hsotg, hprt0, HPRT0);
> - }
> - if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
> - spin_unlock_irqrestore(&hsotg->lock, flags);
> - dwc2_vbus_supply_exit(hsotg);
> - spin_lock_irqsave(&hsotg->lock, flags);
> - } else {
> - pcgctl = readl(hsotg->regs + PCGCTL);
> - pcgctl |= PCGCTL_STOPPCLK;
> - writel(pcgctl, hsotg->regs + PCGCTL);
> - }
> - }
> + if (hsotg->flags.b.port_connect_status == 0)
> + goto skip_power_saving;
>
> - if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
> + switch (hsotg->params.power_down) {
> + case DWC2_POWER_DOWN_PARAM_PARTIAL:
> /* Enter partial_power_down */
> ret = dwc2_enter_partial_power_down(hsotg);
> - if (ret) {
> - if (ret != -ENOTSUPP)
> - dev_err(hsotg->dev,
> - "enter partial_power_down failed\n");
> - goto skip_power_saving;
> - }
> -
> - /* After entering partial_power_down, hardware is no more
> accessible */
> + if (ret)
> + dev_err(hsotg->dev,
> + "enter partial_power_down failed\n");
> + /* After entering suspend, hardware is not accessible */
> clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> + break;
> + case DWC2_POWER_DOWN_PARAM_HIBERNATION:
> + case DWC2_POWER_DOWN_PARAM_NONE:
> + default:
> + goto skip_power_saving;
> }
>
> + spin_unlock_irqrestore(&hsotg->lock, flags);
> + dwc2_vbus_supply_exit(hsotg);
> + spin_lock_irqsave(&hsotg->lock, flags);
> +
> /* Ask phy to be suspended */
> if (!IS_ERR_OR_NULL(hsotg->uphy)) {
> spin_unlock_irqrestore(&hsotg->lock, flags);
>
Re: [PATCH v3 13/14] usb: dwc2: Fix partial power down exiting by system resume
On 4/8/2021 1:46 PM, Artur Petrosyan wrote:
> Fixes the implementation of exiting from partial power down
> power saving mode when PC is resumed.
>
> Added port connection status checking which prevents exiting from
> Partial Power Down mode from _dwc2_hcd_resume() if not in Partial
> Power Down mode.
>
> Rearranged the implementation to get rid of many "if"
> statements.
>
> NOTE: Switch case statement is used for hibernation partial
> power down and clock gating mode determination. In this patch
> only Partial Power Down is implemented the Hibernation and
> clock gating implementations are planned to be added.
>
> Cc:
> Fixes: 6f6d70597c15 ("usb: dwc2: bus suspend/resume for hosts with
> DWC2_POWER_DOWN_PARAM_NONE")
> Signed-off-by: Artur Petrosyan
Acked-by: Minas Harutyunyan
> ---
> Changes in v3:
> - None
> Changes in v2:
> - None
>
> drivers/usb/dwc2/hcd.c | 90 +-
> 1 file changed, 46 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 34030bafdff4..f096006df96f 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -4427,7 +4427,7 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
> {
> struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
> unsigned long flags;
> - u32 pcgctl;
> + u32 hprt0;
> int ret = 0;
>
> spin_lock_irqsave(&hsotg->lock, flags);
> @@ -4438,11 +4438,40 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
> if (hsotg->lx_state != DWC2_L2)
> goto unlock;
>
> - if (hsotg->params.power_down > DWC2_POWER_DOWN_PARAM_PARTIAL) {
> + hprt0 = dwc2_read_hprt0(hsotg);
> +
> + /*
> + * Added port connection status checking which prevents exiting from
> + * Partial Power Down mode from _dwc2_hcd_resume() if not in Partial
> + * Power Down mode.
> + */
> + if (hprt0 & HPRT0_CONNSTS) {
> + hsotg->lx_state = DWC2_L0;
> + goto unlock;
> + }
> +
> + switch (hsotg->params.power_down) {
> + case DWC2_POWER_DOWN_PARAM_PARTIAL:
> + ret = dwc2_exit_partial_power_down(hsotg, 0, true);
> + if (ret)
> + dev_err(hsotg->dev,
> + "exit partial_power_down failed\n");
> + /*
> + * Set HW accessible bit before powering on the controller
> + * since an interrupt may rise.
> + */
> + set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> + break;
> + case DWC2_POWER_DOWN_PARAM_HIBERNATION:
> + case DWC2_POWER_DOWN_PARAM_NONE:
> + default:
> hsotg->lx_state = DWC2_L0;
> goto unlock;
> }
>
> + /* Change Root port status, as port status change occurred after
> resume.*/
> + hsotg->flags.b.port_suspend_change = 1;
> +
> /*
>* Enable power if not already done.
>* This must not be spinlocked since duration
> @@ -4454,52 +4483,25 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
> spin_lock_irqsave(&hsotg->lock, flags);
> }
>
> - if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
> - /*
> - * Set HW accessible bit before powering on the controller
> - * since an interrupt may rise.
> - */
> - set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> -
> -
> - /* Exit partial_power_down */
> - ret = dwc2_exit_partial_power_down(hsotg, 0, true);
> - if (ret && (ret != -ENOTSUPP))
> - dev_err(hsotg->dev, "exit partial_power_down failed\n");
> - } else {
> - pcgctl = readl(hsotg->regs + PCGCTL);
> - pcgctl &= ~PCGCTL_STOPPCLK;
> - writel(pcgctl, hsotg->regs + PCGCTL);
> - }
> -
> - hsotg->lx_state = DWC2_L0;
> -
> + /* Enable external vbus supply after resuming the port. */
> spin_unlock_irqrestore(&hsotg->lock, flags);
> + dwc2_vbus_supply_init(hsotg);
>
> - if (hsotg->bus_suspended) {
> - spin_lock_irqsave(&hsotg->lock, flags);
> - hsotg->flags.b.port_suspend_change = 1;
> - spin_unlock_irqrestore(&hsotg->lock, flags);
> - dwc2_port_resume(hsotg);
> - } else {
> - if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
> - dwc2_vbus_
Re: [PATCH v3 14/14] usb: dwc2: Add exit partial power down before removing driver
On 4/8/2021 1:46 PM, Artur Petrosyan wrote:
> When dwc2 core is in partial power down mode
> loading driver again causes driver fail. Because in
> that mode registers are not accessible.
>
> Added a flow of exiting the partial power down mode
> to avoid the driver reload failure.
>
> Signed-off-by: Artur Petrosyan
Acked-by: Minas Harutyunyan
> ---
> Changes in v3:
> - None
> Changes in v2:
> - None
>
> drivers/usb/dwc2/platform.c | 11 ++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index 5f18acac7406..b28b8cd45799 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -316,6 +316,15 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg
> *hsotg)
> static int dwc2_driver_remove(struct platform_device *dev)
> {
> struct dwc2_hsotg *hsotg = platform_get_drvdata(dev);
> + int ret = 0;
> +
> + /* Exit Partial Power Down when driver is removed. */
> + if (hsotg->in_ppd) {
> + ret = dwc2_exit_partial_power_down(hsotg, 0, true);
> + if (ret)
> + dev_err(hsotg->dev,
> + "exit partial_power_down failed\n");
> + }
>
> dwc2_debugfs_exit(hsotg);
> if (hsotg->hcd_enabled)
> @@ -334,7 +343,7 @@ static int dwc2_driver_remove(struct platform_device *dev)
> reset_control_assert(hsotg->reset);
> reset_control_assert(hsotg->reset_ecc);
>
> - return 0;
> + return ret;
> }
>
> /**
>
Re: [PATCH] usb: dwc2: Fix HiKey regression caused by power_down feature
Hi John,
Please provide log with debug enabled configuration.
On 5/21/2018 11:41 PM, John Stultz wrote:
> On Mon, May 21, 2018 at 1:45 AM, Minas Harutyunyan
> wrote:
>> Hi John,
>>
>> On 5/19/2018 4:49 AM, John Stultz wrote:
>>> In 4.17-rc, commit 03ea6d6e9e1f ("usb: dwc2: Enable power down")
>>> caused the HiKey board to not correctly handle switching between
>>> usb-gadget and usb-host mode.
>>>
>>> Unplugging the OTG port would result in:
OTG port you mean MicroAB, Correct?
dwc2 driver loaded when some device connected to OTG port?
And below message printed after disconnect the device from OTG port?
>>> [ 42.240973] dwc2 f72c.usb: dwc2_restore_host_registers: no host
>>> registers to restore
>>> [ 42.249066] dwc2 f72c.usb: dwc2_host_exit_hibernation: failed to
>>> restore host registers
>>>
>>> And the USB-host ports would not function.
USB-host ports - you mean 2 USB A-ports, connected to TS3USB221 HUB?
Switching ports between OTG and Host ports via TS3USB221 Switch
performing automatically or by some SW tool?
>>>
>>> And plugging in the OTG port, we would see:
>>> [ 46.046557] WARNING: CPU: 3 PID: 6 at drivers/usb/dwc2/gadget.c:260
>>> dwc2_hsotg_init_fifo+0x194/0x1a0
>>> [ 46.055761] CPU: 3 PID: 6 Comm: kworker/u16:0 Not tainted
>>> 4.17.0-rc5-00030-ge67da8c #231
>>> [ 46.055767] Hardware name: HiKey Development Board (DT)
>>> [ 46.055784] Workqueue: dwc2 dwc2_conn_id_status_change
>>> ...
>>>
>> Could you please send full log to debug.
>
> Full dmesg log attached.
>
> I unplugged the usb-otg port at 136
> and replugged it back in at 141
>
>
>>>p->uframe_sched = false;
>>>p->change_speed_quirk = true;
>>> + p->power_down = false;
>>
>> power_down declared as int, suggested to update as follow:
>> p->power_down = DWC2_POWER_DOWN_PARAM_NONE;
>>
>> This can be accepted as temporary solution until we will fully debug
>> hibernation feature for HiKey platform.
>
> Ok, will re-send with the suggested change above.
>
> thanks
> -john
>
Re: Possible usb_request leak in the function dwc2_gadget_complete_isoc_request_ddma
Hi, On 2/28/2018 1:00 PM, Zengtao (B) wrote: > Hi johnyoun: > > I found a suspected bug, and I am writing to confirm with you. > > In the function > dwc2_gadget_complete_isoc_request_ddma(drivers/usb/dwc2/gadget.c). > Only the first request from the eq queue is processed while maybe there are > more than one descriptors done by the HW. > > 1. Each usb request is associated with a DMA descriptor, but this is not > reflect in the driver, so when one DMA descriptor is done, > we don't know which usb request is done, but I think if only one DMA > descriptor is done, we can know that the first USB request in > eq queue is done, because the HW DMA descriptor and SW usb request are both > in sequence. > > 2. In the function dwc2_gadget_complete_isoc_request_ddma, we may complete > more than one DMA descriptor but only the first > Usb request is processed, but in fact, we should all the usb requests > associated with all the done DMA descriptors. > > 3. I noticed that each DMA descriptor is configured to report an interrupt, > and if each DMA descriptor generate an interrupt, the above > Flow should be ok, but the interrupts can merge and we have used the depdma > to figure out the largest finished DMA descriptor index. > Why you suspect that subsequent interrupts can be merged? Did you see this case? Can you provide a log? Even in case of minimal interval=1, time between 2 subsequent interrupts should be about 125us. It's fully enough to process target descriptor, complete request, enqueue new request and prepare new descriptor. Thanks, Minas > Looking forward your reply. > > Thank you. > > Regards > Zengtao > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majord...@vger.kernel.org > More majordomo info at > https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=6z9Al9FrHR_ZqbbtSAsD16pvOL2S3XHxQnSzq8kusyI&m=vbKt9jwY5FRUysFlZg1CB6HKRyFACygkwZBO0mvSQDc&s=7rLKr3g3UyOZT22GAer-w5wDtv-Bb5awlncAJQ1OICM&e= >
Re: [PATCH v4] usb: dwc2: dwc2_vbus_supply_init: fix error check
Hi Tomeu,
On 3/26/2018 1:01 PM, Tomeu Vizoso wrote:
> devm_regulator_get_optional returns -ENODEV if the regulator isn't
> there, so if that's the case we have to make sure not to leave -ENODEV
> in the regulator pointer.
>
> Also, make sure we return 0 in that case, but correctly propagate any
> other errors. Also propagate the error from _dwc2_hcd_start.
>
> Fixes: 531ef5ebea96 ("usb: dwc2: add support for host mode external vbus
> supply")
> Cc: Amelie Delaunay
> Signed-off-by: Tomeu Vizoso
>
> ---
>
> v2: Only overwrite the error in the pointer after checking it (Heiko
> Stübner )
> v3: Remove hunks that shouldn't be in this patch
> v4: Don't overwrite the error code before returning it (kbuild test
> robot )
> ---
> drivers/usb/dwc2/hcd.c | 13 -
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 190f95964000..c51b73b3e048 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -358,9 +358,14 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg)
>
> static int dwc2_vbus_supply_init(struct dwc2_hsotg *hsotg)
> {
> + int ret;
> +
> hsotg->vbus_supply = devm_regulator_get_optional(hsotg->dev, "vbus");
> - if (IS_ERR(hsotg->vbus_supply))
> - return 0;
> + if (IS_ERR(hsotg->vbus_supply)) {
> + ret = PTR_ERR(hsotg->vbus_supply);
> + hsotg->vbus_supply = NULL;
> + return ret == -ENODEV ? 0 : ret;
> + }
>
> return regulator_enable(hsotg->vbus_supply);
> }
> @@ -4342,9 +4347,7 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)
>
> spin_unlock_irqrestore(&hsotg->lock, flags);
>
> - dwc2_vbus_supply_init(hsotg);
> -
> - return 0;
> + return dwc2_vbus_supply_init(hsotg);
> }
>
> /*
>
Not able to apply patch. Please rebase to balbi/next
Thanks,
Minas
Re: [PATCH v4] usb: dwc2: dwc2_vbus_supply_init: fix error check
On 4/5/2018 12:59 PM, Grigor Tovmasyan wrote:
> On 3/26/2018 1:01 PM, Tomeu Vizoso wrote:
>> devm_regulator_get_optional returns -ENODEV if the regulator isn't
>> there, so if that's the case we have to make sure not to leave -ENODEV
>> in the regulator pointer.
>>
>> Also, make sure we return 0 in that case, but correctly propagate any
>> other errors. Also propagate the error from _dwc2_hcd_start.
>>
>> Fixes: 531ef5ebea96 ("usb: dwc2: add support for host mode external vbus
>> supply")
>> Cc: Amelie Delaunay
>> Signed-off-by: Tomeu Vizoso
>>
>> ---
>>
>> v2: Only overwrite the error in the pointer after checking it (Heiko
>> Stübner )
>> v3: Remove hunks that shouldn't be in this patch
>> v4: Don't overwrite the error code before returning it (kbuild test
>> robot )
>> ---
>>drivers/usb/dwc2/hcd.c | 13 -
>>1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>> index 190f95964000..c51b73b3e048 100644
>> --- a/drivers/usb/dwc2/hcd.c
>> +++ b/drivers/usb/dwc2/hcd.c
>> @@ -358,9 +358,14 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg)
>>
>>static int dwc2_vbus_supply_init(struct dwc2_hsotg *hsotg)
>>{
>> +int ret;
>> +
>> hsotg->vbus_supply = devm_regulator_get_optional(hsotg->dev, "vbus");
>> -if (IS_ERR(hsotg->vbus_supply))
>> -return 0;
>> +if (IS_ERR(hsotg->vbus_supply)) {
>> +ret = PTR_ERR(hsotg->vbus_supply);
>> +hsotg->vbus_supply = NULL;
>> +return ret == -ENODEV ? 0 : ret;
>> +}
>>
>> return regulator_enable(hsotg->vbus_supply);
>> }
>> @@ -4342,9 +4347,7 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)
>>
>> spin_unlock_irqrestore(&hsotg->lock, flags);
>>
>> -dwc2_vbus_supply_init(hsotg);
>> -
>> -return 0;
>> +return dwc2_vbus_supply_init(hsotg);
>>}
>>
>>/*
>>
>
> Reviewed-by: Grigor Tovmasyan
>
Acked-by: Minas Harutyunyan
Re: RPi4 DWC2 gadget doesn't copy data to a buffer in ep0 SETUP + DATA OUT transaction
Hi Ruslan, On 2/1/2021 3:44 AM, Ruslan Bilovol wrote: > Hi Minas and other USB experts, > > I'm currently developing new features for UAC1/UAC2 audio gadgets > like Volume/Mute controls which use Control SETUP + DATA OUT > transactions through ep0. > > While it works fine on BeagleBone black board with MUSB UDC, > on Raspberry Pi 4 with DWC2 UDC there is an issue. > > I found that when DWC2 receives ep0 SETUP + DATA OUT transaction, > it doesn't copy data transferred from the host to EP0 in DATA OUT stage > to the usb_request->buf buffer. This buffer contains unchanged data from > previous transactions. > Could you please send debug log with issue and usb trace. Thanks, Minas
Re: [PATCH] usb: dwc2: Remove redundant null check before clk_prepare_enable/clk_disable_unprepare
On 12/8/2020 11:29 AM, Artur Petrosyan wrote: > On 12/4/2020 12:36, Xu Wang wrote: >> Because clk_prepare_enable() and clk_disable_unprepare() already checked >> NULL clock parameter, so the additional checks are unnecessary, just >> remove them. >> >> Signed-off-by: Xu Wang > > Reviewed-by: Artur Petrosyan > Acked-by: Minas Harutyunyan
Re: [PATCH] usb: dwc2: Using tab instead of blank
On 3/19/2021 1:00 PM, Zeng Tao wrote: > Signed-off-by: Zeng Tao Acked-by: Minas Harutyunyan > --- > drivers/usb/dwc2/gadget.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > index ad4c943..53406f0 100644 > --- a/drivers/usb/dwc2/gadget.c > +++ b/drivers/usb/dwc2/gadget.c > @@ -3380,7 +3380,7 @@ void dwc2_hsotg_core_init_disconnected(struct > dwc2_hsotg *hsotg, > > /* remove the HNP/SRP and set the PHY */ > usbcfg &= ~(GUSBCFG_SRPCAP | GUSBCFG_HNPCAP); > -dwc2_writel(hsotg, usbcfg, GUSBCFG); > + dwc2_writel(hsotg, usbcfg, GUSBCFG); > > dwc2_phy_init(hsotg, true); > >
Re: [PATCH v2 01/15] usb: dwc2: Update exit hibernation when port reset is asserted
On 4/16/2021 4:46 PM, Artur Petrosyan wrote:
> No need to check for "DWC2_POWER_DOWN_PARAM_HIBERNATION" param
> as "hsotg->hibernated" flag is already enough for exiting from
> hibernation mode.
>
> - Removes checking of "DWC2_POWER_DOWN_PARAM_HIBERNATION" param.
>
> - For code readability Hibernation exit code moved after
> debug message print.
>
> - Added "dwc2_exit_hibernation()" function error checking.
>
> Signed-off-by: Artur Petrosyan
Acked-by: Minas Harutyunyan
> ---
> Changes in v2:
> - None
>
> drivers/usb/dwc2/hcd.c | 17 +++--
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 04a1b53d65af..cda3f931195d 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -3668,9 +3668,17 @@ static int dwc2_hcd_hub_control(struct dwc2_hsotg
> *hsotg, u16 typereq,
> break;
>
> case USB_PORT_FEAT_RESET:
> - if (hsotg->params.power_down ==
> DWC2_POWER_DOWN_PARAM_HIBERNATION &&
> - hsotg->hibernated)
> - dwc2_exit_hibernation(hsotg, 0, 1, 1);
> + dev_dbg(hsotg->dev,
> + "SetPortFeature - USB_PORT_FEAT_RESET\n");
> +
> + hprt0 = dwc2_read_hprt0(hsotg);
> +
> + if (hsotg->hibernated) {
> + retval = dwc2_exit_hibernation(hsotg, 0, 1, 1);
> + if (retval)
> + dev_err(hsotg->dev,
> + "exit hibernation failed\n");
> + }
>
> if (hsotg->in_ppd) {
> retval = dwc2_exit_partial_power_down(hsotg, 1,
> @@ -3684,9 +3692,6 @@ static int dwc2_hcd_hub_control(struct dwc2_hsotg
> *hsotg, u16 typereq,
> DWC2_POWER_DOWN_PARAM_NONE && hsotg->bus_suspended)
> dwc2_host_exit_clock_gating(hsotg, 0);
>
> - hprt0 = dwc2_read_hprt0(hsotg);
> - dev_dbg(hsotg->dev,
> - "SetPortFeature - USB_PORT_FEAT_RESET\n");
> pcgctl = dwc2_readl(hsotg, PCGCTL);
> pcgctl &= ~(PCGCTL_ENBL_SLEEP_GATING | PCGCTL_STOPPCLK);
> dwc2_writel(hsotg, pcgctl, PCGCTL);
>
Re: [PATCH v2 03/15] usb: dwc2: Fix host mode hibernation exit with remote wakeup flow.
On 4/16/2021 4:47 PM, Artur Petrosyan wrote:
> Added setting "port_connect_status_change" flag to "1" in order
> to re-enumerate, because after exit from hibernation port
> connection status is not detected.
>
> Fixes: c5c403dc4336 ("usb: dwc2: Add host/device hibernation functions")
> Signed-off-by: Artur Petrosyan
Acked-by: Minas Harutyunyan
> ---
> Changes in v2:
> - None
>
> drivers/usb/dwc2/hcd.c | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index cda3f931195d..ff945c40ef8a 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -5650,7 +5650,15 @@ int dwc2_host_exit_hibernation(struct dwc2_hsotg
> *hsotg, int rem_wakeup,
> return ret;
> }
>
> - dwc2_hcd_rem_wakeup(hsotg);
> + if (rem_wakeup) {
> + dwc2_hcd_rem_wakeup(hsotg);
> + /*
> + * Change "port_connect_status_change" flag to re-enumerate,
> + * because after exit from hibernation port connection status
> + * is not detected.
> + */
> + hsotg->flags.b.port_connect_status_change = 1;
> + }
>
> hsotg->hibernated = 0;
> hsotg->bus_suspended = 0;
>
Re: [PATCH v2 08/15] usb: dwc2: Move enter hibernation to dwc2_port_suspend() function
On 4/16/2021 4:47 PM, Artur Petrosyan wrote: > This move is done to call enter hibernation handler in > "dwc2_port_suspend()" function when core receives port suspend. > Otherwise it could be confusing to enter to hibernation in > "dwc2_hcd_hub_control()" function but other power saving modes > in "dwc2_port_suspend()" function. > > Signed-off-by: Artur Petrosyan Acked-by: Minas Harutyunyan > --- > Changes in v2: > - None > > drivers/usb/dwc2/hcd.c | 18 ++ > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c > index ff945c40ef8a..43a2298b7d42 100644 > --- a/drivers/usb/dwc2/hcd.c > +++ b/drivers/usb/dwc2/hcd.c > @@ -3321,6 +3321,18 @@ int dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 > windex) > "enter partial_power_down failed.\n"); > break; > case DWC2_POWER_DOWN_PARAM_HIBERNATION: > + /* > + * Perform spin unlock and lock because in > + * "dwc2_host_enter_hibernation()" function there is a spinlock > + * logic which prevents servicing of any IRQ during entering > + * hibernation. > + */ > + spin_unlock_irqrestore(&hsotg->lock, flags); > + ret = dwc2_enter_hibernation(hsotg, 1); > + if (ret) > + dev_err(hsotg->dev, "enter hibernation failed.\n"); > + spin_lock_irqsave(&hsotg->lock, flags); > + break; > case DWC2_POWER_DOWN_PARAM_NONE: > /* >* If not hibernation nor partial power down are supported, > @@ -3650,10 +3662,8 @@ static int dwc2_hcd_hub_control(struct dwc2_hsotg > *hsotg, u16 typereq, > "SetPortFeature - USB_PORT_FEAT_SUSPEND\n"); > if (windex != hsotg->otg_port) > goto error; > - if (hsotg->params.power_down == > DWC2_POWER_DOWN_PARAM_HIBERNATION) > - dwc2_enter_hibernation(hsotg, 1); > - else > - dwc2_port_suspend(hsotg, windex); > + if (!hsotg->bus_suspended) > + retval = dwc2_port_suspend(hsotg, windex); > break; > > case USB_PORT_FEAT_POWER: >
Re: [PATCH v2 09/15] usb: dwc2: Move exit hibernation to dwc2_port_resume() function
On 4/16/2021 4:47 PM, Artur Petrosyan wrote:
> This move is done to call hibernation exit handler in
> "dwc2_port_resume()" function when core receives port resume.
> Otherwise it could be confusing to exit hibernation in
> "dwc2_hcd_hub_control()" function but other power saving modes
> in "dwc2_port_resume()" function.
>
> Signed-off-by: Artur Petrosyan
Acked-by: Minas Harutyunyan
> ---
> Changes in v2:
> - None
>
> drivers/usb/dwc2/hcd.c | 13 +++--
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 43a2298b7d42..cc9ad6cf02d9 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -3383,6 +3383,11 @@ int dwc2_port_resume(struct dwc2_hsotg *hsotg)
> "exit partial_power_down failed.\n");
> break;
> case DWC2_POWER_DOWN_PARAM_HIBERNATION:
> + /* Exit host hibernation. */
> + ret = dwc2_exit_hibernation(hsotg, 0, 0, 1);
> + if (ret)
> + dev_err(hsotg->dev, "exit hibernation failed.\n");
> + break;
> case DWC2_POWER_DOWN_PARAM_NONE:
> /*
>* If not hibernation nor partial power down are supported,
> @@ -3446,12 +3451,8 @@ static int dwc2_hcd_hub_control(struct dwc2_hsotg
> *hsotg, u16 typereq,
> dev_dbg(hsotg->dev,
> "ClearPortFeature USB_PORT_FEAT_SUSPEND\n");
>
> - if (hsotg->bus_suspended) {
> - if (hsotg->hibernated)
> - dwc2_exit_hibernation(hsotg, 0, 0, 1);
> - else
> - dwc2_port_resume(hsotg);
> - }
> + if (hsotg->bus_suspended)
> + retval = dwc2_port_resume(hsotg);
> break;
>
> case USB_PORT_FEAT_POWER:
>
Re: [PATCH v2 10/15] usb: dwc2: Allow exit hibernation in urb enqueue
On 4/16/2021 4:48 PM, Artur Petrosyan wrote:
> When core is in hibernation state and an external
> hub is connected, upper layer sends URB enqueue request,
> which results in port reset issue.
>
> - Added exit from hibernation state to avoid port
> reset issue and process upper layer request properly.
>
> Signed-off-by: Artur Petrosyan
Acked-by: Minas Harutyunyan
> ---
> Changes in v2:
> - Moved duplicated error checking *if* conditions from innermost to outside
> if.
>
> drivers/usb/dwc2/hcd.c | 14 ++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index cc9ad6cf02d9..093b1717df01 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -4631,12 +4631,26 @@ static int _dwc2_hcd_urb_enqueue(struct usb_hcd *hcd,
> struct urb *urb,
> struct dwc2_qh *qh;
> bool qh_allocated = false;
> struct dwc2_qtd *qtd;
> + struct dwc2_gregs_backup *gr;
> +
> + gr = &hsotg->gr_backup;
>
> if (dbg_urb(urb)) {
> dev_vdbg(hsotg->dev, "DWC OTG HCD URB Enqueue\n");
> dwc2_dump_urb_info(hcd, urb, "urb_enqueue");
> }
>
> + if (hsotg->hibernated) {
> + if (gr->gotgctl & GOTGCTL_CURMODE_HOST)
> + retval = dwc2_exit_hibernation(hsotg, 0, 0, 1);
> + else
> + retval = dwc2_exit_hibernation(hsotg, 0, 0, 0);
> +
> + if (retval)
> + dev_err(hsotg->dev,
> + "exit hibernation failed.\n");
> + }
> +
> if (hsotg->in_ppd) {
> retval = dwc2_exit_partial_power_down(hsotg, 0, true);
> if (retval)
>
Re: [PATCH v2 11/15] usb: dwc2: Add hibernation entering flow by system suspend
On 4/16/2021 4:48 PM, Artur Petrosyan wrote: > Adds a new flow of entering hibernation when PC is > hibernated or suspended. > > Signed-off-by: Artur Petrosyan Acked-by: Minas Harutyunyan > --- > Changes in v2: > - None > > drivers/usb/dwc2/hcd.c | 10 ++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c > index 093b1717df01..92848629cc61 100644 > --- a/drivers/usb/dwc2/hcd.c > +++ b/drivers/usb/dwc2/hcd.c > @@ -4387,6 +4387,16 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd) > clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags); > break; > case DWC2_POWER_DOWN_PARAM_HIBERNATION: > + /* Enter hibernation */ > + spin_unlock_irqrestore(&hsotg->lock, flags); > + ret = dwc2_enter_hibernation(hsotg, 1); > + if (ret) > + dev_err(hsotg->dev, "enter hibernation failed\n"); > + spin_lock_irqsave(&hsotg->lock, flags); > + > + /* After entering suspend, hardware is not accessible */ > + clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags); > + break; > case DWC2_POWER_DOWN_PARAM_NONE: > /* >* If not hibernation nor partial power down are supported, >
Re: [PATCH v2 12/15] usb: dwc2: Add hibernation exiting flow by system resume
On 4/16/2021 4:48 PM, Artur Petrosyan wrote: > Adds a new flow of exiting hibernation when PC is resumed > from suspend state. > > Signed-off-by: Artur Petrosyan Acked-by: Minas Harutyunyan > --- > Changes in v2: > - None > > drivers/usb/dwc2/hcd.c | 10 ++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c > index 92848629cc61..035d4911a3c3 100644 > --- a/drivers/usb/dwc2/hcd.c > +++ b/drivers/usb/dwc2/hcd.c > @@ -4470,6 +4470,16 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd) > set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags); > break; > case DWC2_POWER_DOWN_PARAM_HIBERNATION: > + ret = dwc2_exit_hibernation(hsotg, 0, 0, 1); > + if (ret) > + dev_err(hsotg->dev, "exit hibernation failed.\n"); > + > + /* > + * Set HW accessible bit before powering on the controller > + * since an interrupt may rise. > + */ > + set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags); > + break; > case DWC2_POWER_DOWN_PARAM_NONE: > /* >* If not hibernation nor partial power down are supported, >
Re: [PATCH v2 13/15] usb: dwc2: Add exit hibernation mode before removing drive
On 4/16/2021 4:48 PM, Artur Petrosyan wrote:
> When dwc2 core is in hibernation mode loading
> driver again causes driver fail. Because in
> that mode registers are not accessible.
>
> In order to exit from hibernation checking
> dwc2 core power saving state in "dwc2_driver_remove()"
> function. If core is in hibernation, then checking the
> operational mode of the driver. To check whether dwc2 core
> is operating in host mode or device mode there is one way
> which is retrieving the backup value of "gotgctl" and compare
> the "CurMod" value. If previously core entered hibernation
> in host mode then the exit is performed for host if not then
> exit is performed for device mode. The introduced checking
> is because in hibernation state all registers are not
> accessible.
>
> Signed-off-by: Artur Petrosyan
> Reported-by: kernel test robot
> Reported-by: Dan Carpenter
Acked-by: Minas Harutyunyan
> ---
> drivers/usb/dwc2/platform.c | 15 +++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index f8b819cfa80e..8ad33e042a14 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -316,8 +316,23 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg
> *hsotg)
> static int dwc2_driver_remove(struct platform_device *dev)
> {
> struct dwc2_hsotg *hsotg = platform_get_drvdata(dev);
> + struct dwc2_gregs_backup *gr;
> int ret = 0;
>
> + gr = &hsotg->gr_backup;
> +
> + /* Exit Hibernation when driver is removed. */
> + if (hsotg->hibernated) {
> + if (gr->gotgctl & GOTGCTL_CURMODE_HOST)
> + ret = dwc2_exit_hibernation(hsotg, 0, 0, 1);
> + else
> + ret = dwc2_exit_hibernation(hsotg, 0, 0, 0);
> +
> + if (ret)
> + dev_err(hsotg->dev,
> + "exit hibernation failed.\n");
> + }
> +
> /* Exit Partial Power Down when driver is removed. */
> if (hsotg->in_ppd) {
> ret = dwc2_exit_partial_power_down(hsotg, 0, true);
>
Re: [PATCH v2 15/15] usb: dwc2: Get rid of useless error checks in suspend interrupt
On 4/16/2021 4:48 PM, Artur Petrosyan wrote:
> Squashed from Douglas Anderson's suggested commit
> "usb: dwc2: Get rid of useless error checks for
> hibernation/partial power down"
>
> - After this commit there should never be any
> case where dwc2_enter_partial_power_down() and
> dwc2_enter_hibernation() are called when
> 'params.power_down' is not correct. Get rid of
> the pile of error checking.
>
> - As part of this cleanup some of the error messages
> not to have __func__ in them. That's not needed
> for dev_err() calls since they already have the
> device name as part of the message.
>
> Signed-off-by: Artur Petrosyan
> Signed-off-by: Douglas Anderson
Acked-by: Minas Harutyunyan
> ---
> Changes in v2:
> - None
>
> drivers/usb/dwc2/core.c | 3 ---
> drivers/usb/dwc2/core_intr.c | 18 +++---
> 2 files changed, 7 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
> index 576c262dba55..6f70ab9577b4 100644
> --- a/drivers/usb/dwc2/core.c
> +++ b/drivers/usb/dwc2/core.c
> @@ -391,9 +391,6 @@ static bool dwc2_iddig_filter_enabled(struct dwc2_hsotg
> *hsotg)
>*/
> int dwc2_enter_hibernation(struct dwc2_hsotg *hsotg, int is_host)
> {
> - if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_HIBERNATION)
> - return -ENOTSUPP;
> -
> if (is_host)
> return dwc2_host_enter_hibernation(hsotg);
> else
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index 470458ac664b..a5ab03808da6 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -535,13 +535,10 @@ static void dwc2_handle_usb_suspend_intr(struct
> dwc2_hsotg *hsotg)
> switch (hsotg->params.power_down) {
> case DWC2_POWER_DOWN_PARAM_PARTIAL:
> ret = dwc2_enter_partial_power_down(hsotg);
> - if (ret) {
> - if (ret != -ENOTSUPP)
> - dev_err(hsotg->dev,
> - "%s: enter
> partial_power_down failed\n",
> - __func__);
> - goto skip_power_saving;
> - }
> + if (ret)
> + dev_err(hsotg->dev,
> + "enter partial_power_down
> failed\n");
> +
> udelay(100);
>
> /* Ask phy to be suspended */
> @@ -550,10 +547,9 @@ static void dwc2_handle_usb_suspend_intr(struct
> dwc2_hsotg *hsotg)
> break;
> case DWC2_POWER_DOWN_PARAM_HIBERNATION:
> ret = dwc2_enter_hibernation(hsotg, 0);
> - if (ret && ret != -ENOTSUPP)
> + if (ret)
> dev_err(hsotg->dev,
> - "%s: enter hibernation
> failed\n",
> - __func__);
> + "enter hibernation failed\n");
> break;
> case DWC2_POWER_DOWN_PARAM_NONE:
> /*
> @@ -562,7 +558,7 @@ static void dwc2_handle_usb_suspend_intr(struct
> dwc2_hsotg *hsotg)
>*/
> dwc2_gadget_enter_clock_gating(hsotg);
> }
> -skip_power_saving:
> +
> /*
>* Change to L2 (suspend) state before releasing
>* spinlock
>
Re: RPi4 DWC2 gadget doesn't copy data to a buffer in ep0 SETUP + DATA OUT transaction
Hi Pavel,
On 4/19/2021 5:22 PM, Pavel Hofman wrote:
>
> Dne 11. 02. 21 v 12:21 Minas Harutyunyan napsal(a):
>> Hi Ruslan,
>>
>> On 2/1/2021 3:44 AM, Ruslan Bilovol wrote:
>>> Hi Minas and other USB experts,
>>>
>>> I'm currently developing new features for UAC1/UAC2 audio gadgets
>>> like Volume/Mute controls which use Control SETUP + DATA OUT
>>> transactions through ep0.
>>>
>>> While it works fine on BeagleBone black board with MUSB UDC,
>>> on Raspberry Pi 4 with DWC2 UDC there is an issue.
>>>
>>> I found that when DWC2 receives ep0 SETUP + DATA OUT transaction,
>>> it doesn't copy data transferred from the host to EP0 in DATA OUT stage
>>> to the usb_request->buf buffer. This buffer contains unchanged data from
>>> previous transactions.
>>>
>>
>> Could you please send debug log with issue and usb trace.
>>
>
> Hi Minas,
>
> I confirm this problem with DWC2 gadget on RPi4. I rebased Julian's
> multiple audio clocks patch
> https://urldefense.com/v3/__https://lore.kernel.org/patchwork/patch/803422/__;!!A4F2R9G_pg!J3zujH0Tzsp0DBsJm6EPTzG0vdr9plGNx7jmoik8JvN4NqODulvND5RXZiLE8d70RkUod4ZZ$
> on top of Ruslan's
> implicit feedback patches
> https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-usb/list/?series=&submitter=121671&state=&q=&archive=&delegate=__;!!A4F2R9G_pg!J3zujH0Tzsp0DBsJm6EPTzG0vdr9plGNx7jmoik8JvN4NqODulvND5RXZiLE8d70Rtv_duVh$
> on branch rpi-5.12.y
> https://urldefense.com/v3/__https://github.com/raspberrypi/linux/tree/rpi-5.12.y__;!!A4F2R9G_pg!J3zujH0Tzsp0DBsJm6EPTzG0vdr9plGNx7jmoik8JvN4NqODulvND5RXZiLE8d70Rkqjb2Qb$
> and compiled for arm64.
>
> When USB host switches audio playback to non-default samplerate (from
> 96000 to 192000 in my case), usb_request->buf contains the previous
> default value of 96000, instead of the new value sent by the host - see
> the captured USB packet below:
>
> ===
> Frame 9: 68 bytes on wire (544 bits), 68 bytes captured (544 bits) on
> interface usbmon1, id 0
> Interface id: 0 (usbmon1)
> Interface name: usbmon1
> Encapsulation type: USB packets with Linux header and padding (115)
> Arrival Time: Apr 19, 2021 13:41:25.14665 CEST
> [Time shift for this packet: 0.0 seconds]
> Epoch Time: 1618832485.14665 seconds
> [Time delta from previous captured frame: 0.5 seconds]
> [Time delta from previous displayed frame: 0.5 seconds]
> [Time since reference or first frame: 6.013518000 seconds]
> Frame Number: 9
> Frame Length: 68 bytes (544 bits)
> Capture Length: 68 bytes (544 bits)
> [Frame is marked: False]
> [Frame is ignored: False]
> [Protocols in frame: usb]
> USB URB
> [Source: host]
> [Destination: 1.14.0]
> URB id: 0x8c37444f2480
> URB type: URB_SUBMIT ('S')
> URB transfer type: URB_CONTROL (0x02)
> Endpoint: 0x00, Direction: OUT
> 0... = Direction: OUT (0)
> = Endpoint number: 0
> Device: 14
> URB bus id: 1
> Device setup request: relevant (0)
> Data: present (0)
> URB sec: 1618832485
> URB usec: 146650
> URB status: Operation now in progress (-EINPROGRESS) (-115)
> URB length [bytes]: 4
> Data length [bytes]: 4
> [Response in: 10]
> Interval: 0
> Start frame: 0
> Copy of Transfer Flags: 0x
> ...0 = Short not OK: False
> ..0. = ISO ASAP: False
> .0.. = No transfer DMA map: False
> ..0. = No FSBR: False
> .0.. = Zero Packet: False
> 0... = No Interrupt: False
> ...0 = Free Buffer: False
> ..0. = Dir IN: False
> ...0 = DMA Map Single: False
> ..0. = DMA Map Page: False
> .0.. = DMA Map SG: False
> 0... = Map Local: False
> ...0 = Setup Map Single: False
> ..0. = Setup Map Local: False
> .0.. = DMA S-G Combined: False
> .
Re: [PATCH] usb: dwc2: Add STM32 related debugfs entries
On 2/28/2021 1:55 AM, Martin Devera wrote: > These are entries related to STM32MP1 PHY control. > > Signed-off-by: Martin Devera Acked-by: Minas Harutyunyan > --- > drivers/usb/dwc2/debugfs.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/usb/dwc2/debugfs.c b/drivers/usb/dwc2/debugfs.c > index aaafd463d72a..f13eed4231e1 100644 > --- a/drivers/usb/dwc2/debugfs.c > +++ b/drivers/usb/dwc2/debugfs.c > @@ -691,6 +691,8 @@ static int params_show(struct seq_file *seq, void *v) > print_param(seq, p, ulpi_fs_ls); > print_param(seq, p, host_support_fs_ls_low_power); > print_param(seq, p, host_ls_low_power_phy_clk); > + print_param(seq, p, activate_stm_fs_transceiver); > + print_param(seq, p, activate_stm_id_vb_detection); > print_param(seq, p, ts_dline); > print_param(seq, p, reload_ctl); > print_param_hex(seq, p, ahbcfg); >
Re: [PATCH 1/3] usb: dwc2: set ahbcfg parameter for STM32MP15 OTG HS and FS
On 11/23/2020 1:01 PM, Amelie Delaunay wrote: > STM32MP15 ahbcfg register default value sets Burst length/type (HBSTLEN) > to Single (32-bit accesses on AHB), which is not recommended, according > to STM32MP157 Reference manual [1]. > This patch sets Burst length/type (HBSTLEN) so that bus transactions > target 16x32 bit accesses. This improves OTG controller performance. > > [1] > https://urldefense.com/v3/__https://www.st.com/resource/en/reference_manual/dm00327659.pdf__;!!A4F2R9G_pg!J1HTs3kobvYfS453htoIdnxhej4_os5Y5xwMfRtfhptE_QSeVw3O1mozY-v4AvE$ > , p.3149 > > Signed-off-by: Amelie Delaunay Acked-by: Minas Harutyunyan > --- > drivers/usb/dwc2/params.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c > index 267543c3dc38..0df693319f0a 100644 > --- a/drivers/usb/dwc2/params.c > +++ b/drivers/usb/dwc2/params.c > @@ -177,6 +177,7 @@ static void dwc2_set_stm32mp15_fsotg_params(struct > dwc2_hsotg *hsotg) > p->i2c_enable = false; > p->activate_stm_fs_transceiver = true; > p->activate_stm_id_vb_detection = true; > + p->ahbcfg = GAHBCFG_HBSTLEN_INCR16 << GAHBCFG_HBSTLEN_SHIFT; > p->power_down = DWC2_POWER_DOWN_PARAM_NONE; > } > > @@ -189,6 +190,7 @@ static void dwc2_set_stm32mp15_hsotg_params(struct > dwc2_hsotg *hsotg) > p->host_rx_fifo_size = 440; > p->host_nperio_tx_fifo_size = 256; > p->host_perio_tx_fifo_size = 256; > + p->ahbcfg = GAHBCFG_HBSTLEN_INCR16 << GAHBCFG_HBSTLEN_SHIFT; > p->power_down = DWC2_POWER_DOWN_PARAM_NONE; > } > >
Re: [PATCH 2/3] usb: dwc2: enable FS/LS PHY clock select on STM32MP15 FS OTG
On 11/23/2020 1:01 PM, Amelie Delaunay wrote: > When the core is in FS host mode, using the FS transceiver, and a Low-Speed > device is connected, transceiver clock is 6Mhz. > So, to support Low-Speed devices, enable support of FS/LS Low Power mode, > so that the PHY supplies a 6 MHz clock during Low-Speed mode. > > Signed-off-by: Amelie Delaunay Acked-by: Minas Harutyunyan > --- > drivers/usb/dwc2/params.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c > index 0df693319f0a..9e5dd7f3f2f6 100644 > --- a/drivers/usb/dwc2/params.c > +++ b/drivers/usb/dwc2/params.c > @@ -179,6 +179,8 @@ static void dwc2_set_stm32mp15_fsotg_params(struct > dwc2_hsotg *hsotg) > p->activate_stm_id_vb_detection = true; > p->ahbcfg = GAHBCFG_HBSTLEN_INCR16 << GAHBCFG_HBSTLEN_SHIFT; > p->power_down = DWC2_POWER_DOWN_PARAM_NONE; > + p->host_support_fs_ls_low_power = true; > + p->host_ls_low_power_phy_clk = true; > } > > static void dwc2_set_stm32mp15_hsotg_params(struct dwc2_hsotg *hsotg) >
Re: [PATCH 3/3] usb: dwc2: disable Link Power Management on STM32MP15 HS OTG
On 11/23/2020 1:01 PM, Amelie Delaunay wrote:
> Link Power Management (LPM) on STM32MP15 OTG HS encounters instabilities
> with some Host controllers. OTG core fails to exit L1 state in 200us:
> "dwc2 4900.usb-otg: Failed to exit L1 sleep state in 200us."
> Then the device is still not enumerated.
>
> To avoid this issue, disable Link Power Management on STM32MP15 HS OTG.
>
> Signed-off-by: Amelie Delaunay
Acked-by: Minas Harutyunyan
> ---
> drivers/usb/dwc2/params.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
> index 9e5dd7f3f2f6..92df3d620f7d 100644
> --- a/drivers/usb/dwc2/params.c
> +++ b/drivers/usb/dwc2/params.c
> @@ -194,6 +194,10 @@ static void dwc2_set_stm32mp15_hsotg_params(struct
> dwc2_hsotg *hsotg)
> p->host_perio_tx_fifo_size = 256;
> p->ahbcfg = GAHBCFG_HBSTLEN_INCR16 << GAHBCFG_HBSTLEN_SHIFT;
> p->power_down = DWC2_POWER_DOWN_PARAM_NONE;
> + p->lpm = false;
> + p->lpm_clock_gating = false;
> + p->besl = false;
> + p->hird_threshold_en = false;
> }
>
> const struct of_device_id dwc2_of_match_table[] = {
>
Re: [PATCH] usb: dwc2: hcd: call dwc2_is_controller_alive under spinlock
Hi Jaehoon Chung,
On 2/14/2019 2:04 PM, Jaehoon Chung wrote:
> This patch is referred to Robert's patch
> commit cf54772b913b ("usb: dwc2: call dwc2_is_controller_alive() under
> spinlock")
>
> During running sdb with otg mode, the usb is hung sometime.
>
> The one of SDB hang issues should be fixed with this patch.
> After hang, it doesn't never trigger the usb interrupt.
>
> Signed-off-by: Jaehoon Chung
> ---
> drivers/usb/dwc2/hcd_intr.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
> index 88b5dcf..8d3b155 100644
> --- a/drivers/usb/dwc2/hcd_intr.c
> +++ b/drivers/usb/dwc2/hcd_intr.c
> @@ -,13 +,13 @@ irqreturn_t dwc2_handle_hcd_intr(struct dwc2_hsotg
> *hsotg)
> u32 gintsts, dbg_gintsts;
> irqreturn_t retval = IRQ_NONE;
>
> + spin_lock(&hsotg->lock);
> +
> if (!dwc2_is_controller_alive(hsotg)) {
> dev_warn(hsotg->dev, "Controller is dead\n");
> - return retval;
> + goto out;
> }
>
> - spin_lock(&hsotg->lock);
> -
> /* Check if HOST Mode */
> if (dwc2_is_host_mode(hsotg)) {
> gintsts = dwc2_read_core_intr(hsotg);
> @@ -2276,6 +2276,7 @@ irqreturn_t dwc2_handle_hcd_intr(struct dwc2_hsotg
> *hsotg)
> }
> }
>
> +out:
> spin_unlock(&hsotg->lock);
>
> return retval;
>
1. Checking core alive or not was introduced in our internal reference
driver for our setups, because sometime AHB-PCI bridge can hung, not
core itself. If it happen then only way to overcome it - reboot core
setup and PC.
2. Actually this issue was fixed on our setups and currently no need to
check access via bridge to core.
3. Any case if it still happen on some platforms then first need to
check and fix HW.
On asserted any interrupt for dwc2, handlers call sequence is follow:
common->gadget->host handlers. If core alive checked in common handler
then checking same stuff in host handler again not needed at all.
So, core alive checking in host handler create issue for your test, I
suggest to not fix alive checking in host interrupt handler as you
suggested and done in common handler, but just remove it.
Thanks,
Minas
Re: [PATCH] usb: dwc2: use a longer AHB idle timeout in dwc2_core_reset()
On 6/20/2019 9:51 PM, Martin Blumenstingl wrote:
Use a 1us AHB idle timeout in dwc2_core_reset() and make it
consistent with the other "wait for AHB master IDLE state" ocurrences.
This fixes a problem for me where dwc2 would not want to initialize when
updating to 4.19 on a MIPS Lantiq VRX200 SoC. dwc2 worked fine with
4.14.
Testing on my board shows that it takes 180us until AHB master IDLE
state is signalled. The very old vendor driver for this SoC (ifxhcd)
used a 1 second timeout.
Use the same timeout that is used everywhere when polling for
GRSTCTL_AHBIDLE instead of using a timeout that "works for one board"
(180us in my case) to have consistent behavior across the dwc2 driver.
Cc: linux-stable # 4.19+
Signed-off-by: Martin Blumenstingl
---
Acked-by: Minas Harutyunyan
drivers/usb/dwc2/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index 8b499d643461..8e41d70fd298 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -531,7 +531,7 @@ int dwc2_core_reset(struct dwc2_hsotg *hsotg, bool
skip_wait)
}
/* Wait for AHB master IDLE state */
- if (dwc2_hsotg_wait_bit_set(hsotg, GRSTCTL, GRSTCTL_AHBIDLE, 50)) {
+ if (dwc2_hsotg_wait_bit_set(hsotg, GRSTCTL, GRSTCTL_AHBIDLE, 1)) {
dev_warn(hsotg->dev, "%s: HANG! AHB Idle timeout GRSTCTL
GRSTCTL_AHBIDLE\n",
__func__);
return -EBUSY;
Re: REGRESSION: dwc2: gadget: Add scatter-gather mode
Hi Andrzej,
USB CV MSC tests failed starting from Test Case 6 with BNA interrupt on
ep1in. It's first BULK IN transaction after GET MAXLUN.
[319523.955339] dwc2 dwc2.1.auto: dwc2_hsotg_irq: 04088028 0008
(d88c3cc4) retry 8
[319523.955357] dwc2 dwc2.1.auto: dwc2_hsotg_irq: daint=0002
[319523.955366] dwc2 dwc2.1.auto: dwc2_hsotg_epint: ep1(out)
DxEPINT=0x0001
[319523.955372] dwc2 dwc2.1.auto: dwc2_hsotg_epint: XferCompl:
DxEPCTL=0x00098200, DXEPTSIZ=15c7d486
[319523.955377] dwc2 dwc2.1.auto: complete: ep b58e703a ep1out,
req bde78704, 0 => 2620ceb8
[319523.955396] dwc2 dwc2.1.auto: ep1in: req acdadb34:
36@450496f9, noi=0, zero=0, snok=0
[319523.955403] dwc2 dwc2.1.auto: dwc2_hsotg_start_req:
DxEPCTL=0x004a8200, ep 1, dir in
[319523.955406] dwc2 dwc2.1.auto: ureq->length:36 ureq->actual:0
[319523.955408] dwc2 dwc2.1.auto: dwc2_hsotg_start_req: 1@36/36,
0x20080024 => 0x0930
[319523.955410] dwc2 dwc2.1.auto: dwc2_hsotg_start_req: 36cbc000 pad =>
0x0934
[319523.955412] dwc2 dwc2.1.auto: ep0 state:0
[319523.955413] dwc2 dwc2.1.auto: dwc2_hsotg_start_req: DxEPCTL=0x844a8200
[319523.955420] dwc2 dwc2.1.auto: dwc2_hsotg_start_req: DXEPCTL=0x80488200
[319523.955425] dwc2 dwc2.1.auto: ep1in: req f74873bc:
13@7a860fd2, noi=0, zero=0, snok=0
[319523.955446] dwc2 dwc2.1.auto: dwc2_hsotg_irq: 04048028 0004
(d88c3cc4) retry 8
[319523.955451] dwc2 dwc2.1.auto: dwc2_hsotg_irq: daint=0002
[319523.955460] dwc2 dwc2.1.auto: dwc2_hsotg_epint: ep1(in)
DxEPINT=0x0200
[319523.955462] dwc2 dwc2.1.auto: dwc2_hsotg_epint: BNA interrupt
[319553.987219] dwc2 dwc2.1.auto: dwc2_hsotg_irq: 04008428 0400
(d88c3cc4) retry 8
Meanwhile it's passing smoke test with mass storage function.
Thanks,
Minas
On 3/9/2019 12:53 AM, John Stultz wrote:
> Hey Andrzej,
>I just wanted to let you know, trying to boot linus/master on the
> hikey board today, I've been seeing lots of errors on boot (which
> sometimes crash the board, and sometimes doesn't). See the example
> below.
>
> I've bisected the issue down to 10209abe87f5 ("usb: dwc2: gadget: Add
> scatter-gather mode"), and if I revert that change the issue goes
> away.
>
> Looking at the patch, I can't see anything obviously sticking out, but
> let me know if you have anything suggestions you'd like me to test.
>
> thanks
> -john
>
>
> [ 13.088934] functionfs read size 512 > requested size 24, splitting
> request into multiple reads.
> [ 13.089029] [ cut here ]
> [ 13.102665] Trying to vfree() bad address (ecf3404f)
> [ 13.108436] WARNING: CPU: 0 PID: 2014 at mm/vmalloc.c:1516
> __vunmap+0xe0/0xe8
> [ 13.115587] CPU: 0 PID: 2014 Comm: adbd Not tainted
> 5.0.0-08291-ga032141-dirty #897
> [ 13.123244] Hardware name: HiKey Development Board (DT)
> [ 13.128469] pstate: 8045 (Nzcv daif +PAN -UAO)
> [ 13.133266] pc : __vunmap+0xe0/0xe8
> [ 13.136757] lr : __vunmap+0xe0/0xe8
> [ 13.140243] sp : ff80114d3b40
> [ 13.143555] x29: ff80114d3b40 x28: ffc07333d280
> [ 13.148868] x27: ffc071b67a38 x26: 01e8
> [ 13.154180] x25: ff80114d3ca8 x24: ffc07190e400
> [ 13.159492] x23: ffc0748bca00 x22: ff8010ffd000
> [ 13.164803] x21: 0001 x20: ff80114d3c98
> [ 13.170113] x19: ff8010414e74 x18: ff8010ffda48
> [ 13.175423] x17: x16:
> [ 13.180734] x15: ff80914d3867 x14: 0006
> [ 13.186044] x13: ff80114d3875 x12: ff801101c898
> [ 13.191371] x11: ff801101c000 x10: 05f5e0ff
> [ 13.191416] type=1400 audit(16.743:79): avc: denied { read } for
> comm="drmserver" name="enabled" dev="sysfs" ino=5492
> scontext=u:r:drmserver:s0 tcont0
> [ 13.196698] x9 : ff80114d3800 x8 : 2966343034336663
> [ 13.196707] x7 : 6530303030303030 x6 : 0424
> [ 13.196713] x5 : x4 :
> [ 13.196718] x3 : 0001 x2 : 0001
> [ 13.196724] x1 : b357113b3815f700 x0 :
> [ 13.214445] type=1400 audit(17.075:84): avc: denied { read } for
> comm="audioserver" name="enabled" dev="sysfs" ino=5492
> scontext=u:r:audioserver:s0 t0
> [ 13.219604] Call trace:
> [ 13.219616] __vunmap+0xe0/0xe8
> [ 13.219623] __vfree+0x24/0x70
> [ 13.219635] vfree+0x20/0x38
> [ 13.270258] ffs_epfile_io.isra.12+0x190/0x6b0
> [ 13.274707] ffs_epfile_read_iter+0xa0/0x168
> [ 13.278982] __vfs_read+0x10c/0x168
> [ 13.282471] vfs_read+0x8c/0x148
> [ 13.285700] ksys_read+0x5c/0xc8
> [ 13.288930] __arm64_sys_read+0x14/0x20
> [ 13.292773] el0_svc_common.constprop.0+0xb0/0x110
> [ 13.297461] type=1400 audit(17.075:84): avc: denied { read } for
> comm="audioserver" name="enabled" dev="sysfs" ino=5492
> scontext=u:r:audioserver:s0 t0
> [ 13.297570] el0_svc_handler+0x28/0x78
> [ 13.315592] type=1400 audit(17.179:85): avc: denied { read }
Re: [PATCH 0/4] usb: dwc2: fix host mode external vbus supply management
On 9/5/2018 3:40 PM, Amelie Delaunay wrote: > This patchset fixes and improves host mode external vbus supply management, > mainly around suspend/resume use cases. It also avoid 'vbus regulator" > to be requested lots of times upon each call to dwc2_vbus_supply_init(), > especially when pm runtime is enabled. > > Fabrice Gasnier (4): >usb: dwc2: get optional vbus-supply regulator once >usb: dwc2: fix a race with external vbus supply >usb: dwc2: fix call to vbus supply exit routine, call it unlocked >usb: dwc2: fix unbalanced use of external vbus-supply > > drivers/usb/dwc2/hcd.c | 45 > ++--- > drivers/usb/dwc2/platform.c | 8 > 2 files changed, 42 insertions(+), 11 deletions(-) > Tested-by: Artur Petrosyan Acked-by: Minas Harutyunyan
Re: [PATCH] usb: dwc2: Fix HiKey regression caused by power_down feature
Hi John,
On 5/19/2018 4:49 AM, John Stultz wrote:
> In 4.17-rc, commit 03ea6d6e9e1f ("usb: dwc2: Enable power down")
> caused the HiKey board to not correctly handle switching between
> usb-gadget and usb-host mode.
>
> Unplugging the OTG port would result in:
> [ 42.240973] dwc2 f72c.usb: dwc2_restore_host_registers: no host
> registers to restore
> [ 42.249066] dwc2 f72c.usb: dwc2_host_exit_hibernation: failed to
> restore host registers
>
> And the USB-host ports would not function.
>
> And plugging in the OTG port, we would see:
> [ 46.046557] WARNING: CPU: 3 PID: 6 at drivers/usb/dwc2/gadget.c:260
> dwc2_hsotg_init_fifo+0x194/0x1a0
> [ 46.055761] CPU: 3 PID: 6 Comm: kworker/u16:0 Not tainted
> 4.17.0-rc5-00030-ge67da8c #231
> [ 46.055767] Hardware name: HiKey Development Board (DT)
> [ 46.055784] Workqueue: dwc2 dwc2_conn_id_status_change
> ...
>
Could you please send full log to debug.
> Thus, this patch sets the hisi params to disable the power_down
> flag by default, and gets thing working again.
>
> Cc: John Youn
> Cc: Vardan Mikayelyan
> Cc: Artur Petrosyan
> Cc: Grigor Tovmasyan
> Cc: Felipe Balbi
> Cc: linux-...@vger.kernel.org
> Signed-off-by: John Stultz
> ---
> drivers/usb/dwc2/params.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
> index f03e418..96b1b25 100644
> --- a/drivers/usb/dwc2/params.c
> +++ b/drivers/usb/dwc2/params.c
> @@ -70,6 +70,7 @@ static void dwc2_set_his_params(struct dwc2_hsotg *hsotg)
> GAHBCFG_HBSTLEN_SHIFT;
> p->uframe_sched = false;
> p->change_speed_quirk = true;
> + p->power_down = false;
power_down declared as int, suggested to update as follow:
p->power_down = DWC2_POWER_DOWN_PARAM_NONE;
This can be accepted as temporary solution until we will fully debug
hibernation feature for HiKey platform.
> }
>
> static void dwc2_set_rk_params(struct dwc2_hsotg *hsotg)
>
Re: [PATCH v2] usb: dwc3: Prevent indefinite sleep in _dwc3_set_mode during suspend/resume
Hi, On 3/16/2018 4:25 PM, Felipe Balbi wrote: > > Hi, > > Minas Harutyunyan writes: >>>>> On 09/03/18 14:47, Roger Quadros wrote: >>>>>> In the following test we get stuck by sleeping forever in >>>>>> _dwc3_set_mode() >>>>>> after which dual-role switching doesn't work. >>>>>> >>>>>> On dra7-evm's dual-role port, >>>>>> - Load g_zero gadget driver and enumerate to host >>>>>> - suspend to mem >>>>>> - disconnect USB cable to host and connect otg cable with Pen drive in >>>>>> it. >>>>>> - resume system >>>>>> - we sleep indefinitely in _dwc3_set_mode due to. >>>>>> dwc3_gadget_exit()->usb_del_gadget_udc()->udc_stop()-> >>>>>> dwc3_gadget_stop()->wait_event_lock_irq() >>>>>> >>>>>> To fix this instead of waiting indefinitely with wait_event_lock_irq() >>>>>> we use wait_event_interruptible_lock_irq_timeout() and print >>>>>> and error message if there was a timeout. >>>>>> >>>>>> Signed-off-by: Roger Quadros >>>>> >>>>> Thanks for picking this for -next. >>>>> Is it better to have this in v4.16-rc fixes? >>>>> and also stable? v4.12+ >>>> >>>> Well, there was no "Fixes: foobar" or "Cc: stable" lines in the commit >>>> log ;-) >>>> >>>> The best we can do now, is wait for -rc1 and manually send the commit to >>>> stable. >>>> >>> >>> That's fine. Thanks. >>> >> >> Same issue seen in dwc3_gadget_ep_dequeue() function where also used >> wait_event_lock_irq() - as result infinite loop. > > how did this happen? During rmmod dwc3? Or, perhaps, after you unloaded > a gadget driver? > No, not during rmmod's. We using our internal USB testing tool. Test case; ISOC OUT, transfer size N frames. When host starts ISOC OUT traffic then the dwc3 based on "Transfer not ready" event in frame F starts transfers staring from frame F+4 (for bInterval=1) as result 4 requests, which already queued on device side, remain incomplete. Function driver on some timeout trying dequeue these 4 requests (without disabling EP) to complete test. For IN ISOC's these requests completed on MISSED ISOC event, but for ISOC OUT required call dequeue on some timeout. >> Actually to fix this issue I updated condition of wait function >> from: >> !(dep->flags & DWC3_EP_END_TRANSFER_PENDING) >> to: >> !(dep->flags & DWC3_EP_END_TRANSFER_PENDING & DWC3_EP_ENABLED) > > you're not fixing anything. You're, essentially, removing the entire > end transfer pending logic. yes, you are right, but how to overcome this infinite loop? Replace wait_event_lock_irq() by wait_event_interruptible_lock_irq_timeout()? The whole idea of this is that we can > disable the endpoint and wait for the End Transfer interrupt. When you > add a check for the endpoint being enabled, then that code will never > run and, thus, never wait for the End Transfer IRQ. > > If you manage to find a more reliable way of reproducing this, then make > sure to capture dwc3 tracepoints (see the documentation for details) and > let's start trying to figure out what's going on. > > cheers >
Re: [PATCH v2] usb: dwc3: Prevent indefinite sleep in _dwc3_set_mode during suspend/resume
Hi, On 3/19/2018 12:55 PM, Felipe Balbi wrote: > > Hi, > > Minas Harutyunyan writes: >>>>>>> Thanks for picking this for -next. >>>>>>> Is it better to have this in v4.16-rc fixes? >>>>>>> and also stable? v4.12+ >>>>>> >>>>>> Well, there was no "Fixes: foobar" or "Cc: stable" lines in the commit >>>>>> log ;-) >>>>>> >>>>>> The best we can do now, is wait for -rc1 and manually send the commit to >>>>>> stable. >>>>>> >>>>> >>>>> That's fine. Thanks. >>>>> >>>> >>>> Same issue seen in dwc3_gadget_ep_dequeue() function where also used >>>> wait_event_lock_irq() - as result infinite loop. >>> >>> how did this happen? During rmmod dwc3? Or, perhaps, after you unloaded >>> a gadget driver? >>> >> No, not during rmmod's. >> We using our internal USB testing tool. Test case; ISOC OUT, transfer >> size N frames. When host starts ISOC OUT traffic then the dwc3 based on >> "Transfer not ready" event in frame F starts transfers staring from >> frame F+4 (for bInterval=1) as result 4 requests, which already queued >> on device side, remain incomplete. Function driver on some timeout >> trying dequeue these 4 requests (without disabling EP) to complete test. >> For IN ISOC's these requests completed on MISSED ISOC event, but for >> ISOC OUT required call dequeue on some timeout. > > okay > >>>> Actually to fix this issue I updated condition of wait function >>>> from: >>>> !(dep->flags & DWC3_EP_END_TRANSFER_PENDING) >>>> to: >>>> !(dep->flags & DWC3_EP_END_TRANSFER_PENDING & DWC3_EP_ENABLED) >>> >>> you're not fixing anything. You're, essentially, removing the entire >>> end transfer pending logic. >> yes, you are right, but how to overcome this infinite loop? Replace >> wait_event_lock_irq() by wait_event_interruptible_lock_irq_timeout()? > > The best way here would be to figure why we're missing command complete > IRQ in those cases. According to documentation, we *should* receive that > interrupt, so why is it missing? > Additional info on test. Core configuration is HS only mode, test speed HS, core version v2.90a. Maybe it will help to understand cause of issue. BTW, currently to pass above describe ISOC OUT test we just commented wait_event_lock_irq() in dwc3_gadget_ep_dequeue() function and successfully received request completion in function driver. Thanks, Minas
Re: [PATCH v2] usb: dwc3: Prevent indefinite sleep in _dwc3_set_mode during suspend/resume
Hi, On 3/19/2018 3:36 PM, Minas Harutyunyan wrote: > Hi, > > On 3/19/2018 12:55 PM, Felipe Balbi wrote: >> >> Hi, >> >> Minas Harutyunyan writes: >>>>>>>> Thanks for picking this for -next. >>>>>>>> Is it better to have this in v4.16-rc fixes? >>>>>>>> and also stable? v4.12+ >>>>>>> >>>>>>> Well, there was no "Fixes: foobar" or "Cc: stable" lines in the commit >>>>>>> log ;-) >>>>>>> >>>>>>> The best we can do now, is wait for -rc1 and manually send the commit to >>>>>>> stable. >>>>>>> >>>>>> >>>>>> That's fine. Thanks. >>>>>> >>>>> >>>>> Same issue seen in dwc3_gadget_ep_dequeue() function where also used >>>>> wait_event_lock_irq() - as result infinite loop. >>>> >>>> how did this happen? During rmmod dwc3? Or, perhaps, after you unloaded >>>> a gadget driver? >>>> >>> No, not during rmmod's. >>> We using our internal USB testing tool. Test case; ISOC OUT, transfer >>> size N frames. When host starts ISOC OUT traffic then the dwc3 based on >>> "Transfer not ready" event in frame F starts transfers staring from >>> frame F+4 (for bInterval=1) as result 4 requests, which already queued >>> on device side, remain incomplete. Function driver on some timeout >>> trying dequeue these 4 requests (without disabling EP) to complete test. >>> For IN ISOC's these requests completed on MISSED ISOC event, but for >>> ISOC OUT required call dequeue on some timeout. >> >> okay >> >>>>> Actually to fix this issue I updated condition of wait function >>>>> from: >>>>> !(dep->flags & DWC3_EP_END_TRANSFER_PENDING) >>>>> to: >>>>> !(dep->flags & DWC3_EP_END_TRANSFER_PENDING & DWC3_EP_ENABLED) >>>> >>>> you're not fixing anything. You're, essentially, removing the entire >>>> end transfer pending logic. >>> yes, you are right, but how to overcome this infinite loop? Replace >>> wait_event_lock_irq() by wait_event_interruptible_lock_irq_timeout()? >> >> The best way here would be to figure why we're missing command complete >> IRQ in those cases. According to documentation, we *should* receive that >> interrupt, so why is it missing? >> > > Additional info on test. Core configuration is HS only mode, test speed > HS, core version v2.90a. Maybe it will help to understand cause of issue. > BTW, currently to pass above describe ISOC OUT test we just commented > wait_event_lock_irq() in dwc3_gadget_ep_dequeue() function and > successfully received request completion in function driver. > Thanks, > Minas > One more info: while function driver call dequeue, host periodically send control read command to get status of test from function - test In Progress or Finished. Thanks, Minas
Re: [PATCH v2] usb: dwc3: Prevent indefinite sleep in _dwc3_set_mode during suspend/resume
Hi, On 3/16/2018 3:03 PM, Roger Quadros wrote: > On 16/03/18 13:00, Felipe Balbi wrote: >> >> Hi, >> >> Roger Quadros writes: >> >>> Hi Felipe, >>> >>> On 09/03/18 14:47, Roger Quadros wrote: In the following test we get stuck by sleeping forever in _dwc3_set_mode() after which dual-role switching doesn't work. On dra7-evm's dual-role port, - Load g_zero gadget driver and enumerate to host - suspend to mem - disconnect USB cable to host and connect otg cable with Pen drive in it. - resume system - we sleep indefinitely in _dwc3_set_mode due to. dwc3_gadget_exit()->usb_del_gadget_udc()->udc_stop()-> dwc3_gadget_stop()->wait_event_lock_irq() To fix this instead of waiting indefinitely with wait_event_lock_irq() we use wait_event_interruptible_lock_irq_timeout() and print and error message if there was a timeout. Signed-off-by: Roger Quadros >>> >>> Thanks for picking this for -next. >>> Is it better to have this in v4.16-rc fixes? >>> and also stable? v4.12+ >> >> Well, there was no "Fixes: foobar" or "Cc: stable" lines in the commit >> log ;-) >> >> The best we can do now, is wait for -rc1 and manually send the commit to >> stable. >> > > That's fine. Thanks. > Same issue seen in dwc3_gadget_ep_dequeue() function where also used wait_event_lock_irq() - as result infinite loop. Actually to fix this issue I updated condition of wait function from: !(dep->flags & DWC3_EP_END_TRANSFER_PENDING) to: !(dep->flags & DWC3_EP_END_TRANSFER_PENDING & DWC3_EP_ENABLED) Not, sure that this fix is fully correct because I'm familiar with dwc3, but this fix allow us to go forward with request dequeue. I think, need deeper investigation of infinite loop to catch root cause of it, before accept any of fixes. Thanks, Minas
Re: [PATCH 11/30] usb: dwc2: gadget: Avoid pointless read of EP control register
Hi,
On 7/3/2020 11:29 AM, Greg KH wrote:
> On Thu, Jul 02, 2020 at 03:46:06PM +0100, Lee Jones wrote:
>> Commit ec1f9d9f01384 ("usb: dwc2: gadget: parity fix in isochronous mode")
>> moved
>> these checks to dwc2_hsotg_change_ep_iso_parity() back in 2015. The assigned
>> value hasn't been read back since. Let's remove the unnecessary H/W read.
>>
>> Fixes the following W=1 warning:
>>
>> drivers/usb/dwc2/gadget.c: In function ‘dwc2_hsotg_epint’:
>> drivers/usb/dwc2/gadget.c:2981:6: warning: variable ‘ctrl’ set but not
>> used [-Wunused-but-set-variable]
>> 2981 | u32 ctrl;
>> | ^~~~
>>
>> Cc: Minas Harutyunyan
>> Cc: Ben Dooks
>> Signed-off-by: Lee Jones
>> ---
>> drivers/usb/dwc2/gadget.c | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>> index 116e6175c7a48..fa07e3fcb8841 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -2975,10 +2975,8 @@ static void dwc2_hsotg_epint(struct dwc2_hsotg
>> *hsotg, unsigned int idx,
>> u32 epctl_reg = dir_in ? DIEPCTL(idx) : DOEPCTL(idx);
>> u32 epsiz_reg = dir_in ? DIEPTSIZ(idx) : DOEPTSIZ(idx);
>> u32 ints;
>> -u32 ctrl;
>>
>> ints = dwc2_gadget_read_ep_interrupts(hsotg, idx, dir_in);
>> -ctrl = dwc2_readl(hsotg, epctl_reg);
>
> As you know, lots of hardware requires reads to happen to do things, so
> are you sure it is safe to remove this read call?
>
Greg, yes, it's Ok to remove this unnecessary read which remained from
previous implementations.
Lee, please add "Fixes:" tag and resubmit v2.
Thanks,
Minas
> thanks,
>
> greg k-h
>
Re: [PATCH 11/30] usb: dwc2: gadget: Avoid pointless read of EP control register
Hi,
On 7/3/2020 11:43 AM, Greg KH wrote:
> On Fri, Jul 03, 2020 at 07:38:16AM +0000, Minas Harutyunyan wrote:
>> Hi,
>>
>> On 7/3/2020 11:29 AM, Greg KH wrote:
>>> On Thu, Jul 02, 2020 at 03:46:06PM +0100, Lee Jones wrote:
>>>> Commit ec1f9d9f01384 ("usb: dwc2: gadget: parity fix in isochronous mode")
>>>> moved
>>>> these checks to dwc2_hsotg_change_ep_iso_parity() back in 2015. The
>>>> assigned
>>>> value hasn't been read back since. Let's remove the unnecessary H/W read.
>>>>
>>>> Fixes the following W=1 warning:
>>>>
>>>>drivers/usb/dwc2/gadget.c: In function ‘dwc2_hsotg_epint’:
>>>> drivers/usb/dwc2/gadget.c:2981:6: warning: variable ‘ctrl’ set but not
>>>> used [-Wunused-but-set-variable]
>>>>2981 | u32 ctrl;
>>>>| ^~~~
>>>>
>>>> Cc: Minas Harutyunyan
>>>> Cc: Ben Dooks
>>>> Signed-off-by: Lee Jones
>>>> ---
>>>>drivers/usb/dwc2/gadget.c | 2 --
>>>>1 file changed, 2 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>>>> index 116e6175c7a48..fa07e3fcb8841 100644
>>>> --- a/drivers/usb/dwc2/gadget.c
>>>> +++ b/drivers/usb/dwc2/gadget.c
>>>> @@ -2975,10 +2975,8 @@ static void dwc2_hsotg_epint(struct dwc2_hsotg
>>>> *hsotg, unsigned int idx,
>>>>u32 epctl_reg = dir_in ? DIEPCTL(idx) : DOEPCTL(idx);
>>>>u32 epsiz_reg = dir_in ? DIEPTSIZ(idx) : DOEPTSIZ(idx);
>>>>u32 ints;
>>>> - u32 ctrl;
>>>>
>>>>ints = dwc2_gadget_read_ep_interrupts(hsotg, idx, dir_in);
>>>> - ctrl = dwc2_readl(hsotg, epctl_reg);
>>>
>>> As you know, lots of hardware requires reads to happen to do things, so
>>> are you sure it is safe to remove this read call?
>>>
>>
>> Greg, yes, it's Ok to remove this unnecessary read which remained from
>> previous implementations.
>
> Great, thanks for confirming!
>
Acked-by: Minas Harutyunyan
> greg k-h
>
Re: [PATCH for-5.8 v2] usb: dwc2: Add missing cleanups when usb_add_gadget_udc() fails
On 7/4/2020 2:50 AM, Martin Blumenstingl wrote:
> Call dwc2_debugfs_exit() and dwc2_hcd_remove() (if the HCD was enabled
> earlier) when usb_add_gadget_udc() has failed. This ensures that the
> debugfs entries created by dwc2_debugfs_init() as well as the HCD are
> cleaned up in the error path.
>
> Fixes: 207324a321a866 ("usb: dwc2: Postponed gadget registration to the udc
> class driver")
> Signed-off-by: Martin Blumenstingl
Acked-by: Minas Harutyunyan
> ---
> Changes since v1 at [0]
> - also cleanup the HCD as suggested by Minas (thank you!)
> - updated the subject accordingly
>
>
> [0]
> https://urldefense.com/v3/__https://patchwork.kernel.org/patch/11631381/__;!!A4F2R9G_pg!K0ZJIzYVYtrwy2VgxTI28Z_qA995uqmg1dFrMNx3XMQ653ROMKPkt_zQdJe1OOk$
>
>
> drivers/usb/dwc2/platform.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index c347d93eae64..9febae441069 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -582,12 +582,16 @@ static int dwc2_driver_probe(struct platform_device
> *dev)
> retval = usb_add_gadget_udc(hsotg->dev, &hsotg->gadget);
> if (retval) {
> dwc2_hsotg_remove(hsotg);
> - goto error_init;
> + goto error_debugfs;
> }
> }
> #endif /* CONFIG_USB_DWC2_PERIPHERAL || CONFIG_USB_DWC2_DUAL_ROLE */
> return 0;
>
> +error_debugfs:
> + dwc2_debugfs_exit(hsotg);
> + if (hsotg->hcd_enabled)
> + dwc2_hcd_remove(hsotg);
> error_init:
> if (hsotg->params.activate_stm_id_vb_detection)
> regulator_disable(hsotg->usb33d);
>
Re: [PATCH 0/3] Add USB role switch support to DWC2
Hi, On 6/16/2020 6:07 PM, Amelie Delaunay wrote: > When using usb-c connector (but it can also be the case with a micro-b > connector), iddig, avalid, bvalid, vbusvalid input signals may not be > connected to the DWC2 OTG controller. > DWC2 OTG controller features an overriding control of the PHY voltage valid > and ID input signals. > So, missing signals can be forced using usb role from usb role switch and > this override feature. > > This series adds support for usb role switch to dwc2, by using overriding > control of the PHY voltage valid and ID input signals. > > It has been tested on stm32mp157c-dk2 [1], which has a Type-C connector > managed by a Type-C port controller, and connected to USB OTG controller. > > [1] > https://urldefense.com/v3/__https://www.st.com/en/evaluation-tools/stm32mp157c-dk2.html__;!!A4F2R9G_pg!KPU6pLcAMaVrFxzSp3peLkjZzWJzjsc__Kl878UYSLY2Cnv5NMmwACsY4kTCow0$ > > Amelie Delaunay (3): >usb: dwc2: override PHY input signals with usb role switch support >usb: dwc2: don't use ID/Vbus detection if usb-role-switch on STM32MP15 > SoCs >ARM: dts: stm32: enable usb-role-switch on USB OTG on stm32mp15xx-dkx > > arch/arm/boot/dts/stm32mp15xx-dkx.dtsi | 2 +- > drivers/usb/dwc2/Kconfig | 1 + > drivers/usb/dwc2/Makefile | 2 +- > drivers/usb/dwc2/core.h| 8 ++ > drivers/usb/dwc2/drd.c | 190 + > drivers/usb/dwc2/gadget.c | 2 +- > drivers/usb/dwc2/params.c | 4 +- > drivers/usb/dwc2/platform.c| 13 ++ > 8 files changed, 218 insertions(+), 4 deletions(-) > create mode 100644 drivers/usb/dwc2/drd.c > For dwc2: Acked-by Minas Harutyunyan Thanks, Minas
Re: [PATCH] usb: dwc2: Fix error path in gadget registration
Hi Marek, On 7/14/2020 12:48 PM, Marek Szyprowski wrote: > When gadget registration fails, one should not call usb_del_gadget_udc(). > Ensure this by setting gadget->udc to NULL. Also in case of a failure I was able to reproduce issue. I'm Ok with this fix. > there is no need to disable low-level hardware, so return immiedetly > instead of jumping to error_init label. > Why do you think that disable low-level hardware not required which was enabled before? Also for some platforms required to call regulator_disable() which was enabled earlier in probe function. So, I suggest to keep jump to error_init label. > This fixes the following kernel NULL ptr dereference on gadget failure > (can be easily triggered with g_mass_storage without any module > parameters): > Thanks, Minas
Re: [PATCH] sr: dwc2/gadget: remove unneccessary if
Hi Pavel,
On 6/6/2020 7:37 PM, Pavel Machek wrote:
> We don't really need if/else to set variable to 1/0.
>
> Signed-off-by: Pavel Machek (CIP)
>
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 12b98b466287..f9f6fd470c81 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -1761,10 +1761,7 @@ static int dwc2_hsotg_process_req_feature(struct
> dwc2_hsotg *hsotg,
> case USB_RECIP_DEVICE:
> switch (wValue) {
> case USB_DEVICE_REMOTE_WAKEUP:
> - if (set)
> - hsotg->remote_wakeup_allowed = 1;
> - else
> - hsotg->remote_wakeup_allowed = 0;
> + hsotg->remote_wakeup_allowed = set;
> break;
>
> case USB_DEVICE_TEST_MODE:
>
It's good catch, but 'set' declared as 'bool' while
'remote_wakeup_allowed' is 'unsigned int'. Maybe update 'set' type to same.
Thanks,
Minas
Re: [PATCH] usb: dwc2: Avoid leaving the error_debugfs label unused
On 10/17/2020 8:50 PM, Martin Blumenstingl wrote:
> The error_debugfs label is only used when either
> CONFIG_USB_DWC2_PERIPHERAL or CONFIG_USB_DWC2_DUAL_ROLE is enabled. Add
> the same #if to the error_debugfs label itself as the code which uses
> this label already has.
>
> This avoids the following compiler warning:
>warning: label ‘error_debugfs’ defined but not used [-Wunused-label]
>
> Fixes: e1c08cf23172ed ("usb: dwc2: Add missing cleanups when
> usb_add_gadget_udc() fails")
> Reported-by: kernel test robot
> Reported-by: Jens Axboe
> Signed-off-by: Martin Blumenstingl
> ---
> drivers/usb/dwc2/platform.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index e2820676beb1..5f18acac7406 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -608,10 +608,13 @@ static int dwc2_driver_probe(struct platform_device
> *dev)
> #endif /* CONFIG_USB_DWC2_PERIPHERAL || CONFIG_USB_DWC2_DUAL_ROLE */
> return 0;
>
> +#if IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL) || \
> + IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)
> error_debugfs:
> dwc2_debugfs_exit(hsotg);
> if (hsotg->hcd_enabled)
> dwc2_hcd_remove(hsotg);
> +#endif
> error_drd:
> dwc2_drd_exit(hsotg);
>
>
Acked-by: Minas Harutyunyan
Re: [PATCH] usb: dwc2: Fix unused label warning
On 10/31/2020 10:03 AM, YueHaibing wrote: > drivers/usb/dwc2/platform.c: In function ‘dwc2_driver_probe’: > drivers/usb/dwc2/platform.c:611:1: warning: label ‘error_debugfs’ defined but > not used [-Wunused-label] > error_debugfs: > ^ > > Move label 'error_debugfs' to ifdef block. > > Signed-off-by: YueHaibing > --- > drivers/usb/dwc2/platform.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c > index e2820676beb1..5f18acac7406 100644 > --- a/drivers/usb/dwc2/platform.c > +++ b/drivers/usb/dwc2/platform.c > @@ -608,10 +608,13 @@ static int dwc2_driver_probe(struct platform_device > *dev) > #endif /* CONFIG_USB_DWC2_PERIPHERAL || CONFIG_USB_DWC2_DUAL_ROLE */ > return 0; > > +#if IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL) || \ > + IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE) > error_debugfs: > dwc2_debugfs_exit(hsotg); > if (hsotg->hcd_enabled) > dwc2_hcd_remove(hsotg); > +#endif > error_drd: > dwc2_drd_exit(hsotg); > > Thank you for patch. Identical patch submitted by on 10/17/2020 by Martin Blumenstingl: [PATCH] usb: dwc2: Avoid leaving the error_debugfs label unused. I'm already ACKed it. Thanks, Minas
Re: [PATCH for-5.8 v2] usb: dwc2: Add missing cleanups when usb_add_gadget_udc() fails
Hi Martin,
On 7/4/2020 2:50 AM, Martin Blumenstingl wrote:
> Call dwc2_debugfs_exit() and dwc2_hcd_remove() (if the HCD was enabled
> earlier) when usb_add_gadget_udc() has failed. This ensures that the
> debugfs entries created by dwc2_debugfs_init() as well as the HCD are
> cleaned up in the error path.
>
> Fixes: 207324a321a866 ("usb: dwc2: Postponed gadget registration to the udc
> class driver")
> Signed-off-by: Martin Blumenstingl
> ---
> Changes since v1 at [0]
> - also cleanup the HCD as suggested by Minas (thank you!)
> - updated the subject accordingly
>
>
> [0]
> https://urldefense.com/v3/__https://patchwork.kernel.org/patch/11631381/__;!!A4F2R9G_pg!K0ZJIzYVYtrwy2VgxTI28Z_qA995uqmg1dFrMNx3XMQ653ROMKPkt_zQdJe1OOk$
>
>
> drivers/usb/dwc2/platform.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index c347d93eae64..9febae441069 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -582,12 +582,16 @@ static int dwc2_driver_probe(struct platform_device
> *dev)
> retval = usb_add_gadget_udc(hsotg->dev, &hsotg->gadget);
> if (retval) {
> dwc2_hsotg_remove(hsotg);
> - goto error_init;
> + goto error_debugfs;
> }
> }
> #endif /* CONFIG_USB_DWC2_PERIPHERAL || CONFIG_USB_DWC2_DUAL_ROLE */
> return 0;
>
Kernel test robot found issue:
>> warning: unused label 'error_debugfs' [-Wunused-label]
error_debugfs:
^~
1 warning generated.
So, 'error_debugfs:' label should be under #if/#endif:
#if IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL) || \
IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)
error_debugfs:
#endif /* CONFIG_USB_DWC2_PERIPHERAL || CONFIG_USB_DWC2_DUAL_ROLE */
Or you have other suggestion?
Could you please fix this issue and submit new version of patch.
Thanks,
Minas
> +error_debugfs:
> + dwc2_debugfs_exit(hsotg);
> + if (hsotg->hcd_enabled)
> + dwc2_hcd_remove(hsotg);
> error_init:
> if (hsotg->params.activate_stm_id_vb_detection)
> regulator_disable(hsotg->usb33d);
>
Re: [PATCH v2 1/8] usb: dwc2: gadget: Make use of GINTMSK2
Hi Felipe,
On 7/21/2020 1:43 PM, Felipe Balbi wrote:
> Minas Harutyunyan writes:
>
>> On 7/15/2020 1:32 PM, Lee Jones wrote:
>>> The value obtained from GINTSTS2 should be masked with the GINTMSK2
>>> value. Looks like this has been broken since
>>> dwc2_gadget_wkup_alert_handler() was added back in 2018.
>>>
>>> Also fixes the following W=1 warning:
>>>
>>>drivers/usb/dwc2/gadget.c: In function ‘dwc2_gadget_wkup_alert_handler’:
>>>drivers/usb/dwc2/gadget.c:259:6: warning: variable ‘gintmsk2’ set but
>>> not used [-Wunused-but-set-variable]
>>>259 | u32 gintmsk2;
>>>| ^~~~
>>>
>>> Cc: Minas Harutyunyan
>>> Cc: Ben Dooks
>>> Fixes: 187c5298a1229 ("usb: dwc2: gadget: Add handler for WkupAlert
>>> interrupt")
>>> Signed-off-by: Lee Jones
>>
>> Acked-by: Minas Harutyunyan
>
> Should I apply the entire series or only 1/8?
>
In this series only 2 patches are related to dwc2, which I'm already Acked:
[PATCH v2 1/8] usb: dwc2: gadget: Make use of GINTMSK2
[PATCH v2 2/8] usb: dwc2: gadget: Avoid pointless read of EP control
register
I can't acked other patches from this series, because they are not
related to dwc2.
Thanks,
Minas
Re: [PATCH v2 1/8] usb: dwc2: gadget: Make use of GINTMSK2
On 7/15/2020 1:32 PM, Lee Jones wrote:
> The value obtained from GINTSTS2 should be masked with the GINTMSK2
> value. Looks like this has been broken since
> dwc2_gadget_wkup_alert_handler() was added back in 2018.
>
> Also fixes the following W=1 warning:
>
> drivers/usb/dwc2/gadget.c: In function ‘dwc2_gadget_wkup_alert_handler’:
> drivers/usb/dwc2/gadget.c:259:6: warning: variable ‘gintmsk2’ set but not
> used [-Wunused-but-set-variable]
> 259 | u32 gintmsk2;
> | ^~~~
>
> Cc: Minas Harutyunyan
> Cc: Ben Dooks
> Fixes: 187c5298a1229 ("usb: dwc2: gadget: Add handler for WkupAlert
> interrupt")
> Signed-off-by: Lee Jones
Acked-by: Minas Harutyunyan
> ---
> Changelog:
>
> v2: Re-written to *use* instad of *remove* gintmsk2
>
> drivers/usb/dwc2/gadget.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index df5fedaca60a0..03cf1fa856219 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -260,6 +260,7 @@ static void dwc2_gadget_wkup_alert_handler(struct
> dwc2_hsotg *hsotg)
>
> gintsts2 = dwc2_readl(hsotg, GINTSTS2);
> gintmsk2 = dwc2_readl(hsotg, GINTMSK2);
> + gintsts2 &= gintmsk2;
>
> if (gintsts2 & GINTSTS2_WKUP_ALERT_INT) {
> dev_dbg(hsotg->dev, "%s: Wkup_Alert_Int\n", __func__);
>
Re: [PATCH] usb: dwc2: Fix error path in gadget registration
Hi Marek, On 7/15/2020 12:42 PM, Marek Szyprowski wrote: > Hi Minas, > > On 14.07.2020 14:32, Minas Harutyunyan wrote: >> On 7/14/2020 12:48 PM, Marek Szyprowski wrote: >>> When gadget registration fails, one should not call usb_del_gadget_udc(). >>> Ensure this by setting gadget->udc to NULL. Also in case of a failure >> I was able to reproduce issue. I'm Ok with this fix. >> >>> there is no need to disable low-level hardware, so return immiedetly >>> instead of jumping to error_init label. >>> >> Why do you think that disable low-level hardware not required which was >> enabled before? Also for some platforms required to call >> regulator_disable() which was enabled earlier in probe function. >> So, I suggest to keep jump to error_init label. > > If I keep the jump to error_init label, then there is unbalanced call to > dwc2_lowlevel_hw_disable(). usb_add_gadget_udc() can fail in 2 places: > on gadget->bind() or during udc_start(). In the first case, the HW was > not yet enabled, so there is no need to disable it. In the latter one, > the error might be returned only from the dwc2_lowlevel_hw_enable(), so > again there is no need to call dwc2_lowlevel_hw_disable(). > > If I keep the "goto error_init;" line, I get the following errors: > > dwc2 1248.hsotg: dwc2_check_params: Invalid parameter besl=1 > dwc2 1248.hsotg: dwc2_check_params: Invalid parameter > g_np_tx_fifo_size=1024 > dwc2 1248.hsotg: EPs: 16, dedicated fifos, 7808 entries in SPRAM > Mass Storage Function, version: 2009/09/11 > LUN: removable file: (no medium) > no file given for LUN0 > g_mass_storage 1248.hsotg: failed to start g_mass_storage: -22 > [ cut here ] > WARNING: CPU: 3 PID: 49 at drivers/clk/clk.c:958 > clk_core_disable+0x1e4/0x314 > usb_device already disabled > Modules linked in: > CPU: 3 PID: 49 Comm: kworker/3:1 Not tainted > 5.8.0-rc5-next-20200714-3-g105f360ba595-dirty #8758 > Hardware name: Samsung Exynos (Flattened Device Tree) > Workqueue: events deferred_probe_work_func > [] (unwind_backtrace) from [] (show_stack+0x10/0x14) > [] (show_stack) from [] (dump_stack+0xbc/0xe8) > [] (dump_stack) from [] (__warn+0xf0/0x108) > [] (__warn) from [] (warn_slowpath_fmt+0x74/0xb8) > [] (warn_slowpath_fmt) from [] > (clk_core_disable+0x1e4/0x314) > [] (clk_core_disable) from [] > (clk_core_disable_lock+0x18/0x24) > [] (clk_core_disable_lock) from [] > (__dwc2_lowlevel_hw_disable+0x3c/0xa0) > [] (__dwc2_lowlevel_hw_disable) from [] > (dwc2_driver_probe+0x2d4/0x6ac) > [] (dwc2_driver_probe) from [] > (platform_drv_probe+0x6c/0xa4) > [] (platform_drv_probe) from [] > (really_probe+0x200/0x4fc) > [] (really_probe) from [] > (driver_probe_device+0x78/0x1fc) > [] (driver_probe_device) from [] > (bus_for_each_drv+0x74/0xb8) > [] (bus_for_each_drv) from [] > (__device_attach+0xd4/0x16c) > [] (__device_attach) from [] > (bus_probe_device+0x88/0x90) > [] (bus_probe_device) from [] > (deferred_probe_work_func+0x3c/0xd0) > [] (deferred_probe_work_func) from [] > (process_one_work+0x234/0x7dc) > [] (process_one_work) from [] (worker_thread+0x44/0x51c) > [] (worker_thread) from [] (kthread+0x158/0x1a0) > [] (kthread) from [] (ret_from_fork+0x14/0x20) > Exception stack(0xee923fb0 to 0xee923ff8) > ... > irq event stamp: 36966 > hardirqs last enabled at (36965): [] kfree+0x1a4/0x3f0 > hardirqs last disabled at (36966): [] clk_enable_lock+0x14/0x134 > softirqs last enabled at (36814): [] __do_softirq+0x50c/0x608 > softirqs last disabled at (36803): [] irq_exit+0x168/0x16c > ---[ end trace f55f4b28f3080c12 ]--- > [ cut here ] > WARNING: CPU: 3 PID: 49 at drivers/clk/clk.c:817 > clk_core_unprepare+0x33c/0x470 > usb_device already unprepared > Modules linked in: > CPU: 3 PID: 49 Comm: kworker/3:1 Tainted: G W > 5.8.0-rc5-next-20200714-3-g105f360ba595-dirty #8758 > Hardware name: Samsung Exynos (Flattened Device Tree) > Workqueue: events deferred_probe_work_func > [] (unwind_backtrace) from [] (show_stack+0x10/0x14) > [] (show_stack) from [] (dump_stack+0xbc/0xe8) > [] (dump_stack) from [] (__warn+0xf0/0x108) > [] (__warn) from [] (warn_slowpath_fmt+0x74/0xb8) > [] (warn_slowpath_fmt) from [] > (clk_core_unprepare+0x33c/0x470) > [] (clk_core_unprepare) from [] > (clk_unprepare+0x24/0x2c) > [] (clk_unprepare) from [] > (__dwc2_lowlevel_hw_disable+0x44/0xa0) > [] (__dwc2_lowlevel_hw_disable) from [] > (dwc2_driver_probe+0x2d4/0x6ac) > [] (dwc2_driver_probe) from [] > (platform_drv_probe+0x6c/0xa4) > [] (platform_drv_probe) from [] > (really_probe+0x200/0x4fc) > [] (really_probe) from []
Re: [RFC for-5.8] usb: dwc2: Cleanup debugfs when usb_add_gadget_udc() fails
Hi Felipe,
On 7/23/2020 5:03 PM, Felipe Balbi wrote:
> Minas Harutyunyan writes:
>
>> Hi Martin,
>>
>> On 6/29/2020 10:03 PM, Martin Blumenstingl wrote:
>>> Call dwc2_debugfs_exit() when usb_add_gadget_udc() has failed. This
>>> ensure that the debugfs entries created by dwc2_debugfs_init() are
>>> cleaned up in the error path.
>>>
>>> Fixes: 207324a321a866 ("usb: dwc2: Postponed gadget registration to the udc
>>> class driver")
>>> Signed-off-by: Martin Blumenstingl
>>> ---
>>> This patch is compile-tested only. I found this while trying to
>>> understand the latest changes to dwc2/platform.c.
>>>
>>>
>>>drivers/usb/dwc2/platform.c | 4 +++-
>>>1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
>>> index c347d93eae64..02b6da7e21d7 100644
>>> --- a/drivers/usb/dwc2/platform.c
>>> +++ b/drivers/usb/dwc2/platform.c
>>> @@ -582,12 +582,14 @@ static int dwc2_driver_probe(struct platform_device
>>> *dev)
>>> retval = usb_add_gadget_udc(hsotg->dev, &hsotg->gadget);
>>> if (retval) {
>>> dwc2_hsotg_remove(hsotg);
>>> - goto error_init;
>>> + goto error_debugfs;
>>> }
>>> }
>>>#endif /* CONFIG_USB_DWC2_PERIPHERAL || CONFIG_USB_DWC2_DUAL_ROLE */
>>> return 0;
>>>
>>> +error_debugfs:
>>> + dwc2_debugfs_exit(hsotg);
>>>error_init:
>>> if (hsotg->params.activate_stm_id_vb_detection)
>>> regulator_disable(hsotg->usb33d);
>>>
>> I'm Ok with this fix. One more thing. I missed to remove hcd also in
>> fail case. Could you please add dwc2_hcd_remove() call after
>> dwc2_debugfs_exit(hsotg) and submit as patch:
>>
>> +error_debugfs:
>> +dwc2_debugfs_exit(hsotg);
>> +if (hsotg->hcd_enabled)
>> +dwc2_hcd_remove(hsotg);
>> error_init:
>
> looks like it should be a separate patch though. Right?
v2 of this patch already submitted by Martin Blumenstingl
on 7/4/2020 with fix related to
dwc2_hcd_remove() and I acked it:
[PATCH for-5.8 v2] usb: dwc2: Add missing cleanups when
usb_add_gadget_udc() fails
Thanks,
Minas
>
Re: [PATCH v2] usb: dwc2: Fix error path in gadget registration
Hi Marek,
On 7/16/2020 4:09 PM, Marek Szyprowski wrote:
> When gadget registration fails, one should not call usb_del_gadget_udc().
> Ensure this by setting gadget->udc to NULL. Also in case of a failure
> there is no need to disable low-level hardware, so return immiedetly
> instead of jumping to error_init label.
>
> This fixes the following kernel NULL ptr dereference on gadget failure
> (can be easily triggered with g_mass_storage without any module
> parameters):
>
> dwc2 1248.hsotg: dwc2_check_params: Invalid parameter besl=1
> dwc2 1248.hsotg: dwc2_check_params: Invalid parameter
> g_np_tx_fifo_size=1024
> dwc2 1248.hsotg: EPs: 16, dedicated fifos, 7808 entries in SPRAM
> Mass Storage Function, version: 2009/09/11
> LUN: removable file: (no medium)
> no file given for LUN0
> g_mass_storage 1248.hsotg: failed to start g_mass_storage: -22
> 8<--- cut here ---
> Unable to handle kernel NULL pointer dereference at virtual address 0104
> pgd = (ptrval)
> [0104] *pgd=
> Internal error: Oops: 805 [#1] PREEMPT SMP ARM
> Modules linked in:
> CPU: 0 PID: 12 Comm: kworker/0:1 Not tainted 5.8.0-rc5 #3133
> Hardware name: Samsung Exynos (Flattened Device Tree)
> Workqueue: events deferred_probe_work_func
> PC is at usb_del_gadget_udc+0x38/0xc4
> LR is at __mutex_lock+0x31c/0xb18
> ...
> Process kworker/0:1 (pid: 12, stack limit = 0x(ptrval))
> Stack: (0xef121db0 to 0xef122000)
> ...
> [] (usb_del_gadget_udc) from []
> (dwc2_hsotg_remove+0x10/0x20)
> [] (dwc2_hsotg_remove) from []
> (dwc2_driver_probe+0x57c/0x69c)
> [] (dwc2_driver_probe) from []
> (platform_drv_probe+0x6c/0xa4)
> [] (platform_drv_probe) from [] (really_probe+0x200/0x48c)
> [] (really_probe) from [] (driver_probe_device+0x78/0x1fc)
> [] (driver_probe_device) from []
> (bus_for_each_drv+0x74/0xb8)
> [] (bus_for_each_drv) from [] (__device_attach+0xd4/0x16c)
> [] (__device_attach) from [] (bus_probe_device+0x88/0x90)
> [] (bus_probe_device) from []
> (deferred_probe_work_func+0x3c/0xd0)
> [] (deferred_probe_work_func) from []
> (process_one_work+0x234/0x7dc)
> [] (process_one_work) from [] (worker_thread+0x44/0x51c)
> [] (worker_thread) from [] (kthread+0x158/0x1a0)
> [] (kthread) from [] (ret_from_fork+0x14/0x20)
> Exception stack(0xef121fb0 to 0xef121ff8)
> ...
> ---[ end trace 9724c2fc7cc9c982 ]---
>
> While fixing this also fix the double call to dwc2_lowlevel_hw_disable()
> if dr_mode is set to USB_DR_MODE_PERIPHERAL. In such case low-level
> hardware is already disabled before calling usb_add_gadget_udc(). That
> function correctly preserves low-level hardware state, there is no need
> for the second unconditional dwc2_lowlevel_hw_disable() call.
>
> Fixes: 207324a321a8 ("usb: dwc2: Postponed gadget registration to the udc
> class driver")
> Signed-off-by: Marek Szyprowski
> ---
> drivers/usb/dwc2/platform.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index cb8ddbd53718..db9fd4bd1a38 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -582,6 +582,7 @@ static int dwc2_driver_probe(struct platform_device *dev)
> if (hsotg->gadget_enabled) {
> retval = usb_add_gadget_udc(hsotg->dev, &hsotg->gadget);
> if (retval) {
> + hsotg->gadget.udc = NULL;
Consider your recently sent patch "[PATCH] usb: gadget: udc: Flush
pending work also in error path", more probably it's not required,
because root cause of observed dwc2 issue comes from udc.
Am I wrong?
Or we can keep it as sanity solution to avoid any other possible cases?
> dwc2_hsotg_remove(hsotg);
> goto error_init;
> }
> @@ -593,7 +594,8 @@ static int dwc2_driver_probe(struct platform_device *dev)
> if (hsotg->params.activate_stm_id_vb_detection)
> regulator_disable(hsotg->usb33d);
> error:
> - dwc2_lowlevel_hw_disable(hsotg);
> + if (hsotg->dr_mode != USB_DR_MODE_PERIPHERAL)
> + dwc2_lowlevel_hw_disable(hsotg);
> return retval;
> }
>
>
Thanks,
Minas
Re: [PATCH v2] usb: dwc2: Fix error path in gadget registration
Hi,
On 7/16/2020 4:33 PM, Marek Szyprowski wrote:
> Hi Minas,
>
> On 16.07.2020 14:19, Minas Harutyunyan wrote:
>> On 7/16/2020 4:09 PM, Marek Szyprowski wrote:
>>> When gadget registration fails, one should not call usb_del_gadget_udc().
>>> Ensure this by setting gadget->udc to NULL. Also in case of a failure
>>> there is no need to disable low-level hardware, so return immiedetly
>>> instead of jumping to error_init label.
>>>
>>> This fixes the following kernel NULL ptr dereference on gadget failure
>>> (can be easily triggered with g_mass_storage without any module
>>> parameters):
>>>
>>> dwc2 1248.hsotg: dwc2_check_params: Invalid parameter besl=1
>>> dwc2 1248.hsotg: dwc2_check_params: Invalid parameter
>>> g_np_tx_fifo_size=1024
>>> dwc2 1248.hsotg: EPs: 16, dedicated fifos, 7808 entries in SPRAM
>>> Mass Storage Function, version: 2009/09/11
>>> LUN: removable file: (no medium)
>>> no file given for LUN0
>>> g_mass_storage 1248.hsotg: failed to start g_mass_storage: -22
>>> 8<--- cut here ---
>>> Unable to handle kernel NULL pointer dereference at virtual address 0104
>>> pgd = (ptrval)
>>> [0104] *pgd=
>>> Internal error: Oops: 805 [#1] PREEMPT SMP ARM
>>> Modules linked in:
>>> CPU: 0 PID: 12 Comm: kworker/0:1 Not tainted 5.8.0-rc5 #3133
>>> Hardware name: Samsung Exynos (Flattened Device Tree)
>>> Workqueue: events deferred_probe_work_func
>>> PC is at usb_del_gadget_udc+0x38/0xc4
>>> LR is at __mutex_lock+0x31c/0xb18
>>> ...
>>> Process kworker/0:1 (pid: 12, stack limit = 0x(ptrval))
>>> Stack: (0xef121db0 to 0xef122000)
>>> ...
>>> [] (usb_del_gadget_udc) from []
>>> (dwc2_hsotg_remove+0x10/0x20)
>>> [] (dwc2_hsotg_remove) from []
>>> (dwc2_driver_probe+0x57c/0x69c)
>>> [] (dwc2_driver_probe) from []
>>> (platform_drv_probe+0x6c/0xa4)
>>> [] (platform_drv_probe) from []
>>> (really_probe+0x200/0x48c)
>>> [] (really_probe) from []
>>> (driver_probe_device+0x78/0x1fc)
>>> [] (driver_probe_device) from []
>>> (bus_for_each_drv+0x74/0xb8)
>>> [] (bus_for_each_drv) from []
>>> (__device_attach+0xd4/0x16c)
>>> [] (__device_attach) from []
>>> (bus_probe_device+0x88/0x90)
>>> [] (bus_probe_device) from []
>>> (deferred_probe_work_func+0x3c/0xd0)
>>> [] (deferred_probe_work_func) from []
>>> (process_one_work+0x234/0x7dc)
>>> [] (process_one_work) from [] (worker_thread+0x44/0x51c)
>>> [] (worker_thread) from [] (kthread+0x158/0x1a0)
>>> [] (kthread) from [] (ret_from_fork+0x14/0x20)
>>> Exception stack(0xef121fb0 to 0xef121ff8)
>>> ...
>>> ---[ end trace 9724c2fc7cc9c982 ]---
>>>
>>> While fixing this also fix the double call to dwc2_lowlevel_hw_disable()
>>> if dr_mode is set to USB_DR_MODE_PERIPHERAL. In such case low-level
>>> hardware is already disabled before calling usb_add_gadget_udc(). That
>>> function correctly preserves low-level hardware state, there is no need
>>> for the second unconditional dwc2_lowlevel_hw_disable() call.
>>>
>>> Fixes: 207324a321a8 ("usb: dwc2: Postponed gadget registration to the udc
>>> class driver")
>>> Signed-off-by: Marek Szyprowski
Acked-by: Minas Harutyunyan
>>> ---
>>> drivers/usb/dwc2/platform.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
>>> index cb8ddbd53718..db9fd4bd1a38 100644
>>> --- a/drivers/usb/dwc2/platform.c
>>> +++ b/drivers/usb/dwc2/platform.c
>>> @@ -582,6 +582,7 @@ static int dwc2_driver_probe(struct platform_device
>>> *dev)
>>> if (hsotg->gadget_enabled) {
>>> retval = usb_add_gadget_udc(hsotg->dev, &hsotg->gadget);
>>> if (retval) {
>>> + hsotg->gadget.udc = NULL;
>> Consider your recently sent patch "[PATCH] usb: gadget: udc: Flush
>> pending work also in error path", more probably it's not required,
>> because root cause of observed dwc2 issue comes from udc.
>> Am I wrong?
>
> They are two independent issues, both fatal on my test system.
>
> The first one (the lack of NULLing gadget->udc) can be easily triggered
> on any system. The latter one (lack of flush in UDC core) depends on the
> luck and memory layout on the test system.
>
> Best regards
>
Re: [PATCH v6 0/3] Add USB role switch support to DWC2
On 9/24/2020 4:27 PM, Amelie DELAUNAY wrote: > Gentle reminder on the whole series instead. > > Thanks, > Amelie > > On 9/9/20 11:35 AM, Amelie DELAUNAY wrote: >> When using usb-c connector (but it can also be the case with a micro-b >> connector), iddig, avalid, bvalid, vbusvalid input signals may not be >> connected to the DWC2 OTG controller. >> DWC2 OTG controller features an overriding control of the PHY voltage >> valid >> and ID input signals. >> So, missing signals can be forced using usb role from usb role switch >> and >> this override feature. >> >> This series adds support for usb role switch to dwc2, by using >> overriding >> control of the PHY voltage valid and ID input signals. >> >> It has been tested on stm32mp157c-dk2 [1], which has a Type-C connector >> managed by a Type-C port controller, and connected to USB OTG >> controller. >> >> [1] >> https://urldefense.com/v3/__https://www.st.com/en/evaluation-tools/stm32mp157c-dk2.html__;!!A4F2R9G_pg!LArZ8m2rAg5r1gjIUgMe3YNtFeRB8li8yKNkU0n3UqbgNZADD96VXRTHT7BLT4o$ >> >> >> Amelie Delaunay (3): >> dt-bindings: usb: dwc2: add optional usb-role-switch property >> usb: dwc2: override PHY input signals with usb role switch support >> usb: dwc2: don't use ID/Vbus detection if usb-role-switch on >> STM32MP15 >> SoCs >> --- >> Changes in v6: >> - Select USB_ROLE_SWITCH if USB_DWC2, and not only if >> USB_DWC2_DUAL_ROLE: >> drd.c is built whatever DWC2 mode (DUAL, HOST, PERIPHERAL) as it >> is used also >> to detect attach/detach (so a-valid/b-valid sessions). >> Changes in v5: >> - Use device_property_read_bool instead of of_read_property_bool in >> params.c >> Changes in v4: >> - Simplify call to dwc2_force_mode in drd.c >> - Add error_drd label in probe error path in platform.c >> Changes in v3: >> - Fix build issue reported by kernel test robot in drd.c >> Changes in v2: >> - Previous DT patch already in stm32-next branch so removed from v2 >> patchset >> "ARM: dts: stm32: enable usb-role-switch on USB OTG on >> stm32mp15xx-dkx" >> - DWC2 DT bindings update added >> - Build issue reported by kernel test robot fixed >> - Martin's comments taken into account >> --- >> .../devicetree/bindings/usb/dwc2.yaml | 4 + >> drivers/usb/dwc2/Kconfig | 1 + >> drivers/usb/dwc2/Makefile | 2 +- >> drivers/usb/dwc2/core.h | 9 + >> drivers/usb/dwc2/drd.c | 180 ++ >> drivers/usb/dwc2/gadget.c | 2 +- >> drivers/usb/dwc2/params.c | 2 +- >> drivers/usb/dwc2/platform.c | 20 +- >> 8 files changed, 215 insertions(+), 5 deletions(-) >> create mode 100644 drivers/usb/dwc2/drd.c >> Acked-by: Minas Harutyunyan for dwc2
Re: [RFC for-5.8] usb: dwc2: Cleanup debugfs when usb_add_gadget_udc() fails
Hi Martin,
On 6/29/2020 10:03 PM, Martin Blumenstingl wrote:
> Call dwc2_debugfs_exit() when usb_add_gadget_udc() has failed. This
> ensure that the debugfs entries created by dwc2_debugfs_init() are
> cleaned up in the error path.
>
> Fixes: 207324a321a866 ("usb: dwc2: Postponed gadget registration to the udc
> class driver")
> Signed-off-by: Martin Blumenstingl
> ---
> This patch is compile-tested only. I found this while trying to
> understand the latest changes to dwc2/platform.c.
>
>
> drivers/usb/dwc2/platform.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index c347d93eae64..02b6da7e21d7 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -582,12 +582,14 @@ static int dwc2_driver_probe(struct platform_device
> *dev)
> retval = usb_add_gadget_udc(hsotg->dev, &hsotg->gadget);
> if (retval) {
> dwc2_hsotg_remove(hsotg);
> - goto error_init;
> + goto error_debugfs;
> }
> }
> #endif /* CONFIG_USB_DWC2_PERIPHERAL || CONFIG_USB_DWC2_DUAL_ROLE */
> return 0;
>
> +error_debugfs:
> + dwc2_debugfs_exit(hsotg);
> error_init:
> if (hsotg->params.activate_stm_id_vb_detection)
> regulator_disable(hsotg->usb33d);
>
I'm Ok with this fix. One more thing. I missed to remove hcd also in
fail case. Could you please add dwc2_hcd_remove() call after
dwc2_debugfs_exit(hsotg) and submit as patch:
+error_debugfs:
+ dwc2_debugfs_exit(hsotg);
+ if (hsotg->hcd_enabled)
+ dwc2_hcd_remove(hsotg);
error_init:
Thanks,
Minas
Re: [PATCH] usb: dwc2: host: Fix wMaxPacketSize handling (fix webcam regression)
On 6/1/2019 12:05 AM, Douglas Anderson wrote:
In commit abb621844f6a ("usb: ch9: make usb_endpoint_maxp() return
only packet size") the API to usb_endpoint_maxp() changed. It used to
just return wMaxPacketSize but after that commit it returned
wMaxPacketSize with the high bits (the multiplier) masked off. If you
wanted to get the multiplier it was now up to your code to call the
new usb_endpoint_maxp_mult() which was introduced in
commit 541b6fe63023 ("usb: add helper to extract bits 12:11 of
wMaxPacketSize").
Prior to the API change most host drivers were updated, but no update
was made to dwc2. Presumably it was assumed that dwc2 was too
simplistic to use the multiplier and thus just didn't support a
certain class of USB devices. However, it turns out that dwc2 did use
the multiplier and many devices using it were working quite nicely.
That means that many USB devices have been broken since the API
change. One such device is a Logitech HD Pro Webcam C920.
Specifically, though dwc2 didn't directly call usb_endpoint_maxp(), it
did call usb_maxpacket() which in turn called usb_endpoint_maxp().
Let's update dwc2 to work properly with the new API.
Fixes: abb621844f6a ("usb: ch9: make usb_endpoint_maxp() return only packet
size")
Cc: sta...@vger.kernel.org
Signed-off-by: Douglas Anderson
---
Acked-by: Minas Harutyunyan
drivers/usb/dwc2/hcd.c | 29 +
drivers/usb/dwc2/hcd.h | 20 +++-
drivers/usb/dwc2/hcd_intr.c | 5 +++--
drivers/usb/dwc2/hcd_queue.c | 10 ++
4 files changed, 37 insertions(+), 27 deletions(-)
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index b50ec3714fd8..5c51bf5506d1 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -2608,7 +2608,7 @@ static int dwc2_assign_and_init_hc(struct dwc2_hsotg
*hsotg, struct dwc2_qh *qh)
chan->dev_addr = dwc2_hcd_get_dev_addr(&urb->pipe_info);
chan->ep_num = dwc2_hcd_get_ep_num(&urb->pipe_info);
chan->speed = qh->dev_speed;
- chan->max_packet = dwc2_max_packet(qh->maxp);
+ chan->max_packet = qh->maxp;
chan->xfer_started = 0;
chan->halt_status = DWC2_HC_XFER_NO_HALT_STATUS;
@@ -2686,7 +2686,7 @@ static int dwc2_assign_and_init_hc(struct dwc2_hsotg
*hsotg, struct dwc2_qh *qh)
* This value may be modified when the transfer is started
* to reflect the actual transfer length
*/
- chan->multi_count = dwc2_hb_mult(qh->maxp);
+ chan->multi_count = qh->maxp_mult;
if (hsotg->params.dma_desc_enable) {
chan->desc_list_addr = qh->desc_list_dma;
@@ -3806,19 +3806,21 @@ static struct dwc2_hcd_urb *dwc2_hcd_urb_alloc(struct
dwc2_hsotg *hsotg,
static void dwc2_hcd_urb_set_pipeinfo(struct dwc2_hsotg *hsotg,
struct dwc2_hcd_urb *urb, u8 dev_addr,
- u8 ep_num, u8 ep_type, u8 ep_dir, u16 mps)
+ u8 ep_num, u8 ep_type, u8 ep_dir,
+ u16 maxp, u16 maxp_mult)
{
if (dbg_perio() ||
ep_type == USB_ENDPOINT_XFER_BULK ||
ep_type == USB_ENDPOINT_XFER_CONTROL)
dev_vdbg(hsotg->dev,
-"addr=%d, ep_num=%d, ep_dir=%1x, ep_type=%1x,
mps=%d\n",
-dev_addr, ep_num, ep_dir, ep_type, mps);
+"addr=%d, ep_num=%d, ep_dir=%1x, ep_type=%1x, maxp=%d (%d
mult)\n",
+dev_addr, ep_num, ep_dir, ep_type, maxp, maxp_mult);
urb->pipe_info.dev_addr = dev_addr;
urb->pipe_info.ep_num = ep_num;
urb->pipe_info.pipe_type = ep_type;
urb->pipe_info.pipe_dir = ep_dir;
- urb->pipe_info.mps = mps;
+ urb->pipe_info.maxp = maxp;
+ urb->pipe_info.maxp_mult = maxp_mult;
}
/*
@@ -3909,8 +3911,9 @@ void dwc2_hcd_dump_state(struct dwc2_hsotg *hsotg)
dwc2_hcd_is_pipe_in(&urb->pipe_info) ?
"IN" : "OUT");
dev_dbg(hsotg->dev,
- " Max packet size: %d\n",
- dwc2_hcd_get_mps(&urb->pipe_info));
+ " Max packet size: %d (%d mult)\n",
+ dwc2_hcd_get_maxp(&urb->pipe_info),
+
dwc2_hcd_get_maxp_mult(&urb->pipe_info));
dev_dbg(hsotg->dev,
" transfer_buffer: %p\n",
Re: [PATCH] usb: dwc2: Fix DMA cache alignment issues
On 5/29/2019 10:31 PM, Doug Anderson wrote:
Hi,
On Sun, Feb 17, 2019 at 10:37 PM Martin Schiller wrote:
Insert a padding between data and the stored_xfer_buffer pointer to
ensure they are not on the same cache line.
Otherwise, the stored_xfer_buffer gets corrupted for IN URBs on
non-cache-coherent systems. (In my case: Lantiq xRX200 MIPS)
Fixes: 3bc04e28a030 ("usb: dwc2: host: Get aligned DMA in a more supported way")
Fixes: 56406e017a88 ("usb: dwc2: Fix DMA alignment to start at allocated
boundary")
Signed-off-by: Martin Schiller
---
drivers/usb/dwc2/hcd.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
This patch has been in the back of my mind for a while bug I never got
around to it. Today I was debugging memory corruption problems when
using a webcam on dwc2 on rk3288-veyron-jerry. This patch appears to
solve my problems nicely. Thanks!
Tested-by: Douglas Anderson
Reviewed-by: Douglas Anderson
Cc:
Acked-by: Minas Harutyunyan
Re: [PATCH] Revert "usb: dwc2: host: Setting qtd to NULL after freeing it"
On 5/30/2019 12:55 AM, Guenter Roeck wrote:
This reverts commit b0d659022e5c96ee5c4bd62d22d3da2d66de306b.
The reverted commit does nothing but adding two unnecessary lines
of code. It sets a local variable to NULL in two functions, but
that variable is not used anywhere in the rest of those functions.
This is just confusing, so let's remove it.
Cc: Vardan Mikayelyan
Cc: John Youn
Cc: Douglas Anderson
Cc: Felipe Balbi
Signed-off-by: Guenter Roeck
Acked-by: Minas Harutyunyan
---
drivers/usb/dwc2/hcd.c | 1 -
drivers/usb/dwc2/hcd.h | 1 -
2 files changed, 2 deletions(-)
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index b50ec3714fd8..bca64b0d4d15 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -4676,7 +4676,6 @@ static int _dwc2_hcd_urb_enqueue(struct usb_hcd *hcd,
struct urb *urb,
spin_unlock_irqrestore(&hsotg->lock, flags);
urb->hcpriv = NULL;
kfree(qtd);
- qtd = NULL;
fail1:
if (qh_allocated) {
struct dwc2_qtd *qtd2, *qtd2_tmp;
diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
index c089ffa1f0a8..f6bc48432b04 100644
--- a/drivers/usb/dwc2/hcd.h
+++ b/drivers/usb/dwc2/hcd.h
@@ -574,7 +574,6 @@ static inline void dwc2_hcd_qtd_unlink_and_free(struct
dwc2_hsotg *hsotg,
{
list_del(&qtd->qtd_list_entry);
kfree(qtd);
- qtd = NULL;
}
/* Descriptor DMA support functions */
Re: [PATCH] usb: dwc2: use well defined macros for power_down
Hi,
On 6/16/2020 12:26 PM, Jisheng Zhang wrote:
> Use the well defined macros such as DWC2_POWER_DOWN_PARAM_NONE,
> DWC2_POWER_DOWN_PARAM_PARTIAL and DWC2_POWER_DOWN_PARAM_HIBERNATION
> to make code more readable.
>
> Signed-off-by: Jisheng Zhang
Acked-by: Minas Harutyunyan
> ---
> drivers/usb/dwc2/hcd.c| 4 ++--
> drivers/usb/dwc2/params.c | 12 ++--
> 2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index b90f858af960..e9ac215b9663 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -3628,7 +3628,7 @@ static int dwc2_hcd_hub_control(struct dwc2_hsotg
> *hsotg, u16 typereq,
> "SetPortFeature - USB_PORT_FEAT_SUSPEND\n");
> if (windex != hsotg->otg_port)
> goto error;
> - if (hsotg->params.power_down == 2)
> + if (hsotg->params.power_down ==
> DWC2_POWER_DOWN_PARAM_HIBERNATION)
> dwc2_enter_hibernation(hsotg, 1);
> else
> dwc2_port_suspend(hsotg, windex);
> @@ -3646,7 +3646,7 @@ static int dwc2_hcd_hub_control(struct dwc2_hsotg
> *hsotg, u16 typereq,
> break;
>
> case USB_PORT_FEAT_RESET:
> - if (hsotg->params.power_down == 2 &&
> + if (hsotg->params.power_down ==
> DWC2_POWER_DOWN_PARAM_HIBERNATION &&
> hsotg->hibernated)
> dwc2_exit_hibernation(hsotg, 0, 1, 1);
> hprt0 = dwc2_read_hprt0(hsotg);
> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
> index ce736d67c7c3..8f9d061c4d5f 100644
> --- a/drivers/usb/dwc2/params.c
> +++ b/drivers/usb/dwc2/params.c
> @@ -68,14 +68,14 @@ static void dwc2_set_his_params(struct dwc2_hsotg *hsotg)
> p->ahbcfg = GAHBCFG_HBSTLEN_INCR16 <<
> GAHBCFG_HBSTLEN_SHIFT;
> p->change_speed_quirk = true;
> - p->power_down = false;
> + p->power_down = DWC2_POWER_DOWN_PARAM_NONE;
> }
>
> static void dwc2_set_s3c6400_params(struct dwc2_hsotg *hsotg)
> {
> struct dwc2_core_params *p = &hsotg->params;
>
> - p->power_down = 0;
> + p->power_down = DWC2_POWER_DOWN_PARAM_NONE;
> p->phy_utmi_width = 8;
> }
>
> @@ -89,7 +89,7 @@ static void dwc2_set_rk_params(struct dwc2_hsotg *hsotg)
> p->host_perio_tx_fifo_size = 256;
> p->ahbcfg = GAHBCFG_HBSTLEN_INCR16 <<
> GAHBCFG_HBSTLEN_SHIFT;
> - p->power_down = 0;
> + p->power_down = DWC2_POWER_DOWN_PARAM_NONE;
> }
>
> static void dwc2_set_ltq_params(struct dwc2_hsotg *hsotg)
> @@ -319,11 +319,11 @@ static void dwc2_set_param_power_down(struct dwc2_hsotg
> *hsotg)
> int val;
>
> if (hsotg->hw_params.hibernation)
> - val = 2;
> + val = DWC2_POWER_DOWN_PARAM_HIBERNATION;
> else if (hsotg->hw_params.power_optimized)
> - val = 1;
> + val = DWC2_POWER_DOWN_PARAM_PARTIAL;
> else
> - val = 0;
> + val = DWC2_POWER_DOWN_PARAM_NONE;
>
> hsotg->params.power_down = val;
> }
>
Re: [Query]usb: dwc2: suspend/resume support for DWC2_POWER_DOWN_PARAM_NONE case
Hi Jisheng, On 6/16/2020 1:03 PM, Jisheng Zhang wrote: > Hi, > > After reading current dwc2 code, I got an impression that resume from suspend > to ram isn't supported for DWC2_POWER_DOWN_PARAM_NONE case, right? In fact 'ram' Do you mean on suspend save registers in RAM? If yes, then in case when power_down is _NONE then no any register saving/restoring to/from RAM should be performing. > we do see usb device can't resume properly with DWC2_POWER_DOWN_PARAM_NONE > case. > If you see any issue on resume in mentioned case then more probably is some another issue which can debugged. > If the impression is true, what's the proper technical direction? Add > dwc2_host_enter_suspend() as dwc2_host_enter_hibernation() > and > dwc2_host_exit_suspend() as dwc2_host_exit_hibernation()? > > Thanks in advance, > Jisheng > Thanks, Minas
Re: [PATCH] sr: dwc2/gadget: remove unneccessary if
Hi,
On 6/6/2020 11:05 PM, Pavel Machek wrote:
> Hi!
>
>>> We don't really need if/else to set variable to 1/0.
>>>
>>> Signed-off-by: Pavel Machek (CIP)
>>>
>>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>>> index 12b98b466287..f9f6fd470c81 100644
>>> --- a/drivers/usb/dwc2/gadget.c
>>> +++ b/drivers/usb/dwc2/gadget.c
>>> @@ -1761,10 +1761,7 @@ static int dwc2_hsotg_process_req_feature(struct
>>> dwc2_hsotg *hsotg,
>>> case USB_RECIP_DEVICE:
>>> switch (wValue) {
>>> case USB_DEVICE_REMOTE_WAKEUP:
>>> - if (set)
>>> - hsotg->remote_wakeup_allowed = 1;
>>> - else
>>> - hsotg->remote_wakeup_allowed = 0;
>>> + hsotg->remote_wakeup_allowed = set;
>>> break;
>>>
>>> case USB_DEVICE_TEST_MODE:
>>>
>>
>> It's good catch, but 'set' declared as 'bool' while
>> 'remote_wakeup_allowed' is 'unsigned int'. Maybe update 'set' type to same.
>
> I know set is bool. But that should not matter, code is okay and
> compiler will do the right thing:
>
> pavel@amd:/tmp$ cat delme.c
> #include
>
> void main(void)
> {
>bool a = false;
> int b = a;
> }
> pavel@amd:/tmp$ gcc -std=c99 -Wall delme.c
> delme.c:3:6: warning: return type of ‘main’ is not ‘int’ [-Wmain]
> void main(void)
> ^
> delme.c: In function ‘main’:
> delme.c:6:7: warning: unused variable ‘b’
> [-Wunused-variable]
> int b = a;
>^
>
> Best regards,
> Pavel
>
To avoid any possible check patch issues update type of "set" from bool
to same as "remote_wakeup_allowed".
Please update your patch topic to "usb: dwc2: gadget: remove unnecessary
if" and resubmit.
Thanks,
Minas
[PATCH] dwc2: gadget: Fix in control write transfers
After data out stage gadget driver should not initate ZLP on control EP,
because it is up to function driver.
Signed-off-by: Minas Harutyunyan
---
drivers/usb/dwc2/gadget.c | 20 ++--
1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 98a4a79e7f6e..b7520fa3725b 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -1369,6 +1369,11 @@ static int dwc2_hsotg_ep_queue(struct usb_ep *ep, struct
usb_request *req,
return 0;
}
+ /* Change EP direction if status phase request is after data out */
+ if (!hs_ep->index && !req->length && !hs_ep->dir_in &&
+ (hs->ep0_state == DWC2_EP0_DATA_OUT))
+ hs_ep->dir_in = 1;
+
if (first) {
if (!hs_ep->isochronous) {
dwc2_hsotg_start_req(hs, hs_ep, hs_req, false);
@@ -2327,14 +2332,6 @@ static void dwc2_hsotg_handle_outdone(struct dwc2_hsotg
*hsotg, int epnum)
*/
}
- /* DDMA IN status phase will start from StsPhseRcvd interrupt */
- if (!using_desc_dma(hsotg) && epnum == 0 &&
- hsotg->ep0_state == DWC2_EP0_DATA_OUT) {
- /* Move to STATUS IN */
- dwc2_hsotg_ep0_zlp(hsotg, true);
- return;
- }
-
/*
* Slave mode OUT transfers do not go through XferComplete so
* adjust the ISOC parity here.
@@ -2997,14 +2994,9 @@ static void dwc2_hsotg_epint(struct dwc2_hsotg *hsotg,
unsigned int idx,
}
}
- if (ints & DXEPINT_STSPHSERCVD) {
+ if (ints & DXEPINT_STSPHSERCVD)
dev_dbg(hsotg->dev, "%s: StsPhseRcvd\n", __func__);
- /* Move to STATUS IN for DDMA */
- if (using_desc_dma(hsotg))
- dwc2_hsotg_ep0_zlp(hsotg, true);
- }
-
if (ints & DXEPINT_BACK2BACKSETUP)
dev_dbg(hsotg->dev, "%s: B2BSetup/INEPNakEff\n", __func__);
--
2.11.0
Re: dwc2_hc_chhltd_intr_dma - ChHltd set errors?
Hi, On 4/3/2017 9:23 AM, John Youn wrote: > On 03/31/2017 04:04 PM, John Stultz wrote: >> On Thu, Mar 2, 2017 at 12:00 PM, John Stultz wrote: >>> Hey John, >>> We've noticed that when using usb ethernet adapters on HiKey, we >>> occasionally see errors like: >>> >>> dwc2 f72c.usb: dwc2_hc_chhltd_intr_dma: Channel 0 - ChHltd set, >>> but reason is unknown >>> dwc2 f72c.usb: hcint 0x0002, intsts 0x06200029 >>> >>> dwc2 f72c.usb: dwc2_hc_chhltd_intr_dma: Channel 11 - ChHltd set, >>> but reason is unknown >>> dwc2 f72c.usb: hcint 0x0002, intsts 0x04200029 >>> >>> Sometimes followed up by a usb error in the driver, something like: >>> asix 1-1.2:1.0 eth0: asix_rx_fixup() Bad Header Length 0x36000807, offset 68 >>> >>> Curious if you've seen any reports like this? >> >> Hey John, >> Just wanted to check in again on this to see if you've seen anything >> like it? I've not had too much time to debug it, but my attempts so >> far haven't been productive. >> > > I don't think I've seen that. > > We'll try to take a look this week. > > Thanks, > John > > > The core version is less than 2.71a, am I right? Please send full debug log to do more investigation. Also send us regdump after connecting ethernet adapter. Thanks, Minas
Re: dwc2_hc_chhltd_intr_dma - ChHltd set errors?
Hi, On 4/4/2017 7:04 AM, John Stultz wrote: > On Mon, Apr 3, 2017 at 5:54 AM, Minas Harutyunyan > wrote: >> On 4/3/2017 9:23 AM, John Youn wrote: >>> On 03/31/2017 04:04 PM, John Stultz wrote: >>>> On Thu, Mar 2, 2017 at 12:00 PM, John Stultz >>>> wrote: >>>>> Hey John, >>>>> We've noticed that when using usb ethernet adapters on HiKey, we >>>>> occasionally see errors like: >>>>> >>>>> dwc2 f72c.usb: dwc2_hc_chhltd_intr_dma: Channel 0 - ChHltd set, >>>>> but reason is unknown >>>>> dwc2 f72c.usb: hcint 0x0002, intsts 0x06200029 >>>>> >>>>> dwc2 f72c.usb: dwc2_hc_chhltd_intr_dma: Channel 11 - ChHltd set, >>>>> but reason is unknown >>>>> dwc2 f72c.usb: hcint 0x0002, intsts 0x04200029 >>>>> >>>>> Sometimes followed up by a usb error in the driver, something like: >>>>> asix 1-1.2:1.0 eth0: asix_rx_fixup() Bad Header Length 0x36000807, offset >>>>> 68 >>>>> >>>>> Curious if you've seen any reports like this? >> >> The core version is less than 2.71a, am I right? > > So it looks like its reporting 0x4f54300a for hsotg->regs + GSNPSID > which looks like DWC2_CORE_REV_3_00a > >> Please send full debug log to do more investigation. > > Full dmesg, or is there special debugging you want me to enable? Full dmesg around issue. > >> Also send us regdump after connecting ethernet adapter. > > Sorry, can you clarify how to generate this? cat regdump. To locate dwc2 regdump file: cd /; find -name regdump > > thanks > -john > Thanks, Minas
Re: dwc2_hc_chhltd_intr_dma - ChHltd set errors?
Hi,
On 4/4/2017 11:03 PM, John Stultz wrote:
> On Tue, Apr 4, 2017 at 12:38 AM, Felipe Balbi
> wrote:
>>
>> Hi,
>>
>> Minas Harutyunyan writes:
>>>>>>>> We've noticed that when using usb ethernet adapters on HiKey, we
>>>>>>>> occasionally see errors like:
>>>>>>>>
>>>>>>>> dwc2 f72c.usb: dwc2_hc_chhltd_intr_dma: Channel 0 - ChHltd set,
>>>>>>>> but reason is unknown
>>>>>>>> dwc2 f72c.usb: hcint 0x0002, intsts 0x06200029
>>>>>>>>
>>>>>>>> dwc2 f72c.usb: dwc2_hc_chhltd_intr_dma: Channel 11 - ChHltd set,
>>>>>>>> but reason is unknown
>>>>>>>> dwc2 f72c.usb: hcint 0x0002, intsts 0x04200029
>>>>>>>>
>>>>>>>> Sometimes followed up by a usb error in the driver, something like:
>>>>>>>> asix 1-1.2:1.0 eth0: asix_rx_fixup() Bad Header Length 0x36000807,
>>>>>>>> offset 68
>>>>>>>>
>>>>>>>> Curious if you've seen any reports like this?
>>>>>
>>>>> The core version is less than 2.71a, am I right?
>>>>
>>>> So it looks like its reporting 0x4f54300a for hsotg->regs + GSNPSID
>>>> which looks like DWC2_CORE_REV_3_00a
>>>>
>>>>> Please send full debug log to do more investigation.
>>>>
>>>> Full dmesg, or is there special debugging you want me to enable?
>>>
>>> Full dmesg around issue.
>>>>
>>>>> Also send us regdump after connecting ethernet adapter.
>>>>
>>>> Sorry, can you clarify how to generate this?
>>>
>>> cat regdump. To locate dwc2 regdump file: cd /; find -name regdump
>>
>> this won't work if his distro doesn't mount debugfs. Please give
>> complete instructions ;-)
>>
>> # mkdir -p /d
>> # mount -t debugfs none /d
>> # cd /d
>> # find . -name regdump
>>
>> The directory name is the same name as the dwc2 device name, AFAICT. So,
>> check your DTS for the name of the device.
>
> Thanks for the extra details! I didn't have DEBUG_FS built in, so this
> helped clue me in.
>
> Attached are dmesg including the issue and the regdump.
>
> I did notice when cating the regdump file, I saw:
> dwc2 f72c.usb: Mode Mismatch Interrupt: currently in Host mode
> twice. (You'll see it 4 times in the dmesg around 1077 as I cat'ed
> regdump again to verify it wasn't just chance).
>
> Let me know if there is anything else you need!
>
> thanks
> -john
>
Could you please apply attached patch and try again.
Thanks,
Minas
From 412bf7e3dad8a20af937f3682a08116b654deb02 Mon Sep 17 00:00:00 2001
Message-Id:
<412bf7e3dad8a20af937f3682a08116b654deb02.1491410224.git.hmi...@synopsys.com>
From: Minas Harutyunyan
Date: Mon, 11 Jul 2016 03:31:17 -0700
Subject: [PATCH] usb: dwc2: hcd: Fix host channel halt flow
According databook in Buffer and External DMA mode
non-split periodic channels can't be halted.
Change-Id: I7c7c4e16309dda7a3b5af34020d46366c357ed49
Signed-off-by: Minas Harutyunyan
---
drivers/usb/dwc2/hcd.c | 19 +++
1 file changed, 19 insertions(+)
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index f4ef159b538e..4e6ee398efb7 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -979,6 +979,25 @@ void dwc2_hc_halt(struct dwc2_hsotg *hsotg, struct
dwc2_host_chan *chan,
if (dbg_hc(chan))
dev_vdbg(hsotg->dev, "%s()\n", __func__);
+
+ /*
+* In buffer DMA or external DMA mode channel can't be halted
+* for non-split periodic channels. At the end of the next
+* uframe/frame (in the worst case), the core generates a channel
+* halted and disables the channel automatically.
+*/
+ if ((hsotg->core_params->dma_enable > 0 &&
+hsotg->core_params->dma_desc_enable <= 0) ||
+ (hsotg->hw_params.arch == GHWCFG2_EXT_DMA_ARCH)) {
+ if (!chan->do_split &&
+ (chan->ep_type == USB_ENDPOINT_XFER_ISOC ||
+chan->ep_type == USB_ENDPOINT_XFER_INT)) {
+ dev_err(hsotg->dev, "%s() Channel can't be halted\n",
+ __func__);
+ return;
+ }
+ }
+
if (halt_status == DWC2_HC_XFER_NO_HALT_STATUS)
dev_err(hsotg->dev, "!!! halt_status = %d !!!\n", halt_status);
--
2.11.0
Re: dwc2_hc_chhltd_intr_dma - ChHltd set errors?
On 4/6/2017 1:03 AM, John Stultz wrote: > > > On Wed, Apr 5, 2017 at 5:58 AM, Minas Harutyunyan > mailto:minas.harutyun...@synopsys.com>> > wrote: >> On 4/4/2017 11:03 PM, John Stultz wrote: >>> >>> I did notice when cating the regdump file, I saw: >>> dwc2 f72c.usb: Mode Mismatch Interrupt: currently in Host mode >>> twice. (You'll see it 4 times in the dmesg around 1077 as I cat'ed >>> regdump again to verify it wasn't just chance). >>> >>> Let me know if there is anything else you need! >>> >> >> Could you please apply attached patch and try again. > > Thanks for sending this out! > > So it didn't build against mainline, but I tweaked it a bit: > - if ((hsotg->core_params->dma_enable > 0 && > -hsotg->core_params->dma_desc_enable <= 0) || > + if ((hsotg->params.host_dma > 0 && > +hsotg->params.dma_desc_enable <= 0) || > > > But I'm still seeing similar behavior: > [ 91.517417] dwc2 f72c.usb: dwc2_hc_chhltd_intr_dma: Channel 5 - > ChHltd set, but reason is unknown > [ 91.526693] dwc2 f72c.usb: hcint 0x0002, intsts 0x0429 > [ 91.533613] asix 1-1.2:1.0 eth0: asix_rx_fixup() Bad Header Length > 0x8003a0cc, offset 1302 > [ 91.534102] asix 1-1.2:1.0 eth0: asix_rx_fixup() Bad Header Length > 0x73ff5a7d, offset 4 > [ 169.116866] dwc2 f72c.usb: dwc2_hc_chhltd_intr_dma: Channel 1 - > ChHltd set, but reason is unknown > [ 169.126146] dwc2 f72c.usb: hcint 0x0002, intsts 0x06200029 > [ 170.699334] asix 1-1.2:1.0 eth0: asix_rx_fixup() Bad Header Length > 0x36000807, offset 68 > > And I'm not seeing the "Channel can't be halted" error from the new logic. > > Full dmesg and regdump attached. > > Let me know if there is something else I should try. > > thanks > -john Ok. To enable full dwc2 debug messages, please set USB_DWC2_DEBUG and USB_DWC2_VERBOSE in Kernel configuration file. Also provide the topology of connected devices(class, speed) to dwc2 root hub. Thanks, Minas
Re: dwc2_hc_chhltd_intr_dma - ChHltd set errors?
Hi, On 4/7/2017 12:18 PM, Felipe Balbi wrote: > > Hi, > > John Stultz writes: > mailto:minas.harutyun...@synopsys.com>> > wrote: >> On 4/4/2017 11:03 PM, John Stultz wrote: >>> >>> I did notice when cating the regdump file, I saw: >>> dwc2 f72c.usb: Mode Mismatch Interrupt: currently in Host mode >>> twice. (You'll see it 4 times in the dmesg around 1077 as I cat'ed >>> regdump again to verify it wasn't just chance). >>> >>> Let me know if there is anything else you need! >>> >> >> Could you please apply attached patch and try again. > > Thanks for sending this out! > > So it didn't build against mainline, but I tweaked it a bit: > - if ((hsotg->core_params->dma_enable > 0 && > -hsotg->core_params->dma_desc_enable <= 0) || > + if ((hsotg->params.host_dma > 0 && > +hsotg->params.dma_desc_enable <= 0) || > > > But I'm still seeing similar behavior: > [ 91.517417] dwc2 f72c.usb: dwc2_hc_chhltd_intr_dma: Channel 5 - > ChHltd set, but reason is unknown > [ 91.526693] dwc2 f72c.usb: hcint 0x0002, intsts 0x0429 > [ 91.533613] asix 1-1.2:1.0 eth0: asix_rx_fixup() Bad Header Length > 0x8003a0cc, offset 1302 > [ 91.534102] asix 1-1.2:1.0 eth0: asix_rx_fixup() Bad Header Length > 0x73ff5a7d, offset 4 > [ 169.116866] dwc2 f72c.usb: dwc2_hc_chhltd_intr_dma: Channel 1 - > ChHltd set, but reason is unknown > [ 169.126146] dwc2 f72c.usb: hcint 0x0002, intsts 0x06200029 > [ 170.699334] asix 1-1.2:1.0 eth0: asix_rx_fixup() Bad Header Length > 0x36000807, offset 68 > > And I'm not seeing the "Channel can't be halted" error from the new logic. > > Full dmesg and regdump attached. > > Let me know if there is something else I should try. > > thanks > -john Ok. To enable full dwc2 debug messages, please set USB_DWC2_DEBUG and USB_DWC2_VERBOSE in Kernel configuration file. >>> >>> Huh. So interestingly adding USB_DWC2_VERBOSE changes some behavior, >>> which suggests some racy logic somewhere. >>> >>> In the dmesg logs provided earlier, I would unplug the micro-B cable, >>> and the hub would be enabled, detect the mouse, reset to slow-speed, >>> and then both mouse and eth would be detected (see below for topology >>> and quirkiness here). >>> >>> [ 609.726186] dwc2 f72c.usb: Set speed to high-speed >>> [ 609.731529] usb 1-1: new high-speed USB device number 26 using dwc2 >>> [ 609.921447] dwc2 f72c.usb: Set speed to high-speed >>> [ 609.949694] hub 1-1:1.0: USB hub found >>> [ 609.954067] hub 1-1:1.0: 3 ports detected >>> [ 610.246008] dwc2 f72c.usb: Set speed to full-speed >>> [ 610.251388] usb 1-1.1: new low-speed USB device number 27 using dwc2 >>> [ 610.486571] usb 1-1: USB disconnect, device number 26 >>> [ 615.722267] dwc2 f72c.usb: Set speed to full-speed >>> [ 615.727580] usb 1-1: new full-speed USB device number 31 using dwc2 >>> [ 615.914674] dwc2 f72c.usb: Set speed to full-speed >>> [ 615.939233] usb 1-1: not running at top speed; connect to a high speed >>> hub >>> [ 615.949500] hub 1-1:1.0: USB hub found >>> [ 615.953815] hub 1-1:1.0: 3 ports detected >>> [ 616.246178] dwc2 f72c.usb: Set speed to full-speed >>> [ 616.251539] usb 1-1.1: new low-speed USB device number 32 using dwc2 >>> [ 616.342042] dwc2 f72c.usb: Set speed to full-speed >>> [ 616.393609] input: Logitech USB Optical Mouse as >>> /devices/platform/soc/f72c.usb/usb1/1-1/1-1.1/1-1.1:1.0/0003:046D:C058.0004/input/in4 >>> [ 616.408426] hid-generic 0003:046D:C058.0004: input,hidraw0: USB HID >>> v1.11 Mouse [Logitech USB Optical Mouse] on usb-f72c.usb-1.1/inpu0 >>> [ 616.506057] dwc2 f72c.usb: Set speed to full-speed >>> [ 616.511449] usb 1-1.2: new full-speed USB device number 33 using dwc2 >>> [ 616.598037] dwc2 f72c.usb: Set speed to full-speed >>> [ 616.626890] usb 1-1.2: not running at top speed; connect to a high speed >>> hub >>> [ 617.098791] asix 1-1.2:1.0 eth0: register 'asix' at >>> usb-f72c.usb-1.2, ASIX AX88772B USB 2.0 Ethernet, >>> 00:50:b6:18:82:98 >>> [ 617.112147] IPv6: ADDRCONF(NETDEV_UP): eth0: link is not ready >>> [ 618.660076] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready >>> [ 618.682072] asix 1-1.2:1.0 eth0: link up, 100Mbps, full-duplex, lpa >>> 0xCDE1 >>> >>> >>> But with DWC2_VERBOSE enabled, I'm usually seeing the following (this >>> is what hit the console, not from the full dmesg output): >>> >>> [ 83.658545] dwc2 f72c.usb: Set speed to high-speed >>> [ 83.663897] usb 1-1: new high-speed USB device number 2 using dwc2 >>> [ 83.854516] dwc2 f72c.usb: Set speed to high-speed >>> [ 83.901097] hub 1-1:1.0: USB hub found >>> [ 83.910054] hub 1-1:1.0: 3 ports detected >>> [ 84.241753] dwc2 f72c.usb: Set speed to full-speed >>> [ 84.247008] usb 1-1.1: ne
Re: [PATCH] dwc2: gadget: Fix in control write transfers
Hi, On 3/30/2017 2:42 PM, Felipe Balbi wrote: > > Hi, > > Minas Harutyunyan writes: >> After data out stage gadget driver should not initate ZLP on control EP, >> because it is up to function driver. > > not true always, depends on return value from ->setup(). Which problem > did you have? Which gadget driver? How did you reproduce? Which other > tests did you run on this patch? > This required for delayed status support. Tested with Synopsys test gadget. As host used USB tracer traffic generator (different control transfers scenarios). Also performed smoke tests with mass storage function to detect any side effects. Thanks, Minas
[PATCH] usb: dwc2: gadget: Use ep->index in conext of FIFO registers
It's reverting patch for commit "usb: dwc2: gadget:
use ep->fifo_index in context of FIFO registers".
DTXFSTSn indexation based on EP number not TxFIFO number.
Signed-off-by: Minas Harutyunyan
---
drivers/usb/dwc2/gadget.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 98a4a79e7f6e..34c714de75be 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -526,8 +526,7 @@ static int dwc2_hsotg_write_fifo(struct dwc2_hsotg *hsotg,
return -ENOSPC;
}
} else if (hsotg->dedicated_fifos && hs_ep->index != 0) {
- can_write = dwc2_readl(hsotg->regs +
- DTXFSTS(hs_ep->fifo_index));
+ can_write = dwc2_readl(hsotg->regs + DTXFSTS(hs_ep->index));
can_write &= 0x;
can_write *= 4;
@@ -3153,7 +3152,7 @@ static void kill_all_requests(struct dwc2_hsotg *hsotg,
if (!hsotg->dedicated_fifos)
return;
- size = (dwc2_readl(hsotg->regs + DTXFSTS(ep->fifo_index)) & 0x) * 4;
+ size = (dwc2_readl(hsotg->regs + DTXFSTS(ep->index)) & 0x) * 4;
if (size < ep->fifo_size)
dwc2_hsotg_txfifo_flush(hsotg, ep->fifo_index);
}
--
2.11.0
[PATCH] usb: dwc2: Add safety check in setting of descriptor chain pointers
In some MSC CV tests device sending ZLP IN on non EP0 which
reassigning EP0 OUT descriptor pointer to that EP.
Dedicated for EP0 OUT descriptor multiple time re-used by
other EP while that descriptor already in use by EP0 OUT
for SETUP transaction. As result when SETUP packet received
BNA interrupt asserting.
In dwc2_hsotg_program_zlp() function dwc2_gadget_set_ep0_desc_chain()
must be called only for EP0.
Signed-off-by: Minas Harutyunyan
---
drivers/usb/dwc2/gadget.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 98a4a79e7f6e..3dc08f6847df 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -1943,7 +1943,9 @@ static void dwc2_hsotg_program_zlp(struct dwc2_hsotg
*hsotg,
/* Not specific buffer needed for ep0 ZLP */
dma_addr_t dma = hs_ep->desc_list_dma;
- dwc2_gadget_set_ep0_desc_chain(hsotg, hs_ep);
+ if (!index)
+ dwc2_gadget_set_ep0_desc_chain(hsotg, hs_ep);
+
dwc2_gadget_config_nonisoc_xfer_ddma(hs_ep, dma, 0);
} else {
dwc2_writel(DXEPTSIZ_MC(1) | DXEPTSIZ_PKTCNT(1) |
--
2.11.0
[PATCH] usb: dwc2: gadget: On disconnect reset device address to zero
USB CV driver stack doesn't perform USB RESET after device disconnect/
connect, so need to reset to zero DEVADDR field in DCFG to pass
enumeration again.
Signed-off-by: Minas Harutyunyan
---
drivers/usb/dwc2/gadget.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 98a4a79e7f6e..deb3d901b99d 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3174,6 +3174,9 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg)
return;
hsotg->connected = 0;
+ /* On disconnect reset device address to zero */
+ __bic32(hsotg->regs + DCFG, DCFG_DEVADDR_MASK);
+
hsotg->test_mode = 0;
for (ep = 0; ep < hsotg->num_of_eps; ep++) {
--
2.11.0
Re: [PATCH] usb: dwc2: gadget: On disconnect reset device address to zero
Hi Filipe, On 7/10/2017 6:06 PM, Felipe Balbi wrote: > > Hi, > > Minas Harutyunyan writes: >> USB CV driver stack doesn't perform USB RESET after device disconnect/ >> connect, so need to reset to zero DEVADDR field in DCFG to pass >> enumeration again. >> >> Signed-off-by: Minas Harutyunyan >> --- >> drivers/usb/dwc2/gadget.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c >> index 98a4a79e7f6e..deb3d901b99d 100644 >> --- a/drivers/usb/dwc2/gadget.c >> +++ b/drivers/usb/dwc2/gadget.c >> @@ -3174,6 +3174,9 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg) >> return; >> >> hsotg->connected = 0; >> +/* On disconnect reset device address to zero */ >> +__bic32(hsotg->regs + DCFG, DCFG_DEVADDR_MASK); >> + > > Which of the tests are you talking about? Which particular USB CV are > you running? > I used USB 3 Gen X CV (downloaded from usb.org 1-2 week ago). Tests are: 1. "Device Summary" - after 1st pass, disconnect then connect again test failed. Actually it show as "test passed" but not able to enumerate device again. 2. MSC "USB Mass Storage Power Up Test". After disconnect, by suite request, and then connect test failed (pass, if reloading driver). > I don't remember ever seeing this with dwc3. How should I go about > triggering this problem? If this was really the case, then we would have > this on *all* UDCs. > dwc2 driver resetting DEVADDR in DCFG register only in function dwc2_hsotg_core_init_disconnected() by soft reset. This function not called on disconnect/connect with CV SW stack (function call not seen in dmesg). This issue not seen with any other EHCI/XHCI hosts either on Linux/Windows because these hosts issuing USB RESET, as result called dwc2_hsotg_core_init_disconnected(). In dwc3 per my understanding same stuff done in function dwc3_gadget_reset_interrupt(), am I right? > I just did a fresh install of USB 3 Gen X CV (that I just download from > usb.org). Ran Chapter 9 tests against a HS dwc3 board I have around and > I can't see the problem you're talking about. > Yes, this issue not seen with dwc3. > Here are all my non-endpoint interrupt events in order. Test passes > fine. If disconnect, reconnect and run the tests again, then Reset will > be driven on the bus which will cause address to be reset. > > Can you share more details about the problem you're facing? > > events > === > > irq/34-dwc3-1609 [001] d..1 13907.710095: dwc3_event: event (0001): > Disconnect: [U0] > irq/34-dwc3-1609 [001] d..1 13958.860840: dwc3_event: event (0001): > Disconnect: [U0] > irq/34-dwc3-1609 [001] d..1 14161.547651: dwc3_event: event (0101): > Reset [U0] > irq/34-dwc3-1609 [001] d..1 14161.602254: dwc3_event: event (0201): > Connection Done [U0] > irq/34-dwc3-1609 [001] d..1 14163.314174: dwc3_event: event (0101): > Reset [U0] > irq/34-dwc3-1609 [001] d..1 14163.368819: dwc3_event: event (0201): > Connection Done [U0] > irq/34-dwc3-1609 [001] d..1 14168.706773: dwc3_event: event (0101): > Reset [U0] > irq/34-dwc3-1609 [001] d..1 14168.758551: dwc3_event: event (0201): > Connection Done [U0] > irq/34-dwc3-1609 [001] d..1 14190.027840: dwc3_event: event (0401): > WakeUp [U0] > irq/34-dwc3-1609 [001] d..1 14203.428377: dwc3_event: event (0101): > Reset [U0] > irq/34-dwc3-1609 [001] d..1 14203.479869: dwc3_event: event (0201): > Connection Done [U0] > irq/34-dwc3-1609 [001] d..1 14203.739212: dwc3_event: event (0101): > Reset [U0] > irq/34-dwc3-1609 [001] d..1 14203.790550: dwc3_event: event (0201): > Connection Done [U0] > irq/34-dwc3-1609 [001] d..1 14204.025838: dwc3_event: event (0101): > Reset [U0] > irq/34-dwc3-1609 [001] d..1 14204.077335: dwc3_event: event (0201): > Connection Done [U0] > irq/34-dwc3-1609 [001] d..1 14204.329353: dwc3_event: event (0101): > Reset [U0] > irq/34-dwc3-1609 [001] d..1 14204.380840: dwc3_event: event (0201): > Connection Done [U0] > irq/34-dwc3-1609 [001] d..1 14204.629167: dwc3_event: event (0101): > Reset [U0] > irq/34-dwc3-1609 [001] d..1 14204.680503: dwc3_event: event (0201): > Connection Done [U0] > irq/34-dwc3-1609 [001] d..1 14204.941740: dwc3_event: event (0101): > Reset [U0] > irq/34-dwc3-1609 [001] d..1 14204.993141: dwc3_event: event (0201): > Connection Done [U0] > irq/34-dwc3-1609 [001] d..1 14205.233103: dwc3_event: event (00
Re: [PATCH] usb: dwc2: gadget: On disconnect reset device address to zero
Hi Filipe, On 7/10/2017 6:14 PM, Felipe Balbi wrote: > > Hi again, > > Felipe Balbi writes: >> Hi, >> >> Minas Harutyunyan writes: >>> USB CV driver stack doesn't perform USB RESET after device disconnect/ >>> connect, so need to reset to zero DEVADDR field in DCFG to pass >>> enumeration again. >>> >>> Signed-off-by: Minas Harutyunyan >>> --- >>> drivers/usb/dwc2/gadget.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c >>> index 98a4a79e7f6e..deb3d901b99d 100644 >>> --- a/drivers/usb/dwc2/gadget.c >>> +++ b/drivers/usb/dwc2/gadget.c >>> @@ -3174,6 +3174,9 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg) >>> return; >>> >>> hsotg->connected = 0; >>> + /* On disconnect reset device address to zero */ >>> + __bic32(hsotg->regs + DCFG, DCFG_DEVADDR_MASK); >>> + >> >> Which of the tests are you talking about? Which particular USB CV are >> you running? >> >> I don't remember ever seeing this with dwc3. How should I go about >> triggering this problem? If this was really the case, then we would have >> this on *all* UDCs. >> >> I just did a fresh install of USB 3 Gen X CV (that I just download from >> usb.org). Ran Chapter 9 tests against a HS dwc3 board I have around and >> I can't see the problem you're talking about. >> >> Here are all my non-endpoint interrupt events in order. Test passes >> fine. If disconnect, reconnect and run the tests again, then Reset will >> be driven on the bus which will cause address to be reset. >> >> Can you share more details about the problem you're facing? > > I've been looking at dwc2 for a while and I think this is a bug in > dwc2_hsotg_irq() on the branch for GINTSTS_USBRST. I don't have docs for > dwc2. > USB RESET not issuing by CV host stack after connect. Below dmesg: Disconnect [23984.039199] dwc2 dwc2.1.auto: dwc2_hsotg_irq: 04008428 0400 (d0bc3cc4) retry 8 [23984.039204] dwc2 dwc2.1.auto: GINTSTS_ErlySusp [23984.042235] dwc2 dwc2.1.auto: gintsts=04008828 gintmsk=d0bc3cc4 [23984.042240] dwc2 dwc2.1.auto: USB SUSPEND [23984.042247] dwc2 dwc2.1.auto: DSTS=0x5f4c01 [23984.042250] dwc2 dwc2.1.auto: DSTS.Suspend Status=1 HWCFG4.Power Optimize=0 [23984.042260] dwc2 dwc2.1.auto: dwc2_hsotg_irq: 04008028 (d0bc3cc4) retry 8 [23984.272308] dwc2 dwc2.1.auto: gintsts=0480902c gintmsk=d0bc3cc4 [23984.272318] dwc2 dwc2.1.auto: ++OTG Interrupt gotgint=4 [b_peripheral] [23984.272321] dwc2 dwc2.1.auto: ++OTG Interrupt: Session End Detected++ (b_peripheral) [23984.272331] dwc2 dwc2.1.auto: complete: ep 880401a31b28 ep0, req 8803e495ef00, -108 => a00541da [23984.272336] dwc2 dwc2.1.auto: dwc2_hsotg_complete_setup: failed -108 [23984.272346] dwc2 dwc2.1.auto: complete: ep 880401a30328 ep1out, req 88040b6d7c80, -108 => a008865c [23984.272435] dwc2 dwc2.1.auto: dwc2_hsotg_irq: 04809028 00801000 (d0bc3cc4) retry 8 [23984.272437] dwc2 dwc2.1.auto: dwc2_hsotg_irq: USBRstDet [23984.272442] dwc2 dwc2.1.auto: dwc2_hsotg_irq: USBRst [23984.272446] dwc2 dwc2.1.auto: GNPTXSTS=00040300 [23984.272473] dwc2 dwc2.1.auto: dwc2_hsotg_ep_disable(ep 880401a30528) [23984.272478] dwc2 dwc2.1.auto: dwc2_hsotg_ep_disable: DxEPCTL=0x084a0200 [23984.272485] dwc2 dwc2.1.auto: dwc2_hsotg_ep_disable(ep 880401a30328) [23984.272490] dwc2 dwc2.1.auto: dwc2_hsotg_ep_stop_xfr: stopping transfer on ep1out [23984.272513] dwc2 dwc2.1.auto: dwc2_hsotg_ep_disable: DxEPCTL=0x08080200 [23984.272540] dwc2 dwc2.1.auto: dwc2_hsotg_irq: 04008028 (d0bc3cc4) retry 8 Connect [24010.664017] dwc2 dwc2.1.auto: gintsts=44008028 gintmsk=d0bc3cc4 [24010.664023] dwc2 dwc2.1.auto: Session request interrupt - lx_state=0 [24010.664035] dwc2 dwc2.1.auto: dwc2_hsotg_irq: 04008028 (d0bc3cc4) retry 8 Run test First try to enumerate [24021.649528] dwc2 dwc2.1.auto: dwc2_hsotg_irq: 0400a028 2000 (d0bc3cc4) retry 8 [24021.649536] dwc2 dwc2.1.auto: EnumDone (DSTS=0x0043e902) [24021.649539] dwc2 dwc2.1.auto: new device is full-speed [24021.649618] dwc2 dwc2.1.auto: dwc2_hsotg_enqueue_setup: queueing setup request [24021.649623] dwc2 dwc2.1.auto: ep0: req 8803e495ef00: 8@88040ad1d228, noi=0, zero=0, snok=0 [24021.649631] dwc2 dwc2.1.auto: dwc2_hsotg_start_req: DxEPCTL=0x80028000, ep 0, dir out [24021.649636] dwc2 dwc2.1.auto: ureq->length:8 ureq->actual:0 [24021.649640] dwc2 dwc2.1.auto: dwc2_hsotg_start_req: 1@8/8, 0x00080008 => 0x0b10 [24021.649643] dwc2 dwc2.1.auto: dwc2_hsotg_start_req: 0xd9858800 => 0x0b14 [24021.649645] dwc2 dwc2.1.a
[PATCH] usb: dwc2: gadget: On USB RESET reset device address to zero
Reseted DEVADDR field in DCFG to zero on USB RESET. Device address in DCFG register does not reset to zero, which required to pass enumeration, after disconnect and reconnect. Signed-off-by: Minas Harutyunyan --- drivers/usb/dwc2/gadget.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index 98a4a79e7f6e..c5c0a26a4d66 100644 --- a/drivers/usb/dwc2/gadget.c +++ b/drivers/usb/dwc2/gadget.c @@ -3561,6 +3561,9 @@ static irqreturn_t dwc2_hsotg_irq(int irq, void *pw) /* Report disconnection if it is not already done. */ dwc2_hsotg_disconnect(hsotg); + /* Reset device address to zero */ + __bic32(hsotg->regs + DCFG, DCFG_DEVADDR_MASK); + if (usb_status & GOTGCTL_BSESVLD && connected) dwc2_hsotg_core_init_disconnected(hsotg, true); } -- 2.11.0
Re: [PATCH] usb: dwc2: gadget: On disconnect reset device address to zero
Hi Felipe, On 7/11/2017 11:34 AM, Felipe Balbi wrote: > > Hi, > > Minas Harutyunyan writes: >>> Minas Harutyunyan writes: >>>> USB CV driver stack doesn't perform USB RESET after device disconnect/ >>>> connect, so need to reset to zero DEVADDR field in DCFG to pass >>>> enumeration again. >>>> >>>> Signed-off-by: Minas Harutyunyan >>>> --- >>>> drivers/usb/dwc2/gadget.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c >>>> index 98a4a79e7f6e..deb3d901b99d 100644 >>>> --- a/drivers/usb/dwc2/gadget.c >>>> +++ b/drivers/usb/dwc2/gadget.c >>>> @@ -3174,6 +3174,9 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg) >>>>return; >>>> >>>>hsotg->connected = 0; >>>> + /* On disconnect reset device address to zero */ >>>> + __bic32(hsotg->regs + DCFG, DCFG_DEVADDR_MASK); >>>> + >>> >>> Which of the tests are you talking about? Which particular USB CV are >>> you running? >>> >> I used USB 3 Gen X CV (downloaded from usb.org 1-2 week ago). Tests are: >> 1. "Device Summary" - after 1st pass, disconnect then connect again test >> failed. Actually it show as "test passed" but not able to enumerate >> device again. >> 2. MSC "USB Mass Storage Power Up Test". After disconnect, by suite >> request, and then connect test failed (pass, if reloading driver). > > I'll try these tests tomorrow next time I'm in the office. Woke up sick, > working from home today :-s > >>> I don't remember ever seeing this with dwc3. How should I go about >>> triggering this problem? If this was really the case, then we would have >>> this on *all* UDCs. >>> >> dwc2 driver resetting DEVADDR in DCFG register only in function >> dwc2_hsotg_core_init_disconnected() by soft reset. This function not >> called on disconnect/connect with CV SW stack (function call not seen in >> dmesg). > > right, this is a bug in dwc2. You should clear DEVADDR from your Reset > handler, not disconnect. > >> This issue not seen with any other EHCI/XHCI hosts either on >> Linux/Windows because these hosts issuing USB RESET, as result called >> dwc2_hsotg_core_init_disconnected(). >> >> In dwc3 per my understanding same stuff done in function >> dwc3_gadget_reset_interrupt(), am I right? > > right, as it should. If dwc3_gadget_reset_interrupt() runs, this means > the host *is* issuing USB reset signalling. > >>> I just did a fresh install of USB 3 Gen X CV (that I just download from >>> usb.org). Ran Chapter 9 tests against a HS dwc3 board I have around and >>> I can't see the problem you're talking about. >>> >> Yes, this issue not seen with dwc3. > > because we clear DEVADDR on Reset. > Thank you for review and feedback. New patch "[PATCH] usb: dwc2: gadget: On USB RESET reset device address to zero" sent for review. Thanks, Minas
Re: [RESEND x2][PATCH 0/3] dwc2 fixes for edge cases on hikey
On 9/30/2017 9:13 PM, John Youn wrote: > On 09/20/2017 12:57 PM, John Stultz wrote: >> So here are a few dwc2 fixes that I've been using with HiKey. >> I'm not totally sure these are all ideal, but they avoid edge case >> issues that we have been running into with switching between >> gadget mode and host mode. >> >> I'd guess the first two are potentially -stable material, and >> the last might be worth sending to -stable too, as its a relatively >> simple fix, but to my understanding the UDC state tracking has >> always been broken so its not really a regression. But still. >> >> I'd love to get some feedback on the patches and consideration >> to be merged upstream. >> >> thanks >> -john >> >> Cc: Wei Xu >> Cc: Guodong Xu >> Cc: Amit Pundir >> Cc: YongQin Liu >> Cc: John Youn >> Cc: Minas Harutyunyan >> Cc: Douglas Anderson >> Cc: Chen Yu >> Cc: Felipe Balbi >> Cc: Greg Kroah-Hartman >> Cc: linux-...@vger.kernel.org >> >> John Stultz (3): >> usb: dwc2: Improve gadget state disconnection handling >> usb: dwc2: Error out of dwc2_hsotg_ep_disable() if we're in host mode >> usb: dwc2: Fix UDC state tracking >> >> drivers/usb/dwc2/gadget.c | 7 +++ >> drivers/usb/dwc2/hcd.c| 8 ++-- >> 2 files changed, 13 insertions(+), 2 deletions(-) >> > > Hi John, > > I think we have something that fixes these issues. > > Minas, > > Could you take a look at this? I was not able to find the patches we > talked about. If possible, please post them so that John can try them > out. > > Thanks, > John > Hi John Stultz, Could you please apply patch from Vardan Mikayelyan "usb: dwc2: Fix dwc2_hsotg_core_init_disconnected()" submitted at 02/25/2017 (https://marc.info/?l=linux-usb&m=148801589931039&w=2) instead of your "usb: dwc2: Improve gadget state disconnection handling" and test again failing scenario. Other 2 patches from series "[PATCH 0/3] dwc2 fixes for edge cases on hikey" are Ok. Thanks, Minas
Re: [RESEND x2][PATCH 2/3] usb: dwc2: Error out of dwc2_hsotg_ep_disable() if we're in host mode
On 9/20/2017 11:57 PM, John Stultz wrote:
> We've found that while in host mode, using Android, if one runs
> the command:
> stop adbd
>
> The existing usb devices being utilized in host mode are disconnected.
> This is most visible with usb networking devices.
>
> This seems to be due to adbd closing the file:
> /dev/usb-ffs/adb/ep0
> Which calls ffs_ep0_release() and the following backtrace:
>
> [] dwc2_hsotg_ep_disable+0x148/0x150
> [] dwc2_hsotg_udc_stop+0x60/0x110
> [] usb_gadget_remove_driver+0x58/0x78
> [] usb_gadget_unregister_driver+0x74/0xe8
> [] unregister_gadget+0x28/0x58
> [] unregister_gadget_item+0x2c/0x40
> [] ffs_data_clear+0xe8/0xf8
> [] ffs_data_reset+0x20/0x58
> [] ffs_data_closed+0x98/0xe8
> [] ffs_ep0_release+0x20/0x30
>
> Then when dwc2_hsotg_ep_disable() is called, we call
> kill_all_requests() which causes a bunch of the following
> messages:
>
> dwc2 f72c.usb: Mode Mismatch Interrupt: currently in Host mode
> dwc2 f72c.usb: Mode Mismatch Interrupt: currently in Host mode
> dwc2 f72c.usb: Mode Mismatch Interrupt: currently in Host mode
> dwc2 f72c.usb: Mode Mismatch Interrupt: currently in Host mode
> dwc2 f72c.usb: Mode Mismatch Interrupt: currently in Host mode
> dwc2 f72c.usb: Mode Mismatch Interrupt: currently in Host mode
> dwc2 f72c.usb: Mode Mismatch Interrupt: currently in Host mode
> dwc2 f72c.usb: Mode Mismatch Interrupt: currently in Host mode
> init: Service 'adbd' (pid 1915) killed by signal 9
> init: Sending signal 9 to service 'adbd' (pid 1915) process group...
> init: Successfully killed process cgroup uid 0 pid 1915 in 0ms
> init: processing action (init.svc.adbd=stopped) from
> (/init.usb.configfs.rc:15)
> dwc2 f72c.usb: dwc2_hc_chhltd_intr_dma: Channel 8 - ChHltd set, but
> reason is unknown
> dwc2 f72c.usb: hcint 0x0002, intsts 0x04200029
> dwc2 f72c.usb: dwc2_hc_chhltd_intr_dma: Channel 12 - ChHltd set, but
> reason is unknown
> dwc2 f72c.usb: hcint 0x0002, intsts 0x04200029
> dwc2 f72c.usb: dwc2_hc_chhltd_intr_dma: Channel 15 - ChHltd set, but
> reason is unknown
> dwc2 f72c.usb: hcint 0x0002, intsts 0x04200029
> dwc2 f72c.usb: dwc2_hc_chhltd_intr_dma: Channel 3 - ChHltd set, but
> reason is unknown
> dwc2 f72c.usb: hcint 0x0002, intsts 0x04200029
> dwc2 f72c.usb: dwc2_hc_chhltd_intr_dma: Channel 4 - ChHltd set, but
> reason is unknown
> dwc2 f72c.usb: hcint 0x0002, intsts 0x04200029
> dwc2 f72c.usb: dwc2_update_urb_state_abn(): trimming xfer length
>
> And the usb devices connected are basically hung at this point.
>
> It seems like if we're in host mode, we probably shouldn't run
> the dwc2_hostg_ep_disable logic, so this patch returns an error
> in that case.
>
> With this patch (along with the two previous patches mailed out
> earlier:
>
> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2017_8_3_1008&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=6z9Al9FrHR_ZqbbtSAsD16pvOL2S3XHxQnSzq8kusyI&m=YbED3GZFyun_ID3-6Off3kdvd9xTUepWt4lzd2__ZSs&s=65BnVw7lagEfnyuD2WHnFlGTUnjvPHWyFiwL_F1vwfE&e=
>
> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2017_8_3_1010&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=6z9Al9FrHR_ZqbbtSAsD16pvOL2S3XHxQnSzq8kusyI&m=YbED3GZFyun_ID3-6Off3kdvd9xTUepWt4lzd2__ZSs&s=MdrOhV0i6kRrV2mHK5zHIwE1eF21MsTkHIjvsV2k7uw&e=
> ), we avoid the mismatched interrupts and connected usb devices
> continue to function.
>
> I'm not sure if some other solution would be better here, but this seems
> to work, so I wanted to send it out for input on what the right approach
> should be.
>
> Cc: Wei Xu
> Cc: Guodong Xu
> Cc: Amit Pundir
> Cc: YongQin Liu
> Cc: John Youn
> Cc: Minas Harutyunyan
> Cc: Douglas Anderson
> Cc: Chen Yu
> Cc: Felipe Balbi
> Cc: Greg Kroah-Hartman
> Cc: linux-...@vger.kernel.org
> Reported-by: YongQin Liu
> Signed-off-by: John Stultz
> ---
> drivers/usb/dwc2/gadget.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 0d8e09c..7fd0e38 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -4004,6 +4004,11 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
> return -EINVAL;
> }
>
> + if (hsotg->op_state != OTG_STATE_B_PERIPHERAL) {
> + dev_err(hsotg->dev, "%s: called in host mode?\n", __func__);
> + return -EINVAL;
> + }
> +
> epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
>
> spin_lock_irqsave(&hsotg->lock, flags);
>
Tested by: Minas Harutyunyan
Re: [RESEND x2][PATCH 3/3] usb: dwc2: Fix UDC state tracking
On 9/20/2017 11:57 PM, John Stultz wrote: > It has been noticed that the dwc2 udc state reporting doesn't > seem to work (at least on HiKey boards). Where after the initial > setup, the sysfs /sys/class/udc/f72c.usb/state file would > report "configured" no matter the state of the OTG port. > > This patch adds a call so that we report to the UDC layer when > the gadget device is disconnected. > > Cc: Wei Xu > Cc: Guodong Xu > Cc: Amit Pundir > Cc: YongQin Liu > Cc: John Youn > Cc: Minas Harutyunyan > Cc: Douglas Anderson > Cc: Chen Yu > Cc: Felipe Balbi > Cc: Greg Kroah-Hartman > Cc: linux-...@vger.kernel.org > Reported-by: Amit Pundir > Signed-off-by: John Stultz > --- > drivers/usb/dwc2/gadget.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > index 7fd0e38..603c216 100644 > --- a/drivers/usb/dwc2/gadget.c > +++ b/drivers/usb/dwc2/gadget.c > @@ -3202,6 +3202,8 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg) > > call_gadget(hsotg, disconnect); > hsotg->lx_state = DWC2_L3; > + > + usb_gadget_set_state(&hsotg->gadget, USB_STATE_NOTATTACHED); > } > > /** > Tested by: Minas Harutyunyan
Re: [RESEND x2][PATCH 0/3] dwc2 fixes for edge cases on hikey
On 10/10/2017 1:50 AM, John Stultz wrote: > On Tue, Oct 3, 2017 at 2:58 AM, Minas Harutyunyan > wrote: >> >> Could you please apply patch from Vardan Mikayelyan "usb: dwc2: Fix >> dwc2_hsotg_core_init_disconnected()" submitted at 02/25/2017 >> (https://urldefense.proofpoint.com/v2/url?u=https-3A__marc.info_-3Fl-3Dlinux-2Dusb-26m-3D148801589931039-26w-3D2&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=6z9Al9FrHR_ZqbbtSAsD16pvOL2S3XHxQnSzq8kusyI&m=4y4_kSoJQIp-rJkvFNu8yXR67QxLLQrbFkjlyytMUCE&s=3Gmh7tVGk7ncQfBNUjkVdpRa1XX_jf7lWga7kR1O9bQ&e=) >> instead of your >> "usb: dwc2: Improve gadget state disconnection handling" and test again >> failing scenario. > > I may be misunderstanding htings, but I don't believe that patch > addresses the same issue I'm trying to fix (I've tested with it, and > it doesn't cause any trouble for me, but it also doesn't seem to > address the corner-cases I'm hitting). > > My "Improve gadget state disconnection handling" patch handles the > fact that when we switch from B/gadget mode to A/Host mode, we don't > always go through a gadget disconnect path. > > So instead of calling the dwc2_hsotg_disconnect() path initially when > switching to gadget mode (to avoid the state complaining that we set > it up twice), we should instead be calling dwc2_hsotg_disconnect() > when we are setting up the A/host mode. > > So for example, the follow-on fix to the UDC state won't properly work > without my "Improve gadget state disconnection handling" patch, and > "cat /sys/class/udc/f72c.usb/state" will always return configured > (assuming gadget mode was used once) regardless of the gadget state, > since we don't actually call dwc2_hsotg_disconnect when the otg plug > is removed. > >> Other 2 patches from series "[PATCH 0/3] dwc2 fixes for edge cases on >> hikey" are Ok. > > Thanks for the review/feedback! > > thanks > -john > Hi John Stultz, 1. Vardan's patch fixing issue when dwc2 switched from host to device mode. It's allow to make functional device after reconnecting without tracking UDC state. 2. I suppose that your patch "[RESEND x2][PATCH 1/3] usb: dwc2: Improve gadget state disconnection handling" not a good way to set correct UDC state. You added calling device mode functions dwc2_hsotg_disconnect() and dwc2_hsotg_core_init_disconnected() while core in Host mode and as result additional unwanted "mode mismatch" interrupts will be asserted. 3. Function dwc2_conn_id_status_change() called when connector ID status changed. This interrupt asserted only when A-plug connected or disconnected. Connecting/disconnecting B-plug doesn't assert this interrupt. Thanks, Minas
