From: Sudeep Holla <sudeep.holla@arm.com> To: Florian Fainelli <f.fainelli@gmail.com> Cc: linux-arm-kernel@lists.infradead.org, "maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE" <bcm-kernel-feedback-list@broadcom.com>, Mark Rutland <mark.rutland@arm.com>, Sudeep Holla <sudeep.holla@arm.com>, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>, open list <linux-kernel@vger.kernel.org> Subject: Re: [PATCH 0/4] Broadcom STB PM PSCI extensions Date: Mon, 7 Feb 2022 16:27:34 +0000 [thread overview] Message-ID: <20220207162734.nt55nflwqstkej3p@bogus> (raw) In-Reply-To: <06f84f27-973d-1dae-e2c3-3fda4e368331@gmail.com> On Thu, Feb 03, 2022 at 11:33:26AM -0800, Florian Fainelli wrote: > > > On 2/3/2022 10:52 AM, Sudeep Holla wrote: > > Correction: it is known as "freeze" rather than "idle" in terms of values > > as per /sys/power/state. Sorry for referring it as "idle" and creating any > > confusion. > > > > On Thu, Feb 03, 2022 at 09:36:28AM -0800, Florian Fainelli wrote: > > > > > > > > > On 2/3/2022 3:14 AM, Sudeep Holla wrote: > > > > On Fri, Jan 21, 2022 at 07:54:17PM -0800, Florian Fainelli wrote: > > > > > Hi all, > > > > > > > > > > This patch series contains the Broadcom STB PSCI extensions which adds > > > > > some additional functions on top of the existing standard PSCI interface > > > > > which is the reason for having the driver implement a custom > > > > > suspend_ops. > > > > > > > > > > These platforms have traditionally supported a mode that is akin to > > > > > ACPI's S2 with the CPU in WFI and all of the chip being clock gated > > > > > which is entered with "echo standby > /sys/power/state". Additional a > > > > > true suspend to DRAM as defined in ACPI by S3 is implemented with "echo > > > > > mem > /sys/power/state". > > > > > > > > How different is the above "standby" state compare to the standard "idle" > > > > (a.k.a suspend-to-idle which is different from system-to-ram/S3) ? > > > > > > There are a few differences: > > > > > > - s2idle does not power gate the secondary CPUs > > > > > > > Not sure what you mean by that ? S2I takes CPUs to deepest idle state. > > If you want shallower states, one possible option is the disable deeper > > states from the userspace. > > What I mean is that we do not get to call PSCI CPU_OFF here so the CPUs are > idle, but not power gated. Those CPUs do not have any other idle state other > than WFI because the HW designers sort of forgot or rather did not know that > wiring up the ARM GIC power controller back to the power gating logic of the > CPU was a good idea. > Nice 😄 > > > > > - s2idle requires the use of in-band interrupts for wake-up > > > > > > > I am not sure if that is true. S2I behaves very similar to S2R except it > > has low wake latency as all secondaries CPUs are not hotplugged out. > > OK, the fact that secondary CPUs are not hot-plugged could be remedied by > doing this ahead of entering s2idle by user-space so this is not a valid > argument from me anymore. > Fair enough. > > > > > The reasons for implementing "standby" are largely two fold: > > > > > > - we need to achieve decent power savings (typically below 0.5W for the > > > whole system while allowing Wake-on-WLAN, GPIO, RTC, infrared, etc.) > > > > > > > I fail to understand how that is a problem from S2I. It is probably worth > > checking if there are any unnecessary IRQF_NO_SUSPEND users. Check section > > IRQF_NO_SUSPEND and enable_irq_wake() in [1]. I don't see any issues other > > wise in terms of unnecessary/spurious wakeup by in-band(to be precise > > no-wake up) interrupts. > > I don't think your hyperlink referenced by [1] was provided, but my quick > testing with: > Yikes, I meant to refer Documentation/power/suspend-and-interrupts.rst > echo s2idle > /sys/power/mem_sleep > echo mem > /sys/power/state > > appears to work to some extent when I use peripherals that can generate > in-band interrupts. > > It looks like we have s2idle_ops that allows a platform to override some of > the operations before/after entering s2idle, however the actual s2idle idle > loop is still within the kernel, so we will not call into the ARM Trusted > Firmware and engage the power management state machine. > Correct. > This means that there will not be any of the clock gating that only the > hardware state machine is capable of performing, the DRAM controller as a > result will not enter self refresh power down, and in addition the side band > wake-up interrupts will not be activate because the interrupt controller > that aggregates them only outputs to the ARM GIC when the state machine has > been engaged. > One possible solution IIUC the issue is to add this as additional CPU Idle state disabled most of the time. Enable them from user-space just prior to calling freeze/s2idle, so that PSCI CPU_SUSPEND is called with right param to indicate this is deepest idle state(in your case just WFI) + DRAM self refresh/retention mode. Also TF-A can take care to enable the side band interrupts before entering the state. Do you see any issue with this approach ? I am trying to find ways to avoid deviating from standard PSCI. > Essentially, what we need for our systems is a controlled system entry with > semantics similar if not identical to that of S2R but with a shallower state > that does not cut the power to 90% of the SoC (unlike S2R) such that we have > a quicker suspend and resume latency. Years ago when we only had MIPS-based > and 32-bit ARM SoCs, we did come up with using "standby" (see > drivers/soc/bcm/brcmstb/pm/*) and we naturally mapped that when we switched > over to ARMv8 capable devices. > Understood and I do remember that. -- Regards, Sudeep
WARNING: multiple messages have this Message-ID (diff)
From: Sudeep Holla <sudeep.holla@arm.com> To: Florian Fainelli <f.fainelli@gmail.com> Cc: linux-arm-kernel@lists.infradead.org, "maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE" <bcm-kernel-feedback-list@broadcom.com>, Mark Rutland <mark.rutland@arm.com>, Sudeep Holla <sudeep.holla@arm.com>, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>, open list <linux-kernel@vger.kernel.org> Subject: Re: [PATCH 0/4] Broadcom STB PM PSCI extensions Date: Mon, 7 Feb 2022 16:27:34 +0000 [thread overview] Message-ID: <20220207162734.nt55nflwqstkej3p@bogus> (raw) In-Reply-To: <06f84f27-973d-1dae-e2c3-3fda4e368331@gmail.com> On Thu, Feb 03, 2022 at 11:33:26AM -0800, Florian Fainelli wrote: > > > On 2/3/2022 10:52 AM, Sudeep Holla wrote: > > Correction: it is known as "freeze" rather than "idle" in terms of values > > as per /sys/power/state. Sorry for referring it as "idle" and creating any > > confusion. > > > > On Thu, Feb 03, 2022 at 09:36:28AM -0800, Florian Fainelli wrote: > > > > > > > > > On 2/3/2022 3:14 AM, Sudeep Holla wrote: > > > > On Fri, Jan 21, 2022 at 07:54:17PM -0800, Florian Fainelli wrote: > > > > > Hi all, > > > > > > > > > > This patch series contains the Broadcom STB PSCI extensions which adds > > > > > some additional functions on top of the existing standard PSCI interface > > > > > which is the reason for having the driver implement a custom > > > > > suspend_ops. > > > > > > > > > > These platforms have traditionally supported a mode that is akin to > > > > > ACPI's S2 with the CPU in WFI and all of the chip being clock gated > > > > > which is entered with "echo standby > /sys/power/state". Additional a > > > > > true suspend to DRAM as defined in ACPI by S3 is implemented with "echo > > > > > mem > /sys/power/state". > > > > > > > > How different is the above "standby" state compare to the standard "idle" > > > > (a.k.a suspend-to-idle which is different from system-to-ram/S3) ? > > > > > > There are a few differences: > > > > > > - s2idle does not power gate the secondary CPUs > > > > > > > Not sure what you mean by that ? S2I takes CPUs to deepest idle state. > > If you want shallower states, one possible option is the disable deeper > > states from the userspace. > > What I mean is that we do not get to call PSCI CPU_OFF here so the CPUs are > idle, but not power gated. Those CPUs do not have any other idle state other > than WFI because the HW designers sort of forgot or rather did not know that > wiring up the ARM GIC power controller back to the power gating logic of the > CPU was a good idea. > Nice 😄 > > > > > - s2idle requires the use of in-band interrupts for wake-up > > > > > > > I am not sure if that is true. S2I behaves very similar to S2R except it > > has low wake latency as all secondaries CPUs are not hotplugged out. > > OK, the fact that secondary CPUs are not hot-plugged could be remedied by > doing this ahead of entering s2idle by user-space so this is not a valid > argument from me anymore. > Fair enough. > > > > > The reasons for implementing "standby" are largely two fold: > > > > > > - we need to achieve decent power savings (typically below 0.5W for the > > > whole system while allowing Wake-on-WLAN, GPIO, RTC, infrared, etc.) > > > > > > > I fail to understand how that is a problem from S2I. It is probably worth > > checking if there are any unnecessary IRQF_NO_SUSPEND users. Check section > > IRQF_NO_SUSPEND and enable_irq_wake() in [1]. I don't see any issues other > > wise in terms of unnecessary/spurious wakeup by in-band(to be precise > > no-wake up) interrupts. > > I don't think your hyperlink referenced by [1] was provided, but my quick > testing with: > Yikes, I meant to refer Documentation/power/suspend-and-interrupts.rst > echo s2idle > /sys/power/mem_sleep > echo mem > /sys/power/state > > appears to work to some extent when I use peripherals that can generate > in-band interrupts. > > It looks like we have s2idle_ops that allows a platform to override some of > the operations before/after entering s2idle, however the actual s2idle idle > loop is still within the kernel, so we will not call into the ARM Trusted > Firmware and engage the power management state machine. > Correct. > This means that there will not be any of the clock gating that only the > hardware state machine is capable of performing, the DRAM controller as a > result will not enter self refresh power down, and in addition the side band > wake-up interrupts will not be activate because the interrupt controller > that aggregates them only outputs to the ARM GIC when the state machine has > been engaged. > One possible solution IIUC the issue is to add this as additional CPU Idle state disabled most of the time. Enable them from user-space just prior to calling freeze/s2idle, so that PSCI CPU_SUSPEND is called with right param to indicate this is deepest idle state(in your case just WFI) + DRAM self refresh/retention mode. Also TF-A can take care to enable the side band interrupts before entering the state. Do you see any issue with this approach ? I am trying to find ways to avoid deviating from standard PSCI. > Essentially, what we need for our systems is a controlled system entry with > semantics similar if not identical to that of S2R but with a shallower state > that does not cut the power to 90% of the SoC (unlike S2R) such that we have > a quicker suspend and resume latency. Years ago when we only had MIPS-based > and 32-bit ARM SoCs, we did come up with using "standby" (see > drivers/soc/bcm/brcmstb/pm/*) and we naturally mapped that when we switched > over to ARMv8 capable devices. > Understood and I do remember that. -- Regards, Sudeep _______________________________________________ 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:[~2022-02-07 16:33 UTC|newest] Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-01-22 3:54 [PATCH 0/4] Broadcom STB PM PSCI extensions Florian Fainelli 2022-01-22 3:54 ` Florian Fainelli 2022-01-22 3:54 ` [PATCH 1/4] firmware: psci: Export a couple of suspend symbols Florian Fainelli 2022-01-22 3:54 ` Florian Fainelli 2022-01-22 12:22 ` kernel test robot 2022-01-22 12:22 ` kernel test robot 2022-01-22 3:54 ` [PATCH 2/4] soc: bcm: brcmstb: Make legacy PM code depend on !ARM_PSCI_FW Florian Fainelli 2022-01-22 3:54 ` Florian Fainelli 2022-01-22 3:54 ` [PATCH 3/4] soc: bcm: brcmstb: Added support for PSCI system suspend operations Florian Fainelli 2022-01-22 3:54 ` Florian Fainelli 2022-01-22 6:09 ` kernel test robot 2022-01-22 6:09 ` kernel test robot 2022-01-22 7:10 ` kernel test robot 2022-01-22 7:10 ` kernel test robot 2022-02-03 12:09 ` Mark Rutland 2022-02-03 12:09 ` Mark Rutland 2022-02-03 18:45 ` Florian Fainelli 2022-02-03 18:45 ` Florian Fainelli 2022-01-22 3:54 ` [PATCH 4/4] Documentation: ABI: Document Broadcom STB PSCI firmware files Florian Fainelli 2022-01-22 3:54 ` Florian Fainelli 2022-01-27 3:55 ` [PATCH 0/4] Broadcom STB PM PSCI extensions Florian Fainelli 2022-01-27 3:55 ` Florian Fainelli 2022-02-03 10:47 ` Mark Rutland 2022-02-03 10:47 ` Mark Rutland 2022-02-03 18:32 ` Florian Fainelli 2022-02-03 18:32 ` Florian Fainelli 2022-02-03 11:14 ` Sudeep Holla 2022-02-03 11:14 ` Sudeep Holla 2022-02-03 17:36 ` Florian Fainelli 2022-02-03 17:36 ` Florian Fainelli 2022-02-03 18:52 ` Sudeep Holla 2022-02-03 18:52 ` Sudeep Holla 2022-02-03 19:33 ` Florian Fainelli 2022-02-03 19:33 ` Florian Fainelli 2022-02-07 16:27 ` Sudeep Holla [this message] 2022-02-07 16:27 ` Sudeep Holla 2022-02-14 18:12 ` Florian Fainelli 2022-02-14 18:12 ` Florian Fainelli
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=20220207162734.nt55nflwqstkej3p@bogus \ --to=sudeep.holla@arm.com \ --cc=bcm-kernel-feedback-list@broadcom.com \ --cc=f.fainelli@gmail.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=lorenzo.pieralisi@arm.com \ --cc=mark.rutland@arm.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.