From: Florian Fainelli <f.fainelli@gmail.com> To: Mark Rutland <mark.rutland@arm.com>, Florian Fainelli <f.fainelli@gmail.com> Cc: linux-arm-kernel@lists.infradead.org, "maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE" <bcm-kernel-feedback-list@broadcom.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: Thu, 3 Feb 2022 10:32:37 -0800 [thread overview] Message-ID: <f412df05-ffb6-51b6-6b5e-b25d663bd5e9@gmail.com> (raw) In-Reply-To: <YfuysSBnZbnMF4OC@FVFF77S0Q05N> Hello Mark, On 2/3/2022 2:47 AM, Mark Rutland wrote: > On Fri, Jan 21, 2022 at 07:54:17PM -0800, Florian Fainelli wrote: >> Hi all, > > Hi Florian, > >> 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. > > I *really* don't like the idea of having non-standard PSCI extensions, because > it somewhat defeats the point of PSCI being a standard, and opens the door for > the zoo of mechanisms we had on 32-bit. > > I think this needs a fair amount more explanation and justification. > >> 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". > > Why isn't a combination of CPU_SUSPEND and SYSTEM_SUSPEND sufficient here? This is exactly what we are using, just the use of CPU_SUSPEND is not done via the CPU IDLE framework because our platforms did not wire up the ARM GIC power controller interrupt signals back to the power management controller of the system, but via registering a "standby" state into suspend_ops instead. > > What specifically *can't* you do with standard PSCI calls? Since you looked at the patches now, nothing at all, everything we do (with the exception of the funky SIP calls which are not strictly mandatory for system suspend operations) is done by using standard PSCI calls and leveraging the existing vendor space when needed (as with SYSTEM_RESET2 for instance). > >> These platforms also may have an external Broadcom PMIC chip which can >> cause the SoC to be powercycled assuming that we communicate that intent >> via a vendor specific PSCI SYSTEM_RESET2. >> >> Since it is desirable to get any new functionality added to the kernel >> to be loadable as a module as part of shipping said products in a Google >> Kernel Image (GKI) environment, we need to export a couple of symbols from >> drivers/firmware/psci/psci.c. > > I really don't want to export the guts of psci.c. I can appreciate that, and really the sticking point that required me to export the couple of symbols needed was because the alternatives would be to: - to not make this code modular in the first place but that won't fly in the Google Kernel Image grand scheme of things where *everything* that is not necessary for boot must be a loadable module - not support the "standby" mode which is not really an option since we rely on it to achieve our power targets - export cpu_suspend from arch/*/kernel/suspend.c which is probably going to be a no-go plus duplicate the entire set of PSCI function calls to re-implement the psci_system_suspend_enter() functions Thanks for taking a look! -- Florian
WARNING: multiple messages have this Message-ID (diff)
From: Florian Fainelli <f.fainelli@gmail.com> To: Mark Rutland <mark.rutland@arm.com>, Florian Fainelli <f.fainelli@gmail.com> Cc: linux-arm-kernel@lists.infradead.org, "maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE" <bcm-kernel-feedback-list@broadcom.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: Thu, 3 Feb 2022 10:32:37 -0800 [thread overview] Message-ID: <f412df05-ffb6-51b6-6b5e-b25d663bd5e9@gmail.com> (raw) In-Reply-To: <YfuysSBnZbnMF4OC@FVFF77S0Q05N> Hello Mark, On 2/3/2022 2:47 AM, Mark Rutland wrote: > On Fri, Jan 21, 2022 at 07:54:17PM -0800, Florian Fainelli wrote: >> Hi all, > > Hi Florian, > >> 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. > > I *really* don't like the idea of having non-standard PSCI extensions, because > it somewhat defeats the point of PSCI being a standard, and opens the door for > the zoo of mechanisms we had on 32-bit. > > I think this needs a fair amount more explanation and justification. > >> 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". > > Why isn't a combination of CPU_SUSPEND and SYSTEM_SUSPEND sufficient here? This is exactly what we are using, just the use of CPU_SUSPEND is not done via the CPU IDLE framework because our platforms did not wire up the ARM GIC power controller interrupt signals back to the power management controller of the system, but via registering a "standby" state into suspend_ops instead. > > What specifically *can't* you do with standard PSCI calls? Since you looked at the patches now, nothing at all, everything we do (with the exception of the funky SIP calls which are not strictly mandatory for system suspend operations) is done by using standard PSCI calls and leveraging the existing vendor space when needed (as with SYSTEM_RESET2 for instance). > >> These platforms also may have an external Broadcom PMIC chip which can >> cause the SoC to be powercycled assuming that we communicate that intent >> via a vendor specific PSCI SYSTEM_RESET2. >> >> Since it is desirable to get any new functionality added to the kernel >> to be loadable as a module as part of shipping said products in a Google >> Kernel Image (GKI) environment, we need to export a couple of symbols from >> drivers/firmware/psci/psci.c. > > I really don't want to export the guts of psci.c. I can appreciate that, and really the sticking point that required me to export the couple of symbols needed was because the alternatives would be to: - to not make this code modular in the first place but that won't fly in the Google Kernel Image grand scheme of things where *everything* that is not necessary for boot must be a loadable module - not support the "standby" mode which is not really an option since we rely on it to achieve our power targets - export cpu_suspend from arch/*/kernel/suspend.c which is probably going to be a no-go plus duplicate the entire set of PSCI function calls to re-implement the psci_system_suspend_enter() functions Thanks for taking a look! -- Florian _______________________________________________ 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-03 18:32 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 [this message] 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 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=f412df05-ffb6-51b6-6b5e-b25d663bd5e9@gmail.com \ --to=f.fainelli@gmail.com \ --cc=bcm-kernel-feedback-list@broadcom.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.