Re: [PATCH v3 2/6] PCI: brcmstb: Add control of EP voltage regulators

From: Mark Brown <broonie@kernel.org>
To: Jim Quinlan <jim2101024@gmail.com>
Cc: linux-pci <linux-pci@vger.kernel.org>,
	Nicolas Saenz Julienne <nsaenzjulienne@suse.de>,
	Rob Herring <robh@kernel.org>,
	bcm-kernel-feedback-list <bcm-kernel-feedback-list@broadcom.com>,
	Jim Quinlan <james.quinlan@broadcom.com>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Florian Fainelli <f.fainelli@gmail.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 v3 2/6] PCI: brcmstb: Add control of EP voltage regulators
Date: Mon, 29 Mar 2021 21:45:43 +0100	[thread overview]
Message-ID: <20210329204543.GJ5166@sirena.org.uk> (raw)
In-Reply-To: <CANCKTBvwWdVgjgTf620KqaAyyMwPkRgO3FHOqs_Gen+bnYTJFw@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2539 bytes --]

On Mon, Mar 29, 2021 at 03:48:46PM -0400, Jim Quinlan wrote:

> I'm not concerned about a namespace collision and I don't think you
> should be concerned either.  First, this driver is for Broadcom STB
> PCIe chips and boards, and we also deliver the DT to the customers.
> We typically do not have any other regulators in the DT besides the
> ones I am proposing.  For example, the 7216 SOC DT has 0 other

You may not describe these regulators in the DT but you must have other
regulators in your system, most devices need power to operate.  In any
case "this works for me with my DT on my system and nobody will ever
change our reference design" isn't really a great approach, frankly it's
not a claim I entirely believe and even if it turns out to be true for
your systems if we establish this as being how regulators work for
soldered down PCI devices everyone else is going to want to do the same
thing, most likely making the same claims for how much control they have
over the systems things will run on.

> regulators -- no namespace collision possible.  Our DT-generating
> scripts also flag namespace issues.  I admit that this driver is also
> used by RPi chips, but I can easily exclude this feature from the RPI
> if Nicolas has any objection.

That's certainly an issue, obviously the RPI is the sort of system where
I'd imagine this would be particularly useful.

> Further, if you want, I can restrict the search to the two regulators
> I am proposing to add to pci-bus.yaml:  "vpcie12v-supply" and
> "vpcie3v3-supply".

No, that doesn't help - what happens if someone uses separate regulators
for different PCI devices?  The reason the supplies are device namespaced
is that each device can look up it's own supplies and label them how it
wants without reference to anything else on the board.  Alternatively
what happens if some device has another supply it needs to power on (eg,
something that wants a clean LDO output for analogue use)?

> Is the above enough to alleviate your concerns about global namespace collision?

Not really.  TBH it looks like this driver is keeping the regulators
enabled all the time except for suspend and resume anyway, if that's all
that's going on I'd have thought that you wouldn't need any explicit
management in the driver anyway?  Just mark the regualtors as always on
and set up an appropriate suspend mode configuration and everything
should work without the drivers doing anything.  Unless your PMIC isn't
able to provide separate suspend mode configuration for the regulators?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]