On 6/14/2021 6:19 AM, Ulf Hansson wrote: > On Fri, 11 Jun 2021 at 18:54, Florian Fainelli <f.fainelli@xxxxxxxxx> wrote: >> >> >> >> On 6/11/2021 3:23 AM, Ulf Hansson wrote: >>> On Thu, 10 Jun 2021 at 17:59, Florian Fainelli <f.fainelli@xxxxxxxxx> wrote: >>>> >>>> >>>> >>>> On 6/10/2021 1:49 AM, Ulf Hansson wrote: >>>>> On Thu, 10 Jun 2021 at 01:59, Florian Fainelli <f.fainelli@xxxxxxxxx> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 6/9/2021 2:22 AM, Ulf Hansson wrote: >>>>>>> On Wed, 9 Jun 2021 at 05:07, Florian Fainelli <f.fainelli@xxxxxxxxx> wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On 6/8/2021 5:40 AM, Ulf Hansson wrote: >>>>>>>>> On Wed, 2 Jun 2021 at 21:28, Al Cooper <alcooperx@xxxxxxxxx> wrote: >>>>>>>>>> >>>>>>>>>> Add support for the legacy Arasan sdhci controller on the BCM7211 and >>>>>>>>>> related SoC's. This includes adding a .shutdown callback to increase >>>>>>>>>> the power savings during S5. >>>>>>>>> >>>>>>>>> Please split this into two separate changes. >>>>>>>>> >>>>>>>>> May I also ask about the ->shutdown() callback and in relation to S5. >>>>>>>>> What makes the ->shutdown callback only being invoked for S5? >>>>>>>> >>>>>>>> It is not only called for S5 (entered via poweroff on a prompt) but also >>>>>>>> during kexec or reboot. The poweroff path is via: >>>>>>>> >>>>>>>> kernel_power_off() -> kernel_shutdown_prepare() -> device_shutdown() -> >>>>>>>> .shutdown() >>>>>>>> >>>>>>>> For kexec or reboot we do not really care about power savings since we >>>>>>>> are about to load a new image anyway, however for S5/poweroff we do care >>>>>>>> about quiescing the eMMC controller in a way that its clocks and the >>>>>>>> eMMC device can be put into low power mode since we will stay in that >>>>>>>> mode for seconds/hours/days until someone presses a button on their >>>>>>>> remote (or other wake-up sources). >>>>>>> >>>>>>> Hmm, I am not sure I understand correctly. At shutdown we don't care >>>>>>> about wake-up sources from the kernel point of view, instead we treat >>>>>>> everything as if it will be powered off. >>>>>> >>>>>> The same .shutdown() path is used whether you kexec, reboot or poweroff, >>>>>> but for poweroff we do care about allowing specific wake-up sources >>>>>> configured as such to wake-up the system at a later time, like GPIOs, >>>>>> RTC, etc. >>>>> >>>>> That's true, but using the ->shutdown() callbacks in this way would >>>>> certainly be a new use case. >>>>> >>>>> Most subsystems/drivers don't care about power management in those >>>>> callbacks, but rather just about managing a graceful shutdown. >>>>> >>>>> It sounds to me like you should have a look at the hibernation >>>>> path/callbacks instead - or perhaps even the system suspend >>>>> path/callback. Normally, that's where we care about power management. >>>> >>>> The platforms we use do not support hibernation, keep in mind that these >>>> are embedded SoCs that support the S2 (standby), S3 (mem) and poweroff >>>> suspend states, hibernation is not something that we can support. >>>> >>>>> >>>>> I have looped in Rafael, to allow him to share his opinion on this. >>>>> >>>>>> >>>>>>> >>>>>>> We put devices into low power state at system suspend and potentially >>>>>>> also during some of the hibernation phases. >>>>>>> >>>>>>> Graceful shutdown of the eMMC is also managed by the mmc core. >>>>>> >>>>>> AFAICT that calls mmc_blk_shutdown() but that is pretty much it, the >>>>>> SDHCI platform_driver still needs to do something in order to conserve >>>>>> power including disabling host->clk, otherwise we would not have done >>>>>> that for sdhci-brcmstb.c. >>>>> >>>>> That's not entirely correct. When mmc_bus_shutdown() is called for the >>>>> struct device* that belongs to an eMMC card, two actions are taken. >>>>> >>>>> *) We call mmc_blk_shutdown(), to suspend the block device queue from >>>>> receiving new I/O requests. >>>>> **) We call host->bus_ops->shutdown(), which is an eMMC specific >>>>> callback set to mmc_shutdown(). In this step, we do a graceful >>>>> shutdown/power-off of the eMMC card. >>>>> >>>>> When it comes to controller specific resources, like clocks and PM >>>>> domains, for example, those may very well stay turned on. Do deal with >>>>> these, then yes, you would need to implement the ->shutdown() >>>>> callback. But as I said above, I am not sure it's the right thing to >>>>> do. >>>> >>>> As explained before, we can enter S5 for an indefinite amount of time >>>> until a wake-up source wakes us up so we must conserve power, even if we >>>> happen to wake up the next second, we don't know that ahead of time. The >>>> point of calling sdhci_pltfm_suspend() here is to ensure that host->clk >>>> is turned off which cuts the eMMC controller digital clock, I forgot how >>>> much power we save by doing so, but every 10s of mW counts for us. >>> >>> I fully understand that you want to avoid draining energy, every >>> single uA certainly counts in cases like these. >>> >>> What puzzles me, is that your platform seems to keep some resources >>> powered on (like device clocks) when entering the system wide low >>> power state, S5. >> >> More on that below. >> >>> >>> In principle, I am wondering if it would be possible to use S5 as the >>> system-wide low power state for the system suspend path, rather than >>> S3, for example? In this way, we would be able to re-use already >>> implemented ->suspend|resume callbacks from most subsystems/drivers, I >>> believe. Or is there a problem with that? >> >> The specific platform this driver is used on (BCM7211) is only capable >> of supporting S2 and S5. There is no S3 because we have no provision on >> the board to maintain the DRAM supplies on and preserve the DRAM >> contents. This is a design choice that is different from the other >> Broadcom STB platforms where we offer S2, S3 and S5 and we have an >> On/off domain which is shutdown by hardware upon S3 or S5 entry and a >> small always on domain which remains on to service wake-up sources >> (infrared, timer, gpio, UART, etc.). S2 on this platform is implemented >> entirely in software/firmware and does make use of the regular >> suspend/resume calls. >> >> S5 is implemented in part in software/firmware and with the help of the >> hardware that will turn off external board components. We do need the >> help of the various software drivers (PCIe, Ethernet, GPIO, USB, UART, >> RTC, eMMC, SPI, etc.) to do their job and conserve power when we enter >> S5, hence the reason why all of our drivers implement ->shutdown() (in >> addition to needing that for kexec and ensure no DMA is left running). >> >>> >>> I think we need an opinion from Rafel to move forward. >> >> There is already an identical change done for sdhci-brcmstb.c, and the >> exact same rationale applied there since both sdhci-iproc.c and >> sdhci-brcmstb.c are used on this BCM7211 platform. > > Right, thanks for the pointer. Looks like we should have taken this > discussion back then, but better late than never. > >> >> 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. -- Florian
