Thread (28 messages) 28 messages, 7 authors, 2021-10-27

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

From: Florian Fainelli <f.fainelli@gmail.com>
Date: 2021-10-25 22:52:36
Also in: linux-pci, lkml

On 10/25/21 3:43 PM, Rob Herring wrote:
On Mon, Oct 25, 2021 at 03:04:34PM -0700, Florian Fainelli wrote:
quoted
On 10/25/21 7:58 AM, Mark Brown wrote:
quoted
On Mon, Oct 25, 2021 at 09:50:09AM -0400, Jim Quinlan wrote:
quoted
On Fri, Oct 22, 2021 at 3:47 PM Mark Brown [off-list ref] wrote:
quoted
On Fri, Oct 22, 2021 at 03:15:59PM -0400, Jim Quinlan wrote:
quoted
quoted
That sounds like it just shouldn't be a regulator at all, perhaps the
board happens to need a regulator there but perhaps it needs a clock,
GPIO or some specific sequence of actions.  It sounds like you need some
sort of quirking mechanism to cope with individual boards with board
specific bindings.
quoted
The boards involved may have no PCIe sockets, or run the gamut of the different
PCIe sockets.  They all offer gpio(s) to turn off/on their power supply(s) to
make their PCIe device endpoint functional.  It is not viable to add
new Linux quirk or DT
code for each board.  First is the volume and variety of the boards
that use our SOCs.. Second, is
our lack of information/control:  often, the board is designed by one
company (not us), and
given to another company as the middleman, and then they want the
features outlined
in my aforementioned commit message.
Other vendors have plenty of variation in board design yet we still have
device trees that describe the hardware, I can't see why these systems
should be so different.  It is entirely normal for system integrators to
collaborate on this and even upstream their own code, this happens all
the time, there is no need for everything to be implemented directly the
SoC vendor.
This is all well and good and there is no disagreement here that it
should just be that way but it does not reflect what Jim and I are
confronted with on a daily basis. We work in a tightly controlled
environment using a waterfall approach, whatever we come up with as a
SoC vendor gets used usually without further modification by the OEMs,
when OEMs do change things we have no visibility into anyway.

We have a boot loader that goes into great lengths to tailor the FDT
blob passed to the kernel to account for board-specific variations, PCIe
voltage regulators being just one of those variations. This is usually
how we quirk and deal with any board specific details, so I fail to
understand what you mean by "quirks that match a common pattern".

Also, I don't believe other vendors are quite as concerned with
conserving power as we are, it could be that they are just better at it
through different ways, or we have a particular sensitivity to the subject.
quoted
If there are generic quirks that match a common pattern seen in a lot of
board then properties can be defined for those, this is in fact the
common case.  This is no reason to just shove in some random thing that
doesn't describe the hardware, that's a great way to end up stuck with
an ABI that is fragile and difficult to understand or improve.
I would argue that at least 2 out of the 4 supplies proposed do describe
hardware signals. The latter two are more abstract to say the least,
however I believe it is done that way because they are composite
supplies controlling both the 12V and 3.3V supplies coming into a PCIe
device (Jim is that right?). So how do we call the latter supply then?
vpcie12v3v...-supply?
quoted
Potentially some of these things should be being handled in the bindings
and drivers drivers for the relevant PCI devices rather than in the PCI
controller.
The description and device tree binding can be and should be in a PCI
device binding rather than pci-bus.yaml.

The handling however goes back to the chicken and egg situation that we
talked about multiple times before: no supply -> no link UP -> no
enumeration -> no PCI device, therefore no driver can have a chance to
control the regulator. These are not hotplug capable systems by the way,
but even if they were, we would still run into the same problem. Given
that most reference boards do have mechanical connectors that people can
plug random devices into, we cannot provide a compatible string
containing the PCI vendor/device ID ahead of time because we don't know
what will be plugged in. 
I thought you didn't have connectors or was it just they are 
non-standard? If the latter case, what are the supply rails for the 
connector?
We now have reference boards with full-sized x1 and x4 connectors in
addition to half sized and full-sized mini-PCIe connectors and the
soldered down Wi-Fi on board (WOMBO) and the Multi-chip Module packages
(MCM).

When using connectors we would use the standard PCIe pinout nomenclature
however for the latter two, there appears to be a variety of
non-standard stuff being done there. We reviewed some schematics with
Jim and it looks like some of the usages for the regulators are just
laziness on the Wi-Fi driver side, like asking the kernel to keep the
radio on (PA, LNA etc.) as if it was as critical and necessary as the
12V and 3.3V supplies that actually power on the PCIe end-point... We
will get those fixed hopefully.
I'd be okay if there's no compatible as long as there's not a continual 
stream of DT properties trying to describe power sequencing 
requirements.
Have not looked whether Dmitry's power sequencing generalizing is
helping us here:

https://www.spinics.net/lists/linux-bluetooth/msg93564.html

it still looks like the PCIe host controller is involved in requesting
the power sequence.
quoted
In the case of a MCM, we would, but then we
only solved about 15% of the boards we need to support, so we have not
really progressed much.
MCM is multi-chip module?
Correct, see above.
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help