From: Bjorn Helgaas <helgaas@kernel.org> To: Jim Quinlan <james.quinlan@broadcom.com> Cc: "Jim Quinlan" <jim2101024@gmail.com>, "open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS" <linux-pci@vger.kernel.org>, "Nicolas Saenz Julienne" <nsaenz@kernel.org>, "Bjorn Helgaas" <bhelgaas@google.com>, "Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>, "Cyril Brulebois" <kibi@debian.org>, "maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE" <bcm-kernel-feedback-list@broadcom.com>, "Florian Fainelli" <f.fainelli@gmail.com>, "Lorenzo Pieralisi" <lpieralisi@kernel.org>, "Rob Herring" <robh@kernel.org>, "Krzysztof Wilczyński" <kw@linux.com>, "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" <linux-rpi-kernel@lists.infradead.org>, "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" <linux-arm-kernel@lists.infradead.org>, "open list" <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v1 1/4] PCI: brcmstb: Split brcm_pcie_setup() into two funcs Date: Fri, 8 Jul 2022 17:27:38 -0500 [thread overview] Message-ID: <20220708222738.GA378386@bhelgaas> (raw) In-Reply-To: <CA+-6iNxDTTKfxKbNPVnRaKmbXuy8cJAr22mws50=GkX3ncxWgA@mail.gmail.com> On Fri, Jul 08, 2022 at 04:38:30PM -0400, Jim Quinlan wrote: > On Fri, Jul 8, 2022 at 3:59 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Fri, Jul 08, 2022 at 03:40:43PM -0400, Jim Quinlan wrote: > > > On Fri, Jul 8, 2022 at 3:04 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > On Fri, Jul 08, 2022 at 09:29:27AM -0400, Jim Quinlan wrote: > > > > > On Wed, Jul 6, 2022 at 5:56 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > > > On Fri, Jul 01, 2022 at 12:27:22PM -0400, Jim Quinlan wrote: > > > > > > > We need to take some code in brcm_pcie_setup() and put it in a new function > > > > > > > brcm_pcie_linkup(). In future commits the brcm_pcie_linkup() function will > > > > > > > be called indirectly by pci_host_probe() as opposed to the host driver > > > > > > > invoking it directly. > > > > > > > > > > > > > > Some code that was executed after the PCIe linkup is now placed so that it > > > > > > > executes prior to linkup, since this code has to run prior to the > > > > > > > invocation of pci_host_probe(). > > > > > > > > > > > > This says we need to move some code from brcm_pcie_setup() to > > > > > > brcm_pcie_linkup(), but not *why* we need to do that. > > > > > I will elaborate in the commit message. > > > > > > > > > > > > In brcm_pcie_resume(), they're called together: > > > > > > > > > > > > brcm_pcie_resume > > > > > > brcm_pcie_setup > > > > > > brcm_pcie_linkup > > > > > > > > > > > > In the probe path, they're not called together, but they're in the > > > > > > same order: > > > > > > > > > > > > brcm_pcie_probe > > > > > > brcm_pcie_setup > > > > > > pci_host_probe > > > > > > ... > > > > > > brcm_pcie_add_bus # bus->ops->add_bus > > > > > > brcm_pcie_linkup > > > > > > > > > > > > Is there something that must happen *between* them in the probe path? > > > > > > > > > > Yes. In the probe() case, we must do things in this order: > > > > > > > > > > 1. brcm_pcie_setup() > > > > > 2. Turn on regulators > > > > > 3. brcm_pcie_linkup() > > > > > > > > Ah, I see, both 2) and 3) happen in brcm_pcie_add_bus: > > > > > > > > brcm_pcie_add_bus # bus->ops->add_bus > > > > pci_subdev_regulators_add_bus > > > > regulator_bulk_enable # turn on regulators > > > > brcm_pcie_linkup > > > > > > > > > Since the voltage regulators are turned on during enumeration, > > > > > pci_host_probe() must be invoked prior to 3. Before regulators, we > > > > > did not care. > > > > > > > > I guess in the pre-regulator case, i.e., pcie->sr not set, the power > > > > for downstream devices must always be on. > > > > > > > > > In the resume case, there is no enumeration of course but our driver > > > > > has a handle to the regulators and can turn them on/off w/o help. > > > > > > > > And I guess we don't need brcm_pcie_setup() in the resume path because > > > > suspend turns off power only for downstream devices, not for the root > > > > port itself, so the programming done by brcm_pcie_setup() doesn't need > > > > to be done again. > > > > > > I'm not sure I understand what you are saying -- brcm_pcie_setup() is > > > called by brcm_pcie_resume() > > > because it is needed. brcm_pcie_setup() isn't concerned with power it > > > just does the preparation > > > required before attempting link-up. > > > > Oh, sorry, I totally misread that. > > > > But I wonder about the fact that probe and resume do these in > > different orders: > > > > brcm_pcie_probe > > brcm_pcie_setup # setup > > pci_host_probe > > ... > > brcm_pcie_add_bus > > pci_subdev_regulators_add_bus > > regulator_bulk_enable # regulators on > > brcm_pcie_linkup # linkup > > > > brcm_pcie_resume > > regulator_bulk_enable # regulators on > > brcm_pcie_setup # setup > > brcm_pcie_linkup # linkup > > > brcm_pcie_setup() should be order-independent of brcm_pcie_linkup(), > but your point is valid -- it is prudent to keep the orders > consistent. Let me think > about this. > > > Maybe pci_subdev_regulators_add_bus() could be done directly from > > brcm_pcie_probe() instead of in brcm_pcie_add_bus()? > > regulators must be directly under the root port node in DT, it seems > > like it would be reasonable to look for them in the probe path, which > > seems like what pcie-dw-rockchip.c, pcie-tegra194.c, and > > pcie-rockchip-host.c do. > At some point in the original patchset -- IIRC -- the RC driver was > searching the DT > tree for regulators. However, doing a "get" on these regulators is pretty much > impossible if the "owning" device does not exist. I even had a version that > partially created the downstream device; this pullreq was a mess and > got feedback which put me on the current approach. Ah, I suppose because the regulators are not under the host bridge itself, but under the *root port*, which is a PCI device that doesn't exist until we enumerate it. Although I guess the root port is described in the DT, and the regulators are connected with that DT description, not directly with the pci_dev. > Reviews suggested that the best location for the regulators should be located > in the root port DT node(s). I agree with this. In addition, there > was a request to allow multiple regulators > to exist at each of the root ports in the downstream tree. Makes sense. > So if the RC driver > has to potentially add multiple buses. I really don't know how it > would do that, > and then call the pci_host_probe() w/o it failing. Perhaps this is what ACPI > does before boot -- I'm guessing here -- but I would also guess that it is > a decent amount of code as it is not far from doing enumeration. > > One thing I could do is to allow the port driver's suspend/resume to do the > turning off/on of the regulators. There are two issues with this: (1) > feedback suggested > to put the code local to the Brcmstb driver and (2) the "ep wakeup_capable" > code would also have to live in the port driver and I'm not sure this > would be welcome. > > > Or maybe brcm_pcie_resume() should enable the regulators after > > brcm_pcie_setup() so it's the same order as the probe path? > I think I'll do this. Yep, sounds like the right thing. Bjorn
WARNING: multiple messages have this Message-ID (diff)
From: Bjorn Helgaas <helgaas@kernel.org> To: Jim Quinlan <james.quinlan@broadcom.com> Cc: "Jim Quinlan" <jim2101024@gmail.com>, "open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS" <linux-pci@vger.kernel.org>, "Nicolas Saenz Julienne" <nsaenz@kernel.org>, "Bjorn Helgaas" <bhelgaas@google.com>, "Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>, "Cyril Brulebois" <kibi@debian.org>, "maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE" <bcm-kernel-feedback-list@broadcom.com>, "Florian Fainelli" <f.fainelli@gmail.com>, "Lorenzo Pieralisi" <lpieralisi@kernel.org>, "Rob Herring" <robh@kernel.org>, "Krzysztof Wilczyński" <kw@linux.com>, "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" <linux-rpi-kernel@lists.infradead.org>, "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" <linux-arm-kernel@lists.infradead.org>, "open list" <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v1 1/4] PCI: brcmstb: Split brcm_pcie_setup() into two funcs Date: Fri, 8 Jul 2022 17:27:38 -0500 [thread overview] Message-ID: <20220708222738.GA378386@bhelgaas> (raw) In-Reply-To: <CA+-6iNxDTTKfxKbNPVnRaKmbXuy8cJAr22mws50=GkX3ncxWgA@mail.gmail.com> On Fri, Jul 08, 2022 at 04:38:30PM -0400, Jim Quinlan wrote: > On Fri, Jul 8, 2022 at 3:59 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Fri, Jul 08, 2022 at 03:40:43PM -0400, Jim Quinlan wrote: > > > On Fri, Jul 8, 2022 at 3:04 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > On Fri, Jul 08, 2022 at 09:29:27AM -0400, Jim Quinlan wrote: > > > > > On Wed, Jul 6, 2022 at 5:56 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > > > On Fri, Jul 01, 2022 at 12:27:22PM -0400, Jim Quinlan wrote: > > > > > > > We need to take some code in brcm_pcie_setup() and put it in a new function > > > > > > > brcm_pcie_linkup(). In future commits the brcm_pcie_linkup() function will > > > > > > > be called indirectly by pci_host_probe() as opposed to the host driver > > > > > > > invoking it directly. > > > > > > > > > > > > > > Some code that was executed after the PCIe linkup is now placed so that it > > > > > > > executes prior to linkup, since this code has to run prior to the > > > > > > > invocation of pci_host_probe(). > > > > > > > > > > > > This says we need to move some code from brcm_pcie_setup() to > > > > > > brcm_pcie_linkup(), but not *why* we need to do that. > > > > > I will elaborate in the commit message. > > > > > > > > > > > > In brcm_pcie_resume(), they're called together: > > > > > > > > > > > > brcm_pcie_resume > > > > > > brcm_pcie_setup > > > > > > brcm_pcie_linkup > > > > > > > > > > > > In the probe path, they're not called together, but they're in the > > > > > > same order: > > > > > > > > > > > > brcm_pcie_probe > > > > > > brcm_pcie_setup > > > > > > pci_host_probe > > > > > > ... > > > > > > brcm_pcie_add_bus # bus->ops->add_bus > > > > > > brcm_pcie_linkup > > > > > > > > > > > > Is there something that must happen *between* them in the probe path? > > > > > > > > > > Yes. In the probe() case, we must do things in this order: > > > > > > > > > > 1. brcm_pcie_setup() > > > > > 2. Turn on regulators > > > > > 3. brcm_pcie_linkup() > > > > > > > > Ah, I see, both 2) and 3) happen in brcm_pcie_add_bus: > > > > > > > > brcm_pcie_add_bus # bus->ops->add_bus > > > > pci_subdev_regulators_add_bus > > > > regulator_bulk_enable # turn on regulators > > > > brcm_pcie_linkup > > > > > > > > > Since the voltage regulators are turned on during enumeration, > > > > > pci_host_probe() must be invoked prior to 3. Before regulators, we > > > > > did not care. > > > > > > > > I guess in the pre-regulator case, i.e., pcie->sr not set, the power > > > > for downstream devices must always be on. > > > > > > > > > In the resume case, there is no enumeration of course but our driver > > > > > has a handle to the regulators and can turn them on/off w/o help. > > > > > > > > And I guess we don't need brcm_pcie_setup() in the resume path because > > > > suspend turns off power only for downstream devices, not for the root > > > > port itself, so the programming done by brcm_pcie_setup() doesn't need > > > > to be done again. > > > > > > I'm not sure I understand what you are saying -- brcm_pcie_setup() is > > > called by brcm_pcie_resume() > > > because it is needed. brcm_pcie_setup() isn't concerned with power it > > > just does the preparation > > > required before attempting link-up. > > > > Oh, sorry, I totally misread that. > > > > But I wonder about the fact that probe and resume do these in > > different orders: > > > > brcm_pcie_probe > > brcm_pcie_setup # setup > > pci_host_probe > > ... > > brcm_pcie_add_bus > > pci_subdev_regulators_add_bus > > regulator_bulk_enable # regulators on > > brcm_pcie_linkup # linkup > > > > brcm_pcie_resume > > regulator_bulk_enable # regulators on > > brcm_pcie_setup # setup > > brcm_pcie_linkup # linkup > > > brcm_pcie_setup() should be order-independent of brcm_pcie_linkup(), > but your point is valid -- it is prudent to keep the orders > consistent. Let me think > about this. > > > Maybe pci_subdev_regulators_add_bus() could be done directly from > > brcm_pcie_probe() instead of in brcm_pcie_add_bus()? > > regulators must be directly under the root port node in DT, it seems > > like it would be reasonable to look for them in the probe path, which > > seems like what pcie-dw-rockchip.c, pcie-tegra194.c, and > > pcie-rockchip-host.c do. > At some point in the original patchset -- IIRC -- the RC driver was > searching the DT > tree for regulators. However, doing a "get" on these regulators is pretty much > impossible if the "owning" device does not exist. I even had a version that > partially created the downstream device; this pullreq was a mess and > got feedback which put me on the current approach. Ah, I suppose because the regulators are not under the host bridge itself, but under the *root port*, which is a PCI device that doesn't exist until we enumerate it. Although I guess the root port is described in the DT, and the regulators are connected with that DT description, not directly with the pci_dev. > Reviews suggested that the best location for the regulators should be located > in the root port DT node(s). I agree with this. In addition, there > was a request to allow multiple regulators > to exist at each of the root ports in the downstream tree. Makes sense. > So if the RC driver > has to potentially add multiple buses. I really don't know how it > would do that, > and then call the pci_host_probe() w/o it failing. Perhaps this is what ACPI > does before boot -- I'm guessing here -- but I would also guess that it is > a decent amount of code as it is not far from doing enumeration. > > One thing I could do is to allow the port driver's suspend/resume to do the > turning off/on of the regulators. There are two issues with this: (1) > feedback suggested > to put the code local to the Brcmstb driver and (2) the "ep wakeup_capable" > code would also have to live in the port driver and I'm not sure this > would be welcome. > > > Or maybe brcm_pcie_resume() should enable the regulators after > > brcm_pcie_setup() so it's the same order as the probe path? > I think I'll do this. Yep, sounds like the right thing. Bjorn _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-07-08 22:27 UTC|newest] Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-07-01 16:27 [PATCH v1 0/4] PCI: brcmstb: Re-submit reverted patchset Jim Quinlan 2022-07-01 16:27 ` Jim Quinlan 2022-07-01 16:27 ` [PATCH v1 1/4] PCI: brcmstb: Split brcm_pcie_setup() into two funcs Jim Quinlan 2022-07-01 16:27 ` Jim Quinlan 2022-07-06 21:56 ` Bjorn Helgaas 2022-07-06 21:56 ` Bjorn Helgaas 2022-07-08 13:29 ` Jim Quinlan 2022-07-08 13:29 ` Jim Quinlan 2022-07-08 19:04 ` Bjorn Helgaas 2022-07-08 19:04 ` Bjorn Helgaas 2022-07-08 19:40 ` Jim Quinlan 2022-07-08 19:40 ` Jim Quinlan 2022-07-08 19:59 ` Bjorn Helgaas 2022-07-08 19:59 ` Bjorn Helgaas 2022-07-08 20:38 ` Jim Quinlan 2022-07-08 20:38 ` Jim Quinlan 2022-07-08 22:27 ` Bjorn Helgaas [this message] 2022-07-08 22:27 ` Bjorn Helgaas 2022-07-01 16:27 ` [PATCH v1 2/4] PCI: brcmstb: Add mechanism to turn on subdev regulators Jim Quinlan 2022-07-01 16:27 ` Jim Quinlan 2022-07-06 23:12 ` Bjorn Helgaas 2022-07-06 23:12 ` Bjorn Helgaas 2022-07-08 14:14 ` Jim Quinlan 2022-07-08 14:14 ` Jim Quinlan 2022-07-08 19:07 ` Bjorn Helgaas 2022-07-08 19:07 ` Bjorn Helgaas 2022-07-01 16:27 ` [PATCH v1 3/4] PCI: brcmstb: Add control of subdevice voltage regulators Jim Quinlan 2022-07-01 16:27 ` Jim Quinlan 2022-07-06 23:13 ` Bjorn Helgaas 2022-07-06 23:13 ` Bjorn Helgaas 2022-07-08 15:16 ` Jim Quinlan 2022-07-08 15:16 ` Jim Quinlan 2022-07-08 19:09 ` Bjorn Helgaas 2022-07-08 19:09 ` Bjorn Helgaas 2022-07-01 16:27 ` [PATCH v1 4/4] PCI: brcmstb: Do not turn off WOL regulators on suspend Jim Quinlan 2022-07-01 16:27 ` Jim Quinlan 2022-07-01 20:58 ` [PATCH v1 0/4] PCI: brcmstb: Re-submit reverted patchset Florian Fainelli 2022-07-01 20:58 ` Florian Fainelli 2022-07-05 20:55 ` Cyril Brulebois 2022-07-05 20:55 ` Cyril Brulebois 2022-07-05 21:00 ` Florian Fainelli 2022-07-05 21:00 ` Florian Fainelli 2022-07-05 21:28 ` Cyril Brulebois 2022-07-05 21:28 ` Cyril Brulebois 2022-07-05 22:06 ` Jim Quinlan 2022-07-05 22:06 ` Jim Quinlan 2022-07-15 18:27 ` Bjorn Helgaas 2022-07-15 18:27 ` Bjorn Helgaas 2022-07-15 18:30 ` Jim Quinlan 2022-07-15 18:30 ` Jim Quinlan
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: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20220708222738.GA378386@bhelgaas \ --to=helgaas@kernel.org \ --cc=bcm-kernel-feedback-list@broadcom.com \ --cc=bhelgaas@google.com \ --cc=f.fainelli@gmail.com \ --cc=james.quinlan@broadcom.com \ --cc=jim2101024@gmail.com \ --cc=kibi@debian.org \ --cc=kw@linux.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pci@vger.kernel.org \ --cc=linux-rpi-kernel@lists.infradead.org \ --cc=lorenzo.pieralisi@arm.com \ --cc=lpieralisi@kernel.org \ --cc=nsaenz@kernel.org \ --cc=robh@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.