From: Florian Fainelli <f.fainelli@gmail.com> To: Ulf Hansson <ulf.hansson@linaro.org> Cc: Linux PM <linux-pm@vger.kernel.org>, "Rafael J. Wysocki" <rjw@rjwysocki.net>, Al Cooper <alcooperx@gmail.com>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Adrian Hunter <adrian.hunter@intel.com>, BCM Kernel Feedback <bcm-kernel-feedback-list@broadcom.com>, DTML <devicetree@vger.kernel.org>, Linux ARM <linux-arm-kernel@lists.infradead.org>, linux-mmc <linux-mmc@vger.kernel.org>, Nicolas Saenz Julienne <nsaenz@kernel.org>, Ray Jui <rjui@broadcom.com>, Rob Herring <robh+dt@kernel.org>, Scott Branden <sbranden@broadcom.com> Subject: Re: [PATCH 2/2] mmc: sdhci-iproc: Add support for the legacy sdhci controller on the BCM7211 Date: Tue, 15 Jun 2021 08:51:26 -0700 [thread overview] Message-ID: <c356986f-08de-e8d5-5d1e-f4e13c77648f@gmail.com> (raw) In-Reply-To: <CAPDyKFq92mp4CXj8-QHw=DEQ8bcAjtrmLyowrGKSJL2Fch1cJQ@mail.gmail.com> On 6/15/2021 8:30 AM, Ulf Hansson wrote: > [...] > >>> >>>> >>>> In all honesty, I am a bit surprised that the Linux device driver model >>>> does not try to default the absence of a ->shutdown() to a ->suspend() >>>> call since in most cases they are functionally equivalent, or should be, >>>> in that they need to save power and quiesce the hardware, or leave >>>> enough running to support a wake-up event. >>> >>> Well, the generall assumption is that the platform is going to be >>> entirely powered off, thus moving things into a low power state would >>> just be a waste of execution cycles. Of course, that's not the case >>> for your platform. >> >> That assumption may hold true for ACPI-enabled machines but power off is >> offered as a general function towards other more flexible and snowflaky >> systems (read embedded) as well. >> >>> >>> As I have stated earlier, to me it looks a bit questionable to use the >>> kernel_power_off() path to support the use case you describe. On the >>> other hand, we may not have a better option at this point. >> >> Correct, there is not really anything better and I am not sure what the >> semantics of something better could be anyway. >> >>> >>> Just a few things, from the top of my head, that we certainly are >>> missing to support your use case through kernel_power_off() path >>> (there are certainly more): >>> 1. In general, subsystems/drivers don't care about moving things into >>> lower power modes from their ->shutdown() callbacks. >>> 2. System wakeups and devices being affected in the wakeup path, needs >>> to be respected properly. Additionally, userspace should be able to >>> decide if system wakeups should be enabled or not. >>> 3. PM domains don't have ->shutdown() callbacks, thus it's likely that >>> they remain powered on. >>> 4. Etc... >> >> For the particular eMMC driver being discussed here this is a no-brainer > > because it is not a wake-up source, therefore there is no reason not to >> power if off if we can. It also seems proper to have it done by the >> kernel as opposed to firmware. > > Okay, I have applied the $subject patch onto my next branch, along > with patch 1/2 (the DT doc change). > > However, I still think we should look for a proper long term solution, > because the kernel_power_off() path does not currently support your > use case, with system wakeups etc. Not really, it does work fine, some drivers like gpio-keys.c or gpio-brcmstb.c will ensure that the GPIOs that are enabled as wake-up interrupts are configured that way during kernel_power_off() and the various interrupt controllers like irq-brcmstb-l2.c will make sure they don't mask wake-up interrupts. > > I guess it could be a topic that is easier to bring up at the Linux > Plumbers Conf, for example. OK, not sure if I will be able to attend, but would definitively try to. -- Florian
WARNING: multiple messages have this Message-ID (diff)
From: Florian Fainelli <f.fainelli@gmail.com> To: Ulf Hansson <ulf.hansson@linaro.org> Cc: Linux PM <linux-pm@vger.kernel.org>, "Rafael J. Wysocki" <rjw@rjwysocki.net>, Al Cooper <alcooperx@gmail.com>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Adrian Hunter <adrian.hunter@intel.com>, BCM Kernel Feedback <bcm-kernel-feedback-list@broadcom.com>, DTML <devicetree@vger.kernel.org>, Linux ARM <linux-arm-kernel@lists.infradead.org>, linux-mmc <linux-mmc@vger.kernel.org>, Nicolas Saenz Julienne <nsaenz@kernel.org>, Ray Jui <rjui@broadcom.com>, Rob Herring <robh+dt@kernel.org>, Scott Branden <sbranden@broadcom.com> Subject: Re: [PATCH 2/2] mmc: sdhci-iproc: Add support for the legacy sdhci controller on the BCM7211 Date: Tue, 15 Jun 2021 08:51:26 -0700 [thread overview] Message-ID: <c356986f-08de-e8d5-5d1e-f4e13c77648f@gmail.com> (raw) In-Reply-To: <CAPDyKFq92mp4CXj8-QHw=DEQ8bcAjtrmLyowrGKSJL2Fch1cJQ@mail.gmail.com> On 6/15/2021 8:30 AM, Ulf Hansson wrote: > [...] > >>> >>>> >>>> In all honesty, I am a bit surprised that the Linux device driver model >>>> does not try to default the absence of a ->shutdown() to a ->suspend() >>>> call since in most cases they are functionally equivalent, or should be, >>>> in that they need to save power and quiesce the hardware, or leave >>>> enough running to support a wake-up event. >>> >>> Well, the generall assumption is that the platform is going to be >>> entirely powered off, thus moving things into a low power state would >>> just be a waste of execution cycles. Of course, that's not the case >>> for your platform. >> >> That assumption may hold true for ACPI-enabled machines but power off is >> offered as a general function towards other more flexible and snowflaky >> systems (read embedded) as well. >> >>> >>> As I have stated earlier, to me it looks a bit questionable to use the >>> kernel_power_off() path to support the use case you describe. On the >>> other hand, we may not have a better option at this point. >> >> Correct, there is not really anything better and I am not sure what the >> semantics of something better could be anyway. >> >>> >>> Just a few things, from the top of my head, that we certainly are >>> missing to support your use case through kernel_power_off() path >>> (there are certainly more): >>> 1. In general, subsystems/drivers don't care about moving things into >>> lower power modes from their ->shutdown() callbacks. >>> 2. System wakeups and devices being affected in the wakeup path, needs >>> to be respected properly. Additionally, userspace should be able to >>> decide if system wakeups should be enabled or not. >>> 3. PM domains don't have ->shutdown() callbacks, thus it's likely that >>> they remain powered on. >>> 4. Etc... >> >> For the particular eMMC driver being discussed here this is a no-brainer > > because it is not a wake-up source, therefore there is no reason not to >> power if off if we can. It also seems proper to have it done by the >> kernel as opposed to firmware. > > Okay, I have applied the $subject patch onto my next branch, along > with patch 1/2 (the DT doc change). > > However, I still think we should look for a proper long term solution, > because the kernel_power_off() path does not currently support your > use case, with system wakeups etc. Not really, it does work fine, some drivers like gpio-keys.c or gpio-brcmstb.c will ensure that the GPIOs that are enabled as wake-up interrupts are configured that way during kernel_power_off() and the various interrupt controllers like irq-brcmstb-l2.c will make sure they don't mask wake-up interrupts. > > I guess it could be a topic that is easier to bring up at the Linux > Plumbers Conf, for example. OK, not sure if I will be able to attend, but would definitively try to. -- 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:[~2021-06-15 15:58 UTC|newest] Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-06-02 19:27 [PATCH 1/2] dt-bindings: mmc: sdhci-iproc: Add brcm,bcm7211a0-sdhci Al Cooper 2021-06-02 19:27 ` Al Cooper 2021-06-02 19:27 ` [PATCH 2/2] mmc: sdhci-iproc: Add support for the legacy sdhci controller on the BCM7211 Al Cooper 2021-06-02 19:27 ` Al Cooper 2021-06-08 12:40 ` Ulf Hansson 2021-06-08 12:40 ` Ulf Hansson 2021-06-09 3:07 ` Florian Fainelli 2021-06-09 3:07 ` Florian Fainelli 2021-06-09 9:22 ` Ulf Hansson 2021-06-09 9:22 ` Ulf Hansson 2021-06-09 23:59 ` Florian Fainelli 2021-06-09 23:59 ` Florian Fainelli 2021-06-10 8:49 ` Ulf Hansson 2021-06-10 8:49 ` Ulf Hansson 2021-06-10 15:59 ` Florian Fainelli 2021-06-10 15:59 ` Florian Fainelli 2021-06-11 10:23 ` Ulf Hansson 2021-06-11 10:23 ` Ulf Hansson 2021-06-11 16:54 ` Florian Fainelli 2021-06-11 16:54 ` Florian Fainelli 2021-06-14 13:19 ` Ulf Hansson 2021-06-14 13:19 ` Ulf Hansson 2021-06-14 19:29 ` Florian Fainelli 2021-06-14 19:29 ` Florian Fainelli 2021-06-15 15:30 ` Ulf Hansson 2021-06-15 15:30 ` Ulf Hansson 2021-06-15 15:51 ` Florian Fainelli [this message] 2021-06-15 15:51 ` Florian Fainelli 2021-06-15 23:46 ` [PATCH 1/2] dt-bindings: mmc: sdhci-iproc: Add brcm,bcm7211a0-sdhci Rob Herring 2021-06-15 23:46 ` Rob Herring
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=c356986f-08de-e8d5-5d1e-f4e13c77648f@gmail.com \ --to=f.fainelli@gmail.com \ --cc=adrian.hunter@intel.com \ --cc=alcooperx@gmail.com \ --cc=bcm-kernel-feedback-list@broadcom.com \ --cc=devicetree@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mmc@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=nsaenz@kernel.org \ --cc=rjui@broadcom.com \ --cc=rjw@rjwysocki.net \ --cc=robh+dt@kernel.org \ --cc=sbranden@broadcom.com \ --cc=ulf.hansson@linaro.org \ /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.