From: Jim Quinlan <james.quinlan@broadcom.com>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
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>,
Rob Herring <robh@kernel.org>, Mark Brown <broonie@kernel.org>,
"maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE"
<bcm-kernel-feedback-list@broadcom.com>,
"Rafael J. Wysocki" <rafael@kernel.org>,
Bjorn Helgaas <bhelgaas@google.com>,
Saravana Kannan <saravanak@google.com>,
Thomas Gleixner <tglx@linutronix.de>,
Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
Rajat Jain <rajatja@google.com>,
Bhaskar Chowdhury <unixbhaskar@gmail.com>,
Claire Chang <tientzu@chromium.org>,
Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v5 2/6] PCI: allow for callback to prepare nascent subdev
Date: Fri, 22 Oct 2021 11:01:01 -0400 [thread overview]
Message-ID: <CA+-6iNxPN0+Ge5AES-pChQvjM9OKeT__fy-nrE5obwJ0QcY1vw@mail.gmail.com> (raw)
In-Reply-To: <YXLL8IPz2LcNqEj4@kroah.com>
On Fri, Oct 22, 2021 at 10:34 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, Oct 22, 2021 at 10:06:55AM -0400, Jim Quinlan wrote:
> > We would like the Broadcom STB PCIe root complex driver 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:
> >
> > 1. For example, one customer uses multiple PCIe controllers, but wants the
> > ability to, by script, 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.
> >
> > This commit solves the "chicken-and-egg" problem by providing a callback to
> > the RC driver when a downstream device is "discovered" by inspecting its
> > DT, and allowing said driver to allocate the device object prematurely in
> > order to get the regulator(s) and turn them on before the device's ID is
> > read.
> >
> > [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/base/core.c | 5 +++++
> > drivers/pci/probe.c | 47 ++++++++++++++++++++++++++++++++----------
> > include/linux/device.h | 3 +++
> > include/linux/pci.h | 3 +++
> > 4 files changed, 47 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 249da496581a..62d9ac123ae5 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -2864,6 +2864,10 @@ static void klist_children_put(struct klist_node *n)
> > */
> > void device_initialize(struct device *dev)
> > {
> > + /* Return if this has been called already. */
> > + if (dev->device_initialized)
> > + return;
> > +
>
> Ick, no! Who would ever be calling this function more than once? That
> "should" be impossible.
>
> This function should only be called by a bus when it first creates the
> structure and before anything is done with it. As the bus itself
> controls the creation, it already "knows" when the structure was
> initialzed so it should not have to be called again.
>
> Please fix the bus logic that requires this, it is broken.
Got it, thanks for the prompt reply.
JimQ
>
> Consider this a NACK for this patch, sorry.
>
> greg k-h
next prev parent reply other threads:[~2021-10-22 15:01 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-22 14:06 [PATCH v5 0/6] PCI: brcmstb: have host-bridge turn on sub-device power Jim Quinlan
2021-10-22 14:06 ` [PATCH v5 1/6] dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators Jim Quinlan
2021-10-22 14:49 ` Mark Brown
2021-10-22 19:24 ` Jim Quinlan
2021-10-22 19:49 ` Mark Brown
2021-10-22 21:15 ` Rob Herring
2021-10-25 22:24 ` Rob Herring
2021-10-26 21:27 ` Jim Quinlan
2021-10-27 16:58 ` Bjorn Helgaas
2021-10-27 21:37 ` Pali Rohár
2021-10-27 20:54 ` Rob Herring
2021-10-27 21:31 ` Jim Quinlan
2021-10-22 14:06 ` [PATCH v5 2/6] PCI: allow for callback to prepare nascent subdev Jim Quinlan
2021-10-22 14:34 ` Greg Kroah-Hartman
2021-10-22 15:01 ` Jim Quinlan [this message]
2021-10-22 14:06 ` [PATCH v5 3/6] PCI: brcmstb: split brcm_pcie_setup() into two funcs Jim Quinlan
2021-10-22 14:06 ` [PATCH v5 4/6] PCI: brcmstb: Add control of subdevice voltage regulators Jim Quinlan
2021-10-22 14:31 ` Mark Brown
2021-10-22 19:15 ` Jim Quinlan
2021-10-22 19:47 ` Mark Brown
2021-10-25 13:50 ` Jim Quinlan
2021-10-25 14:58 ` Mark Brown
2021-10-25 22:04 ` Florian Fainelli
2021-10-25 22:43 ` Rob Herring
2021-10-25 22:51 ` Florian Fainelli
2021-10-26 13:22 ` Mark Brown
2021-10-25 22:16 ` Rob Herring
2021-10-22 14:06 ` [PATCH v5 5/6] PCI: brcmstb: Do not turn off regulators if EP can wake up Jim Quinlan
2021-10-22 14:39 ` Mark Brown
2021-10-22 15:06 ` Jim Quinlan
2021-10-22 14:06 ` [PATCH v5 6/6] PCI: brcmstb: change brcm_phy_stop() to return void 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+-6iNxPN0+Ge5AES-pChQvjM9OKeT__fy-nrE5obwJ0QcY1vw@mail.gmail.com \
--to=james.quinlan@broadcom.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=bcm-kernel-feedback-list@broadcom.com \
--cc=bhelgaas@google.com \
--cc=broonie@kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=jim2101024@gmail.com \
--cc=konrad.wilk@oracle.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=nsaenz@kernel.org \
--cc=rafael@kernel.org \
--cc=rajatja@google.com \
--cc=robh@kernel.org \
--cc=saravanak@google.com \
--cc=tglx@linutronix.de \
--cc=tientzu@chromium.org \
--cc=unixbhaskar@gmail.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).