[V3,2/6] dt-bindings: PCI: tegra: Add PCIe slot supplies regulator entries
diff mbox series

Message ID 20190828172850.19871-3-vidyas@nvidia.com
State Not Applicable
Headers show
Series
  • PCI: tegra: Enable PCIe C5 controller of Tegra194 in p2972-0000 platform
Related show

Checks

Context Check Description
robh/checkpatch success

Commit Message

Vidya Sagar Aug. 28, 2019, 5:28 p.m. UTC
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@nvidia.com>
---
V3:
* None

V2:
* None

 .../devicetree/bindings/pci/nvidia,tegra194-pcie.txt      | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Thierry Reding Aug. 29, 2019, 12:03 p.m. UTC | #1
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@nvidia.com>
> ---
> 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?

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
>
Vidya Sagar Aug. 29, 2019, 3:18 p.m. UTC | #2
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@nvidia.com>
>> ---
>> 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
>>
Thierry Reding Aug. 29, 2019, 4:41 p.m. UTC | #3
On Thu, Aug 29, 2019 at 08:48:39PM +0530, Vidya Sagar wrote:
> 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@nvidia.com>
> > > ---
> > > 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.

Okay. That sounds like a good reason to make these optional indeed.
There is no way for the PCI controller to know exactly which regulators
will be needed. The only case where it is known is that of the regular
PCIe slot where the 3.3 V and 12 V are mandatory. But since it isn't
always a standard PCIe slot that the controller drives, I think optional
is okay in this case.

Thierry

> > 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
> > > 
>
Andrew Murray Sept. 2, 2019, 10:41 a.m. UTC | #4
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@nvidia.com>

Reviewed-by: Andrew Murray <andrew.murray@arm.com>

> ---
> 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).
>  
>  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
>
Thierry Reding Sept. 2, 2019, 11:38 a.m. UTC | #5
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@nvidia.com>
> ---
> V3:
> * None
> 
> V2:
> * None
> 
>  .../devicetree/bindings/pci/nvidia,tegra194-pcie.txt      | 8 ++++++++
>  1 file changed, 8 insertions(+)

Acked-by: Thierry Reding <treding@nvidia.com>
Rob Herring Sept. 2, 2019, 1:38 p.m. UTC | #6
On Wed, 28 Aug 2019 22:58:46 +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@nvidia.com>
> ---
> V3:
> * None
> 
> V2:
> * None
> 
>  .../devicetree/bindings/pci/nvidia,tegra194-pcie.txt      | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Patch
diff mbox series

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