Re: [PATCH v1 1/2] usb: chipidea: use of extcon framework to work for non OTG case

From: Peter Chen <peter.chen@nxp.com>
To: Igor Opaniuk <igor.opaniuk@gmail.com>
Cc: "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	Marcel Ziswiler <marcel.ziswiler@toradex.com>,
	Philippe Schenker <philippe.schenker@toradex.com>,
	Stefan Agner <stefan.agner@toradex.com>,
	Max Krummenacher <max.krummenacher@toradex.com>,
	Oleksandr Suvorov <oleksandr.suvorov@toradex.com>,
	Sanchayan Maity <maitysanchayan@gmail.com>,
	Igor Opaniuk <igor.opaniuk@toradex.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v1 1/2] usb: chipidea: use of extcon framework to work for non OTG case
Date: Wed, 23 Oct 2019 08:28:07 +0000	[thread overview]
Message-ID: <20191023082755.GA8578@b29397-desktop> (raw)
In-Reply-To: <CAByghJbAnAN8dVAa0taPfLiMx2r4evcZhoryfZYyxjYiaFpTsw@mail.gmail.com>

On 19-10-22 16:54:30, Igor Opaniuk wrote:
> Hi Peter,
> 
> On Tue, Oct 22, 2019 at 5:11 AM Peter Chen <peter.chen@nxp.com> wrote:
> >
> > On 19-10-21 19:16:53, Igor Opaniuk wrote:
> > > From: Stefan Agner <stefan.agner@toradex.com>
> > >
> > > The existing usage of extcon in chipidea driver freezes the kernel
> > > presumably due to OTGSC register access.
> > >
> > > Prevent accessing any OTG registers for SoC with dual role devices
> > > but no true OTG support. Use the flag CI_HDRC_DUAL_ROLE_NOT_OTG for
> > > those devices and in case extcon is present, do the role switch
> > > using extcon only.
> >
> > Hi Igor & Stefan,
> >
> > I have several questions about the problem you met:
> > - Which vendor's controller you have used?
> > - What do you mean "no true OTG"? Does it mean no "OTGSC" register?
> 
> Probably the commit message adds a bit of confusion here
> (I've kept the original one from the patch in our downstream kernel,
> but will probably reword it).
> 
> The actual problem is that USB_OTG1_ID pin isn't wired, so we can't rely
> on the value of ID pin state in OTGSC for the role detection.
> In our SoM (Colibri iMX6ULL) ID pin from USB connector is wired
> to SNVS_TAMPER2 which is pinmuxed as GPIO pin (GPIO5_02),
> [1] (this is schematic for the Carrier Board, not SoM (isn't publicly
> available),
> but there is a pretty good explanation + schematic
> in the section "2.3.2.2 USB 2.0 OTG Schematic Example ").

Ok, I clear now. Then, you may not use CI_HDRC_DUAL_ROLE_NOT_OTG which
is for the controller without OTGSC. For imx6ull, access OTGSC will not
hang the system if USB is NOT at suspend mode.

Current upstream design has already considered the user case for switch
role through GPIO, but there is an issue that the external cable
wakeup doesn't work, I will submit it later (see ci_extcon_wakeup_int
implementation at downstream kernel).

You could try to disable runtime-pm to see if the behaviour is expected
or not, if it is NOT expected, please report what's that?

> 
> >
> > >       if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
> > >               ret = ci_hdrc_host_init(ci);
> > > @@ -1145,8 +1208,18 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> > >
> > >       if (!ci_otg_is_fsm_mode(ci)) {
> > >               /* only update vbus status for peripheral */
> > > -             if (ci->role == CI_ROLE_GADGET)
> > > -                     ci_handle_vbus_change(ci);
> > > +             if (dr_mode == USB_DR_MODE_PERIPHERAL) {
> > > +                     usb_gadget_vbus_connect(&ci->gadget);
> >
> > We only use ci->role at runtime, since it has already considered the
> > dts setting, kernel configuration and hardware setting.
> >
> > If your controller doesn't otgsc register, but do need to support
> > role switch, you may enhance the function ci_get_role
> 
> Btw, ci_get_role() implementation still resides in the NXP dowstream kernel
> and I've never seen anything posted to the ML (if it was, could you
> please point me to
> the patch?). I can introduce the new one, which wraps both OTGSC handling
> + extcon for CI_HDRC_DUAL_ROLE_NOT_OTG controllers.

Sorry about that, I just read code for the upstream kernel with some
downstream patches on it.

> 
> Frankly speaking, I don't know the reason why additional workqueue (ci->work_dr)
> was introduced (will try to reach Stefan regarding this).
> As I see it's valid to call extcon_get_state() from the atomic
> context, so probably
> using something like ci_get_role()(or ci_detect_role(), whatever)
> instead of explicitly
> retrieving bits from OTGSC in every ID pin check is a good choice.
> 

There are VBUS and ID events handling which are not non-atomic.

-- 

Thanks,
Peter Chen