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.
> 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@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";
> 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
