On Tue, May 20, 2025 at 12:12 AM Manivannan Sadhasivam
<manivannan.sadhasivam@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxxx> wrote:
> > > > > On Mon, May 19, 2025 at 1:28 PM Manivannan Sadhasivam
> > > > > <manivannan.sadhasivam@xxxxxxxxxx> 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:
smime.p7s
Description: S/MIME Cryptographic Signature
