Message-ID: <YXgBGWCOF3LVXcuC@sirena.org.uk>
Date:   Tue, 26 Oct 2021 14:22:33 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     Jim Quinlan <james.quinlan@...adcom.com>,
        Rob Herring <robh@...nel.org>,
        Jim Quinlan <jim2101024@...il.com>,
        "open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS" 
        <linux-pci@...r.kernel.org>,
        Nicolas Saenz Julienne <nsaenz@...nel.org>,
        "maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE" 
        <bcm-kernel-feedback-list@...adcom.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" 
        <linux-rpi-kernel@...ts.infradead.org>,
        "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" 
        <linux-arm-kernel@...ts.infradead.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 4/6] PCI: brcmstb: Add control of subdevice voltage
 regulators

On Mon, Oct 25, 2021 at 03:04:34PM -0700, Florian Fainelli wrote:
> On 10/25/21 7:58 AM, Mark Brown wrote:

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

This doesn't really sound terribly unusual frankly, which means it
shouldn't be insurmountable.  It also doesn't sound like a great
approach to base ABIs around.

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

If more than one board needs the same accomodation, for example if it's
common for a reset line to be GPIO controlled, then a common property
could be used by many different boards rather than requiring each
individual board to have board specific code.  This is on some level
what most DT properties boil down to.

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

I'm fairly sure that for example phone vendors pay a bit of attention to
power consumption.

> > 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?

The binding for a consumer should reflect what's going into that
consumer, this means that if you have 12V and 3.3V supplies then the
device should have two distinct supplies for that.  The device binding
should not change based on how those supplies are provided or any
relationship between the supplies outside the device, there should
definitely be no reason to define any new supplies for this purpose -
that would reflect a fundamental misunderstanding of the abstractions in
the API.

If (as it sounds) you've got systems with two supplies with GPIO enables
controlled by a single GPIO then you should just describe that directly
in device tree, this is quite common and there is support in there
already for identifying and reference counting the shared GPIO in that
case.

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

Well, it's a bit of a shared responsibility where the thing being
provided is a standards conforming connector rather than there being an
embedded device with much more potential for variation.

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

I would expect it's possible to make a PCI binding for a standard
physical layer bus connection as part of the generic PCI bindings, for
example by using some standard invalid ID for the device ID that can't
exist in a physical system or just omitting the device ID.  That seems
like a fairly clear case of being able to describe actual hardware that
physically exists - you can see the physical socket on the board.

> > In any case whatever gets done needs to be documented in the bindings
> > documents.

> Is not that what patch #1 does?

It just updated the example, it didn't document any new properties.  The
standard supplies are documented in the patch to the core standard that
was referenced so they're fine but not the extra Broadcom specific ones
that I've raised concerns with.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists