From: Jim Quinlan <james.quinlan@broadcom.com>
To: Rob Herring <robh@kernel.org>
Cc: "Pali Rohár" <pali@kernel.org>,
"Jim Quinlan" <jim2101024@gmail.com>,
PCI <linux-pci@vger.kernel.org>,
"Bjorn Helgaas" <bhelgaas@google.com>,
"Nicolas Saenz Julienne" <nsaenz@kernel.org>,
"Mark Brown" <broonie@kernel.org>,
"maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE"
<bcm-kernel-feedback-list@broadcom.com>,
"Sean V Kelley" <sean.v.kelley@intel.com>,
"Jonathan Cameron" <Jonathan.Cameron@huawei.com>,
"Qiuxu Zhuo" <qiuxu.zhuo@intel.com>,
"Keith Busch" <kbusch@kernel.org>,
"open list" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v8 5/8] PCI/portdrv: add mechanism to turn on subdev regulators
Date: Wed, 17 Nov 2021 09:46:46 -0500 [thread overview]
Message-ID: <CA+-6iNy7-CjQpua9tjMt=bLCanbTqRHPkwdKM2AH7HU1a1BQzg@mail.gmail.com> (raw)
In-Reply-To: <CAL_JsqJno4ROQD38buz8Z-tU5aaQL5b_d1R0-D+c9UwnMKYNOw@mail.gmail.com>
On Tue, Nov 16, 2021 at 12:41 PM Rob Herring <robh@kernel.org> wrote:
>
> +Pali
>
> On Mon, Nov 15, 2021 at 2:44 PM Jim Quinlan <james.quinlan@broadcom.com> wrote:
> >
> > On Thu, Nov 11, 2021 at 5:57 PM Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Wed, Nov 10, 2021 at 4:15 PM Jim Quinlan <jim2101024@gmail.com> wrote:
> > > >
> > > > Adds a mechanism inside the root port device to identify standard PCIe
> > > > regulators in the DT, allocate them, and turn them on before the rest of
> > > > the bus is scanned during pci_host_probe(). A root complex driver can
> > > > leverage this mechanism by setting the pci_ops methods add_bus and
> > > > remove_bus to pci_subdev_regulators_{add,remove}_bus.
> > > >
> > > > The allocated structure that contains the regulators is stored in
> > > > dev.driver_data.
> > > >
> > > > The unabridged reason for doing this is as follows. We would like the
> > > > Broadcom STB PCIe root complex driver (and others) to be able to turn
> > > > off/on regulators[1] that provide power to endpoint[2] devices. Typically,
> > > > the drivers of these endpoint devices are stock Linux drivers that are not
> > > > aware that these regulator(s) exist and must be turned on for the driver to
> > > > be probed. The simple solution of course is to turn these regulators on at
> > > > boot and keep them on. However, this solution does not satisfy at least
> > > > three of our usage modes:drivers/pci/pcie/aspm.cdrivers/pci/pcie/aspm.c
> > > >
> > > > 1. For example, one customer uses multiple PCIe controllers, but wants the
> > > > ability to, by script invoking and unbind, turn any or all of them by and
> > > > their subdevices off to save power, e.g. when in battery mode.
> > > >
> > > > 2. Another example is when a watchdog script discovers that an endpoint
> > > > device is in an unresponsive state and would like to unbind, power toggle,
> > > > and re-bind just the PCIe endpoint and controller.
> > > >
> > > > 3. Of course we also want power turned off during suspend mode. However,
> > > > some endpoint devices may be able to "wake" during suspend and we need to
> > > > recognise this case and veto the nominal act of turning off its regulator.
> > > > Such is the case with Wake-on-LAN and Wake-on-WLAN support where PCIe
> > > > end-point device needs to be kept powered on in order to receive network
> > > > packets and wake-up the system.
> > > >
> > > > In all of these cases it is advantageous for the PCIe controller to govern
> > > > the turning off/on the regulators needed by the endpoint device. The first
> > > > two cases can be done by simply unbinding and binding the PCIe controller,
> > > > if the controller has control of these regulators.
> > > >
> > > > [1] These regulators typically govern the actual power supply to the
> > > > endpoint chip. Sometimes they may be a the official PCIe socket
> > > > power -- such as 3.3v or aux-3.3v. Sometimes they are truly
> > > > the regulator(s) that supply power to the EP chip.
> > > >
> > > > [2] The 99% configuration of our boards is a single endpoint device
> > > > attached to the PCIe controller. I use the term endpoint but it could
> > > > possible mean a switch as well.
> > > >
> > > > Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> > > > ---
> > > > drivers/pci/bus.c | 72 ++++++++++++++++++++++++++++++++++
> > > > drivers/pci/pci.h | 8 ++++
> > > > drivers/pci/pcie/portdrv_pci.c | 32 +++++++++++++++
> > > > 3 files changed, 112 insertions(+)
> > > >
> > > > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> > > > index 3cef835b375f..c39fdf36b0ad 100644
> > > > --- a/drivers/pci/bus.c
> > > > +++ b/drivers/pci/bus.c
> > > > @@ -419,3 +419,75 @@ void pci_bus_put(struct pci_bus *bus)
> > > > if (bus)
> > > > put_device(&bus->dev);
> > > > }
> > > > +
> > > > +static void *alloc_subdev_regulators(struct device *dev)
> > > > +{
> > > > + static const char * const supplies[] = {
> > > > + "vpcie3v3",
> > > > + "vpcie3v3aux",
> > > > + "vpcie12v",
> > > > + };
> > > > + const size_t size = sizeof(struct subdev_regulators)
> > > > + + sizeof(struct regulator_bulk_data) * ARRAY_SIZE(supplies);
> > > > + struct subdev_regulators *sr;
> > > > + int i;
> > > > +
> > > > + sr = devm_kzalloc(dev, size, GFP_KERNEL);
> > > > +
> > > > + if (sr) {
> > > > + sr->num_supplies = ARRAY_SIZE(supplies);
> > > > + for (i = 0; i < ARRAY_SIZE(supplies); i++)
> > > > + sr->supplies[i].supply = supplies[i];
> > > > + }
> > > > +
> > > > + return sr;
> > > > +}
> > > > +
> > > > +
> > > > +int pci_subdev_regulators_add_bus(struct pci_bus *bus)
> > > > +{
> > > > + struct device *dev = &bus->dev;
> > > > + struct subdev_regulators *sr;
> > > > + int ret;
> > > > +
> > > > + if (!pcie_is_port_dev(bus->self))
> > > > + return 0;
> > > > +
> > > > + if (WARN_ON(bus->dev.driver_data))
> > > > + dev_err(dev, "multiple clients using dev.driver_data\n");
> > > > +
> > > > + sr = alloc_subdev_regulators(&bus->dev);
> > > > + if (!sr)
> > > > + return -ENOMEM;
> > > > +
> > > > + bus->dev.driver_data = sr;
> > > > + ret = regulator_bulk_get(dev, sr->num_supplies, sr->supplies);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + ret = regulator_bulk_enable(sr->num_supplies, sr->supplies);
> > > > + if (ret) {
> > > > + dev_err(dev, "failed to enable regulators for downstream device\n");
> > > > + return ret;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(pci_subdev_regulators_add_bus);
> > >
> > > Can't these just go in the portdrv probe and remove functions now?
> > >
> > > Rob
> >
> > Not really. The idea is that only when a host controller driver does this
> >
> > static struct pci_ops my_pcie_ops = {
> > .add_bus = pci_subdev_regulators_add_bus , /* see note below */
> > .remove_bus = pci_subdev_regulators_remove_bus,
> > ...
> > }
> >
> > does it explicitly want this feature. Without doing this, every PCI
> > port in the world will execute a devm_kzalloc() and
> > devm_regulator_bulk_get() to (likely) grab nothing, and then there
> > will be three superfluous lines in the boot log:
>
> You can opt-in based on there being a DT node.
Will do, thanks.
>
> > pci_bus 0001:01: 0001:01 supply vpcie12v not found, using dummy regulator
> > pci_bus 0001:01: 0001:01 supply vpcie3v3 not found, using dummy regulator
> > pci_bus 0001:01: 0001:01 supply vpcie3v3aux not found, using dummy regulator
>
> This would be annoying, but not really a reason for how to design this.
>
> > Secondly, our HW needs to know when the alloc/get/enable of
> > regulators is done so that the PCIe link can then be attempted. This
> > is pretty much the cornerstone of this patchset. To do this the brcm
> > RC driver's call to pci_subdev_regulators_add_bus() is wrapped by
> > brcm_pcie_add_bus() so that we can do this:
> >
> > static struct pci_ops my_pcie_ops = {
> > .add_bus = brcm_pcie_add_bus , /* calls pci_subdev_regulators_add_bus() */
> > .remove_bus = pci_subdev_regulators_remove_bus,
>
> Do add_bus/remove_bus get called during resume/suspend? If not, how do
> you handle the link during resume?
No to the first question, and incorrectly to the second. As it turns
out, my suspend/resume test for this patchset was flawed and wasn't
testing what I thought it was. The suspend/resume is broken in V8.
Using pcie_port_device_resume_noirq() is not early enough to turn on
the regulators during resume. However, if I have my RC driver handle
only the suspend/resume of the regulators, all is fine and as an
added benefit the patchset has no modifications to
portdrv_{pci,core}.c. This is what I will submit for V9, assuming
there is one.
>
> Maybe there needs to be explicit hooks for link handling. Pali has
> been looking into this some.
I will reply to Pali's recent email now.
Regards,
Jim Quinlan
Broadcom STB
>
> Rob
next prev parent reply other threads:[~2021-11-17 14:47 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-10 22:14 [PATCH v8 0/8] PCI: brcmstb: have portdrv turn on sub-device power Jim Quinlan
2021-11-10 22:14 ` [PATCH v8 1/8] PCI: brcmstb: Change brcm_phy_stop() to return void Jim Quinlan
2021-11-11 21:57 ` Bjorn Helgaas
2021-11-15 20:56 ` Jim Quinlan
2021-11-16 20:40 ` Bjorn Helgaas
2021-11-10 22:14 ` [PATCH v8 2/8] dt-bindings: PCI: Correct brcmstb interrupts, interrupt-map Jim Quinlan
2021-11-10 22:14 ` [PATCH v8 3/8] dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators Jim Quinlan
2021-11-11 22:17 ` Bjorn Helgaas
2021-11-12 18:25 ` Jim Quinlan
2021-11-12 20:20 ` Bjorn Helgaas
2021-11-12 21:46 ` Rob Herring
2021-11-13 11:38 ` Pali Rohár
2021-11-10 22:14 ` [PATCH v8 4/8] PCI/portdrv: Create pcie_is_port_dev() func from existing code Jim Quinlan
2021-11-11 21:51 ` Florian Fainelli
2021-11-11 22:53 ` Rob Herring
2021-11-11 23:50 ` Krzysztof Wilczyński
2021-11-12 18:14 ` Jim Quinlan
2021-11-10 22:14 ` [PATCH v8 5/8] PCI/portdrv: add mechanism to turn on subdev regulators Jim Quinlan
2021-11-11 9:44 ` kernel test robot
2021-11-11 22:12 ` Bjorn Helgaas
2021-11-11 22:50 ` Rob Herring
2021-11-11 22:56 ` Rob Herring
2021-11-15 20:44 ` Jim Quinlan
2021-11-16 17:41 ` Rob Herring
2021-11-16 20:53 ` Pali Rohár
2021-11-17 15:14 ` Jim Quinlan
2021-11-17 15:45 ` Pali Rohár
2021-11-18 15:36 ` Jim Quinlan
2021-11-18 15:50 ` Pali Rohár
2021-11-17 14:46 ` Jim Quinlan [this message]
2021-11-11 23:38 ` Krzysztof Wilczyński
2021-11-15 20:26 ` Jim Quinlan
2021-11-10 22:14 ` [PATCH v8 6/8] PCI/portdrv: Do not turn off subdev regulators if EP can wake up Jim Quinlan
2021-11-10 22:14 ` [PATCH v8 7/8] PCI: brcmstb: Split brcm_pcie_setup() into two funcs Jim Quinlan
2021-11-10 22:14 ` [PATCH v8 8/8] PCI: brcmstb: Add control of subdevice voltage regulators 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='CA+-6iNy7-CjQpua9tjMt=bLCanbTqRHPkwdKM2AH7HU1a1BQzg@mail.gmail.com' \
--to=james.quinlan@broadcom.com \
--cc=Jonathan.Cameron@huawei.com \
--cc=bcm-kernel-feedback-list@broadcom.com \
--cc=bhelgaas@google.com \
--cc=broonie@kernel.org \
--cc=jim2101024@gmail.com \
--cc=kbusch@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=nsaenz@kernel.org \
--cc=pali@kernel.org \
--cc=qiuxu.zhuo@intel.com \
--cc=robh@kernel.org \
--cc=sean.v.kelley@intel.com \
/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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).