POSSIBLE REGRESSION: PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created

POSSIBLE REGRESSION: PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 1598 bytes --]

Hello,

I recently rebased to the latest Linux master

ebd297a2affa Linus.Torvalds Merge tag 'net-6.15-rc5' of
git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net

and noticed that PCI is broken for
"drivers/pci/controller/pcie-brcmstb.c"  I've bisected this to the
following commit

2489eeb777af PCI/pwrctrl: Skip scanning for the device further if
pwrctrl device is created

which is part of the series [1].  The driver in pcie-brcmstb.c is
expecting the add_bus() method to be invoked twice per boot-up, but
the second call does not happen.  Not only does this code in
brcm_pcie_add_bus() turn on regulators, it also subsequently initiates
PCIe linkup.

If I revert the aforementioned commit, all is well.

FWIW, I have included the relevant sections of the PCIe DT we use at [2].

Sorry I did not observe this sooner...

Regards,
Jim Quinlan
Broadcom STB/CM



[1] https://lore.kernel.org/lkml/20241231-pci-pwrctrl-slot-v2-0-6a15088ba541@linaro.org/T/#t
[2]

pcie@1000110000 {
        reg = <0x10 0x110000 0x0 0x9130>;
        ...

        pci@0,0 {
                vpcie3v3-supply = <0x45>;
                vpcie12v-supply = <0x44>;
                reg = <0x0 0x0 0x0 0x0 0x0>;
                ranges;
                bus-range = <0x1 0xff>;
                compatible = "pciclass,0604";
                device_type = "pci";
                #address-cells = <0x3>;
                #size-cells = <0x2>;

                pci-ep@0,0 {
                        local-mac-address = [ 00 10 18 f0 35 55 ];
                        reg = <0x10000 0x0 0x0 0x0 0x0>;
                };
        };
}

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4197 bytes --]

Re: POSSIBLE REGRESSION: PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created

[Bjorn Helgaas]

On Mon, May 05, 2025 at 01:39:39PM -0400, Jim Quinlan wrote:
> Hello,
> 
> I recently rebased to the latest Linux master
> 
> ebd297a2affa Linus.Torvalds Merge tag 'net-6.15-rc5' of
> git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net
> 
> and noticed that PCI is broken for
> "drivers/pci/controller/pcie-brcmstb.c"  I've bisected this to the
> following commit
> 
> 2489eeb777af PCI/pwrctrl: Skip scanning for the device further if
> pwrctrl device is created
> 
> which is part of the series [1].  The driver in pcie-brcmstb.c is
> expecting the add_bus() method to be invoked twice per boot-up, but
> the second call does not happen.  Not only does this code in
> brcm_pcie_add_bus() turn on regulators, it also subsequently initiates
> PCIe linkup.
> 
> If I revert the aforementioned commit, all is well.
> 
> FWIW, I have included the relevant sections of the PCIe DT we use at [2].

Mani, Bartosz, where are we at with this?  The 2489eeb777af commit log
doesn't mention a problem fixed by that commit; it sounds more like an
optimization -- just avoiding an unnecessary scan.

2489eeb777af appeared in v6.15-rc1, so there's still time to revert it
before v6.15 if that's the right way to fix this regression.

> [1] https://lore.kernel.org/lkml/20241231-pci-pwrctrl-slot-v2-0-6a15088ba541@linaro.org/T/#t
> [2]
> 
> pcie@1000110000 {
>         reg = <0x10 0x110000 0x0 0x9130>;
>         ...
> 
>         pci@0,0 {
>                 vpcie3v3-supply = <0x45>;
>                 vpcie12v-supply = <0x44>;
>                 reg = <0x0 0x0 0x0 0x0 0x0>;
>                 ranges;
>                 bus-range = <0x1 0xff>;
>                 compatible = "pciclass,0604";
>                 device_type = "pci";
>                 #address-cells = <0x3>;
>                 #size-cells = <0x2>;
> 
>                 pci-ep@0,0 {
>                         local-mac-address = [ 00 10 18 f0 35 55 ];
>                         reg = <0x10000 0x0 0x0 0x0 0x0>;
>                 };
>         };
> }

Re: POSSIBLE REGRESSION: PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created

[Manivannan Sadhasivam]

On Mon, May 19, 2025 at 09:05:57AM -0500, Bjorn Helgaas wrote:
> On Mon, May 05, 2025 at 01:39:39PM -0400, Jim Quinlan wrote:
> > Hello,
> > 
> > I recently rebased to the latest Linux master
> > 
> > ebd297a2affa Linus.Torvalds Merge tag 'net-6.15-rc5' of
> > git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net
> > 
> > and noticed that PCI is broken for
> > "drivers/pci/controller/pcie-brcmstb.c"  I've bisected this to the
> > following commit
> > 
> > 2489eeb777af PCI/pwrctrl: Skip scanning for the device further if
> > pwrctrl device is created
> > 
> > which is part of the series [1].  The driver in pcie-brcmstb.c is
> > expecting the add_bus() method to be invoked twice per boot-up, but
> > the second call does not happen.  Not only does this code in
> > brcm_pcie_add_bus() turn on regulators, it also subsequently initiates
> > PCIe linkup.
> > 
> > If I revert the aforementioned commit, all is well.
> > 
> > FWIW, I have included the relevant sections of the PCIe DT we use at [2].
> 
> Mani, Bartosz, where are we at with this?  The 2489eeb777af commit log
> doesn't mention a problem fixed by that commit; it sounds more like an
> optimization -- just avoiding an unnecessary scan.
> 
> 2489eeb777af appeared in v6.15-rc1, so there's still time to revert it
> before v6.15 if that's the right way to fix this regression.
> 

We need to find out what is happening in the pcie-brcmstb driver first. From
Jim's report, it looks like the driver expects add_bus() callback to be invoked
twice, which seems weird.

If the fix takes time, then we can revert 2489eeb777af in the meantime.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: POSSIBLE REGRESSION: PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 2458 bytes --]

On Mon, May 19, 2025 at 1:28 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Mon, May 19, 2025 at 09:05:57AM -0500, Bjorn Helgaas wrote:
> > On Mon, May 05, 2025 at 01:39:39PM -0400, Jim Quinlan wrote:
> > > Hello,
> > >
> > > I recently rebased to the latest Linux master
> > >
> > > ebd297a2affa Linus.Torvalds Merge tag 'net-6.15-rc5' of
> > > git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net
> > >
> > > and noticed that PCI is broken for
> > > "drivers/pci/controller/pcie-brcmstb.c"  I've bisected this to the
> > > following commit
> > >
> > > 2489eeb777af PCI/pwrctrl: Skip scanning for the device further if
> > > pwrctrl device is created
> > >
> > > which is part of the series [1].  The driver in pcie-brcmstb.c is
> > > expecting the add_bus() method to be invoked twice per boot-up, but
> > > the second call does not happen.  Not only does this code in
> > > brcm_pcie_add_bus() turn on regulators, it also subsequently initiates
> > > PCIe linkup.
> > >
> > > If I revert the aforementioned commit, all is well.
> > >
> > > FWIW, I have included the relevant sections of the PCIe DT we use at [2].
> >
> > Mani, Bartosz, where are we at with this?  The 2489eeb777af commit log
> > doesn't mention a problem fixed by that commit; it sounds more like an
> > optimization -- just avoiding an unnecessary scan.
> >
> > 2489eeb777af appeared in v6.15-rc1, so there's still time to revert it
> > before v6.15 if that's the right way to fix this regression.
> >
>
> We need to find out what is happening in the pcie-brcmstb driver first. From
> Jim's report, it looks like the driver expects add_bus() callback to be invoked
> twice, which seems weird.

Hi Mani,

The pci_ops add_bus() method is invoked once for bus 0 and once for
bus 1. Note that our controller only has one port.  If I do "lspci" I
get (our controller is on pci domain==1):

0001:00:00.0 PCI bridge: Broadcom Inc. and subsidiaries Device 7712 (rev 20)
0001:01:00.0 Ethernet controller: Broadcom Inc. ...

It is the second invocation of add_bus() that has the brcmstb driver
turning on the regulators and the subsequent link-up, and this call
never happens with the 2489eeb777aff9 commit.

Regards,
Jim Quinlan
Broadcom STB/CM


>
> If the fix takes time, then we can revert 2489eeb777af in the meantime.
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4197 bytes --]

Re: POSSIBLE REGRESSION: PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 4007 bytes --]

On Mon, May 19, 2025 at 2:25 PM Jim Quinlan <james.quinlan@broadcom.com> wrote:
>
> On Mon, May 19, 2025 at 1:28 PM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > On Mon, May 19, 2025 at 09:05:57AM -0500, Bjorn Helgaas wrote:
> > > On Mon, May 05, 2025 at 01:39:39PM -0400, Jim Quinlan wrote:
> > > > Hello,
> > > >
> > > > I recently rebased to the latest Linux master
> > > >
> > > > ebd297a2affa Linus.Torvalds Merge tag 'net-6.15-rc5' of
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net
> > > >
> > > > and noticed that PCI is broken for
> > > > "drivers/pci/controller/pcie-brcmstb.c"  I've bisected this to the
> > > > following commit
> > > >
> > > > 2489eeb777af PCI/pwrctrl: Skip scanning for the device further if
> > > > pwrctrl device is created
> > > >
> > > > which is part of the series [1].  The driver in pcie-brcmstb.c is
> > > > expecting the add_bus() method to be invoked twice per boot-up, but
> > > > the second call does not happen.  Not only does this code in
> > > > brcm_pcie_add_bus() turn on regulators, it also subsequently initiates
> > > > PCIe linkup.
> > > >
> > > > If I revert the aforementioned commit, all is well.
> > > >
> > > > FWIW, I have included the relevant sections of the PCIe DT we use at [2].
> > >
> > > Mani, Bartosz, where are we at with this?  The 2489eeb777af commit log
> > > doesn't mention a problem fixed by that commit; it sounds more like an
> > > optimization -- just avoiding an unnecessary scan.
> > >
> > > 2489eeb777af appeared in v6.15-rc1, so there's still time to revert it
> > > before v6.15 if that's the right way to fix this regression.
> > >
> >
> > We need to find out what is happening in the pcie-brcmstb driver first. From
> > Jim's report, it looks like the driver expects add_bus() callback to be invoked
> > twice, which seems weird.
>
> Hi Mani,
>
> The pci_ops add_bus() method is invoked once for bus 0 and once for
> bus 1. Note that our controller only has one port.  If I do "lspci" I
> get (our controller is on pci domain==1):
>
> 0001:00:00.0 PCI bridge: Broadcom Inc. and subsidiaries Device 7712 (rev 20)
> 0001:01:00.0 Ethernet controller: Broadcom Inc. ...
>
> It is the second invocation of add_bus() that has the brcmstb driver
> turning on the regulators and the subsequent link-up, and this call
> never happens with the 2489eeb777aff9 commit.

Hello Mani,

Actually, I don't think it is sufficient to just revert that one
commit.  If I checkout 6.14-rc1 and just bring in

06bf05d7349c PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()

I get the following after getting the Linux prompt:

pci 0001:00:00.0: deferred probe pending: pci: supplier
1000110000.pcie:pci@0,0 not ready
pci 0001:00:00.0: deferred probe pending: pci: supplier
1000110000.pcie:pci@0,0 not ready

Basically, brcmstb already picks up and controls the regulator nodes
under the port driver node.  You folks are adding a new generic system
and we are stepping on one another's toes.  The problem here is that I
cannot seem to turn your system off using CONFIG_PWR* settings.

We would certainly be open to adopting your system when it meets our
requirements; such as turning off/on on regulators @suspend/resume,
and the ability to not do that if the downstream device has
device_may_wakeup(dev)==true.  But until then we need a way to disable
your system or allow brcmstb to "opt out".

AFAICT this regression does not affect the RaspberryPi SoCs, so it is
not a big deal for us if we take our time to fix this.   But if so, it
is incumbent on you folks to help me get past this regression.  Is
that reasonable?

Regards,
Jim Quinlan
Broadcom STB/CM




>
> Regards,
> Jim Quinlan
> Broadcom STB/CM
>
>
> >
> > If the fix takes time, then we can revert 2489eeb777af in the meantime.
> >
> > - Mani
> >
> > --
> > மணிவண்ணன் சதாசிவம்

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4197 bytes --]

Re: POSSIBLE REGRESSION: PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created

[Bjorn Helgaas]

On Mon, May 19, 2025 at 03:59:10PM -0400, Jim Quinlan wrote:
> On Mon, May 19, 2025 at 2:25 PM Jim Quinlan <james.quinlan@broadcom.com> wrote:
> > On Mon, May 19, 2025 at 1:28 PM Manivannan Sadhasivam
> > <manivannan.sadhasivam@linaro.org> wrote:
> > > On Mon, May 19, 2025 at 09:05:57AM -0500, Bjorn Helgaas wrote:
> > > > On Mon, May 05, 2025 at 01:39:39PM -0400, Jim Quinlan wrote:
> > > > > Hello,
> > > > >
> > > > > I recently rebased to the latest Linux master
> > > > >
> > > > > ebd297a2affa Linus.Torvalds Merge tag 'net-6.15-rc5' of
> > > > > git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net
> > > > >
> > > > > and noticed that PCI is broken for
> > > > > "drivers/pci/controller/pcie-brcmstb.c"  I've bisected this
> > > > > to the following commit
> > > > >
> > > > > 2489eeb777af PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created
> > > > >
> > > > > which is part of the series [1].  The driver in
> > > > > pcie-brcmstb.c is expecting the add_bus() method to be
> > > > > invoked twice per boot-up, but the second call does not
> > > > > happen.  Not only does this code in brcm_pcie_add_bus() turn
> > > > > on regulators, it also subsequently initiates PCIe linkup.
> > > > >
> > > > > If I revert the aforementioned commit, all is well.
> > > > >
> > > > > FWIW, I have included the relevant sections of the PCIe DT
> > > > > we use at [2].
> > > >
> > > > Mani, Bartosz, where are we at with this?  The 2489eeb777af
> > > > commit log doesn't mention a problem fixed by that commit; it
> > > > sounds more like an optimization -- just avoiding an
> > > > unnecessary scan.
> > > >
> > > > 2489eeb777af appeared in v6.15-rc1, so there's still time to
> > > > revert it before v6.15 if that's the right way to fix this
> > > > regression.
> > >
> > > We need to find out what is happening in the pcie-brcmstb driver
> > > first. From Jim's report, it looks like the driver expects
> > > add_bus() callback to be invoked twice, which seems weird.
> >
> > The pci_ops add_bus() method is invoked once for bus 0 and once
> > for bus 1. Note that our controller only has one port.  If I do
> > "lspci" I get (our controller is on pci domain==1):
> >
> > 0001:00:00.0 PCI bridge: Broadcom Inc. and subsidiaries Device 7712 (rev 20)
> > 0001:01:00.0 Ethernet controller: Broadcom Inc. ...
> >
> > It is the second invocation of add_bus() that has the brcmstb
> > driver turning on the regulators and the subsequent link-up, and
> > this call never happens with the 2489eeb777aff9 commit.
> 
> Actually, I don't think it is sufficient to just revert that one
> commit.  If I checkout 6.14-rc1 and just bring in
> 
> 06bf05d7349c PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()

06bf05d7349c doesn't exist upstream; I assume it is 957f40d039a9
("PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()")

> I get the following after getting the Linux prompt:
> 
> pci 0001:00:00.0: deferred probe pending: pci: supplier
> 1000110000.pcie:pci@0,0 not ready
> pci 0001:00:00.0: deferred probe pending: pci: supplier
> 1000110000.pcie:pci@0,0 not ready
> 
> Basically, brcmstb already picks up and controls the regulator nodes
> under the port driver node.  You folks are adding a new generic
> system and we are stepping on one another's toes.  The problem here
> is that I cannot seem to turn your system off using CONFIG_PWR*
> settings.
> 
> We would certainly be open to adopting your system when it meets our
> requirements; such as turning off/on on regulators @suspend/resume,
> and the ability to not do that if the downstream device has
> device_may_wakeup(dev)==true.  But until then we need a way to
> disable your system or allow brcmstb to "opt out".
> 
> AFAICT this regression does not affect the RaspberryPi SoCs, so it
> is not a big deal for us if we take our time to fix this.   But if
> so, it is incumbent on you folks to help me get past this
> regression.  Is that reasonable?

What systems does the regression affect?

I'm not keen on adding any kind of regression in v6.15 if we can avoid
it.  Given that v6.15 will likely be released next weekend, I want to
revert whatever is necessary tomorrow unless there's a clear path to a
fix very soon.

You first thought reverting 2489eeb777af ("PCI/pwrctrl: Skip scanning
for the device further if pwrctrl device is created") would be enough
to avoid the regression.

But it sounds like something is broken even if you include only the
first patch of the branch (957f40d039a9 ("PCI/pwrctrl: Move creation
of pwrctrl devices to pci_scan_device()")).

Maybe this means we need to revert the entire branch:

  55d25a101d47 ("Merge branch 'pci/pwrctrl'")
  75996c92f4de ("PCI/pwrctrl: Add pwrctrl driver for PCI slots")
  2a95c1f3468b ("dt-bindings: vendor-prefixes: Document the 'pciclass' prefix")
  2489eeb777af ("PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created")
  2d923930f2e3 ("PCI/pwrctrl: Move pci_pwrctrl_unregister() to pci_destroy_dev()")
  957f40d039a9 ("PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()")

The only patch there that sounds like it might fix a defect is
2d923930f2e3 ("PCI/pwrctrl: Move pci_pwrctrl_unregister() to
pci_destroy_dev()").  I guess if this *does* fix a defect, the defect
only happens when removing a device, and dropping a defect fix is
better than adding a regression.

Guidance please.

> > > If the fix takes time, then we can revert 2489eeb777af in the meantime.
> > >
> > > - Mani
> > >
> > > --
> > > மணிவண்ணன் சதாசிவம்

Re: POSSIBLE REGRESSION: PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 6534 bytes --]

On Mon, May 19, 2025 at 5:56 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Mon, May 19, 2025 at 03:59:10PM -0400, Jim Quinlan wrote:
> > On Mon, May 19, 2025 at 2:25 PM Jim Quinlan <james.quinlan@broadcom.com> wrote:
> > > On Mon, May 19, 2025 at 1:28 PM Manivannan Sadhasivam
> > > <manivannan.sadhasivam@linaro.org> wrote:
> > > > On Mon, May 19, 2025 at 09:05:57AM -0500, Bjorn Helgaas wrote:
> > > > > On Mon, May 05, 2025 at 01:39:39PM -0400, Jim Quinlan wrote:
> > > > > > Hello,
> > > > > >
> > > > > > I recently rebased to the latest Linux master
> > > > > >
> > > > > > ebd297a2affa Linus.Torvalds Merge tag 'net-6.15-rc5' of
> > > > > > git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net
> > > > > >
> > > > > > and noticed that PCI is broken for
> > > > > > "drivers/pci/controller/pcie-brcmstb.c"  I've bisected this
> > > > > > to the following commit
> > > > > >
> > > > > > 2489eeb777af PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created
> > > > > >
> > > > > > which is part of the series [1].  The driver in
> > > > > > pcie-brcmstb.c is expecting the add_bus() method to be
> > > > > > invoked twice per boot-up, but the second call does not
> > > > > > happen.  Not only does this code in brcm_pcie_add_bus() turn
> > > > > > on regulators, it also subsequently initiates PCIe linkup.
> > > > > >
> > > > > > If I revert the aforementioned commit, all is well.
> > > > > >
> > > > > > FWIW, I have included the relevant sections of the PCIe DT
> > > > > > we use at [2].
> > > > >
> > > > > Mani, Bartosz, where are we at with this?  The 2489eeb777af
> > > > > commit log doesn't mention a problem fixed by that commit; it
> > > > > sounds more like an optimization -- just avoiding an
> > > > > unnecessary scan.
> > > > >
> > > > > 2489eeb777af appeared in v6.15-rc1, so there's still time to
> > > > > revert it before v6.15 if that's the right way to fix this
> > > > > regression.
> > > >
> > > > We need to find out what is happening in the pcie-brcmstb driver
> > > > first. From Jim's report, it looks like the driver expects
> > > > add_bus() callback to be invoked twice, which seems weird.
> > >
> > > The pci_ops add_bus() method is invoked once for bus 0 and once
> > > for bus 1. Note that our controller only has one port.  If I do
> > > "lspci" I get (our controller is on pci domain==1):
> > >
> > > 0001:00:00.0 PCI bridge: Broadcom Inc. and subsidiaries Device 7712 (rev 20)
> > > 0001:01:00.0 Ethernet controller: Broadcom Inc. ...
> > >
> > > It is the second invocation of add_bus() that has the brcmstb
> > > driver turning on the regulators and the subsequent link-up, and
> > > this call never happens with the 2489eeb777aff9 commit.
> >
> > Actually, I don't think it is sufficient to just revert that one
> > commit.  If I checkout 6.14-rc1 and just bring in
> >
> > 06bf05d7349c PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()
>
> 06bf05d7349c doesn't exist upstream; I assume it is 957f40d039a9
> ("PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()")

Hi Bjorn,

Yes, sorry, you are correct.

>
> > I get the following after getting the Linux prompt:
> >
> > pci 0001:00:00.0: deferred probe pending: pci: supplier
> > 1000110000.pcie:pci@0,0 not ready
> > pci 0001:00:00.0: deferred probe pending: pci: supplier
> > 1000110000.pcie:pci@0,0 not ready
> >
> > Basically, brcmstb already picks up and controls the regulator nodes
> > under the port driver node.  You folks are adding a new generic
> > system and we are stepping on one another's toes.  The problem here
> > is that I cannot seem to turn your system off using CONFIG_PWR*
> > settings.
> >
> > We would certainly be open to adopting your system when it meets our
> > requirements; such as turning off/on on regulators @suspend/resume,
> > and the ability to not do that if the downstream device has
> > device_may_wakeup(dev)==true.  But until then we need a way to
> > disable your system or allow brcmstb to "opt out".
> >
> > AFAICT this regression does not affect the RaspberryPi SoCs, so it
> > is not a big deal for us if we take our time to fix this.   But if
> > so, it is incumbent on you folks to help me get past this
> > regression.  Is that reasonable?
>
> What systems does the regression affect?

All Broadcom STB chips, including the RPi sister chips.  Now is there
anyone but our team who runs upstream Linux on our boards?  Probably
not.

>
> I'm not keen on adding any kind of regression in v6.15 if we can avoid
> it.  Given that v6.15 will likely be released next weekend, I want to
> revert whatever is necessary tomorrow unless there's a clear path to a
> fix very soon.
>
> You first thought reverting 2489eeb777af ("PCI/pwrctrl: Skip scanning
> for the device further if pwrctrl device is created") would be enough
> to avoid the regression.
>
> But it sounds like something is broken even if you include only the
> first patch of the branch (957f40d039a9 ("PCI/pwrctrl: Move creation
> of pwrctrl devices to pci_scan_device()")).

Correct.

>
> Maybe this means we need to revert the entire branch:
>
>   55d25a101d47 ("Merge branch 'pci/pwrctrl'")
>   75996c92f4de ("PCI/pwrctrl: Add pwrctrl driver for PCI slots")
>   2a95c1f3468b ("dt-bindings: vendor-prefixes: Document the 'pciclass' prefix")
>   2489eeb777af ("PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created")
>   2d923930f2e3 ("PCI/pwrctrl: Move pci_pwrctrl_unregister() to pci_destroy_dev()")
>   957f40d039a9 ("PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()")
>
> The only patch there that sounds like it might fix a defect is
> 2d923930f2e3 ("PCI/pwrctrl: Move pci_pwrctrl_unregister() to
> pci_destroy_dev()").  I guess if this *does* fix a defect, the defect
> only happens when removing a device, and dropping a defect fix is
> better than adding a regression.
>
> Guidance please.

I resisted calling for the entire series to be reverted because I
don't want to be the bad guy and I know how disappointing it is for
the authors.  But truth be told it is easier for me to deal with this
before it goes in than after.

Regards,
Jim Quinlan
Broadcom STB/CM


>
> > > > If the fix takes time, then we can revert 2489eeb777af in the meantime.
> > > >
> > > > - Mani
> > > >
> > > > --
> > > > மணிவண்ணன் சதாசிவம்
>
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4197 bytes --]

Re: POSSIBLE REGRESSION: PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created

[Manivannan Sadhasivam]

On Mon, May 19, 2025 at 07:03:18PM -0400, Jim Quinlan wrote:
> On Mon, May 19, 2025 at 5:56 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > On Mon, May 19, 2025 at 03:59:10PM -0400, Jim Quinlan wrote:
> > > On Mon, May 19, 2025 at 2:25 PM Jim Quinlan <james.quinlan@broadcom.com> wrote:
> > > > On Mon, May 19, 2025 at 1:28 PM Manivannan Sadhasivam
> > > > <manivannan.sadhasivam@linaro.org> wrote:
> > > > > On Mon, May 19, 2025 at 09:05:57AM -0500, Bjorn Helgaas wrote:
> > > > > > On Mon, May 05, 2025 at 01:39:39PM -0400, Jim Quinlan wrote:
> > > > > > > Hello,
> > > > > > >
> > > > > > > I recently rebased to the latest Linux master
> > > > > > >
> > > > > > > ebd297a2affa Linus.Torvalds Merge tag 'net-6.15-rc5' of
> > > > > > > git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net
> > > > > > >
> > > > > > > and noticed that PCI is broken for
> > > > > > > "drivers/pci/controller/pcie-brcmstb.c"  I've bisected this
> > > > > > > to the following commit
> > > > > > >
> > > > > > > 2489eeb777af PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created
> > > > > > >
> > > > > > > which is part of the series [1].  The driver in
> > > > > > > pcie-brcmstb.c is expecting the add_bus() method to be
> > > > > > > invoked twice per boot-up, but the second call does not
> > > > > > > happen.  Not only does this code in brcm_pcie_add_bus() turn
> > > > > > > on regulators, it also subsequently initiates PCIe linkup.
> > > > > > >
> > > > > > > If I revert the aforementioned commit, all is well.
> > > > > > >
> > > > > > > FWIW, I have included the relevant sections of the PCIe DT
> > > > > > > we use at [2].
> > > > > >
> > > > > > Mani, Bartosz, where are we at with this?  The 2489eeb777af
> > > > > > commit log doesn't mention a problem fixed by that commit; it
> > > > > > sounds more like an optimization -- just avoiding an
> > > > > > unnecessary scan.
> > > > > >
> > > > > > 2489eeb777af appeared in v6.15-rc1, so there's still time to
> > > > > > revert it before v6.15 if that's the right way to fix this
> > > > > > regression.
> > > > >
> > > > > We need to find out what is happening in the pcie-brcmstb driver
> > > > > first. From Jim's report, it looks like the driver expects
> > > > > add_bus() callback to be invoked twice, which seems weird.
> > > >
> > > > The pci_ops add_bus() method is invoked once for bus 0 and once
> > > > for bus 1. Note that our controller only has one port.  If I do
> > > > "lspci" I get (our controller is on pci domain==1):
> > > >
> > > > 0001:00:00.0 PCI bridge: Broadcom Inc. and subsidiaries Device 7712 (rev 20)
> > > > 0001:01:00.0 Ethernet controller: Broadcom Inc. ...
> > > >
> > > > It is the second invocation of add_bus() that has the brcmstb
> > > > driver turning on the regulators and the subsequent link-up, and
> > > > this call never happens with the 2489eeb777aff9 commit.
> > >
> > > Actually, I don't think it is sufficient to just revert that one
> > > commit.  If I checkout 6.14-rc1 and just bring in
> > >
> > > 06bf05d7349c PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()
> >
> > 06bf05d7349c doesn't exist upstream; I assume it is 957f40d039a9
> > ("PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()")
> 
> Hi Bjorn,
> 
> Yes, sorry, you are correct.
> 
> >
> > > I get the following after getting the Linux prompt:
> > >
> > > pci 0001:00:00.0: deferred probe pending: pci: supplier
> > > 1000110000.pcie:pci@0,0 not ready
> > > pci 0001:00:00.0: deferred probe pending: pci: supplier
> > > 1000110000.pcie:pci@0,0 not ready
> > >
> > > Basically, brcmstb already picks up and controls the regulator nodes
> > > under the port driver node.  You folks are adding a new generic
> > > system and we are stepping on one another's toes.  The problem here
> > > is that I cannot seem to turn your system off using CONFIG_PWR*
> > > settings.
> > >
> > > We would certainly be open to adopting your system when it meets our
> > > requirements; such as turning off/on on regulators @suspend/resume,
> > > and the ability to not do that if the downstream device has
> > > device_may_wakeup(dev)==true.  But until then we need a way to
> > > disable your system or allow brcmstb to "opt out".
> > >
> > > AFAICT this regression does not affect the RaspberryPi SoCs, so it
> > > is not a big deal for us if we take our time to fix this.   But if
> > > so, it is incumbent on you folks to help me get past this
> > > regression.  Is that reasonable?
> >
> > What systems does the regression affect?
> 
> All Broadcom STB chips, including the RPi sister chips.  Now is there
> anyone but our team who runs upstream Linux on our boards?  Probably
> not.
> 

I forgot to ask you this question. Is your devicetree upstreamed? Because, while
introducing the pwrctrl knobs, I made sure that there are no upstream users
using the supply properties in child nodes. All our existing users are using the
properties in controller nodes, so they are not impacted.

Here it looks like you are running out-of-tree dts, which we do not support
unfortunately. But it doesn't mean that I do not care about the issue you
reported. I'm flying back home from vacation tomorrow and it will be the first
thing I'm goona look into.

Adding suspend/resume support is pretty much what I'm going to work on the
upcoming weeks (combined with some rework). So until then, I request you to
revert the changes in your downstream tree and bear with me.

Bjorn, FYI: We do not need any revert in mainline since the issue is not
affecting upstream users.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: POSSIBLE REGRESSION: PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 7481 bytes --]

On Tue, May 20, 2025 at 12:12 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Mon, May 19, 2025 at 07:03:18PM -0400, Jim Quinlan wrote:
> > On Mon, May 19, 2025 at 5:56 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > >
> > > On Mon, May 19, 2025 at 03:59:10PM -0400, Jim Quinlan wrote:
> > > > On Mon, May 19, 2025 at 2:25 PM Jim Quinlan <james.quinlan@broadcom.com> wrote:
> > > > > On Mon, May 19, 2025 at 1:28 PM Manivannan Sadhasivam
> > > > > <manivannan.sadhasivam@linaro.org> wrote:
> > > > > > On Mon, May 19, 2025 at 09:05:57AM -0500, Bjorn Helgaas wrote:
> > > > > > > On Mon, May 05, 2025 at 01:39:39PM -0400, Jim Quinlan wrote:
> > > > > > > > Hello,
> > > > > > > >
> > > > > > > > I recently rebased to the latest Linux master
> > > > > > > >
> > > > > > > > ebd297a2affa Linus.Torvalds Merge tag 'net-6.15-rc5' of
> > > > > > > > git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net
> > > > > > > >
> > > > > > > > and noticed that PCI is broken for
> > > > > > > > "drivers/pci/controller/pcie-brcmstb.c"  I've bisected this
> > > > > > > > to the following commit
> > > > > > > >
> > > > > > > > 2489eeb777af PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created
> > > > > > > >
> > > > > > > > which is part of the series [1].  The driver in
> > > > > > > > pcie-brcmstb.c is expecting the add_bus() method to be
> > > > > > > > invoked twice per boot-up, but the second call does not
> > > > > > > > happen.  Not only does this code in brcm_pcie_add_bus() turn
> > > > > > > > on regulators, it also subsequently initiates PCIe linkup.
> > > > > > > >
> > > > > > > > If I revert the aforementioned commit, all is well.
> > > > > > > >
> > > > > > > > FWIW, I have included the relevant sections of the PCIe DT
> > > > > > > > we use at [2].
> > > > > > >
> > > > > > > Mani, Bartosz, where are we at with this?  The 2489eeb777af
> > > > > > > commit log doesn't mention a problem fixed by that commit; it
> > > > > > > sounds more like an optimization -- just avoiding an
> > > > > > > unnecessary scan.
> > > > > > >
> > > > > > > 2489eeb777af appeared in v6.15-rc1, so there's still time to
> > > > > > > revert it before v6.15 if that's the right way to fix this
> > > > > > > regression.
> > > > > >
> > > > > > We need to find out what is happening in the pcie-brcmstb driver
> > > > > > first. From Jim's report, it looks like the driver expects
> > > > > > add_bus() callback to be invoked twice, which seems weird.
> > > > >
> > > > > The pci_ops add_bus() method is invoked once for bus 0 and once
> > > > > for bus 1. Note that our controller only has one port.  If I do
> > > > > "lspci" I get (our controller is on pci domain==1):
> > > > >
> > > > > 0001:00:00.0 PCI bridge: Broadcom Inc. and subsidiaries Device 7712 (rev 20)
> > > > > 0001:01:00.0 Ethernet controller: Broadcom Inc. ...
> > > > >
> > > > > It is the second invocation of add_bus() that has the brcmstb
> > > > > driver turning on the regulators and the subsequent link-up, and
> > > > > this call never happens with the 2489eeb777aff9 commit.
> > > >
> > > > Actually, I don't think it is sufficient to just revert that one
> > > > commit.  If I checkout 6.14-rc1 and just bring in
> > > >
> > > > 06bf05d7349c PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()
> > >
> > > 06bf05d7349c doesn't exist upstream; I assume it is 957f40d039a9
> > > ("PCI/pwrctrl: Move creation of pwrctrl devices to pci_scan_device()")
> >
> > Hi Bjorn,
> >
> > Yes, sorry, you are correct.
> >
> > >
> > > > I get the following after getting the Linux prompt:
> > > >
> > > > pci 0001:00:00.0: deferred probe pending: pci: supplier
> > > > 1000110000.pcie:pci@0,0 not ready
> > > > pci 0001:00:00.0: deferred probe pending: pci: supplier
> > > > 1000110000.pcie:pci@0,0 not ready
> > > >
> > > > Basically, brcmstb already picks up and controls the regulator nodes
> > > > under the port driver node.  You folks are adding a new generic
> > > > system and we are stepping on one another's toes.  The problem here
> > > > is that I cannot seem to turn your system off using CONFIG_PWR*
> > > > settings.
> > > >
> > > > We would certainly be open to adopting your system when it meets our
> > > > requirements; such as turning off/on on regulators @suspend/resume,
> > > > and the ability to not do that if the downstream device has
> > > > device_may_wakeup(dev)==true.  But until then we need a way to
> > > > disable your system or allow brcmstb to "opt out".
> > > >
> > > > AFAICT this regression does not affect the RaspberryPi SoCs, so it
> > > > is not a big deal for us if we take our time to fix this.   But if
> > > > so, it is incumbent on you folks to help me get past this
> > > > regression.  Is that reasonable?
> > >
> > > What systems does the regression affect?
> >
> > All Broadcom STB chips, including the RPi sister chips.  Now is there
> > anyone but our team who runs upstream Linux on our boards?  Probably
> > not.
> >
>
> I forgot to ask you this question. Is your devicetree upstreamed? Because, while
> introducing the pwrctrl knobs, I made sure that there are no upstream users
> using the supply properties in child nodes. All our existing users are using the
> properties in controller nodes, so they are not impacted.
Hello Mani,

Our device tree is constructed on the fly by our custom bootloader
and passed to Linux, so it does not make sense (IMO) to put them
upstream as long as we strictly follow our upstreamed YAML bindings
file.  As I mentioned before, our  brcm,stb-pcie.yaml example, which
is upstream, contains a "vpcie3v3-supply" property under the pci@0,0
node.

>
> Here it looks like you are running out-of-tree dts, which we do not support
> unfortunately.
The brcmstb YAML file is upstream, and a grep for the standard pcie
regulator names would have led you to it. I don't see how you can say
you don't support a DT that follows the upstream YAML file(s).

But it doesn't mean that I do not care about the issue you
> reported. I'm flying back home from vacation tomorrow and it will be the first
> thing I'm goona look into.
>
> Adding suspend/resume support is pretty much what I'm going to work on the
> upcoming weeks (combined with some rework). So until then, I request you to
> revert the changes in your downstream tree and bear with me.

I would rather our systems peacefully coexist, and take your changes
voluntarily and on my own schedule, rather than wait for you to add
future features.  I'm a little surprised that the CONFIG_PCI_PWRCTL
code seems to act on the PCI regulators even when its driver is not
present.

In addition, I need the ability to cancel at runtime the
suspend/resume off/on of the regulators if the downstream device.

That being said,  I don't mind if the series stays as long as you
promise to work with me to have our systems coexist.  But I do not
want to wait for future features to be added for our code to work.
>
> Bjorn, FYI: We do not need any revert in mainline since the issue is not
> affecting upstream users.

So is it a rule that all DT  must be upstreamed, or is it sufficient
to have a bindings YAML file that defines the DT for the driver?


Regards,
Jim Quinlan
Broadcom STB/CM

>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4197 bytes --]

Re: POSSIBLE REGRESSION: PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created

[Manivannan Sadhasivam]

On Tue, May 20, 2025 at 11:06:33AM -0400, Jim Quinlan wrote:

[...]

> > > > What systems does the regression affect?
> > >
> > > All Broadcom STB chips, including the RPi sister chips.  Now is there
> > > anyone but our team who runs upstream Linux on our boards?  Probably
> > > not.
> > >
> >
> > I forgot to ask you this question. Is your devicetree upstreamed? Because, while
> > introducing the pwrctrl knobs, I made sure that there are no upstream users
> > using the supply properties in child nodes. All our existing users are using the
> > properties in controller nodes, so they are not impacted.
> Hello Mani,
> 
> Our device tree is constructed on the fly by our custom bootloader
> and passed to Linux, so it does not make sense (IMO) to put them
> upstream as long as we strictly follow our upstreamed YAML bindings
> file.  As I mentioned before, our  brcm,stb-pcie.yaml example, which
> is upstream, contains a "vpcie3v3-supply" property under the pci@0,0
> node.
> 

Okay, thanks for the pointer. I was not aware that you have bootloader
constructed DTB.

> >
> > Here it looks like you are running out-of-tree dts, which we do not support
> > unfortunately.
> The brcmstb YAML file is upstream, and a grep for the standard pcie
> regulator names would have led you to it. I don't see how you can say
> you don't support a DT that follows the upstream YAML file(s).
> 

I didn't say that exactly. I thought you were using some out-of-tree vendor DTS
which doesn't adhere to the DT bindings, but I was wrong. Your usecase is
completely valid.

> But it doesn't mean that I do not care about the issue you
> > reported. I'm flying back home from vacation tomorrow and it will be the first
> > thing I'm goona look into.
> >
> > Adding suspend/resume support is pretty much what I'm going to work on the
> > upcoming weeks (combined with some rework). So until then, I request you to
> > revert the changes in your downstream tree and bear with me.
> 
> I would rather our systems peacefully coexist, and take your changes
> voluntarily and on my own schedule, rather than wait for you to add
> future features.  I'm a little surprised that the CONFIG_PCI_PWRCTL
> code seems to act on the PCI regulators even when its driver is not
> present.
> 

I overlooked at that part. Would the below diff help you?

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 4b8693ec9e4c..b38a62f40448 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2514,6 +2514,9 @@ static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, in
        struct platform_device *pdev;
        struct device_node *np;
 
+       if (!IS_ENABLED(CONFIG_PCI_PWRCTRL))
+               return NULL;
+
        np = of_pci_find_child_device(dev_of_node(&bus->dev), devfn);
        if (!np || of_find_device_by_node(np))
                return NULL;


> In addition, I need the ability to cancel at runtime the
> suspend/resume off/on of the regulators if the downstream device.
> 

I don't get what you mean by 'ability to cancel at runtime'. Like I said before,
I'm going to rework pwrctrl little bit and probably add suspend/resume (system
PM first) on the way, if that's what you are referring to.

> That being said,  I don't mind if the series stays as long as you
> promise to work with me to have our systems coexist.  But I do not
> want to wait for future features to be added for our code to work.
> >
> > Bjorn, FYI: We do not need any revert in mainline since the issue is not
> > affecting upstream users.
> 
> So is it a rule that all DT  must be upstreamed, or is it sufficient
> to have a bindings YAML file that defines the DT for the driver?
> 

The latter IMO. If the diff I supplied above works fine, I'll submit a patch for
that. Eventually, we do like to control the supplies from the pwrctrl driver
instead of from controller drivers. That's the whole point of introducing this
framework. But since it is not enabled in defconfig yet, your driver should
continue working in the meantime with the diff. Later on, I will modify brcmstb
driver to adapt to pwrctrl framework. Since the binding is same, the driver is
going to be backwards compatible also.

Please let me know if it works of not!

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: POSSIBLE REGRESSION: PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 5629 bytes --]

On Tue, May 20, 2025 at 7:40 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Tue, May 20, 2025 at 11:06:33AM -0400, Jim Quinlan wrote:
>
> [...]
>
> > > > > What systems does the regression affect?
> > > >
> > > > All Broadcom STB chips, including the RPi sister chips.  Now is there
> > > > anyone but our team who runs upstream Linux on our boards?  Probably
> > > > not.
> > > >
> > >
> > > I forgot to ask you this question. Is your devicetree upstreamed? Because, while
> > > introducing the pwrctrl knobs, I made sure that there are no upstream users
> > > using the supply properties in child nodes. All our existing users are using the
> > > properties in controller nodes, so they are not impacted.
> > Hello Mani,
> >
> > Our device tree is constructed on the fly by our custom bootloader
> > and passed to Linux, so it does not make sense (IMO) to put them
> > upstream as long as we strictly follow our upstreamed YAML bindings
> > file.  As I mentioned before, our  brcm,stb-pcie.yaml example, which
> > is upstream, contains a "vpcie3v3-supply" property under the pci@0,0
> > node.
> >
>
> Okay, thanks for the pointer. I was not aware that you have bootloader
> constructed DTB.
>
> > >
> > > Here it looks like you are running out-of-tree dts, which we do not support
> > > unfortunately.
> > The brcmstb YAML file is upstream, and a grep for the standard pcie
> > regulator names would have led you to it. I don't see how you can say
> > you don't support a DT that follows the upstream YAML file(s).
> >
>
> I didn't say that exactly. I thought you were using some out-of-tree vendor DTS
> which doesn't adhere to the DT bindings, but I was wrong. Your usecase is
> completely valid.
>
> > But it doesn't mean that I do not care about the issue you
> > > reported. I'm flying back home from vacation tomorrow and it will be the first
> > > thing I'm goona look into.
> > >
> > > Adding suspend/resume support is pretty much what I'm going to work on the
> > > upcoming weeks (combined with some rework). So until then, I request you to
> > > revert the changes in your downstream tree and bear with me.
> >
> > I would rather our systems peacefully coexist, and take your changes
> > voluntarily and on my own schedule, rather than wait for you to add
> > future features.  I'm a little surprised that the CONFIG_PCI_PWRCTL
> > code seems to act on the PCI regulators even when its driver is not
> > present.
> >
>
> I overlooked at that part. Would the below diff help you?
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 4b8693ec9e4c..b38a62f40448 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2514,6 +2514,9 @@ static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, in
>         struct platform_device *pdev;
>         struct device_node *np;
>
> +       if (!IS_ENABLED(CONFIG_PCI_PWRCTRL))
> +               return NULL;
> +
>         np = of_pci_find_child_device(dev_of_node(&bus->dev), devfn);
>         if (!np || of_find_device_by_node(np))
>                 return NULL;
>
Hi Mani,
Yes that works but I have to set

CONFIG_ATH11K=n
CONFIG_ATH12K=n
CONFIG_ATH11K_PCI=n

in order to get

CONFIG_PCI_PWRCTRL=n

This would be a problem if we had a customer system that required
ATH1[2].  We do have one with ATH9k but we are safe for now.
>
> > In addition, I need the ability to cancel at runtime the
> > suspend/resume off/on of the regulators if the downstream device.
> >
>
> I don't get what you mean by 'ability to cancel at runtime'. Like I said before,
> I'm going to rework pwrctrl little bit and probably add suspend/resume (system
> PM first) on the way, if that's what you are referring to.

If you look at  drivers/pci/controller/pcie-brcmstb.c and search for
brcm_pcie_suspend_noirq(),, you will see that we only turn off/on the
regulators on suspend/resume if the downstream device has
device_may_wakeup(dev)==false.  We discover this at runtime by walking
the bus.  The idea here is to support an optional  WOL scenario.

I'm currently inquiring whether we can lessen this requirement somehow
and will let you know when I get the answer.

>
> > That being said,  I don't mind if the series stays as long as you
> > promise to work with me to have our systems coexist.  But I do not
> > want to wait for future features to be added for our code to work.
> > >
> > > Bjorn, FYI: We do not need any revert in mainline since the issue is not
> > > affecting upstream users.
> >
> > So is it a rule that all DT  must be upstreamed, or is it sufficient
> > to have a bindings YAML file that defines the DT for the driver?
> >
>
> The latter IMO. If the diff I supplied above works fine, I'll submit a patch for
> that. Eventually, we do like to control the supplies from the pwrctrl driver
> instead of from controller drivers. That's the whole point of introducing this
> framework. But since it is not enabled in defconfig yet, your driver should
> continue working in the meantime with the diff. Later on, I will modify brcmstb
> driver to adapt to pwrctrl framework. Since the binding is same, the driver is
> going to be backwards compatible also.
>
> Please let me know if it works of not!

Yes it does work.  Not a perfect situation, but it will keep me going
as we move toward having the pcie-brcmstb driver move to the pwrctrl
framework.

Thanks,
Jim Quinlan
Broadcom STB/CM


>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4197 bytes --]

Re: POSSIBLE REGRESSION: PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created

[Manivannan Sadhasivam]

On Wed, May 21, 2025 at 06:08:14PM -0400, Jim Quinlan wrote:
> On Tue, May 20, 2025 at 7:40 PM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > On Tue, May 20, 2025 at 11:06:33AM -0400, Jim Quinlan wrote:
> >
> > [...]
> >
> > > > > > What systems does the regression affect?
> > > > >
> > > > > All Broadcom STB chips, including the RPi sister chips.  Now is there
> > > > > anyone but our team who runs upstream Linux on our boards?  Probably
> > > > > not.
> > > > >
> > > >
> > > > I forgot to ask you this question. Is your devicetree upstreamed? Because, while
> > > > introducing the pwrctrl knobs, I made sure that there are no upstream users
> > > > using the supply properties in child nodes. All our existing users are using the
> > > > properties in controller nodes, so they are not impacted.
> > > Hello Mani,
> > >
> > > Our device tree is constructed on the fly by our custom bootloader
> > > and passed to Linux, so it does not make sense (IMO) to put them
> > > upstream as long as we strictly follow our upstreamed YAML bindings
> > > file.  As I mentioned before, our  brcm,stb-pcie.yaml example, which
> > > is upstream, contains a "vpcie3v3-supply" property under the pci@0,0
> > > node.
> > >
> >
> > Okay, thanks for the pointer. I was not aware that you have bootloader
> > constructed DTB.
> >
> > > >
> > > > Here it looks like you are running out-of-tree dts, which we do not support
> > > > unfortunately.
> > > The brcmstb YAML file is upstream, and a grep for the standard pcie
> > > regulator names would have led you to it. I don't see how you can say
> > > you don't support a DT that follows the upstream YAML file(s).
> > >
> >
> > I didn't say that exactly. I thought you were using some out-of-tree vendor DTS
> > which doesn't adhere to the DT bindings, but I was wrong. Your usecase is
> > completely valid.
> >
> > > But it doesn't mean that I do not care about the issue you
> > > > reported. I'm flying back home from vacation tomorrow and it will be the first
> > > > thing I'm goona look into.
> > > >
> > > > Adding suspend/resume support is pretty much what I'm going to work on the
> > > > upcoming weeks (combined with some rework). So until then, I request you to
> > > > revert the changes in your downstream tree and bear with me.
> > >
> > > I would rather our systems peacefully coexist, and take your changes
> > > voluntarily and on my own schedule, rather than wait for you to add
> > > future features.  I'm a little surprised that the CONFIG_PCI_PWRCTL
> > > code seems to act on the PCI regulators even when its driver is not
> > > present.
> > >
> >
> > I overlooked at that part. Would the below diff help you?
> >
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 4b8693ec9e4c..b38a62f40448 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -2514,6 +2514,9 @@ static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, in
> >         struct platform_device *pdev;
> >         struct device_node *np;
> >
> > +       if (!IS_ENABLED(CONFIG_PCI_PWRCTRL))
> > +               return NULL;
> > +
> >         np = of_pci_find_child_device(dev_of_node(&bus->dev), devfn);
> >         if (!np || of_find_device_by_node(np))
> >                 return NULL;
> >
> Hi Mani,
> Yes that works but I have to set
> 
> CONFIG_ATH11K=n
> CONFIG_ATH12K=n
> CONFIG_ATH11K_PCI=n
> 
> in order to get
> 
> CONFIG_PCI_PWRCTRL=n
> 
> This would be a problem if we had a customer system that required
> ATH1[2].  We do have one with ATH9k but we are safe for now.

Yeah. I do not intend to make it as "the fix", but just a workaround to get your
driver working again. The actual fix would be to convert your driver to use the
pwrctrl framework, which I'll do soon.

> >
> > > In addition, I need the ability to cancel at runtime the
> > > suspend/resume off/on of the regulators if the downstream device.
> > >
> >
> > I don't get what you mean by 'ability to cancel at runtime'. Like I said before,
> > I'm going to rework pwrctrl little bit and probably add suspend/resume (system
> > PM first) on the way, if that's what you are referring to.
> 
> If you look at  drivers/pci/controller/pcie-brcmstb.c and search for
> brcm_pcie_suspend_noirq(),, you will see that we only turn off/on the
> regulators on suspend/resume if the downstream device has
> device_may_wakeup(dev)==false.  We discover this at runtime by walking
> the bus.  The idea here is to support an optional  WOL scenario.
> 

Oh yes! Supporting WOL is indeed a requirement when the system PM support is
added to the pwrctrl framework. Also do note that some endpoints like NVMe would
like to keep the devices in D0 during suspend. So if you connect it to your
platform, it most likely will not work during resume. I'm also intending to fix
it.

> I'm currently inquiring whether we can lessen this requirement somehow
> and will let you know when I get the answer.
> 

I don't think it is needed. We should support it anyway.

> >
> > > That being said,  I don't mind if the series stays as long as you
> > > promise to work with me to have our systems coexist.  But I do not
> > > want to wait for future features to be added for our code to work.
> > > >
> > > > Bjorn, FYI: We do not need any revert in mainline since the issue is not
> > > > affecting upstream users.
> > >
> > > So is it a rule that all DT  must be upstreamed, or is it sufficient
> > > to have a bindings YAML file that defines the DT for the driver?
> > >
> >
> > The latter IMO. If the diff I supplied above works fine, I'll submit a patch for
> > that. Eventually, we do like to control the supplies from the pwrctrl driver
> > instead of from controller drivers. That's the whole point of introducing this
> > framework. But since it is not enabled in defconfig yet, your driver should
> > continue working in the meantime with the diff. Later on, I will modify brcmstb
> > driver to adapt to pwrctrl framework. Since the binding is same, the driver is
> > going to be backwards compatible also.
> >
> > Please let me know if it works of not!
> 
> Yes it does work.  Not a perfect situation, but it will keep me going
> as we move toward having the pcie-brcmstb driver move to the pwrctrl
> framework.
> 

Thanks a lot for testing. I will push out a patch with your tested-by tag.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: POSSIBLE REGRESSION: PCI/pwrctrl: Skip scanning for the device further if pwrctrl device is created

[Manivannan Sadhasivam]

On Mon, May 05, 2025 at 01:39:39PM -0400, Jim Quinlan wrote:
> Hello,
> 
> I recently rebased to the latest Linux master
> 
> ebd297a2affa Linus.Torvalds Merge tag 'net-6.15-rc5' of
> git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net
> 
> and noticed that PCI is broken for
> "drivers/pci/controller/pcie-brcmstb.c"  I've bisected this to the
> following commit
> 
> 2489eeb777af PCI/pwrctrl: Skip scanning for the device further if
> pwrctrl device is created
> 

Thanks a lot for the report and sorry for the delay in getting back.

> which is part of the series [1].  The driver in pcie-brcmstb.c is
> expecting the add_bus() method to be invoked twice per boot-up, but
> the second call does not happen.  Not only does this code in
> brcm_pcie_add_bus() turn on regulators, it also subsequently initiates
> PCIe linkup.
> 

May I know why add_bus() is invoked twice? The regulators are supposed to be
enabled only once. I should be missing something here.

- Mani

-- 
மணிவண்ணன் சதாசிவம்