diff mbox series

[v1,4/4] PCI: brcmstb: add shutdown call to driver

Message ID 20210427175140.17800-5-jim2101024@gmail.com (mailing list archive)
State New, archived
Headers show
Series PCI: brcmstb: Add panic handler and shutdown func | expand

Commit Message

Jim Quinlan April 27, 2021, 5:51 p.m. UTC
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.

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Florian Fainelli April 27, 2021, 7:35 p.m. UTC | #1
On 4/27/2021 10:51 AM, 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.
> 
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>
Bjorn Helgaas May 25, 2021, 9:18 p.m. UTC | #2
Capitalize "Add" in the subject.

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().

It is ... unfortunate that there's such a variety of implementations
here.  I don't believe these driver differences are all necessary
consequences of hardware differences.

> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index d3af8d84f0d6..a1fe1a2ada48 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -1340,6 +1340,15 @@ static int brcm_pcie_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static void brcm_pcie_shutdown(struct platform_device *pdev)
> +{
> +	struct brcm_pcie *pcie = platform_get_drvdata(pdev);
> +
> +	if (pcie->has_err_report)
> +		brcm_unregister_die_notifiers(pcie);
> +	__brcm_pcie_remove(pcie);
> +}
> +
>  static const struct of_device_id brcm_pcie_match[] = {
>  	{ .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
>  	{ .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_cfg },
> @@ -1460,6 +1469,7 @@ static const struct dev_pm_ops brcm_pcie_pm_ops = {
>  static struct platform_driver brcm_pcie_driver = {
>  	.probe = brcm_pcie_probe,
>  	.remove = brcm_pcie_remove,
> +	.shutdown = brcm_pcie_shutdown,
>  	.driver = {
>  		.name = "brcm-pcie",
>  		.of_match_table = brcm_pcie_match,
> -- 
> 2.17.1
>
Jim Quinlan May 25, 2021, 9:40 p.m. UTC | #3
On Tue, May 25, 2021 at 5:18 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> Capitalize "Add" in the subject.
>
> 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().
>
> It is ... unfortunate that there's such a variety of implementations
> here.  I don't believe these driver differences are all necessary
> consequences of hardware differences.

Fair enough, I'll dig into the error.

Regards,
Jim Quinlan
Broadcom STB
>
> > Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index d3af8d84f0d6..a1fe1a2ada48 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -1340,6 +1340,15 @@ static int brcm_pcie_remove(struct platform_device *pdev)
> >       return 0;
> >  }
> >
> > +static void brcm_pcie_shutdown(struct platform_device *pdev)
> > +{
> > +     struct brcm_pcie *pcie = platform_get_drvdata(pdev);
> > +
> > +     if (pcie->has_err_report)
> > +             brcm_unregister_die_notifiers(pcie);
> > +     __brcm_pcie_remove(pcie);
> > +}
> > +
> >  static const struct of_device_id brcm_pcie_match[] = {
> >       { .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
> >       { .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_cfg },
> > @@ -1460,6 +1469,7 @@ static const struct dev_pm_ops brcm_pcie_pm_ops = {
> >  static struct platform_driver brcm_pcie_driver = {
> >       .probe = brcm_pcie_probe,
> >       .remove = brcm_pcie_remove,
> > +     .shutdown = brcm_pcie_shutdown,
> >       .driver = {
> >               .name = "brcm-pcie",
> >               .of_match_table = brcm_pcie_match,
> > --
> > 2.17.1
> >
Rob Herring May 26, 2021, 4:11 p.m. UTC | #4
On Tue, May 25, 2021 at 4:18 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> Capitalize "Add" in the subject.
>
> 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().
>
> It is ... unfortunate that there's such a variety of implementations
> here.  I don't believe these driver differences are all necessary
> consequences of hardware differences.

What's correct here?

This was on my radar and something we should get in front of. It's
only a matter of time until everyone is converting their drivers to
modules since that is now the Android WayTM. My plan here was to
register a devm hook to call pci_stop_root_bus() and
pci_remove_root_bus(). That way every driver doesn't have to implement
a remove() hook. Though maybe they all need to quiesce themselves
anyways.

Rob
Florian Fainelli May 26, 2021, 5:03 p.m. UTC | #5
On 5/25/21 2:18 PM, Bjorn Helgaas wrote:
> Capitalize "Add" in the subject.
> 
> 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.

As far as the crash goes, if we call brcm_pcie_remove() directly from
brcm_pcie_shutdown() we see the following (this is in 5.4, but I have no
reason to believe mainline is different):

# lspci
0001:00:00.0 PCI bridge: Broadcom Inc. and subsidiaries Device 7216 (rev 10)
0001:01:00.0 Network controller: Qualcomm Atheros AR9285 Wireless
Network Adapter (PCI-Express) (rev 01)
# reboot -f
[    8.465111] ------------[ cut here ]------------
[    8.469771] pcieport 0001:00:00.0: disabling already-disabled device
[    8.469817] WARNING: CPU: 3 PID: 1656 at drivers/pci/pci.c:1945
pci_disable_device+0x78/0xd0
[    8.484631] Modules linked in:
[    8.487695] CPU: 3 PID: 1656 Comm: reboot Not tainted
5.4.120-1.1pre-g231f2cfa989a #4
[    8.495537] Hardware name: BCX972160DV (DT)
[    8.499728] pstate: 60000005 (nZCv daif -PAN -UAO)
[    8.504527] pc : pci_disable_device+0x78/0xd0
[    8.508891] lr : pci_disable_device+0x78/0xd0
[    8.513254] sp : ffffffc014d33aa0
[    8.516572] x29: ffffffc014d33aa0 x28: ffffff809deac800
[    8.521894] x27: ffffff809e23a5b0 x26: ffffff809e23a490
[    8.527214] x25: 0000000000000000 x24: ffffff809cf9f000
[    8.532535] x23: ffffffc011d13090 x22: 0000000000000000
[    8.537856] x21: 0000000000000000 x20: ffffff809cf9f0b0
[    8.543175] x19: ffffff809cf9f000 x18: 000000000000000a
[    8.548495] x17: 0000000000000000 x16: 0000000000000000
[    8.553816] x15: 00000000000718d7 x14: 0720072007200720
[    8.559138] x13: 0720072007200720 x12: 0720072007200720
[    8.564458] x11: 0720072007200720 x10: 0720072007200720
[    8.569777] x9 : 0720072007200720 x8 : 656c62617369642d
[    8.575098] x7 : 79646165726c6120 x6 : ffffffc011dc3630
[    8.580417] x5 : 0000000000000038 x4 : 0000000000000000
[    8.585737] x3 : 0000000000000000 x2 : 0000000000000000
[    8.591058] x1 : 73b2329426297100 x0 : 0000000000000000
[    8.596379] Call trace:
[    8.598832]  pci_disable_device+0x78/0xd0
[    8.602852]  pcie_port_device_remove+0x3c/0x48
[    8.607304]  pcie_portdrv_remove+0x5c/0x8c
[    8.611408]  pci_device_remove+0x98/0xe8
[    8.615341]  device_release_driver_internal+0xb0/0x188
[    8.620487]  device_release_driver+0x28/0x34
[    8.624765]  pci_stop_bus_device+0x40/0xa8
[    8.628867]  pci_stop_root_bus+0x58/0x64
[    8.632797]  brcm_pcie_remove+0x24/0x70
[    8.636640]  brcm_pcie_shutdown+0x20/0x2c
[    8.640655]  platform_drv_shutdown+0x2c/0x38
[    8.644934]  device_shutdown+0x16c/0x1e0
[    8.648865]  kernel_restart_prepare+0x44/0x50
[    8.653229]  kernel_restart+0x20/0x68
[    8.656896]  __do_sys_reboot+0x158/0x200
[    8.660816]  __arm64_sys_reboot+0x2c/0x38
[    8.664833]  el0_svc_common.constprop.2+0xe8/0x168
[    8.669631]  el0_svc_handler+0x34/0x80
[    8.673388]  el0_svc+0x8/0x1fc
[    8.676447] ---[ end trace 778d7966a5ef8213 ]---
[    8.694539] pci_bus 0001:01: busn_res: [bus 01] is released
[    8.700279] pci_bus 0001:00: busn_res: [bus 00-ff] is released
[    8.710206] reboot: Restarting system


> 
> It is ... unfortunate that there's such a variety of implementations
> here.  I don't believe these driver differences are all necessary
> consequences of hardware differences.

Yes, that is a bit unfortunate, Rob's idea to consolidate the
implementation sounds good and we can definitively test that.

> 
>> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
>> ---
>>  drivers/pci/controller/pcie-brcmstb.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
>> index d3af8d84f0d6..a1fe1a2ada48 100644
>> --- a/drivers/pci/controller/pcie-brcmstb.c
>> +++ b/drivers/pci/controller/pcie-brcmstb.c
>> @@ -1340,6 +1340,15 @@ static int brcm_pcie_remove(struct platform_device *pdev)
>>  	return 0;
>>  }
>>  
>> +static void brcm_pcie_shutdown(struct platform_device *pdev)
>> +{
>> +	struct brcm_pcie *pcie = platform_get_drvdata(pdev);
>> +
>> +	if (pcie->has_err_report)
>> +		brcm_unregister_die_notifiers(pcie);
>> +	__brcm_pcie_remove(pcie);
>> +}
>> +
>>  static const struct of_device_id brcm_pcie_match[] = {
>>  	{ .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
>>  	{ .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_cfg },
>> @@ -1460,6 +1469,7 @@ static const struct dev_pm_ops brcm_pcie_pm_ops = {
>>  static struct platform_driver brcm_pcie_driver = {
>>  	.probe = brcm_pcie_probe,
>>  	.remove = brcm_pcie_remove,
>> +	.shutdown = brcm_pcie_shutdown,
>>  	.driver = {
>>  		.name = "brcm-pcie",
>>  		.of_match_table = brcm_pcie_match,
>> -- 
>> 2.17.1
>>
Bjorn Helgaas June 3, 2021, 5:23 p.m. UTC | #6
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
somehow.

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
    device_shutdown
      dev->bus->shutdown
        pci_device_shutdown                     # pci_bus_type.shutdown
          drv->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
driver.

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.

Bjorn
Florian Fainelli June 3, 2021, 5:30 p.m. UTC | #7
On 6/3/21 10:23 AM, Bjorn Helgaas wrote:
> 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
> somehow.
> 
> 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
>     device_shutdown
>       dev->bus->shutdown
>         pci_device_shutdown                     # pci_bus_type.shutdown
>           drv->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
> driver.
> 
> 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.

Correct, the S5 shutdown is via kernel_power_off() and will turn off all
that we can in the PCIe root complex and its PHY, drop the PCIe link to
the end-point which signals that the end-point can enter its own suspend
logic, too. And yes, when we do wake-up from S5 it means booting a
completely new kernel. S5 is typically implemented in our chips by
keeping just a little bit of logic active to service wake-up events
(infrared remotes, GPIOs, RTC, etc.).
Bjorn Helgaas June 3, 2021, 8:58 p.m. UTC | #8
On Thu, Jun 03, 2021 at 10:30:37AM -0700, Florian Fainelli wrote:
> On 6/3/21 10:23 AM, Bjorn Helgaas wrote:
> > 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
> > somehow.
> > 
> > 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
> >     device_shutdown
> >       dev->bus->shutdown
> >         pci_device_shutdown                     # pci_bus_type.shutdown
> >           drv->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
> > driver.
> > 
> > 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.
> 
> Correct, the S5 shutdown is via kernel_power_off() and will turn off all
> that we can in the PCIe root complex and its PHY, drop the PCIe link to
> the end-point which signals that the end-point can enter its own suspend
> logic, too. And yes, when we do wake-up from S5 it means booting a
> completely new kernel. S5 is typically implemented in our chips by
> keeping just a little bit of logic active to service wake-up events
> (infrared remotes, GPIOs, RTC, etc.).

Which part of that does this patch change?  Is it that the new
.shutdown() turns off more power than machine_power_off() does by
itself?

Bjorn
Florian Fainelli June 3, 2021, 9:01 p.m. UTC | #9
On 6/3/21 1:58 PM, Bjorn Helgaas wrote:
> On Thu, Jun 03, 2021 at 10:30:37AM -0700, Florian Fainelli wrote:
>> On 6/3/21 10:23 AM, Bjorn Helgaas wrote:
>>> 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
>>> somehow.
>>>
>>> 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
>>>     device_shutdown
>>>       dev->bus->shutdown
>>>         pci_device_shutdown                     # pci_bus_type.shutdown
>>>           drv->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
>>> driver.
>>>
>>> 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.
>>
>> Correct, the S5 shutdown is via kernel_power_off() and will turn off all
>> that we can in the PCIe root complex and its PHY, drop the PCIe link to
>> the end-point which signals that the end-point can enter its own suspend
>> logic, too. And yes, when we do wake-up from S5 it means booting a
>> completely new kernel. S5 is typically implemented in our chips by
>> keeping just a little bit of logic active to service wake-up events
>> (infrared remotes, GPIOs, RTC, etc.).
> 
> Which part of that does this patch change?  Is it that the new
> .shutdown() turns off more power than machine_power_off() does by
> itself?

Yes, with pcie-brcmstb.c providing a .shutdow() callback we have a
chance to turn off our PCIe PHY and the RC's digital clock which would
not be able to do otherwise.
diff mbox series

Patch

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index d3af8d84f0d6..a1fe1a2ada48 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1340,6 +1340,15 @@  static int brcm_pcie_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static void brcm_pcie_shutdown(struct platform_device *pdev)
+{
+	struct brcm_pcie *pcie = platform_get_drvdata(pdev);
+
+	if (pcie->has_err_report)
+		brcm_unregister_die_notifiers(pcie);
+	__brcm_pcie_remove(pcie);
+}
+
 static const struct of_device_id brcm_pcie_match[] = {
 	{ .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
 	{ .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_cfg },
@@ -1460,6 +1469,7 @@  static const struct dev_pm_ops brcm_pcie_pm_ops = {
 static struct platform_driver brcm_pcie_driver = {
 	.probe = brcm_pcie_probe,
 	.remove = brcm_pcie_remove,
+	.shutdown = brcm_pcie_shutdown,
 	.driver = {
 		.name = "brcm-pcie",
 		.of_match_table = brcm_pcie_match,