Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211022140714.28767-1-jim2101024@gmail.com> <20211022140714.28767-3-jim2101024@gmail.com>
 <YXLL8IPz2LcNqEj4@kroah.com>
In-Reply-To: <YXLL8IPz2LcNqEj4@kroah.com>
From:   Jim Quinlan <james.quinlan@broadcom.com>
Date:   Fri, 22 Oct 2021 11:01:01 -0400
Message-ID: <CA+-6iNxPN0+Ge5AES-pChQvjM9OKeT__fy-nrE5obwJ0QcY1vw@mail.gmail.com>
Subject: Re: [PATCH v5 2/6] PCI: allow for callback to prepare nascent subdev
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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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