Message-ID: <410670D7E743164D87FA6160E7907A560113A2E38F@am04wembxa.internal.synopsys.com>
Date:   Mon, 16 Oct 2017 08:36:40 +0000
From:   Minas Harutyunyan <Minas.Harutyunyan@...opsys.com>
To:     John Stultz <john.stultz@...aro.org>,
        Minas Harutyunyan <Minas.Harutyunyan@...opsys.com>
CC:     John Youn <John.Youn@...opsys.com>,
        lkml <linux-kernel@...r.kernel.org>,
        Wei Xu <xuwei5@...ilicon.com>,
        Guodong Xu <guodong.xu@...aro.org>,
        "Amit Pundir" <amit.pundir@...aro.org>,
        YongQin Liu <yongqin.liu@...aro.org>,
        Douglas Anderson <dianders@...omium.org>,
        Chen Yu <chenyu56@...wei.com>,
        Felipe Balbi <felipe.balbi@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>
Subject: 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
> <Minas.Harutyunyan@...opsys.com> 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

Powered by blists - more mailing lists