Message-ID: <CAL_JsqLucKWwgBVAoyXpm1mCD5-OvFj2pM_q2+tcyA+K9fCnKg@mail.gmail.com>
Date:   Thu, 17 Sep 2020 08:47:48 -0600
From:   Rob Herring <robh@...nel.org>
To:     Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
Cc:     Felipe Balbi <balbi@...nel.org>, Linuxarm <linuxarm@...wei.com>,
        mauro.chehab@...wei.com, John Stultz <john.stultz@...aro.org>,
        Manivannan Sadhasivam <mani@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linux USB List <linux-usb@...r.kernel.org>,
        devicetree@...r.kernel.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] dt-bindings: document a new quirk for dwc3

On Thu, Sep 17, 2020 at 1:18 AM Mauro Carvalho Chehab
<mchehab+huawei@...nel.org> wrote:
>
> Em Tue, 15 Sep 2020 10:38:14 -0600
> Rob Herring <robh@...nel.org> escreveu:
>
> > On Tue, Sep 08, 2020 at 09:20:57AM +0200, Mauro Carvalho Chehab wrote:
> > > At Hikey 970, setting the SPLIT disable at the General
> > > User Register 3 is required.
> > >
> > > Without that, the URBs generated by the usbhid driver
> > > return -EPROTO errors. That causes the code at
> > > hid-core.c to call hid_io_error(), which schedules
> > > a reset_work, causing a call to hid_reset().
> > >
> > > In turn, the code there will call:
> > >
> > >     usb_queue_reset_device(usbhid->intf);
> > >
> > > The net result is that the input devices won't work, and
> > > will be reset on every 0.5 seconds:
> > >
> > >     [   33.122384] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> > >     [   33.378220] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > >     [   33.698394] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0000
> > >     [   34.882365] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> > >     [   35.138217] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > >     [   35.458617] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0000
> > >     [   36.642392] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> > >     [   36.898207] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > >     [   37.218598] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0000
> > >     [   38.402368] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> > >     [   38.658174] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > >     [   38.978594] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0000
> > >     [   40.162361] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> > >     [   40.418148] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > >     ...
> > >     [  397.698132] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > >
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
> > > ---
> > >  Documentation/devicetree/bindings/usb/dwc3.txt | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> > > index d03edf9d3935..1aae2b6160c1 100644
> > > --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> > > +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> > > @@ -78,6 +78,9 @@ Optional properties:
> > >                     park mode are disabled.
> > >   - snps,dis_metastability_quirk: when set, disable metastability workaround.
> > >                     CAUTION: use only if you are absolutely sure of it.
> > > + - snps,dis-split-quirk: when set, change the way URBs are handled by the
> > > +                    driver. Needed to avoid -EPROTO errors with usbhid
> > > +                    on some devices (Hikey 970).
> >
> > Can't this be implied by the compatible string? Yes we have quirk
> > properties already, but the problem with them is you can't address them
> > without a DT change.
>
> Short answer:
>
> While technically doable, I don't think that this would be a good idea.
>
> -
>
> Long answer:
>
> The first thing is related to the compatible namespace: should such
> quirk be added at SoC level or at board level?
>
> I don't know if the need to disable split mode came from a different
> dwc3 variant used by the Kirin 970 SoC, or if this is due to the way
> USB is wired at the Hikey 970 board.

If board specific, then I agree that a separate property makes sense.
This doesn't really sound board specific though.

> Right now, I'm assuming the latter, but Felipe suggested that this
> might be due to a different version of the IP. Currently, we have
> no means to check.
>
> So, I'm placing all Hikey 970 specific quirks under the board-specific
> part, e. g., at:
>
>         arch/arm64/boot/dts/hisilicon/hi3670-hikey970.dts
>
> This sounds more flexible, as, if some different hardware based on
> the same chipset reaches upstream, it could use a different set of
> quirks, if needed.
>
> However, if we decide to bind quirks with compatible strings,
> we need to know if we would create a board-specific compatible
> or just SoC-specific ones.
>
> -
>
> Another possibility would be to add generic compatible bindings
> for each quirk featureset. Right now the dwc3 core driver accepts
> only two compatible strings:
>
>        .compatible = "snps,dwc3"
>        .compatible = "synopsys,dwc3"

Yeah, the binding for DWC3 is odd.

> And both are equivalent. No quirks are added there via compatible
> strings.
>
> Ok, we might start adding different compatible strings for different
> sets of quirks, like:
>
>         .compatible = "snps,dwc3-splitdisable"

Um, no.

>
> but, this sounds really ugly, specially when multiple quirks
> would be required.
>
> We might also deprecate the usage of "snps,dwc3"/"synopsys,dwc3",
> in favor of SoC-specific and board-specific compatible strings,
> but that would add a long list of boards there, with lots of code
> to set quirks. IMHO, it is a lot nicer to rely on DT to enable
> or disable those SoC and board-specific optional features of the
> Designware IP.
>
> -
>
> In the specific case of Hikey 970, there are two other
> alternatives:
>
> 1) we ended needing to create a new compatible for the Kirin 970
> SoC, for it to be probed via this driver:
>
>         drivers/usb/dwc3/dwc3-of-simple.c
>
> as, otherwise an async ARM error happens, making the SoC to
> crash. All dwc3-of-simple driver does is to use a different
> code for initializing the clocks.
>
> However, dwc3-of-simple driver is completely independent from
> dwc3: it doesn't pass platform data to the main dwc3 driver.
> So, it doesn't propagate any quirk to the main driver.
>
> One possible hack would be to make dwc3 driver to also
> accept platform data, using it as an interface for the
> dwc3-of-simple to pass quirks.
>
> If we go on that direction, we could also remove all other
> quirks from Kirin 970 dwc3, coding them inside the driver,
> instead of using DT, e. g. the driver would do something like:
>
>         if (of_device_is_compatible(np,
>                                    "hisilicon,hi3670-dwc3")) {
>                 cfg->dis_split_quirk = true;
>                 cfg->foo = true;
>                 cfg->bar = true;

Normally, this would all be driver match data.

>                 ...
>
>         }
>
> such change would require a re-design at the logic around
> dwc3_get_properties(), as the driver should start accepting
> quirks either from platform_data or from DT (or both?).
>
> 2) Because this specific device uses the dwc3-of-simple driver,
> the actual DT binding is:
>
>         usb3: hisi_dwc3 {
>                 compatible = "hisilicon,hi3670-dwc3";
>         ...
>                 dwc3: dwc3@...00000 {
>                         compatible = "snps,dwc3";
>         ...
>                 };
>         };

This parent child split should never have happened for the cases that
don't have 'wrapper registers'. We should have had on node here with
just:

compatible = "hisilicon,hi3670-dwc3", "snps,dwc3";

> For boards that use dwc3-of-simple drivers, we could add a hack
> at the dwc3 core that would seek for the parent's device's
> compatible string with something like (not tested):
>
>         if (of_device_is_compatible(pdev->parent->dev.of_node,
>                                    "hisilicon,hi3670-dwc3"))
>                 dwc->dis_split_quirk = true;

This is what I'd do. You could have a match table instead as that
would scale better.

> It should be noticed that both platform_data and pdev->parent
> alternatives will only work for boards using dwc3-of-simple driver.
> This could limit this quirk usage on future devices.
>
> -
>
> IMO, adding a new quirk is cleaner, and adopts the same solution
> that it is currently used by other drivers with Designware IP.

We already have a bunch of quirk properties. What's one more, sigh. So
if that's what you want, fine.

Rob

Powered by blists - more mailing lists