From: Kevin Hilman <khilman@kernel.org> To: "Théo Lebrun" <theo.lebrun@bootlin.com>, "Roger Quadros" <rogerq@kernel.org>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>, "Rob Herring" <robh+dt@kernel.org>, "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>, "Conor Dooley" <conor+dt@kernel.org>, "Peter Chen" <peter.chen@kernel.org>, "Pawel Laszczak" <pawell@cadence.com>, "Nishanth Menon" <nm@ti.com>, "Vignesh Raghavendra" <vigneshr@ti.com>, "Tero Kristo" <kristo@kernel.org>, "Vardhan, Vibhore" <vibhore@ti.com> Cc: linux-usb@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, "Grégory Clement" <gregory.clement@bootlin.com>, "Thomas Petazzoni" <thomas.petazzoni@bootlin.com> Subject: Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200 Date: Tue, 12 Dec 2023 10:26:05 -0800 [thread overview] Message-ID: <7h7cljcm6a.fsf@baylibre.com> (raw) In-Reply-To: <CX9MMPFL7HAY.NGULD1FN5WPN@tleb-bootlin-xps13-01> Théo Lebrun <theo.lebrun@bootlin.com> writes: > Hello, > > On Sun Nov 26, 2023 at 11:36 PM CET, Kevin Hilman wrote: >> Théo Lebrun <theo.lebrun@bootlin.com> writes: >> > On Wed Nov 22, 2023 at 11:23 PM CET, Kevin Hilman wrote: >> >> Théo Lebrun <theo.lebrun@bootlin.com> writes: >> >> The point is to signal to the power domain the device is in that it can >> >> power on/off. These IP blocks are (re)used on many different SoCs, so >> >> the driver should not make any assumptions about what power domain it is >> >> in (if any.) >> > >> > On my platform, when the device is attached to the PD it gets turned on. >> > That feels logical to me: if a driver is not RPM aware it "just works". >> >> It "just works"... until the domain gets turned off. >> >> > Are there platforms where RPM must get enabled for the attached >> > power-domains to be turned on? >> >> Yes, but but more importantly, there are platforms where RPM must get >> enabled for the power domain to *stay* on. For example, the power >> domain might get turned on due to devices probing etc, but as soon as >> all the RPM-enabled drivers drop their refcount, the domain will turn >> off. If there is a device in that domain with a non-RPM enabled driver, >> that device will be powered off anc cause a crash. > > OK, that makes sense, thanks for taking the time to explain. This topic > makes me see two things that I feel are close to being bugs. I'd be > curious to get your view on both. TL;DR; they are features, not bugs. ;) > - If a device does not use RPM but its children do, it might get its > associated power-domain turned off. That forces every single driver > that want to stay alive to enable & increment RPM. > > What I naively expect: a genpd with a device attached to it that is > not using RPM should mean that it should not be powered off at > runtime_suspend. Benefit: no RPM calls in drivers that do not use > it, and the behavior is that the genpd associated stays alive "as > expected". Your expectation makes sense, but unfortunately, that's not how RPM was designed. Also remember that we don't really want specific device drivers to know which PM domain they are in, or whether they are in a PM domain at all. The same IP block can be integrated in different ways across different SoCs, even within the same SoC family, and we want the device driver to just work. For that to work well, any driver that might be in any PM domain should add RPM calls. > - If a device uses RPM & has a refcount strictly positive, its > associated power-domain gets turned off either way at suspend_noirq. > That feels non-intuitive as well. > > What I naively expect: check for RPM refcounts of attached devices > when doing suspend_noirq of power-domains. Benefit: control of what > power-domains do from attached devices is done through the RPM API. I agree that this is non-intuitive from an RPM PoV, but remember that RPM was added on top of existing system-wide suspend support. And from a system-wide suspend PoV, it might be non-intuitive that a driver thinks it should be active (non-zero refcount) when user just requested a system-wide suspend. Traditionally, when a user requests a system-wide suspend, they expect the whole system to shut down. On real SoCs in real products, power management is not so black and white, and I fully understand that, and personally, I'm definitely open to not forcing RPM-active devices off in suspend, but that would require changes to core code, and remove some assumptions of core code that would need to be validated/tested. Kevin
WARNING: multiple messages have this Message-ID (diff)
From: Kevin Hilman <khilman@kernel.org> To: "Théo Lebrun" <theo.lebrun@bootlin.com>, "Roger Quadros" <rogerq@kernel.org>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>, "Rob Herring" <robh+dt@kernel.org>, "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>, "Conor Dooley" <conor+dt@kernel.org>, "Peter Chen" <peter.chen@kernel.org>, "Pawel Laszczak" <pawell@cadence.com>, "Nishanth Menon" <nm@ti.com>, "Vignesh Raghavendra" <vigneshr@ti.com>, "Tero Kristo" <kristo@kernel.org>, "Vardhan, Vibhore" <vibhore@ti.com> Cc: linux-usb@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, "Grégory Clement" <gregory.clement@bootlin.com>, "Thomas Petazzoni" <thomas.petazzoni@bootlin.com> Subject: Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200 Date: Tue, 12 Dec 2023 10:26:05 -0800 [thread overview] Message-ID: <7h7cljcm6a.fsf@baylibre.com> (raw) In-Reply-To: <CX9MMPFL7HAY.NGULD1FN5WPN@tleb-bootlin-xps13-01> Théo Lebrun <theo.lebrun@bootlin.com> writes: > Hello, > > On Sun Nov 26, 2023 at 11:36 PM CET, Kevin Hilman wrote: >> Théo Lebrun <theo.lebrun@bootlin.com> writes: >> > On Wed Nov 22, 2023 at 11:23 PM CET, Kevin Hilman wrote: >> >> Théo Lebrun <theo.lebrun@bootlin.com> writes: >> >> The point is to signal to the power domain the device is in that it can >> >> power on/off. These IP blocks are (re)used on many different SoCs, so >> >> the driver should not make any assumptions about what power domain it is >> >> in (if any.) >> > >> > On my platform, when the device is attached to the PD it gets turned on. >> > That feels logical to me: if a driver is not RPM aware it "just works". >> >> It "just works"... until the domain gets turned off. >> >> > Are there platforms where RPM must get enabled for the attached >> > power-domains to be turned on? >> >> Yes, but but more importantly, there are platforms where RPM must get >> enabled for the power domain to *stay* on. For example, the power >> domain might get turned on due to devices probing etc, but as soon as >> all the RPM-enabled drivers drop their refcount, the domain will turn >> off. If there is a device in that domain with a non-RPM enabled driver, >> that device will be powered off anc cause a crash. > > OK, that makes sense, thanks for taking the time to explain. This topic > makes me see two things that I feel are close to being bugs. I'd be > curious to get your view on both. TL;DR; they are features, not bugs. ;) > - If a device does not use RPM but its children do, it might get its > associated power-domain turned off. That forces every single driver > that want to stay alive to enable & increment RPM. > > What I naively expect: a genpd with a device attached to it that is > not using RPM should mean that it should not be powered off at > runtime_suspend. Benefit: no RPM calls in drivers that do not use > it, and the behavior is that the genpd associated stays alive "as > expected". Your expectation makes sense, but unfortunately, that's not how RPM was designed. Also remember that we don't really want specific device drivers to know which PM domain they are in, or whether they are in a PM domain at all. The same IP block can be integrated in different ways across different SoCs, even within the same SoC family, and we want the device driver to just work. For that to work well, any driver that might be in any PM domain should add RPM calls. > - If a device uses RPM & has a refcount strictly positive, its > associated power-domain gets turned off either way at suspend_noirq. > That feels non-intuitive as well. > > What I naively expect: check for RPM refcounts of attached devices > when doing suspend_noirq of power-domains. Benefit: control of what > power-domains do from attached devices is done through the RPM API. I agree that this is non-intuitive from an RPM PoV, but remember that RPM was added on top of existing system-wide suspend support. And from a system-wide suspend PoV, it might be non-intuitive that a driver thinks it should be active (non-zero refcount) when user just requested a system-wide suspend. Traditionally, when a user requests a system-wide suspend, they expect the whole system to shut down. On real SoCs in real products, power management is not so black and white, and I fully understand that, and personally, I'm definitely open to not forcing RPM-active devices off in suspend, but that would require changes to core code, and remove some assumptions of core code that would need to be validated/tested. Kevin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2023-12-12 18:26 UTC|newest] Thread overview: 70+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-11-13 14:26 [PATCH 0/6] usb: cdns: fix suspend on J7200 by assuming reset on resume Théo Lebrun 2023-11-13 14:26 ` Théo Lebrun 2023-11-13 14:26 ` [PATCH 1/6] dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible Théo Lebrun 2023-11-13 14:26 ` Théo Lebrun 2023-11-13 19:58 ` Conor Dooley 2023-11-13 19:58 ` Conor Dooley 2023-11-13 14:26 ` [PATCH 2/6] usb: cdns3-ti: move reg writes from probe into an init_hw helper Théo Lebrun 2023-11-13 14:26 ` Théo Lebrun 2023-11-15 11:33 ` Roger Quadros 2023-11-15 11:33 ` Roger Quadros 2023-11-15 14:23 ` Théo Lebrun 2023-11-15 14:23 ` Théo Lebrun 2023-11-16 12:00 ` Roger Quadros 2023-11-16 12:00 ` Roger Quadros 2023-11-13 14:26 ` [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200 Théo Lebrun 2023-11-13 14:26 ` Théo Lebrun 2023-11-13 15:39 ` Gregory CLEMENT 2023-11-13 15:39 ` Gregory CLEMENT 2023-11-14 11:13 ` Théo Lebrun 2023-11-14 11:13 ` Théo Lebrun 2023-11-15 11:37 ` Roger Quadros 2023-11-15 11:37 ` Roger Quadros 2023-11-15 15:02 ` Théo Lebrun 2023-11-15 15:02 ` Théo Lebrun 2023-11-16 12:40 ` Roger Quadros 2023-11-16 12:40 ` Roger Quadros 2023-11-16 18:56 ` Théo Lebrun 2023-11-16 18:56 ` Théo Lebrun 2023-11-16 21:44 ` Roger Quadros 2023-11-16 21:44 ` Roger Quadros 2023-11-17 10:17 ` Théo Lebrun 2023-11-17 10:17 ` Théo Lebrun 2023-11-17 11:51 ` Roger Quadros 2023-11-17 11:51 ` Roger Quadros 2023-11-17 14:20 ` Théo Lebrun 2023-11-17 14:20 ` Théo Lebrun 2023-11-18 10:41 ` Roger Quadros 2023-11-18 10:41 ` Roger Quadros 2023-11-22 22:23 ` Kevin Hilman 2023-11-22 22:23 ` Kevin Hilman 2023-11-23 9:51 ` Théo Lebrun 2023-11-23 9:51 ` Théo Lebrun 2023-11-26 22:36 ` Kevin Hilman 2023-11-26 22:36 ` Kevin Hilman 2023-11-27 13:25 ` Théo Lebrun 2023-11-27 13:25 ` Théo Lebrun 2023-12-12 18:26 ` Kevin Hilman [this message] 2023-12-12 18:26 ` Kevin Hilman 2023-12-12 19:31 ` Alan Stern 2023-12-12 19:31 ` Alan Stern 2023-11-13 14:26 ` [PATCH 4/6] usb: cdns3: support power-off of controller when in host role Théo Lebrun 2023-11-13 14:26 ` Théo Lebrun 2023-11-14 8:38 ` Peter Chen 2023-11-14 8:38 ` Peter Chen 2023-11-14 11:10 ` Théo Lebrun 2023-11-14 11:10 ` Théo Lebrun 2023-11-17 3:38 ` Peter Chen 2023-11-17 3:38 ` Peter Chen 2023-11-17 9:58 ` Théo Lebrun 2023-11-17 9:58 ` Théo Lebrun 2023-11-20 5:44 ` Peter Chen 2023-11-20 5:44 ` Peter Chen 2023-11-13 14:27 ` [PATCH 5/6] usb: cdns3-ti: notify cdns core that hardware resets across suspend on J7200 Théo Lebrun 2023-11-13 14:27 ` Théo Lebrun 2023-11-13 14:27 ` [PATCH 6/6] arm64: dts: ti: k3-j7200: use J7200-specific USB compatible Théo Lebrun 2023-11-13 14:27 ` Théo Lebrun 2023-11-14 10:01 ` Gregory CLEMENT 2023-11-14 10:01 ` Gregory CLEMENT 2023-11-14 11:14 ` Théo Lebrun 2023-11-14 11:14 ` Théo Lebrun
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=7h7cljcm6a.fsf@baylibre.com \ --to=khilman@kernel.org \ --cc=conor+dt@kernel.org \ --cc=devicetree@vger.kernel.org \ --cc=gregkh@linuxfoundation.org \ --cc=gregory.clement@bootlin.com \ --cc=kristo@kernel.org \ --cc=krzysztof.kozlowski+dt@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-usb@vger.kernel.org \ --cc=nm@ti.com \ --cc=pawell@cadence.com \ --cc=peter.chen@kernel.org \ --cc=robh+dt@kernel.org \ --cc=rogerq@kernel.org \ --cc=theo.lebrun@bootlin.com \ --cc=thomas.petazzoni@bootlin.com \ --cc=vibhore@ti.com \ --cc=vigneshr@ti.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.