Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191021161654.14353-1-igor.opaniuk@gmail.com> <20191022020748.GA26973@b29397-desktop>
In-Reply-To: <20191022020748.GA26973@b29397-desktop>
From:   Igor Opaniuk <igor.opaniuk@gmail.com>
Date:   Tue, 22 Oct 2019 16:54:30 +0300
Message-ID: <CAByghJbAnAN8dVAa0taPfLiMx2r4evcZhoryfZYyxjYiaFpTsw@mail.gmail.com>
Subject: Re: [PATCH v1 1/2] usb: chipidea: use of extcon framework to work for
 non OTG case
To:     Peter Chen <peter.chen@nxp.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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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 ").

>
> >       if (dr_mode =3D=3D USB_DR_MODE_OTG || dr_mode =3D=3D USB_DR_MODE_=
HOST) {
> >               ret =3D 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 =3D=3D CI_ROLE_GADGET)
> > -                     ci_handle_vbus_change(ci);
> > +             if (dr_mode =3D=3D 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.

Frankly speaking, I don't know the reason why additional workqueue (ci->wor=
k_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.

Thanks for your feedback!


>
> Peter
>
> > +             } else if (ci->role =3D=3D CI_ROLE_GADGET) {
> > +                     struct ci_hdrc_cable *vbus =3D &ci->platdata->vbu=
s_extcon;
> > +
> > +                     /* Use vbus state from extcon if provided */
> > +                     if (!IS_ERR(vbus->edev) &&
> > +                         extcon_get_state(vbus->edev, EXTCON_USB))
> > +                             usb_gadget_vbus_connect(&ci->gadget);
> > +                     else
> > +                             ci_handle_vbus_change(ci);
> > +             }
> >
> >               ret =3D ci_role_start(ci, ci->role);
> >               if (ret) {
> > @@ -1161,10 +1234,6 @@ static int ci_hdrc_probe(struct platform_device =
*pdev)
> >       if (ret)
> >               goto stop;
> >
> > -     ret =3D ci_extcon_register(ci);
> > -     if (ret)
> > -             goto stop;
> > -
> >       if (ci->supports_runtime_pm) {
> >               pm_runtime_set_active(&pdev->dev);
> >               pm_runtime_enable(&pdev->dev);
> > --
> > 2.17.1
> >
>
> --
>
> Thanks,
> Peter Chen

[1] https://docs.toradex.com/102491-colibri-arm-carrier-board-design-guide.=
pdf

--=20
Best regards - Freundliche Gr=C3=BCsse - Meilleures salutations

Igor Opaniuk

mailto: igor.opaniuk@gmail.com
skype: igor.opanyuk
+380 (93) 836 40 67
http://ua.linkedin.com/in/iopaniuk