archive mirror
 help / color / mirror / Atom feed
From: Bjorn Helgaas <>
To: Florian Fainelli <>
Cc: Jim Quinlan <>,, Bjorn Helgaas <>,
	Nicolas Saenz Julienne <>,,,
	Nicolas Saenz Julienne <>,
	Lorenzo Pieralisi <>,
	Rob Herring <>,
	"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" 
	"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" 
	open list <>
Subject: Re: [PATCH v1 4/4] PCI: brcmstb: add shutdown call to driver
Date: Thu, 3 Jun 2021 12:23:13 -0500	[thread overview]
Message-ID: <20210603172313.GA2123252@bjorn-Precision-5520> (raw)
In-Reply-To: <>

On Wed, May 26, 2021 at 10:03:47AM -0700, Florian Fainelli wrote:
> On 5/25/21 2:18 PM, Bjorn Helgaas wrote:
> > On Tue, Apr 27, 2021 at 01:51:39PM -0400, Jim Quinlan wrote:
> >> The shutdown() call is similar to the remove() call except the former does
> >> not need to invoke pci_{stop,remove}_root_bus(), and besides, errors occur
> >> if it does.
> > 
> > This doesn't explain why shutdown() is necessary.  "errors occur"
> > might be a hint, except that AFAICT, many similar drivers do invoke
> > pci_stop_root_bus() and pci_remove_root_bus() (several of them while
> > holding pci_lock_rescan_remove()), without implementing .shutdown().
> We have to implement .shutdown() in order to meet a certain power budget
> while the chip is being put into S5 (soft off) state and still support
> Wake-on-WLAN, for our latest chips this translates into roughly 200mW of
> power savings at the wall. We could probably add a word or two in a v2
> that indicates this is done for power savings.

"Saving power" is a great reason to do this.  But we still need to
connect this to the driver model and the system-level behavior

The pci_driver comment says @shutdown is to "stop idling DMA
operations" and it hooks into reboot_notifier_list in kernel/sys.c.
That's incorrect or at least incomplete because reboot_notifier_list
isn't mentioned at all in kernel/sys.c, and I don't see the connection
between @shutdown and reboot_notifier_list.

AFAICT, @shutdown is currently used in this path:

  kernel_restart_prepare or kernel_shutdown_prepare
        pci_device_shutdown                     # pci_bus_type.shutdown

so we're going to either reboot or halt/power-off the entire system,
and we're not going to use this device again until we're in a
brand-new kernel and we re-enumerate the device and re-register the

I'm not quite sure how either of those fits into the power-saving
reason.  I guess going to S5 is probably via the kernel_power_off()
path and that by itself doesn't turn off as much power to the PCIe
controller as it could?  And this new .shutdown() method will get
called in that path and will turn off more power, but will still leave
enough for wake-on-LAN to work?  And when we *do* wake from S5,
obviously that means a complete boot with a new kernel.


  reply	other threads:[~2021-06-03 17:23 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-27 17:51 [PATCH v1 0/4] PCI: brcmstb: Add panic handler and shutdown func Jim Quinlan
2021-04-27 17:51 ` [PATCH v1 1/4] PCI: brcmstb: Check return value of clk_prepare_enable() Jim Quinlan
2021-04-27 17:51 ` [PATCH v1 2/4] PCI: brcmstb: Give 7216 SOCs their own config type Jim Quinlan
2021-04-27 17:51 ` [PATCH v1 3/4] PCI: brcmstb: Add panic/die handler to RC driver Jim Quinlan
2021-05-25 20:40   ` Bjorn Helgaas
2021-05-25 21:05     ` Jim Quinlan
2021-05-25 21:17       ` Bjorn Helgaas
2021-04-27 17:51 ` [PATCH v1 4/4] PCI: brcmstb: add shutdown call to driver Jim Quinlan
2021-04-27 19:35   ` Florian Fainelli
2021-05-25 21:18   ` Bjorn Helgaas
2021-05-25 21:40     ` Jim Quinlan
2021-05-26 16:11     ` Rob Herring
2021-05-26 17:03     ` Florian Fainelli
2021-06-03 17:23       ` Bjorn Helgaas [this message]
2021-06-03 17:30         ` Florian Fainelli
2021-06-03 20:58           ` Bjorn Helgaas
2021-06-03 21:01             ` Florian Fainelli
2021-05-25 18:01 ` [PATCH v1 0/4] PCI: brcmstb: Add panic handler and shutdown func Florian Fainelli
2021-06-03 16:32 ` Lorenzo Pieralisi
2021-06-03 17:31   ` Jim Quinlan
2021-06-04  9:22     ` Lorenzo Pieralisi

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:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210603172313.GA2123252@bjorn-Precision-5520 \ \ \ \ \ \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).