Re: [PATCH v5 4/6] PCI: brcmstb: Add control of subdevice voltage regulators

From: Jim Quinlan <james.quinlan@broadcom.com>
To: Mark Brown <broonie@kernel.org>, Rob Herring <robh@kernel.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>,
	"maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE"
	<bcm-kernel-feedback-list@broadcom.com>,
	"Florian Fainelli" <f.fainelli@gmail.com>,
	"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Bjorn Helgaas" <bhelgaas@google.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 v5 4/6] PCI: brcmstb: Add control of subdevice voltage regulators
Date: Fri, 22 Oct 2021 15:15:59 -0400	[thread overview]
Message-ID: <CA+-6iNzmkB5sUL6aqA6229BhxBhF3RKvGsLh0JCYQwP_2wSGaQ@mail.gmail.com> (raw)
In-Reply-To: <YXLLRLwMG7nEwQoi@sirena.org.uk>

On Fri, Oct 22, 2021 at 10:31 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Oct 22, 2021 at 10:06:57AM -0400, Jim Quinlan wrote:
>
> > +static const char * const supplies[] = {
> > +     "vpcie3v3-supply",
> > +     "vpcie3v3aux-supply",
> > +     "brcm-ep-a-supply",
> > +     "brcm-ep-b-supply",
> > +};
>
> Why are you including "-supply" in the names here?  That will lead to
> a double -supply when we look in the DT which probably isn't what you're
> looking for.
I'm not sure how this got past testing; will fix.

>
> Also are you *sure* that the device has supplies with names like
> "brcm-ep-a"?  That seems rather unidiomatic for electrical engineering,
> the names here are supposed to correspond to the names used in the
> datasheet for the part.
I try to explain this in the commit message of"PCI: allow for callback
to prepare nascent subdev".  Wrt to the names,

       "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."

Each different SOC./board we deal with may present different ways of
making the EP device power on.  We are using
an abstraction name "brcm-ep-a"  to represent some required regulator
to make the EP  work for a specific board.  The RC
driver cannot hard code a descriptive name as it must work for all
boards designed by us, others, and third parties.
The EP driver also doesn't know  or care about the regulator name, and
this driver is often closed source and often immutable.  The EP
device itself may come from Brcm, a third party,  or sometimes a competitor.

Basically, we find using a generic name such as "brcm-ep-a-supply"
quite handy and many of our customers embrace this feature.
I know that Rob was initially against such a generic name, but I
vaguely remember him seeing some merit to this, perhaps a tiny bit :-)
Or my memory is shot, which could very well be the case.

>
> > +     /* This is for Broadcom STB/CM chips only */
> > +     if (pcie->type == BCM2711)
> > +             return 0;
>
> It is a relief that other chips have managed to work out how to avoid
> requiring power.
I'm not sure that the other Broadcom groups have our customers, our
customers' requirements, and the amount and variation of boards that
run our PCIe driver on the SOC.

Jim