Re: [PATCH 2/2] dt-bindings: document a new quirk for dwc3

[Mauro Carvalho Chehab]

Em Thu, 17 Sep 2020 08:47:48 -0600
Rob Herring <robh@xxxxxxxxxx> escreveu:

> On Thu, Sep 17, 2020 at 1:18 AM Mauro Carvalho Chehab
> <mchehab+huawei@xxxxxxxxxx> wrote:
> >
> > Em Tue, 15 Sep 2020 10:38:14 -0600
> > Rob Herring <robh@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > > ---
> > > >  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.

Yeah, I agree that the higher chance is that this would be SoC specific,
but I can't discard being board-specific either, as, on this board,
all the output ports are all connected via an USB GPIO hub provided
on a different chipset. Funny enough, setting this is only required
for HID. The USB ports work fine either with split mode enabled or
disabled for USB sticks. So, I guess this affects only INT (and maybe
ISOC) URB transfers. So I wouldn't doubt that such quirk would be
required due to some limitation at the USB GPIO hub.

> > 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@ff100000 {
> >                         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";

I tried to do that, but the ARM CPU panics:

	https://lore.kernel.org/linux-usb/731e13f9fbba3a81bedb39f1c1deaf41200acd0c.1599559004.git.mchehab+huawei@xxxxxxxxxx/

From my tests, it sounds that this is due to some (minor) differences on 
how the power clocks are enabled/suspended when using dwc3-of-simple as
the parent node. 

It sounds that, if the PM clocks are not stable yet by the time
dwc3 tries to initialize, the CPU crashes.
	
> > 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.

Not sure if a match table would work here, because we need to
enforce that the parent node will be called before dwc3, as
otherwise the panic() will occur.

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

Ok, thanks!

Thanks,
Mauro