[PATCH 0/6] PCI: tegra: Enable PCIe C5 controller of Tegra194 in p2972-0000 platform

[PATCH 0/6] PCI: tegra: Enable PCIe C5 controller of Tegra194 in p2972-0000 platform

[Vidya Sagar]

This patch series enables Tegra194's C5 controller which owns x16 slot in
p2972-0000 platform. C5 controller's PERST# and CLKREQ# are not configured as
output and bi-directional signals by default and hence they need to be
configured explicitly. Also, x16 slot's 3.3V and 12V supplies are controlled
through GPIOs and hence they need to be enabled through regulator framework.
This patch series adds required infrastructural support to address both the
aforementioned requirements.
Testing done on p2972-0000 platform
- Able to enumerate devices connected to x16 slot (owned by C5 controller)
- Enumerated device's functionality verified
- Suspend-Resume sequence is verified with device connected to x16 slot

Vidya Sagar (6):
  dt-bindings: PCI: tegra: Add sideband pins configuration entries
  arm64: tegra: Add configuration for PCIe C5 sideband signals
  PCI: tegra: Add support to configure sideband pins
  dt-bindings: PCI: tegra: Add PCIe slot supplies regulator entries
  arm64: tegra: Add PCIe slot supply information in p2972-0000 platform
  PCI: tegra: Add support to enable slot regulators

 .../bindings/pci/nvidia,tegra194-pcie.txt     | 16 +++++
 .../arm64/boot/dts/nvidia/tegra194-p2888.dtsi | 24 +++++++
 .../boot/dts/nvidia/tegra194-p2972-0000.dts   |  4 +-
 arch/arm64/boot/dts/nvidia/tegra194.dtsi      | 38 +++++++++-
 drivers/pci/controller/dwc/pcie-tegra194.c    | 71 +++++++++++++++++++
 5 files changed, 151 insertions(+), 2 deletions(-)

-- 
2.17.1

[PATCH 1/6] dt-bindings: PCI: tegra: Add sideband pins configuration entries

[Vidya Sagar]

Add optional bindings "pinctrl-names" and "pinctrl-0" to describe pin
configuration information of a particular PCIe controller.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
 .../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 674e5adb2895..0ac1b867ac24 100644
--- a/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.txt
@@ -83,6 +83,11 @@ Required properties:
 - vddio-pex-ctl-supply: Regulator supply for PCIe side band signals
 
 Optional properties:
+- pinctrl-names: A list of pinctrl state names.
+  It is mandatory for C5 controller and optional for other controllers.
+  - "default": Configures PCIe I/O for proper operation.
+- pinctrl-0: phandle for the 'default' state of pin configuration.
+  It is mandatory for C5 controller and optional for other controllers.
 - supports-clkreq: Refer to Documentation/devicetree/bindings/pci/pci.txt
 - nvidia,update-fc-fixup: This is a boolean property and needs to be present to
     improve performance when a platform is designed in such a way that it
@@ -120,6 +125,9 @@ Tegra194:
 		num-lanes = <8>;
 		linux,pci-domain = <0>;
 
+		pinctrl-names = "default";
+		pinctrl-0 = <&pex_rst_c5_out_state>, <&clkreq_c5_bi_dir_state>;
+
 		clocks = <&bpmp TEGRA194_CLK_PEX0_CORE_0>;
 		clock-names = "core";
 
-- 
2.17.1

[PATCH 2/6] arm64: tegra: Add configuration for PCIe C5 sideband signals

[Vidya Sagar]

Add support to configure PCIe C5's sideband signals PERST# and CLKREQ#
as output and bi-directional signals respectively which unlike other
PCIe controllers sideband signals are not configured by default.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
 arch/arm64/boot/dts/nvidia/tegra194.dtsi | 38 +++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/nvidia/tegra194.dtsi b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
index adebbbf36bd0..3c0cf54f0aab 100644
--- a/arch/arm64/boot/dts/nvidia/tegra194.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
@@ -3,8 +3,9 @@
 #include <dt-bindings/gpio/tegra194-gpio.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/mailbox/tegra186-hsp.h>
-#include <dt-bindings/reset/tegra194-reset.h>
+#include <dt-bindings/pinctrl/pinctrl-tegra.h>
 #include <dt-bindings/power/tegra194-powergate.h>
+#include <dt-bindings/reset/tegra194-reset.h>
 #include <dt-bindings/thermal/tegra194-bpmp-thermal.h>
 
 / {
@@ -130,6 +131,38 @@
 			};
 		};
 
+		pinmux: pinmux@2430000 {
+			compatible = "nvidia,tegra194-pinmux";
+			reg = <0x2430000 0x17000
+			       0xc300000 0x4000>;
+
+			status = "okay";
+
+			pex_rst_c5_out_state: pex_rst_c5_out {
+				pex_rst {
+					nvidia,pins = "pex_l5_rst_n_pgg1";
+					nvidia,schmitt = <TEGRA_PIN_DISABLE>;
+					nvidia,lpdr = <TEGRA_PIN_ENABLE>;
+					nvidia,enable-input = <TEGRA_PIN_DISABLE>;
+					nvidia,io-high-voltage = <TEGRA_PIN_ENABLE>;
+					nvidia,tristate = <TEGRA_PIN_DISABLE>;
+					nvidia,pull = <TEGRA_PIN_PULL_NONE>;
+				};
+			};
+
+			clkreq_c5_bi_dir_state: clkreq_c5_bi_dir {
+				clkreq {
+					nvidia,pins = "pex_l5_clkreq_n_pgg0";
+					nvidia,schmitt = <TEGRA_PIN_DISABLE>;
+					nvidia,lpdr = <TEGRA_PIN_ENABLE>;
+					nvidia,enable-input = <TEGRA_PIN_ENABLE>;
+					nvidia,io-high-voltage = <TEGRA_PIN_ENABLE>;
+					nvidia,tristate = <TEGRA_PIN_DISABLE>;
+					nvidia,pull = <TEGRA_PIN_PULL_NONE>;
+				};
+			};
+		};
+
 		uarta: serial@3100000 {
 			compatible = "nvidia,tegra194-uart", "nvidia,tegra20-uart";
 			reg = <0x03100000 0x40>;
@@ -1365,6 +1398,9 @@
 		num-viewport = <8>;
 		linux,pci-domain = <5>;
 
+		pinctrl-names = "default";
+		pinctrl-0 = <&pex_rst_c5_out_state>, <&clkreq_c5_bi_dir_state>;
+
 		clocks = <&bpmp TEGRA194_CLK_PEX1_CORE_5>,
 			<&bpmp TEGRA194_CLK_PEX1_CORE_5M>;
 		clock-names = "core", "core_m";
-- 
2.17.1

[PATCH 3/6] PCI: tegra: Add support to configure sideband pins

[Vidya Sagar]

Add support to configure sideband signal pins when information is present
in respective controller's device-tree node.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
 drivers/pci/controller/dwc/pcie-tegra194.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
index fc0dbeb31d78..8a27b25893c9 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -1308,6 +1308,12 @@ static int tegra_pcie_config_rp(struct tegra_pcie_dw *pcie)
 		return ret;
 	}
 
+	ret = pinctrl_pm_select_default_state(pcie->dev);
+	if (ret < 0) {
+		dev_err(pcie->dev, "Failed to configure sideband pins\n");
+		return ret;
+	}
+
 	tegra_pcie_init_controller(pcie);
 
 	pcie->link_state = tegra_pcie_dw_link_up(&pcie->pci);
-- 
2.17.1

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

[Vidya Sagar]

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

[PATCH 5/6] arm64: tegra: Add PCIe slot supply information in p2972-0000 platform

[Vidya Sagar]

Add 3.3V and 12V supplies regulators information of x16 PCIe slot in
p2972-0000 platform which is owned by C5 controller and also enable C5
controller.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
 .../arm64/boot/dts/nvidia/tegra194-p2888.dtsi | 24 +++++++++++++++++++
 .../boot/dts/nvidia/tegra194-p2972-0000.dts   |  4 +++-
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/nvidia/tegra194-p2888.dtsi b/arch/arm64/boot/dts/nvidia/tegra194-p2888.dtsi
index 62e07e1197cc..4c38426a6969 100644
--- a/arch/arm64/boot/dts/nvidia/tegra194-p2888.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra194-p2888.dtsi
@@ -289,5 +289,29 @@
 			gpio = <&gpio TEGRA194_MAIN_GPIO(A, 3) GPIO_ACTIVE_HIGH>;
 			enable-active-high;
 		};
+
+		vdd_3v3_pcie: regulator@2 {
+			compatible = "regulator-fixed";
+			reg = <2>;
+
+			regulator-name = "PEX_3V3";
+			regulator-min-microvolt = <3300000>;
+			regulator-max-microvolt = <3300000>;
+			gpio = <&gpio TEGRA194_MAIN_GPIO(Z, 2) GPIO_ACTIVE_HIGH>;
+			regulator-boot-on;
+			enable-active-high;
+		};
+
+		vdd_12v_pcie: regulator@3 {
+			compatible = "regulator-fixed";
+			reg = <3>;
+
+			regulator-name = "VDD_12V";
+			regulator-min-microvolt = <1200000>;
+			regulator-max-microvolt = <1200000>;
+			gpio = <&gpio TEGRA194_MAIN_GPIO(A, 1) GPIO_ACTIVE_LOW>;
+			regulator-boot-on;
+			enable-active-low;
+		};
 	};
 };
diff --git a/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts b/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
index 23597d53c9c9..d47cd8c4dd24 100644
--- a/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
+++ b/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
@@ -93,9 +93,11 @@
 	};
 
 	pcie@141a0000 {
-		status = "disabled";
+		status = "okay";
 
 		vddio-pex-ctl-supply = <&vdd_1v8ao>;
+		vpcie3v3-supply = <&vdd_3v3_pcie>;
+		vpcie12v-supply = <&vdd_12v_pcie>;
 
 		phys = <&p2u_nvhs_0>, <&p2u_nvhs_1>, <&p2u_nvhs_2>,
 		       <&p2u_nvhs_3>, <&p2u_nvhs_4>, <&p2u_nvhs_5>,
-- 
2.17.1

[PATCH 6/6] PCI: tegra: Add support to enable slot regulators

[Vidya Sagar]

Add support to get regulator information of 3.3V and 12V supplies of a PCIe
slot from the respective controller's device-tree node and enable those
supplies. This is required in platforms like p2972-0000 where the supplies
to x16 slot owned by C5 controller need to be enabled before attempting to
enumerate the devices.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
 drivers/pci/controller/dwc/pcie-tegra194.c | 65 ++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
index 8a27b25893c9..97de2151a738 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -278,6 +278,8 @@ struct tegra_pcie_dw {
 	u32 aspm_l0s_enter_lat;
 
 	struct regulator *pex_ctl_supply;
+	struct regulator *slot_ctl_3v3;
+	struct regulator *slot_ctl_12v;
 
 	unsigned int phy_count;
 	struct phy **phys;
@@ -1047,6 +1049,59 @@ static void tegra_pcie_downstream_dev_to_D0(struct tegra_pcie_dw *pcie)
 	}
 }
 
+static void tegra_pcie_get_slot_regulators(struct tegra_pcie_dw *pcie)
+{
+	pcie->slot_ctl_3v3 = devm_regulator_get_optional(pcie->dev, "vpcie3v3");
+	if (IS_ERR(pcie->slot_ctl_3v3))
+		pcie->slot_ctl_3v3 = NULL;
+
+	pcie->slot_ctl_12v = devm_regulator_get_optional(pcie->dev, "vpcie12v");
+	if (IS_ERR(pcie->slot_ctl_12v))
+		pcie->slot_ctl_12v = NULL;
+}
+
+static int tegra_pcie_enable_slot_regulators(struct tegra_pcie_dw *pcie)
+{
+	int ret;
+
+	if (pcie->slot_ctl_3v3) {
+		ret = regulator_enable(pcie->slot_ctl_3v3);
+		if (ret < 0) {
+			dev_err(pcie->dev,
+				"Failed to enable 3V3 slot supply: %d\n", ret);
+			return ret;
+		}
+	}
+
+	if (pcie->slot_ctl_12v) {
+		ret = regulator_enable(pcie->slot_ctl_12v);
+		if (ret < 0) {
+			dev_err(pcie->dev,
+				"Failed to enable 12V slot supply: %d\n", ret);
+			if (pcie->slot_ctl_3v3)
+				regulator_disable(pcie->slot_ctl_3v3);
+			return ret;
+		}
+	}
+
+	/*
+	 * According to PCI Express Card Electromechanical Specification
+	 * Revision 1.1, Table-2.4, T_PVPERL (Power stable to PERST# inactive)
+	 * should be a minimum of 100ms.
+	 */
+	msleep(100);
+
+	return 0;
+}
+
+static void tegra_pcie_disable_slot_regulators(struct tegra_pcie_dw *pcie)
+{
+	if (pcie->slot_ctl_12v)
+		regulator_disable(pcie->slot_ctl_12v);
+	if (pcie->slot_ctl_3v3)
+		regulator_disable(pcie->slot_ctl_3v3);
+}
+
 static int tegra_pcie_config_controller(struct tegra_pcie_dw *pcie,
 					bool en_hw_hot_rst)
 {
@@ -1060,6 +1115,10 @@ static int tegra_pcie_config_controller(struct tegra_pcie_dw *pcie,
 		return ret;
 	}
 
+	ret = tegra_pcie_enable_slot_regulators(pcie);
+	if (ret < 0)
+		goto fail_slot_reg_en;
+
 	ret = regulator_enable(pcie->pex_ctl_supply);
 	if (ret < 0) {
 		dev_err(pcie->dev, "Failed to enable regulator: %d\n", ret);
@@ -1142,6 +1201,8 @@ static int tegra_pcie_config_controller(struct tegra_pcie_dw *pcie,
 fail_core_clk:
 	regulator_disable(pcie->pex_ctl_supply);
 fail_reg_en:
+	tegra_pcie_disable_slot_regulators(pcie);
+fail_slot_reg_en:
 	tegra_pcie_bpmp_set_ctrl_state(pcie, false);
 
 	return ret;
@@ -1174,6 +1235,8 @@ static int __deinit_controller(struct tegra_pcie_dw *pcie)
 		return ret;
 	}
 
+	tegra_pcie_disable_slot_regulators(pcie);
+
 	ret = tegra_pcie_bpmp_set_ctrl_state(pcie, false);
 	if (ret) {
 		dev_err(pcie->dev, "Failed to disable controller %d: %d\n",
@@ -1372,6 +1435,8 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	tegra_pcie_get_slot_regulators(pcie);
+
 	pcie->pex_ctl_supply = devm_regulator_get(dev, "vddio-pex-ctl");
 	if (IS_ERR(pcie->pex_ctl_supply)) {
 		dev_err(dev, "Failed to get regulator: %ld\n",
-- 
2.17.1

Re: [PATCH 3/6] PCI: tegra: Add support to configure sideband pins

[Andrew Murray]

On Mon, Aug 26, 2019 at 01:01:40PM +0530, Vidya Sagar wrote:
> Add support to configure sideband signal pins when information is present
> in respective controller's device-tree node.
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
>  drivers/pci/controller/dwc/pcie-tegra194.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> index fc0dbeb31d78..8a27b25893c9 100644
> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> @@ -1308,6 +1308,12 @@ static int tegra_pcie_config_rp(struct tegra_pcie_dw *pcie)
>  		return ret;
>  	}
>  
> +	ret = pinctrl_pm_select_default_state(pcie->dev);
> +	if (ret < 0) {
> +		dev_err(pcie->dev, "Failed to configure sideband pins\n");

I think you can just use dev instead of pcie->dev here.

> +		return ret;

Don't you need to pm_runtime_put_sync and pm_runtime_disable here?

Thanks,

Andrew Murray

> +	}
> +
>  	tegra_pcie_init_controller(pcie);
>  
>  	pcie->link_state = tegra_pcie_dw_link_up(&pcie->pci);
> -- 
> 2.17.1
> 

Re: [PATCH 3/6] PCI: tegra: Add support to configure sideband pins

[Vidya Sagar]

On 8/27/2019 9:00 PM, Andrew Murray wrote:
> On Mon, Aug 26, 2019 at 01:01:40PM +0530, Vidya Sagar wrote:
>> Add support to configure sideband signal pins when information is present
>> in respective controller's device-tree node.
>>
>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>> ---
>>   drivers/pci/controller/dwc/pcie-tegra194.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
>> index fc0dbeb31d78..8a27b25893c9 100644
>> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
>> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
>> @@ -1308,6 +1308,12 @@ static int tegra_pcie_config_rp(struct tegra_pcie_dw *pcie)
>>   		return ret;
>>   	}
>>   
>> +	ret = pinctrl_pm_select_default_state(pcie->dev);
>> +	if (ret < 0) {
>> +		dev_err(pcie->dev, "Failed to configure sideband pins\n");
> 
> I think you can just use dev instead of pcie->dev here.
Yup. I can use just 'dev' here.

> 
>> +		return ret;
> 
> Don't you need to pm_runtime_put_sync and pm_runtime_disable here?
Yup. Thanks for catching it. I'll address it in next patch series.

> 
> Thanks,
> 
> Andrew Murray
> 
>> +	}
>> +
>>   	tegra_pcie_init_controller(pcie);
>>   
>>   	pcie->link_state = tegra_pcie_dw_link_up(&pcie->pci);
>> -- 
>> 2.17.1
>>

Re: [PATCH 6/6] PCI: tegra: Add support to enable slot regulators

[Andrew Murray]

On Mon, Aug 26, 2019 at 01:01:43PM +0530, Vidya Sagar wrote:
> Add support to get regulator information of 3.3V and 12V supplies of a PCIe
> slot from the respective controller's device-tree node and enable those
> supplies. This is required in platforms like p2972-0000 where the supplies
> to x16 slot owned by C5 controller need to be enabled before attempting to
> enumerate the devices.
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
>  drivers/pci/controller/dwc/pcie-tegra194.c | 65 ++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> index 8a27b25893c9..97de2151a738 100644
> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> @@ -278,6 +278,8 @@ struct tegra_pcie_dw {
>  	u32 aspm_l0s_enter_lat;
>  
>  	struct regulator *pex_ctl_supply;
> +	struct regulator *slot_ctl_3v3;
> +	struct regulator *slot_ctl_12v;
>  
>  	unsigned int phy_count;
>  	struct phy **phys;
> @@ -1047,6 +1049,59 @@ static void tegra_pcie_downstream_dev_to_D0(struct tegra_pcie_dw *pcie)
>  	}
>  }
>  
> +static void tegra_pcie_get_slot_regulators(struct tegra_pcie_dw *pcie)
> +{
> +	pcie->slot_ctl_3v3 = devm_regulator_get_optional(pcie->dev, "vpcie3v3");
> +	if (IS_ERR(pcie->slot_ctl_3v3))
> +		pcie->slot_ctl_3v3 = NULL;
> +
> +	pcie->slot_ctl_12v = devm_regulator_get_optional(pcie->dev, "vpcie12v");
> +	if (IS_ERR(pcie->slot_ctl_12v))
> +		pcie->slot_ctl_12v = NULL;

Do these need to take into consideration -EPROBE_DEFER?

Thanks,

Andrew Murray

> +}
> +
> +static int tegra_pcie_enable_slot_regulators(struct tegra_pcie_dw *pcie)
> +{
> +	int ret;
> +
> +	if (pcie->slot_ctl_3v3) {
> +		ret = regulator_enable(pcie->slot_ctl_3v3);
> +		if (ret < 0) {
> +			dev_err(pcie->dev,
> +				"Failed to enable 3V3 slot supply: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	if (pcie->slot_ctl_12v) {
> +		ret = regulator_enable(pcie->slot_ctl_12v);
> +		if (ret < 0) {
> +			dev_err(pcie->dev,
> +				"Failed to enable 12V slot supply: %d\n", ret);
> +			if (pcie->slot_ctl_3v3)
> +				regulator_disable(pcie->slot_ctl_3v3);
> +			return ret;
> +		}
> +	}
> +
> +	/*
> +	 * According to PCI Express Card Electromechanical Specification
> +	 * Revision 1.1, Table-2.4, T_PVPERL (Power stable to PERST# inactive)
> +	 * should be a minimum of 100ms.
> +	 */
> +	msleep(100);
> +
> +	return 0;
> +}
> +
> +static void tegra_pcie_disable_slot_regulators(struct tegra_pcie_dw *pcie)
> +{
> +	if (pcie->slot_ctl_12v)
> +		regulator_disable(pcie->slot_ctl_12v);
> +	if (pcie->slot_ctl_3v3)
> +		regulator_disable(pcie->slot_ctl_3v3);
> +}
> +
>  static int tegra_pcie_config_controller(struct tegra_pcie_dw *pcie,
>  					bool en_hw_hot_rst)
>  {
> @@ -1060,6 +1115,10 @@ static int tegra_pcie_config_controller(struct tegra_pcie_dw *pcie,
>  		return ret;
>  	}
>  
> +	ret = tegra_pcie_enable_slot_regulators(pcie);
> +	if (ret < 0)
> +		goto fail_slot_reg_en;
> +
>  	ret = regulator_enable(pcie->pex_ctl_supply);
>  	if (ret < 0) {
>  		dev_err(pcie->dev, "Failed to enable regulator: %d\n", ret);
> @@ -1142,6 +1201,8 @@ static int tegra_pcie_config_controller(struct tegra_pcie_dw *pcie,
>  fail_core_clk:
>  	regulator_disable(pcie->pex_ctl_supply);
>  fail_reg_en:
> +	tegra_pcie_disable_slot_regulators(pcie);
> +fail_slot_reg_en:
>  	tegra_pcie_bpmp_set_ctrl_state(pcie, false);
>  
>  	return ret;
> @@ -1174,6 +1235,8 @@ static int __deinit_controller(struct tegra_pcie_dw *pcie)
>  		return ret;
>  	}
>  
> +	tegra_pcie_disable_slot_regulators(pcie);
> +
>  	ret = tegra_pcie_bpmp_set_ctrl_state(pcie, false);
>  	if (ret) {
>  		dev_err(pcie->dev, "Failed to disable controller %d: %d\n",
> @@ -1372,6 +1435,8 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	tegra_pcie_get_slot_regulators(pcie);
> +
>  	pcie->pex_ctl_supply = devm_regulator_get(dev, "vddio-pex-ctl");
>  	if (IS_ERR(pcie->pex_ctl_supply)) {
>  		dev_err(dev, "Failed to get regulator: %ld\n",
> -- 
> 2.17.1
> 

Re: [PATCH 6/6] PCI: tegra: Add support to enable slot regulators

[Vidya Sagar]

On 8/27/2019 9:17 PM, Andrew Murray wrote:
> On Mon, Aug 26, 2019 at 01:01:43PM +0530, Vidya Sagar wrote:
>> Add support to get regulator information of 3.3V and 12V supplies of a PCIe
>> slot from the respective controller's device-tree node and enable those
>> supplies. This is required in platforms like p2972-0000 where the supplies
>> to x16 slot owned by C5 controller need to be enabled before attempting to
>> enumerate the devices.
>>
>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>> ---
>>   drivers/pci/controller/dwc/pcie-tegra194.c | 65 ++++++++++++++++++++++
>>   1 file changed, 65 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
>> index 8a27b25893c9..97de2151a738 100644
>> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
>> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
>> @@ -278,6 +278,8 @@ struct tegra_pcie_dw {
>>   	u32 aspm_l0s_enter_lat;
>>   
>>   	struct regulator *pex_ctl_supply;
>> +	struct regulator *slot_ctl_3v3;
>> +	struct regulator *slot_ctl_12v;
>>   
>>   	unsigned int phy_count;
>>   	struct phy **phys;
>> @@ -1047,6 +1049,59 @@ static void tegra_pcie_downstream_dev_to_D0(struct tegra_pcie_dw *pcie)
>>   	}
>>   }
>>   
>> +static void tegra_pcie_get_slot_regulators(struct tegra_pcie_dw *pcie)
>> +{
>> +	pcie->slot_ctl_3v3 = devm_regulator_get_optional(pcie->dev, "vpcie3v3");
>> +	if (IS_ERR(pcie->slot_ctl_3v3))
>> +		pcie->slot_ctl_3v3 = NULL;
>> +
>> +	pcie->slot_ctl_12v = devm_regulator_get_optional(pcie->dev, "vpcie12v");
>> +	if (IS_ERR(pcie->slot_ctl_12v))
>> +		pcie->slot_ctl_12v = NULL;
> 
> Do these need to take into consideration -EPROBE_DEFER?
Since these are devm_* APIs, isn't it taken care of automatically?

> 
> Thanks,
> 
> Andrew Murray
> 
>> +}
>> +
>> +static int tegra_pcie_enable_slot_regulators(struct tegra_pcie_dw *pcie)
>> +{
>> +	int ret;
>> +
>> +	if (pcie->slot_ctl_3v3) {
>> +		ret = regulator_enable(pcie->slot_ctl_3v3);
>> +		if (ret < 0) {
>> +			dev_err(pcie->dev,
>> +				"Failed to enable 3V3 slot supply: %d\n", ret);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	if (pcie->slot_ctl_12v) {
>> +		ret = regulator_enable(pcie->slot_ctl_12v);
>> +		if (ret < 0) {
>> +			dev_err(pcie->dev,
>> +				"Failed to enable 12V slot supply: %d\n", ret);
>> +			if (pcie->slot_ctl_3v3)
>> +				regulator_disable(pcie->slot_ctl_3v3);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	/*
>> +	 * According to PCI Express Card Electromechanical Specification
>> +	 * Revision 1.1, Table-2.4, T_PVPERL (Power stable to PERST# inactive)
>> +	 * should be a minimum of 100ms.
>> +	 */
>> +	msleep(100);
>> +
>> +	return 0;
>> +}
>> +
>> +static void tegra_pcie_disable_slot_regulators(struct tegra_pcie_dw *pcie)
>> +{
>> +	if (pcie->slot_ctl_12v)
>> +		regulator_disable(pcie->slot_ctl_12v);
>> +	if (pcie->slot_ctl_3v3)
>> +		regulator_disable(pcie->slot_ctl_3v3);
>> +}
>> +
>>   static int tegra_pcie_config_controller(struct tegra_pcie_dw *pcie,
>>   					bool en_hw_hot_rst)
>>   {
>> @@ -1060,6 +1115,10 @@ static int tegra_pcie_config_controller(struct tegra_pcie_dw *pcie,
>>   		return ret;
>>   	}
>>   
>> +	ret = tegra_pcie_enable_slot_regulators(pcie);
>> +	if (ret < 0)
>> +		goto fail_slot_reg_en;
>> +
>>   	ret = regulator_enable(pcie->pex_ctl_supply);
>>   	if (ret < 0) {
>>   		dev_err(pcie->dev, "Failed to enable regulator: %d\n", ret);
>> @@ -1142,6 +1201,8 @@ static int tegra_pcie_config_controller(struct tegra_pcie_dw *pcie,
>>   fail_core_clk:
>>   	regulator_disable(pcie->pex_ctl_supply);
>>   fail_reg_en:
>> +	tegra_pcie_disable_slot_regulators(pcie);
>> +fail_slot_reg_en:
>>   	tegra_pcie_bpmp_set_ctrl_state(pcie, false);
>>   
>>   	return ret;
>> @@ -1174,6 +1235,8 @@ static int __deinit_controller(struct tegra_pcie_dw *pcie)
>>   		return ret;
>>   	}
>>   
>> +	tegra_pcie_disable_slot_regulators(pcie);
>> +
>>   	ret = tegra_pcie_bpmp_set_ctrl_state(pcie, false);
>>   	if (ret) {
>>   		dev_err(pcie->dev, "Failed to disable controller %d: %d\n",
>> @@ -1372,6 +1435,8 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev)
>>   		return ret;
>>   	}
>>   
>> +	tegra_pcie_get_slot_regulators(pcie);
>> +
>>   	pcie->pex_ctl_supply = devm_regulator_get(dev, "vddio-pex-ctl");
>>   	if (IS_ERR(pcie->pex_ctl_supply)) {
>>   		dev_err(dev, "Failed to get regulator: %ld\n",
>> -- 
>> 2.17.1
>>

Re: [PATCH 6/6] PCI: tegra: Add support to enable slot regulators

[Andrew Murray]

On Tue, Aug 27, 2019 at 09:54:17PM +0530, Vidya Sagar wrote:
> On 8/27/2019 9:17 PM, Andrew Murray wrote:
> > On Mon, Aug 26, 2019 at 01:01:43PM +0530, Vidya Sagar wrote:
> > > Add support to get regulator information of 3.3V and 12V supplies of a PCIe
> > > slot from the respective controller's device-tree node and enable those
> > > supplies. This is required in platforms like p2972-0000 where the supplies
> > > to x16 slot owned by C5 controller need to be enabled before attempting to
> > > enumerate the devices.
> > > 
> > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> > > ---
> > >   drivers/pci/controller/dwc/pcie-tegra194.c | 65 ++++++++++++++++++++++
> > >   1 file changed, 65 insertions(+)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> > > index 8a27b25893c9..97de2151a738 100644
> > > --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> > > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> > > @@ -278,6 +278,8 @@ struct tegra_pcie_dw {
> > >   	u32 aspm_l0s_enter_lat;
> > >   	struct regulator *pex_ctl_supply;
> > > +	struct regulator *slot_ctl_3v3;
> > > +	struct regulator *slot_ctl_12v;
> > >   	unsigned int phy_count;
> > >   	struct phy **phys;
> > > @@ -1047,6 +1049,59 @@ static void tegra_pcie_downstream_dev_to_D0(struct tegra_pcie_dw *pcie)
> > >   	}
> > >   }
> > > +static void tegra_pcie_get_slot_regulators(struct tegra_pcie_dw *pcie)
> > > +{
> > > +	pcie->slot_ctl_3v3 = devm_regulator_get_optional(pcie->dev, "vpcie3v3");
> > > +	if (IS_ERR(pcie->slot_ctl_3v3))
> > > +		pcie->slot_ctl_3v3 = NULL;
> > > +
> > > +	pcie->slot_ctl_12v = devm_regulator_get_optional(pcie->dev, "vpcie12v");
> > > +	if (IS_ERR(pcie->slot_ctl_12v))
> > > +		pcie->slot_ctl_12v = NULL;
> > 
> > Do these need to take into consideration -EPROBE_DEFER?
> Since these are devm_* APIs, isn't it taken care of automatically?

devm_regulator_get_optional can still return -EPROBE_DEFER - for times when
"lookup could succeed in the future".

It's probably helpful here for your driver to distinguish between there not
being a regulator specified in the DT, and there being a regulator but there
is no device for it yet. For the latter case - your driver would probe but
nothing would enumerate.

See pcie-rockchip-host.c for an example of where this is handled.

Of course if, for whatever reason it is unlikely you'll ever get -EPROBE_DEFER
then maybe it's OK as it is.

Thanks,

Andrew Murray

> 
> > 
> > Thanks,
> > 
> > Andrew Murray
> > 
> > > +}
> > > +
> > > +static int tegra_pcie_enable_slot_regulators(struct tegra_pcie_dw *pcie)
> > > +{
> > > +	int ret;
> > > +
> > > +	if (pcie->slot_ctl_3v3) {
> > > +		ret = regulator_enable(pcie->slot_ctl_3v3);
> > > +		if (ret < 0) {
> > > +			dev_err(pcie->dev,
> > > +				"Failed to enable 3V3 slot supply: %d\n", ret);
> > > +			return ret;
> > > +		}
> > > +	}
> > > +
> > > +	if (pcie->slot_ctl_12v) {
> > > +		ret = regulator_enable(pcie->slot_ctl_12v);
> > > +		if (ret < 0) {
> > > +			dev_err(pcie->dev,
> > > +				"Failed to enable 12V slot supply: %d\n", ret);
> > > +			if (pcie->slot_ctl_3v3)
> > > +				regulator_disable(pcie->slot_ctl_3v3);
> > > +			return ret;
> > > +		}
> > > +	}
> > > +
> > > +	/*
> > > +	 * According to PCI Express Card Electromechanical Specification
> > > +	 * Revision 1.1, Table-2.4, T_PVPERL (Power stable to PERST# inactive)
> > > +	 * should be a minimum of 100ms.
> > > +	 */
> > > +	msleep(100);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void tegra_pcie_disable_slot_regulators(struct tegra_pcie_dw *pcie)
> > > +{
> > > +	if (pcie->slot_ctl_12v)
> > > +		regulator_disable(pcie->slot_ctl_12v);
> > > +	if (pcie->slot_ctl_3v3)
> > > +		regulator_disable(pcie->slot_ctl_3v3);
> > > +}
> > > +
> > >   static int tegra_pcie_config_controller(struct tegra_pcie_dw *pcie,
> > >   					bool en_hw_hot_rst)
> > >   {
> > > @@ -1060,6 +1115,10 @@ static int tegra_pcie_config_controller(struct tegra_pcie_dw *pcie,
> > >   		return ret;
> > >   	}
> > > +	ret = tegra_pcie_enable_slot_regulators(pcie);
> > > +	if (ret < 0)
> > > +		goto fail_slot_reg_en;
> > > +
> > >   	ret = regulator_enable(pcie->pex_ctl_supply);
> > >   	if (ret < 0) {
> > >   		dev_err(pcie->dev, "Failed to enable regulator: %d\n", ret);
> > > @@ -1142,6 +1201,8 @@ static int tegra_pcie_config_controller(struct tegra_pcie_dw *pcie,
> > >   fail_core_clk:
> > >   	regulator_disable(pcie->pex_ctl_supply);
> > >   fail_reg_en:
> > > +	tegra_pcie_disable_slot_regulators(pcie);
> > > +fail_slot_reg_en:
> > >   	tegra_pcie_bpmp_set_ctrl_state(pcie, false);
> > >   	return ret;
> > > @@ -1174,6 +1235,8 @@ static int __deinit_controller(struct tegra_pcie_dw *pcie)
> > >   		return ret;
> > >   	}
> > > +	tegra_pcie_disable_slot_regulators(pcie);
> > > +
> > >   	ret = tegra_pcie_bpmp_set_ctrl_state(pcie, false);
> > >   	if (ret) {
> > >   		dev_err(pcie->dev, "Failed to disable controller %d: %d\n",
> > > @@ -1372,6 +1435,8 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev)
> > >   		return ret;
> > >   	}
> > > +	tegra_pcie_get_slot_regulators(pcie);
> > > +
> > >   	pcie->pex_ctl_supply = devm_regulator_get(dev, "vddio-pex-ctl");
> > >   	if (IS_ERR(pcie->pex_ctl_supply)) {
> > >   		dev_err(dev, "Failed to get regulator: %ld\n",
> > > -- 
> > > 2.17.1
> > > 
> 

Re: [PATCH 6/6] PCI: tegra: Add support to enable slot regulators

[Thierry Reding]

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

On Tue, Aug 27, 2019 at 06:13:34PM +0100, Andrew Murray wrote:
> On Tue, Aug 27, 2019 at 09:54:17PM +0530, Vidya Sagar wrote:
> > On 8/27/2019 9:17 PM, Andrew Murray wrote:
> > > On Mon, Aug 26, 2019 at 01:01:43PM +0530, Vidya Sagar wrote:
> > > > Add support to get regulator information of 3.3V and 12V supplies of a PCIe
> > > > slot from the respective controller's device-tree node and enable those
> > > > supplies. This is required in platforms like p2972-0000 where the supplies
> > > > to x16 slot owned by C5 controller need to be enabled before attempting to
> > > > enumerate the devices.
> > > > 
> > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> > > > ---
> > > >   drivers/pci/controller/dwc/pcie-tegra194.c | 65 ++++++++++++++++++++++
> > > >   1 file changed, 65 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> > > > index 8a27b25893c9..97de2151a738 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> > > > @@ -278,6 +278,8 @@ struct tegra_pcie_dw {
> > > >   	u32 aspm_l0s_enter_lat;
> > > >   	struct regulator *pex_ctl_supply;
> > > > +	struct regulator *slot_ctl_3v3;
> > > > +	struct regulator *slot_ctl_12v;
> > > >   	unsigned int phy_count;
> > > >   	struct phy **phys;
> > > > @@ -1047,6 +1049,59 @@ static void tegra_pcie_downstream_dev_to_D0(struct tegra_pcie_dw *pcie)
> > > >   	}
> > > >   }
> > > > +static void tegra_pcie_get_slot_regulators(struct tegra_pcie_dw *pcie)
> > > > +{
> > > > +	pcie->slot_ctl_3v3 = devm_regulator_get_optional(pcie->dev, "vpcie3v3");
> > > > +	if (IS_ERR(pcie->slot_ctl_3v3))
> > > > +		pcie->slot_ctl_3v3 = NULL;
> > > > +
> > > > +	pcie->slot_ctl_12v = devm_regulator_get_optional(pcie->dev, "vpcie12v");
> > > > +	if (IS_ERR(pcie->slot_ctl_12v))
> > > > +		pcie->slot_ctl_12v = NULL;
> > > 
> > > Do these need to take into consideration -EPROBE_DEFER?
> > Since these are devm_* APIs, isn't it taken care of automatically?
> 
> devm_regulator_get_optional can still return -EPROBE_DEFER - for times when
> "lookup could succeed in the future".
> 
> It's probably helpful here for your driver to distinguish between there not
> being a regulator specified in the DT, and there being a regulator but there
> is no device for it yet. For the latter case - your driver would probe but
> nothing would enumerate.
> 
> See pcie-rockchip-host.c for an example of where this is handled.
> 
> Of course if, for whatever reason it is unlikely you'll ever get -EPROBE_DEFER
> then maybe it's OK as it is.

Let's not assume that. We've just recently encountered a case where we
did not handle -EPROBE_DEFER because we had assumed too much, and that
turned into a bit of a hassle to fix.

Vidya, I think what Andrew is saying is that you need to propagate the
-EPROBE_DEFER error to the caller (i.e. the ->probe() callback) so that
the PCI controller driver can be properly added to the defer queue in
case the regulator isn't ready yet.

I think what we want here is something like:

	pcie->slot_ctl_3v3 = devm_regulator_get_optional(pcie->dev, "vpcie3v3");
	if (IS_ERR(pcie->slot_ctl_3v3)) {
		if (PTR_ERR(pcie->slot_ctl_3v3) != -ENODEV)
			return PTR_ERR(pcie->slot_ctl_3v3);

		pcie->slot_ctl_3v3 = NULL;
	}

Andrew, I'm not sure the handling in rockchip_pcie_parse_host_dt() is
correct. It singles out -EPROBE_DEFER, which I think is the wrong way
around. We should be special-casing -ENODEV, because regulator_get()
can return a wide array of error cases, not all of which we actually
want to consider successes. For example we could be getting -ENOMEM,
which, I would argue, is something that we should propagate. I think
it'd be very confusing to take that as meaning "optional regulator
wasn't specified", because in that case the DTS file would've had the
regulator hooked up (we have to assume that it is needed in that case)
but we won't be enabling it, so it's unlikely that devices will
enumerate.

Thierry

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

Re: [PATCH 0/6] PCI: tegra: Enable PCIe C5 controller of Tegra194 in p2972-0000 platform

[Thierry Reding]

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

On Mon, Aug 26, 2019 at 01:01:37PM +0530, Vidya Sagar wrote:
> This patch series enables Tegra194's C5 controller which owns x16 slot in
> p2972-0000 platform. C5 controller's PERST# and CLKREQ# are not configured as
> output and bi-directional signals by default and hence they need to be
> configured explicitly. Also, x16 slot's 3.3V and 12V supplies are controlled
> through GPIOs and hence they need to be enabled through regulator framework.
> This patch series adds required infrastructural support to address both the
> aforementioned requirements.
> Testing done on p2972-0000 platform
> - Able to enumerate devices connected to x16 slot (owned by C5 controller)
> - Enumerated device's functionality verified
> - Suspend-Resume sequence is verified with device connected to x16 slot
> 
> Vidya Sagar (6):
>   dt-bindings: PCI: tegra: Add sideband pins configuration entries
>   arm64: tegra: Add configuration for PCIe C5 sideband signals
>   PCI: tegra: Add support to configure sideband pins
>   dt-bindings: PCI: tegra: Add PCIe slot supplies regulator entries
>   arm64: tegra: Add PCIe slot supply information in p2972-0000 platform
>   PCI: tegra: Add support to enable slot regulators

Hi Vidya,

when you resend with review comments addressed, can you please reorder
the patches slightly? I think it's more natural to order them like this:

    dt-bindings: PCI: tegra: Add sideband pins configuration entries
    PCI: tegra: Add support to configure sideband pins
    dt-bindings: PCI: tegra: Add PCIe slot supplies regulator entries
    PCI: tegra: Add support to enable slot regulators
    arm64: tegra: Add configuration for PCIe C5 sideband signals
    arm64: tegra: Add PCIe slot supply information in p2972-0000 platform

Or perhaps even like this:

    dt-bindings: PCI: tegra: Add sideband pins configuration entries
    dt-bindings: PCI: tegra: Add PCIe slot supplies regulator entries
    PCI: tegra: Add support to configure sideband pins
    PCI: tegra: Add support to enable slot regulators
    arm64: tegra: Add configuration for PCIe C5 sideband signals
    arm64: tegra: Add PCIe slot supply information in p2972-0000 platform

That makes it more obvious that patches 1-2 need an Acked-by from Rob
and patches 1-4 need to go through Lorenzo's tree and that I'll pick up
patches 5-6.

Thierry

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

Re: [PATCH 6/6] PCI: tegra: Add support to enable slot regulators

[Andrew Murray]

On Wed, Aug 28, 2019 at 11:07:57AM +0200, Thierry Reding wrote:
> On Tue, Aug 27, 2019 at 06:13:34PM +0100, Andrew Murray wrote:
> > On Tue, Aug 27, 2019 at 09:54:17PM +0530, Vidya Sagar wrote:
> > > On 8/27/2019 9:17 PM, Andrew Murray wrote:
> > > > On Mon, Aug 26, 2019 at 01:01:43PM +0530, Vidya Sagar wrote:
> > > > > Add support to get regulator information of 3.3V and 12V supplies of a PCIe
> > > > > slot from the respective controller's device-tree node and enable those
> > > > > supplies. This is required in platforms like p2972-0000 where the supplies
> > > > > to x16 slot owned by C5 controller need to be enabled before attempting to
> > > > > enumerate the devices.
> > > > > 
> > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> > > > > ---
> > > > >   drivers/pci/controller/dwc/pcie-tegra194.c | 65 ++++++++++++++++++++++
> > > > >   1 file changed, 65 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> > > > > index 8a27b25893c9..97de2151a738 100644
> > > > > --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> > > > > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> > > > > @@ -278,6 +278,8 @@ struct tegra_pcie_dw {
> > > > >   	u32 aspm_l0s_enter_lat;
> > > > >   	struct regulator *pex_ctl_supply;
> > > > > +	struct regulator *slot_ctl_3v3;
> > > > > +	struct regulator *slot_ctl_12v;
> > > > >   	unsigned int phy_count;
> > > > >   	struct phy **phys;
> > > > > @@ -1047,6 +1049,59 @@ static void tegra_pcie_downstream_dev_to_D0(struct tegra_pcie_dw *pcie)
> > > > >   	}
> > > > >   }
> > > > > +static void tegra_pcie_get_slot_regulators(struct tegra_pcie_dw *pcie)
> > > > > +{
> > > > > +	pcie->slot_ctl_3v3 = devm_regulator_get_optional(pcie->dev, "vpcie3v3");
> > > > > +	if (IS_ERR(pcie->slot_ctl_3v3))
> > > > > +		pcie->slot_ctl_3v3 = NULL;
> > > > > +
> > > > > +	pcie->slot_ctl_12v = devm_regulator_get_optional(pcie->dev, "vpcie12v");
> > > > > +	if (IS_ERR(pcie->slot_ctl_12v))
> > > > > +		pcie->slot_ctl_12v = NULL;
> > > > 
> > > > Do these need to take into consideration -EPROBE_DEFER?
> > > Since these are devm_* APIs, isn't it taken care of automatically?
> > 
> > devm_regulator_get_optional can still return -EPROBE_DEFER - for times when
> > "lookup could succeed in the future".
> > 
> > It's probably helpful here for your driver to distinguish between there not
> > being a regulator specified in the DT, and there being a regulator but there
> > is no device for it yet. For the latter case - your driver would probe but
> > nothing would enumerate.
> > 
> > See pcie-rockchip-host.c for an example of where this is handled.
> > 
> > Of course if, for whatever reason it is unlikely you'll ever get -EPROBE_DEFER
> > then maybe it's OK as it is.
> 
> Let's not assume that. We've just recently encountered a case where we
> did not handle -EPROBE_DEFER because we had assumed too much, and that
> turned into a bit of a hassle to fix.
> 
> Vidya, I think what Andrew is saying is that you need to propagate the
> -EPROBE_DEFER error to the caller (i.e. the ->probe() callback) so that
> the PCI controller driver can be properly added to the defer queue in
> case the regulator isn't ready yet.

Indeed.

> 
> I think what we want here is something like:
> 
> 	pcie->slot_ctl_3v3 = devm_regulator_get_optional(pcie->dev, "vpcie3v3");
> 	if (IS_ERR(pcie->slot_ctl_3v3)) {
> 		if (PTR_ERR(pcie->slot_ctl_3v3) != -ENODEV)
> 			return PTR_ERR(pcie->slot_ctl_3v3);
> 
> 		pcie->slot_ctl_3v3 = NULL;
> 	}
> 
> Andrew, I'm not sure the handling in rockchip_pcie_parse_host_dt() is
> correct. It singles out -EPROBE_DEFER, which I think is the wrong way
> around. We should be special-casing -ENODEV, because regulator_get()
> can return a wide array of error cases, not all of which we actually
> want to consider successes. For example we could be getting -ENOMEM,
> which, I would argue, is something that we should propagate.

Yes I completely agree, given that the regulator is optional: we only want
to proceed if we find the regulator or if a regulator wasn't specified in the
DT. We should fail upon any other error, in case a regulator was specified
but the error prevented it from being returned.

> I think
> it'd be very confusing to take that as meaning "optional regulator
> wasn't specified", because in that case the DTS file would've had the
> regulator hooked up (we have to assume that it is needed in that case)
> but we won't be enabling it, so it's unlikely that devices will
> enumerate.

Thanks,

Andrew Murray

> 
> Thierry

Re: [PATCH 0/6] PCI: tegra: Enable PCIe C5 controller of Tegra194 in p2972-0000 platform

[Vidya Sagar]

On 8/28/2019 2:40 PM, Thierry Reding wrote:
> On Mon, Aug 26, 2019 at 01:01:37PM +0530, Vidya Sagar wrote:
>> This patch series enables Tegra194's C5 controller which owns x16 slot in
>> p2972-0000 platform. C5 controller's PERST# and CLKREQ# are not configured as
>> output and bi-directional signals by default and hence they need to be
>> configured explicitly. Also, x16 slot's 3.3V and 12V supplies are controlled
>> through GPIOs and hence they need to be enabled through regulator framework.
>> This patch series adds required infrastructural support to address both the
>> aforementioned requirements.
>> Testing done on p2972-0000 platform
>> - Able to enumerate devices connected to x16 slot (owned by C5 controller)
>> - Enumerated device's functionality verified
>> - Suspend-Resume sequence is verified with device connected to x16 slot
>>
>> Vidya Sagar (6):
>>    dt-bindings: PCI: tegra: Add sideband pins configuration entries
>>    arm64: tegra: Add configuration for PCIe C5 sideband signals
>>    PCI: tegra: Add support to configure sideband pins
>>    dt-bindings: PCI: tegra: Add PCIe slot supplies regulator entries
>>    arm64: tegra: Add PCIe slot supply information in p2972-0000 platform
>>    PCI: tegra: Add support to enable slot regulators
> 
> Hi Vidya,
> 
> when you resend with review comments addressed, can you please reorder
> the patches slightly? I think it's more natural to order them like this:
> 
>      dt-bindings: PCI: tegra: Add sideband pins configuration entries
>      PCI: tegra: Add support to configure sideband pins
>      dt-bindings: PCI: tegra: Add PCIe slot supplies regulator entries
>      PCI: tegra: Add support to enable slot regulators
>      arm64: tegra: Add configuration for PCIe C5 sideband signals
>      arm64: tegra: Add PCIe slot supply information in p2972-0000 platform
> 
> Or perhaps even like this:
> 
>      dt-bindings: PCI: tegra: Add sideband pins configuration entries
>      dt-bindings: PCI: tegra: Add PCIe slot supplies regulator entries
>      PCI: tegra: Add support to configure sideband pins
>      PCI: tegra: Add support to enable slot regulators
>      arm64: tegra: Add configuration for PCIe C5 sideband signals
>      arm64: tegra: Add PCIe slot supply information in p2972-0000 platform
> 
> That makes it more obvious that patches 1-2 need an Acked-by from Rob
> and patches 1-4 need to go through Lorenzo's tree and that I'll pick up
> patches 5-6.Sure.
I'll do that.
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nvidia.com; s=n1;
	t=1566986650; bh=fKSn9tEu0u/7kvehjleUQGVzZJJHx0MojZ9eQ2aJqWM=;
	h=X-PGP-Universal:Subject:To:CC:References:X-Nvconfidentiality:From:
	 Message-ID:Date:User-Agent:MIME-Version:In-Reply-To:
	 X-Originating-IP:X-ClientProxiedBy:Content-Type:Content-Language:
	 Content-Transfer-Encoding;
	b=aSDWQiNYeJefaP3hT1F2Kc9CSOb6AJPVC1NOPnn4rLtaj7EOJb+j70r/h4+I3bKEk
	 N6siO4F51Zz49OC3S0601fPefcag3AJZZQEdvrpk3pIhqGZ+62QMXTeG4f7t/ETsLY
	 TPYf5ksG6Kb8vxeUEAY6ATiHOIhjJhbLjRn/uJ/Oat+8ZRv3VgTOtujSHpzsKf1rTz
	 gJZbRa8GsOlGBEvMoc7xxgRDx1/GzUqaHw8T8GqLV/JDOW9qNy44TVpHkk4Exc7g5+
	 bM5zCyghflgpfG+ZMzbwZEkd1b4gDyPfxYL2J+AzEjgQyVsBSU6+nXSjJg65VfzSd6
	 bM8cWEfkmyeMA==

Thanks,
Vidya Sagar

> 
> Thierry
>