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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 38BA4C49EA1 for ; Sat, 3 Aug 2024 15:15:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:In-Reply-To:From:References:Cc:To:Subject:MIME-Version:Date: Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=Kmc0HMaa2ajQKvd2gCDJeDWLAt1l3M51pckiwCkUYLg=; b=z+tnig5UHO4HSWcvRSOSaXMRTb TbGw3lySFMpBTZ9D2Y4cJBVrm5OqGDdGt58FtPM7PGa1kdsvp4OPG5eE3Cyhtt0KYKGUW96JJljSP apo78x0KunWnJMeGr8ZJKjBTAmQGUx09z8k5MsdjaWVH7CAx/F0HnrkbQFxkPwLXrnl5TL8qN7rjA wSFYAOPcIraUCwWkJ1byDgNKMKs6lRlCmEa8SNMRKsTVQPH77J/lUgcfTIibE5+SkigOERYnLPh9d rypIiGWLuqhyWLAokXWmildCf7xCAt+bQC1PB/Xrk/Ur2gSGqhzJPNn7+NAMNXumz5gYXK4MstTiW u35ZpADg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1saGT5-0000000Bc4o-1jH8; Sat, 03 Aug 2024 15:14:51 +0000 Received: from dfw.source.kernel.org ([2604:1380:4641:c500::1]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1saGSZ-0000000Bbx3-2Dee for linux-arm-kernel@lists.infradead.org; Sat, 03 Aug 2024 15:14:21 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id A617D6230F; Sat, 3 Aug 2024 15:14:18 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 4DF51C116B1; Sat, 3 Aug 2024 15:14:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1722698058; bh=cwCCGZI2Vd0oUzsSHQwvBk+JNtPxWz1ngmqZB+vVGIU=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=KequMzCIQjXj/O/yRk0GjPW8aiRKXJ5i5UItEcb6jIKf5m4Q0TLviOjKcMITUc4r9 BhDJGF9uh6K1DyJXt/oINs1lvvNhc6Z0F4RVKmtpIYXMW9OIYGg/raOXGzSDbdC5GF 8QdDTLeUVOfzaPTQgbHJHieA8a6CZ0Bm3g//4oo485/muEl3PIo7YS9s64ymksocxe pPW3bR+CO+EWCTY0XP9VcxtibZ1PG0Ubr1MWkcdz6UbP8GnsSPOnVfcCOW9JP9AUwO VsnP053P8yZHLEPZX/wOmSnIjlUJlJ0VwnMGGG11FS42MgcFVyXnFQbnt6Kn0KCck1 JUVAlA6C0wq+g== Message-ID: <37760e53-4e0d-4275-8497-1b51dcf72d5a@kernel.org> Date: Sat, 3 Aug 2024 18:14:11 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v5 00/12] Fix USB suspend on TI J7200 (cdns3-ti, cdns3, xhci) To: =?UTF-8?Q?Th=C3=A9o_Lebrun?= , Greg Kroah-Hartman , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Peter Chen , Pawel Laszczak , Mathias Nyman , Nishanth Menon , Vignesh Raghavendra , Tero Kristo Cc: linux-usb@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Kevin Hilman , =?UTF-8?Q?Gr=C3=A9gory_Clement?= , Thomas Petazzoni , Conor Dooley References: <20240726-s2r-cdns-v5-0-8664bfb032ac@bootlin.com> Content-Language: en-US From: Roger Quadros In-Reply-To: <20240726-s2r-cdns-v5-0-8664bfb032ac@bootlin.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240803_081419_681112_331E0FA1 X-CRM114-Status: GOOD ( 33.70 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi, On 26/07/2024 21:17, Théo Lebrun wrote: > Currently, system-wide suspend is broken on J7200 because of a > controller reset. The TI wrapper does not get re-initialised at resume > and the first register access from cdns core fails. > > We address that in a few ways: > - In cdns3-ti, if a reset has occured at resume, we reconfigure the HW. > - We pass the XHCI_RESET_ON_RESUME quirk, meaning the XHCI core expects > a resume. OK. > - We add a xhci->lost_power flag. Why? > > The previous revision had one big issue: we had to know if > reset-on-resume was true, at probe-time. This is where the main Don't we already know this at probe-time? why not just rely on the existing XHCI_RESET_ON_RESUME qurik, than add a new mechanism? > difference with previous revisions is. We now pass the information from > wrapper devices back up into XHCI. The xhci->lost_power flag gets its > default value from the XHCI_RESET_ON_RESUME quirk. It however allows > wrappers to signal *at resume* if they still expect a reset. > > That means wrappers that are unsure if they will reset should: > - (1) set the quirk at probe and, > - (2) potentially set xhci->lost_power to false at resume. > > We implement that for cdns3, by piggybacking on the host role ->resume() > callback already receives the information from its caller. > > Have a nice day, > Théo > > Signed-off-by: Théo Lebrun > --- > Changes in v5: > - dt-bindings: take Reviewed-by Rob and Conor for the first > patch: "dt-bindings: usb: ti,j721e-usb: fix compatible list". > - cdns3-ti: > - We now do have HW init code inside cdns_ti_reset_and_init_hw(). > - It gets called at probe unconditionally and from ->runtime_resume() > if a reset is detected (using the W1 register). > - Auxdata patches have been reworked now that there is default auxdata > since commit b50a2da03bd9 ("usb: cdns3-ti: Add workaround for > Errata i2409"). We now have a patch that moves auxdata to match > data: "usb: cdns3-ti: grab auxdata from match data". > - cdns3/xhci: those are three new patches. > - First, we rename "hibernated" to "lost_power" in arguments to > the role ->resume() callbacks. > - Then we add the xhci->lost_power flag, and only have it always copy > the value from XHCI_RESET_ON_RESUME. > - Finally, we set the flag from the host role driver. > - Link to v4: https://lore.kernel.org/lkml/20240307-j7200-usb-suspend-v4-0-5ec7615431f3@bootlin.com/ > > Changes in v4: > - dt-bindings: usb: ti,j721e-usb: > - Remove ti,am64-usb single compatible entry. > - Reverse ordering of compatible pair j721e + am64 > (becoming am64 + j721e). > - Add j7200 + j721e compatible pair (versus only j7200). It is the > same thing as am64: only the integration differs with base j721e > compatible. > - NOT taking trailers from Conor as patches changed substantially. > - arm64: dts: ti: j3-j7200: > - Use j7200 + j721e compatible pair (versus only j7200 previously). > - arm64: dts: ti: j3-am64: > - Fix to use am64 + j721e compatible pair (versus only am64). > This is a new patch. > - Link to v3: https://lore.kernel.org/r/20240223-j7200-usb-suspend-v3-0-b41c9893a130@bootlin.com > > Changes in v3: > - dt-bindings: use an enum to list compatibles instead of the previous > odd construct. This is done in a separate patch from the one adding > J7200 compatible. > - dt-bindings: dropped Acked-by Conor as the changes were modified a lot. > - Add runtime PM back. Put the init sequence in ->runtime_resume(). It > gets called at probe for all compatibles and at resume for J7200. > - Introduce a cdns_ti_match_data struct rather than rely on compatible > from code. > - Reorder code changes. Add infrastructure based on match data THEN add > compatible and its match data. > - DTSI: use only J7200 compatible rather than both J7200 then J721E. > - Link to v2: https://lore.kernel.org/r/20231120-j7200-usb-suspend-v2-0-038c7e4a3df4@bootlin.com > > Changes in v2: > - Remove runtime PM from cdns3-ti; it brings nothing. That means our > cdns3-ti suspend/resume patch is simpler; there is no need to handle > runtime PM at suspend/resume. > - Do not add cdns3 host role suspend/resume callbacks; they are not > needed as core detects reset on resume & calls cdns_drd_host_on when > needed. > - cdns3-ti: Move usb2_refclk_rate_code assignment closer to the value > computation. > - cdns3/host.c: do not pass XHCI_SUSPEND_RESUME_CLKS quirk to xHCI; it > is unneeded on our platform. > - Link to v1: https://lore.kernel.org/r/20231113-j7200-usb-suspend-v1-0-ad1ee714835c@bootlin.com > > --- > Théo Lebrun (12): > dt-bindings: usb: ti,j721e-usb: fix compatible list > dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible > usb: cdns3-ti: move reg writes to separate function > usb: cdns3-ti: run HW init at resume() if HW was reset > usb: cdns3: add quirk to platform data for reset-on-resume > usb: cdns3-ti: grab auxdata from match data > usb: cdns3-ti: add J7200 support with reset-on-resume behavior > usb: cdns3: rename hibernated argument of role->resume() to lost_power > xhci: introduce xhci->lost_power flag > usb: cdns3: host: transmit lost_power signal from wrapper to XHCI > arm64: dts: ti: k3-j7200: use J7200-specific USB compatible > arm64: dts: ti: k3-am64: add USB fallback compatible to J721E > > .../devicetree/bindings/usb/ti,j721e-usb.yaml | 5 +- > arch/arm64/boot/dts/ti/k3-am64-main.dtsi | 2 +- > arch/arm64/boot/dts/ti/k3-j7200-main.dtsi | 2 +- > drivers/usb/cdns3/cdns3-gadget.c | 4 +- > drivers/usb/cdns3/cdns3-ti.c | 151 ++++++++++++++------- > drivers/usb/cdns3/cdnsp-gadget.c | 2 +- > drivers/usb/cdns3/core.h | 3 +- > drivers/usb/cdns3/host.c | 13 ++ > drivers/usb/host/xhci.c | 8 +- > drivers/usb/host/xhci.h | 6 + > 10 files changed, 136 insertions(+), 60 deletions(-) > --- > base-commit: c33ffdb70cc6df4105160f991288e7d2567d7ffa > change-id: 20240726-s2r-cdns-4b180cd960ff > > Best regards, -- cheers, -roger