Re: [PATCH V3 2/6] dt-bindings: PCI: tegra: Add PCIe slot supplies regulator entries

[Vidya Sagar]

On 8/29/2019 5:33 PM, Thierry Reding wrote:
> On Wed, Aug 28, 2019 at 10:58:46PM +0530, Vidya Sagar wrote:
> > Add optional bindings "vpcie3v3-supply" and "vpcie12v-supply" to describe
> > regulators of a PCIe slot's supplies 3.3V and 12V provided the platform
> > is designed to have regulator controlled slot supplies.
> >
> > Signed-off-by: Vidya Sagar <vidyas@xxxxxxxxxx>
> > ---
> > V3:
> > * None
> >
> > V2:
> > * None
> >
> >   .../devicetree/bindings/pci/nvidia,tegra194-pcie.txt      | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.txt b/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.txt
> > index 0ac1b867ac24..b739f92da58e 100644
> > --- a/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.txt
> > +++ b/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.txt
> > @@ -104,6 +104,12 @@ Optional properties:
> >      specified in microseconds
> >   - nvidia,aspm-l0s-entrance-latency-us: ASPM L0s entrance latency to be
> >      specified in microseconds
> > +- vpcie3v3-supply: A phandle to the regulator node that supplies 3.3V to the slot
> > +  if the platform has one such slot. (Ex:- x16 slot owned by C5 controller
> > +  in p2972-0000 platform).
> > +- vpcie12v-supply: A phandle to the regulator node that supplies 12V to the slot
> > +  if the platform has one such slot. (Ex:- x16 slot owned by C5 controller
> > +  in p2972-0000 platform).
>
> There's an ongoing discussion regarding the use of optional power
> supplies and I'm wondering if we're not abusing this here. Why exactly
> are these regulators optional?
I made them optional because, the number and type of supplies typically depend on the
kind of slot the controller is owning. If it is a CEM slot, then, it needs 3.3V & 12V
supplies and if it is an M.2 Key-E/M slot, it needs only 3.3V supply. Also, if there are
on-board PCIe endpoint devices, supplies may vary again from vendor to vendor.
Considering all these, I made them optional instead of mandatory.
Also, I agree that regulator framework supplies a dummy regulator if we make them mandatory
but doesn't supply one, but it does so with a warning print in the log which I feel is
an unwanted alert and to avoid that one has to supply dummy/fixed regulators which again
seems an overkill when all of this can be addressed by making slot regulators optional.

> The distinction is somewhat subtle, but the other way to look at
> modelling this in DT is that the supplies are in fact required, but may
> be connected to an always-on regulator with a fixed voltage. Or in some
> cases they may also be shorted to ground. In both cases the PCI
> controller, or rather the slot that the controller connects to, actually
> "requires" the supplies, it's just that we can get away without
> describing them because they can't be controlled anyway.
>
> Looking at the PCI connector pinout for PCI Express, I do see a bunch of
> +3.3 V and +12 V pins. To me that indicates that the 3.3 V and 12 V
> supplies are indeed required for PCI slots. I'm not sure about devices
> that are directly connected to the PCI controller, though. I'll need to
> go look at some schematics to get a better understanding of these.
>
> Bottom line: I'm wondering if we shouldn't really make these supplies
> mandatory and in case where we don't care either just leave them away
> (the regulator framework will supply a dummy regulator in that case) or
> hook them up to a fixed regulator if that matches the hardware design.
>
> Any thoughts?
>
> Thierry
>
> >   
> >   Examples:
> >   =========
> > @@ -156,6 +162,8 @@ Tegra194:
> >   			  0xc2000000 0x18 0x00000000 0x18 0x00000000 0x4 0x00000000>;  /* prefetchable memory (16GB) */
> >   
> >   		vddio-pex-ctl-supply = <&vdd_1v8ao>;
> > +		vpcie3v3-supply = <&vdd_3v3_pcie>;
> > +		vpcie12v-supply = <&vdd_12v_pcie>;
> >   
> >   		phys = <&p2u_hsio_2>, <&p2u_hsio_3>, <&p2u_hsio_4>,
> >   		       <&p2u_hsio_5>;
> > --
> > 2.17.1