From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.6 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_2 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id CCC14C43464 for ; Fri, 18 Sep 2020 09:40:28 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 6519721D42 for ; Fri, 18 Sep 2020 09:40:28 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1600422028; bh=JEO9vjwBuQ6Zi3z0GE1BJYqfFToSd0zOu/hoFWxXemA=; h=Date:From:To:Cc:Subject:In-Reply-To:References:List-ID:From; b=DDUHbGz52RQca3h6EF8YvevUHkY6+2Jy/qKEDgt1ZPgRmlxqQK03/+nyQMdFkzwyL /yBLtB8Eal9TsloAwXLJELyv4RMZh2Y0SSHXSmv9yveXSIOBYkRwT7xE1qHHDpot5U k63i6xrCfcjtlXHBgdjR0S0Ek/jjX8KuISfBrlFA= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726159AbgIRJk2 (ORCPT ); Fri, 18 Sep 2020 05:40:28 -0400 Received: from mail.kernel.org ([198.145.29.99]:45064 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726109AbgIRJk1 (ORCPT ); Fri, 18 Sep 2020 05:40:27 -0400 Received: from coco.lan (ip5f5ad5d2.dynamic.kabel-deutschland.de [95.90.213.210]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id E7BA621973; Fri, 18 Sep 2020 09:40:24 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1600422027; bh=JEO9vjwBuQ6Zi3z0GE1BJYqfFToSd0zOu/hoFWxXemA=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=UJlD56RObZe0dUkYz3ZGNzJRKEN1+2QE0kuzfmT6J5hpsJk54yCXqDVnv75O0cIzU iyxwoDQVwE1l0JpJ1J5vcRDxmL+WNjT1x9XNHFgMo1NFqW4YxIZxgyrdYujtc9JSX3 B9m4Trh5V19cVU4HbxgHLZzoJsTAeinNH7huwkAY= Date: Fri, 18 Sep 2020 11:40:21 +0200 From: Mauro Carvalho Chehab To: Rob Herring Cc: Felipe Balbi , Linuxarm , mauro.chehab@huawei.com, John Stultz , Manivannan Sadhasivam , Greg Kroah-Hartman , Linux USB List , devicetree@vger.kernel.org, "linux-kernel@vger.kernel.org" Subject: Re: [PATCH 2/2] dt-bindings: document a new quirk for dwc3 Message-ID: <20200918114021.5c67610e@coco.lan> In-Reply-To: References: <20200915163814.GA2084568@bogus> <20200917091821.0de18caa@coco.lan> X-Mailer: Claws Mail 3.17.6 (GTK+ 2.24.32; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org Em Thu, 17 Sep 2020 08:47:48 -0600 Rob Herring escreveu: > On Thu, Sep 17, 2020 at 1:18 AM Mauro Carvalho Chehab > wrote: > > > > Em Tue, 15 Sep 2020 10:38:14 -0600 > > Rob Herring escreveu: > > =20 > > > On Tue, Sep 08, 2020 at 09:20:57AM +0200, Mauro Carvalho Chehab wrote= : =20 > > > > 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 u= sing 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 u= sing 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 u= sing 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 u= sing 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 u= sing xhci-hcd > > > > ... > > > > [ 397.698132] usb 1-1.1: reset low-speed USB device number 3 u= sing xhci-hcd > > > > > > > > Signed-off-by: Mauro Carvalho Chehab > > > > --- > > > > Documentation/devicetree/bindings/usb/dwc3.txt | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Docum= entation/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 w= orkaround. > > > > 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 us= bhid > > > > + on some devices (Hikey 970). =20 > > > > > > 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 th= em > > > without a DT change. =20 > > > > 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. =20 >=20 > 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 =3D "hisilicon,hi3670-dwc3"; > > ... > > dwc3: dwc3@ff100000 { > > compatible =3D "snps,dwc3"; > > ... > > }; > > }; =20 >=20 > 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: >=20 > compatible =3D "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@kernel.org/ =46rom my tests, it sounds that this is due to some (minor) differences on=20 how the power clocks are enabled/suspended when using dwc3-of-simple as the parent node.=20 It sounds that, if the PM clocks are not stable yet by the time dwc3 tries to initialize, the CPU crashes. =09 > > 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 =3D true; =20 >=20 > 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. =20 >=20 > 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