On Fri, Oct 22, 2021 at 10:34 AM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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
